[CRCR] Fix the way of searching PR

[KarhouTam]

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:

# 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):

# 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):

// 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:

# 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.

[vercel]

@KarhouTam is attempting to deploy a commit to the Meta Open Source Team on Vercel.

A member of the Team first needs to authorize it.

[subinz1]

Reviewed the change — this is a clean, well-evidenced fix.

The listPullRequestsAssociatedWithCommit endpoint returning empty [] for cross-fork PRs is a known GitHub API limitation, and the Search API is the correct fallback here. The PR description's evidence with real SHA lookups makes the case clear.

A couple of minor notes (not blocking):

• Search API rate limit: 30 req/min vs. 5000 req/hr for REST. Under burst scenarios (multiple downstream repos failing simultaneously on the same PR), the 403 would be caught by the existing try/catch and logged, so no functional impact — just worth being aware of.
• Indexing delay: The Search API can lag a few seconds behind real-time indexing, but since CRCR check runs take minutes to hours, this shouldn't be a practical concern.
  Tests are well-structured — the new cross-fork fallback test exercises the full happy path, and the existing "no PR" test is now properly mocking the Search API instead of relying on an unintentional nock connection error.

LGTM!

[can-gaa-hou]

Search API rate limit: 30 req/min vs. 5000 req/hr for REST.

I think this is worth considering. 30 req/min could not be satisfied if there are more and more L3/L4 repos. Actually, we don't need this extra search. We can add the PR number when we create the check run output on the Lambda side, so we can directly extract the corresponding PR number from the check run; no need to search. WDYT? @KarhouTam

[KarhouTam]

Thx for your comments @subinz1 @can-gaa-hou . Updated and please have a look again! Keep the search API as backup.

[KarhouTam]

Thanks @subinz1! Fixed.

[subinz1]

Re-reviewed after the latest commit — all three issues from the previous round are addressed:

1. else if chain fixed: Search API is now a standalone if (prNumbers.length === 0 && checkRun.head_sha) block, so all three tiers are reachable: output regex → pull_requests → Search API.

2. Trailing comma fixed: pr_number: str = "" — also added a default value so existing callers don't break.

3. Conditional PR format fixed: pr_part = f" for PR {pr_number}" if pr_number else "" — no more awkward "for PR :" when the number is empty.

Tests are properly updated — the "no PR" case now mocks the Search API returning empty instead of relying on an unintentional error path, and the cross-fork test exercises the full output-regex happy path.

LGTM!

[can-gaa-hou]

Thanks @KarhouTam. Overall look good to me except some small nits. Please also update the description.

[KarhouTam]

Hi @atalman. Sorry to pin you. Could you please review this PR when you have chance? Thanks!

[atalman]

Thanks for digging into this — the diagnosis is well evidenced, and embedding the PR number at the source is the right call. It removes the lookup entirely and sidesteps the Search API rate ceiling @can-gaa-hou raised. Two things I'd like addressed before this merges.

1. The description doesn't match the implementation

The ## Fix section still shows the search.issuesAndPullRequests swap, but that code isn't in the branch — there is no Search API call in crcrOncallBot.ts on this head. The implementation now embeds the PR number in output.summary and regex-parses it in the bot. The description looks like it was never updated after that pivot.

Worth fixing before merge, since the PR body becomes the squash commit message and the permanent record would describe an approach that never landed. It also means earlier review comments referring to the Search API as a backup tier, or to three lookup tiers, don't describe the current code. There are two:

if (checkRun.pull_requests && checkRun.pull_requests.length > 0) {
  prNumbers = checkRun.pull_requests.map((pr) => pr.number);
} else if (checkRun.output?.summary) {
  const match = checkRun.output.summary.match(/for PR (\d+)/);
  ...
}

2. output.summary is a display string — external_id is the right channel

The contract as written is:

producer, utils/gh_helper.py:

pr_part = f" for PR {pr_number}" if pr_number else ""
"summary": f"{downstream_repo} workflow{pr_part}: {details_url}",

consumer, torchci/lib/bot/crcrOncallBot.ts:

const match = checkRun.output.summary.match(/for PR (\d+)/);

summary is user-facing text rendered in the check run detail panel. Binding a machine contract to it means any future copy change silently breaks the bot — and the two sides are in different languages and different deploy units (AWS Lambda vs. Probot), with no shared constant and no test spanning both, so nothing would catch it.

GitHub has a field for exactly this, and we already use it: external_id. It's set on all three creation paths (callback_handler.py:237, cleanup_handler.py:121, event_handler.py:208) and already read back at event_handler.py:270 and :353. Storing something like f"{run_id}:{pr_number}", or a small JSON blob, and reading checkRun.external_id in the bot would be immune to copy changes and keeps machine data out of user-visible text.

The cost is real but contained: external_id currently carries the bare run_id, so you'd need to update those two read sites and their coverage in tests/test_event_handler.py.

If you'd rather keep the summary approach, then please at least define the format in one place as an explicit contract and add a producer-side test pinning the exact string. There's no tests/test_gh_helper.py today, so build_check_run_output has no direct coverage, and the new TS test hardcodes its own copy of the string — as it stands, producer and consumer can drift with both suites green.

I have a couple of smaller notes as well (the new negative test uses a summary the producer can't actually emit, and str(pr_field.get("number", "")) renders "None" when number is present but null). Happy to add those as inline comments if useful.

[can-gaa-hou]

Storing something like f"{run_id}:{pr_number}"

Thanks for the suggestion @atalman, that makes sense to me. We can use this approach to store both run_id and pr_number, and split when we read them. Also, the PR description needs to be updated. cc @KarhouTam

[KarhouTam]

Hi @atalman. Thanks for your valuable review and comment! Already fixed them. Please re-review if you have chance. Thanks!

[KarhouTam]

Hi @atalman @can-gaa-hou . Thanks for your reviews and comments. I've already fixed it. Please re-review it if you have chance! Thanks!

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
torchci	Ready	Preview	Aug 6, 2026 2:43pm