Repository navigation
The ratchet fails a run for obeying the allow file — teach it "intermittent" - #2324
Conversation
…ittent" .github/samples-gate.allow line 32 says, in a comment: # PensionFund idempotence — INTERMITTENT known debt. Do not remove on a single green run. The parser strips comments, so the ratchet cannot read that. Its rule — a listed check that starts passing fails the run until its line is removed — assumes a check is DETERMINISTIC, so one green run proves the debt is paid. For a flapping check that inference is wrong, and the history shows the loop it causes: the entry was dropped as stale (768f9ba) and RESTORED one commit later (f32047e, "the check is intermittent"). Since then every run where PensionFund happens to pass fails the gate with "stale allow entr(ies)" and 0 new failures — which is what is currently reddening #2285, a PR that touches none of this. Removing the entry is the wrong fix twice over: the file forbids it, and it was already tried and reverted. So the intent moves out of the comment and into a token the parser reads: PensionFund idempotence intermittent An intermittent entry is exempt from stale-detection and NOTHING else. It still tolerates only the scope/check it names; a failure it does not name is still a new failure. What it gives up is stale-detection for that one line, which is the entire point. Tests, in both directions — a marker that cannot fail would be worse than the bug: • an intermittent entry whose check PASSES is not stale (gate stays green) • an intermittent entry still suppresses its own failure (unchanged) • a TYPO in the marker throws rather than silently degrading to a plain entry, which would restore the stale-failure with nothing pointing at the cause 22/22 in MeshWeaver.PluginTester.Test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the PluginTester gate allowlist “ratchet” to support an intermittent marker for known-debt entries whose checks can flap, so that a single passing run does not incorrectly classify the entry as stale and fail the gate.
Changes:
- Extend
.allowparsing to accept an optional third token (intermittent) and store it onAllowEntry. - Exempt intermittent entries from stale-detection when the corresponding check passes in the current run.
- Add unit tests for intermittent behavior and update
.github/samples-gate.allowto mark the PensionFund idempotence entry as intermittent.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/MeshWeaver.PluginTester/GateAllowlist.cs | Adds the intermittent marker support in parsing and verdict evaluation. |
| test/MeshWeaver.PluginTester.Test/GateAllowlistTest.cs | Adds tests covering intermittent parsing and stale-detection behavior. |
| .github/samples-gate.allow | Marks PensionFund idempotence as intermittent so the gate no longer fails on passing runs. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…is read Two review corrections. 1. The signature gate was right and I was wrong. Adding `bool Intermittent = false` to AllowEntry's PRIMARY CONSTRUCTOR is a binary-breaking change to a public record — precisely the class "Public record signatures (binary compatibility)" exists to refuse, and precisely the class I already knew about. It caught it on the first push. The flag is now an init-only PROPERTY: same data, same parsing, ctor signature untouched. 2. The caveat moves into the FILE, next to the token. A reviewer's point, and the load-bearing half of the change: an `intermittent` marker's failure mode is SOCIAL, not technical — the next person with a deterministic red they cannot fix will find the token and reach for it. A rationale that lives only in a PR nobody reopens does not reach them; one written beside the entry does. It says what the token is for (a check that genuinely FLAPS), what it is not for (a failure that is merely inconvenient), what it buys (stale-detection exemption and nothing else), and how to prove the flap before adding one. The XML doc also now records the symmetry a reviewer named: the ratchet already refuses to infer from a check that did NOT run (Unverifiable — warned, never failed). This refuses to infer from a check that ran once and happened to pass. Both are "one observation is not proof". 22/22, -c Release -warnaserror clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three Copilot findings, all correct.
1. TWO <summary> blocks on one symbol — and it was worse than it read. My earlier
edit anchored on the `Checks` declaration and inserted BEFORE it, which left
`Checks`'s summary orphaned above the new marker doc: IntermittentMarker
inherited a second summary, and `Checks` lost its documentation entirely. The
summary is back on `Checks`; the marker keeps its own.
2. ToString() dropped the marker. The gate prints entries in its diagnostics
("unverifiable allow entry …: {entry}"), and an operator reading that output is
looking for the LINE TO EDIT — a rendering that does not match the file cannot
be pasted back into it. Now renders "<scope> <check> intermittent", pinned by a
round-trip test in both shapes.
3. Passed(report, entry) was evaluated twice per entry, and it walks every package
and type. Computed once.
23/23, -c Release -warnaserror clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Strong diagnosis of the ratchet bug — the comment-stripping parser making the file unreadable to its own enforcer is exactly right, and the drop/restore commit pair you cite was mine. But one input to the decision is wrong, and I wrote it. The
That mechanism is not real. The real cause was found while fixing #2266/#2271 in #2318: Why this matters for your PR: if #2318 lands, Two things worth doing either way, and they are independent of the order the PRs land in:
Evidence for the BOM cause is in #2318 ( |
.github/samples-gate.allowline 32 says, in a comment:The parser strips comments, so the ratchet cannot read that.
The bug
The ratchet's rule — a listed check that starts passing fails the run until its line is removed — assumes the check is deterministic, so one green run proves the debt is paid. For a flapping check that inference is wrong, and the git history shows the loop it produces:
768f9ba0bf32047e0eSince then, every run where PensionFund happens to pass fails the gate with
stale allow entr(ies)and 0 new failures. That is what is currently reddening #2285, a PR that touches nothing related.Why not just remove the entry
Both obvious fixes are wrong:
So the intent moves out of a comment the parser discards and into a token it reads:
What the marker does, and what it deliberately does not
An intermittent entry is exempt from stale-detection and nothing else. It still tolerates only the scope/check it names; a failure it does not name is still a new failure. What it gives up is stale-detection for that one line — which is the entire point, not a side effect.
This is a real weakening of one specific guarantee, so it is deliberately narrow: it cannot be applied by accident (a typo throws), it cannot broaden what an entry suppresses, and it leaves the
Unverifiablebucket's meaning intact.Verified in both directions
A marker that cannot fail would be worse than the bug it fixes:
intermitent) throwsFormatExceptionrather than silently degrading to a plain entry, which would restore the stale-failure with nothing pointing at the cause22/22inMeshWeaver.PluginTester.Test,-c Release -warnaserrorclean.Scope
Found while diagnosing #2285's red. Its failure was
Doc content compiles and runs— the same job name as the Northwind compile error fixed in #2308, but a different check inside it, which is why the fix that landed did not clear it.