fix(workdir): keep diagnostic context transparent to Write creation
This commit is contained in:
@@ -42,11 +42,7 @@ impl From<ToolsError> 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(_)
|
||||
|
||||
@@ -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 { .. }
|
||||
|
||||
+158
-1
@@ -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<workdir::StatResult, WorkdirError> {
|
||||
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::<WorkdirTransportError>(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<workdir::ReadResult, WorkdirError> {
|
||||
self.local.read(r).await
|
||||
}
|
||||
async fn write(
|
||||
&self,
|
||||
r: workdir::WriteRequest,
|
||||
) -> Result<workdir::WriteResult, WorkdirError> {
|
||||
self.writes
|
||||
.fetch_add(1, std::sync::atomic::Ordering::SeqCst);
|
||||
self.local.write(r).await
|
||||
}
|
||||
async fn edit(&self, r: workdir::EditRequest) -> Result<workdir::EditResult, WorkdirError> {
|
||||
self.local.edit(r).await
|
||||
}
|
||||
async fn list(&self, r: workdir::ListRequest) -> Result<workdir::ListResult, WorkdirError> {
|
||||
self.local.list(r).await
|
||||
}
|
||||
async fn glob(&self, r: workdir::GlobRequest) -> Result<workdir::GlobResult, WorkdirError> {
|
||||
self.local.glob(r).await
|
||||
}
|
||||
async fn grep(&self, r: workdir::GrepRequest) -> Result<workdir::GrepResult, WorkdirError> {
|
||||
self.local.grep(r).await
|
||||
}
|
||||
async fn start_command(
|
||||
&self,
|
||||
r: workdir::CommandRequest,
|
||||
) -> Result<workdir::CommandHandle, WorkdirError> {
|
||||
self.local.start_command(r).await
|
||||
}
|
||||
async fn command_status(
|
||||
&self,
|
||||
r: workdir::CommandHandle,
|
||||
) -> Result<workdir::CommandStatus, WorkdirError> {
|
||||
self.local.command_status(r).await
|
||||
}
|
||||
async fn command_output(
|
||||
&self,
|
||||
r: workdir::CommandOutputRequest,
|
||||
) -> Result<workdir::CommandOutput, WorkdirError> {
|
||||
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<StatErrorSession> {
|
||||
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();
|
||||
|
||||
@@ -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<String>) -> Self {
|
||||
Self::Denied(WorkdirDenial {
|
||||
reason: Some(reason),
|
||||
|
||||
@@ -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(_)
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user