Plan Go and process refactoring

docs/roadmap/audit.md

@@ -33,23 +33,25 @@ Verdicts use these terms:
 
 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.
+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. 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.
+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
@@ -68,8 +70,10 @@ maintainability improvements:
    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.
+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
 
@@ -87,16 +91,19 @@ than command wrapping, and the observed size maps to documented responsibilities
 
 **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.
+**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 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.
+**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
 
@@ -138,36 +145,42 @@ 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.
+**Verdict:** Confirmed more broadly than the preliminary hypothesis.
 
-**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.
+**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 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.
+**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 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.
+**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 duplicated filesystem test machinery;
-the overall test-to-production ratio is not itself a defect.
+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
 
@@ -281,28 +294,31 @@ not the stage/adapter boundary.
 
 **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.
+**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 and package-global function substitution, not from the suite as
-a whole.
+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:** Already satisfied, except for one implementation mismatch.
+**Verdict:** Confirmed for filesystem policy; otherwise already satisfied.
 
-**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.
+**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:** 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.
+**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
 
@@ -310,8 +326,9 @@ in owner documents and protected by focused tests. Restore planning's use of
 
 **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.
+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
@@ -320,28 +337,40 @@ separate from refactors.
 
 ### H20 — Replace custom confinement with `os.Root`
 
-**Verdict:** Already substantially satisfied.
+**Verdict:** Focused opportunity, not a package-wide filesystem model.
 
-**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.
+**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:** 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.
+**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:** Retain.
+**Verdict:** Not substantiated as stated; retain only focused product policies.
 
-**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.
+**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:** 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.
+**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
 
@@ -432,14 +461,20 @@ still requiring the existing semantic checks.
 
 ### H29 — Generic atomic-file package
 
-**Verdict:** Not substantiated.
+**Verdict:** Focused opportunity, but no new dependency is presently justified.
 
-**Recommended change:** Retain and simplify `internal/fileops` rather than
-replacing it.
+**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:** 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.
+**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
 
@@ -637,33 +672,42 @@ navigation without distributing transaction ownership.
 
 ### H45 — `internal/fileops/directory.go`
 
-**Verdict:** Confirmed; see H06 and H20.
+**Verdict:** Confirmed for removal; see H06 and H21.
 
-**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.
+**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:** This is the clearest actual code duplication found. It also
-causes tests to exercise a different install implementation from production.
+**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:** 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.
+**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 restore and confined-filesystem contracts. The rest of the function is
-an explicit and readable state matrix whose branching is justified.
+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`
 
@@ -679,29 +723,31 @@ is easier to audit in one owner.
 
 ### H48 — Preliminary priority order
 
-**Verdict:** Confirmed with one correction.
+**Verdict:** Confirmed with a broader filesystem 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.
+**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:** 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.
+**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. Correct restore target confinement and add regression tests.
+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. Consolidate fileops on the production `os.Root` path and revise its tests and
-   internal documentation to the trusted-environment threat model.
+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
@@ -712,6 +758,7 @@ not staged implementation instructions:
 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.
+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.