Harden generated command group discovery

[thomhurst]

Summary

• keep Cobra section-header matching on one physical line so all-word Cosign command tables are parsed
• require a real Usage declaration and plural Commands heading before treating help as a command tree
• add regression fixtures for Cosign bundle, Pulumi env run, and Snyk test

Validation

• full OptionsGenerator tests: 818/818 passed
• OptionsGenerator Release build: 0 warnings, 0 errors
• touched-file whitespace formatting: clean
• reproduced against official Cosign v3.1.3 help

Fixes #3990
Part of #3996

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[coderabbitai]

Warning

Review limit reached

Next included review available in 48 minutes.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bcf93f16-2319-42d1-a305-bf356ffdb9af

📥 Commits

Reviewing files that changed from the base of the PR and between 9c23b33 and d82e48d.

📒 Files selected for processing (6)

• tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/Cli/CliScraperTraversalTests.cs
• tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/CosignCliScraperTests.cs
• tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/PulumiCliScraperTests.cs
• tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/SnykCliScraperTests.cs
• tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliScraperBase.cs
• tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CobraCliScraper.cs

---

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

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

[greptile-apps]

Greptile Summary

The PR hardens CLI command-group discovery while restoring support for multiline Cobra usage declarations.

• Requires a genuine Usage declaration and plural Commands heading before validating a command tree.
• Keeps Cobra section-header matching within one physical line so all-word Cosign command tables remain parseable.
• Adds regression coverage for Cosign, Pulumi, Snyk, and shared traversal behavior.

Confidence Score: 5/5

The PR appears safe to merge.

No blocking failure remains.

Important Files Changed

Filename	Overview
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliScraperBase.cs	Tightens command-group classification around Usage declarations and plural Commands headings while supporting multiline usage synopses.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CobraCliScraper.cs	Restricts section-header whitespace matching to spaces and tabs so command-table content cannot be swallowed across physical lines.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/Cli/CliScraperTraversalTests.cs	Adds coverage for multiline group usage and warning emission when declared groups produce no subcommands.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/CosignCliScraperTests.cs	Adds regression fixtures for multi-row and single-row all-word Cosign command tables.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/PulumiCliScraperTests.cs	Verifies that Pulumi command operands are not misclassified as command groups.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/SnykCliScraperTests.cs	Verifies that descriptive and example text containing command syntax does not create a false command group.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    H[CLI help text] --> U{Usage contains command placeholder?}
    H --> C{Plural Commands heading exists?}
    U -->|Yes| G{Both conditions true?}
    C -->|Yes| G
    U -->|No| L[Handle as leaf command]
    C -->|No| L
    G -->|Yes| E[Extract and traverse subcommands]
    G -->|No| L
    E --> O[Generate typed command options]
    L --> O

Loading

_{Reviews (5): Last reviewed commit: "fix(generator): parse multiline group us..." | Re-trigger Greptile}

[claude]

Code review

No blocking issues found. The regex changes are narrowly scoped and each is backed by a regression test that reproduces the exact failure mode from #3990:

