Skip to content

fix(agent): retry on JSONDecodeError instead of aborting - #1

Closed
BrianLi009 wants to merge 7 commits into
dev/brian/single-agent-refactorfrom
dev/brian/jsondecode-retry-fix
Closed

fix(agent): retry on JSONDecodeError instead of aborting#1
BrianLi009 wants to merge 7 commits into
dev/brian/single-agent-refactorfrom
dev/brian/jsondecode-retry-fix

Conversation

@BrianLi009

Copy link
Copy Markdown

Problem

On a large/slow model call, the provider can return a body the client can't parse as JSON (truncated response, gateway HTML, SSE mismatch) → json.JSONDecodeError. Because json.JSONDecodeError subclasses ValueError, the retry classifier in run_agent.py treated it as a non-retryable local validation error and aborted on attempt 1 (logged as "Non-retryable client error (HTTP None)"), even though the failure is transient and usually clears on retry.

Observed in a report-generation run: a ~221K-token call returned an unparseable body and hard-failed instead of retrying.

Fix

Exclude json.JSONDecodeError from the local-validation bucket so it falls through to the normal retry/backoff path like other transient API errors. One-line change (+ comment).

is_local_validation_error = (
    isinstance(api_error, (ValueError, TypeError))
    and not isinstance(api_error, json.JSONDecodeError)
)

json is already imported. Compiles clean. Base is dev/brian/single-agent-refactor — the branch the current report pipeline runs.

🤖 Generated with Claude Code

GurneeshBudhiraja and others added 7 commits May 27, 2026 20:39
- Replace flat 900s API timeout with httpx.Timeout (connect=15, read=300, write=60, pool=15)
- Add explicit timeout retry logging visible in Modal logs
- Add user_notes toolset (list, read, search) — read-only Supabase access with RLS defense-in-depth
- Thread-local sandbox env for per-execution isolation in concurrent Modal containers
- Inject Snowflake token and HERMES_TMP_DIR into sandbox PYTHONPATH
- Exclude json.JSONDecodeError from non-retryable local validation errors

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
json.JSONDecodeError subclasses ValueError, so an unparseable provider
response (truncated body, gateway HTML, or an SSE mismatch on a large/slow
request) was bucketed as a non-retryable "local validation error" and
aborted on attempt 1. Exclude JSONDecodeError from that bucket so transient
parse failures retry with backoff like other transient API errors.

Surfaced by a ~221K-token report-generation call that returned an
unparseable body and hard-failed instead of retrying.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…basic/advanced modes)

parallel-web 1.0 moved search/extract from the .beta namespace to the
top-level client, replaced fast/one-shot/agentic modes with basic/advanced,
dropped max_results (results now sliced client-side), and returns
full_content by default. This has been running in deployment (Modal mounts
the working tree) since the 1.0 breakage; committing so main matches what
ships.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
read_file returns 'LINE_NUM|CONTENT' lines; models sometimes copy blocks
from a read result into write_file content verbatim, persisting the gutter
into the file — it corrupts <style> blocks and renders as visible junk
(shipped broken report HTML to users three times). The patch path already
strips this format (patch_parser._apply_update); write_file did not.

strip_pasted_line_numbers() fires only on the unmistakable signature:
>=4 prefixed lines covering >=50% of non-empty lines with >=90%
sequentially increasing numbers — markdown tables, shell pipes, and
'id|value' data files pass through untouched. write_file_tool logs a
warning and returns a corrective notice to the model when it fires.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… fixes

The parallel/tavily failover dispatched URLs before the website-policy
check, which only lived inside the Firecrawl fall-through loop — blocked
hosts sailed straight to the new backends, and redirected final URLs were
only re-checked on the Firecrawl path. Hoist the policy check into the
pre-dispatch filter (alongside SSRF) and re-check final URLs uniformly on
every backend's results. Gate the Firecrawl fall-through on client
constructibility instead of env sniffing (equivalent in prod; the client
raises when unconfigured).

