diff --git a/crates/github/src/lib.rs b/crates/github/src/lib.rs index c1da060..db7f553 100644 --- a/crates/github/src/lib.rs +++ b/crates/github/src/lib.rs @@ -234,10 +234,10 @@ pub enum TaskKind { /// Adapter-only reply on an issue/PR conversation (issue #13): command /// acknowledgements, clarifications, status answers, permission declines. /// Executed without spawning coven-code. - CommandReply { - issue_number: u64, - body: String, - }, + CommandReply { issue_number: u64, body: String }, + /// Adapter-only cancellation of queued PR reviews (issue #13). The worker + /// gates the commander before mutating the supersession registry. + 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 b6e8333..c1b45bf 100644 --- a/crates/github/src/tasks.rs +++ b/crates/github/src/tasks.rs @@ -162,6 +162,10 @@ fn task_list_item(task: &Task, familiar_name: &str) -> TaskListItem { TaskKind::CommandReply { issue_number, .. } => { (*issue_number, format!("Reply on #{issue_number}")) } + TaskKind::CancelReviews { pr_number } => ( + *pr_number, + format!("Cancel queued reviews for PR #{pr_number}"), + ), }; TaskListItem { diff --git a/crates/webhook/src/routes.rs b/crates/webhook/src/routes.rs index 19ee25d..5b1597d 100644 --- a/crates/webhook/src/routes.rs +++ b/crates/webhook/src/routes.rs @@ -107,12 +107,14 @@ pub async fn handle_webhook( let task_id = task.id.clone(); // Register auto-reviews BEFORE enqueueing so the worker can never // dequeue a task that a newer event has already superseded (#10). - if let TaskKind::ReviewPullRequest { pr_number, .. } = &task.kind { - let repo = format!("{}/{}", task.repo_owner, task.repo_name); - state - .task_store - .register_pr_review(&repo, *pr_number, &task_id) - .await; + if task.commander.is_none() { + if let TaskKind::ReviewPullRequest { pr_number, .. } = &task.kind { + let repo = format!("{}/{}", task.repo_owner, task.repo_name); + state + .task_store + .register_pr_review(&repo, *pr_number, &task_id) + .await; + } } if state.task_tx.try_send(task).is_err() { warn!(task_id, "task queue full — dropping task"); @@ -312,8 +314,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 replies +/// 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, @@ -379,25 +382,24 @@ 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. - state - .task_store - .register_pr_review(&repo, s.number, &format!("cancelled:{}", uuid::Uuid::new_v4())) - .await; - reply(format!( - "Cancelled queued reviews for PR #{}. Work already running will finish; `@{} review` re-arms the lane.", - s.number, - familiar.bot_username.trim_end_matches("[bot]") - )) + 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::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(), + 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.task_store.list().await; @@ -704,7 +706,10 @@ mod tests { TaskKind::CommandReply { issue_number, body } => { assert_eq!(issue_number, 42); assert!(body.contains("`can`")); - assert!(body.contains("`review`"), "reply should list commands: {body}"); + assert!( + body.contains("`review`"), + "reply should list commands: {body}" + ); } other => panic!("expected CommandReply, got {other:?}"), } @@ -761,7 +766,6 @@ mod tests { let state = app_state(); assert!(event_to_task(&state, GitHubEvent::Ping).await.is_none()); } - } #[cfg(test)] @@ -838,7 +842,9 @@ mod review_lane_tests { async fn pr_opened_is_ignored_when_lane_disabled() { let state = app_state(); assert!( - event_to_task(&state, GitHubEvent::PullRequestChanged(pr_event("opened"))).await.is_none() + event_to_task(&state, GitHubEvent::PullRequestChanged(pr_event("opened"))) + .await + .is_none() ); } @@ -848,7 +854,11 @@ mod review_lane_tests { let state = app_state_with_review(review_on()); let mut event = pr_event("opened"); event.author_login = "coven-cody[bot]".to_string(); - assert!(event_to_task(&state, GitHubEvent::PullRequestChanged(event)).await.is_none()); + assert!( + event_to_task(&state, GitHubEvent::PullRequestChanged(event)) + .await + .is_none() + ); } #[tokio::test] @@ -856,12 +866,20 @@ mod review_lane_tests { let state = app_state_with_review(review_on()); let mut event = pr_event("opened"); event.draft = true; - assert!(event_to_task(&state, GitHubEvent::PullRequestChanged(event.clone())).await.is_none()); + assert!( + event_to_task(&state, GitHubEvent::PullRequestChanged(event.clone())) + .await + .is_none() + ); let mut inclusive = review_on(); inclusive.include_drafts = true; let state = app_state_with_review(inclusive); - assert!(event_to_task(&state, GitHubEvent::PullRequestChanged(event)).await.is_some()); + assert!( + event_to_task(&state, GitHubEvent::PullRequestChanged(event)) + .await + .is_some() + ); } #[tokio::test] @@ -878,7 +896,9 @@ mod review_lane_tests { ); let state = app_state_with_review(review); assert!( - event_to_task(&state, GitHubEvent::PullRequestChanged(pr_event("opened"))).await.is_none() + event_to_task(&state, GitHubEvent::PullRequestChanged(pr_event("opened"))) + .await + .is_none() ); } @@ -901,7 +921,11 @@ mod review_lane_tests { let state = app_state_with_review(review_on()); let mut event = pr_event("labeled"); event.label_name = Some("help wanted".to_string()); - assert!(event_to_task(&state, GitHubEvent::PullRequestChanged(event)).await.is_none()); + assert!( + event_to_task(&state, GitHubEvent::PullRequestChanged(event)) + .await + .is_none() + ); } #[tokio::test] @@ -927,7 +951,26 @@ mod review_lane_tests { mod command_routing_tests { use super::tests::app_state; use super::*; + use axum::http::HeaderValue; use coven_github_api::PrReviewCommentEvent; + use hmac::{Hmac, Mac}; + use sha2::Sha256; + + fn signed_headers(event_type: &str, body: &[u8]) -> HeaderMap { + let mut mac = Hmac::::new_from_slice(b"secret").expect("HMAC accepts key"); + mac.update(body); + let signature = format!("sha256={}", hex::encode(mac.finalize().into_bytes())); + let mut headers = HeaderMap::new(); + headers.insert( + "x-hub-signature-256", + HeaderValue::from_str(&signature).expect("signature header is valid"), + ); + headers.insert( + "x-github-event", + HeaderValue::from_str(event_type).expect("event type header is valid"), + ); + headers + } fn pr_comment(body: &str) -> GitHubEvent { GitHubEvent::PullRequestReviewComment(PrReviewCommentEvent { @@ -941,6 +984,38 @@ mod command_routing_tests { }) } + #[tokio::test] + async fn review_command_does_not_supersede_queued_reviews_in_router() { + let state = app_state(); + state + .task_store + .register_pr_review("OpenCoven/coven-code", 88, "task-queued") + .await; + let body = serde_json::json!({ + "action": "created", + "installation": { "id": 123 }, + "repository": { "name": "coven-code", "owner": { "login": "OpenCoven" } }, + "pull_request": { "number": 88, "title": "Add spell compiler cache" }, + "comment": { "body": "@coven-cody review", "user": { "login": "octocat" } } + }) + .to_string(); + + let _response = handle_webhook( + State(state.clone()), + signed_headers("pull_request_review_comment", body.as_bytes()), + Bytes::from(body), + ) + .await; + + assert!( + state + .task_store + .is_current_pr_review("OpenCoven/coven-code", 88, "task-queued") + .await, + "router must not let an ungated review command supersede queued reviews" + ); + } + #[tokio::test] async fn review_command_on_pr_creates_commanded_review() { let state = app_state(); @@ -972,7 +1047,7 @@ mod command_routing_tests { } #[tokio::test] - async fn cancel_command_tombstones_queued_reviews_and_acknowledges() { + async fn cancel_command_routes_authorized_worker_side_tombstone() { let state = app_state(); state .task_store @@ -981,36 +1056,39 @@ mod command_routing_tests { let task = event_to_task(&state, pr_comment("@coven-cody cancel")) .await - .expect("cancel should acknowledge"); + .expect("cancel should enqueue gated worker-side cancellation"); assert!( - !state + state .task_store .is_current_pr_review("OpenCoven/coven-code", 88, "task-queued") .await, - "queued review must be superseded by the cancel tombstone" + "router must not tombstone before the worker permission gate" ); + assert_eq!(task.commander.as_deref(), Some("octocat")); match task.kind { - TaskKind::CommandReply { issue_number, body } => { - assert_eq!(issue_number, 88); - assert!(body.contains("Cancelled queued reviews")); + TaskKind::CancelReviews { pr_number } => { + assert_eq!(pr_number, 88); } - other => panic!("expected CommandReply, got {other:?}"), + other => panic!("expected CancelReviews, got {other:?}"), } } #[tokio::test] async fn memory_commands_are_acknowledged_but_deferred() { - let state = app_state(); - let task = event_to_task(&state, pr_comment("@coven-cody remember we ship Fridays")) - .await - .expect("remember should acknowledge"); - match task.kind { - TaskKind::CommandReply { body, .. } => { - assert!(body.contains("#6")); - assert!(body.contains("Nothing was stored")); + for command in ["remember we ship Fridays", "forget we ship Fridays"] { + let state = app_state(); + let task = event_to_task(&state, pr_comment(&format!("@coven-cody {command}"))) + .await + .expect("memory command should acknowledge"); + assert_eq!(task.commander.as_deref(), Some("octocat")); + match task.kind { + TaskKind::CommandReply { body, .. } => { + assert!(body.contains("#6")); + assert!(body.contains("Nothing was stored")); + } + other => panic!("expected CommandReply, got {other:?}"), } - other => panic!("expected CommandReply, got {other:?}"), } } diff --git a/crates/worker/src/brief.rs b/crates/worker/src/brief.rs index b871674..7eea4ac 100644 --- a/crates/worker/src/brief.rs +++ b/crates/worker/src/brief.rs @@ -117,6 +117,9 @@ pub fn build( // CommandReply is executed adapter-side before briefing (issue #13); // this arm is a safe fallback, not an expected path. TaskKind::CommandReply { .. } => "issue_mention", + TaskKind::CancelReviews { .. } => { + unreachable!("CancelReviews is handled adapter-side before briefing") + } }; let task_brief = match &task.kind { @@ -149,6 +152,9 @@ pub fn build( issue_number: *issue_number, comment_body: body.clone(), }, + TaskKind::CancelReviews { .. } => { + unreachable!("CancelReviews is handled adapter-side before briefing") + } TaskKind::ReviewPullRequest { pr_number, pr_title, diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index 23d6468..87ebf01 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -53,14 +53,16 @@ pub async fn run( async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Result<()> { // A newer PR event may have superseded this queued review; skip silently // before spending any GitHub calls on it (issue #10). - if let TaskKind::ReviewPullRequest { pr_number, .. } = &task.kind { - let repo = format!("{}/{}", task.repo_owner, task.repo_name); - if !task_store - .is_current_pr_review(&repo, *pr_number, &task.id) - .await - { - info!(task_id = %task.id, "PR review superseded by a newer event — skipping"); - return Ok(()); + if task.commander.is_none() { + if let TaskKind::ReviewPullRequest { pr_number, .. } = &task.kind { + let repo = format!("{}/{}", task.repo_owner, task.repo_name); + if !task_store + .is_current_pr_review(&repo, *pr_number, &task.id) + .await + { + info!(task_id = %task.id, "PR review superseded by a newer event — skipping"); + return Ok(()); + } } } @@ -83,13 +85,9 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res /// Pre-flight outcome: ready to work, or declined at the permission gate. enum Prepared { Ready { - orchestration: String, targets: ResolvedTargets, check_id: u64, }, - Declined { - orchestration: String, - }, } /// Task execution past minter construction; tests inject `Minter::Fixed`. @@ -110,52 +108,144 @@ 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 stop — no coven-code session, no Check Run, no task-store entry. - 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( + let adapter_only = matches!( + &task.kind, + TaskKind::CommandReply { .. } | TaskKind::CancelReviews { .. } + ); + + let orchestration = match minter.mint(TokenRole::Orchestration).await { + Ok(orchestration) => orchestration, + Err(e) => { + if !adapter_only { + task_store + .register_failed(&task, &familiar.display_name) + .await; + } + return Err(e); + } + }; + + // Maintainer permission gate (issue #13): command-initiated work and gated + // command replies need write access before the adapter performs side effects. + if let Some(commander) = &task.commander { + let permission = match repo::get_collaborator_permission_with_base_url( api_base_url, &orchestration, &task.repo_owner, &task.repo_name, - *issue_number, - &marker, - body, + commander, ) - .await?; - return Ok(()); + .await + { + Ok(permission) => permission, + Err(e) => { + if !adapter_only { + task_store + .register_failed(&task, &familiar.display_name) + .await; + } + return Err(e); + } + }; + if !matches!(permission.as_str(), "admin" | "maintain" | "write") { + // Below-write commander: decline on the status surface, do no work. + info!(task_id = %task.id, "declining command from a commander without write access"); + if let Some(number) = surface_number(&task.kind) { + let marker = + status_comment::marker(&familiar.id, &task.repo_owner, &task.repo_name, number); + let body = format!( + "Status: declined\n\nMaintainer commands need write access to {}/{}.", + task.repo_owner, task.repo_name + ); + if let Err(e) = status_comment::upsert( + api_base_url, + &orchestration, + &task.repo_owner, + &task.repo_name, + number, + &marker, + &body, + ) + .await + { + warn!(task_id = %task.id, "failed to post decline: {e:#}"); + } + } + return Ok(()); + } + } else if matches!(&task.kind, TaskKind::CancelReviews { .. }) { + anyhow::bail!("CancelReviews task missing commander"); } - info!(task_id = %task.id, familiar = %familiar.id, "starting task"); + if task.commander.is_some() { + if let TaskKind::ReviewPullRequest { pr_number, .. } = &task.kind { + let repo = format!("{}/{}", task.repo_owner, task.repo_name); + task_store + .register_pr_review(&repo, *pr_number, &task.id) + .await; + } + } - // Pre-flight: installation token, ref resolution, and Check Run creation. - // These run *before* the Check Run exists, so a failure here can't orphan a - // check — but it would otherwise make the task vanish silently. Record it as - // failed so it stays visible in Cave, then propagate. - let prepared = async { - // Adapter-held orchestration authority: resolve refs, drive the Check - // Run, post progress comments. The agent never sees this token. - let orchestration = minter.mint(TokenRole::Orchestration).await?; - - // 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( + // Adapter-only command outcomes stop here — no coven-code session, no Check + // Run, no task-store entry. Permission-gated variants have already passed. + match &task.kind { + TaskKind::CommandReply { issue_number, body } => { + 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, - commander, + *issue_number, + &marker, + body, ) .await?; - if !matches!(permission.as_str(), "admin" | "maintain" | "write") { - return Ok(Prepared::Declined { orchestration }); - } + return Ok(()); + } + TaskKind::CancelReviews { pr_number } => { + let repo = format!("{}/{}", task.repo_owner, task.repo_name); + task_store + .register_pr_review( + &repo, + *pr_number, + &format!("cancelled:{}", uuid::Uuid::new_v4()), + ) + .await; + let marker = + status_comment::marker(&familiar.id, &task.repo_owner, &task.repo_name, *pr_number); + 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]") + ); + status_comment::upsert( + api_base_url, + &orchestration, + &task.repo_owner, + &task.repo_name, + *pr_number, + &marker, + &body, + ) + .await?; + return Ok(()); } + _ => {} + } + info!(task_id = %task.id, familiar = %familiar.id, "starting task"); + + // Pre-flight: ref resolution and Check Run creation. + // These run *before* the Check Run exists, so a failure here can't orphan a + // check — but it would otherwise make the task vanish silently. Record it as + // failed so it stays visible in Cave, then propagate. + let prepared = async { // Resolve target refs and base branch from live GitHub state. Check Runs // must attach to an immutable commit SHA, and PRs must target the repo's // actual base branch rather than a hardcoded "main". @@ -177,50 +267,12 @@ async fn execute_task_with_minter( Some(details_url.as_str()), ) .await?; - Ok::<_, anyhow::Error>(Prepared::Ready { - orchestration, - targets, - check_id, - }) + Ok::<_, anyhow::Error>(Prepared::Ready { targets, check_id }) } .await; - let (orchestration, targets, check_id) = match prepared { - Ok(Prepared::Ready { - orchestration, - targets, - check_id, - }) => (orchestration, targets, check_id), - Ok(Prepared::Declined { orchestration }) => { - // Below-write commander: decline on the status surface, do no work. - info!(task_id = %task.id, "declining command from a commander without write access"); - if let Some(number) = surface_number(&task.kind) { - let marker = status_comment::marker( - &familiar.id, - &task.repo_owner, - &task.repo_name, - number, - ); - let body = format!( - "Status: declined\n\nMaintainer commands need write access to {}/{}.", - task.repo_owner, task.repo_name - ); - if let Err(e) = status_comment::upsert( - api_base_url, - &orchestration, - &task.repo_owner, - &task.repo_name, - number, - &marker, - &body, - ) - .await - { - warn!(task_id = %task.id, "failed to post decline: {e:#}"); - } - } - return Ok(()); - } + let (targets, check_id) = match prepared { + Ok(Prepared::Ready { targets, check_id }) => (targets, check_id), Err(e) => { error!(task_id = %task.id, "pre-flight failed before check run: {e:#}"); task_store @@ -316,12 +368,8 @@ async fn execute_task_with_minter( .await; // Terminal state on the marker-backed status surface (issue #13). if let Some(number) = surface_number(&task.kind) { - let marker = status_comment::marker( - &familiar.id, - &task.repo_owner, - &task.repo_name, - number, - ); + let marker = + status_comment::marker(&familiar.id, &task.repo_owner, &task.repo_name, number); let body = final_status_body(config, &task.id, &published.result, published.opened_pr); if let Err(e) = status_comment::upsert( @@ -357,12 +405,8 @@ async fn execute_task_with_minter( error!(task_id = %task.id, "session failed: {e:#}"); task_store.mark_failed(&task.id).await; if let Some(number) = surface_number(&task.kind) { - let marker = status_comment::marker( - &familiar.id, - &task.repo_owner, - &task.repo_name, - number, - ); + let marker = + status_comment::marker(&familiar.id, &task.repo_owner, &task.repo_name, number); let body = redact::redact( &format!("Status: failed\n\nTask failed: {e}"), &[&orchestration], @@ -718,12 +762,8 @@ async fn open_draft_pr( // the user's view. warn!(task_id = %task.id, "failed to open PR: {e:#}"); if let Some(number) = surface_number(&task.kind) { - let marker = status_comment::marker( - &familiar.id, - &task.repo_owner, - &task.repo_name, - number, - ); + let marker = + status_comment::marker(&familiar.id, &task.repo_owner, &task.repo_name, number); let msg = redact::redact( &format!( "Status: failed\n\nI pushed `{branch}` but could not open the PR automatically: {e}. Open the branch manually or check the App's pull-request permission." @@ -1019,19 +1059,15 @@ fn final_status_body( "Status: needs input\n\n{}\n\nReply on this thread to continue. Session: {session}", result.summary ), - SessionStatus::Failure => format!( - "Status: failed\n\n{}\n\nSession: {session}", - result.summary - ), + SessionStatus::Failure => { + format!("Status: failed\n\n{}\n\nSession: {session}", result.summary) + } SessionStatus::Success | SessionStatus::Partial => match opened_pr { Some(pr_number) => format!( "Status: done\n\n{}\n\nPR #{pr_number} opened. Session: {session}", result.summary ), - None => format!( - "Status: done\n\n{}\n\nSession: {session}", - result.summary - ), + None => format!("Status: done\n\n{}\n\nSession: {session}", result.summary), }, } } @@ -1063,6 +1099,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 for PR #{pr_number}") + } } } @@ -1102,7 +1141,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, @@ -1128,7 +1168,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), } } @@ -1859,8 +1900,7 @@ cat > "$5" <