From 9545bb0bfc60d281fea32dde3ec565eb21847a4b Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 2 Nov 2026 01:39:06 +0000 Subject: [PATCH] Record complexity audit findings --- docs/roadmap/audit/03-complexity.md | 85 +++++++++++++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 docs/roadmap/audit/03-complexity.md diff --git a/docs/roadmap/audit/03-complexity.md b/docs/roadmap/audit/03-complexity.md new file mode 100644 index 0000000..67308e7 --- /dev/null +++ b/docs/roadmap/audit/03-complexity.md @@ -0,0 +1,85 @@ +# Complexity audit + +Reviewed baseline: `620c832` on `main`. The review used the local codebase +knowledge graph's cognitive and cyclomatic complexity estimates to identify +candidates, then inspected their code, callers, package boundaries, and +documented contracts. Scores are heuristics, not defect or refactoring +thresholds. No empirical performance measurements or application tests were +run for this documentation-only review. Existing correctness, concurrency, +security, and efficiency findings are recorded in the neighboring audits. + +## 1. Shared validation combines several kinds of orchestration + +[validation.Run](../../../internal/validation/validation.go) has a graph +cognitive score of 78 and cyclomatic score of 37. It validates settings, +acquires the Go tool, runs tests, vet and compilation, decides whether to +load a committed tree, chooses an error identity for a failed tree load, runs +file checks, and attributes a deferred reader-close error. The tree-load +condition, the reverse-priority error selection, and the ordered check blocks +each separately enumerate selections. Adding a check therefore requires a +maintainer to update several distant parts while preserving ordering and +close-error attribution. + +Consider separating the Go-command sequence and the tree-check sequence into +small private functions, with the tree owner responsible for closure and its +failure attribution. Keep the selected checks and their order visible in one +place; a generic plugin system is unnecessary. Preserve nil versus empty shared +file selections, the current first-failure identities and precedence, and +the fact that a close failure replaces success only after a content read. + +## 2. Git source inspection is repeated across package boundaries + +[build.BuildResolved](../../../internal/build/build.go) uses its own +`gitCommit` and `cleanSource` subprocesses before and after building. +[candidate](../../../internal/candidate/check.go) separately reads branch, +commit and status, while [git](../../../internal/git/source.go) has +`CaptureHead` and another tagged-source inspection path. These overlap in +commands and interpretation but use different execution and error handling. +Understanding a source-state change or a subprocess fix requires tracing all +three owners. The existing [correctness audit](01-correctness.md) identifies +replacement-object and cancellation concerns that make this duplication +particularly relevant. + +Consider moving the common commit and worktree-status primitives behind the +existing internal Git boundary, with callers composing only the observations +they need. Candidate checking still needs branch and tag rules; building still +records dirty source rather than rejecting it; delivery still checks a tag. +Preserve these different contracts, error context, cancellation behavior, and +the timing of before/after observations. Coordinate any such refactor with the +existing correctness work instead of treating code consolidation as its fix. + +## 3. Internal build results retain duplicate settings + +[internal/build.Result](../../../internal/build/build.go) stores normalized +`Settings` alongside separate `MainPackage`, `BinaryName`, and `Targets` +fields containing the same information. `BuildResolved` initializes both, +and [facade conversion](../../../build.go) reconstructs both on input while +reading `Settings` on output. Delivery uses `Settings` for identity and asset +validation. This creates two sources of truth inside the implementation and +additional work when a build setting is added; a future caller could populate +them inconsistently. + +Consider retaining `Settings` as the single internal representation and +updating internal build execution to use it. Keep the existing public +`BuildResult` fields and the CLI's private receipt format at their respective +boundaries; their distinct types and conversions serve deliberate ownership +and serialization requirements. Verify partial build results and prebuilt +delivery remain unchanged. This is a simplification opportunity, not an +observed mismatch in current callers. + +## 4. CLI command handling interleaves routing and command-specific workflows + +[run](../../../cmd/releasekit/main.go) has a graph cognitive score of 85 and +combines routing with profile loading, publication and check reporting, build +execution, receipt invalidation and verification, and exit-code decisions. +The same file also contains delivery preparation and receipt conversions. +The build branch alone has several separate failure paths that must be read +alongside its success and partial-result output; changes to one command add +noise to the central dispatcher. + +Consider keeping `run` as a small dispatcher and moving the check/publish and +build command workflows into named private handlers, grouped by responsibility. +Keep receipt invalidation before build-error reporting, existing messages and +exit statuses, and the CLI's use of the public orchestration facade. Existing +delivery handlers provide a local example of this separation. This does not +require a new command framework or public interfaces.