Consolidate audit findings and remediation priorities

docs/roadmap/audit.md

@@ -0,0 +1,448 @@
+# Consolidated audit recommendations
+
+Reviewed against `91f25c4` on `main`. All eleven reports under
+[`audit/`](audit/) were reviewed, with selected implementation and contract
+checks. Changes since their earliest baseline affect audit documents only;
+the recorded implementation findings remain pending. This document owns the
+consolidated recommendations. The individual reports retain detailed evidence
+and reproduction limits.
+
+The 27 findings across the reports consolidate into 20 work items. Missing
+regressions accompany the defects they should protect. Repeated reports of
+subprocess shutdown and Git inspection duplication each become one work item.
+Temporary-storage fixes and unused-helper removals are grouped by ownership,
+with their distinct cases identified below.
+
+## Priorities
+
+Priority indicates recommended work order, independent of the original audits'
+severity labels. P1 addresses release integrity, credential confidentiality,
+and cancellation guarantees. P2 improves recovery, completed-output contracts,
+cleanup, and bounded asset inspection. P3 covers focused internal improvements.
+P4 defers implementation pending sizing or performance evidence.
+
+| ID | Priority | Work and recommended disposition |
+| --- | --- | --- |
+| A01 | P1 | Force the intended tag push to include the repository itself. |
+| A02 | P1 | Use original Git objects consistently for release evidence. |
+| A03 | P1 | Complete cancellation and shutdown across buffered and piped commands. |
+| A04 | P1 | Redact Git URL credentials in diagnostics. |
+| A05 | P2 | Preserve recovery evidence after local tag creation. |
+| A06 | P2 | Return completed artifacts only after their digests are available. |
+| A07 | P2 | Preserve temporary cleanup paths and report cleanup failures accurately. |
+| A08 | P2 | Apply a finite budget to manually followed asset redirects. |
+| A09 | P2 | Stop remote mismatch reads at the expected size plus one byte. |
+| A10 | P3 | Bound retained status-only Go diagnostics while forwarding complete output. |
+| A11 | P4 | Establish supported JSON response sizes before introducing response caps. |
+| A12 | P3 | Consolidate common Git observations alongside the related fixes. |
+| A13 | P3 | Separate Go validation and committed-tree ownership when touching validation. |
+| A14 | P3 | Keep one normalized settings representation in internal build results. |
+| A15 | P3 | Extract CLI command handlers when updating the affected workflows. |
+| A16 | P3 | Share pure artifact-declaration matching between receipts and delivery. |
+| A17 | P4 | Measure exclusion-list scaling before replacing the current scan. |
+| A18 | P3 | Recheck queued read cancellation after acquiring the reader mutex. |
+| A19 | P3 | Replace incidental test assertions with the behavior they protect. |
+| A20 | P3 | Remove the two unused internal helpers, preserving live behavior. |
+
+The first four items deserve the earliest attention: a release can currently
+be reported as published without a remote tag, validated source can differ
+from published source, canceled operations can remain blocked, and diagnostics
+can expose configured credentials. Internal cleanup can follow or accompany
+those repairs where it reduces duplicated work. These priorities introduce no
+external review or operational gates between work items.
+
+## P1: integrity, confidentiality, and cancellation
+
+### A01. Inherited submodule push mode can suppress publication
+
+Sources: [correctness §1](audit/01-correctness.md#1-high-publication-can-report-success-while-leaving-the-tag-unpublished)
+and [testing §1](audit/08-testing.md#1-publication-fixtures-omit-git-settings-that-change-what-is-published).
+
+**Recommendation:** Add `--recurse-submodules=no` to the ReleaseKit-owned exact
+tag push in `internal/git`. This directly prevents
+`push.recurseSubmodules=only` from suppressing the repository push while Git
+returns success. Protect it through a public publication regression using a
+real local bare remote and an exact remote tag-object assertion.
+
+**Alternative:** Verify the exact authoritative ref after every successful
+push. This supplies independent confirmation and could cover other anomalous
+success responses, but adds network work and uncertainty when the verification
+request fails after an acknowledged push. It also leaves the inherited push
+mode's behavior to be corrected separately. Prefer the targeted command fix;
+consider additional successful-push verification only for an established need.
+
+### A02. Replacement objects separate validated and published content
+
+Sources: [correctness §2](audit/01-correctness.md#2-high-git-replacement-objects-can-separate-validated-content-from-published-source)
+and [testing §1](audit/08-testing.md#1-publication-fixtures-omit-git-settings-that-change-what-is-published).
+
+**Recommendation:** Apply an original-object view consistently to
+ReleaseKit-owned Git inspection, using `GIT_NO_REPLACE_OBJECTS=1` or the
+equivalent command option. Cover committed identity/note reads, batch blobs,
+source-status comparisons, and direct build inspection. A replacement-derived
+worktree should be compared with the original committed tree before release
+evidence is accepted. Consistency across those paths matters more than changing
+one object reader. Preserve ordinary dirty-build behavior and its source
+eligibility reporting.
+
+**Alternative:** Detect replacements affecting the selected commit and contents
+and return a contextual error. This makes the unsupported situation explicit
+but requires correctly identifying affected commits, trees, and blobs. Prefer
+the common original-object view, which also permits unrelated replacement refs.
+Regressions should compare actual validated and remote source/note bytes;
+equal commit IDs alone did not catch the defect.
+
+### A03. Inherited descriptors defeat cancellation and bounded recovery
+
+Sources: [correctness §3](audit/01-correctness.md#3-medium-inherited-process-pipes-can-defeat-cancellation-and-recovery-deadlines),
+[concurrency §1](audit/04-concurrency.md#1-medium-inherited-descriptors-can-keep-canceled-operations-and-reader-shutdown-blocked),
+and [testing §2](audit/08-testing.md#2-cancellation-fixtures-do-not-cover-inherited-subprocess-descriptors).
+
+**Recommendation:** Give command I/O a bounded shutdown path after cancellation
+or child exit, using standard `exec.Cmd.WaitDelay` where applicable and
+explicit ownership of piped-process closure. Verify the batch reader's stdout
+drain and child wait together, and cover direct build subprocesses. Preserve
+context identity, worker joining, and the existing publication recovery bound
+and uncertainty classifications. A03 is one repair spanning process and Git
+ownership, rather than separate fixes for each audit report.
+
+**Alternative:** Use platform-specific process-group termination to interrupt
+descendants as well. That can help when descendant execution itself must stop,
+but brings portability and process-ownership decisions beyond returning from
+blocked I/O. Prefer the standard shutdown mechanisms for the observed defect.
+Tests should establish a descendant's descriptor ownership before canceling,
+then exercise buffered commands, batch-reader cleanup, and a public path.
+
+### A04. Git URL credentials appear in errors and CLI output
+
+Source: [security §1](audit/07-security.md#1-medium-publication-diagnostics-expose-credentials-embedded-in-git-urls).
+
+**Recommendation:** Centralize credential-safe endpoint rendering for
+ReleaseKit-owned diagnostics and CLI output, including expected/resolved URL
+comparisons and reconciliation errors. Mask HTTP(S) userinfo credentials,
+including username-carried tokens, and check wrapped Git diagnostic text.
+Keep original URLs for transport and exact comparison. Preserve the documented
+raw `PublicationResult.Endpoint` value and explain its sensitivity in GoDoc;
+its CLI rendering should use the safe representation. This fixes the observed
+log exposure without changing destination identity or the public result field.
+
+**Alternative:** Return only a sanitized endpoint publicly. This reduces the
+chance that callers log a secret, but changes the current captured-URL contract
+and removes information callers may use for exact comparison. Consider that
+only as a deliberate API change. Regressions should verify both safe output and
+unchanged exact matching with placeholder credentials.
+
+## P2: recovery, output contracts, and asset handling
+
+### A05. Post-creation tag inspection loses recovery evidence
+
+Sources: [error handling](audit/06-error-handling.md#a-post-creation-inspection-failure-hides-a-retained-local-tag)
+and [testing §3](audit/08-testing.md#3-local-tag-creation-lacks-a-post-write-inspection-failure-case).
+
+**Recommendation:** Return any verified tag object already obtained together
+with a later inspection error. When the object cannot be established after
+creation, identify the local ref and explicitly report that a tag may remain.
+Keep the failed verification visible and the remote outcome at `not_published`;
+existence alone does not make the tag eligible for push. This supplies useful
+recovery information within the existing result and error model.
+
+**Alternative:** Perform a bounded independent local inspection after failure
+to gather additional evidence, or add a structured creation-progress result if
+callers need it. Those options improve machine-readable recovery but add work
+or surface area. Prefer preserving existing evidence and clear partial-state
+errors first. A real repository and controlled post-write inspection failure
+should verify the retained ref, cause, and absence of a push.
+
+### A06. A partial build lists an artifact before hashing completes
+
+Source: [interfaces](audit/09-interfaces.md#a-partial-build-result-can-contain-an-artifact-without-its-promised-digest).
+
+**Recommendation:** Hash a newly compiled artifact before appending it to the
+completed `Artifacts` list. On failure, retain earlier completed artifacts and
+name the affected output path in the error. Keep the file available for the
+caller. This restores the documented digest/completed-output contract with a
+small internal change and consistent CLI presentation.
+
+**Alternative:** Represent compiled but unverified output explicitly in a
+separate result state. This supports richer recovery but adds an API distinction
+for which the audits found no current consumer need. Prefer completed-only
+entries. Verify a post-compilation read failure through the public build path,
+including preservation of earlier artifacts and the underlying cause.
+
+### A07. Temporary cleanup loses paths or silently discards failures
+
+Sources: [resources §3](audit/05-resources.md#3-temporary-build-cleanup-failures-are-silently-discarded)
+and [§4](audit/05-resources.md#4-a-failed-temporary-path-resolution-loses-the-cleanup-path).
+
+**Recommendation:** Address both cases in one cleanup-focused unit. In
+`tempdir.Outside`, retain the original created path until resolution succeeds
+and use it on resolution failure, in both allocation branches. At owned build,
+tag-message, and receipt cleanup sites, retain removal failures with the path
+and join them with an existing cause where useful. This corrects concrete
+ownership gaps with ordinary filesystem operations.
+
+After confirmed hosted delivery, preserve `DeliveryComplete` and the known
+remote observations while reporting the cleanup error separately in the error
+text and CLI presentation. That distinguishes completed remote work from
+failed local housekeeping. Caller-owned prebuilt output retains its lifecycle.
+
+**Alternative:** Emit cleanup warnings while returning success. This preserves
+the primary operation's success/error convention, but an optional output writer
+can leave library callers unaware of leaked storage. Prefer a returned error
+with accurate result state. Structured cleanup metadata is another option if
+automation demonstrates a need; a new public error taxonomy is not required
+for contextual errors. Verify failure attribution and retained paths without
+requiring hostile filesystem actors or power-loss guarantees.
+
+### A08. Manual asset redirects have no finite hop budget
+
+Source: [security §2](audit/07-security.md#2-medium-unique-asset-redirects-bypass-the-http-clients-finite-redirect-limit),
+also referenced by the resource audit.
+
+**Recommendation:** Propose ten followed redirects per asset request as the
+initial internal default, matching the ordinary Go client convention, and
+report exhaustion as unverifiable content. Preserve exact-cycle detection,
+body closure, and per-origin authorization checks. This bounds both request
+count and retained URL history. It is an explicit proposed policy choice;
+assess a longer-chain override only for demonstrated consumer requirements.
+
+**Alternative:** Rely on an operation deadline. It preserves arbitrary chain
+lengths and bounds elapsed time when set, but the CLI currently supplies no
+automatic deadline and time alone does not directly bound retained history.
+Prefer a hop budget for this specific defect. Test distinct-URL chains and
+credential handling at the boundary, using local servers.
+
+### A09. Mismatching assets are read beyond their expected size
+
+Source: [security §3](audit/07-security.md#3-low-remote-content-verification-reads-beyond-the-known-matching-size),
+also referenced by the efficiency and resource audits.
+
+**Recommendation:** Pass the locally verified expected size to remote
+verification. Read at most that size plus one byte, with overflow-safe counting;
+extra or missing bytes establish a conflict. Compare digests for equal-length
+streams and retain read, close, and cancellation errors. This bounds excess
+work using existing artifact facts and preserves supported artifact sizes.
+
+**Alternative:** Use reported remote size or HTTP content length to reject a
+mismatch earlier. Such metadata can be absent or inaccurate; it cannot replace
+counting actual bytes. A fixed global download cap would also reject legitimate
+large binaries. Prefer the declaration-derived bound. Cover short, equal,
+oversized, and interrupted streams plus a public delivery conflict case.
+
+## P3 and P4: resource policy and focused maintenance
+
+### A10. Status-only Go output accumulates in memory
+
+Source: [resources §1](audit/05-resources.md#1-go-command-output-is-retained-for-the-entire-execution).
+
+**Recommendation:** For status-only `Tool.Run` and dependency preparation,
+forward the complete stream while retaining a bounded diagnostic tail for
+failure reporting. Start with a proposed 1 MiB internal tail and an explicit
+truncation notice; keep full output for `Tool.Output` consumers. This is a
+deliberate change to diagnostic retention, to be documented with implementation.
+It bounds memory without adding consumer configuration or disk lifecycle work.
+
+**Alternative:** Spool full diagnostics to owned temporary storage. This retains
+complete failure history but requires a disk budget, cleanup, and a way for the
+caller to retrieve retained output. Choose it if actual consumers require full
+failure capture. Preserve forwarding failures and process error identity under
+either approach; benchmarks should measure memory with large output.
+
+### A11. Forge JSON decoding has no response budget
+
+Source: [resources §2](audit/05-resources.md#2-forge-json-responses-have-no-resource-bound).
+
+**Recommendation:** Defer immediate size/time caps and first establish the
+legitimate response envelope for repository metadata, notes, and unpaginated
+attachment lists. The source inspection establishes unbounded consumption, but
+the audit supplies no representative sizes or selected protocol maximum. Record
+those sizes and then propose explicit per-response byte budgets, with a
+distinguishable limit error. Current context cancellation remains applicable.
+
+**Alternative:** Introduce configurable limits now with generous defaults. This
+offers earlier containment for faulty forges but adds public or configuration
+policy and can reject supported notes or attachment lists. Prefer sizing before
+that change. Any eventual create/upload response limit must preserve uncertain
+write reconciliation: a rejected response is not proof of a rejected write.
+
+### A12. Common Git observations have several owners
+
+Sources: [complexity §2](audit/03-complexity.md#2-git-source-inspection-is-repeated-across-package-boundaries)
+and [equivalence §1](audit/10-equivalence.md#1-source-state-observations-are-independently-implemented).
+
+**Recommendation:** Move small HEAD and worktree-status observations behind the
+existing internal Git boundary when repairing A02/A03, or immediately afterward
+if a separate change is easier to verify. This reduces duplicated subprocess,
+cancellation, and object-view handling. Keep observation timing and decisions
+with callers: candidate checking requires clean `main`, builds record dirty
+source eligibility, and delivery checks a tag.
+
+**Alternative:** Patch each current implementation separately and retain the
+duplication. It minimizes refactoring during an urgent repair but leaves several
+places to maintain the same Git mechanics. Prefer narrow shared primitives;
+a generalized release-state object would combine genuinely different policies.
+Retain independent caller-level behavioral tests.
+
+### A13. Shared validation mixes sequencing and tree ownership
+
+Source: [complexity §1](audit/03-complexity.md#1-shared-validation-combines-several-kinds-of-orchestration).
+
+**Recommendation:** Extract private Go-command and committed-tree sequences
+when the owner is next changed. Let the tree sequence own closure and its
+attribution, with check ordering readable in one place. This reduces the number
+of distant edits needed for a new check without changing the facade.
+
+**Alternative:** Keep the current function with explicit blocks until another
+check makes the maintenance cost material. That is viable because complexity
+scores alone do not establish a defect. Prefer small extractions during related
+work; preserve nil/empty selections, failure precedence, and reader-close
+semantics. A generic check registry adds machinery without a demonstrated need.
+
+### A14. Internal build results duplicate normalized settings
+
+Source: [complexity §3](audit/03-complexity.md#3-internal-build-results-retain-duplicate-settings).
+
+**Recommendation:** Retain `Settings` as the internal source of truth and remove
+the duplicate internal `MainPackage`, `BinaryName`, and `Targets` fields. Update
+build execution and conversions together. Public `BuildResult` and receipt
+fields continue to serve their existing contracts. This prevents internal drift
+with modest, localized work.
+
+**Alternative:** Keep the duplicate fields and enforce consistency through a
+constructor. It can reduce call-site changes but preserves redundant state and
+ongoing maintenance. Prefer removal; verify partial outputs, facade conversions,
+and prebuilt delivery independently.
+
+### A15. CLI routing contains complete command workflows
+
+Source: [complexity §4](audit/03-complexity.md#4-cli-command-handling-interleaves-routing-and-command-specific-workflows).
+
+**Recommendation:** Extract named private check/publication and build handlers
+when changing those workflows. Keep `run` focused on parsing, repository
+resolution, and dispatch. Existing delivery handlers demonstrate a sufficient
+local pattern. Preserve receipt invalidation order, output context, profile
+mapping, and exit classifications.
+
+**Alternative:** Retain the current switch while making only targeted fixes.
+This keeps urgent diffs small and is acceptable short term. Prefer extraction
+as the command is touched; a new command framework or public interface would
+add migration cost without addressing an observed consumer need.
+
+### A16. Receipts and delivery repeat declaration matching
+
+Source: [equivalence §2](audit/10-equivalence.md#2-build-artifact-declarations-are-checked-independently-by-receipts-and-delivery).
+
+**Recommendation:** Extract a small pure target/filename matching helper into
+`internal/artifact`, building on `ExpectedBuild`. It should own uniqueness,
+counts, and one expected name per target. Keep receipt schema/path/digest rules
+in the CLI and filesystem/content checks in delivery after preflight. This
+shares one concrete identity rule across existing boundaries.
+
+**Alternative:** Keep the two loops while sharing only name generation, as
+today. That avoids another helper and is viable while conventions are stable,
+but requires coordinated edits to the same matching rules. Prefer the narrow
+helper during build-model maintenance, with concrete independent expected
+filenames in tests and unchanged source-only handling.
+
+### A17. File-size exclusions scale with files times exclusions
+
+Source: [efficiency](audit/02-efficiency.md#file-size-exclusions-are-scanned-for-every-selected-file).
+
+**Recommendation:** Defer an implementation change until a benchmark varies
+both repository size and exclusion count. The reported O(N × E) scan is real,
+but the audit measured no practical slowdown; a small exclusion list may favor
+the existing straightforward loop.
+
+**Alternative:** Index normalized exclusions in a set now and check each file
+and its ancestors. This removes the E factor for large lists at the cost of
+allocation and path walking. Prefer it if measurements justify the tradeoff.
+A trie is viable for very deep paths but has greater implementation cost.
+Preserve literal component boundaries, module-relative normalization, shared
+inputs, and the first oversized-file result under any change.
+
+### A18. A canceled queued read can terminate a healthy blob session
+
+Source: [concurrency §2](audit/04-concurrency.md#2-low-a-canceled-queued-blob-read-can-later-abort-a-healthy-session).
+
+**Recommendation:** Recheck `ctx.Err()` immediately after acquiring the reader
+mutex and before touching the session or installing its cancellation callback.
+This small change prevents already-canceled queued work from aborting a healthy
+child. A controlled helper regression should verify a successful first read
+and subsequent uncanceled read after canceling queued work.
+
+**Alternative:** Make acquisition cancellation-aware so queued calls also
+return promptly. It supports a stronger concurrent-use contract but changes
+the synchronization mechanism. Current validation reads sequentially; defer
+that expansion until a real caller needs it. Keep the existing mutex and
+operation-local candidate caches for the present fix.
+
+### A19. Tests assert mechanisms beyond the protected behavior
+
+Source: [testing §4](audit/08-testing.md#4-some-assertions-lock-in-incidental-internal-mechanisms).
+
+**Recommendation:** Remove Markdown cache pointer-identity assertions after
+retaining unique parsing and source-relative resolution coverage. Replace the
+Linux exact-open-count test with existing integrity/ordering coverage and a
+read-volume benchmark if that efficiency concern remains useful. Remove the
+success-path assertion of exactly three remote queries while retaining exact
+remote tag/message and unrelated-ref checks. The documented single query after
+a failed push remains a separate recovery contract.
+
+**Alternative:** Retain a mechanism assertion as an explicit internal regression
+when it protects a demonstrated resource problem that behavior tests miss.
+Explain that purpose and give refactoring an equivalent observable protection.
+Prefer behavioral assertions and benchmarks for the three reported cases;
+passing existing tests did not detect the audited publication defects.
+
+### A20. Two internal helpers have no production purpose
+
+Sources: [dead code §1](audit/11-deadcode.md#1-committed-selection-identity-wrapper-has-no-callers)
+and [§2](audit/11-deadcode.md#2-local-tag-inspection-helper-is-used-only-by-its-own-test).
+
+**Recommendation:** Remove `FromCommittedSelection`, retaining its private
+implementation and the live selection APIs. Remove `InspectLocalTag` and its
+direct test call, retaining the live candidate guard and the test's other full
+tag/source assertions. Current source references support these removals, which
+reduce internal surface without changing the public facades. Preserve tree
+accessors and constructors whose tests/benchmarks exercise meaningful behavior.
+
+**Alternative:** Use `InspectLocalTag` in the live guard during A12 if it
+actually reduces duplicate mechanics while preserving context, exact-object
+checks, and guard timing. That gives the helper a real owner but requires
+checking its redundant validation and error ordering. Prefer removal as the
+smaller standalone cleanup; keep it only if the shared Git change supplies a
+concrete production use.
+
+## Coverage of the source reports
+
+Each original finding has a disposition above. Cross-references to findings in
+other audits supply supporting context rather than additional work items.
+
+| Report | Original finding → consolidated work |
+| --- | --- |
+| [01 Correctness](audit/01-correctness.md) | 1 → A01; 2 → A02; 3 → A03 |
+| [02 Efficiency](audit/02-efficiency.md) | Exclusion scanning → A17; remote-read cross-reference → A09 |
+| [03 Complexity](audit/03-complexity.md) | 1 → A13; 2 → A12; 3 → A14; 4 → A15 |
+| [04 Concurrency](audit/04-concurrency.md) | 1 → A03; 2 → A18 |
+| [05 Resources](audit/05-resources.md) | 1 → A10; 2 → A11; 3 and 4 → A07; cross-referenced shutdown/asset issues → A03/A08/A09 |
+| [06 Error handling](audit/06-error-handling.md) | Retained local tag → A05 |
+| [07 Security](audit/07-security.md) | 1 → A04; 2 → A08; 3 → A09 |
+| [08 Testing](audit/08-testing.md) | 1 → A01/A02; 2 → A03; 3 → A05; 4 → A19 |
+| [09 Interfaces](audit/09-interfaces.md) | Missing partial-artifact digest → A06 |
+| [10 Equivalence](audit/10-equivalence.md) | 1 → A12; 2 → A16 |
+| [11 Dead code](audit/11-deadcode.md) | 1 and 2 → A20 |
+
+## Verification and scope of follow-up
+
+This consolidation adds recommendations and changes no production behavior.
+Original reproductions, source inspection findings, coverage figures, and
+scanner results retain their stated limits. Existing passing tests establish
+a useful baseline; regressions should demonstrate the behavior of each repaired
+defect and accompany that repair. Reuse the normal offline suite, focused race
+checks for shutdown/read changes, and compiled facade/dependency checks when
+implementation changes warrant them.
+
+Keep the two public facades, explicit forge selection, caller ownership of
+prebuilt artifacts, original credential origin checks, and additive delivery
+semantics. The proposed diagnostic/redirect budgets are deliberate recommendations;
+JSON limits and exclusion indexing remain deferred as explained above.