fix(ci): two gates that could not fail, and the arithmetic that says so (#829, #559)

.pre-commit-config.yaml

@@ -77,10 +77,19 @@ repos:
   # Go quality gate — mirrors the CI workflow and the go-production-engineer standard.
   - repo: local
     hooks:
+      # The exit code is read, and that is the whole fix (#829). The entry used
+      # to be `test -z "$(gofmt -l .)" || ...`, which discards gofmt's status: a
+      # gofmt that cannot run produces an empty substitution, `test -z ""` is
+      # true, and the hook reports **Passed** -- green, asserting it looked. The
+      # window is a fresh clone before `mise trust`, which is a new
+      # contributor's first commit. go vet and golangci-lint below never had it,
+      # because they propagate their own status.
+      #
+      # tools/ci's TestTheGofmtHookFailsWhenGofmtCannotRun fails without this.
       - id: gofmt
         name: gofmt
         language: system
-        entry: bash -c 'test -z "$(gofmt -l .)" || { echo "gofmt needed:"; gofmt -l .; exit 1; }'
+        entry: bash -c 'out=$(gofmt -l .) || { echo "gofmt could not run" >&2; exit 1; }; test -z "$out" || { echo "gofmt needed:"; echo "$out"; exit 1; }'
         pass_filenames: false
         files: \.go$
       - id: go-vet

tools/ci/balancer_sample_test.go

@@ -0,0 +1,113 @@
+package ci
+
+import (
+	"math"
+	"os"
+	"path/filepath"
+	"regexp"
+	"strconv"
+	"strings"
+	"testing"
+)
+
+// maxFalseVerdict is the rate this repository is willing to call acceptable for
+// a verdict drawn from a fixed sample of coin flips.
+//
+// 1e-6 is a chosen horizon rather than a measured one, and the choice is the
+// usual one: small enough that nobody meets it over the life of the suite,
+// large enough that the sample stays cheap. What matters more than the number
+// is that it is written down, because the defect it replaces was a number
+// nobody had computed.
+const maxFalseVerdict = 1e-6
+
+var (
+	probeCall      = regexp.MustCompile(`probe\s+"?\$?\{?([A-Za-z_][A-Za-z0-9_]*|[0-9]+)\}?"?\s*\)`)
+	probeConstant  = regexp.MustCompile(`(?m)^DISTRIBUTION_PROBES=([0-9]+)`)
+	perMachineCase = regexp.MustCompile(`\*"\$vm_[ab]"\*`)
+)
+
+// TestABalancerVerdictDrawsFromASampleLargeEnoughToMeanIt is cited by
+// tools/conformance/outscale/balancer.sh.
+//
+// ## The defect (#559)
+//
+// The suite asked six connections whether a balancer reaches both machines. Six
+// is a coin flipped six times: a balancer that works sends every connection to
+// one side with probability 2 × 0.5⁶ = **3.1 %**, so the gate reddened about
+// one run in thirty-two on a subject that was behaving. A gate that reds on a
+// working system teaches re-running it, which this repository has written down
+// as worse than having no gate at all — and this one sits on the dataplane
+// suite, whose subject is the product's own differentiator.
+//
+// The same arithmetic runs the other way at the unlink site: six probes that
+// happen to avoid a machine the unlink never reached is 0.5⁶ = 1.6 %, a false
+// GREEN over a real defect. That site's comment already named it "#559's defect
+// with a different number".
+//
+// ## What this asserts, and why it reads the script
+//
+// The subject *is* the sample size, so the control reads it — but it checks a
+// property rather than a value: every `probe` call whose verdict names a
+// specific machine must draw from the declared constant, and that constant must
+// put the false-verdict rate under maxFalseVerdict. Raising or lowering the
+// number stays free; going back to a literal, or to a number that cannot
+// support the claim, does not.
+//
+// A liveness probe ("did every connection get an answer") is deliberately out
+// of scope: it has no false negative more probes would remove, and demanding
+// the big sample there would buy wall-clock and nothing else.
+func TestABalancerVerdictDrawsFromASampleLargeEnoughToMeanIt(t *testing.T) {
+	path, err := filepath.Abs(filepath.Join("..", "conformance", "outscale", "balancer.sh"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	body, err := os.ReadFile(path) // #nosec G304 -- a fixed path inside the repository
+	if err != nil {
+		t.Fatal(err)
+	}
+	script := string(body)
+
+	declared := probeConstant.FindStringSubmatch(script)
+	if declared == nil {
+		t.Fatal("balancer.sh declares no DISTRIBUTION_PROBES: the sample a per-machine " +
+			"verdict is drawn from must be named once, with its arithmetic, not spelled at each site")
+	}
+	n, err := strconv.Atoi(declared[1])
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Both tails: every connection landing on one side, either side.
+	rate := 2 * math.Pow(0.5, float64(n))
+	if rate > maxFalseVerdict {
+		t.Errorf("DISTRIBUTION_PROBES=%d puts a working balancer's false verdict at %.3g, "+
+			"over the %.0e this repository accepts: a gate that reds on a healthy subject "+
+			"teaches the reflex of re-running it", n, rate, maxFalseVerdict)
+	}
+
+	// Every per-machine verdict draws from that constant. Walk the lines, and
+	// for each `probe <something>` look ahead at the case arms it feeds.
+	lines := strings.Split(script, "\n")
+	checked := 0
+	for i, line := range lines {
+		call := probeCall.FindStringSubmatch(line)
+		if call == nil {
+			continue
+		}
+		window := strings.Join(lines[i:min(i+10, len(lines))], "\n")
+		if !perMachineCase.MatchString(window) {
+			continue // a liveness probe: out of scope, see above
+		}
+		checked++
+		if call[1] != "DISTRIBUTION_PROBES" {
+			t.Errorf("line %d draws a per-machine verdict from %q instead of DISTRIBUTION_PROBES: "+
+				"a sample written at the site is one that stops agreeing with the arithmetic "+
+				"beside the constant", i+1, call[1])
+		}
+	}
+	// A denominator, because a loop over nothing passes silently — and would
+	// have passed before this suite had any per-machine verdict at all.
+	if checked < 2 {
+		t.Fatalf("only %d per-machine verdict(s) found; this suite draws two, "+
+			"the distribution and the unlink", checked)
+	}
+}

tools/ci/gofmt_hook_test.go

@@ -0,0 +1,146 @@
+package ci
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// gofmtHookEntry reads the `entry:` line of the gofmt hook out of
+// .pre-commit-config.yaml.
+//
+// Out of the file rather than written here, for the reason every gate in this
+// repository that quotes a document reads it instead: a copy is a second
+// source, and a test holding its own copy of the command proves the copy works.
+func gofmtHookEntry(t *testing.T) string {
+	t.Helper()
+	path, err := filepath.Abs(filepath.Join("..", "..", ".pre-commit-config.yaml"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	body, err := os.ReadFile(path) // #nosec G304 -- a fixed path inside the repository
+	if err != nil {
+		t.Fatal(err)
+	}
+	lines := strings.Split(string(body), "\n")
+	for i, line := range lines {
+		if strings.TrimSpace(line) != "- id: gofmt" {
+			continue
+		}
+		for _, next := range lines[i:] {
+			trimmed := strings.TrimSpace(next)
+			if after, found := strings.CutPrefix(trimmed, "entry: bash -c "); found {
+				return strings.Trim(after, "'")
+			}
+		}
+	}
+	t.Fatal("no gofmt hook with a bash entry in .pre-commit-config.yaml")
+	return ""
+}
+
+// runHook executes the hook's own command with a PATH of this test's choosing,
+// and answers its exit code. A directory holding only what the test put there
+// is how "gofmt cannot run" is produced without touching the station.
+func runHook(t *testing.T, entry, path, dir string) (int, string) {
+	t.Helper()
+	cmd := exec.Command("bash", "-c", entry)
+	cmd.Dir = dir
+	cmd.Env = []string{"PATH=" + path, "HOME=" + t.TempDir()}
+	out, err := cmd.CombinedOutput()
+	code := 0
+	var exit *exec.ExitError
+	if err != nil {
+		if !asExitError(err, &exit) {
+			t.Fatalf("running the hook: %v\n%s", err, out)
+		}
+		code = exit.ExitCode()
+	}
+	return code, string(out)
+}
+
+// TestTheGofmtHookFailsWhenGofmtCannotRun is cited by .pre-commit-config.yaml.
+//
+// ## The defect, as it was measured (#829)
+//
+// The entry was `test -z "$(gofmt -l .)" || { … exit 1; }`. `gofmt -l` writes
+// the offending files to stdout and its own errors to stderr, and the verdict
+// was `test -z` over the captured stdout alone — the exit code was never read.
+// So any failure of gofmt produced an empty substitution, `test -z ""` was
+// true, and the hook exited 0.
+//
+// Not `Skipped`, not red: **Passed, asserting it looked**. Its window is a
+// fresh clone before `mise trust`, which is a new contributor's first commit,
+// and it is the only gate in this file with that shape — `go vet` and
+// `golangci-lint` propagate their own status, and both reddened at the same
+// instant on the same cause, which is how the gofmt green was noticed at all.
+//
+// ## Why this test runs the command instead of reading it
+//
+// A test asserting that the entry *contains* `||` or `$?` would pass on any
+// future rewrite that happens to keep the characters and lose the property. The
+// property is behavioural — a hook that cannot measure must not report success
+// — so it is measured, by running the hook's own line with a PATH that holds no
+// gofmt.
+//
+// The accepting half is asserted in the same file, because a hook that always
+// failed would satisfy the test above and refuse every commit, which this
+// repository has written down as worse than no gate at all.
+func TestTheGofmtHookFailsWhenGofmtCannotRun(t *testing.T) {
+	entry := gofmtHookEntry(t)
+
+	// A PATH holding bash's own dependencies and no gofmt. Not an empty PATH:
+	// the entry needs `test` and `echo`, which are builtins, but a future entry
+	// may not be, and an empty PATH would then fail for a reason that is not
+	// the subject.
+	empty := t.TempDir()
+	code, out := runHook(t, entry, empty, t.TempDir())
+	if code == 0 {
+		t.Fatalf("the hook reported success with no gofmt on PATH: it asserts it looked "+
+			"when it could not look, which is the defect #829 names\n%s", out)
+	}
+	if !strings.Contains(out, "could not run") {
+		t.Errorf("the hook failed without saying it could not measure:\n%s", out)
+	}
+}
+
+// TestTheGofmtHookPassesOnFormattedCodeAndFailsOnUnformatted is the accepting
+// half, and the half that keeps the fix from being a refusal of everything.
+//
+// Both directions in one test on purpose: a guard satisfied only by its
+// refusing case is one nobody can tell from a broken one.
+func TestTheGofmtHookPassesOnFormattedCodeAndFailsOnUnformatted(t *testing.T) {
+	entry := gofmtHookEntry(t)
+	path := os.Getenv("PATH")
+	if _, err := exec.LookPath("gofmt"); err != nil {
+		t.Skip("gofmt is not on PATH, so the accepting half cannot be measured here")
+	}
+
+	t.Run("formatted", func(t *testing.T) {
+		dir := t.TempDir()
+		write(t, dir, "a.go", "package p\n\nfunc a() {}\n")
+		if code, out := runHook(t, entry, path, dir); code != 0 {
+			t.Errorf("the hook refused correctly formatted Go (exit %d):\n%s", code, out)
+		}
+	})
+
+	t.Run("unformatted", func(t *testing.T) {
+		dir := t.TempDir()
+		write(t, dir, "a.go", "package p\nfunc a()    {}\n")
+		code, out := runHook(t, entry, path, dir)
+		if code == 0 {
+			t.Errorf("the hook accepted Go that gofmt rewrites:\n%s", out)
+		}
+		if !strings.Contains(out, "a.go") {
+			t.Errorf("the refusal does not name the file to fix:\n%s", out)
+		}
+	})
+}
+
+func write(t *testing.T, dir, name, body string) {
+	t.Helper()
+	if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o600); err != nil {
+		t.Fatal(err)
+	}
+}

tools/conformance/outscale/balancer.sh

@@ -203,14 +203,38 @@ probe() {
   printf '%s' "$out"
 }
 
+# How many connections a verdict about DISTRIBUTION is drawn from (#559).
+#
+# Six was the number here, and six is a coin flipped six times. A balancer that
+# works sends every connection to one side with probability 2 x 0.5^6 = 3.1 %,
+# so this suite reddened about one run in thirty-two on a subject that was
+# behaving — and a gate that reds on a working system teaches the reflex of
+# re-running it, which this repository has written down as worse than having no
+# gate at all.
+#
+# The same arithmetic runs the other way after an unlink: six probes that
+# happen to avoid a machine the unlink never reached is 0.5^6 = 1.6 %, which is
+# a false GREEN over a real defect. The comment at that site already named it
+# "#559's defect with a different number"; it is the same number and it is
+# fixed in the same place.
+#
+# 24 puts both at 1.2e-7 and 6.0e-8. The cost is about ten seconds on a leg
+# that measures in minutes, which is the whole reason the cheap answer is the
+# right one here rather than a cleverer statistic.
+#
+# What it still does NOT prove, and the suite must not be read as claiming: that
+# the distribution is FAIR. Both machines being reached says the balancer has
+# more than one backend, nothing about the ratio.
+DISTRIBUTION_PROBES=24
+
 echo "- the balancer distributes to both machines"
-hits="$(probe 6)"
+hits="$(probe "$DISTRIBUTION_PROBES")"
 case "$hits" in
   *TIMEOUT*) fail "the balancer did not answer every probe: $hits" ;;
 esac
-case "$hits" in *"$vm_a"*) ;; *) fail "no connection reached $vm_a: $hits" ;; esac
-case "$hits" in *"$vm_b"*) ;; *) fail "no connection reached $vm_b: $hits" ;; esac
-ok "6/6 answered, over both machines:$hits"
+case "$hits" in *"$vm_a"*) ;; *) fail "no connection reached $vm_a over $DISTRIBUTION_PROBES probes: $hits" ;; esac
+case "$hits" in *"$vm_b"*) ;; *) fail "no connection reached $vm_b over $DISTRIBUTION_PROBES probes: $hits" ;; esac
+ok "$DISTRIBUTION_PROBES/$DISTRIBUTION_PROBES answered, over both machines:$hits"
 
 # The measurement #315 turned on. An address the runtime has to announce outside
 # the network answers for two minutes and goes dark; an address of the network's
@@ -226,24 +250,31 @@ echo "- and it is still there a minute later"
 # observe nothing changing is to let time pass. Shortening it shortens the
 # claim. docs/limits.md carries the full curve.
 sleep 60
-hits="$(probe 6)"
+# Six is enough HERE, and the difference is the point: this verdict is about
+# liveness, not distribution. "Did every connection get an answer" has no
+# false-negative that more probes would remove, so the count stays where the
+# wall-clock is already a minute.
+liveness=6
+hits="$(probe "$liveness")"
 case "$hits" in
   *TIMEOUT*) fail "the balancer went dark within a minute: $hits" ;;
 esac
-ok "6/6 answered again:$hits"
+ok "$liveness/$liveness answered again:$hits"
 
 # The replace-not-patch rule, measured rather than asserted in a unit test: a
 # machine the API has stopped listing must stop receiving connections.
 echo "- an unlinked machine stops receiving connections"
 osc UnlinkLoadBalancerBackendMachines --LoadBalancerName "$lb_name" --BackendVmIds "$vm_a" >/dev/null \
   || fail "UnlinkLoadBalancerBackendMachines rejected"
 # waits on silence: the verdict below is that $vm_a NEVER answers again across
-# six connections. Nothing announces an unlink reaching the runtime, and the
-# only poll available — "six probes that avoided $vm_a" — is satisfied by luck
-# one run in sixty-four with the unlink not applied at all. That is #559's
-# defect with a different number, so this wait stays fixed.
+# $DISTRIBUTION_PROBES connections. Nothing announces an unlink reaching the
+# runtime, and the only poll available — "probes that avoided $vm_a" — is
+# satisfied by luck when the unlink has not applied at all. At six probes that
+# luck ran one run in sixty-four, which is #559 pointing the other way: a false
+# GREEN over a real defect. The probe count carries the fix; this wait stays
+# fixed, because a verdict drawn from an absence cannot poll for it.
 sleep 2
-hits="$(probe 6)"
+hits="$(probe "$DISTRIBUTION_PROBES")"
 case "$hits" in
   *TIMEOUT*) fail "the balancer stopped answering after an unlink: $hits" ;;
   *"$vm_a"*) fail "$vm_a still receives connections after being unlinked: $hits" ;;

tools/falsify/specs/a-hook-that-cannot-measure-fails.json

@@ -0,0 +1,20 @@
+{
+ "package": "./tools/ci/",
+ "note": "The subject is a gate reporting success when it could not look. The gofmt pre-commit hook read `test -z \"$(gofmt -l .)\"` and never its exit code: `gofmt -l` writes offending files to stdout and its own errors to stderr, so any failure of gofmt produced an empty substitution, `test -z \"\"` was true, and the hook exited 0. Not skipped, not red — Passed, asserting it looked. Its window is a fresh clone before `mise trust`, which is a new contributor's first commit (#829).\n\nIt is the gate-level form of the defect this repository names most often: a control that cannot fail is a comment. It was found while running the measurement #739 asks for, which is the detail worth keeping — the harness broke first and the hook agreed with it, and `go vet` and `golangci-lint` reddening at the same instant on the same cause is the only reason the gofmt green stood out.\n\nThe first mutation restores the defect exactly. The second is the over-correction, and it is mutated because a hook that refuses everything passes the first test and refuses every commit, which teaches `--no-verify` and disarms every other hook at once — this repository has that written down as worse than no gate at all.\n\nBoth tests run the hook's own `entry:` line, read out of .pre-commit-config.yaml rather than copied into Go. A test asserting the entry *contains* `||` would pass on any rewrite that kept the characters and lost the property.",
+ "mutations": [
+  {
+   "label": "the entry goes back to `test -z \"$(gofmt -l .)\"`, which discards gofmt's exit code. This is the defect verbatim: with no gofmt on PATH the substitution is empty, the test is true, and the hook reports success over code nobody formatted",
+   "file": ".pre-commit-config.yaml",
+   "find": "        entry: bash -c 'out=$(gofmt -l .) || { echo \"gofmt could not run\" >&2; exit 1; }; test -z \"$out\" || { echo \"gofmt needed:\"; echo \"$out\"; exit 1; }'",
+   "replace": "        entry: bash -c 'test -z \"$(gofmt -l .)\" || { echo \"gofmt needed:\"; gofmt -l .; exit 1; }'",
+   "test": "TestTheGofmtHookFailsWhenGofmtCannotRun"
+  },
+  {
+   "label": "the hook refuses whatever it is given, which passes the refusing test and rejects every commit including correctly formatted Go. The shape that teaches `--no-verify`, and `--no-verify` disarms every other hook in this file at once",
+   "file": ".pre-commit-config.yaml",
+   "find": "        entry: bash -c 'out=$(gofmt -l .) || { echo \"gofmt could not run\" >&2; exit 1; }; test -z \"$out\" || { echo \"gofmt needed:\"; echo \"$out\"; exit 1; }'",
+   "replace": "        entry: bash -c 'out=$(gofmt -l .) || { echo \"gofmt could not run\" >&2; exit 1; }; echo \"gofmt needed:\"; echo \"$out\"; exit 1'",
+   "test": "TestTheGofmtHookPassesOnFormattedCodeAndFailsOnUnformatted"
+  }
+ ]
+}