diff --git a/docs/internal/adapters.md b/docs/internal/adapters.md index 4f9eff7..e28b198 100644 --- a/docs/internal/adapters.md +++ b/docs/internal/adapters.md @@ -14,9 +14,6 @@ Primary adapters: - `storage.ObjectStore` - `notify.Sender` -Legacy compatibility boundary: -- `storage.Backend` remains in the storage adapter package and defaults to `NoopBackend`; current pipeline stages use `storage.ObjectStore`. - ## Ownership Adapters own: - HTTP/subprocess/SDK argument and transport details. diff --git a/docs/internal/storage.md b/docs/internal/storage.md index 0c8422a..c979093 100644 --- a/docs/internal/storage.md +++ b/docs/internal/storage.md @@ -29,9 +29,6 @@ S3 constructor behavior: - `Upload` streams local file and returns remote metadata. - `Exists` maps not-found responses to `false`. -## Legacy Compatibility Interface -`storage.Backend` (with `ArchiveRequest`) remains as compatibility surface with `NoopBackend`; it is not used by current stage execution. - ## Invariants - storage layer is stateless regarding manifest/stage progression. - publish ordering semantics are owned by stage/app code, not storage adapters. diff --git a/docs/internal/workspace.md b/docs/internal/workspace.md index e8863d5..19cf398 100644 --- a/docs/internal/workspace.md +++ b/docs/internal/workspace.md @@ -40,7 +40,7 @@ Run-local outputs are materialized back into canonical session paths before stag `artifacts.LocalStore` enforces single-writer session lock via `.lock` file (`ErrLockConflict` on contention). ## Cleanup Semantics -Automatic post-publish cleanup (`runPostArchiveCleanup`): +Automatic post-publish cleanup: - only runs when publish actually executed and succeeded; - requires `uploaded=true` and `current_pointer_written=true` metadata; - respects `pipeline.spool.delete_audio_after_publish` and `pipeline.workspace.cleanup_after_publish`; diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md deleted file mode 100644 index c4c452d..0000000 --- a/docs/roadmap/audit.md +++ /dev/null @@ -1,556 +0,0 @@ -# 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 ` 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. diff --git a/docs/roadmap/cleanup.md b/docs/roadmap/cleanup.md deleted file mode 100644 index 136ae0d..0000000 --- a/docs/roadmap/cleanup.md +++ /dev/null @@ -1,294 +0,0 @@ -# Roadmap: Pre-1.0 Code Cleanup - -Status: Implemented (Stages 1-6 complete) - -This roadmap turns the findings in `docs/roadmap/audit.md` into staged cleanup work for the 1.0 release and records completion status for each selected stage. - -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. diff --git a/docs/roadmap/documentation-stage1-audit.md b/docs/roadmap/documentation-stage1-audit.md deleted file mode 100644 index 343699f..0000000 --- a/docs/roadmap/documentation-stage1-audit.md +++ /dev/null @@ -1,105 +0,0 @@ -# Documentation Pass: Stage 1 Audit - -Status: Completed (2026-05-23) - -## Scope Reviewed - -- All non-policy documentation files under `docs/` -- `README.md` -- Documentation references to maintained `examples/` files -- Documentation-related expectations in tests under `internal/**` - -## File Inventory and Canonical Scope - -| File | Intended audience | Canonical scope (per policy) | Primary source-of-truth anchors | -| --- | --- | --- | --- | -| `README.md` | Users, operators | Project orientation and links | `cmd/narratio`, `internal/app/commands.go`, docs index files | -| `docs/cli.md` | Users, operators | CLI syntax, flags, command workflows | `internal/app/*.go`, `internal/app/*_test.go` | -| `docs/config.md` | Operators, advanced users | Config discovery, schema, defaults, examples | `internal/config/*.go`, `internal/config/*_test.go`, `examples/*` | -| `docs/operations.md` | Operators | Run/resume/publish/restore/cleanup workflows | `internal/app/runner.go`, `internal/app/restore*.go`, `internal/stage/archive.go`, `internal/artifacts/*.go` | -| `docs/troubleshooting.md` | Operators | Failure diagnosis and safe fixes | `internal/app`, `internal/stage`, related tests | -| `docs/internal/README.md` | Developers, LLM coding agents | Internal docs index and scope boundaries | `docs/internal/*.md`, policy docs | -| `docs/internal/adapters.md` | Developers, LLM coding agents | Adapter boundaries and ownership | `internal/adapters/*`, `internal/stage/*` | -| `docs/internal/artifacts.md` | Developers, LLM coding agents | Artifact catalog and source resolution contracts | `internal/artifacts/*`, `internal/stage/analyze.go`, `internal/stage/prepare_previous.go` | -| `docs/internal/command-restore.md` | Developers, LLM coding agents | Restore command architecture and contracts | `internal/app/restore*.go`, `internal/app/restore*_test.go` | -| `docs/internal/manifest.md` | Developers, LLM coding agents | Session/run manifest contracts and transitions | `internal/manifest/*`, `internal/app/runner.go`, `internal/stage/*` | -| `docs/internal/stage-prepare.md` | Developers, LLM coding agents | Prepare stage IO and invariants | `internal/stage/prepare.go`, `internal/stage/prepare*_test.go` | -| `docs/internal/stage-transcribe.md` | Developers, LLM coding agents | Transcribe stage IO and invariants | `internal/stage/transcribe.go`, `internal/stage/transcribe_test.go` | -| `docs/internal/stage-merge.md` | Developers, LLM coding agents | Merge stage IO and invariants | `internal/stage/merge.go`, `internal/stage/merge_test.go` | -| `docs/internal/stage-polish.md` | Developers, LLM coding agents | Polish stage IO and invariants | `internal/stage/polish.go`, `internal/stage/polish_test.go` | -| `docs/internal/stage-normalize.md` | Developers, LLM coding agents | Normalize stage IO and invariants | `internal/stage/normalize.go`, `internal/stage/normalize_test.go` | -| `docs/internal/stage-trim.md` | Developers, LLM coding agents | Trim stage IO and invariants | `internal/stage/trim.go`, `internal/stage/trim_test.go` | -| `docs/internal/stage-analyze.md` | Developers, LLM coding agents | Analyze stage artifact execution and selection | `internal/stage/analyze.go`, `internal/stage/analyze_test.go` | -| `docs/internal/stage-publish.md` | Developers, LLM coding agents | Publish-stage commit/upload invariants | `internal/stage/archive.go`, `internal/stage/archive_test.go` | -| `docs/internal/storage.md` | Developers, LLM coding agents | Storage adapter contracts and semantics | `internal/adapters/storage/*`, `internal/app/object_store.go` | -| `docs/internal/workspace.md` | Developers, LLM coding agents | Local workspace/session/run path model | `internal/artifacts/*`, `internal/app/runner.go`, `internal/stage/run_local.go` | -| `docs/integrations/README.md` | Developers, LLM coding agents | Integration docs index | `docs/integrations/*.md` | -| `docs/integrations/audita.md` | Developers, integration maintainers | Audita adapter contract | `internal/adapters/audita/*`, `internal/stage/polish.go` | -| `docs/integrations/seriatim.md` | Developers, integration maintainers | Seriatim adapter contract | `internal/adapters/seriatim/*`, `internal/stage/merge.go`, `internal/stage/normalize.go`, `internal/stage/trim.go` | -| `docs/integrations/scriptorium.md` | Developers, integration maintainers | Scriptorium adapter contract | `internal/adapters/scriptorium/*`, `internal/stage/analyze.go`, `internal/stage/trim.go` | -| `docs/roadmap/documentation.md` | Developers, maintainers | Planning and implementation sequencing for documentation pass | N/A (planning artifact) | -| `docs/roadmap/documentation-stage1-audit.md` | Developers, maintainers | Stage-1 inventory and source-of-truth audit record | N/A (planning artifact) | - -## Source-of-Truth Mapping Summary - -- CLI behaviors and command names are grounded in `internal/app/commands.go` and command handlers in `internal/app/*.go`. -- Stage order and canonical stage names are grounded in `internal/stage/placeholders.go` (`prepare` -> `transcribe` -> `merge` -> `polish` -> `normalize` -> `trim` -> `analyze` -> `publish` -> `notify`). -- Publish behavior and current-pointer commit semantics are grounded in `internal/stage/archive.go`. -- Config schema/defaults/validation are grounded in `internal/config/*`. -- Local/remote paths, publish keys, and workspace layout are grounded in `internal/artifacts/*`. -- Restore behavior and report contracts are grounded in `internal/app/restore*.go`. -- Maintained examples and schema compatibility are grounded in `examples/*` plus `internal/config/load_validate_test.go` (`TestExamplesLoadAndValidate`). - -## Findings - -### Broken or stale references - -1. `README.md` linked to non-existent files: - - `docs/development.md` - - `docs/architecture.md` -2. `docs/internal/README.md` and `docs/integrations/README.md` linked to non-existent path: - - `docs/documentation/policy.md` - -Stage-1 fix applied: -- Updated those links to existing policy docs under `docs/policy/`. - -### Stale terminology sweep - -Sweep terms used: `archive`, `promote`, `promoted`, `promote_artifacts`, `run-stage archive`. - -Findings: -- User-facing docs in scope did not show obvious stale command examples requiring immediate correction. -- Internal code and tests still contain historical `archive` identifiers while user-facing command/stage naming is `publish` (for example, `internal/stage/archive.go` type names). This is acceptable for now but should be normalized deliberately, not incidentally. - -Stage-1 fix applied: -- Updated clearly stale publish-related wording in test expectation messages/comments: - - `internal/app/commands_test.go` - - `internal/app/operator_helpers_test.go` - -### Example path validation - -- All `examples/...` paths referenced from non-policy docs resolve to existing files. -- `internal/config/load_validate_test.go` includes `TestExamplesLoadAndValidate` and points to current example files. - -### Roadmap leakage into current-behavior docs - -- No obvious roadmap-only behavior leakage found in non-roadmap docs during this sweep. - -### Duplicate content and scope drift - -- No severe duplication requiring immediate rewrite in this stage. -- Existing docs still need full content rewrite for 1.0 readiness in later stages (user/operator first, then internal/integrations), as planned. - -### Canonical-home inconsistency to resolve in rewrite stages - -- Policy canonical-home language names `docs/architecture.md` and `docs/development.md`, while current repository stores those policy documents under `docs/policy/`. -- Stage 1 preserves repository behavior by fixing broken links to existing files. Later rewrite stages should converge canonical-home paths and references consistently across docs. - -## Stage-1 Completion Check - -Completed for this stage: -- Full non-policy file inventory with audience and scope mapping. -- Source-of-truth crosswalk to code/tests. -- Stale-term, link, and example-path sweeps. -- Documentation-related stale test wording corrections. -- Minimal fixes only; broad rewrites intentionally deferred. diff --git a/docs/roadmap/documentation.md b/docs/roadmap/documentation.md deleted file mode 100644 index 109d964..0000000 --- a/docs/roadmap/documentation.md +++ /dev/null @@ -1,237 +0,0 @@ -# Roadmap: 1.0 Documentation Pass - -Status: Completed (2026-05-23) - -## Goal - -Prepare Narratio documentation for a 1.0 release by reviewing and rewriting -every non-policy document under `docs/` against the implemented codebase. - -The finished documentation set should be accurate, concise, complete for its -audience, and compliant with: - -- `docs/policy/documentation.md` -- `docs/policy/architecture.md` -- `docs/policy/development.md` - -Do not modify files under `docs/policy/` during this pass. - -Current behavior belongs in canonical docs. Future, planned, aspirational, or -unimplemented behavior belongs only under `docs/roadmap/`. - -## Scope - -In scope: - -- `docs/*.md` -- `docs/internal/*.md` -- `docs/integrations/*.md` -- `docs/roadmap/*.md` -- documentation references to files under `examples/` -- test expectation fixes when the documentation review exposes stale or - incorrect doc/example/path expectations - -Out of scope: - -- product/runtime code changes -- feature implementation -- edits under `docs/policy/` -- adding roadmap behavior to current-behavior docs before that behavior is - implemented - -## Implementation Stages - -### Stage 1: Inventory and Source-of-Truth Audit - -Status: Completed (2026-05-23) - -Create a file-by-file inventory of all non-policy docs before rewriting. - -Implementation requirements: - -- List every non-policy documentation file and assign its intended audience. -- Identify each document's canonical scope using `docs/policy/documentation.md`. -- Compare docs against the current code and tests, especially: - - `internal/app` - - `internal/config` - - `internal/stage` - - `internal/artifacts` - - `examples` - - relevant tests under `internal/**` -- Record stale terminology, broken links, stale example paths, duplicate - content, and roadmap-only behavior that leaked into current-behavior docs. -- Record stale test expectations related to docs, examples, paths, or command - text. -- Do not rewrite content in this stage except obvious broken links or test - corrections needed to make documentation validation meaningful. - -Acceptance criteria: - -- The rewrite has a concrete file inventory and source-of-truth map. -- The team knows which docs are canonical and which should link elsewhere. -- Known stale terms and broken references are identified before broad edits. - -### Stage 2: User and Operator Docs - -Status: Completed (2026-05-23) - -Rewrite the user-facing and operator-facing docs first. - -Implementation requirements: - -- Rewrite these docs as fresh, concise current-behavior references: - - `README.md`, if present - - `docs/cli.md` - - `docs/config.md` - - `docs/operations.md` - - `docs/troubleshooting.md` -- Verify every command, flag, config field, discovery rule, path, and workflow - against implemented behavior. -- Cover implemented 1.0 behavior, including: - - campaign registry selection; - - concrete session loading and template-driven `session init`; - - session-oriented helper commands; - - clean, restore, analyze, and publish workflows; - - artifact selection behavior; - - locks and published output behavior; - - workspace, spool, and cache behavior; - - secrets loading and S3-backed operation. -- Keep examples short and link to maintained files under `examples/` instead - of duplicating large config blocks. -- Fix tests only when they assert stale doc paths, example paths, command - names, or current-behavior text. - -Acceptance criteria: - -- User/operator docs are task-oriented and match actual CLI/config behavior. -- Current-behavior docs do not depend on roadmaps for normal usage. -- No current-behavior doc describes unimplemented roadmap items. - -### Stage 3: Internal Developer Docs - -Status: Completed (2026-05-23) - -Rewrite implemented internal component docs after public docs stabilize. - -Implementation requirements: - -- Rewrite: - - `docs/internal/README.md` - - `docs/internal/adapters.md` - - `docs/internal/artifacts.md` - - `docs/internal/command-restore.md` - - `docs/internal/manifest.md` - - `docs/internal/stage-*.md` - - `docs/internal/storage.md` - - `docs/internal/workspace.md` -- Verify stage docs against current stage names, stage ordering, manifest - records, declared inputs/outputs, adapters, path helpers, storage behavior, - publish/current-state behavior, restore behavior, cache behavior, and - workspace cleanup. -- Keep implementation details in `docs/internal/`, not in user-facing docs. -- Avoid turning internal docs into duplicate config or CLI references; link to - canonical docs when needed. - -Acceptance criteria: - -- Internal docs are accurate enough for developers and LLM coding agents to - change the system safely. -- Stage and adapter boundaries match `docs/policy/architecture.md`. -- Manifest, path, storage, and publish invariants are explicit and current. - -### Stage 4: Integrations and Examples - -Status: Completed (2026-05-23) - -Review integration docs and maintained examples after core docs are rewritten. - -Implementation requirements: - -- Rewrite: - - `docs/integrations/README.md` - - `docs/integrations/audita.md` - - `docs/integrations/scriptorium.md` - - `docs/integrations/seriatim.md` -- Verify integration docs against current adapter contracts and expected - downstream tool behavior. -- Confirm every referenced example file exists. -- Confirm examples match current schema and command usage. -- Run or rely on example validation tests. -- Fix tests when they reference moved, renamed, or intentionally retired - examples. - -Acceptance criteria: - -- Integration docs describe only implemented adapter expectations. -- Maintained examples are valid, secret-free, and linked from canonical docs. -- Example validation tests reflect the documented example set. - -### Stage 5: Roadmap Cleanup and Final Sweep - -Status: Completed (2026-05-23) - -Clean up roadmap state and run final documentation validation. - -Implementation requirements: - -- Review `docs/roadmap/**` for implemented items that should be marked - implemented, retired, or left planned. -- Keep historical and planned behavior in roadmaps only. -- Run final link/path/term sweeps. -- Run validation commands: - - `go test ./internal/config -run TestExamplesLoadAndValidate -v` - - `go test ./internal/app -run TestExecute -v` - - `go test ./...` - -Acceptance criteria: - -- All non-policy docs are current for the 1.0 release. -- Roadmaps do not serve as required user/operator documentation. -- Tests pass after allowed documentation-related test expectation fixes. - -## Required Checks - -Run searches for stale terminology and references during the pass. - -Stale terminology: - -- `archive` -- `promote` -- `promoted` -- `promote_artifacts` -- legacy campaign path/discovery language -- old transcript names and paths -- removed CLI commands or aliases - -Broken or stale references: - -- missing local doc links; -- stale `examples/` paths; -- stale internal doc filenames; -- references to `docs/policy/**` as editable targets; -- command examples that no longer match the CLI. - -Policy checks: - -- Current-behavior docs mention only implemented behavior. -- Planned behavior appears only under `docs/roadmap/`. -- Docs do not expose raw secrets or recommend storing secrets in config. -- Docs use canonical homes: - - `docs/config.md` for config schema; - - `docs/cli.md` for command syntax; - - `docs/operations.md` for operator workflows; - - `docs/troubleshooting.md` for failure diagnosis; - - `docs/internal/` for implementation contracts; - - `docs/integrations/` for downstream tool integration notes; - - `docs/roadmap/` for future work. - -## Assumptions - -- `docs/policy/**` is read-only for this documentation pass. -- This pass is for 1.0 release readiness, not feature implementation. -- Product and runtime code changes are out of scope. -- Test fixes are in scope when they correct stale documentation, example, path, - command, or current-behavior expectations uncovered during the review. -- Roadmap files may remain as planning and historical records. -- Current-behavior docs must be sufficient for normal use without requiring - readers to consult roadmaps. diff --git a/internal/adapters/storage/archive.go b/internal/adapters/storage/archive.go deleted file mode 100644 index bf083b5..0000000 --- a/internal/adapters/storage/archive.go +++ /dev/null @@ -1,31 +0,0 @@ -// Package storage declares archive/storage backend adapter boundaries. -package storage - -import "context" - -// TODO: implement remote storage/archive backends (S3/SFTP/etc.). - -// Backend is the adapter boundary for archive/storage operations. -type Backend interface { - Archive(ctx context.Context, req ArchiveRequest) (ArchiveResult, error) -} - -// ArchiveItem describes one item to archive. -type ArchiveItem struct { - Kind string - LocalPath string - RemoteKey string -} - -// ArchiveRequest describes one archive operation. -type ArchiveRequest struct { - SessionID string - ManifestPath string - Items []ArchiveItem -} - -// ArchiveResult describes archive operation output. -type ArchiveResult struct { - Archived []ArchiveItem - Metadata map[string]any -} diff --git a/internal/adapters/storage/fake.go b/internal/adapters/storage/fake.go index 882dd2e..26b0e48 100644 --- a/internal/adapters/storage/fake.go +++ b/internal/adapters/storage/fake.go @@ -10,23 +10,8 @@ import ( "time" ) -// NoopBackend is a deterministic no-op archive/storage adapter. -type NoopBackend struct{} - -// Archive returns the requested items as archived with placeholder metadata. -func (n *NoopBackend) Archive(ctx context.Context, req ArchiveRequest) (ArchiveResult, error) { - if err := ctx.Err(); err != nil { - return ArchiveResult{}, err - } - return ArchiveResult{Archived: append([]ArchiveItem(nil), req.Items...), Metadata: map[string]any{"placeholder": true}}, nil -} - -// FakeBackend captures archive requests and returns deterministic responses. +// FakeBackend provides a deterministic in-memory object store for tests. type FakeBackend struct { - Requests []ArchiveRequest - Err error - Result ArchiveResult - Objects map[string]FakeObject Uploads []FakeUploadCall Downloads []FakeDownloadCall @@ -50,25 +35,6 @@ type FakeDownloadCall struct { LocalPath string } -// Archive records request and returns configured response. -func (f *FakeBackend) Archive(ctx context.Context, req ArchiveRequest) (ArchiveResult, error) { - if err := ctx.Err(); err != nil { - return ArchiveResult{}, err - } - f.Requests = append(f.Requests, req) - if f.Err != nil { - return ArchiveResult{}, f.Err - } - res := f.Result - if res.Archived == nil { - res.Archived = append([]ArchiveItem(nil), req.Items...) - } - if res.Metadata == nil { - res.Metadata = map[string]any{"fake": true} - } - return res, nil -} - // FakeObject is a deterministic fake object-store record. type FakeObject struct { Key string diff --git a/internal/adapters/storage/fake_test.go b/internal/adapters/storage/fake_test.go index e60b3c8..2222098 100644 --- a/internal/adapters/storage/fake_test.go +++ b/internal/adapters/storage/fake_test.go @@ -9,30 +9,6 @@ import ( "testing" ) -func TestFakeBackendCapturesRequestAndReturnsItems(t *testing.T) { - fake := &FakeBackend{} - req := ArchiveRequest{SessionID: "s1", Items: []ArchiveItem{{Kind: "artifact", LocalPath: "artifacts/log.md"}}} - - res, err := fake.Archive(context.Background(), req) - if err != nil { - t.Fatalf("Archive() error = %v", err) - } - if len(fake.Requests) != 1 || fake.Requests[0].SessionID != "s1" { - t.Fatalf("requests = %#v, want captured request", fake.Requests) - } - if len(res.Archived) != 1 { - t.Fatalf("archived len = %d, want 1", len(res.Archived)) - } -} - -func TestFakeBackendError(t *testing.T) { - fake := &FakeBackend{Err: errors.New("boom")} - _, err := fake.Archive(context.Background(), ArchiveRequest{}) - if err == nil { - t.Fatal("expected error, got nil") - } -} - func TestFakeBackendListPrefixFiltering(t *testing.T) { fake := &FakeBackend{} fake.SeedObject(FakeObject{Key: "dnd/campaigns/forsaken/audio/a.flac", Data: []byte("a")}) diff --git a/internal/adapters/storage/object_store.go b/internal/adapters/storage/object_store.go index 7ecd977..13ee97a 100644 --- a/internal/adapters/storage/object_store.go +++ b/internal/adapters/storage/object_store.go @@ -5,7 +5,7 @@ import ( "time" ) -// ObjectStore is a remote object storage boundary used by future prepare/archive work. +// ObjectStore is a remote object storage boundary used by prepare, restore, and publish work. // // Key invariant: // callers pass full bucket-relative object keys. Backend implementations do not diff --git a/internal/adapters/whisperx/http_test.go b/internal/adapters/whisperx/http_test.go index a7a7e95..e03b194 100644 --- a/internal/adapters/whisperx/http_test.go +++ b/internal/adapters/whisperx/http_test.go @@ -145,7 +145,7 @@ func TestHTTPClientDoesNotRetryOnNonRetryableStatus(t *testing.T) { } } -func TestHTTPClientInvalidJSONFailsAndDoesNotPromote(t *testing.T) { +func TestHTTPClientInvalidJSONFailsAndDoesNotInstallOutput(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte(`not-json`)) })) diff --git a/internal/app/config_loader.go b/internal/app/config_loader.go index f172dfc..b187b8d 100644 --- a/internal/app/config_loader.go +++ b/internal/app/config_loader.go @@ -4,7 +4,6 @@ import ( "context" "fmt" "os" - "path/filepath" "strings" "gitea.maximumdirect.net/eric/narratio/internal/adapters/storage" @@ -59,7 +58,7 @@ func loadCommandConfig(ctx context.Context, pipelineFlag, campaignFlag, campaign if err != nil { return nil, missingSessionConfigError(discoveredSession.Searched, err.Error()) } - sessionTempPath, err := downloadRemoteSessionConfig(ctx, store, remoteKey) + sessionTempPath, err := storage.DownloadObjectToTemp(ctx, store, remoteKey, "narratio-session-*.yml") if err != nil { return nil, missingSessionConfigError(discoveredSession.Searched, fmt.Sprintf("remote session %q download failed: %v", remoteKey, err)) } @@ -134,21 +133,6 @@ func findRemoteSessionConfig(ctx context.Context, store storage.ObjectStore, ses return storage.ObjectInfo{}, fmt.Errorf("remote session %q not found", remoteKey) } -func downloadRemoteSessionConfig(ctx context.Context, store storage.ObjectStore, remoteKey string) (string, error) { - f, err := os.CreateTemp("", "narratio-session-*.yml") - if err != nil { - return "", fmt.Errorf("create temp file: %w", err) - } - path := f.Name() - if err := f.Close(); err != nil { - return "", fmt.Errorf("close temp file %q: %w", path, err) - } - if err := store.Download(ctx, remoteKey, path); err != nil { - return "", err - } - return filepath.Clean(path), nil -} - func s3BucketName(cfg *config.PipelineConfig) string { if cfg == nil || cfg.Storage.S3 == nil { return "" diff --git a/internal/app/restore.go b/internal/app/restore.go index cd01699..57091fa 100644 --- a/internal/app/restore.go +++ b/internal/app/restore.go @@ -34,7 +34,7 @@ func Restore(ctx context.Context, args []string, out io.Writer) error { addCommonConfigFlags(fs, &flags) fs.BoolVar(&dryRun, "dry-run", false, "plan restore actions without writing local files") fs.BoolVar(&force, "force", false, "overwrite local conflicts with remote state") - fs.BoolVar(&includeAudio, "include-audio", false, "include archived session-level audio objects") + fs.BoolVar(&includeAudio, "include-audio", false, "include remote session-level audio objects") fs.Usage = func() { _, _ = fmt.Fprintln(out, "Usage: narratio session restore [--config ] [--campaign ] [--campaign-file ] [--session ] [--previous-session-id ] [--dry-run] [--force] [--include-audio]") _, _ = fmt.Fprintln(out) diff --git a/internal/app/restore_execution_test.go b/internal/app/restore_execution_test.go index e75fa56..1cb3e6e 100644 --- a/internal/app/restore_execution_test.go +++ b/internal/app/restore_execution_test.go @@ -39,7 +39,7 @@ func TestExecuteRestoreNonDryRunRestoresDurableFiles(t *testing.T) { if stderr.Len() != 0 { t.Fatalf("stderr = %q, want empty", stderr.String()) } - if !strings.Contains(stdout.String(), "Restored session archive for sample-campaign/2026-05-03") { + if !strings.Contains(stdout.String(), "Restored session state for sample-campaign/2026-05-03") { t.Fatalf("stdout = %q, want completion summary", stdout.String()) } diff --git a/internal/app/restore_plan_test.go b/internal/app/restore_plan_test.go index 26243c5..6ed0c0b 100644 --- a/internal/app/restore_plan_test.go +++ b/internal/app/restore_plan_test.go @@ -22,7 +22,7 @@ func TestRestorePlanDefaultScope(t *testing.T) { seedRestoreObject(store, current.SessionPrefix+"artifacts/session_recap.md", []byte("# recap\n")) seedRestoreObject(store, current.SessionPrefix+"audio/alice.flac", []byte("audio")) seedRestoreObject(store, current.SessionPrefix+"runs/20260519T010203Z-a1b2/manifest.json", []byte("{}")) - seedRestoreObject(store, current.SessionPrefix+"logs/archive.log", []byte("log")) + seedRestoreObject(store, current.SessionPrefix+"logs/publish.log", []byte("log")) plan, err := buildRestorePlan(context.Background(), cfg, current, store, RestorePlanOptions{}) if err != nil { diff --git a/internal/app/restore_report.go b/internal/app/restore_report.go index 590bcdf..20e8ad8 100644 --- a/internal/app/restore_report.go +++ b/internal/app/restore_report.go @@ -201,7 +201,7 @@ func writeRestoreSuccessSummary(out io.Writer, report *RestoreReport) error { if report == nil { return fmt.Errorf("restore report is required") } - if _, err := fmt.Fprintf(out, "Restored session archive for %s/%s\n", report.Campaign, report.SessionID); err != nil { + if _, err := fmt.Fprintf(out, "Restored session state for %s/%s\n", report.Campaign, report.SessionID); err != nil { return err } if _, err := fmt.Fprintf(out, "Remote run: %s\n", report.RunID); err != nil { diff --git a/internal/app/restore_test.go b/internal/app/restore_test.go index c9d3ed4..c67e1ab 100644 --- a/internal/app/restore_test.go +++ b/internal/app/restore_test.go @@ -369,7 +369,7 @@ func TestExecuteRestoreNonDryRunForceExecutesPlan(t *testing.T) { if code != 0 { t.Fatalf("exit code = %d, want 0; stderr=%q", code, stderr.String()) } - if !strings.Contains(stdout.String(), "Restored session archive for sample-campaign/2026-05-03") { + if !strings.Contains(stdout.String(), "Restored session state for sample-campaign/2026-05-03") { t.Fatalf("stdout = %q, want completion summary", stdout.String()) } if stderr.Len() != 0 { diff --git a/internal/app/runner.go b/internal/app/runner.go index 6a7d3ab..6add9fe 100644 --- a/internal/app/runner.go +++ b/internal/app/runner.go @@ -11,7 +11,6 @@ import ( "gitea.maximumdirect.net/eric/narratio/internal/adapters/notify" "gitea.maximumdirect.net/eric/narratio/internal/adapters/scriptorium" "gitea.maximumdirect.net/eric/narratio/internal/adapters/seriatim" - "gitea.maximumdirect.net/eric/narratio/internal/adapters/storage" "gitea.maximumdirect.net/eric/narratio/internal/adapters/whisperx" "gitea.maximumdirect.net/eric/narratio/internal/artifacts" "gitea.maximumdirect.net/eric/narratio/internal/config" @@ -83,9 +82,6 @@ func executeStages(ctx context.Context, cfg *config.Config, stages []stage.Stage if env.Scriptorium == nil { env.Scriptorium = scriptorium.NewSubprocessRunner() } - if env.Storage == nil { - env.Storage = &storage.NoopBackend{} - } if env.ObjectStore == nil && needsObjectStoreForRun(env.Config, stages) { objectStore, err := newCommandObjectStore(ctx, env.Config, nil) if err != nil { diff --git a/internal/previouscache/previouscache_test.go b/internal/previouscache/previouscache_test.go index fd504e1..16e82a3 100644 --- a/internal/previouscache/previouscache_test.go +++ b/internal/previouscache/previouscache_test.go @@ -14,7 +14,7 @@ import ( "gitea.maximumdirect.net/eric/narratio/internal/manifest" ) -func TestBuildPlanResolvesPromotedArtifactFromPreviousManifest(t *testing.T) { +func TestBuildPlanResolvesPublishedArtifactFromPreviousManifest(t *testing.T) { cfg, paths := previousCacheTestConfig(t) store := &storage.FakeBackend{} seedPreviousCurrent(t, store, cfg, previousManifestWithOutput(t, cfg, "artifacts/session_recap.md", []string{"artifacts/session_recap.md"})) diff --git a/internal/stage/analyze_test.go b/internal/stage/analyze_test.go index ceb37a7..00f0235 100644 --- a/internal/stage/analyze_test.go +++ b/internal/stage/analyze_test.go @@ -255,7 +255,7 @@ func TestAnalyzeOmitsOptionalCanonicalPreviousRecapWhenUnavailable(t *testing.T) } } -func TestAnalyzeUsesRunLocalPathsAndPromotesCanonical(t *testing.T) { +func TestAnalyzeUsesRunLocalPathsAndMaterializesCanonical(t *testing.T) { env, m, fake := setupAnalyzeEnv(t) env.Config.Session.Campaign = "sample-campaign" m.Campaign = "sample-campaign" diff --git a/internal/stage/merge.go b/internal/stage/merge.go index dcba85a..ecb5044 100644 --- a/internal/stage/merge.go +++ b/internal/stage/merge.go @@ -156,7 +156,7 @@ func (mergeStage) Run(ctx context.Context, env *Env, m *manifest.Manifest) (*Sta SessionID: sessionID, }) if err != nil { - return nil, fmt.Errorf("merge: promote merged transcript: %w", err) + return nil, fmt.Errorf("merge: materialize canonical base transcript: %w", err) } outputs := []artifacts.Ref{materializedMerged} if reportEnabled { @@ -166,7 +166,7 @@ func (mergeStage) Run(ctx context.Context, env *Env, m *manifest.Manifest) (*Sta SessionID: sessionID, }) if err != nil { - return nil, fmt.Errorf("merge: promote report: %w", err) + return nil, fmt.Errorf("merge: materialize canonical report: %w", err) } outputs = append(outputs, materializedReport) } diff --git a/internal/stage/merge_test.go b/internal/stage/merge_test.go index 7e735e6..e19e1f5 100644 --- a/internal/stage/merge_test.go +++ b/internal/stage/merge_test.go @@ -289,7 +289,7 @@ func TestMergeStageFailsWhenNormalizedOutputInvalid(t *testing.T) { } } -func TestMergeStageUsesRunLocalPathsAndPromotesCanonical(t *testing.T) { +func TestMergeStageUsesRunLocalPathsAndMaterializesCanonical(t *testing.T) { env, m := setupMergeEnv(t) env.Config.Session.Campaign = "sample-campaign" m.Campaign = "sample-campaign" diff --git a/internal/stage/normalize.go b/internal/stage/normalize.go index 13f44d9..b29535b 100644 --- a/internal/stage/normalize.go +++ b/internal/stage/normalize.go @@ -139,7 +139,7 @@ func (normalizeStage) Run(ctx context.Context, env *Env, m *manifest.Manifest) ( SessionID: sessionID, }) if err != nil { - return nil, fmt.Errorf("normalize: promote normalized transcript: %w", err) + return nil, fmt.Errorf("normalize: materialize canonical final transcript: %w", err) } outputs := []artifacts.Ref{materializedNormalized} if reportEnabled { @@ -149,7 +149,7 @@ func (normalizeStage) Run(ctx context.Context, env *Env, m *manifest.Manifest) ( SessionID: sessionID, }) if err != nil { - return nil, fmt.Errorf("normalize: promote report: %w", err) + return nil, fmt.Errorf("normalize: materialize canonical report: %w", err) } outputs = append(outputs, materializedReport) } diff --git a/internal/stage/normalize_test.go b/internal/stage/normalize_test.go index 53323f0..7f50f29 100644 --- a/internal/stage/normalize_test.go +++ b/internal/stage/normalize_test.go @@ -211,7 +211,7 @@ func TestNormalizeStageReportEnabledFailsWhenReportMissing(t *testing.T) { } } -func TestNormalizeStageUsesRunLocalPathsAndPromotesCanonical(t *testing.T) { +func TestNormalizeStageUsesRunLocalPathsAndMaterializesCanonical(t *testing.T) { env, m, ser := setupNormalizeEnv(t) env.Config.Session.Campaign = "sample-campaign" m.Campaign = "sample-campaign" diff --git a/internal/stage/placeholders_test.go b/internal/stage/placeholders_test.go index ac7201d..db4938f 100644 --- a/internal/stage/placeholders_test.go +++ b/internal/stage/placeholders_test.go @@ -96,7 +96,6 @@ func TestStagesReturnExpectedMetadata(t *testing.T) { Seriatim: sf, Audita: af, Scriptorium: sc, - Storage: st, ObjectStore: st, Notifier: nf, } @@ -221,9 +220,6 @@ func TestStagesReturnExpectedMetadata(t *testing.T) { if len(sc.RunRequests) != 0 { t.Fatalf("scriptorium run calls = %d, want 0 when scriptorium config is absent", len(sc.RunRequests)) } - if len(st.Requests) != 0 { - t.Fatalf("storage publish calls = %d, want 0", len(st.Requests)) - } if _, ok := st.Objects["dnd/campaigns/sample-campaign/sessions/2026-05-03/runs/"+m.RunID+"/manifest.json"]; !ok { t.Fatalf("publish upload missing manifest key in fake object store") } diff --git a/internal/stage/polish.go b/internal/stage/polish.go index 0490379..b7c42f3 100644 --- a/internal/stage/polish.go +++ b/internal/stage/polish.go @@ -155,7 +155,7 @@ func (polishStage) Run(ctx context.Context, env *Env, m *manifest.Manifest) (*St SessionID: sessionID, }) if err != nil { - return nil, fmt.Errorf("polish: promote processed transcript: %w", err) + return nil, fmt.Errorf("polish: materialize canonical polished transcript: %w", err) } outputs := []artifacts.Ref{materializedProcessed} if reportEnabled { @@ -165,7 +165,7 @@ func (polishStage) Run(ctx context.Context, env *Env, m *manifest.Manifest) (*St SessionID: sessionID, }) if err != nil { - return nil, fmt.Errorf("polish: promote report: %w", err) + return nil, fmt.Errorf("polish: materialize canonical report: %w", err) } outputs = append(outputs, materializedReport) } diff --git a/internal/stage/polish_test.go b/internal/stage/polish_test.go index b110fa1..a447b2f 100644 --- a/internal/stage/polish_test.go +++ b/internal/stage/polish_test.go @@ -238,7 +238,7 @@ func TestPolishStageFailsWhenReportInvalid(t *testing.T) { } } -func TestPolishStageUsesRunLocalPathsAndPromotesCanonical(t *testing.T) { +func TestPolishStageUsesRunLocalPathsAndMaterializesCanonical(t *testing.T) { env, m := setupPolishEnv(t) env.Config.Session.Campaign = "sample-campaign" m.Campaign = "sample-campaign" diff --git a/internal/stage/stage.go b/internal/stage/stage.go index 120e258..59d251f 100644 --- a/internal/stage/stage.go +++ b/internal/stage/stage.go @@ -27,7 +27,6 @@ type Env struct { Seriatim seriatim.Runner Audita audita.Runner Scriptorium scriptorium.Runner - Storage storage.Backend ObjectStore storage.ObjectStore Notifier notify.Sender } diff --git a/internal/stage/transcribe_test.go b/internal/stage/transcribe_test.go index f4b07a0..a43b6c4 100644 --- a/internal/stage/transcribe_test.go +++ b/internal/stage/transcribe_test.go @@ -191,7 +191,7 @@ func TestTranscribeStageInvalidJSONFails(t *testing.T) { } } -func TestTranscribeStageUsesRunLocalOutputAndPromotesCanonical(t *testing.T) { +func TestTranscribeStageUsesRunLocalOutputAndMaterializesCanonical(t *testing.T) { env, m := setupTranscribeEnv(t, []string{"alice.flac"}) env.Config.Session.Campaign = "sample-campaign" m.Campaign = "sample-campaign" diff --git a/internal/stage/trim.go b/internal/stage/trim.go index 8d759f2..f8f2001 100644 --- a/internal/stage/trim.go +++ b/internal/stage/trim.go @@ -106,7 +106,7 @@ func (trimStage) Run(ctx context.Context, env *Env, m *manifest.Manifest) (*Stag SessionID: sessionID, }) if err != nil { - return nil, fmt.Errorf("trim: promote trimmed transcript: %w", err) + return nil, fmt.Errorf("trim: materialize canonical final trimmed transcript: %w", err) } metadata["trim_action"] = "copy_disabled" return &StageResult{ @@ -357,7 +357,7 @@ func (trimStage) Run(ctx context.Context, env *Env, m *manifest.Manifest) (*Stag SessionID: sessionID, }) if err != nil { - return nil, fmt.Errorf("trim: promote trimmed transcript: %w", err) + return nil, fmt.Errorf("trim: materialize canonical final trimmed transcript: %w", err) } materializedBounds, err := materializeRunLocalOutput(env.ArtifactStore, finalBoundsOutputPath, canonicalBoundsOutputPath, artifacts.Ref{ Kind: "session_bounds", @@ -365,7 +365,7 @@ func (trimStage) Run(ctx context.Context, env *Env, m *manifest.Manifest) (*Stag SessionID: sessionID, }) if err != nil { - return nil, fmt.Errorf("trim: promote session bounds: %w", err) + return nil, fmt.Errorf("trim: materialize canonical session bounds: %w", err) } return &StageResult{ diff --git a/internal/stage/trim_test.go b/internal/stage/trim_test.go index 6025b93..f42d844 100644 --- a/internal/stage/trim_test.go +++ b/internal/stage/trim_test.go @@ -298,7 +298,7 @@ func TestTrimStageDisabledCopiesNormalizedTranscript(t *testing.T) { } } -func TestTrimStageUsesRunLocalPathsAndPromotesCanonical(t *testing.T) { +func TestTrimStageUsesRunLocalPathsAndMaterializesCanonical(t *testing.T) { env, m, scr, ser := setupTrimEnv(t) env.Config.Session.Campaign = "sample-campaign" m.Campaign = "sample-campaign" @@ -322,7 +322,7 @@ func TestTrimStageUsesRunLocalPathsAndPromotesCanonical(t *testing.T) { t.Fatalf("trim output path = %q, want run-local path", ser.TrimRequests[0].OutputTrimmedPath) } if len(result.Outputs) < 2 { - t.Fatalf("outputs = %#v, want promoted trimmed+bounds outputs", result.Outputs) + t.Fatalf("outputs = %#v, want materialized trimmed+bounds outputs", result.Outputs) } for _, out := range result.Outputs { if strings.Contains(out.AbsolutePath, string(filepath.Separator)+"runs"+string(filepath.Separator)) {