From 7279b96ab4d432b950702b401c4218360d290f5d Mon Sep 17 00:00:00 2001 From: REPPL <77722411+REPPL@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:18:28 +0000 Subject: [PATCH 1/4] fix: let a records-only change preflight without the unit and race stages A change confined to the record tiers waited behind the load gate for the full unit and race stages and could time out before it was pushed. make preflight now takes a shorter rule for a committed change whose every path since the merge base with origin's default branch is under .abcd/development or .abcd/work and named by no test. It runs the same gates, build and vet, and precheck's tests in place of the unit and race stages, and says so. The receipt names its scope, verdict and merge base, and a records-only receipt is refused over a code change. Anything the classifier cannot answer takes the full run; CI's unit lane still runs every test. Resolves: iss-2610251555094324 Assisted-by: Claude:claude-opus-5-5 --- ...make-preflight-cannot-land-a-one-record.md | 2 + AGENTS.md | 9 +- Makefile | 37 ++- internal/core/lint/precheck_test.go | 4 +- internal/core/lint/recordsonly_test.go | 300 ++++++++++++++++++ internal/surface/cli/githook_prepush_test.go | 117 ++++++- scripts/preflight-receipt.sh | 48 ++- scripts/records-only.sh | 187 +++++++++++ 8 files changed, 689 insertions(+), 15 deletions(-) rename .abcd/work/issues/{open => resolved}/iss-2610251555094324-make-preflight-cannot-land-a-one-record.md (78%) create mode 100644 internal/core/lint/recordsonly_test.go create mode 100755 scripts/records-only.sh diff --git a/.abcd/work/issues/open/iss-2610251555094324-make-preflight-cannot-land-a-one-record.md b/.abcd/work/issues/resolved/iss-2610251555094324-make-preflight-cannot-land-a-one-record.md similarity index 78% rename from .abcd/work/issues/open/iss-2610251555094324-make-preflight-cannot-land-a-one-record.md rename to .abcd/work/issues/resolved/iss-2610251555094324-make-preflight-cannot-land-a-one-record.md index 832a171532..f89b000b34 100644 --- a/.abcd/work/issues/open/iss-2610251555094324-make-preflight-cannot-land-a-one-record.md +++ b/.abcd/work/issues/resolved/iss-2610251555094324-make-preflight-cannot-land-a-one-record.md @@ -11,6 +11,8 @@ production_mode: hand-written found_at: "Makefile" remedy: "Let preflight mint a receipt without the test and race stages for a diff that touches only the record tiers (.abcd/work/, .abcd/development/), using the same fail-closed path classifier CI uses to stand the race lane down on a pull request confined to those paths, so the receipt's scope matches what CI would run." refines: [iss-2610161639025702] +resolution: "make preflight runs precheck's tests in place of the unit and race stages for a committed change confined to the record tiers that no test names, judged fail-closed by scripts/records-only.sh against the merge base with origin's default branch; the run says so and its receipt names the scope, the verdict and the merge base" +impact: internal --- make preflight cannot land a one-record change on a busy machine: on 2026-10-24 a branch whose whole diff is one new issue record (iss-2610241228359748) ran preflight twice, every gate before the test stages passed both times, and both runs then waited the load gate's full two hours (abcd implement load --wait --timeout 2h) behind two to four running preflights and six to eight queued waiters, and failed with 'load gate still shut after 2h0m0s'. With no receipt the pre-push hook refuses the push, so a change that touches no Go code waits behind the full test and race stages of every other session and in practice cannot be pushed. diff --git a/AGENTS.md b/AGENTS.md index 255242edaf..04e23f9dd5 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -154,7 +154,11 @@ make preflight # the pre-push gate: the load check first (load-check, # site-render + # smoke + evals-cold-reading, # then build + vet + - # test + race (internal), all on the go.mod toolchain + # test + race (internal), all on the go.mod toolchain; + # a committed change confined to .abcd/development/ and + # .abcd/work/ that no test names runs precheck's tests + # (those that read the record tree among them) in place + # of test + race, says so, and its receipt says so make precheck # minutes, no queue: preflight's gates, build, vet, the # record, page and agent audit packages, and the command # and site audit tests chosen by name; no unit or race @@ -352,7 +356,8 @@ with `*CONCURRENCY` to force it). `check-attribution`, `site-render`), both tagged eval lanes (`smoke`, `evals-cold-reading`), plus `go build ./...`, `go vet ./...`, `go test -timeout 40m ./...`, and - `go test -race -timeout 40m ./internal/...`, under `ABCD_TEST_SHARDS=3`, + `go test -race -timeout 40m ./internal/...` (for a records-only change, + precheck's tests in their place, as the build section says), under `ABCD_TEST_SHARDS=3`, which runs the three longest packages as three test processes each (`internal/testshard`). The load check runs first (`load-check`, a warning, never a failure) and is not a gate: diff --git a/Makefile b/Makefile index 59bb520f2e..8f8493b982 100644 --- a/Makefile +++ b/Makefile @@ -263,6 +263,27 @@ asking-sync: asking-sync-check: @go run ./cmd/asking-sync -check +# A records-only change (iss-2610251555094324) takes a shorter preflight: when +# every path HEAD changes against the merge base with origin's default branch +# lies in .abcd/development/ or .abcd/work/, no test names one of them (by its +# path, its file name or its record id), and the +# tree matches HEAD, scripts/records-only.sh says so and the second preflight +# rule below runs in place of the first. It runs the same gates, build and vet, +# then precheck's test lines in place of the unit and race stages, and says so +# before its tests and in the receipt it mints. Anything else, a failed check included, +# is the full rule. The verdict is read while this Makefile is parsed, as the +# starting state is, and only yes or nothing reaches make: the reason can name a +# changed path, so it is printed by the script itself, never placed in a recipe +# line. The records rule's first step and its mint each run the classifier +# again, so a verdict that no longer holds fails the run rather than skipping +# its tests. `make preflight PREFLIGHT_RECORDS_ONLY=` takes the full rule +# whatever the change. TestPreflightRecordsRuleRunsPrechecksTests holds the +# second rule's gates to the first's and its test lines to precheck's. +ifneq ($(filter preflight,$(MAKECMDGOALS)),) +PREFLIGHT_RECORDS_ONLY := $(shell scripts/records-only.sh >/dev/null 2>&1 && echo yes) +endif + +ifneq ($(PREFLIGHT_RECORDS_ONLY),yes) # Pre-push gate (run before a push, never by it: .githooks/pre-push checks the # receipt the last step mints, below): the load check first (a # warning, never a failure: load-check), then the format gate (fmt-check), the @@ -343,11 +364,25 @@ preflight: load-check fmt-check lint-reviews lint-issues record-lint issue-drift go test -race -p "$$p" -timeout 40m ./internal/surface/cli ./internal/core/reading ./internal/core/launch ./internal/core/lifeboat ./internal/core/lint ./internal/adapter/scanner ./internal/core/implement/loop ./internal/core/interview ./internal/core/capture ./internal/core/ahoy ./internal/core/site ./internal/... @scripts/preflight-receipt.sh mint "$(PREFLIGHT_BEGAN)" +else +preflight: load-check fmt-check lint-reviews lint-issues record-lint issue-drift docs-lint check-attribution site-render smoke evals-cold-reading + @scripts/records-only.sh + @echo "preflight: a records-only change, so the unit and race stages are skipped and precheck's tests run in their place; CI's unit lane still runs every test" + go build ./... + go vet ./... + go test -timeout 20m ./internal/core/lint ./internal/core/launch/scaffold ./internal/core/mdrecord ./internal/core/issueschema ./internal/core/guard ./internal/core/glossary ./internal/core/reading ./internal/core/recordid ./internal/core/changelog ./internal/core/intent ./internal/core/release ./internal/core/releasedigest ./internal/core/decisions ./internal/core/record/match ./internal/core/lifeboat + go test -timeout 20m -run '$(PRECHECK_AUDIT_TESTS)' ./internal/surface/cli ./internal/core/site + @scripts/preflight-receipt.sh mint "$(PREFLIGHT_BEGAN)" records-only +endif + # The precheck (iss-2610230016523625): the minutes-long pass a lane runs before # it queues a preflight, so a test that reads the whole tree fails before the # queue rather than after it. It runs preflight's gates (all but the load check, # which reads the machine and only warns), build and vet, the packages whose # tests audit the record, the generated pages and the agent definitions, in full, +# and the packages whose tests read the committed record tree whole (record ids +# and slugs, the changelog derivation, intents, release pages, decisions, the +# brief's lifeboat table), # and the command and site packages' audit tests, chosen by name: a test in those # two packages that reads the committed tree carries one of the PRECHECK_AUDIT_TESTS # words in its name, which is how a new one joins without a list to keep. It skips @@ -360,7 +395,7 @@ PRECHECK_AUDIT_TESTS := Surface|Page|Brief|Parity|Reference|Snapshot|Appendix|Ap precheck: fmt-check lint-reviews lint-issues record-lint issue-drift docs-lint check-attribution site-render smoke evals-cold-reading go build ./... go vet ./... - go test -timeout 20m ./internal/core/lint ./internal/core/launch/scaffold ./internal/core/mdrecord ./internal/core/issueschema ./internal/core/guard ./internal/core/glossary ./internal/core/reading + go test -timeout 20m ./internal/core/lint ./internal/core/launch/scaffold ./internal/core/mdrecord ./internal/core/issueschema ./internal/core/guard ./internal/core/glossary ./internal/core/reading ./internal/core/recordid ./internal/core/changelog ./internal/core/intent ./internal/core/release ./internal/core/releasedigest ./internal/core/decisions ./internal/core/record/match ./internal/core/lifeboat go test -timeout 20m -run '$(PRECHECK_AUDIT_TESTS)' ./internal/surface/cli ./internal/core/site precheck: export GOTOOLCHAIN := go$(GO_TOOLCHAIN_VERSION) diff --git a/internal/core/lint/precheck_test.go b/internal/core/lint/precheck_test.go index 3c3cecf9c9..a9d3d2190c 100644 --- a/internal/core/lint/precheck_test.go +++ b/internal/core/lint/precheck_test.go @@ -95,7 +95,9 @@ func TestPrecheckRunsTheAuditTests(t *testing.T) { "go build ./...", "go vet ./...", "go test -timeout 20m ./internal/core/lint ./internal/core/launch/scaffold ./internal/core/mdrecord " + - "./internal/core/issueschema ./internal/core/guard ./internal/core/glossary ./internal/core/reading", + "./internal/core/issueschema ./internal/core/guard ./internal/core/glossary ./internal/core/reading " + + "./internal/core/recordid ./internal/core/changelog ./internal/core/intent ./internal/core/release " + + "./internal/core/releasedigest ./internal/core/decisions ./internal/core/record/match ./internal/core/lifeboat", "go test -timeout 20m -run '$(PRECHECK_AUDIT_TESTS)' ./internal/surface/cli ./internal/core/site", } { if !strings.Contains(recipe, want) { diff --git a/internal/core/lint/recordsonly_test.go b/internal/core/lint/recordsonly_test.go new file mode 100644 index 0000000000..43bd045332 --- /dev/null +++ b/internal/core/lint/recordsonly_test.go @@ -0,0 +1,300 @@ +package lint_test + +import ( + "errors" + "os/exec" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/intentdriven/abcd/internal/gittest" +) + +// recordsOnly runs scripts/records-only.sh in r and returns its line and +// whether it classified HEAD records-only (exit 0) or the full run (exit 1). +func recordsOnly(t *testing.T, r *gittest.Repo) (string, bool) { + t.Helper() + script, err := filepath.Abs(filepath.Join("..", "..", "..", "scripts", "records-only.sh")) + if err != nil { + t.Fatal(err) + } + cmd := exec.Command("bash", script) + cmd.Dir, cmd.Env = r.Root(), r.Env() + out, err := cmd.CombinedOutput() + var exit *exec.ExitError + switch { + case err == nil: + return string(out), true + case errors.As(err, &exit) && exit.ExitCode() == 1: + return string(out), false + } + t.Fatalf("records-only.sh: %v\n%s", err, out) + return "", false +} + +// recordsRepo is a repository whose origin/main is its first commit, holding +// a code file, a test that names one record by its path, and that record. +func recordsRepo(t *testing.T) *gittest.Repo { + t.Helper() + r := gittest.NewRepo(t) + r.Write("internal/x/x.go", "package x\n") + r.Write("internal/x/x_test.go", "package x\n\nconst fixture = \".abcd/work/fixture.md\"\n") + r.Write(".abcd/work/fixture.md", "a record a test reads\n") + r.Write(".abcd/work/issues/open/iss-1.md", "one\n") + r.Commit("base") + r.Git("update-ref", "refs/remotes/origin/main", "HEAD") + return r +} + +// TestRecordsOnlyClassifiesARecordTierChange (iss-2610251555094324): a change +// whose every path lies in .abcd/development/ or .abcd/work/, named by no +// test, is records-only, and the line says why. +func TestRecordsOnlyClassifiesARecordTierChange(t *testing.T) { + r := recordsRepo(t) + r.Write(".abcd/work/issues/open/iss-2.md", "two\n") + r.Write(".abcd/development/intents/drafts/itd-1.md", "an intent\n") + r.Write(".abcd/work/issues/open/iss-1.md", "one, edited\n") + r.Commit("records") + base := r.Git("rev-parse", "origin/main") + out, ok := recordsOnly(t, r) + if want := "records-only: all 3 changed paths since the merge base with origin/main, " + base + ", are in the record tiers, and no test names one\n"; !ok || out != want { + t.Fatalf("a records change = %v, %q; want records-only, %q", ok, out, want) + } + // A deleted record is a changed path in the tiers too. + r.Remove(".abcd/work/issues/open/iss-2.md") + r.Commit("drop one") + if out, ok := recordsOnly(t, r); !ok { + t.Fatalf("a records change with a delete = full run, %q; want records-only", out) + } +} + +// TestRecordsOnlyRunsEverythingElseInFull: the classifier is fail-closed. A +// code change mixed into a records change, a path outside the two tiers (the +// docs and the conventions file included, which CI's own classifier treats +// differently), a record a test names, a file moved out of code into a tier, +// an empty diff, and a checkout with no origin default branch each take the +// full run, and the line names why. +func TestRecordsOnlyRunsEverythingElseInFull(t *testing.T) { + for _, c := range []struct { + name string + edit func(r *gittest.Repo) + want string + }{ + {"code mixed in", func(r *gittest.Repo) { + r.Write(".abcd/work/issues/open/iss-2.md", "two\n") + r.Write("internal/x/x.go", "package x\n\nvar Y = 1\n") + }, "full run: internal/x/x.go is outside the record tiers"}, + {"docs", func(r *gittest.Repo) { r.Write("docs/guide.md", "a page\n") }, "full run: docs/guide.md is outside the record tiers"}, + {"conventions file", func(r *gittest.Repo) { r.Write("AGENTS.md", "rules\n") }, "full run: AGENTS.md is outside the record tiers"}, + {"another .abcd tier", func(r *gittest.Repo) { r.Write(".abcd/config.json", "{}\n") }, "full run: .abcd/config.json is outside the record tiers"}, + {"a record a test names", func(r *gittest.Repo) { r.Write(".abcd/work/fixture.md", "edited\n") }, + "full run: .abcd/work/fixture.md is named by a test (its file name fixture.md), so it is that test's fixture"}, + {"a record a test names by its file name", func(r *gittest.Repo) { + r.Write("internal/x/y_test.go", "package x\n\nvar parts = []string{\".abcd\", \"work\", \"personas.json\"}\n") + r.Commit("a test that builds the path") + r.Git("update-ref", "refs/remotes/origin/main", "HEAD") + r.Write(".abcd/work/personas.json", "[]\n") + }, "full run: .abcd/work/personas.json is named by a test (its file name personas.json), so it is that test's fixture"}, + {"a record a test finds by its id", func(r *gittest.Repo) { + r.Write("internal/x/z_test.go", "package x\n\nconst glob = \"shipped/itd-7-*.md\"\n") + r.Commit("a test that globs for a record by its id") + r.Git("update-ref", "refs/remotes/origin/main", "HEAD") + r.Write(".abcd/development/intents/shipped/itd-7-a-real-intent.md", "an intent\n") + }, "full run: .abcd/development/intents/shipped/itd-7-a-real-intent.md is named by a test (its record id itd-7), so it is that test's fixture"}, + {"a record a test finds by its bare number", func(r *gittest.Repo) { + r.Write("internal/x/n_test.go", "package x\n\nconst id = \"adr-261015072933\"\n") + r.Commit("a test that globs for an ADR by its number") + r.Git("update-ref", "refs/remotes/origin/main", "HEAD") + r.Write(".abcd/development/decisions/adrs/261015072933-a-real-decision.md", "a decision\n") + }, "full run: .abcd/development/decisions/adrs/261015072933-a-real-decision.md is named by a test (its record id 261015072933), so it is that test's fixture"}, + {"a local branch named like origin's", func(r *gittest.Repo) { + r.Write("internal/x/x.go", "package x\n\nvar Y = 1\n") + r.Commit("unmerged code") + r.Git("branch", "origin/main") + r.Write(".abcd/work/issues/open/iss-2.md", "two\n") + }, "full run: internal/x/x.go is outside the record tiers"}, + {"moved out of code", func(r *gittest.Repo) { r.Git("mv", "internal/x/x.go", ".abcd/work/x.go") }, + "full run: internal/x/x.go is outside the record tiers"}, + {"empty", func(r *gittest.Repo) { r.Git("commit", "-q", "--allow-empty", "-m", "nothing") }, + "full run: the diff against the merge base with origin/main is empty"}, + {"no origin", func(r *gittest.Repo) { + r.Write(".abcd/work/issues/open/iss-2.md", "two\n") + r.Git("update-ref", "-d", "refs/remotes/origin/main") + }, "full run: there is no origin/main to diff against"}, + } { + t.Run(c.name, func(t *testing.T) { + r := recordsRepo(t) + c.edit(r) + if r.Git("status", "--porcelain") != "" { + r.Commit(c.name) + } + out, ok := recordsOnly(t, r) + if ok || !strings.HasPrefix(out, c.want) { + t.Fatalf("records-only = %v, %q; want the full run, %q", ok, out, c.want) + } + }) + } +} + +// TestRecordsOnlyDiffsAgainstOriginsDefaultBranch: the merge base is taken +// with origin/HEAD when it names the default branch, so a repository whose +// default branch is not main is judged against it. +func TestRecordsOnlyDiffsAgainstOriginsDefaultBranch(t *testing.T) { + r := recordsRepo(t) + r.Git("update-ref", "refs/remotes/origin/trunk", "HEAD") + r.Git("update-ref", "-d", "refs/remotes/origin/main") + r.Git("symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/trunk") + r.Write(".abcd/work/issues/open/iss-2.md", "two\n") + r.Commit("records") + if out, ok := recordsOnly(t, r); !ok || !strings.Contains(out, "merge base with origin/trunk") { + t.Fatalf("against origin/trunk = %v, %q; want records-only against it", ok, out) + } +} + +// TestRecordsOnlyKeepsControlBytesOffTheTerminal: the line can name a +// changed path, which a writer chose, so a control byte in it never reaches +// the terminal. +func TestRecordsOnlyKeepsControlBytesOffTheTerminal(t *testing.T) { + for _, c := range []struct{ name, file, want string }{ + {"a C0 escape", "x\x1b[2Jy.go", "x[2Jy.go is outside"}, + {"a C1 control sequence introducer", "x\u009b2Jy.go", "x2Jy.go is outside"}, + {"a bidirectional override", "x\u202egp.go", "xgp.go is outside"}, + } { + t.Run(c.name, func(t *testing.T) { + r := recordsRepo(t) + r.Write(c.file, "package x\n") + r.Commit("hostile") + out, ok := recordsOnly(t, r) + for _, b := range []byte(strings.TrimSuffix(out, "\n")) { + if b < 0x20 || b > 0x7e { + t.Fatalf("a hostile path printed byte %#x: %q", b, out) + } + } + if ok || !strings.Contains(out, c.want) { + t.Fatalf("a hostile path = %v, %q; want the full run naming %q", ok, out, c.want) + } + }) + } +} + +// TestRecordsOnlyLetsACitationInACommentPass: a test that cites a record's id +// in prose (itd-8, or itd-8)) does not make that record its fixture; only the +// id followed by a dash, the start of its file name, does. +func TestRecordsOnlyLetsACitationInACommentPass(t *testing.T) { + r := recordsRepo(t) + r.Write("internal/x/c_test.go", "package x\n\n// The rule itd-8 states (see itd-8).\n") + r.Commit("a test that cites an intent") + r.Git("update-ref", "refs/remotes/origin/main", "HEAD") + r.Write(".abcd/development/intents/drafts/itd-8-a-cited-intent.md", "an intent\n") + r.Commit("the intent") + if out, ok := recordsOnly(t, r); !ok { + t.Fatalf("a record a test only cites = full run, %q; want records-only", out) + } +} + +// TestRecordsOnlyReadsANewlineInAPathAsOnePath: the changed paths are read +// NUL-delimited, so a path holding a newline is one path, judged whole: a code +// file whose name holds a newline is not split into names that could each look +// like a record. A record whose name holds one cannot be searched for whole +// (git grep splits a pattern at a newline), so it takes the full run too. +func TestRecordsOnlyReadsANewlineInAPathAsOnePath(t *testing.T) { + r := recordsRepo(t) + r.Write("x\n.abcd/work/d.md", "package x\n") + r.Commit("a code file named with a newline") + if out, ok := recordsOnly(t, r); ok || !strings.HasPrefix(out, "full run: x.abcd/work/d.md is outside") { + t.Fatalf("a code file named with a newline = %v, %q; want the full run naming it whole", ok, out) + } + r = recordsRepo(t) + r.Write(".abcd/work/a\nb.md", "a record\n") + r.Commit("a record with a newline in its name") + if out, ok := recordsOnly(t, r); ok || !strings.HasPrefix(out, "full run: .abcd/work/ab.md holds a newline in its name") { + t.Fatalf("a record named with a newline = %v, %q; want the full run, saying why", ok, out) + } +} + +// TestRecordsOnlyRunsAnUncommittedEditInFull: the change is judged by its +// commits, so a tree that differs from HEAD (an edited code file, an untracked +// one, an edit an index flag hides) takes the full run, or the edit's tests +// would be skipped. +func TestRecordsOnlyRunsAnUncommittedEditInFull(t *testing.T) { + for _, c := range []struct { + name string + edit func(r *gittest.Repo) + }{ + {"an edited code file", func(r *gittest.Repo) { r.Write("internal/x/x.go", "package x\n\nvar Y = 1\n") }}, + {"an untracked code file", func(r *gittest.Repo) { r.Write("internal/x/z.go", "package x\n") }}, + {"a hidden edit", func(r *gittest.Repo) { + r.Git("update-index", "--skip-worktree", "internal/x/x.go") + r.Write("internal/x/x.go", "package x\n\nvar Y = 1\n") + }}, + {"an assume-unchanged edit", func(r *gittest.Repo) { + r.Git("update-index", "--assume-unchanged", "internal/x/x.go") + r.Write("internal/x/x.go", "package x\n\nvar Y = 1\n") + }}, + } { + t.Run(c.name, func(t *testing.T) { + r := recordsRepo(t) + r.Write(".abcd/work/issues/open/iss-2.md", "two\n") + r.Commit("records") + c.edit(r) + want := "full run: the working tree differs from HEAD" + if out, ok := recordsOnly(t, r); ok || !strings.HasPrefix(out, want) { + t.Fatalf("records-only = %v, %q; want the full run, %q", ok, out, want) + } + }) + } +} + +// TestPreflightRecordsRuleRunsPrechecksTests: the Makefile carries two +// preflight rules, the full one first, so every reader of the first rule (the +// load gates, the race lane, the receipt) reads the full run. The second, taken +// for a records-only change, runs the same gates, checks the verdict again +// before anything else, runs precheck's build, vet and test lines and nothing +// else, and mints a receipt that says it is records-only. +func TestPreflightRecordsRuleRunsPrechecksTests(t *testing.T) { + makefile := readRepoFile(t, filepath.Join("..", "..", ".."), "Makefile") + lines := strings.Split(makefile, "\n") + var rules []int + for i, l := range lines { + if strings.HasPrefix(l, "preflight:") && !strings.HasPrefix(l, "preflight: export ") { + rules = append(rules, i) + } + } + if len(rules) != 2 { + t.Fatalf("the Makefile declares %d preflight rules, want 2 (the full run, then the records-only run)", len(rules)) + } + if lines[rules[0]] != lines[rules[1]] { + t.Errorf("the records-only rule's gates differ from the full rule's:\n%s\n%s", lines[rules[1]], lines[rules[0]]) + } + var body []string + for _, l := range lines[rules[1]+1:] { + if !strings.HasPrefix(l, "\t") { + break + } + body = append(body, strings.TrimPrefix(l, "\t")) + } + if !slices.Contains(lines[rules[0]:rules[1]], "else") || !slices.Contains(lines[rules[1]:], "endif") { + t.Fatal("the two preflight rules are not the two branches of one conditional") + } + precheck, ok := makeRecipe(makefile, "precheck") + if !ok { + t.Fatal("Makefile declares no `precheck:` recipe") + } + var want []string + for _, l := range strings.Split(precheck, "\n") { + want = append(want, strings.TrimPrefix(l, "\t")) + } + if len(body) != len(want)+3 { + t.Fatalf("the records-only recipe runs %q; want the verdict checked, the skip announced, precheck's %q, then the mint", body, want) + } + if body[0] != "@scripts/records-only.sh" || !strings.HasPrefix(body[1], `@echo "preflight: a records-only change, so the unit and race stages are skipped`) { + t.Errorf("the records-only recipe does not check the verdict and say what it skips before it runs anything: %q", body[:2]) + } + if got := body[2 : len(body)-1]; !slices.Equal(got, want) { + t.Errorf("the records-only recipe runs %q; want precheck's lines, %q", got, want) + } + if last := body[len(body)-1]; last != `@scripts/preflight-receipt.sh mint "$(PREFLIGHT_BEGAN)" records-only` { + t.Errorf("the records-only recipe ends %q; want the records-only receipt minted", last) + } +} diff --git a/internal/surface/cli/githook_prepush_test.go b/internal/surface/cli/githook_prepush_test.go index 8e7f4221b0..22dcb5ee91 100644 --- a/internal/surface/cli/githook_prepush_test.go +++ b/internal/surface/cli/githook_prepush_test.go @@ -7,6 +7,7 @@ import ( "path/filepath" "regexp" "runtime" + "slices" "strings" "testing" @@ -67,7 +68,7 @@ func newPrePushCaseIn(t *testing.T, format string) *prePushCase { c.git("config", "user.name", "Alice Example") c.git("config", "user.email", "alice@example.com") - for _, rel := range []string{".githooks/pre-push", "scripts/preflight-receipt.sh"} { + for _, rel := range []string{".githooks/pre-push", "scripts/preflight-receipt.sh", "scripts/records-only.sh"} { src, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(rel))) if err != nil { t.Fatalf("read the committed %s: %v", rel, err) @@ -156,13 +157,13 @@ func (c *prePushCase) receipt(dir string, args ...string) (string, error) { // preflight stands in for `make preflight`: the state before, the gates, the mint // after. The Makefile wiring that does the same is pinned separately below. -func (c *prePushCase) preflight(dir string) string { +func (c *prePushCase) preflight(dir string, scope ...string) string { c.t.Helper() began, err := c.receipt(dir, "state") if err != nil { c.t.Fatalf("state: %v\n%s", err, began) } - out, err := c.receipt(dir, "mint", strings.TrimSpace(began)) + out, err := c.receipt(dir, append([]string{"mint", strings.TrimSpace(began)}, scope...)...) if err != nil { c.t.Fatalf("mint: %v\n%s", err, out) } @@ -497,7 +498,9 @@ func TestMakePreflightMintsTheReceiptAfterItsLastGate(t *testing.T) { if err != nil { t.Skip("not in a git checkout") } - cmd := exec.Command("make", "-n", "preflight") + // The full rule whatever this checkout's change is: a records-only one would + // read the second rule, which the lint package pins. + cmd := exec.Command("make", "-n", "preflight", "PREFLIGHT_RECORDS_ONLY=") cmd.Dir = strings.TrimSpace(string(out)) cmd.Env = gittest.Env(t) dry, err := cmd.CombinedOutput() @@ -523,3 +526,109 @@ func TestMakePreflightMintsTheReceiptAfterItsLastGate(t *testing.T) { t.Errorf("the receipt is not minted after the race-enabled tests\n%s", dry) } } + +// receiptLines is the receipt the clone holds for a commit, line by line. +func (c *prePushCase) receiptLines(commit string) []string { + c.t.Helper() + b, err := os.ReadFile(filepath.Join(c.dir, ".abcd", ".work.local", "preflight-receipts", commit)) + if err != nil { + c.t.Fatalf("read the receipt for %s: %v", commit, err) + } + return strings.Split(strings.TrimSuffix(string(b), "\n"), "\n") +} + +// TestPreflightReceiptNamesItsScope (iss-2610251555094324): a receipt says +// whether the unit and race stages ran. A records-only one carries the +// classifier's verdict and the merge base it was taken at, so a later reader +// can redo the check, and it lets the push through like a full one. +func TestPreflightReceiptNamesItsScope(t *testing.T) { + t.Run("full", func(t *testing.T) { + c := newPrePushCase(t) + head := c.commit("feature.md", "a feature\n") + c.preflight(c.dir) + if got := c.receiptLines(head); len(got) != 3 || got[2] != "scope full" { + t.Fatalf("a full preflight's receipt is %q; want it to name the full scope", got) + } + }) + t.Run("records-only", func(t *testing.T) { + c := newPrePushCase(t) + base := c.git("rev-parse", "origin/main") + head := c.commit(".abcd/work/issues/open/iss-1.md", "one\n") + out := c.preflight(c.dir, "records-only") + if !strings.Contains(out, "receipt minted for "+head[:12]+", records-only") { + t.Fatalf("a records-only preflight does not say its receipt is records-only\n%s", out) + } + want := []string{ + "scope records-only", + "verdict records-only: all 1 changed paths since the merge base with origin/main, " + base + + ", are in the record tiers, and no test names one", + "base " + base, + } + if got := c.receiptLines(head); len(got) != 5 || !slices.Equal(got[2:], want) { + t.Fatalf("the records-only receipt is %q; want it to end %q", got, want) + } + if out, err := c.push("origin", "feature"); err != nil { + t.Fatalf("a commit with a records-only receipt was refused\n%s", out) + } + }) +} + +// TestPreflightReceiptRefusesARecordsOnlyRunOverACodeChange: a records-only +// receipt is minted only when the classifier, run again at the mint, still +// reads the change as records-only, so a code change mixed into a records +// change gets no receipt for a run that skipped its tests. +func TestPreflightReceiptRefusesARecordsOnlyRunOverACodeChange(t *testing.T) { + c := newPrePushCase(t) + c.commit(".abcd/work/issues/open/iss-1.md", "one\n") + head := c.commit("feature.go", "package feature\n") + out := c.preflight(c.dir, "records-only") + if strings.Contains(out, "receipt minted for") || !strings.Contains(out, "no longer reads as records-only (full run: feature.go is outside") { + t.Fatalf("a records-only mint over a code change = %q; want it refused, naming the code file", out) + } + if _, err := os.Stat(filepath.Join(c.dir, ".abcd", ".work.local", "preflight-receipts", head)); err == nil { + t.Fatal("a records-only run over a code change left a receipt") + } + if pushed, err := c.push("origin", "feature"); err == nil { + t.Fatalf("a code change whose preflight skipped its tests was pushed\n%s", pushed) + } +} + +// TestMakePreflightTakesTheRecordsRuleOnlyForARecordsChange runs the +// committed Makefile, dry, in a clone whose change is records alone, then the +// same change with a code file mixed in: the first takes the records-only rule +// (no race stage, a records-only receipt) and the second the full one. +func TestMakePreflightTakesTheRecordsRuleOnlyForARecordsChange(t *testing.T) { + if _, err := exec.LookPath("make"); err != nil { + t.Skip("make unavailable") + } + c := newPrePushCase(t) + root := prePushSourceRoot(t) + // go.mod too: the Makefile reads the toolchain it declares as it parses. + for _, rel := range []string{"Makefile", "go.mod", "scripts/pinned-toolchain.sh"} { + src, err := os.ReadFile(filepath.Join(root, filepath.FromSlash(rel))) + if err != nil { + t.Fatalf("read the committed %s: %v", rel, err) + } + c.write(rel, string(src), 0o755) + } + c.git("add", "-A") + c.git("-c", "core.hooksPath=/dev/null", "commit", "-q", "-m", "the committed Makefile") + c.git("update-ref", "refs/remotes/origin/main", "HEAD") + dry := func() string { + t.Helper() + out, err := c.run(c.dir, "make", "-n", "preflight") + if err != nil { + t.Fatalf("make -n preflight: %v\n%s", err, out) + } + return out + } + c.commit(".abcd/work/issues/open/iss-1.md", "one\n") + if out := dry(); strings.Contains(out, "go test -race") || !strings.Contains(out, "\nscripts/records-only.sh\n") || + !regexp.MustCompile(`(?m)^scripts/preflight-receipt\.sh mint "[0-9a-f]{40} clean" records-only$`).MatchString(out) { + t.Fatalf("a records change did not take the records-only rule:\n%s", out) + } + c.commit("feature.go", "package feature\n") + if out := dry(); !strings.Contains(out, "go test -race") || strings.Contains(out, "records-only") { + t.Fatalf("a code change mixed into a records change did not take the full rule:\n%s", out) + } +} diff --git a/scripts/preflight-receipt.sh b/scripts/preflight-receipt.sh index ed803da67d..9d9ea67b1d 100755 --- a/scripts/preflight-receipt.sh +++ b/scripts/preflight-receipt.sh @@ -37,9 +37,16 @@ # Usage: # preflight-receipt.sh state print "
clean|dirty|hidden" for this # tree (hidden: an index flag hides edits) -# preflight-receipt.sh mint "