diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index dc44d8f..975a085 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,7 +23,19 @@ jobs: - uses: actions/setup-node@v4 with: node-version: ${{ matrix.node }} - - run: npm ci + # npm intermittently fails to install platform-specific optional deps + # (rolldown native bindings; npm/cli#4828), reddening healthy builds. + # Retry with a clean node_modules, keeping the lockfile for determinism. + - name: Install dependencies + shell: bash + run: | + for attempt in 1 2 3; do + if npm ci; then exit 0; fi + echo "::warning::npm ci failed (attempt $attempt); retrying" + rm -rf node_modules + sleep 5 + done + exit 1 - run: npm run build - run: npm test - run: npm pack --dry-run @@ -41,7 +53,16 @@ jobs: with: node-version: 22 registry-url: https://registry.npmjs.org - - run: npm ci + - name: Install dependencies + shell: bash + run: | + for attempt in 1 2 3; do + if npm ci; then exit 0; fi + echo "::warning::npm ci failed (attempt $attempt); retrying" + rm -rf node_modules + sleep 5 + done + exit 1 - run: npm run build - run: npm test - run: npm publish --provenance --access public diff --git a/src/__fixtures__/win-pid-stub/child.mjs b/src/__fixtures__/win-pid-stub/child.mjs new file mode 100755 index 0000000..5d1c8ac --- /dev/null +++ b/src/__fixtures__/win-pid-stub/child.mjs @@ -0,0 +1,37 @@ +#!/usr/bin/env node +// Stub that stands in for Claude's real node process. It records its own pid +// and writes a Claude-style session file, so a test can verify both: +// 1. the pid node-pty reports for the spawned wrapper vs this real pid, and +// 2. that a PidWatcher discovers this session file by cwd+recency even when +// node-pty's reported pid points at a wrapper (the issue-#1 fix). +// +// POSIX: Claude is a `#!/usr/bin/env node` script, so node-pty execs node in +// place and the reported pid IS this pid. Windows: Claude is launched via +// claude.cmd, so node-pty reports the cmd.exe wrapper pid and this node runs +// as a grandchild with a DIFFERENT pid — the root cause of issue #1. +import { writeFileSync, mkdirSync } from "node:fs"; +import { join } from "node:path"; + +const pidFile = process.env.CLARP_PROBE_PIDFILE; +if (pidFile) writeFileSync(pidFile, String(process.pid)); + +const sessionsDir = process.env.CLARP_PROBE_SESSIONS_DIR; +if (sessionsDir) { + mkdirSync(sessionsDir, { recursive: true }); + writeFileSync( + join(sessionsDir, `${process.pid}.json`), + JSON.stringify({ + pid: process.pid, + sessionId: "probe-session", + cwd: process.env.CLARP_PROBE_CWD || process.cwd(), + kind: "interactive", + status: "idle", + updatedAt: Date.now(), + }), + ); +} + +process.stdout.write("stub-child-ready\n"); +// Stay alive briefly so the test can read node-pty's reported pid and run the +// PidWatcher (whose liveness check requires this process to still exist). +setTimeout(() => process.exit(0), 6000); diff --git a/src/__fixtures__/win-pid-stub/wrapper.cmd b/src/__fixtures__/win-pid-stub/wrapper.cmd new file mode 100644 index 0000000..731aadc --- /dev/null +++ b/src/__fixtures__/win-pid-stub/wrapper.cmd @@ -0,0 +1,5 @@ +@echo off +REM Mirrors how Claude Code installs on Windows: a .cmd shim that invokes node. +REM node-pty/ConPTY runs this through cmd.exe, so the real node process below is +REM a grandchild with a pid that differs from node-pty's reported pid. +node "%~dp0child.mjs" diff --git a/src/pid-watcher.test.ts b/src/pid-watcher.test.ts index a53e16d..c675ca9 100644 --- a/src/pid-watcher.test.ts +++ b/src/pid-watcher.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; -import { PidWatcher, type PidFileData } from "./pid-watcher.js"; +import { PidWatcher, getParentPid, type PidFileData } from "./pid-watcher.js"; import * as fs from "node:fs"; import * as path from "node:path"; import * as os from "node:os"; @@ -147,3 +147,225 @@ describe("PidWatcher", () => { expect(transcriptPath).toBeNull(); }); }); + +// When node-pty reports a wrapper's pid (Windows claude.cmd, POSIX shim) the +// reported pid file never appears, so PidWatcher must discover Claude's real +// session file by matching cwd + recency. The reported pid (WRAPPER_PID) is +// deliberately one whose file is never written. +describe("PidWatcher session discovery (wrapper-pid case)", () => { + const WRAPPER_PID = 4242; + const REAL_PID = process.pid; // a guaranteed-alive "grandchild" pid + let tmpDir: string; + let sessionsDir: string; + let startedAt: number; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "pid-discovery-test-")); + sessionsDir = path.join(tmpDir, ".claude", "sessions"); + fs.mkdirSync(sessionsDir, { recursive: true }); + startedAt = Date.now(); + }); + + afterEach(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + function writeSession(pid: number, data: Partial): void { + const full: PidFileData = { + pid, + sessionId: `sess-${pid}`, + cwd: "/work/project", + kind: "interactive", + updatedAt: startedAt + 1000, + ...data, + }; + fs.writeFileSync(path.join(sessionsDir, `${pid}.json`), JSON.stringify(full)); + } + + function makeWatcher(cwd: string): PidWatcher { + return new PidWatcher(WRAPPER_PID, { onStatusChange: () => {} }, tmpDir, { + cwd, + startedAt, + getParentPid: (pid) => (pid === REAL_PID ? WRAPPER_PID : null), + }); + } + + it("discovers the real session file by cwd when the reported pid file is absent", () => { + writeSession(REAL_PID, { cwd: "/work/project", sessionId: "real-session" }); + const watcher = makeWatcher("/work/project"); + expect(watcher.getSessionId()).toBe("real-session"); + }); + + it("ignores session files for a different cwd", () => { + writeSession(REAL_PID, { cwd: "/some/other/dir", sessionId: "other" }); + const watcher = makeWatcher("/work/project"); + expect(watcher.getSessionId()).toBeNull(); + }); + + it("ignores a stale prior-run file (updatedAt before start)", () => { + writeSession(REAL_PID, { updatedAt: startedAt - 60_000, sessionId: "stale" }); + const watcher = makeWatcher("/work/project"); + expect(watcher.getSessionId()).toBeNull(); + }); + + it("ignores a session whose process is no longer alive", () => { + // pid 0 fails the liveness/validity guard deterministically. + writeSession(0, { cwd: "/work/project", sessionId: "dead" }); + const watcher = makeWatcher("/work/project"); + expect(watcher.getSessionId()).toBeNull(); + }); + + it("reports status changes from the discovered file via polling", () => { + vi.useFakeTimers(); + try { + writeSession(REAL_PID, { cwd: "/work/project", status: "busy", sessionId: "live" }); + const changes: string[] = []; + const watcher = new PidWatcher( + WRAPPER_PID, + { onStatusChange: (status) => changes.push(status) }, + tmpDir, + { + cwd: "/work/project", + startedAt, + getParentPid: (pid) => (pid === REAL_PID ? WRAPPER_PID : null), + }, + ); + watcher.start(); + expect(changes).toEqual(["busy"]); + writeSession(REAL_PID, { cwd: "/work/project", status: "idle", sessionId: "live" }); + vi.advanceTimersByTime(600); + expect(changes).toEqual(["busy", "idle"]); + watcher.stop(); + } finally { + vi.useRealTimers(); + } + }); + + it("matches cwd case-insensitively and across separators on Windows", () => { + if (process.platform !== "win32") return; + writeSession(REAL_PID, { cwd: "C:\\Work\\Project", sessionId: "win" }); + const watcher = makeWatcher("c:/work/project"); + expect(watcher.getSessionId()).toBe("win"); + }); +}); + +// When two clarp runs launch Claude from the same cwd through a wrapper, both +// scan the shared sessions directory. PidWatcher must adopt only the session +// whose process descends from the wrapper IT launched, and refuse rather than +// guess if ancestry can't disambiguate. +describe("PidWatcher concurrent same-cwd disambiguation", () => { + const WRAPPER_PID = 500; + let tmpDir: string; + let sessionsDir: string; + let startedAt: number; + + beforeEach(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "pid-concurrent-test-")); + sessionsDir = path.join(tmpDir, ".claude", "sessions"); + fs.mkdirSync(sessionsDir, { recursive: true }); + startedAt = Date.now(); + }); + afterEach(() => { fs.rmSync(tmpDir, { recursive: true, force: true }); }); + + function writeSession(pid: number, sessionId: string, when: number): void { + fs.writeFileSync( + path.join(sessionsDir, `${pid}.json`), + JSON.stringify({ pid, sessionId, cwd: "/shared/dir", kind: "interactive", updatedAt: when }), + ); + } + + // Wrapper 500 -> our Claude 1001; the other run is wrapper 600 -> Claude 1002. + const parents: Record = { 1001: 500, 1002: 600 }; + + function makeWatcher(opts?: { + startedAt?: number; + parents?: Record; + onWarning?: (message: string) => void; + }): PidWatcher { + const lookup = opts?.parents ?? parents; + return new PidWatcher(WRAPPER_PID, { onStatusChange: () => {} }, tmpDir, { + cwd: "/shared/dir", + startedAt: opts?.startedAt ?? startedAt, + isPidAlive: () => true, + getParentPid: (pid) => lookup[pid] ?? null, + }); + } + + function makeWatcherWithWarning(opts: { + startedAt: number; + parents: Record; + warnings: string[]; + }): PidWatcher { + return new PidWatcher(WRAPPER_PID, { + onStatusChange: () => {}, + onWarning: (message) => opts.warnings.push(message), + }, tmpDir, { + cwd: "/shared/dir", + startedAt: opts.startedAt, + isPidAlive: () => true, + getParentPid: (pid) => opts.parents[pid] ?? null, + }); + } + + it("adopts the session descended from our wrapper, not the concurrent run's", () => { + writeSession(1002, "theirs", startedAt + 2000); // newer, but not ours + writeSession(1001, "ours", startedAt + 1000); + expect(makeWatcher().getSessionId()).toBe("ours"); + }); + + it("refuses to adopt when no candidate descends from our wrapper", () => { + writeSession(1002, "theirs", startedAt + 1000); // descends from 600, not 500 + fs.writeFileSync( + path.join(sessionsDir, "9003.json"), + JSON.stringify({ pid: 9003, sessionId: "alsotheirs", cwd: "/shared/dir", kind: "interactive", updatedAt: startedAt + 1500 }), + ); + expect(makeWatcher().getSessionId()).toBeNull(); + }); + + it("waits during the grace window instead of adopting a sole foreign candidate", () => { + writeSession(1002, "theirs", startedAt + 1000); + const watcher = makeWatcher(); + + expect(watcher.getSessionId()).toBeNull(); + + writeSession(1001, "ours", startedAt + 2000); + expect(watcher.getSessionId()).toBe("ours"); + }); + + it("keeps refusing a positively foreign candidate after the grace window", () => { + writeSession(1002, "theirs", startedAt + 1000); + const watcher = makeWatcher({ + startedAt: Date.now() - 11_000, + parents: { 1002: 600, 600: 1 }, + }); + + expect(watcher.getSessionId()).toBeNull(); + }); + + it("degrades to newest candidate after grace when ancestry is indeterminate", () => { + const warnings: string[] = []; + writeSession(1002, "theirs", startedAt + 1000); + writeSession(1003, "newer", startedAt + 2000); + const watcher = makeWatcherWithWarning({ + startedAt: Date.now() - 11_000, + parents: { 1002: null, 1003: null }, + warnings, + }); + + expect(watcher.getSessionId()).toBe("newer"); + expect(warnings).toHaveLength(1); + }); +}); + +describe("getParentPid", () => { + it("resolves the current process's parent on this platform", () => { + // Verifies the real platform implementation (incl. Windows PowerShell) on + // each CI OS without needing concurrent spawns. + expect(getParentPid(process.pid)).toBe(process.ppid); + }); + + it("returns null for an invalid pid", () => { + expect(getParentPid(0)).toBeNull(); + expect(getParentPid(-1)).toBeNull(); + }); +}); diff --git a/src/pid-watcher.ts b/src/pid-watcher.ts index 9d4df26..8d185f6 100644 --- a/src/pid-watcher.ts +++ b/src/pid-watcher.ts @@ -1,6 +1,7 @@ import * as fs from "node:fs"; import * as path from "node:path"; import * as os from "node:os"; +import { execSync } from "node:child_process"; export type PidFileData = { pid: number; @@ -17,6 +18,7 @@ export type PidFileData = { */ export type PidWatcherCallbacks = { onStatusChange: (status: string, waitingFor: string | undefined, data: PidFileData) => void; + onWarning?: (message: string) => void; }; function isSafeSessionId(sessionId: string): boolean { @@ -24,6 +26,55 @@ function isSafeSessionId(sessionId: string): boolean { return /^[A-Za-z0-9._-]+$/.test(sessionId) && !sessionId.includes(".."); } +// Tolerance below clarp's start time when deciding a session file is fresh +// enough to adopt — small so a prior run's file in the same cwd is excluded, +// but non-zero to absorb filesystem mtime granularity. +const ADOPT_SKEW_MS = 2000; +const ADOPT_ANCESTRY_GRACE_MS = 10_000; + +type AncestryMatch = "ours" | "foreign" | "indeterminate"; + +/** + * Canonicalizes a cwd for cross-session comparison: resolves it, and on Windows + * folds separator and case differences (`\` vs `/`, drive-letter casing). + */ +function normalizeCwd(p: string): string { + let n: string; + try { n = path.resolve(p); } catch { n = p; } + if (process.platform === "win32") n = n.replace(/\\/g, "/").toLowerCase(); + return n.replace(/[\\/]+$/, ""); +} + +function safeMtimeMs(file: string): number { + try { return fs.statSync(file).mtimeMs; } catch { return 0; } +} + +/** Existence check that treats a permission error as "alive". */ +function isPidAlive(pid: number): boolean { + if (!Number.isInteger(pid) || pid <= 0) return false; + try { process.kill(pid, 0); return true; } + catch (e) { return (e as NodeJS.ErrnoException)?.code === "EPERM"; } +} + +/** + * Returns the parent pid of `pid`, or null if it can't be determined. Used only + * to disambiguate concurrent same-cwd sessions, so the subprocess cost is paid + * only in that rare case. Exported for platform verification in tests. + */ +export function getParentPid(pid: number): number | null { + if (!Number.isInteger(pid) || pid <= 0) return null; + try { + const cmd = process.platform === "win32" + ? `powershell -NoProfile -Command "(Get-CimInstance Win32_Process -Filter 'ProcessId=${pid}').ParentProcessId"` + : `ps -o ppid= -p ${pid}`; + const out = execSync(cmd, { encoding: "utf8", stdio: ["ignore", "pipe", "ignore"], timeout: 5000 }); + const value = parseInt(out.trim(), 10); + return Number.isInteger(value) && value > 0 ? value : null; + } catch { + return null; + } +} + /** * Polls Claude Code's per-process session file for status and transcript * metadata. Tests can pass a homeDir to isolate filesystem state. @@ -33,12 +84,38 @@ export class PidWatcher { private lastStatus: string | null = null; private lastWaitingFor: string | undefined = undefined; private pidFilePath: string; + private sessionsDir: string; private homeDir: string; + // clarp's working dir and start time, used to discover Claude's real session + // file when node-pty's reported pid points at a wrapper (Windows claude.cmd + // or a POSIX version-manager shim) rather than Claude's own process. + private cwd: string | undefined; + private startedAt: number; + // Once a session file is located, we pin it and stat it directly instead of + // re-scanning the sessions directory on every poll. + private adoptedPath: string | null = null; + // Injectable for tests; default to real process probes. + private isAlive: (pid: number) => boolean; + private parentPidOf: (pid: number) => number | null; - constructor(private pid: number, private callbacks: PidWatcherCallbacks, homeDir?: string) { + constructor( + private pid: number, + private callbacks: PidWatcherCallbacks, + homeDir?: string, + opts?: { + cwd?: string; + startedAt?: number; + isPidAlive?: (pid: number) => boolean; + getParentPid?: (pid: number) => number | null; + }, + ) { this.homeDir = homeDir || os.homedir(); - const sessionsDir = path.join(this.homeDir, ".claude", "sessions"); - this.pidFilePath = path.join(sessionsDir, `${pid}.json`); + this.sessionsDir = path.join(this.homeDir, ".claude", "sessions"); + this.pidFilePath = path.join(this.sessionsDir, `${pid}.json`); + this.cwd = opts?.cwd; + this.startedAt = opts?.startedAt ?? 0; + this.isAlive = opts?.isPidAlive ?? isPidAlive; + this.parentPidOf = opts?.getParentPid ?? getParentPid; } /** @@ -71,7 +148,10 @@ export class PidWatcher { const data = this.readPidFile(); if (!data) return null; if (!isSafeSessionId(data.sessionId)) return null; - const slug = "-" + data.cwd.replace(/\//g, "-").replace(/^-/, ""); + // Claude encodes the project dir by replacing path separators with "-". + // Normalize Windows backslashes first so the direct path is tried; the + // readdir fallback below still covers any encoding we don't reproduce. + const slug = "-" + data.cwd.replace(/\\/g, "/").replace(/\//g, "-").replace(/^-/, ""); const projectsDir = path.join(this.homeDir, ".claude", "projects"); const direct = path.join(projectsDir, slug, `${data.sessionId}.jsonl`); if (fs.existsSync(direct)) return direct; @@ -157,8 +237,92 @@ export class PidWatcher { } private readPidFile(): PidFileData | null { + // Without a cwd we can't discover by content, so trust the reported pid + // directly (unchanged legacy behavior). + if (this.cwd === undefined) return this.tryReadFile(this.pidFilePath); + + if (!this.adoptedPath) { + this.adoptedPath = this.discoverSessionFile(); + if (!this.adoptedPath) return null; + } + const data = this.tryReadFile(this.adoptedPath); + if (!data) { + // The adopted file vanished (rotated/cleaned); re-discover next poll. + this.adoptedPath = null; + return null; + } + return data; + } + + /** + * Locates Claude's real session file. The reported pid is authoritative when + * its file exists (POSIX, where claude is a node-shebang script). When it + * doesn't — Claude was launched via claude.cmd on Windows or a version-manager + * shim, so node-pty reports the wrapper's pid and the real file is keyed on a + * grandchild pid — find it by matching cwd and recency. + */ + private discoverSessionFile(): string | null { + if (fs.existsSync(this.pidFilePath)) return this.pidFilePath; + if (this.cwd === undefined) return null; + + const targetCwd = normalizeCwd(this.cwd); + let names: string[]; + try { names = fs.readdirSync(this.sessionsDir); } catch { return null; } + + const candidates: Array<{ file: string; pid: number; when: number }> = []; + for (const name of names) { + if (!name.endsWith(".json")) continue; + const file = path.join(this.sessionsDir, name); + const data = this.tryReadFile(file); + if (!data || typeof data.cwd !== "string") continue; + if (normalizeCwd(data.cwd) !== targetCwd) continue; + const when = typeof data.updatedAt === "number" ? data.updatedAt : safeMtimeMs(file); + // Exclude a prior run's stale file in the same cwd, and any session whose + // process is already gone (claim a live, current session only). + if (when < this.startedAt - ADOPT_SKEW_MS) continue; + if (!this.isAlive(data.pid)) continue; + candidates.push({ file, pid: data.pid, when }); + } + + if (candidates.length === 0) return null; + + // Same-cwd sessions can appear before our real child writes its file. Always + // prefer a verified descendant, even when there is only one candidate. + const classified = candidates.map((c) => ({ + ...c, + ancestry: this.classifyDescendantOf(c.pid, this.pid), + })); + const ours = classified.filter((c) => c.ancestry === "ours"); + if (ours.length > 0) return ours.reduce((a, b) => (b.when > a.when ? b : a)).file; + + if (Date.now() - this.startedAt < ADOPT_ANCESTRY_GRACE_MS) return null; + + if (classified.every((c) => c.ancestry === "foreign")) return null; + + const indeterminate = classified.filter((c) => c.ancestry === "indeterminate"); + if (indeterminate.length === 0) return null; + this.callbacks.onWarning?.( + "could not verify Claude session ancestry; adopting newest matching cwd session after grace period", + ); + return indeterminate.reduce((a, b) => (b.when > a.when ? b : a)).file; + } + + /** Walks the parent chain from `pid` looking for `ancestor` (bounded depth). */ + private classifyDescendantOf(pid: number, ancestor: number, maxDepth = 6): AncestryMatch { + let current = pid; + for (let i = 0; i < maxDepth; i++) { + if (current === ancestor) return "ours"; + if (current === 1) return "foreign"; + const parent = this.parentPidOf(current); + if (parent == null || parent <= 0 || parent === current) return "indeterminate"; + current = parent; + } + return "foreign"; + } + + private tryReadFile(filePath: string): PidFileData | null { try { - return JSON.parse(fs.readFileSync(this.pidFilePath, "utf8")) as PidFileData; + return JSON.parse(fs.readFileSync(filePath, "utf8")) as PidFileData; } catch { return null; } diff --git a/src/pty-host.ts b/src/pty-host.ts index 36c8ca2..3bf0e58 100644 --- a/src/pty-host.ts +++ b/src/pty-host.ts @@ -64,13 +64,19 @@ export function ensureNodePtySpawnHelperExecutable(helperPath = getNodePtySpawnH function findClaude(): string { if (os.platform() === "win32") { - try { - return execSync("where claude.cmd", { encoding: "utf8" }).trim().split("\n")[0]!; - } catch { - throw new Error( - "claude not found. Install Claude Code first: https://docs.anthropic.com/en/docs/claude-code" - ); + // npm install gives claude.cmd; the native installer ships claude.exe; a + // bare `claude` covers anything else on PATH. + for (const name of ["claude.cmd", "claude.exe", "claude"]) { + try { + const found = execSync(`where ${name}`, { encoding: "utf8" }).trim().split(/\r?\n/)[0]; + if (found) return found; + } catch { + // Not found under this name; try the next. + } } + throw new Error( + "claude not found. Install Claude Code first: https://docs.anthropic.com/en/docs/claude-code" + ); } try { return execSync("which claude", { encoding: "utf8" }).trim(); diff --git a/src/pty-pid-mechanism.test.ts b/src/pty-pid-mechanism.test.ts new file mode 100644 index 0000000..2d201ee --- /dev/null +++ b/src/pty-pid-mechanism.test.ts @@ -0,0 +1,97 @@ +import { describe, it, expect } from "vitest"; +import * as nodePty from "node-pty"; +import { fileURLToPath } from "node:url"; +import { dirname, join } from "node:path"; +import { mkdtempSync, readFileSync, existsSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { PidWatcher } from "./pid-watcher.js"; + +// Empirically pins the issue-#1 mechanism AND the fix on the real platform PTY +// (runs in the existing cross-OS CI matrix, no real claude needed). +// +// 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 that writes +// ~/.claude/sessions/.json. The bug: clarp polls a file that +// never exists. The fix: discover the real session file by cwd + recency. +// +// The stub mirrors how Claude installs per-platform (POSIX node-shebang script; +// Windows .cmd → node grandchild) and writes a session file. We then assert +// node-pty's reported pid vs the writer pid, and that a real PidWatcher finds +// the session regardless. + +const stubDir = join(dirname(fileURLToPath(import.meta.url)), "__fixtures__", "win-pid-stub"); +const RECORDED_CWD = process.platform === "win32" ? "C:\\clarp\\probe\\cwd" : "/clarp/probe/cwd"; + +function waitFor(predicate: () => boolean, timeoutMs: number, label: string): Promise { + return new Promise((resolve, reject) => { + const startedAt = Date.now(); + const poll = setInterval(() => { + if (predicate()) { clearInterval(poll); resolve(); } + else if (Date.now() - startedAt > timeoutMs) { clearInterval(poll); reject(new Error(label)); } + }, 50); + }); +} + +async function spawnProbe(binary: string): Promise<{ reportedPid: number; writerPid: number; discoveredSessionId: string | null }> { + const home = mkdtempSync(join(tmpdir(), "clarp-probe-home-")); + const sessionsDir = join(home, ".claude", "sessions"); + const pidFile = join(home, "child.pid"); + const startedAt = Date.now(); + const handle = nodePty.spawn(binary, [], { + name: "xterm-256color", + cols: 80, + rows: 24, + // Not `home`: Windows locks the spawned process's working directory, which + // would block cleanup. The stub writes into `home` via env paths instead. + cwd: tmpdir(), + env: { + ...process.env, + CLARP_PROBE_PIDFILE: pidFile, + CLARP_PROBE_SESSIONS_DIR: sessionsDir, + CLARP_PROBE_CWD: RECORDED_CWD, + }, + }); + try { + await waitFor(() => existsSync(pidFile), 10_000, "stub child never recorded its pid"); + const writerPid = Number(readFileSync(pidFile, "utf8").trim()); + await waitFor(() => existsSync(join(sessionsDir, `${writerPid}.json`)), 10_000, "stub never wrote its session file"); + + // The reported pid is the wrapper on Windows; PidWatcher must still find the + // grandchild's session file by cwd + recency. + const watcher = new PidWatcher(handle.pid, { onStatusChange: () => {} }, home, { + cwd: RECORDED_CWD, + startedAt, + }); + const discoveredSessionId = watcher.getSessionId(); + watcher.stop(); + return { reportedPid: handle.pid, writerPid, discoveredSessionId }; + } finally { + try { handle.kill(); } catch { /* already gone */ } + try { rmSync(home, { recursive: true, force: true, maxRetries: 5, retryDelay: 100 }); } catch { /* ephemeral CI temp */ } + } +} + +describe("issue-#1 wrapper-pid mechanism and the discovery fix on the real PTY", () => { + it.runIf(process.platform === "win32")( + "Windows: pid mismatch is real, yet PidWatcher discovers the session", + async () => { + const { reportedPid, writerPid, discoveredSessionId } = await spawnProbe(join(stubDir, "wrapper.cmd")); + // The bug exists: node-pty reports the cmd wrapper, not the node grandchild. + expect(reportedPid).not.toBe(writerPid); + // The fix works: discovery finds the grandchild's session file by cwd. + expect(discoveredSessionId).toBe("probe-session"); + }, + 25_000, + ); + + it.runIf(process.platform !== "win32")( + "POSIX: reported pid IS the node process, and PidWatcher resolves the session", + async () => { + const { reportedPid, writerPid, discoveredSessionId } = await spawnProbe(join(stubDir, "child.mjs")); + expect(reportedPid).toBe(writerPid); + expect(discoveredSessionId).toBe("probe-session"); + }, + 25_000, + ); +}); diff --git a/src/session.test.ts b/src/session.test.ts index 6e6e508..eba7450 100644 --- a/src/session.test.ts +++ b/src/session.test.ts @@ -1421,7 +1421,10 @@ describe("handleStdinEof", () => { expect(ptyHandle.kills).toHaveLength(0); }); - it("times out instead of hanging when queued prompt never becomes ready", async () => { + it("times out via the startup watchdog when Claude is never observed", async () => { + // No status is ever observed (the Windows pid-mismatch / unobservable-session + // case), so the generous startup watchdog (120s) applies — not the 30s + // in-turn timeout — and it fails fast instead of hanging forever. const { controller, ptyHandle, exitCodes } = createTestController({ args: { inputFormat: "stream-json" } }); const stderr = vi.spyOn(process.stderr, "write").mockImplementation(() => true); await controller.start(); @@ -1432,8 +1435,11 @@ describe("handleStdinEof", () => { await Promise.resolve(); await vi.advanceTimersByTimeAsync(30_000); + // Still within the startup window — must not have fired yet. + expect(ptyHandle.kills).toHaveLength(0); - expect(stderr.mock.calls.map(c => String(c[0])).join("")).toContain("Timed out after 30s waiting for Claude"); + await vi.advanceTimersByTimeAsync(90_000); + expect(stderr.mock.calls.map(c => String(c[0])).join("")).toContain("Timed out after 120s waiting for Claude"); expect(ptyHandle.kills).toContain("SIGTERM"); controller.handleClaudeExit(143); @@ -1441,6 +1447,37 @@ describe("handleStdinEof", () => { expect(exitCodes).toEqual([1]); }); + it("emits a terminal error result on startup timeout so stream-json has a terminator", async () => { + const lines: string[] = []; + vi.spyOn(process.stdout, "write").mockImplementation((chunk: any) => { + lines.push(typeof chunk === "string" ? chunk : chunk.toString()); + return true; + }); + vi.spyOn(process.stderr, "write").mockImplementation(() => true); + const { controller } = createTestController({ args: { inputFormat: "stream-json" } }); + await controller.start(); + controller.enqueuePrompt("pending"); + controller.handleStdinEof(); + await Promise.resolve(); + await Promise.resolve(); + + await vi.advanceTimersByTimeAsync(120_000); + + const results = lines + .map(l => { try { return JSON.parse(l); } catch { return null; } }) + .filter(Boolean) + .filter((e: any) => e.type === "result"); + expect(results).toHaveLength(1); + expect(results[0].subtype).toBe("error"); + expect(results[0].is_error).toBe(true); + + // The terminal exit must not emit a second result. + controller.handleClaudeExit(143); + await Promise.resolve(); + expect(lines.map(l => { try { return JSON.parse(l); } catch { return null; } }) + .filter((e: any) => e?.type === "result")).toHaveLength(1); + }); + it("times out instead of hanging when queued prompt waits on unresolved permission", async () => { const { controller, fireStatus, ptyHandle, exitCodes } = createTestController({ args: { inputFormat: "stream-json" } }); const stderr = vi.spyOn(process.stderr, "write").mockImplementation(() => true); diff --git a/src/session.ts b/src/session.ts index 9b8fe89..3006cd3 100644 --- a/src/session.ts +++ b/src/session.ts @@ -43,6 +43,12 @@ function deepEqual(a: unknown, b: unknown): boolean { return ka.every((k) => deepEqual((a as Record)[k], (b as Record)[k])); } const READY_TIMEOUT_MS = 30_000; +// Generous bound for the startup phase: Claude must become observable within +// this window or clarp fails fast with a terminal result instead of hanging +// forever (the issue-#1 symptom when the session file is never found). Longer +// than READY_TIMEOUT_MS so a slow-but-healthy cold start (MCP load, big repo) +// isn't killed. +const STARTUP_READY_TIMEOUT_MS = 120_000; const EMPTY_TURN_TRANSCRIPT_GRACE_MS = 1500; const INTERRUPT_DISPATCH_ACK_TIMEOUT_MS = 2000; const INTERRUPT_ESCAPE_ACK_TIMEOUT_MS = 1000; @@ -84,6 +90,11 @@ export class SessionController { private turnCount = 0; private turnStart = 0; private claudeReady = false; + // True once Claude's status has been observed at all (any status). Distinct + // from claudeReady (set on idle): it marks the end of the startup phase, where + // never seeing a status means the session file was never found (issue #1). + private claudeObserved = false; + private resultEmitted = false; private waitingForAction = false; private processExited = false; private stdinClosed = false; @@ -125,9 +136,16 @@ export class SessionController { private transcriptTurnError: { message: string; status?: number } | null = null; constructor(private opts: SessionControllerOptions) { - this.pidWatcher = (opts.pidWatcherFactory ?? ((pid, callbacks) => new PidWatcher(pid, callbacks)))( + this.pidWatcher = ( + opts.pidWatcherFactory ?? + ((pid, callbacks) => + new PidWatcher(pid, callbacks, undefined, { cwd: opts.args.cwd, startedAt: this.startedAt })) + )( opts.pid, - { onStatusChange: (status, waitingFor, _data) => this.handleStatusChange(status, waitingFor) }, + { + onStatusChange: (status, waitingFor, _data) => this.handleStatusChange(status, waitingFor), + onWarning: (message) => this.log(`PID watcher: ${message}`), + }, ); } @@ -149,13 +167,11 @@ export class SessionController { this.opts.backend.onObservation((obs) => this.handleObservation(obs)); this.pidWatcherTimer = setTimeout(() => { this.startPidWatcherAndBackend().catch((err: Error) => { - this.reportAsyncError("Backend observation start failed", err); - this.requestShutdown(1); + this.failWithTerminalError("Backend observation start failed", err.message); }); }, 1500); this.runDispatchLoop().catch((err: Error) => { - this.reportAsyncError("Dispatch loop failed", err); - this.requestShutdown(1); + this.failWithTerminalError("Dispatch loop failed", err.message); }); } @@ -182,6 +198,13 @@ export class SessionController { */ handleStatusChange(status: string, waitingFor?: string): void { if (this.processExited) return; + // First observation ends the startup phase: the session file was found, so + // the generous startup watchdog stands down and any in-turn watchdog re-arms + // with the shorter timeout on the next dispatch pass. + if (!this.claudeObserved) { + this.claudeObserved = true; + this.clearReadinessTimer(); + } this.log(`Status: ${status}${waitingFor ? ` (${waitingFor})` : ""}`); this.startOrUpdateTranscriptObservation(); output.emitStatus(status, waitingFor); @@ -336,7 +359,8 @@ export class SessionController { this.completeInterrupt("process_exit"); if (this.turnActive) { this.turnActive = false; - if (!this.opts.backend.capabilities.emitsResults) { + if (!this.opts.backend.capabilities.emitsResults && !this.resultEmitted) { + this.resultEmitted = true; const text = output.getAccumulatedText(); if (code === 0) { output.emitResult("success", text, { durationMs: Date.now() - this.turnStart, numTurns: this.turnCount }); @@ -477,37 +501,59 @@ export class SessionController { } if (!this.readinessTimer) { + const timeoutMs = this.claudeObserved ? READY_TIMEOUT_MS : STARTUP_READY_TIMEOUT_MS; this.readinessTimer = setTimeout(() => { this.readinessTimer = null; if (!this.shouldArmReadinessTimer()) return; - const err = new Error( - `Timed out after ${READY_TIMEOUT_MS / 1000}s waiting for Claude to become ready. ` + + const message = + `Timed out after ${timeoutMs / 1000}s waiting for Claude to become ready. ` + `Claude may be showing a startup, trust, or permission prompt, or Clarp may be unable to observe Claude's PID status. ` + `Open Claude Code in this project to resolve any prompts, check that the project is trusted, ` + - `or use --dangerously-skip-permissions only when that matches your security policy.` - ); - this.reportAsyncError("Dispatch loop failed", err); - this.requestShutdown(1); + `or use --dangerously-skip-permissions when that matches your security policy.`; + this.failWithTerminalError("Readiness timeout", message); this.opQueue.wake(); - }, READY_TIMEOUT_MS); + }, timeoutMs); } return this.opQueue.waitForChange(); } private shouldArmReadinessTimer(): boolean { + if (this.processExited || this.shuttingDown || this.interruptInFlight !== null) return false; + if (this.promptDispatchInFlight) return false; + // Startup phase: Claude's status has never been observed. Arm + // unconditionally so a session that is never observable (e.g. the Windows + // pid mismatch) fails fast instead of hanging — including when the prompt + // was passed as a claude arg and the op-queue is empty (`clarp -p "..."`), + // the issue-#1 case. + if (!this.claudeObserved) return true; + // In-turn: only when a queued prompt is blocked waiting (unchanged). const blockedOnPermission = this.waitingForAction || this.pendingPermissionRequestId !== null; return ( this.opQueue.normalLength > 0 && !this.isReadyForPrompt() && - !this.promptDispatchInFlight && - this.interruptInFlight === null && - !this.processExited && - !this.shuttingDown && (!this.turnActive || blockedOnPermission) ); } + /** + * Logs an error and, for machine-readable output formats, emits a terminal + * `result` so a stream-json/json consumer always gets a parseable terminator + * instead of a truncated stream, then requests a non-zero shutdown. Emits at + * most one terminal result for the session. + */ + private failWithTerminalError(context: string, message: string): void { + this.reportAsyncError(context, new Error(message)); + if (!this.resultEmitted && this.opts.args.outputFormat !== "text") { + this.resultEmitted = true; + output.emitResult("error", message, { + durationMs: Date.now() - this.startedAt, + numTurns: this.turnCount, + }); + } + this.requestShutdown(1); + } + private processNextSessionOp(): boolean { const control = this.opQueue.dequeueControl(); if (control) { @@ -646,6 +692,7 @@ export class SessionController { private markReady(): void { this.clearReadinessTimer(); this.claudeReady = true; + this.claudeObserved = true; if (this.isReadyForPrompt()) this.opQueue.wake(); }