fix: recover Kimi bash text tool calls#212
Closed
0xCheetah1 wants to merge 1 commit into
Closed
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGPT plain-text tool-call extraction now recognizes bash ChangesTextual tool-call extraction
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/textual-tool-calls.ts (2)
293-302: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winOptimize array validation to avoid
restspread allocation.Using the spread operator (
...rest) allocates a new array, which is inefficient ifparsed.cmdis unexpectedly large. Enforcing the exact expected length (parsed.cmd.length !== 3) upfront is safer and avoids unnecessary allocations.♻️ Proposed refactor
- if (!isPlainObject(parsed) || !Array.isArray(parsed.cmd)) return null; - const [bin, flag, command, ...rest] = parsed.cmd; - if ( - rest.length > 0 || - typeof bin !== "string" || - typeof flag !== "string" || - typeof command !== "string" - ) { - return null; - } + if (!isPlainObject(parsed) || !Array.isArray(parsed.cmd) || parsed.cmd.length !== 3) return null; + const [bin, flag, command] = parsed.cmd; + if (typeof bin !== "string" || typeof flag !== "string" || typeof command !== "string") { + return null; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/textual-tool-calls.ts` around lines 293 - 302, Update the parsed.cmd validation in the textual tool-call parsing flow to check parsed.cmd.length !== 3 before destructuring, then destructure only the three expected values without a rest array. Preserve the existing string-type validation and null-return behavior.
384-390: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid duplicate parsing and validation of the tool call.
callFromBashCmdObject(trailing.parsed)is evaluated twice when processing bash tool calls. Caching the result from the initial fallback avoids redundant array destructuring, string splitting, and object allocations.♻️ Proposed refactor
- const built = callFromGptObject(trailing.parsed) ?? callFromBashCmdObject(trailing.parsed); + const gptBuilt = callFromGptObject(trailing.parsed); + const bashBuilt = gptBuilt ? null : callFromBashCmdObject(trailing.parsed); + const built = gptBuilt ?? bashBuilt; if (built) { const hasProseBefore = content.slice(0, trailing.start).trim() !== ""; const typeIsFunction = (trailing.parsed as Record<string, unknown>).type === "function"; - const isBashCmd = - built.name === "terminal" && callFromBashCmdObject(trailing.parsed) !== null; + const isBashCmd = bashBuilt !== null; if (!hasProseBefore || typeIsFunction || isBashCmd) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/textual-tool-calls.ts` around lines 384 - 390, Cache the result of callFromBashCmdObject(trailing.parsed) when constructing built, then reuse that cached value for the isBashCmd check instead of invoking the parser again. Preserve the existing fallback order and bash-tool detection behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/textual-tool-calls.ts`:
- Around line 293-302: Update the parsed.cmd validation in the textual tool-call
parsing flow to check parsed.cmd.length !== 3 before destructuring, then
destructure only the three expected values without a rest array. Preserve the
existing string-type validation and null-return behavior.
- Around line 384-390: Cache the result of
callFromBashCmdObject(trailing.parsed) when constructing built, then reuse that
cached value for the isBashCmd check instead of invoking the parser again.
Preserve the existing fallback order and bash-tool detection behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bae19bbc-1473-4c37-acd1-99151f844d6d
⛔ Files ignored due to path filters (4)
dist/cli.jsis excluded by!**/dist/**dist/cli.js.mapis excluded by!**/dist/**,!**/*.mapdist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (2)
src/textual-tool-calls.test.tssrc/textual-tool-calls.ts
0xCheetah1
force-pushed
the
fix/kimi-cmd-text-tool-call
branch
from
July 18, 2026 10:32
32f08ea to
0063b94
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/textual-tool-calls.ts (1)
384-390: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid redundant evaluation of
callFromBashCmdObject.
callFromBashCmdObject(trailing.parsed)is evaluated twice in the worst case (once when computingbuiltand once when checkingisBashCmd). You can refactor this to capture the origin of the parsed object and avoid the duplicate call.♻️ Proposed refactor
- const built = callFromGptObject(trailing.parsed) ?? callFromBashCmdObject(trailing.parsed); + const gptCall = callFromGptObject(trailing.parsed); + const bashCall = gptCall ? null : callFromBashCmdObject(trailing.parsed); + const built = gptCall ?? bashCall; if (built) { const hasProseBefore = content.slice(0, trailing.start).trim() !== ""; const typeIsFunction = (trailing.parsed as Record<string, unknown>).type === "function"; - const isBashCmd = - built.name === "terminal" && callFromBashCmdObject(trailing.parsed) !== null; + const isBashCmd = bashCall !== null; if (!hasProseBefore || typeIsFunction || isBashCmd) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/textual-tool-calls.ts` around lines 384 - 390, Update the parsing logic around built, callFromGptObject, and callFromBashCmdObject to evaluate each parser once while retaining which parser produced the result. Use that captured origin when computing isBashCmd, preserving the existing terminal-name and condition behavior without invoking callFromBashCmdObject a second time.src/textual-tool-calls.test.ts (1)
275-285: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a test case for trailing bash command JSON after prose.
The source code explicitly allows bash command objects to be extracted even when preceded by prose (
if (!hasProseBefore || typeIsFunction || isBashCmd)). Adding a test for this specific trailing scenario would lock in this behavior and prevent regressions.🧪 Proposed test addition
it("does NOT overwrite an existing command when cmd is also present", () => { const content = '{"name":"terminal","parameters":{"cmd":"a","command":"b"}}'; const result = extractTextualToolCalls(content); const args = JSON.parse(result.toolCalls[0]!.function.arguments) as Record<string, unknown>; expect(args.command).toBe("b"); expect(args.cmd).toBe("a"); }); it("extracts Kimi/OpenCode bash cmd JSON as terminal", () => { const content = '{"cmd":["bash","-lc","ls -1a"],"timeout":10000}'; const result = extractTextualToolCalls(content); expect(result.toolCalls).toHaveLength(1); expect(result.toolCalls[0]?.function.name).toBe("terminal"); expect(JSON.parse(result.toolCalls[0]!.function.arguments)).toEqual({ command: "ls -1a", timeout: 10000, }); expect(result.cleanedContent).toBe(""); }); + + it("extracts a trailing bash cmd JSON after prose", () => { + const content = 'I will run this.\n{"cmd":["bash","-lc","ls -1a"],"timeout":10000}'; + const result = extractTextualToolCalls(content); + expect(result.toolCalls).toHaveLength(1); + expect(result.toolCalls[0]?.function.name).toBe("terminal"); + expect(result.cleanedContent.trim()).toBe("I will run this."); + }); it("does NOT fire on non-bash cmd JSON", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/textual-tool-calls.test.ts` around lines 275 - 285, Add a test in the textual tool-call extraction suite for prose followed by a bash command JSON object, using extractTextualToolCalls. Assert that the trailing object becomes one terminal tool call with the expected command and timeout, and that cleanedContent retains only the preceding prose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/textual-tool-calls.test.ts`:
- Around line 275-285: Add a test in the textual tool-call extraction suite for
prose followed by a bash command JSON object, using extractTextualToolCalls.
Assert that the trailing object becomes one terminal tool call with the expected
command and timeout, and that cleanedContent retains only the preceding prose.
In `@src/textual-tool-calls.ts`:
- Around line 384-390: Update the parsing logic around built, callFromGptObject,
and callFromBashCmdObject to evaluate each parser once while retaining which
parser produced the result. Use that captured origin when computing isBashCmd,
preserving the existing terminal-name and condition behavior without invoking
callFromBashCmdObject a second time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6d9f7879-0220-4845-9c65-0b879f9c0906
⛔ Files ignored due to path filters (4)
dist/cli.jsis excluded by!**/dist/**dist/cli.js.mapis excluded by!**/dist/**,!**/*.mapdist/index.jsis excluded by!**/dist/**dist/index.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (2)
src/textual-tool-calls.test.tssrc/textual-tool-calls.ts
0xCheetah1
force-pushed
the
fix/kimi-cmd-text-tool-call
branch
from
July 18, 2026 11:58
0063b94 to
801332a
Compare
Contributor
Author
|
Closing for now while testbed validation continues. Will reopen or recreate after the fix is fully tested. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Verification
Summary by CodeRabbit
cmdarrays, extracting the command and preserving an optional timeout.cmdarrays as tool calls.