diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md deleted file mode 100644 index c589c1b..0000000 --- a/docs/roadmap/implementation.md +++ /dev/null @@ -1,377 +0,0 @@ -# Configurable Output Directory Implementation Plan - -Status: Complete. - -## Purpose And Authority - -This plan implements the accepted [Configurable Output Directory -Roadmap](output-directory.md). The feature roadmap is authoritative for user -intent, scope, policy choices, non-goals, and the desired end state. This -document owns implementation order, concrete changes, verification, and exit -gates. - -The target is one optional `output.directory` configuration value that selects -the ordinary publication directory for both single reports and batches while -preserving explicit CLI overrides, current defaults, stateless execution, -atomic publication, and notification ordering. - -This plan follows the repository's [architecture](../policy/architecture.md), -[documentation](../policy/documentation.md), and -[testing](../policy/testing.md) policies. - -## Implementation Rules - -1. Implement the stages in numeric order. Keep the repository buildable and - the default suite passing at every stage. -2. Use `GOWORK=off` for all Go validation so the sibling workspace cannot - supply unpublished dependency changes. -3. Keep this change within the configurable-output-directory scope. Do not - implement any part of the accepted - [LLM profile comparison roadmap](profile-comparison.md). -4. Preserve one operator-owned Markdown output per generated report. Do not - add a workspace, manifest, history, cache, retention policy, directory scan, - or managed cleanup behavior. -5. Preserve current report filenames, report periods, batch membership, - Promptkit inspection and execution, generated-text validation, rendering, - cancellation, atomic file publication, action summaries, and Distributor - notification ordering. -6. Keep CLI flag parsing and current-working-directory capture in - `internal/cli`, configuration loading and validation in `internal/config`, - and effective destination resolution and publication in `internal/app`. -7. Keep tests deterministic, offline, credential-free, and independent of - process-wide `os.Chdir` or machine-specific directories. Use `t.TempDir()` - and injected working directories. -8. Update current-state documentation only when the corresponding behavior is - implemented. Do not create release notes, select a release version, tag, or - publish a release in these stages. -9. Run `gofmt` on changed Go files and `git diff --check` before completing - every stage. Do not weaken unrelated tests or rewrite historical release - notes. - -## Locked Product And Path Decisions - -Implement exactly this optional configuration shape: - -```yaml -output: - directory: /var/lib/weatherreporter/reports -``` - -The following decisions are fixed: - -- Add `config.OutputConfig` with one `Directory string` field and add - `Config.Output` with YAML key `output`. Do not add other output settings. -- The built-in default is the empty string. Omitted and explicitly empty - values preserve current-directory behavior. -- A whitespace-only configured value is invalid. Preserve a nonblank value as - supplied during configuration loading; do not trim it into a different - filesystem path. -- Configuration loading performs no filesystem lookup and creates no - directory. Runtime output preflight owns path cleaning and filesystem - inspection. -- A relative configured directory resolves against the absolute working - directory captured for the invocation, never against the configuration-file - directory. There is no environment-variable or home-directory expansion. -- A nonexistent configured directory is valid and is created only as needed by - ordinary atomic report publication. -- An existing configured path that is not a directory, or cannot be inspected, - fails through the existing output-preflight boundary before prompt - inspection, weather collection, model execution, or output replacement. -- Single-report precedence is explicit `--out` path, then - `output.directory` plus the report's existing default filename, then the - working directory plus that filename. -- Batch precedence is explicit `--out-dir`, then `output.directory`, then the - working directory. -- An explicit `--out` is the complete file destination. A relative `--out` - remains relative to the invocation working directory and is never rebased - beneath `output.directory`. -- Configuration must still pass strict validation before any command runs. - After a valid configuration loads, explicit output flags completely ignore - the configured directory for runtime resolution and filesystem inspection. - They retain their current absolute and relative semantics. -- Successful result and summary paths remain cleaned absolute paths. Default - filenames and all output, cancellation, and notification safety guarantees - remain unchanged. - -## Stage 1: Add The Configuration Contract - -### Goal - -Make `output.directory` a strict, validated configuration value without -changing runtime destination behavior or current-state documentation yet. - -### Work - -1. In `internal/config/config.go`, add: - - `Output OutputConfig` to `Config` with YAML key `output`; and - - `OutputConfig` containing only `Directory string` with YAML key - `directory`. -2. In `internal/config/defaults.go`, initialize `Output.Directory` to the empty - string explicitly so the public default is visible beside the other - configuration defaults. -3. In `internal/config/validate.go`, reject `Output.Directory` only when it is - nonempty and `strings.TrimSpace` finds no non-whitespace character. Use an - actionable error that identifies `output.directory`. Do not clean, make - absolute, stat, create, or otherwise mutate the path during validation. -4. Rely on the existing `yaml.Decoder.KnownFields(true)` boundary for strict - nested decoding. Do not introduce custom YAML unmarshalling or a second - unknown-field mechanism for `OutputConfig`. -5. Add focused configuration tests that prove: - - the exact empty default; - - omission and explicit `directory: ""` are accepted; - - representative absolute and relative values load without mutation; - - a whitespace-only value fails with `output.directory` context; - - an unknown field beneath `output` fails strict loading; and - - constructed and YAML-loaded configurations receive the same validation - result. -6. Keep the maintained example and current-state documentation unchanged in - this stage; they are updated after runtime behavior lands. - -### Tests - -Run: - -```sh -GOWORK=off go test -count=1 ./internal/config -GOWORK=off go test -count=1 ./... -git diff --check -``` - -### Exit Gate - -The configuration package exposes and validates the additive field, strict -loading rejects malformed output configuration, existing configurations remain -valid, and runtime output behavior is still unchanged. - -## Stage 2: Centralize And Apply Effective Destination Resolution - -### Goal - -Apply the locked precedence to single-report and batch orchestration through -one coherent app-owned output-resolution boundary. - -### Work - -1. Create `internal/app/output.go` and move the existing app-owned output - helpers out of `app.go` without changing their established safety behavior: - `prepareBatchOutputs`, `plannedBatchOutputPath`, - `resolveReportOutputPath`, `resolveOutputDir`, `resolveOutputPath`, - `validateWorkingDir`, and `validateOutputPath`. Clean up imports in - `app.go`; do not export these helpers or introduce a new package. -2. Add one narrow directory-precedence helper in `output.go`. It must select a - nonempty explicit directory first, otherwise the configured directory, and - then pass that selected value to the existing directory resolver, whose - empty case returns the validated working directory. Do not inspect the - lower-precedence configured path when an explicit directory is present. -3. Extend the single-report resolver to accept the configured directory and - implement this exact branch: - - obtain the existing default output name from `report.Resolved`; - - when `GenerateRequest.OutputPath` is nonempty, resolve and validate that - complete explicit path against `WorkingDir` and ignore the configured - directory; - - otherwise resolve the configured directory, falling back to - `WorkingDir`, and place the existing default filename beneath it; and - - validate and return one cleaned absolute final file path. -4. Change `GenerateDetailed` to pass `req.Config.Output.Directory` into that - resolver before debug-writer construction, prompt inspection, weather - collection, or model execution. Preserve its current partial-result and - destination-preservation behavior on resolution failure. -5. Change `RunBatchDetailed` to resolve its base directory from - `req.OutputDir`, `req.Config.Output.Directory`, and `req.WorkingDir` in that - order before debug-writer construction, prompt inspection, or collection. - Preserve data-dependent batch planning and the later preflight of every - final report file. -6. Do not add output-directory fields to `GenerateRequest` or `BatchRequest`. - Their existing explicit override fields remain CLI/action inputs, while - the configured fallback remains in `Config`. -7. Add focused `internal/app` tests, using existing real filesystem behavior - and project-owned fakes, that prove: - - omitted configuration retains current-directory destinations for a - representative single report and batch; - - absolute and relative configured directories produce the expected - cleaned absolute destinations; - - single-report `OutputPath` and batch `OutputDir` overrides win and do not - inspect or use a lower-precedence configured path; - - a configured existing non-directory fails before prompt inspection, - collection, model execution, publication, or notification; - - a missing configured directory is created through successful ordinary - publication; - - existing atomic replacement and cancellation tests continue to pass; and - - result paths and Distributor source paths still identify only the final - selected Markdown files. -8. Prefer a compact resolver table plus representative assembled workflows. - Do not duplicate every path case at configuration, CLI, and app layers. - -### Tests - -Run: - -```sh -GOWORK=off go test -count=1 ./internal/app ./internal/fileutil -GOWORK=off go test -count=1 ./... -git diff --check -``` - -### Exit Gate - -Direct app callers, ordinary `generate`, and batches use the locked -CLI/configuration/working-directory precedence; paths are validated before -expensive work; and output, cancellation, and notification invariants remain -intact. - -## Stage 3: Complete CLI Coverage And Current-State Documentation - -### Goal - -Protect the assembled CLI-to-app contract and make every canonical document -and maintained example describe the now-implemented behavior exactly once. - -### Work - -1. Keep `internal/cli.resolveOutputOverride` limited to explicit `--out` and - `--out-dir` values. It must continue resolving relative flag values against - the captured working directory. Do not resolve `output.directory` in the - CLI or replace the raw configured value in `Config`; the app owns final - precedence and runtime path validation. -2. Add or update focused `internal/cli` request-construction tests to prove: - - a configured relative or absolute directory survives configuration - loading and reaches the app request while the absent CLI override remains - empty; - - relative and absolute `--out` and `--out-dir` values retain their current - resolution; and - - an explicit flag coexists with the loaded configured value so app-owned - precedence can ignore the latter. - Keep these tests narrow; app tests own final path and publication outcomes. -3. Add this illustrative stanza to `examples/config.yml`: - - ```yaml - output: - directory: /var/lib/weatherreporter/reports - ``` - - Keep `examples/minimal-config.yml` unchanged so it continues to demonstrate - the empty-default path. Extend `TestLoadExampleConfig` to assert the exact - maintained example value. -4. Update canonical current-state documentation according to - `docs/policy/documentation.md`: - - `docs/config.md` owns the field, empty default, whitespace validation, - absolute and relative semantics, lack of expansion, and configuration - versus flag precedence; - - `docs/cli.md` owns the unchanged flag syntax and the effective - flag/configuration/working-directory selection visible to users; - - `docs/operations.md` owns normal operation with a configured publication - directory, explicit one-command overrides, missing-directory creation, - atomic replacement, and failure handling; - - `README.md` updates only its short quickstart destination summary and - links to the canonical configuration/CLI references; - - `docs/policy/architecture.md` states that configuration or explicit CLI - input selects the operator-owned output destination without changing the - stateless boundary; - - `docs/internal/cli.md` describes capture and transport of the configured - fallback plus explicit overrides without duplicating the field reference; - and - - `docs/internal/app-orchestration.md` describes app-owned effective - destination precedence and the unchanged single and batch preflight - ordering. -5. Do not change historical release notes. Do not duplicate the complete field - contract outside `docs/config.md`, complete flag syntax outside - `docs/cli.md`, or operational procedures outside `docs/operations.md`. -6. Review all changed commands, values, paths, and examples against executable - code. Follow every added or changed repository-relative link and confirm its - target exists. - -### Tests - -Run: - -```sh -GOWORK=off go test -count=1 ./internal/config ./internal/cli ./internal/app -GOWORK=off go run ./cmd/weatherreporter --help -GOWORK=off go test -count=1 ./... -git diff --check -``` - -Confirm the maintained full and minimal examples both load through the -configuration test suite and no current-state document still claims that the -working directory is always the no-flag destination. - -### Exit Gate - -The CLI-to-app boundary is protected without duplicating app behavior tests, -the maintained example is executable configuration, and all canonical -current-state documentation agrees with the implemented contract. - -## Stage 4: Reconcile Roadmaps And Run The Final Gate - -### Goal - -Verify the complete feature in the same module-isolated environment used for -release preparation and leave roadmap status accurate for post-implementation -review. - -### Work - -1. Review the complete diff against the feature roadmap, this plan, and all - documents under `docs/policy/`. Remove incidental refactors, redundant - tests, duplicate documentation, and any behavior outside the accepted - scope. -2. Confirm repository hygiene: - - no tracked `go.work` or `go.work.sum`; - - no `vendor` directory or `go.mod` replacement; - - no generated binaries, test output, credentials, private paths, or - temporary filesystem artifacts; and - - no unintended changes to the profile-comparison feature. -3. After every implementation and documentation gate passes, update - `docs/roadmap/output-directory.md` from `Accepted; unimplemented` to - `Implemented; retained temporarily for post-implementation review`. - Preserve its target-state purpose and policy content until roadmap cleanup - is separately authorized. -4. Update the prerequisite section in - `docs/roadmap/profile-comparison.md` to state that configurable output - directories are implemented. Link its current behavior summary to - `docs/config.md` and `docs/operations.md` rather than duplicating their - contracts. Do not otherwise advance or implement profile comparison. -5. Mark this plan `Status: Complete` only after the final gate succeeds. Do not - delete either roadmap or this plan in this stage; repository roadmap cleanup - follows post-implementation review. - -### Tests - -Run from the repository root: - -```sh -set -eu -test -z "$(git ls-files go.work go.work.sum)" -test ! -e vendor -if grep -Eq '^[[:space:]]*replace([[:space:]]|\()' go.mod; then - printf '%s\n' 'go.mod contains a replacement' >&2 - exit 1 -fi -GOWORK=off go test -count=1 ./... -GOWORK=off go test -race -count=1 ./... -GOWORK=off go vet ./... -GOWORK=off go build ./... -GOWORK=off go mod tidy -diff -unformatted="$(git ls-files '*.go' | while IFS= read -r file; do gofmt -l "$file"; done)" -test -z "$unformatted" -GOWORK=off go run ./cmd/weatherreporter --help >/dev/null -git diff --check -git diff --cached --check -``` - -Follow every added or changed local documentation link and verify the full and -minimal maintained configuration examples through their tests. - -### Exit Gate - -The complete repository passes normal, race-enabled, vet, build, module, -formatting, help, documentation, and hygiene checks with `GOWORK=off`; roadmap -status accurately distinguishes the implemented output-directory prerequisite -from the still-unimplemented profile-comparison feature; and no required work -remains. - -## Open Questions - -None. The feature roadmap and this plan define every product, architecture, -precedence, path, compatibility, testing, documentation, and completion choice -required for implementation. diff --git a/docs/roadmap/output-directory.md b/docs/roadmap/output-directory.md deleted file mode 100644 index f644304..0000000 --- a/docs/roadmap/output-directory.md +++ /dev/null @@ -1,191 +0,0 @@ -# Configurable Output Directory Roadmap - -Status: Implemented; retained temporarily for post-implementation review. - -## Purpose - -Weatherreporter currently writes generated reports beneath the present working -directory unless an operator supplies `--out` for a single report or -`--out-dir` for a batch. Scheduled and packaged deployments should be able to -select their ordinary report directory in configuration without repeating a -CLI flag on every invocation. - -This feature adds one optional nested configuration value: - -```yaml -output: - directory: /var/lib/weatherreporter/reports -``` - -The nested `output` namespace leaves room for future output-related settings, -but this roadmap approves only `output.directory`. - -## User Intent - -The configured directory is an operator-owned publication destination. It is -not an application workspace, history store, cache, or source of input for a -later run. The feature must preserve Weatherreporter's stateless execution -model and atomic report publication. - -Operators should be able to establish a stable default once in `config.yml` -while retaining explicit per-invocation control through the existing CLI -flags. - -## Target Configuration Contract - -`output.directory` is an optional string with an empty default. - -- An omitted or empty value preserves the current behavior: default report - names are published beneath the process's present working directory. -- A nonempty absolute path selects that directory directly. -- A nonempty relative path is resolved against the process's present working - directory, matching the existing relative-path behavior of `--out` and - `--out-dir`. It is not resolved relative to the configuration file. -- A whitespace-only value is invalid rather than being treated as an implicit - current-directory selection. -- A nonexistent directory is allowed and may be created as part of normal - atomic output publication. -- An existing path that is not a directory, an inaccessible destination, or - any other unsafe output target fails through the existing output preflight - and publication contracts. - -The configuration remains strict: unknown fields beneath `output` are rejected -in the same way as unknown fields elsewhere in the configuration. - -## Destination Precedence - -Effective output selection follows this precedence, from highest to lowest: - -1. An explicit `--out` path for `generate`, or an explicit `--out-dir` path for - `run`. -2. `output.directory` from the effective configuration. -3. The process's present working directory. - -For single-report generation: - -- `generate ` without `--out` publishes the report's existing default - filename beneath `output.directory` when configured. -- `generate --out PATH` treats `PATH` as the complete destination and - ignores `output.directory`. -- A relative `--out` value remains relative to the present working directory; - it is not placed beneath `output.directory`. - -For batch generation: - -- `run ` without `--out-dir` publishes every selected report beneath - `output.directory` when configured. -- `run --out-dir PATH` ignores `output.directory` for that invocation. - -Default report filenames, batch membership, summaries, and notification -behavior do not change. - -## Architectural End State - -The configuration package owns the `output.directory` field, its empty -default, YAML decoding, normalization, and configuration-level validation. -The CLI continues to own flag parsing and the present working directory used -for relative CLI values. Application orchestration owns the final precedence -decision, output preflight, default filename placement, atomic publication, and -the resolved output paths returned in action results. - -Single-report and batch paths must use one coherent destination-resolution -policy rather than maintaining independent interpretations of the configured -directory. Output destinations must still be fully resolved and validated -before weather collection or Promptkit execution where the existing workflow -provides that guarantee. - -The generated Markdown remains the only ordinary durable artifact. No -directory scan, prior report lookup, manifest, metadata file, or managed -cleanup behavior is introduced. - -## Scope - -The completed feature includes: - -- an `OutputConfig` configuration boundary containing `directory`; -- an empty default that preserves current installations; -- strict YAML loading and validation for the nested configuration; -- configured-directory fallback for both `generate` and `run`; -- explicit CLI-over-configuration precedence; -- consistent absolute and relative path handling; -- preservation of output preflight, atomic replacement, cancellation, and - notification ordering guarantees; -- updates to the maintained example configuration; -- updates to the canonical configuration, CLI, operations, architecture, and - app-orchestration documentation where their owned contracts change; and -- focused configuration, CLI, and application tests protecting the public - behavior and important failure paths. - -## Compatibility - -This is an additive configuration feature. Existing configuration files remain -valid, and installations that do not set `output.directory` retain the current -working-directory behavior. - -Existing `--out` and `--out-dir` syntax and relative-path semantics remain -unchanged. The feature does not alter report contents, default filenames, -Promptkit execution, Distributor payloads, or exit behavior. - -## Testing Expectations - -Tests should provide durable coverage for: - -- the empty default and YAML loading of absolute and relative directories; -- rejection of whitespace-only and unknown output configuration values; -- current-working-directory fallback when the field is omitted; -- configured-directory use by representative single-report and batch actions; -- `--out` and `--out-dir` precedence over the configured directory; -- preservation of existing relative CLI path semantics; -- rejection of an existing non-directory destination before expensive work; -- successful publication beneath a directory that does not yet exist; and -- preservation of atomic destination replacement and action-result paths. - -Tests must remain deterministic, offline, and independent of machine-specific -directories. Filesystem cases should use temporary directories and exercise -behavior through the narrowest stable configuration, CLI, or application -boundary that owns the contract. - -## Documentation End State - -Once implemented, the exact field definition, default, and path semantics -belong in the [configuration reference](../config.md). CLI flag syntax and -precedence summaries belong in the [CLI reference](../cli.md). Normal output -handling belongs in the [operations guide](../operations.md), while ownership -and workflow mechanics belong in the [architecture policy](../policy/architecture.md) -and [app-orchestration internals](../internal/app-orchestration.md), -respectively. - -The maintained configuration under `examples/` should illustrate the field -without turning another document into a duplicate configuration reference. -Current-state documentation must not describe this feature as available until -the implementation lands. - -## Non-Goals - -This roadmap does not introduce: - -- per-report or per-batch configured output directories; -- filename or directory templates; -- a new `--out-dir` flag for single-report generation; -- environment-variable expansion or home-directory expansion in paths; -- paths resolved relative to the configuration file; -- output retention, rotation, cleanup, indexing, or history; -- configuration-time directory creation; -- multiple output destinations or output format selection; or -- changes to debug artifact placement or Distributor notification routing. - -Any future output settings require their own accepted scope and should be -added beneath `output` only when a concrete requirement exists. - -## Completion Criteria - -The feature is complete when operators can set `output.directory` and obtain -the same destination behavior for ordinary single-report and batch generation, -explicit CLI destinations reliably take precedence, omitted configuration is -fully backward compatible, output safety invariants remain intact, and all -affected canonical documentation and maintained examples describe the -implemented contract. - -## Open Questions - -None. The target behavior and policy choices are defined above. diff --git a/internal/app/output.go b/internal/app/output.go index 8ce4f1b..0153a91 100644 --- a/internal/app/output.go +++ b/internal/app/output.go @@ -67,14 +67,54 @@ func resolveOutputDir(workingDir, override string) (string, error) { directory = filepath.Join(workingDir, directory) } directory = filepath.Clean(directory) - if info, err := os.Stat(directory); err == nil && !info.IsDir() { - return "", fmt.Errorf("output directory %q is not a directory", directory) - } else if err != nil && !os.IsNotExist(err) { - return "", fmt.Errorf("inspect output directory %q: %w", directory, err) + if err := preflightOutputDirectory(directory); err != nil { + return "", err } return directory, nil } +func preflightOutputDirectory(directory string) error { + info, err := os.Stat(directory) + if err == nil { + if !info.IsDir() { + return fmt.Errorf("output directory %q is not a directory", directory) + } + return nil + } + if !os.IsNotExist(err) { + return fmt.Errorf("inspect output directory %q: %w", directory, err) + } + + // A missing directory is valid, but os.Stat also reports ErrNotExist for a + // dangling symlink. Walk to the first existing component so invalid links + // fail preflight instead of being discovered only during publication. + for component := directory; ; component = filepath.Dir(component) { + componentInfo, componentErr := os.Lstat(component) + if componentErr == nil { + if componentInfo.Mode()&os.ModeSymlink != 0 { + targetInfo, targetErr := os.Stat(component) + if targetErr != nil { + return fmt.Errorf("inspect output directory %q at %q: %w", directory, component, targetErr) + } + if !targetInfo.IsDir() { + return fmt.Errorf("output directory %q has non-directory path component %q", directory, component) + } + return nil + } + if !componentInfo.IsDir() { + return fmt.Errorf("output directory %q has non-directory path component %q", directory, component) + } + return nil + } + if !os.IsNotExist(componentErr) { + return fmt.Errorf("inspect output directory %q at %q: %w", directory, component, componentErr) + } + if filepath.Dir(component) == component { + return fmt.Errorf("inspect output directory %q: no existing directory ancestor", directory) + } + } +} + func resolveOutputPath(workingDir, override, defaultName string) (string, error) { workingDir, err := validateWorkingDir(workingDir) if err != nil { diff --git a/internal/app/output_test.go b/internal/app/output_test.go new file mode 100644 index 0000000..9379ea5 --- /dev/null +++ b/internal/app/output_test.go @@ -0,0 +1,38 @@ +package app + +import ( + "os" + "path/filepath" + "testing" +) + +func TestResolveOutputDirRejectsDanglingSymlinkComponents(t *testing.T) { + workingDir := t.TempDir() + dangling := filepath.Join(workingDir, "dangling") + if err := os.Symlink(filepath.Join(workingDir, "missing"), dangling); err != nil { + t.Fatal(err) + } + + for _, directory := range []string{dangling, filepath.Join(dangling, "reports")} { + t.Run(filepath.Base(directory), func(t *testing.T) { + if _, err := resolveOutputDir(workingDir, directory); err == nil { + t.Fatalf("resolveOutputDir(%q) error = nil, want dangling symlink error", directory) + } + }) + } +} + +func TestResolveOutputDirAllowsMissingDirectoryBelowValidSymlink(t *testing.T) { + workingDir := t.TempDir() + target := t.TempDir() + link := filepath.Join(workingDir, "linked") + if err := os.Symlink(target, link); err != nil { + t.Fatal(err) + } + + directory := filepath.Join(link, "reports") + got, err := resolveOutputDir(workingDir, directory) + if err != nil || got != directory { + t.Fatalf("resolveOutputDir() = %q, %v, want %q, nil", got, err, directory) + } +}