diff --git a/crates/tui/src/runtime_api.rs b/crates/tui/src/runtime_api.rs index 5d27ffeeb9..637d0f2122 100644 --- a/crates/tui/src/runtime_api.rs +++ b/crates/tui/src/runtime_api.rs @@ -2114,8 +2114,9 @@ async fn cancel_agent_run( Path(run_id): Path, ) -> Result<(StatusCode, Json), ApiError> { // Runs this runtime is executing itself (Fleet-launched children) stop - // in place. Only a child running in this process qualifies: records the - // manager loaded from disk belong to whichever process wrote them. + // in place. Only a running child in this process qualifies for mutation; + // a terminal receipt can be returned without mutating or consulting disk. + // Other persisted runs still go through their owning session below. let owned = { let manager = state.sub_agent_manager.read().await; manager @@ -2125,10 +2126,18 @@ async fn cancel_agent_run( .filter(|record| { manager .get_result(&record.spec.worker_id) - .is_ok_and(|agent| agent.status == SubAgentStatus::Running) + .is_ok_and(|agent| { + agent.status == SubAgentStatus::Running || record.status.is_terminal() + }) }) }; if let Some(record) = owned { + // Persistence is asynchronous. A repeated stop must answer from the + // owning manager's terminal receipt, not race the disk projection and + // incorrectly report a run we just stopped as missing or still live. + if record.status.is_terminal() { + return Ok((StatusCode::OK, Json(record))); + } let agent_id = record.spec.worker_id.clone(); let cancelled = { let mut manager = state.sub_agent_manager.write().await; diff --git a/crates/tui/src/runtime_api/tests.rs b/crates/tui/src/runtime_api/tests.rs index 965f2e4841..44e9e325a6 100644 --- a/crates/tui/src/runtime_api/tests.rs +++ b/crates/tui/src/runtime_api/tests.rs @@ -2912,6 +2912,57 @@ async fn agent_run_cancel_stops_a_live_child_and_returns_its_receipt() -> Result Ok(()) } +#[tokio::test] +async fn agent_run_cancel_remains_idempotent_before_receipt_persistence() -> Result<()> { + let temp = tempfile::tempdir()?; + let root = temp.path().to_path_buf(); + let workspace = root.join("workspace"); + fs::create_dir_all(&workspace)?; + // A memory-only manager deterministically models a disk projection that + // has not caught up. Repeated stops must use its authoritative receipt. + let manager = Arc::new(tokio::sync::RwLock::new( + crate::tools::subagent::SubAgentManager::new(workspace.clone(), 2), + )); + let agent_id = { + let mut guard = manager.write().await; + let id = guard.insert_test_running_agent("not-yet-persisted", &workspace); + guard.assign_test_session_owner(&id, "session-stop"); + id + }; + let Some((addr, _runtime_threads, handle)) = + spawn_test_server_with_root_token_mobile_workspace_and_subagents( + root.clone(), + root.join("sessions"), + None, + false, + workspace.clone(), + Some(manager), + None, + ) + .await? + else { + return Ok(()); + }; + let client = crate::tls::reqwest_client(); + for _ in 0..2 { + let response = client + .post(format!("http://{addr}/v1/agent-runs/{agent_id}/cancel")) + .send() + .await?; + assert_eq!(response.status(), StatusCode::OK); + let receipt: serde_json::Value = response.json().await?; + assert_eq!(receipt["spec"]["worker_id"], agent_id.as_str()); + assert_eq!(receipt["status"], "cancelled"); + } + assert!( + !workspace + .join(".codewhale/state/subagents.v1.json") + .exists() + ); + handle.abort(); + Ok(()) +} + #[tokio::test] async fn agent_run_cancel_refuses_a_run_owned_by_a_session_it_does_not_host() -> Result<()> { let root = std::env::temp_dir().join(format!( diff --git a/crates/tui/src/tools/subagent/mod.rs b/crates/tui/src/tools/subagent/mod.rs index 7a6c60d448..49a341b3b8 100644 --- a/crates/tui/src/tools/subagent/mod.rs +++ b/crates/tui/src/tools/subagent/mod.rs @@ -10856,14 +10856,16 @@ fn apply_spawn_write_authority(runtime: &mut SubAgentRuntime, request: &SpawnReq } // `read_only` must be an executable posture, not just metadata. Normally // write-capable identities also inherit Full shell, which could mutate the - // workspace without a scope-aware claim under Auto/Full Access. Clamp that - // shell surface completely; verifier keeps its deliberate test runner. + // workspace without a scope-aware claim under Auto/Full Access. Narrow it + // to the existing classifier-bounded inspection shell, not a file-only + // surface. A parent with no shell stays shell-less; verifier keeps its + // deliberate test runner. The grant and executor enforce the same boundary. runtime.worker_profile.permissions.write = false; if matches!( request.agent_type, FleetRole::Worker | FleetRole::Builder | FleetRole::Custom ) { - runtime.worker_profile.shell = ShellPolicy::None; + runtime.worker_profile.shell = runtime.worker_profile.shell.min_with(ShellPolicy::ReadOnly); } } @@ -18714,18 +18716,18 @@ fn annotate_child_model_error( route_source_label(route), ) }; + let lower = err.to_ascii_lowercase(); match crate::error_taxonomy::classify_error_message(err) { - crate::error_taxonomy::ErrorCategory::Authorization - | crate::error_taxonomy::ErrorCategory::State => hint(), + crate::error_taxonomy::ErrorCategory::Authorization => hint(), + crate::error_taxonomy::ErrorCategory::State if lower.contains("model") => hint(), _ => { // #3020 (#2653): Provider rejections like "Model Not Exist" or // "does not exist or you do not have access" often classify as // `Internal` rather than `Authorization`/`State`. Catch these // patterns in the raw error text and annotate anyway. - let lower = err.to_ascii_lowercase(); if lower.contains("model not exist") || lower.contains("model_not_found") - || lower.contains("does not exist") + || lower.contains("model") && lower.contains("does not exist") || lower.contains("no such model") || lower.contains("invalid model") { diff --git a/crates/tui/src/tools/subagent/tests.rs b/crates/tui/src/tools/subagent/tests.rs index 0377af0c47..6adb7a0bb6 100644 --- a/crates/tui/src/tools/subagent/tests.rs +++ b/crates/tui/src/tools/subagent/tests.rs @@ -625,8 +625,87 @@ fn declared_read_only_write_roles_derive_without_mutating_shell() { false, ); assert!(!profile.permissions.write, "{request:?}"); - assert_eq!(profile.shell, ShellPolicy::None, "{request:?}"); + assert_eq!(profile.shell, ShellPolicy::ReadOnly, "{request:?}"); + + runtime.worker_profile.shell = ShellPolicy::None; + apply_spawn_write_authority(&mut runtime, &request); + assert_eq!(runtime.worker_profile.shell, ShellPolicy::None); + } +} + +#[tokio::test] +async fn explicit_read_only_general_can_inspect_git_but_cannot_mutate() { + let tmp = tempdir().expect("tempdir"); + init_claim_repo(tmp.path()); + let workspace = tmp.path().canonicalize().expect("workspace"); + let request = parse_spawn_request(&json!({ + "prompt": "inspect CI evidence", + "write_authority": "read_only", + "allowed_tools": ["read", "bash"] + })) + .expect("read-only request"); + let mut runtime = + stub_runtime().with_agent_tool_surface_options(enabled_agent_surface_options()); + runtime.context = ToolContext::new(workspace.clone()); + runtime.context.auto_approve = true; + apply_spawn_write_authority(&mut runtime, &request); + runtime.worker_profile = worker_profile_for_spawn( + &runtime, + &request.agent_type, + &AgentWorkerToolProfile::Inherited, + "deepseek-v4-pro", + None, + false, + ); + let registry = SubAgentToolRegistry::new( + runtime, + request.agent_type, + Some(vec!["read".into(), "bash".into()]), + crate::tools::todo::new_shared_todo_list(), + crate::tools::plan::new_shared_plan_state(), + ); + assert!(registry.unavailable_allowed_tools().is_empty()); + assert_ne!( + registry.grant.files, + crate::worker_profile::FileGrant::Write + ); + for command in ["pwd", "git status --short", "git log --oneline -1"] { + let output = registry + .execute("inspection", "bash", json!({"command": command})) + .await + .unwrap_or_else(|error| panic!("{command}: {error}")); + if command != "git status --short" { + assert!(!output.trim().is_empty()); + } } + for command in [ + "gh run view 123 --repo owner/repo --log-failed", + "rg -n TODO src", + ] { + let input = json!({"command": command}); + assert!( + registry.posture_permits_tool("bash", Some(&input)), + "{command}" + ); + assert!( + registry.envelope_refusal("bash", &input).is_none(), + "{command}" + ); + } + for command in [ + "touch forbidden.txt", + "git checkout -b forbidden", + "npm test", + ] { + assert!( + registry + .execute("inspection", "bash", json!({"command": command})) + .await + .is_err(), + "inspection is not arbitrary execution: {command}" + ); + } + assert!(!workspace.join("forbidden.txt").exists()); } #[test] @@ -11964,6 +12043,25 @@ fn annotate_child_model_error_adds_actionable_hint() { ); } +#[test] +fn child_runtime_capability_errors_are_not_misreported_as_model_access_errors() { + for error in [ + "Sub-agent requested unavailable tools: bash", + "Requested source file does not exist", + "The worktree path is unavailable", + ] { + assert_eq!( + annotate_child_model_error( + error, + "deepseek-flash", + crate::config::ApiProvider::Deepseek, + &ModelRoute::Inherit, + ), + error, + ); + } +} + #[test] fn child_launch_error_names_provider_model_and_route_source() { // #4049: a model-not-found child launch failure must name the provider diff --git a/crates/tui/src/tools/workflow/mod.rs b/crates/tui/src/tools/workflow/mod.rs index 1f77d8a26a..967dabf583 100644 --- a/crates/tui/src/tools/workflow/mod.rs +++ b/crates/tui/src/tools/workflow/mod.rs @@ -3518,22 +3518,11 @@ fn leaf_allowed_tools(spec: &LeafSpec) -> Result>, ToolError> if !spec.permissions.allowed_tools.is_empty() { return Ok(Some(spec.permissions.allowed_tools.clone())); } - if spec.mode != TaskMode::ReadOnly { - return Ok(None); - } - Ok(Some( - read_only_allowed_tools(spec.agent_type) - .iter() - .map(|tool| (*tool).to_string()) - .collect(), - )) -} - -fn read_only_allowed_tools(agent_type: AgentType) -> &'static [&'static str] { - match agent_type { - AgentType::Verifier => &["File"], - _ => &["File"], - } + // The child grant already intersects role, parent permissions and the + // emitted writeAuthority. A second File-only default hid bounded Git/CI + // inspection from scouts and Run from verifiers without adding safety. + // Explicit allowlists and deny_all_tools above remain exact restrictions. + Ok(None) } fn is_write_or_shell_tool(tool: &str) -> bool { @@ -7904,6 +7893,36 @@ export default workflow({ ); } + #[test] + fn read_only_workflow_leaves_use_the_runtime_grant_unless_explicitly_narrowed() { + for agent_type in ["explore", "review", "verifier", "general"] { + let mut leaf: LeafSpec = serde_json::from_value(json!({ + "id": "inspect", + "prompt": "Inspect source and CI evidence", + "agent_type": agent_type, + "mode": "read_only" + })) + .expect("read-only leaf"); + assert_eq!(leaf_allowed_tools(&leaf).unwrap(), None); + let source = leaf_task_options_expression(&leaf, None, false).unwrap(); + assert!(source.contains("writeAuthority: \"read_only\""), "{source}"); + assert!(!source.contains("allowedTools:"), "{source}"); + + leaf.permissions.deny_all_tools = true; + assert_eq!(leaf_allowed_tools(&leaf).unwrap(), Some(Vec::new())); + leaf.permissions.deny_all_tools = false; + leaf.permissions.allowed_tools = vec!["File".into()]; + assert_eq!( + leaf_allowed_tools(&leaf).unwrap(), + Some(vec!["File".into()]) + ); + leaf.permissions.allowed_tools = vec!["bash".into()]; + assert!(validate_leaf_runtime_contract(&leaf).is_ok()); + leaf.permissions.allowed_tools = vec!["exec_shell".into()]; + assert!(validate_leaf_runtime_contract(&leaf).is_err()); + } + } + #[test] fn parallel_read_only_children_do_not_default_to_worktree() { let source = r#" diff --git a/crates/tui/src/tools/workflow/shortlist_tests.rs b/crates/tui/src/tools/workflow/shortlist_tests.rs index 18e42874c1..b848a5750f 100644 --- a/crates/tui/src/tools/workflow/shortlist_tests.rs +++ b/crates/tui/src/tools/workflow/shortlist_tests.rs @@ -412,11 +412,8 @@ async fn native_exact_fleet_builder_keeps_the_plan_read_only_ceiling() { !profile.permissions.write, "the authored read_only mode must remain executable policy" ); - assert_eq!(profile.shell, crate::worker_profile::ShellPolicy::None); - assert_eq!( - profile.tools, - crate::worker_profile::ToolScope::Explicit(vec!["File".into()]) - ); + assert_eq!(profile.shell, crate::worker_profile::ShellPolicy::ReadOnly); + assert_eq!(profile.tools, crate::worker_profile::ToolScope::Inherit); assert_eq!( records[0].spec.child_route.as_ref().unwrap().provider_id, "openrouter" diff --git a/crates/workflow/src/js_authoring.rs b/crates/workflow/src/js_authoring.rs index c6604961c0..1cfda09683 100644 --- a/crates/workflow/src/js_authoring.rs +++ b/crates/workflow/src/js_authoring.rs @@ -653,7 +653,15 @@ workflow({ assert_eq!(leaf.role.as_deref(), Some(expected_role)); assert_eq!(leaf.mode, TaskMode::ReadOnly); assert!(!leaf.permissions.allow_write); - assert!(leaf.permissions.allowed_tools.is_empty()); + let expected_tools: &[&str] = if expected_role == "explore" { + &["File"] + } else { + &[] + }; + assert_eq!( + leaf.permissions.allowed_tools, expected_tools, + "the fixture must explicitly restrict source gathering to File" + ); assert_eq!( leaf.permissions.deny_all_tools, expected_role != "explore", diff --git a/docs/SUBAGENTS.md b/docs/SUBAGENTS.md index f46c568040..253e3219c7 100644 --- a/docs/SUBAGENTS.md +++ b/docs/SUBAGENTS.md @@ -76,9 +76,9 @@ stewardship. | Role | Stance | Writes? | Network? | Shell posture | Typical use | |---------------|----------------------------------------|---------|----------|---------------|----------------------------------------------| | `general` | flexible; do whatever the parent says | yes | yes | yes | the default; multi-step tasks | -| `explore` | read-only; map the relevant code fast | no | yes | read-only (net + bounded verify) | "find every call site of `Foo`; check the PR with gh" | +| `explore` | read-only; map the relevant code fast | no | yes | bounded inspection | "find every call site of `Foo`; check the PR with gh" | | `planner` | analyse and produce a strategy | no | yes | read-only probes | "design the migration; don't execute" | -| `reviewer` | read-and-grade with severity scores | no | yes | read-only (net + bounded verify) | "audit this PR for bugs" | +| `reviewer` | read-and-grade with severity scores | no | yes | bounded inspection | "audit this PR for bugs" | | `implement` | land a specific change with min edit | yes | yes | yes | "rewrite `bar.rs::Foo::bar` to do X" | | `test` | run tests / validation, report outcome | no | yes | bounded verification (no writes) | "verify the diff with the bounded test checks; report PASS/FAIL" | | `advisor` | short-lived, high-reasoning counsel | no | yes | none | "what are we missing in this design?" | @@ -213,6 +213,15 @@ real isolated worktree may proceed in parallel. A `custom` role requires explicit write-capable authority to claim writes; otherwise it starts read-only. +Read-only is not file-only. A normally write-capable agent narrowed with +`write_authority: "read_only"` keeps the existing classifier-bounded inspection +shell when its parent permits it: Git history/status, search, and allowed `gh` +log reads, not arbitrary commands or test programs. Workflow read-only steps +likewise use the effective role's tools instead of imposing a second File-only +list; a `test` role retains its bounded verification interface. Explicit tool +allowlists, `deny_all_tools`, parent denials, network limits, and mutation checks +still apply. A parent without shell access cannot delegate it. + Optional fields: - `worktree_branch`: exact branch to create. diff --git a/workflows/stopship.workflow.js b/workflows/stopship.workflow.js index 61654f7991..842b84a25b 100644 --- a/workflows/stopship.workflow.js +++ b/workflows/stopship.workflow.js @@ -78,6 +78,7 @@ export default workflow({ "agent_type": "explore", "role": "explore", "mode": "read_only", + "permissions": { "allowed_tools": ["File"] }, "file_scope": [ "fleets/stopship.toml", "crates/cli/src/lib.rs",