Skip to content

testing: pin stdin closed while recording - #34

Merged
thorwhalen merged 3 commits into
masterfrom
testing-pin-stdin
Aug 30, 2026
Merged

thorwhalen merged 3 commits into
masterfrom
testing-pin-stdin

Conversation

@thorwhalen

@thorwhalen thorwhalen commented Aug 30, 2026 •

Copy link
Copy Markdown
Member

Found while migrating grub as part of the fleet rollout. grub's headline behaviour is that omitting the query drops you into an interactive prompt — and that behaviour could not be characterized at all from a terminal.

The defect

cw.testing._run_subprocess never set stdin, so the recorded child inherited the recorder's. For any CLI with an interactive path — grub's REPL, a cw.confirm prompt — the recording itself depended on where it was run:

recorder's stdin recorded case
a terminal returncode: None, "cw.testing: timed out after 30s"
/dev/null (CI, pytest) returncode: 0, "prompt> read: <EOF>"

Neither is a fact about the CLI. The practical damage is a golden recorded in CI that fails when a developer replays it locally — precisely the class of failure RECORDING_ENV exists to remove ("to remove a source of variation between two machines"). stdin belonged in that list and was missing.

The fix

stdin=subprocess.DEVNULL in _run_subprocess, and the same treatment on descriptor 0 in capture(), saved and restored beside 1 and 2. The module argues that using one mechanism for both sides of a diff is what makes the diff mean something; that only holds if the two recording mechanisms agree about stdin too.

The test

Records the same case twice under two different stdins — an open pipe nobody writes to (what a terminal looks like to the child), then /dev/null — and asserts the two recordings are equal. Verified it fails without the fix: {'returncode': None, 'stderr': 'timed out'} vs {'returncode': 0, 'stdout': 'prompt> read: <EOF>'}.

Worth noting for whoever reads the test: the obvious test — "a stdin-reading command records EOF" — passes either way under pytest, because pytest already hands fd 0 a devnull. Only the two-stdin comparison catches this, which is presumably why it was not caught before.

Checks

  • 874 passed, 2 skipped
  • python -m cw.testing parity → 8 shapes / 137 cases: identical
  • No behaviour change for any CLI that does not read stdin, so no existing golden is invalidated.

Second defect, same theme: Windows' .exe (added in 364c6a3 → 1e43fc2)

Found on the very next CI run of the same migration. The module promises "a golden recorded on a Mac asserts cleanly on a Windows runner". It did not.

argparse takes its prog from basename(sys.argv[0]), and a console script is installed as opsward.exe on Windows:

usage: opsward     [-h] {diagnose,...}     on a Mac
usage: opsward.EXE [-h] {diagnose,...}     on the runner

22 of 31 cases failed on that alone — a wall of red saying nothing about the CLI. The docstring's Windows section actually names the .exe console-script shim as a reason parity avoids subprocesses; characterize and replay do spawn subprocesses and had no defence against the thing that paragraph named.

scrub_exe_suffix rewrites only the program's own stem (a CLI that mentions some other .exe is untouched), case-insensitively (the extension surfaces as .EXE on some Windows configurations and .exe on others — otherwise two Windows runners disagree with each other), applied in _run_subprocess, the single choke point all three subprocess paths share, at record time as well as replay time — the same treatment and the same stated rationale as scrub_addresses.

Two hazards worth keeping in review: the replacement is a function, not a template string, and the stem is split on both separators. A Windows path reaches this on a POSIX host through a golden's recorded prog, where os.path.basename does not split on a backslash and re.sub reads the leftover \p as a bad escape. The doctest caught that on the first run.

The test installs one toy CLI under two names, toy and toy.EXE, records from the first and replays against the second — an exact simulation, not a mock. Verified it fails without the fix.

No POSIX-recorded golden changes, because there is no .exe in one.

Updated checks: 877 passed, 2 skipped; parity 8 shapes / 137 cases: identical.

… about a console

`_run_subprocess` never set `stdin`, so a recorded case inherited the recorder's.
For any CLI with an interactive path -- grub drops into a REPL when the query is
omitted, `cw.confirm` asks a question -- that made the *recording itself* depend
on where it was run:

    recorder's stdin        recorded case
    ----------------        -------------------------------------------
    a terminal              returncode None, "cw.testing: timed out"
    /dev/null (CI, pytest)  returncode 0, "prompt> read: <EOF>"

