diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md new file mode 100644 index 0000000..a6f2fa3 --- /dev/null +++ b/docs/roadmap/audit.md @@ -0,0 +1,465 @@ +# Roadmap: Code Quality and Deduplication Audit + +Status: Draft audit report + +This is a pre-1.0 implementation audit. It identifies remaining high-confidence opportunities to simplify, centralize, or clarify Narratio before release. It is report-only: no code refactors are included here. + +The current repository keeps architecture and development policy under `docs/policy/`, not at `docs/architecture.md` or `docs/development.md`. This audit used `docs/policy/architecture.md`, `docs/policy/development.md`, `docs/policy/documentation.md`, the current user/operator docs, the internal docs, code, examples, and tests. + +## 1. Executive Summary + +Overall code quality is strong. Narratio now has clear package boundaries in the important places: + +- configuration loading is strict and centralized enough for current commands; +- object-store construction is app-owned and secret-aware; +- storage adapters do not infer campaign/session/run semantics; +- S3 key construction and local workspace paths are mostly centralized in `internal/artifacts`; +- artifact source vocabulary has a dedicated `internal/artifactpolicy` package; +- remote current-state loading is centralized in `internal/artifacts`; +- cleanup path safety and temp object downloads have shared helpers; +- publish terminology has replaced the old archive/promote surface in live code. + +There is no major architectural risk and no reason to delay 1.0 for a broad rewrite. The remaining opportunities are targeted cleanup items where a future bug fix could otherwise need changes in several files. + +Top three refactoring targets before 1.0: + +1. Finish consolidating artifact source policy for Scriptorium inputs and previous-session candidates. +2. Centralize session-relative path conversion and atomic file install/copy mechanics. +3. Extract a small read-only inspection/preflight layer used by `session validate` and `session status`. + +The codebase is ready for a limited cleanup pass. Avoid speculative abstractions. + +## 2. High-Confidence Deduplication Opportunities + +### Complete Artifact Source Policy for Scriptorium Inputs + +Affected files/packages: + +- `internal/artifactpolicy` +- `internal/config/validate.go` +- `internal/stage/analyze.go` +- `internal/artifacts/artifact_resolver.go` +- `internal/previouscache/previouscache.go` +- tests under `internal/config`, `internal/stage`, `internal/artifacts`, and `internal/previouscache` + +Duplicated or near-duplicated behavior: + +- `artifactpolicy.ClassifySource` knows built-in, configured, and previous-session source families. +- `config.validateScriptoriumInputSource` still implements Scriptorium-input-specific validation, including previous-session source parsing, configured artifact lookups, and static built-in checks. +- `stage.resolveScriptoriumInput` classifies sources, then maps missing configured/previous/built-in sources to stage-specific required/optional behavior. +- `previouscache.artifactRelativePathCandidates` reconstructs previous-session artifact candidates from manifest outputs, published paths, and configured output paths. +- `artifacts.ResolveSessionArtifactWithCatalog` and `ResolvePreviousSessionArtifactWithCatalog` own runtime resolution, but caller-side policy still reaches into source details. + +Why it matters: + +Artifact source IDs are public configuration. They are used by Scriptorium inputs, previous-session inputs, publish outputs, locks, status, artifacts listing, restore, and validation. `internal/artifactpolicy` solved much of this, but Scriptorium input validation and previous-session candidate derivation still contain source-vocabulary logic outside the policy layer. + +Recommended refactor: + +Extend `internal/artifactpolicy` with narrow Scriptorium-input helpers, not a generic artifact engine: + +- `ValidateScriptoriumInputSource(source, configuredKeys)` returning a classified source plus any referenced configured artifact key. +- a shared helper for "is this source a built-in runtime input source?" +- a small previous-session source descriptor used by config validation, previous-cache planning, and analyze resolution. + +Keep missing/required behavior at call sites. For example, `analyze` should still decide whether a missing optional input is skipped or a missing required input fails with prepare guidance. + +Suggested tests: + +- `internal/artifactpolicy`: valid/invalid Scriptorium input sources, previous-session source format, unknown configured references. +- `internal/config`: Scriptorium input validation still reports field-specific errors. +- `internal/stage`: analyze required/optional source behavior unchanged. +- `internal/previouscache`: previous-session requirements and candidate ordering unchanged. + +Risk level: Medium. This touches public config validation, but the existing tests are good and the policy surface can remain small. + +### Centralize Session-Relative Path Conversion and Safe Local Install Mechanics + +Affected files/packages: + +- `internal/app/restore_plan.go` +- `internal/app/restore_execute.go` +- `internal/stage/run_local.go` +- `internal/stage/prepare.go` +- `internal/previouscache/previouscache.go` +- `internal/artifacts/local.go` +- `internal/audio/s3_audio.go` +- `internal/manifest/store.go` +- `internal/pathsafe` + +Duplicated or near-duplicated behavior: + +- `restore_plan.joinWithinSessionRoot` validates session-relative restore targets. +- `previouscache.relativeToSession`, `deriveManifestRelativePath`, and `manifestSessionRoot` convert local manifest paths back into session-relative paths. +- `stage.runLocalPathForCanonical` validates that canonical outputs stay within a session root and skips `previous/**`. +- `artifacts.resolveInRoot` validates relative paths under an artifact root. +- Atomic write/copy/install flows appear in `artifacts.LocalStore`, `audio.MaterializeS3Audio`, `restore_execute`, `manifest.LocalStore`, and `prepare` helper functions. + +Why it matters: + +These helpers are individually careful, but they all express the same safety policy: relative paths must not escape a scoped root, writes should use temp files plus rename, and cleanup should not leave partial outputs. This is exactly the kind of code where drift is costly. + +Recommended refactor: + +Add narrow helpers without moving application semantics: + +- in `internal/pathsafe`, add session/root helpers such as `JoinWithinRoot(root, rel)` and `RelativeWithinRoot(root, absoluteOrRelative)`; +- add a tiny file operation helper package, or extend `artifacts.LocalStore` carefully, for atomic install/copy/write with optional checksum; +- keep restore scope classification in `restore_plan.go`, because the include/exclude roots are command-specific. + +Suggested tests: + +- `internal/pathsafe`: root escape, absolute path rejection, Windows-style separators, empty paths, valid session-relative joins. +- `internal/app`: restore conflict/force behavior unchanged. +- `internal/stage`: run-local materialization paths unchanged. +- `internal/audio`: cache hit/miss behavior unchanged. +- `internal/manifest`: atomic manifest save behavior unchanged. + +Risk level: Low to Medium. The mechanics are well-contained, but file writes are sensitive and should be protected by focused tests. + +### Extract Shared Read-Only Session Inspection Checks + +Affected files/packages: + +- `internal/app/operator_findings.go` +- `internal/app/operator_session_validate.go` +- `internal/app/operator_status.go` +- `internal/stage/prepare.go` +- `internal/previouscache/previouscache.go` +- `internal/audio` +- `internal/artifacts/current_state.go` + +Duplicated or near-duplicated behavior: + +- `session validate` resolves stable input paths and checks files in a way that mirrors prepare. +- `session validate` checks local audio or remote audio by listing S3 audio objects, while prepare has the materialization path and restore has audio cache materialization. +- `session validate` checks previous-session current state, while previous-cache planning performs a deeper check and restore uses `previouscache.BuildPlan`. +- `status` independently loads local manifest state, remote current state, effective locks, and published output state. + +Why it matters: + +These commands are read-only, so divergence does not corrupt state. But operator trust depends on them matching runtime behavior. If validate says a session is ready while prepare would fail, or status reports a remote condition differently from restore, operators get confusing guidance. + +Recommended refactor: + +Create a small app-level inspection layer, not a workflow engine: + +- one helper for resolved stable-input file checks; +- one helper for audio presence checks that mirrors prepare's selection rules without downloading bodies; +- one helper that can run previous-cache planning in a "metadata only / no body download" mode where possible; +- one helper result type that status and validate can render differently. + +Do not move output formatting into this layer. Keep command-specific text in `internal/app`. + +Suggested tests: + +- `internal/app`: validate/status agree on local vs remote audio state. +- `internal/app`: previous-session missing current pointer/manifest is rendered consistently. +- `internal/previouscache`: optional and required previous inputs keep existing behavior. +- `internal/stage`: prepare behavior remains the authority for actual materialization. + +Risk level: Medium. The semantic differences between status, validate, prepare, and restore are intentional in places, so the helper should centralize checks, not command policy. + +### Atomic File Operation Helpers Are Repeated + +Affected files/packages: + +- `internal/artifacts/local.go` +- `internal/audio/s3_audio.go` +- `internal/app/restore_execute.go` +- `internal/manifest/store.go` +- `internal/stage/prepare.go` + +Duplicated or near-duplicated behavior: + +- create parent directory; +- create sibling temp file; +- copy/write bytes; +- sync/close; +- chmod; +- rename; +- remove temp file on failure; +- sometimes compute SHA-256 while copying. + +Why it matters: + +The duplication is not large, but these are correctness-sensitive operations. A future hardening change, such as more consistent `fsync`, permissions, or Windows behavior, would need repeated edits. + +Recommended refactor: + +Add a small dependency-light file helper, for example `internal/fileops`, with: + +- `WriteAtomic(path, data, perm, pattern)`; +- `CopyAtomic(src, dst, perm)`; +- `CopyAtomicWithSHA256(src, dst, perm)`; +- `InstallDownloadedTemp(tmp, dst, perm)`. + +Keep manifest JSON marshaling, audio cache policy, restore plan policy, and artifact store semantics in their existing packages. + +Suggested tests: + +- temp file removed on write/copy failure; +- destination parent creation; +- checksum from copy matches final file; +- restore and audio cache tests continue to pass. + +Risk level: Low. This is mechanical but should be done in a small commit. + +## 3. Medium-Confidence Opportunities + +### CLI Parsing Is Mostly Centralized, With Intentional Special Cases + +Affected files/packages: + +- `internal/app/session_args.go` +- `internal/app/operator_helpers.go` +- `internal/app/run.go` +- `internal/app/resume.go` +- `internal/app/run_stage.go` +- `internal/app/operator_locks.go` +- `internal/app/restore.go` + +Current state: + +Common config flags and session ID matching are centralized enough for 1.0. `run-stage`, `locks add`, and `locks remove` still do command-specific positional parsing because they have extra positional arguments. + +Recommended refactor: + +Do not introduce a CLI framework. If another session command is added, consider a small parser helper for "session id plus one additional positional argument" so `locks add/remove` and any future similar commands share the same mismatch behavior. + +Suggested tests: + +- existing session-oriented CLI tests; +- positional session ID plus `--session-id` mismatch; +- missing extra positional source for locks add/remove. + +Risk level: Low. + +### Restore Scope Mapping Should Probably Stay Local + +Affected files/packages: + +- `internal/app/restore_plan.go` +- `docs/internal/command-restore.md` + +Current state: + +Restore maps remote keys back to local paths and intentionally includes only `manifest.json`, `transcripts/**`, `artifacts/**`, and optionally `audio/**`. It explicitly excludes current pointers, run history, logs, reports, configs, inputs, and current-session `previous/**`. + +Why it is not a high-priority refactor: + +This policy is restore-specific. Generalizing it too much would obscure the command contract. + +Recommended refactor: + +Leave it local unless restore gains another caller. If it grows, extract only the classifier into a table-driven helper with tests. + +Risk level: Low. + +### Command Output Formatting Is Intentionally Text-Local + +Affected files/packages: + +- `internal/app/operator_findings.go` +- `internal/app/operator_artifact_rendering.go` +- `internal/app/operator_locks.go` +- `internal/app/restore_report.go` +- `internal/app/clean.go` +- `internal/app/plan.go` + +Current state: + +Each command renders plain text directly. Validation-style findings share a small renderer. Artifact list/status share artifact rendering. + +Recommended refactor: + +Do not add a generic renderer before 1.0. Add focused helper functions only if a user-facing inconsistency is found. + +Risk level: Low. + +### Manifest Transition Logic Is Dense but Correctly Centralized + +Affected files/packages: + +- `internal/app/runner.go` +- `internal/manifest` + +Current state: + +The runner owns manifest/run-manifest lifecycle, stage transitions, downstream stale marking, and post-publish cleanup gating. The function is long, but it is one explicit orchestration path. + +Recommended refactor: + +Avoid a broad manifest abstraction. If desired after 1.0, extract tiny helpers for repeated save/error wrapping inside the runner only. + +Risk level: Low. + +## 4. Boundary and Responsibility Concerns + +Healthy boundaries: + +- `internal/adapters/storage` exposes only `ObjectStore`; AWS SDK types stay inside the S3 backend. +- App code owns secret loading and object-store initialization. +- `internal/artifacts` owns local path and S3 key helpers. +- `internal/artifactpolicy` owns most public source vocabulary. +- `internal/audio` owns S3 audio cache materialization. +- Stage code remains explicit and adapter-facing. + +Boundary concerns: + +- Scriptorium input source validation still lives partly in `internal/config` instead of fully using source policy. +- Previous-cache candidate selection mixes manifest interpretation, publish metadata, and configured artifact output paths in `internal/previouscache`; this is defensible but should share source descriptors with artifact policy. +- Read-only operator checks mirror stage behavior in `internal/app`; if they grow, they should move into a small inspection layer rather than further expanding command handlers. +- File operation mechanics are repeated across packages; a narrow helper would clarify that write safety is shared mechanics, not stage policy. + +Recommended homes: + +- source vocabulary and validation: `internal/artifactpolicy`; +- concrete runtime artifact lookup: `internal/artifacts`; +- remote current-state mechanics: `internal/artifacts`; +- command orchestration and rendering: `internal/app`; +- file write mechanics: a small dependency-light helper such as `internal/fileops`, or carefully scoped methods on `artifacts.LocalStore`; +- stage execution policy: `internal/stage`. + +## 5. Path and Remote Key Construction Review + +Remote key construction is centralized enough for 1.0: + +- session, run, audio, session config, locks, current-state, published output, and run-relative keys are in `internal/artifacts/s3_keys.go`; +- storage adapters normalize object keys but do not infer Narratio semantics; +- publish identity helpers resolve bucket/session/run/current-state identities without moving layout policy into storage. + +Local path construction is mostly centralized: + +- session roots, run roots, previous cache paths, spool paths, and S3 audio cache paths are in `internal/artifacts/paths.go`; +- cleanup target validation is in `internal/app/cleanup_targets.go`; +- relative publish destinations use `internal/pathsafe`. + +Areas worth cleanup: + +- session-relative conversion helpers are repeated in restore, previous-cache, run-local stage code, and artifacts local-store helpers; +- `restore_plan.go` has local remote-key normalization, which is acceptable for restore but should not spread; +- tests still build some S3 keys through string concatenation when fixture readability would not suffer from using helpers. + +Recommended action: + +Add narrow `pathsafe` helpers for "join/rel within root" and use them where they reduce escape-check duplication. Do not move restore's include/exclude scope policy out of restore unless it gains another caller. + +## 6. Artifact/Catalog/Source Resolution Review + +Current state is good: + +- transcript source IDs live in `internal/artifactmodel` and are exposed through `internal/artifacts`; +- configured and previous-session source formats live in `internal/artifactpolicy`; +- publish output destination derivation is centralized in `artifactpolicy.ResolvePublishedDestination`; +- runtime catalog behavior is in `internal/artifacts`; +- previous-session requirements are collected by `artifacts.CollectPreviousArtifactRequirements`. + +Remaining gap: + +`artifactpolicy` does not yet cover the full Scriptorium-input validation contract. `internal/config/validate.go` still knows too much about source parsing and built-in source support. `stage/analyze.go` still has source-family-specific missing input messages, which is appropriate, but it should be consuming a richer policy classification rather than re-checking source strings. + +Recommendation: + +Extend the policy layer one step further, but keep runtime existence checks in `internal/artifacts` and command/stage missing behavior at the call sites. + +## 7. Config and Command-Loading Review + +Config loading is consistent: + +- `loadCommandConfig` handles pipeline, campaign, local session discovery, remote session fallback, and session identity checks; +- `loadPipelineCampaignConfig` correctly supports `session init`, which cannot load an existing session; +- `newCommandObjectStore` loads secret files before constructing storage; +- concrete-only session loading is enforced in `internal/config`. + +Intentional differences: + +- `session init` loads pipeline and campaign only. +- `clean --all` is pipeline-scoped, not session-scoped. +- `status`, `validate`, `artifacts`, and `locks` render command-specific output after shared loading. +- `restore` has richer help/output behavior and therefore does slightly more local parsing setup. + +Likely accidental drift remaining: + +- small positional parsing patterns for commands with extra arguments are repeated; +- `session validate` and `status` each decide which remote checks are warnings, errors, or status lines; +- `loadHelperContext` is useful but not used by every helper because some commands need partial failure reporting. + +Recommendation: + +No broad loader rewrite is needed. Future command work should reuse `commonConfigFlags`, `parseSessionAwareFlags`, `loadCommandConfig`, `loadPipelineCampaignConfig`, and `newCommandObjectStore`. + +## 8. Refactors to Avoid Before 1.0 + +Avoid: + +- a generic workflow engine or DAG abstraction; +- a generic CLI framework; +- broad manifest query or transition abstractions; +- moving secret loading into storage adapters; +- making storage adapters infer campaign/session/root-prefix semantics; +- centralizing all command text output into a generic renderer; +- merging audio cache materialization with generic restore downloads; +- compatibility aliases for retired archive/promote, legacy campaign, or legacy transcript names; +- moving restore's command-specific scope policy into storage or artifacts. + +These would add risk without solving current release problems. + +## 9. Recommended Implementation Sequence + +1. Add root-scoped path helpers and optional atomic file helpers. + - Scope: `internal/pathsafe` plus a small file helper if chosen. + - Tests: `internal/pathsafe`, `internal/audio`, `internal/app -run Restore`, `internal/stage -run Prepare`. + +2. Extend artifact source policy for Scriptorium inputs. + - Scope: `internal/artifactpolicy`, `internal/config/validate.go`, `internal/stage/analyze.go`, `internal/previouscache`. + - Tests: `internal/artifactpolicy`, `internal/config`, `internal/stage -run Analyze`, `internal/previouscache`. + +3. Extract read-only session inspection checks. + - Scope: stable inputs, audio presence, previous-session readiness, locks/current-state checks. + - Tests: `internal/app -run 'SessionValidate|Status'`, `internal/previouscache`, `internal/stage -run Prepare`. + +4. Trim command parsing edge duplication only if needed. + - Scope: commands with session ID plus one extra positional argument. + - Tests: `internal/app -run 'Session|Locks|RunStage'`. + +5. Final sweep. + - Run focused searches for retired terminology and old source names. + - Run the focused package tests listed below and then `go test ./...`. + +## 10. Test Strategy + +Focused checks for any cleanup work: + +- `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` + +Full validation after each implementation prompt: + +- `go test ./...` + +Useful 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` +- `rg -n "session_id is required|unexpected positional|--artifacts" internal/app` +- `rg -n "CreateTemp|Rename|copyFileAtomic|WriteFileAtomic|DownloadObjectToTemp" internal` + +## 11. Appendix: Findings Not Worth Acting On + +- Restore's include/exclude scope logic should remain restore-local. It is command policy, not general path policy. +- Direct text rendering in command handlers is acceptable. The output is text-only and command-specific by design. +- Stage-local path joins under run-local directories are acceptable when they use established session/run roots. +- `run-stage`, `locks add`, and `locks remove` deserve explicit positional parsing because their syntax is not identical to simple session commands. +- The runner is long, but it is the right place for explicit stage orchestration and manifest transitions. +- `session init` template rendering should remain separate from ordinary concrete session loading. +- S3 audio cache materialization is intentionally special and should not be folded into generic object download logic.