Files
commandkit/docs/roadmap/audit/03-complexity.md

15 KiB
Raw Blame History

Complexity and maintainability audit

Status: seven open P3 refactoring opportunities. Reviewed source: 1e4fb73d159e2fa3ea0a3591eb56624138e613f6.

Architecture and review method

The internal overview 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 and accepted ADRs 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 and 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, 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, 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, interpretFlagTokens lines 10–103; completion_flags.go, walkCompletionFlags lines 43–98 and completionOptionTarget lines 130–175; shared flag helpers.

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, prepareInputWithOccurrences lines 252–338; 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, configKitDiagnosticsWithHeading and its cloning/rendering helpers, lines 137–209; adapters.go, promptKitDiagnostics and writePromptKitDiagnostics, lines 513–572. Related scheduling copies are queueConfigDiagnostics in operations.go and queuePromptDiagnostics in 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, ExplanationReader lines 76–80 and View.Explanation; consumers in explain.go and show.go. The direct writer invocation in 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. 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, 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.