fix: isolate proto freshness generation

docs/adr/0003-isolate-proto-freshness-generation.md

@@ -0,0 +1,49 @@
+# Isolate proto freshness generation from the scanned repository
+
+Decided 2026-08-25. Check 2f ran `make proto` in the scanned CMS working tree
+and compared `git status` before and after. That made a read-oriented safety
+check destructive: concurrent checker processes generated into the same files,
+an interrupted run could leave a partially rewritten tree, and a generated
+file that was already dirty could change bytes without changing its porcelain
+status.
+
+## Decision
+
+- Copy the proto generation inputs and existing generated outputs into a unique
+  temporary sandbox for every freshness invocation.
+- Run `make proto` only in that sandbox. Link only the installed package-local
+  generator executables and their package entrypoints needed by Buf and the
+  export script; generated output paths never point back into the scanned
+  repository.
+- Snapshot generated regular files and symlinks by type, mode, link target, and
+  SHA-256 content digest before and after generation. Sort paths before
+  reporting added, modified, and removed outputs in the existing porcelain-like
+  CLI format.
+- Remove the owned sandbox on success, generator failure, and all ordinary
+  return paths. Abrupt process interruption can affect only disposable system
+  temporary state, never the scanned working tree.
+- Exercise the boundary with real `make` recipes, including independent
+  concurrent checker processes and a generator that writes partial output
+  before failing.
+
+A repository-wide lock was rejected because it serializes independent safety
+runs and still leaves the live working tree vulnerable to interrupted
+generation. Comparing only Git status in a temporary checkout was rejected
+because status is not a content comparison and can conceal a second rewrite of
+an already-modified file. Copying the entire repository, including dependency
+trees and build artifacts, was rejected because proto generation needs only a
+small, explicit input and output surface.
+
+## Consequences
+
+- Parallel check-safety processes can run proto freshness checks without
+  contending on generated files or corrupting the CMS checkout.
+- Check 2f retains its `make proto` success, failure, and before/after finding
+  messages while its freshness verdict now follows deterministic file content.
+- Generator stderr is still returned through `protoFreshnessResult.output`, and
+  sandbox preparation, snapshot, generation, and cleanup failures retain their
+  causal error chains.
+- `proto_freshness.go` owns isolation and comparison; focused regression
+  coverage lives in `proto_freshness_test.go`.
+
+Keywords: check-safety, check 2f, proto freshness, make proto, concurrency, interruption safety, sandbox, checkProtoGeneratedFreshness, checkProtoGeneratedFreshnessInSandbox, protoFreshnessResult, proto_freshness.go, proto_freshness_test.go, backend/internal/api, backend/internal/mcpserver/generated, packages/api/src, buf generate, pnpm generate-exports

proto_freshness.go

