diff --git a/docs/internal/app-orchestration.md b/docs/internal/app-orchestration.md index 484c6cf..7dcf8ec 100644 --- a/docs/internal/app-orchestration.md +++ b/docs/internal/app-orchestration.md @@ -7,9 +7,9 @@ is owned by the [CLI reference](../cli.md) and [operations guide](../operations. ## Single-Report Flow -`GenerateDetailed` resolves the requested report and output destination before initializing an optional explicit debug writer. An explicit output file wins; otherwise the configured output directory is used, falling back to the captured working directory. Output preflight validates the final filename and the bounded same-directory temporary form without creating a missing parent. It then validates the report's generated-text catalog binding, exact Promptkit prompt, and selected profile before collecting weather data. The resolved profile, backend, and model are carried in the active result. +`GenerateDetailed` resolves the requested report and output destination before initializing an optional explicit debug writer. An explicit output file wins; otherwise the configured output directory is used, falling back to the captured working directory. Output preflight validates the final filename, permits only an absent or regular final destination, and validates the bounded same-directory temporary form without creating a missing parent. It then validates the report's generated-text catalog binding, exact Promptkit prompt, and selected profile before collecting weather data. The resolved profile, backend, and model are carried in the active result. -The workflow builds facts, a module snapshot, briefing metadata, and the YAML prompt package in memory. It executes Promptkit only against the inspected prompt and profile, reconciles the preparation callback and completed result with that identity and the prepared report schema, validates the returned generated text, builds a render context, and renders Markdown. `fileutil` atomically writes the completed Markdown to the selected output path. Only after that write succeeds does single-report notification run. +The workflow builds facts, a module snapshot, briefing metadata, and the YAML prompt package in memory. It executes Promptkit only against the inspected prompt and profile, reconciles the preparation callback and completed result with that identity and the prepared report schema, validates the returned generated text, builds a render context, and renders Markdown. `fileutil` writes the completed Markdown through a same-directory temporary file, rechecks the final destination and context after close and immediately before the atomic rename. Only after that write succeeds does single-report notification run. Failures return an active partial result with safe identity, profile, warning, validation, debug, and output information when available. After rendering and immediately before publication, the workflow checks for cancellation or deadline expiry. Any failure before publication leaves an existing destination unchanged. A notification failure retains the newly published output. diff --git a/docs/operations.md b/docs/operations.md index 2b0ef80..e1528e3 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -40,7 +40,11 @@ Weatherreporter validates the final output filename before prompt inspection or weather collection. A valid long filename is published through a short, same-directory temporary sibling, so temporary naming does not shorten the operator-selected destination. A rejected filename does not create a missing -parent directory. +parent directory. The final destination itself must be absent or a regular +file: symlinks, directories, named pipes, sockets, and other special objects +are rejected before prompt inspection or weather collection. The destination is +checked again immediately before the atomic replacement; cancellation or a +deadline at that point leaves the prior report unchanged and skips notification. `SIGINT` and `SIGTERM` request orderly cancellation of an active action. The command lets cancellation and related cleanup finish before it exits; use the diff --git a/docs/policy/architecture.md b/docs/policy/architecture.md index b5ba871..01e2fca 100644 --- a/docs/policy/architecture.md +++ b/docs/policy/architecture.md @@ -72,6 +72,9 @@ directly. pre-publication failure, including cancellation observed immediately before publication, does not replace an existing destination; a notification failure does not remove a newly published output. +- A single-report final destination is either absent or a regular file. + Symlinks and special filesystem objects are rejected during preflight and + rechecked immediately before the atomic replacement. - Configuration or explicit CLI input selects that operator-owned destination; it does not create an application-owned state boundary. - Comparison bundles are flat, versioned operator outputs. Their guarded diff --git a/internal/app/generation_test.go b/internal/app/generation_test.go index ba21538..c2e6d90 100644 --- a/internal/app/generation_test.go +++ b/internal/app/generation_test.go @@ -26,6 +26,20 @@ type generationCollector struct { beforeRun func() } +type publicationGateContext struct { + context.Context + err error + checks int +} + +func (c *publicationGateContext) Err() error { + c.checks++ + if c.checks >= 2 { + return c.err + } + return nil +} + func (c *generationCollector) Run(context.Context, collect.Request) (*collect.Result, error) { if c.beforeRun != nil { c.beforeRun() @@ -385,6 +399,41 @@ func TestGenerateDetailedPreservesDestinationWhenContextDeadlineExpiresBeforePub } } +func TestGenerateDetailedPreservesDestinationWhenContextChangesDuringPublication(t *testing.T) { + for _, tt := range []struct { + name string + err error + category promptexec.ErrorCategory + }{ + {name: "canceled", err: context.Canceled, category: promptexec.Canceled}, + {name: "deadline", err: context.DeadlineExceeded, category: promptexec.DeadlineExceeded}, + } { + t.Run(tt.name, func(t *testing.T) { + outputPath := filepath.Join(t.TempDir(), "daily.md") + const previousReport = "previous report" + if err := os.WriteFile(outputPath, []byte(previousReport), 0o600); err != nil { + t.Fatal(err) + } + ctx := &publicationGateContext{Context: context.Background(), err: tt.err} + cfg := generationConfig() + cfg.Notify.Distributor.Enabled = true + cfg.Notify.Distributor.PipelineIDTemplate = "weather" + bundle := generationBundle(t) + notifier := &generationNotifier{} + result, err := GenerateDetailed(ctx, GenerateRequest{ + Config: cfg, Report: ReportDaily, + Date: generationTime("2026-05-29T12:00:00-05:00"), Now: generationTime("2026-05-29T08:30:00-05:00"), + WorkingDir: t.TempDir(), OutputPath: outputPath, Collector: &generationCollector{bundle: &bundle}, Executor: &generationExecutor{}, Notifier: notifier, + }) + data, readErr := os.ReadFile(outputPath) + matches, globErr := filepath.Glob(filepath.Join(filepath.Dir(outputPath), ".weatherreporter-*.tmp")) + if !errors.Is(err, tt.err) || promptexec.CategoryOf(err) != tt.category || result == nil || result.OutputPath != "" || notifier.calls != 0 || readErr != nil || string(data) != previousReport || globErr != nil || len(matches) != 0 { + t.Fatalf("GenerateDetailed() result/error/output/notification/temp = %#v/%v/%q/%#v/%v/%v", result, err, data, notifier, matches, globErr) + } + }) + } +} + func TestGenerateDetailedRetainsPublishedOutputWhenNotificationFails(t *testing.T) { cfg := generationConfig() cfg.Notify.Distributor.Enabled = true @@ -407,10 +456,35 @@ func TestGenerateDetailedDoesNotReplaceDirectoryOutput(t *testing.T) { if err := os.Mkdir(outputPath, 0o700); err != nil { t.Fatal(err) } - result, err := GenerateDetailed(context.Background(), GenerateRequest{Config: generationConfig(), Report: ReportDaily, Date: generationTime("2026-05-29T12:00:00-05:00"), Now: generationTime("2026-05-29T08:30:00-05:00"), WorkingDir: t.TempDir(), OutputPath: outputPath, Collector: &generationCollector{bundle: &bundle}, Executor: &generationExecutor{}}) + collector := &generationCollector{bundle: &bundle} + executor := &generationExecutor{} + notifier := &generationNotifier{} + result, err := GenerateDetailed(context.Background(), GenerateRequest{Config: generationConfig(), Report: ReportDaily, Date: generationTime("2026-05-29T12:00:00-05:00"), Now: generationTime("2026-05-29T08:30:00-05:00"), WorkingDir: t.TempDir(), OutputPath: outputPath, Collector: collector, Executor: executor, Notifier: notifier}) info, statErr := os.Stat(outputPath) - if err == nil || result == nil || statErr != nil || !info.IsDir() { - t.Fatalf("GenerateDetailed() result/error/output-info = %#v/%v/%#v (%v)", result, err, info, statErr) + if err == nil || result == nil || statErr != nil || !info.IsDir() || collector.called || executor.promptInspections != 0 || executor.called || notifier.calls != 0 { + t.Fatalf("GenerateDetailed() result/error/output-info/collector/executor/notifier = %#v/%v/%#v (%v)/%t/%#v/%#v", result, err, info, statErr, collector.called, executor, notifier) + } +} + +func TestGenerateDetailedDoesNotReplaceSymbolicLinkOutput(t *testing.T) { + dir := t.TempDir() + backing := filepath.Join(dir, "backing.md") + if err := os.WriteFile(backing, []byte("previous report"), 0o600); err != nil { + t.Fatal(err) + } + outputPath := filepath.Join(dir, "daily.md") + if err := os.Symlink(backing, outputPath); err != nil { + t.Fatal(err) + } + bundle := generationBundle(t) + collector := &generationCollector{bundle: &bundle} + executor := &generationExecutor{} + notifier := &generationNotifier{} + result, err := GenerateDetailed(context.Background(), GenerateRequest{Config: generationConfig(), Report: ReportDaily, Date: generationTime("2026-05-29T12:00:00-05:00"), Now: generationTime("2026-05-29T08:30:00-05:00"), WorkingDir: t.TempDir(), OutputPath: outputPath, Collector: collector, Executor: executor, Notifier: notifier}) + info, statErr := os.Lstat(outputPath) + data, readErr := os.ReadFile(backing) + if err == nil || result == nil || statErr != nil || info.Mode()&os.ModeSymlink == 0 || readErr != nil || string(data) != "previous report" || collector.called || executor.promptInspections != 0 || executor.called || notifier.calls != 0 { + t.Fatalf("GenerateDetailed() result/error/output/backing/collector/executor/notifier = %#v/%v/%#v (%v)/%q (%v)/%t/%#v/%#v", result, err, info, statErr, data, readErr, collector.called, executor, notifier) } } diff --git a/internal/app/output.go b/internal/app/output.go index 52602b8..2e3353b 100644 --- a/internal/app/output.go +++ b/internal/app/output.go @@ -188,10 +188,5 @@ func validateOutputPath(path string) (string, error) { if err := fileutil.ValidateAtomicPath(path); err != nil { return "", fmt.Errorf("validate final output path %q: %w", path, err) } - if info, err := os.Stat(path); err == nil && info.IsDir() { - return "", fmt.Errorf("final output path %q is a directory", path) - } else if err != nil && !os.IsNotExist(err) { - return "", fmt.Errorf("inspect final output path %q: %w", path, err) - } return path, nil } diff --git a/internal/app/output_linux_test.go b/internal/app/output_linux_test.go new file mode 100644 index 0000000..72063d9 --- /dev/null +++ b/internal/app/output_linux_test.go @@ -0,0 +1,59 @@ +//go:build linux + +package app + +import ( + "context" + "net" + "os" + "path/filepath" + "syscall" + "testing" +) + +func TestGenerateDetailedRejectsSpecialOutputBeforeWork(t *testing.T) { + for _, tt := range []struct { + name string + setup func(t *testing.T, path string) + }{ + { + name: "named pipe", + setup: func(t *testing.T, path string) { + t.Helper() + if err := syscall.Mkfifo(path, 0o600); err != nil { + t.Fatal(err) + } + }, + }, + { + name: "socket", + setup: func(t *testing.T, path string) { + t.Helper() + listener, err := net.ListenUnix("unix", &net.UnixAddr{Name: path, Net: "unix"}) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = listener.Close() }) + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + outputPath := filepath.Join(t.TempDir(), "daily.md") + tt.setup(t, outputPath) + bundle := generationBundle(t) + collector := &generationCollector{bundle: &bundle} + executor := &generationExecutor{} + notifier := &generationNotifier{} + + result, err := GenerateDetailed(context.Background(), GenerateRequest{ + Config: generationConfig(), Report: ReportDaily, + Date: generationTime("2026-05-29T12:00:00-05:00"), Now: generationTime("2026-05-29T08:30:00-05:00"), + WorkingDir: t.TempDir(), OutputPath: outputPath, Collector: collector, Executor: executor, Notifier: notifier, + }) + info, statErr := os.Lstat(outputPath) + if err == nil || result == nil || statErr != nil || info.Mode().IsRegular() || collector.called || executor.promptInspections != 0 || executor.called || notifier.calls != 0 { + t.Fatalf("GenerateDetailed() result/error/output/collector/executor/notifier = %#v/%v/%#v (%v)/%t/%#v/%#v", result, err, info, statErr, collector.called, executor, notifier) + } + }) + } +} diff --git a/internal/app/prompt_generate.go b/internal/app/prompt_generate.go index b7e5caa..30542b5 100644 --- a/internal/app/prompt_generate.go +++ b/internal/app/prompt_generate.go @@ -86,7 +86,10 @@ func publishPromptReport(ctx context.Context, req promptPublicationRequest) (*Re if err := publicationContextError(ctx); err != nil { return req.Result, generatedReportError(req.Resolved, req.Result.RunID, "publish report", err) } - if err := fileutil.WriteFileAtomic(req.OutputPath, req.Markdown); err != nil { + if err := fileutil.WriteFileAtomicContext(ctx, req.OutputPath, req.Markdown); err != nil { + if contextErr := publicationContextError(ctx); contextErr != nil { + return req.Result, generatedReportError(req.Resolved, req.Result.RunID, "publish report", contextErr) + } return req.Result, err } req.Result.OutputPath = req.OutputPath diff --git a/internal/fileutil/fileutil.go b/internal/fileutil/fileutil.go index ab4c06f..91161ae 100644 --- a/internal/fileutil/fileutil.go +++ b/internal/fileutil/fileutil.go @@ -2,6 +2,7 @@ package fileutil import ( + "context" "encoding/json" "fmt" "os" @@ -14,15 +15,35 @@ const ( ) // ValidateAtomicPath verifies that the final path can be safely used with this -// package's same-directory atomic-write implementation. +// package's same-directory atomic-write implementation. It accepts only an +// absent path or an existing regular file. func ValidateAtomicPath(path string) error { if len(filepath.Base(path)) > maxFileNameBytes { return fmt.Errorf("final file name exceeds the %d-byte limit", maxFileNameBytes) } + info, err := os.Lstat(path) + if os.IsNotExist(err) { + return nil + } + if err != nil { + return fmt.Errorf("inspect final destination %q: %w", path, err) + } + if !info.Mode().IsRegular() { + return fmt.Errorf("final destination %q must be absent or a regular file", path) + } return nil } func WriteFileAtomic(path string, data []byte) error { + return WriteFileAtomicContext(context.Background(), path, data) +} + +// WriteFileAtomicContext writes data through a same-directory temporary file. +// It checks cancellation immediately before the rename that publishes the file. +func WriteFileAtomicContext(ctx context.Context, path string, data []byte) error { + if ctx == nil { + ctx = context.Background() + } if err := ValidateAtomicPath(path); err != nil { return err } @@ -42,6 +63,12 @@ func WriteFileAtomic(path string, data []byte) error { if err := tmp.Close(); err != nil { return fmt.Errorf("close temporary file for %q: %w", path, err) } + if err := ValidateAtomicPath(path); err != nil { + return err + } + if err := ctx.Err(); err != nil { + return err + } if err := os.Rename(tmpName, path); err != nil { return fmt.Errorf("save %q: %w", path, err) } diff --git a/internal/fileutil/fileutil_test.go b/internal/fileutil/fileutil_test.go index 6f0df82..3288f59 100644 --- a/internal/fileutil/fileutil_test.go +++ b/internal/fileutil/fileutil_test.go @@ -1,6 +1,8 @@ package fileutil import ( + "context" + "errors" "os" "path/filepath" "strings" @@ -21,6 +23,10 @@ func TestWriteFileAtomicCreatesParentDirectory(t *testing.T) { if string(data) != "artifact" { t.Fatalf("data = %q, want artifact", data) } + info, err := os.Stat(path) + if err != nil || info.Mode().Perm() != 0o600 { + t.Fatalf("output mode/error = %o/%v", info.Mode().Perm(), err) + } } func TestWriteFileAtomicOverwritesTarget(t *testing.T) { @@ -67,26 +73,81 @@ func TestWriteFileAtomicSupportsLongestFileName(t *testing.T) { } } -func TestWriteFileAtomicCleansTemporaryFileAfterRenameError(t *testing.T) { +func TestWriteFileAtomicRejectsUnsafeFinalDestinations(t *testing.T) { dir := t.TempDir() - target := filepath.Join(dir, "target") - if err := os.Mkdir(target, 0o755); err != nil { - t.Fatalf("Mkdir() error = %v", err) + backing := filepath.Join(dir, "backing.md") + if err := os.WriteFile(backing, []byte("old"), 0o600); err != nil { + t.Fatal(err) } + for _, tt := range []struct { + name string + setup func(t *testing.T, path string) + }{ + { + name: "directory", + setup: func(t *testing.T, path string) { + t.Helper() + if err := os.Mkdir(path, 0o755); err != nil { + t.Fatal(err) + } + }, + }, + { + name: "symbolic link", + setup: func(t *testing.T, path string) { + t.Helper() + if err := os.Symlink(backing, path); err != nil { + t.Fatal(err) + } + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + target := filepath.Join(dir, tt.name) + tt.setup(t, target) + before, err := os.Lstat(target) + if err != nil { + t.Fatal(err) + } - err := WriteFileAtomic(target, []byte("data")) - if err == nil { - t.Fatal("WriteFileAtomic() error = nil, want rename error") + if err := WriteFileAtomic(target, []byte("new")); err == nil { + t.Fatal("WriteFileAtomic() error = nil") + } + after, err := os.Lstat(target) + if err != nil || after.Mode() != before.Mode() { + t.Fatalf("target mode/error = %v/%v, want %v", after.Mode(), err, before.Mode()) + } + matches, err := filepath.Glob(filepath.Join(dir, ".weatherreporter-*.tmp")) + if err != nil || len(matches) != 0 { + t.Fatalf("temporary files/error = %v/%v", matches, err) + } + }) } - if !strings.Contains(err.Error(), "save") { - t.Fatalf("error = %q, want save context", err.Error()) + data, err := os.ReadFile(backing) + if err != nil || string(data) != "old" { + t.Fatalf("symbolic link target/error = %q/%v", data, err) } - matches, err := filepath.Glob(filepath.Join(dir, ".weatherreporter-*.tmp")) - if err != nil { - t.Fatalf("Glob() error = %v", err) +} + +func TestWriteFileAtomicContextPreservesDestinationWhenCanceledAtPublication(t *testing.T) { + path := filepath.Join(t.TempDir(), "artifact.txt") + if err := os.WriteFile(path, []byte("old"), 0o600); err != nil { + t.Fatal(err) } - if len(matches) != 0 { - t.Fatalf("temporary files = %v, want none", matches) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + err := WriteFileAtomicContext(ctx, path, []byte("new")) + if !errors.Is(err, context.Canceled) { + t.Fatalf("WriteFileAtomicContext() error = %v, want context cancellation", err) + } + data, readErr := os.ReadFile(path) + if readErr != nil || string(data) != "old" { + t.Fatalf("output/error = %q/%v", data, readErr) + } + matches, globErr := filepath.Glob(filepath.Join(filepath.Dir(path), ".weatherreporter-*.tmp")) + if globErr != nil || len(matches) != 0 { + t.Fatalf("temporary files/error = %v/%v", matches, globErr) } }