Skip to content

Add defense-in-depth guards for self-hosted runner jobs - #3153

Merged
gyohuangxin merged 1 commit into
mainfrom
gyohuangxin/route-atom-tests-do-mi350x
Nov 10, 2026
Merged

Add defense-in-depth guards for self-hosted runner jobs#3153
gyohuangxin merged 1 commit into
mainfrom
gyohuangxin/route-atom-tests-do-mi350x

Conversation

@gyohuangxin

@gyohuangxin gyohuangxin commented Nov 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

  • add defense-in-depth forked-PR guards on the reusable SGLang GPU shard jobs that run on self-hosted runner labels
  • add defense-in-depth forked-PR guards on ATOMesh jobs that run on atomesh-cicd self-hosted runners
  • leave caller jobs, k8s runner jobs, and offline smoke unchanged

Current callers are schedule/workflow_dispatch-only for the guarded SGLang shard jobs, and the current ATOMesh pull_request path does not dispatch to these self-hosted jobs. These guards are intentionally scoped to the actual self-hosted runner jobs so a future PR caller cannot route forked PR code onto those runners by accident.

Validation

  • git diff --check
  • parsed all workflow YAML files with Python/PyYAML
  • verified the PR is based on latest origin/main and contains only the self-hosted runner defense-in-depth guards

Copilot AI balanced review requested due to automatic review settings November 10, 2026 06:14
Copilot AI reviewed Nov 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@github-actions

github-actions Bot commented Nov 10, 2026

Copy link
Copy Markdown
Contributor

🏷️ CI Guide

Runs automatically on every eligible PR before approval:

  • ✅ Pre Checkin: Black, Ruff, catalog schema validation, non-GPU unit tests

Heavy model tests:

  • ✅ Run after the PR is approved and Pre Checkin passes
  • ✅ Run immediately when an approval review is submitted
  • ✅ Can be requested before approval with labels
Label Tests
ci:full Run all heavy PR model tests: native ATOM, vLLM, and SGLang
ci:atom Run native ATOM model accuracy tests
ci:vllm Run ATOM vLLM OOT model accuracy tests
ci:sglang Run ATOM SGLang model accuracy tests

Heavy jobs are skipped when the PR is not approved and no matching ci:* label is present.
Add labels via the sidebar or gh pr edit 3153 --add-label <label>

valar-cop
valar-cop reviewed Nov 10, 2026
View reviewed changes

@valar-cop valar-cop left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

some changes needed..

a fork pr or run showing the exposure these guards block?

ci green.

Comment thread .github/workflows/atom-sglang-accuracy-validation-gpu-shard.yaml
jobs:
sglang-model-accuracy:
name: SGLANG Model Accuracy (${{ matrix.model_name }})
if: ${{ github.event_name != 'pull_request' || !github.event.pull_request.head.repo.fork }}

@valar-cop valar-cop Nov 10, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

this guard can never be false, all callers are schedule/workflow_dispatch only. same for the other five files. what exposure are we actually blocking?

@gyohuangxin gyohuangxin Nov 10, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point. You are right that these guards do not block a current fork-PR path: the SGLang callers are schedule/workflow_dispatch only, and ATOMesh PRs do not currently dispatch to the self-hosted jobs. I force-updated the PR to make this explicit defense-in-depth: removed the caller-level no-op guards, kept guards only on the jobs that actually run on self-hosted runner labels, and added comments documenting that this protects against a future PR caller accidentally routing forked PR code onto those runners.

@gyohuangxin
gyohuangxin force-pushed the gyohuangxin/route-atom-tests-do-mi350x branch from 145d4e9 to e0cd72e Compare November 10, 2026 07:18
Copilot AI balanced review requested due to automatic review settings November 10, 2026 07:18
Copilot AI reviewed Nov 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has insufficient remaining quota. Try requesting a review at a lower effort level.

@gyohuangxin gyohuangxin changed the title Protect self-hosted runner jobs from forked PRs Add defense-in-depth guards for self-hosted runner jobs Nov 10, 2026
@gyohuangxin
gyohuangxin merged commit fb39635 into main Nov 10, 2026
46 checks passed
@gyohuangxin
gyohuangxin deleted the gyohuangxin/route-atom-tests-do-mi350x branch November 10, 2026 07:33
rdoshi-amd added a commit to rdoshi-amd/ATOM that referenced this pull request Nov 10, 2026
@rdoshi-amd
Merge ROCm/ATOM main into the V4.1 hi monokernel branch
ae7f604 
Brings in main through 03b584b (ROCm#3109 V4.1 gfx1250 single-launch decode
step, ROCm#3169, ROCm#3167, ROCm#3052, ROCm#3073, ROCm#3153, README news). One conflict, in
model_runner.py: ROCm#3109's step override (`_step_override` / `_served`) and
this branch's model head output (`compute_head`) sit side by side, in
__init__ and as two branches of postprocess's verdict (a tuple head
output; an overridden step's `verify_outputs`). The two are exclusive: an
overridden step returns no head output. The hi runner test's stand-in
runner gains ROCm#3109's `_served` and the stream argument of
`forward_done_event.record`.
@wanzhenchn wanzhenchn mentioned this pull request Nov 10, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot
Copilot review effort
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@junyyang-amd junyyang-amd junyyang-amd approved these changes
+1 more reviewer
@valar-cop valar-cop valar-cop left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gyohuangxin @junyyang-amd @valar-cop