narratio/docs/roadmap/audit.md

Roadmap: Code Quality and Deduplication Audit

Status: Draft audit report

This report is a pre-1.0 implementation audit focused on high-confidence opportunities to simplify, centralize, or clarify Narratio before release. It is intentionally report-only: no refactors are included here.

The requested docs/architecture.md and docs/development.md paths do not exist in the current tree. This audit used the current policy documents at docs/policy/architecture.md and docs/policy/development.md, plus the current user, operator, and internal docs.

1. Executive Summary

Overall code quality is solid. The codebase has strong package boundaries in the important places: storage adapters expose a narrow object-store interface, AWS SDK types do not leak into app or stage logic, config loading is strict, and pipeline execution remains explicit and stage-driven. Recent pre-1.0 work has also produced useful central points for campaign/session config loading, secret-backed object-store creation, S3 audio caching, transcript artifact naming, local session paths, and S3 key construction.

The main release risk is not a large architectural flaw. It is policy drift from rapid feature growth. Several public-interface decisions now appear in more than one implementation path: artifact source interpretation, publish-output destination derivation, remote current-state inspection, cleanup safety checks, and session-oriented command parsing. Most of these are correct today, but a future bug fix would likely have to be made in multiple files.

Top three refactoring targets before 1.0:

1. Centralize artifact source and publish-output resolution across config validation, publish execution, status/artifacts output, restore, previous-cache hydration, and analyze input resolution.
2. Consolidate shared session-command flag parsing and config-loading context for run/resume/run-stage/analyze/publish/restore/clean/session helpers without introducing a generic command framework.
3. Finish the publish terminology cleanup internally so public publish behavior is not implemented through archive-named files, helpers, errors, and tests.

The codebase appears ready for a limited cleanup pass. No major architecture rewrite is warranted before 1.0.

2. High-Confidence Deduplication Opportunities

Artifact Source and Publish Destination Policy Is Split Across Packages

Affected files/packages:

• internal/config/validate.go
• internal/artifacts/artifact_resolver.go
• internal/artifacts/catalog.go
• internal/stage/archive.go
• internal/app/operator_helpers.go
• internal/previouscache/previouscache.go
• internal/stage/analyze.go

Duplicated or near-duplicated behavior:

• Config validation accepts and derives destinations for pipeline.publish.outputs[] in publishSourceKnown and derivePublishOutputDest.
• Publish execution derives destinations again in resolvePublishOutputDest.
• Status and artifacts list derive destination display and remote checks in helperPublishedOutputDest.
• Previous-cache hydration reconstructs candidate artifact locations from manifest outputs, published paths, and configured Scriptorium paths in artifactRelativePathCandidates.
• Analyze resolves previous-session, built-in, and configured artifact sources separately in resolveScriptoriumInput.

Why it matters:

Artifact source IDs now define the public contract for analyze inputs, previous-session inputs, publish outputs, locks, status, artifacts listing, restore, and validation. When source interpretation is spread across these packages, it is easy for one path to accept, reject, or resolve a source differently from another.

Recommended refactor:

Create one small artifact-source policy layer, likely in internal/artifacts or a dependency-light sibling of internal/artifactmodel, that can:

• classify source IDs as built-in, configured artifact, or previous-session configured artifact;
• validate a source against the current Scriptorium config;
• derive the default published destination for a source;
• normalize relative artifact destinations;
• return consistent display metadata for status and artifacts output.

Then update config validation, publish execution, helper commands, previous-cache planning, and analyze input resolution to call that policy instead of deriving partial answers locally.

Suggested tests:

• internal/artifacts: source classification, configured artifact validation, default destination derivation, relative destination normalization.
• internal/config: publish outputs and locks validate through the shared policy.
• internal/stage: publish output resolution preserves locked, optional, required, and selected-artifact behavior.
• internal/app: artifacts list, status, and locks use the same source rules as publish.
• internal/previouscache: previous-session source resolution still checks manifest outputs, published paths, and configured output paths in the intended order.

Risk level: Medium. The behavior is public, but a table-driven shared policy should reduce risk if introduced behind existing tests.

Publish Terminology Cleanup Is Incomplete Internally

Affected files/packages:

• internal/stage/archive.go
• internal/stage/archive_test.go
• internal/artifacts/archive_identity.go
• internal/app/post_archive_cleanup.go
• internal/app/remote_locks.go
• internal/app/operator_helpers.go
• tests under internal/app and internal/config
• internal/adapters/storage/archive.go

Duplicated or near-duplicated behavior:

The public contract now uses publish, published, and publish outputs, but several internal names still use archive, promotion, or promoted. Examples include archiveStage, ResolveArchiveSessionPrefix, ResolveArchiveCurrentStateKeys, runPostArchiveCleanup, staticArchiveLocks, normalizeArchiveRelativePath, and test names such as TestArchiveUploadsRunRecordPromotionsAndCurrentPointer.

Why it matters:

This is mostly clarity risk, not current behavior risk. However, public docs and config now use publish terminology, while implementation and tests still use old names. This makes code review harder and increases the chance that future work reintroduces old config or command language.

Recommended refactor:

Do a mechanical naming cleanup after artifact-source policy is centralized:

• rename internal/stage/archive.go to a publish-oriented file and rename archiveStage to publishStage;
• rename archive identity helpers to publish/current-state helpers while keeping S3 layout unchanged;
• rename post-archive cleanup helpers and tests to post-publish cleanup;
• update old comments and test failure messages that still say archive/promote when they mean publish/published;
• leave the immutable run-history path runs/{run_id} unchanged.

Suggested tests:

• Existing internal/stage, internal/app, and internal/artifacts tests.
• A final term sweep for old terminology, allowing only historical roadmap references and adapter names that are intentionally retained.

Risk level: Low to Medium. Mostly mechanical, but broad enough to create churn.

Session-Oriented CLI Parsing Is Repeated

Affected files/packages:

• internal/app/run.go
• internal/app/resume.go
• internal/app/run_stage.go
• internal/app/restore.go
• internal/app/clean.go
• internal/app/operator_helpers.go
• internal/app/session_args.go

Duplicated or near-duplicated behavior:

Many commands repeat the same flag setup and session ID handling:

• --config, --campaign, --campaign-file, --session, and --previous-session-id;
• positional session ID extraction;
• --session-id compatibility through applyParsedSessionIDArg;
• selected artifact parsing and validation for run/resume/analyze/publish/run-stage;
• load through loadCommandConfig followed by config.Validate.

Why it matters:

The command set has recently moved toward narratio session <subcommand> <session_id> and shorter top-level convenience commands. Repeated parser setup makes it easy for one command to miss a new flag, use a stale help string, or apply session ID precedence differently.

Recommended refactor:

Keep command functions explicit, but add a small internal parser helper for common session-aware commands. Avoid a generic CLI framework. A good target is a helper that returns:

• common config flags;
• resolved positional/flag session ID;
• previous session override;
• optional selected configured artifacts;
• normalized command-specific positional validation.

run-stage can remain special because it has both stage and session positional arguments, but it should reuse the same common flag registration and selected-artifact parsing.

Suggested tests:

• Existing app command tests for run, resume, run-stage, analyze, publish, restore, clean, and session subcommands.
• Focused tests for positional session ID vs --session-id mismatch, missing session ID, and unsupported --artifacts by command/stage.

Risk level: Medium. Refactor is local to app parsing but touches many public commands.

Remote Current-State Discovery Is Reimplemented in Several Forms

Affected files/packages:

• internal/app/restore_discovery.go
• internal/previouscache/previouscache.go
• internal/app/operator_helpers.go
• internal/stage/prepare_previous.go
• internal/app/remote_locks.go

Duplicated or near-duplicated behavior:

Several paths check or download remote current state:

• restore discovers current run ID and current manifest, validates campaign/session identity, and decodes the manifest;
• previous-cache planning repeats current run pointer and manifest checks for the previous session;
• session validation checks previous current state with Exists calls;
• remote lock loading separately checks and downloads locks.yml;
• remote session fallback lists and downloads session.yml.

