diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..29fb380 --- /dev/null +++ b/docs/roadmap/remediation.md @@ -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.