765 lines
36 KiB
Markdown
765 lines
36 KiB
Markdown
# 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.
|