From 10c55874e3cc4c3562f608e10c56ff1c4c2a4d3c Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 9 Oct 2026 22:55:53 +0900 Subject: [PATCH] fix: preserve tool classification and accurate Workdir failure stages --- crates/tools/src/error.rs | 101 +++++++++++++++-- crates/workspace-server/src/server.rs | 5 +- .../src/server/tests/api_logging_tests.rs | 105 +++++++++++++++++- .../workdir-refusal-diagnostics.md | 14 ++- .../t731-review-verdict-registration-401.md | 51 +++++++++ 5 files changed, 259 insertions(+), 17 deletions(-) create mode 100644 docs/report/t731-review-verdict-registration-401.md diff --git a/crates/tools/src/error.rs b/crates/tools/src/error.rs index b25025b7c..23670006d 100644 --- a/crates/tools/src/error.rs +++ b/crates/tools/src/error.rs @@ -38,17 +38,28 @@ 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. + 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::( + 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/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index bbafe82f8..bc345138c 100644 --- a/crates/workspace-server/src/server.rs +++ b/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 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 f800a1987..f4550745d 100644 --- a/crates/workspace-server/src/server/tests/api_logging_tests.rs +++ b/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" + ); +} diff --git a/docs/development/workdir-refusal-diagnostics.md b/docs/development/workdir-refusal-diagnostics.md index 0d90691e7..a29e82dd6 100644 --- a/docs/development/workdir-refusal-diagnostics.md +++ b/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. 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..038978d55 --- /dev/null +++ b/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.