From 7aaf189247f4fd758af1082bd48658835a1148a6 Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 27 Aug 2026 11:11:06 +0900 Subject: [PATCH 1/3] fix: scope delegated commands by cwd --- crates/tools/src/bash.rs | 15 +- crates/tools/tests/integration.rs | 12 + crates/workdir/src/delegation.rs | 330 ++++++++++++++++++++++--- crates/workdir/src/local.rs | 42 +++- crates/workdir/src/operation.rs | 5 + crates/worker/tests/controller_test.rs | 4 + crates/workspace-server/src/server.rs | 1 + 7 files changed, 370 insertions(+), 39 deletions(-) diff --git a/crates/tools/src/bash.rs b/crates/tools/src/bash.rs index 8224472b..a8cff604 100644 --- a/crates/tools/src/bash.rs +++ b/crates/tools/src/bash.rs @@ -5,7 +5,9 @@ use agen::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput}; use async_trait::async_trait; use schemars::JsonSchema; use serde::Deserialize; -use workdir::{CommandHandle, CommandOutputRequest, CommandRequest, WorkdirSessionHandle}; +use workdir::{ + CommandHandle, CommandOutputRequest, CommandRequest, WorkdirPath, WorkdirSessionHandle, +}; const DEFAULT_TIMEOUT_SECS: u64 = 120; const MAX_TIMEOUT_SECS: u64 = 600; @@ -14,6 +16,10 @@ const INLINE_BYTE_BUDGET: usize = 12 * 1024; #[derive(Debug, Deserialize, JsonSchema)] struct BashParams { command: String, + /// Optional logical working directory relative to the bound session cwd. + /// Supplying it lets delegation guards prove the command is disjoint from child write scopes. + #[serde(default)] + cwd: Option, #[serde(default)] timeout: Option, } @@ -51,11 +57,18 @@ impl Tool for BashTool { .timeout .unwrap_or(DEFAULT_TIMEOUT_SECS) .clamp(1, MAX_TIMEOUT_SECS); + let cwd = params + .cwd + .as_deref() + .map(WorkdirPath::new) + .transpose() + .map_err(crate::ToolsError::from)?; let cmd_summary = truncate_for_summary(¶ms.command); let handle = self .session .start_command(CommandRequest { command: params.command, + cwd, timeout_secs, output_limit: INLINE_BYTE_BUDGET, tool_call_id: Some(ctx.call_id), diff --git a/crates/tools/tests/integration.rs b/crates/tools/tests/integration.rs index f8761aa8..fd444c7a 100644 --- a/crates/tools/tests/integration.rs +++ b/crates/tools/tests/integration.rs @@ -390,6 +390,18 @@ async fn bash_inherits_workdir_cwd() { assert_eq!(actual, expected); } +#[tokio::test] +async fn bash_uses_explicit_logical_cwd() { + let (dir, _spill, reg) = setup(); + std::fs::create_dir_all(dir.path().join("nested")).unwrap(); + let bash = reg.get("Bash"); + let out = call(&bash, json!({ "command": "pwd", "cwd": "nested" })).await; + let body = out.content.unwrap(); + let actual = std::fs::canonicalize(body.trim()).unwrap(); + let expected = std::fs::canonicalize(dir.path().join("nested")).unwrap(); + assert_eq!(actual, expected); +} + #[tokio::test] async fn bash_provider_output_does_not_expose_internal_paths() { let (_dir, spill, reg) = setup(); diff --git a/crates/workdir/src/delegation.rs b/crates/workdir/src/delegation.rs index 4a5b313f..4ee2b356 100644 --- a/crates/workdir/src/delegation.rs +++ b/crates/workdir/src/delegation.rs @@ -251,14 +251,43 @@ impl DelegatingWorkdirSession { self.ensure_path(path, WorkdirDelegationPermission::Write) } - fn ensure_command(&self, starting: bool) -> Result<(), WorkdirError> { + fn resolve_command_cwd(&self, cwd: Option<&FsPath>) -> Result { + match cwd { + Some(cwd) => self.resolve_path(cwd), + None => Ok(self.cwd.clone()), + } + } + + fn ensure_command_start(&self, cwd: &FsPath) -> Result<(), WorkdirError> { self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; - if starting && self.has_active_write_lease() { - return Err(WorkdirError::Denied( - "command execution is denied while a child holds a write delegation".into(), - )); + if let Some(scope) = &self.scope + && !scope.iter().any(|rule| { + rule.permission == WorkdirDelegationPermission::Write + && rule_allows_path(rule, cwd, WorkdirDelegationPermission::Write) + }) + { + return Err(WorkdirError::Denied(format!( + "command cwd `{cwd}` is outside the delegated write scope" + ))); + } + + let mut leases = self + .child_write_leases + .lock() + .expect("workdir delegation lease mutex poisoned"); + leases.retain(|_, lease| lease.validity.upgrade().is_some_and(|v| v.is_active())); + if leases.values().any(|lease| { + lease.rules.iter().any(|rule| { + rule.permission == WorkdirDelegationPermission::Write + && command_cwd_overlaps_rule(cwd, rule) + }) + }) { + Err(WorkdirError::Denied(format!( + "command cwd `{cwd}` overlaps a child write delegation" + ))) + } else { + Ok(()) } - Ok(()) } fn ensure_parent_write_available(&self, path: &FsPath) -> Result<(), WorkdirError> { @@ -281,20 +310,6 @@ impl DelegatingWorkdirSession { } } - fn has_active_write_lease(&self) -> bool { - let mut leases = self - .child_write_leases - .lock() - .expect("workdir delegation lease mutex poisoned"); - leases.retain(|_, lease| lease.validity.upgrade().is_some_and(|v| v.is_active())); - leases.values().any(|lease| { - lease - .rules - .iter() - .any(|rule| rule.permission == WorkdirDelegationPermission::Write) - }) - } - fn validate_delegation_rules( &self, rules: &[WorkdirDelegationRule], @@ -502,13 +517,20 @@ impl WorkdirSession for DelegatingWorkdirSession { self.source.grep(request).await } - async fn start_command(&self, request: CommandRequest) -> Result { - self.ensure_command(true)?; + async fn start_command( + &self, + mut request: CommandRequest, + ) -> Result { + let cwd = self.resolve_command_cwd(request.cwd.as_ref())?; + self.ensure_command_start(&cwd)?; + if !self.source.transports_delegation_context() { + request.cwd = Some(cwd); + } self.source.start_command(request).await } async fn command_status(&self, handle: CommandHandle) -> Result { - self.ensure_command(false)?; + self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; self.source.command_status(handle).await } @@ -516,12 +538,12 @@ impl WorkdirSession for DelegatingWorkdirSession { &self, request: CommandOutputRequest, ) -> Result { - self.ensure_command(false)?; + self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; self.source.command_output(request).await } async fn cancel_command(&self, handle: CommandHandle) -> Result<(), WorkdirError> { - self.ensure_command(false)?; + self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; self.source.cancel_command(handle).await } @@ -649,6 +671,11 @@ impl WorkdirSession for ReadOnlyWorkdirSession { } } +fn command_cwd_overlaps_rule(cwd: &FsPath, rule: &WorkdirDelegationRule) -> bool { + rule_allows_path(rule, cwd, WorkdirDelegationPermission::Write) + || Path::new(rule.target.as_str()).starts_with(Path::new(cwd.as_str())) +} + fn rule_allows_path( rule: &WorkdirDelegationRule, path: &FsPath, @@ -763,6 +790,7 @@ mod tests { let handle = parent .start_command(CommandRequest { command: "printf ready; sleep 0.2; printf done".into(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-delegated".into()), @@ -917,6 +945,18 @@ mod tests { .await, Err(WorkdirError::Denied(_)) )); + assert!(matches!( + parent + .start_command(CommandRequest { + command: "printf escaped".into(), + cwd: Some(fs_path("granted/outside")), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("symlink-cwd".into()), + }) + .await, + Err(WorkdirError::Denied(message)) if message.contains("traverses a symlink") + )); parent .write(write("secret/parent", "still-authoritative")) .await @@ -924,7 +964,7 @@ mod tests { } #[tokio::test] - async fn write_lease_blocks_parent_region_until_release() { + async fn write_lease_blocks_only_overlapping_parent_command_cwds_until_release() { let root = TempDir::new().unwrap(); fs::create_dir_all(root.path().join("leased")).unwrap(); fs::create_dir_all(root.path().join("other")).unwrap(); @@ -941,7 +981,8 @@ mod tests { let command = child .scoped_session .start_command(CommandRequest { - command: "printf child-command".into(), + command: "pwd; printf child-command".into(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("delegated-child-command".into()), @@ -958,17 +999,65 @@ mod tests { }) .await .unwrap(); - assert_eq!(command_output.content, "child-command"); assert!( - parent - .start_command(CommandRequest { - command: "printf parent-command".into(), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("blocked-parent-command".into()), - }) - .await - .is_err() + command_output.content.ends_with("leased\nchild-command"), + "child command must run from its delegated cwd: {}", + command_output.content + ); + let denied = parent + .start_command(CommandRequest { + command: "printf parent-command".into(), + cwd: None, + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("blocked-parent-command".into()), + }) + .await + .unwrap_err(); + assert!(matches!( + denied, + WorkdirError::Denied(message) + if message.contains("command cwd `.` overlaps a child write delegation") + )); + let denied = parent + .start_command(CommandRequest { + command: "printf still-denied".into(), + cwd: Some(fs_path("leased")), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("overlapping-parent-command".into()), + }) + .await + .unwrap_err(); + assert!(matches!( + denied, + WorkdirError::Denied(message) + if message.contains("command cwd `leased` overlaps a child write delegation") + )); + + let unrelated = parent + .start_command(CommandRequest { + command: "pwd; printf parent-command".into(), + cwd: Some(fs_path("other")), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("unrelated-parent-command".into()), + }) + .await + .unwrap(); + let unrelated_output = parent + .command_output(CommandOutputRequest { + handle: unrelated, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert!( + unrelated_output.content.ends_with("other\nparent-command"), + "parent command must run from its explicit disjoint cwd: {}", + unrelated_output.content ); assert!(matches!( @@ -982,6 +1071,26 @@ mod tests { .await .unwrap(); child.release(); + let resumed = parent + .start_command(CommandRequest { + command: "printf resumed".into(), + cwd: None, + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("resumed-parent-command".into()), + }) + .await + .unwrap(); + let resumed_output = parent + .command_output(CommandOutputRequest { + handle: resumed, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert_eq!(resumed_output.content, "resumed"); parent .write(write("leased/parent", "parent")) .await @@ -1033,6 +1142,153 @@ mod tests { )); } + #[tokio::test] + async fn nested_write_delegation_uses_each_session_cwd_without_widening_scope() { + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("top/nested")).unwrap(); + fs::create_dir_all(root.path().join("top/peer")).unwrap(); + fs::create_dir_all(root.path().join("other")).unwrap(); + let root_session = session(root.path()); + let child = root_session + .delegate(request("top", WorkdirDelegationPermission::Write)) + .await + .unwrap(); + let nested = child + .scoped_session + .delegate(WorkdirDelegationRequest { + rules: vec![WorkdirDelegationRule { + target: fs_path("top/nested"), + permission: WorkdirDelegationPermission::Write, + recursive: true, + }], + cwd: fs_path("top/nested"), + }) + .await + .unwrap(); + + let nested_handle = nested + .scoped_session + .start_command(CommandRequest { + command: "pwd; printf nested".into(), + cwd: None, + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("nested-command".into()), + }) + .await + .unwrap(); + let nested_output = nested + .scoped_session + .command_output(CommandOutputRequest { + handle: nested_handle, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert!(nested_output.content.ends_with("top/nested\nnested")); + + let denied = child + .scoped_session + .start_command(CommandRequest { + command: "printf blocked".into(), + cwd: None, + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("nested-overlap".into()), + }) + .await + .unwrap_err(); + assert!(matches!( + denied, + WorkdirError::Denied(message) + if message.contains("command cwd `top` overlaps a child write delegation") + )); + + let peer_handle = child + .scoped_session + .start_command(CommandRequest { + command: "pwd; printf peer".into(), + cwd: Some(fs_path("peer")), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("nested-peer".into()), + }) + .await + .unwrap(); + let peer_output = child + .scoped_session + .command_output(CommandOutputRequest { + handle: peer_handle, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert!(peer_output.content.ends_with("top/peer\npeer")); + + let outside_handle = root_session + .start_command(CommandRequest { + command: "pwd; printf outside".into(), + cwd: Some(fs_path("other")), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("root-outside".into()), + }) + .await + .unwrap(); + let outside_output = root_session + .command_output(CommandOutputRequest { + handle: outside_handle, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert!(outside_output.content.ends_with("other\noutside")); + + nested.release(); + child.release(); + } + + #[tokio::test] + async fn reapplied_delegation_chain_preserves_command_cwd() { + let root = TempDir::new().unwrap(); + fs::create_dir_all(root.path().join("delegated")).unwrap(); + let parent = session(root.path()); + let applied = apply_delegation_chain( + parent, + [request("delegated", WorkdirDelegationPermission::Write)], + ) + .await + .unwrap(); + let scoped = &applied.scoped_session; + + let handle = scoped + .start_command(CommandRequest { + command: "pwd; printf reapplied".into(), + cwd: None, + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("reapplied-command".into()), + }) + .await + .unwrap(); + let output = scoped + .command_output(CommandOutputRequest { + handle, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap(); + assert!(output.content.ends_with("delegated\nreapplied")); + } + #[tokio::test] async fn applied_chain_cannot_replace_outer_provider_attenuation() { let root = TempDir::new().unwrap(); diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index 16686cb4..8b422970 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -548,6 +548,40 @@ impl LocalWorkdirSession { self.inner.root.join(path.as_str()) } } + + fn resolve_command_cwd(&self, cwd: Option<&WorkdirPath>) -> Result { + let Some(cwd) = cwd else { + return Ok(self.inner.cwd.clone()); + }; + let host_cwd = self.resolve(cwd); + let canonical_root = self + .inner + .root + .canonicalize() + .map_err(|error| WorkdirError::io(&self.inner.root, error))?; + let expected = if cwd.is_root() { + canonical_root + } else { + canonical_root.join(cwd.as_str()) + }; + let resolved = host_cwd + .canonicalize() + .map_err(|error| WorkdirError::io(&host_cwd, error))?; + if resolved != expected { + return Err(WorkdirError::Denied(format!( + "command cwd `{cwd}` traverses a symlink" + ))); + } + let scope = self.inner.scope.snapshot(); + if !scope.is_readable(&resolved) + || !std::fs::metadata(&resolved).is_ok_and(|metadata| metadata.is_dir()) + { + return Err(WorkdirError::Denied(format!( + "command cwd `{cwd}` is not a readable Workdir directory" + ))); + } + Ok(resolved) + } } #[async_trait] @@ -693,7 +727,7 @@ impl WorkdirSession for LocalWorkdirSession { self.ensure_open()?; let id = self.inner.next_command_id.fetch_add(1, Ordering::Relaxed); let handle = CommandHandle(format!("command-{id}")); - let cwd = self.inner.cwd.clone(); + let cwd = self.resolve_command_cwd(request.cwd.as_ref())?; let (completion_tx, completion) = watch::channel(false); let command_id = handle.0.clone(); let telemetry = self.inner.command_telemetry.clone(); @@ -1438,6 +1472,7 @@ mod tests { &session, CommandRequest { command: "sleep 30".to_owned(), + cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, @@ -1964,6 +1999,7 @@ mod tests { &workdir, CommandRequest { command: "pwd && printf provider-command".into(), + cwd: None, timeout_secs: 5, output_limit: 4096, tool_call_id: None, @@ -1999,6 +2035,7 @@ mod tests { &workdir, CommandRequest { command: "printf 'aéz'".into(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: None, @@ -2222,6 +2259,7 @@ mod tests { &workdir, CommandRequest { command: "printf ready; printf warning >&2; sleep 0.2; printf done".into(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-7".into()), @@ -2325,6 +2363,7 @@ mod tests { &workdir, CommandRequest { command: "sleep 30".into(), + cwd: None, timeout_secs: 1, output_limit: 1024, tool_call_id: None, @@ -2394,6 +2433,7 @@ mod tests { &workdir, CommandRequest { command: "sleep 30".into(), + cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, diff --git a/crates/workdir/src/operation.rs b/crates/workdir/src/operation.rs index 47527ad8..fe19763c 100644 --- a/crates/workdir/src/operation.rs +++ b/crates/workdir/src/operation.rs @@ -1,3 +1,4 @@ +use fs_operation::FsPath; use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] @@ -7,6 +8,10 @@ pub struct CommandHandle(pub String); #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CommandRequest { pub command: String, + /// Optional logical working directory relative to the calling session's cwd. + /// Providers must resolve and validate it before starting the process. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub cwd: Option, pub timeout_secs: u64, pub output_limit: usize, /// Optional caller-owned correlation id. Bash supplies its tool-call id so diff --git a/crates/worker/tests/controller_test.rs b/crates/worker/tests/controller_test.rs index 3e7e750b..c4a226a3 100644 --- a/crates/worker/tests/controller_test.rs +++ b/crates/worker/tests/controller_test.rs @@ -230,6 +230,7 @@ async fn shutdown_closes_bound_workdir_session() { let command = session .start_command(CommandRequest { command: "sleep 30".to_owned(), + cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, @@ -271,6 +272,7 @@ async fn controller_projects_workdir_command_events_and_snapshot_state() { let command = session .start_command(CommandRequest { command: "printf ready; sleep 0.3; printf done".to_owned(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-command-1".into()), @@ -378,6 +380,7 @@ async fn controller_refreshes_command_snapshot_after_high_output_provider_lag() .start_command(CommandRequest { command: "dd if=/dev/zero bs=8192 count=300 2>/dev/null | tr '\\0' x; sleep 5" .to_owned(), + cwd: None, timeout_secs: 10, output_limit: 1024, tool_call_id: Some("tool-high-output".into()), @@ -452,6 +455,7 @@ async fn controller_startup_failure_closes_bound_workdir_session() { session .start_command(CommandRequest { command: "printf unreachable".to_owned(), + cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: None, diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 49909a57..c18546fb 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -15271,6 +15271,7 @@ mod tests { let provider_handle = source .start_command(workdir::CommandRequest { command: "printf ready; sleep 30".to_string(), + cwd: None, timeout_secs: 60, output_limit: 4096, tool_call_id: Some("tool-call-command-session".to_string()), From 060f280fdfb1f55b1b4cb013102d4fb73ef782b8 Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 27 Aug 2026 11:27:27 +0900 Subject: [PATCH 2/3] fix: allow parent commands during write delegation --- crates/tools/src/bash.rs | 15 +- crates/tools/tests/integration.rs | 12 - crates/workdir/src/delegation.rs | 407 ++++++++----------------- crates/workdir/src/local.rs | 42 +-- crates/workdir/src/operation.rs | 5 - crates/worker/tests/controller_test.rs | 4 - crates/workspace-server/src/server.rs | 1 - 7 files changed, 127 insertions(+), 359 deletions(-) diff --git a/crates/tools/src/bash.rs b/crates/tools/src/bash.rs index a8cff604..8224472b 100644 --- a/crates/tools/src/bash.rs +++ b/crates/tools/src/bash.rs @@ -5,9 +5,7 @@ use agen::tool::{Tool, ToolDefinition, ToolError, ToolMeta, ToolOutput}; use async_trait::async_trait; use schemars::JsonSchema; use serde::Deserialize; -use workdir::{ - CommandHandle, CommandOutputRequest, CommandRequest, WorkdirPath, WorkdirSessionHandle, -}; +use workdir::{CommandHandle, CommandOutputRequest, CommandRequest, WorkdirSessionHandle}; const DEFAULT_TIMEOUT_SECS: u64 = 120; const MAX_TIMEOUT_SECS: u64 = 600; @@ -16,10 +14,6 @@ const INLINE_BYTE_BUDGET: usize = 12 * 1024; #[derive(Debug, Deserialize, JsonSchema)] struct BashParams { command: String, - /// Optional logical working directory relative to the bound session cwd. - /// Supplying it lets delegation guards prove the command is disjoint from child write scopes. - #[serde(default)] - cwd: Option, #[serde(default)] timeout: Option, } @@ -57,18 +51,11 @@ impl Tool for BashTool { .timeout .unwrap_or(DEFAULT_TIMEOUT_SECS) .clamp(1, MAX_TIMEOUT_SECS); - let cwd = params - .cwd - .as_deref() - .map(WorkdirPath::new) - .transpose() - .map_err(crate::ToolsError::from)?; let cmd_summary = truncate_for_summary(¶ms.command); let handle = self .session .start_command(CommandRequest { command: params.command, - cwd, timeout_secs, output_limit: INLINE_BYTE_BUDGET, tool_call_id: Some(ctx.call_id), diff --git a/crates/tools/tests/integration.rs b/crates/tools/tests/integration.rs index fd444c7a..f8761aa8 100644 --- a/crates/tools/tests/integration.rs +++ b/crates/tools/tests/integration.rs @@ -390,18 +390,6 @@ async fn bash_inherits_workdir_cwd() { assert_eq!(actual, expected); } -#[tokio::test] -async fn bash_uses_explicit_logical_cwd() { - let (dir, _spill, reg) = setup(); - std::fs::create_dir_all(dir.path().join("nested")).unwrap(); - let bash = reg.get("Bash"); - let out = call(&bash, json!({ "command": "pwd", "cwd": "nested" })).await; - let body = out.content.unwrap(); - let actual = std::fs::canonicalize(body.trim()).unwrap(); - let expected = std::fs::canonicalize(dir.path().join("nested")).unwrap(); - assert_eq!(actual, expected); -} - #[tokio::test] async fn bash_provider_output_does_not_expose_internal_paths() { let (_dir, spill, reg) = setup(); diff --git a/crates/workdir/src/delegation.rs b/crates/workdir/src/delegation.rs index 4ee2b356..031ac9b5 100644 --- a/crates/workdir/src/delegation.rs +++ b/crates/workdir/src/delegation.rs @@ -251,43 +251,8 @@ impl DelegatingWorkdirSession { self.ensure_path(path, WorkdirDelegationPermission::Write) } - fn resolve_command_cwd(&self, cwd: Option<&FsPath>) -> Result { - match cwd { - Some(cwd) => self.resolve_path(cwd), - None => Ok(self.cwd.clone()), - } - } - - fn ensure_command_start(&self, cwd: &FsPath) -> Result<(), WorkdirError> { - self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; - if let Some(scope) = &self.scope - && !scope.iter().any(|rule| { - rule.permission == WorkdirDelegationPermission::Write - && rule_allows_path(rule, cwd, WorkdirDelegationPermission::Write) - }) - { - return Err(WorkdirError::Denied(format!( - "command cwd `{cwd}` is outside the delegated write scope" - ))); - } - - let mut leases = self - .child_write_leases - .lock() - .expect("workdir delegation lease mutex poisoned"); - leases.retain(|_, lease| lease.validity.upgrade().is_some_and(|v| v.is_active())); - if leases.values().any(|lease| { - lease.rules.iter().any(|rule| { - rule.permission == WorkdirDelegationPermission::Write - && command_cwd_overlaps_rule(cwd, rule) - }) - }) { - Err(WorkdirError::Denied(format!( - "command cwd `{cwd}` overlaps a child write delegation" - ))) - } else { - Ok(()) - } + fn ensure_command(&self) -> Result<(), WorkdirError> { + self.ensure_capability(WorkdirSessionCapability::Command, "command execution") } fn ensure_parent_write_available(&self, path: &FsPath) -> Result<(), WorkdirError> { @@ -517,20 +482,13 @@ impl WorkdirSession for DelegatingWorkdirSession { self.source.grep(request).await } - async fn start_command( - &self, - mut request: CommandRequest, - ) -> Result { - let cwd = self.resolve_command_cwd(request.cwd.as_ref())?; - self.ensure_command_start(&cwd)?; - if !self.source.transports_delegation_context() { - request.cwd = Some(cwd); - } + async fn start_command(&self, request: CommandRequest) -> Result { + self.ensure_command()?; self.source.start_command(request).await } async fn command_status(&self, handle: CommandHandle) -> Result { - self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; + self.ensure_command()?; self.source.command_status(handle).await } @@ -538,12 +496,12 @@ impl WorkdirSession for DelegatingWorkdirSession { &self, request: CommandOutputRequest, ) -> Result { - self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; + self.ensure_command()?; self.source.command_output(request).await } async fn cancel_command(&self, handle: CommandHandle) -> Result<(), WorkdirError> { - self.ensure_capability(WorkdirSessionCapability::Command, "command execution")?; + self.ensure_command()?; self.source.cancel_command(handle).await } @@ -671,11 +629,6 @@ impl WorkdirSession for ReadOnlyWorkdirSession { } } -fn command_cwd_overlaps_rule(cwd: &FsPath, rule: &WorkdirDelegationRule) -> bool { - rule_allows_path(rule, cwd, WorkdirDelegationPermission::Write) - || Path::new(rule.target.as_str()).starts_with(Path::new(cwd.as_str())) -} - fn rule_allows_path( rule: &WorkdirDelegationRule, path: &FsPath, @@ -780,6 +733,31 @@ mod tests { } } + async fn run_command( + session: &WorkdirSessionHandle, + command: impl Into, + tool_call_id: impl Into, + ) -> CommandOutput { + let handle = session + .start_command(CommandRequest { + command: command.into(), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some(tool_call_id.into()), + }) + .await + .unwrap(); + session + .command_output(CommandOutputRequest { + handle, + cursor: 0, + limit: 1024, + wait: true, + }) + .await + .unwrap() + } + #[tokio::test] async fn delegation_capable_session_forwards_command_telemetry() { let root = TempDir::new().unwrap(); @@ -790,7 +768,6 @@ mod tests { let handle = parent .start_command(CommandRequest { command: "printf ready; sleep 0.2; printf done".into(), - cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-delegated".into()), @@ -874,6 +851,18 @@ mod tests { ); assert!(child.scoped_session.subscribe_command_events().is_none()); assert!(child.scoped_session.command_snapshot().is_empty()); + assert!(matches!( + child + .scoped_session + .start_command(CommandRequest { + command: "printf denied".into(), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("read-only-command".into()), + }) + .await, + Err(WorkdirError::Denied(_)) + )); } #[cfg(unix)] @@ -945,18 +934,6 @@ mod tests { .await, Err(WorkdirError::Denied(_)) )); - assert!(matches!( - parent - .start_command(CommandRequest { - command: "printf escaped".into(), - cwd: Some(fs_path("granted/outside")), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("symlink-cwd".into()), - }) - .await, - Err(WorkdirError::Denied(message)) if message.contains("traverses a symlink") - )); parent .write(write("secret/parent", "still-authoritative")) .await @@ -964,7 +941,7 @@ mod tests { } #[tokio::test] - async fn write_lease_blocks_only_overlapping_parent_command_cwds_until_release() { + async fn write_lease_keeps_typed_parent_writes_exclusive_without_blocking_commands() { let root = TempDir::new().unwrap(); fs::create_dir_all(root.path().join("leased")).unwrap(); fs::create_dir_all(root.path().join("other")).unwrap(); @@ -978,86 +955,24 @@ mod tests { .capabilities .supports(WorkdirSessionCapability::Command) ); - let command = child - .scoped_session - .start_command(CommandRequest { - command: "pwd; printf child-command".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("delegated-child-command".into()), - }) - .await - .unwrap(); - let command_output = child - .scoped_session - .command_output(CommandOutputRequest { - handle: command, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!( - command_output.content.ends_with("leased\nchild-command"), - "child command must run from its delegated cwd: {}", - command_output.content - ); - let denied = parent - .start_command(CommandRequest { - command: "printf parent-command".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("blocked-parent-command".into()), - }) - .await - .unwrap_err(); - assert!(matches!( - denied, - WorkdirError::Denied(message) - if message.contains("command cwd `.` overlaps a child write delegation") - )); - let denied = parent - .start_command(CommandRequest { - command: "printf still-denied".into(), - cwd: Some(fs_path("leased")), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("overlapping-parent-command".into()), - }) - .await - .unwrap_err(); - assert!(matches!( - denied, - WorkdirError::Denied(message) - if message.contains("command cwd `leased` overlaps a child write delegation") - )); - - let unrelated = parent - .start_command(CommandRequest { - command: "pwd; printf parent-command".into(), - cwd: Some(fs_path("other")), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("unrelated-parent-command".into()), - }) - .await - .unwrap(); - let unrelated_output = parent - .command_output(CommandOutputRequest { - handle: unrelated, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!( - unrelated_output.content.ends_with("other\nparent-command"), - "parent command must run from its explicit disjoint cwd: {}", - unrelated_output.content + let child_output = run_command( + &child.scoped_session, + "printf child-command", + "delegated-child-command", + ) + .await; + assert_eq!(child_output.content, "child-command"); + let parent_output = run_command( + &parent, + "printf parent-write > leased/from-command; printf parent-command", + "parent-command-during-child-write", + ) + .await; + assert_eq!(parent_output.status, CommandStatus::Completed); + assert_eq!(parent_output.content, "parent-command"); + assert_eq!( + fs::read_to_string(root.path().join("leased/from-command")).unwrap(), + "parent-write" ); assert!(matches!( @@ -1071,26 +986,18 @@ mod tests { .await .unwrap(); child.release(); - let resumed = parent - .start_command(CommandRequest { - command: "printf resumed".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("resumed-parent-command".into()), - }) - .await - .unwrap(); - let resumed_output = parent - .command_output(CommandOutputRequest { - handle: resumed, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert_eq!(resumed_output.content, "resumed"); + assert!(matches!( + child + .scoped_session + .start_command(CommandRequest { + command: "printf revoked".into(), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("revoked-child-command".into()), + }) + .await, + Err(WorkdirError::SessionClosed) + )); parent .write(write("leased/parent", "parent")) .await @@ -1143,150 +1050,75 @@ mod tests { } #[tokio::test] - async fn nested_write_delegation_uses_each_session_cwd_without_widening_scope() { + async fn nested_write_leases_do_not_block_command_capable_ancestors() { let root = TempDir::new().unwrap(); - fs::create_dir_all(root.path().join("top/nested")).unwrap(); - fs::create_dir_all(root.path().join("top/peer")).unwrap(); - fs::create_dir_all(root.path().join("other")).unwrap(); + fs::create_dir_all(root.path().join("docs/sub")).unwrap(); let root_session = session(root.path()); let child = root_session - .delegate(request("top", WorkdirDelegationPermission::Write)) + .delegate(request("docs", WorkdirDelegationPermission::Write)) .await .unwrap(); let nested = child .scoped_session - .delegate(WorkdirDelegationRequest { - rules: vec![WorkdirDelegationRule { - target: fs_path("top/nested"), - permission: WorkdirDelegationPermission::Write, - recursive: true, - }], - cwd: fs_path("top/nested"), - }) + .delegate(request("docs/sub", WorkdirDelegationPermission::Write)) .await .unwrap(); - let nested_handle = nested - .scoped_session - .start_command(CommandRequest { - command: "pwd; printf nested".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("nested-command".into()), - }) - .await - .unwrap(); - let nested_output = nested - .scoped_session - .command_output(CommandOutputRequest { - handle: nested_handle, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!(nested_output.content.ends_with("top/nested\nnested")); + for (session, label) in [ + (&root_session, "root"), + (&child.scoped_session, "child"), + (&nested.scoped_session, "nested"), + ] { + let output = run_command( + session, + format!("printf {label}"), + format!("{label}-command-during-nested-write"), + ) + .await; + assert_eq!(output.status, CommandStatus::Completed); + assert_eq!(output.content, label); + } - let denied = child - .scoped_session - .start_command(CommandRequest { - command: "printf blocked".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("nested-overlap".into()), - }) - .await - .unwrap_err(); assert!(matches!( - denied, - WorkdirError::Denied(message) - if message.contains("command cwd `top` overlaps a child write delegation") + root_session.write(write("docs/root", "blocked")).await, + Err(WorkdirError::Denied(_)) )); - - let peer_handle = child + assert!(matches!( + child + .scoped_session + .write(write("sub/child", "blocked")) + .await, + Err(WorkdirError::Denied(_)) + )); + nested .scoped_session - .start_command(CommandRequest { - command: "pwd; printf peer".into(), - cwd: Some(fs_path("peer")), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("nested-peer".into()), - }) + .write(write("nested", "allowed")) .await .unwrap(); - let peer_output = child - .scoped_session - .command_output(CommandOutputRequest { - handle: peer_handle, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!(peer_output.content.ends_with("top/peer\npeer")); - - let outside_handle = root_session - .start_command(CommandRequest { - command: "pwd; printf outside".into(), - cwd: Some(fs_path("other")), - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("root-outside".into()), - }) - .await - .unwrap(); - let outside_output = root_session - .command_output(CommandOutputRequest { - handle: outside_handle, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!(outside_output.content.ends_with("other\noutside")); nested.release(); child.release(); } #[tokio::test] - async fn reapplied_delegation_chain_preserves_command_cwd() { + async fn reapplied_write_delegation_chain_forwards_command_lifecycle() { let root = TempDir::new().unwrap(); fs::create_dir_all(root.path().join("delegated")).unwrap(); - let parent = session(root.path()); let applied = apply_delegation_chain( - parent, + session(root.path()), [request("delegated", WorkdirDelegationPermission::Write)], ) .await .unwrap(); - let scoped = &applied.scoped_session; - let handle = scoped - .start_command(CommandRequest { - command: "pwd; printf reapplied".into(), - cwd: None, - timeout_secs: 5, - output_limit: 1024, - tool_call_id: Some("reapplied-command".into()), - }) - .await - .unwrap(); - let output = scoped - .command_output(CommandOutputRequest { - handle, - cursor: 0, - limit: 1024, - wait: true, - }) - .await - .unwrap(); - assert!(output.content.ends_with("delegated\nreapplied")); + let output = run_command( + &applied.scoped_session, + "printf reapplied", + "reapplied-command", + ) + .await; + assert_eq!(output.status, CommandStatus::Completed); + assert_eq!(output.content, "reapplied"); } #[tokio::test] @@ -1333,6 +1165,17 @@ mod tests { .unwrap(); parent.close().await.unwrap(); + assert!(matches!( + parent + .start_command(CommandRequest { + command: "printf closed".into(), + timeout_secs: 5, + output_limit: 1024, + tool_call_id: Some("closed-parent-command".into()), + }) + .await, + Err(WorkdirError::SessionClosed) + )); assert!(matches!( child.scoped_session.read(read("a")).await, Err(WorkdirError::SessionClosed) diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index 8b422970..16686cb4 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -548,40 +548,6 @@ impl LocalWorkdirSession { self.inner.root.join(path.as_str()) } } - - fn resolve_command_cwd(&self, cwd: Option<&WorkdirPath>) -> Result { - let Some(cwd) = cwd else { - return Ok(self.inner.cwd.clone()); - }; - let host_cwd = self.resolve(cwd); - let canonical_root = self - .inner - .root - .canonicalize() - .map_err(|error| WorkdirError::io(&self.inner.root, error))?; - let expected = if cwd.is_root() { - canonical_root - } else { - canonical_root.join(cwd.as_str()) - }; - let resolved = host_cwd - .canonicalize() - .map_err(|error| WorkdirError::io(&host_cwd, error))?; - if resolved != expected { - return Err(WorkdirError::Denied(format!( - "command cwd `{cwd}` traverses a symlink" - ))); - } - let scope = self.inner.scope.snapshot(); - if !scope.is_readable(&resolved) - || !std::fs::metadata(&resolved).is_ok_and(|metadata| metadata.is_dir()) - { - return Err(WorkdirError::Denied(format!( - "command cwd `{cwd}` is not a readable Workdir directory" - ))); - } - Ok(resolved) - } } #[async_trait] @@ -727,7 +693,7 @@ impl WorkdirSession for LocalWorkdirSession { self.ensure_open()?; let id = self.inner.next_command_id.fetch_add(1, Ordering::Relaxed); let handle = CommandHandle(format!("command-{id}")); - let cwd = self.resolve_command_cwd(request.cwd.as_ref())?; + let cwd = self.inner.cwd.clone(); let (completion_tx, completion) = watch::channel(false); let command_id = handle.0.clone(); let telemetry = self.inner.command_telemetry.clone(); @@ -1472,7 +1438,6 @@ mod tests { &session, CommandRequest { command: "sleep 30".to_owned(), - cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, @@ -1999,7 +1964,6 @@ mod tests { &workdir, CommandRequest { command: "pwd && printf provider-command".into(), - cwd: None, timeout_secs: 5, output_limit: 4096, tool_call_id: None, @@ -2035,7 +1999,6 @@ mod tests { &workdir, CommandRequest { command: "printf 'aéz'".into(), - cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: None, @@ -2259,7 +2222,6 @@ mod tests { &workdir, CommandRequest { command: "printf ready; printf warning >&2; sleep 0.2; printf done".into(), - cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-7".into()), @@ -2363,7 +2325,6 @@ mod tests { &workdir, CommandRequest { command: "sleep 30".into(), - cwd: None, timeout_secs: 1, output_limit: 1024, tool_call_id: None, @@ -2433,7 +2394,6 @@ mod tests { &workdir, CommandRequest { command: "sleep 30".into(), - cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, diff --git a/crates/workdir/src/operation.rs b/crates/workdir/src/operation.rs index fe19763c..47527ad8 100644 --- a/crates/workdir/src/operation.rs +++ b/crates/workdir/src/operation.rs @@ -1,4 +1,3 @@ -use fs_operation::FsPath; use serde::{Deserialize, Serialize}; #[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)] @@ -8,10 +7,6 @@ pub struct CommandHandle(pub String); #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct CommandRequest { pub command: String, - /// Optional logical working directory relative to the calling session's cwd. - /// Providers must resolve and validate it before starting the process. - #[serde(default, skip_serializing_if = "Option::is_none")] - pub cwd: Option, pub timeout_secs: u64, pub output_limit: usize, /// Optional caller-owned correlation id. Bash supplies its tool-call id so diff --git a/crates/worker/tests/controller_test.rs b/crates/worker/tests/controller_test.rs index c4a226a3..3e7e750b 100644 --- a/crates/worker/tests/controller_test.rs +++ b/crates/worker/tests/controller_test.rs @@ -230,7 +230,6 @@ async fn shutdown_closes_bound_workdir_session() { let command = session .start_command(CommandRequest { command: "sleep 30".to_owned(), - cwd: None, timeout_secs: 60, output_limit: 1024, tool_call_id: None, @@ -272,7 +271,6 @@ async fn controller_projects_workdir_command_events_and_snapshot_state() { let command = session .start_command(CommandRequest { command: "printf ready; sleep 0.3; printf done".to_owned(), - cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: Some("tool-command-1".into()), @@ -380,7 +378,6 @@ async fn controller_refreshes_command_snapshot_after_high_output_provider_lag() .start_command(CommandRequest { command: "dd if=/dev/zero bs=8192 count=300 2>/dev/null | tr '\\0' x; sleep 5" .to_owned(), - cwd: None, timeout_secs: 10, output_limit: 1024, tool_call_id: Some("tool-high-output".into()), @@ -455,7 +452,6 @@ async fn controller_startup_failure_closes_bound_workdir_session() { session .start_command(CommandRequest { command: "printf unreachable".to_owned(), - cwd: None, timeout_secs: 5, output_limit: 1024, tool_call_id: None, diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index c18546fb..49909a57 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -15271,7 +15271,6 @@ mod tests { let provider_handle = source .start_command(workdir::CommandRequest { command: "printf ready; sleep 30".to_string(), - cwd: None, timeout_secs: 60, output_limit: 4096, tool_call_id: Some("tool-call-command-session".to_string()), From 0496cd907bc7bb96e9aa1c6d385bedb616bf3233 Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 27 Aug 2026 12:24:19 +0900 Subject: [PATCH 3/3] fix: use Ticket resource keys in handoffs --- crates/tui/src/dashboard/mod.rs | 72 +++++++++++++------ crates/tui/src/dashboard/tests.rs | 24 +++++++ .../src/feature/builtin/orchestration.rs | 51 ++++++++----- crates/worker/src/feature/builtin/ticket.rs | 39 ++++++++-- crates/worker/src/prompt/catalog.rs | 9 ++- crates/workspace-server/src/server.rs | 46 +++++++++--- resources/prompts/catalog.dcdl | 4 -- .../worker/ticket_event_companion_notice.md | 7 -- 8 files changed, 187 insertions(+), 65 deletions(-) delete mode 100644 resources/prompts/worker/ticket_event_companion_notice.md diff --git a/crates/tui/src/dashboard/mod.rs b/crates/tui/src/dashboard/mod.rs index 3ef57175..7216a8c4 100644 --- a/crates/tui/src/dashboard/mod.rs +++ b/crates/tui/src/dashboard/mod.rs @@ -581,6 +581,7 @@ pub(crate) enum IntakeRegistryUpdate { pub(crate) struct ReadyTicketPlanningReturnRequest { workspace_root: PathBuf, ticket_id: String, + ticket_key: String, user_instruction: String, followup: ReadyTicketPlanningReturnFollowup, } @@ -2042,11 +2043,18 @@ impl DashboardApp { return None; }; let ticket_id = ticket.id.clone(); + let ticket_key = match required_ticket_handoff_key(ticket.resource_key.as_deref()) { + Ok(ticket_key) => ticket_key.to_string(), + Err(error) => { + self.notice = Some(error); + return None; + } + }; let mut context = TicketRoleLaunchContext::new(current_workspace_root(), TicketRole::Intake); context.ticket = Some(TicketRef::id(ticket_id.clone())); context.user_instruction = Some(format!( - "Continue Intake for existing Ticket {ticket_id}. Do not create a duplicate Ticket unless the user explicitly requests one. Read ShowTicket body/thread/artifacts before making routing or requirements decisions." + "Continue Intake for existing Ticket {ticket_key}. Do not create a duplicate Ticket unless the user explicitly requests one. Read ShowTicket body/thread/artifacts before making routing or requirements decisions." )); let store = match PanelRegistryStore::default_for_workspace(&context.workspace_root) { Ok(store) => store, @@ -2059,7 +2067,7 @@ impl DashboardApp { Ok(Some(claim)) => { let status = local_claim_status_for_pod(&claim.worker_name, &self.list); self.notice = Some(existing_ticket_claim_notice( - &ticket_id, + &ticket_key, &claim.worker_name, status, )); @@ -2087,7 +2095,7 @@ impl DashboardApp { self.sending = true; self.notice = Some(format!( "Launching Ticket Intake for {} as {}…", - ticket_id, planned.worker_name + ticket_key, planned.worker_name )); Some(IntakeLaunchRequest { context, @@ -2158,10 +2166,17 @@ impl DashboardApp { return None; }; let ticket_id = ticket.id.clone(); + let ticket_key = match required_ticket_handoff_key(ticket.resource_key.as_deref()) { + Ok(ticket_key) => ticket_key.to_string(), + Err(error) => { + self.notice = Some(error); + return None; + } + }; if ticket.workflow_state != TicketWorkflowState::Ready { self.notice = Some(format!( "Ticket {} is {}; expected ready before returning to planning.", - ticket_id, + ticket_key, ticket.workflow_state.as_str() )); return None; @@ -2213,7 +2228,7 @@ impl DashboardApp { TicketRoleLaunchContext::new(workspace_root.clone(), TicketRole::Intake); context.ticket = Some(TicketRef::id(ticket_id.clone())); context.user_instruction = Some(build_ready_ticket_refinement_launch_instruction( - &ticket_id, + &ticket_key, &user_instruction, )); let peer_registration = self.prepare_intake_peer_registration(&mut context); @@ -2237,11 +2252,12 @@ impl DashboardApp { self.sending = true; self.notice = Some(format!( "Returning ready Ticket {} to planning for refinement…", - ticket_id + ticket_key )); Some(ReadyTicketPlanningReturnRequest { workspace_root, ticket_id, + ticket_key, user_instruction, followup, }) @@ -3918,21 +3934,35 @@ fn bounded_refinement_instruction(input: &str) -> String { .to_string() } -fn build_ready_ticket_refinement_thread_body(ticket_id: &str, instruction: &str) -> String { +fn required_ticket_handoff_key(resource_key: Option<&str>) -> Result<&str, String> { + let resource_key = resource_key.ok_or_else(|| { + "Ticket handoff is unavailable because the canonical T-* resource key is missing. Refresh the panel and retry." + .to_string() + })?; + let sequence = resource_key.strip_prefix("T-").filter(|sequence| { + !sequence.is_empty() && sequence.bytes().all(|byte| byte.is_ascii_digit()) + }); + sequence.map(|_| resource_key).ok_or_else(|| { + "Ticket handoff is unavailable because the canonical T-* resource key is invalid. Refresh the panel and retry." + .to_string() + }) +} + +fn build_ready_ticket_refinement_thread_body(ticket_key: &str, instruction: &str) -> String { format!( - "Panel returned ready Ticket {ticket_id} to planning for requirements sync. This is not Queue routing and must not start implementation.\n\n## User refinement instruction\n\n{instruction}\n" + "Panel returned ready Ticket {ticket_key} to planning for requirements sync. This is not Queue routing and must not start implementation.\n\n## User refinement instruction\n\n{instruction}\n" ) } -fn build_ready_ticket_refinement_launch_instruction(ticket_id: &str, instruction: &str) -> String { +fn build_ready_ticket_refinement_launch_instruction(ticket_key: &str, instruction: &str) -> String { format!( - "Continue Ticket Intake / requirements sync for existing Ticket {ticket_id}. The Panel has returned the Ticket from ready to planning; do not queue the Ticket, do not route implementation, and do not create a duplicate unless the user explicitly asks for one. Read ShowTicket body/thread/artifacts before making requirements or readiness decisions.\n\nUser refinement instruction:\n\n{instruction}" + "Continue Ticket Intake / requirements sync for existing Ticket {ticket_key}. The Panel has returned the Ticket from ready to planning; do not queue the Ticket, do not route implementation, and do not create a duplicate unless the user explicitly asks for one. Read ShowTicket body/thread/artifacts before making requirements or readiness decisions.\n\nUser refinement instruction:\n\n{instruction}" ) } -fn build_ready_ticket_refinement_notify(ticket_id: &str, instruction: &str) -> String { +fn build_ready_ticket_refinement_notify(ticket_key: &str, instruction: &str) -> String { format!( - "Ticket {ticket_id} was returned from ready to planning from the Panel for requirements sync. Continue Intake/refinement only; do not Queue or route implementation. Read the Ticket thread for the recorded state change and user instruction.\n\nUser refinement instruction:\n\n{instruction}" + "Ticket {ticket_key} was returned from ready to planning from the Panel for requirements sync. Continue Intake/refinement only; do not Queue or route implementation. Read the Ticket thread for the recorded state change and user instruction.\n\nUser refinement instruction:\n\n{instruction}" ) } @@ -3961,10 +3991,12 @@ async fn dispatch_ready_ticket_planning_return( let ticket = backend .show(id.clone()) .map_err(|error| TicketActionError::Ticket(error.to_string()))?; + let ticket_key = + required_ticket_handoff_key(Some(&request.ticket_key)).map_err(TicketActionError::Stale)?; if ticket.meta.workflow_state != TicketWorkflowState::Ready { return Err(TicketActionError::Stale(format!( "Ticket {} is {}; expected ready before returning it to planning. Refresh the panel and retry if appropriate.", - ticket.meta.id, + ticket_key, ticket.meta.workflow_state.as_str() ))); } @@ -3973,7 +4005,7 @@ async fn dispatch_ready_ticket_planning_return( TicketWorkflowState::Planning.as_str(), "panel_return_to_planning", MarkdownText::from(build_ready_ticket_refinement_thread_body( - &ticket.meta.id, + ticket_key, &request.user_instruction, )), ); @@ -3987,7 +4019,7 @@ async fn dispatch_ready_ticket_planning_return( ReadyTicketPlanningReturnOutcome { notice: format!( "Ticket {} returned to planning for refinement; launching Ticket Intake…", - ticket.meta.id + ticket_key ), followup: ReadyTicketPlanningReturnAfterMutation::LaunchIntake(request), } @@ -3997,19 +4029,19 @@ async fn dispatch_ready_ticket_planning_return( socket_path, } => { let message = - build_ready_ticket_refinement_notify(&ticket.meta.id, &request.user_instruction); + build_ready_ticket_refinement_notify(ticket_key, &request.user_instruction); match send_notify_only(&socket_path, message, true).await { Ok(()) => ReadyTicketPlanningReturnOutcome { notice: format!( "Ticket {} returned to planning for refinement; notified live Intake Worker {}.", - ticket.meta.id, worker_name + ticket_key, worker_name ), followup: ReadyTicketPlanningReturnAfterMutation::None, }, Err(error) => ReadyTicketPlanningReturnOutcome { notice: bounded_panel_diagnostic(format!( "Ticket {} returned to planning and instruction was recorded, but notifying Intake Worker {} failed: {}", - ticket.meta.id, worker_name, error + ticket_key, worker_name, error )), followup: ReadyTicketPlanningReturnAfterMutation::None, }, @@ -4020,7 +4052,7 @@ async fn dispatch_ready_ticket_planning_return( ReadyTicketPlanningReturnOutcome { notice: format!( "Ticket {} returned to planning for refinement; opening/restoring claimed Intake Worker {}…", - ticket.meta.id, worker_name + ticket_key, worker_name ), followup: ReadyTicketPlanningReturnAfterMutation::OpenClaim(request), } @@ -4029,7 +4061,7 @@ async fn dispatch_ready_ticket_planning_return( ReadyTicketPlanningReturnOutcome { notice: bounded_panel_diagnostic(format!( "Ticket {} returned to planning and instruction was recorded, but Intake launch was not attempted because existing Intake claim {} is stale; inspect or clear the local claim before launching another Intake Worker.", - ticket.meta.id, worker_name + ticket_key, worker_name )), followup: ReadyTicketPlanningReturnAfterMutation::None, } diff --git a/crates/tui/src/dashboard/tests.rs b/crates/tui/src/dashboard/tests.rs index 24f32f3e..62d72915 100644 --- a/crates/tui/src/dashboard/tests.rs +++ b/crates/tui/src/dashboard/tests.rs @@ -390,6 +390,7 @@ fn planning_return_request( ReadyTicketPlanningReturnRequest { workspace_root: temp.path().to_path_buf(), ticket_id, + ticket_key: "T-482".to_string(), user_instruction: instruction.to_string(), followup: ReadyTicketPlanningReturnFollowup::BlockedByStaleClaim { worker_name: "stale-intake".to_string(), @@ -494,6 +495,7 @@ fn ready_ticket_intake_enter_prepares_planning_return_not_queue_or_generic_launc }; assert_eq!(request.ticket_id, "20260608-000123-ready"); + assert_eq!(request.ticket_key, "T-1"); assert_eq!(request.user_instruction, "clarify expected behavior"); assert!(matches!( request.followup, @@ -515,6 +517,7 @@ async fn planning_return_with_launch_followup_changes_state_before_launch_follow let request = ReadyTicketPlanningReturnRequest { workspace_root: temp.path().to_path_buf(), ticket_id: ticket_id.clone(), + ticket_key: "T-482".to_string(), user_instruction: "launch intake after state change".to_string(), followup: ReadyTicketPlanningReturnFollowup::LaunchIntake(IntakeLaunchRequest { context: TicketRoleLaunchContext::new(temp.path().to_path_buf(), TicketRole::Intake), @@ -3425,6 +3428,27 @@ fn ticket_action_error_records_f2_diagnostic_details() { assert!(!app.panel_diagnostic_open); } +#[test] +fn ready_ticket_refinement_projection_uses_only_canonical_resource_key() { + const INTERNAL_ID: &str = "00001KZVNXFNK"; + let thread = build_ready_ticket_refinement_thread_body("T-482", "Clarify rollback."); + let launch = build_ready_ticket_refinement_launch_instruction("T-482", "Clarify rollback."); + let notify = build_ready_ticket_refinement_notify("T-482", "Clarify rollback."); + + for projection in [&thread, &launch, ¬ify] { + assert!(projection.contains("T-482")); + assert!(!projection.contains(INTERNAL_ID)); + } +} + +#[test] +fn ticket_handoff_fails_closed_without_canonical_resource_key() { + assert_eq!(required_ticket_handoff_key(Some("T-482")), Ok("T-482")); + for invalid in [None, Some(""), Some("00001KZVNXFNK"), Some("T-key")] { + assert!(required_ticket_handoff_key(invalid).is_err()); + } +} + fn plain_line(line: &Line<'_>) -> String { line.spans .iter() diff --git a/crates/worker/src/feature/builtin/orchestration.rs b/crates/worker/src/feature/builtin/orchestration.rs index 1d623f39..0ad3cdb9 100644 --- a/crates/worker/src/feature/builtin/orchestration.rs +++ b/crates/worker/src/feature/builtin/orchestration.rs @@ -89,18 +89,19 @@ impl Tool for SpawnTicketCoderTool { let input: SpawnTicketCoderInput = serde_json::from_str(input_json).map_err(|error| { ToolError::InvalidArgument(format!("invalid {TOOL_NAME} input: {error}")) })?; - let ticket_id = authority_id(input.ticket_id, "ticket_id")?; - let workflow_state = self + let ticket_ref = authority_id(input.ticket_id, "ticket_id")?; + let ticket = self .ticket_service - .workflow_state(&ticket_id) + .ticket_handoff(&ticket_ref) .map_err(|error| ToolError::ExecutionFailed(error.to_string()))?; if !matches!( - workflow_state, + ticket.workflow_state, ticket::TicketWorkflowState::Queued | ticket::TicketWorkflowState::InProgress ) { return Err(ToolError::ExecutionFailed(format!( - "Ticket {ticket_id} must be queued or inprogress before spawning its Coder; current state is {}", - workflow_state.as_str() + "Ticket {} must be queued or inprogress before spawning its Coder; current state is {}", + ticket.resource_key, + ticket.workflow_state.as_str() ))); } let call_id = non_empty(ctx.call_id, "tool call_id")?; @@ -115,14 +116,14 @@ impl Tool for SpawnTicketCoderTool { )?, relative_cwd, profile: CODER_PROFILE.to_string(), - ticket_id: Some(ticket_id.clone()), - operation_id: Some(format!("spawn-ticket-coder:{ticket_id}:{call_id}")), - display_name: format!("Coder · {ticket_id}"), + ticket_id: Some(ticket.id.clone()), + operation_id: Some(format!("spawn-ticket-coder:{}:{call_id}", ticket.id)), + display_name: format!("Coder · {}", ticket.resource_key), initial_submit: vec![ Segment::Flow { selector: CODER_FLOW.to_string(), }, - Segment::text(format!("Implement Ticket {ticket_id}.")), + Segment::text(format!("Implement Ticket {}.", ticket.resource_key)), ], }) .await @@ -134,7 +135,7 @@ impl Tool for SpawnTicketCoderTool { ))); } Ok(ToolOutput { - summary: format!("Spawned Coder for Ticket {ticket_id}"), + summary: format!("Spawned Coder for Ticket {}", ticket.resource_key), content: Some(response.body), attachments: Vec::new(), }) @@ -201,21 +202,31 @@ mod tests { use crate::worker::{WorkspaceClientError, WorkspaceResponse}; use super::*; + use crate::feature::builtin::ticket::TicketHandoff; #[derive(Default)] struct RecordingTicketService; impl TicketService for RecordingTicketService { - fn workflow_state(&self, _ticket_id: &str) -> Result { - Ok(TicketWorkflowState::Queued) + fn ticket_handoff(&self, ticket_ref: &str) -> Result { + assert_eq!(ticket_ref, "T-482"); + Ok(TicketHandoff { + id: "00001KZXN51C7".to_string(), + resource_key: "T-482".to_string(), + workflow_state: TicketWorkflowState::Queued, + }) } } struct FixedTicketService(TicketWorkflowState); impl TicketService for FixedTicketService { - fn workflow_state(&self, _ticket_id: &str) -> Result { - Ok(self.0) + fn ticket_handoff(&self, _ticket_ref: &str) -> Result { + Ok(TicketHandoff { + id: "00001KZXN51C7".to_string(), + resource_key: "T-482".to_string(), + workflow_state: self.0, + }) } } @@ -247,7 +258,7 @@ mod tests { }; tool.execute( &serde_json::json!({ - "ticket_id": "00001KZXN51C7", + "ticket_id": "T-482", "runtime_id": "runtime-1", "working_directory_id": "workdir-1" }) @@ -265,16 +276,20 @@ mod tests { request.operation_id.as_deref(), Some("spawn-ticket-coder:00001KZXN51C7:call-7") ); - assert_eq!(request.display_name, "Coder · 00001KZXN51C7"); + assert_eq!(request.display_name, "Coder · T-482"); assert_eq!( request.initial_submit, vec![ Segment::Flow { selector: CODER_FLOW.to_string() }, - Segment::text("Implement Ticket 00001KZXN51C7.") + Segment::text("Implement Ticket T-482.") ] ); + assert!(!request.display_name.contains("00001KZXN51C7")); + assert!(request.initial_submit.iter().all(|segment| { + !Segment::flatten_to_text(std::slice::from_ref(segment)).contains("00001KZXN51C7") + })); } #[tokio::test] diff --git a/crates/worker/src/feature/builtin/ticket.rs b/crates/worker/src/feature/builtin/ticket.rs index 9001d6bb..ae39e3cb 100644 --- a/crates/worker/src/feature/builtin/ticket.rs +++ b/crates/worker/src/feature/builtin/ticket.rs @@ -267,7 +267,20 @@ pub const TICKET_SERVICE_ID: &str = "ticket.authority"; const TICKET_SERVICE_VERSION: &str = "1"; pub trait TicketService: Send + Sync { - fn workflow_state(&self, ticket_id: &str) -> Result; + fn ticket_handoff(&self, ticket_ref: &str) -> Result; +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct TicketHandoff { + pub id: String, + pub resource_key: String, + pub workflow_state: TicketWorkflowState, +} + +fn is_canonical_ticket_resource_key(resource_key: &str) -> bool { + resource_key.strip_prefix("T-").is_some_and(|sequence| { + !sequence.is_empty() && sequence.bytes().all(|byte| byte.is_ascii_digit()) + }) } struct BackendTicketService { @@ -275,10 +288,18 @@ struct BackendTicketService { } impl TicketService for BackendTicketService { - fn workflow_state(&self, ticket_id: &str) -> Result { - self.backend - .show(ticket_id.into()) - .map(|ticket| ticket.meta.workflow_state) + fn ticket_handoff(&self, ticket_ref: &str) -> Result { + let ticket = self.backend.show(ticket_ref.into())?; + let resource_key = ticket + .meta + .resource_key + .filter(|key| is_canonical_ticket_resource_key(key)) + .ok_or_else(|| TicketError::Conflict("ticket resource key is unavailable".into()))?; + Ok(TicketHandoff { + id: ticket.meta.id, + resource_key, + workflow_state: ticket.meta.workflow_state, + }) } } @@ -1770,6 +1791,14 @@ provider = "github" assert_eq!(removed.target, "01TARGET"); } + #[test] + fn ticket_handoff_accepts_only_canonical_ticket_resource_keys() { + assert!(is_canonical_ticket_resource_key("T-482")); + for invalid in ["", "00001KZVNXFNK", "T-", "T-key", "O-482"] { + assert!(!is_canonical_ticket_resource_key(invalid)); + } + } + #[test] fn workspace_http_backend_executes_ticket_create_operation() { let listener = TcpListener::bind("127.0.0.1:0").unwrap(); diff --git a/crates/worker/src/prompt/catalog.rs b/crates/worker/src/prompt/catalog.rs index 0922349b..a02d7871 100644 --- a/crates/worker/src/prompt/catalog.rs +++ b/crates/worker/src/prompt/catalog.rs @@ -102,7 +102,6 @@ pub enum WorkerPrompt { AgentsMdSection, ResidentMemorySummarySection, WorkerOrchestrationGuidanceSection, - TicketEventCompanionNotice, SubWorkerSpawnToolDescription, } @@ -122,7 +121,6 @@ impl WorkerPrompt { Self::WorkerOrchestrationGuidanceSection => { "internal.worker_orchestration_guidance_section" } - Self::TicketEventCompanionNotice => "worker.ticket_event_companion_notice", Self::SubWorkerSpawnToolDescription => "internal.sub_worker_spawn_tool_description", } } @@ -139,7 +137,6 @@ impl WorkerPrompt { WorkerPrompt::AgentsMdSection, WorkerPrompt::ResidentMemorySummarySection, WorkerPrompt::WorkerOrchestrationGuidanceSection, - WorkerPrompt::TicketEventCompanionNotice, WorkerPrompt::SubWorkerSpawnToolDescription, ]; } @@ -593,6 +590,12 @@ mod tests { fn builtin_dcdl_catalog_loads() { let catalog = PromptCatalog::builtins_only().unwrap(); assert!(!catalog.projection.templates.is_empty()); + assert!( + !catalog + .projection + .templates + .contains_key("worker.ticket_event_companion_notice") + ); } #[test] diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 49909a57..e46b9d00 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -6414,9 +6414,15 @@ fn worker_ticket_source_context( } } -fn ticket_notification_content(ticket_id: &str, current_state: &str) -> String { +fn canonical_ticket_resource_key(resource_key: &str) -> Option<&str> { + let sequence = resource_key.strip_prefix("T-")?; + (!sequence.is_empty() && sequence.bytes().all(|byte| byte.is_ascii_digit())) + .then_some(resource_key) +} + +fn ticket_notification_content(resource_key: &str, current_state: &str) -> String { format!( - "Ticket notification: ticket_id={ticket_id} current_state={current_state}. Reread the Ticket before acting." + "Ticket {resource_key} changed to {current_state}. Reread the current Ticket before acting." ) } @@ -6498,6 +6504,16 @@ fn notify_ticket_recipients( current_state: &str, source: Option, ) { + let Ok(Some(resource_key)) = + api.store + .resource_key(workspace_id, WorkspaceResourceKind::Ticket, ticket_id) + else { + return; + }; + let Some(resource_key) = canonical_ticket_resource_key(&resource_key) else { + return; + }; + let mut recipients = Vec::new(); if let Some(assignment) = api .store @@ -6518,7 +6534,7 @@ fn notify_ticket_recipients( recipients.sort(); recipients.dedup(); - let content = ticket_notification_content(ticket_id, current_state); + let content = ticket_notification_content(resource_key, current_state); for recipient in recipients { if source.as_ref().is_some_and(|source| source == &recipient) { continue; @@ -18216,15 +18232,25 @@ mod tests { } #[test] - fn ticket_notification_projection_exposes_only_ticket_and_current_state() { + fn ticket_notification_requires_canonical_ticket_resource_key() { + assert_eq!(canonical_ticket_resource_key("T-429"), Some("T-429")); + for invalid in ["", "00001KZ9SR97B", "T-", "T-key", "O-429"] { + assert_eq!(canonical_ticket_resource_key(invalid), None); + } + } + + #[test] + fn ticket_notification_projection_exposes_only_resource_key_and_current_state() { + const INTERNAL_ID: &str = "00001KZ9SR97B"; for current_state in ["queued", "inprogress"] { - let content = ticket_notification_content("00001KZ9SR97B", current_state); + let content = ticket_notification_content("T-429", current_state); assert_eq!( content, format!( - "Ticket notification: ticket_id=00001KZ9SR97B current_state={current_state}. Reread the Ticket before acting." + "Ticket T-429 changed to {current_state}. Reread the current Ticket before acting." ) ); + assert!(!content.contains(INTERNAL_ID)); for forbidden in [ "workspace_id", "event_sequence", @@ -18402,9 +18428,13 @@ mod tests { assert_eq!(inputs.len(), expected_states.len()); for ((recipient, content), current_state) in inputs.iter().zip(expected_states) { assert_eq!(recipient.worker_id.to_string(), orchestrator.worker_id); + assert!(!content.contains(&ticket.id)); assert_eq!( content, - &ticket_notification_content(&ticket.id, current_state) + &ticket_notification_content( + ticket.resource_key.as_deref().unwrap(), + current_state, + ) ); } } @@ -19520,7 +19550,7 @@ mod tests { assert_eq!( notifications[0].1, ticket_notification_content( - ticket_ref.id.as_str(), + ticket_ref.resource_key.as_deref().unwrap(), TicketWorkflowState::Queued.as_str() ) ); diff --git a/resources/prompts/catalog.dcdl b/resources/prompts/catalog.dcdl index 1144f3a3..caa82038 100644 --- a/resources/prompts/catalog.dcdl +++ b/resources/prompts/catalog.dcdl @@ -30,7 +30,6 @@ internalAgentsMdSection = import "./internal/agents_md_section.md"; internalResidentMemorySummarySection = import "./internal/resident_memory_summary_section.md"; internalSubWorkerSpawnToolDescription = import "./internal/sub_worker_spawn_tool_description.md"; panelOrchestratorIdleQueueNotice = import "./panel/orchestrator_idle_queue_notice.md"; -workerTicketEventCompanionNotice = import "./worker/ticket_event_companion_notice.md"; in { default_prompt = defaultDocument.content; @@ -69,7 +68,4 @@ in panel = { orchestrator_idle_queue_notice = panelOrchestratorIdleQueueNotice.content; }; - worker = { - ticket_event_companion_notice = workerTicketEventCompanionNotice.content; - }; } diff --git a/resources/prompts/worker/ticket_event_companion_notice.md b/resources/prompts/worker/ticket_event_companion_notice.md deleted file mode 100644 index 1551bdcb..00000000 --- a/resources/prompts/worker/ticket_event_companion_notice.md +++ /dev/null @@ -1,7 +0,0 @@ -Ticket event notice (weak; auto_run=false) -ticket: {{ ticket_id }} -title: {{ title }} -state: {{ state }} -event: {{ event_kind }} -summary: {{ summary }} -ref: {{ ref_path }}