diff --git a/.github/samples-gate.allow b/.github/samples-gate.allow index b86dfaa3ee..a3d9e56b12 100644 --- a/.github/samples-gate.allow +++ b/.github/samples-gate.allow @@ -40,4 +40,18 @@ PensionFund/BalanceSheet compile # type is registered, the installer materialises a typed `Currency` and re-serialises it, and # default-suppression drops the property whose value equals the type's default. The stored # node then differs from the authored file and the unchanged-skip misses. -PensionFund idempotence \ No newline at end of file +PensionFund idempotence intermittent + +# 🚨 The `intermittent` token above is a REAL WEAKENING, and its failure mode is social rather than +# technical: the next person with a deterministic red they cannot fix will find this token and reach +# for it. It is legitimate for exactly one thing — a check that FLAPS, where a single green run does +# not prove the debt is paid. It is NOT for a failure that is merely inconvenient, hard, or someone +# else's. Marking a deterministic failure intermittent silences the ratchet permanently and is the +# band-aid the ratchet exists to prevent. +# +# It buys exemption from STALE-DETECTION and nothing else: the entry still tolerates only the +# scope/check it names, and a failure it does not name is still a new failure. +# +# Before adding one, prove the flap — two runs of the same commit disagreeing, or the git history of +# this file showing an entry dropped as stale and restored. That is what justified this one: +# 768f9ba0b dropped it, f32047e0e restored it one commit later. \ No newline at end of file diff --git a/test/MeshWeaver.PluginTester.Test/GateAllowlistTest.cs b/test/MeshWeaver.PluginTester.Test/GateAllowlistTest.cs index b572d97697..176003c3b4 100644 --- a/test/MeshWeaver.PluginTester.Test/GateAllowlistTest.cs +++ b/test/MeshWeaver.PluginTester.Test/GateAllowlistTest.cs @@ -160,6 +160,59 @@ public void Evaluate_TypeChecks_MatchOnTheTypePath() // ── the stale rule ─────────────────────────────────────────────────────────────────────── + [Fact] + public void Evaluate_IntermittentEntryWhoseCheckPasses_IsNotStale() + { + // The ratchet's rule — a listed check that starts passing must be removed — assumes the + // check is DETERMINISTIC, so one green run proves the debt is paid. For a flapping check + // that inference is wrong. PensionFund idempotence was dropped as stale and restored one + // commit later for exactly this reason, after which the file carried the intent in a + // comment the parser strips, and the ratchet kept failing runs for obeying the file. + var healthy = new PackageResult("Claims"); + var verdict = GateVerdict.Evaluate( + Report(healthy), GateAllowlist.Parse(["Claims idempotence intermittent"])); + + Assert.Empty(verdict.Stale); + Assert.Empty(verdict.NewFailures); + Assert.True(verdict.Success); + } + + [Fact] + public void Evaluate_IntermittentEntry_StillSuppressesItsOwnFailure() + { + // Marking an entry intermittent gives up stale-detection for that line and NOTHING else: + // it still tolerates the failure it names, exactly as before. + var verdict = GateVerdict.Evaluate( + Report(IdempotenceBroken("Claims")), + GateAllowlist.Parse(["Claims idempotence intermittent"])); + + Assert.Empty(verdict.NewFailures); + Assert.Single(verdict.KnownDebt); + Assert.True(verdict.Success); + } + + [Fact] + public void ToString_RendersTheEntryExactlyAsTheAllowFileHasIt() + { + // The gate prints entries in its diagnostics, and an operator reading that output is looking + // for the LINE TO EDIT. A rendering that drops the marker does not match the file and cannot + // be pasted back into it. + var plain = GateAllowlist.Parse(["Claims idempotence"]).Entries.Single(); + var flappy = GateAllowlist.Parse(["Claims idempotence intermittent"]).Entries.Single(); + + Assert.Equal("Claims idempotence", plain.ToString()); + Assert.Equal("Claims idempotence intermittent", flappy.ToString()); + } + + [Fact] + public void Parse_RejectsAnUnknownThirdToken() + { + // A typo in the marker must not silently degrade to a plain entry — that would restore the + // stale-failure the marker exists to prevent, with nothing pointing at the cause. + Assert.Throws( + () => GateAllowlist.Parse(["Claims idempotence intermitent"])); + } + [Fact] public void Evaluate_EntryWhoseCheckPasses_IsStale_AndFailsTheRun() { diff --git a/tools/MeshWeaver.PluginTester/GateAllowlist.cs b/tools/MeshWeaver.PluginTester/GateAllowlist.cs index d0a61ba2e0..06668d3878 100644 --- a/tools/MeshWeaver.PluginTester/GateAllowlist.cs +++ b/tools/MeshWeaver.PluginTester/GateAllowlist.cs @@ -20,6 +20,25 @@ public sealed record GateAllowlist(IReadOnlyList Entries) /// The empty list — every failure is a new failure. public static readonly GateAllowlist Empty = new([]); + /// + /// Optional third token marking an entry whose check FLAPS. Such an entry is exempt from + /// stale-detection: passing once does not prove the debt is paid, so the ratchet must not + /// demand its removal. It still suppresses only the scope/check it names. + /// + /// 🚨 This is a REAL WEAKENING and its failure mode is social, not technical: the next + /// person with a deterministic red they cannot fix will find this token and reach for it. It is + /// legitimate for a check that genuinely flaps and for nothing else — marking a deterministic + /// failure intermittent silences the ratchet permanently, which is the band-aid the ratchet + /// exists to prevent. Prove the flap first (two runs of one commit disagreeing, or an entry + /// dropped as stale and restored in this file's history). + /// + /// The sibling rule is the same idea from the other direction: an entry listed but ABSENT + /// from a narrowed run is Unverifiable — warned, never failed. That one refuses to infer + /// from a check that did not run; this one refuses to infer from a check that ran once and + /// happened to pass. Both are one observation is not proof. + /// + public const string IntermittentMarker = "intermittent"; + /// The valid check names, package-level and type-level. public static readonly IReadOnlySet Checks = new HashSet(StringComparer.OrdinalIgnoreCase) @@ -38,11 +57,15 @@ public static GateAllowlist Parse(IEnumerable lines) if (line.Length == 0) continue; var parts = line.Split((char[]?)null, StringSplitOptions.RemoveEmptyEntries); - if (parts.Length != 2 || !Checks.Contains(parts[1])) + var intermittent = parts.Length == 3 + && string.Equals(parts[2], IntermittentMarker, StringComparison.OrdinalIgnoreCase); + if ((parts.Length != 2 && !intermittent) || !Checks.Contains(parts[1])) throw new FormatException( - $"allow file line {number}: expected ' ' with check one of " + + $"allow file line {number}: expected ' ' (optionally followed by " + + $"'{IntermittentMarker}') with check one of " + $"[{string.Join(", ", Checks)}], got '{line}'"); - entries.Add(new AllowEntry(parts[0], parts[1].ToLowerInvariant())); + entries.Add(new AllowEntry(parts[0], parts[1].ToLowerInvariant()) + { Intermittent = intermittent }); } return new GateAllowlist(entries); } @@ -101,6 +124,14 @@ private static string StripComment(string line) => /// One known-debt entry: a scope (package id or NodeType path) and a check name. public sealed record AllowEntry(string Scope, string Check) { + /// + /// Whether this entry's check FLAPS — see . + /// Deliberately an init-only PROPERTY rather than a primary-constructor parameter: adding a + /// defaulted parameter to a public record's primary ctor is a binary-breaking change, which the + /// "Public record signatures" gate refuses. It caught exactly that on the first attempt here. + /// + public bool Intermittent { get; init; } + /// Case-insensitive match, exact scope — no globs: an entry must name exactly the /// debt it tolerates, or the ratchet stops ratcheting. public bool Matches(string scope, string check) => @@ -108,7 +139,16 @@ public bool Matches(string scope, string check) => && string.Equals(Check, check, StringComparison.OrdinalIgnoreCase); /// - public override string ToString() => $"{Scope} {Check}"; + /// + /// The entry as it appears in the allow file — INCLUDING the intermittent 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 drops the marker does + /// not match the file and cannot be copy-pasted back into it. + /// + public override string ToString() => + Intermittent + ? $"{Scope} {Check} {GateAllowlist.IntermittentMarker}" + : $"{Scope} {Check}"; } /// One observed failure: where, which check, and the detail text. @@ -141,7 +181,22 @@ public static GateVerdict Evaluate(GateReport report, GateAllowlist allow) { if (failures.Any(f => entry.Matches(f.Scope, f.Check))) continue; - (Passed(report, entry) ? stale : unverifiable).Add(entry); + // 🚨 An INTERMITTENT entry that passed this run is NOT stale. The ratchet's rule — a + // listed check that starts passing fails the run until its line goes — assumes a check + // is deterministic, so one green run proves the debt is paid. For a flapping check that + // inference is wrong, and acting on it churns: PensionFund idempotence was dropped as + // stale (768f9ba0b) and RESTORED one commit later (f32047e0e, "the check is + // intermittent"), after which the allow file carried the intent in a COMMENT the parser + // strips — so the ratchet kept failing runs for obeying the file. Marking the entry + // instead puts that intent where the code can read it. + // + // This never hides a NEW failure: an intermittent entry still only suppresses the + // scope/check it names, and 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 whole point. + var passed = Passed(report, entry); + if (entry.Intermittent && passed) + continue; + (passed ? stale : unverifiable).Add(entry); } return new GateVerdict(newFailures, knownDebt, stale, unverifiable); }