Add safe diagnostics to workspace routing configuration errors (#53805)

## Why Workspace routing reported generic requirements load and reload failures, hiding the cause. Raw configuration errors can contain secrets, so exposing their messages directly is unsafe. ## What changed Include a safe reason in configuration load and reload errors: file locations or non-file line and column numbers, cloud error categories and HTTP status codes, known network policy denials, or OS error details. Exclude raw parser messages, configuration contents, and backend-supplied policy names and IDs. ## Testing Add regression coverage for configuration locations, cloud failures, and network policy denials. Verify that malformed local and cloud configuration reports useful errors without exposing secret values or policy identifiers, and still prevents workspace routing from falling back to startup configuration. GitOrigin-RevId: a71484c700d32662ced9e19041dbedda5d1da8a5
2026-10-19 16:17:49 +00:00 · 2026-10-13 23:59:56 +00:00
parent 923f276f57
commit ca2b076fff
3 changed files with 153 additions and 26 deletions
@@ -6,6 +6,8 @@
 use super::*;
 use codex_app_server_protocol::AccountRoutingOverride;
 use codex_backend_client::AccountEntry;
+use codex_config::ConfigLoadError;
+use codex_http_client::NetworkPolicyDenied;
 use codex_login::WorkspaceRouting;
 use codex_login::WorkspaceRoutingRequest;
 use codex_login::WorkspaceRoutingResolver;
@@ -54,11 +56,12 @@ pub(super) enum AccountReadError {
    Routing(#[from] WorkspaceRoutingError),
 }

-/// Routing failure categories carry no account, URL, or backend data.
+/// Routing failures carry no account, URL, or backend data. Config failures retain
+/// only a location or structured error category, never the raw config error.
 #[derive(Debug, PartialEq, Eq, thiserror::Error)]
 pub(super) enum WorkspaceRoutingError {
-    #[error("failed to load workspace requirements")]
-    RequirementsLoad,
+    #[error("failed to load workspace requirements: {0}")]
+    RequirementsLoad(String),
    #[error("workspace routing requires a ChatGPT account id")]
    MissingAccountId,
    #[error("workspace routing discovery cancelled")]
@@ -73,8 +76,8 @@ pub(super) enum WorkspaceRoutingError {
    MissingWorkspace,
    #[error("duplicate workspace in routing discovery")]
    DuplicateWorkspace,
-    #[error("failed to reload workspace requirements")]
-    RequirementsReload,
+    #[error("failed to reload workspace requirements: {0}")]
+    RequirementsReload(String),
    #[error("configuration changed during workspace routing discovery; retry account/read")]
    ConfigurationChanged,
    #[error("invalid model provider URL")]
@@ -103,6 +106,77 @@ pub(super) enum WorkspaceRoutingError {
    InvalidBackendOrigin,
 }

+fn workspace_config_load_reason(error: &io::Error) -> String {
+    // Parser messages and cloud bundle errors can quote secrets, even without
+    // their source snippets. Retain only structured diagnostics before these
+    // errors reach JSON-RPC clients, login notifications, or tracing.
+    let mut current = error
+        .get_ref()
+        .map(|source| source as &(dyn std::error::Error + 'static));
+    while let Some(source) = current {
+        if let Some(config) = source.downcast_ref::<ConfigLoadError>() {
+            let config = config.config_error();
+            let start = config.range.start;
+            // Non-file diagnostics store a display name, which can contain
+            // backend-supplied policy names and IDs, in this field.
+            if !config.path.is_absolute() {
+                return format!(
+                    "invalid non-file configuration at line {}, column {}",
+                    start.line, start.column
+                );
+            }
+            return format!(
+                "invalid configuration at {}:{}:{}",
+                config.path.display(),
+                start.line,
+                start.column
+            );
+        }
+        if let Some(cloud) = source.downcast_ref::<CloudConfigBundleLoadError>() {
+            let reason = match cloud.code() {
+                CloudConfigBundleLoadErrorCode::Timeout => "request timed out",
+                CloudConfigBundleLoadErrorCode::Auth
+                | CloudConfigBundleLoadErrorCode::RequestFailed => {
+                    // Cloud loading currently erases the typed denial. Match
+                    // only known safe policy messages, never forward raw text.
+                    let message = cloud.to_string();
+                    if let Some(policy) = [
+                        NetworkPolicyDenied::Unavailable,
+                        NetworkPolicyDenied::Destination,
+                        NetworkPolicyDenied::Revoked,
+                        NetworkPolicyDenied::UnsupportedTransport,
+                    ]
+                    .into_iter()
+                    .find(|policy| policy.to_string() == message)
+                    {
+                        return policy.to_string();
+                    }
+                    if cloud.code() == CloudConfigBundleLoadErrorCode::Auth {
+                        "authentication failed"
+                    } else {
+                        "request failed"
+                    }
+                }
+                CloudConfigBundleLoadErrorCode::InvalidBundle => "invalid bundle",
+                CloudConfigBundleLoadErrorCode::Internal => "internal error",
+            };
+            return match cloud.status_code() {
+                Some(status) => format!("cloud config: {reason} (HTTP {status})"),
+                None => format!("cloud config: {reason}"),
+            };
+        }
+        if let Some(policy) = source.downcast_ref::<NetworkPolicyDenied>() {
+            return policy.to_string();
+        }
+        current = source.source();
+    }
+    // Use the OS error or kind, never an untyped error's potentially secret message.
+    match error.raw_os_error() {
+        Some(code) => io::Error::from_raw_os_error(code).to_string(),
+        None => error.kind().to_string(),
+    }
+}
+
 impl WorkspaceRoutingResolver for AccountRequestProcessor {
    fn resolve(
        &self,
@@ -237,7 +311,12 @@ impl AccountRequestProcessor {
                {
                    self.config.as_ref().clone()
                }
-                Err(_) => return Err(WorkspaceRoutingError::RequirementsLoad.into()),
+                Err(error) => {
+                    return Err(WorkspaceRoutingError::RequirementsLoad(
+                        workspace_config_load_reason(&error),
+                    )
+                    .into());
+                }
            };
            let auth = self.auth_manager.auth_cached();
            let provider = create_model_provider(
@@ -353,9 +432,11 @@ impl AccountRequestProcessor {
                    if accounts.next().is_some() {
                        return Err(WorkspaceRoutingError::DuplicateWorkspace.into());
                    }
-                    let latest_config = load_config()
-                        .await
-                        .map_err(|_| WorkspaceRoutingError::RequirementsReload)?;
+                    let latest_config = load_config().await.map_err(|error| {
+                        WorkspaceRoutingError::RequirementsReload(workspace_config_load_reason(
+                            &error,
+                        ))
+                    })?;
                    if latest_config.chatgpt_base_url != config.chatgpt_base_url
                        || latest_config.model_provider != config.model_provider
                        || latest_config
@@ -5,6 +5,27 @@ use pretty_assertions::assert_eq;
 use serde_json::Value;
 use test_case::test_case;

+#[test_case(CloudConfigBundleLoadErrorCode::RequestFailed, "cloud config: request failed"; "request")]
+#[test_case(CloudConfigBundleLoadErrorCode::Auth, "cloud config: authentication failed"; "refresh")]
+fn cloud_failures_preserve_policy_denials_but_redact_other_messages(
+    code: CloudConfigBundleLoadErrorCode,
+    redacted: &str,
+) {
+    let denied = NetworkPolicyDenied::Destination.to_string();
+    for (message, expected) in [
+        (denied.clone(), denied),
+        (
+            "destination denied by application network policy: sk-test-private-value".into(),
+            redacted.into(),
+        ),
+    ] {
+        let error = io::Error::other(CloudConfigBundleLoadError::new(
+            code, /*status_code*/ None, message,
+        ));
+        assert_eq!(workspace_config_load_reason(&error), expected);
+    }
+}
+
 #[test_case(None; "signed_out")]
 #[test_case(Some("dummy"); "api_key")]
 #[tokio::test]
@@ -119,15 +140,23 @@ async fn background_routing_skips_config_until_chatgpt_sign_in(
    );
    assert!(loads.load(Ordering::SeqCst) > initial_loads);

-    std::fs::write(home.path().join("config.toml"), "invalid = [")?;
-    assert!(matches!(
-        processor
-            .read_account_for_background_workspace_routing()
-            .await,
-        Err(AccountReadError::Routing(
-            WorkspaceRoutingError::RequirementsLoad
-        ))
-    ));
+    let config_path = home.path().join("config.toml");
+    std::fs::write(&config_path, "invalid = [ 'sk-test-private-value',")?;
+    let Err(error) = processor
+        .read_account_for_background_workspace_routing()
+        .await
+    else {
+        panic!("invalid config must prevent workspace routing");
+    };
+    let message = error.to_string();
+    assert!(
+        message.starts_with(&format!(
+            "failed to load workspace requirements: invalid configuration at {}:1:",
+            config_path.display(),
+        )),
+        "{message}"
+    );
+    assert!(!message.contains("sk-test-private-value"), "{message}");
    Ok(())
 }

@@ -395,19 +395,32 @@ async fn api_only_login_does_not_discover_chatgpt_routing() -> Result<()> {
    Ok(())
 }

+#[test_case(
+    json!({"requirements_toml": {"enterprise_managed": [{
+        "id": "invalid", "name": "Invalid requirements", "contents": "application = 'sk-test-private-value'",
+    }]}}),
+    "failed to load workspace requirements: cloud config: invalid bundle";
+    "invalid_requirements"
+)]
+#[test_case(
+    json!({"config_toml": {"enterprise_managed": [{
+        "id": "sk-test-private-id", "name": "sk-test-private-name", "contents": "model = 123",
+    }]}}),
+    "failed to load workspace requirements: invalid non-file configuration at line 1, column 9";
+    "invalid_non_file_config"
+)]
 #[tokio::test]
-async fn failed_workspace_requirements_do_not_fall_back_to_startup_config() -> Result<()> {
+async fn failed_workspace_requirements_do_not_fall_back_to_startup_config(
+    bundle: Value,
+    expected_message: &str,
+) -> Result<()> {
    let backend = MockServer::start().await;
    let home = TempDir::new()?;
    config(&home, &backend).await?;
    let mut server = start(&home).await?;
    backend.reset().await;
    Mock::given(path("/backend-api/wham/config/bundle"))
-        .respond_with(ResponseTemplate::new(200).set_body_json(
-            json!({"requirements_toml": {"enterprise_managed": [{
-                "id": "invalid", "name": "Invalid requirements", "contents": "application = true",
-            }]}}),
-        ))
+        .respond_with(ResponseTemplate::new(200).set_body_json(bundle))
        .mount(&backend)
        .await;
    Mock::given(path("/backend-api/wham/accounts/check"))
@@ -421,7 +434,10 @@ async fn failed_workspace_requirements_do_not_fall_back_to_startup_config() -> R
        server.read_stream_until_notification_message("account/login/completed"),
    )
    .await??;
-    assert_eq!(notification.params.unwrap()["success"], false);
+    assert_eq!(
+        notification.params.expect("account/login/completed params")["success"],
+        false
+    );
    let request = server
        .send_get_account_request(GetAccountParams {
            refresh_token: false,
@@ -432,7 +448,8 @@ async fn failed_workspace_requirements_do_not_fall_back_to_startup_config() -> R
        server.read_stream_until_error_message(RequestId::Integer(request)),
    )
    .await??;
-    assert_eq!(error.error.message, "failed to load workspace requirements");
+    assert_eq!(error.error.code, -32603);
+    assert_eq!(error.error.message, expected_message);
    backend.verify().await;
    Ok(())
 }