fix: preserve tool classification and accurate Workdir failure stages

crates/tools/src/error.rs

@@ -38,17 +38,28 @@ pub enum ToolsError {
 impl From<ToolsError> 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.
+                let mut classification = error;
+                while let workdir::WorkdirError::DenialContext { source, .. } = classification {
+                    classification = source.as_ref();
+                }
+                match classification {
+                    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 +145,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::<WorkdirTransportError>(
+                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::<WorkdirTransportError>(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(

crates/workspace-server/src/server.rs

@@ -18354,6 +18354,7 @@ async fn scoped_execute_current_worker_workdir_operation(
                 .start_command(command)
                 .await
                 .map_err(workdir_operation_transport_error)?;
+            context.stage = "command_registration";
             let registered_source = api
                 .workdir_sessions
                 .lock()
@@ -32702,7 +32703,9 @@ impl ApiErrorLog {
             // 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());
-            log.message = "Workdir operation was rejected before provider dispatch".to_string();
+            // 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

crates/workspace-server/src/server/tests/api_logging_tests.rs

@@ -454,10 +454,7 @@ async fn generated_workdir_api_errors_preserve_response_and_log_details() {
     assert_eq!(body, serde_json::to_value(expected).unwrap());
     assert_eq!(events.len(), 1, "{events:?}");
     assert_eq!(events[0]["kind"], "workdir_session_operation_api_403");
-    assert_eq!(
-        events[0]["message"],
-        "Workdir operation was rejected before provider dispatch"
-    );
+    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);
@@ -680,3 +677,103 @@ async fn self_workdir_session_open_refusals_keep_reason_before_registry_conversi
         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"
+    );
+}

docs/development/workdir-refusal-diagnostics.md

@@ -49,8 +49,10 @@ transport carries the enum to its consumer; it contains no free text or paths.
   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, or provider_dispatch. API failures identify the
-  refusing stage without copying raw alias/handle/error text into the log.
+  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.
@@ -125,6 +127,14 @@ is never parsed into a reason.
 - 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.
+- 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.

docs/report/t731-review-verdict-registration-401.md

@@ -0,0 +1,51 @@
+# 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 finding is being fixed in a forward commit 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.
+
+## 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.