fix(cli): surface real engine startup errors, Windows support (#127)#128
Merged
fix(cli): surface real engine startup errors, Windows support (#127)#128
Conversation
Root cause: when the iii-engine binary crashes on startup or isn't
installed at all, the CLI spawned with `stdio: "ignore"` and swallowed
stderr entirely. 15 seconds later it reported "Check that ports 3111,
3112, 49134 are available" — which pointed users at the wrong problem.
On Windows this was the most common failure mode because most users
don't have Rust/cargo installed and nothing tells them they need it.
Changes:
- Capture engine stderr via `stdio: ["ignore", "ignore", "pipe"]` with
a 16KB cap, surface it on startup failure or in --verbose mode.
Processes still spawn detached/unref'd so the parent can exit cleanly.
- `windowsHide: true` so spawning a console app doesn't pop a window.
- Platform-aware iii binary fallback paths: on Windows, search
%USERPROFILE%\.cargo\bin\iii.exe, %LOCALAPPDATA%\cargo\bin\iii.exe,
and %USERPROFILE%\.local\bin\iii.exe. On Unix, add ~/.cargo/bin/iii
alongside the existing paths.
- Use `path.delimiter` instead of hardcoded `:` when prepending to PATH.
- Three-way diagnostic when the engine never becomes ready:
1. Process crashed → show the captured stderr + common causes
2. Process never started → platform-specific install instructions
3. Running but port not responding → platform-specific port check
command (netstat on Windows, lsof/ss on Unix)
- New `--verbose` / `-v` flag echoes spawn commands and surfaces engine
stderr even on success. Useful for diagnosing #127-style reports
across any platform.
- Docker compose lookup searches three candidate paths (was two), fixing
a latent bug where npm-installed docker-compose.yml at the package
root wasn't always found.
- README: new `### Windows` section in the install area documenting
Option A (rustup + cargo install iii-engine), Option B (Docker
Desktop), and Option C (standalone MCP only, no engine), plus a
symptoms→fix troubleshooting table.
No test changes needed — engine startup is a side-effect path exercised
only via the CLI binary itself, not imported by any unit test. Verified
via `npm test` (684/684) and `npm run build` (clean).
📝 WalkthroughWalkthroughExpanded README with OS-specific installation and Windows diagnostics; revamped CLI engine management to detect platform, locate binaries/docker-compose, spawn engine/docker in background while capturing early stderr, and improve startup readiness/error diagnostics. Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Init
participant Platform as Platform Detect
participant Discovery as Binary/Docker Discovery
participant Spawn as Background Spawn
participant Health as REST Health Check
participant Error as Error Reporter
CLI->>Platform: determine IS_WINDOWS
Platform-->>CLI: OS info
CLI->>Discovery: search iii binary and docker-compose candidates
Discovery-->>CLI: candidate paths or none
alt iii binary found
CLI->>Spawn: spawnEngineBackground(iii)
Spawn->>Spawn: detached start + stderr buffer (<=16KB)
else docker-compose found
CLI->>Spawn: spawnEngineBackground(docker-compose up)
Spawn->>Spawn: detached compose + stderr buffer
end
Spawn-->>CLI: startupFailure (none / engine-crash / docker-crash)
CLI->>Health: probe REST API on port 3111
alt API responds
Health-->>CLI: engine ready
else API timeout
CLI->>Error: build error message (startupFailure, stderr, port diagnostics, installInstructions)
Error-->>CLI: display diagnostics & troubleshooting steps
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cli.ts (1)
97-112: Minor:filter(Boolean)has no effect here.
join()always returns a string, so all array elements are truthy. IfUSERPROFILEis empty,join("", ".cargo", "bin", "iii.exe")produces.cargo\bin\iii.exe(a relative path), which is harmless sinceexistsSync()will return false anyway. Not a bug, just a no-op.♻️ Optional: Remove the ineffective filter
return [ join(userProfile, ".cargo", "bin", "iii.exe"), join(localAppData, "cargo", "bin", "iii.exe"), join(userProfile, ".local", "bin", "iii.exe"), - ].filter(Boolean); + ]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 97 - 112, In fallbackIiiPaths() remove the no-op .filter(Boolean) on the Windows branch array since join() always returns strings; simply return the array directly (keep the same join(...) entries and behavior), i.e., edit the code that constructs the Windows paths in fallbackIiiPaths to return the array without calling .filter(Boolean).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.ts`:
- Around line 146-159: The exit handler for child.on("exit") currently only
treats non-zero numeric exit codes as failures; update it to also treat
signal-terminated exits as failures by changing the condition to check for
either (code !== 0 && code !== null) OR (signal !== null); when signal is
present and code is null, populate startupFailure (the object with kind, stderr,
binary) with an appropriate stderr message like `process terminated by signal
${signal}` (or include any buffered stderr if available), keep the existing
label -> kind logic and bin for binary, and preserve the verbose logging path
(IS_VERBOSE and p.log.error) to print buffered stderr when present.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 97-112: In fallbackIiiPaths() remove the no-op .filter(Boolean) on
the Windows branch array since join() always returns strings; simply return the
array directly (keep the same join(...) entries and behavior), i.e., edit the
code that constructs the Windows paths in fallbackIiiPaths to return the array
without calling .filter(Boolean).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+146
to
+159
| child.on("exit", (code, signal) => { | ||
| if (code !== 0 && code !== null) { | ||
| const stderr = Buffer.concat(stderrChunks).toString("utf-8"); | ||
| startupFailure = { | ||
| kind: label.includes("Docker") ? "docker-crashed" : "engine-crashed", | ||
| stderr: stderr.trim() || `process exited with code ${code}${signal ? ` (${signal})` : ""}`, | ||
| binary: bin, | ||
| }; | ||
| vlog(`engine exited early: code=${code} signal=${signal}`); | ||
| if (IS_VERBOSE && stderr.trim()) { | ||
| p.log.error(`engine stderr:\n${stderr}`); | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Signal-only exits are not captured as failures.
When a process is killed by a signal (e.g., SIGSEGV, SIGABRT), code is null and signal contains the signal name. The current condition code !== 0 && code !== null excludes these cases, so a segfault crash wouldn't be recorded in startupFailure.
On Windows this is less of a concern since abnormal terminations typically have non-zero exit codes, but on Unix this could miss legitimate crashes.
🛡️ Proposed fix to also capture signal-killed processes
child.on("exit", (code, signal) => {
- if (code !== 0 && code !== null) {
+ const abnormal = (code !== 0 && code !== null) || (code === null && signal !== null);
+ if (abnormal) {
const stderr = Buffer.concat(stderrChunks).toString("utf-8");
startupFailure = {
kind: label.includes("Docker") ? "docker-crashed" : "engine-crashed",
stderr: stderr.trim() || `process exited with code ${code}${signal ? ` (${signal})` : ""}`,
binary: bin,
};📝 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
| child.on("exit", (code, signal) => { | |
| if (code !== 0 && code !== null) { | |
| const stderr = Buffer.concat(stderrChunks).toString("utf-8"); | |
| startupFailure = { | |
| kind: label.includes("Docker") ? "docker-crashed" : "engine-crashed", | |
| stderr: stderr.trim() || `process exited with code ${code}${signal ? ` (${signal})` : ""}`, | |
| binary: bin, | |
| }; | |
| vlog(`engine exited early: code=${code} signal=${signal}`); | |
| if (IS_VERBOSE && stderr.trim()) { | |
| p.log.error(`engine stderr:\n${stderr}`); | |
| } | |
| } | |
| }); | |
| child.on("exit", (code, signal) => { | |
| const abnormal = (code !== 0 && code !== null) || (code === null && signal !== null); | |
| if (abnormal) { | |
| const stderr = Buffer.concat(stderrChunks).toString("utf-8"); | |
| startupFailure = { | |
| kind: label.includes("Docker") ? "docker-crashed" : "engine-crashed", | |
| stderr: stderr.trim() || `process exited with code ${code}${signal ? ` (${signal})` : ""}`, | |
| binary: bin, | |
| }; | |
| vlog(`engine exited early: code=${code} signal=${signal}`); | |
| if (IS_VERBOSE && stderr.trim()) { | |
| p.log.error(`engine stderr:\n${stderr}`); | |
| } | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli.ts` around lines 146 - 159, The exit handler for child.on("exit")
currently only treats non-zero numeric exit codes as failures; update it to also
treat signal-terminated exits as failures by changing the condition to check for
either (code !== 0 && code !== null) OR (signal !== null); when signal is
present and code is null, populate startupFailure (the object with kind, stderr,
binary) with an appropriate stderr message like `process terminated by signal
${signal}` (or include any buffered stderr if available), keep the existing
label -> kind logic and bin for binary, and preserve the verbose logging path
(IS_VERBOSE and p.log.error) to print buffered stderr when present.
…i-engine Caught during PR #128 review: the install hints I added showed `cargo install iii-engine` as the recommended path on both Windows and Unix. That crate does not exist on crates.io. Verified via: - https://crates.io/api/v1/crates?q=iii (no iii-engine result) - https://crates.io/api/v1/crates/iii-engine (404) The actual install methods per github.com/iii-hq/iii releases: - macOS / Linux: curl -fsSL https://install.iii.dev/iii/main/install.sh | sh (downloads the prebuilt binary into ~/.local/bin/iii) - Windows: download iii-x86_64-pc-windows-msvc.zip from the GitHub release page, extract iii.exe, add to PATH or place at %USERPROFILE%\.local\bin\iii.exe (which our fallback already checks) - Docker: docker pull iiidev/iii:latest (already used by docker-compose) There is no PowerShell installer (https://install.iii.dev/iii/main/install.ps1 returns 404) and no scoop / winget / cargo path on Windows. The previous README "From source" line (`cargo install iii-engine`) was already wrong on main — fixed at the same time so we have one correct source of truth. Changes: - src/cli.ts installInstructions(): rewrite both Windows and Unix branches to show the correct upstream commands. Add link to https://iii.dev/docs at the bottom of each - src/cli.ts troubleshooting note: "iii-engine version mismatch" line no longer references cargo - src/cli.ts fallbackIiiPaths(): drop the .cargo/bin/iii.exe fallbacks on both platforms — they were dead paths since the crate doesn't exist. Keep .local/bin/iii.exe (matches where the sh installer puts it on Unix and where the README tells Windows users to drop iii.exe) - README.md "Windows" section: rewrite Option A as "download the prebuilt MSVC zip from GitHub releases", add explicit disclaimer note that there is no cargo install iii-engine path - README.md "From source" line: replace stale cargo command with the correct sh-installer / Windows-zip / Docker matrix Same test count (684 / 684), build clean.
Owner
Author
|
Correction: the first commit on this PR had Ground truth for how iii actually installs (from the iii-hq/iii GitHub releases page):
No PowerShell installer script exists — I checked Follow-up changes in `ffca536`
Tests: 684 / 684 still passing. Build clean. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cli.ts (1)
174-203:⚠️ Potential issue | 🟠 MajorTry fallback
iiilocations before switching to Docker.Lines 174-203 only consult
fallbackIiiPaths()after the Docker branch. That means a valid~/.local/bin/iii/%USERPROFILE%\.local\bin\iii.exeinstall is ignored whenever Docker is present, even if Docker is the thing that is broken.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 174 - 203, The code prefers Docker over local installs because the fallback loop using fallbackIiiPaths() runs after the Docker branch; move the fallback resolution earlier: iterate fallbackIiiPaths() first, set iiiBin and prepend dirname(iiiPath) to PATH when exists (same logic inside the for loop), then only if iiiBin is still unset check whichBinary("docker") and composeFile and call spawnEngineBackground; ensure vlog/log messages still reflect whether a local iii was found or Docker was started (refer to whichBinary, dockerComposeCandidates, composeFile, spawnEngineBackground, fallbackIiiPaths, and iiiBin).
♻️ Duplicate comments (1)
src/cli.ts (1)
143-155:⚠️ Potential issue | 🟠 MajorSignal-terminated startups still bypass
startupFailure.Line 144 only records failures for non-zero numeric exit codes. A process killed by
SIGSEGV,SIGABRT, etc. still falls through to the generic timeout path instead of the crash diagnostic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 143 - 155, The exit handler currently only sets startupFailure for non-zero numeric exit codes, missing signal-terminated crashes; change the logic in the child.on("exit", ...) handler (the block that builds startupFailure, uses label, bin, stderrChunks, IS_VERBOSE and p.log.error) so that it also treats any truthy signal as a failure (e.g., if ((code !== 0 && code !== null) || signal) ...). Capture stderr as before, set kind using label, include the signal in the stderr or message (and in vlog) and keep the IS_VERBOSE p.log.error branch unchanged so signal-terminated processes produce the same crash diagnostics as non-zero exits.
🧹 Nitpick comments (1)
src/cli.ts (1)
303-347: Stop waiting once an early startup failure is already known.Line 306 still waits the full 15 seconds even after
startupFailurehas been populated by the exit handler. The diagnostics are better now, but crash cases still keep the old timeout delay before surfacing them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 303 - 347, The code still waits the full waitForEngine(15000) even when startupFailure is already set by the exit handler; modify the logic around the spinner and waitForEngine so that if startupFailure is truthy we abort waiting and immediately run the existing crash diagnostics: replace the direct await waitForEngine(15000) with a cancellable pattern (e.g., Promise.race between waitForEngine and a promise that resolves when startupFailure is set, or check startupFailure immediately before awaiting and re-check right after a short race) and if startupFailure is present stop the spinner (s.stop) and execute the same crash-handling branch that currently checks startupFailure (using the existing symbols startupFailure, s, p.log, getRestPort, portInUseDiagnostic) instead of delaying the diagnostic output.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.ts`:
- Around line 88-92: In whichBinary, the Windows `where` output is split with
"\n" which can leave a trailing "\r" on the chosen path; update the parsing
after execFileSync (in the whichBinary function that uses IS_WINDOWS and
execFileSync) to normalize CRLF by splitting on a regex like /\r?\n/ or by
trimming and removing '\r' from the first entry so the returned path has no
trailing CR; this ensures spawn() receives a valid executable path.
---
Outside diff comments:
In `@src/cli.ts`:
- Around line 174-203: The code prefers Docker over local installs because the
fallback loop using fallbackIiiPaths() runs after the Docker branch; move the
fallback resolution earlier: iterate fallbackIiiPaths() first, set iiiBin and
prepend dirname(iiiPath) to PATH when exists (same logic inside the for loop),
then only if iiiBin is still unset check whichBinary("docker") and composeFile
and call spawnEngineBackground; ensure vlog/log messages still reflect whether a
local iii was found or Docker was started (refer to whichBinary,
dockerComposeCandidates, composeFile, spawnEngineBackground, fallbackIiiPaths,
and iiiBin).
---
Duplicate comments:
In `@src/cli.ts`:
- Around line 143-155: The exit handler currently only sets startupFailure for
non-zero numeric exit codes, missing signal-terminated crashes; change the logic
in the child.on("exit", ...) handler (the block that builds startupFailure, uses
label, bin, stderrChunks, IS_VERBOSE and p.log.error) so that it also treats any
truthy signal as a failure (e.g., if ((code !== 0 && code !== null) || signal)
...). Capture stderr as before, set kind using label, include the signal in the
stderr or message (and in vlog) and keep the IS_VERBOSE p.log.error branch
unchanged so signal-terminated processes produce the same crash diagnostics as
non-zero exits.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 303-347: The code still waits the full waitForEngine(15000) even
when startupFailure is already set by the exit handler; modify the logic around
the spinner and waitForEngine so that if startupFailure is truthy we abort
waiting and immediately run the existing crash diagnostics: replace the direct
await waitForEngine(15000) with a cancellable pattern (e.g., Promise.race
between waitForEngine and a promise that resolves when startupFailure is set, or
check startupFailure immediately before awaiting and re-check right after a
short race) and if startupFailure is present stop the spinner (s.stop) and
execute the same crash-handling branch that currently checks startupFailure
(using the existing symbols startupFailure, s, p.log, getRestPort,
portInUseDiagnostic) instead of delaying the diagnostic output.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e5e26660-5f29-4411-86b7-7ee71c6e0511
📒 Files selected for processing (2)
README.mdsrc/cli.ts
✅ Files skipped from review due to trivial changes (1)
- README.md
CodeRabbit caught: `execFileSync(...).trim().split("\n")[0]` leaves a
trailing \r on the first path whenever `where` on Windows returns
multiple matches. `.trim()` only strips leading/trailing whitespace —
the CRLFs between the matches stay, so split("\n")[0] yields
"C:\path\one.exe\r" which spawn() rejects as an invalid path.
Single-match outputs worked by accident because .trim() ate the final
\r\n before the split. The bug only fired when a user had iii installed
in two places simultaneously (e.g. a WSL install alongside a native
Windows install), which is a plausible but non-default setup.
Fix: split on /\r?\n/, trim each line, find the first non-empty one.
Also now returns `null` on empty output instead of the empty string
(behavior is identical through the caller's truthy check, but it
matches the documented Promise<string | null> contract exactly).
Owner
Author
|
Good catch by @coderabbitai. Fixed in `e2ee04f`. The root cause: `execFileSync(...).trim().split("\n")[0]` only works when `where` returns a single match. With multiple matches, the inter-line `\r\n` stays (because `.trim()` only strips leading/trailing whitespace), so `.split("\n")[0]` yields a path with a trailing `\r` that `spawn()` rejects. Single-match `where` outputs worked by accident because `.trim()` eats the final `\r\n` before the split. The bug only fires when a user has `iii` installed in two places simultaneously — e.g. a WSL install alongside a native Windows install, or both `%USERPROFILE%\.local\bin\iii.exe` and an extracted zip on `%PATH%`. Rare but plausible, and exactly the kind of edge case Windows users hit when they follow install instructions from multiple sources. Fix```ts Handles all three cases:
Tests: 684 / 684 still passing. Build clean. |
|
Tip For best results, initiate chat on the files or code changes.
The edge case (multiple ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cli.ts (1)
546-558: Consider capturing timestamp once if observations should share the same time.The timestamp is generated fresh for each observation in the loop. If these observations represent a single seeding operation and should share a timestamp for consistency, capture it once before the loop. However, if distinct timestamps per observation are intentional for realistic demo data, this is fine as-is.
♻️ Optional: capture timestamp once
+ const seedTimestamp = new Date().toISOString(); for (const obs of session.observations) { const url = `${base}/agentmemory/observe`; const payload = { hookType: "post_tool_use", sessionId: session.id, - timestamp: new Date().toISOString(), + timestamp: seedTimestamp,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cli.ts` around lines 546 - 558, The loop over session.observations currently creates a new ISO timestamp for each payload; if observations should share the same time, capture a single timestamp before the loop (e.g., const sharedTimestamp = new Date().toISOString()) and use that in the payload.timestamp field inside the loop instead of calling new Date() each iteration; update references in the payload construction (hookType, sessionId, timestamp, data) so payload.timestamp uses the sharedTimestamp while leaving session.observations and the rest of the payload unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cli.ts`:
- Around line 102-114: In fallbackIiiPaths, avoid producing relative paths when
USERPROFILE or HOME are empty: check IS_WINDOWS and only build the join(...)
entries when process.env["USERPROFILE"] (for Windows) or process.env["HOME"]
(for non-Windows) is truthy, otherwise omit those entries; keep the existing
static fallbacks (e.g. "/usr/local/bin/iii") and the final filter(Boolean)
behavior if desired, ensuring the function only returns absolute paths when env
vars are present.
---
Nitpick comments:
In `@src/cli.ts`:
- Around line 546-558: The loop over session.observations currently creates a
new ISO timestamp for each payload; if observations should share the same time,
capture a single timestamp before the loop (e.g., const sharedTimestamp = new
Date().toISOString()) and use that in the payload.timestamp field inside the
loop instead of calling new Date() each iteration; update references in the
payload construction (hookType, sessionId, timestamp, data) so payload.timestamp
uses the sharedTimestamp while leaving session.observations and the rest of the
payload unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+102
to
+114
| function fallbackIiiPaths(): string[] { | ||
| if (IS_WINDOWS) { | ||
| const userProfile = process.env["USERPROFILE"] || ""; | ||
| return [ | ||
| join(userProfile, ".local", "bin", "iii.exe"), | ||
| join(userProfile, "bin", "iii.exe"), | ||
| ].filter(Boolean); | ||
| } | ||
| return [ | ||
| join(process.env["HOME"] || "", ".local", "bin", "iii"), | ||
| "/usr/local/bin/iii", | ||
| ]; | ||
| } |
There was a problem hiding this comment.
Guard against empty USERPROFILE/HOME to avoid checking unintended relative paths.
When the environment variable is unset or empty, join("", ".local", "bin", "iii") produces a relative path like .local/bin/iii. While mostly harmless (the path likely won't exist), it could theoretically match an unintended file in the current working directory.
🛡️ Proposed fix to skip invalid paths
function fallbackIiiPaths(): string[] {
if (IS_WINDOWS) {
const userProfile = process.env["USERPROFILE"] || "";
+ if (!userProfile) return [];
return [
join(userProfile, ".local", "bin", "iii.exe"),
join(userProfile, "bin", "iii.exe"),
- ].filter(Boolean);
+ ];
}
+ const home = process.env["HOME"] || "";
+ if (!home) return ["/usr/local/bin/iii"];
return [
- join(process.env["HOME"] || "", ".local", "bin", "iii"),
+ join(home, ".local", "bin", "iii"),
"/usr/local/bin/iii",
];
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cli.ts` around lines 102 - 114, In fallbackIiiPaths, avoid producing
relative paths when USERPROFILE or HOME are empty: check IS_WINDOWS and only
build the join(...) entries when process.env["USERPROFILE"] (for Windows) or
process.env["HOME"] (for non-Windows) is truthy, otherwise omit those entries;
keep the existing static fallbacks (e.g. "/usr/local/bin/iii") and the final
filter(Boolean) behavior if desired, ensuring the function only returns absolute
paths when env vars are present.
rohitg00
added a commit
that referenced
this pull request
Apr 13, 2026
…wups) CodeRabbit flagged two remaining minor issues on the Windows diagnostics PR #128 that landed before they were addressed: 1. src/cli.ts:161 — the exit handler only treated non-zero numeric codes as abnormal. Signal-killed processes have code === null and signal !== null (e.g., SIGSEGV, SIGABRT, SIGKILL). Those crashes were silently dropped from startupFailure so the user got the generic "did not become ready" timeout instead of a real diagnosis. On Windows this rarely fires (abnormal exits usually carry codes) but on Unix a segfault in iii-engine would be invisible. Fix: widen the guard to `(code !== null && code !== 0) || (code === null && signal !== null)` so signal-only exits are captured too. Also generate a clearer fallback message when stderr is empty: "process killed by signal SIGSEGV" rather than the old "process exited with code null (SIGSEGV)". 2. src/cli.ts:114 — when USERPROFILE (Windows) or HOME (Unix) is unset, `join("", ".local", "bin", "iii.exe")` produces a relative path like `.local/bin/iii.exe`. Harmless today (the path is checked via existsSync which fails silently), but theoretically it could match an unintended file in the user's current working directory. Fix: guard the env var first, return an empty list (Windows) or skip the per-user path (Unix) when the env var is missing. Keep the absolute /usr/local/bin/iii fallback on Unix since it's valid regardless of HOME. No test changes — whichBinary and fallbackIiiPaths are CLI side-effect paths not covered by unit tests. Tests: 684/684. Build clean.
3 tasks
rohitg00
added a commit
that referenced
this pull request
Apr 13, 2026
fix(cli): capture signal kills + guard empty env paths (#128 followups)
4 tasks
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
1 participant
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.
Addresses #127.
Root cause
@Gakko-IT2 reported on Windows: "Port 3111 (REST/MCP) bindet nicht". The message is a symptom, not the root cause.
When the iii-engine binary crashes on startup or isn't installed at all, the CLI spawned with
stdio: "ignore"and swallowed stderr entirely. 15 seconds later it printed:...which pointed users at the wrong problem. On Windows this was the most common failure mode because most users don't have Rust/cargo installed and nothing told them they needed it. The real failure ("iii binary not found" or "docker not running") was invisible.
Changes
Error capture
stdio: [\"ignore\", \"ignore\", \"pipe\"]instead ofignore— captures stderr with a 16KB cap, processes are still spawned detached + unref'd so the parent exits cleanlywindowsHide: trueso the spawn doesn't pop a console window on WindowsPlatform-aware path handling
iii.exe:%USERPROFILE%\\.cargo\\bin\\iii.exe%LOCALAPPDATA%\\cargo\\bin\\iii.exe%USERPROFILE%\\.local\\bin\\iii.exe~/.cargo/bin/iiialongside the existing~/.local/bin/iiiand/usr/local/bin/iiipath.delimiterinstead of hardcoded:when prepending to PATH — previously corrupted Windows PATH if the Unix fallback block ever randocker-compose.ymlat the package root wasn't always foundThree-way diagnostic on engine-not-ready
Instead of `"Check that ports are available"` as a one-size-fits-all, the CLI now distinguishes:
netstat -ano | findstr :3111on Windows,lsof -i :3111/ss -tlnpon Unix) + firewall hintNew
--verbose/-vflagEchoes every spawn command and surfaces engine stderr even on successful startup. Makes diagnosing future reports like #127 trivial on any platform — tell the reporter to re-run with
--verboseand paste the output.Windows README section
New
### Windowssection in the install area with three documented options:cargo install iii-engineWhat I did NOT change
spawnarguments on the happy path are unchanged — engine still spawns detached and is still unref'd, so the CLI parent still exits cleanly after triggering startup--no-engineescape hatch still works for anyone who wants to skip the engine entirelyTest plan
npm test→ 684 / 684 passingnpm run build→ clean, compiled CLI contains all new code paths (IS_WINDOWS,spawnEngineBackground,startupFailure,installInstructions)--verboseto confirm real stderr surfacesRelated
Summary by CodeRabbit
New Features
Bug Fixes
Documentation