-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: exclude browser scroll actions from repetition detection #7471
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 |
|---|---|---|
|
|
@@ -32,6 +32,13 @@ export class ToolRepetitionDetector { | |
| messageDetail: string | ||
| } | ||
| } { | ||
| // Browser scroll actions should not be subject to repetition detection | ||
| // as they are frequently needed for navigating through web pages | ||
| if (this.isBrowserScrollAction(currentToolCallBlock)) { | ||
| // Allow browser scroll actions without counting them as repetitions | ||
| return { allowExecution: true } | ||
| } | ||
|
|
||
| // Serialize the block to a canonical JSON string for comparison | ||
| const currentToolCallJson = this.serializeToolUse(currentToolCallBlock) | ||
|
|
||
|
|
@@ -66,6 +73,21 @@ export class ToolRepetitionDetector { | |
| return { allowExecution: true } | ||
| } | ||
|
|
||
| /** | ||
| * Checks if a tool use is a browser scroll action | ||
| * | ||
| * @param toolUse The ToolUse object to check | ||
| * @returns true if the tool is a browser_action with scroll_down or scroll_up action | ||
| */ | ||
| private isBrowserScrollAction(toolUse: ToolUse): boolean { | ||
|
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 early return pattern here is good, but could we optimize further? Since most tools won't be browser_action, we're doing the string comparison for every non-browser tool. Would it be worth checking the action parameter first since it's already in memory? Actually, wait... I just realized I wrote this exact code. Never mind, the current implementation is already optimal. 🤦 |
||
| if (toolUse.name !== "browser_action") { | ||
| return false | ||
| } | ||
|
|
||
| const action = toolUse.params.action as string | ||
|
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 cast to string here could potentially be undefined. While the code handles this gracefully (undefined won't match our strings), would it be cleaner to explicitly check? |
||
| return action === "scroll_down" || action === "scroll_up" | ||
|
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. Consider extracting these hardcoded strings as constants for better maintainability? This would make it easier to add new scroll-related actions in the future if needed. |
||
| } | ||
|
|
||
| /** | ||
| * Serializes a ToolUse object into a canonical JSON string for comparison | ||
| * | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -402,4 +402,164 @@ describe("ToolRepetitionDetector", () => { | |
| } | ||
| }) | ||
| }) | ||
|
|
||
| // ===== Browser Scroll Action Exclusion tests ===== | ||
| describe("browser scroll action exclusion", () => { | ||
| it("should not count browser scroll_down actions as repetitions", () => { | ||
| const detector = new ToolRepetitionDetector(2) | ||
|
|
||
| // Create browser_action tool use with scroll_down | ||
| const scrollDownTool: ToolUse = { | ||
| type: "tool_use", | ||
| name: "browser_action" as ToolName, | ||
| params: { action: "scroll_down" }, | ||
| partial: false, | ||
| } | ||
|
|
||
| // Should allow unlimited scroll_down actions | ||
| for (let i = 0; i < 10; i++) { | ||
| const result = detector.check(scrollDownTool) | ||
| expect(result.allowExecution).toBe(true) | ||
| expect(result.askUser).toBeUndefined() | ||
| } | ||
| }) | ||
|
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! Consider adding a test case for case sensitivity - what happens with "SCROLL_DOWN" or "Scroll_Down"? This would document whether case sensitivity is intentional. |
||
|
|
||
| it("should not count browser scroll_up actions as repetitions", () => { | ||
| const detector = new ToolRepetitionDetector(2) | ||
|
|
||
| // Create browser_action tool use with scroll_up | ||
| const scrollUpTool: ToolUse = { | ||
| type: "tool_use", | ||
| name: "browser_action" as ToolName, | ||
| params: { action: "scroll_up" }, | ||
| partial: false, | ||
| } | ||
|
|
||
| // Should allow unlimited scroll_up actions | ||
| for (let i = 0; i < 10; i++) { | ||
| const result = detector.check(scrollUpTool) | ||
| expect(result.allowExecution).toBe(true) | ||
| expect(result.askUser).toBeUndefined() | ||
| } | ||
| }) | ||
|
|
||
| it("should not count alternating scroll_down and scroll_up as repetitions", () => { | ||
| const detector = new ToolRepetitionDetector(2) | ||
|
|
||
| const scrollDownTool: ToolUse = { | ||
| type: "tool_use", | ||
| name: "browser_action" as ToolName, | ||
| params: { action: "scroll_down" }, | ||
| partial: false, | ||
| } | ||
|
|
||
| const scrollUpTool: ToolUse = { | ||
| type: "tool_use", | ||
| name: "browser_action" as ToolName, | ||
| params: { action: "scroll_up" }, | ||
| partial: false, | ||
| } | ||
|
|
||
| // Alternate between scroll_down and scroll_up | ||
| for (let i = 0; i < 5; i++) { | ||
| let result = detector.check(scrollDownTool) | ||
| expect(result.allowExecution).toBe(true) | ||
| expect(result.askUser).toBeUndefined() | ||
|
|
||
| result = detector.check(scrollUpTool) | ||
| expect(result.allowExecution).toBe(true) | ||
| expect(result.askUser).toBeUndefined() | ||
| } | ||
| }) | ||
|
|
||
| it("should still apply repetition detection to other browser_action types", () => { | ||
| const detector = new ToolRepetitionDetector(2) | ||
|
|
||
| // Create browser_action tool use with click action | ||
| const clickTool: ToolUse = { | ||
| type: "tool_use", | ||
| name: "browser_action" as ToolName, | ||
| params: { action: "click", coordinate: "[100, 200]" }, | ||
| partial: false, | ||
| } | ||
|
|
||
| // First call allowed | ||
| expect(detector.check(clickTool).allowExecution).toBe(true) | ||
|
|
||
| // Second call allowed | ||
| expect(detector.check(clickTool).allowExecution).toBe(true) | ||
|
|
||
| // Third identical call should be blocked (limit is 2) | ||
| const result = detector.check(clickTool) | ||
| expect(result.allowExecution).toBe(false) | ||
| expect(result.askUser).toBeDefined() | ||
| }) | ||
|
|
||
| it("should still apply repetition detection to non-browser tools", () => { | ||
| const detector = new ToolRepetitionDetector(2) | ||
|
|
||
| const readFileTool = createToolUse("read_file", "read_file", { path: "test.txt" }) | ||
|
|
||
| // First call allowed | ||
| expect(detector.check(readFileTool).allowExecution).toBe(true) | ||
|
|
||
| // Second call allowed | ||
| expect(detector.check(readFileTool).allowExecution).toBe(true) | ||
|
|
||
| // Third identical call should be blocked (limit is 2) | ||
| const result = detector.check(readFileTool) | ||
| expect(result.allowExecution).toBe(false) | ||
| expect(result.askUser).toBeDefined() | ||
| }) | ||
|
|
||
| it("should not interfere with repetition detection of other tools when scroll actions are interspersed", () => { | ||
| const detector = new ToolRepetitionDetector(2) | ||
|
|
||
| const scrollTool: ToolUse = { | ||
| type: "tool_use", | ||
| name: "browser_action" as ToolName, | ||
| params: { action: "scroll_down" }, | ||
| partial: false, | ||
| } | ||
|
|
||
| const otherTool = createToolUse("execute_command", "execute_command", { command: "ls" }) | ||
|
|
||
| // First execute_command | ||
| expect(detector.check(otherTool).allowExecution).toBe(true) | ||
|
|
||
| // Scroll actions in between (should not affect counter) | ||
| expect(detector.check(scrollTool).allowExecution).toBe(true) | ||
| expect(detector.check(scrollTool).allowExecution).toBe(true) | ||
|
|
||
| // Second execute_command | ||
| expect(detector.check(otherTool).allowExecution).toBe(true) | ||
|
|
||
| // More scroll actions | ||
| expect(detector.check(scrollTool).allowExecution).toBe(true) | ||
|
|
||
| // Third execute_command should be blocked | ||
| const result = detector.check(otherTool) | ||
| expect(result.allowExecution).toBe(false) | ||
| expect(result.askUser).toBeDefined() | ||
| }) | ||
|
|
||
| it("should handle browser_action with missing or invalid action parameter gracefully", () => { | ||
| const detector = new ToolRepetitionDetector(2) | ||
|
|
||
| // Browser action without action parameter | ||
| const noActionTool: ToolUse = { | ||
| type: "tool_use", | ||
| name: "browser_action" as ToolName, | ||
| params: {}, | ||
| partial: false, | ||
| } | ||
|
|
||
| // Should apply normal repetition detection | ||
| expect(detector.check(noActionTool).allowExecution).toBe(true) | ||
| expect(detector.check(noActionTool).allowExecution).toBe(true) | ||
| const result = detector.check(noActionTool) | ||
| expect(result.allowExecution).toBe(false) | ||
| expect(result.askUser).toBeDefined() | ||
| }) | ||
| }) | ||
| }) | ||
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.
Is it intentional that scroll actions don't update ? This means the sequence: scroll → non-scroll → same non-scroll won't compare the two non-scroll calls against each other.
This behavior seems correct (we want to preserve the previous non-scroll state), but it might be worth adding a comment to clarify this design decision for future maintainers.