fix(autocomplete): suggest interactive option names#50
Conversation
Add interactive autocomplete option candidates from global and route option sources so --flag prefixes behave like shell completion.\n\nFixes yllibed#43
|
@codex please review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…parser semantics
Review-panel round on the initial commit — all three field repros fixed
red-first, plus the structural findings:
- One option-candidate source (OptionTokenCompletionSource) now feeds both
the interactive engine and the shell-completion engine: the previous
copy-pasted lists could only drift, and a new global option had to be
added twice. Composed tokens (--alias, --output:format) only allocate
when they can match the typed prefix.
- Prior tokens are normalized through InvocationOptionParser exactly like
the shell path: option VALUES are consumed ("--channel beta" no longer
leaves a stray positional that hides route options) and the bare "--"
end-of-options separator suppresses option suggestions past it.
- The committed command prefix goes through the router''s own
ResolveUniquePrefixes expansion, and route-prefix matching mirrors
RouteResolver''s terminal-segment alias rule: alias and unique-prefix
invocations ("i bib-overalls") now surface the route''s options.
- Route options are offered as soon as the command WORDS are typed (only
dynamic argument segments may be missing): "install --" suggests
--force, matching the parser''s options-anywhere acceptance.
- IsOptionPrefixToken gates suggestions: a single dash surfaces short
aliases (-f) while signed numeric literals (-42) stay positional,
mirroring the invocation parser; token classification uses the same
predicate so short flags no longer paint as invalid.
- Dynamic completion providers are skipped when the current token is an
option prefix — provider values are parameter values, not option names.
- OptionSchema.KnownTokens is materialized once (was a recomputed LINQ
projection hit per keystroke per route).
- Tests: regression coverage for every repro above (valued option, alias,
single dash, missing positionals, end-of-options, negative number,
provider pollution, cross-source dedup); neutral test data.
|
Review round on the initial commit (local multi-perspective review + field repros) — fixes pushed in dc6997b, every behavioral fix red-first. Full suite green (1091 tests). Fixed
Considered, deliberately unchanged
|
Option suggestions share AutocompleteSuggestionKind.Parameter, so the live hint could shortcut to "Param skillName" while the menu listed option names. In option-prefix mode the parameter shortcut is skipped and the hint renders the option alternatives.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…eview round 2) - SECURITY: both completion engines parsed prior tokens with the legacy parser profile, whose AllowResponseFiles default meant a @file token triggered server-side file reads (UNC probes included) — per keystroke on remote-controlled lines in hosted sessions. A shared CompletionParsingOptions profile (response files off) now feeds both engines; the execution pipeline already gated expansion on !isInteractiveSession for the same reason. - Single-dash short flags are dropped from the prefix only BEFORE the POSIX "--" separator; after it they are positional arguments and keep their place in the route prefix (exact-route providers fire again). - The shell gate is now the same IsOptionPrefixToken as the interactive menu: "-" surfaces short aliases in shell completion too, and two parity tests pin interactive == shell candidates for "--" and "-" inputs — a divergent reimplementation on either side now fails. - Unique-prefix/alias expansion is bounded by the deepest route/context template: the per-index candidate derivation no longer scales with the token count of an adversarial line; the misleading "menu shows the competing alternatives" comment now states what really happens to an ambiguous committed shorthand. - The autocomplete literal/alias rule delegates to RouteResolver.IsLiteralMatch (comparison parameterized) instead of mirroring it; ReplCaseSensitivity.ToStringComparison() replaces the ternary re-derived at every call site; GlobalOptionParser reuses the shared IsSignedNumericLiteral. - docs/shell-completion.md documents the two deliberate divergences that remain (empty-token options, enum values).
|
Second review round on a1eaafa (multi-perspective panel) — fixes pushed in 168d644, red-first throughout. Full suite green (1096 tests). Fixed
Analyzed, deliberately unchanged
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…guess (review round 3) Prior-token normalization no longer guesses option arities with the generic parser; it mirrors execution's resolve-then-parse order: - The command prefix is built by stripping global options with the parser that knows their arities (GlobalOptionParser: --no-logo takes no value, --output: does), then resolving the route. A matched route contributes exactly its own segments and leaves every route option in its trailing tokens — so a valued short alias (-c beta) or a valueless global flag before the command can never corrupt the prefix. - Route options are sourced from the single route RouteResolver would actually select (fixes overloaded routes offering the wrong overload's options) and only when every required positional is filled (a resolved match with no leftover positional) — accepting a suggestion always yields a command that runs cleanly. Per maintainer decision, options wait until required arguments are satisfied. - The dynamic completion provider is skipped for an option-prefix token only before the POSIX "--" separator; after it a dash token is a positional and the provider runs. - Shell completion mirrors all of the above (route-first prefix, "--" termination state, valued short-alias handling), keeping the two surfaces consistent. Every behavioral fix is covered by a red-first regression test; full suite green (1102).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2f39a0f88b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…case (review round 4) Removes the last divergences from execution's resolve-then-parse order: - BuildCommandPrefix no longer strips the bare "--" separator or drops dash-prefixed tokens before resolving. Route resolution runs before option parsing (as CommittedInputResolver does), so "--" and short tokens like "-prod" bind to route segments as positional values — "remote -prod status" / "remote -- status" now keep the terminal literal suggestion. The no-match fallback keeps the tokens as typed instead of second-guessing which are options. - Shell completion mirrors the same: "--" only records the end-of-options state, tokens stay as typed for route resolution. - Route-option candidates are filtered against OptionSchema.Entries with each entry's own effective case sensitivity, not the flattened KnownTokens under one global comparison — a case-insensitive option is offered for a differently-cased prefix even under a case-sensitive global default (pinned via the shared source, since a per-option override is otherwise not expressible through the attribute). - Shell enum-value completion recognizes short option aliases: a pending option is detected with the single-dash gate, so "-m <TAB>" offers the enum values like "--mode" does. All red-first; full suite green (1106).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae605dbb7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // positional is filled: a resolved match with no leftover positional means the | ||
| // options belong to a command that runs cleanly. This is the same resolution | ||
| // execution performs, so autocomplete never offers an option the run would reject. | ||
| if (app.Resolve(commandPrefix) is { RemainingTokens.Count: 0 } match) |
There was a problem hiding this comment.
Fresh evidence is the optional-segment case: for a route like run {profile?} with a [ReplOption] bool force, the prefix run -- satisfies this check and offers --force, but executing the completed run --force does not leave --force in match.RemainingTokens; RouteResolver binds it to the optional {profile} before option parsing. Route option candidates should wait until optional positionals are disambiguated, or routing needs to skip option tokens before these suggestions are safe.
Useful? React with 👍 / 👎.
| if (string.Equals(values[i], "--", StringComparison.Ordinal)) | ||
| { | ||
| optionsTerminated = true; |
There was a problem hiding this comment.
-- termination until after route resolution
When an earlier bare -- is consumed by the route as a positional, such as remote {name} status with input remote -- status --, this raw scan has already set OptionsTerminated. The route is terminal and execution would parse a completed trailing --force from the route's remaining tokens, but autocomplete suppresses all option candidates because it treats the positional -- as the separator; track the separator only among tokens left after the matched route/global parsing boundary.
Useful? React with 👍 / 👎.
|
|
||
| foreach (var alias in options.Output.Aliases.Keys) | ||
| { | ||
| TryAddComposed("--", alias, currentTokenPrefix, comparison, dedupe, results); |
There was a problem hiding this comment.
With the default case-sensitive option setting, this filters output aliases using an ordinal comparison, but GlobalOptionParser resolves --json and other output aliases through OutputOptions.Aliases, whose dictionary is case-insensitive. In that default configuration, a user typing --J/--JSON can execute the uppercase alias once completed, but neither interactive nor shell completion offers --json; output alias candidates should use the alias resolver's case-insensitive semantics rather than the general option-token setting.
Useful? React with 👍 / 👎.
| var prefix = stripped as string[] ?? [.. stripped]; | ||
| if (app.Resolve(prefix, activeGraph.Routes) is { } match) |
There was a problem hiding this comment.
Execution expands unique command prefixes before route matching, but this shell-completion path resolves the stripped tokens directly. For a route install {name}, app i pkg -- (where i is the unique prefix for install) runs after prefix expansion, while shell completion sees no terminal route here and omits install's route options; expand unique prefixes against the captured graph before calling app.Resolve.
Useful? React with 👍 / 👎.
| if (!AutocompleteEngine.IsGlobalOptionToken(previousToken)) | ||
| // A single dash is enough: short option aliases (e.g. "-m") take values too, and the | ||
| // schema resolves them like any other token below. | ||
| if (!AutocompleteEngine.IsOptionPrefixToken(previousToken)) |
There was a problem hiding this comment.
Because this now treats any previous single-dash token as a pending option, shell completion can offer enum values for a dash-prefixed route argument that was already consumed by routing. For example, with deploy {target} and an option-only enum alias -m, app deploy -m has target = -m; accepting a suggested enum value leaves it as trailing positional text rather than the value for -m. Resolve the pending option from the matched route's remaining option tokens, not from the raw prior command tokens.
Useful? React with 👍 / 👎.
|
@codex please review again |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
--/partial--flagtokens.OptionSchema.KnownTokens.Fixes #43
Test Plan
dotnet test src/Repl.Tests/Repl.Tests.csproj -c Release --filter 'FullyQualifiedName~Given_InteractiveAutocomplete_OptionCandidates'dotnet test src/Repl.Tests/Repl.Tests.csproj -c Release --filter 'FullyQualifiedName~Given_InteractiveAutocomplete'dotnet test src/Repl.IntegrationTests/Repl.IntegrationTests.csproj -c Release --filter 'FullyQualifiedName~Given_Completions'dotnet test src/Repl.slnx -c Release