Skip to content

Preserve explicit Azure CLI Boolean values - #4008

Merged
thomhurst merged 6 commits into
mainfrom
issue-3989-az-bool-values
Aug 24, 2026
Merged

Preserve explicit Azure CLI Boolean values#4008
thomhurst merged 6 commits into
mainfrom
issue-3989-az-bool-values

Conversation

@thomhurst

Copy link
Copy Markdown
Owner

Summary

  • detect Azure CLI descriptions that require explicit Boolean values before classifying presence-only flags
  • emit affected options as value-taking bool? while preserving ordinary flags
  • add a regression fixture from az eventhubs namespace create

Validation

  • Az scraper regression: 1/1 passed
  • full OptionsGenerator tests: 817/817 passed
  • OptionsGenerator Release build: 0 warnings, 0 errors
  • touched-file whitespace formatting: clean

Fixes #3989
Part of #3996

@chatgpt-codex-connector

Copy link
Copy Markdown

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

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 25 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: 5f4d15f8-9a64-46d2-a46e-f5b6ba5239c8

📥 Commits

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

📒 Files selected for processing (7)
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/AzCliScraperTests.cs
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/WinGetCliScraperTests.cs
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/TypeDetection/DescriptionEnumValueParserTests.cs
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/AzCliScraper.cs
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliScraperBase.cs
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/WinGetCliScraper.cs
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/TypeDetection/DescriptionEnumValueParser.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

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

@greptile-apps

greptile-apps Bot commented Aug 23, 2026

Copy link
Copy Markdown

Greptile Summary

The PR refines CLI help-text interpretation so Azure options requiring explicit Boolean values remain value-taking while ordinary flags retain presence-only behavior.

  • Recognizes comma-separated Azure Boolean choices and distinguishes scalar values from Boolean collections.
  • Prevents WinGet options with explicit contextual choices from being inferred as flags.
  • Adds scraper and parser regression coverage for the affected help-text forms.

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/AzCliScraper.cs Separates explicit scalar Boolean values, Boolean collections, and presence-only flags during Azure option parsing.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliScraperBase.cs Expands explicit Boolean detection to recognize Azure-style allowed-value descriptions.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/WinGetCliScraper.cs Uses parsed contextual choices to prevent value-taking WinGet options from being classified as flags.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/TypeDetection/DescriptionEnumValueParser.cs Adds preference wording to contextual parenthesized enum detection.
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/AzCliScraperTests.cs Adds regression coverage for scalar, list, tri-state, and unrelated-repeatability Boolean descriptions.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
    Help[CLI help text] --> Scraper[CLI scraper]
    Scraper --> Bool{Explicit Boolean values?}
    Bool -->|Yes, scalar| NullableBool[Value-taking bool?]
    Bool -->|Yes, repeatable| BoolCollection[Boolean value collection]
    Bool -->|No| FlagCheck{Presence-only flag?}
    FlagCheck -->|Yes| Flag[CliFlag]
    FlagCheck -->|No| EnumParser[Description enum parser]
    EnumParser --> Generated[Generated option type and attribute]
Loading

Reviews (9): Last reviewed commit: "fix(generator): preserve WinGet mode val..." | Re-trigger Greptile

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown

Code review

Reviewed the fix for #3989 (AzCliScraper now preserving explicit Boolean values instead of collapsing them to presence-only flags). The core change follows the same HelpDeclaresExplicitBooleanValue pattern already used by CobraCliScraper/NbgvCliScraper, and the new regression test correctly locks in the az eventhubs namespace create --disable-local-auth case.

Suggestion: gate the explicit-Boolean promotion behind the existing type/list detection

/// <summary>
/// Determines the C# type based on value hint and description.
/// </summary>
private static string DetermineType(
string valueHint,
string description,
bool isFlag,
bool explicitBooleanValue)
{
if (isFlag || explicitBooleanValue)
{
return "bool?";
}
var lowerHint = valueHint.ToLowerInvariant();
var lowerDesc = description.ToLowerInvariant();
// Check for numeric types
if (lowerHint.Contains("number") || lowerHint.Contains("count") ||
lowerHint.Contains("port") || lowerHint.Contains("size") ||
lowerHint.Contains("timeout") || int.TryParse(valueHint, out _))
{
return "int?";
}
// Check for list types (space-separated or multiple values)
if (lowerDesc.Contains("space-separated") || lowerDesc.Contains("list of") ||
lowerDesc.Contains("multiple"))
{
return "IEnumerable<string>?";
}
return "string?";
}

