diff --git a/delivery_causes_test.go b/delivery_causes_test.go new file mode 100644 index 0000000..5e3a3b0 --- /dev/null +++ b/delivery_causes_test.go @@ -0,0 +1,63 @@ +package releasekit_test + +import ( + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "runtime" + "strings" + "testing" + + releasekit "go.maximumdirect.net/releasekit" +) + +func TestDeliveryPreservesAssetInspectionCauses(t *testing.T) { + repository := newTaggedDeliveryConsumer(t) + prebuilt, err := releasekit.Build(context.Background(), releasekit.BuildRequest{ + RepositoryDir: repository, Version: "v1.2.3", OutputDir: t.TempDir(), + Spec: releasekit.BuildSpec{MainPackage: "./cmd/app", BinaryName: "fixture", Targets: []releasekit.Target{{OS: runtime.GOOS, Arch: runtime.GOARCH}}}, + }) + if err != nil { + t.Fatal(err) + } + filename := prebuilt.Artifacts[0].Filename + for _, cause := range []error{context.Canceled, io.ErrUnexpectedEOF} { + t.Run(cause.Error(), func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + state := &facadeDeliveryServer{data: facadeDeliveryData{ + t: t, commit: prebuilt.CandidateCommit, tag: prebuilt.Tag, version: prebuilt.Version, + noteBody: "# Fixture v1.2.3\n\nDelivery fixture.\n", + pages: map[string]bool{prebuilt.Tag: true}, assets: map[string][]byte{filename: []byte("data")}, + }} + server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasPrefix(r.URL.Path, "/files/") { + if cause == context.Canceled { + cancel() + <-r.Context().Done() + return + } + w.Header().Set("Content-Length", "100") + _, _ = w.Write([]byte("truncated")) + return + } + state.serveHTTP(w, r) + })) + state.data.url = "http://" + server.Listener.Addr().String() + server.Start() + defer server.Close() + result, err := releasekit.Deliver(ctx, releasekit.DeliveryRequest{ + RepositoryDir: repository, Product: "Fixture", Version: "v1.2.3", + Destination: facadeDeliveryDestination(server.URL), Prebuilt: &prebuilt, + }) + if !errors.Is(err, cause) || !strings.Contains(err.Error(), filename) { + t.Fatalf("Deliver() error = %v, want cause %v and asset context", err, cause) + } + if result.State != releasekit.DeliveryNotAttempted || len(result.Uncertain) != 1 || result.Uncertain[0].Filename != filename || state.snapshot().writes != 0 { + t.Fatalf("result = %#v, want read-only uncertain asset without write", result) + } + }) + } +} diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index c20158f..b9519f7 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,13 +1,14 @@ # Remediation: forge boundary and credential preflight review -Status: remediation in progress; completion is recorded per issue below. +Status: complete; all eight issues are addressed, with completion recorded below. ## 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. +This document records review findings and their remediation. Findings describe +the reviewed revisions; completion notes record their corrections. A follow-up integration audit reviewed `72de187`. Its production source is unchanged from `cbdba90`. That audit traced the new boundary and preflight @@ -338,7 +339,14 @@ successful upload coordination can avoid its quadratic bookkeeping term. ## 8. Preserve asset-inspection failure causes during aggregation — P2 -Status: open. +Status: complete. + +**Completed:** Joined per-asset inspection causes with filename context in +unverifiable and conflict summaries, retaining conflict precedence and existing +states/categories. Added focused multiple-cause coverage and public API +regressions for cancellation and truncated downloads. Both regressions detect +the original defect and pass with the fix. Full offline tests, focused race +tests, and the API/dependency check passed; final verification is recorded below. **Evidence:** `inspectRemoteAssets` and `uploadMissingAssets` in [shared asset delivery](../../internal/delivery/remote_assets.go), and the @@ -438,3 +446,20 @@ finding 8; it was removed after the check. Findings 6 and 7 follow from source-level operation counts. No benchmarks or real-world performance measurements were run, and no production code or permanent tests changed. + +### Remediation verification + +All eight issues were addressed sequentially and marked complete with their +implementation or documentation changes. Final checks used Go 1.26.10 on +linux/amd64 with `GOTOOLCHAIN=local`, `GOPROXY=off`, and `GOSUMDB=off`: + +- `go test ./...` passed. +- `go run ./internal/apicheck` passed. +- `go test -race ./internal/delivery ./internal/gitea` passed. +- Focused public API and CLI regressions also passed with the race detector: + invalid destinations, CI module/tag mismatch, phase-specific statuses, and + asset-inspection cause preservation. +- A temporary source overlay verified that the new aggregation/public API + cause-preservation regressions fail against the previous implementation. +- Local links and heading fragments across changed Markdown files resolved; + whitespace checks passed. No benchmark measurements were needed. diff --git a/internal/delivery/remote_assets.go b/internal/delivery/remote_assets.go index a481019..b85346c 100644 --- a/internal/delivery/remote_assets.go +++ b/internal/delivery/remote_assets.go @@ -124,6 +124,7 @@ func uploadMissingAssets(ctx context.Context, backend forge.Backend, releaseHand result = UploadResult{State: Partial} preflightConflict := false preflightUnverifiable := false + var inspectionErrors []error var missing []artifact.ExpectedAsset pending := make(map[string]struct{}, len(expected)) // Keep the initial observation order and compact pending identities once, @@ -152,17 +153,20 @@ func uploadMissingAssets(ctx context.Context, backend forge.Backend, releaseHand case AssetUnverifiable: preflightUnverifiable = true result.Uncertain = append(result.Uncertain, item) + if observation.Err != nil { + inspectionErrors = append(inspectionErrors, fmt.Errorf("verify existing attachment %q: %w", item.Filename, observation.Err)) + } default: return result, &forge.Failure{Kind: forge.FailureInvalid, Err: fmt.Errorf("asset %q has no inspection result", observation.Expected.Filename)} } } if preflightConflict { result.State = Conflict - return result, &forge.Failure{Kind: forge.FailureConflict, Err: errors.New("one or more existing attachments conflict")} + return result, &forge.Failure{Kind: forge.FailureConflict, Err: errors.Join(errors.New("one or more existing attachments conflict"), errors.Join(inspectionErrors...))} } if preflightUnverifiable { result.State = NotAttempted - return result, &forge.Failure{Kind: forge.FailureUnverifiable, Err: errors.New("one or more existing attachments could not be verified")} + return result, &forge.Failure{Kind: forge.FailureUnverifiable, Err: errors.Join(errors.New("one or more existing attachments could not be verified"), errors.Join(inspectionErrors...))} } if len(pending) == 0 { result.State = Complete diff --git a/internal/delivery/remote_assets_test.go b/internal/delivery/remote_assets_test.go index 8042f5e..8867adc 100644 --- a/internal/delivery/remote_assets_test.go +++ b/internal/delivery/remote_assets_test.go @@ -7,6 +7,7 @@ import ( "io" "os" "reflect" + "strings" "testing" "go.maximumdirect.net/releasekit/internal/artifact" @@ -173,6 +174,35 @@ func TestUploadMissingAssetsStopsBeforeWritesForConflictOrUnverifiableContent(t } } +func TestUploadMissingAssetsPreservesInspectionCausesAndConflictPrecedence(t *testing.T) { + readErr := errors.New("asset download failed") + for _, conflict := range []bool{false, true} { + name := "unverifiable" + if conflict { + name = "conflict" + } + t.Run(name, func(t *testing.T) { + backend := &assetBackend{} + inspection := Inspection{Assets: []AssetInspection{ + {Expected: expectedAsset("canceled.bin", []byte("a")), State: AssetUnverifiable, Err: context.Canceled}, + {Expected: expectedAsset("unreadable.bin", []byte("b")), State: AssetUnverifiable, Err: readErr}, + }} + wantState, wantKind := NotAttempted, forge.FailureUnverifiable + if conflict { + inspection.Assets = append(inspection.Assets, AssetInspection{Expected: expectedAsset("conflicting.bin", []byte("c")), State: AssetConflicting}) + wantState, wantKind = Conflict, forge.FailureConflict + } + result, err := uploadMissingAssets(context.Background(), backend, "release", inspection) + if !isForgeFailure(err, wantKind) || result.State != wantState || len(result.Uncertain) != 2 || len(backend.uploadNames) != 0 { + t.Fatalf("result = %#v, error %v", result, err) + } + if !errors.Is(err, context.Canceled) || !errors.Is(err, readErr) || !strings.Contains(err.Error(), "canceled.bin") || !strings.Contains(err.Error(), "unreadable.bin") { + t.Fatalf("aggregate lost cause or asset context: %v", err) + } + }) + } +} + func TestUploadMissingAssetsReconcilesOnceAndKeepsLaterAssetsPending(t *testing.T) { assets := []artifact.ExpectedAsset{ expectedAsset("a.bin", []byte("a")),