ci: gate all checks behind a single required job and fix merge queue - #801
Conversation
`needs:` cannot reference jobs in another workflow file, so a single required status check means the gated jobs have to live in one workflow. Fold code-coverage.yml, dotnet-format.yml, aot-compatibility.yml and e2e.yml into ci.yml and add a `CI Gate` job that depends on all of them. Once branch protection requires only `DCO` + `CI Gate`, build, test, coverage, format, E2E and the AOT matrix all become effectively required, and matrix legs can be added or removed without editing branch protection. Along the way this fixes several things that were silently broken: - ci.yml, code-coverage.yml and dotnet-format.yml had no `merge_group` trigger, so the merge queue never validated build, test, coverage or format against the merged result. They now run there. - ci.yml and code-coverage.yml used workflow-level `paths-ignore: "**.md"`. A workflow skipped by path filtering reports no check runs at all, so a required check inside it would leave docs-only pull requests blocked forever. The filter is gone. - The gate uses `if: always()`. Without it the job inherits the implicit `success()` condition and is *skipped* when a dependency fails - and branch protection treats a skipped check as satisfied, which would let exactly the broken pull requests through. The jq predicate also fails on `cancelled` and on any skip that is not explicitly allow-listed. - `github.event.pull_request.head.repo.fork == false` is true on `push` and `merge_group` because GitHub coerces the missing value to 0 before comparing. Combined with `startsWith(github.ref, 'refs/heads/')`, which matches `refs/heads/gh-readonly-queue/*`, adding a merge_group trigger would have published a NuGet package for every queue entry. The packaging job now checks `github.event_name` explicitly, always packs as a validation step, and publishes only on push or a non-fork pull request. - Merge queue runs are never cancelled by the concurrency group; a cancelled run reports a non-success conclusion and ejects the pull request from the queue. Coverage is skipped on `merge_group` - the `gh-readonly-queue/*` ref is thrown away, so uploading it to Codecov is meaningless and `build` already runs the same tests there. It is the only entry in the gate's allow-list. The E2E job deliberately keeps the bare `e2e-tests` id so its check run name is unchanged; it is a required context today and a required check that stops reporting blocks every open pull request. Duplicated setup-dotnet and NuGet cache blocks move into a local composite action. Branch protection still has to be updated after this merges. Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
Follow-up to the CI Gate consolidation. Rather than one long ci.yml, each unit
of work becomes a `workflow_call` building block and ci.yml is reduced to an
orchestrator that wires them together and evaluates the gate.
ci.yml
build -> reusable-build.yml
coverage -> reusable-coverage.yml
format -> reusable-format.yml
e2e -> reusable-e2e.yml
aot -> reusable-aot.yml
packaging -> reusable-packaging.yml
ci-gate -- the single required status check
The gate still has to live in ci.yml because `needs:` cannot reference a job in
another workflow file. Everything else moves out.
Beyond readability, actionlint cross-validates the calls, so a typo in an input
or secret name becomes a lint error rather than a runtime failure.
The blocks take no inputs. They have exactly one caller in this repository, so
parameters would only ever be passed their own defaults. The one thing that does
have to cross the boundary is the Codecov token: secrets are not visible to a
called workflow unless they are passed explicitly.
One consequence worth calling out: check runs produced through a reusable
workflow are named `<caller job> / <called job>`, so the checks are now
`Build / ubuntu-latest`, `AOT / linux-arm64`, `Format / dotnet format` and so
on. That means the `e2e-tests` context cannot be preserved the way it was in
the previous commit, and branch protection has to drop it before this merges
rather than after. `CI Gate` is unaffected - it is a plain job in ci.yml, so its
name has no prefix and stays stable as jobs move around underneath it.
Reusable workflows are `workflow_call`-only so they never self-trigger, and
concurrency stays in the caller alone - sharing a group between caller and
callee makes a run cancel itself.
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
|
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:
📝 WalkthroughWalkthroughThe pull request adds a shared .NET setup action, converts CI checks to reusable workflows, adds reusable packaging and E2E workflows, and centralizes workflow result evaluation in ChangesCI workflow rearchitecture
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CIWorkflow
participant ReusableWorkflows
participant GitHubPackages
participant CIGate
CIWorkflow->>ReusableWorkflows: invoke reusable validation and packaging workflows
ReusableWorkflows->>GitHubPackages: publish or upload generated packages
ReusableWorkflows-->>CIWorkflow: return job results
CIWorkflow->>CIGate: pass gated results
CIGate-->>CIWorkflow: record results and enforce failures
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #801 +/- ##
=======================================
Coverage 93.55% 93.55%
=======================================
Files 66 66
Lines 3010 3010
Branches 378 378
=======================================
Hits 2816 2816
Misses 130 130
Partials 64 64 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Not ready to approve
The solution file still references three workflow paths deleted by this refactor.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Consolidates CI checks behind a reusable-workflow orchestrator and a single required CI Gate.
Changes:
- Adds reusable workflows and shared .NET setup.
- Gates build, coverage, formatting, E2E, AOT, and packaging.
- Adds merge-queue support and safer package publishing.
File summaries
| File | Description |
|---|---|
.github/workflows/ci.yml |
Orchestrates checks and evaluates the CI gate. |
.github/workflows/reusable-build.yml |
Runs builds and unit tests. |
.github/workflows/reusable-coverage.yml |
Runs and uploads coverage. |
.github/workflows/reusable-format.yml |
Verifies formatting. |
.github/workflows/reusable-e2e.yml |
Runs E2E tests. |
.github/workflows/reusable-aot.yml |
Runs the AOT matrix. |
.github/workflows/reusable-packaging.yml |
Validates and conditionally publishes packages. |
.github/workflows/e2e.yml |
Removes the standalone E2E workflow. |
.github/actions/setup-dotnet/action.yml |
Centralizes SDK and NuGet-cache setup. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
The reusable-workflow split left OpenFeature.slnx pointing at code-coverage.yml, dotnet-format.yml and e2e.yml, which no longer exist. Repoint them at their reusable-* successors and list the two remaining new workflows so the folder covers the whole CI surface. Also drops codeql-analysis.yml, a dead entry left behind when that workflow was deleted in #700, and adds dco-merge-group.yml, which was never listed. Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Human review recommended
Correctness depends on live merge-queue behavior and a coordinated branch-protection update.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/reusable-format.yml (1)
10-12: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDrop the unused
pull-requests: writepermission.
dotnet format --verify-no-changesonly reads the repository and writes to the job log. Removepull-requests: writehere and in theformatjob of.github/workflows/ci.ymlto keep least privilege.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/reusable-format.yml around lines 10 - 12, Remove the unused pull-requests: write permission from the permissions block in reusable-format.yml and from the format job in ci.yml, retaining contents: read so the formatting workflow remains read-only..github/workflows/reusable-build.yml (1)
42-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the sample publish step.
The step name says
aot-publish test, but the command publishes the AspNetCore sample without any AOT or runtime-identifier option. AOT verification runs in.github/workflows/reusable-aot.yml. Rename the step to describe what it does, for examplePublish AspNetCore sample.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/reusable-build.yml around lines 42 - 43, Rename the workflow step currently labeled “aot-publish test” to accurately describe the standard AspNetCore sample publish, such as “Publish AspNetCore sample”; leave the dotnet publish command unchanged..github/workflows/reusable-packaging.yml (1)
51-53: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valuePass the token through
envinstead of template expansion.Interpolating
secrets.GITHUB_TOKENinto therunbody inserts the value into the generated script. Bind it to an environment variable and reference the variable, which also removes the template-injection warning.🔒 Proposed fix
- name: Publish NuGet packages (base) if: github.event_name == 'push' || (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork == false) - run: dotnet nuget push "src/**/*.nupkg" --api-key "${{ secrets.GITHUB_TOKEN }}" --source https://nuget.pkg.github.com/open-feature/index.json + env: + NUGET_API_KEY: ${{ secrets.GITHUB_TOKEN }} + run: dotnet nuget push "src/**/*.nupkg" --api-key "$NUGET_API_KEY" --source https://nuget.pkg.github.com/open-feature/index.json🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/reusable-packaging.yml around lines 51 - 53, Update the “Publish NuGet packages (base)” step to pass secrets.GITHUB_TOKEN through its env configuration, then reference that environment variable in the dotnet nuget push command instead of expanding the secret directly in run. Preserve the existing publish condition and package source.Source: Linters/SAST tools
.github/workflows/ci.yml (1)
91-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe gate cannot detect a job that is missing from
needs.The jq filter checks only the jobs listed in
needs. If a contributor adds a job to this workflow and forgets to add it toneeds, the gate reports success while that job fails. The comment at lines 84-86 promises that jobs can be added without detaching branch protection, so make the check enforce it.Add a step that compares the job keys in this file against the
needslist, or document the requirement in the comment.♻️ Example enforcement step
steps: + - name: Checkout + uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 + with: + persist-credentials: false + sparse-checkout: .github/workflows/ci.yml + + - name: Check that every job is gated + env: + NEEDS_JSON: ${{ toJSON(needs) }} + run: | + all=$(yq -r '.jobs | keys | .[]' .github/workflows/ci.yml | grep -v '^ci-gate$' | sort) + gated=$(echo "$NEEDS_JSON" | jq -r 'keys[]' | sort) + missing=$(comm -23 <(echo "$all") <(echo "$gated")) + if [ -n "$missing" ]; then + echo "::error::Jobs missing from ci-gate needs: $missing" + exit 1 + fi + - name: Check that every gated job succeeded🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ci.yml around lines 91 - 124, Update the CI Gate workflow around the ci-gate job so it validates that every job declared in this workflow is included in needs, rather than checking only NEEDS_JSON. Add a validation step that compares the workflow’s job keys with the needs list and fails with the missing job names before the existing result check runs; preserve the current allowed-skips behavior.OpenFeature.slnx (1)
24-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the missing workflow/action entries.
The listed workflow entries exist, but
.github/actions/setup-dotnet/action.ymlis present on disk while.github/actions/sbom-generator/action.ymlhas no entry. Add the missing entry or remove the file so the solution and repository tree stay aligned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@OpenFeature.slnx` around lines 24 - 33, Add the missing .github/actions/sbom-generator/action.yml entry to OpenFeature.slnx so the solution includes every repository workflow/action file present on disk. Keep the existing workflow entries unchanged and align the solution tree with the actual repository contents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/reusable-build.yml:
- Around line 24-28: Disable credential persistence in the Checkout steps of all
five reusable workflows by setting persist-credentials to false:
.github/workflows/reusable-build.yml lines 24-28, reusable-coverage.yml lines
25-28, reusable-e2e.yml lines 17-21, and reusable-packaging.yml lines 17-21; add
a with block containing the same setting in reusable-format.yml lines 17-18. Do
not alter the workflows’ existing checkout behavior or authentication used by
later steps.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 91-124: Update the CI Gate workflow around the ci-gate job so it
validates that every job declared in this workflow is included in needs, rather
than checking only NEEDS_JSON. Add a validation step that compares the
workflow’s job keys with the needs list and fails with the missing job names
before the existing result check runs; preserve the current allowed-skips
behavior.
In @.github/workflows/reusable-build.yml:
- Around line 42-43: Rename the workflow step currently labeled “aot-publish
test” to accurately describe the standard AspNetCore sample publish, such as
“Publish AspNetCore sample”; leave the dotnet publish command unchanged.
In @.github/workflows/reusable-format.yml:
- Around line 10-12: Remove the unused pull-requests: write permission from the
permissions block in reusable-format.yml and from the format job in ci.yml,
retaining contents: read so the formatting workflow remains read-only.
In @.github/workflows/reusable-packaging.yml:
- Around line 51-53: Update the “Publish NuGet packages (base)” step to pass
secrets.GITHUB_TOKEN through its env configuration, then reference that
environment variable in the dotnet nuget push command instead of expanding the
secret directly in run. Preserve the existing publish condition and package
source.
In `@OpenFeature.slnx`:
- Around line 24-33: Add the missing .github/actions/sbom-generator/action.yml
entry to OpenFeature.slnx so the solution includes every repository
workflow/action file present on disk. Keep the existing workflow entries
unchanged and align the solution tree with the actual repository contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 0fded4f8-0db1-4c5f-93e4-dd3d9ceaace4
📒 Files selected for processing (10)
.github/actions/setup-dotnet/action.yml.github/workflows/ci.yml.github/workflows/e2e.yml.github/workflows/reusable-aot.yml.github/workflows/reusable-build.yml.github/workflows/reusable-coverage.yml.github/workflows/reusable-e2e.yml.github/workflows/reusable-format.yml.github/workflows/reusable-packaging.ymlOpenFeature.slnx
💤 Files with no reviewable changes (1)
- .github/workflows/e2e.yml
…flows Set persist-credentials: false on all six checkouts. Nothing runs git after checkout - the steps are dotnet restore/build/test/pack/push, a Codecov upload over HTTPS and a file copy - so leaving the job token in .git/config only widens the blast radius of a compromised step. The spec submodule is a public HTTPS URL, so recursive checkout is unaffected. Drop pull-requests: write from build, coverage, format and e2e. It was applied uniformly in #439 when permissions blocks were first added, without a per-job need. --report-github emits workflow commands and a step summary, and codecov-action uploads with its own token; the PR comment comes from the Codecov app, not the action. lint-pr and release keep theirs because they really do write to pull requests. Pass the packaging secret and version suffix through env instead of expanding them into the run body, so neither is materialised in the generated script. Also list the two composite actions in the solution file, matching the workflows folder already there. Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/reusable-packaging.yml (1)
60-65: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRequire
nupkgsfor fork packaging steps.
actions/upload-artifactusesif-no-files-found: warnby default, so no matchedsrc/**/*.nupkgfiles still lets this step succeed. Addif-no-files-found: errorto fail fork packaging runs when packages are missing.Proposed fix
with: name: nupkgs path: src/**/*.nupkg + if-no-files-found: error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/reusable-packaging.yml around lines 60 - 65, Add if-no-files-found: error to the with configuration of the “Publish NuGet packages (fork)” upload-artifact step, ensuring fork packaging runs fail when no src/**/*.nupkg files are produced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/reusable-packaging.yml:
- Line 58: Update the package publishing step’s `dotnet nuget push` input to
target the actual `.nupkg` files produced by `dotnet pack`: use the configured
fixed output directory with proper quoting, or discover generated packages with
`find` and publish each file individually. Ensure the existing GitHub Packages
source and `NUGET_API_KEY` authentication remain unchanged.
---
Outside diff comments:
In @.github/workflows/reusable-packaging.yml:
- Around line 60-65: Add if-no-files-found: error to the with configuration of
the “Publish NuGet packages (fork)” upload-artifact step, ensuring fork
packaging runs fail when no src/**/*.nupkg files are produced.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 288b970a-4013-4e29-9c92-0b3bea00f09b
📒 Files selected for processing (8)
.github/workflows/ci.yml.github/workflows/reusable-aot.yml.github/workflows/reusable-build.yml.github/workflows/reusable-coverage.yml.github/workflows/reusable-e2e.yml.github/workflows/reusable-format.yml.github/workflows/reusable-packaging.ymlOpenFeature.slnx
💤 Files with no reviewable changes (1)
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (4)
- OpenFeature.slnx
- .github/workflows/reusable-aot.yml
- .github/workflows/reusable-coverage.yml
- .github/workflows/reusable-format.yml
There was a problem hiding this comment.
🟡 Human review recommended
The CI and merge-queue migration depends on carefully sequenced external branch-protection changes requiring human oversight.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
actions/upload-artifact defaults to if-no-files-found: warn, so a fork pull request that packed nothing would hand the contributor an empty nupkgs artifact and still report success. The artifact is the only way a fork gets the packages, and that path is rarely exercised, so a silent regression could sit unnoticed. Fail instead. Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Human review recommended
The broad CI restructuring and coordinated branch-protection migration warrant final human validation.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The PR lint workflow will fail on merge-group events because those events have no pull-request payload.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 1
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/lint-pr.yml:
- Around line 4-5: Update the workflow job containing the PR-title validation
action so merge_group events succeed without relying on
github.event.pull_request; skip that validation for merge_group runs or split it
into an event-specific job while preserving validation for pull_request events.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d77a335-e044-421c-a744-99a043d65b6f
📒 Files selected for processing (1)
.github/workflows/lint-pr.yml
Updated the linting workflow to include a separate job for merge group checks and adjusted the runner environment to ubuntu-slim. Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…ntain permissions' Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com> Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Human review recommended
The CI control-plane migration and required branch-protection sequencing warrant final human validation.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
There was a problem hiding this comment.
🔵 Human review recommended
Replacing repository-wide CI checks and coordinating branch-protection changes warrants a human-validated rollout.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The merge-group lint job is skipped before its intended queue step can run.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (1)
.github/workflows/lint-pr.yml:16
- This job-level condition makes the merge-queue step below unreachable: on every
merge_groupevent GitHub skips the entire job before evaluating its steps, so it cannot emit the intended successful validation. Either let the job run (the PR-specific action steps are already guarded) or remove themerge_grouptrigger and dummy step if this workflow is intentionally outside the queue.
if: github.event_name == 'pull_request_target'
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Signed-off-by: André Silva <2493377+askpt@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Human review recommended
The branch-protection migration and first merge-queue execution require coordinated human validation.
Review details
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Balanced
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Why
Branch protection on
mainrequires exactly two status checks:DCOande2e-tests. Everything else that runs on a pull request (build, unit tests, code coverage,dotnet format, and the six-way AOT matrix) is advisory. A pull request with a red build merges today.Separately,
ci.yml,code-coverage.ymlanddotnet-format.ymlhave nomerge_grouptrigger, so the merge queue has never validated build, test or format against the merged result. Only E2E, AOT and the dummy DCO reporter run there.This adds a single required check,
CI Gate, that sits downstream of every other job. Once branch protection requires onlyDCO+CI Gate, all of those checks become effectively required, and jobs or matrix legs can be added and removed later without anyone having to remember to edit branch protection.Approach
needs:cannot reference a job in another workflow file, so every gated job has to be reachable from one place.ci.ymlbecomes a thin orchestrator and the work moves intoworkflow_callbuilding blocks:code-coverage.yml,dotnet-format.yml,aot-compatibility.ymlande2e.ymlare folded in and deleted.lint-pr.ymlstays out of the gate on purpose: it usespull_request_targetand structurally cannot report onmerge_group, so requiring it would deadlock the queue.dco-merge-group.ymlandrelease.ymlare untouched.The blocks take no inputs. They have one caller, so parameters would only ever receive their own defaults. The Codecov token is passed explicitly because secrets are not visible to a called workflow otherwise.
A
setup-dotnetcomposite action absorbs the repeated setup-dotnet plus NuGet cache blocks.Things fixed along the way
ci.ymlandcode-coverage.ymlused workflow-levelpaths-ignore: "**.md". A workflow skipped by path filtering reports no check runs at all, so a required check inside one would leave docs-only pull requests blocked forever. Removed.if: always(). Without it the job inherits the implicitsuccess()condition and is reported as skipped when a dependency fails, and branch protection treats a skipped check as satisfied. That would let through exactly the pull requests it is meant to stop. The predicate also fails oncancelledand on any skip that is not explicitly allow-listed.github.event.pull_request.head.repo.fork == falseevaluates to true onpushandmerge_group, because the missing value is coerced to 0 before the comparison. Combined withstartsWith(github.ref, 'refs/heads/'), which also matchesrefs/heads/gh-readonly-queue/*, simply adding amerge_grouptrigger would have published a NuGet package for every queue entry. The packaging job now checksgithub.event_nameexplicitly, always packs as a validation step, and publishes only onpushor a non-fork pull request.cancel-in-progressis now scoped topull_request. A cancelled run reports a non-success conclusion and ejects the pull request from the queue.id-token: writeandattestations: writepermissions from packaging, which has no attest step.Coverage is skipped on
merge_group: thegh-readonly-queue/*ref is thrown away, so uploading it to Codecov is meaningless, andbuildalready runs the same tests there. It is the only entry in the gate's skip allow-list.Required before merging this
Check runs produced through a reusable workflow are named
<caller job> / <called job>, so thee2e-testscontext goes away with this change. A required context that stops reporting does not fail, it stays pending, which means this pull request cannot merge itself whilee2e-testsis still required.So, unusually, the branch protection edit has to happen before the merge, not after:
Two approvals, DCO, conversation resolution and the merge queue's ALLGREEN strategy all still apply in between, so the window is narrow.
Worth doing at the same time: lower the merge queue
checkResponseTimeoutfrom 3600s, which is a long time to wait for a check that is never coming.v1should be left alone. Workflows for a pull request come from its base branch, sov1pull requests still usev1's own 2024-era workflow files and requiringCI Gatethere would deadlock.New check names
Build / ubuntu-latest,Build / windows-latest,Coverage / ubuntu-latest,Coverage / windows-latest,Format / dotnet format,E2E / dotnet test,AOT / {linux,win,osx}-{x64,arm64},Packaging / dotnet pack, andCI Gate.Notes for review
.github/workflows/. GitHub does not support subdirectories for them, and a subdirectory reference fails at run time rather than being caught by a linter, so the filename prefix is standing in for a folder.buildruns Release.buildandcoveragetherefore run the same suite twice. Worth merging later, but it would move the Codecov numbers, so it is deliberately not in this change.actionlint, which also cross-validates the reusable workflow inputs and secrets, and withzizmor. The gate predicate was exercised locally against all-green, coverage-skipped, failure, cancelled and unexpected-skip inputs.pull-requests: writeis dropped from build, coverage, format and e2e. It was applied uniformly in chore(workflows): Add permissions for contents and pull-requests #439 when permissions blocks were first added, rather than per job.--report-githubemits workflow commands and a step summary, and codecov-action uploads with its own token, so none of the four touch the pull request API.lint-prandreleasekeep theirs.persist-credentials: false. Nothing runs git after checkout, so there is no reason to leave the job token in.git/config.OpenFeature.slnxhad stale entries for the deleted workflows, plus a deadcodeql-analysis.ymlleft behind by ci: remove CodeQL analysis workflow file #700. Repointed at thereusable-*files and added the two composite actions.