Record maintainability audit results
This commit is contained in:
717
docs/roadmap/audit.md
Normal file
717
docs/roadmap/audit.md
Normal file
@@ -0,0 +1,717 @@
|
||||
# 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, durability, recovery,
|
||||
and integration requirements.
|
||||
|
||||
The audit did identify one correctness remediation and several proportional
|
||||
maintainability improvements:
|
||||
|
||||
1. Restore planning must inspect existing local targets without following
|
||||
symlinks. `classifyRestoreAction` currently uses `os.Stat` and path-based
|
||||
hashing, so a symlink to a regular file can be classified as `skip_same`
|
||||
despite the documented restore and path-confinement contract.
|
||||
2. `internal/fileops/directory.go` contains a second directory-promotion and
|
||||
tree-copy implementation used only by tests. Delete that alternate
|
||||
implementation and test the production `os.Root` path. Then remove
|
||||
identity-race hooks and rechecks that exist solely for an adversarial
|
||||
concurrent-filesystem threat model, while retaining static symlink checks,
|
||||
root confinement, regular-file restrictions, atomic no-replace promotion,
|
||||
durability, workspace modes, and cleanup guardrails.
|
||||
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 or atomic-file frameworks,
|
||||
generic subprocess machinery, or new identifier/retry/storage abstractions.
|
||||
|
||||
## 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 accidental path-escape and destructive-action
|
||||
guardrails, but remove tests and identity rechecks whose only purpose is to
|
||||
defeat a concurrent adversary replacing directory entries after inspection.
|
||||
Update internal file-operation documentation when that distinction changes.
|
||||
|
||||
**Reasoning:** Narratio still needs safe scope, static symlink rejection,
|
||||
regular-file checks, atomic updates, and crash durability. It does not need to
|
||||
prove that every inspected inode remained at the same name in the presence of
|
||||
an attacker racing each operation. `os.Root` already prevents rooted operations
|
||||
from escaping their root.
|
||||
|
||||
### 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, with a concrete duplication issue.
|
||||
|
||||
**Recommended change:** Delete the test-only `promoteDirectory`,
|
||||
`promoteDirectoryWithHooks`, `copyRegularTree`, `copyRegularDirectory`, and
|
||||
`copyRegularFile` implementation family from production code. Exercise
|
||||
`PromoteDirectory` and its actual handle-relative install path in tests. Remove
|
||||
adversarial replacement hooks and redundant `SameFile` rechecks where `os.Root`
|
||||
already supplies the required confinement, while retaining the application
|
||||
semantics listed in H21.
|
||||
|
||||
**Reasoning:** The first half of `directory.go` is the real `os.Root`-based
|
||||
implementation; the second half reimplements the same tree copy for injectable
|
||||
tests. Tests of the alternate algorithm can pass while production changes, and
|
||||
the duplication accounts for a substantial portion of the file's size and
|
||||
complexity.
|
||||
|
||||
### H07 — Test code larger than production code
|
||||
|
||||
**Verdict:** Focused opportunity, not evidence of overtesting.
|
||||
|
||||
**Recommended change:** Preserve behavior and invariant coverage. Remove tests
|
||||
of the alternate directory implementation, 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 duplicated filesystem test machinery;
|
||||
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 filesystem, manifest, subprocess,
|
||||
and recovery tests. Remove tests tied to the alternate fileops algorithm; 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 and package-global function substitution, not from the suite as
|
||||
a whole.
|
||||
|
||||
### H18 — Invariant discoverability across packages
|
||||
|
||||
**Verdict:** Already satisfied, except for one implementation mismatch.
|
||||
|
||||
**Recommended change:** Fix restore target inspection as described in H46.
|
||||
Otherwise retain the current policy-to-internal-document structure and keep
|
||||
transaction-order comments adjacent to runner and publish code.
|
||||
|
||||
**Reasoning:** Manifest durability, restore recovery, publish commit ordering,
|
||||
workspace modes, lock ownership, and confined file operations are all described
|
||||
in owner documents and protected by focused tests. Restore planning's use of
|
||||
`os.Stat` is inconsistent with those otherwise clear rules.
|
||||
|
||||
### 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. 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:** Already substantially satisfied.
|
||||
|
||||
**Recommended change:** Standardize remaining fileops paths on the existing
|
||||
`os.Root` primitives and delete duplicate compatibility/test paths. Do not
|
||||
rewrite already-rooted code merely to rename helpers.
|
||||
|
||||
**Reasoning:** Current confined writes, reads, cleanup, download installation,
|
||||
and production directory promotion already use `os.Root`. The remaining value
|
||||
is simplification and consolidation, not an initial migration.
|
||||
|
||||
### H21 — Retain Narratio-owned durability and policy code
|
||||
|
||||
**Verdict:** Retain.
|
||||
|
||||
**Recommended change:** Keep atomic replace and no-replace promotion ordering,
|
||||
file and directory syncing, unsupported-platform errors, workspace modes,
|
||||
bounded reads, regular-file restrictions, proper-descendant cleanup checks, and
|
||||
application-owned destructive-operation gates.
|
||||
|
||||
**Reasoning:** These are observable Narratio contracts that `os.Root` does not
|
||||
provide. Generic filesystem packages would either weaken them or require a
|
||||
similar application-specific layer around the dependency.
|
||||
|
||||
### 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:** Not substantiated.
|
||||
|
||||
**Recommended change:** Retain and simplify `internal/fileops` rather than
|
||||
replacing it.
|
||||
|
||||
**Reasoning:** Narratio requires explicit modes, file and parent syncing,
|
||||
pre-commit cancellation, confined parents, downloaded-temp installation, and
|
||||
clear unsupported guarantees. Generic packages do not own that full contract.
|
||||
|
||||
### 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; see H06 and H20.
|
||||
|
||||
**Recommended change:** Remove the alternate path-based implementation and its
|
||||
test hooks, keep one `os.Root` implementation, and simplify adversarial
|
||||
identity-race checks according to H02. Retain platform-specific handle-relative
|
||||
no-replace rename and durability checks.
|
||||
|
||||
**Reasoning:** This is the clearest actual code duplication found. It also
|
||||
causes tests to exercise a different install implementation from production.
|
||||
|
||||
### H46 — `classifyRestoreAction`
|
||||
|
||||
**Verdict:** Confirmed for correctness, not merely complexity.
|
||||
|
||||
**Recommended change:** Pass the session root into classification and inspect an
|
||||
existing target with `os.Lstat` plus the shared confined regular-file opener.
|
||||
Hash the already-opened handle rather than reopening a mutable absolute path.
|
||||
Classify a symlink at the leaf or in an ancestor as
|
||||
`RestoreConflictNonRegular`, even when its target bytes match. Add tests for
|
||||
leaf and ancestor symlinks in 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 restore and confined-filesystem contracts. 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 one correction.
|
||||
|
||||
**Recommended change:** Address restore symlink classification first because it
|
||||
is a correctness issue. Then remove duplicate fileops code and simplify the
|
||||
threat-model-only machinery. Next decompose execution and share its narrow
|
||||
planning decision model. Follow with analyze/config, publish/previous-cache, and
|
||||
finally modernization/test ergonomics.
|
||||
|
||||
**Reasoning:** Execution remains the largest change-blast hotspot, and
|
||||
configuration/analyze remain the next tier. The deeper review elevated the
|
||||
restore mismatch and fileops duplication because they are concrete rather than
|
||||
metric-only findings.
|
||||
|
||||
## Recommended Implementation Sequence
|
||||
|
||||
This is ordering guidance for a future decision-complete implementation plan,
|
||||
not staged implementation instructions:
|
||||
|
||||
1. Correct restore target confinement and add regression tests.
|
||||
2. Upgrade the Go baseline to 1.27 in isolation and run the full release
|
||||
candidate checks.
|
||||
3. Consolidate fileops on the production `os.Root` path and revise its tests and
|
||||
internal documentation to the trusted-environment threat model.
|
||||
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.
|
||||
|
||||
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.
|
||||
Reference in New Issue
Block a user