Record alias implementation review findings
This commit is contained in:
256
docs/roadmap/remediation.md
Normal file
256
docs/roadmap/remediation.md
Normal file
@@ -0,0 +1,256 @@
|
||||
# Alias implementation review remediation
|
||||
|
||||
Status: open; R-1 through R-8 require follow-up.
|
||||
|
||||
## Review scope and evidence
|
||||
|
||||
Reviewed commit `a54a9354b1a563849c31dfda906c602950016b02` against the accepted
|
||||
[alias roadmap](aliases.md), [implementation plan](implementation.md),
|
||||
[ADR 0013](../adr/0013-application-alias-routing.md), and applicable architecture,
|
||||
testing, and documentation policies. The implementation range is
|
||||
`d7cc12a..a54a935`.
|
||||
|
||||
The root module's default suite passed independently with `GOWORK=off`.
|
||||
Both integration default suites, including their maintained examples, passed
|
||||
in the existing three-module workspace against the current local core.
|
||||
These checks used Go 1.26.8 with network resolution disabled. Passing suites
|
||||
do not cover the reproductions below: those failures were confirmed separately
|
||||
through the public API using a disposable consumer outside the repository.
|
||||
No production code or tracked tests were changed during review.
|
||||
|
||||
Unless stated otherwise, reproduction commands below assume an application
|
||||
named `demo`, ordinary string operand conversion, and local success callbacks.
|
||||
They describe fixtures to construct through the public API, not commands
|
||||
already installed by the example applications. Use semantic assertions and
|
||||
supported causes when adding regression tests; do not freeze diagnostic prose
|
||||
or private representations unnecessarily.
|
||||
|
||||
## R-1: Protect prefixes of canonical operand space during installation
|
||||
|
||||
Status: open. Priority: P1. Category: canonical command reachability.
|
||||
|
||||
**Evidence:** [traceCanonicalPath](../../registry.go#L318) uses
|
||||
[acceptsOperandCount](../../registry.go#L337), which requires the source's entire
|
||||
remaining token count to meet the command's minimum. This misses an alias
|
||||
source that becomes a prefix of a valid invocation when more operands follow.
|
||||
|
||||
With canonical `run ARG{2..2}` and a separate runnable `inspect` accepting an
|
||||
optional repeated operand, installation accepts:
|
||||
|
||||
```go
|
||||
commandkit.Alias("run first", "inspect")
|
||||
```
|
||||
|
||||
`demo run first second` then runs `inspect`, passing `second` as its operand;
|
||||
the original valid `run` invocation is no longer reachable with that input.
|
||||
The public probe returned status 0 from the wrong handler.
|
||||
|
||||
**Required work:** Check whether a source can intercept a valid canonical
|
||||
invocation, including prefixes completed by later operands, after normal
|
||||
canonical child selection. Do not require that the alias source already
|
||||
satisfy the command's minimum operand count. Apply this against the complete
|
||||
candidate in both registration orders and preserve failed-batch atomicity.
|
||||
Keep the selected distinction between canonical operand space and explicit
|
||||
longer aliases beneath alternate spellings such as `r → run`.
|
||||
|
||||
**Completion evidence:** Reject the example at construction and when either
|
||||
side is added later; preserve the preexisting registry on rejection. Include
|
||||
multi-required and repeated operands and a valid distinct child/shortcut case
|
||||
so the correction does not reserve every route beneath any runnable ancestor.
|
||||
|
||||
## R-2: Select canonical routing context before validating operand counts
|
||||
|
||||
Status: open. Priority: P2. Category: unintended parsing restriction and identity.
|
||||
|
||||
**Evidence:** [resolveEnteredRoute](../../route.go#L25) also uses the complete
|
||||
operand-count predicate from R-1, but it examines only tokens before the first
|
||||
flag. This is not enough information to determine execution arity when flags
|
||||
and operands are interspersed. A known canonical command can therefore become
|
||||
`routeUnknown`, disabling its aliases and discarding its identity.
|
||||
|
||||
With canonical `run ARG{2..2}`, `r → run`, and scoped
|
||||
`--fmt → --format`:
|
||||
|
||||
| Invocation | Observed result |
|
||||
| --- | --- |
|
||||
| `demo r first --fmt=yaml second` | Status 2: unknown `--fmt`, although both operands are supplied. |
|
||||
| `demo r first --format=yaml second` | Handler succeeds but sees `EnteredPath() == "r first"` and `CanonicalPath() == ""`. |
|
||||
| `demo r first --help` | Status 0 with application-root help instead of `run` help. |
|
||||
|
||||
This introduces an unrequested prerequisite: enough operands must appear
|
||||
before the first option for routing metadata and flag aliases to work.
|
||||
It also contradicts the selected exclusion of operands from invocation paths.
|
||||
|
||||
**Required work:** Separate structural canonical selection from execution
|
||||
operand-cardinality validation and from installation's operand-interception
|
||||
check. Retain the selected command, its spelling context, and the actual
|
||||
entered command span even when execution operands are incomplete or invalid.
|
||||
Let the existing parser/input-preparation boundary validate operands after all
|
||||
tokens are interpreted. Help must continue to skip missing execution-required
|
||||
operands without constructing application inputs.
|
||||
|
||||
**Completion evidence:** The valid interspersed invocation succeeds with
|
||||
entered path `r` and canonical path `run`; partial-operand help describes that
|
||||
command and recognizes its applicable aliases. Test canonical and shortcut
|
||||
forms, and ensure genuinely invalid execution arity still produces usage errors.
|
||||
|
||||
## R-3: Preserve exact target lookup for the explicit help command
|
||||
|
||||
Status: open. Priority: P2. Category: help grammar regression.
|
||||
|
||||
**Evidence:** [renderHelpTargets](../../execute.go#L155) reuses execution lookup
|
||||
and accepts `routeCanonical` results even when remaining target tokens are
|
||||
ordinary operands. It then renders the canonical view and drops those tokens.
|
||||
For the `run` fixture above:
|
||||
|
||||
```text
|
||||
demo help r first second
|
||||
```
|
||||
|
||||
returns status 0 and the help for `run`. `first second` are not registered
|
||||
command components. This contradicts the [exact help-path contract](../cli.md#help)
|
||||
and conflates execution operands with a help target's command path.
|
||||
|
||||
**Required work:** Use an exact-target mode or explicit consumed-token result
|
||||
for help lookup. After alias resolution, all target tokens must identify a
|
||||
canonical command/group or a supported alias-prefix discovery view. Do not
|
||||
silently accept execution operands as part of an explicit help path. Preserve
|
||||
ordinary `r first second --help` behavior and help for commands whose execution
|
||||
requires operands.
|
||||
|
||||
**Completion evidence:** Canonical and aliased operand-bearing explicit help
|
||||
targets fail with usage status 2; exact aliases, group shortcuts, and discovery
|
||||
prefixes still render help without application initialization.
|
||||
|
||||
## R-4: Resolve discovery-help contexts through group shortcuts
|
||||
|
||||
Status: open. Priority: P2. Category: incomplete alias composition.
|
||||
|
||||
**Evidence:** Discovery contexts are indexed by declared source prefixes in
|
||||
[discoverFlagContexts](../../flag_aliases.go#L157), while
|
||||
[resolveEnteredRoute](../../route.go#L49) searches them using the original entered
|
||||
path only. A group shortcut can reach a valid alias-only prefix whose entered
|
||||
spelling has no separate context entry.
|
||||
|
||||
With canonical group `box`, canonical `inspect`, and:
|
||||
|
||||
```text
|
||||
pk → box
|
||||
box admin pick → inspect
|
||||
```
|
||||
|
||||
`demo box admin --help` succeeds and lists `pick → inspect`, but both
|
||||
`demo pk admin --help` and `demo help pk admin` fail with status 2.
|
||||
The complete `pk admin pick` route remains available through composition.
|
||||
|
||||
**Required work:** Select discovery metadata through resolved group-prefix
|
||||
composition while retaining the entered spelling for presentation. Reuse the
|
||||
same rule precedence as execution, including more-specific mappings. Keep
|
||||
discovery contexts separate from canonical command nodes and restrict their
|
||||
flags to core help plus applicable global aliases.
|
||||
|
||||
**Completion evidence:** Both help forms work through the group shortcut,
|
||||
show `pk admin` in usage, and list the routes actually reachable there.
|
||||
Cover the global help alias and a more-specific route override; do not add
|
||||
canonical groups or inherit descendant flags to make discovery work.
|
||||
|
||||
## R-5: Include alias routes in help for groups without canonical children
|
||||
|
||||
Status: open. Priority: P2. Category: help discovery gap.
|
||||
|
||||
**Evidence:** [routeHelpAliases](../../help_view.go#L48) returns immediately when
|
||||
the selected canonical node has no canonical children. Alias records are
|
||||
separate from those children, so this suppresses valid routes beneath an
|
||||
otherwise empty group.
|
||||
|
||||
With `Group("box", "Box group.")`, canonical `inspect`, and
|
||||
`box pick → inspect`, `demo box pick` is available, but `demo box --help`
|
||||
lists no `pick` route. The same problem affects help through `pk → box`.
|
||||
|
||||
**Required work:** Determine relevant help aliases independently of whether the
|
||||
canonical group has children. Preserve the separate display of canonical
|
||||
commands and routing records; do not install fake children to fill the view.
|
||||
|
||||
**Completion evidence:** Canonical and shortcut help for an empty group list
|
||||
its explicit alias routes and their resolved destinations, while groups with
|
||||
no such routes retain their ordinary help behavior.
|
||||
|
||||
## R-6: Keep the flag interpretation pass consistent with value consumption
|
||||
|
||||
Status: open. Priority: P2. Category: canonical/alias parsing divergence.
|
||||
|
||||
**Evidence:** [interpretFlagTokens](../../internal/cobra/flags.go#L28) refuses to
|
||||
consume a following `--` as the value of a value-taking flag in both the long
|
||||
and short branches. It subsequently treats that token as the option terminator
|
||||
and stops resolving aliases. The backend still consumes `--` as the pending
|
||||
flag's value, so subsequent tokens are interpreted differently by the two passes.
|
||||
|
||||
For value flag `--format` and alias `--fmt → --format`:
|
||||
|
||||
```text
|
||||
demo show --format -- --fmt red # status 2: unknown --fmt
|
||||
demo show --format -- --format red # status 0
|
||||
```
|
||||
|
||||
The canonical and aliased second occurrences should feed the same binding.
|
||||
|
||||
**Required work:** Preserve value-taking arity consistently between alias
|
||||
interpretation and backend parsing, including when the consumed value is `--`.
|
||||
Treat a token as an option terminator only when it occupies that grammatical
|
||||
role. Do not address this by banning a previously accepted flag value or by
|
||||
introducing a different operand/option convention.
|
||||
|
||||
**Completion evidence:** The paired invocations have equivalent outcomes and
|
||||
values. Cover canonical and aliased long/short spellings, and preserve ordinary
|
||||
termination when `--` occurs after a completed value or boolean flag.
|
||||
|
||||
## R-7: Retain entered flag spellings in parser-level usage failures
|
||||
|
||||
Status: open. Priority: P3. Category: incomplete diagnostic adaptation.
|
||||
|
||||
**Evidence:** Typed flag conversion uses occurrence provenance, but
|
||||
[Session.Parse](../../internal/cobra/parse.go#L89) returns normalized backend
|
||||
failure text directly, and [rawValue.lastBool](../../internal/cobra/parse.go#L474)
|
||||
hardcodes `--help`. Verified examples:
|
||||
|
||||
- Through `r → run`, a missing value for `--fmt → --format` reports only
|
||||
`flag needs an argument: --format`, omitting the entered flag spelling.
|
||||
- With `--hlep → --help`, `demo show --hlep=bad` reports an invalid value for
|
||||
`--help`, omitting `--hlep`; canonical short `-h=bad` likewise loses `-h`.
|
||||
|
||||
The roadmap and [CLI reference](../cli.md#application-owned-aliases) require
|
||||
usage diagnostics to retain the entered spelling, not only for the later
|
||||
typed-conversion path.
|
||||
|
||||
**Required work:** Carry the relevant occurrence context through missing-value
|
||||
and built-in-help conversion failures. Retain the entered spelling, optionally
|
||||
identifying the canonical target, using neutral adapter records rather than
|
||||
exposing backend error types or making brittle whole-message substitutions.
|
||||
Keep the existing usage classification and delivery behavior.
|
||||
|
||||
**Completion evidence:** Check entered spellings for missing long/short alias
|
||||
values and invalid help values, including an earlier invalid occurrence followed
|
||||
by a valid overwrite. Assert the identifying fragments and existing causes
|
||||
where supported; no new exact diagnostic format is required.
|
||||
|
||||
## R-8: Reconcile the completed feature roadmap's historical descriptions
|
||||
|
||||
Status: open. Priority: P3. Category: documentation drift.
|
||||
|
||||
**Evidence:** [aliases.md](aliases.md) now declares implementation complete
|
||||
but still has a `Current baseline` section saying the public model has no alias
|
||||
layer, calls the implemented declarations `planned APIs`, and describes the
|
||||
old `-c=` behavior as current. The consumer guide also calls the temporary
|
||||
roadmap the routing-contract record despite linking the maintained CLI reference
|
||||
next to it.
|
||||
|
||||
**Required work:** Label the preimplementation baseline and old behavior as
|
||||
historical, distinguish delivered implementation from this open review work,
|
||||
and describe the now-implemented APIs accurately. Point consumer contract
|
||||
navigation to GoDoc/CLI owners; retain the roadmap as planning/history, not a
|
||||
second contract owner. Keep completed stage evidence historical and do not
|
||||
restart completed implementation stages or rewrite published release notes.
|
||||
|
||||
**Completion evidence:** Roadmap status, baseline, and API wording agree;
|
||||
consumer navigation identifies the canonical owners and the open remediation
|
||||
work is discoverable without claiming that these findings are already fixed.
|
||||
Reference in New Issue
Block a user