diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 805eee1..7b26b54 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,7 +11,7 @@ env: jobs: check: - name: cargo check + clippy + test + name: python + cargo check + clippy + test runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 @@ -19,6 +19,10 @@ jobs: with: components: clippy - uses: Swatinem/rust-cache@v2 + - name: hosted adapter python compile + run: python -B -m py_compile deploy/coven-github/coven_github_adapter.py deploy/coven-github/test_coven_github_adapter.py + - name: hosted adapter python tests + run: python -B -m unittest discover -s deploy/coven-github -p "test_*.py" - name: cargo check run: cargo check --all-targets - name: clippy diff --git a/.gitignore b/.gitignore index a45b66e..9d586a0 100644 --- a/.gitignore +++ b/.gitignore @@ -4,4 +4,7 @@ keys/ *.pem .env .DS_Store -.comux* +deploy/coven-github/.coven-github-private-key.pem +deploy/coven-github/coven-github-policy.json +deploy/coven-github/coven-github-state/ +.comux* \ No newline at end of file diff --git a/crates/github/src/lib.rs b/crates/github/src/lib.rs index 5e05f32..bf711a0 100644 --- a/crates/github/src/lib.rs +++ b/crates/github/src/lib.rs @@ -12,11 +12,8 @@ pub const DEFAULT_API_BASE_URL: &str = "https://api.github.com"; /// Major version of the coven-code headless execution contract this adapter /// speaks. See `docs/headless-contract.md`. Bump only on breaking changes. -pub const HEADLESS_CONTRACT_VERSION: &str = "1"; +pub const HEADLESS_CONTRACT_VERSION: &str = "2"; -fn default_contract_version() -> String { - HEADLESS_CONTRACT_VERSION.to_string() -} const GITHUB_API_VERSION: &str = "2026-03-10"; #[derive(Debug, Clone, PartialEq, Eq)] @@ -175,11 +172,10 @@ pub enum TaskKind { /// Structured result envelope written by coven-code --headless. #[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] pub struct SessionResult { - /// Contract major version. Defaults to the current version when a runtime - /// omits it, but conformant producers MUST emit it. See + /// Contract major version. Conformant producers MUST emit it. See /// `docs/headless-contract.md`. - #[serde(default = "default_contract_version")] pub contract_version: String, pub status: SessionStatus, pub branch: Option, @@ -187,6 +183,7 @@ pub struct SessionResult { pub files_changed: Vec, pub summary: String, pub pr_body: String, + pub review: ReviewResult, pub exit_reason: Option, } @@ -200,11 +197,95 @@ pub enum SessionStatus { } #[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] pub struct CommitInfo { pub sha: String, pub message: String, } +#[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub struct ReviewResult { + pub mode: ReviewMode, + pub evidence_status: ReviewEvidenceStatus, + pub reviewed_files: Vec, + pub supporting_files: Vec, + pub findings: Vec, + pub tests_run: Vec, + pub no_findings_reason: Option, + pub limitations: Vec, +} + +impl ReviewResult { + pub fn none() -> Self { + Self { + mode: ReviewMode::None, + evidence_status: ReviewEvidenceStatus::NotApplicable, + reviewed_files: Vec::new(), + supporting_files: Vec::new(), + findings: Vec::new(), + tests_run: Vec::new(), + no_findings_reason: None, + limitations: Vec::new(), + } + } +} + +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum ReviewMode { + None, + PullRequest, + ReviewComment, +} + +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum ReviewEvidenceStatus { + NotApplicable, + Complete, + Partial, + Missing, +} + +#[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub struct ReviewFinding { + pub severity: ReviewSeverity, + pub file: String, + pub line: Option, + pub title: String, + pub body: String, + pub recommendation: Option, +} + +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum ReviewSeverity { + Info, + Low, + Medium, + High, + Critical, +} + +#[derive(Debug, Clone, Deserialize, Serialize)] +#[serde(deny_unknown_fields)] +pub struct ReviewTestRun { + pub command: String, + pub status: ReviewTestStatus, + pub output_summary: Option, +} + +#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] +#[serde(rename_all = "snake_case")] +pub enum ReviewTestStatus { + Passed, + Failed, + NotRun, + Unknown, +} + #[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)] #[serde(rename_all = "snake_case")] pub enum ExitReason { diff --git a/crates/github/src/tasks.rs b/crates/github/src/tasks.rs index 4b1aff8..b35bf6c 100644 --- a/crates/github/src/tasks.rs +++ b/crates/github/src/tasks.rs @@ -191,6 +191,7 @@ mod tests { files_changed: vec![], summary: "Done".to_string(), pr_body: "Body".to_string(), + review: crate::ReviewResult::none(), exit_reason: None, }, Some(9), diff --git a/crates/worker/src/brief.rs b/crates/worker/src/brief.rs index 2dce3d7..11a27b5 100644 --- a/crates/worker/src/brief.rs +++ b/crates/worker/src/brief.rs @@ -6,8 +6,18 @@ use std::path::Path; use coven_github_api::{Task, TaskKind, HEADLESS_CONTRACT_VERSION}; use coven_github_config::FamiliarConfig; -fn default_contract_version() -> String { - HEADLESS_CONTRACT_VERSION.to_string() +fn deserialize_contract_version<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let version = String::deserialize(deserializer)?; + if version != HEADLESS_CONTRACT_VERSION { + return Err(serde::de::Error::custom(format!( + "unsupported session brief contract_version {}; expected {}", + version, HEADLESS_CONTRACT_VERSION + ))); + } + Ok(version) } /// The session-brief.json schema injected into coven-code --headless. @@ -17,19 +27,25 @@ fn default_contract_version() -> String { /// write authority (comments, Check Runs, branches, PRs) stays with the adapter /// behind its publication gate. See issue #4. #[derive(Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct SessionBrief { /// Contract major version this brief is written against. See /// `docs/headless-contract.md`. - #[serde(default = "default_contract_version")] + #[serde(deserialize_with = "deserialize_contract_version")] pub contract_version: String, pub trigger: String, pub repo: RepoBrief, pub task: TaskBrief, pub familiar: FamiliarBrief, pub workspace: WorkspaceBrief, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub review_context: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub audit_instruction: Option, } #[derive(Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct RepoBrief { pub owner: String, pub name: String, @@ -38,7 +54,7 @@ pub struct RepoBrief { } #[derive(Debug, Serialize, Deserialize)] -#[serde(tag = "kind", rename_all = "snake_case")] +#[serde(tag = "kind", rename_all = "snake_case", deny_unknown_fields)] pub enum TaskBrief { FixIssue { issue_number: u64, @@ -57,6 +73,7 @@ pub enum TaskBrief { } #[derive(Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct FamiliarBrief { pub id: String, pub display_name: String, @@ -65,6 +82,7 @@ pub struct FamiliarBrief { } #[derive(Debug, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] pub struct WorkspaceBrief { pub root: String, } @@ -136,6 +154,8 @@ pub fn build( workspace: WorkspaceBrief { root: workspace.to_string_lossy().to_string(), }, + review_context: None, + audit_instruction: None, } } diff --git a/crates/worker/src/lib.rs b/crates/worker/src/lib.rs index c26f7da..a01e644 100644 --- a/crates/worker/src/lib.rs +++ b/crates/worker/src/lib.rs @@ -7,8 +7,8 @@ use tokio::process::Command; use tracing::{error, info, warn}; use coven_github_api::{ - check_run, installation, pr, repo, tasks::TaskStore, SessionResult, SessionStatus, Task, - TaskKind, DEFAULT_API_BASE_URL, + check_run, installation, pr, repo, tasks::TaskStore, ReviewEvidenceStatus, ReviewMode, + SessionResult, SessionStatus, Task, TaskKind, DEFAULT_API_BASE_URL, }; use coven_github_config::{Config, FamiliarConfig}; @@ -391,7 +391,7 @@ fn disposition(result: &SessionResult) -> Disposition { enum Attempt { /// The runtime exited 0/1/3 and wrote a parseable `result.json`. Terminal: /// the adapter acts on `status`/`exit_reason` and MUST NOT retry. - Completed(SessionResult), + Completed(Box), /// Exit 2, timeout, kill-by-signal, an unexpected exit code, or a spawn/read /// failure. Retry-safe per the contract. RetrySafe(anyhow::Error), @@ -430,7 +430,7 @@ async fn run_session_with_backoff( let mut attempts = 0u32; loop { match run_coven_code(config, brief_path, result_path, git_token).await { - Attempt::Completed(result) => return Ok(result), + Attempt::Completed(result) => return Ok(*result), Attempt::RetrySafe(e) if attempts < max_retries => { attempts += 1; warn!("coven-code attempt {attempts} hit a retry-safe failure ({e:#}), retrying…"); @@ -488,7 +488,7 @@ async fn run_coven_code( // exit codes; if it isn't, the runtime misbehaved — fall back to a // retry-safe failure rather than silently losing the task. Some(code @ (0 | 1 | 3)) => match read_result(result_path).await { - Ok(result) => Attempt::Completed(result), + Ok(result) => Attempt::Completed(Box::new(result)), Err(e) => Attempt::RetrySafe(anyhow::anyhow!( "coven-code exited {code} but result.json was unusable: {e}" )), @@ -506,9 +506,74 @@ async fn read_result(result_path: &Path) -> Result { .await .map_err(|_| anyhow::anyhow!("result.json not written by coven-code"))?; let result: SessionResult = serde_json::from_slice(&bytes)?; + validate_result_contract(&result)?; Ok(result) } +fn validate_result_contract(result: &SessionResult) -> Result<()> { + if result.contract_version != coven_github_api::HEADLESS_CONTRACT_VERSION { + anyhow::bail!( + "unsupported result contract_version {}; expected {}", + result.contract_version, + coven_github_api::HEADLESS_CONTRACT_VERSION + ); + } + if result.status == SessionStatus::Success && result.exit_reason.is_some() { + anyhow::bail!("result exit_reason must be null when status is success"); + } + if matches!( + result.status, + SessionStatus::Failure | SessionStatus::NeedsInput + ) && result.exit_reason.is_none() + { + anyhow::bail!( + "result exit_reason is required when status is {:?}", + result.status + ); + } + let is_review_mode = matches!( + result.review.mode, + ReviewMode::PullRequest | ReviewMode::ReviewComment + ); + if is_review_mode { + if result.review.evidence_status == ReviewEvidenceStatus::NotApplicable { + anyhow::bail!( + "review evidence_status not_applicable is invalid for {:?}", + result.review.mode + ); + } + if result.review.evidence_status != ReviewEvidenceStatus::Missing + && result.review.reviewed_files.is_empty() + { + anyhow::bail!( + "reviewed_files is required for review mode {:?}", + result.review.mode + ); + } + if result.review.evidence_status == ReviewEvidenceStatus::Complete + && result.review.findings.is_empty() + && result + .review + .no_findings_reason + .as_deref() + .map(str::trim) + .unwrap_or_default() + .is_empty() + { + anyhow::bail!("no_findings_reason is required when complete review findings are empty"); + } + } + if result.review.mode == ReviewMode::None + && result.review.evidence_status != ReviewEvidenceStatus::NotApplicable + { + anyhow::bail!( + "review evidence_status {:?} is invalid for none mode", + result.review.evidence_status + ); + } + Ok(()) +} + fn cave_base_url(config: &Config) -> &str { config .server @@ -622,10 +687,293 @@ fn task_issue_number(kind: &TaskKind) -> Option { } } +#[cfg(test)] +mod result_tests { + use super::*; + use std::fs; + + #[tokio::test] + async fn read_result_rejects_unsupported_contract_version() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-version-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"contract_version":"1","status":"success","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"none","evidence_status":"not_applicable","reviewed_files":[],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":null,"limitations":[]},"exit_reason":null}"#, + ) + .expect("result fixture should be written"); + + let error = read_result(&path) + .await + .expect_err("v1 result must be rejected"); + assert!( + format!("{error:#}").contains("unsupported result contract_version 1"), + "unexpected error: {error:#}" + ); + + let _ = fs::remove_file(path); + } + + #[tokio::test] + async fn read_result_rejects_missing_contract_version() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-missing-version-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"status":"success","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"none","evidence_status":"not_applicable","reviewed_files":[],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":null,"limitations":[]},"exit_reason":null}"#, + ) + .expect("result fixture should be written"); + + let error = read_result(&path) + .await + .expect_err("missing contract_version result must be rejected"); + assert!( + format!("{error:#}").contains("missing field `contract_version`"), + "unexpected error: {error:#}" + ); + + let _ = fs::remove_file(path); + } + + #[tokio::test] + async fn read_result_rejects_success_with_exit_reason() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-success-exit-reason-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"contract_version":"2","status":"success","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"none","evidence_status":"not_applicable","reviewed_files":[],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":null,"limitations":[]},"exit_reason":"infra_error"}"#, + ) + .expect("result fixture should be written"); + + let error = read_result(&path) + .await + .expect_err("success result must reject non-null exit_reason"); + assert!( + format!("{error:#}").contains("must be null when status is success"), + "unexpected error: {error:#}" + ); + + let _ = fs::remove_file(path); + } + + #[tokio::test] + async fn read_result_rejects_failure_without_exit_reason() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-failure-exit-reason-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"contract_version":"2","status":"failure","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"none","evidence_status":"not_applicable","reviewed_files":[],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":null,"limitations":[]},"exit_reason":null}"#, + ) + .expect("result fixture should be written"); + + let error = read_result(&path) + .await + .expect_err("non-success result must require exit_reason"); + assert!( + format!("{error:#}").contains("exit_reason is required"), + "unexpected error: {error:#}" + ); + + let _ = fs::remove_file(path); + } + + #[tokio::test] + async fn read_result_rejects_unknown_root_field() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-extra-root-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"contract_version":"2","status":"success","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"none","evidence_status":"not_applicable","reviewed_files":[],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":null,"limitations":[]},"exit_reason":null,"extra_root_field":"rejected"}"#, + ) + .expect("result fixture should be written"); + + let error = read_result(&path) + .await + .expect_err("unknown root field must be rejected"); + assert!( + format!("{error:#}").contains("unknown field `extra_root_field`"), + "unexpected error: {error:#}" + ); + + let _ = fs::remove_file(path); + } + + #[tokio::test] + async fn read_result_rejects_unknown_review_field() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-extra-review-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"contract_version":"2","status":"success","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"none","evidence_status":"not_applicable","reviewed_files":[],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":null,"limitations":[],"extra_review_field":"rejected"},"exit_reason":null}"#, + ) + .expect("result fixture should be written"); + + let error = read_result(&path) + .await + .expect_err("unknown review field must be rejected"); + assert!( + format!("{error:#}").contains("unknown field `extra_review_field`"), + "unexpected error: {error:#}" + ); + + let _ = fs::remove_file(path); + } + + #[tokio::test] + async fn read_result_rejects_not_applicable_evidence_for_review_modes() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-review-evidence-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"contract_version":"2","status":"success","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"pull_request","evidence_status":"not_applicable","reviewed_files":["src/lib.rs"],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":"reviewed supplied file","limitations":[]},"exit_reason":null}"#, + ) + .expect("result fixture should be written"); + + let error = read_result(&path) + .await + .expect_err("review result must reject not_applicable evidence"); + assert!( + format!("{error:#}").contains("review evidence_status not_applicable is invalid"), + "unexpected error: {error:#}" + ); + + let _ = fs::remove_file(path); + } + + #[tokio::test] + async fn read_result_rejects_review_without_reviewed_files() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-review-files-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"contract_version":"2","status":"success","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"pull_request","evidence_status":"complete","reviewed_files":[],"supporting_files":[],"findings":[{"severity":"low","file":"src/lib.rs","line":null,"title":"t","body":"b","recommendation":null}],"tests_run":[],"no_findings_reason":null,"limitations":[]},"exit_reason":null}"#, + ) + .expect("result fixture should be written"); + + let error = read_result(&path) + .await + .expect_err("review result must reject missing reviewed_files"); + assert!( + format!("{error:#}").contains("reviewed_files is required"), + "unexpected error: {error:#}" + ); + + let _ = fs::remove_file(path); + } + + #[tokio::test] + async fn read_result_accepts_review_findings_with_reason() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-findings-reason-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"contract_version":"2","status":"success","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"pull_request","evidence_status":"complete","reviewed_files":["src/lib.rs"],"supporting_files":[],"findings":[{"severity":"low","file":"src/lib.rs","line":null,"title":"t","body":"b","recommendation":null}],"tests_run":[],"no_findings_reason":"Also reviewed nearby context and found this issue.","limitations":[]},"exit_reason":null}"#, + ) + .expect("result fixture should be written"); + + let result = read_result(&path) + .await + .expect("findings with a reason should remain valid"); + assert_eq!(result.review.findings.len(), 1); + assert_eq!( + result.review.no_findings_reason.as_deref(), + Some("Also reviewed nearby context and found this issue.") + ); + + let _ = fs::remove_file(path); + } + + #[tokio::test] + async fn read_result_accepts_partial_review_without_reason() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-review-reason-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"contract_version":"2","status":"partial","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"pull_request","evidence_status":"partial","reviewed_files":["src/lib.rs"],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":null,"limitations":["Review output was degraded before a clean-review conclusion."]},"exit_reason":null}"#, + ) + .expect("result fixture should be written"); + + let result = read_result(&path) + .await + .expect("partial degraded review result should remain valid"); + assert_eq!(result.status, SessionStatus::Partial); + assert!(result.exit_reason.is_none()); + assert!(result.review.findings.is_empty()); + assert!(result.review.no_findings_reason.is_none()); + + let _ = fs::remove_file(path); + } + + #[tokio::test] + async fn read_result_rejects_complete_review_without_reason() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-complete-review-reason-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"contract_version":"2","status":"success","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"pull_request","evidence_status":"complete","reviewed_files":["src/lib.rs"],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":" ","limitations":[]},"exit_reason":null}"#, + ) + .expect("result fixture should be written"); + + let error = read_result(&path) + .await + .expect_err("complete empty review result must require a reason"); + assert!( + format!("{error:#}").contains("complete review findings are empty"), + "unexpected error: {error:#}" + ); + + let _ = fs::remove_file(path); + } + + #[tokio::test] + async fn read_result_rejects_applicable_evidence_for_none_mode() { + let path = std::env::temp_dir().join(format!( + "coven-github-result-none-evidence-{}.json", + uuid::Uuid::new_v4() + )); + fs::write( + &path, + r#"{"contract_version":"2","status":"success","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{"mode":"none","evidence_status":"complete","reviewed_files":[],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":null,"limitations":[]},"exit_reason":null}"#, + ) + .expect("result fixture should be written"); + + let error = read_result(&path) + .await + .expect_err("none-mode result must reject applicable evidence"); + assert!( + format!("{error:#}").contains("is invalid for none mode"), + "unexpected error: {error:#}" + ); + + let _ = fs::remove_file(path); + } +} + #[cfg(test)] mod disposition_tests { use super::*; - use coven_github_api::{CommitInfo, HEADLESS_CONTRACT_VERSION}; + use coven_github_api::{CommitInfo, ReviewResult, HEADLESS_CONTRACT_VERSION}; use coven_github_config::{GitHubAppConfig, ServerConfig, WorkerConfig}; use std::path::PathBuf; @@ -643,6 +991,7 @@ mod disposition_tests { files_changed: vec![], summary: "summary".to_string(), pr_body: "body".to_string(), + review: ReviewResult::none(), exit_reason: None, } } @@ -828,7 +1177,7 @@ mod process_tests { /// A minimal contract-valid result.json with the given status/exit_reason. fn result_json(status: &str, exit_reason: &str) -> String { format!( - r#"{{"contract_version":"1","status":"{status}","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","exit_reason":{exit_reason}}}"# + r#"{{"contract_version":"2","status":"{status}","branch":null,"commits":[],"files_changed":[],"summary":"s","pr_body":"","review":{{"mode":"none","evidence_status":"not_applicable","reviewed_files":[],"supporting_files":[],"findings":[],"tests_run":[],"no_findings_reason":null,"limitations":[]}},"exit_reason":{exit_reason}}}"# ) } diff --git a/crates/worker/tests/contract.rs b/crates/worker/tests/contract.rs index b5cfd91..d0b8dc7 100644 --- a/crates/worker/tests/contract.rs +++ b/crates/worker/tests/contract.rs @@ -5,7 +5,10 @@ //! If these fail, either the runtime contract drifted or the docs are stale — //! fix one of them, do not just bless the test. -use coven_github_api::{ExitReason, SessionResult, SessionStatus, HEADLESS_CONTRACT_VERSION}; +use coven_github_api::{ + ExitReason, ReviewEvidenceStatus, ReviewMode, SessionResult, SessionStatus, + HEADLESS_CONTRACT_VERSION, +}; use coven_github_worker::brief::SessionBrief; fn fixture(name: &str) -> String { @@ -35,6 +38,296 @@ fn golden_session_brief_deserializes_into_adapter_type() { assert_eq!(reserialized, original, "brief did not round-trip cleanly"); } +#[test] +fn hosted_review_session_brief_deserializes_optional_context() { + let raw = r#"{ + "contract_version": "2", + "trigger": "pr_review_comment", + "repo": { + "owner": "OpenCoven", + "name": "coven-github", + "clone_url": "https://github.com/OpenCoven/coven-github.git", + "default_branch": "main" + }, + "task": { + "kind": "address_review_comment", + "pr_number": 31, + "comment_body": "review this", + "diff_hunk": null + }, + "familiar": { + "id": "cody", + "display_name": "Cody", + "model": null, + "skills": [] + }, + "workspace": { + "root": "/tmp/coven" + }, + "review_context": { + "pr_number": 31, + "files": [{ "path": "src/lib.rs" }] + }, + "audit_instruction": "Inspect supplied changed-file patches." + }"#; + + let brief: SessionBrief = + serde_json::from_str(raw).expect("hosted review brief must match SessionBrief"); + + assert_eq!(brief.trigger, "pr_review_comment"); + assert_eq!( + brief + .review_context + .as_ref() + .and_then(|context| context.get("pr_number")) + .and_then(serde_json::Value::as_u64), + Some(31) + ); + assert_eq!( + brief.audit_instruction.as_deref(), + Some("Inspect supplied changed-file patches.") + ); +} + +#[test] +fn session_brief_without_contract_version_is_rejected() { + let raw = r#"{ + "trigger": "issue_assigned", + "repo": { + "owner": "OpenCoven", + "name": "coven-github", + "clone_url": "https://github.com/OpenCoven/coven-github.git", + "default_branch": "main" + }, + "task": { + "kind": "fix_issue", + "issue_number": 119, + "issue_title": "Review contract", + "issue_body": "Tighten the contract." + }, + "familiar": { + "id": "cody", + "display_name": "Cody", + "model": null, + "skills": [] + }, + "workspace": { + "root": "/tmp/coven" + } + }"#; + + let error = serde_json::from_str::(raw) + .expect_err("v2 brief without contract_version must be rejected"); + + assert!( + error + .to_string() + .contains("missing field `contract_version`"), + "unexpected error: {error}" + ); +} + +#[test] +fn session_brief_with_unsupported_contract_version_is_rejected() { + let raw = r#"{ + "contract_version": "1", + "trigger": "issue_assigned", + "repo": { + "owner": "OpenCoven", + "name": "coven-github", + "clone_url": "https://github.com/OpenCoven/coven-github.git", + "default_branch": "main" + }, + "task": { + "kind": "fix_issue", + "issue_number": 119, + "issue_title": "Review contract", + "issue_body": "Tighten the contract." + }, + "familiar": { + "id": "cody", + "display_name": "Cody", + "model": null, + "skills": [] + }, + "workspace": { + "root": "/tmp/coven" + } + }"#; + + let error = serde_json::from_str::(raw).expect_err("v1 brief must be rejected"); + + assert!( + error + .to_string() + .contains("unsupported session brief contract_version 1"), + "unexpected error: {error}" + ); +} + +#[test] +fn session_brief_rejects_unknown_fields() { + let fixtures = [ + ( + "extra_root", + r#"{ + "contract_version": "2", + "trigger": "issue_assigned", + "repo": { + "owner": "OpenCoven", + "name": "coven-github", + "clone_url": "https://github.com/OpenCoven/coven-github.git", + "default_branch": "main" + }, + "task": { + "kind": "fix_issue", + "issue_number": 119, + "issue_title": "Review contract", + "issue_body": "Tighten the contract." + }, + "familiar": { + "id": "cody", + "display_name": "Cody", + "model": null, + "skills": [] + }, + "workspace": { + "root": "/tmp/coven" + }, + "extra_root": true + }"#, + ), + ( + "extra_repo", + r#"{ + "contract_version": "2", + "trigger": "issue_assigned", + "repo": { + "owner": "OpenCoven", + "name": "coven-github", + "clone_url": "https://github.com/OpenCoven/coven-github.git", + "default_branch": "main", + "extra_repo": true + }, + "task": { + "kind": "fix_issue", + "issue_number": 119, + "issue_title": "Review contract", + "issue_body": "Tighten the contract." + }, + "familiar": { + "id": "cody", + "display_name": "Cody", + "model": null, + "skills": [] + }, + "workspace": { + "root": "/tmp/coven" + } + }"#, + ), + ( + "extra_task", + r#"{ + "contract_version": "2", + "trigger": "issue_assigned", + "repo": { + "owner": "OpenCoven", + "name": "coven-github", + "clone_url": "https://github.com/OpenCoven/coven-github.git", + "default_branch": "main" + }, + "task": { + "kind": "fix_issue", + "issue_number": 119, + "issue_title": "Review contract", + "issue_body": "Tighten the contract.", + "extra_task": true + }, + "familiar": { + "id": "cody", + "display_name": "Cody", + "model": null, + "skills": [] + }, + "workspace": { + "root": "/tmp/coven" + } + }"#, + ), + ( + "extra_familiar", + r#"{ + "contract_version": "2", + "trigger": "issue_assigned", + "repo": { + "owner": "OpenCoven", + "name": "coven-github", + "clone_url": "https://github.com/OpenCoven/coven-github.git", + "default_branch": "main" + }, + "task": { + "kind": "fix_issue", + "issue_number": 119, + "issue_title": "Review contract", + "issue_body": "Tighten the contract." + }, + "familiar": { + "id": "cody", + "display_name": "Cody", + "model": null, + "skills": [], + "extra_familiar": true + }, + "workspace": { + "root": "/tmp/coven" + } + }"#, + ), + ( + "extra_workspace", + r#"{ + "contract_version": "2", + "trigger": "issue_assigned", + "repo": { + "owner": "OpenCoven", + "name": "coven-github", + "clone_url": "https://github.com/OpenCoven/coven-github.git", + "default_branch": "main" + }, + "task": { + "kind": "fix_issue", + "issue_number": 119, + "issue_title": "Review contract", + "issue_body": "Tighten the contract." + }, + "familiar": { + "id": "cody", + "display_name": "Cody", + "model": null, + "skills": [] + }, + "workspace": { + "root": "/tmp/coven", + "extra_workspace": true + } + }"#, + ), + ]; + + for (field, raw) in fixtures { + let error = match serde_json::from_str::(raw) { + Ok(_) => panic!("brief with {field} must be rejected"), + Err(error) => error, + }; + let message = error.to_string(); + assert!( + message.contains("unknown field"), + "unexpected error for {field}: {message}" + ); + } +} + #[test] fn golden_result_deserializes_into_adapter_type() { let raw = fixture("result.example.json"); @@ -43,6 +336,11 @@ fn golden_result_deserializes_into_adapter_type() { assert_eq!(result.contract_version, HEADLESS_CONTRACT_VERSION); assert_eq!(result.status, SessionStatus::Success); + assert_eq!(result.review.mode, ReviewMode::None); + assert_eq!( + result.review.evidence_status, + ReviewEvidenceStatus::NotApplicable + ); assert_eq!(result.branch.as_deref(), Some("cody/fix-issue-42")); assert_eq!(result.commits.len(), 1); assert_eq!( @@ -53,9 +351,7 @@ fn golden_result_deserializes_into_adapter_type() { } #[test] -fn result_without_contract_version_defaults_to_current() { - // Backward compatibility: a runtime that predates the version field still - // parses, and is treated as the current major version. +fn result_without_contract_version_is_rejected() { let raw = r#"{ "status": "failure", "branch": null, @@ -63,12 +359,27 @@ fn result_without_contract_version_defaults_to_current() { "files_changed": [], "summary": "Could not reproduce.", "pr_body": "", + "review": { + "mode": "none", + "evidence_status": "not_applicable", + "reviewed_files": [], + "supporting_files": [], + "findings": [], + "tests_run": [], + "no_findings_reason": null, + "limitations": [] + }, "exit_reason": "ambiguous_spec" }"#; - let result: SessionResult = serde_json::from_str(raw).expect("legacy result must parse"); - assert_eq!(result.contract_version, HEADLESS_CONTRACT_VERSION); - assert_eq!(result.status, SessionStatus::Failure); - assert_eq!(result.exit_reason, Some(ExitReason::AmbiguousSpec)); + let error = serde_json::from_str::(raw) + .expect_err("v2 result without contract_version must be rejected"); + + assert!( + error + .to_string() + .contains("missing field `contract_version`"), + "unexpected error: {error}" + ); } #[test] diff --git a/deploy/coven-github/README.md b/deploy/coven-github/README.md new file mode 100644 index 0000000..ece56de --- /dev/null +++ b/deploy/coven-github/README.md @@ -0,0 +1,43 @@ +# coven-github hosted dogfood adapter + +This directory tracks the Python adapter used by the hosted coven-github +dogfood GitHub App. + +The adapter is intentionally deployment-specific. It is not the canonical Rust +worker implementation; it exists so hosted dogfood behavior can be reviewed, +reproduced, and changed through PRs instead of invisible server edits. + +## Files + +- `coven_github_adapter.py` - webhook routing helpers, task runner, PR evidence + capture, Codex-backed headless runtime invocation, and dogfood comment + publisher. + +## Runtime inputs + +The deployment expects secrets and mutable state to be supplied outside git: + +- `GITHUB_APP_PRIVATE_KEY_PATH` or `.coven-github-private-key.pem` +- `GITHUB_APP_ID` +- `GITHUB_WEBHOOK_SECRET` +- `COVEN_GITHUB_STATE_DIR` +- `COVEN_GITHUB_POLICY_PATH` +- `COVEN_CODE_BIN` +- `COVEN_REVIEW_FIX_LOOPS` - optional bounded review-fix loop count, clamped + between `0` and `5`; defaults to `0` so hosted repair loops are opt-in +- Codex OAuth tokens under the deployed account's `.coven-code` directory + +Do not commit private keys, webhook secrets, OAuth tokens, generated task state, +workspaces, or attempt artifacts. + +## Current dogfood behavior + +- Emits headless contract v2 session briefs. +- Captures PR checkout metadata and changed-file patches before invoking + `coven-code`. +- Publishes visible structured review evidence, including `reviewed_files`, + `supporting_files`, findings, test evidence, no-findings rationale, and + limitations. +- When `COVEN_REVIEW_FIX_LOOPS` is greater than `0`, reruns `coven-code` with + prior structured review findings as explicit repair instructions until no + findings remain or the configured loop count is exhausted. diff --git a/deploy/coven-github/coven_github_adapter.py b/deploy/coven-github/coven_github_adapter.py new file mode 100644 index 0000000..10290ee --- /dev/null +++ b/deploy/coven-github/coven_github_adapter.py @@ -0,0 +1,1476 @@ +import base64 +import hashlib +import hmac +import json +import os +import re +import subprocess +import tempfile +import time +import traceback +from datetime import datetime, timezone +from pathlib import Path +from urllib.error import HTTPError +from urllib.request import Request, urlopen + + +ROOT_DIR = Path(__file__).resolve().parent +STATE_DIR = Path(os.environ.get("COVEN_GITHUB_STATE_DIR", ROOT_DIR / "coven-github-state")) +DELIVERIES_DIR = STATE_DIR / "deliveries" +TASKS_DIR = STATE_DIR / "tasks" +WORKSPACES_DIR = STATE_DIR / "workspaces" +ATTEMPTS_DIR = STATE_DIR / "attempts" +POLICY_PATH = Path(os.environ.get("COVEN_GITHUB_POLICY_PATH", ROOT_DIR / "coven-github-policy.json")) +PRIVATE_KEY_PATH = Path( + os.environ.get("GITHUB_APP_PRIVATE_KEY_PATH", ROOT_DIR / ".coven-github-private-key.pem") +) +APP_ID = os.environ.get("GITHUB_APP_ID", "").strip() +COVEN_CODE_BIN = os.environ.get("COVEN_CODE_BIN", "coven-code").strip() or "coven-code" +COVEN_CODE_MODEL = os.environ.get("COVEN_CODE_MODEL", "gpt-5.5").strip() +WEBHOOK_SECRET = os.environ.get("GITHUB_WEBHOOK_SECRET", "").strip() + + +def env_int(name, default, minimum=0, maximum=10): + raw = os.environ.get(name, "").strip() + if not raw: + return default + try: + value = int(raw) + except ValueError: + return default + return max(minimum, min(maximum, value)) + + +MAX_REVIEW_FIX_LOOPS = env_int("COVEN_REVIEW_FIX_LOOPS", 0, minimum=0, maximum=5) +TERMINAL_RESULT_EXIT_CODES = {0, 1, 3} +RESULT_STATUSES = {"success", "failure", "partial", "needs_input"} +REVIEW_MODES = {"none", "pull_request", "review_comment"} +REVIEW_EVIDENCE_STATUSES = {"not_applicable", "complete", "partial", "missing"} +RESULT_FIELDS = { + "contract_version", + "status", + "branch", + "commits", + "files_changed", + "summary", + "pr_body", + "review", + "exit_reason", +} +COMMIT_FIELDS = {"sha", "message"} +REVIEW_FIELDS = { + "mode", + "evidence_status", + "reviewed_files", + "supporting_files", + "findings", + "tests_run", + "no_findings_reason", + "limitations", +} +FINDING_FIELDS = {"severity", "file", "line", "title", "body", "recommendation"} +TEST_RUN_FIELDS = {"command", "status", "output_summary"} + + +def account_home(): + try: + import pwd + + return Path(pwd.getpwuid(os.getuid()).pw_dir) + except Exception: + return Path.home() + + +def configured_codex_tokens_path(): + configured = os.environ.get("COVEN_CODE_CODEX_TOKENS_PATH", "").strip() + if configured: + return Path(configured).expanduser() + return account_home() / ".coven-code" / "codex_tokens.json" + + +CODEX_TOKENS_PATH = configured_codex_tokens_path() + +for directory in (DELIVERIES_DIR, TASKS_DIR, WORKSPACES_DIR, ATTEMPTS_DIR): + directory.mkdir(parents=True, exist_ok=True) + + +DEFAULT_FAMILIAR = { + "id": "cody", + "display_name": "Cody", + "model": None, + "skills": ["code-review"], +} + + +DEFAULT_POLICY = { + "version": 1, + "installations": {}, +} + + +def utc_now(): + return datetime.now(timezone.utc).isoformat() + + +def read_json(path, default): + try: + with open(path, "r", encoding="utf-8") as handle: + return json.load(handle) + except FileNotFoundError: + return default + + +def write_json_atomic(path, value): + path.parent.mkdir(parents=True, exist_ok=True) + fd, tmp_name = tempfile.mkstemp(prefix=path.name + ".", suffix=".tmp", dir=str(path.parent)) + try: + with os.fdopen(fd, "w", encoding="utf-8") as handle: + json.dump(value, handle, sort_keys=True, indent=2) + handle.write("\n") + os.replace(tmp_name, str(path)) + finally: + if os.path.exists(tmp_name): + os.unlink(tmp_name) + + +def b64url(raw): + return base64.urlsafe_b64encode(raw).rstrip(b"=").decode("ascii") + + +def sign_rs256(message): + from cryptography.hazmat.primitives import hashes, serialization + from cryptography.hazmat.primitives.asymmetric import padding + + key_data = PRIVATE_KEY_PATH.read_bytes() + private_key = serialization.load_pem_private_key(key_data, password=None) + return private_key.sign(message, padding.PKCS1v15(), hashes.SHA256()) + + +def github_app_jwt(): + if not APP_ID: + raise RuntimeError("GITHUB_APP_ID is required") + now = int(time.time()) + header = {"alg": "RS256", "typ": "JWT"} + payload = {"iat": now - 60, "exp": now + 540, "iss": APP_ID} + signing_input = ( + b64url(json.dumps(header, separators=(",", ":")).encode("utf-8")) + + "." + + b64url(json.dumps(payload, separators=(",", ":")).encode("utf-8")) + ).encode("ascii") + return signing_input.decode("ascii") + "." + b64url(sign_rs256(signing_input)) + + +def github_request(method, url, token, body=None): + headers = { + "Accept": "application/vnd.github+json", + "Authorization": "Bearer " + token, + "User-Agent": "coven-github-hosted-prototype", + "X-GitHub-Api-Version": "2022-11-28", + } + data = None + if body is not None: + data = json.dumps(body).encode("utf-8") + headers["Content-Type"] = "application/json" + request = Request(url, data=data, headers=headers, method=method) + try: + with urlopen(request, timeout=30) as response: + raw = response.read().decode("utf-8") + return json.loads(raw) if raw else {} + except HTTPError as exc: + raw = exc.read().decode("utf-8", errors="replace") + raise RuntimeError("GitHub API {} {} failed: {}".format(method, url, raw)) + + +def installation_token(installation_id): + app_token = github_app_jwt() + response = github_request( + "POST", + "https://api.github.com/app/installations/{}/access_tokens".format(installation_id), + app_token, + {}, + ) + token = response.get("token") + if not token: + raise RuntimeError("GitHub installation token response did not include token") + return token + + +def load_policy(): + if not POLICY_PATH.exists(): + write_json_atomic(POLICY_PATH, DEFAULT_POLICY) + return read_json(POLICY_PATH, DEFAULT_POLICY) + + +def repo_policy(payload): + policy = load_policy() + installation_id = str((payload.get("installation") or {}).get("id") or "") + repository = payload.get("repository") or {} + repo_id = str(repository.get("id") or "") + installation = (policy.get("installations") or {}).get(installation_id) or {} + repo = (installation.get("repositories") or {}).get(repo_id) + return installation_id, repo_id, repo + + +def delivery_path(delivery_id): + return DELIVERIES_DIR / (delivery_id + ".json") + + +def task_path(task_id): + return TASKS_DIR / (task_id + ".json") + + +def payload_hash(payload): + raw = json.dumps(payload, sort_keys=True, separators=(",", ":")).encode("utf-8") + return hashlib.sha256(raw).hexdigest() + + +def delivery_record(delivery_id, event_name, payload): + repository = payload.get("repository") or {} + installation = payload.get("installation") or {} + return { + "delivery_id": delivery_id, + "event": event_name, + "action": payload.get("action"), + "installation_id": installation.get("id"), + "repository_id": repository.get("id"), + "repository": repository.get("full_name"), + "payload_hash": payload_hash(payload), + "received_at": utc_now(), + "state": "received", + "issue_refs": ["OpenCoven/coven-github#2"], + } + + +def mentioned(text, policy): + normalized = text or "" + for username in policy.get("bot_usernames") or []: + login = str(username).strip() + if not login: + continue + pattern = r"(?= 1 or null".format( + index + ) + ) + require_string(finding.get("title"), "result.review.findings[{}].title".format(index)) + require_string(finding.get("body"), "result.review.findings[{}].body".format(index)) + require_optional_string( + finding.get("recommendation"), + "result.review.findings[{}].recommendation".format(index), + ) + + +def validate_tests_run(tests_run): + if not isinstance(tests_run, list): + raise ValueError("result.review.tests_run must be an array") + statuses = {"passed", "failed", "not_run", "unknown"} + for index, test in enumerate(tests_run): + if not isinstance(test, dict): + raise ValueError("result.review.tests_run[{}] must be an object".format(index)) + validate_object_fields(test, TEST_RUN_FIELDS, "result.review.tests_run[{}]".format(index)) + for field in TEST_RUN_FIELDS: + if field not in test: + raise ValueError( + "result.review.tests_run[{}] missing required field {}".format(index, field) + ) + require_string(test.get("command"), "result.review.tests_run[{}].command".format(index)) + status = test.get("status") + if status not in statuses: + raise ValueError("unsupported review test status {}".format(status)) + require_optional_string( + test.get("output_summary"), + "result.review.tests_run[{}].output_summary".format(index), + ) + + +def validate_result_contract(result): + if not isinstance(result, dict): + raise ValueError("result.json must be a JSON object") + + validate_object_fields(result, RESULT_FIELDS, "result.json") + for field in ("contract_version", "status", "commits", "files_changed", "summary", "pr_body", "review"): + if field not in result: + raise ValueError("result.json missing required field {}".format(field)) + + if result.get("contract_version") != "2": + raise ValueError("unsupported result contract_version {}".format(result.get("contract_version"))) + if result.get("status") not in RESULT_STATUSES: + raise ValueError("unsupported result status {}".format(result.get("status"))) + status = result.get("status") + validate_commits(result.get("commits")) + validate_string_array(result.get("files_changed"), "result.files_changed") + if not isinstance(result.get("summary"), str): + raise ValueError("result.summary must be a string") + if not isinstance(result.get("pr_body"), str): + raise ValueError("result.pr_body must be a string") + if result.get("branch") is not None and not isinstance(result.get("branch"), str): + raise ValueError("result.branch must be a string or null") + if result.get("exit_reason") not in ( + "test_failure", + "ambiguous_spec", + "git_conflict", + "infra_error", + None, + ): + raise ValueError("unsupported result exit_reason {}".format(result.get("exit_reason"))) + if status == "success" and result.get("exit_reason") is not None: + raise ValueError("result.exit_reason must be null when status is success") + if status != "success" and result.get("exit_reason") is None: + raise ValueError("result.exit_reason is required when status is {}".format(status)) + + review = result.get("review") + if not isinstance(review, dict): + raise ValueError("result.review must be an object") + validate_object_fields(review, REVIEW_FIELDS, "result.review") + for field in REVIEW_FIELDS: + if field not in review: + raise ValueError("result.review missing required field {}".format(field)) + + mode = review.get("mode") + evidence_status = review.get("evidence_status") + if mode not in REVIEW_MODES: + raise ValueError("unsupported review mode {}".format(mode)) + if evidence_status not in REVIEW_EVIDENCE_STATUSES: + raise ValueError("unsupported review evidence_status {}".format(evidence_status)) + + validate_string_array(review.get("reviewed_files"), "result.review.reviewed_files") + validate_string_array(review.get("supporting_files"), "result.review.supporting_files") + validate_findings(review.get("findings")) + validate_tests_run(review.get("tests_run")) + require_optional_string(review.get("no_findings_reason"), "result.review.no_findings_reason") + validate_string_array(review.get("limitations"), "result.review.limitations") + + if mode in ("pull_request", "review_comment"): + if evidence_status == "not_applicable": + raise ValueError("review evidence_status not_applicable is invalid for {}".format(mode)) + if evidence_status != "missing" and not review.get("reviewed_files"): + raise ValueError("reviewed_files is required for review mode {}".format(mode)) + no_findings_reason = review.get("no_findings_reason") + if not review.get("findings") and not ( + isinstance(no_findings_reason, str) and no_findings_reason.strip() + ): + raise ValueError("no_findings_reason is required when review findings are empty") + if mode == "none" and evidence_status != "not_applicable": + raise ValueError("review evidence_status {} is invalid for none mode".format(evidence_status)) + + +def session_brief(task, workspace, review_context=None, extra_audit_instruction=None): + owner, name = (task["repository"] or "/").split("/", 1) + brief = { + "contract_version": "2", + "trigger": task["trigger"], + "repo": { + "owner": owner, + "name": name, + "clone_url": task["clone_url"], + "default_branch": task["default_branch"], + }, + "task": task["task"], + "familiar": task["familiar"], + "workspace": {"root": str(workspace)}, + } + if review_context: + brief["review_context"] = review_context + instruction = ( + "This run is evidence-backed. Review the supplied PR metadata and " + "changed-file patches in review_context before responding. Cite the " + "specific changed files you inspected in the result summary." + ) + if extra_audit_instruction: + instruction = instruction + "\n\n" + extra_audit_instruction + brief["audit_instruction"] = instruction + return brief + + +def run_coven_code_cycle( + task, + workspace, + review_context, + attempt_dir, + env, + cycle, + extra_audit_instruction=None, +): + suffix = "" if cycle == 0 else "-repair-{}".format(cycle) + brief_path = attempt_dir / "session-brief{}.json".format(suffix) + result_path = attempt_dir / "result{}.json".format(suffix) + run_path = attempt_dir / "run{}.json".format(suffix) + + write_json_atomic( + brief_path, + session_brief(task, workspace, review_context, extra_audit_instruction), + ) + run = run_command( + [ + COVEN_CODE_BIN, + "--headless", + "--hosted-review", + "--provider", + "codex", + "--model", + COVEN_CODE_MODEL, + "--context", + str(brief_path), + "--output", + str(result_path), + ], + cwd=str(workspace), + env=env, + timeout=1800, + ) + write_json_atomic(run_path, redacted_command_result(run)) + result = None + result_error = None + if result_path.exists(): + try: + result = read_json(result_path, None) + validate_result_contract(result) + except Exception as exc: + result_error = str(exc) + result = None + return { + "cycle": cycle, + "brief_path": brief_path, + "result_path": result_path, + "run_path": run_path, + "run": run, + "result": result, + "result_error": result_error, + } + + +def review_findings(result): + if not result: + return [] + review = result.get("review") or {} + mode = review.get("mode") + if mode not in ("pull_request", "review_comment"): + return [] + return review.get("findings") or [] + + +def review_fix_instruction(findings, iteration, max_iterations): + lines = [ + "Autofix review loop iteration {}/{}.".format(iteration, max_iterations), + "The previous hosted review returned structured findings. Fix the findings below, run the relevant checks you can run safely, then perform another bounded review of the updated code using the required review sections.", + "If a finding cannot be fixed safely, leave a clear limitation and explain the remaining blocker. Do not merely restate the findings.", + "", + "Findings to fix:", + ] + for index, finding in enumerate(findings[:10], start=1): + location = finding.get("file") or "unknown file" + if finding.get("line") is not None: + location = "{}:{}".format(location, finding.get("line")) + lines.append( + "{}. [{}] `{}` {}".format( + index, + finding.get("severity") or "unknown", + location, + finding.get("title") or "Untitled finding", + ) + ) + body = (finding.get("body") or "").strip() + if body: + lines.append(" Body: {}".format(body[:1200])) + recommendation = (finding.get("recommendation") or "").strip() + if recommendation: + lines.append(" Recommendation: {}".format(recommendation[:1200])) + if len(findings) > 10: + lines.append("Only the first 10 findings are listed; inspect the prior result for the full set.") + return "\n".join(lines) + + +def task_with_repair_request(task, instruction): + copy = json.loads(json.dumps(task)) + task_data = copy.get("task") or {} + explicit_request = ( + "\n\nPlease fix the review findings from the previous hosted review cycle. " + "After fixing them, rerun relevant checks and produce another structured review.\n\n" + + instruction + ) + if "comment_body" in task_data: + task_data["comment_body"] = (task_data.get("comment_body") or "") + explicit_request + elif "issue_body" in task_data: + task_data["issue_body"] = (task_data.get("issue_body") or "") + explicit_request + copy["task"] = task_data + return copy + + +def run_task(task_id, debug): + path = task_path(task_id) + task = read_json(path, {}) + if task.get("state") != "queued": + return task + + task["state"] = "running" + task["attempts"] = int(task.get("attempts") or 0) + 1 + task["updated_at"] = utc_now() + write_json_atomic(path, task) + + attempt_dir = ATTEMPTS_DIR / task_id / str(task["attempts"]) + attempt_dir.mkdir(parents=True, exist_ok=True) + workspace = WORKSPACES_DIR / task_id / "repo" + + try: + token = installation_token(task["installation_id"]) + askpass = write_askpass(attempt_dir) + env = os.environ.copy() + env["GIT_ASKPASS"] = str(askpass) + env["GIT_TERMINAL_PROMPT"] = "0" + env["COVEN_GIT_TOKEN"] = token + env["COVEN_CODE_PROVIDER"] = "codex" + env["COVEN_CODE_HOSTED_REVIEW"] = "1" + env["HOME"] = str(account_home()) + codex_access_token = load_codex_access_token() + if not codex_access_token: + return fail_task( + path, + task, + "codex_auth_missing", + "Missing Codex access token at {}".format(CODEX_TOKENS_PATH), + ) + env["OPENAI_API_KEY"] = codex_access_token + + if not workspace.exists(): + clone = run_command( + [ + "git", + "clone", + "--depth", + "1", + "--branch", + task["default_branch"], + task["clone_url"], + str(workspace), + ], + env=env, + timeout=180, + ) + write_json_atomic(attempt_dir / "clone.json", redacted_command_result(clone)) + if clone["returncode"] != 0: + return fail_task(path, task, "clone_failed", clone["stderr"]) + + review_context = prepare_review_context(task, workspace, token, env, attempt_dir) + if review_context: + review_context_path = attempt_dir / "review-context.json" + write_json_atomic(review_context_path, review_context) + task["review_context_path"] = str(review_context_path) + task["review_context_sha256"] = file_sha256(review_context_path) + task["review_evidence"] = review_evidence(review_context, review_context_path, task) + write_json_atomic(path, task) + + if not command_exists(COVEN_CODE_BIN): + return fail_task( + path, + task, + "runtime_missing", + "COVEN_CODE_BIN is not available on the host: {}".format(COVEN_CODE_BIN), + ) + + cycle_result = run_coven_code_cycle(task, workspace, review_context, attempt_dir, env, 0) + brief_path = cycle_result["brief_path"] + result_path = cycle_result["result_path"] + run = cycle_result["run"] + task["session_brief_path"] = str(brief_path) + task["session_brief_sha256"] = file_sha256(brief_path) + task["runtime_exit_code"] = run["returncode"] + task["result_path"] = str(result_path) + write_json_atomic(path, task) + + if run["returncode"] not in TERMINAL_RESULT_EXIT_CODES: + return fail_task( + path, + task, + "runtime_non_terminal", + "coven-code exited {} before a terminal result could be used: {}".format( + run["returncode"], run["stderr"] + ), + ) + if cycle_result["result"] is None: + reason = "result_invalid" if cycle_result.get("result_error") else "result_missing" + detail = cycle_result.get("result_error") or "coven-code exited {} without writing result.json: {}".format( + run["returncode"], run["stderr"] + ) + return fail_task( + path, + task, + reason, + detail, + ) + + final_cycle = cycle_result + loop_records = [] + for iteration in range(1, MAX_REVIEW_FIX_LOOPS + 1): + findings = review_findings(final_cycle["result"]) + if not findings: + break + instruction = review_fix_instruction(findings, iteration, MAX_REVIEW_FIX_LOOPS) + repair_task = task_with_repair_request(task, instruction) + repair_cycle = run_coven_code_cycle( + repair_task, + workspace, + review_context, + attempt_dir, + env, + iteration, + instruction, + ) + if repair_cycle["run"]["returncode"] not in TERMINAL_RESULT_EXIT_CODES: + return fail_task( + path, + task, + "runtime_non_terminal", + "review repair loop {} exited {} before a terminal result could be used: {}".format( + iteration, + repair_cycle["run"]["returncode"], + repair_cycle["run"]["stderr"], + ), + ) + if repair_cycle["result"] is None: + reason = "result_invalid" if repair_cycle.get("result_error") else "result_missing" + detail = repair_cycle.get("result_error") or ( + "review repair loop {} exited {} without writing result.json: {}".format( + iteration, + repair_cycle["run"]["returncode"], + repair_cycle["run"]["stderr"], + ) + ) + return fail_task( + path, + task, + reason, + detail, + ) + remaining = review_findings(repair_cycle["result"]) + loop_records.append( + { + "iteration": iteration, + "input_findings": len(findings), + "runtime_exit_code": repair_cycle["run"]["returncode"], + "result_path": str(repair_cycle["result_path"]), + "result_status": (repair_cycle["result"] or {}).get("status"), + "remaining_findings": len(remaining), + } + ) + task["review_fix_loops"] = loop_records + task["runtime_exit_code"] = repair_cycle["run"]["returncode"] + task["result_path"] = str(repair_cycle["result_path"]) + task["updated_at"] = utc_now() + write_json_atomic(path, task) + final_cycle = repair_cycle + + result_path = final_cycle["result_path"] + run = final_cycle["run"] + task["runtime_exit_code"] = run["returncode"] + task["result_path"] = str(result_path) + task["state"] = "completed" if run["returncode"] in (0, 1, 3) else "failed" + task["updated_at"] = utc_now() + publish_result_if_configured(task, result_path, token) + write_json_atomic(path, task) + return task + except Exception as exc: + return fail_task(path, task, "infra_error", repr(exc)) + + +def command_exists(command): + probe = run_command(["/bin/sh", "-lc", "command -v {}".format(shell_quote(command))], timeout=10) + return probe["returncode"] == 0 + + +def shell_quote(value): + return "'" + str(value).replace("'", "'\"'\"'") + "'" + + +def pr_number_for_task(task): + task_data = task.get("task") or {} + target = task.get("target") or {} + if target.get("kind") == "pull_request" and target.get("pr_number"): + return int(target.get("pr_number")) + value = task_data.get("pr_number") + if value: + return int(value) + return None + + +def prepare_review_context(task, workspace, token, env, attempt_dir): + pr_number = pr_number_for_task(task) + if not pr_number: + return None + + repo = task.get("repository") + pr = github_request( + "GET", + "https://api.github.com/repos/{}/pulls/{}".format(repo, pr_number), + token, + ) + files = paginated_pr_files(repo, pr_number, token) + + fetch = run_command( + ["git", "fetch", "--depth", "1", "origin", "pull/{}/head".format(pr_number)], + cwd=str(workspace), + env=env, + timeout=180, + ) + write_json_atomic(attempt_dir / "fetch-pr.json", redacted_command_result(fetch)) + if fetch["returncode"] != 0: + raise RuntimeError("failed to fetch PR #{} head: {}".format(pr_number, fetch["stderr"])) + + checkout = run_command(["git", "checkout", "--detach", "FETCH_HEAD"], cwd=str(workspace), env=env) + write_json_atomic(attempt_dir / "checkout-pr.json", redacted_command_result(checkout)) + if checkout["returncode"] != 0: + raise RuntimeError("failed to checkout PR #{} head: {}".format(pr_number, checkout["stderr"])) + + head = run_command(["git", "rev-parse", "HEAD"], cwd=str(workspace), env=env) + status = run_command(["git", "status", "--short", "--branch"], cwd=str(workspace), env=env) + write_json_atomic(attempt_dir / "workspace-git.json", redacted_command_result({ + "args": ["git evidence"], + "returncode": 0 if head["returncode"] == 0 and status["returncode"] == 0 else 1, + "stdout": "HEAD={}\n{}".format(head["stdout"].strip(), status["stdout"].strip()), + "stderr": head["stderr"] + status["stderr"], + })) + if head["returncode"] != 0: + raise RuntimeError( + "failed to read checked-out PR #{} HEAD: {}".format(pr_number, head["stderr"]) + ) + + workspace_head_sha = head["stdout"].strip() + metadata_head_sha = str(((pr.get("head") or {}).get("sha")) or "").strip() + if not workspace_head_sha: + raise RuntimeError("checked-out PR #{} HEAD was empty".format(pr_number)) + if metadata_head_sha and workspace_head_sha != metadata_head_sha: + raise RuntimeError( + "checked-out PR #{} HEAD {} does not match GitHub metadata head {}".format( + pr_number, + workspace_head_sha, + metadata_head_sha, + ) + ) + + return { + "kind": "pull_request", + "pr_number": pr_number, + "metadata": summarize_pr(pr), + "files": summarize_pr_files(files), + "checkout": { + "fetch_returncode": fetch["returncode"], + "checkout_returncode": checkout["returncode"], + "workspace_head_sha": workspace_head_sha, + "workspace_status": status["stdout"].strip(), + }, + } + + +def paginated_pr_files(repo, pr_number, token): + files = [] + page = 1 + while True: + page_items = github_request( + "GET", + "https://api.github.com/repos/{}/pulls/{}/files?per_page=100&page={}".format( + repo, pr_number, page + ), + token, + ) + files.extend(page_items or []) + if len(page_items or []) < 100: + break + page += 1 + return files + + +def summarize_pr(pr): + return { + "number": pr.get("number"), + "title": pr.get("title"), + "state": pr.get("state"), + "html_url": pr.get("html_url"), + "base_ref": (pr.get("base") or {}).get("ref"), + "base_sha": (pr.get("base") or {}).get("sha"), + "head_ref": (pr.get("head") or {}).get("ref"), + "head_sha": (pr.get("head") or {}).get("sha"), + "merge_commit_sha": pr.get("merge_commit_sha"), + } + + +def summarize_pr_files(files): + summarized = [] + for item in files or []: + patch = item.get("patch") or "" + summarized.append( + { + "filename": item.get("filename"), + "status": item.get("status"), + "additions": item.get("additions"), + "deletions": item.get("deletions"), + "changes": item.get("changes"), + "sha": item.get("sha"), + "patch": patch[:12000], + "patch_truncated": len(patch) > 12000, + } + ) + return summarized + + +def review_evidence(review_context, review_context_path, task): + metadata = review_context.get("metadata") or {} + files = review_context.get("files") or [] + checkout = review_context.get("checkout") or {} + return { + "pr_number": review_context.get("pr_number"), + "base_ref": metadata.get("base_ref"), + "base_sha": metadata.get("base_sha"), + "head_ref": metadata.get("head_ref"), + "head_sha": metadata.get("head_sha"), + "workspace_head_sha": checkout.get("workspace_head_sha"), + "changed_file_count": len(files), + "changed_files": [f.get("filename") for f in files], + "review_context_path": str(review_context_path), + "review_context_sha256": file_sha256(review_context_path), + } + + +def file_sha256(path): + digest = hashlib.sha256() + with open(path, "rb") as handle: + for chunk in iter(lambda: handle.read(65536), b""): + digest.update(chunk) + return digest.hexdigest() + + +def publish_result_if_configured(task, result_path, token): + publication = task.get("publication") or {} + mode = publication.get("mode") or "record_only" + if mode != "comment": + task["publication_state"] = "held_for_issue_11_publication_gates" + return + + result = read_json(result_path, {}) + number = ( + (task.get("task") or {}).get("issue_number") + or (task.get("task") or {}).get("pr_number") + ) + if not number: + task["publication_state"] = "publication_skipped_no_issue_or_pr_number" + return + + body = publication_comment_body(task, result) + repo = task.get("repository") + url = "https://api.github.com/repos/{}/issues/{}/comments".format(repo, int(number)) + try: + response = github_request("POST", url, token, {"body": body}) + task["publication_state"] = "published_comment" + task["publication_url"] = response.get("html_url") + task["publication_comment_id"] = response.get("id") + except Exception as exc: + task["publication_state"] = "publication_failed" + task["publication_error"] = redact_tokenish(repr(exc)) + + +def publication_comment_body(task, result): + status = result.get("status") or "unknown" + summary = result.get("summary") or "No summary returned." + pr_body = result.get("pr_body") or "" + files_changed = result.get("files_changed") or [] + commits = result.get("commits") or [] + task_id = task.get("task_id") or "" + evidence = task.get("review_evidence") or {} + review = result.get("review") or {} + + parts = [ + "## Cody dogfood result", + "", + "**Status:** {}".format(status), + "", + summary.strip(), + ] + if pr_body.strip() and pr_body.strip() != summary.strip(): + parts.extend(["", pr_body.strip()]) + parts.extend(review_fix_loop_lines(task)) + parts.extend(["", "### Evidence"]) + if evidence: + changed_files = evidence.get("changed_files") or [] + parts.extend( + [ + "- PR: #{}".format(evidence.get("pr_number")), + "- Base: `{}` @ `{}`".format(evidence.get("base_ref"), evidence.get("base_sha")), + "- Head: `{}` @ `{}`".format(evidence.get("head_ref"), evidence.get("head_sha")), + "- Checked-out workspace HEAD: `{}`".format(evidence.get("workspace_head_sha")), + "- Changed files supplied to agent: {}".format(evidence.get("changed_file_count")), + "- Review context SHA-256: `{}`".format(evidence.get("review_context_sha256")), + ] + ) + if changed_files: + parts.append("- Files: {}".format(", ".join("`{}`".format(f) for f in changed_files[:20]))) + else: + parts.append("- No PR review evidence was captured for this run.") + parts.extend(structured_review_lines(review)) + parts.extend( + [ + "", + "**Files changed:** {}".format(len(files_changed)), + "**Commits:** {}".format(len(commits)), + "", + "_Task `{}`. Publication is enabled on the hosted test adapter only._".format(task_id), + ] + ) + return "\n".join(parts) + + +def review_fix_loop_lines(task): + loops = task.get("review_fix_loops") or [] + if not loops: + return [] + + lines = ["", "### Review fix loop"] + for loop in loops: + lines.append( + "- Iteration {iteration}: input findings {input_findings}, result `{result_status}`, remaining findings {remaining_findings}.".format( + iteration=loop.get("iteration"), + input_findings=loop.get("input_findings"), + result_status=loop.get("result_status") or "unknown", + remaining_findings=loop.get("remaining_findings"), + ) + ) + if loops and loops[-1].get("remaining_findings", 0): + lines.append( + "- Loop stopped after {} configured iteration(s); unresolved findings remain.".format( + len(loops) + ) + ) + return lines + + +def structured_review_lines(review): + if not review: + return ["", "### Structured review", "- No structured review result was emitted."] + + lines = [ + "", + "### Structured review", + "- Mode: `{}`".format(review.get("mode") or "unknown"), + "- Evidence status: `{}`".format(review.get("evidence_status") or "unknown"), + ] + + reviewed_files = review.get("reviewed_files") or [] + lines.append("- Reviewed files: {}".format(len(reviewed_files))) + if reviewed_files: + lines.append( + "- Reviewed file list: {}".format( + ", ".join("`{}`".format(path) for path in reviewed_files[:20]) + ) + ) + if len(reviewed_files) > 20: + lines.append("- Reviewed file list truncated after 20 entries.") + + supporting_files = review.get("supporting_files") or [] + lines.append("- Supporting files inspected: {}".format(len(supporting_files))) + if supporting_files: + lines.append( + "- Supporting file list: {}".format( + ", ".join("`{}`".format(path) for path in supporting_files[:20]) + ) + ) + if len(supporting_files) > 20: + lines.append("- Supporting file list truncated after 20 entries.") + + findings = review.get("findings") or [] + lines.append("- Findings: {}".format(len(findings))) + for index, finding in enumerate(findings[:10], start=1): + location = finding.get("file") or "unknown file" + if finding.get("line") is not None: + location = "{}:{}".format(location, finding.get("line")) + lines.append( + " {}. `{}` {} - {}".format( + index, + finding.get("severity") or "unknown", + location, + finding.get("title") or "Untitled finding", + ) + ) + if len(findings) > 10: + lines.append("- Findings truncated after 10 entries.") + + no_findings_reason = review.get("no_findings_reason") + if no_findings_reason: + lines.append("- No-findings reason: {}".format(no_findings_reason)) + + tests_run = review.get("tests_run") or [] + lines.append("- Tests reported by runtime: {}".format(len(tests_run))) + for test in tests_run[:10]: + summary = test.get("output_summary") + suffix = " - {}".format(summary) if summary else "" + lines.append( + " - `{}`: `{}`{}".format( + test.get("command") or "unknown command", + test.get("status") or "unknown", + suffix, + ) + ) + if len(tests_run) > 10: + lines.append("- Test list truncated after 10 entries.") + + limitations = review.get("limitations") or [] + lines.append("- Limitations: {}".format(len(limitations))) + for limitation in limitations[:10]: + lines.append(" - {}".format(limitation)) + if len(limitations) > 10: + lines.append("- Limitation list truncated after 10 entries.") + + return lines + + +def load_codex_access_token(): + for path in codex_token_candidates(): + try: + data = read_json(path, {}) + token = str(data.get("access_token") or "").strip() + if token: + return token + except Exception: + continue + return None + + +def codex_token_candidates(): + yield CODEX_TOKENS_PATH + + coven_home = CODEX_TOKENS_PATH.parent + registry = read_json(coven_home / "accounts.json", {}) + active = ( + registry.get("providers", {}) + .get("codex", {}) + .get("active") + ) + if active: + yield coven_home / "accounts" / "codex" / str(active) / "codex_tokens.json" + + accounts_root = coven_home / "accounts" / "codex" + if accounts_root.exists(): + for path in accounts_root.glob("*/codex_tokens.json"): + yield path + + +def redacted_command_result(result): + redacted = dict(result) + for key in ("stdout", "stderr"): + redacted[key] = redact_tokenish(redacted.get(key) or "") + return redacted + + +def redact_tokenish(text): + if not text: + return text + markers = ["ghs_", "ghu_", "github_pat_", "x-access-token:"] + redacted = text + for marker in markers: + while marker in redacted: + index = redacted.find(marker) + end = index + len(marker) + while end < len(redacted) and redacted[end] not in " \n\r\t'\"": + end += 1 + redacted = redacted[:index] + marker + "[redacted]" + redacted[end:] + return redacted + + +def fail_task(path, task, reason, detail): + task["state"] = "failed" + task["failure_category"] = reason + task["failure_detail"] = redact_tokenish(str(detail))[-4000:] + task["updated_at"] = utc_now() + write_json_atomic(path, task) + return task diff --git a/deploy/coven-github/test_coven_github_adapter.py b/deploy/coven-github/test_coven_github_adapter.py new file mode 100644 index 0000000..a466c47 --- /dev/null +++ b/deploy/coven-github/test_coven_github_adapter.py @@ -0,0 +1,84 @@ +import importlib.util +import tempfile +import unittest +from pathlib import Path + + +def load_adapter(): + path = Path(__file__).with_name("coven_github_adapter.py") + spec = importlib.util.spec_from_file_location("coven_github_adapter", path) + module = importlib.util.module_from_spec(spec) + spec.loader.exec_module(module) + return module + + +class HostedAdapterTests(unittest.TestCase): + def test_mentions_are_boundary_aware(self): + adapter = load_adapter() + policy = {"bot_usernames": ["cody"]} + + for text in ("@cody please review", "Please review, @Cody.", "(@cody)"): + self.assertTrue(adapter.mentioned(text, policy), text) + + for text in ( + "@codybot please review", + "@cody_bot please review", + "@cody/team please review", + "email me at x@cody.example", + "prefix@cody", + ): + self.assertFalse(adapter.mentioned(text, policy), text) + + def test_route_signed_delivery_reports_missing_secret(self): + adapter = load_adapter() + result = adapter.route_signed_delivery( + {"x-hub-signature-256": "sha256=abc"}, + b"{}", + debug=True, + webhook_secret="", + ) + + self.assertEqual(result["status"], 500) + self.assertIn("GITHUB_WEBHOOK_SECRET", result["error"]) + + def test_prepare_review_context_rejects_stale_pr_head_evidence(self): + adapter = load_adapter() + + def fake_github_request(method, url, token, body=None): + if "/pulls/123/files" in url: + return [] + if "/pulls/123" in url: + return { + "number": 123, + "head": {"sha": "metadata-sha"}, + "base": {"sha": "base-sha"}, + } + raise AssertionError(url) + + def fake_run_command(args, cwd=None, env=None, timeout=300): + if args[:2] == ["git", "fetch"]: + return {"args": args, "returncode": 0, "stdout": "", "stderr": ""} + if args[:3] == ["git", "checkout", "--detach"]: + return {"args": args, "returncode": 0, "stdout": "", "stderr": ""} + if args[:3] == ["git", "rev-parse", "HEAD"]: + return { + "args": args, + "returncode": 0, + "stdout": "different-sha\n", + "stderr": "", + } + if args[:3] == ["git", "status", "--short"]: + return {"args": args, "returncode": 0, "stdout": "## HEAD\n", "stderr": ""} + raise AssertionError(args) + + adapter.github_request = fake_github_request + adapter.run_command = fake_run_command + + with tempfile.TemporaryDirectory() as tmp: + task = {"task": {"pr_number": 123}, "repository": "OpenCoven/coven-github"} + with self.assertRaisesRegex(RuntimeError, "does not match GitHub metadata head"): + adapter.prepare_review_context(task, Path(tmp), "tok", {}, Path(tmp)) + + +if __name__ == "__main__": + unittest.main() diff --git a/docs/contracts/result.example.json b/docs/contracts/result.example.json index 53fd6ac..7796d31 100644 --- a/docs/contracts/result.example.json +++ b/docs/contracts/result.example.json @@ -1,5 +1,5 @@ { - "contract_version": "1", + "contract_version": "2", "status": "success", "branch": "cody/fix-issue-42", "commits": [ @@ -7,6 +7,22 @@ ], "files_changed": ["src/auth/refresh.rs"], "summary": "Fixed OAuth token refresh by adding a 60-second clock skew buffer.", - "pr_body": "## Hey, I'm Cody 🦄\n\nIssue #42 traced to the OAuth refresh path not accounting for clock skew. I added a 60-second buffer to the expiry check.\n\n**Changed:** `src/auth/refresh.rs` (+12 / -3)\n**Tests:** 8/8 passing (added 2 regression cases)\n", + "pr_body": "## Hey, I'm Cody\n\nIssue #42 traced to the OAuth refresh path not accounting for clock skew. I added a 60-second buffer to the expiry check.\n\n**Changed:** `src/auth/refresh.rs` (+12 / -3)\n**Tests:** 8/8 passing (added 2 regression cases)\n", + "review": { + "mode": "none", + "evidence_status": "not_applicable", + "reviewed_files": [], + "supporting_files": [], + "findings": [], + "tests_run": [ + { + "command": "cargo test -p auth refresh", + "status": "passed", + "output_summary": "8/8 passing" + } + ], + "no_findings_reason": null, + "limitations": [] + }, "exit_reason": null } diff --git a/docs/contracts/result.schema.json b/docs/contracts/result.schema.json index 0a86c4c..4cae129 100644 --- a/docs/contracts/result.schema.json +++ b/docs/contracts/result.schema.json @@ -2,14 +2,14 @@ "$schema": "https://json-schema.org/draft/2020-12/schema", "$id": "https://github.com/OpenCoven/coven-github/docs/contracts/result.schema.json", "title": "coven-code headless result envelope", - "description": "Terminal task state coven-code --headless writes to --output. Contract version 1.", + "description": "Terminal task state coven-code --headless writes to --output. Contract version 2.", "type": "object", "additionalProperties": false, - "required": ["status", "commits", "files_changed", "summary", "pr_body"], + "required": ["contract_version", "status", "commits", "files_changed", "summary", "pr_body", "review"], "properties": { "contract_version": { "type": "string", - "const": "1" + "const": "2" }, "status": { "type": "string", @@ -36,9 +36,140 @@ }, "summary": { "type": "string" }, "pr_body": { "type": "string" }, + "review": { + "type": "object", + "additionalProperties": false, + "required": [ + "mode", + "evidence_status", + "reviewed_files", + "supporting_files", + "findings", + "tests_run", + "no_findings_reason", + "limitations" + ], + "properties": { + "mode": { + "type": "string", + "enum": ["none", "pull_request", "review_comment"] + }, + "evidence_status": { + "type": "string", + "enum": ["not_applicable", "complete", "partial", "missing"] + }, + "reviewed_files": { + "type": "array", + "items": { "type": "string" } + }, + "supporting_files": { + "type": "array", + "items": { "type": "string" } + }, + "findings": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["severity", "file", "line", "title", "body", "recommendation"], + "properties": { + "severity": { + "type": "string", + "enum": ["info", "low", "medium", "high", "critical"] + }, + "file": { "type": "string" }, + "line": { "type": ["integer", "null"], "minimum": 1 }, + "title": { "type": "string" }, + "body": { "type": "string" }, + "recommendation": { "type": ["string", "null"] } + } + } + }, + "tests_run": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["command", "status", "output_summary"], + "properties": { + "command": { "type": "string" }, + "status": { + "type": "string", + "enum": ["passed", "failed", "not_run", "unknown"] + }, + "output_summary": { "type": ["string", "null"] } + } + } + }, + "no_findings_reason": { "type": ["string", "null"] }, + "limitations": { + "type": "array", + "items": { "type": "string" } + } + }, + "allOf": [ + { + "if": { + "properties": { "mode": { "enum": ["pull_request", "review_comment"] } }, + "required": ["mode"] + }, + "then": { + "properties": { + "evidence_status": { + "enum": ["complete", "partial", "missing"] + } + }, + "anyOf": [ + { "properties": { "reviewed_files": { "minItems": 1 } }, "required": ["reviewed_files"] }, + { "properties": { "evidence_status": { "const": "missing" } }, "required": ["evidence_status"] } + ] + } + }, + { + "if": { + "properties": { "mode": { "const": "none" } }, + "required": ["mode"] + }, + "then": { + "properties": { + "evidence_status": { "const": "not_applicable" } + }, + "required": ["evidence_status"] + } + } + ] + }, "exit_reason": { "type": ["string", "null"], "enum": ["test_failure", "ambiguous_spec", "git_conflict", "infra_error", null] } - } + }, + "allOf": [ + { + "if": { + "properties": { "status": { "const": "success" } }, + "required": ["status"] + }, + "then": { + "properties": { + "exit_reason": { "const": null } + } + } + }, + { + "if": { + "properties": { "status": { "enum": ["failure", "needs_input"] } }, + "required": ["status"] + }, + "then": { + "properties": { + "exit_reason": { + "type": "string", + "enum": ["test_failure", "ambiguous_spec", "git_conflict", "infra_error"] + } + }, + "required": ["exit_reason"] + } + } + ] } diff --git a/docs/contracts/session-brief.example.json b/docs/contracts/session-brief.example.json index 8ecbc9a..92719bd 100644 --- a/docs/contracts/session-brief.example.json +++ b/docs/contracts/session-brief.example.json @@ -1,5 +1,5 @@ { - "contract_version": "1", + "contract_version": "2", "trigger": "issue_assigned", "repo": { "owner": "OpenCoven", diff --git a/docs/contracts/session-brief.schema.json b/docs/contracts/session-brief.schema.json index 613a352..60d7625 100644 --- a/docs/contracts/session-brief.schema.json +++ b/docs/contracts/session-brief.schema.json @@ -2,14 +2,14 @@ "$schema": "https://json-schema.org/draft/2020-12/schema", "$id": "https://github.com/OpenCoven/coven-github/docs/contracts/session-brief.schema.json", "title": "coven-code headless session brief", - "description": "Tokenless read-context payload the adapter writes for coven-code --headless --context. Contract version 1.", + "description": "Tokenless read-context payload the adapter writes for coven-code --headless --context. Contract version 2.", "type": "object", "additionalProperties": false, - "required": ["trigger", "repo", "task", "familiar", "workspace"], + "required": ["contract_version", "trigger", "repo", "task", "familiar", "workspace"], "properties": { "contract_version": { "type": "string", - "const": "1" + "const": "2" }, "trigger": { "type": "string", @@ -84,6 +84,33 @@ "properties": { "root": { "type": "string" } } + }, + "audit_instruction": { + "type": ["string", "null"], + "description": "Optional hosted-review instruction paired with review_context." + }, + "review_context": { + "type": ["object", "null"], + "description": "Optional tokenless hosted-review evidence supplied by the adapter.", + "additionalProperties": false, + "required": ["kind", "files"], + "properties": { + "kind": { + "type": ["string", "null"], + "enum": ["pull_request", null] + }, + "files": { + "type": "array", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["filename"], + "properties": { + "filename": { "type": "string" } + } + } + } + } } } } diff --git a/docs/headless-contract.md b/docs/headless-contract.md index f71eb54..3be3dd6 100644 --- a/docs/headless-contract.md +++ b/docs/headless-contract.md @@ -1,6 +1,6 @@ # coven-code Headless Execution Contract -**Contract version: `1`** · Status: **Locked** (V1 / M1) +**Contract version: `2`** - Status: **Locked** (V2 / structured review output) This document is the single source of truth for the interface between `coven-github` (the GitHub ingress adapter) and `coven-code` (the execution @@ -64,7 +64,7 @@ The adapter is the **producer**; the runtime is the **consumer**. The brief is ```json { - "contract_version": "1", + "contract_version": "2", "trigger": "issue_assigned", "repo": { "owner": "OpenCoven", @@ -86,7 +86,9 @@ The adapter is the **producer**; the runtime is the **consumer**. The brief is }, "workspace": { "root": "/tmp/task-abc123" - } + }, + "review_context": null, + "audit_instruction": null } ``` @@ -94,7 +96,7 @@ The adapter is the **producer**; the runtime is the **consumer**. The brief is | Field | Type | Notes | |---|---|---| -| `contract_version` | string | MUST be `"1"`. Consumers MUST reject a brief whose major version they do not implement. | +| `contract_version` | string | MUST be `"2"`. Consumers MUST reject a brief whose major version they do not implement. | | `trigger` | string enum | `issue_assigned` \| `pr_review_comment` \| `issue_mention`. | | `repo.owner` | string | | | `repo.name` | string | | @@ -106,6 +108,8 @@ The adapter is the **producer**; the runtime is the **consumer**. The brief is | `familiar.model` | string \| null | BYOM model id; `null`/absent means runtime default. | | `familiar.skills` | string[] | Skill ids to load for the session. MAY be empty. | | `workspace.root` | string | Absolute path to the pre-cloned, isolated workspace. The runtime operates **inside** this directory and MUST NOT write outside it. | +| `review_context` | object \| null | Optional tokenless hosted-review evidence supplied by the adapter. `kind: "pull_request"` puts the runtime in PR review mode and carries changed-file metadata. | +| `audit_instruction` | string \| null | Optional adapter-authored review instruction appended to the review prompt. | ### 2.2 Task kinds @@ -128,7 +132,7 @@ the file MAY be absent. ```json { - "contract_version": "1", + "contract_version": "2", "status": "success", "branch": "cody/fix-issue-42", "commits": [ @@ -136,7 +140,17 @@ the file MAY be absent. ], "files_changed": ["src/auth/refresh.rs"], "summary": "Fixed OAuth token refresh by adding a 60-second clock skew buffer.", - "pr_body": "## Hey, I'm Cody 🦄\n\nI looked at issue #42…", + "pr_body": "## Hey, I'm Cody\n\nI looked at issue #42...", + "review": { + "mode": "pull_request", + "evidence_status": "complete", + "reviewed_files": ["src/auth/refresh.rs"], + "supporting_files": ["src/auth/mod.rs", "tests/auth_refresh.rs"], + "findings": [], + "tests_run": [], + "no_findings_reason": "Reviewed the supplied PR file and found no blocking issues.", + "limitations": [] + }, "exit_reason": null } ``` @@ -145,34 +159,56 @@ the file MAY be absent. | Field | Type | Notes | |---|---|---| -| `contract_version` | string | MUST be `"1"`. If absent, the consumer assumes `"1"` for backward compatibility, but producers MUST emit it. | -| `status` | string enum | `success` \| `failure` \| `partial` \| `needs_input`. See [3.2](#32-status). | +| `contract_version` | string | MUST be `"2"`. Producers MUST emit it. | +| `status` | string enum | `success` \| `failure` \| `partial` \| `needs_input`. See [3.3](#33-status). | | `branch` | string \| null | Branch the runtime pushed. `null` when no branch was created. The adapter only opens a PR when `branch` is set **and** `commits` is non-empty. | | `commits` | array | `{ "sha": string, "message": string }`. MAY be empty. | | `files_changed` | string[] | Workspace-relative paths. MAY be empty. | | `summary` | string | One-line familiar-voice summary. Used in the Check Run and PR title. | | `pr_body` | string | Full PR body, **authored by the familiar** in its own voice — not a template. | -| `exit_reason` | string enum \| null | `null` on success; otherwise the terminal cause. See [3.3](#33-exit_reason). | +| `review` | object | Structured review evidence and findings. Required even when `mode` is `none`. See [3.2](#32-review). | +| `exit_reason` | string enum \| null | `null` on success; otherwise the terminal cause. See [3.4](#34-exit_reason). | > **Drift note (supersedes `COVEN-GITHUB.md`):** the prose result envelope listed -> an `events` array. Progress/event streaming is **not** part of the v1 result +> an `events` array. Progress/event streaming is **not** part of the v2 result > envelope — it is deferred to M2 and will travel over a separate channel. The -> v1 envelope carries terminal task state only. Producers MUST NOT rely on +> v2 envelope carries terminal task state only. Producers MUST NOT rely on > `events` being read. -### 3.2 `status` +### 3.2 `review` + +`review` is the machine-readable proof that a hosted review actually examined +the intended code. It is required on every result. Non-review tasks MUST set +`mode: "none"` and `evidence_status: "not_applicable"`. + +| Field | Type | Notes | +|---|---|---| +| `mode` | string enum | `none`, `pull_request`, or `review_comment`. | +| `evidence_status` | string enum | `not_applicable`, `complete`, `partial`, or `missing`. PR review modes MUST NOT use `not_applicable`. | +| `reviewed_files` | string[] | Workspace-relative files supplied to or inspected by the runtime. PR review modes MUST include at least one file unless `evidence_status` is `missing`. | +| `supporting_files` | string[] | Workspace-relative files beyond the changed-file list that the runtime can prove were inspected for context. Empty means no broader-codebase inspection was proven, not that none was needed. | +| `findings` | array | Structured findings. Empty is allowed. For complete no-finding reviews, `no_findings_reason` SHOULD explain the clean outcome. | +| `tests_run` | array | Commands run while reviewing, with `passed`, `failed`, `not_run`, or `unknown` status. | +| `no_findings_reason` | string \| null | File-backed explanation for a clean review. MAY be `null` for degraded/partial output when `evidence_status` and `limitations` explain why a substantive clean-review conclusion was not possible. | +| `limitations` | string[] | Evidence gaps, skipped checks, or other caveats. | + +Each finding carries required `severity`, `file`, `line`, `title`, `body`, and +`recommendation` fields. `line` and `recommendation` may be `null`. Valid +severities are `info`, `low`, `medium`, `high`, and `critical`. + +### 3.3 `status` | Value | Meaning | |---|---| | `success` | Work complete; commits made; ready for a PR. | -| `partial` | Some progress committed but the task is not fully done (e.g. tests still failing after the retry budget). The adapter still opens a PR if there are commits. | +| `partial` | Progress or review evidence is incomplete but usable. `exit_reason` MAY be `null` when the result is partial because of review-evidence limitations rather than a terminal error. The adapter still opens a PR if there are commits. | | `failure` | The agent gave up; no usable result. | | `needs_input` | The agent needs human clarification and has posted (or expects the adapter to surface) a question. Pairs with exit code `3`. | The adapter treats `success` and `partial` as PR-opening outcomes; `failure` and `needs_input` do not open a PR by themselves. -### 3.3 `exit_reason` +### 3.4 `exit_reason` `null` on success. Otherwise one of: @@ -211,7 +247,7 @@ A process **killed by signal**, or one that **times out** (the adapter enforces ## 5. Security invariants -These are non-negotiable for v1: +These are non-negotiable for v2: 1. The session brief is **tokenless**. Serializing a brief MUST NOT produce an `auth` field, a `"token"` field, or a credential-bearing `clone_url`