acp-xcode: handle malformed Chinese-model tool calls - #102
setoelkahfi wants to merge 3 commits into
Conversation
![sigit-code-review[bot]](https://avatars.githubusercontent.com/in/3492899?s=60&v=4){kind=small}
There was a problem hiding this comment.
This change refactors how tool call handling is controlled in the backend, replacing Option<&[ToolSpec]> with a required &[ToolSpec] and a new allow_tool_calls boolean. This enables stricter suppression of tool calls (including malformed or inline ones) when tools are not allowed, especially for Chinese-models that emit tool calls as text with extra metadata. The inline_tool_calls module is extended to recognize and discard GLM-family CheckStatus preambles. Tests are added to verify that forced-text rounds suppress all tool calls, including malformed ones, and that no tool calls are executed or leaked to the client. All call sites are updated to the new API, and the OpenAiBackend logic is updated to enforce these boundaries both for structured and inline tool calls. Reviewers should focus on the new allow_tool_calls logic, the inline tool call extraction, and the new/updated tests for edge cases.
Automated review by siGit Code Review · commit 503b97e
setoelkahfi
changed the title
Co-Authored-By: siGit Code <noreply@sigit.si>
503b97e to
e1f3787
Compare
There was a problem hiding this comment.
This change improves handling of malformed or unexpected tool call markup in Chinese-model (GLM-family) responses, especially when tool calls are not allowed (forced-text rounds). The backend now reliably strips known tool call markup from model output, even when models emit tool calls as text or with GLM-specific 'CheckStatus' prefixes. The InferenceBackend trait and implementations are refactored to always pass the tool catalog and an explicit allow_tool_calls flag, simplifying logic and making suppression of tool calls more robust. The inline_tool_calls module is extended to recognize and discard GLM 'CheckStatus' preambles, with comprehensive new tests for edge cases. All call sites are updated to the new API. A new integration test ensures that forced-text rounds do not leak tool markup or execute unintended calls.
Automated review by siGit Code Review · commit e1f3787
The forced-text guard in consume_stream logged inside the per-delta loop, so one suppressed structured call could produce a warning per streamed argument fragment. Move the check to after tool calls are assembled, so it fires once per turn with the count and the names of what was dropped. consume_json's non-streaming path gets the same tool names added to its existing aggregate message. Co-Authored-By: siGit Code <noreply@sigit.si>
There was a problem hiding this comment.
This change improves handling of malformed tool call markup from Chinese GLM-family models, especially in forced-text rounds where tool calls should be suppressed. The InferenceBackend trait and its implementations are refactored to always pass the tool catalog and an explicit allow_tool_calls flag, ensuring correct suppression and recovery of tool calls. The inline_tool_calls module now recognizes and strips GLM CheckStatus preambles, with new tests for edge cases and chunked streaming. All call sites are updated to the new interface, and a new integration test verifies that forced-text rounds do not leak tool markup or execute unintended calls.
Automated review by siGit Code Review · commit 76e8848
history_replay_redraws_the_conversation_for_the_client built its test path from std::env::current_dir(), but four other tests briefly change that same process-global cwd under ENV_TEST_LOCK. This test never took the lock, so it could read one of those temp directories mid-swap. On CI that handed it a path long enough to trip the tool title's truncation, so the assertion compared a truncated title against an untruncated expectation and failed. Use a fixed path instead; the test never needed the real cwd to begin with. Co-Authored-By: siGit Code <noreply@sigit.si>
There was a problem hiding this comment.
This change improves handling of malformed or unexpected tool call markup, especially from Chinese GLM-family models, by updating the backend and inline tool call extraction logic. The InferenceBackend trait and its implementations now use an explicit allow_tool_calls flag instead of Option<&[ToolSpec]>, ensuring that tool calls are only executed or surfaced when allowed. The inline tool call parser is extended to recognize and discard GLM-specific CheckStatus preambles, and new tests verify that malformed or forced tool calls are suppressed from both execution and user-visible output. All call sites and tests are updated to match the new API and logic. Reviewers should focus on the new tool call suppression logic, correctness of the parser changes, and the new/updated tests for edge cases around forced-text rounds and malformed tool markup.
Automated review by siGit Code Review · commit 8198f4c
2 participants
No description provided.