diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index 710538fa1..5750a0cda 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -21944,16 +21944,22 @@ async fn open_current_worker_workdir_session_locked( .lock() .expect("External Workdir provider registry poisoned") .connection(grant_id) - .filter(|connection| { - connection.generation == grant.generation - && connection.provider_instance_id == grant.provider_instance_id - && connection.workdir_id == workdir.workdir_id - }) .ok_or_else(|| Error::RuntimeOperationFailed { runtime_id: "workspace-backend".to_string(), - code: "external_workdir_provider_offline".to_string(), + code: "external_workdir_provider_connection_missing".to_string(), message: "External Workdir provider connection is unavailable".to_string(), })?; + if connection.generation != grant.generation + || connection.provider_instance_id != grant.provider_instance_id + || connection.workdir_id != workdir.workdir_id + { + return Err(Error::RuntimeOperationFailed { + runtime_id: "workspace-backend".to_string(), + code: "external_workdir_provider_identity_mismatch".to_string(), + message: "External Workdir provider connection does not match the current grant" + .to_string(), + }); + } let source: WorkdirSessionHandle = Arc::new(ExternalProviderWorkdirSession::new( connection, Some(ExternalProviderAuditContext { @@ -39386,10 +39392,38 @@ impl ApiErrorLog { log.denial_reason = error.denial_reason; log.failure_reason = error.failure_reason; if log.workdir_log.is_some() { - // Public classification and arbitrary diagnostic text are not safe - // operational causes. Keep only the separately typed reason/stage; - // never infer a reason by parsing request- or provider-authored text. - log.kind = format!("workdir_session_operation_api_{}", status.as_u16()); + // Keep bounded Backend codes as well as typed provider reasons. Never + // log arbitrary codes/messages or infer causes from diagnostic prose. + const BACKEND_CODES: &[&str] = &[ + "db_capacity_exceeded", + "db_executor_unavailable", + "db_operation_outcome_unknown", + "working_directory_not_found", + "external_workdir_grant_not_found", + "external_workdir_provider_pending", + "external_workdir_provider_offline", + "external_workdir_provider_revoked", + "external_workdir_provider_expired", + "external_workdir_provider_unavailable", + "external_workdir_provider_connection_missing", + "external_workdir_provider_identity_mismatch", + "duplicate_workdir_session_close_failed", + "workdir_session_registration_failed", + ]; + let backend_code = error + .diagnostics + .as_deref() + .and_then(|diagnostics| diagnostics.first()) + .and_then(|diagnostic| { + BACKEND_CODES + .iter() + .copied() + .find(|code| diagnostic.code == *code) + }); + log.kind = match backend_code { + Some(code) => format!("workdir_session_operation_{code}"), + None => 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(); 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 4c045e51d..29f00c321 100644 --- a/crates/workspace-server/src/server/tests/api_logging_tests.rs +++ b/crates/workspace-server/src/server/tests/api_logging_tests.rs @@ -1,3 +1,5 @@ +mod workdir_open; + use super::*; use std::io::Write; use tracing::instrument::WithSubscriber; diff --git a/crates/workspace-server/src/server/tests/api_logging_tests/workdir_open.rs b/crates/workspace-server/src/server/tests/api_logging_tests/workdir_open.rs new file mode 100644 index 000000000..9df71f453 --- /dev/null +++ b/crates/workspace-server/src/server/tests/api_logging_tests/workdir_open.rs @@ -0,0 +1,243 @@ +//! Signed operations -> real attachment/grant SQLite records -> session opening +//! -> API error logging. Provider connections use an unconsumed channel: rejected +//! opens must not dispatch commands or reads. No live external provider is used. +use super::*; + +#[tokio::test] +async fn external_session_open_logs_distinguish_unavailable_and_mismatched_connections() { + for (state, mismatch, code) in [ + ("pending", None, "external_workdir_provider_pending"), + ("offline", None, "external_workdir_provider_offline"), + ( + "online", + None, + "external_workdir_provider_connection_missing", + ), + ( + "online", + Some("generation"), + "external_workdir_provider_identity_mismatch", + ), + ( + "online", + Some("provider"), + "external_workdir_provider_identity_mismatch", + ), + ( + "online", + Some("workdir"), + "external_workdir_provider_identity_mismatch", + ), + ] { + let mut fixture = manual_worker_assignment_fixture().await; + let identity = RuntimeIdentityMaterial::generate(&fixture.worker.runtime_id).unwrap(); + configure_runtime_request_auth(&mut fixture.api, &identity, &fixture.worker.runtime_id); + let grant = ExternalWorkdirGrantRecord { + grant_id: "logging-external-grant".into(), + workspace_id: TEST_WORKSPACE_ID.into(), + workdir_id: "logging-external-workdir".into(), + provider_instance_id: "logging-provider".into(), + display_name: "body-secret".into(), + permissions: "read_write_command".into(), + created_by: test_browser_request_actor().account_id, + created_at: TEST_CREATED_AT.into(), + expires_at: None, + generation: 1, + status: state.into(), + updated_at: TEST_CREATED_AT.into(), + }; + let mut record = fixture + .api + .store + .list_workdir_registry(TEST_WORKSPACE_ID, 100) + .unwrap() + .into_iter() + .find(|record| record.workdir_id == fixture.main_workdir_id) + .unwrap(); + record.workdir_id = grant.workdir_id.clone(); + record.source = WorkdirRegistrySource::ExternalGrant { + grant_id: grant.grant_id.clone(), + }; + fixture + .api + .store + .create_external_workdir_grant(&grant, &record) + .unwrap(); + fixture + .api + .store + .attach_worker_workdir(&WorkerWorkdirLinkRecord { + connection_id: String::new(), + workspace_id: TEST_WORKSPACE_ID.into(), + worker: fixture.worker.clone(), + workdir_id: grant.workdir_id.clone(), + alias: "external".into(), + capabilities: workdir::WorkdirSessionCapabilities::ALL, + linked_at: TEST_CREATED_AT.into(), + unlinked_at: None, + }) + .unwrap(); + let (sender, mut operations) = tokio::sync::mpsc::channel(1); + if let Some(mismatch) = mismatch { + let connection = ExternalProviderConnection { + grant_id: grant.grant_id.clone(), + workdir_id: if mismatch == "workdir" { + "other-workdir".into() + } else { + grant.workdir_id.clone() + }, + provider_instance_id: if mismatch == "provider" { + "other-provider".into() + } else { + grant.provider_instance_id.clone() + }, + generation: if mismatch == "generation" { + 2 + } else { + grant.generation + }, + 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, + }; + assert!( + fixture + .api + .external_workdir_providers + .lock() + .unwrap() + .connected(Arc::new(connection)) + .is_none() + ); + } + let links = fixture + .api + .store + .list_worker_workdir_links(TEST_WORKSPACE_ID, &fixture.worker) + .unwrap(); + let app = build_router(fixture.api.clone()); + let path = format!("/api/w/{TEST_WORKSPACE_ID}/workers/self/workdir-session/operations"); + for operation in [ + json!({"operation": "command_start", "request": { + "command": "body-secret", "cwd": "", "timeout_secs": 1, + "transport_byte_budget": 1048576, "tool_call_id": "proof-secret" + }}), + json!({"operation": "read", "request": { + "path": "README.md", "offset": 0, "limit": 10, "max_bytes": 1024 + }}), + ] { + let request = runtime_source_request( + &identity, + Some(&fixture.worker.worker_id), + "POST", + &path, + serde_json::to_vec(&json!({"target_workdir": "external", "operation": operation})) + .unwrap(), + ); + let (status, body, events) = request_with_logs(app.clone(), request).await; + assert_eq!( + status, + StatusCode::BAD_GATEWAY, + "{state}/{mismatch:?}: {body}" + ); + assert!( + body["message"] + .as_str() + .unwrap() + .starts_with(&format!("{code}:")), + "{body}" + ); + assert_eq!(events.len(), 1, "{events:?}"); + assert_eq!( + events[0]["kind"], + format!("workdir_session_operation_{code}") + ); + assert_eq!(events[0]["workdir_stage"], "session_open"); + assert_eq!(events[0]["operation"], operation["operation"]); + assert_eq!(events[0]["message"], "Workdir operation failed"); + assert_eq!(events[0]["diagnostics"], "[]"); + assert_eq!(events[0]["failure_reason"], Value::Null); + assert_workdir_log_identity(&events[0], &fixture, "external", Some(&grant.workdir_id)); + assert!( + operations.try_recv().is_err(), + "failed open dispatched an operation" + ); + assert_eq!( + fixture + .api + .store + .list_worker_workdir_links(TEST_WORKSPACE_ID, &fixture.worker) + .unwrap(), + links + ); + } + } +} + +#[test] +fn workdir_backend_diagnostics_keep_only_known_codes_not_messages_or_unknown_codes() { + for (code, expected) in [ + ( + "external_workdir_provider_revoked", + "workdir_session_operation_external_workdir_provider_revoked", + ), + ( + "external_workdir_provider_expired", + "workdir_session_operation_external_workdir_provider_expired", + ), + ( + "external_workdir_grant_not_found", + "workdir_session_operation_external_workdir_grant_not_found", + ), + ( + "workdir_session_registration_failed", + "workdir_session_operation_workdir_session_registration_failed", + ), + ( + "external_workdir_provider_body-secret", + "workdir_session_operation_api_502", + ), + ( + "unknown_provider_error", + "workdir_session_operation_api_502", + ), + ] { + let error = InternalWorkdirOperationError::Contextual { + error: Box::new( + Error::RuntimeOperationFailed { + runtime_id: "workspace-backend".into(), + code: code.into(), + message: "external_workdir_provider_offline body-secret /host/private".into(), + } + .into(), + ), + context: server_api::WorkdirOperationLogContext { + operation: "command_start", + stage: "session_open", + ..Default::default() + }, + } + .into_contract_error(); + let log = ApiErrorLog::from_workdir_operation_error(&error, StatusCode::BAD_GATEWAY); + assert_eq!(log.kind, expected); + assert_eq!(log.message, "Workdir operation failed"); + assert!(log.diagnostics.is_empty()); + assert!(log.failure_reason.is_none()); + assert!(!format!("{log:?}").contains("secret")); + assert!(!format!("{log:?}").contains("/host/private")); + + // A matching message/classification without an actual diagnostic code + // is not evidence of that Backend failure. + let mut without_diagnostics = error; + without_diagnostics.diagnostics = None; + without_diagnostics.error = Some(code.into()); + without_diagnostics.message = code.into(); + let log = ApiErrorLog::from_workdir_operation_error( + &without_diagnostics, + StatusCode::BAD_GATEWAY, + ); + assert_eq!(log.kind, "workdir_session_operation_api_502"); + } +} diff --git a/crates/workspace-server/src/server/tests/restore_workdir_tests.rs b/crates/workspace-server/src/server/tests/restore_workdir_tests.rs index cfd69913b..f3dff8878 100644 --- a/crates/workspace-server/src/server/tests/restore_workdir_tests.rs +++ b/crates/workspace-server/src/server/tests/restore_workdir_tests.rs @@ -167,7 +167,7 @@ async fn restore_keeps_offline_external_and_unmaterialized_repository_attachment "external" => assert!(matches!( error, Error::RuntimeOperationFailed { code, .. } - if code == "external_workdir_provider_offline" + if code == "external_workdir_provider_connection_missing" )), "repository" => assert!(matches!( error,