Skip to content

fix: Claude credit failure notice, model aliases, CLI rustls provider - #2512

Open
Chessing234 wants to merge 4 commits into
block:mainfrom
Chessing234:fix/credits-notice-cli-tls
Open

Chessing234 wants to merge 4 commits into
block:mainfrom
Chessing234:fix/credits-notice-cli-tls

Conversation

@Chessing234

@Chessing234 Chessing234 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Summary

Review follow-up

The model matching rule now implements the conservative boundary requested in review. The malformed function boundary preceding extract_thought_level_config_id is also repaired.

Validation

  • 22 focused Rust tests pass in an isolated harness containing the actual model resolver/extractor/helper source and its repository tests, using serde/serde_json. Coverage includes all four moving families in both directions, stable and unstable catalogs, cached guards, advertised values, pinned IDs, case differences, incompatible composer fast variants, exact priority, and ambiguous qualifiers.
  • The two new behavior regressions fail with the previous matcher (after repairing its missing closing brace solely to permit the comparison).
  • Rustfmt on the complete changed Rust file and git diff --check pass using the available stable toolchain.
  • Required just ci was attempted after activating Hermit, but its pinned just download failed resolving github.com. The full workspace build, clippy and integrated suite have not passed locally; the isolated harness is not a substitute for that gate.

Runtime checks still outstanding

  • Credits-required model posts an immediate failure notice in-channel.
  • BUZZ_ACP_MODEL=opus[1m] with an adapter advertising opus sends the advertised opus value.
  • Workspace-built secure-relay CLI operation completes without a CryptoProvider panic.

Closes #2265
Closes #2308
Closes #2329

Dead-letter non-retryable application errors with a channel notice, resolve short model aliases like sonnet, and map credit errors in the Agents UI.

Signed-off-by: Taksh <takshkothari09@gmail.com>

Signed-off-by: Taksh <takshkothari09@gmail.com>
Workspace builds enable both ring and aws-lc-rs; without an entrypoint install, agents draft-create panics on secure relays.

Signed-off-by: Taksh <takshkothari09@gmail.com>

Signed-off-by: Taksh <takshkothari09@gmail.com>
@Chessing234

Copy link
Copy Markdown
Contributor Author

@tlongwell-block @wesbillman @wpfleger96 mind taking a look when you get a chance?

@Chessing234
Chessing234 force-pushed the fix/credits-notice-cli-tls branch from 5a7c295 to b177393 Compare August 5, 2026 10:38
Signed-off-by: Taksh <takshkothari09@gmail.com>
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

🔐 Codex Security Review

Status: review required for the current range.

The current range is 78618804ec86a014524ad7d1fb55928e8f5c3edf...4abaa8ef0e21a57c343c42893e22dc0133dde22e.
A new review must complete for this exact range. When manual authorization
is required, a Block organization member must comment exactly
@buzz-security-review 4abaa8ef0e21a57c343c42893e22dc0133dde22e to authorize a new review.
Any previous review applies only to its recorded range.

@nathansmithopenclaw-alt

Copy link
Copy Markdown

The alias slice here looks useful, but I think its current matching rule is too
broad for model selection.

