stack 3/5: carry six contributor bug fixes with authorship intact - #953
stack 3/5: carry six contributor bug fixes with authorship intact#953lidge-jun wants to merge 8 commits into
Conversation
A tool_result whose content is a document block translated to an empty tool output, so routed models treated the attachment as a tool that returned nothing. Surface the same "[document: title]" marker the user-message path already emits. (cherry picked from commit 3cdf190)
Gemini usually drops the malformed call upstream, so the final chunk carries only the finishReason and the started-calls guard never fired. The turn surfaced as a clean empty completion instead of an error. MAX_TOKENS with no started call keeps its plain token-limit stop. (cherry picked from commit 6eb5903)
The streaming translator folded every summary part into one thinking block with no separator, so multi-part summaries rendered as run-on text. The JSON path already joins parts with a blank line; the stream now emits the same separator at part boundaries. (cherry picked from commit b23cc1f)
The native branch buffers the upstream compact JSON and returns it without inspecting the body, so the request log row lands with no usage. Lift usage and response metadata from the buffered body the same way the routed branch gets it through handleResponses. (cherry picked from commit 2ae3b5f)
(cherry picked from commit 88d60c3)
Kiro never returns plaintext reasoning for its Sol-family models. Its `reasoningContentEvent` carries a KMS-encrypted `redactedContent` blob, and `gpt-5.6-sol`'s `additionalModelRequestFieldsSchema` accepts only `reasoning.effort` — there is no display or summary opt-in. Kiro's own CLI replays that blob on the matching `assistantResponseMessage.reasoningContent` to preserve model reasoning across turns. The adapter read only `reasoningContentEvent.text`, which is absent on this wire, so the blob was dropped and never replayed. Every turn therefore restarted without the previous turn's reasoning. - kiro-events: parse `redactedContent`; add the previously unhandled `contextUsageEvent` (Kiro reports context pressure there, not in `metadataEvent`, which carries only `stopReason`). - Carry the blob through the existing `ocxr1:` envelope as `krc` on an envelope-only reasoning item, so it round-trips while staying invisible in the app — the same contract the hidden-thinking path already uses. - Pair it backwards: Kiro emits the event at the END of a turn, after content and tool calls, so a krc-only item belongs to the assistant turn that already closed. Folding it forward would attach turn N's blob to turn N+1. With no assistant turn to own it, the blob is dropped rather than mis-paired. - Replay it on `assistantResponseMessage.reasoningContent`. Verified against kiro-cli 2.14.1 and 2.16.0 request/response captures. (cherry picked from commit cbb5d21)
CodeRabbit caught a real defect that made the round-trip a no-op in the streaming path. Kiro sends its reasoning blob at the END of a turn, while the assistant message is still open. Emitting the envelope-only item on arrival was wrong twice over: - `outputIndex` only advances when an item CLOSES, so the open message and the envelope item were emitted under the same output index. - The envelope landed BEFORE the assistant message, and the parser pairs a krc-only item backwards, so it found no preceding assistant turn and dropped the blob as orphaned — silently defeating the fix. Both paths now stash the blob and flush it after every open item has closed: after the closes in the streaming `done` case, and after the trailing flushes in the batch path. Message phase inference is untouched, so a Kiro final answer is still classified `final_answer` rather than being force-closed as commentary. The batch path also released `bytesOf(encrypted)` through `pushOutput` without ever retaining it. It now charges the blob when stashing and lets `pushOutput` release that retained allocation, so the translator budget balances. Adds tests/kiro-reasoning-roundtrip.test.ts, which bridges adapter events and re-parses the emitted items the way Codex replays history — the end-to-end coverage the original tests lacked. Three of its five cases fail against the previous commit. Scope: verified that gpt-5.6-terra and gpt-5.6-luna return `redactedContent` exactly like gpt-5.6-sol, so the whole GPT-5.6 family was affected. Handling keys off the wire field, not the model id. (cherry picked from commit bd13d48)
…blob The batch budget assertion was vacuous twice over. It passed the budget in the wrong argument position, so `buildResponseJSON` built its own internal budget and the snapshot under test was never written to (`highWaterBytes: 0`). Even wired correctly, asserting `currentBytes >= 0` could not distinguish a leaked raw blob from the finalized items that legitimately stay retained. It now uses a 4 KB blob and asserts `currentBytes` equals EXACTLY the finalized output items' bytes — a still-retained raw blob shows up as ~4 KB of excess, and releasing bytes that were never charged shows up as a shortfall. A separate `highWaterBytes` assertion proves the blob was charged while held rather than never accounted for. Four of the file's five cases now fail against cbb5d21. Also corrects the metadataEvent description: every capture put the context percentage in `contextUsageEvent`, but the parser still accepts a finite `contextUsagePercentage` (and `tokenUsage`) from `metadataEvent` as a fallback, so documenting it as impossible was wrong. (cherry picked from commit 8610630)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Stack navigation
Review and merge bottom-up. Each PR targets the preceding stack branch, so its Files changed view contains only that layer. The layers touch disjoint files — Carried in #953, with authorship preserved: #939, #942, #943, #944, #945, #948. |
![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: 9664abdd18
ℹ️ 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".
| const clientBody = process.platform === "win32" && !needsClientRewrite | ||
| ? nativeBody | ||
| : relaySseWithFailedTail(rewrittenBody, upstream, reason => clientGone.abort(reason)); | ||
| const clientBody = relaySseWithFailedTail(rewrittenBody, upstream, reason => clientGone.abort(reason)); |
There was a problem hiding this comment.
On Windows with the default streamMode: "auto" or explicit legacy-tee, no-rewrite traffic does not enter the eager branch because MIN_FIXED_BUN_VERSION is still null; it therefore reaches this unconditional relaySseWithFailedTail JS pull wrapper. This recreates the Bun#32111 path that the removed nativeBody branch avoided, so the bundled known-bad runtime can crash or lose terminal frames. Keep the native client branch for Windows decisions where useEagerRelay is false, or only use a JS relay once the runtime is proven fixed.
AGENTS.md reference: AGENTS.md:L206-L208
Useful? React with 👍 / 👎.
| return { | ||
| feed(chunk) { | ||
| if (disposed || terminal) return new Uint8Array(0); | ||
| buffer += decoder!.decode(chunk, { stream: true }); |
There was a problem hiding this comment.
When an upstream sends a very large or never-terminated SSE event, every chunk is appended to buffer until a blank-line delimiter arrives, with no byte cap or timeout. This new boundary is now on passthrough response paths, so a stuck or hostile compatible gateway can grow proxy memory without limit and withhold all of that event from the client. Apply a bounded frame policy and fail closed or safely fall back when the cap is exceeded.
AGENTS.md reference: src/AGENTS.md:L17-L17
Useful? React with 👍 / 👎.
Stack navigation
Review and merge bottom-up. Each PR targets the preceding stack branch, so its Files changed view contains only that layer. The layers touch disjoint files — #954 needs human security review per Carried in #953, with authorship preserved: #939, #942, #943, #944, #945, #948. |
lidge-jun
changed the title
lidge-jun
changed the title
4 participants
Stack
3/3 — carried contributor bug fixes
Base:
codex/908-long-context-pricing(#952)Summary
Carries six contributor bug fixes onto the stack, unmodified, with original authorship preserved via
git cherry-pick -x. Every commit keeps its author;git log --format='%an'shows the contributor, not me.tool_resultdocument blocksMALFORMED_FUNCTION_CALLwithout a call partredactedContentreasoning blobEight commits, +604/-70 across 20 files. No content changes were made to any of them — this is a carry, not a rework. Each applied to
devcleanly with no conflict resolution, so what is reviewed here is byte-identical to what the authors wrote.Why carry rather than merge in place
These are small, focused, independently-correct fixes that were sitting unmerged behind a long review queue. Stacking them puts them on one branch with one CI run and one review pass, in the same bottom-up order as the rest of the stack.
The source PRs stay open until this lands. If a maintainer prefers to take any of them directly instead, that path is unaffected — drop the corresponding commits here and merge the original. Once this merges, each source PR can be closed as carried, with credit already recorded in the commit history rather than in a comment.
Not carried, and why
enforce-targetfailingCHANGES_REQUESTED— carrying a PR past requested changes would route around the reviewVerification
bun x tsc --noEmit— exit 0bun teston all 11 touched test files — 331 pass, 0 failbun run test— 7691 pass, 8 skip, 0 fail, 507 filesbun run privacy:scan— passedFull-suite green is what matters here: these six fixes touch Claude inbound/outbound, Google streaming, compaction, passthrough terminals, and the Kiro adapter, and nothing in the suite regressed.