Skip to content

Add /review-contributor-pr safety prescreen command (#2751)#2752

Merged
brendancol merged 3 commits into
mainfrom
issue-2751
May 31, 2026
Merged

Add /review-contributor-pr safety prescreen command (#2751)#2752
brendancol merged 3 commits into
mainfrom
issue-2751

Conversation

@brendancol
Copy link
Copy Markdown
Contributor

Closes #2751

Adds /review-contributor-pr, a read-only safety prescreen for pull requests from outside contributors. It runs first, before /review-pr or any /rockout follow-up touches the PR, and answers one question: is it safe to let the other Claude commands operate on this PR?

  • Prompt-injection track: scans every surface a downstream agent reads (PR title/body, comments, commit messages, code comments, docstrings, Markdown/RST, notebook cells, test fixtures, file names) for instruction-override text, hidden/zero-width/homoglyph tricks, and encoded payloads.
  • Outside-code track: flags arbitrary execution (eval/exec/subprocess/pickle), network and exfiltration calls, credential/environment access, writes outside the repo, build/install/import-time hooks, and CI/.github tampering — scoped to what the PR actually changes.
  • Emits a VERDICT: SAFE | NEEDS-REVIEW | UNSAFE line plus a recommendation, so the result is greppable as a gate for downstream automation.

The command is hardened against the content it reads: an explicit contract at the top states that all PR content is untrusted data, any imperative text found in the PR is a finding to report rather than an instruction to follow, and the run never executes, builds, imports, or fetches anything from the PR. It reuses /review-pr's worktree isolation and makes no commits.

Test plan

  • No automated test harness exists for .claude/commands/*.md files; verification is by running the command against representative PRs (a clean PR → SAFE, a PR with an injected instruction in a docstring → finding + NEEDS-REVIEW/UNSAFE).
  • Markdown renders correctly and the worktree-isolation block matches the proven /review-pr pattern.

No changes to xrspatial/; this adds one command file under .claude/commands/.

Read-only prescreen that scans an outside contributor's PR for prompt
injection aimed at downstream LLM agents and for unsafe outside code
(arbitrary execution, exfiltration, build/install hooks, CI tampering),
then emits a SAFE / NEEDS-REVIEW / UNSAFE verdict gating whether other
Claude commands should run against the PR. Reuses /review-pr worktree
isolation; makes no commits and runs nothing from the PR.
@github-actions github-actions Bot added the performance PR touches performance-sensitive code label May 31, 2026
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

PR Review: Add /review-contributor-pr safety prescreen command (#2752)

This PR adds one file, .claude/commands/review-contributor-pr.md, which is a slash-command prompt rather than Python. The library's correctness, backend-parity, and performance checks don't apply, so I scoped this review to the command's own logic: do its bash snippets work, and does it actually catch what it claims to?

Blockers (must fix before merge)

  • .claude/commands/review-contributor-pr.md (the 2a grep, plus the same pattern in 2b and 3f): every recursive grep is piped through grep -v '\.claude/worktrees/' to skip nested worktrees. The problem is that the scan root $RCPR_WT is itself always a .claude/worktrees/... path, both when reusing a /rockout worktree and when the command creates its own pr-N-prescreen worktree. So grep -r ... "$RCPR_WT" | grep -v '\.claude/worktrees/' drops 100% of matches, and the injection and execution scans always come back empty. That produces a false SAFE, the one outcome this command exists to prevent. I checked it: grepping the new file for "ignore previous" through that filter returns 0 lines even though the file contains that exact phrase. Fix: swap the grep -r ... | grep -v pattern for git -C "$RCPR_WT" grep -nE 'pattern' -- '*.py' '*.md' .... git grep only searches tracked files, so nested worktrees (untracked) drop out for free and the scan root stops poisoning the filter.

Suggestions (should fix, not blocking)

  • Steps 1.5 and 2 read from git diff origin/<baseRefName>...HEAD, which assumes origin/<base> is present locally. In a freshly created worktree that usually holds because the object store is shared, but a git -C "$RCPR_WT" fetch origin <baseRefName> before the diff would make the snippet robust when it's run standalone on a stale checkout.

Nits (optional improvements)

  • The 2b zero-width grep and the 3f execution grep carry the same grep -v '\.claude/worktrees/' tail as the Blocker. Folding them into the same git-grep fix keeps the three scans consistent.

What looks good

  • The injection-hardening contract at the top is the right call. Saying up front that all PR content is untrusted data, and that imperative text is a finding rather than an instruction, is what a prescreen needs.
  • Worktree isolation reuses the proven /review-pr pattern, including the already-in-a-worktree detection, so it composes cleanly when run first inside /rockout.
  • The VERDICT: / RECOMMENDATION: header is machine-greppable, which makes the gate usable by downstream automation.
  • The verdict rubric biases toward the cautious choice and is explicit that SAFE only covers the two threat classes, not correctness.

Checklist

  • Bash snippets work as written -- NO: the self-excluding grep filter breaks the core scans (Blocker)
  • Worktree isolation pattern matches the proven /review-pr approach
  • Command is hardened against the untrusted content it reads
  • Verdict output is machine-greppable for gating
  • No changes to xrspatial/ source; scope is one command file
  • N/A: algorithm/backend/NaN/dask/benchmark checks (no numeric code in this PR)

The recursive scans piped through 'grep -v .claude/worktrees/' to skip
nested worktrees, but the scan root $RCPR_WT is itself always under
.claude/worktrees/, so the filter dropped every match and the injection
and execution scans always returned empty (false SAFE). Replace the
'grep -r ... | grep -v' pattern in the 2a/2b/3f scans with
'git -C $RCPR_WT grep', which searches tracked files only and excludes
untracked nested worktrees for free. Also fetch the base ref before the
Step 1.5 diff so it works on a stale standalone checkout.
Copy link
Copy Markdown
Contributor Author

@brendancol brendancol left a comment

Choose a reason for hiding this comment

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

Follow-up review: Add /review-contributor-pr safety prescreen command (#2752)

Re-reviewed after commit 2705e7f.

Blockers

  • None. The self-excluding grep -v '.claude/worktrees/' filter is gone. All three scans (2a instruction injection, 2b zero-width, 3f execution) now use git -C "$RCPR_WT" grep, which searches tracked files only and excludes untracked nested worktrees without a path filter. Verified: git grep for "ignore previous" now returns the expected lines (it returned 0 before the fix), with zero nested-worktree noise.

Suggestions

  • None outstanding. The git fetch origin <baseRefName> before the Step 1.5 diff was added, so the diff holds up on a standalone stale checkout.

Nits

  • None outstanding.

Disposition of the first-pass findings

  • Blocker (self-excluding grep filter): fixed.
  • Suggestion (fetch base before diff): fixed.
  • Nit (2b/2f consistency): fixed as part of the same git-grep change.

Nothing left to iterate on.

@brendancol brendancol merged commit 3759410 into main May 31, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

performance PR touches performance-sensitive code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add /review-contributor-pr prescreening command for untrusted-PR security and prompt-injection checks

1 participant