diff --git a/crates/github/src/lib.rs b/crates/github/src/lib.rs index c1da060..6afe424 100644 --- a/crates/github/src/lib.rs +++ b/crates/github/src/lib.rs @@ -238,6 +238,11 @@ pub enum TaskKind { issue_number: u64, body: String, }, + /// Adapter-only cancellation of queued PR reviews (issue #13). The worker + /// gates the commander's write access before tombstoning anything. + CancelReviews { + pr_number: u64, + }, } /// Structured result envelope written by coven-code --headless. diff --git a/crates/github/src/tasks.rs b/crates/github/src/tasks.rs index 6756c09..85db30c 100644 --- a/crates/github/src/tasks.rs +++ b/crates/github/src/tasks.rs @@ -63,6 +63,9 @@ pub fn surface_of(kind: &TaskKind) -> (u64, String) { TaskKind::CommandReply { issue_number, .. } => { (*issue_number, format!("Reply on #{issue_number}")) } + TaskKind::CancelReviews { pr_number } => { + (*pr_number, format!("Cancel queued reviews on PR #{pr_number}")) + } } } diff --git a/crates/store/src/lib.rs b/crates/store/src/lib.rs index 9c8ed6f..0dd3e15 100644 --- a/crates/store/src/lib.rs +++ b/crates/store/src/lib.rs @@ -303,14 +303,35 @@ impl Store { /// Tombstones every still-queued task for a supersession key (the /// maintainer `cancel` command). Returns how many were cancelled. pub async fn cancel_queued(&self, supersede_key: &str) -> Result { + self.supersede_queued(supersede_key, None).await + } + + /// Post-gate supersession for a command-initiated review (issue #13): + /// once the worker has verified the commander's write access, older + /// queued reviews of the same PR yield to `current_task_id`. + pub async fn supersede_queued_except( + &self, + supersede_key: &str, + current_task_id: &str, + ) -> Result { + self.supersede_queued(supersede_key, Some(current_task_id.to_string())) + .await + } + + async fn supersede_queued( + &self, + supersede_key: &str, + except_task_id: Option, + ) -> Result { let conn = self.conn.clone(); let key = supersede_key.to_string(); tokio::task::spawn_blocking(move || { let conn = conn.lock().expect("store mutex poisoned"); let n = conn.execute( "UPDATE tasks SET state = 'superseded', updated_at = ?1 - WHERE supersede_key = ?2 AND state = 'queued'", - params![now_rfc3339(), key], + WHERE supersede_key = ?2 AND state = 'queued' + AND (?3 IS NULL OR id <> ?3)", + params![now_rfc3339(), key, except_task_id], )?; Ok(n) }) @@ -542,13 +563,19 @@ fn record_delivery_sync( if let Some(task) = task { let repo = format!("{}/{}", task.repo_owner, task.repo_name); let supersede_key = supersede_key(task); - if let Some(key) = &supersede_key { - // A newer review of the same PR supersedes anything still queued. - tx.execute( - "UPDATE tasks SET state = 'superseded', updated_at = ?1 - WHERE supersede_key = ?2 AND state = 'queued'", - params![now, key], - )?; + // Only adapter-initiated (auto) reviews may tombstone at insert. + // Command-initiated reviews carry a commander whose write access the + // worker has not yet verified — they supersede older queued reviews + // post-gate instead (issue #13), so a drive-by `review` comment can + // never displace legitimate queued work. + if task.commander.is_none() { + if let Some(key) = &supersede_key { + tx.execute( + "UPDATE tasks SET state = 'superseded', updated_at = ?1 + WHERE supersede_key = ?2 AND state = 'queued'", + params![now, key], + )?; + } } tx.execute( "INSERT INTO tasks @@ -929,3 +956,87 @@ mod queue_tests { assert_eq!(items[0].status, TaskListStatus::Queued); } } + +#[cfg(test)] +mod command_gate_tests { + //! Insert-time supersession is an auto-review privilege; command reviews + //! wait for the worker's write-access gate (issue #13). + use super::*; + + fn delivery(id: &str) -> Delivery { + Delivery { + delivery_id: id.to_string(), + event: "pull_request".to_string(), + action: Some("synchronize".to_string()), + installation_id: Some(1), + repo: Some("OpenCoven/demo".to_string()), + payload_hash: "h".to_string(), + } + } + + fn review(id: &str, commander: Option<&str>) -> Task { + Task { + id: id.to_string(), + installation_id: 1, + repo_owner: "OpenCoven".to_string(), + repo_name: "demo".to_string(), + familiar_id: "cody".to_string(), + commander: commander.map(str::to_string), + kind: TaskKind::ReviewPullRequest { + pr_number: 88, + pr_title: "t".to_string(), + reason: "synchronize".to_string(), + }, + } + } + + #[tokio::test] + async fn command_review_does_not_tombstone_at_insert() { + let store = Store::open_in_memory().expect("open"); + store + .record_delivery(delivery("d1"), Routing::Task(&review("auto", None))) + .await + .expect("auto review"); + // An unverified commander's review must not displace queued work. + store + .record_delivery( + delivery("d2"), + Routing::Task(&review("commanded", Some("drive-by"))), + ) + .await + .expect("commanded review"); + + let states: HashMap = + store.task_states().await.unwrap().into_iter().collect(); + assert_eq!(states["auto"], "queued", "insert-time tombstone is auto-only"); + assert_eq!(states["commanded"], "queued"); + } + + #[tokio::test] + async fn post_gate_supersession_spares_the_current_task() { + let store = Store::open_in_memory().expect("open"); + store + .record_delivery(delivery("d1"), Routing::Task(&review("older", None))) + .await + .expect("older review"); + store + .record_delivery( + delivery("d2"), + Routing::Task(&review("commanded", Some("octocat"))), + ) + .await + .expect("commanded review"); + + // The worker calls this once the commander passed the write gate. + let n = store + .supersede_queued_except("OpenCoven/demo#88", "commanded") + .await + .expect("supersede"); + assert_eq!(n, 1); + + let states: HashMap = + store.task_states().await.unwrap().into_iter().collect(); + assert_eq!(states["older"], "superseded"); + assert_eq!(states["commanded"], "queued", "the commanding task survives"); + } +} diff --git a/crates/webhook/src/routes.rs b/crates/webhook/src/routes.rs index 9cf96e1..126d63a 100644 --- a/crates/webhook/src/routes.rs +++ b/crates/webhook/src/routes.rs @@ -405,8 +405,9 @@ struct CommandSurface<'a> { commander: &'a str, } -/// Maps a typed maintainer command to a task. Work commands carry the -/// commander for the worker's permission gate; replies carry none. +/// Maps a typed maintainer command to a task. Work commands and gated +/// acknowledgements (cancel, remember/forget) carry the commander for the +/// worker's permission gate; clarifications and status replies carry none. async fn command_task( state: &AppState, familiar: &coven_github_config::FamiliarConfig, @@ -472,29 +473,27 @@ async fn command_task( }, commander, ), - Command::Cancel if s.on_pull_request => { - // Tombstone queued reviews for this PR. In-flight work is not - // interrupted (documented limitation); the next review command or - // PR event re-arms the lane. - let cancelled = state - .store - .cancel_queued(&format!("{repo}#{}", s.number)) - .await - .unwrap_or_else(|e| { - warn!("cancel could not reach the store: {e:#}"); - 0 - }); - reply(format!( - "Cancelled {cancelled} queued review(s) for PR #{}. Work already running will finish; `@{} review` re-arms the lane.", - s.number, - familiar.bot_username.trim_end_matches("[bot]") - )) - } + // Cancellation mutates queued work, so it rides a gated adapter task: + // the worker verifies the commander's write access before tombstoning + // anything (issue #13). In-flight work is not interrupted (documented + // limitation); the next review command or PR event re-arms the lane. + Command::Cancel if s.on_pull_request => make( + TaskKind::CancelReviews { + pr_number: s.number, + }, + commander, + ), Command::Cancel => reply("`cancel` currently applies to queued pull-request reviews only.".to_string()), - Command::Remember { .. } | Command::Forget { .. } => reply( - "Noted, but memory persistence is not wired up yet — it lands with the hosted \ - memory governance contract (#6). Nothing was stored or deleted." - .to_string(), + // Memory acknowledgements are gated too: only maintainers should hear + // how the familiar handles memory intents. + Command::Remember { .. } | Command::Forget { .. } => make( + TaskKind::CommandReply { + issue_number: s.number, + body: "Noted, but memory persistence is not wired up yet — it lands with the \ + hosted memory governance contract (#6). Nothing was stored or deleted." + .to_string(), + }, + commander, ), Command::Status => { let items = state @@ -1096,7 +1095,7 @@ mod command_routing_tests { } #[tokio::test] - async fn cancel_command_tombstones_queued_reviews_and_acknowledges() { + async fn cancel_command_rides_a_gated_task_not_a_route_side_mutation() { let state = app_state(); let queued = Task { id: "task-queued".to_string(), @@ -1115,21 +1114,21 @@ mod command_routing_tests { let task = event_to_task(&state, pr_comment("@coven-cody cancel")) .await - .expect("cancel should acknowledge"); + .expect("cancel should route"); + // The route must NOT tombstone anything — the worker does, after the + // commander's write access is verified (issue #13). let states = state.store.task_states().await.expect("states"); assert_eq!( states, - vec![("task-queued".to_string(), "superseded".to_string())], - "queued review must be superseded by the cancel tombstone" + vec![("task-queued".to_string(), "queued".to_string())], + "queued review must be untouched until the gate passes" ); match task.kind { - TaskKind::CommandReply { issue_number, body } => { - assert_eq!(issue_number, 88); - assert!(body.contains("Cancelled 1 queued review")); - } - other => panic!("expected CommandReply, got {other:?}"), + TaskKind::CancelReviews { pr_number } => assert_eq!(pr_number, 88), + other => panic!("expected CancelReviews, got {other:?}"), } + assert_eq!(task.commander.as_deref(), Some("octocat")); } #[tokio::test] @@ -1138,6 +1137,9 @@ mod command_routing_tests { let task = event_to_task(&state, pr_comment("@coven-cody remember we ship Fridays")) .await .expect("remember should acknowledge"); + // The acknowledgement is gated: it carries the commander so the + // worker verifies write access before replying (issue #13). + assert_eq!(task.commander.as_deref(), Some("octocat")); match task.kind { TaskKind::CommandReply { body, .. } => { assert!(body.contains("#6")); diff --git a/crates/worker/src/brief.rs b/crates/worker/src/brief.rs index b871674..d06d473 100644 --- a/crates/worker/src/brief.rs +++ b/crates/worker/src/brief.rs @@ -114,9 +114,11 @@ pub fn build( // adapter-initiated review lane rides on pr_review_comment plus // review_context until native pull_request/push triggers land in v3. TaskKind::ReviewPullRequest { .. } => "pr_review_comment", - // CommandReply is executed adapter-side before briefing (issue #13); - // this arm is a safe fallback, not an expected path. + // CommandReply and CancelReviews are executed adapter-side before + // briefing (issue #13); these arms are safe fallbacks, not expected + // paths. TaskKind::CommandReply { .. } => "issue_mention", + TaskKind::CancelReviews { .. } => "issue_mention", }; let task_brief = match &task.kind { @@ -149,6 +151,10 @@ pub fn build( issue_number: *issue_number, comment_body: body.clone(), }, + TaskKind::CancelReviews { pr_number } => TaskBrief::RespondToMention { + issue_number: *pr_number, + comment_body: format!("Cancel queued reviews for PR #{pr_number}."), + }, TaskKind::ReviewPullRequest { pr_number, pr_title, diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index 9a7c123..9882a51 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -115,32 +115,90 @@ async fn execute_task_with_minter( .as_deref() .unwrap_or(DEFAULT_API_BASE_URL); - // Command replies are adapter-only (issue #13): update the status surface - // and finish — no coven-code session, no Check Run. - if let TaskKind::CommandReply { issue_number, body } = &task.kind { - let orchestration = minter.mint(TokenRole::Orchestration).await?; - let marker = - status_comment::marker(&familiar.id, &task.repo_owner, &task.repo_name, *issue_number); - status_comment::upsert( - api_base_url, - &orchestration, - &task.repo_owner, - &task.repo_name, - *issue_number, - &marker, - body, - ) - .await?; - store - .finish( - &task.id, - Terminal { - state: TerminalState::Completed, - ..Terminal::default() - }, + // Adapter-only command surfaces (issue #13): replies, acknowledgements, + // and cancellations run without a coven-code session or Check Run — but + // gated acknowledgements and cancellations still verify the commander's + // write access first, so a drive-by comment earns a decline, not an act. + match &task.kind { + TaskKind::CommandReply { issue_number, body } => { + let orchestration = minter.mint(TokenRole::Orchestration).await?; + let reply = if commander_below_write(api_base_url, &orchestration, &task).await? { + info!(task_id = %task.id, "declining gated reply for a commander without write access"); + decline_body(&task) + } else { + body.clone() + }; + let marker = status_comment::marker( + &familiar.id, + &task.repo_owner, + &task.repo_name, + *issue_number, + ); + status_comment::upsert( + api_base_url, + &orchestration, + &task.repo_owner, + &task.repo_name, + *issue_number, + &marker, + &reply, + ) + .await?; + store + .finish( + &task.id, + Terminal { + state: TerminalState::Completed, + ..Terminal::default() + }, + ) + .await?; + return Ok(()); + } + TaskKind::CancelReviews { pr_number } => { + let orchestration = minter.mint(TokenRole::Orchestration).await?; + let reply = if commander_below_write(api_base_url, &orchestration, &task).await? { + info!(task_id = %task.id, "declining cancel for a commander without write access"); + decline_body(&task) + } else { + // The tombstone happens only past the gate: queued reviews of + // this PR yield; in-flight work finishes (#8 covers staleness). + let key = format!("{}/{}#{pr_number}", task.repo_owner, task.repo_name); + let cancelled = store.cancel_queued(&key).await?; + format!( + "Cancelled {cancelled} queued review(s) for PR #{pr_number}. Work already \ + running will finish; `@{} review` re-arms the lane.", + familiar.bot_username.trim_end_matches("[bot]") + ) + }; + let marker = status_comment::marker( + &familiar.id, + &task.repo_owner, + &task.repo_name, + *pr_number, + ); + status_comment::upsert( + api_base_url, + &orchestration, + &task.repo_owner, + &task.repo_name, + *pr_number, + &marker, + &reply, ) .await?; - return Ok(()); + store + .finish( + &task.id, + Terminal { + state: TerminalState::Completed, + ..Terminal::default() + }, + ) + .await?; + return Ok(()); + } + _ => {} } info!(task_id = %task.id, familiar = %familiar.id, "starting task"); @@ -156,17 +214,17 @@ async fn execute_task_with_minter( // Maintainer permission gate (issue #13): command-initiated work needs // write access on the repo before the adapter spends anything on it. - if let Some(commander) = &task.commander { - let permission = repo::get_collaborator_permission_with_base_url( - api_base_url, - &orchestration, - &task.repo_owner, - &task.repo_name, - commander, - ) - .await?; - if !matches!(permission.as_str(), "admin" | "maintain" | "write") { - return Ok(Prepared::Declined { orchestration }); + if commander_below_write(api_base_url, &orchestration, &task).await? { + return Ok(Prepared::Declined { orchestration }); + } + + // A now-authorized command review supersedes older queued reviews of + // the same PR. Auto reviews tombstone at insert; command reviews wait + // for this gate so unauthorized commenters can't displace queued work. + if task.commander.is_some() { + if let TaskKind::ReviewPullRequest { pr_number, .. } = &task.kind { + let key = format!("{}/{}#{pr_number}", task.repo_owner, task.repo_name); + store.supersede_queued_except(&key, &task.id).await?; } } @@ -215,10 +273,7 @@ async fn execute_task_with_minter( &task.repo_name, number, ); - let body = format!( - "Status: declined\n\nMaintainer commands need write access to {}/{}.", - task.repo_owner, task.repo_name - ); + let body = decline_body(&task); if let Err(e) = status_comment::upsert( api_base_url, &orchestration, @@ -1123,6 +1178,35 @@ fn pr_title(result: &SessionResult, task: &Task) -> String { ) } +/// True when the task carries a commander whose repository permission is +/// below write (issue #13). Auto-triggered tasks (no commander) always pass. +async fn commander_below_write( + api_base_url: &str, + orchestration: &str, + task: &Task, +) -> Result { + 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")) +} + +/// Status-surface body for a below-write commander. +fn decline_body(task: &Task) -> String { + format!( + "Status: declined\n\nMaintainer commands need write access to {}/{}.", + task.repo_owner, task.repo_name + ) +} + fn task_title(kind: &TaskKind) -> String { match kind { TaskKind::FixIssue { @@ -1142,6 +1226,9 @@ fn task_title(kind: &TaskKind) -> String { .. } => format!("Review PR #{pr_number}: {pr_title}"), TaskKind::CommandReply { issue_number, .. } => format!("Reply on #{issue_number}"), + TaskKind::CancelReviews { pr_number } => { + format!("Cancel queued reviews on PR #{pr_number}") + } } } @@ -1181,7 +1268,8 @@ async fn resolve_targets(api_base_url: &str, token: &str, task: &Task) -> Result } TaskKind::FixIssue { .. } | TaskKind::RespondToMention { .. } - | TaskKind::CommandReply { .. } => { + | TaskKind::CommandReply { .. } + | TaskKind::CancelReviews { .. } => { let head_sha = repo::get_branch_sha_with_base_url( api_base_url, token, @@ -1207,7 +1295,8 @@ fn surface_number(kind: &TaskKind) -> Option { | TaskKind::RespondToMention { issue_number, .. } | TaskKind::CommandReply { issue_number, .. } => Some(*issue_number), TaskKind::AddressReviewComment { pr_number, .. } - | TaskKind::ReviewPullRequest { pr_number, .. } => Some(*pr_number), + | TaskKind::ReviewPullRequest { pr_number, .. } + | TaskKind::CancelReviews { pr_number } => Some(*pr_number), } } @@ -2508,4 +2597,172 @@ mod command_and_marker_tests { assert_eq!(states.len(), 1); assert_eq!(states[0].1, "completed"); } + async fn seed(store: &Store, delivery_id: &str, task: &Task) { + store + .record_delivery( + coven_github_store::Delivery { + delivery_id: delivery_id.to_string(), + event: "issue_comment".to_string(), + action: Some("created".to_string()), + installation_id: Some(1), + repo: Some("OpenCoven/demo".to_string()), + payload_hash: "h".to_string(), + }, + coven_github_store::Routing::Task(task), + ) + .await + .expect("seed task row"); + } + + 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 })), + ) + } + + fn comment_mocks() -> (Mock, Mock) { + ( + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/issues/88/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))), + Mock::given(method("POST")) + .and(path("/repos/OpenCoven/demo/issues/88/comments")) + .respond_with( + ResponseTemplate::new(201).set_body_json(serde_json::json!({"id": 1})), + ), + ) + } + + fn queued_review(id: &str) -> Task { + task( + TaskKind::ReviewPullRequest { + pr_number: 88, + pr_title: "t".to_string(), + reason: "synchronize".to_string(), + }, + None, + ) + .with_id(id) + } + + /// A drive-by `cancel` must decline at the gate and leave queued reviews + /// untouched (issue #13 privilege fix). + #[tokio::test] + async fn drive_by_cancel_is_declined_and_tombstones_nothing() { + let server = MockServer::start().await; + permission_mock("drive-by", "read").mount(&server).await; + let (get_comments, post_comment) = comment_mocks(); + get_comments.mount(&server).await; + post_comment.mount(&server).await; + + let store = Store::open_in_memory().expect("store"); + seed(&store, "dl-q", &queued_review("victim")).await; + let cancel = task(TaskKind::CancelReviews { pr_number: 88 }, Some("drive-by")) + .with_id("cancel-task"); + seed(&store, "dl-c", &cancel).await; + + execute_task_with_minter(&config(server.uri()), store.clone(), cancel, &fixed_minter()) + .await + .expect("declined cancel is not an error"); + + let states: std::collections::HashMap = + store.task_states().await.unwrap().into_iter().collect(); + assert_eq!(states["victim"], "queued", "no tombstone below the gate"); + assert_eq!(states["cancel-task"], "completed"); + let requests = server.received_requests().await.expect("requests"); + let posted = requests + .iter() + .find(|r| r.method.as_str() == "POST") + .expect("decline posted"); + let body = String::from_utf8_lossy(&posted.body); + assert!(body.contains("Status: declined"), "body: {body}"); + } + + /// A maintainer `cancel` passes the gate, tombstones the queued review, + /// and acknowledges with the count. + #[tokio::test] + async fn maintainer_cancel_tombstones_queued_reviews_past_the_gate() { + let server = MockServer::start().await; + permission_mock("octocat", "admin").mount(&server).await; + let (get_comments, post_comment) = comment_mocks(); + get_comments.mount(&server).await; + post_comment.mount(&server).await; + + let store = Store::open_in_memory().expect("store"); + seed(&store, "dl-q", &queued_review("victim")).await; + let cancel = + task(TaskKind::CancelReviews { pr_number: 88 }, Some("octocat")).with_id("cancel-task"); + seed(&store, "dl-c", &cancel).await; + + execute_task_with_minter(&config(server.uri()), store.clone(), cancel, &fixed_minter()) + .await + .expect("cancel should succeed"); + + let states: std::collections::HashMap = + store.task_states().await.unwrap().into_iter().collect(); + assert_eq!(states["victim"], "superseded"); + let requests = server.received_requests().await.expect("requests"); + let posted = requests + .iter() + .find(|r| r.method.as_str() == "POST") + .expect("ack posted"); + let body = String::from_utf8_lossy(&posted.body); + assert!(body.contains("Cancelled 1 queued review"), "body: {body}"); + assert!( + !requests.iter().any(|r| r.url.path().contains("check-runs")), + "cancel is adapter-only — no Check Run" + ); + } + + /// Memory acknowledgements carry the commander and decline below write. + #[tokio::test] + async fn drive_by_memory_ack_is_declined() { + let server = MockServer::start().await; + permission_mock("drive-by", "read").mount(&server).await; + let (get_comments, post_comment) = comment_mocks(); + get_comments.mount(&server).await; + post_comment.mount(&server).await; + + execute_task_with_minter( + &config(server.uri()), + Store::open_in_memory().expect("store"), + task( + TaskKind::CommandReply { + issue_number: 88, + body: "Noted, but memory persistence is not wired up yet".to_string(), + }, + Some("drive-by"), + ), + &fixed_minter(), + ) + .await + .expect("declined ack is not an error"); + + let requests = server.received_requests().await.expect("requests"); + let posted = requests + .iter() + .find(|r| r.method.as_str() == "POST") + .expect("reply posted"); + let body = String::from_utf8_lossy(&posted.body); + assert!(body.contains("Status: declined"), "body: {body}"); + assert!(!body.contains("Noted"), "the ungated ack must not leak"); + } +} + +/// Test-only convenience for retargeting a task id. +#[cfg(test)] +trait WithId { + fn with_id(self, id: &str) -> Self; +} +#[cfg(test)] +impl WithId for Task { + fn with_id(mut self, id: &str) -> Self { + self.id = id.to_string(); + self + } }