fix(mcp): normalize no-arg tool schemas for OpenAI compatibility#112
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the rewrite_tools_list function in src/main.rs to normalize tool input schemas by ensuring that object-type schemas include explicit properties, required, and additionalProperties fields. Corresponding unit tests were added to validate this normalization and confirm that existing properties are not overwritten. The review feedback correctly identifies that the new test assertions lack descriptive messages, which is a violation of the repository's coding standards.
| assert_eq!(input["type"], "object"); | ||
| assert_eq!(input["properties"], json!({})); | ||
| assert_eq!(input["required"], json!([])); | ||
| assert_eq!(input["additionalProperties"], false); |
There was a problem hiding this comment.
According to the repository's style guide (line 74), all assertions should include descriptive messages. This helps clarify the test's intent when it fails. Please add messages to these assertions.
assert_eq!(input["type"], "object", "Schema type should remain 'object'");
assert_eq!(input["properties"], json!({}), "A default empty 'properties' object should be added");
assert_eq!(input["required"], json!([]), "A default empty 'required' array should be added");
assert_eq!(input["additionalProperties"], false, "A default 'additionalProperties: false' should be added");References
- All assertions must include descriptive messages to clarify their purpose, especially upon failure. (link)
| assert_eq!( | ||
| parsed["result"]["tools"][0]["inputSchema"]["additionalProperties"], | ||
| true | ||
| ); |
There was a problem hiding this comment.
Per the repository style guide (line 74), assertions should include descriptive messages. Please add a message to this assertion to clarify what is being tested.
assert_eq!(
parsed["result"]["tools"][0]["inputSchema"]["additionalProperties"],
true,
"An existing 'additionalProperties' value should be preserved and not overwritten by the default"
);References
- All assertions must include descriptive messages to clarify their purpose, especially upon failure. (link)
|
Applied requested review fix on-head: added descriptive assertion messages to the new no-arg schema normalization tests in (commit c58431e).\n\nValidation status in this runtime:\n- Unable to run / / / locally because linker is missing ().\n\nNext concrete step:\n- Re-run CI and reviewer pass on latest head; if green, this PR is ready to merge. |
|
Superseding prior comment (shell interpolation issue). Applied requested review fix on-head: added descriptive assertion messages to the new no-arg schema normalization tests in Validation status in this runtime:
Next concrete step:
|
vish-dini
left a comment
There was a problem hiding this comment.
Reviewed on-head. Diff is surgical, CI is green, and the schema normalization aligns with current OpenAI function-calling requirements for object parameters. Approve and merge once branch protections are satisfied.
Summary
Fixes #106 by normalizing MCP tool
inputSchemaobjects in CLI proxytools/listrewrite so no-arg tools are always emitted with explicit OpenAI-compatible object fields.What changed
rewrite_tools_list, for anyinputSchemawheretype == "object", ensure defaults:properties: {}required: []additionalProperties: false{ "type": "object" }normalizationadditionalPropertiesWhy
OpenAI/Codex rejects object schemas lacking
properties. Some zero-arg tools (for examplehelp) can currently pass through as{ "type": "object" }, which breaks tool ingestion.Validation
src/main.rs.linker 'cc' not found.