Complete the Stage 5 CLI audit

docs/roadmap/audit.md

@@ -1,6 +1,6 @@
 # Repository Audit Ledger
 
-Status: In progress; Stages 1-4 complete.
+Status: In progress; Stages 1-5 complete.
 
 This temporary roadmap document is the evidence ledger for the staged audit
 defined by [the audit plan](audit-plan.md). It records audit evidence and
@@ -26,8 +26,15 @@ notification identities may render blank, and programmatically constructed
 report overrides are silently ignored. It also found one low-severity secret
 loading atomicity defect. Defaults, file and CLI precedence, strict YAML field
 handling, active Promptkit settings, output-path preservation, and maintained
-examples otherwise match their contracts. Subsystem conclusions and final
-disposition remain pending the later stages.
+examples otherwise match their contracts. Stage 5 found two medium-severity
+CLI correctness defects: operating-system
+interrupts never become workflow cancellation, and generate validates report
+dates only after constructing its executor. It also found a medium-severity
+test gap around assembled generate and ordinary batch result/exit behavior.
+Command ownership, flag acceptance, output routing, quiet mode, comparison
+error bounding, and the implemented result-to-exit mapping otherwise follow
+the documented contract. Subsystem conclusions and final disposition remain
+pending the later stages.
 
 ## Baseline Metadata
 
@@ -158,7 +165,7 @@ inventory commands, graph index refresh, and graph architecture inspection.
 | 2 | Audit architecture and dependency direction | Complete |
 | 3 | Audit report identity and time foundations | Complete |
 | 4 | Audit configuration, secrets, and validation | Complete |
-| 5 | Audit CLI parsing, wiring, and output contracts | Pending |
+| 5 | Audit CLI parsing, wiring, and output contracts | Complete |
 | 6 | Audit weather data acquisition and collection | Pending |
 | 7 | Audit forecast and fact derivation | Pending |
 | 8 | Audit module contracts, registry, and source-facing briefing modules | Pending |
@@ -193,7 +200,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication.
 | Architecture and dependency direction | Assembled app and CLI tests; graph traces | 2 | Sufficient at the architectural boundary: direct imports are acyclic, external dependency types remain adapter-local, and representative workflow ordering and publication boundaries have focused tests. `AUD-001` records unused persistence APIs that do not participate in normal execution. |
 | Report identity, periods, dates, and timezones | `internal/report`, `internal/timeutil`, plus focused `internal/app` and `internal/forecast` callers | 3 | Partly insufficient. Registry identities, alias policies, batch membership, civil-day boundaries, and elapsed six-hour periods are coherent and exercised. `AUD-002` records incorrect wall-clock windows on daylight-saving transition days, and `AUD-003` records the missing regression test for Daily run-ID uniqueness across dates. |
 | Configuration, validation, and secrets | `internal/config`, focused `internal/cli` and Promptkit adapter mapping, maintained examples | 4 | Partly insufficient. Defaults, precedence, known-field parsing, active URL/duration/template checks, output-path preservation, profile-source rules, secret redaction, and examples are exercised. `AUD-004` through `AUD-007` record unsupported missing-source keys, blank notification identities, constructed-report override inconsistency, and non-atomic secret environment mutation. |
-| CLI parsing, output, and exit behavior | `internal/cli` | 5 | Pending |
+| CLI parsing, output, and exit behavior | `cmd/weatherreporter`, `internal/cli`, representative app-facing CLI tests | 5 | Partly insufficient. Command/flag ownership, paths, one-executor mapping, stream separation, quiet mode, comparison safe errors, and visible result/exit rules are coherent. `AUD-008` records missing signal cancellation, `AUD-009` records late generate date validation, and `AUD-010` records missing assembled non-comparison CLI protection. |
 | Weather transport and normalized collection | `internal/adapters/weatherapi`, `internal/collect`, `internal/weatherdata` | 6 | Pending |
 | Forecast and fact derivation | `internal/forecast`, `internal/facts` | 7 | Pending |
 | Module and briefing contracts | `internal/module`, `internal/briefing` | 8-9 | Pending |
@@ -477,6 +484,125 @@ with evidence about meaningful risks, test ownership, gaps, and duplication.
 - Related findings: none
 - Remediation reference: pending
 
+### AUD-008: Process interrupts do not cancel active workflows
+
+- Stage: 5
+- Status: candidate
+- Severity: medium
+- Confidence: high
+- Category: correctness
+- Area: `cmd/weatherreporter.main` and CLI cancellation propagation
+- Evidence: `main` calls `cli.Run` with `context.Background()`. Repository code
+  search found no `signal.Notify`, `signal.NotifyContext`, `os.Interrupt`, or
+  termination-signal handling. `Runner.Run` correctly forwards the supplied
+  context to generate, batch, and comparison workflows, and focused app tests
+  prove that context cancellation prevents pending publication and joins
+  comparison execution. The production executable never supplies a context
+  that an operator interrupt can cancel, so those paths are reachable only by
+  programmatic callers and tests.
+- Contract at risk: Documented cancellation failures must reach workflow
+  cancellation checks so pre-publication destinations are preserved and a
+  bounded failed result can be finalized when available.
+- Impact: `SIGINT` or `SIGTERM` terminates the process abruptly instead of
+  canceling in-flight collection, Promptkit work, publication preflight, or
+  notification. The CLI cannot emit its documented canceled result path or
+  allow workflow cleanup/coordination to finish, increasing interruption risk
+  around output and comparison transactions.
+- Recommendation: Derive the root context from `signal.NotifyContext` for
+  operator interrupt and termination signals, stop signal delivery on return,
+  and pass that context through the existing CLI boundary. Preserve the
+  current nonzero failure exit and bounded error behavior.
+- Test implications: Add a narrow executable-boundary test or a testable signal
+  context seam proving that an interrupt cancels the context passed to CLI.
+  Keep detailed destination-preservation and goroutine-join assertions with
+  their existing app/publication owners.
+- Validation: An interrupt observed during a controlled blocking action
+  cancels its workflow context, returns the expected failed/canceled result
+  when available, and leaves the existing destination intact; ordinary help,
+  version, and successful exits are unchanged.
+- Related findings: `AUD-010`
+- Remediation reference: pending
+
+### AUD-009: Generate validates report dates after constructing its executor
+
+- Stage: 5
+- Status: candidate
+- Severity: medium
+- Confidence: high
+- Category: correctness
+- Area: `internal/cli.Runner.resolveGenerateAction`
+- Evidence: Generate validates the command name and syntactic flags, then loads
+  configuration and constructs the Promptkit executor before checking that
+  Daily has `--date` or parsing a supplied Daily/Today date. Executor
+  construction can open and parse configured profile files/directories and can
+  fail first. The equivalent comparison resolver validates profile selection
+  before configuration and performs report-date checks before constructing its
+  one executor. Focused comparison tests assert that input failures construct
+  no executor, but generate has no corresponding test. The production
+  `generate daily` diagnostic confirms the missing-date failure is pre-result,
+  but source tracing shows the executor has already been constructed.
+- Contract at risk: Basic command input should be rejected before constructing
+  an external-dependency adapter or performing work unrelated to the invalid
+  request; pre-run errors should identify the user's actionable input failure.
+- Impact: A missing or malformed report date can instead surface a profile
+  source/configuration error, needlessly reads configured Promptkit sources,
+  and follows secret loading from `AUD-007` even though the report request can
+  never execute.
+- Recommendation: After flag parsing, reject a missing Daily date immediately;
+  then load configuration for the effective timezone, parse/default the date,
+  resolve paths, and construct the executor only after all request-local
+  validation succeeds.
+- Test implications: Add generate input-failure cases with an injected executor
+  factory and assert zero factory/application calls and no stdout/stderr for a
+  missing Daily date, malformed Daily/Today date, unsupported flag, and
+  unexpected argument. Avoid duplicating `timeutil` parser cases.
+- Validation: Every generate input failure returns its input diagnostic without
+  constructing an executor or emitting a partial summary; valid requests still
+  construct exactly one executor.
+- Related findings: `AUD-007`, `AUD-010`
+- Remediation reference: pending
+
+### AUD-010: Assembled non-comparison CLI result paths lack durable tests
+
+- Stage: 5
+- Status: candidate
+- Severity: medium
+- Confidence: high
+- Category: testing
+- Area: `internal/cli.Runner.Run`, generate/run command tests, and
+  `cmd/weatherreporter`
+- Evidence: The CLI suite directly tests generate summary field copying and
+  shared output helpers, but has no assembled `Runner.Run` generate success,
+  result-bearing failure, quiet failure, or pre-result failure test. Generate
+  calls `app.GenerateDetailed` directly, unlike the injectable batch and
+  comparison functions, which makes its user-visible wiring difficult to test.
+  Batch has one assembled notification-failure case but no representative
+  ordinary success, report partial failure, quiet failure, or returned-error
+  mapping case. Comparison has comprehensive parser, construction, success,
+  partial failure, cleanup failure, quiet, and pre-execution tests. The command
+  package has no test for executable exit or signal behavior.
+- Contract at risk: Generate and run must translate result presence, action
+  status, quiet mode, stdout JSON, batch stderr lines, returned errors, and the
+  executable's success/failure exit consistently with the CLI reference.
+- Impact: A change can suppress a required failed summary, emit routine output
+  in quiet mode, return success for a failed result, write to the wrong stream,
+  or construct dependencies for invalid input without a focused CLI test
+  failing, even while summary-helper and lower app tests remain green.
+- Recommendation: Add the smallest generate application injection seam
+  symmetrical with batch/comparison, then cover one success, one result-bearing
+  failure, quiet failure, and pre-result failure. Add only representative batch
+  success/report-failure cases and one executable exit/cancellation check;
+  leave workflow details with app tests.
+- Test implications: Prefer structured JSON/status/error assertions and typed
+  errors over complete output snapshots or private call choreography. These
+  tests should own CLI translation, not repeat publication, collection, prompt,
+  or notification mechanics.
+- Validation: Mutations to result/error branching, quiet gating, stream
+  selection, or failure return status break a focused CLI test, while internal
+  app refactors that preserve request/result contracts require no CLI edits.
+- Related findings: `AUD-008`, `AUD-009`
+- Remediation reference: pending
+
 ## Retained Decisions
 
 ### RET-001: Keep the application package as the explicit composition owner
