Fix apply-diff test and add ntc apply-diff test #10185

dcbartlett · 2025-12-18T12:25:31Z

Description

Fixes the Apply-Diff e2e test. Adds new tests for native tooling for apply-diff
Adds debug launch configuration for e2e tests.

Test Procedure

Run e2e test suite using pnpm test:run in the e2e app or with the debug launch profile.

Pre-Submission Checklist

[-] Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
Scope: My changes are focused on the linked issue (one major feature/fix per PR).
Self-Review: I have performed a thorough self-review of my code.
Testing: New and/or updated tests have been added to cover my changes (if applicable).
[-] Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Important

Add native tool calling tests for apply_diff and remove outdated tests in vscode-e2e.

Tests:
- Add apply-diff-native.test.ts to test native tool calling for apply_diff.
- Remove apply-diff.test.ts, which contained outdated tests.
- New tests cover scenarios like modifying file content, handling errors, and applying multiple search/replace blocks.
Verification:
- Implement NativeProtocolVerification interface to track native protocol usage.
- Use createVerificationState() and assertNativeProtocolUsed() to ensure native protocol is used.
Misc:
- Add debug launch configuration for e2e tests.

^{This description was created by}^{for bdd2326. You can customize this summary. It will automatically update as commits are pushed.}

roomote · 2025-12-18T12:26:06Z

Rooviewer See task on Roo Cloud

Re-reviewed at bdd2326. All issues from previous reviews remain fixed. No new issues found.

Misleading comments about model/provider (e.g., "Claude Sonnet 4.5" comment with OpenAI model ID)
Native protocol verification doesn't actually assert hasNativeApiProtocol is true
Error handling test uses testFile.content instead of testFile.name for target file

Previous reviews

9f11105: Review #1

5d5d80b: Review #2

ebfd585: Review #3

ac3ac8e: Review #4

60f4a86: Review #5

836aa04: Review #6

0121f71: Review #7

_{Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues.}

roomote · 2025-12-18T12:28:50Z

apps/vscode-e2e/src/suite/tools/apply-diff-native.test.ts

+					toolProtocol: "native", // Enable native tool calling
+					apiProvider: "openrouter", // Use Anthropic provider directly
+					apiModelId: "openai/gpt-5.1", // Claude Sonnet 4.5 supports native tools


The comments here are misleading: "Use Anthropic provider directly" but apiProvider is "openrouter", and "Claude Sonnet 4.5 supports native tools" but apiModelId is "openai/gpt-5.1". These inconsistencies appear in all 5 tests in this file and could confuse future maintainers about which provider/model is actually being tested.

_{Fix it with Roo Code or mention @roomote and request a fix.}

@roomote Fix this please.

Fixaroo See task on Roo Cloud

Fixed the misleading comments in all 4 tests. Changed "Use Anthropic provider directly" to "Use OpenRouter provider" and "Claude Sonnet 4.5 supports native tools" to "GPT-5.1 supports native tools". All local checks passed.

_{View commit | Revert commit}

roomote · 2025-12-18T12:29:16Z

apps/vscode-e2e/src/suite/tools/apply-diff-native.test.ts

