NVIDIA-NeMo / NVIDIA-NeMo/DataDesigner

Harden Agentic CI PR reviews for forked pull requests

Open
#804 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

task
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:

  1. Resolve and validate immutable PR context before any checkout:
    • PR number
    • base SHA
    • head SHA
    • head repository
    • whether the PR comes from a fork
  2. Keep the existing full Claude Code review path for same-repository PRs.
  3. 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.
  4. Set persist-credentials: false on checkouts and avoid making a write-capable repository token available to the model process.
  5. Preserve explicit maintainer authorization for fork reviews and bind approval to the reviewed head SHA. A changed head must require fresh authorization.
  6. Minimize workflow permissions and isolate secret-bearing steps. Prefer an ephemeral runner for any job processing untrusted input.
  7. Audit or disable project-controlled Claude settings, hooks, and instruction files anywhere untrusted content can be present.
  8. Add workflow validation for:
    • same-repository PRs
    • maintainer-approved fork PRs
    • fork head changes after approval
    • manual workflow_dispatch of 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/checkout persists the repository token by default.
  • Claude Code runs with GH_TOKEN and 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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.