From e46d28a92460f985a70c6e22f790d410c8c909f7 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sat, 7 Nov 2026 16:52:58 +0000 Subject: [PATCH] Record the remaining declaration validation inefficiency --- docs/roadmap/remediation.md | 139 ++++++++++++++++++++++++++++++++++++ 1 file changed, 139 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..581a233 --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,139 @@ +# Implementation efficiency remediation + +Status: one open P3 efficiency finding. Reviewed source: +`1cba65fffe966cb70f48c324baf551502544cbad`. + +## Scope and architecture + +This review follows the completed [implementation plan](implementation.md), +Stage 1 through Stage 23, and its [accepted target](audit-remediation.md). +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](../internal/overview.md) 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](future.md#possible-further-efficiency-work) +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](../../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](implementation.md#stage-17--format-declaration-paths-only-for-diagnostics) +and [catalog F22](audit/catalog.md#f22--p3-successful-declaration-validation-eagerly-formats-every-full-path) +select one append/truncate scratch path and lazy diagnostic formatting. The +[original efficiency finding E4](audit/02-efficiency.md#e4--p3-successful-declaration-validation-eagerly-constructs-every-full-path) +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: + +```go +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.