From 42c169f09fcc617dfaeb8bf14bb3e3754df73e02 Mon Sep 17 00:00:00 2001 From: "Tom D. Snyder" Date: Fri, 7 Aug 2026 00:14:35 -0400 Subject: [PATCH] Fix the split guard reporting a cached PASS in CI main went red on the negative control, and it went red for the right reason: the control caught the guard reporting "ok (cached)" while never looking at the injected internal/dast import. A guard that can replay a stale PASS is exactly the failure mode the control exists to detect, so this is the mechanism working, not a flaky check. Cause: TestSplit's verdict comes from the output of an external `go list -deps` process, which Go's test-result cache does not track. With the build cache restored by setup-go's cache: true, `go test` can serve a previous PASS for a package whose import graph has since changed. Fixes, in increasing order of how much they buy: 1. -count=1 on all three TestSplit invocations, and a prominent warning in split_test.go's doc comment so the trap is not reintroduced by someone running it another way. This alone removes the dependence on cache behaviour. 2. The negative control now asserts the RIGHT failure instead of any failure. A nonzero exit proves only that something went wrong -- a compile error would also produce one and would let a broken guard pass for a working one. It now requires the output to name internal/dast/ciprobe, and explicitly fails if it ever sees "(cached)" again. Honest note on evidence: I could not reproduce the stale PASS locally. Locally the package hash changes when the file is added, so the cache correctly misses and the guard fails as intended both with and without -count=1. The stale result appeared only in CI against a restored cache. Rather than guess at the precise cache-key mechanism, -count=1 removes the question, and the "(cached)" assertion turns any recurrence into an explicit, self-describing failure instead of a silent green. Local evidence: with the violation injected, `go test -count=1 ./cmd/anvil -run TestSplit` FAILS naming internal/dast/probe; after revert it passes; gofmt clean. --- .github/workflows/ci.yml | 35 +++++++++++++++++++++++++++++++---- cmd/anvil/split_test.go | 10 ++++++++++ 2 files changed, 41 insertions(+), 4 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fceae50..7c1a366 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,8 +69,15 @@ jobs: go-version-file: go.mod cache: true + # -count=1 is load-bearing, not decoration. TestSplit shells out to + # `go list -deps`, and Go's test-result cache does not track the output of + # an external command -- so with the build cache restored by setup-go it + # will happily replay a previous PASS for a package whose import graph has + # since changed. That is exactly the stale-pass this guard exists to + # prevent, and it defeated the negative control on the first run against + # a warm cache. - name: Core binary has no DAST capability in its import graph - run: go test ./cmd/anvil -run TestSplit -v + run: go test -count=1 ./cmd/anvil -run TestSplit -v - name: Negative control -- the guard must be able to fail run: | @@ -86,14 +93,34 @@ jobs: import _ "github.com/Susquehanna-Syntax/Anvil/internal/dast/ciprobe" EOF - if go test ./cmd/anvil -run TestSplit; then + # Capture the output rather than just the exit code. A nonzero exit + # proves only that something went wrong -- a compile error would also + # produce one, and would let a broken guard masquerade as a working + # one. Assert the guard failed for the RIGHT reason, naming the + # package it was supposed to catch. + set +e + out=$(go test -count=1 ./cmd/anvil -run TestSplit 2>&1) + rc=$? + set -e + echo "$out" + if [ $rc -eq 0 ]; then echo "::error::TestSplit PASSED with an internal/dast import present. The guard is a no-op." exit 1 fi - echo "guard correctly failed on the injected violation" + if ! echo "$out" | grep -q 'internal/dast/ciprobe'; then + echo "::error::TestSplit failed, but not by detecting internal/dast/ciprobe. The guard may be broken rather than working." + exit 1 + fi + case "$out" in + *"(cached)"*) + echo "::error::Go replayed a cached result instead of running the test. -count=1 is not taking effect." + exit 1 + ;; + esac + echo "guard correctly failed on the injected violation, naming internal/dast/ciprobe" rm -f cmd/anvil/ci_negctl.go rm -rf internal/dast/ciprobe - go test ./cmd/anvil -run TestSplit + go test -count=1 ./cmd/anvil -run TestSplit - name: Static, cgo-free builds for both artifacts env: diff --git a/cmd/anvil/split_test.go b/cmd/anvil/split_test.go index 0a962ca..a1fab49 100644 --- a/cmd/anvil/split_test.go +++ b/cmd/anvil/split_test.go @@ -24,6 +24,16 @@ import ( // guards do. Plan step O.16 owns its final form and must demonstrate the // negative control: add a temporary internal/dast import, watch this fail, // revert. A guard that has never failed has not been tested. +// +// ALWAYS RUN THIS WITH -count=1. +// +// This test's verdict comes from the output of an external `go list` process, +// and Go's test-result cache does not track that. Against a warm build cache it +// will replay a previous PASS for a package whose import graph has since +// changed -- reporting "ok (cached)" while never looking at the new import. +// That is precisely the stale-pass this guard exists to prevent, and it is not +// hypothetical: it defeated the CI negative control on its first run against a +// restored cache. -count=1 forces the run. func TestSplit_CoreBinaryHasNoDASTCapability(t *testing.T) { const forbidden = "/internal/dast"