Fixes #2891

[erikdarlingdata]

Adds the commit-identity gate from #2891 to the existing check-branches job.

What it does

.github/workflows/check-pr-branch.yml now calls .github/scripts/check-commit-identity.sh, which reads the pull request's commit list from GET /repos/{owner}/{repo}/pulls/{number}/commits (not from a checkout of the pull request) and fails on any commit whose commit.author.email or commit.committer.email is off a pinned allowlist. Both fields, because a rebase or an amend routinely leaves one right and the other wrong.

Errors name the short SHA and which of the two fields was wrong, and deliberately not the address: Actions logs on a public repo are public, so echoing it would publish the identity the check exists to keep out. The remediation block shows how to read the value locally and how to rewrite the branch with --reset-author, which fixes both fields at once.

This check cannot run on its own pull request

pull_request_target executes the workflow file as it exists on the base branch. The check-branches run on this pull request is dev's current copy, which has no identity step, so a green check-branches here is not evidence that any of this works. It takes effect only once merged.

So it was verified by running the extracted script directly against real pull request data from the API:

Case	Pull request	Declared commits	Result
Non-conforming author and committer	a merged one-commit pull request	1	exit 1, two ::error:: lines (one per field)
Non-conforming, multi-commit	a merged five-commit pull request	5	exit 1, ten ::error:: lines
Clean, single page	#2900	3	exit 0
Clean, three pages	#1250	250	exit 0, 250 commit(s) over 3 page(s)
Dependabot	#2702	1	exit 0
Above the endpoint's cap	#1611	926	exit 1, coverage failure (API returns 250)

Three corrections to the design in #2891

Each was checked against this repo's actual history rather than assumed.

1. GitHub's committer here is noreply@github.com, not web-flow@github.com. The issue says to allowlist web-flow@github.com or the check fails on every merge commit. That address appears in this repo's history zero times; all 346 GitHub-created merge commits and all 80 squash merges use noreply@github.com. Allowlisting only web-flow yields 101 violations on the 250 commits of #1250, a pull request that is entirely clean:

allowlist without noreply@github.com -> violations: 101
allowlist as shipped                 -> violations: 0

Both are pinned now: noreply@github.com because it is what this repo actually produces, web-flow@github.com because it costs nothing and GitHub still emits it on some paths.

2. Dependabot had to be pinned, or the check would red every Monday. #2891 describes one intended identity. But .github/dependabot.yml has Dependabot opening grouped pull requests against dev weekly, and it authors those commits itself (committer is GitHub, author is Dependabot). With the issue's allowlist taken literally, #2702 fails on its author field. Its numeric prefix was confirmed to be the real global account id via /users/dependabot[bot].

3. The 250-commit cap is not hypothetical in this repo. #2891 calls it "far above anything here". This repo has pull requests at 926, 607, 536, 452, 446, 304 and 304 commits. Rather than pass on the unread tail, the script compares what it read against the count the pull request declares and reports a coverage failure if they differ — a preventive gate that cannot cover its subject should say so rather than report a pass over the part it happened to read. This is a judgement call and the one thing here worth overruling if you would rather a very large pull request warned instead of failed.

Also worth knowing

• Outside contributions will now be red. This is what "one intended commit identity" means, but it is a policy consequence rather than a bug: history shows several merged contributions from other identities. Nothing here exempts forks.
• The older erikdarlingdata@users.noreply.github.com form of the maintainer address (one commit in history, no numeric prefix) is not allowlisted. Pinning one exact string is the point; noting it in case that is wrong.
• A synthetic case for a different account's @users.noreply.github.com was confirmed to fail, which is the address-shape gap Add a CI check on PR commit author identity #2891 names as the reason not to pattern-match.
• The job gained an explicit read-only permissions: block and a base-ref checkout. The checkout pins ref: ${{ github.base_ref }} explicitly rather than relying on the default, so it cannot quietly start resolving to pull request head; no pull request content is fetched or executed.
• One real bug was caught only by running it: inside jq's index(...), . refers to the allowlist array, not the commit field, so the first version died with Cannot index array with string "email" on every pull request. It failed closed, but it was broken.

Not done, deliberately

No existing commit was rewritten. Per #2891 the scope is making future ones impossible; history is out of scope.

Schema version untouched (109), no migration.

Review response (both acted on, neither dismissed)

Anchoring the read to the event head. The coverage assertion compared the paginated commit list against .commits from GET /pulls/{n} — two endpoints in the same family, so a stale post-push view could leave them agreeing with each other while both described the previous head, and comparing them only to each other cannot see that. The read must now also contain the event's head SHA, which is an independent anchor and fails closed: no anchor, no verdict, red, and a re-run clears it. That matches the rest of the script's stance that a false red costs a re-run while a false green is permanent. Skipped when HEAD_SHA is absent, which is how the script runs when pointed at an arbitrary pull request by hand.

Worth noting that this staleness is not theoretical: immediately after pushing the fix, GET /pulls/2904 still reported the previous headRefOid and mergeable: CONFLICTING for a while before catching up. Verified both directions — anchored on the real head SHA it passes and says so; anchored on a SHA absent from the list it fails with the staleness message.

Page over-count. pages_read counted the trailing empty-page probe, so a commit count that is an exact multiple of the page size reported one page too many. It now counts only pages that returned commits. Verified against the patched loop structure: 100 commits now report 1 page (was 2), 200 report 2 (was 3), 250 still report 3, 3 still report 1.

What this check is not

Raised in review and now stated in the script header as well, because it would be easy to read more into a green result than it carries: commit.author.email and commit.committer.email are self-declared git fields that nothing authenticates. Anyone who can push can set user.email to an allowlisted value and pass. What this catches is a misconfigured clone — the wrong user.email left set on a machine, which is the case #2891 was opened about — and nothing more. It is not an impersonation control, and a pass here is not evidence that a commit came from whoever it names. Enforced commit signing is the only thing that gets you that.

[claude]

Possible false-green risk from GitHub API staleness on pull_request_target + synchronize.

declared comes from GET /pulls/{n} (.commits), a field GitHub computes somewhat asynchronously after a push — it's not guaranteed to reflect a force-push instantaneously. If this job starts running before GitHub's backend has caught up to the new head, two things could both under-report the same stale state: declared reads the old commit count, and the /pulls/{n}/commits list (read earlier in the loop) could likewise still be serving the pre-push set. In that case read_count == declared even though neither reflects the commits actually being merged, and the coverage assertion — the safety net this script relies on to avoid a false pass on a truncated read — wouldn't catch it, since both numbers are stale in the same direction.

Given the design's stated principle ("a false red costs a re-run; a false green is permanent"), it might be worth hedging against this specifically — e.g. compare against github.event.pull_request.head.sha (available in the calling workflow) by checking that the last commit returned by the paginated list actually matches the event's head SHA, rather than trusting the /commits list's completeness relative to a same-family stat that could share its staleness.

This is a plausibility concern from reading GitHub's API behavior, not something reproduced against this script — flagging for awareness rather than asserting it's confirmed to happen.

[claude]

Minor: pages_read can overcount by one in the success message. When the commit count is an exact multiple of PER_PAGE (e.g. exactly 100 commits), the loop reads a full page (n == 100, no break), increments page, then makes one more request that returns n == 0 and breaks — so page is 2 even though only 1 page actually had content. The final message would then say "100 commit(s) over 2 page(s)" when it was really 1 page plus an empty probe. Cosmetic only (doesn't affect the pass/fail verdict), but worth a pages_read=$((page - 1)) style fix if the empty-probe path was taken, since this string ends up in the job's log/summary as the audit trail for "how much was actually read."

[claude]

Reviewed. This is a CI-only change (no T-SQL, no Lite/Darling app code), so the T-SQL style conventions and parity rule don't apply here — scoping the review to the workflow/script itself.

Overall: the design is sound — reading commits from the API instead of a checkout (respecting pull_request_target's security model), checking both author and committer, the coverage assertion against the 250-commit API cap, and not echoing the offending address into public logs are all good calls, and match what the PR description walks through in detail.

Two things worth a look, both left as inline comments:

1. Possible false-green path: declared (GET /pulls/{n} → .commits) and the paginated /commits list are two different endpoints that could both be serving a stale pre-force-push view if this runs before GitHub's backend catches up — in which case they'd agree with each other while being wrong together, and the coverage assertion wouldn't catch it since it only compares them to each other. Anchoring the read against github.event.pull_request.head.sha would close this. Flagged as a plausibility concern, not something reproduced.
2. Minor: pages_read over-counts by one when the commit count is an exact multiple of 100 (a trailing empty-page probe bumps the page counter). Cosmetic only — shows up in the log/summary line, doesn't affect the pass/fail verdict.

No correctness issues found in the allowlist logic, the jq filter, or the workflow's permission scoping (contents: read + pull-requests: read is appropriately minimal, and the base-ref pin/persist-credentials: false are correctly defensive for pull_request_target).

[claude]

Worth stating explicitly somewhere (comment or PR description): git commit.author.email / commit.committer.email are self-declared by whoever runs git commit, not authenticated in any way. Anyone with push access to a branch can run:

git config user.email "2136037+erikdarlingdata@users.noreply.github.com"
git config user.name "erikdarlingdata"

and every commit they make afterward will sail through this check with the exact maintainer identity, GitHub's own noreply@github.com, or Dependabot's. Nothing here (and nothing GitHub provides short of enforced commit signing + vigilant mode) binds these fields to the actual pusher.

That's fine if the goal (per #2891, "a commit made under an unintended identity landed on dev") is catching accidental misconfiguration — a contributor's laptop having the wrong user.email set. But several of the comments in this file lean on stronger language ("the exact failure this check exists to stop", "the precise failure mode #2891 names") that reads like an anti-impersonation control. It isn't one, and a reader relying on it as such would be wrong. Might be worth a one-line caveat so nobody later treats a green check here as proof the commit really came from who it claims.

[claude]

Reviewed this against CONTRIBUTING.md conventions, Lite/Darling parity, correctness, and security. Summary:

• Scope: pure CI infrastructure (workflow + a new bash script + CHANGELOG) — no T-SQL, no Lite/Darling app code touched, so the app-parity rule doesn't apply here.
• pull_request_target hardening looks correct: job permissions are pinned read-only (contents: read, pull-requests: read), the base-ref checkout is explicitly pinned to github.base_ref (never PR head), persist-credentials: false is set, and the script never checks out or executes anything from the PR branch — it only reads commit metadata via gh api and does string comparisons through jq. No shell-injection surface: REPO/PR/HEAD_SHA come from GitHub-controlled event context, and all untrusted JSON values flow through jq --argjson/-r rather than being interpolated into command text.
• Pagination/coverage/anchor logic checks out: the loop correctly stops on a short page, the coverage assertion catches the >250-commit case, and the head-SHA anchor correctly guards against a stale post-push view of the PR (verified by tracing the order of operations — coverage check then anchor check, so a fully-stale view is still caught by the anchor).
• One thing worth calling out explicitly (left as an inline comment on the allowlist block): commit.author.email/commit.committer.email are self-declared, unauthenticated git metadata. Anyone with push access can set user.email locally to an allowlisted address (including the maintainer's own) and pass this check trivially. That's consistent with the stated goal in Add a CI check on PR commit author identity #2891 (catching accidental wrong-identity commits), but some of the code comments use stronger "impersonation-stopping" language than the mechanism actually delivers — worth a one-line caveat so it isn't read as a stronger guarantee than it is.

No correctness bugs found beyond that; the two issues called out in the PR description's own review-response section (page over-count, stale-view anchoring) are both correctly fixed in this diff.

[claude]

Reviewed. This PR touches only CI infrastructure (.github/scripts/check-commit-identity.sh, .github/workflows/check-pr-branch.yml, CHANGELOG.md) — no T-SQL, no Lite/Darling application code, so the style/parity conventions this repo usually gets reviewed against don't apply here; noting that explicitly rather than silently skipping it.

Correctness/logic — walked the pagination loop, the coverage assertion, and the HEAD_SHA anchor check line by line. All hold up:

• pages_read correctly counts only pages that returned commits (the fix described in the PR body).
• The coverage assertion (read_count < declared) and the HEAD_SHA anchor are complementary, not redundant: the anchor catches the specific staleness case (two same-family endpoints agreeing with each other while both describe the previous head) that the coverage check alone cannot see.
• set -euo pipefail + no retry on gh api failure is a deliberate fail-closed design and is applied consistently.
• The allowlist comparison is downcased on both sides, and the Dependabot entry's numeric prefix matches the real dependabot[bot] account id (49699333).

Security — this is the part that most needed scrutiny given pull_request_target:

• The added permissions: block is correctly scoped to contents: read / pull-requests: read.
• The checkout step pins ref: ${{ github.base_ref }} explicitly and sets persist-credentials: false — head content is never checked out or executed, and commit identity is read from the API rather than a local clone, consistent with how this trigger type must be handled.
• HEAD_SHA/REPO/PR are passed through env: rather than interpolated into the run: script text, so there's no script-injection surface from PR-controlled values.
• No secrets beyond the default GITHUB_TOKEN are used, and it isn't echoed or persisted anywhere reachable by a later step.

One low-confidence edge case worth being aware of but not blocking: if a force-push lands mid-pagination on a PR with enough commits to span multiple pages (>100), pages fetched before vs. after the push could theoretically describe two different histories, since each page is an independent live API call rather than a snapshot. In the common single-page case (<100 commits) this can't happen, and even in the multi-page case the HEAD_SHA anchor still catches the resulting inconsistency in most orderings. Given how narrow the window is and how much of this class of staleness the script already defends against, I wouldn't hold up merging on it — just flagging in case it's useful for a future hardening pass.

Nice work threading "prevention only, no after-the-fact fix" through the whole design, and being explicit in the header that this checks configuration (a misconfigured clone) rather than identity/authentication.

[erikdarlingdata]

Closing unmerged. See #2891 for the reasoning — the check catches a misconfigured clone rather than enforcing identity (author/committer are self-declared), and the implementation cost turned out disproportionate to a rule that can simply be followed. The work is preserved here if the trade ever looks different. Thanks for the three rounds of review; both substantive findings (the stale-view false-green and the page-count over-count) were real and were fixed before this decision.