From d328a1daa6284bb1819feb1ffc2769e51739fd48 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Wed, 12 Aug 2026 16:58:42 +0000 Subject: [PATCH] Record Stage 17 comparison publication audit --- docs/roadmap/audit.md | 330 +++++++++++++++++++++++++++++++++++++++++- 1 file changed, 327 insertions(+), 3 deletions(-) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 6e6a365..3d1dff9 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,6 +1,6 @@ # Repository Audit Ledger -Status: In progress; Stages 1-16 complete. +Status: In progress; Stages 1-17 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 @@ -124,6 +124,17 @@ replaces the production HTTP client boundary. Collect-once planning, complete destination preflight, deterministic sequential execution, partial publication accounting, published-file upload selection, and the all-success notification gate otherwise match their contracts. +Stage 17 found four medium-severity comparison defects: manifest recognition +accepts duplicate and case-variant JSON fields, successful report paths need +not match the documented ordinal/profile filename, cancellation after moving +the prior bundle does not prevent replacement commit, and backup-cleanup +failures can leave only a partially deleted recovery artifact while describing +it as retained. It also found one low-severity path-handling defect because a +filesystem-valid long destination basename fails during staging and can leave +new parent directories. Flat versioned bundles, deterministic encoding and +hashing, exact file-set recognition, broad-target and final-symlink rejection, +commit-time destination reauthorization, rollback after install failure, and +committed-new-bundle truth otherwise match their contracts. Subsystem conclusions and final disposition remain pending the later stages. ## Baseline Metadata @@ -267,7 +278,7 @@ inventory commands, graph index refresh, and graph architecture inspection. | 14 | Audit application preparation and prompt preflight | Complete | | 15 | Audit single-report generation and atomic output | Complete | | 16 | Audit batch orchestration and Distributor notification | Complete | -| 17 | Audit comparison contracts and transactional publication | Pending | +| 17 | Audit comparison contracts and transactional publication | Complete | | 18 | Audit comparison execution and CLI integration | Pending | | 19 | Audit test hermeticity and execution hygiene | Pending | | 20 | Audit test risk coverage and ownership | Pending | @@ -301,7 +312,7 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. | 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 | 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 | Partly insufficient. Collect-once planning, complete output preflight, deterministic sequential execution, independent partial success, report-only counters, published-source selection, and one all-success batch notification are coherent. `AUD-048` through `AUD-052` record late Distributor endpoint rejection, unsafe and unbounded response diagnostics, cancellation flattened into report failure, and the missing production HTTP adapter test boundary. | -| Comparison identity and transactional publication | `internal/comparison` | 17 | Pending | +| Comparison identity and transactional publication | `internal/comparison` | 17 | Partly insufficient. Versioned flat manifests, ordered identity helpers, canonical encoding, data-package hashing, exact file sets/types, read-only destination classification, commit-time reauthorization, sibling staging, replacement rollback, restrictive modes, and committed cleanup state have focused package owners. `AUD-053` through `AUD-057` record ambiguous manifest fields, unenforced report filenames, a replacement cancellation gap, partially deleted retained backups, and late long-destination staging failure. | | Comparison concurrency and CLI behavior | `internal/app`, `internal/cli` | 18 | Pending | | Hermeticity, execution hygiene, portfolio coverage, and durability | Repository-wide suite | 19-21 | Pending | | Cross-cutting efficiency and maintainability | Graph metrics plus focused tests | 22-23 | Pending | @@ -2286,6 +2297,205 @@ with evidence about meaningful risks, test ownership, gaps, and duplication. - Related findings: `AUD-049`, `AUD-050` - Remediation reference: pending +### AUD-053: Comparison recognition accepts ambiguous manifest fields + +- Stage: 17 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: compatibility +- Area: `internal/comparison.decodeManifest` and `RecognizeBundle` +- Evidence: The manifest decoder enables `DisallowUnknownFields`, but Go's + `encoding/json` still matches struct fields case-insensitively and accepts a + property more than once, using the last decoded value. A temporary package + probe confirmed that one manifest containing both `SchemaVersion` and + `schemaVersion` is accepted, and conflicting duplicate canonical properties + can likewise be resolved by their final occurrence. Recognition then treats + the directory as a current bundle eligible for destructive replacement. + Focused tests reject an unknown property and a trailing JSON value but do not + cover duplicate or case-variant names. +- Contract at risk: A replaceable bundle must exactly satisfy the current + manifest schema. Property names are case-sensitive canonical identities, and + duplicate JSON properties are ambiguous rather than an alternate encoding of + one logical manifest. +- Impact: A malformed or hand-edited directory can be authorized as a current + Weatherreporter bundle even though strict consumers may reject it or + interpret duplicate values differently. Because recognition gates + `--replace`, permissive decoding broadens the set of directories the program + is willing to move and recursively delete. +- Recommendation: Decode through a strict JSON object boundary that rejects + duplicate property names at every manifest object level and requires exact + canonical field spelling before typed unmarshalling and semantic validation. + Preserve the existing unknown-field and single-value checks. +- Test implications: Add package contract cases for duplicate top-level, + nested, result, and safe-error fields, conflicting duplicates, and + case-variant spellings. Assert `ErrUnrecognizedBundle` and preservation under + replacement planning for each malformed directory. +- Validation: Only exact, unique schema property names reach `Manifest.Validate` + and no ambiguous manifest can authorize replacement. +- Related findings: `AUD-037` +- Remediation reference: pending + +### AUD-054: Manifest validation does not enforce canonical profile report names + +- Stage: 17 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: compatibility +- Area: `internal/comparison.Manifest.Validate`, `LogicalBundle.Validate`, and + `RecognizeBundle` +- Evidence: `ReportFilename` implements the documented position width and + profile-slug algorithm, but manifest validation checks only that a successful + `reportPath` is a unique Markdown basename. Logical-bundle validation merely + requires its report path to equal that permissive manifest value. A temporary + package probe changed the first successful path from + `01-weather-light.md` to `arbitrary.md`; the complete logical bundle still + validated and could be published and recognized. Existing naming tests + exercise `ReportFilename` separately, so they do not connect that contract + to durable validation or replacement recognition. +- Contract at risk: Each successful report filename is + `NN-profile-slug.md`, derived from its one-based selected position, total + profile count, and authoritative logical profile ID. A recognized current + bundle must satisfy that durable layout rather than merely use any safe + basename. +- Impact: Weatherreporter can publish and later destructively recognize a + noncanonical bundle that consumers cannot reliably associate with the + documented selected-profile order. The naming helper and durable validator + can drift independently without a package-boundary failure. +- Recommendation: During manifest validation, derive the expected filename + with `ReportFilename(result.Position, manifest.Total, result.ProfileID)` for + every successful result and require exact equality. Keep failed results free + of report paths and retain the logical-bundle byte/path agreement check. +- Test implications: Add manifest, logical-bundle, encoded-round-trip, and + recognition cases for arbitrary safe basenames, wrong positions, wrong + ordinal widths, and incorrect or truncated slugs. One package-boundary + invariant can replace redundant helper-level examples if later test + consolidation warrants it. +- Validation: Every published and recognized successful report path is exactly + reproducible from the manifest's ordered profile identity. +- Related findings: none +- Remediation reference: pending + +### AUD-055: Replacement ignores cancellation after moving the prior bundle + +- Stage: 17 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: reliability +- Area: `internal/comparison.publish` between `authorizeMovedDestination` and + replacement installation +- Evidence: Publication checks `ctx.Err()` immediately before the replacement + branch, then renames the prior target to a sibling backup and re-runs full + recognition against the moved entry. It performs no context check after that + potentially substantial reauthorization and before renaming staging into the + target. A temporary deterministic rename probe canceled the context after + the first rename; publication ignored the observable cancellation, installed + the new bundle, removed the backup, and returned `Committed: true` with no + error. The existing cancellation test cancels through `beforeCommit`, before + either rename, and therefore does not cover this transaction state. +- Contract at risk: Cancellation observed before the new bundle's install + linearization point must restore and preserve the prior recognized bundle. + Moving it to a recoverable sibling is preparation, not commit authorization. +- Impact: An operator can cancel while the old bundle is being reauthorized yet + still have it replaced and its backup deleted. The result reports success + even though cancellation was visible before the new target existed. +- Recommendation: Check context after moved-destination authorization and + immediately before installing staging. On cancellation or deadline, invoke + the existing guarded restoration path and preserve the context cause along + with any recovery failure details. +- Test implications: Cancel deterministically after the backup rename and + during moved-bundle recognition for both empty and recognized destinations. + Assert the old snapshot is restored, staging is removed, no backup remains + after successful restoration, `Committed` is false, and `errors.Is` retains + cancellation or deadline; also test retained-backup diagnostics when restore + fails. +- Validation: Every cancellation observable before the staging-to-target + rename either leaves or restores the exact prior destination and cannot + return a committed replacement. +- Related findings: `AUD-008`, `AUD-047`, `AUD-051` +- Remediation reference: pending + +### AUD-056: Cleanup errors can leave an invalid retained backup + +- Stage: 17 +- Status: candidate +- Severity: medium +- Confidence: high +- Category: reliability +- Area: `internal/comparison.publish`, `PublicationResult`, and + `PublicationCleanupError` +- Evidence: After committing the new target, publication calls + `os.RemoveAll(backupDirectory)`. `RemoveAll` may delete some children before + returning its first error, but every error is reported as a committed result + with `RetainedBackupPath` and documented as the retained prior bundle. A + temporary deterministic cleanup probe removed the backup manifest and then + returned an error; publication advertised the absolute retained path even + though `RecognizeBundle` rejected the remaining directory. The focused test + fake returns an error without modifying the backup and asserts that the old + snapshot remains exact, which does not model the standard operation's partial + failure semantics. +- Contract at risk: Post-commit cleanup truth must distinguish an intact prior + bundle from partial cleanup remnants. Recovery guidance must not imply that a + path still contains a complete rollback artifact when deletion already + damaged it. +- Impact: An operator may preserve or archive a reported “backup” expecting the + prior bundle, only to discover later that required manifest, package, or + report files were already deleted. The new bundle remains truthfully + committed, but the recovery state and diagnostic overstate what survives. +- Recommendation: Model the post-cleanup state explicitly. On removal failure, + inspect the sibling without masking the original cause and report whether it + is an intact recognized prior bundle, partial remnants, or absent. Name result + fields and operational guidance accordingly; do not promise rollback + viability after recursive deletion has begun. +- Test implications: Through the deterministic cleanup seam, cover failure + before deletion, after deleting one file, and after removing the whole path. + Assert the committed new bundle remains recognized and the returned typed + state, safe summary, recovery path, and underlying error are truthful for + each case. +- Validation: Every cleanup failure reports the actual surviving state; only a + complete recognized old bundle is called a retained backup. +- Related findings: none +- Remediation reference: pending + +### AUD-057: Long comparison destinations fail through derived sibling names + +- Stage: 17 +- Status: candidate +- Severity: low +- Confidence: high +- Category: correctness +- Area: `internal/comparison.PlanDestination`, staging creation in `publish`, + and `uniqueSiblingPath` +- Evidence: Destination planning accepts any filesystem-valid target basename, + but staging and backup patterns prepend a dot and append `.staging-` or + `.backup-` plus random text to that complete basename. On a filesystem with a + 255-byte component limit, a 240-byte target component is valid and passes + read-only planning but `os.MkdirTemp` fails with `ENAMETOOLONG`. A temporary + probe also showed that publication creates previously missing parent + directories before this failure and leaves them behind. No focused test + covers boundary-length target components or the derived sibling names. +- Contract at risk: Every destination accepted at preflight must be capable of + forming its private transaction siblings, and failed staging must have an + explicit rule for parent-directory residue. +- Impact: An explicitly selected valid destination can fail only after + comparison generation has completed. If nested parents were missing, the + failed command also leaves empty operator-visible directories despite never + creating a bundle. +- Recommendation: Use bounded fixed transaction prefixes with random suffixes + that fit independently of the final basename, or reject targets whose + derived components cannot fit during read-only preflight. Decide and document + whether newly created empty parents are retained or safely rolled back. +- Test implications: Use filesystem-reported name limits where available and + cover the longest accepted final component, successful initial publication + and replacement, staging/backup cleanup, and the chosen missing-parent + failure lifecycle. +- Validation: Every preflighted basename can create both sibling forms, and a + pre-commit failure leaves only the documented parent-directory state. +- Related findings: `AUD-046` +- Remediation reference: pending + ## Retained Decisions ### RET-001: Keep the application package as the explicit composition owner @@ -2712,6 +2922,41 @@ retry/idempotency mechanics to the pinned Distributor client; address endpoint, response, and production-boundary test gaps without moving application gating policy into the adapter. +### RET-036: Keep comparison bundles flat, versioned, and self-validating + +One current schema indexes the exact shared data-package bytes, ordered profile +outcomes, bounded safe failures, and only the Markdown files produced by +successful profiles. Canonical JSON encoding is deterministic, the data package +has an explicit SHA-256 digest, artifact paths are flat basenames, and logical +bundle validation reconciles manifest outcomes with in-memory report payloads +before filesystem work. This is a compact durable consumer contract with no +application workspace semantics. Retain it while making strict decoding and +canonical report-name enforcement complete under `AUD-053` and `AUD-054`. + +### RET-037: Keep destination planning read-only and reauthorize at commit time + +`PlanDestination` cleans exact absolute paths without mutation, rejects the +filesystem root, working directory, final symlinks, non-directories, and +unrecognized nonempty content, and requires explicit replacement authority for +a current bundle. `Publish` does not trust the caller's earlier snapshot: it +replans after staging, then reauthorizes the exact entry again after moving it +to a private backup. This layered state classification protects ordinary +operator directories from stale preflight decisions. Retain both gates and the +moved-entry check; Stage 25 remains responsible for adversarial component swaps +inside the remaining syscall intervals. + +### RET-038: Keep sibling staging and explicit post-commit cleanup state + +Publication writes restrictive regular files into a private sibling directory, +places the manifest last, and makes the staging-to-target rename the point at +which new artifact paths become authoritative. Replacement first moves the +recognized old target aside, restores it if installing staging fails, and does +not roll back a successfully installed new bundle merely because old-backup +cleanup fails. `PublicationResult.Committed` and a typed cleanup error let the +application distinguish pre-commit failure from a valid committed bundle with +remaining cleanup work. Retain this transaction shape while adding the final +cancellation gate in `AUD-055` and truthful partial-cleanup state in `AUD-056`. + ## Open Questions No Stage 1 open questions or unexplained baseline failures remain. @@ -2991,6 +3236,26 @@ Stage 16 routed these investigation leads to their assigned later stages: the cancellation/accounting contract in `AUD-051` is settled; Stage 16 did not treat the graph metric alone as a defect. +Stage 17 routed these investigation leads to their assigned later stages: + +- `RecognizeBundle` reads every report file fully even though the manifest has + no report digest and recognition uses only file type and membership for those + entries. With generated Markdown already unbounded under `AUD-038`, Stage 22 + should measure and assess this avoidable allocation alongside comparison + bundle construction rather than Stage 17 creating an unrelated size policy. +- Existing non-final symlink components are permitted when they resolve to + directories, while final target and bundle-entry symlinks are rejected. + Concurrent swaps remain possible between `Lstat`, reads, renames, and backup + cleanup. Stage 25 should exercise those adversarial component and cleanup + races and decide whether descriptor-relative operations are warranted; + Stage 17 verified the ordinary static states only. +- Sibling directory renames provide atomic visibility but publication does not + synchronize files or parent directories for power-loss durability. No + canonical contract promises crash persistence, matching the Stage 15 + conclusion for single reports. Stage 24 should keep “transactional” and + “atomic” wording scoped to visible namespace state rather than durable-media + acknowledgment. + ## Stage Log ### Stage 1: Establish The Baseline And Audit Ledger @@ -3857,3 +4122,62 @@ Stage 16 routed these investigation leads to their assigned later stages: - Retained decisions: `RET-033` through `RET-035`. - Open questions: the three leads recorded above are routed to their assigned later stages. + +### Stage 17: Audit Comparison Contracts And Transactional Publication + +- Status: Complete. +- Scope reviewed: all production code and focused tests in + `internal/comparison`, the durable comparison bundle contract, comparison + publication internals, and the narrow architecture/operations rules needed + to establish replacement authorization and recovery semantics. +- Exclusions: Application preparation, concurrent profile execution, partial + result construction, and CLI integration remain Stage 18; repository-wide + test, efficiency, and refactoring review remain Stages 19-23; + documentation-wide reconciliation remains Stage 24; adversarial pathname and + crash robustness remain Stage 25; and remediation remains outside this audit + stage. + +#### Durable Bundle And Filesystem Transaction Accounting + +| Contract or risk | Model, destination, mutation, recovery, and test evidence | Disposition | +| --- | --- | --- | +| Logical identity and names | Profile IDs require at least two exact distinct nonblank values. Comparison IDs derive from report run IDs; directory names derive from report output names; profile slugs retain the documented ASCII set and byte bound; ordinal width expands at 100 and higher. | Pure helpers are deterministic and focused tests cover boundaries. Manifest validation does not connect successful paths back to `ReportFilename`; `AUD-054`. | +| Manifest schema and encoding | One current version validates nonzero ordered timestamps, valid period, identity/hashes, at least two contiguous distinct profile results, exact counters, success/failure shapes, bounded UTF-8 messages, and unique safe report basenames. Encoding is two-space indented in declaration order with one newline. | Core shape is coherent and retained as `RET-036`. Recognition accepts duplicate/case-variant JSON fields (`AUD-053`) and arbitrary safe report names (`AUD-054`). | +| Logical payload agreement | `LogicalBundle.Validate` hashes the exact data-package bytes, requires one report for each and only each successful manifest result, and reconciles report order, positions, and paths before any filesystem mutation. Failed results carry no Markdown. | Coherent aside from the shared canonical-name gap in `AUD-054`. Report-content hashes are deliberately not part of the current schema. | +| Recognition and compatibility | Recognition requires a real directory containing exactly the declared manifest, data package, and successful reports as regular non-symlink files; rejects unknown fields, trailing JSON, unsupported versions, unsafe paths, extra/missing entries, and a data digest mismatch. | Exact file/type/digest protection is strong. Decoder ambiguity is `AUD-053`; full report reads are routed to Stage 22. | +| Read-only destination authorization | Planning requires clean absolute working/target paths, rejects roots, the exact working directory, final symlinks, non-directories, unreadable states, and nonempty content without explicit recognized replacement; missing-parent inspection creates nothing. Empty targets remain usable without `--replace`. | Coherent and retained as `RET-037`. Existing parent symlinks and adversarial path changes are routed to Stage 25. | +| Staging mutations | After bundle validation and replanning, publication creates missing parents, a mode-`0700` sibling staging directory, and mode-`0600` data/report/manifest files with exclusive creation; manifest is written last and deferred cleanup removes uncommitted staging. | Restrictive complete staging is coherent. Derived staging/backup components reject valid long targets and can leave new parents; `AUD-057`. Mid-write failure seams are simple and remain pre-target. | +| Initial commit | Immediately before installing an absent target, publication replans, exposes a deterministic final test hook, checks context, and uses one sibling rename as the visibility linearization point. Rename failure retains no target and deferred staging cleanup runs. | Coherent for ordinary filesystem state and retained in `RET-038`. Syscall races remain Stage 25. | +| Replacement authorization and rollback | Replacement moves the current empty or recognized target to a unique sibling, re-runs destination/recognition policy against the moved entry, and restores it if authorization or staging installation fails. A reappeared target is not overwritten during restoration; the error retains the backup path. | Strong ordinary-state defense, retained as `RET-037` and `RET-038`. Cancellation after the first rename is not rechecked before install; `AUD-055`. | +| Committed state and backup cleanup | Once staging is renamed to target, the new bundle remains authoritative. Successful backup removal returns clean commit; removal failure returns `Committed: true`, a typed error, and a sibling path rather than rolling back the new bundle. | Correct commit truth is retained as `RET-038`. Recursive cleanup may already have damaged or removed the old bundle, so the reported retained-backup state is overstated in `AUD-056`. | +| Safe and recovery errors | Destination kinds and `ErrUnrecognizedBundle` are inspectable; pre-commit restoration errors join the triggering cause with target/backup recovery context; post-commit cleanup errors unwrap the filesystem cause. Manifest failures remain bounded through `SafeError`. | Error identity is actionable. `AUD-056` requires surviving cleanup state to be classified truthfully; Stage 18 owns safe CLI projection of these package errors. | +| Test ownership and transaction cost | Model tests own naming, invariants, encoding, hashing, and logical reconciliation. Real `t.TempDir` tests own layout, modes, recognition, destination states, commit, restore, reauthorization, retained paths, and cancellation; injected rename/remove operations deterministically reach recovery states. Work is linear in profiles and artifact bytes. | Good package-boundary ownership. Missing strict decoding, canonical-name, post-backup cancellation, partial-cleanup, and long-name cases attach directly to `AUD-053` through `AUD-057`; broader cost and suite maintenance remain Stages 20-22. | + +#### Commands And Evidence + +- Used the codebase graph architecture view, symbol search, exact snippets, and + caller/callee metadata for manifest validation/encoding, identity helpers, + destination planning, recognition, publication, restoration, and cleanup. + Bounded text inspection covered all four scoped Go files, every focused test, + the exact Stage 17 boundary, and all assigned canonical documents/policies. +- Traced every filesystem mutation from parent creation through staging files, + absent-target commit, old-target backup, moved-entry reauthorization, + installation, restoration, staging cleanup, and post-commit backup cleanup. + Cross-checked which state authorizes each mutation and which path/result/error + remains after every failure branch. +- Ran temporary deterministic package probes confirming acceptance of ambiguous + manifest fields and a noncanonical safe report path, committed replacement + after cancellation immediately following backup rename, an advertised backup + made unrecognizable by partial cleanup, and late staging failure plus parent + residue for a valid 240-byte target basename. The probe file was removed and + no production/test source change was retained. +- Ran the focused comparison race/coverage command with its profile at + `/tmp/weatherreporter-stage17-comparison-cover.out`; it passed with 83.5% + statement coverage. Coverage guided inspection of staging and transaction + failure branches 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-053` through `AUD-057`. +- Retained decisions: `RET-036` through `RET-038`. +- Open questions: the three leads recorded above are routed to their assigned + later stages.