Record Stage 17 comparison publication audit

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.