commandkit/docs/roadmap/remediation.md

Invocation reporting audit remediation

Status: remediation in progress; 1 of 7 findings complete.

Review scope

Reviewed the reporting implementation from 2a1096b through 7c035cc against the feature roadmap, implementation plan, ADR 0014, ADR 0005, and the architecture policy.

The intended ownership is substantially implemented: core owns invocation reporting, ordered presentation selection, coordinated delivery, and finalization; the internal Cobra adapter supplies neutral parsing evidence; optional integrations own native adaptation and command presentation. Providers remain passive, and ckprompt depends on ckconfig without a reverse dependency.

The findings below concern missing presentation and error evidence within that architecture. They do not require a different public architecture or automatic rendering of application causes. No separate unrequested registration restriction, concurrent-dispatch guarantee, or upstream-domain responsibility was identified.

Evidence includes source and test review, maintained example invocations, and temporary public-API audit consumers outside the repository. The audit did not modify implementation code or add tests, and did not rerun the full test suites.

The follow-up interaction audit at 1e532cd reviewed reporting against existing alias contexts, typed bindings, scope/input/state preparation, cancellation, native reports, and maintained consumers. Representative successful invocations covered command and scoped flag aliases with explicit zero and repeated values, quiet help/version, aggregate configuration YAML, quiet comparison, concrete request validation, and offline aggregate smoke testing. Findings 4 and 5 record the additional gaps; findings 1–3 remain open and are not duplicated below.

The algorithmic audit at ee76b0f examined the reporting-specific parser passes, delivery bookkeeping, deferred storage, native diagnostic partitioning, and integration report coordination. It used source-level complexity analysis, not benchmarks or real-world performance measurements. Selected algorithms were excluded from reconsideration, including synchronous serialized reporting, FIFO deferred delivery, rendering rich content under quiet mode, fresh backend trees, and the chosen configuration/smoke report preparation boundaries. Findings 6 and 7 are implementation opportunities within those decisions.

No newly introduced quadratic-time algorithm was identified in the reviewed paths. In particular:

• Reporting selection is linear in the number of flag occurrences. There are four fixed reporting flags and at most two additional reverse searches for selection provenance; these do not create a quadratic pass.
• Deferred insertion is amortized constant time plus callback-list copying; draining is linear in queued messages and details, excluding callback/I/O costs. The avoidable storage and handoff costs are described in finding 6.
• Native diagnostic snapshotting and partitioning are linear in source records, diagnostic records, and the total related-location entries. Nested copying of related locations is not quadratic in the actual input size.
• Explanation combination uses amortized slice appends. Configuration comparison has two sides, and aggregate smoke execution has four selected operations; neither introduces an unbounded nested traversal through all prior results.

The algorithmic audit found no additional ordinary correctness issue beyond findings 1–5. It made no implementation changes and ran no tests or benchmarks.

1. Preserve native PromptKit operation failure explanations

Priority: P2. Status: complete.

• Completed: bundled native operation failures now supply terminal explanations without printing arbitrary application causes. Explicit provider context and independent warnings retain their presentation paths. Native-error forwarding and single-presentation regressions pass in ordinary and quiet modes. Validation: GOTOOLCHAIN=local go test ./... in integrations/ckprompt.

Finding and evidence

WithRequestValidation and FromEngine carry native operation errors back to their callers, but their acquired terminal explanations contain only configuration-loading diagnostic records. runRequestValidation and definitionFailure then retain the native error solely as Cause and present a generic operation heading when no acquired explanation exists. Native request-validation and lookup failures consequently lose their useful explanation at the CLI boundary.

Reproduced in the maintained composed example:

go run . promptkit requirements validate briefing --campaign spring

Its entire diagnostic is:

ERROR: Unable to validate the PromptKit request for prompt "briefing".

A separate public-API consumer using the bundled validation adapter confirmed that the native error contains missing required input artifact: topic, while the acquisition has neither an explanation heading nor a detail renderer. The cause survives, but the operator cannot see the missing input. The maintained promptkit show prompt missing --campaign spring --quiet invocation likewise omits the native lookup explanation.

This falls short of useful access to native capabilities and the roadmap's requirement to keep failures understandable. Separating causes from presentation does not prohibit an integration from deliberately adapting native failure content into an explanation. Existing native-failure tests primarily check classification and cause identity while discarding the diagnostic destination, so they do not protect this handoff.

Required work

• At the bundled PromptKit adapter/command boundary, deliberately provide the relevant native operation failure explanation, using the owning API's public presentation and disclosure rules. Cover validation and the other native inspection/selection paths affected by the same migration.
• Preserve the original cause independently and preserve an explicitly supplied provider explanation. Keep independent warnings separate and avoid presenting the same native failure through two paths.
• Keep core's cause-neutral behavior. Do not solve this by printing arbitrary Outcome.Cause values or treating every application-owned loader/provider error as safe operator text.

Acceptance evidence

Use controlled upstream failure evidence to verify that CommandKit forwards useful native failure content into the terminal explanation in ordinary and quiet modes, with cause identity retained and unrelated warnings still hidden where selected. Check explicit provider explanations for single presentation. These are adapter/presentation assertions, not tests of whether PromptKit correctly detects missing artifacts or invalid selections.

2. Retain independent rendering errors joined with write errors

Priority: P2. Status: open.

Finding and evidence

In reporter.go, recordRendererErrorLocked drops the entire callback error whenever errors.Is(renderErr, writeErr) is true. That test also matches errors.Join(independentRenderErr, writeErr): the callback error can contain an independent rendering failure as well as the propagated sink failure.

The shared helper serves RenderOutput, message details, and terminal details. Dropping the whole joined error loses evidence both from the immediate returned error and from Presentation.*.RenderError.

A public-API audit consumer used a failing result writer and this callback:

_, writeErr := io.WriteString(w, "result")
return errors.Join(renderErr, writeErr)

The invocation failed as expected and retained the sink error, but errors.Is(returnedError, renderErr) was false and the final output RenderError was nil. This contradicts the selected retention of independently returned rendering failures. The current separation test covers returning a standalone rendering error after a failed write, but not a callback that already joins them.

Required work

• Distinguish a callback merely propagating its managed-writer failure from a callback carrying additional independent error evidence. Do not discard the entire returned error just because its chain also contains the sink error.
• Preserve the independent callback error in the immediate error result and bounded rendering observations. Retain the first write error separately and continue avoiding further writes to the failed destination.
• Keep ordinary propagation of a write error from being misclassified as an independently discovered rendering failure. No transcript or new public error-classification API is needed.

Acceptance evidence

Add a focused joined-error regression at the shared reporting boundary. Verify errors.Is access to both failures in the returned error, rendering evidence in RenderError, and sink evidence in WriteError. Preserve existing coverage for plain/wrapped write-error propagation and first-error retention. Exercise a detail-rendering path as needed to establish that the shared fix applies there.

3. Explain failures introduced by CommandKit presentation

Priority: P2. Status: open.

Finding and evidence

Reporter.finalize changes an otherwise successful outcome to failure when presentation failed, then invokes the same ensureFallbackExplanation used for an unexplained failed handler outcome. Consequently, a CommandKit-invoked renderer failure or a result-write failure produces only:

ERROR: Command failed.

The diagnostic destination may be entirely healthy. The caller can inspect Presentation, but the operator is not told that rendering or delivery failed. This also affects successful domain outcomes changed to non-zero status.

Public-API consumers reproduced the generic explanation after both a callback that failed before writing and a rejected result write. The corresponding reporter and execution tests currently assert this generic text, so they preserve the gap rather than the selected distinction.

Stage 4 requires a useful core-owned explanation when presentation turns success into failure. The roadmap assigns handling of reporting failures to CommandKit and distinguishes healthy diagnostic destinations from failed result destinations. The generic fallback remains appropriate for a failed handler outcome that supplies no explanation; it does not explain the presentation failure core itself discovered.

Required work

• When presentation changes a successful outcome into failure and no explanation was supplied, attach and retain an explanation identifying the failed presentation operation, such as result rendering or result delivery. Present it through a healthy diagnostic destination.
• Keep the original optional cause unchanged and preserve supplied explanations. An informative core-owned heading is sufficient; this does not require automatically exposing callback error strings or application causes.
• Preserve the selected outcome/status table, exact generic fallback for otherwise unexplained failed outcomes, and explicit native explanations, including a non-nil renderer that emits no bytes.
• Do not retry a failed diagnostic destination, redirect into substantive output, replay explanation callbacks, or introduce recursive reporting-failure messages.

Acceptance evidence

Update focused tests so a successful operation with an output-rendering or output-writing failure receives a retained presentation-specific explanation through working diagnostics, including quiet rendering failure and a successful domain outcome. Preserve evidence for supplied explanations/causes, generic fallback on unexplained handler failure, and diagnostic-sink failure without retry. Assert the meaningful failure category rather than inventing a new public exact-wording guarantee.

4. Restore cancellation checks between report preparation and delivery

Priority: P2. Status: open.

Finding and evidence

