Record maintainability audit hypotheses
This commit is contained in:
224
docs/roadmap/potential-audit.md
Normal file
224
docs/roadmap/potential-audit.md
Normal file
@@ -0,0 +1,224 @@
|
||||
# Potential Maintainability Audit
|
||||
|
||||
## Status
|
||||
|
||||
Initial impressions only. This document does not define action items, commit to
|
||||
an audit, or recommend specific refactoring. Its purpose is to preserve signals
|
||||
that may warrant investigation in a subsequent, deeper pass.
|
||||
|
||||
## Context
|
||||
|
||||
Narratio contains approximately 36,000 lines of production Go and 45,000 lines
|
||||
of test Go across 26 packages. That footprint appears broadly consistent with a
|
||||
durable workflow orchestrator that owns configuration, pipeline sequencing,
|
||||
artifact and manifest state, recovery, storage, subprocess integration, and
|
||||
filesystem safety. It is larger than would be expected of a simple command
|
||||
wrapper, but the current size alone is not evidence of a maintainability
|
||||
problem.
|
||||
|
||||
Narratio handles non-sensitive pipeline data and is expected to run in a
|
||||
trusted environment. A future review therefore need not assume an adversary who
|
||||
can manipulate workspace paths, symlinks, or directory entries concurrently
|
||||
with the process. Protection against accidental path escape and destructive
|
||||
operator mistakes remains useful, as do atomic replacement, crash durability,
|
||||
and recoverable state transitions. The distinction between adversarial
|
||||
filesystem hardening and crash-safe data integrity should guide any proposed
|
||||
simplification.
|
||||
|
||||
## Preliminary Signals
|
||||
|
||||
The following observations may justify a focused maintainability audit:
|
||||
|
||||
- Production code is concentrated in `internal/stage`, `internal/app`, and
|
||||
`internal/config`. These packages contain approximately 8,300, 7,900, and
|
||||
5,500 lines respectively.
|
||||
- Several production files exceed 1,000 lines, including stage publishing and
|
||||
analysis, configuration validation, and application runner code. File size
|
||||
does not establish poor cohesion, but it can conceal multiple responsibilities
|
||||
or make change impact harder to assess.
|
||||
- The application, stage, and artifact areas have substantial interaction.
|
||||
Their effective boundaries and the blast radius of changes may merit closer
|
||||
examination.
|
||||
- The most notable function-level complexity observed so far is concentrated in
|
||||
confined filesystem and directory-promotion code. This is security-sensitive
|
||||
logic built around a stronger threat model than Narratio now requires. A
|
||||
review could determine which mechanisms exist only to resist adversarial
|
||||
filesystem races and which are necessary for path safety, atomic updates, and
|
||||
crash-safe integrity.
|
||||
- Test code is larger than production code. This is a positive verification
|
||||
signal, but a deeper pass could determine whether the suite also contains
|
||||
repetition, brittle implementation coupling, or fixture and helper sprawl.
|
||||
|
||||
## Counter-Signals
|
||||
|
||||
The initial review also found evidence against a conclusion of broad or urgent
|
||||
maintainability trouble:
|
||||
|
||||
- Repository-wide tests and `go vet` pass.
|
||||
- Package dependencies are acyclic.
|
||||
- Average function complexity is modest, with higher complexity appearing
|
||||
localized rather than systemic.
|
||||
- The direct dependency set is small.
|
||||
- Documentation is structured around explicit ownership and reading paths.
|
||||
- The repository has substantial automated test coverage by volume.
|
||||
|
||||
## Questions for a Deeper Pass
|
||||
|
||||
A later audit could investigate, without presuming that changes are required:
|
||||
|
||||
- whether responsibilities within `internal/app`, `internal/stage`, and
|
||||
`internal/config` remain cohesive;
|
||||
- whether the largest files contain natural internal modules that are currently
|
||||
obscured;
|
||||
- whether stage and external-adapter implementations contain meaningful
|
||||
duplication;
|
||||
- whether tests are organized around durable behavior or coupled to incidental
|
||||
implementation structure; and
|
||||
- whether manifest, restore, publishing, and confined-filesystem invariants are
|
||||
easy to locate, understand, and verify across package boundaries.
|
||||
|
||||
The deeper pass should also evaluate these specific modernization candidates:
|
||||
|
||||
- upgrading the maintained toolchain baseline from Go 1.25 to Go 1.27, subject
|
||||
to ordinary compatibility and release validation;
|
||||
- replacing custom root-confinement, no-follow traversal, safe cleanup, and
|
||||
confined-read machinery with the standard library's `os.Root` operations
|
||||
where their semantics match Narratio's reduced threat model;
|
||||
- retaining Narratio-owned code where it still provides application-specific
|
||||
crash durability, atomic replacement, no-replace directory promotion, mode
|
||||
policy, bounded reads, or destructive-operation guardrails; and
|
||||
- evaluating the `os.Process.WithHandle` API added in Go 1.26 as a way to avoid
|
||||
reopening a Windows child process by PID before assigning it to an owned job
|
||||
object, while recognizing that it may not replace suspended startup, job
|
||||
object management, or initial-thread resumption;
|
||||
- using the standard library's `slices` and `maps` packages to replace generic
|
||||
cloning, membership, key extraction, sorting, and deduplication helpers where
|
||||
those helpers do not encode domain-specific ordering or copy semantics;
|
||||
- using `golang.org/x/sync/errgroup`, including its bounded-concurrency support,
|
||||
to simplify transcription worker coordination, cancellation, and first-error
|
||||
propagation while retaining Narratio-owned result aggregation and error
|
||||
context;
|
||||
- adopting `github.com/google/go-cmp/cmp` selectively in tests where structural
|
||||
diffs would replace `reflect.DeepEqual` or hand-written comparisons with more
|
||||
useful failure output;
|
||||
- evaluating `github.com/gofrs/flock` after the `os.Root` design is understood,
|
||||
to determine whether it can replace Unix and Windows lock primitives and
|
||||
context-aware polling without losing lock metadata or desired file ownership
|
||||
semantics; and
|
||||
- considering a CLI framework such as Cobra only if command and flag evolution
|
||||
proves costly enough to justify migration risk, dependency growth, and
|
||||
possible changes to existing parsing, help, and error-output behavior.
|
||||
|
||||
The review may also document why a replacement is not appropriate. Current
|
||||
low-confidence candidates include general configuration frameworks or schema
|
||||
validators, generic atomic-file packages, provider-neutral object-storage
|
||||
abstractions, retry or backoff packages, UUID or ULID packages, and generic
|
||||
subprocess frameworks. Narratio's composition provenance, semantic validation,
|
||||
durability guarantees, conditional object writes, identifier needs, and
|
||||
tool-specific adapter contracts may leave little generic machinery for these
|
||||
libraries to replace.
|
||||
|
||||
## Preliminary Complexity Findings
|
||||
|
||||
Complexity metrics are heuristics rather than findings of defects. Guard
|
||||
clauses, validation branches, and explicit state transitions can score highly
|
||||
while remaining appropriate. The following production-only measurements are
|
||||
initial signals for focused code reading.
|
||||
|
||||
### Execution And Planning
|
||||
|
||||
`internal/app.executePlan` is the clearest outlier observed so far:
|
||||
|
||||
- approximately 456 lines;
|
||||
- cyclomatic complexity of 79;
|
||||
- cognitive complexity of 149; and
|
||||
- more than 60 direct collaborators in the code graph.
|
||||
|
||||
The function currently spans invocation preparation, collaborator construction,
|
||||
locking, session and run manifest initialization, resume decisions, stage
|
||||
transitions, invalidation, terminal-failure persistence, and post-publish
|
||||
cleanup. A deeper pass should look for existing lifecycle boundaries that could
|
||||
be made explicit without redesigning the state machine during extraction.
|
||||
|
||||
`internal/app.Plan` is approximately 110 lines with cyclomatic complexity 27
|
||||
and cognitive complexity 51. More importantly, it independently models parts
|
||||
of the run/skip, resume-validation, stale-marking, and invalidation behavior
|
||||
also present in execution. The audit should examine whether a shared, pure stage
|
||||
transition model could reduce drift between planning and execution.
|
||||
|
||||
### Analyze Orchestration
|
||||
|
||||
The analyze implementation has two notable orchestration functions:
|
||||
|
||||
- `analyzeStage.Run` is approximately 252 lines with cyclomatic complexity 33
|
||||
and cognitive complexity 47; and
|
||||
- `executeAnalyzeArtifact` is approximately 259 lines with cyclomatic
|
||||
complexity 28 and cognitive complexity 39.
|
||||
|
||||
`internal/stage/analyze.go` combines setup, catalog construction,
|
||||
reconciliation, planning, per-artifact execution, state projection, dependency
|
||||
invalidation, reuse aggregation, evidence registration, and result assembly.
|
||||
Possible seams to investigate include execution-state initialization,
|
||||
current/reused record projection, a per-artifact transaction, evidence
|
||||
registration, and final result accumulation.
|
||||
|
||||
### Configuration Validation And Composition
|
||||
|
||||
`internal/config/validate.go` contains approximately 1,100 lines and has the
|
||||
highest aggregate file complexity observed: cyclomatic complexity 264 and
|
||||
cognitive complexity 394 across its functions. Notable functions include:
|
||||
|
||||
- `validateScriptorium`: approximately 117 lines, cyclomatic complexity 32,
|
||||
cognitive complexity 74;
|
||||
- `validateNotarius`: approximately 129 lines, cyclomatic complexity 29,
|
||||
cognitive complexity 42; and
|
||||
- `validateAudita`: approximately 54 lines, cyclomatic complexity 21,
|
||||
cognitive complexity 34.
|
||||
|
||||
These scores are partly driven by explicit validation guards. A deeper review
|
||||
should determine whether artifact-level, dependency-graph, path/output,
|
||||
input-source, and variable validation can be separated while preserving precise
|
||||
field-specific errors.
|
||||
|
||||
Composition complexity is concentrated in `internal/config/composition.go` and
|
||||
`internal/config/pipeline_composition.go`. In particular,
|
||||
`splitPipelineCompositionEnvelope` is approximately 87 lines with cyclomatic
|
||||
complexity 21 and cognitive complexity 63. It combines YAML shape checking,
|
||||
source-aware diagnostics, profile parsing, default-profile validation, cloning,
|
||||
and removal of composition metadata. Structural decoding and semantic
|
||||
validation may be separable, but the audit should preserve source provenance
|
||||
and field ownership.
|
||||
|
||||
### Previous-Session Cache
|
||||
|
||||
`internal/previouscache.Resolve` is approximately 180 lines with cyclomatic
|
||||
complexity 29 and cognitive complexity 60, within a package of only about 483
|
||||
production lines. It combines configuration checks, current-state loading,
|
||||
required and optional behavior, immutable-commit resolution, legacy candidate
|
||||
probing, local-path construction, record creation, and sorting. Modern
|
||||
immutable-commit resolution and legacy probing are distinct algorithms and may
|
||||
form a useful extraction boundary.
|
||||
|
||||
### Additional Areas To Inspect
|
||||
|
||||
Lower-priority complexity concentrations include:
|
||||
|
||||
- `internal/stage/publish.go`, approximately 1,175 lines with aggregate
|
||||
cognitive complexity 239, where file collection, output resolution,
|
||||
publication, and commit assembly may be separable;
|
||||
- `internal/fileops/directory.go`, approximately 555 lines with aggregate
|
||||
cognitive complexity 184, some of which may be removed by the `os.Root`
|
||||
evaluation;
|
||||
- `internal/app/restore_plan.go`, particularly the approximately 129-line
|
||||
`classifyRestoreAction`; and
|
||||
- `internal/manifest/analyze_state.go`, whose moderately complex validation may
|
||||
be justified by its ownership of explicit state invariants.
|
||||
|
||||
On the initial evidence, execution and shared plan/execution transition logic
|
||||
should receive the first detailed review. Configuration and analyze
|
||||
orchestration form the next tier, while their higher complexity may more often
|
||||
reflect detailed domain rules.
|
||||
|
||||
These questions should be revisited with change-history analysis, focused code
|
||||
reading, and maintenance experience before forming recommendations or creating
|
||||
work items.
|
||||
Reference in New Issue
Block a user