Remove dead generated Git facade - #3575
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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.
Code review
Reviewed the removal of the dead generated Git command facade and the new GenerateCommandFacade gating in the OptionsGenerator. Overall this is a clean, well-tested cleanup: the deletion of Git.Generated.cs/IGit.Generated.cs doesn't leave any dangling references, the hand-written grouped IGit remains the sole public surface (locked in by GitApiSurfaceTests), and the generator changes correctly avoid regenerating the dead facade going forward. CLAUDE.md compliance was also checked (generated-file handling, code-gen architecture, test project conventions) — no violations found.
One architectural gap surfaced in the new gating logic that's worth addressing before it bites a future tool integration:
GenerateCommandFacade isn't consistently respected across the generator set —
DependencyRegistrationGenerator now short-circuits entirely when GenerateCommandFacade is false. That's correct for the facade registration itself, but the same method is also the sole place that emits sub-domain service DI registrations and the [ModularPipelinesIntegration] Register{Tool}Context entry point (lines 79–104 of that file). SubDomainClassGenerator, meanwhile, is not gated by GenerateCommandFacade at all and will still emit sub-domain classes unconditionally.
Git is unaffected today only because it happens to set SubDomainGroup = null for all its commands and hand-provides RegisterGitContext in GitExtensions.cs. But for any future tool that sets GenerateCommandFacade = false while also having SubDomainGroups (a combination the new GeneratorHardeningTests.Command_Facade_Generation_Can_Be_Disabled_Independently_Of_Options test doesn't exercise), the generator would emit sub-domain service classes with no DI registration and no context entry point — generated-but-unreachable/broken code, discovered only when someone tries to wire that tool up.
Suggested fix: rather than gating the whole DependencyRegistrationGenerator output on one flag, split "facade registration" from "sub-domain + entry-point registration" so the latter is emitted whenever SubDomainGroups is non-empty, independent of GenerateCommandFacade. That keeps the flag scoped to what it's meant to control (the flat command facade) and removes the latent trap for the next tool that combines both settings — it also makes SubDomainClassGenerator's unconditional behavior consistent with what actually gets registered, instead of two generators quietly relying on the same tool never using both options together.
(A second candidate issue — Git's new DocumentationOutputDirectory = null silently skipping markdown doc generation — was investigated but is not a real problem: no docs/docs/mp-packages/cli/git.md file exists or ever has, so there's nothing for the cleanup logic to strand.)
GitHub Actions
Failure
There was a problem hiding this comment.
Code review (update)
Re-reviewed at the latest commit (ecde8a8b8/merge of 4ca8550c → head). The overall change is still solid: the dead ModularPipelines.Git.Services.IGit facade is fully removed with no dangling references, GitApiSurfaceTests locks the exported surface down to the single grouped IGit, and the new GitCliScraperTests / CodeGeneratorOrchestratorTests coverage correctly pins GenerateCommandFacade = false and DocumentationOutputDirectory = null for Git going forward. CodeGeneratorOrchestrator's switch to toolDefinition with { ... } for completeToolDefinition is also a nice simplification that removes the risk of a future property being silently dropped from that copy.
The gap from my previous review is still present and unaddressed — the diff for DependencyRegistrationGenerator.cs, ServiceInterfaceGenerator.cs, and ServiceImplementationGenerator.cs is byte-for-byte identical to the version reviewed earlier:
// DependencyRegistrationGenerator.GenerateAsync
if (!tool.GenerateCommandFacade)
{
return Task.FromResult<IReadOnlyList<GeneratedFile>>([]);
}This still means DependencyRegistrationGenerator — the only place that emits sub-domain DI registrations (services.TryAddScoped<I{Tool}{SubDomain}, ...>()) and the [ModularPipelinesIntegration] Register{Tool}Context entry point — short-circuits entirely when GenerateCommandFacade is false. SubDomainClassGenerator, however, is not gated by that flag at all (confirmed: no GenerateCommandFacade check anywhere in that file) and is invoked unconditionally by the orchestrator's foreach (var generator in _generators) loop. Git dodges this today only because it happens to have zero SubDomainGroups and Git hand-writes its own RegisterGitContext in GitExtensions.cs.
The new Command_Facade_Generation_Can_Be_Disabled_Independently_Of_Options test in GeneratorHardeningTests.cs still only exercises a tool with a single root-level command and no sub-domain groups, so it doesn't cover this combination and wouldn't catch it.
Net effect unchanged: the next tool integration that sets GenerateCommandFacade = false while also using SubDomainGroups (a very plausible combination — e.g. a tool that wants grouped sub-commands but a hand-written top-level facade, similar in shape to Git but without Git's null SubDomainGroups) will silently get SubDomainClassGenerator-emitted sub-domain classes with no DI registration and no context entry point. That's a broken build or a DI-resolution failure discovered by whoever tries to wire that tool up next, not by this generator's own test suite.
Suggested fix (unchanged from before): split DependencyRegistrationGenerator's output into two independently-gated pieces — "root facade registration" (gated on GenerateCommandFacade, since it depends on the generated I{Tool}/{Tool} types) and "sub-domain services + entry point registration" (gated on tool.SubDomainGroups.Count > 0, independent of the facade flag). That keeps GenerateCommandFacade scoped to what its doc comment now says it controls, and removes the latent trap instead of relying on every future tool integration avoiding the combination by accident.
This is worth fixing before merge (or as an immediate fast-follow) since it's a correctness trap for the next consumer of GenerateCommandFacade, not a Git-specific concern — but it doesn't block the core goal of this PR (removing the dead Git facade), which is otherwise clean and well-tested.
GitHub Actions
Failure
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Code reviewReviewed the diff (Git facade removal, No issues found. A few things worth calling out as good design choices:
No CLAUDE.md compliance concerns for the touched files (core build/testing guidance doesn't apply to source changes directly, and no generated-file editing rules were violated — the deleted files were the generated ones, not hand edits to them). |
GitHub Actions
Failure
GitHub Actions
Failure
Code reviewNo new changes to review — the diff for all 12 changed files is byte-for-byte identical to the version already reviewed in the previous comment (commit Standing verdict: no issues found. The Git dead-facade removal is clean, the new |
169013e to
209f583
Compare
Code reviewRe-reviewed after the branch was rebased onto No issues found. Confirmed independently:
No CLAUDE.md concerns — no full-solution or full-build-pipeline commands were needed to review this (source/test diff only), and none of the touched files are hand-edits to |
209f583 to
63f793a
Compare
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Code reviewNo new changes to review — the diff is byte-for-byte identical to the version already reviewed in the three prior comments on this PR (same 12 files, same Standing verdict: no issues found. The dead |
1 participant
Summary
ModularPipelines.Git.Services.IGit/Services.Gitcommand facadeModularPipelines.Git.IGitfacade as the only exported Git entry pointGenerateCommandFacademetadata so scheduled Git option generation cannot recreate the dead facade or a conflicting DI extensionBreaking change
This intentionally removes the public but unreachable
ModularPipelines.Git.Services.IGittype for v4. Consumers should useModularPipelines.Git.IGitand itsCommands,Information, andVersioninggroups.Validation
IGit:ModularPipelines.Git.IGitCloses #3488