diff --git a/README.md b/README.md index 89e46f9..8379f35 100644 --- a/README.md +++ b/README.md @@ -90,7 +90,9 @@ For deeper system, sequence, state, security-boundary, and hosted deployment dia --- -## Triggers (V1) +## Triggers + +Implemented lanes: | Trigger | Action | |---|---| @@ -98,6 +100,16 @@ For deeper system, sequence, state, security-boundary, and hosted deployment dia | `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 | +| 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 | + +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 | --- @@ -111,6 +123,8 @@ For deeper system, sequence, state, security-boundary, and hosted deployment dia | 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. | +| 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. | | Check Run creation and completion | Partial | Creates and updates Check Runs against the resolved target head SHA; stale-ref revalidation before publish is still planned. | | Headless execution contract | Locked (v1) | Brief, result envelope, exit codes, and git-auth channel are pinned in [`docs/headless-contract.md`](docs/headless-contract.md) with JSON Schemas, golden fixtures, and a conformance test. | diff --git a/config/example.toml b/config/example.toml index 3b91084..cebdb8d 100644 --- a/config/example.toml +++ b/config/example.toml @@ -37,3 +37,14 @@ trigger_labels = ["coven:fix", "coven:review"] # Labels that trigger this famil # model = "openai/gpt-5.5" # skills = ["requesting-code-review"] # trigger_labels = ["coven:review"] + +# ── Automatic review policy ───────────────────────────────────────────────── +# Hosted PR review lanes (issue #10). Push/commit review is parsed today and +# ships with headless contract v3. +# [review] +# familiar = "cody" # Familiar that runs auto-triggered reviews +# pull_request = true # Review on opened/synchronize/reopened/ready_for_review +# include_drafts = false # Draft PRs are skipped unless explicitly labeled +# audit_instruction = "Focus on correctness and security." +# [review.repos."OpenCoven/coven-code"] +# pull_request = false # Per-repo override diff --git a/crates/config/src/lib.rs b/crates/config/src/lib.rs index 2d8cf62..8e72816 100644 --- a/crates/config/src/lib.rs +++ b/crates/config/src/lib.rs @@ -10,6 +10,67 @@ pub struct Config { pub github: GitHubAppConfig, pub worker: WorkerConfig, pub familiars: Vec, + /// Automatic review trigger policy. Absent section = all lanes off. + #[serde(default)] + pub review: ReviewConfig, +} + +/// Automatic review trigger policy (issue #10). Lanes default to off. +#[derive(Debug, Clone, Default, Deserialize, Serialize)] +pub struct ReviewConfig { + /// Familiar id that runs auto-triggered reviews. + pub familiar: Option, + /// Review PRs on opened / synchronize / reopened / ready_for_review. + #[serde(default)] + pub pull_request: bool, + /// Also auto-review draft PRs. The adapter's own PRs are drafts, so this + /// defaults to off; an explicit review label still works on drafts. + #[serde(default)] + pub include_drafts: bool, + /// Adapter-authored instruction forwarded as the brief's audit_instruction. + pub audit_instruction: Option, + /// Per-repo overrides keyed "owner/name". + #[serde(default)] + pub repos: std::collections::HashMap, +} + +/// Per-repo override of the global [`ReviewConfig`]; unset fields inherit. +#[derive(Debug, Clone, Default, Deserialize, Serialize)] +pub struct RepoReviewOverride { + pub pull_request: Option, + pub include_drafts: Option, + pub familiar: Option, + pub audit_instruction: Option, +} + +impl ReviewConfig { + fn overrides(&self, repo: &str) -> Option<&RepoReviewOverride> { + self.repos.get(repo) + } + + pub fn pull_request_enabled(&self, repo: &str) -> bool { + self.overrides(repo) + .and_then(|o| o.pull_request) + .unwrap_or(self.pull_request) + } + + pub fn drafts_included(&self, repo: &str) -> bool { + self.overrides(repo) + .and_then(|o| o.include_drafts) + .unwrap_or(self.include_drafts) + } + + pub fn reviewer(&self, repo: &str) -> Option<&str> { + self.overrides(repo) + .and_then(|o| o.familiar.as_deref()) + .or(self.familiar.as_deref()) + } + + pub fn audit_instruction_for(&self, repo: &str) -> Option { + self.overrides(repo) + .and_then(|o| o.audit_instruction.clone()) + .or_else(|| self.audit_instruction.clone()) + } } #[derive(Debug, Clone, Deserialize, Serialize)] @@ -203,6 +264,32 @@ impl Config { } } + // ── Review policy ─────────────────────────────────────────────── + let known_ids: std::collections::HashSet<&str> = + self.familiars.iter().map(|f| f.id.as_str()).collect(); + let mut check_reviewer = |scope: &str, reviewer: Option<&str>| match reviewer { + Some(id) if known_ids.contains(id) => {} + Some(id) => out.push(Diagnostic::error( + "review.familiar", + format!("{scope} resolves to '{id}', which matches no configured familiar id."), + )), + None => out.push(Diagnostic::error( + "review.familiar", + format!("{scope} is enabled but no reviewer familiar is set."), + )), + }; + if self.review.pull_request { + check_reviewer("the pull_request review lane", self.review.familiar.as_deref()); + } + for (repo, o) in &self.review.repos { + if o.pull_request == Some(true) { + check_reviewer( + &format!("the pull_request review override for '{repo}'"), + o.familiar.as_deref().or(self.review.familiar.as_deref()), + ); + } + } + out } } @@ -280,6 +367,9 @@ fn next_step_for(field: &str, _message: &str) -> &'static str { "familiars[].trigger_labels" => { "Add labels such as coven:fix if this familiar should run from labels, or rely on assignment/mentions only." } + "review.familiar" => { + "Set review.familiar (or the repo override's familiar) to the id of a configured [[familiars]] block." + } _ => "Update this config field, then rerun coven-github doctor.", } } @@ -377,6 +467,7 @@ mod tests { github, worker, familiars, + review: ReviewConfig::default(), } } @@ -413,6 +504,83 @@ mod tests { .collect() } + #[test] + fn review_policy_resolves_repo_overrides() { + let policy = ReviewConfig { + familiar: Some("cody".to_string()), + pull_request: true, + include_drafts: false, + audit_instruction: Some("global".to_string()), + repos: std::collections::HashMap::from([( + "OpenCoven/quiet".to_string(), + RepoReviewOverride { + pull_request: Some(false), + include_drafts: Some(true), + familiar: Some("nova".to_string()), + audit_instruction: None, + }, + )]), + }; + + assert!(policy.pull_request_enabled("OpenCoven/loud")); + assert!(!policy.pull_request_enabled("OpenCoven/quiet")); + assert!(!policy.drafts_included("OpenCoven/loud")); + assert!(policy.drafts_included("OpenCoven/quiet")); + assert_eq!(policy.reviewer("OpenCoven/loud"), Some("cody")); + assert_eq!(policy.reviewer("OpenCoven/quiet"), Some("nova")); + assert_eq!( + policy.audit_instruction_for("OpenCoven/quiet").as_deref(), + Some("global") + ); + } + + #[test] + fn review_policy_defaults_to_disabled() { + let policy = ReviewConfig::default(); + assert!(!policy.pull_request_enabled("OpenCoven/any")); + assert!(policy.reviewer("OpenCoven/any").is_none()); + assert!(!policy.drafts_included("OpenCoven/any")); + } + + #[test] + fn doctor_flags_review_enabled_without_known_familiar() { + let dir = tmpdir(); + let pem = write_pem(&dir); + let bin = write_bin(&dir); + let mut cfg = config_with( + GitHubAppConfig { + app_id: 123, + private_key_path: pem, + webhook_secret: "a-long-random-webhook-secret".into(), + api_base_url: None, + }, + WorkerConfig { + concurrency: 4, + coven_code_bin: bin, + workspace_root: dir.clone(), + timeout_secs: 600, + max_retries: 2, + }, + vec![good_familiar()], + ); + cfg.review.pull_request = true; + cfg.review.familiar = Some("ghost".to_string()); + + let diags = cfg.check(); + assert!( + errors(&diags).contains(&"review.familiar"), + "diags: {diags:?}" + ); + + // A known familiar id resolves cleanly. + cfg.review.familiar = Some("cody".to_string()); + assert!(errors(&cfg.check()).is_empty()); + + // The lane enabled with no reviewer at all is also an error. + cfg.review.familiar = None; + assert!(errors(&cfg.check()).contains(&"review.familiar")); + } + #[test] fn clean_config_has_no_errors() { let dir = tmpdir(); diff --git a/crates/github/src/lib.rs b/crates/github/src/lib.rs index bf711a0..8dabc57 100644 --- a/crates/github/src/lib.rs +++ b/crates/github/src/lib.rs @@ -70,6 +70,8 @@ pub enum GitHubEvent { IssueComment(IssueCommentEvent), PullRequestReview(PrReviewEvent), PullRequestReviewComment(PrReviewCommentEvent), + PullRequestChanged(PrChangedEvent), + Push(PushEvent), /// `ping` delivery GitHub sends when a webhook is first configured. Ping, Unsupported { @@ -77,6 +79,46 @@ pub enum GitHubEvent { }, } +/// Pull-request lifecycle change relevant to review triggers +/// (`pull_request` → opened / synchronize / reopened / ready_for_review / +/// labeled). Carries the refs at event time so review tasks pin an immutable +/// target (issue #10). +#[derive(Debug, Clone, Deserialize, Serialize)] +pub struct PrChangedEvent { + pub installation_id: u64, + pub repo_owner: String, + pub repo_name: String, + pub pr_number: u64, + pub pr_title: String, + /// The webhook action that fired. + pub action: String, + /// Set for `labeled` actions. + pub label_name: Option, + pub head_ref: String, + pub head_sha: String, + pub base_ref: String, + pub author_login: String, + pub draft: bool, +} + +/// Branch push. Parsed and typed today; the review execution lane ships with +/// headless contract v3 — v2 task kinds cannot express a PR-less review +/// (issue #10). +#[derive(Debug, Clone, Deserialize, Serialize)] +pub struct PushEvent { + pub installation_id: u64, + pub repo_owner: String, + pub repo_name: String, + /// `None` for refs outside `refs/heads/` (e.g. tag pushes). + pub branch: Option, + pub before_sha: String, + pub after_sha: String, + pub deleted: bool, + pub forced: bool, + pub pusher_login: String, + pub commit_count: usize, +} + #[derive(Debug, Clone, Deserialize, Serialize)] pub struct IssueAssignedEvent { pub installation_id: u64, @@ -168,6 +210,18 @@ 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. + 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, …). + reason: String, + }, } /// Structured result envelope written by coven-code --headless. diff --git a/crates/github/src/repo.rs b/crates/github/src/repo.rs index 529e5da..927948d 100644 --- a/crates/github/src/repo.rs +++ b/crates/github/src/repo.rs @@ -145,6 +145,41 @@ pub async fn get_pull_request_refs_with_base_url( }) } +/// Lists the changed-file paths of a pull request for hosted-review context +/// (issue #10). Fetches the first 100 files only; larger PRs surface the gap +/// through the runtime's review `limitations` evidence. +pub async fn get_pull_request_files_with_base_url( + api_base_url: &str, + installation_token: &str, + owner: &str, + name: &str, + pr_number: u64, +) -> Result> { + let client = client()?; + let response = send_json( + &client, + api_base_url, + installation_token, + get_pull_request_files_request(owner, name, pr_number), + ) + .await?; + let body: Vec = response.json().await?; + Ok(body.into_iter().map(|f| f.filename).collect()) +} + +#[derive(Debug, serde::Deserialize)] +struct PullRequestFile { + filename: String, +} + +fn get_pull_request_files_request(owner: &str, name: &str, pr_number: u64) -> GitHubRequest { + GitHubRequest { + method: "GET", + path: format!("/repos/{owner}/{name}/pulls/{pr_number}/files?per_page=100"), + body: serde_json::Value::Null, + } +} + fn get_repo_request(owner: &str, name: &str) -> GitHubRequest { GitHubRequest { method: "GET", @@ -195,6 +230,24 @@ mod tests { assert_eq!(request.path, "/repos/octo/repo/pulls/7"); } + #[test] + fn get_pull_request_files_request_targets_files_endpoint() { + let request = get_pull_request_files_request("octo", "repo", 7); + assert_eq!(request.method, "GET"); + assert_eq!(request.path, "/repos/octo/repo/pulls/7/files?per_page=100"); + } + + #[test] + fn pull_request_file_extracts_filename() { + let files: Vec = serde_json::from_value(json!([ + { "filename": "src/lib.rs", "status": "modified", "additions": 3 }, + { "filename": "docs/security.md", "status": "added" } + ])) + .unwrap(); + let names: Vec<_> = files.into_iter().map(|f| f.filename).collect(); + assert_eq!(names, vec!["src/lib.rs", "docs/security.md"]); + } + #[test] fn repo_metadata_deserializes_default_branch() { let meta: RepoMetadata = diff --git a/crates/github/src/tasks.rs b/crates/github/src/tasks.rs index b35bf6c..368dd06 100644 --- a/crates/github/src/tasks.rs +++ b/crates/github/src/tasks.rs @@ -35,9 +35,31 @@ pub struct TaskListItem { #[derive(Debug, Clone, Default)] pub struct TaskStore { inner: Arc>>, + /// Latest auto-review task per "owner/repo#pr". Newer PR events supersede + /// queued reviews for the same PR (issue #10): the webhook registers the + /// newest task id before enqueueing, and the worker consults this at + /// dequeue and silently skips stale entries. + review_heads: Arc>>, } impl TaskStore { + pub async fn register_pr_review(&self, repo: &str, pr_number: u64, task_id: &str) { + self.review_heads + .write() + .await + .insert(format!("{repo}#{pr_number}"), task_id.to_string()); + } + + /// True when `task_id` is still the newest registered review for the PR. + /// Unregistered tasks are current by definition (e.g. after a restart). + pub async fn is_current_pr_review(&self, repo: &str, pr_number: u64, task_id: &str) -> bool { + self.review_heads + .read() + .await + .get(&format!("{repo}#{pr_number}")) + .is_none_or(|current| current == task_id) + } + pub async fn mark_running( &self, task: &Task, @@ -119,6 +141,11 @@ fn task_list_item(task: &Task, familiar_name: &str) -> TaskListItem { TaskKind::AddressReviewComment { pr_number, .. } => { (*pr_number, format!("Address review on PR #{pr_number}")) } + TaskKind::ReviewPullRequest { + pr_number, + pr_title, + .. + } => (*pr_number, format!("Review PR #{pr_number}: {pr_title}")), }; TaskListItem { @@ -207,6 +234,43 @@ mod tests { ); } + #[tokio::test] + async fn newer_pr_review_registration_supersedes_older_tasks() { + let store = TaskStore::default(); + + // Unregistered tasks are current (e.g. adapter restarted mid-queue). + assert!( + store + .is_current_pr_review("OpenCoven/coven-code", 88, "task-a") + .await + ); + + store + .register_pr_review("OpenCoven/coven-code", 88, "task-a") + .await; + store + .register_pr_review("OpenCoven/coven-code", 88, "task-b") + .await; + + assert!( + !store + .is_current_pr_review("OpenCoven/coven-code", 88, "task-a") + .await, + "older queued review must be superseded" + ); + assert!( + store + .is_current_pr_review("OpenCoven/coven-code", 88, "task-b") + .await + ); + // A different PR in the same repo is unaffected. + assert!( + store + .is_current_pr_review("OpenCoven/coven-code", 89, "task-a") + .await + ); + } + #[tokio::test] async fn register_failed_inserts_a_failed_task_when_never_running() { // A pre-flight failure (token / ref resolution / Check Run creation) diff --git a/crates/webhook/src/events.rs b/crates/webhook/src/events.rs index 26cd8e9..25c32ef 100644 --- a/crates/webhook/src/events.rs +++ b/crates/webhook/src/events.rs @@ -1,8 +1,8 @@ //! Webhook event parsing: GitHub payload → typed events. use coven_github_api::{ - GitHubEvent, IssueAssignedEvent, IssueCommentEvent, IssueLabeledEvent, PrReviewCommentEvent, - PrReviewEvent, + GitHubEvent, IssueAssignedEvent, IssueCommentEvent, IssueLabeledEvent, PrChangedEvent, + PrReviewCommentEvent, PrReviewEvent, PushEvent, }; use serde::Deserialize; @@ -18,6 +18,19 @@ pub struct WebhookPayload { pub label: Option