Skip to content

Commit 30f7d96

Browse files
authored
[CRCR] Fix the way of searching PR (#8386)
## Summary Fix `crcrOncallBot`'s "commit SHA → PR number" resolution for cross-fork pull requests on `pytorch/pytorch`, where the bot was silently failing to comment on downstream CI failures for the majority of contributions. ## Problem `crcrOncallBot` reacts to `check_run.completed` webhook events and posts oncall-ping comments on the associated PR when downstream CI fails. To find which PR a check run belongs to, it uses a two-step strategy: 1. **`checkRun.pull_requests`** — the `check_run` payload's `pull_requests` array 2. **Fallback: `listPullRequestsAssociatedWithCommit`** (GitHub Commits API) — when `pull_requests` is empty, which happens for **cross-fork PRs** (the code comment itself admits this) The problem: **both paths are dead ends for cross-fork PRs on large repos like `pytorch/pytorch`.** | API | Behavior for fork PR | |-----|---------------------| | `checkRun.pull_requests` | Empty `[]` — GitHub does not populate this for cross-fork check runs | | `GET /repos/{owner}/{repo}/commits/{sha}/pulls` | Returns `[]` — the commits-pulls endpoint does not return PRs from forked repositories on repos of `pytorch/pytorch`'s scale | The commits-pulls API returns HTTP 200 with `[]` (not an error), so the `try/catch` on the fallback doesn't fire — execution falls through to an empty-array check that silently returns without posting a comment. The bot was effectively **completely broken for cross-fork PRs**, which is the dominant contribution pattern for `pytorch/pytorch`. ### Evidence Tested with two real open fork PRs on `pytorch/pytorch`: ```bash # PR #191304 (head: RohitRathore1/pytorch) $ gh api repos/pytorch/pytorch/commits/29249453f.../pulls --jq 'length' 0 # ← commits API returns nothing $ gh api 'search/issues?q=29249453f...+type:pr+repo:pytorch/pytorch' \ --jq '[.items[].number]' [191304] # ← Search API returns the correct PR # PR #191301 (head: vishals-3/pytorch) $ gh api repos/pytorch/pytorch/commits/159d04cd9.../pulls --jq 'length' 0 $ gh api 'search/issues?q=159d04cd9...+type:pr+repo:pytorch/pytorch' \ --jq '[.items[].number]' [191301] ``` Fork-branch PR (not functional): pytorch/pytorch#189246 Intra-branch PR (functional): pytorch/pytorch#191313 ## Fix ### Core approach: embed PR number at the source Instead of a server-side API lookup, the PR number is now embedded in the check run's `external_id` field at creation time — alongside the downstream `run_id` that was already stored there. The bot reads it back directly with no external API call. **Producer side** (Python — `callback_handler.py`, `cleanup_handler.py`, `event_handler.py`): ```python # Before: external_id stored only the run_id external_id=str(run_id) # After: external_id encodes both run_id and pr_number external_id=f"{run_id}:{pr_number}" if pr_number else str(run_id) ``` **Consumer side** (`crcrOncallBot.ts`): ```ts // Before: fallback tried the Commits API (broken for forks) // After: parse external_id to extract the PR number } else if (checkRun.external_id) { const parts = checkRun.external_id.split(":"); if (parts.length === 2 && parts[1]) { prNumbers = [parseInt(parts[1], 10)]; } } ``` **Read sites** (`event_handler.py` — check run rerequest handlers) parse out the run_id portion: ```python # Before: run_id = check_run.get("external_id") or "" # After: split on ":" to extract just the run_id run_id = (check_run.get("external_id") or "").split(":")[0] ``` ### Why `external_id` instead of `output.summary` `output.summary` is user-facing display text rendered in the check run detail panel. Binding a machine contract to it means any future copy change silently breaks the bot, with no shared constant or cross-repo test between the Python (AWS Lambda) and TypeScript (Probot) deploy units. `external_id` is purpose-built for machine-readable data and was already used to store `run_id` for rerequest handling. ### Also fixed: `str(None)` bug `str(pr_field.get("number", ""))` produces the literal string `"None"` when the `number` key exists but is `null` — Python's `dict.get()` only returns the default when the key is missing, not when it's `None`. Changed to `str(pr_field.get("number") or "")` in all three call sites.
1 parent 6bb3c0f commit 30f7d96

7 files changed

Lines changed: 52 additions & 29 deletions

File tree

aws/lambda/cross_repo_ci_relay/callback/callback_handler.py

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -203,6 +203,7 @@ def _create_upstream_check_run(
203203
workflow_name: str,
204204
job_name: str | None,
205205
details_url: str,
206+
pr_number: str = "",
206207
) -> None:
207208
"""Create a new upstream check run mirroring the downstream job's status.
208209
@@ -233,7 +234,7 @@ def _create_upstream_check_run(
233234
details_url=details_url,
234235
# Store the downstream run_id so a check-run rerequest can re-run
235236
# the failed jobs of that workflow run.
236-
external_id=str(run_id),
237+
external_id=f"{run_id}:{pr_number}" if pr_number else str(run_id),
237238
output=output,
238239
)
239240
logger.info(
@@ -369,6 +370,7 @@ def handle(config: RelayConfig, body: dict, verified_repo: str) -> dict:
369370
if repo_level.value >= AllowlistLevel.L3.value:
370371
pr_field = (body.get("payload") or {}).get("pull_request") or {}
371372
head_sha = (pr_field.get("head") or {}).get("sha", "")
373+
pr_number = str(pr_field.get("number") or "")
372374
if head_sha:
373375
conclusion = (body.get("workflow") or {}).get("conclusion")
374376
details_url = f"https://github.com/{verified_repo}/actions/runs/{run_id}"
@@ -408,6 +410,7 @@ def handle(config: RelayConfig, body: dict, verified_repo: str) -> dict:
408410
workflow_name=workflow_name,
409411
job_name=job_name,
410412
details_url=details_url,
413+
pr_number=pr_number,
411414
)
412415

413416
if status == "in_progress":

aws/lambda/cross_repo_ci_relay/callback/cleanup_handler.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,7 @@ def _finalize_timed_out_check_run(
8383

8484
pr_field = (body.get("payload") or {}).get("pull_request") or {}
8585
head_sha = (pr_field.get("head") or {}).get("sha", "")
86+
pr_number = str(pr_field.get("number") or "")
8687
if not head_sha:
8788
return
8889

@@ -117,7 +118,7 @@ def _finalize_timed_out_check_run(
117118
status="completed",
118119
conclusion="timed_out",
119120
details_url=details_url,
120-
external_id=run_id,
121+
external_id=f"{run_id}:{pr_number}" if pr_number else run_id,
121122
output=output,
122123
)
123124
logger.info(

aws/lambda/cross_repo_ci_relay/tests/test_callback_handler.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -365,7 +365,7 @@ def test_check_run_external_id_is_run_id(self):
365365
)
366366

367367
kw = self.mock_gh.create_check_run.call_args[1]
368-
self.assertEqual(kw["external_id"], "99999") # run_id from _body
368+
self.assertEqual(kw["external_id"], "99999:42") # run_id:pr_number
369369

370370
def test_in_progress_callback_creates_check_run(self):
371371
self.mock_gh.create_check_run.return_value = 999

aws/lambda/cross_repo_ci_relay/tests/test_event_handler.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ def test_scenario2_in_progress_job_creates_in_progress_check_run(self):
191191
self.assertEqual(kw["head_sha"], "abc123")
192192
self.assertEqual(kw["status"], "in_progress")
193193
self.assertIsNone(kw["conclusion"])
194-
self.assertEqual(kw["external_id"], "99999") # run_id
194+
self.assertEqual(kw["external_id"], "99999:42") # run_id:pr_number
195195

196196
def test_scenario2_backfills_every_job_not_just_one(self):
197197
"""Multi-job workflow: a mid-run label backfills a check run for EVERY
@@ -215,7 +215,7 @@ def test_scenario2_backfills_every_job_not_just_one(self):
215215
c.kwargs["external_id"]
216216
for c in self.mock_gh.create_check_run.call_args_list
217217
}
218-
self.assertEqual(external_ids, {"99999"}) # run_id
218+
self.assertEqual(external_ids, {"99999:42"}) # run_id:pr_number
219219