Before the reporting migration, PromptKit's writePromptReport checked the invocation context after a report had been prepared and before writing it. The migrated renderProfileReport formats its text report inside RenderOutput, writes it immediately, and returns success without that intervening cancellation check. The surrounding inspection command checks cancellation only before entering report preparation.

A deterministic public-API consumer supplied a profile inspection with a controlled YAML marshaler in EffectiveModelParams.ExtraParams. The marshaler cancelled the invocation context and returned a valid value while the text report was being prepared. This uses the ordinary report serialization boundary; it does not reenter the reporter or require concurrency or an external service.

Observed results for promptkit show profile demo --format text:

Mode	Context at completion	Status	Cause matches cancellation	Report bytes
Ordinary	context.Canceled	0	No	277
Quiet	context.Canceled	0	No	0

Cancellation occurred before the first destination write. Previously, the shared post-preparation check would have returned operational failure with the cancellation cause and left the report destination untouched. The migration also removed similar post-preparation checks from buffered configuration report paths, including config diff. Its current post-render cancellation branch runs only when rendering already returned an error, so successful preparation does not preserve the old checkpoint.

The plan requires preserving existing cancellation behavior outside the selected reporting changes. Quiet suppression is not intended to turn this cancellation into success. Existing cancellation tests cover acquisition and write failures, but do not cover cancellation during successful report preparation.

Required work

• Restore the integration-owned cancellation checkpoint after report preparation and before delivery in migrated paths that previously had it. Review the removed writePromptReport callers and the buffered configuration paths rather than patching only the demonstrated profile-text case.
• Preserve supported cancellation identity in the operation outcome, while retaining reporter ownership of rendering and destination failures. Apply the same operation decision under quiet mode.
• Keep this change bounded to the existing preparation/delivery behavior. Do not introduce a core-wide rule that rejects every cancelled application invocation, interrupt caller writers, withdraw delivered bytes, or promise cancellation after delivery has already begun. New whole-run buffering is unnecessary.

Acceptance evidence

Use a controlled serialization/preparation callback to cancel before the first report write. Assert operational failure, errors.Is access to context.Canceled, and no report delivery, including quiet mode. This verifies CommandKit coordination, not upstream serialization or domain behavior. Preserve existing coverage for cancellation during acquisition, partial output, and caller-owned writes that have already started.

5. Apply reporting-flag aliases in unresolved-route parse contexts

Priority: P2. Status: open.

Finding and evidence

compileFlagAliases now resolves application-wide aliases to the new reporting flags in root and group contexts. However, execution.prepareSession supplies aliases to the parser only for routeCanonical and routeDiscovery. For an unresolved command path it supplies none, even when the normal parser selects the root or a known group whose reporting flags and alias mappings are available.

With an installed application-wide FlagAlias("-d", "--debug"), a public-API consumer reproduced:

Invocation	Observed diagnostics
app missing --debug	Debug route/selection detail, then the unknown-command usage failure.
app missing -d	unknown shorthand flag: 'd' in -d, with no Debug detail.
app group missing --debug	Debug detail, then the group operand/command usage failure.
app group missing -d	unknown shorthand flag: 'd' in -d, with no Debug detail.
app group run -d	Successful command execution with Debug detail.

The spelling registered for a reporting flag is thus treated as unknown on the failure paths where operators may need debugging. Canonical mode selection on these paths already works. This conflicts with the CLI alias contract: an application-wide alias is effective wherever its canonical target is available. The new reporting flags make these existing parse-context gaps observable for mode selection.

Current reporting parse tests cover a canonical reporting flag after a missing command and aliases on valid routes, but not their combination.

Required work

• Supply the alias mapping applicable to the context used by normal parsing when the full entered command path does not resolve. Cover root and known group parse contexts, preserving existing scope/precedence rules where they apply.
• Keep ordered canonical/alias occurrence handling in the normal parser. Do not add a separate reporting-flag scanner or enable unrelated aliases whose canonical target is unavailable.
• Preserve the original routing failure, handler/factory non-execution, and entered-path usage behavior. Finding a parse context must not manufacture a successfully resolved canonical command identity for the invalid route.

Acceptance evidence

Compare canonical and application-wide aliased reporting flags on unresolved root and nested-group paths. They should select the same mode and retain the same underlying command-path failure rather than invent an unknown-flag error. Include a quiet alias followed by another parse error, without suppressing the terminal usage explanation. Retain scoped-alias precedence, unavailable-target, terminator/value handling, and discovery-context coverage at their existing owners.

6. Avoid retaining deferred messages that need no presentation work

Priority: P3. Status: for consideration.

Finding and complexity

