Record reporting integration audit findings
This commit is contained in:
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user