Make run identities collision-resistant and outputs exclusive

2026-07-18 14:21:26 +00:00
parent a39eea7ed6
commit 2111e01142
9 changed files with 367 additions and 75 deletions
--- a/internal/cli/run.go
+++ b/internal/cli/run.go
@@ -37,6 +37,7 @@ type Options struct {
 	Catalog               pipeline.ModuleCatalog
 	Registries            pipeline.Registries
 	LLMClientFactory      LLMClientFactory
+	RunIDGenerator        RunIDGenerator
 	LookupEnv             func(string) (string, bool)
 	Now                   func() time.Time
 	UserCacheDir          func() (string, error)
@@ -91,6 +92,9 @@ func normalizeOptions(opts Options) (Options, error) {
 	if opts.Now == nil {
 		opts.Now = time.Now
 	}
+	if opts.RunIDGenerator == nil {
+		opts.RunIDGenerator = defaultRunIDGenerator
+	}
 	if opts.UserCacheDir == nil {
 		opts.UserCacheDir = os.UserCacheDir
 	}
@@ -213,17 +217,25 @@ func runPipelineCommand(args []string, stdout, stderr io.Writer, opts Options) i
 	}

 	startedAt := opts.Now().UTC()
-	runID := fmt.Sprintf("run-%d", startedAt.UnixNano())
+	runID, err := opts.RunIDGenerator(startedAt)
+	if err != nil {
+		fmt.Fprintf(stderr, "notarius: generate run ID: %v\n", err)
+		return 1
+	}
+	if err := validateRunID(runID); err != nil {
+		fmt.Fprintf(stderr, "notarius: invalid generated run ID: %v\n", err)
+		return 1
+	}
 	var summary *debugbundle.SummaryWriter
 	debugPath := ""
 	debugRecorder := pipeline.NoopDebugRecorder()
 	if *debug {
-		bundle, err := debugbundle.Allocate(cfg.Debug.Directory)
+		bundle, err := debugbundle.Allocate(cfg.Debug.Directory, runID, startedAt)
 		if err != nil {
 			fmt.Fprintf(stderr, "notarius: %v\n", err)
 			return 1
 		}
-		runID, debugPath, summary = bundle.RunID(), bundle.Path(), bundle.Summary()
+		debugPath, summary = bundle.Path(), bundle.Summary()
 		debugRecorder, err = opts.DebugRecorderFactory(bundle.TraceRoot())
 		if err != nil {
 			return failPipelineCommand(stderr, summary, debugPath, fmt.Errorf("create debug recorder: %w", err), true)
@@ -515,8 +527,15 @@ func writeOutputFiles(runOutputDir string, files []contracts.OutputFile) error {
 		targets = append(targets, outputTarget{path: targetPath, file: file})
 	}

-	if err := os.MkdirAll(runOutputDir, 0o755); err != nil {
-		return fmt.Errorf("create output directory %q: %w", runOutputDir, err)
+	outputParent := filepath.Dir(runOutputDir)
+	if err := os.MkdirAll(outputParent, 0o755); err != nil {
+		return fmt.Errorf("create output parent %q: %w", outputParent, err)
+	}
+	if err := os.Mkdir(runOutputDir, 0o755); err != nil {
+		if os.IsExist(err) {
+			return fmt.Errorf("output run directory %q already exists", runOutputDir)
+		}
+		return fmt.Errorf("create output run directory %q: %w", runOutputDir, err)
 	}
 	for _, target := range targets {
 		if err := os.MkdirAll(filepath.Dir(target.path), 0o755); err != nil {
--- a/internal/cli/run_id.go
+++ b/internal/cli/run_id.go
@@ -0,0 +1,34 @@
+package cli
+
+import (
+	"crypto/rand"
+	"encoding/hex"
+	"fmt"
+	"io"
+	"path/filepath"
+	"strings"
+	"time"
+)
+
+type RunIDGenerator func(time.Time) (string, error)
+
+func defaultRunIDGenerator(startedAt time.Time) (string, error) {
+	var suffix [16]byte
+	if _, err := io.ReadFull(rand.Reader, suffix[:]); err != nil {
+		return "", fmt.Errorf("read random run ID suffix: %w", err)
+	}
+	return fmt.Sprintf("run-%d-%s", startedAt.UnixNano(), hex.EncodeToString(suffix[:])), nil
+}
+
+func validateRunID(runID string) error {
+	if runID == "" {
+		return fmt.Errorf("run ID must not be empty")
+	}
+	if runID != strings.TrimSpace(runID) {
+		return fmt.Errorf("run ID %q must not have surrounding whitespace", runID)
+	}
+	if strings.ContainsAny(runID, `/\\`) || filepath.IsAbs(runID) || filepath.Clean(runID) != runID || runID == "." || runID == ".." {
+		return fmt.Errorf("run ID %q must be one safe path component", runID)
+	}
+	return nil
+}
--- a/internal/cli/run_id_test.go
+++ b/internal/cli/run_id_test.go
@@ -0,0 +1,87 @@
+package cli
+
+import (
+	"os"
+	"path/filepath"
+	"regexp"
+	"strings"
+	"testing"
+	"time"
+
+	"gitea.maximumdirect.net/eric/notarius/internal/framework/contracts"
+)
+
+func TestDefaultRunIDGeneratorProducesUniqueSafeIDs(t *testing.T) {
+	startedAt := time.Unix(0, 123456789).UTC()
+	pattern := regexp.MustCompile(`^run-123456789-[0-9a-f]{32}$`)
+	seen := make(map[string]struct{}, 256)
+	for i := 0; i < 256; i++ {
+		runID, err := defaultRunIDGenerator(startedAt)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if !pattern.MatchString(runID) {
+			t.Fatalf("run ID %q does not match production format", runID)
+		}
+		if err := validateRunID(runID); err != nil {
+			t.Fatalf("run ID %q is not path-safe: %v", runID, err)
+		}
+		if _, exists := seen[runID]; exists {
+			t.Fatalf("duplicate run ID %q", runID)
+		}
+		seen[runID] = struct{}{}
+	}
+}
+
+func TestWriteOutputFilesSupportsNestedLogicalPaths(t *testing.T) {
+	runPath := filepath.Join(t.TempDir(), "output", "run-safe")
+	if err := writeOutputFiles(runPath, []contracts.OutputFile{{Name: "nested/result.json", Bytes: []byte("result")}}); err != nil {
+		t.Fatal(err)
+	}
+	data, err := os.ReadFile(filepath.Join(runPath, "nested", "result.json"))
+	if err != nil || string(data) != "result" {
+		t.Fatalf("nested output = %q, %v", data, err)
+	}
+}
+
+func TestWriteOutputFilesRejectsUnsafeNamesBeforeAllocatingRunDirectory(t *testing.T) {
+	outputRoot := filepath.Join(t.TempDir(), "output")
+	runPath := filepath.Join(outputRoot, "run-safe")
+	for _, name := range []string{"", "../outside", "/absolute", `nested\\outside`, "nested/../outside"} {
+		t.Run(name, func(t *testing.T) {
+			if err := writeOutputFiles(runPath, []contracts.OutputFile{{Name: "safe.json"}, {Name: name}}); err == nil {
+				t.Fatalf("writeOutputFiles accepted %q", name)
+			}
+			if _, err := os.Stat(outputRoot); !os.IsNotExist(err) {
+				t.Fatalf("output root exists or stat failed after %q: %v", name, err)
+			}
+		})
+	}
+}
+
+func TestWriteOutputFilesRetainsNewPartialDirectoryAndPreservesSibling(t *testing.T) {
+	outputRoot := filepath.Join(t.TempDir(), "output")
+	siblingPath := filepath.Join(outputRoot, "sibling")
+	if err := os.MkdirAll(siblingPath, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	sentinelPath := filepath.Join(siblingPath, "sentinel")
+	if err := os.WriteFile(sentinelPath, []byte("preserve sibling"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+
+	runPath := filepath.Join(outputRoot, "run-safe")
+	err := writeOutputFiles(runPath, []contracts.OutputFile{
+		{Name: "blocked", Bytes: []byte("partial output")},
+		{Name: "blocked/nested.json", Bytes: []byte("unreachable")},
+	})
+	if err == nil || !strings.Contains(err.Error(), "create output directory") {
+		t.Fatalf("writeOutputFiles() error = %v, want later directory failure", err)
+	}
+	if got, err := os.ReadFile(filepath.Join(runPath, "blocked")); err != nil || string(got) != "partial output" {
+		t.Fatalf("partial output = %q, %v", got, err)
+	}
+	if got, err := os.ReadFile(sentinelPath); err != nil || string(got) != "preserve sibling" {
+		t.Fatalf("sibling sentinel = %q, %v", got, err)
+	}
+}
--- a/internal/cli/state_hardening_test.go
+++ b/internal/cli/state_hardening_test.go
@@ -3,6 +3,7 @@ package cli
 import (
 	"bytes"
 	"context"
+	"encoding/json"
 	"errors"
 	"fmt"
 	"os"
@@ -166,7 +167,7 @@ func TestRunRetainsDebugBundlesAcrossFailures(t *testing.T) {
 			h.extractErr = errors.New("synthetic extraction failure")
 			return h.options()
 		}},
-		{"output", "create output directory", func(t *testing.T, roots stateTestRoots, h *stateTestHarness) Options {
+		{"output", "create output parent", func(t *testing.T, roots stateTestRoots, h *stateTestHarness) Options {
 			if err := os.WriteFile(roots.output, []byte("not a directory"), 0o600); err != nil {
 				t.Fatal(err)
 			}
@@ -291,6 +292,135 @@ func TestRunRedactsSensitiveModuleOptionsFromConfigAndPipelineSummaries(t *testi
 	}
 }

+func TestRunUsesOneInjectedIdentityForDebugOutputAndManifest(t *testing.T) {
+	roots := newStateTestRoots(t)
+	harness := newStateTestHarness()
+	opts := harness.options()
+	const runID = "run-1000000000-11111111111111111111111111111111"
+	opts.RunIDGenerator = func(time.Time) (string, error) { return runID, nil }
+
+	result := runStateTest(t, roots, opts, true, false, "bypass")
+	if result.code != 0 {
+		t.Fatalf("code=%d stderr=%q", result.code, result.stderr)
+	}
+	outputPath := filepath.Join(roots.output, runID)
+	debugPath := filepath.Join(roots.debug, runID)
+	assertFile(t, filepath.Join(outputPath, "result.json"))
+	assertFile(t, filepath.Join(debugPath, "summary", "run-manifest.json"))
+	if !strings.Contains(result.stdout, "output="+outputPath) || !strings.Contains(result.stdout, "debug="+debugPath) {
+		t.Fatalf("stdout=%q, want shared run identity", result.stdout)
+	}
+	data, err := os.ReadFile(filepath.Join(debugPath, "summary", "run-manifest.json"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	var manifest artifacts.RunManifest
+	if err := json.Unmarshal(data, &manifest); err != nil {
+		t.Fatal(err)
+	}
+	if manifest.RunID != runID {
+		t.Fatalf("manifest run ID = %q, want %q", manifest.RunID, runID)
+	}
+}
+
+func TestRunRefusesExistingOutputDirectoryWithoutChangingIt(t *testing.T) {
+	roots := newStateTestRoots(t)
+	const runID = "run-1000000000-22222222222222222222222222222222"
+	runPath := filepath.Join(roots.output, runID)
+	if err := os.MkdirAll(filepath.Join(runPath, "nested"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(runPath, "sentinel"), []byte("existing output"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(runPath, "nested", "data"), []byte("preserve me"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	before := readTree(t, runPath)
+	opts := newStateTestHarness().options()
+	opts.RunIDGenerator = func(time.Time) (string, error) { return runID, nil }
+
+	result := runStateTest(t, roots, opts, false, false, "bypass")
+	if result.code != 1 || !strings.Contains(result.stderr, "output run directory") || !strings.Contains(result.stderr, "already exists") {
+		t.Fatalf("code=%d stderr=%q", result.code, result.stderr)
+	}
+	if after := readTree(t, runPath); !sameFiles(after, before) {
+		t.Fatalf("existing output changed: before=%v after=%v", before, after)
+	}
+}
+
+func TestRepeatedRunIdentityCannotOverwriteFirstOutput(t *testing.T) {
+	roots := newStateTestRoots(t)
+	const runID = "run-1000000000-33333333333333333333333333333333"
+	harness := newStateTestHarness()
+	opts := harness.options()
+	opts.RunIDGenerator = func(time.Time) (string, error) { return runID, nil }
+
+	first := runStateTest(t, roots, opts, false, false, "bypass")
+	if first.code != 0 {
+		t.Fatalf("first code=%d stderr=%q", first.code, first.stderr)
+	}
+	runPath := filepath.Join(roots.output, runID)
+	before := readTree(t, runPath)
+	second := runStateTest(t, roots, opts, false, false, "bypass")
+	if second.code != 1 || !strings.Contains(second.stderr, "already exists") {
+		t.Fatalf("second code=%d stderr=%q", second.code, second.stderr)
+	}
+	if after := readTree(t, runPath); !sameFiles(after, before) {
+		t.Fatalf("first output changed: before=%v after=%v", before, after)
+	}
+}
+
+func TestRunRefusesExistingDebugBundleWithoutChangingIt(t *testing.T) {
+	roots := newStateTestRoots(t)
+	const runID = "run-1000000000-44444444444444444444444444444444"
+	bundlePath := filepath.Join(roots.debug, runID)
+	if err := os.MkdirAll(bundlePath, 0o700); err != nil {
+		t.Fatal(err)
+	}
+	sentinelPath := filepath.Join(bundlePath, "sentinel")
+	if err := os.WriteFile(sentinelPath, []byte("existing debug"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	opts := newStateTestHarness().options()
+	opts.RunIDGenerator = func(time.Time) (string, error) { return runID, nil }
+
+	result := runStateTest(t, roots, opts, true, false, "bypass")
+	if result.code != 1 || !strings.Contains(result.stderr, "debug bundle") || !strings.Contains(result.stderr, "already exists") {
+		t.Fatalf("code=%d stderr=%q", result.code, result.stderr)
+	}
+	if got, err := os.ReadFile(sentinelPath); err != nil || string(got) != "existing debug" {
+		t.Fatalf("sentinel = %q, %v", got, err)
+	}
+	assertAbsent(t, roots.output)
+}
+
+func TestRunIDGenerationFailurePrecedesDebugAllocation(t *testing.T) {
+	roots := newStateTestRoots(t)
+	opts := newStateTestHarness().options()
+	opts.RunIDGenerator = func(time.Time) (string, error) { return "", errors.New("random source unavailable") }
+
+	result := runStateTest(t, roots, opts, true, false, "bypass")
+	if result.code != 1 || !strings.Contains(result.stderr, "generate run ID: random source unavailable") {
+		t.Fatalf("code=%d stderr=%q", result.code, result.stderr)
+	}
+	assertAbsent(t, roots.debug)
+	assertAbsent(t, roots.output)
+}
+
+func TestRunRejectsUnsafeGeneratedIdentityBeforePathUse(t *testing.T) {
+	roots := newStateTestRoots(t)
+	opts := newStateTestHarness().options()
+	opts.RunIDGenerator = func(time.Time) (string, error) { return "../outside", nil }
+
+	result := runStateTest(t, roots, opts, true, false, "bypass")
+	if result.code != 1 || !strings.Contains(result.stderr, "invalid generated run ID") || !strings.Contains(result.stderr, "one safe path component") {
+		t.Fatalf("code=%d stderr=%q", result.code, result.stderr)
+	}
+	assertAbsent(t, roots.debug)
+	assertAbsent(t, roots.output)
+}
+
 type stateTestRoots struct{ config, input, output, plans, checkpoints, debug string }

 func newStateTestRoots(t *testing.T) stateTestRoots {
@@ -456,6 +586,7 @@ func sameFiles(left, right map[string][]byte) bool {
 type stateTestHarness struct {
 	mu                       sync.Mutex
 	chunkCalls, extractCalls int
+	runIDCalls               uint64
 	extractErr               error
 }

@@ -483,7 +614,12 @@ func (h *stateTestHarness) options() Options {
 	if err := registries.Outputs.RegisterWithSpec(pipeline.ModuleSpec{Key: "test/output", Stage: pipeline.StageOutput, Requires: []string{"normalized"}, Provides: []string{"output"}}, func() (contracts.OutputEncoder, error) { return stateTestOutput{}, nil }); err != nil {
 		panic(err)
 	}
-	return Options{Catalog: catalogFromRegistries(registries), Registries: registries, LookupEnv: emptyLookup, Now: func() time.Time { return time.Unix(1, 0) }, UserCacheDir: func() (string, error) { return "", errors.New("unexpected user cache lookup") }, LLMClientFactory: func(context.Context, config.Config, string) (contracts.StructuredLLMClient, []artifacts.LLMProfileManifest, error) {
+	return Options{Catalog: catalogFromRegistries(registries), Registries: registries, LookupEnv: emptyLookup, Now: func() time.Time { return time.Unix(1, 0) }, RunIDGenerator: func(startedAt time.Time) (string, error) {
+		h.mu.Lock()
+		defer h.mu.Unlock()
+		h.runIDCalls++
+		return fmt.Sprintf("run-%d-%032x", startedAt.UnixNano(), h.runIDCalls), nil
+	}, UserCacheDir: func() (string, error) { return "", errors.New("unexpected user cache lookup") }, LLMClientFactory: func(context.Context, config.Config, string) (contracts.StructuredLLMClient, []artifacts.LLMProfileManifest, error) {
 		return nil, nil, nil
 	}}
 }
--- a/internal/cli/state_surfaces_test.go
+++ b/internal/cli/state_surfaces_test.go
@@ -6,8 +6,13 @@ import (
 	"path/filepath"
 	"strings"
 	"testing"
+	"time"
 )

+const stateSurfaceRunID = "run-1000000000-55555555555555555555555555555555"
+
+func stateSurfaceRunIDGenerator(time.Time) (string, error) { return stateSurfaceRunID, nil }
+
 func TestRunRejectsDebugDirectoryWithoutDebug(t *testing.T) {
 	var stdout, stderr bytes.Buffer
 	code := RunWithOptions([]string{"run", "example", "--input", "source.json", "--debug-dir", t.TempDir()}, &stdout, &stderr, Options{})
@@ -20,7 +25,7 @@ func TestRunDebugAllocatesBeforePipelineResolution(t *testing.T) {
 	root := t.TempDir()
 	configPath := writeV3Config(t, "")
 	var stdout, stderr bytes.Buffer
-	code := RunWithOptions([]string{"run", "missing", "--config", configPath, "--input", "source.json", "--debug", "--debug-dir", root, "--chunk_cache", "bypass"}, &stdout, &stderr, Options{LookupEnv: emptyLookup})
+	code := RunWithOptions([]string{"run", "missing", "--config", configPath, "--input", "source.json", "--debug", "--debug-dir", root, "--chunk_cache", "bypass"}, &stdout, &stderr, Options{LookupEnv: emptyLookup, RunIDGenerator: stateSurfaceRunIDGenerator})
 	if code != 1 {
 		t.Fatalf("code=%d stderr=%q", code, stderr.String())
 	}
@@ -28,7 +33,10 @@ func TestRunDebugAllocatesBeforePipelineResolution(t *testing.T) {
 	if err != nil || len(entries) != 1 {
 		t.Fatalf("debug bundles: %v, %v", entries, err)
 	}
-	bundle := filepath.Join(root, entries[0].Name())
+	if entries[0].Name() != stateSurfaceRunID {
+		t.Fatalf("debug bundle name = %q, want %q", entries[0].Name(), stateSurfaceRunID)
+	}
+	bundle := filepath.Join(root, stateSurfaceRunID)
 	for _, name := range []string{"summary", "trace"} {
 		if info, err := os.Stat(filepath.Join(bundle, name)); err != nil || !info.IsDir() {
 			t.Fatalf("%s: %v", name, err)
@@ -49,7 +57,7 @@ func TestRunWithoutDebugDoesNotAllocateDebugRoot(t *testing.T) {
 		}
 		return "", false
 	}
-	code := RunWithOptions([]string{"run", "missing", "--config", configPath, "--input", "source.json", "--chunk_cache", "bypass"}, &stdout, &stderr, Options{LookupEnv: lookup})
+	code := RunWithOptions([]string{"run", "missing", "--config", configPath, "--input", "source.json", "--chunk_cache", "bypass"}, &stdout, &stderr, Options{LookupEnv: lookup, RunIDGenerator: stateSurfaceRunIDGenerator})
 	if code != 1 {
 		t.Fatalf("code=%d stderr=%q", code, stderr.String())
 	}