diff --git a/README.md b/README.md index 29098d5..e5775fe 100644 --- a/README.md +++ b/README.md @@ -156,7 +156,7 @@ duplicate 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. | -| 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. | +| Check Run creation and completion | Implemented | Check Runs attach to the resolved target head SHA; PR reviews re-fetch the head pre-publish and complete as neutral/Stale instead of publishing findings against a moved ref. | | 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. | | `coven-code --headless` execution | Partial | Worker spawns headless sessions with a tokenless session brief and enforces task timeouts; result quality depends on the runtime. | | Pull request creation | Partial | Opens draft PRs from session results against the repository's resolved default/base branch. | diff --git a/crates/github/src/tasks.rs b/crates/github/src/tasks.rs index 9ce8391..b6e8333 100644 --- a/crates/github/src/tasks.rs +++ b/crates/github/src/tasks.rs @@ -12,6 +12,9 @@ pub enum TaskListStatus { Review, Done, Failed, + /// The target moved while the task ran (e.g. a PR head advanced during a + /// review); the output was withheld as stale (issue #8). + Superseded, } #[derive(Debug, Clone, Serialize, Deserialize)] @@ -106,6 +109,16 @@ impl TaskStore { } } + /// Marks a task whose target moved mid-run; its output was withheld as + /// stale rather than published against the wrong ref (issue #8). + pub async fn mark_superseded(&self, task_id: &str) { + let mut items = self.inner.write().await; + if let Some(item) = items.get_mut(task_id) { + item.status = TaskListStatus::Superseded; + item.updated_at = now_rfc3339(); + } + } + /// Records a task as failed, inserting it if it was never marked running. /// /// Used for pre-flight failures (token, ref resolution, Check Run creation) diff --git a/crates/webhook/src/routes.rs b/crates/webhook/src/routes.rs index 0c34576..19ee25d 100644 --- a/crates/webhook/src/routes.rs +++ b/crates/webhook/src/routes.rs @@ -440,6 +440,7 @@ fn status_label(status: &TaskListStatus) -> &'static str { TaskListStatus::Review => "awaiting review", TaskListStatus::Done => "done", TaskListStatus::Failed => "failed", + TaskListStatus::Superseded => "superseded", } } diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index 1d24e4b..23d6468 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -257,6 +257,58 @@ async fn execute_task_with_minter( // The Check Run ALWAYS reaches a terminal conclusion; both arms complete it. match outcome { + // Head moved mid-review (issue #8): the findings describe a commit the + // PR no longer points at. Mark superseded everywhere — never publish + // stale review output as if it covered the current head. + Ok(published) if published.stale.is_some() => { + let stale = published.stale.as_ref().expect("guarded by arm"); + task_store.mark_superseded(&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 = format!( + "Status: superseded\n\nThe PR head moved from `{}` to `{}` while the \ + review ran, so these findings no longer describe the current diff. \ + The newer push is reviewed by its own event, or re-run with a \ + `retry` command.", + stale.reviewed_sha, stale.current_sha + ); + 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 superseded status: {e:#}"); + } + } + if let Err(e) = check_run::complete_with_base_url( + api_base_url, + &orchestration, + &task.repo_owner, + &task.repo_name, + check_id, + check_run::CheckConclusion::Neutral, + "Stale", + &format!( + "Reviewed {}, but the PR head is now {} — findings withheld as stale.", + stale.reviewed_sha, stale.current_sha + ), + ) + .await + { + error!(task_id = %task.id, "failed to finalize stale check run: {e:#}"); + } + } Ok(published) => { let disp = disposition(&published.result); task_store @@ -355,6 +407,17 @@ async fn execute_task_with_minter( struct Published { result: SessionResult, opened_pr: Option, + /// Set when a PR review's head moved while the session ran (issue #8): + /// the findings were computed against `reviewed_sha`, but the PR is now + /// at `current_sha`. Publication must mark the task superseded instead of + /// presenting stale findings as current. + stale: Option, +} + +/// Evidence of a mid-session head move on a reviewed PR. +struct StaleRefs { + reviewed_sha: String, + current_sha: String, } /// Mints repo-scoped installation tokens for one task. Tests substitute @@ -529,6 +592,38 @@ async fn run_and_publish( // PR body, Check Run output). redact::sanitize_result(&mut result, &[orchestration, &agent_git]); + // Stale-ref gate (issue #8): review findings are only valid for the head + // SHA that was actually reviewed. Re-fetch the PR before publishing; if + // the head moved mid-session, surface the task as superseded rather than + // presenting stale findings as current. The newer push's own event (or a + // maintainer `retry`) re-reviews the fresh head. + if let TaskKind::ReviewPullRequest { pr_number, .. } = &task.kind { + let refs = repo::get_pull_request_refs_with_base_url( + api_base_url, + orchestration, + &task.repo_owner, + &task.repo_name, + *pr_number, + ) + .await?; + if refs.head_sha != targets.head_sha { + info!( + task_id = %task.id, + reviewed = %targets.head_sha, + current = %refs.head_sha, + "PR head moved during review — publishing as superseded" + ); + return Ok(Published { + result, + opened_pr: None, + stale: Some(StaleRefs { + reviewed_sha: targets.head_sha.clone(), + current_sha: refs.head_sha, + }), + }); + } + } + // Publish according to the terminal disposition of the result. let disp = disposition(&result); let mut opened_pr = None; @@ -583,7 +678,11 @@ async fn run_and_publish( // Terminal state (done / needs input / failed) lands on the status surface // from execute_task's outcome handling. - Ok(Published { result, opened_pr }) + Ok(Published { + result, + opened_pr, + stale: None, + }) } /// Opens the draft PR and posts the PR-opened comment with post-validation @@ -1958,6 +2057,204 @@ mod supersession_tests { } } +#[cfg(test)] +mod stale_ref_tests { + use super::*; + use coven_github_api::installation::TokenRole; + use coven_github_api::tasks::TaskListStatus; + use coven_github_config::{FamiliarConfig, GitHubAppConfig, ServerConfig, WorkerConfig}; + use std::collections::HashMap; + use std::fs; + use std::os::unix::fs::PermissionsExt; + use std::path::PathBuf; + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + const ORCHESTRATION: &str = "ghs_orchestration0000000000000000000000"; + const AGENT_GIT: &str = "******"; + + fn fixed_minter() -> Minter { + Minter::Fixed(HashMap::from([ + (TokenRole::Orchestration, ORCHESTRATION.to_string()), + (TokenRole::AgentGit, AGENT_GIT.to_string()), + ])) + } + + fn pr_refs_body(head_sha: &str) -> serde_json::Value { + serde_json::json!({ + "head": { "ref": "feat/change", "sha": head_sha }, + "base": { "ref": "main", "sha": "base0000" } + }) + } + + /// A hosted review whose PR head advances mid-session must complete the + /// Check Run as neutral/Stale, surface `Status: superseded` on the status + /// comment, and mark the task superseded — never publishing the findings + /// as if they covered the current head (issue #8). + #[tokio::test] + async fn review_of_moved_head_is_published_as_superseded() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo")) + .respond_with( + ResponseTemplate::new(200) + .set_body_json(serde_json::json!({"default_branch": "main"})), + ) + .mount(&server) + .await; + // First fetch (target resolution) sees the reviewed head; the + // pre-publish re-fetch sees that the head has moved on. + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/pulls/88")) + .respond_with(ResponseTemplate::new(200).set_body_json(pr_refs_body("sha-reviewed"))) + .up_to_n_times(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/pulls/88")) + .respond_with(ResponseTemplate::new(200).set_body_json(pr_refs_body("sha-moved"))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/pulls/88/files")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([ + { "filename": "src/lib.rs" } + ]))) + .mount(&server) + .await; + Mock::given(method("POST")) + .and(path("/repos/OpenCoven/demo/check-runs")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"id": 7}))) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path("/repos/OpenCoven/demo/check-runs/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({}))) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/repos/OpenCoven/demo/issues/88/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/88/comments")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({"id": 1}))) + .mount(&server) + .await; + + // Contract-conformant review result with a finding that must NOT be + // presented as current once the head has moved. + let script = r#"#!/usr/bin/env bash +cat > "$5" < = requests + .iter() + .filter(|r| { + r.method.as_str() == "PATCH" + && r.url.path() == "/repos/OpenCoven/demo/check-runs/7" + }) + .map(|r| String::from_utf8_lossy(&r.body).to_string()) + .collect(); + let terminal = check_patches + .last() + .expect("the check run must reach a terminal state"); + assert!(terminal.contains("\"neutral\""), "conclusion: {terminal}"); + assert!(terminal.contains("Stale"), "title: {terminal}"); + assert!(terminal.contains("sha-reviewed") && terminal.contains("sha-moved")); + assert!( + !check_patches.iter().any(|b| b.contains("\"success\"")), + "stale findings must never publish as a successful review" + ); + + // The status surface says superseded, and the finding text was withheld. + let comment_posts: Vec = requests + .iter() + .filter(|r| { + r.method.as_str() == "POST" + && r.url.path() == "/repos/OpenCoven/demo/issues/88/comments" + }) + .map(|r| String::from_utf8_lossy(&r.body).to_string()) + .collect(); + assert!( + comment_posts.iter().any(|b| b.contains("Status: superseded")), + "status surface must say superseded: {comment_posts:?}" + ); + assert!( + !comment_posts.iter().any(|b| b.contains("Off-by-one")), + "stale findings must not land on the status surface" + ); + + let _ = fs::remove_dir_all(root); + } +} + #[cfg(test)] mod command_and_marker_tests { use super::*;