From 82691033c67e3eb353d3a831d807861ffd54d152 Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Fri, 11 Sep 2026 14:06:52 +0100 Subject: [PATCH 1/5] Stage 8 exit audit: close three criteria gaps, three stale documents make ci was green throughout -- 1200 tests, 144 scenarios, 99% coverage -- and three of Stage 8's nine Completion Criteria were marked Met against checks that did not test what the criterion said. None needed judgement to find; each is a sentence the criterion had already written down. This is docs/practices.md's own "An exit audit reads each criterion to its last sentence", recurring on the one stage whose previous audit is why Criteria 6-9 exist at all. Criterion 6's rendered-pixel clause ("a keyboard seek and a mouse drag each change window.renderer.snapshot()'s content") was checked by nothing: neither scrub test calls snapshot() at all, and the keyboard one's own docstring says it proves PlaybackState.position. Two new tests close it. Criterion 8 named a scale nothing had ever run ("a real multi-hundred-frame pyflow record run"); the CLI test used --max-frames 20 and TASK-049's by-hand check used 25. Closed by a real 300-frame CLI run, plus a separate test pinning peak on-disk count during the run -- the claim _advance_and_checkpoint's docstring makes ("as the recording grows, not only once it finishes") and which the final file set cannot prove either way. Criterion 1 rested on a by-hand check and said so in its own text. Re-verified by hand here, then gated: a fresh-subprocess check over the transitive closure for all four headless modules. All three fixes mutation-tested before being trusted green. The Criterion 6 pair took three attempts, and the second is the useful part: comparing a seek-reached frame against an autoplay-reached frame at the same index PASSED against a deliberately frozen field, because both sides came from the same run and were equally frozen. The working version takes its reference from a separate play() window launched at the target frame, and compares the field region only, cropping away the scrub bar. Recorded as a standing rule in docs/practices.md: a reference the run under test produced proves consistency, not correctness. Documentation the stage invalidated and never updated, all in files no Stage 8 task opened: - docs/architecture/CLAUDE.md said "all five" criteria met, falsified two days later by the reopening that added four more. - docs/repository-manifest.md said TASK-046/047 were "still unbuilt" -- stale from the day it was written, since both landed alongside the TASK-045 the same sentence credits. - docs/architecture/sequences.md (Checked-by: stage-boundary) described only TASK-045/046/047, and still said playback had "no declared-field/scalar-colormap path yet" two days after TASK-051 built exactly that. All four reopening tasks landed without it being touched. Its Maintenance section now records why a task anchor cannot reach a task the file has never named. Also: pyflow play --help documented none of the controls the subcommand exists for (pause, speed, scrub) -- README was the only place to find them, now pinned by a test. Linux CI had no DISPLAY and no xvfb, so all 8 display-guarded tests skipped there while passing on Windows, making a green two-platform matrix prove live-window behaviour on one platform; now run under xvfb-run. 19 stray checkpoint_*.pt from a 2026-09-07 manual run removed from the repository root, and .gitignore widened to match the files rather than only the default directory. Not done, deliberately, and now recorded where a decision goes rather than in a task's Design decisions: the Goal's "scrubbed to any point" is still satisfied only within the window one pyflow play invocation loads. The maintainer was asked at the audit and chose to defer. Building it needs a new task and a new criterion, not a quiet widening of Criterion 6. A user meets the boundary in pyflow play --help. Verified live, not only unit-tested: pyflow play --help and pyflow record --help run and checked by eye; a real 300-frame capped record run leaves exactly {0, 290, 295, 300} on disk from 61 written. 1200 -> 1209 tests. make ci green. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 21 +- .gitignore | 13 ++ docs/architecture/CLAUDE.md | 10 +- docs/architecture/sequences.md | 112 +++++++-- docs/planning/roadmap.md | 141 ++++++++++- docs/planning/status.md | 2 +- docs/practices.md | 49 ++++ docs/repository-manifest.md | 2 +- src/pyflow/__main__.py | 18 ++ tests/integration/CLAUDE.md | 34 +++ tests/integration/test_import_order.py | 53 +++++ tests/integration/test_playback_cli.py | 309 +++++++++++++++++++++++++ tests/integration/test_record_cli.py | 45 ++++ tests/unit/test_recording.py | 53 ++++- 14 files changed, 838 insertions(+), 24 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d5df175..7ec7dcf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,12 +72,31 @@ jobs: run: | echo "http://archive.ubuntu.com/ubuntu" | sudo tee /etc/apt/apt-mirrors.txt sudo apt-get update - sudo apt-get install -y libegl1 libgl1 mesa-vulkan-drivers + sudo apt-get install -y libegl1 libgl1 mesa-vulkan-drivers xvfb - name: make install run: make install + # `xvfb` above, and `xvfb-run` below, are what let the display- + # guarded tests (`@_needs_a_real_display` in `tests/integration/ + # test_interactive_window.py` and `test_playback_cli.py`) run here + # at all. **Added 2026-09-11 by the Stage 8 exit audit**, which + # found those 10 tests -- including every check that Stage 8's own + # Completion Criterion 6 rests on -- skipping silently on Linux + # while passing on Windows, so a green two-platform matrix was + # proving live-window behaviour on exactly one platform. Without a + # `DISPLAY`, `_display_available()` short-circuits before GLFW is + # ever constructed (deliberately: GLFW hard-aborts the process + # rather than raising, see `tests/integration/CLAUDE.md`), so the + # skip was invisible in the run summary. + - name: make ci (Linux, under a virtual display) + if: runner.os == 'Linux' + env: + PYTEST_WORKERS: "8" + run: xvfb-run -a make ci + - name: make ci + if: runner.os != 'Linux' env: # Runs with more test parallelism than a local `make test` # (Makefile's own `PYTEST_WORKERS ?= 4` default) -- CI runs on diff --git a/.gitignore b/.gitignore index 6499b69..838a584 100644 --- a/.gitignore +++ b/.gitignore @@ -46,3 +46,16 @@ checkpoints/ # `replay.py`) -- README's own documented example uses `--cache cache` # from the repository root; same reasoning as `checkpoints/` above. cache/ + +# The same run output as `checkpoints/` above, but written straight into +# whatever directory `pyflow record`/`resume` was invoked from -- what +# `--output-dir .` produces, and what a user experimenting from the +# repository root gets. **Added 2026-09-11 by the Stage 8 exit audit**, +# which found 19 of these sitting untracked in the repository root from +# a 2026-09-07 manual run: the `checkpoints/` rule above only ever +# matched the default directory, never the files themselves. +checkpoint_*.pt + +# `pyflow play --cache`'s own materialized windows, for the same reason +# -- `window_{from}_{to}.pt` written to a cache directory given as `.`. +window_*.pt diff --git a/docs/architecture/CLAUDE.md b/docs/architecture/CLAUDE.md index a69bd2b..25ddc69 100644 --- a/docs/architecture/CLAUDE.md +++ b/docs/architecture/CLAUDE.md @@ -42,12 +42,20 @@ all three of that stage's pieces in real code: `src/pyflow/checkpoint.py`/`recording.py`/`simulation_run.py` (TASK-045, checkpointing), and `src/pyflow/replay.py`/`playback.py` (TASK-046/047, windowed replay and interactive playback, built the same -day). Stage 8 itself is complete, all five of its own Completion +day). Stage 8 itself is complete, all **nine** of its own Completion Criteria met -- see `docs/planning/roadmap.md`'s own Stage 8 Status section for the one real course-correction along the way (its Golden Demo moved from Heat Diffusion to Lid-Driven Cavity once TASK-047's own scope decision made the first choice incompatible). +**This paragraph said "all five" until 2026-09-11**, correct when +written on 2026-09-07 and falsified two days later when the stage was +reopened and Criteria 6-9 were added (TASK-048/049/050/051). Found by +that stage's own exit audit, not by anything that runs -- a criteria +*count* is exactly the decaying-number shape `docs/practices.md` +warns about under "A stage's documentation sweep is a grep, not a diff +review", restated here in a file no Stage 8 task ever opened. + **Section 3's anchor history is worth keeping, because it is why the subsection existed to be finished at all.** It used to say the subsection was "anchored to the specific roadmap task that will build it diff --git a/docs/architecture/sequences.md b/docs/architecture/sequences.md index 28707f0..0b37a72 100644 --- a/docs/architecture/sequences.md +++ b/docs/architecture/sequences.md @@ -384,6 +384,9 @@ sequenceDiagram alt frame_count % checkpoint_interval == 0, or final frame recording->>checkpoint: write_checkpoint(frame_count, config, state.fields) checkpoint->>Disk: checkpoint_{frame_count:08d}.pt + opt max_checkpoints_retained set (TASK-049) + recording->>Disk: _prune_checkpoints() -- keep frame 0 + newest N + end end end ``` @@ -443,6 +446,9 @@ sequenceDiagram alt frame_count % checkpoint_interval == 0, or final frame recording->>checkpoint: write_checkpoint(frame_count, config, state.fields) checkpoint->>Disk: checkpoint_{frame_count:08d}.pt + opt max_checkpoints_retained set (TASK-049) + recording->>Disk: _prune_checkpoints() -- keep frame 0 + newest N + end end end ``` @@ -472,6 +478,18 @@ nothing. Those are Stage 8's own second and third bullets, and both are built now (TASK-046/047, 2026-09-07): the rest of this subsection covers them. +**Retention prunes after every write, not once at the end** (TASK-049, +added to both diagrams above 2026-09-11) -- which is what makes +`max_checkpoints_retained` bound peak disk use *as a long run grows* +rather than only its final footprint. A 300-frame run at `interval=5` +with a cap of 3 writes 61 checkpoints and never holds more than 5 at +once. Opt-in: unset (the default) deletes nothing, so every config +written before TASK-049 produces exactly the files it always did. Frame +0 is excluded from the count itself rather than merely surviving by +being newest, so a capped recording always keeps a point to restart +from. `resume` prunes the whole `output_dir`, including checkpoints an +earlier `record` left there, not only the files it wrote itself. + ### Built today: windowed replay and interactive playback (`pyflow play`) ```mermaid @@ -487,6 +505,10 @@ sequenceDiagram playback->>replay: materialize_or_load_window(...) alt --cache given and an exact-range match exists replay->>Disk: read_materialized_window(cache_path) + else --cache given and a cached window is a strict superset (TASK-050) + replay->>replay: _find_superset_window(cache_dir, from_frame, to_frame) + replay->>Disk: read_materialized_window(superset_path) + replay->>replay: slice out [from_frame, to_frame] -- no re-simulation else materialize fresh replay->>replay: find_checkpoint_at_or_before(checkpoints_dir, from_frame) replay->>checkpoint: read_checkpoint() + restore_simulation_state() @@ -499,13 +521,23 @@ sequenceDiagram end replay-->>playback: MaterializedWindow (config, frames[]) playback->>Window: RenderWindow(config.rendering), build_vector_field_arrows(frames[0]) + opt config declares fields (TASK-051) + playback->>Window: field_visualization.panel_colors/build_panel_legend per panel + end + playback->>Window: scrub bar track + thumb (gfx.Line + gfx.Points) Window-->>Window: window.playback_state = PlaybackState() playback->>Window: canvas.add_event_handler(_on_key, "key_down") + playback->>Window: canvas.add_event_handler(pointer_down/move/up) -- scrub drag (TASK-048) loop each real draw - Window->>playback: on_frame() - playback->>playback: advance_playback_position() -- position += speed unless paused + alt a seek event arrived (TASK-048) + Window->>playback: _on_key ArrowLeft/Right -> seek_relative, Home/End -> seek_to + Window->>playback: pointer drag on the bar -> frame_index_from_fraction -> seek_to + else no input + Window->>playback: on_frame() + playback->>playback: advance_playback_position() -- position += speed unless paused + end opt materialized frame index changed - playback->>Window: remove old arrows, build_vector_field_arrows(frames[index]) + playback->>Window: rebuild arrows, panels and thumb from frames[index] end end ``` @@ -517,20 +549,43 @@ window` (what `play()` actually calls) is the only place a cache is read or written, and only when `--cache DIR` is given. Nothing is written to disk by a bare `pyflow play`. -**Scoped to solved-velocity-only rendering for this first cut** -- +**Arrows and declared-field panels, from the same materialized frame.** `playback.py` builds `gfx` arrows from `MaterializedWindow.frames[i]`'s `velocity.0`/`velocity.1` tensors the same way `bootstrap.py`'s own -`_add_solved_velocity_rendering` does from a live `SimulationState`, but -has no declared-field/scalar-colormap path yet -(`UnsupportedPlaybackConfigError` otherwise) -- see `src/pyflow/ -CLAUDE.md`'s own `playback.py` entry for the full reasoning and the -scene-rebuild-cost measurements that shaped the speed mechanism -(`position += speed`, not more draws per second). - -**Every subsection in this section is now built.** `sequences.md`'s own -Maintenance note, below, no longer names a task to re-read this file -for -- update it again the next time Stage 8 gains a fourth piece, or -whenever any task named in this section is touched. +`_add_solved_velocity_rendering` does from a live `SimulationState`, and +-- since TASK-051 -- colours any declared field's panels from that same +frame, through the `rendering/field_visualization.py` helpers +`bootstrap.py`'s own live path calls rather than a second copy of them. +A config with **no** solved velocity at all is still rejected +(`UnsupportedPlaybackConfigError`); see `src/pyflow/CLAUDE.md`'s own +`playback.py` entry for the full reasoning and the scene-rebuild-cost +measurements that shaped the speed mechanism (`position += speed`, not +more draws per second). + +**Seeking is bounded by the window loaded at launch.** `seek_relative`/ +`seek_to` clamp to `[0, max_index]` of the materialized window, so +Home/End reach that window's own edges and not the recording's -- +reaching anything outside it still needs a fresh `pyflow play` with +different `--from-frame`/`--to-frame`. A stated Stage 8 scope boundary, +not an oversight: `docs/planning/roadmap.md`'s own Completion Criterion +6 records it, and `pyflow play --help` tells a user so directly. + +**Every subsection in this section is built.** Seven Stage 8 tasks are +covered here now: TASK-045 (recording, `resume`), TASK-049 (retention), +TASK-046 (windowed replay) and TASK-050 (subset cache reuse) above, +TASK-047 (playback) and TASK-048 (live scrub) and TASK-051 (declared +fields in playback) in this subsection. + +**This subsection described only TASK-046/047 until 2026-09-11**, and +one of its paragraphs ("scoped to solved-velocity-only rendering for +this first cut... no declared-field/scalar-colormap path yet") had been +false since TASK-051 landed on 2026-09-09. It was found by that stage's +own exit audit rather than by the re-read this file's own closing note +below asked for -- the note said to "update it again the next time +Stage 8 gains a fourth piece", Stage 8 then gained four pieces, and +nothing re-read it. **A `Checked-by: stage-boundary` declaration is a +promise about the stage boundary, not about the tasks in between**, +which is exactly how four tasks landed without touching this file. --- @@ -653,3 +708,30 @@ playback are both real, built sequences in Section 3 now, not Planned in substance under a different heading; that section's own closing note says so and names no further task, since Stage 8 has nothing left undrafted to anchor to. + +**And then the mechanism failed a second time, in the one way the +paragraphs above did not anticipate: no anchor existed to fail.** +Stage 8 was reopened on 2026-09-09 and gained four more tasks +(TASK-048/049/050/051 -- live scrub, checkpoint retention, subset cache +reuse, combined declared-field playback). All four landed without this +file being touched, and one of Section 3's own paragraphs was left +saying playback had "no declared-field/scalar-colormap path yet" two +days after TASK-051 built exactly that. Found 2026-09-11 by the Stage 8 +exit audit; Section 3 now covers all seven tasks. + +**Why the existing lesson did not reach it.** Every mechanism described +above keys off a *named task* in this file -- "grep this file's own +TASK-NNN mentions the next time any named task is touched". TASK-048-051 +were new tasks this file had never named, so there was nothing to grep +and nothing to notice. The paragraph that should have caught them was +the closing note's own "update it again the next time Stage 8 gains a +fourth piece", which is a standing instruction addressed to nobody in +particular and fired for no one. + +**So the rule this file actually needs is the one its own header already +declares:** `Checked-by: stage-boundary` means this document is re-read +when a stage opens or closes, and a stage that *reopens* is a stage +boundary twice over. The reopening on 2026-09-09 was the moment to +re-read this file, and the audit two days later was the second. Neither +a task anchor nor a "next time" note substitutes for that, because both +depend on a reader who already knows to look here. diff --git a/docs/planning/roadmap.md b/docs/planning/roadmap.md index 7ddfcf3..0f3f885 100644 --- a/docs/planning/roadmap.md +++ b/docs/planning/roadmap.md @@ -306,7 +306,20 @@ This paragraph previously said `make install` and `make test` were still expected to fail, pending `uv.lock` and a test suite (B2/C1) -- stale since 2026-08-16 and corrected 2026-08-19. Both now succeed: `uv.lock` is committed (B2) and `make test` runs the suite with coverage -(C1a/C1b): **1200 tests as of 2026-09-09**, up from 1199 the same day +(C1a/C1b): **1209 tests as of 2026-09-11**, up from 1200 on 2026-09-09 +(the Stage 8 exit audit, closing the gap between three Completion +Criteria and what actually checked them: 4 in +`tests/integration/test_import_order.py` (Criterion 1's own "imports +neither `rendering` nor anything that transitively imports it", which +that criterion's own text admitted had only ever been "checked by hand +at implementation time"), 1 in `tests/unit/test_recording.py` and 1 in +`tests/integration/test_record_cli.py` (Criterion 8's own +"multi-hundred-frame" run, and the peak-disk bound that makes retention +mean anything on a long run), 2 in +`tests/integration/test_playback_cli.py` (Criterion 6's own "each +change `window.renderer.snapshot()`'s content", which neither scrub +test had ever called), and 1 more there for `pyflow play --help`'s own +controls block), 1200 itself up from 1199 on 2026-09-09 (`_add_declared_field_transport` also drawing the solved velocity as arrows when `config.simulation.velocity_solved` is set, alongside its declared fields' own panels -- the live `pyflow run` counterpart to @@ -11346,6 +11359,22 @@ them, which had not been drafted yet when these were written. a different `pyflow play` invocation, exactly as today. Extending scrub to seek beyond the loaded window is real, deferred future work, not built here. + - **Standing exclusion, reaffirmed at the 2026-09-11 exit audit and + recorded here rather than left to a task's Design decisions.** + This stage's own Goal promises playback "paused, **scrubbed to any + point**, and watched at a different speed" -- and "any point" + still means any point *in the loaded window*, not in the + recording. The gap is deliberate and the stage closes over it + knowingly, which is a different thing from the 2026-09-09 + reopening, where the same Goal clause had no criterion at all and + nobody had noticed. **Closing a stage over a Goal clause it only + partly satisfies is a decision, so it is written where a decision + goes**: the maintainer was asked directly at the exit audit and + chose to defer rather than build it. If it is built later it needs + a new task and a new criterion, not a quiet widening of this one. + A user meets the boundary in `pyflow play --help`'s own controls + block ("Seeking covers the loaded window only"), which is the one + place it stops reading as a bug. 7. **A config combining a solved velocity field with one or more declared fields plays back correctly, not only a solved-velocity-only config.** TASK-047's own stated scope boundary @@ -11445,14 +11474,14 @@ what shipped rather than against the criteria that were meant to operationalise | Criterion | Verdict | |-----------|---------| -| 1. Recording never opens a rendering window | **Met** -- TASK-045 | +| 1. Recording never opens a rendering window | **Met** -- TASK-045. Verdict unchanged, evidence changed: true all along, but gated only from 2026-09-11 (exit audit) | | 2. A recording's disk footprint is bounded | **Met** -- TASK-045 | | 3. Resuming reproduces the same trajectory, bit-identically | **Met** -- TASK-045, mutation-tested | | 4. A checkpoint file is self-contained | **Met** -- TASK-045 | | 5. Golden Demo runs end to end, both halves | **Met** -- TASK-045 (record), TASK-046/047 (playback), against Lid-Driven Cavity | -| 6. Live scrub, keyboard and mouse | **Met** -- TASK-048, verified against a real window | +| 6. Live scrub, keyboard and mouse | **Met, 2026-09-11** -- TASK-048 built it and proved it against `PlaybackState`; the criterion's own rendered-pixel clause was discharged by the exit audit, mutation-tested | | 7. Combined solved-velocity + declared-field playback | **Met** -- TASK-051, verified against a real Smoke Transport run | -| 8. Checkpoint retention, opt-in, frame 0 never pruned | **Met** -- TASK-049, mutation-tested | +| 8. Checkpoint retention, opt-in, frame 0 never pruned | **Met, 2026-09-11** -- TASK-049, mutation-tested; the criterion's own "multi-hundred-frame" clause discharged by the exit audit | | 9. Partial-overlap (subset) cache reuse | **Met** -- TASK-050, mutation-tested | **All nine criteria are met; the stage is complete again, reclosed @@ -11476,6 +11505,110 @@ directly) would have violated `field_visualization.py`'s own standing "owns no window" rule if moved as-is -- see that task's own Design decision 3. +### Exit audit, 2026-09-11 + +**Run at the maintainer's request, against a stage already marked +complete twice.** `make ci` was green throughout -- 1200 tests, 144 +scenarios, 99% coverage -- and three of the nine criteria were marked +Met against checks that did not test what the criterion said. None of +the three needed judgement to find: each is a sentence the criterion had +already written down, in a criterion somebody had already read. This is +`docs/practices.md`'s own "An exit audit reads each criterion to its +last sentence", recurring one stage after it was written down as a +standing rule, and now recurring on a stage whose *previous* audit is +the reason Criteria 6-9 exist at all. + +**What the three were, and what closed them.** + +1. **Criterion 6's rendered-pixel clause was checked by nothing.** The + criterion says "A keyboard seek and a mouse drag each change + `window.renderer.snapshot()`'s content". Neither + `test_arrow_and_home_end_keys_seek_playback_live` nor + `test_dragging_the_scrub_bar_seeks_without_panning_the_camera` calls + `snapshot()` at all; both assert on `PlaybackState.position`, and + the first one's own docstring says so. Closed by two new tests + (`test_keyboard_seeking_rerenders_the_field_in_real_pixels`, + `test_dragging_the_scrub_bar_rerenders_the_field_in_real_pixels`). +2. **Criterion 8 named a scale nothing had ever run.** "Checked + directly against a real multi-hundred-frame `pyflow record` run" -- + the CLI test used `--max-frames 20`, and TASK-049's own by-hand + verification used 25. The behaviour was right; the evidence was two + orders of magnitude short of the sentence. Closed by a real + 300-frame CLI run and, separately, by pinning the peak on-disk count + during the run, which is the claim `_advance_and_checkpoint`'s own + docstring makes ("as the recording grows, not only once it + finishes") and which the final file set cannot prove either way. +3. **Criterion 1 rested on a by-hand check, and said so.** Its own text + admits "the stronger claim was checked by hand at implementation + time", and names `test_import_order.py` while noting that test "does + not by itself prove the absence of a `rendering` import". It was + true -- re-verified by hand during this audit -- and one added import + would have silently falsified it. Closed by a fresh-subprocess gate + over the transitive closure, for all four headless modules. + +**Every one of the three fixes was mutation-tested before being trusted +green**, and the Criterion 6 pair took three attempts to get teeth, +which is the most useful thing this audit produced: + +- *"The pixels changed after seeking"* proves nothing -- the scrub thumb + moves with the index, so any seek repaints something whether or not + the field was rebuilt. +- *"A seek-reached frame matches an autoplay-reached frame at the same + index"* -- the obvious repair -- **also passed against a deliberately + frozen field**, because both sides of that comparison come from the + same run and were equally frozen. A reference computed by the run + under test cannot detect a fault common to the whole run. +- What works: an independent reference (a separate `play()` window + launched *at* the target frame, where that frame is index 0 and is + drawn by the initial scene build, never by the seek path) compared + over the field region only (cropping away the scrub bar). Both halves + then fail correctly -- against a frozen field, and against an + off-by-one field, checked separately. + +The second bullet is the transferable lesson and is recorded as a rule +in `docs/practices.md`: **a test whose expected value is produced by +the same run it is checking proves consistency, not correctness.** + +**Documentation the stage invalidated and never updated.** Three +findings, all in files no Stage 8 task opened -- the shape +`docs/practices.md`'s "A stage's documentation sweep is a grep, not a +diff review" already names: + +- `docs/architecture/CLAUDE.md` said Stage 8 was complete with "all + five" criteria met, written 2026-09-07 and falsified two days later by + the reopening that added four more. +- `docs/repository-manifest.md`'s `sequences.md` row said "Deterministic + replay and playback (TASK-046/047) are still unbuilt" -- stale from + the day it was written, since both landed 2026-09-07 alongside the + TASK-045 the same sentence credits. +- `docs/architecture/sequences.md`, a `Checked-by: stage-boundary` + document, described only TASK-045/046/047. All four reopening tasks + landed without it being touched, and one paragraph still said playback + had "no declared-field/scalar-colormap path yet" two days after + TASK-051 built exactly that. Its own closing note had asked to "update + it again the next time Stage 8 gains a fourth piece"; Stage 8 gained + four pieces and nobody re-read it. See that file's own Maintenance + section for why neither a task anchor nor a "next time" note reaches a + task the file has never named. + +**Three smaller things fixed on the way past.** `pyflow play --help` +documented none of the controls the subcommand exists for -- pause, +speed, scrub -- so `README.md` was the only place to find them; Linux CI +had no `DISPLAY` and no `xvfb`, so all 8 display-guarded tests (every +check Criterion 6 rests on) skipped there while passing on Windows, +making a green two-platform matrix prove live-window behaviour on one +platform; and 19 stray `checkpoint_*.pt` files from a 2026-09-07 manual +run sat untracked in the repository root, which `.gitignore`'s +`checkpoints/` rule never matched. + +**What this audit deliberately did not do.** The Goal's own "scrubbed +to any point" is still satisfied only within the window one `pyflow +play` invocation loads -- see the standing exclusion recorded under +Completion Criterion 6 below. That is now stated in three places a user +or a future contributor will actually meet it (the criterion, `pyflow +play --help`, and `sequences.md`), rather than only in a task's Design +decisions, which is where it was hiding when this audit started. + --- ## TASK-045 — Periodic Checkpointing (Headless Recording) diff --git a/docs/planning/status.md b/docs/planning/status.md index 9e2533e..cd5c08b 100644 --- a/docs/planning/status.md +++ b/docs/planning/status.md @@ -46,7 +46,7 @@ pie showData ## Live repository facts - **49** `CLAUDE.md` files -- **1200** tests collected +- **1209** tests collected - **144** Gherkin scenarios (`tests/features/*.feature`) ## Stages diff --git a/docs/practices.md b/docs/practices.md index 5efe8d0..e552e6e 100644 --- a/docs/practices.md +++ b/docs/practices.md @@ -1493,6 +1493,55 @@ not reading the second half of three sentences" -- restated as a rule rather than as a stage's own observation, because describing it once did not stop it recurring at a higher count one stage later. +## A reference the run under test produced proves consistency, not correctness + +**Standing rule, 2026-09-11, from the Stage 8 exit audit.** That audit +had to write the same test three times, and the second version is the +one worth recording, because it looked right, passed, and proved +almost nothing. + +The criterion asked that a scrub seek "change `window.renderer. +snapshot()`'s content". The obvious test -- *did the pixels change after +seeking?* -- is worthless here: the scrub thumb moves with the frame +index, so any seek repaints something whether or not the field +underneath it was ever rebuilt. The obvious repair was to compare the +seek-reached frame against an **autoplay-reached** frame at the same +index, so the thumb is in the same place on both sides and only the +field can differ. + +**That version passed against a deliberately frozen field.** Both sides +of the comparison were rendered by the same run, so freezing the field +froze the reference too, and the two matched exactly as before. The test +was not checking that the field was right; it was checking that the +field was *the same on both paths*, which a global fault satisfies +trivially. + +The repair that works has two parts, and both were needed: + +1. **The reference comes from outside the run under test.** A separate + `play()` window launched *at* the target frame, where that frame is + index 0 and is therefore drawn by the initial scene build -- not by + the seek path, not by the advance path, not by anything the test is + trying to exercise. +2. **The comparison excludes whatever moves for a reason other than + the one being tested.** Here that is the scrub bar, cropped out by + hashing only the rows above the mesh's own bottom edge, so the + thumb cannot reintroduce the confound from the first version. + +**So, when writing a test whose expected value is computed rather than +written down: ask what produced the expected value, and whether the +fault you are hunting would have corrupted it too.** If the answer is +yes, the test can only ever prove the two paths agree. Mutation-testing +is what surfaces this and nothing else reliably does -- the frozen-field +mutation took under a minute to apply and was the only reason the +second version was not merged looking green and meaning nothing. + +This is a sharper form of the repository's existing mutation-testing +discipline. That discipline says *run the test against a broken +implementation before trusting it green*. This rule says what to do +with the answer when the test passes anyway: the problem is usually not +the assertion, it is where the expected value came from. + ## A gap recorded in a `CLAUDE.md` is not recorded against a criterion **Standing rule, 2026-08-29, from the Stage 5 exit audit.** That stage diff --git a/docs/repository-manifest.md b/docs/repository-manifest.md index f4af87f..4adae08 100644 --- a/docs/repository-manifest.md +++ b/docs/repository-manifest.md @@ -154,7 +154,7 @@ Not present, deferred consciously rather than overlooked: | overview.md | 🟩 | Top-level system map -- no KA entry; legitimate but unspecified | | rendering.md | 🟩 | Architecture of the adopted renderer -- no KA entry | | repository.md | 🟩 | Repository architecture -- no KA entry | -| sequences.md | 🟩 | Time-ordered runtime sequences (setup, timestep loop, data flow, rendering) as Mermaid diagrams -- no KA entry. Its last `Planned` subsection (checkpointing, Section 3) was built 2026-09-07 (TASK-045, Stage 8); Section 2's live-loop wiring went real earlier, 2026-08-28 (TASK-030). Deterministic replay and playback (TASK-046/047) are still unbuilt, but Section 3 now says so in prose rather than under a `Planned` heading. | +| sequences.md | 🟩 | Time-ordered runtime sequences (setup, timestep loop, data flow, rendering) as Mermaid diagrams -- no KA entry. Its last `Planned` subsection (checkpointing, Section 3) was built 2026-09-07 (TASK-045, Stage 8); Section 2's live-loop wiring went real earlier, 2026-08-28 (TASK-030). Deterministic replay and playback (TASK-046/047) were built the same day as TASK-045, and Section 3 covers all three; live scrub, checkpoint retention, subset cache reuse and combined declared-field playback (TASK-048/049/050/051) were added to it 2026-09-11. (This cell read "TASK-046/047 are still unbuilt" until 2026-09-11 -- stale from the day it was written, since both landed 2026-09-07 alongside the TASK-045 the same sentence credits; found by the Stage 8 exit audit.) | | compute-and-rendering-stack.md | 🟨 | Survey and compatibility matrix for array-library × renderer combinations; decision-support for the stack ADRs. Both questions it exists to support are decided: the class (A2b) via `ADR-004`, the instances (A2c, PyTorch + wgpu/pygfx) via `ADR-005`, both 2026-08-15. It remains the record of why, and of the options not taken. (This row read "not yet decided" for A2c until 2026-08-18 -- stale since the day it was written, since `ADR-005` landed the same day.) | `engine.md` and `icds.md` written 2026-08-17 (`docs/planning/backlog.md` diff --git a/src/pyflow/__main__.py b/src/pyflow/__main__.py index 221f312..ce30826 100644 --- a/src/pyflow/__main__.py +++ b/src/pyflow/__main__.py @@ -363,7 +363,25 @@ def main(argv: list[str] | None = None) -> None: "play", help="Open a window and render a checkpointed run between two " "frames, with live Space to pause/resume and +/- to change speed.", + # The controls block was **added 2026-09-11 by the Stage 8 exit + # audit**: pause, speed and scrub are what this subcommand is + # for, and `--help` listed none of them, so the only place a + # user could discover them was `README.md`. Seeking is bounded by + # the window `--from-frame`/`--to-frame` loaded, which the last + # line says outright rather than leaving a user to infer from + # Home/End not going any further. epilog=( + "window controls:\n" + " Space pause / resume\n" + " + / - double / halve playback speed\n" + " Left / Right step one frame back / forward\n" + " Home / End jump to the first / last loaded frame\n" + " drag scrub bar seek directly to any loaded frame\n" + " Escape / Enter close the window\n" + "\n" + " Seeking covers the loaded window only -- to reach frames\n" + " outside it, rerun with different --from-frame/--to-frame.\n" + "\n" "examples:\n" " pyflow play --checkpoints-dir checkpoints --to-frame 500\n" " pyflow play --checkpoints-dir checkpoints --from-frame 100 " diff --git a/tests/integration/CLAUDE.md b/tests/integration/CLAUDE.md index 4a83d48..d60a3c2 100644 --- a/tests/integration/CLAUDE.md +++ b/tests/integration/CLAUDE.md @@ -73,3 +73,37 @@ short-circuits by making a monkeypatched `GlfwRenderCanvas` raise a bare `BaseException` that `except Exception` would not catch -- standing in for the real SIGABRT without actually crashing the test runner to prove it. + +**`test_import_order.py` gained a second, stronger check 2026-09-11** +(Stage 8 exit audit): `test_headless_module_never_reaches_the_renderer`. +The original test proves each module *imports cleanly* first, which is +the D4 circular-import bug it was written for -- it would pass just as +happily with `import pyflow.rendering` at the top of `recording.py`. +Stage 8's own Completion Criterion 1 needs the opposite claim (the +headless modules **cannot reach** the renderer, transitively), so the +new test reads `sys.modules` back out of the same fresh subprocess and +fails on any `pyflow.rendering`/`pygfx`/`rendercanvas`/`wgpu`/`glfw` +entry. **Add a module to `HEADLESS_MODULES` whenever a new module +belongs on the headless side**, the same standing obligation `MODULES` +above it already carries. + +**The display-guarded tests run on Linux CI as of 2026-09-11**, under +`xvfb-run` (`.github/workflows/ci.yml`). Before that they skipped there +and ran only on Windows, which meant a green two-platform matrix was +proving live-window behaviour on one platform -- found by the Stage 8 +exit audit, since every check that stage's Completion Criterion 6 rests +on is display-guarded. The `_display_available()` probe is unchanged and +still short-circuits before GLFW is constructed when there is no +`DISPLAY`; `xvfb` simply means there now is one. + +**Comparing rendered pixels: never build the reference from the run +under test.** `test_playback_cli.py`'s two `*_rerenders_the_field_in_ +real_pixels` tests are the worked example, and their module comments +record the two versions that passed against a deliberately broken +renderer before the third one worked. The short form: a reference frame +captured during the same `play()` window is frozen by exactly the faults +you are hunting, so it can only prove the two code paths agree with each +other. Take the reference from a separate window launched at the target +frame, and crop out anything that moves for an unrelated reason (here, +the scrub bar). Full rule: `docs/practices.md`, "A reference the run +under test produced proves consistency, not correctness". diff --git a/tests/integration/test_import_order.py b/tests/integration/test_import_order.py index 1ceacc0..72e1a4e 100644 --- a/tests/integration/test_import_order.py +++ b/tests/integration/test_import_order.py @@ -49,3 +49,56 @@ def test_module_imports_cleanly_first(module: str) -> None: check=False, ) assert result.returncode == 0, result.stderr + + +# Stage 8 Completion Criterion 1 (`docs/planning/roadmap.md`): the +# headless-recording modules must not be able to reach the renderer at +# all -- not merely default to headless. TASK-046's own Acceptance +# Criteria state the same claim for `replay.py`. +# +# **Added 2026-09-11 by the Stage 8 exit audit.** Both criteria were +# true and neither was gated: Criterion 1 said in its own text that the +# claim "was checked by hand at implementation time", and +# `test_module_imports_cleanly_first` above proves only that each module +# imports without error -- it would pass just as happily with +# `import pyflow.rendering` at the top of `recording.py`. A by-hand +# check does not survive the next import somebody adds, which is exactly +# what this criterion exists to prevent. +HEADLESS_MODULES = [ + "pyflow.checkpoint", + "pyflow.recording", + "pyflow.replay", + "pyflow.simulation_run", +] + +# `pyflow.rendering` itself, plus the three third-party layers underneath +# it -- a module could pull in `pygfx`/`wgpu`/`glfw` directly without +# going through `pyflow.rendering`, and that would break this criterion +# just as thoroughly. +_RENDERING_PREFIXES = ("pyflow.rendering", "pygfx", "rendercanvas", "wgpu", "glfw") + + +@pytest.mark.parametrize("module", HEADLESS_MODULES) +def test_headless_module_never_reaches_the_renderer(module: str) -> None: + """A fresh subprocess, so `sys.modules` reflects this import alone. + + Checks the *transitive* closure, not the module's own import + statements: anything `recording.py` imports that itself imports + `rendering` would show up here, which is the form the criterion is + actually written in ("nor anything that transitively imports it"). + """ + probe = ( + f"import sys; import {module}; " + f"print([m for m in sys.modules if m.startswith({_RENDERING_PREFIXES!r})])" + ) + result = subprocess.run( + [sys.executable, "-c", probe], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0, result.stderr + assert result.stdout.strip() == "[]", ( + f"{module} transitively imported a rendering module: {result.stdout.strip()}" + ) diff --git a/tests/integration/test_playback_cli.py b/tests/integration/test_playback_cli.py index 3a73023..f298850 100644 --- a/tests/integration/test_playback_cli.py +++ b/tests/integration/test_playback_cli.py @@ -76,6 +76,48 @@ def _record_cavity(output_dir: Path, *, max_frames: int, checkpoint_interval: in assert result.returncode == 0, result.stderr +def _record_cavity_without_stats( + tmp_path: Path, output_dir: Path, *, max_frames: int, checkpoint_interval: int +) -> None: + """Record the same cavity, but with `rendering.show_stats: false`. + + **Added 2026-09-11 by the Stage 8 exit audit**, for the pixel-level + scrub check below. The stats HUD rewrites the frame number into the + rendered image every single frame, so with it on, *any* two frames + at different positions hash differently whether or not the field + itself was ever rebuilt -- a full-frame hash would pass vacuously. + Turning it off leaves the field, the arrows and the scrub thumb as + the only things in the image, which is what the criterion is + actually about. + """ + config_text = Path("examples/golden-demos/lid_driven_cavity.yaml").read_text(encoding="utf-8") + config_file = tmp_path / "cavity_no_stats.yaml" + # `rendering:` is the last section in the golden demo, so appending + # one more key to it needs no YAML round trip. + config_file.write_text(config_text + " show_stats: false\n", encoding="utf-8") + + result = subprocess.run( + [ + sys.executable, + "-m", + "pyflow", + "record", + "--config", + str(config_file), + "--max-frames", + str(max_frames), + "--output-dir", + str(output_dir), + "--checkpoint-interval", + str(checkpoint_interval), + ], + capture_output=True, + text=True, + check=False, + ) + assert result.returncode == 0, result.stderr + + def test_play_renders_a_real_recorded_run_headlessly(tmp_path: Path) -> None: checkpoints_dir = tmp_path / "checkpoints" _record_cavity(checkpoints_dir, max_frames=20, checkpoint_interval=20) @@ -505,3 +547,270 @@ def _on_frame(window: RenderWindow) -> None: # `stop_propagation` kept `RenderWindow.run`'s pan handlers from ever # starting a pan for this gesture. assert found["camera_after"] == found["camera_before"] + + +# Stage 8 Completion Criterion 6's second bullet asks for the two scrub +# paths checked "against real rendered pixels, the same way Space's own +# pause already is". **The three helpers and two tests below were added +# 2026-09-11 by the Stage 8 exit audit**, which found that bullet marked +# Met against two tests (above) that assert only on `PlaybackState. +# position` and never call `snapshot()` at all. +# +# **Two false starts are recorded here because each one passed against a +# deliberately broken renderer, and the shape of the mistake is the +# useful part.** +# +# 1. *"The pixels changed after seeking"* proves nothing: the scrub +# thumb moves with the index, so any index change repaints something +# whether or not the field was rebuilt underneath it. +# 2. *"A seek-reached frame matches an autoplay-reached frame at the +# same index"* -- the obvious repair -- proves almost nothing either, +# and this one actually had to be mutation-tested to see it. Freezing +# the field (`_rebuild_arrows(0)`/`_rebuild_panels(0)`, thumb left +# working) leaves *both* sides of that comparison equally frozen, so +# they still match. A reference computed by the same run cannot +# detect a fault common to the whole run. +# +# So the reference comes from **a separate `play()` window launched at +# the target frame** (`_reference_field_hash`), where that frame is +# index 0 and therefore rendered by the initial scene build rather than +# by the seek path at all -- and the comparison is over the **field +# region only** (`_field_region_hash`), cropping away the scrub bar, +# which `playback.py` draws below `mesh_min_y` and which would otherwise +# reintroduce exactly the thumb-moved-so-pixels-changed confound from +# (1). Both tests were re-run against the same frozen-field mutation and +# confirmed to fail before being trusted green. +# +# One timing fact the helpers depend on: **`snapshot()` is one frame +# behind `position`.** `play()`'s own `_on_frame` advances the position, +# rebuilds the scene, and only then invokes this callback -- all before +# the canvas presents anything -- so the image read back while +# `position` reads N is the rendering of frame N-1. Established +# empirically by logging `(frame_count, position, hash)` against a plain +# autoplay run, not inferred from reading `playback.py`. +def _field_region_hash(window: RenderWindow, mesh_bounds: tuple[float, float, float, float]) -> str: + """Hash only the rows of `snapshot()` above the mesh's own bottom + edge -- the field and its arrows, never the scrub bar. + + `playback.py` puts the bar at `mesh_min_y - mesh_height * 0.08`, so + cropping at `mesh_min_y` excludes the bar and its thumb entirely + while keeping every pixel the field itself can touch. + """ + logical_width, logical_height = window.canvas.get_logical_size() + mesh_min_x, mesh_min_y, _mesh_max_x, _mesh_max_y = mesh_bounds + _screen_x, screen_y = _world_to_screen( + window.camera, logical_width, logical_height, mesh_min_x, mesh_min_y + ) + + image = np.asarray(window.renderer.snapshot()) + # `snapshot()` is in physical pixels and `_world_to_screen` returns + # logical ones, so convert through a fraction of the height rather + # than assuming the two are the same scale. + cut = int(round((screen_y / logical_height) * image.shape[0])) + cut = max(1, min(cut, image.shape[0])) + return _frame_hash(image[:cut]) + + +def _reference_field_hash( + checkpoints_dir: Path, frame: int, mesh_bounds: tuple[float, float, float, float] +) -> str: + """The field region of absolute `frame`, rendered by a *separate* + `play()` window that starts there. + + In that window `frame` is index 0, so it is drawn by the initial + scene build and never by a seek or an advance -- which is what makes + it an independent reference rather than a second reading of the same + possibly-broken path. + """ + captured: dict[str, str] = {} + + def _on_frame(window: RenderWindow) -> None: + if "hash" not in captured: + captured["hash"] = _field_region_hash(window, mesh_bounds) + window.canvas.submit_event({"event_type": "key_down", "key": "Escape"}) + + play( + checkpoints_dir, + from_frame=frame, + to_frame=frame + 1, + backend="glfw", + max_frames=_SCRUB_SAFETY_MAX_FRAMES, + on_frame=_on_frame, + ) + assert "hash" in captured, "reference window never rendered a frame" + return captured["hash"] + + +@_needs_a_real_display +def test_keyboard_seeking_rerenders_the_field_in_real_pixels(tmp_path: Path) -> None: + """ArrowLeft, through the real key wiring, against a real window.""" + checkpoints_dir = tmp_path / "checkpoints" + _record_cavity_without_stats(tmp_path, checkpoints_dir, max_frames=30, checkpoint_interval=30) + checkpoint = read_checkpoint(checkpoints_dir / "checkpoint_00000000.pt") + mesh_bounds = mesh_bounding_box(StructuredCartesianMesh.from_config(checkpoint.config.mesh)) + + step = _SCRUB_STEP_FRAMES + seen: dict[str, object] = {} + injected: set[str] = set() + + def _on_frame(window: RenderWindow) -> None: + assert window.playback_state is not None + index = int(window.playback_state.position) + frame = window.frame_count + + if frame == step and "pause" not in injected: + injected.add("pause") + window.canvas.submit_event({"event_type": "key_down", "key": " "}) + elif frame == 2 * step and "left" not in injected: + seen["paused_index"] = index + seen["paused_hash"] = _field_region_hash(window, mesh_bounds) + injected.add("left") + window.canvas.submit_event({"event_type": "key_down", "key": "ArrowLeft"}) + elif frame == 3 * step: + # One frame behind `position`, so the settled image here + # depicts `index`, which has not moved for a whole step. + seen["sought_index"] = index + seen["sought_hash"] = _field_region_hash(window, mesh_bounds) + window.canvas.submit_event({"event_type": "key_down", "key": "Escape"}) + + window = play( + checkpoints_dir, + from_frame=0, + to_frame=30, + backend="glfw", + max_frames=_SCRUB_SAFETY_MAX_FRAMES, + on_frame=_on_frame, + ) + + assert window.canvas.get_closed() + assert injected == {"pause", "left"}, injected + + paused_index = seen["paused_index"] + sought_index = seen["sought_index"] + assert isinstance(paused_index, int) and isinstance(sought_index, int) + assert sought_index == paused_index - 1, (paused_index, sought_index) + + # The weak half: the rendered field moved at all. Already stronger + # than a whole-frame hash, since the thumb is cropped out. + assert seen["sought_hash"] != seen["paused_hash"], ( + "ArrowLeft left the rendered field byte-identical" + ) + # The strong half: it is the genuine rendering of that frame, + # according to a window that never seeked. + assert seen["sought_hash"] == _reference_field_hash( + checkpoints_dir, sought_index, mesh_bounds + ), ( + f"ArrowLeft moved position to {sought_index} but rendered a different field " + "than a fresh window started at that frame" + ) + + +@_needs_a_real_display +def test_dragging_the_scrub_bar_rerenders_the_field_in_real_pixels(tmp_path: Path) -> None: + """The same check for the mouse path, through real pointer events.""" + checkpoints_dir = tmp_path / "checkpoints" + _record_cavity_without_stats(tmp_path, checkpoints_dir, max_frames=30, checkpoint_interval=30) + checkpoint = read_checkpoint(checkpoints_dir / "checkpoint_00000000.pt") + mesh_bounds = mesh_bounding_box(StructuredCartesianMesh.from_config(checkpoint.config.mesh)) + mesh_min_x, _mesh_min_y, mesh_max_x, _mesh_max_y = mesh_bounds + mesh_width = mesh_max_x - mesh_min_x + + step = _SCRUB_STEP_FRAMES + max_index = 30 + target_index = 21 + + seen: dict[str, object] = {} + injected: set[str] = set() + + def _on_frame(window: RenderWindow) -> None: + assert window.playback_state is not None + index = int(window.playback_state.position) + frame = window.frame_count + logical_width, logical_height = window.canvas.get_logical_size() + + if "bar_y" not in seen: + thumb = next(child for child in window.scene.children if isinstance(child, gfx.Points)) + _thumb_x, bar_y, _z = thumb.geometry.positions.data[0] + seen["bar_y"] = float(bar_y) + + bar_y = seen["bar_y"] + assert isinstance(bar_y, float) + target_x = mesh_min_x + (target_index / max_index) * mesh_width + + if frame == step and "pause" not in injected: + injected.add("pause") + window.canvas.submit_event({"event_type": "key_down", "key": " "}) + elif frame == 2 * step and "down" not in injected: + seen["paused_hash"] = _field_region_hash(window, mesh_bounds) + injected.add("down") + screen_x, screen_y = _world_to_screen( + window.camera, logical_width, logical_height, target_x, bar_y + ) + seen["screen_pos"] = (screen_x, screen_y) + window.canvas.submit_event( + {"event_type": "pointer_down", "x": screen_x, "y": screen_y, "button": 1} + ) + elif frame == 3 * step and "up" not in injected: + injected.add("up") + screen_pos = seen["screen_pos"] + assert isinstance(screen_pos, tuple) + screen_x, screen_y = screen_pos + window.canvas.submit_event( + {"event_type": "pointer_up", "x": screen_x, "y": screen_y, "button": 1} + ) + elif frame == 4 * step: + seen["dragged_index"] = index + seen["dragged_hash"] = _field_region_hash(window, mesh_bounds) + window.canvas.submit_event({"event_type": "key_down", "key": "Escape"}) + + window = play( + checkpoints_dir, + from_frame=0, + to_frame=max_index, + backend="glfw", + max_frames=_SCRUB_SAFETY_MAX_FRAMES, + on_frame=_on_frame, + ) + + assert window.canvas.get_closed() + assert injected == {"pause", "down", "up"}, injected + assert seen["dragged_index"] == target_index, seen["dragged_index"] + + assert seen["dragged_hash"] != seen["paused_hash"], ( + "the scrub drag left the rendered field byte-identical" + ) + assert seen["dragged_hash"] == _reference_field_hash( + checkpoints_dir, target_index, mesh_bounds + ), ( + f"the drag moved position to {target_index} but rendered a different field " + "than a fresh window started at that frame" + ) + + +def test_play_help_documents_every_window_control() -> None: + """`pyflow play --help` must name the controls the subcommand exists + for, and say where seeking stops. + + **Added 2026-09-11 by the Stage 8 exit audit**, which found pause, + speed and scrub -- everything TASK-047 and TASK-048 built -- absent + from `--help` entirely, discoverable only by reading `README.md`. + Root `CLAUDE.md`'s Feature Verification rule asks for exactly this + check ("confirm every new CLI flag actually appears in `--help`"); + the controls are not flags, but they are the same promise. + + The bounded-seek line is asserted too, because it is the one place a + user is told that Home/End stop at the loaded window rather than at + the recording's own ends -- Stage 8's own stated scope boundary, and + the thing most likely to read as a bug without it. + """ + result = subprocess.run( + [sys.executable, "-m", "pyflow", "play", "--help"], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0, result.stderr + for control in ("Space", "+ / -", "Left / Right", "Home / End", "drag scrub bar"): + assert control in result.stdout, f"{control!r} missing from pyflow play --help" + assert "loaded window only" in result.stdout diff --git a/tests/integration/test_record_cli.py b/tests/integration/test_record_cli.py index ca25067..93e19e7 100644 --- a/tests/integration/test_record_cli.py +++ b/tests/integration/test_record_cli.py @@ -216,3 +216,48 @@ def test_resume_requires_checkpoint_or_config_and_max_frames() -> None: assert result.returncode != 0 assert "--checkpoint" in result.stderr + + +def test_record_prunes_a_multi_hundred_frame_run_through_the_real_cli(tmp_path: Path) -> None: + """Stage 8 Completion Criterion 8 asks for the retention policy + checked "against a real multi-hundred-frame `pyflow record` run, + not only against the pruning function in isolation". + + **Added 2026-09-11 by the Stage 8 exit audit**, which found that + clause marked Met against a 20-frame run here and a 25-frame one by + hand -- the substantive behaviour was right, but no run at the scale + the criterion names had ever happened. 300 frames at `interval=5` + writes 61 checkpoints, which is the scale at which "bounded total" + means something distinct from "bounded interval". + """ + output_dir = tmp_path / "checkpoints" + + result = subprocess.run( + [ + sys.executable, + "-m", + "pyflow", + "record", + "--config", + "examples/golden-demos/heat_diffusion.yaml", + "--max-frames", + "300", + "--output-dir", + str(output_dir), + "--checkpoint-interval", + "5", + "--max-checkpoints-retained", + "3", + ], + capture_output=True, + text=True, + check=False, + ) + + assert result.returncode == 0, result.stderr + # 61 written, 4 kept: frame 0 (never pruned) plus the newest three. + assert "recorded 61 checkpoint(s)" in result.stderr or "61 checkpoint(s)" in result.stdout + remaining = { + int(p.stem.removeprefix("checkpoint_")) for p in output_dir.glob("checkpoint_*.pt") + } + assert remaining == {0, 290, 295, 300} diff --git a/tests/unit/test_recording.py b/tests/unit/test_recording.py index 23a2ee3..bda4418 100644 --- a/tests/unit/test_recording.py +++ b/tests/unit/test_recording.py @@ -6,11 +6,12 @@ from __future__ import annotations from pathlib import Path +from typing import Any import pytest import torch -from pyflow.checkpoint import read_checkpoint +from pyflow.checkpoint import read_checkpoint, write_checkpoint from pyflow.recording import NothingToRecordError, NothingToResumeError, record, resume _DECLARED_FIELD_CONFIG = """\ @@ -378,3 +379,53 @@ def test_resume_rejects_both_checkpoint_path_and_config_path(tmp_path: Path) -> with pytest.raises(ValueError, match="checkpoint_path.*config_path"): resume(output_dir / "checkpoint_00000003.pt", config_path=config_file, max_frames=6) + + +def test_retention_bounds_disk_use_throughout_a_long_run_not_only_at_the_end( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """`_advance_and_checkpoint`'s own docstring claims the cap bounds + disk usage "as the recording grows, not only once it finishes" -- + which the final file set cannot prove either way, since pruning once + at the very end leaves exactly the same files behind. + + So this watches the directory *during* the run, after every single + checkpoint write, and pins the peak. **Added 2026-09-11 by the Stage + 8 exit audit**, which found that claim asserted in a docstring and + checked nowhere: a 300-frame run at `interval=5` writes 61 + checkpoints, so an implementation that pruned only at the end would + hold 61 files at its peak and still pass every other retention test + in this module. + """ + peak = 0 + + def _counting_write_checkpoint(path: Path, **kwargs: Any) -> None: + nonlocal peak + write_checkpoint(path, **kwargs) + peak = max(peak, len(list(path.parent.glob("checkpoint_*.pt")))) + + # Patched by name on `pyflow.recording`, which is where `record` + # resolves it -- patching `pyflow.checkpoint.write_checkpoint` + # instead would leave `recording.py`'s own from-import untouched. + monkeypatch.setattr("pyflow.recording.write_checkpoint", _counting_write_checkpoint) + + config_file = tmp_path / "config.yaml" + config_file.write_text(_DECLARED_FIELD_CONFIG) + output_dir = tmp_path / "checkpoints" + + result = record( + config_file, + max_frames=300, + output_dir=output_dir, + checkpoint_interval=5, + max_checkpoints_retained=3, + ) + + # 61 checkpoints genuinely written (frame 0 plus every multiple of 5 + # up to 300) -- so the peak below is a real bound being enforced + # mid-run, not an artefact of few files ever existing. + assert len(result.checkpoint_frames) == 61 + # Frame 0 (never pruned) + the cap of 3 + the one just written before + # pruning runs = 5. Never the 61 an end-only prune would reach. + assert peak <= 5, f"disk use peaked at {peak} checkpoints, not bounded during the run" + assert _checkpoint_frames_on_disk(output_dir) == {0, 290, 295, 300} From 02dd4e77ca0599bcb8bb2ed02e3790d3746b6551 Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Fri, 11 Sep 2026 14:08:17 +0100 Subject: [PATCH 2/5] Record the exit audit in README's own Stage 8 record paragraph That paragraph claims every edit to it has landed in the same change as the roadmap event it describes -- so the exit audit is an edit it owes. It also needed the distinction it was inviting a reader to miss: this section staying current says nothing about the three criteria and three documents that did not. Co-Authored-By: Claude Opus 5 --- README.md | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index 6dc47dd..681ecab 100644 --- a/README.md +++ b/README.md @@ -148,12 +148,25 @@ the roadmap rather than restated here: **Stage 8's own record, for anyone tracking how reliably this section stays current**: opened and closed in one day (2026-09-07), reopened -two days later, and reclosed the same day it was reopened -- each edit -to this paragraph has so far landed in the same change as the roadmap -event it describes, unlike the multi-day staleness windows the two -paragraphs below describe for Stages 7 and 8's own *earlier* drafts. -Don't read this as the pattern solved; read Stage 9's own eventual -entry here as the next real test of it. +two days later, reclosed the same day it was reopened, and audited at +its exit on 2026-09-11 -- each edit to this paragraph has so far landed +in the same change as the roadmap event it describes, unlike the +multi-day staleness windows the two paragraphs below describe for +Stages 7 and 8's own *earlier* drafts. Don't read this as the pattern +solved; read Stage 9's own eventual entry here as the next real test of +it. + +**And read the exit audit before trusting that record too far.** This +paragraph tracks whether *this section* stays current, which it has. +What the audit found is that three other places did not: three of Stage +8's nine Completion Criteria were marked Met against checks that did not +test what the criterion said, and three documents outside this file +still described the pre-reopening stage -- one of them a +`Checked-by: stage-boundary` document that all four reopening tasks +landed without touching. All six are fixed; `docs/planning/roadmap.md`'s +own **Exit audit, 2026-09-11** section is the record. **A section that +keeps itself current is not evidence that the repository around it +did.** **This sentence said "not yet started" for Stage 8 itself, twice, while that stage was still open** -- once for the same reason a fourth time as From 15edd51abd0098621a6980fc276ce5c6da8a1a20 Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Fri, 11 Sep 2026 14:37:10 +0100 Subject: [PATCH 3/5] Serialise the display-guarded tests onto one xdist worker Linux CI crashed a worker outright on the first run that had a virtual display -- no Python traceback, the hard process abort GLFW produces rather than an exception -- while the run immediately before it passed the identical test code. Two runs of the same tests with opposite results is contention, not a test failure: 8 xdist workers were each creating software-GL contexts against one Xvfb display. --dist loadgroup plus an xdist_group("display") mark on all 10 window-opening tests keeps them on one worker. Ordinary tests still distribute exactly as before; the group is 10 of 1209, so nothing else slows down (make ci: 183s, against 197s and 214s on the two runs before it). This is a hypothesis under test, not a confirmed fix -- the crash was never reproducible locally, so CI on this branch is the only place it can be judged. Co-Authored-By: Claude Opus 5 --- Makefile | 12 +++++++++++- pyproject.toml | 6 ++++++ tests/integration/CLAUDE.md | 12 ++++++++++++ tests/integration/test_interactive_window.py | 5 +++++ tests/integration/test_playback_cli.py | 5 +++++ 5 files changed, 39 insertions(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 90a4a11..75be325 100644 --- a/Makefile +++ b/Makefile @@ -70,8 +70,18 @@ typecheck: # workflow" rule. PYTEST_WORKERS ?= 4 +# `--dist loadgroup` (added 2026-09-11) distributes exactly like the +# default `load` for ordinary tests, and additionally keeps every test +# sharing an `xdist_group` mark on **one** worker. The only group here is +# `display` (`tests/integration/CLAUDE.md`): the tests that open a real +# GLFW window. Added after Linux CI crashed a worker outright -- no +# Python traceback, the hard process abort GLFW produces rather than an +# exception -- on one run and passed the identical test code on the run +# before it, with 8 workers all creating software-GL contexts against one +# Xvfb display at once. Serialising them removes that contention without +# slowing anything else down, since the group is 10 tests out of 1209. test: - uv run pytest -n $(PYTEST_WORKERS) + uv run pytest -n $(PYTEST_WORKERS) --dist loadgroup # Broken relative Markdown links (tools/validators/CLAUDE.md). Mechanizes # one specific instance of the Blast Radius "grep for the thing's name" diff --git a/pyproject.toml b/pyproject.toml index f630af2..11f4882 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -160,6 +160,12 @@ testpaths = ["tests"] # place; step definitions live next to the tests that bind them. bdd_features_base_dir = "tests/features" addopts = "--cov=pyflow --cov-report=term-missing" +# `display` marks the tests that open a real GLFW window. With +# `--dist loadgroup` (the Makefile's own `test` target) every test +# carrying it runs on one xdist worker, so software-GL contexts are +# never created concurrently -- see that target's own comment for the +# Linux CI worker crash that prompted it. +markers = ["display: opens a real GLFW window; serialised onto one xdist worker"] [tool.coverage.run] source = ["src/pyflow"] diff --git a/tests/integration/CLAUDE.md b/tests/integration/CLAUDE.md index d60a3c2..c1d3c8b 100644 --- a/tests/integration/CLAUDE.md +++ b/tests/integration/CLAUDE.md @@ -107,3 +107,15 @@ other. Take the reference from a separate window launched at the target frame, and crop out anything that moves for an unrelated reason (here, the scrub bar). Full rule: `docs/practices.md`, "A reference the run under test produced proves consistency, not correctness". + +**The display-guarded tests are also serialised onto one xdist worker** +(`@pytest.mark.xdist_group("display")` plus the Makefile's own +`--dist loadgroup`), added 2026-09-11. Linux CI crashed a worker +outright on the first run that had a virtual display -- no Python +traceback, the hard process abort GLFW produces rather than an +exception -- and then passed the *identical* test code on the previous +run, which is what identified it as contention rather than a real test +failure: 8 workers were creating software-GL contexts against one Xvfb +display at once. **Carry both decorators on any new test that opens a +real window**, not just the skip guard; the group is what keeps it off a +second worker. diff --git a/tests/integration/test_interactive_window.py b/tests/integration/test_interactive_window.py index 26b4ad9..58947c5 100644 --- a/tests/integration/test_interactive_window.py +++ b/tests/integration/test_interactive_window.py @@ -94,6 +94,7 @@ def _frame_hash(image: np.ndarray) -> str: return hashlib.sha256(np.ascontiguousarray(image).tobytes()).hexdigest() +@pytest.mark.xdist_group("display") @_needs_a_real_display def test_pyflow_run_opens_an_interactive_window_and_exits_cleanly(tmp_path: Path) -> None: """`pyflow run` through the real CLI, with the default interactive @@ -116,6 +117,7 @@ def test_pyflow_run_opens_an_interactive_window_and_exits_cleanly(tmp_path: Path assert "render window closed: 5 frame(s)" in result.stderr +@pytest.mark.xdist_group("display") @_needs_a_real_display def test_render_window_presents_distinct_frames() -> None: """A real glfw window, redrawn several times, actually presents @@ -169,6 +171,7 @@ def on_frame() -> None: """ +@pytest.mark.xdist_group("display") @_needs_a_real_display def test_close_key_terminates_the_render_loop_and_process_cleanly() -> None: """Pressing Escape closes the window and lets the process exit -- @@ -234,6 +237,7 @@ def _close_once_genuinely_live() -> None: assert window.frame_count >= _LIVE_FRAMES +@pytest.mark.xdist_group("display") @_needs_a_real_display def test_wheel_event_zooms_the_camera_live() -> None: """Live zoom (TASK-013): a real scroll-wheel event, injected into a @@ -260,6 +264,7 @@ def test_wheel_event_zooms_the_camera_live() -> None: assert window.camera.zoom > initial_zoom +@pytest.mark.xdist_group("display") @_needs_a_real_display def test_pointer_drag_pans_the_camera_live() -> None: """Live pan (TASK-013): a real pointer_down/move/up sequence, diff --git a/tests/integration/test_playback_cli.py b/tests/integration/test_playback_cli.py index f298850..711e37c 100644 --- a/tests/integration/test_playback_cli.py +++ b/tests/integration/test_playback_cli.py @@ -301,6 +301,7 @@ def _frame_hash(image: np.ndarray) -> str: _SAFETY_MAX_FRAMES = 50 +@pytest.mark.xdist_group("display") @_needs_a_real_display def test_space_pauses_playback_live(tmp_path: Path) -> None: """A real glfw window: inject Space early, and prove the rendered @@ -360,6 +361,7 @@ def _on_frame(window: RenderWindow) -> None: _SCRUB_SAFETY_MAX_FRAMES = 100 +@pytest.mark.xdist_group("display") @_needs_a_real_display def test_arrow_and_home_end_keys_seek_playback_live(tmp_path: Path) -> None: """Real glfw window, genuine injected key events -- proves Left/ @@ -451,6 +453,7 @@ def _world_to_screen( return (screen_x, screen_y) +@pytest.mark.xdist_group("display") @_needs_a_real_display def test_dragging_the_scrub_bar_seeks_without_panning_the_camera(tmp_path: Path) -> None: """The core risk this task started from: `RenderWindow.run`'s own @@ -641,6 +644,7 @@ def _on_frame(window: RenderWindow) -> None: return captured["hash"] +@pytest.mark.xdist_group("display") @_needs_a_real_display def test_keyboard_seeking_rerenders_the_field_in_real_pixels(tmp_path: Path) -> None: """ArrowLeft, through the real key wiring, against a real window.""" @@ -705,6 +709,7 @@ def _on_frame(window: RenderWindow) -> None: ) +@pytest.mark.xdist_group("display") @_needs_a_real_display def test_dragging_the_scrub_bar_rerenders_the_field_in_real_pixels(tmp_path: Path) -> None: """The same check for the mouse path, through real pointer events.""" From af7f527b89d76ff4ab717b329e63e2f441320420 Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Sat, 12 Sep 2026 10:26:02 +0100 Subject: [PATCH 4/5] Revert "Serialise the display-guarded tests onto one xdist worker" This reverts commit 15edd51abd0098621a6980fc276ce5c6da8a1a20. --- Makefile | 12 +----------- pyproject.toml | 6 ------ tests/integration/CLAUDE.md | 12 ------------ tests/integration/test_interactive_window.py | 5 ----- tests/integration/test_playback_cli.py | 5 ----- 5 files changed, 1 insertion(+), 39 deletions(-) diff --git a/Makefile b/Makefile index 75be325..90a4a11 100644 --- a/Makefile +++ b/Makefile @@ -70,18 +70,8 @@ typecheck: # workflow" rule. PYTEST_WORKERS ?= 4 -# `--dist loadgroup` (added 2026-09-11) distributes exactly like the -# default `load` for ordinary tests, and additionally keeps every test -# sharing an `xdist_group` mark on **one** worker. The only group here is -# `display` (`tests/integration/CLAUDE.md`): the tests that open a real -# GLFW window. Added after Linux CI crashed a worker outright -- no -# Python traceback, the hard process abort GLFW produces rather than an -# exception -- on one run and passed the identical test code on the run -# before it, with 8 workers all creating software-GL contexts against one -# Xvfb display at once. Serialising them removes that contention without -# slowing anything else down, since the group is 10 tests out of 1209. test: - uv run pytest -n $(PYTEST_WORKERS) --dist loadgroup + uv run pytest -n $(PYTEST_WORKERS) # Broken relative Markdown links (tools/validators/CLAUDE.md). Mechanizes # one specific instance of the Blast Radius "grep for the thing's name" diff --git a/pyproject.toml b/pyproject.toml index 11f4882..f630af2 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -160,12 +160,6 @@ testpaths = ["tests"] # place; step definitions live next to the tests that bind them. bdd_features_base_dir = "tests/features" addopts = "--cov=pyflow --cov-report=term-missing" -# `display` marks the tests that open a real GLFW window. With -# `--dist loadgroup` (the Makefile's own `test` target) every test -# carrying it runs on one xdist worker, so software-GL contexts are -# never created concurrently -- see that target's own comment for the -# Linux CI worker crash that prompted it. -markers = ["display: opens a real GLFW window; serialised onto one xdist worker"] [tool.coverage.run] source = ["src/pyflow"] diff --git a/tests/integration/CLAUDE.md b/tests/integration/CLAUDE.md index c1d3c8b..d60a3c2 100644 --- a/tests/integration/CLAUDE.md +++ b/tests/integration/CLAUDE.md @@ -107,15 +107,3 @@ other. Take the reference from a separate window launched at the target frame, and crop out anything that moves for an unrelated reason (here, the scrub bar). Full rule: `docs/practices.md`, "A reference the run under test produced proves consistency, not correctness". - -**The display-guarded tests are also serialised onto one xdist worker** -(`@pytest.mark.xdist_group("display")` plus the Makefile's own -`--dist loadgroup`), added 2026-09-11. Linux CI crashed a worker -outright on the first run that had a virtual display -- no Python -traceback, the hard process abort GLFW produces rather than an -exception -- and then passed the *identical* test code on the previous -run, which is what identified it as contention rather than a real test -failure: 8 workers were creating software-GL contexts against one Xvfb -display at once. **Carry both decorators on any new test that opens a -real window**, not just the skip guard; the group is what keeps it off a -second worker. diff --git a/tests/integration/test_interactive_window.py b/tests/integration/test_interactive_window.py index 58947c5..26b4ad9 100644 --- a/tests/integration/test_interactive_window.py +++ b/tests/integration/test_interactive_window.py @@ -94,7 +94,6 @@ def _frame_hash(image: np.ndarray) -> str: return hashlib.sha256(np.ascontiguousarray(image).tobytes()).hexdigest() -@pytest.mark.xdist_group("display") @_needs_a_real_display def test_pyflow_run_opens_an_interactive_window_and_exits_cleanly(tmp_path: Path) -> None: """`pyflow run` through the real CLI, with the default interactive @@ -117,7 +116,6 @@ def test_pyflow_run_opens_an_interactive_window_and_exits_cleanly(tmp_path: Path assert "render window closed: 5 frame(s)" in result.stderr -@pytest.mark.xdist_group("display") @_needs_a_real_display def test_render_window_presents_distinct_frames() -> None: """A real glfw window, redrawn several times, actually presents @@ -171,7 +169,6 @@ def on_frame() -> None: """ -@pytest.mark.xdist_group("display") @_needs_a_real_display def test_close_key_terminates_the_render_loop_and_process_cleanly() -> None: """Pressing Escape closes the window and lets the process exit -- @@ -237,7 +234,6 @@ def _close_once_genuinely_live() -> None: assert window.frame_count >= _LIVE_FRAMES -@pytest.mark.xdist_group("display") @_needs_a_real_display def test_wheel_event_zooms_the_camera_live() -> None: """Live zoom (TASK-013): a real scroll-wheel event, injected into a @@ -264,7 +260,6 @@ def test_wheel_event_zooms_the_camera_live() -> None: assert window.camera.zoom > initial_zoom -@pytest.mark.xdist_group("display") @_needs_a_real_display def test_pointer_drag_pans_the_camera_live() -> None: """Live pan (TASK-013): a real pointer_down/move/up sequence, diff --git a/tests/integration/test_playback_cli.py b/tests/integration/test_playback_cli.py index 711e37c..f298850 100644 --- a/tests/integration/test_playback_cli.py +++ b/tests/integration/test_playback_cli.py @@ -301,7 +301,6 @@ def _frame_hash(image: np.ndarray) -> str: _SAFETY_MAX_FRAMES = 50 -@pytest.mark.xdist_group("display") @_needs_a_real_display def test_space_pauses_playback_live(tmp_path: Path) -> None: """A real glfw window: inject Space early, and prove the rendered @@ -361,7 +360,6 @@ def _on_frame(window: RenderWindow) -> None: _SCRUB_SAFETY_MAX_FRAMES = 100 -@pytest.mark.xdist_group("display") @_needs_a_real_display def test_arrow_and_home_end_keys_seek_playback_live(tmp_path: Path) -> None: """Real glfw window, genuine injected key events -- proves Left/ @@ -453,7 +451,6 @@ def _world_to_screen( return (screen_x, screen_y) -@pytest.mark.xdist_group("display") @_needs_a_real_display def test_dragging_the_scrub_bar_seeks_without_panning_the_camera(tmp_path: Path) -> None: """The core risk this task started from: `RenderWindow.run`'s own @@ -644,7 +641,6 @@ def _on_frame(window: RenderWindow) -> None: return captured["hash"] -@pytest.mark.xdist_group("display") @_needs_a_real_display def test_keyboard_seeking_rerenders_the_field_in_real_pixels(tmp_path: Path) -> None: """ArrowLeft, through the real key wiring, against a real window.""" @@ -709,7 +705,6 @@ def _on_frame(window: RenderWindow) -> None: ) -@pytest.mark.xdist_group("display") @_needs_a_real_display def test_dragging_the_scrub_bar_rerenders_the_field_in_real_pixels(tmp_path: Path) -> None: """The same check for the mouse path, through real pointer events.""" From 2d91e68f9b1d39cf51f9d8e543a6a23f23681a0e Mon Sep 17 00:00:00 2001 From: Adam Clemens Date: Sat, 12 Sep 2026 10:32:14 +0100 Subject: [PATCH 5/5] Revert the xvfb change too; record both CI attempts as a backlog item The loadgroup revert (previous commit) undid the fix that hung Windows. This undoes the xvfb change it was trying to rescue, returning CI to exactly its prior behaviour: the 10 display-guarded tests skip on Linux and run on Windows. Why, with the measurements rather than an impression. xvfb itself worked -- Linux skips went 29 -> 18, matching a local run, so all 10 tests genuinely ran. It was reverted because across three attempts on identical test code, two passed and one crashed an xdist worker with no Python traceback (GLFW aborts the process rather than raising), and that attempt then passed on a plain re-run. A third of attempts failing on a gating check is worse than a documented, stable gap. Both failures point the same way from opposite directions: xvfb raised concurrent window creation across processes, loadgroup raised sequential window creation within one process, and each broke a different platform. What has always worked is 1-2 GLFW windows per process spread across workers. A real fix has to reduce the window count, not redistribute it -- starting with the two new *_rerenders_the_field_in_real_pixels tests, which open two windows each. That is a task, not a polish item, so it is now a backlog entry carrying both measurements, with the same warning in tests/integration/CLAUDE.md and .github/workflows/ci.yml so the next attempt does not rediscover either failure. The roadmap exit audit section records it as an open gap rather than leaving Criterion 6 looking like its evidence runs everywhere: it is met, and checked on one of the two platforms CI covers. make ci green, 1209 tests. Co-Authored-By: Claude Opus 5 --- .github/workflows/ci.yml | 49 ++++++++++++++++++++++--------------- docs/planning/backlog.md | 48 ++++++++++++++++++++++++++++++++++++ docs/planning/roadmap.md | 44 +++++++++++++++++++++++++++------ tests/integration/CLAUDE.md | 41 +++++++++++++++++++++++++------ 4 files changed, 146 insertions(+), 36 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7ec7dcf..d3e3e9c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -72,31 +72,40 @@ jobs: run: | echo "http://archive.ubuntu.com/ubuntu" | sudo tee /etc/apt/apt-mirrors.txt sudo apt-get update - sudo apt-get install -y libegl1 libgl1 mesa-vulkan-drivers xvfb + sudo apt-get install -y libegl1 libgl1 mesa-vulkan-drivers - name: make install run: make install - # `xvfb` above, and `xvfb-run` below, are what let the display- - # guarded tests (`@_needs_a_real_display` in `tests/integration/ - # test_interactive_window.py` and `test_playback_cli.py`) run here - # at all. **Added 2026-09-11 by the Stage 8 exit audit**, which - # found those 10 tests -- including every check that Stage 8's own - # Completion Criterion 6 rests on -- skipping silently on Linux - # while passing on Windows, so a green two-platform matrix was - # proving live-window behaviour on exactly one platform. Without a - # `DISPLAY`, `_display_available()` short-circuits before GLFW is - # ever constructed (deliberately: GLFW hard-aborts the process - # rather than raising, see `tests/integration/CLAUDE.md`), so the - # skip was invisible in the run summary. - - name: make ci (Linux, under a virtual display) - if: runner.os == 'Linux' - env: - PYTEST_WORKERS: "8" - run: xvfb-run -a make ci - + # **No `xvfb` here, and that is a recorded decision rather than an + # omission** -- so the 10 display-guarded tests + # (`@_needs_a_real_display` in `tests/integration/ + # test_interactive_window.py` and `test_playback_cli.py`) skip on + # Linux and run only on Windows. Without a `DISPLAY`, + # `_display_available()` short-circuits before GLFW is ever + # constructed (deliberately: GLFW hard-aborts the process rather + # than raising, see `tests/integration/CLAUDE.md`). + # + # **Tried and reverted 2026-09-11/12 by the Stage 8 exit audit.** + # Installing `xvfb` and running `xvfb-run -a make ci` does work -- + # Linux skips dropped from 29 to 18, matching a local run, so all + # 10 tests genuinely ran. It was reverted because it made the + # Linux job flaky: across three attempts on identical test code, + # two passed and one crashed an xdist worker outright, with no + # Python traceback (the hard process abort GLFW produces), and the + # crashed one then passed on a plain re-run. 8 workers creating + # software-GL contexts against one Xvfb display concurrently is the + # working hypothesis, never reproduced locally. + # + # **Serialising the display tests onto one worker + # (`--dist loadgroup`) is not the fix, and made things worse**: it + # hung the *Windows* job for 5h45m until GitHub's own 6-hour limit + # killed it, at 95% with ~6 tests outstanding and an orphaned + # python process at cleanup. One process creating ~15 GLFW windows + # in sequence is the failure that buys; 1-2 per process spread + # across workers is what has always worked. See + # `docs/planning/backlog.md` for the open item. - name: make ci - if: runner.os != 'Linux' env: # Runs with more test parallelism than a local `make test` # (Makefile's own `PYTEST_WORKERS ?= 4` default) -- CI runs on diff --git a/docs/planning/backlog.md b/docs/planning/backlog.md index 29d4505..bd27213 100644 --- a/docs/planning/backlog.md +++ b/docs/planning/backlog.md @@ -2427,6 +2427,54 @@ here.): actually built (recording, replay, playback's own first cut) and does not track the reopening's own progress. +- [ ] **Run the display-guarded tests on Linux CI.** Raised 2026-09-12 by + the Stage 8 exit audit, which found the gap, tried two fixes, and + reverted both. The 10 tests carrying `@_needs_a_real_display` + (`tests/integration/test_interactive_window.py`, + `test_playback_cli.py`) skip on `ubuntu-latest` and run only on + `windows-latest`, so **a green two-platform matrix proves + live-window behaviour on one platform** -- including every check + Stage 8's own Completion Criterion 6 rests on. + + **Not blocking**: the tests do run, on Windows, every CI run, and + they are the platform the maintainer develops on. What is lost is + the second platform's confirmation, which for GLFW/wgpu behaviour + is exactly where the two have diverged before + (`tests/integration/CLAUDE.md`'s own 2026-08-19 SIGABRT entry). + + **Both obvious approaches are already known to fail, with + measurements** -- read `tests/integration/CLAUDE.md` and + `.github/workflows/ci.yml`'s own comment before attempting this, + rather than rediscovering them: + + 1. **`xvfb` works and is flaky.** `xvfb-run -a make ci` plus an + `xvfb` apt package dropped Linux skips from 29 to 18, matching a + local run -- so all 10 tests genuinely ran. Across three + attempts on *identical* test code, two passed and one crashed an + xdist worker with no Python traceback (GLFW's hard process + abort), and that attempt then passed on a plain re-run. Roughly + a third of attempts, on a gating check. + 2. **Serialising them onto one worker is worse.** `--dist + loadgroup` plus an `xdist_group("display")` mark hung the + *Windows* job for 5h45m until GitHub's own 6-hour limit killed + it -- 95% complete, ~6 tests outstanding, an orphaned python + process at cleanup. It passed `make ci` locally in 183s, so + nothing short of CI could have caught it. + + **The unblock condition is a hypothesis worth testing, not a + design**: both failures point the same way from opposite + directions -- approach 1 raised *concurrent* window creation across + processes, approach 2 raised *sequential* window creation within + one process, and each broke a different platform. What has always + worked is 1-2 GLFW windows per process, spread across workers. So + the likely route is **reducing the number of windows rather than + redistributing them**, starting with the two + `*_rerenders_the_field_in_real_pixels` tests, which open two each + (a main window plus an independent reference window) and are the + highest count in the suite. Whether that alone takes the Linux + flake rate to zero is unmeasured; the flake was never reproducible + locally, so CI is the only place any of this can be judged, which + is what makes it a task rather than a quick fix. --- # Part III — Audit history diff --git a/docs/planning/roadmap.md b/docs/planning/roadmap.md index 0f3f885..465f431 100644 --- a/docs/planning/roadmap.md +++ b/docs/planning/roadmap.md @@ -11591,15 +11591,43 @@ diff review" already names: section for why neither a task anchor nor a "next time" note reaches a task the file has never named. -**Three smaller things fixed on the way past.** `pyflow play --help` +**Two smaller things fixed on the way past.** `pyflow play --help` documented none of the controls the subcommand exists for -- pause, -speed, scrub -- so `README.md` was the only place to find them; Linux CI -had no `DISPLAY` and no `xvfb`, so all 8 display-guarded tests (every -check Criterion 6 rests on) skipped there while passing on Windows, -making a green two-platform matrix prove live-window behaviour on one -platform; and 19 stray `checkpoint_*.pt` files from a 2026-09-07 manual -run sat untracked in the repository root, which `.gitignore`'s -`checkpoints/` rule never matched. +speed, scrub -- so `README.md` was the only place to find them; and 19 +stray `checkpoint_*.pt` files from a 2026-09-07 manual run sat untracked +in the repository root, which `.gitignore`'s `checkpoints/` rule never +matched. + +**A third was attempted and reverted, which is the more useful +record.** Linux CI has no `DISPLAY` and no `xvfb`, so all 10 +display-guarded tests -- every check Criterion 6 rests on -- skip there +and run only on Windows, making a green two-platform matrix prove +live-window behaviour on one platform. **Two fixes were tried over +2026-09-11/12 and both were reverted, each having broken a different +platform:** + +- **`xvfb` worked, and was flaky.** Linux skips dropped from 29 to 18, + matching a local run, so all 10 genuinely ran. But across three + attempts on *identical* test code, two passed and one crashed an xdist + worker with no Python traceback (GLFW's hard process abort), and that + attempt then passed on a plain re-run -- roughly a third of attempts, + on a gating check. +- **Serialising them onto one worker (`--dist loadgroup`) was worse.** + It hung the *Windows* job for 5h45m until GitHub's own 6-hour limit + killed it, at 95% with ~6 tests outstanding. It had passed `make ci` + locally in 183s, so nothing short of CI could have caught it. + +**Both point the same way from opposite directions**: the first raised +concurrent window creation across processes, the second raised +sequential window creation within one process, and what has always +worked is 1-2 GLFW windows per process spread across workers. A real fix +has to reduce the window count rather than redistribute it, which is a +task and not a polish item -- `docs/planning/backlog.md` now carries it +with these measurements, and `tests/integration/CLAUDE.md` and +`.github/workflows/ci.yml` both say why the obvious approaches are +already known to fail. **Recorded as an open gap rather than left +looking closed**: Criterion 6 is met, and the evidence for it runs on +one of the two platforms CI covers. **What this audit deliberately did not do.** The Goal's own "scrubbed to any point" is still satisfied only within the window one `pyflow diff --git a/tests/integration/CLAUDE.md b/tests/integration/CLAUDE.md index d60a3c2..5ce6bd1 100644 --- a/tests/integration/CLAUDE.md +++ b/tests/integration/CLAUDE.md @@ -87,14 +87,39 @@ entry. **Add a module to `HEADLESS_MODULES` whenever a new module belongs on the headless side**, the same standing obligation `MODULES` above it already carries. -**The display-guarded tests run on Linux CI as of 2026-09-11**, under -`xvfb-run` (`.github/workflows/ci.yml`). Before that they skipped there -and ran only on Windows, which meant a green two-platform matrix was -proving live-window behaviour on one platform -- found by the Stage 8 -exit audit, since every check that stage's Completion Criterion 6 rests -on is display-guarded. The `_display_available()` probe is unchanged and -still short-circuits before GLFW is constructed when there is no -`DISPLAY`; `xvfb` simply means there now is one. +**The display-guarded tests still run on Windows CI only, and that is a +recorded decision rather than an oversight.** The Stage 8 exit audit +found it (every check that stage's Completion Criterion 6 rests on is +display-guarded, so a green two-platform matrix was proving live-window +behaviour on one platform), tried to fix it, and reverted the fix. **Read +this before trying again, because both obvious approaches are already +known to fail:** + +1. **`xvfb` works and is flaky.** Installing it and running `xvfb-run -a + make ci` dropped Linux skips from 29 to 18, matching a local run, so + all 10 tests genuinely ran. Across three attempts on *identical* test + code, two passed and one crashed an xdist worker outright -- no Python + traceback, the hard process abort GLFW produces rather than an + exception -- and the crashed attempt then passed on a plain re-run. + Roughly a third of attempts, on a gating check. Working hypothesis, + never reproduced locally: 8 workers creating software-GL contexts + against one Xvfb display at once. +2. **Serialising the group onto one worker is worse, not better.** + `--dist loadgroup` plus an `xdist_group` mark hung the *Windows* job + for 5h45m until GitHub's own 6-hour limit killed it -- at 95%, ~6 + tests outstanding, an orphaned python process at cleanup. It also + passed `make ci` locally in 183s, so nothing short of CI would have + caught it. + +**The transferable fact, which both results point at from opposite +directions: what works is 1-2 GLFW windows per process, spread across +workers.** Approach 1 raised concurrent window creation across processes; +approach 2 raised sequential window creation within one process. Each +broke a different platform. A real fix probably has to reduce the number +of windows rather than redistribute them -- the two +`*_rerenders_the_field_in_real_pixels` tests below open two each, which +is the highest count here. See `docs/planning/backlog.md` for the open +item. **Comparing rendered pixels: never build the reference from the run under test.** `test_playback_cli.py`'s two `*_rerenders_the_field_in_