Skip to content

Validate offsets and lines in create_pull_request_thread parameters #802

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.

Sign up for GitHub

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

Merged
danhellem merged 1 commit into from
Dec 22, 2025
Merged

Validate offsets and lines in create_pull_request_thread parameters #802

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 35 additions & 3 deletions src/tools/repositories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }) => {
Expand Down Expand Up @@ -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) {
Expand All @@ -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,
Expand Down
201 changes: 195 additions & 6 deletions test/src/tools/repositories.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down Expand Up @@ -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);
Expand All @@ -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,
},
Expand Down Expand Up @@ -5470,23 +5661,21 @@ 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,
content: "Test comment",
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 () => {
Expand Down