diff --git a/crates/fs-operation/src/checked.rs b/crates/fs-operation/src/checked.rs index f27b251bb..0e4759353 100644 --- a/crates/fs-operation/src/checked.rs +++ b/crates/fs-operation/src/checked.rs @@ -66,10 +66,7 @@ impl<'a> CheckedTarget<'a> { } else { policy.is_readable_paths(logical, logical) }) { - return Err(std::io::Error::new( - std::io::ErrorKind::PermissionDenied, - "checkout target denied", - )); + return Err(crate::FsDenialReason::CheckoutTargetDenied.into_io_error()); } let parent = if relative.as_os_str().is_empty() { None @@ -294,10 +291,7 @@ impl FsAccessPolicy for CheckedCreate<'_, '_> { if components.peek().is_some() && !self.target.policy.is_writable_paths(&parent, &parent) { - return Err(std::io::Error::new( - std::io::ErrorKind::PermissionDenied, - "Create parent denied", - )); + return Err(crate::FsDenialReason::CheckoutCreateParentDenied.into_io_error()); } } // Nested mkdir may have effects before failure; never auto-retry. diff --git a/crates/fs-operation/src/lib.rs b/crates/fs-operation/src/lib.rs index 791aeae7b..0adc96460 100644 --- a/crates/fs-operation/src/lib.rs +++ b/crates/fs-operation/src/lib.rs @@ -37,6 +37,26 @@ pub const MAX_RESULT_PATH_BYTES: usize = 1024 * 1024; /// Provider-side ceiling for aggregate grep source bytes per operation. pub const MAX_GREP_SOURCE_BYTES: u64 = 64 * 1024 * 1024; +/// Safe origin for a synthetic `PermissionDenied` from this crate. +/// Contains no paths or provider-authored text; real OS errors are not wrapped. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Error)] +pub enum FsDenialReason { + #[error("path is outside provider root")] + ProviderRootExceeded, + #[error("symbolic links are not permitted by this provider")] + ProviderSymlinkDenied, + #[error("checkout target denied")] + CheckoutTargetDenied, + #[error("Create parent denied")] + CheckoutCreateParentDenied, +} + +impl FsDenialReason { + pub fn into_io_error(self) -> std::io::Error { + std::io::Error::new(std::io::ErrorKind::PermissionDenied, self) + } +} + /// Open `path` beneath `root` without following any symbolic link. Linux uses /// `openat2` so resolution and open are one kernel-enforced operation. Other /// platforms fail closed rather than silently weakening an External grant. @@ -91,10 +111,7 @@ pub fn open_beneath_no_symlinks_at( const RESOLVE_BENEATH: u64 = 0x08; if relative.is_absolute() { - return Err(std::io::Error::new( - std::io::ErrorKind::PermissionDenied, - "path is outside provider root", - )); + return Err(crate::FsDenialReason::ProviderRootExceeded.into_io_error()); } let relative = if relative.as_os_str().is_empty() { CString::new(".").expect("static path") @@ -122,10 +139,7 @@ pub fn open_beneath_no_symlinks_at( if fd < 0 { let error = std::io::Error::last_os_error(); if error.raw_os_error() == Some(libc::ELOOP) { - return Err(std::io::Error::new( - std::io::ErrorKind::PermissionDenied, - "symbolic links are not permitted by this provider", - )); + return Err(crate::FsDenialReason::ProviderSymlinkDenied.into_io_error()); } return Err(error); } @@ -146,12 +160,9 @@ pub fn open_beneath_no_symlinks_at( #[cfg(target_os = "linux")] pub fn open_beneath_no_symlinks(root: &Path, path: &Path) -> std::io::Result { - let relative = path.strip_prefix(root).map_err(|_| { - std::io::Error::new( - std::io::ErrorKind::PermissionDenied, - "path is outside provider root", - ) - })?; + let relative = path + .strip_prefix(root) + .map_err(|_| crate::FsDenialReason::ProviderRootExceeded.into_io_error())?; let root = open_root_no_symlinks(root)?; open_beneath_no_symlinks_at(&root, relative) } @@ -184,20 +195,14 @@ pub fn atomic_write_beneath_no_symlinks_at( use std::os::unix::ffi::OsStrExt; if relative.is_absolute() { - return Err(std::io::Error::new( - std::io::ErrorKind::PermissionDenied, - "path is outside provider root", - )); + return Err(crate::FsDenialReason::ProviderRootExceeded.into_io_error()); } let mut components = relative.components().peekable(); let mut parent = root.try_clone()?; let mut file_name = None::; while let Some(component) = components.next() { let std::path::Component::Normal(name) = component else { - return Err(std::io::Error::new( - std::io::ErrorKind::PermissionDenied, - "path is outside provider root", - )); + return Err(crate::FsDenialReason::ProviderRootExceeded.into_io_error()); }; if components.peek().is_none() { file_name = Some(name.to_os_string()); diff --git a/crates/server-api/src/lib.rs b/crates/server-api/src/lib.rs index 1b9976c74..a83c92496 100644 --- a/crates/server-api/src/lib.rs +++ b/crates/server-api/src/lib.rs @@ -98,6 +98,24 @@ pub struct WorkdirOperationApiError { pub diagnostics: Option>, #[serde(skip, default = "default_repository_error_status")] status: u16, + /// Server-local metadata retained by generated error response extensions, never the wire body. + #[serde(skip)] + pub workdir_log: Option, + #[serde(skip)] + pub denial_reason: Option, +} + +/// Internal operational correlation. Producers supply fixed-length ID fingerprints and +/// an allowlisted operation label, not request text, paths, commands or credentials. +#[derive(Clone, Debug, Default)] +pub struct WorkdirOperationLogContext { + pub operation: &'static str, + pub stage: &'static str, + pub workspace_id_hash: String, + pub runtime_id_hash: Option, + pub worker_id_hash: Option, + pub workdir_id_hash: Option, + pub attachment_alias_hash: String, } impl WorkdirOperationApiError { @@ -108,6 +126,8 @@ impl WorkdirOperationApiError { message: error.message, diagnostics: Some(error.diagnostics), status: error.status, + workdir_log: None, + denial_reason: None, } } @@ -119,6 +139,8 @@ impl WorkdirOperationApiError { message: error.message, diagnostics: None, status, + workdir_log: None, + denial_reason: error.denial_reason, } } } @@ -13984,3 +14006,34 @@ mod typescript_tests { .collect() } } + +#[cfg(test)] +mod workdir_error_diagnostic_tests { + use super::*; + + #[test] + fn workdir_internal_diagnostics_never_enter_public_error_body_or_schema() { + let mut error = WorkdirOperationApiError::provider(workdir::http::WorkdirTransportError { + code: workdir::http::WorkdirTransportErrorCode::Denied, + message: "Workdir operation was denied".to_string(), + denial_reason: Some(workdir::WorkdirDenialReason::OsPermissionDenied), + }); + error.workdir_log = Some(WorkdirOperationLogContext { + operation: "write", + workspace_id_hash: "internal-workspace-fingerprint".to_string(), + ..Default::default() + }); + let wire = serde_json::to_value(&error).unwrap(); + assert_eq!( + wire, + serde_json::json!({"code": "denied", "message": "Workdir operation was denied"}) + ); + assert_eq!(api_macros::HttpError::status_code(&error), 403); + let decoded: WorkdirOperationApiError = serde_json::from_value(wire).unwrap(); + assert!(decoded.denial_reason.is_none()); + assert!(decoded.workdir_log.is_none()); + let schema = serde_json::to_value(schemars::schema_for!(WorkdirOperationApiError)).unwrap(); + assert!(schema["properties"].get("workdir_log").is_none()); + assert!(schema["properties"].get("denial_reason").is_none()); + } +} diff --git a/crates/tools/src/checkout_tests.rs b/crates/tools/src/checkout_tests.rs index d8ec7b74e..670f21527 100644 --- a/crates/tools/src/checkout_tests.rs +++ b/crates/tools/src/checkout_tests.rs @@ -421,6 +421,13 @@ fn checkout_known_refusals_have_typed_codes_without_changing_normal_tools() { use workdir::{WorkdirError, WorkdirSessionCapability}; let cases = [ (WorkdirError::Denied("denied".into()), "checkout_denied"), + ( + WorkdirError::DenialContext { + reason: workdir::WorkdirDenialReason::OsPermissionDenied, + source: Box::new(WorkdirError::OutOfScope("outside".into())), + }, + "checkout_denied", + ), ( WorkdirError::OutOfScope("outside".into()), "checkout_denied", diff --git a/crates/tools/src/error.rs b/crates/tools/src/error.rs index b25025b7c..df5cd894f 100644 --- a/crates/tools/src/error.rs +++ b/crates/tools/src/error.rs @@ -38,17 +38,24 @@ pub enum ToolsError { impl From for ToolError { fn from(err: ToolsError) -> Self { match &err { - ToolsError::WorkdirSession( - workdir::WorkdirError::NotFound(_) - | workdir::WorkdirError::Io { .. } - | workdir::WorkdirError::Unavailable(_) - | workdir::WorkdirError::OperationFailed - | workdir::WorkdirError::Transport(_) - | workdir::WorkdirError::Conflict(_) - | workdir::WorkdirError::OutcomeUnknown(_), - ) => ToolError::ExecutionFailed(err.to_string()), + ToolsError::WorkdirSession(error) => { + // Diagnostic context is transparent to the established normal-tool + // classification. Preserve the original display, including unknown + // outcome guidance, while classifying through nested wrappers. + match error.classification_source() { + workdir::WorkdirError::NotFound(_) + | workdir::WorkdirError::Io { .. } + | workdir::WorkdirError::Unavailable(_) + | workdir::WorkdirError::OperationFailed + | workdir::WorkdirError::Transport(_) + | workdir::WorkdirError::Conflict(_) + | workdir::WorkdirError::OutcomeUnknown(_) => { + ToolError::ExecutionFailed(err.to_string()) + } + _ => ToolError::InvalidArgument(err.to_string()), + } + } ToolsError::FileSystem(_) - | ToolsError::WorkdirSession(_) | ToolsError::NotRead(_) | ToolsError::ExternallyModified(_) | ToolsError::StringNotFound { .. } @@ -134,6 +141,76 @@ mod tests { } } + #[test] + fn transport_diagnostic_reasons_preserve_normal_tool_classification_and_display() { + use WorkdirTransportErrorCode as Code; + use workdir::{WorkdirDenialReason as Reason, WorkdirError}; + for (code, execution_failure) in [ + (Code::NotFound, true), + (Code::Conflict, true), + (Code::OutcomeUnknown, true), + (Code::Unsupported, false), + (Code::InvalidRequest, false), + (Code::Denied, false), + (Code::OutOfScope, false), + (Code::SymlinkOutOfScope, false), + (Code::BrokenSymlink, false), + (Code::SymlinkTargetIsDirectory, false), + (Code::ReadOnly, false), + (Code::IsDirectory, false), + (Code::SymlinkDirectoryNotTraversed, false), + (Code::UnknownCommand, false), + (Code::Unavailable, true), + (Code::Io, true), + (Code::Transport, true), + (Code::Internal, true), + ] { + // Wire data may carry a reason independently of its public code. + // Neither enrichment nor nested wrappers may change normal-tool behavior. + let baseline_message = serde_json::from_value::( + serde_json::json!({"code": code, "message": "safe fixture message"}), + ) + .unwrap() + .into_workdir_error() + .to_string(); + for depth in [0, 1, 2] { + let wire = serde_json::json!({ + "code": code, + "message": "safe fixture message", + "denial_reason": (depth > 0).then_some(Reason::OsPermissionDenied), + }); + let mut error = serde_json::from_value::(wire) + .unwrap() + .into_workdir_error(); + if depth == 2 { + error = WorkdirError::DenialContext { + reason: Reason::ReadOnlySession, + source: Box::new(error), + }; + } + let expected_message = error.to_string(); + assert_eq!( + expected_message, baseline_message, + "{code:?}, depth {depth}" + ); + let tool_error = ToolError::from(ToolsError::from(error)); + let (actual_execution_failure, actual_message) = match tool_error { + ToolError::ExecutionFailed(message) => (true, message), + ToolError::InvalidArgument(message) => (false, message), + other => panic!("unexpected classification for {code:?}: {other:?}"), + }; + assert_eq!( + actual_execution_failure, execution_failure, + "{code:?}, depth {depth}" + ); + assert_eq!(actual_message, expected_message, "{code:?}, depth {depth}"); + if code == Code::OutcomeUnknown { + assert!(actual_message.contains("inspect before retrying")); + } + } + } + } + #[test] fn remote_workdir_content_conflict_is_retryable_without_host_path() { let transport = WorkdirTransportError::from_workdir_error( diff --git a/crates/tools/src/file_target.rs b/crates/tools/src/file_target.rs index 17397eea3..424b005e1 100644 --- a/crates/tools/src/file_target.rs +++ b/crates/tools/src/file_target.rs @@ -139,7 +139,8 @@ impl FileTarget { pub(crate) fn checked_error(error: workdir::WorkdirError) -> ToolError { use workdir::WorkdirError; - let code = match &error { + let code = match error.classification_source() { + WorkdirError::DenialContext { .. } => unreachable!("diagnostic context unwrapped"), WorkdirError::Conflict(_) | WorkdirError::NotFound(_) => "checkout_stale", WorkdirError::Denied { .. } | WorkdirError::OutOfScope { .. } diff --git a/crates/tools/src/write.rs b/crates/tools/src/write.rs index a79b01713..fefa660a6 100644 --- a/crates/tools/src/write.rs +++ b/crates/tools/src/write.rs @@ -94,7 +94,9 @@ pub(crate) async fn execute_write( .await { Ok(_) => Some(tracker.expected_workdir_hash(path)?), - Err(WorkdirError::NotFound(_)) => None, + Err(error) if matches!(error.classification_source(), WorkdirError::NotFound(_)) => { + None + } Err(error) => return Err(ToolsError::from(error).into()), } }; @@ -190,6 +192,161 @@ mod tests { assert_eq!(std::fs::read_to_string(&file).unwrap(), "hello\n"); } + /// Stat crosses the real JSON transport boundary; mutations still use the + /// checked local provider, so the assertions cover durable tool effects. + #[derive(Debug)] + struct StatErrorSession { + local: LocalWorkdirSession, + code: &'static str, + depth: u8, + stats: std::sync::atomic::AtomicUsize, + writes: std::sync::atomic::AtomicUsize, + } + + #[async_trait] + impl workdir::WorkdirSession for StatErrorSession { + fn workdir(&self) -> &workdir::Workdir { + self.local.workdir() + } + fn capabilities(&self) -> workdir::WorkdirSessionCapabilities { + self.local.capabilities() + } + async fn stat(&self, _: StatRequest) -> Result { + use workdir::{WorkdirDenialReason as Reason, http::WorkdirTransportError}; + self.stats.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + let wire = serde_json::json!({ + "code": self.code, + "message": "safe fixture message", + "denial_reason": (self.depth > 0).then_some(Reason::OsPermissionDenied), + }); + let mut error = serde_json::from_value::(wire) + .unwrap() + .into_workdir_error(); + if self.depth == 2 { + error = WorkdirError::DenialContext { + reason: Reason::ReadOnlySession, + source: Box::new(error), + }; + } + Err(error) + } + async fn read(&self, r: workdir::ReadRequest) -> Result { + self.local.read(r).await + } + async fn write( + &self, + r: workdir::WriteRequest, + ) -> Result { + self.writes + .fetch_add(1, std::sync::atomic::Ordering::SeqCst); + self.local.write(r).await + } + async fn edit(&self, r: workdir::EditRequest) -> Result { + self.local.edit(r).await + } + async fn list(&self, r: workdir::ListRequest) -> Result { + self.local.list(r).await + } + async fn glob(&self, r: workdir::GlobRequest) -> Result { + self.local.glob(r).await + } + async fn grep(&self, r: workdir::GrepRequest) -> Result { + self.local.grep(r).await + } + async fn start_command( + &self, + r: workdir::CommandRequest, + ) -> Result { + self.local.start_command(r).await + } + async fn command_status( + &self, + r: workdir::CommandHandle, + ) -> Result { + self.local.command_status(r).await + } + async fn command_output( + &self, + r: workdir::CommandOutputRequest, + ) -> Result { + self.local.command_output(r).await + } + async fn cancel_command(&self, r: workdir::CommandHandle) -> Result<(), WorkdirError> { + self.local.cancel_command(r).await + } + async fn close(&self) -> Result<(), WorkdirError> { + self.local.close().await + } + } + + fn stat_error_session(dir: &TempDir, code: &'static str, depth: u8) -> Arc { + Arc::new(StatErrorSession { + local: LocalWorkdirSession::new( + Scope::writable(dir.path()).unwrap(), + dir.path().to_path_buf(), + ), + code, + depth, + stats: Default::default(), + writes: Default::default(), + }) + } + + #[tokio::test] + async fn write_not_found_diagnostics_preserve_create_behavior() { + for depth in [0, 1, 2] { + let dir = TempDir::new().unwrap(); + let session = stat_error_session(&dir, "not_found", depth); + let (_, tool) = write_tool(session.clone(), Tracker::new())(); + let output = tool + .execute( + r#"{"file_path":"new.txt","content":"hello\n"}"#, + Default::default(), + ) + .await + .unwrap_or_else(|error| panic!("depth {depth}: {error:?}")); + assert!(output.summary.contains("Created new.txt"), "{output:?}"); + assert_eq!( + std::fs::read_to_string(dir.path().join("new.txt")).unwrap(), + "hello\n" + ); + assert_eq!(session.stats.load(std::sync::atomic::Ordering::SeqCst), 1); + assert_eq!(session.writes.load(std::sync::atomic::Ordering::SeqCst), 1); + } + } + + #[tokio::test] + async fn write_stat_failures_do_not_create_or_retry_with_diagnostics() { + for code in ["denied", "read_only", "conflict", "outcome_unknown"] { + for depth in [0, 1, 2] { + let dir = TempDir::new().unwrap(); + let session = stat_error_session(&dir, code, depth); + let (_, tool) = write_tool(session.clone(), Tracker::new())(); + let error = tool + .execute( + r#"{"file_path":"new.txt","content":"hello"}"#, + Default::default(), + ) + .await + .unwrap_err(); + if matches!(code, "conflict" | "outcome_unknown") { + assert!( + matches!(error, ToolError::ExecutionFailed(_)), + "{code}, depth {depth}: {error:?}" + ); + } else { + assert!( + matches!(error, ToolError::InvalidArgument(_)), + "{code}, depth {depth}: {error:?}" + ); + } + assert!(!dir.path().join("new.txt").exists()); + assert_eq!(session.stats.load(std::sync::atomic::Ordering::SeqCst), 1); + assert_eq!(session.writes.load(std::sync::atomic::Ordering::SeqCst), 0); + } + } + } + #[tokio::test] async fn write_existing_requires_prior_read() { let (dir, fs, tracker) = setup(); diff --git a/crates/workdir/src/checkout.rs b/crates/workdir/src/checkout.rs index d6592df2b..b9458748c 100644 --- a/crates/workdir/src/checkout.rs +++ b/crates/workdir/src/checkout.rs @@ -148,8 +148,9 @@ impl CheckoutSearchRequest { if !std::path::Path::new(self.operation.path().as_str()) .starts_with(self.output_root.as_str()) { - return Err(WorkdirError::Denied( - "checkout search must stay beneath its output root".into(), + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::CheckoutOutputRootExceeded, + "checkout search must stay beneath its output root", )); } if self.scope_layers.len() > 16 diff --git a/crates/workdir/src/checkout_search_tests.rs b/crates/workdir/src/checkout_search_tests.rs index 2777088b7..80e6c6b6b 100644 --- a/crates/workdir/src/checkout_search_tests.rs +++ b/crates/workdir/src/checkout_search_tests.rs @@ -751,7 +751,15 @@ async fn checkout_search_native_skips_symlinked_ignore_sources_without_outside_e ))) .await .unwrap_err(); - assert!(matches!(error, WorkdirError::OutOfScope(_))); + let classification = match &error { + WorkdirError::DenialContext { source, .. } => source.as_ref(), + error => error, + }; + assert!(matches!(classification, WorkdirError::OutOfScope(_))); + assert_ne!( + error.denial_reason(), + Some(crate::WorkdirDenialReason::OsPermissionDenied) + ); } for (relative, hidden) in [ (".ignore", "root-ignore-visible.txt"), diff --git a/crates/workdir/src/external.rs b/crates/workdir/src/external.rs index 1d91c20a3..76186cbb8 100644 --- a/crates/workdir/src/external.rs +++ b/crates/workdir/src/external.rs @@ -497,18 +497,24 @@ impl<'de> Deserialize<'de> for ExternalWorkdirOperationResult { #[serde(deny_unknown_fields)] pub struct ExternalWorkdirOperationError { code: crate::http::WorkdirTransportErrorCode, + #[serde(default, skip_serializing_if = "Option::is_none")] + denial_reason: Option, } impl ExternalWorkdirOperationError { - /// Collapse provider failures to a closed error code. Provider-authored - /// diagnostics never cross the External Workdir trust boundary. + /// Keep only closed error codes and typed refusal reasons. Provider-authored + /// text never crosses the External Workdir trust boundary. pub fn from_transport_error(error: WorkdirTransportError) -> Self { - Self { code: error.code } + Self { + code: error.code, + denial_reason: error.denial_reason, + } } pub fn into_transport_error(self) -> WorkdirTransportError { WorkdirTransportError { code: self.code, + denial_reason: self.denial_reason, message: format!("External Workdir provider reported {}", self.code.as_str()), } } @@ -719,10 +725,45 @@ mod tests { } } + #[test] + fn external_error_wire_forwards_allowlisted_reasons_without_provider_text() { + use crate::WorkdirDenialReason as Reason; + use crate::http::WorkdirTransportErrorCode as Code; + for reason in [ + Some(Reason::ExternalScopeSymlink), + Some(Reason::OsPermissionDenied), + None, + ] { + let external = + ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError { + code: Code::Denied, + message: "/home/operator/private provider-secret".repeat(4096), + denial_reason: reason, + }); + let wire = serde_json::to_string(&external).unwrap(); + assert!(wire.len() < 128); + assert_no_provider_authority(&serde_json::from_str(&wire).unwrap()); + let decoded: ExternalWorkdirOperationError = serde_json::from_str(&wire).unwrap(); + let error = decoded.into_transport_error().into_workdir_error(); + let forwarded = WorkdirTransportError::from_workdir_error(&error); + assert_eq!(forwarded.code, Code::Denied); + assert_eq!(forwarded.denial_reason, reason); + assert_eq!(forwarded.message, "Workdir operation was denied"); + } + let old: ExternalWorkdirOperationError = + serde_json::from_str(r#"{"code":"denied"}"#).unwrap(); + assert_eq!(old.into_transport_error().denial_reason, None); + for reason in ["/home/operator/private", &"provider-secret".repeat(4096)] { + let input = serde_json::json!({"code":"denied", "denial_reason":reason}); + assert!(serde_json::from_value::(input).is_err()); + } + } + #[test] fn provider_failures_are_closed_and_drop_provider_authored_diagnostics() { let error = ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError { code: crate::http::WorkdirTransportErrorCode::Internal, + denial_reason: None, message: "/home/operator/private provider-secret\ncontrol".to_string(), }); let frame = ExternalWorkdirProviderFrame::current( diff --git a/crates/workdir/src/external_local_tests.rs b/crates/workdir/src/external_local_tests.rs index c0d1b5d18..0a316586f 100644 --- a/crates/workdir/src/external_local_tests.rs +++ b/crates/workdir/src/external_local_tests.rs @@ -357,9 +357,17 @@ async fn external_local_provider_rejects_symlink_roots_and_traversal() { ) .await .unwrap_err(); + let classification = match &error { + WorkdirError::DenialContext { source, .. } => source.as_ref(), + error => error, + }; + assert_ne!( + error.denial_reason(), + Some(crate::WorkdirDenialReason::OsPermissionDenied) + ); assert!( matches!( - error, + classification, WorkdirError::SymlinkOutOfScope { .. } | WorkdirError::OutOfScope(_) ), "unexpected error for {path}: {error:?}" @@ -375,6 +383,10 @@ async fn external_local_provider_rejects_symlink_roots_and_traversal() { BoundedReadLimits::new(4096, 1024).unwrap(), ) .unwrap_err(); + assert_eq!( + error.denial_reason(), + Some(crate::WorkdirDenialReason::ExternalRootSymlink) + ); assert!(matches!(error, WorkdirError::Denied(_))); } @@ -408,9 +420,17 @@ async fn external_read_write_provider_rejects_symlink_mutation_escape() { ) .await .unwrap_err(); + let classification = match &error { + WorkdirError::DenialContext { source, .. } => source.as_ref(), + error => error, + }; + assert_ne!( + error.denial_reason(), + Some(crate::WorkdirDenialReason::OsPermissionDenied) + ); assert!( matches!( - error, + classification, WorkdirError::SymlinkOutOfScope { .. } | WorkdirError::OutOfScope(_) | WorkdirError::Denied(_) @@ -559,7 +579,15 @@ async fn external_grep_honors_gitignore_above_a_nested_search_root() { ) .await .unwrap_err(); - assert!(matches!(symlink_error, WorkdirError::OutOfScope(_))); + let classification = match &symlink_error { + WorkdirError::DenialContext { source, .. } => source.as_ref(), + error => error, + }; + assert!(matches!(classification, WorkdirError::OutOfScope(_))); + assert_ne!( + symlink_error.denial_reason(), + Some(crate::WorkdirDenialReason::OsPermissionDenied) + ); } #[cfg(target_os = "linux")] diff --git a/crates/workdir/src/http.rs b/crates/workdir/src/http.rs index 1050ae3d9..940506ca7 100644 --- a/crates/workdir/src/http.rs +++ b/crates/workdir/src/http.rs @@ -269,6 +269,9 @@ impl WorkdirTransportErrorCode { pub struct WorkdirTransportError { pub code: WorkdirTransportErrorCode, pub message: String, + /// Internal provider diagnostics. Public response boundaries may omit this. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub denial_reason: Option, } impl WorkdirTransportError { @@ -276,6 +279,11 @@ impl WorkdirTransportError { pub fn from_workdir_error(error: &WorkdirError) -> Self { use WorkdirTransportErrorCode as Code; let (code, message) = match error { + WorkdirError::DenialContext { reason, source } => { + let mut transport = Self::from_workdir_error(source); + transport.denial_reason = Some(*reason); + return transport; + } WorkdirError::OutcomeUnknown(_) => ( Code::OutcomeUnknown, "Workdir operation may have effects; inspect before retrying", @@ -289,6 +297,7 @@ impl WorkdirTransportError { return Self { code: Code::Unsupported, message: format!("Workdir capability {capability:?} is not available"), + denial_reason: None, }; } WorkdirError::UnsupportedOperation(_) => { @@ -346,12 +355,13 @@ impl WorkdirTransportError { Self { code, message: message.to_string(), + denial_reason: error.denial_reason(), } } pub fn into_workdir_error(self) -> WorkdirError { use WorkdirTransportErrorCode as Code; - match self.code { + let error = match self.code { Code::NotFound => WorkdirError::NotFound("".into()), Code::Conflict => WorkdirError::Conflict(self.message), Code::OutcomeUnknown => WorkdirError::OutcomeUnknown( @@ -359,7 +369,11 @@ impl WorkdirTransportError { ), Code::Unsupported => WorkdirError::UnsupportedOperation(self.message), Code::InvalidRequest => WorkdirError::InvalidArgument(self.message), - Code::Denied => WorkdirError::Denied(self.message), + Code::Denied => WorkdirError::Denied(crate::WorkdirDenial { + reason: self.denial_reason, + // Provider-authored text is not trusted diagnostic data. + message: "Workdir operation was denied".into(), + }), Code::OutOfScope => WorkdirError::OutOfScope("".into()), Code::SymlinkOutOfScope => WorkdirError::SymlinkOutOfScope { path: "".into(), @@ -390,7 +404,18 @@ impl WorkdirTransportError { }, Code::Transport => WorkdirError::Transport(self.message), Code::Internal => WorkdirError::OperationFailed, + }; + // Preserve any supplied reason independently of the public classification. + // Without a reason, retain the existing classification's deliberate fallback. + if let Some(reason) = self.denial_reason { + if error.denial_reason() != Some(reason) { + return WorkdirError::DenialContext { + reason, + source: Box::new(error), + }; + } } + error } } @@ -691,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), } @@ -949,6 +987,7 @@ mod tests { ] { let transport = WorkdirTransportError { code, + denial_reason: None, message: if code == WorkdirTransportErrorCode::Conflict { "The target file's content or existence changed since it was last observed; read the file again before retrying" .to_string() diff --git a/crates/workdir/src/lib.rs b/crates/workdir/src/lib.rs index f46126384..c77ee7d81 100644 --- a/crates/workdir/src/lib.rs +++ b/crates/workdir/src/lib.rs @@ -211,7 +211,8 @@ pub trait WorkdirSession: std::fmt::Debug + Send + Sync { }) { Ok(()) } else { - Err(WorkdirError::Denied( + Err(WorkdirError::denied( + WorkdirDenialReason::ScopeResolutionUnavailable, "Workdir provider cannot establish resolved scope authority".to_string(), )) } @@ -221,7 +222,8 @@ pub trait WorkdirSession: std::fmt::Debug + Send + Sync { &self, _request: WorkdirScopeOverlapRequest, ) -> Result { - Err(WorkdirError::Denied( + Err(WorkdirError::denied( + WorkdirDenialReason::ScopeComparisonUnavailable, "Workdir provider cannot compare resolved scope authority".to_string(), )) } @@ -292,10 +294,125 @@ pub trait WorkdirSession: std::fmt::Debug + Send + Sync { pub type WorkdirSessionHandle = Arc; +/// Closed diagnostic vocabulary: no request text, paths or secrets. +/// A reason describes a refusing branch, not an authorization grant. +#[derive( + Clone, Copy, Debug, PartialEq, Eq, serde::Serialize, serde::Deserialize, schemars::JsonSchema, +)] +#[serde(rename_all = "snake_case")] +pub enum WorkdirDenialReason { + ScopeResolutionUnavailable, + ScopeComparisonUnavailable, + ScopedCapabilityDenied, + LogicalScopeExceeded, + InvalidScopedPath, + ChildWriteLeaseConflict, + EmptyScope, + ParentCapabilityDenied, + CommandRequiresWritableScope, + ParentScopeExceeded, + CwdOutsideReadableScope, + ReadOnlySession, + ExternalRootSymlink, + ExternalRootChanged, + ExternalRootMoved, + ExternalScopeSymlink, + AttachmentScopeExceeded, + DelegatedScopeExceeded, + CheckoutOutputRootExceeded, + ProviderRootExceeded, + ProviderSymlinkDenied, + CheckoutTargetDenied, + CheckoutCreateParentDenied, + CheckoutSearchPathDenied, + CheckoutEnumerationDenied, + PathOutOfScope, + SymlinkTargetOutOfScope, + PathReadOnly, + OsPermissionDenied, + /// PermissionDenied without a typed provider origin or an OS error code. + /// In particular, do not mislabel lower-layer synthetic refusals as OS errors. + UnclassifiedPermissionDenied, +} + +impl WorkdirDenialReason { + /// Stable allowlisted label for internal diagnostics, matching the wire value. + pub const fn as_str(self) -> &'static str { + match self { + Self::ScopeResolutionUnavailable => "scope_resolution_unavailable", + Self::ScopeComparisonUnavailable => "scope_comparison_unavailable", + Self::ScopedCapabilityDenied => "scoped_capability_denied", + Self::LogicalScopeExceeded => "logical_scope_exceeded", + Self::InvalidScopedPath => "invalid_scoped_path", + Self::ChildWriteLeaseConflict => "child_write_lease_conflict", + Self::EmptyScope => "empty_scope", + Self::ParentCapabilityDenied => "parent_capability_denied", + Self::CommandRequiresWritableScope => "command_requires_writable_scope", + Self::ParentScopeExceeded => "parent_scope_exceeded", + Self::CwdOutsideReadableScope => "cwd_outside_readable_scope", + Self::ReadOnlySession => "read_only_session", + Self::ExternalRootSymlink => "external_root_symlink", + Self::ExternalRootChanged => "external_root_changed", + Self::ExternalRootMoved => "external_root_moved", + Self::ExternalScopeSymlink => "external_scope_symlink", + Self::AttachmentScopeExceeded => "attachment_scope_exceeded", + Self::DelegatedScopeExceeded => "delegated_scope_exceeded", + Self::CheckoutOutputRootExceeded => "checkout_output_root_exceeded", + Self::ProviderRootExceeded => "provider_root_exceeded", + Self::ProviderSymlinkDenied => "provider_symlink_denied", + Self::CheckoutTargetDenied => "checkout_target_denied", + Self::CheckoutCreateParentDenied => "checkout_create_parent_denied", + Self::CheckoutSearchPathDenied => "checkout_search_path_denied", + Self::CheckoutEnumerationDenied => "checkout_enumeration_denied", + Self::PathOutOfScope => "path_out_of_scope", + Self::SymlinkTargetOutOfScope => "symlink_target_out_of_scope", + Self::PathReadOnly => "path_read_only", + Self::OsPermissionDenied => "os_permission_denied", + Self::UnclassifiedPermissionDenied => "unclassified_permission_denied", + } + } +} + +/// Original local messages remain available, but are not diagnostic wire data. +/// Missing reasons from older providers remain explicitly unknown. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct WorkdirDenial { + pub reason: Option, + pub message: String, +} + +impl std::fmt::Display for WorkdirDenial { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(&self.message) + } +} +impl From for WorkdirDenial { + fn from(message: String) -> Self { + Self { + reason: None, + message, + } + } +} +impl From<&str> for WorkdirDenial { + fn from(message: &str) -> Self { + message.to_string().into() + } +} + #[derive(Debug, thiserror::Error)] pub enum WorkdirError { #[error("Workdir operation denied: {0}")] - Denied(String), + Denied(WorkdirDenial), + + /// Retain the source's existing classification and display while attaching + /// a reason (including checkout's historical PermissionDenied mapping). + #[error("{source}")] + DenialContext { + reason: WorkdirDenialReason, + #[source] + source: Box, + }, #[error("Workdir session is closed")] SessionClosed, @@ -398,6 +515,40 @@ pub enum WorkdirError { } impl WorkdirError { + /// The error used for operational classification, ignoring only diagnostic + /// context (including nested context). Keep the original error for display + /// and `denial_reason`: enrichment must not alter behavior or retry policy. + pub fn classification_source(&self) -> &Self { + let mut error = self; + while let Self::DenialContext { source, .. } = error { + error = source; + } + error + } + + pub fn denied(reason: WorkdirDenialReason, message: impl Into) -> Self { + Self::Denied(WorkdirDenial { + reason: Some(reason), + message: message.into(), + }) + } + + /// Safe diagnostic data only; never infer a reason from arbitrary messages. + pub fn denial_reason(&self) -> Option { + use WorkdirDenialReason as Reason; + match self { + Self::Denied(denial) => denial.reason, + Self::DenialContext { reason, .. } => Some(*reason), + Self::OutOfScope(_) => Some(Reason::PathOutOfScope), + Self::SymlinkOutOfScope { .. } => Some(Reason::SymlinkTargetOutOfScope), + Self::ReadOnly(_) => Some(Reason::PathReadOnly), + Self::Io { source, .. } if source.kind() == std::io::ErrorKind::PermissionDenied => { + Some(local::io_denial_reason(source)) + } + _ => None, + } + } + pub(crate) fn io(path: &Path, source: std::io::Error) -> Self { Self::Io { path: path.to_path_buf(), diff --git a/crates/workdir/src/local.rs b/crates/workdir/src/local.rs index 7d8a7697d..7ff202283 100644 --- a/crates/workdir/src/local.rs +++ b/crates/workdir/src/local.rs @@ -213,6 +213,51 @@ struct ScopeAccess { operation_guard: Option, } +/// Policy refusals carried through FsAccessPolicy's I/O interface are not OS +/// PermissionDenied. This marker contains only fixed text and a closed reason. +#[derive(Debug, thiserror::Error)] +#[error("{message}")] +struct PolicyPermissionDenied { + reason: crate::WorkdirDenialReason, + message: &'static str, +} + +fn policy_permission_denied( + reason: crate::WorkdirDenialReason, + message: &'static str, +) -> std::io::Error { + std::io::Error::new( + std::io::ErrorKind::PermissionDenied, + PolicyPermissionDenied { reason, message }, + ) +} + +pub(crate) fn io_denial_reason(error: &std::io::Error) -> crate::WorkdirDenialReason { + use crate::WorkdirDenialReason as Reason; + if let Some(source) = error.get_ref() { + if let Some(denial) = source.downcast_ref::() { + return denial.reason; + } + if let Some(denial) = source.downcast_ref::() { + return match denial { + fs_operation::FsDenialReason::ProviderRootExceeded => Reason::ProviderRootExceeded, + fs_operation::FsDenialReason::ProviderSymlinkDenied => { + Reason::ProviderSymlinkDenied + } + fs_operation::FsDenialReason::CheckoutTargetDenied => Reason::CheckoutTargetDenied, + fs_operation::FsDenialReason::CheckoutCreateParentDenied => { + Reason::CheckoutCreateParentDenied + } + }; + } + } + if error.raw_os_error().is_some() { + Reason::OsPermissionDenied + } else { + Reason::UnclassifiedPermissionDenied + } +} + impl ScopeAccess { fn new( scope: Arc, @@ -238,8 +283,8 @@ impl ScopeAccess { ) })?; let relative = path.strip_prefix(&self.root).map_err(|_| { - std::io::Error::new( - std::io::ErrorKind::PermissionDenied, + policy_permission_denied( + crate::WorkdirDenialReason::ProviderRootExceeded, "path is outside provider root", ) })?; @@ -288,8 +333,8 @@ impl fs_operation::FsAccessPolicy for ScopeAccess { if logical.starts_with(&self.root) { Ok(logical.to_path_buf()) } else { - Err(std::io::Error::new( - std::io::ErrorKind::PermissionDenied, + Err(policy_permission_denied( + crate::WorkdirDenialReason::ProviderRootExceeded, "path is outside provider root", )) } @@ -394,8 +439,8 @@ impl fs_operation::FsAccessPolicy for ScopeAccess { ) })?; let relative = resolved.strip_prefix(&self.root).map_err(|_| { - std::io::Error::new( - std::io::ErrorKind::PermissionDenied, + policy_permission_denied( + crate::WorkdirDenialReason::ProviderRootExceeded, "path is outside provider root", ) })?; @@ -491,8 +536,8 @@ impl SearchAccess { if self.read_allowed(logical, resolved) { Ok(()) } else { - Err(std::io::Error::new( - std::io::ErrorKind::PermissionDenied, + Err(policy_permission_denied( + crate::WorkdirDenialReason::CheckoutSearchPathDenied, "checkout search path denied", )) } @@ -545,8 +590,8 @@ impl fs_operation::FsAccessPolicy for SearchAccess { } fn open_read_dir(&self, logical: &Path, resolved: &Path) -> std::io::Result { if !self.enumeration_allowed(logical, resolved) { - return Err(std::io::Error::new( - std::io::ErrorKind::PermissionDenied, + return Err(policy_permission_denied( + crate::WorkdirDenialReason::CheckoutEnumerationDenied, "checkout directory enumeration denied", )); } @@ -648,7 +693,8 @@ impl ExternalWorkdirRoot { let selected_metadata = std::fs::symlink_metadata(directory) .map_err(|error| WorkdirError::io(Path::new(""), error))?; if selected_metadata.file_type().is_symlink() { - return Err(WorkdirError::Denied( + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::ExternalRootSymlink, "External Workdir root must not be a symbolic link".to_string(), )); } @@ -678,7 +724,8 @@ impl ExternalWorkdirRoot { if (selected_metadata.dev(), selected_metadata.ino()) != (opened_metadata.dev(), opened_metadata.ino()) { - return Err(WorkdirError::Denied( + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::ExternalRootChanged, "External Workdir root changed while it was being pinned".to_string(), )); } @@ -691,7 +738,8 @@ impl ExternalWorkdirRoot { let path = std::fs::read_link(format!("/proc/self/fd/{}", handle.as_raw_fd())) .map_err(|error| WorkdirError::io(Path::new(""), error))?; if !path.is_absolute() || path.to_string_lossy().ends_with(" (deleted)") { - return Err(WorkdirError::Denied( + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::ExternalRootMoved, "External Workdir root moved while it was being pinned".to_string(), )); } @@ -1334,7 +1382,8 @@ impl WorkdirSession for LocalWorkdirSession { self.validate_operation_path(&request.path)?; let logical = self.inner.root.join(request.path.as_str()); if self.inner.reject_symlinks && fs_operation::first_symlink(&logical).is_some() { - return Err(WorkdirError::Denied( + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::ExternalScopeSymlink, "External Workdir scope cannot traverse symbolic links".to_string(), )); } @@ -1353,10 +1402,13 @@ impl WorkdirSession for LocalWorkdirSession { WorkdirToolScopePermission::Write => parent_permission == Some(Permission::Write), }; if !parent_allows { - return Err(WorkdirError::Denied(format!( - "Workdir path `{}` exceeds the provider attachment scope", - request.path - ))); + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::AttachmentScopeExceeded, + format!( + "Workdir path `{}` exceeds the provider attachment scope", + request.path + ), + )); } let allowed = request.rules.iter().any(|rule| { if request.permission == WorkdirToolScopePermission::Write @@ -1383,10 +1435,13 @@ impl WorkdirSession for LocalWorkdirSession { if allowed { Ok(()) } else { - Err(WorkdirError::Denied(format!( - "Workdir path `{}` is outside the provider-resolved delegated scope", - request.path - ))) + Err(WorkdirError::denied( + crate::WorkdirDenialReason::DelegatedScopeExceeded, + format!( + "Workdir path `{}` is outside the provider-resolved delegated scope", + request.path + ), + )) } } @@ -1432,7 +1487,12 @@ impl WorkdirSession for LocalWorkdirSession { let logical = request.operation.path().clone(); let relative = Path::new(logical.as_str()) .strip_prefix(request.output_root.as_str()) - .map_err(|_| WorkdirError::Denied("checkout search escaped output root".into()))?; + .map_err(|_| { + WorkdirError::denied( + crate::WorkdirDenialReason::CheckoutOutputRootExceeded, + "checkout search escaped output root", + ) + })?; *request.operation.path_mut() = WorkdirPath::new(relative.to_str().ok_or_else(|| { WorkdirError::InvalidPath("checkout search path is not UTF8".into()) })?)?; @@ -2088,7 +2148,10 @@ fn sanitize_error(error: WorkdirError, logical: &WorkdirPath) -> WorkdirError { WorkdirError::Io { source, .. } if source.kind() == std::io::ErrorKind::PermissionDenied => { - WorkdirError::OutOfScope(path) + WorkdirError::DenialContext { + reason: io_denial_reason(&source), + source: Box::new(WorkdirError::OutOfScope(path)), + } } WorkdirError::Io { source, .. } => WorkdirError::Unavailable(format!( "I/O operation failed for {logical}: {}", @@ -2587,6 +2650,58 @@ mod tests { use std::fs; use tempfile::TempDir; + #[test] + fn checkout_permission_mapping_distinguishes_provider_policy_from_os_without_reclassifying() { + use crate::WorkdirDenialReason as Reason; + use crate::http::{WorkdirTransportError, WorkdirTransportErrorCode as Code}; + use fs_operation::FsAccessPolicy; + let root = tempfile::tempdir().unwrap(); + let access = ScopeAccess::new( + Arc::new(Scope::writable(root.path()).unwrap()), + root.path(), + None, + true, + None, + ); + // Exercise the actual policy boundary, not a string-based stand-in. + let policy = access + .resolve_access_path(Path::new("/outside/private-secret")) + .unwrap_err(); + let os = std::io::Error::from_raw_os_error(libc::EACCES); + for (source, reason) in [ + (policy, Reason::ProviderRootExceeded), + (os, Reason::OsPermissionDenied), + ] { + let error = sanitize_error( + WorkdirError::io(Path::new("/host/private-secret"), source), + &WorkdirPath::new("file").unwrap(), + ); + assert_eq!(error.denial_reason(), Some(reason)); + assert_eq!(error.to_string(), "path is outside allowed scope: file"); + let transport = WorkdirTransportError::from_workdir_error(&error); + assert_eq!(transport.code, Code::OutOfScope); + assert_eq!(transport.message, "Workdir path is out of scope"); + assert_eq!(transport.denial_reason, Some(reason)); + assert_eq!( + WorkdirTransportError::from_workdir_error(&transport.clone().into_workdir_error()), + transport + ); + assert!( + !serde_json::to_string(&transport) + .unwrap() + .contains("private-secret") + ); + } + // Ordinary OS I/O remains the existing generic denied/403 classification. + let error = WorkdirError::io( + Path::new("/host/private-secret"), + std::io::Error::from_raw_os_error(libc::EACCES), + ); + let transport = WorkdirTransportError::from_workdir_error(&error); + assert_eq!(transport.code, Code::Denied); + assert_eq!(transport.denial_reason, Some(Reason::OsPermissionDenied)); + } + #[test] fn checkout_error_mapping_distinguishes_absence_from_stale_execution_at_all_boundaries() { let path = WorkdirPath::new("missing").unwrap(); 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 8bde43c29..8c59354ad 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, @@ -595,9 +605,10 @@ impl ScopedWorkdirSession { if self.capabilities.supports(required) { Ok(()) } else { - Err(WorkdirError::Denied(format!( - "scoped Workdir tools do not permit {operation}" - ))) + Err(WorkdirError::denied( + crate::WorkdirDenialReason::ScopedCapabilityDenied, + format!("scoped Workdir tools do not permit {operation}"), + )) } } @@ -612,9 +623,12 @@ impl ScopedWorkdirSession { .iter() .any(|rule| rule_allows_path(rule, path, permission)) { - return Err(WorkdirError::Denied(format!( - "logical workdir path `{path}` is outside the scoped {permission:?} scope" - ))); + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::LogicalScopeExceeded, + format!( + "logical workdir path `{path}` is outside the scoped {permission:?} scope" + ), + )); } } Ok(()) @@ -626,9 +640,17 @@ impl ScopedWorkdirSession { } let joined = Path::new(self.cwd.as_str()).join(path.as_str()); let joined = joined.to_str().ok_or_else(|| { - WorkdirError::Denied("logical Workdir path is not valid UTF-8".into()) + WorkdirError::denied( + crate::WorkdirDenialReason::InvalidScopedPath, + "logical Workdir path is not valid UTF-8", + ) })?; - FsPath::new(joined).map_err(|error| WorkdirError::Denied(error.to_string())) + FsPath::new(joined).map_err(|error| { + WorkdirError::denied( + crate::WorkdirDenialReason::InvalidScopedPath, + error.to_string(), + ) + }) } fn ensure_read( @@ -739,9 +761,10 @@ impl ScopedWorkdirSession { }) .await? { - return Err(WorkdirError::Denied(format!( - "path `{path}` is leased to child Workdir tools" - ))); + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::ChildWriteLeaseConflict, + format!("path `{path}` is leased to child Workdir tools"), + )); } } Ok(()) @@ -792,8 +815,9 @@ impl ScopedWorkdirSession { ) -> Result { self.ensure_active()?; if rules.is_empty() { - return Err(WorkdirError::Denied( - "workdir tool scope requires at least one logical scope rule".into(), + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::EmptyScope, + "workdir tool scope requires at least one logical scope rule", )); } let writable = rules @@ -804,22 +828,25 @@ impl ScopedWorkdirSession { && (!self.capabilities.supports(WorkdirSessionCapability::Write) || !self.capabilities.supports(WorkdirSessionCapability::Edit))) { - return Err(WorkdirError::Denied( - "parent Workdir session cannot scope the requested capabilities".into(), + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::ParentCapabilityDenied, + "parent Workdir session cannot scope the requested capabilities", )); } if command { if !writable { - return Err(WorkdirError::Denied( - "command execution requires a writable scoped path".into(), + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::CommandRequiresWritableScope, + "command execution requires a writable scoped path", )); } if !self .capabilities .supports(WorkdirSessionCapability::Command) { - return Err(WorkdirError::Denied( - "parent Workdir session does not support Command".into(), + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::ParentCapabilityDenied, + "parent Workdir session does not support Command", )); } } @@ -829,10 +856,13 @@ impl ScopedWorkdirSession { .iter() .any(|parent| rule_contains_rule(parent, requested)) { - return Err(WorkdirError::Denied(format!( - "logical workdir scope `{}` exceeds the parent tool scope", - requested.target - ))); + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::ParentScopeExceeded, + format!( + "logical workdir scope `{}` exceeds the parent tool scope", + requested.target + ), + )); } } } @@ -866,10 +896,13 @@ impl ScopedWorkdirSession { .iter() .any(|rule| rule_allows_path(rule, &request.cwd, WorkdirToolScopePermission::Read)) { - return Err(WorkdirError::Denied(format!( - "scoped tool cwd `{}` is outside the readable scope", - request.cwd - ))); + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::CwdOutsideReadableScope, + format!( + "scoped tool cwd `{}` is outside the readable scope", + request.cwd + ), + )); } self.ensure_scope_targets_are_authorized(&request.rules) .await?; @@ -913,10 +946,13 @@ impl ScopedWorkdirSession { }) .await? { - return Err(WorkdirError::Denied(format!( - "scoped write path `{}` overlaps an active child scope after provider resolution", - requested.target - ))); + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::ChildWriteLeaseConflict, + format!( + "scoped write path `{}` overlaps an active child scope after provider resolution", + requested.target + ), + )); } } } @@ -1491,7 +1527,10 @@ impl WorkdirSession for ReadOnlyWorkdirSession { request: WorkdirScopeAuthorizationRequest, ) -> Result<(), WorkdirError> { if request.permission == WorkdirToolScopePermission::Write { - return Err(WorkdirError::Denied("read-only workdir session".into())); + return Err(WorkdirError::denied( + crate::WorkdirDenialReason::ReadOnlySession, + "read-only workdir session", + )); } self.inner.authorize_scope_path(request).await } @@ -1554,11 +1593,17 @@ impl WorkdirSession for ReadOnlyWorkdirSession { } async fn write(&self, _request: WriteRequest) -> Result { - Err(WorkdirError::Denied("read-only workdir session".into())) + Err(WorkdirError::denied( + crate::WorkdirDenialReason::ReadOnlySession, + "read-only workdir session", + )) } async fn edit(&self, _request: EditRequest) -> Result { - Err(WorkdirError::Denied("read-only workdir session".into())) + Err(WorkdirError::denied( + crate::WorkdirDenialReason::ReadOnlySession, + "read-only workdir session", + )) } async fn list(&self, request: ListRequest) -> Result { @@ -1574,22 +1619,34 @@ impl WorkdirSession for ReadOnlyWorkdirSession { } async fn start_command(&self, _request: CommandRequest) -> Result { - Err(WorkdirError::Denied("read-only workdir session".into())) + Err(WorkdirError::denied( + crate::WorkdirDenialReason::ReadOnlySession, + "read-only workdir session", + )) } async fn command_status(&self, _handle: CommandHandle) -> Result { - Err(WorkdirError::Denied("read-only workdir session".into())) + Err(WorkdirError::denied( + crate::WorkdirDenialReason::ReadOnlySession, + "read-only workdir session", + )) } async fn command_output( &self, _request: CommandOutputRequest, ) -> Result { - Err(WorkdirError::Denied("read-only workdir session".into())) + Err(WorkdirError::denied( + crate::WorkdirDenialReason::ReadOnlySession, + "read-only workdir session", + )) } async fn cancel_command(&self, _handle: CommandHandle) -> Result<(), WorkdirError> { - Err(WorkdirError::Denied("read-only workdir session".into())) + Err(WorkdirError::denied( + crate::WorkdirDenialReason::ReadOnlySession, + "read-only workdir session", + )) } async fn close(&self) -> Result<(), WorkdirError> { @@ -2273,7 +2330,7 @@ mod tests { assert!(matches!( child.read(read("link")).await, Err(WorkdirError::Denied(message)) - if message.contains("provider-resolved delegated scope") + if message.message.contains("provider-resolved delegated scope") )); } @@ -2299,7 +2356,7 @@ mod tests { assert!(matches!( child.read(read("escape")).await, Err(WorkdirError::Denied(message)) - if message.contains("provider-resolved delegated scope") + if message.message.contains("provider-resolved delegated scope") )); } @@ -2400,7 +2457,7 @@ mod tests { .scope(request("alias-b", WorkdirToolScopePermission::Write)) .await, Err(WorkdirError::Denied(message)) - if message.contains("overlaps an active child scope after provider resolution") + if message.message.contains("overlaps an active child scope after provider resolution") )); assert!(matches!( parent @@ -2411,7 +2468,7 @@ mod tests { }) .await, Err(WorkdirError::Denied(message)) - if message.contains("leased to child Workdir tools") + if message.message.contains("leased to child Workdir tools") )); } @@ -2430,7 +2487,7 @@ mod tests { assert!(matches!( child.scope(expanded).await, Err(WorkdirError::Denied(message)) - if message.contains("exceeds the parent tool scope") + if message.message.contains("exceeds the parent tool scope") )); } @@ -2499,7 +2556,7 @@ mod tests { .write(write("secret/parent", "must-be-blocked")) .await, Err(WorkdirError::Denied(message)) - if message.contains("leased to child Workdir tools") + if message.message.contains("leased to child Workdir tools") )); assert_eq!( fs::read_to_string(root.path().join("secret/from-child")).unwrap(), 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/denial_reasons.rs b/crates/workdir/tests/denial_reasons.rs new file mode 100644 index 000000000..53c64d5dd --- /dev/null +++ b/crates/workdir/tests/denial_reasons.rs @@ -0,0 +1,333 @@ +//! Refusal reasons are selected by providers, not parsed from their messages. +use std::sync::Arc; + +use manifest::{Permission, Scope, ScopeConfig, ScopeRule, SharedScope, SymlinkPolicy}; +use workdir::{ + LocalWorkdirSession, ReadOnlyWorkdirSession, StatRequest, Workdir, + WorkdirDenialReason as Reason, WorkdirPath, WorkdirScopeAuthorizationRequest, WorkdirSession, + WorkdirSessionCapabilities, WorkdirToolBroker, WorkdirToolScope, + WorkdirToolScopePermission as ToolPermission, WorkdirToolScopeRule, WriteRequest, + http::{WorkdirTransportError, WorkdirTransportErrorCode as Code}, +}; + +fn path(value: &str) -> WorkdirPath { + WorkdirPath::new(value).unwrap() +} +fn rule(target: &str, permission: ToolPermission) -> WorkdirToolScopeRule { + WorkdirToolScopeRule { + target: path(target), + permission, + recursive: true, + symlink_policy: SymlinkPolicy::Resolved, + } +} +fn scoped(target: &str, permission: ToolPermission) -> WorkdirToolScope { + WorkdirToolScope { + rules: vec![rule(target, permission)], + cwd: path(target), + command: false, + } +} +fn assert_reason(error: workdir::WorkdirError, reason: Reason) { + assert_eq!(error.denial_reason(), Some(reason), "{error}"); + let transport = WorkdirTransportError::from_workdir_error(&error); + assert_eq!(transport.code.http_status(), 403); + assert_eq!(transport.denial_reason, Some(reason)); + let wire = serde_json::to_string(&transport).unwrap(); + assert!(!wire.contains("private-input")); + let decoded: WorkdirTransportError = serde_json::from_str(&wire).unwrap(); + assert_eq!( + WorkdirTransportError::from_workdir_error(&decoded.into_workdir_error()), + transport + ); +} + +#[tokio::test] +async fn local_authorize_scope_refusals_distinguish_attachment_and_delegation() { + let root = tempfile::tempdir().unwrap(); + let session = LocalWorkdirSession::materialized_bound( + Workdir::new("denials"), + root.path().to_owned(), + root.path().to_owned(), + SharedScope::new( + Scope::from_config(&ScopeConfig { + allow: vec![ScopeRule { + target: root.path().join("allowed"), + permission: Permission::Write, + recursive: true, + symlink_policy: SymlinkPolicy::Resolved, + }], + deny: vec![], + }) + .unwrap(), + ), + WorkdirSessionCapabilities::ALL, + ); + for (rules, requested, reason) in [ + ( + vec![rule("", ToolPermission::Read)], + "private-input", + Reason::AttachmentScopeExceeded, + ), + ( + vec![rule("allowed/child", ToolPermission::Read)], + "allowed/private-input", + Reason::DelegatedScopeExceeded, + ), + ] { + let error = session + .authorize_scope_path(WorkdirScopeAuthorizationRequest { + rules, + path: path(requested), + permission: ToolPermission::Read, + }) + .await + .unwrap_err(); + assert_reason(error, reason); + } +} + +#[tokio::test] +async fn broker_scope_refusal_matrix_preserves_distinct_reasons() { + let root = tempfile::tempdir().unwrap(); + let local = Arc::new(LocalWorkdirSession::new( + Scope::writable(root.path()).unwrap(), + root.path().to_owned(), + )); + let parent = WorkdirToolBroker::new(local); + let mut empty = scoped("", ToolPermission::Read); + empty.rules.clear(); + let mut command = scoped("", ToolPermission::Read); + command.command = true; + let mut cwd = scoped("allowed", ToolPermission::Read); + cwd.cwd = path("private-input"); + for (request, reason) in [ + (empty, Reason::EmptyScope), + (command, Reason::CommandRequiresWritableScope), + (cwd, Reason::CwdOutsideReadableScope), + ] { + assert_reason(parent.scope(request).await.unwrap_err(), reason); + } + let mut child_request = scoped("allowed", ToolPermission::Write); + child_request.rules[0].recursive = false; + let child = parent.scope(child_request).await.unwrap(); + assert_reason( + child + .scope(scoped("private-input", ToolPermission::Read)) + .await + .unwrap_err(), + Reason::ParentScopeExceeded, + ); + assert_reason( + parent + .scope(scoped("allowed", ToolPermission::Write)) + .await + .unwrap_err(), + Reason::ChildWriteLeaseConflict, + ); + assert_reason( + parent + .write(WriteRequest { + path: path("allowed/private-input"), + content: "unused".into(), + expected_hash: None, + }) + .await + .unwrap_err(), + Reason::ChildWriteLeaseConflict, + ); + assert_reason( + child + .stat(StatRequest { + path: path("private-input/nested"), + }) + .await + .unwrap_err(), + Reason::DelegatedScopeExceeded, + ); +} + +#[tokio::test] +async fn read_only_and_scoped_capability_refusals_do_not_collapse() { + let root = tempfile::tempdir().unwrap(); + let local = Arc::new(LocalWorkdirSession::new( + Scope::writable(root.path()).unwrap(), + root.path().to_owned(), + )); + let parent = WorkdirToolBroker::new(local.clone()); + let child = parent + .scope(scoped("", ToolPermission::Read)) + .await + .unwrap(); + let request = WriteRequest { + path: path("private-input"), + content: "unused".into(), + expected_hash: None, + }; + assert_reason( + child + .start_command(workdir::CommandRequest { + command: "must-not-run".into(), + timeout_secs: 1, + output_limit: 1, + cwd: WorkdirPath::root(), + spill_dir: None, + tool_call_id: None, + }) + .await + .unwrap_err(), + Reason::ScopedCapabilityDenied, + ); + let readonly = ReadOnlyWorkdirSession::new(local); + assert_reason( + readonly.write(request).await.unwrap_err(), + Reason::ReadOnlySession, + ); + let error = parent + .checkout_search(workdir::CheckoutSearchRequest { + operation: workdir::CheckoutSearchOperation::List(workdir::ListRequest { + path: path("private-input"), + limit: 10, + after: None, + }), + output_root: path("allowed"), + scope_layers: vec![], + }) + .await + .unwrap_err(); + assert_reason(error, Reason::CheckoutOutputRootExceeded); +} + +#[test] +fn denial_wire_is_allowlisted_and_old_missing_reason_remains_unknown() { + let old: WorkdirTransportError = + serde_json::from_str(r#"{"code":"denied","message":"private-input secret"}"#).unwrap(); + let error = old.into_workdir_error(); + assert_eq!(error.denial_reason(), None); + let forwarded = WorkdirTransportError::from_workdir_error(&error); + assert_eq!(forwarded.code, Code::Denied); + assert_eq!(forwarded.message, "Workdir operation was denied"); + assert_eq!( + serde_json::to_value(forwarded).unwrap(), + serde_json::json!({"code":"denied", "message":"Workdir operation was denied"}) + ); + for reason in ["/host/private-input", "secret", &"x".repeat(8192)] { + let input = + serde_json::json!({"code":"denied", "message":"ignored", "denial_reason":reason}); + assert!(serde_json::from_value::(input).is_err()); + } +} + +// A provider with filesystem methods, but no provider-side scope resolver. +#[derive(Debug)] +struct UnresolvedProvider(LocalWorkdirSession); +#[async_trait::async_trait] +impl WorkdirSession for UnresolvedProvider { + fn workdir(&self) -> &Workdir { + self.0.workdir() + } + fn capabilities(&self) -> WorkdirSessionCapabilities { + self.0.capabilities() + } + async fn stat(&self, r: StatRequest) -> Result { + self.0.stat(r).await + } + async fn read( + &self, + r: workdir::ReadRequest, + ) -> Result { + self.0.read(r).await + } + async fn write(&self, r: WriteRequest) -> Result { + self.0.write(r).await + } + async fn edit( + &self, + r: workdir::EditRequest, + ) -> Result { + self.0.edit(r).await + } + async fn list( + &self, + r: workdir::ListRequest, + ) -> Result { + self.0.list(r).await + } + async fn glob( + &self, + r: workdir::GlobRequest, + ) -> Result { + self.0.glob(r).await + } + async fn grep( + &self, + r: workdir::GrepRequest, + ) -> Result { + self.0.grep(r).await + } + async fn start_command( + &self, + r: workdir::CommandRequest, + ) -> Result { + self.0.start_command(r).await + } + async fn command_status( + &self, + h: workdir::CommandHandle, + ) -> Result { + self.0.command_status(h).await + } + async fn command_output( + &self, + r: workdir::CommandOutputRequest, + ) -> Result { + self.0.command_output(r).await + } + async fn cancel_command(&self, h: workdir::CommandHandle) -> Result<(), workdir::WorkdirError> { + self.0.cancel_command(h).await + } + async fn close(&self) -> Result<(), workdir::WorkdirError> { + self.0.close().await + } +} + +#[tokio::test] +async fn providers_without_scope_resolution_fail_closed_with_specific_reasons() { + let root = tempfile::tempdir().unwrap(); + let provider = UnresolvedProvider(LocalWorkdirSession::new( + Scope::writable(root.path()).unwrap(), + root.path().to_owned(), + )); + let rules = vec![rule("", ToolPermission::Read)]; + assert_reason( + provider + .authorize_scope_path(WorkdirScopeAuthorizationRequest { + rules: rules.clone(), + path: path("private-input"), + permission: ToolPermission::Read, + }) + .await + .unwrap_err(), + Reason::ScopeResolutionUnavailable, + ); + assert_reason( + provider + .scope_rules_overlap(workdir::WorkdirScopeOverlapRequest { + left: rules[0].clone(), + right: rules[0].clone(), + }) + .await + .unwrap_err(), + Reason::ScopeComparisonUnavailable, + ); + let mut logical = rules; + logical[0].symlink_policy = SymlinkPolicy::Logical; + provider + .authorize_scope_path(WorkdirScopeAuthorizationRequest { + rules: logical, + path: path("private-input"), + permission: ToolPermission::Read, + }) + .await + .unwrap(); +} diff --git a/crates/workdir/tests/fs_denial_origins.rs b/crates/workdir/tests/fs_denial_origins.rs new file mode 100644 index 000000000..3a19b0572 --- /dev/null +++ b/crates/workdir/tests/fs_denial_origins.rs @@ -0,0 +1,288 @@ +use std::path::Path; + +use fs_operation::{AtomicWriteMode, CheckedTarget, FsAccessPolicy, FsDenialReason, FsError}; +use workdir::{ + WorkdirDenialReason as Reason, WorkdirError, + http::{WorkdirTransportError, WorkdirTransportErrorCode as Code}, +}; + +fn assert_origin(source: std::io::Error, reason: Reason, message: &str) { + assert_eq!(source.kind(), std::io::ErrorKind::PermissionDenied); + assert_eq!(source.raw_os_error(), None); + assert_eq!(source.to_string(), message); + assert_transport_reason(source, reason); +} + +fn assert_transport_reason(source: std::io::Error, reason: Reason) { + let error = WorkdirError::from(FsError::io("/private-input/secret", source)); + assert!(matches!(&error, WorkdirError::Io { .. })); + assert_eq!(error.denial_reason(), Some(reason)); + let transport = WorkdirTransportError::from_workdir_error(&error); + assert_eq!(transport.code, Code::Denied); + assert_eq!(transport.code.http_status(), 403); + assert_eq!(transport.message, "Workdir operation was denied"); + let wire = serde_json::to_value(&transport).unwrap(); + assert_eq!(wire["denial_reason"], reason.as_str()); + assert!(!wire.to_string().contains("private-input")); + assert!(!wire.to_string().contains("secret")); + let decoded: WorkdirTransportError = serde_json::from_value(wire).unwrap(); + assert_eq!( + WorkdirTransportError::from_workdir_error(&decoded.into_workdir_error()), + transport + ); +} + +#[test] +fn synthetic_fs_origins_survive_fs_workdir_and_transport_conversion() { + for (origin, reason, message) in [ + ( + FsDenialReason::ProviderRootExceeded, + Reason::ProviderRootExceeded, + "path is outside provider root", + ), + ( + FsDenialReason::ProviderSymlinkDenied, + Reason::ProviderSymlinkDenied, + "symbolic links are not permitted by this provider", + ), + ( + FsDenialReason::CheckoutTargetDenied, + Reason::CheckoutTargetDenied, + "checkout target denied", + ), + ( + FsDenialReason::CheckoutCreateParentDenied, + Reason::CheckoutCreateParentDenied, + "Create parent denied", + ), + ] { + assert_origin(origin.into_io_error(), reason, message); + } +} + +#[test] +fn custom_permission_denied_is_not_guessed_from_kind_or_message() { + for message in [ + "Permission denied (os error 13)", + "checkout target denied", + "/private-input/secret", + ] { + assert_transport_reason( + std::io::Error::new(std::io::ErrorKind::PermissionDenied, message), + Reason::UnclassifiedPermissionDenied, + ); + } +} + +#[cfg(unix)] +#[test] +fn raw_os_permission_denied_retains_errno_and_os_origin() { + for errno in [libc::EACCES, libc::EPERM] { + let source = std::io::Error::from_raw_os_error(errno); + let error = WorkdirError::from(FsError::io("/private-input/secret", source)); + let WorkdirError::Io { source, .. } = &error else { + panic!("classification changed") + }; + assert_eq!(source.raw_os_error(), Some(errno)); + assert_transport_reason( + std::io::Error::from_raw_os_error(errno), + Reason::OsPermissionDenied, + ); + } +} + +#[cfg(target_os = "linux")] +#[test] +fn actual_confined_open_and_write_refusals_keep_distinct_origins() { + use fs_operation::{ + atomic_write_beneath_no_symlinks_at, open_beneath_no_symlinks, open_beneath_no_symlinks_at, + open_root_no_symlinks, + }; + let root = tempfile::tempdir().unwrap(); + let directory = open_root_no_symlinks(root.path()).unwrap(); + let outside = root.path().parent().unwrap().join("private-input"); + for source in [ + open_beneath_no_symlinks_at(&directory, &outside).unwrap_err(), + open_beneath_no_symlinks(root.path(), &outside).unwrap_err(), + atomic_write_beneath_no_symlinks_at( + &directory, + &outside, + b"unused", + AtomicWriteMode::CreateNew, + || Ok(()), + ) + .unwrap_err(), + atomic_write_beneath_no_symlinks_at( + &directory, + Path::new("../private-input"), + b"unused", + AtomicWriteMode::CreateNew, + || Ok(()), + ) + .unwrap_err(), + ] { + assert_origin( + source, + Reason::ProviderRootExceeded, + "path is outside provider root", + ); + } + std::fs::write(root.path().join("file"), "secret").unwrap(); + std::os::unix::fs::symlink("file", root.path().join("link")).unwrap(); + assert_origin( + open_beneath_no_symlinks_at(&directory, Path::new("link")).unwrap_err(), + Reason::ProviderSymlinkDenied, + "symbolic links are not permitted by this provider", + ); +} + +struct ExactWrite<'a>(&'a Path); +impl FsAccessPolicy for ExactWrite<'_> { + fn is_readable(&self, _: &Path) -> bool { + true + } + fn is_writable(&self, path: &Path) -> bool { + path == self.0 + } +} + +#[cfg(target_os = "linux")] +#[test] +fn actual_checked_target_and_create_parent_refusals_keep_distinct_origins() { + let root = tempfile::tempdir().unwrap(); + let directory = fs_operation::open_root_no_symlinks(root.path()).unwrap(); + let destination = root.path().join("parent/file"); + let policy = ExactWrite(&destination); + let source = CheckedTarget::pin(&directory, Path::new(""), root.path(), &policy, true) + .err() + .expect("target policy must reject"); + assert_origin( + source, + Reason::CheckoutTargetDenied, + "checkout target denied", + ); + let target = + CheckedTarget::pin(&directory, Path::new(""), root.path(), &policy, false).unwrap(); + let create = target.create_access("parent/file".into(), destination.clone()); + let error = fs_operation::run_write( + root.path(), + fs_operation::WriteRequest { + path: fs_operation::FsPath::new("parent/file").unwrap(), + content: "unused".into(), + expected_hash: None, + }, + &create, + ) + .unwrap_err(); + let FsError::Io { source, .. } = error else { + panic!("expected parent policy denial") + }; + assert_origin( + source, + Reason::CheckoutCreateParentDenied, + "Create parent denied", + ); + assert!(!root.path().join("parent").exists()); +} + +#[test] +fn every_transport_code_forwards_supplied_reason_without_reclassification() { + // JSON is the provider boundary. An optional reason is independent of code/status. + for code in [ + "not_found", + "conflict", + "outcome_unknown", + "unsupported", + "invalid_request", + "denied", + "out_of_scope", + "symlink_out_of_scope", + "broken_symlink", + "symlink_target_is_directory", + "read_only", + "is_directory", + "symlink_directory_not_traversed", + "unknown_command", + "unavailable", + "io", + "transport", + "internal", + ] { + let decode = |reason| { + serde_json::from_value::(serde_json::json!({ + "code": code, "message": "private-input secret", "denial_reason": reason, + })) + .unwrap() + .into_workdir_error() + }; + let baseline_error = decode(None::); + let baseline = WorkdirTransportError::from_workdir_error(&baseline_error); + let error = decode(Some(Reason::CheckoutTargetDenied)); + let forwarded = WorkdirTransportError::from_workdir_error(&error); + assert_eq!( + error.denial_reason(), + Some(Reason::CheckoutTargetDenied), + "{code}" + ); + assert_eq!(forwarded.code, baseline.code, "{code}"); + assert_eq!( + forwarded.code.http_status(), + baseline.code.http_status(), + "{code}" + ); + assert_eq!(forwarded.message, baseline.message, "{code}"); + assert_eq!(error.to_string(), baseline_error.to_string(), "{code}"); + // Missing provider reasons retain the deliberate existing fallback: canonical + // path classifications supply their known reason; all other codes stay unknown. + assert_eq!( + baseline_error.denial_reason(), + match code { + "out_of_scope" => Some(Reason::PathOutOfScope), + "symlink_out_of_scope" => Some(Reason::SymlinkTargetOutOfScope), + "read_only" => Some(Reason::PathReadOnly), + _ => None, + }, + "{code}" + ); + } +} + +#[test] +fn diagnostic_labels_match_allowlisted_wire_values() { + for label in [ + "scope_resolution_unavailable", + "scope_comparison_unavailable", + "scoped_capability_denied", + "logical_scope_exceeded", + "invalid_scoped_path", + "child_write_lease_conflict", + "empty_scope", + "parent_capability_denied", + "command_requires_writable_scope", + "parent_scope_exceeded", + "cwd_outside_readable_scope", + "read_only_session", + "external_root_symlink", + "external_root_changed", + "external_root_moved", + "external_scope_symlink", + "attachment_scope_exceeded", + "delegated_scope_exceeded", + "checkout_output_root_exceeded", + "provider_root_exceeded", + "provider_symlink_denied", + "checkout_target_denied", + "checkout_create_parent_denied", + "checkout_search_path_denied", + "checkout_enumeration_denied", + "path_out_of_scope", + "symlink_target_out_of_scope", + "path_read_only", + "os_permission_denied", + "unclassified_permission_denied", + ] { + let reason: Reason = serde_json::from_value(serde_json::json!(label)).unwrap(); + assert_eq!(reason.as_str(), label); + assert_eq!(serde_json::to_value(reason).unwrap(), label); + } +} diff --git a/crates/workdir/tests/remote_denial_reasons.rs b/crates/workdir/tests/remote_denial_reasons.rs new file mode 100644 index 000000000..f14acc816 --- /dev/null +++ b/crates/workdir/tests/remote_denial_reasons.rs @@ -0,0 +1,218 @@ +#![cfg(feature = "http-client")] +//! Loopback fake at the HTTP provider boundary; no product process or live service. +use std::{ + io::{Read, Write}, + net::{TcpListener, TcpStream}, + thread, + time::{Duration, Instant}, +}; +use workdir::{ + StatRequest, Workdir, WorkdirDenialReason as Reason, WorkdirPath, WorkdirSession, + http::{ + OpenWorkdirSessionRequest, RemoteWorkdirSession, WorkdirTransportError, + WorkdirTransportErrorCode as Code, + }, +}; + +fn reply(listener: &TcpListener, status: &str, body: &str) { + 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(); + stream + .set_write_timeout(Some(Duration::from_secs(5))) + .unwrap(); + read_request(&mut stream); + write!(stream, "HTTP/1.1 {status}\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}", body.len()).unwrap(); +} +fn read_request(stream: &mut TcpStream) { + let mut bytes = Vec::new(); + let mut byte = [0]; + while !bytes.ends_with(b"\r\n\r\n") { + stream.read_exact(&mut byte).unwrap(); + bytes.push(byte[0]); + assert!(bytes.len() < 16384); + } + let headers = String::from_utf8(bytes).unwrap(); + let length: usize = headers + .lines() + .find_map(|line| { + let (name, value) = line.split_once(':')?; + name.eq_ignore_ascii_case("content-length") + .then(|| value.trim().parse().unwrap()) + }) + .unwrap_or(0); + assert!(length < 16384); + stream.read_exact(&mut vec![0; length]).unwrap(); +} + +#[tokio::test] +async fn remote_http_denials_forward_only_typed_reasons_and_old_missing_reason() { + for (code, reason) in [ + (Code::Denied, Some(Reason::ReadOnlySession)), + (Code::Denied, Some(Reason::OsPermissionDenied)), + (Code::OutOfScope, Some(Reason::OsPermissionDenied)), + (Code::Denied, None), + ] { + let listener = TcpListener::bind("127.0.0.1:0").unwrap(); + let address = listener.local_addr().unwrap(); + // Host paths and secrets in provider-authored text must not become diagnostics. + let mut error = serde_json::json!({"code":code, "message":"/secret/host/root bearer-secret command-secret"}); + if let Some(reason) = reason { + error["denial_reason"] = serde_json::to_value(reason).unwrap(); + } + let server = thread::spawn(move || { + reply( + &listener, + "200 OK", + r#"{"session_id":"s","workdir_id":"w","capabilities":{"bits":63}}"#, + ); + reply(&listener, "403 Forbidden", &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 denied = remote + .stat(StatRequest { + path: WorkdirPath::new("file").unwrap(), + }) + .await + .unwrap_err(); + assert_eq!(denied.denial_reason(), reason); + assert!(!denied.to_string().contains("secret")); + let forwarded = WorkdirTransportError::from_workdir_error(&denied); + assert_eq!(forwarded.denial_reason, reason); + assert_eq!(forwarded.code, code); + assert_eq!(forwarded.code.http_status(), 403); + assert_eq!( + forwarded.message, + if code == Code::Denied { + "Workdir operation was denied" + } else { + "Workdir path is out of scope" + } + ); + let wire = serde_json::to_string(&forwarded).unwrap(); + assert!(!wire.contains("secret")); + assert!(wire.len() < 256); + 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/worker-runtime/src/http_server.rs b/crates/worker-runtime/src/http_server.rs index 336817c51..d5d97c67f 100644 --- a/crates/worker-runtime/src/http_server.rs +++ b/crates/worker-runtime/src/http_server.rs @@ -2359,6 +2359,7 @@ impl RuntimeHttpWorkdirError { Self { status, payload: WorkdirTransportError { + denial_reason: None, code, message: message.into(), }, diff --git a/crates/worker/src/checkout.rs b/crates/worker/src/checkout.rs index adba8a547..ce2896062 100644 --- a/crates/worker/src/checkout.rs +++ b/crates/worker/src/checkout.rs @@ -191,15 +191,20 @@ impl Provider { }; let observation = match selected.session.checkout_observe(target.clone()).await { Ok(o) => o, - Err( - workdir::WorkdirError::Denied(_) - | workdir::WorkdirError::OutOfScope(_) - | workdir::WorkdirError::NotFound(_) - | workdir::WorkdirError::SessionClosed - | workdir::WorkdirError::UnsupportedOperation(_) - | workdir::WorkdirError::Unsupported(_), - ) => return Ok(None), - Err(_) => { + Err(failure) => { + // Internal diagnostic enrichment is transparent to WIP's existing + // unavailable/refusal classification, including remote providers. + if matches!( + failure.classification_source(), + workdir::WorkdirError::Denied(_) + | workdir::WorkdirError::OutOfScope(_) + | workdir::WorkdirError::NotFound(_) + | workdir::WorkdirError::SessionClosed + | workdir::WorkdirError::UnsupportedOperation(_) + | workdir::WorkdirError::Unsupported(_) + ) { + return Ok(None); + } return Err(error( ProtocolErrorCode::Internal, "checkout observation unavailable", diff --git a/crates/worker/src/checkout_http_tests.rs b/crates/worker/src/checkout_http_tests.rs index e74e054f6..f7df26e26 100644 --- a/crates/worker/src/checkout_http_tests.rs +++ b/crates/worker/src/checkout_http_tests.rs @@ -36,6 +36,7 @@ struct ProviderState { requests: Mutex>, operations: Mutex>, write_fault: Mutex>, + observe_refusal: Mutex, } impl ProviderState { @@ -49,6 +50,7 @@ impl ProviderState { return Err(( StatusCode::UNAUTHORIZED, AxumJson(WorkdirTransportError { + denial_reason: None, code: WorkdirTransportErrorCode::Denied, message: "provider authorization denied".into(), }), @@ -140,6 +142,16 @@ async fn operate( .lock() .unwrap() .push(request.operation.clone()); + if matches!( + &request.operation, + WorkdirSessionOperation::CheckoutObserve(_) + ) && *state.observe_refusal.lock().unwrap() + { + return provider_error(WorkdirError::DenialContext { + reason: workdir::WorkdirDenialReason::OsPermissionDenied, + source: Box::new(WorkdirError::OutOfScope("/private/provider/path".into())), + }); + } let result = dispatch_workdir_session_operation(state.session.as_ref(), request.operation).await; match result { @@ -225,6 +237,7 @@ impl HttpFixture { requests: Mutex::new(Vec::new()), operations: Mutex::new(Vec::new()), write_fault: Mutex::new(None), + observe_refusal: Mutex::new(false), }); let app = Router::new() .route( @@ -758,3 +771,30 @@ async fn remote_checkout_list_pages_and_direct_inspect_read_use_provider_without .any(|op| matches!(op, WorkdirSessionOperation::List(_))) ); } + +#[tokio::test] +async fn remote_checkout_diagnostic_context_keeps_wip_refusal_classification_without_retry() { + let fixture = HttpFixture::new(WorkdirSessionCapabilities::READ_ONLY).await; + *fixture.state.observe_refusal.lock().unwrap() = true; + let remote = fixture.open(TOKEN).await.unwrap(); + let (runtime, _) = native_runtime(Arc::new(remote)); + let failure = runtime + .inspect("/checkouts/main/src/deep/a.txt".into(), true) + .await + .unwrap_err(); + let text = failure.to_string(); + assert!( + !text.contains("checkout observation unavailable"), + "refusal became an internal error: {text}" + ); + assert!(!text.contains("/private/provider/path")); + assert!( + text.contains("protocol failure (NotFound)"), + "expected unavailable Object: {text}" + ); + assert_eq!( + fixture.state.operations.lock().unwrap().len(), + 1, + "refusal was retried" + ); +} diff --git a/crates/worker/src/feature/builtin/manage_workdir.rs b/crates/worker/src/feature/builtin/manage_workdir.rs index 498996cf3..8cd5477be 100644 --- a/crates/worker/src/feature/builtin/manage_workdir.rs +++ b/crates/worker/src/feature/builtin/manage_workdir.rs @@ -1236,6 +1236,7 @@ mod tests { WorkspaceResponse { status, body: serde_json::to_string(&workdir::http::WorkdirTransportError { + denial_reason: None, code, message: message.to_string(), }) diff --git a/crates/workspace-server/src/hosts.rs b/crates/workspace-server/src/hosts.rs index 642fd44dd..b2efc5bb9 100644 --- a/crates/workspace-server/src/hosts.rs +++ b/crates/workspace-server/src/hosts.rs @@ -794,6 +794,10 @@ pub struct RuntimeSessionAttachment { #[derive(Debug, Clone, PartialEq, Eq)] pub enum RuntimeRegistryError { + WorkdirSessionOpenFailed { + runtime_id: String, + transport: workdir::http::WorkdirTransportError, + }, InvalidIdentifier { kind: &'static str, value: String, @@ -823,11 +827,19 @@ impl RuntimeRegistryError { worker.worker_id, worker.runtime_id ), Self::RuntimeOperationFailed { message, .. } => message.clone(), + Self::WorkdirSessionOpenFailed { transport, .. } => transport.message.clone(), } } pub fn into_error(self) -> Error { match self { + Self::WorkdirSessionOpenFailed { + runtime_id, + transport, + } => Error::WorkdirSessionOpenFailed { + runtime_id, + transport, + }, Self::InvalidIdentifier { kind, value } => Error::InvalidRuntimeIdentifier { kind: kind.to_string(), value, @@ -1885,10 +1897,9 @@ impl RuntimeRegistry { runtime .open_workdir_session(working_directory_id, owner_worker_id) .await - .map_err(|error| RuntimeRegistryError::RuntimeOperationFailed { + .map_err(|error| RuntimeRegistryError::WorkdirSessionOpenFailed { runtime_id: runtime_id.to_string(), - code: "workdir_session_open_failed".to_string(), - message: error.to_string(), + transport: workdir::http::WorkdirTransportError::from_workdir_error(&error), }) } diff --git a/crates/workspace-server/src/lib.rs b/crates/workspace-server/src/lib.rs index 32d2dd2d2..cb605799c 100644 --- a/crates/workspace-server/src/lib.rs +++ b/crates/workspace-server/src/lib.rs @@ -106,6 +106,11 @@ pub enum Error { code: String, message: String, }, + #[error("runtime `{runtime_id}` operation failed (workdir_session_open_failed): {}", transport.message)] + WorkdirSessionOpenFailed { + runtime_id: String, + transport: workdir::http::WorkdirTransportError, + }, #[error("runtime `{runtime_id}` does not support `{capability}`")] RuntimeCapabilityUnsupported { runtime_id: String, diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 6fd3e3741..c22a1b963 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), } @@ -11436,6 +11447,15 @@ impl server_api::ServerApi for ServerApiContractService { server_api::CurrentWorkerWorkdirOperationResponse, server_api::WorkdirOperationApiError, > { + let fallback_log = server_api::WorkdirOperationLogContext { + operation: workdir_operation_label(&request.operation), + stage: "worker_identity", + workspace_id_hash: crate::worker_source::diagnostic_id_hash(&workspace_id), + attachment_alias_hash: crate::worker_source::diagnostic_id_hash( + &request.target_workdir, + ), + ..Default::default() + }; let Json(response) = scoped_execute_current_worker_workdir_operation( State( self.workspace_api() @@ -11443,22 +11463,18 @@ impl server_api::ServerApi for ServerApiContractService { .clone(), ), AxumPath(ScopedWorkspacePath { workspace_id }), - current_worker_contract_headers(&context) - .map_err(server_api::WorkdirOperationApiError::api)?, + current_worker_contract_headers(&context).map_err(|error| { + let mut error = server_api::WorkdirOperationApiError::api(error); + error.workdir_log = Some(fallback_log); + error + })?, Json(WorkspaceWorkdirSessionOperationRequest { target_workdir: request.target_workdir, operation: request.operation, }), ) .await - .map_err(|error| match error { - InternalWorkdirOperationError::Api(error) => { - server_api::WorkdirOperationApiError::api(error.into_repository_api_error()) - } - InternalWorkdirOperationError::Provider(error) => { - server_api::WorkdirOperationApiError::provider(error) - } - })?; + .map_err(InternalWorkdirOperationError::into_contract_error)?; Ok(server_api::CurrentWorkerWorkdirOperationResponse(response)) } @@ -11700,6 +11716,10 @@ fn build_inner_router(api: WorkspaceApi) -> Router { async fn log_failed_api_response(request: Request, next: Next) -> Response { let method = request.method().clone(); let uri = request.uri().clone(); + let request_token_id_hash = request + .extensions() + .get::() + .and_then(|source| source.token_id_hash.clone()); let response = next.run(request).await; let status = response.status(); @@ -11720,11 +11740,23 @@ async fn log_failed_api_response(request: Request, next: Next) -> Response { .get::() .or(generated_error.as_ref()); let event = api_failure_log_event(&method, &uri, status, error); + let context = error.and_then(|error| error.workdir_log.as_ref()); + let workdir_path = context.map(|_| bounded_auth_log_value(uri.path(), 512)); + let workdir_method = context.map(|_| bounded_auth_log_value(method.as_str(), 32)); tracing::error!( target: "yoi::api", event = event.event, - method = event.method, - path = event.path, + method = workdir_method.as_deref().unwrap_or(event.method), + path = workdir_path.as_deref().unwrap_or(event.path), + operation = context.map(|ctx| ctx.operation), + workdir_stage = context.map(|ctx| ctx.stage), + workspace_id_hash = context.map(|ctx| ctx.workspace_id_hash.as_str()), + runtime_id_hash = context.and_then(|ctx| ctx.runtime_id_hash.as_deref()), + worker_id_hash = context.and_then(|ctx| ctx.worker_id_hash.as_deref()), + workdir_id_hash = context.and_then(|ctx| ctx.workdir_id_hash.as_deref()), + attachment_alias_hash = context.map(|ctx| ctx.attachment_alias_hash.as_str()), + request_token_id_hash = context.and(request_token_id_hash.as_deref()), + denial_reason = error.and_then(|error| error.denial_reason).map(|reason| reason.as_str()), status = event.status, kind = event.kind.unwrap_or("unknown"), message = event.message.unwrap_or(""), @@ -17202,9 +17234,10 @@ fn ticket_notification_delivery_error_category( Err(RuntimeRegistryError::UnknownRuntime(_)) => Some("unknown_runtime"), Err(RuntimeRegistryError::UnknownHost(_)) => Some("unknown_host"), Err(RuntimeRegistryError::UnknownWorker { .. }) => Some("unknown_worker"), - Err(RuntimeRegistryError::RuntimeOperationFailed { .. }) => { - Some("runtime_operation_failed") - } + Err( + RuntimeRegistryError::WorkdirSessionOpenFailed { .. } + | RuntimeRegistryError::RuntimeOperationFailed { .. }, + ) => Some("runtime_operation_failed"), } } @@ -18195,6 +18228,33 @@ fn validated_current_worker_attachment( enum InternalWorkdirOperationError { Api(ApiError), Provider(WorkdirTransportError), + Contextual { + error: Box, + context: server_api::WorkdirOperationLogContext, + }, +} + +impl InternalWorkdirOperationError { + fn into_contract_error(self) -> server_api::WorkdirOperationApiError { + match self { + Self::Api(error) => { + let reason = match &error.error { + Error::WorkdirSessionOpenFailed { transport, .. } => transport.denial_reason, + _ => None, + }; + let mut error = + server_api::WorkdirOperationApiError::api(error.into_repository_api_error()); + error.denial_reason = reason; + error + } + Self::Provider(error) => server_api::WorkdirOperationApiError::provider(error), + Self::Contextual { error, context } => { + let mut error = error.into_contract_error(); + error.workdir_log = Some(context); + error + } + } + } } impl From for InternalWorkdirOperationError { @@ -18217,21 +18277,35 @@ impl From for InternalWorkdirOperationError { impl IntoResponse for InternalWorkdirOperationError { fn into_response(self) -> Response { - match self { - Self::Api(error) => error.into_response(), - Self::Provider(error) => { - let status = StatusCode::from_u16(error.code.http_status()) - .expect("Workdir transport error status is valid"); - let log = ApiErrorLog { - kind: format!("workdir_session_operation_{}", error.code.as_str()), - message: error.message.clone(), - diagnostics: Vec::new(), - }; - let mut response = (status, Json(error)).into_response(); - response.extensions_mut().insert(log); - response - } - } + let error = self.into_contract_error(); + let status = StatusCode::from_u16(api_macros::HttpError::status_code(&error)) + .expect("Workdir error status is valid"); + let log = ApiErrorLog::from_workdir_operation_error(&error, status); + let mut response = (status, Json(error)).into_response(); + response.extensions_mut().insert(log); + response + } +} + +fn workdir_operation_label(operation: &WorkdirSessionOperation) -> &'static str { + match operation { + WorkdirSessionOperation::AuthorizeScope(_) => "authorize_scope", + WorkdirSessionOperation::ScopeRulesOverlap(_) => "scope_rules_overlap", + WorkdirSessionOperation::CheckoutSearch(_) => "checkout_search", + WorkdirSessionOperation::CheckoutObserve(_) => "checkout_observe", + WorkdirSessionOperation::CheckoutExecute(_) => "checkout_execute", + WorkdirSessionOperation::Stat(_) => "stat", + WorkdirSessionOperation::Read(_) => "read", + WorkdirSessionOperation::ReadBytes(_) => "read_bytes", + WorkdirSessionOperation::Write(_) => "write", + WorkdirSessionOperation::Edit(_) => "edit", + WorkdirSessionOperation::List(_) => "list", + WorkdirSessionOperation::Glob(_) => "glob", + WorkdirSessionOperation::Grep(_) => "grep", + WorkdirSessionOperation::CommandStart(_) => "command_start", + WorkdirSessionOperation::CommandStatus(_) => "command_status", + WorkdirSessionOperation::CommandOutput(_) => "command_output", + WorkdirSessionOperation::CommandCancel(_) => "command_cancel", } } @@ -18241,19 +18315,35 @@ async fn scoped_execute_current_worker_workdir_operation( headers: HeaderMap, Json(request): Json, ) -> std::result::Result, InternalWorkdirOperationError> { + let mut context = server_api::WorkdirOperationLogContext { + operation: workdir_operation_label(&request.operation), + workspace_id_hash: crate::worker_source::diagnostic_id_hash(&path.workspace_id), + attachment_alias_hash: crate::worker_source::diagnostic_id_hash(&request.target_workdir), + ..Default::default() + }; + let result = async { + context.stage = "workspace_scope"; validate_workspace_scope(&api, &path.workspace_id)?; + context.stage = "worker_identity"; let worker = current_worker_identity(&api, &path.workspace_id, &headers)?; + context.runtime_id_hash = Some(crate::worker_source::diagnostic_id_hash(&worker.runtime_id)); + context.worker_id_hash = Some(crate::worker_source::diagnostic_id_hash(&worker.worker_id)); let target_workdir = request.target_workdir; let result = match request.operation { WorkdirSessionOperation::CommandStart(command) => { let session_lock = current_worker_session_lock(&api, &worker); let _session_guard = session_lock.lock().await; + context.stage = "attachment_validation"; let link = validated_current_worker_attachment(&api, &worker, &target_workdir)?; + context.workdir_id_hash = Some(crate::worker_source::diagnostic_id_hash(&link.workdir_id)); + context.stage = "session_open"; let source = open_current_worker_workdir_session_locked(&api, &worker, &link).await?; + context.stage = "provider_dispatch"; let provider_handle = source .start_command(command) .await - .map_err(|error| current_worker_workdir_operation_error(&worker, error))?; + .map_err(workdir_operation_transport_error)?; + context.stage = "command_registration"; let registered_source = api .workdir_sessions .lock() @@ -18279,31 +18369,31 @@ async fn scoped_execute_current_worker_workdir_operation( } WorkdirSessionOperation::CommandStatus(external_handle) => { let (session, provider_handle) = - current_worker_command_session(&api, &worker, &target_workdir, &external_handle)?; + current_worker_command_session(&api, &worker, &target_workdir, &external_handle, &mut context)?; session .command_status(provider_handle) .await .map(WorkdirSessionOperationResult::CommandStatus) - .map_err(|error| current_worker_workdir_operation_error(&worker, error))? + .map_err(workdir_operation_transport_error)? } WorkdirSessionOperation::CommandOutput(mut output) => { let (session, provider_handle) = - current_worker_command_session(&api, &worker, &target_workdir, &output.handle)?; + current_worker_command_session(&api, &worker, &target_workdir, &output.handle, &mut context)?; output.handle = provider_handle; session .command_output(output) .await .map(WorkdirSessionOperationResult::CommandOutput) - .map_err(|error| current_worker_workdir_operation_error(&worker, error))? + .map_err(workdir_operation_transport_error)? } WorkdirSessionOperation::CommandCancel(external_handle) => { let (session, provider_handle) = - current_worker_command_session(&api, &worker, &target_workdir, &external_handle)?; + current_worker_command_session(&api, &worker, &target_workdir, &external_handle, &mut context)?; session .cancel_command(provider_handle) .await .map(|()| WorkdirSessionOperationResult::CommandCancel) - .map_err(|error| current_worker_workdir_operation_error(&worker, error))? + .map_err(workdir_operation_transport_error)? } operation @ (WorkdirSessionOperation::AuthorizeScope(_) | WorkdirSessionOperation::ScopeRulesOverlap(_) @@ -18320,14 +18410,23 @@ async fn scoped_execute_current_worker_workdir_operation( | WorkdirSessionOperation::Grep(_)) => { let session_lock = current_worker_session_lock(&api, &worker); let _session_guard = session_lock.lock().await; + context.stage = "attachment_validation"; let link = validated_current_worker_attachment(&api, &worker, &target_workdir)?; + context.workdir_id_hash = Some(crate::worker_source::diagnostic_id_hash(&link.workdir_id)); + context.stage = "session_open"; let source = open_current_worker_workdir_session_locked(&api, &worker, &link).await?; + context.stage = "provider_dispatch"; execute_workdir_session_operation(&source, operation) .await - .map_err(|error| current_worker_workdir_operation_error(&worker, error))? + .map_err(workdir_operation_transport_error)? } }; Ok(Json(result)) + }.await; + result.map_err(|error| InternalWorkdirOperationError::Contextual { + error: Box::new(error), + context, + }) } fn current_worker_command_session( @@ -18335,26 +18434,27 @@ fn current_worker_command_session( worker: &RuntimeWorkerRef, target_workdir: &str, external_handle: &CommandHandle, + context: &mut server_api::WorkdirOperationLogContext, ) -> std::result::Result<(WorkdirSessionHandle, CommandHandle), InternalWorkdirOperationError> { - let _link = validated_current_worker_attachment(api, worker, target_workdir)?; + context.stage = "attachment_validation"; + let link = validated_current_worker_attachment(api, worker, target_workdir)?; + context.workdir_id_hash = Some(crate::worker_source::diagnostic_id_hash(&link.workdir_id)); + context.stage = "command_lookup"; let command = api .workdir_sessions .lock() .expect("Workdir session registry lock poisoned") .command(worker, target_workdir, external_handle) .ok_or_else(|| { - InternalWorkdirOperationError::Provider(current_worker_workdir_operation_error( - worker, + InternalWorkdirOperationError::Provider(workdir_operation_transport_error( workdir::WorkdirError::UnknownCommand(external_handle.0.clone()), )) })?; + context.stage = "provider_dispatch"; Ok((command.source, command.provider_handle)) } -fn current_worker_workdir_operation_error( - _worker: &RuntimeWorkerRef, - error: workdir::WorkdirError, -) -> WorkdirTransportError { +fn workdir_operation_transport_error(error: workdir::WorkdirError) -> WorkdirTransportError { WorkdirTransportError::from_workdir_error(&error) } @@ -23453,6 +23553,7 @@ async fn serve_external_workdir_provider( Some(ExternalProviderCommand::Operation { operation_id, operation, response }) => { if revoking { let _ = response.send(Err(WorkdirTransportError { + denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::Unavailable, message: "External Workdir provider shutdown is in progress".to_string(), })); @@ -23460,6 +23561,7 @@ async fn serve_external_workdir_provider( } if pending.len() >= 16 { let _ = response.send(Err(WorkdirTransportError { + denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::Unavailable, message: "External Workdir provider is at its in-flight operation limit".to_string(), })); @@ -23470,6 +23572,7 @@ async fn serve_external_workdir_provider( Ok(operation) => operation, Err(message) => { let _ = response.send(Err(WorkdirTransportError { + denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::InvalidRequest, message, })); @@ -23526,6 +23629,7 @@ async fn serve_external_workdir_provider( Err(error.into_transport_error()) } ExternalWorkdirOperationOutcome::Cancelled => Err(WorkdirTransportError { + denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::Unavailable, message: "External Workdir operation was cancelled".to_string(), }), @@ -24007,6 +24111,9 @@ fn workdir_runtime_failure_code(error: &RuntimeRegistryError) -> String { RuntimeRegistryError::UnknownRuntime(_) | RuntimeRegistryError::UnknownHost(_) => { "runtime_unavailable".to_string() } + RuntimeRegistryError::WorkdirSessionOpenFailed { .. } => { + "workdir_session_open_failed".to_string() + } RuntimeRegistryError::RuntimeOperationFailed { code, .. } => code.clone(), RuntimeRegistryError::InvalidIdentifier { .. } => "invalid_runtime".to_string(), RuntimeRegistryError::UnknownWorker { .. } => "runtime_workdir_lookup_failed".to_string(), @@ -24518,6 +24625,7 @@ fn runtime_reports_workdir_not_found(result: &crate::hosts::RuntimeWorkingDirect fn classify_workdir_provider_error(error: &RuntimeRegistryError) -> (&'static str, bool) { match error { + RuntimeRegistryError::WorkdirSessionOpenFailed { .. } => ("provider_unavailable", true), RuntimeRegistryError::UnknownRuntime(_) => ("runtime_unavailable", true), RuntimeRegistryError::RuntimeOperationFailed { code, .. } => { let category = @@ -32556,6 +32664,8 @@ struct ApiErrorLog { kind: String, message: String, diagnostics: Vec, + workdir_log: Option, + denial_reason: Option, } impl ApiErrorLog { @@ -32568,9 +32678,11 @@ impl ApiErrorLog { kind: format!("workdir_session_operation_{}", code.as_str()), message: sanitize_backend_error(&error.message), diagnostics: Vec::new(), + workdir_log: error.workdir_log.clone(), + denial_reason: error.denial_reason, }; } - Self::from_repository_error(&server_api::RepositoryApiError::new( + let mut log = Self::from_repository_error(&server_api::RepositoryApiError::new( status.as_u16(), error.error.as_deref().unwrap_or_else(|| { status @@ -32579,11 +32691,25 @@ impl ApiErrorLog { }), &error.message, error.diagnostics.clone().unwrap_or_default(), - )) + )); + log.workdir_log = error.workdir_log.clone(); + log.denial_reason = error.denial_reason; + if log.workdir_log.is_some() { + // Existing public API details are unchanged; do not copy request-derived + // alias/handle text or session-open errors into operational diagnostics. + log.kind = format!("workdir_session_operation_api_{}", status.as_u16()); + // This can also fail after a command was dispatched. Do not claim + // a pre-dispatch refusal or absence of effects in the safe message. + log.message = "Workdir operation failed".to_string(); + log.diagnostics.clear(); + } + log } fn from_repository_error(error: &server_api::RepositoryApiError) -> Self { Self { + workdir_log: None, + denial_reason: None, kind: error .diagnostics .first() @@ -32616,6 +32742,11 @@ impl From for ApiError { impl From for ApiError { fn from(error: Error) -> Self { let diagnostics = match &error { + Error::WorkdirSessionOpenFailed { transport, .. } => vec![RuntimeDiagnostic { + code: "workdir_session_open_failed".to_string(), + severity: HostDiagnosticSeverity::Error, + message: transport.message.clone(), + }], Error::RuntimeOperationFailed { code, message, .. } => vec![RuntimeDiagnostic { code: code.clone(), severity: HostDiagnosticSeverity::Error, @@ -32834,13 +32965,18 @@ fn api_error_status(error: &Error) -> StatusCode { StatusCode::SERVICE_UNAVAILABLE } Error::WorkspaceSigningIdentity { .. } => StatusCode::SERVICE_UNAVAILABLE, - Error::RuntimeOperationFailed { .. } => StatusCode::BAD_GATEWAY, + Error::WorkdirSessionOpenFailed { .. } | Error::RuntimeOperationFailed { .. } => { + StatusCode::BAD_GATEWAY + } _ => StatusCode::INTERNAL_SERVER_ERROR, } } fn api_error_response_message(error: &Error) -> String { match error { + Error::WorkdirSessionOpenFailed { transport, .. } => { + format!("workdir_session_open_failed: {}", transport.message) + } Error::RuntimeOperationFailed { code, message, .. } => { format!("{code}: {}", sanitize_backend_error(message)) } @@ -32853,6 +32989,8 @@ impl IntoResponse for ApiError { let status = api_error_status(&self.error); let response_message = api_error_response_message(&self.error); let log = ApiErrorLog { + workdir_log: None, + denial_reason: None, kind: self .diagnostics .first() @@ -32890,6 +33028,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; @@ -33155,6 +33294,7 @@ mod tests { assert!(matches!(operation, WorkdirSessionOperation::Read(_))); response .send(Err(WorkdirTransportError { + denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::NotFound, message: "logical file was not found".to_string(), })) @@ -35153,6 +35293,13 @@ mod tests { #[derive(Clone, Default)] struct WorkdirlessFixtureRuntime { + workdir_session_factory: Option< + Arc< + dyn Fn(&str) -> std::result::Result + + Send + + Sync, + >, + >, input_requests: Arc>>, workers: Arc>>, spawn_requests: Arc>>, @@ -35291,6 +35438,27 @@ mod tests { } impl crate::hosts::WorkspaceWorkerRuntime for WorkdirlessFixtureRuntime { + fn open_workdir_session<'a>( + &'a self, + working_directory_id: &'a str, + _owner_worker_id: Option<&'a str>, + ) -> std::pin::Pin< + Box< + dyn std::future::Future< + Output = std::result::Result, + > + Send + + 'a, + >, + > { + Box::pin(async move { + self.workdir_session_factory.as_ref().ok_or_else(|| { + workdir::WorkdirError::Unavailable( + "Runtime does not expose Workdir operation sessions".to_string(), + ) + })?(working_directory_id) + }) + } + fn send_input( &self, worker_id: &str, @@ -36446,6 +36614,8 @@ mod tests { .parse::() .expect("valid URI"); let error = ApiErrorLog { + workdir_log: None, + denial_reason: None, kind: "ticket_backend_error".to_string(), message: "sqlite error: FOREIGN KEY constraint failed".to_string(), diagnostics: vec![RuntimeDiagnostic { @@ -36587,6 +36757,7 @@ mod tests { ), ] { let response = InternalWorkdirOperationError::Provider(WorkdirTransportError { + denial_reason: None, code, message: "safe provider message".to_string(), }) @@ -36606,14 +36777,11 @@ mod tests { #[tokio::test] async fn local_and_remote_validation_errors_share_workspace_classification() { - let worker = RuntimeWorkerRef::new("runtime", "worker"); - let local = current_worker_workdir_operation_error( - &worker, - workdir::WorkdirError::InvalidGlob("[".to_string()), - ); - let remote = current_worker_workdir_operation_error( - &worker, + let local = + workdir_operation_transport_error(workdir::WorkdirError::InvalidGlob("[".to_string())); + let remote = workdir_operation_transport_error( WorkdirTransportError { + denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::InvalidRequest, message: "Workdir operation request is invalid".to_string(), } @@ -36639,10 +36807,7 @@ mod tests { path: PathBuf::from("/host/private/worktree/secret.txt"), source: std::io::Error::other("token=secret"), }; - let public = current_worker_workdir_operation_error( - &RuntimeWorkerRef::new("runtime", "worker"), - error, - ); + let public = workdir_operation_transport_error(error); let response = InternalWorkdirOperationError::Provider(public).into_response(); let body = to_bytes(response.into_body(), usize::MAX).await.unwrap(); let text = String::from_utf8(body.to_vec()).unwrap(); @@ -36651,10 +36816,7 @@ mod tests { let decoded: WorkdirTransportError = serde_json::from_str(&text).unwrap(); assert_eq!(decoded.code, workdir::http::WorkdirTransportErrorCode::Io); - let remote = current_worker_workdir_operation_error( - &RuntimeWorkerRef::new("runtime", "worker"), - workdir::WorkdirError::OperationFailed, - ); + let remote = workdir_operation_transport_error(workdir::WorkdirError::OperationFailed); let response = InternalWorkdirOperationError::Provider(remote).into_response(); assert_eq!(response.status(), StatusCode::INTERNAL_SERVER_ERROR); } @@ -38254,6 +38416,7 @@ mod tests { .uri(format!("/api/w/{TEST_WORKSPACE_ID}/worker-control/workers")) // Unit fixture stands below proof middleware, so inject its verified subject. .extension(crate::worker_source::VerifiedRuntimeRequestSource { + token_id_hash: None, runtime_id: fixture.controller.runtime_id.clone(), worker_id: Some(fixture.controller.worker_id.clone()), }) @@ -38577,6 +38740,7 @@ mod tests { .method("POST") .uri(format!("/api/w/{TEST_WORKSPACE_ID}/worker-control/workers")) .extension(crate::worker_source::VerifiedRuntimeRequestSource { + token_id_hash: None, runtime_id: controller.runtime_id.clone(), worker_id: Some(controller.worker_id.clone()), }) diff --git a/crates/workspace-server/src/server/tests/api_logging_tests.rs b/crates/workspace-server/src/server/tests/api_logging_tests.rs index 2d70ea91b..803f8392a 100644 --- a/crates/workspace-server/src/server/tests/api_logging_tests.rs +++ b/crates/workspace-server/src/server/tests/api_logging_tests.rs @@ -17,6 +17,12 @@ impl Write for CapturedApiLog { } async fn request_with_logs(app: Router, request: Request) -> (StatusCode, Value, Vec) { + let token_hash = request + .headers() + .get(worker_runtime::auth::RUNTIME_REQUEST_SOURCE_PROOF_HEADER) + .and_then(|value| value.to_str().ok()) + .and_then(|proof| worker_runtime::auth::decode_runtime_request_source_claims(proof).ok()) + .map(|claims| crate::worker_source::diagnostic_id_hash(&claims.jti)); let logs = CapturedApiLog::default(); let writer = logs.clone(); let subscriber = tracing_subscriber::fmt() @@ -32,18 +38,41 @@ async fn request_with_logs(app: Router, request: Request) -> (StatusCode, .await .unwrap(); let status = response.status(); - assert_eq!(response.headers()[CONTENT_TYPE], "application/json"); + let content_type = response.headers()[CONTENT_TYPE].clone(); let body = to_bytes(response.into_body(), usize::MAX).await.unwrap(); + assert_eq!( + content_type, + "application/json", + "status={status}, body={}", + String::from_utf8_lossy(&body) + ); let body: Value = serde_json::from_slice(&body).unwrap(); let text = String::from_utf8(logs.0.lock().unwrap().clone()).unwrap(); - for secret in ["query-secret", "bearer-secret", "body-secret"] { + for secret in [ + "query-secret", + "bearer-secret", + "body-secret", + "/private/provider/path", + "/host/private", + "proof-secret", + "credential-secret", + "key-secret", + ] { assert!(!text.contains(secret), "log leaked request data: {text}"); } - let events = text + let events: Vec = text .lines() .map(|line| serde_json::from_str::(line).unwrap()) .filter(|event| event["event"] == "api_error") .collect(); + for event in &events { + if event.get("operation").is_some() && token_hash.is_some() { + assert_eq!( + event["request_token_id_hash"], + serde_json::to_value(&token_hash).unwrap() + ); + } + } (status, body, events) } @@ -278,17 +307,50 @@ async fn generated_workdir_provider_errors_preserve_response_and_log_details() { ) }; let app = build_router(fixture.api.clone()); - for (code, status, message) in [ - (Code::Denied, 403, "Workdir operation was denied"), - (Code::OutOfScope, 403, "Workdir path is out of scope"), + for (code, status, message, reason) in [ + (Code::Denied, 403, "Workdir operation was denied", None), + ( + Code::Denied, + 403, + "Workdir operation was denied", + Some(workdir::WorkdirDenialReason::OsPermissionDenied), + ), + ( + Code::Denied, + 403, + "Workdir operation was denied", + Some(workdir::WorkdirDenialReason::ReadOnlySession), + ), + ( + Code::OutOfScope, + 403, + "Workdir path is out of scope", + Some(workdir::WorkdirDenialReason::PathOutOfScope), + ), ( Code::SymlinkOutOfScope, 403, "Workdir symlink target is out of scope", + Some(workdir::WorkdirDenialReason::SymlinkTargetOutOfScope), + ), + ( + Code::ReadOnly, + 403, + "Workdir path is read-only", + Some(workdir::WorkdirDenialReason::PathReadOnly), + ), + ( + Code::UnknownCommand, + 404, + "Workdir command was not found", + None, + ), + ( + Code::Unavailable, + 503, + "Workdir session is unavailable", + None, ), - (Code::ReadOnly, 403, "Workdir path is read-only"), - (Code::UnknownCommand, 404, "Workdir command was not found"), - (Code::Unavailable, 503, "Workdir session is unavailable"), ] { let provider = async { let Some(ExternalProviderCommand::Operation { @@ -305,6 +367,7 @@ async fn generated_workdir_provider_errors_preserve_response_and_log_details() { )); response .send(Err(WorkdirTransportError { + denial_reason: reason, code, message: "body-secret /private/provider/path".to_string(), })) @@ -324,6 +387,22 @@ async fn generated_workdir_provider_errors_preserve_response_and_log_details() { assert_eq!(events[0]["status"], status); assert_eq!(events[0]["method"], "POST"); assert_eq!(events[0]["path"], path); + assert_eq!(events[0]["operation"], "command_status"); + assert_eq!(events[0]["workdir_stage"], "provider_dispatch"); + assert_workdir_log_identity( + &events[0], + &fixture, + "checkout", + Some(&fixture.main_workdir_id), + ); + assert_eq!( + events[0]["denial_reason"], + serde_json::to_value(reason).unwrap() + ); + assert_eq!( + events[0]["request_token_id_hash"].as_str().unwrap().len(), + 64 + ); } let provider = async { let Some(ExternalProviderCommand::Operation { response, .. }) = commands.recv().await @@ -374,8 +453,358 @@ async fn generated_workdir_api_errors_preserve_response_and_log_details() { assert_eq!(status, StatusCode::FORBIDDEN); assert_eq!(body, serde_json::to_value(expected).unwrap()); assert_eq!(events.len(), 1, "{events:?}"); - assert_eq!(events[0]["kind"], "forbidden"); - assert_eq!(events[0]["message"], body["message"]); + assert_eq!(events[0]["kind"], "workdir_session_operation_api_403"); + assert_eq!(events[0]["message"], "Workdir operation failed"); + assert_eq!(events[0]["operation"], "command_status"); + assert_eq!(events[0]["workdir_stage"], "worker_identity"); + assert_eq!(events[0]["runtime_id_hash"], Value::Null); + assert_eq!(events[0]["worker_id_hash"], Value::Null); + assert_eq!(events[0]["workdir_id_hash"], Value::Null); assert_eq!(events[0]["diagnostics"], "[]"); assert_eq!(events[0]["path"], path); } + +fn assert_workdir_log_identity( + event: &Value, + fixture: &ManualCoderAssignmentFixture, + alias: &str, + workdir_id: Option<&str>, +) { + let hash = crate::worker_source::diagnostic_id_hash; + assert_eq!(event["workspace_id_hash"], hash(TEST_WORKSPACE_ID)); + assert_eq!(event["runtime_id_hash"], hash(&fixture.worker.runtime_id)); + assert_eq!(event["worker_id_hash"], hash(&fixture.worker.worker_id)); + assert_eq!(event["attachment_alias_hash"], hash(alias)); + assert_eq!( + event["workdir_id_hash"], + serde_json::to_value(workdir_id.map(hash)).unwrap() + ); +} + +#[tokio::test] +async fn self_workdir_provider_refusals_log_operation_identity_without_request_text() { + use workdir::{WorkdirDenialReason as Reason, WorkdirSessionCapabilities as Caps}; + let mut fixture = manual_worker_assignment_fixture().await; + let root = tempfile::tempdir().unwrap(); + let provider_root = root.path().to_path_buf(); + // Fresh sessions exercise the real opening/broker/dispatch path on each request. + fixture.runtime.workdir_session_factory = Some(Arc::new(move |id| { + Ok(Arc::new(workdir::LocalWorkdirSession::materialized_bound( + workdir::Workdir::new(id), + provider_root.clone(), + provider_root.clone(), + manifest::SharedScope::new(manifest::Scope::writable(&provider_root).unwrap()), + Caps::READ_ONLY, + )) as WorkdirSessionHandle) + })); + fixture + .api + .runtime + .register_or_replace(fixture.runtime.clone()); + let identity = + worker_runtime::auth::RuntimeIdentityMaterial::generate(&fixture.worker.runtime_id) + .unwrap(); + configure_runtime_request_auth(&mut fixture.api, &identity, &fixture.worker.runtime_id); + let path = format!("/api/w/{TEST_WORKSPACE_ID}/workers/self/workdir-session/operations"); + let app = build_router(fixture.api.clone()); + let secret = format!( + "body-secret credential-secret key-secret proof-secret{}", + "z".repeat(20_000) + ); + let cases = [ + ( + json!({"operation": "read", "request": {"path": "/host/private/secret.txt", "offset": 0, "limit": 10, "max_bytes": 1024}}), + "read", + "out_of_scope", + Reason::PathOutOfScope, + ), + ( + json!({"operation": "write", "request": {"path": "secret.txt", "content": secret.as_bytes()}}), + "write", + "denied", + Reason::ScopedCapabilityDenied, + ), + ( + json!({"operation": "command_start", "request": {"command": secret, "timeout_secs": 1, "output_limit": 1024, "cwd": "", "tool_call_id": secret}}), + "command_start", + "denied", + Reason::ScopedCapabilityDenied, + ), + ( + // Scoped forwarding validates this READ_ONLY parent's capabilities + // before provider resolution. Local now supports scope resolution; + // denying write rules is not a missing-resolver failure. + json!({"operation": "authorize_scope", "request": {"path": "secret.txt", "permission": "write", "rules": [{"target": "", "permission": "write", "recursive": true}]}}), + "authorize_scope", + "denied", + Reason::ParentCapabilityDenied, + ), + ]; + for (operation, label, code, reason) in cases { + let _: WorkdirSessionOperation = + serde_json::from_value(operation.clone()).expect("valid operation fixture"); + let request = runtime_source_request( + &identity, + Some(&fixture.worker.worker_id), + "POST", + &format!("{path}?token=query-secret"), + serde_json::to_vec(&json!({"target_workdir": "checkout", "operation": operation})) + .unwrap(), + ); + let (status, body, events) = request_with_logs(app.clone(), request).await; + assert_eq!(status, StatusCode::FORBIDDEN, "{body}"); + assert_eq!(body["code"], code, "{body}"); + assert!( + body.get("denial_reason").is_none(), + "internal diagnostics leaked: {body}" + ); + assert_eq!(events.len(), 1, "{events:?}"); + assert_eq!(events[0]["operation"], label); + assert_eq!(events[0]["workdir_stage"], "provider_dispatch"); + assert_eq!( + events[0]["denial_reason"], + serde_json::to_value(reason).unwrap() + ); + assert_workdir_log_identity( + &events[0], + &fixture, + "checkout", + Some(&fixture.main_workdir_id), + ); + assert_eq!( + events[0]["request_token_id_hash"].as_str().unwrap().len(), + 64 + ); + assert!( + events[0].to_string().len() < 2000, + "unbounded log: {events:?}" + ); + } + // Control: the same READ_ONLY provider can authorize read rules through + // scoped forwarding and native resolution. Keep a capability refusal + // distinct from an unavailable scope resolver; successful authorization + // must not emit an api_error or mutate the filesystem. + let read_scope = runtime_source_request( + &identity, + Some(&fixture.worker.worker_id), + "POST", + &format!("{path}?token=query-secret"), + serde_json::to_vec(&json!({ + "target_workdir": "checkout", + "operation": {"operation": "authorize_scope", "request": { + "path": "", "permission": "read", + "rules": [{"target": "", "permission": "read", "recursive": true}], + }}, + })) + .unwrap(), + ); + let (status, body, events) = request_with_logs(app, read_scope).await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!( + body, + serde_json::to_value(WorkdirSessionOperationResult::AuthorizeScope).unwrap(), + ); + assert!( + events.is_empty(), + "successful scope authorization logged as failure: {events:?}" + ); + assert!( + !root.path().join("secret.txt").exists(), + "refusal changed filesystem" + ); +} + +#[tokio::test] +async fn self_workdir_attachment_rejection_hashes_oversized_alias_and_omits_unknown_workdir() { + let mut fixture = manual_worker_assignment_fixture().await; + let identity = + worker_runtime::auth::RuntimeIdentityMaterial::generate(&fixture.worker.runtime_id) + .unwrap(); + configure_runtime_request_auth(&mut fixture.api, &identity, &fixture.worker.runtime_id); + let path = format!("/api/w/{TEST_WORKSPACE_ID}/workers/self/workdir-session/operations"); + let alias = format!( + "body-secret /host/private credential-secret key-secret proof-secret{}", + "あ".repeat(10_000) + ); + let request = runtime_source_request( + &identity, + Some(&fixture.worker.worker_id), + "POST", + &path, + serde_json::to_vec(&json!({ + "target_workdir": alias, + "operation": {"operation": "command_status", "request": "body-secret"} + })) + .unwrap(), + ); + let (status, body, events) = + request_with_logs(build_router(fixture.api.clone()), request).await; + assert_eq!(status, StatusCode::CONFLICT, "{body}"); + assert_eq!(events.len(), 1, "{events:?}"); + assert_eq!(events[0]["workdir_stage"], "attachment_validation"); + assert_eq!(events[0]["operation"], "command_status"); + assert_workdir_log_identity(&events[0], &fixture, &alias, None); + assert_eq!(events[0]["denial_reason"], Value::Null); + assert!( + events[0].to_string().len() < 2000, + "unbounded log: {events:?}" + ); +} + +#[tokio::test] +async fn self_workdir_session_open_refusals_keep_reason_before_registry_conversion() { + use workdir::WorkdirDenialReason as Reason; + for (reason, os_error) in [ + (Reason::ReadOnlySession, false), + (Reason::OsPermissionDenied, true), + ] { + let mut fixture = manual_worker_assignment_fixture().await; + fixture.runtime.workdir_session_factory = Some(Arc::new(move |_| { + Err(if os_error { + workdir::WorkdirError::Io { + path: PathBuf::from("/host/private/key-secret"), + source: std::io::Error::from_raw_os_error(13), + } + } else { + workdir::WorkdirError::denied(reason, "body-secret credential-secret /host/private") + }) + })); + fixture + .api + .runtime + .register_or_replace(fixture.runtime.clone()); + let identity = + worker_runtime::auth::RuntimeIdentityMaterial::generate(&fixture.worker.runtime_id) + .unwrap(); + configure_runtime_request_auth(&mut fixture.api, &identity, &fixture.worker.runtime_id); + let path = format!("/api/w/{TEST_WORKSPACE_ID}/workers/self/workdir-session/operations"); + let request = runtime_source_request(&identity, Some(&fixture.worker.worker_id), "POST", &path, serde_json::to_vec(&json!({ + "target_workdir": "checkout", + "operation": {"operation": "read", "request": {"path": "README.md", "offset": 0, "limit": 10, "max_bytes": 1024}} + })).unwrap()); + let (status, body, events) = + request_with_logs(build_router(fixture.api.clone()), request).await; + // Opening retained its existing Registry HTTP meaning, not an operation's 403. + assert_eq!(status, StatusCode::BAD_GATEWAY, "{body}"); + assert_eq!( + body["message"], + "workdir_session_open_failed: Workdir operation was denied" + ); + assert!(body.get("denial_reason").is_none(), "{body}"); + assert_eq!(events.len(), 1, "{events:?}"); + assert_eq!(events[0]["workdir_stage"], "session_open"); + assert_eq!(events[0]["operation"], "read"); + assert_eq!( + events[0]["denial_reason"], + serde_json::to_value(reason).unwrap() + ); + assert_workdir_log_identity( + &events[0], + &fixture, + "checkout", + Some(&fixture.main_workdir_id), + ); + assert!(!body.to_string().contains("body-secret")); + assert!(!body.to_string().contains("/host/private")); + } +} + +#[tokio::test] +async fn self_workdir_post_start_registration_failure_does_not_claim_pre_dispatch_refusal() { + let mut fixture = manual_worker_assignment_fixture().await; + let identity = + worker_runtime::auth::RuntimeIdentityMaterial::generate(&fixture.worker.runtime_id) + .unwrap(); + configure_runtime_request_auth(&mut fixture.api, &identity, &fixture.worker.runtime_id); + let (sender, mut commands) = tokio::sync::mpsc::channel(1); + let connection = Arc::new(ExternalProviderConnection { + grant_id: "logging-grant".to_string(), + workdir_id: fixture.main_workdir_id.clone(), + provider_instance_id: "logging-provider".to_string(), + generation: 1, + expires_at: None, + capabilities: workdir::WorkdirSessionCapabilities::ALL, + admission: Arc::new(tokio::sync::Semaphore::new(1)), + shutdown_confirmed: Arc::new(std::sync::atomic::AtomicBool::new(false)), + sender, + }); + fixture.runtime.workdir_session_factory = Some(Arc::new(move |_| { + Ok(Arc::new(ExternalProviderWorkdirSession::new( + connection.clone(), + None, + )) as WorkdirSessionHandle) + })); + fixture + .api + .runtime + .register_or_replace(fixture.runtime.clone()); + let path = format!("/api/w/{TEST_WORKSPACE_ID}/workers/self/workdir-session/operations"); + let request = runtime_source_request( + &identity, + Some(&fixture.worker.worker_id), + "POST", + &path, + serde_json::to_vec(&json!({ + "target_workdir": "checkout", + "operation": {"operation": "command_start", "request": { + "command": "body-secret", "timeout_secs": 1, "output_limit": 1024, + "cwd": "", "spill_dir": null, "tool_call_id": "body-secret" + }} + })) + .unwrap(), + ); + // Observe command dispatch, then lose only the fixture registry entry before + // completing start. No OS process is launched, no sleep or retry is needed. + let provider = async { + let Some(ExternalProviderCommand::Operation { + operation, + response, + .. + }) = commands.recv().await + else { + panic!("expected provider start operation"); + }; + assert!(matches!( + operation, + WorkdirSessionOperation::CommandStart(_) + )); + fixture + .api + .workdir_sessions + .lock() + .unwrap() + .remove_attachment(&fixture.worker) + .unwrap(); + response + .send(Ok(WorkdirSessionOperationResult::CommandStart( + CommandHandle("fixture-provider-handle".into()), + ))) + .unwrap(); + }; + let ((status, body, events), ()) = tokio::join!( + request_with_logs(build_router(fixture.api.clone()), request), + provider + ); + assert_eq!(status, StatusCode::BAD_GATEWAY, "{body}"); + assert!( + body["message"] + .as_str() + .unwrap() + .starts_with("workdir_session_registration_failed:") + ); + assert_eq!(events.len(), 1, "{events:?}"); + assert_eq!(events[0]["operation"], "command_start"); + assert_eq!(events[0]["workdir_stage"], "command_registration"); + assert_eq!(events[0]["message"], "Workdir operation failed"); + assert_eq!(events[0]["denial_reason"], Value::Null); + assert_workdir_log_identity( + &events[0], + &fixture, + "checkout", + Some(&fixture.main_workdir_id), + ); + assert!( + commands.try_recv().is_err(), + "post-dispatch failure was retried" + ); +} diff --git a/crates/workspace-server/src/server/tests/drive_api_tests.rs b/crates/workspace-server/src/server/tests/drive_api_tests.rs index b6ec7ac69..7849d5760 100644 --- a/crates/workspace-server/src/server/tests/drive_api_tests.rs +++ b/crates/workspace-server/src/server/tests/drive_api_tests.rs @@ -718,6 +718,7 @@ async fn drive_verified_upload_revoked_during_reception_cannot_commit_or_replay( let app = generated_workspace_contract_router(ServerApiContractService::Workspace(api.clone())) .layer(Extension( crate::worker_source::VerifiedRuntimeRequestSource { + token_id_hash: None, runtime_id: workers[0].runtime_id.clone(), worker_id: Some(workers[0].worker_id.clone()), }, 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/crates/workspace-server/src/worker_source/runtime_request.rs b/crates/workspace-server/src/worker_source/runtime_request.rs index 36e4d6076..5e8b4a619 100644 --- a/crates/workspace-server/src/worker_source/runtime_request.rs +++ b/crates/workspace-server/src/worker_source/runtime_request.rs @@ -12,6 +12,8 @@ use crate::store::ControlPlaneStore; pub struct VerifiedRuntimeRequestSource { pub runtime_id: String, pub worker_id: Option, + /// Existing proof JTI correlation, retained only as a fixed-length diagnostic fingerprint. + pub token_id_hash: Option, } /// Safe diagnostics only: never stores proof, raw claims, keys or store errors. @@ -268,6 +270,7 @@ pub(crate) async fn verify_runtime_request_source_proof_with_clock( })?; } Ok(VerifiedRuntimeRequestSource { + token_id_hash: Some(diagnostic_id_hash(&claims.jti)), runtime_id: claims.iss, worker_id: claims.worker_id, }) diff --git a/crates/yoi/src/workdir_share.rs b/crates/yoi/src/workdir_share.rs index b97bf96ab..6dc60bc1c 100644 --- a/crates/yoi/src/workdir_share.rs +++ b/crates/yoi/src/workdir_share.rs @@ -836,6 +836,7 @@ where .unwrap_or_else(|_| ExternalWorkdirOperationOutcome::Failed { error: ExternalWorkdirOperationError::from_transport_error( WorkdirTransportError { + denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::Internal, message: "External Workdir operation executor failed".to_string(), }, @@ -953,6 +954,7 @@ async fn execute_operation( Ok(result) => ExternalWorkdirOperationOutcome::Completed { result }, Err(message) => ExternalWorkdirOperationOutcome::Failed { error: ExternalWorkdirOperationError::from_transport_error(WorkdirTransportError { + denial_reason: None, code: workdir::http::WorkdirTransportErrorCode::InvalidRequest, message, }), diff --git a/docs/development/workdir-refusal-diagnostics.md b/docs/development/workdir-refusal-diagnostics.md new file mode 100644 index 000000000..1fe2d7575 --- /dev/null +++ b/docs/development/workdir-refusal-diagnostics.md @@ -0,0 +1,159 @@ +# Workdir refusal diagnostics + +This change improves the internal `api_error` record for +`POST /api/w/{workspace_id}/workers/self/workdir-session/operations`. +It does **not** identify or resolve the reported 2026-10-09 403: the running +binary revision and actual request are still unverified. Fixture success is +not evidence about that incident. No deployment, live asset cleanup, privilege +change, retry policy, or WIP dependency/version change is included (WIP remains +0.2.0). + +## Diagnostic path and public contract + +Original refusing branches attach a closed `WorkdirDenialReason`, rather than +recovering meaning from an error string. `WorkdirError::Denied` carries local +message plus optional typed reason. `DenialContext` retains an existing error +classification while enriching its diagnostics (notably checkout's historical +permission-to-out-of-scope mapping). Typed provider markers survive the +`FsAccessPolicy`/`io::Error` → `FsError` → `WorkdirError` boundaries. + +`WorkdirTransportError.denial_reason` and +`ExternalWorkdirOperationError.denial_reason` carry only an optional snake-case +enum. Remote HTTP and External errors preserve that reason when converted back +to `WorkdirError` and then forwarded again. Provider text is not the reason. +The External codec continues dropping provider-authored message text. A missing +reason stays unknown for generic `denied`; existing code-level path/symlink/ +read-only classifications can still identify their original category. Unknown +or oversized reason strings fail deserialization rather than becoming log data. + +Runtime registry session-open failures now retain a safe transport error **before** +the former raw error-to-string conversion. Their existing Workspace HTTP meaning +is still 502 (`workdir_session_open_failed`); operation-level permission failures +remain 403. Session-open messages are normalized to the same safe provider +message vocabulary instead of forwarding raw local errors. No authorization, +scope, capability, session ownership, command ownership, or retry conditions +are relaxed. + +The self handler attaches internal context to its error. Both the handwritten +response adapter and the generated route preserve it through the typed error +response extension to `ApiErrorLog` and the logging middleware. Server API +`workdir_log` and `denial_reason` are `serde(skip)` and omitted from schemas: +the external error envelope remains `{code, message}` for provider failures or +the existing Repository-style envelope for API/domain failures. Workspace +responses do not expose the new internal diagnostics. Authorized Runtime/provider +transport carries the enum to its consumer; it contains no free text or paths. + +## Fields in `yoi::api` / `api_error` + +- `operation`: fixed label from the typed operation (read, read_bytes, write, + edit, command_start/status/output/cancel, authorize_scope, scope_rules_overlap, + checkout_search/observe/execute, stat/list/glob/grep). No operation Debug dump. +- `workdir_stage`: workspace_scope, worker_identity, attachment_validation, + session_open, command_lookup, provider_dispatch, or command_registration. API + failures identify the encountered stage without copying raw alias/handle/error + text into the log. Registration failures can follow successful command start; + they are not evidence of a pre-dispatch refusal or absence of effects. +- `denial_reason`: optional closed reason reported by the provider/error chain. + It describes the encountered refusing branch, not a permission grant and not + an independent attestation of an external provider's explanation. +- `workspace_id_hash`, `runtime_id_hash`, `worker_id_hash`, `workdir_id_hash`, + `attachment_alias_hash`: 64-character lowercase SHA-256 fingerprints via + `worker_source::diagnostic_id_hash`, using the existing + `yoi-runtime-proof-diagnostic-id-v1\0` domain prefix. Hash a known catalog ID + or attachment alias to locate its logs. Workspace and alias represent route/ + request selection; Runtime/Worker are added only after current identity checks, + and Workdir only after a validated attachment is found. Missing information is + omitted, never inferred from a failed lookup or command handle. +- `request_token_id_hash`: fingerprint of the **existing verified Runtime proof + JTI**, reusing the T-729 hash algorithm. It is not the proof, raw token ID, + caller-supplied trace header, or a newly invented cross-service trace ID. + In-process/test callers without that proof correlation omit it. Authentication + still consumes the JTI using the existing policy; logging does not retry it. +- Existing `method`, query-free `path`, `status`, `kind`, `message`, `diagnostics` + remain. Workdir-context method/path are bounded to 32/512 UTF-8 bytes with + controls removed. Provider operation messages remain normalized safe text. + Contextual API failures use a fixed internal message and status-based kind, + with no raw domain diagnostics copied into this log. Empty `diagnostics` does + **not** mean the optional typed reason/context is absent. + +Fingerprints are bounded correlation, not encryption: low-entropy identifiers +can be guessed. Do not put credentials in IDs. Neither hashes nor stage labels +prove the reported incident's cause. The request JTI identifies the incoming +request; External operation IDs/generation/audit records remain their existing +provider-side mechanisms, not a cross-service trace automatically joined here. + +## Refusal vocabulary and limits + +Reasons distinguish scope resolution/comparison unavailable, attenuated +capability refusal, logical/parent/attachment/delegation scope exceeded, +empty scope, invalid scoped path, writable-scope requirements for commands, +child write-lease conflicts, unreadable cwd, read-only session, external root +symlink/change/move, external scope symlink, checkout output-root escape, +provider root escape, checkout search/enumeration refusal, provider symlink/ +checked-target refusal, path out of scope, symlink target out of scope, and +read-only path. Original local messages remain local; arbitrary message text +is never parsed into a reason. + +- `os_permission_denied`: PermissionDenied with an actual OS error code, without + a recognized policy marker. It does not determine whether modes, ACLs, + sandboxing, mount policy, or another OS mechanism caused the refusal. +- `unclassified_permission_denied`: custom PermissionDenied with neither typed + policy origin nor raw OS code. Do not label an arbitrary + `Error::new(PermissionDenied, "...")` as an OS refusal by guessing from text. +- External/remote providers that do not send a reason cannot supply distinctions + erased in their own binary. A registered provider's typed explanation is + reported data, not proof of what happened inside that provider. +- Authentication/JSON ingress failures before a typed Workdir operation and + target can be obtained continue using their existing rejection diagnostics; + no operation/body is guessed or decoded from rejected proof data. + +## Regression proof boundaries + +- Workdir provider fixtures exercise actual local authorization, broker scope/ + capability/read-only/delegation/lease refusals and confinement. Permission + fixtures distinguish typed policy origins, OS-coded PermissionDenied, and + origin-unknown custom errors, including checkout's existing public mapping. +- Loopback HTTP fake exercises remote open/operation decoding and typed reason + forwarding, including missing-reason providers and hostile provider text. + External JSON/frame codecs exercise the bounded error envelope and reject + arbitrary reason strings; the Server's External adapter drives the real self + generated route through command-session dispatch and response/log conversion. +- Server router tests use signed fixture requests and capture actual tracing + events. They cover read/write/command_start/authorize_scope, session-open + failures, External command status failures, no-identity rejection, oversized + UTF-8 alias, oversized command/file/tool-call inputs, query/header/proof/key/ + credential markers, and raw provider/host paths. Identity hashes and the + existing verified JTI fingerprint are checked against fixture identities. +- The Server API boundary proves internal fields are absent from body/schema. + Worker HTTP→WIP coverage verifies diagnostic wrappers do not change refusal + classification or induce retries; tools retain established checkout codes. + Normal-tool JSON→transport→WorkdirError→ToolsError→ToolError coverage checks all + 18 codes with no reason, a typed reason, and nested diagnostic wrappers. In + particular, OutcomeUnknown/Conflict remain execution failures with unchanged + display and inspection-before-retry guidance, not argument errors. + Operational consumers share `WorkdirError::classification_source` to ignore + only diagnostic wrappers while retaining the original error for diagnostics. + Public Write tests cross JSON stat errors into the tool and the checked local + 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 + safe log message; no process is launched and no retry is performed. + +These are hermetic Rust/API/protocol fixtures, not a full product-process E2E, +not a deployed binary test, and not reproduction of the user's asset/request. +Validation commands and concrete counts are recorded on the implementation MR. diff --git a/docs/report/t731-review-verdict-registration-401.md b/docs/report/t731-review-verdict-registration-401.md new file mode 100644 index 000000000..fded7f508 --- /dev/null +++ b/docs/report/t731-review-verdict-registration-401.md @@ -0,0 +1,95 @@ +# T-731 independent review verdict registration rejected + +## Confirmed observation + +- Ticket: T-731, item revision `00001M4G6PHJQ:0`. +- Selected MR: `01a120d6-36fb-7673-92ce-a4cd50ba0d7d`, repository `main`. +- Selectors: `work/T-731-workdir-denial-diagnostics` → `develop`. +- First reviewed source: `f9d70e2ae2c1b226fd5832dfa13102b9f1b6ac4c`. +- Trusted parent-owned Reviewer binding recorded `ReviewRequested` at + `2026-10-09T13:25:51.374292664+00:00`, MR seq2 event + `01a120d7-6c98-7c91-bdc0-6c61d2fe66c1`. +- Reviewer found a real normal-tool diagnostic-wrapper classification regression + through a target-only public-API harness, after 30 focused tests, root check, + formatting and diff checks passed. +- The review-submission tool returned + `Merge Request API returned HTTP 401` with no diagnostic response text. It did + not record the intended request-changes verdict. The committed session's + output reference is `E01a120e2-ae8b-7581-82f9-f0f43565c173`; its reproduction + output is `E01a120de-c8a2-72d3-ba9b-6a9392d19dda`. +- Parent `ShowMergeRequest` read at provider observation + `2026-10-09T13:39:10+00:00` still showed only seq1 comment and seq2 request; + there was no verdict/approval event. The tool response does not provide an + exact failure timestamp, auth reason, or request correlation ID. None is + inferred here. + +## Handling and limitation + +The first finding was fixed in forward commit `10c55874` on the same source ref. +Source movement requires a fresh exact-source trusted review. The original narrative +verdict is not substituted for a recorded authoritative review event. + +Ticket seq4 records this operational blocker for Orchestrator attention. The +Orchestrator directed continuation through the ordinary fresh trusted-review +path after fix/test/publish, and stopping with bounded evidence if submission +again returns 401. No independent Runtime Reviewer, alternate mutation endpoint, +proof/auth/capability change, deployment update, or branch/MR replacement is used. + +The HTTP 401 cause is unknown. It is not established to share a cause with the +reported Workdir 403, and T-731's fixture diagnostics do not resolve either live +incident. The registration failure is not repaired by the product-code changes +in this Ticket. + +## Fresh review: invalid severity rejected (422) + +- Reviewed source after the first forward fix: + `10c55874e3cc4c3562f608e10c56ff1c4c2a4d3c`. +- Fresh request: MR seq3 event `01a120f4-6c90-7791-a36f-69d52e2cbd4b`, + `2026-10-09T13:57:31.897275884+00:00`, same Ticket revision and sole MR. +- Independent review found a second operational regression: normal Write's + bare NotFound match failed to create files for typed/nested diagnostic context. + A public-tool harness reproduced it despite 59 focused tests passing. +- The sole executed verdict attempt returned HTTP 422, specifically invalid + `findings[0].severity = medium`; Backend expects blocker/major/minor/note. + Committed response: `E01a120fc-a7b7-79c3-b812-12cfd1038f75`. +- Observed UTC bracket was 14:05:37.641361776Z–14:06:36.777627982Z on + 2026-10-09, not the exact server request timestamp. Parent MR reread at + 14:11:31Z still had no events after seq3. +- Parent requested a schema-only correction on the unchanged snapshot, but the + Reviewer role's exactly-once instruction disallowed a second verdict call. + Its committed follow-up confirms no corrected submission executed. No other + mutation path or Reviewer was used to bypass this restriction. + +This 422 is input validation evidence, not proof that the earlier 401 has been +resolved. The narrative request-changes judgment is not a recorded verdict. +The reproduced second regression is addressed by the shared classification +source and real Write regression in this forward change on the existing branch. +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 +operation/request-event correlation and classified auth failure (without proof, +credentials, keys, request body, or raw provider errors), so the Orchestrator can +locate the corresponding Backend record. The current bare 401 leaves the +reviewer unable to distinguish authentication failure from downstream authority +rejection. Investigate under separate authorization; do not weaken the guarded +review path or retry blindly to hide the failure. + +Also align the tool schema's severity vocabulary with the Backend enum, rather +than advertising an unconstrained string that consumes an exactly-once verdict +attempt on a deterministic 422. Any correction/review-grant lifecycle policy +needs a separate explicit design; this Ticket does not change it.