Record the deletion conflict test gap
This commit is contained in:
@@ -1,187 +1,73 @@
|
|||||||
# Resilience Review: Required Remediation
|
# Span Deletion Review: Required Remediation
|
||||||
|
|
||||||
**Reviewed:** 2026-09-25, source commit `ec78b5a`.
|
**Reviewed:** 2026-09-25, source commit `24b8992`.
|
||||||
**Scope:** Review of the resilience implementation for unintentionally strengthened
|
**Scope:** The four implementation commits after `31979d4`, compared with
|
||||||
invariants, contracts, guarantees, and test requirements, compared with
|
[span-deletion.md](span-deletion.md), [implementation.md](implementation.md),
|
||||||
[resilience.md](resilience.md), [historical implementation plan](https://gitea.maximumdirect.net/eric/audita/src/commit/a83a518/docs/roadmap/implementation.md), and the
|
the agreed user intent, and repository policies.
|
||||||
[agent policy](../policy/agents.md).
|
**Status:** R15 open. Earlier findings R1–R14 are resolved; their evidence and
|
||||||
**Status:** R11–R14 addressed. The findings below describe the reviewed source;
|
resolution records remain in Git history through `24b8992`.
|
||||||
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.
|
|
||||||
|
|
||||||
## Architecture Assessment
|
## 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,
|
Validator requests carry the replacement and full before/after segment text.
|
||||||
bounded retries/repairs, and backend admission.
|
Approved proposals apply sequentially, with applicability rechecked after earlier
|
||||||
- Audita normalizes and chunks once, preserves section identity and stage barriers,
|
edits. The runner distinguishes original proposal defects from later conflicts.
|
||||||
and validates corrections before applying them. Recoverable model failures stay
|
Debug ledger mapping preserves explicit empty proposed/applied replacements and
|
||||||
local; fatal failures cancel peers and join workers.
|
keeps absent values distinct. Existing resilience and reporting boundaries remain.
|
||||||
- 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 findings below concern unnecessary restrictions and incidental test contracts,
|
No production defect or unintended strengthening of the agreed requirements was
|
||||||
not missing resilience machinery. Some restrictions were introduced by the
|
identified in this review. The finding below concerns a regression test that does
|
||||||
implementation plan itself, rather than by a coding agent deviating from it.
|
not actually protect one explicitly agreed reporting distinction.
|
||||||
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.
|
|
||||||
|
|
||||||
## 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)
|
**Evidence:** `TestRunnerApplyConflictAfterTwoInitiallyValidDeletions` in
|
||||||
compares every active destination against every protected input path and rejects
|
[`runner_test.go`](../../internal/framework/runner/runner_test.go) correctly checks
|
||||||
all overlaps, including aliases. The CLI includes the transcript input among
|
that the first deletion applies and the second is skipped to prevent a blank
|
||||||
those protected paths. Collision tests in
|
segment. However, it uses `fakeModule`, whose `Propose` returns proposals without
|
||||||
[`output_session_test.go`](../../internal/core/diagnostics/output_session_test.go)
|
operation outcomes. Its two deterministic validators also return no operations.
|
||||||
enforce rejection for direct, symbolic-link, and hard-link aliases.
|
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
|
In [`runner.go`](../../internal/framework/runner/runner.go),
|
||||||
report initialization. It did not require forbidding intentional replacement of
|
`mergeFilteredProposals` can attach a recovery only to a successful proposal
|
||||||
the transcript after it has been read and successfully processed. The broad
|
operation. Without that operation, even incorrectly attributing the conflict as
|
||||||
prohibition appeared in the implementation plan and should not become a lasting
|
a defective proposal cannot produce the recovery the test is meant to detect.
|
||||||
product restriction solely on that basis.
|
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.
|
**Required change:** Revise this existing test to supply a successful proposal
|
||||||
Allow intentional in-place transcript output after successful processing, while
|
operation alongside its two candidates, using the existing `outcomeModule` helper
|
||||||
retaining checks for destructive report/input collisions and incompatible output
|
or equivalent small fixture. Assert that the proposal operation is present before
|
||||||
destinations. Keep the change proportional; no filesystem transaction or
|
checking that it has no failure, filtering recovery, or filtered-proposal records.
|
||||||
concurrent-host-mutation protection framework is needed. Update affected contract
|
Retain the existing final-text and skipped-decision assertions. No production
|
||||||
documentation and tests when the behavior changes.
|
change, additional CLI scenario, or new test framework is needed.
|
||||||
|
|
||||||
**Acceptance:** A representative CLI workflow can read and successfully replace
|
**Acceptance:** The revised test passes with the current production behavior and
|
||||||
its input transcript. Preparation does not truncate the source, and failed
|
fails if an initially valid deletion made unsafe by an earlier edit is incorrectly
|
||||||
processing does not publish a replacement. Alias checks do not impose a blanket
|
recorded as proposal filtering. It continues to prove that only the first deletion
|
||||||
ban on otherwise valid in-place output. Required report destinations still cannot
|
applies and that the second remains visible as a skipped decision. Fix the existing
|
||||||
destroy inputs during initialization or conflict with transcript publication.
|
test rather than adding overlapping coverage.
|
||||||
|
|
||||||
## R12 — P2: Protect Only Profiles That Were Actually Loaded
|
## Review Verification and Limits
|
||||||
|
|
||||||
**Category:** Speculative protected-path restriction.
|
- `go test ./...` passed against the committed implementation.
|
||||||
|
- Reviewed schema, domain checks, prompt changes, validator payloads, sequential
|
||||||
**Evidence:** [`selectedProfilePaths`](../../internal/cli/process.go) constructs
|
application/accounting, debug serialization, new tests, and updated contracts.
|
||||||
both `.yaml` and `.yml` paths for each selected profile, regardless of which file
|
- The focused overlay experiment passed despite deliberately removing the
|
||||||
was loaded or whether the alternate exists. These paths enter the output-session
|
conflict-classification guard, confirming R15. The overlay existed only in a
|
||||||
protected-input set alongside actual configuration sources.
|
temporary directory and did not modify repository source.
|
||||||
|
- No live-provider evaluation was performed. Offline fixtures establish pipeline
|
||||||
As a result, an unused alternate profile filename is reserved as though it were
|
behavior, not a guarantee that a model will always propose or approve good edits.
|
||||||
an input. This exceeds the intended protection of known loaded sources and can
|
- This review records remediation only; it does not implement the test correction
|
||||||
reject an otherwise valid output selection.
|
or any deferred feature.
|
||||||
|
|
||||||
**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.
|
|
||||||
|
|||||||
Reference in New Issue
Block a user