Repository navigation
Conversation
…ent identity Kiro CLI panes never produced completion notifications: kiro has no hook service (kiro-cli hooks live in per-agent config files and cannot be attached to its built-in default agent, which is what 'kiro-cli chat' launches), and 'kiro' was not in AGENT_NAMES so its OSC titles were never attributed. - Manage kiro-cli's native notification settings from a new src/main/kiro/hook-service.ts: chat.enableNotifications=true + chat.notificationMethod=bell make kiro-cli emit a terminal BEL at end of turn when the terminal reported focus-out (mode 1004, supported by xterm.js); Orca's existing terminal-bell path then delivers the OS notification. chat.terminalTitle=true turns on kiro's 'kiro: <title>' OSC titles. Prior values are backed up once and restored on remove. - Add 'kiro' to AGENT_NAMES and the title label maps so those titles attribute the pane (and the notification) to Kiro. - Register kiro in the managed hook registry and AGENT_HOOK_TARGETS.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Greptile SummaryThis PR brings Kiro CLI panes into Orca's notification and identity system. Because kiro-cli's built-in default agent cannot be extended with lifecycle hooks, Orca instead manages three entries in
Confidence Score: 5/5Safe to merge — the change is additive, touches only well-isolated JSON files under the user's home directory, and the two correctness issues from the prior review round are both fixed and covered by new tests. The core hook service uses atomic temp-rename writes, the backup-before-early-return fix ensures remove() always has a record of pre-Orca state, and readBackup() now correctly rejects null/array Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/main/kiro/hook-service.ts | New KiroHookService that manages kiro-cli's native notification settings (cli.json); backup/restore logic is atomic, the null-backup early-return issue and previous:null crash (both flagged in prior review) are fixed. |
| src/main/kiro/hook-service.test.ts | Comprehensive test suite covering install, remove, backup idempotency, user-override preservation, unparseable config safety, and the two previously-flagged edge cases (early-return backup skip and malformed previous:null). |
| src/shared/agent-name-token-match.ts | Adds 'kiro' to AGENT_NAMES; boundary-guard regex prevents false matches on cwd titles like ~/kiro or kiro-scratch. |
| src/shared/terminal-title-agent-type.ts | Adds Kiro label detection in getAgentLabel (before the braille-spinner heuristic) and the Kiro → 'kiro' entry in TITLE_LABEL_TO_AGENT; consistent with how other agents are added. |
| src/shared/agent-title-identity.ts | Mirrors the same Kiro branch added in terminal-title-agent-type.ts; no divergence between the two parallel label functions. |
| src/shared/agent-hook-types.ts | Adds 'kiro' to AGENT_HOOK_TARGETS; comment explains why the hook mechanism is settings-based rather than script-based. |
| src/main/agent-hooks/managed-agent-hook-registry.ts | Registers Kiro in all three managed-hook arrays (installers, removers, status readers); follows the identical pattern used for all other agents. |
| src/main/agent-hooks/remote-hook-service-installers.test.ts | Guard test updated to include Kiro (cast to { installRemote?: unknown }) with inline explanation of why Kiro has no remote installer. |
| src/shared/terminal-title-agent-type.test.ts | Adds Kiro-specific title detection tests: native kiro: <session> titles resolve correctly; path/cwd titles containing "kiro" do not. |
Sequence Diagram
sequenceDiagram
participant Orca
participant KiroHookService
participant cli_json as ~/.kiro/settings/cli.json
participant backup as ~/.orca/agent-hooks/kiro-settings-backup.json
Orca->>KiroHookService: install()
KiroHookService->>cli_json: readSettings()
cli_json-->>KiroHookService: "existing settings (or {})"
alt backup does not exist
KiroHookService->>backup: writeJsonAtomic with pre-Orca values
end
alt managed settings not already set
KiroHookService->>cli_json: writeJsonAtomic with MANAGED_SETTINGS merged
end
KiroHookService-->>Orca: AgentHookInstallStatus installed
Orca->>KiroHookService: remove()
KiroHookService->>cli_json: readSettings()
KiroHookService->>backup: readBackup()
alt backup exists and is valid
loop each MANAGED_KEY still at managed value
alt key was in backup.previous
KiroHookService->>cli_json: restore backup value
else
KiroHookService->>cli_json: delete key
end
end
KiroHookService->>backup: unlinkSync consume backup
end
KiroHookService-->>Orca: AgentHookInstallStatus not_installed
Reviews (3): Last reviewed commit: "chore(kiro): address review nits - tight..." | Re-trigger Greptile
📝 Walkthrough
Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check |  |
The description explains the problem, implementation, testing, security, cross-platform behavior, and limitations. However, it does not follow the required template structure and does not provide the … | Add the required template sections, including ELI5, What Changed, Why, Linked Issue with an actual issue number, Visual Proof or N/A, AI Disclosure if applicable, Review, Agent skill upstream boundary, Notes, and Checklist. Provide the issu… |
| Docstring Coverage | |
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies both primary changes: native Kiro completion notifications and title-based agent identity. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Description check
-
Explanation
The description explains the problem, implementation, testing, security, cross-platform behavior, and limitations. However, it does not follow the required template structure and does not provide the required linked issue under a “Fixes #” entry. It also omits the required checklist and agent-skill boundary section.
Resolution
Add the required template sections, including ELI5, What Changed, Why, Linked Issue with an actual issue number, Visual Proof or N/A, AI Disclosure if applicable, Review, Agent skill upstream boundary, Notes, and Checklist. Provide the issue reference in the required “Fixes #<issue>” format.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
✨ Finishing Touches: 💡 1
-
⚔️ Resolve merge conflicts: 💡
-
- Resolve merge conflict in branch
feat/kiro-completion-notifications
- Resolve merge conflict in branch
-
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a88249b1-4b88-49ac-af0f-8b6fe65b0f69
📒 Files selected for processing (10)
config/tsconfig.cli.jsonsrc/main/agent-hooks/managed-agent-hook-registry.tssrc/main/agent-hooks/remote-hook-service-installers.test.tssrc/main/kiro/hook-service.test.tssrc/main/kiro/hook-service.tssrc/shared/agent-hook-types.tssrc/shared/agent-name-token-match.tssrc/shared/agent-title-identity.tssrc/shared/terminal-title-agent-type.test.tssrc/shared/terminal-title-agent-type.ts
- install(): snapshot the backup before the settings-already-match early return, so a later remove() restores the user's own values instead of blind-deleting keys they configured before Orca (Greptile P1 / CodeRabbit). - remove(): treat a missing backup as nothing-to-restore and bail early, instead of deleting keys that merely equal the managed values. - readBackup(): reject previous:null and arrays (typeof null === 'object' otherwise passed validation and crashed remove() with 'in' on null). - tests: redirect USERPROFILE alongside HOME for Windows hermeticity; add regression tests for settings-already-match backup, no-backup remove, and the previous:null malformed backup.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/kiro/hook-service.ts (1)
193-199: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the production comment block.
Lines 193-199 explain the full control flow and implementation details. Replace them with a concise rationale, such as
// Preserve pre-Orca values for safe restoration on remove().As per coding guidelines, comments must be concise and avoid walking through implementation details.
Suggested rewrite
- // Why: snapshot the user's pre-Orca values once, BEFORE any early return. - // Even when the user already has every managed value set, we must record a - // backup - otherwise a later remove() finds none and blind-deletes keys the - // user configured themselves. Snapshot every managed key (not just the ones - // changing) so remove() can tell "user already had this value" apart from - // "Orca introduced this key". Re-installs never overwrite an existing - // backup, keeping the true pre-Orca state. + // Preserve pre-Orca values for safe restoration on remove().Source: Coding guidelines
src/main/kiro/hook-service.test.ts (1)
173-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover array-shaped malformed backups too.
The implementation now rejects both
previous: nullandprevious: [], but this regression test only exercisesnull. Add the array case so the new validation remains protected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 96be4e5f-2849-4252-a77a-a0b86500a9fe
📒 Files selected for processing (2)
src/main/kiro/hook-service.test.tssrc/main/kiro/hook-service.ts
- Shorten the install() backup-snapshot comment per review. - Add a regression test for a malformed backup with previous:[] (arrays are typeof 'object' too; readBackup() already rejects them).
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟠 Major · Add Kiro to the SSH installer boundary. · hook-service.ts:47-56
src/main/kiro/hook-service.ts:47-56
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd Kiro to the SSH installer boundary.
AGENTS.mdrequires filesystem changes to support SSH hosts. The Kiro service useshomedir()and local filesystem APIs, and Kiro is absent fromREMOTE_MANAGED_HOOK_INSTALLERS. The SSH runtime therefore skips Kiro, leaving the remote host's settings unchanged.Implement a remote-target Kiro service using the existing
SFTPWrapperandremoteHomecontract. Preserve the backup and restore behavior, then register that remote implementation. Keep the local methods for local installations.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
eae5e0ea-3641-4c17-919f-072528a8acfe
📒 Files selected for processing: 1
AGENTS.md
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
Summary
Kiro CLI (
kiro-cli) panes never produce completion notifications in Orca, while Claude Code and Codex do. This PR adds them, plus title-based agent identity for Kiro panes.Two root causes:
agentSpawn/stop) only live inside per-agent config files, and the built-in default agent thatkiro-cli chatlaunches cannot be extended or overridden with hooks. Verified empirically against kiro-cli 2.15.x: a local.kiro/agents/kiro_default.json, a global~/.kiro/agents/kiro_default.json, and v3 standalone.kiro/hooks/*.jsonfiles are all ignored by the v2 engine (hooks in explicit--agent <custom>configs do fire).kirois not inAGENT_NAMES, so Kiro's OSC titles (kiro: <session title>) never attribute the pane or a notification to Kiro.Instead of a hook script that can never fire, the new
src/main/kiro/hook-service.tsmanages kiro-cli's NATIVE completion notifications in~/.kiro/settings/cli.json:chat.enableNotifications: true+chat.notificationMethod: "bell"make kiro-cli emit a terminal BEL at end of turn whenever the hosting terminal has reported focus-out (DECSET 1004, which xterm.js supports). Orca's existing terminal-bell notification path then delivers the OS notification. Verified empirically: with these settings and a focus-out event, kiro-cli emits exactly one standalone BEL when the turn completes.chat.terminalTitle: trueturns on Kiro'skiro: <session title>OSC titles, which (with the newAGENT_NAMESentry) attribute the pane and the notification to Kiro.The service snapshots the user's prior values once, on first install, into
~/.orca/agent-hooks/kiro-settings-backup.json, andremove()restores them (deleting keys that did not exist, and leaving keys alone that the user has since changed away from Orca's managed values).Screenshots
No visual change (notifications use the existing terminal-bell pipeline; the Kiro row appears in the existing managed-hooks status list).
Testing
pnpm lintgates on changed files: oxlint, code-quality native + type-aware, React Doctor, reliability-gates, max-lines ratchet - all pass with 0 new findingspnpm typecheck- exit 0pnpm test- 41,440 passed / 107 skipped; the 2 failures aresrc/main/pty/posix-pty-process-groups.integration.test.ts, which fails identically on a pristinemaincheckout on this machine (Amazon Linux dev desktop), so they are environmental and unrelatedsrc/main/kiro/hook-service.test.ts(install/status/backup-restore/re-install idempotency/user-override preservation/unparseable-config safety) and Kiro title-detection cases insrc/shared/terminal-title-agent-type.test.tskiro: <cwd>title captured; no working/idle title transitions (confirming title-based completion cannot work); standalone BEL emitted on turn completion with the managed settings and terminal focus-outAI Review Report
Reviewed with an AI coding agent. Main risks checked:
cli.jsonwrites are temp+rename atomic with a rolling.bak, mirroring the Kimi service's config write path; unparseablecli.jsonreturns anerrorstatus and never writes.remove()distinguishes "user already had this value" from "Orca introduced this key" and skips keys the user changed away from managed values.chat.terminalTitleis not misreported as partially installed.kirois token-matched with the shared boundary guard; path/cwd titles such as~/kiro,kiro-scratch, and.kiro/settingsdo not classify as Kiro (covered by tests).homedir()(~/.kiro/settings/cli.json,~/.orca/agent-hooks/), valid on macOS, Linux, and Windows; no shell, no scripts, no platform-specific escaping; the BEL/focus-tracking mechanism lives in kiro-cli + xterm.js, both platform-independent. No Electron API changes.remote-hook-service-installers.test.tsupdated: Kiro intentionally has noinstallRemote(it manages local CLI settings only), documented inline.Security Audit
cli.jsonis parsed defensively (non-object/array/malformed input short-circuits to anerrorstatus without writing).Notes
installedstatus detail says so ("Uses Kiro CLI native notifications (terminal bell on turn completion)")..kiro/hooks/files that apply to all agents) becomes the default, a real hook service with rich status becomes possible and can replace this mechanism.