Add tool mcp_ado_repo_get_pull_request_changes#801
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #801 +/- ##
=======================================
Coverage ? 91.27%
=======================================
Files ? 17
Lines ? 1834
Branches ? 413
=======================================
Hits ? 1674
Misses ? 56
Partials ? 104
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
danhellem
added
Do Not Merge ❌
|
@posidron this PR has been in draft for over 3 weeks. If we do not see any progress by 1/13, we will go ahead and close the PR. Hope to hear from you soon. Also please follow guidance as documented in Contributions file. We request an Issue be created first for approval before a PR will be considered. |
|
Closing this draft due to no response from author |
|
@danhellem I created the issue here: #868 |
|
@posidron I think we agree that this is a good idea. @nikolapeja6 on our team is going to spend some time on this reviewing your PR and testing to make sure the tool works at scale with large PRs. It will be a week or two before have any results. Stand by |
|
|
||
| **Notes**: | ||
|
|
||
| - If `iterationId` is not specified, returns changes for the latest iteration |
There was a problem hiding this comment.
The way how I use it is in combination with https://dev.azure.com/microsoft/Edge/_git/chromium.src/pullrequest/14659824 - in VSCode you would then write /create-security-review <ADO_PR_URL>.
There was a problem hiding this comment.
Pull request overview
Adds a new repositories tool for retrieving pull request iteration changes, including optional enriched diff metadata and line-level content, and documents/tests the new capability.
Changes:
- Introduces
repo_get_pull_request_changesinsrc/tools/repositories.ts, with options for iteration selection, pagination, diff enrichment, and line-content enrichment. - Adds extensive Jest coverage for the new tool’s behavior (iteration resolution, pagination, diff/line-content paths, and error handling).
- Updates
docs/TOOLSET.mdto list and describe the new tool.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/tools/repositories.ts |
Adds the new PR changes tool and supporting stream-to-string + diff/line-content enrichment logic. |
test/src/tools/repositories.test.ts |
Adds tests validating registration and behavior of the new tool, including diff and line-content enrichment scenarios. |
docs/TOOLSET.md |
Documents the new tool, parameters, and behavioral notes. |
Comments suppressed due to low confidence (2)
src/tools/repositories.ts:1148
includeLineContentenrichment readslineDiffBlocksfields namedoriginalLineNumberStart/originalLinesCountandmodifiedLineNumberStart/modifiedLinesCount, but other added tests in this PR useoriginalLineStart/originalLineCountandmodifiedLineStart/modifiedLineCount. If the actualgetFileDiffsresponse uses the latter shape, this code will never attachoriginalLines/modifiedLines. Consider supporting both field name variants (or switch to the correct one per the SDK type) and make the tests consistent.
// Add original (base) lines if they exist
if (block.originalLineNumberStart && block.originalLinesCount) {
const startIdx = block.originalLineNumberStart - 1;
const endIdx = startIdx + block.originalLinesCount;
enrichedBlock.originalLines = baseLines.slice(startIdx, endIdx);
}
// Add modified (target) lines if they exist
if (block.modifiedLineNumberStart && block.modifiedLinesCount) {
const startIdx = block.modifiedLineNumberStart - 1;
const endIdx = startIdx + block.modifiedLinesCount;
enrichedBlock.modifiedLines = targetLines.slice(startIdx, endIdx);
test/src/tools/repositories.test.ts:47
- The
mockGitApitype definition is missing methods that are added to the mock object (getPullRequestIteration,getFileDiffs,getItemText). With TypeScript's excess property checks, assigning the object literal with these extra keys will fail compilation. Extend themockGitApitype to include these methods (or loosen the typing, e.g., with a mapped type/Partial, or cast the object literal).
let mockGitApi: {
updatePullRequest: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
createPullRequest: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
createPullRequestReviewers: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
deletePullRequestReviewer: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getRepositories: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getPullRequests: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getPullRequestsByProject: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getThreads: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getComments: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getRefs: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getPullRequest: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getPullRequestLabels: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
createPullRequestLabel: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
deletePullRequestLabels: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
createComment: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
createThread: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
updateThread: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getCommits: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getPullRequestQuery: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
updateRefs: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getPullRequestIterationChanges: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
getPullRequestIterations: jest.MockedFunction<(...args: unknown[]) => Promise<unknown>>;
};
| // If no iteration ID provided, get the latest iteration | ||
| let targetIterationId = iterationId; | ||
| let targetIteration; | ||
| if (!targetIterationId) { |
There was a problem hiding this comment.
iterationId is treated as “not provided” via a falsy check (if (!targetIterationId)), which will also treat 0 as missing and fetch the latest iteration instead. Use a null/undefined check (e.g., iterationId == null) to distinguish an omitted value from 0.
| if (!targetIterationId) { | |
| if (targetIterationId == null) { |
| // Exclude added (1) and deleted (16) files as they don't have both versions to diff | ||
| const fileDiffParams = changes.changeEntries | ||
| .filter((entry) => entry.item?.path && entry.changeType !== 1 && entry.changeType !== 16) // Only modified files |
There was a problem hiding this comment.
The change-type filtering for fileDiffParams assumes numeric changeType values (excluding 1 and 16), but the added tests in this PR use string values like "add"/"edit" for changeType. If the API (or mocks) provide strings, added/deleted files won’t be excluded and the behavior won’t match the comment. Normalize changeType to the expected enum representation (or handle both string and numeric forms) and align tests/mocks accordingly.
This issue also appears on line 1137 of the same file.
| // Exclude added (1) and deleted (16) files as they don't have both versions to diff | |
| const fileDiffParams = changes.changeEntries | |
| .filter((entry) => entry.item?.path && entry.changeType !== 1 && entry.changeType !== 16) // Only modified files | |
| // Exclude added (1 / "add") and deleted (16 / "delete") files as they don't have both versions to diff | |
| const fileDiffParams = changes.changeEntries | |
| .filter((entry) => { | |
| if (!entry.item?.path) { | |
| return false; | |
| } | |
| const changeType = entry.changeType as unknown; | |
| if (typeof changeType === "number") { | |
| // Numeric enum values: 1 = add, 16 = delete | |
| return changeType !== 1 && changeType !== 16; | |
| } | |
| if (typeof changeType === "string") { | |
| const normalized = changeType.toLowerCase(); | |
| // String representations: exclude adds/deletes, allow edits/other modifications | |
| return normalized !== "add" && normalized !== "delete"; | |
| } | |
| // If changeType is an unexpected type, be conservative and include it | |
| return true; | |
| }) // Only modified files |
| // Check if response is an error (Azure DevOps returns JSON error in stream) | ||
| if (baseText.startsWith("{") && baseText.includes("innerException")) { | ||
| throw new Error(`Failed to fetch base file content: ${baseText}`); | ||
| } | ||
| if (targetText.startsWith("{") && targetText.includes("innerException")) { | ||
| throw new Error(`Failed to fetch target file content: ${targetText}`); | ||
| } |
There was a problem hiding this comment.
The “Azure DevOps error JSON” detection treats any file content that starts with { and contains innerException as an error. This can misclassify legitimate JSON/code files containing that substring. A more robust approach would be to JSON.parse and check for expected error keys (e.g., message plus a known schema), or rely on HTTP status/SDK errors instead of content heuristics.
| const chunks: Buffer[] = []; | ||
| return new Promise((resolve, reject) => { | ||
| stream.on("data", (chunk: Buffer) => chunks.push(chunk)); | ||
| stream.on("error", reject); | ||
| stream.on("end", () => resolve(Buffer.concat(chunks).toString("utf8"))); |
There was a problem hiding this comment.
streamToString is now duplicated (there is also a streamToString helper in src/tools/wiki.ts). Consider extracting a shared utility (e.g., in src/utils.ts) to avoid divergent implementations (buffer-based vs setEncoding) and make stream handling consistent across tools.
| const chunks: Buffer[] = []; | |
| return new Promise((resolve, reject) => { | |
| stream.on("data", (chunk: Buffer) => chunks.push(chunk)); | |
| stream.on("error", reject); | |
| stream.on("end", () => resolve(Buffer.concat(chunks).toString("utf8"))); | |
| return new Promise((resolve, reject) => { | |
| let data = ""; | |
| stream.setEncoding("utf8"); | |
| stream.on("data", (chunk: string) => { | |
| data += chunk; | |
| }); | |
| stream.on("error", reject); | |
| stream.on("end", () => resolve(data)); |
|
Lets please get the conflicts resolved and comments addressed. If not activity before 3/23, we will close this PR |
|
@danhellem I got side tracked, I can look at the merge conflicts tomorrow. Can you please not close it. |
…epo_get_pull_request_changes # Conflicts: # src/tools/repositories.ts # test/src/tools/repositories.test.ts
Replace by description of the work done
GitHub issue number
Associated Risks
Replace by possible risks this pull request can bring you might have thought of
✅ PR Checklist
🧪 How did you test it?
Replace with use cases tested and models used