Skip to content

Validate PR milestone against the target branch - #4610

Open
paulmedynski wants to merge 6 commits into
mainfrom
dev/paul/milestone-check
Open

Validate PR milestone against the target branch#4610
paulmedynski wants to merge 6 commits into
mainfrom
dev/paul/milestone-check

Conversation

@paulmedynski

@paulmedynski paulmedynski commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Description

The Check Milestone workflow only verified that a PR had an open milestone assigned. It accepted nonsensical combinations, such as a PR targeting main with milestone 7.0.3 — that work belongs on release/7.0.

This adds a target-branch consistency check. Rather than hard-coding a version table that needs maintaining every release, the expected branch is derived from the milestone:

  • A milestone X.Y.Z (with an optional pre-release suffix) maps to release/X.Y.
  • If that branch exists, the version has forked off the default branch and is in servicing, so the PR must target release/X.Y.
  • If that branch does not exist, the version is still in development, so the PR must target the default branch.

This is self-maintaining: cutting release/7.1 automatically moves 7.1.0 enforcement from main to release/7.1, with no workflow change.

Validation matrix

PR target release/X.Y exists? Result
release/X.Y (matching) n/a pass
a different release/* n/a fail — milestone/branch mismatch
default branch no pass
default branch yes fail — retarget, or use a Hotfix X.Y.Z label
anything else (dev/...) n/a skipped

PRs into long-lived integration branches are skipped, because the milestone is enforced when that branch is merged into main or a release branch. Milestones that don't parse as major.minor.patch (e.g. 1.0 Hotfix 2) emit a ::notice:: rather than failing.

The existing hotfix flow is unaffected: a change that lands in main and is cherry-picked carries an in-development milestone plus a Hotfix X.Y.Z label, which the failure message points at.

Reconciling already-open PRs

Cutting release/X.Y flips the expected target for every X.Y.* milestone, but creating a branch emits no pull request activity, so an already-open PR would keep the verdict it last recorded. recheck-milestones.yml closes that gap: on create of a release/* branch it finds the affected open PRs and re-runs their milestone check. Re-running is sufficient because the check queries the live branch list, so the replayed run produces the updated verdict on the same check name that gates the merge.

It lives in a separate workflow so check-milestone.yml stays purely pull-request scoped, and so the actions: write permission needed to re-run checks is isolated from the PR gate. create supports no branch filter, so the job guard skips anything that isn't release/*.

Trigger

check-milestone.yml stays on pull_request (with reopened added alongside the existing types). edited covers base-branch changes, so retargeting a PR — manually, or automatically when a stacked PR's parent merges — re-runs the check.

pull_request_target was considered and rejected. It evaluates the workflow file on the base branch, which means a change to this check can never be exercised by the PR that makes it — confirmed on this PR, where the milestone check ran on neither event until the trigger was reverted. Its one real advantage, preventing a PR author from disabling the check in their own PR, is already covered by the three-reviewer requirement on .github changes.

Issues

None.

Testing

  • .github/scripts/tests/check-milestone-branch.bats — 21 Bats tests covering version parsing, release-branch derivation, default-branch vs release-branch validation, integration-branch and non-semver skips, a newly cut release branch, the exact gh invocation, and API failure handling.
  • .github/scripts/tests/recheck-milestones-for-release-branch.bats — 15 Bats tests covering release-branch name parsing, milestone matching, run lookup by head SHA, re-run invocation, and failure reporting.
  • Replayed the rule against real repository data: every open PR and the 100 most recently updated closed PRs in dotnet/SqlClient that carry a milestone. Zero false positives.
  • Exercised end-to-end on this PR: milestone 7.1.0 targeting main passed with ::notice::Milestone '7.1.0' is still in development (no 'release/7.1' branch); targeting 'main' is correct., and temporarily setting 7.0.3 failed with the expected release/7.0 error before being restored.

The recheck-milestones.yml path cannot be exercised until a release branch is actually cut, so it degrades loudly — a ::warning:: per PR and a non-zero exit — rather than failing silently.

One known limitation, documented in the script header: a re-run replays the original run's commit, so it executes the workflow and script as they were then, with only the release-branch lookup evaluated live. A PR whose last milestone check predates a change to the check itself will replay the older version and keep its stale result. The window is transitional — any PR with activity after the change has a run containing it — so this is recorded rather than detected, which would cost an extra API call per PR. After the first release branch cut following a change to the check, review the affected PRs by hand.

The milestone check only verified that an open milestone was assigned. A PR
targeting main with a servicing milestone such as 7.0.3 was accepted even
though that work belongs on release/7.0.

Derive the expected branch from the milestone instead of hard-coding a version
table: milestone X.Y.Z maps to release/X.Y, and whether that branch exists
decides where the change belongs. Existing branch means the version is in
servicing and the PR must target it; no branch means the version is still in
development on the default branch. PRs into integration branches and
milestones that are not major.minor.patch are skipped.

Switch the trigger to pull_request_target so the check still runs while a PR
has a merge conflict, which is a common state right after a stacked PR is
retargeted.
Copilot AI balanced review requested due to automatic review settings August 28, 2026 11:06
@github-project-automation github-project-automation Bot moved this to To triage in SqlClient Board Aug 28, 2026
@paulmedynski paulmedynski added this to the 7.1.0 milestone Aug 28, 2026
@paulmedynski paulmedynski moved this from To triage to In progress in SqlClient Board Aug 28, 2026
@paulmedynski paulmedynski added the Area\Engineering Use this for issues that are targeted for changes in the 'eng' folder or build systems. label Aug 28, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds milestone-to-target-branch validation for pull requests.

Changes:

  • Derives release branches from semantic-version milestones.
  • Moves validation to pull_request_target.
  • Adds 19 Bats tests and test documentation.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
.github/workflows/check-milestone.yml Integrates branch validation workflow.
.github/scripts/check-milestone-branch.sh Implements milestone/branch validation.
.github/scripts/tests/check-milestone-branch.bats Tests validation scenarios.
.github/scripts/tests/README.md Documents the new test suite.
Suppressed comments (1)

.github/workflows/check-milestone.yml:58

  • A no-ref checkout in a current pull_request_target run uses the repository's default branch rather than the PR's target branch. Naming that source explicitly avoids implying that release/X.Y or another target branch supplies the executed script.
          # Checks out the base branch, not the PR head, so the script is trusted.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread .github/workflows/check-milestone.yml Outdated
Comment thread .github/scripts/tests/check-milestone-branch.bats
Comment thread .github/workflows/check-milestone.yml Outdated
Copilot AI review requested due to automatic review settings August 28, 2026 11:20
pull_request_target evaluates the workflow file on the base branch, so this
check could never be exercised by the PR that changes it. Tamper resistance is
already provided by requiring three reviewers on .github changes, and a
conflicted PR cannot be merged until the conflict is resolved, which fires a
synchronize event and re-runs the check.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comment thread .github/workflows/check-milestone.yml
- Assert the gh invocation in the milestone check tests. The stub answered
  every command identically, so a malformed endpoint or --jq expression would
  still have passed. It now logs its arguments, matching the cherry-pick
  suite, and two tests pin the exact call and the release-branch fast path.

- Re-check open PRs when a release branch is cut. Creating release/X.Y flips
  the expected target for X.Y.* milestones but emits no pull request activity,
  so an already-open PR kept a stale result. A new job re-runs the milestone
  check for the affected PRs; because the check queries the live branch list,
  replaying the run yields the updated verdict.
Keeping the 'create' trigger in check-milestone.yml meant every branch creation
produced a run named "Check Milestone" with the PR job skipped, cluttering the
history of the workflow that gates pull requests. Moving the job to
recheck-milestones.yml keeps check-milestone.yml purely pull request scoped and
isolates the 'actions: write' permission from the PR gate.

A 'create' run defaults to the newly created branch, so the checkout now pins
the default branch rather than relying on the new branch carrying the script.
Copilot AI review requested due to automatic review settings August 28, 2026 12:24

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

.github/scripts/recheck-milestones-for-release-branch.sh:110

  • Re-running an existing workflow run preserves that run's original GITHUB_SHA/GITHUB_REF and workflow definition. Therefore, an open PR whose latest milestone run predates this change will rerun the old existence-only check, leaving its green result intact after release/X.Y is created. The reconciliation path needs to execute the new validation for the current PR head rather than assuming any historical run contains it.
  if gh run rerun "${RUN_ID}" --repo "${GITHUB_REPOSITORY}" >/dev/null 2>&1; then

Comment thread .github/scripts/recheck-milestones-for-release-branch.sh Outdated
Copilot AI review requested due to automatic review settings August 28, 2026 12:36
A single head commit can back pull requests against several bases, so taking
the newest run for the SHA could re-run a different PR's payload and leave the
intended PR unreconciled. Prefer the run whose pull_requests entry names the
PR being reconciled.

Runs originating from forked repositories carry an empty pull_requests array,
so the newest run for the SHA remains the fallback rather than reporting a
failure for every fork PR.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

.github/scripts/check-milestone-branch.sh:126

  • matching-refs is paginated (30 refs by default), but this call reads only the first page. Once the repository has more than 30 release/* branches, a later branch can be missed and its serviced milestone will incorrectly pass against the default branch. Query the exact candidate ref, or add --paginate (and update the invocation test) so the check remains self-maintaining.
if ! RELEASE_REFS=$(gh api "repos/${GITHUB_REPOSITORY}/git/matching-refs/heads/release/" \
    --jq '.[].ref' 2>&1); then

#
#################################################################################

name: Recheck Milestones

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't test this without creating new branches post-PR-merge. I will eyeball the workflow results as we create new dev/ branches, and ensure we skip the processing in that case. We will have to wait until we create release/7.1 to test the full recheck flow.

Copilot AI review requested due to automatic review settings August 28, 2026 13:04
A re-run replays the original run's commit, so it executes the workflow and
script as they were then; only the release branch lookup is live. A pull
request whose last milestone check predates a change to the check itself will
therefore replay the older version and keep its stale result.

The window is transitional and closes as pull requests see activity, so this
records the limitation rather than paying an extra API call per pull request
to detect it.
@paulmedynski

Copy link
Copy Markdown
Contributor Author

Disposition of the suppressed review comments

Copilot filed three low-confidence comments inside its review bodies rather than as threads, so there is nowhere to reply to them individually. Recording the outcome here for reviewers.

1. check-milestone.yml:58 — a no-ref checkout under pull_request_target uses the default branch, not the PR target. No action. This duplicates thread r3880112947, and the comment it objected to was deleted in 0038c79 when the trigger reverted to pull_request. The underlying point does survive in recheck-milestones.yml, where a create run defaults to the newly created branch — that checkout now pins ref: ${{ github.event.repository.default_branch }} explicitly.

2. recheck-milestones-for-release-branch.sh:110 — a re-run replays the original workflow definition, so a PR whose last check predates this change keeps its stale result. Accepted. This is correct and it contradicted an assurance I had given on thread r3880112891, which is corrected in r3880792007. The limitation is documented in d75274b, in a LIMITATION section in the script header with a pointer from the workflow and a note in the PR description. We chose to record it rather than build a detector or move to the Checks API, because the window is transitional and closes as PRs see activity.

3. check-milestone-branch.sh:126matching-refs is paginated at 30 refs, so later release branches could be missed. No action; this one is incorrect. The endpoint ignores per_page and returns every matching ref:

Call Refs returned
matching-refs/heads/release/ 12
matching-refs/heads/release/?per_page=1 12
matching-refs/heads/ (170 branches in this repo) 170
matching-refs/heads/?per_page=5 170

Asking for 5 of 170 returned all 170, so there is no first-page truncation to guard against and adding --paginate would change nothing.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

@paulmedynski
paulmedynski marked this pull request as ready for review August 28, 2026 13:10
@paulmedynski
paulmedynski requested a review from a team August 28, 2026 13:10
@paulmedynski paulmedynski moved this from In progress to In review in SqlClient Board Aug 28, 2026
@paulmedynski
paulmedynski enabled auto-merge (squash) August 28, 2026 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area\Engineering Use this for issues that are targeted for changes in the 'eng' folder or build systems.

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

5 participants