diff --git a/docs/advanced.md b/docs/advanced.md index 5c0c5713..d91bd675 100644 --- a/docs/advanced.md +++ b/docs/advanced.md @@ -534,6 +534,13 @@ Durable hosted memory and transcript namespaces are separated under a hosted review path and require tenant scope plus a canonical repository identity before they can be resolved for persistence. +Hosted review mode also disables write/execute-capable tools, configured MCP +servers, plugins, user memory, and managed rules by default. Trusted hosted +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`. + --- ## Security and permissions diff --git a/docs/configuration.md b/docs/configuration.md index 7bbe5c9a..72c1f0fa 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -132,6 +132,28 @@ new session artifacts as hosted review artifacts, and requires a tenant plus canonical repository identity before resolving hosted durable memory paths. Local-personal mode remains the default and continues to load user memory. +Hosted review also disables write/execute-capable tools, configured MCP +servers, and plugins by default. A trusted hosted policy can opt individual +shared surfaces back in: + +```json +{ + "config": { + "hostedReview": { + "enabled": true, + "allowManagedRules": true, + "allowWriteTools": true, + "allowMcpServers": true, + "allowPlugins": true + } + } +} +``` + +`allowUserMemory` also exists for explicitly trusted deployments, but hosted +review jobs should prefer tenant-approved managed rules over operator-global +user memory. + ### Tool access | Key | Type | Default | Description | diff --git a/docs/shared/hosted-review-phase-1-pr.md b/docs/shared/hosted-review-phase-1-pr.md new file mode 100644 index 00000000..4a44b4c1 --- /dev/null +++ b/docs/shared/hosted-review-phase-1-pr.md @@ -0,0 +1,27 @@ +# Hosted Review Phase 1 PR Notes + +## Linked issues + +Fixes #97. +Fixes #100. + +## Summary + +- Adds trusted hosted-policy switches under `hostedReview` for user memory, managed rules, write tools, MCP servers, and plugins. +- Wires prompt memory loading through the effective hosted policy so hosted review excludes global user memory and managed rules by default. +- Keeps hosted review on read-only built-in tools by default and skips configured MCP/plugin loading unless explicitly allowed. +- Documents the local-personal versus hosted-review defaults and trusted opt-ins. + +## Test evidence + +- `cargo fmt --all` +- `cargo check --workspace` +- `cargo clippy --workspace --all-targets -- -D warnings` +- `cargo test -p claurst-core --lib hosted_review -- --nocapture` +- `cargo test -p claurst --bin coven-code hosted_review -- --nocapture` +- `cargo test --workspace` progressed through unit tests, then failed in `claurst --test acp_smoke` because Windows Application Control blocked spawning the freshly built `coven-code` test binary with OS error 4551. + +## Risk notes + +- Local mode remains the default and keeps existing user/managed memory behavior. +- Hosted deployments that need shared rules, write tools, MCP, or plugins must opt in explicitly through trusted hosted policy settings. diff --git a/src-rust/crates/cli/src/main.rs b/src-rust/crates/cli/src/main.rs index 016cbca6..10940fcc 100644 --- a/src-rust/crates/cli/src/main.rs +++ b/src-rust/crates/cli/src/main.rs @@ -594,9 +594,7 @@ async fn main() -> anyhow::Result<()> { if cli.dump_system_prompt { let ctx = ContextBuilder::new(cwd.clone()) .disable_claude_mds(config.disable_claude_mds) - .memory_load_options(claurst_core::claudemd::MemoryLoadOptions::from_mode( - config.runtime_mode(), - )); + .memory_load_options(config.memory_load_options()); let sys = ctx.build_system_context().await; let user = ctx.build_user_context().await; println!("{}\n\n{}", sys, user); @@ -606,9 +604,7 @@ async fn main() -> anyhow::Result<()> { // Build context let ctx_builder = ContextBuilder::new(cwd.clone()) .disable_claude_mds(config.disable_claude_mds) - .memory_load_options(claurst_core::claudemd::MemoryLoadOptions::from_mode( - config.runtime_mode(), - )); + .memory_load_options(config.memory_load_options()); let system_ctx = ctx_builder.build_system_context().await; let user_ctx = ctx_builder.build_user_context().await; @@ -777,28 +773,32 @@ async fn main() -> anyhow::Result<()> { // Load plugins and register any plugin-provided MCP servers into the // in-memory config (does not modify the settings file on disk). - let plugin_registry = claurst_plugins::load_plugins(&cwd, &[]).await; - { - let plugin_cmd_count = plugin_registry.all_command_defs().len(); - let plugin_hook_count = plugin_registry - .build_hook_registry() - .values() - .map(|v| v.len()) - .sum::(); - info!( - plugins = plugin_registry.enabled_count(), - commands = plugin_cmd_count, - hooks = plugin_hook_count, - "Plugins loaded" - ); + if config.hosted_review_enabled() && !config.hosted_review.allow_plugins { + info!("Plugins skipped because hosted review mode does not allow plugins"); + } else { + let plugin_registry = claurst_plugins::load_plugins(&cwd, &[]).await; + { + let plugin_cmd_count = plugin_registry.all_command_defs().len(); + let plugin_hook_count = plugin_registry + .build_hook_registry() + .values() + .map(|v| v.len()) + .sum::(); + info!( + plugins = plugin_registry.enabled_count(), + commands = plugin_cmd_count, + hooks = plugin_hook_count, + "Plugins loaded" + ); - // Register plugin MCP servers into the in-memory config so they are - // picked up by any subsequent MCP manager construction. - let existing_names: std::collections::HashSet = - config.mcp_servers.iter().map(|s| s.name.clone()).collect(); - for mcp_server in plugin_registry.all_mcp_servers() { - if !existing_names.contains(&mcp_server.name) { - config.mcp_servers.push(mcp_server); + // Register plugin MCP servers into the in-memory config so they are + // picked up by any subsequent MCP manager construction. + let existing_names: std::collections::HashSet = + config.mcp_servers.iter().map(|s| s.name.clone()).collect(); + for mcp_server in plugin_registry.all_mcp_servers() { + if !existing_names.contains(&mcp_server.name) { + config.mcp_servers.push(mcp_server); + } } } } @@ -865,6 +865,7 @@ async fn main() -> anyhow::Result<()> { } else { tools }; + let tools = filter_tools_for_hosted_review(tools, &config); // Spawn the background cron scheduler (fires cron tasks at scheduled times). // Cancelled automatically when the process exits since we use a shared token. @@ -985,6 +986,10 @@ async fn connect_mcp_manager_arc(config: &Config) -> Option>>, + config: &Config, +) -> Arc>> { + if !config.hosted_review_enabled() || config.hosted_review.allow_write_tools { + return tools; + } + + filter_read_only_tools(&tools) +} + fn filter_read_only_tools( tools: &[Box], ) -> Arc>> { @@ -3285,12 +3301,16 @@ async fn run_interactive( if let Some(turns) = def.max_turns { base_query_config.max_turns = turns; } - tools_arc = filter_tools_for_agent(all_tools_arc.clone(), &def.access); + tools_arc = filter_tools_for_hosted_review( + filter_tools_for_agent(all_tools_arc.clone(), &def.access), + &cmd_ctx.config, + ); } else { // "build" with no explicit definition = full access, no agent base_query_config.agent_name = None; base_query_config.agent_definition = None; - tools_arc = all_tools_arc.clone(); + tools_arc = + filter_tools_for_hosted_review(all_tools_arc.clone(), &cmd_ctx.config); } } if !app.is_streaming && app.messages.len() < messages.len() { @@ -4406,7 +4426,10 @@ async fn run_interactive( let new_mcp_manager = connect_mcp_manager_arc(&cmd_ctx.config).await; tool_ctx.mcp_manager = new_mcp_manager.clone(); app.mcp_manager = new_mcp_manager.clone(); - tools_arc = build_tools_with_mcp(new_mcp_manager.clone()); + tools_arc = filter_tools_for_hosted_review( + build_tools_with_mcp(new_mcp_manager.clone()), + &cmd_ctx.config, + ); if app.mcp_view.visible { app.refresh_mcp_view(); } @@ -4415,15 +4438,21 @@ async fn run_interactive( .as_ref() .map(|manager| manager.server_count()) .unwrap_or(0); - app.status_message = Some(if cmd_ctx.config.mcp_servers.is_empty() { - "No MCP servers configured.".to_string() - } else { - format!( - "Reconnected MCP runtime ({} connected server{}).", - connected, - if connected == 1 { "" } else { "s" } - ) - }); + app.status_message = Some( + if cmd_ctx.config.hosted_review_enabled() + && !cmd_ctx.config.hosted_review.allow_mcp_servers + { + "MCP servers are disabled in hosted review mode.".to_string() + } else if cmd_ctx.config.mcp_servers.is_empty() { + "No MCP servers configured.".to_string() + } else { + format!( + "Reconnected MCP runtime ({} connected server{}).", + connected, + if connected == 1 { "" } else { "s" } + ) + }, + ); } if app.should_exit { @@ -5208,6 +5237,47 @@ mod tests { } } + #[test] + fn hosted_review_filters_write_and_execute_tools_by_default() { + let all = Arc::new(claurst_tools::all_tools()); + let config = Config { + hosted_review: claurst_core::hosted_review::HostedReviewConfig { + enabled: true, + ..Default::default() + }, + ..Default::default() + }; + + let names = tool_names(&filter_tools_for_hosted_review(all, &config)); + + assert!(names.contains(&"Read".to_string())); + for forbidden in ["Bash", "Edit", "Write", "NotebookEdit", "ApplyPatch"] { + assert!( + !names.contains(&forbidden.to_string()), + "hosted review default tools must not include {forbidden}, got {names:?}" + ); + } + } + + #[test] + fn hosted_review_trusted_policy_can_keep_write_tools() { + let all = Arc::new(claurst_tools::all_tools()); + let before = tool_names(&all); + let config = Config { + hosted_review: claurst_core::hosted_review::HostedReviewConfig { + enabled: true, + allow_write_tools: true, + ..Default::default() + }, + ..Default::default() + }; + + assert_eq!( + tool_names(&filter_tools_for_hosted_review(all, &config)), + before + ); + } + // 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/claudemd.rs b/src-rust/crates/core/src/claudemd.rs index 0602310f..48b7f2d2 100644 --- a/src-rust/crates/core/src/claudemd.rs +++ b/src-rust/crates/core/src/claudemd.rs @@ -433,22 +433,67 @@ mod tests { let files = load_all_memory_files_with_options(project.path(), &MemoryLoadOptions::hosted_review()); + match original_test_home { + Some(value) => std::env::set_var("COVEN_CODE_TEST_HOME", value), + None => std::env::remove_var("COVEN_CODE_TEST_HOME"), + } match original_home { Some(value) => std::env::set_var("HOME", value), None => std::env::remove_var("HOME"), } + match original_userprofile { + Some(value) => std::env::set_var("USERPROFILE", value), + None => std::env::remove_var("USERPROFILE"), + } + + assert!(files.iter().all(|file| file.scope != MemoryScope::User)); + assert!(files.iter().any(|file| { + file.scope == MemoryScope::Project && file.content.contains("project memory") + })); + } + + #[test] + fn hosted_review_loads_managed_rules_only_when_allowed() { + let project = tempfile::tempdir().unwrap(); + let home = tempfile::tempdir().unwrap(); + let rules = home.path().join(".coven-code").join("rules"); + std::fs::create_dir_all(&rules).unwrap(); + std::fs::write(rules.join("managed.md"), "managed hosted policy").unwrap(); + + let _lock = crate::coven_shared::COVEN_HOME_ENV_LOCK + .lock() + .unwrap_or_else(|err| err.into_inner()); + let original_test_home = std::env::var("COVEN_CODE_TEST_HOME").ok(); + let original_home = std::env::var("HOME").ok(); + let original_userprofile = std::env::var("USERPROFILE").ok(); + std::env::set_var("COVEN_CODE_TEST_HOME", home.path()); + std::env::set_var("HOME", home.path()); + std::env::set_var("USERPROFILE", home.path()); + + let default_hosted = + load_all_memory_files_with_options(project.path(), &MemoryLoadOptions::hosted_review()); + let mut trusted_policy = MemoryLoadOptions::hosted_review(); + trusted_policy.allow_managed_rules = true; + let trusted_hosted = load_all_memory_files_with_options(project.path(), &trusted_policy); + match original_test_home { Some(value) => std::env::set_var("COVEN_CODE_TEST_HOME", value), None => std::env::remove_var("COVEN_CODE_TEST_HOME"), } + match original_home { + Some(value) => std::env::set_var("HOME", value), + None => std::env::remove_var("HOME"), + } match original_userprofile { Some(value) => std::env::set_var("USERPROFILE", value), None => std::env::remove_var("USERPROFILE"), } - assert!(files.iter().all(|file| file.scope != MemoryScope::User)); - assert!(files.iter().any(|file| { - file.scope == MemoryScope::Project && file.content.contains("project memory") + assert!(default_hosted + .iter() + .all(|file| file.scope != MemoryScope::Managed)); + assert!(trusted_hosted.iter().any(|file| { + file.scope == MemoryScope::Managed && file.content.contains("managed hosted policy") })); } @@ -474,14 +519,14 @@ mod tests { let files = load_all_memory_files_with_options(project.path(), &MemoryLoadOptions::local()); - match original_home { - Some(value) => std::env::set_var("HOME", value), - None => std::env::remove_var("HOME"), - } match original_test_home { Some(value) => std::env::set_var("COVEN_CODE_TEST_HOME", value), None => std::env::remove_var("COVEN_CODE_TEST_HOME"), } + match original_home { + Some(value) => std::env::set_var("HOME", value), + None => std::env::remove_var("HOME"), + } match original_userprofile { Some(value) => std::env::set_var("USERPROFILE", value), None => std::env::remove_var("USERPROFILE"), diff --git a/src-rust/crates/core/src/hosted_review.rs b/src-rust/crates/core/src/hosted_review.rs index 5d689567..560875d0 100644 --- a/src-rust/crates/core/src/hosted_review.rs +++ b/src-rust/crates/core/src/hosted_review.rs @@ -21,11 +21,26 @@ impl RuntimeMode { pub struct HostedReviewConfig { #[serde(default, skip_serializing_if = "is_false")] pub enabled: bool, + #[serde(default, skip_serializing_if = "is_false")] + pub allow_user_memory: bool, + #[serde(default, skip_serializing_if = "is_false")] + pub allow_managed_rules: bool, + #[serde(default, skip_serializing_if = "is_false")] + pub allow_write_tools: bool, + #[serde(default, skip_serializing_if = "is_false")] + pub allow_mcp_servers: bool, + #[serde(default, skip_serializing_if = "is_false")] + pub allow_plugins: bool, } impl HostedReviewConfig { pub fn is_default(&self) -> bool { !self.enabled + && !self.allow_user_memory + && !self.allow_managed_rules + && !self.allow_write_tools + && !self.allow_mcp_servers + && !self.allow_plugins } } diff --git a/src-rust/crates/core/src/lib.rs b/src-rust/crates/core/src/lib.rs index 030b27bf..99dc98f8 100644 --- a/src-rust/crates/core/src/lib.rs +++ b/src-rust/crates/core/src/lib.rs @@ -1291,6 +1291,18 @@ pub mod config { self.hosted_review.enabled || crate::hosted_review::env_enables_hosted_review() } + pub fn memory_load_options(&self) -> crate::claudemd::MemoryLoadOptions { + if !self.hosted_review_enabled() { + return crate::claudemd::MemoryLoadOptions::local(); + } + + crate::claudemd::MemoryLoadOptions { + mode: crate::hosted_review::RuntimeMode::HostedReview, + allow_user_memory: self.hosted_review.allow_user_memory, + allow_managed_rules: self.hosted_review.allow_managed_rules, + } + } + /// Resolve the effective model, falling back to a provider-appropriate default. /// /// When a non-Anthropic provider is active and no model is explicitly set, @@ -2069,6 +2081,23 @@ pub mod config { assert!(settings.effective_config().hosted_review_enabled()); } + #[test] + fn hosted_review_trusted_policy_controls_memory_options() { + let settings: Settings = serde_json::from_str( + r#"{"config":{"hostedReview":{"enabled":true,"allowManagedRules":true}}}"#, + ) + .unwrap(); + + let options = settings.effective_config().memory_load_options(); + + assert_eq!( + options.mode, + crate::hosted_review::RuntimeMode::HostedReview + ); + assert!(!options.allow_user_memory); + assert!(options.allow_managed_rules); + } + #[test] fn hosted_review_env_enables_config() { let _lock = crate::coven_shared::COVEN_HOME_ENV_LOCK diff --git a/src-rust/crates/core/src/session_storage.rs b/src-rust/crates/core/src/session_storage.rs index d5df6f93..e3d4c89f 100644 --- a/src-rust/crates/core/src/session_storage.rs +++ b/src-rust/crates/core/src/session_storage.rs @@ -321,6 +321,7 @@ pub async fn write_transcript_entry(path: &Path, entry: &TranscriptEntry) -> cra .await?; file.write_all(line.as_bytes()).await?; + file.flush().await?; Ok(()) }