Skip to content

fix(amdsmi): derive ABI label target from run payload, not artifacts - #11001

Open
marifamd wants to merge 1 commit into
developfrom
fix/abi-report-pr-resolution
Open

fix(amdsmi): derive ABI label target from run payload, not artifacts#11001
marifamd wants to merge 1 commit into
developfrom
fix/abi-report-pr-resolution

Conversation

@marifamd

Copy link
Copy Markdown
Contributor

Motivation

Follow-up to #9955. The ABI report workflow holds pull-requests: write and took both the PR number and the check verdict from artifacts uploaded by the check workflow. On pull_request the fork supplies the workflow file, so those artifacts are fork-controlled and could point label edits at any PR in the repo.

Separately, a run that produced no verdict (cancelled, or an infrastructure failure before the outcome was written) removed existing ABI breakage labels instead of leaving them alone.

Technical Details

Report workflow (.github/workflows/abi-compliance-report.yml):

  • Resolve the PR from the workflow_run payload (head_repository.owner.login + head_branch) and require its head.sha to still match the commit that was checked
  • Drop the pr_number.txt artifact fallback. workflow_run.pull_requests is empty for fork PRs and commits/{sha}/pulls returns [] for them, so the payload branch/owner pair is the only trusted way to identify a fork PR
  • Leave breakage labels untouched when no verdict was reported; only success removes a label and only failure adds one
  • Gate the job on workflow_run.conclusion, and add a per-branch concurrency group so overlapping runs cannot interleave label edits

Check workflow (.github/workflows/abi-compliance-check.yml):

  • Pin abi-compliance-checker to commit 8e819827 (the commit behind tag 2.3) instead of the movable tag, matching the SHA pinning already applied to the actions
  • Narrow the untrusted checkout to projects/amdsmi/include/amd_smi, so the fork's copy of abi_check.py is no longer written to disk. Only the header is needed from the head ref; the script already runs from _base
  • Stop producing pr_number.txt, now unused
  • Add job timeouts and a concurrency group, and drop stale "Strict" wording from the minor-check log lines

Residual, and not fixable here: on pull_request a fork controls its own check workflow, so it can always report its own ABI result as clean. These changes contain the blast radius to the fork's own PR rather than any PR in the repo.

Issue Tracking

JIRA ID: ROCM-26709

Test Plan

Workflow-only change, so the scripts were extracted from the YAML and run directly:

  • Parsed both files with yaml.safe_load
  • Ran the Resolve PR number script against the live API for a real fork PR (hanhanW:users/hanhanW/gfx1251-packed-fp64-fma) and for a deliberately stale head SHA
  • Exercised the label logic against a stubbed gh for four cases: major failure, all clean, artifacts missing, and outcome skipped
  • Reproduced cone-mode sparse-checkout locally to confirm the narrowed pattern still materializes amdsmi.h and no longer materializes tests/abi_check
  • Fetched the pinned abi-compliance-checker commit by SHA and confirmed the Makefile and TOOL_VERSION = "2.3" are intact

Test Result

  • YAML: both files parse
  • PR resolution: the correct fork PR returns number=10997; the stale SHA returns empty and skips labeling
  • Labels: failure adds, success removes, missing and skipped leave labels unchanged (previously both were removed)
  • Sparse checkout: amdsmi.h present, fork copy of abi_check.py absent
  • Pinned checker: fetch-by-SHA resolves to 8e819827e8d707c7addc4a08f5cf74045f2302bb, version string 2.3
  • pre-commit: passed

Submission Checklist

The report workflow holds pull-requests: write and took both the PR
number and the check verdict from artifacts produced by the check
workflow. On pull_request the fork supplies the workflow file, so those
artifacts are fork-controlled and could point label edits at any PR.
Resolve the PR from the workflow_run payload instead and require its
head to still match the commit that was checked.

Also leave breakage labels untouched when no verdict was reported (a
cancelled run previously cleared them), pin the ABI checker to a commit
rather than a movable tag, and stop checking out the fork's copy of the
check script.

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
@marifamd
marifamd requested a review from a team as a code owner August 31, 2026 22:02
@github-actions github-actions Bot added the github actions Pull requests that update GitHub Actions code label Aug 31, 2026
@therock-pr-bot

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass
Forbidden Files ✅ Pass
🧪 Unit Test ✅ Pass PR does not contain code files — Unit Test auto-passed
🚫 Draft PR 🔜 To Be Enabled
🚩 Feature Flag 🔜 To Be Enabled
📊 Code Coverage 🔜 To Be Enabled

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

@JeniferC99 JeniferC99 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, will not affect therock gating ci.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

github actions Pull requests that update GitHub Actions code organization: ROCm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants