diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md index b00330a..618567f 100644 --- a/docs/roadmap/remediation.md +++ b/docs/roadmap/remediation.md @@ -1,6 +1,6 @@ # Invocation reporting audit remediation -Status: three open findings. +Status: five open findings. Findings 4 and 5 were added by the interaction audit. ## Review scope @@ -25,6 +25,14 @@ 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. + ## 1. Preserve native PromptKit operation failure explanations Priority: P2. Status: open. @@ -193,3 +201,122 @@ 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](../../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: open. + +### 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.