narratio/docs/roadmap/audit.md

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.