Files
commandkit/docs/roadmap/remediation.md

8.8 KiB
Raw Blame History

Implementation efficiency remediation

Status: one open P3 efficiency finding. Reviewed source: 1cba65fffe966cb70f48c324baf551502544cbad.

Scope and architecture

This review follows the completed implementation plan, Stage 1 through Stage 23, and its accepted target. Core owns declarations, routing, typed input preparation, completion and reporting, with Cobra/pflag behind private adapters. ckconfig and ckprompt adapt upstream configuration and prompt capabilities; applications own loaders, builders, runtime resources and executable lifecycle. The internal overview inventories the implemented owners.

The review examined source-level algorithmic work introduced or changed by the sprint. Expressly selected algorithms were not reconsidered: per-context alias memoization and source-tree traversal, dense occurrence reconstruction and heap merging, independent input copies, whole-report buffering/normalization, and native upstream algorithms remain selected. The deferred selector/scope optimizations were not reopened. R1 concerns incomplete implementation of the selected scratch path, rather than proposing a different algorithm or public contract.

Stage-by-stage assessment

Stage Assessment
1 Aggregate smoke coordination has four selected kinds; its new cancellation accounting introduces no growing cross-product.
2 The known CLI JSON copier visits each object entry/array element once per required destination. No additional finding.
3 Deferred acceptance closure is constant work; the accepted batch is processed once under the existing lifecycle.
4 The missing prepared-result guard is constant work.
5 The cancellation checkpoint preserves the selected buffered show preparation; no new repeated traversal was introduced.
6 Fixture/default corrections introduce no material algorithm.
7 Releasing the two direct writer references is constant work.
8 Shared detail rendering visits each detail once, excluding application callback work.
9 Provider-interface and conversion-contract changes introduce no replacement algorithm.
10 Naming documentation leaves runtime validation unchanged.
11 Diagnostic capture copies native records, source entries and related locations proportionally to that evidence, then partitions records once.
12 ckprompt delegates to that same adapter; it adds no second capture/partition pass.
13 Removed execution-resource plumbing introduces no new traversal or cache.
14 Shared token consumption advances through short spellings and consumed values; it does not rescan each complete short cluster.
15 Implementation retains the selected context-local memoization and source ordering. No residual independent suffix walks were found.
16 Implementation uses the selected resolver tree and owned-path results for distinct-prefix discovery and descendant lookup. Those selected algorithms were not reopened.
17 R1: a child's grown path slice is not retained for siblings, allowing repeated deep-prefix copying.
18 Operation-preparation extraction preserves the selected independent inputs and existing preflight rules; no new repeated algorithm was introduced.
19 Capture extraction preserves existing selector/scope processing. The explicitly deferred indexing work remains outside this audit.
20 Operand/error attribution extraction preserves the selected dense reconstruction and replay merge, without introducing general sorting.
21 Native pointer decoding splits and replaces escapes linearly in pointer bytes, without revalidation.
22 Documentation/navigation corrections introduce no runtime algorithm.
23 Verification/status recording introduces no runtime algorithm; passing checks do not establish R1's complexity improvement.

R1 — P3: Declaration validation can still copy a shared deep prefix for every sibling

Code: command.go, collectDeclarationIssues lines 450–479, particularly append at line 456 and recursive calls at line 478. validateCommandPlacement starts the walk with nil parents; installation reaches it through the candidate validator.

Selected direction: Stage 17 and catalog F22 select one append/truncate scratch path and lazy diagnostic formatting. The original efficiency finding E4 describes O(N) successful traversal with O(D) scratch storage. Lazy formatting was implemented, and a single deep chain benefits from ordinary append growth. The slice header, however, is local to each recursive call:

path := append(parents, record.name)
// ...
for _, child := range record.children {
    // ...
    collectDeclarationIssues(child, path, false, issues)
}

Circumstances and complexity: consider a valid declaration tree with a shared prefix of D nodes and R leaf children below its final node. Choose a branching depth at which that node's path has length equal to capacity. Such depths arise during ordinary append growth; no invalid declaration or emitted error is needed.

Each leaf appends to a full D-element slice, allocating another backing array and copying D ancestral tokens. The grown header exists only inside the leaf's call. Its parent continues passing the original full slice to every sibling, so the same prefix is copied R times. That portion performs Θ(DR) token copying and allocates Θ(DR) string slots cumulatively across R growth allocations, despite only N = D + R command nodes and no diagnostic output. Taking D and R of comparable size along such capacity boundaries gives Θ(N²) cumulative work. Complexity counts tokens independently of command-name byte costs; it does not claim quadratic retained live memory.

Avoiding eager String formatting solves part of E4, but does not make this a single reusable traversal stack. Validating a wide branch can repeatedly pay for the same ancestral path. Other construction phases retain their own path costs; this finding is confined to collectDeclarationIssues.

Potential remediation: keep the scratch slice header in one traversal-owned variable captured by the recursive visitor, or pass its address. Push a node name on entry and truncate back to the previous length on every exit, retaining the newly grown capacity across siblings. An alternative is to return the grown slice header and preserve its capacity at each caller, although that adds more bookkeeping. A closure over one ordinary slice is the simpler option.

With the selected shared stack, the successful walk takes O(N) work with O(D) scratch storage under amortized append growth. Failure paths additionally pay for labels actually emitted. Preserve child/issue order, retained error identity, root-only placement checks, absent/invalid declaration handling and owned error strings. This needs neither a public API change nor restrictions on declaration size or depth. It completes the selected design without promising that all App construction becomes linear.

Verification and status: a temporary public consumer installed a valid 32-node shared prefix with 64 leaf siblings and dispatched a leaf successfully. The prefix depth was selected at an ordinary slice-capacity boundary. This confirms that the workload shape is accepted; it is not a timing or allocation measurement. The copying bound follows from the recursive slice-header ownership and append semantics above.

Use existing declaration/registry behavioral checks to preserve results, and review that grown capacity survives sibling traversal. Include a branching shape when checking the remedy; a chain alone misses this case. No timing threshold or benchmark is necessary. Stage 17 and F22 are currently marked complete; their completion evidence should be corrected to account for this remaining case when remediation is performed.

Validation and limits

The review used graph symbol discovery/call tracing and source inspection against the unchanged audited commit. The aligned workspace suites passed using the cached Go 1.27.0 binary with GOTOOLCHAIN=local, GOPROXY=off, and go test -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/.... Cached successful results were reused for unchanged code. Those suites validate behavior, not asymptotic copy costs.

No production code, repository tests, manifests or existing roadmap records were modified. No benchmarks, performance profiles, real-world latency/memory data, external services, credentials or publication operations were used. No additional ordinary correctness defect was established during this algorithmic review.