feat: add Kiro (AWS Agentic IDE) as a 5th provider via ACP#760
Open
dgallitelli wants to merge 8 commits into
Open
feat: add Kiro (AWS Agentic IDE) as a 5th provider via ACP#760dgallitelli wants to merge 8 commits into
dgallitelli wants to merge 8 commits into
Conversation
Adds Kiro CLI (kiro-cli, AWS's Claude-based agentic IDE) alongside Claude, Cursor, Codex, and Gemini using the new IProvider abstraction from siteboon#715. Closes siteboon#574. Backend - New `server/modules/providers/list/kiro/` module: composition root, auth (reads ~/.aws/sso/cache/kiro-auth-token.json + kiro-cli whoami), MCP (~/.kiro/settings/mcp.json with disabled/autoApprove preserved on upsert), sessions (parses ~/.kiro/sessions/cli/<id>.jsonl), and synchronizer (preserves user-set custom_name across re-syncs). - `server/kiro-cli.js`: spawns `kiro-cli acp --trust-all-tools` and drives ACP (Agent Client Protocol, https://agentclientprotocol.com) over stdio: initialize → session/new|load → session/prompt. Streams session/update notifications (agent_message_chunk, tool_call, tool_call_update) into NormalizedMessage. Model/agent are passed as spawn-time CLI flags (verified against kiro-cli 2.3.0 — they are NOT session/prompt params). - `server/modules/providers/list/kiro/stdio-jsonrpc-client.ts`: new shared JSON-RPC 2.0 stdio client (no precedent in repo). Handles partial frames, CRLF, response correlation, prefix-namespace handlers, and rejection of pending requests on child close. - Provider registry, sessions watcher, websocket dispatch, agent route, and project listing all widened to include Kiro. SessionsByProvider / ProjectListItem now expose kiroSessions end-to-end. Frontend - KiroLogo + placeholder SVGs (TODO: replace with official asset once trademark policy is confirmed). - LLMProvider union widened in src/types/app.ts. - Provider auth, chat composer, model selector, sidebar utils, command palette source, MCP constants, agent settings tab, and onboarding all updated. KIRO_MODELS in shared/modelConstants.js (Claude-only subset for v1; full catalog is dynamic via `kiro-cli chat --list-models`). Tests (31 new + 1 updated) - StdioJsonRpcClient: 13 unit tests covering frame splitting/joining, request/response correlation, error frames, notification routing, prefix wildcards, close behavior, and timeouts. - KiroSessionsProvider: 8 tests covering Prompt/AssistantMessage/ ToolResults shapes, status:'success'/'error' isError mapping, and empty-content drops. - KiroMcpProvider: 7 end-to-end MCP CRUD tests against an isolated tmp $HOME, including the disabled/autoApprove preservation regression. - KiroSessionSynchronizer: 3 integration tests against an isolated SQLite DB, including the user-renamed-then-resync regression that proved custom_name was being overwritten. - mcp.test.ts updated to include kiro in the global-adder count (5 providers, or 4 on Windows). - New `npm run test:server` script: 63 tests pass, 0 failures. i18n - messageTypes.kiro and providerSelection.readyPrompt.kiro added to all 8 locales (en, de, it, ja, ko, ru, tr, zh-CN). Verified against kiro-cli 2.3.0 with real on-disk artifacts: SQLite session store at ~/.local/share/kiro-cli/data.sqlite3, JSONL+sidecar sessions at ~/.kiro/sessions/cli/<id>.{json,jsonl}, and MCP config at ~/.kiro/settings/mcp.json. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
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:
📝 WalkthroughWalkthroughAdds Kiro (AWS CLI) as a new LLM provider. The integration includes a ChangesKiro Provider Integration
Sequence Diagram(s)sequenceDiagram
participant Browser
participant ChatInterface
participant AgentRoute
participant spawnKiro
participant StdioJsonRpcClient
participant kiro-cli
Browser->>ChatInterface: submit message with provider=kiro
ChatInterface->>AgentRoute: POST /api/agent { provider: kiro, model, message }
AgentRoute->>spawnKiro: spawnKiro(command, options, ws)
spawnKiro->>kiro-cli: spawn kiro-cli acp --trust-all-tools
spawnKiro->>StdioJsonRpcClient: attach to child stdio
StdioJsonRpcClient->>kiro-cli: initialize + session/new or session/load
kiro-cli-->>StdioJsonRpcClient: session_id
spawnKiro->>Browser: WebSocket session_created
kiro-cli->>StdioJsonRpcClient: session/update (text/tool events)
StdioJsonRpcClient->>spawnKiro: normalizeAcpUpdate → NormalizedMessage
spawnKiro->>Browser: WebSocket text / tool_use / tool_result
kiro-cli-->>spawnKiro: process close
spawnKiro->>Browser: WebSocket complete or error
Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
server/modules/providers/list/kiro/stdio-jsonrpc-client.ts (1)
221-233: ⚡ Quick winConsider logging or reporting notification handler errors.
While swallowing handler exceptions prevents them from breaking the JSON-RPC stream (good defensive design), completely silent failures make debugging handler bugs very difficult. Consider at least logging these errors to the console, or adding an optional
onHandlerErrorcallback to the constructor options.🔍 Proposed enhancement
onNotification(method: string, handler: Handler): () => void { this.handlers.set(method, handler); return () => this.handlers.delete(method); } +private handleNotificationError(method: string, params: unknown, error: unknown): void { + console.error(`[StdioJsonRpcClient] Notification handler error for ${method}:`, error); + this.options.onHandlerError?.(method, params, error); +} private dispatchFrame(frame: Record<string, unknown>): void { // ... existing code ... if (method) { const params = frame.params; const exact = this.handlers.get(method); if (exact) { try { exact(params); } catch (error) { - // Handler errors must not break the JSON-RPC stream. + this.handleNotificationError(method, params, error); } } for (const [prefix, handler] of this.prefixHandlers) { if (method.startsWith(prefix)) { try { handler(params); } catch (error) { - // Same defensive policy as above. + this.handleNotificationError(method, params, 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 `@server/modules/providers/list/kiro/stdio-jsonrpc-client.ts` around lines 221 - 233, The notification handler catch blocks for exact(...) and prefix handler loop (prefixHandlers/handler) silently swallow exceptions; modify them to report errors by either calling a provided onHandlerError callback from the constructor options (e.g., this.options.onHandlerError(error, { method, params })) or, if none supplied, fallback to console.error/processLogger.error with contextual info (method, params, handler identifier). Update the constructor to accept an optional onHandlerError and invoke it from both catch blocks so handler errors are visible while preserving the JSON-RPC stream.
🤖 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 `@package.json`:
- Line 32: The npm script "test:server" uses single quotes around the glob
pattern which fails on Windows; update the script value for test:server to use
double quotes (escaped as needed in JSON) for the glob pattern so the command
uses "server/**/*.test.ts" instead of 'server/**/*.test.ts' ensuring
cross-platform compatibility; keep the rest of the script (TSX_TSCONFIG_PATH and
node --import tsx --test) unchanged.
In
`@server/modules/providers/list/kiro/__tests__/kiro-session-synchronizer.test.ts`:
- Around line 38-43: The test teardown currently restores process.env.HOME from
ORIGINAL_HOME but only deletes HOME when ORIGINAL_HOME is undefined, leaving
process.env.USERPROFILE set to the temp path; update the teardown so both
environment variables are symmetrically restored: when ORIGINAL_HOME is defined
set both process.env.HOME and process.env.USERPROFILE to ORIGINAL_HOME, and when
ORIGINAL_HOME is undefined delete both process.env.HOME and
process.env.USERPROFILE (references: ORIGINAL_HOME, process.env.HOME,
process.env.USERPROFILE).
In `@server/modules/providers/list/kiro/kiro-sessions.provider.ts`:
- Around line 138-175: The loop over parts can produce duplicate IDs because
createNormalizedMessage uses baseId (and toolUseId) unchanged for multiple
content parts; change the for-of loop to an indexed loop (or maintain a per-part
counter) and append the index or unique suffix to the generated IDs so each part
gets a distinct id (e.g., use `${baseId}_text_${i}` for text parts and
`${toolUseId}_${i}` for toolUse/toolResult parts); update the id usages in
createNormalizedMessage calls (references: parts loop, isContentPart,
createNormalizedMessage, baseId, toolUseId, part.kind
'text'/'toolUse'/'toolResult') so keyed rendering and associations remain
stable.
In `@src/hooks/useProjectsState.ts`:
- Around line 562-576: The fallback provider normalization currently
misclassifies Kiro sessions as Claude when session payloads are missing; update
the logic that determines and sets the session provider (the code paths using
project.kiroSessions, selectedSession, setSelectedSession and the alternate
fallback block referenced at the other location) to explicitly recognize and
assign '__provider: "kiro"' for Kiro sessions instead of defaulting to 'claude'
— i.e., when you detect a Kiro session (e.g., via project.kiroSessions.find(...)
or the equivalent fallback checks), setSelectedSession should receive the
session object with __provider: 'kiro' and any other fallback normalization code
(the block around the second occurrence) must be updated the same way.
---
Nitpick comments:
In `@server/modules/providers/list/kiro/stdio-jsonrpc-client.ts`:
- Around line 221-233: The notification handler catch blocks for exact(...) and
prefix handler loop (prefixHandlers/handler) silently swallow exceptions; modify
them to report errors by either calling a provided onHandlerError callback from
the constructor options (e.g., this.options.onHandlerError(error, { method,
params })) or, if none supplied, fallback to console.error/processLogger.error
with contextual info (method, params, handler identifier). Update the
constructor to accept an optional onHandlerError and invoke it from both catch
blocks so handler errors are visible while preserving the JSON-RPC stream.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: fb2bb907-1ad0-4917-a191-72fca865a896
⛔ Files ignored due to path filters (2)
public/icons/kiro-white.svgis excluded by!**/*.svgpublic/icons/kiro.svgis excluded by!**/*.svg
📒 Files selected for processing (54)
package.jsonserver/index.jsserver/kiro-cli.jsserver/modules/database/index.tsserver/modules/projects/services/project-management.service.tsserver/modules/projects/services/projects-with-sessions-fetch.service.tsserver/modules/providers/list/kiro/__tests__/fixtures/error-result.jsonlserver/modules/providers/list/kiro/__tests__/fixtures/sample-session.jsonserver/modules/providers/list/kiro/__tests__/fixtures/sample-session.jsonlserver/modules/providers/list/kiro/__tests__/kiro-mcp.provider.test.tsserver/modules/providers/list/kiro/__tests__/kiro-session-synchronizer.test.tsserver/modules/providers/list/kiro/__tests__/kiro-sessions.provider.test.tsserver/modules/providers/list/kiro/__tests__/stdio-jsonrpc-client.test.tsserver/modules/providers/list/kiro/kiro-auth.provider.tsserver/modules/providers/list/kiro/kiro-mcp.provider.tsserver/modules/providers/list/kiro/kiro-session-synchronizer.provider.tsserver/modules/providers/list/kiro/kiro-sessions.provider.tsserver/modules/providers/list/kiro/kiro.provider.tsserver/modules/providers/list/kiro/stdio-jsonrpc-client.tsserver/modules/providers/provider.registry.tsserver/modules/providers/services/session-synchronizer.service.tsserver/modules/providers/services/sessions-watcher.service.tsserver/modules/providers/tests/mcp.test.tsserver/modules/websocket/services/chat-websocket.service.tsserver/routes/agent.jsserver/shared/types.tsshared/modelConstants.jssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useChatProviderState.tssrc/components/chat/view/ChatInterface.tsxsrc/components/chat/view/subcomponents/ChatMessagesPane.tsxsrc/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsxsrc/components/command-palette/sources/useSessionsSource.tssrc/components/llm-logo-provider/KiroLogo.tsxsrc/components/llm-logo-provider/SessionProviderLogo.tsxsrc/components/mcp/constants.tssrc/components/provider-auth/types.tssrc/components/provider-auth/view/ProviderLoginModal.tsxsrc/components/settings/constants/constants.tssrc/components/settings/view/tabs/agents-settings/AgentListItem.tsxsrc/components/settings/view/tabs/agents-settings/AgentsSettingsTab.tsxsrc/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsxsrc/components/settings/view/tabs/agents-settings/sections/content/AccountContent.tsxsrc/components/sidebar/utils/utils.tssrc/hooks/useProjectsState.tssrc/i18n/locales/de/chat.jsonsrc/i18n/locales/en/chat.jsonsrc/i18n/locales/it/chat.jsonsrc/i18n/locales/ja/chat.jsonsrc/i18n/locales/ko/chat.jsonsrc/i18n/locales/ru/chat.jsonsrc/i18n/locales/tr/chat.jsonsrc/i18n/locales/zh-CN/chat.jsonsrc/types/app.ts
Comment on lines
+138
to
+175
| for (const part of parts) { | ||
| if (!isContentPart(part)) { | ||
| continue; | ||
| } | ||
|
|
||
| if (part.kind === 'text') { | ||
| const text = typeof part.data === 'string' ? part.data : ''; | ||
| if (text.trim()) { | ||
| messages.push(createNormalizedMessage({ | ||
| id: `${baseId}_text`, | ||
| sessionId, | ||
| timestamp: ts, | ||
| provider: PROVIDER, | ||
| kind: 'text', | ||
| role: 'assistant', | ||
| content: text, | ||
| })); | ||
| } | ||
| continue; | ||
| } | ||
|
|
||
| if (part.kind === 'toolUse') { | ||
| const data = readObjectRecord(part.data) ?? {}; | ||
| const toolUseId = typeof data.toolUseId === 'string' ? data.toolUseId : baseId; | ||
| const toolName = typeof data.name === 'string' ? data.name : 'Unknown'; | ||
| messages.push(createNormalizedMessage({ | ||
| id: toolUseId, | ||
| sessionId, | ||
| timestamp: ts, | ||
| provider: PROVIDER, | ||
| kind: 'tool_use', | ||
| toolName, | ||
| toolInput: data.input, | ||
| toolId: toolUseId, | ||
| })); | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Ensure normalized message IDs are unique per content part.
On Line 147, Line 164, and Line 195, IDs can collide when a single entry contains multiple text, toolUse, or toolResult parts. This can break keyed rendering and message association logic.
Suggested fix
- for (const part of parts) {
+ for (const [partIndex, part] of parts.entries()) {
if (!isContentPart(part)) {
continue;
}
if (part.kind === 'text') {
const text = typeof part.data === 'string' ? part.data : '';
if (text.trim()) {
messages.push(createNormalizedMessage({
- id: `${baseId}_text`,
+ id: `${baseId}_text_${partIndex}`,
sessionId,
timestamp: ts,
provider: PROVIDER,
kind: 'text',
role: 'assistant',
content: text,
}));
}
continue;
}
if (part.kind === 'toolUse') {
const data = readObjectRecord(part.data) ?? {};
- const toolUseId = typeof data.toolUseId === 'string' ? data.toolUseId : baseId;
+ const toolUseId = typeof data.toolUseId === 'string' && data.toolUseId.trim().length > 0
+ ? data.toolUseId
+ : `${baseId}_tool_${partIndex}`;
const toolName = typeof data.name === 'string' ? data.name : 'Unknown';
messages.push(createNormalizedMessage({
id: toolUseId,
sessionId,
timestamp: ts,
provider: PROVIDER,
kind: 'tool_use',
toolName,
toolInput: data.input,
toolId: toolUseId,
}));
}
}
@@
- for (const part of parts) {
+ for (const [partIndex, part] of parts.entries()) {
if (!isContentPart(part) || part.kind !== 'toolResult') {
continue;
}
const data = readObjectRecord(part.data) ?? {};
- const toolUseId = typeof data.toolUseId === 'string' ? data.toolUseId : '';
+ const toolUseId = typeof data.toolUseId === 'string' && data.toolUseId.trim().length > 0
+ ? data.toolUseId
+ : `${baseId}_tool_${partIndex}`;
const { text, isError: contentIsError } = flattenToolResultContent(data.content);
messages.push(createNormalizedMessage({
- id: `${toolUseId || baseId}_result`,
+ id: `${toolUseId}_result_${partIndex}`,
sessionId,
timestamp: ts,
provider: PROVIDER,
kind: 'tool_result',
toolId: toolUseId,
content: text,
isError: entryIsError || contentIsError,
}));
}Also applies to: 187-204
🤖 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 `@server/modules/providers/list/kiro/kiro-sessions.provider.ts` around lines
138 - 175, The loop over parts can produce duplicate IDs because
createNormalizedMessage uses baseId (and toolUseId) unchanged for multiple
content parts; change the for-of loop to an indexed loop (or maintain a per-part
counter) and append the index or unique suffix to the generated IDs so each part
gets a distinct id (e.g., use `${baseId}_text_${i}` for text parts and
`${toolUseId}_${i}` for toolUse/toolResult parts); update the id usages in
createNormalizedMessage calls (references: parts loop, isContentPart,
createNormalizedMessage, baseId, toolUseId, part.kind
'text'/'toolUse'/'toolResult') so keyed rendering and associations remain
stable.
- useProjectsState: add 'kiro' to fallback provider normalization chain; previously a Kiro session not yet in any project payload was misclassified as Claude (Major). - kiro-sessions.provider: index part position into message ids so an entry with multiple text/toolUse/toolResult parts no longer produces colliding ids that break React keyed rendering and tool_use<->tool_result association (Major). - kiro-session-synchronizer test: also delete USERPROFILE in the env restore branch when HOME was originally unset (Minor). - package.json test:server: drop the bash-only env-prefix syntax in favor of `tsx --tsconfig server/tsconfig.json --test ...`. Cross-platform, no new dependencies (Minor). - stdio-jsonrpc-client: log notification handler errors via console.error instead of silently swallowing them; the dispatch loop still continues on handler exceptions but regressions are now visible (Nit). Tests - Added: AssistantMessage with multiple text + toolUse parts produces unique ids (regression for the colliding-id bug). - Added: ToolResults with multiple parts sharing a toolUseId produces unique ids. - Added: notification handler that throws does not break subsequent request/response correlation, AND the error is logged. 66/66 tests pass; server + client typecheck clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/modules/providers/list/kiro/__tests__/kiro-sessions.provider.test.ts (1)
1-242: ⚡ Quick winConsider loading test data from the fixture files.
The file comment (lines 1-7) states that "Fixture data was captured from a real
kiro-cli acpsession," and the AI summary confirms that fixture files were added in this PR (error-result.jsonl,sample-session.jsonl). However, all test cases inline their data rather than loading from these fixtures.Loading from the actual fixture files would improve maintainability (changes to fixture data propagate automatically) and align the implementation with the documented intent.
Example approach
You could read the JSONL fixture files and parse them into test cases, or reference specific line numbers/events from the fixtures. For example:
import { readFileSync } from 'node:fs'; import { join } from 'node:path'; const sampleSession = readFileSync( join(__dirname, 'fixtures/sample-session.jsonl'), 'utf-8' ) .split('\n') .filter(Boolean) .map(JSON.parse); it('normalizes a Prompt entry to a single user text message', () => { const entry = sampleSession[0]; // or find by kind/message_id // ... assertions });This would ensure tests stay synchronized with the fixture data and provide better traceability to real captured events.
🤖 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 `@server/modules/providers/list/kiro/__tests__/kiro-sessions.provider.test.ts` around lines 1 - 242, Tests inline literal event objects instead of using the shipped JSONL fixtures; update the test file to load and parse the fixtures (error-result.jsonl and sample-session.jsonl) once at top (e.g., using readFileSync + split/filter/JSON.parse) into arrays, then replace each inline `entry` with the corresponding object pulled from the parsed fixture (select by message_id or kind) before calling provider.normalizeMessage; ensure the tests still reference the same `provider.normalizeMessage` calls and assertions but use the fixture-derived entries so changes to the JSONL fixtures automatically update test inputs.
🤖 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.
Nitpick comments:
In `@server/modules/providers/list/kiro/__tests__/kiro-sessions.provider.test.ts`:
- Around line 1-242: Tests inline literal event objects instead of using the
shipped JSONL fixtures; update the test file to load and parse the fixtures
(error-result.jsonl and sample-session.jsonl) once at top (e.g., using
readFileSync + split/filter/JSON.parse) into arrays, then replace each inline
`entry` with the corresponding object pulled from the parsed fixture (select by
message_id or kind) before calling provider.normalizeMessage; ensure the tests
still reference the same `provider.normalizeMessage` calls and assertions but
use the fixture-derived entries so changes to the JSONL fixtures automatically
update test inputs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aa502bac-0449-44f3-867c-c3d2e562f27d
📒 Files selected for processing (7)
package.jsonserver/modules/providers/list/kiro/__tests__/kiro-session-synchronizer.test.tsserver/modules/providers/list/kiro/__tests__/kiro-sessions.provider.test.tsserver/modules/providers/list/kiro/__tests__/stdio-jsonrpc-client.test.tsserver/modules/providers/list/kiro/kiro-sessions.provider.tsserver/modules/providers/list/kiro/stdio-jsonrpc-client.tssrc/hooks/useProjectsState.ts
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- server/modules/providers/list/kiro/tests/stdio-jsonrpc-client.test.ts
- server/modules/providers/list/kiro/tests/kiro-session-synchronizer.test.ts
- server/modules/providers/list/kiro/kiro-sessions.provider.ts
- server/modules/providers/list/kiro/stdio-jsonrpc-client.ts
- src/hooks/useProjectsState.ts
|
Wow, this is exactly the feature I needed—it's awesome! |
Member
|
Hey @dgallitelli, thank for the updated PR!! Apologies for the delay in review. There has been a ton of changes since this PR from many contributors. Can you resolve the merge conflicts and I will make sure to review again as a priority. |
Resolve conflicts from upstream provider-architecture refactors (siteboon#762, siteboon#715): - Adopt per-provider model architecture (IProviderModels): migrate KIRO_MODELS from the deleted shared/modelConstants.js into a new kiro-models.provider.ts; frontend now reads Kiro models via the /api/providers/kiro/models catalog like every other provider. - Add kiro-skills.provider.ts to satisfy the new required `skills` facet on AbstractProvider; wire models + skills into kiro.provider.ts. - Kiro is now the 6th provider alongside opencode (added upstream). Updated all provider lists/unions/records to include both opencode and kiro (registry, routes, websocket, sessions, MCP constants, agents settings, provider grid, i18n, api-docs PROVIDER_ORDER, commands MODEL_PROVIDERS, provider.routes parseProvider). - Drop redundant closeConnection re-export now that upstream exports it. - mcp.test.ts global-adder count 4/5 -> 6. Verified: server+client tsc clean, eslint 0 errors, server tests 86 pass, client+server builds succeed, provider registry resolves all 6 Kiro facets.
…sites Adversarial review found Kiro missing from several shared provider switches that the opencode precedent (added upstream) already covers. Without these, Kiro sessions rendered as "Claude" and shared Claude's tool-settings key, leaving the messageTypes.kiro i18n keys (already in all 8 locales) unused. - MessageComponent / ChatInterface (x2): provider label ternaries now emit messageTypes.kiro instead of falling through to Claude. - ClaudeStatus PROVIDER_LABEL_KEYS + CommandResultModal PROVIDER_LABELS: add kiro. - useChatComposerState: Kiro tool-settings now persist under 'kiro-settings' instead of colliding with 'claude-settings'. - Onboarding AgentConnectionsStep: add the Kiro connection card. Intentionally NOT changed: shell-websocket (Kiro runs `kiro-cli acp` JSON-RPC, not an interactive shell) and the token-usage guard (Kiro is unsupported by design, matching the server's unsupported:true response).
…ken path Verified live against kiro-cli 2.7.0: a logged-in user was reported as authenticated:false / "OAuth token has expired" because the auth provider keyed off ~/.aws/sso/cache/kiro-auth-token.json, which kiro-cli <= 2.3.0 wrote but >= 2.7.0 no longer does (it now uses hashed-filename token files). Make `kiro-cli whoami` the source of truth for login state and parse the auth method (IdC / BuilderId) and email from its output. The legacy token file is now only a best-effort hint to enrich the method label and surface an explicit "expired" error when logged out; a missing file no longer forces a not-authenticated result. Uses plain whoami output (not --format json) for compatibility with older CLIs that lack the flag. Adds kiro-auth.provider.test.ts (the PR shipped no auth test) covering: auth from whoami with no token file (the 2.7.0 regression), auth despite an expired token file, Builder ID detection, logged-out, and the expired-token error path. Impact: chat was never affected (spawn does not gate on auth status); this fixes the Settings/Onboarding "logged out" badge shown to users on current kiro-cli.
Author
|
@blackmammoth conflicts are resolved — the PR is MERGEABLE against current Conflict resolutionCatching up to
What I changed to land Kiro on the new architecture:
Live verification against
|
| Path | Result |
|---|---|
ACP initialize → session/new → session/prompt |
✅ streamed assistant text back |
Session resume (session/load + continuation) |
✅ reloaded prior history, continued the turn |
Sessions provider parsing real ~/.kiro/sessions/cli/*.jsonl |
✅ normalized correctly |
MCP listing real ~/.kiro/settings/mcp.json + disabled/autoApprove round-trip |
✅ (write-path preservation, covered by tests) |
| Auth status detection |
Bug found + fixed: auth detection across CLI versions
The auth provider reported a logged-in user as authenticated:false / "OAuth token has expired". Root cause is version drift, not the merge: it keyed off ~/.aws/sso/cache/kiro-auth-token.json, which kiro-cli <= 2.3.0 wrote but >= 2.7.0 no longer does (it now uses hashed-filename token files).
Fix: make kiro-cli whoami the source of truth for login state (parsing method + email from its output, using the plain format for compatibility with older CLIs). The legacy token file is now only a best-effort hint for the method label / expired error and can no longer force a false negative. After the fix, status is {authenticated:true, email:…, method:"IdC"} on 2.7.0.
Scope note: this never affected chat — the spawn path doesn't gate on auth status (which is why prompts worked even with the wrong badge). It only fixed the misleading "logged out" state in Settings/Onboarding.
Also added kiro-auth.provider.test.ts (the PR previously shipped no auth test) — 5 cases including the exact 2.7.0 regression, Builder ID detection, and the logged-out/expired paths.
Checks on the merged branch
tsc (client + server) clean
eslint src/ server/ 0 errors
npm run test:server 91 pass / 0 fail (86 prior + 5 new auth tests; incl. the 6-provider MCP global-adder)
vite build + server build ok
🤖 Conflict resolution + live verification done with Claude Code.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/modules/providers/list/kiro/kiro-auth.provider.ts (1)
24-27:⚠️ Potential issue | 🟠 MajorReplace
spawn.syncwith async process spawning incheckInstalled().The synchronous process call at line 26 blocks the Node event loop for up to 5 seconds when
getStatus()is invoked (line 37). This impacts responsiveness of other async operations.Convert
checkInstalled()to return aPromise<boolean>using async spawn with proper timeout and error handling, then update line 37 toconst installed = await this.checkInstalled();🤖 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 `@server/modules/providers/list/kiro/kiro-auth.provider.ts` around lines 24 - 27, The checkInstalled() method uses the synchronous spawn.sync() call which blocks the Node event loop for up to 5 seconds. Convert checkInstalled() to return a Promise<boolean> using asynchronous process spawning instead, implement proper timeout and error handling for the async spawn operation, and update the call site in getStatus() to await the result of this.checkInstalled() to properly handle the promise.
🤖 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 `@public/api-docs.html`:
- Around line 831-838: The PROVIDER_ORDER constant has been expanded to include
all supported providers (claude, codex, gemini, cursor, opencode, kiro), but the
static request documentation that describes valid provider values has not been
updated. Update the static provider documentation to list all six supported
providers (claude, codex, gemini, cursor, opencode, kiro) so API consumers have
accurate information about which provider values are accepted. Ensure the
documentation mentions all providers that are now defined in the PROVIDER_ORDER
constant.
In `@server/routes/commands.js`:
- Around line 537-555: The command path validation in the allowlist check is
vulnerable to symlink escape attacks because it uses lexical path resolution
(path.resolve and path.relative) without resolving to canonical paths. A symlink
inside .claude/commands could point outside the allowed directories and still
pass the isUnder validation check. To fix this, resolve the commandPath to its
canonical path using the realpath function (synchronously or asynchronously as
appropriate for your code flow) before performing the isUnder validation checks
against userBase and projectBase. This ensures that even if commandPath is a
symlink, the actual target location is validated against the allowed directories
before fs.readFile reads the file.
In `@src/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsx`:
- Around line 246-248: The string "Choose a model" in the
ProviderSelectionEmptyState component is hardcoded in English, creating
inconsistency with the rest of the component which uses i18n for translations.
Replace this hardcoded string with the appropriate i18n translation call, using
the same localization method already employed elsewhere in this component to
ensure the string is properly translated for all supported locales.
---
Outside diff comments:
In `@server/modules/providers/list/kiro/kiro-auth.provider.ts`:
- Around line 24-27: The checkInstalled() method uses the synchronous
spawn.sync() call which blocks the Node event loop for up to 5 seconds. Convert
checkInstalled() to return a Promise<boolean> using asynchronous process
spawning instead, implement proper timeout and error handling for the async
spawn operation, and update the call site in getStatus() to await the result of
this.checkInstalled() to properly handle the promise.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aa18dbc0-3a5b-4a97-bb02-372fb9cae547
📒 Files selected for processing (49)
package.jsonpublic/api-docs.htmlserver/index.jsserver/modules/projects/services/project-management.service.tsserver/modules/projects/services/projects-with-sessions-fetch.service.tsserver/modules/providers/list/kiro/__tests__/kiro-auth.provider.test.tsserver/modules/providers/list/kiro/kiro-auth.provider.tsserver/modules/providers/list/kiro/kiro-models.provider.tsserver/modules/providers/list/kiro/kiro-skills.provider.tsserver/modules/providers/list/kiro/kiro.provider.tsserver/modules/providers/provider.registry.tsserver/modules/providers/provider.routes.tsserver/modules/providers/services/session-synchronizer.service.tsserver/modules/providers/services/sessions-watcher.service.tsserver/modules/providers/tests/mcp.test.tsserver/modules/websocket/services/chat-websocket.service.tsserver/routes/agent.jsserver/routes/commands.jsserver/shared/types.tssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useChatProviderState.tssrc/components/chat/view/ChatInterface.tsxsrc/components/chat/view/subcomponents/ChatMessagesPane.tsxsrc/components/chat/view/subcomponents/ClaudeStatus.tsxsrc/components/chat/view/subcomponents/CommandResultModal.tsxsrc/components/chat/view/subcomponents/MessageComponent.tsxsrc/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsxsrc/components/command-palette/sources/useSessionsSource.tssrc/components/llm-logo-provider/SessionProviderLogo.tsxsrc/components/mcp/constants.tssrc/components/onboarding/view/subcomponents/AgentConnectionsStep.tsxsrc/components/provider-auth/types.tssrc/components/provider-auth/view/ProviderLoginModal.tsxsrc/components/settings/constants/constants.tssrc/components/settings/view/tabs/agents-settings/AgentListItem.tsxsrc/components/settings/view/tabs/agents-settings/AgentsSettingsTab.tsxsrc/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsxsrc/components/settings/view/tabs/agents-settings/sections/content/AccountContent.tsxsrc/components/sidebar/utils/utils.tssrc/hooks/useProjectsState.tssrc/i18n/locales/de/chat.jsonsrc/i18n/locales/en/chat.jsonsrc/i18n/locales/it/chat.jsonsrc/i18n/locales/ja/chat.jsonsrc/i18n/locales/ko/chat.jsonsrc/i18n/locales/ru/chat.jsonsrc/i18n/locales/tr/chat.jsonsrc/i18n/locales/zh-CN/chat.jsonsrc/types/app.ts
💤 Files with no reviewable changes (8)
- src/i18n/locales/zh-CN/chat.json
- src/i18n/locales/ko/chat.json
- src/i18n/locales/ru/chat.json
- src/types/app.ts
- src/i18n/locales/tr/chat.json
- src/i18n/locales/it/chat.json
- src/i18n/locales/de/chat.json
- src/i18n/locales/ja/chat.json
✅ Files skipped from review due to trivial changes (4)
- src/components/chat/view/subcomponents/CommandResultModal.tsx
- src/components/llm-logo-provider/SessionProviderLogo.tsx
- src/components/chat/view/subcomponents/ClaudeStatus.tsx
- src/components/settings/view/tabs/agents-settings/AgentListItem.tsx
🚧 Files skipped from review as they are similar to previous changes (23)
- server/modules/providers/list/kiro/kiro.provider.ts
- server/modules/providers/services/session-synchronizer.service.ts
- src/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsx
- package.json
- src/components/chat/view/ChatInterface.tsx
- src/components/provider-auth/view/ProviderLoginModal.tsx
- src/components/sidebar/utils/utils.ts
- src/components/provider-auth/types.ts
- src/components/settings/view/tabs/agents-settings/sections/content/AccountContent.tsx
- server/modules/providers/services/sessions-watcher.service.ts
- server/modules/projects/services/project-management.service.ts
- server/routes/agent.js
- src/components/settings/constants/constants.ts
- src/components/chat/view/subcomponents/ChatMessagesPane.tsx
- server/modules/providers/provider.registry.ts
- src/components/chat/hooks/useChatProviderState.ts
- src/components/command-palette/sources/useSessionsSource.ts
- src/i18n/locales/en/chat.json
- server/modules/projects/services/projects-with-sessions-fetch.service.ts
- server/modules/websocket/services/chat-websocket.service.ts
- server/index.js
- server/shared/types.ts
- src/hooks/useProjectsState.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/modules/providers/list/kiro/kiro-auth.provider.ts (1)
24-27:⚠️ Potential issue | 🟠 MajorReplace
spawn.syncwith async process spawning incheckInstalled().The synchronous process call at line 26 blocks the Node event loop for up to 5 seconds when
getStatus()is invoked (line 37). This impacts responsiveness of other async operations.Convert
checkInstalled()to return aPromise<boolean>using async spawn with proper timeout and error handling, then update line 37 toconst installed = await this.checkInstalled();🤖 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 `@server/modules/providers/list/kiro/kiro-auth.provider.ts` around lines 24 - 27, The checkInstalled() method uses the synchronous spawn.sync() call which blocks the Node event loop for up to 5 seconds. Convert checkInstalled() to return a Promise<boolean> using asynchronous process spawning instead, implement proper timeout and error handling for the async spawn operation, and update the call site in getStatus() to await the result of this.checkInstalled() to properly handle the promise.
🤖 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 `@public/api-docs.html`:
- Around line 831-838: The PROVIDER_ORDER constant has been expanded to include
all supported providers (claude, codex, gemini, cursor, opencode, kiro), but the
static request documentation that describes valid provider values has not been
updated. Update the static provider documentation to list all six supported
providers (claude, codex, gemini, cursor, opencode, kiro) so API consumers have
accurate information about which provider values are accepted. Ensure the
documentation mentions all providers that are now defined in the PROVIDER_ORDER
constant.
In `@server/routes/commands.js`:
- Around line 537-555: The command path validation in the allowlist check is
vulnerable to symlink escape attacks because it uses lexical path resolution
(path.resolve and path.relative) without resolving to canonical paths. A symlink
inside .claude/commands could point outside the allowed directories and still
pass the isUnder validation check. To fix this, resolve the commandPath to its
canonical path using the realpath function (synchronously or asynchronously as
appropriate for your code flow) before performing the isUnder validation checks
against userBase and projectBase. This ensures that even if commandPath is a
symlink, the actual target location is validated against the allowed directories
before fs.readFile reads the file.
In `@src/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsx`:
- Around line 246-248: The string "Choose a model" in the
ProviderSelectionEmptyState component is hardcoded in English, creating
inconsistency with the rest of the component which uses i18n for translations.
Replace this hardcoded string with the appropriate i18n translation call, using
the same localization method already employed elsewhere in this component to
ensure the string is properly translated for all supported locales.
---
Outside diff comments:
In `@server/modules/providers/list/kiro/kiro-auth.provider.ts`:
- Around line 24-27: The checkInstalled() method uses the synchronous
spawn.sync() call which blocks the Node event loop for up to 5 seconds. Convert
checkInstalled() to return a Promise<boolean> using asynchronous process
spawning instead, implement proper timeout and error handling for the async
spawn operation, and update the call site in getStatus() to await the result of
this.checkInstalled() to properly handle the promise.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: aa18dbc0-3a5b-4a97-bb02-372fb9cae547
📒 Files selected for processing (49)
package.jsonpublic/api-docs.htmlserver/index.jsserver/modules/projects/services/project-management.service.tsserver/modules/projects/services/projects-with-sessions-fetch.service.tsserver/modules/providers/list/kiro/__tests__/kiro-auth.provider.test.tsserver/modules/providers/list/kiro/kiro-auth.provider.tsserver/modules/providers/list/kiro/kiro-models.provider.tsserver/modules/providers/list/kiro/kiro-skills.provider.tsserver/modules/providers/list/kiro/kiro.provider.tsserver/modules/providers/provider.registry.tsserver/modules/providers/provider.routes.tsserver/modules/providers/services/session-synchronizer.service.tsserver/modules/providers/services/sessions-watcher.service.tsserver/modules/providers/tests/mcp.test.tsserver/modules/websocket/services/chat-websocket.service.tsserver/routes/agent.jsserver/routes/commands.jsserver/shared/types.tssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useChatProviderState.tssrc/components/chat/view/ChatInterface.tsxsrc/components/chat/view/subcomponents/ChatMessagesPane.tsxsrc/components/chat/view/subcomponents/ClaudeStatus.tsxsrc/components/chat/view/subcomponents/CommandResultModal.tsxsrc/components/chat/view/subcomponents/MessageComponent.tsxsrc/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsxsrc/components/command-palette/sources/useSessionsSource.tssrc/components/llm-logo-provider/SessionProviderLogo.tsxsrc/components/mcp/constants.tssrc/components/onboarding/view/subcomponents/AgentConnectionsStep.tsxsrc/components/provider-auth/types.tssrc/components/provider-auth/view/ProviderLoginModal.tsxsrc/components/settings/constants/constants.tssrc/components/settings/view/tabs/agents-settings/AgentListItem.tsxsrc/components/settings/view/tabs/agents-settings/AgentsSettingsTab.tsxsrc/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsxsrc/components/settings/view/tabs/agents-settings/sections/content/AccountContent.tsxsrc/components/sidebar/utils/utils.tssrc/hooks/useProjectsState.tssrc/i18n/locales/de/chat.jsonsrc/i18n/locales/en/chat.jsonsrc/i18n/locales/it/chat.jsonsrc/i18n/locales/ja/chat.jsonsrc/i18n/locales/ko/chat.jsonsrc/i18n/locales/ru/chat.jsonsrc/i18n/locales/tr/chat.jsonsrc/i18n/locales/zh-CN/chat.jsonsrc/types/app.ts
💤 Files with no reviewable changes (8)
- src/i18n/locales/zh-CN/chat.json
- src/i18n/locales/ko/chat.json
- src/i18n/locales/ru/chat.json
- src/types/app.ts
- src/i18n/locales/tr/chat.json
- src/i18n/locales/it/chat.json
- src/i18n/locales/de/chat.json
- src/i18n/locales/ja/chat.json
✅ Files skipped from review due to trivial changes (4)
- src/components/chat/view/subcomponents/CommandResultModal.tsx
- src/components/llm-logo-provider/SessionProviderLogo.tsx
- src/components/chat/view/subcomponents/ClaudeStatus.tsx
- src/components/settings/view/tabs/agents-settings/AgentListItem.tsx
🚧 Files skipped from review as they are similar to previous changes (23)
- server/modules/providers/list/kiro/kiro.provider.ts
- server/modules/providers/services/session-synchronizer.service.ts
- src/components/settings/view/tabs/agents-settings/sections/AgentSelectorSection.tsx
- package.json
- src/components/chat/view/ChatInterface.tsx
- src/components/provider-auth/view/ProviderLoginModal.tsx
- src/components/sidebar/utils/utils.ts
- src/components/provider-auth/types.ts
- src/components/settings/view/tabs/agents-settings/sections/content/AccountContent.tsx
- server/modules/providers/services/sessions-watcher.service.ts
- server/modules/projects/services/project-management.service.ts
- server/routes/agent.js
- src/components/settings/constants/constants.ts
- src/components/chat/view/subcomponents/ChatMessagesPane.tsx
- server/modules/providers/provider.registry.ts
- src/components/chat/hooks/useChatProviderState.ts
- src/components/command-palette/sources/useSessionsSource.ts
- src/i18n/locales/en/chat.json
- server/modules/projects/services/projects-with-sessions-fetch.service.ts
- server/modules/websocket/services/chat-websocket.service.ts
- server/index.js
- server/shared/types.ts
- src/hooks/useProjectsState.ts
🛑 Comments failed to post (3)
public/api-docs.html (1)
831-838:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate static provider docs to match the expanded provider list.
You added
kiro(and dynamic model loading for all providers), but the static request docs still state only Claude/Cursor/Codex (Line 492 and Line 527). This will mislead API consumers about validprovidervalues.🤖 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 `@public/api-docs.html` around lines 831 - 838, The PROVIDER_ORDER constant has been expanded to include all supported providers (claude, codex, gemini, cursor, opencode, kiro), but the static request documentation that describes valid provider values has not been updated. Update the static provider documentation to list all six supported providers (claude, codex, gemini, cursor, opencode, kiro) so API consumers have accurate information about which provider values are accepted. Ensure the documentation mentions all providers that are now defined in the PROVIDER_ORDER constant.server/routes/commands.js (1)
537-555:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden command path validation against symlink escape.
The allowlist check uses lexical paths (
path.resolve/path.relative) but not canonical paths. A symlink inside.claude/commandscan still target files outside the allowed roots and pass the current check; Line 555 then reads the attacker-controlled target.🔒 Suggested fix
- const resolvedPath = path.resolve(commandPath); - const userBase = path.resolve( - path.join(os.homedir(), ".claude", "commands"), - ); - const projectBase = context?.projectPath - ? path.resolve(path.join(context.projectPath, ".claude", "commands")) - : null; - const isUnder = (base) => { + const resolvedPath = await fs.realpath(commandPath); + const userBase = await fs + .realpath(path.join(os.homedir(), ".claude", "commands")) + .catch(() => null); + const projectBase = context?.projectPath + ? await fs + .realpath(path.join(context.projectPath, ".claude", "commands")) + .catch(() => null) + : null; + const isUnder = (base) => { + if (!base) return false; const rel = path.relative(base, resolvedPath); return rel !== "" && !rel.startsWith("..") && !path.isAbsolute(rel); }; - if (!(isUnder(userBase) || (projectBase && isUnder(projectBase)))) { + if (!(isUnder(userBase) || isUnder(projectBase))) { return res.status(403).json({ error: "Access denied", message: "Command must be in .claude/commands directory", }); } } - const content = await fs.readFile(commandPath, "utf8"); + const content = await fs.readFile(resolvedPath, "utf8");🤖 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 `@server/routes/commands.js` around lines 537 - 555, The command path validation in the allowlist check is vulnerable to symlink escape attacks because it uses lexical path resolution (path.resolve and path.relative) without resolving to canonical paths. A symlink inside .claude/commands could point outside the allowed directories and still pass the isUnder validation check. To fix this, resolve the commandPath to its canonical path using the realpath function (synchronously or asynchronously as appropriate for your code flow) before performing the isUnder validation checks against userBase and projectBase. This ensures that even if commandPath is a symlink, the actual target location is validated against the allowed directories before fs.readFile reads the file.src/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsx (1)
246-248:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winLocalize the new model-selector header strings.
These new UI literals are hardcoded English, while this component otherwise uses i18n. This introduces locale inconsistency in translated builds.
🌐 Suggested fix
- <DialogTitle>Model Selector</DialogTitle> + <DialogTitle> + {t("providerSelection.modelSelectorTitle", { defaultValue: "Model Selector" })} + </DialogTitle> <div className="border-b border-border/60 bg-muted/20 px-4 py-3"> - <p className="text-sm font-semibold text-foreground">Choose a model</p> + <p className="text-sm font-semibold text-foreground"> + {t("providerSelection.chooseModel", { defaultValue: "Choose a model" })} + </p> </div>🤖 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/components/chat/view/subcomponents/ProviderSelectionEmptyState.tsx` around lines 246 - 248, The string "Choose a model" in the ProviderSelectionEmptyState component is hardcoded in English, creating inconsistency with the rest of the component which uses i18n for translations. Replace this hardcoded string with the appropriate i18n translation call, using the same localization method already employed elsewhere in this component to ensure the string is properly translated for all supported locales.
…fety, i18n) - kiro-auth: make checkInstalled() async (was spawn.sync, which blocked the event loop up to the 5s timeout during getStatus). Mirrors this file's existing async readWhoami() pattern. Live-reverified against kiro-cli 2.7.0. - commands.js: harden custom-command path validation against symlink escape by canonicalizing commandPath AND the allowlist roots via fs.realpath before the containment check (was lexical path.resolve/relative only). A missing target now denies access. Pre-existing code from siteboon#762, surfaced in this PR's diff. - ProviderSelectionEmptyState: wrap the hardcoded "Model Selector" and "Choose a model" strings in t() with defaultValue, matching the searchModels/noModelsFound i18n convention already used in this dialog. Not changed: CodeRabbit's api-docs.html finding is a false positive — there is no static provider enumeration in that file; PROVIDER_ORDER (already updated to include kiro) is the only provider list and the model docs render from it. Verified: tsc (client+server) clean, eslint 0 errors, test:server 91/91, client+server builds ok.
Author
|
Triaged all 4 findings from CodeRabbit latest pass. 3 fixed in Fixed
False positive
Verification on the updated branch: 🤖 Fix + verification done with Claude Code. |
Resolves conflicts from upstream PR siteboon#887 (unify-websocket-2 + unified session gateway with stable IDs and a single WS protocol), which refactored exactly the websocket/session layer the Kiro provider hooks into. 11 files / 27 conflict regions. Resolution strategy: - Adopted upstream's provider-agnostic gateway wholesale where Kiro needs no special handling: the WS handler now dispatches via spawnFns/abortFns keyed by LLMProvider and resolves the provider from the DB session row, and per-provider session arrays (kiroSessions, cursorSessions, ...) are collapsed into a single flat `sessions[]` discriminated by `provider`. - Re-injected Kiro into the new registration points: * server/index.js: kiro -> spawnKiro / abortKiroSession in the new maps (dropped now-unused isKiroSessionActive/getActiveKiroSessions imports). * useChatComposerState.ts: unified chat.send envelope + kiro in the model-resolution ladder so it sends kiroModel, not claudeModel. * ChatInterface.tsx: kept kiroModel wiring alongside upstream's isLoading -> isProcessing rename. * useChatProviderState.ts: kiro in the new FALLBACK_PERMISSION_MODES map. * provider-capabilities.service.ts: new kiro capability entry (ACP/--trust-all-tools: no images, no token usage, no interactive permission requests, abort supported). * src/types/app.ts: restored 'kiro' in the frontend LLMProvider union. Also fixes a pre-existing gap the integration review surfaced (in scope): shell-websocket.service.ts fell through to launching `claude` for a Kiro session opening the Shell tab. Now launches the interactive `kiro-cli chat` REPL (with --resume-id for resume) and labels the welcome banner "Kiro". Verification: client tsc clean, server tsc clean, eslint 0 errors in touched files, 110/111 server tests pass, client+server builds green. (The 1 test failure and 2 eslint errors are pre-existing upstream issues in files this branch does not touch.)
Contributor
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 `@server/index.js`:
- Around line 121-131: The resolveProviderSessionId function retrieves sessions
using only the sessionId without validating ownership by the authenticated
caller, creating a security vulnerability where users could access sessions they
do not own. Add ownership validation checks when calling
sessionsDb.getSessionById and when accessing sessionManager.getSession to ensure
the authenticated user is the owner of the requested session, and for HTTP
requests, verify the session belongs to the specified projectId. Apply the same
ownership validation pattern to the other location mentioned at lines 1156-1163
to maintain consistent security enforcement across all session-scoped
operations.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f3530474-e789-4916-8649-546b83af34d5
📒 Files selected for processing (12)
server/index.jsserver/modules/providers/provider.routes.tsserver/modules/providers/services/provider-capabilities.service.tsserver/modules/providers/services/sessions-watcher.service.tsserver/modules/websocket/services/shell-websocket.service.tsserver/shared/types.tssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useChatProviderState.tssrc/components/chat/view/ChatInterface.tsxsrc/components/chat/view/subcomponents/ChatMessagesPane.tsxsrc/i18n/locales/en/chat.jsonsrc/types/app.ts
✅ Files skipped from review due to trivial changes (1)
- server/modules/providers/provider.routes.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/chat/view/subcomponents/ChatMessagesPane.tsx
- src/i18n/locales/en/chat.json
- src/components/chat/view/ChatInterface.tsx
- server/modules/providers/services/sessions-watcher.service.ts
- src/components/chat/hooks/useChatProviderState.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
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 `@server/index.js`:
- Around line 121-131: The resolveProviderSessionId function retrieves sessions
using only the sessionId without validating ownership by the authenticated
caller, creating a security vulnerability where users could access sessions they
do not own. Add ownership validation checks when calling
sessionsDb.getSessionById and when accessing sessionManager.getSession to ensure
the authenticated user is the owner of the requested session, and for HTTP
requests, verify the session belongs to the specified projectId. Apply the same
ownership validation pattern to the other location mentioned at lines 1156-1163
to maintain consistent security enforcement across all session-scoped
operations.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f3530474-e789-4916-8649-546b83af34d5
📒 Files selected for processing (12)
server/index.jsserver/modules/providers/provider.routes.tsserver/modules/providers/services/provider-capabilities.service.tsserver/modules/providers/services/sessions-watcher.service.tsserver/modules/websocket/services/shell-websocket.service.tsserver/shared/types.tssrc/components/chat/hooks/useChatComposerState.tssrc/components/chat/hooks/useChatProviderState.tssrc/components/chat/view/ChatInterface.tsxsrc/components/chat/view/subcomponents/ChatMessagesPane.tsxsrc/i18n/locales/en/chat.jsonsrc/types/app.ts
✅ Files skipped from review due to trivial changes (1)
- server/modules/providers/provider.routes.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- src/components/chat/view/subcomponents/ChatMessagesPane.tsx
- src/i18n/locales/en/chat.json
- src/components/chat/view/ChatInterface.tsx
- server/modules/providers/services/sessions-watcher.service.ts
- src/components/chat/hooks/useChatProviderState.ts
🛑 Comments failed to post (1)
server/index.js (1)
121-131:
⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftEnforce per-session ownership before resolving provider-native session IDs.
Both paths read sessions by id alone (
sessionsDb.getSessionById(...)) without validating that the authenticated caller owns that session (and for HTTP, that it belongs to theprojectIdin the route). This permits cross-session token-usage/resume access when a foreign session id is supplied.Based on learnings: session-scoped actions must enforce per-session ownership validation across WebSocket and HTTP transports.
Also applies to: 1156-1163
🤖 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 `@server/index.js` around lines 121 - 131, The resolveProviderSessionId function retrieves sessions using only the sessionId without validating ownership by the authenticated caller, creating a security vulnerability where users could access sessions they do not own. Add ownership validation checks when calling sessionsDb.getSessionById and when accessing sessionManager.getSession to ensure the authenticated user is the owner of the requested session, and for HTTP requests, verify the session belongs to the specified projectId. Apply the same ownership validation pattern to the other location mentioned at lines 1156-1163 to maintain consistent security enforcement across all session-scoped operations.Source: Learnings
Addresses CodeRabbit review on PR siteboon#760. The kiro session-synchronizer and mcp provider tests captured only the original HOME and, on teardown, set USERPROFILE = ORIGINAL_HOME — conflating the two env vars. On any system where USERPROFILE differs from HOME (e.g. Windows), this corrupted USERPROFILE for subsequent tests. Capture USERPROFILE's original value separately and restore (or delete) each variable on its own, matching the robust pattern already used in kiro-auth.provider.test.ts. Also stop seeding kiro-mcp's ORIGINAL_HOME from os.homedir(); read the actual env var so the restore is faithful.
Author
|
Triaged the latest CodeRabbit findings against the post-merge code. Summary: Fixed (commit
Already resolved by the upstream merge (no action needed)
Out of scope for this PR
Verification after the fix: client |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #574 (and supersedes the earlier #575 against the pre-refactor architecture).
Adds Kiro — AWS's Claude-based agentic IDE — as a 5th provider alongside Claude, Cursor, Codex, and Gemini, using the new
IProviderabstraction from #715. Verified end-to-end againstkiro-cli2.3.0 with real on-disk artifacts.Why this is a fresh PR (not a rebase of #575)
Two refactors landed after #575 was opened (#666 and #715) that replaced the entire provider runtime architecture. A rebase would essentially be a full rewrite, so per @blackmammoth's note that's exactly what this PR is — fresh, against current
main.What's interesting about Kiro vs. the other 4 providers
Kiro is the first provider in this codebase to use ACP (Agent Client Protocol) — an open JSON-RPC 2.0 standard (agentclientprotocol.com) originated by Zed. That means Kiro:
kiro-cli acp --trust-all-tools(notkiro-cli chat --no-interactive) for streamed output. Plainchatonly emits ANSI text — no structured tool events.~/.kiro/sessions/cli/<id>.{json,jsonl}with a sidecar.jsoncarryingcwd/title. (There's a separate SQLite store at~/.local/share/kiro-cli/data.sqlite3forchat-mode sessions, but ACP'ssession/loadworks on chat-created sessions too — verified — so we read from the filesystem store and letsession/loadcover resume.)~/.aws/sso/cache/kiro-auth-token.json(AWS IAM Identity Center / Builder ID), not a JWT. Email comes fromkiro-cli whoami.~/.kiro/settings/mcp.json(flatmcpServersmap identical to Claude's, plusdisabledandautoApproveKiro-only fields that we round-trip).--model/--agentCLI flags on thekiro-cli acpspawn — not as JSON-RPC params onsession/prompt(verified — putting them on the prompt is a silent no-op).What's added
Backend
server/modules/providers/list/kiro/kiro.provider.tsserver/modules/providers/list/kiro/kiro-auth.provider.tswhoamiemail lookupserver/modules/providers/list/kiro/kiro-mcp.provider.tsdisabled/autoApproveround-tripserver/modules/providers/list/kiro/kiro-sessions.provider.tsNormalizedMessage(Prompt / AssistantMessage / ToolResults withstatus→isError)server/modules/providers/list/kiro/kiro-session-synchronizer.provider.ts~/.kiro/sessions/cli/, indexes via sidecar.json, preserves user-setcustom_nameserver/modules/providers/list/kiro/stdio-jsonrpc-client.tsserver/kiro-cli.jskiro-cli acp, drivesinitialize→session/new|load→session/prompt, normalizessession/updatenotifications, hardened error/abort pathsPlus minimal additions to:
provider.registry.ts,session-synchronizer.service.ts,sessions-watcher.service.ts,chat-websocket.service.ts,routes/agent.js,index.js,projects-with-sessions-fetch.service.ts,project-management.service.ts,database/index.ts(re-exportscloseConnection),shared/types.ts(LLMProvider union).Frontend
KiroLogo(placeholder SVG — TODO: replace with official asset once trademark policy is confirmed) + the standard provider-card wiring acrossuseChatProviderState,useChatComposerState,ChatInterface,ChatMessagesPane,ProviderSelectionEmptyState,provider-auth/types,ProviderLoginModal,mcp/constants,agents-settings/*,useProjectsState,sidebar/utils,useSessionsSource,LLMProviderunion insrc/types/app.ts, andKIRO_MODELSinshared/modelConstants.js.Tests (31 new + 1 updated)
stdio-jsonrpc-client.test.tskiro-sessions.provider.test.tsstatus:'error'→isError, empty-content drops, unknown-kind safetykiro-mcp.provider.test.tsdisabled/autoApprovepreservation regressionkiro-session-synchronizer.test.tsmcp.test.ts(existing)i18n
messageTypes.kiroandproviderSelection.readyPrompt.kiroadded to all 8 locales.Reviewed by Opus 4.7 before submission
A senior-level Opus 4.7 review of the initial implementation flagged several issues that are all fixed in this PR:
getProjectsWithSessions(hardcoded 4-providerSessionsByProvider). Plumbed end-to-end now.session/promptJSON-RPC params and silently ignored bykiro-cli. Moved to spawn-time--modelCLI flag.data.statuswas being ignored onToolResultsevents;isErroris now read from there with content-part fallback.disabled/autoApprovefields on every UI edit — now round-trips them via aKiroMcpProvider.upsertServeroverride.custom_nameon every re-sync — now mirrors Codex's "preserve user name" pattern.ws.setSessionId(placeholderKey)is called beforesession/newso the frontend can target an abort during the (potentially 30s+) MCP boot phase.visibleAgents/routes/agent.jsvalidator all updated for Kiro.bearer_token_env_varis now persisted on MCP upsert (was only being read, not written).Verification
Run against a real
kiro-cli2.3.0 install:Test plan
curl -fsSL https://cli.kiro.dev/install | bashkiro-cli loginand confirmkiro-cli whoamiprints an email~/.kiro/settings/mcp.jsonround-tripsdisabled/autoApproveif previously setkiro-cli chat ...to trigger a re-sync — confirm rename survives🤖 Generated with Claude Code
Summary by CodeRabbit