Why it matters:

These workflows are similar but not identical. Some need missing remote state to be an error, while status treats it as state. Still, the low-level sequence of key construction, Exists, temp download, decode, and campaign/session/run validation appears multiple times.

Recommended refactor:

Extract narrow app-level or artifact-level helpers for remote session state objects, not a generic storage workflow engine. Candidate helpers:

• download object to temp safely;
• load current run pointer and manifest for a supplied session prefix;
• validate downloaded current manifest identity;
• represent missing current state as a typed error so status can downgrade it while restore/prepare fail.

Keep storage.ObjectStore as the boundary and keep S3 key construction in internal/artifacts.

Suggested tests:

• internal/app: restore current-state discovery, status missing-state behavior, session validate previous-state behavior.
• internal/previouscache: required vs optional previous artifact behavior with missing current pointers/manifests.
• internal/app: malformed remote lock/session data still fails closed where publish-capable execution requires it.

Risk level: Medium. The missing-state policy differs by caller, so the refactor should centralize mechanics and typed outcomes, not final command decisions.

Safe Local Deletion Policy Is Duplicated

Affected files/packages:

• internal/app/clean.go
• internal/app/post_archive_cleanup.go

Duplicated or near-duplicated behavior:

Both clean and post-publish cleanup implement scoped deletion checks:

• reject empty roots/targets;
• resolve absolute paths;
• refuse root deletion;
• refuse deletion outside the configured root;
• refuse symlink deletion;
• handle missing targets as successful no-ops.

Why it matters:

Deletion policy is high-risk code. Even if the current implementations agree, future fixes should not need to be made twice.

Recommended refactor:

Extract a small app-level cleanup safety helper, for example cleanup_target.go, with functions for:

• validating a scoped directory target;
• validating a scoped file target;
• validating removable children under a root.

Keep command-specific reporting in clean.go and manifest metadata handling in post-publish cleanup.

Suggested tests:

• Move the existing focused unsafe-path tests to the shared helper.
• Preserve clean dry-run tests and post-publish cleanup eligibility tests.

Risk level: Low. This is a contained refactor with clear behavior preservation.

Temp Object Download Helper Is Duplicated

Affected files/packages:

• internal/app/restore_discovery.go
• internal/previouscache/previouscache.go
• internal/app/remote_locks.go
• internal/app/config_loader.go

Duplicated or near-duplicated behavior:

Multiple call sites create a temp file, close it, download an object into it, and delete it on error or defer deletion. The app package has one downloadObjectToTemp, while internal/previouscache has another copy.

Why it matters:

Temp-download behavior affects cleanup, error wording, and future hardening. It is not worth abstracting all storage use, but this small operation is repeated enough to centralize.

Recommended refactor:

Add a narrow helper close to the storage boundary. Options:

• internal/adapters/storage helper only if it does not learn Narratio session semantics;
• internal/storageutil if a small internal utility package is acceptable;
• app-level helper plus a previouscache dependency inversion if the team wants to avoid a new package.

The helper should not hide ObjectStore; it should only implement safe temp download mechanics.

Suggested tests:

• temp file cleanup on failed download;
• successful download returns a cleaned temp path;
• callers preserve their current contextual error messages.

Risk level: Low.

3. Medium-Confidence Opportunities

Operator Helper Implementation Is Too Broad for One File

Affected files/packages:

• internal/app/operator_helpers.go

Duplicated or near-duplicated behavior:

This 1,100+ line file owns session validation, status, session init, artifacts listing, locks list/add/remove, lock-store mutation, artifact catalog rendering, remote output availability, finding formatting, local input validation, and template rendering.

Why it matters:

The code is not inherently wrong, and keeping helper commands in internal/app fits the architecture. The issue is discoverability and local coupling. Small changes to one helper command require navigating unrelated helper behavior.

Recommended refactor:

Split by command or responsibility:

• session_init.go
• session_validate.go
• status.go
• artifacts_list.go
• locks.go
• helper_findings.go
• helper_artifacts.go

Do this only after higher-value policy centralization so the file split does not preserve duplicated logic under new names.

Suggested tests:

• Existing internal/app/operator_helpers_test.go can be split later, but a file split alone should not require behavior changes.

Risk level: Low.

Restore Planning Contains Its Own Remote-to-Local Path Policy

Affected files/packages:

• internal/app/restore_plan.go

Duplicated or near-duplicated behavior:

Restore maps remote session keys back to local session paths in restoreLocalRelativePathForKey, with explicit include/exclude rules for current/, runs/, logs/, reports/, config/, inputs/, transcripts/, artifacts/, previous/, and optional audio/.

Why it may be intentional:

Restore is the only command that should translate an entire remote session prefix into a local session subset. It has command-specific conflict and --include-audio semantics.

Recommended refactor:

Do not generalize this immediately. If it changes again, move only the remote-key-to-local-restore-scope classifier into a small helper with table-driven tests. Leave restore action classification local to restore.

Suggested tests:

• Restore scope tests for every included/excluded root.
• Audio-specific conflict behavior remains separate.

Risk level: Low.

Manifest Output Scanning Is Repeated but Mostly Stage-Specific

Affected files/packages:

• internal/artifacts/artifact_resolver.go
• internal/previouscache/previouscache.go
• internal/app/runner.go

Duplicated or near-duplicated behavior:

Several call sites inspect manifest stage outputs or metadata to find artifact paths, published paths, run roots, or configured artifact outputs.

Why it may be intentional:

Manifest state has different meanings depending on caller: runtime artifact resolution, previous-cache reconstruction, and run summary construction are not the same policy.

Recommended refactor:

Avoid a broad manifest-query abstraction before 1.0. Consider adding only narrow helpers for stable metadata reads, such as reading published_paths from the publish stage, if the previous-cache and restore paths continue to grow.

Suggested tests:

• Existing manifest resolver tests plus previous-cache tests.

Risk level: Low.

Command Output Formatting Could Be More Consistent

Affected files/packages:

• internal/app/operator_helpers.go
• internal/app/restore_report.go
• internal/app/restore_plan.go
• internal/app/clean.go
• internal/app/plan.go

Duplicated or near-duplicated behavior:

Status, session validate, artifacts list, locks, clean dry-run, restore dry-run, and plan all render text directly with fmt.Fprintf.

Why it may be intentional:

The output remains text-only and command-specific. A generic renderer would add complexity without much value.

Recommended refactor:

Postpone unless user-facing inconsistencies become painful. A small findings renderer already exists for validation-style output; that is enough for now.

Suggested tests:

• Snapshot-style output tests only for stable operator-facing lines that support workflows.

Risk level: Low.

4. Boundary and Responsibility Concerns

The major boundaries are healthy:

• internal/adapters/storage owns external storage implementation details.
• App code creates object stores through newCommandObjectStore, which loads filesystem secrets first.
• Stage code depends on storage.ObjectStore, not AWS SDK types.
• internal/audio correctly centralizes S3 audio cache materialization without making the storage adapter aware of cache policy.
• internal/artifacts owns most local paths and S3 keys.

Concerns to address:

• Artifact source policy is split between internal/config, internal/artifacts, internal/stage, internal/app, and internal/previouscache. This is the clearest boundary drift because source IDs are a shared public contract.
• internal/config currently derives default publish destinations. Validation should be able to call source policy, but the canonical mapping itself should live outside config.
• internal/app/operator_helpers.go owns artifact catalog rendering and remote published-output state. That is acceptable for formatting, but destination derivation and source classification should move out.
• internal/stage/archive.go implements the public publish stage. This does not violate boundaries, but it creates conceptual drift.

Recommended home for shared logic:

• Source classification and destination derivation: internal/artifacts or internal/artifactmodel plus a small adapter from Scriptorium config.
• Remote key construction: continue using internal/artifacts.
• Object-store initialization: keep in internal/app.
• Command parsing: keep in internal/app.
• Stage-specific execution policy: keep in internal/stage.