@@ -0,0 +1,302 @@
+package main
+
+import (
+	"crypto/sha256"
+	"errors"
+	"fmt"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"sort"
+	"strings"
+)
+
+var protoGeneratedPaths = []string{
+	"backend/internal/api",
+	"backend/internal/mcpserver/generated",
+	"packages/api/src",
+}
+
+var protoSandboxPaths = []string{
+	"Makefile",
+	"buf.yaml",
+	"buf.gen.yaml",
+	"buf.lock",
+	"package.json",
+	"pnpm-lock.yaml",
+	"pnpm-workspace.yaml",
+	"proto",
+	"backend/internal/api",
+	"backend/internal/mcpserver/generated",
+	"packages/api",
+}
+
+var protoSandboxTools = []struct {
+	name        string
+	packagePath string
+}{
+	{name: "protoc-gen-connect-query", packagePath: "@connectrpc/protoc-gen-connect-query"},
+	{name: "protoc-gen-es", packagePath: "@bufbuild/protoc-gen-es"},
+	{name: "tsx", packagePath: "tsx"},
+}
+
+type protoOutputState struct {
+	mode       fs.FileMode
+	digest     [sha256.Size]byte
+	linkTarget string
+}
+
+func checkProtoGeneratedFreshness(repoRoot string) (protoFreshnessResult, error) {
+	result := protoFreshnessResult{}
+	if !samePath(repoRoot, blockNinjaRepoRoot()) {
+		return result, nil
+	}
+	if !fileExists(filepath.Join(repoRoot, "Makefile")) || !fileExists(filepath.Join(repoRoot, "buf.gen.yaml")) {
+		return result, nil
+	}
+
+	return checkProtoGeneratedFreshnessInSandbox(repoRoot, "")
+}
+
+// checkProtoGeneratedFreshnessInSandbox runs generation against a private copy
+// of its inputs and outputs. Each invocation owns its sandbox, so parallel
+// checker processes never serialize on or mutate the scanned repository. An
+// interrupted generator can leave only disposable state beneath the system
+// temporary directory.
+func checkProtoGeneratedFreshnessInSandbox(repoRoot, tempParent string) (result protoFreshnessResult, returnErr error) {
+	result.checked = true
+
+	sandboxRoot, err := os.MkdirTemp(tempParent, "check-safety-proto-*")
+	if err != nil {
+		return result, fmt.Errorf("create proto freshness sandbox: %w", err)
+	}
+	defer func() {
+		if cleanupErr := os.RemoveAll(sandboxRoot); cleanupErr != nil {
+			returnErr = errors.Join(returnErr, fmt.Errorf("remove proto freshness sandbox: %w", cleanupErr))
+		}
+	}()
+
+	if err := prepareProtoSandbox(repoRoot, sandboxRoot); err != nil {
+		return result, fmt.Errorf("prepare proto freshness sandbox: %w", err)
+	}
+
+	before, err := snapshotProtoOutputs(sandboxRoot)
+	if err != nil {
+		return result, fmt.Errorf("snapshot generated proto outputs before generation: %w", err)
+	}
+
+	output, runErr := runCommand(sandboxRoot, "make", "proto")
+	result.output = output
+
+	after, snapshotErr := snapshotProtoOutputs(sandboxRoot)
+	if snapshotErr == nil {
+		result.afterStatus = strings.Join(diffProtoOutputs(before, after), "\n")
+	}
+
+	var generationErrs []error
+	if runErr != nil {
+		generationErrs = append(generationErrs, fmt.Errorf("make proto failed: %w", runErr))
+	}
+	if snapshotErr != nil {
+		generationErrs = append(generationErrs, fmt.Errorf("snapshot generated proto outputs after generation: %w", snapshotErr))
+	}
+	if len(generationErrs) > 0 {
+		return result, errors.Join(generationErrs...)
+	}
+
+	return result, nil
+}
+
+func prepareProtoSandbox(repoRoot, sandboxRoot string) error {
+	for _, relPath := range protoSandboxPaths {
+		if err := copyProtoSandboxPath(repoRoot, sandboxRoot, relPath); err != nil {
+			return fmt.Errorf("copy %s: %w", relPath, err)
+		}
+	}
+
+	return linkProtoSandboxTools(repoRoot, sandboxRoot)
+}
+
+func copyProtoSandboxPath(repoRoot, sandboxRoot, relPath string) error {
+	sourcePath := filepath.Join(repoRoot, relPath)
+	if _, err := os.Lstat(sourcePath); err != nil {
+		if errors.Is(err, os.ErrNotExist) {
+			return nil
+		}
+		return err
+	}
+
+	return filepath.WalkDir(sourcePath, func(path string, entry fs.DirEntry, walkErr error) error {
+		if walkErr != nil {
+			return walkErr
+		}
+
+		rel, err := filepath.Rel(repoRoot, path)
+		if err != nil {
+			return fmt.Errorf("resolve relative path for %s: %w", path, err)
+		}
+		if entry.IsDir() && (rel == filepath.Join("packages", "api", "node_modules") || rel == filepath.Join("packages", "api", "dist")) {
+			return filepath.SkipDir
+		}
+
+		destinationPath := filepath.Join(sandboxRoot, rel)
+		info, err := entry.Info()
+		if err != nil {
+			return fmt.Errorf("inspect %s: %w", path, err)
+		}
+
+		switch {
+		case info.IsDir():
+			if err := os.MkdirAll(destinationPath, info.Mode().Perm()); err != nil {
+				return fmt.Errorf("create directory %s: %w", destinationPath, err)
+			}
+			return nil
+		case info.Mode().IsRegular():
+			if err := copyFile(path, destinationPath, info.Mode()); err != nil {
+				return fmt.Errorf("copy file %s: %w", path, err)
+			}
+			return nil
+		case info.Mode()&fs.ModeSymlink != 0:
+			target, err := os.Readlink(path)
+			if err != nil {
+				return fmt.Errorf("read symlink %s: %w", path, err)
+			}
+			if err := os.MkdirAll(filepath.Dir(destinationPath), 0o755); err != nil {
+				return fmt.Errorf("create symlink parent for %s: %w", destinationPath, err)
+			}
+			if err := os.Symlink(target, destinationPath); err != nil {
+				return fmt.Errorf("copy symlink %s: %w", path, err)
+			}
+			return nil
+		default:
+			return fmt.Errorf("unsupported file type %s at %s", info.Mode().Type(), path)
+		}
+	})
+}
+
+func linkProtoSandboxTools(repoRoot, sandboxRoot string) error {
+	sourceBinDir := filepath.Join(repoRoot, "packages", "api", "node_modules", ".bin")
+	destinationBinDir := filepath.Join(sandboxRoot, "packages", "api", "node_modules", ".bin")
+
+	for _, tool := range protoSandboxTools {
+		sourcePath := filepath.Join(sourceBinDir, tool.name)
+		if _, err := os.Lstat(sourcePath); err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				continue
+			}
+			return fmt.Errorf("inspect proto tool %s: %w", tool.name, err)
+		}
+		if err := os.MkdirAll(destinationBinDir, 0o755); err != nil {
+			return fmt.Errorf("create proto tool directory: %w", err)
+		}
+		if err := os.Symlink(sourcePath, filepath.Join(destinationBinDir, tool.name)); err != nil {
+			return fmt.Errorf("link proto tool %s: %w", tool.name, err)
+		}
+
+		sourcePackagePath := filepath.Join(repoRoot, "packages", "api", "node_modules", filepath.FromSlash(tool.packagePath))
+		if _, err := os.Lstat(sourcePackagePath); err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				continue
+			}
+			return fmt.Errorf("inspect proto tool package %s: %w", tool.packagePath, err)
+		}
+		destinationPackagePath := filepath.Join(sandboxRoot, "packages", "api", "node_modules", filepath.FromSlash(tool.packagePath))
+		if err := os.MkdirAll(filepath.Dir(destinationPackagePath), 0o755); err != nil {
+			return fmt.Errorf("create proto tool package directory for %s: %w", tool.packagePath, err)
+		}
+		if err := os.Symlink(sourcePackagePath, destinationPackagePath); err != nil {
+			return fmt.Errorf("link proto tool package %s: %w", tool.packagePath, err)
+		}
+	}
+
+	return nil
+}
+
+func snapshotProtoOutputs(root string) (map[string]protoOutputState, error) {
+	states := make(map[string]protoOutputState)
+	for _, generatedPath := range protoGeneratedPaths {
+		absolutePath := filepath.Join(root, generatedPath)
+		if _, err := os.Lstat(absolutePath); err != nil {
+			if errors.Is(err, os.ErrNotExist) {
+				continue
+			}
+			return nil, fmt.Errorf("inspect %s: %w", generatedPath, err)
+		}
+
+		err := filepath.WalkDir(absolutePath, func(path string, entry fs.DirEntry, walkErr error) error {
+			if walkErr != nil {
+				return walkErr
+			}
+			if entry.IsDir() {
+				return nil
+			}
+
+			rel, err := filepath.Rel(root, path)
+			if err != nil {
+				return fmt.Errorf("resolve generated output path %s: %w", path, err)
+			}
+			rel = filepath.ToSlash(rel)
+
+			info, err := entry.Info()
+			if err != nil {
+				return fmt.Errorf("inspect generated output %s: %w", rel, err)
+			}
+			state := protoOutputState{mode: info.Mode().Type() | info.Mode().Perm()}
+			switch {
+			case info.Mode().IsRegular():
+				content, err := os.ReadFile(path)
+				if err != nil {
+					return fmt.Errorf("read generated output %s: %w", rel, err)
+				}
+				state.digest = sha256.Sum256(content)
+			case info.Mode()&fs.ModeSymlink != 0:
+				target, err := os.Readlink(path)
+				if err != nil {
+					return fmt.Errorf("read generated output symlink %s: %w", rel, err)
+				}
+				state.linkTarget = target
+			default:
+				return fmt.Errorf("unsupported generated output type %s at %s", info.Mode().Type(), rel)
+			}
+			states[rel] = state
+			return nil
+		})
+		if err != nil {
+			return nil, fmt.Errorf("walk generated outputs under %s: %w", generatedPath, err)
+		}
+	}
+
+	return states, nil
+}
+
+func diffProtoOutputs(before, after map[string]protoOutputState) []string {
+	pathSet := make(map[string]struct{}, len(before)+len(after))
+	for path := range before {
+		pathSet[path] = struct{}{}
+	}
+	for path := range after {
+		pathSet[path] = struct{}{}
+	}
+
+	paths := make([]string, 0, len(pathSet))
+	for path := range pathSet {
+		paths = append(paths, path)
+	}
+	sort.Strings(paths)
+
+	changes := make([]string, 0)
+	for _, path := range paths {
+		beforeState, existedBefore := before[path]
+		afterState, existsAfter := after[path]
+		switch {
+		case !existedBefore:
+			changes = append(changes, "?? "+path)
+		case !existsAfter:
+			changes = append(changes, " D "+path)
+		case beforeState != afterState:
+			changes = append(changes, " M "+path)
+		}
+	}
+
+	return changes
+}

