fix(kiro): round-trip the redactedContent reasoning blob - #948
fix(kiro): round-trip the redactedContent reasoning blob#948mushikingh wants to merge 5 commits into
Conversation
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.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughKiro event parsing now supports redacted reasoning and authoritative context-usage events. The adapter preserves redacted reasoning across turns. Response bridges encode it in ChangesKiro reasoning round-trip
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Kiro
participant KiroEventParser
participant KiroAdapter
participant ResponsesBridge
participant ResponsesParser
participant AssistantMessage
Kiro->>KiroEventParser: Send reasoningContentEvent or contextUsageEvent
KiroEventParser->>KiroAdapter: Emit parsed adapter event
KiroAdapter->>ResponsesBridge: Forward redacted reasoning data
ResponsesBridge->>ResponsesParser: Send krc reasoning envelope
ResponsesParser->>AssistantMessage: Attach krc to preceding assistant turn
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bridge.ts`:
- Around line 872-886: Finalize the preceding assistant output before emitting
the Kiro envelope: in src/bridge.ts lines 872-886, close any open currentMsg,
currentReasoning, currentRawReasoning, and tool-call items before allocating and
emitting the envelope-only item; in src/bridge.ts lines 1546-1554, flush
buffered text, summary reasoning, raw reasoning, and tool calls before pushing
it. Add focused streaming and batch regression tests covering text_delta →
kiro_redacted_reasoning → done, and verify parsing attaches
kiroRedactedReasoning to the preceding assistant message.
- Around line 1549-1552: Update the batch reasoning path around
encodeReasoningEnvelope and pushOutput to reserve and commit the encoded
envelope allocation before releasing it via pushOutput, matching the streaming
path’s retained-allocation lifecycle. Ensure pushOutput replaces the retained
allocation with the finalized reasoning item, and add a regression test covering
this behavior under a constrained translator budget.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 5e85ae1a-d37b-4719-96ed-64342a48d5fc
📒 Files selected for processing (10)
src/adapters/kiro-events.tssrc/adapters/kiro.tssrc/bridge.tssrc/responses/parser.tssrc/responses/reasoning-envelope.tssrc/types.tsstructure/04_transports-and-sidecars.mdtests/anthropic-thinking-signature.test.tstests/kiro-adapter.test.tstests/kiro-stream.test.ts
![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: cbb5d21ec2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| emit("response.output_item.added", { output_index: outputIndex, item }); | ||
| emit("response.output_item.done", { output_index: outputIndex, item }); |
There was a problem hiding this comment.
When a text-only Kiro turn emits visible content followed by redactedContent (the normal order described in this change), the assistant message is still open here, so this branch emits the reasoning item using the same outputIndex as the in-progress message; in the batch sibling branch the same missing flush places the reasoning item before the message, causing parseRequest's backwards attach to treat the blob as orphaned and drop it on the next turn. Flush any open message/raw reasoning/tool item before adding the Kiro envelope, and mirror that in buildResponseJSON.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| if (ev.redactedContent) { | ||
| yield* emitRetained(stage({ type: "kiro_redacted_reasoning", data: ev.redactedContent })); |
There was a problem hiding this comment.
When a tool-enabled Kiro request enters text_fallback and the bounded retry returns the private completion tool followed by redactedContent, staging this new event puts it in fallbackEvents; that terminal path yields fallbackEvents before emitting the extracted completionAnswer, so the krc-only reasoning item reaches the bridge before the final assistant message and the parser's backwards pairing drops or mis-pairs it. Hold the Kiro blob until after the extracted final answer (or attach it to that message) so Sol sessions retain reasoning after validated final-answer retries.
AGENTS.md reference: src/AGENTS.md:L19-L19
Useful? React with 👍 / 👎.
| pushOutput({ | ||
| type: "reasoning", id: `rs_${uuid()}`, summary: [], encrypted_content: encrypted, | ||
| }, bytesOf(encrypted), "reasoning"); |
There was a problem hiding this comment.
In the non-streaming path, this branch never charged encrypted before calling pushOutput, so passing bytesOf(encrypted) as replaced bytes immediately releases most of the newly retained item from the reasoning budget. A Kiro parseResponse turn with multiple large redactedContent blobs can therefore stay below the translator cap instead of failing with translation_buffer_limit; pass 0 here (unlike raw/hidden reasoning, there is no prior string buffer to replace).
Useful? React with 👍 / 👎.
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.
|
Both findings validated and fixed in bd13d48. 1. Ordering /
Rather than closing the open items on arrival, both paths now stash the blob and flush it after every open item has closed — after the closes in the streaming 2. Batch budget imbalance — confirmed.
Coverage. New Scope note. Probing the other models: Gates: |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
structure/04_transports-and-sidecars.md (1)
412-414: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the
metadataEventcontext-usage description.Lines 412-414 state that
metadataEventcarries onlystopReason.src/adapters/kiro-events.tslines 156-172 also accept and propagate a finitecontextUsagePercentagefrom that event.Describe
contextUsageEventas the authoritative source, but document themetadataEventpercentage as a supported fallback or legacy wire field. This prevents operators from treating a parsed usage value as impossible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@structure/04_transports-and-sidecars.md` around lines 412 - 414, The metadataEvent description incorrectly says it carries only stopReason; update the documentation near the Kiro event usage explanation to acknowledge a finite contextUsagePercentage as a supported fallback or legacy field, while identifying contextUsageEvent as the authoritative source.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/kiro-reasoning-roundtrip.test.ts`:
- Around line 98-115: Strengthen the regression test around buildResponseJSON so
it verifies retained accounting excludes the raw KRC blob after finalization,
rather than only checking non-negative currentBytes. Use a constrained
TranslatorBudget or an appropriate snapshot/test seam to distinguish the raw
blob allocation from finalized output allocations, and assert the post-build
state contains only the finalized reasoning item’s expected allocation with no
overflow.
---
Outside diff comments:
In `@structure/04_transports-and-sidecars.md`:
- Around line 412-414: The metadataEvent description incorrectly says it carries
only stopReason; update the documentation near the Kiro event usage explanation
to acknowledge a finite contextUsagePercentage as a supported fallback or legacy
field, while identifying contextUsageEvent as the authoritative source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 9253a02a-4dce-4d76-ab7f-897ac94b4e34
📒 Files selected for processing (4)
src/adapters/kiro-events.tssrc/bridge.tsstructure/04_transports-and-sidecars.mdtests/kiro-reasoning-roundtrip.test.ts
…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.
|
Both findings valid, fixed in 8610630. 1. Budget regression was vacuous — worse than flagged. Beyond the weak Now uses a 4 KB blob and asserts
plus Four of the file's five cases now fail against cbb5d21, up from three. 2. Correct — Gates: |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@structure/04_transports-and-sidecars.md`:
- Around line 412-417: Update the Kiro context-usage parsing logic around the
contextUsageEvent and metadataEvent assignments to track whether an
authoritative contextUsageEvent value has been received. Accept metadataEvent
values only before that authoritative value, preserve the authoritative value
regardless of event order, and add tests covering both event sequences.
In `@tests/kiro-reasoning-roundtrip.test.ts`:
- Around line 102-121: Wrap the budget setup, response construction, snapshot,
and related assertions in the test around createTranslatorBudget in a
try/finally block, and call budget.dispose() from finally. Preserve all existing
assertions while ensuring disposal occurs even when setup or an assertion fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: a54f40a6-82cb-4291-be0b-279350a682dd
📒 Files selected for processing (3)
src/adapters/kiro-events.tsstructure/04_transports-and-sidecars.mdtests/kiro-reasoning-roundtrip.test.ts
Both `contextUsageEvent` and `metadataEvent.contextUsagePercentage` assigned the same variable with last-write-wins, so arrival order alone decided which value survived and a trailing fallback frame could clobber the authoritative one. Precedence is now by source: once a contextUsageEvent value lands, later metadataEvent percentages are ignored. The fallback still works on its own. Covered in both event orders, with the discriminating values asserted to differ first so the ordering assertions cannot pass vacuously. The test fails against the unguarded assignment. Also disposes the translator budget in the batch regression via try/finally. `createTranslatorBudget` registers in module-global aggregate accounting, so an assertion failure would have leaked retained bytes into every later test.
|
Both findings valid, fixed in 3a2f057. 1. Authoritative context usage could be clobbered by the fallback. Confirmed: On the captured wire the order is Covered in both orders. The test first asserts that the 80% and 10% checkpoints are actually distinguishable and that the fallback alone still produces a value, so the ordering assertions cannot pass vacuously; it fails against the unguarded assignment. I also corrected the 2. Budget disposal. Valid — Gates: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/adapters/kiro.ts`:
- Around line 1206-1209: Update the context usage handling in
src/adapters/kiro.ts lines 1206-1209 to accept contextUsagePercentage values >=
0, including zero, and mark contextUsageIsAuthoritative when provided by the
dedicated event. In tests/kiro-stream.test.ts lines 1413-1437, add coverage for
contextUsageEvent(0) followed by a nonzero metadataEvent and assert the result
matches the no-context-percentage baseline.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 756f50c7-d253-408d-9b54-e14c25c29273
📒 Files selected for processing (4)
src/adapters/kiro.tsstructure/04_transports-and-sidecars.mdtests/kiro-reasoning-roundtrip.test.tstests/kiro-stream.test.ts
|
Carried onto the review stack as #953 (stack 3/3), unmodified. Your commits were taken with Verified on the stack: This PR stays open until #953 lands. If a maintainer prefers to take yours directly instead, that path is unaffected — the stack commits get dropped and this one merges. Once #953 merges I'll close this as carried, with the credit already in the commit history rather than in a comment. Stack: #951 (plan, base Thanks for the fix. |
…ading `contextUsageEvent` claimed authority only when its percentage was > 0, so precedence depended on the VALUE rather than the source: a genuine 0% reading (fresh conversation) left contextUsageIsAuthoritative false and a trailing metadataEvent could then override it with a stale nonzero value. Accept >= 0 and claim authority; negatives stay rejected as malformed. Nothing downstream sees a bogus zero-token checkpoint, because contextUsageTotalFloor already discards a zero floor. The ordering test now also covers contextUsageEvent(0) followed by a nonzero metadataEvent, asserting it matches the no-percentage baseline rather than the fallback's reading. It fails against the > 0 guard.
|
Valid, fixed in c3d2998. Confirmed: Now accepts I checked the downstream before widening the guard: I did not widen the Test extended with Gates: |
2 participants
Problem
Kiro never returns plaintext reasoning for its Sol-family models.
reasoningContentEventcarries a KMS-encryptedredactedContentblob, andgpt-5.6-sol'sadditionalModelRequestFieldsSchema(fromListAvailableModels) accepts onlyreasoning.effort— there is no display/summary opt-in:{"modelId":"gpt-5.6-sol", "additionalModelRequestFieldsSchema":{"type":"object","properties":{ "reasoning":{"type":"object","properties":{"effort":{"type":"string", "enum":["none","low","medium","high","xhigh","max"],"default":"high"}}}}, "additionalProperties":false}, "tokenLimits":{"maxInputTokens":272000,"maxOutputTokens":128000}}Kiro's own CLI replays that blob on the matching assistant turn to preserve reasoning across turns:
{"assistantResponseMessage":{ "messageId":"…","content":"…", "reasoningContent":{"redactedContent":"LktUUn5+…"}}}kiro-events.tsread onlyreasoningContentEvent.text, which is absent on this wire, so the blob was dropped and never replayed. Every turn restarted without the previous turn's reasoning.In a long agentic session this degrades badly. The session that prompted this fix ran 123 tool calls at
xhigheffort and ended with the model emitting a 322 KB / 162,779-line assistant message of degenerate self-talk (final.×730,no.×390,done.×311, plus lines like "I'm going in circles again") until it was interrupted.Also fixed
contextUsageEventwas not inKNOWN_EVENT_TYPES, so it was silently ignored. Kiro reports context pressure there;metadataEventcarries onlystopReason. The adapter looked forcontextUsagePercentageinsidemetadataEvent, where it never appears, so authoritative context usage never arrived.Approach
redactedContent, and add the missingcontextUsageEvent.ocxr1:envelope askrc, on an envelope-only reasoning item (summary: [], no text deltas) — it round-trips while staying invisible in the app, the same contract the hidden-thinking path already uses. Prefix-based native scrubbing already strips it for non-Kiro backends.reasoningContentEventat the END of a turn, after content and tool calls, so akrc-only item belongs to the assistant turn that already closed. Folding it forward like ordinary reasoning 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.OcxAssistantMessage.kiroRedactedReasoning, not on a thinking content part, so no other adapter replays provider-private state if the conversation switches providers.Observed event order per assistant turn (tool-calling turn, kiro-cli 2.14.1):
Verification
Request/response captures from the official
kiro-cliat 2.14.1 and 2.16.0 (viaKIRO_LOG_LEVEL=trace KIRO_LOG_STDOUT=1), covering a trivial turn, a reasoning-heavy turn, a tool-calling turn, and a resumed multi-turn conversation. Both versions produce identical wire shapes.bun run typecheck— cleanbun run test— 7625 pass, 4 fail. All 4 pre-existing on cleanupstream/dev, in files this PR does not touch (claude-desktop-cli,cli-restore-back,codex-app-server-processes); confirmed by running the same files on an unmodified worktree — identical failures.bun run privacy:scan— passedkiro-stream(redactedContent capture, text+blob together,contextUsageEvent),kiro-adapter(history replay, and omission when no blob),anthropic-thinking-signature(backwards pairing, and orphan blob dropped).Not included
meteringEventis still ignored. Kiro bills in credits, not tokens — there is notokenUsageon this wire at all, which is why Kiro usage staysestimated. Surfacing credits needs a cost field inOcxUsageplus usage-log/GUI plumbing, so it belongs in its own PR.Summary by CodeRabbit
New Features
Bug Fixes
Documentation