From 1e68f4f73579992c8a672cd00220b1e244eba877 Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Wed, 23 Sep 2026 00:36:57 -0700 Subject: [PATCH 1/2] fix(fleet): retain bounded inspection tools in read-only workflows Remove a second File-only tool default from Workflow lowering. The existing child grant remains the single authority: role, parent permissions, scope and write_authority are intersected before catalog projection and dispatch. Explicit tool allowlists and deny_all_tools remain exact. The stopship fixture now declares its deliberately File-only evidence scope explicitly. Normally write-capable identities narrowed to read_only keep the existing classifier-bounded inspection shell instead of losing every command. Parent shell denials remain binding, arbitrary commands and tests are not inspection, and no write scope is granted. Verifiers retain their bounded Run interface. Do not annotate missing tools and missing source files as unavailable models. Preserve provider/model/route diagnostics for real model access failures. Update the existing agent guide to describe these boundaries accurately. Verification: - cargo fmt --all -- --check: PASS. - cargo clippy -p codewhale-tui --all-targets --all-features --locked -- -D warnings -A clippy::uninlined_format_args -A clippy::too_many_arguments -A clippy::unnecessary_map_or: PASS. - Targeted nextest: 15 passed, 0 failed; 13230 not selected. Includes actual isolated Git inspection through the child executor, mutation denial, parent shell denial, bounded verifier/catalog tests, Workflow narrowing, stopship handoff receipts and accurate failure classification. - Dead-code budget: 279, PASS. Blocking-call budget: 578 sites/171 files, PASS. - git diff --check: PASS. - No web changes. Earlier relevant root-gate evidence was packaging 67, SDK 14, web Vitest 490 passed, and check:web passed; not rerun for this Rust slice. No model substitution is implemented here. Pinned routes, approval decisions, network limits, write claims, and the shared Engine remain authoritative. No provider call or installed-runtime upgrade is claimed. Refs #6407 Signed-off-by: CodeWhale Bot --- crates/tui/src/tools/subagent/mod.rs | 16 ++-- crates/tui/src/tools/subagent/tests.rs | 100 ++++++++++++++++++++++++- crates/tui/src/tools/workflow/mod.rs | 51 +++++++++---- docs/SUBAGENTS.md | 13 +++- workflows/stopship.workflow.js | 1 + 5 files changed, 155 insertions(+), 26 deletions(-) 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/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", From c19e25b3d52584d9daa0f8291b7f86745d1b968a Mon Sep 17 00:00:00 2001 From: CodeWhale Bot Date: Wed, 23 Sep 2026 01:05:41 -0700 Subject: [PATCH 2/2] fix(runtime): keep repeated agent stops independent of receipt persistence Hosted macOS CI reproduced a second Stop returning 404 after the first Stop returned 200: cancel_agent_run filtered out its in-memory terminal record and raced the asynchronous disk projection. Return the owning manager's terminal receipt without issuing another cancellation or weakening foreign-session checks. Add a deterministic memory-only-manager regression for repeated Stop. Complete the intentional inspection change from #6423 by asserting the explicit File-only stopship fixture and the exact builder's inherited, read-only runtime grant. These are contracts for the new behavior, not ignored or deleted tests; write refusal, exact-provider identity and deny-all handoffs stay pinned. Verification: - cargo fmt --all -- --check: passed. - cargo clippy -p codewhale-tui -p codewhale-workflow --all-targets --all-features --locked with CI -D warnings and the three CI lint allowances: passed. - Targeted nextest: 7 passed, 0 failed; 13,506 not selected. Covers repeated Stop before persistence, existing live-child Stop, foreign-session refusal, the two hosted fixture failures, bounded Git vs mutation, and exact Fleet authority non-escalation. - Link emitted the existing large __eh_frame warning; tests linked and passed. - Dead-code budget: 279, passed. Blocking-call budget: 578 sites/171 files, passed. - git diff --check: passed. No web changes; web gates were not rerun. Hosted failure receipts: jobs 107088964032, 107088964169 and 107088964192. No provider call, installed-runtime upgrade or hosted-green claim is made here. Refs #6423 Signed-off-by: CodeWhale Bot --- crates/tui/src/runtime_api.rs | 15 ++++-- crates/tui/src/runtime_api/tests.rs | 51 +++++++++++++++++++ .../tui/src/tools/workflow/shortlist_tests.rs | 7 +-- crates/workflow/src/js_authoring.rs | 10 +++- 4 files changed, 74 insertions(+), 9 deletions(-) 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/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",