Plan remediation of correctness and interface audit findings
This commit is contained in:
285
docs/roadmap/remediation.md
Normal file
285
docs/roadmap/remediation.md
Normal file
@@ -0,0 +1,285 @@
|
||||
# Audit remediation roadmap
|
||||
|
||||
## Status and baseline
|
||||
|
||||
Proposed on 2026-11-01 against `899e7f1`. Implementation has not started.
|
||||
|
||||
This roadmap consolidates all three findings in the
|
||||
[functional correctness audit](correctness.md) and all five findings in the
|
||||
[interface audit](interfaces.md). Those documents retain the reproduction
|
||||
circumstances and evidence. This document owns the proposed corrections,
|
||||
implementation sequence, and completion criteria.
|
||||
|
||||
The baseline has separate orchestration and runtime facades, internal candidate,
|
||||
validation, build, preparation, and publication services, and shared delivery
|
||||
policy behind the neutral forge boundary. The corrections fit those existing
|
||||
owners. The ordinary and race-enabled offline suites and compiled boundary
|
||||
checks passed during the audits; they do not cover all identified defects.
|
||||
|
||||
## Targeted end state
|
||||
|
||||
- Incomplete authoritative tag observations retain an unknown publication
|
||||
outcome after a failed push acknowledgment.
|
||||
- Dependency preparation uses ordinary module discovery from its selected
|
||||
directory despite an inherited or persistent `GOFLAGS=-modfile=...` setting.
|
||||
- Candidate checking establishes that its derived release tag is a valid Git
|
||||
ref before accepting it as an unused local tag.
|
||||
- Cancellation during build source inspection remains identifiable through
|
||||
the returned error chain.
|
||||
- Asset operations classify a missing service-provided release identity as a
|
||||
malformed observation. Existing page-only reads retain their current minimal
|
||||
response requirements.
|
||||
- Preparation path anchoring is explicit in the public documentation. The
|
||||
recommended interpretation retains the supplied-directory anchor; the
|
||||
alternative is discussed under Open Decisions.
|
||||
- Delivery documentation accurately describes identity requirements for empty
|
||||
prebuilt results and credential preflight before full artifact verification.
|
||||
|
||||
These corrections preserve the existing public types, receipt schema, release
|
||||
identity conventions, and publication/delivery model. Work stays focused on the
|
||||
reported cases: Git supplies ref-name rules; local builds retain their current
|
||||
filesystem choices; preparation retains network access and caller operational
|
||||
settings; and page-only reads retain their current field requirements. Existing
|
||||
source-verification observations and preflight evidence limits continue to
|
||||
apply.
|
||||
|
||||
## Finding coverage
|
||||
|
||||
| Audit finding | Work unit |
|
||||
| --- | --- |
|
||||
| Correctness 3: incomplete reconciliation becomes conflict | 1 |
|
||||
| Correctness 2: ambient module-file redirection | 2 |
|
||||
| Correctness 1: invalid derived tag accepted by Check | 3 |
|
||||
| Interfaces 1: source-inspection cancellation identity | 4 |
|
||||
| Interfaces 2: unusable successful release handle | 5 |
|
||||
| Interfaces 3: preparation repository anchor | 6 |
|
||||
| Interfaces 4: empty-prebuilt GoDoc | 7 |
|
||||
| Interfaces 5: hashing before every hosted request | 8 |
|
||||
|
||||
## Implementation sequence
|
||||
|
||||
Each unit includes its relevant validation and canonical documentation updates.
|
||||
Complete the units as focused changes using the repository's ordinary commit
|
||||
policy. Record completion here and update the corresponding audit finding's
|
||||
status when its correction is verified. The sequence requires local repository
|
||||
work and automated checks; it adds no pilot, intermediate release, or external
|
||||
review step.
|
||||
|
||||
### 1. Preserve uncertainty for incomplete remote tag observations
|
||||
|
||||
**Status:** open. **Source:** correctness finding 3.
|
||||
|
||||
**Owners:** `internal/git` ref parsing and `internal/publish` recovery, with CLI
|
||||
outcome verification.
|
||||
|
||||
Finalize tag observations after the entire query response has been parsed.
|
||||
A peeled record without its exact tag-object record is an inconsistent or
|
||||
unreadable observation. Use the existing ref-state/error vocabulary so
|
||||
publication reconciliation retains `PublicationUnknown` and the original push
|
||||
cause. Keep valid records usable in either order; a peeled record may precede
|
||||
its exact tag record.
|
||||
|
||||
**Completion evidence:** a local Git recovery fixture performs the intended
|
||||
push, loses its acknowledgment, and supplies only the peeled record. The public
|
||||
result is unknown, the cause remains inspectable, and the CLI uses status 3.
|
||||
Existing exact-match, absent-tag, and different-object recovery outcomes still
|
||||
work, along with complete records in either order. Query finalization retains
|
||||
both genuinely absent results and valid exact-object-only results.
|
||||
|
||||
### 2. Override the redirecting module-file flag during preparation
|
||||
|
||||
**Status:** open. **Source:** correctness finding 2.
|
||||
|
||||
**Owners:** `internal/gotool` dependency preparation and its CLI integration.
|
||||
|
||||
Give `go mod download` an explicit empty `-modfile` argument. Command-line flag
|
||||
precedence overrides an inherited or persistent alternate module-file selection
|
||||
and restores Go's ordinary module discovery. A local Go 1.26.10 probe verified
|
||||
this behavior from both a module root and its package subdirectory, with each
|
||||
flag source.
|
||||
|
||||
This is narrower than replacing all `GOFLAGS` or disabling the persistent Go
|
||||
environment. Preserve the other effective flags and the current proxy,
|
||||
private-module, authentication, checksum, cache, workspace, and local-toolchain
|
||||
behavior. Preserve Go's existing checksum-update behavior and directory
|
||||
selection; this unit adds no requirement for an immediate `go.mod` in a supplied
|
||||
package directory.
|
||||
|
||||
**Completion evidence:** exercise both inherited and persistent `-modfile`
|
||||
settings against different module graphs. Preparation consumes the ordinary
|
||||
selected graph, including when Go discovers its declaration above the working
|
||||
directory. Verify the existing network/private-module/cache settings remain
|
||||
available and that unrelated flags retain their behavior. Use test-owned
|
||||
`GOMODCACHE` directories for every synthetic-module download. Keep the existing
|
||||
binary CI preparation ordering and cancellation/error propagation covered.
|
||||
|
||||
Document the preparation flag override in its GoDoc and the
|
||||
[Go environment guide](../internal/gotool.md). Update command fixtures for the
|
||||
intentional invocation change; use graph-selection behavior as the regression
|
||||
assertion rather than relying only on argument matching.
|
||||
|
||||
### 3. Validate derived tag names during candidate capture
|
||||
|
||||
**Status:** open. **Source:** correctness finding 1.
|
||||
|
||||
**Owners:** `internal/candidate` and the shared Git ref-validation boundary.
|
||||
|
||||
Apply the existing Git ref-name check to the complete derived tag before local
|
||||
tag absence is accepted during candidate capture. Keep name validity distinct
|
||||
from the existence query: exit status 1 from `show-ref` alone cannot establish
|
||||
both. Reuse the same rule at any candidate recheck boundary that needs it,
|
||||
without duplicating Git's character rules in ReleaseKit.
|
||||
|
||||
**Completion evidence:** public `Check` fails for derived tags containing the
|
||||
reported space and `.lock` directory components, before validation callbacks
|
||||
run. Ordinary root, nested, slash-containing, and supported major-version tags
|
||||
still work. Preserve the captured identity in failure results and existing
|
||||
publication guards. Standalone `Build` retains its current local-build behavior.
|
||||
|
||||
Update candidate guidance only as needed to explain the local failure; this
|
||||
unit introduces no new version syntax or blanket directory-name policy.
|
||||
|
||||
### 4. Preserve cancellation during build source-state inspection
|
||||
|
||||
**Status:** open. **Source:** interfaces finding 1.
|
||||
|
||||
**Owner:** `cleanSource` in `internal/build/build.go`.
|
||||
|
||||
When the Git status command fails and its operation context is canceled or has
|
||||
expired, retain that context error in the returned chain, following the
|
||||
neighboring build helpers or the existing process boundary. Ordinary Git errors
|
||||
retain their current useful diagnostics. Keep the partial build result intact.
|
||||
|
||||
**Completion evidence:** interrupt source inspection before compilation and
|
||||
after completed artifacts exist. `errors.Is` identifies cancellation, and a
|
||||
focused deadline case identifies deadline expiration. The latter build result
|
||||
retains its completed artifacts. Retain existing managed-delivery cancellation
|
||||
and temporary-output cleanup coverage; add an integration case only if the
|
||||
helper-level/public-build coverage leaves a distinct propagation risk.
|
||||
|
||||
### 5. Classify incomplete Gitea handles at asset use
|
||||
|
||||
**Status:** open. **Source:** interfaces finding 2.
|
||||
|
||||
**Owner:** Gitea backend handle interpretation in `internal/gitea/backend.go`.
|
||||
|
||||
Keep `FindRelease` usable for page-only reads with the existing minimal response
|
||||
fields. When an asset operation needs the positive release ID and a
|
||||
backend-produced handle lacks it, return the existing malformed-response
|
||||
classification with a useful missing-identity diagnostic. Distinguish this from
|
||||
a handle of the wrong type, which remains an invalid request. Apply the
|
||||
distinction consistently to listing and uploading; an upload rejected locally
|
||||
retains not-attempted write evidence.
|
||||
|
||||
**Completion evidence:** feed a lookup response with a missing, zero, or negative
|
||||
ID through the backend. Page-only matching still succeeds under its existing
|
||||
rules; binary asset use fails with the malformed classification before an
|
||||
asset request. Wrong-type handles remain invalid requests, valid IDs continue
|
||||
to work, and existing page/write-progress outcomes remain accurate. Retain
|
||||
`TestBackendFindReleaseDoesNotRequireWriteResponseFields` or equivalent
|
||||
behavioral coverage. Update the integration guide's error description as needed.
|
||||
|
||||
### 6. Clarify dependency preparation's path anchor
|
||||
|
||||
**Status:** open; targeted interpretation follows Open Decision 1.
|
||||
**Source:** interfaces finding 3.
|
||||
|
||||
**Owners:** `DependencyPreparationRequest` GoDoc and consumer preparation guidance;
|
||||
selection code only if the alternative is chosen.
|
||||
|
||||
Under the recommendation, document `RepositoryDir` as the supplied base
|
||||
directory for preparation, `ModuleDir` as relative to that base, and empty
|
||||
`ModuleDir` as selecting that base. Explain that callers sharing a location with
|
||||
other orchestration APIs should supply the resolved checkout root when they
|
||||
intend repository-root-relative selection. Keep standalone directory preparation
|
||||
and the CLI's existing root normalization.
|
||||
|
||||
**Completion evidence:** compare the wording and examples with current root,
|
||||
nested, symlink-resolved, and standalone-directory behavior. Reuse existing
|
||||
selection tests for a documentation-only correction. If anchoring behavior
|
||||
changes instead, add behavioral coverage for calls from within a checkout and
|
||||
for standalone preparation, including failed discovery behavior, and document
|
||||
the changed interpretation.
|
||||
|
||||
### 7. Clarify empty prebuilt results
|
||||
|
||||
**Status:** open. **Source:** interfaces finding 4.
|
||||
|
||||
**Owners:** `DeliveryRequest` GoDoc and
|
||||
[delivery consumer guidance](../consumers/delivery.md#go-api).
|
||||
|
||||
Describe a page-only prebuilt result as having no targets or artifacts while
|
||||
still identifying the same repository, module, tag, commit, and version.
|
||||
Present nil `Build` and nil `Prebuilt` as the ordinary page-only request.
|
||||
Keep the existing identity checks and matching-empty-result behavior.
|
||||
|
||||
**Completion evidence:** the exported GoDoc and guide agree with
|
||||
`validateBuildIdentity`, and no wording implies that `&BuildResult{}` is accepted.
|
||||
Review existing page-only and prebuilt identity coverage; this wording correction
|
||||
does not require a new zero-result compatibility path or additional tests.
|
||||
|
||||
### 8. Correct the documented artifact-verification order
|
||||
|
||||
**Status:** open. **Source:** interfaces finding 5.
|
||||
|
||||
**Owner:** the prebuilt paragraph in
|
||||
[delivery consumer guidance](../consumers/delivery.md#go-api).
|
||||
|
||||
Replace the claim that digest checks precede every hosted request with the
|
||||
selected order: cheap local/source checks, read-only credential preflight,
|
||||
full artifact verification, then hosted writes. Keep the concise statement that
|
||||
local verification completes before hosted writes consistent across GoDoc and
|
||||
current guidance.
|
||||
|
||||
**Completion evidence:** affected descriptions agree with the implementation,
|
||||
[ADR 0006](../adr/0006-neutral-forge-delivery-and-preflight.md), and existing
|
||||
`TestAutomaticPreflightFailurePrecedesPrebuiltArtifactHashing` coverage. This
|
||||
unit changes wording and preserves the current operation order.
|
||||
|
||||
## Final verification and completion
|
||||
|
||||
Run focused regression checks with each behavioral correction. Once the units
|
||||
are complete, run the ordinary offline suite and compiled boundary check:
|
||||
|
||||
```sh
|
||||
GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go test ./...
|
||||
GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off go run ./internal/apicheck
|
||||
```
|
||||
|
||||
Use race-enabled checks for affected subprocess/cancellation and adapter paths
|
||||
where the change warrants them. The audit's existing full race-suite result is
|
||||
baseline evidence; repeat broader checks when implementation changes or failures
|
||||
justify it. Keep fixtures local, deterministic, and independent of real
|
||||
credentials, with isolated synthetic module caches.
|
||||
|
||||
Review public errors/results, CLI statuses, and canonical documentation against
|
||||
the completion evidence above. Validate affected links and prose whitespace.
|
||||
Mark this roadmap complete when all eight work units and their corresponding
|
||||
audit findings have been addressed and verified, including the selected path-anchor
|
||||
interpretation. The initial pass that creates this roadmap changes only this
|
||||
document.
|
||||
|
||||
## Open Decisions
|
||||
|
||||
### 1. Dependency preparation's repository anchor
|
||||
|
||||
**Recommendation:** retain the supplied `RepositoryDir` as the preparation base
|
||||
and clarify its meaning in GoDoc and consumer guidance. This corrects the
|
||||
ambiguity without changing existing selection behavior, preserves standalone
|
||||
preparation, and keeps the current CLI experience. The tradeoff is that API
|
||||
callers reusing a directory within a checkout must supply its resolved root
|
||||
when composing root-relative preparation with `Check`, `Build`, or `Deliver`.
|
||||
This is a recommended resolution, not a claim that the audit established a
|
||||
requirement to retain that difference.
|
||||
|
||||
**Viable alternative:** resolve the containing checkout root when the supplied
|
||||
directory belongs to a Git checkout, and preserve supplied-directory preparation
|
||||
for standalone use. This aligns ordinary checkout-based calls with the other
|
||||
facades and reduces caller normalization. It changes the interpretation of
|
||||
existing calls from checkout subdirectories and adds discovery behavior. The
|
||||
implementation would need to distinguish standalone use from discovery failures
|
||||
without silently treating every Git error as permission to use another anchor.
|
||||
Adopting it would require focused selection tests and updated consumer guidance.
|
||||
|
||||
A Git-checkout prerequisite is unnecessary for either correction; the current
|
||||
standalone use remains part of the baseline considered by both choices.
|
||||
Reference in New Issue
Block a user