feat: add Kiro (AWS Agentic IDE) as a 5th provider via ACP#760
Open
dgallitelli wants to merge 2 commits into
Open
feat: add Kiro (AWS Agentic IDE) as a 5th provider via ACP#760dgallitelli wants to merge 2 commits into
dgallitelli wants to merge 2 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
📝 WalkthroughI can rebuild the review stack artifact correctly, but I need a bit more time to produce a fully validated hidden artifact that includes every rangeId exactly once. Do you want me to proceed to generate the corrected review stack now? 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
| "server:dev": "tsx --tsconfig server/tsconfig.json server/index.js", | ||
| "server:dev-watch": "tsx watch --tsconfig server/tsconfig.json server/index.js", | ||
| "client": "vite", | ||
| "test:server": "TSX_TSCONFIG_PATH=server/tsconfig.json node --import tsx --test 'server/**/*.test.ts'", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
npm script single quotes vs double quotes Windows shell compatibility
💡 Result:
On Windows, npm scripts are executed via cmd.exe by default, so single quotes (') are not treated as quoting delimiters the way they are in Unix shells; you generally need to use double quotes ("...") and escape them appropriately inside package.json strings. [1][2] What this means for single vs double quotes 1) Default Windows shell (cmd.exe) doesn’t treat ' as a quoting mechanism - CMD treats single quotes as a regular character (only double quotes group text). [3] - As a result, using single quotes inside package.json scripts for “group this argument” often breaks on Windows. For example, a script that includes single-quoted arguments was fixed by replacing those single quotes with escaped double quotes. [4] 2) npm itself forwards what you write to the underlying shell; quoting rules depend on that shell - npm documents that scripts are run by passing the command line to sh, and that on Windows the actual shell is cmd.exe (platform dependent). [5][2] - Therefore the same package.json script string can behave differently across OSes because the shell that interprets it is different. [5][2] 3) Double quotes often need extra escaping in package.json - Because your script command is itself a JSON string, you typically need to escape embedded double quotes (e.g., " in JSON) so that the Windows shell receives the intended quotes. This same requirement is commonly noted for cross-platform scripting with npm package scripts. [6] Practical guidance - If your npm script needs to wrap a parameter that contains spaces (or should be treated as one argument), prefer double quotes for the wrapped argument and escape them for JSON, e.g.: "test": "node scrappee.js -u "https://example.com/...\"" (This approach is exactly what was used as the fix when single quotes broke the command via npm.) [4] - Avoid relying on Unix-shell single-quote behavior in npm scripts you expect to run on Windows. [3][4] - If you need Unix shell features (bash syntax), you can change npm’s script shell (script-shell config) to use bash/PowerShell, but that’s a separate cross-platform concern from quote style. [1][7] Note about “globs” and other characters - Some tools accept patterns and need quoting so the shell doesn’t expand them (or expands them differently). Quoting globs in npm scripts is recommended for consistent behavior, and that recommendation implicitly depends on understanding your platform shell’s quoting rules. [8]
Citations:
- 1: https://www.unpkg.com/npm@10.8.2/docs/output/commands/npm-run-script.html
- 2: https://docs.npmjs.com/cli/v9/commands/npm-run-script/?v=true
- 3: https://stackoverflow.com/questions/51080215/differences-between-single-and-double-quotes-in-cmd
- 4: https://stackoverflow.com/questions/37500744/how-to-properly-run-a-npm-script-with-arguments
- 5: https://docs.npmjs.com/cli/v8/using-npm/scripts
- 6: https://2ality.com/2022/08/npm-package-scripts.html
- 7: https://stackoverflow.com/questions/23243353/how-to-set-shell-for-npm-run-scripts-in-windows
- 8: https://medium.com/@jakubsynowiec/you-should-always-quote-your-globs-in-npm-scripts-621887a2a784
🏁 Script executed:
# Check existing npm scripts in package.json to see what pattern is used
head -50 package.json | grep -E "^\s+\"(test|build|start)" || rg '"[a-z]+":.*[\'"]\S+[\'"]\s*"' package.json -A 2 -B 2Repository: siteboon/claudecodeui
Length of output: 134
🏁 Script executed:
cat -n package.json | grep -A 2 -B 2 "test:server"Repository: siteboon/claudecodeui
Length of output: 410
🏁 Script executed:
# Also check for other npm scripts with single quotes that might have the same issue
rg "npm.*'.*\*" package.jsonRepository: siteboon/claudecodeui
Length of output: 47
Use double quotes instead of single quotes for cross-platform compatibility.
Line 32 uses single quotes around the glob pattern. On Windows, npm scripts execute via cmd.exe, which does not treat single quotes as quoting delimiters—they are treated as literal characters. This breaks glob pattern matching on Windows. Use double quotes (escaped in JSON) for consistent behavior across platforms.
Suggested fix
- "test:server": "TSX_TSCONFIG_PATH=server/tsconfig.json node --import tsx --test 'server/**/*.test.ts'",
+ "test:server": "TSX_TSCONFIG_PATH=server/tsconfig.json node --import tsx --test \"server/**/*.test.ts\"",📝 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.
Suggested change
| "test:server": "TSX_TSCONFIG_PATH=server/tsconfig.json node --import tsx --test 'server/**/*.test.ts'", | |
| "test:server": "TSX_TSCONFIG_PATH=server/tsconfig.json node --import tsx --test \"server/**/*.test.ts\"", |
🤖 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 `@package.json` at 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.
Comment on lines
+38
to
+43
| if (ORIGINAL_HOME !== undefined) { | ||
| process.env.HOME = ORIGINAL_HOME; | ||
| process.env.USERPROFILE = ORIGINAL_HOME; | ||
| } else { | ||
| delete process.env.HOME; | ||
| } |
Contributor
There was a problem hiding this comment.
Restore USERPROFILE when HOME was originally unset.
On Line 41–43, the fallback branch deletes HOME but leaves USERPROFILE set to the temp path, which can leak state across tests.
Suggested fix
} else {
delete process.env.HOME;
+ delete process.env.USERPROFILE;
}📝 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.
Suggested change
| if (ORIGINAL_HOME !== undefined) { | |
| process.env.HOME = ORIGINAL_HOME; | |
| process.env.USERPROFILE = ORIGINAL_HOME; | |
| } else { | |
| delete process.env.HOME; | |
| } | |
| if (ORIGINAL_HOME !== undefined) { | |
| process.env.HOME = ORIGINAL_HOME; | |
| process.env.USERPROFILE = ORIGINAL_HOME; | |
| } else { | |
| delete process.env.HOME; | |
| delete process.env.USERPROFILE; | |
| } |
🤖 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-session-synchronizer.test.ts`
around lines 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).
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.
Comment on lines
+562
to
+576
|
|
||
| const kiroSession = project.kiroSessions?.find((session) => session.id === sessionId); | ||
| if (kiroSession) { | ||
| const shouldUpdateProject = selectedProject?.projectId !== project.projectId; | ||
| const shouldUpdateSession = | ||
| selectedSession?.id !== sessionId || selectedSession.__provider !== 'kiro'; | ||
|
|
||
| if (shouldUpdateProject) { | ||
| setSelectedProject(project); | ||
| } | ||
| if (shouldUpdateSession) { | ||
| setSelectedSession({ ...kiroSession, __provider: 'kiro' }); | ||
| } | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Add kiro to fallback provider normalization.
When the session is not yet present in project payloads, the fallback path can currently misclassify a Kiro session as Claude, which may route follow-up actions to the wrong provider.
Proposed fix
const normalizedProvider: LLMProvider =
providerFromStorage === 'cursor'
? 'cursor'
: providerFromStorage === 'codex'
? 'codex'
: providerFromStorage === 'gemini'
? 'gemini'
+ : providerFromStorage === 'kiro'
+ ? 'kiro'
: 'claude';Also applies to: 598-605
🤖 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/hooks/useProjectsState.ts` around lines 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.
- 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! |
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
2 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
New Features
Tests
Chores
test:server)