fix(webhook,worker): permission-gate cancel and memory commands (#13)#47
Closed
BunsDev wants to merge 1 commit into
Closed
fix(webhook,worker): permission-gate cancel and memory commands (#13)#47BunsDev wants to merge 1 commit into
BunsDev wants to merge 1 commit into
Conversation
PR #39 let router-side command handling mutate the PR review supersession registry before the worker permission gate, so drive-by commenters could cancel or supersede queued reviews and receive ungated acknowledgements. Memory command acknowledgements also bypassed the gate because CommandReply returned before commander permission checks. Route cancel into an adapter-only CancelReviews task, move the cancel tombstone into the worker after the existing write-permission gate, and gate remember/forget replies by carrying the commander. Keep status and clarification replies ungated, and register command-created reviews only after the worker authorizes the commander. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: Val Alexander <bunsthedev@gmail.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the adapter’s permission-gated command handling by moving cancellation and certain command replies fully behind the worker-side write-access gate, preventing drive-by users from triggering side effects (notably review-lane cancellation) in the webhook/router.
Changes:
- Introduces
TaskKind::CancelReviews { pr_number }and routescancelto a worker-executed, permission-gated cancellation flow instead of mutating the supersession registry in the webhook handler. - Ensures
remember/forgetacknowledgements carry acommanderso they are permission-gated like other maintainer commands. - Moves commanded-review supersession registration worker-side (post-gate) and adjusts auto-review supersession checks to be auto-review-only.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| crates/worker/src/lib.rs | Adds worker-side permission gating ahead of adapter-only fast paths; implements CancelReviews side effects (tombstone + status-surface ack) post-gate. |
| crates/worker/src/brief.rs | Marks CancelReviews as adapter-only (should never reach briefing) via unreachable! arms. |
| crates/webhook/src/routes.rs | Stops router-side supersession mutations for commanded reviews/cancel; routes cancel to CancelReviews and gates memory acks by attaching commander. |
| crates/github/src/{lib.rs,tasks.rs} | Extends task model/list rendering for the new CancelReviews task kind. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+141
to
+148
| Err(e) => { | ||
| if !adapter_only { | ||
| task_store | ||
| .register_failed(&task, &familiar.display_name) | ||
| .await; | ||
| } | ||
| return Err(e); | ||
| } |
Comment on lines
+222
to
+226
| let body = format!( | ||
| "Cancelled queued reviews for PR #{}. Work already running will finish; `@{} review` re-arms the lane.", | ||
| pr_number, | ||
| familiar.bot_username.trim_end_matches("[bot]") | ||
| ); |
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.
Follow-up to #39. Code review after merge found that two command paths bypassed the documented write-access gate.
The bug
cancel(security): the router tombstoned the review-supersession registry (register_pr_review(..., "cancelled:<uuid>")) synchronously in the webhook handler — before any permission check — and acked with a commander-lessCommandReply, so the worker's gate never fired. Any GitHub user able to comment on a public repo could kill queued PR reviews (drive-by DoS on the review lane) and receive a confirmation, contradicting the README ("every command exceptstatusrequires write access").remember/forget: acks carried no commander, so they were ungated. Harmless today (persistence is deferred to Define hosted memory governance contract between coven-github and coven-code #6), but the gate must exist before Define hosted memory governance contract between coven-github and coven-code #6 wires real side effects.reviewregistered its supersession generation in the router pre-gate, letting a below-write commander supersede queued auto-reviews before being declined.The fix
TaskKind::CancelReviews { pr_number }carryingcommander; the router no longer mutates the registry. The worker runs the permission gate first (belowwrite→ polite decline on the marker status surface, zero side effects), then tombstones and acks. No Check Run, no coven-code session.CommandReplyfast path; remember/forget acks carry the commander. Clarification andstatusreplies remain deliberately ungated.Locked contract v2 untouched —
CancelReviews, likeCommandReply, never reaches a session brief.Tests
CancelReviewsand does not tombstone (asserted against the registry through the real webhook handler with a valid HMAC signature); memory acks carry the commander; commanded review does not supersede queued reviews in the router.CommandReplyis declined before the fast path; commanded review registers only after the gate passes.Gates
cargo check --all-targets✅ ·cargo clippy --all-targets -- -D warnings✅ ·cargo test --all✅ — commit GPG-signed and DCO signed-off.