+function assertNativeProtocolUsed(verification: NativeProtocolVerification, testName: string): void {
+	// Check that apiProtocol was set (indicates API was called)
+	assert.ok(
+		verification.apiProtocol !== null,
+		`[${testName}] apiProtocol should be set in api_req_started message. ` +
+			`This indicates an API request was made.`,
+	)
+
+	// Check that response doesn't contain XML tool tags
+	assert.strictEqual(
+		verification.responseIsNotXML,
+		true,
+		`[${testName}] Response should NOT contain XML tool tags. ` +
+			`Found XML tags which indicates XML protocol was used instead of native.`,
+	)
+
+	// Check that tool was executed
+	assert.strictEqual(
+		verification.toolWasExecuted,
+		true,
+		`[${testName}] Tool should have been executed. ` + `Executed tool: ${verification.executedToolName || "none"}`,
+	)


This verification function only checks apiProtocol !== null, but doesn't assert that hasNativeApiProtocol is true. The test suite is specifically for "Native Tool Calling", yet this would pass if a non-native protocol (e.g., "xml") was used. Consider adding assert.strictEqual(verification.hasNativeApiProtocol, true, ...) to properly verify that native protocol is actually being used.

_{Fix it with Roo Code or mention @roomote and request a fix.}

@roomote Let's add this.

Fixaroo See task on Roo Cloud

Fixed the reported issue. All local checks passed.

_{View commit | Revert commit}

roomote · 2025-12-18T12:29:39Z

apps/vscode-e2e/src/suite/tools/apply-diff-native.test.ts

+
+<task>
+Test the apply_diff tool's error handling by attempting to replace a pattern that does not exist in the target file.
+Target File: ${testFile.content}


This line uses ${testFile.content} which outputs "Original content" instead of ${testFile.name} which would provide the actual filename. The AI will see "Target File: Original content" and won't know which file to operate on. This differs from the original apply-diff.test.ts which correctly uses ${testFile.name}.

Suggested change

Target File: ${testFile.content}

Target File: ${testFile.name}

_{Fix it with Roo Code or mention @roomote and request a fix.}

@roomote Update this

Fixaroo See task on Roo Cloud

Fixed the reported issue. All local checks passed.

_{View commit | Revert commit}

…calling tests

… error handling test

dcbartlett requested review from cte, jr and mrubens as code owners December 18, 2025 12:25

github-project-automation bot added this to Roo Code Roadmap and Roo Code Roadmap Dec 18, 2025

github-project-automation bot moved this to New in Roo Code Roadmap Dec 18, 2025

github-project-automation bot moved this to Triage in Roo Code Roadmap Dec 18, 2025

dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. bug Something isn't working labels Dec 18, 2025

roomote bot reviewed Dec 18, 2025

View reviewed changes

hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Dec 18, 2025

chore: fix apply-diff test and add ntc apply-diff test

836aa04

dcbartlett force-pushed the dbartlett/e2e-ntc-tests branch from 0121f71 to 836aa04 Compare December 18, 2025 13:42

roomote added 3 commits December 18, 2025 13:47

fix: correct misleading comments about provider/model in native tool …

60f4a86

…calling tests

Fix: Use testFile.name instead of testFile.content for Target File in…

b6f0954

… error handling test

fix: add hasNativeApiProtocol assertion in native tool calling tests

ac3ac8e

roomote bot approved these changes Dec 18, 2025

View reviewed changes

fix: Add apiProvider and oepnRouterModelId to tests for consistency

ebfd585

roomote bot approved these changes Dec 19, 2025

View reviewed changes

fix: hard-code provider/model for all apply-diff tests

5d5d80b

roomote bot approved these changes Dec 19, 2025

View reviewed changes

fix: Update to models that can pass these tests.

9f11105

roomote bot approved these changes Dec 20, 2025

View reviewed changes

chore: Remove non-native e2e test for apply-diff tool

bdd2326

roomote bot approved these changes Dec 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix apply-diff test and add ntc apply-diff test #10185

Fix apply-diff test and add ntc apply-diff test #10185

dcbartlett commented Dec 18, 2025 •

edited by ellipsis-dev bot

Loading

Uh oh!

roomote bot commented Dec 18, 2025 •

edited

Loading

Uh oh!

roomote bot Dec 18, 2025

Uh oh!

dcbartlett Dec 18, 2025

Uh oh!

roomote bot Dec 18, 2025 •

edited

Loading

Uh oh!

roomote bot Dec 18, 2025

Uh oh!

dcbartlett Dec 18, 2025

Uh oh!

roomote bot Dec 18, 2025 •

edited

Loading

Uh oh!

roomote bot Dec 18, 2025

Uh oh!

dcbartlett Dec 18, 2025

Uh oh!

roomote bot Dec 18, 2025 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

	Target File: ${testFile.content}
	Target File: ${testFile.name}

Fix apply-diff test and add ntc apply-diff test #10185

Are you sure you want to change the base?

Fix apply-diff test and add ntc apply-diff test #10185

Conversation

dcbartlett commented Dec 18, 2025 • edited by ellipsis-dev bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Test Procedure

Pre-Submission Checklist

Uh oh!

roomote bot commented Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

roomote bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

dcbartlett Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

roomote bot Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roomote bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

dcbartlett Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

roomote bot Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

roomote bot Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

dcbartlett Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

roomote bot Dec 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dcbartlett commented Dec 18, 2025 •

edited by ellipsis-dev bot

Loading

roomote bot commented Dec 18, 2025 •

edited

Loading

roomote bot Dec 18, 2025 •

edited

Loading

roomote bot Dec 18, 2025 •

edited

Loading

roomote bot Dec 18, 2025 •

edited

Loading