-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: enhance tool use instructions for Ollama models (#8430) #8431
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| import { describe, it, expect } from "vitest" | ||
| import { getSharedToolUseSection } from "../tool-use" | ||
|
|
||
| describe("getSharedToolUseSection", () => { | ||
| describe("base functionality", () => { | ||
| it("should return base tool use section when no apiProvider is provided", () => { | ||
| const result = getSharedToolUseSection() | ||
|
|
||
| expect(result).toContain("TOOL USE") | ||
| expect(result).toContain("Tool uses are formatted using XML-style tags") | ||
| expect(result).toContain("<actual_tool_name>") | ||
| expect(result).not.toContain("CRITICAL: Tool Use Requirements") | ||
| expect(result).not.toContain("MANDATORY") | ||
| }) | ||
|
|
||
| it("should return base tool use section for non-local providers", () => { | ||
| const providers = ["anthropic", "openai", "openrouter", "bedrock"] | ||
|
|
||
| providers.forEach((provider) => { | ||
| const result = getSharedToolUseSection(provider) | ||
|
|
||
| expect(result).toContain("TOOL USE") | ||
| expect(result).toContain("Tool uses are formatted using XML-style tags") | ||
| expect(result).not.toContain("CRITICAL: Tool Use Requirements") | ||
| expect(result).not.toContain("MANDATORY") | ||
| }) | ||
| }) | ||
| }) | ||
|
|
||
| describe("local model enhancements", () => { | ||
| it("should include enhanced instructions for ollama provider", () => { | ||
| const result = getSharedToolUseSection("ollama") | ||
|
|
||
| // Check base content is still there | ||
| expect(result).toContain("TOOL USE") | ||
| expect(result).toContain("Tool uses are formatted using XML-style tags") | ||
|
|
||
| // Check enhanced instructions | ||
| expect(result).toContain("CRITICAL: Tool Use Requirements for Your Response") | ||
| expect(result).toContain("MANDATORY") | ||
| expect(result).toContain("Every response MUST contain EXACTLY ONE tool use") | ||
| expect(result).toContain("DO NOT") | ||
| expect(result).toContain("Write explanations or text outside of the tool XML tags") | ||
| expect(result).toContain("Guess file locations or code content") | ||
| expect(result).toContain("ALWAYS") | ||
| expect(result).toContain("Start with codebase_search tool when exploring code") | ||
|
|
||
| // Check examples | ||
| expect(result).toContain("Example of a CORRECT response") | ||
| expect(result).toContain("<codebase_search>") | ||
| expect(result).toContain("<query>main function entry point</query>") | ||
| expect(result).toContain("</codebase_search>") | ||
|
|
||
| expect(result).toContain("Example of an INCORRECT response") | ||
| expect(result).toContain("I'll search for the main function") | ||
|
|
||
| // Check final reminder | ||
| expect(result).toContain("Remember: Your ENTIRE response should be the tool XML, nothing else") | ||
| }) | ||
|
|
||
| it("should include enhanced instructions for lmstudio provider", () => { | ||
| const result = getSharedToolUseSection("lmstudio") | ||
|
|
||
| // Check base content is still there | ||
| expect(result).toContain("TOOL USE") | ||
| expect(result).toContain("Tool uses are formatted using XML-style tags") | ||
|
|
||
| // Check enhanced instructions (same as ollama) | ||
| expect(result).toContain("CRITICAL: Tool Use Requirements for Your Response") | ||
| expect(result).toContain("MANDATORY") | ||
| expect(result).toContain("Every response MUST contain EXACTLY ONE tool use") | ||
| expect(result).toContain("DO NOT") | ||
| expect(result).toContain("ALWAYS") | ||
| expect(result).toContain("Example of a CORRECT response") | ||
| expect(result).toContain("Example of an INCORRECT response") | ||
| expect(result).toContain("Remember: Your ENTIRE response should be the tool XML, nothing else") | ||
| }) | ||
| }) | ||
|
|
||
| describe("formatting and structure", () => { | ||
| it("should maintain proper formatting with line breaks", () => { | ||
| const result = getSharedToolUseSection("ollama") | ||
|
|
||
| // Check that there are proper line breaks between sections | ||
| expect(result).toMatch(/TOOL USE\n\n/) | ||
| expect(result).toMatch(/# Tool Use Formatting\n\n/) | ||
| expect(result).toMatch(/# CRITICAL: Tool Use Requirements/) | ||
| }) | ||
|
|
||
| it("should have consistent XML examples", () => { | ||
| const result = getSharedToolUseSection("ollama") | ||
|
|
||
| // Check XML structure is properly formatted | ||
| expect(result).toMatch(/<actual_tool_name>\n<parameter1_name>value1<\/parameter1_name>/) | ||
| expect(result).toMatch(/<codebase_search>\n<query>/) | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,5 +1,8 @@ | ||||||||||||
| export function getSharedToolUseSection(): string { | ||||||||||||
| return `==== | ||||||||||||
| export function getSharedToolUseSection(apiProvider?: string): string { | ||||||||||||
| // Enhanced instructions for local models that may struggle with tool formatting | ||||||||||||
| const isLocalModel = apiProvider === "ollama" || apiProvider === "lmstudio" | ||||||||||||
|
Comment on lines
+2
to
+3
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P1: Normalize provider for robustness and accept common alias. This avoids missing enhanced instructions when the provider value differs in case or includes a hyphen (e.g., 'LMStudio', 'lm-studio'). Suggested minimal change below:
Suggested change
|
||||||||||||
|
|
||||||||||||
| const baseSection = `==== | ||||||||||||
|
|
||||||||||||
| TOOL USE | ||||||||||||
|
|
||||||||||||
|
|
@@ -16,4 +19,33 @@ Tool uses are formatted using XML-style tags. The tool name itself becomes the X | |||||||||||
| </actual_tool_name> | ||||||||||||
|
|
||||||||||||
| Always use the actual tool name as the XML tag name for proper parsing and execution.` | ||||||||||||
|
|
||||||||||||
| if (isLocalModel) { | ||||||||||||
| return ( | ||||||||||||
| baseSection + | ||||||||||||
| ` | ||||||||||||
|
|
||||||||||||
| # CRITICAL: Tool Use Requirements for Your Response | ||||||||||||
|
|
||||||||||||
| **MANDATORY**: Every response MUST contain EXACTLY ONE tool use in the XML format shown above. | ||||||||||||
| **DO NOT**: Write explanations or text outside of the tool XML tags. | ||||||||||||
| **DO NOT**: Guess file locations or code content - use the appropriate search tools first. | ||||||||||||
| **ALWAYS**: Start with codebase_search tool when exploring code for the first time. | ||||||||||||
|
|
||||||||||||
| Example of a CORRECT response (using codebase_search): | ||||||||||||
| <codebase_search> | ||||||||||||
| <query>main function entry point</query> | ||||||||||||
| </codebase_search> | ||||||||||||
|
|
||||||||||||
| Example of an INCORRECT response (this will fail): | ||||||||||||
| I'll search for the main function in your codebase. | ||||||||||||
| <codebase_search> | ||||||||||||
| <query>main function</query> | ||||||||||||
| </codebase_search> | ||||||||||||
|
|
||||||||||||
| Remember: Your ENTIRE response should be the tool XML, nothing else.` | ||||||||||||
| ) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return baseSection | ||||||||||||
| } | ||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -89,6 +89,9 @@ export const generateSystemPrompt = async (provider: ClineProvider, message: Web | |
| .getConfiguration("roo-cline") | ||
| .get<boolean>("newTaskRequireTodos", false), | ||
| }, | ||
| undefined, // todoList | ||
| undefined, // modelId | ||
| apiConfiguration?.apiProvider, | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. P3: Optional: If provider normalization is handled in src/core/prompts/sections/tool-use.ts, no action needed here. Otherwise, consider normalizing |
||
| ) | ||
|
|
||
| return systemPrompt | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P2: Consider adding test coverage for provider case/alias normalization (e.g., 'LMStudio' and 'lm-studio') to prevent regressions. Example to add near the existing lmstudio test: