Skip to content

fix(generator): preserve AWS metavariable arity - #5159

Merged
thomhurst merged 1 commit into
mainfrom
issue-5158-aws-metavariables
Sep 16, 2026
Merged

thomhurst merged 1 commit into
mainfrom
issue-5158-aws-metavariables

Conversation

@thomhurst

@thomhurst thomhurst commented Sep 16, 2026

Copy link
Copy Markdown
Owner

Summary

AWS synopsis notation such as group_name <value> and --names <value> [<value>...] created extra positional operands. In #5157 those operands also made optional CloudWatch Logs stream filters effectively required.

Normalize these AWS metavariables before shared usage parsing. Explicit repeated-value syntax supplies list arity even when the option's element type is documented as string; scalar description prose keeps its existing behavior. The generated CloudWatch tail constructors now require only their actual inputs and accept omitted optional filters.

Related: #5158, closed generated PR #5157, runtime PR #4367. Keep #5158 open until fresh generation succeeds. Merge this source fix and #5156 before regenerating AWS/gcloud from latest main.

Validation

  • Reproduced five failures in six new scraper cases before the fix.
  • 367 parser/AWS/gcloud/kubectl tests pass.
  • Compiled runtime regression passes for both CloudWatch tail commands, including omitted, empty, and populated stream filters.
  • Scoped generator solution formatting passes.

Summary by CodeRabbit

  • Bug Fixes

    • Improved AWS CLI option parsing for repeated metavariables, ensuring they generate multi-value options instead of positional arguments.
    • Correctly handles named positional metavariables and preserves required constructor validation for AWS log commands.
    • Improved support for optional filters and prefix options in generated AWS CLI options.
  • Tests

    • Added coverage for AWS CLI synopsis parsing and generated option validation.

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 16, 2026

Copy link
Copy Markdown

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-16T01:48:48.976504Z b50dce2 PR opened
ℹ️ 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.

@coderabbitai

coderabbitai Bot commented Sep 16, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

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: 40813451-8c18-49ec-99b9-e215e304328a

📥 Commits

Reviewing files that changed from the base of the PR and between d698c0f and b50dce2.

📒 Files selected for processing (6)
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/RequiredConstructorValidationTests.AwsMetavariables.cs
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/RequiredConstructorValidationTests.cs
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/AwsCliScraperTests.Metavariables.cs
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/AwsCliScraperTests.cs
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/PublicAPI.Unshipped.txt
  • tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/AwsCliScraper.cs

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


📝 Walkthrough

Walkthrough

AWS CLI synopsis parsing now preserves required positional arguments and repeatable option values. New tests cover scraper output, generated constructors, optional filters, and validation for logs tail and logs start-live-tail.

Changes

AWS metavariable handling

