diff --git a/crates/workdir/src/http.rs b/crates/workdir/src/http.rs index d8cc61bb3..940506ca7 100644 --- a/crates/workdir/src/http.rs +++ b/crates/workdir/src/http.rs @@ -716,10 +716,23 @@ mod client { "mismatched response after checkout mutation".into(), )), Ok(_) => Err(Self::mismatch("checkout_execute")), - Err(WorkdirError::Unavailable(_) | WorkdirError::Transport(_)) if mutation => { - Err(WorkdirError::OutcomeUnknown( + Err(error) + if mutation + && matches!( + error.classification_source(), + WorkdirError::Unavailable(_) | WorkdirError::Transport(_) + ) => + { + let outcome = WorkdirError::OutcomeUnknown( "checkout transport failed; effects may have occurred".into(), - )) + ); + Err(match error.denial_reason() { + Some(reason) => WorkdirError::DenialContext { + reason, + source: Box::new(outcome), + }, + None => outcome, + }) } Err(error) => Err(error), } diff --git a/crates/workdir/src/router.rs b/crates/workdir/src/router.rs index 02e00004d..54e5c538b 100644 --- a/crates/workdir/src/router.rs +++ b/crates/workdir/src/router.rs @@ -166,7 +166,12 @@ impl RoutedSessionState { for handle in handles { match self.session.command_status(handle.clone()).await { Ok(status) => self.mark_terminal(&handle, status), - Err(WorkdirError::UnknownCommand(_)) => { + Err(error) + if matches!( + error.classification_source(), + WorkdirError::UnknownCommand(_) + ) => + { self.active_commands .lock() .unwrap_or_else(std::sync::PoisonError::into_inner) diff --git a/crates/workdir/src/scope.rs b/crates/workdir/src/scope.rs index 4522c7a97..b94a868b1 100644 --- a/crates/workdir/src/scope.rs +++ b/crates/workdir/src/scope.rs @@ -240,8 +240,18 @@ impl WorkdirScopeLease { .expect("scoped command set mutex poisoned") .remove(&command_id); } - (Ok(()), Err(WorkdirError::UnknownCommand(_))) - | (Err(WorkdirError::UnknownCommand(_)), Err(WorkdirError::UnknownCommand(_))) => { + (cancel, Err(error)) + if matches!( + error.classification_source(), + WorkdirError::UnknownCommand(_) + ) && match &cancel { + Ok(()) => true, + Err(error) => matches!( + error.classification_source(), + WorkdirError::UnknownCommand(_) + ), + } => + { self.broker.authority.publish_terminal_if_missing( &command_id, CommandStatus::Cancelled, diff --git a/crates/workdir/tests/command_denial_diagnostics.rs b/crates/workdir/tests/command_denial_diagnostics.rs new file mode 100644 index 000000000..cfc4c5231 --- /dev/null +++ b/crates/workdir/tests/command_denial_diagnostics.rs @@ -0,0 +1,185 @@ +//! Wire diagnostics must not change command disappearance or lease release. +//! Fake command handles only; no OS process is started, polled, or cancelled. +use async_trait::async_trait; +use std::{path::Path, sync::Arc}; +use tempfile::TempDir; +use workdir::*; + +#[derive(Debug)] +struct CommandFailureSession { + local: LocalWorkdirSession, + code: &'static str, + depth: u8, + cancel_ok: bool, +} +impl CommandFailureSession { + fn new(root: &Path, code: &'static str, depth: u8, cancel_ok: bool) -> Arc { + Arc::new(Self { + local: LocalWorkdirSession::new( + manifest::Scope::writable(root).unwrap(), + root.to_path_buf(), + ), + code, + depth, + cancel_ok, + }) + } + fn failure(&self) -> WorkdirError { + let mut error = serde_json::from_value::(serde_json::json!({ + "code": self.code, "message": "safe fixture error", + "denial_reason": (self.depth > 0).then_some(WorkdirDenialReason::ReadOnlySession), + })) + .unwrap() + .into_workdir_error(); + if self.depth == 2 { + error = WorkdirError::DenialContext { + reason: WorkdirDenialReason::OsPermissionDenied, + source: Box::new(error), + }; + } + error + } +} +#[async_trait] +impl WorkdirSession for CommandFailureSession { + fn workdir(&self) -> &Workdir { + self.local.workdir() + } + fn capabilities(&self) -> WorkdirSessionCapabilities { + self.local.capabilities() + } + async fn authorize_scope_path( + &self, + r: WorkdirScopeAuthorizationRequest, + ) -> Result<(), WorkdirError> { + self.local.authorize_scope_path(r).await + } + async fn scope_rules_overlap( + &self, + r: WorkdirScopeOverlapRequest, + ) -> Result { + self.local.scope_rules_overlap(r).await + } + async fn stat(&self, r: StatRequest) -> Result { + self.local.stat(r).await + } + async fn read(&self, r: ReadRequest) -> Result { + self.local.read(r).await + } + async fn write(&self, r: WriteRequest) -> Result { + self.local.write(r).await + } + async fn edit(&self, r: EditRequest) -> Result { + self.local.edit(r).await + } + async fn list(&self, r: ListRequest) -> Result { + self.local.list(r).await + } + async fn glob(&self, r: GlobRequest) -> Result { + self.local.glob(r).await + } + async fn grep(&self, r: GrepRequest) -> Result { + self.local.grep(r).await + } + async fn start_command(&self, _: CommandRequest) -> Result { + Ok(CommandHandle("fake-command".into())) + } + async fn command_status(&self, _: CommandHandle) -> Result { + Err(self.failure()) + } + async fn command_output(&self, _: CommandOutputRequest) -> Result { + Err(self.failure()) + } + async fn cancel_command(&self, _: CommandHandle) -> Result<(), WorkdirError> { + if self.cancel_ok { + Ok(()) + } else { + Err(self.failure()) + } + } + async fn close(&self) -> Result<(), WorkdirError> { + self.local.close().await + } +} +fn command() -> CommandRequest { + CommandRequest { + command: "fake command, never executed".into(), + timeout_secs: 1, + output_limit: 100, + cwd: WorkdirPath::root(), + spill_dir: None, + tool_call_id: None, + } +} +fn scope() -> WorkdirToolScope { + WorkdirToolScope { + rules: vec![WorkdirToolScopeRule { + target: WorkdirPath::root(), + permission: WorkdirToolScopePermission::Write, + recursive: true, + symlink_policy: Default::default(), + }], + command: true, + cwd: WorkdirPath::root(), + } +} + +#[tokio::test] +async fn command_diagnostics_preserve_detach_disappearance_and_busy_guard() { + for code in ["unknown_command", "unavailable"] { + for depth in [0, 1, 2] { + let dir = TempDir::new().unwrap(); + let router = WorkdirSessionRouter::new(); + let alias = WorkdirAttachmentAlias::new("fixture").unwrap(); + router + .attach( + alias.clone(), + CommandFailureSession::new(dir.path(), code, depth, false), + ) + .unwrap(); + let session = router.resolve(Some("fixture")).unwrap().session; + session.start_command(command()).await.unwrap(); + let result = router.detach(&alias).await; + if code == "unknown_command" { + result.unwrap_or_else(|error| panic!("depth {depth}: {error:?}")); + assert!(router.resolve(Some("fixture")).is_err()); + } else { + assert!(matches!(result, Err(WorkdirError::Conflict(_)))); + assert!(router.resolve(Some("fixture")).is_ok()); + } + } + } +} + +#[tokio::test] +async fn command_diagnostics_preserve_scope_close_and_write_lease_guard() { + for code in ["unknown_command", "unavailable"] { + for depth in [0, 1, 2] { + for cancel_ok in [false, true] { + let dir = TempDir::new().unwrap(); + let broker = WorkdirToolBroker::new(CommandFailureSession::new( + dir.path(), + code, + depth, + cancel_ok, + )); + let child = broker.scope(scope()).await.unwrap(); + child.start_command(command()).await.unwrap(); + let result = child.close().await; + if code == "unknown_command" { + result.unwrap_or_else(|error| { + panic!("depth {depth}, cancel_ok={cancel_ok}: {error:?}") + }); + assert!(!child.is_active()); + broker.scope(scope()).await.unwrap().close().await.unwrap(); + } else { + assert!(result.is_err()); + // Closing revokes new work immediately, but failed cleanup + // must retain the write lease until ownership is resolved. + assert!(!child.is_active()); + assert!(broker.scope(scope()).await.is_err()); + } + } + } + } +} diff --git a/crates/workdir/tests/remote_denial_reasons.rs b/crates/workdir/tests/remote_denial_reasons.rs index 8ed2edc0c..f14acc816 100644 --- a/crates/workdir/tests/remote_denial_reasons.rs +++ b/crates/workdir/tests/remote_denial_reasons.rs @@ -4,7 +4,7 @@ use std::{ io::{Read, Write}, net::{TcpListener, TcpStream}, thread, - time::Duration, + time::{Duration, Instant}, }; use workdir::{ StatRequest, Workdir, WorkdirDenialReason as Reason, WorkdirPath, WorkdirSession, @@ -15,7 +15,18 @@ use workdir::{ }; fn reply(listener: &TcpListener, status: &str, body: &str) { - let (mut stream, _) = listener.accept().unwrap(); + listener.set_nonblocking(true).unwrap(); + let deadline = Instant::now() + Duration::from_secs(5); + let (mut stream, _) = loop { + match listener.accept() { + Ok(connection) => break connection, + Err(error) if error.kind() == std::io::ErrorKind::WouldBlock => { + assert!(Instant::now() < deadline, "fake provider accept timed out"); + thread::yield_now(); + } + Err(error) => panic!("fake provider accept failed: {error}"), + } + }; stream .set_read_timeout(Some(Duration::from_secs(5))) .unwrap(); @@ -108,3 +119,100 @@ async fn remote_http_denials_forward_only_typed_reasons_and_old_missing_reason() server.join().unwrap(); } } + +#[tokio::test] +async fn remote_checkout_diagnostics_preserve_mutation_outcome_and_read_failures() { + use workdir::{CheckoutOperation as Op, CheckoutRequest, WorkdirSessionCapability}; + for code in [ + Code::Unavailable, + Code::Transport, + Code::Denied, + Code::Conflict, + Code::OutcomeUnknown, + ] { + for reason in [None, Some(Reason::ReadOnlySession)] { + for operation in [ + Op::Read { + offset: 0, + limit: 1, + max_bytes: 10, + }, + Op::Create { + path: WorkdirPath::new("new.txt").unwrap(), + content: b"fixture".to_vec(), + }, + Op::Write { + content: b"fixture".to_vec(), + expected_hash: [0; 32], + }, + Op::Edit { + old_string: "old".into(), + new_string: "new".into(), + replace_all: false, + expected_hash: [0; 32], + }, + ] { + let mutation = operation.capability() != WorkdirSessionCapability::Read; + let expected_code = + if mutation && matches!(code, Code::Unavailable | Code::Transport) { + Code::OutcomeUnknown + } else { + code + }; + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let address = listener.local_addr().unwrap(); + let error = serde_json::json!({"code":code, "message":"/secret/host provider-secret", "denial_reason":reason}); + let server = thread::spawn(move || { + reply( + &listener, + "200 OK", + r#"{"session_id":"s","workdir_id":"w","capabilities":{"bits":63}}"#, + ); + reply( + &listener, + &format!("{} Fixture failure", code.http_status()), + &error.to_string(), + ); + }); + let remote = RemoteWorkdirSession::open( + reqwest::Client::builder() + .no_proxy() + .timeout(Duration::from_secs(5)) + .build() + .unwrap(), + format!("http://{address}").parse().unwrap(), + "request-secret", + Workdir::new("w").id().clone(), + OpenWorkdirSessionRequest::default(), + ) + .await + .unwrap(); + let error = remote + .checkout_execute(CheckoutRequest { + target: WorkdirPath::root(), + validator: vec![1], + operation, + }) + .await + .unwrap_err(); + server.join().unwrap(); + let forwarded = WorkdirTransportError::from_workdir_error(&error); + assert_eq!( + forwarded.code, expected_code, + "{code:?}, {reason:?}, mutation={mutation}: {error:?}" + ); + assert_eq!(error.denial_reason(), reason); + assert_eq!(forwarded.denial_reason, reason); + assert_eq!(forwarded.code.http_status(), expected_code.http_status()); + if expected_code == Code::OutcomeUnknown { + assert!(forwarded.message.contains("inspect before retrying")); + } + assert!( + !serde_json::to_string(&forwarded) + .unwrap() + .contains("secret") + ); + } + } + } +} diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index bc345138c..fb49f7803 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -1225,12 +1225,23 @@ impl workdir::WorkdirSession for ExternalProviderWorkdirSession { "mismatched checkout mutation response".into(), )), Ok(_) => Err(Self::mismatch("checkout_execute")), - Err(workdir::WorkdirError::Unavailable(_) | workdir::WorkdirError::Transport(_)) - if mutation => + Err(error) + if mutation + && matches!( + error.classification_source(), + workdir::WorkdirError::Unavailable(_) | workdir::WorkdirError::Transport(_) + ) => { - Err(workdir::WorkdirError::OutcomeUnknown( + let outcome = workdir::WorkdirError::OutcomeUnknown( "checkout provider response lost; inspect effects before retry".into(), - )) + ); + Err(match error.denial_reason() { + Some(reason) => workdir::WorkdirError::DenialContext { + reason, + source: Box::new(outcome), + }, + None => outcome, + }) } Err(error) => Err(error), } @@ -33033,6 +33044,7 @@ mod tests { mod drive_api_tests; mod drive_tests; mod drive_web_roundtrip_tests; + mod external_checkout_tests; mod subject_spawn_tests; mod subjektiv_jobs_tests; mod ticket_evidence_tests; diff --git a/crates/workspace-server/src/server/tests/external_checkout_tests.rs b/crates/workspace-server/src/server/tests/external_checkout_tests.rs new file mode 100644 index 000000000..c7619bb36 --- /dev/null +++ b/crates/workspace-server/src/server/tests/external_checkout_tests.rs @@ -0,0 +1,129 @@ +//! External codec -> actual session dispatch -> mutation safety -> HTTP error. +//! Channel fixture only; no product process or real mutation. +use super::*; +use workdir::{ + CheckoutOperation as Op, CheckoutRequest, WorkdirDenialReason as Reason, WorkdirPath, + WorkdirSession, WorkdirSessionCapability, external::ExternalWorkdirOperationOutcome, + http::WorkdirTransportErrorCode as Code, +}; + +#[tokio::test] +async fn external_checkout_diagnostics_preserve_mutation_outcome_and_read_failures() { + for code in [ + Code::Unavailable, + Code::Transport, + Code::Denied, + Code::Conflict, + Code::OutcomeUnknown, + ] { + for reason in [None, Some(Reason::ReadOnlySession)] { + for operation in [ + Op::Read { + offset: 0, + limit: 1, + max_bytes: 10, + }, + Op::Create { + path: WorkdirPath::new("new.txt").unwrap(), + content: b"fixture".to_vec(), + }, + Op::Write { + content: b"fixture".to_vec(), + expected_hash: [0; 32], + }, + Op::Edit { + old_string: "old".into(), + new_string: "new".into(), + replace_all: false, + expected_hash: [0; 32], + }, + ] { + let mutation = operation.capability() != WorkdirSessionCapability::Read; + let expected_code = + if mutation && matches!(code, Code::Unavailable | Code::Transport) { + Code::OutcomeUnknown + } else { + code + }; + let (sender, mut commands) = tokio::sync::mpsc::channel(1); + let connection = Arc::new(ExternalProviderConnection { + grant_id: "fixture-grant".into(), + workdir_id: "fixture-workdir".into(), + provider_instance_id: "fixture-provider".into(), + generation: 1, + expires_at: None, + capabilities: workdir::WorkdirSessionCapabilities::ALL, + admission: Arc::new(tokio::sync::Semaphore::new(16)), + shutdown_confirmed: Arc::new(std::sync::atomic::AtomicBool::new(false)), + sender, + }); + let session = Arc::new(ExternalProviderWorkdirSession::new(connection, None)); + let caller = tokio::spawn(async move { + session + .checkout_execute(CheckoutRequest { + target: WorkdirPath::root(), + validator: vec![1], + operation, + }) + .await + }); + let command = + tokio::time::timeout(std::time::Duration::from_secs(5), commands.recv()) + .await + .unwrap() + .unwrap(); + let ExternalProviderCommand::Operation { + operation, + response, + .. + } = command + else { + panic!("expected dispatched operation"); + }; + assert!(matches!( + operation, + WorkdirSessionOperation::CheckoutExecute(_) + )); + let failed = serde_json::from_value::(json!({ + "outcome":"failed", "error":{"code":code, "denial_reason":reason}, + })) + .unwrap(); + let ExternalWorkdirOperationOutcome::Failed { error } = failed else { + panic!("expected fixture failure"); + }; + response.send(Err(error.into_transport_error())).unwrap(); + let error = tokio::time::timeout(std::time::Duration::from_secs(5), caller) + .await + .unwrap() + .unwrap() + .unwrap_err(); + assert!( + matches!( + commands.try_recv(), + Err(tokio::sync::mpsc::error::TryRecvError::Empty + | tokio::sync::mpsc::error::TryRecvError::Disconnected) + ), + "must not retry" + ); + let forwarded = WorkdirTransportError::from_workdir_error(&error); + assert_eq!( + forwarded.code, expected_code, + "{code:?}, {reason:?}, mutation={mutation}: {error:?}" + ); + assert_eq!(error.denial_reason(), reason); + assert_eq!(forwarded.denial_reason, reason); + if expected_code == Code::OutcomeUnknown { + assert!(forwarded.message.contains("inspect before retrying")); + } + let response = InternalWorkdirOperationError::Provider(forwarded).into_response(); + assert_eq!(response.status().as_u16(), expected_code.http_status()); + let body = axum::body::to_bytes(response.into_body(), 2048) + .await + .unwrap(); + let body: serde_json::Value = serde_json::from_slice(&body).unwrap(); + assert_eq!(body["code"], expected_code.as_str()); + assert!(body.get("denial_reason").is_none()); + } + } + } +} diff --git a/docs/development/workdir-refusal-diagnostics.md b/docs/development/workdir-refusal-diagnostics.md index 84e309c5f..1fe2d7575 100644 --- a/docs/development/workdir-refusal-diagnostics.md +++ b/docs/development/workdir-refusal-diagnostics.md @@ -137,6 +137,18 @@ is never parsed into a reason. provider: missing files are created identically without/with/nested reasons; denied/read-only/conflict/unknown-outcome stat failures never write or retry. This protects actual create control flow in addition to error conversion. +- Real Remote HTTP and channel-backed External session dispatch tests cover + Read/Create/Write/Edit crossed with missing/typed reasons and five error codes. + After a dispatched mutation, Unavailable/Transport still become OutcomeUnknown + with the original reason preserved: forwarded 500 and inspect-before-retry + guidance are unchanged. Read and definite refusal/conflict controls retain + their existing codes. The External path also checks actual HTTP error body/ + status conversion and absence of a new dispatch after the failure. +- Fake-command JSON fixtures cover attachment detach and scoped-command close + without launching processes. Missing/typed/nested UnknownCommand all retain + disappearance/lease-release semantics. Unavailable remains a busy detach or + failed scope cleanup; scope close revokes new work immediately but retains the + write lease while cleanup is unresolved. Diagnostic data never grants cleanup. - A signed real self-handler test observes command dispatch, then removes only its fixture session-registration entry before returning the start handle. The 502 remains a post-start `command_registration` failure with a neutral diff --git a/docs/report/t731-review-verdict-registration-401.md b/docs/report/t731-review-verdict-registration-401.md index 8f08213c5..fded7f508 100644 --- a/docs/report/t731-review-verdict-registration-401.md +++ b/docs/report/t731-review-verdict-registration-401.md @@ -68,6 +68,17 @@ The new test fails for the typed-reason case before the fix and passes all three forms after it. That source movement still needs fresh exact-source review, not reuse of the old review binding. The user-reported Workdir 403 remains unidentified. +## Later registration observation + +A subsequent fresh binding on source +`6ac87c885e58f11cb6dde0557c9229249f2c428e` successfully recorded request-changes: +MR seq5 event `01a12114-771a-76b1-b0fb-2fde0d56e3e1` at +`2026-10-09T14:32:31.769486531+00:00`, confirmed by parent MR reread. This proves +that selected submission succeeded; it does not explain the earlier 401. +No authentication, proof, capability, Backend, or running binary changes were +made to obtain it. The remaining code finding and routine review evidence are +on that MR, not an approval or Ticket conclusion in this report. + ## Improvement proposal A verdict-registration rejection should expose a safe, bounded internal