From 022dc3b7d19aef2d3d217e2f89e0dde517a7119f Mon Sep 17 00:00:00 2001
From: Dan Hellem <dahellem@microsoft.com>
Date: Fri, 19 Dec 2025 18:03:55 +0000
Subject: [PATCH] Validate offsets and lines in create_pull_request_thread
 parameters

---
 src/tools/repositories.ts           |  38 +++++-
 test/src/tools/repositories.test.ts | 201 +++++++++++++++++++++++++++-
 2 files changed, 230 insertions(+), 9 deletions(-)

diff --git a/src/tools/repositories.ts b/src/tools/repositories.ts
index ee1cc7fe..580c66b7 100644
--- a/src/tools/repositories.ts
+++ b/src/tools/repositories.ts
@@ -1033,19 +1033,19 @@ function configureRepoTools(server: McpServer, tokenProvider: () => Promise<stri
         .number()
         .optional()
         .describe(
-          "Position of first character of the thread's span in right file. The line number of a thread's position. The character offset of a thread's position inside of a line. Starts at 1. Must only be set if rightFileStartLine is also specified. (optional)"
+          "Position of first character of the thread's span in right file. The line number of a thread's position. The character offset of a thread's position inside of a line. Starts at 1. Must be set if rightFileStartLine is also specified. (optional)"
         ),
       rightFileEndLine: z
         .number()
         .optional()
         .describe(
-          "Position of last character of the thread's span in right file. The line number of a thread's position. Starts at 1. Must only be set if rightFileStartLine is also specified. (optional)"
+          "Position of last character of the thread's span in right file. The line number of a thread's position. Starts at 1. Must be set if rightFileStartLine is also specified. (optional)"
         ),
       rightFileEndOffset: z
         .number()
         .optional()
         .describe(
-          "Position of last character of the thread's span in right file. The character offset of a thread's position inside of a line. Must only be set if rightFileEndLine is also specified. (optional)"
+          "Position of last character of the thread's span in right file. The character offset of a thread's position inside of a line. Must be set if rightFileEndLine is also specified. (optional)"
         ),
     },
     async ({ repositoryId, pullRequestId, content, project, filePath, status, rightFileStartLine, rightFileStartOffset, rightFileEndLine, rightFileEndOffset }) => {
@@ -1093,6 +1093,13 @@ function configureRepoTools(server: McpServer, tokenProvider: () => Promise<stri
             };
           }
 
+          if (rightFileEndOffset === undefined) {
+            return {
+              content: [{ type: "text", text: "rightFileEndOffset must be specified if rightFileEndLine is specified." }],
+              isError: true,
+            };
+          }
+
           threadContext.rightFileEnd = { line: rightFileEndLine };
 
           if (rightFileEndOffset !== undefined) {
@@ -1107,6 +1114,31 @@ function configureRepoTools(server: McpServer, tokenProvider: () => Promise<stri
           }
         }
 
+        if (rightFileEndOffset !== undefined && rightFileEndLine === undefined) {
+          return {
+            content: [{ type: "text", text: "rightFileEndLine must be specified if rightFileEndOffset is specified." }],
+            isError: true,
+          };
+        }
+
+        if (rightFileStartLine !== undefined && rightFileStartOffset !== undefined) {
+          if (rightFileEndLine === undefined || rightFileEndOffset === undefined) {
+            return {
+              content: [{ type: "text", text: "rightFileEndLine and rightFileEndOffset must both be specified when rightFileStartLine and rightFileStartOffset are both specified." }],
+              isError: true,
+            };
+          }
+        }
+
+        if (rightFileStartLine !== undefined && rightFileEndLine !== undefined && rightFileStartLine === rightFileEndLine) {
+          if (rightFileEndOffset !== undefined && rightFileStartOffset !== undefined && rightFileEndOffset < rightFileStartOffset) {
+            return {
+              content: [{ type: "text", text: "rightFileEndOffset must be greater than or equal to rightFileStartOffset when both are on the same line." }],
+              isError: true,
+            };
+          }
+        }
+
         const thread = await gitApi.createThread(
           { comments: [{ content: content }], threadContext: threadContext, status: CommentThreadStatus[status as keyof typeof CommentThreadStatus] },
           repositoryId,
diff --git a/test/src/tools/repositories.test.ts b/test/src/tools/repositories.test.ts
index bd0c279e..2b9fb76a 100644
--- a/test/src/tools/repositories.test.ts
+++ b/test/src/tools/repositories.test.ts
@@ -4638,6 +4638,194 @@ describe("repos tools", () => {
       });
     });
 
+    it("should reject invalid rightFileStartOffset in create_pull_request_thread", async () => {
+      configureRepoTools(server, tokenProvider, connectionProvider, userAgentProvider);
+
+      const call = (server.tool as jest.Mock).mock.calls.find(([toolName]) => toolName === REPO_TOOLS.create_pull_request_thread);
+      if (!call) throw new Error("repo_create_pull_request_thread tool not registered");
+      const [, , , handler] = call;
+
+      const params = {
+        repositoryId: "repo123",
+        pullRequestId: 456,
+        content: "Test comment",
+        filePath: "/test/file.js",
+        rightFileStartLine: 10,
+        rightFileStartOffset: 0, // Invalid offset (should be >= 1)
+      };
+
+      const result = await handler(params);
+      expect(result).toEqual({
+        content: [{ type: "text", text: "rightFileStartOffset must be greater than or equal to 1." }],
+        isError: true,
+      });
+    });
+
+    it("should reject rightFileEndLine without rightFileStartLine in create_pull_request_thread", async () => {
+      configureRepoTools(server, tokenProvider, connectionProvider, userAgentProvider);
+
+      const call = (server.tool as jest.Mock).mock.calls.find(([toolName]) => toolName === REPO_TOOLS.create_pull_request_thread);
+      if (!call) throw new Error("repo_create_pull_request_thread tool not registered");
+      const [, , , handler] = call;
+
+      const params = {
+        repositoryId: "repo123",
+        pullRequestId: 456,
+        content: "Test comment",
+        filePath: "/test/file.js",
+        rightFileEndLine: 15, // End line without start line
+      };
+
+      const result = await handler(params);
+      expect(result).toEqual({
+        content: [{ type: "text", text: "rightFileEndLine must only be specified if rightFileStartLine is also specified." }],
+        isError: true,
+      });
+    });
+
+    it("should reject invalid rightFileEndLine in create_pull_request_thread", async () => {
+      configureRepoTools(server, tokenProvider, connectionProvider, userAgentProvider);
+
+      const call = (server.tool as jest.Mock).mock.calls.find(([toolName]) => toolName === REPO_TOOLS.create_pull_request_thread);
+      if (!call) throw new Error("repo_create_pull_request_thread tool not registered");
+      const [, , , handler] = call;
+
+      const params = {
+        repositoryId: "repo123",
+        pullRequestId: 456,
+        content: "Test comment",
+        filePath: "/test/file.js",
+        rightFileStartLine: 10,
+        rightFileEndLine: 0, // Invalid end line (should be >= 1)
+        rightFileEndOffset: 5,
+      };
+
+      const result = await handler(params);
+      expect(result).toEqual({
+        content: [{ type: "text", text: "rightFileEndLine must be greater than or equal to 1." }],
+        isError: true,
+      });
+    });
+
+    it("should reject rightFileEndLine without rightFileEndOffset in create_pull_request_thread", async () => {
+      configureRepoTools(server, tokenProvider, connectionProvider, userAgentProvider);
+
+      const call = (server.tool as jest.Mock).mock.calls.find(([toolName]) => toolName === REPO_TOOLS.create_pull_request_thread);
+      if (!call) throw new Error("repo_create_pull_request_thread tool not registered");
+      const [, , , handler] = call;
+
+      const params = {
+        repositoryId: "repo123",
+        pullRequestId: 456,
+        content: "Test comment",
+        filePath: "/test/file.js",
+        rightFileStartLine: 10,
+        rightFileEndLine: 15, // End line without end offset
+      };
+
+      const result = await handler(params);
+      expect(result).toEqual({
+        content: [{ type: "text", text: "rightFileEndOffset must be specified if rightFileEndLine is specified." }],
+        isError: true,
+      });
+    });
+
+    it("should reject invalid rightFileEndOffset in create_pull_request_thread", async () => {
+      configureRepoTools(server, tokenProvider, connectionProvider, userAgentProvider);
+
+      const call = (server.tool as jest.Mock).mock.calls.find(([toolName]) => toolName === REPO_TOOLS.create_pull_request_thread);
+      if (!call) throw new Error("repo_create_pull_request_thread tool not registered");
+      const [, , , handler] = call;
+
+      const params = {
+        repositoryId: "repo123",
+        pullRequestId: 456,
+        content: "Test comment",
+        filePath: "/test/file.js",
+        rightFileStartLine: 10,
+        rightFileEndLine: 15,
+        rightFileEndOffset: 0, // Invalid offset (should be >= 1)
+      };
+
+      const result = await handler(params);
+      expect(result).toEqual({
+        content: [{ type: "text", text: "rightFileEndOffset must be greater than or equal to 1." }],
+        isError: true,
+      });
+    });
+
+    it("should reject rightFileEndOffset without rightFileEndLine in create_pull_request_thread", async () => {
+      configureRepoTools(server, tokenProvider, connectionProvider, userAgentProvider);
+
+      const call = (server.tool as jest.Mock).mock.calls.find(([toolName]) => toolName === REPO_TOOLS.create_pull_request_thread);
+      if (!call) throw new Error("repo_create_pull_request_thread tool not registered");
+      const [, , , handler] = call;
+
+      const params = {
+        repositoryId: "repo123",
+        pullRequestId: 456,
+        content: "Test comment",
+        filePath: "/test/file.js",
+        rightFileStartLine: 10,
+        rightFileEndOffset: 5, // End offset without end line
+      };
+
+      const result = await handler(params);
+      expect(result).toEqual({
+        content: [{ type: "text", text: "rightFileEndLine must be specified if rightFileEndOffset is specified." }],
+        isError: true,
+      });
+    });
+
+    it("should require both rightFileEndLine and rightFileEndOffset when rightFileStartLine and rightFileStartOffset are specified", async () => {
+      configureRepoTools(server, tokenProvider, connectionProvider, userAgentProvider);
+
+      const call = (server.tool as jest.Mock).mock.calls.find(([toolName]) => toolName === REPO_TOOLS.create_pull_request_thread);
+      if (!call) throw new Error("repo_create_pull_request_thread tool not registered");
+      const [, , , handler] = call;
+
+      const params = {
+        repositoryId: "repo123",
+        pullRequestId: 456,
+        content: "Test comment",
+        filePath: "/test/file.js",
+        rightFileStartLine: 10,
+        rightFileStartOffset: 5,
+        // Missing rightFileEndLine and rightFileEndOffset
+      };
+
+      const result = await handler(params);
+      expect(result).toEqual({
+        content: [{ type: "text", text: "rightFileEndLine and rightFileEndOffset must both be specified when rightFileStartLine and rightFileStartOffset are both specified." }],
+        isError: true,
+      });
+    });
+
+    it("should reject rightFileEndOffset less than rightFileStartOffset on same line in create_pull_request_thread", async () => {
+      configureRepoTools(server, tokenProvider, connectionProvider, userAgentProvider);
+
+      const call = (server.tool as jest.Mock).mock.calls.find(([toolName]) => toolName === REPO_TOOLS.create_pull_request_thread);
+      if (!call) throw new Error("repo_create_pull_request_thread tool not registered");
+      const [, , , handler] = call;
+
+      const params = {
+        repositoryId: "repo123",
+        pullRequestId: 456,
+        content: "Test comment",
+        filePath: "/test/file.js",
+        rightFileStartLine: 10,
+        rightFileStartOffset: 20,
+        rightFileEndLine: 10, // Same line
+        rightFileEndOffset: 5, // End offset less than start offset
+      };
+
+      const result = await handler(params);
+      expect(result).toEqual({
+        content: [{ type: "text", text: "rightFileEndOffset must be greater than or equal to rightFileStartOffset when both are on the same line." }],
+        isError: true,
+      });
+    });
+
     it("should handle create_pull_request with undefined forkSourceRepositoryId", async () => {
       configureRepoTools(server, tokenProvider, connectionProvider, userAgentProvider);
 
@@ -4989,6 +5177,8 @@ describe("repos tools", () => {
         status: "Active", // Provide explicit status
         rightFileStartLine: 5,
         rightFileStartOffset: 10, // Valid offset
+        rightFileEndLine: 5, // Must specify both end line and offset when start offset is specified
+        rightFileEndOffset: 20,
       };
 
       const result = await handler(params);
@@ -4999,6 +5189,7 @@ describe("repos tools", () => {
           threadContext: {
             filePath: "/test/file.js",
             rightFileStart: { line: 5, offset: 10 },
+            rightFileEnd: { line: 5, offset: 20 },
           },
           status: CommentThreadStatus.Active,
         },
@@ -5470,9 +5661,6 @@ describe("repos tools", () => {
       if (!call) throw new Error("repo_create_pull_request_thread tool not registered");
       const [, , , handler] = call;
 
-      const mockThread = { id: 1, status: CommentThreadStatus.Active };
-      mockGitApi.createThread.mockResolvedValue(mockThread);
-
       const params = {
         repositoryId: "repo123",
         pullRequestId: 456,
@@ -5480,13 +5668,14 @@ describe("repos tools", () => {
         filePath: "/test/file.js",
         rightFileStartLine: 5,
         rightFileStartOffset: 10,
-        rightFileEndOffset: 20, // End offset without end line - should still work
+        rightFileEndOffset: 20, // End offset without end line - should trigger error
       };
 
       const result = await handler(params);
 
-      expect(mockGitApi.createThread).toHaveBeenCalled();
-      expect(result.content[0].text).toBe(JSON.stringify(mockThread, null, 2));
+      // Should return an error because rightFileEndLine must be specified when rightFileEndOffset is specified
+      expect(result.isError).toBe(true);
+      expect(result.content[0].text).toBe("rightFileEndLine must be specified if rightFileEndOffset is specified.");
     });
 
     it("should handle error in list_pull_requests_by_commits", async () => {