diff --git a/rust/crates/api/src/error.rs b/rust/crates/api/src/error.rs index e8ec73a4c5..d4dc0daa94 100644 --- a/rust/crates/api/src/error.rs +++ b/rust/crates/api/src/error.rs @@ -52,19 +52,13 @@ pub enum ApiError { body_snippet: String, source: serde_json::Error, }, - Api { - status: reqwest::StatusCode, - error_type: Option, - message: Option, - request_id: Option, - body: String, - retryable: bool, - /// Suggested user action based on error type (e.g., "Reduce prompt size" for 413) - suggested_action: Option, - /// Parsed Retry-After header value (seconds) for 429 responses. - /// When present, overrides the exponential backoff delay. - retry_after: Option, - }, + /// A provider returned an error response. + /// + /// Boxed: the payload is ~139 bytes, and `ApiError` is the `Err` half of nearly every + /// `Result` in this crate, so inlining it inflates the size of every one of those returns + /// (clippy::result_large_err). Boxing keeps `ApiError` small and means adding a field here + /// cannot silently re-inflate every signature in the crate. + Api(Box), RetriesExhausted { attempts: u32, last_error: Box, @@ -81,7 +75,38 @@ pub enum ApiError { }, } +/// Payload of [`ApiError::Api`], held behind a `Box` so the enum stays small. +#[derive(Debug)] +pub struct ApiErrorDetails { + pub status: reqwest::StatusCode, + pub error_type: Option, + pub message: Option, + pub request_id: Option, + pub body: String, + pub retryable: bool, + /// Suggested user action based on error type (e.g., "Reduce prompt size" for 413) + pub suggested_action: Option, + /// Parsed Retry-After header value (seconds) for 429 responses. + /// When present, overrides the exponential backoff delay. + pub retry_after: Option, +} + impl ApiError { + /// Convenience constructor so callers do not have to spell out the `Box` at every site. + #[must_use] + pub fn api(details: ApiErrorDetails) -> Self { + Self::Api(Box::new(details)) + } + + /// Borrow the provider-error payload, if this is one. + #[must_use] + pub fn api_details(&self) -> Option<&ApiErrorDetails> { + match self { + Self::Api(details) => Some(details), + _ => None, + } + } + #[must_use] pub const fn missing_credentials( provider: &'static str, @@ -137,7 +162,7 @@ impl ApiError { /// over the computed backoff delay when it exists. pub fn retry_after(&self) -> Option { match self { - Self::Api { retry_after, .. } => *retry_after, + Self::Api(details) => details.retry_after, Self::RetriesExhausted { last_error, .. } => last_error.retry_after(), _ => None, } @@ -146,7 +171,7 @@ impl ApiError { pub fn is_retryable(&self) -> bool { match self { Self::Http(error) => error.is_connect() || error.is_timeout() || error.is_request(), - Self::Api { retryable, .. } => *retryable, + Self::Api(details) => details.retryable, Self::RetriesExhausted { last_error, .. } => last_error.is_retryable(), Self::MissingCredentials { .. } | Self::ContextWindowExceeded { .. } @@ -164,7 +189,7 @@ impl ApiError { #[must_use] pub fn request_id(&self) -> Option<&str> { match self { - Self::Api { request_id, .. } => request_id.as_deref(), + Self::Api(details) => details.request_id.as_deref(), Self::RetriesExhausted { last_error, .. } => last_error.request_id(), Self::MissingCredentials { .. } | Self::ContextWindowExceeded { .. } @@ -191,12 +216,12 @@ impl ApiError { Self::MissingCredentials { .. } | Self::ExpiredOAuthToken | Self::Auth(_) => { "provider_auth" } - Self::Api { status, .. } if matches!(status.as_u16(), 401 | 403) => "provider_auth", + Self::Api(details) if matches!(details.status.as_u16(), 401 | 403) => "provider_auth", Self::ContextWindowExceeded { .. } => "context_window", - Self::Api { .. } if self.is_context_window_failure() => "context_window", - Self::Api { status, .. } if status.as_u16() == 429 => "provider_rate_limit", - Self::Api { .. } if self.is_generic_fatal_wrapper() => "provider_internal", - Self::Api { .. } => "provider_error", + Self::Api(_) if self.is_context_window_failure() => "context_window", + Self::Api(details) if details.status.as_u16() == 429 => "provider_rate_limit", + Self::Api(_) if self.is_generic_fatal_wrapper() => "provider_internal", + Self::Api(_) => "provider_error", Self::Http(_) | Self::InvalidSseFrame(_) | Self::BackoffOverflow { .. } => { "provider_transport" } @@ -208,11 +233,12 @@ impl ApiError { #[must_use] pub fn is_generic_fatal_wrapper(&self) -> bool { match self { - Self::Api { message, body, .. } => { - message + Self::Api(details) => { + details + .message .as_deref() .is_some_and(looks_like_generic_fatal_wrapper) - || looks_like_generic_fatal_wrapper(body) + || looks_like_generic_fatal_wrapper(&details.body) } Self::RetriesExhausted { last_error, .. } => last_error.is_generic_fatal_wrapper(), Self::MissingCredentials { .. } @@ -229,21 +255,36 @@ impl ApiError { } } + /// True for failures whose own text cannot say what went wrong: transport, decode, and frame + /// errors. + /// + /// These matter because a server that answers an oversized request with a non-SSE HTTP 500 + /// body lands here rather than in [`Self::is_context_window_failure`] — reqwest reports + /// "error decoding response body" and the real cause is lost. Callers that know how large the + /// request was can combine the two signals; callers that do not must treat this as unknown, + /// never as an overflow. + #[must_use] + pub fn is_ambiguous_transport_failure(&self) -> bool { + match self { + Self::Http(_) | Self::Json { .. } | Self::InvalidSseFrame(_) => true, + Self::RetriesExhausted { last_error, .. } => { + last_error.is_ambiguous_transport_failure() + } + _ => false, + } + } + #[must_use] pub fn is_context_window_failure(&self) -> bool { match self { Self::ContextWindowExceeded { .. } => true, - Self::Api { - status, - message, - body, - .. - } => { - matches!(status.as_u16(), 400 | 413 | 422) - && (message + Self::Api(details) => { + matches!(details.status.as_u16(), 400 | 413 | 422) + && (details + .message .as_deref() .is_some_and(looks_like_context_window_error) - || looks_like_context_window_error(body)) + || looks_like_context_window_error(&details.body)) } Self::RetriesExhausted { last_error, .. } => last_error.is_context_window_failure(), Self::MissingCredentials { .. } @@ -327,14 +368,14 @@ impl Display for ApiError { "failed to parse {provider} response for model {model}: {source}; first 200 chars of body: {body_snippet}" ), // #28: enhance 401/403 errors with actionable auth guidance - Self::Api { - status, - error_type, - message, - request_id, - body, - .. - } if matches!(status.as_u16(), 401 | 403) => { + Self::Api(details) if matches!(details.status.as_u16(), 401 | 403) => { + let (status, error_type, message, request_id, body) = ( + &details.status, + &details.error_type, + &details.message, + &details.request_id, + &details.body, + ); if let (Some(error_type), Some(message)) = (error_type, message) { write!(f, "api returned {status} ({error_type})")?; if let Some(request_id) = request_id { @@ -356,14 +397,14 @@ impl Display for ApiError { Run `claw doctor` to verify your credential configuration." ) } - Self::Api { - status, - error_type, - message, - request_id, - body, - .. - } => { + Self::Api(details) => { + let (status, error_type, message, request_id, body) = ( + &details.status, + &details.error_type, + &details.message, + &details.request_id, + &details.body, + ); if let (Some(error_type), Some(message)) = (error_type, message) { write!(f, "api returned {status} ({error_type})")?; if let Some(request_id) = request_id { @@ -469,6 +510,7 @@ fn truncate_body_snippet(body: &str, max_chars: usize) -> String { #[cfg(test)] mod tests { + use super::ApiErrorDetails; use super::{truncate_body_snippet, ApiError}; #[test] @@ -533,7 +575,7 @@ mod tests { #[test] fn detects_generic_fatal_wrapper_and_classifies_it_as_provider_internal() { - let error = ApiError::Api { + let error = ApiError::Api(Box::new(ApiErrorDetails { status: reqwest::StatusCode::INTERNAL_SERVER_ERROR, error_type: Some("api_error".to_string()), message: Some( @@ -545,7 +587,7 @@ mod tests { retryable: true, suggested_action: None, retry_after: None, - }; + })); assert!(error.is_generic_fatal_wrapper()); assert_eq!(error.safe_failure_class(), "provider_internal"); @@ -557,7 +599,7 @@ mod tests { fn retries_exhausted_preserves_nested_request_id_and_failure_class() { let error = ApiError::RetriesExhausted { attempts: 3, - last_error: Box::new(ApiError::Api { + last_error: Box::new(ApiError::Api(Box::new(ApiErrorDetails { status: reqwest::StatusCode::BAD_GATEWAY, error_type: Some("api_error".to_string()), message: Some( @@ -569,7 +611,7 @@ mod tests { retryable: true, suggested_action: None, retry_after: None, - }), + }))), }; assert!(error.is_generic_fatal_wrapper()); @@ -579,7 +621,7 @@ mod tests { #[test] fn classifies_provider_context_window_errors() { - let error = ApiError::Api { + let error = ApiError::Api(Box::new(ApiErrorDetails { status: reqwest::StatusCode::BAD_REQUEST, error_type: Some("invalid_request_error".to_string()), message: Some( @@ -591,7 +633,7 @@ mod tests { retryable: false, suggested_action: None, retry_after: None, - }; + })); assert!(error.is_context_window_failure()); assert_eq!(error.safe_failure_class(), "context_window"); @@ -600,7 +642,7 @@ mod tests { #[test] fn classifies_openai_configured_limit_errors_as_context_window_failures() { - let error = ApiError::Api { + let error = ApiError::Api(Box::new(ApiErrorDetails { status: reqwest::StatusCode::BAD_REQUEST, error_type: Some("invalid_request_error".to_string()), message: Some( @@ -612,7 +654,7 @@ mod tests { retryable: false, suggested_action: None, retry_after: None, - }; + })); assert!(error.is_context_window_failure()); assert_eq!(error.safe_failure_class(), "context_window"); diff --git a/rust/crates/api/src/lib.rs b/rust/crates/api/src/lib.rs index e96e92f830..599789c448 100644 --- a/rust/crates/api/src/lib.rs +++ b/rust/crates/api/src/lib.rs @@ -10,7 +10,7 @@ pub use client::{ oauth_token_is_expired, read_base_url, read_xai_base_url, resolve_saved_oauth_token, resolve_startup_auth_source, MessageStream, OAuthTokenSet, ProviderClient, }; -pub use error::ApiError; +pub use error::{ApiError, ApiErrorDetails}; pub use http_client::{ build_http_client, build_http_client_or_default, build_http_client_with, build_http_client_with_opts, ProxyConfig, TimeoutConfig, @@ -27,9 +27,10 @@ pub use providers::openai_compat::{ OpenAiCompatConfig, }; pub use providers::{ - detect_provider_kind, max_tokens_for_model, max_tokens_for_model_with_override, - model_family_identity_for, model_family_identity_for_kind, provider_diagnostics_for_model, - resolve_model_alias, ProviderDiagnostics, ProviderKind, + context_window_override, detect_provider_kind, estimate_message_request_input_tokens, + max_tokens_for_model, max_tokens_for_model_with_override, model_family_identity_for, + model_family_identity_for_kind, model_token_limit, provider_diagnostics_for_model, + resolve_model_alias, ModelTokenLimit, ProviderDiagnostics, ProviderKind, }; pub use sse::{parse_frame, SseParser}; pub use types::{ diff --git a/rust/crates/api/src/providers/anthropic.rs b/rust/crates/api/src/providers/anthropic.rs index 430b3eff6d..be6eb3a465 100644 --- a/rust/crates/api/src/providers/anthropic.rs +++ b/rust/crates/api/src/providers/anthropic.rs @@ -12,7 +12,7 @@ use serde::Deserialize; use serde_json::{Map, Value}; use telemetry::{AnalyticsEvent, AnthropicRequestProfile, ClientIdentity, SessionTracer}; -use crate::error::ApiError; +use crate::error::{ApiError, ApiErrorDetails}; use crate::http_client::build_http_client_or_default; use crate::prompt_cache::{PromptCache, PromptCacheRecord, PromptCacheStats}; @@ -892,7 +892,7 @@ async fn expect_success(response: reqwest::Response) -> Result Result bool { const SK_ANT_BEARER_HINT: &str = "sk-ant-* keys go in ANTHROPIC_API_KEY (x-api-key header), not ANTHROPIC_AUTH_TOKEN (Bearer header). Move your key to ANTHROPIC_API_KEY."; fn enrich_bearer_auth_error(error: ApiError, auth: &AuthSource) -> ApiError { - let ApiError::Api { - status, - error_type, - message, - request_id, - body, - retryable, - suggested_action, - retry_after, - .. - } = error - else { + let ApiError::Api(mut details) = error else { return error; }; - if status.as_u16() != 401 { - return ApiError::Api { - status, - error_type, - message, - request_id, - body, - retryable, - suggested_action, - retry_after, - }; + if details.status.as_u16() != 401 { + return ApiError::Api(details); } let Some(bearer_token) = auth.bearer_token() else { - return ApiError::Api { - status, - error_type, - message, - request_id, - body, - retryable, - suggested_action, - retry_after, - }; + return ApiError::Api(details); }; if !bearer_token.starts_with("sk-ant-") { - return ApiError::Api { - status, - error_type, - message, - request_id, - body, - retryable, - suggested_action, - retry_after, - }; + return ApiError::Api(details); } // Only append the hint when the AuthSource is pure BearerToken. If both // api_key and bearer_token are present (`ApiKeyAndBearer`), the x-api-key // header is already being sent alongside the Bearer header and the 401 // is coming from a different cause — adding the hint would be misleading. if auth.api_key().is_some() { - return ApiError::Api { - status, - error_type, - message, - request_id, - body, - retryable, - suggested_action, - retry_after, - }; - } - let enriched_message = match message { - Some(existing) => Some(format!("{existing} — hint: {SK_ANT_BEARER_HINT}")), - None => Some(format!("hint: {SK_ANT_BEARER_HINT}")), - }; - ApiError::Api { - status, - error_type, - message: enriched_message, - request_id, - body, - retryable, - suggested_action, - retry_after, + return ApiError::Api(details); } + details.message = Some(match details.message.take() { + Some(existing) => format!("{existing} — hint: {SK_ANT_BEARER_HINT}"), + None => format!("hint: {SK_ANT_BEARER_HINT}"), + }); + ApiError::Api(details) } fn anthropic_wire_model(model: &str) -> &str { @@ -1083,6 +1027,7 @@ struct AnthropicErrorBody { #[cfg(test)] mod tests { use super::{ALT_REQUEST_ID_HEADER, REQUEST_ID_HEADER}; + use crate::error::ApiErrorDetails; use std::io::{Read, Write}; use std::net::TcpListener; use std::sync::{Mutex, OnceLock}; @@ -1647,7 +1592,7 @@ mod tests { fn enrich_bearer_auth_error_appends_sk_ant_hint_on_401_with_pure_bearer_token() { // given let auth = AuthSource::BearerToken("sk-ant-api03-deadbeef".to_string()); - let error = crate::error::ApiError::Api { + let error = crate::error::ApiError::Api(Box::new(ApiErrorDetails { status: reqwest::StatusCode::UNAUTHORIZED, error_type: Some("authentication_error".to_string()), message: Some("Invalid bearer token".to_string()), @@ -1656,7 +1601,7 @@ mod tests { retryable: false, suggested_action: None, retry_after: None, - }; + })); // when let enriched = super::enrich_bearer_auth_error(error, &auth); @@ -1678,8 +1623,8 @@ mod tests { "request id should still flow through the enriched error: {rendered}" ); match enriched { - crate::error::ApiError::Api { status, .. } => { - assert_eq!(status, reqwest::StatusCode::UNAUTHORIZED); + crate::error::ApiError::Api(details) => { + assert_eq!(details.status, reqwest::StatusCode::UNAUTHORIZED); } other => panic!("expected Api variant, got {other:?}"), } @@ -1689,7 +1634,7 @@ mod tests { fn enrich_bearer_auth_error_leaves_non_401_errors_unchanged() { // given let auth = AuthSource::BearerToken("sk-ant-api03-deadbeef".to_string()); - let error = crate::error::ApiError::Api { + let error = crate::error::ApiError::Api(Box::new(ApiErrorDetails { status: reqwest::StatusCode::INTERNAL_SERVER_ERROR, error_type: Some("api_error".to_string()), message: Some("internal server error".to_string()), @@ -1698,7 +1643,7 @@ mod tests { retryable: true, suggested_action: None, retry_after: None, - }; + })); // when let enriched = super::enrich_bearer_auth_error(error, &auth); @@ -1719,7 +1664,7 @@ mod tests { fn enrich_bearer_auth_error_ignores_401_when_bearer_token_is_not_sk_ant() { // given let auth = AuthSource::BearerToken("oauth-access-token-opaque".to_string()); - let error = crate::error::ApiError::Api { + let error = crate::error::ApiError::Api(Box::new(ApiErrorDetails { status: reqwest::StatusCode::UNAUTHORIZED, error_type: Some("authentication_error".to_string()), message: Some("Invalid bearer token".to_string()), @@ -1728,7 +1673,7 @@ mod tests { retryable: false, suggested_action: None, retry_after: None, - }; + })); // when let enriched = super::enrich_bearer_auth_error(error, &auth); @@ -1748,7 +1693,7 @@ mod tests { api_key: "sk-ant-api03-legitimate".to_string(), bearer_token: "sk-ant-api03-deadbeef".to_string(), }; - let error = crate::error::ApiError::Api { + let error = crate::error::ApiError::Api(Box::new(ApiErrorDetails { status: reqwest::StatusCode::UNAUTHORIZED, error_type: Some("authentication_error".to_string()), message: Some("Invalid bearer token".to_string()), @@ -1757,7 +1702,7 @@ mod tests { retryable: false, suggested_action: None, retry_after: None, - }; + })); // when let enriched = super::enrich_bearer_auth_error(error, &auth); @@ -1774,7 +1719,7 @@ mod tests { fn enrich_bearer_auth_error_ignores_401_when_auth_source_has_no_bearer() { // given let auth = AuthSource::ApiKey("sk-ant-api03-legitimate".to_string()); - let error = crate::error::ApiError::Api { + let error = crate::error::ApiError::Api(Box::new(ApiErrorDetails { status: reqwest::StatusCode::UNAUTHORIZED, error_type: Some("authentication_error".to_string()), message: Some("Invalid x-api-key".to_string()), @@ -1783,7 +1728,7 @@ mod tests { retryable: false, suggested_action: None, retry_after: None, - }; + })); // when let enriched = super::enrich_bearer_auth_error(error, &auth); diff --git a/rust/crates/api/src/providers/mod.rs b/rust/crates/api/src/providers/mod.rs index 2524e5520a..21da2d28ce 100644 --- a/rust/crates/api/src/providers/mod.rs +++ b/rust/crates/api/src/providers/mod.rs @@ -609,16 +609,43 @@ fn web_passthrough_diagnostic( } } +/// Overrides the context window every sizing guard is computed against. +/// +/// The model *name* is not a reliable statement of the window when the endpoint is a local +/// server: the harness sends its compiled-in default name (a Claude alias) while the backend +/// serves whatever `-c` it was started with. Every guard downstream of [`model_token_limit`] +/// then sizes itself against the name's window instead of the real one, which makes the +/// preflights unable to fire rather than merely inaccurate. This variable is the operator's +/// statement of the truth, and it wins over the table. +const CONTEXT_WINDOW_ENV_VAR: &str = "CLAW_CONTEXT_WINDOW"; + +/// Fraction of the window a single response may be allowed to claim when the window is +/// declared. Output and input share one budget on a local server, so reserving a quarter for +/// the reply leaves three quarters for prompt plus transcript. +const MAX_OUTPUT_SHARE_OF_WINDOW: u32 = 4; + +/// The operator-declared context window, when set and parseable as a positive token count. #[must_use] -pub fn max_tokens_for_model(model: &str) -> u32 { - let canonical = resolve_model_alias(model); - let heuristic = if canonical.contains("opus") { - 32_000 - } else { - 64_000 - }; +pub fn context_window_override() -> Option { + parse_context_window_override(std::env::var(CONTEXT_WINDOW_ENV_VAR).ok().as_deref()) +} - model_token_limit(model).map_or(heuristic, |limit| heuristic.min(limit.max_output_tokens)) +#[must_use] +fn parse_context_window_override(value: Option<&str>) -> Option { + value + .and_then(|raw| raw.trim().parse::().ok()) + .filter(|window| *window > 0) +} + +#[must_use] +pub fn max_tokens_for_model(model: &str) -> u32 { + // A declared window caps the reply too. Asking a 65k server for 32k of output reserves half + // the window for a single message, which the server then silently clamps — leaving the + // client's own accounting wrong by the difference. + model_token_limit(model).map_or_else( + || max_tokens_for_model_by_name(model), + |limit| max_tokens_for_model_by_name(model).min(limit.max_output_tokens), + ) } /// Returns the effective max output tokens for a model, preferring a plugin @@ -631,6 +658,33 @@ pub fn max_tokens_for_model_with_override(model: &str, plugin_override: Option Option { + // A declared window replaces the table's, and supplies one for models the table has never + // heard of. Returning `Some` for an unknown model is the point: `None` makes both preflights + // return `Ok` without looking at anything, so an unrecognised local model gets no guard at all. + if let Some(context_window_tokens) = context_window_override() { + return Some(ModelTokenLimit { + max_output_tokens: max_tokens_for_model_by_name(model) + .min(context_window_tokens / MAX_OUTPUT_SHARE_OF_WINDOW), + context_window_tokens, + }); + } + model_token_limit_by_name(model) +} + +#[must_use] +fn max_tokens_for_model_by_name(model: &str) -> u32 { + let canonical = resolve_model_alias(model); + let heuristic = if canonical.contains("opus") { + 32_000 + } else { + 64_000 + }; + model_token_limit_by_name(model) + .map_or(heuristic, |limit| heuristic.min(limit.max_output_tokens)) +} + +#[must_use] +fn model_token_limit_by_name(model: &str) -> Option { let canonical = resolve_model_alias(model); let base_model = canonical.rsplit('/').next().unwrap_or(canonical.as_str()); match base_model { @@ -698,7 +752,7 @@ pub fn preflight_message_request(request: &MessageRequest) -> Result<(), ApiErro Ok(()) } -fn estimate_message_request_input_tokens(request: &MessageRequest) -> u32 { +pub fn estimate_message_request_input_tokens(request: &MessageRequest) -> u32 { let mut estimate = estimate_serialized_tokens(&request.messages); estimate = estimate.saturating_add(estimate_serialized_tokens(&request.system)); estimate = estimate.saturating_add(estimate_serialized_tokens(&request.tools)); diff --git a/rust/crates/api/src/providers/openai_compat.rs b/rust/crates/api/src/providers/openai_compat.rs index 8fb3969913..fd48ebd911 100644 --- a/rust/crates/api/src/providers/openai_compat.rs +++ b/rust/crates/api/src/providers/openai_compat.rs @@ -7,7 +7,7 @@ use std::time::{Duration, SystemTime, UNIX_EPOCH}; use serde::Deserialize; use serde_json::{json, Value}; -use crate::error::ApiError; +use crate::error::{ApiError, ApiErrorDetails}; use crate::http_client::build_http_client_or_default; use crate::types::{ ContentBlockDelta, ContentBlockDeltaEvent, ContentBlockStartEvent, ContentBlockStopEvent, @@ -239,7 +239,7 @@ impl OpenAiCompatClient { .get("code") .and_then(serde_json::Value::as_u64) .map(|c| c as u16); - return Err(ApiError::Api { + return Err(ApiError::Api(Box::new(ApiErrorDetails { status: reqwest::StatusCode::from_u16(code.unwrap_or(400)) .unwrap_or(reqwest::StatusCode::BAD_REQUEST), error_type: err_obj @@ -255,7 +255,7 @@ impl OpenAiCompatClient { .unwrap_or(reqwest::StatusCode::BAD_REQUEST), ), retry_after: None, - }); + }))); } } let payload = serde_json::from_str::(&body).map_err(|error| { @@ -1608,7 +1608,7 @@ fn parse_sse_frame( .map(|c| c as u16); let status = reqwest::StatusCode::from_u16(code.unwrap_or(500)) .unwrap_or(reqwest::StatusCode::INTERNAL_SERVER_ERROR); - return Err(ApiError::Api { + return Err(ApiError::Api(Box::new(ApiErrorDetails { status, error_type: err_obj .get("type") @@ -1620,12 +1620,12 @@ fn parse_sse_frame( retryable: false, suggested_action: suggested_action_for_status(status), retry_after: None, - }); + }))); } } // Detect HTML responses if trimmed.starts_with('<') || trimmed.starts_with("(&payload) .map(Some) @@ -1749,7 +1749,7 @@ async fn expect_success(response: reqwest::Response) -> Result Result { assert_eq!(attempts, 2); - assert!(matches!( - *last_error, - ApiError::Api { - status: reqwest::StatusCode::SERVICE_UNAVAILABLE, - retryable: true, - .. - } - )); + let details = last_error + .api_details() + .expect("last error should be a provider Api error"); + assert_eq!(details.status, reqwest::StatusCode::SERVICE_UNAVAILABLE); + assert!(details.retryable); } other => panic!("expected retries exhausted, got {other:?}"), } diff --git a/rust/crates/claw-rag-service/src/ingest.rs b/rust/crates/claw-rag-service/src/ingest.rs index eae4362912..c7083d14a7 100644 --- a/rust/crates/claw-rag-service/src/ingest.rs +++ b/rust/crates/claw-rag-service/src/ingest.rs @@ -66,7 +66,7 @@ async fn flush_path_batch( #[cfg(feature = "qdrant-index")] let mut qdrant_points: Vec = Vec::with_capacity(batch.len()); - for ((ord, t), vec) in batch.drain(..).zip(vecs.into_iter()) { + for ((ord, t), vec) in batch.drain(..).zip(vecs) { let dim = vec.len(); let cid = insert_chunk(conn, path, ord, &t)?; insert_embedding(conn, cid, dim, &vec)?; diff --git a/rust/crates/runtime/src/compact.rs b/rust/crates/runtime/src/compact.rs index 797d3a6b29..d0273f44d3 100644 --- a/rust/crates/runtime/src/compact.rs +++ b/rust/crates/runtime/src/compact.rs @@ -95,10 +95,21 @@ pub fn get_compact_continuation_message( #[must_use] pub fn compact_session(session: &Session, config: CompactionConfig) -> CompactionResult { if !should_compact(session, config) { + // "Nothing to drop" is not the same as "nothing to do". A session can exceed the window + // while holding fewer messages than `preserve_recent_messages` — a single read of a large + // file does it — and message-granular compaction has no move to make there. Trim the + // payloads anyway, so the one lever that still works is not gated behind the one that + // does not. + let mut trimmed_session = session.clone(); + trimmed_session.messages = session + .messages + .iter() + .map(truncate_oversized_tool_results) + .collect(); return CompactionResult { summary: String::new(), formatted_summary: String::new(), - compacted_session: session.clone(), + compacted_session: trimmed_session, removed_message_count: 0, }; } @@ -165,7 +176,14 @@ pub fn compact_session(session: &Session, config: CompactionConfig) -> Compactio k }; let removed = &session.messages[compacted_prefix_len..keep_from]; - let preserved = session.messages[keep_from..].to_vec(); + // Dropping whole messages is not enough on its own. An agentic turn is five messages + // (user / assistant+tool_use / tool_result / assistant+tool_use / tool_result), so the + // preserved tail IS the pair of tool results that overflowed the window, and the only + // message compaction may drop is the short user prompt. Trim the payloads that must stay. + let preserved = session.messages[keep_from..] + .iter() + .map(truncate_oversized_tool_results) + .collect::>(); let summary = merge_compact_summaries(existing_summary.as_deref(), &summarize_messages(removed)); let formatted_summary = format_compact_summary(&summary); @@ -190,6 +208,78 @@ pub fn compact_session(session: &Session, config: CompactionConfig) -> Compactio } } +/// Largest tool-result payload compaction will preserve verbatim, in characters. +/// +/// ~16 KB is ~4 k tokens: big enough that ordinary tool output is never touched, small enough +/// that several preserved results still fit a small window. +const MAX_PRESERVED_TOOL_RESULT_CHARS: usize = 16 * 1024; + +/// Characters kept from the tail of a trimmed payload. The head carries the structure a model +/// needs to recognise what it read; the tail is where a file's most recent edits usually are. +const PRESERVED_TOOL_RESULT_TAIL_CHARS: usize = 2 * 1024; + +/// Returns `message` with any oversized `ToolResult` payload replaced by a head/tail excerpt. +/// +/// Clones only when something is actually trimmed, so the common path stays cheap. +fn truncate_oversized_tool_results(message: &ConversationMessage) -> ConversationMessage { + let needs_trim = message.blocks.iter().any(|block| match block { + ContentBlock::ToolResult { output, .. } => { + output.chars().count() > MAX_PRESERVED_TOOL_RESULT_CHARS + } + _ => false, + }); + if !needs_trim { + return message.clone(); + } + + let blocks = message + .blocks + .iter() + .map(|block| match block { + ContentBlock::ToolResult { + tool_use_id, + tool_name, + output, + is_error, + } => ContentBlock::ToolResult { + tool_use_id: tool_use_id.clone(), + tool_name: tool_name.clone(), + output: excerpt_tool_result(output), + is_error: *is_error, + }, + other => other.clone(), + }) + .collect(); + + ConversationMessage { + role: message.role, + blocks, + usage: message.usage, + } +} + +/// Builds a head + marker + tail excerpt of an oversized tool-result payload. +/// +/// Operates on chars, not bytes, so a multi-byte boundary can never be split. +fn excerpt_tool_result(output: &str) -> String { + let chars: Vec = output.chars().collect(); + let total = chars.len(); + if total <= MAX_PRESERVED_TOOL_RESULT_CHARS { + return output.to_string(); + } + + let tail_len = PRESERVED_TOOL_RESULT_TAIL_CHARS.min(total); + let head_len = MAX_PRESERVED_TOOL_RESULT_CHARS.saturating_sub(tail_len); + let elided = total.saturating_sub(head_len + tail_len); + if elided == 0 { + return output.to_string(); + } + + let head: String = chars[..head_len].iter().collect(); + let tail: String = chars[total - tail_len..].iter().collect(); + format!("{head}\n\n[... {elided} characters elided by compaction; re-read this file if you need the omitted portion ...]\n\n{tail}") +} + fn compacted_summary_prefix_len(session: &Session) -> usize { usize::from( session @@ -573,7 +663,7 @@ fn extract_summary_timeline(summary: &str) -> Vec { #[cfg(test)] mod tests { use super::{ - collect_key_files, compact_session, format_compact_summary, + collect_key_files, compact_session, estimate_session_tokens, format_compact_summary, get_compact_continuation_message, infer_pending_work, should_compact, CompactionConfig, }; use crate::session::{ContentBlock, ConversationMessage, MessageRole, Session}; @@ -763,6 +853,185 @@ mod tests { /// user(ToolResult) pair at the boundary. An orphaned tool-result message /// without the preceding assistant `tool_calls` causes a 400 on the /// OpenAI-compat path (gaebal-gajae repro 2026-04-09). + /// The failure this exists for: an agentic turn is 5 messages (user / assistant+tool_use / + /// tool_result / assistant+tool_use / tool_result), so `preserve_recent_messages: 4` protects + /// exactly the two giant tool results that overflow the window. Message-granular compaction + /// can only drop the tiny user prompt, so the payloads themselves must be trimmed. + #[test] + fn compaction_truncates_oversized_tool_results_it_must_preserve() { + let huge = "z".repeat(80_000); + let mut session = Session::new(); + session + .push_message(ConversationMessage::user_text("read two files")) + .expect("user message"); + session + .push_message(ConversationMessage::assistant(vec![ + ContentBlock::ToolUse { + id: "call-1".to_string(), + name: "read_file".to_string(), + input: "{\"path\":\"a.rs\"}".to_string(), + }, + ])) + .expect("assistant tool use"); + session + .push_message(ConversationMessage::tool_result( + "call-1", + "read_file", + huge.clone(), + false, + )) + .expect("tool result"); + session + .push_message(ConversationMessage::assistant(vec![ + ContentBlock::ToolUse { + id: "call-2".to_string(), + name: "read_file".to_string(), + input: "{\"path\":\"b.rs\"}".to_string(), + }, + ])) + .expect("second assistant tool use"); + session + .push_message(ConversationMessage::tool_result( + "call-2", + "read_file", + huge.clone(), + false, + )) + .expect("second tool result"); + + let before = estimate_session_tokens(&session); + let result = compact_session( + &session, + CompactionConfig { + preserve_recent_messages: 4, + max_estimated_tokens: 0, + }, + ); + let after = estimate_session_tokens(&result.compacted_session); + + assert!( + after < before / 2, + "compaction must shrink an oversized preserved tool result: {before} -> {after}" + ); + + let preserved_output = result + .compacted_session + .messages + .iter() + .flat_map(|message| &message.blocks) + .find_map(|block| match block { + ContentBlock::ToolResult { output, .. } => Some(output.clone()), + _ => None, + }) + .expect("the tool result must still be present, not deleted"); + + assert!( + preserved_output.len() < huge.len(), + "the payload must actually be trimmed" + ); + assert!( + preserved_output.contains("elided"), + "the model must be told the view is partial, got: {}", + &preserved_output[..preserved_output.len().min(200)] + ); + assert!( + preserved_output.starts_with('z'), + "the head of the output must be kept" + ); + assert!( + preserved_output.ends_with('z'), + "the tail of the output must be kept" + ); + } + + /// A session can exceed the window while holding FEWER messages than + /// `preserve_recent_messages` — one read of a large file does it. `should_compact` returns + /// false there (nothing is droppable), so trimming must not be gated behind it. + #[test] + fn oversized_tool_results_are_trimmed_even_when_no_message_can_be_dropped() { + let huge = "q".repeat(80_000); + let mut session = Session::new(); + session + .push_message(ConversationMessage::user_text("read one enormous file")) + .expect("user message"); + session + .push_message(ConversationMessage::assistant(vec![ + ContentBlock::ToolUse { + id: "call-1".to_string(), + name: "read_file".to_string(), + input: "{\"path\":\"huge.rs\"}".to_string(), + }, + ])) + .expect("assistant tool use"); + session + .push_message(ConversationMessage::tool_result( + "call-1", + "read_file", + huge, + false, + )) + .expect("tool result"); + + // 3 messages, preserve 4: there is nothing to remove, yet the session is far too big. + let before = estimate_session_tokens(&session); + let result = compact_session( + &session, + CompactionConfig { + preserve_recent_messages: 4, + max_estimated_tokens: 0, + }, + ); + let after = estimate_session_tokens(&result.compacted_session); + + assert!( + after < before / 2, + "an untrimmable session must still have its payloads trimmed: {before} -> {after}" + ); + } + + /// Trimming must not touch payloads that already fit — otherwise every ordinary tool result + /// would grow an elision marker it does not need. + #[test] + fn compaction_leaves_small_tool_results_untouched() { + let small = "ok".repeat(50); + let mut session = Session::new(); + for index in 0..6 { + session + .push_message(ConversationMessage::user_text(format!("turn {index}"))) + .expect("user message"); + session + .push_message(ConversationMessage::tool_result( + format!("call-{index}"), + "read_file", + small.clone(), + false, + )) + .expect("tool result"); + } + + let result = compact_session( + &session, + CompactionConfig { + preserve_recent_messages: 4, + max_estimated_tokens: 0, + }, + ); + + for block in result + .compacted_session + .messages + .iter() + .flat_map(|message| &message.blocks) + { + if let ContentBlock::ToolResult { output, .. } = block { + assert_eq!( + output, &small, + "a small tool result must pass through as-is" + ); + } + } + } + #[test] fn compaction_does_not_split_tool_use_tool_result_pair() { use crate::session::{ContentBlock, Session}; diff --git a/rust/crates/runtime/src/conversation.rs b/rust/crates/runtime/src/conversation.rs index 9c36329a16..e8cda7c8d1 100644 --- a/rust/crates/runtime/src/conversation.rs +++ b/rust/crates/runtime/src/conversation.rs @@ -18,6 +18,47 @@ use crate::usage::{TokenUsage, UsageTracker}; const DEFAULT_AUTO_COMPACTION_INPUT_TOKENS_THRESHOLD: u32 = 100_000; const AUTO_COMPACTION_THRESHOLD_ENV_VAR: &str = "CLAUDE_CODE_AUTO_COMPACT_INPUT_TOKENS"; +const DEFAULT_MAX_STALL_NUDGES: usize = 3; +const MAX_STALL_NUDGES_ENV_VAR: &str = "CLAW_MAX_STALL_NUDGES"; +/// Placeholder recorded in the trace when a turn is resumed rather than started, so the trace +/// does not attribute a second user message to a turn that only had one. +const RESUMED_TURN_MARKER: &str = "[resumed turn]"; +/// Operator-declared context window. Kept as a literal rather than imported because `runtime` +/// sits below `api` in the dependency graph; `api::context_window_override` reads the same name. +const CONTEXT_WINDOW_ENV_VAR: &str = "CLAW_CONTEXT_WINDOW"; +/// Share of a declared context window at which compaction is triggered, leaving the remainder +/// as headroom for the reply plus whatever the next tool result carries. +const AUTO_COMPACTION_WINDOW_PERCENT: u32 = 70; + +/// Injected when the model narrates its next step (or asks for a go-ahead mid-task) +/// instead of emitting the tool call that would perform it. +const STALL_NUDGE_PROMPT: &str = "[auto-continue] You ended the turn without calling a tool, \ + but the work is not finished. Do not describe the next step and do not ask for \ + confirmation — issue the tool call now and keep going until the task is done. If \ + everything asked for is genuinely complete, reply with a one-line summary containing \ + no forward-looking sentence."; + +/// Phrases that mark text as an announcement of work the model has not performed yet. +/// Matched against the final non-empty line of the assistant's reply. +const PENDING_ACTION_MARKERS: [&str; 16] = [ + "let me ", + "let's ", + "lets ", + "i'll ", + "i will ", + "i'm going to", + "i am going to", + "going to run", + "about to ", + "now running", + "now checking", + "now i", + "next i", + "next, i", + "proceeding to", + "moving on to", +]; + /// Fully assembled request payload sent to the upstream model client. #[derive(Debug, Clone, PartialEq, Eq)] pub struct ApiRequest { @@ -90,6 +131,7 @@ impl std::error::Error for ToolError {} #[derive(Debug, Clone, PartialEq, Eq)] pub struct RuntimeError { message: String, + context_window: bool, } impl RuntimeError { @@ -97,8 +139,28 @@ impl RuntimeError { pub fn new(message: impl Into) -> Self { Self { message: message.into(), + context_window: false, } } + + /// A failure the caller may be able to clear by shrinking the session. + /// + /// This is a decision the API layer makes and hands up, not one the caller reconstructs by + /// matching substrings against the rendered message. Recovering from context exhaustion means + /// discarding conversation history, so a transport hiccup misread as an overflow destroys the + /// session for no reason — the classification has to come from whoever saw the real error. + #[must_use] + pub fn context_window(message: impl Into) -> Self { + Self { + message: message.into(), + context_window: true, + } + } + + #[must_use] + pub const fn is_context_window_failure(&self) -> bool { + self.context_window + } } impl Display for RuntimeError { @@ -140,6 +202,7 @@ pub struct ConversationRuntime { hook_abort_signal: HookAbortSignal, hook_progress_reporter: Option>, session_tracer: Option, + max_stall_nudges: usize, } impl ConversationRuntime @@ -189,9 +252,17 @@ where hook_abort_signal: HookAbortSignal::default(), hook_progress_reporter: None, session_tracer: None, + max_stall_nudges: max_stall_nudges_from_env(), } } + /// Override how many auto-continue nudges a stalled turn may receive. `0` disables them. + #[must_use] + pub fn with_max_stall_nudges(mut self, max_stall_nudges: usize) -> Self { + self.max_stall_nudges = max_stall_nudges; + self + } + #[must_use] pub fn with_max_iterations(mut self, max_iterations: usize) -> Self { self.max_iterations = max_iterations; @@ -321,14 +392,33 @@ where } } - #[allow(clippy::too_many_lines)] pub fn run_turn( &mut self, user_input: impl Into, - mut prompter: Option<&mut dyn PermissionPrompter>, + prompter: Option<&mut dyn PermissionPrompter>, ) -> Result { - let user_input = user_input.into(); + self.drive_turn(Some(user_input.into()), prompter) + } + + /// Continues the turn already recorded in the session, without adding a user message. + /// + /// Recovery paths retry a turn that failed part-way through. Calling [`Self::run_turn`] again + /// with the original input would append that input a second time and invite the model to redo + /// tool calls whose side effects have already landed. Resuming reads the transcript as it + /// stands — including the tool results that did complete — and carries on from there. + pub fn resume_turn( + &mut self, + prompter: Option<&mut dyn PermissionPrompter>, + ) -> Result { + self.drive_turn(None, prompter) + } + #[allow(clippy::too_many_lines)] + fn drive_turn( + &mut self, + user_input: Option, + mut prompter: Option<&mut dyn PermissionPrompter>, + ) -> Result { // ROADMAP #38: Session-health canary - probe if context was compacted if self.session.compaction.is_some() { if let Err(error) = self.run_session_health_probe() { @@ -340,16 +430,24 @@ where } } - self.record_turn_started(&user_input); - self.session - .push_user_text(user_input) - .map_err(|error| RuntimeError::new(error.to_string()))?; + match user_input { + Some(user_input) => { + self.record_turn_started(&user_input); + self.session + .push_user_text(user_input) + .map_err(|error| RuntimeError::new(error.to_string()))?; + } + None => self.record_turn_started(RESUMED_TURN_MARKER), + } let mut assistant_messages = Vec::new(); let mut tool_results = Vec::new(); let mut prompt_cache_events = Vec::new(); let mut iterations = 0; let mut auto_compaction = None; + let max_stall_nudges = self.max_stall_nudges; + let mut stall_nudges = 0; + let mut tools_ran = false; loop { iterations += 1; @@ -361,6 +459,23 @@ where return Err(error); } + // Compact BEFORE the request goes out, not only after a reply lands. + // + // The usage-based check below is post-hoc: it can only see the context once a call + // has returned and reported its counters. Between that measurement and the next + // request the session can grow enormously — a reasoning model emitting a long + // block, then a tool result carrying a large file. Observed live: a call + // measured at 17,858 prompt tokens generated 10,842 more, a read_file added ~19k on + // top, and the next request hit the provider at 47,755 tokens and was rejected + // outright. No amount of tuning the post-hoc threshold prevents that, because + // nothing measures the session in between. + // + // `estimate_session_tokens` is a local heuristic over the transcript, so this costs + // no API call and works before any usage has ever been recorded. + if let Some(compaction) = self.maybe_auto_compact_before_request() { + auto_compaction = Some(compaction); + } + let request = ApiRequest { system_prompt: self.system_prompt.clone(), messages: self.session.messages.clone(), @@ -376,6 +491,7 @@ where match build_assistant_message(events) { Ok(result) => result, Err(error) => { + let error = self.classify_empty_reply(error); self.record_turn_failed(iterations, &error); return Err(error); } @@ -412,8 +528,35 @@ where } if pending_tool_uses.is_empty() { + // A reply with no tool call usually means the turn is done. On small local + // models it just as often means the model narrated the next step — or asked + // for a go-ahead mid-task — and then emitted EOS, leaving the announced work + // undone. Nudge it back into the loop instead of dropping the user at the + // prompt, bounded so a model that insists it is finished still gets to stop. + let stalled = stall_nudges < max_stall_nudges + && is_stalled_reply( + &assistant_text( + assistant_messages + .last() + .expect("assistant message pushed above"), + ), + tools_ran, + ); + if stalled { + stall_nudges += 1; + // Pushed as a system turn, not a user one. The model needs to see it, but the + // transcript must not claim the user asked for it: this text is persisted, + // re-sent on every later request, and folded into the next compaction summary, + // where a user-labelled nudge becomes indistinguishable from something the + // user actually said. + self.session + .push_message(ConversationMessage::system_text(STALL_NUDGE_PROMPT)) + .map_err(|error| RuntimeError::new(error.to_string()))?; + continue; + } break; } + tools_ran = true; for (tool_use_id, tool_name, input) in pending_tool_uses { let pre_hook_result = self.run_pre_tool_use_hook(&tool_name, &input); @@ -568,13 +711,62 @@ where self.session } + /// Compaction check run immediately before a request is sent, based on a locally estimated + /// transcript size rather than on provider-reported usage. + /// + /// This is the guard that actually keeps a session inside the context window: it sees growth + /// that usage counters cannot, because it runs after tool results and long generations have + /// been appended but before the request that would carry them is built. + /// Decides whether an empty or unreadable reply was an overflow, by size rather than by text. + /// + /// A stream that produces no content says nothing about why. Some backends answer an + /// oversized request that way instead of with a typed error, so the failure is worth + /// recovering from — but only when the session was already large enough for that to be the + /// plausible cause. Below the compaction threshold it is a blip, and discarding the + /// transcript would cost more than the retry could win. + fn classify_empty_reply(&self, error: RuntimeError) -> RuntimeError { + if error.is_context_window_failure() { + return error; + } + if estimate_session_tokens(&self.session) + >= self.auto_compaction_input_tokens_threshold as usize + { + return RuntimeError::context_window(error.to_string()); + } + error + } + + fn maybe_auto_compact_before_request(&mut self) -> Option { + let estimated = estimate_session_tokens(&self.session); + if estimated < self.auto_compaction_input_tokens_threshold as usize { + return None; + } + self.compact_now() + } + fn maybe_auto_compact(&mut self) -> Option { - if self.usage_tracker.cumulative_usage().input_tokens + // Gate on the LIVE context size — the most recent call's input counters — not on a + // cumulative running total, and count cached input as the context it is. + // + // The previous gate (`cumulative_usage().input_tokens`) was wrong twice over: + // 1. It ignored `cache_read_input_tokens`. With prompt caching a warm call reports + // `input_tokens: 2`, so the counter crawled and compaction never fired however + // large the context grew. + // 2. Summing across calls is not a context measurement, and the sum never decreased, + // so once it did trip, every later iteration compacted forever. + // Reading the latest turn fixes both: it rises with the real context and falls back + // below the threshold as soon as a compaction has done its job. + if self.usage_tracker.current_turn_usage().total_input_tokens() < self.auto_compaction_input_tokens_threshold { return None; } + self.compact_now() + } + + /// Compacts the session unconditionally, returning `None` when nothing actually changed. + fn compact_now(&mut self) -> Option { let result = compact_session( &self.session, CompactionConfig { @@ -583,7 +775,12 @@ where }, ); - if result.removed_message_count == 0 { + // `removed_message_count` alone is the wrong test for "did anything happen": compaction + // also trims oversized tool-result payloads in messages it must PRESERVE, and on a + // session too short to have anything droppable that trimming is the entire result. + // Gating on the count would compute the smaller session and then throw it away. + let shrank = result.compacted_session.messages != self.session.messages; + if result.removed_message_count == 0 && !shrank { return None; } @@ -704,19 +901,88 @@ where /// Reads the automatic compaction threshold from the environment. #[must_use] pub fn auto_compaction_threshold_from_env() -> u32 { - parse_auto_compaction_threshold( + resolve_auto_compaction_threshold( std::env::var(AUTO_COMPACTION_THRESHOLD_ENV_VAR) .ok() .as_deref(), + std::env::var(CONTEXT_WINDOW_ENV_VAR).ok().as_deref(), ) } +/// Resolves the threshold from an explicit setting, else from the declared context window. +/// +/// `DEFAULT_AUTO_COMPACTION_INPUT_TOKENS_THRESHOLD` is a 200k-window number. Against a smaller +/// server it sits above the window entirely, so the pre-request guard can never fire and the +/// first sign of trouble is the backend rejecting the request. Deriving from the declared window +/// keeps the guard meaningful without every caller having to compute a second number by hand. #[must_use] -fn parse_auto_compaction_threshold(value: Option<&str>) -> u32 { +fn resolve_auto_compaction_threshold(explicit: Option<&str>, context_window: Option<&str>) -> u32 { + if let Some(threshold) = parse_positive_u32(explicit) { + return threshold; + } + parse_positive_u32(context_window).map_or( + DEFAULT_AUTO_COMPACTION_INPUT_TOKENS_THRESHOLD, + |window| { + (u64::from(window) * u64::from(AUTO_COMPACTION_WINDOW_PERCENT) / 100) + .try_into() + .unwrap_or(DEFAULT_AUTO_COMPACTION_INPUT_TOKENS_THRESHOLD) + }, + ) +} + +#[must_use] +fn parse_positive_u32(value: Option<&str>) -> Option { value .and_then(|raw| raw.trim().parse::().ok()) - .filter(|threshold| *threshold > 0) - .unwrap_or(DEFAULT_AUTO_COMPACTION_INPUT_TOKENS_THRESHOLD) + .filter(|parsed| *parsed > 0) +} + +/// How many auto-continue nudges a single turn may inject. `0` disables the behaviour. +#[must_use] +pub fn max_stall_nudges_from_env() -> usize { + parse_max_stall_nudges(std::env::var(MAX_STALL_NUDGES_ENV_VAR).ok().as_deref()) +} + +#[must_use] +fn parse_max_stall_nudges(value: Option<&str>) -> usize { + value + .and_then(|raw| raw.trim().parse::().ok()) + .unwrap_or(DEFAULT_MAX_STALL_NUDGES) +} + +/// Concatenated text of an assistant message, ignoring thinking and tool blocks. +#[must_use] +fn assistant_text(message: &ConversationMessage) -> String { + message + .blocks + .iter() + .filter_map(|block| match block { + ContentBlock::Text { text } => Some(text.as_str()), + _ => None, + }) + .collect::>() + .join("\n") +} + +/// True when a tool-less reply is a stall rather than a finished turn. +/// +/// Small local models routinely narrate the next tool call in prose and then emit EOS, which +/// ends the turn with the announced work never performed. `after_tool_use` additionally treats +/// a mid-task question as a stall: once tools have run in this turn, stopping to ask for a +/// go-ahead is the same failure wearing a different hat. +#[must_use] +fn is_stalled_reply(text: &str, after_tool_use: bool) -> bool { + let Some(last_line) = text.lines().map(str::trim).rfind(|line| !line.is_empty()) else { + return false; + }; + let lowered = last_line.to_lowercase(); + + if after_tool_use && last_line.ends_with('?') { + return true; + } + PENDING_ACTION_MARKERS + .iter() + .any(|marker| lowered.contains(marker)) } fn build_assistant_message( @@ -848,11 +1114,13 @@ impl ToolExecutor for StaticToolExecutor { #[cfg(test)] mod tests { use super::{ - build_assistant_message, parse_auto_compaction_threshold, ApiClient, ApiRequest, - AssistantEvent, AutoCompactionEvent, ConversationRuntime, PromptCacheEvent, RuntimeError, + assistant_text, build_assistant_message, is_stalled_reply, parse_max_stall_nudges, + resolve_auto_compaction_threshold, ApiClient, ApiRequest, AssistantEvent, + AutoCompactionEvent, ConversationRuntime, PromptCacheEvent, RuntimeError, StaticToolExecutor, ToolExecutor, DEFAULT_AUTO_COMPACTION_INPUT_TOKENS_THRESHOLD, + DEFAULT_MAX_STALL_NUDGES, }; - use crate::compact::CompactionConfig; + use crate::compact::{estimate_session_tokens, CompactionConfig}; use crate::config::{RuntimeFeatureConfig, RuntimeHookConfig}; use crate::permissions::{ PermissionMode, PermissionPolicy, PermissionPromptDecision, PermissionPrompter, @@ -1620,19 +1888,246 @@ mod tests { assert_eq!(runtime.session().messages.len(), 2); } + /// Prompt caching reports the bulk of the context as `cache_read_input_tokens` and leaves + /// `input_tokens` at a token or two, so a gate that reads `input_tokens` alone never fires + /// on a cached session no matter how large the context has grown. + #[test] + fn auto_compaction_counts_cached_context_toward_threshold() { + struct CachedApi; + impl ApiClient for CachedApi { + fn stream( + &mut self, + _request: ApiRequest, + ) -> Result, RuntimeError> { + Ok(vec![ + AssistantEvent::TextDelta("done".to_string()), + AssistantEvent::Usage(TokenUsage { + input_tokens: 2, + output_tokens: 4, + cache_creation_input_tokens: 1_804, + cache_read_input_tokens: 112_639, + }), + AssistantEvent::MessageStop, + ]) + } + } + + let mut session = Session::new(); + session.messages = vec![ + crate::session::ConversationMessage::user_text("one"), + crate::session::ConversationMessage::assistant(vec![ContentBlock::Text { + text: "two".to_string(), + }]), + crate::session::ConversationMessage::user_text("three"), + crate::session::ConversationMessage::assistant(vec![ContentBlock::Text { + text: "four".to_string(), + }]), + ]; + + let mut runtime = ConversationRuntime::new( + session, + CachedApi, + StaticToolExecutor::new(), + PermissionPolicy::new(PermissionMode::DangerFullAccess), + vec!["system".to_string()], + ) + .with_auto_compaction_input_tokens_threshold(100_000); + + let summary = runtime + .run_turn("trigger", None) + .expect("turn should succeed"); + + assert_eq!( + summary.auto_compaction, + Some(AutoCompactionEvent { + removed_message_count: 2, + }) + ); + } + + /// The threshold describes the LIVE context, not a monotonic running total. Once compaction + /// has shrunk the context back down, later small turns must not keep compacting. + #[test] + fn auto_compaction_stops_once_the_context_shrinks_again() { + struct ShrinkingApi { + calls: u32, + } + impl ApiClient for ShrinkingApi { + fn stream( + &mut self, + _request: ApiRequest, + ) -> Result, RuntimeError> { + let call = self.calls; + self.calls += 1; + // The first call is over budget; every call after it is small. + let input_tokens = if call == 0 { 150_000 } else { 900 }; + Ok(vec![ + AssistantEvent::TextDelta("done".to_string()), + AssistantEvent::Usage(TokenUsage { + input_tokens, + output_tokens: 4, + cache_creation_input_tokens: 0, + cache_read_input_tokens: 0, + }), + AssistantEvent::MessageStop, + ]) + } + } + + let mut session = Session::new(); + session.messages = vec![ + crate::session::ConversationMessage::user_text("one"), + crate::session::ConversationMessage::assistant(vec![ContentBlock::Text { + text: "two".to_string(), + }]), + crate::session::ConversationMessage::user_text("three"), + crate::session::ConversationMessage::assistant(vec![ContentBlock::Text { + text: "four".to_string(), + }]), + ]; + + let mut runtime = ConversationRuntime::new( + session, + ShrinkingApi { calls: 0 }, + // Every turn after a compaction runs the session health probe, which calls + // `glob_search`; without it the probe fails the turn before the assertion is reached. + StaticToolExecutor::new().register("glob_search", |_| Ok(String::new())), + PermissionPolicy::new(PermissionMode::DangerFullAccess), + vec!["system".to_string()], + ) + .with_auto_compaction_input_tokens_threshold(100_000); + + let first = runtime.run_turn("trigger", None).expect("first turn"); + assert!( + first.auto_compaction.is_some(), + "the over-budget turn should compact" + ); + + let second = runtime.run_turn("again", None).expect("second turn"); + assert_eq!( + second.auto_compaction, None, + "a small turn after compaction must not compact again" + ); + let third = runtime.run_turn("and again", None).expect("third turn"); + assert_eq!( + third.auto_compaction, None, + "compaction must not latch on once the threshold has been crossed" + ); + } + + /// Usage-based compaction is post-hoc: it only sees the context AFTER a call returns. A + /// large tool result (or a long reasoning block) appended after that measurement can blow + /// the window on the very next request, which the provider rejects outright — so there must + /// also be a check BEFORE the request goes out, based on a locally estimated session size. + #[test] + fn auto_compaction_fires_before_the_first_request_when_the_session_is_already_oversized() { + struct RecordingApi { + largest_request_messages: std::rc::Rc>, + } + impl ApiClient for RecordingApi { + fn stream(&mut self, request: ApiRequest) -> Result, RuntimeError> { + self.largest_request_messages.set( + self.largest_request_messages + .get() + .max(request.messages.len()), + ); + Ok(vec![ + AssistantEvent::TextDelta("done".to_string()), + // Deliberately no Usage event: nothing has been measured yet, which is + // exactly the situation the pre-send check has to cover. + AssistantEvent::MessageStop, + ]) + } + } + + // Six messages that are individually large — the shape of a session that has just + // absorbed a couple of big file reads. + let bulk = "x".repeat(40_000); + let mut session = Session::new(); + for _ in 0..3 { + session + .messages + .push(crate::session::ConversationMessage::user_text(bulk.clone())); + session + .messages + .push(crate::session::ConversationMessage::assistant(vec![ + ContentBlock::Text { text: bulk.clone() }, + ])); + } + let before = estimate_session_tokens(&session); + assert!( + before > 24_000, + "test fixture must start over the threshold, got {before}" + ); + + let largest = std::rc::Rc::new(std::cell::Cell::new(0usize)); + let mut runtime = ConversationRuntime::new( + session, + RecordingApi { + largest_request_messages: std::rc::Rc::clone(&largest), + }, + StaticToolExecutor::new().register("glob_search", |_| Ok(String::new())), + PermissionPolicy::new(PermissionMode::DangerFullAccess), + vec!["system".to_string()], + ) + .with_auto_compaction_input_tokens_threshold(24_000); + + let summary = runtime + .run_turn("trigger", None) + .expect("turn should succeed"); + + assert!( + summary.auto_compaction.is_some(), + "an already-oversized session must compact before the request is sent" + ); + assert!( + estimate_session_tokens(runtime.session()) < before, + "compaction must actually shrink the session" + ); + assert!( + largest.get() < 8, + "the request must be sent with the compacted history, not the original 7 messages" + ); + } + #[test] fn auto_compaction_threshold_defaults_and_parses_values() { assert_eq!( - parse_auto_compaction_threshold(None), + resolve_auto_compaction_threshold(None, None), DEFAULT_AUTO_COMPACTION_INPUT_TOKENS_THRESHOLD ); - assert_eq!(parse_auto_compaction_threshold(Some("4321")), 4321); + assert_eq!(resolve_auto_compaction_threshold(Some("4321"), None), 4321); assert_eq!( - parse_auto_compaction_threshold(Some("0")), + resolve_auto_compaction_threshold(Some("0"), None), DEFAULT_AUTO_COMPACTION_INPUT_TOKENS_THRESHOLD ); assert_eq!( - parse_auto_compaction_threshold(Some("not-a-number")), + resolve_auto_compaction_threshold(Some("not-a-number"), None), + DEFAULT_AUTO_COMPACTION_INPUT_TOKENS_THRESHOLD + ); + } + + #[test] + fn auto_compaction_threshold_derives_from_a_declared_context_window() { + // 70% of 65536, so the guard sits inside the window instead of 34k above it. + assert_eq!( + resolve_auto_compaction_threshold(None, Some("65536")), + 45875 + ); + } + + #[test] + fn an_explicit_threshold_still_wins_over_the_declared_window() { + assert_eq!( + resolve_auto_compaction_threshold(Some("48000"), Some("65536")), + 48000 + ); + } + + #[test] + fn an_unparseable_context_window_falls_back_to_the_default_threshold() { + assert_eq!( + resolve_auto_compaction_threshold(None, Some("plenty")), DEFAULT_AUTO_COMPACTION_INPUT_TOKENS_THRESHOLD ); } @@ -1804,6 +2299,146 @@ mod tests { assert_eq!(error.to_string(), "unknown tool: missing"); } + /// Replays a fixed script of stream outcomes, one per call. + struct ScriptedApi { + steps: Vec, String>>, + calls: usize, + } + + impl ScriptedApi { + fn new(steps: Vec, String>>) -> Self { + Self { steps, calls: 0 } + } + } + + impl ApiClient for ScriptedApi { + fn stream(&mut self, _request: ApiRequest) -> Result, RuntimeError> { + let step = self + .steps + .get(self.calls) + .cloned() + .unwrap_or_else(|| Err("script exhausted".to_string())); + self.calls += 1; + step.map_err(RuntimeError::new) + } + } + + fn tool_call_events(id: &str) -> Vec { + vec![ + AssistantEvent::ToolUse { + id: id.to_string(), + name: "echo".to_string(), + input: "payload".to_string(), + }, + AssistantEvent::MessageStop, + ] + } + + fn scripted_runtime( + steps: Vec, String>>, + ) -> ConversationRuntime { + ConversationRuntime::new( + Session::new(), + ScriptedApi::new(steps), + StaticToolExecutor::new().register("echo", |input| Ok(input.to_string())), + PermissionPolicy::new(PermissionMode::DangerFullAccess), + vec!["system".to_string()], + ) + } + + #[test] + fn a_failed_turn_keeps_the_results_of_tools_that_already_ran() { + // given: a turn whose first iteration runs a tool, and whose second call dies + let mut runtime = scripted_runtime(vec![ + Ok(tool_call_events("tool-1")), + Err("upstream failed".to_string()), + ]); + + // when + runtime + .run_turn("do the thing", None) + .expect_err("the second call fails"); + + // then: the executed tool's result is still in the transcript. The side effect happened; + // a caller that discards this record will let the model run the same tool again. + let tool_results = runtime + .session() + .messages + .iter() + .filter(|message| { + message + .blocks + .iter() + .any(|block| matches!(block, ContentBlock::ToolResult { .. })) + }) + .count(); + assert_eq!(tool_results, 1); + } + + #[test] + fn resume_turn_continues_without_appending_another_user_message() { + // given: a session that already carries a user turn and a completed tool result + let mut runtime = scripted_runtime(vec![ + Ok(tool_call_events("tool-1")), + Err("upstream failed".to_string()), + Ok(vec![ + AssistantEvent::TextDelta("done".to_string()), + AssistantEvent::MessageStop, + ]), + ]); + runtime.run_turn("do the thing", None).expect_err("fails"); + let user_messages_before = runtime + .session() + .messages + .iter() + .filter(|message| message.role == MessageRole::User) + .count(); + + // when + runtime + .resume_turn(None) + .expect("the resumed turn succeeds"); + + // then: the prompt was not repeated + let user_messages_after = runtime + .session() + .messages + .iter() + .filter(|message| message.role == MessageRole::User) + .count(); + assert_eq!(user_messages_before, user_messages_after); + } + + #[test] + fn a_stall_nudge_is_recorded_as_a_system_turn_not_a_user_one() { + // given: a reply that announces work and stops, so the nudge fires once + let mut runtime = nudge_runtime( + vec![ + text_only("Let me check the logs."), + text_only("The logs were clean."), + ], + 1, + ); + runtime + .run_turn("check the logs", None) + .expect("the nudged turn completes"); + + // then: exactly one user message — the real one + let user_messages = runtime + .session() + .messages + .iter() + .filter(|message| message.role == MessageRole::User) + .count(); + assert_eq!(user_messages, 1); + assert!(runtime + .session() + .messages + .iter() + .any(|message| message.role == MessageRole::System + && assistant_text(message).contains("[auto-continue]"))); + } + #[test] fn run_turn_errors_when_max_iterations_is_exceeded() { struct LoopingApi; @@ -1875,4 +2510,136 @@ mod tests { // then assert_eq!(error.to_string(), "upstream failed"); } + + fn text_only(text: &str) -> Vec { + vec![ + AssistantEvent::TextDelta(text.to_string()), + AssistantEvent::MessageStop, + ] + } + + struct CountingApi { + calls: usize, + replies: Vec>, + } + + impl ApiClient for CountingApi { + fn stream(&mut self, _request: ApiRequest) -> Result, RuntimeError> { + let reply = self + .replies + .get(self.calls) + .cloned() + .unwrap_or_else(|| text_only("Now running the next probe:")); + self.calls += 1; + Ok(reply) + } + } + + fn nudge_runtime( + replies: Vec>, + max_stall_nudges: usize, + ) -> ConversationRuntime { + ConversationRuntime::new( + Session::new(), + CountingApi { calls: 0, replies }, + StaticToolExecutor::new().register("probe", |_| Ok("probe output".to_string())), + PermissionPolicy::new(PermissionMode::DangerFullAccess), + vec!["system".to_string()], + ) + .with_max_stall_nudges(max_stall_nudges) + } + + #[test] + fn announced_but_uncalled_tool_is_auto_continued() { + // given: the model narrates the next probe instead of calling it + let mut runtime = nudge_runtime( + vec![ + text_only("Now running the Ford EEC-V tuner probe (ATSP1):"), + vec![ + AssistantEvent::ToolUse { + id: "tool-1".to_string(), + name: "probe".to_string(), + input: "atsp1".to_string(), + }, + AssistantEvent::MessageStop, + ], + text_only("No response on ATSP1, which is expected here."), + ], + 3, + ); + + // when + let summary = runtime.run_turn("run the probes", None).expect("turn runs"); + + // then: the nudge pulled it back into the loop and the tool actually ran + assert_eq!(summary.iterations, 3); + assert_eq!(summary.tool_results.len(), 1); + } + + #[test] + fn nudges_are_bounded_so_a_stuck_model_still_stops() { + // given: every reply is an announcement and nothing is ever called + let mut runtime = nudge_runtime(Vec::new(), 2); + + // when + let summary = runtime.run_turn("run the probes", None).expect("turn runs"); + + // then: initial reply plus exactly two nudged retries + assert_eq!(summary.iterations, 3); + assert!(summary.tool_results.is_empty()); + } + + #[test] + fn a_plain_completion_is_not_nudged() { + // given + let mut runtime = nudge_runtime(vec![text_only("The DTC scan came back clean.")], 3); + + // when + let summary = runtime.run_turn("scan for codes", None).expect("turn runs"); + + // then + assert_eq!(summary.iterations, 1); + } + + #[test] + fn zero_budget_disables_auto_continue() { + // given + let mut runtime = nudge_runtime(vec![text_only("Let me run the DTC scan.")], 0); + + // when + let summary = runtime.run_turn("scan for codes", None).expect("turn runs"); + + // then + assert_eq!(summary.iterations, 1); + } + + #[test] + fn a_question_stalls_only_after_a_tool_has_run() { + assert!(is_stalled_reply("Should I run the DTC scan next?", true)); + assert!(!is_stalled_reply("Which port is the adapter on?", false)); + } + + #[test] + fn stall_detection_reads_the_last_line_not_the_whole_reply() { + // an announcement earlier in the reply does not make a finished turn look stalled + assert!(!is_stalled_reply( + "Let me check the port.\nThe scan finished with no codes stored.", + true + )); + assert!(is_stalled_reply( + "The scan finished.\nNext I'll read the freeze-frame data.", + true + )); + } + + #[test] + fn stall_nudge_budget_parses_from_env_value() { + // asserted against the literal, not the constant: comparing the parser to the same + // constant it falls back to would pass no matter what the default became + assert_eq!(DEFAULT_MAX_STALL_NUDGES, 3); + assert_eq!(parse_max_stall_nudges(None), 3); + assert_eq!(parse_max_stall_nudges(Some("not a number")), 3); + assert_eq!(parse_max_stall_nudges(Some(" 5 ")), 5); + assert_eq!(parse_max_stall_nudges(Some("0")), 0); + } } diff --git a/rust/crates/runtime/src/file_ops.rs b/rust/crates/runtime/src/file_ops.rs index aa7b58135e..6b8fe1a6a0 100644 --- a/rust/crates/runtime/src/file_ops.rs +++ b/rust/crates/runtime/src/file_ops.rs @@ -11,8 +11,48 @@ use serde::{Deserialize, Serialize}; use walkdir::{DirEntry, WalkDir}; /// Maximum file size that can be read (10 MB). +/// +/// This is a *process* guard — it stops us slurping a huge or binary blob into memory. It is +/// not a context guard: 10 MB is on the order of 2.5 M tokens, so it never fires on the files +/// that actually blow a model's context window. `DEFAULT_READ_LINES` / `MAX_READ_CHARS` below +/// are the context guard. const MAX_READ_SIZE: u64 = 10 * 1024 * 1024; +/// Lines returned by `read_file` when the caller does not pass an explicit `limit`. +/// +/// This is a *paging* size, not a refusal. An unbounded read is the easiest way for a model to +/// destroy its own context: one 100 KB source file is ~25 k tokens, which does not fit alongside +/// a system prompt in a 32 k window. When that happens the backend rejects the whole request and +/// the session dies — so reading everything at once is what makes it stop. Returning a page plus +/// `nextOffset` means the read always succeeds and the model can keep going. +const DEFAULT_READ_LINES: usize = 2000; + +/// Hard ceiling on the characters `read_file` returns, applied even when `limit` is explicit. +/// +/// `DEFAULT_READ_LINES` alone does not bound the payload: minified bundles, JSON blobs and +/// generated sources routinely put megabytes on a handful of lines. ~64 KB is ~16 k tokens. +/// +/// That default assumes a large (200 k) context window. Against a small self-hosted backend it +/// is far too generous: two reads at this size put a request over 47 k tokens, which a 32 k +/// window rejects outright — and compaction cannot recover it, because the preserved recent +/// messages ARE the oversized tool results. Override with [`MAX_READ_CHARS_ENV_VAR`]. +const DEFAULT_MAX_READ_CHARS: usize = 64 * 1024; + +/// Environment override for [`DEFAULT_MAX_READ_CHARS`], in characters. +const MAX_READ_CHARS_ENV_VAR: &str = "CLAW_MAX_READ_CHARS"; + +/// Effective character ceiling for one `read_file` page. +/// +/// Read per call rather than cached: a long-lived process should pick up a changed budget, and +/// this runs once per read, not per line. +fn max_read_chars() -> usize { + std::env::var(MAX_READ_CHARS_ENV_VAR) + .ok() + .and_then(|raw| raw.trim().parse::().ok()) + .filter(|chars| *chars > 0) + .unwrap_or(DEFAULT_MAX_READ_CHARS) +} + /// Maximum file size that can be written (10 MB). const MAX_WRITE_SIZE: usize = 10 * 1024 * 1024; @@ -65,6 +105,17 @@ pub struct TextFilePayload { pub start_line: usize, #[serde(rename = "totalLines")] pub total_lines: usize, + /// True when the file has more lines past this page. Signals the caller that it has NOT seen + /// the whole file, so it does not silently reason over a partial view. + #[serde(default)] + pub truncated: bool, + /// Line index to pass back as `offset` to continue reading. Absent once the file is exhausted. + #[serde( + rename = "nextOffset", + default, + skip_serializing_if = "Option::is_none" + )] + pub next_offset: Option, } /// Output envelope for the `read_file` tool. @@ -213,10 +264,28 @@ pub fn read_file( let content = fs::read_to_string(&absolute_path)?; let lines: Vec<&str> = content.lines().collect(); let start_index = offset.unwrap_or(0).min(lines.len()); - let end_index = limit.map_or(lines.len(), |limit| { - start_index.saturating_add(limit).min(lines.len()) - }); + // An absent `limit` pages rather than reading to EOF: see DEFAULT_READ_LINES. + let requested_end = start_index + .saturating_add(limit.unwrap_or(DEFAULT_READ_LINES)) + .min(lines.len()); + + // Apply the character ceiling on top of the line window, so a file with very long lines + // cannot blow the budget the line cap was meant to enforce. Always yield at least one line, + // otherwise an oversized single line would return nothing and the caller could not advance. + let mut end_index = start_index; + let mut chars = 0usize; + let char_ceiling = max_read_chars(); + for line in &lines[start_index..requested_end] { + let next = chars + line.chars().count() + 1; + if next > char_ceiling && end_index > start_index { + break; + } + chars = next; + end_index += 1; + } + let selected = lines[start_index..end_index].join("\n"); + let truncated = end_index < lines.len(); Ok(ReadFileOutput { kind: String::from("text"), @@ -226,6 +295,8 @@ pub fn read_file( num_lines: end_index.saturating_sub(start_index), start_line: start_index.saturating_add(1), total_lines: lines.len(), + truncated, + next_offset: truncated.then_some(end_index), }, }) } @@ -777,10 +848,18 @@ mod tests { use super::{ component_contains_glob, derive_glob_walk_root, edit_file, expand_braces, glob_search, - grep_search, is_symlink_escape, read_file, read_file_in_workspace, write_file, - write_file_in_workspace, GrepSearchInput, MAX_WRITE_SIZE, + grep_search, is_symlink_escape, max_read_chars, read_file, read_file_in_workspace, + write_file, write_file_in_workspace, GrepSearchInput, DEFAULT_MAX_READ_CHARS, + DEFAULT_READ_LINES, MAX_READ_CHARS_ENV_VAR, MAX_WRITE_SIZE, }; + /// Serializes the tests that mutate `CLAW_MAX_READ_CHARS`; env vars are process-global. + fn env_lock() -> std::sync::MutexGuard<'static, ()> { + static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + LOCK.lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } + fn temp_path(name: &str) -> std::path::PathBuf { let unique = SystemTime::now() .duration_since(UNIX_EPOCH) @@ -801,6 +880,132 @@ mod tests { assert_eq!(read_output.file.content, "two"); } + #[test] + fn unbounded_read_of_a_large_file_pages_instead_of_returning_everything() { + // Regression: an unbounded read used to return the whole file, which on a small-context + // backend overflows the window and the request is rejected outright — the session dies. + let path = temp_path("paged-read.txt"); + let total = DEFAULT_READ_LINES + 500; + let body = (0..total) + .map(|i| format!("line {i}")) + .collect::>() + .join("\n"); + write_file(path.to_string_lossy().as_ref(), &body).expect("write should succeed"); + + let first = read_file(path.to_string_lossy().as_ref(), None, None).expect("first page"); + assert_eq!(first.file.num_lines, DEFAULT_READ_LINES); + assert_eq!(first.file.total_lines, total); + assert!(first.file.truncated, "caller must be told it is partial"); + assert_eq!(first.file.next_offset, Some(DEFAULT_READ_LINES)); + assert!(first.file.content.starts_with("line 0\n")); + + // The advertised next_offset must actually resume where the last page stopped. + let second = read_file( + path.to_string_lossy().as_ref(), + first.file.next_offset, + None, + ) + .expect("second page"); + assert_eq!(second.file.num_lines, 500); + assert!(!second.file.truncated, "the tail is the final page"); + assert_eq!(second.file.next_offset, None); + assert!(second + .file + .content + .starts_with(&format!("line {DEFAULT_READ_LINES}\n"))); + } + + #[test] + fn char_ceiling_bounds_files_whose_lines_are_enormous() { + // A line cap alone does not bound the payload: minified bundles put megabytes on a few + // lines, so the byte ceiling has to apply too — and must still yield forward progress. + let _guard = env_lock(); + std::env::remove_var(MAX_READ_CHARS_ENV_VAR); + let path = temp_path("long-lines.txt"); + let huge = "x".repeat(DEFAULT_MAX_READ_CHARS); + let body = format!("{huge}\n{huge}\n{huge}"); + write_file(path.to_string_lossy().as_ref(), &body).expect("write should succeed"); + + let page = read_file(path.to_string_lossy().as_ref(), None, None).expect("first page"); + assert_eq!(page.file.num_lines, 1, "one oversized line still advances"); + assert!(page.file.truncated); + assert_eq!(page.file.next_offset, Some(1)); + + // An explicit limit must not be able to defeat the ceiling. + let greedy = read_file(path.to_string_lossy().as_ref(), None, Some(3)).expect("greedy"); + assert_eq!(greedy.file.num_lines, 1); + assert!(greedy.file.truncated); + } + + /// The 64 KB default is a 200 k-context number. A small self-hosted backend must be able to + /// shrink it, or two reads overflow a 32 k window in a single turn. + #[test] + fn read_char_ceiling_is_configurable_via_the_environment() { + let _guard = env_lock(); + let path = temp_path("configurable-ceiling.txt"); + // 40 lines of 100 chars = ~4 KB: far under the default ceiling, over a 1 KB one. + let body = (0..40) + .map(|_| "y".repeat(100)) + .collect::>() + .join("\n"); + write_file(path.to_string_lossy().as_ref(), &body).expect("write should succeed"); + + std::env::remove_var(MAX_READ_CHARS_ENV_VAR); + let whole = read_file(path.to_string_lossy().as_ref(), None, None).expect("default read"); + assert_eq!( + whole.file.num_lines, 40, + "default ceiling returns the file whole" + ); + assert!(!whole.file.truncated); + + std::env::set_var(MAX_READ_CHARS_ENV_VAR, "1024"); + let clipped = read_file(path.to_string_lossy().as_ref(), None, None).expect("clipped read"); + std::env::remove_var(MAX_READ_CHARS_ENV_VAR); + assert!( + clipped.file.num_lines < 40, + "a smaller ceiling must actually clip, got {} lines", + clipped.file.num_lines + ); + assert!( + clipped.file.truncated, + "a clipped page must report truncated" + ); + assert_eq!( + clipped.file.next_offset, + Some(clipped.file.num_lines), + "the caller must be told where to resume" + ); + } + + #[test] + fn read_char_ceiling_ignores_junk_and_zero_values() { + let _guard = env_lock(); + for value in ["", "0", "-5", "not-a-number"] { + std::env::set_var(MAX_READ_CHARS_ENV_VAR, value); + assert_eq!( + max_read_chars(), + DEFAULT_MAX_READ_CHARS, + "{value:?} must fall back to the default rather than disabling reads" + ); + } + std::env::set_var(MAX_READ_CHARS_ENV_VAR, "24000"); + assert_eq!(max_read_chars(), 24_000); + std::env::remove_var(MAX_READ_CHARS_ENV_VAR); + assert_eq!(max_read_chars(), DEFAULT_MAX_READ_CHARS); + } + + #[test] + fn small_files_are_returned_whole_and_not_marked_truncated() { + let path = temp_path("small-read.txt"); + write_file(path.to_string_lossy().as_ref(), "one\ntwo\nthree").expect("write"); + + let output = read_file(path.to_string_lossy().as_ref(), None, None).expect("read"); + assert_eq!(output.file.content, "one\ntwo\nthree"); + assert_eq!(output.file.num_lines, 3); + assert!(!output.file.truncated); + assert_eq!(output.file.next_offset, None); + } + #[test] fn edits_file_contents() { let path = temp_path("edit.txt"); diff --git a/rust/crates/runtime/src/prompt.rs b/rust/crates/runtime/src/prompt.rs index e62e32ea5d..94eed83d4a 100644 --- a/rust/crates/runtime/src/prompt.rs +++ b/rust/crates/runtime/src/prompt.rs @@ -42,7 +42,25 @@ pub const SYSTEM_PROMPT_DYNAMIC_BOUNDARY: &str = "__SYSTEM_PROMPT_DYNAMIC_BOUNDA pub const FRONTIER_MODEL_NAME: &str = "Claude Opus 4.6"; const MAX_INSTRUCTION_FILE_CHARS: usize = 4_000; const MAX_TOTAL_INSTRUCTION_CHARS: usize = 12_000; -const MAX_GIT_DIFF_CHARS: usize = 50_000; +/// Character budget for the git diff snapshot embedded in the system prompt. +/// +/// This lives in the *system prompt*, so its cost is paid on every session whose +/// working tree has changed — and a diff changes on essentially every edit. The +/// previous 50_000 budget was four times the entire instruction-file budget and +/// could add ~15_000 tokens to the prompt from a single modified file, which is +/// affordable against a hosted frontier model and ruinous against a local one. +/// +/// Override with `CLAW_MAX_GIT_DIFF_CHARS`. `0` drops the diff snapshot entirely; +/// git status and the recent-commit list are unaffected. +const DEFAULT_MAX_GIT_DIFF_CHARS: usize = 4_000; +const MAX_GIT_DIFF_CHARS_ENV_VAR: &str = "CLAW_MAX_GIT_DIFF_CHARS"; + +fn max_git_diff_chars() -> usize { + std::env::var(MAX_GIT_DIFF_CHARS_ENV_VAR) + .ok() + .and_then(|value| value.trim().parse().ok()) + .unwrap_or(DEFAULT_MAX_GIT_DIFF_CHARS) +} /// Neutral identity for the model family line in generated prompts. #[derive(Debug, Clone, Copy, Default, PartialEq, Eq)] @@ -451,8 +469,15 @@ fn read_git_diff(cwd: &Path) -> Option { } fn truncate_diff(mut diff: String) -> String { - if diff.len() > MAX_GIT_DIFF_CHARS { - let mut end = MAX_GIT_DIFF_CHARS; + let budget = max_git_diff_chars(); + // A zero budget means "omit the diff", not "truncate to nothing" -- returning + // an empty string here would still push an empty "Git diff snapshot:" heading + // into the prompt. + if budget == 0 { + return String::new(); + } + if diff.len() > budget { + let mut end = budget; while !diff.is_char_boundary(end) { end -= 1; } @@ -706,6 +731,7 @@ fn get_simple_doing_tasks_section() -> String { "Do not add speculative abstractions, compatibility shims, or unrelated cleanup.".to_string(), "Do not create files unless they are required to complete the task.".to_string(), "If an approach fails, diagnose the failure before switching tactics.".to_string(), + "Act in the turn you plan in: when the next step is a tool call, make the call. Never end a turn describing a step you have not taken, and do not stop to ask for a go-ahead on work already requested.".to_string(), "Be careful not to introduce security vulnerabilities such as command injection, XSS, or SQL injection.".to_string(), "Report outcomes faithfully: if verification fails or was not run, say so explicitly.".to_string(), ]); @@ -730,7 +756,8 @@ mod tests { collapse_blank_lines, display_context_path, normalize_instruction_content, render_instruction_content, render_instruction_files, truncate_diff, truncate_instruction_content, ContextFile, ModelFamilyIdentity, ProjectContext, - SystemPromptBuilder, MAX_GIT_DIFF_CHARS, SYSTEM_PROMPT_DYNAMIC_BOUNDARY, + SystemPromptBuilder, DEFAULT_MAX_GIT_DIFF_CHARS, MAX_GIT_DIFF_CHARS_ENV_VAR, + SYSTEM_PROMPT_DYNAMIC_BOUNDARY, }; use crate::config::ConfigLoader; use std::fs; @@ -1392,33 +1419,69 @@ mod tests { assert!(rendered.contains("Project rules")); } + /// Serializes the tests that read or mutate `CLAW_MAX_GIT_DIFF_CHARS`; env vars + /// are process-global and the default-budget tests fail if another test leaves + /// an override set. + fn diff_env_lock() -> std::sync::MutexGuard<'static, ()> { + static LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(()); + LOCK.lock() + .unwrap_or_else(std::sync::PoisonError::into_inner) + } + #[test] fn truncate_diff_preserves_short_content() { + let _guard = diff_env_lock(); + std::env::remove_var(MAX_GIT_DIFF_CHARS_ENV_VAR); let short = "a".repeat(1_000); let result = truncate_diff(short.clone()); assert_eq!(result, short); assert!(!result.contains("[diff truncated")); } + #[test] + fn truncate_diff_honors_env_override() { + let _guard = diff_env_lock(); + std::env::set_var(MAX_GIT_DIFF_CHARS_ENV_VAR, "100"); + let result = truncate_diff("z".repeat(1_000)); + std::env::remove_var(MAX_GIT_DIFF_CHARS_ENV_VAR); + let marker = "\n\n... [diff truncated — too large for system prompt]"; + assert!(result.ends_with(marker)); + assert_eq!(result.len() - marker.len(), 100); + } + + #[test] + fn truncate_diff_zero_budget_omits_the_snapshot_entirely() { + // Zero must yield nothing at all -- a truncated-to-empty body would still + // drag a "Git diff snapshot:" heading and the truncation marker into the + // prompt, which is the opposite of what the operator asked for. + let _guard = diff_env_lock(); + std::env::set_var(MAX_GIT_DIFF_CHARS_ENV_VAR, "0"); + let result = truncate_diff("z".repeat(1_000)); + std::env::remove_var(MAX_GIT_DIFF_CHARS_ENV_VAR); + assert!(result.is_empty()); + } + #[test] fn truncate_diff_caps_oversized_content() { - let large = "x".repeat(MAX_GIT_DIFF_CHARS + 5_000); + let _guard = diff_env_lock(); + std::env::remove_var(MAX_GIT_DIFF_CHARS_ENV_VAR); + let large = "x".repeat(DEFAULT_MAX_GIT_DIFF_CHARS + 5_000); let result = truncate_diff(large); assert!(result.contains("... [diff truncated — too large for system prompt]")); - // The body before the marker must be at most MAX_GIT_DIFF_CHARS bytes + // The body before the marker must be at most DEFAULT_MAX_GIT_DIFF_CHARS bytes let marker = "\n\n... [diff truncated — too large for system prompt]"; let body_len = result.len() - marker.len(); - assert!(body_len <= MAX_GIT_DIFF_CHARS); + assert!(body_len <= DEFAULT_MAX_GIT_DIFF_CHARS); } #[test] fn truncate_diff_respects_utf8_char_boundaries() { - // Build a string where MAX_GIT_DIFF_CHARS falls in the middle of a + // Build a string where DEFAULT_MAX_GIT_DIFF_CHARS falls in the middle of a // multi-byte character (U+1F600 = 4 bytes in UTF-8). - let prefix_len = MAX_GIT_DIFF_CHARS - 2; + let prefix_len = DEFAULT_MAX_GIT_DIFF_CHARS - 2; let mut input = "a".repeat(prefix_len); // Append a 4-byte emoji so bytes [prefix_len..prefix_len+4] are the - // emoji. MAX_GIT_DIFF_CHARS lands at prefix_len+2, inside the emoji. + // emoji. DEFAULT_MAX_GIT_DIFF_CHARS lands at prefix_len+2, inside the emoji. input.push('\u{1F600}'); input.push_str(&"b".repeat(10_000)); @@ -1430,7 +1493,7 @@ mod tests { // inside it would be invalid UTF-8. let marker = "\n\n... [diff truncated — too large for system prompt]"; let body = &result[..result.len() - marker.len()]; - assert!(body.len() <= MAX_GIT_DIFF_CHARS); + assert!(body.len() <= DEFAULT_MAX_GIT_DIFF_CHARS); assert!(body.is_char_boundary(body.len())); } } diff --git a/rust/crates/runtime/src/session.rs b/rust/crates/runtime/src/session.rs index 2ecfd97dab..61cca8c8a2 100644 --- a/rust/crates/runtime/src/session.rs +++ b/rust/crates/runtime/src/session.rs @@ -718,6 +718,20 @@ impl ConversationMessage { } } + /// Text the harness injected on its own initiative, not something the user typed. + /// + /// Reaches the model as an ordinary turn — the wire protocol has nowhere else to put it — but + /// stays labelled in the transcript, so a replay, an export, or a compaction summary does not + /// present the harness's own prompting back as the user's words. + #[must_use] + pub fn system_text(text: impl Into) -> Self { + Self { + role: MessageRole::System, + blocks: vec![ContentBlock::Text { text: text.into() }], + usage: None, + } + } + #[must_use] pub fn assistant(blocks: Vec) -> Self { Self { diff --git a/rust/crates/runtime/src/trident.rs b/rust/crates/runtime/src/trident.rs index 2346a4ea29..0757682249 100644 --- a/rust/crates/runtime/src/trident.rs +++ b/rust/crates/runtime/src/trident.rs @@ -29,7 +29,7 @@ impl Default for TridentConfig { } /// Statistics from a Trident compaction run. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Debug, Clone, PartialEq, Eq, Default)] pub struct TridentStats { pub superseded_count: usize, pub collapsed_chains: usize, @@ -41,21 +41,6 @@ pub struct TridentStats { pub final_message_count: usize, } -impl Default for TridentStats { - fn default() -> Self { - Self { - superseded_count: 0, - collapsed_chains: 0, - messages_collapsed: 0, - clusters_found: 0, - messages_clustered: 0, - tokens_saved_estimate: 0, - original_message_count: 0, - final_message_count: 0, - } - } -} - impl TridentStats { pub fn format_report(&self) -> String { let compression = if self.final_message_count > 0 { @@ -192,7 +177,7 @@ fn stage1_supersede(messages: &[ConversationMessage]) -> (Vec = BTreeSet::new(); - for (_path, ops) in &file_ops { + for ops in file_ops.values() { if ops.len() < 2 { continue; } @@ -205,10 +190,8 @@ fn stage1_supersede(messages: &[ConversationMessage]) -> (Vec = Vec::new(); let mut cluster_buffers: BTreeMap> = BTreeMap::new(); @@ -677,7 +660,7 @@ fn truncate_text(text: &str, max_chars: usize) -> String { mod tests { use super::*; use crate::compact::CompactionConfig; - use crate::session::{ContentBlock, ConversationMessage, MessageRole, Session}; + use crate::session::{ContentBlock, ConversationMessage, Session}; #[test] fn stage1_removes_obsolete_file_reads() { @@ -736,7 +719,7 @@ mod tests { fn stage2_collapses_chatty_messages() { let mut messages = vec![]; for i in 0..6 { - messages.push(ConversationMessage::user_text(&format!("ok {i}"))); + messages.push(ConversationMessage::user_text(format!("ok {i}"))); messages.push(ConversationMessage::assistant(vec![ContentBlock::Text { text: format!("got {i}"), }])); @@ -767,9 +750,9 @@ mod tests { }, ])); messages.push(ConversationMessage::tool_result( - &format!("read_{i}"), + format!("read_{i}"), "read_file", - &format!(r#"{{"path":"src/{i}.rs","content":"data {i}"}}"#), + format!(r#"{{"path":"src/{i}.rs","content":"data {i}"}}"#), false, )); } diff --git a/rust/crates/runtime/src/usage.rs b/rust/crates/runtime/src/usage.rs index 9241f7c2d7..ceafa7d0d7 100644 --- a/rust/crates/runtime/src/usage.rs +++ b/rust/crates/runtime/src/usage.rs @@ -89,6 +89,19 @@ impl TokenUsage { + self.cache_read_input_tokens } + /// Total tokens the provider had to read to serve this call, i.e. the live size of the + /// context window. + /// + /// `input_tokens` alone is NOT that number. With prompt caching a warm call reports + /// `input_tokens: 2` and puts the real bulk in `cache_read_input_tokens`, so anything + /// budgeting against the context window must sum all three input counters. + #[must_use] + pub fn total_input_tokens(self) -> u32 { + self.input_tokens + .saturating_add(self.cache_creation_input_tokens) + .saturating_add(self.cache_read_input_tokens) + } + #[must_use] pub fn estimate_cost_usd(self) -> UsageCostEstimate { self.estimate_cost_usd_with_pricing(ModelPricing::default_sonnet_tier()) @@ -243,6 +256,41 @@ mod tests { assert_eq!(tracker.cumulative_usage().total_tokens(), 48); } + #[test] + fn total_input_tokens_includes_cached_context() { + // The shape a warm prompt-cached call actually reports: input_tokens is ~nothing and + // the real context size is in the cache counters. + let cached = TokenUsage { + input_tokens: 2, + output_tokens: 1_133, + cache_creation_input_tokens: 1_453, + cache_read_input_tokens: 111_186, + }; + assert_eq!(cached.total_input_tokens(), 112_641); + // Output tokens are not part of the input budget. + assert_ne!(cached.total_input_tokens(), cached.total_tokens()); + + // A backend without prompt caching reports the whole prompt as input_tokens. + let uncached = TokenUsage { + input_tokens: 29_056, + output_tokens: 73, + cache_creation_input_tokens: 0, + cache_read_input_tokens: 0, + }; + assert_eq!(uncached.total_input_tokens(), 29_056); + } + + #[test] + fn total_input_tokens_saturates_instead_of_overflowing() { + let usage = TokenUsage { + input_tokens: u32::MAX, + output_tokens: 0, + cache_creation_input_tokens: u32::MAX, + cache_read_input_tokens: u32::MAX, + }; + assert_eq!(usage.total_input_tokens(), u32::MAX); + } + #[test] fn computes_cost_summary_lines() { let usage = TokenUsage { diff --git a/rust/crates/rusty-claude-cli/src/main.rs b/rust/crates/rusty-claude-cli/src/main.rs index 665ce632cf..2c11af42cd 100644 --- a/rust/crates/rusty-claude-cli/src/main.rs +++ b/rust/crates/rusty-claude-cli/src/main.rs @@ -72,6 +72,22 @@ use tools::{ const DEFAULT_MODEL: &str = "anthropic/claude-opus-4-7"; +/// Ceiling on model/tool round trips within one turn. +/// +/// `ConversationRuntime` defaults to `usize::MAX`, which for the interactive and `-p` paths means +/// no ceiling at all: a model that keeps re-reading the same file loops until the user kills it. +/// Spawned subagents have always been bounded; this gives the top-level loop the same treatment. +const DEFAULT_MAX_TURN_ITERATIONS: usize = 32; +const MAX_TURN_ITERATIONS_ENV_VAR: &str = "CLAW_MAX_TURN_ITERATIONS"; + +fn max_turn_iterations_from_env() -> usize { + std::env::var(MAX_TURN_ITERATIONS_ENV_VAR) + .ok() + .and_then(|raw| raw.trim().parse::().ok()) + .filter(|iterations| *iterations > 0) + .unwrap_or(DEFAULT_MAX_TURN_ITERATIONS) +} + /// #148: Model provenance for `claw status` JSON/text output. Records where /// the resolved model string came from so claws don't have to re-read argv /// to audit whether their `--model` flag was honored vs falling back to env @@ -1939,9 +1955,7 @@ fn parse_args(args: &[String]) -> Result { // Only reject for known top-level subcommands that don't use compact. let first = rest[0].as_str(); if is_known_top_level_subcommand(first) && first != "prompt" { - return Err(format!( - "invalid_flag_value: --compact is only supported with prompt mode.\nUsage: claw --compact \"\" or echo \"\" | claw --compact" - )); + return Err("invalid_flag_value: --compact is only supported with prompt mode.\nUsage: claw --compact \"\" or echo \"\" | claw --compact".to_string()); } } @@ -3134,10 +3148,11 @@ fn print_model_validation_warning_status( usage, permission_mode, context, - None, - None, - allowed_tools, - Some(&format_selection), + &StatusJsonExtras { + allowed_tools, + format_selection: Some(&format_selection), + ..StatusJsonExtras::default() + }, ); let object = value .as_object_mut() @@ -3213,9 +3228,7 @@ fn parse_system_prompt_args( })?; // #99: validate --date is a plausible date string (no newlines, reasonable length) if value.contains('\n') || value.contains('\r') { - return Err(format!( - "invalid_flag_value: --date value contains invalid characters.\nUsage: --date " - )); + return Err("invalid_flag_value: --date value contains invalid characters.\nUsage: --date ".to_string()); } if value.len() > 20 { return Err(format!( @@ -3452,11 +3465,7 @@ impl DiagnosticCheck { fn json_value(&self) -> Value { // Derive a stable snake_case id from the check name for machine-readable keying (#704). - let id = self - .name - .to_ascii_lowercase() - .replace(' ', "_") - .replace('-', "_"); + let id = self.name.to_ascii_lowercase().replace([' ', '-'], "_"); let mut value = Map::from_iter([ ("id".to_string(), Value::String(id.clone())), ( @@ -6589,10 +6598,8 @@ fn run_resume_command( }, default_permission_mode().as_str(), &context, - None, // #148: resumed sessions don't have flag provenance - None, - None, - None, + // #148: resumed sessions don't have flag provenance + &StatusJsonExtras::default(), )), }) } @@ -6730,16 +6737,15 @@ fn run_resume_command( } SlashCommand::Plugins { action, target } => { // Only list is supported in resume mode (no runtime to reload) - match action.as_deref() { - Some(action @ ("install" | "uninstall" | "enable" | "disable" | "update")) => { - // #777: use interactive_only: prefix + \n hint so #776's classify/split - // emits error_kind:interactive_only + non-null hint instead of unknown+null. - // Orchestrators can now detect this and switch to a live REPL instead of retrying. - return Err(format!( - "interactive_only: /plugins {action} requires a live session to reload the plugin runtime.\nStart `claw` and run `/plugins {action}` inside the REPL, or use `claw plugins {action}` as a direct CLI command." - ).into()); - } - _ => {} + if let Some(action @ ("install" | "uninstall" | "enable" | "disable" | "update")) = + action.as_deref() + { + // #777: use interactive_only: prefix + \n hint so #776's classify/split + // emits error_kind:interactive_only + non-null hint instead of unknown+null. + // Orchestrators can now detect this and switch to a live REPL instead of retrying. + return Err(format!( + "interactive_only: /plugins {action} requires a live session to reload the plugin runtime.\nStart `claw` and run `/plugins {action}` inside the REPL, or use `claw plugins {action}` as a direct CLI command." + ).into()); } let cwd = env::current_dir()?; let payload = plugins_command_payload_for( @@ -7783,6 +7789,15 @@ impl LiveCli { Ok(()) } Err(error) => { + // Salvage the transcript before the failed runtime is torn down. + // + // `prepare_turn_runtime` handed `run_turn` a CLONE of the session, and the turn + // mutated only that clone — appending the user message, each assistant reply, and + // a tool_result for every tool that actually executed. Dropping the clone throws + // all of that away while the side effects of those tools remain on disk, so the + // retry below would re-issue them against a session that shows no sign they ever + // ran. Adopting the clone keeps the record and the reality in step. + *self.runtime.session_mut() = runtime.session().clone(); runtime.shutdown_plugins()?; spinner.fail( "❌ Request failed", @@ -7802,37 +7817,17 @@ impl LiveCli { // This eliminates the need for users to manually run /compact when they // hit context limits - the recovery happens automatically. // - // Detection: We look for "context_window" or "Context window" in the error - // message, which covers error types like: - // - "context_window_blocked" - // - "Context window blocked" - // - "This model's maximum context length is X tokens..." + // Detection: the API layer classified this when it still had the typed error and + // the outgoing request in hand — see `AnthropicRuntimeClient::runtime_error_from_api`. + // It is deliberately NOT re-derived from the rendered message here: compaction + // discards conversation history, and the substrings that used to gate it + // ("error decoding response body", "Failed to parse input at pos") are emitted by + // ordinary transport failures as well as by overflow, so a dropped connection cost + // the session its transcript. // ============================================================================ let error_str = error.to_string(); - // Detect context window overflow. Some providers (e.g. OpenAI-compat backends) - // return 400 with "no parseable body" instead of a proper context_length_exceeded - // error when the request is too large to even parse — treat that as context overflow too. - // Also detect model-specific context error markers (e.g. llama.cpp returns - // "Context size has been exceeded." / "exceed_context_size_error" / "exceeds the available context size"). - let is_context_window = error_str.contains("context_window") - || error_str.contains("Context window") - || error_str.contains("no parseable body") - || error_str.contains("exceed_context_size") - || error_str.contains("exceeds the available context size") - || error_str - .to_ascii_lowercase() - .contains("context size has been exceeded"); - - // Also treat "assistant stream produced no content" and reqwest decode failures - // as recoverable errors that may benefit from auto-compaction. Some backends (e.g. - // llama.cpp) return a non-SSE HTTP 500 body when context overflows, causing - // reqwest to fail with "error decoding response body" — treat that as context overflow too. - let is_no_content = error_str.contains("assistant stream produced no content") - || error_str.contains("Failed to parse input at pos") - || error_str.contains("error decoding response body"); - - if is_context_window || is_no_content { + if error.is_context_window_failure() { // If the error tells us the server's actual context window, adapt our // auto-compaction threshold so future auto-compact-trigger checks are accurate. if let Some(window) = extract_context_window_tokens_from_error(&error_str) { @@ -7852,8 +7847,7 @@ impl LiveCli { let max_compact_rounds = 4; let preserve_schedule = [4, 2, 1, 0]; - for round in 0..max_compact_rounds { - let preserve = preserve_schedule[round]; + for (round, &preserve) in preserve_schedule.iter().enumerate() { println!( " Auto-compacting session (round {}/{}, preserving {} recent messages)...", round + 1, @@ -7900,7 +7894,11 @@ impl LiveCli { drop(hook_abort_monitor); let mut rp = CliPermissionPrompter::new(self.permission_mode); - match new_runtime.run_turn(input, Some(&mut rp)) { + // Resume, do not replay. The salvaged session already carries this turn's + // user message and every tool result that completed before the failure; + // re-running `run_turn(input, ..)` would duplicate the prompt and invite a + // second execution of tools that are not idempotent. + match new_runtime.resume_turn(Some(&mut rp)) { Ok(summary) => { self.replace_runtime(new_runtime)?; spinner.finish( @@ -7924,20 +7922,7 @@ impl LiveCli { } Err(retry_error) => { let retry_str = retry_error.to_string(); - let still_context_window = retry_str.contains("context_window") - || retry_str.contains("Context window") - || retry_str.contains("no parseable body") - || retry_str.contains("exceed_context_size") - || retry_str.contains("exceeds the available context size") - || retry_str - .to_ascii_lowercase() - .contains("context size has been exceeded"); - let still_no_content = retry_str - .contains("assistant stream produced no content") - || retry_str.contains("Failed to parse input at pos") - || retry_str.contains("error decoding response body"); - - if (still_context_window || still_no_content) + if retry_error.is_context_window_failure() && round + 1 < max_compact_rounds { // If the retry error reveals the context window, adapt threshold. @@ -8611,8 +8596,8 @@ impl LiveCli { let cwd = env::current_dir()?; // #803: reject flag-shaped tokens in list filter for BOTH text and JSON modes. // Previously the guard was JSON-only (#793); text mode silently returned empty success. - if action.as_deref() == Some("list") { - if let Some(filter) = target.as_deref() { + if action == Some("list") { + if let Some(filter) = target { if filter.starts_with('-') { if matches!(output_format, CliOutputFormat::Json) { // ROADMAP #817: this is a handled local inventory parse error. @@ -9567,31 +9552,49 @@ fn print_status_snapshot( usage, permission_mode.mode.as_str(), &context, - Some(&provenance), - Some(&permission_mode), - allowed_tools, - Some(&format_selection), + &StatusJsonExtras { + provenance: Some(&provenance), + permission_provenance: Some(&permission_mode), + allowed_tools, + format_selection: Some(&format_selection), + }, ))? ), } Ok(()) } +/// Optional provenance and selection inputs for [`status_json_value`]. +/// +/// Grouped into a struct rather than trailing positional parameters: they are all +/// `Option<&_>` and most callers pass `None` for most of them, which makes a +/// positional tail easy to transpose silently. +/// +/// `provenance` (#148) drives the `model_source` field ("flag" | "env" | "config" | +/// "default") and `model_raw` (user input before alias resolution, or null when the +/// source is "default"). Callers without provenance (legacy resume paths) leave it +/// `None`, in which case both fields are omitted. +#[derive(Default)] +struct StatusJsonExtras<'a> { + provenance: Option<&'a ModelProvenance>, + permission_provenance: Option<&'a PermissionModeProvenance>, + allowed_tools: Option<&'a AllowedToolSet>, + format_selection: Option<&'a OutputFormatSelection>, +} + fn status_json_value( model: Option<&str>, usage: StatusUsage, permission_mode: &str, context: &StatusContext, - // #148: optional provenance for `model` field. Surfaces `model_source` - // ("flag" | "env" | "config" | "default") and `model_raw` (user input - // before alias resolution, or null when source is "default"). Callers - // that don't have provenance (legacy resume paths) pass None, in which - // case both new fields are omitted. - provenance: Option<&ModelProvenance>, - permission_provenance: Option<&PermissionModeProvenance>, - allowed_tools: Option<&AllowedToolSet>, - format_selection: Option<&OutputFormatSelection>, + extras: &StatusJsonExtras<'_>, ) -> serde_json::Value { + let StatusJsonExtras { + provenance, + permission_provenance, + allowed_tools, + format_selection, + } = *extras; // #143: top-level `status` marker so claws can distinguish // a clean run from a degraded run (config parse failed but other fields // are still populated). `config_load_error` carries the parse-error string @@ -10073,9 +10076,7 @@ fn sandbox_json_value(status: &runtime::SandboxStatus) -> serde_json::Value { // (#731: "not supported on macOS" is a degraded state, not a hard error; // filesystem_active:true means partial containment is working) // error = enabled but unsupported AND no filesystem sandbox either (nothing active) - let top_status = if !status.enabled { - "ok" - } else if status.active { + let top_status = if !status.enabled || status.active { "ok" } else if status.supported { "warn" @@ -10450,18 +10451,20 @@ fn render_doctor_help_json() -> serde_json::Value { } /// #683-#692: extract structured metadata from help prose -fn extract_help_metadata( - topic: LocalHelpTopic, -) -> ( - Option, // usage - Option, // purpose - Option, // output description - Option>, // formats - Option>, // related - Option>, // aliases - bool, // local_only - bool, // requires_credentials -) { +/// Parsed fields of a help topic: usage, purpose, output description, formats, +/// related topics, aliases, local-only, requires-credentials. +type HelpMetadata = ( + Option, + Option, + Option, + Option>, + Option>, + Option>, + bool, + bool, +); + +fn extract_help_metadata(topic: LocalHelpTopic) -> HelpMetadata { let text = render_help_topic(topic); let mut usage = None; let mut purpose = None; @@ -12436,7 +12439,8 @@ fn build_runtime_with_plugin_state( policy, system_prompt, &feature_config, - ); + ) + .with_max_iterations(max_turn_iterations_from_env()); if emit_output { runtime = runtime.with_hook_progress_reporter(Box::new(CliHookProgressReporter)); } @@ -12674,7 +12678,49 @@ impl ApiClient for AnthropicRuntimeClient { } } +/// How full a request must be, as a percentage of the declared context window, before an +/// unreadable failure is taken to mean the window overflowed. +const AMBIGUOUS_FAILURE_WINDOW_PERCENT: u64 = 85; + +/// True when a request was close enough to the window that an otherwise unreadable failure is +/// most likely an overflow. +/// +/// This is the piece that lets transport errors be classified without matching on their prose. +/// A backend can answer an oversized request with a non-SSE 500 that reqwest reports as +/// "error decoding response body"; the same string also appears when the connection simply +/// dropped. What separates them is not the text, it is whether the request we just sent had any +/// room left. Requires a declared window — with none, every request looks small and this +/// correctly returns false. +fn request_fills_context_window(request: &MessageRequest) -> bool { + let Some(limit) = api::model_token_limit(&request.model) else { + return false; + }; + let estimated = + api::estimate_message_request_input_tokens(request).saturating_add(request.max_tokens); + u64::from(estimated) * 100 + >= u64::from(limit.context_window_tokens) * AMBIGUOUS_FAILURE_WINDOW_PERCENT +} + impl AnthropicRuntimeClient { + /// Converts an API failure into a runtime error, carrying the overflow verdict with it. + /// + /// The verdict is made here, where the error still has its type and the request is still in + /// hand, rather than reconstructed downstream from the rendered message. + fn runtime_error_from_api( + &self, + error: &api::ApiError, + request: &MessageRequest, + ) -> RuntimeError { + let message = format_user_visible_api_error(&self.session_id, error); + if error.is_context_window_failure() + || (error.is_ambiguous_transport_failure() && request_fills_context_window(request)) + { + RuntimeError::context_window(message) + } else { + RuntimeError::new(message) + } + } + /// Consume a single streaming response, optionally applying a stall /// timeout on the first event for post-tool continuations. #[allow(clippy::too_many_lines)] @@ -12687,9 +12733,7 @@ impl AnthropicRuntimeClient { .client .stream_message(message_request) .await - .map_err(|error| { - RuntimeError::new(format_user_visible_api_error(&self.session_id, &error)) - })?; + .map_err(|error| self.runtime_error_from_api(&error, message_request))?; let mut stdout = io::stdout(); let mut sink = io::sink(); let out: &mut dyn Write = if self.emit_output { @@ -12710,9 +12754,8 @@ impl AnthropicRuntimeClient { loop { let next = if apply_stall_timeout && !received_any_event { match tokio::time::timeout(POST_TOOL_STALL_TIMEOUT, stream.next_event()).await { - Ok(inner) => inner.map_err(|error| { - RuntimeError::new(format_user_visible_api_error(&self.session_id, &error)) - })?, + Ok(inner) => inner + .map_err(|error| self.runtime_error_from_api(&error, message_request))?, Err(_elapsed) => { return Err(RuntimeError::new( "post-tool stall: model did not respond within timeout", @@ -12720,9 +12763,10 @@ impl AnthropicRuntimeClient { } } } else { - stream.next_event().await.map_err(|error| { - RuntimeError::new(format_user_visible_api_error(&self.session_id, &error)) - })? + stream + .next_event() + .await + .map_err(|error| self.runtime_error_from_api(&error, message_request))? }; let Some(event) = next else { @@ -12862,9 +12906,7 @@ impl AnthropicRuntimeClient { ..message_request.clone() }) .await - .map_err(|error| { - RuntimeError::new(format_user_visible_api_error(&self.session_id, &error)) - })?; + .map_err(|error| self.runtime_error_from_api(&error, message_request))?; let mut events = response_to_events(response, out)?; push_prompt_cache_record(&self.client, &mut events); Ok(events) @@ -12988,8 +13030,8 @@ fn format_context_window_blocked_error(session_id: &str, error: &api::ApiError) )); lines.push(format!(" Context window {context_window_tokens} tokens")); } - api::ApiError::Api { message, body, .. } => { - let detail = message.as_deref().unwrap_or(body).trim(); + api::ApiError::Api(details) => { + let detail = details.message.as_deref().unwrap_or(&details.body).trim(); if !detail.is_empty() { lines.push(format!( " Detail {}", @@ -12999,7 +13041,7 @@ fn format_context_window_blocked_error(session_id: &str, error: &api::ApiError) } api::ApiError::RetriesExhausted { last_error, .. } => { let detail = match last_error.as_ref() { - api::ApiError::Api { message, body, .. } => message.as_deref().unwrap_or(body), + api::ApiError::Api(details) => details.message.as_deref().unwrap_or(&details.body), other => return format_context_window_blocked_error(session_id, other), } .trim(); @@ -14008,39 +14050,37 @@ fn convert_messages(messages: &[ConversationMessage]) -> Vec { let content = message .blocks .iter() - .filter_map(|block| match block { - ContentBlock::Text { text } => { - Some(InputContentBlock::Text { text: text.clone() }) - } + .map(|block| match block { + ContentBlock::Text { text } => InputContentBlock::Text { text: text.clone() }, ContentBlock::Thinking { thinking, signature, } => { // 保留 Thinking 块：OpenAI 兼容协议会把它转成 reasoning_content 字段 // 回传给 DeepSeek V4（避免 400 "reasoning_content must be passed back" 错误） - Some(InputContentBlock::Thinking { + InputContentBlock::Thinking { thinking: thinking.clone(), signature: signature.clone(), - }) + } } - ContentBlock::ToolUse { id, name, input } => Some(InputContentBlock::ToolUse { + ContentBlock::ToolUse { id, name, input } => InputContentBlock::ToolUse { id: id.clone(), name: name.clone(), input: serde_json::from_str(input) .unwrap_or_else(|_| serde_json::json!({ "raw": input })), - }), + }, ContentBlock::ToolResult { tool_use_id, output, is_error, .. - } => Some(InputContentBlock::ToolResult { + } => InputContentBlock::ToolResult { tool_use_id: tool_use_id.clone(), content: vec![ToolResultContentBlock::Text { text: output.clone(), }], is_error: *is_error, - }), + }, }) .collect::>(); (!content.is_empty()).then(|| InputMessage { @@ -14293,7 +14333,7 @@ mod tests { SessionLifecycleSummary, SlashCommand, StatusUsage, TmuxPaneSnapshot, DEFAULT_MODEL, LATEST_SESSION_REFERENCE, STUB_COMMANDS, }; - use api::{ApiError, MessageResponse, OutputContentBlock, Usage}; + use api::{ApiError, ApiErrorDetails, MessageResponse, OutputContentBlock, Usage}; use plugins::{ PluginManager, PluginManagerConfig, PluginTool, PluginToolDefinition, PluginToolPermission, }; @@ -14338,7 +14378,7 @@ mod tests { #[test] fn opaque_provider_wrapper_surfaces_failure_class_session_and_trace() { - let error = ApiError::Api { + let error = ApiError::Api(Box::new(ApiErrorDetails { status: "500".parse().expect("status"), error_type: Some("api_error".to_string()), message: Some( @@ -14350,7 +14390,7 @@ mod tests { retryable: true, suggested_action: None, retry_after: None, -}; +})); let rendered = format_user_visible_api_error("session-issue-22", &error); assert!(rendered.contains("provider_internal")); @@ -14362,7 +14402,7 @@ mod tests { fn retry_exhaustion_uses_retry_failure_class_for_generic_provider_wrapper() { let error = ApiError::RetriesExhausted { attempts: 3, - last_error: Box::new(ApiError::Api { + last_error: Box::new(ApiError::Api(Box::new(ApiErrorDetails { status: "502".parse().expect("status"), error_type: Some("api_error".to_string()), message: Some( @@ -14374,7 +14414,7 @@ mod tests { retryable: true, suggested_action: None, retry_after: None, -}), +}))), }; let rendered = format_user_visible_api_error("session-issue-22", &error); @@ -14427,7 +14467,7 @@ mod tests { #[test] fn provider_context_window_errors_are_reframed_with_same_guidance() { - let error = ApiError::Api { + let error = ApiError::Api(Box::new(ApiErrorDetails { status: "400".parse().expect("status"), error_type: Some("invalid_request_error".to_string()), message: Some( @@ -14439,7 +14479,7 @@ mod tests { retryable: false, suggested_action: None, retry_after: None, -}; +})); let rendered = format_user_visible_api_error("session-issue-32", &error); assert!(rendered.contains("context_window_blocked"), "{rendered}"); @@ -14461,7 +14501,7 @@ mod tests { #[test] fn openai_configured_limit_errors_are_rendered_as_context_window_guidance() { - let error = ApiError::Api { + let error = ApiError::Api(Box::new(ApiErrorDetails { status: "400".parse().expect("status"), error_type: Some("invalid_request_error".to_string()), message: Some( @@ -14473,7 +14513,7 @@ mod tests { retryable: false, suggested_action: None, retry_after: None, - }; + })); let rendered = format_user_visible_api_error("session-issue-32", &error); assert!(rendered.contains("Context window blocked"), "{rendered}"); @@ -14499,7 +14539,7 @@ mod tests { fn retry_wrapped_context_window_errors_keep_recovery_guidance() { let error = ApiError::RetriesExhausted { attempts: 2, - last_error: Box::new(ApiError::Api { + last_error: Box::new(ApiError::Api(Box::new(ApiErrorDetails { status: "413".parse().expect("status"), error_type: Some("invalid_request_error".to_string()), message: Some("Request is too large for this model's context window.".to_string()), @@ -14508,7 +14548,7 @@ mod tests { retryable: false, suggested_action: None, retry_after: None, - }), + }))), }; let rendered = format_user_visible_api_error("session-issue-32", &error); @@ -14555,11 +14595,54 @@ mod tests { ); } - fn env_lock() -> MutexGuard<'static, ()> { + /// Serialises tests that mutate process-global state, and isolates them from the + /// developer's real `~/.claw`. + /// + /// `parse_args` resolves defaults — notably `permissions.defaultMode` — through the + /// user-scope config, which `default_config_home()` reads from `CLAW_CONFIG_HOME`, + /// falling back to `$HOME/.claw`. Without an override, these tests therefore assert + /// against whatever the machine running them happens to have configured: a + /// `defaultMode` of `dontAsk` there resolves to `DangerFullAccess` and turns a dozen + /// `WorkspaceWrite` assertions red. Point the config home at a fresh empty directory + /// so "no user config" is the state under test, and restore the caller's value on drop. + /// + /// Tests that need a *populated* config home can still set `CLAW_CONFIG_HOME` + /// themselves after taking this guard; the guard restores the real value afterwards. + struct EnvGuard { + _lock: MutexGuard<'static, ()>, + config_home: PathBuf, + previous_config_home: Option, + } + + impl Drop for EnvGuard { + fn drop(&mut self) { + match self.previous_config_home.take() { + Some(previous) => std::env::set_var("CLAW_CONFIG_HOME", previous), + None => std::env::remove_var("CLAW_CONFIG_HOME"), + } + // Best effort: a leftover temp dir must never fail a test. + let _ = std::fs::remove_dir_all(&self.config_home); + } + } + + fn env_lock() -> EnvGuard { static LOCK: OnceLock> = OnceLock::new(); - LOCK.get_or_init(|| Mutex::new(())) + let lock = LOCK + .get_or_init(|| Mutex::new(())) .lock() - .unwrap_or_else(std::sync::PoisonError::into_inner) + .unwrap_or_else(std::sync::PoisonError::into_inner); + + // Deliberately empty: the point is that no user settings.json is discoverable. + let config_home = temp_dir(); + std::fs::create_dir_all(&config_home).expect("isolated config home should be creatable"); + let previous_config_home = std::env::var_os("CLAW_CONFIG_HOME"); + std::env::set_var("CLAW_CONFIG_HOME", &config_home); + + EnvGuard { + _lock: lock, + config_home, + previous_config_home, + } } fn with_current_dir(cwd: &Path, f: impl FnOnce() -> T) -> T { @@ -15286,6 +15369,8 @@ mod tests { #[test] fn removed_login_and_logout_subcommands_error_helpfully() { + // Asserts a `Default`-sourced permission mode, so it must not see a user config. + let _guard = env_lock(); let login = parse_args(&["login".to_string()]).expect_err("login should be removed"); assert!(login.contains("ANTHROPIC_API_KEY")); let logout = parse_args(&["logout".to_string()]).expect_err("logout should be removed"); @@ -15913,10 +15998,7 @@ mod tests { usage, "workspace-write", &context, - None, - None, - None, - None, + &super::StatusJsonExtras::default(), ); assert_eq!( json.get("status").and_then(|v| v.as_str()), @@ -15988,10 +16070,10 @@ mod tests { usage, "workspace-write", &context, - None, - None, - Some(&allowed), - None, + &super::StatusJsonExtras { + allowed_tools: Some(&allowed), + ..super::StatusJsonExtras::default() + }, ); assert_eq!( restricted_json @@ -16021,10 +16103,7 @@ mod tests { usage, "workspace-write", &clean_context, - None, - None, - None, - None, + &super::StatusJsonExtras::default(), ); assert_eq!( clean_json.get("status").and_then(|v| v.as_str()), @@ -16972,7 +17051,7 @@ mod tests { for action in ["remove", "uninstall", "delete"] { assert_eq!( parse_args(&["skills".to_string(), action.to_string()]) - .expect(&format!("skills {action} should parse")), + .unwrap_or_else(|_| panic!("skills {action} should parse")), CliAction::Skills { args: Some(action.to_string()), output_format: CliOutputFormat::Text, @@ -17926,10 +18005,7 @@ mod tests { }, "workspace-write", &context, - None, - None, - None, - None, + &super::StatusJsonExtras::default(), ); assert_eq!( diff --git a/rust/crates/rusty-claude-cli/src/setup_wizard.rs b/rust/crates/rusty-claude-cli/src/setup_wizard.rs index c2f7b6ff39..aa315dd69d 100644 --- a/rust/crates/rusty-claude-cli/src/setup_wizard.rs +++ b/rust/crates/rusty-claude-cli/src/setup_wizard.rs @@ -2,8 +2,6 @@ use std::io::{self, IsTerminal, Write}; use runtime::{save_user_provider_settings, ConfigLoader, RuntimeProviderConfig}; -use serde_json; - const PROVIDERS: &[(&str, &str, &str)] = &[ ("1", "Anthropic", "anthropic"), ("2", "xAI / Grok", "xai"), diff --git a/rust/crates/rusty-claude-cli/tests/output_format_contract.rs b/rust/crates/rusty-claude-cli/tests/output_format_contract.rs index c9ba752b03..35cba33217 100644 --- a/rust/crates/rusty-claude-cli/tests/output_format_contract.rs +++ b/rust/crates/rusty-claude-cli/tests/output_format_contract.rs @@ -451,11 +451,16 @@ fn direct_resume_safe_slash_commands_route_to_local_json_actions_831() { .output() .expect("git init should launch"); - for (command, expected_kind, expected_status) in [ - ("/version", "version", "ok"), - ("/sandbox", "sandbox", "warn"), - ("/diff", "diff", "ok"), - ("/status", "status", "ok"), + // `/sandbox`'s status reflects the host's real sandbox capability, so it is the one + // entry with more than one legitimate value: "ok" where namespace isolation works, + // "warn" where it is unsupported and only the filesystem sandbox is active (#731). + // This test is about #831 routing — that these commands produce a local JSON action + // instead of `interactive_only` — not about the kernel the suite happens to run on. + for (command, expected_kind, expected_statuses) in [ + ("/version", "version", &["ok"][..]), + ("/sandbox", "sandbox", &["ok", "warn"][..]), + ("/diff", "diff", &["ok"][..]), + ("/status", "status", &["ok"][..]), ] { let output = run_claw(&root, &["--output-format", "json", command], &[]); assert!( @@ -470,9 +475,9 @@ fn direct_resume_safe_slash_commands_route_to_local_json_actions_831() { .unwrap_or_else(|_| panic!("{command} must emit JSON (#831), got: {stdout:?}")); assert_eq!(parsed["kind"], expected_kind, "{command} kind: {parsed}"); - assert_eq!( - parsed["status"], expected_status, - "{command} status: {parsed}" + assert!( + expected_statuses.contains(&parsed["status"].as_str().unwrap_or_default()), + "{command} status should be one of {expected_statuses:?}: {parsed}" ); assert_ne!( parsed["error_kind"], "interactive_only", @@ -1007,13 +1012,13 @@ fn inventory_commands_emit_structured_json_when_requested() { assert!( !plugins .as_object() - .map_or(false, |o| o.contains_key("reload_runtime")), + .is_some_and(|o| o.contains_key("reload_runtime")), "plugins list should not include reload_runtime" ); assert!( !plugins .as_object() - .map_or(false, |o| o.contains_key("target")), + .is_some_and(|o| o.contains_key("target")), "plugins list should not include target" ); // #703: structured summary replaces prose message @@ -1706,13 +1711,13 @@ fn resumed_inventory_commands_emit_structured_json_when_requested() { assert!( !plugins .as_object() - .map_or(false, |o| o.contains_key("reload_runtime")), + .is_some_and(|o| o.contains_key("reload_runtime")), "plugins list should not include reload_runtime" ); assert!( !plugins .as_object() - .map_or(false, |o| o.contains_key("target")), + .is_some_and(|o| o.contains_key("target")), "plugins list should not include target" ); assert!( @@ -2945,7 +2950,7 @@ fn prompt_empty_arg_json_stdout_missing_prompt_823() { "claw prompt empty arg must retain abort action (#823); got: {parsed}" ); assert!( - parsed["hint"].as_str().map_or(false, |h| !h.is_empty()), + parsed["hint"].as_str().is_some_and(|h| !h.is_empty()), "claw prompt empty arg missing_prompt hint must be non-empty (#823)" ); } @@ -2983,9 +2988,9 @@ fn flag_value_errors_have_error_kind_and_hint_756() { "invalid --reasoning-effort must be invalid_flag_value (#756): {parsed}" ); assert!( - parsed["hint"].as_str().map_or(false, |h| h.contains("low") - || h.contains("medium") - || h.contains("high")), + parsed["hint"] + .as_str() + .is_some_and(|h| h.contains("low") || h.contains("medium") || h.contains("high")), "hint must mention valid values (#756): {parsed}" ); @@ -3011,7 +3016,7 @@ fn flag_value_errors_have_error_kind_and_hint_756() { "missing --model value must be missing_flag_value (#756): {parsed2}" ); assert!( - parsed2["hint"].as_str().map_or(false, |h| !h.is_empty()), + parsed2["hint"].as_str().is_some_and(|h| !h.is_empty()), "missing --model hint must be non-empty (#756): {parsed2}" ); } @@ -3255,7 +3260,7 @@ fn short_p_flag_swallows_no_flags_755() { "flag-like token after -p must be rejected as missing_prompt (#755): {parsed2}" ); assert!( - parsed2["hint"].as_str().map_or(false, |h| !h.is_empty()), + parsed2["hint"].as_str().is_some_and(|h| !h.is_empty()), "missing_prompt hint must be non-empty (#755)" ); } @@ -3397,7 +3402,7 @@ fn config_unsupported_section_json_hint_741() { assert!( parsed["supported_sections"] .as_array() - .map_or(false, |a| !a.is_empty()), + .is_some_and(|a| !a.is_empty()), "config {section} JSON must include supported_sections (#741)" ); } @@ -3454,7 +3459,7 @@ fn export_json_has_kind_702() { // On success stdout has kind:export; on failure stderr has type:error. // Either way, both envelopes must be valid JSON. let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr) + let _stderr = String::from_utf8_lossy(&output.stderr) .lines() .filter(|l| l.starts_with('{')) .collect::>() @@ -3546,8 +3551,8 @@ fn config_parse_error_has_typed_error_kind_and_hint_764() { !output.status.success(), "malformed settings.json should cause non-zero exit" ); - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stdout = String::from_utf8_lossy(&output.stdout); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_line = stdout .lines() @@ -3581,8 +3586,8 @@ fn login_logout_removed_subcommands_have_error_kind_and_hint_765() { !output.status.success(), "claw {subcmd} should exit non-zero" ); - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stdout = String::from_utf8_lossy(&output.stdout); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_line = stdout .lines() @@ -3721,8 +3726,8 @@ fn resume_non_slash_trailing_arg_has_typed_error_kind_and_hint_768() { !output.status.success(), "claw --resume latest compact should exit non-zero" ); - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stdout = String::from_utf8_lossy(&output.stdout); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_line = stdout .lines() @@ -3761,8 +3766,8 @@ fn session_with_unknown_subcommand_returns_interactive_only_not_credentials_767( !output.status.success(), "claw session {sub} should exit non-zero" ); - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stdout = String::from_utf8_lossy(&output.stdout); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_line = stdout .lines() @@ -3815,7 +3820,7 @@ fn slash_only_verbs_with_args_return_interactive_only_not_credentials_770() { "claw {} should exit non-zero", args.join(" ") ); - let stdout = String::from_utf8_lossy(&output.stdout); + let _stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_line = stdout @@ -3857,8 +3862,8 @@ fn agents_plugins_mcp_unknown_subcommand_have_hint_774() { { let output = run_claw(&root, &["--output-format", "json", "agents", "bogus"], &[]); assert!(!output.status.success(), "agents bogus should fail"); - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stdout = String::from_utf8_lossy(&output.stdout); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_line = stdout .lines() @@ -3881,8 +3886,8 @@ fn agents_plugins_mcp_unknown_subcommand_have_hint_774() { { let output = run_claw(&root, &["--output-format", "json", "plugins", "bogus"], &[]); assert!(!output.status.success(), "plugins bogus should fail"); - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stdout = String::from_utf8_lossy(&output.stdout); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_line = stdout .lines() @@ -3901,7 +3906,7 @@ fn agents_plugins_mcp_unknown_subcommand_have_hint_774() { { let output = run_claw(&root, &["--output-format", "json", "mcp", "bogus"], &[]); assert!(!output.status.success(), "mcp bogus should fail"); - let stdout = String::from_utf8_lossy(&output.stdout); + let _stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_str = if stdout.trim().starts_with('{') { @@ -4016,7 +4021,7 @@ fn interactive_only_guard_batch_769_to_771() { "claw {} should exit non-zero", args.join(" ") ); - let stdout = String::from_utf8_lossy(&output.stdout); + let _stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_line = stdout @@ -4080,7 +4085,7 @@ fn resume_plugin_mutations_are_typed_interactive_only_777() { !output.status.success(), "/plugins {mutation} in resume mode should exit non-zero" ); - let stdout = String::from_utf8_lossy(&output.stdout); + let _stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_line = stdout @@ -4137,7 +4142,7 @@ fn resume_skills_invocation_is_typed_interactive_only_779() { !output.status.success(), "/skills in resume mode should exit non-zero" ); - let stdout = String::from_utf8_lossy(&output.stdout); + let _stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_line = stdout @@ -4178,8 +4183,8 @@ fn acp_unsupported_invocation_has_hint_782() { let output = run_claw(&root, &["--output-format", "json", "acp", "start"], &[]); assert!(!output.status.success(), "acp start should fail"); - let stdout = String::from_utf8_lossy(&output.stdout); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stdout = String::from_utf8_lossy(&output.stdout); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let json_line = stdout .lines() @@ -4216,7 +4221,7 @@ fn init_json_envelope_has_hint_and_already_initialized_783() { // Fresh init — already_initialized should be false, hint should mention CLAUDE.md let output = run_claw(&root, &["--output-format", "json", "init"], &[]); assert!(output.status.success(), "init should succeed"); - let stdout = String::from_utf8_lossy(&output.stdout); + let _stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let raw = if stdout.trim_start().starts_with('{') { @@ -4285,7 +4290,7 @@ fn init_json_envelope_has_hint_and_already_initialized_783() { // Idempotent re-init — already_initialized should be true let output2 = run_claw(&root, &["--output-format", "json", "init"], &[]); assert!(output2.status.success(), "re-init should succeed"); - let stdout2 = String::from_utf8_lossy(&output2.stdout); + let _stdout2 = String::from_utf8_lossy(&output2.stdout); let stderr2 = String::from_utf8_lossy(&output2.stderr); let stdout2 = String::from_utf8_lossy(&output2.stdout); let raw2 = if stdout2.trim_start().starts_with('{') { @@ -4366,7 +4371,7 @@ fn export_arg_errors_have_typed_kind_and_hint_784() { &[], ); assert!(!out1.status.success(), "--output with no value should fail"); - let stderr1 = String::from_utf8_lossy(&out1.stderr); + let _stderr1 = String::from_utf8_lossy(&out1.stderr); let stdout1 = String::from_utf8_lossy(&out1.stdout); let j1: serde_json::Value = stdout1 .lines() @@ -4393,7 +4398,7 @@ fn export_arg_errors_have_typed_kind_and_hint_784() { &[], ); assert!(!out2.status.success(), "extra positional should fail"); - let stderr2 = String::from_utf8_lossy(&out2.stderr); + let _stderr2 = String::from_utf8_lossy(&out2.stderr); let stdout2 = String::from_utf8_lossy(&out2.stdout); let j2: serde_json::Value = stdout2 .lines() @@ -4430,7 +4435,7 @@ fn unknown_subcommand_returns_typed_kind_785() { // "dump" is close enough to "dump-manifests" to trigger the typo suggestion path let output = run_claw(&root, &["--output-format", "json", "dump"], &[]); assert!(!output.status.success(), "unknown subcommand should fail"); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let j: serde_json::Value = stdout .lines() @@ -4478,7 +4483,7 @@ fn dump_manifests_missing_dir_has_typed_kind_and_hint_786() { &[], ); assert!(!out1.status.success()); - let stderr1 = String::from_utf8_lossy(&out1.stderr); + let _stderr1 = String::from_utf8_lossy(&out1.stderr); let stdout1 = String::from_utf8_lossy(&out1.stdout); let j1: serde_json::Value = stdout1 .lines() @@ -4510,7 +4515,7 @@ fn dump_manifests_missing_dir_has_typed_kind_and_hint_786() { &[], ); assert!(!out2.status.success()); - let stderr2 = String::from_utf8_lossy(&out2.stderr); + let _stderr2 = String::from_utf8_lossy(&out2.stderr); let stdout2 = String::from_utf8_lossy(&out2.stdout); let j2: serde_json::Value = stdout2 .lines() @@ -4560,7 +4565,7 @@ fn resume_directory_path_returns_typed_kind_and_hint_787() { !output.status.success(), "resume with directory should fail" ); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let j: serde_json::Value = stdout .lines() @@ -4610,7 +4615,7 @@ fn skills_show_not_found_emits_single_json_object_788() { assert!(!output.status.success(), "skills show unknown should fail"); // Skills handler emits JSON to stdout; the duplicate was on stderr from the main error path. // After fix: stdout has 1 JSON object, stderr has none (no duplicate). - let stdout = String::from_utf8_lossy(&output.stdout); + let _stdout = String::from_utf8_lossy(&output.stdout); let stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); @@ -4772,7 +4777,7 @@ fn system_prompt_unknown_option_returns_typed_kind_790() { &[], ); assert!(!out1.status.success()); - let stderr1 = String::from_utf8_lossy(&out1.stderr); + let _stderr1 = String::from_utf8_lossy(&out1.stderr); let stdout1 = String::from_utf8_lossy(&out1.stdout); let j1: serde_json::Value = stdout1 .lines() @@ -4799,7 +4804,7 @@ fn system_prompt_unknown_option_returns_typed_kind_790() { &[], ); assert!(!out2.status.success()); - let stderr2 = String::from_utf8_lossy(&out2.stderr); + let _stderr2 = String::from_utf8_lossy(&out2.stderr); let stdout2 = String::from_utf8_lossy(&out2.stdout); let j2: serde_json::Value = stdout2 .lines() @@ -4837,7 +4842,7 @@ fn config_extra_args_have_non_null_hint_791() { &[], ); assert!(!out1.status.success()); - let stderr1 = String::from_utf8_lossy(&out1.stderr); + let _stderr1 = String::from_utf8_lossy(&out1.stderr); let stdout1 = String::from_utf8_lossy(&out1.stdout); let j1: serde_json::Value = stdout1 .lines() @@ -4871,7 +4876,7 @@ fn config_extra_args_have_non_null_hint_791() { &[], ); assert!(!out2.status.success()); - let stderr2 = String::from_utf8_lossy(&out2.stderr); + let _stderr2 = String::from_utf8_lossy(&out2.stderr); let stdout2 = String::from_utf8_lossy(&out2.stdout); let j2: serde_json::Value = stdout2 .lines() @@ -5057,7 +5062,7 @@ fn plugins_uninstall_not_found_has_hint_793() { "plugins uninstall not-found must exit non-zero (#793)" ); // Error envelope goes to stderr (propagated via ? to main error handler) - let stderr = String::from_utf8_lossy(&output.stderr); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let j: serde_json::Value = stdout .lines() @@ -5102,7 +5107,7 @@ fn plugins_install_not_found_path_returns_typed_kind_794() { !output.status.success(), "plugins install not-found-path must exit non-zero (#794)" ); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let j: serde_json::Value = stdout .lines() @@ -5366,7 +5371,7 @@ fn agents_create_scaffolds_toml_and_lists_locally_431() { .iter() .any(|agent| { agent["name"] == "my-agent" - && PathBuf::from(agent["path"].as_str().expect("listed agent path")) + && Path::new(agent["path"].as_str().expect("listed agent path")) == fs::canonicalize(&agent_path).expect("canonical listed agent path") })); } @@ -5491,7 +5496,7 @@ fn plugins_extra_args_have_non_null_hint_797() { !output.status.success(), "plugins show with extra arg must exit non-zero (#797)" ); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let j: serde_json::Value = stdout .lines() @@ -5557,7 +5562,7 @@ fn plugins_list_trailing_dash_text_error_stays_on_stderr_817() { String::from_utf8_lossy(&output.stdout) ); let stderr = String::from_utf8_lossy(&output.stderr); - let stdout = String::from_utf8_lossy(&output.stdout); + let _stdout = String::from_utf8_lossy(&output.stdout); assert!(stderr.contains("[error-kind: cli_parse]"), "{stderr}"); assert!( stderr.contains("unknown option for `claw plugins list`: --"), @@ -5582,7 +5587,7 @@ fn empty_prompt_has_non_null_hint_798() { !output.status.success(), "empty prompt must exit non-zero (#798)" ); - let stderr = String::from_utf8_lossy(&output.stderr); + let _stderr = String::from_utf8_lossy(&output.stderr); let stdout = String::from_utf8_lossy(&output.stdout); let j: serde_json::Value = stdout .lines() diff --git a/rust/crates/tools/src/lib.rs b/rust/crates/tools/src/lib.rs index a72261ed02..51a0b570cb 100644 --- a/rust/crates/tools/src/lib.rs +++ b/rust/crates/tools/src/lib.rs @@ -506,13 +506,17 @@ pub fn mvp_tool_specs() -> Vec { }, ToolSpec { name: "read_file", - description: "Read a text file from the workspace.", + description: "Read a text file from the workspace. Large files come back one page at \ + a time: if the response has \"truncated\": true you have NOT seen the \ + whole file, and \"nextOffset\" is the line to pass as `offset` to \ + continue. Prefer grep_search to locate what you need over paging \ + through a big file, and never re-read a page you already have.", input_schema: json!({ "type": "object", "properties": { "path": { "type": "string" }, - "offset": { "type": "integer", "minimum": 0 }, - "limit": { "type": "integer", "minimum": 1 } + "offset": { "type": "integer", "minimum": 0, "description": "First line to read (0-based). Pass the previous response's nextOffset to continue." }, + "limit": { "type": "integer", "minimum": 1, "description": "Maximum lines to return. Capped by a byte ceiling regardless." } }, "required": ["path"], "additionalProperties": false @@ -10271,6 +10275,12 @@ mod tests { #[test] fn repl_executes_python_code() { + // Resolving the python runtime reads PATH, and PATH is process-global: the PowerShell + // tests below blank it while they run. Without this guard that races and this test + // intermittently fails with "python runtime not found" on a machine that has python. + let _guard = env_lock() + .lock() + .unwrap_or_else(std::sync::PoisonError::into_inner); let result = execute_tool( "REPL", &json!({"language": "python", "code": "print(1 + 1)", "timeout_ms": 500}),