DetermineType now short-circuits to bool? whenever explicitBooleanValue is true, before the existing "space-separated list of…" check runs:

if (isFlag || explicitBooleanValue)
{
    return "bool?";
}
...
if (lowerDesc.Contains("space-separated") || lowerDesc.Contains("list of") || ...)
{
    return "IEnumerable<string>?";
}

ExplicitBooleanValuePattern matches on the literal text true or false anywhere in the description, with no requirement that the option is actually scalar. That collides with a real, already-scraped case in this repo: az vmss application set --treat-deployment-as-failure / az vm application set --treat-deployment-as-failure, whose help text is "Space-separated list of true or false corresponding to the application version ids…" (see src/ModularPipelines.Azure/Options/AzVmssApplicationSetOptions.Generated.cs:36). Under the new code:

  • explicitBooleanValue becomes true (matches true\s+or\s+false)
  • IsPresenceOnlyFlag short-circuits to false (no longer a bare flag — good, that's actually more correct than before)
  • but DetermineType also short-circuits to bool? before ever checking "space-separated", so the option is still modeled as a single scalar value instead of the list it actually is, and AcceptsMultipleValues stays false.

The result: once Azure options are regenerated, this property will require exactly one true/false value even though the real CLI expects one boolean per application-version id, space-separated. Note the base-class safety net ValidateOptionShapes (CliScraperBase.cs:943) won't catch this either, since RepeatableValuePattern doesn't match "space-separated list of" — so this would fail silently rather than throwing.

CobraCliScraper avoids this class of bug because its boolean promotion (IsBooleanValueOption) is only consulted once isBoolean has already been derived from the real type hint (CobraCliScraper.cs:334-340), not purely from free-text pattern matching on the description. Recommend the same ordering here: check for list/array signals in the description before applying the explicit-boolean promotion (or otherwise scope HelpDeclaresExplicitBooleanValue so it doesn't fire when DetermineType's list-detection would also fire), so a description that says "list of true/false values" doesn't collapse into a scalar bool?.

This is a narrow edge case (appears to affect a couple of Az CLI options in the current dataset), and doesn't block the main fix — flagging it since it's a direct, real-data consequence of the changed condition in this diff.

No other issues found; the rest of the change is a clean, minimal extension of the existing shared pattern.

@thomhurst
thomhurst force-pushed the issue-3989-az-bool-values branch from 3d13823 to 8abe85a Compare August 23, 2026 19:45
@thomhurst

Copy link
Copy Markdown
Owner Author

Addressed the review's real-data edge case in 2a57ba326a: Azure's space-separated true/false values retain IEnumerable<string>?, while scalar explicit Boolean options remain bool?. Added the exact az vm application set --treat-deployment-as-failure regression. OptionsGenerator tests pass 818/818; Release build has 0 warnings/errors.

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown

Code review

Follow-up on my previous review of this PR: the edge case I flagged (az vm application set --treat-deployment-as-failure, a space-separated list of booleans that would have collapsed into a scalar bool?) is fixed correctly in 2a57ba326a.

DetermineType now gates the bool? promotion through HelpDeclaresBooleanList, which checks for "space-separated"/"list of true"/"list of false" before falling back to the collection type instead of always returning bool? once explicitBooleanValue is true (AzCliScraper.cs:368-391). The new Boolean_Lists_Remain_Collections test locks in exactly that regression, alongside the earlier Boolean_Accepted_Values_Require_An_Explicit_Value case.

I pulled the branch and ran the full ModularPipelines.OptionsGenerator.Tests suite: 820/820 passing, consistent with the 818/818 the author reported (small delta likely just from main moving).

One minor maintainability note (non-blocking)

HelpDeclaresBooleanList (AzCliScraper.cs:388-391) and the general list-detection in DetermineType (AzCliScraper.cs:379-383) both re-implement "does this description describe a space-separated/list value," with slightly different substrings ("space-separated" / "list of true" / "list of false" vs. "space-separated" / "list of" / "multiple"). They're separate code paths today because the Boolean case intentionally excludes the "multiple" heuristic (per the comment at AzCliScraper.cs:368-370, since "multiple" can describe the resources rather than the values). That's the right call for correctness, but the partial overlap means a future change to one list-detection heuristic (e.g. adding "comma-separated" as a synonym) could easily be made in only one of the two places and silently reintroduce this class of bug. Consider extracting the shared "space-separated list" check into one helper that both call, with the Boolean path adding only its extra "list of true/false" variants and explicitly opting out of "multiple". This keeps the two independent-on-purpose axes (is it a Boolean, is it a list) from drifting apart in their shared "is it a list" logic.

No other issues found. The change is a clean, well-tested extension of the existing HelpDeclaresExplicitBooleanValue pattern already used by CobraCliScraper/NbgvCliScraper.

@thomhurst

Copy link
Copy Markdown
Owner Author

Addressed the non-blocking drift note in 4599e73. General list detection and Boolean-list detection now share HelpDeclaresSpaceSeparatedList; Boolean handling still deliberately excludes the broad multiple heuristic. Validation: OptionsGenerator tests 820/820; Release build 0 warnings/errors.

@thomhurst

Copy link
Copy Markdown
Owner Author

Rebased onto current main (13fd094). Validation: Azure scraper tests 4/4; OptionsGenerator Release build succeeded with 0 warnings/errors. Combined run 32664459107 additionally exposed pre-existing generated API drift for AzAksCreateOptions.NodeCount (�ool? baseline vs correct live int?), to migrate with #3935 compatibility support.

@thomhurst
thomhurst force-pushed the issue-3989-az-bool-values branch from 4599e73 to cdee36f Compare August 23, 2026 21:14
@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown

Code review

Follow-up on my previous review of this PR: the maintainability note from my second pass is addressed correctly in 4599e73c33HelpDeclaresBooleanList and the general list check in DetermineType now share HelpDeclaresSpaceSeparatedList (AzCliScraper.cs:379, AzCliScraper.cs:388-394), so that drift risk is gone.

Reviewed against the current head (cdee36f5). Two more issues worth raising before this lands, both stemming from the same root cause: the new ExplicitBooleanValuePattern/HelpDeclaresBooleanList checks are unanchored substring matches, so they fire on text that merely contains the boolean phrasing rather than is a boolean description.

1. ExplicitBooleanValuePattern's new "allowed values" branch matches tri-state enums, not just booleans

[GeneratedRegex(
@"(?:[\[{(<]\s*true\s*(?:\||/|or)\s*false\s*[\]})>]|(?:boolean|bool)\s+value|true\s+or\s+false|allowed\s+values?\s*:\s*(?:true\s*,\s*false|false\s*,\s*true))",
RegexOptions.IgnoreCase)]
private static partial Regex ExplicitBooleanValuePattern();

@"(?:[\[{(<]\s*true\s*(?:\||/|or)\s*false\s*[\]})>]|(?:boolean|bool)\s+value|true\s+or\s+false|allowed\s+values?\s*:\s*(?:true\s*,\s*false|false\s*,\s*true))"

The allowed\s+values?\s*:\s*(?:true\s*,\s*false|...) alternative has no trailing boundary, so it matches as a prefix of a longer enumeration. A description like "Allowed values: true, false, auto" is a three-value enum, but this pattern still reports explicitBooleanValue = true because it only checks that "true, false" appears right after "allowed values:" — it never checks what comes after.

This isn't hypothetical for this codebase: HeuristicTypeDetectorTests.cs:455 already asserts that the exact same string, "Allowed values: true, false, auto", must be detected as CliOptionType.Enum (3+ values) by the sibling HeuristicTypeDetector. So there's already a known real-world shape for this text, just not exercised against the new regex in AzCliScraper/CobraCliScraper/NbgvCliScraper (all three call HelpDeclaresExplicitBooleanValue). If an Az/Cobra/Nbgv option ever has this phrasing, DetermineType collapses it to bool?, silently discarding the third allowed value.

Suggested fix: anchor the alternation so it only matches an exact two-value list, e.g. add (?:\.|,?\s*$|\s*[.)\r\n]) after the pair, or reuse the same enum-parsing logic HeuristicTypeDetector already has for "allowed values:" lists and only fall back to the boolean pattern when exactly two values are present.

