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.
|