diff --git a/src/openhuman/migrations/mod.rs b/src/openhuman/migrations/mod.rs index 38b42e688..69dd24ea7 100644 --- a/src/openhuman/migrations/mod.rs +++ b/src/openhuman/migrations/mod.rs @@ -26,11 +26,12 @@ use crate::openhuman::config::Config; mod expand_autonomy_defaults; mod phase_out_profile_md; mod remove_write_auto_approve; +mod repair_http_request_limits; mod retire_chat_v1_model; mod unify_ai_provider_settings; /// Current target schema version. Bumped alongside every new migration. -pub const CURRENT_SCHEMA_VERSION: u32 = 5; +pub const CURRENT_SCHEMA_VERSION: u32 = 6; /// Run any migrations whose `schema_version` gate hasn't yet been /// crossed for this workspace. @@ -250,6 +251,50 @@ pub async fn run_pending(config: &mut Config) { } } } + + // 5 -> 6: repair stale-zero `[http_request]` limits. Older builds could + // persist `timeout_secs = 0` / `max_response_size = 0`, which the network + // tools apply literally — `Duration::from_secs(0)` is an instant timeout + // that fails every web_fetch/http_request, and a 0-byte cap truncates + // every body. serde defaults only fill *missing* keys, so a persisted 0 + // survives an update. Coerce to schema defaults (30s / 1 MB). Guard on + // `== 5` so an earlier failed step doesn't get skipped. + // + // NOTE: another in-flight migration (`reconcile_orphaned_providers`) also + // targets the 5 -> 6 transition. When that lands, both modules run inside + // this single `== 5` branch before the version is bumped to 6 — keep them + // as separate modules, one shared version bump. The tool constructors + // also clamp 0 at the point of use, so a user who crosses this gate via + // the other migration without running this one still gets working fetches. + if config.schema_version == 5 { + match repair_http_request_limits::run(config) { + Ok(stats) => { + let previous_version = config.schema_version; + config.schema_version = 6; + if let Err(err) = config.save().await { + config.schema_version = previous_version; + log::warn!( + "[migrations] repair_http_request_limits ran but config.save failed: \ + {err:#} — rolled in-memory schema_version back to {previous_version}, \ + will retry on next launch" + ); + return; + } + log::info!( + "[migrations] schema_version bumped to 6 (repair_http_request_limits \ + timeout_repaired={} max_response_size_repaired={})", + stats.timeout_repaired, + stats.max_response_size_repaired, + ); + } + Err(err) => { + log::warn!( + "[migrations] repair_http_request_limits failed: {err:#} — \ + will retry on next launch" + ); + } + } + } } #[cfg(test)] diff --git a/src/openhuman/migrations/mod_tests.rs b/src/openhuman/migrations/mod_tests.rs index 58e26c7fd..52d423cb8 100644 --- a/src/openhuman/migrations/mod_tests.rs +++ b/src/openhuman/migrations/mod_tests.rs @@ -112,8 +112,8 @@ async fn run_pending_runs_phase_out_when_version_zero() { let on_disk = std::fs::read_to_string(&config.config_path).unwrap(); assert!( - on_disk.contains("schema_version = 5"), - "saved config.toml must record schema_version=5, got:\n{on_disk}" + on_disk.contains("schema_version = 6"), + "saved config.toml must record schema_version=6, got:\n{on_disk}" ); } @@ -128,7 +128,7 @@ async fn run_pending_bumps_version_on_fresh_install() { assert_eq!(config.schema_version, CURRENT_SCHEMA_VERSION); let on_disk = std::fs::read_to_string(&config.config_path).unwrap(); - assert!(on_disk.contains("schema_version = 5")); + assert!(on_disk.contains("schema_version = 6")); } #[tokio::test] @@ -252,8 +252,8 @@ async fn run_pending_expands_autonomy_defaults_from_v3() { // On-disk config must reflect the new schema_version. let on_disk = fs::read_to_string(&config.config_path).unwrap(); assert!( - on_disk.contains("schema_version = 5"), - "saved config.toml must record schema_version=5, got:\n{on_disk}" + on_disk.contains("schema_version = 6"), + "saved config.toml must record schema_version=6, got:\n{on_disk}" ); } @@ -285,8 +285,44 @@ async fn run_pending_v4_to_v5_removes_write_tools_from_auto_approve() { let on_disk = fs::read_to_string(&config.config_path).unwrap(); assert!( - on_disk.contains("schema_version = 5"), - "saved config.toml must record schema_version=5, got:\n{on_disk}" + on_disk.contains("schema_version = 6"), + "saved config.toml must record schema_version=6, got:\n{on_disk}" + ); +} + +// ── v5 → v6: repair_http_request_limits integration test ──────────────────── + +/// A workspace persisted at schema_version=5 with stale-zero `[http_request]` +/// limits (the exact bug this PR fixes) must be repaired to the schema +/// defaults and bumped to v6 by the full `run_pending` wiring — not just the +/// migration's own unit tests. +#[tokio::test] +async fn run_pending_v5_to_v6_repairs_http_request_limits() { + let tmp = TempDir::new().unwrap(); + fs::create_dir_all(tmp.path().join("workspace")).unwrap(); + + let mut config = config_in(&tmp); + config.schema_version = 5; + config.http_request.timeout_secs = 0; + config.http_request.max_response_size = 0; + + run_pending(&mut config).await; + + let defaults = crate::openhuman::config::HttpRequestConfig::default(); + assert_eq!(config.schema_version, CURRENT_SCHEMA_VERSION); + assert_eq!(config.http_request.timeout_secs, defaults.timeout_secs); + assert_eq!( + config.http_request.max_response_size, + defaults.max_response_size + ); + assert_ne!(config.http_request.timeout_secs, 0); + assert_ne!(config.http_request.max_response_size, 0); + + // The version bump must be persisted to disk too. + let on_disk = fs::read_to_string(&config.config_path).unwrap(); + assert!( + on_disk.contains("schema_version = 6"), + "saved config.toml must record schema_version=6, got:\n{on_disk}" ); } diff --git a/src/openhuman/migrations/repair_http_request_limits.rs b/src/openhuman/migrations/repair_http_request_limits.rs new file mode 100644 index 000000000..fa5f09f08 --- /dev/null +++ b/src/openhuman/migrations/repair_http_request_limits.rs @@ -0,0 +1,109 @@ +//! Migration 5 -> 6: repair stale-zero `[http_request]` limits. +//! +//! Older builds could persist `http_request.timeout_secs = 0` and/or +//! `http_request.max_response_size = 0` (the plain numeric default before +//! the custom serde defaults landed). The network tools apply these +//! literally: `reqwest`'s `Duration::from_secs(0)` is an *instant* timeout +//! that fails every `web_fetch` / `http_request`, and a 0-byte cap +//! truncates every response body to nothing. `#[serde(default = …)]` only +//! fills *missing* keys, so a persisted `0` is taken as-is and silently +//! survives an app update — there is no way to express "blocked" or +//! "unlimited" with these zeros, only "broken". +//! +//! Coerce any `0` back to the schema default (30s / 1 MB). The tool +//! constructors also clamp `0` at the point of use (defence in depth); this +//! migration additionally repairs the persisted `config.toml` so it stops +//! shipping the misleading zeros to every consumer. + +use crate::openhuman::config::{Config, HttpRequestConfig}; + +#[derive(Debug, Default, Clone, Copy, PartialEq, Eq)] +pub struct MigrationStats { + pub timeout_repaired: bool, + pub max_response_size_repaired: bool, +} + +/// Returns `anyhow::Result` to match the shared migration-step signature that +/// [`crate::openhuman::migrations::run_pending`] dispatches (`Ok` → bump + +/// save, `Err` → log + retry next launch). This step is a pure in-memory +/// transform and is currently infallible, but keeping the `Result` lets a +/// future I/O-backed repair slot in without churning the runner or callers. +pub fn run(config: &mut Config) -> anyhow::Result { + // Source the replacement values from the schema's own defaults so this + // migration can't drift from `HttpRequestConfig`'s canonical defaults. + let defaults = HttpRequestConfig::default(); + let mut stats = MigrationStats::default(); + + if config.http_request.timeout_secs == 0 { + config.http_request.timeout_secs = defaults.timeout_secs; + stats.timeout_repaired = true; + } + if config.http_request.max_response_size == 0 { + config.http_request.max_response_size = defaults.max_response_size; + stats.max_response_size_repaired = true; + } + + log::info!( + "[migrations][repair-http-request-limits] done timeout_repaired={} \ + max_response_size_repaired={}", + stats.timeout_repaired, + stats.max_response_size_repaired + ); + + Ok(stats) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::openhuman::config::Config; + + #[test] + fn repairs_zero_timeout_and_size() { + let mut config = Config::default(); + config.http_request.timeout_secs = 0; + config.http_request.max_response_size = 0; + + let stats = run(&mut config).expect("migration should succeed"); + + let defaults = HttpRequestConfig::default(); + assert!(stats.timeout_repaired); + assert!(stats.max_response_size_repaired); + assert_eq!(config.http_request.timeout_secs, defaults.timeout_secs); + assert_eq!( + config.http_request.max_response_size, + defaults.max_response_size + ); + // The whole point: no zeros survive. + assert_ne!(config.http_request.timeout_secs, 0); + assert_ne!(config.http_request.max_response_size, 0); + } + + #[test] + fn repairs_only_the_zero_field() { + let mut config = Config::default(); + config.http_request.timeout_secs = 0; + config.http_request.max_response_size = 2_000_000; + + let stats = run(&mut config).expect("migration should succeed"); + + assert!(stats.timeout_repaired); + assert!(!stats.max_response_size_repaired); + assert_ne!(config.http_request.timeout_secs, 0); + assert_eq!(config.http_request.max_response_size, 2_000_000); + } + + #[test] + fn leaves_nonzero_values_untouched() { + let mut config = Config::default(); + config.http_request.timeout_secs = 45; + config.http_request.max_response_size = 3_000_000; + + let stats = run(&mut config).expect("migration should succeed"); + + assert!(!stats.timeout_repaired); + assert!(!stats.max_response_size_repaired); + assert_eq!(config.http_request.timeout_secs, 45); + assert_eq!(config.http_request.max_response_size, 3_000_000); + } +} diff --git a/src/openhuman/tools/impl/network/http_request.rs b/src/openhuman/tools/impl/network/http_request.rs index 4a9078724..328b9a1c6 100644 --- a/src/openhuman/tools/impl/network/http_request.rs +++ b/src/openhuman/tools/impl/network/http_request.rs @@ -1,4 +1,5 @@ use super::url_guard::{normalize_allowed_domains, validate_url_with_dns_check}; +use crate::openhuman::config::HttpRequestConfig; use crate::openhuman::security::SecurityPolicy; use crate::openhuman::tools::traits::{Tool, ToolResult}; use async_trait::async_trait; @@ -22,6 +23,33 @@ impl HttpRequestTool { max_response_size: usize, timeout_secs: u64, ) -> Self { + // Treat `0` as "use default": a 0-byte cap or 0-second timeout is never + // a meaningful limit, only a footgun (see migration 5→6). Pull the + // fallbacks from `HttpRequestConfig::default()` so the tool, the schema + // default, and the migration share one source and can't drift. A `0` + // here means a stale/invalid config slipped past the migration, so + // surface it with a stable, grep-friendly, non-sensitive log line. + let defaults = HttpRequestConfig::default(); + let max_response_size = if max_response_size == 0 { + log::warn!( + "[tool.http_request] coercing invalid limit field=max_response_size \ + from=0 to={} (stale/invalid config — see migration 5→6)", + defaults.max_response_size + ); + defaults.max_response_size + } else { + max_response_size + }; + let timeout_secs = if timeout_secs == 0 { + log::warn!( + "[tool.http_request] coercing invalid limit field=timeout_secs \ + from=0 to={} (stale/invalid config — see migration 5→6)", + defaults.timeout_secs + ); + defaults.timeout_secs + } else { + timeout_secs + }; Self { security, allowed_domains: normalize_allowed_domains(allowed_domains), diff --git a/src/openhuman/tools/impl/network/http_request_tests.rs b/src/openhuman/tools/impl/network/http_request_tests.rs index c35c1c05d..8ba216da8 100644 --- a/src/openhuman/tools/impl/network/http_request_tests.rs +++ b/src/openhuman/tools/impl/network/http_request_tests.rs @@ -14,6 +14,34 @@ fn test_tool(allowed_domains: Vec<&str>) -> HttpRequestTool { ) } +#[test] +fn zero_limits_fall_back_to_defaults() { + // Stale configs (or a bad write) can pass 0; a 0-second timeout fails + // every request and a 0-byte cap truncates every body. The constructor + // must coerce both to the module defaults — never let 0 reach reqwest. + let security = Arc::new(SecurityPolicy { + autonomy: AutonomyLevel::Supervised, + ..SecurityPolicy::default() + }); + let tool = HttpRequestTool::new(security, vec!["example.com".to_string()], 0, 0); + let defaults = crate::openhuman::config::HttpRequestConfig::default(); + assert_eq!(tool.max_response_size, defaults.max_response_size); + assert_eq!(tool.timeout_secs, defaults.timeout_secs); + assert_ne!(tool.timeout_secs, 0); + assert_ne!(tool.max_response_size, 0); +} + +#[test] +fn nonzero_limits_are_preserved() { + let security = Arc::new(SecurityPolicy { + autonomy: AutonomyLevel::Supervised, + ..SecurityPolicy::default() + }); + let tool = HttpRequestTool::new(security, vec!["example.com".to_string()], 2048, 12); + assert_eq!(tool.max_response_size, 2048); + assert_eq!(tool.timeout_secs, 12); +} + #[test] fn validate_accepts_valid_methods() { let tool = test_tool(vec!["example.com"]); diff --git a/src/openhuman/tools/impl/network/web_fetch.rs b/src/openhuman/tools/impl/network/web_fetch.rs index f78ff2e67..20226666b 100644 --- a/src/openhuman/tools/impl/network/web_fetch.rs +++ b/src/openhuman/tools/impl/network/web_fetch.rs @@ -7,6 +7,7 @@ //! as text, capped, with a tiny preamble (status + final URL). use super::url_guard::{normalize_allowed_domains, validate_url_with_dns_check}; +use crate::openhuman::config::HttpRequestConfig; use crate::openhuman::security::SecurityPolicy; use crate::openhuman::tools::traits::{PermissionLevel, Tool, ToolResult}; use async_trait::async_trait; @@ -14,9 +15,6 @@ use serde_json::json; use std::sync::Arc; use std::time::Duration; -const DEFAULT_MAX_BYTES: usize = 1_000_000; -const DEFAULT_TIMEOUT_SECS: u64 = 20; - pub struct WebFetchTool { security: Arc, allowed_domains: Vec, @@ -31,11 +29,45 @@ impl WebFetchTool { max_bytes: Option, timeout_secs: Option, ) -> Self { + // Treat both `None` and `Some(0)` as "use default": callers wire these + // from `[http_request]`, and a 0-byte cap truncates every body to + // nothing while a 0-second timeout fails every request instantly. + // Stale-zero configs are repaired on load (migration 5→6); this clamp + // is the always-on guard at the point of use. Pull the fallbacks from + // `HttpRequestConfig::default()` so the tool shares one source with the + // schema + migration (no cross-layer drift). `Some(0)` is a genuine + // misconfiguration, so log it (grep-friendly, no payload); a bare + // `None` is a normal "use default" call and stays quiet. + let defaults = HttpRequestConfig::default(); + let max_bytes = match max_bytes { + Some(0) => { + log::warn!( + "[tool.web_fetch] coercing invalid limit field=max_bytes \ + from=0 to={} (stale/invalid config — see migration 5→6)", + defaults.max_response_size + ); + defaults.max_response_size + } + Some(n) => n, + None => defaults.max_response_size, + }; + let timeout_secs = match timeout_secs { + Some(0) => { + log::warn!( + "[tool.web_fetch] coercing invalid limit field=timeout_secs \ + from=0 to={} (stale/invalid config — see migration 5→6)", + defaults.timeout_secs + ); + defaults.timeout_secs + } + Some(n) => n, + None => defaults.timeout_secs, + }; Self { security, allowed_domains: normalize_allowed_domains(allowed_domains), - max_bytes: max_bytes.unwrap_or(DEFAULT_MAX_BYTES), - timeout_secs: timeout_secs.unwrap_or(DEFAULT_TIMEOUT_SECS), + max_bytes, + timeout_secs, } } } @@ -195,6 +227,40 @@ mod tests { .contains(&json!("url"))); } + #[test] + fn zero_and_none_limits_fall_back_to_defaults() { + // Callers wire these from `[http_request]`; a stale `Some(0)` is a + // 0-byte cap (empty bodies) and a 0-second timeout (instant failure). + // Both `None` and `Some(0)` must coerce to the shared schema defaults. + let defaults = crate::openhuman::config::HttpRequestConfig::default(); + let from_zero = WebFetchTool::new( + test_security(), + vec!["example.com".into()], + Some(0), + Some(0), + ); + assert_eq!(from_zero.max_bytes, defaults.max_response_size); + assert_eq!(from_zero.timeout_secs, defaults.timeout_secs); + assert_ne!(from_zero.timeout_secs, 0); + assert_ne!(from_zero.max_bytes, 0); + + let from_none = WebFetchTool::new(test_security(), vec!["example.com".into()], None, None); + assert_eq!(from_none.max_bytes, defaults.max_response_size); + assert_eq!(from_none.timeout_secs, defaults.timeout_secs); + } + + #[test] + fn nonzero_limits_are_preserved() { + let tool = WebFetchTool::new( + test_security(), + vec!["example.com".into()], + Some(4096), + Some(15), + ); + assert_eq!(tool.max_bytes, 4096); + assert_eq!(tool.timeout_secs, 15); + } + #[tokio::test] async fn web_fetch_rejects_disallowed_domain() { let tool = WebFetchTool::new(test_security(), vec!["example.com".into()], None, None);