@@ -590,6 +716,29 @@ profile parsing into config would duplicate dependency-specific behavior and
 leak Promptkit mechanics. Reconsider only if Weatherreporter adopts a
 repository-owned profile format independent of Promptkit.
 
+### RET-011: Keep command-specific parsers around one shared flag core
+
+The three action parsers use separate `flag.FlagSet` values so unsupported
+flags fail at the command that owns them: generate alone accepts `--out`, run
+and compare accept `--out-dir`, compare alone accepts repeated `--profile` and
+`--replace`, and only Daily/Today generate or compare accept `--date`.
+`addCommonFlags` shares only config, units, timezone, and prompt-debug options.
+This small explicit split keeps the acceptance matrix reviewable without a
+general command framework. Reconsider if additional actions make the common
+registration or post-parse checks materially larger or divergent.
+
+### RET-012: Keep action summaries distinct and bounded by CLI ownership
+
+Generate, batch, and comparison have different public result contracts, so
+separate summary types and constructors are preferable to a generic result
+envelope. Generate copies selected fields and a compact notification result;
+batch preserves ordered report outcomes and independent notification status;
+comparison locks its field order and converts top-level failures to bounded
+safe categories. Pre-result failures emit no JSON, while result-bearing
+failures emit a failed summary before returning an error, and one shared output
+gate implements quiet mode. Reconsider common abstraction only if multiple
+actions acquire the same complete summary and failure semantics.
+
 ## Open Questions
 
 No Stage 1 open questions or unexplained baseline failures remain.