• CommandGroupUsagePattern (CliScraperBase.cs#L1000-L1003): making the Usage: prefix mandatory (it was previously optional) stops the pattern from matching any line that merely contains the literal text <command>/[command], e.g. pulumi env run <environment-name> -- <command> [args] or snyk <command> -- [...]. That's exactly what caused the false-positive "declares a command group" classification for pulumi env run and snyk test.
• CommandSectionHeadingPattern (same file, line 1005): requiring the plural COMMANDS (was COMMANDS?) is a reasonable extra guard. No existing fixture relies on a singular COMMAND: heading, so this narrowing looks safe.
• CobraCliScraper.SectionHeaderPattern (CobraCliScraper.cs#L947-L950): swapping \s for [ \t] in the colon-terminated alternative keeps the "next section" search on one physical line. Previously, with IgnoreCase + [\w\s]*:, a lowercase row like create Create a Sigstore protobuf bundle could satisfy [A-Z] case-insensitively at the start, and \s would then happily consume newlines while hunting for the next : — in Cosign's all-word command table this swallowed the whole table before reaching the real next heading (Flags:), causing the section slice to misalign and subcommands to silently vanish. Restricting to same-line whitespace fixes that cleanly.

One non-blocking follow-up: issue #3990 also lists cosign signing-config and cosign trusted-root as affected by the same all-word command-table layout as cosign bundle, but only bundle gets a regression fixture here. Since the fix lives in the shared parser rather than a Cosign-specific adapter it should cover those too, but a fixture per affected subcommand would make that explicit and guard the shared regex against future regressions.

Architectural note: this is not the first time a shared regex meant to serve many CLI dialects (Cobra, Cosign, Pulumi, Snyk, ...) has needed a targeted tweak because a fix motivated by one tool's help-text quirk risked affecting another's — this PR alone touches behavior shared by three unrelated tools to fix one. The per-tool regression-fixture discipline used here is a good mitigation, but as more CLIs get onboarded, a small explicit-state parser (e.g. a line-by-line state machine tracking "in Usage section" / "in Commands section") would likely be easier to reason about and extend safely than continuing to layer exceptions onto a handful of increasingly specific shared regexes. Worth considering if this class of cross-tool regression keeps recurring.

[thomhurst]

Added the review's missing Cosign signing-config and rusted-root fixtures in 37c6bd0, using their exact failed workflow help layout. Both single-row all-word command tables now explicitly assert discovery of create. Validation: OptionsGenerator tests 820/820; Release build 0 warnings/errors.

[claude]

Code review (follow-up)

The previous follow-up is addressed: Extracts_Cosign_V3_Single_Row_All_Word_Command_Tables (commit 37c6bd0) now covers both cosign signing-config and cosign trusted-root with their exact single-row all-word command-table layout, asserting create is discovered for each. That closes the gap from issue #3990 cleanly.

Two small non-blocking observations on the fixes as they stand now:

• CommandGroupUsagePattern narrowing (CliScraperBase.cs#L1000): making Usage: mandatory now also restricts the match to the Usage: line plus exactly one following line. A command whose synopsis prints two lines under one Usage: block (e.g. Usage:\n app [flags]\n app [command] [flags]) would no longer be detected as declaring a command group by this pattern alone — verified this divergence directly against old vs. new pattern behavior. No currently-supported tool's fixtures exercise this shape, so it's latent rather than a live bug, but it's worth a regression fixture precisely because of the next point.
• No test exercises the safety-net throw path in ValidateSubcommandDiscovery (CliScraperBase.cs#L434). Every test added across this PR (Cosign, Pulumi, Snyk) asserts the "not a command group" / "subcommands still extracted" side. None asserts that a genuine command-group help text with zero extracted subcommands still throws InvalidOperationException. Since this PR narrows the two ANDed detector regexes twice now in response to false positives, adding one fixture that pins the "true positive still throws" behavior would guard against a future narrowing pass silently failing closed (detector says "no group", extraction is skipped, no exception) instead of failing loud.

Neither blocks merge — both are about locking in the other direction of this detector (avoiding false negatives) now that two rounds of fixes have tightened it against false positives. The per-tool fixture discipline in this PR is good; extending it to a "still detects a real group" case for the narrowed patterns would round it out.

[thomhurst]

Rebased onto current main (13fd094). Validation: Cosign 7/7; Pulumi 4/4; Snyk 18/18; OptionsGenerator Release build clean.

[claude]

Code review (follow-up)

No new changes to review — this update is a rebase onto current main (13fd094b55) only. Diffed the five PR files against the previously-reviewed commit (37c6bd0e58) and they are byte-identical; the rebase pulled in unrelated upstream commits (e.g. registry-credential masking in GeneratorUtils) but didn't touch this PR's actual changes.

Prior review stands: the narrowed CommandGroupUsagePattern/CommandSectionHeadingPattern/Cobra SectionHeaderPattern regexes are each backed by a regression fixture reproducing the original false positive, and the two earlier follow-up items (missing Cosign signing-config/trusted-root fixtures) were already resolved in 37c6bd0e58.

Two previously-raised non-blocking observations remain open (not required for merge, just flagged again in case they get lost across the rebase):

• CommandGroupUsagePattern now only matches a Usage: line plus exactly one following line, so a two-line synopsis under one Usage: block would be missed. No current fixture exercises this shape.
• No test pins the "true positive still throws" path in ValidateSubcommandDiscovery — every fixture in this PR asserts the false-positive side, none asserts a genuine command-group help text with zero extracted subcommands still throws.

Architectural note (repeated from before, still relevant): this shared regex-based parser has now needed three separate narrowing passes across Cobra/Cosign/Pulumi/Snyk to fix cross-tool false positives. As more CLI dialects get onboarded, a small explicit-state line-by-line parser (tracking "in Usage section" / "in Commands section" as distinct states) would likely scale better than continuing to layer exceptions onto a handful of increasingly specific shared regexes — worth considering if this pattern of fixes keeps recurring.

[thomhurst]

Addressed both non-blocking follow-ups in a46a2b4fcf:

• command-group detection now accepts a placeholder on the second synopsis line while remaining bounded to at most two lines after Usage;
• the genuine empty-command-group traversal test now asserts the caught InvalidOperationException validation warning, pinning the fail-closed path.

Validation: CliScraperTraversalTests 20/20, Cosign 7/7, Pulumi 4/4, Snyk 18/18; scoped whitespace clean.

@claude review exact head a46a2b4fcf9050ed01c3e98f50cbc6b6a958204b.

[claude]

Code review (follow-up)

Reviewed exact head a46a2b4fcf9050ed01c3e98f50cbc6b6a958204b as requested. Both non-blocking observations from the previous round are addressed:

• CommandGroupUsagePattern two-line synopsis gap (CliScraperBase.cs#L997-L1003): the pattern now allows {0,2} full lines between Usage: and the line containing <command>/[command], so it stays bounded rather than scanning arbitrarily far into the help text. The new SharedTraversal_Detects_Command_On_Second_Usage_Line fixture (fake [flags] / fake <command> [flags] under one Usage: block) pins this. Checked that the wider bound doesn't reopen the Pulumi env run false positive from earlier in this PR — that fixture stays safe because it's the separate CommandSectionHeadingPattern AND-condition (no Commands: heading in that help text) that excludes it, not this regex, so the two changes are independent.
• Fail-closed path now pinned: SharedTraversal_Skips_Invalid_Group_And_Continues_With_Sibling now uses a RecordingLogger to assert the InvalidOperationException warning ("Failed to validate subcommand discovery: fake parent") is actually raised and caught in CliScraperBase (CliScraperBase.cs#L390-L398) for a genuine empty command-group. That locks in the "true positive still throws" behavior that was previously unverified.

No new issues in this delta. The architectural note from earlier still stands (regex narrowing has needed several rounds across Cobra/Cosign/Pulumi/Snyk; a small explicit-state parser would likely scale better as more CLI dialects are onboarded) — repeating it only for visibility, not blocking. Nothing else to flag.

[thomhurst]

Validated new head a46a2b4 after the multiline Usage parsing fix: CliScraperTraversalTests passed 20/20; OptionsGenerator Release build succeeded with 0 warnings/errors; commit diff check clean.

[thomhurst]

Rebased onto current main after #4023/#3985 and force-pushed with an exact lease. Validation: traversal, Cosign, Pulumi, and Snyk suites passed (49/49); OptionsGenerator Release build succeeded with 0 warnings/errors; diff checks clean. Verified remote head: d82e48d.

[claude]

Code review

Reviewed the diff (5 files, regex fixes in CliScraperBase.cs / CobraCliScraper.cs + regression tests). No prior Claude comment or unresolved bot findings to reconcile against — codex/coderabbit hit their rate limits with no content, and greptile's automated pass gave a 5/5 with no blocking issues.

Assessment: solid, well-targeted fix. I traced through why each regex change matters rather than taking the PR description at face value:

• CommandSectionHeaderPattern in CobraCliScraper.cs previously used [A-Z][\w\s]*: — because \s matches newlines, this could greedily swallow multiple physical lines when scanning for the next section header after Available Commands:. For Cosign's all-word command tables (create, inspect, upgrade — no punctuation breaking the \w/\s run), the match would jump ahead to Flags:, collapsing the extracted section to empty and silently dropping every subcommand. Restricting the class to [\w \t]* (space/tab only) confines the match to one physical line, which is what the docstring ("Matches section headers like 'Flags:', 'Usage:'") always intended. Good catch, and the regression test (Extracts_Cosign_V3_All_Word_Command_Table) pins the actual failure mode rather than just the surface symptom.
• CommandGroupUsagePattern making Usage:? mandatory (instead of optional) correctly stops any line containing <command>/[command] from being treated as a command-group signal on its own — this is what let pulumi env run <environment-name> -- <command> [args] false-positive. The {0,2} intervening-line allowance is a deliberate, tested bound: it's wide enough to cover Cobra's multiline usage synopses (see SharedTraversal_Detects_Command_On_Second_Usage_Line) but tight enough to still reject the Snyk case where a <command> mention shows up 3 lines into unrelated prose (Command_Help_Does_Not_Treat_Examples_As_A_Command_Group). Worth knowing this is a hand-tuned magic number rather than a structural guarantee — if a future CLI's usage block runs 3+ lines before the command placeholder, this will need bumping again. Not asking for a change here, just flagging it since the next person hitting this will want to know why "2" was chosen.

One reuse suggestion (non-blocking): CliScraperTraversalTests.cs adds a private RecordingLogger : ILogger nested class to capture warning log calls. OptionTypeEnhancerTests.cs already has an almost identical RecordingLogger<T> : ILogger<T> (same BeginScope/IsEnabled stubs, same "only capture Warning-level" filter, differing only in whether it stores plain messages vs. (message, exception) tuples). There's no shared Testing/fakes location in this test project yet, so extracting one now — parameterized on whether you need the exception — would let both tests share it and prevent a third copy from appearing next time someone needs to assert on logged warnings. Small thing, but two near-duplicate hand-rolled ILogger fakes is the kind of drift that's cheap to fix now and annoying to reconcile later.

No correctness or CLAUDE.md-compliance issues found otherwise. The regex changes are narrowly scoped, each is backed by a fixture reproducing the real-world failure it fixes, and the PR's own validation (818/818 OptionsGenerator tests, clean Release build) lines up with what the diff should do.

[thomhurst]

CI triage: the Ubuntu pipeline failure is an unrelated timeout in RunReportTests.DistributedMasterReconcilesMatchedMetricsPerWorker after 15m30s. The generator changes are outside that test, and the Windows build plus all generator-adjacent checks passed. The workflow still has a queued macOS job, so GitHub does not yet allow rerunning failed jobs; I will rerun the failed job once the run completes.

[thomhurst]

Workflow is now terminal; re-ran the unrelated Ubuntu timeout once (run 32669378123, attempt 2). No branch mutation.