fix(logs,web-search): bound response inspection and withheld search events (#4775, #4743) - #4801
Merged
Merged
Conversation
…lure Carry the held-event limits and lazy flush from luvs01#481 onto current upstream dev. Close the source review finding by routing read/limit failures through shared pre-execution leg cleanup. Add stream lifecycle regressions and document the UTF-16 accounting boundary. Source commit: 1efcfad Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Adapt luvs01#177 and #106 onto current dev. Preserve terminal ownership with bounded tee read-ahead, stream non-SSE bodies, and cover cancellation races.
Preserve the exact conflict-free GitHub merge tree for PR #4775. Upstream advanced 48 commits after the pinned implementation baseline; no response-inspection patch content or validation claims are rewritten.
Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
…570-response-lifecycle-carry
The bounded-response-inspection change wrote a literal two-character backslash-n instead of a newline at three paragraph boundaries, so structure/runtime.md, structure/transports/byte-accounting.md and structure/transports/responses.md each render a stray "\\n" glued to the start of the following heading or paragraph. Restore the intended blank-line separation. Documentation text only; no runtime, test or contract change. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
… proxy bound A flat cap of 1,000 withheld client-tool events discarded legitimate work. Every held event counts, including each response.function_call_arguments.delta for a withheld call, so a sizeable client-executed tool call streamed as fine-grained deltas -- an ordinary apply_patch, not an exotic case -- crossed the cap, failed its leg and dropped the tool call. Bounding a leak by trading it for a different user-visible failure is what this release is not allowed to do. Derive the cap from the budget that actually guards memory instead. A fine-grained argument delta serializes to roughly 128 code units, so the existing 8 MiB MAX_HELD_CALL_CHARS budget is exhausted at 65,536 such events; the count is set there and now only bounds the per-event object overhead the character budget cannot see. The derivation is stated at the constant so the next reader does not inherit another unexplained number. Also report overflow as the bridge's own admission bound. It was thrown as a plain Error into the upstream-read catch and surfaced to the client as "web-search bridge upstream read failed", blaming the provider for a refusal this proxy made. A distinct HeldCallBudgetExceededError keeps the two apart. Tests follow the exported constant rather than a hardcoded boundary, so the acceptance and refusal cases stay pinned to the real limit, and a regression asserts the overflow message is not attributed to the upstream. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
Contributor
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (29)
📝 WalkthroughWalkthroughThe pull request adds bounded response-log inspection, paced SSE inspection, and bounded web-search event buffering. It preserves streamed response bytes, records bounded terminal states, adds lifecycle tests, and documents the inspection contract. ChangesBounded streaming contracts
Estimated code review effort: 4 (Complex) | ~60 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Client
participant PassthroughDelivery
participant InspectionTee
participant ResponseLogInspector
Client->>PassthroughDelivery: request streamed response
PassthroughDelivery->>InspectionTee: tee passthrough SSE body
InspectionTee-->>Client: forward client branch
InspectionTee-->>ResponseLogInspector: paced inspection branch
Client->>PassthroughDelivery: consume or disconnect
PassthroughDelivery->>InspectionTee: release pacing on client departure
ResponseLogInspector-->>PassthroughDelivery: inspect terminal and usage state
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
|
✅ Deterministic PR hygiene checks passed. |
This was referenced Sep 16, 2026
lidge-jun
marked this pull request as ready for review
September 16, 2026 10:54
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. |
Open
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Carries #4775 and #4743 onto the current dev head with the fixes their review found. Both bound a buffer that grows without limit today; neither may drop user output to do it.
Bounded response inspection (#4775)
The change holds: createBoundedResponseLogBody and teeWithBoundedInspection each finalize exactly once across EOF, read error and cancellation, and neither awaits a tee sibling's cancel promise. The carry fixes a mechanical defect in the authoring: three structure paragraphs were written with a literal two-character backslash-n instead of a newline, so each rendered glued to the following heading. Docs text only.
Two behaviour notes recorded rather than blocked on. The non-stream JSON and error path now forwards chunk by chunk instead of buffering and emitting once, so a mid-body upstream failure can arrive after partial bytes: same failure class, different shape, and it is the point of the change. And the tee's pacing bound is one-sided, bounding inspection running ahead of the client but not the reverse; inspection is the fast side in practice.
Withheld search events (#4743)
The cleanup is right: failLegFrames closes every search cell the leg opened as failed before exactly one response.failed and one [DONE], read failure and overflow share that single path, and lazy release cannot double-emit.
The carry changes two things. The event bound was 1,000, which counts every held function_call_arguments delta, so a sizeable client-executed apply_patch streamed as fine-grained deltas could plausibly cross it and the leg would then fail and discard a legitimate tool call. Bounding memory must not cost a valid call, so the count is re-derived from the 8 MiB code-unit budget that is the actual memory guard, with the derivation stated in the comment instead of another unexplained constant. And overflow reported itself through the upstream-read-failure message, blaming the provider for a proxy-side admission bound; it now reads as what it is.
One pre-existing residual is explicitly not addressed here: decide() returns end when no search ran, before inspecting the terminal, so a leg with a client-executed call and a failed terminal still releases the held call. That predates this change and the phase doc defers it.
Verification
Checklist
Summary by CodeRabbit
New Features
Documentation