fix(tools): coerce zero http_request timeout/size + migrate stale-zero configs (5→6) (#2760)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

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)]

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}"
     );
 }
 

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<MigrationStats> {
+    // 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);
+    }
+}

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),

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"]);

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<SecurityPolicy>,
     allowed_domains: Vec<String>,
@@ -31,11 +29,45 @@ impl WebFetchTool {
         max_bytes: Option<usize>,
         timeout_secs: Option<u64>,
     ) -> 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);