feat: enhance HTTP tool parameter parsing

[BenLocal]

Summary

• Improve the built-in http tool to tolerate common LLM-generated parameter shape issues instead of
  failing on valid intent with invalid typing.
• Add support for stringified headers JSON values such as "[]" or stringified header arrays/objects.
• Accept timeout_secs as either an integer or a numeric string, and apply the parsed timeout to the
  outgoing request.
• Treat empty-string save_to and empty-string body as unset values to avoid unnecessary validation
  failures on GET requests.
• Add unit tests covering the new parsing behavior and the reported malformed tool-call payload shape.

Change Type

• Bug fix
• New feature
• Refactor
• Documentation
• CI/Infrastructure
• Security
• Dependencies

Linked Issue

None

Validation

• cargo fmt
• cargo clippy --all --benches --tests --examples --all-features
• Relevant tests pass:
  • env -u http_proxy -u https_proxy -u HTTP_PROXY -u HTTPS_PROXY -u ALL_PROXY -u all_proxy cargo test
    tools::builtin::http::tests --lib
• Manual testing: not performed

Security Impact

Low. This change only relaxes input normalization for the existing built-in http tool. It does not broaden
URL allowlists, approval policy, secret injection behavior, or file-write scope. save_to still requires a
validated path under /tmp/.

Database Impact

None

Blast Radius

Touches only the built-in HTTP tool parameter parsing and request construction path. Potential regressions
are limited to:

• normalization of malformed tool-call params
• request timeout handling
• save_to detection for file-download responses

Core HTTP safety checks, approval behavior, and path validation remain unchanged.

Rollback Plan

Revert the changes in src/tools/builtin/http.rs to restore strict parameter handling. No migrations,
config changes, or data rollback are required.

———

Review track: B

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[zmanian]

Review: feat: enhance HTTP tool parameter parsing

The overall intent is good -- tolerating LLM-generated stringified parameters is a practical improvement. However, there are several issues that should be addressed before merging.

Issues

1. Security: Recursive parse_headers_param with unbounded depth (Medium)

The new String arm in parse_headers_param deserializes the string into a serde_json::Value and then recursively calls parse_headers_param(Some(&parsed)). If the parsed result is also a String (e.g., "\"[]\"" -- a JSON string containing another JSON string), this recurses again. While serde won't produce infinite depth from a finite input, the recursion depth is data-dependent and not explicitly bounded. Consider either:

• Matching only on Object / Array after parsing (not calling the full function recursively), or
• Adding a comment documenting that recursion terminates because serde_json::from_str on a JSON string produces a Value::String only if the inner content is a plain string (not parseable further), so one level of recursion is the max in practice.

Actually, re-reading more carefully: if input is "\"hello\"", serde_json::from_str produces Value::String("hello"), and the recursive call hits the String arm again, tries serde_json::from_str("hello") which fails, returning an error. So it terminates -- but with a confusing error message ("headers string must contain valid JSON object/array: ...") for what was originally a doubly-quoted string. A comment explaining the recursion bound would help maintainability.

2. Bug: Timeout error handler still hardcodes 30 seconds

The existing error handler at the request.send().await call reports ToolError::Timeout(Duration::from_secs(30)) regardless of the actual timeout_secs value. Now that timeout is configurable, this should use the parsed value:

ToolError::Timeout(Duration::from_secs(timeout_secs.unwrap_or(30)))

3. Missing upper bound on timeout_secs (Security/DoS)

There is no upper bound on the timeout value. An LLM could pass timeout_secs: 999999999 which would create a request that blocks for ~31 years. Add a reasonable cap (e.g., 300 seconds) and reject or clamp values above it.

4. save_to result still references the original save_to variable after .clone() refactor

In the save_to response JSON:

"saved_to": save_to,

After the refactor, save_to is now a String (from parse_save_to_param), which is the trimmed version. This is actually fine behavior-wise (trimmed is better), but the .clone() on the next line (save_to.clone()) is unnecessary -- save_to_owned could just be save_to directly since it's already an owned String. Minor nit.

5. Test test_extract_host_from_params_error is misleading

The test name says "error" but it calls requires_approval and discards the result with let _ = .... It doesn't assert anything. This appears to be a smoke test for the specific malformed payload shape, but:

• It should have at least one assertion
• The name should describe what it's testing (e.g., test_requires_approval_with_stringified_params)
• Without assertions, this test provides no regression protection

6. Validation checklist in PR body shows clippy and tests NOT checked

The PR description shows clippy and test checkboxes are unchecked, with a note that cargo test couldn't run due to proxy issues. This is a concern -- the code should be verified to compile and pass tests before merge.

Minor / Style

• The method_upper extraction is a nice cleanup (avoids calling .to_uppercase() twice).
• The empty-body-string handling is correct and prevents unnecessary body attachment on GET requests.

Summary

The two blocking issues are:

1. Timeout error handler hardcodes 30s -- bug that reports wrong timeout to the user
2. No upper bound on timeout_secs -- potential DoS vector

The rest are improvements that would make the code more robust and the tests more useful.

[zmanian]

Solid PR -- the parameter normalization is well-scoped and correctly handles the common LLM-generated shape issues (stringified JSON headers, numeric strings for timeout, empty-string-as-unset). Code quality is good.

Checked:

• No .unwrap() in production code -- confirmed, only in tests.
• Edge cases -- empty strings, null, stringified objects/arrays, malformed JSON all handled with proper error messages.
• Security -- MAX_TIMEOUT_SECS cap prevents LLM-controlled DoS via long timeouts. Stringified headers still go through the same validation pipeline (object or array-of-{name,value}). save_to still goes through validate_save_to_path. No new injection vectors.
• Pattern consistency -- follows existing parse_*_param extraction style, returns proper ToolError::InvalidParameters.

Two minor observations (non-blocking):

1. timeout_secs: 0 is accepted -- Duration::from_secs(0) would make every request fail immediately with a timeout error. Consider a minimum like 1 or treating 0 as "use default". Not a security issue, just a usability footgun if the LLM sends it.

2. method_upper vs method.to_uppercase() -- method_upper is computed early but the match on line ~482 still calls method.to_uppercase() again. Could reuse method_upper there. Trivial.

LGTM.