Buzz already has a concrete case where two bracket variants are materially
different: composer-2.5[fast=false] versus the advertised
composer-2.5[fast=true] (#4004). A case-insensitive or hyphen-segment matcher
also makes a short family name select a full dated model ID, which turns catalog
drift into a model choice rather than an explicit unsupported result.

Could we narrow this to the known moving-family compatibility boundary?

  • exact, case-sensitive IDs always win;
  • only opus, fable, sonnet, and haiku may match the same bare family
    with one bracket qualifier in either direction;
  • full/pinned IDs and all other bracket variants remain exact; and
  • the adapter's advertised value is sent on the wire.

The same helper should gate both fresh-session resolution and the cached
idle-switch check. Tests for opus[1m]opus, a pinned Claude ID ↛ opus,
and fast=falsefast=true would pin the safety boundary.

This could also be split from the credit-notice and rustls changes so the model
behavior can be reviewed independently.

Signed-off-by: Taksh <takshkothari09@gmail.com>
@Chessing234

Copy link
Copy Markdown
Contributor Author

@nathansmithopenclaw-alt Addressed the matching boundary in 4abaa8e: exact case-sensitive IDs win; only the four bare moving families accept one bracket qualifier in either direction. Fresh resolution and the cached guard share the matcher and preserve the adapter's advertised value. All 22 focused actual-source Rust tests pass, including pinned IDs and incompatible bracket variants. The full workspace CI gate remains unverified locally because Hermit's pinned tool download failed; that limitation is in the body. Could you re-review the update?

@nathansmithopenclaw-alt

Copy link
Copy Markdown

Thanks — the matching boundary now reads exactly as intended, and I checked it against the
real crate rather than the description.

Confirmed at 4abaa8ef:

  • exact, case-sensitive IDs win from anywhere in the option list (pick_matching_model_id
    returns on MatchKind::Exact regardless of position);
  • only the four bare moving families take one bracket qualifier in either direction, and
    opus[1m][fast=true] / opus[] correctly fall out;
  • composer-2.5[fast=false]composer-2.5[fast=true] holds, which was the [Bug] Agent model composer-2.5[fast=false] set in Desktop but ACP session bills/runs as fast=true #4004 case;
  • the adapter's advertised value is what goes on the wire — apply_model_switch sends the
    resolved option_value/model_id, with desired only in the log line;
  • fresh resolution and the cached idle-switch guard both route through the one helper.

The three new tests pass in-crate, not just in the isolated harness.

One blocker, in the other slice

cargo test -p buzz-acp at 4abaa8ef is red on one pre-existing test:

---- error_outcome_emission_tests::non_auth_application_error_is_requeued stdout ----
assertion `left == right` failed: non-auth application error must requeue the batch for retry
  left: 0
 right: 1

test result: FAILED. 901 passed; 1 failed

Control run on 59328d5ae (the upstream-main parent of b902c417d), same toolchain
(1.95.0) and same environment: ok. 899 passed; 0 failed. 899 + the 3 new tests = 902,
so the arithmetic closes and the failure is introduced by this branch.

The test's fixture is AgentError { code: -32000, message: "Usage credits required for 1M context" } and it asserts the batch is requeued. is_non_retryable_application_error
matches "usage credits", so it dead-letters instead.

That test came in with #2751 (0a9c26ee8, "dead-letter auth errors immediately with re-auth
hint"), which picked the usage-credits message deliberately as the canonical retryable
application error to fence the auth dead-letter path. So this isn't a stale assertion to
update in passing — #2265 and #2751 disagree about whether a credits failure keeps its retry
budget, and a maintainer should settle that before either behaviour ships. It stayed
invisible because the first-time-contributor gate has kept CI from running on this branch.

Smaller notes on the same arm

  1. The new else if let PromptOutcome::Error(ref err) block re-implements the generic
    requeue path in its else, which makes PromptOutcome::Error(e) => format!("{e}") in
    the final arm unreachable. The arm just above it uses
    } else if matches!(&result.outcome, PromptOutcome::Error(e) if is_auth_error(e)) {
    matching that shape keeps the requeue path single-sourced.

  2. is_auth_error inspects only AcpError::AgentError and only "Re-authenticate" /
    "API Error: 401". The new predicate matches Display across every variant and adds
    invalid api key, authentication_error, incorrect api key, and unauthorized+api.
    Net effect: an "invalid API key" failure gets the new generic "fix the configuration"
    copy rather than the specific re-authenticate copy directly above it, and a transport-class
    error whose text contains unauthorized + api is dead-lettered rather than retried.
    Extending is_auth_error with those substrings and keeping the new predicate
    credits-only would give each class one home and one copy.

  3. Minor, model slice: resolve_model_switch_method applies the "exactly one alias" rule
    per config option and returns on the first hit, while model_in_catalog flattens all
    model-category options into one candidate list first. extract_model_config_options
    returns a Vec, so with option A = ["opus[1m]"], option B = ["opus[fast=true]"] and
    desired opus, the idle-switch guard reports unsupported while fresh resolution switches
    to opus[1m]. Flattening the same way in both closes it.

cargo clippy -p buzz-acp -p buzz-cli --all-targets -- -D warnings is clean at this head.

Given the model slice is independently correct and the credits question needs a decision,
splitting them would let this land now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants