fix: permission-gate cancel, memory acks, and command supersession (#13)#49
Merged
Merged
Conversation
…ommand supersession (#13) Drive-by commenters could steer queued work: the cancel command tombstoned queued reviews at the route (pre-gate), memory acknowledgements replied ungated, and a command review superseded queued reviews at durable insert — all before anyone checked the commander's write access. Now every mutating or acknowledging command lane passes the worker's write gate first: - cancel rides a new adapter-only CancelReviews task; the tombstone happens in the worker after the gate, and below-write commanders get the decline body instead - remember/forget acknowledgements carry the commander and decline below write; status and clarification replies stay ungated - insert-time supersession is auto-review-only: command reviews supersede older queued reviews post-gate via supersede_queued_except, sparing the commanding task itself Re-lands the intent of fix/issue-13-command-gates (pre-durable-store) adapted to the SQLite queue from #2. 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 tightens the maintainer-command permission model (issue #13) in the GitHub App adapter by ensuring that “adapter-only” command surfaces (cancel + memory acknowledgements) cannot mutate queued work or reveal gated acknowledgement text until the worker has verified the commander has write access.
Changes:
- Routes
@… cancelinto a new adapter-onlyTaskKind::CancelReviewsso queued-review tombstoning happens only in the worker post-permission-gate (not in the webhook route). - Gates memory acknowledgement
CommandReplytasks by carrying the commander through to the worker and declining below-write without leaking the ungated acknowledgement body. - Restricts insert-time review supersession to auto-triggered reviews (no commander) and adds post-gate supersession for authorized command reviews via
supersede_queued_except.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/worker/src/lib.rs | Executes CancelReviews and commander-carrying CommandReply tasks adapter-side with permission gating; adds post-gate review supersession and new worker tests. |
| crates/worker/src/brief.rs | Adds safe-fallback briefing mappings for CancelReviews (though it should not normally reach briefing). |
| crates/webhook/src/routes.rs | Stops route-side cancel tombstoning; emits CancelReviews tasks and gates remember/forget acks by carrying the commander. |
| crates/store/src/lib.rs | Adds supersede_queued_except and makes insert-time supersession auto-review-only; adds store tests for command-gate behavior. |
| crates/github/src/{lib.rs,tasks.rs} | Extends task kind enum and surface naming to include CancelReviews. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1183
to
+1199
| async fn commander_below_write( | ||
| api_base_url: &str, | ||
| orchestration: &str, | ||
| task: &Task, | ||
| ) -> Result<bool> { | ||
| let Some(commander) = &task.commander else { | ||
| return Ok(false); | ||
| }; | ||
| let permission = repo::get_collaborator_permission_with_base_url( | ||
| api_base_url, | ||
| orchestration, | ||
| &task.repo_owner, | ||
| &task.repo_name, | ||
| commander, | ||
| ) | ||
| .await?; | ||
| Ok(!matches!(permission.as_str(), "admin" | "maintain" | "write")) |
Comment on lines
+2617
to
+2626
| fn permission_mock(login: &str, permission: &str) -> Mock { | ||
| Mock::given(method("GET")) | ||
| .and(path(format!( | ||
| "/repos/OpenCoven/demo/collaborators/{login}/permission" | ||
| ))) | ||
| .respond_with( | ||
| ResponseTemplate::new(200) | ||
| .set_body_json(serde_json::json!({ "permission": permission })), | ||
| ) | ||
| } |
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.
Re-lands the privilege fix from
fix/issue-13-command-gates(written pre-durable-store) adapted to the SQLite queue that landed with #2.The holes (all real on
main)The #13 command protocol gated work commands (fix/review/retry) behind the worker's write-access check — but three lanes acted before any permission check:
@cody canceltombstoned queued reviews at the route — a drive-by commenter could cancel a maintainer's queued review@cody remember/forgetacknowledgements replied ungatedreviewsuperseded older queued reviews at durable insert — an unauthorized commenter could displace legitimate queued work just by askingThe fix
cancelrides a new adapter-onlyTaskKind::CancelReviews: the worker verifies the commander's write access first, then tombstones viastore.cancel_queuedand acknowledges with the count; below-write commanders getStatus: declined. No Check Run, no session — same adapter-only footprint as replies.commander IS NULL); command reviews supersede older queued reviews post-gate viasupersede_queued_except, which spares the commanding task itself.Verification (151 tests, all green)
cancelproduces a gatedCancelReviewstask and mutates nothing at the route;remembercarries the commander-D warningsclean · python gates green · demo 21/21 through the real binaryCredit: original analysis and approach from the
fix/issue-13-command-gatesbranch; this PR supersedes it (that branch predates the durable queue and no longer applies).