From a4ef76f17afaa08232e6061f2270dc475e9e4f8d Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 12 Aug 2026 16:45:47 +0000 Subject: [PATCH] Record Stage 15 output publication audit --- docs/roadmap/audit.md | 245 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 241 insertions(+), 4 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index b1489cd..308e3f8 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stages 1-14 complete. +Status: In progress; Stages 1-15 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 @@ -104,8 +104,17 @@ without reconciling inspected, prepared, callback, completed-execution, and schema-validation provenance. Side-effect-free prompt/profile inspection, credential preflight, immutable prepared values, one-build comparison reuse, per-execution copies, typed generated-text validation, debug failure handling, -and partial execution results otherwise match their contracts. Subsystem -conclusions and final disposition remain pending the later stages. +and partial execution results otherwise match their contracts. Stage 15 +found one medium-severity destination-safety defect because ordinary +publication replaces non-regular filesystem objects, one low-severity path +handling defect because a valid long output basename makes the temporary name +overflow only after expensive work and leaves its newly created directory, and +one medium-severity cancellation defect because context is checked before the +temporary write rather than at the rename linearization point. Ordinary +regular-file replacement, prior-output preservation before rename, restrictive +new-file modes, temporary cleanup, committed-path result truthfulness, and +post-publication notification semantics otherwise match their contracts. +Subsystem conclusions and final disposition remain pending the later stages. ## Baseline Metadata @@ -280,7 +289,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | Generated-text validation | `internal/generatedtext` | 12 | Partly insufficient. The four current registry definitions dispatch to typed validators with coherent required-field and normalization behavior, and schema bytes are independently copied. `AUD-036` through `AUD-039` record report-independent catalog matching, case-insensitive Go field aliases, unbounded prose/diagnostics, and duplicated or misplaced contract tests. | | Render contexts and templates | `internal/generatedtext`, `internal/reporttemplate` | 13 | Partly insufficient. Typed module extraction, deterministic daypart ordering, report-specific sections, partial conditions, template lookup, and semantic output assertions are coherent. `AUD-040` through `AUD-042` record conflicting report identities, structural Markdown injection, and Today's empty daypart section. | | Prompt preflight and prepared inputs | `internal/app` | 14 | Partly insufficient. Exact prompt input/output shape, profile selection, credentials, immutable preparation, per-execution copies, typed output validation, rendering, callback errors, and partial execution outcomes have focused owners. `AUD-043` and `AUD-044` record late generated-text catalog validation and missing end-to-end execution-provenance reconciliation. | -| Single-report publication and preservation | `internal/app`, `internal/fileutil` | 15 | Pending | +| Single-report publication and preservation | `internal/app`, `internal/fileutil` | 15 | Partly insufficient. Destination precedence, regular-file publication, same-directory temporary replacement, prior-output preservation, temporary cleanup, restrictive modes, truthful committed paths, and notification-after-publication have focused owners. `AUD-045` through `AUD-047` record destructive non-regular target handling, late failure for valid long basenames, and cancellation not checked at the rename boundary. | | Batch partial success and notification | `internal/app`, `internal/adapters/distributor` | 16 | Pending | | Comparison identity and transactional publication | `internal/comparison` | 17 | Pending | | Comparison concurrency and CLI behavior | `internal/app`, `internal/cli` | 18 | Pending | @@ -1941,6 +1950,133 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: `AUD-036`, `AUD-037`, `AUD-040` - Remediation reference: pending +### AUD-045: Single-report publication replaces non-regular destinations + +- Stage: 15 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: correctness +- Area: `internal/app.validateOutputPath` and + `internal/fileutil.WriteFileAtomic` +- Evidence: Destination validation follows the final path with `os.Stat` and + rejects it only when the result is a directory. FIFOs, sockets, devices, and + other non-regular objects therefore pass; a symlink to a regular file also + passes even though rename replaces the symlink itself rather than its target. + `WriteFileAtomic` creates a regular temporary file and calls `os.Rename` + without reclassifying the target. A temporary Unix probe created a FIFO at an + explicit Daily output path, ran the complete offline generation workflow, + and confirmed successful publication had replaced the FIFO with a regular + Markdown file. Existing app tests protect only a directory collision, and + fileutil's rename-error test also uses a directory. +- Contract at risk: An operator-selected report destination may replace an + absent path or an existing regular report file, but publication must reject + filesystem objects whose semantics or ownership are not ordinary report + content. +- Impact: A typo or name collision can silently destroy a named pipe, Unix + socket entry, device node where permissions permit, or other special object + and report success. Accepted symlink behavior is also surprising: the link + is removed instead of atomically updating the file it names. +- Recommendation: Use `Lstat` to define and enforce an explicit final-component + policy before work and immediately before rename. Accept only absent or + regular-file destinations; either reject symlinks or document and implement + one deliberate link policy without following at validation and replacing at + publication. +- Test implications: Add a portable table for absent, regular, directory, and + symlink targets plus platform-gated FIFO/socket cases. Assert invalid targets + fail before prompt inspection or collection and remain unchanged. +- Validation: Every accepted existing target is a regular report file under + the documented symlink policy; non-regular objects retain their type and + identity with zero collector, executor, or notifier calls. +- Related findings: none +- Remediation reference: pending + +### AUD-046: Valid long output names fail through the temporary-name pattern + +- Stage: 15 +- Status: candidate +- Severity: low +- Confidence: high +- Category: correctness +- Area: `internal/fileutil.WriteFileAtomic` and explicit single-report output + paths +- Evidence: The atomic writer gives `os.CreateTemp` the pattern + `.` plus the complete destination basename plus a random suffix and `.tmp`. + A destination component can therefore fit the filesystem's name limit while + its derived temporary component does not. A temporary offline probe used a + valid 248-byte `.md` basename beneath a missing directory on the current + filesystem. Resolution, prompt inspection, collection, model execution, and + rendering all completed; publication created the parent, then failed with + `ENAMETOOLONG` while creating the temporary file. A direct write to the exact + target succeeded afterward, proving the requested component itself was + valid. The failed run also left the newly created empty output directory, + showing that failure cleanup covers the temporary file but not parent + directories created by the same publication attempt. +- Contract at risk: A filesystem-valid operator-selected output name should + not become invalid solely because of an internal temporary naming scheme; + publication side effects and cleanup should also match the documented output + lifecycle. +- Impact: An explicit long output path fails only after all remote and model + work, returns no published path, and can leave a directory requiring manual + cleanup. Default report names are short, limiting current exposure to custom + output paths. +- Recommendation: Use a short fixed temporary prefix independent of the final + basename while retaining same-directory rename. Validate any unavoidable + platform path constraint before external work, and reconcile directory + creation/cleanup semantics without deleting concurrently created content. +- Test implications: Add a filesystem-boundary case whose final component is + valid but would overflow the old temp pattern; assert successful replacement, + mode, content, and no temporary residue. Add a publication-failure case for + the documented missing-directory lifecycle through an injectable stable + filesystem boundary rather than relying on a platform error string. +- Validation: Every accepted final basename can create its temporary sibling, + and a failed publication leaves neither a partial file nor an app-created + empty directory under the documented lifecycle. +- Related findings: none +- Remediation reference: pending + +### AUD-047: Cancellation is not checked at the publication linearization point + +- Stage: 15 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: reliability +- Area: `internal/app.publishPromptReport`, `publicationContextError`, and + `internal/fileutil.WriteFileAtomic` +- Evidence: The application checks `ctx.Err()` once before calling the atomic + writer. The writer then creates missing directories, creates and writes the + temporary file, closes it, and finally publishes with `os.Rename`; it accepts + no context and performs no check immediately before rename. Thus cancellation + that becomes observable during directory creation or temporary-file I/O is + ignored and the destination is still replaced. The linearization point is + the successful same-directory rename. Current cancellation and deadline + tests arrange cancellation before `publishPromptReport` starts, so they prove + only the earlier check. Unbounded generated Markdown from `AUD-038` can widen + the unchecked write interval. +- Contract at risk: Cancellation observed immediately before publication must + preserve the prior operator-owned report; completed rendering does not itself + authorize replacement after the run is canceled. +- Impact: A canceled command can replace an existing report and, with + notification disabled, return success if cancellation occurs after the one + check. With notification enabled it can instead report a later notification + failure while retaining a publication the operator expected cancellation to + prevent. +- Recommendation: Make the atomic writer context-aware, cleanly write and close + its temporary file, then classify and return cancellation/deadline immediately + before rename. Keep the application check for fast failure, but make the + filesystem boundary own the final pre-commit observation. +- Test implications: Add a deterministic writer seam or controlled atomic-write + test that cancels after temporary-file close but before rename; assert the + old destination and empty `OutputPath` remain, no notifier runs, and no temp + survives. Retain the current cancellation-before-publication cases. +- Validation: Cancellation or deadline made observable at the final pre-rename + gate prevents replacement and returns the matching stable category; once + rename succeeds, results truthfully report publication regardless of later + notification outcome. +- Related findings: `AUD-038` +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -2303,6 +2439,32 @@ remote-work prerequisites. Retain the inspection seam while expanding it to cover catalog compatibility and complete, later-reconciled provenance under `AUD-043` and `AUD-044`. +### RET-031: Keep same-directory temporary replacement for one report file + +The narrow `fileutil.WriteFileAtomic` mechanism gives ordinary report +publication the right visibility boundary: it creates a mode-`0600` temporary +file beside the selected destination, writes and closes complete Markdown, and +uses one same-directory rename as the linearization point. Failures before +rename leave an existing regular report unchanged, and deferred removal cleans +the temporary name after a rename error. This is simpler and safer than +truncating the destination in place or building a multi-file transaction for a +single artifact. Retain the mechanism while using a bounded temporary name, +enforcing valid target types, and adding the final context gate required by +`AUD-045` through `AUD-047`; do not describe atomic visibility as crash +durability without explicit synchronization. + +### RET-032: Keep publication truth separate from notification success + +`publishPromptReport` records `ReportResult.OutputPath` only after the atomic +rename succeeds, then constructs and sends the optional notification from that +exact operator-owned Markdown file. Every earlier failure returns an empty +published path; any request-construction or delivery failure afterward retains +the committed path and notification detail in the active result. The CLI +summary copies those fields and marks the action failed when the returned error +is non-nil, so it can distinguish an unpublished generation failure from a +published report whose delivery failed. Retain this ordering and result model; +batch aggregation policy remains Stage 16. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -2546,6 +2708,23 @@ Stage 14 routed these investigation leads to their assigned later stages: the actionable wrapper are sufficient when consolidating application fixtures; `AUD-033` separately owns the reachable stanza serialization gap. +Stage 15 routed these investigation leads to their assigned later stages: + +- Existing directory symlinks are deliberately accepted for normal output + directories, but pathname components can change between preflight and rename. + `AUD-045` owns the final-component policy; Stage 25 should include a + deterministic component-swap case when validating remediation rather than + Stage 15 claiming containment the current operator-owned-path contract does + not define. +- The writer provides atomic visibility but does not call `Sync` on the file or + parent directory. No canonical document promises power-loss durability, so + Stage 15 did not invent that guarantee. Stage 24 should keep “atomic” wording + clear enough that operators do not read it as a persistence acknowledgment. +- `WriteJSONAtomic` remains outside the ordinary report path and is reached in + production only through unused persistence helpers. `AUD-001` already owns + that surface; Stage 23 should remove or justify it without mixing legacy JSON + persistence into the single-Markdown publication contract. + ## Stage Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -3293,3 +3472,61 @@ Stage 14 routed these investigation leads to their assigned later stages: - Retained decisions: `RET-029` and `RET-030`. - Open questions: the three leads recorded above are routed to their assigned later stages. + +### Stage 15: Audit Single-Report Generation And Atomic Output + +- Status: Complete. +- Scope reviewed: the complete single-report path in `internal/app/app.go`, + single-report destination logic in `internal/app/output.go`, publication in + `prompt_generate.go`, all production code and tests in `internal/fileutil`, + generation/output tests, the application-orchestration and operations + contracts, and the narrow CLI result projection needed to trace committed + paths and errors without re-auditing parsing. +- Exclusions: Batch output planning, continuation, and notification remain + Stage 16; comparison destination identity and transactional bundle + publication remain Stage 17; comparison cancellation remains Stage 18; + suite-wide durability and refactoring remain Stages 19-23; documentation-wide + coherence remains Stage 24; and adversarial pathname races remain Stage 25. + +#### Single-Report Publication Accounting + +| Contract or risk | Destination, filesystem, result, and test evidence | Disposition | +| --- | --- | --- | +| Stage order and destination precedence | Report resolution precedes output resolution; an explicit file wins over the configured directory, which wins over the captured absolute working directory. Existing configured non-directories fail before prompt inspection or collection, while missing directories are deferred until publication. | Coherent for ordinary paths. Catalog preflight remains `AUD-043`; CLI executor-before-date ordering remains `AUD-009`. | +| Path normalization and target type | Relative paths are anchored to the captured working directory, absolute paths are cleaned, roots and directories are rejected, and result paths are absolute. Final validation otherwise accepts every non-directory object and follows symlinks. | Destructive special-file and ambiguous symlink behavior is `AUD-045`. | +| Atomic write and linearization | The writer creates a temporary file in the destination directory, writes and closes complete bytes, then uses same-directory `os.Rename`. Successful rename is the single visibility/commit point. Existing regular output is untouched on all earlier application failures and writer failures before rename. | Mechanism retained as `RET-031`. Temporary naming can reject a valid final component in `AUD-046`; crash durability is not claimed. | +| File and directory modes | `os.CreateTemp` creates new report files as mode `0600`; `MkdirAll` requests `0755` for missing operator-selected parents and leaves existing modes unchanged. Replacement takes the temporary file's restrictive mode. | Safe for report content. Focused tests do not assert the final mode, but the standard-library primitive directly defines it and no conflicting user contract exists; no coverage-only finding. | +| Temporary and failure cleanup | A deferred remove covers write, close, and rename failures after temporary creation; the rename-error test proves no matching temp remains. Parent directories created before a later create/write failure are not rolled back. | Temp cleanup is coherent. The valid-long-name failure and directory residue are `AUD-046`; broad directory rollback would need concurrency-safe ownership. | +| Cancellation | Cancellation/deadline before publication entry returns the stable category, leaves `OutputPath` empty, preserves previous bytes, and skips notification; focused app tests cover both. | The check precedes all temporary I/O rather than the rename boundary, so later-observable cancellation can still commit; `AUD-047`. | +| Pre-publication preservation | Prompt inspection, collection, generation, schema/typed validation, render, debug-write, destination, cancellation, and atomic-write failures occur before `OutputPath` is set. Tests preserve an existing file across generation, render, cancellation, deadline, and directory-collision failures; fileutil tests cover rename error and temp cleanup. | Sufficient for regular files aside from the final cancellation gap. Stage 14 owns preparation/execution provenance and Stage 16 owns multi-output consequences. | +| Post-publication semantics | Immediately after rename, the result records the absolute path. Optional notification is then built from and sent with that exact file. Notification failure returns a failed action while retaining the committed path and Markdown; disabled notification returns success without a notification object. | Coherent and retained as `RET-032`. Single-report identity rendering gaps remain `AUD-005`; Distributor adapter/batch policy remain Stage 16. | +| Errors and CLI result truth | Cancellation is wrapped with report/run/operation context and preserves its category. Filesystem errors carry the exact affected directory, temp operation, or final path; notification errors carry report/run/output context. CLI projection copies active safe fields, committed path, notification, and returned error, marking any result-bearing error failed. | Actionable and truthful at the reviewed boundary. Assembled non-comparison CLI status/exit tests remain the existing `AUD-010`. | +| Complexity and test ownership | Resolution and publication are short guarded pipelines; file publication is constant-pass work over rendered bytes and has no service-sized loop beyond the write itself. App tests own workflow preservation/result ordering; fileutil tests own atomic replacement and temporary cleanup. | No Stage 15 complexity finding. Missing special-target, long-name, and final cancellation cases attach directly to `AUD-045` through `AUD-047`. | + +#### Commands And Evidence + +- Used graph symbol and augmented-code search, exact snippets, inbound/outbound + traces, and scoped complexity/coverage data for `GenerateDetailed`, output + resolution and validation, publication/cancellation, atomic writing, + notification, and CLI result projection. Bounded text inspection covered all + scoped production/tests, task-specific documents, policies, and the exact + Stage 15 boundary. +- Traced every consequential result through the CLI projection: pre-result + resolution failure, resolved but unpublished destination/preflight, + collection/generation/render/cancellation/write failure, successful commit, + and committed output followed by notification failure. CLI parsing and + aggregate batch behavior were not re-audited. +- Ran temporary offline filesystem probes showing that complete generation + replaces a FIFO with a regular Markdown file and that a filesystem-valid + 248-byte basename fails after model work because the derived temp component + is too long, leaves its new parent directory, and remains directly writable. + The probes were removed without retaining source/test changes. +- Ran focused coverage for `./internal/app` and `./internal/fileutil`; both + passed with 76.0% and 68.4% statement coverage respectively. Coverage guided + failure-branch inspection but was not itself treated as a finding. Ran + `go test ./...`, `go vet ./...`, `go run ./cmd/weatherreporter --help`, and + `git diff --check`; all passed. +- Findings: `AUD-045`, `AUD-046`, and `AUD-047`. +- Retained decisions: `RET-031` and `RET-032`. +- Open questions: the three leads recorded above are routed to their assigned + later stages.