Finish output directory follow-up work
This commit is contained in:
@@ -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.
|
||||
Reference in New Issue
Block a user