proto_freshness_test.go

@@ -0,0 +1,178 @@
+package main
+
+import (
+	"crypto/sha256"
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"sync"
+	"testing"
+)
+
+const protoFreshnessHelperEnv = "CHECK_SAFETY_PROTO_FRESHNESS_HELPER"
+
+func TestDiffProtoOutputsIsDeterministic(t *testing.T) {
+	before := map[string]protoOutputState{
+		"packages/api/src/unchanged.ts": {digest: [sha256.Size]byte{1}},
+		"packages/api/src/modified.ts":  {digest: [sha256.Size]byte{2}},
+		"packages/api/src/removed.ts":   {digest: [sha256.Size]byte{3}},
+	}
+	after := map[string]protoOutputState{
+		"packages/api/src/unchanged.ts": {digest: [sha256.Size]byte{1}},
+		"packages/api/src/modified.ts":  {digest: [sha256.Size]byte{4}},
+		"packages/api/src/added.ts":     {digest: [sha256.Size]byte{5}},
+	}
+
+	want := strings.Join([]string{
+		"?? packages/api/src/added.ts",
+		" M packages/api/src/modified.ts",
+		" D packages/api/src/removed.ts",
+	}, "\n")
+	if got := strings.Join(diffProtoOutputs(before, after), "\n"); got != want {
+		t.Fatalf("diffProtoOutputs() = %q, want %q", got, want)
+	}
+}
+
+func TestCheckProtoGeneratedFreshnessInSandboxLeavesSourceUntouched(t *testing.T) {
+	repoRoot := newProtoFreshnessTestRepo(t, "@printf 'generated\\n' > packages/api/src/example.ts")
+	tempParent := t.TempDir()
+
+	result, err := checkProtoGeneratedFreshnessInSandbox(repoRoot, tempParent)
+	if err != nil {
+		t.Fatalf("checkProtoGeneratedFreshnessInSandbox() error = %v\n%s", err, result.output)
+	}
+	if !result.checked {
+		t.Fatal("checkProtoGeneratedFreshnessInSandbox() checked = false, want true")
+	}
+	if !result.changed() {
+		t.Fatal("checkProtoGeneratedFreshnessInSandbox() changed = false, want true")
+	}
+	if result.beforeStatus != "" {
+		t.Fatalf("beforeStatus = %q, want clean", result.beforeStatus)
+	}
+	if result.afterStatus != " M packages/api/src/example.ts" {
+		t.Fatalf("afterStatus = %q, want modified generated file", result.afterStatus)
+	}
+	assertProtoTestSourceAndSandboxesClean(t, repoRoot, tempParent)
+}
+
+func TestCheckProtoGeneratedFreshnessInSandboxReportsClean(t *testing.T) {
+	repoRoot := newProtoFreshnessTestRepo(t, "@:")
+	tempParent := t.TempDir()
+
+	result, err := checkProtoGeneratedFreshnessInSandbox(repoRoot, tempParent)
+	if err != nil {
+		t.Fatalf("checkProtoGeneratedFreshnessInSandbox() error = %v\n%s", err, result.output)
+	}
+	if result.changed() {
+		t.Fatalf("checkProtoGeneratedFreshnessInSandbox() changed = true, status = %q", result.afterStatus)
+	}
+	assertProtoTestSourceAndSandboxesClean(t, repoRoot, tempParent)
+}
+
+func TestCheckProtoGeneratedFreshnessInSandboxCleansUpAfterFailure(t *testing.T) {
+	repoRoot := newProtoFreshnessTestRepo(t, "@printf 'partial\\n' > packages/api/src/example.ts; printf 'generator failed\\n' >&2; exit 7")
+	tempParent := t.TempDir()
+
+	result, err := checkProtoGeneratedFreshnessInSandbox(repoRoot, tempParent)
+	if err == nil {
+		t.Fatal("checkProtoGeneratedFreshnessInSandbox() error = nil, want generator failure")
+	}
+	if !strings.Contains(err.Error(), "make proto failed") {
+		t.Fatalf("error = %q, want make proto context", err)
+	}
+	if !strings.Contains(result.output, "generator failed") {
+		t.Fatalf("output = %q, want generator stderr", result.output)
+	}
+	assertProtoTestSourceAndSandboxesClean(t, repoRoot, tempParent)
+}
+
+func TestCheckProtoGeneratedFreshnessConcurrentProcesses(t *testing.T) {
+	repoRoot := newProtoFreshnessTestRepo(t, "@mkdir generation-exclusive; sleep 0.05; printf 'generated in %s\\n' \"$$PWD\" > packages/api/src/example.ts")
+	tempParent := t.TempDir()
+	testBinary, err := os.Executable()
+	if err != nil {
+		t.Fatalf("resolve test executable: %v", err)
+	}
+
+	const processCount = 6
+	type processResult struct {
+		index  int
+		output string
+		err    error
+	}
+	results := make(chan processResult, processCount)
+
+	var workers sync.WaitGroup
+	for index := range processCount {
+		workers.Go(func() {
+			cmd := exec.CommandContext(t.Context(), testBinary, "-test.run=^TestProtoFreshnessHelperProcess$")
+			cmd.Env = append(os.Environ(),
+				protoFreshnessHelperEnv+"=1",
+				"CHECK_SAFETY_PROTO_REPO="+repoRoot,
+				"CHECK_SAFETY_PROTO_TEMP_PARENT="+tempParent,
+			)
+			output, runErr := cmd.CombinedOutput()
+			results <- processResult{index: index, output: string(output), err: runErr}
+		})
+	}
+	workers.Wait()
+	close(results)
+
+	for result := range results {
+		if result.err != nil {
+			t.Errorf("checker process %d failed: %v\n%s", result.index, result.err, result.output)
+		}
+	}
+	assertProtoTestSourceAndSandboxesClean(t, repoRoot, tempParent)
+}
+
+func TestProtoFreshnessHelperProcess(t *testing.T) {
+	if os.Getenv(protoFreshnessHelperEnv) != "1" {
+		return
+	}
+
+	repoRoot := os.Getenv("CHECK_SAFETY_PROTO_REPO")
+	tempParent := os.Getenv("CHECK_SAFETY_PROTO_TEMP_PARENT")
+	result, err := checkProtoGeneratedFreshnessInSandbox(repoRoot, tempParent)
+	if err != nil {
+		t.Fatalf("checkProtoGeneratedFreshnessInSandbox() error = %v\n%s", err, result.output)
+	}
+	if !result.changed() {
+		t.Fatal("checkProtoGeneratedFreshnessInSandbox() changed = false, want true")
+	}
+}
+
+func newProtoFreshnessTestRepo(t *testing.T, recipe string) string {
+	t.Helper()
+	if _, err := exec.LookPath("make"); err != nil {
+		t.Skip("make is required for proto freshness integration coverage")
+	}
+
+	repoRoot := t.TempDir()
+	writeTestFile(t, filepath.Join(repoRoot, "Makefile"), ".PHONY: proto\nproto:\n\t"+recipe+"\n", 0o644)
+	writeTestFile(t, filepath.Join(repoRoot, "buf.gen.yaml"), "version: v2\n", 0o644)
+	writeTestFile(t, filepath.Join(repoRoot, "packages", "api", "src", "example.ts"), "source\n", 0o644)
+	return repoRoot
+}
+
+func assertProtoTestSourceAndSandboxesClean(t *testing.T, repoRoot, tempParent string) {
+	t.Helper()
+
+	content, err := os.ReadFile(filepath.Join(repoRoot, "packages", "api", "src", "example.ts"))
+	if err != nil {
+		t.Fatalf("read source generated file: %v", err)
+	}
+	if string(content) != "source\n" {
+		t.Fatalf("source generated file = %q, want unchanged", content)
+	}
+
+	entries, err := os.ReadDir(tempParent)
+	if err != nil {
+		t.Fatalf("read sandbox parent: %v", err)
+	}
+	if len(entries) != 0 {
+		t.Fatalf("sandbox parent contains residual entries: %v", entries)
+	}
+}

