From 6fab30337a03cf93bfa2a803fbd09e4d0dfb7782 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 2 Nov 2026 13:54:16 +0000 Subject: [PATCH] Prepare v0.7.0 notes and retire completed roadmaps --- docs/releases/v0.7.0.md | 106 ++++ docs/roadmap/audit.md | 407 --------------- docs/roadmap/audit/01-correctness.md | 191 ------- docs/roadmap/audit/02-efficiency.md | 35 -- docs/roadmap/audit/03-complexity.md | 85 ---- docs/roadmap/audit/04-concurrency.md | 169 ------ docs/roadmap/audit/05-resources.md | 101 ---- docs/roadmap/audit/06-error-handling.md | 54 -- docs/roadmap/audit/07-security.md | 200 -------- docs/roadmap/audit/08-testing.md | 98 ---- docs/roadmap/audit/09-interfaces.md | 51 -- docs/roadmap/audit/10-equivalence.md | 84 --- docs/roadmap/audit/11-deadcode.md | 59 --- docs/roadmap/deferred-resources.md | 67 +++ docs/roadmap/implementation.md | 648 ------------------------ docs/roadmap/remediation.md | 231 --------- 16 files changed, 173 insertions(+), 2413 deletions(-) create mode 100644 docs/releases/v0.7.0.md delete mode 100644 docs/roadmap/audit.md delete mode 100644 docs/roadmap/audit/01-correctness.md delete mode 100644 docs/roadmap/audit/02-efficiency.md delete mode 100644 docs/roadmap/audit/03-complexity.md delete mode 100644 docs/roadmap/audit/04-concurrency.md delete mode 100644 docs/roadmap/audit/05-resources.md delete mode 100644 docs/roadmap/audit/06-error-handling.md delete mode 100644 docs/roadmap/audit/07-security.md delete mode 100644 docs/roadmap/audit/08-testing.md delete mode 100644 docs/roadmap/audit/09-interfaces.md delete mode 100644 docs/roadmap/audit/10-equivalence.md delete mode 100644 docs/roadmap/audit/11-deadcode.md create mode 100644 docs/roadmap/deferred-resources.md delete mode 100644 docs/roadmap/implementation.md delete mode 100644 docs/roadmap/remediation.md diff --git a/docs/releases/v0.7.0.md b/docs/releases/v0.7.0.md new file mode 100644 index 0000000..bf55b2c --- /dev/null +++ b/docs/releases/v0.7.0.md @@ -0,0 +1,106 @@ +# ReleaseKit v0.7.0 + +## Summary + +ReleaseKit improves source-publication integrity, cancellation, credential-safe +diagnostics, partial-result reporting, and temporary-file cleanup. It also bounds +retained Go failure diagnostics and remote attachment comparison work while +preserving the existing public facades and configuration formats. + +## Publication and Git operations + +- Exact tag pushes disable submodule recursion, correcting inherited + `push.recurseSubmodules=only` settings that could report success without + publishing the release tag. +- ReleaseKit-owned Git inspection and preparation use original objects when + replacement refs exist. Committed module identity, release notes, validation + inputs, and build source eligibility use that consistent view. Ordinary dirty + builds remain supported, with their source eligibility reported separately. +- A verified local tag object remains available in `PublicationResult.TagObject` + when later inspection fails. Errors identify a possibly retained local ref + when its object ID could not be established. Failed verification stops before + a push. +- HTTP(S) userinfo is masked in owned Git diagnostics and CLI output, including + literal apostrophes in credentials and failed CI checkout preparation. Native + Git authentication, exact endpoint comparisons, and the raw captured + `PublicationResult.Endpoint` value retain their existing behavior. +- Child environment overlays preserve the working-directory-derived `PWD` on + POSIX systems. Native Git hooks now receive the expected directory environment + when ReleaseKit is invoked from another checkout or module subdirectory. + +## Cancellation and diagnostic retention + +- Buffered subprocess output has bounded I/O teardown after cancellation or + child exit, including when descendants retain inherited output descriptors. + Piped Git-reader cancellation closes owned pipes and joins child cleanup. + Cancellation remains recognizable through `errors.Is`; ordinary execution + failures preserve their underlying causes. +- A queued blob read rechecks cancellation after acquiring the reader mutex, + preserving a healthy batch session for later reads. +- Status-only Go commands and dependency preparation retain a 1 MiB diagnostic + tail. Failed commands identify omitted earlier output. Existing streaming + output continues to be forwarded in full, and commands requiring returned + output keep their complete capture. + +## Builds and hosted delivery + +- Completed build artifacts are reported only after hashing succeeds. A hashing + failure identifies the affected output and preserves earlier completed + artifacts and their digests. +- Temporary allocation and cleanup failures retain their paths and underlying + causes, including candidate builds, native version checks, delivery builds, + tag-message files, and build receipts. +- A managed-build cleanup failure after confirmed hosted delivery returns an + error alongside `DeliveryComplete` and the confirmed asset observations. The + CLI reports completed remote work and the local cleanup failure, returning + operational status 1. +- Gitea attachment downloads follow at most ten redirects, retaining cycle + detection and per-origin authorization handling. Limit exhaustion is + unverifiable content. +- Remote attachment comparison reads at most the locally verified expected size + plus one byte. Shorter or longer content conflicts; equal-length content still + requires the expected digest. Read, close, and cancellation errors remain + inspection failures, with existing attempted-write reconciliation preserved. + +Shared Git observations, validation sequencing, artifact declaration matching, +and CLI workflows were consolidated internally. Consumer-facing types continue +to belong to the root `releasekit` and runtime `buildinfo` facades. + +## Compatibility and upgrade guidance + +The module identifier remains `go.maximumdirect.net/releasekit`, and the Go +consumer minimum remains 1.26.0. Existing v0.6.0 API calls, profile fields, and +build receipt formats remain supported. This minor release changes operational +behavior through the diagnostic and redirect budgets and cleanup-error reporting +described above. + +When adopting v0.7.0: + +1. Update library and CLI dependencies to v0.7.0 using the existing import paths + and profiles. +2. Handle a delivery error together with `DeliveryResult.State`. For + `DeliveryComplete`, address the reported local cleanup path; the confirmed + hosted delivery is already complete. Continue to resolve partial, conflicting, + and unknown outcomes through their existing recovery guidance. +3. Retain candidate validation's streamed `Output` when complete test diagnostics + are needed. Status-only failure text now contains the retained tail and an + omission notice when applicable. +4. When using Git replacement refs, expect release evidence to be evaluated + against original committed objects. A build of dirty or replacement-derived + working content can succeed while remaining ineligible for binary delivery. +5. Inspect the reported local tag ref after a tag-creation or verification + failure, including when `TagObject` is empty. Continue to treat the raw + `PublicationResult.Endpoint` as sensitive when handling it in application + output or logs. + +See the [publication guide](https://forge.maximumdirect.net/base-layer/releasekit/src/tag/v0.7.0/docs/consumers/publish.md), +[build guide](https://forge.maximumdirect.net/base-layer/releasekit/src/tag/v0.7.0/docs/consumers/build.md), +[delivery guide](https://forge.maximumdirect.net/base-layer/releasekit/src/tag/v0.7.0/docs/consumers/delivery.md), +[CLI contract](https://forge.maximumdirect.net/base-layer/releasekit/src/tag/v0.7.0/docs/cli.md), +and [Gitea integration guide](https://forge.maximumdirect.net/base-layer/releasekit/src/tag/v0.7.0/docs/integrations/gitea.md) +for current behavior. Source publication and hosted delivery retain their +separate outcomes under the [maintainer procedure](https://forge.maximumdirect.net/base-layer/releasekit/src/tag/v0.7.0/docs/release.md). + +[COPYRIGHT](https://forge.maximumdirect.net/base-layer/releasekit/src/tag/v0.7.0/COPYRIGHT) | +[LICENSE](https://forge.maximumdirect.net/base-layer/releasekit/src/tag/v0.7.0/LICENSE) | +[ATTRIBUTION](https://forge.maximumdirect.net/base-layer/releasekit/src/tag/v0.7.0/docs/oss/attribution.md) diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md deleted file mode 100644 index e44185a..0000000 --- a/docs/roadmap/audit.md +++ /dev/null @@ -1,407 +0,0 @@ -# Audit remediation target - -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). - -## Accepted scope and priorities - -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 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. - -| ID | Priority | Target outcome | -| --- | --- | --- | -| 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. | - -## 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). - -**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. - -**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). - -**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. - -**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 - -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). - -**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. - -**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). - -**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. - -**Rationale:** The observed mismatch and failure paths can expose configured -credentials to readers of logs or shared diagnostics. - -**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). - -**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. - -**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). - -**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. - -**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). - -**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. - -**Rationale:** Discarded errors and lost allocation paths can leave storage -behind without actionable recovery information. - -**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. - -**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. - -**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. - -**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. - -**Rationale:** Once size mismatch is established, further download and hashing -cannot produce a matching asset and can consume unbounded resources. - -**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). - -**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. - -**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). - -**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. - -**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). - -**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. - -**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). - -**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. - -**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). - -**Target:** Internal build results have one normalized `Settings` -representation. Public `BuildResult` and private receipt fields keep their -existing contracts and explicit boundary conversions. - -**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). - -**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. - -**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). - -**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. - -**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). - -**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. - -**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). - -**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. - -**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). - -**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. - -**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). - -**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. - -**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 - -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 | diff --git a/docs/roadmap/audit/01-correctness.md b/docs/roadmap/audit/01-correctness.md deleted file mode 100644 index 1a15b8f..0000000 --- a/docs/roadmap/audit/01-correctness.md +++ /dev/null @@ -1,191 +0,0 @@ -# Functional correctness audit - -Reviewed baseline: `dd0a557` on `main`, using Go 1.26.10 and Git 2.53.0 on -Linux. Findings describe the implementation at that commit; remediation remains -pending. - -## Scope and evidence - -The review covered the root orchestration facade, runtime version facade, CLI -and profile/receipt conversions, candidate capture and validation, module and -tag identity, Git publication and recovery, build/version verification, local -artifact declarations, shared hosted delivery, Gitea adaptation, preparation, -process execution, and maintained release/CI examples. Contracts were checked -against GoDoc, consumer and integration guides, architectural decisions, and -existing tests. - -The following existing checks passed, with Go's test cache used where applicable: - -```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 -``` - -Temporary programs and repositories outside this checkout substantiated the -findings below. Publication probes used temporary local bare remotes; -cancellation probes used local processes and a simulated SSH executable. -Production code and repository tests remain unchanged. Live forge behavior was -outside this audit's verification scope. - -## 1. High: publication can report success while leaving the tag unpublished - -**Relevant code:** [Endpoint.PushTag](../../../internal/git/tag.go), particularly -the push invocation at line 139, and the success transition in -[publish.Publish](../../../internal/publish/publish.go), lines 167–174. - -**Circumstances:** Git's effective configuration contains -`push.recurseSubmodules=only`. This can be a repository or inherited user -setting. A repository without submodules is sufficient to trigger the defect. - -`PushTag` specifies an exact object-to-tag refspec and disables automatic -following of other tags, but inherits submodule push mode. Git's `only` mode -leaves the superproject unpushed and can exit zero. `Publish` treats that exit -as confirmed publication and immediately returns `published`/`complete`. -Reconciliation runs only after an error, so it never observes the missing tag. -Git documents this mode in its [push reference](https://git-scm.com/docs/git-push). - -**Expected behavior:** Under the existing -[source publication contract](../../consumers/publish.md), a successful result -confirms publication of the captured annotated tag at the selected endpoint. - -**Actual behavior and reproduction:** A temporary clean root-module candidate -on `main` contained a valid versioned release note, and its bare `origin` had -the same `main` commit. After setting `push.recurseSubmodules=only`, a public -`releasekit.Publish` call for `v1.0.0` returned: - -```text -state=published phase=complete tag=v1.0.0 error= -``` - -An independent `git ls-remote origin refs/tags/v1.0.0` returned no records. The -annotated tag existed locally. The corresponding exact-object Git push exited -zero and printed only `Done`. - -**Potential remediation:** Set `--recurse-submodules=no` explicitly on the -ReleaseKit-owned tag push, preserving its existing single-tag scope. A focused -public publication regression can configure `only`, invoke `Publish`, and -verify that the remote receives the exact captured tag object. Validate the -successful porcelain response if further evidence is needed; an additional -remote query on every successful push is a larger alternative to fixing this -specific inherited setting. - -## 2. High: Git replacement objects can separate validated content from published source - -**Relevant code:** candidate capture and local-state inspection in -[internal/candidate/check.go](../../../internal/candidate/check.go), committed -file/tree reads in [internal/git/source.go](../../../internal/git/source.go), -batched blob reads in [internal/git/blob_reader.go](../../../internal/git/blob_reader.go), -and tag creation/publication in [internal/git/tag.go](../../../internal/git/tag.go). -`ReadCommitFile` at line 255 reads through `git show`; `readLocalObjects` at -line 408 controls lazy fetching and transports but retains replacement-object -interpretation. - -**Circumstances:** A local replacement ref maps the captured commit, tree, or -blob to different content. The working tree and index reflect that replacement -view. The authoritative remote has the original candidate commit and lacks -the local replacement mapping. - -Git normally applies replacement refs during local object inspection, while -pack transfer uses the original objects. See the official -[replacement-object documentation](https://git-scm.com/docs/git-replace#_description). -ReleaseKit captures the original object ID, checks cleanliness and reads -committed content using the replacement view, and runs Go validation against -that working tree. It then creates and pushes a tag identifying the original -commit. Exact commit-ID and tag-object checks do not detect the content split. - -**Expected behavior:** The selected module declaration, release note, -validated source, and published commit describe the same candidate, as required -by the [publication contract](../../consumers/publish.md). An affected local -replacement must be handled consistently before publication can succeed. - -**Actual behavior and reproduction:** A temporary repository had original -candidate commit A on both local and remote `main`. A separate parentless -commit B contained different source and a different note with the same valid -product/version heading. After `git replace A B` and `git reset --hard HEAD`, -ordinary Git status was clean. A public `releasekit.Publish` call succeeded: - -- Go validation inspected the replacement working tree. -- The annotated tag's message contained `Replacement note.`. -- Reading `v1.0.0:docs/releases/v1.0.0.md` directly from the bare remote returned - `Original note.`. -- The published tag identified A, whose source differed from the validated - working tree. - -A second probe made A fail compilation with an undefined identifier and made B -compile successfully. `Publish` still returned `published` with no error, and -the bare remote retained A's unbuildable source. Running Git status with -replacement interpretation disabled exposed staged source and note differences -that the ordinary status check had hidden. - -The same committed-object readers feed delivery and optional file checks, so -their handling should be reviewed alongside publication. - -**Potential remediation:** Use an original-object Git view consistently for -ReleaseKit-owned candidate/object inspection and source-state comparisons, -for example through `GIT_NO_REPLACE_OBJECTS=1` or `--no-replace-objects` on -the relevant commands. Comparing the working tree against that view should -identify replacement-derived differences before validation/publication. Cover -both ordinary Git commands and the batched blob reader, plus the direct Git -inspection used by builds. Another viable approach is detecting replacements -that affect the selected source and returning a contextual error. Scope that -check to the identity/content mismatch; unrelated replacement refs do not -independently justify a repository-wide prohibition. Regression coverage should -verify original committed bytes and the published remote source, rather than -commit IDs alone. - -## 3. Medium: inherited process pipes can defeat cancellation and recovery deadlines - -**Relevant code:** [process.Run and newCommand](../../../internal/process/process.go), -lines 30–45 and 132–141; buffered Git execution in -[internal/git/remote.go](../../../internal/git/remote.go); and the bounded -reconciliation context in [publish.reconcilePush](../../../internal/publish/publish.go), -lines 177–181. Direct `exec.CommandContext` calls in -[internal/build/build.go](../../../internal/build/build.go) also merit the same -review. - -**Circumstances:** A command launches a descendant that retains an inherited -stdout or stderr pipe, and ReleaseKit captures that stream through an -`io.Writer`. Git invoking an SSH process is a concrete example. Cancellation -kills the immediate command, while the descendant can keep the pipe open. - -`newCommand` uses the default `exec.CommandContext` cancellation and leaves -`Cmd.WaitDelay` at zero. `Run` consequently waits for its output-copy goroutines -to observe EOF before checking and returning the context error. An inherited -descriptor can delay EOF indefinitely. The Go standard library describes this -behavior and its existing mitigation in -[Cmd.WaitDelay](https://pkg.go.dev/os/exec#Cmd). - -**Expected behavior:** Canceled preparation and inspection operations return -their cancellation outcome, and the documented bounded authoritative -reconciliation can finish with an unresolved outcome when its deadline expires. - -**Actual behavior and reproduction:** A temporary program called the public -`PrepareTaggedCheckout` API with a 500 ms deadline and a local simulated SSH -transport that launched `sleep 3` while retaining its standard streams. The -SSH executable was reached; the API returned after approximately 3.01 seconds, -preserving `context.DeadlineExceeded` only after the descendant exited. A -separate direct `process.Run` probe with buffered output, a 200 ms deadline, -and `sh -c 'sleep 3 & wait'` similarly took approximately 3.01 seconds. A -descendant that keeps the descriptor open indefinitely leaves the operation -waiting indefinitely. The reconciliation consequence follows from its use of -the same Git/process boundary; a stalled reconciliation was not separately -run for its full 30-second timeout. - -**Potential remediation:** Give captured command I/O a bounded shutdown path -after cancellation, using standard `exec.Cmd.WaitDelay` where applicable, and -preserve the existing context-error identity. Review explicitly owned pipes -and direct build subprocesses so their cleanup does not wait for EOF before -the cancellation path can close the pipe. Add controlled regression coverage -with a ready descendant retaining a descriptor; ordinary single-process -cancellation tests do not exercise this case. Keep the change focused on -returning cancellation and preserving the existing recovery bound; a general -descendant-process supervision contract would require a separate decision. - -## Recommended order - -Address findings 1 and 2 before the next release because they can report a -successful release without the required tag or validated source. Then address -finding 3 at the shared process boundary and verify its Git and build callers. -The existing suite and facade-boundary checks remain useful, with focused -regressions needed for these uncovered cases. diff --git a/docs/roadmap/audit/02-efficiency.md b/docs/roadmap/audit/02-efficiency.md deleted file mode 100644 index 6fd8fd0..0000000 --- a/docs/roadmap/audit/02-efficiency.md +++ /dev/null @@ -1,35 +0,0 @@ -# Efficiency audit - -Reviewed baseline: `812c24d` on `main`. This review examined the Go facade, -CLI, candidate validation, committed-tree and path inspection, build, -publication, and hosted-delivery paths for correctness and avoidable scaling -costs. Complexity estimates below describe the code, not measured timings. -Previously recorded correctness and security findings remain in their -respective audit documents. - -## File-size exclusions are scanned for every selected file - -**Relevant code:** [checkFileSize and excludedByPath](../../../internal/validation/file_size.go). -The public [file-size settings](../../../check.go) accept literal file and -directory exclusions. - -For each selected blob, `checkFileSize` calls `excludedByPath`, which scans -the normalized exclusions until it finds a match. With N selected blobs and E -exclusions, inputs with no matches require N × E path comparisons; when N and -E grow together, this is quadratic, before Git is asked for any blob sizes. -String comparison also depends on path length. This is a potential scaling -cost for consumers with large repositories and extensive exclusion lists; the -review did not measure its practical impact. - -**Potential refactoring:** Index normalized exclusions once in a hash set, -then test each file path and its directory ancestors, including the special -`.` exclusion. This takes expected O(E) indexing plus O(N × D) lookups for -maximum path depth D, apart from string-processing costs, instead of scanning -all E exclusions for each file. A component trie is another option if path -processing warrants it. Preserve literal component-boundary matching, -module-relative normalization, the order of selected files, and the first -oversized-file result. Existing exclusion cases cover the semantics; add -targeted cases for shared prefixes and nested directories if refactored. - -The [security audit](07-security.md#3-low-remote-content-verification-reads-beyond-the-known-matching-size) -separately records avoidable remote asset reads beyond the expected size. diff --git a/docs/roadmap/audit/03-complexity.md b/docs/roadmap/audit/03-complexity.md deleted file mode 100644 index 67308e7..0000000 --- a/docs/roadmap/audit/03-complexity.md +++ /dev/null @@ -1,85 +0,0 @@ -# Complexity audit - -Reviewed baseline: `620c832` on `main`. The review used the local codebase -knowledge graph's cognitive and cyclomatic complexity estimates to identify -candidates, then inspected their code, callers, package boundaries, and -documented contracts. Scores are heuristics, not defect or refactoring -thresholds. No empirical performance measurements or application tests were -run for this documentation-only review. Existing correctness, concurrency, -security, and efficiency findings are recorded in the neighboring audits. - -## 1. Shared validation combines several kinds of orchestration - -[validation.Run](../../../internal/validation/validation.go) has a graph -cognitive score of 78 and cyclomatic score of 37. It validates settings, -acquires the Go tool, runs tests, vet and compilation, decides whether to -load a committed tree, chooses an error identity for a failed tree load, runs -file checks, and attributes a deferred reader-close error. The tree-load -condition, the reverse-priority error selection, and the ordered check blocks -each separately enumerate selections. Adding a check therefore requires a -maintainer to update several distant parts while preserving ordering and -close-error attribution. - -Consider separating the Go-command sequence and the tree-check sequence into -small private functions, with the tree owner responsible for closure and its -failure attribution. Keep the selected checks and their order visible in one -place; a generic plugin system is unnecessary. Preserve nil versus empty shared -file selections, the current first-failure identities and precedence, and -the fact that a close failure replaces success only after a content read. - -## 2. Git source inspection is repeated across package boundaries - -[build.BuildResolved](../../../internal/build/build.go) uses its own -`gitCommit` and `cleanSource` subprocesses before and after building. -[candidate](../../../internal/candidate/check.go) separately reads branch, -commit and status, while [git](../../../internal/git/source.go) has -`CaptureHead` and another tagged-source inspection path. These overlap in -commands and interpretation but use different execution and error handling. -Understanding a source-state change or a subprocess fix requires tracing all -three owners. The existing [correctness audit](01-correctness.md) identifies -replacement-object and cancellation concerns that make this duplication -particularly relevant. - -Consider moving the common commit and worktree-status primitives behind the -existing internal Git boundary, with callers composing only the observations -they need. Candidate checking still needs branch and tag rules; building still -records dirty source rather than rejecting it; delivery still checks a tag. -Preserve these different contracts, error context, cancellation behavior, and -the timing of before/after observations. Coordinate any such refactor with the -existing correctness work instead of treating code consolidation as its fix. - -## 3. Internal build results retain duplicate settings - -[internal/build.Result](../../../internal/build/build.go) stores normalized -`Settings` alongside separate `MainPackage`, `BinaryName`, and `Targets` -fields containing the same information. `BuildResolved` initializes both, -and [facade conversion](../../../build.go) reconstructs both on input while -reading `Settings` on output. Delivery uses `Settings` for identity and asset -validation. This creates two sources of truth inside the implementation and -additional work when a build setting is added; a future caller could populate -them inconsistently. - -Consider retaining `Settings` as the single internal representation and -updating internal build execution to use it. Keep the existing public -`BuildResult` fields and the CLI's private receipt format at their respective -boundaries; their distinct types and conversions serve deliberate ownership -and serialization requirements. Verify partial build results and prebuilt -delivery remain unchanged. This is a simplification opportunity, not an -observed mismatch in current callers. - -## 4. CLI command handling interleaves routing and command-specific workflows - -[run](../../../cmd/releasekit/main.go) has a graph cognitive score of 85 and -combines routing with profile loading, publication and check reporting, build -execution, receipt invalidation and verification, and exit-code decisions. -The same file also contains delivery preparation and receipt conversions. -The build branch alone has several separate failure paths that must be read -alongside its success and partial-result output; changes to one command add -noise to the central dispatcher. - -Consider keeping `run` as a small dispatcher and moving the check/publish and -build command workflows into named private handlers, grouped by responsibility. -Keep receipt invalidation before build-error reporting, existing messages and -exit statuses, and the CLI's use of the public orchestration facade. Existing -delivery handlers provide a local example of this separation. This does not -require a new command framework or public interfaces. diff --git a/docs/roadmap/audit/04-concurrency.md b/docs/roadmap/audit/04-concurrency.md deleted file mode 100644 index 94b21bd..0000000 --- a/docs/roadmap/audit/04-concurrency.md +++ /dev/null @@ -1,169 +0,0 @@ -# Concurrency audit - -Reviewed baseline: `d884ad7` on `main`, using Go 1.26.10 and Git 2.53.0 on -Linux. The findings below remain pending. Finding 1 overlaps an existing -correctness finding; finding 2 is an additional internal-helper weakness. - -## Architecture and scope - -The root facade coordinates candidate checking, publication, builds, preparation, -and hosted delivery. The CLI supplies profiles, credentials, output, and process -signals. Internal packages own the operation state and external effects; -`internal/forge` separates shared delivery policy from the Gitea adapter. -The `buildinfo` facade has a small, independent runtime implementation. - -Most orchestration is sequential. Go settings owners, candidate-tree caches, -delivery result slices, and backend instances belong to individual operations. -The principal concurrency mechanisms are: - -- `internal/process` uses context-driven subprocess cancellation, standard - library I/O goroutines, and `sync.Once` for piped-process waiting. -- `internal/git.BlobReader` serializes batch exchanges with a mutex and joins - each read's cancellation callback before releasing the exchange. -- `internal/gotool` serializes combined stdout/stderr capture and forwarding - with a mutex. Captured bytes are consumed after process completion. -- `internal/gitea` streams multipart uploads through an `io.Pipe` and a worker - goroutine, joining that worker before returning the borrowed content reader. -- `internal/runtimeinfo` caches fallback version discovery with - `sync.OnceValue`; the injected version is initialized at link time. - -The review examined those ownership and shutdown paths, callback sequencing, -shared caches and output, build cleanup, publication guards/reconciliation, -delivery recovery, and signal propagation. Operation-local helpers such as the -Go settings owner and candidate tree are used sequentially by current callers; -their mutable maps alone do not establish a concurrency defect. - -## Verification - -The repeated, uncached race checks passed: - -```sh -GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test -race -count=5 \ - ./internal/process ./internal/git ./internal/gitea ./internal/runtimeinfo -``` - -The preceding correctness audit also passed the full ordinary and race-enabled -suites and compiled API/dependency checks. Production code and tests are -unchanged between those checks and this review's baseline. - -Temporary race-enabled programs outside this checkout reproduced the two -findings below without race-detector diagnostics. Controlled Git substitutes -provided readiness and release signals; short-lived descendants exercised -inherited descriptors. Local HTTP/1.1 and HTTP/2 probes of early upload -rejection returned promptly and produced no additional finding. These probes -used local fixtures and placeholder credentials. - -No data race or unjoined multipart-worker leak was identified in the reviewed -current orchestration paths. Race detection complements the liveness evidence; -a clean race run does not detect a blocked operation. - -## 1. Medium: inherited descriptors can keep canceled operations and reader shutdown blocked - -**Relevant code:** [process.Run, newCommand, and PipedProcess.Wait](../../../internal/process/process.go), -buffered Git execution in [internal/git/remote.go](../../../internal/git/remote.go), -and [BlobReader.Close](../../../internal/git/blob_reader.go), especially the -stdout drain at line 110 followed by the child wait. The owning cleanup runs -from [shared validation](../../../internal/validation/validation.go). - -**Circumstances and mechanism:** A subprocess launches a descendant that -inherits stdout or stderr. Cancellation kills the immediate process, while -the descendant retains the descriptor. Buffered command execution waits for -its standard library I/O workers to receive EOF. The batched reader's normal -shutdown closes stdin, drains stdout to EOF, and then waits for the child. -Both paths can remain blocked after the governing context is canceled. - -This is the same underlying issue recorded as finding 3 in the -[correctness audit](01-correctness.md#3-medium-inherited-process-pipes-can-defeat-cancellation-and-recovery-deadlines). -That audit reproduced delayed cancellation through public -`PrepareTaggedCheckout` using a simulated SSH transport. This concurrency pass -additionally exercised the explicitly owned batch-reader shutdown path. - -**Additional reproduction:** A controlled `git cat-file --batch` substitute -returned a valid three-byte blob. After stdin reached EOF, it launched -`sleep 3` with inherited descriptors and exited. The reader's operation context -had a 500 ms deadline. The initial read succeeded, but `BlobReader.Close` -returned only after approximately 3.005 seconds, with the deadline error then -preserved. The probe ran under the race detector and completed after its -descendant exited. A descendant retaining the descriptors indefinitely can -keep this shutdown waiting indefinitely. - -**Consequences:** Cancellation can leave validation or preparation occupied -beyond its deadline, delay temporary-output cleanup, and prevent publication -reconciliation from returning its documented bounded unresolved outcome. -`BlobReader.Close` holds the reader mutex during its drain, so other readers -and repeated closes also wait for that shutdown to finish. - -**Potential remediation:** Give subprocess I/O a bounded cancellation/shutdown -path. Use the standard `exec.Cmd.WaitDelay` mechanism where applicable, and -verify that explicitly owned stdin/stdout pipes also close when their operation -is canceled. Coordinate cancellation callbacks, pipe closure, and `Wait` so -the child and workers are joined before the owning operation returns. Apply -the same review to direct subprocess calls in -[internal/build](../../../internal/build/build.go). Preserve existing context -error identity and publication uncertainty semantics. - -Regression coverage should include a ready descendant retaining a descriptor -during both buffered command execution and batch-reader cleanup. The existing -single-process cancellation tests pass. This finding calls for completing the -selected cancellation behavior; it does not require a general service for -supervising every descendant process. Treat this and correctness finding 3 as -one remediation unit. - -## 2. Low: a canceled queued blob read can later abort a healthy session - -**Relevant code:** [BlobReader.Read](../../../internal/git/blob_reader.go), -lines 44–88. It checks the request context before acquiring `reader.mu`, then -starts or reuses the session and registers the cancellation callback after -acquiring the mutex. It does not recheck the request context between those -steps. `abortLocked` records any exchange cancellation as the reader's terminal -error. - -**Circumstances:** Two goroutines call `Read` on one reader with independent -request contexts. The first holds the mutex during a slow exchange. The -second passes its initial context check, waits for the mutex, and is canceled -while waiting. Its cancellation callback is not registered during that wait. - -Once the first exchange finishes, the second acquires the mutex and proceeds -with an already-canceled context. Its newly registered `context.AfterFunc` -can immediately cancel the healthy child and close its pipes. The read then -records a terminal cancellation, affecting subsequent uncanceled requests. - -**Reproduction:** A controlled batch process signaled when the first exchange -was active and waited for an external release signal. An observed request -context established that the second read had passed its initial context check -before cancellation. The canceled request remained blocked during a 100 ms -observation window. After releasing the first exchange: - -```text -first read error: -second read canceled: true -next uncanceled read inherits cancellation: true -``` - -The race-enabled probe produced no data-race diagnostic. The defect concerns -the ordering of cancellation and session ownership. - -**Consequences and current reach:** Queued canceled work can wait behind an -unrelated exchange and then terminate its otherwise usable shared reader. -The helper advertises serialized reads and implements mutex-based sharing, -but current candidate validation calls it sequentially. This is an internal -concurrent-use weakness; the audit did not identify a current public operation -that issues these overlapping reads on one reader. - -**Potential remediation:** At minimum, recheck `ctx.Err()` immediately after -acquiring the mutex and before starting an exchange or registering a callback -that can cancel the child. This prevents a request canceled while queued from -changing the healthy session. If queued reads should return promptly on their -own cancellation, use cancellation-aware acquisition of the exchange slot. -Keep current candidate-tree traversal sequential while assessing that need; -this finding supplies no reason to parallelize validation or add locks to its -operation-local caches. A focused helper regression can verify the healthy -first exchange and a later uncanceled read after cancellation of queued work. - -## Recommendation - -Complete the existing subprocess-cancellation remediation first, including the -explicit batch-reader shutdown path. Address the queued-read context recheck -as a small internal correctness improvement; select cancellation-aware queue -waiting according to the helper's intended concurrent-use contract. Retain the -current operation-local architecture and joined worker lifetimes. diff --git a/docs/roadmap/audit/05-resources.md b/docs/roadmap/audit/05-resources.md deleted file mode 100644 index 86f1dab..0000000 --- a/docs/roadmap/audit/05-resources.md +++ /dev/null @@ -1,101 +0,0 @@ -# Resource management audit - -Reviewed baseline: `9545bb0` on `main`. This review traced subprocesses, -buffers, committed blobs, temporary storage, HTTP responses, upload workers, -and cancellation across the public facade, CLI, and internal packages. The -findings below are based on source inspection; no load measurements or new -tests were run. The existing [concurrency audit](04-concurrency.md) records -inherited subprocess descriptors delaying cancellation and batch-reader -shutdown, and the [security audit](07-security.md) records unbounded unique -asset redirects and remote asset reads beyond the expected size. Those remain -separate remediation items. - -## 1. Go command output is retained for the entire execution - -**Relevant code:** [gotool.execute and combinedWriter](../../../internal/gotool/gotool.go), -and its callers in [shared validation](../../../internal/validation/validation.go) -and [dependency preparation](../../../internal/preparation/preparation.go). - -Every write to the combined stdout/stderr writer is appended to an in-memory -buffer, even for status-only `Run` calls and even when output is forwarded to -a caller's writer. A large or long-running Go test suite, compiler, or module -download with voluminous diagnostics therefore retains memory proportional to -all output until the process exits. On failure, formatting the full output in -an error can add another large allocation. Multiple simultaneous operations -multiply that consumption and can exhaust the process or host. The -[internal guide](../../internal/gotool.md#command-output-ownership) already -documents the full-capture choice and its memory tradeoff. - -**Potential approaches:** Evaluate spooling status-only diagnostics to a -temporary file while streaming output, with explicit cleanup and error -handling, or select a bounded diagnostic policy after deciding how much error -output callers should retain. Preserve full output where `Tool.Output` needs -it, forwarding behavior, error identity, and useful failure diagnostics; a -truncation policy changes the current diagnostic behavior and needs a deliberate -decision. Avoid replacing memory exhaustion with unbounded, unowned disk usage. - -## 2. Forge JSON responses have no resource bound - -**Relevant code:** [getJSON, getRelease, postRelease, and decodeJSON](../../../internal/gitea/release.go), -including attachment listing in [assets.go](../../../internal/gitea/assets.go) -and upload response decoding in [upload.go](../../../internal/gitea/upload.go). - -Successful HTTP response bodies are decoded directly without a byte limit. -The decoder buffers complete JSON values, including fields that are not used, -and attachment lists and release bodies are retained in memory. A faulty or -compromised forge returning a very large value or continuously streaming an -unfinished value can consume substantial memory or keep the operation active. -The second decode waits for EOF to reject trailing values; a response that -never ends can also prevent completion. The default client has no overall -timeout, although callers can cancel through the operation context. Response -bodies are closed when these functions return; the issue is consumption before -they can return, not a missing close. - -**Potential approaches:** Choose response-size and time budgets appropriate -to the supported forge contract, potentially configurable for large release -notes or attachment lists. Enforce a distinguishable limit rather than -silently accepting truncated JSON, and retain context cancellation and -malformed-response classification. For a failed create or upload response, -preserve uncertain-write reconciliation: rejecting a response does not prove -that the remote write failed. Any default limit is a compatibility decision, -not an implied existing restriction. - -## 3. Temporary build cleanup failures are silently discarded - -**Relevant code:** [candidate build callback](../../../internal/build/candidate.go), -[native smoke build](../../../internal/build/build.go), and -[managed delivery build](../../../internal/delivery/delivery.go). Each defers -`os.RemoveAll` without inspecting its error. - -If removal fails, for example because permissions or filesystem availability -change, the operation can return success or an unrelated failure while leaving -generated binaries and their directories behind. Repeated builds can consume -disk space, and an operator is not told where to clean up. The delivery guide -describes managed output as removed when delivery returns. This is distinct -from the previously recorded cancellation hang, which can prevent cleanup -from being reached at all. - -**Potential approaches:** Capture cleanup errors and report the affected path, -joining them with an existing failure where useful. Decide how to expose a -cleanup failure after remote delivery has succeeded without misrepresenting the -already completed remote state or encouraging an unsafe retry. Keep caller-owned -prebuilt artifacts outside this cleanup path. Apply the same review to the -temporary tag-message and receipt files, which also discard removal errors. - -## 4. A failed temporary-path resolution loses the cleanup path - -**Relevant code:** [tempdir.Outside](../../../internal/tempdir/tempdir.go), -at both calls to `filepath.EvalSymlinks` after `os.MkdirTemp`. - -The resolved value is assigned back to `directory` before checking the error; -on a resolution error the cleanup uses that returned value instead of the -saved `created` path. If the returned path is empty or otherwise unusable, -the newly created directory is not removed. This can occur if path resolution -fails after creation, for example after a concurrent filesystem or permission -change. The helper returns an error without telling the caller the directory's -name, so the caller cannot perform cleanup. The directory is initially empty; -the impact is leaked temporary entries rather than leaked build artifacts. - -**Potential approach:** Keep the created path until resolution succeeds and -use it for error-path removal, including the fallback allocation. Preserve the -original resolution error and consider reporting a cleanup error as well. diff --git a/docs/roadmap/audit/06-error-handling.md b/docs/roadmap/audit/06-error-handling.md deleted file mode 100644 index 66f5422..0000000 --- a/docs/roadmap/audit/06-error-handling.md +++ /dev/null @@ -1,54 +0,0 @@ -# Error-handling audit - -Reviewed baseline: `3754242` on `main`. The review traced public errors and -partial results through candidate validation, Git publication, build and -preparation, subprocess execution, and hosted delivery. Existing findings about -subprocess cancellation, publication integrity, temporary cleanup, and remote -resource consumption are retained in the [correctness](01-correctness.md), -[concurrency](04-concurrency.md), [resource](05-resources.md), and -[security](07-security.md) audits rather than duplicated here. - -The existing `internal/delivery`, `internal/git`, and `internal/gitea` tests -passed with the Go test cache. An isolated temporary repository, a copy of the -Git and process packages, and a Git wrapper were used to exercise the finding -below; no production source or repository tests were modified. - -## A post-creation inspection failure hides a retained local tag - -**Relevant code:** [CreateAnnotatedTag](../../../internal/git/tag.go), -[publish.Publish](../../../internal/publish/publish.go), and the public -[PublicationResult](../../../publish.go). - -**Failure condition:** Git successfully creates the annotated local tag, but -a subsequent `cat-file` or `rev-parse` inspection fails, for example because -the process is interrupted or the local Git operation encounters an error. -`CreateAnnotatedTag` returns an empty object ID on every such error. Its caller -returns immediately without setting `PublicationResult.TagObject`. The local -tag is not removed, and the next publication attempt's candidate check rejects -that existing tag. No remote push has been attempted at this point. - -**Expected and actual behavior:** `PublicationResult.TagObject` is documented -as the local annotated tag object ID when created, and publication results are -intended to give callers evidence for recovery. The result instead reports -`not_published` and the tag-creation phase with an empty object field even -though a local tag may remain. The returned error identifies the failed -inspection but does not explain that tag creation already succeeded. An -operator may have to discover the tag independently before deciding how to -recover; the absence of an object ID is not evidence that no tag exists. - -**Evidence:** In a temporary repository, a wrapper delegated real Git commands -except for the post-creation `cat-file -t` inspection, which returned an error. -`CreateAnnotatedTag` returned an empty object ID and an error, while an -independent Git query confirmed the local tag existed. The public result's -empty field follows from the caller's immediate error return; the probe did -not run the complete public publication workflow. - -**Potential approaches:** Preserve any verified object ID obtained before a -later failure and return it with the error. When creation succeeded but the -object could not be inspected, report explicitly that the local tag may exist -and identify its ref for manual inspection. Keep the failed verification and -original cause visible, never treat the tag as publishable without the existing -checks, and avoid deleting or changing a ref whose current ownership is not -established. A focused regression can force an inspection failure after -successful creation and check both the local ref and returned recovery -information. diff --git a/docs/roadmap/audit/07-security.md b/docs/roadmap/audit/07-security.md deleted file mode 100644 index c18c11f..0000000 --- a/docs/roadmap/audit/07-security.md +++ /dev/null @@ -1,200 +0,0 @@ -# Security audit - -Reviewed baseline: `1b39e71` on `main`, using Go 1.26.10 and Git 2.53.0 on -Linux. The three findings below remain pending. - -## Architecture and threat scope - -The root facade coordinates candidate validation, source publication, builds, -preparation, and delivery. `buildinfo` supplies independent runtime version -access. Internal packages own Git, Go execution, filesystem operations, -artifact verification, shared delivery policy, and the Gitea protocol adapter. -The CLI owns configuration, credential loading, output, and exit statuses. - -Repository source and release profiles are trusted execution inputs: candidate -checks run Go tests, configured checks invoke selected programs, and binary -builds execute the application's version command. ReleaseKit runs with the -operator's permissions. Its offline Go settings control module/toolchain -downloads. Repository commands retain the operator's filesystem and network -permissions. -This review assessed that implemented operating model under the -[architecture policy](../../policy/architecture.md#trust-boundaries-and-threat-scope). - -Security-relevant external inputs include remote Git observations, forge HTTP -responses, attachment download URLs and redirects, and downloaded bytes. -Cross-origin asset downloads are an explicitly supported -[integration behavior](../../integrations/gitea.md). The audit evaluated both -credential origin checks and resource consumption while following those -externally supplied URLs. - -The review covered credential supply and diagnostics, HTTP authentication and -redirects, preflight evidence, command arguments, committed path/symlink -resolution, build and receipt paths, artifact integrity, temporary-file -ownership, CI bootstrap examples, and dependency advisories. Local privileged -attackers and malicious code already authorized to run as the operator were -outside the assessed boundary. - -## Verification and observed protections - -The following checks passed: - -```sh -GOTOOLCHAIN=local go run golang.org/x/vuln/cmd/govulncheck@v1.9.0 ./... -GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test -count=1 \ - ./internal/gitea ./internal/artifact ./cmd/releasekit/internal/cli -``` - -`govulncheck` reported `No vulnerabilities found.` for the host build using the -current Go vulnerability database. This is a known-vulnerability result for -the scanned build; it does not establish the security of every supported -platform or older compatible toolchain. See the Go project's -[vulnerability management documentation](https://go.dev/doc/security/vuln/). - -The preceding audits' full ordinary/race suites and facade-boundary checks -cover the same unchanged production code. A targeted check of 191 tracked files -at this baseline found no private-key headers or credential-bearing HTTP URL -patterns. Ignored credential files were outside that content scan. - -Isolated local programs reproduced the findings below. Their credentials were -placeholders; the Git credential probe stopped before any network operation. -HTTP probes used local servers. Production code and repository tests remain -unchanged. - -The review confirmed useful existing protections: direct argument vectors for -commands, committed-tree path boundary handling, strict receipt declarations, -artifact hashing before hosted writes, explicit destination selection, and -read-only preflight with documented permission-evidence limits. Asset redirects -are handled manually, and the Gitea token was withheld from the separate asset -origin in the redirect probe. The findings concern other aspects of those -boundaries. - -## 1. Medium: publication diagnostics expose credentials embedded in Git URLs - -**Relevant code:** [publish.Publish](../../../internal/publish/publish.go), -lines 106–108; publication failure output in -[cmd/releasekit/main.go](../../../cmd/releasekit/main.go), line 63; and URL-bearing -errors in [internal/git/remote.go](../../../internal/git/remote.go), -[internal/git/tag.go](../../../internal/git/tag.go), and -[publication reconciliation](../../../internal/publish/publish.go). - -**Vulnerability and circumstances:** Git supports HTTPS remotes containing -userinfo credentials, including an access token in the password position. -ReleaseKit retains that complete effective URL and interpolates it directly -into errors. The CLI also prints `PublicationResult.Endpoint` verbatim on a -publication failure. An ordinary URL mismatch, unavailable remote, or later -publication failure can therefore copy the credential from local configuration -into terminal transcripts, CI logs, or a shared diagnostic report. - -**Reproduction:** A temporary clean candidate used an HTTPS `origin` containing -a placeholder password, with a different credential-free `ExpectedPushURL`. -A public `Publish` call returned `ErrUnexpectedPushURL`. Both the error text -and returned endpoint contained the complete placeholder password. This -happened before validation or network access. Inspection of the CLI's failure -formatter confirms that both values flow into its output. - -**Impact:** Readers of diagnostic output can obtain a working Git password or -access token even when they have no access to the operator's local repository -configuration. The token's permissions determine the resulting repository -access. This exposure concerns Git URL credentials; the probe did not find a -leak of the separately supplied Gitea delivery token. - -**Potential remediation:** Introduce a credential-safe URL representation at -diagnostic boundaries and use it consistently in library errors and CLI output. -The standard library's [URL.Redacted](https://pkg.go.dev/net/url#URL.Redacted) -handles passwords; review username-carried tokens when choosing userinfo -redaction. Check wrapped Git diagnostics as well as ReleaseKit-owned messages. -Retain the original URL for transport and exact destination comparison, and -preserve error identity. `PublicationResult.Endpoint` currently promises the -captured effective URL: keep that contract explicit, document its sensitivity, -and sanitize its CLI rendering rather than silently changing transport or -comparison semantics. A regression should exercise a credential-bearing -remote and verify safe errors/output while exact comparison remains intact. - -## 2. Medium: unique asset redirects bypass the HTTP client's finite redirect limit - -**Relevant code:** [Client.httpClient](../../../internal/gitea/release.go), -which disables automatic redirects, and -[Client.openAsset](../../../internal/gitea/assets.go), lines 58–85. - -**Vulnerability and circumstances:** The manual redirect loop retains every -visited URL and rejects an exact repeated URL. It has no hop limit. A content -server can continually redirect to a new query value or path, so every request -passes cycle detection and adds another retained string. The ordinary Go -client's redirect bound is bypassed because each hop is issued as a separate -`Do` call with automatic redirection disabled. The CLI uses a signal-driven -context without an automatic delivery deadline, and the default HTTP client -has no total timeout. - -A compromised external attachment host, or an accidentally recursive redirect -rule that keeps changing the URL, can trigger this during asset inspection. -This scenario uses the supported cross-origin download path; it requires no -ability to execute repository code or obtain the delivery token. - -**Reproduction:** A local forge fixture returned an attachment URL on a second -local server. That server issued 64 redirects with distinct query values and -then returned content. `Backend.OpenAsset` completed after 65 asset requests. -An ordinary Go HTTP client rejected the same chain under its default redirect -policy. The fixture separately confirmed that no Gitea authorization header -reached the second origin. The standard client's default policy is documented -under [http.Client](https://pkg.go.dev/net/http#Client). - -**Impact:** A hostile or faulty content host can occupy the delivery process -and generate requests indefinitely. The visited-URL map grows with the chain, -creating a memory-exhaustion path as well as network and runner-time costs. -The probe demonstrated excess redirects, not an out-of-memory crash. - -**Potential remediation:** Apply a finite request-local redirect budget while -retaining the current per-origin credential checks and response-body cleanup. -The normal Go limit of ten redirects is a proportionate starting point. -Choose and document the budget deliberately during remediation. If actual -consumer workflows need longer chains, evaluate an explicit override alongside -that decision. A configured deadline can supplement this, while a hop budget -also bounds retained URL history. Regression coverage should include distinct-URL -chains, ordinary redirects, exact cycles, and authorization across origins. -This finding does not call for prohibiting supported external content hosts. - -## 3. Low: remote content verification reads beyond the known matching size - -**Relevant code:** [inspectRemoteAssets and remoteAssetDigest](../../../internal/delivery/remote_assets.go), -particularly lines 80 and 102–113. Each expected artifact already carries its -local verified `Size`, but the download comparison passes only the remote -handle and hashes the stream to EOF. - -**Vulnerability and circumstances:** An attachment response can be much larger -than the declared local asset, or continuously supply bytes without ending. -After the response exceeds the expected file size, it cannot match the local -declaration, but ReleaseKit continues downloading and hashing it. An external -content host controlling this response can prolong useful-looking activity and -consume bandwidth and CPU for a result that is already known to conflict. - -**Reproduction:** A public `Deliver` call used a valid temporary tagged checkout -and a caller-owned three-byte prebuilt artifact. The local forge fixture listed -the expected filename and a reported size of three bytes, while its download -endpoint returned 65,536 different bytes. A counting response-body wrapper -confirmed that delivery read all 65,536 bytes before reporting `conflict`. -No hosted writes occurred. The stream was finite to keep the probe bounded. - -**Impact:** A small expected asset can induce arbitrarily larger downloads and -hashing work. This is a resource-consumption weakness; the finite probe still -reported the content conflict correctly. It is lower priority than the -credential exposure and redirect-history growth. - -**Potential remediation:** Pass the locally verified expected size into remote -content verification and consume at most that size plus one byte. Detecting an -extra byte establishes a conflict; a shorter stream also conflicts. Keep the -digest comparison for equal-length streams, close the stream on every path, -and retain inspection errors/cancellation causes. Derive the bound from the -local declaration, preserving the supported artifact sizes and avoiding an -arbitrary global download cap. This addresses excess bytes; a stalled server -before that bound still follows the operation's cancellation/timeout behavior. - -## Related work and recommendation - -The [correctness audit](01-correctness.md) tracks publication integrity issues, -and the [concurrency audit](04-concurrency.md) tracks subprocess shutdown and -queued-reader cancellation. Their remediation remains relevant to release -integrity and availability. - -Prioritize credential-safe diagnostics, then bound manual redirect handling -and remote mismatch reads. Retain the existing source/profile trust model, -explicit forge selection, token origin checks, and additive delivery policy. diff --git a/docs/roadmap/audit/08-testing.md b/docs/roadmap/audit/08-testing.md deleted file mode 100644 index a184b90..0000000 --- a/docs/roadmap/audit/08-testing.md +++ /dev/null @@ -1,98 +0,0 @@ -# Test coverage and effectiveness audit - -Reviewed baseline: `3d7a691` on `main` against the -[testing policy](../../policy/testing.md). The review considered public and -package contracts, failure and recovery paths, fixture realism, and the cost -of implementation-coupled assertions. Coverage is a way to locate questions, -not evidence that every exercised behavior is correct. No production code or -tests were modified for this audit. - -The offline instrumented suite passed with -`GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test -coverpkg=./... -coverprofile=/tmp/releasekit-audit-coverage.out ./...`. -`go tool cover -func` reported 86.0% -aggregate statement coverage across instrumented packages. Child executables -and platform-specific paths are not necessarily represented by that aggregate; -it does not measure branch coverage or assertion strength. - -## 1. Publication fixtures omit Git settings that change what is published - -The public [publication success test](../../../publish_test.go) checks the -remote tag object, peeled commit, message, and unrelated refs using a local -bare remote. However, its ordinary Git configuration does not exercise -`push.recurseSubmodules=only`, under which the -[correctness audit](01-correctness.md#1-high-publication-can-report-success-while-leaving-the-tag-unpublished) -reproduced a successful result with no remote tag. The same test checks object -IDs and tag-message bytes, but its fixture has no Git replacement objects; -the [second correctness finding](01-correctness.md#2-high-git-replacement-objects-can-separate-validated-content-from-published-source) -demonstrates that the validated contents and remote contents can differ while -the recorded commit ID remains the same. - -When these defects are addressed, add focused public-boundary regressions with -a real local bare remote. For the inherited push mode, assert that a successful -result corresponds to the exact remotely present tag object. For replacement -objects, compare the actual validated and remotely published source and note -bytes, or assert a contextual rejection before publication. Preserve the -existing ordinary success case; it protects different important behavior. - -## 2. Cancellation fixtures do not cover inherited subprocess descriptors - -[process cancellation tests](../../../internal/process/process_test.go) and -[candidate reader lifecycle tests](../../../internal/validation/tree_lifecycle_test.go) -exercise cancellation of a ready child and check error identity and cleanup. -Their blocked children do not leave a descendant holding an output descriptor -after the direct child is killed. The independent probes recorded in the -[concurrency audit](04-concurrency.md#1-medium-inherited-descriptors-can-keep-canceled-operations-and-reader-shutdown-blocked) -show that both buffered execution and batch-reader shutdown can then remain -blocked past cancellation, despite those tests passing. - -Add controlled regressions when repairing the shared shutdown path: establish -that a descendant holds a descriptor, cancel only after readiness, and verify -bounded return, preserved context identity, and cleanup. Exercise both the -buffered subprocess boundary and the explicitly owned batch-reader path; retain -one public preparation or publication-recovery check if needed to show the -shared behavior reaches callers. Avoid treating arbitrary descendant-process -supervision as an additional contract. - -## 3. Local tag creation lacks a post-write inspection failure case - -The [publication tests](../../../publish_test.go) cover successful exact tag -publication, a rejected push with the local tag retained, and a changed tag -before push. They do not cover failure of the inspection commands immediately -after successful local tag creation. The isolated probe in the -[error-handling audit](06-error-handling.md#a-post-creation-inspection-failure-hides-a-retained-local-tag) -shows that the tag can remain while the returned object ID is empty and the -error does not identify that partial local state. - -When the recovery behavior is selected and fixed, use a controlled Git wrapper -and a real temporary repository to fail post-creation inspection. Verify both -the actual local ref and the reported recovery evidence, the original error, -and absence of a remote push. Do not rely solely on a mocked return value. - -## 4. Some assertions lock in incidental internal mechanisms - -[TestMarkdownCacheSharesParsedDocumentsByBlobObjectID](../../../internal/validation/markdown_test.go) -requires two parsed document pointers to be identical. A correct cache that -returns independent immutable values, or another memory strategy, would fail -without changing link validation. The separate -[shared-document link test](../../../internal/validation/markdown_check_test.go) -already checks that identical content resolves relative to each source path. -Consider removing the pointer-identity assertions or the cache-specific test -after retaining any unique parsing checks at the behavior boundary. - -`TestDeliverReadsLocalBinaryOnceForVerificationAndOnceForUpload` in -`delivery_reads_linux_test.go` at audit baseline `3d7a691` -requires exactly two filesystem opens using Linux inotify. Verification before -hosted writes and correct uploaded bytes matter; the exact number of opens is -an implementation choice, and a harmless extra metadata open or a different -safe streaming approach would fail the test. Existing delivery and artifact -tests cover tampering and uploaded content. Consider removing this exact-count -test or moving a justified read-volume concern to a benchmark, while retaining -behavioral verification of artifact integrity and ordering. - -The success case in [TestPublishPushesOnlyTheCapturedAnnotatedTag](../../../publish_test.go) -also asserts exactly three remote queries, including no post-success read-back. -That exact success-path count is not necessary to establish the public result, -and would reject an additional verification step even if it preserved the -contract. Consider removing that assertion while retaining the exact remote -tag, message, and unrelated-ref checks. The separately documented single -reconciliation query after a failed push remains a distinct contract. diff --git a/docs/roadmap/audit/09-interfaces.md b/docs/roadmap/audit/09-interfaces.md deleted file mode 100644 index 4363ccc..0000000 --- a/docs/roadmap/audit/09-interfaces.md +++ /dev/null @@ -1,51 +0,0 @@ -# API and interface contract audit - -Reviewed baseline: `b345bc0` on `main`. The review compared the public Go -facades, CLI and profile behavior, and internal Git, process, validation, -build, and forge interfaces with their documented contracts and callers. -The existing [correctness](01-correctness.md), [concurrency](04-concurrency.md), -[resource](05-resources.md), [error-handling](06-error-handling.md), and -[security](07-security.md) audits already record related contract violations; -they remain pending and are not repeated here. - -The compiled public API and dependency-boundary check passed. The finding -below was reproduced through the public API in an isolated temporary Git -repository with a controlled Go-command wrapper. No production code or -repository tests were modified. - -## A partial build result can contain an artifact without its promised digest - -**Relevant interfaces and code:** [BuildArtifact and BuildResult](../../../build.go), -[BuildResolved and hashArtifact](../../../internal/build/build.go), and the -build failure presentation in [the CLI](../../../cmd/releasekit/main.go). - -`BuildArtifact.SHA256` is documented as the lowercase digest of the complete -file, and `BuildResult.Artifacts` describes completed outputs, including -partial success. After a compiler reports success, `BuildResolved` appends the -artifact to its result before opening and hashing the file. If opening or -reading that output fails, it returns the partial result with that artifact's -`SHA256` still empty. The public conversion forwards the entry unchanged; the -CLI also prints it among the built outputs without a digest. - -This can occur if the generated file becomes unavailable or unreadable before -or during hashing, or if the filesystem reports a read failure. A caller -examining partial results cannot treat every returned artifact as carrying the -documented digest, and an unavailable path may be presented as an output. The -operation does return an error and does not claim a successful complete build; -this finding concerns the contents of its partial result. - -**Evidence:** A temporary clean module and a wrapper around the real Go -command simulated a file disappearing after a successful compilation. The public -`Build` call returned an error and one artifact with `SHA256` equal to the -empty string. This controlled probe exercised the failure boundary, not a real -filesystem fault. - -**Potential approaches:** Decide how callers should learn about a compiled -output whose digest could not be obtained. One option is appending an artifact -to the completed list only after hashing succeeds, while preserving earlier -completed artifacts and explaining the affected path in the error. If -reporting the unverified output is required for recovery, represent that state -explicitly and document the corresponding partial-result contract. Preserve -the failure cause and existing partial outputs; do not silently claim a digest -or delete caller-owned output. A focused regression can inject a post-build -read failure and verify the public result and CLI presentation. diff --git a/docs/roadmap/audit/10-equivalence.md b/docs/roadmap/audit/10-equivalence.md deleted file mode 100644 index 121612b..0000000 --- a/docs/roadmap/audit/10-equivalence.md +++ /dev/null @@ -1,84 +0,0 @@ -# Behavioral equivalence and shared abstraction audit - -Reviewed baseline: `5e6db4d` on `main`. This review started with tests of -corresponding behavior and traced the implementation behind each candidate. -It considered the existing facade and package boundaries, including the -[architecture policy](../../policy/architecture.md), and retained deliberate -differences between operations. No production code or tests were modified. - -The offline tests for `internal/candidate`, `internal/git`, `internal/delivery`, -and `cmd/releasekit/internal/cli` passed using the Go test cache. Focused public -tests of build source eligibility, inspection errors, and dirty tagged-source -delivery also passed. - -## 1. Source-state observations are independently implemented - -**Test evidence:** -[TestCheckRejectsDirtyTrackedAndUntrackedState and TestCheckRechecksHeadAndWorktreeAfterCallback](../../../internal/candidate/check_test.go) -exercise clean worktree/index and stable-HEAD requirements for candidate -checking. [TestBuildSourceEligibilitySurvivesLaterCheckoutCleanup](../../../build_test.go) -checks the same kinds of changes before and during a build, while -[TestDeliverRejectsDirtyTaggedSourceBeforeBuildOrHostedWrites](../../../delivery_test.go) -and [TestInspectTaggedSourceReportsDirtyCheckoutAndReadCommitFile](../../../internal/git/source_test.go) -exercise dirty-state observations for tagged delivery. These tests imply a -common need for accurate Git HEAD and repository-wide status observations; -they do not imply identical decisions about dirty source. - -**Implementation:** [candidate.inspectLocalState](../../../internal/candidate/check.go) -runs branch, HEAD and porcelain-status commands. [build.gitCommit and cleanSource](../../../internal/build/build.go) -independently run HEAD and status commands through direct subprocesses. -[git.CaptureHead and InspectTaggedSource](../../../internal/git/source.go) -perform overlapping observations through the Git package's command boundary. - -**Opportunity:** The existing Git package could own small shared commit and -status observation primitives, leaving each operation to choose when to read -them and how to interpret them. This would centralize a concrete Git -responsibility and reduce the number of paths needing parallel fixes for -inspection, error propagation, and cancellation. The -[complexity audit](03-complexity.md#2-git-source-inspection-is-repeated-across-package-boundaries) -also identifies the duplication; the tests above establish the common -behavioral evidence for considering it here. - -**Tradeoffs:** Candidate checking rejects dirty state and checks the branch; -builds can succeed with dirty state but record source eligibility before and -after compilation; delivery additionally checks a specific tag. Do not merge -those policies or alter observation timing. Retain separate caller-level -tests, so a shared helper cannot make every test pass while all consumers -interpret its results incorrectly. Coordinate with the existing -[replacement-object and cancellation findings](01-correctness.md). - -## 2. Build-artifact declarations are checked independently by receipts and delivery - -**Test evidence:** -[TestBuildReceiptRoundTripBindsRootAndNestedModuleIdentity](../../../cmd/releasekit/internal/cli/receipt_test.go) -constructs and reloads a receipt binding a selected target to its expected -filename. [TestDeclaredBuildAssetsGenerateManifestFromBuildResult and TestDeclaredBuildAssetsRejectsArtifactDeclarationMismatch](../../../internal/delivery/assets_test.go) -exercise the corresponding build-result declarations and reject a mismatched -filename. [TestBuildAndPrebuiltDeliveryUseSameDirectoryBase](../../../cmd/releasekit/assets_test.go) -connects the two paths through a real build, receipt, and prebuilt delivery. -Together they support a shared requirement that a target's recorded artifact -identity agree with the selected build, without requiring receipt loading to -read artifact contents. - -**Implementation:** Both -[validateBuildReceipt](../../../cmd/releasekit/internal/cli/receipt.go) and -[declaredBuildAssets](../../../internal/delivery/assets.go) independently -check target uniqueness, artifact counts, one artifact per target, and names -derived from targets. They already share `artifact.ExpectedBuild` and -`artifact.ValidComponent`, but each builds and checks its own target-to-name -mapping. - -**Opportunity:** Consider a narrow, pure declaration-matching helper in the -existing [artifact package](../../../internal/artifact/artifact.go), accepting -target/filename metadata and reporting a mismatch without opening files. Both -paths could reuse it after converting their own types. Centralizing the shared -identity rule would reduce drift when target or filename conventions evolve. - -**Tradeoffs:** The receipt additionally requires a schema, specific output -path and digest syntax, and intentionally loads without checking the files; -delivery accepts source-only results and separately stats and verifies local -content after preflight. Keep those boundary-specific rules and ordering where -they are. Avoid making `internal/artifact` depend on CLI receipt types, -`internal/build`, or forge policy. Preserve independent public and CLI tests -with concrete expected filenames and bytes rather than using the new helper to -compute every test expectation. diff --git a/docs/roadmap/audit/11-deadcode.md b/docs/roadmap/audit/11-deadcode.md deleted file mode 100644 index ef3b0e8..0000000 --- a/docs/roadmap/audit/11-deadcode.md +++ /dev/null @@ -1,59 +0,0 @@ -# Dead code and obsolete functionality audit - -Reviewed baseline: `083bc83` on `main`. The review considered supported public -facades and CLI commands, internal package callers, tests and benchmarks, -maintained examples, documented decisions, and build-specific uses. Public -symbols were not classified as unused merely because this repository does not -call them. No production code or tests were changed. - -The local code graph and a source reference scan identified candidates, which -were checked against current source and documentation. The graph contained a -stale build-function entry and excluded `internal/build` during indexing, so -its reachability results were not treated as conclusive. The scan is a -candidate-finding aid rather than a proof of whole-program reachability. -`go vet ./...` and the compiled public API/dependency-boundary check passed -offline. No dedicated dead-code analyzer was installed. - -## 1. Committed-selection identity wrapper has no callers - -**Relevant code:** [FromCommittedSelection](../../../internal/releaseidentity/identity.go). -The repository contains no call or function-value reference to this exported -internal wrapper, including in tests and maintained examples. It creates a -committed-path resolver and delegates to the private `fromCommittedSelection`. -The live `FromCommitted` path calls that private function directly with the -resolver it already used to select the directory. The independently used -`ResolveCommittedSelection` serves the CLI's asset-directory selection and -must not be confused with the unused wrapper. - -Because Go's `internal` import restriction prevents ordinary external consumers -from importing this package, this is not a root-facade compatibility removal. -No documented retention or future use was found. Consider removing only the -unused wrapper after confirming that no maintained in-tree integration needs -it. Preserve the private implementation and the live selection APIs; the -wrapper's absence does not imply that committed identity derivation is dead. - -## 2. Local tag inspection helper is used only by its own test - -**Relevant code:** [InspectLocalTag](../../../internal/git/tag.go), its only -call in [TestTagOperationsAcceptFullSlashContainingRefs](../../../internal/git/source_test.go), -and the live local guard in [candidate.Recheck](../../../internal/candidate/check.go). - -The helper checks that a local ref identifies the captured annotated tag -object, but no production caller invokes it. `candidate.Recheck` independently -performs the corresponding object-ID and type checks before publication. The -test calls the helper directly and then separately inspects the tagged source; -the public publication guard is exercised through the candidate and -publication tests. The helper is in an internal package, not the supported Go -facade. No documented reason to retain this separate test-only path was found. - -Consider removing the unused helper and its direct test call while retaining -the test's full-tag and source assertions and the live publication-guard -coverage. Alternatively, evaluate using the existing helper in the live guard -as part of the [shared Git inspection work](10-equivalence.md#1-source-state-observations-are-independently-implemented), -provided error behavior, guard timing, and the exact captured-object check are -preserved. Do not remove the live guard merely because the duplicate is unused. - -The `CandidateTree` accessors and root-scope constructor are also used by tests -or benchmarks, but those uses exercise meaningful tree behavior; this review -does not recommend removing them solely because production uses a different -entry point. diff --git a/docs/roadmap/deferred-resources.md b/docs/roadmap/deferred-resources.md new file mode 100644 index 0000000..a3911dc --- /dev/null +++ b/docs/roadmap/deferred-resources.md @@ -0,0 +1,67 @@ +# Deferred resource work + +These two proposals remain deferred after the completed audit remediation for +v0.7.0. Their assessments are complete; production limits and exclusion indexing +await a future decision supported by consumer needs. The assessment evidence +was recorded in commit `9dc87ae`. + +## Forge JSON response budgets + +**Status:** Deferred; local sizing assessment complete. + +Forge JSON decoding can consume memory proportional to response size. Release +notes and unpaginated attachment lists have no established supported maximum. +A future proposal should establish an appropriate response envelope before +selecting byte/time budgets or configuration. Any limit must retain context +cancellation, useful error classification, and uncertain-write reconciliation: +a rejected response does not establish that a create or upload failed remotely. + +The [Gitea fixtures](../../internal/gitea/backend_test.go) construct repository, +release, tag, and attachment responses. Representative encoded fixtures measured +81 bytes for repository identity, 136 with permissions, 125 for a sparse release, +144 for a populated release, 73 for a one-item attachment list, and 62 for an +upload response, including the encoder's newline. Four literal release-ID +fixtures measured 21, 28, 29, and 29 bytes without a newline. Generated URLs, +notes, and lists vary with fixture inputs. + +The checked-in v0.1.0–v0.6.0 release notes measured 1,235–7,899 bytes during the +assessment. These observations describe local examples and establish no +deployment or protocol maximum. Production response caps remain deferred. + +## File-size exclusion indexing + +**Status:** Deferred; benchmark assessment complete. + +The current [exclusion scan](../../internal/validation/file_size.go) compares +each selected file with the normalized exclusions until a match is found. +The adoption decision weighs measured speedups against allocation and +path-walking costs for consumer workloads. Preserve literal component boundaries, +module-relative normalization, shared inputs, and the first oversized-file result. + +The [benchmark-only prototype](../../internal/validation/file_size_benchmark_test.go) +compares the scan with a normalized set and ancestor lookups. It covers 1,000 +and 10,000 paths, 1/10/100/1,000 exclusions, shallow/deep paths, early matches, +and nonmatches. Each measured operation includes normalization and one index +construction where applicable; fixture strings are prepared outside timing. + +On Linux/amd64, Go 1.26.8, Intel Core i5-7260U, 64 combinations ran with three +samples each using: + +```sh +GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test ./internal/validation \ + -run '^$' -bench '^BenchmarkFileSizeExclusions$' -benchtime=100ms -count=3 +``` + +Representative median times and allocations were: + +| Paths / exclusions / case | Scan | Index | +| --- | --- | --- | +| 1,000 / 1 / shallow early match | 24.5 µs, 16 B, 1 allocation | 44.0 µs, 16 B, 1 allocation | +| 1,000 / 1 / deep nonmatch | 25.7 µs, 16 B, 1 allocation | 119.0 µs, 16 B, 1 allocation | +| 10,000 / 1,000 / shallow early match | 270 µs, 16,384 B, 1 allocation | 564 µs, 70,992 B, 6 allocations | +| 10,000 / 1,000 / shallow nonmatch | 227.0 ms, 16,384 B, 1 allocation | 521 µs, 70,992 B, 6 allocations | +| 10,000 / 1,000 / deep nonmatch | 233.4 ms, 16,384 B, 1 allocation | 1.36 ms, 70,992 B, 6 allocations | + +The prototype was slower for small lists and early matches, and faster for +large nonmatching lists with higher allocation costs. Production matching +remains unchanged pending an adoption decision. diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md deleted file mode 100644 index 5d2da08..0000000 --- a/docs/roadmap/implementation.md +++ /dev/null @@ -1,648 +0,0 @@ -# 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. - -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 | Complete — regression failed before the fix and passed afterward; focused publication and reconciliation tests passed. | -| 2 | A02, A12: common Git mechanics | Complete — focused Git, candidate, identity, and public build/publication/delivery tests passed; A02 regression evidence follows in Stage 3. | -| 3 | A02: source identity regression evidence | Complete — commit, tree, and blob replacement regressions and public publication, build, and delivery cases passed. | -| 4 | A03: buffered command teardown | Complete — inherited-pipe, cancellation, healthy-command, native build, and publication recovery checks passed. | -| 5 | A03, A18: piped lifetime and queued cancellation | Complete — process and Git lifecycle tests passed under the race detector; public preparation cancellation passed. | -| 6 | A04 | Complete — Git display, endpoint change, public mismatch, CLI, publication, and Git failure regressions passed. | -| 7 | A05 | Complete — real-repository inspection failures preserve the local ref, available object ID, and Git cause without a push. | -| 8 | A06, A14 | Complete — public build and prebuilt-delivery checks and CLI receipt/partial-artifact regressions passed. | -| 9 | A16 | Complete — shared declaration, receipt, delivery, prebuilt, and source-only regressions passed. | -| 10 | A07: allocation and temporary metadata | Complete — allocation resolution, joined cleanup causes, tag evidence, and receipt installation checks passed. | -| 11 | A07: build and delivery outcomes | Complete — managed and native cleanup tests cover success, failure, cancellation, hosted completion, and CLI reporting. | -| 12 | A08 | Complete — redirect boundary, origin authorization, cycle, and delivery outcome tests passed. | -| 13 | A09 | Complete — bounded reads, size and digest mismatches, stream failures, cancellation, reconciliation, and public prebuilt delivery checks passed. | -| 14 | A10 | Complete — bounded and full capture, forwarding, error identity, concurrency, and benchmark modes passed. | -| 15 | A13 | Complete — validation ordering, scope, tree lifecycle, diagnostics, and public callback checks passed. | -| 16 | A15 | Complete — the full CLI command suite passed after workflow extraction. | -| 17 | Remaining A19, A20 | Complete — Git, identity, and Markdown tests passed; delivery benchmark ran for fresh uploads and matching retries. | -| 18 | A11, A17: assessment only | Assessment complete; production JSON limits and exclusion indexing remain deferred. Evidence follows below. | -| 19 | Integrated verification and current documentation | Complete — full offline tests, race tests, API/dependency checks, formatting, and documentation link checks passed. | - -## 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. - -### Assessment evidence - -**A11 — observed local sizes, not limits.** The Gitea tests construct repository, -release, tag, and attachment responses in code; the public delivery fixture also -constructs release and attachment lists with variable URLs, note bodies, and -asset counts. Representative encoded fixture responses, including the JSON -encoder's trailing newline, measured 81 bytes for repository identity, 136 -bytes with permissions, 125 bytes for a sparse release response, 144 bytes for -a populated release response, 73 bytes for a one-item attachment list, and 62 -bytes for an attachment upload response. The four literal release-ID fixtures -in `internal/gitea/backend_test.go` are 21, 28, 29, and 29 bytes without a -newline. Empty, malformed, and missing-field responses are also represented; -generated list and note lengths vary with the fixture inputs. - -The eight checked-in release notes measured: `v0.1.0` 3,400 bytes, `v0.2.0` -6,086, `v0.3.0` 6,006, `v0.4.0` 7,899, `v0.4.1` 1,235, `v0.4.2` 1,826, -`v0.5.0` 3,179, and `v0.6.0` 7,297. These local examples do not establish -deployment or protocol maxima. JSON size/time caps and new configuration remain -deferred; existing cancellation and ambiguous-write handling remain in force. - -**A17 — benchmark-only index.** On Linux/amd64, Go 1.26.8, Intel Core -i5-7260U, the following command ran 64 combinations, three samples each: - -```sh -PATH=/home/eric/sdk/go1.26.8/bin:$PATH TMPDIR=/var/tmp \ - GOCACHE=/tmp/releasekit-go-cache GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off \ - go test ./internal/validation -run '^$' -bench '^BenchmarkFileSizeExclusions$' \ - -benchtime=100ms -count=3 -``` - -Each operation normalizes exclusions and, for the prototype, builds one index. -Paths and fixture strings are prepared outside the timed interval. Median -times and allocations for representative cases were: - -| Paths / exclusions / case | Scan | Index | -| --- | --- | --- | -| 1,000 / 1 / shallow early match | 24.5 µs, 16 B, 1 allocation | 44.0 µs, 16 B, 1 allocation | -| 1,000 / 1 / deep nonmatch | 25.7 µs, 16 B, 1 allocation | 119.0 µs, 16 B, 1 allocation | -| 10,000 / 1,000 / shallow early match | 270 µs, 16,384 B, 1 allocation | 564 µs, 70,992 B, 6 allocations | -| 10,000 / 1,000 / shallow nonmatch | 227.0 ms, 16,384 B, 1 allocation | 521 µs, 70,992 B, 6 allocations | -| 10,000 / 1,000 / deep nonmatch | 233.4 ms, 16,384 B, 1 allocation | 1.36 ms, 70,992 B, 6 allocations | - -The prototype is slower for small lists and early matches but substantially -faster for the large nonmatching list, at a higher allocation cost. Production -matching remains unchanged; these observations support a later adoption decision. - -## 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. - -### Integrated verification - -Reviewed the combined A01–A20 outcomes and their cross-package interactions. -The selected repairs and maintenance are implemented; A11 JSON caps and A17 -production exclusion indexing remain deferred with the assessment evidence above. -The public facade shape and receipt schema remain unchanged. Current consumer, -CLI, integration, internal, and Go documentation reflects the selected -contracts and the source audit reports remain available. - -On Linux/amd64 with Go 1.26.8, the following root commands passed with -`GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off` and a temporary build -cache. The test commands used `/var/tmp` for fixtures because the restricted -environment's `/tmp/.git` marker interfered with standalone Git discovery: - -- `go test ./...` -- `go test -race ./...` -- `go run ./internal/apicheck` - -All tracked Go files passed `gofmt` inspection. Local links and Markdown -fragments across 50 documentation files resolved; the policy's literal -`COMMIT_URL` template placeholder was intentionally excluded. Markdown and -combined changes passed `git diff --check`, and the internal guide's referenced -tests were checked against current test declarations. diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md deleted file mode 100644 index 95c8a37..0000000 --- a/docs/roadmap/remediation.md +++ /dev/null @@ -1,231 +0,0 @@ -# Implementation review remediation - -Reviewed `2f95753`, including the implementation commits after `a1dc705`, -against the accepted [audit target](audit.md) and completed -[implementation plan](implementation.md). A follow-up integration audit reviewed -`c8fa165`, whose production code is identical, and added R02 and R03. Findings -and completion records below track the remediation. Production code and existing -tests were unchanged during both reviews. - -## R01. Git diagnostic redaction misses apostrophes in credentials - -**Priority:** P2. **Status:** Complete. This addressed a gap in A04 and -[Stage 6](implementation.md#stage-6-sanitize-git-endpoint-diagnostics). - -**Completion:** Diagnostic matching recognizes complete userinfo before -surrounding quotation, preserving error context and wrapped causes. Native Git, -literal/encoded apostrophe, malformed userinfo, and wrapped-error regressions -pass; the new regressions failed before the fix. The complete `internal/git` -suite and public/CLI endpoint-mismatch redaction tests passed offline. - -**Relevant code:** `httpURLSpan` and `displayDiagnostic` in -[internal/git/display.go](../../internal/git/display.go), lines 9 and 32–39; -`pushURLs` and `run` in -[internal/git/remote.go](../../internal/git/remote.go), lines 209–252. - -**Circumstances and actual behavior:** The diagnostic URL matcher treats every -apostrophe as the end of a URL. An apostrophe is also accepted within URL -userinfo. For `https://user:secret'part@example.test/repo`, the matcher passes -only `https://user:secret` to `DisplayURL`. That truncated span has no `@`, so -it is returned unchanged, leaving the entire credential in the diagnostic. -Direct rendering of the complete URL correctly masks it; sanitizing surrounding -Git text takes the defective path. - -A local reproduction used a real temporary Git repository and passed the URL -above as `Resolve`'s remote name, representing a URL mistakenly configured in -the remote-name field. Native `git remote get-url --push --all` rejects that -name and includes it in its error. The returned ReleaseKit error was: - -```text -resolve push URL for remote "https://redacted@example.test/repo": run "git": exit status 2: error: No such remote 'https://user:secret'part@example.test/repo' -``` - -The values above are placeholders. This reproduction makes no network request. -A separate controlled Git failure reproduced the same leak through `PushTag`. -The existing tests cover simple and percent-encoded credentials but omit this -literal punctuation case. - -**Expected behavior and impact:** Owned diagnostics mask HTTP(S) userinfo while -preserving the underlying Git error identity. The observed failure can expose -a password to callers, terminal output, or logs even though the outer endpoint -field is redacted. The accepted credential-confidentiality repair is incomplete. - -**Recommended remediation:** Recognize the authority and userinfo boundaries -before treating surrounding quotation as a URL delimiter. Sanitize the complete -credential span while retaining useful host/path and surrounding error context. -Keep malformed userinfo handling and `displayError.Unwrap`'s cause preservation. -Add a regression using native Git's local remote-name rejection, plus coverage -for the wrapped-error rendering path and literal versus encoded apostrophes. -Preserve raw transport URLs, exact comparisons, and the raw public endpoint -result. This repair requires no new credential restrictions or public API. - -## R02. Original-object Git overlays retain the caller's PWD - -**Priority:** P2. **Status:** Complete. This addressed the interaction of A02/A12 -with the existing process environment helper and native Git hooks. - -**Completion:** Child overlays now start from `Cmd.Environ()` after setting -`Dir`. Directory-derived `PWD`, explicit overrides/removals, and parent-state -preservation have process regressions; a native Go pre-push hook verifies public -publication from another directory. Both new regressions failed before the fix -and pass afterward. Process, Go-tool, and Git suites passed under `-race`; -public hook and original-object source/build eligibility regressions passed. - -**Relevant code:** `run` in -[internal/git/remote.go](../../internal/git/remote.go), lines 233–240; -`newCommand` and `environmentEntries` in -[internal/process/process.go](../../internal/process/process.go), lines 146–184. - -**Circumstances and actual behavior:** A caller invokes `Publish` for a checkout -other than its current directory, or invokes the CLI from a selected module -subdirectory. The new `GIT_NO_REPLACE_OBJECTS` overlay makes Git's command -environment explicit. The existing environment helper copies `os.Environ()` -before applying that overlay, retaining the parent process's `PWD`. The local -pre-push hook executes in the selected repository, while `PWD` still names the -caller's directory. Previously, this Git runner left `Cmd.Env` nil, allowing -Go to derive the child `PWD` from `Cmd.Dir` on POSIX systems. - -An uncached public `Publish` probe used a real local bare remote and a Python -pre-push hook that compares `PWD` with its actual working directory. The hook -rejected the push, leaving publication `not_published` and the local tag retained. -An otherwise identical temporary comparison overlay, removing only the Git -runner's newly added environment assignment, published successfully. The -comparison isolates environment construction as the regression's cause. - -**Expected behavior and impact:** Selecting another checkout preserves the -existing working-directory behavior of Git collaborators while adding the -original-object setting. Hooks that locate repository files through `PWD` can -now inspect the wrong checkout or reject a legitimate publication. The observed -failure is attributable to the new Git overlay exposing the older environment -helper's directory handling. - -**Recommended remediation:** Build environment overlays from `command.Environ()` -after assigning `command.Dir`, then apply the requested removals and overrides. -This retains Go's directory-aware inherited environment and still honors an -explicit `PWD` override or removal. Keep `GIT_NO_REPLACE_OBJECTS=1` and the -existing local-object transport controls. Preserve process-global state. Add -focused process coverage for `Dir` combined with an unrelated overlay and a -public publication regression using a local hook from another caller directory. - -## R03. Checkout preparation reintroduces credentials after Git redaction - -**Priority:** P2. **Status:** Complete. This addressed an A04 integration gap in the -existing preparation workflow, separate from R01's URL-span parsing defect. - -**Completion:** Fetch-failure errors render the remote with `DisplayURL`, while -the fetch retains its original URL and wrapped Git cause. Public preparation -and CLI regressions failed before the fix and now pass with local HTTP servers; -the public test also verifies authentication with the configured credentials. -The remaining owned Git URL renderers were reviewed, and the CLI guide describes -the implemented diagnostic behavior. Combined verification below passed. - -**Relevant code:** `PrepareTaggedCheckout` in -[internal/git/prepare.go](../../internal/git/prepare.go), lines 18–20; the public -[preparation facade](../../preparation.go); and error presentation in -`runCIDelivery` in [cmd/releasekit/main.go](../../cmd/releasekit/main.go). - -**Circumstances and actual behavior:** `TaggedCheckoutRequest.Remote` supplies -an HTTP(S) repository URL with userinfo, and fetching its tag fails. The shared -Git runner sanitizes its captured diagnostic, but `PrepareTaggedCheckout` -formats the original `remote` directly into its outer error. The facade returns -that error unchanged; `ci-deliver` prints it directly. - -An uncached public preparation probe used placeholder credentials and a local -HTTP server returning 503. The resulting error began: - -```text -fetch tag v1.2.3 from remote http://review-user:review-password@127.0.0.1:PORT/repo.git: run "git": exit status 128 -``` - -Git's own diagnostic in that reproduction omitted userinfo. The enclosing -ReleaseKit error exposed it. Ordinary username/password credentials reproduce -this issue. - -**Expected behavior and impact:** The preparation workflow uses the same safe -URL rendering as publication. As implemented, failed CI preparation or direct -facade use can expose credentials to terminal output, callers, and logs despite -the shared runner's new redaction. - -**Recommended remediation:** Render the remote through `DisplayURL` in the -preparation error and check the remaining owned URL-formatting boundaries for -the same bypass. Preserve the actual fetch URL and wrapped Git cause. Add a -public preparation failure regression with a local server and placeholder -credentials, and cover the CLI error path through its existing presentation -tests. Credential-bearing transport URLs keep their current supported behavior. - -## Review coverage and verification - -The review covered all A01–A20 outcomes and the changed production code, tests, -and current documentation. The implementation retains the root orchestration -facade and lightweight `buildinfo` facade. Internal Git/process operations own -source observations and subprocess lifetime; validation owns its Go and tree -sequences; build settings have one internal representation; artifact declaration -matching is shared; neutral delivery policy remains separate from Gitea protocol -operations; and private CLI handlers own presentation. - -R01–R03 record findings and remediation status from the initial review and -follow-up integration audit. Package ownership and public facade shape match -the target; the findings concern diagnostic and subprocess-environment behavior -across existing boundaries. The review identified no inadvertently strengthened public -contract. The selected redirect and diagnostic limits match the accepted scope. -JSON response caps and production exclusion indexing remain explicitly deferred -with assessment evidence, as planned. - -During the initial review on Linux/amd64 with Go 1.26.8, these checks passed with -`GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off`: - -- `go test ./...` -- `go test -race ./...` -- `go vet ./...` -- `go run ./internal/apicheck` - -Some test results were reused from Go's cache. The suite used `/var/tmp` for -temporary fixtures because an environment-provided `/tmp/.git` marker affects -standalone repository discovery. The R01 probes ran uncached through a temporary -Go test overlay and failed their credential-absence assertions on the reviewed -implementation; no probe test was installed in the checkout. Those failures -substantiate R01 and are separate from the passing committed suite. - -### Follow-up integration evidence - -The follow-up audit traced original-object observations through candidate, -build, checkout preparation, and publication; subprocess cancellation through -batch readers and committed-tree validation; completed build facts through CLI -receipts and prebuilt delivery; cleanup errors through publication/delivery -outcomes and CLI statuses; and redirect/content limits through read-only asset -inspection and attempted-write reconciliation. It also checked status-only -output capture against full-output consumers and existing forwarding errors. - -Focused committed tests ran uncached with the same offline Go settings. Public -and CLI cases covered exact publication, rejected/lost-acknowledgment recovery, -partial tag evidence, replacement-derived build eligibility, original-note -delivery, managed build cleanup/cancellation, receipt-to-prebuilt delivery, -CI preflight ordering, and completed-delivery cleanup reporting. Selected tests -in `internal/process`, `internal/git`, `internal/gotool`, `internal/delivery`, -and `internal/validation` passed under `-race`, including queued cancellation, -inherited-pipe cancellation, forwarding errors, joined cleanup outcomes, -bounded asset reads, upload reconciliation, and tree-reader cleanup precedence. - -R02 and R03 were reproduced independently with temporary Go test overlays. -Their assertions failed against the reviewed implementation; R02's comparison -using the earlier Git environment behavior passed. Both audits left production -code and existing tests unchanged. The full-suite, static, and compiled -API/dependency results above record the audit baseline; completion notes record -verification of the subsequent repairs. - -## Completed remediation verification - -All three findings are complete. Each fix includes regressions that reproduced -the defect before the production change, and each unit has its own commit with -the corresponding completion marker. - -On Linux/amd64 with Go 1.26.8, combined verification passed with -`GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off`, the prepared build cache, and -`TMPDIR=/var/tmp` for test fixtures: - -- `go test ./...` -- `go test -race ./...` -- `go run ./internal/apicheck` - -Changed Go files passed formatting inspection. Local links and fragments in -this document and the CLI guide resolved, and the diff passed whitespace checks.