Skip to content

fix: capture killed/aborted on record; remove internal export leakage - #130

Merged
slgoodrich merged 3 commits into
mainfrom
fix/open-issues-batch
May 3, 2026
Merged

slgoodrich merged 3 commits into
mainfrom
fix/open-issues-batch

Conversation

@slgoodrich

Copy link
Copy Markdown
Owner

Summary

Bundles four open issues into one PR (#72 and #81 deferred as features).

Why

#129: replay synth conflated killed with signal !== null

execa's killed is true only when subprocess.kill() was called. A process terminated by an external signal has signal !== null but killed === false on the real result. Replay synth derived killed: signal !== null, so cassettes recorded for externally-signaled processes replayed with the wrong killed value. Both adapters now store r.killed === true on capture; serialize emits killed when defined; synth uses rec.result.killed ?? signal !== null so cassettes recorded before this change still replay correctly.

#126: tinyexec recorded aborted: false / killed: false on every call

tinyexec's Result = PromiseLike<Output> & OutputApi. Awaiting it drops the OutputApi getters (aborted, killed) that live on the ExecProcess. The adapter previously read those on the awaited Output and got undefined, so every cassette stored aborted: false/killed: false regardless of the call's outcome. The adapter's realCall now grabs the ExecProcess synchronously, awaits it for the Output, then enriches the resolved value with proc.aborted / proc.killed before captureResult runs.

#102: inputFile was read twice on the record path

buildCall reads inputFile to populate Call.stdin (for canonical matching). Real execa then re-reads the same file because we forwarded the original options. RunnerHooks.realCall gains a fourth parameter resolvedStdin: call.stdin on the record path, undefined on passthrough/no-session paths. The execa adapter swaps options.inputFile for input: resolvedStdin when both are present, so real execa consumes the resolved bytes instead of re-reading the file.

#128: _captureResultForTesting leaked into public sub-paths

The underscore-prefixed test export appeared in the published .d.ts for shell-cassette/execa and shell-cassette/tinyexec. Each adapter now imports captureResult from a sibling internal module (src/execa-capture.ts, src/tinyexec-capture.ts) that is not in package.json's exports map. Tests import the same path.

Schema and API

Schema stays additive. Cassettes without the killed field load and replay correctly via the synth fallback. Format stays at version 2.

RunnerHooks lives in src/wrapper.ts, which is not in the package's exports map, so the signature change (4th param) is internal.

Related Issues

Closes #126
Closes #128
Closes #129
Closes #102

Test Plan

  • npm run lint
  • npm run typecheck
  • npm test (777 passing, 2 skipped)
  • npm run build
  • tests/integration/record-replay-aborted.test.ts extended to cover tinyexec
  • tests/unit/execa-capture-result.test.ts and tinyexec-capture-result.test.ts cover the new killed field
  • tests/integration/wrapper.test.ts covers synth fallback (legacy cassette without killed, new cassette with killed: false and signal: 'SIGTERM')
  • tests/unit/wrapper-resolved-stdin.test.ts covers resolvedStdin threading on all five wrapper paths
  • tests/unit/execa-input-file-swap.test.ts covers the execa adapter's inputFile -> input swap
  • tests/unit/tinyexec-adapter.test.ts updated for the pre-await snapshot contract

Notes

tests/integration/record-replay-aborted.test.ts previously had a banner deferring tinyexec coverage to #126. That banner is removed; the file now covers both adapters.

The two tests in tests/unit/tinyexec-adapter.test.ts that asserted on aborted: true / killed: true were rewritten to mock the realistic shape: a Promise carrying the OutputApi fields as own properties, mirroring how real tinyexec exposes them. The previous mocks (flat object on mockResolvedValueOnce) implicitly tested the buggy path.

slgoodrich added 3 commits May 3, 2026 14:55
#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<Output> & 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.
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.
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.
@slgoodrich
slgoodrich merged commit 1cabaf0 into main May 3, 2026
5 checks passed
@slgoodrich
slgoodrich deleted the fix/open-issues-batch branch May 3, 2026 19:43
@slgoodrich slgoodrich mentioned this pull request May 3, 2026
4 tasks done
slgoodrich added a commit that referenced this pull request May 3, 2026
Patch release covering two bug-fix PRs since v0.6.0:

- #127 (replay shape fidelity): timedOut, failed-derivation correctness,
  and execa subprocess-API stubs
- #130 (record-side flag capture): killed/aborted captured correctly on
  both adapters; inputFile double-read retired; internal export leakage
  removed

Cassette schema stays at version 2; new Result fields are JSON-additive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant