fix: send PR head SHA instead of OIDC merge SHA on pull_request events#8
Conversation
For pull_request and pull_request_target workflow events, the GitHub
OIDC token's 'sha' claim refers to the ephemeral test-merge commit on
refs/pull/{n}/merge, not the PR's actual head commit. Anchoring a
GitHub Check Run to that merge commit makes it unaddressable from the
PR Checks tab — GitHub returns 'No check run found with ID <id> for
this pull request' when the user clicks the check.
Resolve the real PR head SHA from GITHUB_EVENT_PATH
(pull_request.head.sha) and forward it to testing-service as the new
commit_sha override field on both /api/v1/ci/run and
/api/v1/ci/builds/upload. The server only honours the override on PR
events; for any other event the OIDC sha claim is used as-is.
The new resolvePrHeadSha helper falls back to undefined (so callers
use the OIDC claim) on any failure, with core.warning logging — losing
the override is preferable to crashing the action.
📝 WalkthroughWalkthroughThe PR extends the API and action logic to support commit SHA override for pull request events. It adds optional Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions<br/>(pull_request event)
participant Main as Main Action Flow
participant CSha as Commit SHA<br/>Resolution
participant OIDC as OIDC Token<br/>Provider
participant API as API<br/>Server
GH->>Main: Trigger with eventName,<br/>GITHUB_EVENT_PATH
Main->>OIDC: Extract OIDC claims
OIDC-->>Main: oidcSha from claims
Main->>CSha: resolvePrHeadSha(eventName)
CSha->>CSha: Check if PR event
CSha->>CSha: Read GITHUB_EVENT_PATH<br/>Parse event JSON
CSha->>CSha: Validate head.sha<br/>(40-char hex)
CSha-->>Main: prHeadSha or undefined
Main->>Main: Resolve final commitSha:<br/>prHeadSha || oidcSha
Main->>API: uploadBuild(commitSha)
API-->>Main: Success
Main->>API: triggerRun(commitSha)
API-->>Main: Run triggered
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/commit-sha.ts (1)
29-32: Clarify event name in fallback warning text.Line 31 mentions only
pull_request, but this branch also runs forpull_request_target. Consider making the message event-accurate.Proposed tweak
- 'GITHUB_EVENT_PATH is unset on a pull_request event — falling back to OIDC sha (may not match PR head)', + `GITHUB_EVENT_PATH is unset on ${eventName} event — falling back to OIDC sha (may not match PR head)`,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commit-sha.ts` around lines 29 - 32, The warning logged when eventPath is unset (the core.warning call in src/commit-sha.ts that runs when eventPath is falsy) is inaccurate because it only mentions "pull_request" while the branch also applies to "pull_request_target"; update the message to be event-accurate by reading the current event name (e.g., from process.env.GITHUB_EVENT_NAME) and interpolating it into the warning, or explicitly mention both "pull_request" and "pull_request_target" so the fallback message accurately reflects the events handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/commit-sha.ts`:
- Around line 29-32: The warning logged when eventPath is unset (the
core.warning call in src/commit-sha.ts that runs when eventPath is falsy) is
inaccurate because it only mentions "pull_request" while the branch also applies
to "pull_request_target"; update the message to be event-accurate by reading the
current event name (e.g., from process.env.GITHUB_EVENT_NAME) and interpolating
it into the warning, or explicitly mention both "pull_request" and
"pull_request_target" so the fallback message accurately reflects the events
handled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1275020a-1b2b-4d2d-901b-6e69390e8e81
📒 Files selected for processing (4)
AGENTS.mdsrc/api.tssrc/commit-sha.tssrc/main.ts
Summary
For
pull_requestandpull_request_targetworkflow events, GitHub's OIDC tokenshaclaim refers to the ephemeral test-merge commit onrefs/pull/{n}/merge, not the PR's actual head commit. When the action sends that merge SHA, testing-service anchors the GitHub Check Run to it — and the PR "Checks" tab fails to resolve the check, showing:This PR resolves the real PR head SHA from
GITHUB_EVENT_PATH(pull_request.head.sha) and forwards it to testing-service as a newcommit_shaoverride field on both endpoints.Changes
src/commit-sha.ts—resolvePrHeadSha(eventName)readspull_request.head.shafrom the event payload for PR events. Falls back toundefined(so the caller uses the OIDC claim) on any failure, withcore.warninglogging — never throws.src/main.ts— wires the resolver in:commitSha = prHeadSha ?? oidcSha. Logs an info message when an override is applied.src/api.ts— addscommitSha?: stringto thetriggerRunJSON body (with doc-comment about PR-only override semantics) and acommit_shamultipart field touploadBuild.AGENTS.md— updates the API contract section to reflect the new override field and replaces the now-stale "never sent in the request body" note.Server contract
The matching testing-service change (separate PR) only honours the
commit_shaoverride when the OIDCevent_nameispull_requestorpull_request_target. For any other event, the override is ignored and the OIDCshaclaim is used as-is — so this action remains correct on push/tag/dispatch events.Verification
npm run all(build + lint + format:check + bundle) passes locallyMinitestcheck currently anchored to merge SHA98c60bfe…, not the PR head0a03326c…. After this PR + the testing-service fix ship, new runs on that PR will anchor to the head SHA and the check will resolve from the PR Checks tab.Notes
dist/is gitignored — the release workflow rebuilds it.commit_shafield is silently ignored (FastAPI accepts unknown form fields and Pydantic withextra='ignore'drops unknown JSON fields).Summary by CodeRabbit