Merge pull request 'The base branch's merge-check.sh judges a pull request, not the change's own copy (hq issue 310)' (#135) from fix/the-base-branchs-check-judges-a-pull-request into main

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))
+	// **The base branch's check judges the change** (novox/hq issue 310), never the one the change brings.
-	hasScript := scriptErr == nil
+	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)
+	}
+}