From af5fbc1c4f8d8d3ad2a28fe0e7a0df5c56720862 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 31 Aug 2026 04:43:24 +0000 Subject: [PATCH] Record maintainability audit results --- docs/roadmap/audit.md | 717 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 717 insertions(+) create mode 100644 docs/roadmap/audit.md diff --git a/docs/roadmap/audit.md b/docs/roadmap/audit.md new file mode 100644 index 0000000..8a4a0be --- /dev/null +++ b/docs/roadmap/audit.md @@ -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.