Judge a pull request by the base branch's merge-check.sh, not its own copy

A change could delete or gut its merge-check.sh and skip its own tests, and a
branch cut before the script escaped them. Where the base branch holds one, its
script, declared parts and toolchain run against the change's tree, and the
verdict says when the change lacks or alters the check (novox/hq issue 310).

internal/builder/check.go

@@ -43,7 +43,9 @@ import (
 //     <script>`, one line each, among the same first lines): each named script is run from the root in
 //     its own toolchain's container, after merge-check.sh, and the layer passes only when every part
 //     does (novox/hq issue 302 — the lab's Go replays went unchecked because its script runs in the
-//     TypeScript toolchain, which holds no Go compiler).
+//     TypeScript toolchain, which holds no Go compiler). **The script, its parts and its toolchain are
+//     the base branch's when it holds one** (novox/hq issue 310, check_base.go): a pull request cannot
+//     skip its own tests by deleting or weakening them, and one that alters them is said so.
 //
 // One check:
 //
@@ -264,8 +266,16 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry
 		return CheckVerdict{}, err
 	}
 	tree := filepath.Join(root, name)
-	script, scriptErr := os.ReadFile(filepath.Join(tree, CheckScript))
-	hasScript := scriptErr == nil
+	// **The base branch's check judges the change** (novox/hq issue 310), never the one the change brings.
+	judging, err := TheCheckThatJudges(ctx, tree, spec.Base)
+	if err != nil {
+		return CheckVerdict{}, err
+	}
+	script := judging.Script
+	hasScript := script != nil
+	if judging.Said != "" {
+		say("check", "%s", judging.Said)
+	}
 	gated := spec.Gated()
 	if !gated && !hasScript {
 		// Nothing to run: said, never passed silently.
@@ -447,12 +457,23 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry
 	case timedOut():
 		v.Repo = &Layer{Verdict: "error", Summary: fmt.Sprintf("the check ran past %s before its %s ran", CheckTimeout, CheckScript)}
 	default:
+		// The base branch's scripts in place of the change's, in the change's tree — after the gate, which
+		// composes the change as it is.
+		if err := judging.Put(tree); err != nil {
+			return v, err
+		}
+		if judging.Said != "" {
+			fmt.Fprintf(&out, "--- %s\n", judging.Said)
+		}
 		v.Repo = ownCheck(ctx, spec, ScriptParts(script), tree, &out, timedOut, func(image string, script string) *exec.Cmd {
 			return exec.CommandContext(ctx, "docker", LabelledArgs("docker", in(image, tree, env, "sh", script), spec.ID)...)
 		}, say)
 		if v.Repo == nil {
 			return v, ctx.Err()
 		}
+		if judging.Said != "" {
+			v.Repo.Summary = judging.Said + "; " + v.Repo.Summary
+		}
 	}
 
 	v.Verdict, v.Summary = v.Gate.Verdict, v.Gate.Summary

internal/builder/check_base.go

