diff --git a/docs/roadmap/audit/01-correctness.md b/docs/roadmap/audit/01-correctness.md new file mode 100644 index 0000000..db7da3c --- /dev/null +++ b/docs/roadmap/audit/01-correctness.md @@ -0,0 +1,218 @@ +# 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](../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.