From 5e2e863fbddb6d5385cce3ca0c55f810686e2290 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Fri, 25 Sep 2026 03:03:14 +0000 Subject: [PATCH] Record the deletion conflict test gap --- docs/roadmap/remediation.md | 232 +++++++++--------------------------- 1 file changed, 59 insertions(+), 173 deletions(-) diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index 5dfc27a..a13f3d6 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,187 +1,73 @@ -# Resilience Review: Required Remediation +# Span Deletion Review: Required Remediation -**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), [historical implementation plan](https://gitea.maximumdirect.net/eric/audita/src/commit/a83a518/docs/roadmap/implementation.md), and the -[agent policy](../policy/agents.md). -**Status:** R11–R14 addressed. The findings below describe the reviewed source; -see the resolution record for the implemented changes and verification. - -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. +**Reviewed:** 2026-09-25, source commit `24b8992`. +**Scope:** The four implementation commits after `31979d4`, compared with +[span-deletion.md](span-deletion.md), [implementation.md](implementation.md), +the agreed user intent, and repository policies. +**Status:** R15 open. Earlier findings R1–R14 are resolved; their evidence and +resolution records remain in Git history through `24b8992`. ## Architecture Assessment -The intended resilience architecture remains substantially implemented: +The intended span-deletion behavior is implemented without introducing another +processing path. PromptKit accepts required string replacements including the +empty string; Audita retains responsibility for literal matching, module-specific +validation, and safe application. Nonempty whitespace-only original spans are +eligible. The renamed resulting-segment validator and application guard prevent +blank segments, while existing module scopes and replacement policies remain. -- 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. +Validator requests carry the replacement and full before/after segment text. +Approved proposals apply sequentially, with applicability rechecked after earlier +edits. The runner distinguishes original proposal defects from later conflicts. +Debug ledger mapping preserves explicit empty proposed/applied replacements and +keeps absent values distinct. Existing resilience and reporting boundaries remain. -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. +No production defect or unintended strengthening of the agreed requirements was +identified in this review. The finding below concerns a regression test that does +not actually protect one explicitly agreed reporting distinction. -## R11 — P2: Allow Intentional In-Place Transcript Output +## R15 — P2: Exercise Operation Accounting in the Deletion Conflict Test -**Category:** Overly broad filesystem collision restriction. +**Category:** Ineffective regression coverage for conflict classification. -**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. +**Evidence:** `TestRunnerApplyConflictAfterTwoInitiallyValidDeletions` in +[`runner_test.go`](../../internal/framework/runner/runner_test.go) correctly checks +that the first deletion applies and the second is skipped to prevent a blank +segment. However, it uses `fakeModule`, whose `Propose` returns proposals without +operation outcomes. Its two deterministic validators also return no operations. +The final loop asserting absence of errors/recoveries therefore checks an empty +operation list. -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. +In [`runner.go`](../../internal/framework/runner/runner.go), +`mergeFilteredProposals` can attach a recovery only to a successful proposal +operation. Without that operation, even incorrectly attributing the conflict as +a defective proposal cannot produce the recovery the test is meant to detect. +A temporary Go build overlay that removed the initially-applicable guard in +`attributeApplyFiltering` still passed this exact test. The production guard was +not changed and appears correct. -**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. +**Required change:** Revise this existing test to supply a successful proposal +operation alongside its two candidates, using the existing `outcomeModule` helper +or equivalent small fixture. Assert that the proposal operation is present before +checking that it has no failure, filtering recovery, or filtered-proposal records. +Retain the existing final-text and skipped-decision assertions. No production +change, additional CLI scenario, or new test framework is needed. -**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. +**Acceptance:** The revised test passes with the current production behavior and +fails if an initially valid deletion made unsafe by an earlier edit is incorrectly +recorded as proposal filtering. It continues to prove that only the first deletion +applies and that the second remains visible as a skipped decision. Fix the existing +test rather than adding overlapping coverage. -## R12 — P2: Protect Only Profiles That Were Actually Loaded +## Review Verification and Limits -**Category:** Speculative protected-path restriction. - -**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. - -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:** 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:** 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. - -## R13 — P2: Do Not Limit Transcript Destinations to Regular Files - -**Category:** New output-interface restriction introduced by replacement mechanics. - -**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. - -**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. - -**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. - -## R14 — P2: Test Markdown Semantics Without Freezing Incidental Prose - -**Category:** Over-specified presentation tests and redundant assertions. - -**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. - -**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. - -**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. - -## Original Review Verification and Limits - -- 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. - -## Resolution Record - -- **R11:** Output selection distinguishes the transcript input from other protected - sources. Final transcript publication may replace its input; early report writes - still cannot overwrite it, and other source/output collisions remain checked. - `TestSubprocessInPlaceTranscript` proves corrected output replaces the source on - success and an invalid source remains untouched on failure. Output-session tests - cover direct and alias acceptance, nontruncating preparation, and rejection of - report/input aliases and conflicting output destinations. -- **R12:** The runtime records successful reads through PromptKit's existing profile - filesystem and supplies actual loaded paths to CLI. It no longer guesses two - filenames per profile ID. `TestSubprocessProfileSources` proves an unused - alternate filename is allowed and a loaded profile with a different filename - remains protected from report initialization. Configuration sources continue to - come from ConfigKit's source records. -- **R13:** Regular transcript files retain per-file replacement. Existing device - and pipe destinations receive ordinary writes during finalization and are not - opened during preflight. `TestTranscriptStreamDestinations` verifies `/dev/null` - and named-pipe delivery without replacing either destination, including pipe - preparation before opening its reader. `TestTranscriptStreamWriteFailure` - checks a real stream write failure using `/dev/full` where available. Existing - directory, parent, permission, cleanup, and late-report-failure tests remain. -- **R14:** Removed the whole-document Markdown golden fixture and overlapping - matrix assertions. Tests compare complete ordered table rows, summary quantities, - reason counts, escaping/redaction, and YAML/CLI count agreement. Summary prose is - not snapshotted, and empty-work handling no longer requires an exact sentence. - No renderer or output schema changed. - -Current operations and internal documentation describe the revised boundaries. -The completed implementation plan explicitly identifies its superseded choices. - -Validation: `go test ./...` and `go vet ./...` passed, as did focused race checks -for the LLM runtime, diagnostics/output, and CLI subprocess packages. The refined -Markdown assertions passed their package tests. Changed-document links and -`git diff --check` passed. No live provider, dependency change, tag, or publication -was involved. +- `go test ./...` passed against the committed implementation. +- Reviewed schema, domain checks, prompt changes, validator payloads, sequential + application/accounting, debug serialization, new tests, and updated contracts. +- The focused overlay experiment passed despite deliberately removing the + conflict-classification guard, confirming R15. The overlay existed only in a + temporary directory and did not modify repository source. +- No live-provider evaluation was performed. Offline fixtures establish pipeline + behavior, not a guarantee that a model will always propose or approve good edits. +- This review records remediation only; it does not implement the test correction + or any deferred feature.