@@ -0,0 +1,134 @@
+package builder
+
+import (
+	"bytes"
+	"context"
+	"errors"
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+)
+
+// **The base branch's check judges a pull request** (novox/hq issue 310).
+//
+// A repository's own check is its merge-check.sh, the parts it declares (`# mesh-check-also:`) and the
+// toolchain it names. Were the check the one in the pull request's head, a change could delete or weaken
+// the script and so skip its own tests — and a branch cut before the repository had a script escaped
+// its tests without deleting anything. So when the base branch holds a merge-check.sh, **the base
+// branch's** script, its declared parts and its toolchain run against the change's tree, and the verdict
+// says so whenever the change lacks the script or alters any of it.
+//
+// A change that legitimately alters the check is judged by the check it replaces: its own version judges
+// the pull requests after it merges. A base branch with no script keeps what was so before — the head's,
+// when it brings one, and a warning when it brings none.
+
+// Judging is the check that judges a change, and what of it the base branch puts in the change's tree.
+type Judging struct {
+	// Script is the merge-check.sh that judges, nil when neither the base branch nor the change holds one.
+	Script []byte
+	// Said is what the verdict says of it, empty when the change's own script runs as the base's would.
+	Said string
+	// files are the base branch's scripts, by path in the tree, put there before the check runs.
+	files map[string][]byte
+}
+
+// Put writes the base branch's scripts into the change's tree, in place of the change's own.
+func (j Judging) Put(tree string) error {
+	for path, body := range j.files {
+		at := filepath.Join(tree, path)
+		if err := os.MkdirAll(filepath.Dir(at), 0o755); err != nil {
+			return err
+		}
+		if err := os.WriteFile(at, body, 0o755); err != nil {
+			return fmt.Errorf("the base branch's %s cannot be put in the change's tree: %w", path, err)
+		}
+	}
+	return nil
+}
+
+// TheCheckThatJudges is the check that judges the change cloned at tree, merging into base: the base
+// branch's when it holds one, the change's own otherwise. An error is that the base branch could not be
+// read — never a reason to fall back to the change's script.
+func TheCheckThatJudges(ctx context.Context, tree, base string) (Judging, error) {
+	head, err := os.ReadFile(filepath.Join(tree, CheckScript))
+	if err != nil && !errors.Is(err, os.ErrNotExist) {
+		return Judging{}, err
+	}
+	if err != nil {
+		head = nil
+	}
+	if base == "" {
+		return Judging{Script: head}, nil
+	}
+	ref := "origin/" + base
+	if _, err := gitIn(ctx, tree, "rev-parse", "--verify", "--quiet", ref+"^{commit}"); err != nil {
+		return Judging{}, fmt.Errorf("the base branch %s cannot be read, and the check it holds judges the change: %w",
+			base, err)
+	}
+	script, held, err := onBranch(ctx, tree, ref, CheckScript)
+	if err != nil {
+		return Judging{}, err
+	}
+	if !held {
+		return Judging{Script: head}, nil
+	}
+
+	j := Judging{Script: script, files: map[string][]byte{CheckScript: script}}
+	var altered []string
+	if head != nil && !bytes.Equal(head, script) {
+		altered = append(altered, CheckScript)
+	}
+	for _, part := range ScriptParts(script)[1:] {
+		if !filepath.IsLocal(part.Script) {
+			continue // refused by name when the check runs
+		}
+		body, held, err := onBranch(ctx, tree, ref, part.Script)
+		if err != nil {
+			return Judging{}, err
+		}
+		if !held {
+			continue // the base branch's own check lacks it: the change's, if it brings it
+		}
+		j.files[filepath.Clean(part.Script)] = body
+		if own, err := os.ReadFile(filepath.Join(tree, part.Script)); err != nil || !bytes.Equal(own, body) {
+			altered = append(altered, part.Script)
+		}
+	}
+	switch {
+	case head == nil:
+		j.Said = fmt.Sprintf("the change holds no %s: %s's judged it", CheckScript, base)
+		if len(altered) > 0 {
+			j.Said += fmt.Sprintf(", and the change alters its part(s) %s", strings.Join(altered, ", "))
+		}
+	case len(altered) > 0:
+		j.Said = fmt.Sprintf("THE CHANGE ALTERS ITS OWN CHECK (%s): %s's version judged it; the change's judges "+
+			"the pull requests after it merges", strings.Join(altered, ", "), base)
+	}
+	return j, nil
+}
+
+// onBranch is a file as a ref has it, and whether the ref holds it at all.
+func onBranch(ctx context.Context, tree, ref, path string) ([]byte, bool, error) {
+	at := ref + ":" + filepath.ToSlash(filepath.Clean(path))
+	// ls-tree answers nothing for a path the ref does not hold, and fails only when it cannot read.
+	listed, err := gitIn(ctx, tree, "ls-tree", ref, "--", filepath.ToSlash(filepath.Clean(path)))
+	if err != nil {
+		return nil, false, fmt.Errorf("%s cannot be read: %w", ref, err)
+	}
+	if fields := strings.Fields(string(listed)); len(fields) < 2 || fields[1] != "blob" {
+		return nil, false, nil // the ref holds no such file
+	}
+	body, err := gitIn(ctx, tree, "cat-file", "blob", at)
+	if err != nil {
+		return nil, false, fmt.Errorf("%s cannot be read: %w", at, err)
+	}
+	return body, true, nil
+}
+
+func gitIn(ctx context.Context, dir string, args ...string) ([]byte, error) {
+	cmd := exec.CommandContext(ctx, "git", args...)
+	cmd.Dir = dir
+	return cmd.Output()
+}

internal/builder/check_base_test.go

