fix(review): print the provider failure beneath "request failed"; classify it in the review workflow

[Hmbown]

Summary

The Codewhale review bot has failed on every non-draft run since 752bae3 (2026-09-21 00:01 UTC, workflow runs 729 through 750) with one bare line:

error: Review pass 1/1 request failed: Responses API request failed; no partial review was accepted or posted; publication: not_attempted; completed review passes: 0/1; accumulated usage: {"input_tokens":0,"output_tokens":0}

The last runs that completed their DeepSeek request were 710 (2026-09-19 04:00 UTC, tree 919d602, review posted) and 713 (04:38 UTC, tree 041e628, request completed with 47k input tokens, then failed on the output budget). Between 041e628 and 752bae3 the request path is byte-identical: crates/tui/src/client/ (Responses handler, stream entry, prepared request) is unchanged, client.rs changed only for StepFun, Cargo.lock changed only workspace version numbers, and codewhale-review.yml is the same file. So the failure was provider-side, and the message hid which class.

Real cause, confirmed by this PR's own review run on bcbf690: LLM error: HTTP 402: Insufficient Balance. The DeepSeek review key is out of funds. That is a maintainer action (fund or rotate the key), not a code defect; this PR makes the failure say so and keeps it advisory.

Why the line was bare. The client wraps the retry loop's LlmError in .context("Responses API request failed"), and both review paths formatted the failure with {error}, which prints only the outermost anyhow layer. The LlmError underneath names the class (quota, auth, rate limit, upstream 5xx, network, timeout) and carries the sanitized provider body. The sub-agent failure receipt already renders the chain with the alternate format for exactly this masking (#3884).

Four commits.

1. 4598534 rendered the chain in the agent-callable ReviewTool and widened the workflow classifier.
2. bcbf690 (Devin finding): codewhale review, the path the workflow runs, dispatches to run_review in the crate root, which formats its own request failure and was still masked. Both paths now build the line through one request_failure_message helper beside the add_review_usage they already share, and a regression test pins that a wrapped LlmError keeps its class and body beneath the outer context.
3. 32b9256 (Devin finding): the classifier searched the whole output, so provider body text deeper in the chain could impersonate a class. The class is now anchored to its position: right after Review pass N/M request failed: and the client's one optional <wire> API request failed: layer.
4. 35e0b84: braces the pattern expansion for shellcheck (SC1087); no behaviour change.

Why the job failed instead of posting the non-run note. The workflow's classifier only matched the legacy LLM error: HTTP <status> form, which LlmError's Display produces only for statuses without a class of their own (402 without quota wording, 408). It now also matches the class prefixes (Provider plan quota exhausted, Authentication failed, Authorization failed, Rate limit exceeded, Server error (NNN), Network error, Request timed out) at the anchored position, so a funding, key, rate-limit, upstream or network failure posts the advisory non-run note and exits 0, while an Invalid request (400) or an exhausted output budget still fails the job as a real review failure. On bcbf690 and again on 35e0b84 the run posted exactly that note and the check went green.

No-Issue: review-bot outage diagnosis before the v0.10.0 tag; a shared one-line formatter, its test, and the workflow classifier.

Testing

rustc 1.98.1, tui lib test build with --all-features --locked, hermetic test HOME, default 2 MiB test stack:

tools::review::tests::request_failure_message_keeps_the_provider_failure_beneath_the_context   test result: ok. 1 passed; 0 failed
tools::review::tests::                                                                           test result: ok. 35 passed; 0 failed

Regression test shown failing without the fix (rebuilt with the helper neutralized to {error}; call sites and test kept):

request_failure_message_keeps_the_provider_failure_beneath_the_context   test result: FAILED. 0 passed; 1 failed
  assertion `left == right` failed  (crates/tui/src/tools/review.rs:1986)

• cargo fmt --all -- --check: clean.

• Non-test cargo check -p codewhale-tui --lib --locked: 0 warnings, exit 0.

• Classifier exercised in bash against ten sample lines: the masked line, three impersonation lines (400 and model-error bodies mentioning a class), the budget-exhausted and partial-review lines all still fail the job; the chained quota, 5xx, 402 and network lines, with and without the wire layer, classify as non-run.

• The workflow YAML parses; Workflow lint (actionlint + shellcheck) passes on 35e0b84.

