From c42bc010522d0d8fd27c6b2be03efaf4d87cfd31 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Thu, 24 Sep 2026 03:07:36 +0000 Subject: [PATCH] Record library adoption review findings --- docs/roadmap/remediation.md | 199 ++++++++++++++++++++++++++++++++++++ 1 file changed, 199 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..7bea293 --- /dev/null +++ b/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.