From 7de41eb3bd6d5ce6731441545a5a98512ef0c83b Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 4 Jul 2026 01:11:57 +0000 Subject: [PATCH] Write run outputs and diagnostics --- internal/cli/run.go | 231 +++++++++++++-- internal/cli/run_test.go | 333 +++++++++++++++++++++- internal/core/diagnostics/run_dir.go | 40 ++- internal/core/diagnostics/run_dir_test.go | 18 ++ 4 files changed, 595 insertions(+), 27 deletions(-) diff --git a/internal/cli/run.go b/internal/cli/run.go index d9705b6..5aee8df 100644 --- a/internal/cli/run.go +++ b/internal/cli/run.go @@ -7,17 +7,21 @@ import ( "fmt" "io" "os" + "path" + "path/filepath" "sort" "strings" "time" "gitea.maximumdirect.net/eric/notarius/internal/core/artifacts" "gitea.maximumdirect.net/eric/notarius/internal/core/config" + "gitea.maximumdirect.net/eric/notarius/internal/core/diagnostics" "gitea.maximumdirect.net/eric/notarius/internal/framework/contracts" "gitea.maximumdirect.net/eric/notarius/internal/framework/pipeline" ) const defaultConfigPath = "/usr/local/etc/notarius/config.yml" +const defaultOutputRoot = "./notarius-output" const usage = `Usage: notarius help @@ -118,7 +122,7 @@ func runPipelineCommand(args []string, stdout, stderr io.Writer, opts Options) i return 2 } - cfg, _, err := loadConfig(*configPath, opts) + cfg, loadedConfigPath, err := loadConfig(*configPath, opts) if err != nil { fmt.Fprintf(stderr, "notarius: %v\n", err) return 1 @@ -127,11 +131,30 @@ func runPipelineCommand(args []string, stdout, stderr io.Writer, opts Options) i cfg.Diagnostics.WorkDir = dir } - catalog, err := effectiveCatalog(opts) + startedAt := opts.Now().UTC() + runDir, err := diagnostics.NewRunDirectory(cfg.Diagnostics.WorkDir, cfg.Diagnostics.Retention) if err != nil { fmt.Fprintf(stderr, "notarius: %v\n", err) return 1 } + invocation := diagnostics.InvocationMetadata{ + Operation: "run", + PipelineID: pipelineID, + InputPath: strings.TrimSpace(*inputPath), + ConfigPath: loadedConfigPath, + ConfigSource: configSource(*configPath), + OnlyLanes: append([]string(nil), only...), + RunID: runDir.RunID(), + StartedAt: startedAt, + } + if err := runDir.WriteInvocationMetadata(invocation); err != nil { + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("write diagnostics invocation metadata: %w", err)) + } + + catalog, err := effectiveCatalog(opts) + if err != nil { + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, err) + } effective, err := cfg.Resolve(config.ResolveInput{ PipelineID: pipelineID, Only: only, @@ -139,33 +162,38 @@ func runPipelineCommand(args []string, stdout, stderr io.Writer, opts Options) i LLMProfileOverride: *llmProfile, }) if err != nil { - fmt.Fprintf(stderr, "notarius: %v\n", err) - return 1 + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, err) + } + invocation.PipelineDigest = effective.ResolvedPipeline.Digest + if err := runDir.WriteInvocationMetadata(invocation); err != nil { + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("write diagnostics invocation metadata: %w", err)) + } + if err := runDir.WriteRedactedEffectiveConfig(effective); err != nil { + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("write diagnostics effective config: %w", err)) + } + if err := runDir.WriteResolvedPipeline(effective.ResolvedPipeline); err != nil { + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("write diagnostics resolved pipeline: %w", err)) } profileIDs := effectiveLLMProfileIDs(effective.ResolvedPipeline) if len(profileIDs) != 1 { - fmt.Fprintf(stderr, "notarius: pipeline %q uses %d distinct LLM profiles; current runs require exactly one: %s\n", pipelineID, len(profileIDs), strings.Join(profileIDs, ", ")) - return 1 + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("pipeline %q uses %d distinct LLM profiles; current runs require exactly one: %s", pipelineID, len(profileIDs), strings.Join(profileIDs, ", "))) } rawInput, err := os.ReadFile(strings.TrimSpace(*inputPath)) if err != nil { - fmt.Fprintf(stderr, "notarius: read input %q: %v\n", strings.TrimSpace(*inputPath), err) - return 1 + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("read input %q: %w", strings.TrimSpace(*inputPath), err)) } registries, err := effectiveRegistries(opts) if err != nil { - fmt.Fprintf(stderr, "notarius: %v\n", err) - return 1 + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, err) } ctx := context.Background() llmClient, llmProfiles, err := opts.LLMClientFactory(ctx, effective.Config, profileIDs[0]) if err != nil { - fmt.Fprintf(stderr, "notarius: create LLM client for profile %q: %v\n", profileIDs[0], err) - return 1 + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("create LLM client for profile %q: %w", profileIDs[0], err)) } output, err := pipeline.New(registries).Run(ctx, pipeline.RunInput{ @@ -173,22 +201,193 @@ func runPipelineCommand(args []string, stdout, stderr io.Writer, opts Options) i Path: strings.TrimSpace(*inputPath), RawInput: rawInput, LLMClient: llmClient, - StartedAt: opts.Now().UTC(), + RunID: runDir.RunID(), + StartedAt: startedAt, LLMProfiles: llmProfiles, Metadata: runMetadata(*outputDir, *diagnosticsDir), }) if err != nil { - fmt.Fprintf(stderr, "notarius: run pipeline %q: %v\n", pipelineID, err) - return 1 + if output.Manifest.PipelineID != "" { + _ = runDir.WriteRunManifest(output.Manifest) + } + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("run pipeline %q: %w", pipelineID, err)) } - fmt.Fprintf(stdout, "pipeline %q complete: approved=%d rejected=%d\n", effective.PipelineID, len(output.Approved), len(output.Rejected)) + runOutputDir := filepath.Join(outputRoot(*outputDir), runDir.RunID()) + if err := writeOutputFiles(runOutputDir, output.OutputFiles); err != nil { + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, err) + } + if err := runDir.WriteRunManifest(output.Manifest); err != nil { + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("write diagnostics run manifest: %w", err)) + } + if err := runDir.WriteWarnings(output.Warnings); err != nil { + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("write diagnostics warnings: %w", err)) + } + if err := runDir.WriteRunReport(runReport{ + RunID: runDir.RunID(), + PipelineID: effective.PipelineID, + OutputPath: runOutputDir, + DiagnosticsPath: runDir.Path(), + ApprovedCount: len(output.Approved), + RejectedCount: len(output.Rejected), + WarningCount: len(output.Warnings), + ValidationStatus: output.Manifest.ValidationStatus, + }); err != nil { + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("write diagnostics run report: %w", err)) + } + if err := runDir.ApplyRetention(diagnostics.RetentionDecisionInput{ + RetentionMode: cfg.Diagnostics.Retention, + RunSucceeded: true, + HasWarnings: len(output.Warnings) > 0, + }); err != nil { + return failPipelineCommand(stderr, runDir, cfg.Diagnostics.Retention, fmt.Errorf("apply diagnostics retention: %w", err)) + } + + fmt.Fprintf(stdout, "pipeline %q complete: approved=%d rejected=%d output=%s\n", effective.PipelineID, len(output.Approved), len(output.Rejected), runOutputDir) if len(output.Warnings) > 0 { fmt.Fprintf(stderr, "notarius: run completed with %d warning(s)\n", len(output.Warnings)) } return 0 } +type runReport struct { + RunID string `json:"run_id"` + PipelineID string `json:"pipeline_id"` + OutputPath string `json:"output_path"` + DiagnosticsPath string `json:"diagnostics_path,omitempty"` + ApprovedCount int `json:"approved_count"` + RejectedCount int `json:"rejected_count"` + WarningCount int `json:"warning_count"` + ValidationStatus string `json:"validation_status,omitempty"` +} + +func failPipelineCommand(stderr io.Writer, runDir *diagnostics.RunDirectory, retention diagnostics.RetentionMode, err error) int { + fmt.Fprintf(stderr, "notarius: %v\n", err) + if runDir != nil { + if logErr := runDir.WriteErrorLog(err.Error()); logErr != nil { + fmt.Fprintf(stderr, "notarius: write diagnostics error log: %v\n", logErr) + } + if retentionErr := runDir.ApplyRetention(diagnostics.RetentionDecisionInput{ + RetentionMode: retention, + RunSucceeded: false, + }); retentionErr != nil { + fmt.Fprintf(stderr, "notarius: apply diagnostics retention: %v\n", retentionErr) + } + } + return 1 +} + +func configSource(configPath string) string { + if strings.TrimSpace(configPath) != "" { + return "flag" + } + return "discovered" +} + +func outputRoot(outputDir string) string { + if dir := strings.TrimSpace(outputDir); dir != "" { + return dir + } + return defaultOutputRoot +} + +func writeOutputFiles(runOutputDir string, files []contracts.OutputFile) error { + type outputTarget struct { + path string + file contracts.OutputFile + } + targets := make([]outputTarget, 0, len(files)) + for _, file := range files { + targetPath, err := outputFilePath(runOutputDir, file.Name) + if err != nil { + return err + } + 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) + } + for _, target := range targets { + if err := os.MkdirAll(filepath.Dir(target.path), 0o755); err != nil { + return fmt.Errorf("create output directory %q: %w", filepath.Dir(target.path), err) + } + if err := writeFileAtomic(target.path, target.file.Bytes, 0o644); err != nil { + return fmt.Errorf("write output file %q: %w", target.file.Name, err) + } + } + return nil +} + +func outputFilePath(runOutputDir, logicalName string) (string, error) { + name := strings.TrimSpace(logicalName) + if name == "" { + return "", fmt.Errorf("output file name must not be empty") + } + if strings.Contains(name, `\`) { + return "", fmt.Errorf("output file name %q must use slash-separated relative paths", name) + } + if path.IsAbs(name) || filepath.IsAbs(name) { + return "", fmt.Errorf("output file name %q must be relative", name) + } + if strings.Contains(name, "..") { + return "", fmt.Errorf("output file name %q must not contain ..", name) + } + cleaned := path.Clean(name) + if cleaned == "." || cleaned != name { + return "", fmt.Errorf("output file name %q must be clean", name) + } + + root, err := filepath.Abs(runOutputDir) + if err != nil { + return "", fmt.Errorf("resolve output directory %q: %w", runOutputDir, err) + } + target, err := filepath.Abs(filepath.Join(root, filepath.FromSlash(cleaned))) + if err != nil { + return "", fmt.Errorf("resolve output file %q: %w", name, err) + } + rel, err := filepath.Rel(root, target) + if err != nil { + return "", fmt.Errorf("resolve output file %q: %w", name, err) + } + if rel == "." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || rel == ".." { + return "", fmt.Errorf("output file name %q resolves outside output directory", name) + } + return target, nil +} + +func writeFileAtomic(path string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(path) + temp, err := os.CreateTemp(dir, "."+filepath.Base(path)+".tmp-*") + if err != nil { + return err + } + tempPath := temp.Name() + removeTemp := true + defer func() { + if removeTemp { + _ = os.Remove(tempPath) + } + }() + + if _, err := temp.Write(data); err != nil { + _ = temp.Close() + return err + } + if err := temp.Chmod(perm); err != nil { + _ = temp.Close() + return err + } + if err := temp.Close(); err != nil { + return err + } + if err := os.Rename(tempPath, path); err != nil { + return err + } + removeTemp = false + return nil +} + func reorderRunArgs(args []string) []string { var flags []string var positionals []string diff --git a/internal/cli/run_test.go b/internal/cli/run_test.go index 427e21a..052536d 100644 --- a/internal/cli/run_test.go +++ b/internal/cli/run_test.go @@ -13,6 +13,7 @@ import ( "gitea.maximumdirect.net/eric/notarius/internal/core/artifacts" "gitea.maximumdirect.net/eric/notarius/internal/core/config" + "gitea.maximumdirect.net/eric/notarius/internal/core/diagnostics" "gitea.maximumdirect.net/eric/notarius/internal/framework/contracts" "gitea.maximumdirect.net/eric/notarius/internal/framework/pipeline" "gitea.maximumdirect.net/eric/notarius/internal/modules/chunk/generic" @@ -546,10 +547,11 @@ func TestRunPipelineRejectsUnknownFlag(t *testing.T) { func TestRunPipelineUnknownPipeline(t *testing.T) { configPath := writeTestConfig(t, mvpConfigYAML("dnd-session", "dnd/spells")) inputPath := writeSeriatimInput(t) + diagnosticsDir := t.TempDir() var stdout bytes.Buffer var stderr bytes.Buffer - code := RunWithOptions([]string{"run", "missing", "--config", configPath, "--input", inputPath}, &stdout, &stderr, Options{ + code := RunWithOptions([]string{"run", "missing", "--config", configPath, "--input", inputPath, "--diagnostics-dir", diagnosticsDir}, &stdout, &stderr, Options{ LLMClientFactory: fakeLLMFactory(newFakeRunLLMClient(false), nil), }) @@ -564,10 +566,11 @@ func TestRunPipelineUnknownPipeline(t *testing.T) { func TestRunPipelineUnknownOnlyLane(t *testing.T) { configPath := writeTestConfig(t, mvpConfigYAML("dnd-session", "dnd/spells")) inputPath := writeSeriatimInput(t) + diagnosticsDir := t.TempDir() var stdout bytes.Buffer var stderr bytes.Buffer - code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--only", "missing"}, &stdout, &stderr, Options{ + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--only", "missing", "--diagnostics-dir", diagnosticsDir}, &stdout, &stderr, Options{ LLMClientFactory: fakeLLMFactory(newFakeRunLLMClient(false), nil), }) @@ -582,10 +585,11 @@ func TestRunPipelineUnknownOnlyLane(t *testing.T) { func TestRunPipelineInvalidInputPath(t *testing.T) { configPath := writeTestConfig(t, mvpConfigYAML("dnd-session", "dnd/spells")) inputPath := filepath.Join(t.TempDir(), "missing.json") + diagnosticsDir := t.TempDir() var stdout bytes.Buffer var stderr bytes.Buffer - code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath}, &stdout, &stderr, Options{ + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--diagnostics-dir", diagnosticsDir}, &stdout, &stderr, Options{ LLMClientFactory: fakeLLMFactory(newFakeRunLLMClient(false), nil), }) @@ -600,11 +604,13 @@ func TestRunPipelineInvalidInputPath(t *testing.T) { func TestRunPipelineSuccessUsesProductionRegistriesAndFakeLLM(t *testing.T) { configPath := writeTestConfig(t, mvpConfigYAML("dnd-session", "dnd/spells")) inputPath := writeSeriatimInput(t) + outputDir := t.TempDir() + diagnosticsDir := t.TempDir() client := newFakeRunLLMClient(false) var stdout bytes.Buffer var stderr bytes.Buffer - code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath}, &stdout, &stderr, Options{ + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--output-dir", outputDir, "--diagnostics-dir", diagnosticsDir}, &stdout, &stderr, Options{ LLMClientFactory: fakeLLMFactory(client, nil), }) @@ -614,7 +620,7 @@ func TestRunPipelineSuccessUsesProductionRegistriesAndFakeLLM(t *testing.T) { if client.calls != 1 { t.Fatalf("LLM calls = %d, want 1", client.calls) } - for _, want := range []string{"dnd-session", "approved=1", "rejected=0"} { + for _, want := range []string{"dnd-session", "approved=1", "rejected=0", outputDir} { if !strings.Contains(stdout.String(), want) { t.Fatalf("stdout = %q, want substring %q", stdout.String(), want) } @@ -627,11 +633,13 @@ func TestRunPipelineSuccessUsesProductionRegistriesAndFakeLLM(t *testing.T) { func TestRunPipelineOnlySelectsRequestedLane(t *testing.T) { configPath := writeTestConfig(t, mvpConfigYAMLForLanes("dnd-session", "spells", "rituals")) inputPath := writeSeriatimInput(t) + outputDir := t.TempDir() + diagnosticsDir := t.TempDir() client := newFakeRunLLMClient(false) var stdout bytes.Buffer var stderr bytes.Buffer - code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--only", "spells"}, &stdout, &stderr, Options{ + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--only", "spells", "--output-dir", outputDir, "--diagnostics-dir", diagnosticsDir}, &stdout, &stderr, Options{ LLMClientFactory: fakeLLMFactory(client, nil), }) @@ -649,10 +657,11 @@ func TestRunPipelineOnlySelectsRequestedLane(t *testing.T) { func TestRunPipelineLLMFactoryFailure(t *testing.T) { configPath := writeTestConfig(t, mvpConfigYAML("dnd-session", "dnd/spells")) inputPath := writeSeriatimInput(t) + diagnosticsDir := t.TempDir() var stdout bytes.Buffer var stderr bytes.Buffer - code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath}, &stdout, &stderr, Options{ + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--diagnostics-dir", diagnosticsDir}, &stdout, &stderr, Options{ LLMClientFactory: fakeLLMFactory(newFakeRunLLMClient(false), errors.New("factory unavailable")), }) @@ -667,11 +676,13 @@ func TestRunPipelineLLMFactoryFailure(t *testing.T) { func TestRunPipelineValidationRejectionCompletesSuccessfully(t *testing.T) { configPath := writeTestConfig(t, mvpConfigYAML("dnd-session", "dnd/spells")) inputPath := writeSeriatimInput(t) + outputDir := t.TempDir() + diagnosticsDir := t.TempDir() client := newFakeRunLLMClient(true) var stdout bytes.Buffer var stderr bytes.Buffer - code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath}, &stdout, &stderr, Options{ + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--output-dir", outputDir, "--diagnostics-dir", diagnosticsDir}, &stdout, &stderr, Options{ LLMClientFactory: fakeLLMFactory(client, nil), }) @@ -686,12 +697,14 @@ func TestRunPipelineValidationRejectionCompletesSuccessfully(t *testing.T) { func TestRunPipelineLLMProfileOverrideSelectsFactoryProfile(t *testing.T) { configPath := writeTestConfig(t, mvpConfigYAMLWithProfiles("dnd-session")) inputPath := writeSeriatimInput(t) + outputDir := t.TempDir() + diagnosticsDir := t.TempDir() client := newFakeRunLLMClient(false) factory := &recordingLLMFactory{client: client} var stdout bytes.Buffer var stderr bytes.Buffer - code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--llm-profile", "runtime"}, &stdout, &stderr, Options{ + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--llm-profile", "runtime", "--output-dir", outputDir, "--diagnostics-dir", diagnosticsDir}, &stdout, &stderr, Options{ LLMClientFactory: factory.build, }) @@ -703,6 +716,190 @@ func TestRunPipelineLLMProfileOverrideSelectsFactoryProfile(t *testing.T) { } } +func TestRunPipelineWritesDurableOutputFiles(t *testing.T) { + diagnosticsDir := t.TempDir() + outputDir := t.TempDir() + configPath := writeTestConfig(t, mvpConfigYAMLWithDiagnostics("dnd-session", diagnosticsDir, "always")) + inputPath := writeSeriatimInput(t) + var stdout bytes.Buffer + var stderr bytes.Buffer + + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--output-dir", outputDir}, &stdout, &stderr, Options{ + LLMClientFactory: fakeLLMFactory(newFakeRunLLMClient(false), nil), + }) + + if code != 0 { + t.Fatalf("RunWithOptions() code = %d, stderr=%q", code, stderr.String()) + } + runOutputDir := onlyChildDir(t, outputDir) + for _, name := range []string{ + "index.json", + "manifest.json", + "artifacts/dnd.spell_cast.json", + "rejected.json", + "warnings.json", + } { + if _, err := os.Stat(filepath.Join(runOutputDir, filepath.FromSlash(name))); err != nil { + t.Fatalf("expected output file %q: %v", name, err) + } + } + if !strings.Contains(stdout.String(), runOutputDir) { + t.Fatalf("stdout = %q, want output path %q", stdout.String(), runOutputDir) + } + assertNoTemporaryFiles(t, runOutputDir) +} + +func TestRunPipelineRejectsUnsafeOutputFileName(t *testing.T) { + diagnosticsDir := t.TempDir() + outputDir := t.TempDir() + configPath := writeTestConfig(t, mvpConfigYAMLWithDiagnostics("dnd-session", diagnosticsDir, "always")) + inputPath := writeSeriatimInput(t) + registries := registriesWithOutput(t, unsafeOutputEncoder{}) + var stdout bytes.Buffer + var stderr bytes.Buffer + + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--output-dir", outputDir}, &stdout, &stderr, Options{ + Registries: registries, + LLMClientFactory: fakeLLMFactory(newFakeRunLLMClient(false), nil), + }) + + if code != 1 { + t.Fatalf("RunWithOptions() code = %d, want 1", code) + } + if !strings.Contains(stderr.String(), "output file name") { + t.Fatalf("stderr = %q, want unsafe output file error", stderr.String()) + } + runDir := onlyChildDir(t, diagnosticsDir) + if got := string(readFile(t, filepath.Join(runDir, diagnostics.ArtifactErrorLog))); !strings.Contains(got, "output file name") { + t.Fatalf("error log = %q, want unsafe output file error", got) + } +} + +func TestRunPipelineWritesDiagnosticsArtifactsOnSuccess(t *testing.T) { + diagnosticsDir := t.TempDir() + outputDir := t.TempDir() + configPath := writeTestConfig(t, mvpConfigYAMLWithDiagnostics("dnd-session", diagnosticsDir, "always")) + inputPath := writeSeriatimInput(t) + var stdout bytes.Buffer + var stderr bytes.Buffer + + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--output-dir", outputDir}, &stdout, &stderr, Options{ + LLMClientFactory: fakeLLMFactory(newFakeRunLLMClient(false), nil), + }) + + if code != 0 { + t.Fatalf("RunWithOptions() code = %d, stderr=%q", code, stderr.String()) + } + runDir := onlyChildDir(t, diagnosticsDir) + for _, name := range []string{ + diagnostics.ArtifactInvocationMetadata, + diagnostics.ArtifactEffectiveConfig, + diagnostics.ArtifactResolvedPipeline, + diagnostics.ArtifactRunManifest, + diagnostics.ArtifactRunReport, + diagnostics.ArtifactWarnings, + } { + if _, err := os.Stat(filepath.Join(runDir, name)); err != nil { + t.Fatalf("expected diagnostics artifact %q: %v", name, err) + } + } + report := string(readFile(t, filepath.Join(runDir, diagnostics.ArtifactRunReport))) + if !strings.Contains(report, `"approved_count": 1`) || !strings.Contains(report, `"validation_status": "approved"`) || !strings.Contains(report, outputDir) { + t.Fatalf("unexpected run report: %s", report) + } +} + +func TestRunPipelineWritesErrorLogAfterDiagnosticsCreation(t *testing.T) { + diagnosticsDir := t.TempDir() + configPath := writeTestConfig(t, mvpConfigYAMLWithDiagnostics("dnd-session", diagnosticsDir, "always")) + inputPath := writeSeriatimInput(t) + var stdout bytes.Buffer + var stderr bytes.Buffer + + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath}, &stdout, &stderr, Options{ + LLMClientFactory: fakeLLMFactory(newFakeRunLLMClient(false), errors.New("factory unavailable")), + }) + + if code != 1 { + t.Fatalf("RunWithOptions() code = %d, want 1", code) + } + runDir := onlyChildDir(t, diagnosticsDir) + if got := string(readFile(t, filepath.Join(runDir, diagnostics.ArtifactErrorLog))); !strings.Contains(got, "factory unavailable") { + t.Fatalf("error log = %q, want factory error", got) + } +} + +func TestRunPipelineRetentionNeverRemovesSuccessfulWarningFreeDiagnostics(t *testing.T) { + diagnosticsDir := t.TempDir() + outputDir := t.TempDir() + configPath := writeTestConfig(t, mvpConfigYAMLWithDiagnostics("dnd-session", diagnosticsDir, "never")) + inputPath := writeSeriatimInput(t) + var stdout bytes.Buffer + var stderr bytes.Buffer + + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--output-dir", outputDir}, &stdout, &stderr, Options{ + LLMClientFactory: fakeLLMFactory(newFakeRunLLMClient(false), nil), + }) + + if code != 0 { + t.Fatalf("RunWithOptions() code = %d, stderr=%q", code, stderr.String()) + } + if entries := childDirs(t, diagnosticsDir); len(entries) != 0 { + t.Fatalf("diagnostics run dirs = %v, want none", entries) + } +} + +func TestRunPipelineWarningsAreDiagnosedAndReported(t *testing.T) { + diagnosticsDir := t.TempDir() + outputDir := t.TempDir() + configPath := writeTestConfig(t, mvpConfigYAMLWithDiagnostics("dnd-session", diagnosticsDir, "auto")) + inputPath := writeSeriatimInput(t) + registries := registriesWithOutput(t, warningOutputEncoder{}) + var stdout bytes.Buffer + var stderr bytes.Buffer + + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--output-dir", outputDir}, &stdout, &stderr, Options{ + Registries: registries, + LLMClientFactory: fakeLLMFactory(newFakeRunLLMClient(false), nil), + }) + + if code != 0 { + t.Fatalf("RunWithOptions() code = %d, stderr=%q", code, stderr.String()) + } + if !strings.Contains(stderr.String(), "1 warning") { + t.Fatalf("stderr = %q, want warning count", stderr.String()) + } + runDir := onlyChildDir(t, diagnosticsDir) + warnings := string(readFile(t, filepath.Join(runDir, diagnostics.ArtifactWarnings))) + if !strings.Contains(warnings, "synthetic_warning") { + t.Fatalf("warnings artifact = %q, want synthetic warning", warnings) + } +} + +func TestRunPipelineDiagnosticsDirFlagOverridesConfig(t *testing.T) { + configDiagnosticsDir := t.TempDir() + overrideDiagnosticsDir := t.TempDir() + outputDir := t.TempDir() + configPath := writeTestConfig(t, mvpConfigYAMLWithDiagnostics("dnd-session", configDiagnosticsDir, "always")) + inputPath := writeSeriatimInput(t) + var stdout bytes.Buffer + var stderr bytes.Buffer + + code := RunWithOptions([]string{"run", "dnd-session", "--config", configPath, "--input", inputPath, "--output-dir", outputDir, "--diagnostics-dir", overrideDiagnosticsDir}, &stdout, &stderr, Options{ + LLMClientFactory: fakeLLMFactory(newFakeRunLLMClient(false), nil), + }) + + if code != 0 { + t.Fatalf("RunWithOptions() code = %d, stderr=%q", code, stderr.String()) + } + if entries := childDirs(t, configDiagnosticsDir); len(entries) != 0 { + t.Fatalf("config diagnostics dir entries = %v, want none", entries) + } + if entries := childDirs(t, overrideDiagnosticsDir); len(entries) != 1 { + t.Fatalf("override diagnostics dir entries = %v, want one run dir", entries) + } +} + func writeTestConfig(t *testing.T, content string) string { t.Helper() return writeFile(t, "config.yml", content) @@ -778,6 +975,20 @@ pipelines: ` } +func mvpConfigYAMLWithDiagnostics(pipelineID, diagnosticsDir, retention string) string { + return `version: 1 +diagnostics: + work_dir: ` + diagnosticsDir + ` + retention: ` + retention + ` +pipelines: + ` + pipelineID + `: + input: seriatim + artifacts: + spells: + extract: dnd/spells +` +} + func writeSeriatimInput(t *testing.T) string { t.Helper() return writeFile(t, "source.json", `{ @@ -857,6 +1068,110 @@ func (factory *recordingLLMFactory) build(ctx context.Context, cfg config.Config return factory.client, []artifacts.LLMProfileManifest{{ID: strings.TrimSpace(profileID)}}, nil } +type unsafeOutputEncoder struct{} + +func (unsafeOutputEncoder) Key() string { + return "json" +} + +func (unsafeOutputEncoder) Encode(ctx context.Context, req contracts.OutputRequest) (contracts.OutputResult, error) { + return contracts.OutputResult{ + Files: []contracts.OutputFile{ + { + Name: "../escape.json", + ContentType: "application/json", + Bytes: []byte("{}\n"), + }, + }, + }, nil +} + +type warningOutputEncoder struct{} + +func (warningOutputEncoder) Key() string { + return "json" +} + +func (warningOutputEncoder) Encode(ctx context.Context, req contracts.OutputRequest) (contracts.OutputResult, error) { + result, err := jsonoutput.New().Encode(ctx, req) + if err != nil { + return contracts.OutputResult{}, err + } + result.Warnings = append(result.Warnings, contracts.Warning{ + Scope: "output", + ReasonCode: "synthetic_warning", + Message: "synthetic output warning", + }) + return result, nil +} + +func registriesWithOutput(t *testing.T, encoder contracts.OutputEncoder) pipeline.Registries { + t.Helper() + registries, err := productionRegistries() + if err != nil { + t.Fatalf("productionRegistries: %v", err) + } + outputs := pipeline.NewOutputEncoderRegistry() + if err := outputs.RegisterWithSpec(jsonoutput.ModuleSpec(), func() (contracts.OutputEncoder, error) { + return encoder, nil + }); err != nil { + t.Fatalf("register test output encoder: %v", err) + } + registries.Outputs = outputs + return registries +} + +func onlyChildDir(t *testing.T, root string) string { + t.Helper() + children := childDirs(t, root) + if len(children) != 1 { + t.Fatalf("child dirs under %q = %v, want one", root, children) + } + return children[0] +} + +func childDirs(t *testing.T, root string) []string { + t.Helper() + entries, err := os.ReadDir(root) + if err != nil { + if os.IsNotExist(err) { + return nil + } + t.Fatalf("read dir %q: %v", root, err) + } + var dirs []string + for _, entry := range entries { + if entry.IsDir() { + dirs = append(dirs, filepath.Join(root, entry.Name())) + } + } + return dirs +} + +func readFile(t *testing.T, path string) []byte { + t.Helper() + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %q: %v", path, err) + } + return data +} + +func assertNoTemporaryFiles(t *testing.T, root string) { + t.Helper() + if err := filepath.WalkDir(root, func(path string, entry os.DirEntry, err error) error { + if err != nil { + return err + } + if strings.Contains(entry.Name(), ".tmp-") { + t.Fatalf("temporary file remains after success: %s", path) + } + return nil + }); err != nil { + t.Fatalf("walk output dir %q: %v", root, err) + } +} + func fakeCatalog(t *testing.T) pipeline.ModuleCatalog { t.Helper() inputs := pipeline.NewInputAdapterRegistry() diff --git a/internal/core/diagnostics/run_dir.go b/internal/core/diagnostics/run_dir.go index 3616ab2..788d6d6 100644 --- a/internal/core/diagnostics/run_dir.go +++ b/internal/core/diagnostics/run_dir.go @@ -173,7 +173,7 @@ func (r *RunDirectory) WriteErrorLog(errorMessage string) error { if err != nil { return err } - if err := os.WriteFile(path, []byte(errorMessage+"\n"), 0o644); err != nil { + if err := writeFileAtomic(path, []byte(errorMessage+"\n"), 0o644); err != nil { return fmt.Errorf("write diagnostics artifact %q: %w", ArtifactErrorLog, err) } return nil @@ -193,7 +193,7 @@ func (r *RunDirectory) WriteJSONArtifact(name string, payload any) error { return fmt.Errorf("marshal diagnostics artifact %q: %w", name, err) } data = append(data, '\n') - if err := os.WriteFile(path, data, 0o644); err != nil { + if err := writeFileAtomic(path, data, 0o644); err != nil { return fmt.Errorf("write diagnostics artifact %q: %w", name, err) } return nil @@ -241,3 +241,39 @@ func (r *RunDirectory) artifactPath(name string) (string, error) { } return artifactPath, nil } + +func writeFileAtomic(path string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(path) + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + + temp, err := os.CreateTemp(dir, "."+filepath.Base(path)+".tmp-*") + if err != nil { + return err + } + tempPath := temp.Name() + removeTemp := true + defer func() { + if removeTemp { + _ = os.Remove(tempPath) + } + }() + + if _, err := temp.Write(data); err != nil { + _ = temp.Close() + return err + } + if err := temp.Chmod(perm); err != nil { + _ = temp.Close() + return err + } + if err := temp.Close(); err != nil { + return err + } + if err := os.Rename(tempPath, path); err != nil { + return err + } + removeTemp = false + return nil +} diff --git a/internal/core/diagnostics/run_dir_test.go b/internal/core/diagnostics/run_dir_test.go index e20a224..2d7a6e3 100644 --- a/internal/core/diagnostics/run_dir_test.go +++ b/internal/core/diagnostics/run_dir_test.go @@ -118,6 +118,24 @@ func TestWriteJSONArtifactWritesIndentedNewlineTerminatedJSON(t *testing.T) { } } +func TestWriteJSONArtifactLeavesNoTemporaryFiles(t *testing.T) { + runDir := newTestRunDirectory(t) + + if err := runDir.WriteJSONArtifact("artifact.json", map[string]any{"value": "ok"}); err != nil { + t.Fatalf("WriteJSONArtifact: %v", err) + } + + entries, err := os.ReadDir(runDir.Path()) + if err != nil { + t.Fatalf("read run directory: %v", err) + } + for _, entry := range entries { + if strings.Contains(entry.Name(), ".tmp-") { + t.Fatalf("temporary diagnostics file remains after success: %s", entry.Name()) + } + } +} + func TestWriteInvocationMetadataFillsMissingRunIDAndStartTime(t *testing.T) { runDir := newTestRunDirectory(t)