• Live: the Codewhale review run on bcbf690 printed LLM error: HTTP 402: Insufficient Balance, posted the non-run note, and exited 0; the anchored classifier on 35e0b84 updated the note with the anchored reason and exited 0.

• Version drift went red once on 4598534 in the release test script's temp-dir cleanup (rm: cannot remove ... Directory not empty), outside this PR's files; it passed on the single re-run. The script fix is queued separately.

• cargo fmt --all -- --check

• Non-test cargo check -p codewhale-tui --lib --locked

• Targeted tests and the proof without the fix (above)

• Codewhale review run shows the underlying provider failure (402 on bcbf690)

• Workflow lint, Lint, Test ubuntu / macos / windows — all 30 checks green on 35e0b84

Checklist

• This PR adds a new layer/module/abstraction — it names or deletes the layer it replaces (no new layer; one shared helper replaces two format strings)
• Updated docs or comments as needed
• Added or updated tests where relevant
• Verified TUI behavior manually if UI changes — no UI change
• Harvested/co-authored credit uses a GitHub numeric noreply address (n/a)

🤖 Generated with Claude Code

https://claude.ai/code/session_0134iUMxmGuXiG1LzPgfZVnv

[Hmbown]

Version drift is red on 4598534 for a reason outside this PR. The job ran every release helper contract test and failed after require-release-tag-checkout tests passed on the script's own EXIT trap:

rm: cannot remove '/tmp/tmp.8yJzVEomyC/checkout': Directory not empty
##[error]Process completed with exit code 1.

This PR changes crates/tui/src/tools/review.rs and .github/workflows/codewhale-review.yml only; neither is on that job's path. The same step passed on the base commit ccbef2c at 10:43 UTC, and scripts/release/require-release-tag-checkout.test.sh passes 5/5 locally. The trap's rm -rf raced something still writing into the disposable clone, most likely a detached git gc --auto spawned by the git commit --allow-empty / git checkout --detach calls the test makes inside it (this checkout has gc.auto=0, the hosted runner does not). No fix for that exists on main yet; the fix belongs in the test script (git config gc.auto 0 on the fixture clone, or a tolerant cleanup) and is queued separately rather than widened into this PR. Re-running the failed job once.

---

Generated by Claude Code

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[claude]

Claude finished @Hmbown's task in 1m 24s —— View job

---

Review: PR #6384

• Gather context (read diff, changed files, CLAUDE.md/AGENTS.md)
• Understand the request (code review of the diff)
• Review crates/tui/src/tools/review.rs changes
• Review .github/workflows/codewhale-review.yml changes
• Post findings

Summary

Small, well-scoped fix. No correctness bugs, no provider/route-safety issues, nothing to flag on security.

crates/tui/src/tools/review.rs:1481 — format!("...{error:#}...") replaces {error} with the alternate Display on error: anyhow::Error (confirmed Result in client.rs:11 is anyhow::Result). This walks the error chain instead of printing only the outermost .context("Responses API request failed") layer (crates/tui/src/client/responses.rs:213), exposing the underlying LlmError variant's class prefix (Provider plan quota exhausted, Rate limit exceeded, Server error (NNN), etc. — crates/tui/src/llm_client/mod.rs:400-425). This is exactly the same technique already used and tested for the equivalent sub-agent masking bug (subagent_failure_message in crates/tui/src/tools/subagent/mod.rs:18498, {err:#}), so it's reuse of an established, precedented pattern rather than a new one — good rung-2 discipline.

.github/workflows/codewhale-review.yml:249-254 — the classifier regex is extended from the legacy LLM error: HTTP <status> form to also match the LlmError class prefixes. Verified against the Display impl: every added alternative (Provider plan quota exhausted, Authentication failed, Authorization failed, Rate limit exceeded, Server error \([0-9]{3}\), Network error, Request timed out) has a corresponding exact-text match in LlmError::fmt. The retained LLM error: HTTP (...) branch still covers the statuses that fall through to LlmError::Other(format!("HTTP {status}: {body}")) (mod.rs:548) — e.g. 402/408 without class-specific wording — so no status class is dropped. Invalid request (NNN) and the "response incomplete" (budget-exhausted) message correctly have no match in NONRUN, so those still fail the job as real review failures, matching the PR's stated bash-simulation testing.

