Fix dotnet tool run generation - #4246
Conversation
|
Too many files changed for review (129 files, 100 file limit). Bypass the limit by tagging |
GitHub Actions
Inactive
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (12)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change adds repeated option terminator support, improves DotNet CLI positional metadata and compatibility preservation, removes current-directory defaults from generated comments, adds validation tests, and documents the ChangesDotNet CLI generation and documentation
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR corrects generated .NET command metadata and argument rendering, with regression tests, a clean release build, and generated-output validation reported successful. No actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant DotNetOptions
participant CommandArgumentBuilder
participant DotNetCLI
DotNetOptions->>CommandArgumentBuilder: provide platform and extension arguments
CommandArgumentBuilder->>DotNetCLI: emit first -- and platform options
CommandArgumentBuilder->>DotNetCLI: emit repeated -- and extension options
DotNetCLI-->>CommandArgumentBuilder: execute rendered command
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The parser and command-builder changes address the required tool-run argument handling. However, the generated DotNetToolRunOptions.Generated.cs file is excluded from review, so the final generated Tool operand and execution path cannot be fully verified. Resolution Provide the generated DotNetToolRunOptions.Generated.cs file or equivalent generated-output evidence confirming that DotNetToolOptions.Tool is emitted into the command as dotnet tool run <tool> <arguments>. Confirm empty or missing tool handling if that behavior is required. Full details: Out of Scope Changes checkExplanation The changes support the stated objectives. Documentation, generator hardening, metadata preservation, option-terminator handling, and regression tests are related to the .NET command-generation fix. No clearly unrelated code changes are shown.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 3eaaf8aba7
ℹ️ 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".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review of #4246 — Fix dotnet tool run generation
Verified the diff directly (121 auto-regenerated files + 5 hand-written source/test/generator files) against a fresh clone of the PR head. Summary of what changed:
DotNetCliScraper.ApplyUsageCardinalitynow mergesCSharpType,IsRequired, andIsVariadicfrom the usage-synopsis parser into any name-matched positional argument, instead of only mergingIsVariadic/CSharpTypewhen the usage-derived argument was variadic. This is what fixesdotnet tool run'sCommandNamelosing itsIsRequired = true(previously the merge only ever looked at variadic args, so a required-but-non-variadic operand's required-ness from the usage line was silently dropped).- New
GeneratorUtils.RemoveCurrentDirectoryDefaultstrips[default: <cwd>]suffixes (in both slash orientations) from generated doc comments so they don't bake in the machine's build-time working directory. - The other 121 files are the expected regenerated
*.Generated.csoutput; spot-checked several against sibling files and found nothing that looks hand-edited or coincidentally altered — no other command'sRequired/CSharpTypemetadata changed in this regen.
Finding: ApplyUsageCardinality's broadened trust isn't scoped, and only tool run is regression-tested
ParsePositionalArguments (help-text Arguments: section parser) hardcodes IsRequired = false for every operand it emits — it has no way to know an operand is actually required. Previously ApplyUsageCardinality never trusted the usage-line parser's IsRequired for non-variadic matches either, so this hardcoded false was effectively unconditional for non-variadic operands. The fix now trusts the usage-line parser's IsRequired/CSharpType for any name-matched operand, for every dotnet subcommand that has both a help-text Arguments: section and a usage synopsis (i.e. most of them, not just tool run — see the call site at DotNetCliScraper.cs around the ApplyUsageCardinality(positionalArgs, usagePositionalArgs) call, which isn't conditioned on the command).
That's the right fix for tool run, and I confirmed empirically that no other command's generated Required/CSharpType flipped in this particular regen (the two parsers happen to agree today). But nothing in the code scopes the broadened trust to the case it was written for, and only tool run's CommandName/ToolArguments got new assertions in DotNetCliScraperTests.cs. If a future SDK version's usage synopsis and help text ever disagree on optionality for some other command (say dotnet build's PROJECT | SOLUTION | FILE operand), this merge would silently flip that command's generated property from optional to required on the next scrape/regen, which is a breaking API change for consumers with no test catching it.
Suggested improvement: add a regression test asserting a non-tool-run command (e.g. dotnet build's ProjectSolution) keeps its expected IsRequired/CSharpType through ApplyUsageCardinality, so future scrapes that hit a genuine parser disagreement fail loudly in CI instead of silently reshaping a public options class. That turns "we verified this by hand once" into a durable guardrail, which matters more here than in typical code because this generator's output is a public API surface.
Minor: duplicated path-variant logic in GeneratorUtils.cs
NormalizeRunnerHomePaths and the new RemoveCurrentDirectoryDefault both independently build an identical forward/backslash variants array (.Replace('\\','/') / .Replace('/','\\') + .Distinct(StringComparer.OrdinalIgnoreCase)) from a trimmed path, and the two already use different trimming calls (.TrimEnd('/', '\\') vs Path.TrimEndingDirectorySeparator) for the same intent. Factoring this into a shared GetPathSeparatorVariants(string path) helper would remove the duplication and the risk of the two implementations drifting further apart on edge cases (UNC paths, trailing dots, etc.).
Everything else — the generated-file regen, the tool run fix itself, and the new tests for it — looks correct and consistent with the project's documented generator workflow.
Preserve authoritative usage cardinality when rich .NET help descriptions are merged. Strip generation-directory defaults from XML documentation. Closes #4245
3eaaf8a to
5e1404d
Compare
|
Addressed review findings in 5e1404d:
Validation: 1,231 generator tests passed; 36 DotNet tests passed with 2 pre-disabled; deterministic second generation produced no diff; touched-file format verification and docs build passed. |
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e1404d8dd
ℹ️ 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".
There was a problem hiding this comment.
Review of #4246 — Fix dotnet tool run generation
Re-reviewed at head 5e1404d8dd2905f12df9a181c4210494f31476fe. The two findings from the prior review (scoping/regression-test gap in ApplyUsageCardinality, and duplicated path-variant logic in GeneratorUtils) have both been addressed:
Build_Preserves_Optional_Project_Operand_Metadatanow locks in thatdotnet build'sProjectSolutionstays optional/non-variadic throughApplyUsageCardinality, guarding against the broadened merge silently flipping some other command's operand to required on a future regen.NormalizeRunnerHomePathsandRemoveCurrentDirectoryDefaultnow shareGetPathSeparatorVariants, removing the duplicated slash/backslash variant logic.
Finding: dotnet tool run's ToolArguments uses the wrong Phase, so combining it with any Normal-phase option throws at runtime
ApplyPositionalArgumentFixes (tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/DotNetCliScraper.cs, the "tool run" branch) only sets PrependOptionTerminator = true on ToolArguments; it leaves Phase at the default CommandLinePhase.EarlyOperand inherited from ParsePositionalArguments. The generated DotNetToolRunOptions.Generated.cs reflects this:
[CliArgument(1, Phase = CommandLinePhase.EarlyOperand, PrependOptionTerminator = true)]
public IEnumerable<string>? ToolArguments { get; set; }CommandArgumentBuilder.GetRenderOrder renders EarlyOperand (order 0) before Normal (order 1, the phase every plain [CliFlag]/[CliOption] defaults to — including AllowRollForward here). ValidateOptionTerminatorOrdering in src/ModularPipelines/Helpers/Internal/CommandArgumentBuilder.cs explicitly guards against an operand emitting its -- terminator before a later-phase option renders, and throws InvalidOperationException when it detects that. Concretely:
new DotNetToolRunOptions("csharpier")
{
AllowRollForward = true,
ToolArguments = ["check", "--help"]
}throws "CLI argument 'ToolArguments' emits an end-of-options marker before a later flag or option. Move the argument to a later phase or remove its option-terminator setting." at command-build time, instead of producing the valid dotnet tool run csharpier --allow-roll-forward -- check --help. This isn't a hypothetical edge case — --allow-roll-forward is DotNetToolRunOptions's only other property, so it's the first thing a consumer would reach for alongside forwarded tool arguments, and it fails every time.
The one test added for this command, Tool_Run_Prepends_Option_Terminator, only exercises ToolArguments alone, so this doesn't surface in CI. The scraper test Preserves_Current_Option_And_Operand_Arity also only asserts toolArguments.PrependOptionTerminator is true, never checking Phase.
Why this is the right fix, not just a smaller one: the codebase already has an established, tested pattern for pass-through operands that need an option terminator — BrewCliScraper's RequiredArgument/VariadicArgument helpers and the Go/Gradle/Jq scrapers all pair PrependOptionTerminator/variadic pass-through args with Phase = CommandLinePhase.Passthrough (order 3, i.e. after Normal-phase options), specifically so real options can still render before the -- marker. The "tool run" fix should follow that same pairing instead of leaving Phase at its EarlyOperand default:
if (commandKey.Equals("tool run", StringComparison.OrdinalIgnoreCase))
{
return args.Select(argument =>
argument.PropertyName.Equals("ToolArguments", StringComparison.OrdinalIgnoreCase)
? argument with { Phase = CommandLinePhase.Passthrough, PrependOptionTerminator = true }
: argument).ToList();
}More durably: since ValidateOptionTerminatorOrdering already encodes the invariant "an option-terminating argument must render in a phase at or after every option it could conflict with," it would be worth adding a GeneratorHardeningTests (or CliArgumentAttribute-level analyzer) check that flags PrependOptionTerminator = true combined with a Phase earlier than Passthrough/Terminal at generation time — this exact mistake (right terminator flag, wrong phase) is easy to repeat in the next scraper fix and currently fails silently until someone hits the runtime exception.
I'd also suggest extending Tool_Run_Prepends_Option_Terminator (or adding a sibling test) to cover AllowRollForward + ToolArguments together, asserting dotnet tool run csharpier --allow-roll-forward -- check --help, so this is caught the moment the Phase fix lands.
Everything else — the regenerated 121 files, the GeneratorUtils doc-cleanup fix, and the ApplyUsageCardinality change itself — looks correct and matches the stated intent.
Render dotnet tool run options before the option terminator and forwarded tool arguments. Allow explicitly opted-in phase migrations to adopt their phase-scoped position.\n\nRefs #4245
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c4b6246b9
ℹ️ 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".
There was a problem hiding this comment.
Review of #4246 — Fix dotnet tool run generation
Re-reviewed at head 3c4b6246b98121959c1d6b571fb55cb9d30a467c. This commit addresses the one remaining finding from the prior review: dotnet tool run's ToolArguments was rendering in CommandLinePhase.EarlyOperand (order 0) while carrying PrependOptionTerminator = true, so any Normal-phase option (AllowRollForward, order 1) rendered after the -- terminator and tripped ValidateOptionTerminatorOrdering's runtime guard.
Verified the fix directly:
DotNetCliScraper.cs's"tool run"branch now setsPhase = CommandLinePhase.Passthrough(order 3, afterNormal) alongsidePrependOptionTerminator = trueandAllowRenderingPhaseMigrationFromBaseline = true— matching the pattern already used elsewhere (Brew/Go/Gradle/Jq scrapers) for pass-through operands paired with an option terminator.- The compatibility guard change in
GeneratedApiCompatibilityPreserver.cs(AllowsRenderingPhaseMigration) drops theArgumentPositionequality requirement only when the specific property hasAllowRenderingPhaseMigrationFromBaselineset — it's scoped per-property viacommand.PositionalArguments.Any(...), so this doesn't loosen the guard for any other generated property. DotNetToolRunOptions.Generated.csnow emits[CliArgument(0, Phase = CommandLinePhase.Passthrough, PrependOptionTerminator = true)]forToolArguments, as expected.- The new test in
DotNetCommandParserTests.csreproduces exactly the scenario I flagged (AllowRollForward = truecombined withToolArguments) and asserts the correct rendered commanddotnet tool run csharpier --allow-roll-forward -- check --help. I built and ran it locally against this head (ModularPipelines.DotNet.UnitTests,Tool_Run_Prepends_Option_Terminator) — passes. - I also ran the full
GeneratorHardeningTests/DotNetCliScraperTestssuites locally (183 tests) — all pass, including the updated hardening test that now separately assertsPhaseandPositionIndexrather than conflating them.
Both findings from the earlier rounds (the unscoped ApplyUsageCardinality trust/regression-test gap, and this Phase ordering bug) are now resolved with matching regression coverage. Nothing further to flag.
Preserve distinct platform and extension boundaries.
There was a problem hiding this comment.
Review of #4246 — Fix dotnet tool run generation
Re-reviewed at head 3653bb9b1c18d3bc94d86e16716598776a3cdf96. This is a follow-up commit ("fix(dotnet): delimit test argument groups") on top of the previously-cleared head (3c4b6246b9); the earlier findings (unscoped ApplyUsageCardinality, and the tool run Phase/terminator-ordering bug) remain fixed and unaffected by this change.
What changed: dotnet test's PlatformOptions (position 0) and ExtensionOptions (position 1) are both Passthrough-phase arguments with PrependOptionTerminator = true. Previously, CommandArgumentBuilder.AddArguments only ever emitted one -- per render (!emittedOptionTerminator guard), so once PlatformOptions emitted its terminator, ExtensionOptions's own request was silently swallowed — collapsing dotnet test -- <platform> -- <extension> into a single --, merging two argument groups the CLI treats as distinct. This commit adds a new RepeatOptionTerminator flag on CliArgumentAttribute/CliPositionalArgument that, when set, forces a fresh -- even if one was already emitted, and applies it to ExtensionOptions via the same ApplyPositionalArgumentFixes special-case pattern already used for tool run.
Verified directly:
- Traced
AddArguments: forPlatformOptionsthenExtensionOptionsin the same phase, the second argument now re-emits--becauseRepeatOptionTerminatoris true, whileoptionTerminatorIndex ??= args.Countcorrectly keeps pointing at the first terminator (used downstream byCommandLineBuilder.InsertManualOptionsto splice manual/global options before the first--, which is still the right insertion point — options can never legally appear after any terminator perValidateOptionTerminatorOrdering). GeneratedApiCompatibilityPreserver's equality/migration checks (AllowsRenderingPhaseMigration,IsCompatible) were extended to includeRepeatOptionTerminatorsymmetrically with the existingPrependOptionTerminatorhandling, so compatibility auditing stays accurate.- The new/updated tests reproduce exactly this scenario and I ran them locally against this head:
ModularPipelines.UnitTests:CommandLineBuilderTests.Build_Repeats_Explicit_Option_Terminator_For_Later_Argument_Group(1 passed),GeneratedRuntimeMetadataTests.*(32 passed)ModularPipelines.DotNet.UnitTests:DotNetCommandParserTests.*(5 passed), including the newTest_Preserves_Platform_And_Extension_Option_Terminatorsassertingdotnet test --no-build -- --filter Category=Unit -- --report-trxModularPipelines.OptionsGenerator.Tests:DotNetCliScraperTests.Test_Preserves_Both_Option_Terminators(1 passed)
DotNetTestOptions.Generated.cs's new attribute arguments (RepeatOptionTerminator = trueonExtensionOptions) match whatOptionsClassGenerator/CommandOptionsGeneratorwould emit from the scraper'sApplyPositionalArgumentFixes("test", ...)special case — consistent field ordering throughPropertyMetadataandCliPositionalArgument.
No regressions or gaps found. Nothing further to flag.
1 participant
Summary
dotnet tool runValidation
Closes #4245
Summary by CodeRabbit
Documentation
ModularPipelines.DotNetpackage, including installation, usage, supported commands, and configuration options.Bug Fixes
Tests
dotnet tool run,dotnet test, command generation, documentation cleanup, and CLI argument metadata.