From 1e532cdfa5a84d03ebfa6f3cbe001e05f5a7be20 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 26 Oct 2026 12:41:36 +0000 Subject: [PATCH] Record reporting audit findings --- docs/roadmap/remediation.md | 195 ++++++++++++++++++++++++++++++++++++ 1 file changed, 195 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..b00330a --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,195 @@ +# 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.