Both are wrong in the same way: neither is a fact about the CLI. The practical
damage is a golden recorded in CI that fails when a developer replays it locally,
for a reason that has nothing to do with the command line -- which is exactly the
class of failure RECORDING_ENV exists to remove ("to remove a source of variation
between two machines"). stdin belongs in that list, and it was missing.

`capture()` gets the same treatment on descriptor 0, saved and restored beside 1
and 2. The module argues that using one mechanism for both sides of a diff is what
makes the diff mean something; that only holds if the two recording mechanisms
agree about stdin too.

Found while migrating `grub`, whose headline behaviour is the no-query REPL and
which therefore could not be characterized at all from a terminal.

The regression test records the same case twice under two different stdins -- an
open pipe nobody writes to, then /dev/null -- and asserts the two recordings are
equal. Verified it fails without the fix (returncode None + timeout vs 0 + EOF).
Note the more obvious test, "a stdin-reading command records EOF", passes either
way under pytest, because pytest already hands fd 0 a devnull; only the two-stdin
comparison catches this.

874 passed. Parity gate: 8 shapes / 137 cases identical.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr
…serts on the runner

The module promises that "a golden recorded on a Mac asserts cleanly on a Windows
runner", and it did not. `argparse` takes its `prog` from `basename(sys.argv[0])`,
and a console script is installed as `opsward.exe` on Windows, so the same CLI at
the same commit reports

    usage: opsward     [-h] {diagnose,...}     on a Mac
    usage: opsward.EXE [-h] {diagnose,...}     on the runner

and every case that prints a usage line or an error prefix differs. In the
migration that found this, that was 22 of 31 cases -- a wall of red saying nothing
about the CLI.

The docstring's Windows section actually named the `.exe` console-script shim as a
reason `parity` avoids subprocesses. `characterize` and `replay` DO spawn
subprocesses, and had no defence against the very thing that paragraph named.

`scrub_exe_suffix` rewrites only the *program's own* stem, so a CLI that talks
about some other `.exe` is untouched; the match is case-insensitive because the
extension surfaces as `.EXE` on some Windows configurations and `.exe` on others,
which would otherwise make two Windows runners disagree with each other. It is
applied in `_run_subprocess`, the single choke point all three subprocess paths go
through, at record time as well as replay time -- the same treatment, and for the
same stated reason, as `scrub_addresses`.

Two hazards worth keeping: the replacement is a function, not a template string,
and the stem is split on both separators. A Windows path reaches this on a POSIX
host through a golden's recorded `prog`, where `os.path.basename` does not split on
a backslash and `re.sub` reads the leftover `\p` as a bad escape. The doctest found
that on the first run.

The test installs one toy CLI under two names, `toy` and `toy.EXE`, records from
the first and replays against the second -- an exact simulation rather than a mock.
Verified it fails without the fix.

877 passed. Parity gate: 8 shapes / 137 cases identical. No POSIX-recorded golden
changes, since there is no `.exe` in one.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr
…'s first word

Two fixes to the previous commit, both surfaced by its own CI.

1. The regression test was POSIX-only. It wrote a shebang and chmod +x, which the
   Windows runner rejected with `OSError: [WinError 193] %1 is not a valid Win32
   application` -- exactly the sort of assumption a class named for Windows should
   not be making. Both toys now run as `[sys.executable, path]`.

2. That fix exposed a real narrowness in `scrub_exe_suffix`: it derived the stem
   from `command[0]` alone, so `['python', 'toy.exe']` scrubbed nothing, because
   the program argparse names is the *second* word. It now considers every part of
   the command. A part that is not the program yields a stem appearing nowhere in
   the text, so considering it costs nothing -- and the test is now stronger than
   the one that failed, since it covers the harder shape rather than the easy one.

878 passed. Parity gate: 8 shapes / 137 cases identical.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr
@thorwhalen
thorwhalen merged commit 4948cbc into master Aug 30, 2026
12 checks passed
@thorwhalen
thorwhalen deleted the testing-pin-stdin branch August 30, 2026 22:25
thorwhalen added a commit to thorwhalen/grub that referenced this pull request Aug 30, 2026
argparse takes its `prog` from `basename(sys.argv[0])`, and on Windows the console
script is installed as `grub.EXE`, so every recorded usage line and error prefix
differed on the runner:

    - usage: grub     [-h] [-m METHOD] ... source [query ...]
    + usage: grub.EXE [-h] [-m METHOD] ... source [query ...]

That is a fact about packaging, not about the command line. cw 0.1.1 scrubs it
(i2mint/cw#34), which is also where the stdin fix this repo's no-query REPL case
depends on landed. Both were found by this migration, so the floor is raised rather
than left at 0.1: with `cw>=0.1` a resolver could pick 0.1.0 and this repo's own
parity test would fail on Windows for a reason nobody could act on.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr
thorwhalen added a commit to thorwhalen/opsward that referenced this pull request Aug 30, 2026
argparse takes its `prog` from `basename(sys.argv[0])`, and on Windows the console
script is installed as `opsward.EXE`, so 22 of the 31 recorded cases differed on
the runner:

    - usage: opsward     [-h] {diagnose,generate,maintain,recommend,install-skills,find} ...
    + usage: opsward.EXE [-h] {diagnose,generate,maintain,recommend,install-skills,find} ...

That is a fact about packaging, not about the command line. cw 0.1.1 scrubs it
(i2mint/cw#34), a defect this migration found. The floor is raised rather than left
at 0.1 because with `cw>=0.1` a resolver could pick 0.1.0 and this repo's own parity
test would fail on Windows for a reason nobody could act on.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr
thorwhalen added a commit to thorwhalen/opsward that referenced this pull request Aug 30, 2026
The Windows runner is down to six differing cases, and none is about the command
line. opsward prints `misc\docs\...` where POSIX prints `misc/docs/...`, and it
reports file sizes inflated by CRLF checkout (a 37-byte stub measures 40). Both
are the pre-existing Windows bugs in #21 -- the same run fails
test_generate.py::test_python_docs_path for the same reason -- and argh printed
exactly the same thing.

They are listed as `expect_diff` rather than skipping the test on Windows, so
everything else there stays asserted: exit codes, stdout, stderr, usage lines and
the whole --help body. That distinction is not academic. The `.exe` defect this
migration found in cw (i2mint/cw#34) lived exactly in the part a platform-wide
skip would have stopped checking, and it took a Windows run to see it.

If one of the six starts matching, the test fails with `unexpected-match`. That is
the correct outcome: it means #21 was fixed and the entry should be deleted.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr
thorwhalen added a commit to thorwhalen/grub that referenced this pull request Aug 30, 2026
* Characterize the argh CLI before migrating to cw

18 argv-level cases recorded from the current argh-based console script: the full
`--help` surface, the no-query interactive prompt (grub's headline behaviour), a
search through each flag that reaches the searcher and each that reaches the
Searcher constructor, both spellings argh derives for every parameter (including
`--n`, the long form for a one-character name), grub's own exit-1 bad-source path,
and five usage errors that must keep exit code 2.

Recorded first, on purpose: this is the baseline the migration is measured
against, so it has to exist in history before any source changes.

`tests/fixtures/corpus/` is four one-line documents, well separated by topic, so a
search case has a single unambiguous top hit. `misc/README.md` documents the
harness: how to re-record, and the four rules for adding a case.

One case was written and then deliberately removed: `--extensions .md` over a
.txt-only corpus raises `ValueError: empty vocabulary` out of scikit-learn and the
uncaught traceback names 17 absolute paths -- unreplayable on another machine, and
it names the dispatcher's own frames, so it could never have survived this
migration regardless. Worth knowing that path exists; it is a grub rough edge for
another day, not something to pin.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr

* Wave 0: swap argh for cw's compat shim (one line)

`from cw import compat as argh` -- nothing else changes, including the `argv=`
keyword `dispatch_command` is called with here. Replayed against the golden
recorded in the previous commit:

    18/18 identical   (--strict-help, so the --help body is asserted too)

Kept as its own commit because it is the evidence for the claim: the compat shim
is a drop-in, and any behaviour difference in the next commit is attributable to
the move to cw's own API rather than to leaving argh.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr

* Wave 2: dispatch through cw's own API, and pin the CLI surface in CI

`main` now returns `cw.dispatch(run, argv)` directly rather than going through the
compat shim, and `argh` is replaced by `cw>=0.1,<0.2` in `dependencies`.

Note the argv handoff: argh took it as the keyword `argv=`, cw takes it as the
second positional. `main` still returns an int and still returns 0 on success, so
`test_cli_main_entry_point`'s `main([...]) == 0` holds, and the console script's
exit code is unchanged.

Replay against the pre-migration goldens, with the --help body asserted:

    18/18 identical on CPython 3.12   (recorded from argh)
    18/18 identical on CPython 3.10   (recorded from argh)
    no --help changes
    38 passed

New `tests/test_cli_parity.py` replays the golden in the ordinary test run, so
this cannot rot: a refactor here, or a future cw release, that changes any exit
code, any byte of stdout/stderr, or the usage line fails the suite. Verified it
can fail -- renaming `snippets` to `snippet` turned it red naming exactly that
flag. (A first attempt at a negative test, forcing `convention=cw.MODERN`, did
NOT fail, and that is correct rather than alarming: for a single command whose
options are all keyword defaults, the two conventions produce identical grammar.)

`misc/cli_golden_py310.json` is recorded from *argh* too, on a throwaway 3.10 venv
over a worktree pinned at the pre-migration commit -- a migration proof on both
matrix versions, not a baseline taken after the fact. The only cases that differ
between the two recordings are the two usage errors, where CPython 3.12 stopped
listing `nargs='*'` positionals among "the following arguments are required".

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr

* Require cw>=0.1.1: the Windows console-script fix

argparse takes its `prog` from `basename(sys.argv[0])`, and on Windows the console
script is installed as `grub.EXE`, so every recorded usage line and error prefix
differed on the runner:

    - usage: grub     [-h] [-m METHOD] ... source [query ...]
    + usage: grub.EXE [-h] [-m METHOD] ... source [query ...]

That is a fact about packaging, not about the command line. cw 0.1.1 scrubs it
(i2mint/cw#34), which is also where the stdin fix this repo's no-query REPL case
depends on landed. Both were found by this migration, so the floor is raised rather
than left at 0.1: with `cw>=0.1` a resolver could pick 0.1.0 and this repo's own
parity test would fail on Windows for a reason nobody could act on.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr
thorwhalen added a commit to thorwhalen/opsward that referenced this pull request Aug 30, 2026
* Characterize the argh CLI before migrating to cw

31 argv-level cases recorded from the current argh-based console script: the
whole help surface (top level + every subcommand), a happy path per subcommand,
the short/long/`=` option spellings argh derives, seven usage errors that must
keep exit code 2, and argh's own no-argument behaviour (usage on *stdout*,
exit 0 -- which plain argparse does not do).

Recorded first, on purpose: this is the baseline the migration is measured
against, so it has to exist in history before any source changes.

The corpus deliberately excludes `--format json` and every `install-skills`
happy path. Both print the *resolved* project root, and a committed golden that
embeds an absolute path cannot be replayed on another machine. Their grammar is
still pinned by the tier-2 usage line of the corresponding `--help` case.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr

* Wave 0: swap argh for cw's compat shim (one line)

`from cw import compat as argh` -- nothing else changes. Replayed against the
golden recorded in the previous commit:

    31/31 identical   (--strict-help, so the --help bodies are asserted too)
    no --help changes
    135 passed

Kept as its own commit because it is the evidence for the claim: the compat
shim is a drop-in, and any behaviour difference in the next commit is
attributable to the move to cw's own API rather than to leaving argh.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr

* Wave 2: dispatch through cw's own API, and pin the CLI surface in CI

`__main__.py` now calls `cw.dispatch(_dispatch_funcs)` directly rather than
going through the compat shim, and `argh` is replaced by `cw>=0.1,<0.2` in
`dependencies`. `rg -n argh opsward/` returns nothing.

Replay against the pre-migration golden, with the --help bodies asserted:

    31/31 identical
    no --help changes
    136 passed

`prog=` is deliberately NOT passed. Letting argparse derive the program name
from sys.argv[0] is what keeps `python -m opsward` reporting `__main__.py` and
the console script reporting `opsward` -- pinning `prog='opsward'` would have
changed the `-m` form's usage line, which is the invocation the module
docstring documents and the whole existing test suite uses.

New `tests/test_cli_parity.py` replays the golden as part of the ordinary test
run, so this cannot rot: a refactor here, or a future cw release, that changes
any exit code, any byte of stdout/stderr, or any `usage:` line fails the suite.
Verified it can fail -- forcing a different naming convention produced a red
test naming the exact flags that moved. The full --help *body* is asserted only
when the running CPython matches the one that recorded the golden, because
CPython rewrites its own help rendering between versions and the matrix here is
3.10 + 3.12.

Docs updated to describe the pinned surface and how to re-record it.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr

* One CLI golden per CPython version in the CI matrix

CI went red on Python 3.10, and it was not cw. argparse is stdlib and rewrites
its own text between versions:

  * 3.12 stopped listing `nargs='*'` positionals among "the following arguments
    are required", so bare `opsward find` says `required: query` on 3.12 and
    `required: query, project-roots` on 3.10;
  * 3.12 also changed how `invalid choice` quotes the choices.

Verified against plain argparse on 3.10/3.11/3.12/3.13 with no argh and no cw in
the picture. Those two cases are the ONLY difference between the two recordings;
argh produces them identically. A single golden asserted across a matrix fails
for something nobody caused, which is the fastest way to teach a team to ignore
a red parity test.

So: `misc/cli_golden_py310.json` and `misc/cli_golden_py312.json`, and the test
picks the one matching the running interpreter. A version with no recording
fails loudly with instructions -- a parity test that quietly skips is worse than
none.

The 3.10 golden is recorded from *argh* too, not from the migrated code: a
throwaway 3.10 venv over a worktree pinned at the pre-migration commit. It is a
migration proof on both matrix versions, not a baseline taken after the fact.
Replaying it against the cw code on 3.10 gives 31/31 identical, same as 3.12.

`strict_help=True` unconditionally now. The gate on the recording interpreter is
what makes that safe, and it upgrades the --help body from advisory to asserted.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr

* Require cw>=0.1.1: the Windows console-script fix

argparse takes its `prog` from `basename(sys.argv[0])`, and on Windows the console
script is installed as `opsward.EXE`, so 22 of the 31 recorded cases differed on
the runner:

    - usage: opsward     [-h] {diagnose,generate,maintain,recommend,install-skills,find} ...
    + usage: opsward.EXE [-h] {diagnose,generate,maintain,recommend,install-skills,find} ...

That is a fact about packaging, not about the command line. cw 0.1.1 scrubs it
(i2mint/cw#34), a defect this migration found. The floor is raised rather than left
at 0.1 because with `cw>=0.1` a resolver could pick 0.1.0 and this repo's own parity
test would fail on Windows for a reason nobody could act on.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr

* Assert the CLI on Windows too, minus six known-bad content cases

The Windows runner is down to six differing cases, and none is about the command
line. opsward prints `misc\docs\...` where POSIX prints `misc/docs/...`, and it
reports file sizes inflated by CRLF checkout (a 37-byte stub measures 40). Both
are the pre-existing Windows bugs in #21 -- the same run fails
test_generate.py::test_python_docs_path for the same reason -- and argh printed
exactly the same thing.

They are listed as `expect_diff` rather than skipping the test on Windows, so
everything else there stays asserted: exit codes, stdout, stderr, usage lines and
the whole --help body. That distinction is not academic. The `.exe` defect this
migration found in cw (i2mint/cw#34) lived exactly in the part a platform-wide
skip would have stopped checking, and it took a Windows run to see it.

If one of the six starts matching, the test fails with `unexpected-match`. That is
the correct outcome: it means #21 was fixed and the entry should be deleted.

Claude-Session: https://claude.ai/code/session_01K6LB3AwUmKDxaFNZ2NqPGr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant