fix(tools): coerce zero http_request timeout/size + migrate stale-zero configs (5→6)#2760
Conversation
…o configs (5→6) A persisted `[http_request].timeout_secs = 0` produced a 0-second reqwest timeout (`Duration::from_secs(0)`) that failed every `web_fetch`/`http_request` instantly; `max_response_size = 0` truncated every body to nothing. Because `#[serde(default = …)]` only fills *missing* keys, these stale zeros — written by older builds — survived app updates with nothing to repair them, so the allowlist could be "Allow all" yet every fetch still failed. - http_request/web_fetch constructors coerce `0 → schema default` (always-on guard: 0 can never reach reqwest regardless of how the config got there) - new migration 5→6 (`repair_http_request_limits`) rewrites persisted zeros to `HttpRequestConfig::default()` (30s / 1 MB) and tidies the on-disk config - tests: tool guards (zero→default, nonzero preserved) + migration unit tests Note: `reconcile_orphaned_providers` also targets 5→6; on integration both modules run inside the single `== 5` branch (one shared bump to 6). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a v5→v6 migration repairing persisted HTTP request limits stored as zero, updates the migration runner and tests to target schema version 6, and coerces zero-valued limits to safe defaults in HttpRequestTool and WebFetchTool constructors with tests. ChangesHTTP Request Limits Zero-Value Repair
Sequence Diagram(s)sequenceDiagram
participant MigrationRunner
participant repair_http_request_limits
participant Config
MigrationRunner->>repair_http_request_limits: run(config)
repair_http_request_limits-->>MigrationRunner: MigrationStats(timeout_repaired,max_response_size_repaired)
MigrationRunner->>Config: set schema_version = 6
MigrationRunner->>Config: save()
Config-->>MigrationRunner: Ok / Err
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
coderabbitai
Bot
added
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/openhuman/tools/impl/network/http_request.rs (1)
9-18: ⚡ Quick winAvoid duplicating schema defaults in tool-local literals.
Line 12 and Line 18 hardcode values that are documented as schema mirrors. Prefer pulling fallback values from a single source (e.g.,
HttpRequestConfig::default()or shared config constants) so migration/default/tool behavior cannot drift again.Also applies to: 41-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/tools/impl/network/http_request.rs` around lines 9 - 18, The two tool-local constants DEFAULT_MAX_RESPONSE_SIZE and DEFAULT_TIMEOUT_SECS duplicate schema defaults; replace their hardcoded literals by referencing the canonical defaults (e.g., from HttpRequestConfig::default() or the shared config constants) when computing the fallback in truncate_response/timeout handling and the other occurrences around 41-50 so the tool uses a single source of truth and avoids drift; update code paths that currently read DEFAULT_MAX_RESPONSE_SIZE or DEFAULT_TIMEOUT_SECS to derive values from HttpRequestConfig::default() (or the exported shared constants) instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/tools/impl/network/http_request.rs`:
- Around line 39-50: The branch that coerces zero inputs to defaults
(max_response_size and timeout_secs) silently rewrites invalid values; add
structured, grep-friendly logs when coercion occurs: emit a log entry before
returning the coerced value that includes stable keys like "tool=http_request",
"field=max_response_size" or "field=timeout_secs", "coerced=true", "from=0", and
"to=<DEFAULT_*>" (use DEFAULT_MAX_RESPONSE_SIZE and DEFAULT_TIMEOUT_SECS symbol
names rather than raw values) and ensure no secrets/PII are logged; place these
logs adjacent to the existing conditionals that set max_response_size and
timeout_secs so callers of the constructor/initializer for the HTTP request can
trace stale/bad config sources.
In `@src/openhuman/tools/impl/network/web_fetch.rs`:
- Around line 37-45: When clamping max_bytes and timeout_secs to
DEFAULT_MAX_BYTES / DEFAULT_TIMEOUT_SECS in the constructor (the lines setting
max_bytes: max_bytes.filter(|n| *n > 0).unwrap_or(DEFAULT_MAX_BYTES) and
timeout_secs: timeout_secs.filter(|n| *n > 0).unwrap_or(DEFAULT_TIMEOUT_SECS)),
add stable-prefix structured diagnostic logs that emit when a clamp happens
(i.e., when input was None or Some(0)); log a grep-friendly prefix like
"web_fetch:clamped:" and include non-sensitive correlation fields (e.g., a
request_id or component_id if available), the field name ("max_bytes" or
"timeout_secs"), the original raw value presence/state (e.g., "none" or "zero")
and the effective default used, ensuring no secrets/PII are logged; use the
project's existing logging facility (tracing/log/processLogger) so these
messages occur at info/warn level per guidelines.
---
Nitpick comments:
In `@src/openhuman/tools/impl/network/http_request.rs`:
- Around line 9-18: The two tool-local constants DEFAULT_MAX_RESPONSE_SIZE and
DEFAULT_TIMEOUT_SECS duplicate schema defaults; replace their hardcoded literals
by referencing the canonical defaults (e.g., from HttpRequestConfig::default()
or the shared config constants) when computing the fallback in
truncate_response/timeout handling and the other occurrences around 41-50 so the
tool uses a single source of truth and avoids drift; update code paths that
currently read DEFAULT_MAX_RESPONSE_SIZE or DEFAULT_TIMEOUT_SECS to derive
values from HttpRequestConfig::default() (or the exported shared constants)
instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a97b2bd8-9ab6-4285-b203-0554aa6c60d9
📒 Files selected for processing (6)
src/openhuman/migrations/mod.rssrc/openhuman/migrations/mod_tests.rssrc/openhuman/migrations/repair_http_request_limits.rssrc/openhuman/tools/impl/network/http_request.rssrc/openhuman/tools/impl/network/http_request_tests.rssrc/openhuman/tools/impl/network/web_fetch.rs
Address CodeRabbit review on tinyhumansai#2760: the constructors silently rewrote invalid 0 limits. Emit stable, grep-friendly warn logs ([tool.http_request] / [tool.web_fetch], field=…, from=0, to=<DEFAULT>) when coercion fires, per the repo's mandatory debug-logging rule. A bare `None` stays quiet — it is the intended "use default" path, not a misconfiguration. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/tools/impl/network/web_fetch.rs`:
- Around line 52-59: The Some(0) match arm for timeout_secs currently coerces
zeros to DEFAULT_TIMEOUT_SECS (20) causing drift from the schema/migration
default; change that coercion to use the schema default by replacing
DEFAULT_TIMEOUT_SECS with the timeout default from HttpRequestConfig::default()
(e.g., HttpRequestConfig::default().timeout_secs) so stale-zero configs are
repaired to the same 30s value used by the migration; update the match arm
handling of timeout_secs (the Some(0) branch) and the related log message to
reflect that you are coercing to the schema default rather than
DEFAULT_TIMEOUT_SECS.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f96d430c-e137-4ce9-87f5-c4fe1be52525
📒 Files selected for processing (2)
src/openhuman/tools/impl/network/http_request.rssrc/openhuman/tools/impl/network/web_fetch.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/openhuman/tools/impl/network/http_request.rs
…ema defaults Address CodeRabbit on tinyhumansai#2760: the tool-local DEFAULT_* consts duplicated the schema defaults and had already drifted — web_fetch's timeout default was 20 while the canonical `[http_request]` schema default is 30. Drop the consts; both constructors now pull fallbacks from `HttpRequestConfig::default()`, so the schema, the 5→6 migration, and both tool guards share one source and can't drift. Tests assert against the same `HttpRequestConfig::default()`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
Code Review
Overview
Fixes two silent bugs where stale [http_request] config values of 0 caused all web tool calls to fail:
timeout_secs = 0→Duration::from_secs(0)→ instant timeout on everyweb_fetch/http_requestmax_response_size = 0→ every response body truncated to zero bytes
Two-layer fix: tool constructors coerce 0 to schema defaults (defense in depth), and a v5→v6 migration repairs the persisted config.toml on next launch.
Strengths
- Defense in depth: both
HttpRequestToolandWebFetchToolconstructors now guard at point-of-use, so even if the migration never runs, tools work correctly. - Single source of truth: both tools and the migration derive fallback values from
HttpRequestConfig::default()— no cross-layer drift. - Good logging: grep-friendly
[tool.http_request]/[tool.web_fetch]prefixes, no payload, distinguishSome(0)(logged) fromNone(silent). - Idempotent migration: safe to retry on next launch.
- Pattern consistency: save/rollback,
== 5guard, log stats on success — matches prior migrations exactly. - Tests: zero, partial-zero, and nonzero cases covered for both tools; migration has three clean cases.
Issues
1. Critical: version conflict with PR #2761 (reconcile_orphaned_providers)
Both PRs bump CURRENT_SCHEMA_VERSION 5→6 and use an if config.schema_version == 5 gate. Merged separately, whichever lands second will skip users already at v6 from the first. The PR body acknowledges this but defers resolution ("should be re-sequenced to 6→7"). This needs to be resolved before merge — either:
- Land both in the same release with a single
== 5branch running both modules before the bump, or - Explicitly re-sequence this one to
6 → 7(change the guard, bump target, and update comments).
Leaving it ambiguous risks silently skipping the http_request repair for any user who gets #2761 first.
2. Behavior change: WebFetchTool default timeout shifts from 20s → 30s
The removed constants were:
-const DEFAULT_MAX_BYTES: usize = 1_000_000; // 1 MB
-const DEFAULT_TIMEOUT_SECS: u64 = 20; // ← 20 secondsThe new code falls back to HttpRequestConfig::default() for None — which from the PR description is 30s. So callers that pass timeout_secs: None previously got 20s; they now get 30s. This is a silent behavior change for normal (non-zero) configs and isn't called out in the PR. Confirm this is intentional and note it in the description.
3. Missing integration test for v5→v6 path in mod_tests.rs
The four schema_version = 5 → 6 string changes in mod_tests.rs are mechanical. There's no run_pending_v5_to_v6_repairs_http_request_limits test that actually exercises the wiring — i.e., starts at schema_version = 5, sets both fields to 0, calls run_pending, and asserts they're repaired and the version is 6. The same gap exists in #2761; both PRs should add it.
4. anyhow::Result return is vestigial
run() has no ? operators and never fails. The Err arm in mod.rs is dead code. Add a comment that the Result is forward-compatible for future I/O paths, or drop it to -> MigrationStats.
Test Coverage
| Scenario | Covered |
|---|---|
| Both zeros repaired | ✅ |
| Only timeout-zero repaired | ✅ |
| Nonzero values preserved | ✅ |
HttpRequestTool: zero → default |
✅ |
HttpRequestTool: nonzero preserved |
✅ |
WebFetchTool: None + Some(0) → default |
✅ |
WebFetchTool: nonzero preserved |
✅ |
run_pending wiring at schema_version == 5 |
❌ Missing |
Verdict
Request changes on two points before merge:
- Resolve the version conflict with #2761 — pick a sequencing strategy and implement it. Don't leave it as a documented known risk.
- Confirm or revert the 20s → 30s default timeout shift for
WebFetchTool— if intentional, document it in the PR body.
Add the integration test for good measure. Everything else is well-structured and follows project conventions.
…esult signature Address M3gA-Mind review on tinyhumansai#2760: - add `run_pending_v5_to_v6_repairs_http_request_limits`: exercises the full run_pending wiring (start at v5 with zero limits → repaired to schema defaults, bumped to v6, persisted to disk), not just the migration's own unit tests - document that run()'s `anyhow::Result` matches the shared migration-step signature dispatched by run_pending (forward-compat for future I/O-backed repairs), so the Err arm is intentional, not dead code Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@M3gA-Mind thanks for the thorough review — all four addressed in 1. Version conflict with #2761 (Critical). Keeping this at 2. 3. Missing v5→v6 integration test. Added 4. Vestigial |
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
All four issues from my initial review are addressed — LGTM.
- Version conflict with #2761 ✅ — Same-release merge plan documented; tool-layer guard makes a skipped migration non-fatal.
- 20s → 30s timeout shift ✅ — Confirmed intentional, documented under Behavior Changes.
- Integration test ✅ —
run_pending_v5_to_v6_repairs_http_request_limitscovers the full wiring path. anyhow::Result✅ — Doc comment added, consistent with #2761.
One reminder for merge: whoever lands second of these two PRs should verify the two == 5 branches are combined into a single block before the shared version bump.
M3gA-Mind
left a comment
M3gA-Mind
left a comment
There was a problem hiding this comment.
LGTM — all review items addressed. Ready to merge.
…tion (tinyhumansai#2759) A per-workload provider string like `chat_provider = "openai:gpt-4o"` points at a cloud_providers slug. When a user removes/disables that provider, the entry leaves cloud_providers but the workload string is left dangling: the AI panel shows the gone model and factory::make_cloud_provider_by_slug hard-errors at inference time instead of falling back to managed. Add `reconcile_orphaned_providers`: resets any *_provider whose slug is absent from cloud_providers (plus the always-managed openhuman:<model> form and bare non-sentinel strings) to None (= managed), and clears a dangling primary_cloud. Mirrors the factory's exact, case-sensitive grammar (verbatim stored slugs/ids; no fail-soft, since main deliberately fails closed via BYOK_INCOMPLETE_SENTINEL). Rebased onto tinyhumansai#2760: rather than a separate 6->7 bump, both 5->6 migrations (repair_http_request_limits + reconcile_orphaned_providers) run inside the single `== 5` branch behind one shared bump+save, per the NOTE tinyhumansai#2760 left. Provider values are redacted in logs (slug:<redacted>). 13 unit tests + a run_pending v5->6 integration test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2 participants
Summary
[http_request].timeout_secs = 0madereqwestbuild a 0-second total timeout (Duration::from_secs(0)), so everyweb_fetch/http_requestfailed instantly — even with the allowlist set to "Allow all".max_response_size = 0separately truncated every body to nothing.#[serde(default = …)]only fills missing keys, so a present0is taken literally and survives an app update with nothing to repair it.HttpRequestTool/WebFetchToolconstructors now coerce0 → schema default, so0can never reach reqwest regardless of how it got into the config.repair_http_request_limits) rewrites persisted zeros toHttpRequestConfig::default()(30s / 1 MB) and tidies the on-diskconfig.toml.Problem
After updating from an older build, web research silently broke: the agent's
web_fetch/http_requestreturned"Request failed: … timed out"on every call. The cause was not the allowlist (which was["*"]) but[http_request].timeout_secs = 0applied as a literal 0-second timeout (http_request.rs/web_fetch.rsboth didreqwest…timeout(Duration::from_secs(self.timeout_secs))with no> 0guard).max_response_size = 0was a second, latent bug masked behind the timeout. Nothing coerced these stale zeros on load, and#[serde(default)]doesn't catch present-but-zero values.Solution
tools/impl/network/{http_request,web_fetch}.rs: constructors coerce0(and, forweb_fetch,Some(0)/None) to fallbacks pulled fromHttpRequestConfig::default()— one shared source with the schema + migration, no tool-local constants left to drift. A genuine0/Some(0)is logged ([tool.*] coercing invalid limit …, grep-friendly, no payload); a bareNonestays quiet.migrations/repair_http_request_limits.rs: new 5→6 migration sourcing replacements fromHttpRequestConfig::default()(can't drift from the schema). Registered inmigrations/mod.rs,CURRENT_SCHEMA_VERSION5→6, following the existing rollback-on-save-failure pattern.0is treated as "use default", not "unlimited" — an unbounded timeout can hang and an unbounded body is a memory risk; the schema defaults are finite.Submission Checklist
zero → default, nonzero preserved) for both tools; migration unit tests (both-zero, one-zero, no-op); existingrun_pending_*integration tests updated for the version bump.docs/RELEASE-MANUAL-SMOKE.md.Impact
timeout_secs = 0/max_response_size = 0(broken) is repaired to working defaults; web research starts succeeding again. No change for configs with sane non-zero values.5 → 6, idempotent, best-effort (failures logged, retried next launch). The tool guard means even a config that never runs the migration still gets working fetches.Related
reconcile_orphaned_providers) — both PRs add a5 → 6migration. Resolution: ship both in the same release, merging their== 5branches into one block that runs both modules before the single bump to 6 (the code is already structured for this — see the note inmigrations/mod.rs). This stays safe even if the two merge a few commits apart, because the tool-layer guard makes a skipped repair non-fatal: an un-migrated config keeps a cosmetic0on disk but the tools still coerce it at use. (If release plans ever split them across separate builds, re-sequence this one to6 → 7.)AI Authored PR Metadata
Linear Issue
Commit & Branch
Validation Run
cargo check --manifest-path Cargo.toml --lib --tests— exit 0 (no new warnings).cargo test --lib): tool-guard tests (zero→default, nonzero preserved), migration unit tests, and the newrun_pending_v5_to_v6_repairs_http_request_limitsintegration test all pass.cargo fmt— clean on changed files.format:check/typecheck— noapp/changes.app/src-taurichanges.Behavior Changes
[http_request]limits no longer disable web tools;0is coerced to the schema default at both the config layer (migration) and the tool layer (guard).web_fetch/http_requestwork after an update that previously left them timing out instantly.web_fetch's tool-localDEFAULT_TIMEOUT_SECS = 20means a caller passingtimeout_secs: Nonenow falls back to the schema default (30s, was 20s). Production wiresSome(<config>), so this only affects unconfigured/Nonecallers; the change alignsweb_fetchwith the singleHttpRequestConfig::default()source (removes the cross-layer drift raised in review).Parity Contract
timeout_secs/max_response_sizeare untouched; allowlist/SSRF behaviour unchanged.NoneandSome(0)resolve to the same finite default inWebFetchTool;HttpRequestToolmaps0 → defaultfor both fields.Duplicate / Superseded PR Handling
Summary by CodeRabbit
Bug Fixes
Tests