diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..af7451f --- /dev/null +++ b/docs/roadmap/remediation.md @@ -0,0 +1,170 @@ +# Alias interaction audit remediation + +Status: open; three confirmed findings. + +## Review scope and evidence + +Reviewed commit `08ad7427e2aefa54170c0fa50ef760fe187e3cc2`, including the alias +implementation and its subsequent corrections, against CommandKit's existing +dispatch, help/version, binding, integration, and maintained-example behavior. +The [CLI reference](../cli.md), public GoDoc, accepted +[ADR 0013](../adr/0013-application-alias-routing.md), and applicable policies +remain the contract owners. This review does not reopen completed implementation +stages or select additional public APIs. + +Validation used Go 1.26.8 with network resolution disabled: + +- The uncached root default suite passed with `GOWORK=off`. +- Both integration default suites and their maintained examples passed in the + existing workspace against the local core. This is workspace composition + evidence, not independent module-release validation. Their manifests still + reference published core v0.2.0; the deliberate core-first publication sequence + remains described in the prepared core release note. +- The core example's `ws greet --times=0 --tag=first --tag second`, composed + example's `pk show prompt briefing --campaign spring --fmt yaml`, and simple + ConfigKit example's two-file `config diff` completed successfully. +- Disposable consumers outside the repository confirmed the findings below + through the public API. Additional checks passed for aliases to built-in help + and version, a scoped version-format alias, and the global help alias. +- The runnable-parent failure in R-3 was also reproduced against an exported + pre-alias baseline at `f3b8a129d99d04230f998d0327ffad05f6002d7a`. + +No production code or repository tests were changed during the audit. The +reproductions below describe small fixtures to assemble through the public API; +they are not commands already installed in the maintained examples. Assertions +for remediation should cover CommandKit-owned routing, presentation, and callback +behavior, without retesting upstream domain semantics or fixing whole diagnostic +messages as contracts. + +## R-1: Include intermediate alias prefixes in help discovery + +Status: open. Priority: P2. Category: alias-chain composition. + +**Evidence:** [resolveEnteredRoute](../../route.go#L25) resolves the entered path +to its final form, then looks for discovery contexts using only that final path +and the original path. Intermediate prefixes are lost. +[routeHelpAliases](../../help_view.go#L62) similarly searches only the entered +and canonical prefixes, and +[discoveryHelpAliases](../../help_view.go#L33) searches only the entered and +selected discovery prefixes. Execution can select a longer mapping at an +intermediate step that these help views never examine. + +With canonical empty group `box`, canonical runnable `inspect`, and: + +```text +q → box +p → q +q pick → inspect +``` + +`demo q --help` lists `pick → inspect`; `demo p --help` omits it, even though +`demo p pick` successfully executes `inspect`. + +Replace the last mapping with `q admin pick → inspect`. Then: + +```text +demo q admin --help # status 0; lists pick → inspect +demo p admin --help # status 2; unexpected group operand admin +demo help p admin # status 2; unknown help path +demo p admin pick # status 0; executes inspect +``` + +The existing single-shortcut regressions exercise aliases beneath the final +canonical group. Execution-chain tests also pass. Neither covers help for +routes declared beneath an intermediate shortcut. + +**Required work:** Make help selection and route discovery account for relevant +prefixes reached through alias composition. Display destinations reachable under +actual longest-match precedence, including entered-prefix overrides. Preserve +the original entered path and keep discovery separate from canonical command +nodes. Intermediate rewrites need not become public invocation metadata, and +help need not enumerate every possible composed spelling. + +**Completion evidence:** Both help forms work for the chained discovery example; +group help includes the explicitly declared route reachable through the chain. +Cover an overriding mapping and applicable global help aliases while preserving +descendant flag exclusion and metadata-only help. Execution and help must agree +on the displayed destinations. + +## R-2: Retain discovery-prefix context in usage diagnostics + +Status: open. Priority: P3. Category: presentation consistency. + +**Evidence:** Discovery parsing uses the backend root with only trailing flag +tokens, so parser failures have an empty canonical path. +[execution.present](../../execute.go#L195) adapts usage only for canonical alias +routes. For discovery routes, backend root usage is displayed instead; +[execution.usageLine](../../execute.go#L182) also falls back to root usage when +given a discovery path that is not a canonical node. + +With canonical `inspect`, `list things → inspect`, and `--hlep → --help`: + +```text +demo list --help # status 0; help identifies demo list +demo list --missing # status 2; unknown flag, then Usage: demo [flags] [command] +demo list --hlep=bad # status 2; invalid --hlep, then root usage again +``` + +The latter two diagnostics contain no indication that the entered command path +was `list`. A bare `demo list` identifies `list` in its explanatory message but +still appends root usage. This differs from the entered-path diagnostic behavior +documented in the [CLI contract](../cli.md#application-owned-aliases). + +**Required work:** Present usage from the selected discovery context, retaining +the entered prefix for incomplete-prefix and parser failures. Do not assign a +canonical command identity to a discovery view or add synthetic canonical nodes. +Preserve usage status 2, entered flag spelling, and existing delivery handling. + +**Completion evidence:** Bare-prefix, unknown-flag, and invalid-help-value cases +identify `demo list` in usage. Repeat through a composed group shortcut after +R-1. Check identifying fragments rather than exact formatting, and confirm that +ordinary root and canonical command diagnostics retain their existing behavior. + +## R-3: Preserve empty and dash operands before child-name tokens + +Status: open. Priority: P2. Category: inherited parser/routing inconsistency. + +**Evidence:** [Session.Parse](../../internal/cobra/parse.go#L55) gives the entire +leading non-option token sequence to Cobra's `Find`. Cobra's command search +filters out empty strings and the single `-` token while searching for child +commands. It can therefore look past the first operand and select a child whose +name appears later in the operand list. The subsequent exact-prefix check +rejects that selection instead of parsing the parent's operands. + +Core's [structural route selection](../../route.go#L34) stops at the first token +that is not a canonical child and correctly identifies the runnable parent. +The backend then independently makes a different selection. Alias metadata and +flag context can describe the parent while parsing reports a child-path error. + +Fixture: runnable `run` with repeatable string operand `ARG` accepting zero or +more values, a runnable zero-operand child `run child`, and shortcut `r → run`: + +```text +demo run first child # status 0; parent receives ["first", "child"] +demo run "" child # status 2; exact command-path error, child usage +demo r "" child # status 2; same parser error, parent alias usage +demo run - child # status 2; exact command-path error, child usage +demo r -- "" child # status 0; parent receives ["", "child"] +``` + +Empty strings and `-` are ordinary operands here, not options or registered +child names. They must establish the operand boundary just as `first` does. +The canonical failures also occur at the pre-alias baseline identified above; +this is not a newly introduced alias regression. It remains relevant because +the new routing layer and the existing parser disagree on the selected command, +and both canonical and aliased entry points reject valid parent invocations. +Existing runnable-parent tests use ordinary nonempty operands without this +later child-name token. + +**Required work:** Keep backend selection consistent with the structural command +boundary and preserve operand tokens for ordinary typed preparation. Do not +strip or normalize these operands, require an extra `--` to make them usable, +or relax exact command-name matching. Keep genuine child selection working when +the child name immediately follows the parent. + +**Completion evidence:** Canonical and aliased parent invocations accept empty +and `-` operands followed by a child-name token, deliver the original ordered +values, and expose parent invocation paths. Cover parent-local flag aliases and +help on the same inputs, plus genuine child selection and ordinary terminator +behavior. Tests should establish CommandKit's boundary, not depend on a specific +replacement routing algorithm or backend type.