@@ -0,0 +1,210 @@
+package builder
+
+import (
+	"fmt"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+	"time"
+)
+
+// novox/hq issue 310: the build seat ran the merge-check.sh found in the pull request's head, so a change
+// could delete or gut it and skip its own tests, and a branch cut before the script escaped them. The base
+// branch's script — its parts and its toolchain with it — judges the change's tree.
+
+// aPullRequest is a repository whose main holds base, and a branch "change" off it that holds change on
+// top (a nil body deletes the file): cloned as the check clones it, at the change's head. It answers the
+// clone's tree.
+func aPullRequest(t *testing.T, base, change map[string]*string) string {
+	t.Helper()
+	origin := t.TempDir()
+	git := func(dir string, args ...string) string {
+		t.Helper()
+		cmd := exec.Command("git", args...)
+		cmd.Dir = dir
+		cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@example.org",
+			"GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.org")
+		out, err := cmd.CombinedOutput()
+		if err != nil {
+			t.Fatalf("git %v: %v\n%s", args, err, out)
+		}
+		return strings.TrimSpace(string(out))
+	}
+	write := func(files map[string]*string) {
+		for name, body := range files {
+			at := filepath.Join(origin, name)
+			if body == nil {
+				if err := os.Remove(at); err != nil {
+					t.Fatal(err)
+				}
+				continue
+			}
+			if err := os.MkdirAll(filepath.Dir(at), 0o755); err != nil {
+				t.Fatal(err)
+			}
+			if err := os.WriteFile(at, []byte(*body), 0o755); err != nil {
+				t.Fatal(err)
+			}
+		}
+		git(origin, "add", "-A")
+		git(origin, "commit", "--quiet", "--allow-empty", "-m", "x")
+	}
+	git(origin, "init", "--quiet", "-b", "main")
+	write(base)
+	git(origin, "checkout", "--quiet", "-b", "change")
+	write(change)
+	head := git(origin, "rev-parse", "HEAD")
+	git(origin, "checkout", "--quiet", "main")
+
+	root := t.TempDir()
+	git(root, "clone", "--quiet", origin, "checked")
+	tree := filepath.Join(root, "checked")
+	git(tree, "checkout", "--quiet", head)
+	return tree
+}
+
+func s(body string) *string { return &body }
+
+// judged runs the check that judges the change as the build seat would — each part a plain sh here.
+func judged(t *testing.T, tree, base string) (Judging, *Layer) {
+	t.Helper()
+	j, err := TheCheckThatJudges(t.Context(), tree, base)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if j.Script == nil {
+		return j, nil
+	}
+	if err := j.Put(tree); err != nil {
+		t.Fatal(err)
+	}
+	var out tail
+	layer := ownCheck(t.Context(), CheckSpec{Toolchain: "go-image", Toolchains: map[string]string{"go": "go-image",
+		"typescript": "ts-image"}}, ScriptParts(j.Script), tree, &out, func() bool { return false },
+		func(image, script string) *exec.Cmd {
+			cmd := exec.CommandContext(t.Context(), "sh", script)
+			cmd.Dir = tree
+			return cmd
+		}, func(string, string, ...any) {})
+	return j, layer
+}
+
+// main's check: the suite fails unless the tree holds what main requires.
+const mainsCheck = "#!/bin/sh\nset -eu\ntest -f covered || { echo 'FAIL: the suite does not pass'; exit 1; }\n"
+
+func TestAHeadWithoutTheScriptIsJudgedByMains(t *testing.T) {
+	// Deleted by the change — or never there, on a branch cut before main had it: the head lacks it.
+	tree := aPullRequest(t, map[string]*string{CheckScript: s(mainsCheck)}, map[string]*string{CheckScript: nil})
+	j, layer := judged(t, tree, "main")
+	if layer == nil || layer.Verdict != "fail" || !strings.Contains(layer.Summary, "does not pass") {
+		t.Fatalf("a head without the script answered %+v — main's script was not what judged it", layer)
+	}
+	if !strings.Contains(j.Said, "the change holds no "+CheckScript) || !strings.Contains(j.Said, "main's judged it") {
+		t.Errorf("the verdict does not say main's judged it: %q", j.Said)
+	}
+
+	// And a head that holds what main's script asks passes by it.
+	tree = aPullRequest(t, map[string]*string{CheckScript: s(mainsCheck)},
+		map[string]*string{CheckScript: nil, "covered": s("x")})
+	if _, layer = judged(t, tree, "main"); layer == nil || layer.Verdict != "pass" {
+		t.Fatalf("a head main's script passes answered %+v", layer)
+	}
+}
+
+func TestAHeadThatGutsTheScriptIsStillJudgedByMains(t *testing.T) {
+	tree := aPullRequest(t, map[string]*string{CheckScript: s(mainsCheck)},
+		map[string]*string{CheckScript: s("#!/bin/sh\nexit 0\n")})
+	j, layer := judged(t, tree, "main")
+	if layer == nil || layer.Verdict != "fail" {
+		t.Fatalf("a gutted script answered %+v — the change's own copy judged it", layer)
+	}
+	if !strings.Contains(j.Said, "ALTERS ITS OWN CHECK") || !strings.Contains(j.Said, CheckScript) {
+		t.Errorf("the verdict does not say the change alters its check: %q", j.Said)
+	}
+
+	// Gutting the declared part instead, or declaring none: main's lines and main's part still judge.
+	two := "#!/bin/sh\n# mesh-check-toolchain: typescript\n# mesh-check-also: go replays/merge-check.sh\nexit 0\n"
+	tree = aPullRequest(t, map[string]*string{CheckScript: s(two), "replays/merge-check.sh": s(mainsCheck)},
+		map[string]*string{CheckScript: s("#!/bin/sh\n# mesh-check-toolchain: typescript\nexit 0\n"),
+			"replays/merge-check.sh": s("exit 0\n")})
+	j, layer = judged(t, tree, "main")
+	if layer == nil || layer.Verdict != "fail" || !strings.Contains(layer.Summary, "replays/merge-check.sh failed") {
+		t.Fatalf("a gutted part answered %+v", layer)
+	}
+	if parts := ScriptParts(j.Script); len(parts) != 2 || parts[0].Toolchain != "typescript" {
+		t.Errorf("main's toolchain and parts did not judge: %+v", parts)
+	}
+	if !strings.Contains(j.Said, CheckScript+", replays/merge-check.sh") {
+		t.Errorf("the verdict does not name both altered scripts: %q", j.Said)
+	}
+}
+
+// A change that legitimately alters the check is judged by main's, says so, and passes when main's passes:
+// its own version judges the pull requests after it.
+func TestAChangeToTheCheckIsJudgedByTheOneItReplaces(t *testing.T) {
+	tree := aPullRequest(t, map[string]*string{CheckScript: s(mainsCheck), "covered": s("x")},
+		map[string]*string{CheckScript: s(mainsCheck + "test -f also-covered\n")})
+	j, layer := judged(t, tree, "main")
+	if layer == nil || layer.Verdict != "pass" || !strings.Contains(j.Said, "ALTERS ITS OWN CHECK") {
+		t.Fatalf("a change to the check answered %+v, said %q", layer, j.Said)
+	}
+	// The same script as main's: nothing to say.
+	tree = aPullRequest(t, map[string]*string{CheckScript: s(mainsCheck), "covered": s("x")},
+		map[string]*string{"other.go": s("package x\n")})
+	if j, layer = judged(t, tree, "main"); layer == nil || layer.Verdict != "pass" || j.Said != "" {
+		t.Fatalf("an unchanged check answered %+v, said %q", layer, j.Said)
+	}
+}
+
+// Where main holds no script, what was so stays: the head's, or none.
+func TestWhereMainHoldsNoScriptTheHeadsRuns(t *testing.T) {
+	tree := aPullRequest(t, map[string]*string{"README.md": s("x")},
+		map[string]*string{CheckScript: s("echo 'the first check'; exit 0\n")})
+	j, layer := judged(t, tree, "main")
+	if layer == nil || layer.Verdict != "pass" || j.Said != "" {
+		t.Fatalf("the head's first script answered %+v, said %q", layer, j.Said)
+	}
+	tree = aPullRequest(t, map[string]*string{"README.md": s("x")}, map[string]*string{"b.md": s("y")})
+	if j, _ = judged(t, tree, "main"); j.Script != nil {
+		t.Fatalf("neither holds a script, and one judged: %q", j.Script)
+	}
+	// No base asked (a check by hand without one): the head's, as before.
+	tree = aPullRequest(t, map[string]*string{CheckScript: s(mainsCheck)}, map[string]*string{CheckScript: s("exit 0\n")})
+	if j, layer = judged(t, tree, ""); layer == nil || layer.Verdict != "pass" {
+		t.Fatalf("with no base the head's answered %+v", layer)
+	}
+}
+
+// A base branch that cannot be read is an error — never a fall back to the change's own script.
+func TestABaseThatCannotBeReadIsAnError(t *testing.T) {
+	tree := aPullRequest(t, map[string]*string{CheckScript: s(mainsCheck)}, map[string]*string{CheckScript: s("exit 0\n")})
+	if _, err := TheCheckThatJudges(t.Context(), tree, "no-such-branch"); err == nil ||
+		!strings.Contains(err.Error(), "no-such-branch") {
+		t.Fatalf("an unreadable base said %v", err)
+	}
+}
+
+// The whole check, against a real container runtime: a head without the script fails by main's.
+func TestACheckOfAHeadWithoutTheScriptRunsMains(t *testing.T) {
+	registry := checkEnvironment(t)
+	tree := aPullRequest(t, map[string]*string{CheckScript: s(mainsCheck)}, map[string]*string{CheckScript: nil})
+	head, err := gitIn(t.Context(), tree, "rev-parse", "HEAD")
+	if err != nil {
+		t.Fatal(err)
+	}
+	origin, err := gitIn(t.Context(), tree, "remote", "get-url", "origin")
+	if err != nil {
+		t.Fatal(err)
+	}
+	v, err := Check(t.Context(), Command, CheckSpec{ID: fmt.Sprintf("check-base-%d", time.Now().UnixNano()),
+		Repository: strings.TrimSpace(string(origin)), Ref: strings.TrimSpace(string(head)), Owner: "novox",
+		Repo: "hq", Base: "main", Toolchain: goToolchain}, t.TempDir(), registry, GitCredential{}, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if v.Repo == nil || v.Repo.Verdict != "fail" || !strings.Contains(v.Repo.Summary, "main's judged it") {
+		t.Fatalf("a head without the script answered %+v\n%s", v.Repo, v.Report)
+	}
+}

internal/builder/replay310_test.go

@@ -0,0 +1,52 @@
+package builder
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+// TestReplay310 is the replay of novox/hq issue 310, written with only what the build agent had before its
+// fix, so mesh-lab's prover can lay it over the commit before: a pull request whose branch holds no
+// merge-check.sh, into a main that holds one. Before the fix the check read the head's tree alone, found no
+// script, and answered the repository's layer a warning — none of its tests run, and the forge took it.
+// After it, main's script judges the change: with no artifact store to read the facts from here, the check
+// goes on to run it and says it cannot, and never answers the warning.
+func TestReplay310(t *testing.T) {
+	origin := t.TempDir()
+	git := func(args ...string) string {
+		t.Helper()
+		cmd := exec.Command("git", args...)
+		cmd.Dir = origin
+		cmd.Env = append(os.Environ(), "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@example.org",
+			"GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@example.org")
+		out, err := cmd.CombinedOutput()
+		if err != nil {
+			t.Fatalf("git %v: %v\n%s", args, err, out)
+		}
+		return strings.TrimSpace(string(out))
+	}
+	git("init", "--quiet", "-b", "main")
+	if err := os.WriteFile(filepath.Join(origin, CheckScript), []byte("echo 'FAIL: the suite'; exit 1\n"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	git("add", "-A")
+	git("commit", "--quiet", "-m", "main holds its check")
+	git("checkout", "--quiet", "-b", "change")
+	git("rm", "--quiet", CheckScript)
+	git("commit", "--quiet", "-m", "the change holds none")
+	head := git("rev-parse", "HEAD")
+	git("checkout", "--quiet", "main")
+
+	v, err := Check(t.Context(), Command, CheckSpec{ID: "replay-310", Repository: origin, Ref: head, Owner: "novox",
+		Repo: "mesh-media-catalog", Number: 11, Base: "main"}, t.TempDir(), "", GitCredential{}, nil)
+	if err == nil && (v.Repo == nil || v.Repo.Verdict != "fail") {
+		t.Fatalf("a pull request without main's %s was answered %+v: none of its tests ran, and main's script "+
+			"did not judge it", CheckScript, v.Repo)
+	}
+	if err != nil && !strings.Contains(err.Error(), "artifact store") {
+		t.Fatalf("the check stopped before main's script could run: %v", err)
+	}
+}