diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index d18c73c..ea4b2b5 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,289 +1,199 @@ -# Library Adoption Review: Required Remediation +# Resilience Review: Required Remediation -**Reviewed:** 2026-09-24, source commit `7cefb3f`. -**Scope:** The twelve implementation commits after `1ad2119`, compared with the -[implementation plan](https://gitea.maximumdirect.net/eric/audita/src/commit/cb405f6/docs/roadmap/implementation.md), [combined target](library-adoption.md), -and [configuration target](configkit.md). -**Status:** R1–R5 addressed and verified on 2026-09-24. The evidence in the -original findings below describes source commit `7cefb3f`; line references are -historical. See the resolution record at the end for the implemented fixes. +**Reviewed:** 2026-09-24, source commit `6523d7a`. +**Scope:** The fourteen implementation commits after `8ffbf41`, compared with +[implementation.md](implementation.md), [resilience.md](resilience.md), and the +current architecture and output contracts. +**Status:** Five open findings, R6–R10. This review implements no fixes. + +The earlier library-adoption findings R1–R5 were resolved before this work. +Their findings and resolution evidence remain in the +[previous remediation document](https://gitea.maximumdirect.net/eric/audita/src/commit/6523d7a/docs/roadmap/remediation.md). +Numbering continues here to distinguish new work from those resolved findings. ## 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. +The intended architecture is substantially implemented: -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. +- ConfigKit owns configuration loading, overlays, explicit overrides, and resolved + credentials. Reporting and debug are independent configuration selections. +- One shared PromptKit engine serves proposal and validation roles, owns bounded + retries/repairs and backend admission, and supplies execution facts. Audita's + adapter distinguishes recoverable model failures from fatal setup/internal + errors and caller cancellation. +- Audita normalizes and chunks once, retains section identity as text evolves, + preserves stage barriers, and validates proposals through the required chains. + Failed validation batches exclude their candidates while successful survivors + continue. Model errors do not cancel unrelated work; fatal errors cancel peers + and join workers. +- Processing-owned outcomes feed a separate report mapper and presentation DTOs. + The ordinary bundle contains versioned error/recovery YAML and fixed Markdown + matrices. The CLI uses the same counts; detailed persisted evidence is opt-in. +- The filesystem output session checks destinations, initializes reports, and + publishes transcript and reports through per-file replacement. CLI composition + owns streams, lifecycle, fatal finalization, and optional debug handling. -## R1 — P1: Redact Model-Derived Content in Every Retained Report Artifact +The ordinary resilience path is supported by the retained integration tests, +including total model failure, partial validation success, stable chunks, +recovery counting, and reporting/debug combinations. The findings below concern +specific boundary defects and incomplete evidence handling, not a need to +redesign the library ownership or introduce compatibility machinery. -**Category:** Incomplete implementation of Stage 10's redaction boundary. +## R6 — P1: Honor Restrictive Creation Permissions -**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. +**Category:** New filesystem/security regression in Stage 10. -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. +**Evidence:** [`atomicReplace`](../../internal/core/diagnostics/output_session.go#L249) +creates a temporary file and then unconditionally changes its mode to `0644` for +an absent destination (lines 261–265). `Chmod` does not apply the process umask. +The previous transcript writer used ordinary file creation, which did apply it. +This affects both the published output and the temporary file while populated. -**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. +An actual CLI run under umask `0077`, with no LLM modules and a valid transcript, +returned success but created both `out.json` and `errors.yml` with mode `0644`, +rather than `0600`. In a traversable output directory, other users can therefore +read transcript content despite the operator's restrictive creation policy. -**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. +**Required change:** Keep standard per-file replacement, but honor ordinary +creation permissions for previously absent destinations. Preserve intended +existing-file permission behavior without blindly widening new files. Do not +read/change a process-global umask around concurrent application operations or +add a transaction/durability framework. -## R2 — P1: Remove the Report Redactor's Unredacted Fallback +**Acceptance:** A subprocess under a restrictive umask produces new transcript +and report files with appropriately restricted permissions. Existing restrictive +output permissions remain preserved after replacement. Keep the current cleanup +and late-write failure behavior. Test umask behavior in an isolated process. -**Category:** New code-quality/security defect in Stage 10. +## R7 — P2: Do Not Report Success Without Publishing a Transcript -**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. +**Category:** New destination-selection inconsistency in Stages 10/12. -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. +**Evidence:** [`PrepareOutput`](../../internal/core/diagnostics/output_session.go#L53) +uses a trimmed path to decide whether a file destination exists, as does +[`selectedReportDir`](../../internal/cli/config_input.go#L215). However, +[`finish`](../../internal/cli/process.go#L377) selects stdout only when the original +path equals the empty string. Parsing does not reject a whitespace-only output +path. Consequently, neither branch publishes the transcript. -**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. +A real invocation with `--output ' ' --reporting=false` returned exit 0 and the +successful completion message, wrote zero bytes to stdout, and created no output +file. This violates the central contract that successful processing produces a +transcript, independently of reporting selection. -**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. +**Required change:** Resolve file versus stdout once and use that decision +consistently throughout placement, preflight, publication, and reporting. Prefer +preserving a supplied nonempty filename literally, including legal whitespace, +with only the empty value selecting stdout. Do not silently turn a file request +into discarded output. -## R3 — P1: Cancel Remaining Work When a Validator Fails +**Acceptance:** Exercise a whitespace-only output filename through the actual CLI: +success must mean the requested file exists and contains the encoded transcript. +Omitted output still writes the transcript to stdout, and reports follow the same +resolved destination decision. -**Category:** Pre-existing runner behavior left inconsistent with the selected -Stage 8 final state and architecture policy; not newly introduced by PromptKit. +## R8 — P2: Redact Paths Printed to Stderr -**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. +**Category:** New report-location bypass plus an unclosed debug-location gap in +the Stage 11/12 redaction boundary. -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. +**Evidence:** [`finish`](../../internal/cli/process.go#L398) prints report paths and +the debug directory without redaction. The debug locations printed on failure +at lines 365 and 394 do the same. Ordinary report renderers and error diagnostics +already have configured-secret redaction; these announcements bypass it. -**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. +A CLI reproduction used a synthetic configured credential also present in the +selected output-directory name. `report.md` correctly redacted the value, but +the successful run printed it verbatim in all three `Report:` lines on stderr. +Configured credentials may be short or coincide with ordinary metadata; their +redaction cannot depend on the value not appearing in a path. -**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. +**Required change:** Apply the existing configured-secret string redactor to +path announcements on both success and failure, including debug locations and +unselected backend credentials. Leave actual filesystem paths and processing data +unchanged. No new redaction implementation is needed. -## R4 — P2: Separate Module Identity from Section-Specific Diagnostic Names +**Acceptance:** A controlled CLI workflow with synthetic credential-bearing report +and debug paths exposes no credential value in stderr, while files are created at +the real selected locations. Preserve typed/numeric report fields and transcript +output behavior. -**Category:** New observation/report attribution defect in Stages 9–10. +## R9 — P2: Retain Completed Validation Decisions on Fatal Return -**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. +**Category:** Incomplete integration of partial validator results in Stage 6. -The subprocess reproduction with one grammar module and one section produced: +**Evidence:** The LLM validator deliberately returns prior batch decisions with +its fatal error in +[`Validate`](../../internal/framework/validators/llm_validators.go#L182). +[`validateSectionCandidates`](../../internal/framework/runner/runner.go#L640) +appends the operation outcomes, but returns on error before incorporating +`vResult.Decisions`. The retained decisions only describe earlier validators, +not completed batches of the validator that was interrupted. -| Reported module instance | Proposal calls | Validation calls | -| --- | ---: | ---: | -| `grammar` | 1 | 0 | -| `grammar:section-0000` | 0 | 1 | +A focused runner probe supplied one completed negative decision and one successful +operation alongside a later fatal error. The runner retained one operation and +zero decisions, while correctly returning the fatal cause and no approved +candidates. The retained validator test already proves that successful earlier +batch decisions reach this boundary; they are lost at the next layer. -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. +As a result, debug ledgers can label a proposal as merely interrupted even when a +validator already rejected it, and omit the completed decision and its rationale. +This conflicts with the selected partial-evidence/debug contract. It does not +currently cause unsafe corrections to be applied. -**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. +**Required change:** Preserve trustworthy completed partial decisions as diagnostic +evidence when a validator returns a fatal error. Keep identity/duplicate checks +appropriate to a partial result, without requiring decisions for unattempted +candidates. Do not turn partial approval into permission to apply a candidate or +weaken fatal cancellation and worker joining. -**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. +**Acceptance:** A runner-level multi-batch workflow with completed decisions +followed by fatal failure/cancellation retains those decisions in the final debug +ledger, retains operation facts and the originating error, and publishes no +completed transcript or unchecked correction. -## R5 — P2: Index Execution Events Instead of Repeatedly Scanning Them +## R10 — P2: Surface Optional Interaction-Artifact Write Failures -**Category:** New report-building performance/maintainability defect in Stage 10. +**Category:** Pre-existing ignored errors left inconsistent with the selected +Stage 11 debug contract. -**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. +**Evidence:** Both +[`GenerateCandidates`](../../internal/framework/proposal_generation/generate.go#L148) +and [`Validate`](../../internal/framework/validators/llm_validators.go#L154) +discard the error from `WriteInteraction`. These errors never reach the CLI's +nonfatal debug diagnostics. The CLI also silently skips configuration-origin and +selected-profile serialization errors in `startDebug`. -A synthetic review probe using one terminal event per call measured: +A controlled local-provider reproduction enabled debug, then placed a regular file +at the module interaction-directory path before returning a valid empty proposal +response. Interaction writes failed, but the process printed only its success +summary and debug location, with no indication that requested evidence was missing. +Exit 0 and unchanged processing are correct; silent evidence loss is not. -| 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 | +**Required change:** Carry optional debug failures to the CLI through a small, +explicit mechanism and emit redacted stderr diagnostics. Preserve nonfatal +behavior, keep failures out of model error/recovery counts, and retain paths only +for artifacts actually written. Handle the analogous serialization failures in +`startDebug`; do not introduce a logging framework or make debug a success gate. -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. +**Acceptance:** A representative failed interaction write produces a safe stderr +diagnostic, while transcript, counts, and exit status match a run without that +failure. Cover both proposal and validator plumbing at their narrow boundaries; +one assembled CLI case is sufficient. Debug-off must still create no artifacts. -**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. +## Verification and Review Limits -**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. +- The full uncached offline suite passed: `go test -count=1 ./...`. +- `go vet ./...` passed. +- Focused race checks passed for runner, LLM, validators, process-report assembly, + diagnostics/output, reporting, and CLI subprocess packages. +- A temporary CLI binary and controlled local HTTP provider reproduced R6, R7, + R8, and R10. No live provider or paid service was used. +- A temporary runner probe reproduced R9; its expected evidence-preservation + assertion failed. The probe and all temporary artifacts were removed. -## 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. -At review time, the retained suite passed despite these defects. Add durable regression -coverage with the fixes, using the narrowest useful production boundaries. - -The initial review changed only this remediation document. 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. - -## Resolution Record - -All five findings are resolved without compatibility layers, new scheduling, -or changes to library ownership. - -### R1 and R2 — Shared Serialization Boundary - -`core/redaction` now sanitizes decoded JSON strings and YAML string nodes, -retaining numeric/boolean types and avoiding mutation of domain objects. -Longest matching credentials take precedence in a single replacement pass; -JSON-escaped credential forms are handled for embedded response content. -Serialization or key-collision failures return no payload. The unredacted report -fallback has been removed. - -The CLI supplies all resolved backend credentials to the external report writer -and run directory. Report copies, ledgers, execution/utilization artifacts, -interaction diagnostics, selected profiles, and error text share the redactor. -ConfigKit still owns credential-reference redaction of effective configuration. - -Evidence: - -- `TestSubprocessRedactsValidModelReasonsOnSuccessAndPartialFailure` exercises - schema-valid negative assessments containing selected and unselected secrets, - including quotes/backslashes. Both reports and retained artifacts are checked; - partial failure preserves earlier decisions and writes no corrected output. -- `TestReportWritersRedactStringsAndFailClosed` exercises both actual report - writers with credential `2`, preserving numeric configuration version 2 while - redacting string content, and verifies failed serialization writes no artifact. -- `TestStringRedactionPreservesSerializationAndDomainData` covers overlapping - secrets, escaped nested content, large numbers, booleans, YAML scalar types, - unchanged domain data, and failures without an original-payload fallback. - -### R3 — Immediate Shared Cancellation - -Validation workers record their error and cancel shared work immediately. -All accesses to the originating-error state use a mutex. Proposal collection -stops launching validation after a recorded failure; active workers are joined -and available results retain deterministic collection order. - -`TestValidatorFailureCancelsActiveAndPendingProviderWork` uses the real PromptKit -runtime with backend concurrency one and queue capacity one. A failing validator -cancels two peer executions, one active in a controlled transport and one pending, -without manually releasing either. The originating error survives, only one -provider attempt occurs, and both logical calls retain finished observations. -The runner and runtime pass the race detector. - -### R4 — Independent Module and Section Identity - -Validation requests carry the configured module instance and an explicit section -index. The structured call forwards both; only interaction filenames combine -module, section, validator, and batch into a diagnostic name. The runner test -helper now uses section index rather than batch index to identify sections. - -`TestSubprocessRepeatedModulesKeepSectionAttribution` runs two grammar instances -across two sections through the actual CLI/runtime. It verifies exactly two -utilization entries with two proposal and two validation calls each, explicit -section/batch identity, four distinct validation response artifacts, and unchanged -proposal ordering in rejected decisions. - -### R5 — Indexed Report Aggregation - -Execution mapping indexes events by call ID once. Utilization directly groups -calls/events by backend and indexes modules by real instance, seeded in module -result order. Neither builder scans all events for each logical call. - -Existing summary tests and `TestIndexedReportsPreserveOrderAndPartialUsage` -verify pre-dispatch failures, original call/event order, backend grouping, and -known-but-incomplete usage. `BenchmarkExecutionReports` retains a reproducible -1,000/10,000/20,000-call benchmark without timing assertions. - -A local three-iteration sample measured 20,000 calls at approximately **43 ms** -for execution mapping and **84 ms** for utilization, compared with the review -probe's 8.08 s and 2.42 s respectively. Timings are illustrative; the direct -indexes establish removal of the quadratic scans independently of machine load. - -### Completion Validation - -- Full offline suite: `go test ./...`. -- Race checks: runner, LLM runtime, process reporting, redaction, and CLI subprocess - packages. -- Static analysis: `go vet ./...`. -- Both required Linux architectures: `scripts/build-release.sh` in a temporary - output directory, without tagging or publishing. -- Documentation/diff whitespace checks. - -Release publication and deferred release-distribution work remain outside this -remediation unit. +Passing retained tests do not cover the five failures above. Add proportionate +regression coverage with their fixes. This review changes only this document; +application fixes, upstream changes, deferred feature work, and release publication +remain outside scope. Target-platform release builds were not repeated for this +review; the native temporary CLI build succeeded.