fix(generator): preserve repeatable GitHub CLI attachments - #4855
Conversation
|
Warning Review limit reachedNext included review available in 1 minute. View limit detailsLimit 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. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (43)
📒 Files selected for processing (13)
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 |
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. |
Greptile SummaryThis PR updates GitHub CLI generation so all six issue and pull-request create, edit, and comment commands represent
Confidence Score: 5/5The PR appears safe to merge, with both previous findings resolved and no new actionable defects identified. The attachment override is now limited to the intended issue and pull-request commands, generated serialization preserves each value as a separate argument, and the generator now emits ConfigureAwait(false) consistently across the refreshed GitHub service surface. The previous attachment-scoping thread was resolved after its targeted fix, and the previous ConfigureAwait requirement was manually resolved after the generator and generated services were updated.
|
| Filename | Overview |
|---|---|
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Scrapers/Cli/GhCliScraper.cs | Scopes repeatable attachment inference to the six GitHub issue and pull-request commands while retaining existing inference elsewhere. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Generators/GeneratorUtils.cs | Updates generated service awaits to use ConfigureAwait(false) for both required and optional option parameters. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/ServiceAwaitGenerationTests.cs | Covers top-level and nested service generation with required and optional options. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Scrapers/ZeroOutputScraperTests.cs | Verifies attachment cardinality, scalar body-file behavior, and isolation from unrelated commands. |
| test/ModularPipelines.GitHub.UnitTests/GhAttachmentOptionsTests.cs | Exercises the actual generated option classes and confirms each attachment is serialized separately. |
| src/ModularPipelines.GitHub/Services/Gh.Generated.cs | Regenerates the root GitHub service with the new stack command and context-safe awaits. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
H[GitHub CLI help fixtures] --> S[GhCliScraper]
S --> C{Command is issue/pr create, edit, or comment?}
C -->|Yes, --attach| R[Repeatable IEnumerable<string> option]
C -->|No| N[Normal scalar inference]
R --> G[Generated GitHub options]
N --> G
G --> B[Command-line builder]
B --> A[One --attach=value argument per attachment]
Reviews (3): Last reviewed commit: "chore(github): regenerate wrappers with ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Code reviewThanks for the regeneration work here — the attachment fixture/test coverage across the six issue/PR commands is thorough, and the generator/scraper changes for Blocking issue
private static readonly HashSet<string> RepeatableOptions =
[
with(StringComparer.OrdinalIgnoreCase),
"--field",
"--raw-field",
"--header",
// gh documents multiple attachments in command prose/examples, but prints a scalar file hint.
"--attach",
];
This also silently drops the case-insensitive comparer the old private static readonly HashSet<string> RepeatableOptions = new(StringComparer.OrdinalIgnoreCase)
{
"--field",
"--raw-field",
"--header",
// gh documents multiple attachments in command prose/examples, but prints a scalar file hint.
"--attach",
};The PR description states "Release builds of the OptionsGenerator and GitHub solutions pass," so it's worth double-checking whether this was introduced after that validation step, or in a local formatting/cleanup pass that wasn't rebuilt afterward. |
|
The compiler concern in review comment 5654051678 does not apply to this repository. I built the unchanged 7e534dc generator solution using the pinned SDK through Invoke-AgentDotNet.ps1: zero warnings/errors. Directory.Build.props selects LangVersion=preview. Collection expression arguments explicitly support this comparer syntax: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/collection-expression-arguments. The comparer remains intact. Separately, db2cc76 scopes --attach to the six established commands and adds two failing-before/passing-after regressions. All 1,355 generator tests and 37 GitHub tests pass. GitHub Release build passes with 19 existing style/nullability warnings; scoped formatting passes. Regeneration with gh 2.100.0 preserves all 222 commands and the generated API, with updated provenance. Waiting for the new review/CI cycle before resolving the remaining thread. |
GitHub Actions
Failure
|
Code review — PR #4855 ( Reviewed the Summary The fix is well-scoped and matches the repo's generator-first constraints:
Addressing prior feedback A previous automated review flagged The current HEAD also no longer touches the static Non-blocking observation (out of scope for this PR) Greptile noted the new Verdict No actionable findings against this PR's own changes. Nice job scoping the fix narrowly and backing it with both scraper-level and serialization-level tests. |
(cherry picked from commit 3996816)
|
The ConfigureAwait(false) finding is fixed in a0c84e6 and 43c097f using the shared generator change already developed in #4713, followed by gh 2.100.0 regeneration. All 222 wrappers now suppress context capture, with no API or command-coverage change. Four generator regressions and all 37 GitHub integration tests pass. Release builds pass (the GitHub dependency chain retains 19 existing warnings), and scoped formatting passes. The full generator test attempt reached the default 2 GB process-tree limit (2,198 MB) and was not retried; the four focused tests ran successfully from the completed build. Broader testing remains with CI. The previous Ubuntu failure was unrelated: SendAsync_CustomClientKeepsTimeoutOutsideLoggedReplayContent hit its 100 ms timeout during HttpContentPreviewReader.ReadAsync, before acquiring the stream. Tracked separately as #4864 with the failing run and deterministic-test acceptance criteria. No rerun-for-green was requested. |
Code reviewReviewed the current head ( Prior findings — verified resolved:
Other checks:
No actionable findings against the current head. |
1 participant
GitHub CLI accepts multiple attachments, but its scalar
filehelp hint made the generator emit a single string. Model--attachas a repeatable GitHub CLI option and regenerate all six issue/PR create, edit, and comment options withIEnumerable<string>? Attach. Each value produces its own--attach=...argument, preserving spaces, commas, and#alt text.The rule belongs in the existing GitHub adapter: the flag row does not express cardinality, and inferring repeatability from arbitrary shell examples would risk treating quoted body text or unrelated command examples as flags. Six captured gh 2.100.0 help fixtures also verify that
--body-fileremains scalar. Serialization tests exercise the actual generated classes for all six commands.Regeneration uses the current gh 2.100.0 binary and includes its new
gh stackcommand andgh issue develop --worktreeoption. Command coverage increases from 221 to 222 commands with no removals. API baselines, CLI documentation, and generator provenance are regenerated; generated options were not edited manually.The shared service template now emits ConfigureAwait(false), and all 222 GitHub wrappers are regenerated from that template. Four root/nested and required/optional generation regressions cover this rule.
Validation:
Latest follow-up: four generator regressions and all 37 GitHub integration tests pass; Release builds and scoped formatting pass. The full generator test attempt hit the default 2 GB guard (2,198 MB), so broader validation is deferred to CI. An unrelated HTTP test setup race in the previous Ubuntu run is tracked as #4864.
Closes #4796