diff --git a/docs/adr/0003-isolate-proto-freshness-generation.md b/docs/adr/0003-isolate-proto-freshness-generation.md new file mode 100644 index 0000000..d74a0cd --- /dev/null +++ b/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 diff --git a/proto_freshness.go b/proto_freshness.go new file mode 100644 index 0000000..cafd39f --- /dev/null +++ b/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 +} diff --git a/proto_freshness_test.go b/proto_freshness_test.go new file mode 100644 index 0000000..0d22dc7 --- /dev/null +++ b/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) + } +} diff --git a/proto_rbac.go b/proto_rbac.go index f0e0a8f..ef01df8 100644 --- a/proto_rbac.go +++ b/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...) -}