fix(autocomplete): option-value & option-region completion parity edges - #55
Conversation
…es (#54) Follow-ups from the #50 review. None is a regression; each is a suggestion-vs-execution parity edge in the option-value / option-region handling. - Result-flow pending honors the configured option case sensitivity: under CaseInsensitive, GlobalOptionParser accepts "--RESULT:PAGE-SIZE" and consumes the next token as the value, so completion no longer offers a command there (was compared Ordinal-only). - "--answer:" is offered like the other static globals: the built-in prefill (GlobalOptionParser.TryParsePromptAnswer) was missing from the shared OptionTokenCompletionSource, so "--ans" completed nothing. - Child contexts follow the same option-region guard as commands: once the terminal route carries trailing option tokens, "parent --force c" no longer offers the "child" context (execution treats it as the route's option text, not a context entry). - A pending option value invokes ONLY that option's own provider: the pending path reused the route's sole registered completion without checking which parameter it targeted, so "run app --channel " offered the target positional's values as channel values (and offered nothing when both had providers). It now resolves the pending option's parameter via the route OptionSchema and runs that parameter's completion, or nothing if it has none. Shell completion inherits the result-flow fix (it shares IsPendingOptionValue). All red-first; full suite green (1166, 1 external skip). Extracted CollectStandardAutocompleteSuggestionsAsync to keep the entry point under MA0051.
![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.
…etion output The "What completion returns" enumeration was stale: completion also surfaces the built-in `--answer:` prefill (added here for interactive too) and the `--result:*` result-flow flags (since the option-completion feature landed). List them so the doc matches what OptionTokenCompletionSource actually offers.
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.
…alue filter (review) Address the three P2 parity threads on #55 (Codex + autocarl), all suggestion-vs-execution mismatches on the contract this PR closes. - A pending GLOBAL value after a route option no longer runs the route option's provider. ResolveCommitted strips globals before route resolution, so terminalRoute.RemainingTokens can still end with an earlier route option; the pending-value path now keys off the ACTUAL pending token (the last committed token, carried on the resolution state) and only runs a provider when that token is itself a pending route option. A global (no per-command provider) offers nothing. - "--answer:" is matched case-insensitively regardless of OptionCaseSensitivity, mirroring GlobalOptionParser.TryParsePromptAnswer (OrdinalIgnoreCase). Moved out of the case-filtered static list into a dedicated case-insensitive add, so "--ANS" surfaces it under case-sensitive options. - Pending option value providers only surface values the invocation parser would consume as a separate value: dash-prefixed candidates are dropped (they parse as the next option, leaving the option unset), signed numeric literals are kept. Reuses the parser's own ShouldConsumeFollowingTokenAsValue (now internal) so the two cannot drift; the filter is prefix-independent, covering the empty-prefix case. Tests (red-first): When_PendingGlobalFollowsRouteOption_Then_RouteProviderIsNotInvoked, When_AnswerPrefixIsUpperCasedUnderCaseSensitive_Then_AnswerOptionIsSuggested, When_PendingOptionProviderReturnsDashValue_Then_ItIsNotOffered. Full suite green (1169, 1 external skip).
…t on pending value (review batch 2) Address autocarl's second review batch on #55 — three more suggestion-vs-execution parity edges on the option-value / option-region path. - Pending enum options now complete their member names on the interactive path too, matching shell completion. When the pending route option has no explicit WithCompletion provider and its target parameter is an enum, its member names are offered (filtered by the parameter's effective case sensitivity). Mirrors the shell engine's TryAddRouteEnumValueCandidates; closes the documented interactive gap. - Ambient commands (help/exit/…) follow the same option-region guard as commands and contexts: once the terminal route carries trailing option tokens, no ambient (or ambient-continuation) candidate is offered — "parent --force help" is routed option text, not an ambient call. - A pending option value with no candidates is no longer flagged "Invalid" in the live hint: a free-form value (or one whose provider returned nothing) is a value the parser accepts, so BuildLiveHint suppresses the invalid fallback when the position is a pending value. Tests (red-first): When_PendingEnumOptionAwaitsValue_Then_EnumMembersAreOffered, When_OptionPrecedesAmbientPosition_Then_AmbientIsNotSuggested, When_PendingOptionValueHasNoProvider_Then_HintIsNotInvalid. Updated the shell-completion doc's parity note (enum values now complete on both surfaces). Extracted FormatSingleSelectionHint for MA0051. Full suite green (1172, 1 external skip).
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Convergence re-review — ship
Rechecked the current head d43e881 and the two fix commits since 23d5d09. All six previously material reproducers now pass:
- a pending global after a route option no longer invokes that route option's provider;
--ANSoffers--answer:under case-sensitive options;- provider output keeps
alphaand signed-42, but drops non-consumable--prod; run --mode Doffers the enum memberDebug;- scoped
--force eno longer offers the ambientexitcommand; run --channel alphano longer rendersInvalid: alphafor a free-form value.
I also checked a global/route alias collision (--channel): the global still wins and the route provider does not leak.
Validation: 8/8 focused regressions passed; full solution passed 1,172/1,173 with the one expected external MCP Inspector skip; CI is green; git diff --check passes; worktree is clean.
One small non-blocking UX edge remains suitable for follow-up: an invalid enum prefix such as run --mode Z now has no candidate and no Invalid hint because hint suppression is keyed to the broad pending-value state. That does not change execution or completion correctness, and I would not hold this PR for another round.
|
@codex please review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d43e881296
ℹ️ 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".
…ase-sensitive value dedupe (review batch 3) Address Codex's review on d43e881. - The pending-value invalid-hint suppression (from the previous batch) was too broad. It now suppresses the "Invalid: <token>" hint only when the current token is one the parser would consume as the option's value (empty, a plain value, or a signed numeric literal). An option-like token such as "--prod" is read as the next option and leaves the option unset, so "run --channel --prod" keeps its Invalid feedback. - Pending option values dedupe case-sensitively (Ordinal). A string option value is case-significant at execution, so provider results that differ only by case ("Prod"/"prod") must both survive rather than collapse under the UI's (possibly case-insensitive) comparer. Tests (red-first): When_PendingOptionValueIsOptionLike_Then_HintIsInvalid, When_PendingProviderReturnsCaseDistinctValues_Then_BothAreOffered. Full suite green (1174, 1 external skip). Not changed: Codex's ambiguous-pending-token finding is not reachable through the public API — ReplOptionAttribute.CaseSensitivity is a nullable enum (cannot be set via attribute), and duplicate tokens are rejected at build by ValidateTokenCollisions, so resolution can never return two distinct parameters for a token. The constrained-enum invalid-hint edge autocarl noted is tracked in #56 (non-blocking UX follow-up).
|
Review batch 3 (commit 3133330): addressed the two reachable Codex findings — the invalid hint is now restored for non-consumable pending values (e.g. |
|
@codex Last review please. |
autocarl
left a comment
autocarl
left a comment
There was a problem hiding this comment.
Final re-review — approved
Rechecked head 3133330: the latest hint-consumability and case-distinct provider fixes are correct, all prior material reproducers remain fixed, 10/10 focused regressions pass, the full solution passes 1,174/1,175 with the one expected external MCP Inspector skip, CI is green, git diff --check passes, and the worktree is clean.
I left one non-blocking P3 follow-up in the existing enum-dedupe thread. It is a niche case-distinct enum edge and does not change my merge recommendation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3133330d7a
ℹ️ 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".
…ve case (review batch 4) Address the final re-review of 3133330 (Codex P2 + autocarl P3). - The invalid-hint consumability check is now option-kind-aware. Result- flow suboptions (GlobalOptionParser.TryParseResultFlowOption) reject ANY dash-prefixed value — even a signed numeric like "-1" — unlike route and custom-global options which bind "-42". So "--result:page-size -1" keeps its "Invalid: -1" hint instead of being suppressed. The consumability verdict is computed at the resolution site (which knows the pending option token) via IsCurrentTokenConsumableAsPendingValue and passed to BuildLiveHint. - Pending enum value completion dedupes by the enum's EFFECTIVE case sensitivity, not the UI comparer. C# enums may have case-distinct members (Prod/prod); under case-insensitive parsing execution maps both spellings to the first member, so only one candidate is offered (as shell does). Provider string values keep their case-sensitive dedupe. Tests (red-first): When_PendingResultFlowOptionValueIsSignedNumeric_Then_HintIsInvalid, When_PendingEnumHasCaseDistinctMembers_Then_EffectiveSensitivityDedupes. Full suite green (1176, 1 external skip).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5b8bd7dbd
ℹ️ 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".
…override A token matching both a case-insensitive alias and another option's case-sensitive canonical token — reachable now that the per-option case-sensitivity override is settable — is rejected as 'Ambiguous option' at parse time. Documents the behavior flagged during the #55 review as previously unreachable.
2 participants
Follow-ups from the #50 review — four suggestion-vs-execution parity edges in the option-value / option-region completion handling. None is a regression (pre-#50 offered none of these completions); each closes a case where completion suggested something execution would parse differently.
Closes #54.
Fixes (each red-first)
Result-flow pending honors option case sensitivity. Under
CaseInsensitiveparsing,GlobalOptionParseraccepts--RESULT:PAGE-SIZEand consumes the next token as its value, butIsPendingResultFlowOptionmatchedOrdinal-only — so--RESULT:PAGE-SIZE inscould offerinstall, which execution would swallow as the page size. Now compares with the configured option case sensitivity.--answer:is offered like the other static globals. The built-in--answer:<name>[=value]prefill (accepted byGlobalOptionParser.TryParsePromptAnswer, documented as a global flag) was missing from the sharedOptionTokenCompletionSource, so typing--anscompleted nothing. Added alongside the other static tokens (reaches both interactive and shell completion).Child contexts follow the same option-region guard as commands. fix(autocomplete): suggest interactive option names #50 suppressed route command candidates once a terminal route carries trailing option tokens, but the context path was still invoked. With a
parentroute ([ReplOption] bool force) and aparent childcontext,parent --force cofferedchild— which execution treats as the route's trailing option text, not a context entry. Now gated by the same condition.A pending option value invokes ONLY that option's own provider. The pending path reused the route's sole registered completion without checking which parameter it targeted. For
run {target}with a provider ontargetplus[ReplOption] string channel,run app --channeloffered target values as channel values; with providers on both, the count was no longer one and nothing was offered. It now resolves the pending option's parameter via the routeOptionSchemaand runs that parameter's completion, or nothing if it has none.Notes
IsPendingOptionValue.CollectStandardAutocompleteSuggestionsAsyncto keep the entry point under MA0051.Verification
🤖 Generated with Claude Code