2. Newly-non-flag explicit-boolean options are exposed to a validation check with inconsistent detection logic, risking silent command drops

Before this PR, everything matching HelpDeclaresExplicitBooleanValue was still IsFlag = true (a bare presence flag), so it never reached the second branch of ValidateOptionShapes:

if (!option.IsFlag
&& HelpDeclaresRepeatableOption(helpText, option.SwitchName, description)
&& !option.AcceptsMultipleValues)
{
throw new InvalidOperationException(
$"{command.FullCommand} {option.SwitchName} is documented as repeatable, "
+ "but the parsed model is scalar.");
}

if (!option.IsFlag
    && HelpDeclaresRepeatableOption(helpText, option.SwitchName, description)
    && !option.AcceptsMultipleValues)
{
    throw new InvalidOperationException(...);
}

Now that IsPresenceOnlyFlag returns false whenever explicitBooleanValue is true (AzCliScraper.cs:331-341), these options are non-flag for the first time and do reach this check. The check's own repeatability detector, RepeatableValuePattern (CliScraperBase.cs:1012-1015), recognizes wording like "one or more", "repeatable", "multiple values" — but HelpDeclaresBooleanList (AzCliScraper.cs:388-391), which decides whether the type becomes a collection, only recognizes "space-separated" / "list of true" / "list of false". Those two wordings aren't the same set.

