Record library adoption review findings

docs/roadmap/remediation.md

@@ -0,0 +1,199 @@
+# Library Adoption Review: Required Remediation
+
+**Reviewed:** 2026-09-24, source commit `7cefb3f`.
+**Scope:** The twelve implementation commits after `1ad2119`, compared with the
+[implementation plan](implementation.md), [combined target](library-adoption.md),
+and [configuration target](configkit.md).
+**Status:** Findings confirmed; fixes have not been implemented.
+
+## Architecture Assessment
+
+The intended ownership split is substantially implemented. ConfigKit v0.4.0 owns
+schema-2 loading, overlays, presence/default resolution, explicit overrides,
+credentials, and configuration inspection. One shared PromptKit v0.11.0 engine
+owns both roles' profiles, embedded prompt rendering, prepared execution, provider
+transport, retry/repair behavior, and per-backend admission. Audita retains domain
+configuration, CLI composition, ordered modules, deterministic validators,
+proposal/application checks, validation batching/cardinality, reporting, and
+artifact lifecycle. The root module requires Go 1.25.5 and has no local replacement.
+
+Legacy loaders, provider transport, template rendering, global/role schedulers,
+and the isolated prototype were removed. All nine prompt definitions preserve
+the original instruction text after accounting for the new hardening variable
+and whitespace. There is no need to revisit the library-adoption design or add
+compatibility machinery. The findings below are concrete defects or unfulfilled
+parts of the selected final contract.
+
+## R1 — P1: Redact Model-Derived Content in Every Retained Report Artifact
+
+**Category:** Incomplete implementation of Stage 10's redaction boundary.
+
+**Evidence:** Both success and failure paths in
+[`runProcess`](../../internal/cli/run.go#L311) write
+`BuildCorrectionLedger(...)` directly through `RunDirectory.WriteJSONArtifact`.
+The success-path write is at line 348. That writer serializes its input without
+redaction. Validator decision/rejection messages contain provider-supplied reason
+text. Redacting the separate process report does not protect the ledger.
+
+A controlled HTTP subprocess reproduction returned a schema-valid negative
+assessment whose reason contained the synthetic configured backend credential.
+Processing succeeded and the process report was redacted, but
+`correction-ledger.json` retained the credential verbatim. The existing provider-
+error test does not exercise this successful, schema-valid response path.
+
+**Required change:** Apply the configured-secret redaction boundary to the ledger
+and other model-derived diagnostic/report outputs before persistence. Audit the
+new execution/utilization and selected-profile artifact paths for the same bypass;
+metadata-only output is not a substitute for enforcing the selected secret-value
+contract. Use all resolved backend credentials, including unselected backends.
+Keep processing/domain data separate from its sanitized diagnostic representation.
+Coordinate the shared redaction implementation with R2.
+
+**Acceptance:** A controlled provider echoes synthetic credentials in a valid
+assessment reason. Neither success nor partial-failure reports, ledgers, interaction
+artifacts, or other affected diagnostic outputs contain those values. Include an
+unselected backend credential. Preserve valid serialization and domain outcomes;
+do not expand tests into exhaustive permutations of every artifact.
+
+## R2 — P1: Remove the Report Redactor's Unredacted Fallback
+
+**Category:** New code-quality/security defect in Stage 10.
+
+**Evidence:** [`redactProcessReport`](../../internal/cli/run.go#L392) replaces secret
+substrings in the serialized JSON document, then decodes it back into the report.
+If decoding fails, it returns the original report. Replacements can corrupt JSON
+syntax or typed values rather than just redact strings.
+
+A focused reproduction loaded an otherwise valid config with the synthetic
+credential `2`, then redacted a report containing `config_version: 2` and the string
+`echo:2`. Replacing the numeric token made the JSON invalid; the helper returned
+the original `echo:2`. Short numeric credentials are accepted by the selected
+secret-reference contract, so the implementation cannot silently assume otherwise.
+Quotes, escapes, overlapping credentials, and replacement order also need handling
+at the serialization boundary.
+
+**Required change:** Redact string content without changing JSON structure or
+numeric/boolean types. Never return or write the original sensitive object when
+redaction/serialization fails. Share the appropriate string-redaction logic across
+report and artifact writers rather than maintaining incompatible versions. Handle
+escaped content and overlapping values deliberately; retain typed error causes
+without exposing provider error text.
+
+**Acceptance:** Focused tests cover a numeric credential coinciding with report
+numbers, quoted/backslash-containing credentials, and overlapping values. Output
+remains valid with the original numeric/boolean meanings, secrets are removed,
+and a redaction failure cannot emit the original object. Exercise the actual
+external and run-local report writing paths with representative coverage.
+
+## R3 — P1: Cancel Remaining Work When a Validator Fails
+
+**Category:** Pre-existing runner behavior left inconsistent with the selected
+Stage 8 final state and architecture policy; not newly introduced by PromptKit.
+
+**Evidence:** [`runModulePipeline`](../../internal/framework/runner/runner.go#L383)
+puts validation errors into a buffered channel, but reads that channel only after
+`vwg.Wait()` (lines 414–421). `setErr` therefore cannot cancel the shared context
+in response to a validator failure until all validation workers have finished.
+Proposal work can continue launching validation in the meantime.
+
+A controlled two-section reproduction made one validator fail while its peer
+waited for context cancellation or an explicit test release. The peer was not
+canceled; the pipeline returned its error only after the peer was manually released.
+In production, a failed validation operation can thus leave other queued or active
+provider calls consuming capacity and money until they finish or reach their own
+deadlines. Adapter-level cancellation tests do not cover this runner behavior.
+
+**Required change:** Propagate validation failures to shared cancellation as soon
+as they occur, while retaining the originating error, joining active workers, and
+collecting available results deterministically. Use a coordinator or properly
+synchronized failure state; simply calling the current `setErr` from workers would
+also require protecting concurrent reads of `firstErr`. Do not add a scheduler,
+outer retry loop, or compatibility layer.
+
+**Acceptance:** A runner integration test with a failing validator and a blocked
+peer proves cancellation arrives without releasing the peer manually. Cover queued
+provider work and preservation of the originating failure/partial observations.
+The affected runner/runtime tests pass under the race detector, and failure emits
+no purportedly complete corrected transcript.
+
+## R4 — P2: Separate Module Identity from Section-Specific Diagnostic Names
+
+**Category:** New observation/report attribution defect in Stages 9–10.
+
+**Evidence:** [`validateSectionCandidates`](../../internal/framework/runner/runner.go#L525)
+passes `ModuleInstanceForStages` as the validation request's module identity. This
+value includes `:section-NNNN`. The
+[validator call](../../internal/framework/validators/llm_validators.go#L110) copies
+it into `DiagnosticContext.ModuleInstance` and leaves `SectionIndex` unset.
+[`BuildUtilization`](../../internal/framework/processreport/llm.go#L25) groups
+these records by that field, so proposal and validation calls for one real module
+are reported under different module identities.
+
+The subprocess reproduction with one grammar module and one section produced:
+
+| Reported module instance | Proposal calls | Validation calls |
+| --- | ---: | ---: |
+| `grammar` | 1 | 0 |
+| `grammar:section-0000` | 0 | 1 |
+
+The latter is not a configured module instance. Its timing cannot join the real
+module result, and the actual module incorrectly reports zero validation calls.
+Multi-section runs multiply these spurious entries. Explicit section attribution
+is also lost from validation call/event records.
+
+**Required change:** Carry actual module instance and section index separately
+through the validator request and structured-call context. Keep section-qualified
+artifact names as a separate naming concern so files remain distinct. Group counts
+and timing by the actual module instance. Update tests that currently use batch
+index as a substitute for section identity.
+
+**Acceptance:** A multi-section workflow with repeated module instances yields one
+utilization entry per actual instance, correct proposal/validation totals, and
+explicit section/batch identity in validation observations. Artifact names remain
+unique and correction-ledger ordering is unchanged.
+
+## R5 — P2: Index Execution Events Instead of Repeatedly Scanning Them
+
+**Category:** New report-building performance/maintainability defect in Stage 10.
+
+**Evidence:** [`BuildLLMExecution`](../../internal/framework/processreport/llm.go#L151)
+scans every event for every call. `BuildUtilization` additionally tests every event
+against a linear `hasCall` search for each backend's calls. With C calls and E
+events, these paths perform O(C × E) work even for one backend. Retries and repairs
+increase E. This work occurs after processing, before the CLI completes.
+
+A synthetic review probe using one terminal event per call measured:
+
+| Calls/events | Execution mapping | Utilization mapping |
+| ---: | ---: | ---: |
+| 1,000 | 14 ms | 6 ms |
+| 10,000 | 1.61 s | 0.52 s |
+| 20,000 | 8.08 s | 2.42 s |
+
+These are local diagnostic measurements, not performance guarantees; the nested
+scans establish the underlying growth independently of machine timing. Actual
+snapshots usually contain more than one event per call.
+
+**Required change:** Index calls/events by call ID once and aggregate by backend
+and actual module identity using direct lookups. Reuse terminal records for totals
+without rescanning all events for each call. Preserve event/call ordering and
+knownness/completeness semantics. Straightforward maps are sufficient; no reporting
+framework or new persistent cache is needed.
+
+**Acceptance:** Existing report semantics remain intact, including preparation
+failures, undispatched calls, and incomplete usage. A local benchmark or profiling
+comparison demonstrates removal of the quadratic scans. Do not add fragile
+wall-clock timing assertions to the default test suite.
+
+## Review Verification and Limits
+
+The existing root `go test ./...`, focused race tests for LLM/runner/subprocess
+packages, `go vet ./...`, and Linux amd64/arm64 builds passed. Temporary review
+probes reproduced R1–R4 and measured R5; those probe files were removed afterward.
+The retained suite currently passes despite these defects. Add durable regression
+coverage with the fixes, using the narrowest useful production boundaries.
+
+Only this remediation document is changed by the review. No application fixes,
+upstream changes, live provider calls, release publication, or compatibility work
+were performed. Release-distribution compliance remains the explicitly deferred
+release task and is not counted as an implementation defect here.