fix: 兼容 additionalProperties schema 解析#104
Conversation
There was a problem hiding this comment.
Pull request overview
本 PR 旨在让 ToolSchema(tool 与 toolset 两处)在解析 OpenAPI/JSON Schema 时,能够正确处理 additionalProperties 为“schema 对象”的情况,并在序列化回 JSON Schema 时保持该结构不丢失,从而提升对 MCP/OpenAPI 真实 schema 的兼容性。
Changes:
- 将
additional_properties字段类型从bool扩展为Union[bool, ToolSchema],支持 schema-valuedadditionalProperties from_any_openapi_schema递归解析additionalProperties为ToolSchema,to_json_schema序列化时保留为对象- 增加回归测试覆盖 OpenAPI 与 MCP tool schema 中的该场景
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
agentrun/tool/model.py |
扩展 Tool 侧 ToolSchema.additional_properties 支持 schema 对象并在序列化时保留 |
agentrun/toolset/model.py |
扩展 ToolSet 侧 ToolSchema.additional_properties 支持 schema 对象并在序列化时保留 |
tests/unittests/tool/test_model.py |
新增 OpenAPI additionalProperties=Schema 与 MCP schema 回归测试 |
tests/unittests/toolset/test_model.py |
新增 OpenAPI additionalProperties=Schema 回归测试 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
d1aacf2 to
514015b
Compare
514015b to
59a0730
Compare
Sodawyx
left a comment
There was a problem hiding this comment.
核心修复(让 ToolSchema 兼容 schema 形式的 additionalProperties)方向正确、实现合理、测试覆盖到位(OpenAPI、MCP、空 schema 三种 case),整体 LGTM。下面几点建议主要不在代码逻辑本身,而在 PR 工程化层面:
1. 混入了大量与核心修复无关的格式化变更
实际只有 4 个文件和本次 fix 直接相关(agentrun/tool/model.py、agentrun/toolset/model.py 和两个对应的 test),剩下 15 个文件全是纯空白/换行格式化(credential / knowledgebase / memory_collection / model / sandbox / 一些 tests)。
这导致 diff 从 ~50 行的真实修复膨胀到 +268/-59,且:
- review 时需要在 15 个无关 diff 之间过滤核心修复,认知负担增加
- 容易和并行开发产生 merge 冲突
git blame信号被稀释:这些文件最后一次修改的人会显示成是本次格式化提交,而不是真正改逻辑的人
建议拆成两个独立 commit/PR:
fix(tool, toolset): 兼容 additionalProperties schema 解析—— 只动 4 个相关文件style: format code—— 其余 15 个文件,并且最好是跑一次完整的 formatter(black / ruff format)让所有文件保持一致,而不是手工挑文件改
2. Commit message 与实际内容不符
当前 PR 只有 1 个 commit,title 是 style(...): format code for better readability,但实际把这次 fix 也包进去了。按 Conventional Commits,核心修复应当是 fix: 前缀,这关系到 changelog/release notes 的自动生成,也会影响后续 cherry-pick / revert 的判断。建议拆分时把 fix commit 显式分离出来。
3. (pre-existing, 不必本 PR 解决) ToolSchema 在两个 package 下重复定义
agentrun/tool/model.py 和 agentrun/toolset/model.py 各有一份几乎一字不差的 ToolSchema,本次同样的 fix 需要两边各打一遍。短期可以接受,但建议在 issue 里跟踪一下后续抽取共享基类的工作。
| additional_properties_raw = pydash_get(schema, "additionalProperties") | ||
| additional_properties = ( | ||
| cls() | ||
| if additional_properties_raw == {} |
There was a problem hiding this comment.
这一行 additional_properties_raw == {} 单独走 cls() 分支是 load-bearing 的,建议加一行注释说明原因,避免后续被人当成多余分支删掉。
原因:from_any_openapi_schema 在第 255 行有一个早返回:
if not schema or not isinstance(schema, dict):
return cls(type="string")not {} 为 True,所以如果直接递归 cls.from_any_openapi_schema({}),会得到 ToolSchema(type="string"),再序列化就变成 {"type": "string"},丢失 "任意 schema" 的语义(这正是 Copilot 之前指出的 bug)。
或者更稳健的做法是在入口区分 "无效输入" 和 "合法的空 schema":
if schema is None or not isinstance(schema, dict):
return cls(type="string")
# 空 dict 是合法的 JSON Schema('any'),继续往下走这样这里就不再需要 == {} 的特判,整体逻辑也更可读。
| if isinstance(additional_properties_raw, dict) | ||
| else additional_properties_raw | ||
| ) | ||
| ) |
There was a problem hiding this comment.
嵌套三元(A if X else (B if Y else C))读起来比较费劲,建议改成显式 if/elif/else,更容易跟着分支判断走:
if additional_properties_raw == {}:
additional_properties = cls()
elif isinstance(additional_properties_raw, dict):
additional_properties = cls.from_any_openapi_schema(additional_properties_raw)
else:
additional_properties = additional_properties_raw # bool or None等价但可读性更好,同样的写法也适用于 agentrun/toolset/model.py:191。
| if isinstance(additional_properties_raw, dict) | ||
| else additional_properties_raw | ||
| ) | ||
| ) |
There was a problem hiding this comment.
这段和 agentrun/tool/model.py:295-303 一模一样(只是 pydash_get 在这里 alias 成了 pg)。本次 PR 已经被迫两边各打一遍 patch,不算关键问题,但建议在 issue 里跟踪 ToolSchema 跨 package 的统一工作——同步成本只会越来越高。
59a0730 to
a4e6903
Compare
- 将 additional_properties 字段类型从 bool 扩展为 Union[bool, ToolSchema]
- 递归解析 schema-valued additionalProperties,序列化时保留为对象
- 修正 from_any_openapi_schema 入口条件:if not schema → if schema is None,
使空 dict {} 作为合法 JSON Schema(any)正确解析,而非降级为 string
- 将嵌套三元表达式重构为 if/elif/else 提升可读性
- 添加 OpenAPI、MCP tool schema 及空 schema 的回归测试
Change-Id: I1e14f80b169adbb7693f0ee668ed6462e2a4803c
Co-developed-by: Claude <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Signed-off-by: OhYee <oyohyee@oyohyee.com>
…format code for better readability Change-Id: If7b4e36b9005765d075e7ac2e535a10c5beb4f37 Co-developed-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: OhYee <oyohyee@oyohyee.com>
a4e6903 to
e9b7d2f
Compare
Summary
Validation