Record remaining implementation review issues

docs/roadmap/remediation.md

@@ -2,8 +2,10 @@
 
 ## Status and scope
 
-Remediation complete against baseline `59bb074`. The implementation plan
+The planned remediation was implemented against baseline `59bb074`. A
-records stage and validation evidence.
+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.