Remove completed audit and remediation records
This commit is contained in:
@@ -1,276 +0,0 @@
|
||||
# Audit remediation roadmap
|
||||
|
||||
Status: source remediation complete; the [implementation plan](implementation.md)
|
||||
records the sprint and verification. Release and dependency alignment remain
|
||||
separate work in [future work](future.md#pending-release-and-integration-work).
|
||||
|
||||
Baseline: `d131999c7c474010a4814bb81601cd32920e6c83`. The
|
||||
[catalog](audit/catalog.md) records 27 distinct findings, five P2 and 22 P3,
|
||||
from seven audits. Its accepted decisions and the
|
||||
[remediation analysis](audit/remediation-analysis.md) supply the scope and
|
||||
rationale for this roadmap. The audited state was
|
||||
`db732e3a1de55c844f77870e1c6db683c2685d5d`.
|
||||
|
||||
This roadmap owns the target, scope, and completion criteria. The implementation
|
||||
plan owns ordered stages, dependencies, concrete changes, and verification steps.
|
||||
The catalog owns finding status and accepted remediation decisions;
|
||||
the original audit entries own detailed evidence. This is separate from the
|
||||
older, completed [shared-reporting remediation](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/264b82bd5759f052a3646d57828945a87b7586d3/docs/roadmap/remediation.md), whose findings
|
||||
are not reopened here. Unselected capabilities in [future work](future.md)
|
||||
remain outside this effort.
|
||||
|
||||
## Current baseline and targeted end state
|
||||
|
||||
CommandKit has three modules. Core owns declarations, routing, typed inputs,
|
||||
completion, invocation reporting, and outcomes, with Cobra/pflag behind private
|
||||
adapters. ckconfig exposes configuration inspection through application-supplied
|
||||
scopes. ckprompt exposes PromptKit inspection, preparation, requirements, and
|
||||
smoke testing and composes configuration inspection through ckconfig.
|
||||
Applications retain loading, runtime construction, resources, and executable
|
||||
lifecycle; upstream libraries retain domain semantics.
|
||||
|
||||
The current manifests select ConfigKit v1.1.0 and PromptKit v0.20.0. Both
|
||||
integrations select core v0.4.0, and ckprompt selects ckconfig v0.4.0. The ignored
|
||||
workspace can align the three local source modules. Core declares Go 1.26.0;
|
||||
the integrations declare Go 1.27.0. This remediation selects no upstream upgrade
|
||||
or consumer toolchain change.
|
||||
|
||||
The target keeps that architecture and existing command suite while delivering:
|
||||
|
||||
- Accurate outcomes and retained evidence when cancellation, provider mistakes,
|
||||
or presentation failures occur, and isolation of CommandKit-parsed JSON
|
||||
between the identified request destinations.
|
||||
- An explicit deferred-message acceptance cutoff, release of unnecessary
|
||||
borrowed-writer references after finalization, and one implementation of
|
||||
shared detail-rendering bookkeeping.
|
||||
- Working maintained examples and completion validation, with tests exercising
|
||||
current production paths instead of obsolete private entry points.
|
||||
- Narrower provider responsibilities and shared ConfigKit diagnostic adaptation
|
||||
serving both integrations' CLI reporting.
|
||||
- Shared internal flag-consumption mechanics, reuse of the existing alias tree,
|
||||
removal of avoidable repeated work, and bounded private extractions.
|
||||
- Public documentation that accurately describes implemented provider and flag
|
||||
contracts, with current guidance separated from historical records.
|
||||
|
||||
CommandKit is pre-release with no consumers. Compatibility scaffolding and
|
||||
consumer migration work are unnecessary for the selected API changes. This does
|
||||
not alter the handling of published tags or historical release contents.
|
||||
|
||||
## Scope and implementation boundaries
|
||||
|
||||
Apply the [agent](../policy/agents.md), [architecture](../policy/architecture.md),
|
||||
[testing](../policy/testing.md), and [documentation](../policy/documentation.md)
|
||||
policies. The [CLI reference](../cli.md) and exported GoDoc remain the current
|
||||
behavior owners; update them when their corresponding remedy lands.
|
||||
|
||||
The following limits keep the remedies within their accepted scope:
|
||||
|
||||
- F01 retains cancellation when it stops remaining smoke work; it does not make
|
||||
every cancellation after completed work a failure.
|
||||
- F02 copies the known CLI JSON maps and arrays, preserving presence and numeric
|
||||
values. It does not promise arbitrary deep copies of application objects or
|
||||
repair application-created sharing.
|
||||
- F03 closes deferred acceptance atomically when finalization begins, presents
|
||||
the accepted batch under existing visibility and failure rules, and rejects
|
||||
later deferred submissions with `ErrReporterClosed`. It does not promise
|
||||
successful delivery when rendering or writing fails, or require waiting for
|
||||
application producers or owning their goroutines.
|
||||
- F04 rejects a missing prepared request in that command. Core can still render
|
||||
a legitimate null report. F05 keeps operation causes distinct from rendering
|
||||
and writing failures.
|
||||
- F07 releases direct writer references after final presentation, without
|
||||
closing or flushing borrowed writers or promising immediate garbage collection.
|
||||
Retained errors may legitimately retain their own referenced objects.
|
||||
- F10 documents the agreed current naming rules, including Unicode lowercase
|
||||
letters and digits, reserved names, and existing short spellings. It adds no
|
||||
ASCII-only or leading-letter restriction or automatic name normalization.
|
||||
- F13–F15 are justified by the repeated-work analysis. Performance testing is
|
||||
not a prerequisite and no timing, allocation, or whole-installation complexity
|
||||
guarantee is introduced.
|
||||
- F16–F20 remain internal simplifications. Keep meaningful caller differences,
|
||||
existing types, and necessary coordination; do not introduce a public parser,
|
||||
generic pipeline, replay framework, or new concurrency contract.
|
||||
- F21 serves CLI presentation. It does not load or validate configuration,
|
||||
choose operation outcomes, or add general application-internal convenience
|
||||
interfaces. Core remains independent of ConfigKit and PromptKit.
|
||||
|
||||
Preserve native safe evidence, selected report content, error identity, explicit
|
||||
presence, and the established independent comparison selections through these
|
||||
changes. Do not replace upstream domain behavior or expand validation merely to
|
||||
simplify implementation. Private helper names and exact signatures are ordinary
|
||||
implementation choices unless explicitly selected as public API.
|
||||
|
||||
## Target capabilities and completion evidence
|
||||
|
||||
The areas below cover every catalog finding and define the desired end state,
|
||||
without specifying implementation order. Each finding remains accountable for
|
||||
its own remedy and evidence; a nearby refactor or passing broad suite alone does
|
||||
not establish that a defect is fixed.
|
||||
|
||||
### Correct outcomes, request ownership, and reporting lifecycle
|
||||
|
||||
| Finding | Required work | Evidence of the intended result |
|
||||
| --- | --- | --- |
|
||||
| F01 | Retain cancellation at the aggregate smoke checkpoint that omits remaining kinds, preserving previous errors and completed reports. | Cancellation during progress delivery or between operations produces a failed partial outcome with the cancellation cause; prior errors survive and unattempted kinds receive no invented report. |
|
||||
| F02 | Copy parsed extra-parameter objects and nested arrays/maps at binding application and each aggregate seed boundary. | Mutating one destination does not alter a later smoke request or an independent captured-binding application; empty/nil and numeric distinctions remain intact. |
|
||||
| F03 | Make deferred acceptance closure and batch detachment atomic while retaining the finalizer's delivery path. Document the cutoff. | A controlled concurrent submission is either accepted for the final batch or rejected as closed; successful acceptance cannot disappear into an abandoned queue. |
|
||||
| F04 | Reject nil prepared results returned without an error, after the existing acquisition/cancellation handling. | The prepared-preview command fails operationally and retains supplied diagnostics/explanation; valid previews and core null reports retain their existing treatment. |
|
||||
| F05 | Carry buffered configuration-show cancellation separately from renderer errors. | Interrupted preparation emits no partial aggregate report, retains cancellation as the operation cause, and does not invent a rendering failure; genuine rendering/writing failures remain observable. |
|
||||
| F07 | Clear direct destination references under the existing reporting lock after final presentation. | Closed handles still reject late operations, returned presentation evidence survives, and CommandKit no longer retains writers through those direct fields or closes them. |
|
||||
|
||||
F03 and F07 touch the same finalization boundary but fix different problems.
|
||||
Keep them separately explainable. F20's bookkeeping extraction must not
|
||||
be used as a substitute for either correction.
|
||||
|
||||
### Working examples and completion validation
|
||||
|
||||
| Finding | Required work | Evidence of the intended result |
|
||||
| --- | --- | --- |
|
||||
| F06 | Remove the completion fixture's collision with core's reserved `format` flag. Use core's flag when testing report selection, or rename the fixture's custom choice flag when testing application completion. Adjust corresponding expectations. | The fixture constructs and generates scripts, and available opt-in shell checks exercise completion rather than failing during installation. |
|
||||
| F08 | Assign the example's optional scenario argument only when supplied. | Bare `report` runs its `demo` default; an explicitly supplied value still reaches ordinary validation. |
|
||||
|
||||
The existing completion harness remains the validation owner; its prerequisites
|
||||
and commands belong to the [developer guide](../development.md#shell-completion-validation).
|
||||
Unavailable shell checks remain distinct from successful checks. This does not
|
||||
select a new platform support matrix.
|
||||
|
||||
### Clear and narrow integration boundaries
|
||||
|
||||
| Finding | Required work | Evidence of the intended result |
|
||||
| --- | --- | --- |
|
||||
| F09 | Document `EffectiveWriter` as supplying safe YAML for `DecodeYAMLReport`, link the conversion contract, and correct `View`'s report description. | GoDoc describes the actual single-document conversion boundary and does not imply native formatting passthrough or broader YAML support. |
|
||||
| F10 | Document the accepted naming/reservation rules once in `FlagMeta`; link relevant rename and alias helpers. | Public guidance matches installation behavior without changing the validator or flag-value semantics. |
|
||||
| F11 | Remove `WriteExplanation` from `ExplanationReader`; update GoDoc and obsolete stubs/assertions. | Custom explanation providers need only acquisition, and standard handlers continue rendering acquired evidence through core. Native objects may keep their own writer methods. |
|
||||
| F21 | Expose one narrow ckconfig diagnostic adaptation function and use it in both integrations, replacing the duplicated adaptation. | Both callers retain the selected snapshots, severity treatment, source labels, ordering, and native formatting; command-specific context, causes, outcomes, and scheduling remain with callers. |
|
||||
| F23 | Make concise-diff token decoding accept native `configkit.Path` and remove unreachable arbitrary-string validation. | Literal key tokens, including `~01`, empty keys, slash, tilde, and root paths, remain correct; snapshot failures and actual operator-input validation remain intact. |
|
||||
|
||||
For F21, the accepted input/output shape is a caller-supplied heading, native
|
||||
source records, and native diagnostic records returning existing core diagnostics
|
||||
and an explanation. The illustrative `AdaptDiagnostics` name in the analysis is
|
||||
not a separate required spelling. Reuse the existing private helper's work;
|
||||
keep the smaller scheduling switches with their callers. No new module is needed.
|
||||
|
||||
For F23, ConfigKit's opaque path is already valid, but the concise report still
|
||||
needs key-token decoding. Remove revalidation, not that necessary translation.
|
||||
Use bounded single-pass escape decoding so `~01` becomes `~1`, not `/`.
|
||||
|
||||
### Cohesive private responsibilities
|
||||
|
||||
| Finding | Required work | Evidence of the intended result |
|
||||
| --- | --- | --- |
|
||||
| F12 | Remove unused execution-resource fields and parser options and redundant normalization. Keep invocation resources with their real owners. | Parsing and buffered help retain their behavior without application input/context/streams threaded through unused backend fields; internal discard destinations still prevent accidental process output. |
|
||||
| F24 | Move tests to real selection, finalization, routed parsing, and occurrence-aware preparation paths; remove obsolete production wrappers. | Useful assertions remain, superseded paths have no production test support burden, and any small setup helpers live in test files. |
|
||||
| F16 | Share flag identity, attached/missing values, next-argument consumption, and short-cluster position through small neutral internal helpers. | Execution and completion agree on shared grammar while preserving entered spellings, explicit empty values, and their distinct treatment of unfinished input. |
|
||||
| F20 | Extract common detail-rendering/error bookkeeping under the existing reporting lock. | Ordinary and terminal reporting retain distinct headings/visibility/timing while sharing rendering-versus-writing classification, callback order, and error retention. |
|
||||
| F18 | Separate operation checks, scope/capability selection, and two-sided comparison preparation using private functions. | Help and preflight remain metadata/input-only where required; input checks precede state/acquisition, and comparison sides remain independent. |
|
||||
| F17 | Separate cohesive scope capture/validation and selector attachment work; make operation bindings and discovery inventory construction explicit. | Captured ownership, retained errors, binding identity, discovery, and construction without provider acquisition remain intact. |
|
||||
| F19 | Extract conversion-error attribution and operand preparation modestly, keeping flag preparation and occurrence ordering together. | Entered spelling, supplied-state distinctions, error identity, application order, and captured replay remain correct without replacing dense reconstruction or the existing merge algorithm. |
|
||||
|
||||
F24 covers all identified obsolete entry points. Extractions retain related
|
||||
loops and existing prepared types; F19 remains conservative. Tests protect
|
||||
behavior rather than helper counts, incidental private call sequences, or
|
||||
complexity scores.
|
||||
|
||||
For F16, retain execution normalization and failure handling with the parser,
|
||||
and partial-word handling and suggestions with completion. Keep caller-specific
|
||||
branches where unification requires extensive modes or special cases. This
|
||||
sharing introduces no new alias-completion behavior.
|
||||
|
||||
### Efficient alias and diagnostic-path traversal
|
||||
|
||||
| Finding | Required work | Evidence of the intended result |
|
||||
| --- | --- | --- |
|
||||
| F13 | Resolve each context's effective flag-alias graph with visitation states and memoized terminal results, including inactive endpoints. | Each shared suffix is resolved once per context; scoped precedence, canonical endpoints, missing-target handling, and cycle diagnostics retain their meaning. |
|
||||
| F14 | Enumerate distinct nonempty proper source prefixes through the existing resolver tree and classify each once. | Shared prefixes avoid repeated construction/classification; canonical paths, exact aliases, and valid canonical operand space retain their exclusions. |
|
||||
| F15 | Query strict descendant sources through that same tree for each existing intermediate resolution path. | Help avoids full-catalog scans for unrelated sources while preserving authoritative resolution, longer overrides, canonical-target checks, suffix deduplication, and output order. |
|
||||
| F22 | Use a scratch path during declaration validation and format full paths only for emitted diagnostics. | Successful validation avoids eager full-path formatting, while retained issues, error identity, child order, and absent/root-only declaration handling remain correct. |
|
||||
|
||||
F14 and F15 share internal read-only operations on the existing resolver tree,
|
||||
with private nodes, correctly owned captured paths, and the existing
|
||||
routing-snapshot lifetime. Root help may still use the complete catalog when
|
||||
all entries are needed. No second index, persistent result cache, new alias
|
||||
limits, or alias-completion features are selected.
|
||||
|
||||
Source-level review should establish removal of the identified repeated work;
|
||||
behavioral checks establish preserved results. No performance threshold or
|
||||
benchmark suite is required to justify these remedies. Other routing,
|
||||
termination-analysis, formatting, and installation costs remain outside those
|
||||
specific complexity improvements.
|
||||
|
||||
### Consistent current guidance and historical navigation
|
||||
|
||||
| Finding | Required work | Evidence of the intended result |
|
||||
| --- | --- | --- |
|
||||
| F25 | Replace stale planning/status directions in reporting ADR 0006 with current GoDoc/CLI references, preserving its accepted decision. | The ADR no longer presents implemented reporting as a pending prerequisite; no replacement ADR is introduced for unchanged architecture. |
|
||||
| F26 | Keep a brief module/status orientation in README and link the consumer guide for exact compatibility and installation details. | Volatile compatibility guidance has one canonical owner. |
|
||||
| F27 | Add a useful forward-only correction identifying the correct pinned consumer-guide section for the historical ckprompt note. | The correction points to the actual section without modifying the published tag, historical note, or hosted release contents. |
|
||||
|
||||
GoDoc, CLI documentation, examples, and focused internal documentation must
|
||||
describe the implemented boundaries, with valid links and semantic claims.
|
||||
Existing audit evidence remains available; the catalog records each finding's
|
||||
implementation/verification result. A selected remedy alone does not close it.
|
||||
|
||||
## Verification and module alignment
|
||||
|
||||
Verification establishes CommandKit's adaptation and coordination, without
|
||||
independently testing upstream domain correctness or turning private mechanics
|
||||
into public guarantees. Consequential defects have focused behavioral protection;
|
||||
useful contract coverage survives deletion of obsolete test machinery. The
|
||||
implementation plan defines proportionate checks and when to run them.
|
||||
|
||||
For F03, acceptance/finalization evidence must establish queue ordering; a race
|
||||
detector alone cannot do so. F07 does not establish a garbage collection timing
|
||||
promise. Ordinary suites remain deterministic, offline, and independent of
|
||||
credentials or live model services.
|
||||
|
||||
The [developer guide](../development.md#documentation-validation) owns module and
|
||||
workspace test commands. The root suite does not traverse nested modules.
|
||||
Evidence covers combined local source, representative maintained consumers,
|
||||
and available repaired completion checks using the already selected Go 1.27.0.
|
||||
|
||||
The independent-module check has a concrete dependency constraint: ckprompt's
|
||||
new F21 call will not exist in its currently selected ckconfig v0.4.0 dependency.
|
||||
Workspace success therefore cannot be reported as proof that unchanged
|
||||
independent module requirements are ready. Preserve the ignored workspace files;
|
||||
do not commit sibling replacements or fabricate dependency checksums.
|
||||
|
||||
During source development, record workspace evidence separately. Temporary
|
||||
external-consumer modules or alternate module files with explicit local
|
||||
replacements may provide additional boundary evidence, but are not validation
|
||||
of the published dependency selection. Once the required CommandKit module
|
||||
versions are available, align requirements/checksums and run each independent
|
||||
module suite with the workspace disabled. Until then, report the independent
|
||||
dependency limitation explicitly rather than marking that check complete.
|
||||
|
||||
The [release procedure](../release.md) owns dependency-order publication and
|
||||
candidate validation. Release version selection, notes, and publication remain
|
||||
separate work; this roadmap neither authorizes publication nor makes it a
|
||||
prerequisite for source refactoring. When preparing a release, reuse that
|
||||
procedure rather than inventing additional gates. The absence of consumers
|
||||
removes migration work, not the need for coherent module requirements.
|
||||
|
||||
## Completion criteria
|
||||
|
||||
- [x] Every finding F01–F27 has its selected remediation implemented and recorded
|
||||
with relevant evidence; related findings remain individually accountable.
|
||||
- [x] The five P2 defects have focused behavioral protection, including error
|
||||
identity, retained evidence, and the deferred acceptance boundary.
|
||||
- [x] F06 and F08's maintained consumers work, with actual completion checks
|
||||
distinguished from unavailable or skipped shell checks.
|
||||
- [x] F11 and F21's selected API changes and F09/F10's clarified contracts are
|
||||
reflected in canonical GoDoc and affected integration guidance.
|
||||
- [x] Obsolete production wrappers/plumbing and duplicate adaptation/bookkeeping
|
||||
are removed at the agreed boundaries, with useful tests retained.
|
||||
- [x] F13–F15 and F22 remove the identified repeated work while preserving
|
||||
behavior, without a new performance guarantee or measurement prerequisite.
|
||||
- [x] F17–F20 improve responsibility separation within their bounded scope;
|
||||
public behavior and necessary ordering remain protected.
|
||||
- [x] Documentation corrections F25–F27 are complete and local links resolve.
|
||||
- [x] Relevant module and aligned-workspace results are recorded. Independent
|
||||
results identify the exact dependency selection; any release-alignment work
|
||||
still pending is explicitly distinguished from completed source work.
|
||||
|
||||
The significant choices have been resolved in the catalog and remediation
|
||||
analysis. Routine implementation details do not require an Open Decisions
|
||||
section. Revisit a selected direction only if concrete implementation evidence
|
||||
reveals a conflict or materially different tradeoff.
|
||||
@@ -1,218 +0,0 @@
|
||||
# Functional correctness audit
|
||||
|
||||
Status: six open findings: four P2 library findings and two P3 example/validation
|
||||
fixture findings. Reviewed source:
|
||||
`7b2dc2f2c975abe2708945097f5dd4802313215e`.
|
||||
|
||||
## Scope and architecture
|
||||
|
||||
The review follows the [architecture policy](../../policy/architecture.md),
|
||||
[CLI contract](../../cli.md), public GoDoc, and accepted decisions. The
|
||||
[internal overview](../../internal/overview.md) inventories the current owners:
|
||||
|
||||
- Core owns captured declarations, atomic registration, command and flag alias
|
||||
resolution, typed conversion and composition, completion, help/version,
|
||||
shared report rendering, and invocation outcomes. Neutral metadata separates
|
||||
those responsibilities from the internal Cobra/pflag adapter.
|
||||
- ckconfig owns configuration scope selection, independent comparison inputs,
|
||||
opt-in overrides, acquisition coordination, and configuration reports. ConfigKit
|
||||
owns configuration resolution, validation, metadata, safe snapshots, and diffs.
|
||||
- ckprompt owns PromptKit command assembly, reusable request bindings, namespace
|
||||
forwarding, inspection/requirements/preparation adapters, and smoke aggregation.
|
||||
PromptKit owns the underlying domain operations. Applications own loaders,
|
||||
builders, execution dependencies, and executable lifecycle.
|
||||
- Release launchers and module profiles select ReleaseKit's validation and
|
||||
publication facilities; they are separate from library runtime behavior.
|
||||
|
||||
The audit examined ordinary and error paths, declaration capture and registration
|
||||
state, routing and parsing boundaries, presence/default handling, captured input
|
||||
replay, report conversions, reporting finalization, cancellation, provider result
|
||||
adaptation, scope/comparison preparation, request forwarding, smoke aggregation,
|
||||
maintained examples, and completion/release wiring. Upstream domain algorithms
|
||||
were treated as upstream-owned. Findings concern CommandKit-owned behavior;
|
||||
they do not request arbitrary object copying or concurrent App dispatch.
|
||||
|
||||
## Validation evidence
|
||||
|
||||
Validation used Go 1.27.0 and the existing dependency cache, without modifying
|
||||
production code or repository tests:
|
||||
|
||||
- All three independent module default suites passed with
|
||||
`GOWORK=off GOTOOLCHAIN=local go test -mod=readonly ./...`, run separately from
|
||||
the root, ckconfig, and ckprompt module directories.
|
||||
- The aligned workspace suites passed with
|
||||
`GOTOOLCHAIN=local go test -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/...`.
|
||||
- Temporary external-consumer probes, using local replacements for all three
|
||||
CommandKit modules, reproduced C1–C4. Their assertions fail against the
|
||||
reviewed implementation. C2 reproduced in three additional consecutive runs.
|
||||
- Running the maintained core example reproduced C5.
|
||||
- The opt-in backend Bash completion checks passed. The opt-in consumer Bash
|
||||
checks failed before interactive completion, as described in C6. Zsh and Fish
|
||||
are not installed, so their interactive checks were not run.
|
||||
|
||||
The completed findings in [remediation.md](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/264b82bd5759f052a3646d57828945a87b7586d3/docs/roadmap/remediation.md) remain historical
|
||||
records of the preceding reviews. This audit records additional findings against
|
||||
the corrected source, rather than reopening those issues. Independent integration
|
||||
modules still select released CommandKit dependencies; workspace tests and the
|
||||
external probes exercised the combined current source.
|
||||
|
||||
## C1 — P2: CLI provider-parameter objects leak mutations between requests
|
||||
|
||||
**Code:** [request_flags.go](../../../integrations/ckprompt/request_flags.go),
|
||||
NewExecutionFlags/ExtraParams and executionFlag, lines 107–115;
|
||||
[smoke_command.go](../../../integrations/ckprompt/smoke_command.go),
|
||||
runSmokeAggregate and smokeRequestForKind, particularly lines 227–240.
|
||||
|
||||
**Trigger and expected behavior:** supply `--llm-extra-params` containing an
|
||||
object, then customize a smoke request in a builder/callback. Each aggregate
|
||||
operation should begin with the operator's original seed, with its own kind.
|
||||
Similarly, applying captured reusable execution flags to separately initialized
|
||||
inputs should not let one destination rewrite the CommandKit-parsed JSON values
|
||||
subsequently applied to another. Shared application input/state is a separate,
|
||||
deliberate contract.
|
||||
|
||||
**Actual behavior:** ExtraParams assigns the captured map directly, and
|
||||
smokeRequestForKind copies the execution struct and scalar pointers without
|
||||
copying that map. The map and nested maps/arrays remain shared. In a probe using
|
||||
`{"top":"original","nested":{"value":"original"}}`, mutations made by the
|
||||
reachability callback appeared in authentication, text inference, and structured
|
||||
output requests. The command still returned status 0. A second probe applied
|
||||
the public ExtraParams handle through CaptureFlags/ApplyTo, mutated a nested
|
||||
value, then used ApplySuppliedTo on an independent destination; it received the
|
||||
mutated value instead of the supplied JSON.
|
||||
|
||||
**Potential remediation:** copy the library-parsed JSON object and its nested
|
||||
objects/arrays when applying ExtraParams and when preparing each aggregate seed.
|
||||
Retain nil versus an explicitly empty object, numeric types, and authoritative
|
||||
builder replacements. Keep this localized to the known CLI JSON representation;
|
||||
do not add generic deep-copy behavior to ValueFlag or arbitrary application
|
||||
objects. Regression coverage should exercise both smoke aggregation and public
|
||||
captured binding replay with nested objects/arrays.
|
||||
|
||||
## C2 — P2: Finalization can discard a successfully queued deferred message
|
||||
|
||||
**Code:** [reporter.go](../../../reporter.go), DeferredReporter.enqueue lines
|
||||
254–279 and Reporter.finalize lines 282–325.
|
||||
|
||||
**Trigger and expected behavior:** a goroutine submits a visible deferred
|
||||
message while Execute is flushing the invocation's existing deferred messages.
|
||||
Reporter operations are documented as safe concurrently, and deferred submission
|
||||
returns its acceptance error immediately. The submission should either be
|
||||
retained for delivery or rejected because the deferred queue has closed.
|
||||
|
||||
**Actual behavior:** finalize detaches the current queue under the mutex, then
|
||||
unlocks between deliveries. Until the final closed flag is set, enqueue can
|
||||
append to the newly empty state.deferred and return nil. Finalization never
|
||||
drains that new queue and clears it when closing. The accepted message disappears.
|
||||
A probe used a diagnostic writer that briefly pauses delivery, with another
|
||||
goroutine calling AtEnd().Warn during that flush. Submission returned nil, the
|
||||
message was absent, and Execute returned success with diagnostic delivery marked
|
||||
complete. This does not require concurrent App dispatch or reentering a reporter
|
||||
from a rendering callback.
|
||||
|
||||
**Potential remediation:** make the deferred queue's acceptance/finalization
|
||||
transition atomic. For example, stop accepting deferred submissions when the
|
||||
queue is detached and return ErrReporterClosed to later submissions, while
|
||||
allowing the internal finalization messages to finish. Alternatively, ensure
|
||||
every accepted message is drained before closure. Preserve existing visibility,
|
||||
message ordering, and callback non-reentrancy rules.
|
||||
|
||||
## C3 — P2: Prepared-request inspection reports success without a result
|
||||
|
||||
**Code:** [preparation.go](../../../integrations/ckprompt/preparation.go),
|
||||
runRequestPreparation lines 95–104; the PrepareRequest callback contract in
|
||||
[adapters.go](../../../integrations/ckprompt/adapters.go).
|
||||
|
||||
**Trigger and expected behavior:** a supplied Suite.PrepareRequest callback
|
||||
returns a zero Acquired value and nil error. Its contract supplies a native
|
||||
prepared-request report. Absence of that report should produce an operational
|
||||
provider/wiring failure, consistent with named resource inspection, requirements
|
||||
inspection, and smoke commands.
|
||||
|
||||
**Actual behavior:** the handler checks the error and context but never checks
|
||||
acquired.Value. `promptkit show prepared example --format=json` with such a
|
||||
callback writes `null` and returns status 0 with no explanation. A missing
|
||||
prepared request is therefore indistinguishable from successful preparation in
|
||||
the invocation outcome. The neighboring inspection handlers explicitly reject
|
||||
missing results; smoke handling rejects a missing report without an error.
|
||||
|
||||
**Potential remediation:** reject a nil prepared result after acquisition and
|
||||
cancellation handling, preserving independent diagnostics and any supplied
|
||||
explanation. Leave core's support for deliberate null reports unchanged; this
|
||||
check belongs to the prepared-request command. Cover the empty-success callback
|
||||
alongside valid reports and report-plus-error behavior.
|
||||
|
||||
## C4 — P2: Show preparation labels ordinary cancellation as a rendering error
|
||||
|
||||
**Code:** [show.go](../../../integrations/ckconfig/show.go), runConfigShow lines
|
||||
69–95, interacting with Reporter.RenderOutput in [reporter.go](../../../reporter.go).
|
||||
|
||||
**Trigger and expected behavior:** context cancellation becomes visible between
|
||||
buffered effective-document conversions, before report delivery starts. The
|
||||
handler should stop preparation and return context.Canceled as the operation
|
||||
cause. Cancellation alone should not manufacture an encoder or output failure.
|
||||
The same distinction is protected for buffered configuration reports in
|
||||
[report_cancellation_test.go](../../../integrations/ckconfig/report_cancellation_test.go).
|
||||
|
||||
**Actual behavior:** the loop's context checkpoint returns the cancellation
|
||||
error from inside RenderOutput. Core records it as Output.RenderError. The
|
||||
subsequent configFailure preserves the cancellation cause but cannot undo that
|
||||
false presentation evidence. A two-scope `config show --scope=all` probe acquired
|
||||
both scopes successfully; the first native writer emitted valid safe YAML and
|
||||
canceled the context while returning nil. The second writer was not called and
|
||||
no report bytes were delivered, yet the outcome contained both context.Canceled
|
||||
and an incomplete output delivery with context.Canceled as RenderError.
|
||||
|
||||
**Potential remediation:** carry preparation cancellation separately from the
|
||||
rendering callback's error, following the cancellation checkpoint approach in
|
||||
renderPreparedConfigReport. Retain real native-writing, decoding, and delivery
|
||||
errors independently; preserve cancellation identity and the existing rule that
|
||||
an incomplete aggregate show emits no report.
|
||||
|
||||
## C5 — P3: The core example overwrites its optional scenario default
|
||||
|
||||
**Code:** [examples/core/main.go](../../../examples/core/main.go), report command
|
||||
input factory and optional argument setter, lines 70–84.
|
||||
|
||||
**Trigger and expected behavior:** invoke the example's `report` command without
|
||||
a scenario. The operand is optional and NewInput explicitly initializes Scenario
|
||||
to `demo`, so the default reporting demonstration should run.
|
||||
|
||||
**Actual behavior:** ValueArgument applies an unsupplied zero string, and the
|
||||
example's setter assigns it unconditionally. CheckInput then rejects the empty
|
||||
scenario. Running `go run ./examples/core report` prints
|
||||
`ERROR: unknown reporting scenario ""` and exits with the command's usage status
|
||||
2. Existing example cases always supply a scenario and miss this path.
|
||||
|
||||
**Potential remediation:** only replace Scenario when the argument's Set field
|
||||
is true. This preserves the intended default while still letting an explicitly
|
||||
supplied empty string reach the ordinary validation path. Do not change core's
|
||||
documented argument/default application semantics to accommodate the example.
|
||||
|
||||
## C6 — P3: The interactive completion consumer cannot construct its App
|
||||
|
||||
**Code:** [testdata/completion/main.go](../../../testdata/completion/main.go),
|
||||
lines 28–44; [completion_shell_test.go](../../../completion_shell_test.go),
|
||||
TestCompletionConsumerInShells; core's reserved-name check in
|
||||
[command.go](../../../command.go), validateBindings.
|
||||
|
||||
**Trigger and expected behavior:** enable the documented optional completion
|
||||
consumer checks, or run the fixture's completion command. The fixture must
|
||||
construct the application and generate the requested script before its shell
|
||||
dispatch/completion behavior can be exercised.
|
||||
|
||||
**Actual behavior:** the fixture still declares its own `format` value flag.
|
||||
Core reserves that spelling for universal report format selection, so New
|
||||
rejects the declaration. The check
|
||||
`COMMANDKIT_COMPLETION_SHELLS=1 go test -run '^TestCompletionConsumerInShells$/bash/' .`
|
||||
fails while generating the Bash script with
|
||||
`flag name "format" is reserved for reporting`. This precedes the interactive
|
||||
shell step and affects every shell using this fixture. The ordinary suite skips
|
||||
the opt-in check, explaining why its passing result does not cover this failure.
|
||||
The separate backend Bash script checks pass.
|
||||
|
||||
**Potential remediation:** adapt the fixture to the core format flag, or rename
|
||||
its application-specific choice flag and update the consumer cases accordingly.
|
||||
Then rerun the consumer checks with the available shells. This is a fixture and
|
||||
validation-path defect; the observed construction error does not establish that
|
||||
all generated completion scripts are broken.
|
||||
@@ -1,206 +0,0 @@
|
||||
# Algorithmic correctness and efficiency audit
|
||||
|
||||
Status: four open P3 efficiency findings. Reviewed source:
|
||||
`d8da761a213c82837276266faeebf93beda8eb08`.
|
||||
|
||||
## Scope and architecture
|
||||
|
||||
The [internal overview](../../internal/overview.md) inventories the implemented
|
||||
owners. Core captures declarations, validates installation batches, resolves
|
||||
routes and bindings, coordinates invocations, and supplies completion and shared
|
||||
reporting. Its neutral metadata and internal Cobra/pflag adapter separate public
|
||||
contracts from backend state. ckconfig prepares scopes and comparison inputs and
|
||||
projects ConfigKit evidence. ckprompt assembles PromptKit operations and adapts
|
||||
native requests, catalogs, reports, and smoke results. Applications own loaders,
|
||||
builders, and execution dependencies; upstream libraries own domain algorithms.
|
||||
Release launchers delegate maintainer operations to ReleaseKit.
|
||||
|
||||
This pass reviewed declaration/registry traversal, alias compilation and help
|
||||
discovery, parsing/completion, operand allocation, captured binding replay,
|
||||
report conversion and delivery, integration selection/filtering/projection, and
|
||||
maintained consumer/tooling boundaries. Findings describe source-level scaling,
|
||||
not measured bottlenecks or new performance guarantees. No benchmarks or
|
||||
real-world performance measurements were gathered.
|
||||
|
||||
Explicitly selected approaches were excluded from reconsideration:
|
||||
|
||||
- Fresh mutable backend state per invocation, immutable metadata reuse, and
|
||||
report buffering, as selected in the retired performance records and current
|
||||
[internal overview](../../internal/overview.md).
|
||||
- The finite pushdown termination analysis, longest-prefix routing semantics,
|
||||
canonical flag endpoints, scoped precedence, and intermediate-route discovery
|
||||
selected in the retired alias implementation plan at `f42aefe`.
|
||||
- Completion's explicit routing check for each proposed child, selected in the
|
||||
retired completion implementation plan at `10a799a`.
|
||||
- JSON report normalization, whole-report aggregation, and the composed example's
|
||||
invocation-local list cache using reflect.DeepEqual, selected in the retired
|
||||
adoption implementation plan at `2f43f78`.
|
||||
- The [deferred scope/selector and backend work](../future.md#possible-further-efficiency-work),
|
||||
upstream domain algorithms, and the completed dense occurrence reconstruction,
|
||||
heap merge, validation-filter indexing, and subtree-prefix preparation in
|
||||
[remediation.md](https://gitea.maximumdirect.net/eric/CommandKit/src/commit/264b82bd5759f052a3646d57828945a87b7586d3/docs/roadmap/remediation.md).
|
||||
|
||||
The findings preserve those decisions. The plans prescribe endpoint, precedence,
|
||||
discovery, and validation behavior, but do not require independent repeated
|
||||
chain walks, repeated processing of identical prefixes, full-catalog scans per
|
||||
rewrite, or eager diagnostic path formatting at every valid node.
|
||||
|
||||
## Validation and interpretation
|
||||
|
||||
All three independent module default suites passed with Go 1.27.0 using
|
||||
`GOWORK=off GOTOOLCHAIN=local go test -mod=readonly ./...`, run separately from
|
||||
each module directory. The aligned workspace suite also passed using
|
||||
`GOTOOLCHAIN=local go test -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/...`.
|
||||
The unchanged source reused successful cached results. Dependency downloads were
|
||||
disabled. No production files or repository tests were modified.
|
||||
|
||||
A temporary external consumer confirmed that the workload shapes below are
|
||||
accepted: a flag-alias chain with an active command endpoint and inactive group
|
||||
contexts; a single-token group-alias chain whose help has no descendant aliases;
|
||||
distinct aliases sharing a discovery-only prefix; and a valid nested declaration
|
||||
chain. These are behavioral checks, not timing experiments. The complexity
|
||||
findings follow from the implementation's loops and copies.
|
||||
|
||||
Complexities below use expected constant-time map operations and count tokens
|
||||
or records separately from their string-byte costs. They describe the identified
|
||||
phase, not the entire construction or execution pipeline. Nested loops over
|
||||
disjoint child collections, required independent input copies, sorted output,
|
||||
and work proportional to actual serialized output are not findings by themselves.
|
||||
|
||||
No additional ordinary correctness defect was established during this pass.
|
||||
The six open findings in [01-correctness.md](01-correctness.md) remain separate;
|
||||
passing default suites do not resolve those findings.
|
||||
|
||||
## E1 — P3: Flag-alias compilation repeatedly resolves shared chain suffixes
|
||||
|
||||
**Code:** [flag_aliases.go](../../../flag_aliases.go), resolveFlagAliases lines
|
||||
226–251 and resolveFlagTarget lines 254–279; compileFlagAliases invokes this work
|
||||
independently for each canonical and discovery context.
|
||||
|
||||
**Circumstances and complexity:** consider A effective mappings forming
|
||||
`--a1 → --a2 → … → --aA → --value`. Each source starts a fresh visited map and
|
||||
chain walk. The suffix traversed for `--a2` was already traversed for `--a1`, and
|
||||
so on. There are Θ(A²) total target examinations and corresponding cumulative
|
||||
chain/visited storage operations per context, despite only A mappings. The same
|
||||
cost occurs when the final flag is unavailable and all these global aliases are
|
||||
inactive. Across C contexts with the same chain shape, this part can cost
|
||||
Θ(CA²), apart from sorting, canonical metadata, and string processing. Peak
|
||||
temporary storage for a single walk is O(A).
|
||||
|
||||
**Potential refactoring:** resolve the effective functional graph once per
|
||||
context using visitation states and memoized terminal results. Cache both an
|
||||
available canonical endpoint and an unavailable endpoint, and detect a cycle
|
||||
using the active traversal stack. This reduces graph traversal to expected O(A)
|
||||
work and O(A) scratch storage per context. Keeping the existing source sort for
|
||||
deterministic validation leaves O(A log A) total per-context work for these
|
||||
steps. A context-local cache suffices; no cross-invocation provider cache is
|
||||
needed.
|
||||
|
||||
Canonical spellings must still terminate target resolution before following a
|
||||
mapping of the same spelling. Resolve each source's target before separately
|
||||
checking active source collisions. Preserve scoped overrides, inactive globals,
|
||||
scoped missing-target failures, useful cycle diagnostics, and revalidation after
|
||||
registration. Results cannot be shared indiscriminately between contexts.
|
||||
|
||||
## E2 — P3: Alias help scans every source for every intermediate route
|
||||
|
||||
**Code:** [help_view.go](../../../help_view.go), routeHelpAliases lines 38–51;
|
||||
[resolver.go](../../../internal/alias/resolver.go), ResolutionPaths lines 139–150.
|
||||
|
||||
**Circumstances and complexity:** let P be the number of paths visited while
|
||||
rewriting the entered help path and A the number of declared command aliases.
|
||||
For every visited path, the function scans all A aliases and copies each source
|
||||
through FromTokens before checking its length and prefix. This performs PA
|
||||
source examinations, plus repeated source-token copying and prefix comparisons,
|
||||
even when no source can contribute an entry.
|
||||
|
||||
For a valid chain of A single-token aliases ending at a canonical group,
|
||||
`help a1` visits Θ(A) paths. Every source is one token long, so every candidate
|
||||
is discarded by the length check. Discovery of an empty alias section still
|
||||
takes Θ(A²) examinations/copies. This cost recurs when requesting that help
|
||||
again even if the routing snapshot is already prepared; it is separate from
|
||||
registration, termination analysis, and backend construction.
|
||||
|
||||
**Potential refactoring:** query an immutable source-prefix index for strict
|
||||
descendants of each visited prefix. The existing internal alias trie is a
|
||||
possible owner; alternatively prepare a source index with the routing snapshot.
|
||||
Building such an index is proportional to total source tokens. Prefix lookup
|
||||
then visits matching descendants rather than the entire catalog. For the
|
||||
single-token-chain example, the identified discovery work becomes O(A), with no
|
||||
descendant entries to enumerate. General cases still pay for prefix lengths,
|
||||
matching candidates, authoritative target resolution, and sorting the result.
|
||||
|
||||
Keep intermediate-route discovery and resolve each surviving composed entered
|
||||
path through the existing resolver. Preserve longer overrides, canonical-target
|
||||
validation, suffix deduplication, and deterministic presentation. A prefix index
|
||||
does not authorize enumerating additional composed routes or introducing the
|
||||
deferred alias-completion feature.
|
||||
|
||||
## E3 — P3: Discovery-context preparation rebuilds shared prefixes repeatedly
|
||||
|
||||
**Code:** [flag_aliases.go](../../../flag_aliases.go), discoverFlagContexts lines
|
||||
181–200, particularly the prefix cloning/key construction at 185–187 and
|
||||
resolution at 194–195.
|
||||
|
||||
**Circumstances and complexity:** R distinct alias sources can share a prefix
|
||||
of depth D, for example `legacy nested … action1`, `legacy nested … action2`,
|
||||
and so on, all targeting an ordinary canonical group. Every source independently
|
||||
enumerates the same D proper prefixes. For each prefix length k, it copies k
|
||||
tokens, builds a key, and may resolve and classify the prefix again. The
|
||||
prefixes map deduplicates only the eventual result; it does not prevent this
|
||||
work, including for prefixes previously classified as ineligible.
|
||||
|
||||
With bounded component lengths and no rewrite within those proper prefixes,
|
||||
the shared-prefix portion costs Θ(RD²), although the source declarations contain
|
||||
only Θ(RD) tokens and there are only D distinct proper prefixes. When R and D
|
||||
both grow, that is cubic work in their common scale for quadratic input size.
|
||||
Additional rewrite work can increase the classification cost further.
|
||||
|
||||
**Potential refactoring:** enumerate distinct proper source prefixes once,
|
||||
preferably by traversing the already compiled source trie, then run the existing
|
||||
classification once per unique prefix. Retaining full paths and keys once per
|
||||
prefix is sufficient; no change to command routing is required. In the example,
|
||||
source traversal plus prefix materialization/classification can be
|
||||
O(RD + D²), instead of O(RD²). More generally, account for total source tokens,
|
||||
each distinct prefix's representation, and one resolution per distinct prefix.
|
||||
|
||||
A seen set consulted only after rebuilding each full prefix key would remove
|
||||
duplicate resolutions but retain the Θ(RD²) cloning/key cost. Use a traversal
|
||||
that actually shares prefix processing. Preserve exclusion of canonical paths,
|
||||
exact alias sources, and prefixes intercepted by valid canonical operand space;
|
||||
cache negative classifications too if using an alternative memoized traversal.
|
||||
The source index proposed in E2 could support this work as well.
|
||||
|
||||
## E4 — P3: Successful declaration validation eagerly constructs every full path
|
||||
|
||||
**Code:** [command.go](../../../command.go), collectDeclarationIssues lines
|
||||
444–470, especially path copying and formatting at 451–453.
|
||||
|
||||
**Circumstances and complexity:** every visited declaration copies its entire
|
||||
ancestral path and clones/joins it again to make a diagnostic label before
|
||||
checking whether any issue needs that label. For N valid commands arranged in
|
||||
a depth-N chain, this performs Θ(1 + 2 + … + N) = Θ(N²) token-copying and label
|
||||
construction work, with quadratic cumulative allocation, while producing no
|
||||
diagnostic. A valid tree can be checked for retained issues and root-only
|
||||
placement in one traversal.
|
||||
|
||||
**Potential refactoring:** maintain one scratch path for the validation walk using
|
||||
an append/truncate stack and format it only when emitting an actual issue or a
|
||||
root-only placement error. With ordinary amortized slice growth, the successful
|
||||
validation walk becomes O(N) work and O(D) path storage for depth D. Error cases
|
||||
also pay for the path strings actually included in their diagnostics.
|
||||
|
||||
Preserve child order, retained error identity, root-only placement checks, and
|
||||
invalid/absent declaration handling. This changes scratch diagnostic preparation,
|
||||
not captured declaration ownership. Other installation phases retain their own
|
||||
metadata/path costs; this finding does not claim that complete App construction
|
||||
would consequently become linear for every tree shape.
|
||||
|
||||
## Recommendation
|
||||
|
||||
Consider E1 first: memoizing a context's flag graph removes repeated suffix
|
||||
work with a small, local change. E2 and E3 can share a private source-prefix
|
||||
traversal/index while retaining the selected resolver and discovery semantics.
|
||||
E4 is an independent simplification of diagnostic traversal. All four are
|
||||
efficiency opportunities for consideration, not evidence of measured operational
|
||||
impact or a reason to reopen the selected algorithms listed above.
|
||||
@@ -1,259 +0,0 @@
|
||||
# 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.
|
||||
@@ -1,178 +0,0 @@
|
||||
# Concurrency audit
|
||||
|
||||
Status: three relevant P2 findings: one new cancellation defect and two
|
||||
reconfirmed findings from the functional correctness audit. Reviewed source:
|
||||
`2ea26afcc5ae691152e80d2fa5ee4d572d390a95`.
|
||||
|
||||
## Architecture, contracts, and scope
|
||||
|
||||
The [internal overview](../../internal/overview.md) inventories the implemented
|
||||
owners. Core captures declarations and routing metadata, builds fresh backend
|
||||
parse state per invocation, prepares typed inputs, and coordinates invocation
|
||||
reporting and outcomes. ckconfig owns configuration scope/comparison coordination;
|
||||
ckprompt owns PromptKit request/inspection coordination and sequential aggregate
|
||||
smoke tests. Applications own loaders, builders, runtime dependencies, contexts,
|
||||
and executable lifecycle. Upstream libraries own their domain operations.
|
||||
|
||||
The [reporting contract](../../consumers/guide.md#invocation-reporting) and
|
||||
[Reporter GoDoc](../../../reporter.go) explicitly permit concurrent operations on
|
||||
one invocation's reporter. One reporting-state mutex protects visibility,
|
||||
deferred messages, destination observations, and writes. Managed output writes
|
||||
acquire it; rendering callbacks use a coordinated writer while the rendering
|
||||
operation holds it. Rendering callbacks must not reenter that reporter, and
|
||||
their supplied writer is valid only while the callback runs. Concurrent message
|
||||
submission order is unspecified. Reporter handles close before Execute returns;
|
||||
caller-owned writers are not closed.
|
||||
|
||||
Concurrent Execute/Register access to one App is not established by the current
|
||||
contracts; [concurrent dispatch remains separate future work](../future.md#other-future-proposals).
|
||||
The audit did not infer that guarantee from independent invocation state or
|
||||
declaration reuse. Application callback objects and factory results retain their
|
||||
own ownership and synchronization responsibilities under the
|
||||
[architecture policy](../../policy/architecture.md#handler-inputs-and-invocation-state).
|
||||
|
||||
Static review covered locks and state access, reporter acceptance/finalization,
|
||||
callback and writer lifetimes, declaration/snapshot ownership, integration input
|
||||
and shared-state lifetimes, cancellation checkpoints, comparison/aggregate
|
||||
sequencing, and maintained examples and release launchers. CommandKit's runtime
|
||||
code starts no goroutines and owns no channels or worker pools. Integration
|
||||
callbacks run synchronously; aggregate scopes, comparison sides, and smoke kinds
|
||||
are coordinated sequentially. Release launchers use exec to delegate to ReleaseKit.
|
||||
Application/upstream workers and subprocess internals remain with their owners.
|
||||
|
||||
## Validation evidence
|
||||
|
||||
Codebase-memory-mcp symbol/source searches and call tracing were supplemented
|
||||
with repository-wide searches for synchronization, goroutines, channels, and
|
||||
cancellation. The indexed Go source matches this revision. Validation used Go
|
||||
1.27.0, cached dependencies, and no live services or credentials:
|
||||
|
||||
- The aligned current-source workspace suite passed with race detection and
|
||||
fresh test execution:
|
||||
`GOTOOLCHAIN=local go test -race -count=1 -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/...`.
|
||||
This includes all three modules and their maintained examples. The optional
|
||||
interactive shell checks remain opt-in and were not enabled in this pass.
|
||||
- A temporary external consumer run with `go run -race` reproduced N1 through
|
||||
the public ckprompt/core APIs. It canceled the context from the progress
|
||||
destination's writer without reentering the reporter.
|
||||
- Existing temporary external probes using local replacements for all three
|
||||
CommandKit modules reproduced N2 and N3 in three consecutive runs each with
|
||||
`go test -race -count=3`. Their functional assertions failed as described
|
||||
below. No data-race report was emitted.
|
||||
|
||||
No production files or repository tests were changed. These failures concern
|
||||
ordering and cancellation accounting; memory-race detection alone does not
|
||||
establish that an accepted queue item is delivered or a stopped aggregate fails.
|
||||
|
||||
## N1 — P2: Cancellation can stop aggregate smoke testing with a success outcome
|
||||
|
||||
**Code:** [smoke_command.go](../../../integrations/ckprompt/smoke_command.go),
|
||||
runSmokeAggregate lines 159–176 and final outcome selection at 183–187;
|
||||
queuePromptDiagnostics in [command.go](../../../integrations/ckprompt/command.go),
|
||||
lines 586–603; core's Progress delivery in [reporter.go](../../../reporter.go).
|
||||
|
||||
**Circumstances:** a smoke callback returns a report without error. The first
|
||||
post-callback context check sees no cancellation. Cancellation becomes visible
|
||||
while diagnostics are presented, or asynchronously before the later checkpoint
|
||||
at line 175. That checkpoint stops the loop, but does not retain its error.
|
||||
Progress diagnostics are delivered immediately, so this window can be exercised
|
||||
deterministically with a valid progress writer that cancels the context.
|
||||
|
||||
**Observed behavior:** a temporary consumer invoked
|
||||
`promptkit test all --progress --format=json`. Its first callback returned a
|
||||
completed reachability report and a progress diagnostic. The diagnostic writer
|
||||
canceled the context and accepted all bytes. Only one callback ran; the JSON
|
||||
report contained one successful entry with no error. Execute nevertheless
|
||||
returned Kind success, status 0, and nil Cause. Rendering and writing errors
|
||||
were nil. The other three requested kinds were never attempted.
|
||||
A second probe returned a provider error from that callback: status was 1, but
|
||||
errors.Is(outcome.Cause, context.Canceled) was still false after the same stop.
|
||||
|
||||
**Expected behavior and consequences:** the
|
||||
[aggregate smoke contract](../../cli.md#promptkit-smoke-testing) requires all four
|
||||
callbacks to return without error for status 0 and preserves cancellation causes
|
||||
when cancellation stops the sequence. A partial aggregate stopped by cancellation
|
||||
must not report successful completion. Automation can otherwise accept an
|
||||
incomplete aggregate, and callers cannot detect the cancellation through
|
||||
errors.Is on the returned cause. If an operation error already exists, the same
|
||||
window can omit cancellation from its joined causes.
|
||||
|
||||
**Potential remediation:** when the later checkpoint uses cancellation to omit
|
||||
remaining kinds, retain that error in the aggregate cause before leaving the
|
||||
loop. Deduplicate an already retained cancellation as appropriate. Preserve
|
||||
completed reports, prior operation errors/explanations, and attempted-kind order;
|
||||
do not invent a report for an unattempted kind. Moving only the earlier check
|
||||
leaves another interval in which cancellation can be observed and discarded.
|
||||
This does not require treating every cancellation after completed report delivery
|
||||
as a new failure. Coverage should distinguish cancellation in the provider,
|
||||
during immediate progress presentation, and between aggregate operations.
|
||||
|
||||
## N2 — P2: Deferred-message acceptance remains open after the final batch detaches
|
||||
|
||||
This is the same issue as
|
||||
[C2 in the correctness audit](01-correctness.md#c2--p2-finalization-can-discard-a-successfully-queued-deferred-message),
|
||||
reconfirmed here rather than a separate remediation item.
|
||||
|
||||
**Code:** [reporter.go](../../../reporter.go), DeferredReporter.enqueue lines
|
||||
254–279 and Reporter.finalize lines 282–325.
|
||||
|
||||
**Circumstances:** finalization detaches state.deferred and unlocks between
|
||||
deliveries. Another goroutine can acquire the mutex during that flush, append a
|
||||
visible message to the now-empty queue, and receive nil from its submission.
|
||||
The reporter has not yet set closed. The finalizer drains only the detached
|
||||
batch, then clears the new queue when closing.
|
||||
|
||||
**Consequences and evidence:** the accepted message silently disappears, while
|
||||
Execute can return success and mark diagnostic delivery complete. The probe
|
||||
used one invocation, a briefly pausing diagnostic destination, and a goroutine
|
||||
submitting AtEnd().Warn during the flush. All three race-enabled runs accepted
|
||||
and lost the message. There was no unsynchronized memory access or callback
|
||||
reentry; the defect is the queue's acceptance/finalization ordering.
|
||||
|
||||
**Potential remediation:** atomically close deferred acceptance when detaching
|
||||
the final batch, rejecting later submissions explicitly, or drain every accepted
|
||||
item before closure. Keep finalizer-owned delivery possible after acceptance
|
||||
closes; setting the reporter's existing closed flag too early would also reject
|
||||
its internal delivery. Preserve message visibility, ordering, and error evidence.
|
||||
Goroutines created by an application remain application-owned; this finding
|
||||
concerns an operation that already returned successful acceptance.
|
||||
|
||||
## N3 — P2: Buffered configuration-show cancellation becomes a presentation failure
|
||||
|
||||
This is the same issue as
|
||||
[C4 in the correctness audit](01-correctness.md#c4--p2-show-preparation-labels-ordinary-cancellation-as-a-rendering-error),
|
||||
reconfirmed here rather than a separate remediation item.
|
||||
|
||||
**Code:** [show.go](../../../integrations/ckconfig/show.go), runConfigShow lines
|
||||
69–95, interacting with core Reporter.RenderOutput in
|
||||
[reporter.go](../../../reporter.go).
|
||||
|
||||
**Circumstances:** cancellation becomes visible between buffered effective
|
||||
document conversions. The checkpoint inside RenderOutput returns ctx.Err as
|
||||
the rendering callback's error. Core records it as Output.RenderError before
|
||||
ckconfig returns cancellation as the operation cause.
|
||||
|
||||
**Consequences and evidence:** normal cancellation is reported as both an
|
||||
operation failure and an incomplete output/rendering failure. A two-scope probe
|
||||
acquired both views; the first native writer emitted valid safe YAML, canceled
|
||||
the context, and returned nil. The second writer was skipped and no report
|
||||
bytes were delivered. All three race-enabled runs retained context.Canceled as
|
||||
Cause but also incorrectly recorded it as Output.RenderError, with no writer
|
||||
error. This can mislead callers distinguishing interrupted preparation from
|
||||
encoder or output failures.
|
||||
|
||||
**Potential remediation:** retain preparation cancellation separately from the
|
||||
rendering callback's error, following renderPreparedConfigReport in
|
||||
[operations.go](../../../integrations/ckconfig/operations.go). Preserve real
|
||||
native-writing, conversion, and destination errors independently, and keep an
|
||||
incomplete aggregate report from being delivered. Retain the selected distinction
|
||||
between cancellation before delivery and cancellation after delivery has started.
|
||||
|
||||
## Assessment
|
||||
|
||||
N1 is new. N2 and N3 remain open in the correctness audit and should be tracked
|
||||
as the same underlying issues. No data race, supported-path deadlock,
|
||||
goroutine leak, or channel-lifecycle defect was established in the reviewed
|
||||
CommandKit code. The passing race suite supports the exercised synchronization
|
||||
paths; the three findings demonstrate why lifecycle and cancellation reasoning
|
||||
are also necessary.
|
||||
@@ -1,132 +0,0 @@
|
||||
# Resource management audit
|
||||
|
||||
Status: one open P3 memory-retention finding. Reviewed source:
|
||||
`8fe8b67b29bbd96e321a09d797b3df5a21cfd0bf`.
|
||||
|
||||
## Architecture and resource ownership
|
||||
|
||||
The [internal overview](../../internal/overview.md) inventories the implemented
|
||||
owners. Core retains captured command declarations and immutable routing metadata
|
||||
per App, constructs fresh backend state per invocation, prepares typed inputs,
|
||||
and owns shared reporting/finalization. ckconfig coordinates configuration scopes
|
||||
and comparisons; ckprompt adapts PromptKit inspection, request, and smoke
|
||||
operations. Applications own executable lifecycle, contexts, loaders, builders,
|
||||
runtime dependencies, and factory results. ConfigKit and PromptKit own their
|
||||
native operations and resource handling. Release launchers delegate to ReleaseKit.
|
||||
|
||||
The [architecture policy](../../policy/architecture.md) and public GoDoc govern
|
||||
those boundaries. In particular, [Reporter](../../../reporter.go) borrows its
|
||||
destinations: it closes its reporting handles before Execute returns, but does
|
||||
not close or flush caller-owned writers. Releasing a reference and closing a
|
||||
writer are separate operations.
|
||||
|
||||
| Resource | Observed ownership and lifecycle |
|
||||
| --- | --- |
|
||||
| Files and network connections | Library runtime code opens no files or connections directly. Application loaders and adopted upstream operations supply configuration, artifacts, engines, and client effects. CommandKit does not assume ownership of their closers. |
|
||||
| Contexts, timers, and workers | The caller supplies the invocation context. CommandKit creates no runtime cancel functions, timers, goroutines, channels, or worker pools. Integration callbacks and aggregate operations run synchronously; cancellation is cooperative at the selected checkpoints. |
|
||||
| Command declarations and metadata | Captured bindings/callbacks and routing metadata have App/declaration lifetime. Backend sessions and parsed values are invocation-local; metadata caching does not store those mutable sessions on App. |
|
||||
| Report buffers and pending diagnostics | Native conversion buffers, normalized report values, and projections are scoped to preparation/delivery. Deferred detail closures remain pending until finalization; processed queue entries are cleared and the queue is detached/cleared. Closed destination references remain retained as described in RM1. |
|
||||
| Examples and tooling fixtures | Example caches are invocation state supplied by the application. The completion logging fixture explicitly closes its file; Go harnesses run and wait for subprocesses, and the interactive Python driver closes its shell in finally. Temporary fixture directories use t.TempDir. Release launchers use exec rather than starting background workers. |
|
||||
|
||||
The review covered normal, error, and cancellation exits, retained callbacks and
|
||||
binding values, destination ownership, buffers/encoders, registration/snapshot
|
||||
replacement, integration acquisition/comparison lifetimes, and fixture cleanup.
|
||||
Codebase-memory-mcp symbol/source search and call tracing were supplemented by
|
||||
direct inspection of examples, shell/Python tooling, and acquisition/cleanup
|
||||
calls. The indexed Go source matches the reviewed revision.
|
||||
|
||||
## Validation evidence
|
||||
|
||||
Validation used Go 1.27.0 and existing dependencies, with dependency downloads
|
||||
disabled:
|
||||
|
||||
- The aligned workspace suites passed with
|
||||
`GOTOOLCHAIN=local go test -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/...`.
|
||||
Successful cached results were reused for unchanged code, including maintained
|
||||
examples and launcher tests. The optional interactive completion checks were
|
||||
not enabled for this pass.
|
||||
- A temporary external consumer used standard weak pointers and runtime.GC to
|
||||
examine destination reachability after Execute. It retained one reporting
|
||||
handle at a time and used runtime.KeepAlive to ensure that handle remained live
|
||||
during the observation. Dropping it supplied the collection control. This was
|
||||
a reachability check, not a performance benchmark or heap-size measurement.
|
||||
- Existing reporter tests were inspected for caller-owned writer handling,
|
||||
finalization/late-write behavior, sticky destination errors, and deferred
|
||||
callback release. Other acquisition and cleanup paths were reviewed statically.
|
||||
|
||||
No production files or repository tests were changed. No external services,
|
||||
credentials, or publication operations were used.
|
||||
|
||||
## RM1 — P3: Closed reporting handles retain both destination objects
|
||||
|
||||
**Code:** [reporter.go](../../../reporter.go), reportingState/destinationState
|
||||
lines 38–57; newReporter lines 106–113; Output/managedOutputWriter lines 144–156;
|
||||
AtEnd lines 226–231; Reporter.finalize lines 282–329, particularly closure at
|
||||
324–326. The private close helper at 679–685 also leaves destination references
|
||||
intact.
|
||||
|
||||
**Circumstances:** a caller retains a Reporter, its Output writer, or an
|
||||
AtEnd view after Execute returns, while otherwise releasing the destinations.
|
||||
For example, a longer-lived worker or job record may keep a handle so late
|
||||
submissions can receive ErrReporterClosed. All three handles retain the same
|
||||
reportingState, which continues to hold both Streams.Out and Streams.Err even
|
||||
after finalization. No successful future operation needs those destination
|
||||
objects: closed operations reject before using them.
|
||||
|
||||
**Observed behavior:** the temporary consumer supplied separate output and
|
||||
diagnostic writer objects, each carrying a 1 MiB payload. It retained only one
|
||||
closed handle, with weak references to the two destinations. For each of the
|
||||
three handle kinds, it exercised successful, failed, and canceled invocations.
|
||||
Every late submission correctly returned ErrReporterClosed, but both destination
|
||||
objects remained reachable after garbage collection in all nine cases. After
|
||||
dropping the handle and collecting again, both weak references became nil in
|
||||
all nine cases.
|
||||
|
||||
The reference chain is:
|
||||
|
||||
```text
|
||||
retained closed handle
|
||||
→ reportingState
|
||||
→ output.writer
|
||||
→ diagnostics.writer
|
||||
```
|
||||
|
||||
**Potential consequences:** completed jobs can retain large output buffers,
|
||||
compression state, or other writer-owned object graphs for the lifetime of an
|
||||
otherwise small closed handle. Even keeping only Output retains the diagnostic
|
||||
destination. Across N retained handles, retained destination memory can grow
|
||||
with the sum of those objects' sizes. This is conditional retention, not an
|
||||
unreachable-heap leak: dropping the handles releases the references. File and
|
||||
connection closure remains the caller's responsibility; the observation does
|
||||
not establish a CommandKit-owned descriptor leak.
|
||||
|
||||
**Potential remediation:** after final presentation is captured and the shared
|
||||
state is closed, clear its direct destination writer references under the
|
||||
existing mutex, or replace them with a lightweight discard destination. Keep
|
||||
the shared closed state so all copied handles continue to return
|
||||
ErrReporterClosed. Do not call Close or Flush on borrowed writers. Preserve
|
||||
returned presentation observations and error identity; errors may legitimately
|
||||
retain their own referenced data independently. Apply consistent reference
|
||||
release to any private closure helper that remains relevant.
|
||||
|
||||
This improves resource lifetime without adding a public garbage collection
|
||||
guarantee. Future regression coverage should check that closure
|
||||
releases unnecessary direct references while preserving caller ownership,
|
||||
late-operation errors, and presentation evidence.
|
||||
|
||||
## Other reviewed costs and limits
|
||||
|
||||
Whole-report JSON normalization, YAML adaptation, aggregate report preparation,
|
||||
and pending deferred messages require memory proportional to their selected
|
||||
content and representations. Rendering still runs in quiet mode to retain
|
||||
rendering failures. These are selected behaviors; this audit does not infer
|
||||
arbitrary report-size limits, queue truncation, or provider cleanup ownership.
|
||||
The [future efficiency record](../future.md#possible-further-efficiency-work)
|
||||
preserves buffering/reuse proposals, and the
|
||||
[efficiency audit](02-efficiency.md) separately records avoidable allocation and
|
||||
repeated-work opportunities. Those findings remain separate assignments.
|
||||
|
||||
No additional file-descriptor, network-connection, timer, goroutine, or child
|
||||
process cleanup defect was established in CommandKit-owned code. The
|
||||
[concurrency audit](04-concurrency.md) records the existing queue-acceptance and
|
||||
cancellation defects; they do not establish additional resource leaks here.
|
||||
@@ -1,258 +0,0 @@
|
||||
# Documentation consistency audit
|
||||
|
||||
Status: five open P3 documentation findings, plus cross-references to previously
|
||||
recorded implementation discrepancies. Reviewed source:
|
||||
`efb5f774176526bc6de74eca5f7472d078ea1704`.
|
||||
|
||||
## Architecture and scope
|
||||
|
||||
The [internal overview](../../internal/overview.md) describes the implemented
|
||||
owners. CommandKit consists of three independently versioned Go modules:
|
||||
|
||||
- Core owns captured command declarations, typed bindings and presence,
|
||||
installation, alias routing, dispatch, completion, help/version, shared
|
||||
text/YAML/JSON rendering, and invocation reporting and outcomes. Private
|
||||
command metadata, path and alias packages connect the facade to an internal
|
||||
Cobra/pflag adapter.
|
||||
- ckconfig depends on core and ConfigKit v1.1.0. It assembles six standard
|
||||
configuration commands from declared scope capabilities: scopes, validate,
|
||||
show, sources, explain, and diff. Loaders supply configuration and evidence;
|
||||
the integration selects report content and core renders it.
|
||||
- ckprompt depends on core, ckconfig, ConfigKit v1.1.0, and PromptKit v0.20.0.
|
||||
It provides optional catalogs, resource inspection, requirements discovery
|
||||
and validation, prepared-request preview, smoke testing, reusable request
|
||||
flags, and a configuration scope. Applications explicitly install these
|
||||
capabilities and retain engine/client construction and executable lifecycle.
|
||||
|
||||
Core has no ConfigKit or PromptKit dependency. Upstream libraries own domain
|
||||
semantics; applications own contexts, streams, runtime effects, credentials,
|
||||
and final exit mapping. The release launchers delegate source-only module
|
||||
publication and optional hosted-page delivery to ReleaseKit.
|
||||
|
||||
The review covered the 47 tracked Markdown files, public package/API comments,
|
||||
maintained examples and fixtures, module metadata, release profiles and
|
||||
launchers, and relevant implementation and tests. The
|
||||
[documentation policy](../../policy/documentation.md), architecture, testing,
|
||||
release, and applicable attribution rules supplied the review boundaries.
|
||||
Codebase-memory-mcp discovery and source inspection were supplemented with
|
||||
direct reads of documentation, examples, and tooling.
|
||||
|
||||
The current shared reporting and PromptKit v0.20.0 features are implemented.
|
||||
The ckprompt v0.3.0 candidate remains unpublished according to the current
|
||||
release records; implementation and publication are different states.
|
||||
Older release notes were evaluated as historical records against their local
|
||||
tags, not as current CLI instructions. Retired roadmaps, completed remediation
|
||||
descriptions, and explicitly deferred features were not treated as promises of
|
||||
current functionality.
|
||||
|
||||
## Validation evidence
|
||||
|
||||
Validation used the cached Go 1.27.0 toolchain with `GOTOOLCHAIN=local`,
|
||||
`GOPROXY=off`, `-mod=readonly`, and a temporary writable build cache. The initial
|
||||
attempt to use the default cache encountered the sandbox's read-only filesystem;
|
||||
the temporary cache resolved that environment limitation.
|
||||
|
||||
- All default suites passed in the aligned workspace:
|
||||
`go test -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/...`.
|
||||
Each module's independent `GOWORK=off go test -mod=readonly ./...` also passed.
|
||||
- All 45 `go run` examples in the three example READMEs and all 20 developer-guide
|
||||
example invocations were exercised using binaries built from the aligned
|
||||
workspace. Their selected arguments and working directories were preserved.
|
||||
The documented invalid-input examples failed as intended, and the differing
|
||||
`config diff --check` example returned status 3. Representative reports and
|
||||
help were compared with the CLI reference.
|
||||
- Temporary external-consumer probes substantiated D1 and D2 below. No
|
||||
production files, existing documentation, or repository tests were changed.
|
||||
- The local-link pass checked 334 Markdown link occurrences. The only apparent
|
||||
missing path was the intentional `COMMIT_URL` attribution-format placeholder
|
||||
in the OSS policy. Repository-hosted links were also checked against locally
|
||||
available Git objects: their file paths existed; unpublished ckprompt footer
|
||||
targets and the release URL template were excluded. Checking 11 referenced
|
||||
section fragments in those objects found D4.
|
||||
- Independent module graphs agreed with the versions listed in their attribution
|
||||
entries. The declared Go requirements, release profiles, launchers, and
|
||||
release-note dependency descriptions were checked against local metadata and
|
||||
tags. No publication, delivery, credentialed network operation, or live model
|
||||
call was attempted. External upstream URL availability was not tested.
|
||||
- Default tests skip interactive shell completion. The consumer fixture's
|
||||
script-generation step was run separately and reproduced the already recorded
|
||||
reserved-flag failure C6; the full interactive shell matrix was not run.
|
||||
|
||||
Passing default suites do not establish that the previously recorded exceptional
|
||||
paths below are correct. They also do not validate every possible consumer
|
||||
provider or upstream configuration schema.
|
||||
|
||||
## D1 — P3: Configuration provider GoDoc omits the effective-output contract
|
||||
|
||||
**Documentation:** [contracts.go](../../../integrations/ckconfig/contracts.go),
|
||||
EffectiveWriter at lines 66–69 and View at 82–87. EffectiveWriter promises a
|
||||
native effective representation without specifying its encoding. View still
|
||||
describes exposing facets without reconstructing their reports.
|
||||
|
||||
**Implementation:** [show.go](../../../integrations/ckconfig/show.go),
|
||||
runConfigShow at lines 68–101, buffers WriteEffective, passes its bytes to
|
||||
[DecodeYAMLReport](../../../report_value.go), and submits the decoded value to
|
||||
the shared reporter. The
|
||||
[CLI reference](../../cli.md#show-effective-configuration) already describes
|
||||
conversion to a logical report value.
|
||||
|
||||
**Discrepancy and consequence:** a custom scope implementer reading the owning
|
||||
GoDoc is not told that WriteEffective must supply one supported YAML document,
|
||||
or that its formatting is not passed through. Valid YAML is not sufficient in
|
||||
every case: the converter rejects multiple documents, unsupported tags, and
|
||||
non-string mapping keys. This is an undocumented provider requirement, rather
|
||||
than evidence that all native YAML must be accepted.
|
||||
|
||||
**Observed behavior:** a temporary custom EffectiveWriter emitting
|
||||
`answer: 42` produced a JSON mapping under `config show --format=json` with
|
||||
status 0. A two-document YAML stream and a mapping containing an explicitly
|
||||
tagged `!!timestamp` both returned status 1, empty result output, and a rendering
|
||||
error identifying the unsupported input. No acquisition error was needed.
|
||||
|
||||
**Correct information and potential remediation:** describe WriteEffective as
|
||||
supplying safe effective YAML for conversion through DecodeYAMLReport. Link that
|
||||
helper as the common owner of the conversion contract, and make its supported
|
||||
value boundary sufficiently explicit there. Update View's comment to describe
|
||||
evidence used to build reports, rather than imply native report passthrough.
|
||||
Do not promise preservation of comments, styles, or original framing, or broaden
|
||||
YAML support merely to accommodate the old wording.
|
||||
|
||||
The related unused WriteExplanation requirement is already tracked as
|
||||
[M6](03-complexity.md#m6--p3-explanation-providers-must-implement-an-unused-rendering-capability).
|
||||
Clarifying the current read-versus-render boundary does not require resolving
|
||||
that API simplification in this documentation change.
|
||||
|
||||
## D2 — P3: Public flag metadata omits enforced declaration constraints
|
||||
|
||||
**Documentation:** [command.go](../../../command.go), FlagMeta.Name and Short
|
||||
at lines 91–103, describe a name without leading dashes and an optional one-byte
|
||||
short spelling. The [registered CLI syntax](../../cli.md#registered-command-syntax)
|
||||
describes invocation grammar, not the additional declaration grammar.
|
||||
|
||||
**Implementation:** validateBindings and validFlagName in the same file,
|
||||
lines 477–545. Long names must be nonempty segments of Unicode lowercase
|
||||
letters or digits separated by single hyphens. Leading, trailing, and repeated
|
||||
hyphens are rejected. Core also rejects reserved help/reporting names and `h`
|
||||
as a short spelling; `-` is not a valid short spelling. Declaration errors are
|
||||
retained until installation.
|
||||
|
||||
**Discrepancy and consequence:** the exact exported metadata contract leaves
|
||||
consumers to discover ordinary naming restrictions through installation errors
|
||||
or private implementation. Names such as `foo_bar` and `fooBar` satisfy the
|
||||
published field description but cannot be installed. RenameFlag and flag-alias
|
||||
declarations also depend on the long-name grammar.
|
||||
|
||||
**Observed behavior:** temporary public-API constructions accepted `foo-bar`,
|
||||
`123`, and `équipe`, but rejected `foo_bar`, `fooBar`, and `foo--bar` during New
|
||||
with the lowercase/hyphen-separated-name error.
|
||||
|
||||
**Correct information and potential remediation:** document the enforced naming
|
||||
and reserved-spelling rules once with FlagMeta, and refer related public helpers
|
||||
to that contract. Keep the CLI reference focused on operator syntax. Do not
|
||||
accidentally introduce an ASCII-only or leading-letter restriction: neither is
|
||||
enforced by the implementation. If the intended naming policy differs, surface
|
||||
that as a deliberate API decision rather than silently changing validation.
|
||||
|
||||
## D3 — P3: Reporting ADR still directs readers to a planned implementation
|
||||
|
||||
**Documentation:** [ADR 0006](../../adr/0006-invocation-reporting.md), lines 5–7
|
||||
say shared formatting is implemented; line 54 leaves implementation status and
|
||||
format choices in the now-retired roadmap; lines 76–77 still describe the
|
||||
extension as planned until implementation and canonical contracts land.
|
||||
|
||||
**Implementation and current contracts:** [report.go](../../../report.go)
|
||||
exports Report, Format, and RenderReport;
|
||||
[reporter.go](../../../reporter.go) integrates Report with invocation delivery;
|
||||
the [CLI reporting contract](../../cli.md#reporting) and
|
||||
[consumer guidance](../../consumers/guide.md#invocation-reporting) describe the
|
||||
implemented formats. The default suites exercise these APIs.
|
||||
|
||||
**Discrepancy:** the ADR mixes current implementation status with pre-implementation
|
||||
wording and points to a historical roadmap for information now owned by current
|
||||
API and CLI documentation. This can make a completed prerequisite look pending.
|
||||
|
||||
**Correct information and potential remediation:** keep the accepted ownership
|
||||
decision intact. A status clarification can explicitly identify the remaining
|
||||
future-tense passages as historical and direct current behavior to GoDoc and
|
||||
the CLI reference. If editorial cleanup of those passages is approved, remove
|
||||
only stale lifecycle wording; the documentation policy's accepted-decision
|
||||
immutability rule does not justify rewriting the architectural decision or
|
||||
creating a new ADR for an unchanged decision.
|
||||
|
||||
## D4 — P3: A historical release note links to a nonexistent section
|
||||
|
||||
**Documentation:** [ckprompt v0.2.0 release note](../../../integrations/ckprompt/docs/releases/v0.2.0.md),
|
||||
line 32, links to the consumer guide at the immutable
|
||||
`integrations/ckprompt/v0.2.0` tag with fragment
|
||||
`#promptkit-configuration-and-definitions`.
|
||||
|
||||
**Evidence:** `git show integrations/ckprompt/v0.2.0:docs/consumers/guide.md`
|
||||
contains the heading `PromptKit configuration, definitions, and requirements`
|
||||
at line 244. The correct fragment is
|
||||
`#promptkit-configuration-definitions-and-requirements`. The linked document
|
||||
exists, but the requested section does not. The tagged modules' dependency
|
||||
versions agree with the note; this is a navigation defect, not a claim that
|
||||
historical dependencies should match current source.
|
||||
|
||||
**Correct information and potential remediation:** provide the
|
||||
[correct pinned section link](https://forge.maximumdirect.net/base-layer/commandkit/src/tag/integrations/ckprompt/v0.2.0/docs/consumers/guide.md#promptkit-configuration-definitions-and-requirements)
|
||||
in a forward-only documentation correction or erratum where useful. Preserve
|
||||
the published tag, note, and hosted release contents. This defect does not
|
||||
warrant republishing the old version or backfilling hosted pages as part of the
|
||||
audit. Future note preparation should check fragments against the target
|
||||
revision as well as checking that the linked file exists.
|
||||
|
||||
## D5 — P3: The root README duplicates the compatibility matrix
|
||||
|
||||
**Documentation:** [README.md](../../../README.md), lines 25–30, repeats the
|
||||
published core/integration pairings, the incompatible ckconfig v0.3.0 and
|
||||
ckprompt v0.2.0 combination, and the current candidate dependency selection.
|
||||
The [consumer guide](../../consumers/guide.md#module-selection), lines 18–29,
|
||||
maintains those same details alongside installation guidance.
|
||||
|
||||
**Implementation and policy:** the module requirements in
|
||||
[core](../../../go.mod), [ckconfig](../../../integrations/ckconfig/go.mod), and
|
||||
[ckprompt](../../../integrations/ckprompt/go.mod), plus locally available
|
||||
release tags, corroborate the current descriptions. However, the
|
||||
[one-owner rule](../../policy/documentation.md#one-canonical-owner) permits only
|
||||
a short stable summary in a non-owning document and requires links for volatile
|
||||
contract details. README owns orientation and current status, not a second
|
||||
historical module compatibility reference.
|
||||
|
||||
**Discrepancy:** the facts currently agree; the finding is duplicated ownership
|
||||
under the adopted policy. The README's existing link does not remove the need
|
||||
to update its separate multi-version matrix whenever compatibility guidance
|
||||
changes.
|
||||
|
||||
**Potential remediation:** retain a brief current-status statement and the module
|
||||
overview in README, and make the consumer guide the single maintained source of
|
||||
exact version combinations and installation choices. This requires no dependency
|
||||
change and no invented compatibility guarantee. Historical release notes remain
|
||||
historical records rather than duplicate current reference documents.
|
||||
|
||||
## Previously recorded discrepancies affecting documented behavior
|
||||
|
||||
These remain relevant to this audit but are not additional documentation
|
||||
findings. Their detailed triggers and remediation belong in the earlier audits.
|
||||
Current implementation paths were inspected; the completion-fixture failure was
|
||||
also reproduced here. Do not weaken the selected documentation contracts merely
|
||||
to make them describe these defects.
|
||||
|
||||
| Current documentation or promised behavior | Existing implementation finding |
|
||||
| --- | --- |
|
||||
| Reusable parsed inputs and per-kind smoke seeds preserve the operator's selected values. | [C1](01-correctness.md#c1--p2-cli-provider-parameter-objects-leak-mutations-between-requests): mutation of parsed provider-parameter maps can cross replay/aggregate boundaries. |
|
||||
| Concurrent reporter submissions and deferred delivery follow the documented invocation lifecycle. | [C2](01-correctness.md#c2--p2-finalization-can-discard-a-successfully-queued-deferred-message) and [N2](04-concurrency.md#n2--p2-deferred-message-acceptance-remains-open-after-the-final-batch-detaches): a deferred submission can return success during finalization and be discarded. |
|
||||
| Prepared-request preview returns the prepared request; aggregate smoke success covers all requested kinds. | [C3](01-correctness.md#c3--p2-prepared-request-inspection-reports-success-without-a-result) permits null successful preview output; [N1](04-concurrency.md#n1--p2-cancellation-can-stop-aggregate-smoke-testing-with-a-success-outcome) can return success after cancellation stops an aggregate. |
|
||||
| Operation cancellation and presentation failures remain separately observable. | [C4](01-correctness.md#c4--p2-show-preparation-labels-ordinary-cancellation-as-a-rendering-error) and [N3](04-concurrency.md#n3--p2-buffered-configuration-show-cancellation-becomes-a-presentation-failure): a buffered-show cancellation is also recorded as a rendering failure. |
|
||||
| The developer guide's optional completion consumer check can reach shell validation. | [C6](01-correctness.md#c6--p3-the-interactive-completion-consumer-cannot-construct-its-app): the fixture declares the now-reserved `format` flag. Direct script generation still fails during App construction, before any interactive shell check. |
|
||||
|
||||
## Assessment
|
||||
|
||||
The principal current CLI descriptions, maintained README commands, module
|
||||
requirements, report defaults, and release-launcher instructions are consistent
|
||||
with the reviewed implementation on the exercised paths. The new findings are
|
||||
documentation clarifications and maintenance issues, not reasons to redesign
|
||||
the command suite or extend upstream functionality. Prioritize the public
|
||||
provider and naming contracts, then reconcile status/navigation and duplicated
|
||||
compatibility guidance. The previously recorded runtime defects remain separate
|
||||
implementation work.
|
||||
@@ -1,266 +0,0 @@
|
||||
# Architectural responsibility review
|
||||
|
||||
Status: four open P3 candidates for consideration; no implementation changes.
|
||||
Reviewed source: `4a91f29bcd6583fb77d7283f955a0220e7291f8c`.
|
||||
|
||||
## Scope and conclusion
|
||||
|
||||
This review examined repository-owned core, internal packages, ckconfig,
|
||||
ckprompt, maintained consumers, and release-launcher boundaries. Dependency
|
||||
interfaces were consulted to establish what CommandKit can delegate; dependency
|
||||
implementations were not audited. The review followed the
|
||||
[architecture policy](../../policy/architecture.md),
|
||||
[agent policy](../../policy/agents.md), and accepted [ADRs](../../adr/README.md).
|
||||
Future proposals and retired plans were distinguished from selected contracts.
|
||||
|
||||
The broad ownership split is sound. No substantial configuration resolver,
|
||||
prompt preparation engine, or application runtime framework was found that
|
||||
should be removed wholesale. The actionable opportunities are narrower remnants
|
||||
of earlier boundaries and execution paths. Replacing these with another generic
|
||||
adapter framework would defeat the purpose of the changes.
|
||||
|
||||
| Outcome | Assessment |
|
||||
| --- | --- |
|
||||
| Necessary machinery | Captured declarations, typed preflight, native capability adaptation, safe report conversion, and invocation reporting have concrete selected responsibilities. See the retention assessment below. |
|
||||
| Unnecessary implementation complexity | R2–R4 identify redundant parser plumbing, validation of already-valid native paths, and private entry points sustained only by tests. |
|
||||
| Complexity caused by an optional contract | R1 identifies an unused method in a public provider interface. Removing it preserves CLI behavior but changes what callers can invoke through that interface. |
|
||||
|
||||
R1 overlaps M6 in the [complexity audit](03-complexity.md); it is not an additional
|
||||
independent issue. This review confirms its historical cause and consumer impact.
|
||||
Other findings in that audit concerning function extraction or consolidation
|
||||
are not repeated merely because the functions are long.
|
||||
|
||||
## Current architecture
|
||||
|
||||
- **Core** captures typed declarations, validates installation batches, resolves
|
||||
application aliases, prepares supplied inputs, dispatches callbacks, and owns
|
||||
help, completion, reporting, and outcomes. Its shared report API owns text,
|
||||
YAML, and JSON encoding. Applications and integrations choose report content.
|
||||
- **Private packages** isolate command paths, alias resolution/termination,
|
||||
neutral metadata, and the mutable Cobra/pflag tree. The adapter parses and
|
||||
renders help; core executes application callbacks. No backend type is required
|
||||
in the public API.
|
||||
- **ckconfig** composes scope capabilities and independently prepared inputs,
|
||||
invokes application loaders, and adapts ConfigKit's effective configuration,
|
||||
diagnostics, provenance, metadata, and native differences into CLI reports.
|
||||
ConfigKit remains responsible for configuration semantics and redaction.
|
||||
- **ckprompt** composes operator-facing resource inspection, requirements,
|
||||
request validation, prepared previews, and smoke tests using native PromptKit
|
||||
operations. It also supplies reusable CLI bindings and a ckconfig scope.
|
||||
PromptKit owns preparation, resolution, and smoke-test semantics; applications
|
||||
own engine/client construction and request builders.
|
||||
- **Applications** own process exit, contexts, streams, startup, long-lived
|
||||
resources, and domain workflows. The release launcher delegates release
|
||||
operations to ReleaseKit. Neither integration belongs in core's dependency
|
||||
graph; ckprompt uses ckconfig's public API.
|
||||
|
||||
The [internal overview](../../internal/overview.md) owns the full package
|
||||
inventory. Reviewed integration dependencies are ConfigKit v1.1.0 and PromptKit
|
||||
v0.20.0, as selected in the integration module files.
|
||||
|
||||
## R1 — P3: Explanation providers still owe an unused text writer
|
||||
|
||||
**Outcome:** complexity caused by an optional public contract; recommend narrowing
|
||||
that contract deliberately.
|
||||
|
||||
**Code and evidence:** [contracts.go](../../../integrations/ckconfig/contracts.go),
|
||||
`ExplanationReader` at lines 76–80 requires both `Explain` and
|
||||
`WriteExplanation`. Production consumers in
|
||||
[explain.go](../../../integrations/ckconfig/explain.go) and
|
||||
[show.go](../../../integrations/ckconfig/show.go) use only `Explain`.
|
||||
The direct interface-writer call remains in
|
||||
[contracts_adapter_test.go](../../../integrations/ckconfig/contracts_adapter_test.go),
|
||||
line 121; provider doubles also implement the unused method.
|
||||
|
||||
**Requirement and cause:** ckconfig needs the native explanation value to select
|
||||
report content. Core owns formatting that content. Commit `c61e125` replaced the
|
||||
text template's call back into `WriteExplanation` with a template over the
|
||||
acquired explanation, making text and structured output use the same evidence.
|
||||
The provider interface was not narrowed with that change. It still assigns text
|
||||
rendering work to a provider even though the CLI no longer uses it.
|
||||
|
||||
**Removal test:** deleting the method from the interface would leave production
|
||||
CLI operations unchanged. Existing native providers still satisfy the smaller
|
||||
interface. A consumer calling `view.Explanation.WriteExplanation(...)` through
|
||||
the interface would stop compiling; a consumer calling the native ConfigKit
|
||||
object directly would be unaffected. That is a real source-level consideration,
|
||||
but the repository supplies no remaining CLI requirement for this accommodation.
|
||||
|
||||
**Potential remediation:** reduce `ExplanationReader` to `Explain`, update its
|
||||
GoDoc, and remove writer-only provider stubs and interface assertions. Retain
|
||||
coverage of native explanation acquisition and rendering from acquired values.
|
||||
Record the public API change through the normal release process. Do not restore
|
||||
the obsolete callback, add a second writer interface, or recreate the writer
|
||||
through another adapter merely to preserve this unused requirement.
|
||||
|
||||
## R2 — P3: Parser sessions carry application execution resources they do not use
|
||||
|
||||
**Outcome:** unnecessary implementation complexity; simplification can preserve
|
||||
the public execution contract.
|
||||
|
||||
**Code and evidence:** [metadata.go](../../../internal/commandmeta/metadata.go),
|
||||
`ParseOptions` at lines 184–194;
|
||||
[parse.go](../../../internal/cobra/parse.go), `normalizeOptions` and `buildTree`
|
||||
at lines 292–358; [execute.go](../../../execute.go), `execution` and
|
||||
`newExecution` at lines 80–89 and 118–130.
|
||||
|
||||
`newExecution` retains an unread `streams` field, gives the streams to the
|
||||
reporter, and also passes output/error writers and context into parser options.
|
||||
The adapter fills in four resource defaults and installs context/input/output/
|
||||
error resources on each Cobra node. Its actual path uses `Find`, `ParseFlags`,
|
||||
`Usage`, and `UseLine`; it does not execute Cobra callbacks or read input.
|
||||
`RenderHelp` temporarily uses its own buffer and returns the text for core to
|
||||
deliver. The existing `TestSessionReusesTreeForParseHelpAndUsageWithFreshState`
|
||||
explicitly checks that supplied session streams remain unused.
|
||||
|
||||
**Requirement and cause:** contexts and destinations belong to the invocation
|
||||
and its reporter. Parser options retain an execution-shaped interface even
|
||||
though the backend's job is now parsing and help rendering. The extra resources
|
||||
have no selected reader or execution behavior to support. `newExecution` also
|
||||
normalizes the same streams that `newReporter` normalizes immediately afterward.
|
||||
|
||||
**Removal test:** dropping the unread execution field and parser context/input
|
||||
plumbing does not remove access to `Invocation.Context` or reporter destinations.
|
||||
Dropping caller-writer plumbing can likewise preserve behavior if the adapter
|
||||
retains explicit discard destinations and local help buffering. Simply removing
|
||||
all backend stream setup is not equivalent: Cobra defaults must not introduce
|
||||
unmanaged process output. This is a necessary output boundary, not a reason to
|
||||
retain caller-owned streams throughout the backend tree.
|
||||
|
||||
**Potential remediation:** keep resource ownership in execution/reporting,
|
||||
remove the unused field and parser resource options, normalize streams once,
|
||||
and give the adapter only the alias-selection inputs it consumes. Keep a simple
|
||||
internal sink and help buffer where necessary. Preserve the runnable marker
|
||||
used by help and other backend metadata; this finding does not justify deleting
|
||||
every Cobra setting indiscriminately. Retain the existing tests that show help
|
||||
does not escape core delivery and that invocations receive their real context.
|
||||
|
||||
## R3 — P3: Concise diff rendering revalidates native ConfigKit paths
|
||||
|
||||
**Outcome:** unnecessary implementation complexity; preserve the concise report.
|
||||
|
||||
**Code and evidence:** [diff_report.go](../../../integrations/ckconfig/diff_report.go),
|
||||
`decodeJSONPointer` at lines 11–44 and its sole production caller,
|
||||
`renderDiffReport` at lines 127–155.
|
||||
The helper accepts arbitrary strings and validates leading slashes and each
|
||||
escape, although its caller supplies only `difference.Path.String()`.
|
||||
[diff_report_test.go](../../../integrations/ckconfig/diff_report_test.go) tests
|
||||
malformed strings that this production boundary cannot supply.
|
||||
|
||||
**Requirement and owner:** ConfigKit owns path construction and parsing. Its
|
||||
public `Path` is immutable, has unexported representation, has a valid root zero
|
||||
value, and documents its string representation as JSON Pointer. Even a custom
|
||||
comparison callback returns `[]configkit.Difference`, whose paths have this
|
||||
native type. ckconfig needs decoded key tokens to assemble the selected concise
|
||||
report's nested additions/changes. ConfigKit exposes no public token accessor,
|
||||
so this small adaptation remains necessary; a general pointer validator does not.
|
||||
|
||||
**Removal test:** eliminating validation of the native representation would not
|
||||
reject or accept different supported comparison inputs. Removing token decoding
|
||||
altogether would break literal slash/tilde keys. Replacing the entire concise
|
||||
report with native difference records would change the deliberately selected
|
||||
operator report, including the distinction between ordinary and detailed output.
|
||||
|
||||
**Potential remediation:** make the private helper explicitly consume a native
|
||||
`configkit.Path`, split its canonical pointer text, and decode the two escapes
|
||||
with the standard library, without an error-returning validation state machine.
|
||||
Use a single-pass replacement so `~01` becomes `~1`, not `/`. Remove only the
|
||||
now-unreachable validation branches and malformed-string assertions. Preserve
|
||||
root/empty/numeric/dot/percent/Unicode/slash/tilde key behavior and snapshot
|
||||
decoding errors. Keep `configkit.ParsePath` for actual operator-supplied paths.
|
||||
This does not require an upstream change, a public pointer utility, or relaxed
|
||||
validation of untrusted CLI syntax.
|
||||
|
||||
## R4 — P3: Private production entry points exist only to sustain older tests
|
||||
|
||||
**Outcome:** unnecessary implementation complexity; lower priority than the
|
||||
boundary changes above.
|
||||
|
||||
**Code and evidence:** [reporter.go](../../../reporter.go), `setVisibility` at
|
||||
lines 668–677 and `close` at lines 679–686, have only test callers. Actual
|
||||
execution uses `applySelection` and `finalize`. The former test setter repeats
|
||||
visibility-state assignment while omitting the current closed-state check and
|
||||
format selection; the latter only flips the closed bit, bypassing deferred
|
||||
delivery and finalization. `TestReporterClosedHandlesReturnClosedErrorIncludingQuiet`
|
||||
therefore exercises a synthetic close path rather than invocation completion.
|
||||
|
||||
Similarly, `App.parse` in [parser.go](../../../parser.go), lines 13–19, is called
|
||||
only by `TestAppParseUsesRegisteredBindingsWithTheAdapter`. Actual dispatch
|
||||
uses `execution.parse`, which prepares the route-aware session.
|
||||
`prepareInput` and `prepareInputWithSpellings` in
|
||||
[binding.go](../../../binding.go), lines 244–250, form a test-only compatibility
|
||||
chain into the current `prepareInputWithOccurrences` entry point.
|
||||
|
||||
**Requirement and cause:** testing internal behavior is legitimate, but these
|
||||
superseded entry points are not needed to provide a supported consumer API.
|
||||
They retain earlier call shapes after occurrence-aware preparation, routed
|
||||
sessions, and reporter finalization became the real paths. Passing their tests
|
||||
does not show that the corresponding current lifecycle path was exercised.
|
||||
|
||||
**Removal test:** production behavior and exported APIs would be unchanged;
|
||||
the affected tests would need to use current preparation, selection, and
|
||||
finalization entry points. This is test accommodation, not consumer compatibility.
|
||||
Other tests already exercise actual finalization and session preparation, so
|
||||
the finding is not a claim that these behaviors have no coverage.
|
||||
|
||||
**Potential remediation:** use the actual private lifecycle/preparation methods
|
||||
where the test is asserting their effects, and remove the obsolete production
|
||||
wrappers. If concise test setup still benefits from a helper, keep it in a
|
||||
`_test.go` file and delegate to the real path. A read-only test observation helper
|
||||
such as `reportingState.presentation` can likewise live with the tests; it is
|
||||
not itself an alternate lifecycle. Do not remove useful behavioral assertions,
|
||||
export new testing hooks, or replace these few helpers with a testing framework.
|
||||
|
||||
## Machinery worth retaining under the selected contracts
|
||||
|
||||
These were pressure-tested as potential deletion targets. Their responsibilities
|
||||
are more substantial than their surface resemblance to dependency functionality.
|
||||
|
||||
| Area | Why direct delegation or deletion is insufficient |
|
||||
| --- | --- |
|
||||
| Core declarations, neutral metadata, and fresh backend sessions | The selected facade isolates backend types, captures declarations, installs atomically, converts supplied inputs before factories, and supports fresh mutable parse state. Calling Cobra execution directly changes these boundaries. There is no need for a public backend/plugin framework. |
|
||||
| Alias prefix resolution and termination analysis | Repeated longest-prefix rewrites and residual tokens support selected chained/group aliases. An expanding cycle need not repeat an identical whole command path. A visited-string check or arbitrary hop cap would weaken correctness or introduce an unselected restriction. A single-rewrite product would simplify this, but would change accepted application alias behavior; no such change is recommended here. |
|
||||
| Scope plans and independently prepared diff inputs | Scope capabilities, aggregate operations, and independent left/right selections are selected CLI contracts. Loading through application callbacks preserves startup ownership. Sharing one prepared input or inferring the right side from the left would alter those contracts. |
|
||||
| Native request conversion and reusable request flags | CLI strings, files, explicit presence, and occurrence order must become native PromptKit request values. The implementation delegates domain preparation and smoke execution upstream. The flag collections serve explicitly selected application and standard-command reuse, rather than a general internal PromptKit convenience layer. |
|
||||
| Safe configuration YAML to shared report values | ConfigKit exposes effective output and snapshots through safe YAML interfaces. Returning the original Go settings could bypass native redaction; raw YAML passthrough would bypass selected shared formatting and JSON support. The conversion loses presentation details by design and does not rerun configuration resolution. |
|
||||
| Shared JSON/YAML normalization and numeric handling | JSON tags and marshalers define the common report meaning; ordinary YAML marshaling of a `json.Number` is not automatically an equivalent numeric representation. The existing adapter preserves that meaning. CLI JSON's bounded native numeric conversion is a separate selected policy, not grounds for silently rounding or rejecting application report values. No arbitrary-precision public number system is warranted. |
|
||||
| PromptKit report projections | Native smoke reports explicitly have no stable JSON contract. The projection supplies the selected field names, duration strings, empty collections, and unknown/false distinctions. Passing the native struct directly would visibly change reports. This is CLI content adaptation; a reflection-based tagging framework would merely move the complexity. |
|
||||
| Reporter state and synchronization | Visibility, deferred messages, operation-versus-presentation evidence, borrowed writers, and concurrent whole reporter operations are selected behavior. A bare `fmt.Fprint` cannot preserve them. Existing concurrency/resource findings remain valid and separate; retaining the boundary does not endorse every current implementation detail. |
|
||||
|
||||
The [complexity audit](03-complexity.md) also identified duplicate ConfigKit
|
||||
diagnostic adaptation across the integrations (M5). Both already delegate
|
||||
diagnostic text to ConfigKit. Sharing that adapter could reduce maintenance,
|
||||
but adding a public helper is an API decision, not evidence that the adaptation
|
||||
can simply be deleted. The earlier finding remains open; this review does not
|
||||
recommend creating another abstraction to resolve it automatically.
|
||||
|
||||
## Evidence and limits
|
||||
|
||||
- Used the repository code graph for symbol/call discovery, inspected source and
|
||||
selected history, and checked textual references where graph receiver-name
|
||||
collisions or callback references made a call trace insufficient. No-call
|
||||
graph results alone were not treated as proof of dead code.
|
||||
- Consulted the pinned dependencies' local public GoDoc, including ConfigKit
|
||||
`Path` and PromptKit `SmokeTestReport`, to check delegation boundaries.
|
||||
- Ran 21 existing focused top-level tests, all passing, in the aligned workspace:
|
||||
parser/session isolation, reporter visibility/closure/finalization, native diff
|
||||
projection and pointer tokens, smoke forwarding/projection/preparation errors,
|
||||
requirements evidence, CLI numeric policy, and shared report number
|
||||
preservation. Subtests are additional to that count.
|
||||
- Used the cached Go 1.27.0 compiler with `GOTOOLCHAIN=local`, `GOPROXY=off`,
|
||||
`-mod=readonly`, and a writable temporary build cache. No dependency files
|
||||
changed. The preceding [documentation audit](13-documentation.md) records
|
||||
passing full workspace and independent-module suites for the unchanged code;
|
||||
those full suites were not repeated for this document-only review.
|
||||
- No production code or repository tests were edited, and no replacement
|
||||
implementations were tested. Passing current tests substantiates the existing
|
||||
boundaries; it does not prove that a proposed deletion is already safe to
|
||||
merge. Each eventual change still needs focused validation of its effects.
|
||||
- No network service, live model, release publication, or external consumer
|
||||
migration was exercised. No additional restrictions on caller input or
|
||||
upstream behavior are proposed by this report.
|
||||
|
||||
Recommended order: resolve the small public-interface decision in R1, remove
|
||||
the unused parser resource plumbing in R2, then address the bounded R3/R4
|
||||
cleanup when touching those areas. No architectural rewrite is warranted by
|
||||
these findings.
|
||||
@@ -1,586 +0,0 @@
|
||||
# Consolidated audit catalog
|
||||
|
||||
Status: **no open findings**, consolidated from 30
|
||||
numbered entries in seven audit documents. Reviewed audit state:
|
||||
`db732e3a1de55c844f77870e1c6db683c2685d5d`.
|
||||
|
||||
This catalog owns grouping, recommended review order, and subsequent remediation
|
||||
decisions. The linked original entries retain code locations, evidence,
|
||||
contract analysis, and detailed
|
||||
remediation options. Consolidation alone does not resolve a finding or select a
|
||||
proposed contract change. Explicit decisions recorded below supersede the
|
||||
original entries' alternatives; findings remain open until remediated.
|
||||
Original priorities are retained. Within those priorities, incorrect outcomes
|
||||
and request isolation lead; blocked validation,
|
||||
resource retention, and consumer contracts precede scaling and maintainability
|
||||
work; editorial and historical navigation issues follow.
|
||||
|
||||
The [remediation analysis](remediation-analysis.md) records the completed design
|
||||
review and its rationale. All choices identified for discussion have been
|
||||
resolved; no further product or architectural decision is required before
|
||||
implementation planning. Accepted conclusions are recorded in this catalog;
|
||||
the remaining findings still require remediation and validation.
|
||||
|
||||
Three pairs describe the same underlying issue: correctness C2/concurrency N2,
|
||||
correctness C4/concurrency N3, and complexity M6/responsibility R1. Other
|
||||
related findings remain distinct where they concern different failure modes or
|
||||
changes. Documentation cross-references and the responsibility review's closing
|
||||
assessment are supporting discussion, not additional numbered findings.
|
||||
|
||||
The [internal overview](../../internal/overview.md) inventories the
|
||||
architecture: core owns declarations, routing, typed input preparation,
|
||||
dispatch, completion, and reporting; neutral private packages isolate the
|
||||
Cobra/pflag adapter. ckconfig and ckprompt adapt native upstream capabilities
|
||||
through separate optional modules. Applications retain loaders, builders,
|
||||
runtime resources, and executable lifecycle; release launchers delegate to
|
||||
ReleaseKit. The findings call for specific fixes and simplifications within
|
||||
these boundaries.
|
||||
|
||||
## F01 — P2: Aggregate smoke testing can stop on cancellation and return success
|
||||
|
||||
**Complete.** The later cancellation checkpoint retains a cause when work is
|
||||
omitted, without duplicating a cause already recorded. Focused aggregate and
|
||||
cancellation tests pass in the aligned workspace; the new progress-delivery
|
||||
regression failed against the original behavior for both successful and failed
|
||||
provider calls, and also covers cancellation after the last completed check.
|
||||
|
||||
In ckprompt's `runSmokeAggregate`, cancellation triggered during progress
|
||||
delivery can stop the remaining smoke kinds without entering the operation
|
||||
cause. A partial aggregate can consequently return status 0; cancellation can
|
||||
also be omitted when another error already exists.
|
||||
|
||||
Record cancellation at the checkpoint that actually omits remaining work,
|
||||
preserving acquired reports and prior errors. This does not require treating
|
||||
cancellation after all requested work has completed as a failure.
|
||||
|
||||
**Original:** [04-concurrency N1](04-concurrency.md#n1--p2-cancellation-can-stop-aggregate-smoke-testing-with-a-success-outcome).
|
||||
Also noted in the [documentation cross-references](13-documentation.md#previously-recorded-discrepancies-affecting-documented-behavior).
|
||||
|
||||
## F02 — P2: CLI provider parameters share mutable state across requests
|
||||
|
||||
**Complete.** CLI JSON is copied at captured binding application and each
|
||||
aggregate seed boundary. Both regressions failed before the fix and focused
|
||||
aligned-workspace tests passed, including nested objects and arrays, nil and
|
||||
empty values, and numeric types.
|
||||
|
||||
ckprompt assigns parsed `ExtraParams` maps directly to request destinations, and
|
||||
smoke seeds share those nested maps. Mutation by one callback can change later
|
||||
smoke requests or another destination receiving the same captured bindings.
|
||||
|
||||
Clone the CLI-owned JSON maps and arrays at binding application and seed
|
||||
creation. Preserve presence and numeric semantics, and application builders'
|
||||
authority; a general deep-copy framework is unnecessary.
|
||||
|
||||
**Original:** [01-correctness C1](01-correctness.md#c1--p2-cli-provider-parameter-objects-leak-mutations-between-requests).
|
||||
Also noted in the [documentation cross-references](13-documentation.md#previously-recorded-discrepancies-affecting-documented-behavior).
|
||||
|
||||
## F03 — P2: Finalization can discard an accepted deferred message
|
||||
|
||||
**Complete.** The acceptance cutoff and batch detachment now share one locked
|
||||
transition. A deterministic regression reproduces the former lost submission and
|
||||
checks late empty/hidden submissions, accepted delivery, completion, and terminal
|
||||
ordering. Focused reporter tests passed normally and under the race detector;
|
||||
existing tests cover visibility and presentation failures.
|
||||
|
||||
Core reporter finalization detaches the deferred batch before closing
|
||||
acceptance. A concurrent submission can return success while presentation is in
|
||||
progress, then be discarded when finalization clears the queue. The returned
|
||||
delivery evidence can still indicate success.
|
||||
|
||||
**Decision:** atomically close deferred-message acceptance when finalization
|
||||
begins, deliver the already accepted batch under the existing presentation rules,
|
||||
and return `ErrReporterClosed` for later deferred submissions. Preserve the
|
||||
finalizer's own delivery path and document the acceptance cutoff. Repeatedly
|
||||
draining newly arriving submissions is not selected; completion must not depend
|
||||
on application producers eventually stopping. This does not add responsibility
|
||||
for application goroutines or change ordinary message visibility.
|
||||
|
||||
The two source entries describe the same logical ordering defect, rather than a
|
||||
data race.
|
||||
|
||||
**Originals:** [01-correctness C2](01-correctness.md#c2--p2-finalization-can-discard-a-successfully-queued-deferred-message);
|
||||
[04-concurrency N2](04-concurrency.md#n2--p2-deferred-message-acceptance-remains-open-after-the-final-batch-detaches).
|
||||
Also noted in the [documentation cross-references](13-documentation.md#previously-recorded-discrepancies-affecting-documented-behavior).
|
||||
|
||||
## F04 — P2: Prepared-request preview can succeed without a prepared result
|
||||
|
||||
**Complete.** The command rejects a missing prepared request after provider and
|
||||
cancellation checks, preserving acquired evidence. Its public-command regression
|
||||
failed against the original behavior; focused preparation tests pass with valid,
|
||||
missing, provider-error and canceled acquisitions.
|
||||
|
||||
The ckprompt prepared-request handler accepts an empty `Acquired` value with a
|
||||
nil error from `PrepareRequest`. It renders `null` and reports success despite
|
||||
having no prepared request.
|
||||
|
||||
Reject the missing result as an operational failure in this handler while
|
||||
preserving diagnostics and explanations. Keep core's general ability to render a
|
||||
legitimate null report.
|
||||
|
||||
**Original:** [01-correctness C3](01-correctness.md#c3--p2-prepared-request-inspection-reports-success-without-a-result).
|
||||
Also noted in the [documentation cross-references](13-documentation.md#previously-recorded-discrepancies-affecting-documented-behavior).
|
||||
|
||||
## F05 — P2: Configuration-show cancellation is also recorded as a rendering failure
|
||||
|
||||
**Complete.** A cancellation checkpoint stops buffered show preparation without
|
||||
reporting cancellation as a render error. A regression failed before the fix;
|
||||
focused tests now verify no partial output, cancellation identity, and retained
|
||||
native rendering errors when both occur.
|
||||
|
||||
ckconfig checks cancellation during buffered show preparation inside the
|
||||
`RenderOutput` callback. Returning that cancellation from the renderer records
|
||||
both the correct operation cause and a false presentation failure, conflating
|
||||
two established outcome dimensions.
|
||||
|
||||
Carry preparation cancellation separately from renderer errors while retaining
|
||||
genuine rendering and writing failures and avoiding partial aggregate output.
|
||||
Both audits identify the same classification defect.
|
||||
|
||||
**Originals:** [01-correctness C4](01-correctness.md#c4--p2-show-preparation-labels-ordinary-cancellation-as-a-rendering-error);
|
||||
[04-concurrency N3](04-concurrency.md#n3--p2-buffered-configuration-show-cancellation-becomes-a-presentation-failure).
|
||||
Also noted in the [documentation cross-references](13-documentation.md#previously-recorded-discrepancies-affecting-documented-behavior).
|
||||
|
||||
## F06 — P3: The optional interactive completion consumer fails before shell validation
|
||||
|
||||
**Complete.** The renamed application-owned choice flag permits script
|
||||
generation and the Bash interactive consumer and backend checks pass. Zsh and
|
||||
Fish interactive checks remain unavailable because those shells are not installed.
|
||||
|
||||
The completion consumer fixture declares the reserved `format` flag, so App
|
||||
construction fails before it can generate scripts or exercise interactive
|
||||
completion. Default test suites skip this optional path.
|
||||
|
||||
Use core's format flag or rename the fixture's custom flag, update its
|
||||
expectations, and rerun available shell checks. The fixture failure does not
|
||||
establish a defect in generated completion scripts.
|
||||
|
||||
**Original:** [01-correctness C6](01-correctness.md#c6--p3-the-interactive-completion-consumer-cannot-construct-its-app).
|
||||
Also noted in the [documentation cross-references](13-documentation.md#previously-recorded-discrepancies-affecting-documented-behavior).
|
||||
|
||||
## F07 — P3: Closed reporting handles retain both borrowed destinations
|
||||
|
||||
**Complete.** Finalization drops direct destination references after presentation
|
||||
is captured. Focused reporter tests cover successful, failed and canceled outcomes,
|
||||
closed handles, retained causes, and caller ownership.
|
||||
|
||||
A retained closed `Reporter`, `Output`, or `DeferredReporter` keeps the shared
|
||||
reporting state and both destination writers reachable. Applications retaining a
|
||||
closed handle can therefore retain large buffers or resource-bearing writer
|
||||
objects after invocation completion.
|
||||
|
||||
Clear direct borrowed-writer references after final presentation under the
|
||||
reporting lock, preserving closed-handle behavior and returned error evidence.
|
||||
Do not close or flush application-owned writers. This is conditional retention
|
||||
through a reachable handle, not an unreachable heap leak.
|
||||
|
||||
**Original:** [05-resources RM1](05-resources.md#rm1--p3-closed-reporting-handles-retain-both-destination-objects).
|
||||
|
||||
## F08 — P3: The core example overwrites its optional argument default
|
||||
|
||||
**Complete.** The example assigns a scenario only when supplied; bare and
|
||||
explicit demo invocations succeed and an invalid scenario returns usage status 2.
|
||||
|
||||
The example's scenario setter replaces the factory's `"demo"` default with an
|
||||
unsupplied empty value. The ordinary `report` invocation consequently fails with
|
||||
a usage error.
|
||||
|
||||
Assign the optional argument only when its supplied state is set. Fix the
|
||||
consumer without changing core's optional-binding semantics.
|
||||
|
||||
**Original:** [01-correctness C5](01-correctness.md#c5--p3-the-core-example-overwrites-its-optional-scenario-default).
|
||||
|
||||
## F09 — P3: The configuration provider contract omits its required YAML representation
|
||||
|
||||
**Complete.** EffectiveWriter, View and DecodeYAMLReport GoDoc describe the
|
||||
actual single-document conversion boundary; focused conversion and show tests pass.
|
||||
|
||||
`EffectiveWriter` GoDoc promises a native representation but omits that `config
|
||||
show` converts one supported YAML document through `DecodeYAMLReport`. Custom
|
||||
providers can supply output that appears compatible with the description yet
|
||||
fails conversion; native formatting is not passed through.
|
||||
|
||||
Document that boundary with the public interface and conversion helper, and
|
||||
update `View`'s report description. This clarification is independent of the
|
||||
unused explanation-writer requirement in F11.
|
||||
|
||||
**Original:** [13-documentation D1](13-documentation.md#d1--p3-configuration-provider-godoc-omits-the-effective-output-contract).
|
||||
|
||||
## F10 — P3: Public flag metadata omits enforced naming and reserved-spelling rules
|
||||
|
||||
**Complete.** FlagMeta documents the existing declaration rules and related
|
||||
rename and alias GoDoc refers to them. The prose was checked against the
|
||||
installation validator; no runtime rule changed.
|
||||
|
||||
`FlagMeta` describes names without leading dashes and an optional short
|
||||
spelling, while installation enforces additional name grammar and reserved
|
||||
spellings. Consumers must discover those constraints through errors or private
|
||||
implementation.
|
||||
|
||||
**Decision:** retain and document the existing declaration rules. Long names use
|
||||
Unicode lowercase letters and digits separated by single hyphens; numeric names
|
||||
and names beginning with digits remain allowed. Keep core-owned reserved names
|
||||
and the optional one-byte short-spelling rules, excluding `-` and reserved `h`.
|
||||
Do not add ASCII-only or leading-letter restrictions, or automatic rewriting of
|
||||
invalid names. These rules concern flag names, not supplied values.
|
||||
|
||||
Document the rules once with `FlagMeta` and link related public helpers. This
|
||||
remediation changes documentation, not runtime validation.
|
||||
|
||||
The [F10 analysis](remediation-analysis.md#f10--flag-declaration-naming) reviews
|
||||
the naming convention's basis and records the accepted direction.
|
||||
|
||||
**Original:** [13-documentation D2](13-documentation.md#d2--p3-public-flag-metadata-omits-enforced-declaration-constraints).
|
||||
|
||||
## F11 — P3: Explanation providers must implement an unused text writer
|
||||
|
||||
**Complete.** The interface now requires only Explain; focused tests exercise an
|
||||
Explain-only provider for focused show and explanations across formats, including
|
||||
delivery failures. Obsolete writer stubs and the interface-writer assertion were removed.
|
||||
|
||||
`ExplanationReader` still requires `WriteExplanation` even though production
|
||||
show/explain handlers acquire evidence with `Explain` and core renders it. This
|
||||
burdens custom providers and suggests a presentation responsibility that
|
||||
standard commands no longer use.
|
||||
|
||||
**Decision:** remove `WriteExplanation` from `ExplanationReader`, leaving the
|
||||
acquisition responsibility, and update GoDoc, writer-only stubs, and assertions.
|
||||
Native provider objects may retain their own rendering methods. Do not add
|
||||
compatibility scaffolding or restore provider-owned rendering to justify the
|
||||
unused requirement. Both audits identify the same obsolete requirement.
|
||||
|
||||
The interface change removes the ability to invoke that method through
|
||||
`View.Explanation`. CommandKit is still pre-release with no consumers, so no
|
||||
consumer compatibility or migration work is needed.
|
||||
|
||||
**Originals:** [03-complexity M6](03-complexity.md#m6--p3-explanation-providers-must-implement-an-unused-rendering-capability);
|
||||
[A1-responsibility R1](A1-responsibility.md#r1--p3-explanation-providers-still-owe-an-unused-text-writer).
|
||||
|
||||
## F12 — P3: Parser sessions carry unused execution resources
|
||||
|
||||
**Complete.** Unused execution and parser resource plumbing was removed; the
|
||||
backend retains discard destinations and buffered help. Focused routing, help,
|
||||
context-forwarding and session-isolation tests passed.
|
||||
|
||||
Core and the Cobra adapter duplicate stream normalization and retain context and
|
||||
streams through parser options and backend nodes, although the adapter parses
|
||||
and renders buffered help rather than executing callbacks or reading application
|
||||
input. The execution object also retains an unread streams field.
|
||||
|
||||
Remove unused resource plumbing while preserving real invocation resources and
|
||||
explicit internal discard destinations/local help buffers. This simplifies
|
||||
ownership; it is separate from the closed-reporting-handle retention in F07.
|
||||
|
||||
**Original:** [A1-responsibility R2](A1-responsibility.md#r2--p3-parser-sessions-carry-application-execution-resources-they-do-not-use).
|
||||
|
||||
## F13 — P3: Flag-alias compilation repeatedly walks shared chain suffixes
|
||||
|
||||
**Complete.** Each effective context memoizes its target resolutions, including
|
||||
inactive endpoints, while retaining canonical precedence and cycle diagnostics.
|
||||
Focused alias and discovery tests pass, including shared suffixes and registration.
|
||||
|
||||
For A chained effective flag mappings across C contexts, independent target
|
||||
walks can perform Θ(CA²) target examinations, including inactive chains. Each
|
||||
source repeats work already performed for another source's suffix.
|
||||
|
||||
Use context-local visitation states and memoized terminal results. Traversal can
|
||||
become expected O(A) per context, with existing deterministic sorting retaining
|
||||
O(A log A) total work for these steps. Preserve scoped precedence, canonical
|
||||
endpoints, missing-target behavior, and cycle diagnostics.
|
||||
|
||||
**Decision for F13–F15:** the repeated-work and scaling analysis sufficiently
|
||||
justifies remediation. Performance testing is not a prerequisite for selecting
|
||||
or implementing these changes. Validate correctness and preservation of the
|
||||
selected routing and discovery semantics.
|
||||
|
||||
**Original:** [02-efficiency E1](02-efficiency.md#e1--p3-flag-alias-compilation-repeatedly-resolves-shared-chain-suffixes).
|
||||
|
||||
## F14 — P3: Discovery-context preparation repeatedly rebuilds shared prefixes
|
||||
|
||||
**Complete.** Distinct trie prefixes are enumerated and classified once;
|
||||
focused discovery and resolver tests pass, and traversal was reviewed for
|
||||
elimination of repeated per-declaration prefix construction.
|
||||
|
||||
R alias sources sharing a depth-D proper prefix cause Θ(RD²) copying and key
|
||||
construction before result deduplication, with repeated classification as well.
|
||||
The source declarations contain only Θ(RD) tokens.
|
||||
|
||||
Traverse distinct source prefixes once and classify each once. For the audited
|
||||
shape, this phase can become O(RD + D²).
|
||||
|
||||
**Decision:** implement F14 and F15 together using the existing alias resolver's
|
||||
source-path tree. Add narrow internal read-only operations to enumerate distinct
|
||||
nonempty proper prefixes for F14 and find strictly descendant alias sources for
|
||||
F15. Retain the existing discovery classification and exclusions, applying them
|
||||
once per candidate prefix.
|
||||
|
||||
Reuse the tree's routing-snapshot lifetime; do not introduce a parallel index or
|
||||
a cross-invocation result cache. The repeated preparation and help-time scan
|
||||
remain distinct findings. The [joint analysis](remediation-analysis.md#f14f15--joint-source-prefix-design)
|
||||
records the accepted design. The performance-testing decision under F13 applies
|
||||
to both.
|
||||
|
||||
**Original:** [02-efficiency E3](02-efficiency.md#e3--p3-discovery-context-preparation-rebuilds-shared-prefixes-repeatedly).
|
||||
|
||||
## F15 — P3: Alias help scans the entire source catalog for each intermediate route
|
||||
|
||||
**Complete.** Help queries strict descendants from each resolution path instead
|
||||
of scanning the complete catalog. Focused alias/help tests pass, and the tree
|
||||
traversal was reviewed for the removal of unrelated source scans.
|
||||
|
||||
Help discovery examines all A alias sources at each of P rewrite paths, doing PA
|
||||
examinations and repeated copies even when there are no descendants. A valid
|
||||
single-token alias chain produces Θ(A²) work to discover an empty alias section
|
||||
on every such help request.
|
||||
|
||||
**Decision:** query the existing resolver tree for strict descendants through
|
||||
the internal operation shared with F14's design. Continue visiting intermediate
|
||||
resolution paths, resolving composed entered paths through the existing
|
||||
resolver, and retaining canonical-target checks, suffix deduplication, and
|
||||
output sorting. Root help may retain the complete catalog when all entries are
|
||||
needed. No public API or CLI behavior change is selected.
|
||||
|
||||
The identified discovery work becomes O(A) for that example; general cases still pay for
|
||||
matching candidates, resolution, and output sorting. Design this together with
|
||||
F14 under the decisions recorded there and in F13.
|
||||
|
||||
**Original:** [02-efficiency E2](02-efficiency.md#e2--p3-alias-help-scans-every-source-for-every-intermediate-route).
|
||||
|
||||
## F16 — P3: Parsing and completion duplicate flag-consumption grammar
|
||||
|
||||
**Complete.** A neutral consumption operation is shared by execution parsing
|
||||
and completion. Focused grammar, provenance and completion tests and available
|
||||
Bash interactive checks pass; missing-value and partial-input policies remain
|
||||
with their callers.
|
||||
|
||||
Execution parsing and completion separately implement short clusters,
|
||||
attached/next values, boolean assignments, terminators, and unknown spellings.
|
||||
Shared classification helpers cover only part of the grammar, leaving changes to
|
||||
cross the core/backend boundary.
|
||||
|
||||
**Decision:** share a small flag-consumption operation through the existing
|
||||
neutral internal helpers. Share facts about flag identity, attached versus
|
||||
missing values, consumption of the next argument, and short-cluster position.
|
||||
Keep execution normalization, failure provenance, and partial-word completion
|
||||
policies in their existing owners. Preserve explicitly empty values and current
|
||||
CLI behavior; no public parser or replacement backend is selected.
|
||||
|
||||
Retain caller-specific branches where combining them would require extensive
|
||||
modes or special cases. Repair F06's completion fixture before validating this
|
||||
refactor. The [F16 analysis](remediation-analysis.md#f16--shared-flag-consumption)
|
||||
records the accepted boundary.
|
||||
|
||||
**Original:** [03-complexity M3](03-complexity.md#m3--p3-parsing-and-completion-maintain-parallel-flag-consumption-logic).
|
||||
|
||||
## F17 — P3: Configuration scope capture combines several compilation phases
|
||||
|
||||
**Complete.** Scope capture, selector attachment, operation binding unions and
|
||||
inventory construction are distinct responsibilities. Focused family, selector,
|
||||
comparison, capture and discovery checks pass.
|
||||
|
||||
ckconfig's `captureSuite` interleaves scope and selector capture, validation,
|
||||
comparison-binding renaming, binding attachment, operation unions, and discovery
|
||||
inventory. Several partially populated collections must be understood together.
|
||||
|
||||
**Decision:** extract cohesive scope capture/validation and selector attachment
|
||||
responsibilities, and make operation bindings and discovery inventory
|
||||
construction explicit. Use existing prepared types and keep related loops
|
||||
together. Preserve retained error order, declaration ownership, scope and binding
|
||||
identity, and metadata-only construction. Do not create new intermediate records
|
||||
or a helper for each loop merely to shorten the function.
|
||||
|
||||
**Original:** [03-complexity M1](03-complexity.md#m1--p3-scope-capture-combines-several-compilation-phases).
|
||||
|
||||
## F18 — P3: Configuration preflight interleaves shared and operation-specific rules
|
||||
|
||||
**Complete.** Operation checks, scope selection and two-sided input preparation
|
||||
have separate private functions; comparison flags are grouped. Focused preflight,
|
||||
help, scope, override, show and comparison tests pass.
|
||||
|
||||
The configuration command builder and `prepareConfigOperation` mix flag
|
||||
combinations, help selection, capabilities, pointer parsing, plan assembly,
|
||||
ordinary inputs, and independent comparison inputs. Understanding one operation
|
||||
requires reasoning through irrelevant submodes.
|
||||
|
||||
**Decision:** separate operation-specific option checks, shared scope/capability
|
||||
selection, and comparison input preparation with small private functions. Keep
|
||||
the overall sequence explicit, including explain's help path, aggregate checks
|
||||
before input construction, independent comparison sides, and input checks before
|
||||
shared state creation and acquisition. Extract flag groups where this clarifies
|
||||
their purpose; no generic operation engine or rule registry is selected.
|
||||
|
||||
**Original:** [03-complexity M2](03-complexity.md#m2--p3-configuration-preflight-mixes-shared-and-operation-specific-rules).
|
||||
|
||||
## F19 — P3: Typed input preparation mixes conversion, provenance, and replay assembly
|
||||
|
||||
**Complete.** Conversion-error attribution and operand preparation are extracted
|
||||
without replacing flag ordering or replay. Focused conversion, occurrence,
|
||||
capture and replay tests pass.
|
||||
|
||||
`prepareInputWithOccurrences` handles shape validation, conversion-error
|
||||
spelling, occurrence identities, application order, required flags, operands,
|
||||
and prepared callbacks. Captured binding replay depends on those shared
|
||||
identities across another code path.
|
||||
|
||||
**Decision:** make small extractions for conversion-error attribution and operand
|
||||
preparation. Keep flag preparation, occurrence identity, and order reconstruction
|
||||
together; that block may remain in the orchestration function if extraction
|
||||
requires excessive plumbing. Retain dense reconstruction, the captured-replay
|
||||
heap merge, binding identity, supplied-state distinctions, conversion before
|
||||
input factories, and error identity. No new state containers or replay framework
|
||||
are selected.
|
||||
|
||||
**Original:** [03-complexity M4](03-complexity.md#m4--p3-typed-preparation-combines-conversion-provenance-and-replay-assembly).
|
||||
|
||||
## F20 — P3: Reporter message paths duplicate detail-rendering bookkeeping
|
||||
|
||||
**Complete.** Both message paths share detail callback and error bookkeeping;
|
||||
focused reporter tests cover visibility, ordering, closed handles, and joined
|
||||
render/write evidence.
|
||||
|
||||
Terminal and ordinary message paths repeat callback invocation, managed-writer
|
||||
setup, render-versus-write error classification, and first-error retention.
|
||||
Maintaining the same presentation policy in two branches increases the chance of
|
||||
drift.
|
||||
|
||||
**Decision:** extract one private detail-rendering helper under the existing
|
||||
reporting lock, returning rendering and writing errors separately. Callers retain
|
||||
visibility/discard selection, headings, delivery observations, and terminal
|
||||
timing. Preserve callback order, hidden-detail execution, sticky writer failures,
|
||||
and non-reentrancy; add no locks or interchangeable rendering machinery.
|
||||
|
||||
Keep this refactor separately reviewable from F03 and F07; it does not itself
|
||||
fix deferred-message loss or writer retention. After correctness repairs, favor
|
||||
F20, then F18 and F17, while keeping F19 modest. All four use private functions
|
||||
and existing types, with no intended public API or CLI behavior change. The
|
||||
[F17–F20 analysis](remediation-analysis.md#f17f20--proportionate-private-extractions)
|
||||
records the accepted scope.
|
||||
|
||||
**Original:** [03-complexity M7](03-complexity.md#m7--p3-terminal-and-ordinary-messages-duplicate-detail-rendering-mechanics).
|
||||
|
||||
## F21 — P3: Both integrations maintain the same ConfigKit diagnostic adaptation
|
||||
|
||||
**Complete.** Both integrations use `ckconfig.AdaptDiagnostics`; focused
|
||||
workspace adapter, acquisition and comparison tests pass, including captured
|
||||
locations, severity grouping, side labels and deferred writer failures. ckprompt's
|
||||
independent check against released ckconfig v0.4.0 remains blocked on release
|
||||
alignment because that version lacks the helper.
|
||||
|
||||
ckconfig and ckprompt independently snapshot ConfigKit diagnostic evidence,
|
||||
partition severities, construct detail callbacks, label sources, and delegate
|
||||
native text formatting. Related Reporter scheduling switches are duplicated too.
|
||||
|
||||
**Decision:** expose one narrow public ckconfig diagnostic adaptation helper
|
||||
and use it in both integrations. Reuse the existing private helper's shape:
|
||||
accept a caller-supplied heading, native sources, and native diagnostic records;
|
||||
return existing core diagnostics and an explanation ready for CLI reporting.
|
||||
Preserve evidence snapshots, severity treatment, ordering within each group,
|
||||
source labels, and delegation to ConfigKit's native formatter.
|
||||
|
||||
Callers retain operation-specific context, outcomes, error causes, and message
|
||||
scheduling. Core retains visibility and delivery. Leave the smaller scheduling
|
||||
switches with their callers; no additional public scheduling API or general
|
||||
diagnostics framework is selected. ckprompt already depends on ckconfig, so no
|
||||
new module dependency is needed.
|
||||
|
||||
CommandKit is pre-release with no consumers; compatibility and migration are
|
||||
not constraints. The [F21 analysis](remediation-analysis.md#f21--shared-configkit-diagnostic-adaptation)
|
||||
records the accepted scope and illustrative signature.
|
||||
|
||||
**Original:** [03-complexity M5](03-complexity.md#m5--p3-configuration-diagnostic-adaptation-is-maintained-in-both-integrations).
|
||||
Also discussed in the [responsibility assessment](A1-responsibility.md#machinery-worth-retaining-under-the-selected-contracts).
|
||||
|
||||
## F22 — P3: Successful declaration validation eagerly formats every full path
|
||||
|
||||
**Complete.** The walk retains one shared scratch slice header across siblings,
|
||||
truncates on every return, and formats only retained issues or root-only placement
|
||||
errors. The [R1 follow-up](../remediation.md#r1--p3-declaration-validation-can-still-copy-a-shared-deep-prefix-for-every-sibling)
|
||||
completes the initial path refactor and records source review, passing focused
|
||||
and full suites, and a deep branching consumer check.
|
||||
|
||||
`collectDeclarationIssues` copies and formats each ancestral path before knowing
|
||||
whether a diagnostic is needed. A valid depth-N declaration chain incurs Θ(N²)
|
||||
cumulative path work and allocation while producing no errors.
|
||||
|
||||
Use an append/truncate scratch path and format only emitted diagnostics. The
|
||||
successful validation walk can become O(N) with O(D) path storage. Other
|
||||
installation phases retain their own costs; this is not a claim that all App
|
||||
construction becomes linear.
|
||||
|
||||
**Original:** [02-efficiency E4](02-efficiency.md#e4--p3-successful-declaration-validation-eagerly-constructs-every-full-path).
|
||||
|
||||
## F23 — P3: Concise diff rendering validates already-valid native paths
|
||||
|
||||
**Complete.** The private decoder accepts ConfigKit's native path and performs
|
||||
single-pass token decoding. Focused diff, token and snapshot tests pass.
|
||||
|
||||
ckconfig's private pointer decoder validates arbitrary strings even though its
|
||||
sole production caller supplies the canonical string of an immutable ConfigKit
|
||||
`Path`. Tests sustain malformed-input branches that this boundary cannot reach.
|
||||
|
||||
Accept the native path directly and retain only the key-token decoding needed by
|
||||
the selected concise report. Decode escapes once so `~01` remains `~1`; preserve
|
||||
snapshot errors and operator-input validation.
|
||||
|
||||
**Original:** [A1-responsibility R3](A1-responsibility.md#r3--p3-concise-diff-rendering-revalidates-native-configkit-paths).
|
||||
|
||||
## F24 — P3: Obsolete private production entry points remain for tests alone
|
||||
|
||||
**Complete.** Reporter selection/finalization, routed parsing and occurrence-aware
|
||||
preparation replace the obsolete wrappers. Graph and text checks confirm their
|
||||
removal; useful lifecycle and replay tests pass.
|
||||
|
||||
Legacy reporter setters/close, `App.parse`, and typed-preparation wrappers have
|
||||
only test callers, while production uses current selection, finalization,
|
||||
routed-session, and occurrence-aware paths. Some tests therefore exercise
|
||||
superseded lifecycle entry points.
|
||||
|
||||
Move tests to the real private paths and remove obsolete production wrappers.
|
||||
Keep any concise setup helpers in `_test.go` files, retaining useful behavioral
|
||||
assertions without new exported test hooks.
|
||||
|
||||
**Original:** [A1-responsibility R4](A1-responsibility.md#r4--p3-private-production-entry-points-exist-only-to-sustain-older-tests).
|
||||
|
||||
## F25 — P3: The reporting ADR mixes implemented status with stale planning language
|
||||
|
||||
**Complete.** ADR status identifies its future-tense lifecycle language as
|
||||
historical and points to current GoDoc and CLI contracts.
|
||||
|
||||
ADR 0006 marks shared reporting implemented but still directs implementation
|
||||
status and format choices to a retired roadmap and describes the extension as
|
||||
planned. Readers can mistake completed work for a pending prerequisite.
|
||||
|
||||
Clarify historical wording and link current behavior to GoDoc and the CLI
|
||||
reference. Preserve the accepted architectural decision; an unchanged decision
|
||||
does not need a replacement ADR.
|
||||
|
||||
**Original:** [13-documentation D3](13-documentation.md#d3--p3-reporting-adr-still-directs-readers-to-a-planned-implementation).
|
||||
|
||||
## F26 — P3: The README duplicates volatile module compatibility guidance
|
||||
|
||||
**Complete.** README keeps module orientation and links to the consumer guide
|
||||
as the owner of exact compatibility and installation guidance.
|
||||
|
||||
The root README repeats exact version combinations and candidate dependency
|
||||
details already maintained by the consumer guide. The facts currently agree, but
|
||||
two owners create policy and maintenance drift risk.
|
||||
|
||||
Keep a brief status/module overview in README and link the consumer guide for
|
||||
exact compatibility and installation choices. This is documentation
|
||||
consolidation, not a dependency change.
|
||||
|
||||
**Original:** [13-documentation D5](13-documentation.md#d5--p3-the-root-readme-duplicates-the-compatibility-matrix).
|
||||
|
||||
## F27 — P3: A historical release note points to a nonexistent section
|
||||
|
||||
**Complete.** A discoverable forward-only erratum links the corrected section,
|
||||
verified against the locally tagged consumer guide. The historical note and tag
|
||||
were not changed.
|
||||
|
||||
The ckprompt v0.2.0 release note uses the wrong consumer-guide fragment at its
|
||||
immutable tag. The file exists, but the requested section does not.
|
||||
|
||||
Provide the correct pinned section link in a useful forward-only correction or
|
||||
erratum. Preserve the published tag, historical note, and hosted release
|
||||
contents; republishing is unnecessary.
|
||||
|
||||
**Original:** [13-documentation D4](13-documentation.md#d4--p3-a-historical-release-note-links-to-a-nonexistent-section).
|
||||
@@ -1,373 +0,0 @@
|
||||
# Audit remediation analysis
|
||||
|
||||
Status: remediation decisions agreed; ready for implementation planning. No
|
||||
implementation changes made.
|
||||
|
||||
This document records analysis of the remediation choices reviewed in the
|
||||
discussion order below. The [catalog](catalog.md) owns the findings, accepted remediation
|
||||
decisions, and their status. Original audits retain detailed evidence. Record
|
||||
accepted conclusions in the catalog and mark the corresponding discussion here
|
||||
decided; a decision does not close a finding before implementation.
|
||||
|
||||
Existing catalog decisions for F03, F10, F11, and F13–F21 remain selected. In
|
||||
particular, performance testing is not a prerequisite for F13–F15, and F14 and
|
||||
F15 are to be designed together. CommandKit is pre-release with no consumers;
|
||||
consumer compatibility and migration are not constraints on these choices.
|
||||
Straightforward correctness repairs need not wait for this discussion sequence.
|
||||
|
||||
| Order | Findings | Question | Discussion status |
|
||||
| --- | --- | --- | --- |
|
||||
| 1 | F10 | Which flag declaration rules should be documented as the intended contract? | Decided: retain and document existing rules; remediation pending |
|
||||
| 2 | F21 | What narrow diagnostic adaptation API would simplify both integrations? | Decided: shared public ckconfig helper; remediation pending |
|
||||
| 3 | F16 | Which flag-consumption facts can parsing and completion usefully share? | Decided: shared internal mechanics, separate caller policies; remediation pending |
|
||||
| 4 | F14–F15 | How should both operations reuse source-prefix processing? | Decided: reuse existing resolver tree; remediation pending |
|
||||
| 5 | F17–F20 | Which private extractions materially simplify the existing code? | Decided: bounded private extractions, conservative F19; remediation pending |
|
||||
|
||||
## F10 — Flag declaration naming
|
||||
|
||||
**Question:** should remediation document the existing naming rules, or change
|
||||
them first? This concerns names declared by applications, not the contents of
|
||||
operator-supplied flag values.
|
||||
|
||||
**Evidence:** [catalog F10](catalog.md#f10--p3-public-flag-metadata-omits-enforced-naming-and-reserved-spelling-rules)
|
||||
and [documentation audit D2](13-documentation.md#d2--p3-public-flag-metadata-omits-enforced-declaration-constraints).
|
||||
`FlagMeta`, `validateBindings`, and `validFlagName` in
|
||||
[command.go](../../../command.go) establish the documentation gap and current
|
||||
validation. Shared reporting spellings are defined in
|
||||
[commandmeta](../../../internal/commandmeta/metadata.go).
|
||||
|
||||
The historical accepted grammar ADR explicitly selected lowercase names with
|
||||
hyphen-separated words. It is available at commit
|
||||
`82128463cd9649a426da7c5e3f3ce33bd37cdd9f`, path
|
||||
`docs/adr/0006-command-grammar.md`; the same commit's
|
||||
`docs/roadmap/initial.md` carried that direction into implementation. This
|
||||
supports the broad naming convention, but does not independently establish every
|
||||
character-level detail of the current validator. Historical records provide
|
||||
context; they do not replace the current canonical API documentation.
|
||||
|
||||
Current long names consist of nonempty groups of Unicode lowercase letters or
|
||||
digits separated by single hyphens. Names may start with digits or consist
|
||||
entirely of digits. Uppercase letters, underscores, dots, and leading, trailing,
|
||||
or repeated hyphens are rejected. Examples:
|
||||
|
||||
| Declaration name | Current result |
|
||||
| --- | --- |
|
||||
| `max-tokens`, `profile2`, `2fa`, `123`, `équipe` | Accepted, subject to ordinary collision checks |
|
||||
| `maxTokens`, `max_tokens`, `max.tokens`, `max--tokens`, `-profile`, `profile-` | Rejected |
|
||||
|
||||
Core reserves `help`, `quiet`, `verbose`, `debug`, `progress`, and `format` for
|
||||
its own flags. A short spelling is optional; when present it is one byte,
|
||||
cannot be `-`, and cannot be the reserved help spelling `h`. The long-name
|
||||
lowercase rule does not apply to short spellings. Declaration defects are
|
||||
reported at installation, before command execution.
|
||||
|
||||
**Options:**
|
||||
|
||||
1. Retain and document the current rules. This makes the shared convention
|
||||
explicit without introducing new validation or conversion machinery.
|
||||
2. Relax the stylistic restrictions, allowing application names such as
|
||||
`maxTokens` or `max_tokens` while preserving core reservations and necessary
|
||||
parsing constraints. This gives applications more naming freedom but weakens
|
||||
the common vocabulary; its precise supported grammar would need selection.
|
||||
3. Tighten the rules, for example to ASCII names beginning with a letter. This
|
||||
creates a narrower convention but removes currently accepted names without
|
||||
an identified operator benefit.
|
||||
|
||||
**Recommendation:** option 1. Retain lowercase, hyphen-separated long names,
|
||||
including current Unicode lowercase and digit support. Keep core reservations
|
||||
and the existing short-spelling rules. Recommend readable names without banning
|
||||
numeric names or adding an ASCII-only restriction. Do not automatically rewrite
|
||||
invalid names or accept alternate spellings through normalization.
|
||||
|
||||
Document the exact declaration contract once in `FlagMeta` GoDoc and link related
|
||||
helpers, including `RenameFlag` and flag-alias declarations, as applicable. Keep
|
||||
operator syntax in the CLI reference. This is a documentation remediation;
|
||||
it does not require a new ADR, parser, or validation framework.
|
||||
|
||||
**Decision:** option 1 accepted and recorded in the catalog. Retain the current
|
||||
long-name grammar, Unicode lowercase and digit support, reservations, and
|
||||
short-spelling rules. No runtime validation change or name normalization is
|
||||
selected. F10 remains open until its public documentation is corrected.
|
||||
|
||||
## F21 — Shared ConfigKit diagnostic adaptation
|
||||
|
||||
**Question:** what concrete helper, if any, removes duplicated CLI adaptation
|
||||
without introducing a broader diagnostics framework?
|
||||
|
||||
**Evidence:** [catalog F21](catalog.md#f21--p3-both-integrations-maintain-the-same-configkit-diagnostic-adaptation)
|
||||
links both adapters and the original analysis.
|
||||
|
||||
**Options considered:** a narrow public ckconfig helper returning existing core
|
||||
diagnostics and an explanation, or retaining private implementations if the
|
||||
shared API adds more complexity than it removes.
|
||||
|
||||
**Analysis:** ckconfig's private `configKitDiagnosticsWithHeading` already
|
||||
accepts a heading, native sources, and native diagnostic records, and returns
|
||||
core diagnostics and an explanation. ckprompt duplicates the evidence snapshot,
|
||||
severity partition, source legend, and delegation to `configkit.WriteDiagnostics`.
|
||||
Its scope and comparison-side labels can be supplied as the heading; they do not
|
||||
require another shared options type. See
|
||||
[ckconfig's adapter](../../../integrations/ckconfig/configkit_adapter.go) and
|
||||
[ckprompt's adapter](../../../integrations/ckprompt/adapters.go).
|
||||
|
||||
**Recommendation:** expose the existing adaptation responsibility through one
|
||||
narrow ckconfig function and have both integrations use it. An illustrative
|
||||
signature, not an implemented API, is:
|
||||
|
||||
```go
|
||||
func AdaptDiagnostics(
|
||||
heading string,
|
||||
sources []configkit.SourceInfo,
|
||||
records []configkit.Diagnostic,
|
||||
) ([]commandkit.Diagnostic, commandkit.Explanation)
|
||||
```
|
||||
|
||||
Retain the existing evidence snapshot and severity treatment, ordering within
|
||||
each group, source labels, and native record formatting. Return evidence ready
|
||||
for CLI reporting; do not print immediately, select an invocation outcome, or
|
||||
load or validate configuration. Callers supply headings and operation-specific
|
||||
explanations and retain their error causes. Reporter remains responsible for
|
||||
visibility and delivery.
|
||||
|
||||
Keep scheduling with the callers. The smaller scheduling switches do not
|
||||
independently justify another public helper. ckprompt already depends on
|
||||
ckconfig; this introduces no new module dependency or ConfigKit dependency in
|
||||
core. The public function serves CLI presentation, within the integration's
|
||||
ownership boundary. Compatibility and migration are not obstacles; clarity and
|
||||
maintenance cost are the decision criteria.
|
||||
|
||||
**Decision:** shared public ckconfig adaptation helper accepted and recorded in
|
||||
the catalog, using the input/output shape above. Both integrations will use it;
|
||||
the smaller scheduling switches remain with their callers. The function name
|
||||
above is illustrative; implementation remains pending.
|
||||
|
||||
## F16 — Shared flag consumption
|
||||
|
||||
**Question:** which consumption facts can be shared while execution and
|
||||
completion retain their different handling of incomplete input?
|
||||
|
||||
**Evidence:** [catalog F16](catalog.md#f16--p3-parsing-and-completion-duplicate-flag-consumption-grammar).
|
||||
|
||||
**Options considered:** a small neutral consumption primitive or deliberate
|
||||
retention of duplication where a shared helper needs extensive caller modes.
|
||||
|
||||
**Analysis:** execution's `interpretFlagTokens` in
|
||||
[internal/cobra/flags.go](../../../internal/cobra/flags.go) and completion's
|
||||
`walkCompletionFlags` and `completionOptionTarget` in
|
||||
[completion_flags.go](../../../completion_flags.go) repeat the decisions about
|
||||
short-flag clusters, attached values, boolean assignments, and whether a flag
|
||||
consumes the next argument. They already share classification and lookup through
|
||||
[commandmeta's flag helpers](../../../internal/commandmeta/flags.go).
|
||||
|
||||
Both paths need to recognize that `--profile=team` has an attached value and
|
||||
`--profile team` takes the following argument. Their responses to unfinished
|
||||
input differ: executing a bare `--profile` requires a missing-value error, while
|
||||
completion after `--profile ` should offer values. Likewise, an unfinished
|
||||
`--pro` can be useful for suggesting flag names without being a valid execution
|
||||
argument. A single policy-heavy parser would obscure that difference.
|
||||
|
||||
**Recommendation:** extend the existing neutral internal helpers with a small
|
||||
consumption operation. Given flag metadata and the current spelling/position,
|
||||
it should expose facts such as the identified flag, any attached value, whether
|
||||
the next argument is required, and where to continue within a short cluster.
|
||||
Distinguish an explicitly empty attached value from an absent value. The exact
|
||||
private signature is an implementation choice, not a new public contract.
|
||||
|
||||
Execution retains alias normalization, entered-spelling and occurrence tracking,
|
||||
backend error handling, and typed conversion. Completion retains unfinished-word
|
||||
handling, suggestion filtering, and insertion text. Existing token classification
|
||||
continues to identify `--`; callers retain their appropriate end-of-options
|
||||
handling. Supply each caller's existing lookup rather than extending completion
|
||||
to new alias-discovery behavior. Do not add execution/completion mode flags,
|
||||
callback registries, another public parser, or a replacement for Cobra/pflag.
|
||||
|
||||
Repair F06's fixture before validating the change. Use focused behavioral
|
||||
coverage for shared grammar and the deliberately different missing/partial-value
|
||||
paths, preserving explicit empties, short clusters, and entered spelling.
|
||||
If a proposed extraction needs extensive caller-specific controls, narrow it
|
||||
and retain the remaining duplication rather than forcing complete unification.
|
||||
|
||||
**Decision:** the recommended internal sharing boundary is accepted and recorded
|
||||
in the catalog. Execution and completion retain their separate policies; exact
|
||||
private signatures remain implementation choices. No refactor is implemented.
|
||||
|
||||
## F14–F15 — Joint source-prefix design
|
||||
|
||||
**Question:** how can discovery preparation and help lookup reuse one source
|
||||
structure without parallel indexes or changed routing semantics?
|
||||
|
||||
**Evidence:** [catalog F14](catalog.md#f14--p3-discovery-context-preparation-repeatedly-rebuilds-shared-prefixes)
|
||||
and [F15](catalog.md#f15--p3-alias-help-scans-the-entire-source-catalog-for-each-intermediate-route).
|
||||
|
||||
**Options considered:** traversal and descendant lookup through the existing
|
||||
alias source structure, or one shared private index if the existing structure
|
||||
cannot serve both operations simply.
|
||||
|
||||
**Analysis:** the existing [alias resolver](../../../internal/alias/resolver.go)
|
||||
already builds a source-path tree. Each node represents one distinct prefix,
|
||||
has child links, and identifies whether a mapping ends there. Shared source
|
||||
prefixes therefore already have a single structural representation.
|
||||
|
||||
F14's `discoverFlagContexts` in [flag_aliases.go](../../../flag_aliases.go)
|
||||
instead walks each declaration independently, repeatedly copying and classifying
|
||||
common prefixes before deduplicating accepted results. F15's `routeHelpAliases`
|
||||
in [help_view.go](../../../help_view.go) scans the full alias catalog at every
|
||||
intermediate rewrite path, even when that prefix has no descendants.
|
||||
|
||||
For example, sources `legacy team show` and `legacy team check` share `legacy`
|
||||
and `legacy team`. Setup can examine each shared prefix once. Help at
|
||||
`legacy team` can find its descendants through that tree branch without scanning
|
||||
unrelated source paths.
|
||||
|
||||
**Recommendation:** add narrow internal read-only operations on the existing
|
||||
resolver tree for these two needs:
|
||||
|
||||
1. Enumerate each distinct nonempty proper source prefix once for F14. Apply
|
||||
the existing discovery classification once per candidate, retaining exclusions
|
||||
for canonical paths, exact alias sources, and valid canonical operand space.
|
||||
2. Find strictly descendant alias sources beneath a supplied prefix for F15.
|
||||
Continue visiting the existing intermediate resolution paths. Resolve each
|
||||
composed entered path through the authoritative resolver, then retain current
|
||||
canonical-target checks, suffix deduplication, and output sorting.
|
||||
|
||||
Keep trie nodes private and preserve captured path ownership. Reuse the tree's
|
||||
existing routing-snapshot lifetime rather than creating a parallel index or a
|
||||
cross-invocation result cache. Root help can continue using the complete catalog
|
||||
when it actually needs all entries. No public API or routing grammar change is
|
||||
needed; internal method names and return shapes remain implementation choices.
|
||||
|
||||
This removes the identified repeated scans and preparation, without claiming
|
||||
that all routing or installation costs become linear. Test the preserved
|
||||
discovery/help behavior, including shared prefixes, chains, longer overrides,
|
||||
and updated registration where relevant. As already decided, performance testing
|
||||
is not a prerequisite for remediation.
|
||||
|
||||
**Decision:** reuse of the existing resolver tree through the two internal
|
||||
operations above is accepted and recorded in the catalog. Preserve existing
|
||||
discovery and routing semantics; no parallel index, new cache lifecycle, public
|
||||
API change, or performance-testing prerequisite is selected. Implementation
|
||||
remains pending.
|
||||
|
||||
## F17–F20 — Proportionate private extractions
|
||||
|
||||
**Question:** which proposed extractions make responsibilities clearer without
|
||||
adding records, phases, or configurable mechanisms merely to shorten functions?
|
||||
|
||||
**Evidence:** catalog entries [F17](catalog.md#f17--p3-configuration-scope-capture-combines-several-compilation-phases),
|
||||
[F18](catalog.md#f18--p3-configuration-preflight-interleaves-shared-and-operation-specific-rules),
|
||||
[F19](catalog.md#f19--p3-typed-input-preparation-mixes-conversion-provenance-and-replay-assembly),
|
||||
and [F20](catalog.md#f20--p3-reporter-message-paths-duplicate-detail-rendering-bookkeeping).
|
||||
|
||||
**Options considered:** cohesive functions over existing private types, or
|
||||
retaining related code together where extraction only adds indirection.
|
||||
|
||||
These are maintenance improvements, not newly established incorrect CLI
|
||||
outcomes. Recommendations below use existing private types and ordinary
|
||||
functions. No public API, generic pipeline, or additional validation policy is
|
||||
needed. Keep related work together; a lower function-length or complexity score
|
||||
is not itself a completion criterion.
|
||||
|
||||
### F17 — Separate configuration suite setup responsibilities
|
||||
|
||||
**Analysis:** `captureSuite` in [family.go](../../../integrations/ckconfig/family.go)
|
||||
captures scope and selector declarations, validates provider wiring, attaches
|
||||
selectors to scopes, determines command availability, and builds discovery
|
||||
metadata. Maintaining one of those responsibilities requires understanding
|
||||
several partially populated collections.
|
||||
|
||||
**Recommendation:** extract cohesive scope capture/validation and selector
|
||||
attachment responsibilities, then make operation bindings and discovery
|
||||
inventory construction explicit. Keep related loops together and use the
|
||||
existing prepared-suite types. Retain declaration ownership, accumulated
|
||||
errors and their order, binding identity, and metadata-only construction.
|
||||
Do not load configuration or invoke application factories during this setup.
|
||||
|
||||
The benefit is making it clear where a new scope or selector rule belongs.
|
||||
Exact helper boundaries can follow the implementation; do not create a helper
|
||||
for each loop or new intermediate records merely to divide the function.
|
||||
|
||||
### F18 — Separate ordinary configuration preparation from comparison
|
||||
|
||||
**Analysis:** `prepareConfigOperation` in
|
||||
[input.go](../../../integrations/ckconfig/input.go) handles command-specific flag
|
||||
rules, scope/capability checks, path parsing, and both ordinary and two-sided
|
||||
input preparation. The command builder also mixes several operation-specific
|
||||
flag groups into its declarations.
|
||||
|
||||
**Recommendation:** isolate operation-specific option checks, shared scope and
|
||||
capability selection, and comparison input preparation with small private
|
||||
functions. Keep an explicit orchestration function showing the actual order;
|
||||
do not replace its operation branches with a rule registry. Extract flag groups
|
||||
only where that makes their purpose clearer.
|
||||
|
||||
Preserve bare explain's help path, aggregate capability checking before input
|
||||
construction, independent left/right comparison selections, and completion of
|
||||
input checks before shared state creation and acquisition. This makes it easier
|
||||
to change one command without reasoning through every other command's submode.
|
||||
|
||||
### F19 — Keep input-ordering machinery together
|
||||
|
||||
**Analysis:** `prepareInputWithOccurrences` in [binding.go](../../../binding.go)
|
||||
combines value conversion, error attribution, occurrence-position assignment,
|
||||
required checks, operand allocation, and assembly of later input setters.
|
||||
Captured binding replay relies on the same occurrence identities.
|
||||
|
||||
**Recommendation:** start with small extractions for conversion-error
|
||||
attribution and operand preparation. Keep flag preparation, occurrence identity,
|
||||
and order reconstruction together as one coherent block; that block may remain
|
||||
in the orchestration function if moving it requires excessive plumbing.
|
||||
|
||||
For example, a conversion error should still identify the spelling the operator
|
||||
actually used, and repeated/interleaved inputs should still apply in their
|
||||
selected order. Preserve supplied-state distinctions, original error identity,
|
||||
and conversion before application input factories run. Retain the existing
|
||||
dense reconstruction and captured-replay merge algorithms. This is the most
|
||||
conservative extraction recommendation of the four; introducing new state
|
||||
containers or a replay framework would defeat its purpose.
|
||||
|
||||
### F20 — Share detail-rendering error bookkeeping
|
||||
|
||||
**Analysis:** `renderTerminalLocked` and `submitMessageLocked` in
|
||||
[reporter.go](../../../reporter.go) both invoke detail callbacks through managed
|
||||
writers, distinguish rendering failures from destination write failures, and
|
||||
retain the first error of each kind. Their headings and visibility rules differ.
|
||||
|
||||
**Recommendation:** use one private detail-rendering helper under the existing
|
||||
reporting lock, returning rendering and writing errors separately. Callers keep
|
||||
headings, visibility/discard selection, destination delivery observations, and
|
||||
terminal timing. Preserve callback order, hidden-detail execution, sticky writer
|
||||
errors, and callback non-reentrancy; add no locks or interchangeable renderers.
|
||||
|
||||
This is the clearest duplication removal of the four. Keep it separately
|
||||
reviewable from F03's deferred-acceptance fix and F07's writer-reference release;
|
||||
it does not itself repair either defect.
|
||||
|
||||
**Recommendation on sequencing:** correctness fixes remain first. Among these
|
||||
maintenance changes, favor F20, then F18 and F17, with F19 kept small. Evaluate
|
||||
each resulting diff by whether responsibilities are easier to follow and
|
||||
existing behavioral coverage still protects the selected contracts. Tests should
|
||||
not freeze new helper boundaries or incidental private call order.
|
||||
|
||||
**Decision:** all four bounded extraction recommendations and the sequencing
|
||||
guidance above are accepted and recorded in the catalog. F19 remains deliberately
|
||||
conservative. No refactoring has been implemented.
|
||||
|
||||
## Readiness for implementation planning
|
||||
|
||||
The discussion sequence is complete. No unresolved product or architectural
|
||||
choice identified in this review blocks preparation of the implementation plan.
|
||||
The catalog's 27 findings remain open until their remedies are implemented and
|
||||
validated.
|
||||
|
||||
The plan can assign work units, dependencies, and proportionate verification.
|
||||
Routine details such as private helper signatures, the public diagnostic
|
||||
helper's final name, and the completion fixture's replacement flag are ordinary
|
||||
implementation choices within the accepted scope. Preserve the recorded
|
||||
boundaries; revisit a decision only if concrete implementation evidence exposes
|
||||
a conflict or a materially different tradeoff.
|
||||
|
||||
Keep F14–F15 together, repair F06 before validating F16, and coordinate the
|
||||
reporter work in F03, F07, F20, and the relevant parts of F24 without obscuring
|
||||
their separate purposes. Validation should distinguish the aligned workspace
|
||||
from independent modules. No implementation plan or production changes are
|
||||
included in this analysis update.
|
||||
@@ -1,6 +1,7 @@
|
||||
# Future work
|
||||
|
||||
Status: pending ckprompt publication and future capability proposals.
|
||||
Status: pending release/dependency alignment, optional shell validation, and
|
||||
future capability proposals.
|
||||
No delivery dates are selected here.
|
||||
|
||||
## Purpose and authority
|
||||
@@ -8,6 +9,9 @@ No delivery dates are selected here.
|
||||
Earlier completed roadmaps, implementation plans, and review findings have been retired.
|
||||
This document preserves unfinished work and unselected ideas under the
|
||||
[release policy](../policy/release.md#release-documentation-and-preparation).
|
||||
The completed audits, consolidated remediation, implementation plan, and
|
||||
declaration-path follow-up remain available in the
|
||||
[audit remediation retirement snapshot](https://forge.maximumdirect.net/base-layer/commandkit/src/commit/aac0e11ae67c78a35745ed9a25688aa51c15e793/docs/roadmap).
|
||||
The completed PromptKit v0.18.0 adoption roadmap and implementation plan remain
|
||||
available in the
|
||||
[PromptKit retirement snapshot](https://forge.maximumdirect.net/base-layer/commandkit/src/commit/540e2359d66d604a563df0be1c9fc961588f5b9c/docs/roadmap).
|
||||
@@ -44,6 +48,14 @@ under the [release procedure](../release.md).
|
||||
The published ckprompt v0.2.0 retains its earlier dependencies; see the
|
||||
[consumer guide](../consumers/guide.md#module-selection) for module selection.
|
||||
|
||||
### Optional shell completion validation
|
||||
|
||||
Zsh and Fish interactive completion checks remain unverified because those
|
||||
executables were unavailable during remediation. Script generation for Bash,
|
||||
Zsh and Fish and the Bash interactive checks passed. Run the existing opt-in
|
||||
backend and consumer checks when those prerequisites are available, following
|
||||
the [developer guide](../development.md#shell-completion-validation).
|
||||
|
||||
## Future capability targets
|
||||
|
||||
### Comparison extensions
|
||||
|
||||
@@ -1,790 +0,0 @@
|
||||
# Audit remediation implementation plan
|
||||
|
||||
Status: source sprint complete; Stages 1–23 complete. Release and dependency
|
||||
alignment remain separate work.
|
||||
|
||||
Baseline: `d492c7747175c01a53261206c2b4356e53453621`. Before this sprint,
|
||||
intervening changes recorded decisions and retired the completed older
|
||||
remediation. The [roadmap](audit-remediation.md) owns the target and scope,
|
||||
the [catalog](audit/catalog.md) owns accepted decisions and finding status, and
|
||||
its original audit links own defect evidence. The
|
||||
[analysis](audit/remediation-analysis.md) explains the selected designs.
|
||||
|
||||
## Instructions for the implementing agent
|
||||
|
||||
Once the user approves this sprint, execute Stage 1 through Stage 23 in order
|
||||
under that approval. Stages are bounded work units within one continuous run,
|
||||
not separate prompts, approval gates, or handoffs. Proceed to the next stage
|
||||
without asking for confirmation after a stage, test run, or local commit.
|
||||
Keep progress updates concise and report the assembled result after Stage 23.
|
||||
Stages 11–12 split the cross-module diagnostic change for implementation clarity.
|
||||
|
||||
Intermediate stages need not compile independently. If a check depends on a later
|
||||
stage, record it as pending, continue in order, and run it when its prerequisites
|
||||
exist. Stage completion criteria describe what must eventually be evidenced;
|
||||
they do not require stopping for dependent verification before later work can
|
||||
proceed. Do not add temporary compatibility machinery to make an intermediate
|
||||
stage green. The final source must be coherent and validated through the aligned
|
||||
workspace.
|
||||
|
||||
Resolve ordinary implementation choices and fix failures within the approved
|
||||
scope without another design round. A real contradiction in the selected target
|
||||
or an unavoidable external blocker may require input; first investigate in-scope
|
||||
remedies and complete remaining independent work. Do not silently change the
|
||||
target, waive a relevant failing check, or claim blocked work is complete.
|
||||
|
||||
Read [development guidance](../development.md) and the applicable
|
||||
[agent](../policy/agents.md), [architecture](../policy/architecture.md),
|
||||
[testing](../policy/testing.md), and [documentation](../policy/documentation.md)
|
||||
policies. Preserve core/backend isolation and application/upstream ownership.
|
||||
This sprint selects no upstream upgrades, toolchain requirement changes, new
|
||||
commands, report formats, arbitrary input limits, or concurrent App dispatch.
|
||||
Keep the existing safe evidence, error identity, presence, and independent
|
||||
comparison-input semantics. The selected public changes need no consumer
|
||||
migration scaffolding. Private helper names and signatures may follow idiomatic
|
||||
implementation; the explicit responsibilities below are the decisions to retain.
|
||||
|
||||
Relevant accepted boundaries are [ADR 0002](../adr/0002-public-facade-and-cli-backend.md),
|
||||
[ADR 0004](../adr/0004-optional-integration-modules.md), and
|
||||
[ADR 0006](../adr/0006-invocation-reporting.md). Use repository graph discovery
|
||||
before textual code searches as directed by AGENTS.md.
|
||||
|
||||
For every stage, inspect its source and the catalog's linked originals before
|
||||
editing. Reuse existing tests; extend them only for meaningful missing coverage.
|
||||
Consequential correctness fixes need focused regressions that demonstrate the
|
||||
original defect. Refactor tests only where they depend on removed mechanics;
|
||||
do not rewrite behavioral expectations to accommodate accidental changes. Test
|
||||
CommandKit adaptation and coordination, trusting upstream domain contracts.
|
||||
Update the owning GoDoc, CLI, consumer or internal documentation with each change.
|
||||
|
||||
Record stage completion and actual verification here. Add completion evidence to
|
||||
the corresponding catalog finding only after its entire remedy is implemented
|
||||
and verified. F21 spans stages 11–12; F24 spans stages 3, 13, and 20. Preserve the
|
||||
original audits and accepted decisions. Commit completed nonempty work units with
|
||||
plain English messages under repository policy; a commit is not a user-review
|
||||
checkpoint. Related stages may form one validated commit when needed. Do not
|
||||
publish or push.
|
||||
|
||||
## Validation conventions
|
||||
|
||||
Use the cached Go 1.27.0 toolchain for cross-module checks. Preserve ignored
|
||||
`go.work` and `go.work.sum`. In the aligned three-module workspace, run focused
|
||||
tests for the affected package during the stages; select existing test names from
|
||||
the named test files plus new defect regressions. Use `GOTOOLCHAIN=local`,
|
||||
`GOPROXY=off`, and `-mod=readonly` for offline checks with cached dependencies.
|
||||
If the default cache is not writable, use a temporary `GOCACHE` under `/tmp`.
|
||||
Format changed Go files with gofmt. No benchmarks, timing thresholds, complexity
|
||||
score targets, or new validation framework are needed.
|
||||
|
||||
Focused workspace commands run from the repository root. Select the stage's
|
||||
relevant tests with `-run` when existing coverage permits; the package targets
|
||||
are `.` for core, `./internal/cobra` for the adapter, `./internal/alias` for the
|
||||
resolver, `./integrations/ckconfig` and `./integrations/ckprompt` for integrations.
|
||||
For example, reporter checks use
|
||||
`GOTOOLCHAIN=local GOPROXY=off go test -mod=readonly -run 'TestReporter|TestHiddenDeferred' .`.
|
||||
Example package targets follow their paths under the corresponding module.
|
||||
|
||||
Stage 23 owns the final broad checks. Root `./...` excludes nested modules.
|
||||
Workspace results cover current combined sources; independent checks with
|
||||
`GOWORK=off` cover each manifest's selected dependencies. After stage 12,
|
||||
ckprompt's new helper call cannot compile against its selected ckconfig v0.4.0.
|
||||
Do not change manifests to invented versions, fabricate sums, or commit local
|
||||
replacements. Validate combined source in the workspace, and record that released
|
||||
module alignment remains pending. Temporary external-consumer modules or alternate
|
||||
module files may use explicit local replacements as additional boundary evidence;
|
||||
label them as local-source checks. Once appropriate releases exist, dependency
|
||||
alignment and independent validation follow the [release procedure](../release.md)
|
||||
as separate work. Publication is neither authorized nor a prerequisite here.
|
||||
Do not wait for those releases or request publication to advance or finish this
|
||||
source sprint. Record the known independent-module limitation and continue.
|
||||
|
||||
Unavailable optional shell prerequisites or race-detector support are evidence
|
||||
limitations, not stage gates. Run the available checks, record what could not
|
||||
run, and continue. Deterministic regression checks remain required; failures in
|
||||
supported checks require investigation and in-scope correction. Revisit checks
|
||||
deferred for internal dependencies before the final report. A genuine inability
|
||||
to run required final verification must be reported as incomplete verification,
|
||||
not converted into a passed check or a request for per-stage approval.
|
||||
|
||||
## Stage 1 — Retain aggregate smoke cancellation
|
||||
|
||||
**Complete.** The progress-delivery cancellation regression reproduced both
|
||||
the false success and missing joined cause before the fix. The focused aligned
|
||||
workspace checks passed: `go test -mod=readonly -run 'TestSmokeAggregate|TestSmokeTestCancellation' ./integrations/ckprompt`
|
||||
with Go 1.27.0, `GOTOOLCHAIN=local`, and `GOPROXY=off`. Tests cover partial
|
||||
results with and without a provider error, retained diagnostics and explanation,
|
||||
omitted callbacks, and cancellation after the final check.
|
||||
|
||||
Findings: F01. Primary source: `integrations/ckprompt/smoke_command.go`.
|
||||
|
||||
1. In runSmokeAggregate, retain cancellation observed at the later checkpoint
|
||||
when it omits remaining smoke kinds. Join it with prior operation causes;
|
||||
avoid redundantly adding cancellation already retained by an operation.
|
||||
2. Preserve attempted-kind order, acquired reports, diagnostics, prior errors,
|
||||
and explanations. Do not invent reports for unattempted kinds. Keep the
|
||||
existing acquisition/cancellation handling and final reporting path.
|
||||
3. Do not add a blanket late-cancellation failure after all requested work is
|
||||
complete. Distinguish the final completed kind from a stop that omits work.
|
||||
4. Extend smoke aggregate/outcome tests using a progress writer that cancels
|
||||
without reentering the Reporter. Assert a failed partial outcome and
|
||||
errors.Is for cancellation, both with and without an existing provider error.
|
||||
Check omitted callbacks, preserved evidence, and ordinary successful all-kind
|
||||
execution. Keep the ordinary suites offline.
|
||||
|
||||
Complete when the progress-delivery cancellation window cannot produce a
|
||||
successful incomplete aggregate or lose cancellation beside another cause.
|
||||
|
||||
## Stage 2 — Isolate parsed provider-parameter JSON
|
||||
|
||||
**Complete.** Both captured binding replay and aggregate request seed creation
|
||||
copy nested JSON objects and arrays. The two new boundary regressions failed
|
||||
against the original implementation; focused request-flag and aggregate tests
|
||||
passed in the aligned workspace with Go 1.27.0, `GOTOOLCHAIN=local`,
|
||||
`GOPROXY=off`, and `-mod=readonly`. Nil, empty, integer and decimal values are
|
||||
covered.
|
||||
|
||||
Findings: F02. Sources: ckprompt `request_flags.go` and `smoke_command.go`.
|
||||
|
||||
1. Add one private recursive copier for the known CLI JSON object/array
|
||||
representation. Copy maps and arrays; preserve scalar numeric types and
|
||||
nil versus explicitly empty collections. Do not JSON-marshal to copy values.
|
||||
2. Use it when applying NewExecutionFlags.ExtraParams to a destination and when
|
||||
smokeRequestForKind copies each aggregate execution seed. Retain existing
|
||||
scalar-pointer copies and authoritative application builder replacements.
|
||||
3. Keep this out of generic core ValueFlag/captured-binding machinery. Do not
|
||||
promise cloning of arbitrary application object graphs or repair sharing
|
||||
introduced by application builders.
|
||||
4. Extend request flag and smoke tests: mutate a nested map/array in one request
|
||||
and inspect the next kind's original values; separately apply captured flags
|
||||
to independent destinations using ApplyTo and ApplySuppliedTo, mutate one,
|
||||
and confirm the other retains supplied values. Cover empty/nil and the current
|
||||
CLI numeric representation without retesting upstream preparation.
|
||||
|
||||
Complete when both binding replay and per-kind seed boundaries are isolated.
|
||||
|
||||
## Stage 3 — Close deferred acceptance atomically
|
||||
|
||||
**Complete.** Finalization closes deferred acceptance while detaching its
|
||||
batch; completion and terminal reporting remain available. The channel-controlled
|
||||
regression failed against the original code and now passes alongside focused
|
||||
reporter and hidden-deferred tests, both normally and with `-race -count=1`, in
|
||||
the aligned Go 1.27.0 workspace with offline, read-only module resolution. The
|
||||
old reporter test wrappers were replaced with real selection and finalization.
|
||||
|
||||
Findings: F03 and the reporter-lifecycle portion of F24. Source: `reporter.go`.
|
||||
|
||||
1. Add a private deferred-acceptance/finalization state distinct from the final
|
||||
closed state. Under the existing mutex, end acceptance and detach the batch
|
||||
in one transition at the start of finalize.
|
||||
2. Make DeferredReporter.enqueue return ErrReporterClosed after that transition,
|
||||
including hidden or empty submissions. Retain pre-cutoff visibility and detail
|
||||
handling. Keep finalizer-owned batch delivery, completion reporting, and
|
||||
terminal explanations usable until final closure.
|
||||
3. Do not repeatedly drain arrivals or wait for application producers. Do not
|
||||
set the general closed flag before internal presentation finishes. Ordinary
|
||||
concurrent Reporter operations retain their existing lifecycle.
|
||||
4. Move tests using the synthetic close/setVisibility helpers to actual
|
||||
finalize/applySelection entry points, preserving useful assertions. Remove
|
||||
those obsolete production helpers once their callers have moved. Keep any
|
||||
concise setup helpers in `_test.go`, delegating to the real paths.
|
||||
5. Extend reporter finalization tests with channels controlling a destination's
|
||||
flush boundary. Submit a deferred message while the accepted batch is being
|
||||
presented; assert ErrReporterClosed. Check pre-cutoff accepted delivery,
|
||||
completion/terminal ordering and visibility, plus render/write failure
|
||||
evidence. Avoid sleeps and callback reentry. Run the focused reporter tests
|
||||
with `-race -count=1` where supported, supplementing the required deterministic
|
||||
acceptance/finalization regression. Record unavailable race support and proceed.
|
||||
6. Document the cutoff in Reporter/AtEnd GoDoc and relevant reporting guidance.
|
||||
|
||||
Complete F03 when acceptance cannot succeed into an abandoned batch. Keep F24
|
||||
open for the remaining parsing and binding wrappers.
|
||||
|
||||
## Stage 4 — Reject missing prepared-request results
|
||||
|
||||
**Complete.** Missing prepared requests produce an operational failure while
|
||||
retaining diagnostics and explanations. The public-command regression reproduced
|
||||
the original successful null preview; focused preparation tests now pass in the
|
||||
offline aligned workspace with Go 1.27.0 and read-only module resolution, covering
|
||||
valid results, provider errors, and cancellation as well.
|
||||
|
||||
Findings: F04. Source: ckprompt `preparation.go`.
|
||||
|
||||
1. After the existing provider-error and cancellation handling in
|
||||
runRequestPreparation, reject a nil acquired.Value with an operational failure.
|
||||
Use the neighboring missing-result handling style; retain supplied diagnostic
|
||||
and explanation evidence and existing error identities.
|
||||
2. Keep core null-report support and valid preview/report-plus-error handling
|
||||
unchanged. Do not add new checks to upstream prepared-request contents.
|
||||
3. Extend preparation tests through the public command: nil result/nil error,
|
||||
valid result, and existing provider error/cancellation paths. Assert failure
|
||||
classification and absence of a successful null preview, with retained
|
||||
diagnostic/explanation evidence. Reuse core's existing null-report coverage.
|
||||
|
||||
Complete when empty successful acquisition cannot masquerade as prepared preview.
|
||||
|
||||
## Stage 5 — Separate show preparation cancellation from rendering failures
|
||||
|
||||
**Complete.** Buffered show preparation carries cancellation outside managed
|
||||
rendering without emitting the incomplete report. The two-scope regression
|
||||
failed against the original behavior and now passes, including simultaneous
|
||||
native rendering failure; focused show, buffered cancellation and write-failure
|
||||
tests passed in the offline aligned Go 1.27.0 workspace with `-mod=readonly`.
|
||||
|
||||
Findings: F05. Source: ckconfig `show.go`; reference `operations.go`'s
|
||||
renderPreparedConfigReport checkpoint pattern.
|
||||
|
||||
1. Keep buffered effective-document preparation under managed rendering so
|
||||
genuine native-writer/DecodeYAMLReport failures remain presentation evidence.
|
||||
2. Capture cancellation observed by preparation checkpoints separately; return
|
||||
nil from the renderer for a cancellation-only stop, then return configFailure
|
||||
with that cancellation outside it. Do not deliver an incomplete report.
|
||||
3. Preserve genuine native-writing/decoding errors even when cancellation is
|
||||
also visible. Do not discard cancellation identity or alter the distinction
|
||||
between interrupted preparation and cancellation after delivery starts.
|
||||
4. Extend `report_cancellation_test.go`: two acquired scopes, first writer emits
|
||||
valid YAML and cancels while returning nil, second writer remains uncalled.
|
||||
Assert no output, cancellation Cause, and no manufactured RenderError or
|
||||
WriteError. Reuse real render/write failure and focused-show coverage.
|
||||
|
||||
Complete when cancellation alone is not a presentation error and actual
|
||||
presentation failures remain separately observable.
|
||||
|
||||
## Stage 6 — Repair the maintained consumers
|
||||
|
||||
**Complete.** The fixture generates Bash, Zsh and Fish scripts; the available
|
||||
interactive Bash backend and consumer checks passed. The full opt-in commands
|
||||
reported failures for Zsh and Fish because those shells are not installed;
|
||||
their interactive behavior remains unverified. The core example's bare and
|
||||
explicit `demo` invocations succeeded, and an invalid scenario returned usage
|
||||
status 2. Checks used Go 1.27.0 with offline, read-only module resolution.
|
||||
|
||||
Findings: F06 and F08. Sources: `testdata/completion/main.go`,
|
||||
`completion_shell_test.go`, and `examples/core/main.go`.
|
||||
|
||||
1. Rename the fixture's custom `format` choice flag to `choice`; retain its
|
||||
text/yaml choices. It tests application-owned completion, so keep it separate
|
||||
from core's actual format flag. Update the root consumer harness inputs,
|
||||
expected argv, prefix case (`--cho`), and terminal invocation consistently.
|
||||
Do not rename unrelated backend fixtures or core's reporting flag.
|
||||
2. Generate scripts from this fixture to prove App construction succeeds.
|
||||
Run the opt-in consumer and backend shell checks available locally using the
|
||||
developer guide's shell-completion commands. Record missing prerequisites or
|
||||
shells as unavailable; do not count skips as successful interactive checks.
|
||||
3. Guard the example scenario assignment with Supplied.Set, retaining the factory
|
||||
default and explicitly supplied empty-value validation. Exercise bare
|
||||
`go run ./examples/core report`, explicit demo, and an invalid scenario.
|
||||
Reuse example coverage; no new default suite inventory is required for this
|
||||
small example correction.
|
||||
|
||||
Complete when the example default works and the completion fixture reaches
|
||||
script/shell validation. Stage 14 relies on this repaired fixture.
|
||||
|
||||
## Stage 7 — Release finalized reporter destination references
|
||||
|
||||
**Complete.** Finalization clears both borrowed writer references after
|
||||
capturing presentation, without closing them. Tests cover success, operational
|
||||
failure, cancellation, late handles and writer ownership; focused reporter tests
|
||||
pass in the offline aligned Go 1.27.0 workspace with `-mod=readonly`.
|
||||
|
||||
Findings: F07. Source: `reporter.go`; builds on stage 3.
|
||||
|
||||
1. After deferred/completion/terminal presentation and capture of observations,
|
||||
mark state closed and clear both direct destination writer references while
|
||||
holding the existing mutex. Retain closed state and errors/observations.
|
||||
2. Do not Close/Flush either borrowed writer. Keep ErrReporterClosed behavior for
|
||||
retained Reporter, Output, and DeferredReporter handles. Return the same
|
||||
error identities; error objects may legitimately retain their own data.
|
||||
3. Extend actual-finalization tests for success, operational failure, and
|
||||
cancellation. Inspect that unnecessary direct references are released and
|
||||
late operations reject without touching writers. Reuse caller-writer
|
||||
ownership and presentation-evidence checks. Avoid GC-timing assertions in
|
||||
ordinary tests; a temporary reachability probe is supplementary only.
|
||||
|
||||
Complete when finalized handles no longer keep destinations through those fields,
|
||||
without promising immediate garbage collection or taking ownership of closers.
|
||||
|
||||
## Stage 8 — Share reporter detail-rendering bookkeeping
|
||||
|
||||
**Complete.** Terminal and ordinary reporting share detail-rendering and error
|
||||
classification under the existing lock. Focused reporter and hidden-deferred
|
||||
tests passed in the offline aligned Go 1.27.0 workspace with `-mod=readonly`.
|
||||
|
||||
Findings: F20. Source: `reporter.go`; follows lifecycle fixes in stages 3 and 7.
|
||||
|
||||
1. Extract a private helper running under the existing reporting lock to invoke
|
||||
detail callbacks through the managed writer and return render/write errors
|
||||
separately, retaining first errors and their joined evidence as currently done.
|
||||
2. Use it from renderTerminalLocked and submitMessageLocked. Keep headings,
|
||||
visibility/discard selection, leading-message failures, destination delivery
|
||||
observations, and terminal timing in the callers.
|
||||
3. Preserve nil details, order/lifetime, hidden callbacks against io.Discard,
|
||||
sticky destination failures and callback non-reentrancy. Add no locks or
|
||||
configurable rendering pipeline.
|
||||
4. Run focused reporter tests for hidden details, terminal/deferred ordering,
|
||||
joined renderer evidence, independent destination failures and closed handles,
|
||||
including stage 3's regression. Extend only uncovered behavioral cases.
|
||||
|
||||
Complete when both paths use one implementation of the delicate detail policy.
|
||||
|
||||
## Stage 9 — Clarify configuration evidence and narrow explanation providers
|
||||
|
||||
**Complete.** Provider and YAML conversion GoDoc now describe the current
|
||||
handoff; `ExplanationReader` only acquires evidence. Focused core YAML conversion
|
||||
and ckconfig show, explanation, adapter and delivery tests passed in the offline
|
||||
aligned Go 1.27.0 workspace with `-mod=readonly`, including a custom Explain-only
|
||||
provider for path show and all explanation formats.
|
||||
|
||||
Findings: F09 and F11. Sources: ckconfig `contracts.go`, `show.go`, `explain.go`,
|
||||
`contracts_adapter_test.go`; core `report_value.go` conversion GoDoc.
|
||||
|
||||
1. Document EffectiveWriter's single supported safe-YAML-document handoff and
|
||||
link DecodeYAMLReport as the conversion contract owner. Clarify that formatting,
|
||||
comments and framing are not passed through; document unsupported tags,
|
||||
non-string mapping keys and multiple documents at the owning helper as needed.
|
||||
Correct View's description to reflect acquired evidence used to build reports.
|
||||
2. Reduce ExplanationReader to Explain(configkit.Path) (configkit.Explanation,
|
||||
bool). Update its GoDoc and remove writer-only provider stubs/assertions and
|
||||
direct interface-writer test calls across the repository.
|
||||
3. Retain native objects' own writer methods. Do not add compatibility adapters,
|
||||
another writer interface, or restore provider-owned report rendering.
|
||||
4. Reuse acquired-explanation cross-format and delivery-error tests. Ensure a
|
||||
custom provider implementing only Explain works for explanation/path show.
|
||||
Reuse YAML conversion tests and review GoDoc against current conversion rules;
|
||||
do not expand accepted YAML types to accommodate old wording.
|
||||
5. Update affected consumer integration guidance for the selected provider API.
|
||||
|
||||
Complete each finding independently: accurate effective-output documentation for
|
||||
F09, and acquisition-only provider wiring with preserved rendering for F11.
|
||||
|
||||
## Stage 10 — Document the selected flag declaration contract
|
||||
|
||||
**Complete.** FlagMeta describes the existing Unicode long-name grammar,
|
||||
reserved names and short spellings; rename and alias guidance links to it.
|
||||
Reviewed against `validateBindings`, `validFlagName`, and the accepted catalog
|
||||
decision. No runtime validation was changed.
|
||||
|
||||
Findings: F10. Source: `command.go` FlagMeta and related rename/alias GoDoc.
|
||||
|
||||
1. Define long names once with FlagMeta: nonempty Unicode lowercase-letter or
|
||||
digit segments separated by single hyphens. Numeric-only and digit-leading
|
||||
names remain valid. Describe the implemented reserved names help, quiet,
|
||||
verbose, debug, progress and format.
|
||||
2. Describe optional one-byte short spellings, excluding `-` and reserved `h`.
|
||||
Do not apply the long-name lowercase rule to short spellings.
|
||||
3. Link RenameFlag and flag-alias helpers to that declaration contract. Retain
|
||||
CLI operator syntax in its existing owner and installation-time error timing.
|
||||
4. Compare prose with validateBindings/validFlagName and the accepted catalog
|
||||
decision. Make no runtime validator changes, normalization, ASCII-only rule,
|
||||
or new tests solely to restate documentation.
|
||||
|
||||
Complete when exported guidance accurately states the already selected rules.
|
||||
|
||||
## Stage 11 — Export the shared ConfigKit diagnostic adapter
|
||||
|
||||
**Complete.** `ckconfig.AdaptDiagnostics` captures the existing diagnostic
|
||||
presentation for ordinary and comparison callers. Focused ConfigKit adapter and
|
||||
comparison tests passed offline in the aligned Go 1.27.0 workspace with
|
||||
`-mod=readonly`, including public helper snapshot and writer-failure coverage.
|
||||
F21 remains open until ckprompt adopts it in Stage 12.
|
||||
|
||||
Findings: F21, ckconfig portion. Source: ckconfig `configkit_adapter.go`.
|
||||
|
||||
1. Expose the existing configKitDiagnosticsWithHeading responsibility as:
|
||||
|
||||
```go
|
||||
func AdaptDiagnostics(heading string, sources []configkit.SourceInfo,
|
||||
records []configkit.Diagnostic) ([]commandkit.Diagnostic, commandkit.Explanation)
|
||||
```
|
||||
|
||||
2. Route ckconfig's ordinary and comparison-side adaptation through it. Keep any
|
||||
small scope-to-heading constructor that serves a caller; remove the replaced
|
||||
duplicate implementation rather than leaving parallel helpers.
|
||||
3. Retain captured sources, cloned Primary/Related locations, grouping of error
|
||||
records into explanation details and other records into ordinary diagnostics,
|
||||
within-group order, source legends, native formatting and short-write handling.
|
||||
4. Document that it returns captured CLI presentation evidence; it does not print
|
||||
immediately, acquire configuration, determine outcomes, or schedule messages.
|
||||
5. Reuse native adapter tests, extending public helper coverage for mutations of
|
||||
input slices/locations after adaptation and deferred rendering failures where
|
||||
those protections are not already exercised. Assert CommandKit's snapshots
|
||||
and classifications, not ConfigKit's diagnostic formatting algorithm.
|
||||
|
||||
Keep F21 open until stage 12 removes the other integration's duplication.
|
||||
|
||||
## Stage 12 — Adopt shared diagnostics in ckprompt
|
||||
|
||||
**Complete.** ckprompt delegates native evidence adaptation to ckconfig while
|
||||
retaining its scope and side headings. Focused acquisition, comparison and
|
||||
diagnostic tests passed in the offline aligned Go 1.27.0 workspace. An independent
|
||||
`GOWORK=off` ckprompt check against its selected ckconfig v0.4.0 failed as expected
|
||||
because that release lacks `AdaptDiagnostics`; released dependency alignment
|
||||
remains separate work.
|
||||
|
||||
Findings: F21 completion. Source: ckprompt `adapters.go` and related tests.
|
||||
|
||||
1. Make promptKitDiagnostics call ckconfig.AdaptDiagnostics. Preserve its current
|
||||
heading: `Configuration scope ` plus strconv.Quote(scope), optional
|
||||
` (side)`, and the existing colon/newline. Supply that complete heading to
|
||||
the shared helper, whose writer consumes it as provided.
|
||||
2. Translate the returned diagnostics/explanation into the existing private
|
||||
acquisition record. Remove the duplicated snapshot/severity partition and
|
||||
writePromptKitDiagnostics implementation and now-unused imports.
|
||||
3. Keep operation-specific fallback explanations, causes, outcomes, scope/side
|
||||
labels, and both integrations' scheduling switches with their callers. No
|
||||
new core dependency or shared module is required.
|
||||
4. Run focused ckprompt acquisition/comparison tests in the aligned workspace,
|
||||
covering available/empty evidence, warning versus error adaptation, side
|
||||
labels, cause retention and deferred writer failure. Verify integration
|
||||
documentation describes the new helper only under ckconfig's CLI ownership.
|
||||
5. Record the independent ckprompt limitation stated in Validation conventions;
|
||||
do not conceal it with committed replacements or conditional fallbacks.
|
||||
|
||||
Complete F21 when both callers share one adapter and preserve their context.
|
||||
|
||||
## Stage 13 — Remove unused parser resource plumbing
|
||||
|
||||
**Complete.** Parser options carry only alias selection, backend output and
|
||||
error destinations explicitly discard, and core retains invocation resources.
|
||||
The test-only App.parse wrapper was removed. Focused routed parsing, help,
|
||||
context, backend parsing and session-isolation tests passed in the offline aligned
|
||||
Go 1.27.0 workspace with `-mod=readonly`. F24 remains open for binding wrappers.
|
||||
|
||||
Findings: F12 and parsing portion of F24. Sources: `execute.go`, `parser.go`,
|
||||
`internal/commandmeta/metadata.go`, and `internal/cobra/parse.go`.
|
||||
|
||||
1. Remove execution.streams and ParseOptions.Context/In/Out/Err, then remove their
|
||||
unused propagation/default normalization from the adapter. Retain consumed
|
||||
alias-selection options. Normalize invocation streams once with the reporter.
|
||||
2. Keep Invocation.Context and real reporter destinations unchanged. Set explicit
|
||||
internal discard output/error destinations on the backend and preserve local
|
||||
RenderHelp buffering; Cobra defaults must not write to process destinations.
|
||||
Preserve runnable markers, help metadata, fresh sessions and session reuse
|
||||
within one invocation. Remove only settings associated with unused resources.
|
||||
3. Replace App.parse's test-only path with current execution/session preparation
|
||||
in its test and remove the obsolete wrapper. Keep concise test setup in tests.
|
||||
4. Adapt session tests to the smaller options shape, retaining behavioral help,
|
||||
routing, parser-state isolation and context-forwarding coverage. Run core and
|
||||
adapter focused tests; confirm help is delivered through core reporting.
|
||||
5. Update focused internal documentation for the simplified parser boundary.
|
||||
|
||||
Keep F24 open until the binding wrappers are handled in stage 20.
|
||||
|
||||
## Stage 14 — Share internal flag-consumption facts
|
||||
|
||||
**Complete.** Parsing and completion share neutral consumption facts while
|
||||
retaining their separate failure and partial-input policies. Focused parser,
|
||||
alias, occurrence and completion tests and both interactive Bash checks passed
|
||||
in the offline aligned Go 1.27.0 workspace. Zsh and Fish remain unavailable as
|
||||
recorded in Stage 6.
|
||||
|
||||
Findings: F16. Sources: `internal/commandmeta/flags.go`,
|
||||
`internal/cobra/flags.go`, and `completion_flags.go`; stage 6 is prerequisite.
|
||||
|
||||
1. Extend the existing neutral helpers with a small consumption operation over
|
||||
lookup, token and short-cluster position. Return flag identity, attached value
|
||||
presence separately from its text, next-argument need, and cluster continuation.
|
||||
Use each caller's existing lookup; keep `--` classification/end handling clear.
|
||||
2. Migrate interpretFlagTokens, walkCompletionFlags and completionOptionTarget
|
||||
to these facts. Execution retains canonical normalization, entered spellings,
|
||||
occurrences, consumed failure prefixes and backend errors. Completion retains
|
||||
unfinished-word interpretation, pending values, suggestions and insertion text.
|
||||
3. Preserve boolean assignments, attached/next values (including option-looking
|
||||
values), explicit empties, unknown spellings and short-cluster boundaries.
|
||||
Keep caller-specific branches where sharing would require extensive modes.
|
||||
4. Reuse parser, alias/provenance and public completion tests; extend uncovered
|
||||
cases that compare shared grammar and intentionally different missing/partial
|
||||
input behavior. Run available repaired shell checks. Do not add alias discovery,
|
||||
public parser APIs, execution/completion modes, or a replacement backend.
|
||||
|
||||
Complete when common grammar mechanics have one owner and caller policies remain
|
||||
clear, with selected syntax and provenance preserved.
|
||||
|
||||
## Stage 15 — Memoize each context's flag-alias graph
|
||||
|
||||
**Complete.** Context-local traversal memoizes canonical and unavailable
|
||||
endpoints, with an active stack for cycle diagnostics. Source review confirms
|
||||
resolved suffixes are reused within each context. Focused alias, registration,
|
||||
cycle and discovery tests passed offline with Go 1.27.0 and `-mod=readonly`,
|
||||
including shared suffixes and unavailable destinations.
|
||||
|
||||
Findings: F13. Source: `flag_aliases.go`.
|
||||
|
||||
1. Replace independent suffix walks with context-local visitation and memoized
|
||||
terminal results, including unavailable endpoints. Resolve canonical spellings
|
||||
before following a mapping of that spelling. Cache token resolution without
|
||||
conflating a source's collision check with its target's resolution.
|
||||
2. Process the existing sorted sources, resolving each mapping's target before
|
||||
the active-source collision check. Keep scoped overrides, inactive globals,
|
||||
scoped missing-target failures and useful participating-cycle diagnostics.
|
||||
Maintain an active traversal stack to identify cycles; no hop cap is needed.
|
||||
3. Keep the cache invocation/snapshot preparation local to its effective context,
|
||||
revalidating on registration. Do not share results across different contexts.
|
||||
4. Run existing flag-alias chain, cycle, duplicate/scope and discovery tests;
|
||||
add representative shared-suffix/inactive-endpoint coverage only if missing.
|
||||
Review loops to confirm each target suffix is resolved once per context.
|
||||
Existing sorting remains; no benchmark or whole-installation bound is required.
|
||||
|
||||
Complete when repeated suffix traversal is removed without routing changes.
|
||||
|
||||
## Stage 16 — Reuse the resolver tree for prefix discovery and help
|
||||
|
||||
**Complete.** Discovery classifies each eligible trie prefix once (F14), and
|
||||
help visits only strict descendants for each resolution path (F15). Source
|
||||
review confirms both changes use the existing resolver tree; focused alias,
|
||||
discovery, help and resolver tests pass offline with Go 1.27.0 and
|
||||
`-mod=readonly`, including returned-path ownership and exact aliases with children.
|
||||
|
||||
Findings: F14 and F15, one coordinated change. Sources:
|
||||
`internal/alias/resolver.go`, `flag_aliases.go`, and `help_view.go`.
|
||||
|
||||
1. Add two narrow internal read-only tree operations: enumerate distinct nonempty
|
||||
proper source prefixes, and find mapping sources strictly below a supplied
|
||||
prefix. Reuse trie nodes and their captured paths; keep nodes private and
|
||||
return owned paths/mappings so consumers cannot mutate the resolver.
|
||||
2. For enumeration, visit each eligible tree node once rather than rebuilding
|
||||
each declaration's prefixes. A node may itself end an alias and have children;
|
||||
leave exact-alias exclusion to the existing classification. Apply canonical,
|
||||
exact-source and canonical-operand-space exclusions once per distinct prefix.
|
||||
Update discoverFlagContexts's consumed resolver shape accordingly.
|
||||
3. For descendant lookup, locate the supplied prefix branch and enumerate only
|
||||
descendants, excluding that node's mapping. Use it for every existing
|
||||
ResolutionPaths entry in routeHelpAliases. Keep root help's complete catalog
|
||||
when needed, composed-path authoritative resolution, canonical-target checks,
|
||||
longer overrides, suffix deduplication and sorted presentation.
|
||||
4. Reuse routing snapshot lifetime, without another index or result cache. Do
|
||||
not replace the resolver's selected termination proof or longest-prefix rules.
|
||||
5. Check shared deep prefixes, alias chains with no descendant output, exact
|
||||
aliases with children, longer overrides, canonical operand exclusions and
|
||||
post-registration refresh using existing alias/help/discovery coverage plus
|
||||
missing focused cases. Inspect traversal to establish reduced repeated work;
|
||||
assert results rather than timing or private call counts.
|
||||
|
||||
Record evidence separately for F14 preparation and F15 repeated help lookup.
|
||||
Neither finding closes merely because the new tree methods exist.
|
||||
|
||||
## Stage 17 — Format declaration paths only for diagnostics
|
||||
|
||||
**Complete.** Declaration traversal now captures one shared scratch slice header,
|
||||
retaining grown capacity across siblings and truncating on every return. Labels
|
||||
are formatted only for emitted issues. The [R1 follow-up](remediation.md#r1--p3-declaration-validation-can-still-copy-a-shared-deep-prefix-for-every-sibling)
|
||||
corrects the initial by-value traversal and records source-level complexity
|
||||
verification, passing focused and full suites, and a deep branching consumer check.
|
||||
|
||||
Findings: F22. Source: `command.go` collectDeclarationIssues.
|
||||
|
||||
1. Traverse with one scratch append/truncate path. Construct an owned formatted
|
||||
label only when emitting a retained issue or root-only-placement diagnostic.
|
||||
Do not capture mutable scratch slices in stored errors or declarations.
|
||||
2. Preserve child/issue order, error wrapping and identity, and absent/invalid
|
||||
declaration handling. Keep other capture/installation phases unchanged.
|
||||
3. Reuse registry/declaration tests for invalid children, root-only placement,
|
||||
retained errors and valid nested trees. Review that the successful walk no
|
||||
longer copies/formats every ancestral path. No timing test or new installation
|
||||
performance guarantee is required.
|
||||
|
||||
Complete when diagnostic scratch work is proportional to the traversal and
|
||||
actual emitted paths rather than every valid full path.
|
||||
|
||||
## Stage 18 — Separate configuration operation preparation
|
||||
|
||||
**Complete.** Operation checks, scope/capability selection, comparison input
|
||||
preparation, and comparison flags have distinct private responsibilities.
|
||||
Focused preflight, help, scope, show, override and comparison tests passed in
|
||||
the offline aligned Go 1.27.0 workspace with `-mod=readonly`.
|
||||
|
||||
Findings: F18. Source: ckconfig `input.go` and its command builder.
|
||||
|
||||
1. Extract operation-specific option checks, shared scope/capability selection,
|
||||
and independent comparison-side preparation into small private functions over
|
||||
existing types. Keep an explicit orchestration function and ordinary branches.
|
||||
2. Extract cohesive flag groups where they clarify command construction. Preserve
|
||||
bare explain help, supplied-state distinctions, full aggregate capability
|
||||
preflight, binding application, and both input checks before shared state
|
||||
construction/acquisition. Do not invent a plan hierarchy or rule registry.
|
||||
3. Run existing operation-preflight, help/no-loading, scope-selector and comparison
|
||||
preparation tests. Confirm independently initialized equal selections and
|
||||
failure classifications remain correct. Retain stage 5's show protection.
|
||||
|
||||
Complete when ordinary and comparison paths can be followed separately without
|
||||
changing the selected lifecycle or adding machinery solely to shorten functions.
|
||||
|
||||
## Stage 19 — Separate configuration suite capture responsibilities
|
||||
|
||||
**Complete.** Scope capture, selector attachment, operation binding unions and
|
||||
inventory construction are explicit responsibilities over the existing prepared
|
||||
types. Focused suite, selector, comparison, capture, help and discovery tests
|
||||
passed offline in the aligned Go 1.27.0 workspace with `-mod=readonly`.
|
||||
|
||||
Findings: F17. Source: ckconfig `family.go` captureSuite.
|
||||
|
||||
1. Extract cohesive scope capture/validation and selector capture/attachment;
|
||||
make operation binding unions and discovery inventory construction explicit.
|
||||
Reuse preparedSuite/preparedScope and keep related loops together.
|
||||
2. Retain declaration copies, scope order, accumulated error order, selector and
|
||||
binding identity, comparison requiredness, empty-suite handling and availability.
|
||||
Setup remains metadata-only: no input factories, state or provider acquisition.
|
||||
3. Run family wiring/capture, operation selector and comparison contract tests.
|
||||
Reuse mutation-after-capture and help/discovery checks. Avoid new indexes or
|
||||
intermediate records merely to split the function into phases.
|
||||
|
||||
Complete when each capture responsibility has a clear place and behavior holds.
|
||||
|
||||
## Stage 20 — Simplify typed preparation and retire remaining test wrappers
|
||||
|
||||
**Complete.** Conversion-error attribution and operand preparation are separate
|
||||
private responsibilities (F19); obsolete reporter, parser and binding wrappers
|
||||
are removed and test-only observation lives in tests (F24). Graph discovery and
|
||||
text references confirmed the current occurrence-aware callers and absence of
|
||||
the retired entry points. Focused binding, replay, occurrence, conversion, routed
|
||||
parsing and reporter tests passed offline with Go 1.27.0 and `-mod=readonly`.
|
||||
|
||||
Findings: F19 and completion of F24. Sources: `binding.go`, `flag_values.go`,
|
||||
`parser.go` and related tests; review reporter cleanup from stage 3 as well.
|
||||
|
||||
1. Extract entered-spelling conversion-error attribution and operand preparation
|
||||
from prepareInputWithOccurrences. Keep flag preparation, ordinal identity and
|
||||
dense reconstruction together, in the orchestration function if appropriate.
|
||||
2. Preserve conversion before input construction, ordinary/ordered/argument setter
|
||||
order, fallback versus parser-log ordering, deferred required checks, original
|
||||
binding identity, supplied states, error identity and the existing replay heap
|
||||
merge. Do not introduce a new state container or replay framework.
|
||||
3. Move tests from prepareInput/prepareInputWithSpellings to occurrence-aware
|
||||
preparation and remove those obsolete production wrappers. Any small setup or
|
||||
read-only observation helpers used solely by tests belong in `_test.go`.
|
||||
4. Confirm all F24 targets: reporter setVisibility/close, App.parse and both
|
||||
preparation wrappers have been retired without losing useful assertions.
|
||||
Use graph call traces supplemented by text references where necessary; absence
|
||||
from one graph query alone is not proof of no callers.
|
||||
5. Run binding conversion/error, typed composition, ordered occurrence, nested
|
||||
capture, reversed/subset/duplicate selections and both replay-method tests.
|
||||
Reuse actual reporter lifecycle and routed-parser checks from prior stages.
|
||||
|
||||
Complete F19 for the bounded extractions and F24 for all obsolete entry points;
|
||||
record these outcomes separately.
|
||||
|
||||
## Stage 21 — Decode native diff paths without revalidation
|
||||
|
||||
**Complete.** Concise diff adaptation decodes tokens from native ConfigKit paths
|
||||
without revalidating arbitrary strings. Focused diff, pointer and snapshot tests
|
||||
pass offline in the aligned Go 1.27.0 workspace with `-mod=readonly`, including
|
||||
root, empty, Unicode and escaped keys.
|
||||
|
||||
Findings: F23. Source: ckconfig `diff_report.go` and diff report tests.
|
||||
|
||||
1. Change the private token helper to accept configkit.Path. Split its canonical
|
||||
JSON Pointer representation, preserving root versus the empty-key token.
|
||||
Decode `~1` and `~0` in one pass, for example with strings.NewReplacer.
|
||||
`~01` must become `~1`, never `/`.
|
||||
2. Remove arbitrary-string validation branches/error returns and corresponding
|
||||
unreachable malformed-string tests. Preserve real snapshot decoding errors,
|
||||
configkit.ParsePath for operator input, and the selected concise diff projection.
|
||||
3. Reuse diff report token/render coverage for root, empty/numeric/dot/percent/
|
||||
Unicode/slash/tilde keys, present null/deletion, structural values and malformed
|
||||
present snapshots. Keep upstream path validity owned by ConfigKit.
|
||||
|
||||
Complete when native-path adaptation retains needed translation without duplicate
|
||||
validation or a new public pointer utility.
|
||||
|
||||
## Stage 22 — Correct current documentation and historical navigation
|
||||
|
||||
**Complete.** ADR status identifies historical lifecycle language, README links
|
||||
to the consumer guide's compatibility owner, and the forward-only ckprompt
|
||||
v0.2.0 erratum links the corrected pinned section. The heading was verified in
|
||||
the tagged Git object and Markdown whitespace checks passed.
|
||||
|
||||
Findings: F25, F26 and F27.
|
||||
|
||||
1. In ADR 0006's Status section, explicitly mark remaining pre-implementation
|
||||
lifecycle wording as historical and point current contracts to GoDoc and the
|
||||
CLI reference. Preserve the accepted decision text rather than rewrite it or
|
||||
create a replacement ADR. Check readers no longer infer reporting is pending.
|
||||
2. In README retain brief status/module orientation and a link to the consumer
|
||||
guide's module-selection section. Remove the duplicate exact compatibility
|
||||
matrix; keep the consumer guide as its owner. No dependency changes follow.
|
||||
3. Add `integrations/ckprompt/docs/releases/errata.md` with a brief forward-only
|
||||
correction for v0.2.0's consumer-guide section link. The corrected target is
|
||||
the existing pinned v0.2.0 guide with fragment
|
||||
`#promptkit-configuration-definitions-and-requirements`. Link this correction
|
||||
from the developer guide's completed-history navigation so it is discoverable.
|
||||
Preserve the old note, immutable tag and hosted release contents.
|
||||
4. Verify the corrected fragment against the local tagged Git object, all new
|
||||
local paths/fragments, and the semantic claims of the edited prose. Run
|
||||
documentation whitespace checks. Do not republish or contact hosted services.
|
||||
|
||||
Complete when present guidance and useful historical navigation have clear owners.
|
||||
|
||||
## Stage 23 — Verify the assembled sprint and record completion
|
||||
|
||||
**Complete.** The full aligned workspace suite passed with Go 1.27.0 using
|
||||
`GOTOOLCHAIN=local GOPROXY=off go test -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/...`.
|
||||
Focused reporter tests passed with `-race -count=1`. Independent `GOWORK=off`
|
||||
root and ckconfig suites passed; root selects Cobra v1.10.1, pflag v1.0.9 and
|
||||
YAML v4.0.0-rc.6, and ckconfig selects core v0.4.0 and ConfigKit v1.1.0.
|
||||
Independent ckprompt selects ckconfig v0.4.0, core v0.4.0, ConfigKit v1.1.0
|
||||
and PromptKit v0.20.0; its suite fails to compile because released ckconfig lacks
|
||||
`AdaptDiagnostics`. This is pending release alignment, not a passed independent
|
||||
check. No manifests, checksums, workspace files, or Go requirements were changed.
|
||||
|
||||
Core's default reporting example, ckconfig ordinary show and two-sided diff,
|
||||
composed all-scope show, prepared preview and local fixture-backed aggregate smoke
|
||||
completed. Bash interactive backend and consumer checks passed; full opt-in runs
|
||||
reported missing Zsh and Fish executables, so those interactive checks are
|
||||
unavailable. The five P2 regressions reproduced their original defects before the
|
||||
fixes. All 27 catalog findings have completion evidence, including both F02
|
||||
boundaries, F14/F15 traversal costs, F21 callers and F24 wrappers. Go formatting,
|
||||
changed Markdown local links and fragments, the tagged historical fragment, and
|
||||
tracked and staged Markdown whitespace checks passed. Source completion does not
|
||||
publish any module; pending release alignment is recorded in future work.
|
||||
|
||||
Findings: all F01–F27; no new feature work.
|
||||
|
||||
1. Review the full diff against this baseline and all roadmap completion criteria.
|
||||
Verify every finding has its remedy and relevant evidence, including both
|
||||
F14/F15 costs, both F02 boundaries, all F24 wrappers, and both F21 callers.
|
||||
Check tests added for P2 defects fail against their original behavior and
|
||||
exercise the actual public/package lifecycle; avoid exact incidental wording.
|
||||
2. Run the full aligned current-source workspace suites from the root:
|
||||
|
||||
```sh
|
||||
GOTOOLCHAIN=local GOPROXY=off go test -mod=readonly ./... ./integrations/ckconfig/... ./integrations/ckprompt/...
|
||||
```
|
||||
|
||||
Run focused reporter race checks where supported for the final reporting
|
||||
changes, recording unavailable support. Broaden or repeat tests only for
|
||||
changed content, failures or unresolved concerns.
|
||||
3. Run independent suites from the root and each integration directory with
|
||||
`GOWORK=off GOTOOLCHAIN=local GOPROXY=off go test -mod=readonly ./...` when
|
||||
their selected dependencies support the changes. Record exact selections.
|
||||
After F21, explicitly record ckprompt's expected missing-helper limitation
|
||||
against ckconfig v0.4.0; it is not a workspace regression and is not a passed
|
||||
independent check. Do not silently switch dependencies for that claim.
|
||||
4. Exercise representative maintained consumers: the core reporting default,
|
||||
ckconfig ordinary/all-scope show and two-sided diff, and the composed ckprompt
|
||||
prepared preview and fixture-backed aggregate smoke operation. Use the
|
||||
developer guide and example READMEs for actual invocations. Run available
|
||||
opt-in shell checks following repaired fixture/grammar changes; record skips
|
||||
and environment limits accurately. No live model/service or credential use.
|
||||
5. Confirm owning GoDoc/CLI/consumer/internal docs match the final code; validate
|
||||
local Markdown links and fragments, pinned historical links through Git,
|
||||
provenance and absence of template residue. Run tracked and staged Markdown
|
||||
whitespace checks from the developer guide. Ensure source is gofmt-clean and
|
||||
ignored workspace files, upstream versions and Go requirements were preserved.
|
||||
6. Record each finding's completed source work and actual checks in the catalog,
|
||||
stage completion here, and target completion in the roadmap. Clearly preserve
|
||||
pending release/dependency alignment as release work in the existing future
|
||||
record; source remediation must not be described as independently released.
|
||||
Do not retire unresolved findings or mark unavailable checks passed.
|
||||
7. Review status, stage only sprint changes and commit remaining completed units
|
||||
with plain English messages. Report the final source result, validation,
|
||||
pending release alignment, commit identifiers and worktree state. No release,
|
||||
push, or publication is part of this sprint.
|
||||
|
||||
Complete when the targeted source state and its verification are evidenced,
|
||||
with external release alignment explicitly distinguished from source completion.
|
||||
|
||||
## Open Questions
|
||||
|
||||
None. Product, ownership and remediation choices are resolved in the catalog
|
||||
and roadmap. Publication versions remain separate release work. Routine private
|
||||
helper signatures and test organization are implementation choices within the
|
||||
boundaries above, not blockers requiring another design round.
|
||||
@@ -1,169 +0,0 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user