proto_rbac.go

@@ -833,48 +833,6 @@ func isExcludedServiceMethod(method string) bool {
 	return excludedServices[svcName]
 }
 
-func checkProtoGeneratedFreshness(repoRoot string) (protoFreshnessResult, error) {
-	result := protoFreshnessResult{}
-	if !samePath(repoRoot, blockNinjaRepoRoot()) {
-		return result, nil
-	}
-	if !fileExists(filepath.Join(repoRoot, "Makefile")) || !fileExists(filepath.Join(repoRoot, "buf.gen.yaml")) {
-		return result, nil
-	}
-
-	result.checked = true
-	before, err := protoGeneratedStatus(repoRoot)
-	if err != nil {
-		return result, err
-	}
-	result.beforeStatus = before
-
-	output, runErr := runCommand(repoRoot, "make", "proto")
-	result.output = output
-
-	after, statusErr := protoGeneratedStatus(repoRoot)
-	if statusErr != nil {
-		return result, statusErr
-	}
-	result.afterStatus = after
-
-	if runErr != nil {
-		return result, fmt.Errorf("make proto failed: %w", runErr)
-	}
-	return result, nil
-}
-
 func (r protoFreshnessResult) changed() bool {
 	return r.checked && r.beforeStatus != r.afterStatus
 }
-
-func protoGeneratedStatus(repoRoot string) (string, error) {
-	paths := []string{
-		"backend/internal/api",
-		"backend/internal/mcpserver/generated",
-		"packages/api/src",
-	}
-	args := []string{"status", "--porcelain=v1", "--untracked-files=all", "--"}
-	args = append(args, paths...)
-	return runCommand(repoRoot, "git", args...)
-}