fix(generator): preserve gcloud 585 command coverage

[thomhurst]

Google Cloud SDK 585.0.0 help caused the generator to drop commands when wrapped option values looked like operands, nested option-only groups looked ambiguous, or inherited group prose looked like collection syntax. This fixes those cases in the scraper/shared parser and replays all 54 affected SDK fixtures. All previously failing fixtures now retain their commands. The handwritten ParseUsageSynopsis API change is tracked.

This also fixes the three core concurrency-test failures seen in this PR's Ubuntu pipeline (issue #5154). Intentionally blocking fixture workers now use dedicated LongRunning tasks. The disposal test releases/drains work even when startup fails and verifies the transition remains in flight. Existing timeout values and secret-registration/snapshot assertions remain intact.

Validation:

• 361 focused AWS/gcloud/shared-parser tests pass after rebasing onto main, including the 54 SDK fixtures.
• A one-worker thread pool reproduces both logging failures before the synchronization fix; all three affected tests pass afterward in 478 ms. No thread-pool setting is committed.
• All 89 related core disposal/secret-masking tests pass with normal runtime settings.
• Generator and core-test scoped severity-info formatting pass. Full Linux CI is rerunning with the fixes.

#5147 and #5150 are merged. Merge this source fix before fresh gcloud generation from main. Issue #5146 stays open until that complete generation succeeds; no generated options are patched here.

Closes #5154.

Refs #5146.

Summary by CodeRabbit

• New Features

  • Expanded Google Cloud CLI coverage for version 585.0.0 across Compute, Dataproc, IAM, Storage, Transfer, Spanner, configuration, and additional services.
  • Improved recognition of wrapped option values, positional arguments, boolean options, repeatable parameters, and complex command syntax.

• Tests

  • Added broader validation using captured Google Cloud command help and edge-case argument formats.
  • Improved test reliability for operations involving blocked or long-running workers.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 4b429fb0-058f-4842-9e5b-a74e30557cbd

📥 Commits

Reviewing files that changed from the base of the PR and between 363bc39 and 8d24e48.

📒 Files selected for processing (1)

• tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliScraperBase.cs

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.

---

📝 Walkthrough

Walkthrough

The change improves gcloud synopsis parsing, adds version 585.0.0 help fixtures with scraper coverage, and runs three blocking concurrency-test workers on dedicated long-running tasks.

Changes

CLI parsing and fixture coverage

