fix(generator): emit context-independent service awaits - #4872
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. |
|
Authoritative brew and podman generation is running at https://github.com/thomhurst/ModularPipelines/actions/runs/34772450013, with auto-merge and coverage-shrinkage approval disabled. Expected output branches: automated/update-cli-options-brew-issue-4848-generated-service-awaits and automated/update-cli-options-podman-issue-4848-generated-service-awaits. Generated-output review and integration validation remain required. |
|
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 ignored due to path filters (93)
📒 Files selected for processing (10)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThe workflow now derives ref-specific branches for generated CLI options. Validation accepts feature-branch refreshes. Homebrew and Podman public API records and CLI documentation add or update command support. ChangesCLI options generation and validation
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant GetGeneratedOptionsBranch
participant GitHubPullRequests
GitHubActions->>GetGeneratedOptionsBranch: resolve branch from tool and source ref
GetGeneratedOptionsBranch-->>GitHubActions: return GENERATED_OPTIONS_BRANCH
GitHubActions->>GitHubPullRequests: look up pull request by branch
GitHubActions->>GitHubPullRequests: create or update pull request
Merge Risk: ⚪ Minimal · up to No actionable current-head risk remains from the reviewed changes. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (9 skipped: 9 unsupported.)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 reads each line, Comment |
Greptile SummaryThis PR updates generated service wrappers to avoid synchronization-context capture and adds focused coverage for required/optional options in root and nested services. It also isolates feature-branch generation output by incorporating the source branch into generated PR branch names.
Confidence Score: 4/5The PR is not safe to merge until generated-integration CI can correctly recognize branches using the new feature-branch suffix. The service-generation fix is complete and covered, but feature-branch generated PRs are parsed with an invalid tool identifier, bypassing their intended freshness and integration-validation route. Files Needing Attention: .github/workflows/generate-cli-options.yml
|
| Filename | Overview |
|---|---|
| .github/workflows/generate-cli-options.yml | Isolates generated branches by source ref, but the resulting names are incompatible with generated-integration CI tool parsing. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator.Tests/Generators/ServiceAwaitGenerationTests.cs | Adds focused regression coverage for all root/nested and required/optional service-await combinations. |
| tools/ModularPipelines.OptionsGenerator/src/ModularPipelines.OptionsGenerator/Generators/GeneratorUtils.cs | Emits context-independent service awaits in both template branches and applies semantics-preserving collection-expression formatting. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Feature-branch workflow dispatch] --> B[Generated branch: prefix + tool + source ref]
B --> C[Generated pull request]
C --> D[CI takes everything after prefix as tool]
D --> E[Manifest lookup fails]
E --> F[Generated integration and freshness route skipped]
Reviews (1): Last reviewed commit: "fix(ci): isolate feature-branch generati..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 7f2288dc97
ℹ️ 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
Failure
ReviewBuild-breaking bug (blocking): The PR converts private static readonly HashSet<string> CSharpKeywords =
[
with(StringComparer.Ordinal),
"abstract", "as", "base", ...
];
Suggested fix — revert to the explicit constructor form, since constructing a private static readonly HashSet<string> CSharpKeywords = new(StringComparer.Ordinal)
{
"abstract", "as", "base", ...
};Secondary, non-blocking concern: GENERATED_OPTIONS_BRANCH: automated/update-cli-options-${{ matrix.tool }}${{ github.ref_name != github.event.repository.default_branch && format('-{0}', github.ref_name) || '' }}This embeds the raw
Sanitizing the ref segment (e.g. replacing What's good: the |
7f2288d to
4dbd41a
Compare
|
Too many files changed for review (103 files, 100 file limit). Bypass the limit by tagging |
|
Pushed and verified 4dbd41a. This incorporates both generated child outputs and addresses the source-ref naming/parser findings. The current generator template is already on main via #4855; its comparer-bearing collection expression compiled in that PR's successful generator CI under the pinned SDK and LangVersion=preview. No syntax rewrite is needed for the earlier compiler claim. Both integration Release builds pass with zero warnings/errors, all four integration tests pass, and every one of the 382 Homebrew/Podman generated awaits uses ConfigureAwait(false). Full formatting verification finds existing diagnostics in unchanged dependencies; the PowerShell/workflow checks and git diff --check pass. Freshness correctly rejects the old source fingerprint after shared generator/workflow changes; actual regeneration is running at https://github.com/thomhurst/ModularPipelines/actions/runs/34779079394. The PR remains blocked on that output, fresh CI/review, and the shared Microsoft.Build 18.10.1 NU1701 restore failure. No generated provenance was edited to bypass freshness. |
GitHub Actions
Failure
ReviewPrevious blocking findings — both resolved:
Spot-checked the regenerated output:
One non-blocking observation: No other issues found. This is a well-scoped fix that directly incorporates the prior review feedback with real regression coverage. |
Co-authored-by: thomhurst <[email protected]>
Co-authored-by: thomhurst <[email protected]>
Code reviewReviewed at head Previously flagged issues — still resolved, unchanged at this head:
Spot-checked the newly-incorporated regeneration output:
Non-blocking, already-noted: No actionable issues found in the incorporated regeneration output. |
|
Review gate audit: the branch naming/parser concern is fixed by 4dbd41a and explicitly cleared by both subsequent Claude reviews. The regenerated-output concern is fixed and validated at fca5a30. CodeRabbit reports no actionable current-head risk; its aggregate docstring percentage does not identify a missing public contract, and these changes add a script entry point and regression cases rather than undocumented library APIs. No documentation-only rewrite is warranted for that aggregate warning. The broader feature-target CI trigger is intentional so stacked PRs receive the same validation gates. |
1 participant
Generated Homebrew and Podman wrappers captured the caller's synchronization context. This PR incorporates authoritative Homebrew 6.0.22 and Podman 6.1.1 regeneration: all 382 service awaits now use ConfigureAwait(false). The shared template and its four root/nested, required/optional regression cases landed through #4855.
Feature-branch generation now uses a SHA-256 identity of the complete source ref in output branch names. The resolver recognizes that suffix, and CI runs for PRs targeting feature branches. Tests cover nested refs, case differences, slash/dash collisions, repeat runs, and routing for four tools.
Validation:
Refs #4848. That issue remains open until the affected gcloud output tracked by #4869/#4870 is incorporated.