Prepare v0.7.0 notes and retire completed roadmaps
This commit is contained in:
106
docs/releases/v0.7.0.md
Normal file
106
docs/releases/v0.7.0.md
Normal file
@@ -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)
|
||||
@@ -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 |
|
||||
@@ -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=<nil>
|
||||
```
|
||||
|
||||
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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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: <nil>
|
||||
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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
@@ -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.
|
||||
67
docs/roadmap/deferred-resources.md
Normal file
67
docs/roadmap/deferred-resources.md
Normal file
@@ -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.
|
||||
@@ -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/<tag>` 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.
|
||||
@@ -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.
|
||||
Reference in New Issue
Block a user