From 5a0f5ed7a7fbaf588e37d1a2b156136f4c113ad0 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Tue, 3 Nov 2026 10:21:27 +0100 Subject: [PATCH] ratchet: a re-record that could not tell a win from a loss MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `-update` wrote straight over BASELINE WITHOUT READING IT. The whole body was f, err := os.Create(*baseline) write(f, got, header) so the one thing it could never report is the one thing that matters: whether the sweep replacing the record was better or worse than the record. The comparison never happened at all. The hazard was already written down in this repository, TWICE, and gated in neither: scripts/.../cmd/guards/main.go:18 "UPDATE_BASELINE=1 records whatever it sees and would absorb a loss" .github/workflows/rubyspec-ratchet.yml:41 "the same run also showed kernel/require +1, equally environmental, and UPDATE_BASELINE=1 would bank it as progress." Two notes about the same gap, written months apart, neither of which could stop anything. This is the third time that pattern has been paid for here, and the answer is not a third note. You re-record to lock in a WIN. A file that went backwards is either a real regression or an environment that moved -- the lane is pinned to ubuntu-24.04 precisely because the environment changes the answer -- and both need a person, not an overwrite that erases the evidence of itself. -update now reads the old baseline, compares, prints every section the judging path prints, and REFUSES: re-record: 2206 files measured, 2206 in the baseline being replaced -- REGRESSED (1) -- core/a_spec.rb: 9 passing, baseline 10 (-1) ::error::REFUSING to re-record: 1 file(s) went backwards -- named above errWouldAbsorb is kept DISTINCT from errRegression although both exit 1: "this branch broke something" and "I declined to erase the evidence that something is broken" send a reader to different places. -absorb is the named derogation and it stamps the ARTEFACT, not just the log -- a console warning scrolls away and a baseline outlives the run that wrote it by months: # ⛔ ABSORBED 1 file(s) that went backwards, recorded with -absorb on 2026-11-03: # core/a_spec.rb: 9 passing, baseline 10 (-1) Tested, with the controls beside the refusals: - the refused re-record leaves BASELINE byte for byte as it was; a refusal that truncated the file would destroy the record it declined to replace; - a file that stops loading, and a file that vanishes from the sweep, refuse the same way; - a re-record that only improves is ACCEPTED and is not stamped -- without that control the rest only proves the gate always fires; - a FIRST recording, with no baseline to be worse than, is not a refusal. The existing round-trip test already re-recorded an improvement, so it passes unchanged, which is the cheapest control of all. Refs #799 Co-Authored-By: Claude Opus 5 --- scripts/conformance/rubyspec/README.md | 47 +++++++ .../conformance/rubyspec/cmd/ratchet/main.go | 73 ++++++++++- .../rubyspec/cmd/ratchet/main_test.go | 120 ++++++++++++++++++ 3 files changed, 238 insertions(+), 2 deletions(-) diff --git a/scripts/conformance/rubyspec/README.md b/scripts/conformance/rubyspec/README.md index e271e206..a09f2067 100644 --- a/scripts/conformance/rubyspec/README.md +++ b/scripts/conformance/rubyspec/README.md @@ -80,6 +80,53 @@ and be recorded as not loading, which bakes the load into the gate. The CI job (`.github/workflows/rubyspec-ratchet.yml`) enforces the baseline on every push and pull request, and raises `TIMEOUT` to 60s for exactly this reason. +### ⛔ A re-record reads the old baseline first, and refuses to bank a loss + +`-update` used to write straight over `BASELINE` **without reading it**, so the +one thing it could never report was whether the sweep replacing the record was +better or worse than it. The comparison simply never happened. + +The hazard was written down twice in this repository and gated in neither — +`cmd/guards`' own header (*"`UPDATE_BASELINE=1` records whatever it sees and +would absorb a loss"*) and the ratchet lane's comment about an Ubuntu image +roll: *"the same run also showed `kernel/require` +1, equally environmental, and +`UPDATE_BASELINE=1` would bank it as progress."* Two notes, no refusal. + +You re-record to lock in a **win**. A file that went backwards is either a real +regression or an environment that moved, and both need a person — not an +overwrite that erases the evidence of itself: + +```console +$ UPDATE_BASELINE=1 scripts/conformance/rubyspec/run.sh +re-record: 2206 files measured, 2206 in the baseline being replaced + +-- REGRESSED (1) -- + core/a_spec.rb: 9 passing, baseline 10 (-1) + +::error::REFUSING to re-record: 1 file(s) went backwards -- named above +A re-record locks in a win. Fix the regression, or pass -absorb to record +it on purpose -- the baseline then carries a line naming every file absorbed. +``` + +It exits **1**, distinct in the source from the plain regression verdict +although they share the status: *"this branch broke something"* and *"I declined +to erase the evidence that something is broken"* send a reader to different +places. + +`-absorb` is the named derogation and it **stamps the artefact**, not just the +log — a console warning scrolls away, and the baseline outlives the run that +wrote it by months: + +```text +# ⛔ ABSORBED 1 file(s) that went backwards, recorded with -absorb on 2026-11-03: +# core/a_spec.rb: 9 passing, baseline 10 (-1) +``` + +A refused re-record leaves `BASELINE` **byte for byte as it was**, which is +asserted: a refusal that truncated the file would destroy the record it declined +to replace. A first recording, with no baseline to be worse than, is not a +refusal at all. + ## `cmd/phantom` — is a pass the behaviour, or the method's absence? A spec that asserts an exception class cannot tell why it got one. diff --git a/scripts/conformance/rubyspec/cmd/ratchet/main.go b/scripts/conformance/rubyspec/cmd/ratchet/main.go index 05350f11..725dc556 100644 --- a/scripts/conformance/rubyspec/cmd/ratchet/main.go +++ b/scripts/conformance/rubyspec/cmd/ratchet/main.go @@ -33,6 +33,7 @@ import ( "sort" "strconv" "strings" + "time" ) // state is what the baseline records for one file, and what a sweep measures. @@ -183,12 +184,21 @@ func section(w io.Writer, title string, items []string, limit int) { // verdict is reachable from a test. var errRegression = errors.New("regression") +// errWouldAbsorb is the re-record refusing to overwrite a better baseline with +// a worse sweep. It is kept DISTINCT from errRegression although both exit 1: +// one says "this branch broke something", the other says "I declined to erase +// the evidence that something is broken", and a reader who confuses them looks +// in the wrong place. +var errWouldAbsorb = errors.New("would absorb a regression") + func main() { err := run(os.Args[1:], os.Stdin, os.Stdout) switch { case err == nil: return - case errors.Is(err, errRegression): + case errors.Is(err, errRegression), errors.Is(err, errWouldAbsorb): + // Both are answers, not malfunctions: exit 1, never the 2 that means + // the tool could not read its input. os.Exit(1) default: fmt.Fprintf(os.Stderr, "ratchet: %v\n", err) @@ -202,6 +212,7 @@ func run(args []string, stdin io.Reader, stdout io.Writer) error { baseline := fs.String("baseline", "", "per-file baseline TSV (required)") results := fs.String("results", "", "sweep results TSV (default: stdin)") update := fs.Bool("update", false, "rewrite the baseline from this sweep and exit 0") + absorb := fs.Bool("absorb", false, "with -update, re-record even though files went backwards; the baseline is stamped with their names") limit := fs.Int("limit", 40, "cap each printed section at N entries (0 = no cap)") if err := fs.Parse(args); err != nil { return err @@ -225,9 +236,67 @@ func run(args []string, stdin io.Reader, stdout io.Writer) error { } if *update { - const header = "# ruby/spec per-file baseline. Regenerate with UPDATE_BASELINE=1 scripts/conformance/rubyspec/run.sh.\n" + + header := "# ruby/spec per-file baseline. Regenerate with UPDATE_BASELINE=1 scripts/conformance/rubyspec/run.sh.\n" + "# One record per spec file: OKpathpassing, or FILEFAILpath.\n" + "# Judged per file, so no margin is needed and every move is attributable.\n" + + // ⛔ A RE-RECORD READS THE OLD BASELINE FIRST. It used to write straight + // over it, so the one thing it could never report is the one thing that + // matters: -update had no idea whether this sweep was better or worse + // than the record it was replacing. The comparison simply never happened. + // + // The hazard was already written down, in this repository, twice — + // cmd/guards' header ("UPDATE_BASELINE=1 records whatever it sees and + // would absorb a loss") and the ratchet lane's own comment about an + // Ubuntu image roll: "the same run also showed kernel/require +1, + // equally environmental, and UPDATE_BASELINE=1 would bank it as + // progress." A note in two places and no refusal in either. + // + // You re-record to lock in a WIN. A file that went backwards is either a + // real regression or an environment that moved, and both need a person, + // not an overwrite that erases the evidence of itself. + var base map[string]state + if bf, err := os.Open(*baseline); err == nil { + base, err = parse(bf, "baseline") + bf.Close() + if err != nil { + return err + } + } else if !os.IsNotExist(err) { + return err + } + // No baseline yet is the first recording, and has nothing to go + // backwards from — distinct from a baseline that exists and is worse. + if base != nil { + v := compare(base, got) + fmt.Fprintf(stdout, "re-record: %d files measured, %d in the baseline being replaced\n", len(got), len(base)) + section(stdout, "REGRESSED", v.regressed, *limit) + section(stdout, "FAILED TO LOAD (the baseline says they load)", v.unloaded, *limit) + section(stdout, "MISSING FROM THE SWEEP ENTIRELY", v.missing, *limit) + section(stdout, "improved", v.improved, *limit) + section(stdout, "now loading", v.nowLoading, *limit) + section(stdout, "new files, not in the baseline", v.added, *limit) + if v.bad() { + n := len(v.regressed) + len(v.unloaded) + len(v.missing) + if !*absorb { + fmt.Fprintf(stdout, "\n::error::REFUSING to re-record: %d file(s) went backwards -- named above\n", n) + fmt.Fprintln(stdout, "A re-record locks in a win. Fix the regression, or pass -absorb to record") + fmt.Fprintln(stdout, "it on purpose -- the baseline then carries a line naming every file absorbed.") + return errWouldAbsorb + } + // ⛔ The derogation STAMPS THE ARTEFACT, not just the log. A + // console warning scrolls away and the baseline outlives it by + // months; the next reader has to be able to see the loss in the + // file that was written, without having the run that wrote it. + header += fmt.Sprintf("# ⛔ ABSORBED %d file(s) that went backwards, recorded with -absorb on %s:\n", + n, time.Now().UTC().Format("2006-01-02")) + for _, s := range append(append(append([]string{}, v.regressed...), v.unloaded...), v.missing...) { + header += "# " + s + "\n" + } + fmt.Fprintf(stdout, "\n⛔ -absorb: recording %d file(s) that went backwards; the baseline names them.\n", n) + } + } + f, err := os.Create(*baseline) if err != nil { return err diff --git a/scripts/conformance/rubyspec/cmd/ratchet/main_test.go b/scripts/conformance/rubyspec/cmd/ratchet/main_test.go index e5e13d86..a04a1173 100644 --- a/scripts/conformance/rubyspec/cmd/ratchet/main_test.go +++ b/scripts/conformance/rubyspec/cmd/ratchet/main_test.go @@ -166,6 +166,126 @@ func TestRatchetUpdateRoundTrips(t *testing.T) { } } +// ⛔ -update used to write straight over the baseline WITHOUT READING IT, so the +// one thing it could never report was whether the sweep replacing the record was +// better or worse than it. The hazard was written down twice in this repository +// — cmd/guards' header, and the ratchet lane's comment about an Ubuntu image +// roll ("UPDATE_BASELINE=1 would bank it as progress") — and gated in neither. +// +// recordOver re-records `sweep` over the three-file baseline and returns the +// tool's output, its error, and what the baseline file holds afterwards. +func recordOver(t *testing.T, sweep string, extra ...string) (string, error, string) { + t.Helper() + dir := t.TempDir() + bp := filepath.Join(dir, "BASELINE") + if err := os.WriteFile(bp, []byte(baseline3), 0o644); err != nil { + t.Fatal(err) + } + var out strings.Builder + err := run(append([]string{"-baseline", bp, "-update"}, extra...), strings.NewReader(sweep), &out) + after, rerr := os.ReadFile(bp) + if rerr != nil { + t.Fatal(rerr) + } + return out.String(), err, string(after) +} + +func TestARerecordWillNotSilentlyBankALoss(t *testing.T) { + // a_spec goes 10 → 9 while b_spec gains: a NET of zero, which is exactly the + // shape no total can see. + lost := "OK\tcore/a_spec.rb\t9\nOK\tcore/b_spec.rb\t21\nFILEFAIL\tcore/c_spec.rb\n" + + out, err, after := recordOver(t, lost) + if !errors.Is(err, errWouldAbsorb) { + t.Fatalf("err = %v, want errWouldAbsorb; out:\n%s", err, out) + } + if !errors.Is(err, errWouldAbsorb) || errors.Is(err, errRegression) { + t.Errorf("the refusal must be distinct from a plain regression verdict") + } + if !strings.Contains(out, "core/a_spec.rb: 9 passing, baseline 10 (-1)") { + t.Errorf("the refusal does not name the file that went backwards:\n%s", out) + } + // ⛔ And it must not have half-written. A refusal that truncated the file + // would destroy the very record it declined to replace. + if after != baseline3 { + t.Errorf("the refused re-record changed the baseline:\n%q", after) + } + + // -absorb is the named derogation, and it STAMPS THE ARTEFACT: the console + // scrolls away, the baseline outlives the run by months. + out, err, after = recordOver(t, lost, "-absorb") + if err != nil { + t.Fatalf("-absorb: %v\n%s", err, out) + } + if !strings.Contains(after, "ABSORBED 1 file(s)") || !strings.Contains(after, "core/a_spec.rb: 9 passing, baseline 10 (-1)") { + t.Errorf("-absorb did not stamp the baseline with what it absorbed:\n%s", after) + } + if !strings.Contains(after, "OK\tcore/a_spec.rb\t9\n") { + t.Errorf("-absorb did not actually record the sweep:\n%s", after) + } + + // The CONTROL, without which the above only proves the gate always fires: + // a re-record that merely locks in a win is accepted, and is NOT stamped. + won := "OK\tcore/a_spec.rb\t12\nOK\tcore/b_spec.rb\t20\nFILEFAIL\tcore/c_spec.rb\n" + out, err, after = recordOver(t, won) + if err != nil { + t.Fatalf("a winning re-record was refused: %v\n%s", err, out) + } + if strings.Contains(after, "ABSORBED") { + t.Errorf("a clean re-record was stamped:\n%s", after) + } + if !strings.Contains(out, "improved (1)") { + t.Errorf("the re-record does not report what it locked in:\n%s", out) + } +} + +// The other two ways a file goes backwards are the same refusal, and a first +// recording -- with no baseline to be worse than -- is not one at all. +func TestTheRerecordRefusalCoversEveryWayBackwards(t *testing.T) { + for _, tc := range []struct { + name, sweep, want string + }{ + { + "a file stops loading", + "OK\tcore/a_spec.rb\t10\nFILEFAIL\tcore/b_spec.rb\nFILEFAIL\tcore/c_spec.rb\n", + "core/b_spec.rb", + }, + { + "a file vanishes from the sweep", + "OK\tcore/a_spec.rb\t10\nFILEFAIL\tcore/c_spec.rb\n", + "core/b_spec.rb", + }, + } { + t.Run(tc.name, func(t *testing.T) { + out, err, after := recordOver(t, tc.sweep) + if !errors.Is(err, errWouldAbsorb) { + t.Fatalf("err = %v, want errWouldAbsorb; out:\n%s", err, out) + } + if !strings.Contains(out, tc.want) { + t.Errorf("the refusal does not name %s:\n%s", tc.want, out) + } + if after != baseline3 { + t.Errorf("the refused re-record changed the baseline") + } + }) + } + + t.Run("the first recording has nothing to go backwards from", func(t *testing.T) { + bp := filepath.Join(t.TempDir(), "BASELINE") + var out strings.Builder + if err := run([]string{"-baseline", bp, "-update"}, strings.NewReader(baseline3), &out); err != nil { + t.Fatalf("a first recording was refused: %v\n%s", err, out.String()) + } + got, err := os.ReadFile(bp) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(got), "OK\tcore/a_spec.rb\t10\n") { + t.Errorf("the first recording did not write the sweep:\n%s", got) + } + }) +} + // TestRatchetMissingBaselineIsAnError: pointing at a baseline that is not there // must fail loudly. A tool that treats an absent baseline as "nothing to beat" // would pass forever.