Restore generated option API compatibility - #3935
Conversation
|
Too many files changed for review (143 files, 100 file limit). Bypass the limit by tagging |
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (61)
📒 Files selected for processing (19)
Comment |
GitHub Actions
Inactive
![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: f46ac6a2c1
ℹ️ 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".
|
Addressed the Docker Compose --no-TTY casing finding in 9168cdb. Targeted generation was also attempted; the compatibility guard correctly stopped on unrelated installed-Docker API drift (DockerLoadOptions.Platform), so no limits were raised and the single backed generated artifact was synchronized. Focused tests and both relevant Release builds are green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9168cdb0eb
ℹ️ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a0d30e311
ℹ️ 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.
Code review
Reviewed the core logic of this PR: GeneratedApiCompatibilityPreserver (new), its wiring into CodeGeneratorOrchestrator.GenerateForToolAsync, and the supporting scraper/test changes. The regression-rollback and the new hardening tests (ApiCompatibilityPreserver_* in GeneratorHardeningTests.cs) look solid in isolation — type-change, optional→required, and required-member add/remove are all well covered with clear violation messages.
Two architectural concerns on the compatibility-preservation design itself:
1. Ordering hazard: Preserve runs before InheritedPropertyCollisionResolver.Resolve, so its "preserved" name isn't guaranteed final
In CodeGeneratorOrchestrator.GenerateForToolAsync:
var compatibleTool = GeneratedApiCompatibilityPreserver.Preserve(tool with { ... }, outputDirectory);
var toolDefinition = InheritedPropertyCollisionResolver.Resolve(
ExecutablePrerequisiteCatalog.PrepareForGeneration(compatibleTool));RestoreRequiredMemberNames mutates Options/PositionalArguments property names to match the baseline before InheritedPropertyCollisionResolver.Resolve runs. Resolve unconditionally renames any property whose name matches one of the seven reserved names on CommandLineToolOptions (Tool, CommandParts, Arguments, AdditionalArguments, ArgumentsContainToolOptions, ArgumentsContainOptionTerminator, RunSettings) — see InheritedPropertyCollisionResolver.ResolveName. If a restored/baseline name ever lands on one of those reserved names (e.g. a scraper fix renames a property such that its CLI-identity match now resolves to a baseline name of Arguments), Resolve will silently rename it again after Preserve already returned success with zero violations — quietly reintroducing exactly the kind of breaking rename this PR exists to prevent, with no test coverage of the composed pipeline (every ApiCompatibilityPreserver_* test calls Preserve and OptionsClassGenerator directly, never through Resolve).
Why this matters architecturally: the whole point of GeneratedApiCompatibilityPreserver is to make a guarantee ("this public property name will not change"), but two independent, mutually-unaware stages both own property renaming and run in an order where the later stage can undo the earlier stage's guarantee. That's a layering violation — a component that promises API stability shouldn't be upstream of another component that can still rename the API.
Suggested fix: either (a) run Preserve last, comparing the baseline against the fully-resolved (post-collision) property set so what it returns really is final, or (b) merge the two into a single naming-resolution pass that's aware of both the reserved-name constraint and the compatibility baseline, or at minimum (c) add an orchestrator-level (or paired) test that exercises Preserve followed by Resolve together, so a future regression here would actually be caught.
2. Baseline is derived by re-parsing the generator's own previous output via ad-hoc Roslyn syntax-tree scraping
GeneratedApiCompatibilityPreserver.ReadBaseline/ReadProperties re-parses the committed *.Generated.cs files with CSharpSyntaxTree.ParseText and walks RecordDeclarationSyntax/PropertyDeclarationSyntax nodes, inferring required-ness from constructor parameters vs. properties, and recovering CLI identity from CliArgument/CliOption/CliFlag attribute text. This makes the compatibility contract implicit in — and fully coupled to — the incidental shape of the generator's own emitted source text, rather than an explicit, versioned artifact.
Why this matters architecturally: any future change to how the generator formats or structures emitted code (a new attribute wrapper, an additional interface/base type, a different accessor style) can silently break baseline extraction with no compiler-level signal — the failure mode is "the preserver just stops seeing that property as part of the baseline," which is the opposite of what a compatibility gate should do on interpretation failure. This is effectively a hand-rolled, narrower version of the public-API-tracking pattern .NET already has established tooling for (Microsoft.CodeAnalysis.PublicApiAnalyzers's PublicAPI.Shipped.txt), which stores the contract as an explicit, diffable, structured file rather than deriving it by re-parsing generated source. Consider emitting a small explicit baseline manifest (e.g. JSON keyed by class/property with type + required + CLI-identity fields) as a generation byproduct and diffing against that, instead of syntax-tree-mining the .Generated.cs files. That decouples "what API did we promise" from "how does the generator currently choose to format its output," and would surface a parsing/shape mismatch as a loud, structured diff rather than a silent gap in coverage.
Everything else (the DockerCliScraper/CobraCliScraper NormalizeOptionSwitchName hook for docker compose exec --no-TTY, and the rollback of the nine published option files) looks like a straightforward, well-targeted fix.
Keep current constructor and optional-value APIs while extending baseline preservation to global and supplemental options.
|
Addressed every exact-head finding in
Validation:
The broad generator format gate remains blocked by unrelated pre-existing whitespace/info diagnostics (first @codex review exact head |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbe256f0c7
ℹ️ 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".
GitHub Actions
Inactive
|
Exact head 08f5dbc validation:
Pulumi and Flux solution builds each exceeded the repository's fixed 2 GB local agent guard (2376/2278 MB and 2452 MB respectively), so they were not retried; CI owns those expensive checks. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08f5dbc4bf
ℹ️ 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".
|
Addressed all three review findings in b8699c4: reject setter-breaking optional-to-required changes, restore required constructor contract order, and preserve former Deconstruct overloads across regeneration. Validation: GeneratorHardeningTests 72/72 passed; ModularPipelines.OptionsGenerator.slnx Release build passed with 0 warnings and 0 errors. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8699c461e
ℹ️ 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".
GitHub Actions
Inactive
|
Addressed the latest exact-head P1 and CodeFactor finding in 91f7247:
Validation:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91f724799c
ℹ️ 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".
|
Addressed shared-package facade ownership in a2e4b80. Validation: GeneratorHardeningTests 75/75 passed; ModularPipelines.OptionsGenerator.slnx Release build passed with 0 warnings and 0 errors. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2e4b807d4
ℹ️ 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".
GitHub Actions
Inactive
Track declaring type, method name, and options type so leaf commands retain named facades when they gain children and other method moves fail validation.
|
Addressed leaf-command facade moves in ded2814. Named facades are preserved when children appear, and compatibility validation now compares declaring type + method + options type. Hardening tests 76/76; Release build clean. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ded2814206
ℹ️ 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".
|
Addressed the latest exact-head P2 in Validation: @codex review exact head 5d4fea6 @claude review exact head 5d4fea6 and submit approval if clean. |
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d4fea6255
ℹ️ 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".
| /// <summary> | ||
| /// specify a path to config file for secret scanning (default "trivy-secret.yaml") | ||
| /// </summary> | ||
| [SecretValue] |
There was a problem hiding this comment.
When callers set SecretConfig, this marker registers the ordinary config-file path as secret material and redacts its occurrences from command diagnostics. Fresh evidence beyond the fixed private-key-path thread is that the exact tree still adds this marker to six Trivy option records even though their descriptions explicitly say “path to config file”; the Cobra scraper calls IsSecretOption without its available description, bypassing the path check. Pass the description through or otherwise classify --secret-config as a path so regenerated files omit these markers.
AGENTS.md reference: AGENTS.md:L164-L168
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 36da58a. Cobra secret classification now receives the parsed description, so GeneratorUtils recognizes --secret-config as a documented path. Regenerated Trivy 0.70.0 output removes [SecretValue] from all six affected records. The regeneration also preserves shipped command-group operands and passthrough positions. Trivy tests 11/11, Cosign tests 4/4; OptionsGenerator and Trivy Release builds are clean.
|
Fixed in 36da58a. Cobra secret classification now receives the parsed description, so GeneratorUtils recognizes |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 36da58ad83
ℹ️ 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".
| methods.Add(new GeneratedFacadeMethod( | ||
| declaringType, | ||
| method.Identifier.ValueText, | ||
| optionsType.TrimEnd('?'))); |
There was a problem hiding this comment.
When an existing command without required members later gains a required option or operand, BuildOptionsParameter changes its facade from FooOptions? options = null to required FooOptions options. This baseline identity strips nullability and records neither the default value nor parameter optionality, so RejectRemovedFacadeMethods considers the method unchanged even though existing calls such as FooAsync() no longer compile; preserve an overload/default alongside the parameterless compatibility options constructor.
AGENTS.md reference: AGENTS.md:L164-L168
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 14a6a28. Baseline facade parsing now records whether the options parameter had a default; matching commands preserve that optional contract even after gaining required members. Generated implementations use the already-preserved parameterless compatibility constructor when callers omit options, and alias facades share the same requirement decision. Regression passes; GeneratorHardeningTests pass 77/77; OptionsGenerator Release build is clean.
GitHub Actions
Inactive
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 14a6a28119
ℹ️ 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".
| Commands = compatibleTool.Commands | ||
| .Select(command => baseline.TryGetValue(command.ClassName, out var commandBaseline) | ||
| ? Preserve(command, commandBaseline.Properties, commandBaseline.Constructors) | ||
| : command) |
There was a problem hiding this comment.
When a command exposed through a public command-group alias gains a required member, this pass only applies the baseline keyed by the canonical command.ClassName; the corresponding alias baseline (for example, DockerBuilder*Options) is never processed. GenerateCompatibilityOptionsAlias then emits only the current required constructor, so existing calls to the alias's previous constructor stop compiling even though the canonical options record receives a compatibility constructor. Preserve alias constructor baselines as well.
AGENTS.md reference: AGENTS.md:L164-L168
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in fe4f6d2. Compatibility preservation now reads each generated command-group alias baseline, records alias-specific constructor history (including the generated base(...) primary constructor), maps aliased enum parameter types through the shared generator helper, and emits retained this(...) overloads on the alias record. Regressions cover parameterless-to-required and one-to-two required constructor evolution. GeneratorHardeningTests pass 79/79; OptionsGenerator Release build is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe4f6d2eeb
ℹ️ 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".
| PreserveCompatibilityConstructors( | ||
| aliasBaseline.Properties, | ||
| aliasBaseline.Constructors, | ||
| currentRequired, | ||
| compatibilityConstructors); |
There was a problem hiding this comment.
When an enum-valued option is removed or ceases to be recognized as an enum, this alias pass preserves only constructors. For example, a DockerBuilder*Options property currently typed with a DockerBuilder* enum disappears even though the canonical property is retained as an obsolete compatibility member with the incompatible DockerBuildx* enum type, breaking existing alias consumers. Validate and retain the alias record's declared enum wrapper properties and corresponding alias enum types as well as its constructors.
AGENTS.md reference: AGENTS.md:L164-L168
Useful? React with 👍 / 👎.
| var root = CSharpSyntaxTree.ParseText(File.ReadAllText(path)).GetRoot(); | ||
| foreach (var declaration in root.DescendantNodes().OfType<RecordDeclarationSyntax>()) | ||
| { | ||
| baseline[declaration.Identifier.ValueText] = new GeneratedApiBaseline( | ||
| ReadProperties(declaration), | ||
| ReadCompatibilityConstructors(declaration)); |
There was a problem hiding this comment.
When upgraded CLI help removes or renames one documented enum value while leaving the option itself intact, this baseline scan ignores the generated enum declaration and therefore accepts deletion of its public member. EnumGenerator then emits only the newly scraped values, so source using the old enum member no longer compiles even though the option record passes compatibility validation; include generated enum names, members, CLI values, and stable numeric values in the baseline check.
AGENTS.md reference: AGENTS.md:L164-L168
Useful? React with 👍 / 👎.
GitHub Actions
Inactive
1 participant
Summary
CliOptionValueshapesValidation
GeneratorHardeningTests: 60 passedCodeGeneratorOrchestratorTests: 16 passedDiraddition already merged in Keep Kustomize build path optional #3928Closes #3912