diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md index 3db3187..e44185a 100644 --- a/docs/roadmap/audit.md +++ b/docs/roadmap/audit.md @@ -1,111 +1,83 @@ -# Audit remediation roadmap +# Audit remediation target -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 -accepted remediation priorities and approaches. The individual reports retain -detailed evidence and reproduction limits. +The target state and policy choices below are accepted. They cover the 27 +findings consolidated from the eleven reports in [`audit/`](audit/) into 20 +work items. Original reports retain reproduction evidence and limitations. +The implementation baseline is `b5b3f09`; only documentation changed since the +audits' source baselines. Implementation status and ordered work are owned by +[the implementation plan](implementation.md). -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. +## Accepted scope and priorities -## Accepted decisions and status +P1 addresses publication integrity, credential confidentiality, and cancellation. +P2 covers recovery, completed-output contracts, cleanup, and asset inspection. +P3 covers focused internal improvements. P4 retains the accepted deferral of +production changes while response sizing and exclusion performance are assessed. -The approaches and priorities for A01–A20 are accepted. Implementation remains -pending. Each recommendation below records the accepted approach; -alternatives are retained as decision rationale. +The target preserves the root and `buildinfo` facades, explicit forge selection, +caller ownership of prebuilt artifacts, credential origin checks, and additive +hosted delivery. The accepted resource policies are ten followed redirects per +asset request and a 1 MiB retained tail for status-only Go diagnostics. A11 and +A17 assessment supplies evidence for later policy or optimization decisions. -- A01–A04 are the first implementation priorities. -- A08 uses an internal limit of ten followed redirects per asset request. -- A10 retains a 1 MiB diagnostic tail for status-only Go commands while - forwarding complete output and preserving full `Tool.Output` results. -- A11 response caps and A17 exclusion indexing remain deferred pending the - sizing and benchmark evidence described in those items. -- Internal consolidation and refactoring follow the timing stated in their - accepted approaches, alongside related repairs or maintenance. - -## Priorities - -Priority indicates accepted 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 | Accepted work and disposition | +| ID | Priority | Target outcome | | --- | --- | --- | -| 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. | +| A01 | P1 | Exact source-tag publication with submodule recursion disabled. | +| A02 | P1 | Consistent original-object release evidence. | +| A03 | P1 | Bounded command I/O shutdown and joined pipe ownership. | +| A04 | P1 | Credential-safe Git URL diagnostics. | +| A05 | P2 | Accurate retained-tag recovery evidence. | +| A06 | P2 | Digests on every completed artifact. | +| A07 | P2 | Actionable cleanup errors with accurate primary outcomes. | +| A08 | P2 | At most ten followed redirects per asset request. | +| A09 | P2 | Content comparison bounded by expected size plus one byte. | +| A10 | P3 | Bounded status diagnostics and complete existing forwarding. | +| A11 | P4 | Response-size assessment; production caps deferred. | +| A12 | P3 | Shared Git observations with caller-owned policy. | +| A13 | P3 | Clear Go-validation and committed-tree ownership. | +| A14 | P3 | One internal normalized build-settings representation. | +| A15 | P3 | Private command workflows behind a small dispatcher. | +| A16 | P3 | Shared pure artifact-declaration matching. | +| A17 | P4 | Exclusion benchmarks; production indexing deferred. | +| A18 | P3 | Queued cancellation preserves a healthy reader session. | +| A19 | P3 | Tests protect behavior and selected resource concerns. | +| A20 | P3 | Unused helpers removed with live coverage preserved. | -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 +## Accepted outcomes and rationale ### 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. +**Target:** Source publication pushes only the selected annotated tag object, +with submodule recursion disabled. A successful publication identifies the +exact tag published to the authoritative endpoint. -**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. +**Rationale:** An inherited submodule-only push mode can otherwise return +success while leaving the release tag absent. + +**Considered alternative:** Successful-push read-back supplies independent +evidence but adds network work and an uncertain outcome when that read fails. +The selected remedy controls the owned push operation. ### 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. +**Target:** ReleaseKit-owned Git inspection uses original objects consistently +for module identity, notes, candidate files, and source eligibility. Working +source is compared with that view before release evidence is accepted. Ordinary +dirty builds retain their supported behavior; unrelated replacement refs remain +permitted. -**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. +**Rationale:** Replacement interpretation can make validated bytes differ from +the original objects transferred to the remote, despite matching recorded IDs. + +**Considered alternative:** Rejecting replacements affecting selected objects +is viable but needs additional commit/tree/blob reachability analysis. The +accepted original-object view provides one consistent interpretation. ### A03. Inherited descriptors defeat cancellation and bounded recovery @@ -113,320 +85,307 @@ Sources: [correctness §3](audit/01-correctness.md#3-medium-inherited-process-pi [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. +**Target:** Buffered command execution has bounded I/O teardown after +cancellation or child exit. Piped exchanges close owned pipes on operation +cancellation and join their cancellation work and child wait. Context error +identity and the existing bounded publication reconciliation remain meaningful. -**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. +**Rationale:** Descendants can retain descriptors after the immediate child +exits or is canceled. Both buffered execution and batch-reader cleanup need +ownership of their shutdown paths. + +**Considered alternative:** Process-group termination can stop descendants but +introduces platform and ownership policy beyond completing the selected +cancellation behavior. Standard subprocess and pipe mechanisms are the accepted +scope. ### 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. +**Target:** ReleaseKit-owned Git URL diagnostics and CLI output mask HTTP(S) +userinfo credentials, including username-carried tokens. Transport and exact +comparisons use original URLs. `PublicationResult.Endpoint` retains its raw +captured value, with its sensitivity documented. -**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. +**Rationale:** The observed mismatch and failure paths can expose configured +credentials to readers of logs or shared diagnostics. -## P2: recovery, output contracts, and asset handling +**Considered alternative:** Sanitizing the public endpoint value reduces +accidental logging but changes the captured-URL contract. The accepted choice +sanitizes diagnostic boundaries while preserving that contract. ### 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. +**Target:** Publication preserves any verified local tag object obtained before +a later inspection failure. When its identity is unavailable after creation, +the error identifies the ref and possible retained local tag. Failed +verification remains a failure, with no push and a `not_published` remote +outcome. -**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. +**Rationale:** Recovery needs accurate partial evidence even when the initial +tag command succeeded and a later local command failed. + +**Considered alternative:** Independent post-failure inspection or a new +creation-progress type can provide more machine-readable evidence, but adds +work or public surface. Existing results and contextual errors are sufficient +for the selected fix. ### 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. +**Target:** Every entry in the completed artifact list has its digest. A +hashing failure preserves earlier completed artifacts and identifies the +affected path in the error. Generated caller-owned files remain available for +recovery. -**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. +**Rationale:** An empty digest in a completed-output entry violates the current +partial-result contract even though the overall build returns an error. + +**Considered alternative:** A separate compiled-but-unverified result state is +viable if consumers need richer recovery. The accepted choice retains the +existing completed-only model. ### 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. +**Target:** Owned temporary paths remain identifiable through allocation, +resolution, and cleanup failures. Removal failures are returned with the +affected path and any primary cause. Confirmed hosted outcomes remain intact: +`DeliveryComplete` can accompany a local cleanup error, and the CLI +distinguishes completed remote work from failed housekeeping. Caller-owned +prebuilt artifacts retain their lifecycle. -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. +**Rationale:** Discarded errors and lost allocation paths can leave storage +behind without actionable recovery information. -**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. +**Considered alternative:** Warnings alone preserve primary success conventions +but can disappear when an optional writer is absent. Structured cleanup +metadata could support future automation; contextual returned errors are the +accepted representation. ### 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:** Allow up to 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. The ten-redirect budget is accepted; -assess a longer-chain override only for demonstrated consumer requirements. +**Target:** An asset request follows at most ten redirects. Limit exhaustion is +unverifiable content. Exact-cycle detection, response-body closure, and +per-origin authorization remain in place. A longer-chain override requires +demonstrated consumer need. -**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. +**Rationale:** A finite hop budget bounds requests and retained URL history for +distinct-URL chains supplied by external content hosts. + +**Considered alternative:** An operation deadline can bound elapsed time but +does not directly bound redirect history and is not automatically supplied by +the CLI. The selected protection is a redirect budget. ### 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. +**Target:** Remote content comparison consumes at most the locally verified +expected size plus one byte, with overflow-safe counting. Shorter or longer +content conflicts; equal-length content still requires a matching digest. Read, +close, and cancellation failures retain their causes. -**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. +**Rationale:** Once size mismatch is established, further download and hashing +cannot produce a matching asset and can consume unbounded resources. -## P3 and P4: resource policy and focused maintenance +**Considered alternative:** Reported remote size or content length can be +absent or inaccurate. A global byte cap would also restrict legitimate +binaries. The accepted bound derives from local artifact facts. ### 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. Retain up to 1 MiB internally with an explicit truncation -notice; keep full output for `Tool.Output` consumers. This accepted change to -diagnostic retention will be documented with implementation. -It bounds memory without adding consumer configuration or disk lifecycle work. +**Target:** Status-only Go command diagnostics retain at most a 1 MiB tail, +with an explicit truncation notice on failure. Existing output destinations +receive the complete stream; operations without a destination retain their +quiet behavior. `Tool.Output` continues returning complete output. Process and +forwarding error identity are preserved. -**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. +**Rationale:** A bounded tail controls retained memory for long-running +commands without adding public configuration or temporary-log ownership. + +**Considered alternative:** Full spooling retains complete failure history but +requires disk budgets, cleanup, and retrieval semantics. It remains a future +option if consumers require full captured failure history. ### 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. +**Target:** JSON response caps remain deferred pending a supported response +envelope for repository metadata, notes, and unpaginated attachment lists. +Assessment records available local sizing evidence and its limits. Context +cancellation and uncertain-write reconciliation remain unchanged. -**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. +**Rationale:** Source inspection demonstrates unbounded consumption, while the +reports establish neither representative maximum sizes nor a selected protocol +limit. Arbitrary caps would introduce a new compatibility restriction. + +**Considered alternative:** Configurable limits with generous defaults provide +earlier containment but add policy and may reject supported responses. Sizing +evidence precedes that future decision. ### 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. +**Target:** The existing internal Git boundary owns common HEAD and +repository-wide status observations. Callers retain their observation timing +and decisions: clean-main candidate rules, dirty-build eligibility, and tagged +delivery checks stay distinct. -**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. +**Rationale:** One mechanics owner reduces duplicated object-view, +cancellation, and error propagation handling. + +**Considered alternative:** Separate implementations keep individual repairs +small but retain parallel maintenance. A generalized release-state object would +combine different policies; narrow observations are the accepted abstraction. ### 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. +**Target:** Shared validation exposes a readable ordered Go-command sequence +and a committed-tree sequence that owns reader lifetime and close-error +attribution. Nil/empty selections, check order, failure precedence, and current +diagnostic identities are preserved. -**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. +**Rationale:** Separating these ownership responsibilities reduces distant +coordinated edits when checks change. + +**Considered alternative:** Explicit blocks in one large function are viable +while stable. The accepted simplification uses private functions within +existing packages; a plugin registry has no demonstrated consumer 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. +**Target:** Internal build results have one normalized `Settings` +representation. Public `BuildResult` and private receipt fields keep their +existing contracts and explicit boundary conversions. -**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. +**Rationale:** Removing duplicate internal package/name/target fields prevents +drift without changing consumer types. + +**Considered alternative:** A constructor can enforce duplicate-field +consistency but preserves redundant state and maintenance. A single internal +representation is accepted. ### 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. +**Target:** The CLI dispatcher delegates check/publication and build workflows +to named private handlers. Profile mapping, receipt invalidation order, +diagnostic context, and exit classifications retain their current +responsibilities. -**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. +**Rationale:** Command-specific workflows become easier to change and verify +without adding a command framework or public interface. + +**Considered alternative:** Keeping the current switch is viable for small +repairs but continues interleaving routing with full workflows. Private +handlers are the accepted organization. ### 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. +**Target:** Receipts and delivery share pure target/filename declaration +matching in `internal/artifact`. Receipt schema/path/digest rules remain +CLI-owned; source-only handling and filesystem/content verification remain with +delivery after preflight. -**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. +**Rationale:** The same identity rule has two callers and one natural internal +owner. Its independent boundary rules remain explicit. + +**Considered alternative:** Sharing only name generation avoids another helper +but duplicates uniqueness and matching logic. A narrow metadata-only helper is +accepted. ### 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. +**Target:** Exclusion indexing remains deferred pending benchmarks varying both +selected files and exclusion count. Production matching retains literal +component boundaries, module-relative normalization, shared inputs, and +first-oversized-file behavior. -**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. +**Rationale:** The O(N × E) scan is established, but practical slowdown has not +been measured; common small lists may favor the existing loop. + +**Considered alternative:** A set plus ancestor lookups removes the +exclusion-count factor at an allocation/path-walking cost. A trie is viable for +deeper paths with additional complexity. Adoption remains an evidence-driven +future choice. ### 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. +**Target:** A read canceled while queued is rejected after acquiring the reader +mutex and before it can change the healthy batch session. Current sequential +validation and mutex-based serialization remain in place. -**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. +**Rationale:** An already-canceled queued request can otherwise install a +callback that terminates a healthy child and affects subsequent requests. + +**Considered alternative:** Cancellation-aware acquisition also provides prompt +queued cancellation but changes synchronization and expands its effective +concurrent-use contract. That expansion remains deferred until a caller needs +it. ### 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. +**Target:** Tests protect parsing and source-relative resolution, artifact +integrity and ordering, and exact publication outcomes. Cache pointer identity, +exact binary-open counts, and successful-path remote query counts remain +implementation details. The documented single reconciliation query after a +failed push retains its separate 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. +**Rationale:** Mechanism assertions can reject correct refactoring while +failing to cover the observed correctness defects. Relevant performance +concerns are measured through benchmarks. + +**Considered alternative:** A mechanism assertion is appropriate when it +protects a demonstrated resource problem that behavioral checks miss. The three +reported cases use behavioral evidence and benchmarks instead. ### 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. +**Target:** The unused `FromCommittedSelection` wrapper and test-only +`InspectLocalTag` helper are removed. Their live private implementations, +selection APIs, candidate tag guard, and useful full-tag/source assertions +remain. Tree accessors with meaningful tests or benchmarks remain supported +internally. -**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. +**Rationale:** These two helpers have no current production purpose or +documented retention need and are outside the public facades. + +**Considered alternative:** A real shared-guard use could justify retaining +`InspectLocalTag`, but adds integration and ordering considerations. The +accepted standalone disposition is removal while preserving the live guard. ## Coverage of the source reports @@ -446,18 +405,3 @@ other audits supply supporting context rather than additional work items. | [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 roadmap records accepted work; production behavior remains unchanged. -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 diagnostic and redirect budgets above are accepted decisions; -JSON limits and exclusion indexing remain deferred as explained above. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md new file mode 100644 index 0000000..e001de9 --- /dev/null +++ b/docs/roadmap/implementation.md @@ -0,0 +1,578 @@ +# Audit remediation implementation plan + +## Objective and baseline + +Implement the accepted target in [audit.md](audit.md) from baseline `b5b3f09`. +The eleven [audit reports](audit/) supply evidence and reproduction details; +the target document owns the accepted outcomes and policy choices. This plan +owns execution order, concrete implementation choices, and completion evidence. + +All stages are pending. Follow Stage 1 through Stage 19 in order as one +end-to-end sprint. Stages separate work into units suitable for a Luna coding +agent. Intermediate stages may leave related code or tests uncompilable; +complete the dependencies before evaluating the final implementation. Run +focused checks when the affected packages are ready, and reserve the complete +verification pass for Stage 19. Proceed through the sprint without intermediate +external actions, pilot integrations, releases, or human review gates. + +This sprint implements the selected repairs and internal maintenance. A11 and +A17 deliver local assessment evidence while their production changes remain +deferred. New JSON limits, exclusion indexing, public configuration, broader +concurrent-use contracts, and alternative approaches are future decisions. +The public module path, Go minimum, facade types, receipt schema, and source +publication completion point keep their current contracts. + +Read [development.md](../development.md) and the applicable policies before +working. Preserve unrelated changes. Use existing fixture helpers, local Git +remotes, and local HTTP servers. Keep synthetic module-proxy tests in test-owned +module caches. All default tests remain offline and credential-independent. + +## Completion tracking + +Update this table with the completed work and relevant check results as the +sprint proceeds. A completed assessment in Stage 18 leaves the corresponding +production change explicitly deferred. Commit completed nonempty units under +the agent policy; an intermediate compilation dependency is recorded accurately. + +| Stage | Work items | Status | +| --- | --- | --- | +| 1 | A01; publication assertion portion of A19 | Pending | +| 2 | A02, A12: common Git mechanics | Pending | +| 3 | A02: source identity regression evidence | Pending | +| 4 | A03: buffered command teardown | Pending | +| 5 | A03, A18: piped lifetime and queued cancellation | Pending | +| 6 | A04 | Pending | +| 7 | A05 | Pending | +| 8 | A06, A14 | Pending | +| 9 | A16 | Pending | +| 10 | A07: allocation and temporary metadata | Pending | +| 11 | A07: build and delivery outcomes | Pending | +| 12 | A08 | Pending | +| 13 | A09 | Pending | +| 14 | A10 | Pending | +| 15 | A13 | Pending | +| 16 | A15 | Pending | +| 17 | Remaining A19, A20 | Pending | +| 18 | A11, A17: assessment only | Pending | +| 19 | Integrated verification and current documentation | Pending | + +## Stage 1: Correct the owned tag push + +Owners: [internal/git/tag.go](../../internal/git/tag.go) and +[publication tests](../../publish_test.go). Target: A01 and the publication +assertion portion of A19. + +1. Add `--recurse-submodules=no` to `Endpoint.PushTag`. Retain its captured + object-to-exact-tag refspec, `--no-follow-tags`, scoped endpoint, and + non-forcing push behavior. +2. Extend the real local-bare-remote publication fixture with + `push.recurseSubmodules=only`. Set it after fixture setup has published + `main`. A successful public `Publish` must leave the captured annotated + object at the exact remote ref, with the intended peeled commit and note. +3. Remove the ordinary success test's assertion of exactly three queries. + Keep its exact remote object/message and unrelated-ref assertions. Retain + the documented single authoritative reconciliation after a failed push. + +Evidence: the new regression fails on the baseline and passes with the push +option. Existing rejected-push, conflict, and unknown-result cases retain their +outcomes. This stage adds no successful-push read-back operation. + +## Stage 2: Establish shared original-object Git observations + +Owners: [internal/git](../../internal/git/source.go), +[candidate](../../internal/candidate/check.go), +[build](../../internal/build/build.go), and +[committed identity](../../internal/releaseidentity/identity.go). +Targets: the mechanics of A02 and A12. + +1. Make ReleaseKit-owned Git subprocesses use `GIT_NO_REPLACE_OBJECTS=1` + through per-command environment overlays. Cover the Git command runner, + local-object reader, batch-reader startup, discovery/preparation commands, + and remaining direct candidate Git commands. Preserve local-only object + reads' lazy-fetch/transport controls and network-capable preparation. + Apply the override to owned Git children; preserve the existing environments + of application Go tests, product checks, and unrelated caller operations. +2. Reuse `git.CaptureHead` and add a small repository-wide worktree-status + observation in `internal/git`. Have candidate state inspection, build + eligibility inspection, and tagged-source inspection use these primitives. + Keep branch and tag observations explicit at their current owners. +3. Remove build's independent `gitCommit` and `cleanSource` execution paths. + Preserve its initial/final observation timing, captured commit, dirty-build + success behavior, `SourceVerified` calculation, and contextual causes. +4. Keep candidate checks requiring clean `main`, and delivery requiring the + selected tag and clean checkout. Shared observation helpers return facts; + they do not impose those caller policies or combine the observations into + a new release-state framework. +5. Leave the unused `InspectLocalTag` helper unused for removal in Stage 17. + Preserve the live candidate guard's exact captured-object/type checks. + +Evidence: existing dirty-source, changed-HEAD, sparse-checkout, nested-module, +Git failure, and source-cancellation cases still express their distinct +contracts. Stage 3 supplies the replacement-object regressions before A02 is +marked complete. + +## Stage 3: Prove validated and published source identity + +Owners: public publication/build/delivery tests and existing Git/tree fixtures. +Target: A02, including the missing regression coverage in testing audit §1. + +1. Use a real candidate and bare authoritative remote at original commit A. + Create a local replacement B with different source and note bytes. Include + a case where A fails compilation while B compiles. Materialize the replaced + checkout using ordinary fixture Git, then invoke the public release path. +2. Assert that a replacement-derived worktree cannot pass candidate validation + and publish A as validated source. Compare actual bytes or the contextual + rejection, rather than only commit IDs. Also verify that a worktree matching + original objects can release normally with unrelated replacement refs. +3. Cover original committed reads for commit, tree, and blob replacements at + the Git/identity/tree boundary. Reuse one public fixture to show this behavior + reaches publication and one existing tagged-delivery fixture to prove the + committed note remains the original note. Avoid a module/layout cross-product. +4. Retain dirty working-tree `Build` support: replacement-derived dirty bytes + may compile, but their result is ineligible as clean binary delivery source. + Keep ordinary partial build facts and inspection causes. + +Evidence: original committed identity/note reads and the publication outcome +agree. Do not alter refs, disable unrelated caller configuration globally, or +add a repository-wide prohibition on replacement refs. + +## Stage 4: Bound buffered subprocess I/O teardown + +Owners: [internal/process/process.go](../../internal/process/process.go), +[native version execution](../../internal/build/build.go), and process tests. +Target: the buffered-command part of A03. + +1. Set an internal `exec.Cmd.WaitDelay` of one second in the common command + construction path. This is an I/O teardown grace after cancellation or + observed child exit, not a runtime deadline for healthy commands. It bounds + drainage of pipes inherited by a surviving descendant while using the + standard library's child waiting and error behavior. +2. Preserve `errors.Is` cancellation/deadline identity when the governing + context is done. Preserve ordinary `*exec.ExitError` and `exec.ErrWaitDelay` + when appropriate. Retain the existing 30-second recovery context; its + cancellation may be followed by bounded teardown, rather than an unbounded + inherited-descriptor wait. Introduce no new public timing SLA or setting. +3. Route build's native version command through `process.Run`, capturing stdout + and stderr together. Preserve its current working-directory behavior, + literal `version` argument, exact expected output, and contextual failures. + Stage 2 has already moved build's Git commands to the shared boundary. +4. Add ready-child/ready-descendant fixtures for cancellation and normal child + exit with inherited descriptors. Check bounded return and error identity; + clean up the fixture's descendants explicitly in test cleanup. Keep a + generous test watchdog separate from the internal teardown grace. + +Evidence: buffered Go/Git preparation can return after cancellation, healthy +long-running commands continue until their caller cancels or they finish, and +the existing source-publication recovery classifications remain unchanged. + +## Stage 5: Complete piped ownership and queued-read cancellation + +Owners: `PipedProcess` in [internal/process](../../internal/process/process.go) +and [BlobReader](../../internal/git/blob_reader.go). Targets: A03 and A18. + +1. Keep serialized batch exchanges and the existing `sync.Once` wait ownership. + After a piped command starts, register an operation-context cancellation + callback that closes its owned stdin/stdout pipes so an exchange or normal + shutdown drain can unblock even if a descendant holds a descriptor. +2. Retain the callback's stop function and a completion signal. During `Wait`, + join the child and stop or join that callback before returning and before + reading captured stderr. On healthy completion, stop the callback before + releasing the derived context. Startup failures close acquired pipes and + release the context. Explicit `Cancel` continues leaving the parent context + unchanged. +3. Retain `BlobReader.Read`'s per-read cancellation hook and its completion + join; it may delegate pipe interruption to the piped-process owner. Keep + aborted sessions terminal, and preserve ordinary missing-object reuse. + Ensure `Close` uses the same cancellation/pipe lifetime and reaps the child. + Healthy exchanges still consume stdout before the normal wait; the one-second + grace does not become a lifetime limit on a healthy batch reader. +4. Recheck `ctx.Err()` immediately after acquiring `reader.mu`, before session + access or callback registration. A canceled queued request returns its own + context error without aborting the healthy session. Retain the mutex; + prompt cancellation while waiting for it remains outside this sprint. +5. Add controlled regressions for inherited-descriptor shutdown, cancellation + during a body read, and a canceled queued request between a successful first + read and a successful later read. Preserve double-close and parent-context + behavior. Use readiness signals and joined cleanup, not timing alone. + +Evidence: process and reader lifecycle tests pass under the race detector. +Use one public preparation cancellation case to confirm caller-visible return; +the test owns any descendant left running after the direct child is reaped. + +## Stage 6: Sanitize Git endpoint diagnostics + +Owners: [internal/git/remote.go](../../internal/git/remote.go), +[tag operations](../../internal/git/tag.go), +[publication](../../internal/publish/publish.go), and the CLI. Target: A04. + +1. Add narrow internal Git URL display helpers. Mask the entire HTTP(S) + userinfo component, covering password- and username-carried credentials. + Retain useful scheme/host/path context. Keep non-HTTP Git forms unchanged. +2. Use the helper for expected/resolved endpoints, endpoint changes, queries, + pushes, and reconciliation messages. Sanitize credential-bearing HTTP(S) + URL spans in captured Git error diagnostics before embedding them. If a + detected userinfo URL cannot be parsed safely for display, replace that span + with a redacted-URL marker. Preserve wrapped causes and error classification. + Where the cause's own text contains credentials, use a private error wrapper + whose `Error` renders sanitized text and whose `Unwrap` retains the cause; + embedding the raw cause with `%w` would expose its text again. + This addresses URL credentials in owned diagnostics, not arbitrary secrets + emitted by application checks or unrelated transport programs. +3. Render the CLI's `endpoint=` field through the same display logic. Keep + `PublicationResult.Endpoint`, captured transport URLs, URL rewrites, and + `ExpectedPushURL` comparisons raw and exact. Clarify the public field's + sensitivity in GoDoc without adding a new public accessor or field. +4. Cover password and username tokens, encoded userinfo, embedded Git diagnostic + URLs, malformed display inputs, and ordinary SSH destinations. A public + mismatch fixture must preserve exact mismatch detection while both the + library error and CLI output hide placeholder credentials. Also exercise + a later query/push failure and preserve the original cause. + +Evidence: owned diagnostics are safe to display, while endpoint identity and +transport behavior retain their selected contracts. Use placeholder credentials +and local fixtures; live Git or forge credentials are unnecessary. + +## Stage 7: Preserve partial local tag evidence + +Owners: [CreateAnnotatedTag](../../internal/git/tag.go), +[publish.Publish](../../internal/publish/publish.go), and publication tests. +Target: A05. + +1. Retain the annotated tag object ID once its existing inspections establish + that evidence. Return it with a later verification error rather than + returning an empty string on every error. +2. Assign any returned tag object to the publication result before handling the + error. Keep phase `tag_creation` and state `not_published` for these failures. +3. After successful tag creation followed by failed inspection, identify + `refs/tags/` in the error and explain that the local tag may remain. + When no ID was obtainable, retain the ref-level evidence and empty object + field. Preserve the original cause and failed verification; perform no + automatic local ref deletion, retagging, independent retry, or push. +4. Add public regressions using real repositories and a command wrapper that + fails the first post-creation type inspection and a later inspection after + the ID is known. Inspect the actual local ref independently and confirm + retained evidence, original error, and absence of a remote push. + +Evidence: the caller can distinguish possible retained local work from a +verified tag object without a new public progress type. Update the relevant +public comment and recovery guidance to reflect unavailable evidence accurately. + +## Stage 8: Normalize internal build results and completed artifacts + +Owners: [internal/build/build.go](../../internal/build/build.go), +[facade conversion](../../build.go), and public build/CLI tests. +Targets: A06 and A14. + +1. Remove internal `Result.MainPackage`, `BinaryName`, and `Targets`; use + normalized `Result.Settings` throughout compilation, native smoke selection, + and conversions. Keep public `BuildResult` fields and receipt fields intact. + Preserve early-failure normalization and result copying. +2. Build and hash each artifact in a local value before appending it to the + completed artifact list. A hash failure names that path, preserves its cause, + retains earlier completed artifacts, and leaves generated files in place. + Later smoke or source-inspection failures still return all already completed + artifacts. Introduce no compiled-but-unverified public result state. +3. Extend the public post-compilation read-failure fixture to include an earlier + successful target. Assert complete digests on the retained prefix, absence + of an incomplete entry, accurate CLI presentation, and preserved source/ + settings facts. Retain no-target, dirty-build, duplicate-target normalization, + partial-target-failure, and version-injection coverage. + +Evidence: public artifacts satisfy the documented digest contract, and prebuilt +delivery still accepts the same normalized public results. + +## Stage 9: Share pure artifact declaration matching + +Owners: [internal/artifact](../../internal/artifact/artifact.go), +[receipt validation](../../cmd/releasekit/internal/cli/receipt.go), and +[delivery declarations](../../internal/delivery/assets.go). Target: A16. + +1. Introduce a pure helper accepting selected targets, expected-name inputs, + and recorded target/filename metadata in artifact-owned internal types. + Reuse `ExpectedBuild` and existing component rules to check counts, target + uniqueness, one artifact per selected target, and expected names. +2. Adapt both callers through explicit conversions. Keep receipt schema, + absolute-path, identity, digest-syntax, and nonempty-target rules in the CLI. + Keep delivery's empty/source-only behavior, file stats, manifest generation, + and byte verification in their current owners and ordering. +3. Preserve current acceptance rules; add no Go target allowlist or content + access during receipt loading. Keep `internal/artifact` independent of build, + CLI, delivery, and forge packages. +4. Retain independent concrete expected filenames in receipt, delivery, and + public receipt-to-delivery tests. Reuse mismatch cases rather than computing + every expected value with the helper under test. + +Evidence: schema and content validation remain distinct, with one owner for +their shared declaration rule and unchanged source-only behavior. + +## Stage 10: Preserve allocation paths and temporary metadata cleanup + +Owners: [internal/tempdir](../../internal/tempdir/tempdir.go), +[tag message files](../../internal/git/tag.go), and +[receipt files](../../cmd/releasekit/internal/cli/receipt.go). Target: A07 part 1. + +1. In both `Outside` allocation branches, keep the original `MkdirTemp` path + until `Abs`/`EvalSymlinks` succeed. Use that original path for error-path + removal. Retain the resolution error and report any failed removal with + its path. Preserve outside-worktree selection and fallback behavior. +2. Capture deferred removal errors for owned tag-message and receipt temporary + files and join them with primary errors. A temporary path absent after a + successful rename is normal; ignore `os.ErrNotExist` for that removal. + Keep successful receipt installation and caller output paths intact. +3. Preserve any verified local tag ID through a later metadata cleanup error, + as established in Stage 7. Cleanup failure before the push remains a local + publication failure; no extra remote action follows it. +4. Use a narrow private, per-operation removal/resolution test seam where real + filesystem failures are otherwise nondeterministic. Keep the seam behind + internal wrappers, with standard functions in production and no mutable + global hooks or public filesystem abstraction. Test path retention and joined + causes, including the renamed-receipt absent-path case. + +Evidence: failed resolution never loses the owned cleanup path, and routine +successful receipt writes do not acquire a spurious cleanup error. + +## Stage 11: Report managed build cleanup with accurate outcomes + +Owners: [candidate build callback](../../internal/build/candidate.go), +[native smoke build](../../internal/build/build.go), +[managed delivery](../../internal/delivery/delivery.go), and CLI delivery output. +Target: A07 part 2. + +1. Use named returns or equivalent explicit finalization to join failed owned + directory removal with a primary build/delivery error. Include the cleanup + path and preserve completed artifacts, source facts, and remote observations. +2. If remote delivery completed, return its `Complete` state and asset lists + together with the cleanup error. The public facade forwards both. The CLI + returns operational status 1, explicitly reports remote completion plus local + cleanup failure, and gives no instruction to redo completed hosted delivery. + Earlier partial/conflict/unknown outcomes retain their original classification. +3. Keep managed temporary output outside the checkout. Caller-owned prebuilt + artifacts and normal caller-selected build directories are not removed by + this finalizer. Candidate build cleanup occurs before tagging as today. +4. Test success-plus-cleanup-failure, primary-error-plus-cleanup-failure, + cancellation, and confirmed hosted completion with cleanup failure. Use a + private per-operation remover parameter at the internal operation boundary + where needed for deterministic failures; keep production entry points using + `os.RemoveAll`. Test the outcome, not only a standalone join helper. + +Evidence: errors identify leaked paths without misreporting remote state or +discarding existing causes. Update delivery/CLI guidance for `Complete` plus +a local operational error and preserve ordinary cleanup success cases. + +## Stage 12: Bound manual asset redirects + +Owners: [internal/gitea/assets.go](../../internal/gitea/assets.go) and adapter/ +delivery tests. Target: A08. + +1. Count followed redirect hops in `openAsset`. Allow ten hops: the initial + request plus at most ten follow-up requests. After ten followed redirects, + a further redirect response is closed and rejected before requesting its + target. A successful response after the tenth hop remains accepted. +2. Keep fragment normalization, exact-cycle handling, invalid-userinfo rejection, + and per-origin token checks. Close every intermediate/rejected body. Return + the existing unverifiable-content failure classification with useful context. + Add no public override or automatic operation timeout. +3. Test zero hops, exactly ten, and an eleventh attempted hop with distinct + URLs. Retain same-origin, cross-origin, return-to-origin, and exact-cycle + checks. Use local servers and request counters to verify the boundary. +4. Preserve shared outcome mapping: read-only verification failure leaves + `NotAttempted`, or `Partial` when this invocation created the page; ambiguity + after an attempted asset write still follows existing reconciliation rules. + +Evidence: the selected hop count is unambiguous and authorization never follows +the token to another origin. Document the limit in the Gitea integration owner. + +## Stage 13: Bound remote content comparison by the declaration + +Owners: [internal/delivery/remote_assets.go](../../internal/delivery/remote_assets.go) +and delivery/artifact fixtures. Target: A09. + +1. Pass expected local size into the digest comparison. Read at most that size + through a limited reader, then probe one additional byte. Keep the extra-byte + fact separate from an `int64` total so `MaxInt64 + 1` never overflows. +2. Distinguish definite size mismatch from inspection failure. Short or excess + content becomes `AssetConflicting`; equal-length content still requires + digest equality. A non-EOF read error, close error, or context error remains + unverifiable with its cause retained. Close the stream on every path. +3. Apply the same verification during upload reconciliation. A mismatch there + conflicts; an unresolved read after a possibly successful write remains + unknown under the existing policy. Keep local hashing before hosted writes, + additive uploads, and asset observation categories unchanged. +4. Test empty, shorter, equal/matching, equal/different, and oversized streams, + plus a read returning data with an error, close failure, and cancellation. + Count bytes in a controlled stream to prove the declaration-derived bound. + Include a public prebuilt-delivery case and retain manifest comparison. + +Evidence: remote metadata size/content length is not used as proof of matching +bytes, and legitimate artifact size has no new global cap. + +## Stage 14: Bound retained status-only Go diagnostics + +Owners: [internal/gotool/gotool.go](../../internal/gotool/gotool.go), its tests, +and [output benchmarks](../../internal/gotool/performance_test.go). Target: A10. + +1. Give combined capture an explicit full-output mode and a status-diagnostics + mode. Full mode retains `Tool.Output` bytes and the existing ownership + transfer. Status mode retains the last 1,048,576 bytes and whether earlier + bytes were omitted. Use a lazy bounded circular tail so repeated small writes + copy in proportion to incoming data rather than shifting a 1 MiB buffer on + every write. Keep synchronization around capture and forwarding. +2. Use status mode for `Tool.Run` and dependency preparation. Preserve complete + forwarding to existing optional writers, short-write/error handling, quiet + behavior without a writer, and the separate preparation Go environment. + Keep configuration-discovery JSON and `Tool.Output` complete. Add no public + output field or diagnostic-retention setting. +3. On failed status commands, include a clear truncation notice only when + needed, followed by the retained tail, while wrapping original process and + forwarding errors. Successful status calls release retained diagnostics. +4. Adapt existing output tests and benchmarks to exercise both modes. Test + below/exact/above budget, a single oversized write, wrapped small writes, + concurrent stdout/stderr, forwarded bytes, and error identity. Benchmark + modest output and 8 MiB/32 KiB-chunk output with allocated bytes reported. + +Evidence: the retained status tail is bounded, full-output consumers keep all +bytes, and forwarded output remains complete. Update the internal Go-output +guide and its benchmark description to the implemented modes. + +## Stage 15: Separate validation sequence and tree lifetime + +Owners: [internal/validation/validation.go](../../internal/validation/validation.go) +and validation lifecycle/diagnostic tests. Target: A13. + +1. Extract private Go-command and committed-tree execution functions. Keep the + visible order: tests, selected full vet, selected compilation, forbidden + files, formatting, workspace files, local replacements, file size, Markdown. + Shared-file selection alone still validates explicit file presence. +2. Let the committed-tree sequence own its close and the last content-reading + check. Preserve tree-load failure precedence, first check failure, and the + rule that reader-close failure replaces success only after a content read. + Keep context and underlying diagnostic causes through the public facade. +3. Preserve nil versus empty shared files and optional check selections, + operation-local Go settings reuse, and single execution of additional checks + and build callbacks by their existing owners. Use private functions and + explicit check blocks; retain current package and public interfaces. + +Evidence: existing combined-check order, scope, Markdown, and lifecycle tests +remain meaningful. Add a focused case only if existing tests leave one of the +preserved precedence or metadata-only close behaviors unprotected. + +## Stage 16: Extract private CLI workflows + +Owners: [cmd/releasekit/main.go](../../cmd/releasekit/main.go) and CLI tests. +Target: A15. + +1. Extract named private handlers for check/publication and for build. Keep + argument parsing, containing-repository resolution, version dispatch, and + command routing in `run`. Existing delivery handlers remain the local model. +2. Preserve profile loading, captured-candidate environment, expected module + and endpoint mapping, receipt invalidation before build-error reporting, + receipt verification, partial-artifact output, and all exit classifications. + Carry Stage 6's safe endpoint display and Stage 11's cleanup reporting into + the handlers without adding command flags or public interfaces. +3. Keep the CLI calling the public facade for release operations. Existing + internal path/configuration helpers retain their current role. Reuse the + command tests for profile errors, successful operations, known failures, + and unknown publication/delivery outcomes. + +Evidence: the dispatcher is small while command behavior and failure evidence +match the selected contracts. No new CLI framework is needed. + +## Stage 17: Remove incidental assertions and unused helpers + +Owners: Markdown and Linux delivery-read tests, public publication tests, +[release identity](../../internal/releaseidentity/identity.go), and +[local tag helpers](../../internal/git/tag.go). Targets: A19 and A20. + +1. Remove Markdown cache pointer-identity assertions; retain unique parsing + checks and identical-blob source-relative link behavior. Existing behavioral + tests remain the protection for correctness. +2. Remove `TestDeliverReadsLocalBinaryOnceForVerificationAndOnceForUpload` + as an exact inotify-open-count contract. Retain independent tampering, + pre-write verification, and uploaded-byte checks. Use the existing delivery + benchmark to observe read/verification cost; no replacement exact-open + assertion is required. Stage 1 handled successful-path query counts. +3. Remove only the unused `FromCommittedSelection` wrapper, keeping the private + implementation and live `FromCommitted`/`ResolveCommittedSelection` paths. +4. Remove unused `InspectLocalTag` and its direct test call, preserving the + test's other full-tag/source assertions and the live candidate guard. + Retain tree accessors and constructors used meaningfully by tests/benchmarks. + +Evidence: source references show no remaining use of removed symbols; caller +behavior and public exports remain covered. Avoid tests that merely mirror +the deleted helpers or assert a new incidental call count. + +## Stage 18: Record the deferred resource assessments + +Targets: A11 and A17. Deliver evidence in their status notes here; accepted +production behavior remains as defined in [audit.md](audit.md). + +1. For A11, inventory local repository/release/attachment JSON fixtures and + checked-in release-note sizes. Record measured byte sizes, the kinds of + responses represented, and missing evidence about deployment/protocol maxima. + Local fixtures establish observed sizes, not a supported maximum. Record + that JSON size/time caps and new configuration remain deferred; preserve + current cancellation and ambiguous-write handling. Live service queries + or an external sizing decision are not prerequisites to finish this sprint. +2. For A17, add `BenchmarkFileSizeExclusions` in a validation benchmark file. + Compare the current scan with a benchmark-only normalized set/ancestor + prototype for 1,000 and 10,000 paths; 1, 10, 100, and 1,000 exclusions; and + representative shallow/deep paths with early matches and nonmatches. + Count one index construction per simulated operation, report allocations, + and prepare unrelated fixture strings outside timing. Preserve literal + semantics in the comparison; do not add the prototype to production code. +3. Run the benchmark with a bounded measurement interval, for example: + + ```sh + GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test ./internal/validation \ + -run '^$' -bench '^BenchmarkFileSizeExclusions$' -benchtime=100ms -count=3 + ``` + +4. Record toolchain/platform, command, timings/allocations, and representative + common/large-list tradeoffs. Keep production exclusion matching unchanged + in this sprint; the assessment supplies evidence for a later adoption + decision. Mark both items as assessment completed / production change + deferred, rather than claiming either resource policy is implemented. + +Evidence: both deferred items have a concrete disposition and useful evidence +without inventing unapproved protocol limits or performance thresholds. + +## Stage 19: Verify the complete target and update current documentation + +1. Inspect the combined implementation against all A01–A20 outcomes and the + stage table. Confirm the fixes interact correctly: original-object reads + use the shared canceled-process boundary; partial tag evidence survives + cleanup errors; completed artifacts feed shared declarations; content limits + preserve preflight/write order and reconciliation outcomes; CLI output + presents causes and reached state accurately. +2. Complete canonical documentation updates in GoDoc, `docs/cli.md`, consumer + publication/build/delivery guides, `docs/integrations/gitea.md`, and relevant + internal guides. Document the accepted redirect and diagnostic policies, + raw endpoint sensitivity, and completed hosted state with cleanup error. + Keep implementation mechanics out of user flows. Preserve public facade + shape, receipt schema, source-only release behavior, and the Go baseline. +3. Format changed Go files and run from the repository root: + + ```sh + GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test ./... + GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test -race ./... + GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go run ./internal/apicheck + ``` + + Run focused regressions first as needed while completing their owners. + Reuse successful checks for unchanged content. Full-suite green results + complement, rather than replace, the targeted failure evidence above. +4. Check local documentation links/fragments and Markdown whitespace. Review + the diff for accidental public exports, stronger target/path restrictions, + new global deadlines, secret values, unrelated changes, and altered caller + ownership. Resolve failures within the selected scope before finishing. +5. Update this plan's stage statuses and record final verification. Keep A11 + caps and A17 indexing explicitly deferred, with Stage 18 evidence. Retain + the source audit reports for review and traceability. Commit completed + nonempty work with plain English messages and report the result, remaining + deferrals, and any actual limitations. Release preparation remains governed + by the maintainer runbook. + +Completion means the selected repairs and maintenance are implemented, their +behavioral evidence passes, current contracts are documented, and the two +accepted deferrals have assessment records. Intermediate stages introduce no +extra compilation, review, or deployment gates.