Repository navigation
fix(glab): tokenize glab args instead of walking them by hand - #4005
Merged
Merged
Conversation
📊 Automated PR Analysis
SummaryReplaces the hand-rolled arg walker in glab_cmd.rs's identifier extraction with a proper tokenizer-based approach, using per-subcommand value-taking flag tables and correct Review Checklist
Analyzed automatically by wshm · This is an automated analysis, not a human review. |
`extract_identifier_and_extra_args` picked the MR/issue identifier with a
hand-rolled walker: a hardcoded eight-entry list of value-taking flags, a
`skip_next` bool, and `arg.starts_with('-')`. Anything the list missed had its
value read as the identifier, and the list itself was a mix of flags from
different subcommands.
Verified against glab 1.36.0 and 1.117.0:
rtk glab mr view --page 2 -> glab mr view 2 -F json --page
rtk glab issue view -P 50 -> glab issue view 50 -F json -P
rtk glab mr view --jq .iid -> glab mr view .iid -F json --jq
Replace it with `arg_tokenizer::tokenize_grammar` plus one `takes_value`
predicate per glab subcommand, transcribed from each subcommand's own `--help`,
and take the first free positional. `mr merge`, `mr approve`, `mr note` and
`mr update` each get their own table: `-s` is the boolean `--squash` on merge
but the value-taking `--sha` on approve, and `-d` is `--remove-source-branch`
on merge but `--description` on update.
`run()` also restores the `--` that clap's `trailing_var_arg` consumes, over
the reassembled `[subcommand] + args` region rather than `args` alone, and
moves rtk's own `-R`/`-g` from the end of the args to the tokenizer's
injection point — glab reads everything past `--` as a positional, so an
appended `-R` arrived as two extra arguments instead of the repo flag.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two fallouts of classifying `--jq` and restoring `--`. `--jq` now reaches glab with its value intact, so glab returns the user's projection — and `run_glab_json` reformatted it as an MR summary of fields the projection does not have (`? MR !0: ???`). Previously the value was hoisted out as the MR number and glab failed loudly, so this turned a visible error into a silent wrong answer. `has_output_flag` now counts `--jq`. A `--` at the head of the region ended rtk's own `-R`/`-g` parsing, not glab's. Forwarding it makes glab stop looking for a subcommand: `glab -- mr view 42` prints root help on 1.117.0. Drop it instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`trailing_var_arg` only ever lets clap strip one `--`, the one at the head of the region it hands to glab — before the subcommand, or right after it, since `-R`/`-g` are parsed before the subcommand positional either way. Only checking index 0 let the index-1 case through: `glab mr -- view 42` prints the `mr` help instead of viewing the MR, and `glab api -- projects/1 --paginate` is `Accepts 1 arg(s), received 2` on glab 1.117. Every later `--` clap passes through untouched and still reaches glab verbatim. `split_identifier` reads positionals past `--` because glab does too (`glab mr view -- 42` views MR 42), but the callers re-emit the identifier ahead of the boundary. When the escaped region held more than the identifier, that promoted a flag the user had escaped back into flag position: `rtk glab mr view -- --web 42` ran glab with `--web` live, opening a browser the user had asked glab to treat as a positional. glab takes at most one positional for these subcommands, so an escaped region holding more than one token is something glab rejects whatever rtk sends. Also isolate HOME and RTK_DB_PATH in the argv test, which otherwise read the developer's own config and wrote to their real tracking database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Refusing to look past `--` only when two or more tokens trailed it still unescaped the single-token case, which is the one the user is most likely to type: `rtk glab mr view -- --web` hoisted `--web` into identifier position and ran glab with the browser flag live, where develop passed the command through untouched. What makes hoisting safe is not how much the boundary escapes but whether unescaping it is a no-op: reach past `--` only for one token that is still read as a positional without it, so `glab mr view -- 42` keeps gaining `-F json`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`escapes_a_bare_positional` landed between the header and the function it describes, so rustdoc rendered the helper under `split_identifier`'s summary and left `split_identifier` undocumented. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
KuSh
force-pushed
the
feat/glab-arg-tokenizer
branch
from
September 17, 2026 22:38
997f192 to
fd84246
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
glab_cmd.rs'sextract_identifier_and_extra_argspicked the MR/issue identifier with a hand-rolled walker: a hardcoded eight-entry list of value-taking flags (-R,--repo,-g,--group,-F,--output,-m,--message), askip_nextbool, andarg.starts_with('-'). Any value-taking flag missing from that list had its value hoisted out and re-emitted as the identifier. The list also mixed flags from unrelated subcommands —-m/--messagebelongs tomr merge/mr note, not tomr view, whilemr view's own-p,-Pand--jqwere absent.run()additionally had no--awareness, and rtk's own-R/-gwere appended after the user's arguments.Reproducers
Verified against a
glabstub that records its argv (tests/glab_argv_test.rs), flags transcribed from glab 1.36.0 and 1.117.0--help, and every expectation checked against real glab 1.117.0 (registry.gitlab.com/gitlab-org/cli:latest,LC_ALL=C):rtk glab mr view --page 2mr view 2 -F json --pagemr view -F json --page 2rtk glab issue view -P 50issue view 50 -F json -Pissue view -F json -P 50rtk glab mr view --jq .iidmr view .iid -F json --jqmr view --jq .iidrtk glab mr view --page 2 42mr view 2 -F json --page 42mr view 42 -F json --page 2rtk glab -R o/r mr diff -- 42mr diff -- 42 -R o/rmr diff -R o/r -- 42rtk glab mr view -- --web 42mr view 42 -- --webmr view -- --web 42rtk glab mr view -- --webmr view -- --webmr view -- --webWhat changed
flags_with_valuearray are gone.split_identifierrunsarg_tokenizer::tokenize_grammarand takes the first free positional (Token::is_free_positional).takes_valuetable per glab subcommand, each transcribed from that subcommand's own--helpand taken as the union over 1.36.0 and 1.117.0.mr merge,mr approve,mr noteandmr updatedo not share one:-sis the boolean--squashon merge but the value-taking--shaon approve,-dis--remove-source-branchon merge but--descriptionon update, and-mis--messageon merge but--milestoneon update.mr viewandissue viewdo share a table — their value-taking flag sets are identical on both versions..claiming_dash_dash(): measured on 1.117.0,glab mr view --page -- 5reportsInvalid argument "--" for "-p, --page" flag, so pflag reads--as the flag's value, not as the boundary.run()restores the--clap'strailing_var_argconsumes, over the reassembled[subcommand] + argsregion rather thanargsalone; overargsalone it lands one token off and duplicates the subcommand.trailing_var_argonly ever lets clap strip one--, the one at the head of that region — before the subcommand, or right after it, since-R/-gare parsed before the subcommand positional either way. That one is rtk's own option terminator and glab never saw it:glab -- mr view 42prints root help on 1.117.0,glab mr -- view 42prints themrhelp instead of dispatching toview, andglab api -- projects/1 --paginateisAccepts 1 arg(s), received 2. So that one--is dropped and every later one reaches glab verbatim.--(glab mr view -- 42views MR 42), so the identifier search reaches past the boundary — but only where unescaping is a no-op: one token, still read as a positional without the--. Re-emitting the identifier in front of the boundary unescapes it along with whatever trailed it, andrtk glab mr view -- --webor-- --web 42would then run glab with--weblive, opening a browser the user had asked glab to treat as a positional. Anything else glab rejects on arity whatever rtk sends (Accepts at most 1 arg(s), received 2), so it goes to glab untouched.-R/-gnow go in atarg_tokenizer::injection_pointinstead of being appended. Position is unchanged when there is no--.has_output_flagnow counts--jq. With the value correctly attached, glab returns the user's own projection, and reformatting it as an MR printed a summary of fields the projection does not have (? MR !0: ???).Tests
tests/glab_argv_test.rsasserts on the child argv, and on rtk's stdout for the--jqcase; each of the 13 tests was confirmed failing before the corresponding change and passing after. The run is isolated with its ownHOMEandRTK_DB_PATH, which it otherwise read and wrote for real. Sixteen unit tests inglab_cmd.rscover the per-subcommand tables, including the-smerge/approve split.Not in scope
has_output_flagandshould_passthrough_viewstill scan witha == "--output", so attached spellings (--jq=.iid) and short clusters miss. That is the separateargs.iter().any(...)bug class, not the identifier walker; left for a follow-up.