5. Path and Remote Key Construction Review

Local path construction is mostly centralized:

• internal/artifacts/paths.go owns session work roots, run roots, spool paths, previous-cache paths, and audio cache paths.
• Stage code often gets artifacts.SessionPaths and joins stage-local files from those roots, which is appropriate.
• The previous-cache redundant nested artifact path has already been addressed by previousArtifactCacheRelativePath.

Remote key construction is mostly centralized:

• internal/artifacts/s3_keys.go owns session prefixes, run prefixes, audio prefixes, session.yml, locks.yml, current manifest/run pointer keys, published output keys, and run-relative keys.
• App and stage code call these helpers rather than scattering full S3 key string concatenation.

Areas needing cleanup:

• ResolveArchiveBucket, ResolveArchiveSessionPrefix, ResolveArchiveRunPrefix, and ResolveArchiveCurrentStateKeys should be renamed to publish/current-state terminology.
• normalizeArchiveRelativePath exists in both internal/stage/archive.go and internal/previouscache/previouscache.go; normalizeHelperArchiveRelativePath exists in internal/app/operator_helpers.go. These should converge into one helper for clean relative artifact destination paths.
• restore_plan.go owns normalizeRemoteKey and remote key scope mapping. That may remain restore-specific, but it should be watched because it overlaps with S3 key normalization helpers.
• downloadObjectToTemp exists in more than one package and can be centralized.

6. Artifact/Catalog/Source Resolution Review

Artifact source handling has a strong foundation:

• Transcript source IDs and paths are centralized in internal/artifactmodel/transcripts.go.
• Runtime artifact registry and resolver live in internal/artifacts/artifact_resolver.go.
• Configured artifact source IDs are consistently formed by artifacts.ConfiguredArtifactSourceID.
• Previous-session source IDs are recognized by artifacts.PreviousSessionArtifactName.
• The runtime catalog supports built-ins, configured artifacts, selected artifact execution, and availability.

The remaining issue is that consumers still build their own partial views of this model:

• config validation validates and derives publish output destinations;
• publish execution resolves included outputs, skipped optional outputs, skipped unselected outputs, and locked outputs;
• status/artifacts list derives display destinations and remote published state;
• previous-cache planning reconstructs candidate remote paths from previous manifests and publish metadata;
• analyze input resolution has its own missing-source messages and previous-session behavior.

Recommendation:

Make artifact/source resolution the next cleanup target. The goal is not to create one all-purpose resolver. The goal is to centralize the public source vocabulary and destination derivation so each caller can keep its own policy for missing/required/locked behavior.

7. Config and Command-Loading Review

Config loading is generally consistent:

• loadCommandConfig is the main command path for pipeline, campaign, session, local discovery, and remote session fallback.
• loadPipelineCampaignConfig covers commands that create session config and therefore cannot load an existing session.
• newCommandObjectStore correctly centralizes secret-backed object-store creation.
• config.LoadSessionBytesWithOptions now rejects session templates outside session init, preserving strict concrete session loading.

Intentional differences:

• session init loads only pipeline and campaign because it creates session.yml.
• clean --all loads only pipeline because it is not session-specific.
• status --manifest remains a compatibility/local-manifest mode.

Likely accidental drift to clean up:

• Common flags and help strings are repeated across commands.
• Some helper-command messages still say archive where they now mean publish.
• restore uses fs.SetOutput(out) while most other command parsers discard flag package output and wrap errors themselves. This may be intentional for --help, but it is a difference worth documenting or standardizing.
• App tests and helper names still contain old archive/promotion terminology, making it harder to see which public contract is current.

8. Refactors to Avoid Before 1.0

Avoid these before release:

