NVIDIA-NeMo / NVIDIA-NeMo/DataDesigner
Harden Agentic CI PR reviews for forked pull requests
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 2.2k
- Forks
- 211
- Avg merge
- 2d 6h
- Merged PRs (30d)
- 40
Description
Priority Level
High
Task Summary
Harden the Agentic CI PR review workflow so maintainer-approved fork PRs can be reviewed without exposing secrets, repository credentials, or the self-hosted runner to fork-controlled code and project configuration.
PR #795 upgrades actions/checkout to v7, which blocks fork PR checkout from pull_request_target workflows unless allow-unsafe-pr-checkout: true is set. As a short-term compatibility decision, #795 will explicitly opt in so behavior remains equivalent to the existing checkout v6 workflow. This issue tracks replacing that accepted risk with a hardened design.
Technical Details & Implementation Plan
Suggested approach:
- Resolve and validate immutable PR context before any checkout:
- PR number
- base SHA
- head SHA
- head repository
- whether the PR comes from a fork
- Keep the existing full Claude Code review path for same-repository PRs.
- For fork PRs, use a hardened diff-only path:
- Do not check out the fork.
- Do not run repository scripts, tests, hooks, project settings, or Claude Code tools.
- Fetch PR metadata, changed file names, and the diff through GitHub using the validated PR number and head SHA.
- Treat all PR metadata and diff content as untrusted data.
- Send only that inert review payload to the model through a tool-free API request.
- Set
persist-credentials: falseon checkouts and avoid making a write-capable repository token available to the model process. - Preserve explicit maintainer authorization for fork reviews and bind approval to the reviewed head SHA. A changed head must require fresh authorization.
- Minimize workflow permissions and isolate secret-bearing steps. Prefer an ephemeral runner for any job processing untrusted input.
- Audit or disable project-controlled Claude settings, hooks, and instruction files anywhere untrusted content can be present.
- Add workflow validation for:
- same-repository PRs
- maintainer-approved fork PRs
- fork head changes after approval
- manual
workflow_dispatchof a fork PR - missing model configuration and API failures
Acceptance criteria:
- Fork PR review does not check out or execute fork-controlled content.
- Fork-controlled project configuration cannot run on the self-hosted runner.
- Repository credentials are not persisted in the checkout.
- Authorization is tied to an immutable head SHA.
- Same-repository review behavior remains unchanged.
- The workflow still posts a useful advisory review or an explicit incomplete-review result.
Investigation / Context
The current workflow was intentionally moved to pull_request_target in #541 so approved fork PR reviews could access the model configuration and secrets. Existing safeguards include collaborator/label gating, an agentic-ci environment gate, and loading review recipes and tools from the base branch.
Those controls reduce exposure but do not fully isolate untrusted content:
- The fork working tree is placed on a self-hosted runner.
actions/checkoutpersists the repository token by default.- Claude Code runs with
GH_TOKENand model API credentials while inspecting untrusted content. - Prompt injection, project-controlled configuration, hooks, or tool execution could potentially read and exfiltrate credentials.
Checkout v7 made this trust boundary explicit by requiring allow-unsafe-pr-checkout: true for fork PR heads in pull_request_target and workflow_run workflows: https://github.com/actions/checkout/pull/2454
Related: #795 and #541.
Agent Plan / Findings
Implement the fork/non-fork split first. The preferred fork path is metadata-plus-diff review through a direct, tool-free model request. This retains useful automated review while removing the need to place fork code in the secret-bearing workspace.
After implementation, validate workflow syntax locally and perform a controlled fork PR smoke test. Confirm that the reviewed SHA appears in workflow logs and that updating the fork requires new authorization.
Dependencies
Short-term compatibility opt-in in PR #795.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with the current pull_request_target workflow and compare its checkout and credential handling with the compatibility change in PR #795 and the original move in #541. Trace the fork and same-repository paths, then validate the workflow and controlled fork-PR scenarios described in the issue; done means fork content is never executed or checked out, authorization is tied to the head SHA, and useful review results remain.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- github-actions
- Domain
- ci-cd, devops, security
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100