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