From ea87c335d6246fc86d9b0fa23aa7d8363bbcb971 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 23 May 2026 08:10:00 -0500 Subject: [PATCH] Add a roadmap to implement the high-priority items revealed by the code quality audit --- docs/roadmap/cleanup.md | 294 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 294 insertions(+) create mode 100644 docs/roadmap/cleanup.md diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md new file mode 100644 index 0000000..ab1e0c7 --- /dev/null +++ b/docs/roadmap/cleanup.md @@ -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.`; + - parse previous-session artifact keys from `narratio.previous_session.artifact.`; + - 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.