diff --git a/cmd/weatherreporter/main.go b/cmd/weatherreporter/main.go index c8f6dd6..b195adf 100644 --- a/cmd/weatherreporter/main.go +++ b/cmd/weatherreporter/main.go @@ -19,7 +19,13 @@ func main() { } func runCommand(args []string, stdout, stderr io.Writer, runner func(context.Context, []string, io.Writer, io.Writer) error) error { - ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt, syscall.SIGTERM) + return runCommandWithSignalContext(args, stdout, stderr, runner, signal.NotifyContext) +} + +type signalContextFunc func(context.Context, ...os.Signal) (context.Context, context.CancelFunc) + +func runCommandWithSignalContext(args []string, stdout, stderr io.Writer, runner func(context.Context, []string, io.Writer, io.Writer) error, signalContext signalContextFunc) error { + ctx, stop := signalContext(context.Background(), os.Interrupt, syscall.SIGTERM) defer stop() return runner(ctx, args, stdout, stderr) } diff --git a/cmd/weatherreporter/main_test.go b/cmd/weatherreporter/main_test.go index 3a8d525..129462f 100644 --- a/cmd/weatherreporter/main_test.go +++ b/cmd/weatherreporter/main_test.go @@ -7,50 +7,30 @@ import ( "os" "syscall" "testing" - "time" ) -func TestRunCommandCancelsActionContextOnSignal(t *testing.T) { - for _, tt := range []struct { - name string - signal os.Signal - }{ - {name: "Interrupt", signal: os.Interrupt}, - {name: "Terminate", signal: syscall.SIGTERM}, - } { - t.Run(tt.name, func(t *testing.T) { - started := make(chan struct{}) - done := make(chan error, 1) - go func() { - done <- runCommand(nil, io.Discard, io.Discard, func(ctx context.Context, _ []string, _, _ io.Writer) error { - close(started) - <-ctx.Done() - return ctx.Err() - }) - }() +func TestRunCommandBuildsCancelableSignalContext(t *testing.T) { + var signals []os.Signal + stopped := false + signalContext := func(parent context.Context, requested ...os.Signal) (context.Context, context.CancelFunc) { + signals = append([]os.Signal(nil), requested...) + ctx, cancel := context.WithCancel(parent) + cancel() + return ctx, func() { + stopped = true + } + } - select { - case <-started: - case <-time.After(time.Second): - t.Fatal("runner did not receive an action context") - } - - process, err := os.FindProcess(os.Getpid()) - if err != nil { - t.Fatalf("FindProcess() error = %v", err) - } - if err := process.Signal(tt.signal); err != nil { - t.Fatalf("Signal(%v) error = %v", tt.signal, err) - } - - select { - case err := <-done: - if !errors.Is(err, context.Canceled) { - t.Fatalf("runCommand() error = %v, want context cancellation", err) - } - case <-time.After(time.Second): - t.Fatal("interrupt did not cancel the action context") - } - }) + err := runCommandWithSignalContext(nil, io.Discard, io.Discard, func(ctx context.Context, _ []string, _, _ io.Writer) error { + return ctx.Err() + }, signalContext) + if !errors.Is(err, context.Canceled) { + t.Fatalf("runCommandWithSignalContext() error = %v, want context cancellation", err) + } + if len(signals) != 2 || signals[0] != os.Interrupt || signals[1] != syscall.SIGTERM { + t.Fatalf("requested signals = %#v, want Interrupt and SIGTERM", signals) + } + if !stopped { + t.Fatal("signal context stop function was not called") } } diff --git a/cmd/weatherreporter/main_unix_test.go b/cmd/weatherreporter/main_unix_test.go new file mode 100644 index 0000000..021a4a4 --- /dev/null +++ b/cmd/weatherreporter/main_unix_test.go @@ -0,0 +1,58 @@ +//go:build unix + +package main + +import ( + "context" + "errors" + "io" + "os" + "syscall" + "testing" + "time" +) + +func TestRunCommandCancelsActionContextOnSignal(t *testing.T) { + for _, tt := range []struct { + name string + signal os.Signal + }{ + {name: "Interrupt", signal: os.Interrupt}, + {name: "Terminate", signal: syscall.SIGTERM}, + } { + t.Run(tt.name, func(t *testing.T) { + started := make(chan struct{}) + done := make(chan error, 1) + go func() { + done <- runCommand(nil, io.Discard, io.Discard, func(ctx context.Context, _ []string, _, _ io.Writer) error { + close(started) + <-ctx.Done() + return ctx.Err() + }) + }() + + select { + case <-started: + case <-time.After(time.Second): + t.Fatal("runner did not receive an action context") + } + + process, err := os.FindProcess(os.Getpid()) + if err != nil { + t.Fatalf("FindProcess() error = %v", err) + } + if err := process.Signal(tt.signal); err != nil { + t.Fatalf("Signal(%v) error = %v", tt.signal, err) + } + + select { + case err := <-done: + if !errors.Is(err, context.Canceled) { + t.Fatalf("runCommand() error = %v, want context cancellation", err) + } + case <-time.After(time.Second): + t.Fatal("interrupt did not cancel the action context") + } + }) + } +} diff --git a/docs/cli.md b/docs/cli.md index fcb8146..f2877b7 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -176,7 +176,7 @@ complete prior bundle is a rollback artifact. | `--units VALUE` | `generate`, `run`, `compare` | Override `weather_api.units` for this command. | | `--tz NAME` | `generate`, `run`, `compare` | Override `weather_api.timezone` for this command. | | `--out PATH` | every `generate` command | Write the report to this complete file destination instead of the configured or current-directory default. | -| `--llm-debug-dir PATH` | every `generate`, `run`, and `compare` command | Write requested sensitive prompt diagnostics under this absolute path. | +| `--llm-debug-dir PATH` | every `generate`, `run`, and `compare` command | On Unix hosts, write requested sensitive prompt diagnostics under this absolute path. Other hosts fail closed when the flag is requested. | | `--profile PROFILE` | `compare` | Select one explicit profile. Repeat at least twice with distinct, nonblank IDs. | | `--out-dir PATH` | `run morning`, `run evening`, `compare` | Write batch reports beneath this directory, or select the exact comparison directory. | | `--replace` | `compare` | Authorize replacement of a recognized nonempty comparison bundle. | diff --git a/docs/config.md b/docs/config.md index 5c4088f..f67cf1e 100644 --- a/docs/config.md +++ b/docs/config.md @@ -188,6 +188,8 @@ key is rejected with a migration error; it is not translated or ignored. Prompt debug capture has no YAML setting. Use `--llm-debug-dir PATH` on an individual `generate`, `run`, or `compare` command when explicitly needed. +See [optional prompt debug capture](operations.md#optional-prompt-debug-capture) +for platform availability, security, and retention requirements. | Field | Default | Rules | | --- | --- | --- | diff --git a/docs/integrations/promptkit.md b/docs/integrations/promptkit.md index 220efb5..7f4f4a5 100644 --- a/docs/integrations/promptkit.md +++ b/docs/integrations/promptkit.md @@ -33,7 +33,9 @@ When capture is enabled, its preparation artifact projects a provider endpoint to its scheme and host and retains only reviewed execution settings. Provider extras and URL user information, paths, queries, and fragments are omitted. Capture storage remains confined to the operator-selected debug root; an unsafe -filesystem path causes the requested execution to fail. +filesystem path causes the requested execution to fail. Host availability and +operator handling are documented in the +[operations guide](../operations.md#optional-prompt-debug-capture). ## Comparison Execution diff --git a/docs/internal/comparison-execution.md b/docs/internal/comparison-execution.md index af8bb26..924cc41 100644 --- a/docs/internal/comparison-execution.md +++ b/docs/internal/comparison-execution.md @@ -22,7 +22,9 @@ profile slug. This keeps concurrent captures separate. The debug writer itself owns secure-root validation and file permissions. It safely creates shared missing ancestors during concurrent writes, then rejects symlink and non- directory components. Operational retention and sensitivity are documented in -the [operations guide](../operations.md). +the [operations guide](../operations.md). This secure writer is enabled only on +Unix hosts; comparison fails before execution when another host requests debug +capture. The output result and its safe errors are converted into the durable contract only by comparison publication. See [comparison publication diff --git a/docs/operations.md b/docs/operations.md index 374e985..652f647 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -166,6 +166,12 @@ remain distinct. Normal output, summaries, and routine logs omit that sensitive content. Debug capture is never created for an ordinary command without `--llm-debug-dir`. +Secure prompt debug capture is currently available only on Unix hosts, where +Weatherreporter can keep every traversal and write anchored to opened directory +descriptors without following symbolic links. On other platforms, requesting +`--llm-debug-dir` fails before prompt inspection, weather collection, or +provider execution; ordinary commands without the flag remain available. + Preparation captures retain only the provider endpoint origin and reviewed execution settings. URL user information, paths, queries, fragments, and unrecognized provider parameters are omitted. diff --git a/docs/releases/v0.12.0.md b/docs/releases/v0.12.0.md new file mode 100644 index 0000000..728cb9c --- /dev/null +++ b/docs/releases/v0.12.0.md @@ -0,0 +1,66 @@ +# Weatherreporter v0.12.0 + +This release completes a repository-wide correctness, security, efficiency, +test-durability, and documentation audit. + +## Summary + +Weatherreporter now applies stricter validation and bounded diagnostics across +its configuration, weather collection, Promptkit, rendering, publication, +comparison, and Distributor boundaries. Report preparation and execution carry +one reconciled identity, independent weather sources are collected +concurrently, and cancellation preserves completed report and comparison +outcomes. + +The release also removes obsolete compatibility surfaces and consolidates +duplicated implementation and test policy without changing ordinary report +commands or output identities. + +## Compatibility + +This release is compatible with `v0.11.0` for ordinary `generate`, `run`, and +`compare` commands, configuration files, report filenames, comparison bundles, +and Distributor integration. + +Sensitive prompt-debug capture through `--llm-debug-dir` is now supported only +on Unix hosts. Non-Unix hosts reject an explicit capture request before prompt +inspection, weather collection, or provider execution because the required +handle-relative, no-follow filesystem guarantees are unavailable there. + +Several unused internal compatibility exports were removed. They were not part +of the documented CLI, configuration, artifact, or integration contracts. + +## Upgrade + +No special action is required for ordinary installations. Operators who use +`--llm-debug-dir` on Windows must run that diagnostic workflow on a Unix host. +Review any automation that depended on undocumented internal Go APIs removed by +this release. + +## Changes + +- Hardened configuration loading, source validation, secrets rollback, + endpoint validation, HTTP diagnostics, generated-text limits, prompt-debug + redaction, output publication, comparison replacement, and Distributor + failure reporting. +- Reconciled inspected, prepared, callback, and completed Promptkit identity + and provenance before accepting generated content. +- Preserved metric values, civil-day and daypart identity, overnight alerts, + precipitation semantics, and Markdown structure across deterministic report + preparation and rendering. +- Collected independent Weather API sources concurrently and reused readiness + data while retaining deterministic normalized results. +- Preserved completed report and comparison failures independently from shared + cancellation, stopped unfinished work, and skipped batch notification after + cancellation or partial report failure. +- Made secure prompt-debug traversal descriptor-relative on Unix and fail + closed elsewhere. See the [operations + guide](../operations.md#optional-prompt-debug-capture). +- Strengthened default test portability and determinism, including + capability-aware symbolic-link fixtures and platform-appropriate process + signal coverage. +- Removed obsolete compatibility helpers, duplicated test ownership, dormant + persistence code, and completed audit and implementation roadmaps. +- Updated the [architecture policy](../policy/architecture.md), [testing + policy](../policy/testing.md), and focused internal guides to describe the + implemented final state. diff --git a/go.mod b/go.mod index 3b6e55d..9f90895 100644 --- a/go.mod +++ b/go.mod @@ -11,6 +11,4 @@ require ( golang.org/x/sys v0.45.0 ) -require ( - golang.org/x/text v0.14.0 // indirect -) +require golang.org/x/text v0.14.0 // indirect diff --git a/internal/app/app.go b/internal/app/app.go index 0bf309b..227560d 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -359,7 +359,7 @@ func RunBatchDetailed(ctx context.Context, req BatchRequest) (*BatchResult, erro result := &BatchResult{Batch: req.Batch, StartedAt: startedAt} var cancellation error for index, planned := range plannedReports { - if cancellation = batchCancellationCause(ctx, nil); cancellation != nil { + if cancellation = batchContextCancellationCause(ctx); cancellation != nil { appendCanceledBatchReports(result, plannedReports[index:]) break } @@ -382,7 +382,8 @@ func RunBatchDetailed(ctx context.Context, req BatchRequest) (*BatchResult, erro copyBatchReportDetails(&item, reportResult) } if err != nil { - if cancellation = batchCancellationCause(ctx, err); cancellation != nil { + if reportCancellation := batchReportCancellationCause(err); reportCancellation != nil { + cancellation = reportCancellation item.Status = "canceled" result.Canceled++ } else { @@ -396,7 +397,7 @@ func RunBatchDetailed(ctx context.Context, req BatchRequest) (*BatchResult, erro } result.Reports = append(result.Reports, item) if cancellation == nil { - cancellation = batchCancellationCause(ctx, nil) + cancellation = batchContextCancellationCause(ctx) } if cancellation != nil { appendCanceledBatchReports(result, plannedReports[index+1:]) @@ -404,7 +405,11 @@ func RunBatchDetailed(ctx context.Context, req BatchRequest) (*BatchResult, erro } } result.Total = len(result.Reports) - batchNotification := notifyBatch(ctx, req.Config, req.Batch, batchRunID(startedAt, req.Batch), startedAt, result, plannedReports, req.Notifier) + batchNotification := notifyBatch(batchNotificationInput{ + ctx: ctx, cancellation: cancellation, cfg: req.Config, batch: req.Batch, + runID: batchRunID(startedAt, req.Batch), startedAt: startedAt, + result: result, planned: plannedReports, notifier: req.Notifier, + }) if batchNotification != nil { result.Notification = batchNotification } @@ -414,12 +419,7 @@ func RunBatchDetailed(ctx context.Context, req BatchRequest) (*BatchResult, erro return nil, fmt.Errorf("run is not implemented") } -func batchCancellationCause(ctx context.Context, err error) error { - if ctx != nil { - if contextErr := ctx.Err(); contextErr != nil { - return contextErr - } - } +func batchReportCancellationCause(err error) error { if errors.Is(err, context.Canceled) { return context.Canceled } @@ -429,6 +429,13 @@ func batchCancellationCause(ctx context.Context, err error) error { return nil } +func batchContextCancellationCause(ctx context.Context) error { + if ctx == nil { + return nil + } + return ctx.Err() +} + func appendCanceledBatchReports(result *BatchResult, plannedReports []plannedBatchReport) { if result == nil { return diff --git a/internal/app/batch_generation_test.go b/internal/app/batch_generation_test.go index bba053e..fc084f8 100644 --- a/internal/app/batch_generation_test.go +++ b/internal/app/batch_generation_test.go @@ -9,6 +9,7 @@ import ( "testing" "gitea.maximumdirect.net/eric/weatherreporter/internal/config" + "gitea.maximumdirect.net/eric/weatherreporter/internal/promptexec" ) func TestRunBatchDetailedKeepsSuccessfulOutputAndSkipsNotificationAfterPartialFailure(t *testing.T) { @@ -54,6 +55,48 @@ func TestRunBatchDetailedStopsAfterReportCancellation(t *testing.T) { } } +func TestRunBatchDetailedPreservesIndependentFailureDuringCancellation(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + bundle := generationBundle(t) + bundle.Hourly.Periods = bundle.Hourly.Periods[:1] + notifier := &generationNotifier{} + executor := &generationExecutor{ + executeErr: errors.New("independent report failure"), + beforeExecute: func(promptexec.ExecuteRequest) { + cancel() + }, + } + + result, err := RunBatchDetailed(ctx, BatchRequest{ + Config: generationDistributorConfig(), Batch: BatchMorning, + Now: generationTime("2026-05-29T08:30:00-05:00"), WorkingDir: t.TempDir(), OutputDir: t.TempDir(), + Collector: &generationCollector{bundle: &bundle}, Executor: executor, Notifier: notifier, + }) + if !errors.Is(err, context.Canceled) || result == nil || result.Total != 2 || result.Succeeded != 0 || result.Failed != 1 || result.Canceled != 1 || notifier.batchCalls != 0 || result.Notification == nil || result.Notification.Status != "skipped" || result.Notification.Reason != "batch canceled" { + t.Fatalf("RunBatchDetailed() result/error/notifier = %#v/%v/%#v", result, err, notifier) + } + if result.Reports[0].Status != "failed" || result.Reports[0].Error == "" || result.Reports[1].Status != "canceled" { + t.Fatalf("report results = %#v", result.Reports) + } +} + +func TestNotifyBatchSkipsCancellationObservedAfterReportsComplete(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + notifier := &generationNotifier{} + + result := notifyBatch(batchNotificationInput{ + ctx: ctx, cfg: generationDistributorConfig(), batch: BatchMorning, + runID: "run-id", startedAt: generationTime("2026-05-29T08:30:00-05:00"), + result: &BatchResult{Total: 1, Succeeded: 1, Reports: []BatchReportResult{{Status: "succeeded"}}}, + notifier: notifier, + }) + + if result == nil || result.Status != "skipped" || result.Reason != "batch canceled" || notifier.batchCalls != 0 { + t.Fatalf("notifyBatch() result/notifier = %#v/%#v", result, notifier) + } +} + func TestRunBatchDetailedRetainsPublishedReportBeforeCancellation(t *testing.T) { for _, cause := range []error{context.Canceled, context.DeadlineExceeded} { t.Run(cause.Error(), func(t *testing.T) { diff --git a/internal/app/batch_notification.go b/internal/app/batch_notification.go index a1a45af..005597d 100644 --- a/internal/app/batch_notification.go +++ b/internal/app/batch_notification.go @@ -42,47 +42,59 @@ type batchNotifier interface { NotifyBatch(context.Context, batchNotificationRequest) (*NotificationResult, error) } +type batchNotificationInput struct { + ctx context.Context + cancellation error + cfg config.Config + batch BatchKind + runID string + startedAt time.Time + result *BatchResult + planned []plannedBatchReport + notifier Notifier +} + func batchRunID(startedAt time.Time, batch BatchKind) string { return startedAt.UTC().Format(runIDTimestampLayout) + "_" + string(batch) } -func notifyBatch(ctx context.Context, cfg config.Config, batch BatchKind, runID string, startedAt time.Time, result *BatchResult, planned []plannedBatchReport, notifier Notifier) *BatchNotificationResult { - if !cfg.Notify.Distributor.Enabled { +func notifyBatch(input batchNotificationInput) *BatchNotificationResult { + if !input.cfg.Notify.Distributor.Enabled { return nil } - if !cfg.Notify.Distributor.Batch.Enabled { + if !input.cfg.Notify.Distributor.Batch.Enabled { return nil } - if result == nil { + if input.result == nil { return failedBatchNotificationResult(batchNotificationRequest{}, fmt.Errorf("batch result is required")) } - if result.Canceled > 0 { + if input.cancellation != nil || batchContextCancellationCause(input.ctx) != nil || input.result.Canceled > 0 { return &BatchNotificationResult{ Status: "skipped", Reason: "batch canceled", } } - if result.Failed > 0 { + if input.result.Failed > 0 { return &BatchNotificationResult{ Status: "skipped", Reason: "one or more reports failed", } } - req, err := buildBatchNotificationRequest(cfg, batch, runID, startedAt, result.Reports, planned) + req, err := buildBatchNotificationRequest(input.cfg, input.batch, input.runID, input.startedAt, input.result.Reports, input.planned) if err != nil { return failedBatchNotificationResult(batchNotificationRequest{}, err) } - batchNotifier, err := resolveBatchNotifier(cfg, notifier) + batchNotifier, err := resolveBatchNotifier(input.cfg, input.notifier) if err != nil { return failedBatchNotificationResult(req, err) } - notification, notifyErr := batchNotifier.NotifyBatch(ctx, req) + notification, notifyErr := batchNotifier.NotifyBatch(input.ctx, req) wrappedErr := notifyErr if notifyErr != nil { - wrappedErr = fmt.Errorf("notify batch %q run %q bundle %q: %w", batch, runID, req.BundleID, notifyErr) + wrappedErr = fmt.Errorf("notify batch %q run %q bundle %q: %w", input.batch, input.runID, req.BundleID, notifyErr) } batchResult := batchNotificationResult(req, notification) if wrappedErr != nil { diff --git a/internal/app/comparison_execution_test.go b/internal/app/comparison_execution_test.go index d792385..d072c3f 100644 --- a/internal/app/comparison_execution_test.go +++ b/internal/app/comparison_execution_test.go @@ -101,6 +101,9 @@ func TestExecuteComparisonProfilesUsesDistinctDeterministicDebugReferences(t *te {ProfileID: "deep/two", BackendID: "cloud", ModelName: "deep"}, } debugWriter, err := promptdebug.NewPromptDebugWriter(t.TempDir()) + if errors.Is(err, promptdebug.ErrSecureCaptureUnsupported) { + t.Skipf("secure prompt debug capture is unavailable: %v", err) + } if err != nil { t.Fatalf("NewPromptDebugWriter() error = %v", err) } diff --git a/internal/app/generation_test.go b/internal/app/generation_test.go index 6097ca7..d570c1a 100644 --- a/internal/app/generation_test.go +++ b/internal/app/generation_test.go @@ -13,8 +13,10 @@ import ( "gitea.maximumdirect.net/eric/weatherreporter/internal/collect" "gitea.maximumdirect.net/eric/weatherreporter/internal/config" + "gitea.maximumdirect.net/eric/weatherreporter/internal/promptdebug" "gitea.maximumdirect.net/eric/weatherreporter/internal/promptexec" "gitea.maximumdirect.net/eric/weatherreporter/internal/report" + "gitea.maximumdirect.net/eric/weatherreporter/internal/testutil" "gitea.maximumdirect.net/eric/weatherreporter/internal/weatherdata" ) @@ -510,7 +512,7 @@ func TestGenerateDetailedDoesNotReplaceSymbolicLinkOutput(t *testing.T) { t.Fatal(err) } outputPath := filepath.Join(dir, "daily.md") - requireSymlink(t, backing, outputPath) + testutil.RequireSymlink(t, backing, outputPath) bundle := generationBundle(t) collector := &generationCollector{bundle: &bundle} executor := &generationExecutor{} @@ -531,6 +533,9 @@ func TestGenerateDetailedWritesRequestedPromptDebugArtifacts(t *testing.T) { Date: generationTime("2026-05-29T12:00:00-05:00"), Now: generationTime("2026-05-29T08:30:00-05:00"), WorkingDir: t.TempDir(), LLMDebugDir: debugRoot, Collector: &generationCollector{bundle: &bundle}, Executor: &generationExecutor{}, }) + if errors.Is(err, promptdebug.ErrSecureCaptureUnsupported) { + t.Skipf("secure prompt debug capture is unavailable: %v", err) + } if err != nil || result == nil || result.LLMDebugPath == "" { t.Fatalf("GenerateDetailed() result/error = %#v/%v", result, err) } diff --git a/internal/app/output_test.go b/internal/app/output_test.go index 2d655fa..e5308a1 100644 --- a/internal/app/output_test.go +++ b/internal/app/output_test.go @@ -4,6 +4,8 @@ import ( "os" "path/filepath" "testing" + + "gitea.maximumdirect.net/eric/weatherreporter/internal/testutil" ) func TestResolveComparisonOutputDirectory(t *testing.T) { @@ -46,7 +48,7 @@ func TestResolveComparisonOutputDirectory(t *testing.T) { func TestResolveOutputDirRejectsDanglingSymlinkComponents(t *testing.T) { workingDir := t.TempDir() dangling := filepath.Join(workingDir, "dangling") - requireSymlink(t, filepath.Join(workingDir, "missing"), dangling) + testutil.RequireSymlink(t, filepath.Join(workingDir, "missing"), dangling) for _, directory := range []string{dangling, filepath.Join(dangling, "reports")} { t.Run(filepath.Base(directory), func(t *testing.T) { @@ -61,7 +63,7 @@ func TestResolveOutputDirAllowsMissingDirectoryBelowValidSymlink(t *testing.T) { workingDir := t.TempDir() target := t.TempDir() link := filepath.Join(workingDir, "linked") - requireSymlink(t, target, link) + testutil.RequireSymlink(t, target, link) directory := filepath.Join(link, "reports") got, err := resolveOutputDir(workingDir, directory) @@ -69,10 +71,3 @@ func TestResolveOutputDirAllowsMissingDirectoryBelowValidSymlink(t *testing.T) { t.Fatalf("resolveOutputDir() = %q, %v, want %q, nil", got, err, directory) } } - -func requireSymlink(t *testing.T, target string, link string) { - t.Helper() - if err := os.Symlink(target, link); err != nil { - t.Skipf("symlink support is unavailable: %v", err) - } -} diff --git a/internal/app/profile_execution_test.go b/internal/app/profile_execution_test.go index 0b793ba..0f5c178 100644 --- a/internal/app/profile_execution_test.go +++ b/internal/app/profile_execution_test.go @@ -37,6 +37,9 @@ func TestExecutePreparedProfileRendersWithoutPublishing(t *testing.T) { func TestExecutePreparedProfileKeepsDebugCallbackFailureLocal(t *testing.T) { prepared, inspection := preparedDailyProfile(t) debugWriter, err := promptdebug.NewPromptDebugWriter(t.TempDir()) + if errors.Is(err, promptdebug.ErrSecureCaptureUnsupported) { + t.Skipf("secure prompt debug capture is unavailable: %v", err) + } if err != nil { t.Fatalf("NewPromptDebugWriter() error = %v", err) } diff --git a/internal/cli/root.go b/internal/cli/root.go index b72c99b..1636679 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -36,7 +36,7 @@ Options: --units VALUE Override weather API units. --tz NAME Override weather API timezone. --out PATH Write the generated Markdown report to PATH. - --llm-debug-dir PATH Write sensitive prompt debug artifacts under PATH. + --llm-debug-dir PATH Write sensitive prompt debug artifacts under PATH (Unix only). --profile PROFILE Select a prompt profile for compare; repeat for every profile. --date YYYY-MM-DD Required for generate/compare daily; optional for generate/compare today. --out-dir PATH Write generated Markdown reports beneath PATH for run commands, or select the exact comparison directory. @@ -407,7 +407,7 @@ func addCommonFlags(fs *flag.FlagSet, opts *commonOptions, includeOutput bool) { fs.StringVar(&opts.ConfigPath, "config", "", "configuration file path") fs.StringVar(&opts.Units, "units", "", "weather API units") fs.StringVar(&opts.Timezone, "tz", "", "weather API timezone") - fs.StringVar(&opts.LLMDebugDir, "llm-debug-dir", "", "write sensitive prompt debug artifacts under PATH") + fs.StringVar(&opts.LLMDebugDir, "llm-debug-dir", "", "write sensitive prompt debug artifacts under PATH (Unix only)") if includeOutput { fs.StringVar(&opts.Output, "out", "", "generated Markdown report path") } diff --git a/internal/comparison/publish_test.go b/internal/comparison/publish_test.go index 216c789..d9a34cb 100644 --- a/internal/comparison/publish_test.go +++ b/internal/comparison/publish_test.go @@ -11,6 +11,8 @@ import ( "runtime" "strings" "testing" + + "gitea.maximumdirect.net/eric/weatherreporter/internal/testutil" ) func TestPlanDestination(t *testing.T) { @@ -51,11 +53,11 @@ func TestPlanDestination(t *testing.T) { t.Run("symbolic links", func(t *testing.T) { link := filepath.Join(workingDirectory, "link") - requireSymlink(t, empty, link) + testutil.RequireSymlink(t, empty, link) assertDestinationErrorKind(t, workingDirectory, link, false, DestinationSymlink) dangling := filepath.Join(workingDirectory, "dangling") - requireSymlink(t, filepath.Join(workingDirectory, "missing"), dangling) + testutil.RequireSymlink(t, filepath.Join(workingDirectory, "missing"), dangling) assertDestinationErrorKind(t, workingDirectory, dangling, false, DestinationSymlink) assertDestinationErrorKind(t, workingDirectory, filepath.Join(dangling, "child"), false, DestinationInspection) }) @@ -116,7 +118,7 @@ func TestRecognizeBundleRejectsUnsafeAndMalformedContents(t *testing.T) { if err := os.Remove(path); err != nil { t.Fatal(err) } - requireSymlink(t, filepath.Join(directory, DataPackageFilename), path) + testutil.RequireSymlink(t, filepath.Join(directory, DataPackageFilename), path) }}, {name: "digest mismatch", mutate: func(t *testing.T, directory string) { t.Helper() @@ -365,7 +367,7 @@ func TestPublishReauthorizesMovedDestination(t *testing.T) { }, mutate: func(t *testing.T, target string) { t.Helper() - requireSymlink(t, "unrelated-target", target) + testutil.RequireSymlink(t, "unrelated-target", target) }, verify: func(t *testing.T, target string) { t.Helper() @@ -430,13 +432,6 @@ func TestPublishReauthorizesMovedDestination(t *testing.T) { } } -func requireSymlink(t *testing.T, target string, link string) { - t.Helper() - if err := os.Symlink(target, link); err != nil { - t.Skipf("symlink support is unavailable: %v", err) - } -} - func TestPublishRetainsUnauthorizedMovedDestinationWhenRestoreFails(t *testing.T) { workingDirectory := t.TempDir() target := filepath.Join(workingDirectory, "comparison-daily") diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 3e9a7fc..e5a3395 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -11,6 +11,7 @@ import ( "gitea.maximumdirect.net/eric/weatherreporter/internal/module" "gitea.maximumdirect.net/eric/weatherreporter/internal/report" + "gitea.maximumdirect.net/eric/weatherreporter/internal/testutil" "gopkg.in/yaml.v3" ) @@ -2000,9 +2001,7 @@ func TestLoadSecretsRejectsInvalidDirectoryEntries(t *testing.T) { if err := os.WriteFile(target, []byte("secret-value"), 0o600); err != nil { t.Fatalf("write target: %v", err) } - if err := os.Symlink(target, filepath.Join(dir, "SYMLINK")); err != nil { - t.Fatalf("create symlink: %v", err) - } + testutil.RequireSymlink(t, target, filepath.Join(dir, "SYMLINK")) }, wantErr: "not a symlink", }, diff --git a/internal/fileutil/fileutil_test.go b/internal/fileutil/fileutil_test.go index 0b30465..6ddfa82 100644 --- a/internal/fileutil/fileutil_test.go +++ b/internal/fileutil/fileutil_test.go @@ -7,6 +7,8 @@ import ( "path/filepath" "strings" "testing" + + "gitea.maximumdirect.net/eric/weatherreporter/internal/testutil" ) func TestWriteFileAtomicCreatesParentDirectory(t *testing.T) { @@ -96,9 +98,7 @@ func TestWriteFileAtomicRejectsUnsafeFinalDestinations(t *testing.T) { name: "symbolic link", setup: func(t *testing.T, path string) { t.Helper() - if err := os.Symlink(backing, path); err != nil { - t.Fatal(err) - } + testutil.RequireSymlink(t, backing, path) }, }, } { diff --git a/internal/promptdebug/debug_writer.go b/internal/promptdebug/debug_writer.go index ad7aa52..94210ed 100644 --- a/internal/promptdebug/debug_writer.go +++ b/internal/promptdebug/debug_writer.go @@ -4,6 +4,7 @@ package promptdebug import ( "encoding/json" + "errors" "fmt" "net/url" "path/filepath" @@ -14,6 +15,11 @@ import ( "gitea.maximumdirect.net/eric/weatherreporter/internal/report" ) +// ErrSecureCaptureUnsupported reports that the host cannot provide the +// handle-relative, no-follow filesystem operations required for prompt debug +// artifacts. +var ErrSecureCaptureUnsupported = errors.New("secure prompt debug capture is unavailable on this platform") + const ( promptPreparationDebugSchemaVersion = "weatherreporter.prompt_preparation_debug.v2" promptExecutionDebugSchemaVersion = "weatherreporter.prompt_execution_debug.v2" diff --git a/internal/promptdebug/debug_writer_other_test.go b/internal/promptdebug/debug_writer_other_test.go new file mode 100644 index 0000000..7fb99ce --- /dev/null +++ b/internal/promptdebug/debug_writer_other_test.go @@ -0,0 +1,28 @@ +//go:build !unix + +package promptdebug + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +func TestPromptDebugWriterFailsClosedWithoutSecureTraversal(t *testing.T) { + root := filepath.Join(t.TempDir(), "debug") + writer, err := NewPromptDebugWriter(root) + if writer != nil || !errors.Is(err, ErrSecureCaptureUnsupported) { + t.Fatalf("NewPromptDebugWriter() = %#v, %v, want unsupported error", writer, err) + } + if _, statErr := os.Stat(root); !os.IsNotExist(statErr) { + t.Fatalf("prompt debug root was accessed: %v", statErr) + } +} + +func TestDisabledPromptDebugWriterRemainsPortable(t *testing.T) { + writer, err := NewPromptDebugWriter("") + if err != nil || writer == nil || writer.Enabled() { + t.Fatalf("NewPromptDebugWriter(empty) = %#v, %v", writer, err) + } +} diff --git a/internal/promptdebug/debug_writer_test.go b/internal/promptdebug/debug_writer_test.go index a450015..eb2f3eb 100644 --- a/internal/promptdebug/debug_writer_test.go +++ b/internal/promptdebug/debug_writer_test.go @@ -1,10 +1,11 @@ +//go:build unix + package promptdebug import ( "fmt" "os" "path/filepath" - "runtime" "strings" "sync" "testing" @@ -12,6 +13,7 @@ import ( "gitea.maximumdirect.net/eric/weatherreporter/internal/promptexec" "gitea.maximumdirect.net/eric/weatherreporter/internal/report" + "gitea.maximumdirect.net/eric/weatherreporter/internal/testutil" ) func TestPromptDebugWriterWritesIsolatedArtifacts(t *testing.T) { @@ -58,12 +60,10 @@ func TestPromptDebugWriterWritesIsolatedArtifacts(t *testing.T) { t.Fatalf("debug artifact contains credentials:\n%s", data) } } - if runtime.GOOS != "windows" { - assertPromptDebugMode(t, root, debugDirectoryMode) - assertPromptDebugMode(t, preparationDir, debugDirectoryMode) - assertPromptDebugMode(t, filepath.Join(preparationDir, "preparation.json"), debugFileMode) - assertPromptDebugMode(t, filepath.Join(executionDir, "execution.json"), debugFileMode) - } + assertPromptDebugMode(t, root, debugDirectoryMode) + assertPromptDebugMode(t, preparationDir, debugDirectoryMode) + assertPromptDebugMode(t, filepath.Join(preparationDir, "preparation.json"), debugFileMode) + assertPromptDebugMode(t, filepath.Join(executionDir, "execution.json"), debugFileMode) } func TestPromptDebugWriterProjectsProviderConfigurationSafely(t *testing.T) { @@ -222,21 +222,16 @@ func TestPromptDebugWriterCreatesSharedMissingAncestorsConcurrently(t *testing.T if len(directories) != writerCount { t.Fatalf("debug directories = %#v", directories) } - if runtime.GOOS != "windows" { - assertPromptDebugMode(t, root, debugDirectoryMode) - assertPromptDebugMode(t, filepath.Join(root, "daily"), debugDirectoryMode) - assertPromptDebugMode(t, filepath.Join(root, "daily", "2026-05-29"), debugDirectoryMode) - for directory := range directories { - assertPromptDebugMode(t, directory, debugDirectoryMode) - assertPromptDebugMode(t, filepath.Join(directory, "preparation.json"), debugFileMode) - } + assertPromptDebugMode(t, root, debugDirectoryMode) + assertPromptDebugMode(t, filepath.Join(root, "daily"), debugDirectoryMode) + assertPromptDebugMode(t, filepath.Join(root, "daily", "2026-05-29"), debugDirectoryMode) + for directory := range directories { + assertPromptDebugMode(t, directory, debugDirectoryMode) + assertPromptDebugMode(t, filepath.Join(directory, "preparation.json"), debugFileMode) } } func TestPromptDebugWriterKeepsWritesAnchoredToOpenedRoot(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("directory replacement behavior is covered on Unix hosts") - } parent := t.TempDir() root := filepath.Join(parent, "debug") writer, err := NewPromptDebugWriter(root) @@ -253,7 +248,7 @@ func TestPromptDebugWriterKeepsWritesAnchoredToOpenedRoot(t *testing.T) { if err := os.Rename(root, anchoredRoot); err != nil { t.Fatalf("replace opened root: %v", err) } - requireSymlink(t, outside, root) + testutil.RequireSymlink(t, outside, root) ref := promptDebugRef() ref.RunID = "run-anchored" @@ -299,16 +294,14 @@ func TestPromptDebugWriterRejectsUnsafeRootsAndReferences(t *testing.T) { if _, err := NewPromptDebugWriter(nonDirectory); err == nil { t.Fatal("NewPromptDebugWriter(file) error = nil") } - if runtime.GOOS != "windows" { - target := filepath.Join(root, "target") - link := filepath.Join(root, "root-link") - if err := os.Mkdir(target, debugDirectoryMode); err != nil { - t.Fatalf("create root symlink target: %v", err) - } - requireSymlink(t, target, link) - if _, err := NewPromptDebugWriter(link); err == nil { - t.Fatal("NewPromptDebugWriter(symlink) error = nil") - } + target := filepath.Join(root, "target") + link := filepath.Join(root, "root-link") + if err := os.Mkdir(target, debugDirectoryMode); err != nil { + t.Fatalf("create root symlink target: %v", err) + } + testutil.RequireSymlink(t, target, link) + if _, err := NewPromptDebugWriter(link); err == nil { + t.Fatal("NewPromptDebugWriter(symlink) error = nil") } writer, err := NewPromptDebugWriter(filepath.Join(root, "debug")) @@ -325,22 +318,13 @@ func TestPromptDebugWriterRejectsUnsafeRootsAndReferences(t *testing.T) { t.Fatalf("WritePreparation(%#v) error = nil", ref) } } - if runtime.GOOS != "windows" { - outside := filepath.Join(root, "outside") - if err := os.Mkdir(outside, debugDirectoryMode); err != nil { - t.Fatalf("create symlink component target: %v", err) - } - requireSymlink(t, outside, filepath.Join(root, "debug", "daily")) - if _, err := writer.WritePreparation(promptDebugRef(), promptDebugPreparationFixture(), nil); err == nil { - t.Fatal("WritePreparation(symlink component) error = nil") - } + outside := filepath.Join(root, "outside") + if err := os.Mkdir(outside, debugDirectoryMode); err != nil { + t.Fatalf("create symlink component target: %v", err) } -} - -func requireSymlink(t *testing.T, target string, link string) { - t.Helper() - if err := os.Symlink(target, link); err != nil { - t.Skipf("symlink support is unavailable: %v", err) + testutil.RequireSymlink(t, outside, filepath.Join(root, "debug", "daily")) + if _, err := writer.WritePreparation(promptDebugRef(), promptDebugPreparationFixture(), nil); err == nil { + t.Fatal("WritePreparation(symlink component) error = nil") } } diff --git a/internal/promptdebug/secure_directory_other.go b/internal/promptdebug/secure_directory_other.go index 99d3320..e6159ed 100644 --- a/internal/promptdebug/secure_directory_other.go +++ b/internal/promptdebug/secure_directory_other.go @@ -3,141 +3,29 @@ package promptdebug import ( - "crypto/rand" - "encoding/hex" - "encoding/json" "fmt" - "io" - "os" "path/filepath" - "strings" ) -type secureDirectory struct { - root *os.Root -} +// secureDirectory has no enabled implementation on platforms that cannot +// provide handle-relative, no-follow directory traversal. +type secureDirectory struct{} func openSecureDirectory(path string) (*secureDirectory, error) { if !filepath.IsAbs(path) { return nil, fmt.Errorf("directory must be absolute") } - if err := os.MkdirAll(path, debugDirectoryMode); err != nil { - return nil, err - } - info, err := os.Lstat(path) - if err != nil { - return nil, err - } - if info.Mode()&os.ModeSymlink != 0 || !info.IsDir() { - return nil, fmt.Errorf("prompt debug root is not a directory") - } - if err := os.Chmod(path, debugDirectoryMode); err != nil { - return nil, err - } - root, err := os.OpenRoot(path) - if err != nil { - return nil, err - } - return &secureDirectory{root: root}, nil + return nil, ErrSecureCaptureUnsupported } -func (directory *secureDirectory) Close() error { - if directory == nil || directory.root == nil { - return nil - } - root := directory.root - directory.root = nil - return root.Close() -} - -func (directory *secureDirectory) openDirectory(components ...string) (*secureDirectory, error) { - if directory == nil || directory.root == nil { - return nil, fmt.Errorf("prompt debug directory is closed") - } - for _, component := range components { - if err := validateSecureDirectoryName(component); err != nil { - return nil, err - } - } - path := filepath.Join(components...) - if err := directory.root.MkdirAll(path, debugDirectoryMode); err != nil { - return nil, err - } - child, err := directory.root.OpenRoot(path) - if err != nil { - return nil, err - } - return &secureDirectory{root: child}, nil -} - -func validateSecureDirectoryName(name string) error { - if name == "" || name == "." || name == ".." || strings.ContainsRune(name, filepath.Separator) { - return fmt.Errorf("prompt debug directory component %q is invalid", name) - } +func (*secureDirectory) Close() error { return nil } -func (directory *secureDirectory) writeJSON(name string, value any) error { - if directory == nil || directory.root == nil { - return fmt.Errorf("prompt debug directory is closed") - } - if err := validateSecureDirectoryName(name); err != nil { - return err - } - data, err := json.MarshalIndent(value, "", " ") - if err != nil { - return fmt.Errorf("marshal prompt debug artifact: %w", err) - } - if info, err := directory.root.Lstat(name); err == nil { - if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() { - return fmt.Errorf("prompt debug file %q is not a regular file", name) - } - } else if !os.IsNotExist(err) { - return err - } - temporaryName, temporary, err := directory.createTemporaryFile(name) - if err != nil { - return err - } - defer func() { - if temporary != nil { - _ = temporary.Close() - } - _ = directory.root.Remove(temporaryName) - }() - if err := temporary.Chmod(debugFileMode); err != nil { - return err - } - if written, err := temporary.Write(data); err != nil { - return err - } else if written != len(data) { - return io.ErrShortWrite - } - if err := temporary.Close(); err != nil { - return err - } - temporary = nil - if err := directory.root.Rename(temporaryName, name); err != nil { - return fmt.Errorf("replace prompt debug file %q: %w", name, err) - } - return nil +func (*secureDirectory) openDirectory(...string) (*secureDirectory, error) { + return nil, ErrSecureCaptureUnsupported } -func (directory *secureDirectory) createTemporaryFile(name string) (string, *os.File, error) { - for attempt := 0; attempt < 16; attempt++ { - random := make([]byte, 12) - if _, err := rand.Read(random); err != nil { - return "", nil, err - } - temporaryName := "." + name + "." + hex.EncodeToString(random) + ".tmp" - temporary, err := directory.root.OpenFile(temporaryName, os.O_WRONLY|os.O_CREATE|os.O_EXCL, debugFileMode) - if os.IsExist(err) { - continue - } - if err != nil { - return "", nil, err - } - return temporaryName, temporary, nil - } - return "", nil, fmt.Errorf("create temporary prompt debug file: too many name collisions") +func (*secureDirectory) writeJSON(string, any) error { + return ErrSecureCaptureUnsupported } diff --git a/internal/testutil/symlink.go b/internal/testutil/symlink.go new file mode 100644 index 0000000..b5a13cb --- /dev/null +++ b/internal/testutil/symlink.go @@ -0,0 +1,26 @@ +// Package testutil contains test-only helpers shared across package suites. +package testutil + +import ( + "errors" + "io/fs" + "os" + "testing" +) + +// RequireSymlink creates a symbolic link or skips when the host explicitly +// reports that the operation is unsupported or unavailable to the test user. +// Unexpected setup failures remain test failures. +func RequireSymlink(t testing.TB, target, link string) { + t.Helper() + if err := os.Symlink(target, link); err != nil { + if symlinkUnavailable(err) { + t.Skipf("symlink support is unavailable: %v", err) + } + t.Fatalf("create symlink %q -> %q: %v", link, target, err) + } +} + +func symlinkUnavailable(err error) bool { + return errors.Is(err, fs.ErrPermission) || errors.Is(err, errors.ErrUnsupported) || platformSymlinkUnavailable(err) +} diff --git a/internal/testutil/symlink_other.go b/internal/testutil/symlink_other.go new file mode 100644 index 0000000..26de5cb --- /dev/null +++ b/internal/testutil/symlink_other.go @@ -0,0 +1,7 @@ +//go:build !windows + +package testutil + +func platformSymlinkUnavailable(error) bool { + return false +} diff --git a/internal/testutil/symlink_test.go b/internal/testutil/symlink_test.go new file mode 100644 index 0000000..6ffe9b4 --- /dev/null +++ b/internal/testutil/symlink_test.go @@ -0,0 +1,25 @@ +package testutil + +import ( + "errors" + "io/fs" + "testing" +) + +func TestSymlinkUnavailableClassifiesOnlyCapabilityErrors(t *testing.T) { + for _, test := range []struct { + name string + err error + want bool + }{ + {name: "permission", err: fs.ErrPermission, want: true}, + {name: "unsupported", err: errors.ErrUnsupported, want: true}, + {name: "unexpected", err: errors.New("fixture path is invalid"), want: false}, + } { + t.Run(test.name, func(t *testing.T) { + if got := symlinkUnavailable(test.err); got != test.want { + t.Fatalf("symlinkUnavailable(%v) = %t, want %t", test.err, got, test.want) + } + }) + } +} diff --git a/internal/testutil/symlink_windows.go b/internal/testutil/symlink_windows.go new file mode 100644 index 0000000..05950fb --- /dev/null +++ b/internal/testutil/symlink_windows.go @@ -0,0 +1,12 @@ +//go:build windows + +package testutil + +import ( + "errors" + "syscall" +) + +func platformSymlinkUnavailable(err error) bool { + return errors.Is(err, syscall.ERROR_PRIVILEGE_NOT_HELD) +}