Preserve handwritten Git API during coverage validation - #4037
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedNext included review available in 13 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: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
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 |
GitHub Actions
Inactive
|
Addressed the CodeFactor finding in 3ba9972 by extracting code-generation, cleanup, and assembly-info branches from GenerateForToolAsync. Behavior is unchanged; GenerateCode=false still performs coverage validation while preserving handwritten Git API files.\n\nValidated locally:\n- GitCliScraperTests: 3/3 passed\n- CodeGeneratorOrchestratorTests: 16/16 passed\n- OptionsGenerator Release build: 0 warnings, 0 errors\n- ModularPipelines.Git Release build: 0 warnings, 0 errors\n- git diff --check: clean |
Greptile SummaryThe PR preserves the handwritten Git API while continuing to scrape and validate command coverage.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Generators/CodeGeneratorOrchestrator.cs | Gates generated files, cleanup, and assembly metadata behind GenerateCode while preserving containment and coverage validation. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/GitCliScraper.cs | Integrates Git with the shared scraper, combines short-help streams, and derives nested commands from usage rows. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliScraperBase.cs | Adds path-aware subcommand extraction and propagates command-facade configuration into tool definitions. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Models/CliToolDefinition.cs | Introduces the GenerateCode switch for integrations whose APIs are intentionally maintained by hand. |
| src/ModularPipelines.Git/Generated/Git.CommandCoverage.json | Refreshes Git command coverage with usage-derived command groups and the installed Git version. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Run Git scraper] --> B[Read help from stdout and stderr]
B --> C[Discover top-level and usage-derived commands]
C --> D[Build command definitions]
D --> E[Validate command coverage]
E --> F[Write coverage manifest]
D --> G{GenerateCode?}
G -->|Yes| H[Generate and reconcile artifacts]
G -->|No| I[Preserve handwritten Git API]
Reviews (5): Last reviewed commit: "fix(generator): preserve facade ownershi..." | Re-trigger Greptile
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: PR #4037 — Git CLI scraper subcommand discovery + GenerateCode gate
Nice change overall — generalizing help-text parsing to discover remote/worktree children via usage lines (instead of hardcoding them) is a good direction, and gating generation/cleanup/assembly-info behind a single GenerateCode flag is a cleaner design than the ad-hoc GenerateCommandFacade checks scattered across three generators. Good test coverage added for both the scraper and orchestrator paths.
A few things worth addressing:
1. CommandGroups is still hardcoded, undermining the PR's own goal
GitCliScraper.cs:20-21
private static readonly HashSet<string> CommandGroups =
["remote", "worktree"];The new ExtractSubcommands/usage-line parser is fully generic — it works off any usage: git X ... / or: git X <subcmd> ... block. But it's only ever invoked for commands in this hardcoded set. Verified against real git 2.55.0 help output: git stash -h, git submodule -h, git bundle -h, and git notes -h all emit the same usage-line shape the parser targets, so stash pop, submodule add, bundle create, etc. are silently never scraped or fed into CommandCoverageGuard.
Suggestion: drop CommandGroups and instead attempt subcommand discovery for every top-level command, treating "usage-line parser found no children" as the natural no-op case for genuinely flat commands (e.g. git status). That's a better architecture here because it makes the scraper self-describing from git's own help output rather than requiring a maintainer to remember to update a second list every time a new grouped command needs coverage — which is exactly the kind of manual-sync problem this PR is trying to move away from.
2. Duplicate git <command> -h invocation per group
GitCliScraper.cs:339 and GitCliScraper.cs:431
ScrapeAsync calls ParseCommandAsync([command]), which runs git remote -h. Immediately after, for anything in CommandGroups, DiscoverSubcommandsAsync(command) runs git remote -h again to get the same help text. This doubles process spawns for every grouped command with no behavioral difference. Since ParseCommandAsync already has the combined help text in scope, consider having it return (CliCommandDefinition?, string helpText) (or exposing a lower-level helper both callers can share) so the second invocation is unnecessary.
3. Child commands can reference a parent that was never yielded
GitCliScraper.cs:84-93
var definition = await ParseCommandAsync([command], cancellationToken);
if (definition is not null) { ... yield return definition; }
if (!CommandGroups.Contains(command)) { continue; }
foreach (var subcommand in await DiscoverSubcommandsAsync(command, cancellationToken))
{
var childDefinition = await ParseCommandAsync([command, subcommand], cancellationToken);
...
yield return childDefinition;
}The subcommand-discovery branch doesn't check whether definition was non-null before proceeding. If git remote -h ever returns empty output on both streams (unusual, but ParseCommandAsync explicitly handles that case by returning null), no GitRemoteOptions definition gets yielded, yet children still get yielded with ParentClassName = "GitRemoteOptions" — a class that won't exist in the generated output. This is a low-probability edge case for real git, but worth an explicit guard (if (definition is null) { continue; } before the subcommand loop) so a downstream consumer never gets a dangling parent reference.
4. GenerateCommandFacade = false is now dead configuration for Git
GitCliScraper.cs:49-50, and the three generators listed above that read it
GenerateCommandFacade is only consulted inside DependencyRegistrationGenerator, ServiceImplementationGenerator, and ServiceInterfaceGenerator. Since CodeGeneratorOrchestrator.GenerateFilesAsync now returns [] immediately whenever !toolDefinition.GenerateCode, none of those generators ever run for the Git tool definition anymore — so GenerateCommandFacade on Git's CliToolDefinition is unreachable dead config. Recommend removing it from GitCliScraper.CreateToolDefinition() (or adding a comment noting it's now superseded by GenerateCode) so a future reader doesn't spend time tuning a flag that has no effect.
None of these are blockers — #1 is the one I'd push back on before merging since it directly limits the stated purpose of this PR (broader coverage validation for hand-maintained Git commands); #2–#4 are efficiency/robustness/cleanup nits.
3ba9972 to
fb37300
Compare
|
Rebased two commits onto current main after Kevlar 0.9 (#4032) and force-pushed with an exact lease. Validation: GitCliScraperTests 3/3; CodeGeneratorOrchestratorTests 16/16; OptionsGenerator and ModularPipelines.Git Release builds both 0 warnings/errors; diff check clean. Verified remote head: fb37300. |
|
Addressed all four review items at exact head
Validation: @claude Please review exact head |
GitHub Actions
Failure
Reuse shared traversal and reject operand-shaped usage groups.\n\nRefs #4037
b757798 to
82ad98a
Compare
GitHub Actions
Inactive
96d4178 to
7920d8a
Compare
Reuse shared traversal and reject operand-shaped usage groups.\n\nRefs #4037
7920d8a to
1bd819e
Compare
Review: re-review at head
|
GitHub Actions
In progress
1 participant
Part of #3996
Summary
Validation