Repository navigation
fix: make the NFR-1 spawn test wait for its child — this is why CI was red - #294
Merged
Merged
Conversation
… why CI was red
CI was not failing because of Linux. It was failing because CI runs a
different command than I did, and I had never compared them.
mine: pytest tests/ --ignore=tests/integration --ignore=tests/load
-p no:randomly
CI: pytest tests/ -m "not integration"
Three differences, all mine: two directories excluded, and -p
no:randomly, WHICH MEANS EVERY RUN I CALLED GREEN WAS IN FIXED ORDER
WHILE CI'S WAS RANDOMISED. Under CI's exact command the suite fails on
Windows too, which removes the platform from the explanation entirely.
The failure is
test_nfr1_harness.py::TestRealSpawnDetection::test_real_spawn_is_detected_and_fails
-- the flake every node in this sprint was warned about and correctly
attributed to itself. Nobody diagnosed it, including me; it was carried
as a known nuisance for the whole sprint.
THE CAUSE, MEASURED. The test spawned a child that lived 0.6 seconds,
and the measurement window is only as long as ten near-empty tool calls
take. subprocess.Popen returns before the operating system has
necessarily made the process enumerable. So on a loaded machine the
window could close before the child became visible, and THE HARNESS WAS
BLAMED FOR MISSING A SPAWN THAT HAD NOT HAPPENED YET.
The child now writes a byte to stdout and the test blocks on reading it,
which makes the child's existence a PRECONDITION of the measurement
rather than a race against it. It sleeps long enough to outlive any
window and is terminated explicitly. Nothing about what the test asserts
changed -- only whether the thing it asserts about exists yet.
NEGATIVE CONTROL, SAME LOAD, SAME MACHINE: pre-fix 3 of 8 failed,
post-fix 0 of 8. Unloaded, 0 of 12. The fix is what changed the outcome,
not the weather.
Recorded honestly: the FIRST load attempt also reported 0 of 8 and
proved nothing, because multiprocessing spawned from stdin cannot start
workers on Windows and every one of them crashed. That run measured an
idle machine while claiming to measure a loaded one, and was discarded
rather than counted.
ALL FOUR CI STEPS NOW PASS LOCALLY END TO END: collect-only, unit tests
under CI's exact invocation including randomised order, integration
tests, and the coverage step.
AND THE COVERAGE COMMENT WAS WRONG IN BOTH HALVES. ci.yml said it
"enforces 50% minimum threshold configured in .coveragerc
(fail_under=50)". The file reads fail_under = 30, and measured coverage
is 44.17% -- so a real 50% floor would have been failing this build
rather than passing it, and anyone reading the comment would have
believed the opposite. Corrected in place with the measurement.
REVIEW-INDEX 52's original entry is left standing rather than rewritten,
with the diagnosis appended beneath it. The release was merged on an
honest but wrong hypothesis, and the sequence is worth more than a tidy
record. The lesson is not that Linux differs. It is that A SUITE IS ONLY
GREEN UNDER THE COMMAND YOU ACTUALLY RAN, and for the length of this
sprint that command differed from CI's in three ways nobody had checked.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…green The spawn-flake fix in this branch is real and proven: 3 of 8 failures before, 0 of 8 after, under identical load. IT DID NOT MAKE CI GREEN. On this branch, CI #157 still fails at step 14, "Run unit tests", on both Python 3.10 and 3.11. So the flake was A cause and not THE cause, and the claim "this is not a Windows/Linux split" is withdrawn. Something still fails on ubuntu-latest that has never failed here across many full runs under CI's exact command. What is established, and nothing more: CI's command differed from the one used through the sprint in three real ways; one genuine flake was found and fixed by closing a spawn race; and a Linux-only failure remains UNDIAGNOSED because the run logs need an authenticated session this one did not have. The failing test name is one click away for anyone signed in and was never obtained. REVIEW-INDEX 52 has now been wrong twice and rewritten three times, and all three passes are left standing. Each was honestly reasoned from what had just been measured and two were wrong -- conclusions outrunning their evidence, which is the failure mode that record exists to document. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… wrote them Both were found by running CI's exact command inside a Linux container rather than by guessing, after two earlier attempts at this failure drew conclusions their evidence did not support. THE DOCS-INDEX TEST WAS MINE, WRITTEN HOURS EARLIER, AND COUNTED THE WRONG THING. It counted files with os.listdir, so it counted whatever sat in the working tree -- including docs/phase-0-reverse-engineering/ast_call_graph.json, a gitignored analysis dump that exists here and in no clone. The index was generated saying 26 where the repository holds 25. It now counts what git tracks, because the index describes the REPOSITORY and not whoever's checkout happens to be on disk. The index is regenerated from the same source. A test written to stop documentation drifting from the tree was itself reading a different tree than the one it documents. THE MODEL-FALLBACK TEST GUARDED AGAINST AN EXCEPTION THE FUNCTION DOES NOT RAISE. locate_library_root is documented Optional[Path] and RETURNS None when no candidate exists; the test caught LibrarySetupError only. So the guard never fired: on a machine with the sibling checkout the test ran, and on every machine without one -- which is every CI runner -- it died on `None / "knowledge-graph"` with a TypeError rather than skipping. Now it handles the documented return. The exception arm is kept, because the resolver may still raise for a malformed override. This is the sprint's dominant defect class arriving in the tests themselves: a guard written against a contract the callee does not have, invisible because the happy path was the only path ever taken. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…xposed Docker was available the whole time. Running CI's exact command inside python:3.11-slim, against a clone of the branch, produced the three failing test names in minutes -- the thing that had been guessed at twice and was one authenticated click away throughout. TWO WERE GENUINE CI FAILURES, BOTH NOW FIXED. The docs-index test was mine, written hours earlier, and counted os.listdir -- so it counted the working tree including a gitignored analysis dump, and the index was generated saying 26 where the repository holds 25. The model-fallback test guarded against an exception locate_library_root does not raise; its contract is Optional[Path] and it RETURNS None, so the guard never fired and the test died wherever the sibling library is absent, which is every CI runner. THE THIRD WAS A CONTAINER ARTIFACT, AND PROVEN SO RATHER THAN ASSUMED. Re-run with the clone at a runner-shaped path, the whole suite passes with zero failures. But proving it exposed something worth keeping: the push-gate catalogue RESOLVES BY DIRECTORY NAME. Resolution is repo_root.parent / <repo-name> / <entry>, which is correct on GitHub Actions and on any clone that kept the default directory name, and SILENTLY WRONG for anyone who cloned into a different one. It fails by producing a path rather than an error, so register-mcp would write a settings entry pointing at nothing. Recorded as REVIEW-INDEX 54, not fixed -- it is V2-024's design and deserves its own change. That is the second time in two days a TEST environment exposed a product defect nobody was looking for, after the bootstrap-template sentinel. Both were found by changing something incidental and watching what broke downstream. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
One commit, three files. This is the diagnosis of the red CI that #255 was merged despite.
It was never Linux
CI runs a different command than the one used throughout the v2.0.0 sprint, and nobody compared them:
Three differences: two excluded directories, and
-p no:randomly— so every run called green was in fixed order while CI's was randomised. Under CI's exact command the suite fails on Windows too, which removes the platform from the explanation entirely.The failure
test_nfr1_harness.py::TestRealSpawnDetection::test_real_spawn_is_detected_and_fails— the flake every node in the sprint was warned about and correctly attributed to itself. Nobody diagnosed it; it was carried as a known nuisance.Cause, measured. The test spawned a child that lived 0.6s, and the measurement window is only as long as ten near-empty tool calls take.
subprocess.Popenreturns before the OS has necessarily made the process enumerable — so on a loaded machine the window closed before the child became visible, and the harness was blamed for missing a spawn that had not happened yet.The child now writes a byte to stdout and the test blocks on reading it, making the child's existence a precondition of the measurement rather than a race against it. Nothing about what the test asserts changed — only whether the thing it asserts about exists yet.
Evidence
All four CI steps now pass locally end to end: collect-only, unit tests under CI's exact invocation including randomised order, integration, and coverage.
Recorded honestly: the first load attempt also reported 0/8 and proved nothing, because
multiprocessingspawned from stdin cannot start workers on Windows and every one crashed. That run measured an idle machine while claiming to measure a loaded one, and was discarded rather than counted.Also in here
ci.yml's coverage comment claimed it "enforces 50% minimum threshold configured in.coveragerc(fail_under=50)". The file readsfail_under = 30, and measured coverage is 44.17% — so a real 50% floor would have been failing this build rather than passing it, and anyone reading the comment would have believed the opposite. Corrected in place with the measurement.docs/REVIEW-INDEX.mdentry 52's original text is left standing with the diagnosis appended beneath it rather than rewritten. The release was merged on an honest but wrong hypothesis, and the sequence is worth more than a tidy record.The lesson is not that Linux differs. It is that a suite is only green under the command you actually ran.
🤖 Generated with Claude Code