• A generic workflow engine or DAG abstraction. The explicit stage list is a core design choice and is working.
• A broad manifest query framework. Add narrow helpers only where repeated policy is clear.
• Moving secret loading into storage adapters. Secret loading is app orchestration policy and should stay out of adapters.
• Making storage adapters infer campaign/session/root-prefix semantics. They should continue to receive concrete keys.
• Replacing command functions with a generic CLI framework. Small shared flag parsers are enough.
• Generalizing all file copy/download behavior. S3 audio cache materialization is intentionally special; ordinary restore/download logic has different semantics.
• Adding compatibility aliases for old archive/promote or old transcript names during cleanup. The repo has intentionally made hard cutovers.

9. Recommended Implementation Sequence

1. Centralize relative artifact destination normalization and temp object download helpers.

   • Scope: low-risk shared helpers for repeated mechanics.
   • Tests: internal/artifacts or helper-package tests, plus existing app/stage tests.

2. Centralize artifact source and publish-output policy.

   • Scope: source classification, source validation, default published destination derivation, destination normalization.
   • Tests: internal/artifacts, internal/config, internal/stage -run Publish, internal/app -run 'Artifacts|Status|Locks', internal/previouscache.

3. Finish publish terminology cleanup.

   • Scope: rename archive-named files/helpers/tests/comments where they now mean publish; keep S3 layout stable.
   • Tests: go test ./internal/stage -v, go test ./internal/app -v, go test ./internal/artifacts -v.

4. Consolidate session-aware command parsing.

   • Scope: common config/session/artifact flag registration and session ID resolution; no public CLI behavior change.
   • Tests: app command tests for run, resume, run-stage, analyze, publish, restore, clean, session helpers.

5. Extract remote current-state mechanics.

   • Scope: shared helpers for current run pointer/manifest load and identity validation, with typed missing-state errors.
   • Tests: restore discovery, previous-cache, status, session validate.

6. Split operator helper implementation by responsibility.

   • Scope: file organization and small formatting/helper extraction only after policy deduplication.
   • Tests: existing internal/app tests.

7. Sweep dead transitional terminology and stale tests.

   • Scope: comments, test names, old strings, internal docs that still say archive/promote where publish is now canonical.
   • Tests: final rg sweeps plus full test run.

10. Test Strategy

Focused package checks for cleanup work:

• go test ./internal/artifacts -v
• go test ./internal/config -v
• go test ./internal/stage -run 'Analyze|Publish|Prepare|Restore' -v
• go test ./internal/app -run 'Run|RunStage|Analyze|Publish|Restore|Clean|Status|Artifacts|Locks|Session' -v
• go test ./internal/previouscache -v
• go test ./internal/adapters/storage -v
• go test ./internal/manifest -v

Tests to add or strengthen during follow-up refactors:

• one table of valid/invalid artifact source IDs used by config validation, publish, locks, status, and analyze;
• one table of default published destination derivation for built-in and configured artifacts;
• relative destination normalization and path traversal rejection;
• shared remote current-state load outcomes: missing pointer, missing manifest, malformed manifest, campaign mismatch, session mismatch, run ID mismatch;
• shared cleanup safety helper behavior for files, directories, roots, symlinks, and outside-root paths;
• common session command parsing behavior for positional session IDs, --session-id, mismatch errors, and unsupported artifacts flags.

Full validation after each cleanup commit:

• go test ./...

Useful final searches:

• rg -n "archive|promote|promoted|promotion" internal docs examples cmd
• rg -n "ResolveArchive|archiveStage|post_archive|staticArchive|normalizeArchive" internal
• 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

11. Appendix: Findings Not Worth Acting On

• Stage-local path joins for files inside a stage run directory are acceptable. They are local implementation details, not shared path policy.
• Direct fmt.Fprintf output in simple commands is acceptable. A generic renderer would likely obscure behavior.
• Restore's remote-session-prefix filtering is command-specific enough to stay local unless restore scope changes again.
• session init template rendering should remain separate from ordinary session loading. That separation is now a useful safety boundary.
• S3 audio cache materialization is already centralized in internal/audio; do not fold it into a generic downloader.
• Manifest-driven resume behavior should not be abstracted broadly. The explicit runner behavior is easier to audit.