236 lines
12 KiB
Markdown
236 lines
12 KiB
Markdown
# Audit remediation roadmap
|
|
|
|
## Status and scope
|
|
|
|
The planned remediation was implemented against baseline `59bb074`. A
|
|
post-implementation review of `6796310` found two follow-up issues below;
|
|
remediation remains open until they are addressed. The implementation plan
|
|
records the original stage and validation evidence.
|
|
|
|
This roadmap defines the desired behavior and scope for all three findings in
|
|
the [functional correctness audit](correctness.md) and all five findings in the
|
|
[interface audit](interfaces.md). Those documents retain their reproduction
|
|
circumstances and evidence. The [implementation plan](implementation.md) owns
|
|
staging, code changes, regression checks, and completion tracking.
|
|
|
|
## Purpose and architectural scope
|
|
|
|
ReleaseKit owns routine release mechanics so consumers supply product facts and
|
|
product-specific checks with little release-related code. As recorded in
|
|
[ADR 0002](../adr/0002-routine-releases.md), internal complexity is justified
|
|
when it reduces caller-owned release work. In particular, callers can reuse a
|
|
repository location across preparation and subsequent orchestration without
|
|
normalization code or another configuration option.
|
|
|
|
Preparation owns checkout-root resolution when the supplied location is inside
|
|
a checkout. Ordinary root-module callers keep the same request shape; callers
|
|
selecting a nested module supply its repository-relative `ModuleDir`.
|
|
Standalone preparation retains the supplied directory as its base. This is the
|
|
selected approach to the preparation-anchor question.
|
|
|
|
Apply the same ownership principle to the other corrections: ReleaseKit
|
|
validates derived tags, prepares the selected module's dependencies, preserves
|
|
cancellation and uncertain publication outcomes, and classifies backend
|
|
observations. Consumers continue to use the existing operations and outcomes.
|
|
The empty-prebuilt and verification-order findings call for documentation that
|
|
accurately explains existing behavior. Evaluate each correction by whether an
|
|
ordinary consumer can use it without additional release code or configuration.
|
|
|
|
The existing public orchestration and runtime facades and internal package
|
|
boundaries remain appropriate. These corrections use the existing public types,
|
|
receipt schema, module/tag conventions, and publication/delivery model.
|
|
|
|
## Desired behavior
|
|
|
|
### Incomplete publication observations
|
|
|
|
After a failed push acknowledgment, a query containing only a peeled tag record
|
|
provides insufficient evidence about the exact annotated tag object.
|
|
Publication remains unknown and retains the original push cause. A complete
|
|
response works in either record order. Existing exact-object matches, genuinely
|
|
absent tags, different objects, and exact-object-only responses retain their
|
|
current outcomes.
|
|
|
|
### Dependency preparation and ambient module files
|
|
|
|
Preparation uses ordinary Go module discovery from its selected directory even
|
|
when inherited or persistent `GOFLAGS` selects an alternate module file. It
|
|
retains other effective flags and existing proxy, private-module,
|
|
authentication, checksum, cache, workspace, and local-toolchain behavior.
|
|
Go retains its existing checksum-update behavior, including upward declaration
|
|
discovery for standalone package-directory preparation.
|
|
|
|
### Valid candidate tags
|
|
|
|
Candidate checking establishes that the complete derived release tag is a valid
|
|
Git ref before accepting it as unused. Git's own ref rules remain the authority.
|
|
The correction concerns release candidate validity; standalone local builds
|
|
retain their existing filesystem choices and version rules.
|
|
|
|
### Identifiable build cancellation
|
|
|
|
Cancellation or deadline expiration during build source inspection remains
|
|
identifiable through the error chain. Ordinary Git failures retain useful
|
|
diagnostics, and partial results retain already completed artifacts.
|
|
|
|
### Gitea release identity at asset use
|
|
|
|
A backend-produced handle with a missing or nonpositive service release ID is a
|
|
malformed observation when an asset operation needs that ID. A handle of the
|
|
wrong type remains an invalid request. Locally rejected uploads retain
|
|
not-attempted write evidence.
|
|
|
|
Existing page-only reads retain their current minimal response requirements.
|
|
This correction adds no universal positive-ID or page-URL requirement to a
|
|
successful release lookup and preserves existing delivery progress semantics.
|
|
|
|
### Consistent preparation paths
|
|
|
|
Within a Git checkout, preparation resolves its containing root automatically
|
|
and interprets `ModuleDir` relative to that root. Empty selection means the
|
|
root module, consistently with `Check`, `Build`, and `Deliver`. This includes
|
|
calls made from checkout subdirectories and filesystem aliases.
|
|
|
|
Outside a Git checkout, preparation retains the supplied directory as its base
|
|
and the existing relative module selection. Standalone use remains supported.
|
|
It retains its existing Go-only use when Git is unavailable and the directory
|
|
can be identified as outside a checkout.
|
|
Discovery distinguishes this case from cancellation, permission failures, and
|
|
other Git errors, which remain failures. Existing symlink and containment rules
|
|
and the CLI's root normalization continue to apply.
|
|
|
|
### Empty prebuilt results
|
|
|
|
Page-only callers ordinarily omit both `Build` and `Prebuilt`. A supplied
|
|
prebuilt result with no targets or artifacts still identifies the matching
|
|
repository, module, tag, commit, and version. Documentation states that
|
|
requirement clearly. A zero `BuildResult` gains no new compatibility behavior.
|
|
|
|
### Artifact-verification order
|
|
|
|
Documentation consistently describes cheap local/source checks, read-only
|
|
credential preflight, full local artifact verification, and then hosted writes.
|
|
Digest verification completes before hosted writes. Credential preflight can
|
|
precede that verification and confirms only the access evidence exposed by the
|
|
backend.
|
|
|
|
## Scope boundaries
|
|
|
|
The corrections retain the current source-verification observations, bounded
|
|
publication recovery, explicit forge selection, and additive hosted delivery
|
|
policy. They introduce no new public configuration knobs, compatibility shims,
|
|
release procedure, pilot release, or intermediate human-review requirement.
|
|
The targeted sprint reaches this complete end state through local repository
|
|
work and automated validation.
|
|
|
|
## Post-implementation review findings
|
|
|
|
The review compared implementation commits `0632aa5` through `6796310` with the
|
|
agreed target and the plan at `730e638`. It examined production changes, new
|
|
regressions, current documentation, and the final package boundaries. The
|
|
following findings require follow-up; the other planned corrections are present.
|
|
|
|
### 1. Damaged checkout metadata can silently select standalone preparation
|
|
|
|
**Priority:** medium. **Status:** complete.
|
|
|
|
**Completion evidence:** `TestPrepareBuildDependenciesRejectsDamagedCheckout`
|
|
verifies healthy root selection, preservation of the damaged checkout's Git
|
|
failure with no Go invocation, and genuine standalone preparation. Git discovery
|
|
regressions cover ceiling directories, physical and literal aliases, and the
|
|
filesystem boundary. Focused preparation and discovery checks pass offline with
|
|
the race detector. The implementation plan now requires marker inspection before
|
|
accepting the recognized no-repository diagnostic.
|
|
|
|
**Code:** `DiscoverRepositoryRoot` in
|
|
[internal/git/source.go](../../internal/git/source.go), particularly the
|
|
recognized no-repository branch, and public `PrepareBuildDependencies` in
|
|
[preparation.go](../../preparation.go).
|
|
|
|
The discovery code treats exit status 128 and the recognized no-repository
|
|
diagnostic as conclusive evidence of standalone use. Git emits that same
|
|
diagnostic when a checkout's `.git` directory remains present but its `HEAD`
|
|
file is missing. The marker inspection used when Git is unavailable does not
|
|
run on this branch. A broken `.git` file has coverage, but this damaged-directory
|
|
case does not.
|
|
|
|
**Reproduction:** a temporary public-API probe created and committed root and
|
|
child modules, then called preparation from the child with empty `ModuleDir`.
|
|
With healthy metadata it selected the root. After moving `.git/HEAD` out of the
|
|
disposable checkout, the same request returned nil and ran Go in the child.
|
|
A local Go recording shim observed the directory without downloading anything.
|
|
|
|
**Expected:** damaged checkout discovery fails before dependency preparation;
|
|
the request does not silently acquire a different selection base. This follows
|
|
the agreed treatment of broken metadata and other discovery failures.
|
|
|
|
**Actual consequence:** in a multi-module repository, metadata damage can make
|
|
an unchanged request prepare another module's graph and report success. This
|
|
is a remaining gap in checkout selection and failure handling.
|
|
|
|
**Correction:** establish that the no-repository result represents standalone
|
|
use before accepting it as a fallback. Account for relevant existing checkout
|
|
metadata and preserve the Git failure when that metadata is damaged. Keep Git's
|
|
normal discovery boundaries and genuine standalone/Go-only use; do not turn
|
|
this into a general directory-name restriction or a new public option. Add a
|
|
real damaged-`.git`-directory regression through the public preparation facade,
|
|
asserting an error and no Go invocation, alongside healthy and standalone
|
|
controls. The original plan's diagnostic-only classification also needs this
|
|
refinement; reproducing its algorithm alone is insufficient completion evidence.
|
|
|
|
### 2. The deadline regression starts its timeout before inspection readiness
|
|
|
|
**Priority:** low. **Status:** open.
|
|
|
|
**Code:** `TestBuildSourceInspectionCancellation/deadline` in
|
|
[build_source_cancellation_test.go](../../build_source_cancellation_test.go).
|
|
|
|
The deadline context starts with a three-second timeout before public `Build`
|
|
resolves the repository and module and reaches `cleanSource`. The test waits
|
|
for the inspection wrapper's readiness marker afterward, but that marker does
|
|
not control when expiration begins. The separate 45-second readiness watchdog
|
|
cannot prevent this earlier deadline from expiring during unrelated setup.
|
|
|
|
**Reproduction:** running only this subtest with a temporary delegating Git
|
|
wrapper that delays `rev-parse --show-toplevel` by four seconds fails with
|
|
`build ended before inspection` and a repository-resolution deadline error.
|
|
The test never exercises source inspection. The ordinary suite passes without
|
|
the injected startup delay.
|
|
|
|
**Expected:** the deadline regression exercises expiration during the blocked
|
|
source-inspection command, as required by the plan's synchronized-readiness
|
|
case. Unrelated setup latency does not determine whether that path is tested.
|
|
|
|
**Actual consequence:** a slower runner can fail the default suite for timing
|
|
outside the behavior being tested. This is a test reliability gap; the probe
|
|
does not demonstrate another production cancellation defect.
|
|
|
|
**Correction:** make the expiration case readiness-controlled, or isolate it at
|
|
the source-inspection helper boundary so earlier repository/module work cannot
|
|
consume its deadline. Retain public cancellation and partial-artifact coverage,
|
|
bounded cleanup, and the `errors.Is(context.DeadlineExceeded)` assertion.
|
|
Increasing the whole-operation timeout alone does not establish the intended
|
|
inspection-specific synchronization. Verify the revised case under delayed
|
|
unrelated setup as well as normal and race-enabled execution.
|
|
|
|
## Review validation and scope assessment
|
|
|
|
The ordinary offline `go test ./...` suite and compiled
|
|
`go run ./internal/apicheck` check passed on the reviewed code with
|
|
`GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off`. Race-enabled Git, Gitea, Go-tool, and
|
|
candidate package checks also passed. The temporary delayed-startup test above
|
|
failed as expected and substantiated finding 2; it is separate from the ordinary
|
|
passing suite. Focused public preparation, publication, cancellation, delivery,
|
|
and CLI regressions also passed with the race detector. Probes used disposable
|
|
local repositories and command wrappers, with no production changes or new
|
|
committed tests.
|
|
|
|
The root orchestration facade, separate runtime facade, neutral forge boundary,
|
|
and Gitea adapter retain their responsibilities. Ref finalization, candidate tag
|
|
validation, cancellation error propagation, module-file override, and asset-use
|
|
failure classification follow the selected approach. The review found no
|
|
additional public types, configuration knobs, receipt changes, stronger page-only
|
|
response requirements, or broader source/preflight guarantees introduced by the
|
|
implementation. The two findings concern incomplete realization of the agreed
|
|
behavior and regression reliability rather than an intentional scope expansion.
|