-
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
Conversation
- Add new response format for implicit rejection with feedback - Update askApproval logic to detect implicit rejections (messageResponse with feedback on file change tools) - Add system prompt rule clarifying file change feedback handling - Add tests for the new implicit rejection response format Fixes #7480
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| "apply_diff", | ||
| "insert_content", | ||
| "search_and_replace", | ||
| ].includes(block.name) |
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.
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?
| ].includes(block.name) | |
| const isImplicitFileChangeRejection = (response: string, text: string | undefined, toolName: string): boolean => { | |
| const fileChangeTools = ["write_to_file", "apply_diff", "insert_content", "search_and_replace"]; | |
| return response === "messageResponse" && !!text && fileChangeTools.includes(toolName); | |
| }; |
| if (text) { | ||
| // Check if this is a file change tool and user provided feedback via messageResponse | ||
| const isFileChangeTool = [ | ||
| "write_to_file", |
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_action that might save files?
| import { describe, it, expect } from "vitest" | ||
| import { formatResponse } from "../responses" | ||
|
|
||
| describe("formatResponse - Implicit Rejection", () => { |
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.
Great test coverage for the main scenarios! Could we add a few edge case tests?
- What happens when feedback is empty/null?
- Behavior with non-file-change tools receiving messageResponse
- An integration test showing the full flow from user feedback to LLM response
| * 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 |
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 explanation is clear, but could benefit from a concrete example of what NOT to do. Something like:
| * The user's feedback is meant to guide you toward a better solution, not to trigger a revert operation | |
| * The user's feedback is meant to guide you toward a better solution, not to trigger a revert operation | |
| * DON'T: attempt to use apply_diff to 'undo' changes that were never applied | |
| * DO: read the file's current state and create a new proposal based on the feedback |
| `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.`, |
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.
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?
|
Not enough info on the issue |
Summary
This PR addresses Issue #7480 by improving how the LLM understands user feedback on proposed file changes. When users provide feedback without explicitly accepting a file change, the system now correctly interprets this as an implicit rejection and guides the LLM to create a new proposal incorporating the feedback.
Problem
Previously, when the LLM proposed a file change and users provided feedback via a new message (without clicking accept/reject), the LLM would:
Solution
This implementation:
Changes
Testing
Impact
This change is backward compatible and only affects the specific scenario where users provide feedback without explicitly accepting/rejecting file changes. It should significantly improve the user experience when iterating on file changes with the LLM.
Fixes #7480
Important
Improves LLM's handling of implicit file change rejections by detecting feedback without explicit acceptance and guiding new proposal creation.
presentAssistantMessage.tswhen users provide feedback without explicit acceptance.toolImplicitlyRejectedWithFeedback()inresponses.tsto format implicit rejection messages.responses.implicitRejection.spec.tsto test implicit rejection message formatting.rules.tsto clarify file change feedback handling.This description was created by
for cf90414. You can customize this summary. It will automatically update as commits are pushed.