diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index c7f9859..2de813a 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,4 +1,106 @@ -# Transcription Checkpoint Review Remediation +# Audita v2 and ConfigKit Migration Review Remediation + +Status: Pending. + +Reviewed `83eaa96` through `fae18bf` against the approved clean-cutover decisions, +[feature roadmap](audita.md), and [implementation plan](implementation.md). +Review date: 2026-09-25. No production changes were made. The previous completed +transcription review is retained below as historical context. + +## 1. Preserve omitted modules in direct YAML serialization + +Priority: P2. Status: Pending. + +Location: `internal/config/credential_projection.go:25` (`AuditaConfig.MarshalYAML`), +especially the plain slice field at line 30 and assignment at line 40. + +The direct safe YAML projection passes a nil module slice to yaml.v3, which +serializes it as `modules: []`. Loading that output produces a non-nil empty +slice. Under the new Audita contract these are different instructions: nil +delegates to Audita's configuration/default sequence; empty passes an explicit +empty module argument and disables correction modules. Serialization therefore +silently changes behavior for a configuration that is subsequently reloaded. + +Evidence: a temporary package test marshaled +`AuditaConfig{Binary: "audita", Timeout: "3h"}`, unmarshaled it into another +`AuditaConfig`, and required `Modules == nil`. It failed with output containing +`modules: []`. The test was removed after reproduction. This concerns direct +`yaml.Marshal` of the typed configuration; ordinary CLI `config show` uses +`MarshalEffectivePipeline` through a separate JSON/value-tree projection and +does not use this faulty path. + +Required remediation: + +- Make the direct safe YAML projection preserve nil as null or omission and + explicit empty as `[]`. Do not simply add `omitempty` to the slice: that would + instead erase the explicit empty selection. +- Keep credential serialization name-only and preserve module order/repetitions. +- Add a small round-trip regression covering nil, explicit empty, and an ordered + repeated selection; retain effective-output inspection coverage without + duplicating its whole test matrix. + +## 2. Remove the unapproved manifest compatibility decoder + +Priority: P2. Status: Pending. + +Location: `internal/manifest/manifest.go:79-89`, +`internal/manifest/store_test.go:78`, and `docs/internal/manifest.md`'s decoding +contract. Also correct the conflicting migration language in the plan/roadmap. + +The new `EffectiveConfigProvenance.UnmarshalJSON` bypasses the manifest store's +`DisallowUnknownFields` by using permissive `json.Unmarshal` for the entire +provenance object. It accepts retired `selected_profile` data and silently +discards it. It also silently accepts unknown current provenance fields, +including unknown nested selection fields, rather than limiting behavior to +the renamed label. + +The user expressly selected a clean cutover without backward-compatibility +shims. This adds precisely such a shim and a new historical-input acceptance +guarantee. The regression test explicitly requires the old selection to load, +and the internal guide now declares permissive informational-field decoding. + +The plan contributed to this divergence: it said existing generic decoding +would handle historical optional provenance and required old manifests to load. +That assumption was false; the baseline store already strictly decoded these +fields. Repetition in the plan, tests, and documentation does not authorize +overriding the user's clean-cutover decision. + +Required remediation: + +- Remove the custom compatibility unmarshaler and retain ordinary strict typed + decoding for the new `selected_overlay` representation. Do not add dual fields, + special retired-field handling, automatic manifest rewrites, or a format-version + bump just for this rename. +- Replace the historical-acceptance test with coverage for current provenance + round trips and unknown/retired provenance rejection through the real store. +- Correct the plan and current documentation to acknowledge the upgrade effect: + manifests with retired provenance require deliberate operator migration before + loading. Document narrow removal/renaming of retired informational provenance + on a backed-up manifest, preserving execution/checkpoint state; do not advise + deleting all session progress or rerunning everything. +- Keep provenance informational: this correction does not authorize adding it + to semantic resume keys or invalidating work based on overlay identity alone. + +If transparent historical-manifest acceptance is desired instead, obtain an +explicit exception to the clean-cutover decision before retaining a compatibility +reader. Do not treat that exception as already granted. + +## Review verification + +- Existing `go test ./...` passed. +- Race tests passed for subprocess, Audita, Notarius, config, stage, app, and + manifest packages after removing the temporary reproduction. +- `go vet ./...` passed. +- The temporary direct-YAML regression failed as described above. +- Source inspection and the new historical-provenance test establish finding 2; + `LocalStore.LoadReader` still uses strict decoding outside the custom hook. + +The primary architecture is otherwise in place: ConfigKit v0.4.0 overlays, +shared named credential forwarding/redaction, a v2-only Audita subprocess +adapter, ordinary reports as diagnostics, and versioned ordered-module polish +evidence. No live model/provider runs were performed during this review. + +## Historical completed transcription review Status: Complete. @@ -7,7 +109,7 @@ upload-replay roadmap and the original user request. No production changes were made during review. The two failures below were reproduced with temporary tests using the existing stage fixtures; those temporary tests were removed afterward. -## 1. Reconcile Aggregate State Even When No Speaker Needs HTTP Work +### 1. Reconcile Aggregate State Even When No Speaker Needs HTTP Work Priority: P2. Status: Complete. @@ -54,7 +156,7 @@ an added speaker), plus incomplete/inconsistent aggregate output coverage. Exercise the runner skip path and assert zero additional WhisperX requests, correct rebuilt outputs, and no removed speaker reaching merge. -## 2. Remove the Unrequested Positive Audio-Size Requirement +### 2. Remove the Unrequested Positive Audio-Size Requirement Priority: P2. Status: Complete. @@ -94,7 +196,7 @@ Verification: cover a zero-size record round-trip and successful adapter result being checkpointed/reused for that input; retain negative-size rejection and ordinary downstream-failure tests. -## Verification After Remediation +### Verification After Remediation Completed on 2026-09-22 with Go 1.26.8: focused stage/application tests, changed-package race tests, `go test ./...`, `go test -race ./...`,