diff --git a/.github/scripts/smoke-pty.mjs b/.github/scripts/smoke-pty.mjs new file mode 100644 index 00000000..a143a481 --- /dev/null +++ b/.github/scripts/smoke-pty.mjs @@ -0,0 +1,42 @@ +// Loads node-pty from a globally installed tlive and spawns a real ConPTY. +// +// A prebuilt binary that resolves can still fail to load, and one that loads +// can still fail to spawn. Only a real install on a real Windows runner sees +// this; the unit-test job installs from the workspace with a full toolchain. + +import { createRequire } from 'node:module'; +import { join } from 'node:path'; + +const globalRoot = process.env.TLIVE_GLOBAL_ROOT; +if (!globalRoot) { + console.error('TLIVE_GLOBAL_ROOT is not set'); + process.exit(1); +} + +const require = createRequire(import.meta.url); +const ptyPath = require.resolve('node-pty', { paths: [join(globalRoot, 'tlive')] }); +const { spawn } = require(ptyPath); + +const p = spawn('cmd.exe', ['/c', 'echo pty-ok'], { cols: 80, rows: 24 }); +let out = ''; +let done = false; + +// 5s is a failure deadline, not a fixed wait: resolve as soon as the output +// arrives so a passing run doesn't burn 5s of runner time, while a ConPTY +// that never answers still fails instead of hanging the job. +const deadline = setTimeout(() => { + if (done) return; + done = true; + console.error('pty produced no output; got: ' + JSON.stringify(out)); + process.exit(1); +}, 5000); + +p.onData((d) => { + out += d; + if (!done && out.includes('pty-ok')) { + done = true; + clearTimeout(deadline); + console.log('pty loads and runs'); + process.exit(0); + } +}); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 57bdac28..5fa5ff10 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -109,3 +109,74 @@ jobs: ' echo "::endgroup::" done + + # Windows counterpart to install-smoke, and the only end-to-end Windows + # coverage that exists. `test (windows-latest)` runs unit tests; nothing else + # touches what a Windows user hits on day one — the node-pty win32 prebuild + # actually loading, a ConPTY actually spawning, %APPDATA% paths, the .cmd shim + # npm writes for the bin, and the named-pipe daemon. + # + # The linux install-smoke earned its place by catching node-pty 1.1.0 shipping + # no linux prebuild, which no unit test could see. Same reasoning here: issue + # #59 was a real Windows daemon crash that spent months filed as test flake + # precisely because nothing exercised the product on Windows. + # + # Single job, no matrix — a matrix renames the check and required contexts are + # matched by exact name (see the note on install-smoke above). + install-smoke-windows: + runs-on: windows-latest + steps: + - uses: actions/checkout@v4 + - uses: pnpm/action-setup@v4 + with: + version: 11 + - uses: actions/setup-node@v4 + with: + node-version: '22' + cache: pnpm + - run: pnpm install --frozen-lockfile + - run: pnpm run build + - run: npm pack + - name: install the real tarball globally + shell: pwsh + run: | + $tgz = Get-ChildItem -Filter tlive-*.tgz | Select-Object -First 1 + npm i -g $tgz.FullName --no-audit --no-fund + - name: tlive resolves through the .cmd shim + shell: pwsh + run: tlive --version + - name: node-pty loads and spawns a ConPTY + shell: pwsh + run: | + # `npm root -g` can in principle emit more than one line, which makes + # PowerShell 7+ treat the captured value as a string[] — .Trim() would + # then enumerate silently instead of throwing, and $env:TLIVE_GLOBAL_ROOT + # would end up joined with a space into a corrupted path. Out-String + # collapses it to a single string first. + $env:TLIVE_GLOBAL_ROOT = (npm root -g | Out-String).Trim() + node .github/scripts/smoke-pty.mjs + - name: daemon start / status / stop over the named pipe + shell: pwsh + run: | + # pwsh does not halt on a native (non-PowerShell) command's non-zero + # exit, and GitHub Actions only inspects $LASTEXITCODE once, after the + # whole script runs — so without explicit gates the step's verdict + # would come from `tlive stop` alone, which exits 0 even when nothing + # was running (stop is deliberately idempotent so `stop && start` + # chains work). Gate every command, and assert on `status`'s actual + # text rather than just its exit code: match the "running (pid ...)" + # line status.ts prints, not a bare "running" substring — "not + # running" also contains the word "running". + tlive start + if ($LASTEXITCODE -ne 0) { throw "tlive start failed ($LASTEXITCODE)" } + $s = tlive status | Out-String + if ($LASTEXITCODE -ne 0) { throw "tlive status failed ($LASTEXITCODE)" } + Write-Host $s + if ($s -notmatch 'daemon:\s+running \(pid') { throw "daemon not reported running:`n$s" } + tlive stop + if ($LASTEXITCODE -ne 0) { throw "tlive stop failed ($LASTEXITCODE)" } + - name: stop the daemon even if a step above failed + if: always() + shell: pwsh + run: tlive stop + continue-on-error: true diff --git a/src/kernel/__tests__/wait.ts b/src/kernel/__tests__/wait.ts new file mode 100644 index 00000000..c54f6144 --- /dev/null +++ b/src/kernel/__tests__/wait.ts @@ -0,0 +1,20 @@ +// Condition-based waiting for tests. +// +// Fixed `setTimeout` waits before an arrival assertion are the project's main +// source of Windows CI flake: named pipes are slower than unix sockets, so a +// margin that holds on Linux does not hold there (#45, #59's sibling red). +// vitest's default waitFor timeout is 1s, which is too tight for daemon +// bootstrap over a pipe. +// +// Use this for ARRIVAL assertions — "X has appeared". Do NOT use it for +// ABSENCE assertions — "X did not happen" needs a real elapsed interval, and a +// condition that is already true returns immediately, asserting nothing. + +import { vi } from 'vitest'; + +const TIMEOUT_MS = 5000; +const INTERVAL_MS = 20; + +export function until(assertion: () => void | Promise): Promise { + return vi.waitFor(assertion, { timeout: TIMEOUT_MS, interval: INTERVAL_MS }) as Promise; +} diff --git a/src/kernel/daemon/__tests__/bootstrap.test.ts b/src/kernel/daemon/__tests__/bootstrap.test.ts index 97c6505a..4e2dd073 100644 --- a/src/kernel/daemon/__tests__/bootstrap.test.ts +++ b/src/kernel/daemon/__tests__/bootstrap.test.ts @@ -6,6 +6,7 @@ import { bootstrapDaemon, shouldFastNullContinue, clampPermissionTimeout, makeCo import { request, daemonSocketPath } from '../../ipc/client'; import type { IMAdapter, IMChannel, OutgoingMessage, IncomingEnvelope } from '../../contracts/im-adapter'; import { SessionRegistry } from '../../web/session-registry'; +import { until } from '../../__tests__/wait.js'; // #45 — robustness helpers for this file's "held request" pattern // (const pending = request(...); …asserts…; await pending). On a slow/jittery @@ -177,7 +178,7 @@ describe('local-answer cancel + Stop fast-null (integration)', () => { { socketPath: sock, timeoutMs: 10_000 }, ); held(pending); - await new Promise((r) => setTimeout(r, 100)); // let the card go pending + await until(() => { expect(h.sessions.get('s1')?.pending).toBeDefined(); }); // let the card go pending await request( { kind: 'hook.event', event: { event: 'activity', cwd: '/w', sessionId: 's1', toolName: 'Bash', result: {} } }, { socketPath: sock, timeoutMs: 2000 }, @@ -344,8 +345,7 @@ describe('AskUserQuestion remote card (Task 9)', () => { { socketPath: sock, timeoutMs: 10_000 }, ); held(pending); - await new Promise((r) => setTimeout(r, 100)); - expect(notes).toHaveLength(1); // exactly once — not once per configured channel + await until(() => { expect(notes).toHaveLength(1); }); // exactly once — not once per configured channel expect(notes[0].title).toContain('Bash'); const card = sent[0] as { kind: 'card'; buttons?: Array<{ id: string; label: string }> }; adapter.fire({ channel: 'telegram', chatId: 'c1', userId: 'u1', messageId: 'x1', text: card.buttons!.find((b) => b.id.startsWith('approve:'))!.id, ts: 0 }); @@ -372,9 +372,8 @@ describe('AskUserQuestion remote card (Task 9)', () => { { socketPath: sock, timeoutMs: 10_000 }, ); held(pending); - await new Promise((r) => setTimeout(r, 150)); + await until(() => { expect(notes).toHaveLength(1); }); expect(sent).toHaveLength(0); // …but the desktop already knows - expect(notes).toHaveLength(1); // Local answer within grace (PostToolUse cancel) → card never sent, toast cleared. await request( { kind: 'hook.event', event: { event: 'activity', cwd: '/w', sessionId: 's1', toolName: 'Bash', result: {} } }, @@ -405,9 +404,8 @@ describe('AskUserQuestion remote card (Task 9)', () => { { socketPath: sock, timeoutMs: 10_000 }, ); held(pending); - await new Promise((r) => setTimeout(r, 100)); + await until(() => { expect(sent).toHaveLength(1); }); expect(notes).toHaveLength(0); // toast gated off; the IM card still went out - expect(sent).toHaveLength(1); const card = sent[0] as { kind: 'card'; buttons?: Array<{ id: string; label: string }> }; adapter.fire({ channel: 'telegram', chatId: 'c1', userId: 'u1', messageId: 'x1', text: card.buttons!.find((b) => b.id.startsWith('approve:'))!.id, ts: 0 }); await pending; @@ -437,8 +435,7 @@ describe('AskUserQuestion remote card (Task 9)', () => { { socketPath: sock, timeoutMs: 10_000 }, ); held(pending); - await new Promise((r) => setTimeout(r, 150)); - expect(sent).toHaveLength(2); // one card per channel… + await until(() => { expect(sent).toHaveLength(2); }); // one card per channel… expect(notes).toHaveLength(1); // …but a single desktop ping const card = sent[0] as { kind: 'card'; buttons?: Array<{ id: string; label: string }> }; tg.fire({ channel: 'telegram', chatId: 'c1', userId: 'u1', messageId: 'x1', text: card.buttons!.find((b) => b.id.startsWith('approve:'))!.id, ts: 0 }); @@ -458,9 +455,8 @@ describe('AskUserQuestion remote card (Task 9)', () => { // and Codex turn/completed paths both funnel through). Floating request with // a tiny window; we only assert the notification surfaces, not the reply. void h.continueBroker.request({ cwd: '/w', context: 'Finished building the feature', timeoutSec: 1 }); - await new Promise((r) => setTimeout(r, 50)); + await until(() => { expect(sent).toHaveLength(1); }); // …still surfaced on IM expect(infos).toHaveLength(0); // no desktop flood on completion… - expect(sent).toHaveLength(1); // …still surfaced on IM expect((sent[0] as { title?: string }).title).toContain('Turn finished'); }); @@ -545,7 +541,7 @@ describe('AskUserQuestion remote card (Task 9)', () => { { socketPath: sock, timeoutMs: 10_000 }, ); held(pending); - await new Promise((r) => setTimeout(r, 100)); + await waitForSent(sent); // let the card go out before referencing its messageId const cardMsgId = 'm1'; // interactiveAdapter assigns sequential ids m1, m2, … adapter.fire({ channel: 'telegram', chatId: 'c1', userId: 'u1', messageId: 'x2', text: 'use mv to the scratchpad instead', replyToMessageId: cardMsgId, ts: 0 }); const r = await pending as { decision?: string; message?: string }; @@ -645,15 +641,13 @@ describe('AskUserQuestion remote card (Task 9)', () => { // fix) — the edit lands on a later microtask, no longer synchronously // within fire(), so each assertion needs a tick to let the queue drain. adapter.fire({ channel: 'telegram', chatId: 'c1', userId: 'u1', messageId: 'x1', text: toggleBlue.id, ts: 0 }); - await new Promise((r) => setTimeout(r, 0)); - expect(edits).toHaveLength(1); + await until(() => { expect(edits).toHaveLength(1); }); const edited1 = edits[0].msg as { kind: 'card'; buttons?: Array<{ id: string; label: string }> }; expect(edited1.buttons!.map((b) => b.label)).toEqual(['▢ Red', '▣ Blue', 'Submit (1)', 'Skip']); // toggling the same option again flips it back off adapter.fire({ channel: 'telegram', chatId: 'c1', userId: 'u1', messageId: 'x1', text: toggleBlue.id, ts: 0 }); - await new Promise((r) => setTimeout(r, 0)); - expect(edits).toHaveLength(2); + await until(() => { expect(edits).toHaveLength(2); }); const edited2 = edits[1].msg as { kind: 'card'; buttons?: Array<{ id: string; label: string }> }; expect(edited2.buttons!.map((b) => b.label)).toEqual(['▢ Red', '▢ Blue', 'Submit (0)', 'Skip']); @@ -876,8 +870,7 @@ describe('quoting a live approval card = deny with guidance (Task 7)', () => { expect(r.decision).toBe('deny'); expect(r.message).toBe('Do not use rm -rf, move it to /tmp/.trash instead'); - await new Promise((res) => setTimeout(res, 50)); // let the queued settlement edit land - expect(edits).toHaveLength(1); + await until(() => { expect(edits).toHaveLength(1); }); // let the queued settlement edit land const editedTitle = (edits[0].msg as { title?: string }).title ?? ''; expect(editedTitle).toContain('Denied with guidance'); }); @@ -904,8 +897,7 @@ describe('quoting a live approval card = deny with guidance (Task 7)', () => { adapter.fire({ channel: 'telegram', chatId: 'c1', userId: 'u1', messageId: 'x1', text: denyBtn.id, ts: 0 }); await pending; - await new Promise((res) => setTimeout(res, 50)); - expect(edits).toHaveLength(1); + await until(() => { expect(edits).toHaveLength(1); }); const editedTitle = (edits[0].msg as { title?: string }).title ?? ''; expect(editedTitle).toContain('Denied'); expect(editedTitle).not.toContain('Denied with guidance'); diff --git a/src/kernel/daemon/__tests__/event-broadcast.test.ts b/src/kernel/daemon/__tests__/event-broadcast.test.ts index 7dc8b18a..789ec87a 100644 --- a/src/kernel/daemon/__tests__/event-broadcast.test.ts +++ b/src/kernel/daemon/__tests__/event-broadcast.test.ts @@ -7,6 +7,7 @@ import { WebSocket } from 'ws'; import { bootstrapDaemon, type DaemonHandle } from '../bootstrap.js'; import { request } from '../../ipc/client.js'; import type { IMAdapter, IMChannel, OutgoingMessage, IncomingEnvelope } from '../../contracts/im-adapter.js'; +import { until } from '../../__tests__/wait.js'; let tmp: string; let h: DaemonHandle; @@ -109,8 +110,7 @@ describe('daemon → /ws/events downstream broadcast', () => { expect(f.session.lastMessage).toBe('last_msg'); // Effect 2: ContinueBroker received the request → IM message sent containing requestId. - await new Promise((r) => setTimeout(r, 100)); - expect(capturedMsg).toMatch(/last_msg/); // excerpt = 真正的最后一句 + await until(() => { expect(capturedMsg).toMatch(/last_msg/); }); // excerpt = the actual last message // requestId no longer appears in the display text — take it from the registry const continueId = h.sessions.get('s')!.continueId!; expect(continueId).toMatch(/[a-f0-9-]{36}/); @@ -182,20 +182,22 @@ describe('daemon → /ws/events downstream broadcast', () => { const f = await waitFor(frames, (x) => x.type === 'session-upsert' && x.session.id === 's2' && x.session.status === 'waiting-approval'); const rid = f.session.pending.requestId; expect(f.session.pending.ask).toMatchObject({ question: 'First?', index: 0, total: 2 }); - await new Promise((r) => setTimeout(r, 100)); - const first = sentIm[0] as { title?: string; body: string }; - expect(first.title).toContain('Question 1/2'); + await until(() => { + expect(sentIm[0]).toBeDefined(); + expect((sentIm[0] as { title?: string })?.title).toContain('Question 1/2'); + }); // Answer question 1 from the dashboard — the batch must NOT resolve yet, // and BOTH surfaces must move to question 2. ws.send(JSON.stringify({ type: 'ask', requestId: rid, picks: [0] })); const second = await waitFor(frames, (x) => x.type === 'session-upsert' && x.session.id === 's2' && x.session.pending?.ask?.index === 1); expect(second.session.pending.ask).toMatchObject({ question: 'Second?', index: 1, total: 2 }); - await new Promise((r) => setTimeout(r, 100)); + await until(() => { + expect((edits.at(-1) as { title?: string } | undefined)?.title).toContain('Question 2/2'); + }); // The IM card follows the dashboard: title progress AND body, not just one // of them (the badge froze at "Question 1/2" while the body moved on). const edited = edits.at(-1) as { title?: string; body: string; buttons?: Array<{ id: string }> }; - expect(edited.title).toContain('Question 2/2'); expect(edited.body).toContain('Second?'); expect(edited.buttons?.map((b) => b.id)).toContain(`askback:${rid}`); @@ -225,8 +227,7 @@ describe('daemon → /ws/events downstream broadcast', () => { ); const f = await waitFor(frames, (x) => x.type === 'session-upsert' && x.session.id === 's' && x.session.status === 'waiting-approval'); expect(f.session.pending.ask).toEqual({ question: 'Pick a color?', options: [{ label: 'Red' }, { label: 'Blue' }], multiSelect: false, index: 0, total: 1 }); - await new Promise((r) => setTimeout(r, 100)); - expect(sentIm).toHaveLength(1); // IM still gets the ask card with option buttons + await until(() => { expect(sentIm).toHaveLength(1); }); // IM still gets the ask card with option buttons // Answer from the dashboard: pick "Blue" → allow + updatedInput.answers // (same wire as the IM buttons — CC treats the tool as answered). @@ -342,7 +343,26 @@ describe('daemon → /ws/events downstream broadcast', () => { h = await bootstrapDaemon({ home: tmp, imAdapters: [adapter] }); const t0 = Date.now(); const p = request({ kind: 'hook.continue.request', cwd: '/g', sessionId: 's', context: 'ctx' }, { socketPath: sock, timeoutMs: 8000 }); - await new Promise((r) => setTimeout(r, 150)); + // No `until()` here on purpose: this is a sequencing gate (let the request + // above reach the daemon's grace-registration point), not an arrival + // assertion, and there is no arrival to poll for. `h.sessions.get('s')` + // stays undefined and `continueId` is never set on THIS (suppressed) path + // — continueId is only ever assigned inside ContinueBroker's onRequest + // callback, which fires from `continueBroker.request()`, and that call is + // skipped entirely once `suppressed` resolves true (bootstrap.ts's + // 'hook.continue.request' case returns before reaching it). Waiting on + // continueId would hang for the full 5s `until()` timeout every run. + // This is the one fixed sleep in this file that has no daemon-observable + // substitute (the other fixed waits elsewhere in this file are deliberate + // absence assertions, not stand-ins for this). The margin is widened + // (150ms → 600ms) as the only mitigation available while the daemon + // exposes no grace-registration signal: too short and the suppression + // never arms, failing this test outright on the `toBeLessThan(4000)` below + // rather than merely flaking. 600ms still leaves ample room under that 4s + // budget. If the daemon ever exposes a grace-registration signal (e.g. a + // test-only callback off `bootstrapDaemon`/`DaemonHandle`), convert this to + // an `until()` on it instead. + await new Promise((r) => setTimeout(r, 600)); // user starts a new turn within the grace window → suppress the continue card await request({ kind: 'hook.event', event: { event: 'prompt', cwd: '/g', sessionId: 's', prompt: 'next' } }, { socketPath: sock, timeoutMs: 2000 }); const res = (await p) as { reply: string | null }; @@ -380,15 +400,14 @@ describe('daemon → /ws/events downstream broadcast', () => { // seed the session so the card gets a label tag await request({ kind: 'hook.event', event: { event: 'session-start', cwd: '/tag/repo', sessionId: 's', source: 'startup' } }, { socketPath: sock, timeoutMs: 2000 }); const p = request({ kind: 'hook.permission.request', cwd: '/tag/repo', sessionId: 's', toolName: 'Edit', input: { file_path: '/x', old_string: 'a', new_string: 'b' } }, { socketPath: sock, timeoutMs: 5000 }); - await new Promise((r) => setTimeout(r, 150)); + await until(() => { expect(sent.find((s) => s.kind === 'card')).toBeDefined(); }); const card = sent.find((s) => s.kind === 'card'); expect(card?.title).toContain('repo · '); // session tag prefix (still basename(cwd) — label is unaffected by the key change) // answer → card edited to outcome. Keyed by session id ('s'), not cwd. const reqId = h.sessions.get('s')!.pending!.requestId; await request({ kind: 'hook.permission.answer', requestId: reqId, approved: false }, { socketPath: sock, timeoutMs: 2000 }); await p; - await new Promise((r) => setTimeout(r, 100)); - expect(edits.length).toBe(1); + await until(() => { expect(edits.length).toBe(1); }); expect(edits[0].title).toContain('Denied'); }); @@ -458,7 +477,12 @@ describe('daemon → /ws/events downstream broadcast', () => { // resolve A (deny) — must NOT wipe B's pending indicator await request({ kind: 'hook.permission.answer', requestId: reqA, approved: false }, { socketPath: sock, timeoutMs: 2000 }); expect(((await pA) as { decision: string }).decision).toBe('deny'); - await new Promise((r) => setTimeout(r, 80)); + // Invariant, not an arrival: `reqB` was read off the frame where pending had + // ALREADY moved to B (see the waitFor above), so this asserts that denying A + // did not wipe B's indicator. A condition that is already true needs real + // elapsed time to mean anything — until() would return on its first check and + // prove nothing. + await new Promise((r) => setTimeout(r, 200)); expect(h.sessions.get('s')?.pending?.requestId).toBe(reqB); // cleanup await request({ kind: 'hook.permission.answer', requestId: reqB, approved: false }, { socketPath: sock, timeoutMs: 2000 }); @@ -531,8 +555,7 @@ describe('daemon → /ws/events downstream broadcast', () => { ); await waitFor(frames, (x) => x.type === 'session-upsert' && x.session.id === 's' && x.session.lastMessage === 'Bash failed: permission denied'); - await new Promise((r) => setTimeout(r, 100)); - expect(sent).toHaveLength(1); + await until(() => { expect(sent).toHaveLength(1); }); expect(sent[0]).toContain('permission denied'); }); @@ -550,8 +573,7 @@ describe('daemon → /ws/events downstream broadcast', () => { ); await waitFor(frames, (x) => x.type === 'session-upsert' && x.session.id === 's'); - await new Promise((r) => setTimeout(r, 100)); - expect(sent).toHaveLength(1); + await until(() => { expect(sent).toHaveLength(1); }); }); }); diff --git a/src/kernel/daemon/__tests__/session-ipc.test.ts b/src/kernel/daemon/__tests__/session-ipc.test.ts index 7199da29..634d799e 100644 --- a/src/kernel/daemon/__tests__/session-ipc.test.ts +++ b/src/kernel/daemon/__tests__/session-ipc.test.ts @@ -7,6 +7,7 @@ import { bootstrapDaemon, type DaemonHandle } from '../bootstrap'; import { request, daemonSocketPath } from '../../ipc/client'; import type { SessionMeta } from '../../ipc/protocol'; import type { IMAdapter, OutgoingMessage } from '../../contracts/im-adapter'; +import { until } from '../../__tests__/wait.js'; const recordingAdapter = (sent: OutgoingMessage[]): IMAdapter => ({ channel: 'telegram', @@ -54,8 +55,7 @@ describe('session.* over IPC', () => { }); }); -describe('parent-session 清场 must be agent-scoped (backgrounded sub-agent approval survival)', () => { - const tick = () => new Promise((r) => setTimeout(r, 40)); +describe('parent-session teardown must be agent-scoped (backgrounded sub-agent approval survival)', () => { // A configured chat keeps requestPermission pending; with no injected imAdapters // sendToChat is a no-op (no network), so the pending simply sits in the router. // holdSubagents:true — these tests exercise the *held* sub-agent path (the @@ -73,14 +73,13 @@ describe('parent-session 清场 must be agent-scoped (backgrounded sub-agent app writeConfig(); h = await bootstrapDaemon({ home: tmp }); const sub = fireSubAgentApproval(); - await tick(); - expect(h.permissionRouter.pendingCount()).toBe(1); + await until(() => { expect(h.permissionRouter.pendingCount()).toBe(1); }); // The parent (main session, no agent_id) submits a new prompt while the // sub-agent is still waiting. The sub-agent's tool call is genuinely pending // and has no local answer path — its card must survive. await request({ kind: 'hook.event', event: { event: 'prompt', cwd: '/proj', sessionId: 'parent', prompt: 'do something else' } }, { socketPath: sock, timeoutMs: 2000 }); - await tick(); + await new Promise((r) => setTimeout(r, 100)); // real elapsed time: prove the cancel did NOT arrive expect(h.permissionRouter.pendingCount()).toBe(1); h.permissionRouter.cancel({ key: 'parent' }); @@ -92,12 +91,10 @@ describe('parent-session 清场 must be agent-scoped (backgrounded sub-agent app h = await bootstrapDaemon({ home: tmp }); const main = request({ kind: 'hook.permission.request', cwd: '/proj', sessionId: 'parent', toolName: 'Bash', input: {} }, { socketPath: sock, timeoutMs: 4000 }).catch(() => undefined); // no agentId = main session - await tick(); - expect(h.permissionRouter.pendingCount()).toBe(1); + await until(() => { expect(h.permissionRouter.pendingCount()).toBe(1); }); await request({ kind: 'hook.event', event: { event: 'prompt', cwd: '/proj', sessionId: 'parent', prompt: 'next' } }, { socketPath: sock, timeoutMs: 2000 }); - await tick(); - expect(h.permissionRouter.pendingCount()).toBe(0); // main-session dialog is gone → its card is withdrawn + await until(() => { expect(h.permissionRouter.pendingCount()).toBe(0); }); // main-session dialog is gone → its card is withdrawn await main; }); @@ -106,13 +103,12 @@ describe('parent-session 清场 must be agent-scoped (backgrounded sub-agent app writeConfig(); h = await bootstrapDaemon({ home: tmp }); const sub = fireSubAgentApproval(); - await tick(); - expect(h.permissionRouter.pendingCount()).toBe(1); + await until(() => { expect(h.permissionRouter.pendingCount()).toBe(1); }); // Stop long-polls (grace + continue window); fire-and-forget — the cancel // it performs runs synchronously on arrival, before any waiting. const stop = request({ kind: 'hook.continue.request', cwd: '/proj', sessionId: 'parent', context: 'turn ended' }, { socketPath: sock, timeoutMs: 300 }).catch(() => undefined); - await tick(); + await new Promise((r) => setTimeout(r, 100)); // real elapsed time: prove the cancel did NOT arrive expect(h.permissionRouter.pendingCount()).toBe(1); h.permissionRouter.cancel({ key: 'parent' }); @@ -147,10 +143,11 @@ describe('continuation card has no on-card input box (quote-reply is the entry)' // Stop long-polls (continue window); fire-and-forget — the card is sent // synchronously when continueBroker registers the request, before the wait. request({ kind: 'hook.continue.request', cwd: '/proj', sessionId: 'sess', context: 'All green.' }, { socketPath: sock, timeoutMs: 300 }).catch(() => undefined); - await new Promise((r) => setTimeout(r, 120)); - const card = sent.find((m) => m.kind === 'card' && (m.title ?? '').includes('Turn finished')); - expect(card).toBeTruthy(); - expect((card as { inputAction?: unknown }).inputAction).toBeUndefined(); + await until(() => { + const card = sent.find((m) => m.kind === 'card' && (m.title ?? '').includes('Turn finished')); + expect(card).toBeTruthy(); + expect((card as { inputAction?: unknown }).inputAction).toBeUndefined(); + }); }); }); diff --git a/src/kernel/ipc/__tests__/server.test.ts b/src/kernel/ipc/__tests__/server.test.ts index fbaaeede..88ec529b 100644 --- a/src/kernel/ipc/__tests__/server.test.ts +++ b/src/kernel/ipc/__tests__/server.test.ts @@ -5,6 +5,7 @@ import { join } from 'node:path'; import { createConnection } from 'node:net'; import { startIpcServer, type IpcServer } from '../server'; import { request, daemonSocketPath } from '../client'; +import { until } from '../../__tests__/wait.js'; let cleanup: Array<() => void> = []; afterEach(() => { cleanup.forEach((f) => f()); cleanup = []; }); @@ -36,8 +37,7 @@ describe('IpcCallContext.onDisconnect', () => { setTimeout(() => { sock.destroy(); resolve(); }, 30); }); }); - await new Promise((r) => setTimeout(r, 60)); - expect(fired).toBe(true); + await until(() => { expect(fired).toBe(true); }); }); it('does not fire synchronously when registered, before any disconnect has occurred', async () => { diff --git a/src/kernel/pty/__tests__/session-host.test.ts b/src/kernel/pty/__tests__/session-host.test.ts index f100ccef..dd449384 100644 --- a/src/kernel/pty/__tests__/session-host.test.ts +++ b/src/kernel/pty/__tests__/session-host.test.ts @@ -1,15 +1,28 @@ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, vi, afterAll } from 'vitest'; import { createConnection } from 'node:net'; import { mkdtempSync } from 'node:fs'; import { join, basename } from 'node:path'; import { tmpdir } from 'node:os'; import { SessionHost, authoritativeSize } from '../session-host'; +// Spied (NOT auto-mocked) for the wiring test below: on Linux +// guardWindowsConinSocket is a no-op by design (see win-conin-guard.ts), so +// no black-box test can see whether session-host.ts still calls it. `vi.mock` +// is file-global and hoisted, so a plain automock would replace the guard +// with a no-op for every OTHER test in this file too — including the ones +// that spawn real ptys and drive real writes and kills, which is exactly +// where a real Windows conin write failure would need swallowing. `spy: true` +// keeps the real implementation running for all of them and only adds +// call/return tracking on top. +vi.mock('../win-conin-guard.js', { spy: true }); +import { guardWindowsConinSocket } from '../win-conin-guard.js'; + // Per-session socket: fs path on POSIX (directly in `dir`, no subdir), named // pipe on win32. basename(dir) is unique per mkdtemp → collision-free pipe. const sessSock = (dir: string, name: string): string => process.platform === 'win32' ? `\\\\.\\pipe\\tlive-t-${basename(dir)}-${name}` : join(dir, `${name}.sock`); import { FrameDecoder, FrameType, encodeAttach, encodeData, parseDims } from '../../web/stream-protocol.js'; +import { until } from '../../__tests__/wait.js'; describe('authoritativeSize', () => { it('defaults to 80x24 with no sources', () => { @@ -148,26 +161,55 @@ describe('SessionHost (socket-only, attachLocal:false)', () => { attachLocal: false, }); await host.start(); - // let the output land in the shadow terminal before anyone attaches - await new Promise((r) => setTimeout(r, 400)); + + // A client that is already watching the session. It stays attached for the rest + // of the test: "late joiner" means "attached after the output happened", not + // "the only client" — every client gets its own snapshot on its own first attach, + // and the web terminal really does have tabs joining a session others are in. + // The error handler goes on before the first write; see the comment in the + // sizing test above for why. + const probe = createConnection(sockPath); + probe.on('error', () => { /* teardown race, not a test failure */ }); + await new Promise((r) => probe.once('connect', () => r())); + probe.write(encodeAttach(80, 24)); + + // The precondition the snapshot path needs: EARLY-SCREEN must have reached the + // shadow terminal BEFORE the client under test attaches. Otherwise the snapshot + // is empty, the live stream carries the string anyway, and this test passes + // without touching the snapshot path at all. + // + // Seeing EARLY-SCREEN arrive on a live client does NOT establish that: pty.onData + // broadcasts to sockets synchronously, but xterm's write buffer parses on a later + // macrotask, so serialize() can still return '' while those bytes are already on + // the wire. Wait on the shadow's own content instead — the thing the snapshot is + // built from. (onActivity is no use either: on Windows ConPTY emits initialisation + // sequences before the child writes anything, so a flip says nothing about output.) + const internals = host as unknown as { serializer: { serialize(): string } | null }; + await until(() => { expect(internals.serializer?.serialize() ?? '').toContain('EARLY-SCREEN'); }); const dec = new FrameDecoder(); const got = await new Promise((resolve, reject) => { - const t = setTimeout(() => reject(new Error('no snapshot')), 8000); - const chunks: Buffer[] = []; + const t = setTimeout(() => reject(new Error('late joiner got no snapshot')), 8000); + let firstDataPayload = ''; + let gotFirstData = false; const sock = createConnection(sockPath, () => { sock.write(encodeAttach(80, 24)); }); sock.on('error', reject); sock.on('data', (chunk: Buffer) => { for (const f of dec.push(chunk)) { - if (f.type === FrameType.Data) { - chunks.push(f.payload); - const s = Buffer.concat(chunks).toString('utf8'); - if (s.includes('EARLY-SCREEN')) { clearTimeout(t); sock.end(); resolve(s); } + if (f.type === FrameType.Data && !gotFirstData) { + // The first Data frame on a first attach is the snapshot (serializer.serialize). + // Live output (from pty.onData) comes after. + gotFirstData = true; + firstDataPayload = f.payload.toString('utf8'); + clearTimeout(t); + sock.end(); + resolve(firstDataPayload); } } }); }); expect(got).toContain('EARLY-SCREEN'); + probe.end(); await host.stop(); }); @@ -204,6 +246,32 @@ describe('SessionHost (socket-only, attachLocal:false)', () => { await host.stop(); }); + it.skipIf(process.platform === 'win32')('a silent child (no output) does not report as running', async () => { + const dir = mkdtempSync(join(tmpdir(), 'tlive-host-')); + const sockPath = sessSock(dir, 's'); + // child that consumes stdin but produces no output + const host = new SessionHost({ + id: 'act-silent', cmd: process.execPath, + args: ['-e', 'process.stdin.resume(); setInterval(()=>{},1000);'], + cwd: dir, sockPath, attachLocal: false, + }); + const flips: boolean[] = []; + host.onActivity((a) => flips.push(a)); + await host.start(); + // Not an absence-assertion violation: the `false` flip below is a genuine + // arrival (the first poll tick, with no output yet, always reports idle), + // so until() has something real to wait on. The `not.toContain(true)` + // check afterwards is then made against state that can no longer change, + // because this test's child never writes — there is no later tick that + // could still flip it to running. + // Note: ConPTY emits initialisation sequences before any child output, + // so a pty with no output does not exist on Windows and the assertion + // is therefore meaningless rather than merely flaky. This is why the test + // is skipped there. + await until(() => { expect(flips).toContain(false); }); // a tick demonstrably ran… + expect(flips).not.toContain(true); // …and it reported idle + await host.stop(); + }); it('reports running on output then idle after silence', async () => { const dir = mkdtempSync(join(tmpdir(), 'tlive-host-')); @@ -217,11 +285,79 @@ describe('SessionHost (socket-only, attachLocal:false)', () => { const flips: boolean[] = []; host.onActivity((a) => flips.push(a)); await host.start(); - // within ~1s: running (true); after IDLE_MS(1.5s)+poll: idle (false) - await new Promise((r) => setTimeout(r, 3200)); - expect(flips[0]).toBe(true); // saw output → running - expect(flips).toContain(false); // then went idle + // Both are arrivals — wait on each instead of a fixed sleep. (flips[0] is + // no longer guaranteed to be `true` by construction: it now races the + // child's first byte against the poll tick at spawn+~1000ms.) + await until(() => { expect(flips).toContain(true); }); + await until(() => { expect(flips).toContain(false); }); await host.stop(); }); + // Regression: stop() killed the pty but left the handle in place, so a client + // Data frame already queued could still reach a pty whose win32 conin socket + // had just been destroyed — an uncaught 'write EAGAIN'. See #59. + it('clears the pty handle on stop so late input cannot reach a killed pty', async () => { + const dir = mkdtempSync(join(tmpdir(), 'tlive-dispose-')); + const host = new SessionHost({ + id: 'd1', cmd: process.execPath, args: ['-e', 'process.stdin.pipe(process.stdout)'], + cwd: dir, sockPath: sessSock(dir, 'd'), attachLocal: false, + }); + await host.start(); + expect((host as unknown as { pty: unknown }).pty).not.toBeNull(); + + await host.stop(); + expect((host as unknown as { pty: unknown }).pty).toBeNull(); + }); + + it('clears the pty handle when the child exits on its own', async () => { + const dir = mkdtempSync(join(tmpdir(), 'tlive-exit-')); + const host = new SessionHost({ + id: 'd2', cmd: process.execPath, args: ['-e', 'process.exit(0)'], + cwd: dir, sockPath: sessSock(dir, 'e'), attachLocal: false, + }); + const exited = new Promise((resolve) => host.onExit(resolve)); + await host.start(); + await exited; + expect((host as unknown as { pty: unknown }).pty).toBeNull(); + }); + + // Regression guard for #59: guardWindowsConinSocket() is a no-op on Linux + // by design, so nothing black-box can tell whether start() still calls it — + // dropping the call here would leave the whole suite green. Assert the + // wiring directly via the spied import instead. + it('wires guardWindowsConinSocket into start()', async () => { + const dir = mkdtempSync(join(tmpdir(), 'tlive-guard-')); + const host = new SessionHost({ + id: 'guard-1', cmd: process.execPath, args: ['-e', 'setInterval(()=>{},1000)'], + cwd: dir, sockPath: sessSock(dir, 'g'), attachLocal: false, + }); + // Captured immediately before the action under test, not asserted as an + // absolute count: other tests in this file also call start() against the + // same spied import, so only the delta from this one call is meaningful. + const before = vi.mocked(guardWindowsConinSocket).mock.calls.length; + await host.start(); + expect(vi.mocked(guardWindowsConinSocket).mock.calls.length).toBe(before + 1); + // `spy: true` runs the REAL body, not a stub, proven by the return value: + // an automocked function would return undefined, but the real + // implementation returns a platform-dependent boolean (false off win32, + // true on win32 once it finds a live conin socket to attach to). + expect(vi.mocked(guardWindowsConinSocket).mock.results.at(-1)?.value).toBe(process.platform === 'win32'); + await host.stop(); + }); + + // Confirms `spy: true` above is genuinely running the guard's own body for + // EVERY test in this file, not just the one above — an automocked stub + // would return undefined for all of them, whereas the real implementation + // always returns a platform-dependent boolean. If this ever fails, the + // guard has silently gone back to being a no-op across the whole file, + // which is the exact regression this test exists to catch. + afterAll(() => { + const results = vi.mocked(guardWindowsConinSocket).mock.results; + expect(results.length).toBeGreaterThan(1); // every SessionHost.start() above called it too + for (const r of results) { + expect(r.type).toBe('return'); + expect(r.value).toBe(process.platform === 'win32'); + } + }); + }); diff --git a/src/kernel/pty/__tests__/win-conin-guard.test.ts b/src/kernel/pty/__tests__/win-conin-guard.test.ts new file mode 100644 index 00000000..a0c9d9c9 --- /dev/null +++ b/src/kernel/pty/__tests__/win-conin-guard.test.ts @@ -0,0 +1,37 @@ +import { describe, it, expect } from 'vitest'; +import { EventEmitter } from 'node:events'; +import type { IPty } from 'node-pty'; +import { guardWindowsConinSocket } from '../win-conin-guard.js'; + +/** Shaped like node-pty's WindowsTerminal: a private _agent exposing inSocket. */ +const fakeWinPty = (): { pty: IPty; inSocket: EventEmitter } => { + const inSocket = new EventEmitter(); + return { pty: { _agent: { inSocket } } as unknown as IPty, inSocket }; +}; + +describe('guardWindowsConinSocket', () => { + it('attaches an error listener to the conin socket on win32', () => { + const { pty, inSocket } = fakeWinPty(); + expect(guardWindowsConinSocket(pty, 'win32')).toBe(true); + expect(inSocket.listenerCount('error')).toBe(1); + }); + + it('swallows a conin error instead of letting it become an uncaught exception', () => { + const { pty, inSocket } = fakeWinPty(); + guardWindowsConinSocket(pty, 'win32'); + // Without a listener, EventEmitter rethrows. With one, this is a no-op. + expect(() => inSocket.emit('error', Object.assign(new Error('write EAGAIN'), { code: 'EAGAIN' }))).not.toThrow(); + }); + + it('does nothing on non-win32 platforms', () => { + const { pty, inSocket } = fakeWinPty(); + expect(guardWindowsConinSocket(pty, 'linux')).toBe(false); + expect(inSocket.listenerCount('error')).toBe(0); + }); + + it('reports false rather than throwing when node-pty no longer exposes the socket', () => { + // Future-proofing: an upstream rename must degrade to a no-op, not a crash. + expect(guardWindowsConinSocket({} as IPty, 'win32')).toBe(false); + expect(guardWindowsConinSocket({ _agent: {} } as unknown as IPty, 'win32')).toBe(false); + }); +}); diff --git a/src/kernel/pty/session-host.ts b/src/kernel/pty/session-host.ts index cb8a7a8b..5137107b 100644 --- a/src/kernel/pty/session-host.ts +++ b/src/kernel/pty/session-host.ts @@ -12,6 +12,7 @@ import { Terminal as HeadlessTerminal } from '@xterm/headless'; import { SerializeAddon } from '@xterm/addon-serialize'; import { isPipePath } from '../ipc/client.js'; import { FrameDecoder, FrameType, encodeData, encodeSize, parseDims } from '../web/stream-protocol.js'; +import { guardWindowsConinSocket } from './win-conin-guard.js'; export interface SessionHostOpts { id: string; @@ -57,7 +58,7 @@ export class SessionHost { private shadow: HeadlessTerminal | null = null; private serializer: SerializeAddon | null = null; // Vendor-neutral activity: recent pty output = running, silence = idle. - private lastOutputAt = Date.now(); + private lastOutputAt: number | null = null; private activeState: boolean | null = null; private activityTimer: ReturnType | null = null; private onActivityCb: ((active: boolean) => void) | null = null; @@ -84,6 +85,9 @@ export class SessionHost { // lets scripts detect the wrapper and lets `tlive run` refuse to nest. env: { ...(this.opts.env ?? process.env), TLIVE_SESSION: this.opts.id } as Record, }); + // node-pty leaves the win32 conin socket without an 'error' listener; an + // unguarded write failure there is an uncaught exception. See the module. + guardWindowsConinSocket(this.pty); this.shadow = new HeadlessTerminal({ cols: size.cols, rows: size.rows, scrollback: 1000, allowProposedApi: true }); this.serializer = new SerializeAddon(); @@ -101,13 +105,15 @@ export class SessionHost { }); this.pty.onExit(({ exitCode }) => { + // The child is gone; drop the handle so nothing can write to it. + this.pty = null; this.cleanup(); this.onExitCb?.(exitCode); }); // Poll output-activity; report only on a running↔idle flip. this.activityTimer = setInterval(() => { - const active = Date.now() - this.lastOutputAt < SessionHost.IDLE_MS; + const active = this.lastOutputAt !== null && Date.now() - this.lastOutputAt < SessionHost.IDLE_MS; if (active !== this.activeState) { this.activeState = active; this.onActivityCb?.(active); } }, 1000); this.activityTimer.unref?.(); @@ -141,7 +147,16 @@ export class SessionHost { async stop(): Promise { this.cleanup(); - try { this.pty?.kill(); } catch { /* already dead */ } + this.disposePty(); + } + + /** Kill the child and drop the handle. Every write/resize site uses `this.pty?.`, + * so clearing it makes late input a no-op instead of a write to a dead pipe. */ + private disposePty(): void { + const pty = this.pty; + this.pty = null; + if (!pty) return; + try { pty.kill(); } catch { /* already dead */ } } private onLocalInput = (chunk: Buffer): void => { diff --git a/src/kernel/pty/win-conin-guard.ts b/src/kernel/pty/win-conin-guard.ts new file mode 100644 index 00000000..3eb9c100 --- /dev/null +++ b/src/kernel/pty/win-conin-guard.ts @@ -0,0 +1,31 @@ +// +// Containment for an upstream node-pty defect on Windows. +// +// node-pty opens two pipes to the ConPTY. The read side (conout) gets an +// 'error' listener in lib/windowsTerminal.js:90; the write side (conin) is +// constructed in lib/windowsPtyAgent.js:79-84 with none. A net.Socket without +// an 'error' listener turns any emitted error into an uncaught exception, so a +// failing pty write takes the whole daemon down. Two triggers reach it: EAGAIN +// when conin's buffer fills under backpressure, and a write racing kill()'s +// _inSocket.destroy() (lib/windowsPtyAgent.js:176). +// +// Reaching through the private _agent is deliberate — Terminal.on() forwards to +// the conout socket, so there is no public route to conin. Verified against +// node-pty@1.2.0-beta.14. Reported upstream as +// https://github.com/microsoft/node-pty/issues/942 — delete this once a release +// carries the listener; re-check the internal shape on every node-pty bump. + +import type { IPty } from 'node-pty'; + +interface ErrorEmitter { on(event: 'error', listener: (err: Error) => void): unknown } +interface WindowsPtyInternals { _agent?: { inSocket?: ErrorEmitter } } + +/** Returns whether a listener was attached (false on non-win32, or if the shape is gone). */ +export function guardWindowsConinSocket(pty: IPty, platform: string = process.platform): boolean { + if (platform !== 'win32') return false; + const sock = (pty as unknown as WindowsPtyInternals)._agent?.inSocket; + if (!sock || typeof sock.on !== 'function') return false; + // A failed write is not fatal: the pty's real lifecycle is driven by onExit. + sock.on('error', () => { /* swallowed — see the note above */ }); + return true; +} diff --git a/src/kernel/web/__tests__/pty-bridge.test.ts b/src/kernel/web/__tests__/pty-bridge.test.ts index 0d5cbdc8..97e13dce 100644 --- a/src/kernel/web/__tests__/pty-bridge.test.ts +++ b/src/kernel/web/__tests__/pty-bridge.test.ts @@ -5,6 +5,7 @@ import { mkdtempSync } from 'node:fs'; import { join, basename } from 'node:path'; import { tmpdir } from 'node:os'; import * as net from 'node:net'; +import { until } from '../../__tests__/wait.js'; // Per-session socket: fs path on POSIX, named pipe on win32 (no unix sockets). const sessSock = (dir: string, name: string): string => @@ -35,9 +36,11 @@ describe('PtyBridge', () => { const ws = new FakeWs(); const b = bridge(ws as never, sockPath); - // give the bridge's net.connect a moment, then drive input through the ws - await new Promise((r) => setTimeout(r, 100)); + // Writes before connect are queued by Node, so the attach can go straight + // out. The host answers an Attach with a Size frame — wait for that instead + // of guessing at a connect delay, then send the payload. ws.emit('message', encodeAttach(80, 24)); + await until(() => { expect(ws.sent.length).toBeGreaterThan(0); }); ws.emit('message', encodeData(Buffer.from('ping\n'))); await new Promise((resolve, reject) => {