Record Stage 15 output publication audit
This commit is contained in:
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user