fix: Windows readiness — session discovery, fail-fast watchdog, CI PTY harness#12
Merged
Conversation
Read-only analysis only; no source changes. - WINDOWS_ISSUE_1_RCA.md: root cause of the Windows hang (node-pty reports the claude.cmd wrapper pid, so PidWatcher polls a sessions/<pid>.json that never exists -> no busy/idle -> never exits), proper fix, risks, validation plan, and a read-only confirmation probe. - PRODUCTION_READINESS.md: multi-dimension audit backing finding #5.
…platform PTY clarp keys PidWatcher on the pid node-pty reports for the spawned binary. On Windows that binary is claude.cmd, so node-pty reports the cmd.exe wrapper pid while the real Claude node process runs as a grandchild — clarp then polls ~/.claude/sessions/<wrapper-pid>.json, which never exists, and hangs forever. This adds a stub that mirrors how Claude installs on each platform (POSIX: a node-shebang script; Windows: a .cmd that invokes node) and an integration test that spawns it through real node-pty and compares the reported pid against the pid the child actually records. It runs in the existing cross-OS CI matrix: - POSIX: reported pid == writer pid (why clarp works there today). - Windows: reported pid != writer pid (the issue-#1 root cause), confirmed on the windows-latest runner without needing a real claude install. This harness is the ground truth for verifying the upcoming pid-discovery fix on the platform we can't test locally (closes audit HIGH #5/#7 for the spawn seam).
…ort rm) The first Windows CI run EPERM'd in cleanup before the assertion ran: Windows locks a live process's working directory and the kill+rm raced a held handle. Spawn the stub with cwd=tmpdir() (not the dir we delete) and make rmSync best-effort so cleanup can't mask the pid measurement.
…, issue #1) PidWatcher keyed the status file on the pid node-pty reports for the spawned binary. That pid is authoritative on POSIX (claude is a node-shebang script, exec'd in place) but NOT on Windows: claude.cmd runs through cmd.exe, so the reported pid is the wrapper's and the real Claude node process is a grandchild. clarp polled ~/.claude/sessions/<wrapper-pid>.json, which never exists, so no busy/idle ever fired and `clarp -p "test"` hung forever with no output. - PidWatcher now takes clarp's cwd + start time. When sessions/<pid>.json is absent (Windows claude.cmd, or a POSIX version-manager shim), it discovers the real file by scanning for the session whose cwd matches and whose updatedAt/mtime is after start, tie-broken by newest and filtered to a live pid. Once found it pins the file (claim-once). The POSIX fast path is unchanged: an existing <pid>.json is used directly. - Broaden findClaude on Windows to claude.cmd / claude.exe / claude (the native installer ships claude.exe, which the cmd-only lookup missed entirely). - Normalize backslashes when deriving the transcript slug; the readdir fallback still covers any encoding we don't reproduce. Verified on the windows-latest CI runner via the real-PTY probe: node-pty's reported pid != the writer pid, yet PidWatcher discovers the grandchild's session by cwd. Plus unit coverage for cwd match, recency, liveness, newest- wins, and adoption.
…audit HIGH #1) Defense-in-depth so clarp can never silently hang again, even if observation breaks for a reason the cwd-discovery fix doesn't cover. - Arm the startup readiness watchdog unconditionally until Claude's status is first observed, instead of only when a prompt is queued. `clarp -p "..."` passes the prompt as a claude arg (op-queue empty), so the old gate never armed and the Windows hang had no timeout at all. - Use a separate, generous startup timeout (120s) distinct from the 30s in-turn timeout, so a slow-but-healthy cold start (MCP load, big repo) isn't killed. The phase boundary is "claudeObserved" (any status seen), not "claudeReady" (idle seen), so a busy/waiting-then-stuck turn keeps the 30s in-turn timeout. - On timeout (and on backend-start / dispatch-loop failure) emit a terminal stream-json `result` (subtype error) so a consumer always gets a parseable terminator instead of a truncated stream, guarded to emit at most once. Tests: startup watchdog fires at 120s (not 30s) when never observed, the 30s in-turn timeout still applies once observed, and a single terminal error result is emitted with no duplicate from the subsequent exit.
npm intermittently fails to install platform-specific optional dependencies (vitest 4's rolldown native bindings; npm/cli#4828), which reddened all nine matrix jobs at once on a healthy commit. Retry up to 3x with a clean node_modules, keeping the lockfile for determinism, in both the test matrix and the release publish job.
dn00
changed the title
![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: 411b1c8105
ℹ️ 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".
| // process is already gone (claim a live, current session only). | ||
| if (when < this.startedAt - ADOPT_SKEW_MS) continue; | ||
| if (!isPidAlive(data.pid)) continue; | ||
| if (!best || when > best.when) best = { file, when }; |
There was a problem hiding this comment.
When two clarp invocations start Claude from the same cwd at the same time through a wrapper (so pidFilePath is absent for both), both watchers scan the shared sessions directory and this picks whichever matching file has the newest updatedAt, without tying it back to this wrapper process. In that scenario the older invocation can adopt the other process's session/transcript and report or complete against the wrong Claude run; add a per-launch discriminator or verify process ancestry before selecting among same-cwd live sessions.
Useful? React with 👍 / 👎.
…odex P2) When two clarp runs launch Claude from the same cwd through a wrapper at the same time, both watchers scan the shared sessions directory and the newest-wins tie-break could adopt the other run's session, reporting against the wrong Claude. Now when multiple live sessions share the cwd, PidWatcher adopts only the one whose process descends from the wrapper it launched (bounded parent- chain walk), and refuses rather than guess if ancestry can't single one out. The parent-pid lookup runs only in that rare multi-candidate case. The probe is injectable for tests; the real implementation is verified on each CI OS (including the Windows PowerShell path) via getParentPid(process.pid).
Move WINDOWS_ISSUE_1_RCA.md and PRODUCTION_READINESS.md out of the published tree into .notes/ (git-excluded). The candid 69-issue audit is internal working analysis, not something to publish; the fix and its tests still ship.
dn00
force-pushed
the
fix/windows-readiness
branch
from
3b4fe09 to
d8c9551
Compare
dn00
force-pushed
the
main
branch
2 times, most recently
from
e37ff8e to
9cfc8e4
Compare
dn00
force-pushed
the
fix/windows-readiness
branch
from
d8c9551 to
208fa2f
Compare
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.
Closes #1.
Makes clarp actually work on Windows.
clarp -p "test"previously hung forever (no output, never exits, silent Ctrl-C); it now produces output and exits like on macOS/Linux. Every claim below is verified on thewindows-latestCI runner via a real-PTY probe (no real claude needed).Root cause (issue #1)
clarp's turn lifecycle is driven by Claude's status file at
~/.claude/sessions/<pid>.json, polled byPidWatcherkeyed on the pid node-pty reports. That pid is authoritative on POSIX (claude is a node-shebang script, exec'd in place) but not on Windows:claude.cmdruns throughcmd.exe, so the reported pid is the wrapper's and the real Claudenodeprocess is a grandchild. clarp polled a file that never exists → no busy/idle → never exited. The 30s readiness watchdog never armed either, becauseclarp -ppasses the prompt as a claude arg (op-queue empty).The fix
PidWatchertakes clarp's cwd + start time; whensessions/<pid>.jsonis absent (Windowsclaude.cmd, or a POSIX version-manager shim) it finds the session whose cwd matches and whoseupdatedAt/mtime is after start, filtered to a live pid, then pins it (claim-once). POSIX fast path unchanged. When multiple live sessions share a cwd (concurrent clarp runs), it adopts only the one whose process descends from the wrapper it launched, and refuses rather than guess.resultso consumers get a parseable terminator.findClaudetoclaude.cmd/claude.exe(native installer) / bareclaude; normalize\in the transcript slug.Verification (on
windows-latestCI)PidWatcherdiscovers the grandchild's session by cwd (the fix). POSIX contrast asserted on ubuntu/macos.getParentPid(process.pid)verifies the platform parent-pid lookup (incl. Windows PowerShell) on every CI OS.Also here
npm cito absorb the rolldown optional-deps install flake ([BUG] Platform-specific optional dependencies not being included inpackage-lock.jsonwhen reinstalling withnode_modulespresent npm/cli#4828) that reddened a healthy build.