fix(web): retry JS-shell 200s past every cache and name the escalation - #5936
Conversation
A 200 whose body extracts to nothing was terminal: `extract.rs` raised
`js_required_error` and `fetch.rs`'s retry counter only covered transport
failures. Edge caches can hold a prerendered variant while an origin MISS
serves the client-side shell, so the same URL failed on one fetch and
succeeded minutes later, non-deterministically losing research sources.
- `web/fetch.rs` gains `fetch_readable`, the one place that fetches and
extracts: a 2xx that yields no readable content is re-fetched once with
`Cache-Control: no-cache` / `Pragma: no-cache`, skipping the session
cache. Transport failures keep exactly their existing single retry.
- The receipt now carries `attempts`: per attempt the session `cache_hit`,
whether it bypassed caches, which one produced content, and the response
headers that explain the cache state (`age`, `cf-cache-status`,
`x-nextjs-prerender`, `x-vercel-cache`). The terminal error carries the
same receipt inline, so a failure explains itself too.
- The terminal error names the recovery the *calling role* owns, read from
the capability facts already on `ToolContext` — `Feature::WebSearch` plus
the authority envelope's `network_access` for the `web.run` browse
surface, `shell_policy` for a shell fallback. A read-only, network-denied
worker is told plainly that it has neither, instead of being pointed at a
surface it cannot call.
- `fetch_url` and `web_run` both moved onto `fetch_readable`; the raw
`fetch` entry point had no remaining callers and is gone.
Verified locally (no live third-party fetches; wiremock fixtures only):
cargo fmt --all -- --check: clean
cargo check -p codewhale-tui: clean
RUST_MIN_STACK=16777216 cargo test -p codewhale-tui --lib -- tools::web
194 passed; 0 failed; 1 ignored
RUST_MIN_STACK=16777216 cargo test -p codewhale-tui --lib -- tools::
1937 passed; 0 failed; 4 ignored
Both new regression tests were shown failing without the retry
(`js_shell_is_refetched_past_every_cache_before_it_becomes_an_error` and
`two_shells_fail_with_the_escalation_the_calling_role_owns`: 7 passed;
2 failed).
Closes #5904
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0188XYyJaw9Mh9uSrqQBoqhm
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_0967aec1-310e-471d-bec9-194281b1a788) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Claude finished @Hmbown's task in 6m 55s —— View job Review: fix(web): retry JS-shell 200s past every cache
Overall this is a well-scoped fix: the retry/receipt mechanics in 1. The escalation message's core justification doesn't match the code (correctness). 2. 3. Minor: a transport failure on the revalidate (second) attempt drops the first attempt's receipt. Nits:
Tests are solid and specifically designed to fail without the fix (per the PR's before/after numbers), and correctly distinguish the JS-shell retry path from the pre-existing transport retry path. No security concerns beyond what's already reviewed — the |
There was a problem hiding this comment.
🟡 Changes recommended
The JS-shell detection and terminal error text have correctness issues (brittle matching and an inaccurate “second attempt” claim) that should be fixed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR makes fetch_url and web.run resilient to cache-dependent “HTTP 200 JS shell” responses by moving fetch+extraction into a shared pipeline that can retry once with cache revalidation, while improving receipts and role-aware recovery guidance.
Changes:
- Introduces
fetch_readableto couple fetch + extract and re-fetch once withCache-Control/Pragma: no-cachewhen extraction indicates a JS shell on a 2xx response. - Extends
fetch_urlreceipts to include per-attempt cache/state facts (including selective cache headers) and threads that receipt into terminal JS-shell errors. - Migrates
fetch_urlandweb.runto the new pipeline and refactorsweb.runto split “document extraction” from “page rendering” for retry reuse.
File summaries
| File | Description |
|---|---|
| crates/tui/src/tools/web/fetch.rs | Adds readable-fetch pipeline with one cache-busting revalidate attempt, attempt receipts, and role-aware JS-shell recovery text. |
| crates/tui/src/tools/web/extract.rs | Adds a stable JS-shell marker + predicate so the fetch layer can recognize extraction failures worth retrying. |
| crates/tui/src/tools/web_run.rs | Routes web.run open through fetch_readable and splits extraction from rendering so extraction can be retried. |
| crates/tui/src/tools/fetch_url.rs | Routes fetch_url through fetch_readable and emits the new per-attempt receipt in results/errors. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| pub(crate) fn is_js_shell_error(error: &ToolError) -> bool { | ||
| error.to_string().contains(JS_SHELL_MARKER) | ||
| } |
| ToolError::execution_failed(format!( | ||
| "{marker} {url} after {count} attempts, the second past every cache ({receipt}). The response parsed but held no readable body, which usually means the page renders its content with JavaScript. Recovery: {recovery}", | ||
| marker = super::extract::JS_SHELL_MARKER, |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 820c4a9a06
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| fn js_shell_recovery(context: &ToolContext, tool_label: &str) -> String { | ||
| // `web.run` is itself the escalation, so it never names itself. | ||
| if tool_label != "web_run" && browser_surface_available(context) { | ||
| return "open this URL with the `web.run` browse surface (`web.run {\"open\": {\"url\": ...}}`), which requests it with a browser user-agent and a ten-megabyte budget and usually receives the prerendered variant.".to_string(); |
There was a problem hiding this comment.
When both fetch attempts return a JavaScript shell, the suggested escalation cannot be executed: WebRunTool::input_schema requires open to be an array containing objects with a ref_id, while this message supplies a single object with a url. The implementation consequently ignores open and reports that no operation was performed. Show the accepted shape, such as {"open":[{"ref_id":"..."}]}, so the recovery path actually works.
Useful? React with 👍 / 👎.
| ToolError::execution_failed(format!( | ||
| "{marker} {url} after {count} attempts, the second past every cache ({receipt}). The response parsed but held no readable body, which usually means the page renders its content with JavaScript. Recovery: {recovery}", | ||
| marker = super::extract::JS_SHELL_MARKER, | ||
| count = attempts.len(), | ||
| recovery = js_shell_recovery(context, tool_label), | ||
| )) |
There was a problem hiding this comment.
This newly constructed ToolError is rendered in the transcript but hard-codes English, so every non-English locale receives an untranslated failure and recovery message. Define message IDs and produce the prose through tr(locale, MessageId::...) as required for TUI-visible text.
AGENTS.md reference: crates/tui/AGENTS.md:L25-L26
Useful? React with 👍 / 👎.
| fn network_authorized(context: &ToolContext) -> bool { | ||
| context | ||
| .tool_authority | ||
| .as_deref() | ||
| .is_none_or(|authority| authority.network_access != Some(false)) |
There was a problem hiding this comment.
For a context with a tool-authority envelope whose network_access is None, this predicate returns true, but ToolRegistry::build_api_tools exposes Web/web.run only when the same field is Some(true). If such a worker reaches this error through the retained Web.fetch path, it is therefore instructed to call a tool absent from its catalog; match the registry's authorization predicate rather than treating an unspecified grant as allowed.
Useful? React with 👍 / 👎.
| fn js_shell_recovery(context: &ToolContext, tool_label: &str) -> String { | ||
| // `web.run` is itself the escalation, so it never names itself. | ||
| if tool_label != "web_run" && browser_surface_available(context) { | ||
| return "open this URL with the `web.run` browse surface (`web.run {\"open\": {\"url\": ...}}`), which requests it with a browser user-agent and a ten-megabyte budget and usually receives the prerendered variant.".to_string(); |
There was a problem hiding this comment.
For URL opens, web.run calls fetch_page, which now calls this same fetch_readable pipeline, and fetch_attempt always sets the Codewhale guarded-fetch user-agent. The browser user-agent imported by web_run.rs is used by the separate image-search request, not by open, so an origin that serves the shell based on user-agent receives the same request identity after this escalation. Remove the claim or actually configure the open path with the browser user-agent.
Useful? React with 👍 / 👎.
| for mode in [CacheMode::Default, CacheMode::Revalidate] { | ||
| let payload = fetch_inner( | ||
| url, | ||
| options, | ||
| context, |
There was a problem hiding this comment.
When the first response consumes most of options.timeout before yielding a JavaScript shell, the second loop iteration passes the unchanged options to a fresh fetch_inner, which creates a new full deadline. A fetch_url call advertised with a 60-second maximum can therefore spend nearly 120 seconds in network fetching. Establish one deadline around the readable-fetch sequence and pass only the remaining duration to the revalidation attempt.
Useful? React with 👍 / 👎.
| return Ok(ReadableFetch { | ||
| payload, | ||
| document, | ||
| attempts, | ||
| }); |
There was a problem hiding this comment.
If the default fetch receives a transient 5xx, retries successfully to a JavaScript shell, and the cache-busting fetch then succeeds directly, this returns only the second payload. FetchResponse.receipt.retries consequently becomes zero even though three HTTP requests occurred, and FetchAttempt has no field retaining the first payload's retry count. Aggregate retries across both readable attempts or record them per attempt so the new receipt remains accurate.
Useful? React with 👍 / 👎.
| format!( | ||
| "{unavailable} This role is read-only and cannot fall back to a shell fetch, so report this URL as unreadable rather than substituting another source." | ||
| ) |
There was a problem hiding this comment.
When a normal Agent or Operate session has shell access disabled, its ShellPolicy is None even though the role can still have mutation authority. If web.run itself reaches this terminal shell error, shell_fallback_available is false and this branch incorrectly tells the model that its role is read-only, which can cause it to abandon other permitted work. Describe only the missing shell fallback, or inspect the mutation authority separately before labeling the role read-only.
Useful? React with 👍 / 👎.
cargo clippy -p codewhale-tui --all-targets -- -D warnings: clean. Signed-off-by: CodeWhale Bot <hmbown@gmail.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_2f79d791-62ca-4b07-95fd-06f63b47efc7) |
|
Claude finished @Hmbown's task in 3m 4s —— View job Review: fix(web): retry JS-shell 200s past every cache and name the escalation
The mechanism ( 1. The suggested 2. "Requests it with a browser user-agent" is false ( 3. Each readable-fetch attempt gets its own full timeout, silently doubling the advertised max ( 4. The failure message always claims "the second past every cache" even when only one attempt was made ( Reuse/simplification: Minor / already-acceptable:
Tests: the three new tests are well-targeted — mock-based, cover the shell→content recovery, the two-shells terminal failure (including the read-only/network-denied wording), and that transport retries don't earn a second cache-busting request. Not covered: a transport failure on the revalidate leg, and nothing exercises that the suggested Security: no new concerns. The |
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
PR #5936 adds a single cache-busting retry for 2xx responses whose extraction hits the JS-shell marker, plus role-aware escalation text and richer receipt attempts. The core mechanism is sound and well-tested at the fetch_readable layer, but a few interactions and test gaps remain.
Findings
- [WARNING] Field projection may mask the JS-shell marker and bypass the retry (
crates/tui/src/tools/fetch_url.rs:198)
In the fetch_url extraction closure, project_json_fields is called before extract_fetched_document. If a non-empty requested_fields input causes project_json_fields to fail on a text/html shell body, fetch_readable sees a non-JS-shell error, is_js_shell_error returns false, and the cache-busting retry never runs. The new tests only exercise the no-fields path, so this interaction is unverified and likely still broken for field requests. - [WARNING] A failed cache-busting second attempt loses the first JS-shell attempt and escalation (
crates/tui/src/tools/web/fetch.rs:210)
fetch_readable_inner uses?on fetch_inner for the revalidation pass. If the no-cache request fails with a transport or policy error after the first attempt was a JS shell, the returned error is the transport error only; the first attempt's record and the role-aware web.run recovery text are discarded. This contradicts the PR goal that terminal failures carry the attempt receipt and recovery guidance. - [INFO] Missing tests for session-cache bypass and tool-level receipt attempts (
crates/tui/src/tools/web/fetch.rs:758)
The new tests cover shell-then-prerender, double shell, and transport retry at the fetch_readable layer, but they do not populate the session fetch cache to prove a cached JS shell is re-fetched past the cache, nor do they exercise the fetch_url/web_run tool paths to assert receipt.attempts serialization or the exact web_run recovery wording.
Suggestions
crates/tui/src/tools/fetch_url.rs:198— Run extraction before field projection so JS-shell detection always wins, or ensure project_json_fields cannot return an error for HTML shells and add a regression test with non-empty requested_fields.crates/tui/src/tools/web/fetch.rs:210— Catch fetch_inner errors on the revalidation pass after an earlier JS-shell attempt and wrap them with js_shell_failure or otherwise append the prior attempt, so the model still receives the escalation context.
Assessment
The retry-by-revalidation mechanism is a good, bounded fix and the new tests demonstrate the intended behavior for the common shell-then-prerender case. Before merge, the field-projection interaction should be verified and the second-attempt error path should preserve the first attempt's JS-shell context; otherwise the fix may still miss some fetch_url invocations and produce unhelpful errors when the cache-busting request itself fails.
Advisory review by Codewhale (codewhale review --pr 5936 --post, head 77cf192081c44a46e2860a9d9d17eafd9f7a1284). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| }; | ||
| let fields = match body_text.as_deref() { | ||
| Some(body) => { | ||
| project_json_fields(body, &fetched.content_type, requested_fields)? |
There was a problem hiding this comment.
[WARNING] Field projection may mask the JS-shell marker and bypass the retry
In the fetch_url extraction closure, project_json_fields is called before extract_fetched_document. If a non-empty requested_fields input causes project_json_fields to fail on a text/html shell body, fetch_readable sees a non-JS-shell error, is_js_shell_error returns false, and the cache-busting retry never runs. The new tests only exercise the no-fields path, so this interaction is unverified and likely still broken for field requests.
| extract, | ||
| ) | ||
| .await | ||
| } |
There was a problem hiding this comment.
[WARNING] A failed cache-busting second attempt loses the first JS-shell attempt and escalation
fetch_readable_inner uses ? on fetch_inner for the revalidation pass. If the no-cache request fails with a transport or policy error after the first attempt was a JS shell, the returned error is the transport error only; the first attempt's record and the role-aware web.run recovery text are discarded. This contradicts the PR goal that terminal failures carry the attempt receipt and recovery guidance.
| @@ -467,6 +758,229 @@ mod tests { | |||
| assert!(!large.cache_hit); | |||
There was a problem hiding this comment.
[INFO] Missing tests for session-cache bypass and tool-level receipt attempts
The new tests cover shell-then-prerender, double shell, and transport retry at the fetch_readable layer, but they do not populate the session fetch cache to prove a cached JS shell is re-fetched past the cache, nor do they exercise the fetch_url/web_run tool paths to assert receipt.attempts serialization or the exact web_run recovery wording.
| project_json_fields(body, &fetched.content_type, requested_fields)? | ||
| } | ||
| None => None, | ||
| }; |
There was a problem hiding this comment.
Run extraction before field projection so JS-shell detection always wins, or ensure project_json_fields cannot return an error for HTML shells and add a regression test with non-empty requested_fields.
| async fn fetch_readable_inner<'e, T, F>( | ||
| url: &str, | ||
| options: &FetchOptions, | ||
| context: &ToolContext, |
There was a problem hiding this comment.
Catch fetch_inner errors on the revalidation pass after an earlier JS-shell attempt and wrap them with js_shell_failure or otherwise append the prior attempt, so the model still receives the escalation context.
Closes #5904
What was broken
A 200 whose body extracted to nothing was terminal.
crates/tui/src/tools/web/extract.rs:197,206raisedjs_required_erroron the spot, and theretriescounter incrates/tui/src/tools/web/fetch.rsonly ever covered transport failures — a 200-with-JS-shell was never re-fetched. Because an edge cache can hold a prerendered variant while an origin MISS serves the client-side shell, the same URL failed on one fetch and succeeded minutes later, and research agents lost primary sources non-deterministically.What changed
web/fetch.rsgainsfetch_readable: the single place that fetches and extracts. A 2xx that yields no readable content is re-fetched once withCache-Control: no-cacheandPragma: no-cache, skipping the session fetch cache.no-cacherather thanno-storedeliberately — shared caches still get to serve a validated copy, which is what recovers a page whose prerendered variant exists but was not the one served. Transport failures keep exactly their existing single retry and earn no extra request.fetch_url's receipt gainsattempts: per attempt the sessioncache_hit, whether it bypassed caches (cache_busted), which oneproduced_content, and the response headers that explain the cache state (age,cf-cache-status,x-nextjs-prerender,x-vercel-cache, only when present). The terminal error carries the same receipt inline, so a failure explains itself as well as a success does.ToolContext— no second registry:Feature::WebSearchplus the authority envelope'snetwork_accessdecide whetherweb.runis reachable (that same envelope is what removesweb.runviafleet::role::NETWORK_TOOL_DENYLIST), andshell_policydecides whether a shell fetch is even an option. A role that has the browse surface is told to use it; a read-only, network-denied worker is told plainly that it has neitherweb.runnor acurlfallback and should report the URL as unreadable rather than substitute a source.fetch_urlandweb_runnow go throughfetch_readable; the rawfetchentry point had no remaining callers and was removed rather than left as a second path.Naming
web.runas the escalation is accurate for this codebase and matches the evidence in the issue:web.runopen requests the same URL with a browser user-agent and a ten-megabyte budget wherefetch_urluses the codewhale UA and one megabyte, which is exactly the difference that gets the prerendered variant from a Vercel/Next origin.Verified locally
No live network calls to third-party sites; every new test drives a
wiremockmock server whose responder returns the JS shell to an ordinary request and the prerendered body to one carryingCache-Control: no-cache.Both new regression tests were shown failing without the fix (retry guard disabled:
7 passed; 2 failed):js_shell_is_refetched_past_every_cache_before_it_becomes_an_error— shell then content: succeeds, exactly one extra request, receipt shows two attempts with the secondcache_bustedandproduced_content, and the cache-state headers on both.two_shells_fail_with_the_escalation_the_calling_role_owns— shell twice: fails, no third request, error namesweb.runfor a normal role and says "not available to this role" / "cannot fall back to a shell fetch" for a read-only network-denied envelope, and carries both attempt lines plusx-vercel-cache=MISS.transport_failures_do_not_earn_a_cache_busting_refetch— a 503-then-200 costs the existing single transport retry and records one readable-fetch attempt, not two.What only a live fetch can prove
The mock proves the mechanism: we send the cache-busting headers, we re-fetch once, and a differing second response is what the caller receives. It cannot prove the rate at which real edges (Vercel, Cloudflare) actually hand back a prerendered variant on a revalidating request — that is a property of those CDNs, and the issue's own evidence (
x-vercel-cache: HIT+x-nextjs-prerender: 1on the successful fetch ofdeepgram.com/pricing) is the only sample we have. The pages named in the issue were not fetched from this branch, and thealibabacloud.comdocs app may well be a page that never has a prerendered variant, in which case the value delivered there is the second half of this change: an error that tells the model exactly which surface to escalate to, rather than one that reads as "this URL is unfetchable".🤖 Generated with Claude Code
https://claude.ai/code/session_0188XYyJaw9Mh9uSrqQBoqhm
Note
Medium Risk
Changes default HTTP fetch behavior for fetch_url and web.run (up to one additional network request per URL) and alters error text agents act on, but scope is bounded to JS-shell 2xx cases with new regression tests.
Overview
Fixes non-deterministic failures when a 200 OK returns a JavaScript app shell (empty readable body) while a prerendered variant might exist behind edge caches (#5904).
fetch_readablereplaces the old fetch-then-extract split forfetch_urlandweb.runopen. On a 2xx whose extraction fails with the new JS-shell marker, the pipeline makes one extra request withCache-Control: no-cache/Pragma: no-cache, skips the session fetch cache, and runs extraction again. Transport retries are unchanged and do not trigger this second pass.Successful
fetch_urlresponses now include a richerreceipt.attemptslist (cache hit, cache-busted, which attempt produced content, and selective cache-state headers). Terminal JS-shell errors embed the same attempt summary and role-aware recovery text (web.run, shell/curl, or “report unreadable”) derived from existingToolContextcapabilities.extract.rsexposesJS_SHELL_MARKER/is_js_shell_errorso the fetch layer can recognize shell failures without conflating transport errors. Wiremock tests cover shell-then-prerender recovery, double-shell failure messaging, and that flaky HTTP status retries stay a single readable-fetch attempt.Reviewed by Cursor Bugbot for commit 77cf192. Bugbot is set up for automated code reviews on this repo. Configure here.