feat(kiro): harden completion and transport integration#302
Conversation
|
✅ Target branch corrected This pull request now targets The |
github-actions
Bot
changed the title
|
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:
📝 WalkthroughWalkthroughThe Kiro adapter now uses typed event decoding, structured errors, private completion-tool fallback, provider continuation state, improved endpoint recovery, stricter credential selection, and expanded Responses/Claude lifecycle propagation. Tests and documentation cover these changes. ChangesKiro adapter protocol and request flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ResponsesServer
participant KiroAdapter
participant KiroRuntime
participant ResponseState
Client->>ResponsesServer: submit response request
ResponsesServer->>ResponseState: load provider continuation state
ResponsesServer->>KiroAdapter: build and send Kiro request
KiroAdapter->>KiroRuntime: consume typed event stream
KiroRuntime-->>KiroAdapter: progress, reasoning, tools, completion events
KiroAdapter-->>ResponsesServer: terminal event and provider state
ResponsesServer->>ResponseState: persist continuation state
ResponsesServer-->>Client: completed or incomplete response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/bridge.ts`:
- Around line 780-781: Update buildResponseJSON’s error-event handling: remove
the unused errorMessage variable, assign the event’s usage to the local usage
variable, include errorEvent.retryable in the returned response, and derive
status from errorEvent presence so errors remain "failed" even when message is
empty. Keep the existing incomplete-event usage and retryable details behavior
intact.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f8d10e7-ab1a-4c17-a973-3cddeef07569
📒 Files selected for processing (35)
docs-site/src/content/docs/guides/providers.mddocs-site/src/content/docs/reference/adapters.mdsrc/adapters/kiro-constants.tssrc/adapters/kiro-errors.tssrc/adapters/kiro-events.tssrc/adapters/kiro-retry.tssrc/adapters/kiro-tools.tssrc/adapters/kiro-wire.tssrc/adapters/kiro.tssrc/bridge.tssrc/claude/outbound.tssrc/lib/eventstream-decoder.tssrc/oauth/index.tssrc/oauth/kiro-credentials.tssrc/providers/kiro-models.tssrc/responses/parser.tssrc/responses/schema.tssrc/responses/state.tssrc/server/claude-messages.tssrc/server/responses.tssrc/types.tssrc/web-search/loop.tssrc/web-search/progress-stream.tstests/bridge.test.tstests/claude-outbound.test.tstests/eventstream-decoder.test.tstests/kiro-adapter.test.tstests/kiro-oauth.test.tstests/kiro-retry.test.tstests/kiro-stream.test.tstests/responses-parser.test.tstests/responses-state.test.tstests/server-kiro-completion-e2e.test.tstests/server-kiro-oauth-401-replay.test.tstests/web-search-progress-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: 3efd463674
ℹ️ 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".
| const KNOWN_EVENT_TYPES = new Set([ | ||
| "assistantResponseEvent", | ||
| "reasoningContentEvent", | ||
| "toolUseEvent", |
There was a problem hiding this comment.
For Kiro streams that emit text/code through codeEvent, this new whitelist returns null before inspecting the JSON payload, while the previous parser accepted any event carrying a content string. Those chunks are now silently dropped; a code-heavy answer can arrive as an empty/incomplete response or miss its generated code. Add the Kiro content event variants to the known set and route them through the same content handling.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I investigated this separately and have not changed the whitelist without protocol evidence. The current local Kiro CLI, codex-kiro implementation, and official Kiro/AWS SDK sources available to this review did not expose a codeEvent variant or fixture. Unknown events are intentionally ignored without payload parsing as a fail-closed privacy boundary. If a sanitized runtime frame or authoritative schema is available, I can add the exact event type and a regression fixture.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/bridge.ts (1)
684-694: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist continuation state for explicit streaming incompletes.
This path emits
response.incompletebut never callsonCompletedResponse.src/server/responses.tspersists streaming replay/provider state only through that callback, so an incomplete Kiro turn cannot be continued even whenevent.providerStateis present. Mirror themax_tokenspath and add a provider-state regression test.Proposed fix
case "incomplete": { + const response = { + ...responseSnapshot("incomplete", finishedItems, event.endTurn), + usage: responsesUsage(event.usage), + incomplete_details: { + reason: event.reason, + ...(event.message ? { message: event.message } : {}), + ...(event.retryable !== undefined ? { retryable: event.retryable } : {}), + }, + }; + options?.onCompletedResponse?.(response, event.providerState); emit("response.incomplete", { - response: { - ... - }, + response, });As per path instructions,
src/**must avoid provider/adapter contract drift andtests/**behavior changes need focused regression coverage.🤖 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 `@src/bridge.ts` around lines 684 - 694, Update the explicit incomplete-response path in the surrounding streaming handler to invoke onCompletedResponse with the incomplete response and event.providerState, mirroring the existing max_tokens continuation flow before or alongside emitting response.incomplete. Preserve the current payload fields and add focused regression coverage verifying provider state is persisted and the incomplete Kiro turn can be continued.Source: Path instructions
src/responses/parser.ts (1)
336-368: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
OcxMessagePhaseinstead of an inline literal union.Line 337 re-declares
phase?: "commentary" | "final_answer"locally instead of importing the exportedOcxMessagePhasetype fromsrc/types.ts. Since this phase value drives the private-completion-tool fallback contract described in the PR, keeping two independent definitions in sync manually is fragile — a future addition/rename of a phase value intypes.tswon't be caught by the type checker here.♻️ Suggested fix
+import type { OcxMessagePhase } from "../types"; ... - const msg = item as { role?: string; content?: unknown; phase?: "commentary" | "final_answer" }; + const msg = item as { role?: string; content?: unknown; phase?: OcxMessagePhase };🤖 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 `@src/responses/parser.ts` around lines 336 - 368, Update the local message shape in the effectiveType === "message" handling to use the exported OcxMessagePhase type for phase instead of the inline "commentary" | "final_answer union. Import OcxMessagePhase from src/types.ts and apply it to phase while leaving the surrounding message-processing logic unchanged.src/responses/state.ts (1)
199-215: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
checkpointUsableheuristic missescustom_tool_call/tool_search_callpending turns.
normalizedProviderState.cursor.checkpointUsableis derived by checking only for an output item withtype === "function_call":normalizedProviderState.cursor.checkpointUsable = !response.output.some(item => { return !!item && typeof item === "object" && (item as { type?: unknown }).type === "function_call"; });Per the comment right above this code, the intent is: "a turn that ended with a pending client tool call produced an incomplete agent turn on the Cursor side ... so its checkpoint must not be reused." However,
src/bridge.ts'sbuildResponseJSON/flushToolCallcan terminate a turn withcustom_tool_call(freeform tools, e.g. apply_patch) ortool_search_calloutput items instead offunction_call, depending onfreeformToolNames/toolSearchToolNames. Those turns will be silently mis-markedcheckpointUsable: true, and a later turn may reuse a Cursor checkpoint that assumes a clean state while a tool result is actually still outstanding — risking corrupted/rejected continuation on the Cursor backend.🐛 Proposed fix
+ const PENDING_TOOL_CALL_TYPES = new Set(["function_call", "custom_tool_call", "tool_search_call"]); if (normalizedProviderState.cursor?.conversationId) { normalizedProviderState.cursor.checkpointUsable = !response.output.some(item => { - return !!item && typeof item === "object" && (item as { type?: unknown }).type === "function_call"; + return !!item && typeof item === "object" + && PENDING_TOOL_CALL_TYPES.has((item as { type?: unknown }).type as string); }); }Recommend also adding a regression test (near
tests/responses-state.test.ts's existingcheckpointUsablecoverage) with acustom_tool_call/tool_search_call-terminatedbuildResponseJSONoutput to lock in the fix.🤖 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 `@src/responses/state.ts` around lines 199 - 215, Update the checkpointUsable calculation in the response state flow to mark checkpoints unusable when response.output contains pending custom_tool_call or tool_search_call items, in addition to function_call. Preserve the existing behavior for clean turns and add regression coverage near the existing checkpointUsable tests using buildResponseJSON output for both tool-call types.src/types.ts (1)
404-421: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment language inconsistency for new
OcxCustomModel/customModelsdocs.Both
OcxCustomModel(Line 405) and thecustomModelsfield (Line 477) use Korean-only doc comments, while every other field inOcxConfig/OcxProviderConfigin this file is documented in English. Mixed-language comments in the same interface hurt discoverability/maintainability for contributors who can't read Korean.✏️ Suggested bilingual/English comment
-/** 사용자가 대시보드에서 직접 추가한 커스텀 모델 정의. */ +/** Custom model definition added directly by the user from the dashboard. */ export interface OcxCustomModel {Also applies to: 477-478
🤖 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 `@src/types.ts` around lines 404 - 421, Update the documentation comments for OcxCustomModel and the customModels field in OcxConfig to use English, or consistently bilingual English/Korean wording, matching the surrounding interface documentation while preserving the existing descriptions and field behavior.
🤖 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.
Outside diff comments:
In `@src/bridge.ts`:
- Around line 684-694: Update the explicit incomplete-response path in the
surrounding streaming handler to invoke onCompletedResponse with the incomplete
response and event.providerState, mirroring the existing max_tokens continuation
flow before or alongside emitting response.incomplete. Preserve the current
payload fields and add focused regression coverage verifying provider state is
persisted and the incomplete Kiro turn can be continued.
In `@src/responses/parser.ts`:
- Around line 336-368: Update the local message shape in the effectiveType ===
"message" handling to use the exported OcxMessagePhase type for phase instead of
the inline "commentary" | "final_answer union. Import OcxMessagePhase from
src/types.ts and apply it to phase while leaving the surrounding
message-processing logic unchanged.
In `@src/responses/state.ts`:
- Around line 199-215: Update the checkpointUsable calculation in the response
state flow to mark checkpoints unusable when response.output contains pending
custom_tool_call or tool_search_call items, in addition to function_call.
Preserve the existing behavior for clean turns and add regression coverage near
the existing checkpointUsable tests using buildResponseJSON output for both
tool-call types.
In `@src/types.ts`:
- Around line 404-421: Update the documentation comments for OcxCustomModel and
the customModels field in OcxConfig to use English, or consistently bilingual
English/Korean wording, matching the surrounding interface documentation while
preserving the existing descriptions and field behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c77bb9a7-be1b-4dd9-bb0b-79ade2521ba5
📒 Files selected for processing (10)
docs-site/src/content/docs/guides/providers.mdsrc/bridge.tssrc/oauth/index.tssrc/responses/parser.tssrc/responses/state.tssrc/server/responses.tssrc/types.tstests/bridge.test.tstests/responses-parser.test.tstests/responses-state.test.ts
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 520-523: Consolidate the duplicated normalized-answer comparison
logic in the Kiro response flow into a shared predicate near
normalizedKiroAnswer. Use that predicate consistently at the completionAnswer
and assistantText checks around the fallback branches, preserving their current
opposite outcomes while ensuring future normalization changes apply uniformly.
🪄 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: 77e3e5d0-345c-470a-bb0f-8bea811331ce
📒 Files selected for processing (3)
docs-site/src/content/docs/reference/adapters.mdsrc/adapters/kiro.tstests/kiro-stream.test.ts
Add provider-aware terminal state (OcxProviderContinuationState), phase-aware text deltas (OcxMessagePhase), incomplete event type in bridge SSE, structured error extraction (adapterFailureFromEvent), endTurn field, retryable flag, and non-streaming parity. Co-authored-by: Mushikingh <164845020+mushikingh@users.noreply.github.com> Ref: PR #302 NOTE: Sol review (Schrodinger) GO-WITH-FIXES (0 blockers). Post-merge improvements needed: coherent error status across bridges, memory-only store:false continuation, tighter OcxProviderContinuationState type.
2 participants
Summary
gpt-5.6-solreasoning effortcodex_kiro_final_answerprivate while enforcing explicit completion with one bounded text fallback across Codex and Claude Code/v1/responsesand/v1/messagesregression coverage plus Kiro operator documentationValidation
bun x tsc --noEmitbun run privacy:scangit diff --checkbun x astro build(97 pages)bun test --isolate ./tests/run: all Kiro tests passed; unrelated existing localhost timeout and test-loader/runtime failures remain in the wider suiteNotes
Summary by CodeRabbit
end_turn, richerincomplete/failedsemantics, and provider-scoped continuation state.