♻️ refactor(mcp): 统一 MCP 工具注册与协议入口 - #155
Conversation
移除硬编码的日程工具实现，新增工具注册、查询、列表和回调分发能力，并完善参数校验与中文注释
显式初始化 ToolCall 的全部字段，兼容 GCC 严格告警编译
保持组件依赖声明与仓库架构检查规则一致
限制为字符串参数，统一处理必填参数、默认值和多余参数
引入类型安全的工具参数值并在网关边界校验参数类型
原有 ToolDefinition 直接使用 input 列表，无法表达 MCP 要求的 inputSchema.type、properties 和 required 层级。\n\n新增对象根 Schema，并将字段名移入 properties、必填约束移入 required；同步调整注册校验、默认值补全和主机测试。\n\n主机 CMake 构建及 7 个测试全部通过。\n\nBREAKING CHANGE: ToolDefinition 的 input 接口迁移为 input_schema，调用方需按 JSON Schema 结构填写工具参数。
删除 McpToolGateway 和独立定义文件，由 McpServer 统一负责注册、校验、分发和 Schema 导出，并重写专项测试
合并最新 Schedule、Timing、Storage 与 CI 变更，并保留 McpServer 单入口设计
![fennoai[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found two CI-blocking issues in this diff. Details are inline.
Verification run locally against the locked PR range:
python3 scripts/check_public_api_docs.pyfails on the new MCP public header comments.cmake -S tests/host -B /tmp/voicelife-host-reviewfails while resolvingcjson/cJSON.h.
| namespace voicelife::mcp { | ||
|
|
||
| // MCP 工具参数支持的数据类型。 | ||
| enum class ToolInputType { kString, kInteger, kBoolean }; |
There was a problem hiding this comment.
The new public header comments are plain // comments, but the repository's public API docs gate only accepts Doxygen comments. Running python3 scripts/check_public_api_docs.py fails on this header starting here and then on the other public types/functions/accessors, so scripts/run_checks.sh will fail before tests run. Please convert the public API comments in this header to the required /// or /** ... */ form and cover the inline public functions as well.
| "${ROOT_DIR}/components/voicelife_mcp/src/mcp_tool_gateway.cc") | ||
| target_link_libraries(mcp PUBLIC contracts) | ||
| "${ROOT_DIR}/components/voicelife_mcp/src/mcp_server.cc") | ||
| find_path(CJSON_INCLUDE_DIR cjson/cJSON.h PATHS /usr/local/include /opt/homebrew/include REQUIRED) |
There was a problem hiding this comment.
This makes the host test build require a system cJSON install, but the repo's GitHub Actions host/coverage jobs do not install libcjson-dev and the repo does not vendor a host cJSON target. In this workspace, cmake -S tests/host -B /tmp/voicelife-host-review now fails at configure with Could not find CJSON_INCLUDE_DIR, so the PR would break the existing ./scripts/run_checks.sh path. Please either provide a portable host-side dependency path/stub or update the CI/bootstrap scripts to install the dependency before making it required.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
@fennoai 你是资深后端/全栈架构师 + 嵌入式硬件工程师复合型专家，执行严格、客观、不留情面的代码仓库Review，请遵循下面所有评审规则，逐条输出审查结果，禁止敷衍、禁止只说空话、禁止笼统概括。 评审维度
输出要求
现在开始对提供的仓库代码/PR变更、硬件资料、结构模型进行评审 |
Review Findings
整体仓库风险总结这次重构方向是对的，但当前实现把“协议 schema、运行时参数契约、序列化、构建依赖”一起改了，最危险的是契约退化和 host 构建不可用。现在最直接的故障面不是业务逻辑本身，而是工具定义会失真、默认值会越界、空环境下连测试都起不来。 优先级整改清单
长期架构、软硬件协同优化方案把 验证：我还尝试了 |
|
@fennoai 当前实现使用了cJSON 但是cJSON会接受 RFC 8259 禁止的前导零 01 你觉得这个问题需要注意吗 |
|
当前PR不会复现 #149 里最核心的“宽松解析器导致双端拒绝语义漂移”问题，因为它当前没有用 cJSON 解析输入。 PR 中 cJSON 只出现在：
因此， 但当前序列化实现仍有几个类似的契约风险。 需要修正的问题
cJSON_AddNumberToObject(property, "minimum",
static_cast<double>(*field.minimum));但公共 API 支持完整的 我用 cJSON 1.7.19 实测： 这会造成导出的 JSON Schema 与设备实际的整数校验范围不一致。尤其是 解决方向只有两个：
mcp_server.cc#L97-L120 多处把 cJSON_AddStringToObject(tool, "name", definition.name.c_str());实测： 所以 另外，非法 UTF-8 字节会被 cJSON 原样写入输出。例如 如果工具定义被认为是可信的编译期数据，这个风险较低；但当前 API 没有声明或校验这个前提。建议至少：
这和 #149 中“内嵌 NUL 不应丢失”的测试关注点是同一类边界问题，只不过这里发生在序列化端。
mcp_server.cc#L93-L127 没有检查 如果内存不足， 对于 ESP32，建议让接口返回带错误状态的结果，或者至少检查整棵 cJSON 树和 关于“IDF 自带 json” 这里需要区分 ESP-IDF 版本：
所以当前PR使用： PRIV_REQUIRES espressif__cjson以及 idf_component.yml 是符合 IDF 6 官方迁移方式的。不能简单改成 我的判断 我不会因为“cJSON 宽松解析”这个理由否掉当前PR；当前 PR 是输出-only，和 #149 的输入解析场景不同。 但我不会让它按当前实现直接合入，至少应补：
另外，#149 已经把 yyjson 放进当前 |
结论要注意，但就当前 PR 的代码路径来说，它不是阻塞项。 现在 所以我的判断是：
对应修复方向只有一个：外部输入边界用严格 JSON 解析/校验，不要把 |
# Conflicts: # components/voicelife_runtime/src/runtime.cc
关于范围问题，只有-9,007,199,254,740,991 ～ 9,007,199,254,740,991在这个范围之外才会出现问题，这个范围已经足够大，满足我们的业务需求。 不过为了最终统一，我决定改为 yyJSON |
|
@fennoai 你是资深后端/全栈架构师 + 嵌入式硬件工程师复合型专家，执行严格、客观、不留情面的代码仓库Review，请遵循下面所有评审规则，逐条输出审查结果，禁止敷衍、禁止只说空话、禁止笼统概括。 评审维度
输出要求
现在开始对提供的仓库代码/PR变更、硬件资料、结构模型进行评审 |
Review Complete
已提交一条 |
There was a problem hiding this comment.
Review Findings
- 🔴 [components/voicelife_mcp/src/mcp_server.cc:66-79] Empty property names are not rejected. Risk: a tool can register an unnamed parameter, which produces a schema key clients cannot address reliably. Fix: reject
property.name().empty()during registration. - 🔴 [components/voicelife_mcp/src/mcp_server.cc:66-79] Duplicate property names are silently collapsed by
unordered_map::emplace. Risk: schema and runtime contracts diverge, and the later property never reachestools/list. Fix: reject duplicate names before building the schema. - 🔴 [components/voicelife_mcp/src/mcp_server.cc:100-123] Default values are only type-checked, not range-checked. Risk: an out-of-range default is accepted and later injected into handler calls when the caller omits the field, which can push unsafe values downstream. Fix: validate defaults against the same min/max gate used at call time.
- 🟡 [components/voicelife_mcp/include/voicelife/mcp/mcp_server.h:52-77]
Propertyhas no way to setdescription. Risk: the schema serializer will always emit blank parameter docs, so generated MCP prompts lose context. Fix: add a description argument or setter and propagate it intoToolInputField. - 🟡 [components/voicelife_mcp/include/voicelife/mcp/mcp_server.h:52-77] Required-vs-optional semantics are inferred solely from the presence of a default. Risk: you cannot express “optional but no default”, so the public API is narrower than the protocol contract. Fix: add an explicit
requiredflag toPropertyand serialize it directly. - 🔴 [components/voicelife_mcp/src/mcp_json_writer.cc:118-153]
SerializeListToolsResult()never emitsdefault. Risk:tools/listtells clients a field is required even when runtime will auto-fill it, so model-facing contract and execution path diverge. Fix: serializedefaultalongsidetypeanddescription. - 🔴 [components/voicelife_mcp/src/mcp_json_writer.cc:118-153] The schema never emits
additionalProperties: false. Risk: the JSON schema suggests extra fields are allowed whileMcpServer::call()rejects them, causing protocol/client mismatch. Fix: writeadditionalProperties: falseinto every tool schema. - 🟡 [components/voicelife_mcp/src/mcp_json_writer.cc:134-153] Property iteration uses
unordered_map, so JSON key order changes across runs. Risk: snapshot tests, diff-based audits, and client caches see noisy churn. Fix: preserve insertion order in the schema model or sort keys before serialization. - 🔴 [components/voicelife_mcp/src/mcp_json_writer.cc:158-183] Any allocation or append failure collapses to
{}. Risk: OOM or serializer bugs are silently converted into a valid-looking empty object, making protocol failures indistinguishable from “no tools”. Fix: surface an error status or at least a distinguishable sentinel. - 🟡 [components/voicelife_mcp/src/mcp_server.cc:82-92]
list_tools_json()materializes a fullListToolsResultand then serializes it. Risk: every list call allocates and copies the full registry twice. Fix: serialize directly from the registry or cache the rendered JSON. - 🟢 [components/voicelife_mcp/src/mcp_server.cc:82-89]
list_tools()copies everyToolDefinitioninto a new vector on each call. Risk: repeated list calls create unnecessary heap churn. Fix: return a view/cached snapshot or store definitions in a stable container. - 🟡 [components/voicelife_mcp/src/mcp_server.cc:137-183]
McpServerhas no freeze or lock boundary. Risk: if registration and dispatch ever become concurrent, the registry becomes data-race prone and late registration changes live behavior. Fix: split build-time registration from read-only dispatch or guard the registry with a mutex and explicit freeze. - 🟢 [components/voicelife_mcp/src/mcp_server.cc:149-160] Required-parameter lookup does a linear
std::findfor every missing field. Risk: the check becomes quadratic as tool signatures grow. Fix: store required names in a set or a boolean bit on each field. - 🟢 [components/voicelife_mcp/src/mcp_server.cc:149-151]
defined_namesis rebuilt as anunordered_seton every call. Risk: each dispatch allocates extra memory and hashes every parameter name again. Fix: precompute the lookup structure at registration time. - 🟢 [components/voicelife_mcp/src/mcp_server.cc:94-97]
PropertyList::with_values()copies the whole arguments map before handing it to the callback. Risk: every call pays an extra map copy even when handlers only read one value. Fix: pass the normalizedToolCalldirectly or bind values by view. - 🟡 [components/voicelife_mcp/test/mcp_server_test.cc:63-85] Registration validation tests miss the two most dangerous new failures: empty parameter names and duplicate names. Risk: the contract bugs above can regress silently. Fix: add explicit negative cases for both.
- 🟡 [components/voicelife_mcp/test/mcp_server_test.cc:155-207] The JSON assertions only check a happy path and one integer-boundary tool. Risk: regressions in
default,required,additionalProperties, or property ordering will still pass. Fix: parse and assert those fields explicitly. - 🟡 [components/voicelife_mcp/test/mcp_server_test.cc:74-85] There is no test that an integer default outside min/max is rejected. Risk: the runtime can accept a tool definition that later injects unsafe values on omitted input. Fix: add a negative registration case for out-of-range defaults.
- 🟡 [tests/host/runtime_smoke_test.cc:36-48] The smoke test still never registers a real tool before dispatch. Risk: the new
McpServerbridge path can break without the integration test noticing. Fix: register a trivial tool and call it throughVoiceSessionCoordinator. - 🟡 [components/voicelife_mcp/test/CMakeLists.txt:1-35] The standalone component test project is not referenced by the root build or CI. Risk: it can rot while looking like coverage. Fix: either wire it into the build or remove it.
- 🟡 [components/voicelife_mcp/test/CMakeLists.txt:23-35, tests/host/CMakeLists.txt:178-181] The same
mcp_server_testis defined in two CMake entry points. Risk: future umbrella builds will hit target collisions or diverging test logic. Fix: keep one authoritative test target and delete the duplicate project.
Overall Risk
This refactor moves the right direction, but the current implementation still leaves contract drift between tool registration, schema export, and runtime dispatch. The biggest practical risks are invalid tool definitions slipping through, protocol output that does not describe runtime behavior accurately, and test coverage that still misses the new failure modes.
Priority Fixes
- Block invalid tool definitions first: empty names, duplicate names, and out-of-range defaults.
- Align schema output with runtime:
default,required, andadditionalPropertiesmust match call-time behavior. - Remove silent failure paths in JSON serialization.
- Tighten coverage on registration negatives and bridge-path integration.
Long-term
Split the MCP surface into a build-time registration model and a read-only runtime snapshot. That gives you one place for schema validation, one place for serialization, and a clear boundary for future concurrent registration or hardware-safety policy.
验证：./scripts/run_host_tests.sh -R mcp_server_test 已通过。
@fennoai response in Chinese |
中文回复这组评审结论是成立的，重点问题没有偏离：
结论保持不变：这些属于应优先修复的阻断项，先修契约和 schema 一致性，再补失败路径和测试覆盖。 |
3 participants
结论
本 PR 删除职责重复的
McpToolGateway，将工具定义、注册、参数校验、调用分发和tools/listJSON 导出统一收敛到McpServer。希望 Reviewer 重点判断：
McpServer是否适合作为唯一工具注册和协议入口McpServer的依赖方向是否合理Refs #154
变更
mcp_tool_gateway.h/.cctool_definition.hmcp_server.hMcpServer统一负责工具注册、列表和调用分发tools/listJSON 导出McpServermcp_server_testlist_tools_json()，便于检查实际协议输出明确未包含：
架构与兼容
公共接口发生变化：
McpToolGatewayMcpServer::add_tool()McpServer::call()McpServer依赖方向：