86 lines
5.0 KiB
Markdown
86 lines
5.0 KiB
Markdown
# 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.
|