Fix AWS CLI man-page help parsing - #4005
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe AWS CLI scraper now removes man-page overstrike sequences from help text. A fixture-backed test validates extraction of three services from AWS CLI 2.36.29 output. ChangesAWS CLI scraping
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This localized parsing fix is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR normalizes groff backspace overstrikes before shared CLI help parsing and adds an AWS CLI 2.36.29 regression fixture.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliScraperBase.cs | Extends shared help-text normalization to remove groff backspace overstrike prefixes before parsing. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/AwsCliScraperTests.cs | Adds an end-to-end AWS scraper regression test using recorded root-help output. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/ModularPipelines.OptionsGenerator.Tests.csproj | Copies fixture files to the test output directory at the relative path expected by tests. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Fixtures/AwsCli/aws-2.36.29-root-help.json | Records AWS CLI 2.36.29 help containing the groff overstrike service bullets addressed by the fix. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[AWS CLI help output] --> B[Strip ANSI sequences]
B --> C[Remove groff overstrike prefixes]
C --> D[Extract AWS services]
D --> E[Traverse discovered commands]
E --> F[CliCommandDefinition results]
Reviews (6): Last reviewed commit: "fix(options): normalize man-page overstr..." | Re-trigger Greptile
Code reviewReviewed the diff ( Summary: This fixes AWS CLI service-list parsing when help output comes through Architecture note (positive): Landing the fix in Test coverage: The new fixture-driven test ( No bugs or CLAUDE.md violations found in the diff. This is a well-scoped, well-tested fix. |
dc61771 to
22b35e4
Compare
There was a problem hiding this comment.
Code Review — PR #4005 (Fix AWS CLI man-page help parsing)
Verification performed: Built tools/ModularPipelines.OptionsGenerator/ModularPipelines.OptionsGenerator.slnx and ran the full ModularPipelines.OptionsGenerator.Tests project (817 tests, all passing, including the new Extracts_Services_From_Aws_2_36_29_Help_Fixture test). Hand-traced the new ManPageOverstrikePattern (.\x08) against the +\bo overstrike fixture to confirm it correctly collapses char+backspace+char sequences without corrupting adjacent bullet text.
Overall this is a small, well-tested, well-scoped fix — the regex change is minimal and the new fixture reproduces the real-world AWS CLI 2.36.29 output that motivated it. No correctness bugs found. Two low-severity, non-blocking notes:
- Shared blast radius, single-source test coverage (
CliScraperBase.cs:664) —NormalizeHelpTextis shared by all ~48 CLI scrapers (Docker, Terraform, kubectl, gcloud, etc.), but the new overstrike-stripping regex is only exercised by a synthetic AWS fixture. If any other scraped CLI ever emits a literal backspace byte for a reason other than bold/underline overstrike (e.g. a stray control character from a spinner leaking into captured help text), the preceding character would be silently deleted with no error surfaced. Not a reason to block — just worth keeping in mind if another scraper's generated output ever looks subtly truncated. - Doc comment is now slightly stale (
CliScraperBase.cs:657) — the XML doc onNormalizeHelpTextdescribes ANSI-sequence removal but doesn't mention the new man-page overstrike handling, which could lead a future maintainer to duplicate the logic elsewhere instead of extending this method.
Nice fix — approving from a review standpoint modulo those two minor notes.
3724a3c to
127667d
Compare
Code reviewRe-reviewed the current diff — it's unchanged from the version already reviewed in the earlier comment on this PR (same three files: Confirms:
No bugs or CLAUDE.md violations found. Nothing outstanding from the prior review needs addressing — this is a well-scoped, well-tested fix. |
|
Rebased onto current main (13fd094). Validation: AWS scraper tests 2/2; OptionsGenerator Release build succeeded with 0 warnings/errors. |
127667d to
1d7b64b
Compare
Code reviewRe-checked after the rebase onto current No bugs or CLAUDE.md violations found. Nothing outstanding from the prior reviews needs addressing — this remains a well-scoped, well-tested fix. |
1d7b64b to
5c5f318
Compare
Summary
+\boservice bulletsScrapeAsynctraversalTest plan
Full solution
dotnet format --verify-no-changes --severity infoalso reports pre-existing findings outside this change, includingIHelpTextCache.cswhitespace andAzureCliHelpTypeDetector.csstyle errors.Closes #3986
Summary by CodeRabbit
Bug Fixes
Tests