-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: improve LLM understanding of implicit file change rejections #7481
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 | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -285,11 +285,27 @@ export async function presentAssistantMessage(cline: Task) { | |||||||||||
| ) | ||||||||||||
|
|
||||||||||||
| if (response !== "yesButtonClicked") { | ||||||||||||
| // Handle both messageResponse and noButtonClicked with text. | ||||||||||||
| if (text) { | ||||||||||||
| // Check if this is a file change tool and user provided feedback via messageResponse | ||||||||||||
| const isFileChangeTool = [ | ||||||||||||
| "write_to_file", | ||||||||||||
| "apply_diff", | ||||||||||||
| "insert_content", | ||||||||||||
| "search_and_replace", | ||||||||||||
| ].includes(block.name) | ||||||||||||
|
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. I notice the implicit rejection detection logic is duplicated here and in the inline approval handling below (lines 288-310). Could we extract this into a shared helper function to avoid duplication and ensure consistency?
Suggested change
|
||||||||||||
| const isImplicitRejection = response === "messageResponse" && text && isFileChangeTool | ||||||||||||
|
|
||||||||||||
| if (isImplicitRejection) { | ||||||||||||
| // User provided feedback without explicitly accepting - treat as implicit rejection with improvement request | ||||||||||||
| await cline.say("user_feedback", text, images) | ||||||||||||
| pushToolResult( | ||||||||||||
| formatResponse.toolResult(formatResponse.toolImplicitlyRejectedWithFeedback(text), images), | ||||||||||||
| ) | ||||||||||||
| } else if (text) { | ||||||||||||
| // Explicit rejection with feedback | ||||||||||||
| await cline.say("user_feedback", text, images) | ||||||||||||
| pushToolResult(formatResponse.toolResult(formatResponse.toolDeniedWithFeedback(text), images)) | ||||||||||||
| } else { | ||||||||||||
| // Explicit rejection without feedback | ||||||||||||
| pushToolResult(formatResponse.toolDenied()) | ||||||||||||
| } | ||||||||||||
| cline.didRejectTool = true | ||||||||||||
|
|
||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { describe, it, expect } from "vitest" | ||
| import { formatResponse } from "../responses" | ||
|
|
||
| describe("formatResponse - Implicit Rejection", () => { | ||
|
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. Great test coverage for the main scenarios! Could we add a few edge case tests?
|
||
| it("should format implicit rejection message correctly", () => { | ||
| const feedback = "Please add error handling to this code" | ||
| const result = formatResponse.toolImplicitlyRejectedWithFeedback(feedback) | ||
|
|
||
| expect(result).toContain("implicitly rejected") | ||
| expect(result).toContain("NOT applied") | ||
| expect(result).toContain(feedback) | ||
| expect(result).toContain("Do not attempt to revert") | ||
| expect(result).toContain("read the current file content") | ||
| expect(result).toContain("create a new proposal") | ||
| }) | ||
|
|
||
| it("should differentiate from explicit rejection message", () => { | ||
| const feedback = "This doesn't look right" | ||
| const implicitResult = formatResponse.toolImplicitlyRejectedWithFeedback(feedback) | ||
| const explicitResult = formatResponse.toolDeniedWithFeedback(feedback) | ||
|
|
||
| expect(implicitResult).not.toBe(explicitResult) | ||
| expect(implicitResult).toContain("implicitly rejected") | ||
| expect(explicitResult).not.toContain("implicitly rejected") | ||
| }) | ||
|
|
||
| it("should differentiate from approval with feedback message", () => { | ||
| const feedback = "Looks good, thanks!" | ||
| const implicitResult = formatResponse.toolImplicitlyRejectedWithFeedback(feedback) | ||
| const approvalResult = formatResponse.toolApprovedWithFeedback(feedback) | ||
|
|
||
| expect(implicitResult).not.toBe(approvalResult) | ||
| expect(implicitResult).toContain("rejected") | ||
| expect(approvalResult).toContain("approved") | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,9 @@ export const formatResponse = { | |
| toolApprovedWithFeedback: (feedback?: string) => | ||
| `The user approved this operation and provided the following context:\n<feedback>\n${feedback}\n</feedback>`, | ||
|
|
||
| toolImplicitlyRejectedWithFeedback: (feedback?: string) => | ||
| `The user implicitly rejected the current file change proposal by providing improvement feedback instead of explicitly accepting it. The proposed change was NOT applied. The user wants you to create a NEW, improved proposal based on their feedback:\n<feedback>\n${feedback}\n</feedback>\n\nIMPORTANT: The previous file change proposal was rejected. Do not attempt to revert any changes. Instead, read the current file content and create a new proposal incorporating the user's feedback.`, | ||
|
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. This implicit rejection message is quite dense. Could we consider breaking it into clearer sections with formatting to help the LLM parse the key points more easily? Perhaps using numbered steps or clearer section headers? |
||
|
|
||
| toolError: (error?: string) => `The tool execution failed with the following error:\n<error>\n${error}\n</error>`, | ||
|
|
||
| rooIgnoreError: (path: string) => | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -69,6 +69,12 @@ RULES | |||||||||
| - All file paths must be relative to this directory. However, commands may change directories in terminals, so respect working directory specified by the response to <execute_command>. | ||||||||||
| - You cannot \`cd\` into a different directory to complete a task. You are stuck operating from '${cwd.toPosix()}', so be sure to pass in the correct 'path' parameter when using tools that require a path. | ||||||||||
| - Do not use the ~ character or $HOME to refer to the home directory. | ||||||||||
| - **CRITICAL: File Change Feedback Handling** - When you propose a file change (using write_to_file, apply_diff, insert_content, or search_and_replace) and the user provides feedback without explicitly accepting it, this means they have IMPLICITLY REJECTED your current proposal. In this case: | ||||||||||
| * The proposed change was NOT applied to the file | ||||||||||
| * You should NOT attempt to "revert" anything (there's nothing to revert since the change wasn't applied) | ||||||||||
| * You MUST read the current file content again to understand its actual state | ||||||||||
| * You should create a NEW proposal that incorporates the user's feedback | ||||||||||
| * The user's feedback is meant to guide you toward a better solution, not to trigger a revert operation | ||||||||||
|
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. The explanation is clear, but could benefit from a concrete example of what NOT to do. Something like:
Suggested change
|
||||||||||
| - Before using the execute_command tool, you must first think about the SYSTEM INFORMATION context provided to understand the user's environment and tailor your commands to ensure they are compatible with their system. You must also consider if the command you need to run should be executed in a specific directory outside of the current working directory '${cwd.toPosix()}', and if so prepend with \`cd\`'ing into that directory && then executing the command (as one command since you are stuck operating from '${cwd.toPosix()}'). For example, if you needed to run \`npm install\` in a project outside of '${cwd.toPosix()}', you would need to prepend with a \`cd\` i.e. pseudocode for this would be \`cd (path to project) && (command, in this case npm install)\`. | ||||||||||
| ${codebaseSearchRule}- When using the search_files tool${isCodebaseSearchAvailable ? " (after codebase_search)" : ""}, craft your regex patterns carefully to balance specificity and flexibility. Based on the user's task you may use it to find code patterns, TODO comments, function definitions, or any text-based information across the project. The results include context, so analyze the surrounding code to better understand the matches. Leverage the search_files tool in combination with other tools for more comprehensive analysis. For example, use it to find specific code patterns, then use read_file to examine the full context of interesting matches before using ${diffStrategy ? "apply_diff or write_to_file" : "write_to_file"} to make informed changes. | ||||||||||
| - When creating a new project (such as an app, website, or any software project), organize all new files within a dedicated project directory unless the user specifies otherwise. Use appropriate file paths when writing files, as the write_to_file tool will automatically create any necessary directories. Structure the project logically, adhering to best practices for the specific type of project being created. Unless otherwise specified, new projects should be easily run without additional setup, for example most projects can be built in HTML, CSS, and JavaScript - which you can open in a browser. | ||||||||||
|
|
||||||||||
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.
The hardcoded array of file change tools appears twice in this file. Would it be better to define this as a constant at the module level or in a configuration file for easier maintenance? Also, should we consider tools like
browser_actionthat might save files?