Minor, non-blocking observations:

• Unlike subagent_failure_message, this path doesn't downcast to LlmError and prepend a stable [class] tag — the workflow instead re-derives the class by matching on Display text. That's a reasonable choice here since the consumer is a shell classifier, not another Rust caller, but it does mean the review-tool message and the sub-agent-failure message use two different conventions for the same underlying problem. Not worth unifying for a 12-line fix; worth a footnote if a third caller needs this.
• No new Rust test, consistent with this repo's policy (AGENTS.md: "tests are selective evidence, not the specification") and the PR's own reasoning — the format change has no branching logic to pin, and the chain is only observable against a live provider failure. The bash classifier was exercised against seven sample lines per the PR description.

No blocking issues found.

[claude]

Claude finished @Hmbown's task in 1m 27s —— View job

---

Review: PR #6384

• Gather context (read diff, changed files, CLAUDE.md/AGENTS.md)
• Check whether the Devin finding (CLI run_review path still masked) was addressed by commit bcbf690
• Review full diff for correctness / provider-safety / reuse / tests
• Post findings

Summary

The second commit (bcbf690) fixes the real issue Devin flagged, and does it the right way — by extracting a shared helper rather than duplicating the format string at the two call sites. No correctness bugs, no provider/route-safety issues, nothing to flag on security.

