Skip to content

fix(ci): resolve fork PRs for recipe-evidence sticky comment#1297

Merged
njhensley merged 2 commits into
NVIDIA:mainfrom
njhensley:ci/evidence-comment-fork-pr-resolution
Jun 10, 2026
Merged

fix(ci): resolve fork PRs for recipe-evidence sticky comment#1297
njhensley merged 2 commits into
NVIDIA:mainfrom
njhensley:ci/evidence-comment-fork-pr-resolution

Conversation

@njhensley

Copy link
Copy Markdown
Member

Summary

Resolve the PR number for the recipe-evidence sticky comment by listing open PRs and matching on head SHA, so the comment posts on fork PRs instead of being silently skipped.

Motivation / Context

The comment workflow resolved the PR via GET /repos/<base>/commits/{HEAD_SHA}/pulls. That endpoint does not reliably return fork-originated PRs — the head commit lives in the contributor's fork, not the base repo — so every external-contributor PR resolved to 0 matches and the comment was skipped with:

Warning: expected exactly 1 open PR for head c3daa9f… from yuanchen8911; got 0; skipping comment

Confirmed empirically: commits/c3daa9f…/pulls on NVIDIA/aicr returns [], yet PR #1295 (open, from yuanchen8911) has exactly that commit as its head. Listing open PRs and matching .head.sha finds it.

Fixes: N/A
Related: #1292 (companion fix: the upload step that produces the artifact this workflow consumes)

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • Build/CI/tooling

Component(s) Affected

  • Other: GitHub Actions workflow (.github/workflows/recipe-evidence-comment.yaml)

Implementation Notes

  • Replaced the commits/{sha}/pulls lookup with gh api --paginate "…/pulls?state=open" --jq streaming PR numbers, slurped into a JSON array with jq -s. This matches both same-repo and fork PRs uniformly.
  • HEAD_SHA/HEAD_OWNER are read via jq's env. rather than interpolated into the filter, so a hostile value can't break out of the jq expression. (gh's --jq doesn't accept jq's --arg.)
  • Used the repo's existing gh api --paginate --jq + standalone jq -s pattern rather than gh api --slurp, which isn't available in the pinned gh.
  • Still derives the PR strictly from the trusted workflow_run event (SHA + owner), never an artifact. The exactly-one-match guard and owner cross-check are unchanged.

Testing

yamllint -c .yamllint.yaml .github/workflows/recipe-evidence-comment.yaml   # exit 0

# Live replay of the failing case against NVIDIA/aicr:
#   HEAD_SHA=c3daa9f… HEAD_OWNER=yuanchen8911 → match_count=1, pr_number=1295
#   bogus owner       → [] (count 0, skips cleanly)

End-to-end validation requires a live fork PR run on GitHub. No Go code changed.

Risk Assessment

  • Low — Isolated CI-only change, easy to revert. The affected gate is warning-only and never blocks merge.

Rollout notes: N/A — takes effect on the next recipes/** PR.

Checklist

  • Tests pass locally (make test with -race) — N/A, no Go changes
  • Linter passes (yamllint, exit 0)
  • I did not skip/disable tests to make CI green
  • I added/updated tests for new functionality — N/A (CI workflow fix)
  • I updated docs if user-facing behavior changed — N/A (no user-facing behavior change)
  • Changes follow existing patterns in the codebase
  • Commits are cryptographically signed (git commit -S)

@njhensley njhensley requested a review from a team as a code owner June 10, 2026 21:04
@njhensley njhensley added area/ci theme/ci-dx CI pipelines, developer experience, and build tooling labels Jun 10, 2026
@coderabbitai

coderabbitai Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: 259dea89-88bd-4768-9e99-75fdd49a6bfe

📥 Commits

Reviewing files that changed from the base of the PR and between 4e8f778 and 84d210e.

📒 Files selected for processing (1)
  • .github/workflows/recipe-evidence-comment.yaml

📝 Walkthrough

Walkthrough

This PR updates the GitHub Actions workflow to fix PR detection for fork-originated pull requests. The "Resolve PR number from trusted workflow_run metadata" step now retrieves open PRs via paginated listing and matches them by comparing the PR head commit SHA and repository owner against workflow metadata. This replaces the previous approach that queried the commits API endpoint, which did not return PRs from forked repositories. The implementation includes pagination handling and jq injection safeguards by sourcing variable values through environment variables.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested labels

size/S

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately reflects the main change: fixing fork PR resolution in the recipe-evidence comment workflow.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, explaining the bug, motivation, implementation, and testing of the fork PR resolution fix.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Stopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a @coderabbit review after the pipeline has finished.


Comment @coderabbitai help to get the list of available commands and usage tips.

@njhensley njhensley enabled auto-merge (squash) June 10, 2026 21:25
@njhensley njhensley merged commit 1ef5bdb into NVIDIA:main Jun 10, 2026
32 of 33 checks passed
@njhensley njhensley deleted the ci/evidence-comment-fork-pr-resolution branch June 10, 2026 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci size/S theme/ci-dx CI pipelines, developer experience, and build tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants