181 lines
9.4 KiB
Markdown
181 lines
9.4 KiB
Markdown
# Alias interaction audit remediation
|
|
|
|
Status: in progress; completed findings are marked individually below.
|
|
|
|
## 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: complete. 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.
|
|
|
|
**Completed:** Discovery selection now checks intermediate rewrite paths when
|
|
the entered and final paths do not supply a context. Canonical and discovery
|
|
help share alias listing across the resolution chain and resolve each displayed
|
|
suffix through actual execution precedence. New public-boundary regressions
|
|
failed before the correction and now cover group/discovery help, expanded
|
|
chains, entered overrides, both help forms, global help aliases, descendant
|
|
flag exclusion, metadata-only help, and matching execution destinations. The
|
|
complete offline root suite passed on Go 1.26.8. No canonical nodes or public
|
|
invocation metadata were added.
|
|
|
|
## 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.
|