260 lines
15 KiB
Markdown
260 lines
15 KiB
Markdown
# Complexity and maintainability audit
|
||
|
||
Status: seven open P3 refactoring opportunities. Reviewed source:
|
||
`1e4fb73d159e2fa3ea0a3591eb56624138e613f6`.
|
||
|
||
## Architecture and review method
|
||
|
||
The [internal overview](../../internal/overview.md) inventories the implemented
|
||
owners. Core owns captured declarations, installation, routing, typed input
|
||
preparation, completion, reporting, and outcomes. Neutral metadata separates
|
||
those responsibilities from the private Cobra/pflag adapter. ckconfig owns
|
||
configuration command assembly, scope/input coordination, and ConfigKit report
|
||
adaptation. ckprompt owns PromptKit command assembly and native operation/report
|
||
adaptation and depends on public core and ckconfig contracts. Applications own
|
||
loaders, builders, runtime dependencies, and executable lifecycle; upstream
|
||
libraries own domain algorithms. Release tooling delegates to ReleaseKit.
|
||
|
||
The review used codebase-memory-mcp's graph complexity/cognitive metrics, symbol
|
||
search, source snippets, and call traces, followed by direct source review.
|
||
Production and test symbols were considered separately; excluded examples and
|
||
launcher tests were inspected directly. The indexed Go source matches the
|
||
reviewed revision. Scores below are the extractor's `complexity` and `cognitive`
|
||
values, not portable thresholds or proof of a defect. Nested callbacks and
|
||
branch-heavy type handling can raise a score without justifying a refactor.
|
||
|
||
Representative candidates from the metric pass:
|
||
|
||
| Function | Function span | Complexity | Cognitive |
|
||
| --- | --- | ---: | ---: |
|
||
| ckconfig.captureSuite | family.go:86–211 | 31 | 78 |
|
||
| walkCompletionFlags | completion_flags.go:43–98 | 16 | 61 |
|
||
| cobra.interpretFlagTokens | internal/cobra/flags.go:10–103 | 16 | 51 |
|
||
| prepareInputWithOccurrences | binding.go:252–338 | 21 | 45 |
|
||
| ckconfig.prepareConfigOperation | input.go:196–317 | 25 | 38 |
|
||
|
||
The findings below identify responsibilities or policies that could be made
|
||
easier to maintain, rather than asking for functions to meet a numeric limit.
|
||
The [architecture policy](../../policy/architecture.md) and accepted
|
||
[ADRs](../../adr/README.md) remain authoritative: retain separate optional
|
||
modules, typed public contracts, backend isolation, upstream domain ownership,
|
||
and operation/presentation error separation. Avoid a universal operation engine,
|
||
new declaration language, or generic provider framework merely to reduce lines.
|
||
|
||
No production code or repository tests were changed. No runtime tests,
|
||
benchmarks, or performance measurements were run for this static audit.
|
||
Documentation links, source locations, whitespace, and the final diff were
|
||
checked. No additional ordinary correctness defect was established. The open
|
||
findings in [01-correctness.md](01-correctness.md) and
|
||
[02-efficiency.md](02-efficiency.md) remain separate; these refactors do not
|
||
constitute fixes for those findings.
|
||
|
||
## M1 — P3: Scope capture combines several compilation phases
|
||
|
||
**Code:** [family.go](../../../integrations/ckconfig/family.go), captureSuite
|
||
lines 86–211.
|
||
|
||
**Complexity:** the highest reported production cognitive score corresponds to
|
||
126 lines that copy selectors, rename comparison bindings, normalize and validate
|
||
scopes, collect retained errors, validate provider wiring, establish scope order
|
||
and indexes, deduplicate selector scope references, attach bindings to scopes,
|
||
derive operation-wide binding unions, and build discovery inventory. The nested
|
||
selector/scope/operation branches require tracking several partially populated
|
||
collections at once. A new operation or selector rule can affect multiple
|
||
regions of the same function.
|
||
|
||
**Potential refactoring:** make captureSuite an explicit sequence of private
|
||
phases: capture/validate scope declarations; establish ordered scope metadata;
|
||
capture/validate selectors; attach scope and comparison bindings; derive the
|
||
operation unions and inventory. Extract complete responsibilities with clear
|
||
inputs/results, reusing preparedSuite and preparedScope where appropriate.
|
||
Keep small related loops together instead of creating a helper for every branch.
|
||
|
||
Preserve declaration ownership, retained error order, empty-suite behavior,
|
||
scope order, selector/binding identity, comparison requiredness, and metadata-only
|
||
construction. This opportunity does not require selector-membership indexes or
|
||
reopening the deferred efficiency work.
|
||
|
||
## M2 — P3: Configuration preflight mixes shared and operation-specific rules
|
||
|
||
**Code:** [input.go](../../../integrations/ckconfig/input.go),
|
||
newConfigOperationCommand lines 61–170, configOperationPlan lines 40–59, and
|
||
prepareConfigOperation lines 196–317.
|
||
|
||
**Complexity:** one command builder and one preparation function cover five
|
||
operations, with fields for several distinct submodes in the same private input
|
||
and plan. Preparation interleaves flag-combination rules, explain-help selection,
|
||
scope selection, capability checks, pointer parsing, plan construction, ordinary
|
||
scope input preparation, and independent comparison-side preparation. Understanding
|
||
one path requires determining which fields and branches are irrelevant to it.
|
||
Changing show, explain, or comparison behavior requires reasoning about the same
|
||
large control flow and its factory/check ordering.
|
||
|
||
**Potential refactoring:** separate operation-specific option preparation from
|
||
shared scope/capability selection, and isolate comparison input preparation from
|
||
ordinary selected-scope preparation. Give meaningful groups of operation flags
|
||
small private constructors where that makes the declarations easier to scan.
|
||
Start with functions over the existing types; split private option records only
|
||
where that materially clarifies which data a handler consumes. A generic rule
|
||
registry or elaborate plan hierarchy is unnecessary.
|
||
|
||
Keep the current ordering of preflight, input factories, binding application,
|
||
input checks, state creation, and acquisition. Preserve explain's help path,
|
||
supplied-state distinctions, full aggregate capability checks, independent
|
||
comparison selections, and existing failure classifications.
|
||
|
||
## M3 — P3: Parsing and completion maintain parallel flag-consumption logic
|
||
|
||
**Code:** [flags.go](../../../internal/cobra/flags.go), interpretFlagTokens lines
|
||
10–103; [completion_flags.go](../../../completion_flags.go), walkCompletionFlags
|
||
lines 43–98 and completionOptionTarget lines 130–175;
|
||
[shared flag helpers](../../../internal/commandmeta/flags.go).
|
||
|
||
**Complexity:** token classification, long-flag splitting, and spelling lookup
|
||
are already shared, but the consumers separately implement the remaining grammar:
|
||
short-cluster traversal, boolean `=value`, attached nonboolean values, consumption
|
||
of the next token, option-looking values, unknown spellings, and terminators.
|
||
The completion functions add partial-token handling to substantially the same
|
||
branch structure. Grammar maintenance therefore crosses the public core package
|
||
and the internal backend adapter, despite their deliberate neutral boundary.
|
||
|
||
**Potential refactoring:** extend the existing internal neutral helpers with a
|
||
small token-consumption primitive that reports the reached flag, attached value
|
||
or remaining suffix, and whether another token is needed. Keep normalization,
|
||
occurrence/spelling recording, backend failure handling, and completion choices
|
||
in their current owners. Share the consumption facts rather than forcing both
|
||
callers through one policy-heavy parser.
|
||
|
||
Completion must continue to accept an unfinished current word or a pending value
|
||
where execution would fail. Preserve the backend's consumed failure prefix,
|
||
entered spelling provenance, short-cluster boundaries, explicitly empty values,
|
||
and `--` behavior. This can remain entirely internal; no public parser API or
|
||
backend type exposure is needed.
|
||
|
||
## M4 — P3: Typed preparation combines conversion, provenance, and replay assembly
|
||
|
||
**Code:** [binding.go](../../../binding.go), prepareInputWithOccurrences lines
|
||
252–338; [flag_values.go](../../../flag_values.go), CaptureFlags and applyTo.
|
||
|
||
**Complexity:** prepareInputWithOccurrences validates parallel input shapes,
|
||
converts flags, translates indexed conversion errors into entered spellings,
|
||
assigns occurrence ordinals, reconstructs dense application order, tracks required
|
||
flags, allocates operands, converts arguments, and assembles prepared callbacks.
|
||
Deeply nested error/provenance branches sit inside the conversion loop. Captured
|
||
bindings retain shared ordinal references for later subset replay, so changing
|
||
this code also requires understanding the separate FlagValues path.
|
||
|
||
**Potential refactoring:** separate entered-spelling error translation,
|
||
flag/occurrence preparation, and argument preparation into cohesive private
|
||
functions. Leave a short orchestration function that makes conversion-before-input
|
||
construction and the application order visible. Keep ordinal assignment next to
|
||
the preparation that owns its identity, with a short explanation of how captured
|
||
streams reuse it.
|
||
|
||
Retain the selected dense reconstruction and heap merge, parser-log versus
|
||
fallback ordering, original binding identity, deferred required checks, and
|
||
conversion error identity. Do not introduce a generic event-processing framework
|
||
or replace the already remediated algorithms to lower the complexity score.
|
||
|
||
## M5 — P3: Configuration diagnostic adaptation is maintained in both integrations
|
||
|
||
**Code:** [configkit_adapter.go](../../../integrations/ckconfig/configkit_adapter.go),
|
||
configKitDiagnosticsWithHeading and its cloning/rendering helpers, lines 137–209;
|
||
[adapters.go](../../../integrations/ckprompt/adapters.go), promptKitDiagnostics
|
||
and writePromptKitDiagnostics, lines 513–572. Related scheduling copies are
|
||
queueConfigDiagnostics in [operations.go](../../../integrations/ckconfig/operations.go)
|
||
and queuePromptDiagnostics in [command.go](../../../integrations/ckprompt/command.go).
|
||
|
||
**Complexity:** both native adapters snapshot ConfigKit sources and diagnostic
|
||
locations, partition error records from ordinary messages, create deferred detail
|
||
callbacks, print a source legend, and delegate record formatting to ConfigKit.
|
||
These are the same upstream data types and closely matching presentation rules.
|
||
The two integrations also have matching severity-to-Reporter scheduling switches.
|
||
Changes to evidence capture, source labeling, or diagnostic scheduling need
|
||
coordinated maintenance across separate modules.
|
||
|
||
**Potential refactoring:** consider one narrow, supported ConfigKit-diagnostic
|
||
adaptation function owned by ckconfig, returning the existing core Diagnostic
|
||
and Explanation values. ckprompt already depends on public ckconfig and could
|
||
reuse it while retaining its operation-specific fallback explanations and supplied
|
||
heading/side labels. This would consolidate the substantial native adaptation
|
||
without making core depend on ConfigKit or exposing integration internals.
|
||
|
||
The smaller scheduling duplication could separately use a narrow core reporting
|
||
helper if the public API benefit justifies it; it does not justify a new shared
|
||
module or universal diagnostics framework. Treat any new exported helper as an
|
||
API decision, not an automatic consequence of deduplication. Preserve snapshot
|
||
ownership, record order, error identity, independent explanation/message evidence,
|
||
and immediate progress versus deferred ordinary diagnostic timing.
|
||
|
||
## M6 — P3: Explanation providers must implement an unused rendering capability
|
||
|
||
**Code:** [contracts.go](../../../integrations/ckconfig/contracts.go),
|
||
ExplanationReader lines 76–80 and View.Explanation; consumers in
|
||
[explain.go](../../../integrations/ckconfig/explain.go) and
|
||
[show.go](../../../integrations/ckconfig/show.go). The direct writer invocation
|
||
in [contracts_adapter_test.go](../../../integrations/ckconfig/contracts_adapter_test.go)
|
||
is at line 121.
|
||
|
||
**Complexity:** ExplanationReader requires both Explain and WriteExplanation.
|
||
After the completed acquired-report remediation, production handlers use only
|
||
Explain: core renders the selected report and its text template. Graph tracing
|
||
and a repository-wide source search found no production WriteExplanation call;
|
||
the remaining call is an adapter test, with additional test implementations.
|
||
Custom providers must still supply a renderer that standard commands never use,
|
||
and the contract suggests a parallel presentation responsibility.
|
||
|
||
**Potential refactoring:** consider narrowing the provider requirement to native
|
||
explanation acquisition and retiring the unused writer requirement from the
|
||
CommandKit-facing interface. Native ConfigKit/PromptKit objects can retain their
|
||
own rendering methods. Update provider examples, the direct adapter assertion,
|
||
and the interface's GoDoc consistently if that change is selected.
|
||
|
||
This is a public contract opportunity, not dead code that can be removed without
|
||
review. External consumers may call WriteExplanation through View.Explanation;
|
||
repository searches cannot establish otherwise. Account for that source impact
|
||
under the [compatibility policy](../../policy/release.md#versioning-and-compatibility).
|
||
Do not reinstate provider-owned text rendering merely to give the method a caller.
|
||
|
||
## M7 — P3: Terminal and ordinary messages duplicate detail-rendering mechanics
|
||
|
||
**Code:** [reporter.go](../../../reporter.go), renderTerminalLocked lines 393–423
|
||
and submitMessageLocked lines 439–491; recordRendererErrorLocked and
|
||
combineOperationErrors supply the common error classification.
|
||
|
||
**Complexity:** both paths skip nil detail callbacks, create a coordinated writer,
|
||
invoke each callback, distinguish its rendering error from its writer error,
|
||
retain the first of each, and combine the results. Ordinary messages add the
|
||
hidden-detail/io.Discard branch. This duplicates delicate bookkeeping inside a
|
||
reporter that already coordinates visibility, locking, deferred delivery, and
|
||
outcome finalization. The concern is duplicated policy, rather than file length
|
||
or a large score for either individual function.
|
||
|
||
**Potential refactoring:** extract a private detail-rendering helper that runs
|
||
under the existing reporting lock and returns separate rendering and writing
|
||
errors. Let the callers retain headings, visibility selection, leading-message
|
||
failure handling, and destination delivery-state updates. Keep the visible versus
|
||
discarded writer choice explicit so hidden details still run as selected.
|
||
|
||
Preserve nil-detail handling, callback order/lifetime, sticky writer failures,
|
||
joined error evidence, non-reentrancy, and terminal explanation timing. Keep one
|
||
coordination boundary; additional locks or an interchangeable rendering pipeline
|
||
would add complexity. This extraction is independent of the deferred-finalization
|
||
correctness finding in the earlier audit.
|
||
|
||
## Assessment and suggested order
|
||
|
||
M1, M2, and M4 are local extractions with clear phase boundaries. M7 consolidates
|
||
one existing internal policy. M3 has broader grammar impact and should retain
|
||
distinct execution and completion policies. M5 and M6 need deliberate public
|
||
contract decisions because the integrations are independently released modules.
|
||
|
||
Other high-scoring areas were examined without additional findings. The finite
|
||
alias termination proof has inherent algorithmic complexity and an expressly
|
||
selected design. YAML conversion's type/tag and alias checks protect selected
|
||
representation rules; replacing them with a generalized conversion framework
|
||
would not be a demonstrated simplification. The large interactive completion test
|
||
combines several necessary shell cases, while already sharing its terminal driver.
|
||
The composed example's larger functions mostly contain explicit wiring and fixture
|
||
data. None warrants a rewrite solely because of a score or line count.
|