Skip to content

Review untrusted pull requests for malicious intent - #59

Merged
matthewp merged 1 commit into
mainfrom
malicious-pr-check
Oct 30, 2026
Merged

matthewp merged 1 commit into
mainfrom
malicious-pr-check

Conversation

@matthewp

Copy link
Copy Markdown
Contributor

Changes

  • Adds an opt-in PR safety check (prSafety.enabled in factory.yml). It asks whether an untrusted pull request is an attack, not whether its code has bugs: a planted vulnerability, exfiltration, GitHub Actions abuse, supply-chain tampering, hidden payloads, or text aimed at AI agents. It runs on every push from authors who aren't OWNER/MEMBER/COLLABORATOR, and on renovate and dependabot pull requests, in public repos.
  • Two tiers per head. First, deterministic rule signals over the merge-base diff (src/pr-safety/signals.ts): workflows, install scripts, lockfile sources, hidden Unicode, credentials, new hosts, removed security checks, and prompt injection. Then a reviewer agent with the new skills/pr-safety skill, in release security's read-only checkout (no shell, no network), which checks each change against the PR's stated purpose and returns CLEAN/SUSPICIOUS/MALICIOUS/INCOMPLETE.
  • Results stay private: the report and signal table go to R2, and SUSPICIOUS/MALICIOUS post to the Discord security channel. The Factory PR safety check always concludes neutral with the same text, so it doesn't tell contributors whether they were flagged.
  • Fixes the router so fork pull requests to withastro/astro aren't rejected by the release security route before it checks whether the branch is a release branch. Before this, every fork PR was dropped there.
  • Adds a v9 migration for FluePrSafetyReviewerAgent and PrSafetyCoordinator. This conflicts with Bring back the Discord assistant on Houston, confirmed by mention #57, which also uses v9; whichever merges second renumbers.

Testing

  • tests/pr-safety-signals.test.ts: diff parsing (renames, quoted paths, binaries) and each rule family, including severity relaxing in tests, grouping, and the signal cap.
  • tests/pr-safety.test.ts: author scope, routing (including fork PRs to withastro/astro and release PRs still going to release security), result validation, and alert and report formatting with untrusted text.
  • tests/config.test.ts: the full-config equality now includes prSafety, plus a test that it's off by default and parses overrides.

Docs

  • README documents the capability, its two tiers, where results go, and the prSafety config section.

@matthewp
matthewp merged commit 8647744 into main Oct 30, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant