From b7084a19e9bf1547766503ebe85b44db8e70448d Mon Sep 17 00:00:00 2001 From: mesh-admin Date: Wed, 14 Oct 2026 13:30:46 +0000 Subject: [PATCH] Build every judge where no pull request can reach, and never read mesh-lab at a release Review of #329: a judge built with HOME in the workspace runs git under a configuration a pull request's script can plant, so a hook could rewrite the judge's sources before they compile and the result be kept under an honest key (issue 482, issue 704). Every judge now builds with its own HOME, configuration and Go cache outside the workspace, no git configuration and no VCS stamping. Co-Authored-By: Claude Opus 5.5 --- cmd/mesh-controller/checks.go | 2 +- cmd/mesh-controller/release_branch_test.go | 10 ++ internal/builder/check.go | 76 ++++++++---- internal/builder/release_branch_test.go | 137 ++++++++++++++++++++- 4 files changed, 200 insertions(+), 25 deletions(-) diff --git a/cmd/mesh-controller/checks.go b/cmd/mesh-controller/checks.go index 866c2988..1c7eefda 100644 --- a/cmd/mesh-controller/checks.go +++ b/cmd/mesh-controller/checks.go @@ -370,7 +370,7 @@ func releaseBranchOf(ctx context.Context, p link.PullUpdated, scope checkScope, // trunkOf is the branch a module's source follows: main when it records none, or records a commit. func trunkOf(s inventory.Source) string { - if t := followedBranch(s.Ref); t != "" { + if t := strings.TrimSpace(followedBranch(s.Ref)); t != "" { return t } return "main" diff --git a/cmd/mesh-controller/release_branch_test.go b/cmd/mesh-controller/release_branch_test.go index 52cf43d8..298b1588 100644 --- a/cmd/mesh-controller/release_branch_test.go +++ b/cmd/mesh-controller/release_branch_test.go @@ -261,3 +261,13 @@ func TestATrunkIsTheBranchTheRepositoryFollows(t *testing.T) { } } } + +// trunkOf: the branch a source follows, and main when it records none or records a commit (issue 215's rule). +func TestATrunkIsMainWhenTheSourceRecordsNone(t *testing.T) { + for ref, want := range map[string]string{"": "main", "main": "main", "master": "master", "nox-mesh": "nox-mesh", + "0123456789abcdef0123456789abcdef01234567": "main", " ": "main"} { + if got := trunkOf(inventory.Source{Ref: ref}); got != want { + t.Errorf("a source recording %q follows %q, not %q", ref, got, want) + } + } +} diff --git a/internal/builder/check.go b/internal/builder/check.go index 240a0901..202a9abf 100644 --- a/internal/builder/check.go +++ b/internal/builder/check.go @@ -429,10 +429,17 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry if dir == name || !safeName.MatchString(dir) { continue } - if dir == "mesh-lab" && b.Else != "" { - // **mesh-lab is read at its trunk, always** (novox/hq ADR 0358): its replays run with the container - // runtime's socket, so a branch of a release's name — which anyone may create — is never what runs there. - b.Ref, b.Else = b.Else, "" + if dir == "mesh-lab" { + // **mesh-lab is never read at a release branch** (novox/hq ADR 0358): its replays run with the container + // runtime's socket, so a branch of a release's name — which anyone may create — is never what runs there, + // whether it is asked with a fallback or without. + if b.Else != "" || (spec.Release != "" && b.Ref == spec.Release) { + b.Ref = b.Else + if b.Ref == "" { + b.Ref = "main" + } + } + b.Else = "" } if err := cloneAt(b.Repository, b.Ref, b.Else, dir); err != nil { return CheckVerdict{}, fmt.Errorf("beside it, %w", err) @@ -565,21 +572,47 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry user = fmt.Sprintf("%d:%d", os.Getuid(), os.Getgid()) } in := func(image, dir string, env []string, command ...string) []string { + home := workspace + for _, e := range env { + if strings.HasPrefix(e, "HOME=") { + home = strings.TrimPrefix(e, "HOME=") + } + } // As the builder itself: what a check writes into the workspace is the builder's to remove. args := []string{"run", "--rm", "--network", "host", "--volume", workspace + ":" + workspace, "--workdir", dir, - "--user", user, "--env", "HOME=" + workspace, + "--user", user, "--env", "HOME=" + home, // The check's own checkouts, whoever cloned them: git in a container of another user than the // one that cloned refuses a repository it does not own ("dubious ownership"), and Go's build // stamps the version from git — a judge that would not build for want of it. "--env", "GIT_CONFIG_COUNT=1", "--env", "GIT_CONFIG_KEY_0=safe.directory", "--env", "GIT_CONFIG_VALUE_0=*"} for _, e := range env { - args = append(args, "--env", e) + if !strings.HasPrefix(e, "HOME=") { + args = append(args, "--env", e) + } } return append(append(args, image), command...) } inToolchain := func(dir string, env []string, command ...string) []string { return in(spec.Toolchain, dir, env, command...) } + // **Every judge is built where no pull request can reach** (novox/hq issue 482, ADR 0358 review): the workspace's + // Go cache, HOME and git configuration are written by pull requests' own scripts, and git runs during a Go build + // to stamp its version — a planted hook would rewrite the judge's sources before they compile. So a judge is built + // with a HOME, configuration and Go cache of its own, outside the workspace and made fresh for this check, no + // global or system git configuration, no Go environment file, and no version stamping from git. + judgeHome, err := os.MkdirTemp("", "mesh-check-judge-") + if err != nil { + return CheckVerdict{}, err + } + defer os.RemoveAll(judgeHome) + if withinDir(workspace, judgeHome) { + return CheckVerdict{}, fmt.Errorf("the temporary directory %s is inside the workspace %s, which a check's "+ + "containers mount: no judge is built where a pull request could reach it", judgeHome, workspace) + } + inJudgeBuild := func(dir string, _ []string, command ...string) []string { + args := in(spec.Toolchain, dir, JudgeBuildEnv(judgeHome), command...) + return append([]string{args[0], "--volume", judgeHome + ":" + judgeHome}, args[1:]...) + } var out tail // running runs one container of the check, its output into the report, in a process group of its own. running := func(args []string) error { @@ -591,7 +624,7 @@ func Check(ctx context.Context, run Runner, spec CheckSpec, workspace, registry // The judge. Its failing to build is the change's fault when the change is the judge or its validator, // and the check's when it is the controller the mesh runs. - gate, judgeFault, err := judgeFor(ctx, labelled, run, spec, root, tree, judged.BesideAt, inToolchain, say) + gate, judgeFault, err := judgeFor(ctx, labelled, run, spec, root, tree, judged.BesideAt, inJudgeBuild, say) if err != nil { return CheckVerdict{}, err } @@ -1046,8 +1079,7 @@ func judgeFor(ctx context.Context, labelled, run Runner, spec CheckSpec, root, t switch spec.Judge { case "self": bin := filepath.Join(root, "bin", "judge-of-itself") - if _, err := labelled(ctx, root, "docker", inToolchain(tree, - []string{"CGO_ENABLED=0", "GOFLAGS=-mod=vendor", "GOPROXY=off", "GOCACHE=" + filepath.Join(filepath.Dir(root), "go-cache")}, + if _, err := labelled(ctx, root, "docker", inToolchain(tree, nil, "go", "build", "-o", bin, "./cmd/mesh-controller")...); err != nil { return "", "the change's controller, which judges itself, does not build: " + firstLine(err.Error()), nil } @@ -1080,8 +1112,7 @@ func judgeFor(ctx context.Context, labelled, run Runner, spec CheckSpec, root, t } } withValidator := filepath.Join(root, "bin", "judge-with-this-validator") - if _, err := labelled(ctx, root, "docker", inToolchain(filepath.Join(root, judgeTree), - []string{"CGO_ENABLED=0", "GOFLAGS=-mod=vendor", "GOPROXY=off", "GOCACHE=" + filepath.Join(filepath.Dir(root), "go-cache")}, + if _, err := labelled(ctx, root, "docker", inToolchain(filepath.Join(root, judgeTree), nil, "go", "build", "-o", withValidator, "./cmd/mesh-controller")...); err != nil { if spec.Release != "" { return "", spec.Release + "'s controller does not build with this change's validator: " + @@ -1135,12 +1166,8 @@ func releaseJudge(ctx context.Context, run, plain Runner, spec CheckSpec, root s } } } - // **Built with a cache of its own, never the workspace's** (novox/hq issue 482): the workspace's Go cache and HOME are - // written by every pull request's own script, so a poisoned cache entry or Go environment file would otherwise be - // built into the judge kept for this head. A fresh cache under this check's root, made before anything of the - // change runs, and no Go environment file. - if _, err := run(ctx, root, "docker", inToolchain(filepath.Join(root, dir), - []string{"CGO_ENABLED=0", "GOFLAGS=-mod=vendor", "GOPROXY=off", "GOENV=off", "GOCACHE=" + judgeGoCache(root)}, + // Built where no pull request reaches (JudgeBuildEnv): a judge kept for this head is the head's, nothing else. + if _, err := run(ctx, root, "docker", inToolchain(filepath.Join(root, dir), nil, "go", "build", "-o", bin, "./cmd/mesh-controller")...); err != nil { return "", "", fmt.Errorf("the controller of %s does not build%s: %w", spec.Release, with, err) } @@ -1150,9 +1177,15 @@ func releaseJudge(ctx context.Context, run, plain Runner, spec CheckSpec, root s return bin, dir, nil } -// judgeGoCache is the Go cache a release branch's judge is built with: fresh for each check, under its root, which is -// emptied before the check clones anything — never the workspace's shared cache. -func judgeGoCache(root string) string { return filepath.Join(root, "judge-go-cache") } +// JudgeBuildEnv is the environment every judge is built in (novox/hq issue 482): home, a directory outside the +// workspace made fresh for the check, as HOME, configuration and Go cache; no global or system git configuration; no +// Go environment file; and no version stamped from git, so no git runs during the build at all. +func JudgeBuildEnv(home string) []string { + return []string{"HOME=" + home, "XDG_CONFIG_HOME=" + filepath.Join(home, ".config"), + "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_NOSYSTEM=1", "GOENV=off", + "GOCACHE=" + filepath.Join(home, "go-cache"), "GOPATH=" + filepath.Join(home, "go"), + "CGO_ENABLED=0", "GOFLAGS=-mod=vendor -buildvcs=false", "GOPROXY=off"} +} // judgesKept is how many release-branch judges a judge cache keeps: a head moved past is not built from again. const judgesKept = 4 @@ -1514,8 +1547,7 @@ func judge(ctx context.Context, run, plain Runner, root string, inToolchain func say func(step, format string, args ...any)) (string, string, error) { bin := filepath.Join(root, "bin", "mesh-controller") build := func(dir string) error { - _, err := run(ctx, root, "docker", inToolchain(filepath.Join(root, dir), - []string{"CGO_ENABLED=0", "GOFLAGS=-mod=vendor", "GOPROXY=off", "GOCACHE=" + filepath.Join(filepath.Dir(root), "go-cache")}, + _, err := run(ctx, root, "docker", inToolchain(filepath.Join(root, dir), nil, "go", "build", "-o", bin, "./cmd/mesh-controller")...) return err } diff --git a/internal/builder/release_branch_test.go b/internal/builder/release_branch_test.go index 1ca1609b..d5509687 100644 --- a/internal/builder/release_branch_test.go +++ b/internal/builder/release_branch_test.go @@ -144,6 +144,7 @@ run) if [ -n "$detached" ]; then echo a-container; exit 0; fi shift if [ "$1" = go ]; then echo "GOCACHE=$GOCACHE GOENV=$GOENV $*" >> "` + built + `"; fi + if [ "$1" = go ] && [ "$2" = build ] && [ -n "$JUDGE_HOMES" ]; then echo "$HOME" >> "$JUDGE_HOMES"; fi cd "$workdir" && exec "$@" ;; esac exit 1 @@ -258,8 +259,8 @@ func TestACataloguePullRequestIntoAReleaseBranchIsJudgedByThatBranchsController( // **The judge is built with a cache no pull request's script can write** (issue 482): a fresh one under the // check's own root, never the workspace's shared go-cache, and no Go environment file from the workspace's HOME. said, _ := os.ReadFile(built) - if !strings.Contains(string(said), "/check/judge-go-cache GOENV=off go build") || - strings.Contains(string(said), "/go-cache GOENV") { + if !strings.Contains(string(said), "/mesh-check-judge-") || !strings.Contains(string(said), "GOENV=off go build") || + strings.Contains(string(said), "/check/../go-cache") { t.Fatalf("the release judge was built with the workspace's cache or Go environment:\n%s", said) } @@ -412,3 +413,135 @@ func TestAReleaseBranchCheckReadsMeshLabAtItsTrunkEvenWhenItHasThatBranch(t *tes labMain[:12]) } } + +// aPlantedWorkspace is a workspace a pull request's own script has written into, as it can: a git configuration in its +// HOME whose fsmonitor hook says each directory git runs in, a git configuration under XDG_CONFIG_HOME doing the same, +// and a Go environment file that would hand the judge's build a tool of its own. It answers the workspace and the +// file the hook writes to. +func aPlantedWorkspace(t *testing.T) (string, string) { + t.Helper() + workspace, logs := t.TempDir(), t.TempDir() + ran := filepath.Join(logs, "hook-ran") + hook := filepath.Join(workspace, "hook.sh") + files := map[string]string{ + hook: "#!/bin/sh\necho \"$PWD\" >> " + ran + "\n", + filepath.Join(workspace, ".gitconfig"): "[core]\n\tfsmonitor = " + hook + "\n", + filepath.Join(workspace, ".config", "git", "config"): "[core]\n\tfsmonitor = " + hook + "\n", + filepath.Join(workspace, ".config", "go", "env"): "GOFLAGS=-toolexec=" + hook + "\n", + } + for name, body := range files { + if err := os.MkdirAll(filepath.Dir(name), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(name, []byte(body), 0o755); err != nil { + t.Fatal(err) + } + } + return workspace, ran +} + +// judgeRanTheHook says whether the planted hook ran in a judge's tree: the controller beside the check, or the +// change's own when it judges itself. +func judgeRanTheHook(t *testing.T, ran string) string { + t.Helper() + body, _ := os.ReadFile(ran) + for _, line := range strings.Split(string(body), "\n") { + if strings.HasSuffix(line, "/check/mesh-controller") { + return string(body) + } + } + return "" +} + +// **Every judge is built where no pull request reaches** (issue 482, ADR 0358 review): a git configuration with a +// hook, and a Go environment file, planted in the workspace by an earlier pull request's script, do not run during +// the release judge's build — while the pull request's own script, in the workspace, still meets them. +func TestAHookPlantedInTheWorkspaceDoesNotRunDuringTheReleaseJudgesBuild(t *testing.T) { + aRuntimeThatBuildsTheJudge(t) + homes := filepath.Join(t.TempDir(), "homes") + t.Setenv("JUDGE_HOMES", homes) + registry := aFactsRegistry(t) + workspace, ran := aPlantedWorkspace(t) + beside, _, _, _ := theReleasedMesh(t) + catalogue, head := aPullRequestInto(t, map[string]string{"modules/gitea/module.json": `{"module":"gitea"}`, + CheckScript: "git status >/dev/null 2>&1; true\n"}, releaseBranch, map[string]string{"modules/gitea/index.ts": "x"}) + v, err := Check(t.Context(), Command, CheckSpec{ID: "check-704-planted", Repository: catalogue, Ref: head, + Owner: "novox", Repo: "mesh-catalog", Number: 286, Base: releaseBranch, Release: releaseBranch, + JudgeCache: t.TempDir(), Paths: []string{"modules/gitea/index.ts"}, Modules: []string{"gitea"}, Beside: beside, + Toolchain: "golang"}, workspace, registry, GitCredential{}, nil) + if err != nil { + t.Fatal(err) + } + if v.Gate == nil || v.Gate.Verdict != "pass" { + t.Fatalf("the check did not judge: %+v\n%s", v.Gate, v.Report) + } + if body, _ := os.ReadFile(ran); !strings.Contains(string(body), "/check/mesh-catalog") { + t.Fatalf("the planted hook never ran, so this test proves nothing: %q", body) + } + if said := judgeRanTheHook(t, ran); said != "" { + t.Fatalf("the hook planted in the workspace ran during the judge's build:\n%s", said) + } + if body, _ := os.ReadFile(homes); len(body) == 0 || strings.Contains(string(body), workspace) { + t.Fatalf("the judge was built with a HOME in the workspace: %q", body) + } +} + +// The same for the judge built with a node-engine change's own validator, into a release branch (ADR 0358 §2). +func TestAHookPlantedInTheWorkspaceDoesNotRunDuringTheValidatorJudgesBuild(t *testing.T) { + aRuntimeThatBuildsTheJudge(t) + homes := filepath.Join(t.TempDir(), "homes") + t.Setenv("JUDGE_HOMES", homes) + registry := aFactsRegistry(t) + workspace, ran := aPlantedWorkspace(t) + beside, _, _, _ := theReleasedMesh(t) + delete(beside, "mesh-host") + files := aHostSaying("the release branch's validator") + files["module.json"] = `{"module":"mesh-host"}` + files[CheckScript] = "git status >/dev/null 2>&1; true\n" + host, head := aPullRequestInto(t, files, releaseBranch, aHostSaying("this change's validator")) + v, err := Check(t.Context(), Command, CheckSpec{ID: "check-704-planted-validator", Repository: host, Ref: head, + Owner: "novox", Repo: "mesh-host", Number: 111, Base: releaseBranch, Release: releaseBranch, Judge: "validator", + JudgeCache: t.TempDir(), Paths: []string{"validate/v.go"}, Modules: []string{"mesh-host"}, Beside: beside, + Toolchain: "golang"}, workspace, registry, GitCredential{}, nil) + if err != nil { + t.Fatal(err) + } + if v.Gate == nil || !strings.HasSuffix(v.Gate.Summary, "the release branch's controller with this change's validator") { + t.Fatalf("a node-engine change into a release branch was not judged by that branch's controller with its own "+ + "validator: %+v\n%s", v.Gate, v.Report) + } + if body, _ := os.ReadFile(ran); !strings.Contains(string(body), "/check/mesh-host") { + t.Fatalf("the planted hook never ran, so this test proves nothing: %q", body) + } + if said := judgeRanTheHook(t, ran); said != "" { + t.Fatalf("the hook planted in the workspace ran during a judge's build:\n%s", said) + } + if body, _ := os.ReadFile(homes); strings.Count(string(body), "\n") != 2 || strings.Contains(string(body), workspace) { + t.Fatalf("the judges were not both built with a HOME outside the workspace: %q", body) + } +} + +// **mesh-lab is never read at a release branch**, asked with a fallback or without (ADR 0358). +func TestTheBuildSeatNeverReadsMeshLabAtAReleaseBranch(t *testing.T) { + aRuntimeThatBuildsTheJudge(t) + registry := aFactsRegistry(t) + lab, labMain, _ := aRepositoryWithARelease(t, map[string]string{"README": "the lab"}, + map[string]string{"README": "a branch anyone could push"}) + for _, asked := range []Beside{{Repository: lab, Ref: releaseBranch}, {Repository: lab, Ref: releaseBranch, Else: "main"}} { + beside, _, _, _ := theReleasedMesh(t) + beside["mesh-lab"] = asked + catalogue, head := aPullRequestInto(t, map[string]string{"modules/gitea/module.json": `{"module":"gitea"}`, + CheckScript: "true\n"}, releaseBranch, map[string]string{"modules/gitea/index.ts": "x"}) + v, err := Check(t.Context(), Command, CheckSpec{ID: "check-704-lab-asked", Repository: catalogue, Ref: head, + Owner: "novox", Repo: "mesh-catalog", Number: 286, Base: releaseBranch, Release: releaseBranch, + JudgeCache: t.TempDir(), Paths: []string{"modules/gitea/index.ts"}, Modules: []string{"gitea"}, Beside: beside, + Toolchain: "golang"}, t.TempDir(), registry, GitCredential{}, nil) + if err != nil { + t.Fatal(err) + } + if v.BesideAt["mesh-lab"] != "main" || v.Beside["mesh-lab"] != labMain[:12] { + t.Fatalf("mesh-lab asked as %+v was read at %q (%s), not main at %s", asked, v.BesideAt["mesh-lab"], + v.Beside["mesh-lab"], labMain[:12]) + } + } +}