fix(generator): scope gcloud argument group descriptions - #4870
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. |
|
Authoritative Linux gcloud generation is queued at https://github.com/thomhurst/ModularPipelines/actions/runs/34771969139. Expected output branch: automated/update-cli-options-gcloud-issue-4799-gcloud-group-descriptions. All 1,354 generator tests pass; generated output and Google integration validation remain required before merge. |
|
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 (3)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe Gcloud parser now detects argument groups from indentation and uses generated regular expressions. Tests cover enum, sibling, same-indentation, nested, and unrelated option parsing. New fixtures document Metastore migration and storage diagnostic commands. ChangesGcloud scraper parsing
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to No actionable merge-blocking issue is established. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 4.17% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 3 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 reads each line, Comment |
ReviewBlocking: the diff does not compile. In private static readonly HashSet<string> CSharpKeywords =
[
with(StringComparer.Ordinal),
"abstract", "as", "base", ...
];
Suggested fix: keep the comparer by reverting to object-creation syntax instead of forcing a collection-expression conversion here: private static readonly HashSet<string> CSharpKeywords = new(StringComparer.Ordinal)
{
"abstract", "as", "base", ...
};Collection expressions are a nice simplification when there's no comparer/capacity to pass (see the Discrepancy with the PR description: the description states "All 32 focused group-parser tests and all 1,354 generator tests pass. Generator solution Release build: zero warnings/errors," but the head commit as checked out does not compile. Please re-verify the build/test run was actually against this commit before merge. The Once the |
Greptile SummaryThis PR improves gcloud help parsing so plain-text argument-group introductions remain scoped to their groups and Python-style, multi-word default annotations are recognized correctly.
Confidence Score: 5/5The reviewed source and test changes appear safe to merge once the PR’s separately required authoritative generated-output validation is completed. No actionable new defect or repository-rule violation was found. The latest regex change preserves option value capture while accepting genuine multi-word default annotations, and the added tests exercise the affected type and description boundaries. Focused tests could not be rerun in this environment because PowerShell is unavailable, but this does not establish a code issue.
|
| Filename | Overview |
|---|---|
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliArgumentGroupParser.cs | Uses prose indentation when constructing argument-group hierarchy so sibling and nested group descriptions do not leak into root options. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/GcloudCliScraper.cs | Recognizes multi-word gcloud default annotations and moves equivalent parsing expressions to generated regex methods. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/Cli/GcloudGroupDescriptionTests.cs | Adds regression coverage for authoritative same-indentation groups, nested context isolation, enum defaults, and generated XML documentation. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Fixtures/Gcloud/metastore-services-migrations-start.txt | Adds authoritative Linux gcloud help containing sibling Hive and Iceberg group introductions. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Fixtures/Gcloud/storage-diagnose.txt | Adds gcloud help containing Python enum default representations and nested argument groups. |
Reviews (5): Last reviewed commit: "fix(generator): parse gcloud enum defaul..." | Re-trigger Greptile
|
The compiler claim in issuecomment-5654919275 is incorrect for this repository: global.json pins SDK 10.0.401 and Directory.Build.props enables LangVersion=preview, which supports comparer-bearing collection expressions. The generator solution compiled locally on 923abaf with zero warnings/errors and all 1,354 tests passed (archived TRX). The same shared source compiled in Linux CI under SDK 10.0.401 at https://github.com/thomhurst/ModularPipelines/actions/runs/34771665246/job/103762311459. No syntax rewrite is needed. Authoritative gcloud generation remains queued at run 34771969139; generated output and integration validation still block merge. |
|
Pushed 7e58658 after the final generated-output audit found #4877 still leaking both Hive and Iceberg introductions into Async and unrelated flags. Provenance matched the source, so the original fixture had missed real formatting rather than generation using stale code. Captured unmodified All 33 group-parser tests and all 1,355 generator tests pass; the Release solution build has zero warnings/errors. Scoped formatting and diff checks pass. This also reconfirms that the collection-expression compiler concern in review 5654919275 does not apply to the pinned SDK and preview language configuration. Replacement authoritative generation has been dispatched from 7e58658 with auto-merge and coverage-shrinkage approval disabled. #4877 and this parent remain blocked until corrected output is reviewed and validated. |
GitHub Actions
Failure
Code reviewSummary: This PR fixes the gcloud argument-group parser so that plain-text group introductions at the same indentation as their flags (e.g. "Configuration for Hive migration.") are recognized as group boundaries, preventing that prose from leaking into sibling/unrelated option descriptions. It adds solid regression coverage (synthetic sibling/nested cases plus an authoritative fixture captured from real Blocking:
private static readonly HashSet<string> CSharpKeywords =
[
with(StringComparer.Ordinal),
"abstract", "as", "base", ...
];This is unchanged from the previous review of this PR (flagged at commit The PR's own comments assert this compiles under the pinned SDK/preview LangVersion and cite a passing local build and CI run, but that claim doesn't hold up against the actual C# grammar, and it doesn't matter which SDK is pinned — no shipped or preview C# version supports comparer arguments inside private static readonly HashSet<string> CSharpKeywords = new(StringComparer.Ordinal)
{
"abstract", "as", "base", ...
};(The Everything else looks solid:
No CLAUDE.md violations otherwise: the changes are confined to the generator/scraper's generic logic and its tests (no hand-edited generated option files), consistent with "fix the scraper or generator ... never hand-edit generated options." Note: I wasn't able to execute a local build/test run in this review environment to get a compiler-verified confirmation of the |
|
The current-head Ubuntu pipeline failed in RunReportOmitsOutputWhenTimedOutEnricherCanRegisterSecretsLater with TimeoutException at the existing outer test guard. Failed job: https://github.com/thomhurst/ModularPipelines/actions/runs/34790257718/job/103813476167. This is the shared RunReportTests guard addressed by #4678, which is currently awaiting CI. No CI rerun was requested; sync the test fix after it lands. This failure does not identify a generator regression. |
7e58658 to
629a0d0
Compare
|
Pushed the rebase onto main b9e1a0f as 629a0d0. The old Ubuntu job failed only in RunReportOmitsOutputWhenTimedOutEnricherCanRegisterSecretsLater at its outer three-second guard; this branch now includes merged #4678. All 144 RunReportTests pass, as do all 1,396 generator tests. Generator Release build has zero warnings/errors, and scoped severity-info formatting and diff checks pass under the unchanged guard limits. Range-diff preserves both parser fixes. Main already contains the safe-await template and generated-branch isolation, so the obsolete extra workflow environment overrides were removed. GeneratorUtils is no longer changed by this PR; its comparer-bearing collection expressions compile under the pinned SDK as demonstrated by the successful build and tests. Fresh CI/review will evaluate the new head. Generation remains blocked by #4644/#4638, matching the existing baseline audit in #4877 comment 5657672069. I verified the orphaned-marker condition and recorded the native #4799 blocked-by #4638 dependency. Another expensive gcloud refresh is deferred until that baseline fix is part of the generation source. Generated files were not edited, and no failed CI was rerun. |
![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: 629a0d01c1
ℹ️ 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".
| var firstPreludeStartsGroup = StartsArgumentGroup(firstPreludeLines, firstPrelude) | ||
| || firstPreludeIndentation <= declarations[0].Argument.Indentation; | ||
| var root = new ArgumentGroupBuilder( | ||
| declarations.Min(declaration => declaration.Argument.Indentation), | ||
| Math.Min(firstPreludeIndentation, declarations.Min(declaration => declaration.Argument.Indentation)), |
There was a problem hiding this comment.
This parser change is not reflected in the committed generated output: in this commit, src/ModularPipelines.Google/Options/GcloudMetastoreServicesMigrationsStartOptions.Generated.cs still prefixes Async and the other unrelated properties with both the Hive and Iceberg descriptions, so consumers continue to receive the behavior this change is intended to fix. Regenerate and commit the affected gcloud options together with the scraper update, as required for generator changes.
AGENTS.md reference: AGENTS.md:L33-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The baseline prerequisite #4644 is now merged. Rebased onto main 2e2ddab and pushed b3c750f. Authoritative replacement gcloud generation is running at https://github.com/thomhurst/ModularPipelines/actions/runs/34802649779 with auto-merge disabled. Keeping this finding open until generated output is incorporated and validated; the old #4877 output must be refreshed.
There was a problem hiding this comment.
The replacement output in #4926 exposed an additional enum-default header defect. Fixed in 0f63058; all 1,432 generator tests pass. Fresh authoritative generation: https://github.com/thomhurst/ModularPipelines/actions/runs/34808204420. Keeping this finding open until corrected generated output is incorporated and validated.
There was a problem hiding this comment.
Corrected full output from #4926 is incorporated at cd0f2b1 and passed Google integration checks. Subsequent review identified a separate narrative-aside grouping issue, now fixed in 160e06d with four before-fix failures and all 1,437 generator tests passing. Final-source Gcloud generation: https://github.com/thomhurst/ModularPipelines/actions/runs/34824921392. Keeping this thread open until new output is incorporated and validated.
629a0d0 to
b3c750f
Compare
|
The baseline prerequisite #4638/#4644 has landed. Rebased the unchanged parser patches onto main 2e2ddab (also including #4688 timeout reporting) and pushed b3c750f. Range-diff confirms all four patches are unchanged. The Release generator solution builds with zero warnings/errors; all 41 focused argument-group tests, scoped severity-info formatting verification, all 61 package baseline checks, and diff checks pass under the default guard limits. Authoritative replacement gcloud generation is now running from this source at https://github.com/thomhurst/ModularPipelines/actions/runs/34802649779, with auto-merge and coverage-shrinkage approval disabled. This addresses the prerequisite to the current Codex finding 4001787137; the finding remains open until the generated output is incorporated and validated. #4877 must be refreshed before it is eligible for merge. No generated files were hand-edited. |
b3c750f to
0f63058
Compare
|
Manual review of the authoritative #4926 output found a remaining header defect: Fixed in 0f63058. Default annotations are now treated as display text, preserving each header as a declaration. An unmodified SDK 550.0.0 Windows storage-diagnose fixture reproduces the same syntax visible in SDK 584.0.0 generation; its regression failed before the fix and now verifies restored strategy flags, string paths, integer counts, and exact descriptions. All 1,432 generator tests pass; Release build has zero warnings/errors. Scoped severity-info formatting verification and all 61 API baseline checks pass under unchanged resource guards. The four existing patches are unchanged after rebasing onto main c66bb4e. Replacement authoritative generation is running from the new source at https://github.com/thomhurst/ModularPipelines/actions/runs/34808204420 with auto-merge and coverage-shrinkage approval disabled. Keeping the generated-output finding open until the replacement is reviewed and incorporated. #4926 must not merge its current output; generated options and baselines were not manually patched. |
ReviewReviewed the current head ( Prior blocking issue is resolved / no longer applicable. Earlier rounds (commits Core fix looks correct. Traced both logic changes by hand against the new authoritative fixtures and the existing test suite:
Not verified by execution. I wasn't able to run No actionable findings. The change stays within the stated scope (scraper/generator generic logic only, regression tests use unmodified real |
Co-authored-by: thomhurst <[email protected]>
|
Too many files changed for review (1664 files, 100 file limit). |
ReviewReviewed the current head ( Actionable: the new same-indentation heuristic can evict an already-classified flag out of its mutually-exclusive group.
var preludeStartsGroup = StartsArgumentGroup(preludeLines, prelude)
|| preludeIndentation <= declaration.Argument.Indentation;Tracing This doesn't reproduce against any current fixture — the two new tests ( Why this is an architectural gap, not just a missing test case: the indentation-equality heuristic can't distinguish "this prose introduces a new named subsection" (should evict/create a sibling) from "this prose is a mid-group aside" (should just extend the current group's description) — both look identical from indentation alone. A sturdier signal would be to only let the indentation-based path evict the current frame when the current frame doesn't already carry a non- Everything else holds up:
|
|
Fixed the narrative-aside finding in comment 5661323520 with 160e06d. Four regressions reproduce same-indentation prose splitting AtMostOne, AtLeastOne, Alternative, and Resource groups. The parser now checks the containing frame after indentation unwinding and permits an indentation-only sibling only for an unclassified frame. Explicit headings still start siblings. All 1,437 generator tests pass, including all 47 group tests and the existing authoritative Hive/Iceberg and enum-default fixtures. Release build has zero warnings/errors; scoped info verification and diff checks pass. Fresh full Gcloud generation: https://github.com/thomhurst/ModularPipelines/actions/runs/34824921392. The prior #4926 output is incorporated, but this new source requires final provenance before merge. |
1 participant
Gcloud group introductions and unrecognized flag headers could leak into unrelated option descriptions and type inference. Hive/Iceberg migration context appeared on --async; storage diagnostic paths and numeric counts incorrectly became enums from adjacent transfer-strategy flags.
Plain group introductions at their flags' indentation now keep sibling context separate. The parser treats default annotations as display text, including enum representations containing spaces, so download/upload headers remain declarations. Their descriptions and enum values stay with their own options. #4926 incorporated full authoritative output from source 0f63058, including clean LogsPath/ProcessCount/ThreadCount values and separate DownloadType/UploadType properties.
The latest review follow-up preserves classified argument groups across narrative asides. After moving to the containing group, indentation alone can start a sibling only when that group has no classification. Explicit headings can still start a sibling. This preserves AtMostOne, AtLeastOne, Alternative, and Resource membership without regressing plain Hive/Iceberg sections.
Regression coverage includes nested/sibling groups, narrative asides, explicit replacement headings, exact descriptions, option types, and generated XML documentation. Captured Linux SDK 584.0.0 migration help and Windows SDK 550.0.0 storage diagnostic help remain unchanged.
Validation at 160e06d:
Full Gcloud generation from this exact commit: https://github.com/thomhurst/ModularPipelines/actions/runs/34824921392. Source fingerprint: 8b78e4fe7cbe37504736e6adc1a6cd6c63331f0df244f57ce30804d384bd369f. Auto-merge and coverage-shrinkage approval are disabled. No generated options or baselines were hand-edited.
Refs #4799. Final-source output remains outstanding.