From c225a66c97e07cf7a81fa4fc4bec6ec27c7727e9 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 20 Oct 2026 00:03:54 +0000 Subject: [PATCH] Record alias implementation review findings --- docs/roadmap/remediation.md | 256 ++++++++++++++++++++++++++++++++++++ 1 file changed, 256 insertions(+) create mode 100644 docs/roadmap/remediation.md diff --git a/docs/roadmap/remediation.md b/docs/roadmap/remediation.md new file mode 100644 index 0000000..a9a4190 --- /dev/null +++ b/docs/roadmap/remediation.md @@ -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.