Repository navigation
docs(measurement): record why the host lock, not an announce protocol - #2175
Merged
Merged
Conversation
The lock has been mandatory since #1806 (`73c76458c`), but the *argument* for it lived in issue threads and agent messages, so it gets re-litigated roughly once a week -- usually by someone who has just had a good day with announce-before/announce-after, and always from scratch. Writing it down next to the rule makes the next round short. Three arguments, each from a measured failure rather than a preference: * Announcement is pairwise and the host is not. On 2026-08-25 one agent correctly yielded the box to a second and a third then negotiated with the first for a host already given away. Nobody defected; the protocol simply has nowhere to put a third party. * An announcement describes an edge, a lock covers the interval. Both false "host free" claims that day were assertions that outlived their measurement -- one from a reading 74 minutes stale, sent 14 minutes after the hung process it missed had started. One came from the agent who proposed the discipline and one from the agent policing it, which is the tell that it is not carelessness. Same defect as `ps`-based liveness, one layer up, and the reason the *outer harness* holds the lock rather than each bench child. * A per-run efficiency guard is self-protective, not preventive. It is an excellent instrument -- it took an A/A null from 52% to 0.04-0.56% here -- and it tells you when somebody contaminated *your* run. It says nothing about you contaminating *theirs*, so it does not compose across agents, and it is blind to SMT-sibling contention and steady external load, both of which hold efficiency near 1.0 while moving the number. So the framing is: the lock decides whether you may **start**, the guard decides whether to **believe** the reps you got, and the guards are supplementary -- a clean efficiency trace with no lock is not a defensible measurement. Also records why `/proc/loadavg` is the wrong instrument for admission control in either direction: a bounded 4-of-32-CPU protocol trips a `-le 3` gate while being a good citizen, and a single-threaded 100% hog passes it. Documentation only. No behaviour change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Aug 26, 2026
justinchuby
added a commit
that referenced
this pull request
Aug 26, 2026
…2179) `merge_when_green.sh` is the gate every pull request in this lane goes through: it waits for the required contexts named by the repository ruleset, and merges only when all of them are `SUCCESS`. That is one terminal state, and it is not enough. ## The failure A docs-only pull request never reaches it. The `Detect change scope` job skips the heavy lanes when the diff touches no code, so both required contexts conclude **SKIPPED**, `mergeStateStatus` is `UNSTABLE`, and the watcher polls until it times out at exit 3. The pull request is mergeable, correct, and reviewable, and the tool will never merge it. PR #2175 is sitting on exactly this right now. This is the failure mode the script exists to prevent, pointed the other way. A guard with no recovery path does not stop its operator; it routes them around it — and the way around this one is `gh pr merge --admin`, which is the single thing the standing policy forbids. A tool that can only be obeyed by being bypassed has already lost. ## The change SKIPPED becomes a third outcome, rather than being folded into either of the other two. | condition | result | |---|---| | all required SUCCESS | merge (unchanged) | | any required FAILURE | exit 2 (unchanged) | | some required SKIPPED, rest SUCCESS | **exit 7**, no merge, names the contexts | | same, with `--allow-skipped`, state CLEAN/UNSTABLE/HAS_HOOKS | merge | | same, with `--allow-skipped`, state BLOCKED/DIRTY/BEHIND/DRAFT | exit 6, still refuses | | same, with `--allow-skipped`, state UNKNOWN or absent | keeps polling, then exit 3 | | a skip alongside anything absent/queued/failed | unchanged refusal | | a skip alongside a failure **under the same name** | exit 2 | Exit 7's message names the skipped contexts, points at the flag, and says in the same breath **not** to reach for `--admin`. `--allow-skipped` accepts skips and only skips. The skipped set must be *exactly* the not-green set, so one skipped lane cannot excuse an absent, queued or failed sibling — and, per review, that is enforced per **run** and not per folded row, so a required name that skipped *once* and failed *once* is a failure no matter which order GitHub lists them in. And even with the flag, GitHub's own `mergeStateStatus` must be one of the states that accept a merge. That is an **allowlist**: refusing only `BLOCKED` would mean `UNKNOWN` — GitHub saying it has not computed mergeability yet — counted as consent, as would the field vanishing in an API change. Not-yet-known is a third outcome there too, and it waits. The caller may decide that a skip is acceptable for their diff; they may not overrule the repository, and they may not accept on its behalf before it has spoken. ## Why a flag, and not just treating SKIPPED as green Because whether a skip is acceptable is a property of the diff, not of the check. `Detect change scope` skipping the Rust lanes is correct information about a docs change and a **serious** signal about a code one — a misconfigured path filter that silently stops testing the thing you changed looks identical from here. The tool cannot tell those apart; the person who wrote the diff can. `--allow-skipped` is that person asserting it, at the call site, where the evidence is. Folding SKIPPED into SUCCESS would make every future path-filter regression merge silently. That is a worse trade than one flag. ## Testing 31 new cells (41 → **72**, all passing). Sixteen came with the original push, of which four are negative controls: absent, queued, failed and `BLOCKED` inputs each still refuse *with the flag set*. Two more pin that the flag does not alter the all-green path — it must not turn a green merge into a reported skip. Ten mutants, **eight caught**. **The two survivors are equivalent by construction** and are declared so rather than quietly dropped: `green + nskipped == want` and `notgreen == skipped` are the same predicate while the upstream `rows != want` guard admits exactly one verdict row per required name, so no input can separate them. Both clauses are kept — the guard is what makes them equivalent and is not a law of the file — and the comment says all of this out loud instead of letting the pair be mistaken for coverage. `bash -n` and `shellcheck` clean on both files. ## Follow-up Once this lands, #2175 (`measurement-discipline`: why a lock beats an announce protocol) merges with `--allow-skipped` instead of timing out. ## Review round An adversarial review found **two MUSTs, both reproduced by execution**, and both are fixed in `2857220ac`: 1. **A skipped run masked a failed one under the same name.** The pessimistic fold reported the array-order-first bad entry, which was free while the only question was green-or-not and stopped being free once SKIPPED became mergeable. Worse: with *no* flag that input exited **7** and advised `--allow-skipped`, so the gate was routing its operator into merging over a failure. Every pre-existing negative control gave its bad conclusion its own required name, so none of them ever reached the fold — they asserted the verdict, not the mechanism. 2. **The merge-state guard was a denylist that failed open** on `UNKNOWN`, `DIRTY`, `BEHIND` and a missing field. Now an allowlist with a distinct not-yet-known outcome. Both are pinned by new mutants (10 total, 8 caught) and by cells that fail if either fix is reverted. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Documentation only. Records, next to the rule it justifies, why saturating benchmarks hold the host lock instead of coordinating by announcement.
The lock has been mandatory since #1806 (
73c76458c) and.github/skills/measurement-discipline/SKILL.mdalready carries the rule and the mechanics. What it does not carry is the argument, which lives in issue threads and agent messages — so it gets re-litigated roughly once a week, always from scratch, usually by someone who has just had a good day with announce-before/announce-after. That is a reasonable position to arrive at from one day of evidence, which is exactly why the counter-evidence should be in the repo rather than in a scrollback.The three arguments, each from a measured failure
Announcement is pairwise; the host is not. On 2026-08-25 one agent correctly yielded the box to a second, and a third then negotiated with the first for a host that had already been given away. Nobody defected and everybody was polite. Pairwise etiquette has nowhere to put a third party; a lock has one holder and every non-holder reads the same answer.
An announcement describes an edge; a lock covers the interval. Both false "host free" claims that day were state assertions that outlived their measurement — one sent from a reading that was 74 minutes stale, 14 minutes after the hung process it missed had started. Three test processes ran 75/61/51 minutes against a ~7-minute baseline and were found only because somebody went looking. One false claim came from the agent who proposed the announce discipline and one from the agent policing it, which is the tell that this is a property of the protocol and not of carelessness. It is the same defect as
ps-based liveness one layer up, and the reason the outer harness holds the lock rather than each bench child.A per-run efficiency guard is self-protective, not preventive. Per-run rusage
(utime+stime)/wallis an excellent instrument — it took an A/A null from 52% to 0.04–0.56% on this box — and it belongs in every harness. But it tells you when somebody contaminated your run and says nothing about you contaminating theirs, so it does not compose across agents: if everyone adopts it and nobody locks, every run is correctly labelled and half are discarded. It is also blind to SMT-sibling contention and to steady external load, both of which hold efficiency near 1.0 while moving the number.The resulting framing, which is the part worth remembering: the lock decides whether you may start; the guard decides whether to believe the reps you got. Neither substitutes for the other, and the guards are supplementary — a clean efficiency trace with no lock is not a defensible measurement.
Also written down:
/proc/loadavgis the wrong instrument for admission control in both directions. A deliberately-bounded 4-of-32-CPU protocol shows runnable ≈4–5 and trips a-le 3gate while being a good citizen; a single-threaded 100% CPU hog shows ≈1 and passes.Validation
Documentation only, no behaviour change.
scripts/ort_ab/test_gate_conformance.py97/97 green; nothing inscripts/or.github/workflows/reads this file, so there is no conformance cell to update.No
--admin, no ruleset bypass; merging on required checks only viascripts/merge_when_green.sh.