Add timeout and retry logic to web tools

[ssevera1]

What

Add retry logic with exponential backoff to web search and fetch requests, matching the pattern used for LLM requests.

Why

Improves reliability against transient network failures.

[claude]

Thanks for adding retries — the general approach (bounded attempts + linear backoff, mirroring the LLM client) is the right shape, but there are three blocking issues in agent/tools/web.py.

1. agent/tools/web.py:213 — broken re.sub call, web_fetch now raises on every HTML page

text = re.sub(r"<br\s*/?>\n", text)

The "" replacement argument was dropped, so this is re.sub(pattern, repl) with the required string argument missing → TypeError: sub() missing 1 required positional argument: 'string'. It also silently changed the pattern (\n got appended to the regex instead of being the replacement).

Every WebFetchTool.execute() call on a text/html response hits _html_to_text, raises, and gets swallowed by the bare except Exception at line 192, returning ToolResult(False, "", "Failed to fetch: sub() missing 1 required positional argument: 'string'"). HTML fetching is completely broken. Should be:

text = re.sub(r"<br\s*/?>", "\n", text)

Separately, this line is unrelated to the stated scope ("add timeout and retry logic to web tools") — it looks like an accidental edit rather than an intended change.

2. agent/tools/web.py:173-174 — every fetch now issues two HTTP requests

body = self._fetch_with_retry(req)
resp = urllib.request.urlopen(req, timeout=self.timeout)
content_type = resp.headers.get("Content-Type", "")

The old urlopen call was kept to read Content-Type, so each web_fetch hits the remote server twice (and up to 6 times if the first call retries). The second call is unretried, so a transient failure there still fails the whole operation — partially defeating the point of the change. It also doubles latency and rate-limit consumption, and the two requests can disagree (body from request 1, content type from request 2). Suggest having the helper return both, e.g.:

def _fetch_with_retry(self, req) -> tuple[str, str]:
    for attempt in range(self.max_retries + 1):
        try:
            resp = urllib.request.urlopen(req, timeout=self.timeout)
            return resp.read().decode("utf-8", errors="replace"), resp.headers.get("Content-Type", "")
        ...

3. agent/tools/web.py:82-92 and 195-205 — HTTPError is retried, and the helper has no terminal return

urllib.error.HTTPError subclasses URLError, so the except (socket.timeout, urllib.error.URLError) clause retries permanent 4xx responses. A 404 or 403 now costs 3 requests and 3s of sleeping before surfacing the same error. Consider re-raising non-transient HTTP status codes immediately, or at minimum only retrying 5xx/429.

Minor, same blocks: as e is bound but unused, and the for loop has no return/raise after it, so the function's control flow reads as "may return None" to type checkers even though it's unreachable in practice.

4. Duplication

_fetch_with_retry is byte-identical in WebSearchTool and WebFetchTool. Worth lifting to a module-level helper (or a shared mixin) so the two copies can't drift.

Testing

tests/ has no coverage for WebFetchTool or _html_to_text — issue #1 would have been caught by a single unit test asserting _html_to_text("a<br>b") == "a\nb". Please add tests for the HTML conversion and for the retry helper (a mocked urlopen that fails once then succeeds) alongside the fix.

[claude]

All four points checked out against the code and are fixed in 76f6923.

Fixed:

1. _html_to_text's re.sub(r"<br\s*/?>\n", text) was missing the replacement arg (sub() needs (pattern, repl, string)) — every HTML web_fetch was raising TypeError and getting swallowed by the bare except Exception. Restored to re.sub(r"<br\s*/?>", "\n", text).
2. WebFetchTool.execute called _fetch_with_retry and a raw unretried urlopen for the same URL — double request per fetch, and the second call could fail without benefiting from retry. Replaced both per-class _fetch_with_retry methods with one module-level _fetch_with_retry(req, timeout, max_retries) -> (body, content_type) that both tools call once.
3. urllib.error.HTTPError subclasses URLError, so 404/403 responses were being retried 3x with sleeps for a permanent failure. Confirmed this is also how agent/core/llm.py:103-106 already handles it (comment there: "Must be checked before URLError: HTTPError subclasses it"). The shared helper now catches HTTPError first and re-raises immediately, no retry/sleep.
4. Duplication — the retry logic is now a single module-level function instead of byte-identical copies in WebSearchTool and WebFetchTool. Also dropped the unused as e binding the reviewer flagged.

Added tests/test_web.py: a regression test for <br> → \n conversion (fails on the old code with the TypeError), retry-then-succeed and give-up-after-max-retries cases for the shared helper, a test asserting HTTPError triggers zero sleeps/retries, and a test asserting WebFetchTool.execute issues exactly one urlopen call for a successful fetch (regression for the double-request bug).

Checks: pytest -q → 87 passed. mypy --ignore-missing-imports agent → no issues.

Nothing to push back on — all four issues were real and independently verifiable in the code.

[claude]

Reviewed the full diff. _fetch_with_retry is correct: it catches HTTPError before URLError (HTTPError subclasses URLError), so 4xx/5xx responses propagate immediately instead of burning retries, and only socket.timeout/URLError get the 1s/2s backoff across 3 total attempts. Both call sites are idempotent GETs, so re-issuing the same Request object is safe, and the pre-existing except handlers in each execute() still produce the same user-facing error strings once the helper re-raises.

Scope is tight — only agent/tools/web.py plus a new tests/test_web.py — and it matches the stated intent. The new tests cover the success path, retry-then-succeed, give-up-after-max-retries, and the no-retry-on-HTTP-error case, and TestWebFetchTool pins urlopen.call_count == 1 to guard against the request being issued twice. I verified statically rather than by running pytest; CI (mypy + pytest) will gate the merge.

Non-blocking nits: max_retries here counts extra attempts (range(max_retries + 1)) while llm.py uses it as total attempts (range(self.max_retries)) — same effective 3 attempts, but the differing semantics for the same name could trip someone up later. Also, retrying every URLError means non-transient failures (DNS NXDOMAIN, TLS cert errors) now cost an extra ~3s before surfacing.