Remove completed roadmap and audit documents
This commit is contained in:
@@ -1,148 +0,0 @@
|
||||
# Audit Decision Register
|
||||
|
||||
## Status
|
||||
|
||||
Complete. This document inventories the conclusions of Narratio's three
|
||||
maintainability audit passes and records their final disposition. It is a
|
||||
traceability register, not an implementation plan and not an additional audit.
|
||||
|
||||
The source audits are:
|
||||
|
||||
- the contract-preserving [Maintainability Audit](audit.md);
|
||||
- the contract-relaxing [Contract Simplification Audit](contract-audit.md); and
|
||||
- the [Reusable Library Extraction Audit](library-audit.md).
|
||||
|
||||
The detailed [File Operations Simplification](fileops.md) roadmap supplies the
|
||||
approved filesystem rationale and policy that refine the audit findings. The
|
||||
consolidated [Refactoring Roadmap](refactoring.md) is the controlling inventory
|
||||
of adopted work for the upcoming refactor.
|
||||
|
||||
## Dispositions
|
||||
|
||||
- **Immediate action:** approved work included in `refactoring.md`.
|
||||
- **Constraint:** a rule governing immediate work, but not a separate code
|
||||
change by itself.
|
||||
- **Superseded:** a later approved decision replaces the earlier recommendation.
|
||||
- **Conditional:** reconsider only when a stated future trigger occurs.
|
||||
- **Deferred:** deliberately outside the upcoming refactor.
|
||||
- **No action:** already satisfied, unsubstantiated, retained as-is, or merely
|
||||
an observation.
|
||||
|
||||
An immediate action can also carry a constraint. When findings overlap, the
|
||||
contract audit controls product semantics, `fileops.md` resolves the detailed
|
||||
filesystem policy, and the library audit controls extraction decisions. The
|
||||
resulting immediate work is consolidated in `refactoring.md`.
|
||||
|
||||
## Pass 1 — Contract-Preserving Maintainability Audit
|
||||
|
||||
All identifiers in this table refer to sections of [`audit.md`](audit.md).
|
||||
|
||||
| ID | Subject | Final disposition | Controlling decision and destination |
|
||||
| --- | --- | --- | --- |
|
||||
| H01 | Repository footprint | No action | Size alone was not a defect. |
|
||||
| H02 | Trusted-environment threat model | Immediate action and constraint | Adopt the trusted, group-owned workspace model in `fileops.md`; retain secret handling and confined destructive scope. See `refactoring.md`, Filesystem. |
|
||||
| H03 | Production-code concentration | No action | Do not split packages merely because they are large. |
|
||||
| H04 | Production files over 1,000 lines | Immediate action | Decompose only the identified runner, analyze, configuration, publish, and previous-cache seams. See the corresponding `refactoring.md` workstreams. |
|
||||
| H05 | Application, stage, and artifact interaction | Constraint | Preserve current package ownership and dependency direction. |
|
||||
| H06 | Confined filesystem and directory promotion | Immediate action | Replace the broad subsystem under `fileops.md`; delete custom directory promotion. See Filesystem. |
|
||||
| H07 | Test code larger than production | Immediate action | Remove tests only with retired contracts, consolidate focused fixtures, and replace mutable app globals as affected code is refactored. See Quality and testability. |
|
||||
| H08 | Repository tests and vet | Constraint | Keep repository-wide tests and vet as required refactoring validation. |
|
||||
| H09 | Acyclic package dependencies | Constraint | Preserve the existing acyclic dependency direction; no reorganization is required. |
|
||||
| H10 | Localized complexity | Constraint | Limit work to audited hotspots and immediate collaborators. |
|
||||
| H11 | Small dependency surface | Constraint | Add dependencies only where an adopted finding demonstrates net value. |
|
||||
| H12 | Documentation ownership | Immediate action and constraint | Update canonical owners with each behavior change and avoid parallel narratives. See Quality and documentation. |
|
||||
| H13 | Coverage by volume | Immediate action | Add risk-focused restore and transition coverage; do not preserve or target a test-line or coverage ratio. |
|
||||
| H14 | Cohesion of app, stage, and config | Immediate action and constraint | Keep packages intact while introducing private lifecycle, transaction, and validation-domain seams. |
|
||||
| H15 | Natural modules in large files | Immediate action | Use the audited internal seams for runner, analyze, configuration, publish, and current previous-cache logic. |
|
||||
| H16 | Duplication in stages and adapters | No action | Do not introduce base-stage or base-adapter abstractions; keep tool contracts local. |
|
||||
| H17 | Durable versus implementation-coupled tests | Immediate action | Preserve behavior tests, delete mechanism tests for retired contracts, use invocation-scoped dependencies, and improve large structural diffs selectively. |
|
||||
| H18 | Invariant discoverability | Immediate action | Correct filesystem documentation and restore behavior; retain transaction-order comments near runner and publish ownership. |
|
||||
| H19 | Go baseline upgrade | Immediate action, revised to Go 1.26 | The audit proposed Go 1.27, but the approved implementation baseline is Go 1.26: it provides `os.Process.WithHandle`, while `os.Root.RemoveAll` was already available in Go 1.25. Upgrade the module, CI/release images, module state, and release documentation in one bounded change. See Platform modernization. |
|
||||
| H20 | `os.Root` confinement | Immediate action | Use `os.Root` only for genuinely root-scoped cleanup and restore; use ordinary filesystem operations elsewhere. See Filesystem. |
|
||||
| H21 | Narratio-owned durability and policy | Superseded in part; immediate action | `fileops.md` retains atomic visibility, limits, modes, validated downloads, destructive scope, and secret protection while retiring stronger guarantees. |
|
||||
| H22 | `os.Process.WithHandle` | Immediate action | Adopt after the Go baseline upgrade while preserving Windows Job Object lifecycle. See Platform modernization. |
|
||||
| H23 | `slices` and `maps` | Immediate opportunistic action | Apply only in touched code where semantics are identical; no bulk rewrite. See Quality and testability. |
|
||||
| H24 | `errgroup` for transcription | Immediate action | Replace the manual worker pool while preserving cancellation, qualified errors, and deterministic aggregation. See Platform modernization. |
|
||||
| H25 | `go-cmp` in tests | Immediate opportunistic action | Use for selected large structural comparisons only. See Quality and testability. |
|
||||
| H26 | Adopt `gofrs/flock` | No action | Retain the small existing platform lock primitives. |
|
||||
| H27 | Adopt Cobra | Deferred | Reconsider only after demonstrated command/help/flag inheritance costs. |
|
||||
| H28 | Generic configuration framework | No action | Retain typed strict decoding, composition, defaults, and semantic validation. |
|
||||
| H29 | Generic atomic-file package | Immediate action with constraint | Keep a small local helper unless focused research finds a maintained dependency that removes more code while meeting platform and mode needs. See Filesystem. |
|
||||
| H30 | Provider-neutral object storage | No action | The narrow internal storage ports already satisfy the requirement. |
|
||||
| H31 | Retry or backoff package | No action | Retain the explicit WhisperX policy and provider-owned AWS retries. |
|
||||
| H32 | UUID or ULID package | No action | Retain the existing readable cryptographically random run ID. |
|
||||
| H33 | Generic subprocess framework | Superseded in part | The library audit retains the code internally now, requires local mechanism/policy separation, and makes external extraction conditional. See Platform modernization and L01. |
|
||||
| H34 | `executePlan` | Immediate action | Decompose around invocation preparation, collaborator composition, one-stage transition, and finalization while keeping lock and persistence ordering explicit. See Runner lifecycle. |
|
||||
| H35 | Shared planning/execution transition model | Immediate action | Introduce only the shared pure pre-execution decision and small identical transition helpers. See Runner lifecycle. |
|
||||
| H36 | `analyzeStage.Run` | Superseded in part; immediate action | Contract Finding C03 removes invocation projections. Use a session-only execution accumulator or equivalent private state under the checkpointing model. See Analyze recovery. |
|
||||
| H37 | `executeAnalyzeArtifact` | Immediate action | Split preparation, adapter invocation, validated materialization, and evidence construction under one transaction owner. See Analyze recovery. |
|
||||
| H38 | `config/validate.go` | Superseded in part; immediate action | Split by semantic domain, but C06 removes exact validation order and full prose as compatibility requirements. See Configuration. |
|
||||
| H39 | `validateScriptorium` | Immediate action | Separate declaration/output, dependency/input, and variable validation; retain deterministic semantics without promising diagnostic choreography. |
|
||||
| H40 | `validateNotarius` | Immediate action | Separate reference binding from output-lane normalization and collision checks. |
|
||||
| H41 | `validateAudita` | Immediate mechanical action | Move it with its adapter-validation domain but do not decompose the function. |
|
||||
| H42 | Composition-envelope splitting | Immediate action | Separate shape decoding, cross-field semantics, and root metadata removal while preserving useful source-aware conflict diagnostics. |
|
||||
| H43 | `previouscache.Resolve` | Superseded | C01 requires complete removal of legacy candidate probing rather than isolation. Simplify the remaining canonical path as part of the compatibility purge. |
|
||||
| H44 | `stage/publish.go` | Immediate action | Decompose internal concerns while retaining one publish transaction; C05 controls the new lock and carry-forward semantics. See Remote publication. |
|
||||
| H45 | `fileops/directory.go` | Immediate action | Delete directory promotion and replace it with validated same-workspace Notarius rename. See Filesystem. |
|
||||
| H46 | Restore classification | Immediate correctness action | Classify leaf and ancestor symlinks as non-regular conflicts inside a root-scoped restore design. See Filesystem. |
|
||||
| H47 | Manifest analyze-state validation | No action | Retain the cohesive state-model owner; only trivial clone cleanup is permitted when touched. |
|
||||
| H48 | Preliminary priority order | Constraint | Use as dependency guidance for implementation planning; it creates no separate deliverable. |
|
||||
|
||||
## Pass 2 — Contract Simplification Audit
|
||||
|
||||
All identifiers in this table refer to findings of
|
||||
[`contract-audit.md`](contract-audit.md). These approved decisions control any
|
||||
conflicting recommendation from the first pass.
|
||||
|
||||
| ID | Subject | Final disposition | Destination |
|
||||
| --- | --- | --- | --- |
|
||||
| C01 | Deprecated compatibility | Immediate action | Purge all named legacy configuration, party/player, remote-state, previous-cache, extraction, analyze, and manifest compatibility paths. See Canonical contracts. |
|
||||
| C02 | Structured run manifests | Immediate action | Eliminate the run-manifest schema and lifecycle; keep the session manifest as the sole local progress authority. See Manifest authority. |
|
||||
| C03 | Analyze recovery | Immediate action | Checkpoint each successful artifact in dependency order, retain valid successes across later failure, and resume failed, stale, or missing work. See Analyze recovery. |
|
||||
| C04 | Cleanup hidden inside publish | Immediate action | Add cleanup as the final idempotent pipeline stage and remove post-publish cleanup obligations. See Cleanup stage. |
|
||||
| C05 | Shared mutable remote locks | Immediate action | Replace the central/static lock model with per-source remote lock objects and carry prior locked declarations into new commits. See Remote publication. |
|
||||
| C06 | Configuration diagnostic contracts | Immediate action and constraint | Preserve strict deterministic semantics and useful source-aware errors while retiring exact validation order, wording, punctuation, formatting, and incidental provenance. See Configuration. |
|
||||
|
||||
The contract audit also records that the combined work remains eligible for a
|
||||
minor release because the compatibility surfaces were already deprecated and
|
||||
removable. Release execution is outside this refactoring roadmap.
|
||||
|
||||
## Pass 3 — Reusable Library Extraction Audit
|
||||
|
||||
Identifiers refer to [`library-audit.md`](library-audit.md). Candidate 5 is
|
||||
expanded into its separately evaluated helper groups, and the audit's explicit
|
||||
non-candidates are listed so the register is complete.
|
||||
|
||||
| ID | Candidate | Final disposition | Controlling decision and destination |
|
||||
| --- | --- | --- | --- |
|
||||
| L01 | Cross-platform subprocess supervision | Conditional extraction; immediate internal cleanup | Keep internal until a real second consumer, overlapping multi-platform contract, alternative review, clean API, and maintainer exist. Separate process mechanism from Narratio policy locally and remove unrelated file writers. See Platform modernization. |
|
||||
| L02 | Hardened FileOps | Immediate simplification; no extraction | Follow `fileops.md`; version control preserves reusable algorithms without making Narratio consume rejected guarantees. |
|
||||
| L03 | YAML composition and provenance | No extraction | Keep Narratio's configuration DSL internal. Dependency evaluation for low-level node mechanics is distinct and not required now. |
|
||||
| L04 | Immutable remote commit protocol | No extraction | Keep the domain protocol internal; evaluate established content-addressed specifications only if interoperable cross-application snapshots become a real requirement. |
|
||||
| L05a | Path-safety helpers | No extraction; opportunistic local action | Keep small and internal; use standard-library path primitives only where semantics match exactly. |
|
||||
| L05b | Bounded I/O and streaming redaction | No extraction | Retain adapter limits, secret policy, and errors in Narratio. |
|
||||
| L05c | Atomic file replacement | No extraction; immediate local action | Use the small local or established implementation selected under H29 and `fileops.md`. |
|
||||
| L05d | Object-store ports, file locks, IDs, and DAG helpers | No extraction | Keep the narrow ports and domain algorithms internal. |
|
||||
| L06 | Release automation | No action | Keep repository-local; reconsider a standalone tool or template only after materially identical scripts drift across repositories. |
|
||||
| L07 | Artifact source and destination policy | No extraction | Domain ownership is the source of its value. |
|
||||
| L08 | Manifest and durable state validation | No extraction | Keep state schemas and validation with Narratio. |
|
||||
| L09 | Stage ordering, selection, invalidation, resume, and force | No extraction | Keep pipeline lifecycle semantics with Narratio. |
|
||||
| L10 | Analyze checkpoint and dependency recovery | No extraction | Implement C03 as Narratio domain behavior. |
|
||||
| L11 | Cleanup and remote lock policy | No extraction | Implement C04 and C05 as Narratio operational policy. |
|
||||
| L12 | External tool and storage adapters | No extraction | Keep integration-specific requests, validation, and errors local. |
|
||||
| L13 | Configuration validation, party resolution, and artifact-family expansion | No extraction | Keep product configuration rules internal. |
|
||||
|
||||
## Completeness and Routing Summary
|
||||
|
||||
Every audited hypothesis or candidate is represented above. Only entries marked
|
||||
**Immediate action** or **Immediate opportunistic action** produce scope in
|
||||
[`refactoring.md`](refactoring.md). Constraints govern that work without
|
||||
creating independent implementation tasks. Superseded, conditional, deferred,
|
||||
and no-action entries remain here for traceability and are intentionally absent
|
||||
from the actionable roadmap except where a boundary must be stated to prevent
|
||||
scope creep.
|
||||
|
||||
The consolidation adds no hypothesis or feature that was not identified by an
|
||||
audit. A future implementation plan should derive its stages from
|
||||
`refactoring.md`, use this register to verify completeness, and return to the
|
||||
source audits only when it needs the supporting analysis behind a decision.
|
||||
@@ -1,764 +0,0 @@
|
||||
# Maintainability Audit
|
||||
|
||||
## Status
|
||||
|
||||
Complete. This audit evaluates the hypotheses in
|
||||
[`potential-audit.md`](potential-audit.md) in their original order. That source
|
||||
was committed unchanged as `5e4d201` and treated as immutable. The code under
|
||||
review is the `v1.6.0` tree at `545aa68`; the source-document commit adds no
|
||||
production or test code.
|
||||
|
||||
This document records findings and recommended changes. It is not an
|
||||
implementation plan and does not authorize behavior changes by itself.
|
||||
|
||||
## Method
|
||||
|
||||
The review combined repository metrics, package and call-graph inspection,
|
||||
focused source reading, change-history review, policy and internal-documentation
|
||||
comparison, and test-suite inspection. The named functions and files in the
|
||||
source document were read directly. Baseline `go test ./...` and `go vet ./...`
|
||||
passed with the maintained Go 1.25 configuration. The complete test suite and
|
||||
`go vet` also passed under Go 1.27.0, and the release asset builder successfully
|
||||
cross-compiled all six supported OS/architecture targets with Go 1.27.0.
|
||||
|
||||
Verdicts use these terms:
|
||||
|
||||
- **Confirmed:** the hypothesis identifies work worth scheduling.
|
||||
- **Focused opportunity:** improvement is justified, but only at a narrow seam.
|
||||
- **Already satisfied:** the desired end state is present.
|
||||
- **Retain:** the current application-owned implementation is justified.
|
||||
- **Not substantiated:** the evidence does not justify the proposed change.
|
||||
|
||||
## Executive Summary
|
||||
|
||||
Narratio does not have a broad maintainability problem. Its package direction,
|
||||
explicit contracts, documentation, tests, and small dependency surface are all
|
||||
healthy. Most size is a consequence of real orchestration, recovery, and
|
||||
integration requirements. The exception is filesystem code, where mechanisms
|
||||
originally chosen for adversarial races and power-loss durability have been
|
||||
treated as durable product requirements without sufficient operational need.
|
||||
|
||||
The audit did identify one correctness remediation and several proportional
|
||||
maintainability improvements:
|
||||
|
||||
1. Simplify `internal/fileops` around Narratio's actual trusted-workspace model.
|
||||
Retain root-scoped destructive and restore operations, atomic visibility for
|
||||
corruption-sensitive file replacement, bounded I/O, group-owned workspace
|
||||
modes, and stricter handling of API-key material. Drop power-loss durability,
|
||||
hostile-race defenses, universal symlink and regular-file restrictions, and
|
||||
atomic no-replace directory promotion as application contracts.
|
||||
2. Restore planning must still classify local targets safely within the session
|
||||
root. `classifyRestoreAction` currently uses `os.Stat` and path-based hashing,
|
||||
so a symlink to a regular file can be classified as `skip_same`; correct this
|
||||
within the simpler root-scoped restore design rather than adding more
|
||||
identity-checking machinery to the current abstraction.
|
||||
3. Decompose `executePlan` around its existing lifecycle boundaries, and share
|
||||
the pre-execution stage-decision model used by planning and execution. Do not
|
||||
introduce a generic workflow engine or change transition ordering during the
|
||||
extraction.
|
||||
4. Split the long per-artifact analyze transaction, configuration validation
|
||||
domains, publish source/transfer/commit concerns, and modern-versus-legacy
|
||||
previous-cache resolution at their existing internal seams. Keep the current
|
||||
packages and public contracts.
|
||||
5. Upgrade the maintained baseline to Go 1.27, use `os.Process.WithHandle` for
|
||||
Windows job assignment, and replace the transcription worker pool with
|
||||
`errgroup` bounded concurrency. Use `slices`, `maps`, and `go-cmp` only where
|
||||
they make simple mechanics or test failures clearer.
|
||||
6. Replace mutable package-global function seams in `internal/app` with a small
|
||||
invocation-scoped dependency set as affected command areas are refactored.
|
||||
This will reduce test reset boilerplate and permit safe parallel tests without
|
||||
creating a general dependency-injection framework.
|
||||
|
||||
The audit does **not** recommend package-wide reorganization, a CLI framework,
|
||||
an external flock library, generic configuration frameworks, generic subprocess
|
||||
machinery, or new identifier/retry/storage abstractions. Atomic replacement is
|
||||
a generic operation under the reduced contract, but a small local helper is
|
||||
preferable to a dependency that does not support Narratio's full platform set.
|
||||
|
||||
## Findings In Source Order
|
||||
|
||||
### H01 — Repository footprint
|
||||
|
||||
**Verdict:** Already satisfied; size alone is not a problem.
|
||||
|
||||
**Recommended change:** None based on aggregate line count.
|
||||
|
||||
**Reasoning:** The measured tree contains 36,107 production Go lines and 45,132
|
||||
test Go lines across 26 packages. The implementation owns substantially more
|
||||
than command wrapping, and the observed size maps to documented responsibilities.
|
||||
|
||||
### H02 — Trusted-environment threat model
|
||||
|
||||
**Verdict:** Confirmed as a refactoring constraint.
|
||||
|
||||
**Recommended change:** Preserve root-scoped destructive and restore operations
|
||||
and the checks needed to avoid accidental path escape. Remove tests and
|
||||
identity rechecks whose only purpose is to defeat a concurrent adversary, and
|
||||
stop applying universal no-follow and regular-file rules to ordinary reads,
|
||||
writes, and copies. Update the owning architecture, operations, and internal
|
||||
documents when the policy changes.
|
||||
|
||||
**Reasoning:** Narratio operates on trusted D&D campaign workspaces under a
|
||||
session lock; their contents are intended to be shared and are not sensitive.
|
||||
The application still needs safe destructive scope and special protection for
|
||||
API keys, but it does not need to prove inode identity against hostile races or
|
||||
reject ordinary workspace symlinks by default. `os.Root` remains useful at the
|
||||
few boundaries where containment is itself the requirement.
|
||||
|
||||
### H03 — Production-code concentration
|
||||
|
||||
**Verdict:** Not substantiated as a package-boundary problem.
|
||||
|
||||
**Recommended change:** Keep `internal/stage`, `internal/app`, and
|
||||
`internal/config` intact; use file- and function-level decomposition described
|
||||
below.
|
||||
|
||||
**Reasoning:** Their measured 8,340, 7,869, and 5,542 lines correspond to three
|
||||
coherent owners: pipeline stages, application lifecycle/commands, and strict
|
||||
configuration. Splitting by size would add cross-package contracts without
|
||||
improving dependency direction.
|
||||
|
||||
### H04 — Production files over 1,000 lines
|
||||
|
||||
**Verdict:** Confirmed selectively.
|
||||
|
||||
**Recommended change:** Split `runner.go`, `publish.go`, and `validate.go` along
|
||||
the seams identified below. Refactor `analyze.go` functions without forcing a
|
||||
purely mechanical file-count target.
|
||||
|
||||
**Reasoning:** Several large files contain natural modules, but other large
|
||||
files remain cohesive. File size is useful here as a navigation and change-blast
|
||||
signal, not as a universal limit.
|
||||
|
||||
### H05 — Application, stage, and artifact interaction
|
||||
|
||||
**Verdict:** Already satisfied at the architectural boundary.
|
||||
|
||||
**Recommended change:** Retain the current dependency direction. Reduce
|
||||
`executePlan` collaborator handling with a private execution context rather than
|
||||
moving stage or artifact policy across packages.
|
||||
|
||||
**Reasoning:** Production imports remain acyclic: the application composes
|
||||
stages and adapters; stages consume Narratio-owned configuration, manifest,
|
||||
artifact, and adapter contracts; artifact services do not choose stage policy.
|
||||
The high interaction count reflects the application boundary's intended role.
|
||||
|
||||
### H06 — Confined-filesystem and directory-promotion complexity
|
||||
|
||||
**Verdict:** Confirmed more broadly than the preliminary hypothesis.
|
||||
|
||||
**Recommended change:** Replace the general confined-filesystem and directory-
|
||||
promotion subsystem with ordinary standard-library operations plus a few
|
||||
focused helpers for the retained policies in H21. Delete both directory-copy
|
||||
algorithms, their race hooks and identity rechecks, and the platform-specific
|
||||
no-replace and directory-sync machinery. Let Notarius write a validated staging
|
||||
bundle in the session workspace and rename that directory directly to its
|
||||
unique durable destination.
|
||||
|
||||
**Reasoning:** The duplicate implementation is real, but removing only the
|
||||
test-only half would preserve a much larger abstraction whose guarantees are
|
||||
not operational requirements. The session lock, trusted workspace, same-
|
||||
workspace staging location, and unique run destination remove the practical
|
||||
need for adversarial no-replace promotion. A direct rename also avoids the
|
||||
second full-tree copy that the current implementation performs.
|
||||
|
||||
### H07 — Test code larger than production code
|
||||
|
||||
**Verdict:** Focused opportunity, not evidence of overtesting.
|
||||
|
||||
**Recommended change:** Preserve behavior and retained-invariant coverage.
|
||||
Delete tests for abandoned power-loss, hostile-race, universal no-follow, and
|
||||
directory-promotion guarantees along with the implementation they constrain.
|
||||
Add focused tests for the smaller atomic-file helper, bounded I/O, modes,
|
||||
root-scoped cleanup and restore, validated Notarius rename, and API-key handling.
|
||||
Also consolidate repeated fixture setup within individual packages and replace
|
||||
app-level mutable global function seams with invocation-scoped dependencies as
|
||||
related code is touched.
|
||||
|
||||
**Reasoning:** The suite makes extensive legitimate use of real temporary
|
||||
filesystems, fake object stores, and subprocess helpers. The clearest hygiene
|
||||
signals are a few very large test files, 11 mutable production function seams
|
||||
that are reset repeatedly by tests, and a filesystem suite that treats
|
||||
unneeded implementation guarantees as policy; the overall test-to-production
|
||||
ratio is not itself a defect.
|
||||
|
||||
### H08 — Repository-wide tests and vet
|
||||
|
||||
**Verdict:** Confirmed counter-signal.
|
||||
|
||||
**Recommended change:** Keep both checks as required validation for every
|
||||
refactoring stage.
|
||||
|
||||
**Reasoning:** `go test ./...` and `go vet ./...` pass on the maintained
|
||||
baseline and on Go 1.27.0. This argues for incremental, behavior-preserving
|
||||
changes rather than a broad rewrite.
|
||||
|
||||
### H09 — Acyclic package dependencies
|
||||
|
||||
**Verdict:** Confirmed counter-signal.
|
||||
|
||||
**Recommended change:** Preserve the existing direction and reject proposed
|
||||
extractions that create app/stage/artifact cycles.
|
||||
|
||||
**Reasoning:** `go list` resolves all 26 packages without a cycle, and focused
|
||||
import inspection matches the ownership policy.
|
||||
|
||||
### H10 — Localized rather than systemic complexity
|
||||
|
||||
**Verdict:** Confirmed counter-signal.
|
||||
|
||||
**Recommended change:** Refactor only the named hotspots and their immediate
|
||||
helpers.
|
||||
|
||||
**Reasoning:** Most functions are modest. High scores cluster in explicit
|
||||
orchestration, semantic validation, compatibility, and filesystem invariants.
|
||||
There is no evidence for repository-wide abstraction or style replacement.
|
||||
|
||||
### H11 — Small direct dependency set
|
||||
|
||||
**Verdict:** Confirmed counter-signal.
|
||||
|
||||
**Recommended change:** Add only `golang.org/x/sync/errgroup` and selectively
|
||||
`github.com/google/go-cmp/cmp` if the planned changes use them; keep other
|
||||
candidates out.
|
||||
|
||||
**Reasoning:** Direct dependencies are limited to the AWS SDK boundary,
|
||||
`x/sys`, and YAML. The small surface is consistent with the architecture policy
|
||||
and raises the bar for dependencies that merely replace a few lines.
|
||||
|
||||
### H12 — Structured documentation ownership
|
||||
|
||||
**Verdict:** Confirmed counter-signal.
|
||||
|
||||
**Recommended change:** Update the existing owner document when a refactor
|
||||
changes mechanics; do not add another overlapping architecture narrative.
|
||||
|
||||
**Reasoning:** Policy, operator, integration, and internal implementation
|
||||
documents have explicit ownership and reading paths. The relevant invariants
|
||||
were readily discoverable.
|
||||
|
||||
### H13 — Automated coverage by volume
|
||||
|
||||
**Verdict:** Confirmed, with a qualification.
|
||||
|
||||
**Recommended change:** Judge future test changes by behavior and risk rather
|
||||
than preserving a line ratio. Add focused restore-symlink coverage and transition
|
||||
parity coverage for the recommended changes.
|
||||
|
||||
**Reasoning:** There are 164 test files and 45,132 test lines. A focused
|
||||
`go test -cover ./...` run reported roughly 74–90% statement coverage for most
|
||||
stateful production packages (with `internal/fileops` lower at 59.6%), but the
|
||||
policy correctly treats behavioral risk—not a repository-wide percentage—as
|
||||
the objective. Existing tests are strongest around lifecycle, recovery,
|
||||
filesystem, and adapter boundaries.
|
||||
|
||||
### H14 — Cohesion of app, stage, and config
|
||||
|
||||
**Verdict:** Already satisfied at package level; focused internal work remains.
|
||||
|
||||
**Recommended change:** Keep package ownership stable. Make application
|
||||
lifecycle phases, analyze transactions, and validation domains explicit through
|
||||
private types and files.
|
||||
|
||||
**Reasoning:** Moving commands, stages, or validation into new packages would
|
||||
mostly expose private data and increase interfaces. The internal seams are real,
|
||||
but they do not require new architectural layers.
|
||||
|
||||
### H15 — Natural modules hidden in large files
|
||||
|
||||
**Verdict:** Confirmed.
|
||||
|
||||
**Recommended change:** Use these existing seams: runner initialization,
|
||||
composition, stage transition, and finalization; publish source collection,
|
||||
transfer preparation, immutable upload, and current-pointer commit; adapter and
|
||||
artifact validation domains; modern and legacy previous-cache selection.
|
||||
|
||||
**Reasoning:** The functions and types already cluster around these concerns.
|
||||
Extraction can therefore be primarily mechanical and contract-preserving.
|
||||
|
||||
### H16 — Duplication in stages and external adapters
|
||||
|
||||
**Verdict:** Not substantiated as a broad problem.
|
||||
|
||||
**Recommended change:** Keep tool-specific validation, request construction,
|
||||
and error context local. Consolidate only exact mechanics with a stable shared
|
||||
contract; do not add base-stage or base-adapter types.
|
||||
|
||||
**Reasoning:** Adapters already share `internal/adapters/subprocess`, and stage
|
||||
helpers already own run layout, materialization, and transcript validation.
|
||||
Remaining similarities are mostly small wrappers or deliberately different
|
||||
tool contracts. The meaningful duplicate found by this audit is in fileops,
|
||||
not the stage/adapter boundary.
|
||||
|
||||
### H17 — Durable versus implementation-coupled tests
|
||||
|
||||
**Verdict:** Mostly healthy, with focused opportunities.
|
||||
|
||||
**Recommended change:** Retain behavior-level manifest, subprocess, recovery,
|
||||
and filesystem tests for the reduced contract. Delete tests tied to abandoned
|
||||
fileops mechanisms and guarantees; use a small dependency struct instead of
|
||||
mutable app globals; use structural diffs for large values where useful.
|
||||
|
||||
**Reasoning:** Most tests assert outputs, persisted state, CLI behavior, or
|
||||
external command contracts. The main incidental coupling comes from private
|
||||
hook algorithms, tests that elevate defensive implementation choices into
|
||||
contracts, and package-global function substitution, not from the suite as a
|
||||
whole.
|
||||
|
||||
### H18 — Invariant discoverability across packages
|
||||
|
||||
**Verdict:** Confirmed for filesystem policy; otherwise already satisfied.
|
||||
|
||||
**Recommended change:** Revise the architecture, operations, and internal
|
||||
filesystem documents so they distinguish required outcomes from the current
|
||||
implementation's power-loss, no-follow, identity-race, and no-replace
|
||||
mechanisms. Fix restore target inspection as described in H46. Keep transaction-
|
||||
order comments adjacent to runner and publish code.
|
||||
|
||||
**Reasoning:** The invariants are discoverable, but the filesystem documents
|
||||
currently make several defensive mechanisms appear to be permanent product
|
||||
contracts. Restore planning's use of `os.Stat` is also inconsistent with the
|
||||
root-scoped restore rule that remains justified.
|
||||
|
||||
### H19 — Upgrade Go 1.25 to Go 1.27
|
||||
|
||||
**Verdict:** Confirmed.
|
||||
|
||||
**Recommended change:** In one dedicated change, update `go.mod`, all Woodpecker
|
||||
images including the pinned release image, release documentation, and generated
|
||||
module state. Use the Go 1.27 `os.Root.RemoveAll` operation where root-scoped
|
||||
recursive removal remains required. Run the full test, race, vet, documentation,
|
||||
example, and six-target release-build checks required by the release procedure.
|
||||
|
||||
**Reasoning:** [Go 1.27.0](https://go.dev/doc/go1.27) is now released. The full
|
||||
suite and vet pass under it, and all supported release targets cross-compile
|
||||
successfully. A dedicated upgrade keeps any dependency or tool behavior changes
|
||||
separate from refactors.
|
||||
|
||||
### H20 — Replace custom confinement with `os.Root`
|
||||
|
||||
**Verdict:** Focused opportunity, not a package-wide filesystem model.
|
||||
|
||||
**Recommended change:** Use `os.Root` only where a root-relative boundary is an
|
||||
actual requirement: destructive cleanup, restore, and any other explicitly
|
||||
scoped operation. Prefer direct `os` and `io` operations for ordinary trusted-
|
||||
workspace reads, writes, copies, and renames. Do not preserve a universal
|
||||
confined-filesystem facade merely because its current implementation uses
|
||||
`os.Root`.
|
||||
|
||||
**Reasoning:** `os.Root` is an idiomatic way to express scope, particularly with
|
||||
`RemoveAll` available in Go 1.27. Applying it to every filesystem operation,
|
||||
however, preserves no-follow and handle-relative complexity that Narratio's
|
||||
trusted workspace model does not require.
|
||||
|
||||
### H21 — Retain Narratio-owned durability and policy code
|
||||
|
||||
**Verdict:** Not substantiated as stated; retain only focused product policies.
|
||||
|
||||
**Recommended change:** Keep atomic visibility for corruption-sensitive file
|
||||
replacement, centralized bounded-I/O limits and clear adapter errors, group-
|
||||
owned workspace modes, validated downloads before installation, root-scoped
|
||||
destructive and restore operations, and stricter treatment of API-key material.
|
||||
Drop file and directory syncing, power-loss durability claims, platform-
|
||||
unsupported directory-sync errors, atomic no-replace directory promotion,
|
||||
universal regular-file/no-follow restrictions, and hostile-race identity checks.
|
||||
|
||||
**Reasoning:** Most Narratio data is intentionally shareable D&D material, and
|
||||
the application runs under a session lock in a trusted workspace without heavy
|
||||
write concurrency. Atomic replacement still prevents readers from observing a
|
||||
partially written manifest or configuration, and bounded I/O still prevents a
|
||||
runaway adapter from exhausting disk or memory. The stronger guarantees do not
|
||||
address a realistic product risk proportionate to their implementation and test
|
||||
cost. In this context, “durable” should mean managed persistent state, not a
|
||||
promise of survival across every acknowledged power-loss window.
|
||||
|
||||
### H22 — Use `os.Process.WithHandle` on Windows
|
||||
|
||||
**Verdict:** Confirmed after the Go baseline upgrade.
|
||||
|
||||
**Recommended change:** Assign the suspended Windows child to the owned job
|
||||
inside `cmd.Process.WithHandle`, removing the PID-based `OpenProcess` and its
|
||||
separate close. Retain suspended creation, job configuration, initial-thread
|
||||
resumption, failure cleanup, and job disposal.
|
||||
|
||||
**Reasoning:** `WithHandle` supplies a lifetime-stable process handle and
|
||||
eliminates the PID reopen race. It does not replace the other job-object
|
||||
lifecycle responsibilities.
|
||||
|
||||
### H23 — Use `slices` and `maps`
|
||||
|
||||
**Verdict:** Focused opportunity.
|
||||
|
||||
**Recommended change:** Replace plain shallow clones, membership checks, and
|
||||
sorted key extraction with standard helpers when nil/empty and ordering
|
||||
semantics remain identical. Do not replace topological ordering, domain
|
||||
normalization, deep copies, or provenance-preserving merges.
|
||||
|
||||
**Reasoning:** The codebase contains many simple map-key/sort and slice-clone
|
||||
patterns, but many similarly shaped helpers encode real domain semantics. This
|
||||
is suitable for opportunistic cleanup with tests, not a bulk rewrite.
|
||||
|
||||
### H24 — Use `errgroup` for transcription
|
||||
|
||||
**Verdict:** Confirmed.
|
||||
|
||||
**Recommended change:** Use `errgroup.WithContext` with `SetLimit` to run one
|
||||
goroutine per prepared audio job. Return speaker-qualified errors from each job,
|
||||
lock only successful aggregation, and preserve deterministic output ordering and
|
||||
the useful parent-cancellation diagnostic.
|
||||
|
||||
**Reasoning:** The current worker pool manually owns a job channel, wait group,
|
||||
cancel function, first-error lock, and three repeated error paths. `errgroup`
|
||||
directly provides the intended bounded cancellation and first-error behavior,
|
||||
while result aggregation remains Narratio-specific.
|
||||
|
||||
### H25 — Use `go-cmp` selectively
|
||||
|
||||
**Verdict:** Focused opportunity.
|
||||
|
||||
**Recommended change:** Adopt `cmp.Diff` in tests comparing large composed
|
||||
configuration, analyze-state projections, restore plans, and publish mappings.
|
||||
Use `slices.Equal` or direct comparisons for simple values; do not migrate all
|
||||
107 `reflect.DeepEqual` uses mechanically.
|
||||
|
||||
**Reasoning:** Large structural mismatches currently provide poor diagnostics,
|
||||
but small comparisons do not justify a new idiom or options boilerplate.
|
||||
|
||||
### H26 — Adopt `gofrs/flock`
|
||||
|
||||
**Verdict:** Not substantiated.
|
||||
|
||||
**Recommended change:** Retain the current Unix and Windows lock primitives and
|
||||
context polling. Reconsider only if lock modes or platforms expand materially.
|
||||
|
||||
**Reasoning:** The platform-specific operations are very small. Narratio also
|
||||
owns confined creation, group-writable mode, metadata, held file lifetime, and
|
||||
error adaptation; an external flock object would not remove those obligations.
|
||||
|
||||
### H27 — Adopt Cobra
|
||||
|
||||
**Verdict:** Deferred; not currently substantiated.
|
||||
|
||||
**Recommended change:** Keep `flag` and the existing shared parsers. Re-evaluate
|
||||
only if another round of nested commands produces demonstrated help, completion,
|
||||
or flag-inheritance costs that outweigh compatibility risk.
|
||||
|
||||
**Reasoning:** The command surface has grown, but dispatch and common bounded-run
|
||||
and config flags are already centralized. A migration would touch extensively
|
||||
tested parsing, help, error, and output behavior without addressing a current
|
||||
correctness or maintenance blocker.
|
||||
|
||||
### H28 — General configuration framework or schema validator
|
||||
|
||||
**Verdict:** Not substantiated.
|
||||
|
||||
**Recommended change:** Retain strict YAML decoding, presence-aware composition,
|
||||
source ownership, profile overlays, defaults, and semantic Go validation.
|
||||
|
||||
**Reasoning:** Most complexity is Narratio's field relationships and provenance,
|
||||
not generic schema shape. A framework would add a second representation while
|
||||
still requiring the existing semantic checks.
|
||||
|
||||
### H29 — Generic atomic-file package
|
||||
|
||||
**Verdict:** Focused opportunity, but no new dependency is presently justified.
|
||||
|
||||
**Recommended change:** Treat same-directory temporary-file replacement as a
|
||||
generic operation and expose only a small local helper for it. Localize any
|
||||
unavoidable platform-specific replace mechanic behind that helper. Use direct
|
||||
standard-library operations for everything else instead of retaining a broad
|
||||
`fileops` facade.
|
||||
|
||||
**Reasoning:** Once file and directory syncing, hostile-race defenses, and
|
||||
universal confinement are removed from the contract, atomic file replacement
|
||||
is no longer Narratio-specific. A thin local implementation removes more
|
||||
complexity than a general framework while keeping the supported-platform and
|
||||
group-mode behavior explicit. Dependency research may revisit that choice if a
|
||||
maintained package demonstrably removes more code than it adds.
|
||||
|
||||
### H30 — Provider-neutral object-storage abstraction
|
||||
|
||||
**Verdict:** Already satisfied internally; no external abstraction is needed.
|
||||
|
||||
**Recommended change:** Keep the narrow Narratio-owned `storage.ObjectStore` and
|
||||
`ReaderUploader` interfaces, with AWS SDK types confined to the S3 adapter.
|
||||
|
||||
**Reasoning:** The boundary already expresses the required list/read/upload and
|
||||
conditional-write semantics without inferring session policy. A generic storage
|
||||
framework would be broader, not simpler.
|
||||
|
||||
### H31 — Retry or backoff package
|
||||
|
||||
**Verdict:** Not substantiated.
|
||||
|
||||
**Recommended change:** Keep the explicit fixed-delay WhisperX retry loop and
|
||||
the AWS SDK's provider-owned retry behavior.
|
||||
|
||||
**Reasoning:** Narratio has one small application retry policy with specific
|
||||
status, cancellation, attempt, and error semantics. A general policy library
|
||||
would add configuration and indirection without removing domain decisions.
|
||||
|
||||
### H32 — UUID or ULID package
|
||||
|
||||
**Verdict:** Not substantiated.
|
||||
|
||||
**Recommended change:** Retain the timestamp plus cryptographic-random suffix
|
||||
run ID and its injected test seam.
|
||||
|
||||
**Reasoning:** The identifier has a documented, readable format and needs no
|
||||
UUID interoperability or ULID parsing/ordering contract. Its implementation is
|
||||
small and correctly uses `crypto/rand`.
|
||||
|
||||
### H33 — Generic subprocess framework
|
||||
|
||||
**Verdict:** Not substantiated.
|
||||
|
||||
**Recommended change:** Retain `internal/adapters/subprocess` and tool-specific
|
||||
adapters.
|
||||
|
||||
**Reasoning:** Shared execution already owns redaction, bounded diagnostic
|
||||
capture, environment policy, timeouts, and cross-platform process-tree cleanup.
|
||||
Individual adapters correctly retain command construction and output contracts.
|
||||
|
||||
### H34 — `internal/app.executePlan`
|
||||
|
||||
**Verdict:** Confirmed; highest-priority maintainability refactor after the
|
||||
restore correctness fix.
|
||||
|
||||
**Recommended change:** Introduce a private invocation runtime carrying the
|
||||
resolved identity, paths, stores, manifests, environment, and summary state.
|
||||
Extract five lifecycle operations: prepare and lock the invocation; create
|
||||
durable invocation state; compose required collaborators; execute one stage
|
||||
transition; and finalize/clean up. Keep lock acquisition/release and all
|
||||
manifest persistence ordering explicit in the top-level function.
|
||||
|
||||
**Reasoning:** The approximately 456-line function has changed in at least 53
|
||||
commits and spans preflight, locking, manifest creation, dependency composition,
|
||||
resume, transitions, failure persistence, and cleanup. These are existing seams;
|
||||
making them explicit reduces change blast without redesigning the state machine.
|
||||
|
||||
### H35 — Shared plan/execution transition model
|
||||
|
||||
**Verdict:** Confirmed narrowly.
|
||||
|
||||
**Recommended change:** Share a pure pre-execution decision value that combines
|
||||
recorded status, force, semantic/resume validation outcome, stale reason, and
|
||||
required invalidation. Share small manifest-transition helpers where planning
|
||||
and execution apply identical state. Keep execution persistence and actual
|
||||
stage outcomes separate, and add parity tests for run/skip, non-resumable,
|
||||
forced invalidation, self-skip, and changed-success cases.
|
||||
|
||||
**Reasoning:** `Plan` and `executePlan` use several shared helpers but still
|
||||
independently sequence stale marking and invalidation. A full transition engine
|
||||
would overfit because planning models outcomes while execution observes them;
|
||||
the common pre-execution decision is the stable seam.
|
||||
|
||||
### H36 — `analyzeStage.Run`
|
||||
|
||||
**Verdict:** Focused opportunity; the preliminary description is partly stale.
|
||||
|
||||
**Recommended change:** Add a private execution accumulator for session and
|
||||
invocation records, outputs, logs, generated configs, reused artifacts, and
|
||||
metadata. Let `Run` retain setup, ordered execution, and final result ownership.
|
||||
|
||||
**Reasoning:** Planning, reconciliation, fingerprinting, input identity, and
|
||||
resume logic are already separated into dedicated files. `Run` still coordinates
|
||||
many parallel result collections, so an accumulator would make projection and
|
||||
final assembly easier to verify without another package.
|
||||
|
||||
### H37 — `executeAnalyzeArtifact`
|
||||
|
||||
**Verdict:** Confirmed.
|
||||
|
||||
**Recommended change:** Split request/input preparation, adapter invocation,
|
||||
validated output materialization, and analyze-record/evidence construction into
|
||||
private helpers under one transaction wrapper. Replace the current ten
|
||||
parameters with a narrow execution context plus the artifact plan.
|
||||
|
||||
**Reasoning:** The 239-line function is cohesive as a transaction but has 10
|
||||
parameters and mixes resolution, provenance aggregation, command preparation,
|
||||
filesystem validation, canonical installation, checksum/contract construction,
|
||||
and result projection. Those phases already have explicit data boundaries.
|
||||
|
||||
### H38 — Aggregate complexity of `config/validate.go`
|
||||
|
||||
**Verdict:** Confirmed as a navigation and ownership issue, not a validation
|
||||
design failure.
|
||||
|
||||
**Recommended change:** Split the file within package `config` into core
|
||||
pipeline/session validation, external-adapter validation, publish/storage
|
||||
validation, and Scriptorium artifact/dependency validation. Preserve validation
|
||||
order and exact field-specific errors.
|
||||
|
||||
**Reasoning:** The file is 1,111 lines and has changed in at least 48 commits.
|
||||
Its guards are mostly necessary, but unrelated configuration domains sharing one
|
||||
file increases conflict and review scope.
|
||||
|
||||
### H39 — `validateScriptorium`
|
||||
|
||||
**Verdict:** Confirmed.
|
||||
|
||||
**Recommended change:** Extract artifact declaration/output validation,
|
||||
dependency/input validation, and variable validation into helpers. Sort artifact
|
||||
keys before validation so error order does not depend on Go map iteration.
|
||||
|
||||
**Reasoning:** The function validates several distinct artifact-level contracts
|
||||
and accumulates cross-artifact state. The dependency graph already has a separate
|
||||
validator, demonstrating the natural boundary.
|
||||
|
||||
### H40 — `validateNotarius`
|
||||
|
||||
**Verdict:** Confirmed narrowly.
|
||||
|
||||
**Recommended change:** Extract reference-binding normalization and output-lane
|
||||
normalization/collision checking into separate helpers, leaving enablement and
|
||||
top-level scalar checks in `validateNotarius`.
|
||||
|
||||
**Reasoning:** The function now has clear reference and output phases with
|
||||
different collision domains. Separating them improves reviewability while
|
||||
preserving the normalization mutation contract.
|
||||
|
||||
### H41 — `validateAudita`
|
||||
|
||||
**Verdict:** Not substantiated as a function-level refactor.
|
||||
|
||||
**Recommended change:** Move it with the other adapter validators during the
|
||||
file split, but keep the function intact.
|
||||
|
||||
**Reasoning:** Its 54 lines are a straightforward sequence of independent,
|
||||
field-specific guards. Extracting each switch or numeric check would add names
|
||||
without reducing conceptual load.
|
||||
|
||||
### H42 — `splitPipelineCompositionEnvelope`
|
||||
|
||||
**Verdict:** Focused opportunity.
|
||||
|
||||
**Recommended change:** Separate envelope shape decoding from cross-field
|
||||
semantic validation and root-metadata removal. Reuse the existing profile
|
||||
declaration parser and preserve every source-aware diagnostic.
|
||||
|
||||
**Reasoning:** The function still combines mapping/list/scalar decoding,
|
||||
unknown-field checks, profile parsing, default-profile membership, cloning, and
|
||||
root mutation. These are separable phases, but provenance makes a generic YAML
|
||||
decoder inappropriate.
|
||||
|
||||
### H43 — `previouscache.Resolve`
|
||||
|
||||
**Verdict:** Confirmed.
|
||||
|
||||
**Recommended change:** Extract committed-artifact selection and legacy
|
||||
candidate probing into separate functions returning one common selected-object
|
||||
value. Keep required/optional policy, local record construction, and final
|
||||
sorting in `Resolve`. Mark the legacy helper as compatibility-only and removable
|
||||
after migration.
|
||||
|
||||
**Reasoning:** Immutable commits and legacy existence probing are distinct
|
||||
algorithms currently interleaved in the requirement loop. Isolating the legacy
|
||||
surface makes both paths easier to test and eventually delete.
|
||||
|
||||
### H44 — `internal/stage/publish.go`
|
||||
|
||||
**Verdict:** Confirmed as a file-level decomposition.
|
||||
|
||||
**Recommended change:** Split source discovery/opening, output rule resolution,
|
||||
transfer preparation and verification, and commit/pointer encoding into focused
|
||||
files. Keep `publishStage.Run` as the visible transaction and preserve the exact
|
||||
ordering: immutable objects, immutable commit, lock revalidation, then one final
|
||||
current-pointer write.
|
||||
|
||||
**Reasoning:** The 1,175-line file already consists of cohesive helper groups.
|
||||
The main risk is obscuring the commit boundary, so decomposition should improve
|
||||
navigation without distributing transaction ownership.
|
||||
|
||||
### H45 — `internal/fileops/directory.go`
|
||||
|
||||
**Verdict:** Confirmed for removal; see H06 and H21.
|
||||
|
||||
**Recommended change:** Delete both directory-promotion implementations, their
|
||||
test hooks, copy helpers, identity checks, directory syncing, and platform-
|
||||
specific no-replace rename code. Have extract validate the Notarius staging
|
||||
bundle and use a direct same-workspace `os.Rename` to its absent, unique run
|
||||
destination. Treat a destination collision as an ordinary error and clean up
|
||||
only verified staging paths.
|
||||
|
||||
**Reasoning:** The duplicate implementation is the clearest literal duplication
|
||||
found, but the production implementation is also unnecessary under the revised
|
||||
contract. Notarius already writes beneath the invocation output root, and the
|
||||
destination is protected by the session lock and unique run ID. Direct rename
|
||||
removes an entire tree copy and the need to preserve a second pre-promotion copy;
|
||||
the receipt and diagnostic log remain the appropriate troubleshooting record.
|
||||
|
||||
### H46 — `classifyRestoreAction`
|
||||
|
||||
**Verdict:** Confirmed for correctness, not merely complexity.
|
||||
|
||||
**Recommended change:** Fold classification into the simpler root-scoped
|
||||
restore implementation. Resolve and inspect targets relative to an `os.Root`,
|
||||
without layering inode-identity rechecks onto the operation. Classify a symlink
|
||||
at the leaf or in an ancestor as `RestoreConflictNonRegular`, even when its
|
||||
target bytes match. Hash a verified regular target without reopening an
|
||||
unconfined absolute path. Add leaf- and ancestor-symlink tests for force and
|
||||
non-force plans. Optionally extract the legacy remote-download digest comparison
|
||||
after this behavior is fixed.
|
||||
|
||||
**Reasoning:** `os.Stat(localPath)` follows a leaf symlink, and
|
||||
`artifacts.SHA256File(localPath)` follows it again. A matching target can
|
||||
therefore produce `skip_same` and leave an unmanaged symlink in place, contrary
|
||||
to the retained restore-scope contract. This is an accidental-scope issue, not
|
||||
a reason to retain hostile-race defenses throughout ordinary file operations.
|
||||
The rest of the function is an explicit and readable state matrix whose
|
||||
branching is justified.
|
||||
|
||||
### H47 — `internal/manifest/analyze_state.go`
|
||||
|
||||
**Verdict:** Retain.
|
||||
|
||||
**Recommended change:** Keep validation together with the state model. Apply
|
||||
only opportunistic `slices.Clone` cleanup where semantics are unchanged.
|
||||
|
||||
**Reasoning:** The file is organized into small validators for record status,
|
||||
current output, dependencies, fingerprints, provenance, bounds, normalization,
|
||||
and deep cloning. Its complexity directly expresses durable state invariants and
|
||||
is easier to audit in one owner.
|
||||
|
||||
### H48 — Preliminary priority order
|
||||
|
||||
**Verdict:** Confirmed with a broader filesystem correction.
|
||||
|
||||
**Recommended change:** Define the reduced filesystem policy and replace the
|
||||
current fileops subsystem, incorporating the restore classification correction
|
||||
into the root-scoped restore design. Next decompose execution and share its
|
||||
narrow planning decision model. Follow with analyze/config, publish/previous-
|
||||
cache, and finally modernization/test ergonomics.
|
||||
|
||||
**Reasoning:** The restore mismatch remains a correctness issue, but preserving
|
||||
the surrounding abstraction solely to fix it would reinforce the mistaken
|
||||
contract. Execution remains the largest non-filesystem change-blast hotspot,
|
||||
and configuration/analyze remain the next tier.
|
||||
|
||||
## Recommended Implementation Sequence
|
||||
|
||||
This is ordering guidance for a future decision-complete implementation plan,
|
||||
not staged implementation instructions:
|
||||
|
||||
1. Define the reduced filesystem policy in its canonical documents.
|
||||
2. Upgrade the Go baseline to 1.27 in isolation and run the full release
|
||||
candidate checks.
|
||||
3. Replace the broad fileops facade with direct standard-library operations and
|
||||
focused helpers, simplify extract to validated directory rename, incorporate
|
||||
the root-scoped restore correction, and replace obsolete mechanism tests with
|
||||
tests of the retained outcomes.
|
||||
4. Adopt `os.Process.WithHandle` and simplify transcription with `errgroup` as
|
||||
separate, bounded modernization changes.
|
||||
5. Decompose `executePlan`, then introduce the shared narrow pre-execution
|
||||
decision model with parity tests.
|
||||
6. Refactor analyze orchestration and the per-artifact transaction.
|
||||
7. Split configuration validation and composition-envelope responsibilities.
|
||||
8. Split previous-cache compatibility resolution and publish file concerns.
|
||||
9. Improve app test dependency injection and apply selective `slices`, `maps`,
|
||||
and `go-cmp` cleanup only in code touched by the preceding work.
|
||||
|
||||
The filesystem decisions are specified in [`fileops.md`](fileops.md). No open
|
||||
product or architecture decision is required before turning these findings into
|
||||
an implementation plan. The implementation plan should preserve the scope
|
||||
boundaries and rejected alternatives recorded here.
|
||||
@@ -1,525 +0,0 @@
|
||||
# Contract Simplification Audit
|
||||
|
||||
## Status
|
||||
|
||||
Complete and approved as a statement of intended direction. This document
|
||||
records product and architecture decisions for future work; it does not describe
|
||||
implemented behavior and is not an implementation plan.
|
||||
|
||||
This audit applies the same contract-challenge premise used by the
|
||||
[File Operations Simplification roadmap](fileops.md) to other maintainability
|
||||
findings in [`audit.md`](audit.md): current behavior is not automatically a
|
||||
durable product requirement merely because production code, tests, or
|
||||
documentation currently preserve it.
|
||||
|
||||
## Context
|
||||
|
||||
Narratio is a trusted, single-session workflow orchestrator for intentionally
|
||||
shareable D&D material. Its most important operational outcomes are correct
|
||||
pipeline progress, economical reuse of expensive successful work, coherent
|
||||
remote publication, understandable recovery, safe cleanup scope, and protection
|
||||
of API keys. It does not need compliance-grade invocation history, indefinite
|
||||
compatibility with already-deprecated formats, or distributed coordination
|
||||
machinery disproportionate to its actual use.
|
||||
|
||||
The maintainability audit found several areas where implementation mechanisms
|
||||
had become apparent contracts. This follow-up review explicitly permits those
|
||||
contracts to change when a simpler policy preserves the user outcome that
|
||||
matters.
|
||||
|
||||
## Governing Decisions
|
||||
|
||||
- Remove deprecated legacy and compatibility behavior completely rather than
|
||||
continuing to isolate and maintain it.
|
||||
- Keep the session manifest as the sole durable local progress authority.
|
||||
- Preserve successful analyze artifacts individually and checkpoint them as
|
||||
they complete.
|
||||
- Express local cleanup as an ordinary final pipeline stage rather than a
|
||||
publish side effect or special manifest obligation.
|
||||
- Represent each remote artifact lock as one independently managed remote
|
||||
object and carry locked artifacts forward into later current snapshots.
|
||||
- Preserve configuration semantics and useful diagnostics without treating
|
||||
exact validation order, prose, or formatting as compatibility contracts.
|
||||
- Release this work as a minor version. The removed compatibility surfaces were
|
||||
already deprecated and explicitly subject to removal; their deletion does not
|
||||
require a major-version release under the project decision recorded here.
|
||||
|
||||
## Finding 1 — Deprecated Compatibility Has Become Cross-Cutting Product Code
|
||||
|
||||
### Underlying issue
|
||||
|
||||
Narratio retains multiple compatibility paths for state and configuration that
|
||||
predate current canonical contracts. These paths include legacy remote current
|
||||
state, candidate probing for previous-session artifacts, unversioned party and
|
||||
separate players inputs, aggregate-only analyze state, older extraction
|
||||
evidence, and manifests without current semantic evidence.
|
||||
|
||||
Although several paths are labeled temporary or migration-only, their presence
|
||||
still requires alternate parsing and selection algorithms, ambiguity handling,
|
||||
branches in current workflows, compatibility-specific errors, fixtures, tests,
|
||||
and documentation. Isolating these paths makes later removal easier, but does
|
||||
not remove their ongoing cost. Continued support also makes it harder to reason
|
||||
about which representation is authoritative.
|
||||
|
||||
### Approved changes
|
||||
|
||||
Purge deprecated compatibility behavior across production code, tests,
|
||||
configuration, documentation, examples, and schemas. The purge includes:
|
||||
|
||||
- the legacy remote `current/manifest.json` and `current/run_id.txt` reader;
|
||||
- legacy previous-session artifact path candidates and existence probing;
|
||||
- unversioned party documents and separate `players_file` behavior;
|
||||
- legacy session-level party and players overrides that exist only for that
|
||||
input model;
|
||||
- aggregate-only legacy analyze availability and fallback to incidental files;
|
||||
- compatibility for extraction records that lack the current contract and
|
||||
provenance evidence;
|
||||
- compatibility branches for manifests or artifact records that predate the
|
||||
current supported schema; and
|
||||
- associated legacy constants, types, helpers, errors, tests, fixtures,
|
||||
documentation, and examples.
|
||||
|
||||
Current state must have an explicit supported version or enough required
|
||||
authority markers to be recognized unambiguously. Unsupported old local or
|
||||
remote state must fail early with a clear migration/version error. Narratio must
|
||||
not silently normalize, reinterpret, probe around, or partially accept it.
|
||||
|
||||
Migration guidance may explain how to regenerate, republish, or otherwise
|
||||
replace old state, but migration readers must not remain in the steady-state
|
||||
runtime merely to preserve automatic compatibility.
|
||||
|
||||
### Rationale
|
||||
|
||||
Compatibility was already deprecated and bounded for removal. Retaining it
|
||||
indefinitely converts a temporary cost into permanent architecture. One
|
||||
canonical representation is easier to validate, test, document, restore, and
|
||||
extend, and a clear rejection is safer than heuristic interpretation of old
|
||||
state.
|
||||
|
||||
### Final state
|
||||
|
||||
Every supported configuration, party document, local manifest, remote current
|
||||
snapshot, extraction record, analyze record, and previous-session artifact is
|
||||
resolved through one canonical contract. Deprecated formats are absent from
|
||||
production code and tests and receive a clear unsupported-version or migration
|
||||
error rather than fallback behavior.
|
||||
|
||||
## Finding 2 — Invocation Diagnostics Participate in Pipeline Correctness
|
||||
|
||||
### Underlying issue
|
||||
|
||||
Narratio currently maintains both an authoritative session manifest and a
|
||||
structured run manifest for each invocation. The runner persists overlapping
|
||||
stage actions, statuses, outputs, semantic evidence, failures, and terminal
|
||||
state to both documents. This creates dual-save ordering, joined persistence
|
||||
errors, separate session and invocation projections, terminal-failure recovery
|
||||
machinery, and tests for keeping a diagnostic record synchronized with the
|
||||
actual progress ledger.
|
||||
|
||||
Historical per-invocation records are not operationally important. They are at
|
||||
most useful diagnostics and do not justify being a second participant in
|
||||
pipeline transactions. Their placement inside a run directory that cleanup may
|
||||
delete further demonstrates that they are not durable progress authority.
|
||||
|
||||
### Approved changes
|
||||
|
||||
Eliminate the structured run manifest and its persistence lifecycle.
|
||||
|
||||
Retain:
|
||||
|
||||
- the run ID used for invocation identity, run-local paths, adapter provenance,
|
||||
and immutable remote publication;
|
||||
- the session manifest as the sole durable local record of current pipeline and
|
||||
artifact state;
|
||||
- ordinary bounded logs, generated configuration, receipts, and other
|
||||
diagnostics where already useful; and
|
||||
- the in-memory command result or summary reported to the caller.
|
||||
|
||||
Remove:
|
||||
|
||||
- run-manifest types, storage methods, path helpers, schema and validation;
|
||||
- before-stage, after-stage, skip, failure, and terminal run-manifest saves;
|
||||
- invocation-only analyze projections and output projections;
|
||||
- publish handling that archives or classifies a run manifest;
|
||||
- terminal persistence code whose only purpose is to coordinate session and
|
||||
run records; and
|
||||
- run-manifest documentation, compatibility handling, fixtures, and tests.
|
||||
|
||||
No replacement historical record is required. A lightweight diagnostic summary
|
||||
may be reconsidered later only if it can remain independent of pipeline
|
||||
correctness and cleanup.
|
||||
|
||||
### Rationale
|
||||
|
||||
One durable authority eliminates synchronization and split-brain questions.
|
||||
The session manifest already records the state needed for resume, status,
|
||||
restore, artifact resolution, and publication. Logs and the immediate command
|
||||
summary provide proportionate invocation diagnostics without making diagnostic
|
||||
write failures fail otherwise valid pipeline work.
|
||||
|
||||
### Final state
|
||||
|
||||
The session manifest alone determines durable local pipeline progress. A run ID
|
||||
still identifies an invocation and its files, but no second manifest mirrors
|
||||
session transitions. Pipeline success or failure cannot be changed by an error
|
||||
writing historical invocation metadata.
|
||||
|
||||
## Finding 3 — Analyze Recovery Is Obscured by Dual Projection Machinery
|
||||
|
||||
### Underlying issue
|
||||
|
||||
Narratio correctly models configured analyze artifacts individually, but the
|
||||
implementation combines that valuable behavior with separate session and
|
||||
invocation collections, partial-result projection validation, rollback of
|
||||
in-memory authority after persistence failure, and special runner handling for
|
||||
a stage that returns both a result and an error.
|
||||
|
||||
The desired user behavior is simpler than the machinery. Expensive successful
|
||||
artifacts should remain reusable; a failed artifact and work that depends on it
|
||||
should be retried. Invocation-specific audit projections are unnecessary once
|
||||
run manifests are removed.
|
||||
|
||||
### Approved behavior
|
||||
|
||||
Assume an ordered artifact plan contains A, B, C, and D. If A and B succeed, C
|
||||
fails, and D is not attempted:
|
||||
|
||||
- A and B remain current, provided their output and semantic evidence still
|
||||
validate;
|
||||
- C is recorded as failed and has no current output authority;
|
||||
- D remains missing if it has never succeeded, or becomes stale if an older
|
||||
current D depends directly or transitively on C;
|
||||
- the aggregate analyze stage is failed and execution stops;
|
||||
- an ordinary later run or resume retains A and B, retries C, and proceeds to D
|
||||
after C succeeds; and
|
||||
- a forced analyze invocation reruns the effective selection regardless of
|
||||
current records.
|
||||
|
||||
Without `--artifacts`, force applies to the full configured analyze artifact
|
||||
set. With `--artifacts`, force applies to the selected artifacts and the
|
||||
dependency closure required by the established artifact-selection contract;
|
||||
unrelated valid artifacts remain outside that invocation.
|
||||
|
||||
### Approved changes
|
||||
|
||||
Use the session manifest's analyze artifact collection as the only durable
|
||||
artifact authority. Remove the invocation collection and all logic that exists
|
||||
only to construct, validate, apply, or roll back an invocation-specific
|
||||
projection.
|
||||
|
||||
Execute the effective artifact set in deterministic dependency order. After
|
||||
each artifact succeeds and its canonical output has been validated and
|
||||
materialized:
|
||||
|
||||
1. construct or update its current session artifact record;
|
||||
2. stale any current transitive dependents when the new result invalidates
|
||||
their dependency evidence;
|
||||
3. atomically save the session manifest; and
|
||||
4. only then proceed to the next artifact.
|
||||
|
||||
If the checkpoint save fails, the new file alone does not establish authority;
|
||||
the invocation fails and the artifact is eligible to run again. This safe
|
||||
recomputation is preferable to a rollback transaction around incidental output
|
||||
bytes.
|
||||
|
||||
When an artifact fails, record its failed state, stale current transitive
|
||||
dependents, mark the aggregate stage failed, and atomically save those changes
|
||||
to the session manifest before returning the artifact error. A later run
|
||||
reconciles each selected artifact from the persisted collection instead of
|
||||
rerunning the entire stage automatically.
|
||||
|
||||
Retain:
|
||||
|
||||
- per-artifact fingerprints and validated output evidence;
|
||||
- deterministic dependency ordering;
|
||||
- selected-artifact and dependency-closure semantics;
|
||||
- invalidation when configuration, inputs, dependencies, or output identity
|
||||
changes; and
|
||||
- the rule that incidental canonical files never create current authority.
|
||||
|
||||
Simplify failure and persistence tests around these observable checkpoints
|
||||
rather than the current projection and rollback choreography.
|
||||
|
||||
### Rationale
|
||||
|
||||
Checkpointing after each successful artifact matches the economic reality that
|
||||
LLM work is expensive while one manifest write is cheap. It preserves A and B
|
||||
after an ordinary error, cancellation, or process interruption. The manifest
|
||||
remains the one authority, and a failed checkpoint safely causes recomputation
|
||||
rather than ambiguous reuse.
|
||||
|
||||
Dependency-aware reuse is worth retaining because character families,
|
||||
artifact selection, and ongoing prompt development make whole-stage reruns
|
||||
unnecessarily expensive. Invocation-only state is not.
|
||||
|
||||
### Final state
|
||||
|
||||
Analyze behaves as a resumable deterministic artifact queue backed by one
|
||||
session collection. Successful valid artifacts survive later failures, the
|
||||
first failed or invalid artifact is retried, downstream work resumes when its
|
||||
prerequisites become current, and `--force` deliberately replaces normal reuse
|
||||
within its effective selection.
|
||||
|
||||
## Finding 4 — Cleanup Is a Hidden Publish Transaction
|
||||
|
||||
### Underlying issue
|
||||
|
||||
Remote publication and local deletion are independent operator outcomes, but
|
||||
Narratio currently couples them. After publish commits remotely, the runner
|
||||
creates a special durable cleanup obligation in the session manifest, deletes
|
||||
eligible local targets one by one, persists completion evidence after each,
|
||||
and reports the invocation as incomplete if cleanup fails. An operator can
|
||||
therefore observe an apparent publish failure even though the remote commit is
|
||||
already authoritative.
|
||||
|
||||
The special obligation duplicates behavior already provided by pipeline stage
|
||||
status and resume.
|
||||
|
||||
### Approved changes
|
||||
|
||||
Add `cleanup` as the final stage in the canonical pipeline, after every stage
|
||||
that may need run-local material or diagnostics. Remove post-publish cleanup
|
||||
from the runner and remove the special `post_publish_cleanup` manifest model.
|
||||
|
||||
The cleanup stage must:
|
||||
|
||||
- use existing cleanup configuration to determine whether spool audio, run
|
||||
workspace data, both, or neither should be removed;
|
||||
- require evidence of a successful current remote publish before deleting data
|
||||
whose retention policy is conditioned on publication;
|
||||
- bind cleanup targets to the applicable successfully published run rather than
|
||||
infer arbitrary paths from current configuration;
|
||||
- self-skip clearly when no cleanup is configured;
|
||||
- use the root-scoped destructive safeguards defined by
|
||||
[`fileops.md`](fileops.md);
|
||||
- treat an already-absent eligible target as success so retry is idempotent;
|
||||
- record ordinary stage success or failure in the session manifest; and
|
||||
- produce no canonical artifact outputs.
|
||||
|
||||
A cleanup failure must not alter the succeeded publish record or remote current
|
||||
state. A later ordinary run skips still-valid upstream work and retries cleanup.
|
||||
If an earlier stage is forced or otherwise changes, the fixed invalidation
|
||||
relation must make the prior cleanup result stale where appropriate.
|
||||
|
||||
Selecting a range that ends before cleanup must not delete local data. A full
|
||||
default run includes cleanup because it is the canonical final stage.
|
||||
|
||||
### Rationale
|
||||
|
||||
Stage boundaries should match outcomes an operator can understand. Publication
|
||||
has succeeded when the remote commit becomes current; local deletion has
|
||||
succeeded when the selected local targets are absent. Modeling them separately
|
||||
gives each its own status, errors, retry, selection, and documentation without a
|
||||
custom transaction layered after the stage loop.
|
||||
|
||||
The removal of run manifests also eliminates the present problem in which
|
||||
cleanup deletes the directory containing an invocation record that the runner
|
||||
otherwise attempts to finalize.
|
||||
|
||||
### Final state
|
||||
|
||||
`publish` owns only coherent remote publication. `cleanup` is the final,
|
||||
idempotent local-maintenance stage. A failure is reported as `cleanup` failure,
|
||||
publish remains visibly successful, and normal resume retries cleanup without
|
||||
republishing unchanged artifacts.
|
||||
|
||||
## Finding 5 — Remote Locks Use a Shared Mutable Control Plane
|
||||
|
||||
### Underlying issue
|
||||
|
||||
The current lock system stores remote locks in one mutable `locks.yml`, merges
|
||||
them with static pipeline locks, defines precedence, validates the combined
|
||||
document, uses generation-bound conditional replacement and bounded retries to
|
||||
avoid concurrent lost updates, injects the effective set into runtime
|
||||
configuration, and reloads it before current-pointer selection.
|
||||
|
||||
This complexity follows from choosing one shared mutable document and two lock
|
||||
authorities. The product requirement is narrower: an already canonical logical
|
||||
artifact can be frozen so a later publish does not replace the canonical
|
||||
version until an operator unlocks it.
|
||||
|
||||
There is an additional semantic mismatch in treating a locked artifact as a
|
||||
simple omission. Narratio publishes immutable run-scoped objects and advances a
|
||||
current commit pointer; it does not overwrite a stable artifact object in
|
||||
place. Omitting a locked artifact from a later commit can make it disappear
|
||||
from current-state discovery rather than preserve the locked canonical version.
|
||||
|
||||
### Approved lock model
|
||||
|
||||
Use one remote lock object per stable logical artifact source. Its location is:
|
||||
|
||||
```text
|
||||
{session_prefix}/locks/{sha256(normalized_source_id)}.lock
|
||||
```
|
||||
|
||||
The lowercase hexadecimal SHA-256 name provides a fixed, portable object-key
|
||||
segment; the bounded object payload is versioned JSON containing the normalized
|
||||
source ID, an optional reason, and its creation time. Readers verify that the
|
||||
payload source hashes to the object name. The stable artifact source ID, not the
|
||||
configured remote destination, is the lock identity. Configuration can change
|
||||
a destination; it must not accidentally evade a lock on the logical artifact.
|
||||
|
||||
Lock object presence is authoritative, subject to successful bounded decoding
|
||||
and identity validation of that payload. No central index is authoritative.
|
||||
|
||||
### Approved changes
|
||||
|
||||
- Remove static `pipeline.publish.locks`, the mutable central lock-store schema,
|
||||
static/remote merging and precedence, shared-document mutation retries, and
|
||||
runtime injection of locks into resolved pipeline configuration.
|
||||
- Extend the narrow object-store contract with deletion so an individual lock
|
||||
object can be removed. Implement and test the operation in each storage
|
||||
backend and fake.
|
||||
- Keep the existing lock list/add/remove user workflow, adapted to per-source
|
||||
objects:
|
||||
- list enumerates and validates the lock prefix;
|
||||
- add creates one object, fails clearly when it already exists, and permits
|
||||
`--force` to replace its non-authoritative reason and creation metadata; and
|
||||
- remove deletes that object, treating an already-absent object as an
|
||||
idempotent success with a clear informational result.
|
||||
- Check each relevant lock before spending work to publish its artifact and
|
||||
check again immediately before advancing the current commit pointer.
|
||||
- Do not introduce a distributed transaction protocol. Narratio accepts the
|
||||
proportionate, documented boundary that a lock created after the final check
|
||||
may race the pointer write; its trusted, low-concurrency operating model does
|
||||
not justify cross-object transaction machinery.
|
||||
|
||||
### Locked-artifact carry-forward
|
||||
|
||||
When building a new remote commit:
|
||||
|
||||
- an unlocked artifact uses the newly validated and uploaded artifact
|
||||
declaration;
|
||||
- a locked artifact already present in the prior current commit carries that
|
||||
exact prior immutable declaration into the new commit;
|
||||
- a locked artifact with no prior current declaration remains absent and is not
|
||||
treated as a missing required output; and
|
||||
- invalid or unverifiable prior current state fails publication rather than
|
||||
silently replacing or dropping an artifact that is supposed to be locked.
|
||||
|
||||
The initial lock read determines which prior declarations are carried into the
|
||||
new immutable commit. If the final recheck finds that an artifact became locked
|
||||
after that commit was constructed, publication must not advance the current
|
||||
pointer; the unselected immutable attempt may remain, and the next run rebuilds
|
||||
the commit with the prior declaration carried forward. If a lock was removed
|
||||
after the initial read, the in-progress commit may conservatively carry the old
|
||||
declaration once; a later publish can advance it.
|
||||
|
||||
The remote commit contract and validation must permit a carried declaration to
|
||||
reference an immutable object under an earlier run prefix within the same
|
||||
campaign/session namespace. Its destination key, checksum, size, storage
|
||||
generation, source identity, and originating run identity remain unchanged and
|
||||
fully declared. Restore and current-state readers continue to trust only the
|
||||
objects explicitly declared by the selected commit; they must not scan mutable
|
||||
prefixes to find a carried artifact.
|
||||
|
||||
Publish metadata and inspection commands should distinguish newly published,
|
||||
carried-forward locked, locked-but-never-published, optional missing, and
|
||||
unselected artifacts.
|
||||
|
||||
### Rationale
|
||||
|
||||
Independent lock objects make the existence check the concurrency model. Adding
|
||||
or removing one lock cannot overwrite another lock's state, so a shared YAML
|
||||
merge and retry loop are unnecessary. A stable source identity prevents a
|
||||
configuration destination change from bypassing the operator's intent.
|
||||
|
||||
Carrying the prior declaration forward matches the meaning of canonical lock:
|
||||
other artifacts may advance while the locked artifact remains exactly the
|
||||
version that was current when locked. It preserves coherent immutable snapshots
|
||||
without freezing the entire session or copying the old bytes into every new run
|
||||
prefix.
|
||||
|
||||
### Final state
|
||||
|
||||
Each artifact lock is one independently visible remote object managed by small
|
||||
list/add/remove commands. Publish performs a direct lock lookup and a final
|
||||
recheck. New commits preserve prior locked artifact declarations while allowing
|
||||
unlocked artifacts to advance, with no static lock authority, shared mutable
|
||||
lock manifest, precedence rules, or lost-update retry loop.
|
||||
|
||||
## Finding 6 — Configuration Diagnostics Are Treated as Semantic Contracts
|
||||
|
||||
### Underlying issue
|
||||
|
||||
Configuration composition and validation correctly enforce meaningful product
|
||||
rules, but maintenance guidance and tests have also tended to preserve exact
|
||||
validation order, exact error prose, incidental formatting, and provenance
|
||||
detail whether or not a user-facing operation consumes it. These expectations
|
||||
increase refactoring cost and can discourage clearer validation structure.
|
||||
|
||||
The same configuration area also carries deprecated party/player and other
|
||||
legacy compatibility branches addressed by Finding 1.
|
||||
|
||||
### Approved changes
|
||||
|
||||
Retain these configuration contracts:
|
||||
|
||||
- strict YAML decoding and rejection of unknown fields;
|
||||
- additive imports with no duplicate field ownership;
|
||||
- deterministic selection and application of one profile;
|
||||
- meaningful explicit empty, false, zero, list, and map overlay behavior;
|
||||
- centralized defaults and semantic validation;
|
||||
- canonical party and artifact-family resolution;
|
||||
- deterministic effective configuration and secret-free digest behavior; and
|
||||
- diagnostics that identify the relevant field and, for composition conflicts,
|
||||
the conflicting sources well enough for an operator to correct the problem.
|
||||
|
||||
Do not treat these as durable contracts unless a specific machine-readable
|
||||
interface explicitly says otherwise:
|
||||
|
||||
- the order in which independent validation failures are discovered;
|
||||
- complete prose wording or punctuation;
|
||||
- incidental formatting of errors;
|
||||
- private helper boundaries or validation call order; and
|
||||
- per-node provenance retained only because the current implementation can
|
||||
produce it.
|
||||
|
||||
Remove legacy configuration fields and branches under Finding 1. Refactor
|
||||
validators and composition code around semantic domains without preserving
|
||||
obsolete diagnostics solely to keep exact-string tests unchanged.
|
||||
|
||||
Tests should prefer typed errors, error categories, field paths, structured
|
||||
data, or the smallest stable semantic fragment. Exact full-string assertions
|
||||
are appropriate only for documented CLI syntax or an intentionally stable
|
||||
machine-consumed contract.
|
||||
|
||||
### Rationale
|
||||
|
||||
Strictness and deterministic effective behavior prevent ambiguous execution;
|
||||
exact diagnostic choreography does not. Useful errors can remain precise
|
||||
without making their full prose a versioned API. This aligns configuration
|
||||
tests with the repository testing policy: protect meaningful outcomes while
|
||||
allowing legitimate internal reorganization.
|
||||
|
||||
### Final state
|
||||
|
||||
Configuration remains strict, deterministic, source-aware where useful, and
|
||||
operator-friendly. Validators may be reorganized and diagnostics improved
|
||||
without preserving incidental order or wording. Only the canonical party,
|
||||
composition, profile, and artifact-family models remain.
|
||||
|
||||
## Consolidated Target State
|
||||
|
||||
After the approved work is complete:
|
||||
|
||||
- Narratio supports one current configuration and state model with no legacy
|
||||
runtime fallbacks;
|
||||
- the session manifest is the only durable local progress ledger;
|
||||
- analyze checkpoints each successful artifact, preserves valid completed work,
|
||||
and resumes from failed, stale, or missing work in dependency order;
|
||||
- `cleanup` is the final canonical stage and cannot make a committed publish
|
||||
appear to have failed;
|
||||
- remote locks are independent per-artifact objects, and locked canonical
|
||||
artifacts are carried forward across new current snapshots;
|
||||
- configuration tests protect semantic rules rather than diagnostic
|
||||
choreography;
|
||||
- ordinary logs and command summaries provide proportionate diagnostics without
|
||||
participating in correctness; and
|
||||
- the combined simplification is eligible for a minor release because the
|
||||
removed compatibility behavior was already deprecated and removable.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None. The policy and target-state decisions required for a later feature
|
||||
roadmap and staged implementation plan are resolved by this audit.
|
||||
@@ -1,273 +0,0 @@
|
||||
# File Operations Simplification
|
||||
|
||||
## Status
|
||||
|
||||
Accepted feature roadmap. This document defines the desired filesystem policy
|
||||
and target state. It is not an implementation plan and does not describe
|
||||
implemented behavior. It refines the filesystem conclusions in
|
||||
[`audit.md`](audit.md).
|
||||
|
||||
## Goal
|
||||
|
||||
Make Narratio's filesystem code proportionate to its real operating model:
|
||||
single-invocation work under a session lock in a trusted, group-owned workspace
|
||||
containing intentionally shareable D&D material. Use the Go standard library
|
||||
directly for ordinary filesystem work and retain custom code only where it
|
||||
expresses a concrete Narratio requirement.
|
||||
|
||||
The change should materially reduce production code, platform-specific code,
|
||||
test machinery, full-tree copying, and the number of filesystem guarantees that
|
||||
maintainers must preserve.
|
||||
|
||||
## Underlying Issue
|
||||
|
||||
`internal/fileops` has accumulated a general confined-filesystem abstraction,
|
||||
two directory-promotion implementations, adversarial race hooks and inode
|
||||
identity rechecks, handle-relative no-replace promotion, explicit file and
|
||||
directory syncing, platform capability errors, universal symlink and regular-
|
||||
file restrictions, and tests for each mechanism.
|
||||
|
||||
Those mechanisms are internally coherent, but they solve a stronger problem
|
||||
than Narratio has. Narratio does not operate on a hostile multi-writer tree, and
|
||||
its ordinary workspace data is not private. The session lock prevents normal
|
||||
concurrent mutation by Narratio, while run IDs make durable extract destinations
|
||||
unique. Treating every current mechanism as a permanent policy has therefore
|
||||
created a self-reinforcing contract: implementation detail drives tests and
|
||||
documentation, which in turn appear to require the implementation detail.
|
||||
|
||||
This roadmap resets that contract around required outcomes. It does not assume
|
||||
that a replacement must preserve identical mechanics or failure behavior.
|
||||
|
||||
## Policy Decisions
|
||||
|
||||
### Operating and threat model
|
||||
|
||||
- Campaign and session workspaces are trusted operational inputs, not hostile
|
||||
filesystems. Narratio does not defend ordinary reads and writes against an
|
||||
attacker racing directory entries during an invocation.
|
||||
- Workspace transcripts, artifacts, manifests, logs, and configuration are
|
||||
intended to be shareable. World readability is acceptable. Group-owned,
|
||||
group-writable operation remains a requirement.
|
||||
- API keys are the exception. Narratio must not write credentials into runtime
|
||||
artifacts or diagnostics, and any filesystem path that can contain API-key
|
||||
material retains appropriately strict handling.
|
||||
- Narratio must still prevent accidental destructive action or restore outside
|
||||
the selected application-owned root. Trusted input does not justify an
|
||||
unbounded delete or an escaped restore target.
|
||||
|
||||
### Guarantees to retain
|
||||
|
||||
- Corruption-sensitive single-file updates use same-directory temporary files
|
||||
and replacement so readers do not observe partially written content. This
|
||||
applies to such state as manifests and generated configuration; it is an
|
||||
atomic-visibility requirement, not a power-loss durability promise.
|
||||
- Files downloaded from remote storage are completely received and validated
|
||||
before they become the canonical local file.
|
||||
- Adapter and configuration input remains bounded by centralized, discoverable
|
||||
constants. Limits remain deliberately far above expected workloads and
|
||||
produce unmistakable errors naming both the limit and the enforcing adapter.
|
||||
- Workspace files and directories preserve the repository's group-owned,
|
||||
group-writable mode policy, including after temporary-file replacement or
|
||||
directory rename.
|
||||
- Cleanup and restore operate relative to an explicitly opened root and reject
|
||||
targets outside that scope. Recursive removal uses the standard-library root
|
||||
primitive available at the maintained Go baseline.
|
||||
- Restore treats leaf or ancestor symlinks as non-regular conflicts rather than
|
||||
following them to classify or overwrite a target. This is a restore boundary,
|
||||
not a universal rule for ordinary workspace access.
|
||||
- Staging cleanup is limited to paths that Narratio created and verified as
|
||||
descendants of the applicable invocation or session workspace.
|
||||
|
||||
### Guarantees to retire
|
||||
|
||||
- Narratio does not promise that a successful write or directory install has
|
||||
been forced through every operating-system and storage cache before return.
|
||||
File and directory `fsync` calls, directory-sync capability probes, and
|
||||
unsupported-durability errors are not part of the target contract.
|
||||
- Narratio does not defend against a concurrent adversary replacing a file or
|
||||
directory after it has been inspected. Inode identity snapshots, `SameFile`
|
||||
rechecks, and race-injection hooks are not part of the target contract.
|
||||
- Ordinary workspace operations do not universally reject symlinks or require
|
||||
every source to be a regular file. Components may validate a concrete external
|
||||
contract where needed, but there is no package-wide no-follow policy.
|
||||
- Directory installation does not require a custom atomic no-replace primitive
|
||||
or an exact cross-platform race guarantee. The session lock, absent-destination
|
||||
check, and unique run ID are the intended concurrency controls.
|
||||
- “Durable” in Narratio documentation means managed persistent application
|
||||
state. It does not mean guaranteed survival of every acknowledged update
|
||||
across immediate power loss.
|
||||
|
||||
## Required Changes
|
||||
|
||||
### Narrow the filesystem API
|
||||
|
||||
Replace the broad `internal/fileops` facade with direct `os`, `io`, and
|
||||
`path/filepath` operations at ordinary call sites. Retain only small helpers
|
||||
that enforce a shared product rule rather than wrapping the standard library.
|
||||
Expected shared concerns are:
|
||||
|
||||
- corruption-resistant single-file replacement with explicit final mode;
|
||||
- bounded reads or copies with centralized limits and contextual errors;
|
||||
- application workspace file and directory modes; and
|
||||
- narrowly scoped root-relative cleanup and restore operations.
|
||||
|
||||
The target is not a one-for-one rewrite of existing exported functions. Remove
|
||||
unused or single-purpose wrappers when the calling package can express the
|
||||
operation clearly with the standard library. Keep platform-specific code only
|
||||
if it is necessary for the small atomic-file replacement helper to provide the
|
||||
supported cross-platform behavior.
|
||||
|
||||
Do not add a general filesystem framework. Same-directory atomic replacement is
|
||||
a generic abstraction, but a small local helper is the preferred target unless
|
||||
implementation research identifies a maintained cross-platform dependency that
|
||||
removes more code than it adds and preserves the required mode behavior.
|
||||
|
||||
### Simplify ordinary reads, writes, and copies
|
||||
|
||||
Use ordinary standard-library path operations for trusted workspace data.
|
||||
Preserve contextual errors and the existing centralized safety limits, but do
|
||||
not route every access through an `os.Root`, reopen files to prove identity, or
|
||||
reject symlinks merely because the current facade does so.
|
||||
|
||||
Callers that consume a defined external format may still validate type, size,
|
||||
checksum, structure, or placement because their own contract requires it. Those
|
||||
checks should live with the consumer and must not reconstitute a universal
|
||||
FileOps policy indirectly.
|
||||
|
||||
### Replace file updates without power-loss machinery
|
||||
|
||||
Use a minimal same-directory temporary-file flow for corruption-sensitive file
|
||||
updates: create, write, close, apply the intended mode, and replace the canonical
|
||||
path. Ensure temporary files are removed after pre-commit failures. Preserve
|
||||
context cancellation checks where a caller can still abandon an update before
|
||||
replacement.
|
||||
|
||||
Remove file and parent-directory syncing, sync-related platform probes and
|
||||
sentinel errors, and tests that exist only to assert power-loss guarantees. If a
|
||||
small operating-system-specific replace shim is still required, keep it private
|
||||
to this helper and test its observable replacement behavior rather than its
|
||||
internal syscall sequence.
|
||||
|
||||
### Simplify download installation
|
||||
|
||||
Keep remote downloads in a temporary file until transfer, size, checksum, and
|
||||
format validation succeeds. Install the validated file using the same minimal
|
||||
replacement helper or a direct rename when the destination is known absent.
|
||||
Remove standalone download-install APIs that have no production caller and
|
||||
avoid duplicating replacement policy between download and ordinary writes.
|
||||
|
||||
### Replace Notarius directory promotion
|
||||
|
||||
Have Notarius write its output bundle to a unique staging directory within the
|
||||
invocation workspace. Validate that the reported bundle is inside the requested
|
||||
output root and satisfies the Notarius integration contract. Normalize required
|
||||
workspace modes, confirm that the unique durable destination is absent, and use
|
||||
`os.Rename` to move the validated directory directly to that destination.
|
||||
|
||||
Delete the production and test-only directory tree-copy implementations,
|
||||
promotion hooks, handle-relative no-replace code, directory syncing, and their
|
||||
platform files. A destination collision must return a clear ordinary error; it
|
||||
does not require adversarial race recovery.
|
||||
|
||||
The successfully renamed bundle becomes the sole retained artifact tree. The
|
||||
pre-promotion Notarius tree is not duplicated for diagnostics. Existing receipt
|
||||
and log records remain the troubleshooting evidence, and failed staging trees
|
||||
are cleaned up only after their scope is verified.
|
||||
|
||||
### Keep destructive and restore operations root-scoped
|
||||
|
||||
Retain `os.Root` at boundaries where containment is the requirement. Use the
|
||||
maintained Go version's root-relative recursive removal for cleanup. Continue to
|
||||
require proper-descendant relationships and application-owned eligibility
|
||||
before destructive operations.
|
||||
|
||||
Rework restore classification and installation around a session root. Inspect
|
||||
targets without following leaf or ancestor symlinks, classify such targets as
|
||||
non-regular conflicts in both force and non-force plans, and hash only a target
|
||||
verified within that root. Do not add inode-stability or hostile-race rechecks.
|
||||
|
||||
### Preserve group-owned workspace operation and protect secrets
|
||||
|
||||
Centralize the intended workspace modes in one discoverable location and apply
|
||||
them consistently to created directories, ordinary files, atomic replacements,
|
||||
downloads, and renamed Notarius bundles. Do not tighten general workspace
|
||||
permissions on the assumption that transcripts or artifacts are sensitive.
|
||||
|
||||
Audit API-key ingress separately from general workspace operations. Secret
|
||||
values must remain out of files unless an existing explicit credential contract
|
||||
requires a file, must never enter logs or errors, and must not inherit permissive
|
||||
workspace modes. The simplification must not weaken existing credential
|
||||
redaction or environment handling.
|
||||
|
||||
### Align documentation and tests with outcomes
|
||||
|
||||
Revise the canonical architecture, operations, troubleshooting, and focused
|
||||
internal documents to remove power-loss durability, universal confinement,
|
||||
universal no-follow, and atomic directory-promotion claims. Keep volatile modes,
|
||||
paths, limits, and operational behavior in their existing canonical owners
|
||||
rather than duplicating them across documents.
|
||||
|
||||
Delete tests for removed algorithms, syscall ordering, race hooks, directory-
|
||||
sync capability behavior, and guarantees no longer offered. Retain or add
|
||||
focused tests for:
|
||||
|
||||
- successful and failed atomic file replacement on every supported platform;
|
||||
- cleanup of temporary files before replacement;
|
||||
- bounded input and clear adapter-specific limit errors;
|
||||
- group-owned file and directory modes;
|
||||
- validated download installation;
|
||||
- validated Notarius bundle rename, collision, and cleanup behavior;
|
||||
- root-scoped cleanup and refusal of escaped destructive targets;
|
||||
- restore handling of leaf and ancestor symlinks; and
|
||||
- API-key redaction and any credential-file permission contract.
|
||||
|
||||
Tests should assert externally meaningful results and failure boundaries, not
|
||||
recreate the removed implementation through new private hooks.
|
||||
|
||||
## Compatibility and Migration
|
||||
|
||||
This work changes internal guarantees rather than CLI, configuration, manifest,
|
||||
or external artifact schemas. Existing valid workspaces should remain usable.
|
||||
Users may observe fewer failures on filesystems that cannot sync directories,
|
||||
faster extract installation, less temporary disk consumption, and ordinary
|
||||
symlink behavior outside explicitly scoped restore and destructive operations.
|
||||
|
||||
The removal of the duplicate pre-promotion Notarius output tree is an intentional
|
||||
operational change and must be reflected in the operations and troubleshooting
|
||||
documentation. No compatibility layer should preserve the old FileOps API solely
|
||||
for internal callers; migrate callers and delete obsolete surfaces together.
|
||||
|
||||
## Target End State
|
||||
|
||||
At completion:
|
||||
|
||||
- ordinary workspace file operations are clear standard-library code located
|
||||
near their domain owners;
|
||||
- shared filesystem code is small and limited to atomic file replacement,
|
||||
bounded I/O and modes, plus explicitly root-scoped destructive or restore
|
||||
helpers;
|
||||
- directory promotion, tree-copy duplication, adversarial race defenses,
|
||||
directory syncing, and unsupported-durability machinery no longer exist;
|
||||
- extract validates and renames one Notarius staging bundle without copying the
|
||||
complete tree or retaining a duplicate source tree;
|
||||
- manifests and other corruption-sensitive files are never intentionally
|
||||
exposed partially, without claiming power-loss durability;
|
||||
- accidental out-of-root deletion and restore remain prevented;
|
||||
- group-owned workspaces remain first-class, and API keys remain the only data
|
||||
requiring secret-specific filesystem treatment;
|
||||
- tests cover the reduced observable contract rather than removed mechanisms;
|
||||
and
|
||||
- canonical documentation accurately distinguishes persistent application state,
|
||||
atomic visibility, trusted-workspace assumptions, and destructive-operation
|
||||
confinement.
|
||||
|
||||
## Out of Scope
|
||||
|
||||
- Changing CLI commands, configuration schemas, artifact schemas, or manifest
|
||||
formats.
|
||||
- Adding multi-process writes to one session or removing the session lock.
|
||||
- Defending trusted workspaces against malicious concurrent mutation.
|
||||
- Adding encryption or restrictive permissions for shareable campaign data.
|
||||
- Redesigning remote publish commit ordering or object-store concurrency.
|
||||
- Adding user-configurable size limits in this feature; constants should remain
|
||||
easy to promote to configuration later.
|
||||
@@ -1,431 +0,0 @@
|
||||
# Workstream 10 Implementation Plan — Quality, Testability, And Documentation Alignment
|
||||
|
||||
## Status
|
||||
|
||||
Planned implementation for Workstream 10, **Quality, testability, and
|
||||
documentation alignment**, in [`refactoring.md`](refactoring.md). Workstreams 1
|
||||
through 9 are complete prerequisites. This is the final actionable workstream
|
||||
in the consolidated refactoring roadmap; it closes cross-cutting gaps that were
|
||||
not already completed alongside the behavior-owning workstreams.
|
||||
|
||||
## Authoritative Scope And Fixed Decisions
|
||||
|
||||
Workstream 10 in [`refactoring.md`](refactoring.md) is authoritative. Its
|
||||
supporting first-pass findings are H07, H08, H12, H13, H17, H18, H23, and H25
|
||||
in [`audit.md`](audit.md). The corresponding contract decisions are C01 through
|
||||
C06 in [`audit-decisions.md`](audit-decisions.md); their full rationale remains
|
||||
in [`contract-audit.md`](contract-audit.md). Read those cited sections rather
|
||||
than re-auditing unrelated hypotheses.
|
||||
|
||||
Most of Workstream 10's risk coverage and documentation alignment landed with
|
||||
Workstreams 1 through 9. Preserve that completed work. The remaining decisions
|
||||
are:
|
||||
|
||||
- Do not reopen completed product, compatibility, filesystem, manifest,
|
||||
analyze, configuration, publication, lock, or cleanup decisions. This
|
||||
workstream changes internal testability and test diagnostics without changing
|
||||
CLI, configuration, artifact, manifest, integration, publish, restore, or
|
||||
cleanup behavior.
|
||||
- Remove the seven remaining mutable application function seams:
|
||||
`readSecretsDirectoryEntries`, `loadPipelineConfigFn`,
|
||||
`downloadObjectToTempFn`, `newObjectStoreFromConfigFn`,
|
||||
`discoverRemoteCurrentStateFn`, `buildRestorePlanFn`, and
|
||||
`executeRestorePlanFn`. Compiled regular-expression values, canonical stage
|
||||
relations, and fixed command-name data are not collaborator seams and are not
|
||||
part of this removal.
|
||||
- Use one package-private, value-owned `commandDependencies` set for these
|
||||
collaborators. A normalization function supplies production defaults for nil
|
||||
fields. Each command invocation receives its own value; there is no mutable
|
||||
default singleton, context-carried service locator, dependency-injection
|
||||
framework, or new public interface. Keep runner `invocationDependencies`
|
||||
separate because it owns a different lifecycle.
|
||||
- Preserve exported command entry points and their existing signatures.
|
||||
Production wrappers use normalized defaults. Private dependency-aware entry
|
||||
points let tests supply only the external or phase boundary they control.
|
||||
Thread the dependency value explicitly through application-owned command,
|
||||
configuration-loading, secret-loading, object-store, and restore helpers.
|
||||
- Tests must never replace or restore package-level functions. Use dependency
|
||||
values, real temporary filesystems, `storage.FakeBackend`, and existing
|
||||
controlled subprocess helpers at their durable boundaries. Process
|
||||
environment mutation remains permitted where credential behavior itself is
|
||||
under test, but such tests must remain non-parallel and restore state through
|
||||
`t.Setenv` or the existing focused helper.
|
||||
- Add `github.com/google/go-cmp/cmp` only for the audit's selected large
|
||||
structural comparisons: composed configuration values, analyze plans/state,
|
||||
restore plans/actions, and publish output plans/mappings. Preserve nil versus
|
||||
empty and ordering semantics unless a domain contract explicitly equates
|
||||
them. Use direct comparison or `slices.Equal` for simple comparable values;
|
||||
do not mechanically replace every `reflect.DeepEqual` call.
|
||||
- Apply `slices` or `maps` in production only when code touched by this
|
||||
workstream performs a genuinely shallow clone, membership check, or
|
||||
deterministic key operation with identical nil, empty, and ordering
|
||||
behavior. Do not conduct a repository-wide style rewrite and do not replace
|
||||
domain normalization, deep copies, provenance merges, or topological order.
|
||||
- Retain the focused behavior coverage already established for atomic file
|
||||
visibility, bounded reads and adapter diagnostics, group modes, validated
|
||||
downloads, Notarius rename, destructive confinement, restore symlink
|
||||
conflicts, secret protection, session-only progress, per-artifact analyze
|
||||
checkpoints, planning/execution parity, remote-lock carry-forward, and
|
||||
idempotent cleanup. Delete only dead helpers and reset choreography associated
|
||||
with the removed mutable seams; do not delete a current-contract test merely
|
||||
to reduce test lines.
|
||||
- Update only canonical internal documentation whose implemented dependency or
|
||||
test boundary changes. Because public behavior is unchanged, do not churn
|
||||
CLI, configuration, operations, troubleshooting, integration, examples, or
|
||||
release documentation unless implementation uncovers a factual inaccuracy in
|
||||
its owning document.
|
||||
|
||||
## Instructions For Every Stage
|
||||
|
||||
Before each stage:
|
||||
|
||||
1. Read `docs/development.md`, all documents under `docs/policy/`, this plan,
|
||||
Workstream 10 in [`refactoring.md`](refactoring.md), and only its cited audit
|
||||
sections. Read [`../internal/overview.md`](../internal/overview.md) and, for
|
||||
restore changes, [`../internal/command-restore.md`](../internal/command-restore.md).
|
||||
2. Inspect the affected production code and focused tests. Prefer the codebase
|
||||
knowledge graph for Go definitions and call paths; use text search for
|
||||
mutable assignments, test reset choreography, documentation, fixtures, and
|
||||
final removal checks.
|
||||
3. Confirm worktree state and preserve unrelated changes. Do not implement
|
||||
release work or any deferred/no-action audit hypothesis.
|
||||
|
||||
During each stage, keep dependencies package-private and invocation-owned,
|
||||
preserve acyclic package direction, keep default tests deterministic and
|
||||
offline, and retain credential redaction and filesystem safety. Prefer behavior
|
||||
assertions over collaborator choreography. Run `gofmt` on changed Go files,
|
||||
`go mod tidy` when module state changes, and `git diff --check`. Commit only a
|
||||
completed stage with a short plain-English message, and update a canonical
|
||||
internal document in the same stage only when its owned behavior changes.
|
||||
|
||||
## Stage 1 — Replace Configuration And Secret-Loading Globals
|
||||
|
||||
**Status:** Completed
|
||||
|
||||
### Goal
|
||||
|
||||
Establish the command-scoped dependency value and remove global substitution
|
||||
from configuration and API-key loading.
|
||||
|
||||
### Required Work
|
||||
|
||||
1. Add a package-private `commandDependencies` type in `internal/app` with
|
||||
typed function fields for pipeline loading, temporary object download,
|
||||
secrets-directory reading, object-store construction, remote-current
|
||||
discovery, restore-plan construction, and restore-plan execution. Add a
|
||||
value-returning normalizer that fills nil fields with the current production
|
||||
functions. Do not store a normalized instance in a package variable.
|
||||
2. Add dependency-aware private variants of the configuration-loading path:
|
||||
`loadCommandConfig`, `loadPipelineCampaignConfig`, and `loadPipelineConfig`
|
||||
should receive or delegate to an explicit dependency value. Preserve local
|
||||
session precedence, remote fallback, source provenance, bounded temporary
|
||||
download behavior, and idempotent temporary-file cleanup.
|
||||
3. Add a dependency-aware private secret loader used by focused tests and by
|
||||
later command composition. Production callers continue through a wrapper
|
||||
using normalized defaults. Preserve directory/file privacy checks, symlink
|
||||
rejection, bounded reads, existing-environment precedence, redaction, and
|
||||
the ancestor-replacement failure case.
|
||||
4. Remove `loadPipelineConfigFn`, `downloadObjectToTempFn`, and
|
||||
`readSecretsDirectoryEntries`. Rewrite `config_loader_test.go`,
|
||||
loader-focused cases in `remote_session_test.go`, and `secrets_env_test.go`
|
||||
to pass copied dependency values instead of saving, replacing, and restoring
|
||||
globals. Leave command-routing remote-session cases for the command stage
|
||||
that owns their entry point. Retain tests at the narrowest existing behavior
|
||||
boundary; do not reproduce the loader's internal call sequence with mocks.
|
||||
5. Keep production command behavior unchanged while this foundation is
|
||||
introduced. It is acceptable for the object-store and restore fields to use
|
||||
their production defaults until Stages 2 through 4 migrate their callers.
|
||||
|
||||
### Verification And Exit Criteria
|
||||
|
||||
Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/app -run 'Config|RemoteSession|Secret'
|
||||
go test -race ./internal/app -run 'Config|RemoteSession|Secret'
|
||||
git diff --check
|
||||
```
|
||||
|
||||
The three named mutable globals are absent, remote/local configuration behavior
|
||||
and secret protections remain covered, and tests supply collaborators through
|
||||
local dependency values.
|
||||
|
||||
## Stage 2 — Carry Dependencies Through Pipeline Commands
|
||||
|
||||
**Status:** Completed
|
||||
|
||||
### Goal
|
||||
|
||||
Carry one dependency value through the pipeline-oriented command paths,
|
||||
including remote session fallback, without changing their public API.
|
||||
|
||||
### Required Work
|
||||
|
||||
1. Add a private dependency-aware execution dispatcher alongside `Execute`.
|
||||
`Execute` retains its signature and uses normalized production dependencies.
|
||||
Extend the existing private pipeline-executor path rather than creating a
|
||||
second command switch or an application framework.
|
||||
2. Thread `commandDependencies` through `run`, `regenerate-artifacts`,
|
||||
`run-stage`, `analyze`, `publish`, `plan`, and `status` paths and the shared
|
||||
configuration loader they use. Preserve the independent `pipelineExecutor`
|
||||
seam and pass both values explicitly where required.
|
||||
3. Make remote session fallback initialize its object store through the
|
||||
dependency value and preserve the rule that secrets load before the factory.
|
||||
Public command wrappers continue to use production defaults.
|
||||
4. Rewrite the assembled pipeline-command tests and only those remote-session
|
||||
tests reached through pipeline commands to call the dependency-aware
|
||||
dispatcher with a fake store. Remove their global object-store
|
||||
replacement/reset choreography. Leave remote-session cases reached through
|
||||
`session` commands for Stage 3. It is acceptable for the factory global
|
||||
itself to remain temporarily for restore and operator callers until Stages 3
|
||||
and 4 migrate them.
|
||||
5. Preserve command syntax, exit classification, stdout/stderr behavior,
|
||||
configuration discovery, artifact selection, executor injection, and remote
|
||||
session provenance. Do not move adapter construction into configuration or
|
||||
storage packages.
|
||||
|
||||
### Verification And Exit Criteria
|
||||
|
||||
Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/app -run 'Run|Stage|Analyze|Publish|Plan|Status|RemoteSession|Command'
|
||||
go test -race ./internal/app -run 'Run|Stage|Analyze|Publish|Plan|Status|RemoteSession'
|
||||
git diff --check
|
||||
```
|
||||
|
||||
Pipeline commands use invocation-owned dependencies, production defaults still
|
||||
select the real storage constructor, and focused tests remain offline.
|
||||
|
||||
## Stage 3 — Carry Dependencies Through Operator Commands
|
||||
|
||||
**Status:** Completed
|
||||
|
||||
### Goal
|
||||
|
||||
Migrate session and maintenance commands to the same invocation-owned command
|
||||
dependency path before removing the final shared storage seam.
|
||||
|
||||
### Required Work
|
||||
|
||||
1. Thread `commandDependencies` through the `session` dispatcher, session
|
||||
initialization and validation, artifact listing, lock listing/add/remove,
|
||||
and session-scoped or cache-aware clean helpers. Exported command functions
|
||||
remain production-default wrappers.
|
||||
2. Update the dependency-aware top-level dispatcher so every operator command
|
||||
reached through `Execute` receives the invocation's value. Keep local-only
|
||||
paths independent of remote construction; do not initialize an object store
|
||||
merely because a dependency value exists.
|
||||
3. Rewrite `object_store_test.go`, `operator_helpers_test.go`, affected
|
||||
session/clean tests, and remote-session cases reached through the `session`
|
||||
dispatcher to supply a local object-store factory. Keep complete CLI routing
|
||||
tests at the private dispatcher and narrow construction-order tests at
|
||||
`newCommandObjectStore`. Remove original-value capture and reset boilerplate
|
||||
in the migrated tests.
|
||||
4. Preserve secret-before-factory ordering, local/remote session initialization,
|
||||
validation findings, artifact availability, per-source remote lock behavior,
|
||||
cleanup scope, output, and errors. Continue using `storage.FakeBackend` for
|
||||
remote behavior.
|
||||
5. Leave only restore callers on the temporary global storage seam. Do not
|
||||
remove it until Stage 4 can update restore and its tests atomically.
|
||||
|
||||
### Verification And Exit Criteria
|
||||
|
||||
Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/app -run 'ObjectStore|SessionInit|SessionValidate|Artifacts|Locks|Clean|Operator'
|
||||
go test -race ./internal/app -run 'ObjectStore|SessionInit|SessionValidate|Artifacts|Locks|Clean'
|
||||
git diff --check
|
||||
```
|
||||
|
||||
All non-restore commands use invocation-owned dependencies, their tests mutate
|
||||
no factory global, and remote behavior remains deterministic and offline.
|
||||
|
||||
## Stage 4 — Replace Restore Phase Globals With Explicit Dependencies
|
||||
|
||||
**Status:** Completed
|
||||
|
||||
### Goal
|
||||
|
||||
Make restore sequencing testable per invocation without global phase hooks or a
|
||||
new restore framework.
|
||||
|
||||
### Required Work
|
||||
|
||||
1. Keep `Restore` as the public production wrapper and add one private
|
||||
dependency-aware restore entry point. Normalize dependencies once at that
|
||||
boundary and use its object-store constructor, remote-current discovery,
|
||||
plan builder, and plan executor throughout the invocation.
|
||||
2. Remove `newObjectStoreFromConfigFn`, `discoverRemoteCurrentStateFn`,
|
||||
`buildRestorePlanFn`, and `executeRestorePlanFn`. Call the concrete
|
||||
production functions through the normalized dependency value; do not add
|
||||
interfaces around pure phase functions or move restore orchestration out of
|
||||
`internal/app`.
|
||||
3. Rewrite `restore_test.go`, `restore_execution_test.go`, and
|
||||
`restore_recovery_test.go` to pass local dependencies. Consolidate repeated
|
||||
dependency construction into one small test helper that returns a fresh
|
||||
value and never mutates package state.
|
||||
4. Preserve the observable restore boundaries: dry-run performs no local
|
||||
installation; non-dry-run planning occurs while holding the session lock;
|
||||
conflicts do not execute; the incomplete marker brackets installation;
|
||||
reports reflect failure or success; manifest installation remains last; and
|
||||
lock release errors remain joined with an earlier failure.
|
||||
5. Prefer real `buildRestorePlan` and `executeRestorePlan` with
|
||||
`storage.FakeBackend` where the test is about restore results or filesystem
|
||||
state. Stub a phase only when the phase call or its timing is the behavior
|
||||
being tested, such as proving the transition lock is already held.
|
||||
6. Update `docs/internal/command-restore.md` only if its description of phase
|
||||
composition or test seams is no longer accurate. Do not change the operator
|
||||
restore contract.
|
||||
|
||||
### Verification And Exit Criteria
|
||||
|
||||
Run:
|
||||
|
||||
```sh
|
||||
go test ./internal/app -run 'Restore'
|
||||
go test -race -shuffle=on -count=3 ./internal/app -run 'Restore'
|
||||
git diff --check
|
||||
```
|
||||
|
||||
No command has a mutable production function seam, restore transaction ordering
|
||||
and recovery outcomes remain covered, and restore tests can execute
|
||||
independently without save/restore choreography.
|
||||
|
||||
## Stage 5 — Improve Selected Large Structural Test Diagnostics
|
||||
|
||||
**Status:** Completed
|
||||
|
||||
### Goal
|
||||
|
||||
Make failures in the four audit-selected large-value test categories readable
|
||||
without imposing a repository-wide assertion style.
|
||||
|
||||
### Required Work
|
||||
|
||||
1. Add `github.com/google/go-cmp/cmp` as a direct test dependency and update
|
||||
`go.mod` and `go.sum` with the Go toolchain. Do not add an assertion framework
|
||||
or helper wrapper around `cmp.Diff`.
|
||||
2. Replace existing whole-value `reflect.DeepEqual` assertions with
|
||||
`cmp.Diff` where a field-level diff is materially useful in these files:
|
||||
- composed/effective configuration comparisons in
|
||||
`internal/config/pipeline_composition_test.go` and
|
||||
`internal/config/pipeline_profiles_test.go`;
|
||||
- complete analyze plans, artifact-state maps, or stage records in
|
||||
`internal/stage/analyze_plan_test.go`,
|
||||
`internal/stage/analyze_reconciliation_test.go`, and
|
||||
`internal/app/analyze_checkpoint_recovery_test.go`;
|
||||
- restore action collections or complete plans in
|
||||
`internal/app/restore_plan_test.go`; and
|
||||
- complete output outcomes, declarations, or mappings in
|
||||
`internal/stage/publish_output_plan_test.go`.
|
||||
3. Keep direct field assertions that intentionally isolate one invariant. Use
|
||||
`slices.Equal` for simple ordered slices in the touched files where it is
|
||||
clearer; retain `reflect.DeepEqual` when its value is small and the mismatch
|
||||
is already obvious. Do not expand this stage to every test package.
|
||||
4. Do not use `cmpopts.EquateEmpty`, `cmp.AllowUnexported`, blanket ignore
|
||||
options, or custom comparers merely to force a passing comparison. Compare
|
||||
the same observable values as before and preserve distinctions in nil,
|
||||
empty, order, timestamps, and provenance.
|
||||
5. Run `go mod tidy` and confirm the dependency is used only by tests. Apply
|
||||
`slices` and `maps` to production touched in Stages 1–4 only where the fixed
|
||||
decisions above permit an identical shallow operation; do not manufacture a
|
||||
production change to demonstrate their use.
|
||||
|
||||
### Verification And Exit Criteria
|
||||
|
||||
Run:
|
||||
|
||||
```sh
|
||||
go mod tidy
|
||||
go test ./internal/config ./internal/stage ./internal/app
|
||||
go test -race ./internal/config ./internal/stage ./internal/app
|
||||
git diff --check
|
||||
```
|
||||
|
||||
The selected large comparisons emit useful structural diffs, simple assertions
|
||||
remain simple, module state is tidy, and no broad assertion-style migration has
|
||||
occurred.
|
||||
|
||||
## Stage 6 — Verify Coverage, Align Internal Documentation, And Close The Roadmap
|
||||
|
||||
**Status:** Completed
|
||||
|
||||
### Goal
|
||||
|
||||
Prove that the reduced test and documentation system protects the final
|
||||
refactored architecture, then close Workstream 10 and the consolidated roadmap.
|
||||
|
||||
### Required Work
|
||||
|
||||
1. Search production and tests for all seven retired function-seam names and
|
||||
for assignments that replace application functions. Remove any remaining
|
||||
dead wrappers, original-value captures, reset helpers, or cleanup callbacks
|
||||
whose sole purpose was global substitution. Do not remove compiled regexes,
|
||||
canonical data tables, or legitimate local function values.
|
||||
2. Review the existing focused owners—not the entire suite indiscriminately—to
|
||||
confirm retained coverage for:
|
||||
- atomic visibility and bounded reads in `internal/fileops` and storage
|
||||
adapter tests;
|
||||
- POSIX group modes in `internal/fileops/modes_posix_test.go`;
|
||||
- validated storage downloads and Notarius promotion in storage adapter and
|
||||
extract tests;
|
||||
- destructive confinement and restore symlink conflicts in fileops,
|
||||
cleanup, and restore tests;
|
||||
- credential privacy and redaction in secrets, logging, subprocess, and
|
||||
adapter tests;
|
||||
- session-only progress and per-artifact recovery in manifest, stage, and
|
||||
application checkpoint tests;
|
||||
- planning/execution parity in `internal/app/run_control_test.go`;
|
||||
- remote-lock carry-forward in publish planning/execution tests; and
|
||||
- idempotent final cleanup in stage and application cleanup tests.
|
||||
Add a test only if a listed consequential boundary is genuinely absent;
|
||||
consolidate or delete only demonstrably redundant mechanism coverage.
|
||||
3. Review `docs/policy/architecture.md`, `docs/internal/overview.md`, focused
|
||||
internal component documents, and their test inventories for current
|
||||
accuracy. Remove stale implementation claims and duplicate volatile detail;
|
||||
retain links to the canonical CLI, configuration, operations, integration,
|
||||
and release owners rather than copying those contracts.
|
||||
4. Confirm no current-behavior document treats a retired compatibility path,
|
||||
run manifest, cleanup obligation, central lock manifest, custom directory
|
||||
promotion, or mutable test hook as supported behavior. Historical roadmap
|
||||
and audit records remain unchanged.
|
||||
5. Run the complete validation matrix below. Use a fresh temporary directory
|
||||
for the release-asset build and remove it afterward; do not create a release
|
||||
note, tag, or publication. Record native-platform limitations exactly as in
|
||||
`docs/development.md` rather than treating cross-compilation as runtime
|
||||
evidence.
|
||||
6. Only after every check passes, mark Stages 1–6 in this plan and Workstream 10
|
||||
in `refactoring.md` **Complete**. Confirm every numbered workstream is then
|
||||
complete. Do not delete the audits, roadmap, or implementation plan and do
|
||||
not prepare or publish a release.
|
||||
|
||||
### Verification And Exit Criteria
|
||||
|
||||
At minimum run:
|
||||
|
||||
```sh
|
||||
go mod tidy -diff
|
||||
test -z "$(gofmt -l $(git ls-files '*.go'))"
|
||||
go test -count=1 ./...
|
||||
go test -race -count=1 ./...
|
||||
go test -race -shuffle=on -count=3 ./...
|
||||
go vet ./...
|
||||
go build ./...
|
||||
go test -count=1 ./internal/doccheck
|
||||
go test -count=1 ./internal/config -run '^TestExamplesLoadAndValidate$'
|
||||
git diff --check
|
||||
```
|
||||
|
||||
Also run `scripts/build-release-assets.sh v0.0.0-test <fresh-absolute-temp-dir>`
|
||||
and delete only that verified temporary directory afterward. The workstream is
|
||||
complete when application collaborators are invocation-scoped, selected large
|
||||
test failures are diagnostic, retained high-risk behavior is credibly covered,
|
||||
canonical documentation describes the implemented reduced contracts, all ten
|
||||
workstreams are complete, and the worktree contains no release artifact.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None. The audits, completed prerequisite workstreams, and fixed decisions above
|
||||
make the remaining quality and documentation work decision-complete.
|
||||
@@ -1,395 +0,0 @@
|
||||
# Reusable Library Extraction Audit
|
||||
|
||||
## Status
|
||||
|
||||
Complete. This audit reviews the Narratio code at `af5fbc1c4f8d` and records
|
||||
candidate-extraction decisions for future refactoring. It does not authorize an
|
||||
extraction, create a new module, or change application behavior.
|
||||
|
||||
This is the third maintainability pass. It complements the two earlier passes:
|
||||
|
||||
1. [`audit.md`](audit.md) asks whether implementation can be simplified while
|
||||
preserving current behavior and contracts.
|
||||
2. [`contract-audit.md`](contract-audit.md) asks whether apparent contracts can
|
||||
reasonably be changed to permit substantially simpler behavior.
|
||||
3. This document asks whether code that Narratio no longer needs should instead
|
||||
be preserved as a generic, independently reusable library.
|
||||
|
||||
The question in this pass is deliberately separate from whether Narratio should
|
||||
continue using an extracted library. Code may be valuable in a more demanding
|
||||
application while remaining the wrong dependency for Narratio.
|
||||
|
||||
## Method and Decision Standard
|
||||
|
||||
The review inventoried Narratio's internal packages, concentrated on mechanisms
|
||||
whose contracts can be stated without D&D or pipeline terminology, inspected
|
||||
their call sites and tests, and compared the most plausible candidates with the
|
||||
actual implementations in the neighboring Notarius and WeatherReporter
|
||||
repositories. It also checked the Go standard library and representative
|
||||
maintained or emerging third-party implementations before treating custom code
|
||||
as a library opportunity.
|
||||
|
||||
A candidate is suitable for extraction only when all of the following are true:
|
||||
|
||||
- its contract can be explained without Narratio types or product policy;
|
||||
- Narratio-specific configuration, paths, modes, diagnostics, and error prose
|
||||
can remain in Narratio rather than leak into the generic API;
|
||||
- at least one credible independent consumer needs materially the same
|
||||
guarantees;
|
||||
- extraction removes meaningful implementation and test ownership from
|
||||
consumers instead of adding adapters around a small helper;
|
||||
- the generic API is narrow, stable, and independently testable on every
|
||||
promised platform;
|
||||
- an existing maintained library does not already satisfy the need; and
|
||||
- the extracted module has an identified maintenance, compatibility, release,
|
||||
and vulnerability-response owner.
|
||||
|
||||
The verdicts used below are:
|
||||
|
||||
- **Extract now:** a proven shared contract and consumer justify a separate
|
||||
maintained module.
|
||||
- **Incubate:** the code contains a credible generic core, but extraction should
|
||||
wait for a concrete trigger.
|
||||
- **Keep internal:** the implementation is coherent but belongs to Narratio's
|
||||
domain or is too small to justify an external lifecycle.
|
||||
- **Simplify or delete:** independent reuse does not justify retaining the code
|
||||
in Narratio; source history is enough to preserve ideas for later recovery.
|
||||
- **Prefer an established abstraction:** a future generic consumer should start
|
||||
from an existing standard or library rather than Narratio's implementation.
|
||||
|
||||
## Executive Summary
|
||||
|
||||
No package should be extracted before the approved Narratio refactoring begins.
|
||||
The audit found one serious incubation candidate: the cross-platform subprocess
|
||||
supervision core. It implements a substantial and reasonably generic process-
|
||||
tree lifecycle that the Go standard library does not provide as one operation.
|
||||
It is nevertheless mixed with Narratio-owned environment, logging, redaction,
|
||||
filesystem, and diagnostic policy, and neither Notarius nor WeatherReporter is
|
||||
currently a matching second consumer. Extraction now would therefore invent a
|
||||
public API before its reuse requirements are known.
|
||||
|
||||
The current FileOps subsystem may be useful source material for an application
|
||||
that genuinely needs hostile-race resistance, confined traversal, no-replace
|
||||
directory promotion, or power-loss durability. It is not a cohesive generic
|
||||
library as written, and its strongest guarantees are specifically the ones
|
||||
Narratio has decided to retire. The approved FileOps simplification should
|
||||
proceed. Git history is a sufficient preservation mechanism unless a real
|
||||
external consumer appears before deletion.
|
||||
|
||||
Configuration composition, remote commit snapshots, artifact policy, session
|
||||
state, and pipeline recovery contain useful patterns, but their contracts are
|
||||
Narratio product contracts rather than generic library seams. Small path,
|
||||
bounded-I/O, locking, identifier, and atomic-write helpers do not carry enough
|
||||
independent value to justify separate versioning. Release automation is a
|
||||
possible shared tool or template, not a Go library, and current sibling
|
||||
repositories do not yet share one implementation that needs central ownership.
|
||||
|
||||
| Candidate | Verdict | Refactoring consequence |
|
||||
| --- | --- | --- |
|
||||
| Cross-platform subprocess supervision | Incubate | Separate generic mechanism from Narratio policy locally; extract only after a second consumer and owner exist. |
|
||||
| Hardened FileOps subsystem | Simplify or delete | Follow `fileops.md`; preserve history, not a Narratio dependency on an overbuilt library. |
|
||||
| YAML composition and provenance | Keep internal | Retain as Narratio's configuration DSL; reconsider structural merge dependencies separately if they reduce code. |
|
||||
| Immutable remote commit and pointer protocol | Keep internal | Preserve as Narratio's publication contract; do not market it as a generic object-store transaction library. |
|
||||
| Path-safety and bounded-I/O helpers | Keep internal | Prefer small local helpers and newer standard-library primitives where semantics match. |
|
||||
| Atomic file replacement | Keep internal for Narratio | Use a small cross-platform helper or an established dependency after focused implementation research; do not extract a new module. |
|
||||
| Object-store interface and session locks | Keep internal | These are narrow ports and product coordination rules, not reusable implementations. |
|
||||
| Release scripts and checks | Keep repository-local | Share a template or standalone tool only if multiple repositories develop materially identical behavior and drift. |
|
||||
| Artifact, manifest, analyze, and pipeline policy | Keep internal | These are Narratio domain code even where individual algorithms are conventional. |
|
||||
|
||||
## Candidate 1 — Cross-Platform Subprocess Supervision
|
||||
|
||||
### Why it is a credible library seed
|
||||
|
||||
`internal/adapters/subprocess` contains substantially more than a wrapper around
|
||||
`os/exec`. Its process owner:
|
||||
|
||||
- starts each command in an owned Unix process group or Windows Job Object;
|
||||
- prevents the Windows child from running before job assignment;
|
||||
- terminates the owned tree on cancellation or timeout;
|
||||
- attempts graceful then forceful shutdown where the platform supports it;
|
||||
- reaps the leader and bounds waits when descendants retain stream handles;
|
||||
- distinguishes exit, timeout, cancellation, capture-limit, and cleanup
|
||||
outcomes; and
|
||||
- has focused Unix and Windows tests for the lifecycle.
|
||||
|
||||
That core contract is understandable without Narratio terminology and is large
|
||||
enough that a proven consumer could benefit from sharing it. The Go
|
||||
[`os/exec`](https://pkg.go.dev/os/exec) package supplies command execution,
|
||||
custom cancellation, and `WaitDelay`, but its default cancellation targets the
|
||||
leader process rather than establishing cross-platform process-tree ownership.
|
||||
|
||||
### What is not generic
|
||||
|
||||
The package currently also owns policy that must not cross an extraction
|
||||
boundary:
|
||||
|
||||
- Narratio's inherited-environment allowlist and test-helper variables;
|
||||
- API-key environment names and extraction of their secret values;
|
||||
- fixed diagnostic size limits and application-specific owner wording;
|
||||
- log paths, group-owned file modes, and FileOps calls;
|
||||
- stderr-tail formatting and Python/file-descriptor diagnostic hints; and
|
||||
- atomic YAML and byte-writing helpers unrelated to process ownership.
|
||||
|
||||
A reusable core should accept an explicit environment and ordinary `io.Writer`
|
||||
or capture interfaces. Narratio should remain responsible for choosing and
|
||||
redacting the environment, opening bounded diagnostic sinks, selecting paths
|
||||
and modes, and translating typed results into operator-facing errors.
|
||||
|
||||
### Existing alternatives
|
||||
|
||||
The standard library remains the foundation but does not provide the whole-tree
|
||||
contract. Third-party packages now exist in this space, including
|
||||
[`processkit-go`](https://github.com/ZelAnton/processkit-go), but the candidates
|
||||
found during this audit are comparatively new or offer a materially broader
|
||||
contract than Narratio needs. They should be reevaluated at extraction time;
|
||||
the existence and maturity of this category are changing quickly, and copying
|
||||
Narratio code into a new module is not automatically preferable.
|
||||
|
||||
### Decision and extraction trigger
|
||||
|
||||
**Verdict: Incubate; do not extract now.**
|
||||
|
||||
During Narratio refactoring, make the internal mechanism/policy boundary clear
|
||||
and remove unrelated file-writing helpers from the package. Do not contort the
|
||||
internal API solely to resemble a hypothetical public module.
|
||||
|
||||
Reopen extraction only when all of these triggers are present:
|
||||
|
||||
1. a second production application needs whole-process-tree ownership on at
|
||||
least two of Narratio's supported operating systems;
|
||||
2. its timeout, graceful-shutdown, cleanup, and result semantics substantially
|
||||
overlap Narratio's rather than merely needing `exec.CommandContext`;
|
||||
3. maintained alternatives have been reevaluated and rejected for documented
|
||||
technical or maintenance reasons;
|
||||
4. an independent module API can exclude all Narratio filesystem, environment,
|
||||
redaction, and diagnostic policy; and
|
||||
5. a maintainer accepts cross-platform CI, semantic versioning, releases, and
|
||||
security ownership for the module.
|
||||
|
||||
If those conditions are met, extract the generic core from the simplified
|
||||
internal boundary and let Narratio consume it through a thin policy adapter.
|
||||
|
||||
## Candidate 2 — Hardened FileOps
|
||||
|
||||
### Why the code may be useful elsewhere
|
||||
|
||||
The present `internal/fileops` implementation includes confined root operations,
|
||||
symlink-resistant traversal, identity rechecks, same-directory replacement,
|
||||
checksummed copy and download installation, directory tree validation, atomic
|
||||
no-replace promotion, mode enforcement, file and directory syncing, and
|
||||
platform-specific implementations. An application operating on an untrusted,
|
||||
concurrently modified tree could reasonably need some of those mechanisms.
|
||||
|
||||
### Why it is not an extraction candidate as written
|
||||
|
||||
The package is a bundle of distinct concerns rather than one small generic
|
||||
contract. It combines:
|
||||
|
||||
- atomic single-file visibility and durability;
|
||||
- hostile-race-resistant traversal and copying;
|
||||
- directory transaction and no-replace semantics;
|
||||
- Narratio workspace ownership modes;
|
||||
- bounded application I/O; and
|
||||
- application-specific unsupported-platform and contextual errors.
|
||||
|
||||
A credible external library would first need to choose which threat model and
|
||||
durability level it promises, define a much smaller API, specify filesystem and
|
||||
platform assumptions, and split policy such as modes and limits from mechanism.
|
||||
That is a new library design project, not a package move.
|
||||
|
||||
There is also no matching current consumer nearby. Notarius implements its
|
||||
ordinary atomic byte write in a small local helper, and WeatherReporter likewise
|
||||
uses a focused same-directory temporary-file replacement helper. Neither needs
|
||||
Narratio's complete confined traversal and directory-promotion contract.
|
||||
|
||||
Existing components cover portions of the space. Go provides
|
||||
[`os.Root`](https://pkg.go.dev/os#Root) for root-scoped operations; libraries
|
||||
such as [`renameio`](https://github.com/google/renameio) and
|
||||
[`moby/sys/atomicwriter`](https://github.com/moby/sys/tree/main/atomicwriter)
|
||||
address atomic file replacement with different platform, durability, and mode
|
||||
tradeoffs. No single dependency should be selected merely to preserve the
|
||||
current aggregate abstraction.
|
||||
|
||||
### Decision
|
||||
|
||||
**Verdict: Simplify or delete; do not extract before refactoring.**
|
||||
|
||||
Implement the reduced contract in [`fileops.md`](fileops.md). Preserve the
|
||||
current implementation through version control and, if useful, a short design
|
||||
note identifying the historical commit. Do not make Narratio depend on an
|
||||
externalized version of guarantees that Narratio has explicitly rejected.
|
||||
|
||||
If another application later demonstrates a need, design a new module from its
|
||||
requirements and selectively recover proven algorithms and tests from history.
|
||||
That work should not block or enlarge the Narratio refactor.
|
||||
|
||||
## Candidate 3 — YAML Composition and Provenance
|
||||
|
||||
`internal/config/composition.go` implements a presence-aware YAML tree with
|
||||
source identity, duplicate-key detection, additive disjoint-source composition,
|
||||
profile overlays, deterministic canonical rendering, semantic leaf projection,
|
||||
and source attribution. These mechanics look generic at first glance.
|
||||
|
||||
The actual merge algebra is nevertheless Narratio's configuration language:
|
||||
root-only composition metadata, disjoint imports, named pipeline profiles,
|
||||
overlay ownership, atomic list treatment, null and kind-change policy, pipeline
|
||||
field provenance, effective digests, and strict decoding into `PipelineConfig`.
|
||||
The useful diagnostics also depend on those policy choices. Extracting only the
|
||||
tree mechanics would leave much of the code and all product decisions in
|
||||
Narratio, while extracting the whole package would publish Narratio's DSL as a
|
||||
supposedly generic API.
|
||||
|
||||
Notarius does not implement the same import/profile language, and
|
||||
WeatherReporter's configuration merge is a short typed decode into its own
|
||||
configuration. They are not current consumers. Structural YAML libraries such
|
||||
as [`yaml/go-yaml-dom`](https://github.com/yaml/go-yaml-dom) now provide clone
|
||||
and configurable merge operations, but they do not by themselves establish
|
||||
Narratio's provenance and additive-ownership rules.
|
||||
|
||||
**Verdict: Keep internal.** Keep product semantics explicit in Narratio. A
|
||||
future implementation pass may evaluate a maintained YAML DOM/merge dependency
|
||||
as a way to delete low-level node mechanics, but dependency adoption and public
|
||||
library extraction are different decisions. Do not create a generic module
|
||||
without a second consumer that independently chooses the same merge algebra.
|
||||
|
||||
## Candidate 4 — Immutable Remote Commit Protocol
|
||||
|
||||
The remote publication design uses immutable run-scoped objects, checksummed
|
||||
and generation-bound artifact descriptors, an immutable commit manifest, and a
|
||||
single mutable current pointer. The pattern is reusable in the abstract and
|
||||
provides coherent publication on an object store without pretending the store
|
||||
offers a multi-object transaction.
|
||||
|
||||
Its concrete schema and validation are Narratio-specific: campaign and session
|
||||
identity, run IDs, session and run manifests, configured artifact sources,
|
||||
canonical S3 key layout, publication categories, previous-session resolution,
|
||||
and current-state reconstruction. The storage interface alone is not the
|
||||
protocol; extracting it would either export Narratio's schema or require a new
|
||||
generic content model and transaction API.
|
||||
|
||||
Established content-addressed specifications already model similar generic
|
||||
ideas. The [OCI image specification](https://github.com/opencontainers/image-spec)
|
||||
defines descriptors, digests, manifests, indexes, and mutable names over
|
||||
content-addressed objects. Narratio should not migrate to OCI merely for reuse,
|
||||
but any future cross-application protocol should evaluate established formats
|
||||
before turning Narratio's private contract into a library.
|
||||
|
||||
**Verdict: Keep internal.** Treat the implementation as a good domain protocol,
|
||||
not a generic object-store transaction package. Reconsider only if multiple
|
||||
applications need interoperable snapshots; at that point start with a shared
|
||||
protocol specification and compatibility requirements, not a code extraction.
|
||||
|
||||
## Candidate 5 — Small Infrastructure Helpers
|
||||
|
||||
### Path safety
|
||||
|
||||
`internal/pathsafe` is small, pure, and widely used, but it encodes Narratio's
|
||||
slash-style relative destination rules and cross-platform object-key behavior.
|
||||
It is below the threshold where a separate module's dependency and release
|
||||
lifecycle would pay for itself. Newer Go path APIs such as
|
||||
[`filepath.IsLocal`](https://pkg.go.dev/path/filepath#IsLocal) should be used
|
||||
where their host-path semantics exactly match; object keys and canonical slash
|
||||
paths may still warrant a few local functions.
|
||||
|
||||
**Verdict: Keep internal and small.**
|
||||
|
||||
### Bounded I/O and streaming redaction
|
||||
|
||||
Bounded reads, tail capture, and streaming secret redaction are useful generic
|
||||
techniques. Narratio's instances are tied to adapter-specific limits, owner
|
||||
names, failure wording, log files, and its API-key policy. Separating them into
|
||||
external modules would trade a few local helpers for additional APIs without a
|
||||
known consumer.
|
||||
|
||||
**Verdict: Keep internal.** If subprocess incubation later proceeds, bounded
|
||||
capture may become part of that module only if consumers agree on typed limit
|
||||
semantics; secret selection and error prose remain application policy.
|
||||
|
||||
### Atomic file replacement
|
||||
|
||||
Same-directory temporary-file replacement is generic, but Narratio's reduced
|
||||
need is a small operation with explicit final mode and supported-platform
|
||||
behavior. Notarius and WeatherReporter each maintain a short local variant, but
|
||||
their differing modes, cancellation points, and parent-directory behavior show
|
||||
that superficial duplication is not yet one shared contract.
|
||||
|
||||
**Verdict: Do not create a new library.** During FileOps implementation,
|
||||
reevaluate established atomic-writer packages against Narratio's exact Windows,
|
||||
mode, and atomic-visibility requirements. Otherwise retain a small local helper.
|
||||
|
||||
### Object-store ports, file locks, IDs, and DAG helpers
|
||||
|
||||
The object-store interfaces are deliberately narrow ports over provider SDKs;
|
||||
the implementations and error semantics remain application adapters. Session
|
||||
locks enforce Narratio's one-session execution rule. Run/session identifiers
|
||||
and artifact dependency traversal are bound to Narratio schemas and lifecycle.
|
||||
Extracting any of these would either expose domain types or create generic
|
||||
abstractions with less clarity than the current code.
|
||||
|
||||
**Verdict: Keep internal.** Use established provider SDKs and standard-library
|
||||
algorithms directly rather than creating thin generic wrapper modules.
|
||||
|
||||
## Candidate 6 — Release Automation
|
||||
|
||||
Narratio's release scripts combine source-tree validation, asset-matrix builds,
|
||||
archive naming, checksum generation, Git state checks, tag creation, and push
|
||||
behavior. Those ideas are reusable, but the concrete release contract includes
|
||||
Narratio's binary name, supported target matrix, documentation, tag policy,
|
||||
asset layout, and responsibility boundary. Notarius currently has a narrower
|
||||
source-release check, while WeatherReporter documents its procedure without a
|
||||
matching shared script suite.
|
||||
|
||||
**Verdict: Keep repository-local.** This is not a runtime Go library candidate.
|
||||
If two or more repositories later carry materially identical scripts and fixes
|
||||
begin to drift, extract a versioned standalone release tool or a maintained
|
||||
template with per-project declarative metadata. Until then, local scripts keep
|
||||
release changes atomic with each repository and avoid a bootstrap dependency in
|
||||
the release path.
|
||||
|
||||
## Explicit Non-Candidates
|
||||
|
||||
The following code is intentionally excluded from generic extraction despite
|
||||
containing reusable programming techniques:
|
||||
|
||||
- artifact source classification and published-destination policy;
|
||||
- session and remote manifest schemas and validation;
|
||||
- stage ordering, selection, invalidation, resume, and force semantics;
|
||||
- analyze artifact checkpointing and dependency-aware recovery;
|
||||
- cleanup and remote lock policy;
|
||||
- Notarius, Seriatim, Scriptorium, Audita, WhisperX, and S3 adapters; and
|
||||
- configuration validation domains and party/artifact-family expansion.
|
||||
|
||||
Their value comes from making Narratio's product rules explicit. Moving them to
|
||||
a generic module would hide ownership rather than create reuse. Conventional
|
||||
algorithms inside domain code are not by themselves library candidates.
|
||||
|
||||
## Refactoring Guidance
|
||||
|
||||
This audit does not add an extraction prerequisite to the approved refactoring.
|
||||
Use the following sequence:
|
||||
|
||||
1. implement the FileOps and contract simplifications already recorded in the
|
||||
roadmap documents;
|
||||
2. keep refactored domain ownership within Narratio unless a stage explicitly
|
||||
calls for a package seam;
|
||||
3. while touching subprocess code, separate process ownership from Narratio's
|
||||
diagnostics and filesystem policy, but retain an internal API;
|
||||
4. record deleted FileOps history rather than preserving dead compatibility or
|
||||
an external dependency for hypothetical users; and
|
||||
5. revisit subprocess extraction only when the trigger conditions in this
|
||||
audit are satisfied.
|
||||
|
||||
An extraction, if later approved, should be planned and released as its own
|
||||
project. It should not be smuggled into a Narratio refactoring stage, because
|
||||
public API design, multi-platform CI, module ownership, migration, and release
|
||||
policy are independent deliverables.
|
||||
|
||||
## Final State of This Audit
|
||||
|
||||
The audit preserves one future option without delaying simplification:
|
||||
Narratio's subprocess supervision core is a credible library seed, while all
|
||||
other reviewed areas should remain internal, use established abstractions, or
|
||||
be simplified and deleted. No current code must be retained merely because it
|
||||
might someday be reusable. Version control preserves implementation history,
|
||||
and a demonstrated second consumer—not speculative generality—will trigger any
|
||||
future extraction.
|
||||
@@ -1,224 +0,0 @@
|
||||
# Potential Maintainability Audit
|
||||
|
||||
## Status
|
||||
|
||||
Initial impressions only. This document does not define action items, commit to
|
||||
an audit, or recommend specific refactoring. Its purpose is to preserve signals
|
||||
that may warrant investigation in a subsequent, deeper pass.
|
||||
|
||||
## Context
|
||||
|
||||
Narratio contains approximately 36,000 lines of production Go and 45,000 lines
|
||||
of test Go across 26 packages. That footprint appears broadly consistent with a
|
||||
durable workflow orchestrator that owns configuration, pipeline sequencing,
|
||||
artifact and manifest state, recovery, storage, subprocess integration, and
|
||||
filesystem safety. It is larger than would be expected of a simple command
|
||||
wrapper, but the current size alone is not evidence of a maintainability
|
||||
problem.
|
||||
|
||||
Narratio handles non-sensitive pipeline data and is expected to run in a
|
||||
trusted environment. A future review therefore need not assume an adversary who
|
||||
can manipulate workspace paths, symlinks, or directory entries concurrently
|
||||
with the process. Protection against accidental path escape and destructive
|
||||
operator mistakes remains useful, as do atomic replacement, crash durability,
|
||||
and recoverable state transitions. The distinction between adversarial
|
||||
filesystem hardening and crash-safe data integrity should guide any proposed
|
||||
simplification.
|
||||
|
||||
## Preliminary Signals
|
||||
|
||||
The following observations may justify a focused maintainability audit:
|
||||
|
||||
- Production code is concentrated in `internal/stage`, `internal/app`, and
|
||||
`internal/config`. These packages contain approximately 8,300, 7,900, and
|
||||
5,500 lines respectively.
|
||||
- Several production files exceed 1,000 lines, including stage publishing and
|
||||
analysis, configuration validation, and application runner code. File size
|
||||
does not establish poor cohesion, but it can conceal multiple responsibilities
|
||||
or make change impact harder to assess.
|
||||
- The application, stage, and artifact areas have substantial interaction.
|
||||
Their effective boundaries and the blast radius of changes may merit closer
|
||||
examination.
|
||||
- The most notable function-level complexity observed so far is concentrated in
|
||||
confined filesystem and directory-promotion code. This is security-sensitive
|
||||
logic built around a stronger threat model than Narratio now requires. A
|
||||
review could determine which mechanisms exist only to resist adversarial
|
||||
filesystem races and which are necessary for path safety, atomic updates, and
|
||||
crash-safe integrity.
|
||||
- Test code is larger than production code. This is a positive verification
|
||||
signal, but a deeper pass could determine whether the suite also contains
|
||||
repetition, brittle implementation coupling, or fixture and helper sprawl.
|
||||
|
||||
## Counter-Signals
|
||||
|
||||
The initial review also found evidence against a conclusion of broad or urgent
|
||||
maintainability trouble:
|
||||
|
||||
- Repository-wide tests and `go vet` pass.
|
||||
- Package dependencies are acyclic.
|
||||
- Average function complexity is modest, with higher complexity appearing
|
||||
localized rather than systemic.
|
||||
- The direct dependency set is small.
|
||||
- Documentation is structured around explicit ownership and reading paths.
|
||||
- The repository has substantial automated test coverage by volume.
|
||||
|
||||
## Questions for a Deeper Pass
|
||||
|
||||
A later audit could investigate, without presuming that changes are required:
|
||||
|
||||
- whether responsibilities within `internal/app`, `internal/stage`, and
|
||||
`internal/config` remain cohesive;
|
||||
- whether the largest files contain natural internal modules that are currently
|
||||
obscured;
|
||||
- whether stage and external-adapter implementations contain meaningful
|
||||
duplication;
|
||||
- whether tests are organized around durable behavior or coupled to incidental
|
||||
implementation structure; and
|
||||
- whether manifest, restore, publishing, and confined-filesystem invariants are
|
||||
easy to locate, understand, and verify across package boundaries.
|
||||
|
||||
The deeper pass should also evaluate these specific modernization candidates:
|
||||
|
||||
- upgrading the maintained toolchain baseline from Go 1.25 to Go 1.27, subject
|
||||
to ordinary compatibility and release validation;
|
||||
- replacing custom root-confinement, no-follow traversal, safe cleanup, and
|
||||
confined-read machinery with the standard library's `os.Root` operations
|
||||
where their semantics match Narratio's reduced threat model;
|
||||
- retaining Narratio-owned code where it still provides application-specific
|
||||
crash durability, atomic replacement, no-replace directory promotion, mode
|
||||
policy, bounded reads, or destructive-operation guardrails; and
|
||||
- evaluating the `os.Process.WithHandle` API added in Go 1.26 as a way to avoid
|
||||
reopening a Windows child process by PID before assigning it to an owned job
|
||||
object, while recognizing that it may not replace suspended startup, job
|
||||
object management, or initial-thread resumption;
|
||||
- using the standard library's `slices` and `maps` packages to replace generic
|
||||
cloning, membership, key extraction, sorting, and deduplication helpers where
|
||||
those helpers do not encode domain-specific ordering or copy semantics;
|
||||
- using `golang.org/x/sync/errgroup`, including its bounded-concurrency support,
|
||||
to simplify transcription worker coordination, cancellation, and first-error
|
||||
propagation while retaining Narratio-owned result aggregation and error
|
||||
context;
|
||||
- adopting `github.com/google/go-cmp/cmp` selectively in tests where structural
|
||||
diffs would replace `reflect.DeepEqual` or hand-written comparisons with more
|
||||
useful failure output;
|
||||
- evaluating `github.com/gofrs/flock` after the `os.Root` design is understood,
|
||||
to determine whether it can replace Unix and Windows lock primitives and
|
||||
context-aware polling without losing lock metadata or desired file ownership
|
||||
semantics; and
|
||||
- considering a CLI framework such as Cobra only if command and flag evolution
|
||||
proves costly enough to justify migration risk, dependency growth, and
|
||||
possible changes to existing parsing, help, and error-output behavior.
|
||||
|
||||
The review may also document why a replacement is not appropriate. Current
|
||||
low-confidence candidates include general configuration frameworks or schema
|
||||
validators, generic atomic-file packages, provider-neutral object-storage
|
||||
abstractions, retry or backoff packages, UUID or ULID packages, and generic
|
||||
subprocess frameworks. Narratio's composition provenance, semantic validation,
|
||||
durability guarantees, conditional object writes, identifier needs, and
|
||||
tool-specific adapter contracts may leave little generic machinery for these
|
||||
libraries to replace.
|
||||
|
||||
## Preliminary Complexity Findings
|
||||
|
||||
Complexity metrics are heuristics rather than findings of defects. Guard
|
||||
clauses, validation branches, and explicit state transitions can score highly
|
||||
while remaining appropriate. The following production-only measurements are
|
||||
initial signals for focused code reading.
|
||||
|
||||
### Execution And Planning
|
||||
|
||||
`internal/app.executePlan` is the clearest outlier observed so far:
|
||||
|
||||
- approximately 456 lines;
|
||||
- cyclomatic complexity of 79;
|
||||
- cognitive complexity of 149; and
|
||||
- more than 60 direct collaborators in the code graph.
|
||||
|
||||
The function currently spans invocation preparation, collaborator construction,
|
||||
locking, session and run manifest initialization, resume decisions, stage
|
||||
transitions, invalidation, terminal-failure persistence, and post-publish
|
||||
cleanup. A deeper pass should look for existing lifecycle boundaries that could
|
||||
be made explicit without redesigning the state machine during extraction.
|
||||
|
||||
`internal/app.Plan` is approximately 110 lines with cyclomatic complexity 27
|
||||
and cognitive complexity 51. More importantly, it independently models parts
|
||||
of the run/skip, resume-validation, stale-marking, and invalidation behavior
|
||||
also present in execution. The audit should examine whether a shared, pure stage
|
||||
transition model could reduce drift between planning and execution.
|
||||
|
||||
### Analyze Orchestration
|
||||
|
||||
The analyze implementation has two notable orchestration functions:
|
||||
|
||||
- `analyzeStage.Run` is approximately 252 lines with cyclomatic complexity 33
|
||||
and cognitive complexity 47; and
|
||||
- `executeAnalyzeArtifact` is approximately 259 lines with cyclomatic
|
||||
complexity 28 and cognitive complexity 39.
|
||||
|
||||
`internal/stage/analyze.go` combines setup, catalog construction,
|
||||
reconciliation, planning, per-artifact execution, state projection, dependency
|
||||
invalidation, reuse aggregation, evidence registration, and result assembly.
|
||||
Possible seams to investigate include execution-state initialization,
|
||||
current/reused record projection, a per-artifact transaction, evidence
|
||||
registration, and final result accumulation.
|
||||
|
||||
### Configuration Validation And Composition
|
||||
|
||||
`internal/config/validate.go` contains approximately 1,100 lines and has the
|
||||
highest aggregate file complexity observed: cyclomatic complexity 264 and
|
||||
cognitive complexity 394 across its functions. Notable functions include:
|
||||
|
||||
- `validateScriptorium`: approximately 117 lines, cyclomatic complexity 32,
|
||||
cognitive complexity 74;
|
||||
- `validateNotarius`: approximately 129 lines, cyclomatic complexity 29,
|
||||
cognitive complexity 42; and
|
||||
- `validateAudita`: approximately 54 lines, cyclomatic complexity 21,
|
||||
cognitive complexity 34.
|
||||
|
||||
These scores are partly driven by explicit validation guards. A deeper review
|
||||
should determine whether artifact-level, dependency-graph, path/output,
|
||||
input-source, and variable validation can be separated while preserving precise
|
||||
field-specific errors.
|
||||
|
||||
Composition complexity is concentrated in `internal/config/composition.go` and
|
||||
`internal/config/pipeline_composition.go`. In particular,
|
||||
`splitPipelineCompositionEnvelope` is approximately 87 lines with cyclomatic
|
||||
complexity 21 and cognitive complexity 63. It combines YAML shape checking,
|
||||
source-aware diagnostics, profile parsing, default-profile validation, cloning,
|
||||
and removal of composition metadata. Structural decoding and semantic
|
||||
validation may be separable, but the audit should preserve source provenance
|
||||
and field ownership.
|
||||
|
||||
### Previous-Session Cache
|
||||
|
||||
`internal/previouscache.Resolve` is approximately 180 lines with cyclomatic
|
||||
complexity 29 and cognitive complexity 60, within a package of only about 483
|
||||
production lines. It combines configuration checks, current-state loading,
|
||||
required and optional behavior, immutable-commit resolution, legacy candidate
|
||||
probing, local-path construction, record creation, and sorting. Modern
|
||||
immutable-commit resolution and legacy probing are distinct algorithms and may
|
||||
form a useful extraction boundary.
|
||||
|
||||
### Additional Areas To Inspect
|
||||
|
||||
Lower-priority complexity concentrations include:
|
||||
|
||||
- `internal/stage/publish.go`, approximately 1,175 lines with aggregate
|
||||
cognitive complexity 239, where file collection, output resolution,
|
||||
publication, and commit assembly may be separable;
|
||||
- `internal/fileops/directory.go`, approximately 555 lines with aggregate
|
||||
cognitive complexity 184, some of which may be removed by the `os.Root`
|
||||
evaluation;
|
||||
- `internal/app/restore_plan.go`, particularly the approximately 129-line
|
||||
`classifyRestoreAction`; and
|
||||
- `internal/manifest/analyze_state.go`, whose moderately complex validation may
|
||||
be justified by its ownership of explicit state invariants.
|
||||
|
||||
On the initial evidence, execution and shared plan/execution transition logic
|
||||
should receive the first detailed review. Configuration and analyze
|
||||
orchestration form the next tier, while their higher complexity may more often
|
||||
reflect detailed domain rules.
|
||||
|
||||
These questions should be revisited with change-history analysis, focused code
|
||||
reading, and maintenance experience before forming recommendations or creating
|
||||
work items.
|
||||
@@ -1,579 +0,0 @@
|
||||
# Narratio Refactoring Roadmap
|
||||
|
||||
## Status
|
||||
|
||||
Consolidated roadmap for the upcoming refactoring work. This document defines
|
||||
the approved actionable scope and target state. It does not describe current
|
||||
behavior and is not a staged implementation plan.
|
||||
|
||||
The complete audit inventory and final dispositions are recorded in the
|
||||
[Audit Decision Register](audit-decisions.md). Supporting analysis remains in
|
||||
the three source audits. This roadmap includes only work that at least one audit
|
||||
identified and that the final decision register classifies for immediate
|
||||
action.
|
||||
|
||||
## Goal
|
||||
|
||||
Reduce Narratio's implementation and maintenance burden while preserving the
|
||||
operational outcomes that matter: correct resumable progress, economical reuse
|
||||
of expensive generated artifacts, coherent remote publication, understandable
|
||||
cleanup and recovery, safe destructive scope, group-owned workspaces, and
|
||||
protection of API keys.
|
||||
|
||||
The refactor deliberately permits approved internal and deprecated-contract
|
||||
changes. It is not limited to producing an implementation mechanically
|
||||
equivalent to the current one.
|
||||
|
||||
## Governing Boundaries
|
||||
|
||||
- Keep the existing package-level ownership and acyclic dependency direction.
|
||||
Prefer private types, functions, and focused files over new architectural
|
||||
layers.
|
||||
- Treat campaign and session workspaces as trusted, intentionally shareable,
|
||||
group-owned data. API keys remain sensitive, and cleanup and restore remain
|
||||
confined to explicit application-owned roots.
|
||||
- Keep the session manifest as the sole durable local progress authority.
|
||||
- Preserve strict, deterministic configuration semantics without preserving
|
||||
incidental diagnostic order, complete wording, punctuation, or private
|
||||
validation choreography.
|
||||
- Keep integration-specific requests, validation, and errors with their
|
||||
adapters. Shared infrastructure should expose only exact stable mechanics.
|
||||
- Add dependencies selectively when they remove audited complexity and preserve
|
||||
the required supported-platform behavior.
|
||||
- Update tests and canonical documentation with the behavior they own rather
|
||||
than preserving retired implementation mechanisms.
|
||||
|
||||
## Actionable Workstreams
|
||||
|
||||
Each workstream uses one of three status identifiers: `Pending`, `In Progress`,
|
||||
or `Complete`. Workstreams are numbered in dependency-aware implementation
|
||||
order. The first workstream not marked `Complete` is the next workstream to
|
||||
address unless its status or an implementation plan explicitly records a
|
||||
blocker.
|
||||
|
||||
### Workstream 1 — Go baseline, process supervision, and transcription concurrency
|
||||
|
||||
**Status:** Complete
|
||||
|
||||
**Audit sources:** H11, H19, H22, H24, H33, and L01.
|
||||
|
||||
#### Underlying issue
|
||||
|
||||
The maintained Go baseline prevents use of newer root and process primitives,
|
||||
the Windows subprocess owner reopens a PID instead of using a lifetime-stable
|
||||
handle, transcription manually implements bounded cancellation and first-error
|
||||
coordination, and the shared subprocess package mixes generic process ownership
|
||||
with Narratio environment, diagnostic, redaction, and file-writing policy.
|
||||
|
||||
#### Required changes
|
||||
|
||||
- Upgrade `go.mod`, generated module state, every maintained CI and release Go
|
||||
image, and release documentation to Go 1.26 in one bounded change. Go 1.26
|
||||
provides `os.Process.WithHandle`; the required `os.Root.RemoveAll` operation
|
||||
was already available in Go 1.25.
|
||||
- Validate the upgrade with the repository-wide tests, race checks, vet,
|
||||
maintained documentation and examples, and all supported release builds.
|
||||
- Assign the suspended Windows child to its Job Object inside
|
||||
`os.Process.WithHandle`. Preserve suspended creation, job configuration,
|
||||
initial-thread resumption, fail-closed cleanup, and job disposal.
|
||||
- Replace the transcription worker pool with `errgroup.WithContext` and
|
||||
`SetLimit`. Preserve speaker-qualified failures, parent-cancellation context,
|
||||
aggregation of successful results under a narrow lock, and deterministic
|
||||
output ordering.
|
||||
- Clarify the internal subprocess boundary: the reusable mechanism owns process
|
||||
tree start, wait, termination, reaping, and typed outcomes; Narratio owns
|
||||
environment selection, secret-value selection and redaction, bounded log
|
||||
sinks, paths, modes, diagnostic wording, and tool-specific hints.
|
||||
- Remove atomic YAML and byte-writing helpers from the subprocess package and
|
||||
route file replacement through the focused filesystem owner.
|
||||
|
||||
#### Target state
|
||||
|
||||
Narratio builds and tests on Go 1.26, uses the safer native Windows handle API,
|
||||
and has concise bounded transcription concurrency. Process-tree ownership is
|
||||
internally separable from Narratio policy without creating or depending on a
|
||||
new external subprocess library during this refactor.
|
||||
|
||||
### Workstream 2 — Filesystem simplification and restore correctness
|
||||
|
||||
**Status:** Complete
|
||||
|
||||
**Audit sources:** H02, H06, H07, H18, H20, H21, H29, H45, H46, C04,
|
||||
L02, L05a, and L05c. Supporting filesystem rationale and policy are recorded
|
||||
in [`fileops.md`](fileops.md); the requirements below are the consolidated
|
||||
action scope.
|
||||
|
||||
#### Underlying issue
|
||||
|
||||
The current FileOps subsystem promises adversarial race resistance, universal
|
||||
confinement and no-follow behavior, atomic no-replace directory promotion, and
|
||||
power-loss durability that Narratio's trusted, session-locked workspace does
|
||||
not require. The complexity also obscures one retained correctness boundary:
|
||||
restore classification can follow a symlink and treat it as an already matching
|
||||
regular file.
|
||||
|
||||
#### Required changes
|
||||
|
||||
- Replace ordinary workspace reads, writes, copies, and renames with direct
|
||||
`os`, `io`, and `path/filepath` operations near their domain owners.
|
||||
- Retain small shared helpers only for corruption-resistant same-directory file
|
||||
replacement, centralized bounded I/O, workspace modes, and explicitly
|
||||
root-scoped cleanup or restore.
|
||||
- Keep atomic visibility for manifests and other corruption-sensitive single
|
||||
files without file or directory syncing or a power-loss durability promise.
|
||||
- Receive remote downloads into temporary files and validate size, checksum,
|
||||
format, and any adapter contract before installation. Remove redundant
|
||||
download-install surfaces.
|
||||
- Have Notarius write one unique staging bundle inside the invocation workspace;
|
||||
validate its location and integration contract, normalize workspace modes,
|
||||
require an absent unique destination, and rename the directory directly.
|
||||
- Delete directory tree-copy promotion, race hooks, identity rechecks,
|
||||
no-replace platform code, directory syncing, capability errors, and the
|
||||
tests that exist solely for those retired mechanisms.
|
||||
- Use `os.Root` for destructive cleanup and restore. Reject escaped targets and
|
||||
classify a leaf or ancestor symlink as a non-regular restore conflict in both
|
||||
force and non-force planning. Hash only a regular target verified within the
|
||||
root.
|
||||
- Centralize and preserve group-owned directory and file modes. Keep API-key
|
||||
values out of permissive workspace files, logs, and errors, and preserve all
|
||||
explicit credential protections.
|
||||
- Keep size limits centralized and discoverable, deliberately above ordinary
|
||||
workloads, with errors that name the limit and enforcing adapter.
|
||||
|
||||
#### Target state
|
||||
|
||||
Ordinary filesystem work is clear standard-library code. Shared filesystem
|
||||
code is small and policy-focused; extract performs one validated rename rather
|
||||
than a second full-tree copy; restore and cleanup cannot escape their roots or
|
||||
follow symlinked targets; group-owned operation remains first-class; and API
|
||||
keys remain protected without treating ordinary D&D data as private.
|
||||
|
||||
### Workstream 3 — Canonical contracts and compatibility purge
|
||||
|
||||
**Status:** Complete
|
||||
|
||||
**Audit sources:** H43 and C01.
|
||||
|
||||
#### Underlying issue
|
||||
|
||||
Deprecated configuration and state formats remain active runtime branches.
|
||||
They require alternate parsing, probing, ambiguity handling, fixtures, errors,
|
||||
and documentation, and prevent one canonical representation from being clearly
|
||||
authoritative.
|
||||
|
||||
#### Required changes
|
||||
|
||||
- Remove the legacy remote `current/manifest.json` and `current/run_id.txt`
|
||||
reader.
|
||||
- Remove legacy previous-session artifact candidates and existence probing;
|
||||
resolve only artifacts declared by the current committed state.
|
||||
- Remove unversioned party documents, separate `players_file` handling, and
|
||||
legacy session-level party or player overrides tied to that model.
|
||||
- Remove aggregate-only analyze availability, fallback to incidental files,
|
||||
extraction records without current contract and provenance evidence, and
|
||||
manifest or artifact branches predating the supported schema.
|
||||
- Delete associated fields, constants, types, helpers, schemas, tests, fixtures,
|
||||
examples, and documentation rather than retaining an internal compatibility
|
||||
facade.
|
||||
- Require explicit supported versions or unambiguous current authority markers.
|
||||
Reject old state early with a clear unsupported-version or migration error.
|
||||
- Provide migration guidance through regeneration or republication without
|
||||
keeping steady-state migration readers in production.
|
||||
|
||||
#### Target state
|
||||
|
||||
Every supported configuration, party document, local manifest, remote current
|
||||
snapshot, extraction record, analyze record, and previous-session input has one
|
||||
canonical resolution path. Deprecated representations are absent from runtime
|
||||
code and fail clearly instead of being inferred or normalized.
|
||||
|
||||
### Workstream 4 — One durable local manifest authority
|
||||
|
||||
**Status:** Complete
|
||||
|
||||
**Audit sources:** H14, H15, H17, C02, and C03.
|
||||
|
||||
#### Underlying issue
|
||||
|
||||
The session manifest and structured run manifest currently mirror stage and
|
||||
artifact progress. Keeping both synchronized makes diagnostic persistence part
|
||||
of pipeline correctness and creates dual-save ordering, projections, rollback,
|
||||
terminal recovery, and cleanup conflicts.
|
||||
|
||||
#### Required changes
|
||||
|
||||
- Eliminate the structured run-manifest schema, store, paths, validation,
|
||||
before/after/skip/failure/terminal saves, invocation-only projections,
|
||||
publish classification, compatibility code, tests, fixtures, and
|
||||
documentation.
|
||||
- Retain the run ID for invocation identity, run-local paths, adapter provenance,
|
||||
and immutable remote publication.
|
||||
- Retain bounded logs, generated configuration, receipts, immediate command
|
||||
summaries, and other useful diagnostics without making their persistence a
|
||||
second progress transaction.
|
||||
- Make the session manifest the sole local authority for stage status, artifact
|
||||
currency, resume, status, restore, and publication.
|
||||
|
||||
#### Target state
|
||||
|
||||
One atomic session-manifest update determines durable local progress. Diagnostic
|
||||
write failure cannot overturn otherwise valid pipeline work, and no invocation
|
||||
manifest must remain synchronized with the progress ledger.
|
||||
|
||||
### Workstream 5 — Runner lifecycle and pre-execution decisions
|
||||
|
||||
**Status:** Complete
|
||||
|
||||
**Audit sources:** H04, H07, H14, H15, H17, H34, and H35.
|
||||
|
||||
#### Underlying issue
|
||||
|
||||
`executePlan` combines preflight, locking, invocation setup, collaborator
|
||||
construction, resume decisions, stage transitions, failure persistence, and
|
||||
cleanup in one high-change function. Planning and execution also independently
|
||||
sequence some stale marking and invalidation decisions, while application tests
|
||||
replace package-global functions repeatedly.
|
||||
|
||||
#### Required changes
|
||||
|
||||
- Introduce a private invocation runtime or equivalent cohesive private state
|
||||
carrying resolved identity, paths, stores, the session manifest, environment,
|
||||
collaborators, and command summary state.
|
||||
- Extract lifecycle operations for preparation and lock acquisition,
|
||||
invocation-state setup, collaborator composition, one-stage execution, and
|
||||
finalization.
|
||||
- Keep session-lock acquisition and release, session-manifest persistence, and
|
||||
the top-level transaction order visible in the lifecycle owner.
|
||||
- Introduce a pure pre-execution decision value combining recorded status,
|
||||
force, resume validation, stale reason, and required invalidation.
|
||||
- Share only manifest-transition helpers that planning and execution apply
|
||||
identically. Keep planned outcomes distinct from actual stage results and
|
||||
persistence.
|
||||
- Add parity tests for run versus skip, non-resumable records, forced
|
||||
invalidation, self-skip, and changed successful results.
|
||||
- Replace affected mutable package-global function seams with a small
|
||||
invocation-scoped dependency set. Do not introduce a general dependency
|
||||
injection framework.
|
||||
|
||||
#### Target state
|
||||
|
||||
The top-level runner reads as an explicit lifecycle with one visible lock and
|
||||
persistence authority. Planning and execution agree through one narrow pure
|
||||
decision model, actual stage outcomes remain execution-owned, and focused tests
|
||||
do not require process-global substitution.
|
||||
|
||||
### Workstream 6 — Per-artifact analyze checkpointing and recovery
|
||||
|
||||
**Status:** Complete
|
||||
|
||||
**Audit sources:** H04, H14, H15, H36, H37, C02, and C03.
|
||||
|
||||
#### Underlying issue
|
||||
|
||||
Analyze already has useful per-artifact identity and dependency semantics, but
|
||||
successful results participate in session and invocation projections and are
|
||||
not expressed as the simple resumable queue operators need. The per-artifact
|
||||
transaction also mixes preparation, invocation, validation, installation, and
|
||||
evidence construction behind a broad parameter list.
|
||||
|
||||
#### Required changes
|
||||
|
||||
- Use only the session manifest's analyze artifact collection as durable
|
||||
authority. Remove invocation projections and their validation, application,
|
||||
and rollback machinery.
|
||||
- Execute the effective artifact set in deterministic dependency order.
|
||||
- After each artifact succeeds and its canonical output is validated and
|
||||
materialized, update its current record, stale any transitive dependents whose
|
||||
evidence is invalidated, atomically save the session manifest, and only then
|
||||
continue.
|
||||
- If the checkpoint save fails, leave incidental output non-authoritative and
|
||||
allow safe recomputation on a later run.
|
||||
- On artifact failure, persist its failed state, stale current transitive
|
||||
dependents, mark the aggregate analyze stage failed, and stop before later
|
||||
artifacts.
|
||||
- On an ordinary later run, retain valid successes, retry the first failed,
|
||||
stale, or missing selected artifact, and continue after it succeeds.
|
||||
- Preserve established force and artifact-selection semantics: unqualified
|
||||
force reruns the configured analyze set; selected force applies to the
|
||||
selected artifacts and required dependency closure without rerunning
|
||||
unrelated valid artifacts.
|
||||
- Retain fingerprints, output evidence, deterministic dependency ordering,
|
||||
dependency-aware invalidation, and the rule that incidental files cannot
|
||||
create current authority.
|
||||
- Give `analyzeStage.Run` a private session-only execution accumulator or
|
||||
equivalent focused state. Split each artifact transaction into input/request
|
||||
preparation, adapter invocation, validated materialization, and
|
||||
record/evidence construction under one wrapper and narrow context.
|
||||
|
||||
#### Target state
|
||||
|
||||
Analyze is a deterministic resumable artifact queue backed by one manifest.
|
||||
Successful expensive work survives a later failure, invalid dependents cannot
|
||||
remain current, and ordinary resume continues at the first work that actually
|
||||
needs attention.
|
||||
|
||||
### Workstream 7 — Configuration structure and semantic diagnostics
|
||||
|
||||
**Status:** Complete
|
||||
|
||||
**Audit sources:** H04, H14, H15, H17, H38, H39, H40, H41, H42, C01, and C06.
|
||||
|
||||
#### Underlying issue
|
||||
|
||||
Configuration behavior is appropriately strict, but unrelated validation
|
||||
domains share one large file, several functions combine distinct phases, legacy
|
||||
fields remain active, and tests treat exact diagnostic choreography as though
|
||||
it were a versioned semantic contract.
|
||||
|
||||
#### Required changes
|
||||
|
||||
- Complete the legacy configuration purge defined by the canonical-contract
|
||||
workstream.
|
||||
- Split validation within package `config` into core pipeline/session,
|
||||
external-adapter, publish/storage, and Scriptorium artifact/dependency
|
||||
domains.
|
||||
- Separate Scriptorium artifact declaration and output checks,
|
||||
dependency/input checks, and variable checks. Traverse declarations
|
||||
deterministically where ordering affects reproducibility, without promising
|
||||
one particular independent-error discovery order.
|
||||
- Separate Notarius reference-binding normalization from output-lane
|
||||
normalization and collision checks; retain enablement and scalar checks in
|
||||
the top-level validator.
|
||||
- Move the cohesive Audita validator with the other adapter validators without
|
||||
fragmenting its straightforward guards.
|
||||
- Split composition-envelope shape decoding, cross-field semantic validation,
|
||||
and removal of root-only metadata while retaining Narratio's product-specific
|
||||
composition model.
|
||||
- Preserve strict YAML decoding, unknown-field rejection, additive imports with
|
||||
no duplicate ownership, deterministic profile selection, meaningful overlay
|
||||
behavior for explicit values and collections, centralized defaults, canonical
|
||||
party and artifact-family resolution, and secret-free effective digests.
|
||||
- Preserve actionable field identification and enough source information to
|
||||
correct composition conflicts. Stop preserving complete prose, punctuation,
|
||||
formatting, validation call order, or unused per-node provenance.
|
||||
- Rewrite brittle tests to assert typed categories, field paths, structured
|
||||
details, or the smallest stable semantic fragment. Retain exact strings only
|
||||
for intentionally stable CLI or machine-consumed contracts.
|
||||
|
||||
#### Target state
|
||||
|
||||
Configuration remains strict, deterministic, and operator-friendly, but its
|
||||
implementation is organized by semantic owner and can improve diagnostics
|
||||
without treating private ordering or wording as compatibility behavior.
|
||||
|
||||
### Workstream 8 — Remote publication, per-artifact locks, and publish decomposition
|
||||
|
||||
**Status:** Complete
|
||||
|
||||
**Audit sources:** H04, H15, H18, H44, and C05.
|
||||
|
||||
#### Underlying issue
|
||||
|
||||
Publish contains several cohesive concerns in one large file, while remote
|
||||
locks use a shared mutable YAML control plane with static precedence, merging,
|
||||
conditional replacement, and retry machinery. Omitting a locked artifact from
|
||||
a new immutable snapshot can also make the canonical artifact disappear rather
|
||||
than keep it frozen.
|
||||
|
||||
#### Required changes
|
||||
|
||||
- Split publish source discovery and opening, output-rule resolution, transfer
|
||||
preparation and verification, immutable upload, and commit/pointer encoding
|
||||
into focused files while retaining `publishStage.Run` as the transaction
|
||||
owner.
|
||||
- Replace static pipeline locks and the central remote lock manifest with one
|
||||
versioned JSON lock object per normalized stable source ID at the approved
|
||||
hashed key. Validate that each payload source hashes to its object name.
|
||||
- Remove static/remote merging, precedence, configuration injection, shared
|
||||
document replacement retries, and the retired lock schema and tests.
|
||||
- Extend the narrow object-store port and each implementation or fake with the
|
||||
deletion operation needed to remove one lock.
|
||||
- Adapt lock list/add/remove behavior to independent objects: list validates the
|
||||
prefix, add fails if present unless force replaces metadata, and remove treats
|
||||
absence as idempotent success.
|
||||
- Check each relevant lock before spending work to publish its artifact and
|
||||
recheck immediately before advancing the current pointer.
|
||||
- Build a new commit so an unlocked artifact uses its newly uploaded
|
||||
declaration, while a locked artifact carries the exact prior immutable
|
||||
declaration when one exists. A locked artifact never previously published may
|
||||
remain absent without becoming a required-output error.
|
||||
- If the final recheck discovers a newly added lock, do not advance the pointer;
|
||||
allow unselected immutable attempt objects to remain and rebuild on the next
|
||||
run. A lock removed after the initial read may conservatively carry the prior
|
||||
declaration once.
|
||||
- Allow carried declarations to reference an earlier immutable run prefix in
|
||||
the same campaign and session, retaining source identity, originating run,
|
||||
destination, checksum, size, and generation. Readers must use only objects
|
||||
declared by the selected commit and must not scan mutable prefixes.
|
||||
- Distinguish newly published, carried-forward locked,
|
||||
locked-but-never-published, optional missing, and unselected outcomes in
|
||||
publish metadata and inspection.
|
||||
- Preserve the visible transaction boundary: immutable objects, immutable
|
||||
commit, final lock revalidation, then one mutable current-pointer write.
|
||||
|
||||
#### Target state
|
||||
|
||||
Publish remains one coherent immutable-snapshot transaction with navigable
|
||||
internal files. Each lock is an independent remote object, changes to one lock
|
||||
cannot overwrite another, and later snapshots retain the exact prior canonical
|
||||
version of a locked artifact while unlocked artifacts advance.
|
||||
|
||||
### Workstream 9 — Cleanup as the final pipeline stage
|
||||
|
||||
**Status:** Complete
|
||||
|
||||
**Audit sources:** H20, H21, C02, and C04.
|
||||
|
||||
#### Underlying issue
|
||||
|
||||
Local deletion currently occurs as a post-publish transaction with a special
|
||||
manifest obligation. A cleanup error can therefore make a successfully
|
||||
committed remote publication appear to have failed, and the extra state model
|
||||
duplicates ordinary stage retry behavior.
|
||||
|
||||
#### Required changes
|
||||
|
||||
- Add `cleanup` as the final canonical stage, after every stage that may need
|
||||
run-local data or diagnostics.
|
||||
- Remove post-publish cleanup from the runner and delete the
|
||||
`post_publish_cleanup` manifest model, persistence, compatibility code,
|
||||
tests, and documentation.
|
||||
- Use existing cleanup configuration to select spool audio, run workspace data,
|
||||
both, or neither.
|
||||
- Require evidence of a successful current remote publish before deleting data
|
||||
whose retention depends on publication, and bind targets to that published
|
||||
run rather than inferring arbitrary paths from current configuration.
|
||||
- Self-skip clearly when nothing is configured, treat already-absent eligible
|
||||
targets as success, use the filesystem workstream's root-scoped safeguards,
|
||||
and produce no canonical artifact output.
|
||||
- Record cleanup through ordinary session-manifest stage status. A cleanup
|
||||
failure must leave publish success and remote current state unchanged; an
|
||||
ordinary later run should retry cleanup while reusing valid upstream stages.
|
||||
- Include cleanup in the fixed invalidation relation so a changed or forced
|
||||
prerequisite makes an earlier cleanup result stale where appropriate.
|
||||
- Ensure a selected range ending before cleanup performs no deletion, while a
|
||||
default full run includes cleanup.
|
||||
|
||||
#### Target state
|
||||
|
||||
Remote publication and local deletion are distinct, understandable outcomes.
|
||||
Publish succeeds when its pointer becomes current; cleanup succeeds when its
|
||||
eligible local targets are absent. Cleanup failure is reported and retried as a
|
||||
cleanup-stage failure without republishing unchanged data.
|
||||
|
||||
### Workstream 10 — Quality, testability, and documentation alignment
|
||||
|
||||
**Status:** Complete
|
||||
|
||||
**Audit sources:** H07, H08, H12, H13, H17, H18, H23, H25, C01, C02, C03,
|
||||
C04, C05, and C06.
|
||||
|
||||
#### Underlying issue
|
||||
|
||||
Most tests protect meaningful behavior, but some preserve mechanisms and
|
||||
contracts now approved for removal, some application tests rely on mutable
|
||||
global seams, and large structural failures can be difficult to diagnose.
|
||||
Canonical documentation also describes several filesystem mechanisms as
|
||||
durable policy.
|
||||
|
||||
#### Required changes
|
||||
|
||||
- Delete tests, fixtures, hooks, and helpers only when their underlying legacy
|
||||
format, run-manifest model, cleanup obligation, central lock store, or FileOps
|
||||
guarantee is removed.
|
||||
- Add or retain focused behavior coverage for atomic visibility, bounded limits
|
||||
and adapter errors, group modes, validated downloads, Notarius rename,
|
||||
destructive scope, restore symlink conflicts, secret redaction, session-only
|
||||
progress, per-artifact checkpoints, planning/execution parity, lock
|
||||
carry-forward, and idempotent cleanup.
|
||||
- Prefer real temporary filesystems, fake object stores, and controlled helper
|
||||
processes at their current durable boundaries. Do not recreate removed
|
||||
algorithms through private test hooks.
|
||||
- Replace affected mutable application globals with the invocation-scoped
|
||||
dependency set defined by the runner workstream.
|
||||
- Use `cmp.Diff` for selected large composed configurations, analyze state,
|
||||
restore plans, and publish mappings. Use direct or standard-library equality
|
||||
for simple values.
|
||||
- Apply `slices` and `maps` only to shallow clones, membership checks, and
|
||||
deterministic key handling in code already being touched, with nil, empty,
|
||||
and ordering semantics verified.
|
||||
- Update architecture, operations, troubleshooting, integration, internal,
|
||||
configuration, CLI, examples, and release documentation only where each is
|
||||
the canonical owner of changed behavior. Remove retired claims rather than
|
||||
maintaining compatibility narratives as current behavior.
|
||||
- Keep repository-wide tests and vet mandatory throughout. Apply focused race,
|
||||
documentation/example, and cross-platform release-build checks in proportion
|
||||
to each workstream, with the complete release-candidate validation at the
|
||||
integrated boundary.
|
||||
|
||||
#### Target state
|
||||
|
||||
Tests protect retained outcomes and failure boundaries instead of obsolete
|
||||
mechanisms. Collaborators are injectable per invocation without a framework,
|
||||
large diffs are readable where useful, and canonical documentation accurately
|
||||
describes the simplified implemented contracts as each workstream lands.
|
||||
|
||||
## Dependency Relationships
|
||||
|
||||
These relationships constrain a future implementation plan without defining
|
||||
its numbered stages:
|
||||
|
||||
- The Go 1.26 baseline is required before relying on the process-handle API;
|
||||
the root-removal API is already present in the current Go 1.25 baseline.
|
||||
- The compatibility purge should remove legacy branches before maintainers
|
||||
invest in decomposing the corresponding current-only previous-cache,
|
||||
configuration, analyze, and publication paths.
|
||||
- Removing the run manifest establishes the session-only authority needed by
|
||||
runner decomposition, per-artifact analyze checkpointing, and the ordinary
|
||||
cleanup stage.
|
||||
- Runner lifecycle decomposition and its pre-execution decision model provide
|
||||
the clean stage boundary used by analyze recovery and cleanup retry.
|
||||
- The per-artifact lock representation and carry-forward rules must be reflected
|
||||
together in publish construction, commit validation, current-state reading,
|
||||
operator commands, and tests.
|
||||
- Filesystem simplification and root-scoped destructive helpers must be
|
||||
available to the cleanup stage, while restore correctness belongs in the same
|
||||
filesystem change rather than the old facade.
|
||||
- Test and documentation alignment accompanies the workstream that changes the
|
||||
behavior; it is not deferred to a final bulk cleanup.
|
||||
|
||||
## Compatibility and Release Intent
|
||||
|
||||
The compatibility purge intentionally rejects deprecated formats rather than
|
||||
silently migrating them. Migration guidance should direct users to regenerate,
|
||||
republish, or otherwise replace old state. No runtime compatibility layer should
|
||||
survive solely to make the internal refactor invisible.
|
||||
|
||||
The approved audit decision is that this combined work remains eligible for a
|
||||
minor release because the removed compatibility surfaces were already
|
||||
deprecated and explicitly removable. Preparing or publishing that release is
|
||||
not part of this roadmap.
|
||||
|
||||
## Consolidated Target State
|
||||
|
||||
After this roadmap is implemented:
|
||||
|
||||
- Narratio has one canonical configuration and state model with no deprecated
|
||||
runtime fallback paths;
|
||||
- the session manifest alone owns durable local progress;
|
||||
- analyze preserves each valid successful artifact and resumes failed, stale,
|
||||
or missing work in dependency order;
|
||||
- application orchestration has explicit private lifecycle phases and one
|
||||
narrow planning/execution decision model;
|
||||
- ordinary filesystem work uses the standard library, while small focused
|
||||
helpers retain atomic visibility, bounds, modes, destructive scope, restore
|
||||
safety, and secret protection;
|
||||
- Notarius output is installed by one validated rename without a duplicate tree;
|
||||
- publish owns coherent remote publication, per-artifact locks preserve prior
|
||||
canonical declarations, and cleanup is a separate final stage;
|
||||
- configuration remains strict and deterministic without freezing incidental
|
||||
diagnostic choreography;
|
||||
- Windows subprocess ownership and transcription concurrency use the audited
|
||||
Go 1.26 primitives, and subprocess mechanism is internally distinct from
|
||||
Narratio policy; and
|
||||
- tests and documentation protect and describe the reduced observable contract.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None. The audits and subsequent approved decisions resolve the product and
|
||||
architecture choices required to prepare a staged implementation plan.
|
||||
Reference in New Issue
Block a user