fix(generator): keep clap option groups and option-attached operands out of required positionals - #4722
fix(generator): keep clap option groups and option-attached operands out of required positionals#4722thomhurst wants to merge 1 commit into
Conversation
…out of required positionals Two regressions from #4470 broke the cargo regeneration (#4692): - CargoCliScraper only read option rows under headings containing "option" or ending in "selection", so clap's `Source:` and `Section:` groups on `cargo add`/`cargo remove` (--path, --git, --branch, --dev, ...) vanished from the generated options. Every heading is now an option section except the positional, command and prose ones (Arguments, Commands, Examples, ...). - UsageSynopsisParser treated an operand attached to an option (`--path <PATH>`) as a positional slot when deciding whether an operand missing from an alternate usage form is still required. With `[OPTIONS]` ahead of every form the phases matched, so `<DEP>` stayed a required constructor parameter even though `--path`/`--git` are alternatives to it. Option-attached operands no longer count as positional slots. Regenerated cargo from cargo 1.98.0 (the same tree as the baseline) and recorded provenance; the only generated change is the explicit IValidatableObject.Validate implementation the generator now emits, and CargoOptionsTests compiles and passes unchanged. Closes #4692 Claude-Session: https://claude.ai/code/session_01PkLNTUfwGXjqXZrYrHaDGC
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Warning Review limit reachedNext included review available in 3 minutes. View limit detailsLimit details: You’ve used all 4 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 03fa1cf31a
ℹ️ 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".
| var positionalCandidates = alternativeArguments.Where(IsPositionalSlot).ToArray(); | ||
| return positionalCandidates.Length == selectedArgumentCount | ||
| && positionalCandidates.Any(candidate => | ||
| candidate.IsRequired | ||
| && candidate.PositionIndex == selectedArgument.PositionIndex |
There was a problem hiding this comment.
When the highest-ranked synopsis contains an option-owned operand, such as tool run --file <FILE> <TARGET>, and another form contains one renamed positional such as tool run <OBJECT>, the filtered counts are both one, but <FILE> still participates in the index fallback and matches <OBJECT> at index 0. Consequently <FILE> remains required even though the alternate form omits --file; require selectedArgument to be a positional slot before applying this fallback.
Useful? React with 👍 / 👎.
GitHub Actions
Inactive
ReviewThis is a well-targeted fix for the cargo regeneration regression, and the root-cause analysis in the description is solid — both bugs are demonstrated with new, focused tests rather than just patched around the symptom.
Both changes are made in the shared scraper/generator logic rather than cargo-specific special-casing, in line with the project's stated preference for generic fixes that benefit every tool. Regenerated output is minimal (just the One non-blocking observation: No prior review comments on this PR raised anything actionable (Codex found nothing, CodeRabbit was rate-limited), so there's nothing outstanding to verify against. |
1 participant
Summary
The scheduled cargo regeneration (#4680, run 34006412119) failed because
CargoOptionsTestsno longer compiled against the regeneratedCargoAddOptions. Regenerating locally from cargo 1.98.0 (Dockerrust:1-slim, the same command tree as the baseline) showed the test break was a symptom: the generator had lost every option undercargo add'sSource:andSection:headings (--path,--git,--branch,--rev,--registry,--dev,--build,--target) and turnedDepinto a required constructor parameter. Both regressions came from #4470 (2026-09-02), one day after the last successful cargo regeneration (#4487).Changes
CargoCliScraper: the heading gate accepted only headings containing "option" or ending in "selection". clap groups options under arbitrary headings, so every heading is now an option section except the positional, command and prose ones (Arguments,Commands,Examples,Usage,Environment,Notes,See also). The existingExamples:fixture still stays out.UsageSynopsisParser.RelaxArgumentsMissingFromAlternatives: an operand attached to an option (--path <PATH>) counted as a positional slot at the same index, so with[OPTIONS]in front of each form (same phase)<DEP>was never relaxed even though--path/--gitare its alternatives. The old fixture only passed because its forms had different phases. Option-attached operands are no longer positional slots. This is the same mechanism behind thePipUninstallOptions(Package)finding on [Automated] Update pip CLI Options #4554 (-r <requirements file>is option-attached).Cargo.Generation.jsonprovenance the way the workflow does. The only generated change is the explicitIValidatableObject.Validatethe generator now emits (same as [Automated] Update gcloud CLI Options #4665's gcloud output);CargoOptionsTestscompiles and passes unchanged.Validation
CargoCliScraperTests,UsageSynopsisParserTests,CliScraperTraversalTests(157) and the full Scrapers namespace (664) pass.ModularPipelines.Rust.UnitTests(7) passes against the regenerated options.CargoAddOptions.Validateentry, so the explicit implementation removes nothing that was declared.Sync-PublicApiBaselines.ps1threw "Compiler error log contains no RS0016 public API entries" locally and was not used.Closes #4692
https://claude.ai/code/session_01PkLNTUfwGXjqXZrYrHaDGC