fix(generator): preserve gcloud groups and credential metadata - #5334
Conversation
|
Understand this PR’s impact Explore downstream dependencies and potential security impact with Blast Radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe parser preserves alternative resource branches, separates nested resource groups, and applies documented groups per parsed argument group. New Gcloud 585 fixtures cover two commands. Tests validate authentication alternatives and required database option groups. ChangesGcloud parser and validation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Medium 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation Issue Resolution Regenerate gcloud from the SDK 585.0.0 artifact after this generator fix. Apply the generated output in a replacement for
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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. A rabbit checks each option line Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Review
This PR fixes two gcloud CLI generator regressions: (1) mutually-exclusive authentication branches (API key / OAuth2 / user-password) were being merged into one flattened group instead of kept as independent alternative branches, and (2) an Oracle Database required-choice group was truncating a nested secret-version resource. The fix touches CliArgumentGroupParser.BeginArgumentGroup (extends the same-depth indentation bump to resource prose sitting inside an at-least/at-most-one choice) and UsageSynopsisParser.GetOptionalResourceOptionGroups (splits selectors on top-level | into whole branches before extracting switches, rather than flattening each token individually), plus a widened NamedBundleHeadingPattern regex to recognize "config for" and "parameters to support" headings.
Review evidence
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/UsageSynopsisParser.cs: Verified GetOptionalResourceOptionGroups now splits selectors after ':' on top-level '|' into whole alternative branches (via SplitTopLevelAlternatives) before tokenizing and extracting switches, instead of flattening every token individually; hand-traced against both new UsageSynopsisParserTests cases and confirmed the yielded switch sets match the asserted branch groupings, including nested explicitly-optional resource sub-groups.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliArgumentGroupParser.cs: Verified BeginArgumentGroup's indentation-bump condition now also triggers when a Resource-classified prelude sits inside an at-least-one/at-most-one choice (not just inside a named bundle), which prevents the choice group from prematurely ending before a same-depth secret-version resource; also verified the NamedBundleHeadingPattern regex extension ('config for', 'parameters to support') matches the new Oracle Database fixture headings.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/GcloudPeerResourceValidationTests.cs: New regression tests scrape the two captured fixtures and validate generated runtime option classes reject/accept the expected property combinations for independent auth branches and for the required database bundle retaining its nested secret-version resource; structure matches the established sibling test files (e.g. GcloudIndependentGroupValidationTests) in the same partial class.
No actionable findings.
Optional follow-up notes
The fix to GetOptionalResourceOptionGroups is well covered by two new UsageSynopsisParserTests (branch independence and nested explicit-optional resources), and I traced both test cases by hand through SplitTopLevelAlternatives/TokenizeOptionGroup/EnumerateInlineOptionSwitches — the yielded sets match the asserted expectations exactly.
CliArgumentGroupParser is a heuristic, indentation-based parser with no dedicated unit-test file; it's validated indirectly through captured real CLI fixtures (consistent with the existing pattern in this repo, e.g. GcloudCapturedSemanticsTests). The new fixtures (apihub plugins instances create, oracle-database db-systems create) combined with GcloudPeerResourceValidationTests exercise the exact regressions described, and the PR description states these fixtures failed before the fix and pass afterward — I was not able to independently execute the test suite in this review environment to re-confirm.
The description notes a broader local test run hit the 2 GB memory guard (exit 137) and defers full validation to CI, consistent with AGENTS.md guidance on the sandbox memory limit; this is expected and not a defect to flag.
Follow-up work (regenerating gcloud output and opening a fresh replacement PR for the closed #5333) is explicitly scoped as out-of-band per the PR description, so it's not something to request here.
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. |
There was a problem hiding this comment.
Claude Review
This PR (now at 3f282d5, after the earlier commit e3447c0 which a prior Claude review already cleared) adds a new confirmation step, not present when this PR was last reviewed: GcloudCliScraper now re-derives optional-resource groups from the synopsis a second time per FLAGS section, passing the section's freshly-parsed CliArgumentGroup as a 'documentedGroups' hint into UsageSynopsisParser.GetOptionalResourceOptionGroups. When a colon-selector alternative branch (split on top-level '|') has more than one flag, the parser now only keeps it as one whole optional branch if some documented CliArgumentGroup's flattened switch names exactly match; otherwise it falls back to treating each flag in that branch as independently optional. I traced this new logic and the surrounding call sites in GcloudCliScraper.cs and UsageSynopsisParser.cs, checked it against both new gcloud 585 fixtures/tests and the two prior review passes, and looked at FlattenArguments/MarkOptionalResourceGroups to understand how the confirmation result feeds into final option-group marking.
Review evidence
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/UsageSynopsisParser.cs: Verified the new documentedGroups parameter and its confirmation branch (lines ~1372-1422): when a pipe-split alternative selector has multiple flags, it now checks EnumerateArgumentGroups(documentedGroups) for an exact FlattenArguments SwitchName match before keeping the branch whole, falling back to per-flag optional sets otherwise; confirmed this branch is reachable only when documentedGroups is non-null, which none of the direct unit tests in UsageSynopsisParserTests.cs exercise.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/GcloudCliScraper.cs: Verified ParseArguments now performs a second GetOptionalResourceOptionGroups pass per FLAGS section using that section's own parsedGroup as documentedGroups, feeding the result into MarkOptionalResourceGroups; traced that mismatches against an unrelated section's parsedGroup are benign since MarkOptionalResourceGroups only acts on switch sets that match arguments actually present in that section's own tree.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/GcloudPeerResourceValidationTests.cs: Confirmed these new tests scrape real captured gcloud 585 fixtures end-to-end (via GcloudCapturedSemanticsTests.Scrape, not a mocked parser) and validate generated runtime option classes accept/reject the expected flag combinations for the apihub auth branches and the Oracle DB required bundle; confirmed neither test's fixture forces the new documentedGroups mismatch/fallback path to execute, since the apihub auth branches are flat leaf groups that match exactly.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/UsageSynopsisParserTests.cs: Verified both new tests call GetOptionalResourceOptionGroups with a single synopsis argument only (documentedGroups defaults to null), so they validate the pre-existing raw branch-splitting behavior (already cleared in the prior review) but do not exercise the new confirmation/fallback logic added since that review.
The new confirmation fallback added since the last review (tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/UsageSynopsisParser.cs:1406-1422, wired in from tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/GcloudCliScraper.cs:315-317) is completely unexercised by any test. Both new UsageSynopsisParserTests cases call GetOptionalResourceOptionGroups with no documentedGroups argument (so documentedGroups is not null is always false there, skipping the new branch entirely), and the only two callers that pass documentedGroups (via the real GcloudCliScraper pipeline in GcloudPeerResourceValidationTests.cs) only exercise the fixture where the SetEquals confirmation succeeds (the apihub auth-config branches, which are flat 2-3 flag leaf groups with no further nesting). No test forces EnumerateArgumentGroups(documentedGroups).Any(...) to fail and drive the fallback path that splits a branch into independently-optional single flags. Since this fallback silently changes the outcome (an alternative branch that should be validated as an all-or-nothing bundle becomes a set of independently optional flags) rather than surfacing a failure, an undetected bug in this new safety net could quietly produce incorrect required/optional validation for future generated commands without any test catching it. Add a unit test (e.g., in UsageSynopsisParserTests.cs, constructing a synopsis plus a documentedGroups list that deliberately does not match one of the pipe-separated branches) asserting the fallback yields the expected per-flag switch sets, and/or a fixture-based case in GcloudPeerResourceValidationTests.cs that forces a documented-group mismatch, so this new heuristic is locked in by a test rather than only validated by hand-tracing.
Optional follow-up notes
GetOptionalResourceOptionGroups now has two different behaviors depending on whether documentedGroups is null (raw synopsis-only parsing, used for CliArgumentGroupParser's own group-boundary inference at line 312) or non-null (confirmation-gated parsing, used only for final optional-marking at line 316-317). Splitting this into two explicitly named methods (e.g., a raw GetOptionalResourceOptionGroups plus a ConfirmOptionalResourceOptionGroups(rawGroups, documentedGroups) post-processing step) would make the two responsibilities and their different call sites easier to reason about than threading an optional parameter through the recursive Visit closure that only changes behavior in one specific colon+pipe branch.
GcloudCliScraper.ParseArguments recomputes UsageSynopsisParser.GetOptionalResourceOptionGroups(usage.Synopsis, [parsedGroup]) from scratch for every one of the three FLAGS sections (REQUIRED FLAGS, OPTIONAL FLAGS, POSITIONAL ARGUMENTS), re-walking the entire synopsis each time even though only one section typically contains the relevant colon-selector bundle. This is functionally harmless today (a mismatch against an unrelated section's parsedGroup just produces candidate groups that don't match anything in that section, so MarkOptionalResourceGroups ignores them), but it's redundant work and slightly obscures that the confirmation is effectively single-section-scoped; documenting that scoping assumption (or restricting the recomputation to sections likely to contain the bundle) would make the intent clearer for future maintainers.
The inline confirmation check at UsageSynopsisParser.cs:1409-1411 (EnumerateArgumentGroups(documentedGroups).Any(group => switches.SetEquals(...))) does not require group.Kind.HasFlag(CliArgumentGroupKind.AtLeastOne) the way the sibling MatchesDocumentedOptionGroup helper (used by IsDocumentedOptionGroup) does. This may be intentional since the two helpers serve different purposes, but the asymmetry between two very similarly-named 'does a documented group confirm these switches' checks is worth a comment or consolidation if the difference is deliberate, to avoid a future refactor accidentally unifying them and changing behavior.
|
Addressed the review-body coverage request in e69d577, confirmed in the remote PR head. The existing captured regression |
There was a problem hiding this comment.
Claude Review
This fix restores independent mutually-exclusive authentication branches and preserves required nested secret-version resources inside gcloud choice groups, correcting two generator regressions from closed PR #5333. The previously BLOCKING review finding (an untested documentedGroups confirmation/fallback path in UsageSynopsisParser) is now covered by a new targeted unit test added at the current head; I traced that test's logic against the implementation and confirmed it exercises both the fallback-split and pass-through branches as intended, with no remaining actionable concerns in the diff.
Review evidence
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/UsageSynopsisParser.cs: GetOptionalResourceOptionGroups now splits colon-selectors on top-level '|' into whole alternative branches, and (via the new documentedGroups parameter) falls back to per-flag splitting only when no documented group's flattened switches exactly match an unbracketed multi-flag branch. Hand-traced against both prior tests and the new test; logic is unchanged from the previously-reviewed commit 3f282d5.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliArgumentGroupParser.cs: BeginArgumentGroup's indentation-bump condition now also triggers for a Resource-classified prelude nested inside an at-least/at-most-one choice, preventing the choice group from closing prematurely before a same-depth secret-version resource; unchanged since the prior review that already validated it against the Oracle DB fixture.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/UsageSynopsisParserTests.cs: New test Optional_Common_Selectors_Remain_Independent_Of_Nested_Alternatives constructs a documentedGroups tree that does NOT match the flat 3-flag branch (--directory/--certificate/--token), forcing the previously-untested confirmation-mismatch fallback to run and split it into independent single-flag sets, while a separate nested bracketed branch (--username/--password) still passes through the confirmation-skip (token count == 1) and yields the whole pair plus its own required --password selector. This closes the gap the prior BLOCKING review identified where the fallback path had zero test coverage.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/GcloudPeerResourceValidationTests.cs: Scrapes the two new captured 585.0.0 fixtures end-to-end and validates the generated runtime option class enforces independent auth branches for apihub and retains the DbHomeVersion-gated database bundle members for oracle-database, matching the regressions described in the PR.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Fixtures/Gcloud/585.0.0/gcloud-apihub-plugins-instances-create.txt: New captured real gcloud help fixture reproducing the merged-authentication-branch regression from closed PR #5333; used as scrape input for the new regression test, not hand-written.
No actionable findings.
Optional follow-up notes
The PR fully addresses the prior BLOCKING automated review at commit 3f282d5, which flagged that the new documentedGroups confirmation/fallback branch in UsageSynopsisParser.GetOptionalResourceOptionGroups (used to disambiguate an unbracketed multi-flag selector from a documented group) had zero test coverage. The new test Optional_Common_Selectors_Remain_Independent_Of_Nested_Alternatives, added at head e69d577, constructs a documentedGroups tree that deliberately does not match a flat multi-flag branch, driving the fallback path and asserting its per-flag output, while a sibling nested-bracket branch exercises the pass-through path.
CodeRabbit's optional follow-up suggestions (splitting GetOptionalResourceOptionGroups into a raw-parse method plus a separate confirmation/ConfirmOptionalResourceOptionGroups post-processing step, and documenting that GcloudCliScraper's per-FLAGS-section recomputation of documentedGroups is effectively single-section-scoped) are reasonable readability improvements but are non-blocking; they can be picked up as separate follow-up cleanup rather than in this fix PR.
CodeRabbit's 'Linked Issues' and 'Docstring Coverage' pre-merge warnings are expected and already accounted for by the PR description: this PR is explicitly scoped as the source-only generator prerequisite for #5333 (regeneration/replacement PR is a separate follow-up), and the repo's documented convention (AGENTS.md / Directory.Build.props preview-C# style) does not require doc comments on internal parser helpers.
thomhurst
changed the title
There was a problem hiding this comment.
Claude Review
This PR fixes two gcloud generator regressions: mutually-exclusive auth branches (API key/OAuth2/user-password) being merged into one group instead of independent alternatives, and an Oracle Database required-choice group truncating a nested secret-version resource; it also stops flagging ordinary API-key HTTP-location metadata as a secret. I independently traced the UsageSynopsisParser branch-splitting/confirmation logic and the GeneratorUtils secret-classification logic (including a substring-collision edge case between 'ApiKey' and 'Location' suffixes) against the current pr-head source, and confirmed the code is unchanged since a prior automated review pass already cleared it (including remediation of an earlier BLOCKING finding about untested fallback logic, now covered by a new unit test).
Review evidence
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/UsageSynopsisParser.cs: Confirmed GetOptionalResourceOptionGroups now splits colon-selectors on top-level '|' into whole branches (SplitTopLevelAlternatives) rather than per-token, and gates whole-branch retention on the new documentedGroups confirmation, falling back to per-flag splitting only when no documented group's flattened switches exactly match a multi-flag branch. Read current pr-head source (lines 1372-1430) and it matches the diff exactly, so no drift since the last CLEAR automated review at e69d577.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Generators/GeneratorUtils.cs: Hand-traced IsSecretOption/IsSecretMetadataOption for the new 'Location' suffix branch against tricky cases: 'ApiKeyConfigHttpElementLocation' contains the substring 'ApiKey' (a SecretKeyword) but is correctly excluded from secret status because IsSecretMetadataOption's Location-suffix check runs and short-circuits before the later hasSecretKeyword substring check is reached; verified 'SecretLocation'/'PrivateKeyLocation' still classify as secret via the SecretKeywords.EndsWith/Contains paths since DescriptionIdentifiesSecretValue matches their descriptions, consistent with all new GeneratorUtilsTests arguments.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliArgumentGroupParser.cs: Verified BeginArgumentGroup's indentation-bump condition now also fires when a Resource-classified prelude sits inside an at-least/at-most-one choice (resourceWithinChoice), not only inside a named bundle, preventing the choice group from closing before a same-depth secret-version resource; this heuristic parser has no dedicated unit tests and is validated only via the two new captured 585.0.0 fixtures plus GcloudPeerResourceValidationTests, consistent with the existing repo pattern for this file.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/GcloudPeerResourceValidationTests.cs: Confirmed this new file is a valid partial of the existing RequiredConstructorValidationTests class (sibling files GcloudIndependentGroupValidationTests.cs etc. already extend the same partial class), scrapes the two real captured fixtures end-to-end, and asserts both valid and invalid property combinations for the independent auth branches and the required database bundle, directly covering both described regressions.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/UsageSynopsisParserTests.cs: Verified the new Optional_Common_Selectors_Remain_Independent_Of_Nested_Alternatives test exercises the previously-untested documentedGroups mismatch fallback path (flagged BLOCKING in an earlier automated review at 3f282d5), closing that coverage gap; the diff content here is unchanged from the already-CLEARed head e69d577.
No actionable findings.
Optional follow-up notes
The diff at the current review head (425362e) is byte-identical in scope to what a prior automated review pass already cleared at e69d577, including the fix for its own earlier BLOCKING finding (untested documentedGroups fallback path); no new commits changed the reviewed code since then.
As noted in earlier passes, GetOptionalResourceOptionGroups now has two distinct behaviors gated on whether documentedGroups is null (raw parse, used for CliArgumentGroupParser's own boundary inference) vs non-null (confirmation-gated parse, used for final optional-marking in GcloudCliScraper). Splitting this into two explicitly named methods would make the two call sites and responsibilities easier to follow than threading an optional parameter through the recursive Visit closure — worth considering as non-blocking follow-up cleanup, not required for this fix.
GcloudCliScraper.ParseArguments recomputes GetOptionalResourceOptionGroups(usage.Synopsis, [parsedGroup]) once per FLAGS section (REQUIRED/OPTIONAL/POSITIONAL), redundantly re-walking the full synopsis each time even though only one section typically contains the relevant colon-selector bundle. This is functionally harmless (mismatches against an unrelated section's parsedGroup are ignored by MarkOptionalResourceGroups) but is wasted work; documenting or narrowing the recomputation scope would clarify intent for future maintainers.
This PR is explicitly scoped as the source-only generator prerequisite for the closed #5333 gcloud SDK 585 regeneration; per AGENTS.md and the PR description, no generated CLI options or PublicAPI baselines are touched here, and that is correctly out of scope for this review.
1 participant
Google SDK 585 output in closed #5333 merges API Hub authentication branches, truncates an Oracle Database required choice before its secret-version resource, and registers ordinary API-key HTTP locations such as query/header/body as secrets. These errors reject valid configurations and mask unrelated diagnostics. #4799 and #5180 track the captured help and failure evidence.
The shared group parser now recognizes configuration/parameter branch headings and retains same-depth resource children inside their containing choice. Optional-selector inference preserves whole alternatives when the parsed help tree confirms their boundaries, while retaining common optional selectors outside nested choices. The shared secret classifier treats location metadata as public while preserving documented credential content and explicit secret overrides.
Captured SDK 585 fixtures test independent authentication methods, incomplete credentials, mutually exclusive methods, Oracle inline/secret-version inputs, and API-key location metadata. Synopsis tests cover branch membership, explicit optional nesting, and common-selector fallback. Existing provider runtime cases verify optional HTTP selectors remain optional.
Validation: all 3,328 generator tests passed at 425362e (run 35637459596); 249 focused classifier/enhancer/captured-help tests also pass. Both grouping regressions and the API-key location case failed before their fixes. The affected generator solution was formatted and git diff --check passes. A broader local combined run reached the mandated 2 GB guard (exit 137); full validation runs in CI without increasing that limit.
This is the source-only prerequisite. After merging, regenerate gcloud from latest main and apply the authoritative workflow artifact to a fresh replacement for #5333, preserving and extending the handwritten Google runtime tests. No generated output or generated API baselines are edited here. The issues and generated-output finding remain open until the replacement passes validation.