Repository navigation
feat: add youcom (You.com) provider to WebSearchTool - #358
valueadd-lgtm wants to merge 11 commits into
Conversation
Add a fourth pluggable search backend, "youcom", which calls the You.com you-search MCP tool over stateless JSON-RPC (streamable HTTP): - keyless by default via the free-profile endpoint (https://api.you.com/mcp?profile=free); YDC_API_KEY (or api_key) switches to the authenticated endpoint - maps web + news hits to SearchHit (description first, query-relevant highlights as snippet fallback) - lang maps to the inline lang: filter; blocked_domains map to server-side exclude_domains, allowed_domains filter client-side - JSON-RPC / tool errors surface as structured summaries - youcom_extra_params passthrough (freshness, safesearch, ...) - tests: SSE parsing, keyless/authenticated URLs, Bearer header, domain filtering, extra params, error paths, dedup - docs updated (en + zh): backend list, parameter tables, usage example Default provider and all existing behavior unchanged.
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
AI Code Review审查结论通过 审查范围：commit ed846d4（base 02509b2）为 WebSearchTool 新增 youcom（You.com you-search MCP）provider，共 4 个变更文件（trpc_agent_sdk/tools/_websearch_tool.py、tests/tools/test_websearch_tool.py、docs/mkdocs 英文与中文文档），对全部非生成变更文件完成了逐行、跨文件、协议与语言语义层面的审查。 计划符合性：计划为“add youcom (You.com) provider to WebSearchTool”。实现完整覆盖计划：youcom_extra_params 构造参数、YDC_API_KEY 环境变量回退、free/认证端点自动切换、_run_async_impl 分派分支、ProviderType Literal 扩展、SSE 解析（_parse_sse_payloads/_post_sse）与 15 个新增单元测试，中英文文档同步更新，无缺项。 测试充分性：正常路径、lang 内联过滤、域名过滤、extra params、count 限制、URL 去重、JSON-RPC 错误、isError、无结果与畸形 text 等路径均有覆盖，且所有新测试均正确关闭 mock client（aclose 无遗漏），测试内嵌 mock 与实现假设一致。缺口主要在与上游真实行为相关的畸形响应形态（highlights 非数组、isError 空文本、SSE 多行帧、results 形状漂移）上，已随各条评论给出。 主要风险：已证实 2 个 MODERATE 问题——(1) highlights[0] 未做类型校验，字符串→单字符 snippet、dict→KeyError 逃逸到 _run_async_impl 宽泛 except 使整次搜索变 SEARCH_ERROR；(2) isError=true 且 text 为空时落入误导性的 unexpected payload 摘要、且从不读取 MCP structuredContent。另有 3 个机制真实但触发依赖上游行为的 PLAUSIBLE 问题（error/result 帧优先级与 id 匹配、lang: 操作符拼接、SSE 跨行 data 帧/BOM）。经核实的排除项：域名 allow/block 列表有第 868 行 _is_blocked 客户端二次过滤、无绕过；JSON 对象键恒为字符串，"result" in item 不可能抛 TypeError；text 经 str() 强转后 json.loads 的类型异常已被捕获；_post_sse 客户端生命周期与既有 _get_json/_post_json 模式一致（注入 client 不 aclose、内部 client 在 finally 关闭），无泄漏。未发现安全、数据完整性或向后兼容性破坏。 门禁结论：无 SEVERE 级缺陷，state 为 PASSED。建议后续针对上述已证实问题（highlights 类型防御、isError 空文本路径）及其余 PLAUSIBLE 项补充畸形 payload 测试并修复。 发现的问题中等
问题： 触发条件： You.com 返回的命中项中 实际影响： 一条畸形命中会把整次成功搜索变成 修正方向： 与其它字段一致做类型防御： 中等
问题： 工具错误分支 触发条件： 上游服务端标记 实际影响： LLM 收到 "unexpected payload" 而非真实错误信息，无法区分“上游失败”与“响应格式异常”，文档承诺的“错误写入 summary”契约在该形态下失效，排障与降级都受到误导，并可能因此误报为“无结果”。 修正方向： 将 中等
问题： 对 SSE 帧的扫描 触发条件： 流内任一帧（如进度通知）携带 实际影响： 合法搜索结果被误报为错误摘要，或真实限流/配额错误被吞掉并表现为空结果成功，两种情况都与文档“错误写入 summary”的契约相反，结果不稳定且难以排障。 修正方向： 按 JSON-RPC 语义处理：只接受与请求 较低
问题： 触发条件： 上游把长 JSON 结果拆分为两行 实际影响： 修正方向： 按 SSE 规范支持跨行 data 帧（累计同一事件的多行 data 并检查本行是否为续行 较低
问题： 触发条件： 调用方固化 实际影响： 调用时的 query / 语言过滤被静默替换或失效（ 修正方向： 仅允许 extra params 覆盖白名单之外的键（对 较低
问题： 触发条件： You.com 响应结构漂移（如 实际影响： LLM 收到“搜索成功但 0 结果”，会如实向用户报告“未找到结果”而不会重试，与其它错误路径“结构化错误写入 summary”的契约不一致，掩盖了真实的协议兼容问题。 修正方向： 对非 dict 且非空的 |
| contents = item.get("contents") | ||
| highlights = contents.get("highlights") if isinstance(contents, dict) else None | ||
| if highlights: | ||
| description = str(highlights[0]).strip() |
There was a problem hiding this comment.
问题： _search_youcom 中 highlights 的类型校验缺失：代码只检查了 contents 是否为 dict，随后 if highlights: description = str(highlights[0]).strip() 直接对 highlights[0] 按下标取值，与其它 provider 对每个字段先做类型判断的防御风格不一致。
触发条件： You.com 返回的命中项中 contents.highlights 不是数组。若为字符串（如 "some text"），highlights[0] 取到单字符 "s"；若为 dict 或其它类型，highlights[0] 抛 KeyError/TypeError。该异常在 _search_youcom 内部未捕获，逃逸到 _run_async_impl 第 566 行的宽泛 except Exception（SEARCH_ERROR）。已实测复现两种行为。
实际影响： 一条畸形命中会把整次成功搜索变成 {"error": "SEARCH_ERROR: ..."}，所有已解析的合法命中与 query 一并丢失，LLM 无法降级使用部分结果；highlights 为字符串时用户看到 1 个字符的残缺 snippet。
修正方向： 与其它字段一致做类型防御：if isinstance(highlights, list) and highlights:，并对首元素做元素级校验后取值，或至少用 try/except 包裹该段，避免异常逃逸出 _search_youcom。
| if result.get("isError") and text: | ||
| return WebSearchResult( | ||
| query=query, | ||
| provider="youcom", | ||
| results=[], | ||
| summary=_truncate(f"You.com search error: {text}", self._snippet_len), | ||
| ) | ||
|
|
||
| data: Any = None | ||
| try: | ||
| data = json.loads(text) | ||
| except (json.JSONDecodeError, TypeError): |
There was a problem hiding this comment.
问题： 工具错误分支 if result.get("isError") and text: 要求 text 非空才返回错误摘要；若 isError=true 且 content[].text 为空（或信息只存在于 MCP 规范的 structuredContent 字段），代码落入后续 json.loads("")，被捕获后返回误导性的 "You.com search returned an unexpected payload." 成功形态摘要，且代码从未读取 structuredContent。
触发条件： 上游服务端标记 isError: true 但未携带 text 内容（MCP 层常如此），或把结构化结果放在 structuredContent 而 text 仅为人读摘要。两种情况在单元测试中均未覆盖（测试只覆盖了 isError 且 text 非空、以及 text 携带 JSON 两种形态）。
实际影响： LLM 收到 "unexpected payload" 而非真实错误信息，无法区分“上游失败”与“响应格式异常”，文档承诺的“错误写入 summary”契约在该形态下失效，排障与降级都受到误导，并可能因此误报为“无结果”。
修正方向： 将 isError 判定提前到内容提取之前：先检查 result.get("isError")，无论 text 是否为空都返回 You.com search error: ... 摘要；可同时读取 structuredContent 作为 text 的补充来源。
| result: Optional[dict[str, Any]] = None | ||
| error: Optional[dict[str, Any]] = None | ||
| for item in payloads: | ||
| if "result" in item: | ||
| result = item["result"] | ||
| elif "error" in item: | ||
| error = item["error"] | ||
| if error is not None: |
There was a problem hiding this comment.
问题： 对 SSE 帧的扫描 if "result" in item ... elif "error" in item 使“先 error 后 result”或“同帧双键”的流中 error 信息被错误取舍：错误分支在循环外 if error is not None 无条件优先于 result，且末帧胜出语义未按 JSON-RPC 的 id 匹配（请求固定 id: 1，响应未校验 id）。
触发条件： 流内任一帧（如进度通知）携带 error 键且其后跟有效 result 帧（已用模拟流实测：error 残留导致有效结果被丢弃）；或单帧同时含 result: null 与 error（elif 使 error 被丢弃，最终报 "no result payload"）。
实际影响： 合法搜索结果被误报为错误摘要，或真实限流/配额错误被吞掉并表现为空结果成功，两种情况都与文档“错误写入 summary”的契约相反，结果不稳定且难以排障。
修正方向： 按 JSON-RPC 语义处理：只接受与请求 id 匹配的帧；对同帧 result+error 共存时以 error 为准（先判 error 再判 result 或独立记录）；采用“最后一帧有效结果、显式指定优先级”的明确策略并补充对应测试。
| def _parse_sse_payloads(text: str) -> List[dict[str, Any]]: | ||
| """Parse the ``data:`` lines of a streamable-HTTP SSE body. | ||
|
|
||
| A streamable-HTTP server answers a JSON-RPC request with one SSE body | ||
| whose ``data:`` lines carry one JSON object each (progress notifications | ||
| followed by the final result). Lines that fail to parse are skipped so a | ||
| stray keep-alive or partial frame cannot break the search. | ||
| """ | ||
| payloads: List[dict[str, Any]] = [] | ||
| for line in (text or "").splitlines(): | ||
| if not line.startswith("data:"): | ||
| continue | ||
| raw = line[len("data:"):].strip() | ||
| if not raw: | ||
| continue | ||
| try: | ||
| decoded = json.loads(raw) | ||
| except json.JSONDecodeError: | ||
| logger.warning("WebSearchTool: skipping unparseable SSE line: %.80s", raw) | ||
| continue | ||
| if isinstance(decoded, dict): | ||
| payloads.append(decoded) | ||
| return payloads |
There was a problem hiding this comment.
问题： _parse_sse_payloads 把每个物理 data: 行当作独立 JSON 文档解析，未实现 SSE 规范允许的“单条 data 帧跨多行（后续行以空格开头）拼接”语义；第 297 行 line[len("data:"):] 只按行切片，且未处理首个 data: 行前的 UTF-8 BOM（Python 默认 utf-8 解码不剥离 BOM，line.startswith("data:") 对该行直接失配）。
触发条件： 上游把长 JSON 结果拆分为两行 data: 帧（两段各自无法被 json.loads 解析而被静默跳过，实测确认），或响应体以 BOM 起始；二者都会让一次 200 成功响应被解析为空帧列表。
实际影响： _search_youcom 返回 "You.com search returned no result payload." 空结果——成功搜索被误报为无结果，LLM 无法区分“真无结果”与“解析失败”，影响召回准确性。
修正方向： 按 SSE 规范支持跨行 data 帧（累计同一事件的多行 data 并检查本行是否为续行 data:），并在首行前剥离 BOM；或直接使用仓库已声明的 httpx-sse 依赖（pyproject 已含 httpx-sse>=0.4.0）解析。
| arguments: dict[str, Any] = {"query": query, "count": n} | ||
| if lang: | ||
| # You.com maps an inline ``lang:`` operator to a language filter. | ||
| arguments["query"] = f"{query} lang:{lang}" | ||
| if blocked: | ||
| arguments["exclude_domains"] = blocked | ||
| arguments.update(self._youcom_extra_params) |
There was a problem hiding this comment.
问题： arguments.update(self._youcom_extra_params) 在 query/count/lang: 重写之后执行，youcom_extra_params 中若包含 query、count 或 exclude_domains 键会静默覆盖本工具构造的核心参数；且 lang 值未做任何清洗，schema 描述（第 456 行）明确告诉模型“You.com maps it to the inline lang: filter”，模型可能直接传 lang="lang:ja"，拼接出 query lang:lang:ja。
触发条件： 调用方固化 youcom_extra_params={"query": "...", "count": 3} 等键，或 LLM 按 schema 字面解释传入已带 lang: 前缀的 lang 值；两种都无任何校验或警告。
实际影响： 调用时的 query / 语言过滤被静默替换或失效（lang:lang: 双操作符无法被服务端正确匹配），行为与文档“每次工具调用自动带上固定高级参数、不覆盖调用参数”的表述不符，且无日志可追踪。
修正方向： 仅允许 extra params 覆盖白名单之外的键（对 query/count/exclude_domains 做保护性合并，异常键记录 warning），并给 lang 增加防重前缀清洗（如去除已存在的 lang: 前缀）。
| sections = data.get("results") | ||
| if not isinstance(sections, dict): | ||
| sections = {} | ||
| items: List[dict[str, Any]] = [] | ||
| for section in ("web", "news"): | ||
| for item in sections.get(section) or []: | ||
| if isinstance(item, dict): | ||
| items.append(item) |
There was a problem hiding this comment.
问题： sections = data.get("results"); if not isinstance(sections, dict): sections = {} 对 dict 但形状非预期的响应（如 results 为数组、或嵌套层级与实现假设不同）静默降级为 {}，返回 summary="" 的空成功结果，未写入任何错误信号；测试仅覆盖了 results 为 dict 的正常形态。
触发条件： You.com 响应结构漂移（如 results 变更为扁平数组，或 free profile 命中位于其它嵌套层级）时发生。
实际影响： LLM 收到“搜索成功但 0 结果”，会如实向用户报告“未找到结果”而不会重试，与其它错误路径“结构化错误写入 summary”的契约不一致，掩盖了真实的协议兼容问题。
修正方向： 对非 dict 且非空的 results 返回显式提示（如 _truncate("You.com search returned an unexpected results shape.", ...) 写入 summary），并在映射循环中对 sections.get(section) 的非 list 值同样降级提示。
Address the openreview findings on the youcom provider: - _parse_sse_payloads: join multi-line data: frames per the SSE spec, strip a leading UTF-8 BOM, and fall back to per-line parsing for servers that omit blank-line event separators. - Frame selection: match the JSON-RPC request id (frames for other requests are ignored), give a same-frame error precedence over result: null, and let a later valid result supersede a stray earlier error frame. - isError: report the error whatever the text content is, using structuredContent as the detail source when no text is present, instead of falling through to payload parsing. - Read structuredContent as the results payload when the text content is not JSON (human-readable text + structured results). - Surface a non-dict non-empty results field as an explicit unexpected-shape summary instead of silently reporting 0 hits; skip non-list web/news sections without crashing. - Validate the first highlight is a string before using it as the description fallback. - Strip a pre-existing lang: prefix from the lang argument and ignore youcom_extra_params keys that would override per-call query/count/exclude_domains. Adds 13 tests covering each path; full websearch suite (104 tests), yapf and flake8 clean.
|
I have read the CLA Document and I hereby sign the CLA |
|
Thanks for the thorough review. Both verified MODERATE issues and the other findings are fixed in 04c3ea9, plus malformed-payload tests as suggested:
Added 13 targeted tests (malformed highlights, isError without text, structuredContent, cross-request ids, multi-line data, BOM, missing separators, shape drift). Full websearch suite: 104 tests passing; yapf and flake8 clean. The CLA is signed via the bot's comment format as well. |
|
I have read the CLA Document and I hereby sign the CLA |
AI Code Review审查结论通过 审查范围：base 计划符合性：实现与计划"add youcom (You.com) provider to WebSearchTool"一致——JSON-RPC 主要风险：1) 命中字段映射与官方 you-search 契约存在偏差：官方规范字段为 测试充分性：104 个 websearch 测试（新增 26 个 youcom + 4 个 SSE 解析测试），覆盖 keyless/认证端点、Bearer、域过滤、lang 过滤、JSON-RPC 错误、isError、structuredContent、去重、形状漂移等路径，质量高；缺口是 fixture 未按官方 门禁结论：未发现阻止合入的 SEVERE 级缺陷；报告 2 条 MODERATE、3 条 LOW 可操作问题，建议修复后合入。 发现的问题中等
问题： 新增的 触发条件： 真实端点的 实际影响： 在缺省配置（免密钥 free profile）下，凡是没有 修正方向： 按官方契约补充 中等
问题： 触发条件： 文档（ 实际影响： 未预期的凭据会被随请求发送到第三方端点 修正方向： 在 较低
问题： 帧选择循环中，一旦出现 error 帧随后又出现 result 帧（本提交新增的 触发条件： 服务端在返回最终有效结果前发出携带 实际影响： 相对加固前“无条件报错”的行为，现在真实发生过的失败在结果中完全不可见（ 修正方向： 在“后到 result 覆盖前序 error”的分支处补一条 较低
问题： 触发条件： 服务端把请求 id 以不同 JSON 类型回显（例如把 实际影响： 搜索结果被整体丢弃，返回 修正方向： 仿照官方客户端改为宽松比较（ 较低
问题： 触发条件： 服务端在事件流中周期性发送空 实际影响： 每个 keep-alive 行产生一条无意义告警（空内容告警尤其无信息量），日志噪音增大，掩盖真正值得关注的解析失败；旧实现（ 修正方向： 在 |
| description = str(item.get("description") or "").strip() | ||
| if not description: | ||
| contents = item.get("contents") | ||
| highlights = contents.get("highlights") if isinstance(contents, dict) else None | ||
| # Only a list of strings is a usable fallback; anything else | ||
| # (a bare string, a dict, ...) must not crash the whole search. | ||
| if isinstance(highlights, list) and highlights and isinstance(highlights[0], str): | ||
| description = highlights[0].strip() |
There was a problem hiding this comment.
问题： 新增的 _search_youcom 命中解析只读取 description，回退使用 contents.highlights[0]，完全没有读取 You.com you-search 的规范摘要字段 snippets（字符串数组）。
触发条件： 真实端点的 results.web[].snippets 是默认返回的规范字段（官方 quickstart 与官方 @youdotcom-oss/dsh-plugin 客户端均以 snippets 为第一摘要来源），而 description 并非所有命中都带（news 命中及部分 web 命中缺失）；contents.highlights 仅在开启 extraction 时存在，而本实现默认的 free-profile 端点恰好不提供 extraction。
实际影响： 在缺省配置（免密钥 free profile）下，凡是没有 description 的命中（尤以 news 区为主）其 snippet 会退化为空字符串，LLM 拿到的摘要信息与官方返回不一致，工具核心产出（标题/URL/摘要）质量下降；新增的 26 个测试 fixture 全部按 description+contents.highlights 建模，测试通过但不能发现该契约偏差。
修正方向： 按官方契约补充 snippets 读取：优先取 item.get("description") 或 item.get("snippets") 列表中第一个非空字符串，其次才回退 contents.highlights；同时为测试 fixture 增加带 snippets 字段的用例，与官方响应样例对齐。
| request_headers = { | ||
| "User-Agent": self._user_agent, | ||
| # The streamable-HTTP server rejects JSON-only Accept headers | ||
| # with 406 Not Acceptable, so SSE must be listed here. | ||
| "Accept": "application/json, text/event-stream", | ||
| } | ||
| if headers: | ||
| request_headers.update(headers) |
There was a problem hiding this comment.
问题： _post_sse（youcom 免密钥路径）的请求头固定为 User-Agent + Accept，仅当 self._api_key 非空时才在调用处追加 Authorization；httpx 对 client.post(..., headers=...) 的语义是请求头与客户端级头合并、同名覆盖，客户端级头不会被清除。
触发条件： 文档（docs/mkdocs/en/tool.md:2552/2665）明确推荐把预建的 httpx.AsyncClient 通过 http_client 参数跨 Agent/调用共享连接池；当该共享客户端带有客户端级敏感头（如为代理/网关/其它服务配置的 Authorization、Cookie、X-API-Key）时，youcom 免密钥请求不会覆盖这些头。
实际影响： 未预期的凭据会被随请求发送到第三方端点 https://api.you.com/mcp?profile=free，构成凭据泄露；现有测试 test_keyless_search_maps_web_and_news_hits 仅断言无头客户端下 authorization is None，该路径无测试覆盖。（DDG 免密钥路径有同款模式，但 youcom 是本变更新增的暴露面。）
修正方向： 在 _post_sse 内对 youcom 请求执行白名单化：显式构造仅含 User-Agent/Accept/（有 key 时的）Authorization 的请求头，避免继承客户端级头；或至少在 _search_youcom 免密钥分支记录警告，并为共享带头客户端场景补充测试。
| if "error" in item: | ||
| # A JSON-RPC response carries either ``result`` or ``error``; | ||
| # a frame carrying both is treated as an error. | ||
| error = item["error"] | ||
| error_wins = True | ||
| if "result" in item and "error" not in item: | ||
| # A later valid result supersedes an earlier stray error. | ||
| result = item["result"] | ||
| error_wins = False |
There was a problem hiding this comment.
问题： 帧选择循环中，一旦出现 error 帧随后又出现 result 帧（本提交新增的 test_stray_error_superseded_by_later_result 已证明服务端确实可能先发 error 帧再发 result 帧），error_wins 被置回 False，最终在 error is not None and (error_wins or result is None) 处走成功分支，被覆盖的 error 对象被静默丢弃、无任何日志。
触发条件： 服务端在返回最终有效结果前发出携带 id: 1 的 error 帧（如瞬时限流、配额提示），随后正常返回结果帧。
实际影响： 相对加固前“无条件报错”的行为，现在真实发生过的失败在结果中完全不可见（summary=""），运维无法区分“恢复后成功”与“从未失败”，限流类问题的可观测性丢失。
修正方向： 在“后到 result 覆盖前序 error”的分支处补一条 logger.warning（记录被覆盖的 error code/message），使瞬时失败可观测，同时保留“结果优先”的既有语义。
| if frame_id is not None and frame_id != payload["id"]: | ||
| continue |
There was a problem hiding this comment.
问题： frame_id is not None and frame_id != payload["id"] 使用严格的 Python 类型比较（int vs int），而 payload["id"] 恒为整数 1；官方 @youdotcom-oss/dsh-plugin 客户端对同一过滤使用 String(candidate.id) === String(id) 的宽松比较。
触发条件： 服务端把请求 id 以不同 JSON 类型回显（例如把 1 回显为字符串 "1"）时，所有帧都会被当作“其它请求的帧”过滤掉。
实际影响： 搜索结果被整体丢弃，返回 "You.com search returned no result payload."，且无任何日志提示原因，排障困难。
修正方向： 仿照官方客户端改为宽松比较（str(frame_id) != str(payload["id"]) 或对二者做类型归一后相等判断），并增加 id 为字符串形式的测试用例。
| for piece in buffer: | ||
| decoded = _try_decode(piece) | ||
| if decoded is None: | ||
| logger.warning("WebSearchTool: skipping unparseable SSE line: %.80s", piece) | ||
| continue | ||
| payloads.append(decoded) |
There was a problem hiding this comment.
问题： _parse_sse_payloads 逐行回退路径中，空 data: 行（keep-alive 常用）或内容为合法 JSON 但非 dict 的 data: 行（如 data: 123、data: [1,2]）现在会触发 logger.warning("WebSearchTool: skipping unparseable SSE line: ...")；同时 _try_decode 对空串/非 dict 不再静默跳过。
触发条件： 服务端在事件流中周期性发送空 data: keep-alive 行，或发送非对象 JSON 的 data 行；这些行导致所在块的整块 join 解析失败后进入逐行回退，每行各打一条警告。
实际影响： 每个 keep-alive 行产生一条无意义告警（空内容告警尤其无信息量），日志噪音增大，掩盖真正值得关注的解析失败；旧实现（if not raw: continue）对这些行是静默处理的。
修正方向： 在 _try_decode/回退循环中将空串与非 dict 的 JSON（列表、数字、字符串、true/null）与真正的 JSON 解析失败区分开：空串直接跳过不告警，非 dict 可降级为 debug 级别或仅在计数告警；为空 data 行场景补充测试。
…, harden SSE parsing - Send the JSON-RPC request as an explicitly built httpx.Request via client.send() so client-level headers on a shared http_client (gateway bearer tokens, cookie jars) are never forwarded to the You.com endpoint; the per-call timeout is preserved through request.extensions. - Map You.com's canonical snippets list into hit descriptions when description is empty or missing (notably news hits), guarded the same way as contents.highlights. - Compare JSON-RPC frame ids by string form so servers that echo the id back as "1" still match the outgoing request. - Log a stray error frame superseded by a later result frame instead of dropping it silently. - Quiet down SSE parsing: empty keep-alive data lines and valid non-object JSON no longer warn; only truly unparseable lines do. Tests: new cases for each fix; the full file passes (110 tests) on both httpx 0.27.2 and 0.28.1.
|
Thanks for the thorough review — all five points are addressed in 5874601: Header isolation (the big one): confirmed the leak —
Frame id comparison: compared by string form now — a server that echoes the id back as "1" still matches the outgoing integer id, so reply frames are no longer dropped. Superseded error frames: a stray error frame overridden by a later result frame is logged at WARNING (with the error payload included) instead of disappearing silently. SSE noise: empty keep-alive The full |
AI Code Review审查结论不通过 审查范围：base 02509b2..head 5874601，共 4 个文件（ 发现的问题严重
问题： 触发条件： 调用方复用共享 实际影响： 网关凭据（Basic 用户名密码、Bearer token）被发送到第三方 You.com 端点，凭据泄露；You.com 设置的 cookie 随后可能被发送到 Tavily/Google 等端点，造成跨供应商凭据污染，属于安全边界破坏，且新增测试（ 修正方向： 在 中等
问题： 触发条件： 默认客户端 实际影响： 搜索静默返回空结果，下游 LLM 会得到误导性“无结果”结论，且无法从日志区分是真实无结果还是端点重定向。 修正方向： 在 较低
问题： 当 触发条件： 上游把 实际影响： 现实命中被静默丢弃，LLM 得到 0 结果，无法区分“真无结果”与“上游形状漂移”，可用性下降。 修正方向： 在该分支把可解析的 list 命中映射为 |
| request = httpx.Request("POST", url, json=payload, headers=request_headers) | ||
| # send() falls back to the client timeout when the extension is | ||
| # absent; keep the per-call timeout semantics of the other providers. | ||
| request.extensions["timeout"] = httpx.Timeout(self._timeout).as_dict() | ||
| try: | ||
| resp = await client.send(request) |
There was a problem hiding this comment.
问题： _post_sse() 通过 client.send(request) 发送裸 httpx.Request，仅隔离了客户端级 headers/cookies，但客户端级的 auth（如 httpx.AsyncClient(auth=(user, pass))、BearerAuth、自定义 Auth 流程）仍会被应用：httpx 0.27–0.28 的 BaseClient.send() 内部调用 _build_request_auth(request, USE_CLIENT_DEFAULT) 返回 self._auth，随后 _send_handling_auth 在裸请求上执行 auth 流程（BasicAuth/BearerAuth 直接设置 Authorization 头，DigestAuth 甚至会响应 You.com 服务端发来的 WWW-Authenticate 质询），这与代码注释和英文文档承诺的“凭据不会转发给 You.com”（docs/mkdocs/en/tool.md:2540、docs/mkdocs/zh/tool.md:2592）相矛盾。另外 client.send() 仍会把 You.com 响应中的 Set-Cookie 写入共享客户端 cookie jar（self.cookies.extract_cookies(response)），该 cookie 会被后续经同一 http_client 发给其他供应商（如 Tavily）的请求附带。
触发条件： 调用方复用共享 http_client 且该客户端配置了 auth（网关 Basic/Bearer/自定义鉴权），或 You.com 端点为某次响应设置了 cookie 后再次复用同一客户端。
实际影响： 网关凭据（Basic 用户名密码、Bearer token）被发送到第三方 You.com 端点，凭据泄露；You.com 设置的 cookie 随后可能被发送到 Tavily/Google 等端点，造成跨供应商凭据污染，属于安全边界破坏，且新增测试（test_shared_client_credentials_not_forwarded 等）只覆盖了 headers= 场景，未覆盖 auth=，测试结论会误导后续维护者。
修正方向： 在 await client.send(request) 处显式传入 auth=None 禁止应用客户端 auth（已在 httpx 0.27/0.28 验证可抑制），并考虑在发送后不依赖共享客户端 cookie jar 的隐式写入（或使用独立的请求级 client/transport 发送该请求）；同时补充以 auth=("u", "p") 配置的共享客户端用例来锁定该行为。
| if self._api_key: | ||
| headers = {"Authorization": f"Bearer {self._api_key}"} | ||
|
|
||
| payloads = await self._post_sse(self._base_url, payload, headers=headers) |
There was a problem hiding this comment.
问题： _post_sse 的请求未经 follow_redirects 控制，且 You.com 端点在重定向场景下可能把裸请求重新发送到第三方地址；更直接的问题是：httpx.Request 未启用 follow_redirects 时，3xx 响应不会跟随，但 resp.raise_for_status() 对 3xx 不抛错，落到 _parse_sse_payloads(resp.text) 解析空 body，静默返回 0 结果，掩盖重定向/端点变更。
触发条件： 默认客户端 follow_redirects 为 False（httpx 默认）时 You.com 端点返回 301/302（如 base_url 变更）。
实际影响： 搜索静默返回空结果，下游 LLM 会得到误导性“无结果”结论，且无法从日志区分是真实无结果还是端点重定向。
修正方向： 在 client.send(request) 处显式传 follow_redirects=True（与其他 provider 的 client.post 一致），并在 raise_for_status 前对 3xx 记录警告。
| sections = data.get("results") | ||
| if not isinstance(sections, dict): | ||
| if sections: | ||
| # A non-empty non-dict ``results`` is a shape change upstream, | ||
| # not an empty search; surface it instead of reporting 0 hits. | ||
| return WebSearchResult( | ||
| query=query, | ||
| provider="youcom", | ||
| results=[], | ||
| summary="You.com search returned an unexpected results shape.", | ||
| ) | ||
| sections = {} |
There was a problem hiding this comment.
问题： 当 data["results"] 为非空非 dict（如列表 [{"url":...}, ...]，You.com 可能的形状变体）时，代码返回 results=[] + summary="You.com search returned an unexpected results shape."，直接丢弃全部命中且摘要不包含任何命中数量或提示可恢复的信息，与 Tavily/Google 路径对形状漂移的处理（保留可解析命中）不一致。
触发条件： 上游把 results 从 dict 改为 list 形状（测试 test_non_dict_results_shape_surfaced_as_summary 也证实该路径存在）。
实际影响： 现实命中被静默丢弃，LLM 得到 0 结果，无法区分“真无结果”与“上游形状漂移”，可用性下降。
修正方向： 在该分支把可解析的 list 命中映射为 SearchHit（复用下方 hit 构建逻辑），或至少在 summary 中带上命中数与命中标题样例，便于诊断。
… call - Pass auth=None to client.send so client-level Basic/Bearer/custom auth flows on a shared http_client are not applied to the bare You.com request (httpx applies them even to requests sent as-is). - Drop any Set-Cookie the You.com response wrote into the shared client's cookie jar afterwards, so it cannot be replayed to other providers (e.g. Tavily) through the same client. - Pass follow_redirects=True so an endpoint 301/302 does not degrade to parsing the empty 3xx body as a silent 0-hit search; log a warning if a residual 3xx still surfaces. - Map a flat list of hits in results to SearchHit entries instead of dropping them, and name the observed type in the unexpected-shape summary. - Add tests: shared-client basic-auth suppression (with and without the tool key), Set-Cookie jar isolation, redirect following, flat-list results shape.
|
Thanks again for the careful review — you were right that
Full |
AI Code Review审查结论不通过 审查范围：base 02509b2..head b5515a2，共 4 个提交（feat: add youcom provider + 三次加固），涉及 3 个文件： 发现的问题严重
问题： 触发条件： 配置了 实际影响： YDC API Key 或共享客户端的 cookie 会随重定向按原样发给重定向目标主机，第三方可借此获取凭据或身份信息，属于本变更引入的凭据泄露风险。 修正方向： 在 严重
问题： 触发条件： 上游在 SSE 事件内用多行 data 传输（例如 You.com 将 实际影响： 整次搜索静默降级为空结果（ 修正方向： 多行 data 拼接时应按 SSE 规范将各 中等
问题： 触发条件： You.com 服务端按 MCP 流式规范持续推送事件或 keep-alive 不结束正文，或响应正文较大导致客户端读取时间超过超时阈值。 实际影响： 搜索请求挂起 15 秒后报 HTTP_ERROR 或异常，且共享连接池内连接状态损坏，同进程后续其它 provider 的搜索也会失败；用户侧表现为偶发的『搜索超时 + 后续搜索全部报错』。 修正方向： 使用 中等
问题： 触发条件： 用户设置了 实际影响： 修正方向： 将 较低
问题： 触发条件： 服务端返回的 SSE 正文以 BOM 开头且仅含 BOM（或 BOM 后无任何 实际影响： 空 payload 导致 修正方向： 对 |
…llback, highlights scan - _parse_sse_payloads now joins buffered data chunks with no separator first (the faithful reassembly of a frame split at any boundary, including inside a JSON string) and only then falls back to the newline join, instead of the reverse order where the newline join could be accepted first and corrupt string contents. - The _post_sse cookie rollback snapshots the jar with values and restores pre-call values for cookies the call overwrote (Set-Cookie on a name that already existed), not just deleting additions; and deletions are scoped to the endpoint host, so a cookie written by a concurrent call on the same shared client for an unrelated domain is not caught in the rollback. - The contents.highlights fallback scans for the first string element (mirroring the snippets traversal) instead of checking only highlights[0], which disabled the fallback behind a non-string first element. - Tests: string-boundary split without separators restored intact; mixed complete/multi-line frames without separators pin the per-line trade-off; overwritten cookie value restored; unrelated-domain cookie survives the rollback.
|
Thanks for another sharp round — all four findings are addressed in 62a652c, with regression tests for each:
136 websearch tests pass locally; flake8 (the repo's |
AI Code Review审查结论不通过 审查范围：02509b2..62a652c，共 4 个文件（trpc_agent_sdk/tools/_websearch_tool.py 新增 598 行、tests/tools/test_websearch_tool.py 新增 1448 行、docs 英文/中文两份），为 WebSearchTool 新增 youcom（You.com MCP streamable HTTP）provider。 计划符合性：基本符合。youcom 已接入 ProviderType 与构造函数（YDC_API_KEY 环境变量回退、youcom_extra_params、keyless 与认证双端点）、lang 内联过滤符、blocked_domains 服务端 exclude_domains、snippets/contents.highlights 摘要回退、SSE 解析、手动逐跳重定向 + 共享 client cookie 回滚等，并有约 60 个针对性测试用例。 主要风险（均经 httpx 0.27 源码逐行验证）：
安全评估：凭据隔离（显式 Request + auth=None、逐跳同源校验、请求级 headers）与 cookie 回滚意图正确、测试充分，未发现 YDC_API_KEY 泄漏到第三方 origin 或明文 http 的路径。 测试充分性：整体非常充分（SSE 解析各切分形态、BOM、重定向同源/降级、cookie 回滚三形态、类型防护、JSON-RPC 帧关联均有覆盖），但缺少：空 Location 循环测试、中间跳异常连接释放测试、并发同域 cookie 覆盖测试、result 先于 error 的帧序测试、query 内已含 lang: 的重复拼接测试。 结论：存在 6 个可操作问题（2 SEVERE / 3 MODERATE / 1 LOW），门禁 FAILED。 发现的问题严重
问题： 触发条件： 服务端（或中间 CDN/代理）对 POST 返回 302/301 且 实际影响： 每个此类响应会让工具连续发出最多 4 次完全相同的 POST（叠加评论#2 的 off-by-one 实际可发 5 个请求），放大带宽与上游负载；若可跟随的 3xx 走完预算后循环退出，随后的 修正方向： 在读取 严重
问题： cookie 回滚的"值恢复"路径会并发覆盖同一域名上其他调用写回的 cookie 值。快照在首个请求发出前对整本 jar 取 (name, domain, path)→value； 触发条件： 共享 实际影响： 并发调用写入的有效会话值被静默回滚为旧值，后续请求携带过期的会话 cookie，导致其他 provider 调用鉴权失败或状态丢失；同一根因也可能误删并发新增的同域 cookie（snapshot 中不存在的 key 且域名匹配 endpoint host 时）。 修正方向： 快照记录时同时记录原 cookie 的过期/版本等标识，恢复前校验其未被并发修改（如比较 中等
问题： 触发条件： 端点首先返回同源 3xx（如 302 到 实际影响： 半读的响应连接残留在共享 修正方向： 在循环内每次发送后记录当前 response 并保证其关闭：发送下一跳前先 中等
问题： 重定向预算 off-by-one。 触发条件： 端点连续返回 4 个可合法跟随、同源的 3xx（例如路径/负载均衡多次迁移）或叠加评论#1 的空 Location 重复 POST 场景。 实际影响： 每张凭证（ 修正方向： 将判断改为在发送前一跳时计数： 中等
问题： JSON-RPC 帧循环对"result 帧先到、error 帧后到"的顺序不对称。 触发条件： 服务端（或读取/缓冲顺序）在同一个 id 下先发送 result 帧、随后再发送一个 error 帧——JSON-RPC 2.0 规定同 id 只能有一个响应，但代码的注释和测试（ 实际影响： 完整且可解析的搜索结果被替换为 修正方向： 将最终判断改为"最后一个语义帧胜出"：例如只在 较低
问题： 内联 触发条件： LLM 在 query 文本中自行使用 实际影响： 发送给 You.com 的查询被改成含重复 修正方向： 拼接前检查 |
| location = resp.headers.get("location", "") | ||
| try: | ||
| next_obj = httpx.URL(current_url).join(location) |
There was a problem hiding this comment.
问题： _post_sse 的重定向循环在 Location 头存在但为空（''）或空白（' '）时会把同一请求重复发送到同一个 URL。resp.has_redirect_location 只要求 3xx 且 Location 头存在（httpx 源码 _models.py:704），空字符串会通过；随后 httpx.URL(current_url).join('') 按 urljoin 语义返回原 URL（含 query），_same_origin 检查必然通过，于是循环像正常重定向一样递增 redirects 并再次 POST 同一请求，直到预算耗尽。
触发条件： 服务端（或中间 CDN/代理）对 POST 返回 302/301 且 Location: 为空或仅为空白字符——这是生产中出现过的不规范响应形态，测试没有任何覆盖。
实际影响： 每个此类响应会让工具连续发出最多 4 次完全相同的 POST（叠加评论#2 的 off-by-one 实际可发 5 个请求），放大带宽与上游负载；若可跟随的 3xx 走完预算后循环退出，随后的 300 <= resp.status_code < 400 分支只打 warning，raise_for_status() 抛出 HTTPStatusError 被 _run_async_impl 包装为 HTTP_ERROR 返回给 LLM。
修正方向： 在读取 location 后先 strip()，为空时按"无法跟随的重定向"处理（打 warning 并 break，回到既有的 3xx 残余处理路径），不要进入 URL 拼接与再发送。
| for cookie in list(client.cookies.jar): | ||
| key = (cookie.name, cookie.domain, cookie.path) | ||
| if key in cookie_snapshot: | ||
| if cookie.value != cookie_snapshot[key]: | ||
| # The call overwrote a pre-existing cookie: restore | ||
| # the pre-call value in place so every other | ||
| # attribute (expiry, secure, ...) survives. | ||
| cookie.value = cookie_snapshot[key] | ||
| continue | ||
| domain = cookie.domain.lstrip(".") | ||
| if endpoint_host == domain or endpoint_host.endswith("." + domain): | ||
| client.cookies.delete(cookie.name, domain=cookie.domain, path=cookie.path) |
There was a problem hiding this comment.
问题： cookie 回滚的"值恢复"路径会并发覆盖同一域名上其他调用写回的 cookie 值。快照在首个请求发出前对整本 jar 取 (name, domain, path)→value；finally 中只要 key in cookie_snapshot 且值不同，就无条件 cookie.value = cookie_snapshot[key] 原地恢复。若同一个共享 http_client 上并发的另一 provider 调用（如 Tavily）在快照之后把同名同域 cookie 从旧值写成了新值，回滚会把被并发调用的新值覆盖回旧值。代码注释声称删除范围限定在 endpoint host 域、"并发调用写入的无关域名 cookie 不受影响"，但被覆盖的正是与 You.com 同域（该 host 上的每个 hop 都被验证保持同源）的 cookie——例如 gateway 域下多个云 API 共用的会话 cookie。
触发条件： 共享 http_client 的 jar 中预先存在 You.com endpoint host（或包含该 host 的域后缀、带初始点）上的 cookie；You.com 调用在快照之后、回滚之前，另一个 provider 调用或用户代码更新了该 cookie 的值；且 You.com 响应本身也 Set-Cookie 了这个名字（使其值偏离快照）。
实际影响： 并发调用写入的有效会话值被静默回滚为旧值，后续请求携带过期的会话 cookie，导致其他 provider 调用鉴权失败或状态丢失；同一根因也可能误删并发新增的同域 cookie（snapshot 中不存在的 key 且域名匹配 endpoint host 时）。
修正方向： 快照记录时同时记录原 cookie 的过期/版本等标识，恢复前校验其未被并发修改（如比较 client.cookies.jar 中该 cookie 的 expires/version 或使用局部锁）；或将"恢复值"改为"删除本次 Set-Cookie 带来的新值并重放并发修改"的合并策略，并明确并发场景的文档化保证。
| try: | ||
| current_url = url | ||
| redirects = 0 | ||
| while True: | ||
| request = httpx.Request("POST", current_url, json=payload, headers=request_headers) | ||
| # send() falls back to the client timeout when the extension is | ||
| # absent; keep the per-call timeout semantics of the other providers. | ||
| request.extensions["timeout"] = httpx.Timeout(self._timeout).as_dict() | ||
| resp = await client.send(request, auth=None, follow_redirects=False) | ||
| if not resp.has_redirect_location: | ||
| break | ||
| redirects += 1 | ||
| if redirects > _SSE_MAX_REDIRECTS: | ||
| logger.warning( | ||
| "WebSearchTool: You.com endpoint exceeded %d redirects; " | ||
| "search may return no results.", | ||
| _SSE_MAX_REDIRECTS, | ||
| ) | ||
| break | ||
| location = resp.headers.get("location", "") | ||
| try: | ||
| next_obj = httpx.URL(current_url).join(location) | ||
| # URL.join follows urljoin semantics, so a relative | ||
| # Location drops the original query string: a keyless | ||
| # "?profile=free" call would silently land on the | ||
| # authenticated endpoint after the redirect. Re-attach | ||
| # the profile param, keeping any the Location carries. | ||
| origin_profile = httpx.URL(url).params.get("profile") | ||
| if origin_profile is not None and next_obj.params.get("profile") is None: | ||
| next_obj = next_obj.copy_with(params=next_obj.params.merge({"profile": origin_profile})) | ||
| next_url = str(next_obj) | ||
| except Exception as ex: # pylint: disable=broad-except | ||
| logger.warning( | ||
| "WebSearchTool: You.com redirect target %r is not a valid URL (%s); " | ||
| "search may return no results.", | ||
| location, | ||
| ex, | ||
| ) | ||
| break | ||
| # Validate each hop on its own: the request (and the tool's | ||
| # Authorization header) travels to current_url's origin, so | ||
| # the hop must be same-origin with the *previous* URL — not | ||
| # merely with the first one — and it may not downgrade to | ||
| # plaintext http unless the endpoint was configured that way | ||
| # (same-origin with the original URL). Otherwise the bearer | ||
| # header (and any cookies httpx re-attaches to redirect | ||
| # requests) would travel to whatever host or plaintext hop | ||
| # the 3xx names. | ||
| if not (_same_origin(current_url, next_url) and | ||
| (_same_origin(url, next_url) or httpx.URL(next_url).scheme == "https")): | ||
| logger.warning( | ||
| "WebSearchTool: dropping redirect from the You.com endpoint to " | ||
| "URL %r (not same-origin with the previous hop or plaintext http); " | ||
| "search may return no results.", | ||
| next_url, | ||
| ) | ||
| return [] | ||
| current_url = next_url | ||
| if 300 <= resp.status_code < 400: | ||
| # Defensive: a 3xx that was not followed (missing/invalid | ||
| # location, or the redirect budget ran out) must not read | ||
| # downstream as a normal empty search. | ||
| logger.warning( | ||
| "WebSearchTool: You.com endpoint responded %d without " | ||
| "following the redirect; search may return no results.", | ||
| resp.status_code, | ||
| ) | ||
| resp.raise_for_status() | ||
| return _parse_sse_payloads(resp.text) |
There was a problem hiding this comment.
问题： _post_sse 中每次 hop 的 httpx.Response 都写入同一个 resp 变量，且 resp.aclose() 只在 finally 中对"最后一个"response 执行。当第 2 跳及以后的 client.send 抛出异常（读超时、连接重置、传输错误）时，之前跳的 response 流从未被关闭，连接不会被归还到共享连接池——这恰是该函数 docstring（第 757-760 行）声称要防止的"断连接残留"。
触发条件： 端点首先返回同源 3xx（如 302 到 /mcp-moved），随后对 follow-up POST 的响应体读取中途异常（keep-alive 密集体上的读超时、上游断连）。
实际影响： 半读的响应连接残留在共享 http_client 的连接池中，之后每个同类失败都泄漏一条连接，直至空闲超时/GC 回收；泄漏的连接可能被后续其他 provider（Tavily/Google）请求复用而失败，HTTP_ERROR 返回给 LLM。
修正方向： 在循环内每次发送后记录当前 response 并保证其关闭：发送下一跳前先 aclose() 上一跳的响应（3xx 响应体未读，直接关闭安全），并/或用列表收集所有 hop 的 response，finally 中逐个 aclose()。
| redirects += 1 | ||
| if redirects > _SSE_MAX_REDIRECTS: | ||
| logger.warning( | ||
| "WebSearchTool: You.com endpoint exceeded %d redirects; " | ||
| "search may return no results.", | ||
| _SSE_MAX_REDIRECTS, | ||
| ) | ||
| break |
There was a problem hiding this comment.
问题： 重定向预算 off-by-one。redirects 在每跳前先 += 1 再判断 redirects > _SSE_MAX_REDIRECTS，因此 _SSE_MAX_REDIRECTS = 3 时实际允许 4 次跳转、共 5 个请求，与常量注释"Maximum redirects the SSE POST follows"不一致。
触发条件： 端点连续返回 4 个可合法跟随、同源的 3xx（例如路径/负载均衡多次迁移）或叠加评论#1 的空 Location 重复 POST 场景。
实际影响： 每张凭证（YDC_API_KEY bearer）比承诺多发出一跳到目标外的路径，且放大评论#1 的重复请求次数（空 Location 时实际 5 个请求而不是注释承诺的 3 个），超出预算的循环以最后一次 3xx 的 warning 结束。
修正方向： 将判断改为在发送前一跳时计数：redirects >= _SSE_MAX_REDIRECTS 即拒绝跟随（在递增后判断 redirects > _SSE_MAX_REDIRECTS 之前先检查 redirects == _SSE_MAX_REDIRECTS），或把递增移到发送之后。
| result: Optional[dict[str, Any]] = None | ||
| error: Optional[dict[str, Any]] = None | ||
| error_wins = False | ||
| for item in payloads: | ||
| if not isinstance(item, dict): | ||
| continue | ||
| frame_id = item.get("id") | ||
| # Only frames replying to this request carry its id; frames | ||
| # without one are notifications and carry no result/error. | ||
| # Compare the string forms: some servers echo the request id | ||
| # back as a string ("1"), and a strict typed comparison would | ||
| # drop every reply frame. | ||
| if frame_id is not None and str(frame_id) != str(payload["id"]): | ||
| continue | ||
| if "error" in item: | ||
| # A JSON-RPC response carries either ``result`` or ``error``; | ||
| # a frame carrying both is treated as an error. | ||
| error = item["error"] | ||
| error_wins = True | ||
| if "result" in item and "error" not in item: | ||
| # A later valid result supersedes an earlier stray error. | ||
| if error is not None: | ||
| # Keep transient failures observable: a stray error frame | ||
| # (e.g. a rate-limit notice) was overridden by a later | ||
| # result frame; log it rather than dropping it silently. | ||
| logger.warning("WebSearchTool: You.com error frame superseded by a later result: %.200s", error) | ||
| result = item["result"] | ||
| error_wins = False | ||
| if error is not None and (error_wins or result is None): | ||
| message = error.get("message") if isinstance(error, dict) else str(error) | ||
| return WebSearchResult( | ||
| query=query, | ||
| provider="youcom", | ||
| results=[], | ||
| summary=_truncate(f"You.com search error: {message}", self._snippet_len), | ||
| ) |
There was a problem hiding this comment.
问题： JSON-RPC 帧循环对"result 帧先到、error 帧后到"的顺序不对称。error_wins 是一个锁存位：只有后续的 result 帧能把它复位，而后续的 error 帧会把 error_wins 置 True 并保留已先到的 result 值；最终 error is not None and (error_wins or result is None) 恒为 True，先到的有效 result 被丢弃并静默返回错误摘要（该路径没有任何 warning 日志，与"转瞬错误帧被后续 result 覆盖要打日志"的对称处理不一致）。
触发条件： 服务端（或读取/缓冲顺序）在同一个 id 下先发送 result 帧、随后再发送一个 error 帧——JSON-RPC 2.0 规定同 id 只能有一个响应，但代码的注释和测试（test_same_frame_result_and_error_treated_as_error、test_stray_error_superseded_by_later_result）明确把"服务器违规"当作要防御的输入类型，本分支却未防御。
实际影响： 完整且可解析的搜索结果被替换为 results=[] + "You.com search error: ..." 摘要，模型把一次正常搜索当成失败，且日志无提示，排障困难。
修正方向： 将最终判断改为"最后一个语义帧胜出"：例如只在 result is None 时才让 error 生效（error is not None and result is None），或在循环中记录"最后出现的帧类型"，用最后类型决定返回 result 还是 error，并对"error 覆盖 result"的方向同样打 warning。
| if lang: | ||
| # You.com maps an inline ``lang:`` operator to a language filter. | ||
| # Guard against the model passing the operator form verbatim | ||
| # (``lang:ja``, possibly with stray whitespace or casing) after | ||
| # reading the schema description. | ||
| lang_value = str(lang).strip() | ||
| while lang_value[:5].lower() == "lang:": | ||
| lang_value = lang_value[5:].strip() | ||
| if lang_value: | ||
| arguments["query"] = f"{query} lang:{lang_value}" |
There was a problem hiding this comment.
问题： 内联 lang: 过滤符拼接未检查 query 自身是否已包含 lang: 操作符。lang_value[:5].lower() == "lang:" 循环只剥离 lang 参数值的前缀，之后无条件执行 arguments["query"] = f"{query} lang:{lang_value}"；当模型按 schema 描述（"You.com maps it to the inline lang: filter"）在 query 里已写了 lang:ja、同时又传 lang="ja" 时，结果是 "python lang:ja lang:ja"。相应测试也只断言前缀剥离后的拼接，未覆盖 query 内已含 lang: 的重复场景。
触发条件： LLM 在 query 文本中自行使用 lang: 操作符（schema 描述鼓励该用法），同时调用参数带 lang，或工具级 lang 与 query 内操作符同值。
实际影响： 发送给 You.com 的查询被改成含重复 lang: 过滤符的复合串，可能导致该端拒绝或语义改变，搜索结果与预期不符；属于模型实际会走到的调用形态。
修正方向： 拼接前检查 query 中是否已存在 lang: 操作符（正则匹配词边界），若存在则不再追加；并把该场景补为测试用例。
…edirect budget, lang dedup Six findings from the latest review round (2 SEVERE / 3 MODERATE / 1 LOW): 1. SEVERE: Empty Location header caused repeated POST to the same URL. Strip the Location value and break if empty, instead of joining an empty string (which returns the same URL). 2. SEVERE: Cookie value restore could overwrite a concurrent call's update to the same cookie. Only delete cookies the call added (absent from the snapshot); leave pre-existing cookies whose values were overwritten as-is. 3. MODERATE: Intermediate 3xx hop responses were not closed before the next hop, leaking pooled connections. Close each intermediate response before issuing the next hop (aclose is idempotent, so the finally-block close on the final response is still safe). 4. MODERATE: Redirect budget was off-by-one — redirects > MAX allowed MAX+1 redirects. Check redirects >= MAX before incrementing to enforce exactly MAX redirect follows. 5. MODERATE: Frame order asymmetry — a result frame followed by an error frame silently discarded the valid result via the error_wins latch. Remove the latch: result always wins regardless of frame order; error only takes effect if no result arrived. 6. LOW: lang: operator duplication — if the query already contains 'lang:', don't append a second one from the lang parameter.
|
Thanks for another sharp round — all six findings are addressed in 7504580, with regression tests for each: 1. Empty Location (SEVERE): The redirect loop now strips the Location value and breaks immediately if it's empty or whitespace-only, instead of joining an empty string (which returns the same URL and causes repeated identical POSTs). 2. Cookie value restore overwriting concurrent calls (SEVERE): The rollback no longer restores pre-existing cookie values to their snapshot. Only cookies the call added (absent from the snapshot) are deleted; pre-existing cookies whose values were overwritten by a You.com Set-Cookie are left as-is. This avoids silently clobbering a concurrent call that updated the same cookie after our snapshot. 3. Intermediate hop responses not closed (MODERATE): Each 3xx response is now closed with 4. Redirect budget off-by-one (MODERATE): The check is now 5. Frame order asymmetry (MODERATE): Removed the 6. lang: operator duplication (LOW): The query is checked for an existing |
AI Code Review审查结论不通过 审查范围： 计划符合性：整体实现完整、测试覆盖充分（SSE 多行/BOM/分帧、重定向预算/跨源丢弃/明文降级、cookie 回滚、凭据隔离、lang 去重、JSON-RPC 帧序、shape drift 等 40+ 用例，且均围绕真实 httpx 行为编写）。经 httpx 0.27.2/0.28.1 源码核验与实测确认的关键机制均正确： 主要风险（3 项，均已建立证据链）：(1) SEVERE：非 SSE（application/json）错误响应被 测试影响：测试覆盖成功路径与大部分失败路径，但缺少「application/json 错误体」与「同 URL 重定向循环」两个用例（正是上述缺陷的触发条件）； 门禁结论：存在 1 条 SEVERE 缺陷（错误被静默吞掉的正确性问题），按标准判定为 FAILED。 发现的问题严重
问题： 触发条件： 服务端对 200 请求用 实际影响： 服务端带诊断信息的拒绝被静默降级为一次「成功但零结果」的搜索，LLM 会据此回答「未找到结果」而不是上报失败，属于数据完整性问题（错误被吞掉）。 修正方向： 在 中等
问题： 重定向循环对「Location 指向与当前 URL 完全相同的地址」的自我循环没有防护：预算检查 触发条件： 服务端（MCP 会话瞬态故障/负载均衡器）对 实际影响： 携带 修正方向： 在发送下一跳前比较解析后的 中等
问题： cookie 回滚只能识别「快照里不存在的同域名 cookie 就删除」，无法区分「本调用通过 Set-Cookie 新增的 cookie」与「并发调用在同域新增的 cookie」。注释（第 897 行前后）声称「unrelated domain 的并发 cookie 不会被误删」，但未覆盖同域并发写：快照在本次调用开始前拍摄，若共享同一 触发条件： 同域（ 实际影响： 并发请求的会话 cookie 被静默删除，导致共享 client 上同域请求的连接态丢失（如临时认证态失效、会话被取消），属于真实的并发数据竞争，尽管触发窗口较小。 修正方向： 将快照进一步细化为「域名 + cookie 名」两级集合，回滚时只删除 (1) 快照中不存在的、且 (2) 域名匹配的 cookie；或回滚前用带值快照做双向比对，仅当值确实等于本调用响应写入的值时才删除，避免误删并发写入的 cookie。 |
| resp.raise_for_status() | ||
| return _parse_sse_payloads(resp.text) |
There was a problem hiding this comment.
问题： _post_sse 的成功路径只有一条：无条件调用 _parse_sse_payloads(resp.text)，而该解析器只认 data: 前缀行。当 You.com 端点以 application/json 返回 JSON-RPC 错误帧（如参数校验失败 -32602 Invalid language filter、50x 限流等，HTTP 仍为 200）时，整个 ssse 体没有一行以 data: 开头，解析结果为 []，_search_youcom 里已有的 JSON-RPC error 分支（第 1084-1118 行）因此永远不可达，最终走到 "You.com search returned no result payload."（第 1119-1125 行）。已在受控环境实测：_parse_sse_payloads('{"jsonrpc":"2.0","id":1,"error":{...}}') 返回 []。
触发条件： 服务端对 200 请求用 application/json（而非 text/event-stream）返回错误帧——作者在 776-778 行的注释里已亲历过 406 非 SSE 响应，证明该端点确实存在非 SSE 响应；非法的 lang:/freshness 等参数值正是这类仍返回 200 的服务端校验错误。
实际影响： 服务端带诊断信息的拒绝被静默降级为一次「成功但零结果」的搜索，LLM 会据此回答「未找到结果」而不是上报失败，属于数据完整性问题（错误被吞掉）。
修正方向： 在 _post_sse 解析前检查 resp.headers.get("content-type", "")：若不含 text/event-stream，则直接 json.loads(resp.text)，把 JSON-RPC 错误帧放进返回列表（或直接走错误路径），让现有 error 分支可见。
| if not resp.has_redirect_location: | ||
| break | ||
| if redirects >= _SSE_MAX_REDIRECTS: | ||
| logger.warning( | ||
| "WebSearchTool: You.com endpoint exceeded %d redirects; " | ||
| "search may return no results.", | ||
| _SSE_MAX_REDIRECTS, | ||
| ) | ||
| break | ||
| redirects += 1 | ||
| location = resp.headers.get("location", "").strip() | ||
| if not location: | ||
| logger.warning( | ||
| "WebSearchTool: You.com endpoint returned a redirect " | ||
| "(%d) with an empty Location; search may return no " | ||
| "results.", | ||
| resp.status_code, | ||
| ) | ||
| break |
There was a problem hiding this comment.
问题： 重定向循环对「Location 指向与当前 URL 完全相同的地址」的自我循环没有防护：预算检查 if redirects >= _SSE_MAX_REDIRECTS 在首轮迭代时 redirects == 0 必然通过，且 redirects += 1 在读到 Location 之后才递增（第 809 行），所以同一 URL 的 3xx 会被连续 4 次重新 POST 才中断。空 Location 的分支（第 811-818 行）能立即中断，但服务端自我循环时发出的恰恰是「非空且指向自身」的 Location，恰好绕过同样的防护。306-655 行的本地测试 test_redirect_budget_limits_to_three 只断言 4 次请求，未覆盖同 URL 循环。
触发条件： 服务端（MCP 会话瞬态故障/负载均衡器）对 POST /mcp 持续返回 302 Location: https://api.you.com/mcp（与当前 URL 相同）。
实际影响： 携带 Authorization: Bearer <YDC_API_KEY> 与完整 JSON-RPC 体的同一个 POST 在 3 秒超时窗口内被重复发送 4 次（错误路径还有 raise_for_status，不会无限次），造成 4 倍凭据请求与无谓的上游负载；且 302/303 语义下 httpx 自身会自动转 GET（httpx _redirect_method 已实测确认），本实现却始终重发 POST，与 has_redirect_location 所建模的 307/308 之后行为的注释意图不符。
修正方向： 在发送下一跳前比较解析后的 httpx.URL(current_url) 与 next_url，若完全相同则按空 Location 同样处理（告警并 break）；同时按 RFC 9110 对 302/303 在下一跳改用 GET（或仅对 307/308 保持 POST）。
| for cookie in list(client.cookies.jar): | ||
| key = (cookie.name, cookie.domain, cookie.path) | ||
| if key in cookie_snapshot: | ||
| # A pre-existing cookie was in the jar before this call. | ||
| # Don't restore its snapshot value: a concurrent call on | ||
| # the same shared client may have updated it after our | ||
| # snapshot, and restoring the old value would silently | ||
| # overwrite that concurrent update. Only cookies this | ||
| # call added (absent from the snapshot) are cleaned up. | ||
| continue | ||
| domain = cookie.domain.lstrip(".") | ||
| if endpoint_host == domain or endpoint_host.endswith("." + domain): | ||
| client.cookies.delete(cookie.name, domain=cookie.domain, path=cookie.path) |
There was a problem hiding this comment.
问题： cookie 回滚只能识别「快照里不存在的同域名 cookie 就删除」，无法区分「本调用通过 Set-Cookie 新增的 cookie」与「并发调用在同域新增的 cookie」。注释（第 897 行前后）声称「unrelated domain 的并发 cookie 不会被误删」，但未覆盖同域并发写：快照在本次调用开始前拍摄，若共享同一 http_client 的另一协程（如另一次对 api.you.com 的请求，或 app 自身对 You.com 域名的调用）在本调用进行期间写入同域 cookie，本调用的 finally 回滚会把它一并删除。
触发条件： 同域（endpoint_host 或其父域）cookie 在本调用快照之后、finally 回滚之前，被并发协程写入共享 client 的 jar。
实际影响： 并发请求的会话 cookie 被静默删除，导致共享 client 上同域请求的连接态丢失（如临时认证态失效、会话被取消），属于真实的并发数据竞争，尽管触发窗口较小。
修正方向： 将快照进一步细化为「域名 + cookie 名」两级集合，回滚时只删除 (1) 快照中不存在的、且 (2) 域名匹配的 cookie；或回滚前用带值快照做双向比对，仅当值确实等于本调用响应写入的值时才删除，避免误删并发写入的 cookie。
Three findings from the latest review round: 1. SEVERE: _post_sse only parsed SSE (data: lines); a non-SSE response (application/json carrying a JSON-RPC error frame for a parameter-validation failure or rate-limit) produced [] from the parser, silently swallowing the error. Now checks content-type first and falls back to json.loads so the error branch in _search_youcom can surface the real failure reason. 2. MODERATE: A redirect whose Location resolved to the same URL as the current request looped 4 times (the redirect budget) repeating the credential-bearing POST each time. Now breaks immediately when next_url == current_url. 3. MODERATE: Cookie rollback could delete a same-domain cookie written by a concurrent call on the shared http_client. Now tracks cookies each response actually added (cookies_added_by_call) and only deletes those whose current value still matches — a concurrent call that updated the value is left alone.
|
All three findings from the latest round are addressed in e2478f5, with regression tests for each: 1. Non-SSE error frames silently swallowed (SEVERE): 2. Same-URL self-redirect loop (MODERATE): after resolving the Location (including the profile re-attach), the loop now compares On the 302/303 → GET point: I kept 302/303 following with POST rather than breaking, because the existing redirect-followed test path and real-world MCP endpoints issue 302 for base-URL changes where re-POSTing is the correct behavior. The same-URL break handles the practical concern (the repeated credential POST); switching 302/303 to GET would drop the JSON-RPC body and break the MCP protocol. Happy to revisit if you see it differently. 3. Cookie rollback deleting concurrent same-domain writes (MODERATE): the rollback now tracks which cookies this call's responses actually added (
|
AI Code Review审查结论通过 审查范围 发现的问题中等
问题： 触发条件： 服务器返回形如 实际影响： 命中标题/摘要内容被静默改写（如 修正方向： 与注释意图一致，应只剥离修饰性空白而不是数据：对 中等
问题： 触发条件： 调用方在共享 实际影响： 之后该共享客户端对 修正方向： 对快照中存在的键，在其值在本次调用期间被改变时也做恢复（可按调用粒度串行化的场景直接还原快照值；若必须兼顾并发，则至少记录被覆盖键并在调用结束时以最后一次写入为准评估），或在文档中如实声明该遗留值不会被清理。 较低
问题： 触发条件： 端点（或被劫持的中间层）返回同源但带 userinfo 的 实际影响： 后续请求发出的是 修正方向： 在构造跳转 URL 时剥离 userinfo（如 较低
问题： 触发条件： 任何将来对值比较分支的重构或删除。 实际影响： 测试套件全绿，但"并发调用更新 cookie 后其值不被误删"的保证悄然失效，回滚逻辑退化为无条件的按名删除，回到并发覆盖问题。 修正方向： 在响应处理之后、回滚之前插入并发写入（例如在 handler 内或 |
| if line.startswith("data:"): | ||
| buffer.append(line[len("data:"):].strip()) | ||
| elif not line.strip(): |
There was a problem hiding this comment.
问题： _parse_sse_payloads 在收集每一行 data: 片段时执行 buffer.append(line[len("data:"):].strip())，strip() 删除了片段首尾所有空白。当服务器按任意边界把 JSON 帧拆成多行 data:（本函数注释宣称要忠实重建的场景），这些空白可能恰是 JSON 字符串值的一部分，导致字符串内容被静默改写——这正是该注释声称要避免的另一种损坏形式。
触发条件： 服务器返回形如 data: {"snippet": "ab + data: cd"} 的拆分帧（字符串值在片段边界带空格），或字符串尾部空格紧贴 } 前的片段被切分。已按实现原样复现：data: {"snippet": "ab + data: cd"} 解析为 {"snippet": "abcd"}（原值 "ab cd"），单帧 data: {"title": "foo ", ...} 中字符串尾随空格也被剥离。
实际影响： 命中标题/摘要内容被静默改写（如 "ab cd" → "abcd"），模型可能据此引用错误原文；现有测试只覆盖无空格边界（test_parse_sse_payloads_string_split_without_separator_keeps_string_intact），无法捕捉该损坏。
修正方向： 与注释意图一致，应只剥离修饰性空白而不是数据：对 data: 前缀使用 line[5:].lstrip(" ")（SSE 规范中前缀后仅一个空格是分隔符），并仅在拼接前的整段 raw.strip() 处处理首尾空白；或按 SSE 规范改用 data: 行内容逐行拼接后整体 strip()，避免在片段级别修改内容。
| for cookie in list(client.cookies.jar): | ||
| key = (cookie.name, cookie.domain, cookie.path) | ||
| if key not in cookies_added_by_call: | ||
| # Not added by this call (either pre-existing or | ||
| # written by a concurrent call on the shared client). | ||
| continue | ||
| # Only delete if the value hasn't changed since this call | ||
| # set it — a concurrent call may have updated it since. | ||
| if (cookie.value or "") != cookies_added_by_call[key]: | ||
| continue | ||
| domain = cookie.domain.lstrip(".") |
There was a problem hiding this comment.
问题： _post_sse 的 cookie 回滚只删除本次调用新增的 cookie（cookies_added_by_call，行 927），而被 You.com Set-Cookie 覆盖的同名存量 cookie（键在 cookie_snapshot 中）被明确跳过（行 930-935 注释），新值永久留在共享 jar 中且没有任何恢复路径。函数文档（行 758-761）声称 You.com cookie "cannot be replayed to other providers through the same http_client"，但对该场景该保证并不成立。
触发条件： 调用方在共享 http_client 上预置 cookie（如 client.cookies.set("session", "app-value", domain="api.you.com")，为其自身访问 You.com 的会话），You.com 响应返回同名 Set-Cookie: session=you-value; Path=/（对共享客户端而言这是「写到共享 jar 里」的跨调用副作用，与注释的并发论点无关，因快照事先已存在）。
实际影响： 之后该共享客户端对 api.you.com 的任何请求（包括下一次 youcom 搜索）都会把 you-value 当作会话发送，调用方自己的会话值被静默顶替且无法复原（回滚只覆盖加了 value 记录的 cookie）。
修正方向： 对快照中存在的键，在其值在本次调用期间被改变时也做恢复（可按调用粒度串行化的场景直接还原快照值；若必须兼顾并发，则至少记录被覆盖键并在调用结束时以最后一次写入为准评估），或在文档中如实声明该遗留值不会被清理。
| if not (_same_origin(current_url, next_url) and | ||
| (_same_origin(url, next_url) or httpx.URL(next_url).scheme == "https")): | ||
| logger.warning( |
There was a problem hiding this comment.
问题： _same_origin（行 394-396）只比较 (host, port) 与 scheme，完全忽略 URL userinfo。而每跳请求直接以 current_url 构造（行 796），client.send(..., auth=None) 在无显式 auth 时，httpx 的 _build_request_auth（已核对 0.27/0.28 源码）会取 request.url.username/password 构造 BasicAuth，其 auth 流会覆盖请求头中的 Authorization（即工具配置的 Bearer YDC_API_KEY）。
触发条件： 端点（或被劫持的中间层）返回同源但带 userinfo 的 Location，如 https://tracker:tx@api.you.com/mcp（host/port/scheme 均通过 _same_origin 校验）；或调用方配置 base_url="https://user:pass@host/mcp"（代理/隧道常见写法）。
实际影响： 后续请求发出的是 Authorization: Basic dHJhY2tlcjp0eA== 而非 Bearer，带 key 的搜索在重定向后静默变成未认证调用（401/JSON-RPC 错误），且攻击者可通过控制 Location 诱导工具把 URL 内嵌凭据发给同源端点；_same_origin 的安全模型（"相同 host 即安全"）在此场景被绕过。
修正方向： 在构造跳转 URL 时剥离 userinfo（如 next_obj.copy_with(raw_userinfo=b"")），或对含 userinfo 的 Location 直接按非同源拒绝。
| async def test_concurrent_same_domain_cookie_value_change_not_deleted(self): | ||
| # A cookie this call added (recorded in cookies_added_by_call) | ||
| # whose value was subsequently changed by a concurrent call must | ||
| # not be deleted — the value-aware rollback skips it because the | ||
| # current value no longer matches what this call's response set. | ||
| def handler(request: httpx.Request) -> httpx.Response: | ||
| return httpx.Response( | ||
| 200, | ||
| text=_youcom_sse_body(_YOUCOM_TOOL_TEXT), | ||
| headers={ | ||
| "content-type": "text/event-stream", | ||
| "set-cookie": "you_session=ours; Path=/; Domain=api.you.com", | ||
| }, | ||
| ) | ||
|
|
||
| client = httpx.AsyncClient(transport=httpx.MockTransport(handler)) | ||
| t = WebSearchTool( | ||
| provider="youcom", | ||
| http_client=client, | ||
| base_url="https://api.you.com/mcp?profile=free", | ||
| ) | ||
|
|
||
| # Wrap _post_sse so that after it returns (cookies rolled back), | ||
| # we can verify the rollback worked. Then run a second call where | ||
| # a "concurrent" cookie overwrites the value mid-flight. | ||
| await t._run_async_impl(tool_context=_tool_ctx(), args={"query": "python"}) | ||
| # This call's cookie was rolled back. | ||
| assert client.cookies.get("you_session") is None |
There was a problem hiding this comment.
问题： test_concurrent_same_domain_cookie_value_change_not_deleted（tests/tools/test_websearch_tool.py 行 2914-2941）声称验证"值感知回滚不会删除并发调用更新的 cookie"，但第二次并发写入从未发生：测试只跑了一次 _run_async_impl，之后唯一的断言是 client.cookies.get("you_session") is None（行 2941）——即回滚发生。行 933 的 if (cookie.value or "") != cookies_added_by_call[key]: continue 分支（该测试名义上要钉住的行为）在此测试中不可能触发，测试注释（行 2936-2938）也承认"second call"并未执行。
触发条件： 任何将来对值比较分支的重构或删除。
实际影响： 测试套件全绿，但"并发调用更新 cookie 后其值不被误删"的保证悄然失效，回滚逻辑退化为无条件的按名删除，回到并发覆盖问题。
修正方向： 在响应处理之后、回滚之前插入并发写入（例如在 handler 内或 _post_sse 返回前改写 jar 中该 cookie 的值），使值比较分支真正被覆盖，并断言该 cookie 未被删除。
…erinfo redirect, concurrent test
Round 4 review follow-up (2 MODERATE, 2 LOW):
- SSE data: fragment strip corruption: _parse_sse_payloads now strips
only the single SSE separator space after "data:" (removeprefix(" "))
instead of .strip(), so a JSON frame split inside a string value at a
whitespace boundary is reassembled faithfully. The whole-buffer strip
in _try_decode is unchanged (JSON-safe).
- Cookie value restore: the finally-block rollback now restores the
snapshot value for pre-existing cookies overwritten by a You.com
Set-Cookie, not just deleting newly-added ones. The You.com response
value no longer leaks to subsequent calls on the shared http_client.
Host-scoped so unrelated-domain cookies are untouched.
- Userinfo redirect: a Location carrying userinfo (e.g.
https://tracker:tx@api.you.com/mcp) is now dropped as non-same-origin,
preventing httpx from constructing BasicAuth from the URL that would
override the tool's Bearer header on the next hop.
- Concurrent cookie test: test_concurrent_same_domain_cookie_value_change
_not_deleted now actually exercises the value-aware rollback branch by
injecting a concurrent cookie update (via an SSE-parser spy) between the
recording loop and the finally block, and asserts the concurrent value
survives. The previous test only ran one call and asserted the cookie
was deleted, never touching the value-mismatch branch.
145 tests pass; ruff/flake8 clean.
|
Thanks for the thorough round 4 review — all four findings are addressed in 52178f7: 1. SSE 2. Cookie rollback leaves overwritten pre-existing values in the jar (MODERATE): the finally-block rollback now restores the snapshot value for pre-existing cookies overwritten by a You.com 3. 4. Full |
AI Code Review审查结论通过 审查范围： 发现的问题中等
问题： 触发条件： 使用共享 实际影响： You.com 写入的 cookie 永久滞留在共享 jar 中，之后同一 修正方向： 在 中等
问题： 恢复循环（第 972-981 行）只遍历 触发条件： 调用方在共享 实际影响： 调用方固定在共享 jar 上的 cookie 被 You.com 的响应永久移除，后续经同一 修正方向： 恢复循环应遍历 |
| resp = await client.send(request, auth=None, follow_redirects=False) | ||
| # Record cookies this response added to the jar, so the | ||
| # finally-block rollback can distinguish them from cookies | ||
| # a concurrent call wrote to the same shared client. | ||
| for _c in client.cookies.jar: | ||
| _ck = (_c.name, _c.domain, _c.path) | ||
| if _ck not in cookie_snapshot: | ||
| cookies_added_by_call[_ck] = _c.value or "" |
There was a problem hiding this comment.
问题： _post_sse 的 cookie 回滚依赖 cookies_added_by_call 记录，但该记录循环位于 await client.send(...) 成功返回之后（第 808-815 行）。当 send() 因响应体读取中途失败（如 SSE 流停滞触发 httpx.ReadTimeout）而抛异常时，cookies_added_by_call 保持为空，而 httpx 的 extract_cookies 在 _send_single_request 中、响应体读取之前就已把 Set-Cookie 写入共享 jar（已用 httpx 0.27.2 实测验证：发送失败后 jar 中仍残留 you_session=.api.you.com）。finally 块因此既不删除也不恢复任何 cookie，与代码注释声称的"rollback 在任何异常路径上都会执行"（第 933-935 行）相违背。
触发条件： 使用共享 http_client 且 provider 为 youcom 时，第一跳（或任一跳）响应携带 Set-Cookie 且正文读取超时/中断（You.com 的 streamable HTTP 长连接若不持续推流，15 秒默认 read timeout 极易命中）。
实际影响： You.com 写入的 cookie 永久滞留在共享 jar 中，之后同一 http_client 上所有 provider（Tavily、Google、DDG）的调用都会通过 Cookie 头把它转发给第三方 API——正是该回滚机制要防止的跨 provider 凭据泄漏，且会造成共享客户端状态污染。
修正方向： 在 client.send(...) 调用之前无法预知响应，应将 cookie 记录逻辑放到 finally 块内（遍历当前 jar 与快照差集，不依赖 send 成功后执行），或把 recording 循环移到 try 之外无法覆盖的异常路径上并行处理（如用 try/except 包裹 send()，在 except 中同样执行差集记录后再 re-raise）。
| for cookie in list(client.cookies.jar): | ||
| key = (cookie.name, cookie.domain, cookie.path) | ||
| if key not in cookie_snapshot: | ||
| continue | ||
| snap_value = cookie_snapshot[key] or "" | ||
| if (cookie.value or "") == snap_value: | ||
| continue | ||
| domain = cookie.domain.lstrip(".") | ||
| if endpoint_host == domain or endpoint_host.endswith("." + domain): | ||
| client.cookies.set(cookie.name, snap_value, domain=cookie.domain, path=cookie.path) |
There was a problem hiding this comment.
问题： 恢复循环（第 972-981 行）只遍历 client.cookies.jar 中仍然存在的 cookie。当 You.com 响应通过 Set-Cookie: name=; Max-Age=0（或 Expires=<过去时间>）删除一个快照中已存在的同名 cookie 时，该 cookie 已从 jar 中消失，循环无法命中它，快照值永远不会被恢复（已用 httpx 0.27.2 实测：快照中有 you_session=old、响应删除后 jar 为空、恢复循环结束后仍为空）。代码注释声称"对快照中存在且当前值不一致的 cookie 恢复快照值"（"for any cookie present in the snapshot whose current value differs from the snapshot, restore the snapshot value"），但删除场景下该契约未实现。
触发条件： 调用方在共享 http_client 上固定了与 You.com 响应的 Set-Cookie 同名的 cookie（例如为 api.you.com 配置的网关会话），而 You.com 在对该请求的响应中通过 Max-Age=0/过期 Expires 清除该 cookie（会话失效、登出类流程的常见行为）。
实际影响： 调用方固定在共享 jar 上的 cookie 被 You.com 的响应永久移除，后续经同一 http_client 发出的请求（包括其他 provider）丢失该会话凭据，导致会话中断或鉴权失败；与快速失败相比，这是共享客户端状态被第三方响应破坏的静默影响。
修正方向： 恢复循环应遍历 cookie_snapshot（而非仅遍历 jar）：对快照中存在、但当前 jar 中已消失（或值不一致）且域与 endpoint host 匹配的 cookie，用 client.cookies.set(...) 以快照值重建，而不是仅对现存 cookie 做值恢复。
…cookies The cookie-isolation rollback in _post_sse had two gaps flagged in review: 1. cookies_added_by_call was recorded only after a successful send(), so a mid-stream ReadTimeout (httpx extracts Set-Cookie before the body read) left You.com's cookies in the shared jar with nothing recording them — the finally block skipped deletion and they leaked to other providers via the shared http_client. Recording now runs in an except around send() (re-raising) as well as on success. 2. The restore loop iterated client.cookies.jar, so a snapshot cookie that a You.com Set-Cookie deleted (Max-Age=0 / expired Expires) was gone from the jar and never restored — the caller's pinned session stayed missing for subsequent calls. The loop now iterates the snapshot and restores any cookie whose current value differs or is missing, scoped to the endpoint host. Adds two offline tests (send-exception rollback, deleted-cookie restore) following the existing cookie-rollback test pattern.
|
Thanks for the second pass. Both MODERATE findings are fixed in 8123290, with the two missing failure-path tests you flagged added:
Both the concurrent-value guard and the endpoint-host scoping are preserved. Full suite passes locally ( |
AI Code Review审查结论通过 审查范围审查 计划符合性计划为"feat: add youcom (You.com) provider to WebSearchTool"。实现完整覆盖： 验证过程（高置信结论的基础）
风险与说明
门禁结论PASS（无评论）。该变更正确、完整、测试充分，满足"youcom provider"计划要求。 发现的问题未发现需要反馈的代码问题。 |
Why
The default
duckduckgobackend is keyless but returns DDG's curated instant-answer set rather than real web results, and thegoogle/tavilybackends both require an API key before they return anything. You.com exposes itsyou-searchMCP tool through a keyless free-profile endpoint (https://api.you.com/mcp?profile=free), so it fits the existing provider surface as a zero-setup backend that returns real web hits — the same property that makes the DDG default attractive.What changed
trpc_agent_sdk/tools/_websearch_tool.py: new"youcom"provider following the existing provider pattern —ProviderTypeliteral,_search_youcomdispatch,youcom_extra_paramspassthrough,YDC_API_KEYenv fallback. The endpoint is stateless JSON-RPC over streamable HTTP: onetools/callPOST answered by atext/event-streambody, parsed by a new_parse_sse_payloadshelper (progress notifications are skipped, unparseable lines ignored). Keyless by default; a configured key switches tohttps://api.you.com/mcpwith aBearerheader.SearchHit(description → snippet, first query-relevant highlight as fallback)langmaps to You.com's inlinelang:filter;blocked_domainsmap to server-sideexclude_domains,allowed_domainsfilter client-side (plus the usual_is_blockedpass)isErrorresults surface assummarytext instead of raisingtests/tools/test_websearch_tool.py:TestYoucomProvidermirroring the Tavily coverage — SSE parsing, keyless/authenticated base URLs, Bearer header, Accept header requirement (the endpoint returns 406 for JSON-only), inlinelang:mapping, server-sideexclude_domains, client-side allowlist, extra params, count clamping, URL dedup, JSON-RPC/isError/missing-result/non-JSON error paths. Plus keyless-by-default and env-fallback init tests and a base-props-only declaration check.docs/mkdocs/en/tool.md+docs/mkdocs/zh/tool.md: backend lists, parameter tables, return-fields table, and a usage example for the keyless configuration (the English page also catches up the missingtavilyrows it was lagging).Default behavior is unchanged:
provider="duckduckgo"stays the default and all existing provider paths are untouched — this is purely additive and opt-in viaprovider="youcom".Setup
No setup required — the free profile is keyless. Optional upgrades:
Validation
pytest tests/tools/test_websearch_tool.py→ 91 passed (75 pre-existing + 16 new)yapf --diffandflake8clean on both changed Python files (the CI checks)test_agent_tool.py,test_function_parameter_parse.py, langgraph/DSL collection errors) fail identically onmainin this environment — unrelated to this changeFor context, You.com also ships ready-made agent skills and MCP server configs (https://github.com/youdotcom-oss/agent-skills) for harnesses that want the full MCP server rather than a single tool.
Happy to adjust the shape if you'd rather land this differently — e.g. folding the SSE handling into the existing
_post_jsonhelper, or trimming the docs diff.Tracking: youdotcom-oss/integration-tracking#477