From 0a9281976f4febc6eb5577445ae13cd754cde78f Mon Sep 17 00:00:00 2001 From: slgoodrich Date: Sun, 3 May 2026 14:55:41 -0400 Subject: [PATCH 1/3] fix: capture killed/aborted on record; remove internal export leakage #129 Capture `killed` separately from `signal`. Replay synth previously derived `killed` from `signal !== null`, conflating "subprocess.kill() was called" with "process was terminated by any signal". An external SIGTERM has signal !== null but killed === false on the real result; replay reported killed: true. Both adapters now store `r.killed === true` on capture; serialize emits the field when defined; synth uses `rec.result.killed ?? signal !== null` so cassettes recorded before this change auto-upgrade. #126 tinyexec record path: snapshot `aborted`/`killed` from the ExecProcess before await. tinyexec's Result is `PromiseLike & OutputApi`; awaiting drops the OutputApi getters, so the previous adapter recorded aborted: false / killed: false on every tinyexec call regardless of outcome. The adapter's realCall now grabs the ExecProcess, awaits it, then enriches the resolved Output with proc.aborted / proc.killed before captureResult runs. #102 Skip the inputFile double-read on record. RunnerHooks.realCall gains a fourth parameter `resolvedStdin` (Call.stdin when buildCall ran, undefined otherwise). The execa adapter swaps `options.inputFile` for `input: resolvedStdin` when both are set, so real execa does not re-read a file buildCall already consumed. #128 Move captureResult into non-exported modules. The `_captureResultForTesting` re-exports leaked into the published `.d.ts` for shell-cassette/execa and shell-cassette/tinyexec; each adapter now imports captureResult from a sibling (src/execa-capture.ts, src/tinyexec-capture.ts) that is not in package.json's exports map. Tests import the same path. Schema stays additive: cassettes without the `killed` field load and replay correctly via the synth fallback. RunnerHooks lives in src/wrapper.ts, which is not in the package exports map, so the signature change is internal. --- src/execa-capture.ts | 45 ++++++ src/execa.ts | 70 ++++----- src/serialize.ts | 1 + src/tinyexec-capture.ts | 43 ++++++ src/tinyexec.ts | 74 ++++----- src/types.ts | 9 ++ src/wrapper.ts | 23 ++- .../integration/record-replay-aborted.test.ts | 41 ++++- tests/integration/wrapper.test.ts | 79 ++++++++++ tests/unit/execa-capture-result.test.ts | 42 ++++- tests/unit/execa-input-file-swap.test.ts | 80 ++++++++++ tests/unit/tinyexec-adapter.test.ts | 32 ++-- tests/unit/tinyexec-capture-result.test.ts | 30 +++- tests/unit/wrapper-resolved-stdin.test.ts | 145 ++++++++++++++++++ 14 files changed, 589 insertions(+), 125 deletions(-) create mode 100644 src/execa-capture.ts create mode 100644 src/tinyexec-capture.ts create mode 100644 tests/unit/execa-input-file-swap.test.ts create mode 100644 tests/unit/wrapper-resolved-stdin.test.ts diff --git a/src/execa-capture.ts b/src/execa-capture.ts new file mode 100644 index 0000000..42162ce --- /dev/null +++ b/src/execa-capture.ts @@ -0,0 +1,45 @@ +import type { Result } from './types.js' + +/** + * Internal: maps an execa raw result (or thrown error shape) to the cassette + * `Result` type. Lives in a non-exported module so it stays out of the + * `shell-cassette/execa` public sub-path's `.d.ts` and IDE autocomplete. + * Tests import this module directly via `src/execa-capture.js`. + */ +export function captureResult(raw: unknown, durationMs: number): Result { + const r = raw as { + stdout?: string | string[] + stderr?: string | string[] + all?: string | string[] + exitCode?: number + signal?: string | null + isCanceled?: boolean + killed?: boolean + failed?: boolean + timedOut?: boolean + isMaxBuffer?: boolean + isForcefullyTerminated?: boolean + isGracefullyCanceled?: boolean + } + return { + stdoutLines: toLines(r.stdout), + stderrLines: toLines(r.stderr), + allLines: r.all === undefined ? null : toLines(r.all), + exitCode: r.exitCode ?? 0, + signal: r.signal ?? null, + durationMs, + aborted: r.isCanceled === true, + killed: r.killed === true, + failed: r.failed === true, + timedOut: r.timedOut === true, + isMaxBuffer: r.isMaxBuffer === true, + isForcefullyTerminated: r.isForcefullyTerminated === true, + isGracefullyCanceled: r.isGracefullyCanceled === true, + } +} + +function toLines(input: string | string[] | undefined): string[] { + if (input === undefined) return [''] + if (Array.isArray(input)) return [...input, ''] + return input.split('\n') +} diff --git a/src/execa.ts b/src/execa.ts index a9b1c79..378f25d 100644 --- a/src/execa.ts +++ b/src/execa.ts @@ -1,8 +1,9 @@ import type { Options, ResultPromise } from 'execa' import { MissingPeerDependencyError, UnsupportedOptionError } from './errors.js' +import { captureResult } from './execa-capture.js' import { readInputFile } from './io.js' import { validateOptions } from './options-execa.js' -import type { Call, Recording, Result } from './types.js' +import type { Call, Recording } from './types.js' import { type RunnerHooks, runWrapped } from './wrapper.js' // Reused across every synth call. `pipe` and async iteration always @@ -69,11 +70,32 @@ export function execaNode( const execaHooks: RunnerHooks = { validate: (opts) => validateOptions(opts as Record | undefined), buildCall, - realCall: (file, args, options) => realExeca(file, args, options) as unknown as Promise, + realCall, captureResult, synthesize, } +// When buildCall already resolved `inputFile` into `Call.stdin`, swap the +// option so real execa consumes the resolved string via `input` instead of +// re-reading the file. The branch only fires on the record path +// (passthrough/no-session pass `resolvedStdin: undefined`). buildCall's +// contract: when inputFile is defined, stdin is always a string (possibly +// empty), never null — so a string-typeof check covers the optimization +// case without a redundant null branch. Closes #102. +function realCall( + file: string, + args: readonly string[], + options: Options, + resolvedStdin: string | null | undefined, +): Promise { + let opts = options + if (typeof resolvedStdin === 'string' && options.inputFile !== undefined) { + const { inputFile: _, ...rest } = options as Options & { inputFile?: unknown } + opts = { ...rest, input: resolvedStdin } as Options + } + return realExeca(file, args, opts) as unknown as Promise +} + async function buildCall(file: string, args: readonly string[], options: Options): Promise { // Validator already rejected the invalid shapes (Uint8Array/Readable input, // input+inputFile conflict). At this point `input` is either undefined, @@ -96,46 +118,6 @@ async function buildCall(file: string, args: readonly string[], options: Options } } -function captureResult(raw: unknown, durationMs: number): Result { - const r = raw as { - stdout?: string | string[] - stderr?: string | string[] - all?: string | string[] - exitCode?: number - signal?: string | null - isCanceled?: boolean - failed?: boolean - timedOut?: boolean - isMaxBuffer?: boolean - isForcefullyTerminated?: boolean - isGracefullyCanceled?: boolean - } - return { - stdoutLines: toLines(r.stdout), - stderrLines: toLines(r.stderr), - allLines: r.all === undefined ? null : toLines(r.all), - exitCode: r.exitCode ?? 0, - signal: r.signal ?? null, - durationMs, - aborted: r.isCanceled === true, - failed: r.failed === true, - timedOut: r.timedOut === true, - isMaxBuffer: r.isMaxBuffer === true, - isForcefullyTerminated: r.isForcefullyTerminated === true, - isGracefullyCanceled: r.isGracefullyCanceled === true, - } -} - -// Test-only export so unit tests can drive captureResult without spawning -// a real subprocess. Underscore prefix marks it as non-public API. -export const _captureResultForTesting = captureResult - -function toLines(input: string | string[] | undefined): string[] { - if (input === undefined) return [''] - if (Array.isArray(input)) return [...input, ''] - return input.split('\n') -} - // Resolve execa's `lines` option to per-stream booleans. The object form lets // users set `lines` independently per fd, e.g. `{ stdout: true, stderr: false }`. // Precedence matches execa's own resolution (see node_modules/execa/types/ @@ -211,7 +193,9 @@ function synthesize(rec: Recording, options: Options): unknown { isTerminated, isForcefullyTerminated: rec.result.isForcefullyTerminated ?? false, isGracefullyCanceled: rec.result.isGracefullyCanceled ?? false, - killed: isTerminated, // NOTE: tracked as semantic bug in #129 — preserves current behavior + // Stored value when present; fall back to isTerminated for legacy + // cassettes recorded before `killed` was captured separately. + killed: rec.result.killed ?? isTerminated, // pipedFrom: no chained subprocess on replay. ipcOutput: ipc: true // is rejected at validation. diff --git a/src/serialize.ts b/src/serialize.ts index b5122c1..3300549 100644 --- a/src/serialize.ts +++ b/src/serialize.ts @@ -64,6 +64,7 @@ function orderRecording(rec: Recording) { ...(rec.result.isGracefullyCanceled !== undefined && { isGracefullyCanceled: rec.result.isGracefullyCanceled, }), + ...(rec.result.killed !== undefined && { killed: rec.result.killed }), }, _redactions: rec.redactions, ...(rec.suppressed.length > 0 ? { _suppressed: rec.suppressed } : {}), diff --git a/src/tinyexec-capture.ts b/src/tinyexec-capture.ts new file mode 100644 index 0000000..d46e52c --- /dev/null +++ b/src/tinyexec-capture.ts @@ -0,0 +1,43 @@ +import type { Result } from './types.js' + +/** + * Internal: maps a tinyexec raw result (with the OutputApi `aborted`/`killed` + * snapshot the wrapper takes pre-await) to the cassette `Result` type. Lives + * in a non-exported module so it stays out of the `shell-cassette/tinyexec` + * public sub-path's `.d.ts` and IDE autocomplete. Tests import this module + * directly via `src/tinyexec-capture.js`. + * + * tinyexec exposes only `killed: boolean` (not the actual signal name); we + * unconditionally record `signal: 'SIGTERM'` when killed. The real signal + * name is not recoverable from tinyexec's surface. + */ +export function captureResult(raw: unknown, durationMs: number): Result { + const r = raw as { + stdout?: string + stderr?: string + exitCode?: number + killed?: boolean + aborted?: boolean + } + // tinyexec.stdout/stderr is always a string per its type contract; the [''] fallback + // exists only for defensive narrowing on `unknown` raw input (e.g., a thrown error + // shape that happens to lack stdout/stderr fields). + const exitCode = r.exitCode ?? 0 + const killed = r.killed === true + const aborted = r.aborted === true + return { + stdoutLines: typeof r.stdout === 'string' ? r.stdout.split('\n') : [''], + stderrLines: typeof r.stderr === 'string' ? r.stderr.split('\n') : [''], + allLines: null, + exitCode, + signal: killed ? 'SIGTERM' : null, + durationMs, + aborted, + killed, + // Derived because tinyexec does not expose a `failed` boolean. Covers + // the three known failure shapes (non-zero exit, signal kill, abort). + // timedOut and isMaxBuffer are intentionally not stored: tinyexec + // exposes neither; synth defaults each to false on replay. + failed: exitCode !== 0 || killed || aborted, + } +} diff --git a/src/tinyexec.ts b/src/tinyexec.ts index 329bcda..dc075b7 100644 --- a/src/tinyexec.ts +++ b/src/tinyexec.ts @@ -1,7 +1,8 @@ import type { Options, Result as TinyResult } from 'tinyexec' import { MissingPeerDependencyError, ShellCassetteError, UnsupportedOptionError } from './errors.js' import { validateOptions } from './options-tinyexec.js' -import type { Call, Result as CassetteResult, Recording } from './types.js' +import { captureResult } from './tinyexec-capture.js' +import type { Call, Recording } from './types.js' import { type RunnerHooks, runWrapped } from './wrapper.js' export type { Options, Result } from 'tinyexec' @@ -37,14 +38,36 @@ export function x( const tinyexecHooks: RunnerHooks, TinyResult> = { validate: (opts) => validateOptions(opts as Record | undefined), buildCall, - // tinyexec's Result is structurally a PromiseLike & OutputApi, not Promise; - // double-cast through unknown is needed to satisfy the hook's Promise signature - realCall: (file, args, options) => - realX(file, [...args], options) as unknown as Promise, + realCall, captureResult, synthesize, } +// tinyexec's Result is `PromiseLike & OutputApi`. Awaiting it resolves +// to Output (`{ stdout, stderr, exitCode }`) and drops the OutputApi getters +// (`aborted`, `killed`). To capture those on the record path we snapshot them +// from the ExecProcess BEFORE the await resolves, then return an enriched +// plain object the cassette captureResult can read. Fixes #126. +// +// `_resolvedStdin` is part of RunnerHooks.realCall's signature for the execa +// adapter's #102 optimization; tinyexec has no `inputFile` option and ignores +// it. +async function realCall( + file: string, + args: readonly string[], + options: Partial, + _resolvedStdin: string | null | undefined, +): Promise { + const proc = realX(file, [...args], options) + const output = await proc + const enriched = { ...output, aborted: proc.aborted, killed: proc.killed } + // Cast: the enriched plain object lacks tinyexec's full Result shape (no + // OutputApi methods, no PromiseLike). The wrapper passes this to + // captureResult and (on record path) returns it to the user. Live ProcessApi + // calls post-await are a known replay limitation, see synthesize() comment. + return enriched as unknown as TinyResult +} + async function buildCall( file: string, args: readonly string[], @@ -61,47 +84,12 @@ async function buildCall( } } -function captureResult(raw: unknown, durationMs: number): CassetteResult { - const r = raw as { - stdout?: string - stderr?: string - exitCode?: number - killed?: boolean - aborted?: boolean - } - // tinyexec.stdout/stderr is always a string per its type contract; the [''] fallback - // exists only for defensive narrowing on `unknown` raw input (e.g., a thrown error - // shape that happens to lack stdout/stderr fields). - // - // tinyexec exposes only `killed: boolean`, not the actual signal name (SIGINT, - // SIGKILL, etc.). We unconditionally record SIGTERM on kill; the real signal is - // lost. Known limitation; tinyexec does not expose the signal name. - const exitCode = r.exitCode ?? 0 - const killed = r.killed === true - const aborted = r.aborted === true - return { - stdoutLines: typeof r.stdout === 'string' ? r.stdout.split('\n') : [''], - stderrLines: typeof r.stderr === 'string' ? r.stderr.split('\n') : [''], - allLines: null, - exitCode, - signal: killed ? 'SIGTERM' : null, - durationMs, - aborted, - // Derived because tinyexec does not expose a `failed` boolean. Covers - // the three known failure shapes (non-zero exit, signal kill, abort). - // timedOut and isMaxBuffer are intentionally not stored: tinyexec - // exposes neither; synth defaults each to false on replay. - failed: exitCode !== 0 || killed || aborted, - } -} - -// Test-only export. See execa.ts for the same pattern. -export const _captureResultForTesting = captureResult - function synthesize(rec: Recording, options: Partial): TinyResult { const stdout = rec.result.stdoutLines.join('\n') const stderr = rec.result.stderrLines.join('\n') - const killed = rec.result.signal !== null + // Stored value when present; fall back to signal-derived for legacy cassettes + // recorded before `killed` was captured separately. Closes #129. + const killed = rec.result.killed ?? rec.result.signal !== null const result = { stdout, diff --git a/src/types.ts b/src/types.ts index 413c955..4890172 100644 --- a/src/types.ts +++ b/src/types.ts @@ -31,6 +31,15 @@ export type Result = { isMaxBuffer?: boolean isForcefullyTerminated?: boolean isGracefullyCanceled?: boolean + /** + * True when `subprocess.kill()` was called (execa's `r.killed`, tinyexec's + * `proc.killed`). Distinct from `signal !== null`: a process terminated by + * an external signal has `signal !== null` but `killed === false`. Stored + * separately so synth doesn't have to derive (and get it wrong, as #129 + * documented). Optional for backward compat with cassettes recorded before + * the field existed. + */ + killed?: boolean } export type Recording = { diff --git a/src/wrapper.ts b/src/wrapper.ts index 8848ff5..8f80273 100644 --- a/src/wrapper.ts +++ b/src/wrapper.ts @@ -21,7 +21,22 @@ CI=true forces replay mode by default; without a session shell-cassette refuses export type RunnerHooks = { validate: (options: Opts | undefined) => void buildCall: (file: string, args: readonly string[], options: Opts) => Promise - realCall: (file: string, args: readonly string[], options: Opts) => Promise + /** + * Invokes the real runner. `resolvedStdin` carries `call.stdin` when + * `buildCall` already ran (record path); it is `undefined` on the + * passthrough/no-session paths where `buildCall` is skipped. + * + * Adapters use it to avoid duplicating side effects buildCall already + * performed. The execa adapter swaps `options.inputFile` for + * `input: resolvedStdin` when both are present, so real execa does not + * re-read the file (#102). + */ + realCall: ( + file: string, + args: readonly string[], + options: Opts, + resolvedStdin: string | null | undefined, + ) => Promise // The wrapper measures elapsed time around realCall and passes it here. // Adapters use this value rather than reading runner-provided fields so // the measurement is uniform across runners (execa exposes durationMs; @@ -53,7 +68,7 @@ export async function runWrapped( if (mode === 'replay') { throw new NoActiveSessionError(NO_ACTIVE_SESSION_HELP) } - return hooks.realCall(file, args, options) + return hooks.realCall(file, args, options, undefined) } const loaded = await ensureSessionLoaded(session) @@ -65,7 +80,7 @@ export async function runWrapped( ) if (mode === 'passthrough') { - return hooks.realCall(file, args, options) + return hooks.realCall(file, args, options, undefined) } const call = await hooks.buildCall(file, args, options) @@ -111,7 +126,7 @@ export async function runWrapped( } const start = performance.now() try { - const result = await hooks.realCall(file, args, options) + const result = await hooks.realCall(file, args, options, call.stdin) captureAndRecord(call, result, performance.now() - start, hooks, loaded) return result } catch (err) { diff --git a/tests/integration/record-replay-aborted.test.ts b/tests/integration/record-replay-aborted.test.ts index b246804..2c36874 100644 --- a/tests/integration/record-replay-aborted.test.ts +++ b/tests/integration/record-replay-aborted.test.ts @@ -2,6 +2,7 @@ import { readFile } from 'node:fs/promises' import path from 'node:path' import { describe, expect, test } from 'vitest' import { execa } from '../../src/execa.js' +import { x } from '../../src/tinyexec.js' import { useCassette } from '../../src/use-cassette.js' import { restoreEnv } from '../helpers/env.js' import { useRecordingEnv } from '../helpers/recording-env.js' @@ -10,12 +11,6 @@ import { useTmpDir } from '../helpers/tmp-dir.js' useRecordingEnv() -// Tinyexec coverage for aborted-call record/replay is tracked in #126. -// tinyexec's awaited Output drops the OutputApi getters (aborted, killed) -// that live on the pre-await ExecProcess, so the wrapper currently records -// `aborted: false` even on cancelled tinyexec calls. Replay synth handles -// the cassette field correctly; the gap is purely on the record path. - describe('record + replay: aborted (execa)', () => { const tmp = useTmpDir('sc-aborted-execa-') @@ -53,3 +48,37 @@ describe('record + replay: aborted (execa)', () => { } }) }) + +// Closes #126: tinyexec's awaited Output drops the OutputApi getters +// (aborted, killed) that live on the pre-await ExecProcess. The adapter's +// realCall snapshots them before await so captureResult sees real values. +describe('record + replay: aborted (tinyexec)', () => { + const tmp = useTmpDir('sc-aborted-tinyexec-') + + test('signal abort records aborted=true; replay surfaces aborted=true', async () => { + const cp = path.join(tmp.ref(), 'cassette.json') + + await useCassette(cp, async () => { + const ac = new AbortController() + setTimeout(() => ac.abort(), 100) + const r = await x('node', [...SLEEP_5S], { signal: ac.signal }) + expect(r.aborted).toBe(true) + }) + + const cassette = JSON.parse(await readFile(cp, 'utf8')) + expect(cassette.recordings[0].result.aborted).toBe(true) + expect(cassette.recordings[0].result.failed).toBe(true) + + const prevMode = process.env.SHELL_CASSETTE_MODE + process.env.SHELL_CASSETTE_MODE = 'replay' + try { + await useCassette(cp, async () => { + const ac = new AbortController() + const r = await x('node', [...SLEEP_5S], { signal: ac.signal }) + expect(r.aborted).toBe(true) + }) + } finally { + restoreEnv('SHELL_CASSETTE_MODE', prevMode) + } + }) +}) diff --git a/tests/integration/wrapper.test.ts b/tests/integration/wrapper.test.ts index 0f23120..bcff8f4 100644 --- a/tests/integration/wrapper.test.ts +++ b/tests/integration/wrapper.test.ts @@ -98,6 +98,85 @@ describe('wrapped execa', () => { } }) + test('synth: legacy cassette without killed field falls back to signal-derived (#129)', async () => { + const tmp = await mkdtemp(path.join(tmpdir(), 'sc-test-')) + try { + const cassettePath = path.join(tmp, 'cassette.json') + // Hand-crafted: signal is set, killed is absent. Pre-#129 behavior + // (signal-derived) is the only source of truth, so synth must report + // killed: true to keep legacy cassettes replaying as before. + const legacy = { + version: 2, + recordings: [ + { + call: { command: 'node', args: ['-v'], cwd: null, env: {}, stdin: null }, + result: { + stdoutLines: [''], + stderrLines: [''], + allLines: null, + exitCode: 0, + signal: 'SIGTERM', + durationMs: 1, + aborted: false, + }, + }, + ], + } + await writeFile(cassettePath, JSON.stringify(legacy), 'utf8') + + const session = sessionAt(cassettePath) + setActiveCassette(session) + const replayed = (await wrappedExeca('node', ['-v'], { reject: false })) as { + killed: boolean + signal: string | null + } + expect(replayed.killed).toBe(true) + expect(replayed.signal).toBe('SIGTERM') + } finally { + await rm(tmp, { recursive: true, force: true }) + } + }) + + test('synth: new cassette with killed:false, signal:SIGTERM reports killed:false (#129)', async () => { + const tmp = await mkdtemp(path.join(tmpdir(), 'sc-test-')) + try { + const cassettePath = path.join(tmp, 'cassette.json') + // External SIGTERM (not via subprocess.kill()): execa would surface + // signal !== null but killed === false. Pre-#129 synth conflated the + // two and reported killed:true; the fix preserves the distinction. + const cassette = { + version: 2, + recordings: [ + { + call: { command: 'node', args: ['-v'], cwd: null, env: {}, stdin: null }, + result: { + stdoutLines: [''], + stderrLines: [''], + allLines: null, + exitCode: 0, + signal: 'SIGTERM', + durationMs: 1, + aborted: false, + killed: false, + }, + }, + ], + } + await writeFile(cassettePath, JSON.stringify(cassette), 'utf8') + + const session = sessionAt(cassettePath) + setActiveCassette(session) + const replayed = (await wrappedExeca('node', ['-v'], { reject: false })) as { + killed: boolean + signal: string | null + } + expect(replayed.killed).toBe(false) + expect(replayed.signal).toBe('SIGTERM') + } finally { + await rm(tmp, { recursive: true, force: true }) + } + }) + test('legacy cassette (no allLines) still replays all via stdout+stderr concat', async () => { const tmp = await mkdtemp(path.join(tmpdir(), 'sc-test-')) try { diff --git a/tests/unit/execa-capture-result.test.ts b/tests/unit/execa-capture-result.test.ts index 8d9ad24..25c4216 100644 --- a/tests/unit/execa-capture-result.test.ts +++ b/tests/unit/execa-capture-result.test.ts @@ -1,13 +1,13 @@ import { describe, expect, test } from 'vitest' -// captureResult is internal; expose a thin re-export by importing the -// adapter and using a known shape. Pure-function unit test bypasses -// the wrapper layer entirely. -import { _captureResultForTesting } from '../../src/execa.js' +// captureResult is internal — its module is intentionally not in +// package.json's exports map, so imports go through the source path +// directly. Pure-function unit test bypasses the wrapper layer entirely. +import { captureResult } from '../../src/execa-capture.js' describe('execa captureResult: optional flag fields', () => { test('reads all five flags when present and true', () => { - const r = _captureResultForTesting( + const r = captureResult( { stdout: 'ok', stderr: '', @@ -31,7 +31,7 @@ describe('execa captureResult: optional flag fields', () => { }) test('absent flags default to false (not undefined)', () => { - const r = _captureResultForTesting({ stdout: '', stderr: '', exitCode: 0 }, 1) + const r = captureResult({ stdout: '', stderr: '', exitCode: 0 }, 1) expect(r.failed).toBe(false) expect(r.timedOut).toBe(false) expect(r.isMaxBuffer).toBe(false) @@ -42,7 +42,7 @@ describe('execa captureResult: optional flag fields', () => { test('non-true truthy values (e.g. truthy strings) coerce to false', () => { // execa types these as boolean. The === true idiom guards against any // accidental string-leaks from real execa shape changes. - const r = _captureResultForTesting( + const r = captureResult( { stdout: '', stderr: '', @@ -56,3 +56,31 @@ describe('execa captureResult: optional flag fields', () => { expect(r.timedOut).toBe(false) }) }) + +describe('execa captureResult: killed (#129)', () => { + test('killed=true on raw records killed=true', () => { + const r = captureResult( + { stdout: '', stderr: '', exitCode: 0, signal: 'SIGTERM', killed: true }, + 1, + ) + expect(r.killed).toBe(true) + expect(r.signal).toBe('SIGTERM') + }) + + test('signal !== null without killed records killed=false (external signal)', () => { + // External SIGTERM (not via subprocess.kill()) — execa exposes + // signal !== null but killed === false. The capture must preserve + // that distinction so synth does not report killed=true. + const r = captureResult( + { stdout: '', stderr: '', exitCode: 0, signal: 'SIGTERM', killed: false }, + 1, + ) + expect(r.killed).toBe(false) + expect(r.signal).toBe('SIGTERM') + }) + + test('absent killed defaults to false', () => { + const r = captureResult({ stdout: '', stderr: '', exitCode: 0 }, 1) + expect(r.killed).toBe(false) + }) +}) diff --git a/tests/unit/execa-input-file-swap.test.ts b/tests/unit/execa-input-file-swap.test.ts new file mode 100644 index 0000000..24efd05 --- /dev/null +++ b/tests/unit/execa-input-file-swap.test.ts @@ -0,0 +1,80 @@ +import { writeFile } from 'node:fs/promises' +import path from 'node:path' +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' + +vi.mock('execa', () => ({ execa: vi.fn() })) + +const { execa: realExecaMock } = await import('execa') +const { execa: wrappedExeca } = await import('../../src/execa.js') +const { _resetForTesting, clearActiveCassette, setActiveCassette } = await import( + '../../src/state.js' +) +const { restoreEnv } = await import('../helpers/env.js') +const { makeSession } = await import('../helpers/session.js') +const { useTmpDir } = await import('../helpers/tmp-dir.js') + +const originalAck = process.env.SHELL_CASSETTE_ACK_REDACTION + +describe('execa adapter: realCall swaps inputFile -> input on record path (#102)', () => { + const tmp = useTmpDir('sc-input-swap-') + + beforeEach(() => { + _resetForTesting() + process.env.SHELL_CASSETTE_ACK_REDACTION = 'true' + delete process.env.SHELL_CASSETTE_MODE + delete process.env.CI + vi.mocked(realExecaMock).mockReset() + // Resolve to a minimal execa-shaped result so captureResult is happy. + vi.mocked(realExecaMock).mockResolvedValue({ + stdout: '', + stderr: '', + exitCode: 0, + signal: null, + isCanceled: false, + failed: false, + } as never) + }) + + afterEach(() => { + _resetForTesting() + clearActiveCassette() + restoreEnv('SHELL_CASSETTE_ACK_REDACTION', originalAck) + }) + + test('record path: real execa receives `input: `, no `inputFile`', async () => { + const fixture = path.join(tmp.ref(), 'in.txt') + await writeFile(fixture, 'hello-from-file', 'utf8') + + const session = makeSession({ + name: 't', + path: path.join(tmp.ref(), 'cassette.json'), + loadedFile: null, + matcher: null, + }) + setActiveCassette(session) + + await wrappedExeca('node', ['-v'], { inputFile: fixture }) + + expect(realExecaMock).toHaveBeenCalledTimes(1) + const passedOptions = vi.mocked(realExecaMock).mock.calls[0]?.[2] as Record + expect(passedOptions.input).toBe('hello-from-file') + expect(passedOptions.inputFile).toBeUndefined() + }) + + test('record path with explicit `input` string: forwarded as-is, no swap needed', async () => { + const session = makeSession({ + name: 't', + path: path.join(tmp.ref(), 'cassette.json'), + loadedFile: null, + matcher: null, + }) + setActiveCassette(session) + + await wrappedExeca('node', ['-v'], { input: 'literal' }) + + expect(realExecaMock).toHaveBeenCalledTimes(1) + const passedOptions = vi.mocked(realExecaMock).mock.calls[0]?.[2] as Record + expect(passedOptions.input).toBe('literal') + expect(passedOptions.inputFile).toBeUndefined() + }) +}) diff --git a/tests/unit/tinyexec-adapter.test.ts b/tests/unit/tinyexec-adapter.test.ts index b20ed3a..5319935 100644 --- a/tests/unit/tinyexec-adapter.test.ts +++ b/tests/unit/tinyexec-adapter.test.ts @@ -102,14 +102,16 @@ describe('tinyexec adapter', () => { const session = makeSession({ loadedFile: null }) setActiveCassette(session) - vi.mocked(realXMock).mockResolvedValueOnce({ - stdout: '', - stderr: '', - exitCode: 1, - pid: 1, - aborted: true, - killed: true, - } as never) + // Real tinyexec exposes aborted/killed as OutputApi getters on the + // pre-await ExecProcess, not on the resolved Output. The wrapper + // snapshots them via `proc.aborted` / `proc.killed` before await + // resolves (#126). Mock that shape: a Promise carrying the OutputApi + // fields as own properties. + const fakeProc = Object.assign( + Promise.resolve({ stdout: '', stderr: '', exitCode: 1, pid: 1 }), + { aborted: true, killed: true }, + ) + vi.mocked(realXMock).mockReturnValueOnce(fakeProc as never) await x('sleep', ['10']) expect(session.newRecordings[0]?.result.aborted).toBe(true) @@ -139,14 +141,12 @@ describe('tinyexec adapter', () => { const session = makeSession({ loadedFile: null }) setActiveCassette(session) - vi.mocked(realXMock).mockResolvedValueOnce({ - stdout: '', - stderr: '', - exitCode: 143, - pid: 1, - aborted: false, - killed: true, - } as never) + // Same pre-await snapshot contract as the aborted test above. + const fakeProc = Object.assign( + Promise.resolve({ stdout: '', stderr: '', exitCode: 143, pid: 1 }), + { aborted: false, killed: true }, + ) + vi.mocked(realXMock).mockReturnValueOnce(fakeProc as never) await x('sleep', ['10']) expect(session.newRecordings[0]?.result.signal).toBe('SIGTERM') diff --git a/tests/unit/tinyexec-capture-result.test.ts b/tests/unit/tinyexec-capture-result.test.ts index e7c31c2..0d9ab27 100644 --- a/tests/unit/tinyexec-capture-result.test.ts +++ b/tests/unit/tinyexec-capture-result.test.ts @@ -1,10 +1,10 @@ import { describe, expect, test } from 'vitest' -import { _captureResultForTesting } from '../../src/tinyexec.js' +import { captureResult } from '../../src/tinyexec-capture.js' describe('tinyexec captureResult: failed derivation', () => { test('plain success (exitCode 0, no kill, no abort): failed false', () => { - const r = _captureResultForTesting( + const r = captureResult( { stdout: 'ok', stderr: '', exitCode: 0, killed: false, aborted: false }, 1, ) @@ -12,7 +12,7 @@ describe('tinyexec captureResult: failed derivation', () => { }) test('non-zero exit: failed true', () => { - const r = _captureResultForTesting( + const r = captureResult( { stdout: '', stderr: 'oops', exitCode: 1, killed: false, aborted: false }, 1, ) @@ -20,7 +20,7 @@ describe('tinyexec captureResult: failed derivation', () => { }) test('killed: failed true (signal kill collapses to SIGTERM in this adapter)', () => { - const r = _captureResultForTesting( + const r = captureResult( { stdout: '', stderr: '', exitCode: 0, killed: true, aborted: false }, 1, ) @@ -29,7 +29,7 @@ describe('tinyexec captureResult: failed derivation', () => { }) test('aborted: failed true', () => { - const r = _captureResultForTesting( + const r = captureResult( { stdout: '', stderr: '', exitCode: 0, killed: false, aborted: true }, 1, ) @@ -38,11 +38,29 @@ describe('tinyexec captureResult: failed derivation', () => { }) test('timedOut and isMaxBuffer not stored (tinyexec does not expose)', () => { - const r = _captureResultForTesting( + const r = captureResult( { stdout: '', stderr: '', exitCode: 0, killed: false, aborted: false }, 1, ) as Record expect(r.timedOut).toBeUndefined() expect(r.isMaxBuffer).toBeUndefined() }) + + test('killed=true on raw records killed=true (#129)', () => { + const r = captureResult( + { stdout: '', stderr: '', exitCode: 0, killed: true, aborted: false }, + 1, + ) + expect(r.killed).toBe(true) + expect(r.signal).toBe('SIGTERM') + }) + + test('killed=false records killed=false even when signal would be derived elsewhere', () => { + const r = captureResult( + { stdout: '', stderr: '', exitCode: 0, killed: false, aborted: false }, + 1, + ) + expect(r.killed).toBe(false) + expect(r.signal).toBeNull() + }) }) diff --git a/tests/unit/wrapper-resolved-stdin.test.ts b/tests/unit/wrapper-resolved-stdin.test.ts new file mode 100644 index 0000000..a971d5e --- /dev/null +++ b/tests/unit/wrapper-resolved-stdin.test.ts @@ -0,0 +1,145 @@ +import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest' +import { _resetForTesting, clearActiveCassette, setActiveCassette } from '../../src/state.js' +import type { Recording, Result } from '../../src/types.js' +import { type RunnerHooks, runWrapped } from '../../src/wrapper.js' +import { restoreEnv } from '../helpers/env.js' +import { makeSession } from '../helpers/session.js' + +const originalMode = process.env.SHELL_CASSETTE_MODE +const originalAck = process.env.SHELL_CASSETTE_ACK_REDACTION + +type Opts = { input?: string; inputFile?: string } + +function makeHooks(): { + hooks: RunnerHooks + realCall: ReturnType +} { + const realCall = vi.fn( + async ( + _file: string, + _args: readonly string[], + _opts: Opts, + _resolvedStdin: string | null | undefined, + ) => ({ stdout: 'ok' }), + ) + const hooks: RunnerHooks = { + validate: () => undefined, + buildCall: async (file, args, opts) => ({ + command: file, + args: [...args], + cwd: null, + env: {}, + stdin: opts.input ?? (opts.inputFile !== undefined ? '' : null), + }), + realCall, + captureResult: (raw): Result => { + const r = raw as { stdout: string } + return { + stdoutLines: r.stdout.split('\n'), + stderrLines: [''], + allLines: null, + exitCode: 0, + signal: null, + durationMs: 0, + aborted: false, + } + }, + synthesize: (rec: Recording): { stdout: string } => ({ + stdout: rec.result.stdoutLines.join('\n'), + }), + } + return { hooks, realCall } +} + +describe('wrapper.realCall: resolvedStdin threading (#102)', () => { + beforeEach(() => { + _resetForTesting() + process.env.SHELL_CASSETTE_ACK_REDACTION = 'true' + delete process.env.SHELL_CASSETTE_MODE + delete process.env.CI + }) + + afterEach(() => { + _resetForTesting() + clearActiveCassette() + restoreEnv('SHELL_CASSETTE_MODE', originalMode) + restoreEnv('SHELL_CASSETTE_ACK_REDACTION', originalAck) + }) + + test('passes resolvedStdin = call.stdin on record path with `input`', async () => { + const session = makeSession({ + name: 't', + path: '/tmp/test.json', + loadedFile: null, + matcher: null, + }) + setActiveCassette(session) + + const { hooks, realCall } = makeHooks() + await runWrapped('node', ['-v'], { input: 'literal-stdin' } satisfies Opts, hooks) + + expect(realCall).toHaveBeenCalledTimes(1) + expect(realCall.mock.calls[0]?.[3]).toBe('literal-stdin') + }) + + test('passes resolvedStdin = call.stdin on record path with `inputFile` resolved by buildCall', async () => { + const session = makeSession({ + name: 't', + path: '/tmp/test.json', + loadedFile: null, + matcher: null, + }) + setActiveCassette(session) + + const { hooks, realCall } = makeHooks() + await runWrapped('node', ['-v'], { inputFile: '/some/path' } satisfies Opts, hooks) + + expect(realCall).toHaveBeenCalledTimes(1) + // makeHooks's buildCall returns '' as a stand-in for the + // resolved stdin; the wrapper must thread that through to realCall. + expect(realCall.mock.calls[0]?.[3]).toBe('') + }) + + test('passes resolvedStdin = null on record path when neither input nor inputFile is set', async () => { + const session = makeSession({ + name: 't', + path: '/tmp/test.json', + loadedFile: null, + matcher: null, + }) + setActiveCassette(session) + + const { hooks, realCall } = makeHooks() + await runWrapped('node', ['-v'], {} satisfies Opts, hooks) + + expect(realCall).toHaveBeenCalledTimes(1) + expect(realCall.mock.calls[0]?.[3]).toBeNull() + }) + + test('passes resolvedStdin = undefined on passthrough mode (buildCall skipped)', async () => { + const session = makeSession({ + name: 't', + path: '/tmp/test.json', + loadedFile: null, + matcher: null, + scopeDefault: 'passthrough', + }) + setActiveCassette(session) + + const { hooks, realCall } = makeHooks() + await runWrapped('node', ['-v'], { input: 'irrelevant' } satisfies Opts, hooks) + + expect(realCall).toHaveBeenCalledTimes(1) + expect(realCall.mock.calls[0]?.[3]).toBeUndefined() + }) + + test('passes resolvedStdin = undefined on no-active-session passthrough', async () => { + // No setActiveCassette() — wrapper falls through to realCall after the + // mode check (resolveMode defaults to passthrough when CI=false). + const { hooks, realCall } = makeHooks() + await runWrapped('node', ['-v'], { input: 'irrelevant' } satisfies Opts, hooks) + + expect(realCall).toHaveBeenCalledTimes(1) + expect(realCall.mock.calls[0]?.[3]).toBeUndefined() + }) +}) From 62703656087f1a7e92929503bac359f0826a5b8a Mon Sep 17 00:00:00 2001 From: slgoodrich Date: Sun, 3 May 2026 15:02:07 -0400 Subject: [PATCH 2/3] chore: drop process-leakage references from source comments Removes inline `Closes #N` / `Fixes #N` lines from comments in src/execa.ts, src/tinyexec.ts, src/types.ts, and the new tests/integration/record-replay-aborted.test.ts banner. Issue closures belong in commit messages and PR bodies, not source. Also rewords the killed-field JSDoc on Result to describe the semantic distinction directly instead of pointing at the issue that surfaced it, and replaces one em-dash in the new wrapper test with a colon. --- src/execa.ts | 4 ++-- src/tinyexec.ts | 13 +++++++------ src/types.ts | 4 ++-- tests/integration/record-replay-aborted.test.ts | 6 +++--- tests/unit/wrapper-resolved-stdin.test.ts | 2 +- 5 files changed, 15 insertions(+), 14 deletions(-) diff --git a/src/execa.ts b/src/execa.ts index 378f25d..746c114 100644 --- a/src/execa.ts +++ b/src/execa.ts @@ -80,8 +80,8 @@ const execaHooks: RunnerHooks = { // re-reading the file. The branch only fires on the record path // (passthrough/no-session pass `resolvedStdin: undefined`). buildCall's // contract: when inputFile is defined, stdin is always a string (possibly -// empty), never null — so a string-typeof check covers the optimization -// case without a redundant null branch. Closes #102. +// empty), never null, so a string-typeof check covers the optimization +// case without a redundant null branch. function realCall( file: string, args: readonly string[], diff --git a/src/tinyexec.ts b/src/tinyexec.ts index dc075b7..f2509dc 100644 --- a/src/tinyexec.ts +++ b/src/tinyexec.ts @@ -46,12 +46,13 @@ const tinyexecHooks: RunnerHooks, TinyResult> = { // tinyexec's Result is `PromiseLike & OutputApi`. Awaiting it resolves // to Output (`{ stdout, stderr, exitCode }`) and drops the OutputApi getters // (`aborted`, `killed`). To capture those on the record path we snapshot them -// from the ExecProcess BEFORE the await resolves, then return an enriched -// plain object the cassette captureResult can read. Fixes #126. +// from the ExecProcess before the await resolves, then return an enriched +// plain object the cassette captureResult can read. // -// `_resolvedStdin` is part of RunnerHooks.realCall's signature for the execa -// adapter's #102 optimization; tinyexec has no `inputFile` option and ignores -// it. +// The fourth parameter is part of RunnerHooks.realCall's signature for the +// execa adapter's inputFile optimization; tinyexec has no `inputFile` +// option and ignores it (underscore prefix per biome's +// noUnusedFunctionParameters). async function realCall( file: string, args: readonly string[], @@ -88,7 +89,7 @@ function synthesize(rec: Recording, options: Partial): TinyResult { const stdout = rec.result.stdoutLines.join('\n') const stderr = rec.result.stderrLines.join('\n') // Stored value when present; fall back to signal-derived for legacy cassettes - // recorded before `killed` was captured separately. Closes #129. + // recorded before `killed` was captured separately. const killed = rec.result.killed ?? rec.result.signal !== null const result = { diff --git a/src/types.ts b/src/types.ts index 4890172..4b1a6b1 100644 --- a/src/types.ts +++ b/src/types.ts @@ -35,8 +35,8 @@ export type Result = { * True when `subprocess.kill()` was called (execa's `r.killed`, tinyexec's * `proc.killed`). Distinct from `signal !== null`: a process terminated by * an external signal has `signal !== null` but `killed === false`. Stored - * separately so synth doesn't have to derive (and get it wrong, as #129 - * documented). Optional for backward compat with cassettes recorded before + * separately rather than derived because the two states are semantically + * different. Optional for backward compat with cassettes recorded before * the field existed. */ killed?: boolean diff --git a/tests/integration/record-replay-aborted.test.ts b/tests/integration/record-replay-aborted.test.ts index 2c36874..7a5938c 100644 --- a/tests/integration/record-replay-aborted.test.ts +++ b/tests/integration/record-replay-aborted.test.ts @@ -49,9 +49,9 @@ describe('record + replay: aborted (execa)', () => { }) }) -// Closes #126: tinyexec's awaited Output drops the OutputApi getters -// (aborted, killed) that live on the pre-await ExecProcess. The adapter's -// realCall snapshots them before await so captureResult sees real values. +// tinyexec's awaited Output drops the OutputApi getters (aborted, killed) +// that live on the pre-await ExecProcess. The adapter's realCall snapshots +// them before await so captureResult sees real values. describe('record + replay: aborted (tinyexec)', () => { const tmp = useTmpDir('sc-aborted-tinyexec-') diff --git a/tests/unit/wrapper-resolved-stdin.test.ts b/tests/unit/wrapper-resolved-stdin.test.ts index a971d5e..587973a 100644 --- a/tests/unit/wrapper-resolved-stdin.test.ts +++ b/tests/unit/wrapper-resolved-stdin.test.ts @@ -134,7 +134,7 @@ describe('wrapper.realCall: resolvedStdin threading (#102)', () => { }) test('passes resolvedStdin = undefined on no-active-session passthrough', async () => { - // No setActiveCassette() — wrapper falls through to realCall after the + // No setActiveCassette(): wrapper falls through to realCall after the // mode check (resolveMode defaults to passthrough when CI=false). const { hooks, realCall } = makeHooks() await runWrapped('node', ['-v'], { input: 'irrelevant' } satisfies Opts, hooks) From b28825267d8c18e483678f02baebc2b5722b09a5 Mon Sep 17 00:00:00 2001 From: slgoodrich Date: Sun, 3 May 2026 15:10:32 -0400 Subject: [PATCH 3/3] chore: remove em-dashes and banned tone phrases across the codebase The project's tone-and-voice rules ban em-dashes and a short list of slangy phrases (`landed`, `lands here`, `load-bearing`) in any prose that ships with the package: source comments, JSDoc, docs, and the PR template. This sweep replaces every remaining occurrence with commas, colons, semicolons, or parentheses as the surrounding context fits. Also brings two doc tables current with the recent shape-fidelity fixes: `docs/execa.md`'s `killed` row now describes the captured field with its legacy fallback (was: "Derived: signal !== null"), and `docs/tinyexec.md` drops the paragraph that flagged `aborted: false` record-side capture as outstanding (the adapter now snapshots aborted/killed pre-await, so the cassette stores real values). No source-logic or test-behavior changes; lint, typecheck, full test suite all clean. --- .github/pull_request_template.md | 10 +++++----- docs/execa.md | 4 ++-- docs/tinyexec.md | 4 +--- src/cli-review.ts | 8 ++++---- src/redact-pipeline.ts | 4 ++-- src/serialize.ts | 4 ++-- tests/helpers/paced-stdin.ts | 6 +++--- tests/plugin/lifecycle.test.ts | 2 +- tests/property/canonicalize.property.test.ts | 2 +- tests/property/scan-record-symmetry.property.test.ts | 6 +++--- tests/security/redact-stdin.test.ts | 2 +- tests/unit/cli-review.test.ts | 10 +++++----- tests/unit/execa-capture-result.test.ts | 4 ++-- tests/unit/matcher.test.ts | 10 +++++----- tests/unit/redact.test.ts | 10 +++++----- 15 files changed, 42 insertions(+), 44 deletions(-) diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 9879d02..717b990 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,6 +1,6 @@ ## What Changed - + ## Why @@ -12,10 +12,10 @@ ## Test Plan -- [ ] `npm run lint` — clean -- [ ] `npm run typecheck` — clean -- [ ] `npm test` — N passed, N skipped -- [ ] `npm run build` — clean +- [ ] `npm run lint`: clean +- [ ] `npm run typecheck`: clean +- [ ] `npm test`: N passed, N skipped +- [ ] `npm run build`: clean ## Notes diff --git a/docs/execa.md b/docs/execa.md index 066c496..dd07607 100644 --- a/docs/execa.md +++ b/docs/execa.md @@ -130,11 +130,11 @@ When the wrapper synthesizes a result on replay, it populates every documented ` | `isTerminated` | Derived: `signal !== null` | | `isForcefullyTerminated` | Stored on capture; defaults to `false` when absent | | `isGracefullyCanceled` | Stored on capture; defaults to `false` when absent | -| `killed` | Derived: `signal !== null` | +| `killed` | Stored on capture from execa's `r.killed`; if absent (older cassettes), derived from `signal !== null` | | `pipedFrom` | Always `[]` (`.pipe()` is stubbed; see below) | | `ipcOutput` | Always `[]` (`ipc: true` is rejected at validation) | -Other fields (`stdout`, `stderr`, `exitCode`, `signal`, `command`, `escapedCommand`, `durationMs`, optional `all`) round-trip as before. The reject branch throws synthesized `ExecaError` when the resolved `failed` is `true` and `reject !== false` (matching execa's default). The fallback derivation lets cassettes recorded before the `failed` field was added auto-upgrade their replay correctness — aborted and signal-killed calls now throw on replay even when the cassette was recorded before the flag existed. +Other fields (`stdout`, `stderr`, `exitCode`, `signal`, `command`, `escapedCommand`, `durationMs`, optional `all`) round-trip as before. The reject branch throws synthesized `ExecaError` when the resolved `failed` is `true` and `reject !== false` (matching execa's default). The fallback derivations let cassettes recorded before the `failed` and `killed` fields were stored auto-upgrade their replay correctness; aborted and signal-killed calls throw on replay even when the cassette predates the flag. `durationMs` is wall-clock measured by shell-cassette's wrapper around the real subprocess. diff --git a/docs/tinyexec.md b/docs/tinyexec.md index 8d4251f..133eace 100644 --- a/docs/tinyexec.md +++ b/docs/tinyexec.md @@ -81,14 +81,12 @@ The cassette schema is narrower than tinyexec's runtime result. One mapping stil - **`signal` (string vs boolean)**: tinyexec exposes `killed: boolean` but not the actual signal name. We unconditionally store `'SIGTERM'` on kill. The real signal name is lost. -`aborted` is preserved through record/replay (captured from tinyexec's `aborted: true` when AbortSignal triggered, synthesized back to `aborted` on replay). +`aborted` and `killed` are preserved through record/replay. Both fields live as getters on the pre-await `ExecProcess`; the adapter's `realCall` snapshots them before the `await` resolves so the cassette captures real values rather than `undefined` reads from the awaited `Output`. ## Failed flag Tinyexec's `Output` does not expose a `failed` boolean. shell-cassette derives `failed` on capture from `exitCode !== 0 || killed || aborted` and stores it in the cassette. Replay surfaces the stored value; older cassettes recorded before the field was stored re-derive at replay time via the same formula. The `throwOnError` reject branch keys on the resolved value, so signal-killed and aborted replays throw under `throwOnError: true` even when the cassette predates the field. -Tinyexec's awaited `Output` also drops the `aborted` and `killed` getters that live on the pre-await `ExecProcess`. As a result, tinyexec cassettes currently record `aborted: false` regardless of whether the original call was canceled. Tracked in [#126](https://github.com/slgoodrich/shell-cassette/issues/126); replay-side handling is correct. - ## Supported tinyexec options | Option | Status | diff --git a/src/cli-review.ts b/src/cli-review.ts index b480ee1..cea20da 100644 --- a/src/cli-review.ts +++ b/src/cli-review.ts @@ -54,7 +54,7 @@ export type Finding = { rule: string /** Raw match (always populated; review --json without --include-match strips it before emit). */ match: string - /** `sha256:<64 hex>` — used for skip-set membership across runs. */ + /** `sha256:<64 hex>`. Used for skip-set membership across runs. */ matchHash: string matchLength: number /** First-4 + ellipsis + last-4 if length >= 12, else full match. */ @@ -78,7 +78,7 @@ const CONTEXT_RADIUS = 2 * 1. Each finding includes context lines surrounding the match for * terminal display. * 2. Matches whose hash appears in any recording's `suppressed` array - * are skipped — the user already chose to skip them. + * are skipped (the user already chose to skip them). */ export function preScan(cassette: CassetteFile, config: Readonly): Finding[] { const skipSet = collectSuppressedHashes(cassette) @@ -273,7 +273,7 @@ export type ReviewState = { /** * Stack of cursors saved before each forward advance. `back` pops from * here so multi-step jumps (e.g. `delete`'s contiguous-recording skip) - * unwind to the actual prior decision point — not just `cursor - 1`. + * unwind to the actual prior decision point, not just `cursor - 1`. */ readonly history: readonly number[] readonly decisions: ReadonlyMap @@ -303,7 +303,7 @@ export type ReviewAction = * - back: pop the prior cursor from history and rewind to it, removing * every decision recorded in the unwound range so the user must * re-decide. Empty history is a no-op (start of review or already - * fully unwound). Allowed from 'confirming' too — pops back to + * fully unwound). Allowed from 'confirming' too; pops back to * 'reviewing' at the last decided finding. * - quit: transition to 'aborted' (decisions discarded by caller). * diff --git a/src/redact-pipeline.ts b/src/redact-pipeline.ts index 0630a11..99a554c 100644 --- a/src/redact-pipeline.ts +++ b/src/redact-pipeline.ts @@ -269,7 +269,7 @@ export function seedCountersFromCassette(cassette: CassetteFile): Map= lines.length) { - // Delay EOF too so the final answer has time to land before stdin closes. + // Delay EOF too so the final answer has time to flush before stdin closes. setTimeout(() => this.push(null), PACE_MS) return } diff --git a/tests/plugin/lifecycle.test.ts b/tests/plugin/lifecycle.test.ts index fa8dd7a..86117c2 100644 --- a/tests/plugin/lifecycle.test.ts +++ b/tests/plugin/lifecycle.test.ts @@ -36,5 +36,5 @@ describe('vitest plugin lifecycle (subprocess-driven)', () => { recursive: true, force: true, }) - }, 60000) // 60s timeout — vitest subprocess startup is slow on Windows + }, 60000) // 60s timeout because vitest subprocess startup is slow on Windows }) diff --git a/tests/property/canonicalize.property.test.ts b/tests/property/canonicalize.property.test.ts index 04ef298..074cb7b 100644 --- a/tests/property/canonicalize.property.test.ts +++ b/tests/property/canonicalize.property.test.ts @@ -5,7 +5,7 @@ import { defaultCanonicalize, MatcherState } from '../../src/matcher.js' import { normalizeTmpPath, TMP_TOKEN } from '../../src/normalize.js' import type { Call, Recording, Result } from '../../src/types.js' -// Reuse generators (deliberate inline duplication — rule of three; two +// Reuse generators (deliberate inline duplication; rule of three: two // property test files don't justify a shared helpers module yet) const genArg = fc.string({ minLength: 0, maxLength: 50 }).filter((s) => !s.includes('\n')) const genArgs = fc.array(genArg, { minLength: 0, maxLength: 5 }) diff --git a/tests/property/scan-record-symmetry.property.test.ts b/tests/property/scan-record-symmetry.property.test.ts index 891666f..7a8efcc 100644 --- a/tests/property/scan-record-symmetry.property.test.ts +++ b/tests/property/scan-record-symmetry.property.test.ts @@ -1,9 +1,9 @@ /** * Property test: scan vs record symmetry. * - * The load-bearing security guarantee: if record mode would have redacted a - * value, scan must report that value as a finding. These tests verify the - * forward direction of that guarantee for every bundled rule and for the + * Core security guarantee: if record mode would have redacted a value, scan + * must report that value as a finding. These tests verify the forward + * direction of that guarantee for every bundled rule and for the * env-key-match path. */ import path from 'node:path' diff --git a/tests/security/redact-stdin.test.ts b/tests/security/redact-stdin.test.ts index bc337b2..f170277 100644 --- a/tests/security/redact-stdin.test.ts +++ b/tests/security/redact-stdin.test.ts @@ -222,7 +222,7 @@ describe('e2e: stdin credential round-trips redacted in cassette JSON', () => { await useCassette(cp, async () => { // The replayed result is whatever the cassette stored (the redacted // stdout), but the assertion that matters is that the call matched - // at all — no ReplayMissError thrown. + // at all (no ReplayMissError thrown). const r = await execa('node', NODE_ECHO_STDIN, { input: SAMPLE_GITHUB_PAT_CLASSIC }) expect(r.exitCode).toBe(0) expect(r.stdout).toBe('') diff --git a/tests/unit/cli-review.test.ts b/tests/unit/cli-review.test.ts index 2570537..3eab99a 100644 --- a/tests/unit/cli-review.test.ts +++ b/tests/unit/cli-review.test.ts @@ -310,8 +310,8 @@ describe('applyAction', () => { state = applyAction(state, { kind: 'back' }) // Cursor returns to 0 (where the user actually was when they pressed delete). expect(state.cursor).toBe(0) - // The delete decision is gone — user must re-decide. b never had a - // decision, but verifying both are clear ensures the unwound range is + // The delete decision is gone, so the user must re-decide. b never had + // a decision, but verifying both are clear ensures the unwound range is // correctly cleared. expect(state.decisions.has('a')).toBe(false) expect(state.decisions.has('b')).toBe(false) @@ -564,9 +564,9 @@ describe('runReview interactive (driven via fake reader)', () => { beforeEach(async () => { tmp = await mkdtemp(path.join(tmpdir(), 'shell-cassette-review-int-')) outBuf = [] - // renderFinding emits the match preview, hash, and surrounding context lines — - // for a fixture with a real-looking PAT, that means secrets land in the - // vitest reporter unless stdout is captured. + // renderFinding emits the match preview, hash, and surrounding context + // lines. For a fixture with a real-looking PAT that means secrets reach + // the vitest reporter unless stdout is captured. process.stdout.write = ((s: string) => { outBuf.push(s) return true diff --git a/tests/unit/execa-capture-result.test.ts b/tests/unit/execa-capture-result.test.ts index 25c4216..4b89e05 100644 --- a/tests/unit/execa-capture-result.test.ts +++ b/tests/unit/execa-capture-result.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from 'vitest' -// captureResult is internal — its module is intentionally not in +// captureResult is internal: its module is intentionally not in // package.json's exports map, so imports go through the source path // directly. Pure-function unit test bypasses the wrapper layer entirely. import { captureResult } from '../../src/execa-capture.js' @@ -68,7 +68,7 @@ describe('execa captureResult: killed (#129)', () => { }) test('signal !== null without killed records killed=false (external signal)', () => { - // External SIGTERM (not via subprocess.kill()) — execa exposes + // External SIGTERM (not via subprocess.kill()): execa exposes // signal !== null but killed === false. The capture must preserve // that distinction so synth does not report killed=true. const r = captureResult( diff --git a/tests/unit/matcher.test.ts b/tests/unit/matcher.test.ts index df7b034..23433a7 100644 --- a/tests/unit/matcher.test.ts +++ b/tests/unit/matcher.test.ts @@ -58,7 +58,7 @@ describe('defaultCanonicalize', () => { expect(canonical.args).toEqual(['remote', 'set-url', 'origin', '/remote.git']) }) - test('is deterministic — same input produces same output', () => { + test('is deterministic: same input produces same output', () => { const c = callOf('git', ['log']) expect(defaultCanonicalize(c, DEFAULT_CONFIG.redact)).toEqual( defaultCanonicalize(c, DEFAULT_CONFIG.redact), @@ -105,7 +105,7 @@ describe('defaultCanonicalize', () => { }) }) -describe('MatcherState — basic matching', () => { +describe('MatcherState: basic matching', () => { test('returns null when no recordings', () => { const m = new MatcherState([], defaultCanonicalize, DEFAULT_CONFIG.redact) expect(m.findMatch(callOf('git', []))).toBeNull() @@ -147,7 +147,7 @@ describe('MatcherState — basic matching', () => { }) }) -describe('MatcherState — sequential consumption', () => { +describe('MatcherState: sequential consumption', () => { test('first call gets first match, second call gets second match', () => { const recs = [recordingOf('git', ['status'], 'first'), recordingOf('git', ['status'], 'second')] const m = new MatcherState(recs, defaultCanonicalize, DEFAULT_CONFIG.redact) @@ -175,7 +175,7 @@ describe('MatcherState — sequential consumption', () => { }) }) -describe('MatcherState — ambiguity warning', () => { +describe('MatcherState: ambiguity warning', () => { test('logs warning when multiple unconsumed recordings could match', () => { const stderrSpy = vi.spyOn(process.stderr, 'write').mockImplementation(() => true) try { @@ -192,7 +192,7 @@ describe('MatcherState — ambiguity warning', () => { }) }) -describe('MatcherState — custom canonicalize', () => { +describe('MatcherState: custom canonicalize', () => { test('uses provided canonicalize fn (e.g., command-only matching)', () => { const commandOnly: Canonicalize = (call) => ({ command: call.command }) const recs = [recordingOf('git', ['status'])] diff --git a/tests/unit/redact.test.ts b/tests/unit/redact.test.ts index df94004..636c188 100644 --- a/tests/unit/redact.test.ts +++ b/tests/unit/redact.test.ts @@ -14,7 +14,7 @@ const baseConfig: RedactConfig = { } describe('public redact() wraps pipeline', () => { - test('bundledPatterns: false — input unchanged for credential-shaped value', () => { + test('bundledPatterns: false leaves input unchanged for credential-shaped value', () => { const r = redact({ source: 'env', value: SAMPLE_GITHUB_PAT_CLASSIC }, baseConfig, { counted: false, }) @@ -23,7 +23,7 @@ describe('public redact() wraps pipeline', () => { expect(r.warnings).toEqual([]) }) - test('counted: false — emits placeholder without counter', () => { + test('counted: false emits placeholder without counter', () => { const config: RedactConfig = { ...baseConfig, bundledPatterns: true } const r = redact({ source: 'env', value: SAMPLE_GITHUB_PAT_CLASSIC }, config, { counted: false, @@ -31,7 +31,7 @@ describe('public redact() wraps pipeline', () => { expect(r.output).toBe('') }) - test('counted: true — increments counter and returns counted placeholder', () => { + test('counted: true increments counter and returns counted placeholder', () => { const config: RedactConfig = { ...baseConfig, bundledPatterns: true } const counters = new Map() const r = redact({ source: 'env', value: SAMPLE_GITHUB_PAT_CLASSIC }, config, { @@ -58,7 +58,7 @@ describe('public redact() wraps pipeline', () => { expect(r.output).toBe('') }) - test('source: "stdin", counted: true — counter is per (stdin, rule)', () => { + test('source: "stdin", counted: true uses per-(stdin, rule) counter', () => { const config: RedactConfig = { ...baseConfig, bundledPatterns: true } const counters = new Map() const r = redact({ source: 'stdin', value: SAMPLE_GITHUB_PAT_CLASSIC }, config, { @@ -69,7 +69,7 @@ describe('public redact() wraps pipeline', () => { expect(counters.get('stdin:github-pat-classic')).toBe(1) }) - test('source: "stdin" — custom regex rule fires', () => { + test('source: "stdin" custom regex rule fires', () => { const config: RedactConfig = { ...baseConfig, customPatterns: [{ name: 'my-secret', pattern: /SECRET-[A-Z0-9]+/ }],