Layer / File(s)	Summary
Synopsis parsing and option-shape validation tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/*, Models/CliOptionDefinition.cs, PublicAPI.Unshipped.txt, Scrapers/Cli/GcloudResourceArgumentTests.cs	Gcloud wrapped option values are normalized before synopsis parsing. Option-local descriptions now support shape validation. Tests cover command paths, glob repeatability, and multiword option values.
Gcloud 585.0.0 help fixtures tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Fixtures/Gcloud/585.0.0/*	Help-output fixtures were added for gcloud commands, including standard and preview configuration commands.
Blocking test worker scheduling test/ModularPipelines.UnitTests/Engine/ModuleSchedulerDisposalTests.cs, test/ModularPipelines.UnitTests/Logging/SecretMaskingPatternTests.cs	Three blocked workers now use TaskCreationOptions.LongRunning with TaskScheduler.Default. The disposal test releases and awaits its transition worker during cleanup.

Priority: ➖ Normal

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TestScraper
  participant GcloudCliScraper
  participant CliScraperBase
  participant UsageSynopsisParser
  TestScraper->>GcloudCliScraper: Parse fixture help text
  GcloudCliScraper->>GcloudCliScraper: Normalize wrapped ValueHint patterns
  GcloudCliScraper->>CliScraperBase: Parse cleaned synopsis
  CliScraperBase->>UsageSynopsisParser: Parse operands and option groups
  UsageSynopsisParser-->>TestScraper: Return command and option definitions

Loading

Merge Risk: ⚪ Minimal · up to 8d24e

The reviewed change does not introduce an identified user-facing or operational regression and is ready to merge after normal checks.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	The directly linked issue is #5154. It covers startup scheduling and cleanup for three core concurrency tests. The PR also changes the generator, shared parser, scraper API, gcloud parsing, and adds 5…	Remove the generator, parser, scraper API, fixture, and related generator-test changes from this PR, or directly link the issue that requires those coding changes and provide its scope.
Docstring Coverage	⚠️ Warning	Docstring coverage is 6.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 15 functions across 7 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the primary change: preserving Google Cloud SDK 585 command coverage in the generator.
Linked Issues check	✅ Passed	Issue #5154 requires blocking fixture workers to run independently of the shared thread pool. The three affected tests now use dedicated long-running tasks. The disposal test verifies that the transit…

Full details: Out of Scope Changes check

Explanation

The directly linked issue is #5154. It covers startup scheduling and cleanup for three core concurrency tests. The PR also changes the generator, shared parser, scraper API, gcloud parsing, and adds 585.0.0 fixtures and generator regression tests. The supplied evidence does not connect these changes to #5154. Reference to #5146 does not establish direct linked-issue scope.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch issue-5146-gcloud-coverage

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{A rabbit sees workers leave the crowded queue
Dedicated threads hold each gate anew
Gcloud words now find their proper shape
Fixtures guide each parsing escape
Tests wait, release, and finish true}

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-15T23:11:02.170968Z	8d24e48	New commits

ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b8c088cf70

ℹ️ 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]

Review

Reviewed the parser/generator source changes (the fixture .txt files are captured gcloud output, not hand-authored, so I focused on UsageSynopsisParser.cs, GcloudCliScraper.cs, CliScraperBase.cs, and CliOptionDefinition.cs).

Summary

This is a well-scoped, well-tested fix. The core changes:

• GetWrappedPrefixSuffix now tracks bracket/paren nesting depth instead of taking the first ], correctly handling nested optional groups (e.g. malformed-looking but valid nested [...[...]...] synopses).
• UsageRequiredAlternativeGroup/Member gain an IsRequired flag that's threaded end-to-end: parsed in ParseOptionalOperandBundle/TryParseNestedOperands, carried through TryResolveRequiredAlternativeGroup in CliScraperBase.cs (member-level IsRequired correctly aligned by index against group.Members), and consumed by both GeneratorUtils.cs:750 and OptionsClassGenerator.cs:469 — so the new field isn't a half-wired addition, it actually reaches codegen.
• CliOptionDefinition.ValueShapeDescription cleanly separates option-local prose (for shape/boolean-value validation) from the description used for docs, addressing the "inherited group prose" false-positive described in the PR.
• The new GcloudCliScraper.ParseUsageSynopsis override rewrites multi-word option-value placeholders (e.g. --buckets=BUCKET URI,[BUCKET URI,...]) to --buckets=VALUE before the shared parser runs, using Regex.Escape on all interpolated fixture-derived text, so no regex-injection/ReDoS concern there.
• Test coverage is strong: a data-driven regression test replays all 54 previously-broken fixtures end-to-end, plus targeted unit tests for the specific defects called out in the description (multi-word option values swallowing a real operand, group-condition prose not turning a scalar into a collection, required-group constructor wiring, group repeatability).

Minor, non-blocking notes

1. GcloudCliScraper.cs:794 — UsageSynopsisParser.GetOperandPropertyName(match.Groups["operand"].Value)! uses null-forgiving on a call that can legitimately return null (e.g. if ParseOperand ever decides the canonical name starts with -, or normalizes to empty). Today the ResourceOperandPattern regex effectively guarantees a safe input, but the two pieces of logic (regex shape vs. ParseOperand's own validity checks) live in different files and can drift independently. A ?? line.Trim() fallback (or a comment noting why the invariant holds) would make the coupling explicit instead of implicit and would avoid turning a future edge case into an unhandled NullReferenceException during scraping rather than a graceful "skip this line" like the rest of the parser does.
2. ResolveInlineAlternativeGroups (UsageSynopsisParser.cs) — this was simplified from "reparse with option context, then keep only original groups whose members SequenceEqual the reparse" down to just returning the reparse result directly. That's a real behavior change, not just a refactor: previously a mismatch between the coarse and option-aware parses caused the group to be dropped entirely; now the option-aware result is trusted unconditionally. Given the extensive fixture coverage this is presumably intentional and validated, but it's worth calling out explicitly in review since it removes a safety net rather than just deleting dead code — worth double-checking there isn't a synopsis shape where the coarse pass caught something the option-aware reparse now silently misses.

Neither point blocks merge; both are worth a quick look/acknowledgment rather than a required change.

[thomhurst]

Addresses the two non-blocking notes in #5152 (comment):

1. ResourceOperandPattern requires an uppercase operand name beginning with A-Z, optionally after a compound prefix. That excludes empty names and standalone option switches before GetOperandPropertyName is called. The captured compound-operand regressions cover this invariant. I kept the canonical property name instead of falling back to raw help text, which could introduce an invalid or inconsistent identifier.
2. Trusting the option-aware reparse is intentional. Commit 18444a7 on fix(generator): preserve gcloud argument contracts #5147 fixes the concrete [--verbose RESOURCE] case: the coarse parse associates RESOURCE with an unknown option, while resolved flag metadata proves it is a positional operand. Intersecting both parses discarded the required conditional constraint. The new regression plus 270 parser/resource tests pass.

Current remote head f8da14a includes the parent fixes and API tracking for the newly overridable ParseUsageSynopsis method. The generator solution Release build passes with zero warnings and errors.

[github-actions]

Review

Reviewed the parser/generator source changes; the Fixtures/Gcloud/585.0.0/*.txt files are captured gcloud --help output (test data, not hand-authored), so I focused on UsageSynopsisParser.cs, GcloudCliScraper.cs, CliScraperBase.cs, CliOptionDefinition.cs, and the new tests in GcloudResourceArgumentTests.cs.

Summary

This is a well-scoped, well-tested generator fix, consistent with the repo's "fix the scraper, never hand-edit generated output" constraint:

• UsageSynopsisParser.TryParseNestedOperandGroup now uses the bracket/paren-aware SplitTopLevelAlternatives instead of a naive content.Contains('|') / nestedTokens.Contains(":") check, so a | nested inside an inner [...]/(...) group no longer gets misread as a top-level alternative, and the "option-only bracket group" detection no longer requires a colon token to trigger. Both are real, narrowly-targeted bug fixes to malformed-looking-but-valid synopsis shapes.
• CliOptionDefinition.ValueShapeDescription cleanly separates option-local prose (used only for boolean/shape validation in CliScraperBase.ValidateOptionShapes) from Description (still used for generated docs, which legitimately includes inherited group context). This directly fixes the "inherited group prose mistaken for evidence of a repeatable scalar" defect called out in the PR description.
• The new GcloudCliScraper.ParseUsageSynopsis override rewrites multi-word option-value placeholders (e.g. --buckets=BUCKET URI,[BUCKET URI,...]) down to --buckets=VALUE in the SYNOPSIS text before delegating to the shared parser, so a wrapped/multi-token value grammar can no longer be mistaken for a real positional operand. All interpolated text goes through Regex.Escape, so there's no injection/ReDoS concern from fixture-derived content, and value hints are matched longest-first with DistinctBy to avoid a shorter hint's pattern clobbering a longer overlapping one.
• Test coverage is strong: Captured_585_Commands_Retain_Coverage replays all 54 previously-broken fixtures end-to-end, and two targeted tests (Group_Condition_Prose_Does_Not_Turn_Glob_Into_A_Collection, Multiword_Option_Value_Does_Not_Consume_A_Real_Operand) cover the specific defects from the description with minimal synthetic fixtures.
• PublicAPI.Unshipped.txt is correctly updated for the newly-virtual/overridden ParseUsageSynopsis — appropriate here since ModularPipelines.OptionsGenerator is the handwritten tool itself, not a generated CLI integration package.

Follow-up on the prior review

The previous automated review on this PR (posted at commit 8cc5283a) raised two non-blocking notes. Both turn out to be about code from the stacked parent PR (#5147: GetOperandPropertyName null-forgiving usage, and ResolveInlineAlternativeGroups trusting the option-aware reparse) rather than this PR's own diff. The author's reply explains why both are safe (the ResourceOperandPattern regex invariant, and the specific [--verbose RESOURCE] case fixed by trusting the reparse, backed by regression tests) without needing a code change, and the current head (f8da14a2) only adds the PublicAPI.Unshipped.txt tracking for the new overridable method on top of that — matching the author's description. Nothing further to address there.

Non-blocking observation

GcloudCliScraper.ParseUsageSynopsis fixes the multi-word-value-placeholder problem by mutating the SYNOPSIS text with regexes derived from the FLAGS section, then handing the rewritten text back to the shared tokenizer. That mirrors the existing NormalizeUsageSynopsis pre/post-processing pattern in this class, so it's consistent with the codebase, but the underlying concept (an option's declared value grammar can span multiple tokens and shouldn't be read as positional operands) isn't really gcloud-specific — other CLIs with wrapped multi-word value placeholders could hit the same problem later. If that turns out to be true, it may be worth eventually promoting "known value-hint patterns" into a parameter the shared UsageSynopsisParser accepts directly, rather than each CLI-specific scraper pre-mutating help text with its own regex pass. Not a reason to hold this PR up — the fix is correct and well-covered as-is.

CI is still in progress (several checks IN_PROGRESS at review time) with nothing failing so far; Fast fail, CodeRabbit, and security/snyk have passed.

[github-actions]

Review

Independently reviewed the parser/generator source changes at head 0576d3c6 (the Fixtures/Gcloud/585.0.0/*.txt files are captured gcloud --help output used as test data, so I focused on UsageSynopsisParser.cs, GcloudCliScraper.cs, CliScraperBase.cs, CliOptionDefinition.cs, and GcloudResourceArgumentTests.cs).

Summary

This is a well-scoped, well-tested generator fix that matches the repo's "fix the scraper, not the generated output" constraint. Traced through the actual logic (not just the diff) to confirm each change:

• GcloudCliScraper.ParseUsageSynopsis (new override) rewrites multi-word option-value placeholders (e.g. --buckets=BUCKET URI,[BUCKET URI,...]) down to --buckets=VALUE in the SYNOPSIS text before delegating to the shared parser via base.ParseUsageSynopsis. All interpolated text goes through Regex.Escape, values are matched longest-first (OrderByDescending(ValueHint.Length) + DistinctBy) so a shorter overlapping hint can't clobber a longer one, and I worked through the greedy-\s*/lookahead interaction by hand — it correctly stops consuming trailing whitespace before the following positional operand rather than swallowing it. ParseUsageSynopsis was correctly promoted to virtual and the base member exposed as its own to PublicAPI.Unshipped.txt, appropriate since ModularPipelines.OptionsGenerator is the handwritten tool itself, not a generated CLI package.
• UsageSynopsisParser.TryParseNestedOperandGroup: the option-only bracket group check no longer requires a : token (broadened to nestedTokens.Count > 1 && ContainsOnlyInlineOptions(...)), and the alternative-detection check now uses the bracket/paren-aware SplitTopLevelAlternatives(content).Count > 1 instead of a naive content.Contains('|'). Both are narrowly-targeted, verified against the existing (unchanged) ambiguous-colon-group validation that still runs afterward — no ordering regression there.
• CliOptionDefinition.ValueShapeDescription cleanly separates option-local prose (used only for boolean/shape validation in CliScraperBase.ValidateOptionShapes) from Description (still used for generated docs, which legitimately retains group context). This directly fixes the "inherited group prose mistaken for evidence of a repeatable/boolean scalar" defect, and explains why the affected commands were previously failing outright: ValidateOptionShapes throws, which is caught by the outer catch in CliScraperBase and silently drops the whole command.
• Test coverage is strong: Captured_585_Commands_Retain_Coverage replays all 54 previously-broken fixtures end-to-end, and Group_Condition_Prose_Does_Not_Turn_Glob_Into_A_Collection / Multiword_Option_Value_Does_Not_Consume_A_Real_Operand are minimal, targeted regressions for the two specific defects called out in the PR description.

