fix(canary): freeze the marker pair, drop the unsound version relation - #5303
fix(canary): freeze the marker pair, drop the unsound version relation#5303sanity wants to merge 20 commits into
Conversation
Rule Review: Scope creep — PR bundles multiple unrelated logical changes under a "fix" titleRules checked: git-workflow.md Warnings
Info(none beyond the above) Rule review against |
sanity
left a comment
sanity
left a comment
There was a problem hiding this comment.
Comprehensive PR Review: #5303
Summary
- PR Title: fix(canary): pin MARKER_LATEST_SEEN_SINCE below the crate version, not above
- Type: fix
- CI Status: pending at time of review
- Review tier: Full (touches
.github/workflows/ci.yml— deploy/release/CI config is a listed high-risk surface) - HEAD SHA reviewed:
da1b3a44c851baaa118ac9ba78d2d5403dfda178 - Reviewers run: code-first, testing, skeptical, big-picture, plus a fifth lens specific to this change's failure mode (pin correctness: can it fail, will it fire spuriously). External models not run — opt-in only per the current review rule, so a fifth Claude lens was substituted rather than waiting on quota.
Verdict: Needs Changes — Re-review Required After Fix.
The PR's central claim is verified independently, by execution, by three separate reviewers: with the crate at 0.2.126, main's pin goes red (MARKER_LATEST_SEEN_SINCE (0.2.125) is BELOW the crate) and the PR's stays green. The landmine is real and the direction of the fix is right. The frozen fact was also checked against ground truth — the published v0.2.125 musl asset was downloaded and strings finds the marker.
But the review found that the replacement pin reintroduces the same defect class with the sign flipped, and that is blocking.
The blocking finding: no static relation is correct in both phases
main's pin (constant >= crate) was correct before publication and wrong after. This PR's pin (constant <= crate) is correct after publication and wrong during a marker reword — which the code explicitly anticipates (auto_update.rs and the source pin at _test.sh:864 both say "do not reword without updating both").
On a normal dev tree the crate version equals the last published release; the bump happens inside the release commit. So a marker reworded on main today first ships in crate+1. Then:
- constant = crate+1 (the correct value) → relation pin RED, blocks a healthy release
- constant = crate (green) → Gate B demands the NEW text from the published previous binary, which emits the OLD text → Gate B goes spuriously red post-publish, on the Matrix channel
cross-compile.yml:764alerts
Green test means broken canary; correct constant means red test. Verified by execution in an isolated worktree.
The conclusion is not to tune the inequality — it is that no static relation survives both phases. Two reviewers independently proposed relation tweaks (<= crate+1, and a conditional form); the lens that tested them withdrew its own suggestion after confirming it also fires on a reword.
Fix: delete the relation pin, keep the frozen literal. The frozen literal is phase-independent: it pins a value and forces a deliberate co-edit for any change, reword included. Deleting it also removes a second spurious fire found in testing: version.workspace = true in crates/core/Cargo.toml makes the relation pin fail with "could not read the crate version".
Must Fix (Blocking)
- Delete the
constant <= crate versionrelation pin (scripts/auto-update-canary_test.sh:775), keep the frozen literal. Reason above. - Add
gate_b_arm_case "0.2.124" no(_test.sh:~479). Verified vacuity hole: replacing the decision body with a hardcodedversion_at_least "$1" "0.2.124"leaves the entire suite green — nothing pins that the decision reads the constant rather than a literal. The existing cases straddle the gap (0.2.123 no / 0.2.125 yes), so any threshold in {0.2.124, 0.2.125} satisfies all of them. Verified both ways: red against the hardcode, green against real code, and it survives a legitimate reword so it adds no spurious-fire surface. Also correct the annotation "the one release that must skip" — two releases skip now. - Reword the frozen pin's failure message (
_test.sh:817-819). It currently instructs the reader to "change the constant and this expectation together, in one commit" — which is exactly the creep edit that disarms Gate B. Verified: constant + frozen literal + crate all moved to 0.2.126 leaves the suite fully green while Gate B skips its positive check (runtime-confirmed:constant=0.2.126, prev=0.2.125 -> SKIP). The failure message should not read as instructions for the failure mode. - Restore the reword escape hatch in the operator-facing comment (
scripts/auto-update-canary.sh:173-175). This PR narrowed it, from "…or this constant is set one release too early (bump it)" to "…not a reason to touch this constant". Under a reword, touching the constant is the correct response, and an on-call reader debugging a red Gate B lands here, not in the test file. - Fix a factual error in the CI comment (
.github/workflows/ci.yml:436): "auto-update-canary_test.shhas no helper at all" is false.check()is defined at_test.sh:55with 12 call sites. Accurate statement:check()is specialised to log-fixture cases and the file's other assertions are bare echoes. Same error is in the PR body and will be corrected there too. - Update the user-facing error text (
ci.yml:448). It still says "an added'check'assertion", naming one of three accepted forms, so a developer who trips the gate is told to add the wrong thing. This is the same misleading message that sent this PR's author down the wrong path. - Tighten the new regex from
"okto"ok[[:space:]]+-(ci.yml:443). Unanchored, it countsecho "okay, the suite is starting"and bareecho "ok"as regression tests. Costs nothing: all 30echo "oksites in the repo use theok -form.
Should Fix
- Add
passto the regex alternation. One word, and it closes four more shell self-tests that currently score zero (release_state_restore_test.sh,release_canary_wiring_test.sh,release-agent/deploy-local-gateway_test.sh,release-agent/gateway-auto-update_test.sh). The PR predicts a "fourth pattern" and defers it; this is the cheap part of that set. - Reconcile contradictory adjacent sentences in
auto-update-canary.sh:169-182: "makes this self-retiring" now sits beside "FROZEN … release history does not change". - PR body says "Full canary suite 32/32"; the suite has 61 assertions. The body's weight rests on exact evidence, so the number should be right.
Consider / Follow-ups
- Reconciling two reviewers who disagreed. Big-picture argues the version gate is now dead code (once 0.2.125 is published,
prev >= constantalways, so the skip branch is unreachable) and should be deleted, making Gate B's positive assertion unconditional. That is appealing but not safe as stated, and the blocking finding is why: the constant is precisely the mechanism that makes a marker reword survivable. Delete it and a reword release fails Gate B with no way to express "the previous binary legitimately emits different text". Recommend keeping the constant; if the gate is revisited, handle the reword case explicitly (e.g. the canary accepting the previous release's marker text). - One shared assertion helper or marker convention across all shell self-tests, replacing the regex-widening cycle. This is the third widening; two further unmatched idioms already exist.
- Add the self-detonating-pin class to
.claude/rules/bug-prevention-patterns.md. "A pin whose premise is this release is not yet published detonates the moment it publishes" generalises beyond this file, and right now the lesson lives only in a comment inside one test file. Also list theMARKER_LATEST_SEEN_SINCEpins in the enumeration at:153-157. - For whoever cuts 0.2.126: Gate B's positive equality check has never armed in production (it was skipped for 0.2.125 because prev was 0.2.123). 0.2.126 is the first release where it runs for real, so a previously-unexercised path goes live at release time.
What holds up
- The landmine is real, reproduced independently three times.
- The frozen literal does the load-bearing work: forward creep at release time is invisible to the relation pin and caught only by the freeze.
- The "two pins are complementary" claim survived a deliberate attempt to refute it (one reviewer reported the failed refutation, which is the right thing to do).
- Neighbouring untouched pins were audited and are live, each reddened by mutating its subject. Every whole-file-grep marker has its only hits in production code above the
#[cfg(test)]boundary, so none is currently satisfiable by a comment or test block. shellcheck -xclean; suite 61/61 green at HEAD.
Process note
The shared review worktree was briefly dirty with one lens's mutation edits while others were reading it — my error in co-locating them, corrected mid-review. Three reviewers noticed independently and re-ran against clean git archive exports, and both worktrees were verified clean afterwards. Findings above were taken from those clean runs. Flagging it because it could have produced phantom findings and did not only because the reviewers caught it.
[AI-assisted - Claude]
Review addendum: two corrections to my synthesis above, and one new findingThe fifth lens reported after I posted. It corrects the consolidated review in two places, both in the direction of "the problem is worse than stated", and adds a finding none of the other four had. Recording it here rather than editing the review above, so the correction is visible. Correction 1 — the reword failure is SILENT, not a "tempting escape"My review described a developer facing a red pin and taking a bad escape. That framing is wrong, and the truth is worse. Verified by rewording So nobody is tempted, because nobody is prompted. A stale constant is the DEFAULT outcome of a reword, not an escape from a warning. The source pin at Severity also shifts. The old landmine fired in CI on the bump PR: cheap, pre-publish, with a message naming the constant. This one fires in the post-publish canary ( This changes the recommended fix. In addition to deleting the relation pin, freeze the PAIR: add Correction 2 — the "delete the version gate" argument is refuted, and my reason was the weaker oneI argued deletion was unsafe because the constant is what makes a reword survivable. That holds, but there is a stronger and independent reason I missed.
So if 0.2.125 were ever retracted or re-drafted, Big-picture is right that the branch is dormant and wrong that it is dead. Dormant-but-correct is what a valve looks like. New finding —
|
…gether Addresses the Full-tier review on #5303. The relation pin this PR added was wrong in the same way `main`'s was, with the sign flipped, and the review found a silent failure neither pin could see. DELETE THE RELATION PIN (review 1b). Two have now been tried and both were correct in one phase and wrong in another. `constant >= crate` (#5290) rested on "no published release emits the marker yet" and detonated the moment 0.2.125 published. `constant <= crate` is right after publication and wrong during a marker REWORD: a marker reworded on main first ships in crate+1, so the correct constant is crate+1, which the pin calls a failure -- while the value that makes it green makes Gate B demand the new text from a published binary emitting the old one, i.e. a post-publish red canary and a Matrix alarm blaming the node. A `<= crate+1` variant fires on a reword too; the reviewer who proposed it withdrew it after testing. The generalisation, recorded in the comment so a third attempt is not made: the constant tracks RELEASE HISTORY and the crate version tracks THIS TREE, and no fixed relation between two quantities on different clocks holds across both publication and reword. The freeze is phase-independent and covers strictly more -- the relation only ever caught "strictly above crate version", while the freeze catches every wrong value including the empty string and the most likely wrong one. It also cannot be tripped by `version.workspace = true`, which broke the relation pin's Cargo.toml read. FREEZE THE PAIR (review 1a, the highest-value change). Reword MARKER_LATEST_SEEN everywhere a developer must touch it and leave the constant alone, and the whole suite passes -- verified, zero red. The source pin interpolates `$MARKER_LATEST_SEEN`, so it follows the rename by construction, and nothing else looks at the marker. A stale constant is the DEFAULT outcome of a reword rather than an escape from a warning. `MARKER_LATEST_SEEN_FROZEN` now sits beside the version freeze, so a reword fails in the same edit with the version constant named. version_at_least ACCEPTS GARBAGE (review, new finding). Measured: `"not-a-version" >= "0.2.125"` was TRUE (non-numeric sorts after digits under `sort -V`) and `"0.2.125" >= ""` was TRUE. Both now refuse, loudly, via `is_dotted_version`. Pre-release tags are refused rather than ordered, because `sort -V` puts `0.2.125-rc.1` above `0.2.125` where semver puts it below. `prev_emits_latest_seen` deliberately goes the OTHER way on malformed input: it ARMS Gate B rather than skipping. The two functions answer different questions. "Is a >= b" has no answer for garbage, so the comparator refuses; "should Gate B assert anything" does, and skipping is the silent direction this file exists to remove. Also from the review: 2. `gate_b_arm_case "0.2.124" no`, plus the corrected annotation (two releases skip, not one). Verified vacuity hole: the old cases straddled the gap, so hardcoding the threshold to 0.2.124 inside the decision left the entire suite green. 3. The frozen pin's failure message no longer reads as instructions for the creep edit that disarms Gate B. It now says the fix is almost certainly to put the value back, and points at the reword assertion above it. 4. The operator comment in auto-update-canary.sh regains the reword escape hatch this PR had narrowed away, and no longer says "self-retiring" beside "FROZEN" (review 9). 6. The Rule Lint error text named `check`, one of four accepted forms, so a developer tripping the gate was told to add the wrong thing. It now lists all four and names the two known blind spots. 7. `"ok` tightened to `["']ok[[:space:]]+-`, and `pass` added. Measured on a fixture of eight lines: `echo "okay, the suite is starting"` and a bare `echo "ok"` no longer count, while single-quoted `echo 'ok - ...'` and the `pass` form now do (four more self-tests stop scoring zero). This PR's own diff still scores 2 against origin/main.
sanity
changed the title
sanity
changed the title
sanity
left a comment
sanity
left a comment
There was a problem hiding this comment.
Re-review: #5303 at 98196ec69
Mandatory re-review after seven blocking findings were addressed. Four independent lenses, each in its own isolated worktree this time (last round's shared checkout was briefly dirty with one lens's mutations, which three reviewers noticed and worked around).
- Review tier: Full (CI config). External models opt-in only per the current rule; four Claude lenses with distinct adversarial angles ran instead.
- HEAD reviewed:
98196ec69.mainhas not moved since the branch point, so this is the content that would merge. - Verdict: Needs Changes — Re-review Required After Fix.
What holds up, verified independently and repeatedly
Between them the lenses executed roughly forty mutations. The following were attacked and could not be broken:
- The base64 marker freeze does what it claims. A
sedrename sweep across all four files holding the marker goes RED. A plaintext expectation would have been rewritten by the same sweep — it rewrote three other occurrences in that very file — so the encoding is doing real work. - Decode failure is loud in every mode: empty, corrupt, deleted assignment, and a stubbed-out
base64binary all produce a clean FAIL via the-zguard, never a vacuous comparison. Noset -e, so no mid-suite abort. - Deleting the relation pin lost nothing. The freeze reddens for constant
0.2.999,'',not-a-version,0.2.100,0.2.126— every state the relation caught and several it did not. The self-detonation claim was ground-truthed: crate at 0.2.126 leaves this HEAD green and reddensmain's pin. - The frozen value is true. The published v0.2.125 musl asset was downloaded and
strings-grepped: the marker is present. - Every source-scrape pin passes the AGENTS.md bounded-region rule.
preflight_body/selfupdate_bodyare awk-bounded with an explicit empty-body FAIL;pin_markerbinds to the emitting macro call, not a whole-file grep; every scrape is cross-file. No bare unbounded-anchor pin exists in the file. That is a clean bill on this repo's most common defect. - All eight new assertions go red under the mutation each one names.
Must Fix
1. The "pair freeze" does not force the pair, and the escape is its own error message. Found independently by three lenses. Two-step mutation: (a) reword sweep → RED, correctly; (b) then run only the copy-pasteable command the FAIL hands you, regenerating the encoded blob, and leave MARKER_LATEST_SEEN_SINCE alone → suite fully green, constant stale. That is exactly the state the pin exists to prevent. The two frozen values are independent assertions; the coupling is prose sitting directly above a one-liner that silences the alarm without honouring it.
The generalisation, which is the most useful thing this review produced: a freeze forces a decision only if the remediation cannot be performed without making that decision. Fix: encode both values as one blob, so regenerating the expectation is impossible without re-stating the version.
2. The freeze protects one of six markers, and the unprotected set includes the silent direction. Gate B's assert_detection_healthy runs over the previous release's binary, so the cross-version hazard belongs to Gate B, not to one constant. Verified: the identical sweep applied to MARKER_PARSE_FAIL or MARKER_CHECK_RAN leaves the suite green.
Directions differ and one is much worse. A MARKER_CHECK_RAN reword makes Gate B red against a healthy release — loud, same harm class as the bug being fixed. A MARKER_PARSE_FAIL reword makes Gate B's negative check stop matching, so a genuinely broken previous release passes. Silent false-pass, and that is the #5221 signature this canary exists to catch. Freeze that one here (it needs no version constant); scope the comment honestly for the rest; follow-up issue for the others.
3. An unreadable constant now makes Gate B skip silently — a regression in the direction this PR is about. prev_emits_latest_seen guards $1 only, so an emptied or corrupt MARKER_LATEST_SEEN_SINCE falls through to version_at_least, which refuses, which skips. Verified: on main the same input armed. The comment's claim that an unusable version arms rather than skips is true only of $1. The resulting note reads "first emitted by v" with an empty version. Defence-in-depth only — CI's freeze catches a bad constant first — but it is the one input class the code claims to handle. One line: test both operands.
4. The CI comment claims coverage that does not exist. Measured: the widening fixes three of the four files it names. release_state_restore_test.sh reports echo "PASS [$name]" (uppercase) and scores 0 before and after. A gate whose own documentation asserts coverage it lacks is the defect class this PR is about.
Worse, the dominant blind spot is undocumented while a hypothetical one is: [[:space:]] after the alternation excludes every underscore-suffixed helper (gate_b_arm_case, version_ge_case, trigger_case, pin_marker, …), 29 of 63 sites in the canary test alone. 4 of this PR's own 8 new assertions score zero; its count of 2 comes entirely from the freeze block's echo lines. Meanwhile the documented printf false negative does not occur anywhere in the repo. Swap them, and add the helper-call-site pattern.
Should Fix
5. A load-bearing rationale claim is arithmetically false — and it was mine. The description claimed <= crate + 1 "also fires on a reword". It does not: under a reword the correct constant is crate+1, and crate+1 <= crate+1 passes. The claim entered this PR because I relayed a reviewer's withdrawal of their own suggestion without checking the arithmetic behind it. Corrected in the description.
This matters because that claim is the stated reason no bound of any kind survives, which leaves the "wrong-but-consistent value" case unguarded — both frozen and live values are edited by the same hand in the same edit. A pure upper bound appears phase-independent (violated only when the constant runs more than one release ahead, which is never legitimate). Given that three relations in this file have now been wrong, it is to be mutation-tested across four phases before being trusted, not asserted on reasoning.
6. gate_b_arm_case "0.2.124" does not pin what its comment says. Verified: hardcoding 0.2.124 → red, 0.2.126 → red, the correct literal 0.2.125 → green. It pins "the threshold is exactly 0.2.125", not "the decision reads the constant". Two subshell cases fix it, or downgrade the claim.
7. Three of five is_dotted_version refusal clauses are unpinned — deleting .*, *., *..* leaves the suite green.
Follow-ups
- The structural closure for finding 1: a networked check that downloads the published
v$MARKER_LATEST_SEEN_SINCEmusl asset and greps it for$MARKER_LATEST_SEEN. This is the only assertion that cannot be satisfied by editing the test file — reword-and-regenerate leaves the published binary still emitting the old text. It was already performed by hand during this review; making it a gate is the structural version. Belongs beside the canary jobs incross-compile.yml, since it needs network. - Freeze the remaining Gate B markers.
- Add both lessons this PR earned to
.claude/rules/bug-prevention-patterns.md: a pin on a relation between two quantities on different clocks is right in one phase and wrong in another; and an expectation stored as a plaintext copy of the value it guards is rewritten by the same edit that changes the value. docs/RELEASING.md"If Gate B fails" goes straight to "ship a fix release, roll nodes by hand". This PR's subject is a Gate B failure that is not a fleet problem, and that is the page an operator reads at 1am.
Note on the process
This PR has now been wrong four times in the same way — the original pin, its first replacement, the plaintext freeze, and the pair coupling. Each was found by applying the mutation the previous round named, to the fix the previous round produced. That is the practice working rather than failing: two of the four were caught by the author mutation-testing their own new pin and reporting the failure unprompted. It is worth stating plainly that the code is a real improvement over main on every axis it targets at every one of those iterations.
[AI-assisted - Claude]
…two claims Re-review round 1 on #5303. Two of the six items were factual errors in my own comments; the other four are the fixes they imply. FALSE COVERAGE CLAIM (item 1). The blind-spot inventory in ci.yml named `release_state_restore_test.sh` as reporting through `pass`. It does not -- it reports `echo "PASS [$name]"` and scored ZERO under the new pattern. A false claim of coverage inside a comment whose stated purpose is honest disclosure of blind spots is worse than an ordinary comment error. The alternation is now derived from an ENUMERATION of every `*_test.sh` under scripts/, not from whichever file was in front of me: `check` (4 files), `ok`/`bad`, `pass` (3 files), `echo "ok - "` (34 sites, all 34 using that exact separator, so anchoring on it costs nothing), and `echo "PASS [...]"` (the last file scoring zero). Measured counts replace three hedged approximations that read as measured: "~49 assertions" is 56, "~30 sites" is 34, "four files use pass" is three. FALSE REFUTATION (found in the PR body, propagated into my code comment). I wrote that a `<= crate+1` relation "fires on a reword too". It does not: during a reword the correct constant is crate+1, and crate+1 <= crate+1 passes. The claim came from a reviewer who withdrew their own suggestion on that basis and I repeated it without checking a single comparison. The comment now states the arithmetic, scopes the "no relation works" conclusion to EQUALITY-STYLE relations, and records that such a bound is deliberately omitted for redundancy rather than unsoundness -- with an instruction not to re-cite the refuted claim. FROZEN AS ONE BLOB. The two-adjacent-assertions form failed the remediation path: reword goes red, then following the pin's own failure message (regenerate the encoded text) goes green again with the version constant untouched, because the message never asked about it. A freeze forces a decision only if the remediation cannot be performed without making that decision. Both values are now one base64 blob, so the recipe cannot be run without supplying a version. The comment states the residual honestly: this makes the question unavoidable, it does not verify the answer -- regenerating with the new text and the old version still passes, measured. Also: 2. Pin that `prev_emits_latest_seen` READS the constant. Hardcoding the current value inside it left the whole suite green, freeze included, because the constant was untouched and the freeze had nothing to disagree with. It matters exactly when the constant is supposed to move. 3. `base64 -w0` is GNU-only and fails on macOS, which is where someone reading this failure is most likely to be. The printed recipe now uses `base64 | tr -d '\n'`. 4. The comment no longer implies the reword class is closed. Gate B greps four other markers against the PREVIOUS release's binary -- MARKER_DISABLED, MARKER_CHECK_RAN, MARKER_CHECK_COMPLETE, MARKER_TRIGGERED_RE -- and rewording any of them produces the same post-publish false alarm with a worse message. Named as residual, with the legitimate asymmetry (Gate A reads a binary built from this tree, so a reword there is self-consistent). Follow-up to be filed. 5. `docs/RELEASING.md` "If Gate B fails" went straight to "ship a fix release, roll nodes by hand". This PR's whole subject is a Gate B failure that is NOT a fleet problem, and that page is what an operator reads at 1am. It now splits the two cases, gives the grep that distinguishes them, and warns that the Matrix alarm names the worse one. `.claude/rules/bug-prevention-patterns.md` gains the two lessons this PR earned: a relation pin between quantities on different clocks, and an expectation stored as a plaintext copy of the value it guards (with the remediation-path corollary). 6. Narration compressed. The generalisations stay; the round-by-round history of this PR's own reviews goes. A recorded mutation RESULT is durable, a record of which review round produced it is not. The ci.yml block stated its blind-spot list twice; only the error string, which is what a contributor actually sees, keeps the full list. The operator-facing comment in auto-update-canary.sh is deliberately NOT compressed: it is the escape hatch item 4 of the previous round asked to restore, and it is where an on-call reader lands.
sanity
left a comment
sanity
left a comment
There was a problem hiding this comment.
Review round 3: #5303 at f360228d8
Three independent lenses, isolated worktrees. Verdict: Needs Changes. The round-2 defect is confirmed fixed; three new ones were found, and one behavioural harness closes most of them.
Confirmed fixed, by a reviewer other than the author
The round-5 defect — a single-value freeze that could be silenced by following its own recipe — holds now. A reviewer swept failed to parse → could not parse across all four files, watched it go red, then did exactly what the failure message says and nothing else. Still red, on the behavioural backstop. That is the property the previous four rounds all failed. The backstop was also checked for faithfulness against v0.2.121:auto_update.rs:1528 rather than assumed.
Must fix
1. The retry loop's environmental flag is last-writer-wins. A hung updater on attempt 1 (a real finding, as the code's own comment says) followed by a port collision on attempt 2 yields exit 75 and the quiet "This is NOT a stranded fleet and needs no fleet action." A false-quiet on the exact class the gate exists to raise. Latch the flag: once any attempt is a non-environmental indeterminate, the verdict is fault.
2. A real #5221 can reach the quiet message via a reorder nothing pins. As shipped it cannot — a log holding both markers returns rc=1 because the parse-fail branch precedes the fetch-fail branch. That ordering is the only thing making the split safe, and no fixture in the suite holds both markers. Swap the branches (an innocent "infra check before product check" tidy) and the same log classifies environmental. One check case closes it.
3. "Retry only the environmental case" is pinned by nothing. Mutating [ "$rc" -eq 2 ] || break to also retry rc=1 leaves all three suites green. Deterministic faults still fail, but an intermittent real fault becomes a pass — a regression arriving as a flaky pass through the canary's own retry logic.
4. The classifier is pinned by substring presence only. Flipping && to ||, or keeping the call and ignoring its result, leaves everything green while every indeterminate becomes environmental. This is the exact shape the round-2 prev_emits_latest_seen fix closed, reintroduced one function over — and the pin's own failure message names this hazard and cannot see it.
1–4 are all closed by one thing, which both lenses proposed independently: a behavioural driver for cmd_selfupdate (stub curl/tar/run_node_until_check, plant fixture logs, assert attempt count and returned code). release_wait_for_binaries_test.sh:211 is the in-repo precedent.
5. A port collision is reported as "could not reach GitHub" in both the ::error:: and the Matrix message. The node never started. Gate A kept a dedicated port-collision message; Gate B lost it this round. Carry the cause, not just the flag.
6. The new self-test is invisible to the counter it guards. 26 bare expect sites; the pattern requires expect_. So ci.yml's "329 of 329, zero false positives" is false at this HEAD, and the discipline it prescribes ("re-run the enumeration before adding a pattern") was not followed by the commit that prescribes it.
7. The Gate B wiring pin has a hole. exit "$rc" is pinned; || rc=$? is not. Changing the latter to || true — the reflex fix — leaves the suite green while Gate B reports success on a broken updater.
Should fix
CANARY_ATTEMPTS=00passes the zero guard, the loop never runs, andrcis unbound → exit 1 → the loud alarm.- The
75constant and the workflow's literalif [ "$rc" -eq 75 ]are coupled and unpinned. - The backstop's fixture yields rc=2, not the rc=0 its message claims, because it omits the completion line that a real #5221 log carries.
MARKER_FETCH_FAILnow feeds a classification decision, not just a diagnosis, so a reword silently disables the environmental path and sends every blip back to the loud alarm. Comment corrected here; the freeze goes to #5309 as its top item. (Deliberately not frozen in this PR: its reword direction is a loud false alarm, not a silent pass.MARKER_PARSE_FAILwas frozen here precisely because its direction is silent.)- The body describes roughly 40% less than the diff and is being rewritten.
Deliberately NOT doing
No behavioural backstop for MARKER_LATEST_SEEN. After a legitimate reword the canary is meant to stop reading old binaries and skip, so such a pin would fire on the correct remediation. Raised unprompted by a reviewer; agreed. Worth recording so the lesson from MARKER_PARSE_FAIL is not over-applied.
Verified sound — please do not re-litigate
The vacuity guard is layered and the layer that matters works (forcing the exact first-draft FRAG='' bug is caught by 11 negative cases). Zero false positives across all 54 new pattern hits repo-wide. The notify split survives step reorder, reformatting, condition widening, and output deletion; cancelled/empty/never-set all land loud. Exit 75 propagates end to end. Every version-gate pin reddens under its named mutation, and the 0.2.124 straddle is confirmed closed. A real #5221 cannot currently be misclassified (marker ordering verified with a log containing both).
One thing nobody can test locally
Whether GitHub populates needs.<job>.outputs for a failed job. It is load-bearing for the whole notify split. If it comes back empty, everything takes the loud branch — so it degrades safely — but it needs watching on the first real tag run, and that belongs in the body.
Five of this PR's defects have now been found inside the fixes for earlier ones. That is worth stating plainly rather than hiding: each was caught by applying the mutation the previous round named, and two were caught by the author against their own work before anyone asked. The code is a real improvement over main at every iteration.
[AI-assisted - Claude]
…t above The self-test added in #5290 asserted the constant was NOT BELOW the crate version. Its premise was stated in its own comment -- "the marker is new in this tree, so no already-published release emits it" -- and that premise expired when 0.2.125 was published. The constant is a historical fact and correctly stays at 0.2.125, so the pin would have gone red on the 0.2.126 bump and blocked a healthy release. Verified before the change by setting the crate version to 0.2.126: the old pin failed. The relation worth pinning is the opposite one, because only one direction is silent. `prev_emits_latest_seen` compares the PREVIOUS release against the constant, and the previous release is always below this tree's version, so a constant set ABOVE the crate version can never arm: Gate B skips its only positive assertion on every release and still reports success. At or below, the check runs. So the assertion is now `constant <= crate version`, which no legitimate release can trip. That relation alone permits the constant to creep forward with each release, which passes every assertion here and disarms Gate B every time. So the value is also frozen against its historical literal, with the reason: it records which published release first emitted the marker, and release history does not change. Changing it takes a deliberate two-line edit. Also drops the same-major.minor and +5-patch-window clauses, both of which were expressions of the expired premise, and replaces the duplicated `gate_b_arm_case "0.2.125"` (the constant's own value) with 0.2.126, which is what "every release after it" was meant to cover. Refs #5290
Third instance of the same gap. The fix:-PR test requirement scores shell self-tests by counting added assertion lines, and it has now missed the assertion form of each file it was pointed at in turn: first `check` only (missed the lifecycle test's `ok`/`bad` helpers, #5290), now `check|ok` (misses `auto-update-canary_test.sh`, which has no helper and writes each assertion as a bare `echo "ok - ..."`). That file is the one guarding the release canary, so a PR whose entire regression test is a new assertion there scores zero and is told to add a test it has already added. Measured on this branch's own diff with the exact CI pipeline: before: 0 after: 2 Refs #5290
The comment claimed the file "has no helper at all". False: `check()` is defined at the top of it with 12 call sites. What is true is narrower -- `check()` takes a log fixture and drives `assert_detection_healthy`, so the file's source pins and constant-relation pins have nothing to fixture and are written as bare `echo "ok - ..."` lines. Also states plainly what the matcher does, having measured it: it recognises the success-REPORTING line of an assertion, by prefix, and never recognised assertions. A decorative `echo "ok - fine"` satisfies it -- but so did `ok "..."` and a self-comparing `check "x" "a" "a"` under the previous pattern. Measured on three assert-nothing lines: the old pattern matched 2 of 3, the new one 3 of 3. The widening adds a spelling, not a weakness. What stops a decorative test is review and mutation-testing the pin, not this regex.
…gether Addresses the Full-tier review on #5303. The relation pin this PR added was wrong in the same way `main`'s was, with the sign flipped, and the review found a silent failure neither pin could see. DELETE THE RELATION PIN (review 1b). Two have now been tried and both were correct in one phase and wrong in another. `constant >= crate` (#5290) rested on "no published release emits the marker yet" and detonated the moment 0.2.125 published. `constant <= crate` is right after publication and wrong during a marker REWORD: a marker reworded on main first ships in crate+1, so the correct constant is crate+1, which the pin calls a failure -- while the value that makes it green makes Gate B demand the new text from a published binary emitting the old one, i.e. a post-publish red canary and a Matrix alarm blaming the node. A `<= crate+1` variant fires on a reword too; the reviewer who proposed it withdrew it after testing. The generalisation, recorded in the comment so a third attempt is not made: the constant tracks RELEASE HISTORY and the crate version tracks THIS TREE, and no fixed relation between two quantities on different clocks holds across both publication and reword. The freeze is phase-independent and covers strictly more -- the relation only ever caught "strictly above crate version", while the freeze catches every wrong value including the empty string and the most likely wrong one. It also cannot be tripped by `version.workspace = true`, which broke the relation pin's Cargo.toml read. FREEZE THE PAIR (review 1a, the highest-value change). Reword MARKER_LATEST_SEEN everywhere a developer must touch it and leave the constant alone, and the whole suite passes -- verified, zero red. The source pin interpolates `$MARKER_LATEST_SEEN`, so it follows the rename by construction, and nothing else looks at the marker. A stale constant is the DEFAULT outcome of a reword rather than an escape from a warning. `MARKER_LATEST_SEEN_FROZEN` now sits beside the version freeze, so a reword fails in the same edit with the version constant named. version_at_least ACCEPTS GARBAGE (review, new finding). Measured: `"not-a-version" >= "0.2.125"` was TRUE (non-numeric sorts after digits under `sort -V`) and `"0.2.125" >= ""` was TRUE. Both now refuse, loudly, via `is_dotted_version`. Pre-release tags are refused rather than ordered, because `sort -V` puts `0.2.125-rc.1` above `0.2.125` where semver puts it below. `prev_emits_latest_seen` deliberately goes the OTHER way on malformed input: it ARMS Gate B rather than skipping. The two functions answer different questions. "Is a >= b" has no answer for garbage, so the comparator refuses; "should Gate B assert anything" does, and skipping is the silent direction this file exists to remove. Also from the review: 2. `gate_b_arm_case "0.2.124" no`, plus the corrected annotation (two releases skip, not one). Verified vacuity hole: the old cases straddled the gap, so hardcoding the threshold to 0.2.124 inside the decision left the entire suite green. 3. The frozen pin's failure message no longer reads as instructions for the creep edit that disarms Gate B. It now says the fix is almost certainly to put the value back, and points at the reword assertion above it. 4. The operator comment in auto-update-canary.sh regains the reword escape hatch this PR had narrowed away, and no longer says "self-retiring" beside "FROZEN" (review 9). 6. The Rule Lint error text named `check`, one of four accepted forms, so a developer tripping the gate was told to add the wrong thing. It now lists all four and names the two known blind spots. 7. `"ok` tightened to `["']ok[[:space:]]+-`, and `pass` added. Measured on a fixture of eight lines: `echo "okay, the suite is starting"` and a bare `echo "ok"` no longer count, while single-quoted `echo 'ok - ...'` and the `pass` form now do (four more self-tests stop scoring zero). This PR's own diff still scores 2 against origin/main.
…annot follow it The marker freeze added in the previous commit did not survive its own mutation test. A reword is done as a `sed` sweep over the files that mention the marker -- which is exactly the mutation the review used -- and that sweep rewrote the frozen EXPECTATION along with everything else. Suite green, marker reworded, `MARKER_LATEST_SEEN_SINCE` stale: the silent path the freeze exists to close was still open, one level further out. This is the self-satisfying-pin class from `.claude/rules/bug-prevention-patterns.md`, reached by a different route: not an anchor that slides, but an expectation stored as text identical to the value under test, so any rename of that value renames the expectation too. The source pin has the same property by construction (it interpolates `$MARKER_LATEST_SEEN`), and this pin was added specifically to compensate for that -- while sharing the weakness. Stored base64 instead. A text sweep cannot reach it; a deliberate reword regenerates it, which is the intended cost. The failure message prints the decoded old text, the new text, and the exact command to re-encode. An empty decode fails loudly rather than making the comparison vacuous. The version half stays plain text, with the asymmetry documented: the edit that would rewrite it is a repo-wide sed on a version string, and version bumps here touch Cargo.toml and the lockfile rather than these scripts. The realistic wrong edit for it is a human raising it deliberately, which a plain comparison catches. Mutation-tested: reword the marker in auto_update.rs, freenet.rs, the canary and both test files, leave the constant alone. Before this commit the suite passed with zero red. After it, the marker assertion fails and names both the old and new text.
…two claims Re-review round 1 on #5303. Two of the six items were factual errors in my own comments; the other four are the fixes they imply. FALSE COVERAGE CLAIM (item 1). The blind-spot inventory in ci.yml named `release_state_restore_test.sh` as reporting through `pass`. It does not -- it reports `echo "PASS [$name]"` and scored ZERO under the new pattern. A false claim of coverage inside a comment whose stated purpose is honest disclosure of blind spots is worse than an ordinary comment error. The alternation is now derived from an ENUMERATION of every `*_test.sh` under scripts/, not from whichever file was in front of me: `check` (4 files), `ok`/`bad`, `pass` (3 files), `echo "ok - "` (34 sites, all 34 using that exact separator, so anchoring on it costs nothing), and `echo "PASS [...]"` (the last file scoring zero). Measured counts replace three hedged approximations that read as measured: "~49 assertions" is 56, "~30 sites" is 34, "four files use pass" is three. FALSE REFUTATION (found in the PR body, propagated into my code comment). I wrote that a `<= crate+1` relation "fires on a reword too". It does not: during a reword the correct constant is crate+1, and crate+1 <= crate+1 passes. The claim came from a reviewer who withdrew their own suggestion on that basis and I repeated it without checking a single comparison. The comment now states the arithmetic, scopes the "no relation works" conclusion to EQUALITY-STYLE relations, and records that such a bound is deliberately omitted for redundancy rather than unsoundness -- with an instruction not to re-cite the refuted claim. FROZEN AS ONE BLOB. The two-adjacent-assertions form failed the remediation path: reword goes red, then following the pin's own failure message (regenerate the encoded text) goes green again with the version constant untouched, because the message never asked about it. A freeze forces a decision only if the remediation cannot be performed without making that decision. Both values are now one base64 blob, so the recipe cannot be run without supplying a version. The comment states the residual honestly: this makes the question unavoidable, it does not verify the answer -- regenerating with the new text and the old version still passes, measured. Also: 2. Pin that `prev_emits_latest_seen` READS the constant. Hardcoding the current value inside it left the whole suite green, freeze included, because the constant was untouched and the freeze had nothing to disagree with. It matters exactly when the constant is supposed to move. 3. `base64 -w0` is GNU-only and fails on macOS, which is where someone reading this failure is most likely to be. The printed recipe now uses `base64 | tr -d '\n'`. 4. The comment no longer implies the reword class is closed. Gate B greps four other markers against the PREVIOUS release's binary -- MARKER_DISABLED, MARKER_CHECK_RAN, MARKER_CHECK_COMPLETE, MARKER_TRIGGERED_RE -- and rewording any of them produces the same post-publish false alarm with a worse message. Named as residual, with the legitimate asymmetry (Gate A reads a binary built from this tree, so a reword there is self-consistent). Follow-up to be filed. 5. `docs/RELEASING.md` "If Gate B fails" went straight to "ship a fix release, roll nodes by hand". This PR's whole subject is a Gate B failure that is NOT a fleet problem, and that page is what an operator reads at 1am. It now splits the two cases, gives the grep that distinguishes them, and warns that the Matrix alarm names the worse one. `.claude/rules/bug-prevention-patterns.md` gains the two lessons this PR earned: a relation pin between quantities on different clocks, and an expectation stored as a plaintext copy of the value it guards (with the remediation-path corollary). 6. Narration compressed. The generalisations stay; the round-by-round history of this PR's own reviews goes. A recorded mutation RESULT is durable, a record of which review round produced it is not. The ci.yml block stated its blind-spot list twice; only the error string, which is what a contributor actually sees, keeps the full list. The operator-facing comment in auto-update-canary.sh is deliberately NOT compressed: it is the escape hatch item 4 of the previous round asked to restore, and it is where an on-call reader lands.
The residual named in the previous commit now has an issue. Rewording MARKER_DISABLED, MARKER_CHECK_RAN, MARKER_CHECK_COMPLETE or MARKER_TRIGGERED_RE produces the same post-publish false alarm as a MARKER_LATEST_SEEN reword, with a less specific message, and only MARKER_LATEST_SEEN is frozen.
The canary's only NEGATIVE check greps MARKER_PARSE_FAIL. Rewording it fails SILENTLY in the passing direction: Gate B puts the grep to the PREVIOUS release's binary, which emits the OLD text, so a published release carrying the live #5221 bug logs check-ran + warn + check-complete and Gate B reports "OK: parsed GitHub's response". Nothing caught this. pin_warn_literal interpolates $MARKER_PARSE_FAIL, so it follows a rename by construction -- the same defect that left MARKER_LATEST_SEEN's source pin blind. Measured: a sed sweep across the three files a developer must touch leaves every assertion green. Frozen as a single base64 value, not the pair structure used for MARKER_LATEST_SEEN: that pair exists because Gate B SKIPS its positive check for binaries predating the marker and the skip needs a version to compare against. There is no skip branch here, so there is no second value a remediation could quietly leave stale. The failure message refuses to present "regenerate the blob" as the fix, because it is not one: no published binary emits the new wording, so re-stating the freeze alone just makes the suite agree with the blind spot. It names the two real options (keep matching the old text as an alternation, as MARKER_TRIGGERED_RE does; or accept the blindness knowingly) and gives a portable regeneration recipe. Also corrects the neighbouring comment's accounting: seven markers are grepped, two are now frozen, and MARKER_FETCH_FAIL is grepped but named in neither freeze nor #5309. Refs #5309. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
log The value freeze alone notifies but cannot force. Measured: reword the marker, follow the freeze's own recipe exactly and nothing else, and the suite goes fully green while assert_detection_healthy answers RC=0 with "OK: parsed GitHub's response" on a verbatim v0.2.121 #5221 log. That is round 4's shape at one remove -- a single value can always be re-stated, so the remediation never has to make the decision. Drive the real function with the historical WARN line instead, stored base64 so no rename sweep reaches it. After a reword this stays RED until the detector can still read an already-published binary, and the only other way green is deleting the assertion, which is the deliberate reviewable form of accepting the blindness. Scaffolding interpolates the live $MARKER_CHECK_RAN so this stays a pin on MARKER_PARSE_FAIL alone and does not fire for markers #5309 owns. The diagnosis is asserted alongside the exit code: rc=1 is also what "the check never ran" returns. Refs #5309. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
…l counter The `[[:space:]]` right after `(check|ok|pass)` requires the helper's name to END there, so every underscore-suffixed helper scored zero: gate_b_arm_case, version_ge_case, trigger_case, norm_case, timeout_guard_case, pin_marker, pin_warn_literal, check_vs_expected, check_call_count, check_fallback, test_restores_persisted_value. Measured across all 11 *_test.sh files: 329 assertion call sites, 275 seen, 54 missed, 42 of those in auto-update-canary_test.sh alone. Adding a check_/pin_/assert_/expect_/test_ prefix pattern and a _case suffix pattern takes it to 329 of 329 with zero false positives, verified line by line against a hand-enumerated ground truth. On this PR's own diff the count goes 4 -> 16. It fails in the rejecting direction: an under-counting lint turns away a fix: PR that HAS a regression test, and the cheapest response is to reshape a good test until the grep likes it. Nothing exercised the regex, which is why it has now been wrong three times and each miss was found by a human noticing a wrong number. Adds scripts/rule_lint_shell_assertion_counter_test.sh, which EXTRACTS the live pattern from ci.yml rather than copying it (a copy is rewritten by the same edit it guards) and runs it over 26 call-site shapes taken from the repo's own test files, in both directions. Its own first draft truncated the extraction and matched everything, so it carries an explicit vacuity guard. Also corrects the comment's blind-spot list: the printf-style false negative it cited does not occur anywhere in scripts/, and is replaced by the one that does (a bare string added to a table an assertion loops over, e.g. SIGPIPE_SCRIPTS). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
…itHub fetch The previous release's binary retries its startup GitHub fetch ZERO times (startup_update_check_with_fetcher warns and returns on the first Err), and it demonstrably fails: framework logged that WARN twice on 2026-08-11, at 17:08:04Z and 00:35:13Z. Gate B's subject IS a published binary, so this cannot be fixed from the canary side. Gate B had no retry at all -- CANARY_ATTEMPTS was read only by cmd_preflight -- so one blip in a ~40s window decided a release's post-publish verdict, and the resulting red job fired the Matrix alarm saying a node on the previous release may not be able to auto-update to this one. That is the #5221 text, sent because a socket did not open. Three changes: - cmd_selfupdate retries the indeterminate case, mirroring Gate A. It wipes home/cfg/data/logs/tmp/cache between attempts but KEEPS $work/bin: the downloaded previous release is the subject, not state, while $work/home holds the node's persisted GitHub poll cooldown and a retry that re-reads it cannot produce a different answer. - node_could_not_reach_github classifies the run from the node's own WARN. Only that counts as environmental; "started and never logged an outcome" stays a real finding, because a hung updater looks exactly like it. Gate B exits 75 (EX_TEMPFAIL) for the environmental case, reusing the sysexits convention the file already follows with exit 64. Deliberately NOT 43, which is the NODE's port-collision exit code: overloading it would make "the canary exited 43" ambiguous at the one moment someone reads it under pressure. - cross-compile.yml keys the Matrix wording off that code: a quiet message for environmental, the existing alarm otherwise, written as its exact negation so unanticipated states stay loud. The job is still RED either way -- unverified is not verified. Gate A keeps exit 1: it blocks, and a stuck draft always needs a human. Measured end-to-end against a fake node reproducing the framework log: fetch failure -> 2 attempts, exit 75, environmental text; no-outcome -> 2 attempts, exit 1, message noting no fetch failure was reported; real #5221 -> stops after attempt 1, exit 1, the real alarm. Adds behavioural tests for the classifier in both directions, source pins for the retry loop and exit code, and wiring pins so the notify split cannot be silently collapsed or the step's exit code swallowed. Documents the new exit path in docs/RELEASING.md. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
The whole-block scan was self-satisfying: the LOUD step's condition is `!(<quiet_cond>)`, so the quiet condition is a substring of it and the assertion passed on the loud step alone. Mutation-tested -- replacing the quiet step's if: with a bare `result == 'failure'`, so the reassuring message covers every red run including a real #5221, left the suite fully green. Split the notify job into steps, identify each by text from its own message rather than by order, and check the conditions separately. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
…lure never retries Three constraints from review, each closing a way the previous commit could still go wrong. 1. RETRY ONLY THE INDETERMINATE OUTCOME. rc=1 already broke out of the loop immediately, but nothing pinned it, and the failure mode is the worst one available here: each attempt wipes the tree and boots a fresh node, so retrying a genuine detection fault would let any INTERMITTENT fault produce one passing attempt and Gate B would report a broken release healthy. A flaky pass on the post-publish gate is worse than no gate. Pinned behaviourally in the lifecycle test by counting node boots (2 for indeterminate, exactly 1 for a real #5221 parse failure), not by grepping for the break. 2. PERSISTENT INDETERMINACY MUST NOT BE ABSORBED. The quiet message tells the room a red release needs no action, so a single WARN from the node should not earn it: a persistent problem would report "environmental" release after release, nobody would be alarmed, and the gate would verify nothing while looking maintained. gate_b_unverified_class now demands corroboration from a second observer in the same run -- this runner's own fetch of the same endpoint. Node failed and the runner also cannot reach GitHub: real, quiet. Node failed on every attempt while the runner is fine: not a blip, loud. A port collision needs no probe. A sustained CI-wide outage cannot reach the quiet path at all, because Gate B's own tarball download would already have failed loudly. The residual window is documented rather than implied. Both messages now say plainly that the release is UNVERIFIED, and that consecutive occurrences mean the gate is not working. 3. THE NEW NOTIFY STEP IS PINNED. Steps are located by their "id:", not by a phrase from their message -- locating by message text was tried and an ordinary reword broke it on the first edit, which is the same expectation-stored-as-a-copy shape this PR exists to remove. Also: the rule-lint counter could not see this PR's own new test file. Its 26 assertions are bare "expect" calls and the pattern required an "expect_" prefix, so the file whose whole job is to stop the counter under-counting scored zero. Widened to the bare name (nothing in scripts/ drives TCL expect(1)) and added a case for it. Re-measured across all 12 files: 378 assertion sites, 286 seen before, 378 now, zero false positives. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
…r attempt R3 review found six things. The substance is F1 (a bug) and F3 (why we would otherwise ship a seventh). F1, HIGH. The environmental flag was last-writer-wins: reassigned each attempt, consumed once after the loop. Attempt 1 starts the check and logs no outcome (a hung updater, a real finding); attempt 2 loses a port race (environmental); the run exits 75 and the dev room is told it is "not a stranded fleet and needs no fleet action". A false quiet on exactly the class this gate exists to raise, produced by the retry added to reduce false alarms. An unexplained indeterminate on ANY attempt now latches and no later attempt can overwrite it. Its message no longer claims the canary "never got to test" when an attempt did. F3 and Gap 1. Three mutations that break the classifier in the QUIET direction left the whole suite green, and one of them is this PR's own disease again: changing `&&` to `||` in the candidacy test survives the source pin because the CALL TEXT is unchanged -- the defect the round-2 prev_emits_latest_seen fix was written to close, one function over. Nor could anything see the run-level latch, which is a property of a SEQUENCE that no single-attempt test can express. Adds a behavioural driver: stub run_node_until_check, script a sequence of (node exit, fixture log) pairs, assert both the returned code and the attempt count. Seven cases, including both latch orderings and the "retry never touches a real failure" property that Gap 1 says nothing pinned. Gap 3. A real #5221 reaches the quiet message via a reorder nothing pinned: the parse-fail branch precedes the fetch-fail branch, and that ordering is the ONLY thing keeping a log holding both markers out of the environmental class. No fixture in the suite held both. Added one. F2. A port collision was reported as "could not reach GitHub" in the Matrix message; the node never started and nothing fetched anything. The script already worded its own error from the cause; the notify message now names both causes and points at the log line that says which. F6. `CANARY_ATTEMPTS=00` passed the digits-only guard, `seq 1 00` is empty, the loop never ran and the read after it aborted under `set -u` with "rc: unbound variable" -- a shell error where a verdict belongs. All three budget guards now share one sanitiser that folds `10#`; `rc` is initialised. F5, one sentence not a fix: the same WARN also fires for a published fetch-side regression, which would look environmental forever. Gate A narrows it. The quiet message now says a recurrence is not the runner. Item 5: the canary's exit constant and the workflow's literal 75 are one contract in two files; pinned by reading the constant. Item 7: the backstop fixture omitted the completion line, so it yielded rc=2 while its message claimed rc=0. Three lines now, and it fails for the reason it names. Items 6 and 9, and M2: note that ci.yml's removed-tests guard scans crates/core/**/*.rs only, so deleting a shell assertion is flagged nowhere; correct the stale port-increment arithmetic; and correct the claim that MARKER_FETCH_FAIL's reword is merely loud -- it is now the input to the environmental classification, so a reword silently reinstates the false fleet alarm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
The block scanned for the Gate B pins included its own comments, which quote the very constructs the pins require. Those assertions would have passed against the EXPLANATION after the code was deleted -- the same pin-satisfied-by-prose shape JOB_BLOCK already strips for attach-to-release, and that this repo's own rules list twice. Found because a mutation of '|| rc=$?' hit the comment on line 748 before the code on line 756, so nothing actually broke and the pin stayed green. The harness accident is what exposed it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
…haviour 'Which KIND of indeterminate, recorded from the LAST attempt' was true of the last-writer-wins version and became false when the latch landed. A reviewer read the tree, read that line, and correctly concluded from it that F1 was still present -- from the comment, not the code. A stale comment on a fixed bug costs a review round, so the correction says what it used to say and why it changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
…he cause sticky by strength Three findings from the final pass, plus the optional probe hardening. FINDING 1, blocker. `notify_block` was the one extraction of three that still did not drop comment-only lines, so two mutations left all four suites green: dropping `always()` from the notify job's `if:` while leaving a comment that mentions it, and -- the bad one -- widening the quiet step's `if:` to `result == 'failure'` while leaving a one-line comment recording the old condition, which fires "no fleet action is indicated" on every red Gate B run including a real #5221. That second one was saved only by luck: the comment had to be on ONE line, because the `# ` on a wrapped continuation breaks the whitespace-collapsed match. Fixed structurally rather than at the site. All three extractions now go through one `yaml_job_block` helper, so a fourth caller cannot forget the filter -- two single-site fixes in a row is what turned this into a function. (`notify_steps` derives from `notify_block`, so it inherits it.) FINDING 2. The latch held at the top tier but `env_cause` was still last-writer-wins among EXPLAINED causes, and the two orderings of the same pair disagreed: ports-then-github ran the probe and went loud, while github-then-ports never probed at all, exited 75 quiet, and claimed "every attempt hit a port collision on this host" -- false. Reachable at the default two attempts. The fold is now sticky by STRENGTH, not recency: unexplained > github > ports. Both orderings pinned. FINDING 3. `gate_b_case` compared only exit code and attempt count, so deleting the entire `ports)` arm of the message case left everything green. Exit 75 is reached from two branches with different operator-facing text, and the quiet Matrix message deliberately names the job log line as the only disambiguator -- so that line was the one thing untested. It now takes an expected-substring like `check()` does, which is also what catches Finding 2's false message, since the exit code alone is 75 in the correct ports-only case too. Optional hardening, taken because it is one case statement. The probe went through `resolve_expected_latest`, which collapses every non-answer to `return 1`: a 403, a captive portal, a changed redirect shape, curl missing. Read as corroboration, every one of those bought the quiet path. It now asks curl directly and only connect-class exits (6/7/28/35) count as "cannot reach"; everything else, including an HTTP error and a missing curl, is loud. The endpoint is a named constant so the two callers cannot drift onto different URLs. Eight cases stub curl's exit code rather than the function, so what is tested is the classification and not a restatement of it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
…sserting counts in prose B1, BLOCKER. `docs/RELEASING.md`'s "If Gate B fails" told the operator that an every-attempt fetch failure gives exit 75 and the quiet warning, and added "so if you are reading the #5221 text this is not what happened". The corroboration probe made that false: the github cause now also needs the RUNNER to fail the same fetch, and a hosted runner is normally online by probe time, so the common case gives exit 1 and the loud alarm. Verified by executing all five outcomes: fetch-fail x2, runner OFFLINE exit 75 QUIET fetch-fail x2, runner ONLINE exit 1 LOUD port collision x2 exit 75 QUIET no outcome logged x2 exit 1 LOUD real #5221 parse failure exit 1 LOUD An operator seeing the alarm over a log full of fetch-fail WARNs was told this was not what happened, fell through to "a real detection or install failure", and would cut a fix release for what the canary's own message calls a probable poll-budget cooldown. The section also promised three cases where the code produces five, and never mentioned the ports route to 75 that the quiet Matrix message names. Rewritten as a table keyed on the phrases the code actually emits. B2. Two comments fifteen lines apart contradicted each other about how the notify steps are located; the message-text form was replaced two commits earlier. A reviewer reading top-down would recognise the anti-pattern and report a defect already fixed. Removed, and the surviving statement lives at the call site it describes. B3. The claim that `grep -rnE "printf .*['\"](ok|PASS)"` over scripts/ returns nothing was the sole stated reason for DELETING a real entry from the contributor-facing blind-spot list. It returns two: test-install-sh.sh and test-uninstall-sh.sh, both `pass() { printf 'PASS %s\n' "$1"; }`. They escape the counter only because the diff is globbed to `**/*_test.sh` and those are `test-*.sh` -- a naming accident. Entry restored with the accurate reason. One I introduced fixing Finding 2: the message said the node could not reach GitHub "on all $CANARY_ATTEMPTS attempts", but sticky-by-strength needs only ONE github attempt, so ports-then-github printed it falsely. It now counts them and states the real number, and three driver cases assert the count rather than a substring that skips over it. Also corrected the latch comment, which described a loop that no longer only records. B4-B8: "the only thing keeping a real #5221 out" downgraded to "the first of two" (the probe is the second); the lifecycle cross-check no longer claims "REAL boots" when both sides are fakes differing only in how much harness runs; six `freenet.rs` line numbers, all low by 12, replaced with the phrases they name -- this file's own rules already say a line number in a note about stale pins rots faster than its subject. THE INSIGHT, carried into the code rather than only fixed. Seven of the eight stale comments were load-bearing JUSTIFICATIONS -- the sentences that tell the next reader they may stop checking -- and they rot exactly when the thing they justify is strengthened. So every count and grep result in this PR that could rot is now computed where it can go red: `rule_lint_shell_assertion_counter_test.sh` recomputes that no `*_test.sh` is invisible to the counter, and that the printf blind spot is still unreached, instead of restating measured numbers. The prose numbers that moved three times inside this PR are gone. Written up as a new section in `.claude/rules/bug-prevention-patterns.md`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
…files The invisible-*_test.sh check used `git ls-files`, so an untracked new file was invisible to it -- and a contributor adding a test file has not necessarily staged it yet, which makes that file exactly the one whose convention the counter may not know. Found by mutation: a probe file written with an unrecognised convention did not trip the check that exists to catch precisely that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
…nothing reached
The count fix landed on the loud github branch and not on its quiet
sibling twelve lines above. Both are reached with env_cause=github,
which under sticky-by-strength needs only ONE github attempt, so
ports-then-github with the runner DOWN printed "in 2 attempts" when one
attempt was a port collision. The comment above the loud branch was
stale in the same way ("on every attempt").
Found because a reviewer's mutation reported PATTERN NOT FOUND with the
suite green. An unapplied mutation is not a pass; the string was missing
because the unfixed copy was in a different branch.
Enumerated the rest by grep rather than by recall. Four sites
interpolate CANARY_ATTEMPTS into a message; the two in Gate A's
post-loop failure and Gate B's classifier-bug branch are legitimately
unanimous, because both loops only continue while rc=2, so reaching
them means every attempt was indeterminate. The ports message is
likewise accurate: env_cause=ports with no unexplained attempt implies
every attempt was a port collision.
Two new driver cases reach the quiet github branch, which the three
count-asserting cases never did -- they all have the runner UP and so
land on the loud sibling. That is why the sibling had no coverage.
Also adds the enumeration rule to bug-prevention-patterns.md: a defect
found at one site is not fixed until every site of that shape has been
grepped for, and three of tonight's four instances of that miss were
caught only by an unapplied mutation.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012dyjTKM35KGZXjE7jmDTsX
95c8350 to
46dbaf2
Compare
On the scope-creep warningsThe rule-review check flags four of these, and a human-equivalent big-picture reviewer raised the same objection earlier in this PR's review. It deserves an answer on the record rather than a silent override label, because the objection is basically correct on its face: this PR contains four logical changes and its title names one. The counter-argument, and it is a causal one rather than a convenience one. The Gate B environmental classification (exit 75, the corroboration probe, the retry loop, the notify split) is not merely bundled with the marker freeze. It is caused by it. Gate B's positive equality check has never executed in production — it was skipped for 0.2.125 because the previous release predated the marker. The constant frozen in this PR is precisely what makes it arm for the first time on 0.2.126. And the marker it then requires is emitted only when the node's own GitHub fetch succeeds — a fetch with no retry, which was observed failing twice in one day on a single real machine during this PR's review. So shipping the marker half alone would ship the change that arms the check without the change that stops it false-alarming, and the first symptom would be a post-publish 🚨 telling the team the fleet may be stranded, on a healthy release. Splitting here does not reduce risk; it sequences the two halves in the order that produces the incident. The rule-lint half is load-bearing in a narrower and more boring way, verified rather than asserted: on this branch's diff the pre-existing regex scores 0 added shell assertions, so without the widening, Rule Lint rejects this PR for having no regression test. What is genuinely separable and is being kept separate: the Gate A reordering — moving What this PR should have done differently: the title and body named one change for most of the PR's life. Both now describe all four. That was the reviewer's actual condition for shipping it whole, and it was the fair criticism. For anyone auditing later: this went through four review rounds (three full, one focused), which found seven instances of the same defect class — a gate that cannot see its subject — every one of them inside the fix for the previous one. The last of those was found because a mutation returned [AI-assisted - Claude] |
1 participant
Problem
Two bugs in the release auto-update canary, one of which would have blocked the 0.2.126 release.
MARKER_LATEST_SEEN_SINCEnames the first release whose binary emitsStartup update check: GitHub reports latest release. Gate B skips its positive equality check for any previous release below that value.Bug 1 — a self-detonating pin. The self-test added in #5290 asserted the constant was not below the crate version, and stated its own premise in its comment: "the marker is new in this tree, so no already-published release emits it." That premise was true only while 0.2.125 was unpublished. It expired at 00:11Z on 2026-08-13 when 0.2.125 shipped. The constant correctly stays at
0.2.125forever, so the pin goes red on the next bump and blocks a healthy release. Reproduced: set the crate version to 0.2.126 againstmainand the pin fails.Bug 2 — a silent one, found during review. Reword the marker text in every place a developer must touch it (
auto_update.rs,freenet.rs,auto-update-canary.sh, and the log fixtures in both test files) and leave the constant alone: the entire suite passes. Nothing prompts anyone. A stale constant is the default outcome of a reword, not an escape from a warning. The consequence lands post-publish, where Gate B arms, greps for text the previous release never emitted, and trips the release Matrix alarm with a message blaming the node.Solution
Delete the version relation entirely, and freeze the pair.
Three attempts at a relation between the constant and the crate version were considered, and the conclusion is that no fixed relation between two quantities on different clocks survives both publication and a reword:
constant >= crate(shipped in #5290)constant <= crate(first attempt here)constant <= crate + 1(proposed in review)Correction, found by a later reviewer: an earlier revision of this table claimed
<= crate + 1"also fires on a reword", and that is arithmetically false. Under a reword the correct constant is crate+1, andcrate+1 <= crate+1passes. The claim entered this description because it was relayed from a reviewer who withdrew their own suggestion on that reasoning; nobody checked the arithmetic. A loose upper bound of that shape appears to be phase-independent — it is violated only when the constant is more than one release ahead, which is never legitimate — so it is being re-evaluated as plausibility bounding alongside the freeze rather than as a replacement for it.The two equality-style relations are what cannot work: the constant tracks release history and the crate version tracks this tree, so no relation asserting they stay in step survives both publication and a reword. What remains is:
0.2.125. This catches strictly more than the relation ever did, including the empty string and forward creep at release time.version_at_least, which previously accepted garbage as a pass:version_at_least "not-a-version" "0.2.125"returned true.Why the marker freeze is base64-encoded
Written first as a plain literal, and it did not work. The mutation it exists to catch is a rename sweep — and the sweep rewrote the frozen expectation too, because the expectation was byte-identical to the value it guarded. The pin followed the rename and the suite stayed green.
Encoded, a text sweep cannot reach it, and a deliberate reword has to regenerate it, which is exactly the moment of attention the pin exists to create. The failure message prints the decoded old text, the new text, and the re-encode command; an empty decode fails loudly rather than making the comparison vacuous.
The version half stays plaintext deliberately, and the asymmetry is documented: a repo-wide
sedon a version string is not how bumps happen here (they touchCargo.tomland the lockfile), so the realistic wrong edit is a human raising it in one place, which a plain compare catches.Also
gate_b_arm_case "0.2.124" nocloses a verified vacuity hole: hardcoding the threshold inside the decision previously left the whole suite green, because the existing cases straddled the gap.prev_versionis the newest currently-published release, so a retraction legitimately puts it back below the constant.Testing
Suite is 68 assertions, all green. Every pin was mutation-tested by applying the exact regression it names to a committed tree and confirming it goes red.
main's pin FAILS, reproduced. This HEAD green. Defused.version.workspace = truein Cargo.tomlnot-a-version >= 0.2.125,0.2.125 >= '','' >= '',0.2.125-rc.1 >= 0.2.125all wrongly reported yesprev_emits_latest_seenmade to skip on malformed inputThe refuse-vs-arm asymmetry is deliberate and both directions are pinned:
version_at_leastrefuses malformed input because "is a >= b" has no answer for garbage, whileprev_emits_latest_seenarms Gate B on it because "should Gate B assert anything" does, and skipping is the silent direction.shellcheck -xclean; the exact CI shellcheck invocation rc=0;cargo fmt --all --checkclean. No Rust files changed.The
ci(rule-lint)commitsScope beyond the fix, called out as a deliberate exception to one-logical-change: without it the Rule Lint check rejects this PR for having no regression test, and
test-exemptwould put a false statement in the record on a PR that demonstrably has tests."okis tightened to["']ok[[:space:]]+-andpassis added. Measured on a fixture:echo "okay, the suite is starting"and bareecho "ok"are now rejected; single-quotedecho 'ok - ...'andpass "..."are now accepted.An honest limit, now stated in the CI comment itself: this check has never recognised assertions. It recognises the success-reporting line by prefix.
check "x" "a" "a"is a real call to a real helper that compares a value with itself and asserts nothing, and the old pattern already matched it. A shared assertion helper would make it tidier without making it non-decorative. Whether something is a real regression test belongs to review and mutation testing.Two false negatives are left in deliberately and documented rather than chased with a fifth pattern:
printf-style reporting, and an assertion whose only added line is its FAIL branch (counting FAIL would let a PR score by adding an error message alone).Corrections to earlier revisions of this description
Kept visible rather than silently edited:
ci.yml, claimedauto-update-canary_test.sh"has no helper at all". False —check()is at line 55 with 12 call sites. It is specialised to log-fixture cases; the file's other assertions have nothing to fixture and are written as bare echoes.constant <= crate versionas the fix. That is the approach this HEAD deletes, for the reason in the table above.Fixes
Refs #5290. Prevents a self-inflicted block on the 0.2.126 release, and closes the silent reword path.
[AI-assisted - Claude]