Record forge delivery review findings
This commit is contained in:
166
docs/roadmap/remediation.md
Normal file
166
docs/roadmap/remediation.md
Normal file
@@ -0,0 +1,166 @@
|
||||
# Remediation: forge boundary and credential preflight review
|
||||
|
||||
Status: open; the findings below are unaddressed.
|
||||
|
||||
## Review baseline and scope
|
||||
|
||||
Reviewed `cbdba90`, including the implementation commits after `9d6952a`,
|
||||
against the [feature roadmap](forge-boundary.md),
|
||||
[implementation plan](implementation.md), and the user's selected scope.
|
||||
This document records work identified by the review; it does not implement it.
|
||||
|
||||
The intended production boundary is present. The root facade owns neutral
|
||||
destination selection and public preflight errors; `internal/forge` supplies
|
||||
the backend contract; `internal/delivery` owns page/asset policy and recovery;
|
||||
`internal/gitea` owns protocol operations; and `internal/artifact` owns local
|
||||
declarations and content verification. Gitea remains the implemented backend.
|
||||
Automatic and standalone preflight use read-only repository requests, and binary
|
||||
CI checks access before dependency preparation. Source publication retains its
|
||||
separate Git authentication and completion semantics.
|
||||
|
||||
## 1. Complete local destination preflight error handling — P2
|
||||
|
||||
Status: open.
|
||||
|
||||
**Evidence:** [Gitea preflight](../../internal/gitea/backend.go),
|
||||
[public error conversion](../../delivery.go), and
|
||||
[CLI error handling](../../cmd/releasekit/main.go).
|
||||
|
||||
`Backend.Preflight` returns `destinationAPI` validation errors directly. Those
|
||||
errors are neutral `forge.Failure` values with `FailureInvalid`, rather than
|
||||
classified preflight failures. `publicPreflightError` consequently passes them
|
||||
through. Both CLI delivery paths treat the resulting unclassified error as
|
||||
status 1.
|
||||
|
||||
A temporary consumer with an otherwise valid tagged source and a nonblank
|
||||
synthetic token reproduced this with `base_url: "ssh://forge.example"`:
|
||||
`PreflightDelivery` returned `*forge.Failure`, `errors.As` could not obtain a
|
||||
`*DeliveryPreflightError`, and `deliver` exited 1. A malformed owner/repository
|
||||
segment follows the same local validation path. No server request is needed to
|
||||
identify these configuration problems.
|
||||
|
||||
This leaves the public preflight error description incomplete and conflicts
|
||||
with the documented status 2 for profile/configuration failures. Operators
|
||||
cannot reliably distinguish a locally invalid destination from an operational
|
||||
verification failure through the new boundary.
|
||||
|
||||
**Required work:**
|
||||
|
||||
- Make deterministic destination-setting failures recognizable through the
|
||||
facade and classify them as CLI configuration failures with status 2 in both
|
||||
ordinary and CI delivery. Keep backend-specific validation in its adapter;
|
||||
share its classification instead of duplicating Gitea rules in the CLI.
|
||||
- Make GoDoc and the selected public error representation agree about local
|
||||
configuration failures. Preserve useful causes and identify the invalid
|
||||
setting; avoid labeling a malformed URL as a rejected token or an unavailable
|
||||
service. Keep real service/transport failures at status 1.
|
||||
- Add focused coverage for an invalid URL and invalid repository addressing,
|
||||
including standalone preflight and the CLI mapping. Verify early failure,
|
||||
`not_attempted` delivery state, and absence of hosted requests/build work.
|
||||
|
||||
This correction concerns existing destination rules and failure classification;
|
||||
it adds no token-format restriction, remote permission requirement, or probe.
|
||||
|
||||
## 2. Protect the runtime boundary from the new forge contract — P2
|
||||
|
||||
Status: open.
|
||||
|
||||
**Evidence:** [dependency checker](../../internal/apicheck/main.go), especially
|
||||
`forbiddenRuntimeDependency`, and the runtime boundary in
|
||||
[architecture policy](../policy/architecture.md#runtime-version-boundary).
|
||||
Implementation Stage 9 also calls for keeping the new delivery packages out of
|
||||
the runtime dependency graph through the existing checking mechanism.
|
||||
|
||||
The checker excludes named delivery implementations such as `gitea` and
|
||||
`delivery`, but its runtime exclusion list omits the newly added `forge`
|
||||
package. A temporary test overlay supplied this package graph:
|
||||
|
||||
```text
|
||||
buildinfo -> internal/forge
|
||||
```
|
||||
|
||||
`checkImportDirections` returned nil. The graph would make the runtime facade
|
||||
depend on the release backend contract while the advertised boundary check
|
||||
continued to pass. The actual current `buildinfo` graph remains appropriately
|
||||
small; the gap is in the planned regression protection.
|
||||
|
||||
**Required work:**
|
||||
|
||||
- Extend the existing runtime check to exclude the new forge delivery contract.
|
||||
Retain the existing policy for legitimate runtime dependencies.
|
||||
- Add a focused fixture showing rejection of a runtime dependency on
|
||||
`internal/forge`, including traversal through a small intermediary where
|
||||
needed to protect the existing transitive rule. Retain the passing actual
|
||||
repository check.
|
||||
|
||||
This enforces the already selected separation from orchestration. It does not
|
||||
require a new public package restriction or a broader dependency policy.
|
||||
|
||||
## 3. Qualify cancellation exit-status documentation — P3
|
||||
|
||||
Status: open.
|
||||
|
||||
**Evidence:** [CLI exit statuses](../cli.md),
|
||||
[v0.6.0 release note](../releases/v0.6.0.md),
|
||||
[write evidence](../../internal/gitea/backend.go),
|
||||
[asset reconciliation](../../internal/delivery/remote_assets.go), and
|
||||
[CLI delivery mapping](../../cmd/releasekit/main.go).
|
||||
|
||||
The new CLI prose and release note broadly state that cancellation returns
|
||||
status 1. That is correct for preflight and preparation cancellation. After a
|
||||
possibly successful write, cancellation can prevent reconciliation and leave
|
||||
delivery unknown; the CLI correctly returns status 3 in that case.
|
||||
|
||||
A temporary shared-delivery test supplied an attempted uncertain upload with
|
||||
`context.Canceled` and an unavailable canceled reconciliation. It returned
|
||||
`State == Unknown`, retained the cancellation cause, and placed the asset in
|
||||
`Uncertain`. The CLI's unknown-state branch maps that result to 3.
|
||||
|
||||
The broad documentation introduces a stronger cancellation guarantee than the
|
||||
requested preflight behavior and conflicts with preserved write recovery.
|
||||
|
||||
**Required work:**
|
||||
|
||||
- Scope status 1 wording to preflight/preparation cancellation and cancellation
|
||||
that leaves no unresolved write. State that an unresolved write retains
|
||||
status 3, including when cancellation prevented verification.
|
||||
- Align the CLI reference and unreleased v0.6.0 note. Keep implementation
|
||||
behavior and unknown-write recovery intact.
|
||||
- Reuse existing cancellation/recovery tests. If CLI coverage does not already
|
||||
distinguish these cases, add a narrow case protecting unknown-write status
|
||||
after cancellation; avoid an expanded cancellation matrix.
|
||||
|
||||
## 4. Align roadmap completion records — P3
|
||||
|
||||
Status: open.
|
||||
|
||||
**Evidence:** [feature roadmap](forge-boundary.md) and
|
||||
[implementation plan](implementation.md).
|
||||
|
||||
The implementation plan marks all ten stages complete, while the feature
|
||||
roadmap still says implementation has not started. The plan's introductory
|
||||
baseline text also uses present-tense descriptions of the old implementation
|
||||
and says the neutral destination and preflight are planned.
|
||||
|
||||
**Required work:** Mark the feature roadmap's implementation status accurately
|
||||
and clearly label the implementation plan's original baseline as historical.
|
||||
Keep these records consistent with the completed stages and remaining
|
||||
remediation. Roadmap retirement can follow the existing release-preparation
|
||||
cleanup; this finding does not require removing plans now.
|
||||
|
||||
## Verification performed
|
||||
|
||||
Using Go 1.26.10 on linux/amd64 with module/toolchain downloads disabled:
|
||||
|
||||
- `go test ./...` passed.
|
||||
- `go run ./internal/apicheck` passed for the current repository.
|
||||
- `go test -race ./internal/delivery ./internal/gitea` passed.
|
||||
- Temporary review fixtures reproduced findings 1 and 2, and confirmed the
|
||||
cancellation outcome underlying finding 3. The runtime-boundary reproduction
|
||||
intentionally failed its assertion because the prohibited graph was accepted.
|
||||
- An additional temporary HTTP/1.1 and HTTP/2 check confirmed that a large upload
|
||||
rejected before its body was consumed returns promptly as a known rejection;
|
||||
that investigated path produced no finding.
|
||||
|
||||
Temporary fixtures and test overlays were kept outside the checkout. No
|
||||
implementation files or permanent tests were changed during this review.
|
||||
Reference in New Issue
Block a user