Record the functional correctness audit

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.