Also: add force=True to the invalid-JSON retry-cap error vprint, and
update test_blocked_tools_constant for the deliberate execute_code
allowance in subagents (b0bf702) with an explicit companion assertion.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The cache used a constant '__default__' key and the disabled-policy fast
path returned early without checking key or TTL — after HERMES_HOME
changed at runtime, a stale 'disabled' entry from the previous home
failed-open the new home's blocklist (and vice versa could serve stale
blocks). Key the cache on the resolved default config path and make the
fast path require key + freshness. Surfaced by CI once web_extract began
calling check_website_access pre-dispatch.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@aman-a-shah

Copy link
Copy Markdown

Review of PR #1 — retry on JSONDecodeError

TL;DR: this PR is already fully merged into main (via PR #2, merge commit 51b4c99b, 2026-06-23). The branch tip c0a1f377 is an ancestor of origin/main, which is now 18 commits ahead. Recommend closing this PR as superseded — nothing here is lost.

The change itself (for the record)

The core fix is sound and I'd have approved it on the merits:

  • Where the error arises: in the API-call except block of run_agent.py — the client fails to parse the provider's HTTP response body (truncated body, gateway HTML, SSE mismatch on large/slow calls). It is not a tool-call-arguments parse; it's a transport-level response failure, so re-sending the same request is the right remedy (no model correction needed).
  • Root cause correctly identified: json.JSONDecodeError subclasses ValueError, so the is_local_validation_error bucket swallowed it and aborted on attempt 1 as a "non-retryable client error (HTTP None)".
  • Bounded retry: yes — the loop is while retry_count < max_retries with max_retries = 3 and exponential backoff, so excluding JSONDecodeError from the non-retryable bucket cannot loop forever.
  • Minor gap: no dedicated unit test asserting that json.JSONDecodeError is classified as retryable while plain ValueError/TypeError still abort. Moot now, but worth adding on main if this classifier gets touched again.

Note the PR's visible diff is much larger than the one-line fix because (a) the base is the stale dev/brian/single-agent-refactor branch (itself now an ancestor of main), and (b) the branch later merged origin/main into itself (cbe81506) and picked up unrelated "converge deployed state" work (httpx granular timeouts, user_notes_tool registration, web-tools/policy fixes). All of that is on main too.

CI failure diagnosis (test job, run 27239550639)

Not caused by this PR's change. The 18 failures + 1 collection error (2026-06-09) fall into three buckets, all pre-existing staleness/env issues:

  1. test_run_agent.py::TestBuildApiKwargs::test_basic_kwargsassert Timeout(connect=15.0, read=300.0, ...) == 900.0: the test wasn't updated when the branch adopted httpx.Timeout for _build_api_kwargs.
  2. 17 failures in test_streaming.py / test_run_agent.py / test_context_token_tracking.pyRuntimeError: Failed to initialize OpenAI client: Missing credentials: CI env lacked dummy API keys.
  3. tests/acp/test_server.pyImportError: cannot import name 'AuthMethod' from 'acp.schema': unpinned acp dependency drifted.

Evidence that these were environmental/stale: main's own Tests run failed identically right after PR #2 merged this same branch (run 28062459400), and was then fixed on main by 1fb278ab ("Fix pre-existing CI test failures (dummy keys, acp pin, timeout assert)") and 97801e0e ("Isolate no-key tests from the CI dummy API keys"). main's Tests have been green since (latest: 2026-07-24).

Verdict & recommendation

Close as superseded — do not merge, do not rebase. Every commit on dev/brian/jsondecode-retry-fix is already in main, including the exact JSONDecodeError exclusion (now at run_agent.py:6403-6410 on main with the same explanatory comment). The failing CI here would only be fixable by merging main into the branch — which would make the PR diff empty. The base branch dev/brian/single-agent-refactor is likewise fully contained in main and can probably be deleted along with this branch after closing.

🤖 Generated with Claude Code

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants