Repository navigation
Conversation
…xport/apply Four Windows-only crash reports (InsForge#306, InsForge#300, InsForge#286, InsForge#270) trace to two distinct mechanisms, not one shared bug: 1. TTY-capability bypass (InsForge#270, InsForge#286): config export/apply call @clack/prompts' confirm() directly instead of the isInteractive-guarded wrapper in lib/prompts.ts, so they try to init a TTY on stdin even when stdout/stdin isn't a real console handle, crashing with "TTY initialization failed: uv_tty_init returned EINVAL". branch create has the same gap for its spinner (`!json` guard only, no isInteractive check), unlike the identical pattern already used in ai/setup.ts, posthog/setup.ts, and webscraper/apify/connect.ts. Fix: route export.ts/apply.ts's confirm through lib/prompts.ts, and add the same isInteractive guard to branch/create.ts's spinner. 2. Forced process.exit() racing a still-active handle (InsForge#306, InsForge#300): - create.ts calls process.exit(0) right after a cancelled prompt; in the non-interactive fallback, that fires the instant after the underlying readline interface's own 'close' event (stdin EOF) — exiting synchronously while that handle is still unwinding is the documented Windows libuv race in src/win/async.c. - Every command's handleError() runs through PostHog analytics first. analytics.ts constructs PostHog without enableLocalEvaluation: false, which (confirmed by reading posthog-node's source) unconditionally starts a background feature-flag poller and fires an *unawaited* flag-definitions fetch on construction — one the CLI never needs, since nothing in src/ calls getFeatureFlag/isFeatureEnabled. shutdownAnalytics()'s stopPoller() only clears the *next* scheduled poll via clearTimeout; it doesn't cancel that in-flight request. The immediately-following process.exit() can race its still-open socket. Fix: disable PostHog's local evaluation (one-line, single shared constructor) so the pointless poller/fetch never starts, and replace create.ts's cancel-time process.exit(0) with process.exitCode + return so Node drains naturally instead of forcing a hard stop. Confidence: high on (1), source-verified exact error-message match. Medium-high on (2): no Windows box to reproduce the assertion directly, but both fixes are independently correct (dead poller removed; Node's own docs recommend exitCode over exit() for exactly this class of race) and directly address the mechanisms described in each issue's own repro. Tests: added regression coverage for the export/apply confirm-routing fix (previously uncovered — existing tests always pass --force/--yes/ --json, which skip the prompt entirely). branch/create.test.ts updated to mock isInteractive: true, since it asserts on the spinner-driven path. Fixes InsForge#306, Fixes InsForge#300, Fixes InsForge#286, Fixes InsForge#270
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
agent-zhang-beihai
Bot
added
the
needs-claim
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 22 minutes. View limit details
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 8 files
Reply with feedback, questions, or to request a fix.
View guided diff | Re-trigger cubic
 [Medium impact] The PR appears safe to merge, with the previously reported missing success message remaining a non-blocking issue. Findings
SummaryThe PR guards Windows terminal use and avoids forced exits when project creation is cancelled.
Reviews (2) · Last reviewed commit: "fix(test): correct stale TTY_TEST refere..." · Reviewed by Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // before an error). JSON mode skips the spinner — `outputJson({ branch: | ||
| // ready })` below remains the sole authoritative output. | ||
| const spinner = !json ? clack.spinner() : null; | ||
| const spinner = !json && isInteractive ? clack.spinner() : null; |
There was a problem hiding this comment.
When stdout is redirected, isInteractive is false and the spinner is now absent. A successful branch create <name> --no-switch then prints nothing: its only success message is in spinner?.stop(), and the later output block has no message for this case.
Keep the spinner guard, but print a plain success message when !json && !isInteractive. Add a test with isInteractive: false so this path stays covered.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
agent-zhang-beihai
Bot
removed
the
needs-claim
cubic-dev-ai follow-up on InsForge#312: the comment pointed at a nonexistent TTY_TEST. Point it at the actual regression test name instead.
|
Fixed in bd92545 — the comment now points at the actual test name ( |
Summary
Investigated whether #306, #300, #286, #270 (all Windows-only libuv/TTY crash reports) share one root cause. They trace to two distinct mechanisms, not one:
1. TTY-capability bypass → fixes #270, #286
src/commands/config/export.tsandsrc/commands/config/apply.tscall@clack/prompts'confirm()directly instead of theisInteractive-guarded wrapper already defined insrc/lib/prompts.ts. Clack's rawconfirm()always tries to init a TTY on stdin regardless of whether stdout/stdin is actually a console handle, which is exactly what produces #270'sError: TTY initialization failed: uv_tty_init returned EINVAL.src/commands/branch/create.ts's spinner has the same category of gap — it's guarded by!jsononly, not!json && isInteractive, unlike the identical pattern already established inai/setup.ts,posthog/setup.ts, andwebscraper/apify/connect.ts. That mismatch matches #286 (spinner writes into a non-interactive Windows terminal and eventually EPIPEs).Fix: route
export.ts/apply.ts's confirmation prompt throughlib/prompts.js, and add the sameisInteractiveguard tobranch/create.ts's spinner — both changes adopt a pattern that already exists and is tested elsewhere in this codebase.2. Forced
process.exit()racing a still-open handle → fixes #306, #300create.tscallsprocess.exit(0)immediately after a cancelled prompt. In the non-interactive fallback (lib/prompts.ts'snonTtyText/LineReader), a cancel means stdin just hit EOF and the underlyingreadline.Interface's own'close'event only just fired. Callingprocess.exit()synchronously the instant after that is the documented class of Windows libuv race insrc/win/async.c— forcing an immediate handle teardown while one is already mid-close.handleError()(the single shared error path) runs through PostHog analytics first.src/lib/analytics.tsconstructs thePostHogclient withoutenableLocalEvaluation: false. I pulledposthog-node@5.28.9's source to confirm: with local evaluation enabled (the default), theFeatureFlagsPollerconstructor callsloadFeatureFlags()unconditionally and unawaited, immediately firing a flag-definitions fetch and scheduling a recurringsetTimeoutpoll — entirely pointless here, since nothing insrc/ever callsgetFeatureFlag/isFeatureEnabled.shutdownAnalytics()→client.shutdown()→stopPoller()only doesclearTimeout(this.poller), which cancels the next scheduled poll — it does not cancel an already in-flight fetch.handleError()'sprocess.exit()fires right after thatshutdown()resolves, so an in-flight PostHog socket can still be open when the process is forced to exit. This matches Migration name validation triggers Windows libuv assertion #300'sAssertion failed: !(handle->flags & UV_HANDLE_CLOSING), file src\win\async.c, line 94and the issue's own triage note about "the catch-path PostHog flush."Fix: disable PostHog's local evaluation (one line, in the single shared client constructor — removes the dead poller/fetch entirely), and replace
create.ts's cancel-timeprocess.exit(0)withprocess.exitCode = 0; return;so Node drains naturally instead of forcing a hard stop (Node's own docs recommendexitCodeoverexit()for exactly this class of issue).Confidence
uv_tty_init returned EINVAL), minimal diff adopting an existing in-repo pattern.process.exitCodeoverprocess.exit()is the documented-safe pattern for this exact race), and they directly target the mechanisms each issue's own repro describes.Test plan
npx tsc --noEmit— no new errors vs. the pre-fix baseline (diffed explicitly)npx eslint src/— cleannpx vitest run— 835 passed, 5 pre-existing failures (Windows path-separator issues inflyctl.test.ts,migrations.test.ts,local/checkout.test.ts— none of these files were touched by this PR)config export/config apply's overwrite/apply confirmation prompt — previously uncovered, since every existing test passes--force/--yes/--jsonand skips the prompt entirely. New tests assert the command callslib/prompts.js'sconfirm()(not raw@clack/prompts) and that declining leaves state untouched.branch/create.test.tsto mockisInteractive: true, since those tests specifically assert on the spinner-driven interactive path.🤖 Generated with Claude Code
Summary by cubic
Fixes four Windows-only crashes in
create,branch create,config export, andconfig apply(#306, #300, #286, #270). They came from two sources: prompts and spinners that bypassed the CLI'sisInteractiveTTY guard, andprocess.exit()racing a still-open libuv handle.Bug Fixes
config exportandconfig apply's confirmation prompts throughlib/prompts.js's guardedconfirm()instead of raw@clack/prompts, which tried to init a TTY on stdin even without a console handle and crashed withuv_tty_init returned EINVAL.branch create's spinner withisInteractive(previously only!json), matching the pattern used elsewhere in the CLI.enableLocalEvaluation: false) so the unused feature-flag poller never starts, leaving no in-flight socket forhandleError()'sprocess.exit()to race.process.exit(0)increate.tswithprocess.exitCode = 0; return;so Node drains naturally instead of forcing a hard stop.--force/--yes/--json) and mockisInteractive: trueinbranch/create.test.tsfor its spinner path.Written for commit bd92545. Summary will update on new commits.