diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 65ddd47..3203097 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -15,6 +15,11 @@ through existing CI preparation, module/tag selection, build receipts, token loading, source publication, and retry/error handling. It added finding 5 and expanded finding 3 with a later authorization-failure reproduction. +An algorithmic audit reviewed `370fe3a`, whose production source is still +unchanged from `cbdba90`. It added findings 6–8. These opportunities and defects +were retained when policy moved from the former Gitea implementation into +shared delivery. + 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; @@ -225,6 +230,99 @@ validation behavior across the two supported CI paths. requests or dependency preparation. Retain the valid binary path's early credential check and subsequent delivery behavior. +## 6. Make pending-asset bookkeeping linear — P3 + +Status: open. + +**Evidence:** `uploadMissingAssets`, `reconcileAssetUpload`, and +`removeIdentity` in [shared asset delivery](../../internal/delivery/remote_assets.go). + +Every acknowledged upload removes its filename from `result.Pending` through +`removeIdentity`. That helper searches the slice and shifts its remaining tail. +For `k` initially missing assets uploaded successfully in their pending order, +the searches find the first element immediately, but the deletions still move +`(k-1) + (k-2) + ... + 1 = k(k-1)/2` identities. Bookkeeping alone is therefore +Theta(`k²`), independently of network and content processing. A different +pending order can also incur repeated linear searches. Reconciled uploads use +the same removal helper. + +**Refactoring opportunity:** Track pending membership or per-filename state in +a map, update it in expected constant time per completed/reconciled upload, +and materialize `Pending` with one pass through the initial pending order when +returning. For `n` declarations, this permits O(`n`) total category bookkeeping +and O(`n`) metadata space. Keep the current category ordering during this +refactor; a final pass avoids repeated shifts without requiring swap deletion. +Apply finalization to every return path so partial, conflicting, and unknown +outcomes retain later missing assets and disjoint categories. + +Existing progress/reconciliation tests should remain useful. Add a focused +case only if needed to protect pending results after early termination. The +usual small target sets limit practical impact; this finding establishes an +avoidable asymptotic cost without claiming measured release-time savings. + +## 7. Order the missing upload set once — P3 + +Status: open. + +**Evidence:** `orderedExpected`, `orderMissing`, and `uploadMissingAssets` in +[shared asset delivery](../../internal/delivery/remote_assets.go). + +`orderedExpected` copies and sorts all `n` declarations before validation and +classification. This happens even when every asset matches, a conflict stops +delivery, or an unverifiable asset prevents uploads. If uploads proceed, the +coordinator copies and sorts the full list again, then skips entries outside +the missing set. The first full sort requires O(`n log n`) comparisons in the +worst case. The second sort receives already ordered input, so its actual cost +can be lower, but its full-list copy and ordering pass are still redundant. + +**Refactoring opportunity:** Collect declarations without ordering, validate +and classify them in a linear pass, and return immediately when classification +prevents or eliminates uploads. Collect only the `k` missing declarations and +sort that worklist once, retaining filename order and `SHA256SUMS` last. +Declaration/classification/ordering work becomes O(`n + k log k`), with O(`n`) +metadata space, and O(`n`) work when no upload is needed. This also removes +redundant full-list copies. These bounds treat filename operations as unit +cost and exclude inline-content hashing, which remains linear in its bytes. + +Keep complete sorted manifest generation and all validation. The plan selected +the upload ordering and manifest format; this opportunity concerns the size +and duplication of the scheduling work, not either selected behavior. The +existing ordering, matching-manifest, and no-write-on-conflict tests cover the +main behavior that the refactor should preserve. Combined with finding 6, +successful upload coordination can avoid its quadratic bookkeeping term. + +## 8. Preserve asset-inspection failure causes during aggregation — P2 + +Status: open. + +**Evidence:** `inspectRemoteAssets` and `uploadMissingAssets` in +[shared asset delivery](../../internal/delivery/remote_assets.go), and the +cause-preservation requirement in the +[implementation plan](implementation.md#internal-ownership-and-dependencies). + +An asset-open or content-read failure is retained in `AssetInspection.Err` and +classified as `AssetUnverifiable`. `uploadMissingAssets` then discards those +errors and returns a fresh generic `FailureUnverifiable` message. `Deliver` +returns that aggregate error, so callers lose the underlying diagnostic and +cannot use `errors.Is` or `errors.As` to recognize its cause. + +A temporary test overlay supplied one existing asset whose `OpenAsset` returned +`context.Canceled`, then repeated the case with a distinct read-failure +sentinel. Inspection retained each cause. Aggregation correctly reported +`NotAttempted`, one uncertain asset, and no uploads, but `errors.Is` could find +neither cause in its returned error. Whole-list failures already return their +original cause directly; this finding concerns per-asset failures folded into +an otherwise successful inspection. + +**Required work:** Collect relevant non-nil observation errors during the +existing classification pass and wrap or join them with the aggregate summary. +Preserve actionable asset context, cancellation identity, and the existing +conflict precedence, asset categories, and delivery states. Read-only failures +remain distinct from unresolved writes. This takes O(`n`) traversal and O(`u`) +error references for `u` unverifiable observations, without a new public error +type or protocol operation. Retain a focused regression asserting cause +identity through the returned delivery error, including cancellation. + ## Verification performed Using Go 1.26.10 on linux/amd64 with module/toolchain downloads disabled: @@ -261,3 +359,37 @@ The build, receipt, publication, candidate-validation, dependency-preparation, and Go-tool implementation owners were also compared with the pre-feature baseline. Their production code is unchanged; the confirmed new interaction occurs in the CI adapter's composition and the phase-specific error contract. + +### Algorithmic audit + +The audit covered the implemented plan's facade/CLI adaptation, backend +contract and preflight, page coordination/recovery, local artifact mechanics, +remote inspection/upload bookkeeping, Gitea read/write transport, and dependency +checks. It compared the moved code with the pre-feature baseline. Bounds below +describe collection/content work with ordinary expected constant-time map +operations and unit-cost metadata comparisons; variable-length names and URLs +add their corresponding byte-processing costs. + +| Reviewed operation | Assessment | +| --- | --- | +| Remote asset inspection | Expected O(`n + r + B`) time for `n` declarations, `r` listed assets, and `B` downloaded bytes; filename maps avoid nested list searches, and content hashing is streamed. | +| Artifact declaration and local verification | Map-based identity/duplicate checks are linear; hashing is linear in content bytes. Complete manifest sorting remains the selected O(`n log n`) algorithm. | +| Page matching and recovery | Note comparisons are linear in note bytes; shared coordination uses the selected bounded follow-up lookup. | +| Gitea adaptation and transport | Metadata conversion is linear in listed entries; uploads stream bytes. Redirect tracking uses a visited map rather than repeated scans. | +| Facade result conversion and dependency checks | Result conversion is linear in category entries. Dependency traversal uses visited sets from a fixed number of roots, O(`V + E`) in the reachable graph. | +| Upload scheduling and pending state | Findings 6 and 7 identify avoidable work beyond the selected ordering and recovery behavior. | + +The audit preserved explicitly selected algorithms: complete sorted manifests, +filename upload ordering with the manifest last, full local verification before +writes, streamed remote comparisons, one follow-up reconciliation per ambiguous +write, current asset listing behavior, and fresh credential checks around CI +preparation. It proposes no batching, pagination, concurrency, validation cache, +or altered recovery policy. + +Fresh offline tests for `internal/delivery`, `internal/artifact`, and +`internal/gitea` and the API/dependency-boundary check passed. A temporary +overlay intentionally failed two cause-preservation assertions to reproduce +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.