So a description combining explicit-boolean phrasing with repeatable phrasing that isn't one of HelpDeclaresBooleanList's three substrings (e.g. "one or more values: true or false") would produce explicitBooleanValue = true, IsFlag = false, and a scalar bool? (since HelpDeclaresBooleanList doesn't match) — which is exactly the shape ValidateOptionShapes throws on. TryParseCommandAsync (CliScraperBase.cs:495-511) catches that exception and logs a warning, silently dropping every option for that entire command, not just the mismatched one.

I don't see this combination in the current Az/Cobra/Nbgv help-text corpus (grepped for it — no hits), so it's not actively breaking generation today. But it's a latent trap the PR introduces: two independent "is this a list" heuristics that must stay in sync purely by convention, guarding a validation path whose failure mode is "silently discard the whole command" rather than a loud, scoped error. Given HelpDeclaresBooleanList already delegates to the shared HelpDeclaresSpaceSeparatedList after the last round of feedback, consider having it also delegate to (or reuse) RepeatableValuePattern/HelpDeclaresRepeatableOption for its "is it plural" signal, so the two checks can't drift apart the same way the space-separated ones did.


Both are non-blocking for the immediate #3989 fix (neither reproduces against the current Azure/Cobra/Nbgv help-text dataset), but I'd want at least #1 addressed before merge since it's directly demonstrated by an existing test elsewhere in this repo, not just a theoretical string.

No other issues — the core fix and its test coverage remain solid.

@thomhurst

Copy link
Copy Markdown
Owner Author

Addressed both current-head review points in 5c7e8646ac:

  • Allowed values: true, false now requires an exact two-value terminator, so true, false, auto remains non-Boolean.
  • explicit Boolean collections reuse DescriptionDeclaresRepeatableOption, keeping type inference aligned with shape validation instead of dropping the command.

Added Azure regressions for the tri-state and one or more values: true or false cases. Validation: AzCliScraperTests 6/6, NbgvCliScraperTests 5/5, CliScraperTraversalTests 19/19; scoped whitespace clean.

@claude review exact head 5c7e8646ac01028836d805ee32aa6b80c5ab265b.

@thomhurst

Copy link
Copy Markdown
Owner Author

Validated new head 5c7e864 after Boolean-hint bounding: AzCliScraperTests 6/6 and WinGetCliScraperTests 4/4 passed; OptionsGenerator Release build succeeded with 0 warnings/errors; commit diff check clean.

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown

Code review

Follow-up review at the requested head 5c7e8646ac01028836d805ee32aa6b80c5ab265b (merged into main as f6a881d0 for this checkout).

I re-verified both points raised in my previous review and confirmed they're fixed:

  1. Tri-state boundingExplicitBooleanValuePattern (CliScraperBase.cs:1007-1010) now requires the two-value list to be followed by ., ), or end-of-string, so "Allowed values: true, false, auto" no longer collapses to bool?. Covered by Tri_State_Allowed_Values_Are_Not_Collapsed_To_Boolean.
  2. Repeatable/Boolean-list driftHelpDeclaresBooleanList (AzCliScraper.cs:388-392) now calls the shared DescriptionDeclaresRepeatableOption, so "One or more values: true or false" is correctly typed as IEnumerable<string>? instead of tripping ValidateOptionShapes's "documented as repeatable but parsed as scalar" exception. Covered by Repeatable_Explicit_Boolean_Values_Remain_A_Collection.

I pulled the branch and ran ModularPipelines.OptionsGenerator.TestsAzCliScraperTests: 6/6 passing, consistent with what was reported.

One residual gap in the same "keep type inference aligned with shape validation" fix (non-blocking)

The alignment in point 2 only covers the description-level signal. ValidateOptionShapes's repeatable check actually calls the two-step HelpDeclaresRepeatableOption(helpText, switchName, description) (CliScraperBase.cs:889-925), which falls back to scanning the raw help-text window from the option's line up to the next blank line or option line (CliScraperBase.cs:899-921) — a window bounded only by blank-line/option-line, with no indentation or section-header cutoff. HelpDeclaresBooleanList, by contrast, only ever sees the already-accumulated description field, which AccumulateMultiLineDescription (AzCliScraper.cs:413-462) stops populating earlier — on insufficient indentation (< 20 spaces) or a short section-header-looking line — cutoffs the raw-text fallback doesn't apply.

So a continuation line carrying repeatable wording ("one or more", "repeatable", etc.) that fails AccumulateMultiLineDescription's indentation/header cutoff but still falls inside the wider raw-scan window would: leave description without the repeatable wording → HelpDeclaresBooleanList returns falseDetermineType emits scalar bool? / AcceptsMultipleValues = false, while ValidateOptionShapes's HelpDeclaresRepeatableOption still finds the wording via its raw-text fallback and throws InvalidOperationException. TryParseCommandAsync (CliScraperBase.cs:489-511) catches that and silently drops every option for the whole command — not just the mismatched one.

This is the same class of bug the last two rounds caught (a heuristic used for typing drifting from the heuristic used for validation), just relocated from wording mismatch to text-capture-boundary mismatch. It doesn't reproduce against the current Az/Cobra/Nbgv corpus and none of the new tests happen to hit it, so it's not blocking. Worth noting separately: the new AzCliScraperTests (Boolean_Lists_Remain_Collections, Repeatable_Explicit_Boolean_Values_Remain_A_Collection, etc.) call ParseCommandAsync directly through the TestAzCliScraper helper, bypassing TryParseCommandAsync/ValidateOptionShapes entirely — so even the already-fixed alignment in point 2 isn't exercised end-to-end against the validator that would actually throw in production. A test that goes through the public TryParseCommandAsync-equivalent path (or calls ValidateOptionShapes directly) for at least one explicit-Boolean-collection case would close that gap and also catch the residual issue above if it's ever hit.

Two smaller, lower-priority notes (non-blocking, unlikely in practice)

  • DetermineType's numeric-hint check (AzCliScraper.cs:361-365) runs before the explicitBooleanValue branch (AzCliScraper.cs:371-376). Before this PR that branch was unreachable for explicit-Boolean options (they were always IsFlag = true and short-circuited at the top of the method); now that IsPresenceOnlyFlag treats them as non-flags, an option whose value hint happens to contain "count"/"number"/"port"/"size"/"timeout" alongside explicit true/false allowed values would be typed int? instead of bool?. Very unlikely combination in real Azure CLI help text, so low priority.
  • HelpDeclaresBooleanList only recognizes the literal substrings "list of true" / "list of false" (AzCliScraper.cs:391-392), narrower than the generic "list of" check used three lines above for the non-Boolean branch. A description phrased as "List of Booleans... Allowed values: true, false." would collapse to scalar bool?. Again, not seen in the current corpus.

Nothing here blocks merge — the core #3989 fix and its two prior review rounds' follow-ups are correctly and demonstrably fixed by tests. The residual gap I called out is the same kind of latent trap flagged (and accepted as non-blocking) in the last two rounds, now at a different layer of the same alignment problem.

Treat Azure CLI options documented as Boolean values as value-taking bool options while retaining ordinary presence-only flags.`n`nFixes #3989
Keep space-separated true/false values as collections before promoting scalar Boolean options.
Recognize only exact two-value allowed-value lists as Boolean, and share repeatability detection when explicit Boolean values form a collection. Refs #3989.
@thomhurst
thomhurst force-pushed the issue-3989-az-bool-values branch from 5c7e864 to 47fba03 Compare August 23, 2026 22:05
@thomhurst

Copy link
Copy Markdown
Owner Author

Rebased five commits onto current main after #4023/#3985 and force-pushed with an exact lease. Validation: AzCliScraperTests 6/6 and WinGetCliScraperTests 4/4 passed; OptionsGenerator Release build succeeded with 0 warnings/errors; diff checks clean. Verified remote head: 47fba03.

@thomhurst

Copy link
Copy Markdown
Owner Author

@claude Please re-review exact head 884b0e0. Added WinGet authentication-mode regression and shared contextual enum parsing for preference lists, preventing explicit mode choices from becoming boolean flags. Validation: WinGetCliScraperTests 5/5, DescriptionEnumValueParserTests 20/20, AzCliScraperTests 6/6.

@claude

claude Bot commented Aug 23, 2026

Copy link
Copy Markdown

Code Review

Reviewed the changes to AzCliScraper's explicit-boolean detection (and the small follow-on tweaks to WinGetCliScraper / DescriptionEnumValueParser). The fix is well-targeted and the new AzCliScraperTests cover the tricky cases nicely (space-separated lists, tri-state "allowed values", repeatable booleans). Nice touch reusing DescriptionEnumValueParser.TryParse in WinGetCliScraper.IsBooleanDescription to avoid a false positive — that's the right pattern.

A few things worth considering before/after merge:

1. The new allowed values: true, false regex alternative is narrower than the parser you already have

CliScraperBase.cs:1008 adds allowed\s+values?\s*:\s*(?:true\s*,\s*false|false\s*,\s*true)... as a one-off, hand-rolled pattern. But DescriptionEnumValueParser.ExplicitValuesPattern already generically recognizes "valid values", "possible values", "accepted values", "must be one of", etc. (and is proven out — you just reused it in WinGetCliScraper in this same PR). A description like "Possible values: false, true." or "Valid values: true, false." — same semantics as the phrasing your tests use, just from a different CLI's help text style — won't be caught by the new alternative, so that option would still be misclassified as a presence-only flag, which is the exact bug this PR fixes for az.

Suggested approach: have HelpDeclaresExplicitBooleanValue (or a caller) check DescriptionEnumValueParser.TryParse(description) for a 2-value {true, false} result, rather than adding another regex alternative to ExplicitBooleanValuePattern. That gets you "allowed/valid/possible/accepted values: true, false" for free from a single source of truth, instead of growing this pattern one phrasing at a time.

2. The "flag unless explicit boolean" combination is now implemented three times

AzCliScraper.IsPresenceOnlyFlag (new in this PR) duplicates logic that already exists independently in NbgvCliScraper (~line 132-134) and CobraCliScraper.IsBooleanValueOption (~line 798, inherited by every Cobra-derived scraper — Docker, Helm, kubectl, gh, Podman, etc.). CliScraperBase.ValidateOptionShapes (line 948) already encodes this invariant centrally (it throws if HelpDeclaresExplicitBooleanValue && option.IsFlag), which is a good post-hoc guard, but the actual computation of IsFlag from that same signal is left to each scraper to reimplement.

Given the base class already knows the rule well enough to validate it, consider promoting a protected helper (e.g. IsPresenceOnlyFlag(valueHint, explicitBooleanValue)) onto CliScraperBase itself so Az/Nbgv/Cobra (and any future scraper) share one implementation instead of three that can silently drift out of sync — as NbgvCliScraper already has, since it also branches ValueSeparator on explicitBoolean in a way AzCliScraper's new code doesn't.

3. Broadening the shared regex widens ValidateOptionShapes's blast radius for scrapers that don't use it

ExplicitBooleanValuePattern feeds ValidateOptionShapes, which runs for every scraper via TryParseCommandAsync, but only AzCliScraper, NbgvCliScraper, and the Cobra-derived scrapers actually consult HelpDeclaresExplicitBooleanValue when computing their own IsFlag. The other ~20 direct CliScraperBase subclasses (Maven, Terraform, Jq, Gcloud, Flyway, Pnpm, DotNet, Vault, Cargo, Aws, Go, Gradle, etc.) compute IsFlag independently. If any of those tools' real help text ever matches the new "allowed values: true, false" wording while that scraper's heuristic still marks the option as a flag, ValidateOptionShapes throws, the surrounding try/catch in TryParseCommandAsync swallows it, and the entire command (not just the one option) is silently dropped from codegen output with only a logged warning.

I checked — no existing test fixture currently triggers this, so it's not a regression today, just a latent risk this PR slightly widens. Worth keeping in mind if #1/#2 get consolidated, since a shared IsPresenceOnlyFlag helper on the base class would let you wire this into all scrapers at once (or at least make the gap visible) rather than having it silently degrade to "drop the command."

Nothing here blocks the PR — the fix does what it says on the tin and is well tested — these are suggestions for tightening the abstraction and closing a couple of related gaps while you're in this code.

@thomhurst
thomhurst merged commit 3279c9b into main Aug 24, 2026
15 checks passed
@thomhurst
thomhurst deleted the issue-3989-az-bool-values branch August 24, 2026 04:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

OptionsGenerator: az options declaring explicit true/false values are parsed as presence-only flags (~12 options)

1 participant