DeferredReporter.enqueue unconditionally stores the category, heading, and copied detail list. Visibility is checked only when finalize drains the queue through submitMessage. Consequently, a long-running handler that submits many simple deferred Debug messages in ordinary mode, or simple Info/Warning messages in quiet mode, retains all of them despite there being no output or renderer work to perform.

Let M be the number of these renderer-free hidden messages and B the bytes of otherwise unreferenced distinct heading text. Reporter storage can grow as Θ(M + B) until finalization. Finalization then makes a second Θ(M) queue array copy before visiting and suppressing every entry. This is linear, not quadratic, but the additional reporter storage for this case can be O(1). The M public submissions still require Θ(M) total call work; this is not a claim that the whole invocation can become sublinear.

Refactoring opportunity

Under the current execution flow, reporting selection is applied before handlers and providers run, with no public operation to change it mid-handler. For a hidden deferred submission with no non-nil detail callback, consider recording the eventual suppression observation without retaining the heading or queue entry. Preserve the existing no-content treatment of an empty heading with no renderer. This removes the avoidable Θ(M + B) retention and its later sweep for renderer-free suppressed messages.

Retain rich hidden submissions: their callbacks must still execute at the selected deferred time against discard so rendering failures are preserved. Retain visible messages in FIFO order. Closed-reporter checks must still run before accepting or discarding any submission. This optimization does not add a public immutability guarantee for future mode APIs or change the selected emission-time visibility rules.

Independently, finalize can detach the reporter-owned queue under its existing mutex instead of copying its array and then clearing the original reference. That makes queue handoff O(1) instead of Θ(M) extra copying; actual visible/ rich message delivery remains linear. An internal delivery path can also avoid recopying callback lists already captured by enqueue, while preserving the selected defensive copy when first accepting caller-owned lists.

These opportunities preserve the chosen queue and synchronous delivery model. They do not justify message caps, dropping visible messages, skipping rich renderers, or a background delivery subsystem.

Review evidence

The retention and copies follow directly from enqueue, the array copy at the start of finalize, and submitMessage's later visibility check. If selected, use focused behavior checks for suppression observations, FIFO delivery, callback capture, hidden renderer failures, and late submissions. Source-level storage analysis is sufficient; no benchmark or allocation threshold is proposed.

7. Stop normalizing an argument tail that parsing cannot reach

Priority: P3. Status: for consideration.

Finding and complexity

In interpretFlagTokens, the first unknown long flag or unknown short-cluster member sets recordingOccurrences = false, but interpretation continues through the remaining argv. Later recognized flags still populate spelling lists and construct canonical --name=value strings. Session.Parse then calls the backend, which fails at the earlier unknown flag without consuming that normalized tail. The adapter does not enable unknown-flag whitelisting.

For example, after --unknown, a later valid --message=LARGE_VALUE still causes the value bytes to be copied into a new canonical argument, even though no parser, converter, or handler reaches that value. The recent occurrence tracking correctly excludes the tail from selection evidence but retains this pre-existing unnecessary normalization work.

Let F and A be the flag/alias table sizes, P the token/byte processing cost through the first fatal unknown flag, and T the remaining normalization work, including copied value bytes. With expected constant-time map lookups, the helper currently takes O(F + A + P + T) time on that failure path. It can take O(F + A + P) by stopping at the failing token and growing temporary arrays with the prefix instead of preallocating for all argv. The existing backend can still produce the same failure from that prefix. This is an early-failure improvement, not a quadratic-time finding or a change to the successful parse's necessary linear work.

Refactoring opportunity

Consider ending interpretation immediately after preserving the first unknown flag or failing short-cluster suffix. Preserve any earlier valid occurrences and their spellings, including recognized members preceding a bad short member. Let the backend retain its existing failure classification, wording, and precedence; do not replace normal parsing with a reporting-specific scanner. This can also remove the repeated recordingOccurrences guards needed solely because the current implementation continues past the failure.

The helper currently preallocates both output and occurrence slices from total argv length, so an early return alone does not eliminate its O(argc) upfront array allocation and initialization. To obtain the prefix-bounded helper cost, grow those arrays with the interpreted prefix rather than reserving for the unreachable tail. This changes allocation strategy, not accepted input sizes.

Other routing/parser steps still copy argv string headers, so this refactoring does not make the entire invocation prefix-linear. It specifically removes avoidable tail interpretation, spelling storage, and large-value byte copies. There is no need to reconsider the selected backend or fresh-tree strategy.

Review evidence

The unknown-flag branches and continued spelling/string construction establish the extra work statically. Existing prefix-order, missing-value precedence, short-cluster, and terminator/value tests provide relevant behavior coverage if this is selected. Preserve the backend error and consumed occurrence prefix on early failure; no timing threshold or real-world performance data is needed.