Fail a touched manifest's module check only for what the change brings (hq ADR 0237)

de-spiegel's and link2pay's manifests already fail the module check on main; without comparing against the base branch every pull request touching them would fail the gate for a fault none of them made. The gate's own rule: what was already so is said.
2026-10-06 22:34:56 +02:00
parent 14127d4878
commit 327654e57b
3 changed files with 109 additions and 15 deletions
@@ -72,6 +72,8 @@ type CheckSpec struct {
 	Modules   []string
 	New       []string
 	Manifests []string
+	// Base is the branch the pull request merges into: what the gate compares the change against.
+	Base string
 	// Judge is who judges the gate: "" the controller the mesh runs, "self" the change's own, "validator"
 	// the running one with the change's validator.
 	Judge string
@@ -390,7 +392,10 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry
 func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFile string, env []string,
 	inToolchain func(string, []string, ...string) []string, running func([]string) error, out *tail, bus, workspace,
 	name string, say func(step, format string, args ...any)) (string, string) {
-	// 1. The manifests the change touches, as the judge reads them: a manifest it cannot read fails here.
+	// 1. The manifests the change touches, as the judge reads them: a manifest it cannot read, or one with a
+	// problem the change brings, fails here. **What was already so on the base branch is said and fails
+	// nothing** — the gate's own rule: a module whose manifest the running controller already finds fault
+	// with would otherwise fail every pull request that touches it, for a fault none of them made.
 	var manifests []string
 	for _, m := range spec.Manifests {
 		if _, err := os.Stat(filepath.Join(tree, m)); err == nil {
@@ -398,17 +403,44 @@ func gateLayer(ctx context.Context, spec CheckSpec, tree, root, gate, verdictFil
 		}
 	}
 	if len(manifests) > 0 {
-		var own tail
-		cmd := exec.CommandContext(ctx, "docker", LabelledArgs("docker",
-			inToolchain(tree, env, append([]string{gate, "module", "check"}, manifests...)...), spec.ID)...)
-		inItsOwnGroup(cmd)
-		w := io.MultiWriter(out, &own)
-		cmd.Stdout, cmd.Stderr = w, w
-		if err := cmd.Run(); err != nil {
+		checked := func(dir string) (string, error) {
+			var own tail
+			cmd := exec.CommandContext(ctx, "docker", LabelledArgs("docker",
+				inToolchain(dir, env, append([]string{gate, "module", "check"}, manifests...)...), spec.ID)...)
+			inItsOwnGroup(cmd)
+			w := io.MultiWriter(out, &own)
+			cmd.Stdout, cmd.Stderr = w, w
+			err := cmd.Run()
+			return own.String(), err
+		}
+		said, err := checked(tree)
+		if err != nil {
 			if ctx.Err() != nil {
 				return "error", "the check was ended during the module check"
 			}
-			return "fail", "a manifest the change touches fails the module check: " + firstProblem(own.String())
+			// The same manifests as the base branch has them, beside the change.
+			was := ""
+			if spec.Base != "" {
+				base := filepath.Join(root, "base-manifests")
+				for _, m := range manifests {
+					body, err := gitShow(ctx, tree, "origin/"+spec.Base, m)
+					if err != nil {
+						continue // new on this branch: nothing was so before it
+					}
+					if err := os.MkdirAll(filepath.Dir(filepath.Join(base, m)), 0o755); err == nil {
+						_ = os.WriteFile(filepath.Join(base, m), body, 0o644)
+					}
+				}
+				fmt.Fprintf(out, "--- the same manifests on %s\n", spec.Base)
+				if _, err := os.Stat(base); err == nil {
+					was, _ = checked(base)
+				}
+			}
+			brought := newProblems(said, was)
+			if len(brought) > 0 {
+				return "fail", "a manifest the change touches fails the module check: " + brought[0]
+			}
+			fmt.Fprintf(out, "the module check's problems were all so on %s already: said, not the change's\n", spec.Base)
 		}
 	}

@@ -551,15 +583,46 @@ func replaceGoFiles(from, into string) error {
 	return nil
 }

-// firstProblem is the first line of a module check's output that names a problem.
-func firstProblem(s string) string {
+// problemLines are the lines of a module check's output that name a problem: not a manifest found ok, not
+// the count, not the closing note.
+func problemLines(s string) []string {
+	var out []string
 	for _, line := range strings.Split(s, "\n") {
 		line = strings.TrimSpace(line)
-		if line != "" && !strings.HasSuffix(line, "tool(s)") && !strings.Contains(line, ": ok") {
-			return line
+		switch {
+		case line == "", strings.Contains(line, ": ok"), strings.Contains(line, "problem(s) in"),
+			strings.Contains(line, "manifest(s) checked"):
+			continue
+		}
+		out = append(out, line)
+	}
+	return out
+}
+
+// newProblems are the problems a module check said of the change that it did not say of the base.
+func newProblems(change, base string) []string {
+	was := map[string]bool{}
+	for _, p := range problemLines(base) {
+		was[p] = true
+	}
+	var out []string
+	for _, p := range problemLines(change) {
+		if !was[p] {
+			out = append(out, p)
 		}
 	}
-	return lastLine(s)
+	if len(out) == 0 && len(problemLines(change)) == 0 {
+		// It failed and named nothing: not readable as already so.
+		out = append(out, lastLine(change))
+	}
+	return out
+}
+
+// gitShow is a file as a ref has it.
+func gitShow(ctx context.Context, dir, ref, file string) ([]byte, error) {
+	cmd := exec.CommandContext(ctx, "git", "show", ref+":"+filepath.ToSlash(file))
+	cmd.Dir = dir
+	return cmd.Output()
 }

 func prefixed(prefix string, items []string) []string {
@@ -292,6 +292,37 @@ func TestTheGateRunsWhenTheGraphIsTouchedBesideTheRepositorysOwnCheck(t *testing
 		t.Fatalf("a manifest the judge refuses answered %+v / %+v\n%s", v.Gate, v.Repo, v.Report)
 	}

+	// A fault the base branch already had is said and fails nothing; one the change brings fails.
+	repo, _ := aCheckedRepository(t, map[string]string{"modules/gitea/module.json": `{"module":"gitea","broken":true}`})
+	git := func(args ...string) string {
+		cmd := exec.Command("git", args...)
+		cmd.Dir = repo
+		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("checkout", "--quiet", "-b", "change")
+	if err := os.WriteFile(filepath.Join(repo, "modules/gitea/index.ts"), []byte("x"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	git("add", "-A")
+	git("commit", "--quiet", "-m", "y")
+	id := fmt.Sprintf("check-base-%d", time.Now().UnixNano())
+	v, err := Check(t.Context(), Command, CheckSpec{ID: id, Repository: repo, Ref: git("rev-parse", "HEAD"), Owner: "novox",
+		Repo: "mesh-catalog", Base: "main", Paths: []string{"modules/gitea/index.ts"}, Beside: beside,
+		Modules: []string{"gitea"}, Manifests: []string{"modules/gitea/module.json"}, Toolchain: goToolchain},
+		t.TempDir(), registry, GitCredential{}, nil)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if v.Gate.Verdict != "warning" || !strings.Contains(v.Report, "already") {
+		t.Fatalf("a fault the base already had failed the change: %+v\n%s", v.Gate, v.Report)
+	}
+
 	// A change to the controller judges itself: one that does not build fails its own gate.
 	v = check(map[string]string{"go.mod": "module x\n\ngo 1.22\n", "cmd/mesh-controller/main.go": "package main\nfunc main() { nope }\n",
 		"modules/gitea/module.json": "{}"}, "self")