From efaefdbf3d9ba6634c8819e6d6c5f4479e44d2ce Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 25 Sep 2026 00:30:23 +0000 Subject: [PATCH] Record unnecessary restrictions found in the resilience review --- docs/roadmap/remediation.md | 323 +++++++++++++----------------------- 1 file changed, 116 insertions(+), 207 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index e916806..d71d544 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,241 +1,150 @@ # Resilience Review: Required Remediation -**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:** R6–R10 addressed and verified. The findings below describe the reviewed -source; their line references are historical. See the resolution record below. +**Reviewed:** 2026-09-25, source commit `ec78b5a`. +**Scope:** Review of the resilience implementation for unintentionally strengthened +invariants, contracts, guarantees, and test requirements, compared with +[resilience.md](resilience.md), [implementation.md](implementation.md), and the +[agent policy](../policy/agents.md). +**Status:** R11–R14 remain open. This document records required follow-up work; +no application behavior has changed as part of this review. -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). +The earlier findings R1–R10 have been addressed. Their findings and resolution +records remain in Git history; R6–R10 were resolved in `ec78b5a`. Numbering continues here to distinguish new work from those resolved findings. ## Architecture Assessment -The intended architecture is substantially implemented: +The intended resilience architecture remains substantially implemented: -- 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. +- ConfigKit owns configuration loading and credentials; PromptKit owns transport, + bounded retries/repairs, and backend admission. +- Audita normalizes and chunks once, preserves section identity and stage barriers, + and validates corrections before applying them. Recoverable model failures stay + local; fatal failures cancel peers and join workers. +- Processing outcomes feed separate report mapping and presentation. Ordinary + YAML reports and Markdown matrices share counts with the CLI summary; debug + persistence is independently selectable. +- The output session prepares destinations, initializes reports, and publishes + required outputs. Optional debug failures remain nonfatal and visible. -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. +The findings below concern unnecessary restrictions and incidental test contracts, +not missing resilience machinery. Some restrictions were introduced by the +implementation plan itself, rather than by a coding agent deviating from it. +Preserve validation-before-application, fatal cancellation, required-report +initialization and finalization checks, ordinary file permissions, configured- +secret redaction, and independent debug selection while addressing these findings. -## R6 — P1: Honor Restrictive Creation Permissions +## R11 — P2: Allow Intentional In-Place Transcript Output -**Category:** New filesystem/security regression in Stage 10. +**Category:** Overly broad filesystem collision restriction. -**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. +**Evidence:** [`PrepareOutput`](../../internal/core/diagnostics/output_session.go) +compares every active destination against every protected input path and rejects +all overlaps, including aliases. The CLI includes the transcript input among +those protected paths. Collision tests in +[`output_session_test.go`](../../internal/core/diagnostics/output_session_test.go) +enforce rejection for direct, symbolic-link, and hard-link aliases. -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. +The required protection was to avoid destroying inputs during preflight or early +report initialization. It did not require forbidding intentional replacement of +the transcript after it has been read and successfully processed. The broad +prohibition appeared in the implementation plan and should not become a lasting +product restriction solely on that basis. -**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. +**Required change:** Distinguish transcript publication from early report writes. +Allow intentional in-place transcript output after successful processing, while +retaining checks for destructive report/input collisions and incompatible output +destinations. Keep the change proportional; no filesystem transaction or +concurrent-host-mutation protection framework is needed. Update affected contract +documentation and tests when the behavior changes. -**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. +**Acceptance:** A representative CLI workflow can read and successfully replace +its input transcript. Preparation does not truncate the source, and failed +processing does not publish a replacement. Alias checks do not impose a blanket +ban on otherwise valid in-place output. Required report destinations still cannot +destroy inputs during initialization or conflict with transcript publication. -## R7 — P2: Do Not Report Success Without Publishing a Transcript +## R12 — P2: Protect Only Profiles That Were Actually Loaded -**Category:** New destination-selection inconsistency in Stages 10/12. +**Category:** Speculative protected-path restriction. -**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. +**Evidence:** [`selectedProfilePaths`](../../internal/cli/process.go) constructs +both `.yaml` and `.yml` paths for each selected profile, regardless of which file +was loaded or whether the alternate exists. These paths enter the output-session +protected-input set alongside actual configuration sources. -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. +As a result, an unused alternate profile filename is reserved as though it were +an input. This exceeds the intended protection of known loaded sources and can +reject an otherwise valid output selection. -**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. +**Required change:** Derive protected profile paths from actual loaded sources, +using the existing profile-loading boundary rather than reserving hypothetical +alternates. Retain protection appropriate to real inputs and the publication +phase, consistent with R11. Do not introduce a second profile-discovery policy. -**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. +**Acceptance:** An unused alternate profile filename is not rejected solely +because its stem matches a selected profile. The actual loaded profile remains +protected from destructive early report writes. Configuration-source protection +continues to use actual loaded sources. -## R8 — P2: Redact Paths Printed to Stderr +## R13 — P2: Do Not Limit Transcript Destinations to Regular Files -**Category:** New report-location bypass plus an unclosed debug-location gap in -the Stage 11/12 redaction boundary. +**Category:** New output-interface restriction introduced by replacement mechanics. -**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. +**Evidence:** [`preflightTranscript`](../../internal/core/diagnostics/output_session.go) +rejects existing destinations that are not regular files. The previous ordinary +transcript writer did not impose that restriction, allowing destinations such as +`/dev/null` and named pipes. Per-file atomic replacement is useful for regular +files, but does not justify narrowing every transcript destination to that kind +of file. -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. +**Required change:** Separate regular-file replacement from writing to supported +stream destinations. Preserve normal write-error handling and directory rejection; +do not attempt to rename over a device or pipe. Apply preparation checks relevant +to the selected destination, without requiring regular-file replacement machinery +for stream output. Keep ordinary report-file handling unchanged. -**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. +**Acceptance:** Transcript output to `/dev/null` succeeds without replacing the +device. A representative named-pipe workflow can deliver the transcript to its +reader without replacing the pipe. Preparation does not open a pipe merely to +probe it and thereby unnecessarily wait for a reader. Regular-file publication, +permission handling, directory rejection, and real write-failure reporting remain +covered by focused tests. -**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. +## R14 — P2: Test Markdown Semantics Without Freezing Incidental Prose -## R9 — P2: Retain Completed Validation Decisions on Fatal Return +**Category:** Over-specified presentation tests and redundant assertions. -**Category:** Incomplete integration of partial validator results in Stage 6. +**Evidence:** [`markdown_test.go`](../../internal/core/reporting/markdown_test.go) +compares the complete rendered report byte-for-byte with a golden file, then +checks overlapping exact fragments. Additional assertions depend on exact prose. +This treats incidental wording and whitespace as durable contracts beyond the +selected requirements for complete fixed matrices, ordering, counts, states, +escaping, and readability. -**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. +**Required change:** Prefer semantic assertions for the intended report contract, +or limit a retained layout golden to the table structure whose layout matters. +Remove redundant assertions and exact-prose requirements that do not protect +meaningful behavior. Do not weaken the selected fixed-layout reporting contract +or introduce adaptive formatting as part of this cleanup. -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. - -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:** 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 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. - -## R10 — P2: Surface Optional Interaction-Artifact Write Failures - -**Category:** Pre-existing ignored errors left inconsistent with the selected -Stage 11 debug contract. - -**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 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. - -**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. - -**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. +**Acceptance:** Tests still detect missing or misordered stage/chunk rows and +columns, incorrect error/recovery counts or states, and broken table escaping. +Harmless prose or whitespace edits outside the required layout no longer require +updating a whole-document fixture and overlapping assertions. Retain meaningful +agreement checks between YAML, Markdown, and CLI totals. ## Verification and Review Limits -- 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. - -At review time, the retained tests did not cover the five failures above. -The initial review changed 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. - - -## Resolution Record - -All five findings are addressed without changing retry budgets, library ownership, -correction acceptance, or the independent reporting/debug selections. - -- **R6:** Per-file replacement uses ordinary file creation inside a private - temporary directory in the destination parent. Creation applies the umask; - existing permission bits are restored only for existing destinations. Temporary - files/directories are removed on success and failure. The subprocess permission - test covers both restrictive and ordinary masks and subsequent replacement of - restrictive files; existing output-session failure/cleanup tests still pass. -- **R7:** File selection uses the literal nonempty output path throughout report - placement and output preparation. Final CLI stream selection uses the prepared - output session. `TestSubprocessWhitespaceOutputIsALiteralFile` proves the output - and reports are produced; the existing stdout integration test remains passing. -- **R8:** Every CLI report/debug location announcement uses the shared configured- - secret string redactor. `TestSubprocessPathAnnouncementsAreRedacted` covers - success and failure with selected and unselected credentials in real paths, - while confirming that the real destinations are used. -- **R9:** Fatal validator returns use partial identity/cardinality validation, - retain completed decisions and rejection details, and never return application - approvals. `TestFatalBatchRetainsEarlierDecisionsInLedger` exercises the real - runner and batching validator with two completed batches and a third fatal - failure, retaining their decisions and all operation facts in the debug ledger. - A separate runner regression rejects unknown/duplicate partial identities while - retaining the originating failure. Existing cancellation/joining tests pass. -- **R10:** An explicit diagnostic-error callback connects proposal and validator - writers to the CLI's synchronized, redacted nonfatal diagnostics. Origin/profile - serialization failures use the same path. Narrow proposal/validator tests and - `TestSubprocessInteractionWriteFailureIsVisibleAndNonfatal` verify visible - failures, unchanged transcript/counts/exit status, and no fabricated artifact - paths. Existing reporting/debug-selection tests verify debug-off persistence - remains disabled. - -Validation completed: full `go test ./...`, `go vet ./...`, focused race tests for -runner, validators, proposal generation, report assembly, diagnostics/output, CLI, -and subprocess integration; required Linux amd64/arm64 builds in a temporary -output directory; and documentation link/whitespace checks. No live provider, -upstream change, tag, or publication was involved. +- This was a source and contract review; no application changes or new runtime + reproductions were made during this pass. +- The preceding remediation passed the full Go test suite, vet, focused race + checks, and Linux amd64/arm64 builds. Those checks were not repeated for this + documentation-only review and do not establish that R11–R14 are resolved. +- Several current tests deliberately enforce the restrictions identified above; + passing them does not establish that those restrictions were intended. +- The prior race checks and target-platform builds were proportionate to the + resilience refactor. They should not be promoted into mandatory gates for every + subsequent edit solely because they were used there. +- Application fixes, upstream changes, deferred feature work, and release + publication remain outside this document update.