commandkit/docs/roadmap/remediation.md

# Invocation reporting audit remediation

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

## Review scope

Reviewed the reporting implementation from `2a1096b` through `7c035cc` against
the [feature roadmap](reporting.md), [implementation plan](implementation.md),
[ADR 0014](../adr/0014-invocation-reporting.md),
[ADR 0005](../adr/0005-invocation-outcomes.md), and the
[architecture policy](../policy/architecture.md).

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.

- [x] 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](../../integrations/ckprompt/adapters.go)
carry native operation errors back to their callers, but their acquired terminal
explanations contain only configuration-loading diagnostic records.
[runRequestValidation](../../integrations/ckprompt/validation_command.go) and
[definitionFailure](../../integrations/ckprompt/command.go) 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:

```sh
go run . promptkit requirements validate briefing --campaign spring
```

Its entire diagnostic is:

```text
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: complete.

- [x] Completed: joined callback errors retain independent rendering evidence,
  while plain and wrapped sink errors remain write failures. Output and detail
  regressions pass, including joined destination errors.
  Validation: `GOTOOLCHAIN=local go test ./...` in the repository root.

### Finding and evidence

In [reporter.go](../../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:

```go
_, 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: complete.

- [x] Completed: presentation failures that turn success into failure now retain
  a core-owned rendering/delivery explanation. Supplied explanations and causes,
  generic handler fallback, quiet rendering, domain status, and failed-sink
  behavior remain covered. Validation: `GOTOOLCHAIN=local go test ./...` at root.

### Finding and evidence

[Reporter.finalize](../../reporter.go) 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:

```text
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](implementation.md#stage-4-implement-deferred-delivery-and-finalization)
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: complete.

- [x] Completed: restored preparation-before-delivery cancellation checks for
  PromptKit catalogs, definitions, requirements, and validation, and buffered
  configuration diff/sources/YAML explanation reports. Cancellation retains its
  operation cause without becoming a rendering failure; native streaming and
  smoke-report behavior remain unchanged. Controlled preparation/serialization
  checks cover ordinary and quiet modes. Validation: module suites pass in
  `integrations/ckconfig` and `integrations/ckprompt`.

### 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](../../integrations/ckprompt/command.go)
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](../../integrations/ckconfig/diff.go). 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: complete.

- [x] Completed: unresolved routes obtain aliases from the context selected by
  normal parsing, through neutral internal metadata. Root and nested-group
  canonical/alias comparisons, quiet failure visibility, scoped precedence,
  unchanged unresolved identity, and factory non-execution pass.
  Validation: `GOTOOLCHAIN=local go test ./...` at the repository root.

### Finding and evidence

[compileFlagAliases](../../flag_aliases.go) now resolves application-wide aliases
to the new reporting flags in root and group contexts. However,
[execution.prepareSession](../../execute.go) 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](../cli.md#application-owned-aliases): 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](../../reporter.go) 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](../../internal/cobra/flags.go), 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](../../internal/cobra/parse.go) 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.