diff --git a/README.md b/README.md index 8379f35..68cda9f 100644 --- a/README.md +++ b/README.md @@ -98,8 +98,7 @@ Implemented lanes: |---|---| | Issue assigned to bot user (`@cody`) | Agent picks up issue, opens PR | | `coven:` label applied to issue | Same as above | -| `@cody` mention in issue comment | Agent responds / iterates | -| PR review comment `@cody fix:` | Agent addresses review feedback | +| Maintainer command in a comment (`@cody `) | See the command table below | | PR opened / synchronize / reopened / ready_for_review | Automatic hosted review when the `[review]` policy enables the lane (drafts skipped by default; newer pushes supersede queued reviews of the same PR) | | Review label applied to a PR | Explicit per-PR review opt-in — works even with the automatic lane off, including drafts | @@ -108,9 +107,32 @@ Planned lanes: | Trigger | Status | |---|---| | Push / commit-range review | `push` events are parsed and typed with fixtures today; execution needs a PR-less task kind, which ships with headless contract v3 | -| Mention command protocol (re-run, deepen, fix) | Issue #13 | | Advisory / blocking publication gates | Issue #11 | +## Maintainer commands + +A mention only acts when it is the **first token of the comment**, followed by a +command verb — `@cody review`, `@cody fix: the lint is failing`. Casual +mentions mid-sentence trigger nothing, and an unknown verb in command position +gets a clarification reply instead of launching work. Every command except +`status` requires **write access** to the repository; the familiar's own +comments never re-trigger it. + +| Command | On an issue | On a PR | +|---|---|---| +| `review` | Clarification (needs a PR) | Hosted review of the PR | +| `fix [text]` | Fix the issue (opens a PR) | Address the feedback in the comment | +| `deepen` | Clarification | Re-review with a wider lens (supporting files, tests) | +| `retry` | Re-run the fix lane | Re-run the review | +| `cancel` | Clarification (PR reviews only) | Cancel queued reviews for the PR (in-flight work finishes) | +| `remember` / `forget` | Acknowledged; persistence lands with the memory governance contract (#6) | Same | +| `status` | Current task state for this thread | Same | + +Each familiar keeps **one marker-backed status comment per issue/PR**, edited +in place through the task lifecycle (working → done / needs input / failed), +with links to the Check Run, PR, and Cave session — repeated runs never stack +duplicate comments. + --- ## Status @@ -122,7 +144,8 @@ Planned lanes: | Webhook HMAC validation | Implemented | Rejects unsigned or invalid GitHub webhook payloads. | | Issue assignment trigger | Implemented | Routes matching bot assignees to configured familiars. | | Label trigger | Implemented | Routes configured `trigger_labels` such as `coven:fix`. | -| Issue / PR mention trigger | Implemented | Ignores familiar bot self-comments to avoid loops. | +| Maintainer command protocol | Implemented | Typed `@familiar ` grammar; casual mentions ignored; write-access gate; self-comments never re-trigger. | +| Marker-backed status comments | Implemented | One edited-in-place status surface per issue/PR; no duplicate bot comments. | | PR lifecycle review trigger | Implemented | Policy-gated auto-review on opened/synchronize/reopened/ready_for_review plus label opt-in; familiar-authored PRs are never auto-reviewed. | | Push / commit review trigger | Partial | Events parsed and typed with fixtures; execution lane needs headless contract v3. | | GitHub App installation tokens | Implemented | Mints installation access tokens from the App private key. | diff --git a/crates/github/src/lib.rs b/crates/github/src/lib.rs index 8dabc57..c1da060 100644 --- a/crates/github/src/lib.rs +++ b/crates/github/src/lib.rs @@ -147,6 +147,10 @@ pub struct IssueCommentEvent { pub repo_owner: String, pub repo_name: String, pub issue_number: u64, + /// Issue (or PR) title — needed when a `fix` command turns the comment + /// into a FixIssue task (issue #13). + pub issue_title: String, + pub issue_body: String, pub comment_body: String, pub commenter_login: String, /// `issue_comment` fires for pull-request conversation comments as well as @@ -166,6 +170,7 @@ pub struct PrReviewEvent { pub repo_owner: String, pub repo_name: String, pub pr_number: u64, + pub pr_title: String, pub review_body: String, /// Review verdict: `approved`, `changes_requested`, or `commented`. pub review_state: String, @@ -178,6 +183,7 @@ pub struct PrReviewCommentEvent { pub repo_owner: String, pub repo_name: String, pub pr_number: u64, + pub pr_title: String, pub comment_body: String, pub commenter_login: String, } @@ -191,6 +197,11 @@ pub struct Task { pub repo_name: String, pub kind: TaskKind, pub familiar_id: String, + /// Login of the maintainer whose command initiated this task (issue #13). + /// The worker checks their repo permission pre-flight and declines below + /// `write`. `None` for tasks from non-command triggers. + #[serde(default)] + pub commander: Option, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -210,18 +221,23 @@ pub enum TaskKind { issue_number: u64, comment_body: String, }, - /// Adapter-initiated hosted review of a pull request (issue #10). Carries - /// the refs captured at event time; supersession — not ref pinning — keeps - /// reviews current when the head moves. + /// Adapter-initiated hosted review of a pull request (issue #10). Target + /// refs are resolved live at execution; supersession — not ref pinning — + /// keeps reviews current when the head moves. ReviewPullRequest { pr_number: u64, pr_title: String, - head_ref: String, - head_sha: String, - base_ref: String, - /// The webhook action that triggered the review (opened, synchronize, …). + /// What triggered the review: a webhook action (opened, synchronize, …) + /// or a maintainer command (`command:review`, `command:deepen`, …). reason: String, }, + /// 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, + }, } /// Structured result envelope written by coven-code --headless. diff --git a/crates/github/src/pr.rs b/crates/github/src/pr.rs index 0253074..75a2464 100644 --- a/crates/github/src/pr.rs +++ b/crates/github/src/pr.rs @@ -188,3 +188,111 @@ mod tests { assert_eq!(request.body, json!({ "body": "On it" })); } } + +/// One issue/PR conversation comment, trimmed to what marker lookup needs. +#[derive(Debug, Deserialize)] +pub struct IssueComment { + pub id: u64, + pub body: String, + pub user: CommentUser, +} + +#[derive(Debug, Deserialize)] +pub struct CommentUser { + pub login: String, +} + +/// Lists the first 100 conversation comments on an issue or PR (oldest first). +/// Marker-backed status comments are posted early in a thread, so a single +/// page suffices; threads beyond 100 comments fall back to posting fresh +/// (issue #13). +pub async fn list_comments_with_base_url( + api_base_url: &str, + installation_token: &str, + repo_owner: &str, + repo_name: &str, + issue_number: u64, +) -> Result> { + let client = client()?; + let response = send_json( + &client, + api_base_url, + installation_token, + list_comments_request(repo_owner, repo_name, issue_number), + ) + .await?; + Ok(response.json().await?) +} + +/// Edits an existing conversation comment in place (issue #13). +pub async fn update_comment_with_base_url( + api_base_url: &str, + installation_token: &str, + repo_owner: &str, + repo_name: &str, + comment_id: u64, + body: &str, +) -> Result<()> { + let client = client()?; + send_json( + &client, + api_base_url, + installation_token, + update_comment_request(repo_owner, repo_name, comment_id, body), + ) + .await?; + Ok(()) +} + +fn list_comments_request(repo_owner: &str, repo_name: &str, issue_number: u64) -> GitHubRequest { + GitHubRequest { + method: "GET", + path: format!("/repos/{repo_owner}/{repo_name}/issues/{issue_number}/comments?per_page=100"), + body: serde_json::Value::Null, + } +} + +fn update_comment_request( + repo_owner: &str, + repo_name: &str, + comment_id: u64, + body: &str, +) -> GitHubRequest { + GitHubRequest { + method: "PATCH", + path: format!("/repos/{repo_owner}/{repo_name}/issues/comments/{comment_id}"), + body: serde_json::json!({ "body": body }), + } +} + +#[cfg(test)] +mod comment_tests { + use super::*; + use serde_json::json; + + #[test] + fn list_comments_request_targets_issue_comments_endpoint() { + let request = list_comments_request("octo", "repo", 42); + assert_eq!(request.method, "GET"); + assert_eq!(request.path, "/repos/octo/repo/issues/42/comments?per_page=100"); + } + + #[test] + fn update_comment_request_patches_the_comment_by_id() { + let request = update_comment_request("octo", "repo", 77, "new body"); + assert_eq!(request.method, "PATCH"); + assert_eq!(request.path, "/repos/octo/repo/issues/comments/77"); + assert_eq!(request.body["body"], json!("new body")); + } + + #[test] + fn issue_comment_deserializes_id_body_and_author() { + let comments: Vec = serde_json::from_value(json!([ + { "id": 7, "body": "\nStatus: working", "user": { "login": "coven-cody[bot]" }, "created_at": "2026-07-06T00:00:00Z" } + ])) + .unwrap(); + assert_eq!(comments[0].id, 7); + assert!(comments[0].body.contains("coven:cody")); + assert_eq!(comments[0].user.login, "coven-cody[bot]"); + } +} diff --git a/crates/github/src/repo.rs b/crates/github/src/repo.rs index 927948d..4aefd01 100644 --- a/crates/github/src/repo.rs +++ b/crates/github/src/repo.rs @@ -276,3 +276,66 @@ mod tests { assert_eq!(body.base.sha, "basesha"); } } + +/// Fetches the repository permission level GitHub reports for a user: +/// `admin`, `write`, `read`, or `none` (`maintain`/`triage` map onto these in +/// the legacy `permission` field). Used to gate maintainer commands (#13); +/// only requires the always-granted metadata scope. +pub async fn get_collaborator_permission_with_base_url( + api_base_url: &str, + installation_token: &str, + owner: &str, + name: &str, + username: &str, +) -> Result { + let client = client()?; + let response = send_json( + &client, + api_base_url, + installation_token, + collaborator_permission_request(owner, name, username), + ) + .await?; + let body: CollaboratorPermission = response.json().await?; + Ok(body.permission) +} + +#[derive(Debug, serde::Deserialize)] +struct CollaboratorPermission { + permission: String, +} + +fn collaborator_permission_request(owner: &str, name: &str, username: &str) -> GitHubRequest { + GitHubRequest { + method: "GET", + path: format!("/repos/{owner}/{name}/collaborators/{username}/permission"), + body: serde_json::Value::Null, + } +} + +#[cfg(test)] +mod permission_tests { + use super::*; + use serde_json::json; + + #[test] + fn collaborator_permission_request_targets_permission_endpoint() { + let request = collaborator_permission_request("octo", "repo", "hexadecimal-cat"); + assert_eq!(request.method, "GET"); + assert_eq!( + request.path, + "/repos/octo/repo/collaborators/hexadecimal-cat/permission" + ); + } + + #[test] + fn collaborator_permission_extracts_the_legacy_permission_field() { + let body: CollaboratorPermission = serde_json::from_value(json!({ + "permission": "write", + "role_name": "maintain", + "user": { "login": "hexadecimal-cat" } + })) + .unwrap(); + assert_eq!(body.permission, "write"); + } +} diff --git a/crates/github/src/tasks.rs b/crates/github/src/tasks.rs index 368dd06..9ce8391 100644 --- a/crates/github/src/tasks.rs +++ b/crates/github/src/tasks.rs @@ -146,6 +146,9 @@ fn task_list_item(task: &Task, familiar_name: &str) -> TaskListItem { pr_title, .. } => (*pr_number, format!("Review PR #{pr_number}: {pr_title}")), + TaskKind::CommandReply { issue_number, .. } => { + (*issue_number, format!("Reply on #{issue_number}")) + } }; TaskListItem { @@ -185,6 +188,7 @@ mod tests { issue_title: "Fix auth".to_string(), issue_body: "Body".to_string(), }, + commander: None, } } diff --git a/crates/webhook/src/commands.rs b/crates/webhook/src/commands.rs new file mode 100644 index 0000000..99bd025 --- /dev/null +++ b/crates/webhook/src/commands.rs @@ -0,0 +1,227 @@ +//! Maintainer command protocol (issue #13). +//! +//! A mention only carries authority when it is the comment's first token +//! (command position) followed by a known verb. Mentions elsewhere in the text +//! are casual and trigger nothing; an unknown verb in command position earns a +//! clarification reply rather than work. + +/// A typed maintainer intent parsed from a comment. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum Command { + /// Run a hosted review of the pull request. + Review, + /// Fix the issue, or address feedback on the PR. `args` is the free text + /// after the verb (e.g. `@cody fix: the lint is still failing`). + Fix { args: String }, + /// Re-review the PR with greater depth. + Deepen, + /// Re-run the default action for this surface. + Retry, + /// Cancel queued work for this surface. + Cancel, + /// Memory governance intents — parsed and acknowledged; persistence lands + /// with the hosted memory contract (issue #6). + Remember { note: String }, + Forget { note: String }, + /// Report current task state for this surface. + Status, + /// Verb in command position that matches nothing known. + Unknown { verb: String }, +} + +/// How a comment relates to a familiar. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum MentionKind { + /// Command-position mention with a verb. + Command(Command), + /// The familiar is mentioned, but not as a command — ignored. + Casual, + /// No mention of this familiar at all. + None, +} + +/// Parses a comment body against a familiar's bot username +/// (e.g. `coven-cody[bot]`, mentioned as `@coven-cody`). +pub fn parse_mention(body: &str, bot_username: &str) -> MentionKind { + let handle = bot_username.trim_end_matches("[bot]"); + if handle.is_empty() { + return MentionKind::None; + } + let needle = format!("@{handle}"); + + if let Some(rest) = strip_command_mention(body.trim_start(), &needle) { + let mut words = rest.trim_start().splitn(2, char::is_whitespace); + let verb_raw = words.next().unwrap_or(""); + let args = words.next().unwrap_or("").trim().to_string(); + // Tolerate the documented `fix:` colon form on any verb. + let verb = verb_raw.trim_end_matches(':').to_ascii_lowercase(); + let command = match verb.as_str() { + "review" => Command::Review, + "fix" => Command::Fix { args }, + "deepen" => Command::Deepen, + "retry" => Command::Retry, + "cancel" => Command::Cancel, + "remember" => Command::Remember { note: args }, + "forget" => Command::Forget { note: args }, + "status" => Command::Status, + // A bare mention with no verb carries no intent. + "" => return MentionKind::Casual, + other => Command::Unknown { + verb: other.to_string(), + }, + }; + return MentionKind::Command(command); + } + + if mentions(body, bot_username) { + MentionKind::Casual + } else { + MentionKind::None + } +} + +/// The commands a clarification reply should list. +pub const COMMAND_LIST: &str = + "`review`, `fix`, `deepen`, `retry`, `cancel`, `remember`, `forget`, `status`"; + +/// If `text` starts with the mention as a whole token, returns the remainder. +fn strip_command_mention<'a>(text: &'a str, needle: &str) -> Option<&'a str> { + let rest = text.strip_prefix(needle)?; + match rest.chars().next() { + // `@codyx`, `@cody-2`, `@cody/team` are not mentions of `@cody`. + Some(c) if c.is_alphanumeric() || c == '-' || c == '_' || c == '/' => None, + _ => Some(rest), + } +} + +/// Returns true if `body` mentions the familiar's `@handle` as a whole token. +/// +/// `bot_username` is the GitHub App bot login (e.g. `coven-cody[bot]`); the +/// `[bot]` suffix is dropped since mentions are written `@coven-cody`. Matching +/// is boundary-aware: `@cody` inside `@codyx`, `@cody-2`, or `email@cody` does +/// not count, and `@coven-cody/team` (a team mention) is not a bot mention. +pub fn mentions(body: &str, bot_username: &str) -> bool { + let handle = bot_username.trim_end_matches("[bot]"); + if handle.is_empty() { + return false; + } + let needle = format!("@{handle}"); + let mut offset = 0; + while let Some(pos) = body[offset..].find(&needle) { + let start = offset + pos; + let end = start + needle.len(); + let before = body[..start].chars().next_back(); + let after = body[end..].chars().next(); + // The character before `@` must be a separator (or start of string), + // and the character after the handle must not continue an identifier. + let boundary_before = before.is_none_or(|c| !c.is_alphanumeric() && c != '@'); + let boundary_after = + after.is_none_or(|c| !(c.is_alphanumeric() || c == '-' || c == '_' || c == '/')); + if boundary_before && boundary_after { + return true; + } + offset = start + 1; + } + false +} + +#[cfg(test)] +mod tests { + use super::*; + + const BOT: &str = "coven-cody[bot]"; + + #[test] + fn every_verb_parses_in_command_position() { + let cases: Vec<(&str, Command)> = vec![ + ("@coven-cody review", Command::Review), + ( + "@coven-cody fix the flaky auth test", + Command::Fix { + args: "the flaky auth test".to_string(), + }, + ), + ( + "@coven-cody fix: the lint is still failing", + Command::Fix { + args: "the lint is still failing".to_string(), + }, + ), + ("@coven-cody deepen", Command::Deepen), + ("@coven-cody retry", Command::Retry), + ("@coven-cody cancel", Command::Cancel), + ( + "@coven-cody remember we ship Fridays", + Command::Remember { + note: "we ship Fridays".to_string(), + }, + ), + ( + "@coven-cody forget the Friday rule", + Command::Forget { + note: "the Friday rule".to_string(), + }, + ), + ("@coven-cody status", Command::Status), + (" @coven-cody REVIEW ", Command::Review), + ]; + for (body, expected) in cases { + assert_eq!( + parse_mention(body, BOT), + MentionKind::Command(expected), + "body: {body:?}" + ); + } + } + + #[test] + fn unknown_verb_in_command_position_is_a_typed_unknown() { + assert_eq!( + parse_mention("@coven-cody frobnicate the flux", BOT), + MentionKind::Command(Command::Unknown { + verb: "frobnicate".to_string() + }) + ); + } + + #[test] + fn casual_mentions_carry_no_command() { + // Mid-sentence mention. + assert_eq!( + parse_mention("thanks @coven-cody, great work on this", BOT), + MentionKind::Casual + ); + // Conversational lead that is not a verb reads as Unknown (it sits in + // command position), NOT Casual — the reply will clarify. + assert!(matches!( + parse_mention("@coven-cody can you take a look?", BOT), + MentionKind::Command(Command::Unknown { .. }) + )); + // Bare mention with no verb at all. + assert_eq!(parse_mention("@coven-cody", BOT), MentionKind::Casual); + } + + #[test] + fn non_mentions_are_none() { + assert_eq!(parse_mention("no bots here", BOT), MentionKind::None); + assert_eq!(parse_mention("ping @coven-codyx review", BOT), MentionKind::None); + assert_eq!( + parse_mention("cc @coven-cody/maintainers review", BOT), + MentionKind::None + ); + assert_eq!( + parse_mention("mail user@coven-cody.example", BOT), + MentionKind::None + ); + } + + #[test] + fn mention_matching_is_boundary_aware() { + assert!(mentions("hey @coven-cody can you help", BOT)); + assert!(!mentions("ping @coven-codyx instead", BOT)); + assert!(!mentions("ping @coven-cody-2 instead", BOT)); + assert!(!mentions("cc @coven-cody/maintainers", BOT)); + assert!(!mentions("mail user@coven-cody.example", BOT)); + assert!(mentions("over to you @coven-cody", BOT)); + } +} diff --git a/crates/webhook/src/events.rs b/crates/webhook/src/events.rs index 25c32ef..87e6b31 100644 --- a/crates/webhook/src/events.rs +++ b/crates/webhook/src/events.rs @@ -160,6 +160,8 @@ pub fn parse_event(event_type: &str, payload: &WebhookPayload) -> GitHubEvent { repo_owner: repo.owner.login.clone(), repo_name: repo.name.clone(), issue_number: issue.number, + issue_title: issue.title.clone(), + issue_body: issue.body.clone().unwrap_or_default(), comment_body: comment.body.clone(), commenter_login: comment.user.login.clone(), on_pull_request: issue.pull_request.is_some(), @@ -178,6 +180,7 @@ pub fn parse_event(event_type: &str, payload: &WebhookPayload) -> GitHubEvent { repo_owner: repo.owner.login.clone(), repo_name: repo.name.clone(), pr_number: pr.number, + pr_title: pr.title.clone().unwrap_or_default(), review_body: review.body.clone().unwrap_or_default(), review_state: review.state.clone().unwrap_or_default(), reviewer_login: review.user.login.clone(), @@ -196,6 +199,7 @@ pub fn parse_event(event_type: &str, payload: &WebhookPayload) -> GitHubEvent { repo_owner: repo.owner.login.clone(), repo_name: repo.name.clone(), pr_number: pr.number, + pr_title: pr.title.clone().unwrap_or_default(), comment_body: comment.body.clone(), commenter_login: comment.user.login.clone(), }); diff --git a/crates/webhook/src/lib.rs b/crates/webhook/src/lib.rs index 1af896c..d51491a 100644 --- a/crates/webhook/src/lib.rs +++ b/crates/webhook/src/lib.rs @@ -4,6 +4,7 @@ use anyhow::Result; use hmac::{Hmac, Mac}; use sha2::Sha256; +pub mod commands; pub mod events; pub mod routes; diff --git a/crates/webhook/src/routes.rs b/crates/webhook/src/routes.rs index 66cb226..0c34576 100644 --- a/crates/webhook/src/routes.rs +++ b/crates/webhook/src/routes.rs @@ -14,9 +14,11 @@ use coven_github_api::{tasks::TaskStore, GitHubEvent, Task, TaskKind}; use coven_github_config::Config; use crate::{ + commands::{parse_mention, Command, MentionKind, COMMAND_LIST}, events::{parse_event, WebhookPayload}, verify_signature, }; +use coven_github_api::tasks::TaskListStatus; /// Shared application state passed to route handlers. #[derive(Clone)] @@ -101,7 +103,7 @@ pub async fn handle_webhook( } // 4. Route event → task. - if let Some(task) = event_to_task(&state, event) { + if let Some(task) = event_to_task(&state, event).await { 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). @@ -123,7 +125,7 @@ pub async fn handle_webhook( } /// Maps a parsed event to a worker task, or returns None if not actionable. -fn event_to_task(state: &AppState, event: GitHubEvent) -> Option { +async fn event_to_task(state: &AppState, event: GitHubEvent) -> Option { match event { GitHubEvent::IssueAssigned(e) => { // Find a familiar whose bot_username matches the assignee. @@ -139,6 +141,7 @@ fn event_to_task(state: &AppState, event: GitHubEvent) -> Option { repo_owner: e.repo_owner, repo_name: e.repo_name, familiar_id: familiar.id.clone(), + commander: None, kind: TaskKind::FixIssue { issue_number: e.issue_number, issue_title: e.issue_title, @@ -160,6 +163,7 @@ fn event_to_task(state: &AppState, event: GitHubEvent) -> Option { repo_owner: e.repo_owner, repo_name: e.repo_name, familiar_id: familiar.id.clone(), + commander: None, kind: TaskKind::FixIssue { issue_number: e.issue_number, issue_title: e.issue_title, @@ -168,77 +172,54 @@ fn event_to_task(state: &AppState, event: GitHubEvent) -> Option { }) } + // Comment surfaces speak the maintainer command protocol (issue #13): + // only command-position mentions act; casual mentions are ignored. GitHubEvent::IssueComment(e) => { - // Find a familiar mentioned in the comment body (skip the bot's own - // comments to avoid self-trigger loops). - let familiar = state.config.familiars.iter().find(|f| { - e.commenter_login != f.bot_username && mentions(&e.comment_body, &f.bot_username) - })?; - - // GitHub delivers PR conversation comments through `issue_comment`. - // Route those through PR iteration so the familiar gets PR context - // rather than issue context (the numbers coincide in GitHub). - let kind = if e.on_pull_request { - TaskKind::AddressReviewComment { - pr_number: e.issue_number, - comment_body: e.comment_body, - diff_hunk: None, - } - } else { - TaskKind::RespondToMention { - issue_number: e.issue_number, - comment_body: e.comment_body, - } - }; - - Some(Task { - id: uuid::Uuid::new_v4().to_string(), + let (familiar, command) = parse_command(state, &e.comment_body, &e.commenter_login)?; + let surface = CommandSurface { installation_id: e.installation_id, - repo_owner: e.repo_owner, - repo_name: e.repo_name, - familiar_id: familiar.id.clone(), - kind, - }) + repo_owner: &e.repo_owner, + repo_name: &e.repo_name, + number: e.issue_number, + title: &e.issue_title, + issue_body: &e.issue_body, + comment_body: &e.comment_body, + on_pull_request: e.on_pull_request, + commander: &e.commenter_login, + }; + command_task(state, familiar, command, surface).await } GitHubEvent::PullRequestReview(e) => { - // A submitted review carries a summary body and a verdict. Trigger - // on an explicit mention in the body, same as inline comments. - let familiar = state.config.familiars.iter().find(|f| { - e.reviewer_login != f.bot_username && mentions(&e.review_body, &f.bot_username) - })?; - - Some(Task { - id: uuid::Uuid::new_v4().to_string(), + let (familiar, command) = parse_command(state, &e.review_body, &e.reviewer_login)?; + let surface = CommandSurface { installation_id: e.installation_id, - repo_owner: e.repo_owner, - repo_name: e.repo_name, - familiar_id: familiar.id.clone(), - kind: TaskKind::AddressReviewComment { - pr_number: e.pr_number, - comment_body: e.review_body, - diff_hunk: None, - }, - }) + repo_owner: &e.repo_owner, + repo_name: &e.repo_name, + number: e.pr_number, + title: &e.pr_title, + issue_body: "", + comment_body: &e.review_body, + on_pull_request: true, + commander: &e.reviewer_login, + }; + command_task(state, familiar, command, surface).await } GitHubEvent::PullRequestReviewComment(e) => { - let familiar = state.config.familiars.iter().find(|f| { - e.commenter_login != f.bot_username && mentions(&e.comment_body, &f.bot_username) - })?; - - Some(Task { - id: uuid::Uuid::new_v4().to_string(), + let (familiar, command) = parse_command(state, &e.comment_body, &e.commenter_login)?; + let surface = CommandSurface { installation_id: e.installation_id, - repo_owner: e.repo_owner, - repo_name: e.repo_name, - familiar_id: familiar.id.clone(), - kind: TaskKind::AddressReviewComment { - pr_number: e.pr_number, - comment_body: e.comment_body, - diff_hunk: None, - }, - }) + repo_owner: &e.repo_owner, + repo_name: &e.repo_name, + number: e.pr_number, + title: &e.pr_title, + issue_body: "", + comment_body: &e.comment_body, + on_pull_request: true, + commander: &e.commenter_login, + }; + command_task(state, familiar, command, surface).await } GitHubEvent::PullRequestChanged(e) => { @@ -281,12 +262,10 @@ fn event_to_task(state: &AppState, event: GitHubEvent) -> Option { repo_owner: e.repo_owner, repo_name: e.repo_name, familiar_id: familiar.id.clone(), + commander: None, kind: TaskKind::ReviewPullRequest { pr_number: e.pr_number, pr_title: e.pr_title, - head_ref: e.head_ref, - head_sha: e.head_sha, - base_ref: e.base_ref, reason: e.action, }, }) @@ -302,35 +281,166 @@ fn event_to_task(state: &AppState, event: GitHubEvent) -> Option { } } -/// Returns true if `body` mentions the familiar's `@handle` as a whole token. -/// -/// `bot_username` is the GitHub App bot login (e.g. `coven-cody[bot]`); the -/// `[bot]` suffix is dropped since mentions are written `@coven-cody`. Matching -/// is boundary-aware: `@cody` inside `@codyx`, `@cody-2`, or `email@cody` does -/// not count, and `@coven-cody/team` (a team mention) is not a bot mention. -fn mentions(body: &str, bot_username: &str) -> bool { - let handle = bot_username.trim_end_matches("[bot]"); - if handle.is_empty() { - return false; - } - let needle = format!("@{handle}"); - let mut offset = 0; - while let Some(pos) = body[offset..].find(&needle) { - let start = offset + pos; - let end = start + needle.len(); - let before = body[..start].chars().next_back(); - let after = body[end..].chars().next(); - // The character before `@` must be a separator (or start of string), - // and the character after the handle must not continue an identifier. - let boundary_before = before.is_none_or(|c| !c.is_alphanumeric() && c != '@'); - let boundary_after = - after.is_none_or(|c| !(c.is_alphanumeric() || c == '-' || c == '_' || c == '/')); - if boundary_before && boundary_after { - return true; +/// Finds the first familiar addressed in command position, skipping the bots' +/// own comments (self-trigger loop guard). +fn parse_command<'a>( + state: &'a AppState, + body: &str, + author: &str, +) -> Option<(&'a coven_github_config::FamiliarConfig, Command)> { + state.config.familiars.iter().find_map(|f| { + if author == f.bot_username { + return None; + } + match parse_mention(body, &f.bot_username) { + MentionKind::Command(command) => Some((f, command)), + MentionKind::Casual | MentionKind::None => None, + } + }) +} + +/// The issue/PR conversation a command arrived on. +struct CommandSurface<'a> { + installation_id: u64, + repo_owner: &'a str, + repo_name: &'a str, + number: u64, + title: &'a str, + issue_body: &'a str, + comment_body: &'a str, + on_pull_request: bool, + 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. +async fn command_task( + state: &AppState, + familiar: &coven_github_config::FamiliarConfig, + command: Command, + s: CommandSurface<'_>, +) -> Option { + let repo = format!("{}/{}", s.repo_owner, s.repo_name); + let make = |kind: TaskKind, commander: Option| Task { + id: uuid::Uuid::new_v4().to_string(), + installation_id: s.installation_id, + repo_owner: s.repo_owner.to_string(), + repo_name: s.repo_name.to_string(), + familiar_id: familiar.id.clone(), + commander, + kind, + }; + let commander = Some(s.commander.to_string()); + let reply = |body: String| { + make( + TaskKind::CommandReply { + issue_number: s.number, + body, + }, + None, + ) + }; + + Some(match command { + Command::Review | Command::Deepen | Command::Retry if s.on_pull_request => make( + TaskKind::ReviewPullRequest { + pr_number: s.number, + pr_title: s.title.to_string(), + reason: format!("command:{}", verb(&command)), + }, + commander, + ), + // `retry` on an issue re-runs the fix lane. + Command::Retry => make( + TaskKind::FixIssue { + issue_number: s.number, + issue_title: s.title.to_string(), + issue_body: s.issue_body.to_string(), + }, + commander, + ), + Command::Review | Command::Deepen => reply(format!( + "`{}` needs a pull request; this is an issue. Commands here: {COMMAND_LIST}.", + verb(&command) + )), + Command::Fix { .. } if s.on_pull_request => make( + TaskKind::AddressReviewComment { + pr_number: s.number, + comment_body: s.comment_body.to_string(), + diff_hunk: None, + }, + commander, + ), + Command::Fix { .. } => make( + TaskKind::FixIssue { + issue_number: s.number, + issue_title: s.title.to_string(), + issue_body: s.issue_body.to_string(), + }, + 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 => 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::Status => { + let items = state.task_store.list().await; + let mut lines: Vec = items + .iter() + .filter(|t| t.repo == repo && t.issue_number == s.number) + .map(|t| format!("- {} — {}", t.issue_title, status_label(&t.status))) + .collect(); + let body = if lines.is_empty() { + format!("No tracked tasks for {repo}#{}.", s.number) + } else { + lines.sort(); + format!("Tasks for {repo}#{}:\n{}", s.number, lines.join("\n")) + }; + reply(body) } - offset = start + 1; + Command::Unknown { verb } => reply(format!( + "I don't recognize `{verb}`. Supported commands: {COMMAND_LIST}." + )), + }) +} + +fn verb(command: &Command) -> &'static str { + match command { + Command::Review => "review", + Command::Fix { .. } => "fix", + Command::Deepen => "deepen", + Command::Retry => "retry", + Command::Cancel => "cancel", + Command::Remember { .. } => "remember", + Command::Forget { .. } => "forget", + Command::Status => "status", + Command::Unknown { .. } => "unknown", + } +} + +fn status_label(status: &TaskListStatus) -> &'static str { + match status { + TaskListStatus::Running => "running", + TaskListStatus::Review => "awaiting review", + TaskListStatus::Done => "done", + TaskListStatus::Failed => "failed", } - false } #[cfg(test)] @@ -380,8 +490,8 @@ mod tests { } } - #[test] - fn labeled_issue_routes_to_familiar_trigger_label() { + #[tokio::test] + async fn labeled_issue_routes_to_familiar_trigger_label() { let state = app_state(); let task = event_to_task( &state, @@ -395,6 +505,7 @@ mod tests { label_name: "coven:fix".to_string(), }), ) + .await .expect("matching trigger label should create a task"); assert_eq!(task.installation_id, 123); @@ -415,8 +526,8 @@ mod tests { } } - #[test] - fn labeled_issue_ignores_unknown_labels() { + #[tokio::test] + async fn labeled_issue_ignores_unknown_labels() { let state = app_state(); let task = event_to_task( &state, @@ -429,13 +540,14 @@ mod tests { issue_body: "Token refresh is broken.".to_string(), label_name: "help wanted".to_string(), }), - ); + ) + .await; assert!(task.is_none()); } - #[test] - fn issue_comment_ignores_bot_self_mentions() { + #[tokio::test] + async fn issue_comment_ignores_bot_self_mentions() { let state = app_state(); let task = event_to_task( &state, @@ -444,17 +556,20 @@ mod tests { repo_owner: "OpenCoven".to_string(), repo_name: "coven-code".to_string(), issue_number: 42, + issue_title: "Fix auth".to_string(), + issue_body: "Body".to_string(), comment_body: "@coven-cody thanks, I opened a PR.".to_string(), commenter_login: "coven-cody[bot]".to_string(), on_pull_request: false, }), - ); + ) + .await; assert!(task.is_none()); } - #[test] - fn pr_review_comment_ignores_bot_self_mentions() { + #[tokio::test] + async fn pr_review_comment_ignores_bot_self_mentions() { let state = app_state(); let task = event_to_task( &state, @@ -463,16 +578,18 @@ mod tests { repo_owner: "OpenCoven".to_string(), repo_name: "coven-code".to_string(), pr_number: 7, - comment_body: "@coven-cody please fix.".to_string(), + pr_title: "Harden sigil parser".to_string(), + comment_body: "@coven-cody fix: please".to_string(), commenter_login: "coven-cody[bot]".to_string(), }), - ); + ) + .await; assert!(task.is_none()); } - #[test] - fn issue_comment_on_pr_routes_to_pr_iteration() { + #[tokio::test] + async fn issue_comment_on_pr_routes_to_pr_iteration() { let state = app_state(); let task = event_to_task( &state, @@ -481,28 +598,66 @@ mod tests { repo_owner: "OpenCoven".to_string(), repo_name: "coven-code".to_string(), issue_number: 73, - comment_body: "@coven-cody the lint is still failing".to_string(), + issue_title: "Add spell compiler cache".to_string(), + issue_body: "".to_string(), + comment_body: "@coven-cody fix: the lint is still failing".to_string(), commenter_login: "octocat".to_string(), on_pull_request: true, }), ) + .await .expect("a mention on a PR comment should create a task"); match task.kind { TaskKind::AddressReviewComment { pr_number, + comment_body, diff_hunk, - .. } => { assert_eq!(pr_number, 73); + assert!(comment_body.contains("lint is still failing")); assert!(diff_hunk.is_none()); } other => panic!("expected AddressReviewComment for a PR comment, got {other:?}"), } } - #[test] - fn issue_comment_on_issue_routes_to_respond_to_mention() { + #[tokio::test] + async fn fix_command_on_issue_routes_to_fix_issue_with_commander() { + let state = app_state(); + let task = event_to_task( + &state, + GitHubEvent::IssueComment(IssueCommentEvent { + installation_id: 123, + repo_owner: "OpenCoven".to_string(), + repo_name: "coven-code".to_string(), + issue_number: 42, + issue_title: "Fix auth".to_string(), + issue_body: "Token refresh is broken.".to_string(), + comment_body: "@coven-cody fix".to_string(), + commenter_login: "octocat".to_string(), + on_pull_request: false, + }), + ) + .await + .expect("a fix command on an issue should create a task"); + + assert_eq!(task.commander.as_deref(), Some("octocat")); + match task.kind { + TaskKind::FixIssue { + issue_number, + issue_title, + .. + } => { + assert_eq!(issue_number, 42); + assert_eq!(issue_title, "Fix auth"); + } + other => panic!("expected FixIssue for a fix command, got {other:?}"), + } + } + + #[tokio::test] + async fn casual_mention_creates_no_task() { let state = app_state(); let task = event_to_task( &state, @@ -511,21 +666,51 @@ mod tests { repo_owner: "OpenCoven".to_string(), repo_name: "coven-code".to_string(), issue_number: 42, + issue_title: "Fix auth".to_string(), + issue_body: "Body".to_string(), + comment_body: "thanks @coven-cody, great work".to_string(), + commenter_login: "octocat".to_string(), + on_pull_request: false, + }), + ) + .await; + + assert!(task.is_none(), "casual mentions must not trigger work"); + } + + #[tokio::test] + async fn unknown_command_earns_a_clarification_reply() { + let state = app_state(); + let task = event_to_task( + &state, + GitHubEvent::IssueComment(IssueCommentEvent { + installation_id: 123, + repo_owner: "OpenCoven".to_string(), + repo_name: "coven-code".to_string(), + issue_number: 42, + issue_title: "Fix auth".to_string(), + issue_body: "Body".to_string(), comment_body: "@coven-cody can you take a look?".to_string(), commenter_login: "octocat".to_string(), on_pull_request: false, }), ) - .expect("a mention on an issue comment should create a task"); + .await + .expect("unknown command should earn a clarification"); + assert!(task.commander.is_none(), "replies need no permission gate"); match task.kind { - TaskKind::RespondToMention { issue_number, .. } => assert_eq!(issue_number, 42), - other => panic!("expected RespondToMention for an issue comment, got {other:?}"), + TaskKind::CommandReply { issue_number, body } => { + assert_eq!(issue_number, 42); + assert!(body.contains("`can`")); + assert!(body.contains("`review`"), "reply should list commands: {body}"); + } + other => panic!("expected CommandReply, got {other:?}"), } } - #[test] - fn submitted_review_mention_routes_to_pr_iteration() { + #[tokio::test] + async fn submitted_review_mention_routes_to_pr_iteration() { let state = app_state(); let task = event_to_task( &state, @@ -534,11 +719,13 @@ mod tests { repo_owner: "OpenCoven".to_string(), repo_name: "coven-code".to_string(), pr_number: 73, - review_body: "@coven-cody please add test coverage before merge.".to_string(), + pr_title: "Harden sigil parser".to_string(), + review_body: "@coven-cody fix: add test coverage before merge.".to_string(), review_state: "changes_requested".to_string(), reviewer_login: "octocat".to_string(), }), ) + .await .expect("a mention in a review body should create a task"); match task.kind { @@ -547,8 +734,8 @@ mod tests { } } - #[test] - fn submitted_review_without_mention_is_ignored() { + #[tokio::test] + async fn submitted_review_without_mention_is_ignored() { let state = app_state(); let task = event_to_task( &state, @@ -557,35 +744,23 @@ mod tests { repo_owner: "OpenCoven".to_string(), repo_name: "coven-code".to_string(), pr_number: 73, + pr_title: "Harden sigil parser".to_string(), review_body: "LGTM, nice work!".to_string(), review_state: "approved".to_string(), reviewer_login: "octocat".to_string(), }), - ); + ) + .await; assert!(task.is_none()); } - #[test] - fn ping_event_produces_no_task() { + #[tokio::test] + async fn ping_event_produces_no_task() { let state = app_state(); - assert!(event_to_task(&state, GitHubEvent::Ping).is_none()); - } - - #[test] - fn mention_matching_is_boundary_aware() { - // Exact handle (with the [bot] suffix stripped) matches. - assert!(mentions("hey @coven-cody can you help", "coven-cody[bot]")); - // Trailing identifier characters must not be swallowed into the handle. - assert!(!mentions("ping @coven-codyx instead", "coven-cody[bot]")); - assert!(!mentions("ping @coven-cody-2 instead", "coven-cody[bot]")); - // A team mention (`@org/team`) is not a bot mention. - assert!(!mentions("cc @coven-cody/maintainers", "coven-cody[bot]")); - // An email-like local part is not a mention. - assert!(!mentions("mail user@coven-cody.example", "coven-cody[bot]")); - // Mention at end of string still matches. - assert!(mentions("over to you @coven-cody", "coven-cody[bot]")); + assert!(event_to_task(&state, GitHubEvent::Ping).await.is_none()); } + } #[cfg(test)] @@ -622,79 +797,74 @@ mod review_lane_tests { } } - #[test] - fn pr_opened_routes_to_review_when_lane_enabled() { + #[tokio::test] + async fn pr_opened_routes_to_review_when_lane_enabled() { let state = app_state_with_review(review_on()); let task = event_to_task(&state, GitHubEvent::PullRequestChanged(pr_event("opened"))) + .await .expect("enabled lane should create a review task"); assert_eq!(task.familiar_id, "cody"); match task.kind { TaskKind::ReviewPullRequest { - pr_number, - head_sha, - reason, - .. + pr_number, reason, .. } => { assert_eq!(pr_number, 88); - assert_eq!(head_sha, "abc123"); assert_eq!(reason, "opened"); } other => panic!("expected ReviewPullRequest, got {other:?}"), } } - #[test] - fn pr_synchronize_carries_event_time_head() { + #[tokio::test] + async fn pr_synchronize_carries_event_time_head() { let state = app_state_with_review(review_on()); let mut event = pr_event("synchronize"); event.head_sha = "f00dface".to_string(); let task = event_to_task(&state, GitHubEvent::PullRequestChanged(event)) + .await .expect("synchronize should create a review task"); match task.kind { - TaskKind::ReviewPullRequest { - head_sha, reason, .. - } => { - assert_eq!(head_sha, "f00dface"); + TaskKind::ReviewPullRequest { reason, .. } => { assert_eq!(reason, "synchronize"); } other => panic!("expected ReviewPullRequest, got {other:?}"), } } - #[test] - fn pr_opened_is_ignored_when_lane_disabled() { + #[tokio::test] + async fn pr_opened_is_ignored_when_lane_disabled() { let state = app_state(); assert!( - event_to_task(&state, GitHubEvent::PullRequestChanged(pr_event("opened"))).is_none() + event_to_task(&state, GitHubEvent::PullRequestChanged(pr_event("opened"))).await.is_none() ); } - #[test] - fn familiar_authored_prs_are_never_auto_reviewed() { + #[tokio::test] + async fn familiar_authored_prs_are_never_auto_reviewed() { // The adapter's own draft PRs must not re-trigger reviews (loop guard). 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)).is_none()); + assert!(event_to_task(&state, GitHubEvent::PullRequestChanged(event)).await.is_none()); } - #[test] - fn draft_prs_are_skipped_unless_policy_includes_them() { + #[tokio::test] + async fn draft_prs_are_skipped_unless_policy_includes_them() { 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())).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)).is_some()); + assert!(event_to_task(&state, GitHubEvent::PullRequestChanged(event)).await.is_some()); } - #[test] - fn per_repo_override_disables_the_lane() { + #[tokio::test] + async fn per_repo_override_disables_the_lane() { let mut review = review_on(); review.repos.insert( "OpenCoven/coven-code".to_string(), @@ -707,33 +877,34 @@ mod review_lane_tests { ); let state = app_state_with_review(review); assert!( - event_to_task(&state, GitHubEvent::PullRequestChanged(pr_event("opened"))).is_none() + event_to_task(&state, GitHubEvent::PullRequestChanged(pr_event("opened"))).await.is_none() ); } - #[test] - fn review_label_is_an_explicit_opt_in_even_with_lane_off_and_draft() { + #[tokio::test] + async fn review_label_is_an_explicit_opt_in_even_with_lane_off_and_draft() { // No [review] policy at all — the label alone routes, like issue labels. let state = app_state(); let mut event = pr_event("labeled"); event.label_name = Some("coven:review".to_string()); event.draft = true; let task = event_to_task(&state, GitHubEvent::PullRequestChanged(event)) + .await .expect("review label should opt the PR in"); assert_eq!(task.familiar_id, "cody"); assert!(matches!(task.kind, TaskKind::ReviewPullRequest { .. })); } - #[test] - fn unknown_labels_do_not_trigger_review() { + #[tokio::test] + async fn unknown_labels_do_not_trigger_review() { 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)).is_none()); + assert!(event_to_task(&state, GitHubEvent::PullRequestChanged(event)).await.is_none()); } - #[test] - fn push_events_produce_no_task_until_contract_v3() { + #[tokio::test] + async fn push_events_produce_no_task_until_contract_v3() { let state = app_state_with_review(review_on()); let event = GitHubEvent::Push(coven_github_api::PushEvent { installation_id: 123, @@ -747,6 +918,153 @@ mod review_lane_tests { pusher_login: "octocat".to_string(), commit_count: 2, }); - assert!(event_to_task(&state, event).is_none()); + assert!(event_to_task(&state, event).await.is_none()); + } +} + +#[cfg(test)] +mod command_routing_tests { + use super::tests::app_state; + use super::*; + use coven_github_api::PrReviewCommentEvent; + + fn pr_comment(body: &str) -> GitHubEvent { + GitHubEvent::PullRequestReviewComment(PrReviewCommentEvent { + installation_id: 123, + repo_owner: "OpenCoven".to_string(), + repo_name: "coven-code".to_string(), + pr_number: 88, + pr_title: "Add spell compiler cache".to_string(), + comment_body: body.to_string(), + commenter_login: "octocat".to_string(), + }) + } + + #[tokio::test] + async fn review_command_on_pr_creates_commanded_review() { + let state = app_state(); + let task = event_to_task(&state, pr_comment("@coven-cody review")) + .await + .expect("review command should create a task"); + + assert_eq!(task.commander.as_deref(), Some("octocat")); + match task.kind { + TaskKind::ReviewPullRequest { + pr_number, reason, .. + } => { + assert_eq!(pr_number, 88); + assert_eq!(reason, "command:review"); + } + other => panic!("expected ReviewPullRequest, got {other:?}"), + } + } + + #[tokio::test] + async fn deepen_command_carries_its_verb_in_the_reason() { + let state = app_state(); + let task = event_to_task(&state, pr_comment("@coven-cody deepen")) + .await + .expect("deepen command should create a task"); + assert!( + matches!(task.kind, TaskKind::ReviewPullRequest { ref reason, .. } if reason == "command:deepen") + ); + } + + #[tokio::test] + async fn cancel_command_tombstones_queued_reviews_and_acknowledges() { + let state = app_state(); + state + .task_store + .register_pr_review("OpenCoven/coven-code", 88, "task-queued") + .await; + + let task = event_to_task(&state, pr_comment("@coven-cody cancel")) + .await + .expect("cancel should acknowledge"); + + assert!( + !state + .task_store + .is_current_pr_review("OpenCoven/coven-code", 88, "task-queued") + .await, + "queued review must be superseded by the cancel tombstone" + ); + match task.kind { + TaskKind::CommandReply { issue_number, body } => { + assert_eq!(issue_number, 88); + assert!(body.contains("Cancelled queued reviews")); + } + other => panic!("expected CommandReply, 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")); + } + other => panic!("expected CommandReply, got {other:?}"), + } + } + + #[tokio::test] + async fn status_command_reports_tracked_tasks_for_the_surface() { + let state = app_state(); + let tracked = Task { + id: "task-1".to_string(), + installation_id: 123, + repo_owner: "OpenCoven".to_string(), + repo_name: "coven-code".to_string(), + familiar_id: "cody".to_string(), + commander: None, + kind: TaskKind::ReviewPullRequest { + pr_number: 88, + pr_title: "Add spell compiler cache".to_string(), + reason: "opened".to_string(), + }, + }; + state.task_store.mark_running(&tracked, "Cody", None).await; + + let task = event_to_task(&state, pr_comment("@coven-cody status")) + .await + .expect("status should reply"); + match task.kind { + TaskKind::CommandReply { body, .. } => { + assert!(body.contains("Review PR #88"), "status body: {body}"); + assert!(body.contains("running"), "status body: {body}"); + } + other => panic!("expected CommandReply, got {other:?}"), + } + } + + #[tokio::test] + async fn review_command_on_an_issue_is_clarified() { + let state = app_state(); + let task = event_to_task( + &state, + GitHubEvent::IssueComment(coven_github_api::IssueCommentEvent { + installation_id: 123, + repo_owner: "OpenCoven".to_string(), + repo_name: "coven-code".to_string(), + issue_number: 42, + issue_title: "Fix auth".to_string(), + issue_body: "Body".to_string(), + comment_body: "@coven-cody review".to_string(), + commenter_login: "octocat".to_string(), + on_pull_request: false, + }), + ) + .await + .expect("review on an issue should clarify"); + assert!(matches!( + task.kind, + TaskKind::CommandReply { ref body, .. } if body.contains("needs a pull request") + )); } } diff --git a/crates/worker/src/brief.rs b/crates/worker/src/brief.rs index 16d7811..b871674 100644 --- a/crates/worker/src/brief.rs +++ b/crates/worker/src/brief.rs @@ -114,6 +114,9 @@ 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. + TaskKind::CommandReply { .. } => "issue_mention", }; let task_brief = match &task.kind { @@ -142,6 +145,10 @@ pub fn build( issue_number: *issue_number, comment_body: comment_body.clone(), }, + TaskKind::CommandReply { issue_number, body } => TaskBrief::RespondToMention { + issue_number: *issue_number, + comment_body: body.clone(), + }, TaskKind::ReviewPullRequest { pr_number, pr_title, @@ -223,6 +230,7 @@ mod tests { issue_title: "Fix auth".to_string(), issue_body: "Body".to_string(), }, + commander: None, } } @@ -233,12 +241,10 @@ mod tests { repo_owner: "OpenCoven".to_string(), repo_name: "coven-code".to_string(), familiar_id: "cody".to_string(), + commander: None, kind: TaskKind::ReviewPullRequest { pr_number: 88, pr_title: "Add spell compiler cache".to_string(), - head_ref: "feat/spell-cache".to_string(), - head_sha: "abc123".to_string(), - base_ref: "main".to_string(), reason: "synchronize".to_string(), }, } diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index 2505224..1d24e4b 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -15,6 +15,7 @@ use coven_github_config::{Config, FamiliarConfig}; pub mod brief; pub mod redact; +pub mod status_comment; /// Base unit for exponential backoff between retry-safe coven-code attempts. /// Attempt `n` sleeps `RETRY_BACKOFF_BASE * 2^n` (so 2s, 4s, … in production). @@ -50,12 +51,6 @@ pub async fn run( } async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Result<()> { - let familiar = config - .familiars - .iter() - .find(|f| f.id == task.familiar_id) - .ok_or_else(|| anyhow::anyhow!("unknown familiar: {}", task.familiar_id))?; - // 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 { @@ -69,30 +64,98 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res } } - info!(task_id = %task.id, familiar = %familiar.id, "starting task"); let api_base_url = config .github .api_base_url .as_deref() .unwrap_or(DEFAULT_API_BASE_URL); + let private_key = std::fs::read_to_string(&config.github.private_key_path)?; + let minter = Minter::App { + api_base_url: api_base_url.to_string(), + app_id: config.github.app_id, + private_key, + installation_id: task.installation_id, + repo_name: task.repo_name.clone(), + }; + execute_task_with_minter(config, task_store, task, &minter).await +} + +/// 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`. +async fn execute_task_with_minter( + config: &Config, + task_store: TaskStore, + task: Task, + minter: &Minter, +) -> Result<()> { + let familiar = config + .familiars + .iter() + .find(|f| f.id == task.familiar_id) + .ok_or_else(|| anyhow::anyhow!("unknown familiar: {}", task.familiar_id))?; + let api_base_url = config + .github + .api_base_url + .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( + api_base_url, + &orchestration, + &task.repo_owner, + &task.repo_name, + *issue_number, + &marker, + body, + ) + .await?; + return Ok(()); + } + + info!(task_id = %task.id, familiar = %familiar.id, "starting task"); // 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 { - let private_key = std::fs::read_to_string(&config.github.private_key_path)?; - let minter = Minter::App { - api_base_url: api_base_url.to_string(), - app_id: config.github.app_id, - private_key, - installation_id: task.installation_id, - repo_name: task.repo_name.clone(), - }; // 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( + 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 }); + } + } + // 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". @@ -114,12 +177,50 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res Some(details_url.as_str()), ) .await?; - Ok::<_, anyhow::Error>((minter, orchestration, targets, check_id)) + Ok::<_, anyhow::Error>(Prepared::Ready { + orchestration, + targets, + check_id, + }) } .await; - let (minter, orchestration, targets, check_id) = match prepared { - Ok(prepared) => prepared, + 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(()); + } Err(e) => { error!(task_id = %task.id, "pre-flight failed before check run: {e:#}"); task_store @@ -142,7 +243,7 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res config, &task, familiar, - &minter, + minter, &orchestration, api_base_url, &targets, @@ -161,6 +262,30 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res task_store .mark_complete(&task.id, &repo, &published.result, published.opened_pr) .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 body = + final_status_body(config, &task.id, &published.result, published.opened_pr); + 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 upsert final status: {e:#}"); + } + } if let Err(e) = check_run::complete_with_base_url( api_base_url, &orchestration, @@ -179,6 +304,31 @@ async fn execute_task(config: &Config, task_store: TaskStore, task: Task) -> Res Err(e) => { 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 body = redact::redact( + &format!("Status: failed\n\nTask failed: {e}"), + &[&orchestration], + ); + if let Err(ce) = status_comment::upsert( + api_base_url, + &orchestration, + &task.repo_owner, + &task.repo_name, + number, + &marker, + &body, + ) + .await + { + warn!(task_id = %task.id, "failed to upsert failure status: {ce:#}"); + } + } if let Err(ce) = check_run::complete_with_base_url( api_base_url, &orchestration, @@ -273,7 +423,9 @@ async fn run_and_publish( // Hosted reviews carry tokenless changed-file context so the runtime can // prove coverage (issue #10); other task kinds brief without it. let review = match &task.kind { - TaskKind::ReviewPullRequest { pr_number, .. } => { + TaskKind::ReviewPullRequest { + pr_number, reason, .. + } => { let files = repo::get_pull_request_files_with_base_url( api_base_url, orchestration, @@ -282,11 +434,21 @@ async fn run_and_publish( *pr_number, ) .await?; + let mut audit = config + .review + .audit_instruction_for(&format!("{}/{}", task.repo_owner, task.repo_name)); + // A `deepen` command widens the lens beyond the changed set. + if reason == "command:deepen" { + let depth = "Perform a deep review: inspect supporting files beyond the \ + changed set and verify behavior with tests where possible."; + audit = Some(match audit { + Some(existing) => format!("{existing}\n\n{depth}"), + None => depth.to_string(), + }); + } Some(brief::ReviewContext { files, - audit_instruction: config - .review - .audit_instruction_for(&format!("{}/{}", task.repo_owner, task.repo_name)), + audit_instruction: audit, }) } _ => None, @@ -309,21 +471,25 @@ async fn run_and_publish( ); tokio::fs::write(&brief_path, brief_json).await?; - // Best-effort "starting" comment — a flaky comment API call must not abort - // the task or orphan the Check Run. - if let Some(issue_number) = task_issue_number(&task.kind) { + // Best-effort status surface (issue #13): one marker-backed comment per + // target, edited in place — a flaky comment API call must not abort the + // task or orphan the Check Run. + if let Some(number) = surface_number(&task.kind) { + let marker = + status_comment::marker(&familiar.id, &task.repo_owner, &task.repo_name, number); let start_msg = starting_comment(config, familiar, &task.id); - if let Err(e) = pr::post_comment_with_base_url( + if let Err(e) = status_comment::upsert( api_base_url, orchestration, &task.repo_owner, &task.repo_name, - issue_number, + number, + &marker, &start_msg, ) .await { - warn!(task_id = %task.id, "failed to post starting comment: {e:#}"); + warn!(task_id = %task.id, "failed to upsert status comment: {e:#}"); } } @@ -373,8 +539,8 @@ async fn run_and_publish( match minter.mint(TokenRole::Publication).await { Ok(publication) => { opened_pr = open_draft_pr( - config, task, + familiar, api_base_url, &publication, targets, @@ -385,19 +551,26 @@ async fn run_and_publish( } Err(e) => { warn!(task_id = %task.id, "failed to mint publication token: {e:#}"); - if let Some(issue_number) = task_issue_number(&task.kind) { + if let Some(number) = surface_number(&task.kind) { + let marker = status_comment::marker( + &familiar.id, + &task.repo_owner, + &task.repo_name, + number, + ); let msg = redact::redact( &format!( - "I pushed `{branch}` but could not obtain publication credentials to open the PR: {e}" + "Status: failed\n\nI pushed `{branch}` but could not obtain publication credentials to open the PR: {e}" ), &[orchestration], ); - let _ = pr::post_comment_with_base_url( + let _ = status_comment::upsert( api_base_url, orchestration, &task.repo_owner, &task.repo_name, - issue_number, + number, + &marker, &msg, ) .await; @@ -407,24 +580,8 @@ async fn run_and_publish( } } - // Needs-input: surface the familiar's clarifying question on the issue/PR. - if result.status == SessionStatus::NeedsInput { - if let Some(issue_number) = task_issue_number(&task.kind) { - let msg = format!("I need input before I can continue:\n\n{}", result.summary); - if let Err(e) = pr::post_comment_with_base_url( - api_base_url, - orchestration, - &task.repo_owner, - &task.repo_name, - issue_number, - &msg, - ) - .await - { - warn!(task_id = %task.id, "failed to post clarifying comment: {e:#}"); - } - } - } + // Terminal state (done / needs input / failed) lands on the status surface + // from execute_task's outcome handling. Ok(Published { result, opened_pr }) } @@ -433,8 +590,8 @@ async fn run_and_publish( /// publication authority. Best-effort: failures are surfaced on the issue /// rather than failing the task, since the branch is already pushed. async fn open_draft_pr( - config: &Config, task: &Task, + familiar: &FamiliarConfig, api_base_url: &str, publication: &str, targets: &ResolvedTargets, @@ -454,42 +611,33 @@ async fn open_draft_pr( ) .await { - Ok(pr_num) => { - if let Some(issue_number) = task_issue_number(&task.kind) { - let msg = pr_opened_comment(config, &task.id, pr_num); - if let Err(e) = pr::post_comment_with_base_url( - api_base_url, - publication, - &task.repo_owner, - &task.repo_name, - issue_number, - &msg, - ) - .await - { - warn!(task_id = %task.id, "failed to post PR comment: {e:#}"); - } - } - Some(pr_num) - } + // The final status upsert in execute_task announces the opened PR. + Ok(pr_num) => Some(pr_num), Err(e) => { // The branch is already pushed; the PR just didn't open. Surface it // rather than failing the whole task, so the work isn't lost from // the user's view. warn!(task_id = %task.id, "failed to open PR: {e:#}"); - if let Some(issue_number) = task_issue_number(&task.kind) { + if let Some(number) = surface_number(&task.kind) { + let marker = status_comment::marker( + &familiar.id, + &task.repo_owner, + &task.repo_name, + number, + ); let msg = redact::redact( &format!( - "I pushed `{branch}` but could not open the PR automatically: {e}. Open the branch manually or check the App's pull-request permission." + "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." ), &[publication], ); - let _ = pr::post_comment_with_base_url( + let _ = status_comment::upsert( api_base_url, publication, &task.repo_owner, &task.repo_name, - issue_number, + number, + &marker, &msg, ) .await; @@ -759,18 +907,41 @@ fn starting_comment(config: &Config, familiar: &FamiliarConfig, task_id: &str) - ) } -fn pr_opened_comment(config: &Config, task_id: &str, pr_number: u64) -> String { - format!( - "PR #{pr_number} opened.\n\nSession: {}", - cave_session_url(config, task_id) - ) +/// Terminal body for the marker-backed status surface. +fn final_status_body( + config: &Config, + task_id: &str, + result: &SessionResult, + opened_pr: Option, +) -> String { + let session = cave_session_url(config, task_id); + match result.status { + SessionStatus::NeedsInput => format!( + "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::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 + ), + }, + } } fn pr_title(result: &SessionResult, task: &Task) -> String { format!( "{} (#{} via Coven)", result.summary, - task_issue_number(&task.kind).unwrap_or(0) + surface_number(&task.kind).unwrap_or(0) ) } @@ -792,6 +963,7 @@ fn task_title(kind: &TaskKind) -> String { pr_title, .. } => format!("Review PR #{pr_number}: {pr_title}"), + TaskKind::CommandReply { issue_number, .. } => format!("Reply on #{issue_number}"), } } @@ -829,7 +1001,9 @@ async fn resolve_targets(api_base_url: &str, token: &str, task: &Task) -> Result head_sha: refs.head_sha, }) } - TaskKind::FixIssue { .. } | TaskKind::RespondToMention { .. } => { + TaskKind::FixIssue { .. } + | TaskKind::RespondToMention { .. } + | TaskKind::CommandReply { .. } => { let head_sha = repo::get_branch_sha_with_base_url( api_base_url, token, @@ -847,13 +1021,15 @@ async fn resolve_targets(api_base_url: &str, token: &str, task: &Task) -> Result } } -fn task_issue_number(kind: &TaskKind) -> Option { +/// The issue/PR conversation number a task's status surface lives on. PR +/// conversation comments ride the issues API, so PR numbers work directly. +fn surface_number(kind: &TaskKind) -> Option { match kind { - TaskKind::FixIssue { issue_number, .. } => Some(*issue_number), - TaskKind::RespondToMention { issue_number, .. } => Some(*issue_number), - // PR-scoped tasks post no issue-thread comments; review results travel - // via the Check Run (publication gates are issue #11). - TaskKind::AddressReviewComment { .. } | TaskKind::ReviewPullRequest { .. } => None, + TaskKind::FixIssue { issue_number, .. } + | TaskKind::RespondToMention { issue_number, .. } + | TaskKind::CommandReply { issue_number, .. } => Some(*issue_number), + TaskKind::AddressReviewComment { pr_number, .. } + | TaskKind::ReviewPullRequest { pr_number, .. } => Some(*pr_number), } } @@ -1279,13 +1455,31 @@ mod disposition_tests { "starting comment should be calm operator copy, not decorative chrome: {started}" ); - let opened = pr_opened_comment(&config, "task-42", 17); - assert!(opened.contains("PR #17 opened")); - assert!(opened.contains("https://cave.example.test/sessions/task-42")); + let result = SessionResult { + contract_version: HEADLESS_CONTRACT_VERSION.to_string(), + status: SessionStatus::Success, + branch: Some("cody/fix".to_string()), + commits: vec![], + files_changed: vec![], + summary: "Fixed the auth refresh.".to_string(), + pr_body: "body".to_string(), + review: ReviewResult::none(), + exit_reason: None, + }; + let done = final_status_body(&config, "task-42", &result, Some(17)); + assert!(done.starts_with("Status: done")); + assert!(done.contains("PR #17 opened")); + assert!(done.contains("https://cave.example.test/sessions/task-42")); assert!( - !opened.contains('✅') && !opened.contains('→'), - "PR comment should stay concise and actionable: {opened}" + !done.contains('✅') && !done.contains('→'), + "status body should stay concise and actionable: {done}" ); + + let mut needs_input = result.clone(); + needs_input.status = SessionStatus::NeedsInput; + let waiting = final_status_body(&config, "task-42", &needs_input, None); + assert!(waiting.starts_with("Status: needs input")); + assert!(waiting.contains("Reply on this thread")); } } @@ -1534,6 +1728,11 @@ mod publication_tests { #[tokio::test] async fn publication_uses_post_validation_token_and_leaks_nothing() { let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/issues/42/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(&server) + .await; Mock::given(method("POST")) .and(path("/repos/OpenCoven/demo/issues/42/comments")) .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"id": 1}))) @@ -1604,6 +1803,7 @@ exit 0 repo_owner: "OpenCoven".to_string(), repo_name: "demo".to_string(), familiar_id: "cody".to_string(), + commander: None, kind: TaskKind::FixIssue { issue_number: 42, issue_title: "t".to_string(), @@ -1680,13 +1880,11 @@ exit 0 auth_of("/repos/OpenCoven/demo/check-runs/7", "PATCH"), vec![format!("Bearer {ORCHESTRATION}")] ); - // Starting comment (orchestration), then PR-opened comment (publication). + // One marker-backed status comment, posted with orchestration authority; + // the terminal status edit happens in execute_task, not here. assert_eq!( auth_of("/repos/OpenCoven/demo/issues/42/comments", "POST"), - vec![ - format!("Bearer {ORCHESTRATION}"), - format!("Bearer {PUBLICATION}"), - ] + vec![format!("Bearer {ORCHESTRATION}")] ); let _ = fs::remove_dir_all(root); @@ -1737,12 +1935,10 @@ mod supersession_tests { repo_owner: "OpenCoven".to_string(), repo_name: "demo".to_string(), familiar_id: "cody".to_string(), + commander: None, kind: TaskKind::ReviewPullRequest { pr_number: 88, pr_title: "t".to_string(), - head_ref: "feat/x".to_string(), - head_sha: "old".to_string(), - base_ref: "main".to_string(), reason: "synchronize".to_string(), }, }; @@ -1761,3 +1957,215 @@ mod supersession_tests { ); } } + +#[cfg(test)] +mod command_and_marker_tests { + use super::*; + use coven_github_api::installation::TokenRole; + use coven_github_config::{FamiliarConfig, GitHubAppConfig, ServerConfig, WorkerConfig}; + use std::collections::HashMap; + use std::path::PathBuf; + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + const ORCHESTRATION: &str = "ghs_orchestration0000000000000000000000"; + + fn fixed_minter() -> Minter { + Minter::Fixed(HashMap::from([( + TokenRole::Orchestration, + ORCHESTRATION.to_string(), + )])) + } + + fn config(api_base_url: String) -> Config { + Config { + server: ServerConfig { + bind: "127.0.0.1:0".to_string(), + cave_base_url: None, + }, + github: GitHubAppConfig { + app_id: 1, + private_key_path: PathBuf::from("/nonexistent/never-read.pem"), + webhook_secret: "secret".to_string(), + api_base_url: Some(api_base_url), + }, + worker: WorkerConfig { + concurrency: 1, + // Would fail loudly if any of these paths spawned a session. + coven_code_bin: PathBuf::from("/nonexistent/coven-code"), + workspace_root: PathBuf::from("/nonexistent/workspaces"), + timeout_secs: 1, + max_retries: 0, + }, + familiars: vec![FamiliarConfig { + id: "cody".to_string(), + display_name: "Cody".to_string(), + bot_username: "coven-cody[bot]".to_string(), + model: None, + skills: vec![], + trigger_labels: vec![], + }], + review: coven_github_config::ReviewConfig::default(), + } + } + + fn task(kind: TaskKind, commander: Option<&str>) -> Task { + Task { + id: "task-cmd".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, + } + } + + #[tokio::test] + async fn command_reply_upserts_without_spawning_coven_code() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/issues/42/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/repos/OpenCoven/demo/issues/42/comments")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"id": 1}))) + .mount(&server) + .await; + + execute_task_with_minter( + &config(server.uri()), + TaskStore::default(), + task( + TaskKind::CommandReply { + issue_number: 42, + body: "Tasks for OpenCoven/demo#42: none".to_string(), + }, + None, + ), + &fixed_minter(), + ) + .await + .expect("command reply should post cleanly"); + + let requests = server.received_requests().await.expect("requests recorded"); + let posted = requests + .iter() + .find(|r| r.method.as_str() == "POST") + .expect("reply should be posted"); + let body = String::from_utf8_lossy(&posted.body); + assert!( + body.contains(""), + "reply must carry the marker: {body}" + ); + assert!(body.contains("Tasks for OpenCoven/demo#42")); + // No Check Run, no session, no other GitHub calls. + assert_eq!(requests.len(), 2, "GET comments + POST comment only"); + } + + #[tokio::test] + async fn existing_marker_comment_is_edited_in_place() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/issues/42/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([ + { "id": 5, "body": "unrelated first comment", "user": { "login": "octocat" } }, + { "id": 7, "body": "\nStatus: working", "user": { "login": "coven-cody[bot]" } } + ]))) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path("/repos/OpenCoven/demo/issues/comments/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({}))) + .mount(&server) + .await; + + execute_task_with_minter( + &config(server.uri()), + TaskStore::default(), + task( + TaskKind::CommandReply { + issue_number: 42, + body: "Status: done".to_string(), + }, + None, + ), + &fixed_minter(), + ) + .await + .expect("upsert should edit in place"); + + let requests = server.received_requests().await.expect("requests recorded"); + let patch = requests + .iter() + .find(|r| r.method.as_str() == "PATCH") + .expect("existing marker comment must be edited, not duplicated"); + let body = String::from_utf8_lossy(&patch.body); + assert!(body.contains("Status: done")); + assert!( + !requests.iter().any(|r| r.method.as_str() == "POST"), + "no duplicate comment may be created" + ); + } + + #[tokio::test] + async fn below_write_commander_is_declined_before_any_work() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/collaborators/drive-by/permission")) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(serde_json::json!({"permission": "read"})), + ) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/issues/42/comments")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/repos/OpenCoven/demo/issues/42/comments")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"id": 1}))) + .mount(&server) + .await; + + let store = TaskStore::default(); + execute_task_with_minter( + &config(server.uri()), + store.clone(), + task( + TaskKind::FixIssue { + issue_number: 42, + issue_title: "t".to_string(), + issue_body: "b".to_string(), + }, + Some("drive-by"), + ), + &fixed_minter(), + ) + .await + .expect("a declined command is not an error"); + + let requests = server.received_requests().await.expect("requests recorded"); + assert!( + !requests.iter().any(|r| r.url.path().contains("check-runs")), + "no Check Run may be created for a declined command" + ); + let posted = requests + .iter() + .find(|r| r.method.as_str() == "POST") + .expect("decline should land on the status surface"); + let body = String::from_utf8_lossy(&posted.body); + assert!( + body.contains("Status: declined"), + "decline body: {body}" + ); + assert!( + store.list().await.is_empty(), + "declined commands must not surface as tasks" + ); + } +} diff --git a/crates/worker/src/status_comment.rs b/crates/worker/src/status_comment.rs new file mode 100644 index 0000000..2cf86d4 --- /dev/null +++ b/crates/worker/src/status_comment.rs @@ -0,0 +1,69 @@ +//! Marker-backed status comments (issue #13). +//! +//! Each familiar keeps ONE mutable status comment per issue/PR, identified by +//! a hidden HTML marker and edited in place through the task lifecycle — +//! repeated runs update the surface instead of stacking new comments. + +use anyhow::Result; +use coven_github_api::pr; + +/// Hidden marker identifying a familiar's status comment on a target. +pub fn marker(familiar_id: &str, repo_owner: &str, repo_name: &str, number: u64) -> String { + format!("") +} + +/// Creates or edits-in-place the marker-backed status comment. +/// +/// Searches the first 100 conversation comments; status comments are posted at +/// task start so they land early. Threads that exceed the page before the +/// familiar ever commented fall back to posting fresh rather than paging. +pub async fn upsert( + api_base_url: &str, + token: &str, + repo_owner: &str, + repo_name: &str, + number: u64, + marker: &str, + body: &str, +) -> Result<()> { + let full = format!("{marker}\n{body}"); + let comments = + pr::list_comments_with_base_url(api_base_url, token, repo_owner, repo_name, number) + .await?; + if let Some(existing) = comments.iter().find(|c| c.body.contains(marker)) { + pr::update_comment_with_base_url( + api_base_url, + token, + repo_owner, + repo_name, + existing.id, + &full, + ) + .await + } else { + pr::post_comment_with_base_url(api_base_url, token, repo_owner, repo_name, number, &full) + .await + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn marker_is_scoped_to_familiar_and_target() { + assert_eq!( + marker("cody", "OpenCoven", "coven-code", 42), + "" + ); + // Distinct targets and familiars must never collide. + assert_ne!( + marker("cody", "OpenCoven", "coven-code", 42), + marker("cody", "OpenCoven", "coven-code", 43) + ); + assert_ne!( + marker("cody", "OpenCoven", "coven-code", 42), + marker("nova", "OpenCoven", "coven-code", 42) + ); + } +}