fix: restore NVSkills workflow startup

[andreatnvidia]

📋 Summary

Restores the Request NVSkills CI workflow after the shared NVIDIA/skills reusable workflow added pull-request status enforcement. DataDesigner's caller was missing the new trigger and permission contract, causing workflow startup failures before any job ran.

The failures were visible on PR #838 in runs 31109567364 and 31113460876. Both ended with startup_failure and created zero jobs.

🔗 Related Issue

N/A

🔄 Changes

• Trigger the NVSkills reusable workflow for pull-request lifecycle events.
• Route pull-request events through the existing reusable workflow job.
• Grant statuses: read, matching the reusable workflow's required permission.
• Preserve the existing issue-comment and trusted signature-push request paths.

🧪 Testing

• make test passes - not run; workflow-only change
• .venv/bin/pytest packages/data-designer/tests/docs/test_docs_workflows.py - 6 passed
• .venv/bin/pre-commit run check-yaml --files .github/workflows/request-nvskills-ci.yml
• .venv/bin/ruff check --fix .
• .venv/bin/ruff format .
• Unit tests added/updated - N/A, workflow wiring only
• E2E tests added/updated - N/A, workflow wiring only

✅ Checklist

• Follows commit message conventions
• Commits are signed off (DCO)
• Architecture docs updated - N/A, no architecture change

[greptile-apps]

Greptile Summary

Adds pull-request lifecycle events to the NVSkills request workflow and grants read access to commit statuses, restoring compatibility with the shared reusable workflow while preserving comment and trusted-push entry points.

• Routes opened, reopened, synchronized, and ready-for-review pull requests through the reusable workflow.
• Adds the statuses: read permission required by the shared workflow.
• Retains the existing issue-comment and signature-push conditions.

Confidence Score: 5/5

The PR appears safe to merge, with no concrete blocking or independently actionable issue identified.

The workflow now supplies the pull-request trigger and read-only status permission described by the shared workflow contract while leaving the existing request paths intact.

Important Files Changed

Filename	Overview
.github/workflows/request-nvskills-ci.yml	Extends the NVSkills reusable-workflow caller with pull-request event routing and the documented read-only status permission; no actionable defect is established.

_{Reviews (1): Last reviewed commit: "fix: restore NVSkills workflow startup" | Re-trigger Greptile}

[github-actions]

Thanks for tracking this down and getting the NVSkills workflow back on its feet, @andreatnvidia — the writeup with the two startup_failure runs made the root cause easy to follow.

Summary

This restores the Request NVSkills CI caller so it satisfies the reusable NVIDIA/skills/.github/workflows/team-request.yml contract after that workflow added PR status enforcement. It adds a pull_request trigger, an unconditional pull_request branch to the job's if, and the statuses: read permission. The implementation matches the stated intent, and the existing issue_comment and signature-push paths are preserved intact.

Findings

Warnings — Worth addressing

.github/workflows/request-nvskills-ci.yml:6 — pull_request from forks won't have the dispatch secret

• What: The request job passes secrets.NVSKILLS_CI_DISPATCH_TOKEN to the reusable workflow. On the pull_request event (as opposed to pull_request_target), PRs opened from forks run with a read-only GITHUB_TOKEN and no access to repository secrets — so NVSKILLS_CI_DISPATCH_TOKEN resolves to an empty string for those PRs. The prior triggers (issue_comment, push) always ran in the base-repo context where the secret is available, so this is a new exposure introduced by the pull_request trigger.
• Why: If this repo ever receives external fork PRs, each one would spin up a "Request NVSkills CI" run that fails inside the reusable workflow due to the missing token — turning a green-by-default checks list into a persistent red mark for outside contributors. (Internal same-repo branches like this PR's own head are unaffected, which is why it works here.)
• Suggestion: Confirm whether NVSkills CI is expected to run for fork PRs. If it's intentionally internal-only, consider gating the new branch on same-repo head, e.g. github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository. If forks are in scope, the token-dependent dispatch will need a different trigger model. Even a short comment noting the intended scope would help future maintainers.

Suggestions — Take it or leave it

.github/workflows/request-nvskills-ci.yml:6 — No concurrency guard for synchronize

• What: synchronize fires on every push to an open PR branch, so rapid successive pushes will each kick off a fresh request run.
• Why: Without a concurrency group, superseded runs keep executing instead of being cancelled, which adds noise and consumes dispatch churn.
• Suggestion: Optionally add a concurrency block keyed on the PR, e.g.:

  concurrency:
    group: nvskills-ci-${{ github.event.pull_request.number || github.ref }}
    cancel-in-progress: true

  Only worth it if PR branches see frequent re-pushes.

.github/workflows/request-nvskills-ci.yml:22 — Verify statuses: read is sufficient

• What: The PR grants statuses: read to match the reusable workflow's required permission. If the reusable workflow (or the enforcement it performs) ever needs to write a commit status/check, read would be insufficient.
• Why: A permission mismatch here would resurface as another silent startup or runtime failure, similar to the one being fixed.
• Suggestion: This is likely correct given you matched it against team-request.yml@main. Worth a quick double-check of that reusable workflow's declared permissions to be sure read (not write) is what it expects.

What Looks Good

• Minimal, surgical fix. The change is scoped tightly to the workflow contract — trigger, if branch, and permission — without touching the working issue_comment/push paths. That keeps the blast radius tiny.
• Good trigger type selection. Including ready_for_review alongside opened/reopened/synchronize covers the draft→ready transition, so PRs that start as drafts still get enforced once they're marked ready.
• Safe event choice. Using pull_request rather than pull_request_target avoids running untrusted PR code with elevated permissions — the right default from a security standpoint.

Verdict

Needs discussion — The change is clean and clearly fixes the startup failure for internal branches. The one thing worth settling before merge is the intended scope for fork PRs (the pull_request + secret interaction): if forks are out of scope, a same-repo guard would prevent future red-check noise; if they're in scope, the token-based dispatch needs a rethink. The other two items are optional.

---

This review was generated by an AI assistant.