commandkit/docs/roadmap/audit/01-correctness.md

# 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.