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