-
Notifications
You must be signed in to change notification settings - Fork 499
fix(kiro): resolve Windows kiro-cli executable without PATH #768
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,6 +169,65 @@ export function resolveKiroCliNativeSessionEntries( | |
| return [{ location: "kiro-cli-linux-data", path: posix.join(home, ".local", "share", "kiro-cli", "data.sqlite3") }]; | ||
| } | ||
|
|
||
| /** | ||
| * 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. | ||
| */ | ||
| export function resolveKiroCliExecutable( | ||
| inputs: KiroCliNativeInputs & { | ||
| pathEntries?: string[]; | ||
| exists?: (path: string) => boolean; | ||
| }, | ||
| ): string { | ||
| 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")); | ||
|
Comment on lines
+189
to
+200
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 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 🤖 Prompt for AI Agents |
||
|
|
||
| const installCandidates: string[] = []; | ||
| if (inputs.platform === "win32") { | ||
| const localBase = inputs.env.LOCALAPPDATA?.trim() | ||
| || (inputs.env.USERPROFILE?.trim() ? win32.join(inputs.env.USERPROFILE.trim(), "AppData", "Local") : "") | ||
| || win32.join(inputs.home, "AppData", "Local"); | ||
| const programFiles = inputs.env["ProgramFiles"]?.trim() || "C:\\Program Files"; | ||
| installCandidates.push( | ||
| win32.join(localBase, "Kiro-Cli", "kiro-cli.exe"), | ||
| win32.join(programFiles, "Kiro-Cli", "kiro-cli.exe"), | ||
| ); | ||
| } else if (inputs.platform === "darwin") { | ||
| installCandidates.push( | ||
| posix.join(inputs.home, ".local", "bin", "kiro-cli"), | ||
| "/usr/local/bin/kiro-cli", | ||
| "/opt/homebrew/bin/kiro-cli", | ||
| ); | ||
| } else { | ||
| installCandidates.push( | ||
| posix.join(inputs.home, ".local", "bin", "kiro-cli"), | ||
| "/usr/local/bin/kiro-cli", | ||
| ); | ||
| } | ||
|
|
||
| for (const candidate of [...pathCandidates, ...installCandidates]) { | ||
| if (exists(candidate)) return candidate; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On macOS/Linux, if an earlier Useful? React with 👍 / 👎. |
||
| } | ||
| return inputs.platform === "win32" ? "kiro-cli.exe" : "kiro-cli"; | ||
| } | ||
|
|
||
| function nativeKiroCliSessionEntries(): Array<{ location: KiroCliNativeLocation; path: string }> { | ||
| // Only the stores that `kiro-cli logout` / `kiro-cli login` themselves mutate. Import fallbacks | ||
| // (Amazon Q / SSO cache) and KIROCLI_DB_PATH selectors must not be snapshotted for rollback. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| import { describe, expect, test } from "bun:test"; | ||
| import { resolveKiroCliExecutable } from "../src/oauth/kiro-credentials"; | ||
|
|
||
| /** | ||
| * Forced/add-account Kiro login still shells out to the local CLI. After #710 fixed Windows | ||
| * SQLite discovery, Windows installs can import tokens while PATH still lacks `kiro-cli`. | ||
| * The pure executable resolver covers that layout without launching the real binary. | ||
| */ | ||
| describe("kiro-cli executable resolution", () => { | ||
| const WIN_HOME = "C:\\Users\\u"; | ||
|
|
||
| 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"); | ||
|
Comment on lines
+12
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win Exercise production PATH parsing in at least one test. All tests inject Suggested adjustment platform: "win32",
home: WIN_HOME,
- pathEntries: ["C:\\Tools", "C:\\Windows\\System32"],
exists,Add a separate case for the Windows 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 AgentsSource: Path instructions |
||
| }); | ||
|
|
||
| test("win32 falls back to %LOCALAPPDATA%\\Kiro-Cli\\kiro-cli.exe", () => { | ||
| const exists = (path: string) => path === "C:\\Users\\u\\AppData\\Local\\Kiro-Cli\\kiro-cli.exe"; | ||
| expect(resolveKiroCliExecutable({ | ||
| env: { | ||
| PATH: "C:\\Windows\\System32", | ||
| LOCALAPPDATA: "C:\\Users\\u\\AppData\\Local", | ||
| }, | ||
| platform: "win32", | ||
| home: WIN_HOME, | ||
| pathEntries: ["C:\\Windows\\System32"], | ||
| exists, | ||
| })).toBe("C:\\Users\\u\\AppData\\Local\\Kiro-Cli\\kiro-cli.exe"); | ||
| }); | ||
|
|
||
| test("win32 falls back to Program Files\\Kiro-Cli when LOCALAPPDATA binary is absent", () => { | ||
| const exists = (path: string) => path === "C:\\Program Files\\Kiro-Cli\\kiro-cli.exe"; | ||
| expect(resolveKiroCliExecutable({ | ||
| env: { | ||
| PATH: "C:\\Windows\\System32", | ||
| LOCALAPPDATA: "C:\\Users\\u\\AppData\\Local", | ||
| ProgramFiles: "C:\\Program Files", | ||
| }, | ||
| platform: "win32", | ||
| home: WIN_HOME, | ||
| pathEntries: ["C:\\Windows\\System32"], | ||
| exists, | ||
| })).toBe("C:\\Program Files\\Kiro-Cli\\kiro-cli.exe"); | ||
| }); | ||
|
|
||
| test("linux keeps PATH-first resolution and falls back to ~/.local/bin", () => { | ||
| const exists = (path: string) => path === "/home/u/.local/bin/kiro-cli"; | ||
| expect(resolveKiroCliExecutable({ | ||
| env: { PATH: "/usr/bin" }, | ||
| platform: "linux", | ||
| home: "/home/u", | ||
| pathEntries: ["/usr/bin"], | ||
| exists, | ||
| })).toBe("/home/u/.local/bin/kiro-cli"); | ||
| }); | ||
|
|
||
| test("returns the bare command when no candidate exists so spawn can report the original error", () => { | ||
| expect(resolveKiroCliExecutable({ | ||
| env: { PATH: "C:\\Windows\\System32" }, | ||
| platform: "win32", | ||
| home: WIN_HOME, | ||
| pathEntries: ["C:\\Windows\\System32"], | ||
| exists: () => false, | ||
| })).toBe("kiro-cli.exe"); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 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 defaultexistsSyncperforms filesystem I/O. Document the actual behavior or enforce the stronger contract.Also applies to: 228-229
🤖 Prompt for AI Agents