151 lines
8.4 KiB
Markdown
151 lines
8.4 KiB
Markdown
# Resilience 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), [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 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 resilience architecture remains substantially implemented:
|
||
|
||
- 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 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.
|
||
|
||
## R11 — P2: Allow Intentional In-Place Transcript Output
|
||
|
||
**Category:** Overly broad filesystem collision restriction.
|
||
|
||
**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.
|
||
|
||
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:** 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 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.
|
||
|
||
## R12 — P2: Protect Only Profiles That Were Actually Loaded
|
||
|
||
**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.
|
||
|
||
## Verification and Review 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.
|