diff --git a/crates/tools/src/error.rs b/crates/tools/src/error.rs index 23670006d..df5cd894f 100644 --- a/crates/tools/src/error.rs +++ b/crates/tools/src/error.rs @@ -42,11 +42,7 @@ impl From for ToolError { // 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 { + match error.classification_source() { workdir::WorkdirError::NotFound(_) | workdir::WorkdirError::Io { .. } | workdir::WorkdirError::Unavailable(_) diff --git a/crates/tools/src/file_target.rs b/crates/tools/src/file_target.rs index afe7d82e2..424b005e1 100644 --- a/crates/tools/src/file_target.rs +++ b/crates/tools/src/file_target.rs @@ -139,12 +139,8 @@ impl FileTarget { pub(crate) fn checked_error(error: workdir::WorkdirError) -> ToolError { use workdir::WorkdirError; - if let WorkdirError::DenialContext { source, .. } = error { - // Diagnostic enrichment must not change the established tool classification. - return checked_error(*source); - } - let code = match &error { - WorkdirError::DenialContext { .. } => unreachable!("unwrapped above"), + 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/lib.rs b/crates/workdir/src/lib.rs index 7dec2c00a..c77ee7d81 100644 --- a/crates/workdir/src/lib.rs +++ b/crates/workdir/src/lib.rs @@ -515,6 +515,17 @@ 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), diff --git a/crates/worker/src/checkout.rs b/crates/worker/src/checkout.rs index 5d540c2cc..ce2896062 100644 --- a/crates/worker/src/checkout.rs +++ b/crates/worker/src/checkout.rs @@ -194,12 +194,8 @@ impl Provider { Err(failure) => { // Internal diagnostic enrichment is transparent to WIP's existing // unavailable/refusal classification, including remote providers. - let mut classification = &failure; - while let workdir::WorkdirError::DenialContext { source, .. } = classification { - classification = source.as_ref(); - } if matches!( - classification, + failure.classification_source(), workdir::WorkdirError::Denied(_) | workdir::WorkdirError::OutOfScope(_) | workdir::WorkdirError::NotFound(_) diff --git a/docs/development/workdir-refusal-diagnostics.md b/docs/development/workdir-refusal-diagnostics.md index a29e82dd6..84e309c5f 100644 --- a/docs/development/workdir-refusal-diagnostics.md +++ b/docs/development/workdir-refusal-diagnostics.md @@ -131,6 +131,12 @@ is never parsed into a reason. 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. - 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 diff --git a/docs/report/t731-review-verdict-registration-401.md b/docs/report/t731-review-verdict-registration-401.md index 038978d55..8f08213c5 100644 --- a/docs/report/t731-review-verdict-registration-401.md +++ b/docs/report/t731-review-verdict-registration-401.md @@ -25,8 +25,8 @@ ## 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 +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 @@ -40,6 +40,34 @@ 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. + ## Improvement proposal A verdict-registration rejection should expose a safe, bounded internal @@ -49,3 +77,8 @@ 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.