Add a roadmap to implement the high-priority items revealed by the code quality audit
This commit is contained in:
294
docs/roadmap/cleanup.md
Normal file
294
docs/roadmap/cleanup.md
Normal file
@@ -0,0 +1,294 @@
|
||||
# Roadmap: Pre-1.0 Code Cleanup
|
||||
|
||||
Status: Planned
|
||||
|
||||
This roadmap turns the findings in `docs/roadmap/audit.md` into staged cleanup work for the 1.0 release. It is planning-only. Do not implement these refactors until a stage is explicitly selected for implementation.
|
||||
|
||||
The cleanup work must follow the policy documents under `docs/policy/`, especially these invariants:
|
||||
|
||||
- keep Narratio explicit and stage-driven;
|
||||
- do not introduce a generic workflow engine, DAG abstraction, or generic CLI framework;
|
||||
- keep external-system details behind adapters;
|
||||
- do not move campaign/session/root-prefix semantics into storage adapters;
|
||||
- keep AWS SDK types out of app and stage logic;
|
||||
- keep path and remote key construction centralized;
|
||||
- preserve manifest-driven run state;
|
||||
- keep public CLI/config behavior stable unless a stage explicitly says it is an internal naming cleanup.
|
||||
|
||||
## Non-Goals
|
||||
|
||||
- Do not change public command syntax, config schema, S3 key layout, manifest schema, or artifact source IDs as part of this cleanup.
|
||||
- Do not add compatibility aliases or migration logic.
|
||||
- Do not rewrite stage execution, manifest state transitions, or adapter contracts.
|
||||
- Do not generalize text output into a generic reporting framework.
|
||||
- Do not fold S3 audio cache behavior into a generic downloader.
|
||||
- Do not move secret loading into storage adapters.
|
||||
|
||||
## Stage 1: Shared Low-Risk Mechanics
|
||||
|
||||
Goal: remove duplicated mechanics that are easy to test and should not affect public behavior.
|
||||
|
||||
Implementation decisions:
|
||||
|
||||
- Add one shared helper for safe relative artifact destination normalization.
|
||||
- It must reject empty paths, absolute paths, `.`, `..`, and traversal outside the artifact/session scope.
|
||||
- It must normalize separators to slash-form for artifact and S3 destination logic.
|
||||
- It must be dependency-light enough to be called from config validation, app helpers, publish execution, and previous-cache planning.
|
||||
- Add one shared object-store temp download helper.
|
||||
- It must take `context.Context`, `storage.ObjectStore`, a key, and a temp-file pattern.
|
||||
- It must create and close the temp file before download, remove the temp file on failed download, and return a cleaned local path on success.
|
||||
- It must not infer bucket, campaign, session, run, or root-prefix semantics.
|
||||
- Extract shared cleanup target validation for local deletion.
|
||||
- Cover scoped directory deletion, scoped file deletion, and removable children under a root.
|
||||
- Preserve existing safety rules: reject empty roots/targets, root deletion, outside-root paths, symlinks, and wrong target types.
|
||||
- Keep command-specific output in `clean` and manifest metadata handling in post-publish cleanup.
|
||||
|
||||
Expected callers:
|
||||
|
||||
- replace duplicate relative destination normalization in publish execution, helper command rendering, and previous-cache planning;
|
||||
- replace duplicate temp download helpers in app and previous-cache code;
|
||||
- replace duplicate scoped deletion validation in clean and post-publish cleanup.
|
||||
|
||||
Tests:
|
||||
|
||||
- Add focused tests for destination normalization and path traversal rejection.
|
||||
- Add temp download tests for success, failed download cleanup, and preserved contextual caller errors.
|
||||
- Add shared cleanup validation tests for directories, files, symlinks, missing targets, root deletion, and outside-root targets.
|
||||
- Run:
|
||||
- `go test ./internal/artifacts -v`
|
||||
- `go test ./internal/adapters/storage -v`
|
||||
- `go test ./internal/app -run 'Clean|Post' -v`
|
||||
- `go test ./...`
|
||||
|
||||
Completion criteria:
|
||||
|
||||
- duplicated low-level mechanics are removed;
|
||||
- public behavior and output are unchanged;
|
||||
- no stage, command, or config semantics move into storage adapters.
|
||||
|
||||
## Stage 2: Artifact Source and Published Output Policy
|
||||
|
||||
Goal: make artifact source IDs and published-output destination derivation a single shared policy.
|
||||
|
||||
Implementation decisions:
|
||||
|
||||
- Introduce `internal/artifactpolicy` as the shared source policy package.
|
||||
- This package is the long-term home because it avoids config/artifacts import cycles.
|
||||
- It may depend on dependency-light model packages, but it must not depend on app, stage, manifest stores, storage adapters, or downstream adapters.
|
||||
- Centralize these behaviors in `internal/artifactpolicy`:
|
||||
- classify source IDs as built-in, configured artifact, or previous-session configured artifact;
|
||||
- parse configured artifact keys from `narratio.artifact.<key>`;
|
||||
- parse previous-session artifact keys from `narratio.previous_session.artifact.<key>`;
|
||||
- validate configured artifact sources against `pipeline.scriptorium.artifacts`;
|
||||
- validate publish lock/output sources;
|
||||
- derive default published destinations for built-in and configured artifact sources;
|
||||
- normalize safe relative published-output destinations.
|
||||
- Update callers to consume the shared policy:
|
||||
- config validation for `publish.outputs` and `publish.locks`;
|
||||
- publish-stage output resolution;
|
||||
- status and `artifacts list` rendering;
|
||||
- locks list/add/remove validation;
|
||||
- analyze input source handling;
|
||||
- previous-cache candidate planning.
|
||||
- Preserve caller-specific policy at call sites.
|
||||
- Required vs optional behavior remains in publish, analyze, restore, and previous-cache callers.
|
||||
- Locked output behavior remains in publish.
|
||||
- Text formatting remains in app commands.
|
||||
- Manifest path scanning remains in artifact/previous-cache logic unless directly tied to source policy.
|
||||
|
||||
Tests:
|
||||
|
||||
- Add `internal/artifactpolicy` table tests for source classification, configured artifact validation, previous-session parsing, default destination derivation, and destination normalization.
|
||||
- Update `internal/config` tests so publish outputs and locks validate through the shared policy.
|
||||
- Update `internal/stage` publish tests for selected, unselected, optional, required, and locked output behavior.
|
||||
- Update `internal/app` tests for status, artifacts list, and locks.
|
||||
- Update `internal/previouscache` tests for previous-session candidate ordering.
|
||||
- Run:
|
||||
- `go test ./internal/artifacts -v`
|
||||
- `go test ./internal/config -v`
|
||||
- `go test ./internal/stage -run 'Analyze|Publish' -v`
|
||||
- `go test ./internal/app -run 'Artifacts|Status|Locks' -v`
|
||||
- `go test ./internal/previouscache -v`
|
||||
- `go test ./...`
|
||||
|
||||
Completion criteria:
|
||||
|
||||
- artifact source vocabulary and destination derivation are no longer reimplemented in config, app, stage, and previous-cache packages;
|
||||
- every caller still owns its own missing/required/optional/locked decision;
|
||||
- public behavior is unchanged.
|
||||
|
||||
## Stage 3: Publish Terminology Cleanup
|
||||
|
||||
Goal: align internal implementation names with the public publish contract.
|
||||
|
||||
Implementation decisions:
|
||||
|
||||
- Rename archive-named internal files, types, helpers, comments, and tests that now implement publish behavior.
|
||||
- Replace names such as:
|
||||
- `archiveStage` with `publishStage`;
|
||||
- `ResolveArchiveSessionPrefix` with publish/current-state terminology;
|
||||
- `ResolveArchiveRunPrefix` with publish/run-history terminology;
|
||||
- `ResolveArchiveCurrentStateKeys` with current-state terminology;
|
||||
- `runPostArchiveCleanup` with post-publish cleanup terminology;
|
||||
- `staticArchiveLocks` with publish lock terminology.
|
||||
- Keep the S3 layout stable:
|
||||
- `{session_prefix}/runs/{run_id}/`;
|
||||
- `{session_prefix}/current/manifest.json`;
|
||||
- `{session_prefix}/current/run_id.txt`;
|
||||
- `{session_prefix}/locks.yml`.
|
||||
- Keep the public stage name `publish`.
|
||||
- Keep old archive/promote references only where they are historical roadmap context or intentionally describe immutable run history.
|
||||
|
||||
Tests and checks:
|
||||
|
||||
- Run:
|
||||
- `go test ./internal/stage -v`
|
||||
- `go test ./internal/app -v`
|
||||
- `go test ./internal/artifacts -v`
|
||||
- `go test ./...`
|
||||
- Run stale-term sweeps:
|
||||
- `rg -n "archive|promote|promoted|promotion" internal docs examples cmd`
|
||||
- `rg -n "ResolveArchive|archiveStage|post_archive|staticArchive|normalizeArchive" internal`
|
||||
|
||||
Completion criteria:
|
||||
|
||||
- public publish behavior is no longer implemented through archive/promote names;
|
||||
- remaining old terms are intentionally historical, test-fixture bucket names, or roadmap-only context;
|
||||
- no config, CLI, manifest, or S3 layout changes are introduced.
|
||||
|
||||
## Stage 4: Session Command Parsing Consolidation
|
||||
|
||||
Goal: reduce command-loading drift while keeping command handlers explicit.
|
||||
|
||||
Implementation decisions:
|
||||
|
||||
- Add a small app-level parser helper for common session-aware commands.
|
||||
- Centralize:
|
||||
- common config flags: `--config`, `--campaign`, `--campaign-file`, `--session`;
|
||||
- positional session ID handling;
|
||||
- `--session-id` compatibility;
|
||||
- `--previous-session-id`;
|
||||
- optional selected-artifact parsing for commands that support it.
|
||||
- Keep command handlers explicit and readable.
|
||||
- Do not introduce a generic CLI framework.
|
||||
- Treat these as intentional special cases:
|
||||
- `session init` loads pipeline and campaign but not session;
|
||||
- `clean --all` loads pipeline only;
|
||||
- `status --manifest` remains local-manifest mode;
|
||||
- `run-stage` keeps its stage-name positional handling but reuses common flag parsing where practical.
|
||||
- Standardize flag help text where commands use the same semantics.
|
||||
|
||||
Tests:
|
||||
|
||||
- Update app command tests for:
|
||||
- positional session ID;
|
||||
- `--session-id`;
|
||||
- positional/flag mismatch;
|
||||
- missing session ID;
|
||||
- `--previous-session-id`;
|
||||
- unsupported `--artifacts` by command/stage;
|
||||
- unchanged behavior for `session init`, `clean --all`, and `status --manifest`.
|
||||
- Run:
|
||||
- `go test ./internal/app -run 'Run|RunStage|Analyze|Publish|Restore|Clean|Session' -v`
|
||||
- `go test ./internal/app -v`
|
||||
- `go test ./...`
|
||||
|
||||
Completion criteria:
|
||||
|
||||
- shared session flag/session ID behavior has one implementation;
|
||||
- command handlers remain command-specific;
|
||||
- public command syntax and output stay unchanged.
|
||||
|
||||
## Stage 5: Remote Current-State Mechanics
|
||||
|
||||
Goal: centralize remote current-state loading mechanics without hiding caller policy.
|
||||
|
||||
Implementation decisions:
|
||||
|
||||
- Extract narrow helpers for remote current state.
|
||||
- Load current run pointer through `storage.ObjectStore`.
|
||||
- Load and decode current manifest through `storage.ObjectStore`.
|
||||
- Validate campaign, session, and run identity when requested by the caller.
|
||||
- Return typed missing-state errors.
|
||||
- Preserve caller policy:
|
||||
- restore treats missing or invalid current state as an error;
|
||||
- previous-cache hydration fails for required previous artifacts and skips optional missing artifacts;
|
||||
- status reports missing remote state as state, not command failure;
|
||||
- session validate emits findings and fails only for error findings.
|
||||
- Keep all remote key construction in `internal/artifacts`.
|
||||
- Keep object-store initialization in `internal/app`.
|
||||
- Do not add storage adapter knowledge of campaigns, sessions, runs, root prefixes, current state, or manifests.
|
||||
|
||||
Tests:
|
||||
|
||||
- Add helper tests for:
|
||||
- missing current run pointer;
|
||||
- missing current manifest;
|
||||
- empty run pointer;
|
||||
- malformed manifest;
|
||||
- campaign mismatch;
|
||||
- session mismatch;
|
||||
- run ID mismatch.
|
||||
- Update restore, previous-cache, status, and session validate tests to prove their caller-specific behavior is unchanged.
|
||||
- Run:
|
||||
- `go test ./internal/app -run 'Restore|Status|SessionValidate' -v`
|
||||
- `go test ./internal/previouscache -v`
|
||||
- `go test ./...`
|
||||
|
||||
Completion criteria:
|
||||
|
||||
- low-level remote current-state mechanics are shared;
|
||||
- missing-state behavior remains caller-specific;
|
||||
- storage adapter boundaries remain unchanged.
|
||||
|
||||
## Stage 6: Operator Helper File Split and Final Sweep
|
||||
|
||||
Goal: improve maintainability after shared policy and mechanics are already centralized.
|
||||
|
||||
Implementation decisions:
|
||||
|
||||
- Split the large operator helper implementation by command or responsibility.
|
||||
- Suggested file grouping:
|
||||
- session init;
|
||||
- session validate;
|
||||
- status;
|
||||
- artifacts list;
|
||||
- locks;
|
||||
- helper findings;
|
||||
- helper artifact rendering.
|
||||
- Do not change command syntax, text output, config loading, remote loading, lock behavior, or artifact catalog behavior during the split.
|
||||
- Keep output formatting text-only and command-specific unless a concrete inconsistency remains after the split.
|
||||
- Update roadmap status notes after each completed stage.
|
||||
|
||||
Tests and checks:
|
||||
|
||||
- Run:
|
||||
- `go test ./internal/app -v`
|
||||
- `go test ./...`
|
||||
- Final searches:
|
||||
- `rg -n "archive|promote|promoted|promotion" internal docs examples cmd`
|
||||
- `rg -n "narratio.transcript.merged|narratio.transcript.full|narratio.transcript.trimmed" internal docs examples`
|
||||
- `rg -n "previous_session_artifact|promote_artifacts|pipeline.archive" internal docs examples`
|
||||
|
||||
Completion criteria:
|
||||
|
||||
- operator helper code is easier to navigate;
|
||||
- stale implementation terminology is removed or intentionally documented;
|
||||
- no behavior changes are introduced by file organization.
|
||||
|
||||
## Overall Validation
|
||||
|
||||
After each implementation stage:
|
||||
|
||||
- run the focused tests listed for that stage;
|
||||
- run `go test ./...`;
|
||||
- run `git status --short`;
|
||||
- update this roadmap to mark the completed stage implemented only after code, tests, and documentation are aligned.
|
||||
|
||||
## Assumptions
|
||||
|
||||
- This roadmap is a cleanup plan, not a feature plan.
|
||||
- Stages may be implemented as separate prompts/commits.
|
||||
- `internal/artifactpolicy` is the chosen home for shared source policy.
|
||||
- Shared object-store temp download helpers must not learn Narratio session semantics.
|
||||
- Public behavior must remain stable unless a stage explicitly says it is internal terminology cleanup.
|
||||
Reference in New Issue
Block a user