fix(kiro): resolve Windows kiro-cli executable without PATH - #768
fix(kiro): resolve Windows kiro-cli executable without PATH#768aljjang95 wants to merge 1 commit into
Conversation
After the Windows SQLite store path fix, forced/add-account login still spawned bare `kiro-cli` and failed when the installer binary was not on PATH. Resolve PATH first, then fall back to the LocalAppData and Program Files Kiro-Cli install layouts, with pure unit coverage.
📝 WalkthroughWalkthroughThe PR adds cross-platform Kiro CLI executable resolution, uses it for logout and CLI commands, tests Windows and Linux lookup behavior, and documents Windows credential import paths. ChangesKiro CLI executable resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant KiroOAuth
participant KiroCliResolver
participant Bun
KiroOAuth->>KiroCliResolver: Resolve platform-specific executable
KiroCliResolver-->>KiroOAuth: Return installed path or bare command
KiroOAuth->>Bun: Spawn logout or CLI command
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e22ef94209
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| for (const candidate of [...pathCandidates, ...installCandidates]) { | ||
| if (exists(candidate)) return candidate; |
There was a problem hiding this comment.
On macOS/Linux, if an earlier PATH directory contains a non-executable file or directory named kiro-cli while a later directory contains the real executable, this existsSync check selects the unusable entry and Bun.spawn fails with EACCES. The previous bare-command spawn allowed Bun's PATH search to skip that entry and launch the later executable. Validate that candidates are executable regular files (for example with stat and X_OK) or leave PATH resolution to Bun.spawn before trying the fixed install-directory fallbacks.
Useful? React with 👍 / 👎.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/oauth/kiro-credentials.ts`:
- Around line 172-181: The exported resolver’s JSDoc overstates its guarantees.
Update the documentation for the resolver near resolveKiroCliExecutable to
describe that it may return a bare or relative executable name and performs
filesystem checks via existsSync, or change the implementation to enforce
absolute, pure behavior; align the contract with the existing PATH and
platform-directory fallback behavior.
- Around line 189-200: Update the PATH candidate validation in the Kiro
credential resolver to use an injectable isRunnable predicate, or equivalent
regular-file-and-executable check, instead of existsSync alone. Ensure
directories and non-executable files are rejected before selecting a candidate,
preserving fallback to later install candidates and the bare command; update the
related validation around the resolver’s candidate selection flow.
In `@tests/kiro-windows-cli-executable-path.test.ts`:
- Around line 12-23: Add regression coverage in the Windows
executable-resolution tests that omits the injected pathEntries and exercises
env.PATH parsing, including Windows separator handling and trimming. Add a
separate case for the env.Path casing if resolveRuntimeKiroCliExecutable
intentionally supports that compatibility path, while preserving the existing
injected-entry tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e2aaa48f-2937-4560-9fc6-0e7a0730c421
📒 Files selected for processing (4)
docs-site/src/content/docs/guides/providers.mdsrc/oauth/kiro-credentials.tssrc/oauth/kiro.tstests/kiro-windows-cli-executable-path.test.ts
| /** | ||
| * Resolve the absolute kiro-cli executable for spawn/login helpers. | ||
| * | ||
| * Pure + parameterized like `resolveKiroCliNativeSessionEntries` so Windows install layouts can be | ||
| * covered from any host. PATH remains the first choice; only when bare `kiro-cli` is missing do we | ||
| * fall back to the platform-native install directories next to the session database. | ||
| * | ||
| * Windows: official MSI installs to `C:\Program Files\Kiro-Cli\kiro-cli.exe`, while some local | ||
| * installs keep the binary next to `%LOCALAPPDATA%\Kiro-Cli\data.sqlite3`. | ||
| * macOS/Linux: prefer PATH, then the usual user-local bin directories. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the exported resolver’s documented contract.
The JSDoc describes the resolver as “absolute” and “pure,” but the fallback intentionally returns bare kiro-cli/kiro-cli.exe, relative PATH entries can produce relative results, and the default existsSync performs filesystem I/O. Document the actual behavior or enforce the stronger contract.
Also applies to: 228-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/oauth/kiro-credentials.ts` around lines 172 - 181, The exported
resolver’s JSDoc overstates its guarantees. Update the documentation for the
resolver near resolveKiroCliExecutable to describe that it may return a bare or
relative executable name and performs filesystem checks via existsSync, or
change the implementation to enforce absolute, pure behavior; align the contract
with the existing PATH and platform-directory fallback behavior.
| const exists = inputs.exists ?? existsSync; | ||
| const pathEntries = inputs.pathEntries | ||
| ?? (inputs.env.PATH ?? inputs.env.Path ?? "").split(inputs.platform === "win32" ? ";" : ":") | ||
| .map(entry => entry.trim()) | ||
| .filter(Boolean); | ||
|
|
||
| const pathCandidates = inputs.platform === "win32" | ||
| ? pathEntries.flatMap(entry => [ | ||
| win32.join(entry, "kiro-cli.exe"), | ||
| win32.join(entry, "kiro-cli"), | ||
| ]) | ||
| : pathEntries.map(entry => posix.join(entry, "kiro-cli")); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -e
printf '\n== file outline ==\n'
ast-grep outline src/oauth/kiro-credentials.ts --view expanded || true
printf '\n== relevant slice ==\n'
sed -n '160,250p' src/oauth/kiro-credentials.ts
printf '\n== search for spawn/exec/path checks ==\n'
rg -n "Bun\.spawn|spawn\(|existsSync|accessSync|statSync|isFile|mode &|chmod|executable" src/oauth -SRepository: lidge-jun/opencodex
Length of output: 13864
🏁 Script executed:
set -e
sed -n '1,260p' src/oauth/kiro-credentials.tsRepository: lidge-jun/opencodex
Length of output: 10569
🏁 Script executed:
set -e
printf '\n== references ==\n'
rg -n "resolveKiroCliExecutable|resolveKiroCliNativeSessionEntries|kiro-cli.exe|kiro-cli" src test tests . -g '!**/node_modules/**'Repository: lidge-jun/opencodex
Length of output: 32085
🏁 Script executed:
set -e
sed -n '1,200p' tests/kiro-windows-cli-executable-path.test.tsRepository: lidge-jun/opencodex
Length of output: 2934
Reject non-runnable PATH hits src/oauth/kiro-credentials.ts:189-200, 225-228 — existsSync() only checks presence, so a directory or non-executable file can win here and short-circuit the later install candidates and the bare-command fallback. That leaves Bun.spawn to fail on a path the resolver treated as valid. Use a regular-file + execute check, or inject a richer isRunnable predicate here.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/oauth/kiro-credentials.ts` around lines 189 - 200, Update the PATH
candidate validation in the Kiro credential resolver to use an injectable
isRunnable predicate, or equivalent regular-file-and-executable check, instead
of existsSync alone. Ensure directories and non-executable files are rejected
before selecting a candidate, preserving fallback to later install candidates
and the bare command; update the related validation around the resolver’s
candidate selection flow.
| test("prefers the first PATH hit before install-directory fallbacks", () => { | ||
| const exists = (path: string) => path === "C:\\Tools\\kiro-cli.exe"; | ||
| expect(resolveKiroCliExecutable({ | ||
| env: { | ||
| PATH: "C:\\Tools;C:\\Windows\\System32", | ||
| LOCALAPPDATA: "C:\\Users\\u\\AppData\\Local", | ||
| }, | ||
| platform: "win32", | ||
| home: WIN_HOME, | ||
| pathEntries: ["C:\\Tools", "C:\\Windows\\System32"], | ||
| exists, | ||
| })).toBe("C:\\Tools\\kiro-cli.exe"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Exercise production PATH parsing in at least one test.
All tests inject pathEntries, so they do not cover env.PATH/env.Path splitting, trimming, or platform-specific separators—the path used by resolveRuntimeKiroCliExecutable().
Suggested adjustment
platform: "win32",
home: WIN_HOME,
- pathEntries: ["C:\\Tools", "C:\\Windows\\System32"],
exists,Add a separate case for the Windows Path casing if that compatibility path is intentional.
As per path instructions, source behavior changes should have focused regression coverage near the existing subsystem tests; the current suite bypasses the runtime PATH-parsing branch.
Also applies to: 26-37, 40-52, 55-63, 66-73
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/kiro-windows-cli-executable-path.test.ts` around lines 12 - 23, Add
regression coverage in the Windows executable-resolution tests that omits the
injected pathEntries and exercises env.PATH parsing, including Windows separator
handling and trimming. Add a separate case for the env.Path casing if
resolveRuntimeKiroCliExecutable intentionally supports that compatibility path,
while preserving the existing injected-entry tests.
Source: Path instructions
…696) HKCU Run values are capped at 260 characters. The tray registered a full PowerShell invocation carrying the Bun path, the CLI entry, the tray script and both home directories, so a user whose npm prefix sits under a long Windows profile name blew the cap and the tray silently never launched after sign-in. An owned VBS launcher holds the long command instead, and the Run value becomes `wscript.exe //B //NoLogo <launcher>`. The launcher is written to the config directory, tracked in tray state, ownership-checked before reuse, and rolled back on failure, so a stale or foreign file at that path is not executed. The regression test builds an entry with a realistically long profile path and asserts the Run value stays within 260 characters, names `wscript.exe`, points at the .vbs — and that the PowerShell form still exceeds 260, which is what makes the first three assertions mean something. Verified by ablation, and the first attempt at that was wrong: disabling the 260-character guard did not fail anything, because the guard is a startup assertion rather than the fix. Substituting the old PowerShell command back into `buildWindowsTrayRunCommand` -- the actual pre-fix behavior -- fails the test (0 pass / 1 fail), and restoring passes 18/18 across both tray suites. This PR was blocked earlier in the round by a conflict in tests/windows-tray.test.ts. Landing #768 first resolved it: the branch now merges clean. Windows behavior cannot be exercised from macOS, so the matrix CI is what confirms it; the local evidence is limited to the pure command-construction functions, which is exactly what these tests cover. Co-authored-by: bitkyc08-arch <bitkyc08@gmail.com>
|
Landed on
The core fix is right and the layout coverage is thorough: PATH first, both Two things I added:
Both were checked by ablation rather than assumed — removing the Windows behavior is unverifiable from macOS, so the matrix CI on the branch is what confirms it. |
2 participants
Summary
kiro-cliand failed when the installer binary was not on PATH.%LOCALAPPDATA%\Kiro-Cli\kiro-cli.exeandC:\Program Files\Kiro-Cli\kiro-cli.exe.Why
%LOCALAPPDATA%\Kiro-Cli\data.sqlite3whilekiro-cli.exeremained undiscoverable for add-account/browser login.C:\Program Files\Kiro-Cli\, and some local installs keep the binary next to the session DB.Testing
bun test tests/kiro-windows-cli-executable-path.test.ts tests/kiro-windows-cli-db-path.test.ts tests/kiro-oauth.test.tsbun x tsc --noEmitbun run privacy:scanRisks
kiro-cli.exeoutside PATH, LocalAppData, and Program Files, the bare-command fallback still reports the original spawn error.Summary by CodeRabbit
New Features
Documentation
Tests