Conversation
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. |
|
Caution CodeRabbit couldn't post its review summary. Error details |
💡 Codex ReviewWhen a caller sets AGENTS.md reference: AGENTS.md:L33-L33 ℹ️ 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". |
|
Validation update: supported Node 24.14.1 with yarn install --frozen-lockfile and yarn build passed (331 documents). All 54 previously skipped paths from #5146 are present; coverage includes 1,455 groups and zero exclusions, and the reported GROUP dispatch sentinels have no synthetic Group operand. Codex review completed without findings. CodeFactor currently publishes legacy status error (Something went wrong) although its public report says No issues found; it exposes no GitHub check run to re-request, and no authenticated browser is available here. This remains a merge blocker while the remaining CI/review checks run. |
|
The first Linux build identified a missing namespace import in my new handwritten |
|
Review generation is currently blocked by #5316, independently of this PR's source changes. The trusted diagnostic run failed before model execution and produced no review. Existing merge and review gates remain intact; no unchanged-failure rerun is planned until service/account evidence changes. |
|
Caution CodeRabbit couldn't post its review summary. Error details |
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThe gcloud CLI reference documentation adds rows for ChangesGcloud CLI reference
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The summary reports compliance with ✨ Finishing Touches 💡 1🛠️ 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 the command trail Comment |
💡 Codex ReviewWhen the new AGENTS.md reference: AGENTS.md:L33-L33 ℹ️ 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". |
There was a problem hiding this comment.
Claude Review
This PR regenerates ~7,951 Google gcloud CLI options/enums/docs and consolidates related runtime tests. Two previously reported, unaddressed defects remain in the generated output at the current head (604bd21): GcloudMemorystoreInstancesCreateTokenAuthUserOptions.TokenAuthUser is still incorrectly marked [SecretValue] (a resource name, not a token), and GcloudSqlInstancesCreateOptions's Validate() still flattens independent option groups (KMS/EntraId/autoscaling/Region-Zone) into one shared trigger, causing spurious validation failures for valid configurations. New handwritten tests (delimited-list, composite-value, resource-argument, and regeneration regression tests) are otherwise correct and consistent with the generated behavior they exercise, and the new gcloud.md doc rows match the generated type names.
Review evidence
src/ModularPipelines.Google/Options/GcloudMemorystoreInstancesCreateTokenAuthUserOptions.Generated.cs: Checked whether TokenAuthUser (a user/resource name per its own XML doc) is marked [SecretValue]; it still is at line 50 in the current head, so Command will redact this identifier and potentially mask unrelated matching output in logs.
src/ModularPipelines.Google/Options/GcloudMemorystoreInstancesTokenAuthUsersDeleteOptions.Generated.cs: Compared sibling token-auth-user commands' TokenAuthUser property; none of them apply [SecretValue] to this identifier, confirming the create-token-auth-user file is inconsistently classified versus its siblings.
src/ModularPipelines.Google/Options/GcloudSqlInstancesCreateOptions.Generated.cs: Inspected the generated Validate() method; the DiskEncryptionKey/EntraId 'required' conditions and the mutual-exclusion Region/Zone conditions all reuse one giant flattened OR across unrelated option groups (KMS, EntraId, autoscaling, Region, Zone), so setting only Region or an autoscaling flag now incorrectly forces DiskEncryptionKey/EntraId errors for otherwise-valid configurations.
src/ModularPipelines.Google/Options/GcloudDeveloperConnectConnectionsCreateOptions.Generated.cs: Verified as a contrasting correct example: its provider-requirement conditions correctly scope the 'must specify X' check to only fire when that group's own trigger predicate is true, proving independent-group scoping is achievable and making the SQL create flattening a generator regression rather than an inherent limitation.
test/ModularPipelines.Google.UnitTests/GcloudRegenerationTests.cs: New regression test Inline_Private_Key_Is_Secret_But_Resource_Identifiers_Are_Not asserts TokenAuthUser is not [SecretValue] on GcloudMemorystoreInstancesTokenAuthUsersAuthTokensDescribeOptions, but does not cover the newly generated create-token-auth-user command where the same property is still incorrectly tagged, so the regression suite doesn't catch the reported defect.
docs/docs/mp-packages/cli/gcloud.md: Confirmed the added app-topology, compute image-views, and memorystore token-auth-user documentation rows match the newly generated option type names exactly, with no stray or missing entries.
src/ModularPipelines.Google/Options/GcloudMemorystoreInstancesCreateTokenAuthUserOptions.Generated.cs:50 — TokenAuthUser is marked [SecretValue], but per its own XML doc ('The name of the token auth user to create') and every sibling token-auth-user command (e.g. GcloudMemorystoreInstancesTokenAuthUsersDeleteOptions.TokenAuthUser, GcloudMemorystoreInstancesTokenAuthUsersCreateAuthTokenOptions.TokenAuthUser), this is a resource/user name, not an authentication token. Command redacts [SecretValue]-tagged values from logs before execution, so this identifier will be hidden from diagnostics and could spuriously mask unrelated output that happens to match the same substring. This was already flagged by Codex on this same commit (604bd21) and remains unaddressed. Fix the generator/scraper's secret-classification heuristic (it appears to key off the word 'token' in the flag name '--token-auth-user' rather than the described semantics) so this property is generated without [SecretValue], then regenerate.
src/ModularPipelines.Google/Options/GcloudSqlInstancesCreateOptions.Generated.cs:866-892 — The generated Validate() method flattens several independent gcloud sql instances create option groups (Cloud KMS disk-encryption key, Entra ID app/tenant, read-pool autoscaling, and Region/Zone) into one shared 'any of these fields set' trigger condition. As a result, constructing new GcloudSqlInstancesCreateOptions("db") { Region = "us-central1" } (or setting only an autoscaling flag) now fails validation demanding DiskEncryptionKey, EntraIdApplicationId, and EntraIdTenantId even though those are unrelated, independent option groups per the gcloud reference. This was flagged by Codex as a P1 on commit 255d10a and is still present at the current head. Compare with GcloudDeveloperConnectConnectionsCreateOptions.Generated.cs in this same PR, whose analogous provider-requirement checks correctly scope each 'must be specified' condition to only that group's own trigger predicate — showing the generator can produce independently-scoped group validation and that this file's flattening is a regression to fix in the scraper/generator (preserve nested subgroup boundaries) and then regenerate, not a hand-edit to this generated file.
Optional follow-up notes
The new GcloudRegenerationTests.Inline_Private_Key_Is_Secret_But_Resource_Identifiers_Are_Not test is a good pattern for guarding secret classification; once the generator fix above lands, consider extending that test (or adding a parallel case) to assert GcloudMemorystoreInstancesCreateTokenAuthUserOptions.TokenAuthUser is also not [SecretValue], so a future regression in this specific command is caught automatically.
Given the large blast radius of the flattened-group validation bug (any command with multiple independent gcloud arg groups could be affected, not just SQL create), it may be worth a follow-up audit across other regenerated Options files for the same flattening pattern rather than assuming SQL create is the only instance.
|
Closing this generated snapshot for confirmed SQL group-validation and token-auth-user secret-classification defects, tracked in #5180. Fixing source separately before fresh main regeneration; handwritten runtime tests will be retained in the replacement. |
|
Source fix is ready in #5328 at 2211b1e. The captured SQL runtime regression reproduces the spurious Region requirements before the fix; all ten valid/invalid combinations now pass. Secret-name regressions also fail before the fix and pass afterward. Generator Release build is clean and changed files are formatted. Wider suites are delegated to CI after the mandatory 2 GB guard stopped them; limits were not raised. Closed #5315, cancelled its active CI (including force-cancellation when the initial request did not stop it), and verified no pending runs remain on its branch. Its handwritten Google test patch is preserved for the fresh replacement after this source fix merges. |
|
Fresh replacement: #5338, confirmed remote head 34bc46d. It applies the unmodified Google SDK 585 artifact from latest-main run 35666140726 after source fixes through #5337, with the handwritten Google regression tests. Coverage includes 7,951 commands and 1,455 groups with no removals or exclusions; docs and freshness checks pass. Explicit Google integration validation is running at https://github.com/thomhurst/ModularPipelines/actions/runs/35669471943. Acceptance findings remain open until runtime CI and review complete. |
|
Fresh replacement: #5341, generated unchanged from main 1be7211 by https://github.com/thomhurst/ModularPipelines/actions/runs/35676360503 after source fix #5340. Provenance and coverage pass: 7,951 commands, 1,455 groups, 24 additions, no removals/exclusions, all 54 reported omissions present. The Google library and documentation build; dedicated integration CI and reviews are pending in https://github.com/thomhurst/ModularPipelines/actions/runs/35678558349. This closed PR remains superseded. |
|
Fresh latest-main replacement: #5344, generated by run 35684123650 after source fixes through #5343. Artifact 10676728248 is unchanged and passes provenance, coverage, and documentation checks. The replacement contains 76 nested-choice runtime cases plus the earlier regression suites. Actual Google integration CI and reviews are pending; acceptance issues remain open until those pass. |
|
Fresh latest-main replacement: #5347, generated by run 35690354313 after source fixes through #5346. Artifact 10678711892 is unchanged and passes provenance, coverage, and documentation checks. Compared with rejected #5344, only eight embedded-credential SecretValue annotations and the fingerprint change; no masking annotation is removed. The replacement retains 76 nested-choice cases and adds runtime attribute checks for all eight credential fields. Explicit integration CI: https://github.com/thomhurst/ModularPipelines/actions/runs/35692851347. Acceptance remains pending actual Google test execution and reviews. |
Refresh Google Cloud SDK 585 options from the unmodified
generated-gcloud-patchartifact of run 35531729515, generated from main commit 3ae83d9 after source fixes #5181 and #5256. Coverage rises from 7,927 to 7,951 commands, with 24 additions and no removals or parse-failure diagnostics. Recorded generator fingerprint: b618ebba785210b052052b4a76870121813d7b3099c74e76c95b99501321ed98.The fresh API requires resource operands in four existing rendering fixtures. Supply those operands and retain their argument assertions. Consolidate the upload-source, delimited-list, and resource-selector runtime tests from #4863, #4869, and #4956, adapting list fixtures to current required resource constructors. Add runtime regressions for command-specific enum values, conditional provider requirements, secret-version resource values, and secret classification.
Generated options/services/enums/docs are exactly the workflow artifact; no generated files or API baselines were hand-edited.
git apply --reverse --checkverified the artifact was unchanged after scoped formatting.Validation: the workflow generated and built the Google library, then reported only the four existing CS7036 test-constructor errors addressed here. Handwritten tests are formatted. Full Google tests are delegated to CI because the earlier local Google build exceeded the mandatory 2 GB guard; the limit was not raised or retried. With supported Node 24.14.1, yarn install --frozen-lockfile and yarn build both passed; Docusaurus generated 331 documents.
Closes #5178
Refs #5180, #5146, #5166, #4850, #4849, #4799, #4797, #4474, #4331, #5009.
Supersedes runtime-test work in #4863, #4869, and #4956; replaces closed generated snapshots #5179 and #5255.
Summary by CodeRabbit
app-topologycommand family.