diff --git a/docs/advanced.md b/docs/advanced.md index b683c76..f58ec86 100644 --- a/docs/advanced.md +++ b/docs/advanced.md @@ -571,6 +571,9 @@ policy settings such as `hostedReview.allowManagedRules`, `hostedReview.allowWriteTools`, `hostedReview.allowMcpServers`, and `hostedReview.allowPlugins` can opt specific surfaces back in for controlled deployments. Prefer tenant-approved managed rules over `allowUserMemory`. +Automated GitHub repair should use the dedicated `--hosted-repair` headless +flag, which exposes only repository file tools and never shell, web, plugin, +MCP, task, or sub-agent tools. Session memory extraction is also approval-gated in hosted review mode. Untrusted fork or contributor sessions cannot automatically append learned diff --git a/docs/configuration.md b/docs/configuration.md index 04f52f0..fa9d3e3 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -153,6 +153,14 @@ shared surfaces back in: } ``` +The GitHub worker's `--hosted-repair` mode is narrower than +`allowWriteTools`: it enables only `Read`, `Grep`, `Glob`, `Edit`, `Write`, +`ApplyPatch`, `BatchEdit`, and `NotebookEdit`. It requires the complete +`--headless --context ... --output ...` invocation and still disables command, +network-tool, sub-agent, plugin, MCP, user-memory, and automatic-memory access. +Use this worker-controlled mode for automated branch repair; do not enable the +broader `allowWriteTools` setting for contributor-controlled reviews. + `allowUserMemory` also exists for explicitly trusted deployments, but hosted review jobs should prefer tenant-approved managed rules over operator-global user memory. diff --git a/docs/headless-contract.md b/docs/headless-contract.md index 46edf16..cba95c6 100644 --- a/docs/headless-contract.md +++ b/docs/headless-contract.md @@ -2,6 +2,10 @@ **Contract version: `3`** - Status: **Locked** (V3 / memory provenance in review artifacts) +The runtime also accepts version `2` briefs during the coven-github migration +and echoes `"2"` in their result envelopes. Version `3` remains the normative +schema; all other major versions are rejected. + This document is the single source of truth for the interface between `coven-github` (the GitHub ingress adapter) and `coven-code` (the execution runtime) when the runtime is invoked in headless mode. @@ -31,6 +35,10 @@ The adapter spawns the runtime as a child process: coven-code --headless --context --output ``` +Trusted branch-repair workers add `--hosted-repair`. That flag requires all +three structured headless arguments and exposes only repository file tools; +the control plane, not the runtime model, performs validation, commit, and push. + | Flag | Meaning | |---|---| | `--headless` | Disables the ratatui TUI entirely. All human-facing output is suppressed; the process is non-interactive and reads no stdin. | diff --git a/src-rust/crates/cli/src/headless.rs b/src-rust/crates/cli/src/headless.rs index a687c03..faece05 100644 --- a/src-rust/crates/cli/src/headless.rs +++ b/src-rust/crates/cli/src/headless.rs @@ -34,6 +34,7 @@ use std::path::{Path, PathBuf}; /// Major contract version this build implements (contract §6). pub const CONTRACT_VERSION: &str = "3"; +pub const COMPATIBLE_CONTRACT_VERSIONS: &[&str] = &["2", "3"]; /// Environment variable carrying the GitHub App **installation access token** /// used to authenticate `git push`. This is the ONLY git credential channel; it @@ -150,11 +151,11 @@ impl SessionBrief { /// "Consumers MUST reject a payload whose major version they do not /// implement, rather than silently mis-parsing it." pub fn ensure_supported_version(&self) -> anyhow::Result<()> { - if self.contract_version != CONTRACT_VERSION { + if !COMPATIBLE_CONTRACT_VERSIONS.contains(&self.contract_version.as_str()) { bail!( "unsupported headless contract version {:?}; this build implements {:?}", self.contract_version, - CONTRACT_VERSION + COMPATIBLE_CONTRACT_VERSIONS ); } Ok(()) @@ -195,6 +196,17 @@ impl SessionBrief { /// Build the first-turn user prompt injected into the headless session. pub fn to_prompt(&self) -> String { + self.to_prompt_for_mode(false) + } + + /// Build the first-turn prompt for a trusted hosted repair. The runtime may + /// edit repository files, while the worker remains responsible for trusted + /// validation, committing, and pushing. + pub fn to_hosted_repair_prompt(&self) -> String { + self.to_prompt_for_mode(true) + } + + fn to_prompt_for_mode(&self, hosted_repair: bool) -> String { let mut lines = vec![ format!( "You are {}, the Coven coding familiar assigned to {}/{} through the coven-github App.", @@ -268,6 +280,23 @@ impl SessionBrief { lines.push(instruction.trim().to_string()); } + if hosted_repair { + lines.push(String::new()); + lines.push( + "Hosted repair mode: inspect the cited findings and make only the repository file edits needed to fix them. Use the available file read and edit tools directly in the current workspace. Do not merely describe a patch." + .to_string(), + ); + lines.push( + "Command execution, network access, git operations, and test execution are intentionally unavailable. Do not create a branch, commit, or push. The trusted worker will validate, commit, and push the edited workspace after this runtime exits." + .to_string(), + ); + lines.push( + "If no safe source-backed edit can be made, explain the blocker. Otherwise, finish with a concise summary of the files you actually changed." + .to_string(), + ); + return lines.join("\n"); + } + if self.review_mode() != ReviewMode::None { lines.push(String::new()); lines.push( @@ -331,6 +360,7 @@ pub fn apply_to_config(config: &mut claurst_core::config::Config, brief: &Sessio config.hosted_review.enabled = true; config.hosted_review.allow_user_memory = false; config.hosted_review.allow_write_tools = false; + config.hosted_review.allow_file_write_tools = false; config.hosted_review.allow_mcp_servers = false; config.hosted_review.allow_plugins = false; config.hosted_review.allow_auto_memory_persistence = false; @@ -418,6 +448,7 @@ pub struct GitSummary { pub branch: Option, pub commits: Vec, pub files_changed: Vec, + pub workspace_dirty: bool, } /// Inspect the workspace and summarize the branch, the commits ahead of the base @@ -448,6 +479,8 @@ pub fn collect_git_summary(workspace: &Path, default_branch: &str) -> GitSummary }) .unwrap_or_default(); + let workspace_dirty = git_stdout(workspace, &["diff", "--name-only", "HEAD"]).is_some() + || git_stdout(workspace, &["diff", "--name-only", "--cached"]).is_some(); let files_changed = git_stdout(workspace, &["diff", "--name-only", "HEAD"]) .into_iter() .chain(git_stdout(workspace, &["diff", "--name-only", "--cached"])) @@ -470,6 +503,7 @@ pub fn collect_git_summary(workspace: &Path, default_branch: &str) -> GitSummary branch, commits, files_changed, + workspace_dirty, } } @@ -1467,6 +1501,7 @@ fn build_result( final_text, review_trace, ReviewMemoryUse::default(), + false, ) } @@ -1479,10 +1514,11 @@ pub fn build_result_with_memory( final_text: &str, review_trace: Option<&ReviewTrace>, memory: ReviewMemoryUse, + hosted_repair: bool, ) -> (ResultEnvelope, i32) { - let comment_only = brief.map(SessionBrief::is_comment_only).unwrap_or(false); - let (mut status, mut exit_reason, code) = - classify(outcome, !git.commits.is_empty(), comment_only); + let comment_only = !hosted_repair && brief.map(SessionBrief::is_comment_only).unwrap_or(false); + let has_progress = !git.commits.is_empty() || (hosted_repair && git.workspace_dirty); + let (mut status, mut exit_reason, code) = classify(outcome, has_progress, comment_only); let review = ReviewResult::from_brief_with_memory(brief, review_trace, final_text, memory); if review.mode != ReviewMode::None @@ -1496,7 +1532,9 @@ pub fn build_result_with_memory( let pr_body = compose_pr_body(brief, final_text, git, status); let envelope = ResultEnvelope { - contract_version: CONTRACT_VERSION.to_string(), + contract_version: brief + .map(|value| value.contract_version.clone()) + .unwrap_or_else(|| CONTRACT_VERSION.to_string()), status, branch: git.branch.clone(), commits: git.commits.clone(), @@ -1518,7 +1556,9 @@ pub fn infra_error_result( ) -> (ResultEnvelope, i32) { let name = familiar_name(brief); let envelope = ResultEnvelope { - contract_version: CONTRACT_VERSION.to_string(), + contract_version: brief + .map(|value| value.contract_version.clone()) + .unwrap_or_else(|| CONTRACT_VERSION.to_string()), status: Status::Failure, branch: git.branch.clone(), commits: git.commits.clone(), @@ -1678,6 +1718,19 @@ mod tests { serde_json::from_str(raw).expect("review brief parses") } + fn sample_repair_brief() -> SessionBrief { + let raw = r#"{ + "contract_version": "2", + "trigger": "pull_request_autoreview", + "repo": { "owner": "o", "name": "r", "clone_url": "https://github.com/o/r.git", "default_branch": "main" }, + "task": { "kind": "respond_to_mention", "issue_number": 7, "comment_body": "Fix the source-backed finding." }, + "familiar": { "id": "cody", "display_name": "Cody", "skills": [] }, + "workspace": { "root": "/tmp/ws" }, + "audit_instruction": "Edit src/lib.rs to fix the finding." + }"#; + serde_json::from_str(raw).expect("repair brief parses") + } + fn review_workspace() -> (tempfile::TempDir, ReviewTrace) { let dir = tempfile::tempdir().unwrap(); let ws = dir.path().to_path_buf(); @@ -1793,6 +1846,7 @@ mod tests { "## Findings\n- [low] src/lib.rs:1 — fine\n\n## Supporting Context Used\n- src/support.rs: checked", Some(&trace), memory, + false, ); let value = serde_json::to_value(&envelope).unwrap(); @@ -1916,6 +1970,33 @@ mod tests { ); } + #[test] + fn contract_v2_pull_request_review_remains_supported_end_to_end() { + let raw = r#"{ + "contract_version": "2", + "trigger": "pull_request_autoreview", + "repo": { "owner": "OpenCoven", "name": "example", "clone_url": "https://github.com/OpenCoven/example.git", "default_branch": "main" }, + "task": { "kind": "respond_to_mention", "issue_number": 73, "comment_body": "Review pull request #73 at the captured head." }, + "familiar": { "id": "covencat", "display_name": "Covencat", "skills": [] }, + "workspace": { "root": "/workspace" }, + "review_context": { "kind": "pull_request", "files": [{ "filename": "src/app.ts" }] } + }"#; + let brief: SessionBrief = serde_json::from_str(raw).expect("v2 review brief parses"); + brief + .ensure_supported_version() + .expect("v2 remains supported"); + assert_eq!(brief.review_mode(), ReviewMode::PullRequest); + assert!(brief.to_prompt().contains("Review pull request #73")); + let (envelope, _) = build_result( + Some(&brief), + &GitSummary::default(), + RunOutcome::Completed, + "### Files inspected\n- `src/app.ts`\n### Supporting context used\n- `src/app.ts` - reviewed source\n### Findings\nNone\n### No-findings justification\nNo defect was found in `src/app.ts`.\n### Tests/commands considered\n- `npm test` - not run: host validates\n### Confidence/limitations\nHost validation is pending.", + None, + ); + assert_eq!(envelope.contract_version, "2"); + } + #[test] fn prompt_is_derived_from_task_and_never_leaks_a_token() { let brief = sample_brief(); @@ -1928,6 +2009,16 @@ mod tests { assert!(!prompt.contains("x-access-token:")); } + #[test] + fn hosted_repair_prompt_requires_real_edits_and_delegates_privileged_steps() { + let prompt = sample_brief().to_hosted_repair_prompt(); + + assert!(prompt.contains("Do not merely describe a patch")); + assert!(prompt.contains("current workspace")); + assert!(prompt.contains("trusted worker will validate, commit, and push")); + assert!(!prompt.contains("make the change on a new branch")); + } + #[test] fn review_prompt_requires_structured_review_sections() { let prompt = sample_review_brief().to_prompt(); @@ -1953,6 +2044,7 @@ mod tests { let mut config = claurst_core::config::Config { hosted_review: claurst_core::hosted_review::HostedReviewConfig { allow_write_tools: true, + allow_file_write_tools: true, allow_mcp_servers: true, allow_plugins: true, allow_user_memory: true, @@ -1966,6 +2058,7 @@ mod tests { assert!(config.hosted_review.enabled); assert!(!config.hosted_review.allow_write_tools); + assert!(!config.hosted_review.allow_file_write_tools); assert!(!config.hosted_review.allow_mcp_servers); assert!(!config.hosted_review.allow_plugins); assert!(!config.hosted_review.allow_user_memory); @@ -2065,6 +2158,7 @@ mod tests { message: "Add clock-skew buffer".to_string(), }], files_changed: vec!["src/auth/refresh.rs".to_string()], + workspace_dirty: false, }; let (env, code) = build_result( Some(&sample_brief()), @@ -2417,6 +2511,52 @@ N/A } } + #[test] + fn hosted_repair_requires_a_workspace_edit() { + let brief = sample_repair_brief(); + let git = GitSummary { + files_changed: vec!["src/lib.rs".to_string()], + workspace_dirty: false, + ..Default::default() + }; + let (env, code) = build_result_with_memory( + Some(&brief), + &git, + RunOutcome::Completed, + "No changes were necessary.", + None, + ReviewMemoryUse::default(), + true, + ); + + assert_eq!(code, 1); + assert_eq!(env.status, Status::Failure); + assert_eq!(env.exit_reason, Some(ExitReason::AmbiguousSpec)); + } + + #[test] + fn hosted_repair_accepts_uncommitted_workspace_edits_for_worker_validation() { + let brief = sample_repair_brief(); + let git = GitSummary { + files_changed: vec!["src/lib.rs".to_string()], + workspace_dirty: true, + ..Default::default() + }; + let (env, code) = build_result_with_memory( + Some(&brief), + &git, + RunOutcome::Completed, + "Updated `src/lib.rs` to address the finding.", + None, + ReviewMemoryUse::default(), + true, + ); + + assert_eq!(code, 0); + assert_eq!(env.status, Status::Success); + assert!(env.exit_reason.is_none()); + } + #[test] fn address_review_comment_without_commits_is_successful_review_output() { let raw = r#"{ @@ -2457,6 +2597,7 @@ N/A message: "do the thing".to_string(), }], files_changed: vec!["a.rs".to_string()], + workspace_dirty: false, }; let (env, _) = build_result( Some(&sample_brief()), @@ -2481,6 +2622,7 @@ N/A message: "m".to_string(), }], files_changed: vec!["a.rs".to_string(), "b.rs".to_string()], + workspace_dirty: false, }; let (env, _) = build_result( Some(&sample_brief()), @@ -2541,6 +2683,12 @@ N/A assert_eq!(summary.commits.len(), 1, "one commit ahead of main"); assert_eq!(summary.commits[0].message, "add feature"); assert!(summary.files_changed.iter().any(|f| f == "feature.rs")); + assert!(!summary.workspace_dirty); + + std::fs::write(ws.join("base.txt"), "edited").unwrap(); + let dirty = collect_git_summary(ws, "main"); + assert!(dirty.workspace_dirty); + assert!(dirty.files_changed.iter().any(|f| f == "base.txt")); } #[test] diff --git a/src-rust/crates/cli/src/main.rs b/src-rust/crates/cli/src/main.rs index 96acf08..47635b5 100644 --- a/src-rust/crates/cli/src/main.rs +++ b/src-rust/crates/cli/src/main.rs @@ -255,6 +255,17 @@ struct Cli { #[arg(long = "hosted-review", env = "COVEN_CODE_HOSTED_REVIEW", action = ArgAction::SetTrue)] hosted_review: bool, + /// Hosted repair mode: allow repository file edits but no command execution, + /// plugins, MCP servers, sub-agents, or network tools. + #[arg( + long = "hosted-repair", + env = "COVEN_CODE_HOSTED_REPAIR", + action = ArgAction::SetTrue, + requires_all = ["headless", "context", "output"], + conflicts_with_all = ["hosted_review", "dangerously_skip_permissions"] + )] + hosted_repair: bool, + /// Billing workload tag #[arg(long = "workload", value_name = "TAG")] workload: Option, @@ -319,6 +330,17 @@ impl From for PermissionMode { } } +fn permission_mode_for_invocation( + cli_mode: CliPermissionMode, + has_github_context: bool, +) -> PermissionMode { + if has_github_context { + PermissionMode::BypassPermissions + } else { + cli_mode.into() + } +} + #[derive(Copy, Clone, Debug, PartialEq, Eq, ValueEnum)] enum CliOutputFormat { Text, @@ -570,6 +592,15 @@ async fn main() -> anyhow::Result<()> { if let Some(brief) = github_context.as_ref() { headless::apply_to_config(&mut config, brief); } + if cli.hosted_repair { + config.hosted_review.enabled = true; + config.hosted_review.allow_file_write_tools = true; + config.hosted_review.allow_write_tools = false; + config.hosted_review.allow_user_memory = false; + config.hosted_review.allow_mcp_servers = false; + config.hosted_review.allow_plugins = false; + config.hosted_review.allow_auto_memory_persistence = false; + } if cli.dangerously_skip_permissions { // Mirror TS setup.ts: block bypass mode when running as root/sudo. #[cfg(unix)] @@ -580,7 +611,8 @@ async fn main() -> anyhow::Result<()> { } config.permission_mode = PermissionMode::BypassPermissions; } else { - config.permission_mode = cli.permission_mode.into(); + config.permission_mode = + permission_mode_for_invocation(cli.permission_mode, github_context.is_some()); } config.additional_dirs = cli.add_dir.clone(); if cli.no_auto_compact { @@ -965,6 +997,7 @@ async fn main() -> anyhow::Result<()> { &r.final_text, Some(&r.review_trace), review_memory.clone().unwrap_or_default(), + cli.hosted_repair, ), Err(e) => headless::infra_error_result( github_context.as_ref(), @@ -1582,6 +1615,31 @@ fn filter_tools_for_hosted_review( return tools; } + if config.hosted_review.allow_file_write_tools { + // SECURITY (load-bearing): github_context forces BypassPermissions, so + // hosted-repair mode has NO permission prompts. This allowlist is the + // sole containment boundary — it must expose only repository file tools + // and never command/network/task/sub-agent/plugin/MCP surfaces. The + // exhaustive test `hosted_repair_allows_only_repository_file_tools` + // (with its catch-all assertion) guards against a new tool silently + // leaking into this set. Do not widen without updating that test. + const FILE_TOOLS: &[&str] = &[ + "Read", + "Grep", + "Glob", + "Edit", + "Write", + "ApplyPatch", + "BatchEdit", + "NotebookEdit", + ]; + let filtered = claurst_tools::all_tools() + .into_iter() + .filter(|tool| FILE_TOOLS.contains(&tool.name())) + .collect(); + return Arc::new(filtered); + } + filter_read_only_tools(&tools) } @@ -1654,7 +1712,11 @@ async fn run_headless( let prompt = if let Some(ref p) = cli.prompt { p.clone() } else if let Some(brief) = github_context { - brief.to_prompt() + if cli.hosted_repair { + brief.to_hosted_repair_prompt() + } else { + brief.to_prompt() + } } else { use tokio::io::{self, AsyncReadExt}; let mut stdin = io::stdin(); @@ -5482,6 +5544,112 @@ mod tests { ); } + #[test] + fn hosted_repair_allows_only_repository_file_tools() { + let all = Arc::new(claurst_tools::all_tools()); + let config = Config { + hosted_review: claurst_core::hosted_review::HostedReviewConfig { + enabled: true, + allow_file_write_tools: true, + ..Default::default() + }, + ..Default::default() + }; + + let names = tool_names(&filter_tools_for_hosted_review(all, &config)); + for required in ["Read", "Grep", "Glob", "Edit", "Write", "ApplyPatch"] { + assert!( + names.contains(&required.to_string()), + "hosted repair missing {required}: {names:?}" + ); + } + for forbidden in [ + "Bash", + "PtyBash", + "PowerShell", + "WebSearch", + "WebFetch", + "Agent", + "TeamCreate", + "Skill", + "ToolSearch", + "ListMcpResources", + "ReadMcpResource", + ] { + assert!( + !names.contains(&forbidden.to_string()), + "hosted repair must not include {forbidden}: {names:?}" + ); + } + assert!( + names.iter().all(|name| [ + "Read", + "Grep", + "Glob", + "Edit", + "Write", + "ApplyPatch", + "BatchEdit", + "NotebookEdit", + ] + .contains(&name.as_str())), + "hosted repair exposed an unexpected tool: {names:?}" + ); + } + + #[test] + fn hosted_repair_cli_requires_the_structured_headless_contract() { + assert!(Cli::try_parse_from(["coven-code", "--hosted-repair"]).is_err()); + assert!(Cli::try_parse_from([ + "coven-code", + "--headless", + "--hosted-repair", + "--context", + "brief.json", + "--output", + "result.json", + ]) + .is_ok()); + assert!(Cli::try_parse_from([ + "coven-code", + "--headless", + "--hosted-review", + "--hosted-repair", + "--context", + "brief.json", + "--output", + "result.json", + ]) + .is_err()); + assert!(Cli::try_parse_from([ + "coven-code", + "--headless", + "--hosted-repair", + "--dangerously-skip-permissions", + "--context", + "brief.json", + "--output", + "result.json", + ]) + .is_err()); + } + + #[test] + fn github_context_forces_noninteractive_permission_bypass() { + assert_eq!( + permission_mode_for_invocation(CliPermissionMode::Default, true), + PermissionMode::BypassPermissions + ); + assert_eq!( + permission_mode_for_invocation(CliPermissionMode::Plan, true), + PermissionMode::BypassPermissions + ); + assert_eq!( + permission_mode_for_invocation(CliPermissionMode::AcceptEdits, false), + PermissionMode::AcceptEdits + ); + } + // NOTE: the coven-github headless-contract types + behavior moved to the // `headless` module; their conformance tests live in `headless.rs` // (`#[cfg(test)] mod tests`), pinned to the vendored golden fixtures. diff --git a/src-rust/crates/core/src/hosted_review.rs b/src-rust/crates/core/src/hosted_review.rs index ec9c421..7ee252c 100644 --- a/src-rust/crates/core/src/hosted_review.rs +++ b/src-rust/crates/core/src/hosted_review.rs @@ -30,6 +30,10 @@ pub struct HostedReviewConfig { pub allow_managed_rules: bool, #[serde(default, skip_serializing_if = "is_false")] pub allow_write_tools: bool, + /// Allow only repository file read/write tools. Unlike `allow_write_tools`, + /// this never admits command execution, network, task, plugin, or MCP tools. + #[serde(default, skip_serializing_if = "is_false")] + pub allow_file_write_tools: bool, #[serde(default, skip_serializing_if = "is_false")] pub allow_mcp_servers: bool, #[serde(default, skip_serializing_if = "is_false")] @@ -52,6 +56,7 @@ impl Default for HostedReviewConfig { allow_user_memory: false, allow_managed_rules: false, allow_write_tools: false, + allow_file_write_tools: false, allow_mcp_servers: false, allow_plugins: false, allow_auto_memory_persistence: false, @@ -67,6 +72,7 @@ impl HostedReviewConfig { && !self.allow_user_memory && !self.allow_managed_rules && !self.allow_write_tools + && !self.allow_file_write_tools && !self.allow_mcp_servers && !self.allow_plugins && !self.allow_auto_memory_persistence