diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 892488e..60fb490 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -1,341 +1,431 @@ -# Workstream 9 Implementation Plan — Cleanup As The Final Pipeline Stage +# Workstream 10 Implementation Plan — Quality, Testability, And Documentation Alignment ## Status -Planned implementation for Workstream 9, **Cleanup as the final pipeline -stage**, in [`refactoring.md`](refactoring.md). Workstreams 1 through 8 are -complete prerequisites. This plan does not independently undertake Workstream -10; its test and documentation requirements apply only where this work changes -the owning behavior. +Planned implementation for Workstream 10, **Quality, testability, and +documentation alignment**, in [`refactoring.md`](refactoring.md). Workstreams 1 +through 9 are complete prerequisites. This is the final actionable workstream +in the consolidated refactoring roadmap; it closes cross-cutting gaps that were +not already completed alongside the behavior-owning workstreams. ## Authoritative Scope And Fixed Decisions -Workstream 9 in [`refactoring.md`](refactoring.md) is authoritative. Its -supporting records are H20 and H21 in [`audit.md`](audit.md), Findings 2 and 4 -in [`contract-audit.md`](contract-audit.md), and C02 and C04 in -[`audit-decisions.md`](audit-decisions.md). Read those cited sections rather -than rereading or reinterpreting the complete audits. +Workstream 10 in [`refactoring.md`](refactoring.md) is authoritative. Its +supporting first-pass findings are H07, H08, H12, H13, H17, H18, H23, and H25 +in [`audit.md`](audit.md). The corresponding contract decisions are C01 through +C06 in [`audit-decisions.md`](audit-decisions.md); their full rationale remains +in [`contract-audit.md`](contract-audit.md). Read those cited sections rather +than re-auditing unrelated hypotheses. -The following decisions are final: +Most of Workstream 10's risk coverage and documentation alignment landed with +Workstreams 1 through 9. Preserve that completed work. The remaining decisions +are: -- Add `cleanup` after `notify` as the twelfth and final canonical stage. - `publish` commits remote state, `notify` completes delivery behavior, and - only then may `cleanup` remove local run data. -- Retain the existing configuration names - `pipeline.spool.delete_audio_after_publish` and - `pipeline.workspace.cleanup_after_publish`. They select cleanup-stage - targets; they no longer cause a runner-finalization side effect. -- When neither policy is enabled, `cleanup` returns an explicit self-skip with - no outputs. Self-skips remain reconsidered on later invocations under the - ordinary runner contract. -- When either policy is enabled, cleanup requires a succeeded `publish` stage - whose metadata proves a completed pointer-selected upload: `uploaded=true`, - `skipped` is not true, and canonical nonempty `published_run_id`, - `remote_commit_key`, and `current_commit_pointer_key` values agree with the - manifest's campaign/session remote prefix. Missing, malformed, skipped, or - disabled publication evidence is an error and no deletion begins. -- The succeeded publish record is sufficient durable evidence. Cleanup does - not reread object storage: publish success is recorded only after the pointer - write, immutable run objects remain durable if a later pointer supersedes - them, and adding a network dependency would make local retry less reliable. -- Compute targets from the validated published run ID, manifest campaign and - session identities, and the currently resolved application-owned workspace - and spool roots using canonical artifact path helpers. Never use the new - invocation's `RunID`, `LocalWorkDir`, or `LocalSpoolDir`, and never accept a - path from publish metadata as a deletion target. -- Process requested targets deterministically in spool-then-workspace order. - Use `fileops.RemoveAllUnderRoot`; a missing root or target is successful. - Preserve refusal of a root itself, traversal, and out-of-root deletion. -- Do not record per-target progress. If one deletion succeeds and a later one - fails, the ordinary stage record becomes failed. A later run recomputes the - same canonical targets, treats the first as already absent, and retries the - remainder. -- A successful cleanup produces no canonical artifacts. Its bounded metadata - may identify the published run and completed policy names, but must not - become a second cleanup ledger. -- Cleanup has a semantic fingerprint over the two policy booleans and their - normalized configured roots. Its resume validator revalidates publish - evidence and confirms every enabled canonical target is absent. Recreated or - newly enabled targets make a prior success non-resumable. -- Reject statically impossible configuration: either cleanup policy requires - publish to be enabled with run upload enabled. Dynamic absence or corruption - of publish evidence remains a cleanup-stage error. -- `notify -> cleanup` is the final direct invalidation edge and `cleanup` has - no dependents. This makes every earlier prerequisite transitively stale a - prior cleanup result while retaining the fixed relation separately from - execution order. -- A default full run includes cleanup. `--through notify`, any earlier bounded - range, `narratio publish`, and artifact-regeneration aliases do not perform - cleanup. `narratio run-stage cleanup SESSION` is the explicit single-stage - retry path and applies cleanup's own publication-evidence checks. -- Remove `post_publish_cleanup` completely. Do not migrate, reinterpret, or - retain its types, validators, runner hooks, restore clearing, fixtures, or - tests. Keep session manifest schema version `narratio.session-manifest.v1`: - the retired field was optional, and strict decoding will reject manifests - that actually contain it without invalidating otherwise canonical v1 state. +- Do not reopen completed product, compatibility, filesystem, manifest, + analyze, configuration, publication, lock, or cleanup decisions. This + workstream changes internal testability and test diagnostics without changing + CLI, configuration, artifact, manifest, integration, publish, restore, or + cleanup behavior. +- Remove the seven remaining mutable application function seams: + `readSecretsDirectoryEntries`, `loadPipelineConfigFn`, + `downloadObjectToTempFn`, `newObjectStoreFromConfigFn`, + `discoverRemoteCurrentStateFn`, `buildRestorePlanFn`, and + `executeRestorePlanFn`. Compiled regular-expression values, canonical stage + relations, and fixed command-name data are not collaborator seams and are not + part of this removal. +- Use one package-private, value-owned `commandDependencies` set for these + collaborators. A normalization function supplies production defaults for nil + fields. Each command invocation receives its own value; there is no mutable + default singleton, context-carried service locator, dependency-injection + framework, or new public interface. Keep runner `invocationDependencies` + separate because it owns a different lifecycle. +- Preserve exported command entry points and their existing signatures. + Production wrappers use normalized defaults. Private dependency-aware entry + points let tests supply only the external or phase boundary they control. + Thread the dependency value explicitly through application-owned command, + configuration-loading, secret-loading, object-store, and restore helpers. +- Tests must never replace or restore package-level functions. Use dependency + values, real temporary filesystems, `storage.FakeBackend`, and existing + controlled subprocess helpers at their durable boundaries. Process + environment mutation remains permitted where credential behavior itself is + under test, but such tests must remain non-parallel and restore state through + `t.Setenv` or the existing focused helper. +- Add `github.com/google/go-cmp/cmp` only for the audit's selected large + structural comparisons: composed configuration values, analyze plans/state, + restore plans/actions, and publish output plans/mappings. Preserve nil versus + empty and ordering semantics unless a domain contract explicitly equates + them. Use direct comparison or `slices.Equal` for simple comparable values; + do not mechanically replace every `reflect.DeepEqual` call. +- Apply `slices` or `maps` in production only when code touched by this + workstream performs a genuinely shallow clone, membership check, or + deterministic key operation with identical nil, empty, and ordering + behavior. Do not conduct a repository-wide style rewrite and do not replace + domain normalization, deep copies, provenance merges, or topological order. +- Retain the focused behavior coverage already established for atomic file + visibility, bounded reads and adapter diagnostics, group modes, validated + downloads, Notarius rename, destructive confinement, restore symlink + conflicts, secret protection, session-only progress, per-artifact analyze + checkpoints, planning/execution parity, remote-lock carry-forward, and + idempotent cleanup. Delete only dead helpers and reset choreography associated + with the removed mutable seams; do not delete a current-contract test merely + to reduce test lines. +- Update only canonical internal documentation whose implemented dependency or + test boundary changes. Because public behavior is unchanged, do not churn + CLI, configuration, operations, troubleshooting, integration, examples, or + release documentation unless implementation uncovers a factual inaccuracy in + its owning document. ## Instructions For Every Stage Before each stage: 1. Read `docs/development.md`, all documents under `docs/policy/`, this plan, - Workstream 9, and only its cited audit sections. Read the affected portions - of [`../internal/overview.md`](../internal/overview.md), - [`../internal/workspace.md`](../internal/workspace.md), - [`../internal/manifest.md`](../internal/manifest.md), - [`../internal/stage-publish.md`](../internal/stage-publish.md), - [`../config.md`](../config.md), [`../cli.md`](../cli.md), and - [`../operations.md`](../operations.md). -2. Inspect affected production code and focused tests. Prefer the codebase - knowledge graph for Go discovery and call tracing; use text search for - configuration, schemas, fixtures, documentation, and removal checks. + Workstream 10 in [`refactoring.md`](refactoring.md), and only its cited audit + sections. Read [`../internal/overview.md`](../internal/overview.md) and, for + restore changes, [`../internal/command-restore.md`](../internal/command-restore.md). +2. Inspect the affected production code and focused tests. Prefer the codebase + knowledge graph for Go definitions and call paths; use text search for + mutable assignments, test reset choreography, documentation, fixtures, and + final removal checks. 3. Confirm worktree state and preserve unrelated changes. Do not implement - Workstream 10 work that is not made necessary by this cleanup change. + release work or any deferred/no-action audit hypothesis. -During each stage, use ordinary stage and manifest transitions, keep paths -root-confined, preserve group-owned workspace behavior, avoid raw secrets, -honor context before destructive work, and keep tests deterministic and -offline. Prefer real temporary filesystems over private deletion hooks. Add -tests for meaningful destructive, recovery, range, and persistence boundaries; -delete tests whose only subject is the retired obligation choreography. Run -`gofmt` on changed Go files and `git diff --check`. Commit only a completed -stage with a short plain-English message, and update each canonical document in -the same stage as the behavior it owns. +During each stage, keep dependencies package-private and invocation-owned, +preserve acyclic package direction, keep default tests deterministic and +offline, and retain credential redaction and filesystem safety. Prefer behavior +assertions over collaborator choreography. Run `gofmt` on changed Go files, +`go mod tidy` when module state changes, and `git diff --check`. Commit only a +completed stage with a short plain-English message, and update a canonical +internal document in the same stage only when its owned behavior changes. -## Stage 1 — Define Cleanup Evidence, Targets, And Configuration Rules +## Stage 1 — Replace Configuration And Secret-Loading Globals -**Status:** Completed +**Status:** Pending ### Goal -Create one deterministic, side-effect-free cleanup plan and reject -configurations that can never authorize it. +Establish the command-scoped dependency value and remove global substitution +from configuration and API-key loading. ### Required Work -1. Add package-private cleanup planning types in `internal/stage`. A plan - contains the validated published run ID and zero, one, or two root/target/ - policy entries in deterministic spool-then-workspace order. Do not register - the stage or delete files yet. -2. If both cleanup flags are false, return a disabled plan without requiring - publish evidence. Otherwise require and validate the succeeded `publish` - record and its metadata exactly as fixed above. Use artifact identity/key - helpers rather than string-prefix approximations; add a narrow helper only - if the existing remote-key API cannot validate the commit identity. -3. Construct the spool target with `artifacts.SessionSpoolAudioDir` and the - workspace target with `artifacts.SessionRunRootForCampaign`, always using - the publish metadata's run ID. Validate manifest/config identities before - constructing paths. Do not consume `Manifest.LocalWorkDir` or - `Manifest.LocalSpoolDir`. -4. Add cross-field configuration validation so either cleanup flag requires - effective `publish.enabled=true` and `publish.upload_run=true`. Preserve - existing defaults and field names. Add focused tests for each impossible - combination and both valid enabled policies without duplicating exact error - prose outside the config owner. -5. Table-test planning for disabled cleanup, each target selection, stable - ordering, prior-run publication, missing/failed/skipped publish state, - false or mistyped metadata, invalid run identity, wrong commit key, wrong - pointer key, and canonical paths. Prove every invalid evidence case produces - no plan that can be executed. -6. Update the cleanup fields in `docs/config.md` in this stage to describe the - new validation and their role as final-stage target selectors. Do not yet - describe the unregistered stage as implemented elsewhere. +1. Add a package-private `commandDependencies` type in `internal/app` with + typed function fields for pipeline loading, temporary object download, + secrets-directory reading, object-store construction, remote-current + discovery, restore-plan construction, and restore-plan execution. Add a + value-returning normalizer that fills nil fields with the current production + functions. Do not store a normalized instance in a package variable. +2. Add dependency-aware private variants of the configuration-loading path: + `loadCommandConfig`, `loadPipelineCampaignConfig`, and `loadPipelineConfig` + should receive or delegate to an explicit dependency value. Preserve local + session precedence, remote fallback, source provenance, bounded temporary + download behavior, and idempotent temporary-file cleanup. +3. Add a dependency-aware private secret loader used by focused tests and by + later command composition. Production callers continue through a wrapper + using normalized defaults. Preserve directory/file privacy checks, symlink + rejection, bounded reads, existing-environment precedence, redaction, and + the ancestor-replacement failure case. +4. Remove `loadPipelineConfigFn`, `downloadObjectToTempFn`, and + `readSecretsDirectoryEntries`. Rewrite `config_loader_test.go`, + loader-focused cases in `remote_session_test.go`, and `secrets_env_test.go` + to pass copied dependency values instead of saving, replacing, and restoring + globals. Leave command-routing remote-session cases for the command stage + that owns their entry point. Retain tests at the narrowest existing behavior + boundary; do not reproduce the loader's internal call sequence with mocks. +5. Keep production command behavior unchanged while this foundation is + introduced. It is acceptable for the object-store and restore fields to use + their production defaults until Stages 2 through 4 migrate their callers. ### Verification And Exit Criteria -Run `go test ./internal/config ./internal/stage -run 'Cleanup|Publish'` and -`git diff --check`. Cleanup authorization and targets are deterministic and -cannot be derived from the current invocation or uncommitted publication. +Run: -## Stage 2 — Implement Idempotent Cleanup Stage Behavior +```sh +go test ./internal/app -run 'Config|RemoteSession|Secret' +go test -race ./internal/app -run 'Config|RemoteSession|Secret' +git diff --check +``` -**Status:** Completed +The three named mutable globals are absent, remote/local configuration behavior +and secret protections remain covered, and tests supply collaborators through +local dependency values. + +## Stage 2 — Carry Dependencies Through Pipeline Commands + +**Status:** Pending ### Goal -Implement cleanup as an ordinary, independently testable stage without yet -changing the canonical pipeline. +Carry one dependency value through the pipeline-oriented command paths, +including remote session fallback, without changing their public API. ### Required Work -1. Add `cleanupStage` with name `cleanup`. Its `Run` obtains the Stage 1 plan, - self-skips when disabled, checks context before each target, and removes - each enabled target with the root-scoped filesystem helper. -2. Make `fileops.RemoveAllUnderRoot` explicitly idempotent when either the - configured root or target is already absent. Do not weaken rejection of - root deletion, traversal, or out-of-root targets and do not introduce a - general filesystem facade. -3. Return a successful `StageResult` with no outputs, logs, or generated - configuration. Keep metadata bounded to the published run and completed - policy identifiers; do not persist target paths, completion flags, or an - obligation-like structure. -4. Implement cleanup semantic fingerprinting with a new private versioned - payload containing normalized roots and policy booleans. Implement - `ResumeValidator`: disabled cleanup is reconsidered through self-skip rather - than reusable success; enabled cleanup first validates the current publish - evidence and then reports resumable only when all planned targets remain - absent. -5. Test through the stage contract with `t.TempDir`: disabled self-skip; - spool-only, workspace-only, and both; preservation of another run and the - durable cache; already-absent root/target; cancellation before deletion; - refusal of unsafe scope; first-target success followed by second-target - failure and successful idempotent retry; no canonical outputs; fingerprint - changes; and resume after success versus target recreation. Use real - filesystem state rather than adding mutable package globals or a deletion - interface solely for tests. +1. Add a private dependency-aware execution dispatcher alongside `Execute`. + `Execute` retains its signature and uses normalized production dependencies. + Extend the existing private pipeline-executor path rather than creating a + second command switch or an application framework. +2. Thread `commandDependencies` through `run`, `regenerate-artifacts`, + `run-stage`, `analyze`, `publish`, `plan`, and `status` paths and the shared + configuration loader they use. Preserve the independent `pipelineExecutor` + seam and pass both values explicitly where required. +3. Make remote session fallback initialize its object store through the + dependency value and preserve the rule that secrets load before the factory. + Public command wrappers continue to use production defaults. +4. Rewrite the assembled pipeline-command tests and only those remote-session + tests reached through pipeline commands to call the dependency-aware + dispatcher with a fake store. Remove their global object-store + replacement/reset choreography. Leave remote-session cases reached through + `session` commands for Stage 3. It is acceptable for the factory global + itself to remain temporarily for restore and operator callers until Stages 3 + and 4 migrate them. +5. Preserve command syntax, exit classification, stdout/stderr behavior, + configuration discovery, artifact selection, executor injection, and remote + session provenance. Do not move adapter construction into configuration or + storage packages. ### Verification And Exit Criteria -Run `go test ./internal/fileops ./internal/stage -run Cleanup`, -`go test -race ./internal/stage -run Cleanup`, and `git diff --check`. The -unregistered stage provides safe, idempotent behavior through existing -filesystem and manifest contracts. +Run: -## Stage 3 — Integrate Cleanup Into Canonical Orchestration +```sh +go test ./internal/app -run 'Run|Stage|Analyze|Publish|Plan|Status|RemoteSession|Command' +go test -race ./internal/app -run 'Run|Stage|Analyze|Publish|Plan|Status|RemoteSession' +git diff --check +``` -**Status:** Completed +Pipeline commands use invocation-owned dependencies, production defaults still +select the real storage constructor, and focused tests remain offline. + +## Stage 3 — Carry Dependencies Through Operator Commands + +**Status:** Pending ### Goal -Make cleanup the normal final stage and remove cleanup from runner -finalization. +Migrate session and maintenance commands to the same invocation-owned command +dependency path before removing the final shared storage seam. ### Required Work -1. Register `cleanupStage{}` after `notify` in `stage.All`; make it selectable - by the existing planner and `run-stage` command without a special command or - alias. -2. Extend the validated invalidation relation with `notify -> cleanup` and an - empty cleanup classification. Update transitive expectations so every - earlier stage reaches cleanup while preserving the render/extract sibling - relationship. -3. Reduce `finalizeInvocation` to summary construction. Delete its cleanup - call and cleanup-specific terminal-failure path. Remove `removeScopedDir` - from `invocationDependencies` and all normalization/test plumbing made dead - by that removal. -4. Delete `internal/app/post_publish_cleanup.go`. Retain application cleanup - helpers only where the explicit `clean` command still uses them; remove or - rename types and wrappers whose only owner was post-publish cleanup. -5. Replace `post_publish_cleanup_test.go` with proportionate runner-level - integration coverage. Prove full order ends in cleanup, notify failure - prevents cleanup, publish failure prevents cleanup, cleanup failure leaves - publish succeeded and remote state untouched, a later ordinary full run - skips reusable upstream work and retries cleanup, and a forced prerequisite - stales cleanup. -6. Cover selection semantics: the default full plan includes cleanup; - `--through notify` and earlier ranges do not delete; `publish` executes only - publish; direct `run-stage cleanup` works with valid evidence and fails - safely without it; artifact selection remains accepted only by analyze and - publish. Update plan, command, assembled-workflow, bounded-prerequisite, - runner, and stage-count expectations without broad snapshots. -7. Update current-behavior owners in the same stage: architecture's publish and - cleanup boundary, the internal overview/stage list and lifecycle, - `docs/cli.md`, `docs/operations.md`, `docs/internal/workspace.md`, and - `docs/internal/stage-publish.md`. Add a focused - `docs/internal/stage-cleanup.md` and link it from the overview. Clearly state - that the standalone publish command does not invoke cleanup and normal - resume retries a failed cleanup stage without republishing valid state. +1. Thread `commandDependencies` through the `session` dispatcher, session + initialization and validation, artifact listing, lock listing/add/remove, + and session-scoped or cache-aware clean helpers. Exported command functions + remain production-default wrappers. +2. Update the dependency-aware top-level dispatcher so every operator command + reached through `Execute` receives the invocation's value. Keep local-only + paths independent of remote construction; do not initialize an object store + merely because a dependency value exists. +3. Rewrite `object_store_test.go`, `operator_helpers_test.go`, affected + session/clean tests, and remote-session cases reached through the `session` + dispatcher to supply a local object-store factory. Keep complete CLI routing + tests at the private dispatcher and narrow construction-order tests at + `newCommandObjectStore`. Remove original-value capture and reset boilerplate + in the migrated tests. +4. Preserve secret-before-factory ordering, local/remote session initialization, + validation findings, artifact availability, per-source remote lock behavior, + cleanup scope, output, and errors. Continue using `storage.FakeBackend` for + remote behavior. +5. Leave only restore callers on the temporary global storage seam. Do not + remove it until Stage 4 can update restore and its tests atomically. ### Verification And Exit Criteria -Run `go test ./internal/stage ./internal/app`, -`go test -race ./internal/app -run 'Cleanup|Runner|RunStage|Bounded|Plan'`, -`go test ./internal/doccheck`, and `git diff --check`. Execution order, range -behavior, failure attribution, and retry are ordinary stage behavior, with no -post-loop deletion path. +Run: -## Stage 4 — Remove The Cleanup Obligation From Manifest State +```sh +go test ./internal/app -run 'ObjectStore|SessionInit|SessionValidate|Artifacts|Locks|Clean|Operator' +go test -race ./internal/app -run 'ObjectStore|SessionInit|SessionValidate|Artifacts|Locks|Clean' +git diff --check +``` -**Status:** Completed +All non-restore commands use invocation-owned dependencies, their tests mutate +no factory global, and remote behavior remains deterministic and offline. + +## Stage 4 — Replace Restore Phase Globals With Explicit Dependencies + +**Status:** Pending ### Goal -Delete the second cleanup state machine and leave stage status as the only -durable cleanup progress model. +Make restore sequencing testable per invocation without global phase hooks or a +new restore framework. ### Required Work -1. Remove `CleanupTarget`, `PostPublishCleanup`, and - `Manifest.PostPublishCleanup`; remove their validation, cloning, test - helpers, JSON fixtures, and persistence assertions. -2. Remove restore's special clearing of `post_publish_cleanup`. Restore should - treat cleanup exactly like other stage state and must not synthesize or - reset a separate obligation. -3. Keep `SessionManifestSchemaVersion` at v1. Confirm strict decoding rejects a - literal `post_publish_cleanup` field as unknown, while canonical v1 - manifests without it still round-trip unchanged. Add no compatibility - reader, migration code, or silent field discard. -4. Update `docs/internal/manifest.md`, restore/workspace internals, operations, - and troubleshooting where they own the removed state or recovery advice. - Migration guidance should tell operators with an outstanding old obligation - to finish cleanup with the pre-upgrade binary or verify remote publication, - remove/regenerate the incompatible local manifest, and then use the cleanup - stage. Do not present manual JSON editing as the normal migration path. -5. Search production, tests, fixtures, examples, and non-audit documentation - for `PostPublishCleanup`, `CleanupTarget`, `post_publish_cleanup`, - `runPostPublishCleanup`, and `post-publish cleanup obligation`. Remove every - live reference; historical roadmap/audit records may retain the terms. +1. Keep `Restore` as the public production wrapper and add one private + dependency-aware restore entry point. Normalize dependencies once at that + boundary and use its object-store constructor, remote-current discovery, + plan builder, and plan executor throughout the invocation. +2. Remove `newObjectStoreFromConfigFn`, `discoverRemoteCurrentStateFn`, + `buildRestorePlanFn`, and `executeRestorePlanFn`. Call the concrete + production functions through the normalized dependency value; do not add + interfaces around pure phase functions or move restore orchestration out of + `internal/app`. +3. Rewrite `restore_test.go`, `restore_execution_test.go`, and + `restore_recovery_test.go` to pass local dependencies. Consolidate repeated + dependency construction into one small test helper that returns a fresh + value and never mutates package state. +4. Preserve the observable restore boundaries: dry-run performs no local + installation; non-dry-run planning occurs while holding the session lock; + conflicts do not execute; the incomplete marker brackets installation; + reports reflect failure or success; manifest installation remains last; and + lock release errors remain joined with an earlier failure. +5. Prefer real `buildRestorePlan` and `executeRestorePlan` with + `storage.FakeBackend` where the test is about restore results or filesystem + state. Stub a phase only when the phase call or its timing is the behavior + being tested, such as proving the transition lock is already held. +6. Update `docs/internal/command-restore.md` only if its description of phase + composition or test seams is no longer accurate. Do not change the operator + restore contract. ### Verification And Exit Criteria -Run `go test ./internal/manifest ./internal/app ./internal/stage`, -`go test ./internal/doccheck`, and `git diff --check`. The session manifest -contains only the ordinary cleanup stage record, and old obligation-bearing -documents fail strict decoding rather than entering a compatibility path. +Run: -## Stage 5 — Consolidate, Validate, And Close Workstream 9 +```sh +go test ./internal/app -run 'Restore' +go test -race -shuffle=on -count=3 ./internal/app -run 'Restore' +git diff --check +``` -**Status:** Completed +No command has a mutable production function seam, restore transaction ordering +and recovery outcomes remain covered, and restore tests can execute +independently without save/restore choreography. + +## Stage 5 — Improve Selected Large Structural Test Diagnostics + +**Status:** Pending ### Goal -Ensure the reduced implementation, tests, and canonical documentation agree, -then record the workstream complete. +Make failures in the four audit-selected large-value test categories readable +without imposing a repository-wide assertion style. ### Required Work -1. Review the complete diff for duplicated evidence/target logic, obsolete - runner branches, redundant wrappers, stale fixtures, accidental remote - cleanup reads, and any new per-target persistence. Consolidate only into the - narrow owners established above. -2. Confirm the effective behavioral matrix: disabled self-skip; valid publish - evidence before deletion; final-stage ordering; bounded exclusion; direct - retry; idempotent absence; partial-deletion recovery; publish success - surviving cleanup failure; semantic invalidation; and destructive - confinement. -3. Check all current-behavior documentation, CLI help, examples, configuration - diffs, stage lists, and migration guidance. Volatile details must have one - canonical owner and roadmap documents must not be cited as current runtime - documentation. -4. Run every repository-wide validation command from `docs/development.md`, - plus `git diff --check`. Run the scheduled shuffle/repetition command on the - affected `internal/app`, `internal/stage`, `internal/manifest`, and - `internal/fileops` packages. Record platform-only limitations without - weakening or silently skipping applicable checks. -5. Only after all checks pass, mark every stage in this plan and Workstream 9 - in `refactoring.md` **Complete**. Leave Workstream 10 as the next pending - scope and do not prepare or publish a release. +1. Add `github.com/google/go-cmp/cmp` as a direct test dependency and update + `go.mod` and `go.sum` with the Go toolchain. Do not add an assertion framework + or helper wrapper around `cmp.Diff`. +2. Replace existing whole-value `reflect.DeepEqual` assertions with + `cmp.Diff` where a field-level diff is materially useful in these files: + - composed/effective configuration comparisons in + `internal/config/pipeline_composition_test.go` and + `internal/config/pipeline_profiles_test.go`; + - complete analyze plans, artifact-state maps, or stage records in + `internal/stage/analyze_plan_test.go`, + `internal/stage/analyze_reconciliation_test.go`, and + `internal/app/analyze_checkpoint_recovery_test.go`; + - restore action collections or complete plans in + `internal/app/restore_plan_test.go`; and + - complete output outcomes, declarations, or mappings in + `internal/stage/publish_output_plan_test.go`. +3. Keep direct field assertions that intentionally isolate one invariant. Use + `slices.Equal` for simple ordered slices in the touched files where it is + clearer; retain `reflect.DeepEqual` when its value is small and the mismatch + is already obvious. Do not expand this stage to every test package. +4. Do not use `cmpopts.EquateEmpty`, `cmp.AllowUnexported`, blanket ignore + options, or custom comparers merely to force a passing comparison. Compare + the same observable values as before and preserve distinctions in nil, + empty, order, timestamps, and provenance. +5. Run `go mod tidy` and confirm the dependency is used only by tests. Apply + `slices` and `maps` to production touched in Stages 1–4 only where the fixed + decisions above permit an identical shallow operation; do not manufacture a + production change to demonstrate their use. + +### Verification And Exit Criteria + +Run: + +```sh +go mod tidy +go test ./internal/config ./internal/stage ./internal/app +go test -race ./internal/config ./internal/stage ./internal/app +git diff --check +``` + +The selected large comparisons emit useful structural diffs, simple assertions +remain simple, module state is tidy, and no broad assertion-style migration has +occurred. + +## Stage 6 — Verify Coverage, Align Internal Documentation, And Close The Roadmap + +**Status:** Pending + +### Goal + +Prove that the reduced test and documentation system protects the final +refactored architecture, then close Workstream 10 and the consolidated roadmap. + +### Required Work + +1. Search production and tests for all seven retired function-seam names and + for assignments that replace application functions. Remove any remaining + dead wrappers, original-value captures, reset helpers, or cleanup callbacks + whose sole purpose was global substitution. Do not remove compiled regexes, + canonical data tables, or legitimate local function values. +2. Review the existing focused owners—not the entire suite indiscriminately—to + confirm retained coverage for: + - atomic visibility and bounded reads in `internal/fileops` and storage + adapter tests; + - POSIX group modes in `internal/fileops/modes_posix_test.go`; + - validated storage downloads and Notarius promotion in storage adapter and + extract tests; + - destructive confinement and restore symlink conflicts in fileops, + cleanup, and restore tests; + - credential privacy and redaction in secrets, logging, subprocess, and + adapter tests; + - session-only progress and per-artifact recovery in manifest, stage, and + application checkpoint tests; + - planning/execution parity in `internal/app/run_control_test.go`; + - remote-lock carry-forward in publish planning/execution tests; and + - idempotent final cleanup in stage and application cleanup tests. + Add a test only if a listed consequential boundary is genuinely absent; + consolidate or delete only demonstrably redundant mechanism coverage. +3. Review `docs/policy/architecture.md`, `docs/internal/overview.md`, focused + internal component documents, and their test inventories for current + accuracy. Remove stale implementation claims and duplicate volatile detail; + retain links to the canonical CLI, configuration, operations, integration, + and release owners rather than copying those contracts. +4. Confirm no current-behavior document treats a retired compatibility path, + run manifest, cleanup obligation, central lock manifest, custom directory + promotion, or mutable test hook as supported behavior. Historical roadmap + and audit records remain unchanged. +5. Run the complete validation matrix below. Use a fresh temporary directory + for the release-asset build and remove it afterward; do not create a release + note, tag, or publication. Record native-platform limitations exactly as in + `docs/development.md` rather than treating cross-compilation as runtime + evidence. +6. Only after every check passes, mark Stages 1–6 in this plan and Workstream 10 + in `refactoring.md` **Complete**. Confirm every numbered workstream is then + complete. Do not delete the audits, roadmap, or implementation plan and do + not prepare or publish a release. ### Verification And Exit Criteria At minimum run: ```sh -go test ./... -go test -race ./... -go test -race -shuffle=on -count=3 ./internal/app ./internal/stage ./internal/manifest ./internal/fileops +go mod tidy -diff +test -z "$(gofmt -l $(git ls-files '*.go'))" +go test -count=1 ./... +go test -race -count=1 ./... +go test -race -shuffle=on -count=3 ./... go vet ./... go build ./... -go test ./internal/doccheck -go test ./internal/config -run '^TestExamplesLoadAndValidate$' +go test -count=1 ./internal/doccheck +go test -count=1 ./internal/config -run '^TestExamplesLoadAndValidate$' git diff --check ``` -The worktree contains one final canonical cleanup stage, no special cleanup -obligation or runner side effect, aligned documentation, and no Workstream 10 -scope beyond changes required by this behavior. +Also run `scripts/build-release-assets.sh v0.0.0-test ` +and delete only that verified temporary directory afterward. The workstream is +complete when application collaborators are invocation-scoped, selected large +test failures are diagnostic, retained high-risk behavior is credibly covered, +canonical documentation describes the implemented reduced contracts, all ten +workstreams are complete, and the worktree contains no release artifact. ## Open Questions -None. The approved audits, refactoring roadmap, and fixed decisions above make -this implementation plan decision-complete. +None. The audits, completed prerequisite workstreams, and fixed decisions above +make the remaining quality and documentation work decision-complete.