Repository navigation
fix: 修复SkillToolSet中tool_filter失效的问题(#352) - #357
Conversation
AI Code Review审查结论通过 审查范围与方法审查 计划符合性核心修复成立： 主要风险（本变更引入）
测试充分性新增 3 个参数化测试验证了主流程（列表过滤、谓词按上下文重判、include-all 覆盖），但在上述真实失败路径（None 上下文、子代理、空过滤、非 list 过滤）上存在缺口。 门禁结论无高置信的 SEVERE 级缺陷（核心修复正确，无数据损坏与主运行路径崩溃），存在 2 个 MODERATE 运行时行为回归与 1 个 MODERATE 文档-行为不一致，以及若干 LOW 边界/契约回归。结论：PASSED，建议合入前修复谓词 None 上下文与子代理两类 MODERATE 问题并修正文档示例。 发现的问题中等
问题： 新增的 触发条件： 配置 实际影响： a2a 卡片构建与 ag_ui 长时运行检测的 修正方向： 在 中等
问题： 本次变更使 触发条件： 父级 实际影响： 子代理在无任何报错的情况下丢失全部 Skill 工具（ 修正方向： 明确子代理场景下谓词的求值上下文——要么让 中等
问题： 本次新增的中英文档示例 触发条件： 用户按本次新增的 zh/en 实际影响： 文档承诺的访问约束门控不生效， 修正方向： 将文档示例改为顶层 较低
问题： 触发条件： 任一调用方对 实际影响： 一旦发生修改， 修正方向： 两条路径统一为 较低
问题： 触发条件： 运维以空列表表达“临时禁用全部工具 / 零白名单”（在访问控制场景下是合理意图）。 实际影响： 静默暴露全部内置工具（含 修正方向： 将判断改为 较低
问题： 触发条件： 实际影响： 按会话/租户收紧的访问控制在动态工具层被静默绕过（已缓存工具直接返回），或 preflight 的截断结果被永久缓存导致工具持续缺失，与权限控制类部署场景直接相关。 修正方向： 在 较低
问题： 本次新增测试未覆盖修复行为的关键失败路径与边界：1) 谓词型过滤在 触发条件： 谓词过滤真实部署到 a2a 卡片、子代理等路径时的回归无法被这些测试拦截。 实际影响： 测试对修复行为给出虚假信心，静默丢工具（卡片/子代理/零工具）等真实缺陷会直接发布。 修正方向： 补充 |
| def _get_selected_tools(self, invocation_context: Optional[InvocationContext]) -> List[ToolABC]: | ||
| """Return tools selected for the current invocation.""" | ||
| if not self._tool_filter or self._is_include_all_tools: | ||
| return self._default_tools | ||
| return [tool for tool in self._default_tools if self._is_tool_selected(tool, invocation_context)] |
There was a problem hiding this comment.
问题： 新增的 _get_selected_tools 把调用方传入的 invocation_context 原样传给 base 的 _is_tool_selected（abc/_toolset.py:107-108），谓词型 tool_filter 在上下文缺失时会对 None 直接解引用——本次新增文档中的谓词示例 invocation_context.session_state.get("allowed_skill_tools", []) 会抛 AttributeError。变更前 get_tools 从不执行谓词（这也是本修复的目标），此崩溃路径完全由本次变更引入。
触发条件： 配置 tool_filter=<谓词函数> 且 is_include_all_tools=False，随后 get_tools 在无有效上下文处被调用：server/a2a/_agent_card_builder.py:197 与 a2a_v1/_agent_card_builder.py:212 显式传 None（卡片构建发生在 agent.run 之外，而 get_invocation_ctx() 的 contextvar 默认值也是 None，行 167-168 的回退无效）；server/ag_ui/_core/_agui_agent.py:411 同样传 None；或用户按 skills/__init__.py:25 的文档示例单独调用 await toolset.get_tools()（contextvar 未设置）。
实际影响： a2a 卡片构建与 ag_ui 长时运行检测的 try/except（_agent_card_builder.py:149-154、_agui_agent.py:410-414）吞掉异常后，Agent 卡片上全部 Skill 工具被静默丢弃（运行时缓存已填充、单独跑正常，卡片与实际能力长期不一致）；独立调用场景则直接向调用方抛出 AttributeError。
修正方向： 在 _get_selected_tools 中对 invocation_context is None 跳过谓词求值（符合 base ToolSetABC.get_tools 文档 'If None, all tools are returned' 的契约），或改为先取 get_invocation_ctx() 且保证非 None 后再调用谓词，并为 ctx=None 场景补充测试。
| if self._default_tools: | ||
| return self._default_tools.copy() | ||
| return self._get_selected_tools(invocation_context) |
There was a problem hiding this comment.
问题： 本次变更使 tool_filter 谓词在每次 get_tools() 时以“当前” InvocationContext 重新求值；而子代理通过 _BorrowedToolSet（agents/sub_agent/_runner.py:58-59,168）继承父级 SkillToolSet 实例，子代理会话是全新空状态（_runner.py:376 state={}）。父级配置的会话相关谓词（正是文档示例的模式）在子代理上下文中求值恒返回 false，全部继承的 Skill 工具被过滤为空。变更前过滤是死代码，子代理始终继承完整工具集，属本次修复引入的行为回归。
触发条件： 父级 SkillToolSet 配置依赖 session_state 的谓词过滤（is_include_all_tools=False），并以默认方式（archetype.tools is None 继承父级工具）派生子代理。
实际影响： 子代理在无任何报错的情况下丢失全部 Skill 工具（ToolsProcessor 仅记录 'No valid tools to add to request'），能力被静默削弱；若谓词还会崩溃，子代理请求直接失败。父代理与子代理看到的工具能力不一致。
修正方向： 明确子代理场景下谓词的求值上下文——要么让 _BorrowedToolSet/子代理透传父级 parent_ctx，要么在无继承会话状态时跳过谓词并显式告警，并在本次新增的文档中说明谓词过滤与子代理的交互及限制。
| tool_filter=["skill_load", "skill_run"], | ||
| is_include_all_tools=False, | ||
| run_tool_kwargs={"require_skill_loaded": True}, | ||
| ) |
There was a problem hiding this comment.
问题： 本次新增的中英文档示例 run_tool_kwargs={"require_skill_loaded": True} 声称开启“必须先 skill_load 才能 skill_run”的门控，但该嵌套写法在运行期被静默丢弃：SkillToolSet.__init__ 的 **run_tool_kwargs（_toolset.py:81）把这个嵌套 dict 以名为 run_tool_kwargs 的关键字传给 SkillRunTool，落入其 **kwargs（_skill_run.py:384,436）并存入 self._run_tool_kwargs；执行期仅将 SkillRunInput.model_fields 内的键写回参数（_skill_run.py:647-650），而 require_skill_loaded 不是输入字段，构造参数 _require_skill_loaded 保持默认 False（已用签名级绑定推演验证）。只有 examples/skills_code_review_agent/agent/tools.py:78 那种把 require_skill_loaded=True 作为顶层关键字传入才生效。
触发条件： 用户按本次新增的 zh/en skill.md 文档示例原样配置 SkillToolSet。
实际影响： 文档承诺的访问约束门控不生效，skill_run 无需先 skill_load 即可直接调用，与文档“必须同时允许 skill_load”的说明矛盾且无任何警告，治理型部署按文档配置后得不到预期保护。
修正方向： 将文档示例改为顶层 require_skill_loaded=True（与 GovernedSkillToolSet 实际用法一致），或在 SkillRunTool 中对 _run_tool_kwargs 中无法识别的键（非 SkillRunInput.model_fields）打印告警以免静默失效。
| if not self._tool_filter or self._is_include_all_tools: | ||
| return self._default_tools |
There was a problem hiding this comment.
问题： _get_selected_tools 在默认路径（tool_filter=None 或 is_include_all_tools=True，即绝大多数用例）直接返回内部列表 self._default_tools 本身，取代了变更前 return self._default_tools.copy()（行 177 旧代码）的快照语义；首次调用路径（行 192-193）也从返回局部新建的 tools 列表变成返回共享缓存列表。此后所有 get_tools() 调用方拿到的是同一个可变列表对象，且过滤路径返回新列表、默认路径返回共享列表，同一实例两种语义并存。
触发条件： 任一调用方对 get_tools() 返回值做 append/pop/sort/remove/clear 等原地修改。当前仓库内 8 处调用（_tool_adapter.py:104、_claude_agent.py:461,501、_agui_agent.py:411、_mcp.py:44、_dynamic_toolset.py:193、_runner.py:59 等）经逐一核对均为只读，风险为潜在回归；但对发布版 SDK 的外部调用方，旧契约（返回快照）允许修改返回值。
实际影响： 一旦发生修改，_default_tools 缓存被永久污染，后续所有会话的工具列表缺失或错乱；此变更删除了有意为之的防御性拷贝。
修正方向： 两条路径统一为 return [tool for tool in self._default_tools if self._is_tool_selected(tool, invocation_context)]——base 的 _is_tool_selected（abc/_toolset.py:104）已自带 not self._tool_filter or self._is_include_all_tools 短路，一行表达式同时消除重复判断并天然返回新列表，一举修复别名泄漏与重复逻辑。
| if not self._tool_filter or self._is_include_all_tools: | ||
| return self._default_tools | ||
| return [tool for tool in self._default_tools if self._is_tool_selected(tool, invocation_context)] |
There was a problem hiding this comment.
问题： _get_selected_tools 行 149 用 if not self._tool_filter or self._is_include_all_tools 判断，空列表 [] 是 falsy，tool_filter=[] + is_include_all_tools=False 时会走 return self._default_tools 暴露全部工具，与本次文档宣传的“过滤器采用白名单语义、用于权限控制”相反。
触发条件： 运维以空列表表达“临时禁用全部工具 / 零白名单”（在访问控制场景下是合理意图）。
实际影响： 静默暴露全部内置工具（含 skill_exec、workspace_exec 等执行类工具），权限控制意图被反转且无任何告警。
修正方向： 将判断改为 if self._tool_filter is None or self._is_include_all_tools，并明确空列表语义（返回空工具集或抛出校验错误）。
| if self._default_tools: | ||
| return self._default_tools.copy() | ||
| return self._get_selected_tools(invocation_context) |
There was a problem hiding this comment.
问题： DynamicSkillToolSet._resolve_tool（skills/_dynamic_toolset.py:181-198）把 toolset.get_tools(ctx) 返回的每个工具永久缓存在 _tool_cache 且优先命中（行 181-182）。当该动态工具集的 available_toolsets 中含有本次新启用过滤的 SkillToolSet 时，工具名一旦在宽松上下文下首次解析，之后的收紧上下文调用直接命中缓存返回，谓词不再重新求值——与本次新增文档“Predicate 会在每次 get_tools() 时使用当前 InvocationContext 重新判断”的承诺冲突。
触发条件： DynamicSkillToolSet（其 docstring 行 76-86 明确支持 ToolSet 混合配置）与带谓词过滤的 SkillToolSet 组合使用，且技能选中的工具名此前已被解析过；或 a2a/ag_ui 的 preflight 空结果先被缓存。
实际影响： 按会话/租户收紧的访问控制在动态工具层被静默绕过（已缓存工具直接返回），或 preflight 的截断结果被永久缓存导致工具持续缺失，与权限控制类部署场景直接相关。
修正方向： 在 _resolve_tool 中区分“工具实例缓存”与“每次按上下文重新做的谓词判定”——缓存命中也应每次用当前 ctx 复核一次谓词，或为缓存项记录求值时上下文并按其失效。
| async def test_predicate_rechecks_current_context_without_filtering_cache(self, tmp_path, toolset_cls): | ||
|
|
||
| def predicate(tool, invocation_context): | ||
| return tool.name in invocation_context.allowed_tools | ||
|
|
||
| ts = toolset_cls( | ||
| paths=[str(tmp_path)], | ||
| tool_filter=predicate, | ||
| is_include_all_tools=False, | ||
| ) | ||
|
|
||
| for allowed_tools in ({"skill_run"}, {"skill_load"}): | ||
| ctx = _make_ctx() | ||
| ctx.allowed_tools = allowed_tools | ||
| tools = await ts.get_tools(ctx) | ||
| assert {tool.name for tool in tools} == allowed_tools | ||
|
|
||
| async def test_include_all_tools_overrides_filter(self, tmp_path, toolset_cls): | ||
| ts = toolset_cls( | ||
| paths=[str(tmp_path)], | ||
| tool_filter=["skill_load"], | ||
| is_include_all_tools=True, | ||
| ) | ||
|
|
||
| tools = await ts.get_tools(_make_ctx()) | ||
| assert "skill_load" in {tool.name for tool in tools} | ||
| assert len(tools) > 1 |
There was a problem hiding this comment.
问题： 本次新增测试未覆盖修复行为的关键失败路径与边界：1) 谓词型过滤在 ctx=None（a2a 卡片、ag_ui、独立 get_tools()）下崩溃的场景没有用例；2) test_include_all_tools_overrides_filter 仅断言 len(tools) > 1，未断言等于完整未过滤集合，回归到“部分工具被误过滤”也能通过；3) 空列表 tool_filter=[] 与非 list 可迭代（tuple/set，_is_tool_selected 对它们会落到底部 return False 静默返回零工具）均未覆盖。
触发条件： 谓词过滤真实部署到 a2a 卡片、子代理等路径时的回归无法被这些测试拦截。
实际影响： 测试对修复行为给出虚假信心，静默丢工具（卡片/子代理/零工具）等真实缺陷会直接发布。
修正方向： 补充 ctx=None 谓词用例、is_include_all_tools=True 下与完整集合的相等断言，以及 tuple/set/空列表过滤的边界用例。
3 participants
No description provided.