Devin's finding is resolved. run_review in crates/tui/src/lib.rs:8718-8730 (the path codewhale review actually dispatches to, and the one the workflow classifier reads) previously still formatted the failure with plain {error}, so the workflow's NONRUN classifier would never have seen the class prefix — exactly as Devin described. bcbf690 fixes this by pulling the format logic into pub(crate) fn request_failure_message (crates/tui/src/tools/review.rs:1591), which uses {error:#} to walk the chain, and pointing both ReviewTool::execute (review.rs:1479) and run_review (lib.rs:8723) at it. That's better than a second {error:#} edit in lib.rs alone — it removes the duplicated format string instead of adding a third copy, in keeping with this repo's rung-2 reuse rule.

Verified the new test against LlmError::fmt. request_failure_message_keeps_the_provider_failure_beneath_the_context (review.rs:1978) wraps LlmError::ServerError { status: 503, .. } in .context("Responses API request failed") and asserts the output is "Review pass 1/1 request failed: Responses API request failed: Server error (503): upstream unavailable". Checked against LlmError::fmt (llm_client/mod.rs:407-408, write!(f, "Server error ({status}): {message}")) — matches exactly. This is a real regression test for the masking bug (not one that would also pass on the old code), consistent with AGENTS.md's "prove it fails without the fix" guidance in spirit for the format string being tested.

Workflow classifier still correct. Re-checked the extended NONRUN pattern in .github/workflows/codewhale-review.yml:249-256 against every LlmError::fmt arm: Provider plan quota exhausted, Authentication failed, Authorization failed, Rate limit exceeded, Server error \([0-9]{3}\), Network error, Request timed out all have exact matches, and the retained LLM error: HTTP (...) branch still catches the LlmError::Other fallthrough (402/408 without class wording). Invalid request (NNN) and the output-budget-exhausted message still have no match, so real review failures still fail the job.

No new provider/route-safety issue. This change only touches how an already-classified LlmError is displayed and matched; it doesn't infer provider/model selection from any prefix or freeform text.

Security. The alternate {:#} format only exposes more of the same error chain that was already being constructed — sanitize_http_error_body (llm_client/mod.rs:646) sanitizes provider HTTP bodies upstream of LlmError construction, and this PR doesn't touch that path, so nothing newly-sensitive reaches the log.

One thing I couldn't verify myself: I don't have permission to run cargo check in this sandbox, so I read the diff and cross-referenced types/signatures instead of compiling. pub(crate) fn request_failure_message in tools::review is reachable from lib.rs via crate::tools::review::request_failure_message (module is pub mod review; in tools/mod.rs), and the call sites' argument types line up (usize, usize, &anyhow::Error) — but the pending "Lint, Test" CI run on this head is the authoritative check here.

No blocking issues found.

[codewhale-agent]

Codewhale review did not run

codewhale review --pr 6384 (provider: deepseek) could not reach the model: Review pass 1/1 request failed: Responses API request failed: LLM error: HTTP 402: Insufficient Balance; no partial review was accepted or posted; publication: n.
This is a provider funding/config problem, not a finding about this PR. The check stays advisory; a maintainer with secret access needs to fund or rotate the review key (see .github/workflows/codewhale-review.yml). Re-run the workflow after that.

[claude]

Review: PR #6384

• Gather context (diff, changed files, CLAUDE.md/AGENTS.md)
• Review crates/tui/src/tools/review.rs changes (shared helper + test)
• Review crates/tui/src/lib.rs changes (run_review call site)
• Review .github/workflows/codewhale-review.yml classifier changes (incl. Devin's "Network error" substring finding)
• Post findings

[claude]

Claude finished @Hmbown's task in 3m 43s —— View job

---

Review: PR #6384

• Gather context (diff, changed files, CLAUDE.md/AGENTS.md)
• Review crates/tui/src/tools/review.rs changes (shared helper + test)
• Review crates/tui/src/lib.rs changes (run_review call site)
• Review .github/workflows/codewhale-review.yml classifier changes (incl. latest 35e0b84 shellcheck brace fix)
• Post findings

Summary

Re-reviewed the head at 35e0b84. Both of Devin's findings are resolved correctly, and the final commit is a genuinely cosmetic follow-up. No blocking issues.

Anchoring fix (32b9256) verified by hand-tracing the regex, not just re-reading it. NONRUN="Review pass [0-9]+/[0-9]+ request failed: ([A-Za-z ]+ request failed: )?($CLASS)" — the optional group must start immediately after the literal anchor (no .* gap), and [A-Za-z ]+ can't cross a digit or punctuation character, so it can only ever consume the one real <wire> API request failed: layer (Responses API request failed, Chat API request failed, or Anthropic Messages API request failed — confirmed these are the only three such context strings in crates/tui/src/client/, each applied exactly once between the retry loop's LlmError and the Review pass N/M prefix). I traced Devin's own counter-example (Invalid request (400): unsupported parameter: Network error behavior) through the pattern: the digits/colon in (400): break the letters-only group before it can reach the embedded "Network error" text, so the class alternation is attempted against Invalid request (400)... directly and correctly fails to match → still a real failure. I also tried to construct a worse bypass (a crafted body containing a second, letters-only "... request failed: <class>" substring) and confirmed the contiguous-match requirement blocks that too, since the group can't skip over the punctuation in the real wire-layer text to reach a second occurrence deeper in the body.

Shared-helper fix (bcbf690) is correct and is real reuse, not a new layer. request_failure_message in review.rs:1591 is called from both ReviewTool::execute (review.rs:1479) and run_review (lib.rs:8718-8732, the path codewhale review actually dispatches to), replacing two duplicated format strings with one. pub(crate) visibility is correct for the cross-module call from lib.rs.

Compile-correctness of the new test (review.rs:1978-1989): anyhow::Error::new(LlmError::ServerError{..}).context(...) doesn't need use anyhow::Context in scope — .context() is an inherent method on anyhow::Error itself (the trait is only for Result/Option). Confirmed this exact pattern is already used and compiling on main in tools/subagent/tests.rs (e.g. line 9605), so this isn't a novel risk. The asserted output also matches LlmError::fmt's Server error ({status}): {message} arm exactly.

35e0b84 is a pure no-op fix. Diff is only "$NONRUN[^\"]{0,80}" → "${NONRUN}[^\"]{0,80}" — bracing the variable before a literal [ is the standard fix for shellcheck's SC1087 (unbraced-var-before-bracket looks like array indexing), and doesn't change the pattern or the extracted REASON text.

Provider/route safety: no concern — this only changes how an already-constructed LlmError is displayed/matched; nothing infers provider or model selection from prefix text.

Security: no new exposure — the alternate {:#} format surfaces more of the same chain that was already built; sanitize_http_error_body runs upstream of LlmError construction and is untouched.

Tests: the one new Rust test is a real regression test for the masking bug (PR shows it failing when the helper is neutralized to {error}), consistent with the repo's "selective evidence" testing policy — appropriately scoped, not over-tested.

Nothing further to flag; this looks ready pending the outstanding CI runs (Workflow lint, Lint, Test ubuntu/macos/windows) noted in the PR's own checklist.