feat: fork-PR trust hardening for memory writes (#6)#52
Merged
Conversation
Detect cross-repo (fork) PRs from the PR metadata (head repo id != base, or a deleted head repo) and thread head_is_fork through ResolvedTargets. The memory trust derivation now maps a fork PR to ForkPr — which grants no write scope — overriding even a maintainer trigger, since the risk is the untrusted content, not the actor. Closes the follow-up flagged in #50: auto and command reviews of fork content can no longer write durable memory even with approval. Signed-off-by: Val Alexander <bunsthedev@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens hosted memory governance for pull-request reviews by reliably detecting fork PRs and ensuring fork-sourced content can never write durable memory, even when a maintainer triggers the run. It strengthens the adapter-side enforcement for issue #6 without changing default deployments (memory remains opt-in).
Changes:
- Add fork PR detection in GitHub PR metadata resolution (
head_is_fork), including tests for same-repo, cross-repo, and deleted-fork scenarios. - Thread
head_is_forkthrough target resolution so trust decisions can incorporate it. - Extract and unit-test a pure
derive_trustfunction that maps fork PRs toTrustScope::ForkPrand overrides actor standing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/worker/src/memory.rs | Adds derive_trust and tests to ensure fork PRs map to ForkPr and yield no write scope. |
| crates/worker/src/lib.rs | Threads head_is_fork into ResolvedTargets and uses derive_trust when computing the memory policy. |
| crates/github/src/repo.rs | Extends PR refs fetch to compute and return head_is_fork via PR metadata, with unit tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// maintainer who triggered the review. Otherwise a commander (who already | ||
| /// passed the #13 write-access gate) is a maintainer, and auto-triggered work | ||
| /// gets the safe collaborator default. | ||
| pub fn derive_trust(head_is_fork: bool, has_commander: bool) -> TrustScope { |
Comment on lines
+47
to
+49
| /// A PR is a fork PR when its head repository differs from its base. A | ||
| /// missing head repo means the fork was deleted — treated as a fork | ||
| /// (untrusted) so memory stays fail-closed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the fork-PR trust follow-up flagged in #50. Small, self-contained hardening of the #6 memory enforcement.
The gap
Phase 2 derived memory trust as: commander → maintainer, otherwise collaborator. A fork PR therefore landed as
collaborator, which forbids durable writes without approval — but a fork PR is untrusted content: facts a hostile fork author plants could be written into durable memory (with approval) and poison later reviews. The contract's hard rule is that fork content can never write durable memory.The fix
crates/github/src/repo.rs): the PR metadata fetch now reportshead_is_fork— true when the head repo id differs from the base, or the head repo is absent (a deleted fork, treated as untrusted so memory stays fail-closed). Unit tests cover same-repo, cross-repo, and deleted-head-repo.ResolvedTargets(false for all non-PR tasks).crates/worker/src/memory.rs::derive_trust): a fork PR maps toForkPr— no write scope — and overrides the actor, so even a maintainer's@familiar reviewon a fork PR can't write durable memory. Same-repo command reviews stayMaintainer; auto-triggered same-repo work staysCollaborator.Extracting
derive_trustas a pure function makes the override rule directly unit-testable (fork overrides maintainer; the fork grant carries through to an empty write scope).This tightens genuine fork PRs from "no durable write without approval" to "no durable write, period", while leaving default deployments unchanged (memory still off by default).
Local gates:
cargo check --all-targets+cargo clippy --all-targets -- -D warnings+cargo test --all(171 passed, 0 failed).🤖 Generated with Claude Code