From 00fe0c3e96f60702cec3890df1c68833cf81bfb0 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 12 Aug 2026 15:08:34 +0000 Subject: [PATCH] Record the architecture audit findings --- docs/roadmap/audit.md | 173 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 162 insertions(+), 11 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index c825083..5d4f970 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stage 1 complete. +Status: In progress; Stages 1-2 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 @@ -11,9 +11,12 @@ canonical owners identified by the ## Executive Summary Stage 1 established a clean, reproducible baseline. The repository-wide test -suite, CLI help check, formatting check, and vet check all pass. No audit -finding or retained decision was established during baseline collection. -Subsystem conclusions and final disposition remain pending the later stages. +suite, CLI help check, formatting check, and vet check all pass. Stage 2 found +that the implemented package graph and principal workflows follow the intended +dependency direction and assigned ownership. It recorded one low-severity +candidate finding for unused internal persistence helpers left outside the +documented stateless workflows. Subsystem conclusions and final disposition +remain pending the later stages. ## Baseline Metadata @@ -141,7 +144,7 @@ inventory commands, graph index refresh, and graph architecture inspection. | Stage | Scope | Status | | ---: | --- | --- | | 1 | Establish the baseline and audit ledger | Complete | -| 2 | Audit architecture and dependency direction | Pending | +| 2 | Audit architecture and dependency direction | Complete | | 3 | Audit report identity and time foundations | Pending | | 4 | Audit configuration, secrets, and validation | Pending | | 5 | Audit CLI parsing, wiring, and output contracts | Pending | @@ -176,7 +179,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | Risk area | Current test owner or evidence source | Audit stage | Coverage assessment | | --- | --- | ---: | --- | -| Architecture and dependency direction | Assembled app and CLI tests; graph traces | 2 | Pending | +| 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` | 3 | Pending | | Configuration, validation, and secrets | `internal/config` | 4 | Pending | | CLI parsing, output, and exit behavior | `internal/cli` | 5 | Pending | @@ -199,17 +202,107 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. ## Findings -No findings were recorded in Stage 1. A passing baseline is not evidence that -later audit scopes have no findings. +### AUD-001: Unused persistence helpers remain after the stateless redesign + +- Stage: 2 +- Status: candidate +- Severity: low +- Confidence: high +- Category: architecture +- Area: `internal/promptinput.Save`, + `internal/adapters/weatherapi.SaveBundle`, and + `internal/fileutil.WriteJSONAtomic` +- Evidence: Graph inbound traces show `promptinput.Save` has no callers, + `weatherapi.SaveBundle` is called only by `TestSaveBundle`, and + `fileutil.WriteJSONAtomic` is called only by `SaveBundle` and its focused + test. Repository text search found no documentation or other call sites. + Representative generate, batch, comparison, and collection traces do not + reach any of these helpers. `promptinput.Save` and `SaveBundle` write + intermediate prompt input or normalized weather data to arbitrary paths, + while normal publication uses `fileutil.WriteFileAtomic` for selected + Markdown and `comparison.Publish` for selected bundles. +- Contract at risk: The architecture policy and ADR 0001 define normal + execution as an in-memory stateless transformation whose durable files are + operator-selected report outputs, comparison bundles, or explicitly + requested secure prompt debugging. +- Impact: These unreachable exported functions do not create state during + current workflows, but they retain an unsupported persistence surface and + low-value tests that can invite accidental reintroduction of intermediate + artifacts or require maintenance despite having no product caller. +- Recommendation: Remove the two unused domain/adapter save functions, remove + `WriteJSONAtomic` if it then has no production use, and delete or consolidate + tests that protect only those retired APIs. +- Test implications: `TestSaveBundle` and `TestWriteJSONAtomic` protect unused + persistence mechanisms; `promptinput.Save` has no focused test. Preserve + tests for `WriteFileAtomic`, normal report publication, comparison + publication, and explicit prompt-debug writes. +- Validation: Graph and text searches show no remaining production references + to the retired helpers; `go test ./...` passes; normal output, comparison, + and debug-publication tests remain unchanged and pass. +- Related findings: none +- Remediation reference: pending ## Retained Decisions -No retained decisions were recorded in Stage 1. +### RET-001: Keep the application package as the explicit composition owner + +The package import graph, outbound traces from `GenerateDetailed`, +`RunBatchDetailed`, and `compareDetailed`, and their focused tests were +inspected. `internal/app` has deliberately broad outbound dependencies because +it sequences project-owned domain, collection, execution, publication, and +notification contracts; those dependencies do not point back into app. Splitting +that fan-out merely to reduce a graph metric would obscure workflow ownership. +Reconsider only if a second composition owner emerges or a coherent workflow +can move behind a narrower contract without duplicating ordering policy. + +### RET-002: Keep external dependencies behind repository-owned contracts + +Production import and data-flow inspection found Promptkit imports only in +`internal/adapters/promptkit` and Distributor imports only in +`internal/adapters/distributor`. The Promptkit adapter implements +`promptexec.Executor`; Weather API collection returns `weatherdata.Bundle`; +and Distributor results are translated before application and CLI summaries. +This explicit translation prevents dependency types and sensitive diagnostics +from becoming application contracts. Reconsider only when an upstream type is +intentionally adopted as a public repository contract with corresponding +architecture and compatibility changes. + +### RET-003: Keep publication mechanisms separate by artifact contract + +Filesystem-write discovery and call traces were inspected for single reports, +comparison bundles, and secure prompt debugging. `fileutil.WriteFileAtomic` +owns one-file replacement, `comparison.Publish` owns guarded transactional +directory replacement and recovery, and `promptdebug` owns secure explicit +diagnostic files. Their shared use of temporary paths and rename operations is +mechanical similarity, while their authorization, permissions, commit, and +recovery semantics differ materially. Reconsider common abstraction only if +multiple artifact kinds acquire the same complete transaction contract. + +### RET-004: Keep collection as a narrow application-facing seam + +The `app.Collector` contract, `collect.Run`, the Weather API adapter boundary, +and focused collection tests were inspected. Although `collect.Run` is small, +it keeps adapter creation and error context out of orchestration and gives app +tests a dependency-neutral deterministic seam. Reconsider if collection gains +no additional policy and an equally narrow project-owned adapter contract can +replace it without leaking transport construction into app. ## Open Questions -No Stage 1 open questions or unexplained baseline failures remain. Later -stages may add scoped questions according to the audit plan. +No Stage 1 open questions or unexplained baseline failures remain. + +Stage 2 routed these investigation leads without treating graph metrics or an +unusual edge as findings: + +- `internal/config` imports `internal/briefing` to use module definitions while + validating report-module options. Stages 4, 8, and 23 should determine + whether this remains the smallest single-owner validation path or creates + avoidable registry coupling. +- The graph identifies `internal/cli.Run`, `internal/app.RunBatchDetailed`, and + `internal/app.compareDetailed` as relatively complex orchestration paths. + Their ownership and top-level ordering are coherent; Stages 5, 16, 18, 22, + and 23 should assess their local behavior and maintainability rather than + inferring a finding from metrics. ## Stage Log @@ -227,3 +320,61 @@ stages may add scoped questions according to the audit plan. - Findings: none. - Retained decisions: none. - Open questions: none. + +### Stage 2: Audit Architecture And Dependency Direction + +- Status: Complete. +- Scope reviewed: `cmd/weatherreporter`, production import boundaries for all + 24 Go packages, principal call and data-flow paths, external adapter + containment, project-owned interfaces, publication boundaries, filesystem + writes, ADR 0001, focused orchestration tests, and the architecture policy. +- Exclusions: Detailed report/time, configuration, CLI, weather, domain, + module, prompt, rendering, comparison, and notification correctness remains + assigned to Stages 3-18. Cross-cutting complexity, deduplication, and full + documentation coherence remain assigned to Stages 22-24. + +#### Boundary Accounting + +| Normative boundary or invariant | Implementation owner and evidence | Disposition | +| --- | --- | --- | +| Binary entry and CLI ownership | `cmd/weatherreporter.main` calls `cli.Run`; `internal/cli` owns action parsing, config loading, executor construction, and bounded result output. Graph traces place app calls below action resolution. | Matches policy. | +| Configuration ownership | `internal/config` owns defaults, YAML loading, normalization, secrets, and validation. CLI loads config and passes effective values down; app does not parse config files. | Matches policy; the config-to-briefing validation edge is routed to Stages 4, 8, and 23. | +| Application orchestration | `GenerateDetailed`, `RunBatchDetailed`, and `compareDetailed` sequence preflight, collection, preparation, execution, publication, and notification through repository-owned values and interfaces. No lower package imports app. | Matches policy; retained as `RET-001`. | +| Report and domain ownership | `report`, `timeutil`, `weatherdata`, `forecast`, `facts`, `module`, `briefing`, `promptinput`, `generatedtext`, and `reporttemplate` form one-way deterministic dependencies below app. | Matches policy at package level; local rules remain for Stages 3 and 7-13. | +| Prompt execution boundary | `promptexec.Executor` is dependency-neutral. CLI constructs the Promptkit adapter once per action, app consumes only `promptexec` requests/results, and the prepared-report data flow supplies curated serialized module packages. | Matches policy; retained as `RET-002`. | +| Weather API boundary | App calls its `Collector` interface; the production implementation delegates through `collect.Run` to `adapters/weatherapi.FetchBundle`, which returns normalized `weatherdata.Bundle`. Production HTTP imports occur only in external adapters. | Matches policy; retained as `RET-004`. | +| Distributor boundary | App owns notification timing and repository request/result types; `distributorNotifier` translates them to adapter-local types and the adapter alone imports the Distributor dependency. Comparison exposes no notifier path. | Matches policy. | +| Prompt preflight and curated inputs | Generate inspects the exact prompt/profile before `collectWeather`; batch inspects all candidates before its one collection; comparison inspects prompt and all profiles before collection, then calls `prepareReport` once. Data-flow traces reach `promptinput.Build` and `MarshalYAML`, not raw bundle serialization into Promptkit. | Matches policy at workflow level; detailed checks remain for Stages 10, 11, 14, and 18. | +| Single output and notification order | `publishPromptReport` checks context, calls `fileutil.WriteFileAtomic`, records the path, and only then calls `notifyReport`. Focused tests cover pre-publication preservation and notification ordering. | Matches policy; publication split retained as `RET-003`. | +| Batch output and notification order | `RunBatchDetailed` validates every planned path before sequential execution, suppresses item notification, continues across item failures, and calls `notifyBatch` only after the loop. Focused tests cover preflight, partial failure, and notification-after-publication. | Matches policy at workflow level; detailed review remains for Stage 16. | +| Comparison publication | App preflights before external work and again before publication. `internal/comparison` owns bundle validation, guarded replacement, staging, commit, restoration, and cleanup results. The comparison request has no notifier and the trace does not reach Distributor. | Matches policy at workflow level; detailed review remains for Stages 17-18. | +| Stateless execution | Normal traces retain preparation and execution values in memory. Production filesystem writes are limited to selected Markdown publication, selected comparison bundles, explicit prompt-debug capture, and the unreachable helpers in `AUD-001`; no cache, workspace, history, receipt, or resume owner appears in the package graph. | Matches normal-runtime policy; `AUD-001` records the unused persistence surface. | +| Dependency cycles and direction | The compiler-derived direct-import inventory is acyclic. Entry packages point inward, adapters do not import app/CLI, domain packages do not import orchestration, and no external dependency type appears outside its production adapter. | Matches policy. | + +#### Commands And Evidence + +- Refreshed graph project `home-eric-Workspace-weatherreporter` in moderate + mode. The graph's branch identity remains production baseline `e7c7262`; + Stage 1's intervening commit changes only this excluded audit document. +- Used graph architecture views for structure, dependencies, entry points, + hotspots, boundaries, layers, and clusters; queried the graph schema and + direct import edges. +- Used graph search, snippets, inbound/outbound call traces, and data-flow + traces for `main`, `cli.Run`, `GenerateDetailed`, `RunBatchDetailed`, + `compareDetailed`, `prepareReport`, `executePreparedProfile`, `collect.Run`, + all three external adapters, `publishPromptReport`, + `comparison.PlanDestination`, `comparison.Publish`, and Distributor + notification. +- Used compiler-derived `go list` direct imports to separate production edges + from test-only graph edges and confirm the build has no import cycle. +- Used graph-augmented code search to inventory external dependency imports, + HTTP ownership, and production filesystem writes. A bounded text search + confirmed the unused persistence helpers have no documentation or hidden + non-code callers. +- Ran + `go test ./internal/app ./internal/cli ./internal/collect ./internal/comparison ./internal/adapters/... ./internal/fileutil ./internal/promptdebug ./internal/promptexec`; + all focused packages passed. +- Findings: `AUD-001`. +- Retained decisions: `RET-001`, `RET-002`, `RET-003`, and `RET-004`. +- Open questions: the two leads recorded above are routed to their assigned + later stages.