fix(kiro): restore hardening and complete text turns#304
Conversation
📝 WalkthroughWalkthroughThis PR substantially revises Kiro event parsing, completion fallback, retry and OAuth behavior, provider continuation persistence, response terminal semantics, web-search streaming, credential import, and isolated test execution. It also adds extensive adapter, bridge, OAuth, persistence, and end-to-end coverage. ChangesKiro adapter and protocol flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ServerResponses
participant KiroAdapter
participant KiroUpstream
participant ResponseBridge
Client->>ServerResponses: POST /v1/responses
ServerResponses->>KiroAdapter: build and send request
KiroAdapter->>KiroUpstream: stream Kiro event frames
KiroUpstream-->>KiroAdapter: content, reasoning, tool, metadata, terminal events
KiroAdapter->>KiroUpstream: bounded completion fallback when required
KiroAdapter->>ResponseBridge: adapter events and provider state
ResponseBridge-->>Client: SSE or JSON response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)package.jsonTraceback (most recent call last): 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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: 9
🤖 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 `@docs-site/src/content/docs/guides/providers.md`:
- Around line 98-110: Synchronize the new “Kiro credential import” section from
the English providers guide into the Japanese, Korean, Russian, and Simplified
Chinese locale guides. Translate the heading and content while preserving the
environment variable names, privacy guidance, and read-only/database behavior;
alternatively mark each locale page as intentionally pending translation to
prevent drift.
In `@docs-site/src/content/docs/reference/adapters.md`:
- Around line 87-118: Synchronize the Kiro sections in the Japanese, Korean,
Russian, and Simplified Chinese adapter documentation with the current English
semantics: custom baseUrl handling and bounded canonical-endpoint fallback,
replay/refresh ownership, private completion-tool and continuation behavior,
web-search buffering, and model-specific reasoning-effort support. Preserve each
locale’s existing formatting and terminology while replacing the outdated
descriptions.
In `@scripts/test.ts`:
- Line 12: Sanitize the environment used by the test runner instead of
inheriting credential and provider-state variables through baseEnv. Update the
baseEnv setup and child-process environment construction in scripts/test.ts to
allow only safe variables or explicitly remove OPENCODEX_API_AUTH_TOKEN, OAuth
credentials, and Kiro provider database-selection variables such as
KIROCLI_DB_PATH, while preserving required test configuration. Add coverage in
tests/test-runner.test.ts verifying these variables cannot escape the temporary
test environment.
In `@src/adapters/kiro-errors.ts`:
- Around line 112-118: Update the quota classification branch in the
authoritative classifier to exclude rate-limit evidence, matching the rate-quota
disambiguation used by classifyKiroText. Ensure messages such as “requests per
minute quota exceeded” continue to the 429/rate-limit branch, while genuine
exhausted quota errors still return non-retryable insufficient_quota.
In `@src/adapters/kiro-events.ts`:
- Around line 136-150: Compute the result of parseTokenUsage once in the
metadataEvent case, store it in a local variable, and reuse that value for both
the undefined check and usage property. Apply the same compute-once pattern to
optionalString and optionalBoolean in the assistantResponseEvent,
reasoningContentEvent, and toolUseEvent cases, preserving the existing
conditional spread behavior.
In `@src/oauth/kiro-credentials.ts`:
- Around line 265-266: Replace the message-substring check in the catch block
with a dedicated error type or stable error identifier used by the fatal
selection errors thrown at the earlier validation points. Update those throw
sites and the catch guard together so fatal selection errors are always rethrown
even if their messages change, while other errors retain the existing
schema-mismatch handling.
- Around line 228-247: Update the sqliteEntries() scan around selectTokenRow(db)
so the "ambiguous" and "selected_missing" cases record their diagnostics and
failure details, then continue to the next candidate instead of throwing
immediately. Preserve the existing successful-token flow and, after all
candidates are exhausted without a usable credential, propagate an appropriate
final error consistent with the surrounding scan behavior.
In `@tests/claude-messages-endpoint.test.ts`:
- Around line 266-268: In the temporary server setup for the refusal-path test,
await the promise returned by closed.stop(true) before issuing the downstream
request. Keep closedOrigin captured before shutdown and preserve the existing
request assertions.
In `@tests/kiro-oauth.test.ts`:
- Around line 116-157: Add a focused regression test alongside the existing
KIROCLI SQLite tests for a configured KIROCLI_TOKEN_KEY that is absent from
auth_kv. Seed one valid token row, set KIROCLI_DB_PATH and a nonexistent
KIROCLI_TOKEN_KEY, then assert readKiroCliSqlite explicitly throws the
missing-selection error.
🪄 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: c3f39104-f119-4f8a-aefe-d55afda14ace
📒 Files selected for processing (53)
docs-site/src/content/docs/guides/providers.mddocs-site/src/content/docs/reference/adapters.mdpackage.jsonscripts/test.tssrc/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/cli/index.tssrc/codex/catalog.tssrc/codex/home.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/ports.tssrc/server/responses.tssrc/types.tssrc/web-search/loop.tssrc/web-search/progress-stream.tsstructure/04_transports-and-sidecars.mdtests/bridge.test.tstests/claude-management-api.test.tstests/claude-messages-endpoint.test.tstests/claude-models-discovery.test.tstests/claude-native-passthrough.test.tstests/claude-outbound.test.tstests/cli-account.test.tstests/eventstream-decoder.test.tstests/kiro-adapter.test.tstests/kiro-oauth.test.tstests/kiro-retry.test.tstests/kiro-stream.test.tstests/ports.test.tstests/provider-connection-test.test.tstests/responses-parser.test.tstests/responses-state.test.tstests/server-auth.test.tstests/server-combo-failover-e2e.test.tstests/server-kiro-completion-e2e.test.tstests/server-kiro-oauth-401-replay.test.tstests/test-runner.test.tstests/web-search-progress-stream.test.tstests/web-search.test.ts
| ### Kiro credential import | ||
|
|
||
| `ocx login kiro` searches the platform Kiro CLI stores and opens SQLite databases read-only. Two | ||
| environment variables make selection explicit without copying credentials into opencodex: | ||
|
|
||
| - `KIROCLI_DB_PATH` selects a nonstandard Kiro CLI SQLite database. The path must already exist; | ||
| opencodex does not create it or modify the database, WAL, or SHM files. | ||
| - `KIROCLI_TOKEN_KEY` selects the exact `auth_kv` token key when a database contains multiple | ||
| otherwise ambiguous token rows. A missing selection fails login instead of guessing. | ||
|
|
||
| Keep these variables and the selected database private. Do not attach database files or raw login | ||
| diagnostics to bug reports. | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -a 'providers.md' docs-site/src/content/docs -t fRepository: lidge-jun/opencodex
Length of output: 502
🏁 Script executed:
#!/bin/bash
set -euo pipefail
files=(
docs-site/src/content/docs/guides/providers.md
docs-site/src/content/docs/ja/guides/providers.md
docs-site/src/content/docs/ko/guides/providers.md
docs-site/src/content/docs/ru/guides/providers.md
docs-site/src/content/docs/zh-cn/guides/providers.md
)
for f in "${files[@]}"; do
echo "===== $f ====="
rg -n -C 3 'Kiro credential import|KIROCLI_DB_PATH|KIROCLI_TOKEN_KEY|Kiro' "$f" || true
echo
doneRepository: lidge-jun/opencodex
Length of output: 5876
Sync the new Kiro import section into the locale docs docs-site/src/content/docs/ja/guides/providers.md, docs-site/src/content/docs/ko/guides/providers.md, docs-site/src/content/docs/ru/guides/providers.md, and docs-site/src/content/docs/zh-cn/guides/providers.md still stop at the “multiple OAuth accounts” section and don’t include the new ### Kiro credential import block from docs-site/src/content/docs/guides/providers.md:98-110. Add the translated section, or mark these pages as intentionally pending translation so they don’t drift from the English source.
🤖 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 `@docs-site/src/content/docs/guides/providers.md` around lines 98 - 110,
Synchronize the new “Kiro credential import” section from the English providers
guide into the Japanese, Korean, Russian, and Simplified Chinese locale guides.
Translate the heading and content while preserving the environment variable
names, privacy guidance, and read-only/database behavior; alternatively mark
each locale page as intentionally pending translation to prevent drift.
Source: Path instructions
| - Uses the configured `baseUrl` verbatim when it is custom. A canonical | ||
| `runtime.{region}.kiro.dev` URL follows the imported credential's API region; only that canonical | ||
| shape is eligible for one bounded fallback to `q.{region}.amazonaws.com` after an endpoint, | ||
| signature, DNS, or connection failure. | ||
| - Owns replay-safe connection-reset recovery, that single eligible endpoint fallback, and one OAuth | ||
| refresh/replay after HTTP 401. The client owns throttling, timeout, and ordinary service retries; | ||
| opencodex does not multiply those policies inside the adapter. | ||
| - Its non-streaming parser drains the same event stream for the web-search loop. | ||
|
|
||
| ### Completion semantics | ||
|
|
||
| Kiro text events do not carry a dependable end-turn phase. When an ordinary client tool is present, | ||
| opencodex therefore adds a private `codex_kiro_final_answer` tool to the upstream request. Progress | ||
| text streams as commentary and cannot terminate the turn. The adapter consumes the private call, | ||
| emits its answer as final text, and never exposes the private tool to Codex or Claude Code. | ||
| When the web-search sidecar is active, this commentary still streams immediately; only the events | ||
| needed to decide whether the model requested a synthetic search remain buffered. | ||
|
|
||
| If Kiro emits progress without calling the completion tool, the adapter makes one continuation. That | ||
| single retry may finish with a validated private completion or plain final text. It cannot recurse: | ||
| an empty or reasoning-only retry is returned as retryable incomplete, while a real client tool call | ||
| keeps the turn open. If the retry only repeats the preceding commentary after whitespace | ||
| normalization, the duplicate output is suppressed while the turn still completes. Tool-free | ||
| requests retain normal text completion behavior. | ||
|
|
||
| ### Reasoning effort | ||
|
|
||
| `gpt-5.6-sol` has verified native effort support. Its selected `low`, `medium`, `high`, `xhigh`, or | ||
| `max` value is sent as `additionalModelRequestFields.reasoning.effort`. Other Kiro models currently | ||
| use emulated reasoning: opencodex converts the selected level into bounded thinking instructions in | ||
| the user content because their native effort field has not been verified. Do not interpret an | ||
| advertised effort control on those models as proof of upstream-native reasoning support. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate localized copies of the kiro adapters page and check whether they still
# reference the pre-change endpoint/completion wording.
fd -t f 'adapters.md' docs-site/src/content/docs | sort
echo "---- locale files mentioning kiro ----"
fd -t f 'adapters.md' docs-site/src/content/docs \
| rg -l --iglob '*/{ja,ko,ru,zh-cn}/*' 'kiro' || echo "no localized kiro adapter pages found"Repository: lidge-jun/opencodex
Length of output: 494
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
docs-site/src/content/docs/ja/reference/adapters.md \
docs-site/src/content/docs/ko/reference/adapters.md \
docs-site/src/content/docs/ru/reference/adapters.md \
docs-site/src/content/docs/zh-cn/reference/adapters.md
do
echo "===== $f ====="
wc -l "$f"
# Show the Kiro-related section if present, otherwise the first 220 lines.
if rg -n "Kiro|kiro|completion semantics|reasoning effort|baseUrl|fallback|401|codex_kiro_final_answer" "$f" >/tmp/rg.$$; then
sed -n '1,240p' "$f" | nl -ba | sed -n '1,240p'
else
sed -n '1,220p' "$f" | nl -ba | sed -n '1,220p'
fi
echo
done
rm -f /tmp/rg.$$Repository: lidge-jun/opencodex
Length of output: 317
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
docs-site/src/content/docs/ja/reference/adapters.md \
docs-site/src/content/docs/ko/reference/adapters.md \
docs-site/src/content/docs/ru/reference/adapters.md \
docs-site/src/content/docs/zh-cn/reference/adapters.md
do
echo "===== $f ====="
echo "-- headings / Kiro-related matches --"
grep -nEi '^(#|##|###|####|Kiro|kiro|baseUrl|fallback|401|reasoning|completion|tool|progress)' "$f" || true
echo "-- first 160 lines --"
sed -n '1,160p' "$f" | cat -n
echo
doneRepository: lidge-jun/opencodex
Length of output: 27106
Sync the Kiro section in the localized adapters docs
docs-site/src/content/docs/ja/reference/adapters.md:65-74, docs-site/src/content/docs/ko/reference/adapters.md:78-89, docs-site/src/content/docs/ru/reference/adapters.md:83-97, and docs-site/src/content/docs/zh-cn/reference/adapters.md:74-85 still describe the older Kiro behavior. Update them to match the English page’s current baseUrl/fallback, completion, and reasoning-effort semantics so the locales don’t contradict the source.
🤖 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 `@docs-site/src/content/docs/reference/adapters.md` around lines 87 - 118,
Synchronize the Kiro sections in the Japanese, Korean, Russian, and Simplified
Chinese adapter documentation with the current English semantics: custom baseUrl
handling and bounded canonical-endpoint fallback, replay/refresh ownership,
private completion-tool and continuation behavior, web-search buffering, and
model-specific reasoning-effort support. Preserve each locale’s existing
formatting and terminology while replacing the outdated descriptions.
Source: Path instructions
| } | ||
|
|
||
| export function createIsolatedTestEnvironment( | ||
| baseEnv: Record<string, string | undefined> = process.env, |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Sanitize inherited credential and provider-state variables.
baseEnv preserves the caller’s OPENCODEX_API_AUTH_TOKEN and variables such as KIROCLI_DB_PATH. Tests can therefore use the developer’s real proxy token or open a real Kiro CLI database outside the temporary root, bypassing the intended isolation. Build the child environment from an allowlist or explicitly remove authentication, OAuth, and provider database-selection variables; add coverage for this behavior in tests/test-runner.test.ts.
Also applies to: 22-28
🤖 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 `@scripts/test.ts` at line 12, Sanitize the environment used by the test runner
instead of inheriting credential and provider-state variables through baseEnv.
Update the baseEnv setup and child-process environment construction in
scripts/test.ts to allow only safe variables or explicitly remove
OPENCODEX_API_AUTH_TOKEN, OAuth credentials, and Kiro provider
database-selection variables such as KIROCLI_DB_PATH, while preserving required
test configuration. Add coverage in tests/test-runner.test.ts verifying these
variables cannot escape the temporary test environment.
| if ( | ||
| evidence.includes("insufficient_quota") | ||
| || evidence.includes("quota exhausted") | ||
| || evidence.includes("quota exceeded") | ||
| ) { | ||
| return { message, status: 429, errorType: "insufficient_quota", code: "insufficient_quota", retryable: false }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Rate-limit messages containing "quota exceeded" get misclassified as non-retryable insufficient_quota.
The quota branch here runs before the 429/rate-limit branch and matches a bare "quota exceeded" substring with no rate-limit disambiguation. Because evidence folds in the raw payload details, an upstream throttle worded like "requests per minute quota exceeded" lands in this branch and returns retryable: false, so a transient rate limit is never replayed. Note the sibling classifyKiroText deliberately avoids this with its quotaExhausted && !rateQuota guard — that guard was lost in the authoritative classifier.
🐛 Proposed fix: restore rate-quota disambiguation
+ const rateQuota = /requests?\s+per\s+(?:min|minute|second)|rpm|tpm/.test(evidence);
if (
- evidence.includes("insufficient_quota")
- || evidence.includes("quota exhausted")
- || evidence.includes("quota exceeded")
+ (evidence.includes("insufficient_quota")
+ || evidence.includes("quota exhausted")
+ || evidence.includes("quota exceeded"))
+ && !rateQuota
) {
return { message, status: 429, errorType: "insufficient_quota", code: "insufficient_quota", retryable: false };
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if ( | |
| evidence.includes("insufficient_quota") | |
| || evidence.includes("quota exhausted") | |
| || evidence.includes("quota exceeded") | |
| ) { | |
| return { message, status: 429, errorType: "insufficient_quota", code: "insufficient_quota", retryable: false }; | |
| } | |
| const rateQuota = /requests?\s+per\s+(?:min|minute|second)|rpm|tpm/.test(evidence); | |
| if ( | |
| (evidence.includes("insufficient_quota") | |
| || evidence.includes("quota exhausted") | |
| || evidence.includes("quota exceeded")) | |
| && !rateQuota | |
| ) { | |
| return { message, status: 429, errorType: "insufficient_quota", code: "insufficient_quota", retryable: false }; | |
| } |
🤖 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/adapters/kiro-errors.ts` around lines 112 - 118, Update the quota
classification branch in the authoritative classifier to exclude rate-limit
evidence, matching the rate-quota disambiguation used by classifyKiroText.
Ensure messages such as “requests per minute quota exceeded” continue to the
429/rate-limit branch, while genuine exhausted quota errors still return
non-retryable insufficient_quota.
| case "metadataEvent": { | ||
| const contextUsagePercentage = parsed.contextUsagePercentage; | ||
| if ( | ||
| contextUsagePercentage !== undefined | ||
| && (typeof contextUsagePercentage !== "number" || !Number.isFinite(contextUsagePercentage)) | ||
| ) { | ||
| return malformed(eventType, "contextUsagePercentage must be a finite number"); | ||
| } | ||
| return { | ||
| type: "metadata", | ||
| ...(parseTokenUsage(eventType, parsed.tokenUsage) !== undefined | ||
| ? { usage: parseTokenUsage(eventType, parsed.tokenUsage) } | ||
| : {}), | ||
| ...(typeof contextUsagePercentage === "number" ? { contextUsagePercentage } : {}), | ||
| }; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Redundant double-evaluation of parseTokenUsage (and the optionalString pattern) — compute once.
In the metadataEvent case (Lines 146‑147) parseTokenUsage(eventType, parsed.tokenUsage) runs twice: once in the !== undefined guard and once to produce the value. Each invocation fully re-validates five token fields and allocates a fresh OcxUsage object, so every metadata frame pays double the work. The same call-it-twice shape recurs for optionalString/optionalBoolean in the assistantResponseEvent, reasoningContentEvent, and toolUseEvent cases (Lines 99‑127). It is functionally correct (the extractors are deterministic and throw on the first call), just wasteful and harder to read.
♻️ Compute once, then spread
- case "metadataEvent": {
- const contextUsagePercentage = parsed.contextUsagePercentage;
+ case "metadataEvent": {
+ const contextUsagePercentage = parsed.contextUsagePercentage;
+ const usage = parseTokenUsage(eventType, parsed.tokenUsage);
if (
contextUsagePercentage !== undefined
&& (typeof contextUsagePercentage !== "number" || !Number.isFinite(contextUsagePercentage))
) {
return malformed(eventType, "contextUsagePercentage must be a finite number");
}
return {
type: "metadata",
- ...(parseTokenUsage(eventType, parsed.tokenUsage) !== undefined
- ? { usage: parseTokenUsage(eventType, parsed.tokenUsage) }
- : {}),
+ ...(usage !== undefined ? { usage } : {}),
...(typeof contextUsagePercentage === "number" ? { contextUsagePercentage } : {}),
};
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "metadataEvent": { | |
| const contextUsagePercentage = parsed.contextUsagePercentage; | |
| if ( | |
| contextUsagePercentage !== undefined | |
| && (typeof contextUsagePercentage !== "number" || !Number.isFinite(contextUsagePercentage)) | |
| ) { | |
| return malformed(eventType, "contextUsagePercentage must be a finite number"); | |
| } | |
| return { | |
| type: "metadata", | |
| ...(parseTokenUsage(eventType, parsed.tokenUsage) !== undefined | |
| ? { usage: parseTokenUsage(eventType, parsed.tokenUsage) } | |
| : {}), | |
| ...(typeof contextUsagePercentage === "number" ? { contextUsagePercentage } : {}), | |
| }; | |
| case "metadataEvent": { | |
| const contextUsagePercentage = parsed.contextUsagePercentage; | |
| const usage = parseTokenUsage(eventType, parsed.tokenUsage); | |
| if ( | |
| contextUsagePercentage !== undefined | |
| && (typeof contextUsagePercentage !== "number" || !Number.isFinite(contextUsagePercentage)) | |
| ) { | |
| return malformed(eventType, "contextUsagePercentage must be a finite number"); | |
| } | |
| return { | |
| type: "metadata", | |
| ...(usage !== undefined ? { usage } : {}), | |
| ...(typeof contextUsagePercentage === "number" ? { contextUsagePercentage } : {}), | |
| }; |
🤖 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/adapters/kiro-events.ts` around lines 136 - 150, Compute the result of
parseTokenUsage once in the metadataEvent case, store it in a local variable,
and reuse that value for both the undefined check and usage property. Apply the
same compute-once pattern to optionalString and optionalBoolean in the
assistantResponseEvent, reasoningContentEvent, and toolUseEvent cases,
preserving the existing conditional spread behavior.
| const row = selectTokenRow(db); | ||
| if (row === "ambiguous") { | ||
| diagnostics.push({ location, status: "token_ambiguous" }); | ||
| throw new Error("Kiro CLI credential database contains multiple tokens; set KIROCLI_TOKEN_KEY to select one"); | ||
| } | ||
| if (row === "selected_missing") { | ||
| diagnostics.push({ location, status: "token_key_missing" }); | ||
| throw new Error("The KIROCLI_TOKEN_KEY selection was not found in the Kiro CLI credential database"); | ||
| } | ||
| if (!tokenData) { | ||
| if (!row) { | ||
| diagnostics.push({ location, status: "token_missing" }); | ||
| continue; | ||
| } | ||
| let tokenData: JsonObject; | ||
| try { | ||
| tokenData = JSON.parse(row.value) as JsonObject; | ||
| } catch { | ||
| diagnostics.push({ location, status: "invalid_json" }); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Ambiguous/missing-selection errors abort the entire multi-location scan instead of trying remaining candidates.
Unlike "missing", "unreadable", "token_missing", and "invalid_json" (all of which continue to the next candidate in sqliteEntries()), the "ambiguous" (Line 229-232) and "selected_missing" (Line 233-236) branches immediately throw. When KIROCLI_DB_PATH is unset, sqliteEntries() can return up to 4 default candidate paths (mac/linux kiro-cli, amazon-q, kiro-sso-cache); if the first readable candidate happens to have ambiguous tokens (or lacks the KIROCLI_TOKEN_KEY-selected row) while a later candidate holds a perfectly usable, unambiguous credential, login now fails hard instead of falling through — a real functional regression versus the surrounding fallback design.
Consider recording the failure and continuing the loop, only throwing after all candidates are exhausted with no successful credential:
🔧 Proposed fix
function readSqliteCredentials(diagnostics: KiroImportDiagnostic[]): ImportedKiroCredential | undefined {
+ let selectionError: Error | undefined;
for (const { location, path } of sqliteEntries()) {
...
try {
const row = selectTokenRow(db);
if (row === "ambiguous") {
diagnostics.push({ location, status: "token_ambiguous" });
- throw new Error("Kiro CLI credential database contains multiple tokens; set KIROCLI_TOKEN_KEY to select one");
+ selectionError ??= new Error("Kiro CLI credential database contains multiple tokens; set KIROCLI_TOKEN_KEY to select one");
+ continue;
}
if (row === "selected_missing") {
diagnostics.push({ location, status: "token_key_missing" });
- throw new Error("The KIROCLI_TOKEN_KEY selection was not found in the Kiro CLI credential database");
+ selectionError ??= new Error("The KIROCLI_TOKEN_KEY selection was not found in the Kiro CLI credential database");
+ continue;
}
...
} catch (error) {
- if (error instanceof Error && error.message.includes("KIROCLI_TOKEN_KEY")) throw error;
diagnostics.push({ location, status: "schema_mismatch" });
} finally {
db.close();
}
}
+ if (selectionError) throw selectionError;
return undefined;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const row = selectTokenRow(db); | |
| if (row === "ambiguous") { | |
| diagnostics.push({ location, status: "token_ambiguous" }); | |
| throw new Error("Kiro CLI credential database contains multiple tokens; set KIROCLI_TOKEN_KEY to select one"); | |
| } | |
| if (row === "selected_missing") { | |
| diagnostics.push({ location, status: "token_key_missing" }); | |
| throw new Error("The KIROCLI_TOKEN_KEY selection was not found in the Kiro CLI credential database"); | |
| } | |
| if (!tokenData) { | |
| if (!row) { | |
| diagnostics.push({ location, status: "token_missing" }); | |
| continue; | |
| } | |
| let tokenData: JsonObject; | |
| try { | |
| tokenData = JSON.parse(row.value) as JsonObject; | |
| } catch { | |
| diagnostics.push({ location, status: "invalid_json" }); | |
| continue; | |
| } | |
| function readSqliteCredentials(diagnostics: KiroImportDiagnostic[]): ImportedKiroCredential | undefined { | |
| let selectionError: Error | undefined; | |
| for (const { location, path } of sqliteEntries()) { | |
| const row = selectTokenRow(db); | |
| if (row === "ambiguous") { | |
| diagnostics.push({ location, status: "token_ambiguous" }); | |
| selectionError ??= new Error("Kiro CLI credential database contains multiple tokens; set KIROCLI_TOKEN_KEY to select one"); | |
| continue; | |
| } | |
| if (row === "selected_missing") { | |
| diagnostics.push({ location, status: "token_key_missing" }); | |
| selectionError ??= new Error("The KIROCLI_TOKEN_KEY selection was not found in the Kiro CLI credential database"); | |
| continue; | |
| } | |
| if (!row) { | |
| diagnostics.push({ location, status: "token_missing" }); | |
| continue; | |
| } | |
| let tokenData: JsonObject; | |
| try { | |
| tokenData = JSON.parse(row.value) as JsonObject; | |
| } catch { | |
| diagnostics.push({ location, status: "invalid_json" }); | |
| continue; | |
| } | |
| } | |
| if (selectionError) throw selectionError; | |
| return undefined; | |
| } |
🤖 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/oauth/kiro-credentials.ts` around lines 228 - 247, Update the
sqliteEntries() scan around selectTokenRow(db) so the "ambiguous" and
"selected_missing" cases record their diagnostics and failure details, then
continue to the next candidate instead of throwing immediately. Preserve the
existing successful-token flow and, after all candidates are exhausted without a
usable credential, propagate an appropriate final error consistent with the
surrounding scan behavior.
| } catch (error) { | ||
| if (error instanceof Error && error.message.includes("KIROCLI_TOKEN_KEY")) throw error; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Brittle substring-based error discrimination in the rethrow guard.
error.message.includes("KIROCLI_TOKEN_KEY") couples control flow to the exact wording of the errors thrown at Lines 231 and 235. Any future edit to either message (e.g. localization, wording tweak) that drops that literal substring would silently misclassify a fatal selection error as "schema_mismatch" and swallow it instead of propagating. A dedicated error type is more robust:
🔧 Proposed fix
+class KiroTokenSelectionError extends Error {}
+
function readSqliteCredentials(diagnostics: KiroImportDiagnostic[]): ImportedKiroCredential | undefined {
...
if (row === "ambiguous") {
diagnostics.push({ location, status: "token_ambiguous" });
- throw new Error("Kiro CLI credential database contains multiple tokens; set KIROCLI_TOKEN_KEY to select one");
+ throw new KiroTokenSelectionError("Kiro CLI credential database contains multiple tokens; set KIROCLI_TOKEN_KEY to select one");
}
if (row === "selected_missing") {
diagnostics.push({ location, status: "token_key_missing" });
- throw new Error("The KIROCLI_TOKEN_KEY selection was not found in the Kiro CLI credential database");
+ throw new KiroTokenSelectionError("The KIROCLI_TOKEN_KEY selection was not found in the Kiro CLI credential database");
}
...
} catch (error) {
- if (error instanceof Error && error.message.includes("KIROCLI_TOKEN_KEY")) throw error;
+ if (error instanceof KiroTokenSelectionError) throw error;
diagnostics.push({ location, status: "schema_mismatch" });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| if (error instanceof Error && error.message.includes("KIROCLI_TOKEN_KEY")) throw error; | |
| } catch (error) { | |
| if (error instanceof KiroTokenSelectionError) throw error; |
🤖 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/oauth/kiro-credentials.ts` around lines 265 - 266, Replace the
message-substring check in the catch block with a dedicated error type or stable
error identifier used by the fatal selection errors thrown at the earlier
validation points. Update those throw sites and the catch guard together so
fatal selection errors are always rethrown even if their messages change, while
other errors retain the existing schema-mismatch handling.
| const closed = Bun.serve({ port: 0, fetch: () => new Response() }); | ||
| const closedOrigin = closed.url.toString().replace(/\/$/, ""); | ||
| closed.stop(true); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File context =="
sed -n '230,310p' tests/claude-messages-endpoint.test.ts | cat -n
echo
echo "== Search for stop(true) usage =="
rg -n "stop\\(true\\)" tests src . -g '!**/node_modules/**' || true
echo
echo "== Bun type/docs hints in repo =="
rg -n "Promise<void>|stop\\(true\\)|Server.stop" . -g '!**/node_modules/**' || trueRepository: lidge-jun/opencodex
Length of output: 50375
🌐 Web query:
Bun.serve server stop(true) Promise<void> official docs
💡 Result:
In Bun, the server.stop method is used to stop the server from accepting new connections [1][2]. When you call server.stop(true), you are performing a force stop [1]. This immediately terminates all active connections, including in-flight HTTP requests and WebSocket connections, and stops the server from accepting new ones [1][3][2]. By default, calling server.stop without any arguments (or with false) performs a graceful stop, allowing existing in-flight requests and WebSocket connections to complete before the server shuts down [1][2]. The method returns a Promise, which resolves once the server has fully stopped [1][3]. Example usage: const server = Bun.serve({ fetch(req) { return new Response("Hello!"); }, }); // Force stop and close all active connections await server.stop(true);
Citations:
- 1: https://bun.sh/docs/runtime/http/server
- 2: https://bun.sh/reference/bun/Server/stop
- 3: https://bun.sh/reference/bun/Server
🌐 Web query:
Bun server stop(true) await Promise<void> docs
💡 Result:
In Bun, the server.stop method is used to stop a server from accepting new connections [1][2]. The method accepts an optional boolean argument, closeActiveConnections [2][3]. The syntax is: await server.stop(closeActiveConnections?: boolean); When called as await server.stop(true), the server immediately terminates all in-flight requests and WebSocket connections, in addition to stopping the acceptance of new connections [2][3]. By default, if you call await server.stop without arguments (or with false), the server stops accepting new connections but allows existing in-flight requests and WebSocket connections to complete, which may take some time [2][3]. The method returns a Promise that resolves once the server has stopped [2][3].
Citations:
- 1: https://bun.sh/reference/bun/Server
- 2: https://bun.sh/docs/runtime/http/server
- 3: https://bun.com/reference/bun/Server/stop
🌐 Web query:
site:bun.sh/docs server.stop(true) Promise<void>
💡 Result:
In the context of the Bun runtime, server.stop(true) is a method used to terminate a server instance [1]. The method server.stop(closeActiveConnections?: boolean): Promise stops the server from accepting new connections [1]. When you pass true as the argument (server.stop(true)), it instructs the server to immediately terminate all active connections [1]. If the argument is omitted or false (the default behavior), the server performs a graceful shutdown, allowing existing in-flight requests and WebSocket connections to complete before stopping [1]. The method returns a Promise that resolves when the server has fully stopped [1].
Citations:
Await closed.stop(true) before sending the refusal-path request.
tests/claude-messages-endpoint.test.ts:266-268 calls closed.stop(true) and immediately issues the downstream request. Bun’s stop(true) is asynchronous and resolves only after the server has fully stopped, so this can race shutdown and intermittently hit the temporary server instead of the connection-refused path.
Proposed fix
- closed.stop(true);
+ await closed.stop(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const closed = Bun.serve({ port: 0, fetch: () => new Response() }); | |
| const closedOrigin = closed.url.toString().replace(/\/$/, ""); | |
| closed.stop(true); | |
| const closed = Bun.serve({ port: 0, fetch: () => new Response() }); | |
| const closedOrigin = closed.url.toString().replace(/\/$/, ""); | |
| await closed.stop(true); |
🤖 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 `@tests/claude-messages-endpoint.test.ts` around lines 266 - 268, In the
temporary server setup for the refusal-path test, await the promise returned by
closed.stop(true) before issuing the downstream request. Keep closedOrigin
captured before shutdown and preserve the existing request assertions.
| test("KIROCLI_DB_PATH selects a nonstandard read-only database without creating missing paths", () => { | ||
| const path = join(tmp, "custom", "credentials.sqlite3"); | ||
| seedCustomTokenDb(path, [["kirocli:social:token", { access_token: "aoa-custom", refresh_token: "rt-custom" }]]); | ||
| process.env.KIROCLI_DB_PATH = path; | ||
| expect(readKiroCliSqlite()?.access).toBe("aoa-custom"); | ||
|
|
||
| const missing = join(tmp, "missing", "credentials.sqlite3"); | ||
| seedKiroCliDb({ access_token: "aoa-default-must-not-win", refresh_token: "rt-default" }); | ||
| process.env.KIROCLI_DB_PATH = missing; | ||
| expect(readKiroCliSqlite()).toBeNull(); | ||
| expect(existsSync(missing)).toBe(false); | ||
| }); | ||
|
|
||
| test("known token rows are preferred deterministically and KIROCLI_TOKEN_KEY overrides them", () => { | ||
| const path = join(tmp, "selection", "credentials.sqlite3"); | ||
| seedCustomTokenDb(path, [ | ||
| ["kirocli:social:token", { access_token: "aoa-social", refresh_token: "rt-social" }], | ||
| ["kirocli:odic:token", { access_token: "aoa-oidc", refresh_token: "rt-oidc" }], | ||
| ]); | ||
| process.env.KIROCLI_DB_PATH = path; | ||
| expect(readKiroCliSqlite()?.access).toBe("aoa-oidc"); | ||
| process.env.KIROCLI_TOKEN_KEY = "kirocli:social:token"; | ||
| expect(readKiroCliSqlite()?.access).toBe("aoa-social"); | ||
| }); | ||
|
|
||
| test("otherwise ambiguous token rows require explicit selection without leaking paths or secrets", () => { | ||
| const path = join(tmp, "ambiguous", "credentials.sqlite3"); | ||
| seedCustomTokenDb(path, [ | ||
| ["custom:a:token", { access_token: "aoa-secret-a", refresh_token: "rt-a" }], | ||
| ["custom:b:token", { access_token: "aoa-secret-b", refresh_token: "rt-b" }], | ||
| ]); | ||
| process.env.KIROCLI_DB_PATH = path; | ||
| expect(() => readKiroCliSqlite()).toThrow("set KIROCLI_TOKEN_KEY to select one"); | ||
| try { | ||
| readKiroCliSqlite(); | ||
| } catch (error) { | ||
| const message = error instanceof Error ? error.message : String(error); | ||
| expect(message).not.toContain(tmp); | ||
| expect(message).not.toContain("aoa-secret"); | ||
| } | ||
| }); | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Missing regression test for KIROCLI_TOKEN_KEY pointing at a nonexistent key.
The three new tests cover default DB-path resolution, deterministic preference + a matching KIROCLI_TOKEN_KEY override, and ambiguous-row rejection — but none set KIROCLI_TOKEN_KEY to a key absent from the seeded auth_kv rows. That leaves the "selected_missing" branch (kiro-credentials.ts lines 233-236, token_key_missing diagnostic) completely untested.
🧪 Suggested additional test
test("KIROCLI_TOKEN_KEY selecting a nonexistent key fails login explicitly", () => {
const path = join(tmp, "missing-key", "credentials.sqlite3");
seedCustomTokenDb(path, [["kirocli:social:token", { access_token: "aoa-social", refresh_token: "rt-social" }]]);
process.env.KIROCLI_DB_PATH = path;
process.env.KIROCLI_TOKEN_KEY = "kirocli:does-not-exist:token";
expect(() => readKiroCliSqlite()).toThrow("KIROCLI_TOKEN_KEY selection was not found");
});As per path instructions for tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 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 `@tests/kiro-oauth.test.ts` around lines 116 - 157, Add a focused regression
test alongside the existing KIROCLI SQLite tests for a configured
KIROCLI_TOKEN_KEY that is absent from auth_kv. Seed one valid token row, set
KIROCLI_DB_PATH and a nonexistent KIROCLI_TOKEN_KEY, then assert
readKiroCliSqlite explicitly throws the missing-selection error.
Source: Path instructions
![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: 9ca7ea3264
ℹ️ 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".
| if (currentToolCall) closeCurrentToolCall(); | ||
| if (currentWebSearch) closeCurrentWebSearch("failed", []); | ||
| const failure = adapterFailureFromMessage(event.message); | ||
| const failure = adapterFailureFromEvent(event); |
There was a problem hiding this comment.
When a streaming adapter now provides a structured event.status (Kiro 401/429/400), adapterFailureFromEvent computes failure.httpStatus but the emitted response.failed only serializes failure.error, which has no status field. The Claude translator explicitly reads response.error.status and defaults absent values to 500, so Kiro auth or rate-limit stream failures are returned to Claude Code as retryable overloaded_error instead of authentication/rate-limit errors; include the status in error/last_error when emitting the failed response.
Useful? React with 👍 / 👎.
|
|
||
| /** Providers whose upstream-401 replay path may force a snapshot refresh. */ | ||
| const FORCE_REFRESH_PROVIDERS = new Set(["xai", "github-copilot"]); | ||
| const FORCE_REFRESH_PROVIDERS = new Set(["xai", "github-copilot", "kiro"]); |
There was a problem hiding this comment.
Adding Kiro to the forced-401 replay set sends rejected local-cli credentials through the generic refresh path. For the normal Kiro login source (source: "local-cli"), refreshAndPersistAccessToken first re-imports any unexpired Kiro CLI row and returns it before calling refreshKiroToken; if that row is the same access token that just received 401 (for example a stale/revoked token with future or missing expiry), the replay uses the identical Bearer and cannot recover. Skip the local-cli shortcut on forced refresh unless the imported generation differs.
AGENTS.md reference: AGENTS.md:L65-L69
Useful? React with 👍 / 👎.
| return; | ||
| } | ||
| if (!fallback.response.ok) { | ||
| const payload = await fallback.response.text().catch(() => ""); |
There was a problem hiding this comment.
When the bounded Kiro completion retry returns a non-2xx response, this reads the raw upstream body with Response.text(). Unlike the normal Kiro HTTP error path, that has no byte or inactivity bound and buffers the whole body, so a large or stalled 401/429/5xx retry body can hang the turn or consume memory after the first attempt; use the same bounded error-body helper used elsewhere before classifying the retry failure.
Useful? React with 👍 / 👎.
| const remembered = parsed._providerContinuation?.kiro?.conversationId; | ||
| if (isValidKiroConversationId(remembered)) return remembered; |
There was a problem hiding this comment.
For a previous_response_id request that appends a new context_compaction marker, the parser sets _contextCompactionBoundary, but this code still reuses the remembered Kiro conversation id unconditionally. That lets Kiro continue its server-side conversation with the pre-compaction context even though the local history has started a compacted epoch, defeating compaction and risking old context influencing later turns; mint a fresh Kiro conversation id when the boundary flag is set.
Useful? React with 👍 / 👎.
|
Closing this restoration PR. Its stated purpose is to restore the old #302 change set after that merge became unreachable, and it now spans 53 files across Kiro OAuth, bridge, Responses state, web search, test runner, and shared types. Against current |
…tack, review doc 050) — conflicts resolved to PR-side which already contains upstream dev
…bb94ecb, security CLEAN), stack complete
…idge-jun#307/lidge-jun#309/lidge-jun#279/lidge-jun#303/lidge-jun#318/lidge-jun#319 + v2.7.34 joined with local lidge-jun#304 merge and lidge-jun#279 fixups (models-auth revert, abortable SSE decoder)
2 participants
Summary
devRuntime verification
Real
codex exec -m kiro/gpt-5.6-solprompts completed successfully through the local proxy, including the previously hanging Central Europe date prompt. The corrected path made one Kiro request instead of an unnecessary completion replay.Validation
bun run typecheckbun run privacy:scanbun run test— 3,571 passed, 0 failedReview note
This touches Kiro OAuth, credential refresh, and transport behavior, so it requires explicit security review under
MAINTAINERS.md. @Ingwannu, please review the security-sensitive paths.Summary by CodeRabbit
New Features
Bug Fixes