@@ -640,6 +789,30 @@ Stage 4 routed these investigation leads to their assigned later stages:
   idempotency remain assigned to Stage 16. Stage 4 accounts only for active
   configuration and safe identity/path rendering before execution.
 
+Stage 5 routed these investigation leads to their assigned later stages:
+
+- Generate and batch summaries intentionally carry project-owned app error and
+  warning fields, while comparison applies an additional bounded safe-error
+  translation. Stages 6, 11, and 16 should verify that Weather API, Promptkit,
+  and Distributor producers never place sensitive endpoint, credential, or
+  provider detail in the non-comparison fields before Stage 24 judges the full
+  documented output claim.
+- Generate and comparison repeat the Daily/Today flag-acceptance and date
+  defaulting policy. They currently agree, and the report registry remains the
+  period owner. Stage 23 should assess whether a shared CLI policy descriptor
+  would reduce drift without moving parsing into the domain registry.
+- The static help text includes `--date` in usage but omits it from the Options
+  list and describes `--quiet` as suppressing successful output even though it
+  suppresses failed action summaries too. `docs/internal/cli.md` also still
+  says the CLI dispatches only generation and batch actions despite implemented
+  comparison dispatch. Stage 24 should reconcile these documentation/help
+  statements without expanding Stage 5 into the repository-wide documentation
+  audit.
+- Batch status writes deliberately ignore stderr write failures while JSON
+  encoding errors are returned. Stage 25 should exercise broken-pipe and output
+  writer failures dynamically before deciding whether routine-status I/O must
+  affect the process exit.
+
 ## Stage Log
 
 ### Stage 1: Establish The Baseline And Audit Ledger
@@ -822,3 +995,56 @@ Stage 4 routed these investigation leads to their assigned later stages:
 - Retained decisions: `RET-008`, `RET-009`, and `RET-010`.
 - Open questions: the four leads recorded above are routed to their assigned
   later stages.
