196 lines
9.3 KiB
Markdown
196 lines
9.3 KiB
Markdown
# Invocation reporting audit remediation
|
|
|
|
Status: three open findings.
|
|
|
|
## 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.
|
|
|
|
## 1. Preserve native PromptKit operation failure explanations
|
|
|
|
Priority: P2. Status: open.
|
|
|
|
### 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: open.
|
|
|
|
### 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: open.
|
|
|
|
### 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.
|