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, CLI contract, public GoDoc, and accepted decisions. The internal overview 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 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, NewExecutionFlags/ExtraParams and executionFlag, lines 107–115; 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, 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, runRequestPreparation lines 95–104; the PrepareRequest callback contract in 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, runConfigShow lines 69–95, interacting with Reporter.RenderOutput in 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.

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, 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, lines 28–44; completion_shell_test.go, TestCompletionConsumerInShells; core's reserved-name check in 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.

13 KiB Raw Blame History Unescape Escape