Keep covered checks out of the protected prefix after an earlier stop (#406)

lab/internal/molecule/accept_protected_failures.go

@@ -322,7 +322,8 @@ func failingProtectedChecks(p readiness.Plan, result verification.Result) ([]fai
 // result stopped on and its observation. The protected view must have failed for
 // that check's own exit, not for setup, a definition change or a source mutation.
 // An earlier check may have failed only where the view accepted it under declared
-// deltas.
+// deltas. The checks that ran must be the plan's regression checks in plan order, up to
+// the one it stopped on, less the covered checks protectedRanChecks leaves out.
 func failingProtectedCheck(p readiness.Plan, result verification.Result) (readiness.Check, verification.Observation, error) {
 	v := result.Protected
 	if v == nil || v.RequiredChecksPassed {
@@ -369,8 +370,10 @@ func failingProtectedCheck(p readiness.Plan, result verification.Result) (readin
 // protectedRanChecks returns the observations of protected view v that ran on their own,
 // in plan order, and the plan's regression checks they are matched against. A targeted
 // check the view recorded as covered by a baseline check (a CoveredBy observation in
-// Checks, or a not-run record when that baseline failed) never ran, so it is left out of
-// both; each such record must name the covering check the plan itself gives it.
+// Checks, or a CoveredBy not-run record when that baseline failed or an earlier required
+// check stopped the view before it) never ran on its own, so it is left out of both,
+// even when plan order lists it before the check the view stopped on; each such record
+// must name the covering check the plan itself gives it.
 func protectedRanChecks(p readiness.Plan, v *verification.Result) ([]verification.Observation, []readiness.Check, error) {
 	planned := map[string]readiness.Check{}
 	for _, c := range p.Analysis.Checks {

lab/internal/molecule/accept_protected_failures_test.go

@@ -3,6 +3,7 @@ package molecule
 import (
 	"bytes"
 	"context"
+	"encoding/json"
 	"os"
 	"path/filepath"
 	"reflect"
@@ -123,6 +124,25 @@ func acceptBaselineFixture(t *testing.T, mode string) (Options, Record) {
 }
 
 func acceptFixtureFiles(t *testing.T, mode string, tests map[string]string, baseline bool, argv []string) (Options, Record) {
+	t.Helper()
+	return acceptFixtureWith(t, mode, tests, nil, func(a *readiness.Analysis, manifest string) {
+		a.Checks = append(a.Checks, acceptRegressionCheck(acceptCheck, manifest, "Module tests", argv...))
+		if baseline {
+			a.Scope = append(a.Scope, readiness.ScopedPath{Path: acceptOtherTest, Reason: "Another test of the package", Evidence: []string{"issue"}})
+			a.Checks = append(a.Checks, acceptRegressionCheck(acceptBaseline, manifest, "Formula baseline", "go", "test", "-timeout", "20m", "./..."))
+		}
+	})
+}
+
+// acceptRegressionCheck is a required regression check in lab defined by its manifest.
+func acceptRegressionCheck(id, manifest, reason string, argv ...string) readiness.Check {
+	return readiness.Check{ID: id, Category: "regression", Argv: append([]string{}, argv...), Dir: "lab", TimeoutMS: fixtureCheckTimeoutMS, Required: true, Definitions: []string{manifest}, Reason: reason, Evidence: []string{"issue", manifest}}
+}
+
+// acceptFixtureWith runs the accept fixture: prepare, when set, changes the source
+// repository or the approved Formula before the pinned test is committed, and extend
+// adds the regression checks (and any scope) to the analysis after the pinned test's.
+func acceptFixtureWith(t *testing.T, mode string, tests map[string]string, prepare func(o Options, repo string), extend func(a *readiness.Analysis, manifest string)) (Options, Record) {
 	t.Helper()
 	bin := resolvedTempDir(t)
 	if err := writeStub(filepath.Join(bin, "go"), []byte(acceptFakeGo), 0700); err != nil {
@@ -131,6 +151,9 @@ func acceptFixtureFiles(t *testing.T, mode string, tests map[string]string, base
 	setAcceptMode(t, bin, mode)
 	t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH"))
 	o, repo := fixture(t)
+	if prepare != nil {
+		prepare(o, repo)
+	}
 	files := map[string]string{"lab/go.mod": "module example.test/lab\n"}
 	for file, body := range tests {
 		files[file] = body
@@ -159,11 +182,7 @@ func acceptFixtureFiles(t *testing.T, mode string, tests map[string]string, base
 			}
 		}
 		a.Scope = append(a.Scope, readiness.ScopedPath{Path: acceptTest, Reason: "The pinned behaviour changes by design", Evidence: []string{"issue"}})
-		a.Checks = append(a.Checks, readiness.Check{ID: acceptCheck, Category: "regression", Argv: append([]string{}, argv...), Dir: "lab", TimeoutMS: fixtureCheckTimeoutMS, Required: true, Definitions: []string{manifest}, Reason: "Module tests", Evidence: []string{"issue", manifest}})
-		if baseline {
-			a.Scope = append(a.Scope, readiness.ScopedPath{Path: acceptOtherTest, Reason: "Another test of the package", Evidence: []string{"issue"}})
-			a.Checks = append(a.Checks, readiness.Check{ID: acceptBaseline, Category: "regression", Argv: []string{"go", "test", "-timeout", "20m", "./..."}, Dir: "lab", TimeoutMS: fixtureCheckTimeoutMS, Required: true, Definitions: []string{manifest}, Reason: "Formula baseline", Evidence: []string{"issue", manifest}})
-		}
+		extend(&a, manifest)
 		return a, err
 	}
 	r, err := runApprovedFixture(t, o)
@@ -501,6 +520,119 @@ func TestAcceptProtectedFailuresSkipsCoveredTargetedChecks(t *testing.T) {
 	}
 }
 
+// The #403 order: a full-suite check listed first in the plan is covered by the later
+// Formula baseline, so the protected view skips it and stops on the targeted check
+// after it, which fails by design. The covered check is recorded as not run with
+// CoveredBy, so --accept-protected-failures leaves it out of the plan-order prefix
+// and declares the targeted check's failure, which the approved checks-only run
+// accepts.
+func TestAcceptProtectedFailuresAcceptsACoveredFullSuiteListedFirst(t *testing.T) {
+	const full, targeted = "go-test-all", "pin-tests"
+	prepare := func(o Options, repo string) {
+		if err := os.MkdirAll(filepath.Join(repo, ".github", "workflows"), 0700); err != nil {
+			t.Fatal(err)
+		}
+		if err := os.WriteFile(filepath.Join(repo, filepath.FromSlash(baselineWorkflow)), []byte("name: CI\non: pull_request\njobs: {}\n"), 0600); err != nil {
+			t.Fatal(err)
+		}
+		raw, err := os.ReadFile(filepath.Join(o.LabDir, "formulas", "sample.yaml"))
+		if err != nil {
+			t.Fatal(err)
+		}
+		entry, _ := json.Marshal(map[string]any{"id": acceptBaseline, "argv": []string{"go", "test", "-timeout", "20m", "./..."}, "dir": "lab", "timeoutMs": fixtureCheckTimeoutMS, "workflow": baselineWorkflow, "reason": "CI runs the module tests."})
+		approveFixture(t, o.LabDir, string(raw)+"requiredChecks:\n  - "+string(entry)+"\n", "baseline")
+	}
+	o, r := acceptFixtureWith(t, "by-design", map[string]string{acceptTest: acceptPinnedBase}, prepare, func(a *readiness.Analysis, manifest string) {
+		a.Checks = append(a.Checks,
+			acceptRegressionCheck(full, manifest, "The full suite", "go", "test", "-timeout", "20m", "./..."),
+			acceptRegressionCheck(targeted, manifest, "The pinned package", "go", "test", "-json", "./pin"))
+	})
+	parent := latestPlan(r)
+	var regression []string
+	for _, c := range parent.Plan.Analysis.Checks {
+		if c.Category == "regression" {
+			regression = append(regression, c.ID)
+		}
+	}
+	if want := []string{full, targeted, acceptBaseline}; !reflect.DeepEqual(regression, want) || !reflect.DeepEqual(parent.Plan.FormulaBaseline, []string{acceptBaseline}) {
+		t.Fatalf("regression checks %v baseline %v, want %v", regression, parent.Plan.FormulaBaseline, want)
+	}
+	v := r.Verification
+	if statePhase(r) != "failed" || v.Protected == nil || v.Protected.RequiredChecksPassed || !strings.HasPrefix(v.Protected.Error, "required command "+targeted+" did not pass") {
+		t.Fatalf("fixture did not fail only in the protected view on %s: %s %+v", targeted, statePhase(r), v.Protected)
+	}
+	for _, obs := range v.Protected.Checks {
+		if obs.ID == full {
+			t.Fatalf("the covered full suite is among the protected checks: %+v", obs)
+		}
+	}
+	coveredBy := map[string]string{}
+	for _, obs := range v.Protected.NotRun {
+		if obs.Result.OK || obs.NotRun != verification.NotRunEarlierFailure {
+			t.Fatalf("protected not-run record %+v", obs)
+		}
+		coveredBy[obs.ID] = obs.CoveredBy
+	}
+	if want := map[string]string{acceptBaseline: "", full: acceptBaseline}; !reflect.DeepEqual(coveredBy, want) {
+		t.Fatalf("protected not-run records %v, want %v", coveredBy, want)
+	}
+	tree := r.Capture.Tree
+	r, err := AmendFromLatest(context.Background(), o.LabDir, r.ID, AmendRequest{AcceptProtectedFailures: true})
+	if err != nil {
+		t.Fatalf("the by-design failure after a covered full suite was refused: %v", err)
+	}
+	got := latestPlan(r)
+	if got.Plan.Continuation != "checks-only" || !reflect.DeepEqual(got.Plan.Analysis.Checks, parent.Plan.Analysis.Checks) {
+		t.Fatalf("the amendment changed the checks or continuation %q", got.Plan.Continuation)
+	}
+	deltas := got.Plan.ExpectedProtectedDeltas
+	if len(deltas) != 1 || deltas[0].CheckID != targeted || deltas[0].Package != acceptPackage || deltas[0].Test != "TestX" || deltas[0].ScopePath != acceptTest || deltas[0].DeltaScope != readiness.DeltaScopeIdentity {
+		t.Fatalf("deltas %+v", deltas)
+	}
+	o.Approve = got.Digest
+	if r, err = Run(context.Background(), o); err != nil {
+		t.Fatal(err)
+	}
+	v = r.Verification
+	if r.Readiness.Phase != "review-waiting" || v == nil || !v.RequiredChecksPassed || len(r.HarnessAttempts) != 1 || r.Capture.Tree != tree || len(v.AcceptedDeltas) == 0 {
+		t.Fatalf("checks-only did not accept the declared failure: phase %s %+v", r.Readiness.Phase, v)
+	}
+	for _, d := range v.AcceptedDeltas {
+		if d.Test != "TestX" || d.Package != acceptPackage {
+			t.Fatalf("accepted deltas %+v", v.AcceptedDeltas)
+		}
+	}
+}
+
+// TestAcceptProtectedFailuresLeavesOutACheckCoveredByANotRunBaseline: a covered check
+// listed before the check the protected view stopped on is left out of the plan-order
+// prefix only through a CoveredBy record the plan backs. Without one, as a record
+// written before CoveredBy was kept, the run is still refused.
+func TestAcceptProtectedFailuresLeavesOutACheckCoveredByANotRunBaseline(t *testing.T) {
+	t.Parallel()
+	tree, derived := strings.Repeat("a", 40), strings.Repeat("b", 40)
+	full := readiness.Check{ID: "go-test-all", Category: "regression", Argv: []string{"go", "test", "-timeout", "20m", "./..."}, Dir: "lab", Required: true}
+	targeted := readiness.Check{ID: acceptCheck, Category: "regression", Argv: []string{"go", "test", "-json", "./pin"}, Dir: "lab", Required: true}
+	baseline := readiness.Check{ID: acceptBaseline, Category: "regression", Argv: []string{"go", "test", "-timeout", "20m", "./..."}, Dir: "lab", Required: true}
+	p := readiness.Plan{FormulaBaseline: []string{baseline.ID}, Analysis: readiness.Analysis{Checks: []readiness.Check{full, targeted, baseline}}}
+	failing := verification.Observation{ID: targeted.ID, Category: "regression", SourceTree: derived, Command: targeted, Result: process.Result{Reconciled: true, Code: 1}}
+	notRun := func(c readiness.Check, coveredBy string) verification.Observation {
+		return verification.Observation{ID: c.ID, Category: "regression", SourceTree: derived, Command: c, CoveredBy: coveredBy, NotRun: verification.NotRunEarlierFailure, Result: process.Result{Reconciled: true, Code: -1}}
+	}
+	result := func(covered verification.Observation) verification.Result {
+		return verification.Result{SourceTree: tree, Protected: &verification.Result{SourceTree: derived, Setup: []verification.Observation{}, Checks: []verification.Observation{failing}, NotRun: []verification.Observation{notRun(baseline, ""), covered}, Error: "required command " + targeted.ID + " did not pass"}}
+	}
+	if c, _, err := failingProtectedCheck(p, result(notRun(full, baseline.ID))); err != nil || c.ID != targeted.ID {
+		t.Fatalf("a covered check left unrun by an earlier stop: %v %q", err, c.ID)
+	}
+	if _, _, err := failingProtectedCheck(p, result(notRun(full, ""))); err == nil || !strings.Contains(err.Error(), "observation 1 does not match the approved check") {
+		t.Fatalf("a not-run record without CoveredBy was left out of the prefix: %v", err)
+	}
+	if _, _, err := failingProtectedCheck(p, result(notRun(full, "go-vet"))); err == nil || !strings.Contains(err.Error(), "does not cover it with") {
+		t.Fatalf("coverage the plan does not give was accepted: %v", err)
+	}
+}
+
 func TestAcceptProtectedFailuresRequestValidation(t *testing.T) {
 	t.Parallel()
 	o, r := amendFixture(t)

lab/internal/verification/order.go

@@ -80,7 +80,10 @@ func formatOnlyScript(script string) bool {
 // keeping plan order within a tier. The protected view keeps plan order: it runs only
 // once the candidate view's cheap checks passed (or beside it, stopped by its failure),
 // and --accept-protected-failures reads its run as a plan-order prefix ending at the
-// check it stopped on.
+// check it stopped on. A targeted check a baseline covers is skipped in that order and
+// recorded with CoveredBy naming the baseline: as covered once the baseline ran, or as
+// not run when the baseline failed or an earlier required check stopped the view first.
+// The prefix leaves every such record out, wherever plan order lists the check.
 func executionOrder(commands []readiness.Check, setup int, protected bool) []int {
 	order := make([]int, len(commands))
 	for i := range order {

lab/internal/verification/order_test.go

@@ -359,6 +359,62 @@ func TestCheckCoveredByAFailingBaselineIsNotRun(t *testing.T) {
 	}
 }
 
+// TestCheckCoveredByANotRunBaselineKeepsCoveredBy: the #403 order. A full-suite check
+// the Formula baseline covers is listed first and a cheaper targeted check after it
+// fails in the protected view before the baseline runs. The covered check is listed in
+// NotRun as not run, with CoveredBy naming the baseline, never among the checks and
+// never as evidence. The candidate view runs every check, cheapest first, as before.
+func TestCheckCoveredByANotRunBaselineKeepsCoveredBy(t *testing.T) {
+	bin, dir, p, c := orderFixture(t, "by-design")
+	full := p.Analysis.Checks[1]
+	full.ID, full.Reason = "go-test-all", "The full suite"
+	pinned := p.Analysis.Checks[0]
+	pinned.ID, pinned.Argv = "pinned", []string{"go", "test", "-run", "TestA", "./pkg/..."}
+	p.Analysis.Checks = []readiness.Check{full, pinned, p.Analysis.Checks[1]}
+	if err := p.Validate(); err != nil {
+		t.Fatal(err)
+	}
+	if id, ok := CoveringCheckID(p, full); !ok || id != baselineCheck {
+		t.Fatalf("the full suite is not covered by the baseline: %q %v", id, ok)
+	}
+	v := runDelta(t, dir, p, c)
+	if got := ids(v.Checks); !reflect.DeepEqual(got, []string{full.ID, pinned.ID, baselineCheck}) || len(v.NotRun) != 0 || v.CandidateError != "" {
+		t.Fatalf("candidate checks %v not run %v error %q", got, ids(v.NotRun), v.CandidateError)
+	}
+	for _, o := range v.Checks {
+		if !o.Result.OK || o.CoveredBy != "" {
+			t.Fatalf("candidate record %+v", o)
+		}
+	}
+	if v.RequiredChecksPassed || v.Protected == nil || !strings.HasPrefix(v.Protected.Error, "required command pinned did not pass") {
+		t.Fatalf("protected verdict %q", v.Error)
+	}
+	if got := ids(v.Protected.Checks); !reflect.DeepEqual(got, []string{pinned.ID}) {
+		t.Fatalf("protected checks %v", got)
+	}
+	if got := ids(v.Protected.NotRun); !reflect.DeepEqual(got, []string{baselineCheck, full.ID}) {
+		t.Fatalf("protected not run %v", got)
+	}
+	baseline, covered := v.Protected.NotRun[0], v.Protected.NotRun[1]
+	if baseline.NotRun != NotRunEarlierFailure || baseline.CoveredBy != "" || baseline.Result.OK {
+		t.Fatalf("baseline record %+v", baseline)
+	}
+	if covered.NotRun != NotRunEarlierFailure || covered.CoveredBy != baselineCheck || covered.Result.OK || covered.Result.Stdout != "" || !reflect.DeepEqual(covered.Command, full) {
+		t.Fatalf("covered record %+v", covered)
+	}
+	if !validCoverage(p, full, covered) {
+		t.Fatal("the covered record does not name the plan's covering baseline")
+	}
+	if MatchesCheckObservation(p, full, covered, v.Protected.SourceTree, true) || MatchesCheckObservation(p, full, covered, v.Protected.SourceTree, false) {
+		t.Fatal("a covered check left unrun by an earlier stop matched")
+	}
+	// The candidate view ran all three checks cheapest first; the protected view ran the
+	// targeted check alone.
+	if got := argvLog(t, bin); len(got) != 4 || got[0] != "test -run TestA ./pkg/..." || got[1] != "test -timeout 20m ./..." || got[2] != "test -timeout 20m ./..." || strings.Contains(got[3], "./...") {
+		t.Fatalf("argv %q", got)
+	}
+}
+
 // TestCoverageNeverCrossesANestedModule: go test ./... skips a nested module, so a
 // targeted package in one, or below a directory holding one, is never covered by the
 // baseline; a package in the module itself still is.

lab/internal/verification/run.go

@@ -797,15 +797,20 @@ func runViewDeltas(ctx context.Context, source, destination string, p readiness.
 			checkIndex = append(checkIndex, i)
 		}
 	}
-	// notRun records every command in rest, and the checks those cover, as never run.
+	// notRun records every command in rest, and the checks those cover, as never run. A
+	// covered check keeps CoveredBy naming its baseline, wherever it sits in plan order,
+	// so --accept-protected-failures leaves it out of the plan-order prefix as it does
+	// when the baseline itself failed; a not-run record is never delivery evidence.
 	notRun := func(rest []int, reason string) {
 		for _, j := range rest {
 			if covered[j] {
 				continue
 			}
 			v.NotRun = append(v.NotRun, notRunObservation(commands[j], categoryOf(j), c.Tree, reason))
 			for _, k := range covers[j] {
-				v.NotRun = append(v.NotRun, notRunObservation(commands[k], categoryOf(k), c.Tree, reason))
+				o := notRunObservation(commands[k], categoryOf(k), c.Tree, reason)
+				o.CoveredBy = commands[j].ID
+				v.NotRun = append(v.NotRun, o)
 			}
 		}
 	}