diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md new file mode 100644 index 0000000..a54ceed --- /dev/null +++ b/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.