200 lines
11 KiB
Markdown
200 lines
11 KiB
Markdown
# 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:** 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 architecture is 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.
|
||
|
||
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.
|
||
|
||
## R6 — P1: Honor Restrictive Creation Permissions
|
||
|
||
**Category:** New filesystem/security regression in Stage 10.
|
||
|
||
**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.
|
||
|
||
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.
|
||
|
||
**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.
|
||
|
||
**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.
|
||
|
||
## R7 — P2: Do Not Report Success Without Publishing a Transcript
|
||
|
||
**Category:** New destination-selection inconsistency in Stages 10/12.
|
||
|
||
**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.
|
||
|
||
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.
|
||
|
||
**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.
|
||
|
||
**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.
|
||
|
||
## R8 — P2: Redact Paths Printed to Stderr
|
||
|
||
**Category:** New report-location bypass plus an unclosed debug-location gap in
|
||
the Stage 11/12 redaction boundary.
|
||
|
||
**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.
|
||
|
||
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:** 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:** 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.
|
||
|
||
## R9 — P2: Retain Completed Validation Decisions on Fatal Return
|
||
|
||
**Category:** Incomplete integration of partial validator results in Stage 6.
|
||
|
||
**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.
|
||
|
||
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.
|
||
|
||
## 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.
|
||
|
||
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.
|