diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md new file mode 100644 index 0000000..aa54664 --- /dev/null +++ b/docs/roadmap/cleanup.md @@ -0,0 +1,206 @@ +# Roadmap: Pre-1.0 Code Cleanup + +Status: Planned + +This roadmap turns the remaining findings in `docs/roadmap/audit.md` into decision-complete implementation stages. It follows the policy documents under `docs/policy/` and preserves current public CLI, config, storage layout, manifest, and stage behavior unless a stage explicitly says otherwise. + +## Goals + +- Reduce repeated path, file, source-policy, and read-only inspection logic before 1.0. +- Keep Narratio explicit, stage-driven, and easy to review. +- Keep storage adapters free of campaign/session/run/root-prefix semantics. +- Keep command output text-only and command-specific. +- Keep implementation changes small enough for focused prompts and focused tests. + +## Non-Goals + +- Do not introduce a generic workflow engine or DAG abstraction. +- Do not introduce a generic CLI framework. +- Do not introduce a broad manifest abstraction. +- Do not move secret loading into storage adapters. +- Do not make storage adapters infer Narratio path or key semantics. +- Do not add compatibility aliases for retired archive/promote, campaign, transcript, or previous-session source names. +- Do not move restore's remote-key include/exclude policy out of restore unless another caller is added. + +## Stage 1: Path and File Mechanics + +Add shared mechanics for root-scoped path safety and atomic file operations. + +Implementation decisions: + +- Add root-scoped helpers in `internal/pathsafe`: + - join a slash-style relative path safely under a root; + - derive a safe slash-style relative path from a path under a root; + - reject empty paths, absolute paths, traversal, and paths outside the root. +- Add a small dependency-light `internal/fileops` package for shared file installation mechanics: + - atomic byte write; + - atomic file copy; + - atomic file copy with SHA-256 checksum; + - install an already-downloaded temp file with permissions. +- Prefer `internal/fileops` over extending `artifacts.LocalStore`, because the same mechanics are used by app, stage, audio, artifacts, and manifest code. +- Keep higher-level semantics local: + - restore decides which remote keys are in scope; + - audio decides cache hit/miss behavior; + - manifest decides JSON marshaling and validation; + - stages decide canonical output materialization. + +Implementation targets: + +- Replace duplicate root-escape checks in restore planning, run-local output handling, previous-cache path conversion, and artifact local path helpers where doing so keeps behavior identical. +- Replace duplicate temp-write/copy/rename mechanics in artifact local store, audio cache materialization, restore execution, and prepare helpers where the call site can keep its current error context. +- Leave manifest save behavior unchanged if sharing it would obscure manifest-specific validation or error text. + +Tests: + +- `go test ./internal/pathsafe -v` +- `go test ./internal/audio -v` +- `go test ./internal/app -run Restore -v` +- `go test ./internal/stage -run Prepare -v` +- `go test ./...` + +Acceptance criteria: + +- Root escape, absolute path, empty path, Windows separator, and valid relative path cases are covered by path-safe tests. +- Atomic helper tests prove temp files are cleaned up on failure and checksums match final file contents. +- Restore, prepare, and audio cache behavior remain unchanged. + +## Stage 2: Artifact Source Policy + +Finish centralizing artifact source vocabulary and validation in `internal/artifactpolicy`. + +Implementation decisions: + +- Extend `internal/artifactpolicy` with Scriptorium-input policy helpers: + - classify and validate built-in, configured, and previous-session source IDs; + - validate referenced configured artifact keys against the configured artifact set; + - expose a previous-session source descriptor for callers that need the configured artifact key. +- Keep runtime artifact lookup in `internal/artifacts`. +- Keep missing, required, optional, and operator-guidance behavior at call sites: + - config validation still produces field-specific errors; + - analyze still decides whether missing inputs fail or skip; + - previous-cache planning still decides required vs optional behavior. +- Do not make `artifactpolicy` inspect manifests, files, object storage, or runtime catalogs. + +Implementation targets: + +- Replace source parsing and static built-in checks in config validation with artifactpolicy helpers. +- Update analyze input resolution to consume the shared classification/descriptors while preserving current error messages and required/optional behavior. +- Update previous-cache planning to use shared previous-session source descriptors where source vocabulary is involved. +- Keep publish output destination derivation through `artifactpolicy.ResolvePublishedDestination`. + +Tests: + +- `go test ./internal/artifactpolicy -v` +- `go test ./internal/config -v` +- `go test ./internal/stage -run Analyze -v` +- `go test ./internal/previouscache -v` +- `go test ./...` + +Acceptance criteria: + +- Valid Scriptorium input source cases pass through one shared policy path. +- Invalid source format and unknown configured artifact references keep clear config-field errors. +- Analyze behavior for required/optional built-in, configured, and previous-session sources is unchanged. +- Previous-cache candidate ordering and required/optional behavior are unchanged. + +## Stage 3: Read-Only Inspection Layer + +Extract shared read-only session inspection checks for `session validate` and `session status`. + +Implementation decisions: + +- Keep the new inspection helpers in `internal/app`; they are command orchestration helpers, not stage or storage adapter behavior. +- Create small result types for checks, but do not create a generic reporting framework. +- Keep command-specific rendering local: + - `session validate` renders findings and fails on `ERROR`; + - `session status` renders state and does not fail for missing local/remote state unless config loading fails. +- Do not download artifact bodies for inspection unless current behavior already does so. + +Implementation targets: + +- Extract stable input checks from `operator_findings.go` into a reusable inspection helper. +- Extract local and remote audio presence checks that mirror prepare's selection rules without materializing audio. +- Extract previous-session readiness checks using existing current-state and previous-cache planning mechanics where possible. +- Extract effective lock and remote current-state inspection into reusable command helpers. +- Keep artifact catalog rendering separate from these checks. + +Tests: + +- `go test ./internal/app -run 'SessionValidate|Status' -v` +- `go test ./internal/previouscache -v` +- `go test ./internal/stage -run Prepare -v` +- `go test ./...` + +Acceptance criteria: + +- `session validate` and `session status` agree on local/remote audio and previous-session readiness facts. +- Missing current run pointer/manifest behavior remains command-appropriate: validation reports an error; status reports unavailable state. +- Existing lock and remote current-state behavior is unchanged. + +## Stage 4: Optional CLI Edge Cleanup + +Only implement this stage if Stage 1-3 leave meaningful repeated parser code. + +Implementation decisions: + +- Add at most one small parser helper for commands with `session_id` plus one additional positional argument. +- Use it for `session locks add` and `session locks remove` if it reduces duplication without obscuring syntax. +- Keep command handlers explicit. +- Do not change public syntax, flag names, help text meaning, or error semantics. + +Tests: + +- `go test ./internal/app -run 'Session|Locks|RunStage' -v` +- `go test ./...` + +Acceptance criteria: + +- Positional `session_id` and `--session-id` mismatch errors remain unchanged. +- Missing source arguments for lock add/remove remain clear. +- No new command aliases are introduced. + +## Stage 5: Final Sweep + +Run final validation after the implementation stages. + +Required 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` +- `rg -n "CreateTemp|Rename|copyFileAtomic|WriteFileAtomic|DownloadObjectToTemp" internal` + +Expected search results: + +- Retired terminology should remain only where intentionally historical or where fixture names make it unrelated to current behavior. +- Old transcript and old config/source names should not appear in runtime code, tests, examples, or current-behavior docs. +- File-operation searches should show centralized helpers plus acceptable direct uses where package-specific behavior remains intentional. + +Required tests: + +- `go test ./internal/artifactpolicy -v` +- `go test ./internal/artifacts -v` +- `go test ./internal/config -v` +- `go test ./internal/stage -run 'Analyze|Prepare|Publish' -v` +- `go test ./internal/app -run 'SessionValidate|Status|Restore|Locks|RunStage' -v` +- `go test ./internal/previouscache -v` +- `go test ./internal/audio -v` +- `go test ./internal/pathsafe -v` +- `go test ./internal/adapters/storage -v` +- `go test ./internal/manifest -v` +- `go test ./...` + +Roadmap cleanup: + +- Mark each completed stage as implemented only after its code, tests, and any internal docs are updated. +- Keep this file as planned work until the implementation stages land. +- Do not update canonical user/operator docs unless implementation changes visible behavior, which this roadmap does not intend. + +## Assumptions + +- Public CLI and config behavior remains unchanged throughout this cleanup. +- `internal/fileops` is the preferred home for shared atomic file mechanics. +- `internal/artifactpolicy` remains the source-policy home. +- `internal/artifacts` remains the path/key/current-state home. +- `internal/app` remains the command-loading, inspection, and rendering home. +- Stage packages keep stage execution policy and adapter interaction.