220220
def test_scenario3_completed_job_creates_completed_check_run(self):
221221
"""Scenario 3: label arrives after workflow completed → create completed CR directly."""

aws/lambda/cross_repo_ci_relay/webhook/event_handler.py

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,7 @@ def _handle_pr_labeled(config: RelayConfig, payload: dict) -> dict:
152152
return {"ok": True, "created_check_runs": []}
153153

154154
pr = payload.get("pull_request") or {}
155-
pr_number = str(pr.get("number", ""))
155+
pr_number = str(pr.get("number") or "")
156156
head_sha = (pr.get("head") or {}).get("sha", "")
157157
if not pr_number or not head_sha:
158158
return {"ignored": True, "reason": "missing pr context"}
@@ -205,9 +205,12 @@ def _handle_pr_labeled(config: RelayConfig, payload: dict) -> dict:
205205
status=job_status,
206206
conclusion=(job_conclusion if job_status == "completed" else None),
207207
details_url=details_url,
208-
external_id=str(run_id),
208+
external_id=f"{run_id}:{pr_number}" if pr_number else str(run_id),
209209
output=gh_helper.build_check_run_output(
210-
job_status, job_conclusion, details_url, downstream_repo
210+
job_status,
211+
job_conclusion,
212+
details_url,
213+
downstream_repo,
211214
),
212215
)
213216
created.append(f"{downstream_repo}/{job_name}")
@@ -263,7 +266,7 @@ def _handle_check_run_rerequested(config: RelayConfig, payload: dict) -> dict:
263266
"""
264267
check_run = payload.get("check_run") or {}
265268
name = check_run.get("name", "")
266-
run_id = check_run.get("external_id") or ""
269+
run_id = (check_run.get("external_id") or "").split(":")[0]
267270
downstream_repo = _downstream_repo_from_check_run(name)
268271
if not downstream_repo or not run_id:
269272
return {"ignored": True, "reason": "not a crcr check run"}
@@ -346,7 +349,7 @@ def _handle_check_suite_rerequested(config: RelayConfig, payload: dict) -> dict:
346349
rerun: list[str] = []
347350
for check_run in check_runs:
348351
downstream_repo = _downstream_repo_from_check_run(check_run.get("name", ""))
349-
run_id = check_run.get("external_id") or ""
352+
run_id = (check_run.get("external_id") or "").split(":")[0]
350353
if not downstream_repo or not run_id:
351354
continue
352355
if (downstream_repo, run_id) in seen:

torchci/lib/bot/crcrOncallBot.ts

Lines changed: 4 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -80,27 +80,13 @@ export default function crcrOncallBot(app: Probot): void {
8080
}
8181

8282
// Get the PRs this check run belongs to.
83-
// checkRun.pull_requests is empty for cross-fork PRs (most pytorch
84-
// contributions), so fall back to the commits API to resolve PRs
85-
// from the head SHA — the same strategy used by the merge-blocking path.
8683
let prNumbers: number[] = [];
8784
if (checkRun.pull_requests && checkRun.pull_requests.length > 0) {
8885
prNumbers = checkRun.pull_requests.map((pr) => pr.number);
89-
} else if (checkRun.head_sha) {
90-
try {
91-
const result =
92-
await ctx.octokit.rest.repos.listPullRequestsAssociatedWithCommit({
93-
owner,
94-
repo,
95-
commit_sha: checkRun.head_sha,
96-
});
97-
prNumbers = result.data.map((pr: any) => pr.number);
98-
} catch (err) {
99-
ctx.log(
100-
{ err },
101-
`crcrOncall: failed to resolve PRs for commit ${checkRun.head_sha}, skipping`
102-
);
103-
return;
86+
} else if (checkRun.external_id) {
87+
const parts = checkRun.external_id.split(":");
88+
if (parts.length === 2 && parts[1]) {
89+
prNumbers = [parseInt(parts[1], 10)];
10490
}
10591
}
10692

torchci/test/crcrOncallBot.test.ts

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,16 +176,46 @@ L3:
176176
});
177177
});
178178

179-
test("does not comment when no PR is associated", async () => {
179+
test("does not comment when external_id has no PR number", async () => {
180+
// external_id carries bare run_id (no ":" separator) when pr_number
181+
// is unavailable — the bot returns early without any API call.
180182
await probot.receive({
181183
name: "check_run" as any,
182184
payload: checkRunPayload({
183185
pull_requests: [],
186+
external_id: "12345",
184187
}) as any,
185188
id: "8",
186189
});
187190
});
188191

192+
test("posts comment when external_id contains PR number for cross-fork PR", async () => {
193+
const scope = nock("https://api.github.com")
194+
.get(`/repos/${OWNER}/${REPO}/issues/${PR_NUMBER}/comments`)
195+
.reply(200, [])
196+
.post(
197+
`/repos/${OWNER}/${REPO}/issues/${PR_NUMBER}/comments`,
198+
(body: any) => {
199+
expect(body.body).toContain(
200+
"<!-- crcr-oncall:intel/torch-xpu-ops -->"
201+
);
202+
expect(body.body).toContain("@oncall_xpu");
203+
return true;
204+
}
205+
)
206+
.reply(200);
207+
208+
await probot.receive({
209+
name: "check_run" as any,
210+
payload: checkRunPayload({
211+
pull_requests: [], // empty — simulates cross-fork PR
212+
external_id: `12345:${PR_NUMBER}`,
213+
}) as any,
214+
id: "10",
215+
});
216+
handleScope(scope);
217+
});
218+
189219
test("does nothing for unsupported org", async () => {
190220
const payload = checkRunPayload();
191221
payload.repository = {

0 commit comments

Comments
 (0)