Record performance implementation review findings

docs/roadmap/remediation.md

@@ -0,0 +1,145 @@
+# Performance Implementation Remediation
+
+**Status:** Required follow-up; remediation pending.
+
+**Review baseline:** df064bd, reviewed on 2026-10-09 against the
+[performance target](performance.md) and
+[implementation plan](implementation.md).
+
+## Scope And Architecture
+
+The five intended improvements are present behind the existing public facades:
+an internal piped-process boundary and lazy Git batch reader, operation-local
+validation result caches, consolidated local asset preflight, completed Go-output
+buffer ownership, and a cached runtime metadata fallback. Public API and
+dependency checks pass. This review found no added public input limits, release
+gates, or product restrictions in the implementation commits.
+
+The findings below cover correctness and documentation gaps in that work.
+Implement fixes within the existing architecture and preserve the established
+public contracts. The [agent](../policy/agents.md),
+[testing](../policy/testing.md), and
+[documentation](../policy/documentation.md) policies govern the follow-up.
+
+## R1: Preserve Operation Cancellation Through Reader Shutdown
+
+**Priority:** High.
+
+**Owners:** [candidate tree](../../internal/validation/tree.go),
+[validation executor](../../internal/validation/validation.go), and
+[Git batch reader](../../internal/git/blob_reader.go).
+
+CandidateTree.readBlob creates its reader with context.Background().
+BlobReader.Read registers cancellation for an individual exchange and removes
+that registration when the exchange finishes. The executor's deferred cleanup
+then calls tree.Close(), whose stdout drain and process wait have no connection
+to the validation operation's context.
+
+Consequently, cancellation while the child is shutting down cannot interrupt
+that cleanup. Validation can remain blocked after its caller cancels. If the
+child is subsequently terminated independently, the resulting error reports
+the process exit and loses the caller's cancellation cause.
+
+**Evidence:** A temporary offline fixture served valid committed Go content,
+waited until the executor closed batch stdin, signalled readiness, and stalled
+before exiting. Cancelling Run at that point left it blocked. After the probe
+terminated the fixture child, Run returned a Go-format cleanup failure for
+"signal: killed"; errors.Is(err, context.Canceled) was false. The existing
+TestRunPreservesCancellationDuringReaderCleanup cancels during an unfinished
+blob read, so it exercises the per-read callback rather than this shutdown gap.
+
+**Required outcome:** Associate shutdown with the validation operation context,
+either through the reader's lifetime context or context-aware cleanup. Preserve
+individual-read cancellation and the primary failure when a check has already
+failed. Reap the child on all paths. Use the supplied context for cancellation;
+this fix does not require a new runtime timeout or input restriction.
+
+**Validation:** Add a regression synchronized to batch-input EOF, cancel while
+shutdown is still blocked, and verify completion, child cleanup, and preserved
+cancellation identity. Retain coverage of read cancellation, successful closure,
+and preservation of an earlier check failure.
+
+## R2: Attribute Shutdown Failures To A Check That Read Content
+
+**Priority:** Medium.
+
+**Owner:** [validation executor](../../internal/validation/validation.go).
+
+The lastContentCheck variable is assigned when a content-oriented check is
+selected, even when that check has no applicable files and makes no content
+request. Deferred cleanup therefore reports the last selected check rather than
+the last check that used the reader, diverging from the implementation plan's
+failure-attribution rule and the completion record's description.
+
+**Evidence:** A temporary fixture with only go.mod and main.go selected Go
+formatting and Markdown checking. Only formatting read a blob. The child then
+exited with code 23 during shutdown, and Run reported "markdown-links".
+
+**Required outcome:** Track actual reader use by each check and preserve the
+last check that used content access when a later selected check performs no
+reads. Keep check ordering and earlier failure attribution intact. Reader-use
+tracking remains internal implementation bookkeeping.
+
+**Validation:** Extend the existing unexpected-close-failure fixture with a
+later selected check that has no applicable files. Verify the earlier reader
+user receives the cleanup cause and the child exit error remains discoverable.
+
+## R3: Qualify Output Allocation Measurements By Their Workload
+
+**Priority:** Low.
+
+**Owners:** [Go execution documentation](../internal/gotool.md),
+[output benchmark](../../internal/gotool/performance_test.go), and the
+[implementation results](implementation.md).
+
+The internal Go-output document says an 8 MiB forwarded capture currently
+allocates about 8.39 MB per operation. The cited benchmark makes one 8 MiB
+Write call. Actual command output can arrive in smaller chunks, which incur
+additional bytes.Buffer growth allocations. The statement needs its workload
+qualification to describe the measured behavior accurately.
+
+**Evidence:** Temporary component probes using the current writer and io.Discard
+measured approximately 8,388,691 B/op and two allocations for one 8 MiB write,
+versus 16,744,535 B/op and ten allocations for the same total output in 32 KiB
+writes. These measurements exclude subprocess overhead and failure diagnostic
+strings. Both paths correctly avoid the former final capture copy.
+
+**Required outcome:** Identify the single-write workload wherever that figure
+is quoted. Include a representative chunked capture measurement in the benchmark
+set and results. Distinguish allocated bytes across an operation from retained
+output bytes and peak memory. Complete in-memory capture remains the selected
+design; these findings do not require disk-backed diagnostics or a memory cap.
+
+**Validation:** Compare equal output lengths across the two chunking patterns
+and confirm captured bytes and forwarding remain correct. Report allocation
+results as workload-specific measurements with no numerical runtime guarantee.
+
+## R4: Reconcile Feature And Implementation Status
+
+**Priority:** Low.
+
+**Owner:** [performance roadmap](performance.md).
+
+The performance roadmap still says "Ready for implementation; implementation
+pending." The implementation document marks Stages 1–11 complete, and the code
+contains the five target components. Stage 11 explicitly called for updating the
+feature roadmap status, and its completion record claims the work is complete.
+
+**Required outcome:** Update the feature roadmap to distinguish the implemented
+target components from the outstanding review findings recorded here. Keep
+remediation status in this document and performance results in the implementation
+record, consistent with their documentation ownership.
+
+**Validation:** Check status and navigation across the three roadmap documents
+against the actual implementation and remediation outcomes.
+
+## Review Validation
+
+- The full offline Go test suite passed; some unchanged packages used cached
+  results.
+- The compiled public API and dependency checks passed.
+- Focused race tests passed for internal/git, internal/process,
+  internal/runtimeinfo, internal/gotool, and internal/validation.
+- The targeted probes used temporary Go overlays and local child-process
+  fixtures. They reproduced R1 and R2 and measured the workload distinction in
+  R3. No production code or permanent tests were changed during this review.