From f31315b09f899c811059968346814c676c45fb14 Mon Sep 17 00:00:00 2001 From: wangtsiao Date: Tue, 28 Jul 2026 10:13:49 +0800 Subject: [PATCH 1/4] fix: retry stream idle timeouts and shorten idle window to 60s --- crates/core/src/query.rs | 22 +++++++++ crates/provider/src/anthropic/messages.rs | 7 +-- .../src/openai/chat_completions/stream.rs | 7 +-- crates/provider/src/openai/responses.rs | 7 +-- crates/provider/src/timeout.rs | 46 +++++++++++++++++-- 5 files changed, 77 insertions(+), 12 deletions(-) diff --git a/crates/core/src/query.rs b/crates/core/src/query.rs index 4351d31a..cde7a114 100644 --- a/crates/core/src/query.rs +++ b/crates/core/src/query.rs @@ -289,6 +289,14 @@ fn classify_error(e: &anyhow::Error) -> ErrorClass { } } + if e.chain().any(|cause| { + cause + .downcast_ref::() + .is_some() + }) { + return ErrorClass::NetworkError; + } + if e.chain().any(|cause| { cause.downcast_ref::().is_some_and(|error| { error.is_timeout() @@ -362,6 +370,7 @@ fn classify_error(e: &anyhow::Error) -> ErrorClass { || msg.contains("deadline has elapsed") || msg.contains("deadline exceeded") || msg.contains("provider timeout") + || msg.contains("stream idle timeout") || msg.contains("network error") || msg.contains("network is unreachable") || msg.contains("network unreachable") @@ -2160,6 +2169,19 @@ mod tests { message: "provider request timed out".into(), provider_name: Some("test-provider".into()), }), + anyhow::Error::new(devo_provider::timeout::stream_idle_timeout_provider_error( + "openai", + "gpt-test", + devo_provider::timeout::StreamIdleTimeoutError { + idle_timeout: std::time::Duration::from_secs(60), + }, + )), + anyhow::Error::new(devo_provider::timeout::StreamIdleTimeoutError { + idle_timeout: std::time::Duration::from_secs(60), + }), + anyhow::anyhow!( + "openai stream idle timeout for model gpt-test: provider stream idle timeout after 60s without receiving data" + ), ]; for error in cases { diff --git a/crates/provider/src/anthropic/messages.rs b/crates/provider/src/anthropic/messages.rs index 0ecec216..a87891b8 100644 --- a/crates/provider/src/anthropic/messages.rs +++ b/crates/provider/src/anthropic/messages.rs @@ -391,9 +391,10 @@ impl ModelProviderSDK for AnthropicProvider { Ok(Some(event)) => event, Ok(None) => break, Err(idle) => { - Err(anyhow::anyhow!( - "anthropic stream idle timeout for model {}: {idle}", - request.model + Err(timeout::stream_idle_timeout_provider_error( + "anthropic", + &request.model, + idle, ))? } }; diff --git a/crates/provider/src/openai/chat_completions/stream.rs b/crates/provider/src/openai/chat_completions/stream.rs index b303abd9..7764e7a4 100644 --- a/crates/provider/src/openai/chat_completions/stream.rs +++ b/crates/provider/src/openai/chat_completions/stream.rs @@ -93,9 +93,10 @@ pub(super) async fn completion_stream( Ok(Some(event)) => event, Ok(None) => break, Err(idle) => { - Err(anyhow::anyhow!( - "openai stream idle timeout for model {}: {idle}", - request.model + Err(timeout::stream_idle_timeout_provider_error( + "openai", + &request.model, + idle, ))? } }; diff --git a/crates/provider/src/openai/responses.rs b/crates/provider/src/openai/responses.rs index a178ff58..ff635555 100644 --- a/crates/provider/src/openai/responses.rs +++ b/crates/provider/src/openai/responses.rs @@ -560,9 +560,10 @@ impl ModelProviderSDK for OpenAIResponsesProvider { Ok(Some(event)) => event, Ok(None) => break, Err(idle) => { - Err(anyhow::anyhow!( - "openai responses stream idle timeout for model {}: {idle}", - request.model + Err(timeout::stream_idle_timeout_provider_error( + "openai responses", + &request.model, + idle, ))? } }; diff --git a/crates/provider/src/timeout.rs b/crates/provider/src/timeout.rs index 8bf27371..dc5e90f9 100644 --- a/crates/provider/src/timeout.rs +++ b/crates/provider/src/timeout.rs @@ -9,6 +9,8 @@ use std::time::Duration; use futures::StreamExt; use reqwest_eventsource::{Event, EventSource}; +use crate::error::ProviderError; + /// Total wall-clock timeout for non-streaming provider HTTP requests. pub const REQUEST_TIMEOUT_SECS: u64 = 120; @@ -16,7 +18,7 @@ pub const REQUEST_TIMEOUT_SECS: u64 = 120; pub const CONNECT_TIMEOUT_SECS: u64 = 30; /// Maximum idle time between consecutive SSE events during streaming. -pub const STREAM_IDLE_TIMEOUT_SECS: u64 = 120; +pub const STREAM_IDLE_TIMEOUT_SECS: u64 = 60; /// Total timeout for non-streaming provider HTTP requests. #[inline] @@ -66,6 +68,18 @@ impl std::fmt::Display for StreamIdleTimeoutError { impl std::error::Error for StreamIdleTimeoutError {} +/// Maps a stream idle timeout into a retryable [`ProviderError::ProviderTimeoutError`]. +pub fn stream_idle_timeout_provider_error( + provider_name: &str, + model: &str, + idle: StreamIdleTimeoutError, +) -> ProviderError { + ProviderError::ProviderTimeoutError { + message: format!("{provider_name} stream idle timeout for model {model}: {idle}"), + provider_name: Some(provider_name.to_string()), + } +} + #[cfg(test)] mod tests { use pretty_assertions::assert_eq; @@ -83,7 +97,33 @@ mod tests { } #[test] - fn stream_idle_timeout_is_two_minutes() { - assert_eq!(stream_idle_timeout(), Duration::from_secs(120)); + fn stream_idle_timeout_is_one_minute() { + assert_eq!(stream_idle_timeout(), Duration::from_secs(60)); + } + + #[test] + fn stream_idle_timeout_maps_to_provider_timeout_error() { + let error = stream_idle_timeout_provider_error( + "openai", + "gpt-test", + StreamIdleTimeoutError { + idle_timeout: stream_idle_timeout(), + }, + ); + assert!(error.is_recoverable()); + assert!(error.is_transient()); + match error { + ProviderError::ProviderTimeoutError { + message, + provider_name, + } => { + assert_eq!( + message, + "openai stream idle timeout for model gpt-test: provider stream idle timeout after 60s without receiving data" + ); + assert_eq!(provider_name.as_deref(), Some("openai")); + } + other => panic!("expected ProviderTimeoutError, got {other:?}"), + } } } From c87c5bc8308c2ff13cb89329d4600d9beab18454 Mon Sep 17 00:00:00 2001 From: wangtsiao Date: Tue, 28 Jul 2026 10:38:48 +0800 Subject: [PATCH 2/4] feat: show actionable recovery hints on provider and onboard failures --- crates/core/src/conversation/records.rs | 11 + crates/protocol/src/event.rs | 5 + crates/provider/src/lib.rs | 5 + crates/provider/src/recovery_hint.rs | 208 ++++++++++++++++++ .../server/src/runtime/turn_exec/failure.rs | 29 +++ .../server/src/runtime/turn_exec/finalize.rs | 8 +- crates/server/tests/persistence_resume.rs | 1 + .../tests/provider_failure_reporting.rs | 2 + crates/tui/src/chatwidget/worker_events.rs | 11 +- crates/tui/src/chatwidget_tests.rs | 31 +++ crates/tui/src/events.rs | 4 + crates/tui/src/onboarding_widget.rs | 40 +++- crates/tui/src/onboarding_widget_tests.rs | 4 +- crates/tui/src/worker.rs | 67 +++++- 14 files changed, 409 insertions(+), 17 deletions(-) create mode 100644 crates/provider/src/recovery_hint.rs diff --git a/crates/core/src/conversation/records.rs b/crates/core/src/conversation/records.rs index 45f5fce1..569368bb 100644 --- a/crates/core/src/conversation/records.rs +++ b/crates/core/src/conversation/records.rs @@ -319,6 +319,9 @@ pub struct TurnError { pub code: String, /// The human-readable error message. pub message: String, + /// Optional user-facing next step for recovering from this failure. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub recovery_hint: Option, } /// Stores one persisted item record in the canonical journal. @@ -626,6 +629,7 @@ mod tests { error: Some(TurnError { code: "PROVIDER_SERVER_ERROR".into(), message: "provider request failed".into(), + recovery_hint: None, }), schema_version: 4, ..make_test_turn(TurnStatus::Running) @@ -729,6 +733,7 @@ mod tests { error: Some(TurnError { code: "TOOL_EXECUTION_FAILED".into(), message: "command exited with code 1".into(), + recovery_hint: None, }), ..make_test_item() }; @@ -1036,26 +1041,32 @@ mod tests { TurnError { code: "CONTEXT_LIMIT_EXCEEDED".into(), message: "Too many tokens".into(), + recovery_hint: None, }, TurnError { code: "MODEL_RESOLUTION_FAILED".into(), message: "No valid binding".into(), + recovery_hint: None, }, TurnError { code: "PROVIDER_RATE_LIMITED".into(), message: "Retry after 30s".into(), + recovery_hint: None, }, TurnError { code: "PERSISTENCE_FAILURE".into(), message: "Disk full".into(), + recovery_hint: None, }, TurnError { code: "TOOL_EXECUTION_FAILED".into(), message: "exit code 1".into(), + recovery_hint: None, }, TurnError { code: "APPROVAL_TIMEOUT".into(), message: "User did not respond".into(), + recovery_hint: None, }, ]; for err in &errors { diff --git a/crates/protocol/src/event.rs b/crates/protocol/src/event.rs index 3adbdb21..1ead79cf 100644 --- a/crates/protocol/src/event.rs +++ b/crates/protocol/src/event.rs @@ -116,6 +116,10 @@ pub struct TurnFailedPayload { pub struct TurnErrorPayload { pub code: String, pub message: String, + /// Optional user-facing next step for recovering from this failure. + #[serde(default, skip_serializing_if = "Option::is_none")] + #[ts(optional)] + pub recovery_hint: Option, } #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] @@ -572,6 +576,7 @@ mod tests { error: Some(TurnErrorPayload { code: "PROVIDER_SERVER_ERROR".to_string(), message: "Internal server error".to_string(), + recovery_hint: None, }), }; diff --git a/crates/provider/src/lib.rs b/crates/provider/src/lib.rs index 08b09417..dc4dba71 100644 --- a/crates/provider/src/lib.rs +++ b/crates/provider/src/lib.rs @@ -11,6 +11,7 @@ mod hosted_tools; mod http; pub mod openai; mod provider; +pub mod recovery_hint; mod request; pub mod router; mod text_normalization; @@ -18,5 +19,9 @@ pub mod timeout; pub use http::ProviderHttpOptions; pub use provider::*; +pub use recovery_hint::{ + AUTH_HINT, MODEL_NOT_FOUND_HINT, NETWORK_PROXY_HINT, recovery_hint_for_anyhow, + recovery_hint_for_message, +}; pub(crate) use request::merge_extra_body; pub use router::*; diff --git a/crates/provider/src/recovery_hint.rs b/crates/provider/src/recovery_hint.rs new file mode 100644 index 00000000..5901d964 --- /dev/null +++ b/crates/provider/src/recovery_hint.rs @@ -0,0 +1,208 @@ +//! User-facing recovery hints for provider failures. +//! +//! Keeps actionable next-step copy in one place so query turn failures and +//! onboarding validation can show the same guidance. + +use crate::error::ProviderError; +use crate::timeout::StreamIdleTimeoutError; + +/// Network / proxy / timeout guidance. +pub const NETWORK_PROXY_HINT: &str = "Check network connectivity and proxy settings ([provider_http].proxy_url or HTTPS_PROXY/HTTP_PROXY)."; + +/// API key / credential guidance. +pub const AUTH_HINT: &str = "Verify the API key or credential in auth.json / provider config."; + +/// Model name / access guidance. +pub const MODEL_NOT_FOUND_HINT: &str = + "Confirm the model name and that your provider account can access it."; + +impl ProviderError { + /// Optional user-facing next step for recovering from this error. + pub fn recovery_hint(&self) -> Option<&'static str> { + match self { + Self::AuthenticationError { .. } => Some(AUTH_HINT), + Self::ProviderTimeoutError { .. } | Self::StreamError { .. } => { + Some(NETWORK_PROXY_HINT) + } + Self::ProviderServerError { + status_code: Some(408), + .. + } + | Self::UnknownError { + status_code: Some(408), + .. + } => Some(NETWORK_PROXY_HINT), + Self::ModelNotFoundError { .. } => Some(MODEL_NOT_FOUND_HINT), + Self::RateLimitError { .. } + | Self::ProviderServerError { .. } + | Self::ContextLimitError { .. } + | Self::QuotaExceededError { .. } + | Self::ContentFilteredError { .. } + | Self::InvalidRequestError { .. } + | Self::UnknownError { .. } => None, + } + } +} + +/// Derives a recovery hint from a structured or stringly-typed provider failure. +pub fn recovery_hint_for_anyhow(error: &anyhow::Error) -> Option { + for cause in error.chain() { + if let Some(provider_error) = cause.downcast_ref::() { + return provider_error.recovery_hint().map(str::to_string); + } + if cause.downcast_ref::().is_some() { + return Some(NETWORK_PROXY_HINT.to_string()); + } + if let Some(reqwest_error) = cause.downcast_ref::() { + if reqwest_error.status() == Some(reqwest::StatusCode::UNAUTHORIZED) + || reqwest_error.status() == Some(reqwest::StatusCode::FORBIDDEN) + { + return Some(AUTH_HINT.to_string()); + } + if reqwest_error.is_timeout() + || reqwest_error.is_connect() + || reqwest_error.status() == Some(reqwest::StatusCode::REQUEST_TIMEOUT) + { + return Some(NETWORK_PROXY_HINT.to_string()); + } + } + } + + recovery_hint_for_message(&error.to_string()) +} + +/// Derives a recovery hint from a flattened failure message. +/// +/// Used when only a string is available (for example worker-side validation +/// timeouts or RPC error text). +pub fn recovery_hint_for_message(message: &str) -> Option { + let msg = message.to_lowercase(); + if msg.contains("authentication failed") + || msg.contains("unauthorized") + || msg.contains("api key") + || msg.contains("credential") + || msg.contains("invalid api key") + || msg.contains("missing credential") + || (msg.contains("401") && !msg.contains("1401")) + || msg.contains("403") + { + return Some(AUTH_HINT.to_string()); + } + if msg.contains("model not found") + || (msg.contains("404") + && (msg.contains("does not exist") + || msg.contains("not found") + || msg.contains("model"))) + { + return Some(MODEL_NOT_FOUND_HINT.to_string()); + } + if msg.contains("stream idle timeout") + || msg.contains("provider timeout") + || msg.contains("request timeout") + || msg.contains("request timed out") + || msg.contains("operation timed out") + || msg.contains("timed out") + || msg.contains("deadline has elapsed") + || msg.contains("deadline exceeded") + || msg.contains("connection refused") + || msg.contains("connection reset") + || msg.contains("connection closed") + || msg.contains("connection aborted") + || msg.contains("connection timed out") + || msg.contains("connection failure") + || msg.contains("connection failed") + || msg.contains("failed to connect") + || msg.contains("connect error") + || msg.contains("error trying to connect") + || msg.contains("error sending request") + || msg.contains("dns error") + || msg.contains("failed to lookup address information") + || msg.contains("temporary failure in name resolution") + || msg.contains("name or service not known") + || msg.contains("nodename nor servname") + || msg.contains("could not resolve host") + || msg.contains("network is unreachable") + || msg.contains("network unreachable") + || msg.contains("host unreachable") + || msg.contains("proxy") + || msg.contains("408") + { + return Some(NETWORK_PROXY_HINT.to_string()); + } + None +} + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + + use super::*; + use crate::timeout::StreamIdleTimeoutError; + use std::time::Duration; + + #[test] + fn provider_timeout_maps_to_network_hint() { + let error = ProviderError::ProviderTimeoutError { + message: "idle".into(), + provider_name: Some("openai".into()), + }; + assert_eq!(error.recovery_hint(), Some(NETWORK_PROXY_HINT)); + } + + #[test] + fn authentication_maps_to_auth_hint() { + let error = ProviderError::AuthenticationError { + message: "bad key".into(), + provider_name: Some("openai".into()), + status_code: Some(401), + }; + assert_eq!(error.recovery_hint(), Some(AUTH_HINT)); + } + + #[test] + fn model_not_found_maps_to_model_hint() { + let error = ProviderError::ModelNotFoundError { + message: "missing".into(), + model_name: Some("gpt-test".into()), + }; + assert_eq!(error.recovery_hint(), Some(MODEL_NOT_FOUND_HINT)); + } + + #[test] + fn rate_limit_has_no_hint() { + let error = ProviderError::RateLimitError { + message: "slow down".into(), + retry_after_seconds: Some(30), + provider_name: None, + }; + assert_eq!(error.recovery_hint(), None); + } + + #[test] + fn anyhow_idle_timeout_maps_to_network_hint() { + let error = anyhow::Error::new(StreamIdleTimeoutError { + idle_timeout: Duration::from_secs(60), + }); + assert_eq!( + recovery_hint_for_anyhow(&error).as_deref(), + Some(NETWORK_PROXY_HINT) + ); + } + + #[test] + fn message_heuristics_cover_validation_timeout() { + assert_eq!( + recovery_hint_for_message("provider validation request timed out").as_deref(), + Some(NETWORK_PROXY_HINT) + ); + assert_eq!( + recovery_hint_for_message("anthropic provider requires an API key").as_deref(), + Some(AUTH_HINT) + ); + assert_eq!( + recovery_hint_for_message("model not found: gpt-missing").as_deref(), + Some(MODEL_NOT_FOUND_HINT) + ); + assert_eq!(recovery_hint_for_message("quota exceeded"), None); + } +} diff --git a/crates/server/src/runtime/turn_exec/failure.rs b/crates/server/src/runtime/turn_exec/failure.rs index 1f7f243e..a5dfbdb6 100644 --- a/crates/server/src/runtime/turn_exec/failure.rs +++ b/crates/server/src/runtime/turn_exec/failure.rs @@ -1,5 +1,6 @@ use devo_protocol::{TurnErrorPayload, TurnFailureReason}; use devo_provider::error::ProviderError; +use devo_provider::recovery_hint_for_anyhow; pub(super) fn turn_failure_reason_from_error( error: &devo_core::AgentError, @@ -22,9 +23,16 @@ pub(super) fn turn_error_payload_from_error(error: &devo_core::AgentError) -> Tu devo_core::AgentError::ContextTooLong => "CONTEXT_TOO_LONG", devo_core::AgentError::Aborted => "ABORTED", }; + let recovery_hint = match error { + devo_core::AgentError::Provider(source) => recovery_hint_for_anyhow(source), + devo_core::AgentError::MaxTurnsExceeded(_) + | devo_core::AgentError::ContextTooLong + | devo_core::AgentError::Aborted => None, + }; TurnErrorPayload { code: code.to_string(), message: error.to_string(), + recovery_hint, } } @@ -33,6 +41,7 @@ mod tests { use pretty_assertions::assert_eq; use super::*; + use devo_provider::NETWORK_PROXY_HINT; #[test] fn preserves_structured_provider_error_code() { @@ -51,6 +60,26 @@ mod tests { message: "model provider error: provider server error (Some(500)): Internal server error" .to_string(), + recovery_hint: None, + } + ); + } + + #[test] + fn provider_timeout_includes_network_recovery_hint() { + let error = devo_core::AgentError::Provider(anyhow::Error::new( + ProviderError::ProviderTimeoutError { + message: "stream idle timeout".to_string(), + provider_name: Some("openai".to_string()), + }, + )); + + assert_eq!( + turn_error_payload_from_error(&error), + TurnErrorPayload { + code: "PROVIDER_TIMEOUT_ERROR".to_string(), + message: "model provider error: provider timeout: stream idle timeout".to_string(), + recovery_hint: Some(NETWORK_PROXY_HINT.to_string()), } ); } diff --git a/crates/server/src/runtime/turn_exec/finalize.rs b/crates/server/src/runtime/turn_exec/finalize.rs index a2cd0c92..6d6eb42a 100644 --- a/crates/server/src/runtime/turn_exec/finalize.rs +++ b/crates/server/src/runtime/turn_exec/finalize.rs @@ -92,6 +92,7 @@ impl ServerRuntime { .map(|error| TurnError { code: error.code, message: error.message, + recovery_hint: error.recovery_hint, }); if let Some(snapshot) = usage_snapshot { session_total_input_tokens = snapshot.session_totals.input_tokens; @@ -344,6 +345,7 @@ impl ServerRuntime { error: terminal_error.map(|error| devo_protocol::TurnErrorPayload { code: error.code.clone(), message: error.message.clone(), + recovery_hint: error.recovery_hint.clone(), }), })) .await; @@ -381,10 +383,14 @@ fn append_terminal_history_items( terminal_error: Option<&TurnError>, ) { if let Some(error) = terminal_error { + let title = error + .recovery_hint + .clone() + .unwrap_or_else(|| error.code.clone()); state.history_items.push(SessionHistoryItem::new( None, SessionHistoryItemKind::Error, - error.code.clone(), + title, error.message.clone(), )); } diff --git a/crates/server/tests/persistence_resume.rs b/crates/server/tests/persistence_resume.rs index 0c2ce4d7..3c5dc74c 100644 --- a/crates/server/tests/persistence_resume.rs +++ b/crates/server/tests/persistence_resume.rs @@ -867,6 +867,7 @@ async fn failed_turn_resume_restores_terminal_history_without_prompt_contaminati let terminal_error = TurnError { code: "PROVIDER_SERVER_ERROR".to_string(), message: "exact persisted provider failure".to_string(), + recovery_hint: None, }; let session = SessionRecord { id: session_id, diff --git a/crates/server/tests/provider_failure_reporting.rs b/crates/server/tests/provider_failure_reporting.rs index c1427855..1ce616c7 100644 --- a/crates/server/tests/provider_failure_reporting.rs +++ b/crates/server/tests/provider_failure_reporting.rs @@ -205,6 +205,7 @@ async fn exhausted_provider_retries_persist_for_history_but_do_not_enter_context message: format!( "model provider error: provider server error (Some(500)): {PROVIDER_ERROR_TEXT}" ), + recovery_hint: None, }) ); assert_eq!(failed_agent_items, Vec::new()); @@ -226,6 +227,7 @@ async fn exhausted_provider_retries_persist_for_history_but_do_not_enter_context message: format!( "model provider error: provider server error (Some(500)): {PROVIDER_ERROR_TEXT}" ), + recovery_hint: None, }) ); diff --git a/crates/tui/src/chatwidget/worker_events.rs b/crates/tui/src/chatwidget/worker_events.rs index 42b4ae3b..d42d2023 100644 --- a/crates/tui/src/chatwidget/worker_events.rs +++ b/crates/tui/src/chatwidget/worker_events.rs @@ -995,6 +995,7 @@ impl ChatWidget { } WorkerEvent::TurnFailed { message, + hint, turn_count, total_input_tokens, total_output_tokens, @@ -1042,7 +1043,7 @@ impl ChatWidget { .unwrap_or_default() }; let accent_color = self.active_accent_color(); - self.add_to_history(history_cell::new_error_event(message)); + self.add_to_history(history_cell::new_error_event_with_hint(message, hint)); self.add_to_history(history_cell::TurnSummaryCell::new_failed( input_mode, model_name, @@ -1067,15 +1068,17 @@ impl ChatWidget { self.busy = false; self.set_status_message("Saving provider"); } - WorkerEvent::ProviderValidationFailed { message } => { + WorkerEvent::ProviderValidationFailed { message, hint } => { if let Some(onboarding) = self.onboarding.as_mut() { - onboarding.on_validation_failed(message.clone()); + onboarding.on_validation_failed(message.clone(), hint.clone()); } self.drain_onboarding_transcript_events(); self.busy = false; + let transcript_hint = + hint.unwrap_or_else(|| "provider validation failed".to_string()); self.add_to_history(history_cell::new_error_event_with_hint( message, - Some("provider validation failed".to_string()), + Some(transcript_hint), )); self.set_status_message("Provider validation failed"); } diff --git a/crates/tui/src/chatwidget_tests.rs b/crates/tui/src/chatwidget_tests.rs index 2157266e..76d7b0ac 100644 --- a/crates/tui/src/chatwidget_tests.rs +++ b/crates/tui/src/chatwidget_tests.rs @@ -3319,6 +3319,7 @@ fn onboarding_validation_bypassed_exits_when_configured() { widget.handle_worker_event(crate::events::WorkerEvent::ProviderValidationFailed { message: "validation failed".to_string(), + hint: None, }); widget.handle_key_event(press_key(KeyCode::Enter)); match app_event_rx.try_recv().expect("skip validation command") { @@ -5008,6 +5009,7 @@ fn paired_failed_turn_events_finalize_ui_once_in_order() { widget.handle_worker_event(crate::events::WorkerEvent::TurnFailed { message: provider_error.to_string(), + hint: None, turn_count: 0, total_input_tokens: 10, total_output_tokens: 2, @@ -5072,6 +5074,35 @@ fn paired_failed_turn_events_finalize_ui_once_in_order() { assert!(!history.contains("interrupted"), "history:\n{history}"); } +#[test] +fn turn_failed_renders_recovery_hint() { + let mut widget = widget_with_live_explored_cell(); + let provider_error = "model provider error: provider timeout: stream idle timeout"; + let recovery_hint = devo_provider::NETWORK_PROXY_HINT; + + widget.handle_worker_event(crate::events::WorkerEvent::TurnFailed { + message: provider_error.to_string(), + hint: Some(recovery_hint.to_string()), + turn_count: 0, + total_input_tokens: 10, + total_output_tokens: 2, + total_tokens: 12, + total_cache_read_tokens: 1, + prompt_token_estimate: 10, + last_query_input_tokens: 10, + }); + + let history = scrollback_plain_lines(&widget.drain_scrollback_lines(100)).join("\n"); + assert!( + history.contains(provider_error), + "history should contain provider error:\n{history}" + ); + assert!( + history.contains(recovery_hint), + "history should contain recovery hint:\n{history}" + ); +} + #[test] fn legacy_failed_turn_finished_flushes_explored_before_footer() { let mut widget = widget_with_live_explored_cell(); diff --git a/crates/tui/src/events.rs b/crates/tui/src/events.rs index 4a3d9d22..a43fd38a 100644 --- a/crates/tui/src/events.rs +++ b/crates/tui/src/events.rs @@ -384,6 +384,8 @@ pub(crate) enum WorkerEvent { TurnFailed { /// Human-readable error text to surface in the transcript and status bar. message: String, + /// Optional user-facing next step for recovering from this failure. + hint: Option, /// Total turns completed in the session so far. turn_count: usize, /// Total input tokens accumulated in the session. @@ -408,6 +410,8 @@ pub(crate) enum WorkerEvent { ProviderValidationFailed { /// Human-readable failure reason from the probe request. message: String, + /// Optional user-facing next step for recovering from this failure. + hint: Option, }, /// Current provider vendors were listed from the server. ProviderVendorsListed { diff --git a/crates/tui/src/onboarding_widget.rs b/crates/tui/src/onboarding_widget.rs index 4b752fc9..b933c5ea 100644 --- a/crates/tui/src/onboarding_widget.rs +++ b/crates/tui/src/onboarding_widget.rs @@ -218,6 +218,7 @@ enum OnboardingState { base_url: Option, api_key: Option, error_message: String, + recovery_hint: Option, selected_action: usize, }, } @@ -423,6 +424,7 @@ impl OnboardingWidget { .. } = &self.state { + let recovery_hint = devo_provider::recovery_hint_for_message(&error_message); self.state = OnboardingState::ValidationFailed { model: model_slug.clone(), request_model: request_model.clone(), @@ -434,13 +436,18 @@ impl OnboardingWidget { base_url: base_url.clone(), api_key: api_key.clone(), error_message, + recovery_hint, selected_action: 0, }; } } /// Called when validation fails. - pub(crate) fn on_validation_failed(&mut self, error_message: String) { + pub(crate) fn on_validation_failed( + &mut self, + error_message: String, + recovery_hint: Option, + ) { if let OnboardingState::Validating { model_slug, request_model, @@ -465,6 +472,7 @@ impl OnboardingWidget { base_url: base_url.clone(), api_key: api_key.clone(), error_message, + recovery_hint, selected_action: 0, }; } @@ -1439,6 +1447,7 @@ impl OnboardingWidget { base_url, api_key, error_message: _, + recovery_hint: _, selected_action, } = &mut self.state else { @@ -2350,6 +2359,7 @@ impl OnboardingWidget { fn render_validation_failed( error_message: &str, + recovery_hint: Option<&str>, selected_action: usize, area: Rect, buf: &mut Buffer, @@ -2373,8 +2383,14 @@ impl OnboardingWidget { error_message.to_string(), Style::default().red(), )]), - Line::from(""), ]; + if let Some(hint) = recovery_hint.filter(|hint| !hint.trim().is_empty()) { + lines.push(Line::from(vec![Span::styled( + hint.to_string(), + Style::default().dim(), + )])); + } + lines.push(Line::from("")); for (idx, action) in actions.iter().enumerate() { let is_selected = idx == selected_action; @@ -2473,7 +2489,16 @@ impl Renderable for OnboardingWidget { } OnboardingState::Validating { .. } => 10, OnboardingState::Saving { .. } => 10, - OnboardingState::ValidationFailed { .. } => 13, + OnboardingState::ValidationFailed { recovery_hint, .. } => { + if recovery_hint + .as_ref() + .is_some_and(|hint| !hint.trim().is_empty()) + { + 14 + } else { + 13 + } + } } } @@ -2642,10 +2667,17 @@ impl Renderable for OnboardingWidget { } OnboardingState::ValidationFailed { error_message, + recovery_hint, selected_action, .. } => { - Self::render_validation_failed(error_message, *selected_action, area, buf); + Self::render_validation_failed( + error_message, + recovery_hint.as_deref(), + *selected_action, + area, + buf, + ); } } } diff --git a/crates/tui/src/onboarding_widget_tests.rs b/crates/tui/src/onboarding_widget_tests.rs index 94200c23..42819502 100644 --- a/crates/tui/src/onboarding_widget_tests.rs +++ b/crates/tui/src/onboarding_widget_tests.rs @@ -159,7 +159,7 @@ fn failed_validation_widget() -> (OnboardingWidget, mpsc::UnboundedReceiver { let _ = event_tx.send(WorkerEvent::TurnFailed { message: format!("failed to resume session: {error}"), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -984,6 +986,7 @@ async fn run_worker_inner( Err(error) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1002,6 +1005,7 @@ async fn run_worker_inner( if active_turn_id.is_some() { let _ = event_tx.send(WorkerEvent::TurnFailed { message: "cannot run shell command while a turn is in progress".to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1086,13 +1090,22 @@ async fn run_worker_inner( }); } Ok(Err(error)) => { + let message = error.to_string(); + let hint = + devo_provider::recovery_hint_for_message(&message); let _ = event_tx.send(WorkerEvent::ProviderValidationFailed { - message: error.to_string(), + message, + hint, }); } Err(_) => { + let message = + "provider validation request timed out".to_string(); + let hint = + devo_provider::recovery_hint_for_message(&message); let _ = event_tx.send(WorkerEvent::ProviderValidationFailed { - message: "provider validation request timed out".to_string(), + message, + hint, }); } } @@ -1112,6 +1125,7 @@ async fn run_worker_inner( Ok(Err(error)) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1124,6 +1138,7 @@ async fn run_worker_inner( Err(_) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: "provider list request timed out".to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1227,6 +1242,7 @@ async fn run_worker_inner( Ok(Err(error)) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1239,6 +1255,7 @@ async fn run_worker_inner( Err(_) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: "session list request timed out".to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1256,6 +1273,7 @@ async fn run_worker_inner( { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1293,6 +1311,7 @@ async fn run_worker_inner( let Some(active_session_id) = session_id else { let _ = event_tx.send(WorkerEvent::TurnFailed { message: "no active session exists yet; send a prompt or switch to a saved session first".to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1306,6 +1325,7 @@ async fn run_worker_inner( if active_turn_id.is_some() { let _ = event_tx.send(WorkerEvent::TurnFailed { message: "cannot compact while a turn is in progress".to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1335,6 +1355,7 @@ async fn run_worker_inner( Err(error) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1553,6 +1574,7 @@ async fn run_worker_inner( Err(error) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1690,6 +1712,7 @@ async fn run_worker_inner( Err(error) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1705,6 +1728,7 @@ async fn run_worker_inner( let Some(active_session_id) = session_id else { let _ = event_tx.send(WorkerEvent::TurnFailed { message: "no active session exists yet; send a prompt or switch to a saved session first".to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1734,6 +1758,7 @@ async fn run_worker_inner( Err(error) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1752,6 +1777,7 @@ async fn run_worker_inner( let Some(active_session_id) = session_id else { let _ = event_tx.send(WorkerEvent::TurnFailed { message: "no active session exists yet; send a prompt or switch to a saved session first".to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1823,6 +1849,7 @@ async fn run_worker_inner( Err(error) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1838,6 +1865,7 @@ async fn run_worker_inner( let Some(active_session_id) = session_id else { let _ = event_tx.send(WorkerEvent::TurnFailed { message: "no active session exists yet; send a prompt or switch to a saved session first".to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1929,6 +1957,7 @@ async fn run_worker_inner( Err(error) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -1943,6 +1972,7 @@ async fn run_worker_inner( Err(error) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -2027,6 +2057,7 @@ async fn run_worker_inner( Err(error) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -2058,6 +2089,7 @@ async fn run_worker_inner( { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -2085,6 +2117,7 @@ async fn run_worker_inner( { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -2105,6 +2138,7 @@ async fn run_worker_inner( { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -2181,6 +2215,7 @@ async fn run_worker_inner( Err(error) => { let _ = event_tx.send(WorkerEvent::TurnFailed { message: error.to_string(), + hint: None, turn_count, total_input_tokens, total_output_tokens, @@ -2635,10 +2670,29 @@ async fn run_worker_inner( "turn/failed" => { if let ServerEvent::TurnFailed(TurnFailedPayload { turn, error, .. }) = event { active_turn_id = None; - let message = error - .map(|error| error.message) - .or_else(|| latest_completed_agent_message.take()) - .unwrap_or_else(|| format!("turn failed with status {:?}", turn.status)); + let (message, hint) = match error { + Some(error) => { + let hint = error.recovery_hint.or_else(|| { + devo_provider::recovery_hint_for_message( + &error.message, + ) + }); + (error.message, hint) + } + None => { + let message = latest_completed_agent_message + .take() + .unwrap_or_else(|| { + format!( + "turn failed with status {:?}", + turn.status + ) + }); + let hint = + devo_provider::recovery_hint_for_message(&message); + (message, hint) + } + }; if let Some(usage) = &turn.usage { if !saw_usage_update_for_turn { last_query_input_tokens = usage.input_tokens as usize; @@ -2658,6 +2712,7 @@ async fn run_worker_inner( } let _ = event_tx.send(WorkerEvent::TurnFailed { message, + hint, turn_count, total_input_tokens, total_output_tokens, From 39571419b31b9827ef1346345f63ad2eeb63777b Mon Sep 17 00:00:00 2001 From: wangtsiao Date: Tue, 28 Jul 2026 18:03:29 +0800 Subject: [PATCH 3/4] fix: harden edit input handling and exact-match safety --- crates/core/src/tools/edit.txt | 1 + crates/core/src/tools/handlers/edit.rs | 295 +++++++++++++++++++++++-- crates/core/src/tools/registry_plan.rs | 30 ++- 3 files changed, 306 insertions(+), 20 deletions(-) diff --git a/crates/core/src/tools/edit.txt b/crates/core/src/tools/edit.txt index 0a73c018..50520e71 100644 --- a/crates/core/src/tools/edit.txt +++ b/crates/core/src/tools/edit.txt @@ -2,6 +2,7 @@ Performs exact string replacements in files. Usage: - You must use your `Read` tool at least once on the full file (no offset/limit) in this session before editing. This tool will error if the file has not been read, or if the file changed since it was last read. +- Preferred input field names are `filePath`, `oldString`, `newString`, and `replaceAll`. For compatibility, `path`, `file_path`, `old_string`, `new_string`, and `replace_all` are also accepted. - When editing text from Read tool output, ensure you preserve the exact indentation (tabs/spaces) as it appears AFTER the line number prefix. The line number prefix format is: line number + colon + space (e.g., `1: `). Everything after that space is the actual file content to match. Never include any part of the line number prefix in the oldString or newString. - ALWAYS prefer editing existing files in the codebase. NEVER write new files unless explicitly required. Use `Write` to create new files; `edit` only modifies existing files. - Prefer `edit` for small, surgical changes. Prefer `apply_patch` for multi-file or large structured edits. Prefer `Write` for full-file rewrites. diff --git a/crates/core/src/tools/handlers/edit.rs b/crates/core/src/tools/handlers/edit.rs index 53aacfb6..f63107f0 100644 --- a/crates/core/src/tools/handlers/edit.rs +++ b/crates/core/src/tools/handlers/edit.rs @@ -6,9 +6,10 @@ use std::path::PathBuf; use async_trait::async_trait; use devo_tools::ClientTextFileRead; use devo_tools::ClientTextFileWrite; +use devo_tools::FileReadFreshnessError; use tracing::info; -use super::file_change_metadata::write_tool_result; +use super::file_change_metadata::{file_mtime, write_tool_result}; use crate::contracts::{ ToolCallError, ToolContext, ToolProgressSender, ToolResult, ToolResultContent, }; @@ -39,26 +40,48 @@ impl EditHandler { std::collections::BTreeMap::from([ ( "filePath".to_string(), - JsonSchema::string(Some("The absolute path to the file to modify")), + JsonSchema::string(Some( + "The absolute path to the file to modify. Preferred field name; `path` and `file_path` are also accepted.", + )), + ), + ( + "path".to_string(), + JsonSchema::string(Some("Alias for `filePath`.")), + ), + ( + "file_path".to_string(), + JsonSchema::string(Some("Alias for `filePath`.")), ), ( "oldString".to_string(), JsonSchema::string(Some( - "The exact text to replace. Must be non-empty and unique unless replaceAll is true.", + "The exact text to replace. Must be non-empty and unique unless replaceAll is true. Preferred field name; `old_string` is also accepted.", )), ), + ( + "old_string".to_string(), + JsonSchema::string(Some("Alias for `oldString`.")), + ), ( "newString".to_string(), JsonSchema::string(Some( - "The text to replace oldString with. May be empty to delete text.", + "The text to replace oldString with. May be empty to delete text. Preferred field name; `new_string` is also accepted.", )), ), + ( + "new_string".to_string(), + JsonSchema::string(Some("Alias for `newString`.")), + ), ( "replaceAll".to_string(), JsonSchema::boolean(Some( - "Replace every occurrence of oldString. Defaults to false.", + "Replace every occurrence of oldString. Defaults to false. Preferred field name; `replace_all` is also accepted.", )), ), + ( + "replace_all".to_string(), + JsonSchema::boolean(Some("Alias for `replaceAll`.")), + ), ]), Some(vec![ "filePath".to_string(), @@ -92,16 +115,13 @@ impl ToolHandler for EditHandler { input: serde_json::Value, _progress: Option, ) -> Result { - let path_str = input["filePath"] - .as_str() + let path_str = string_field(&input, &["filePath", "path", "file_path"]) .ok_or_else(|| ToolCallError::InvalidInput("missing 'filePath' field".into()))?; - let old_string = input["oldString"] - .as_str() + let old_string = string_field(&input, &["oldString", "old_string"]) .ok_or_else(|| ToolCallError::InvalidInput("missing 'oldString' field".into()))?; - let new_string = input["newString"] - .as_str() + let new_string = string_field(&input, &["newString", "new_string"]) .ok_or_else(|| ToolCallError::InvalidInput("missing 'newString' field".into()))?; - let replace_all = input["replaceAll"].as_bool().unwrap_or(false); + let replace_all = bool_field(&input, &["replaceAll", "replace_all"]).unwrap_or(false); if old_string.is_empty() { return Ok(ToolResult::error( @@ -145,12 +165,55 @@ impl ToolHandler for EditHandler { )); } + if let Some(ledger) = ctx.file_read_ledger.as_ref() { + match ledger.require_fresh(&path, &previous, file_mtime(&path)) { + Ok(()) => {} + Err(FileReadFreshnessError::NotRead) => { + return Ok(ToolResult::error( + ToolResultContent::Text(format!( + "You must Read the full file before using edit on {}. Read the file without offset/limit, then retry with the exact oldString from that output.", + path.display() + )), + "Read required", + ToolCallError::ExecutionFailed(format!( + "must read file before editing: {}", + path.display() + )), + )); + } + Err(FileReadFreshnessError::Stale) => { + return Ok(ToolResult::error( + ToolResultContent::Text(format!( + "The file {} changed since it was last read. Read the full file again, then retry with an updated oldString.", + path.display() + )), + "Stale read", + ToolCallError::ExecutionFailed(format!( + "file changed since it was last read: {}", + path.display() + )), + )); + } + } + } + let match_count = previous.matches(old_string).count(); if match_count == 0 { return Ok(ToolResult::error( - ToolResultContent::Text("oldString not found in content".into()), + ToolResultContent::Text(old_string_not_found_message(old_string)), "No match", - ToolCallError::ExecutionFailed("oldString not found".into()), + ToolCallError::ExecutionFailed(old_string_not_found_error(old_string)), + )); + } + if match_count > 1 && !replace_all { + return Ok(ToolResult::error( + ToolResultContent::Text( + "Found multiple matches for oldString. Provide more surrounding lines in oldString to identify the correct match, or set replaceAll to true if every match should change.".into(), + ), + "Ambiguous match", + ToolCallError::ExecutionFailed( + "found multiple matches for oldString".into(), + ), )); } let content = if replace_all { @@ -184,6 +247,40 @@ fn resolve_path(cwd: &Path, path: &str) -> PathBuf { if p.is_absolute() { p } else { cwd.join(p) } } +fn string_field<'a>(input: &'a serde_json::Value, keys: &[&str]) -> Option<&'a str> { + keys.iter() + .find_map(|key| input.get(*key).and_then(serde_json::Value::as_str)) +} + +fn bool_field(input: &serde_json::Value, keys: &[&str]) -> Option { + keys.iter() + .find_map(|key| input.get(*key).and_then(serde_json::Value::as_bool)) +} + +fn old_string_not_found_error(old_string: &str) -> String { + if looks_like_numbered_read_line(old_string) { + "oldString not found; it appears to include a Read tool line number prefix".into() + } else { + "oldString not found".into() + } +} + +fn old_string_not_found_message(old_string: &str) -> String { + if looks_like_numbered_read_line(old_string) { + "oldString not found in content. It looks like oldString includes a Read tool line number prefix such as `12: `. Remove the line number prefix and retry with only the actual file text.".into() + } else { + "oldString not found in content. Read the full file again and copy the exact text, including whitespace, tabs, and newlines. Do not include Read tool line number prefixes like `12: `.".into() + } +} + +fn looks_like_numbered_read_line(old_string: &str) -> bool { + let digits = old_string + .bytes() + .take_while(|byte| byte.is_ascii_digit()) + .count(); + digits > 0 && old_string[digits..].starts_with(": ") +} + async fn read_text_file(ctx: &ToolContext, path: &Path) -> Result, ToolCallError> { if let Some(client_filesystem) = ctx.client_filesystem.clone() { match client_filesystem @@ -232,7 +329,10 @@ async fn write_text_file( ) .await? { - ClientTextFileWrite::Written => return Ok(()), + ClientTextFileWrite::Written => { + record_write_in_ledger(ctx, path, content); + return Ok(()); + } ClientTextFileWrite::Unsupported => {} } } @@ -244,7 +344,15 @@ async fn write_text_file( } tokio::fs::write(path, content) .await - .map_err(|e| ToolCallError::ExecutionFailed(format!("failed to write file: {e}"))) + .map_err(|e| ToolCallError::ExecutionFailed(format!("failed to write file: {e}")))?; + record_write_in_ledger(ctx, path, content); + Ok(()) +} + +fn record_write_in_ledger(ctx: &ToolContext, path: &Path, content: &str) { + if let Some(ledger) = ctx.file_read_ledger.as_ref() { + ledger.record_write(path, content, file_mtime(path)); + } } #[cfg(test)] @@ -348,4 +456,159 @@ mod tests { )); assert_eq!(std::fs::read_to_string(&path).expect("read"), "1 2 three"); } + + #[tokio::test] + async fn edit_accepts_path_aliases() { + let root = tempfile::tempdir().expect("tempdir"); + let path = root.path().join("a.txt"); + std::fs::write(&path, "hello").expect("write"); + let ledger = Arc::new(FileReadLedger::new()); + ledger.record_full_read(&path, "hello", file_mtime(&path)); + + let result = EditHandler::new() + .handle( + ctx(root.path(), ledger), + serde_json::json!({ + "path": path, + "old_string": "hello", + "new_string": "world", + }), + None, + ) + .await + .expect("handle"); + + assert!(matches!( + result.structured_status, + ToolTerminalStatus::Completed + )); + assert_eq!(std::fs::read_to_string(&path).expect("read"), "world"); + } + + #[tokio::test] + async fn edit_rejects_when_file_was_not_read_first() { + let root = tempfile::tempdir().expect("tempdir"); + let path = root.path().join("a.txt"); + std::fs::write(&path, "hello").expect("write"); + + let result = EditHandler::new() + .handle( + ctx(root.path(), Arc::new(FileReadLedger::new())), + serde_json::json!({ + "filePath": path, + "oldString": "hello", + "newString": "world", + }), + None, + ) + .await + .expect("handle"); + + match &result.structured_status { + ToolTerminalStatus::Failed(ToolCallError::ExecutionFailed(message)) => { + assert!( + message.contains("must read"), + "unexpected message: {message}" + ); + } + other => panic!("expected failed execution status, got {other:?}"), + } + } + + #[tokio::test] + async fn edit_rejects_stale_read_content() { + let root = tempfile::tempdir().expect("tempdir"); + let path = root.path().join("a.txt"); + std::fs::write(&path, "hello").expect("write"); + let ledger = Arc::new(FileReadLedger::new()); + ledger.record_full_read(&path, "hello", file_mtime(&path)); + std::fs::write(&path, "hello there").expect("rewrite"); + + let result = EditHandler::new() + .handle( + ctx(root.path(), ledger), + serde_json::json!({ + "filePath": path, + "oldString": "hello", + "newString": "world", + }), + None, + ) + .await + .expect("handle"); + + match &result.structured_status { + ToolTerminalStatus::Failed(ToolCallError::ExecutionFailed(message)) => { + assert!( + message.contains("changed since it was last read"), + "unexpected message: {message}" + ); + } + other => panic!("expected failed execution status, got {other:?}"), + } + } + + #[tokio::test] + async fn edit_rejects_ambiguous_old_string_without_replace_all() { + let root = tempfile::tempdir().expect("tempdir"); + let path = root.path().join("a.txt"); + std::fs::write(&path, "dup dup").expect("write"); + let ledger = Arc::new(FileReadLedger::new()); + ledger.record_full_read(&path, "dup dup", file_mtime(&path)); + + let result = EditHandler::new() + .handle( + ctx(root.path(), ledger), + serde_json::json!({ + "filePath": path, + "oldString": "dup", + "newString": "value", + }), + None, + ) + .await + .expect("handle"); + + match &result.structured_status { + ToolTerminalStatus::Failed(ToolCallError::ExecutionFailed(message)) => { + assert!( + message.contains("multiple matches"), + "unexpected message: {message}" + ); + } + other => panic!("expected failed execution status, got {other:?}"), + } + } + + #[tokio::test] + async fn edit_old_string_not_found_message_mentions_line_numbers() { + let root = tempfile::tempdir().expect("tempdir"); + let path = root.path().join("a.txt"); + std::fs::write(&path, "alpha\nbeta\n").expect("write"); + let ledger = Arc::new(FileReadLedger::new()); + ledger.record_full_read(&path, "alpha\nbeta\n", file_mtime(&path)); + + let result = EditHandler::new() + .handle( + ctx(root.path(), ledger), + serde_json::json!({ + "filePath": path, + "oldString": "1: alpha", + "newString": "gamma", + }), + None, + ) + .await + .expect("handle"); + + match &result.structured_status { + ToolTerminalStatus::Failed(ToolCallError::ExecutionFailed(message)) => { + assert!( + message.contains("line number"), + "unexpected message: {message}" + ); + } + other => panic!("expected failed execution status, got {other:?}"), + } + } } diff --git a/crates/core/src/tools/registry_plan.rs b/crates/core/src/tools/registry_plan.rs index 336d18f4..74d73279 100644 --- a/crates/core/src/tools/registry_plan.rs +++ b/crates/core/src/tools/registry_plan.rs @@ -226,26 +226,48 @@ fn edit_schema() -> JsonSchema { BTreeMap::from([ ( "filePath".to_string(), - JsonSchema::string(Some("The absolute path to the file to modify")), + JsonSchema::string(Some( + "The absolute path to the file to modify. Preferred field name; `path` and `file_path` are also accepted.", + )), + ), + ( + "path".to_string(), + JsonSchema::string(Some("Alias for `filePath`.")), + ), + ( + "file_path".to_string(), + JsonSchema::string(Some("Alias for `filePath`.")), ), ( "oldString".to_string(), JsonSchema::string(Some( - "The exact text to replace. Must be non-empty and unique unless replaceAll is true.", + "The exact text to replace. Must be non-empty and unique unless replaceAll is true. Preferred field name; `old_string` is also accepted.", )), ), + ( + "old_string".to_string(), + JsonSchema::string(Some("Alias for `oldString`.")), + ), ( "newString".to_string(), JsonSchema::string(Some( - "The text to replace oldString with. May be empty to delete text.", + "The text to replace oldString with. May be empty to delete text. Preferred field name; `new_string` is also accepted.", )), ), + ( + "new_string".to_string(), + JsonSchema::string(Some("Alias for `newString`.")), + ), ( "replaceAll".to_string(), JsonSchema::boolean(Some( - "Replace every occurrence of oldString. Defaults to false.", + "Replace every occurrence of oldString. Defaults to false. Preferred field name; `replace_all` is also accepted.", )), ), + ( + "replace_all".to_string(), + JsonSchema::boolean(Some("Alias for `replaceAll`.")), + ), ]), Some(vec![ "filePath".to_string(), From aa3ef2ab4e461aa4c1208c7dd36a100be4937e8e Mon Sep 17 00:00:00 2001 From: wangtsiao Date: Tue, 28 Jul 2026 18:04:03 +0800 Subject: [PATCH 4/4] fix: better tool name at TUI --- crates/tools/src/tool_summary.rs | 15 ++++-- crates/tui/src/chatwidget/worker_events.rs | 49 ++++++++++++++--- crates/tui/src/chatwidget_tests.rs | 63 ++++++++++++++++++++++ 3 files changed, 115 insertions(+), 12 deletions(-) diff --git a/crates/tools/src/tool_summary.rs b/crates/tools/src/tool_summary.rs index 7d36c062..0569d8be 100644 --- a/crates/tools/src/tool_summary.rs +++ b/crates/tools/src/tool_summary.rs @@ -66,7 +66,7 @@ pub fn tool_summary(name: &str, input: &serde_json::Value, cwd: &Path) -> String match name { "bash" | "shell_command" => { let cmd = string_arg_any(input, &["command", "cmd"], ""); - format!("{name}: {cmd}") + format!("Shell: {cmd}") } "exec_command" => { let cmd = string_arg_any(input, &["cmd", "command"], ""); @@ -143,7 +143,7 @@ pub fn tool_summary(name: &str, input: &serde_json::Value, cwd: &Path) -> String let rel = make_relative(cwd, path); format!("{name}: {pattern} in {rel}") } - "apply_patch" => "apply_patch".to_string(), + "apply_patch" => "Patch".to_string(), "webfetch" | "web_fetch" | "web-fetch" | "fetch_url" | "fetch-url" => { let url = string_arg(input, "url", ""); format!("web_fetch: {url}") @@ -205,14 +205,21 @@ mod tests { fn bash_summary() { let input = json!({"cmd": "echo hello"}); let s = tool_summary("bash", &input, &cwd()); - assert_eq!(s, "bash: echo hello"); + assert_eq!(s, "Shell: echo hello"); } #[test] fn shell_command_summary() { let input = json!({"command": "npm run build"}); let s = tool_summary("shell_command", &input, &cwd()); - assert_eq!(s, "shell_command: npm run build"); + assert_eq!(s, "Shell: npm run build"); + } + + #[test] + fn apply_patch_summary() { + let input = json!({}); + let s = tool_summary("apply_patch", &input, &cwd()); + assert_eq!(s, "Patch"); } #[test] diff --git a/crates/tui/src/chatwidget/worker_events.rs b/crates/tui/src/chatwidget/worker_events.rs index d42d2023..38ae0b42 100644 --- a/crates/tui/src/chatwidget/worker_events.rs +++ b/crates/tui/src/chatwidget/worker_events.rs @@ -8,6 +8,7 @@ use std::time::Instant; use devo_protocol::ProviderRetryPhase; use devo_protocol::parse_command::ParsedCommand; use devo_protocol::protocol::ExecCommandSource; +use devo_protocol::protocol::FileChange; use ratatui::text::Line; use crate::bottom_pane::ApprovalOverlay; @@ -37,6 +38,33 @@ fn format_retry_status_message(attempt: usize, backoff_ms: u64) -> String { format!("Retrying provider request in {seconds:.1}s (attempt {attempt})") } +fn normalize_approval_action_summary(action_summary: String) -> String { + if action_summary == "apply_patch" { + return "Patch".to_string(); + } + if let Some(command) = action_summary.strip_prefix("shell_command: ") { + return format!("Shell: {command}"); + } + if let Some(command) = action_summary.strip_prefix("bash: ") { + return format!("Shell: {command}"); + } + action_summary +} + +fn has_visible_file_changes( + changes: &std::collections::HashMap, +) -> bool { + changes.values().any(|change| match change { + FileChange::Add { content } | FileChange::Delete { content } => !content.trim().is_empty(), + FileChange::Update { + unified_diff, + old_text, + new_text, + move_path, + } => !unified_diff.trim().is_empty() || old_text != new_text || move_path.is_some(), + }) +} + impl ChatWidget { fn start_command_execution_cell( &mut self, @@ -759,13 +787,15 @@ impl ChatWidget { self.active_tool_calls.remove(&tool_use_id); self.pending_tool_calls .retain(|pending| pending.tool_use_id != tool_use_id); - self.add_to_history(FileChangeToolIoCell::new( - Some(Self::ran_tool_line(&tool_name)), - tool_name, - input, - changes, - self.session.cwd.clone(), - )); + if has_visible_file_changes(&changes) { + self.add_to_history(FileChangeToolIoCell::new( + Some(Self::ran_tool_line(&tool_name)), + tool_name, + input, + changes, + self.session.cwd.clone(), + )); + } self.set_status_message("Patch applied"); } WorkerEvent::PatchApplied { @@ -775,7 +805,9 @@ impl ChatWidget { self.active_tool_calls.remove(&tool_use_id); self.pending_tool_calls .retain(|pending| pending.tool_use_id != tool_use_id); - self.add_to_history(history_cell::new_patch_event(changes, &self.session.cwd)); + if has_visible_file_changes(&changes) { + self.add_to_history(history_cell::new_patch_event(changes, &self.session.cwd)); + } self.set_status_message("Patch applied"); } WorkerEvent::ApprovalRequest { @@ -793,6 +825,7 @@ impl ChatWidget { command_prefix, } => { self.commit_active_streams(DotStatus::Completed); + let action_summary = normalize_approval_action_summary(action_summary); self.pending_approval = Some(PendingApprovalRequest { session_id, turn_id, diff --git a/crates/tui/src/chatwidget_tests.rs b/crates/tui/src/chatwidget_tests.rs index 76d7b0ac..1a039692 100644 --- a/crates/tui/src/chatwidget_tests.rs +++ b/crates/tui/src/chatwidget_tests.rs @@ -1076,6 +1076,37 @@ fn approval_request_does_not_duplicate_already_committed_assistant_text() { ); } +#[test] +fn approval_request_apply_patch_uses_friendly_label() { + let model = Model { + slug: "test-model".to_string(), + display_name: "Test Model".to_string(), + ..Model::default() + }; + let (mut widget, _app_event_rx) = widget_with_model(model, PathBuf::from(".")); + let session_id = SessionId::new(); + let turn_id = TurnId::new(); + + widget.handle_worker_event(crate::events::WorkerEvent::ApprovalRequest { + session_id, + turn_id, + approval_id: "approval-call-friendly".to_string(), + action_summary: "apply_patch".to_string(), + justification: "Tool execution requires approval.".to_string(), + resource: Some("FileWrite".to_string()), + available_scopes: vec!["once".to_string()], + path: Some("src/main.rs".to_string()), + host: None, + target: None, + command_pattern: None, + command_prefix: None, + }); + + let rendered = rendered_rows(&widget, 80, 24).join("\n"); + assert!(rendered.contains("Permission approval required")); + assert!(rendered.contains("Patch")); +} + #[test] fn approval_request_bottom_pane_menu_denies_with_n_shortcut() { let model = Model { @@ -9518,6 +9549,38 @@ fn patch_applied_event_with_diff_only_reports_non_zero_counts() { ); } +#[test] +fn patch_applied_event_with_empty_update_is_not_rendered() { + let model = Model { + slug: "test-model".to_string(), + display_name: "Test Model".to_string(), + ..Model::default() + }; + let (mut widget, _app_event_rx) = widget_with_model(model, PathBuf::from(".")); + + let mut changes = std::collections::HashMap::new(); + changes.insert( + PathBuf::from("foo.txt"), + devo_protocol::protocol::FileChange::Update { + unified_diff: String::new(), + old_text: None, + new_text: None, + move_path: None, + }, + ); + + widget.handle_worker_event(crate::events::WorkerEvent::PatchApplied { + tool_use_id: "tool-1".to_string(), + changes, + }); + + let blob = scrollback_plain_lines(&widget.drain_scrollback_lines(80)).join("\n"); + assert!( + !blob.contains("Edited"), + "empty patch summary should not be rendered:\n{blob}" + ); +} + #[test] fn session_switch_without_rich_edited_metadata_degrades_to_tool_result_path() { let cwd = std::env::current_dir().expect("current directory is available");