+
+### Stage 5: Audit CLI Parsing, Wiring, And Output Contracts
+
+- Status: Complete.
+- Scope reviewed: `cmd/weatherreporter`, all production and representative
+  tests in `internal/cli`, `docs/cli.md`, `docs/internal/cli.md`, immediate
+  config/report/time/executor mapping, and app request/result/error boundaries.
+  Comparison parsing and CLI translation were reviewed, while detailed
+  comparison execution remains excluded.
+- Exclusions: Weather collection, report generation, publication, notification,
+  and comparison execution correctness remain assigned to Stages 6-18.
+  Cross-cutting parser deduplication, test portfolio durability, documentation
+  coherence, and dynamic robustness remain assigned to Stages 21 and 23-25.
+
+#### Command And Exit Accounting
+
+| Command or risk | Parsing, result, and exit evidence | Disposition |
+| --- | --- | --- |
+| Root help and version | No arguments, `-h`, and `--help` print static help without config/provider setup. Exact `--version` prints the injected or build version; extra version arguments fail. Main prints any returned error to stderr and exits `1`; nil returns normally. | Matches the documented root contract. Help wording drift is routed to Stage 24. |
+| Generate dispatch | Requires one of four exact report names. Command-specific flag sets accept only documented flags; Daily requires a date, Today defaults or parses one, and Tomorrow/Hourly reject `--date`. Config and explicit output/working-directory values map into one app request and one executor. | Functional mapping matches, but date validation occurs after executor construction; `AUD-009`. Assembled result/exit coverage is `AUD-010`. |
+| Run dispatch | Requires exact Morning or Evening batch identity, accepts `--out-dir` rather than `--out`, resolves only explicit relative overrides against the absolute invocation directory, preserves configured fallback, and constructs one executor/request. | Matches the parser/wiring contract. Representative assembled result coverage is incomplete in `AUD-010`; detailed batch behavior remains Stage 16. |
+| Compare dispatch | Requires a valid report and two distinct nonblank ordered profiles before config/executor work; applies the same report date eligibility; clears the configured default profile; maps exact bundle destination/replace/debug options; constructs one executor. | Matches the CLI contract. Focused tests cover parser, construction frequency, pre-execution failure, success/partial/cleanup summaries, quiet behavior, and safe errors. Detailed execution remains Stage 18. |
+| Common flags and ownership | `addCommonFlags` registers config, units, timezone, and prompt debug only. Each action separately registers its output, quiet, date, profile, and replacement flags. Report and batch name lookup delegates to `internal/report`; config precedence delegates to `internal/config`. | Intentional explicitness retained as `RET-011`; repeated date policy is routed to Stage 23. |
+| Validation ordering | Unknown commands/reports, unsupported flags, extra args, and comparison profile shape fail before configuration or executor creation. Generate performs Daily presence and Daily/Today parsing after config and executor creation. | Partly incorrect; `AUD-009`. |
+| Output paths | CLI captures/validates one absolute working directory, preserves configured fallbacks, and resolves only nonempty relative explicit output overrides against that directory. App owns default naming and publication. | Matches CLI/config documentation and architecture ownership. |
+| Stdout, stderr, and quiet | Result-bearing actions emit indented JSON to stdout. Run emits compact ordered item/notification/batch lines to stderr first. Quiet returns before both routine channels but the returned error still reaches main stderr. Pre-result failures emit no structured output. | Matches the documented separation; summary design retained as `RET-012`. Broken status-writer behavior is routed to Stage 25. |
+| Generate summaries | Selected report, prompt, time, profile/model, warnings, validation, output, debug, notification, and error fields are copied; status is failed exactly when a result accompanies an error. | Shape matches documentation, but only the constructor is tested; assembled behavior is `AUD-010`. Producer redaction remains for Stages 6, 11, and 16. |
+| Batch summaries and exit | Report counters remain independent of notification failure. Any failed report or failed notification makes summary status/error failed, emits status lines unless quiet, and returns typed `BatchError` so main exits nonzero. | Mapping is coherent and notification failure is assembled-tested. Broader representative translation coverage is `AUD-010`. |
+| Comparison summaries | Field order is fixed by the explicit DTO; profile order is retained; unpublished artifact fields are omitted; errors are mapped to bounded aggregate, cancellation, deadline, Promptkit, destination, cleanup, or application categories. | Matches the documented safe CLI contract with strong focused coverage. |
+| Cancellation | `Runner.Run` forwards its context to every app action, and app tests protect cancellation behavior. Production main supplies an uncancelable background context and has no signal bridge. | Programmatic propagation is correct; executable cancellation is `AUD-008`. |
+| Test ownership | Parser/resolver tests own flag acceptance, paths, configuration mapping, and one-executor construction; summary tests own selected fields; comparison command tests own assembled translation. | Comparison is sufficient. Non-comparison assembled coverage is insufficient in `AUD-010`; lower workflow details correctly remain in app tests. |
+
+#### Commands And Evidence
+
+- Used graph inventory, source snippets, inbound/outbound traces, and
+  graph-augmented searches for root dispatch, every action parser/resolver,
+  common flag registration, working/output path resolution, executor mapping,
+  summary/status construction, output routing, error classification, app
+  cancellation consumers, and process signal handling.
+- Compared executable help, accepted flags, summary fields, error/status rules,
+  and action wiring with `docs/cli.md`, `docs/internal/cli.md`, and the
+  configuration, architecture, documentation, and testing contracts.
+- Ran `go test -cover ./internal/cli`; the suite passed at 75.8% statement
+  coverage. Coverage was used only to guide test inspection, not as a finding.
+- Ran `go run ./cmd/weatherreporter --help`,
+  `go run ./cmd/weatherreporter --version`, and
+  `go run ./cmd/weatherreporter generate daily`; help and development-version
+  paths succeeded, while missing Daily date returned the documented pre-result
+  error and nonzero status without contacting an external service.
+- Findings: `AUD-008`, `AUD-009`, and `AUD-010`.
+- Retained decisions: `RET-011` and `RET-012`.
+- Open questions: the four leads recorded above are routed to their assigned
+  later stages.