From 1ad6ddc198376b39a4bcf0190f0353d7932e2a46 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 1 Nov 2026 21:42:04 +0000 Subject: [PATCH] Record remaining implementation review issues --- docs/roadmap/remediation.md | 109 +++++++++++++++++++++++++++++++++++- 1 file changed, 107 insertions(+), 2 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 2bc4dfa..1764f6a 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -2,8 +2,10 @@ ## Status and scope -Remediation complete against baseline `59bb074`. The implementation plan -records stage and validation evidence. +The planned remediation was implemented against baseline `59bb074`. A +post-implementation review of `6796310` found two follow-up issues below; +remediation remains open until they are addressed. The implementation plan +records the original stage and validation evidence. This roadmap defines the desired behavior and scope for all three findings in the [functional correctness audit](correctness.md) and all five findings in the @@ -106,3 +108,106 @@ policy. They introduce no new public configuration knobs, compatibility shims, release procedure, pilot release, or intermediate human-review requirement. The targeted sprint reaches this complete end state through local repository work and automated validation. + +## Post-implementation review findings + +The review compared implementation commits `0632aa5` through `6796310` with the +agreed target and the plan at `730e638`. It examined production changes, new +regressions, current documentation, and the final package boundaries. The +following findings require follow-up; the other planned corrections are present. + +### 1. Damaged checkout metadata can silently select standalone preparation + +**Priority:** medium. **Status:** open. + +**Code:** `DiscoverRepositoryRoot` in +[internal/git/source.go](../../internal/git/source.go), particularly the +recognized no-repository branch, and public `PrepareBuildDependencies` in +[preparation.go](../../preparation.go). + +The discovery code treats exit status 128 and the recognized no-repository +diagnostic as conclusive evidence of standalone use. Git emits that same +diagnostic when a checkout's `.git` directory remains present but its `HEAD` +file is missing. The marker inspection used when Git is unavailable does not +run on this branch. A broken `.git` file has coverage, but this damaged-directory +case does not. + +**Reproduction:** a temporary public-API probe created and committed root and +child modules, then called preparation from the child with empty `ModuleDir`. +With healthy metadata it selected the root. After moving `.git/HEAD` out of the +disposable checkout, the same request returned nil and ran Go in the child. +A local Go recording shim observed the directory without downloading anything. + +**Expected:** damaged checkout discovery fails before dependency preparation; +the request does not silently acquire a different selection base. This follows +the agreed treatment of broken metadata and other discovery failures. + +**Actual consequence:** in a multi-module repository, metadata damage can make +an unchanged request prepare another module's graph and report success. This +is a remaining gap in checkout selection and failure handling. + +**Correction:** establish that the no-repository result represents standalone +use before accepting it as a fallback. Account for relevant existing checkout +metadata and preserve the Git failure when that metadata is damaged. Keep Git's +normal discovery boundaries and genuine standalone/Go-only use; do not turn +this into a general directory-name restriction or a new public option. Add a +real damaged-`.git`-directory regression through the public preparation facade, +asserting an error and no Go invocation, alongside healthy and standalone +controls. The original plan's diagnostic-only classification also needs this +refinement; reproducing its algorithm alone is insufficient completion evidence. + +### 2. The deadline regression starts its timeout before inspection readiness + +**Priority:** low. **Status:** open. + +**Code:** `TestBuildSourceInspectionCancellation/deadline` in +[build_source_cancellation_test.go](../../build_source_cancellation_test.go). + +The deadline context starts with a three-second timeout before public `Build` +resolves the repository and module and reaches `cleanSource`. The test waits +for the inspection wrapper's readiness marker afterward, but that marker does +not control when expiration begins. The separate 45-second readiness watchdog +cannot prevent this earlier deadline from expiring during unrelated setup. + +**Reproduction:** running only this subtest with a temporary delegating Git +wrapper that delays `rev-parse --show-toplevel` by four seconds fails with +`build ended before inspection` and a repository-resolution deadline error. +The test never exercises source inspection. The ordinary suite passes without +the injected startup delay. + +**Expected:** the deadline regression exercises expiration during the blocked +source-inspection command, as required by the plan's synchronized-readiness +case. Unrelated setup latency does not determine whether that path is tested. + +**Actual consequence:** a slower runner can fail the default suite for timing +outside the behavior being tested. This is a test reliability gap; the probe +does not demonstrate another production cancellation defect. + +**Correction:** make the expiration case readiness-controlled, or isolate it at +the source-inspection helper boundary so earlier repository/module work cannot +consume its deadline. Retain public cancellation and partial-artifact coverage, +bounded cleanup, and the `errors.Is(context.DeadlineExceeded)` assertion. +Increasing the whole-operation timeout alone does not establish the intended +inspection-specific synchronization. Verify the revised case under delayed +unrelated setup as well as normal and race-enabled execution. + +## Review validation and scope assessment + +The ordinary offline `go test ./...` suite and compiled +`go run ./internal/apicheck` check passed on the reviewed code with +`GOTOOLCHAIN=local GOPROXY=off GOSUMDB=off`. Race-enabled Git, Gitea, Go-tool, and +candidate package checks also passed. The temporary delayed-startup test above +failed as expected and substantiated finding 2; it is separate from the ordinary +passing suite. Focused public preparation, publication, cancellation, delivery, +and CLI regressions also passed with the race detector. Probes used disposable +local repositories and command wrappers, with no production changes or new +committed tests. + +The root orchestration facade, separate runtime facade, neutral forge boundary, +and Gitea adapter retain their responsibilities. Ref finalization, candidate tag +validation, cancellation error propagation, module-file override, and asset-use +failure classification follow the selected approach. The review found no +additional public types, configuration knobs, receipt changes, stronger page-only +response requirements, or broader source/preflight guarantees introduced by the +implementation. The two findings concern incomplete realization of the agreed +behavior and regression reliability rather than an intentional scope expansion.