From 5ed448df11b9b93804894e7f628a85b7569fa3fd Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 12 Aug 2026 15:35:13 +0000 Subject: [PATCH] Complete the Stage 5 CLI audit --- docs/roadmap/audit.md | 236 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 231 insertions(+), 5 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 3b46199..2d0b4da 100644 --- a/docs/roadmap/audit.md +++ b/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.