fix(generator): parse pnpm 12 clap-style help - #4713
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change adds pnpm 12 clap-style help parsing, including enums, repeatable options, passthrough parameters, and usage validation. It updates pnpm fixtures, tests, generated documentation, public API references, rendering tests, and CLI-generation workflow branches. Changespnpm Clap Help Support
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant PnpmHelp
participant PnpmCliScraper
participant CliScraperBase
participant GeneratedOptions
PnpmHelp->>PnpmCliScraper: Provide pnpm 12 clap help
PnpmCliScraper->>CliScraperBase: Parse usage and option blocks
CliScraperBase->>GeneratedOptions: Create typed options and enums
GeneratedOptions->>GeneratedOptions: Validate operands and render pnpm commands
Merge Risk: ⚪ Minimal · up to The updated pnpm help parsing and generated surface have targeted coverage for the documented clap-format behaviors. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The pull request includes changes unrelated to issue
✨ Finishing Touches 💡 1📝 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. A rabbit parsed the clap-filled scroll, Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b855f637d9
ℹ️ 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
Inactive
There was a problem hiding this comment.
💡 Codex Review
Remove the parent command placeholder from leaf definitions
With this new clap-style stageHelp, ExtractSubcommands(commandPath, ...) returns no children for the repeated leaf help, so traversal does not call RemoveCommandGroupPlaceholders; NormalizeParentGroupUsage only clears operands for the exact pnpm stage path. Consequently both asserted leaf definitions retain a required Command positional from pnpm stage [OPTIONS] <COMMAND>, producing APIs that require callers to supply the subcommand again and can render commands such as pnpm stage download download. Extend the assertion to cover positionals and normalize repeated parent usage before parsing leaf definitions.
AGENTS.md reference: AGENTS.md:L170-L172
ℹ️ 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".
Review: pnpm 12 clap-help scraper rewriteReviewed the diff for PR #4713 ( What I checked
I did not find correctness issues in the scraper/generator logic that's actually part of this diff (confirmed via Nit (non-blocking): No blocking findings. |
GitHub Actions
Inactive
e6deb22 to
5af327c
Compare
|
Pushed review fixes at 5af327c after a clean rebase onto current main.
Full generator suite: 1,359 passed. Release solution build: zero warnings/errors. Scoped formatting and diff checks passed. Local regeneration exceeded the repository's 2,048 MB guard (2,550 MB observed); no local retry or limit increase. Tool-only CI regeneration is running with automatic merging disabled: https://github.com/thomhurst/ModularPipelines/actions/runs/34765043823. Its generated changes and validation must be reconciled before merging this PR. |
Greptile SummaryThis PR updates pnpm option generation for pnpm 12’s clap-style help format.
Confidence Score: 4/5The PR is not ready to merge because the new Node rendering tests reference generated The parser changes expect pnpm audit and stage generation to produce parent Files Needing Attention: test/ModularPipelines.Node.UnitTests/Api/PnpmCommandRenderingTests.cs and the generated pnpm option files under src/ModularPipelines.Node/Options/
|
| Filename | Overview |
|---|---|
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/PnpmCliScraper.cs | Reworks pnpm parsing around clap-style command, option, metadata, and variadic-parameter output. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/CliScraperBase.cs | Adds shared clap block, possible-value, enum, and required-option parsing helpers. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/PnpmCliScraperTests.cs | Adds comprehensive regression coverage for pnpm 12 help parsing and generated metadata. |
| test/ModularPipelines.Node.UnitTests/Api/PnpmCommandRenderingTests.cs | Tests parent variadic parameters that are absent from the currently checked-in generated pnpm classes. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[pnpm 12 help text] --> B[Usage and clap option parsing]
B --> C[CLI command definition]
C --> D[Options and enum generation]
D --> E[Generated pnpm option classes]
E --> F[Command-line rendering tests]
Reviews (3): Last reviewed commit: "test(node): render current pnpm parent p..." | Re-trigger Greptile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5af327cfc2
ℹ️ 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".
|
The first tool-only generation run stopped safely at command coverage validation: pnpm 12.4.1 produced 103 commands versus the old 17-command baseline, but its help no longer lists Fixed the related parameter omission in 0098d3d and recorded explicit coverage reasons for those seven paths. All seven Validation: both new recorded-help cases reproduced the bug before the fix; all 1,361 generator tests now pass. Release build has zero warnings/errors. Scoped formatting and diff checks pass. Fixture blank lines have trailing whitespace removed. Regeneration for the corrected head is running at https://github.com/thomhurst/ModularPipelines/actions/runs/34766164896, with auto-merge disabled. Its generated changes and integration validation must be reconciled before merging this PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0098d3d698
ℹ️ 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".
|
Generation run 34766164896 passed scraping and command coverage: 335 files generated, 103 commands, zero generator errors. It then stopped at the Node integration build because PnpmCommandRenderingTests referenced the removed PnpmStagePublishOptions. Commit 6fd7a87 updates rendering tests to the current PnpmStageOptions/PnpmAuditOptions Params API. Formatting and diff checks pass. These tests require the pending generated output, so their compilation and execution remain deferred until that output is incorporated; the parent branch is not ready to merge yet. Corrected generation run: https://github.com/thomhurst/ModularPipelines/actions/runs/34766806886. Auto-merge and blanket coverage approval remain disabled. Once CI publishes its generated commit, I will bring those changes into this PR and run the Node tests and documentation checks. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Review: pnpm 12 clap-help scraper rewrite (update) A prior review of this PR at commit Blocking findings 1. var commandLine = builder.Build(new PnpmStageOptions
{
Params = ["publish", "package.tgz"], // no Params member
...
});I read 2. Around line 158 it emits a sample using 3. Stale generated artifacts contradict the new
Why this matters and the suggested fix: per this repo own constraint, generated options must never be hand-edited or left stale; the scraper/generator change and its regenerated output are supposed to land together, with current tool output authoritative. The fix here is not to hand-patch the generated files (that would violate the same rule from the other direction); it is to actually run the generator against the new I did not find issues in the actual clap-parsing logic itself ( |
GitHub Actions
Failure
6fd7a87 to
3996816
Compare
|
Too many files changed for review (357 files, 100 file limit). Bypass the limit by tagging |
|
Incorporated the Linux-generated commit from #4861 and rebased onto current main. Required SBOM options, enums, and audit/stage Params are now committed in this PR. Current head: 3996816. The generated review correctly flagged bare awaits. Added the shared ConfigureAwait(false) template fix and four generator regressions; all 1,365 generator tests pass. Final generation from that template is running at https://github.com/thomhurst/ModularPipelines/actions/runs/34767616658 and will update #4861 for incorporation here. Linux generation already built the Node solution. The local Node build hit the 2,048 MB guard at 2,140 MB, so Node build/test execution is deferred to CI without retrying locally. Documentation validation is still running. This PR remains unready to merge until the remaining generated update and validations complete. |
There was a problem hiding this comment.
💡 Codex Review
Regenerate wrappers after changing await emission
At this head, GeneratorUtils now emits .ConfigureAwait(false), but all 103 awaits committed in Pnpm.Generated.cs still omit it, including this new SbomAsync wrapper. Consequently the shipped library does not receive the fix and can capture a caller's synchronization context, potentially deadlocking callers that synchronously wait for these methods; regenerate and commit the pnpm artifacts after the generator change.
AGENTS.md reference: AGENTS.md:L33-L35
ℹ️ 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".
|
Documentation validation completed successfully: Node 24.14.1, yarn install --frozen-lockfile, and yarn build (302.92 seconds). All 331 documents were processed. The generated pnpm documentation is included in this result. The final await-template generation run 34767616658 is queued. Node build/test execution remains deferred to CI after the local memory guard; no local retry will be made. |
Review: pnpm 12 clap-help scraper rewrite (update at
|
|
Final Linux generation is incorporated in 13dbeb1. All 103 wrappers now use ConfigureAwait(false), and Assert-GeneratedOptionsFreshness passes against HEAD. The final CI generation built the Node solution with zero errors. No Markdown or public API changed in this final generation, so the prior successful documentation build still covers the generated docs. Local Node validation is still deferred after the previously recorded 2 GB guard; no expensive local retry was run. Node test execution and current-head CI/review remain required before merge. The generated child #4861 is retained until its active review finishes, then can be closed as incorporated. |
Review: pnpm 12 clap-help scraper rewrite (update at
|
Co-authored-by: thomhurst <9139608+thomhurst@users.noreply.github.com>
|
Addressed finding 1 in review 5659826846 with c907087. GetOptionDescription now bases its fallback on whether an enum actually exists. One-value and 21-value lists retain their choices in aligned and block layouts, and individual value documentation survives the non-enum fallback. All five regressions failed before this fix. Finding 2 does not reproduce with clap's block layout. ReadClapOptionBlock assigns descriptionColumn only after rejecting indentation <= switchColumn; the column cannot fall back to or below the switch column. Subsequent prose at that column remains inside the block even when '-x ...' or '--example ...' satisfies the declaration regex. Two new short-only block tests exercise both forms, preserve the complete description, and retain the following real short/long declarations. Both tests pass on the preceding implementation as well as the current head. A declaration-shaped line at the actual declaration column is a declaration under this layout; adding a regex-only boundary would misclassify legitimate wrapped prose. No boundary change is needed for the reported case. All 130 focused generator tests pass. The Release generator solution build has zero warnings/errors, scoped severity-info formatting passes, and all 61 API baselines validate. #4949 incorporated the previous successful 103-command regeneration; the new source change is now being regenerated in https://github.com/thomhurst/ModularPipelines/actions/runs/34813687920. Current output, provenance, and subsequent CI/review remain merge gates. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c907087459
ℹ️ 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".
Review: pnpm 12 clap-help scraper (PR #4713)I re-reviewed the full diff at head Finding 1 from the previous round (possible-values dropped for out-of-range enum candidates) is fixed and tested. Finding 2 (short-only switches widening what Everything else I checked:
Pre-existing, already-tracked, non-blocking items (unchanged from earlier rounds, not something this PR needs to fix): the enum+optional-value combination still falls through to I was not able to independently execute No new blocking findings. |
Co-authored-by: thomhurst <9139608+thomhurst@users.noreply.github.com>
Review: pnpm 12 clap-style help parsing (#4713)Reviewed at head Independent re-verification of the core logic (
Everything else matches what the long review history on this PR already established as clean: shared clap-parsing state machine, possible-value enum extraction with the 2–20 member gate, I was not able to run No blocking findings. |
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c9b4220fe
ℹ️ 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".
Co-authored-by: thomhurst <9139608+thomhurst@users.noreply.github.com>
Review: pnpm 12 clap-style help parsing (#4713)Reviewed at head
Reviewed the new logic change directly (not just trusted the description):
I was not able to run No new or unresolved correctness issues found. Consistent with the prior |
GitHub Actions
Failure
Review: pnpm 12 clap-style help parsing (#4713)Reviewed at head Reviewed the change directly in
This PR has been through many review rounds (pnpm clap-parsing rewrite, shared No new or unresolved correctness issues found. Consistent with the prior |
GitHub Actions
Failure
|
Pushed 4551be2 for the latest Linux failure in run 34824200007, job 103913733448. The previous missing-result test passes; the only distributed failure is now Cancellation_Observer_Retries_After_Transient_Failure at its outer five-second guard (5.048 seconds; suite duration 20.595 seconds). The observer test now uses the existing 30-second harness convention and passes test cancellation through the public execution-backend overload. This also lets a harness timeout cancel the worker's infinite dequeue mock and background tasks instead of only abandoning the outer wait. The one-millisecond retry interval, transient first failure, exactly two observer attempts, and empty result assertion remain unchanged. No production retry logic or deadlines changed, and no CI rerun was requested. All 198 distributed tests pass locally (1.818 seconds). The Release Tests solution build passes with zero errors and 181 warnings; scoped formatting and diff checks pass. Current pnpm source/output freshness passes at the new head; generator and Node code are unchanged from the previously validated generation. Existing resource limits remain unchanged. The new CI/review cycle must pass before resolving remaining threads or merging. |
Review: pnpm 12 clap-style help parsing (#4713)Reviewed at head Independently verified the new commit's diff directly (
This is a narrow, well-targeted fix consistent with the same pattern already cleared for the missing-result test, and it matches what the PR description claims for this commit. Core scraper/generator logic ( This PR has been through an unusually long and thorough review cycle (compile-error false alarm, a bracketed-trailer-dropping gap, a reintroduced secret-masking regression, staged regeneration of |
1 participant
pnpm 12 uses clap-style help that the previous scraper missed, dropping options, possible values, required SBOM arguments, and audit/stage passthrough parameters. Shared helpers parse wrapped descriptions and metadata, retain bracketed prose, preserve distinct CLI enum values, and derive required options from usage.
The pnpm scraper preserves short-only switches and wrapped option-shaped prose in both clap layouts. Boolean flags and optional-value options do not emit unused enums. Optional values retain CliOptionValue for bare switches or explicit values. Whenever no enum is generated, descriptions retain possible values and their individual documentation, including one-value and more-than-twenty-value lists. Cargo adoption and broader enum-helper consolidation remain tracked in #4712.
The shared repeatability detector recognizes "Repeat or comma-separate for multiple", including wrapped wording. This fixes the documented --cpu options on pnpm create and dlx, which omit an ellipsis. Their generated collections repeat the switch. Ordinary comma-separated scalar descriptions and operational retries retain their existing behavior.
Authoritative pnpm 12.4.1 output is incorporated through #4954. It includes typed audit levels, required SbomFormat, audit/stage Params, ConfigureAwait(false) on all 103 service awaits, possible-value documentation, and the corrected Cpu collections. Current source and generated manifest independently match fingerprint a64e31201c7dc54dd2d30167e4060f2b00e65d6e1d51caccccb85b03f1c8df08; command coverage remains 103 commands.
Unrelated Linux validation failures exhausted five-second harness guards in the distributed missing-result and cancellation-observer tests. Both now use the existing 30-second test convention and pass test cancellation into their executors. The observer still retries its transient failure after one millisecond and requires exactly two attempts. The one-second ModuleResultTimeout, TimedOut status, canceled coordinator token, and scheduler completion assertions remain unchanged.
Validation:
Generation: https://github.com/thomhurst/ModularPipelines/actions/runs/34819042708. Node validation: https://github.com/thomhurst/ModularPipelines/actions/runs/34819430271/job/103898241687. A fresh review/CI cycle is required after the test-only follow-up.
Closes #4691