Repository navigation
fix(errors): keep business text from disabling agent retries - #254
cosminacho wants to merge 3 commits into
Conversation
|
Claude finished @cosminacho's task in 1m 41s —— View job Reviewing PR #254: fix(errors): keep business text from disabling agent retries
|
uipreliga
left a comment
There was a problem hiding this comment.
Review: coder_eval — pr:254 (3 files) axis:1-8
Scope: pr:254 (3 files) axis:1-8 · branch codex/provider-billing-signatures (PR #254) · 51407a2 · 2026-11-08T14:29Z · workflow variant
Change class: complex — changes error classification that gates retry eligibility (billing vs crash), a control-flow/semantics change needing reasoning about provider error strings
The codebase is healthy (9.8/10, with no critical or high findings, and full marks for security and architecture), but its single real risk is the substring billing classifier in src/coder_eval/errors/categorization.py:92-114: it still marks business text and per-minute quota throttles as non-retryable AGENT_BILLING_ERROR, and it now marks real Codex and OpenAI quota denials as retryable crashes, so for identical agent output a task can end with a different final_status or use retries that cannot succeed. The bottom line is to replace that classifier with typed classification before the next release.
Summary
| Axis | Score | 🔴 | 🟠 | 🟡 | 🔵 | Top Issue |
|---|---|---|---|---|---|---|
| 1. Code Quality & Style | 9.8 / 10 | 0 | 0 | 0 | 2 | The new test imports httpx inside the function, and the project does not declare httpx |
| 2. Type Safety | 9.5 / 10 | 0 | 0 | 1 | 0 | Narrowed billing signature list drops real provider billing denials (Codex usage-limit/out-of-credits, OpenAI/Gemini insufficient-quota text, Bedrock/Vertex payment denials), which now fall through to retryable AGENT_CRASH / AGENT_RATE_LIMIT with no test catching it |
| 3. Test Health | 9.9 / 10 | 0 | 0 | 0 | 1 | AgentKind parametrization in the scripted-dialog retry test is vacuous because every kind runs the same MockAgent |
| 4. Security | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 5. Architecture & Design | 10 / 10 | 0 | 0 | 0 | 0 | — |
| 6. Error Handling & Resilience | 9.9 / 10 | 0 | 0 | 0 | 1 | 'quota exceeded' runs before the rate-limit branch, so transient per-minute Vertex/Gemini 429 quotas are classified as non-retryable billing |
| 7. API Surface & Maintainability | 9.9 / 10 | 0 | 0 | 0 | 1 | Billing signature list is an anonymous inline literal with no provider provenance and duplicated underscore/space variants |
| 8. Evaluation Harness Quality | 9.5 / 10 | 0 | 0 | 1 | 0 | Remaining billing patterns ('payment required', 'spending limit', 'quota exceeded', 'insufficient credit', bare 402) still classify business text as non-retryable AGENT_BILLING_ERROR, and no test covers them |
Overall Score: 9.8 / 10 · Weakest Axis: Type Safety at 9.5 / 10
Totals: 🔴 0 · 🟠 0 · 🟡 2 · 🔵 5 across 8 axes.
Blockers
None.
Non-blocking, but please consider before merge
- [Axis 2] Narrowed billing signature list drops real provider billing denials (Codex usage-limit/out-of-credits, OpenAI/Gemini insufficient-quota text, Bedrock/Vertex payment denials), which now fall through to retryable AGENT_CRASH / AGENT_RATE_LIMIT with no test catching it (
src/coder_eval/errors/categorization.py:93) — categorization.py:93-109 decides AGENT_BILLING_ERROR only by substring-matching the lowercased message:if any(pat in error_str for pat in ["insufficient credit", ..., "quota exceeded", "spending limit"]) or _mentions_status(error_str, "402"):. The agents already hold typed signals, and this check throws them away. (1) Codex:_failed_turn_reason(codex_agent.py:274-276) flattens the typedCodexErrorInfoValueenum into free text:suffix = f" (codexErrorInfo={kind})" if kind else "". The installed SDK enum hasusage_limit_exceeded = "usageLimitExceeded"(openai_codex/generated/v2_all.py:544). None of the 11 new patterns matchesusagelimitexceeded. Before this PR, the barecredit/billingwords caught the usage-limit message text. Now that crash is classified as AGENT_CRASH and every configured retry runs against a denial that will not clear. The exact Codex usage-limit wording was not confirmed offline, but the typed kind is certain. (2)_finalize_and_raise_crash(agent.py:150-154) raisesAgentCrashError(message) from cause, so the typed SDK exception is onerror.__cause__(httpx/anthropic/openaistatus_code, openai.code == 'insufficient_quota'). Callers also pass the message throughtruncate_crash_message(200 chars, codex_agent.py:1229), which can cut a trailing'code': 'insufficient_quota'before the substring check runs. Recommended fix: classify from typed data first. Add a typed check in_categorize_by_exception_typethat walkserror.__cause__and checksgetattr(cause, 'status_code', None) == 402orgetattr(cause, 'code', None) in {'insufficient_quota', 'billing_hard_limit_reached', 'billing_not_active'}. Better still, give AgentCrashError a typedprovider_kind: str | None/http_status: int | Nonefield that agents fill from_codex_error_kind, and mapusageLimitExceededexplicitly. Keep the substring list only as the last fallback. Add a test that sendsAgentCrashError("Codex turn failed: ... (codexErrorInfo=usageLimitExceeded)")and asserts the intended category. - [Axis 8] Remaining billing patterns ('payment required', 'spending limit', 'quota exceeded', 'insufficient credit', bare 402) still classify business text as non-retryable AGENT_BILLING_ERROR, and no test covers them (
src/coder_eval/errors/categorization.py:103-108) — The PR says it stops business text in an AgentCrashError from making the error non-retryable. But the narrowed list still has ordinary business phrases: L103 "payment required", L105 "quota exceeded", L106 "spending limit", and L108_mentions_status(error_str, "402"). On the PR-head code, AgentCrashError('The payment required for invoice INV-7 is $50.'), AgentCrashError('Employees have a spending limit of $500 per trip.') and AgentCrashError('Delegate turn failed: Invoice 402 was rejected by the approver.') all return agent_billing_error, and should_retry is False. '$402' matches too, because\b402\bmatches in '$402.00'. This is exactly the failure that the new scripted-dialog test is about. Its fixture TURNS ask about reimbursements, and a crash that carries that answer still loses its configured question retries, so the run ends ERROR where a retry could have succeeded. The bare '401' (auth) and '429' (rate limit) checks on L87-88/L112-113 have the same issue. The new tests at tests/test_error_handling.py:249-251 use only phrases that the new list does not match, so the suite does not show the remaining false positives. Fix: either use only multi-word provider phrases that cannot be normal prose (for example 'credit balance is too low', 'billing_hard_limit_reached'), or match only on the provider-error part of the crash message (the stderr/SDK error, not summary.result or answer text). Add negative test params for 'payment required', 'spending limit', 'quota exceeded' and '$402' in business prose.
Nits
- [Axis 1] The new test imports
httpxinside the function, and the project does not declarehttpx(tests/test_error_handling.py:279) —test_http_payment_required_stays_non_retryablehasimport httpxinside the function (line 279). It is the only use of plainhttpxin src/ and tests/. The project declareshttpx2(pyproject.toml:"httpx2>=2.12.0,<3.0.0", because anthropic>=1.0 moved to httpx2), and the other tests importhttpx2at module level (e.g. tests/test_judge_bedrock.py:12).httpxis available only as a transitive dependency of litellm, openai and mcp, so this test can break when those dependencies change. Also,httpx.HTTPStatusErroris not the exception that the in-tree Anthropic path raises. Move the import to module level and usehttpx2, or create the error from the SDK exception type (anthropic.APIStatusErrorwith status 402) that agents actually receive. - [Axis 1]
_SessionAgentdocstring is now out of date, and a generic test helper hard-codes one scenario's crash text (tests/test_scripted_dialog.py:78) — The class docstring at line 54 still says"A ``MockAgent`` that remembers every prompt, and can time out or run out of turns on cue.". It does not mention the newcrash_once_on_callbehavior. The shared helper also hard-codes the business-text messageraise AgentCrashError("The billing team handles invoice requests.")(line 78), so the parameter namecrash_once_on_calldoes not tell the reader that the crash text is chosen to look like billing. Addcrash_message: strnext tocrash_once_on_calland pass the business text from the test, or name the parameter for what it does. Update the docstring to include the crash cue. - [Axis 3] AgentKind parametrization in the scripted-dialog retry test is vacuous because every kind runs the same MockAgent (
tests/test_scripted_dialog.py:137-147) —@pytest.mark.parametrize("kind", [AgentKind.CLAUDE_CODE, AgentKind.CODEX, AgentKind.DELEGATE])changes onlyagent=parse_agent_config(type=kind)._install_agentreplaces Orchestrator._create_agent with_SessionAgent(MockAgent)for every kind, and the orchestrator retry block (_communicate_with_retry, orchestrator.py ~L1975-2030) does not branch on the agent type. So the three cases are duplicates. They suggest per-backend coverage, yet none of them sends the crash text through a real agent's message construction (e.g. Codex's "Codex turn failed: {message} (codexErrorInfo=...)" or delegate_agent'sraise AgentCrashError(reason)). Fix: drop the parametrization and keep one case. If per-backend coverage is the goal, add a unit test per agent that feeds a business-text failure through the agent's own error-to-AgentCrashError path and asserts that categorize_error returns AGENT_CRASH. - [Axis 6] 'quota exceeded' runs before the rate-limit branch, so transient per-minute Vertex/Gemini 429 quotas are classified as non-retryable billing (
src/coder_eval/errors/categorization.py:105) — The PR keeps"quota exceeded",(line 105) in the billing branch, which runs before the rate-limit branch (... or _mentions_status(error_str, "429"), lines 112-114). Vertex (Claude Code with CLAUDE_CODE_USE_VERTEX) and Gemini report per-minute throttling as "429 Quota exceeded for aiplatform.googleapis.com/online_prediction_tokens_per_minute_per_base_model ...". On PR HEAD (and on main) this gives agent_billing_error, so a transient rate limit fails the task on the first attempt. The behavior existed before this PR, but the PR re-lists this phrase as an explicit provider signature. Exclude per-time-window quotas, for example by checkingper_minute/per_base_model/resource_exhaustedfirst and routing them to AGENT_RATE_LIMIT. Alternatively, movequota exceededbelow the 429 check and keep only the code-shapedinsufficient_quotaahead of it. Add a regression param for the Vertex message. - [Axis 7] Billing signature list is an anonymous inline literal with no provider provenance and duplicated underscore/space variants (
src/coder_eval/errors/categorization.py:92-107) — The list keeps both"billing_hard_limit",and"billing hard limit",, and both"payment required",and"payment_required",. It does not say which backend (Anthropic, OpenAI/Codex, Bedrock, Vertex, OpenRouter for OpenCode/Pi, Delegate) emits which signature. The only comment (L92) says what is excluded, not what is covered. The PR also deleted the old rationale that a false negative wastes every retry, and no notes file records it now. A maintainer who adds a backend cannot tell whether that backend is covered. Fix: move the list to a module-level_PROVIDER_BILLING_SIGNATUREStuple, grouped by provider. Normalizeerror_str.replace("_", " ")once, so that each variant needs only one entry. Record the false-positive/false-negative trade-off in .claude/notes/ behind aRationale:pointer.
What's Missing
Parallel paths:
- 🟡 The PR changes only the matcher. The code that builds the crash messages is unchanged.
ClaudeCodeAgent._format_error_summary(claude_code_agent.py:1922-1923) still returnssummary.result[:200]as the crash reason, and that text can be the model's own answer. Delegate (delegate_agent.py:1291) and Codex (_failed_turn_reason) also put free text intoAgentCrashError. So every pattern that remains in the list, and every pattern added later, can still match answer text. .claude/notes/agents.md already records this lesson for the stderr tail ("No crash reason carries the stderr tail": the word "guardrail" in a task path became non-retryable AGENT_INVALID_OUTPUT). The same rule was not applied tosummary.result. (trigger: src/coder_eval/errors/categorization.py) (restates: Axis 8: Remaining billing patterns still classify business text as non-retryable AGENT_BILLING_ERROR) - 🟡 The Codex agent already knows the typed failure kind:
_codex_error_kindreturnsusageLimitExceeded._failed_turn_reasonturns it into a text suffix, andtruncate_crash_message(200 chars) cuts that suffix off. The PR removed the barecredit/billingwords but did not add a typed mapping on the Codex side to replace them. A Codex usage-limit denial now becomes retryable AGENT_CRASH. (trigger: src/coder_eval/errors/categorization.py) (restates: Axis 2: Narrowed billing signature list drops real provider billing denials)
Tests:
- 🟡 No positive test covers four of the kept billing signatures:
quota exceeded,spending limit, the space formpayment required, or a bare402inside anAgentCrashErrormessage. The newtest_provider_quota_denial_stays_non_retryableparams cover onlycredit balance is too low,insufficient api credit,insufficient_quota,billing_hard_limit*,billing_not_activeandpayment_required. You can delete any of the four uncovered entries from the list and the suite still passes. (trigger: tests/test_error_handling.py) - 🟡 No test sends a real-length provider denial through the actual agent path before categorization (
truncate_crash_messageat 200 chars, thencategorize_error). Examples are a CodexCodex turn failed: ... (codexErrorInfo=usageLimitExceeded)message and the standard OpenAIError code: 429 - {... 'type': 'insufficient_quota'}string. The new params are short strings in which the signature always survives truncation, so the regression that truncation causes cannot be seen. (trigger: tests/test_error_handling.py) (restates: Axis 2: Narrowed billing signature list drops real provider billing denials)
Downstream consumers:
- 🔵 The billing branch in
_categorize_by_messagedoes not check the component. So the narrowed list also changes the category for errors from sandbox, evaluator and task, and for agent errors that are notAgentCrashError. The PR body describes only the AgentCrashError case. I ran the same input on main and on the PR.RuntimeError("pip install failed: insufficient permissions")with component=sandbox gives AGENT_BILLING_ERROR (non-retryable) on main and PACKAGE_INSTALL_ERROR (retryable) on the PR. With component=evaluator, text that contains "credit" changes from AGENT_BILLING_ERROR to CRITERION_CHECK_ERROR. The change is probably an improvement, but no test covers it and the PR body does not mention it. (trigger: src/coder_eval/errors/categorization.py)
Daily/nightly:
- 🟡 The PR body does not say what happens to the nightly run.
run.jsonerror_category(run_record.py:177) is part of the contract with coder-eval-uipath, and its values will change. Billing denials that the narrowed list misses now get retried. If a truncatedinsufficient_quotamessage becomes AGENT_RATE_LIMIT, it gets 5 retries with a 60 s initial delay and x3 backoff (categories.py RETRY_CONFIG), which is about 2 h of backoff for each task. If the denial becomes AGENT_CRASH, it gets 2 retries. During a provider quota outage, every task in the nightly pays this cost instead of failing quickly. (trigger: src/coder_eval/errors/categorization.py) (restates: Axis 2: Narrowed billing signature list drops real provider billing denials)
Harness & Lint Improvements
Static checks (lint / type):
- [ruff] Add "TID251" to
[tool.ruff.lint] selectin pyproject.toml and set[tool.ruff.lint.flake8-tidy-imports.banned-api] "httpx".msg = "anthropic>=1.0 uses httpx2; httpx is only a transitive dependency of litellm/openai/mcp. Import httpx2.". This is a config-only change. TID251 matchesimport httpxat any nesting depth, so it also catches the import inside the function. Do not enable PLC0415 (import-outside-toplevel) as the general fix: tests/ already has about 1.5k indented imports, so it would be noise, not a gate. Prevents: Finding 1 (tests/test_error_handling.py:279import httpxinsidetest_http_payment_required_stays_non_retryable). Any later src/ or tests/ import of the undeclaredhttpxfailsmake checkbefore review. - [ce-lint] CE069
no_undeclared_third_party_import(tests/lint/rules/ce069_no_undeclared_third_party_import.py, wired in tests/lint/runner.py). Every top-level module imported in src/coder_eval/ (runtime deps plus optional extras) and in tests/ (adds the dev group) must map to a distribution named in pyproject.toml. It resolves the mapping withimportlib.metadata.packages_distributions(). Stdlib comes fromsys.stdlib_module_names. First-partycoder_evalis allowed. A small EXEMPT dict covers modules whose dist name differs and cannot be resolved, each with a reason. Blind spot: animportlib.import_module("...")with a dynamic name. TID251 bans the one known name. This rule covers the whole class (a transitive-only import that breaks when an upstream changes its dependencies). Before adding it, check whether a singledeptryrun (DEP003 transitive) covers the same ground. If it does, prefer amake lintstep that calls deptry over a hand-written rule. Prevents: Finding 1 (plainhttpxreached only through litellm/openai/mcp), and the next transitive-only import of any other package, whatever its name. - [pyright] Give
AgentCrashError(src/coder_eval/errors/agent.py:16) a typed provider signal:def __init__(self, message: str, *, provider_error: ProviderErrorKind | None).ProviderErrorKindis a StrEnum (BILLING, USAGE_LIMIT, RATE_LIMIT, AUTH, ...). The keyword is REQUIRED and has NO default, so pyrightreportCallIssueflags everyraise AgentCrashError(...)that has not made a decision. Each agent must then map its typed source explicitly: Codex_codex_error_kind(usageLimitExceeded-> USAGE_LIMIT), the SDKstatus_code == 402, openai.code == "insufficient_quota". The value is set from untruncated typed data, sotruncate_crash_messagecan no longer remove the signal. Prevents: Finding 3 (CodexusageLimitExceededand OpenAIinsufficient_quotaflattened into free text and then cut by the 200-char truncation, so they fall through to retryable AGENT_CRASH / AGENT_RATE_LIMIT). Also blocks the next agent that raises a crash without stating its provider category. - [ce-lint] CE070
crash_category_is_typed(tests/lint/rules/ce070_crash_category_is_typed.py, wired in tests/lint/runner.py). Scope: src/coder_eval/errors/categorization.py. Incategorize_error, anisinstance(error, AgentCrashError)branch must return (readingerror.provider_error) BEFORE any call to_categorize_by_message/_mentions_status. The AST check: no path from the function entry reaches a_categorize_by_message(call without first passing the AgentCrashError guard. A second check in the same rule: substring pattern lists in that module must be module-levelFinaltuple constants (for example_BILLING_SIGNATURES), not inline list literals insideany(pat in s for pat in [...]). Blind spot: a helper that does its own substring matching under a different name. List the helper names in the rule docstring. Prevents: Finding 7 (business text such as 'payment required', 'spending limit', 'quota exceeded', '$402' in an AgentCrashError message is classified as non-retryable AGENT_BILLING_ERROR). Finding 3 (crash classification by substring instead of typed data). Finding 6 (an unnamed inline billing literal with no provider provenance that cannot be iterated by a corpus test).
Harness improvements (not statically reachable):
- Add a provider-error corpus test, tests/fixtures/provider_errors.yaml plus a parametrized test. Each entry holds a REAL provider error string (Anthropic 'credit balance is too low', OpenAI 429
insufficient_quota, Codex failed turn withcodexErrorInfo=usageLimitExceeded, Vertex/Gemini '429 Quota exceeded ... per_minute_per_base_model', Bedrock/OpenRouter payment denials), the backend that emits it, and the expected ErrorCategory and should_retry. The test sends each string through that agent's OWN message-building path (_failed_turn_reason,truncate_crash_message,_finalize_and_raise_crash), not as a hand-builtAgentCrashError(msg). Add a negative corpus of business prose ('payment required for invoice INV-7', 'spending limit of $500', 'Total $402.00', 'monthly quota exceeded for travel', '401k', '429 Main St') that must stay AGENT_CRASH and retryable. Why not static: Whether a string is a provider denial or task content is a semantic judgment about real provider wording. Only examples, not an AST pattern, can pin that wording. The truncation-then-classify ordering bug also shows only when the real 200-char cap runs on a real message. Prevents: Finding 3 (usage-limit / insufficient_quota regressions), Finding 5 (transient per-minute Vertex 429 classified as non-retryable billing), Finding 7 (remaining business-text false positives, which the PR's own negative params did not test). - Record the false-positive / false-negative trade-off of error categorization in .claude/notes/ (a retry wasted on a permanent denial versus a lost retry on a transient or business-text crash). Point to it with a
Rationale:line in categorization.py. Name each backend's typed source of provider-error signals, so a new agent author knows which field to map ontoprovider_error. Why not static: This records a design decision and its reason. A lint rule cannot check that the reason is present or correct. Prevents: Finding 6 (the PR deleted the old 'a false negative wastes every retry' comment and no notes file records it now) and the Finding 3 / Finding 7 oscillation between too-broad and too-narrow pattern lists. - Test-review checklist item for cr-axis Axis 3: a test parametrized over
AgentKind(or any backend enum) must show that the parameter reaches backend-specific code. If the module replacesOrchestrator._create_agentwith a MockAgent, collapse the parametrization to one case. Put per-backend coverage in each agent's own unit test, which feeds a crash through that agent's error-to-AgentCrashError path. Why not static: To prove that a parameter is vacuous, the check must follow the data flow through monkeypatched factories into runtime dispatch. An AST heuristic (parametrize over AgentKind plus patch of_create_agentin the same module) gives false positives on tests that patch only part of the path, so this check belongs to review. Prevents: Finding 4 (tests/test_scripted_dialog.py:137-147: three identical cases that suggest Codex/Delegate coverage that does not exist). - Review-process note: when a shared test helper gains a behavior parameter, the same change updates the helper docstring. Scenario-specific data (for example the billing-looking crash text) is passed in by the calling test, not hard-coded in the helper. Why not static: A tool cannot decide whether a docstring still describes the behavior, or whether a literal is specific to one scenario. Both need a person who knows the intent. Prevents: Finding 2 (the
_SessionAgentdocstring does not mentioncrash_once_on_call, and the helper hard-codes 'The billing team handles invoice requests.').
Top 5 Priority Actions
- Stop business prose from becoming a non-retryable error: remove or narrow 'payment required', 'spending limit', 'quota exceeded', 'insufficient credit' and the bare \b402\b status check in src/coder_eval/errors/categorization.py:103-108 (the bare 401/429 checks at :87-88 and :112-113 have the same flaw), match only the provider-error part of the crash message, and add negative test params such as 'Total $402.00 reimbursed' and 'Employees have a spending limit of $500' to tests/test_error_handling.py:249-251.
- Classify billing from typed data before the substring fallback in src/coder_eval/errors/categorization.py:93: add provider_kind/http_status fields to AgentCrashError, fill them from _codex_error_kind before the 200-char truncate_crash_message call at codex_agent.py:1229, map usageLimitExceeded and insufficient_quota explicitly, and add a regression test, because real Codex and OpenAI quota denials now fall through to retryable AGENT_CRASH / AGENT_RATE_LIMIT.
- Send transient per-minute Vertex/Gemini quota errors ('429 Quota exceeded ... per_minute_per_base_model') to AGENT_RATE_LIMIT and not to billing: either move 'quota exceeded' at src/coder_eval/errors/categorization.py:105 below the 429 check or exclude per-window quotas first, and add a Vertex regression param, so that one throttle does not fail the task on its first attempt.
- Move the inline billing list at src/coder_eval/errors/categorization.py:92-107 into a module-level _PROVIDER_BILLING_SIGNATURES tuple grouped by provider, normalize underscores to spaces once so each phrase needs only one entry, and record the false-positive/false-negative retry trade-off in .claude/notes/ behind a Rationale: pointer.
- Fix the tests: in tests/test_error_handling.py:279, import httpx2 (or anthropic.APIStatusError with status 402) at module level, not the undeclared transitive httpx; in tests/test_scripted_dialog.py:137-147, remove the AgentKind parametrization because every kind runs the same MockAgent; and in tests/test_scripted_dialog.py:54-78, add a crash_message parameter and update the _SessionAgent docstring.
Stats: 0 🔴 · 0 🟠 · 2 🟡 · 5 🔵 across 8 axes reviewed.

Agent failures could skip configured question retries when their messages contained business terms such as “spending limit”, “quota exceeded”, or an invoice amount of $402. Prefer SDK error codes and HTTP statuses, and narrow the text fallback to explicit provider denials. Vertex per-window quotas remain retryable rate limits, recognized by their complete quota-metric identifier. Codex keeps its existing machine error suffix on the raised exception; the stored crash reason stays bounded.
Ambiguous credit language remains retryable without an API denial or structured provider signal. Genuine HTTP 402, insufficient-quota codes, Anthropic API-access denials and Codex usage-limit failures remain terminal. No public fields, configuration, dependencies, retry limits or retry loops were added.
Validation: