ci: require nvskills status for skill changes - #372
Conversation
mosheabr
left a comment
There was a problem hiding this comment.
Thanks @yashrajp22 — this closes a real gap and the overall shape is right. A few things before it can land:
Blockers
- DCO is failing — please
git rebase --signoff origin/main && git push --force-with-lease. - The guard needs to exempt the
automated/sync-skillsbot PRs (same pattern as the DCO/authors checks). Sync PRs touchskills/**with content signed in the source repos, so their head SHAs never carry anNVSkills CIstatus — as written this fails every daily sync PR.
Design
- The
status:trigger won't clear a red check on the PR:status-event runs execute against the default branch, so the new check run doesn't attach to the PR head SHA. Authors would need a manual re-run after/nvskills-cisucceeds. Consider having the NVSkills CI pipeline refresh the check itself, or aworkflow_run-based re-check. - Please drop the
paths:filter on thepull_requesttrigger. Once this check is required in branch protection, PRs that don't touch watched paths (e.g.components.d/onboarding PRs) would never report the check and sit at "Expected" forever. The job already self-skips via the files API, so it's safe to run on every PR.
Minor
- The
pushbranch of the guard'sifis unreachable (the caller only forwards signature pushes, which the guard excludes) — can be removed. - The failure message should note that
/nvskills-cionly works on branches in NVIDIA/skills, not forks, so fork authors know to move their branch rather than re-commenting.
Happy to re-review once updated.
f69c055 to
cb798b4
Compare
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
cb798b4 to
b04f8b6
Compare
mosheabr
left a comment
There was a problem hiding this comment.
Re-reviewed, and I tested the reworked guard locally against real PRs: #379 and #361 (the trailing-benchmarks-commit cases) now pass — the guard correctly walks back to the signature commit's status and treats the head's benchmarks/metadata commit as trailing. A genuinely-failing PR (#370) still blocks, and no-watched-change / bot-branch PRs skip. Design #3 is resolved. Two minor, non-blocking notes remain (the no-trailing-commit timing edge without a status:/workflow_run: refresh, and the unreachable push branch), but those don't gate this. Approving — nice work. 🚀
…outbound callers) #372 added `statuses: read` to team-request.yml's workflow-level permissions. A reusable workflow can't request more permission than its caller grants, and outbound callers (NeMo-RL, cuPyNumeric, etc.) grant only contents+pull-requests — so every /nvskills-ci comment and signature-push on those repos has failed at startup since #372 merged, emailing the workflow authors. Move statuses:read to the require-nvskills-ci job (its only consumer; runs on NVIDIA/skills' own pull_request events, where our caller grants statuses). Workflow- level permissions drop back to contents+pull-requests, matching outbound callers, so they stop startup-failing. Guard behavior is unchanged (run script untouched). Signed-off-by: Moshe Abramovitch <moshea@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…t.yml #372 placed the require-nvskills-ci guard (which needs statuses:read) inside the SHARED reusable workflow team-request.yml. A reusable workflow can't request more permission than its caller grants, and outbound callers (NeMo-RL, cuPyNumeric, ...) grant only contents+pull-requests — so GitHub validates the guard's statuses:read against them at startup (even though the job is skipped for them) and fails every /nvskills-ci and signature-push on those repos. #384's job-level move did not fix this — confirmed by test: GitHub validates skipped-job permissions at startup too. Extract the guard into a native NVIDIA/skills-only workflow (require-nvskills-status.yml, on: pull_request, statuses:read). team-request.yml drops the guard and statuses entirely, needing only contents+pull-requests — matching every outbound caller. Guard script is byte-unchanged. request-nvskills-ci.yml drops the now-unneeded pull_request trigger and statuses:read. Verified with a throwaway repo: a statuses-free reusable workflow called by a contents+pull-requests-only caller no longer startup-fails (the same setup WITH statuses does). Signed-off-by: Moshe Abramovitch <moshea@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…outbound callers) NVIDIA#372 added `statuses: read` to team-request.yml's workflow-level permissions. A reusable workflow can't request more permission than its caller grants, and outbound callers (NeMo-RL, cuPyNumeric, etc.) grant only contents+pull-requests — so every /nvskills-ci comment and signature-push on those repos has failed at startup since NVIDIA#372 merged, emailing the workflow authors. Move statuses:read to the require-nvskills-ci job (its only consumer; runs on NVIDIA/skills' own pull_request events, where our caller grants statuses). Workflow- level permissions drop back to contents+pull-requests, matching outbound callers, so they stop startup-failing. Guard behavior is unchanged (run script untouched). Signed-off-by: Moshe Abramovitch <moshea@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carmelo Gonzales <43048528+melo-gonzo@users.noreply.github.com>
…t.yml NVIDIA#372 placed the require-nvskills-ci guard (which needs statuses:read) inside the SHARED reusable workflow team-request.yml. A reusable workflow can't request more permission than its caller grants, and outbound callers (NeMo-RL, cuPyNumeric, ...) grant only contents+pull-requests — so GitHub validates the guard's statuses:read against them at startup (even though the job is skipped for them) and fails every /nvskills-ci and signature-push on those repos. NVIDIA#384's job-level move did not fix this — confirmed by test: GitHub validates skipped-job permissions at startup too. Extract the guard into a native NVIDIA/skills-only workflow (require-nvskills-status.yml, on: pull_request, statuses:read). team-request.yml drops the guard and statuses entirely, needing only contents+pull-requests — matching every outbound caller. Guard script is byte-unchanged. request-nvskills-ci.yml drops the now-unneeded pull_request trigger and statuses:read. Verified with a throwaway repo: a statuses-free reusable workflow called by a contents+pull-requests-only caller no longer startup-fails (the same setup WITH statuses does). Signed-off-by: Moshe Abramovitch <moshea@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Carmelo Gonzales <43048528+melo-gonzo@users.noreply.github.com>
Summary
NVSkills CIstatus.skills/,team-skills/,rules/team-rules/, andplugins/./nvskills-cidispatch flow unchanged.Validation
.github/workflows/team-request.ymlas YAMLbash -non the inline guard scriptgit diff --check