Layer / File(s) Summary
Synopsis normalization
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/AwsCliScraper.cs, tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/PublicAPI.Unshipped.txt
AwsCliScraper rewrites AWS option and positional metavariables before base parsing. The public API file records the new override.
Repeatable option classification
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/AwsCliScraper.cs
Repeatable synopsis options become array options and are no longer marked as scalar values.
Generated option validation tests
tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/*, tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/*
Tests verify repeated options, named positional metavariables, generated constructors, optional filters, and validation for AWS log tail commands.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to b50dc

The AWS synopsis parsing change is covered for positional arguments, repeatable options, and optional filters. No unresolved merge-blocking risk is identified.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 5 files. (1 skipped: … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 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 main change: preserving AWS metavariable arity in the generator.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage

Explanation

Docstring coverage is 14.29% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 5 files. (1 skipped: 1 unsupported.)

  • 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-5158-aws-metavariables

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

A rabbit checks the synopsis line
Repeatable values now group fine
Required names stay in place
Filters pass each validation case
AWS options compile and shine

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

@github-actions

Copy link
Copy Markdown
Contributor

Review: fix(generator): preserve AWS metavariable arity (#5159)

Summary of the change: AwsCliScraper gains a ParseUsageSynopsis override that normalizes AWS's argparse-style synopsis notation (--names <value> [<value>...] for options, group_name <value> for named positionals) before handing it to the shared parser, so these no longer register as spurious positional operands. ParseOptions also now treats an option as list-typed when the synopsis shows the repeated <value> [<value>...] marker, even if AWS's own (string) type hint says otherwise, and IsScalarValue is corrected to isScalar && !isArray so that case no longer contradicts itself in ValidateRepeatableOption.

What I checked:

  • This mirrors the existing precedent in GcloudCliScraper.ParseUsageSynopsis (rewriting synopsis value placeholders to switch=VALUE before delegating to the base parser), so it's consistent with how this codebase already solves this class of problem rather than a novel approach.
  • Traced the two new regexes (AwsOptionMetavariablePattern, AwsPositionalMetavariablePattern) by hand against the new fixtures, including the wrapped-line case ("[--names <value>\n [<value>...]]") — the \s+ before the repeat group correctly spans the line break, and the option-metavariable pass runs first so it can't collide with the positional-only pattern (which requires no leading --).
  • Confirmed GroupValues/AcceptsMultipleValues semantics referenced in the new option construction already exist and are used the same way elsewhere (e.g. GcloudCliScraper, SonarScannerCliScraper, GoCliScraper), so this isn't inventing new generator concepts.
  • PublicAPI.Unshipped.txt is updated for the new override ParseUsageSynopsis on the handwritten ModularPipelines.OptionsGenerator project — correct per repo convention (generated CLI packages don't track PublicAPI, but the generator tool itself does). No generated option files were hand-edited.
  • New tests (AwsCliScraperTests.Metavariables.cs, RequiredConstructorValidationTests.AwsMetavariables.cs) cover both the option and positional normalization paths, plus a compiled round-trip proving the generated CloudWatch tail constructors validate with omitted/empty/populated optional filters. This directly targets the regression called out in the PR description ([Automated] Update aws CLI Options #5157/Generator: AWS repeated option metavariables become positional operands #5158).

Two non-blocking observations for a maintainer to weigh, not requesting changes:

  1. repeatedSynopsisOptions.Contains(longForm) in ParseOptions (AwsCliScraper.cs:450) is OR'd into isArray without the !isStructure/!isKeyValue guards that gate the heuristic branch right below it. In practice GroupValues/IsScalarValue end up safe either way (both explicitly re-guard against isKeyValue), but if a real AWS command ever documents a (structure) option with a repeated <value> [<value>...] synopsis, it would now generate IEnumerable<string>? instead of the previous scalar string?. I don't have evidence this occurs in practice, but it's worth a second look next time AWS options are regenerated from main.
  2. The repeated-option detection regex is run twice independently — once inside ParseUsageSynopsis (via the replace) and again inside ParseOptions (AwsOptionMetavariablePattern().Matches(...) on the raw helpText). Both derive from the same source text and same pattern, so today they can't disagree, but factoring the "get repeated option names for this help text" computation into one shared helper would remove the risk of the two drifting apart if either is touched independently later.

I wasn't able to execute dotnet build/tests in this sandboxed session (all dotnet/pwsh invocations required approval that wasn't available here), so this review is based on static reading of the diff and the fixtures rather than a green CI run — the PR description's own validation notes (367 passing parser/AWS/gcloud/kubectl tests, compiled runtime regression for both CloudWatch tail commands) cover that gap.

No actionable correctness issues found. No prior human review comments are outstanding (only in-progress bot summaries from Codex/CodeRabbit at time of review).

@thomhurst

Copy link
Copy Markdown
Owner Author

Review dispositions for b50dce2:

  • Explicit repeated metavariables describe multiple CLI values; they intentionally take precedence over an element-type label. Scalar/key-value prose still cannot create a list. Fresh AWS output will be checked against the source CLI before merge.
  • Normalization and option classification already share AwsOptionMetavariablePattern. They consume it separately because normalization rewrites synopsis text while classification extracts arity from the original help; no duplicated matching rule needs another abstraction.
  • CodeRabbit's docstring percentage includes private regex helpers and tests. The new protected override inherits its contract, and comments explain the non-obvious AWS notation. Keeping documentation focused on those contracts rather than adding repetitive test/helper XML summaries.

Both reviews report no actionable findings. Waiting for remaining CI before source merge and regeneration.

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.

1 participant