Follow-up on prior review threads

Two earlier automated reviews on this PR (at 8cc5283a and f8da14a2) already raised and resolved their points — the two non-blocking notes turned out to be about code from the stacked parent PR (#5147), and the author's replies (backed by the ResourceOperandPattern invariant and the [--verbose RESOURCE] regression test) adequately address them without a code change. The current head only adds PublicAPI.Unshipped.txt tracking for the new overridable method on top of that, which I confirmed is present and correct. Nothing outstanding from those threads.

Non-blocking observation (unchanged from prior review, still applies)

The multi-word-value-placeholder fix is implemented as gcloud-specific text pre-mutation in GcloudCliScraper, mirroring the existing NormalizeUsageSynopsis pattern in this class. The underlying concept (an option's declared value grammar can span multiple tokens and shouldn't be read as positional operands) isn't inherently gcloud-specific, so other CLIs with wrapped multi-word value placeholders could hit the same problem later. If so, promoting "known value-hint patterns" into something the shared UsageSynopsisParser accepts directly (rather than each scraper pre-mutating help text) would avoid duplicating this trick per-CLI. Not a reason to hold this PR up.

CI: Fast fail, CodeRabbit, and security/snyk have passed; the remaining checks (analyzers, build matrix, full pipeline) are still in progress with nothing failing so far.

[github-actions]

Review

Re-reviewed at head 9026f0e9 independently of the transcript in prior comments. This head carries the same two commits/content already reviewed and marked CLEAR at 8cc5283a, f8da14a2, and 0576d3c6 (the diff, file list, and line-level changes in CliScraperBase.cs, GcloudCliScraper.cs, UsageSynopsisParser.cs, CliOptionDefinition.cs, and GcloudResourceArgumentTests.cs match verbatim -- the sha change looks like a rebase of the stacked base branch, not new work).

Verified against the actual code (not just the diff)

• UsageSynopsisParser.TryParseNestedOperandGroup's relaxed bracket check (dropping the nestedTokens.Contains(\":\") requirement, keeping ContainsOnlyInlineOptions): confirmed ContainsOnlyInlineOptions (UsageSynopsisParser.cs:1353) still requires all non-:/| tokens to be option switches via .All(...), not "any". A mixed bracket with a real positional placeholder alongside option switches still falls through correctly. No regression here -- matches what the prior review already verified.
• CliOptionDefinition.ValueShapeDescription: traced GcloudCliScraper.CreateOptionDefinition (line ~593-616) -- Description stays argument.Documentation (group+local joined, still used for docs/enum/numeric/structured-value detection), while ValueShapeDescription = argument.Description (local-only) is used solely inside ValidateOptionShapes for the boolean/repeatable canary checks. That's a real, deliberate narrowing shared by both the repeatable check (the one this PR's regression targets) and the explicit-boolean-value check (untouched by this PR's stated intent). Checked the current 585.0.0 fixture set for true or false/true|false/true/false phrasing: none of the new or existing fixtures exercise that branch, so this isn't a demonstrated regression, just a latent design coupling worth a follow-up: if a future gcloud flag declares its boolean value shape only in group-level cardinality prose rather than its own description line, the generator would silently accept it as a presence-only flag instead of throwing the intended InvalidOperationException. Not blocking -- purely hypothetical against current data -- but since the two checks now want different scopes of description for different reasons, splitting them onto distinct variables (or a comment noting why boolean-check narrowing is intentional too) would make the coupling explicit rather than incidental.

Minor, non-blocking polish (new, small enough not to hold up an autogenerated-options PR)

• GcloudResourceArgumentTests.Captured_585_Commands_Retain_Coverage derives commandPath via nameLine[..nameLine.IndexOf(\" - \", ...)] with no guard -- a future fixture whose NAME line doesn't contain " - " throws an ArgumentOutOfRangeException from string-slicing rather than a clear assertion failure. Not worth blocking on for a test helper, just a rough edge for the next person adding a fixture.
• GcloudCliScraper.ParseUsageSynopsis builds one Regex.Replace pattern per collected flag value-hint, per SYNOPSIS section, on every scrape -- fine at the sizes this runs at (this only runs during generation, not pipeline runtime), so not a real concern, just noting it doesn't memoize across structurally-identical value hints if this pattern gets reused elsewhere later.

On the design-scope question raised in earlier reviews

The recurring non-blocking observation that the multi-word-option-value fix is gcloud-specific (regex pre-mutation in GcloudCliScraper rather than a generic UsageSynopsisParser capability) still applies structurally, but it's been raised and accepted as reasonable scope for this fix in every prior review pass -- no new information changes that conclusion here.

Verdict

No actionable/blocking findings. The core fix (multi-word option-value grammar no longer swallowing real positional operands, plus the two narrower parser/validation corrections) is correct, well-isolated to GcloudCliScraper/UsageSynopsisParser/CliScraperBase, and backed by the 54-fixture regression plus two targeted unit tests. PublicAPI.Unshipped.txt is correctly updated for the newly-virtual ParseUsageSynopsis, consistent with ModularPipelines.OptionsGenerator being handwritten tooling rather than generated output.

[thomhurst]

Addresses the clarity notes in #5152 (comment). Commit 724659e adds a clear assertion before slicing fixture NAME lines and documents why both shape checks use option-local prose: inherited group descriptions can describe sibling values. All 54 SDK fixtures and scoped formatting pass. The potential shared-parser generalization and regex memoization remain future work without a demonstrated need in this scope.

These changes are included in verified remote head 363bc39, rebased onto main after #5147 merged. That head also fixes the separate core test scheduling failures from the latest Ubuntu run; details and reproduction are tracked in #5154. All 361 focused generator tests and 89 related core tests pass locally.

[github-actions]

Review

Re-reviewed at head 363bc39c. This head is a rebase onto main (after #5147 merged) plus one genuinely new commit not covered by any prior review pass on this PR: fix(tests): isolate blocking fixture workers (#5154), touching ModuleSchedulerDisposalTests.cs and SecretMaskingPatternTests.cs.

New commit: concurrency-test fix (#5154)

Traced the actual mechanics, not just the diff:

• Dispose_WhileStateTransitionIsInFlight_DoesNotThrow and the two SecretMaskingPatternTests cases previously used Task.Run(...) to run a worker that blocks synchronously (on a ManualResetEventSlim/similar gate) until the test releases it. On a constrained thread pool (e.g. CI runners with few cores), a blocking pool worker can starve the pool of the thread needed to run a second, dependent Task.Run continuation — a classic thread-pool-starvation deadlock. Switching these specific workers to Task.Factory.StartNew(..., TaskCreationOptions.LongRunning, TaskScheduler.Default) gives them a dedicated thread outside the pool, which is the correct, standard fix for this class of flake, and matches the PR description's claim of reproducing the failure on a one-worker pool and confirming the fix.
• Dispose_WhileStateTransitionIsInFlight_DoesNotThrow also tightens the assertion: it now asserts transitionTask.IsCompleted is false right before disposing (verifying the transition is genuinely still in flight, not just assumed), and asserts scheduler.Dispose() throws nothing via Assert.That(...).ThrowsNothing() instead of manually catching into a nullable Exception?. The scheduler's Dispose() is idempotent (Interlocked.Exchange guard in ModuleScheduler.cs), so the using var scheduler plus the explicit scheduler.Dispose() call inside the assertion is safe, not a double-dispose bug.
• Any exception thrown by transitionTask now propagates from the finally block's await transitionTask.WaitAsync(...) instead of being caught into a separate transitionException variable — behaviorally equivalent (test still fails on a transition-side exception), just via propagation. If the try block's assertion also fails, the finally's exception would mask it per normal .NET semantics, but that's a pre-existing pattern in this style of test and not specific to this change.

Follow-up items from the prior review (head 9026f0e9) — confirmed addressed

• GcloudResourceArgumentTests.Captured_585_Commands_Retain_Coverage now asserts separator (the " - " index) is > 0 before slicing the NAME line, replacing the previous unguarded nameLine[..nameLine.IndexOf(" - ", ...)] that could throw ArgumentOutOfRangeException on a malformed fixture.
• CliScraperBase.ValidateOptionShapes now has a comment explaining why both the boolean-value and repeatable-collection checks intentionally use the option-local ValueShapeDescription rather than the group-inclusive Description — the latent coupling flagged previously is now documented rather than incidental.

Both match the author's description of commit 724659ea66 and are visible in the current diff.

Non-blocking observation

The LongRunning conversion was applied to the three tests actually reproduced as flaky (per the PR description's one-worker-pool repro), but SecretMaskingPatternTests.StableSecretEmission_DoesNotHoldStateLockWhileWaitingForReaderLock (and a couple of neighboring tests) still start indefinitely-blocking work via plain Task.Run (e.g. the writer/contender tasks around lines 1618-1652). These have the same shape (block a pool thread on a gate while a second Task.Run is expected to make progress concurrently) and could theoretically hit the same starvation class of flake later. Not a reason to hold up this fix — it's scoped to the failures actually observed and reproduced — but worth keeping in mind if similar intermittent failures show up in that test in the future.

CI at this head is still in progress (full pipeline (ubuntu-latest), build (windows-latest/macos-latest), trim and Native AOT, Analyze (csharp), claude-review); nothing has failed so far (Fast fail, analyzers, CodeRabbit/Codex, security/snyk-equivalent checks already green).

Verdict: No actionable/blocking findings. The new concurrency fix is correct and well-targeted, and both outstanding notes from the previous review pass are properly resolved in code.

[github-actions]

Review

Re-reviewed at head 8d24e48e. This head adds exactly one new commit beyond the previously reviewed 363bc39c: refactor(generator): separate repeatable option validation, touching only CliScraperBase.cs.

New commit: extract-method refactor

Isolated the commit's diff and compared it line-for-line against the prior body of ValidateOptionShapes:

• The repeatable-option check (the !option.IsFlag && !isBoolean && HelpDeclaresRepeatableOption(...) && ... block) was moved verbatim into a new private ValidateRepeatableOption(command, option, helpText, description) method.
• The only textual change to the condition itself is !isBoolean → option.CSharpType is not ("bool" or "bool?"), which is the same predicate inlined (the local isBoolean variable is removed since it's now used in exactly one place).
• The boolean-value check and the description computation (option.ValueShapeDescription ?? option.Description ?? string.Empty, added and explained by a comment in the previously-reviewed 724659ea) are untouched.
• Both methods remain private, so this doesn't change the generator's public API — consistent with PublicAPI.Unshipped.txt having no changes in this commit.

This is a behavior-preserving extract-method refactor with no logic changes. No other file is touched by this commit, and all files/commits reviewed in the prior pass (gcloud fixtures, GcloudCliScraper.cs, UsageSynopsisParser.cs, CliOptionDefinition.cs, the concurrency-test fix, GcloudResourceArgumentTests.cs) are unchanged since that CLEAR verdict.

CI at this head is still in progress (most checks pending/queued); nothing has failed so far.

Verdict: No actionable/blocking findings.

[thomhurst]

The generator changes implement #5146: preserve all commands rejected by the SDK 585 scrape, including structured and multiword option values and option-local shape validation. That issue is referenced in the PR description and intentionally remains open until a fresh full generation verifies command coverage after this source fix reaches main. #5154 separately tracks the core test failures exposed by this PR's CI; commit 363bc39 fixes those reproduced failures.

For the docstring warning, the repository does not require XML comments on private helpers or test methods. The new private ValidateRepeatableOption helper in 8d24e48 names the validation it performs, and the caller documents the non-obvious option-local description rule. Adding boilerplate solely to meet the bot's percentage would not clarify the implementation. All 361 focused generator tests and scoped formatting pass; CodeFactor now passes as well.