170 lines
10 KiB
Markdown
170 lines
10 KiB
Markdown
# Implementation efficiency remediation
|
||
|
||
Status: R1 complete; no open findings. Audited source before remediation:
|
||
`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 complete:** the traversal now retains grown scratch-path capacity across siblings. |
|
||
| 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
|
||
|
||
**Complete.** validateCommandPlacement now owns one path slice captured by its
|
||
recursive collectDeclarationIssues visitor. Each node appends its name and
|
||
defers truncation to its entry length, including on invalid/absent early returns.
|
||
Truncation preserves the grown slice header and capacity for subsequent siblings.
|
||
Successful traversal therefore takes O(N) amortized path work with O(D) scratch
|
||
storage; diagnostic labels are still formatted only when emitted. Child/issue
|
||
order, error wrapping, root-only checks and declaration handling are preserved.
|
||
See [remediation verification](#remediation-verification) for the checks performed.
|
||
|
||
The analysis below records the defect at the audited revision.
|
||
|
||
**Audited 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.
|
||
|
||
**Audit verification:** 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.
|
||
|
||
At audit time, Stage 17 and F22 were marked complete without accounting for this
|
||
branching case. Their completion evidence now records the shared slice remedy
|
||
and links to this follow-up.
|
||
|
||
## 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.
|
||
|
||
During the audit, 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.
|
||
|
||
## Remediation verification
|
||
|
||
The focused declaration/registry checks passed uncached, including retained
|
||
causes, absence, invalid declarations, nested root-only groups and transactional
|
||
installation. The independent core suite and aligned three-module suite also
|
||
passed offline using cached Go 1.27.0 with `GOTOOLCHAIN=local`, `GOPROXY=off` and
|
||
`-mod=readonly`. The aligned command was
|
||
`go test -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/...`;
|
||
the independent core check used `GOWORK=off go test -mod=readonly ./...`.
|
||
|
||
The temporary public consumer was rerun successfully with a valid depth-32 prefix
|
||
and 64 leaf siblings. This checks installation and dispatch for the branching
|
||
shape. Source inspection establishes the complexity improvement: all recursive
|
||
calls update the same slice header, growth survives return, and each node's
|
||
deferred truncation restores its parent's path on every exit. Owned diagnostic
|
||
strings are constructed before sibling traversal can change the scratch path.
|
||
|
||
Existing behavioral tests were reused; no timing/allocation threshold or new
|
||
benchmark was introduced. These results concern this validation walk; other
|
||
construction phases retain their own costs.
|