fix(web-search): retry an empty forced answer once before failing the turn - #4316
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe web-search loop adds one recovery attempt for an empty forced-answer response. The recovery request removes all tools and adds a developer nudge. Malformed calls still fail immediately, while truncated terminals remain incomplete without a retry. ChangesForced-answer recovery
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The retry behavior, tool removal, cancellation handling, and terminal responses are covered without an identified regression. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 2 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. This PR stays in draft until every box above is ticked. Hygiene✅ Deterministic PR hygiene checks passed. |
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. |
![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: 61b3bbecfb
ℹ️ 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".
| if (!split.hasMalformedToolCall && !split.hasRealToolCall && emptyAnswerRetries === 0) { | ||
| emptyAnswerRetries++; | ||
| console.warn("[web-search-loop] empty forced answer — retrying once without tools"); | ||
| yield { type: "heartbeat" }; | ||
| continue; |
There was a problem hiding this comment.
When an empty forced-answer pass ends with stopReason: "content_filter" or "max_tokens", this branch now retries it as though it were a silent normal completion. These stop reasons are deliberately rendered by bridge.ts as response.incomplete; a successful retry instead reports completion and can mask filtering or truncation, while an unsuccessful retry changes the result to a 502 and incurs another upstream call. Exclude recognized incomplete stop reasons from recovery and replay their terminal event unchanged.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
| ...(recoveringEmptyAnswer ? { options: { ...parsed.options, toolChoice: "none" as const } } : {}), | ||
| context: { ...parsed.context, messages: iterMessages, tools: forceAnswer ? toolsNoWebSearch : allTools }, |
There was a problem hiding this comment.
For a Devin-routed request that includes any ordinary client tool, setting only toolChoice: "none" does not make this an answer-only pass: src/adapters/devin.ts:232-238 unconditionally serializes parsed.context.tools and ignores toolChoice. Because the recovery still supplies toolsNoWebSearch, Devin can return a real client tool call, which the forced-answer validation accepts as successful even though this retry is intended to produce assistant text. Set the recovery request's context.tools to [] rather than relying on every adapter to implement toolChoice: "none".
Useful? React with 👍 / 👎.
리뷰 · 우선순위 61 / 80이 PR은 지금 지금 HEAD 해당 지점(대략 844–853행 근처)은 여전히 즉시 throw입니다. 이 PR은 (1) 테스트는 라인 (복구 시
PR 상태 - 아직 draft입니다. 코드 범위는 작고 회귀 테스트가 핵심 경로를 덮습니다. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed 61b3bbe against base 7a0513c. The bounded recovery is valuable, but two current-head defects need changes; I verified the existing automated findings rather than treating them as approval.
-
In src/web-search/loop.ts:877-900, recovery checks type=done and absence of text/tools, but not stopReason. A done/content_filter, refusal, or max_tokens terminal can therefore enter the same retry as a clean empty completion. The bridge's existing truncation contract treats those as explicit incomplete outcomes. Do not retry a filtered/truncated terminal as ordinary silence or turn it into success after another model call. Preserve that terminal outcome; add single-send-per-final-pass controls using the existing truncation vocabulary, alongside the clean empty-done recovery.
-
toolChoice=none does not remove context.tools. prepareIterationEvents still supplies toolsNoWebSearch, and the Devin adapter maps parsed.context.tools directly into its wire call. With a remaining ordinary client tool, the advertised answer-only recovery can still expose that tool. Clear the recovery request's tools explicitly and add a fixture containing both web_search and an ordinary tool; the current web-search-only fixture cannot distinguish the bug.
Keep the initial search/result history unchanged and test cancellation plus persistent-empty/malformed controls. Document the extra upstream attempt in the owning search/runtime documentation; 'either answers or fails' does not describe the changed retry/cost contract.
This is not fixed by increasing retry count, changing policy terminals to completed, or weakening the suite. Keep #4314 open until the corrected PR lands. No billable provider replay or local runtime changes were made during this review.
… turn An empty forced-answer pass (terminal `done`, no visible assistant text, no tool call) is retried exactly once instead of failing the whole turn with a 502. The recovery iteration appends one developer nudge and advertises NO tools at all -- an empty `context.tools` plus `toolChoice: "none"`, because adapters such as Devin serialize `context.tools` verbatim and would otherwise still expose a remaining client tool. A terminal whose stop reason is in the bridge's truncation vocabulary (`content_filter`, `refusal`, `max_tokens`, ...) is NOT silence: it is replayed unchanged so the bridge keeps reporting `response.incomplete`, and it never spends a second upstream call or turns a filtered turn into a success. Malformed calls still fail immediately, a persistent empty pass still fails, and cancellation does not consume the recovery attempt. `HARD_CAP` is `maxSearches + 2` (search rounds + the forced answer + at most one recovery pass). structure/runtime.md records the changed retry contract: a clean empty final pass can now cost one extra upstream call before the turn fails.
61b3bbe to
05e7763
Compare
|
Corrections pushed in
Verification on this head: No billable provider replay was run. Left in draft until you have verified this exact head. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05e7763253
ℹ️ 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".
| if (!truncated) { | ||
| throw new LoopError(502, "forced-answer pass produced no usable assistant output"); | ||
| } |
There was a problem hiding this comment.
When a forced pass contains a malformed tool call and ends with a recognized truncation reason such as max_tokens, truncated is true, so this condition skips the LoopError and replays the malformed stream as response.incomplete. Before this change every malformed forced answer failed regardless of its stop reason, and the hosted web-search contract in structure/runtime.md still says malformed calls fail immediately; check split.hasMalformedToolCall independently before allowing truncated terminals through.
AGENTS.md reference: structure/AGENTS.md:L9-L10
Useful? React with 👍 / 👎.
| if (!truncated && !split.hasMalformedToolCall && !split.hasRealToolCall && emptyAnswerRetries === 0) { | ||
| emptyAnswerRetries++; | ||
| console.warn("[web-search-loop] empty forced answer — retrying once without tools"); | ||
| yield { type: "heartbeat" }; | ||
| continue; |
There was a problem hiding this comment.
When the empty forced pass reports token usage and the recovery succeeds, continue discards that pass's terminal done event, including its usage; the bridge therefore emits and records only the recovery request's usage even though this change made both upstream calls billable. This underreports Responses usage and request-log cost for every such recovery, especially reasoning-only empty attempts, so retain the suppressed usage and merge it into the recovery terminal as the general empty-completion retry guard does.
Useful? React with 👍 / 👎.
|
Superseded by maintainer landing #4356 (same empty forced-answer retry + truncation replay contract on |
lidge-jun
added
the
landed-via-maintainer
3 participants
Summary
runWithWebSearchaborts the whole turn when the forced-answer pass ends with a terminaldoneand no usable assistant output:#1001 turned that case into a hard
LoopError(502, …)atsrc/web-search/loop.ts; the merged fix (#1030) took the "surface an explicit in-stream error" option, and the other option that report offered — a bounded final-answer retry — was never implemented. This PR implements it, on the terminal contract the bridge already has:context.toolsandtoolChoice: "none".toolChoicealone is not enough — adapters such as Devin putcontext.toolson the wire verbatim, so a surviving client tool would still be exposed on a pass that is meant to return text. Search results and the persistedmessagesare untouched, and searches never re-run.donewhose stop reason is in the bridge's truncation vocabulary (content_filter,refusal,max_tokens, and the raw provider spellings that map to them) is replayed unchanged, so the bridge keeps reportingresponse.incomplete. It never spends a second upstream call, never turns a filtered turn into a success, and is never upgraded toresponse.completed.HARD_CAPismaxSearches + 2: the search rounds, the forced answer, and at most one recovery pass.structure/runtime.md.Fixes #4314. Follow-up to #1001 / #1030.
Test plan
tests/web-search/web-search.test.tsdrives every path through the existingsequenceAdapter+drivePassesharness, which records each upstream request, so the number of sends is asserted rather than inferred:web_searchplus an ordinary client tool, so a regression totoolChoice-only shows up as the ordinary tool surviving into the recovery pass;content_filter,max_tokens, andrefusalterminals are replayed asresponse.incompletewith exactly 2 requests and the matchingincomplete_details.reason— the third, good answer in the fixture is never requested;Commands run on macOS 26.6.2 arm64 (Bun 1.4.2):
The four timeouts were in
responses-self-named-namespace-scrub.test.ts,ws-upstream.test.ts, andserver-auth.test.ts, none of which touch the search loop; those three files run alone give 242 pass / 0 fail. Notests/web-search/test failed in the full run.Docs
structure/runtime.mdrecords the retry contract and its cost: the forced-answer pass may be followed by one tool-less recovery request, and a truncated terminal is replayed rather than retried. The earlier description claimed the turn still "either answers or fails" — that is not true of the retry/cost contract, and the document now states the extra upstream attempt explicitly.Checklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
Summary by CodeRabbit
Bug Fixes
Documentation