fix(coord/test): PR 531's clean 5-commit half -- pid-reuse race, leak-gate coverage, session-id refusal, floor raise - #615
Open
wshallwshall wants to merge 13 commits into
Open
fix(coord/test): PR 531's clean 5-commit half -- pid-reuse race, leak-gate coverage, session-id refusal, floor raise#615wshallwshall wants to merge 13 commits into
wshallwshall wants to merge 13 commits into
Conversation
…tead of a bare assert 0 > 0
tests/test_multishard_smoke.py has been red on main with `assert e.reads > 0` firing as `assert 0 > 0`,
and the failure could not be attributed. This does not change what the test ASSERTS. It changes what
the failure SAYS, which is the part that was unusable.
I HANDED THIS INVESTIGATION A HYPOTHESIS AND IT WAS WRONG. I reasoned that because the two assertions
above it PASS -- `inbound_rows == _COUNT_PER_ENGINE` and `foreign_rows == 0` -- the rows had ARRIVED and
only the counter was wrong, so this was a counter defect. That is refuted. Both of those counters are
CONFIG-derived, not traffic-derived: the `/connections` builder appends a source row for EVERY registry
inbound unconditionally and sets `read` to an int rather than None, so both pass unchanged on an engine
that received NOTHING -- including one whose listeners never bound. The test's own docstring already
conceded it: the isolation proof "is config-derived so it holds regardless of the write lock". `reads`
is the ONLY traffic-derived counter of the three, and `reads == 0` means the engine genuinely received
nothing. The pair I called discriminating carries zero traffic information.
THE ENGINE ALREADY KNOWS WHY, AND THE HARNESS WAS THROWING IT AWAY. A lane that failed to start is
reported as not-listening with a reason (ADR 0031, surfaced as `/connections`.error). The harness
fetched that response, read `name` and `read` off each row, and discarded `error`. So the one artifact
that could attribute the failure was fetched and dropped on every run.
`EngineAttribution` now carries `failed_lanes`, the engine's verbatim reasons; the assertion prints them
and distinguishes the two cases -- lanes reported as not listening (the engine never received traffic)
versus no failed lanes at all (they bound and the traffic did not arrive or did not commit, a different
cause this assertion cannot narrow further and now says so rather than implying it can). The JSON
artifact carries the field too, so a CI reader with only the uploaded file can attribute it without
re-running anything, which is the whole point.
Collected for EVERY inbound row rather than only this engine's own: a lane failing under a peer's tag is
equally diagnostic, and filtering by tag here would drop the cross-engine case the isolation assertion
above exists to catch.
READ DIRECTLY AS `row.error`, NOT `getattr(row, "error", None)`. `EngineClient.connections()` is typed
`list[ConnectionRow]` and that model declares the field, so a default could only ever mask a RENAME --
after which this would report "no failed lanes" forever, silently, on precisely the runs it exists to
explain. A diagnostic field that fails closed to "nothing to report" is worse than no field.
VERIFIED with both controls, because a green run has no failed lanes and therefore proves nothing about
the new path:
failed-lane case -> inbound_rows=2, foreign_rows=0, reads=0 (the exact CI triple) AND both reasons
collected into failed_lanes
clean case -> reads=8, failed_lanes=() -- silent on success, so the field is not always-on noise
tests/test_multishard_smoke.py: 2 passed. ruff 0.15.22 clean. No cp1252-unsafe character introduced.
WHAT THIS DOES NOT DO, explicitly: it does not fix the CI red and does not claim to. Which of the
conditions fired on CI is NOT ESTABLISHED -- the engine's own stdout is written to a temp file that
`EngineNode` discards on stop unless `MEFOR_BENCH_KEEP_NODE_LOGS` names a directory, and the CI leg
leaves it unset, so the bind-failure warning is thrown away on every run. Capturing that is the next
step and is a ci.yml change I have not made. This commit makes the NEXT occurrence self-attributing,
which is what four sessions lacked when a neighbouring red was mis-attributed three times today.
…tead of a monkeypatched one BACKLOG #321, BUILD HALF ONLY. Owner-directed. The floor raise in security.yml is NOT here -- see the bottom of this message. THE DEFECT. Every per-class test in tests/test_scan_forbidden.py runs behind the `sf` fixture, which monkeypatches synthetic values over FORBIDDEN / ESTATE_TOKENS / SITE_CODE_RE / _SITE_CODE_FILE. They prove the machinery matches a pattern someone handed it, and never touch the load path. With no prefix loaded both site detectors fall back to `_NEVER` (scan_forbidden.py:194), an empty negative lookahead that matches nothing anywhere -- so a blind scanner and a clean tree are the same green tick, and the suite that looks like per-class coverage CANNOT FAIL when the real token set is wrong. That is the shape of the original defect, reproduced inside its own test suite. TWO ARMS, split by what each is allowed to touch. BEHAVIOURAL: pins the source to the committed synthetic example and drives the REAL pipeline -- MEFOR_FORBIDDEN_TOKENS -> _resolve_token_text -> _parse_tokens -> compilation -> scan_file. Nothing is monkeypatched onto the globals, and probes are DERIVED from what loaded, so a set that loads blind never reaches an assertion: the derivation fails first. REAL SET: runs only where a real source is configured, and asserts STRUCTURE ONLY -- present, not the sentinel, every class counted. It never reads, builds with, or reports a real token. Proving the scanner catches a real token would require putting one in this file, which is exactly the disclosure the scanner exists to prevent (CLAUDE.md sec. 9); the test would become the leak. A HOLE I FOUND IN MY OWN FIRST CUT AND CLOSED. A configured-but-MANGLED source leaves TOKENS_PRESENT false, identically to having no source at all -- so skipping on that alone turned the documented cutover-mangling case (headers lost, comments only, a BOM before the first section) into a green tick. Only the ABSENCE of a source is now a skip; a source that exists and parsed to nothing FAILS. AND ONE THE RED-FIRST PASS FOUND IN MY OWN TEST, recorded because it is this item's exact subject. With FORBIDDEN forced empty, the [names] arm still PASSED -- green against the very class it names. The sets OVERLAP BY DESIGN (a customer name is typically in [names] AND [estate]), so the probe word drawn from [names] was also an estate token and the estate detector produced the hit. The probe is now filtered to a candidate no other detector can explain, and the arm goes red as it should. An over-determined assertion is not coverage, which is the whole reason this item exists. ASSERTED DELIBERATELY, AND NOT: - the estate arm asserts the scan_file PATH, not a count. [estate_body_only] tokens are held out of _ESTATE_FILE_RES and never enter scan_file, while raising the `estate` count identically -- so a count cannot tell a token the file scanner sees from one it does not. - never reason TEXT. The scanner substitutes a generic reason when a reason would itself match a detector, so asserting wording reads the substituted value rather than the finding. - a negative control per class, so an arm cannot be satisfied by a detector that flags everything, and a negative control on the blind state itself, so the guard cannot be asserting something vacuously true. RED-FIRST, ALL FIVE, each broken in scan_forbidden.py and watched to fail, then restored: site prefixes forced to the sentinel; estate held out of the file scan; names loaded empty; the site detector widened to any six-digit run; the blind fallback made unreachable. NOT DONE, AND NOT MINE TO DO: MEFOR_MIN_DETECTORS in .github/workflows/security.yml stays at names=7,estate=13,site_prefixes=1. Raising it to 8/14/2 hard-fails a required check until the owner updates BOTH the Actions and the Dependabot secrets -- both, or every Dependabot PR fails. I will ask rather than infer that from any message. VERIFIED: ruff check + format clean; 102 tests across the three scan_forbidden suites; glyph scan 0 over the new file against a 736-hit positive control. No token value appears in this file, in any assertion message, or in this commit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…1360)
Three test files each spawned `cmd /c exit`, waited for it to EXIT, slept, and
returned its pid as "free". The pid is free at the moment it returns and
NOTHING KEEPS IT FREE: between that return and the moment the tool under test
reads the record, the OS may hand it to a new process.
Replaced all three with tests/_dead_pid.py, returning 2147483647 (Int32.MaxValue):
within the [int] cast the fence performs, non-zero so it takes the liveness
path, and structurally unassignable on either platform -- dead by construction
rather than by timing.
Lifted from stranded PR 531 (per Cleaner's grading) onto a fresh branch. The
number changed from #1303 to #1360: #1303 was legitimately allocated on
2026-08-21 but bound to a worktree confirmed dead (fleet.ps1: freshest record
INTERRUPTED at 119+ hours; absent from git worktree list), and the ledger
gate's allocation registry has no rebind mechanism for either kind (verified
by reading scripts/coord/alloc.ps1 in full -- it always searches forward for
a free slot, never re-touches an existing one). Renumbered to the freshly
allocated #1360 rather than hand-edit the protected registry file, which a
PreToolUse hook explicitly and correctly refuses. #1303 stays a permanent
hole, which this project's own tooling treats as free ("holes are free,
collisions are not").
The ledger banner is Lander-authored per ADR 0165 (the code fix is the
builder's own verified work, landed unmodified) -- verified independently
against the landed diff before writing it: tests/_dead_pid.py exists, defines
NEVER_LIVE_PID: Final = 2147483647, and all three test files import it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… inbox (BACKLOG #1302)
An id-addressed message could strand silently. `mail-drain.ps1:852` compares the recorded
`to.sessionId` against the reading session's harness id with `-ne`; an id from another namespace never
matches, so the message sat in the inbox until it was swept to expired/ -- with the send path printing
`Queued 1 message(s)` the whole time.
THE ASYMMETRY WAS THE DEFECT, AND IT IS WHY THE GUARD IS ON THE SEND SIDE. The drain already reported
its half ("N message(s) are addressed to a different session id and were left in the inbox"), so the
RECIPIENT was told. The SENDER was told nothing -- and the sender is the only party who can correct
the id. I did not touch the drain's filter: it is CORRECT. A worktree outlives its occupant, so an
id-addressed note must not reach a stranger, and loosening the match to "fix" delivery would trade a
silent non-delivery for a silent MIS-delivery, which is worse.
THE SHAPE. A harness session id is a bare UUID (the drain reads `$hook.session_id`). The MCP namespace
prefixes its own as `local_<uuid>`. Two id spaces for one session, compared literally.
MEASURED, AND I WAS THE ONE WHO CAUSED IT. Six of my own messages to the dispatcher stranded for a
whole session -- a level report, a CI mechanism diagnosis, two unprompted self-retractions and a
request to pull two never-started items. The recipient read my lane as silent and wrote "level
unreported" three times. They were found only by opening the box by hand, and were due to expire with
neither end told.
PARTIAL CONTROL, AND THE ITEM SAYS SO RATHER THAN LEAVING IT TO BE DISCOVERED. This catches a
wrong-NAMESPACE id. It does NOT catch a correctly-shaped but STALE one -- an id belonging to a session
that has ended fails identically and just as silently. A pass at send is not a promise of delivery.
THE MUST-NOT-TRIP ARM IS IN THE SAME TEST AS THE REFUSAL, deliberately: a guard that rejected
everything would otherwise pass by satisfying one half. No `-ToSessionId` at all is the ordinary
broadcast and still sends; a genuine bare-UUID id still sends. RED-FIRST IN BOTH DIRECTIONS, each
broken then restored -- disabling the guard reddens the refusal test, widening it to reject everything
reddens the must-not-trip test.
The refusal names the REMEDY, not just the rejection: the sender's next move is to drop the flag and
address by worktree path, and a message that only said "invalid" would leave them hunting an id.
VERIFIED: ruff clean; 103 tests across the two mail suites; red-first both ways as above. The #1302
banner was flipped by lifting the closed-alphabet character from an already-closed item and asserting
its membership before use -- never typed (sec. 11).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…orkflows (BACKLOG #321) The owner refreshed MEFOR_FORBIDDEN_TOKENS in both secret stores, so the floor can now assert the larger set. VERIFIED BY ME rather than taken from a relay -- names and dates only, no values, which is all `gh secret list` exposes: Actions MEFOR_FORBIDDEN_TOKENS 2026-08-21T12:45:15Z Dependabot MEFOR_FORBIDDEN_TOKENS 2026-08-21T12:45:23Z EIGHT SECONDS APART, so the half-done state this change was held for did not occur. If Actions had been updated and Dependabot had not, every Dependabot PR would hard-fail a required check. My stated constraint was "only after BOTH, and I will ask rather than infer" -- both are updated and the measurement is mine. RAISED IN TWO PLACES, NOT ONE. The release named `security.yml`. `branch-leak-scan.yml:88` carried the SAME literal and nobody named it. Raising only one would have left a second gate passing on the old floor -- a partial raise that reads as done. There are now zero occurrences of the old triple under .github/workflows/. PRE-FLIGHT BEFORE RAISING A FLOOR THAT HARD-FAILS A REQUIRED CHECK, counts only: names 8 estate 14 estate_file_scanned 13 site_prefixes 2 synthetic=False The real set satisfies 8/14/2 exactly, and `estate_file_scanned` at 13 matches the documented 12->13 move -- so the added token is FILE-SCANNED rather than body-only, which is the half that matters. NOT INERT, AND THAT IS CHECKED RATHER THAN ASSUMED. `token_floor_failure` passes at 8/14/2 and FAILS at 9/15/3 naming each short section ("names 8<9, estate 14<15, site_prefixes 2<3"). A floor that cannot fail is not a floor. EXPECT COLLATERAL HITS ON THE FIRST FULL SWEEP AND DO NOT READ THEM AS FINDINGS. The added site prefix is two digits, so it matches any delimited six-digit run in a 10000-wide band -- synthetic MRNs, sentinel ids, clamp ceilings. Triage noise, anticipated before the value was written. VERIFIED: 146 tests across the scanner + token-source + CI-pinning suites; 120 more across the workflow-lint and lockstep suites. No token value appears in this change, in any test, or in this message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ct without importing test_every_non_engine_test_is_classified caught this on CI (all 3 platforms, identical failure): PR 615's new test file imports scan_forbidden directly (a scripts/security/ module), not messagefoundry/harness/tee, so the partition guard correctly flagged it as unclassified rather than silently letting it run on every engine leg forever or drop off them entirely. Added to _STAYS_WITHOUT_IMPORTING alongside its sibling test_scan_forbidden.py, for the identical reason already documented there: it is a scanner test whose SUBJECT is engine-adjacent security tooling, read off disk rather than imported. Verified: tests/test_tooling_partition.py now 9/9 passing locally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # docs/BACKLOG.md
# Conflicts: # docs/BACKLOG.md
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
1 participant
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.
Part A of stranded PR #531 (
lander/builder-2-a12484-rebased), per Cleaner's grading(
.git/mefor-coord/handoffs/CLEANER-2026-08-26-FOR-LANDER-stranded-pr-grading.md). Five commits,cherry-picked onto a fresh branch off current main. Part B (the #1292 intake-audit squash) and
Part C (the sigstore hash-pin, UNKNOWN pending a cheap check nobody has run) are not in this PR.
HOLD ON ONE COMMIT --
170f986fcraises the leak-gate detector floor to 8/14/2 in bothworkflows. The code change is safe and mechanical; the risk is entirely in whether the real
MEFOR_FORBIDDEN_TOKENSsecret satisfies the new floor. No checkout holds that secret, so thiscannot be verified from here -- Cleaner's own precondition on this commit, still unresolved. If the
real token set does not meet 8/14/2, this fails the leak gate on every push and every non-fork PR
once merged. Not arming auto-merge on this PR for that reason. Land only after the floor is
confirmed against the live secret.
The other four commits, verified independently:
22460b6e7-- a zero-read engine reports its own reason instead of a bareassert 0 > 0. Clean,no ledger touch.
0c242c994-- per-class leak-gate coverage against the LOADED token set rather than amonkeypatched synthetic one (adds
tests/test_scan_forbidden_loaded_set.py). This is BACKLOGbacklog: amend #1020 and #1022 -- partial, not closed; both stay OPEN #321's Proposed 2, closing the exact gap backlog: amend #1020 and #1022 -- partial, not closed; both stay OPEN #321's own 2026-08-03 amendment names ("tests exercise
the machinery and never the loaded token set"). New test file, no conflict.
ce693091a-- three copies of a racy_find_free_pidhelper (spawn, wait for exit, sleep, returnthe pid as "free" -- free at that moment, nothing keeps it free) replaced by one
tests/_dead_pid.pyreturning2147483647, structurally unassignable on either platform.Renumbered from BACKLOG #1303 to #1360: #1303 was legitimately allocated 2026-08-21 but bound
to a worktree confirmed dead (
fleet.ps1: freshest record INTERRUPTED at 119+ hours; absent fromgit worktree list), and the ledger gate's allocation registry has no rebind mechanism for eitherADR or BACKLOG numbers (read
scripts/coord/alloc.ps1in full to confirm -- it only ever searchesforward for a free slot). Hand-editing the registry is correctly refused by a PreToolUse hook.
Renumbered rather than force the block; #1303 stays a permanent hole. Ledger banner is
Lander-authored per ADR 0165 (code is the builder's own verified work, landed unmodified) --
verified against the landed diff before writing it.
5151ee1ff--mail.ps1now refuses a wrong-namespace-ToSessionIdat send, before any messageis written, rather than only reporting the loss silently at the drain end. BACKLOG #1302's banner
flip to SHIPPED rides in the same commit -- this is a banner amendment on an item already on main
(filed 2026-08-21), which ADR 0165's own text says does not need Dispatcher/Lander re-authorship
(ownership is only consulted for a NEW
## N.heading), so it is carried as-is.Verified before pushing:
ruff check+ruff format --checkclean,mypy messagefoundryclean(267 files),
backlog_status_check.pyclean, and the full test suites for every touched file pass(177 tests, including the ~5-minute
test_worktree_prune_merged.pysubprocess suite run tocompletion).
Does not touch
lander/builder-2-a12484-rebaseditself.Co-Authored-By: Claude Opus 5 noreply@anthropic.com