-
Notifications
You must be signed in to change notification settings - Fork 162
fix(api,web): repair gaia api chat completions, HTTPS SNI, and download overwrite - #3364
fix(api,web): repair gaia api chat completions, HTTPS SNI, and download overwrite #3364kovtcharov wants to merge 4 commits into
Conversation
download_file took its filename from the remote Content-Disposition header and opened the destination "wb" with no existence check, so a page serving filename=report.pdf replaced the user's own ~/Downloads/report.pdf. The sensitive-filename guardrail then ran after the write and reacted by deleting the file -- and only when a _path_validator was attached, so an agent composing BrowserToolsMixin without one kept the overwritten file. The destination is now checked, and handed to the caller's policy screen, before anything is created; the body streams to a sibling .part file that is renamed into place only on success, so an interrupted download never leaves a truncated file under the real name. A .part already in the way is reported rather than deleted, since deleting it would destroy a concurrent download's in-flight file. Landing this before the SNI fix on purpose: most HTTPS downloads currently fail the handshake and never reach the write path, so fixing SNI makes the overwrite reachable.
The IP-pinning adapter closed the DNS-rebind window by rewriting the request URL's host to the resolved IP and stashing the hostname in URL userinfo, which urllib3 never reads for SNI. Every HTTPS request went out with no SNI, so any host that selects its certificate by SNI returned the wrong certificate or refused the handshake. Measured live, 9 of 11 hosts failed through WebClient and succeeded through plain requests -- including example.com, github.com, pypi.org, huggingface.co, and GAIA's own amd-gaia.ai and lemonade-server.ai. urllib3 lets the connect address and the TLS name be set independently, so the pin and correct SNI are both achievable. build_connection_pool_key_ attributes -- the extension point requests documents for this -- now sets server_hostname to the real hostname while the pool host stays the validated IP. urllib3 uses server_hostname both as the ClientHello name and as the name OpenSSL verifies the certificate against, and it is a PoolKey field, so two hostnames resolving to one IP get separate pools instead of racing on a shared one -- which the previous userinfo scheme did not actually achieve, since requests drops userinfo when it builds the pool key. assert_hostname is deliberately left unset. It would move name checking out of the handshake into urllib3's post-hoc matcher by setting check_hostname = False on the SSL context, which requests shares process-wide on some versions. The rebinding defence is unchanged: validate_url still rejects a hostname if any answer is private, the adapter still re-validates the exact IP it dials, and each redirect hop is still re-validated. requests is pinned to >=2.32.3, the release that added the hook. 2.32.2 is specifically unusable: it routes through get_connection_with_tls_context but has no such hook, so SNI would silently revert to the pinned IP. The live test is wired into the unit-tests workflow. It needs a real TLS peer -- the adapter can pass every offline test and still fail every handshake -- and skips itself when the runner has no network.
Every POST /v1/chat/completions returned HTTP 500, streaming or not. The server passed workspace_root= to agent.process_query(); no agent accepts it, so it reached Agent.process_query(user_input, max_steps, trace, filename) and raised TypeError. The only consumer of that keyword was an agent that has since been deleted, and nothing else reads it -- no agent implements the set_workspace_root hook the spec described -- so the extraction helper goes with it. The suite stayed green because tests/test_api.py replaced the agent with a MagicMock, which accepts any keyword: the mock proved the call happened, never that it was valid. The new tests drive a real Agent subclass through the same MemoryMixin -> Agent chain the shipped agents use, so a bad call shape fails in CI. They fail against the previous code with the exact TypeError above. A Copilot <workspace_info> block is now ordinary message text, which is what the model already treats it as.
Verdict: Approve with suggestions
Three genuinely broken things get fixed here: the chat-completions endpoint that returned 500 on every request, HTTPS web fetch that failed on most of the web, and a download that could silently replace a file the user already had. All three fixes look right, and each ships with tests that fail against the old code.
One thing worth fixing before merge: the HTTPS fix quietly does nothing on an older requests. The new approach hangs off a hook that only exists in requests 2.32.3 and up. The dependency file now asks for that version, which covers fresh installs — but anyone who pulls this branch into an environment they already had gets no error, no warning, and the same broken handshakes the PR is fixing. This repo's own checkout is in exactly that state right now (it has 2.31.0), so it is not a hypothetical. A one-line version check at startup turns a silent revert into a message that tells the user to upgrade.
The rest are small: two docstrings came out of the edit with scrambled indentation, and two of the new live TLS tests will go red rather than skip if the third-party test site they depend on is unreachable.
Real-world evidence
No automated evidence bundle was produced for this run, so nothing was exercised on my side — the verdict rests on static review plus the evidence the author posted on the PR, which I am relaying rather than reproducing.
The PR description carries before/after curl output against a running gaia api (500 with the argument error → 200 with a correct answer, plus a working streaming run and a Copilot-style request), and a before/after probe of the web fetch across 11 real hosts showing 9 that failed only through the pinned client now succeeding. That matches the surfaces the change touches, and it supports the verdict.
Two gaps, neither blocking: the download-overwrite behaviour is covered by unit tests only, with no live run of the tool refusing to clobber a real file; and the agent eval that the tool-docstring change would normally require could not run because the judge account is out of credit. The author states this openly with the failure output rather than skipping it silently, which is the right handling — but the eval remains unrun.
🔍 Technical details
Issues
🟡 The SNI fix silently no-ops on requests < 2.32.3 (src/gaia/web/client.py:234)
build_connection_pool_key_attributes was only added in requests 2.32.3. On anything older the override is never called, server_hostname is never set, and SNI reverts to the pinned IP — the exact bug this PR fixes — with no error anywhere. Verified in this checkout:
$ python -c "import requests; from requests.adapters import HTTPAdapter; \
print(requests.__version__, hasattr(HTTPAdapter,'build_connection_pool_key_attributes'))"
2.31.0 False
The setup.py pin protects a fresh pip install, but not a git pull into an existing venv — which is the common dev flow, and the state this repo is in today. CLAUDE.md's "No Silent Fallbacks — Fail Loudly" wants this to raise. Anchored at __init__ (client.py:143-145):
def __init__(self, *args, **kwargs):
# requests <2.32.3 lacks build_connection_pool_key_attributes, so the
# SNI override below would never run and TLS would name the pinned IP.
if not hasattr(HTTPAdapter, "build_connection_pool_key_attributes"):
raise RuntimeError(
"PinnedIPAdapter requires requests>=2.32.3 to set the TLS SNI "
f"while pinning the IP; found {requests.__version__}. "
"Run: pip install --upgrade 'requests>=2.32.3'"
)
super().__init__(*args, **kwargs)
self._pinned_cache: Dict[Tuple[str, int], str] = {}
🟢 Two docstrings lost their indentation in the edit (client.py:101-141, client.py:700-722)
In the PinnedIPAdapter docstring the sentence introducing the pool arguments is split across a de-dented paragraph, so it reads as a fragment:
pool arguments:
``server_hostname``, which
urllib3 uses both as the ClientHello SNI ...
WebClient.download's docstring has the same problem — the summary line is flush while the whole body and Args: block sit at 16 spaces. Black does not reformat docstring bodies, so lint will not catch either. For the class docstring, joining the two halves fixes it (client.py:124-127):
pool arguments. ``server_hostname`` is what urllib3 puts in the
ClientHello SNI *and* the name OpenSSL verifies the certificate
against, so a certificate valid only for the bare IP is never
🟢 Two live TLS tests fail instead of skipping when badssl.com is unreachable (tests/integration/test_web_client_live_sni.py:100-109)
test_certificate_name_is_still_verified and test_expired_certificate_is_still_rejected expect SSLError. If badssl.com is down, blocked, or slow, requests raises ConnectionError/Timeout instead, which escapes pytest.raises and turns an unrelated CI run red. The sibling tests already guard this with pytest.skip — worth applying the same treatment, e.g.:
def test_certificate_name_is_still_verified(web_client):
"""Pinning the IP must not disable certificate-name verification."""
try:
with pytest.raises(requests.exceptions.SSLError):
web_client.get("https://wrong.host.badssl.com/", timeout=30)
except (requests.exceptions.ConnectionError, requests.exceptions.Timeout) as exc:
pytest.skip(f"badssl.com unreachable: {exc}")
🟢 A sentence in the download docstring reads as the opposite of the code (client.py:710-712)
The check is repeated immediately before the rename; a file created in that last instant by another local process is still replaced.
The code raises when save_path.exists() before os.replace, so a file that appears is not replaced — only one created inside the sub-microsecond window between that check and the rename is. Naming it as the residual TOCTOU window would read more clearly.
Verification notes
- Confirmed
workspace_root/set_workspace_roothave no remaining references outside the new test's explanatory docstring, and noprocess_queryinsrc/orhub/accepts the kwarg — so the removal is complete and the API had genuinely been 500-ing on every request. - Confirmed
server_hostnameis a urllib3PoolKeyfield on both 1.26 and 2.x and survives_new_poolfor the https scheme, so the same-IP-different-hosts pool separation the tests assert holds on both. - Confirmed
browser_tools.pyimportstoollazily insideregister_browser_tools, so the new tests'patch("gaia.agents.base.tools.tool", ...)genuinely intercepts registration. - Confirmed
pytest-timeoutis installed intest_unit.yml, so the new step's--timeout=300is valid. - Could not run the test suite — no
pytestin this reviewer environment.
Strengths
- The
.part-file rewrite is the right shape. Exclusiveopen(..., "xb")means a concurrent download fails loudly instead of two writers interleaving, and cleanup only ever unlinks the file this call created — the easy bug here would have been unlinking on any failure and destroying someone else's in-flight download. The tests cover exactly that case. - Moving the sensitive-filename screen from after the write to before it closes a real hole: the old code reacted by deleting an already-written file, and only when a
_path_validatorhappened to be attached. TestRealAgentCallShapetargets the actual failure mode. AMagicMockagent swallows any signature, which is why mock-based tests stayed green while every real request raisedTypeError; binding against the realMemoryMixin→Agentchain is what makes this a regression guard rather than another green mock.- Deliberately not setting
assert_hostname— and saying why in the docstring — is the correct call; it would have flippedcheck_hostname = Falseon a contextrequestsshares process-wide. - Three commits, one logical change each, conventional-commit titles, and the docs for both changed surfaces updated alongside the code.
The SNI fix hangs off build_connection_pool_key_attributes, which only exists in requests >= 2.32.3. On anything older the override is never called, server_hostname is never set, and TLS names the pinned IP again -- the exact bug this branch fixes, with no error anywhere. The setup.py pin covers a fresh install but not a pull into an existing environment, which is the common dev flow, so the pin was a wish rather than enforcement. PinnedIPAdapter now refuses to construct without the hook and names the installed version and the upgrade command. It probes for the attribute rather than parsing a version because 2.32.2 is the awkward case: it routes through get_connection_with_tls_context but ships no such hook, so a ">= 2.32" test would wave it through. Verified against real wheels -- 2.31.0 and 2.32.2 refuse, 2.32.3 and 2.34.2 construct. Also: the two live badssl.com checks now skip when the host is unreachable instead of going red. "The site is down" and "certificate verification regressed" must not look the same, or the lane teaches people to ignore it. Docstrings: the PinnedIPAdapter and download docstrings had picked up inconsistent indentation and a sentence split mid-clause; and the download one claimed a file appearing before the rename "is still replaced", which inverts what the code does -- it raises. Reworded to name the actual residual window.
kovtcharov
commented
Sep 4, 2026
|
All four addressed in The silent no-op on an older
On 2.31.0, the version you have: Two tests cover it: one deletes the attribute and asserts the error names the requirement, the installed version, and the fix; one asserts the adapter still constructs on the pinned floor. The badssl tests — unreachable is now a skip, a handshake failure is still a failure, and a successful fetch is an explicit failure rather than silently passing. Worth noting for anyone touching that file: Docstrings — both reindented, the split sentence rejoined. You also caught that the The download evidence gap — cheap, so I added it. Live run of the tool itself, not Before, the tool reports success while destroying the file: After: A free name still works ( The eval remains unrun for the reason in the description — the judge account is out of credit, not a backend problem. Lint: Black / isort / Flake8 / Bandit pass; Pylint's 9 errors are pre-existing Windows |
kovtcharov
commented
Sep 4, 2026
|
The eval now has results — closing the one gap you named. Full numbers and caveats are in the PR description; the short version:
No status or score regressions, and the baseline's one failing scenario ( Three things I'd rather state than let the numbers imply:
On my earlier claim that the eval was blocked by an out-of-credit account: that diagnosis was wrong, and worth recording because the real cause is a trap for anyone else running evals here. Two pre-existing harness defects fell out of that, both untouched by this PR and noted in the description for separate filing: the |
Uh oh!
There was an error while loading. Please reload this page.
Summary
Fixes three shipped features that are broken in ordinary use:
gaia apireturned HTTP 500 on every chat completion, web fetch failed on most HTTPS sites, and a download could silently replace a file the user already had.Why
POST /v1/chat/completionsfailed on every request, streaming or not — the server passed aworkspace_root=keyword that no agent accepts, so the call raised before reaching the model. The whole endpoint was dead for anyone running the flagship agent.Web fetch was failing on most of the web. The IP-pinning adapter closed a DNS-rebinding window by rewriting the request URL's host to the resolved IP, which meant every HTTPS request went out with no SNI — so any host that picks its certificate by SNI returned the wrong one or refused the handshake. Measured live, 9 of 11 hosts failed through
WebClientwhile succeeding through plainrequests, including GitHub, PyPI, Hugging Face and GAIA's ownamd-gaia.ai. "Summarise this GitHub URL" returned an SSL error.The third is the reason the first two ship together.
download_filetook its filename from the remoteContent-Dispositionheader and opened the destination with no existence check, so a page servingfilename=report.pdfreplaced the user's own~/Downloads/report.pdf. Today the handshake usually fails first, so the write path is rarely reached — fixing SNI un-gates it. It is fixed in the earlier commit on purpose.Linked issue
Closes #3360
Changes
workspace_rootkwarg and theextract_workspace_roothelper that fed it. Nothing consumed it: its only consumer was a deleted agent, and no agent implements theset_workspace_roothook the spec described. A Copilot<workspace_info>block is now ordinary message text.build_connection_pool_key_attributes, the extension pointrequestsdocuments for this, so the socket still dials the validated pinned IP while TLS names the real host.assert_hostnameis deliberately not set: it moves name checking out of the handshake and flipscheck_hostname = Falseon an SSL contextrequestsshares process-wide on some versions.requestsis pinned>=2.32.3— 2.32.2 is specifically unusable, it routes throughget_connection_with_tls_contextbut has no such hook, so SNI would silently revert to the IP..partfile renamed into place only on success. The sensitive-filename blocklist moved from after the write (where it reacted by deleting the file, and only when a_path_validatorwas attached) to before it.The rebinding defence is unchanged and re-verified:
validate_urlstill rejects a hostname if any answer is private, the adapter still re-validates the exact IP it dials, and each redirect hop is still re-validated.Test plan
python util/lint.py --all— Black / isort / Flake8 / Bandit pass. Pylint reports 9 pre-existing errors, allos.killpg/geteuidWindows false positives indaemon/sidecars/*andinstaller/lemonade_installer.py; none in a file this PR touches.pytest tests/unit/test_browser_tools.py— 73 passedpytest tests/unit/test_web_client_ip_pinning.py tests/integration/test_web_client_live_sni.py— 24 passedpytest tests/test_api.py— 57 passed, 22 skipped (skips aregaia_agent_email-gated)pytest tests/unit/has ~1000 failures/errors from the knownsocket.socketpair()/_block_networkguard, andtests/test_sdk.pyfails 17 tests. Both reproduce identically with this branch's source files reverted toupstream/main. None are in a file this PR touches. CI runs these on ubuntu, where the socket guard does not fire.TypeError: Agent.process_query() got an unexpected keyword argument 'workspace_root'; 5/7 live TLS tests fail withSSLError; 6/8 download tests fail.tests/unit/**→test_unit.yml(pytest tests/unit/, installs.[api]+pytest-mock).tests/test_api.py→test_api.yml(triggers onsrc/gaia/api/**andtests/test_api.py, installs.[dev,api], sofastapiis present and the new class runs rather than skipping).tests/integration/test_web_client_live_sni.pyhad no lane — added as a step totest_unit.yml, alongside the existing live-upstream Lemonade asset test. Verified with-vthat every new test reportsPASSED, noneSKIPPED.Agent eval —
tool_selection, no regressionsThis touches an LLM-affecting surface (two lines added to
download_file's@tooldocstring), so per CLAUDE.md an eval is required. It ran against this branch at87b9479c— the Agent UI backend on:4200was started from this worktree's own venv, so the numbers describe this PR's code, not an installed release. Lemonade servedGemma-4-E4B-it-GGUFon GPU atctx_size=32768.tool_selectionis the category the changed tool docstring feeds.🔍 `gaia eval agent --compare` — the tool's own output
Per scenario:
known_path_readmulti_step_planno_tools_neededsmart_discoverydata_vs_recall_disambiguationNo status or score regressions. The baseline's one failing scenario now passes.
Three caveats, so the numbers are not read for more than they are worth:
claude-sonnet-4-6, this run byclaude-opus-5; the compare tool warns about exactly this. Treat "no regression" as the signal, not the+2.5onmulti_step_plan.data_vs_recall_disambiguationhas no baseline entry because it did not exist yet: it was added 2026年06月25日 in test(tool-loader): pin #800 doc-profile data-vs-recall disambiguation #1844 , two months after the baseline was captured (2026年04月24日). The extra scenario is not a discrepancy. On the 4 overlapping scenarios the result is 4/4 pass vs the baseline's 3/4.eval-20260904-210157), so the clean run is itself the confirmation that the diagnosis was right.smart_discoverywent 179s → 417s, but the baseline machine sustained 109–115 tok/s where this one runs at 38–45 tok/s — ×ばつ slower, which accounts for the gap on the token-heaviest scenario (8335→2083 tokens). The other three scenarios are faster in wall-clock despite that. The docstring addition is ~25 tokens on one tool description, 0.3% of that prompt.🔍 Two pre-existing eval-harness defects found while getting this to run
Both are untouched by this PR and are filed separately as #3367 and #3368:
runner.py:949adds--barewheneverANTHROPIC_API_KEYis non-empty, and--barerestricts auth to that key — OAuth and keychain are never consulted. Becauseload_dotenv()walks up parent directories, a.envanywhere above the checkout injects a key into the process even when the shell has none, silently disabling working subscription auth. A stale key then surfaces as HTTP 400"Credit balance is too low", which points at billing rather than at the override. This is the "no silent fallbacks" rule in CLAUDE.md: it should fail loudly on an auth error, or prefer OAuth when the key is rejected."error": "". The real cause above was invisible until the call was instrumented by hand.Evidence
HTTP API / REST — real requests against
gaia api start --port 8199with the flagship agent installed and Lemonade servingGemma-4-E4B-it-GGUF.Before (this commit's parent):
After — same request, same server:
Streaming path — 7 chunks, terminates with
data: [DONE], no"error"in the body. A Copilot<workspace_info>payload also returns 200 (5 plus 6→11).🔍 Web fetch — before/after probe across 11 hosts
Same script, same machine, only
client.pydiffering.www.amd.comtimes out identically both ways and through plainrequests, so it is not an SNI failure.WebClientbeforeWebClientafterrequests*.python.org)s.sni-810-default.ssl.fastly.net)9 of 11 failed → 1 of 11 fails, and the one remaining failure is identical under plain
requests.Security posture re-verified after the change:
🔍 Download — live run of the download_file tool
Live run of the tool (not
WebClientdirectly), into a directory that already holds aREADME.md:🔍 Runtime guard against an older requests
build_connection_pool_key_attributesonly exists in requests >= 2.32.3, so on anything older the SNI override silently never runs.PinnedIPAdapternow refuses to construct. Verified against real wheels:get_connection_with_tls_contextbut ships no hook)🔍 Download — WebClient-level before/after
A directory containing the user's own
README.md, then downloading a URL whose name resolves toREADME.md:gaia api startused to produce the evidence above.Checklist
Closes #3360).python util/lint.py --all,pytest tests/unit/).docs/spec/api-server.mdx(removed the workspace-root section and renumbered the flow) anddocs/spec/browser-tools.mdx+ the@tooldocstring (the new overwrite refusal, so the model learns the rule instead of discovering it from an error).