-
Notifications
You must be signed in to change notification settings - Fork 418
Analyze: update_pull_request with empty args silently drops safe outputs #33134
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 3 commits
41ed207
6dafb93
d00aae7
d58eb03
8c4c500
815d328
783555d
dd679de
79c65cc
0db0368
c25ece2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1359,6 +1359,33 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| }; | ||
| }; | ||
|
|
||
| /** | ||
| * Handler for update_pull_request tool | ||
| * Spec cross-reference: Safe Output Outcome Evaluation §update_pull_request. | ||
| * Per Safe Outputs Specification MCE1: Enforces constraints during tool invocation | ||
| * to provide immediate feedback to the LLM before recording to NDJSON. | ||
| * Validates that at least one of 'title', 'body', or 'update_branch' is provided, | ||
| * matching the server-side requiresOneOf:title,body,update_branch validation. | ||
| */ | ||
| const updatePullRequestHandler = args => { | ||
| const safeArgs = args || {}; | ||
| const hasTitle = safeArgs.title !== undefined; | ||
| const hasBody = safeArgs.body !== undefined; | ||
| // update_branch: false is treated as not provided because it carries no update intent | ||
| // (it's the default behaviour). This mirrors the downstream requiresOneOf validator in | ||
| // safe_output_type_validator.cjs which also excludes field === false from the count. | ||
| const hasUpdateBranch = safeArgs.update_branch !== undefined && safeArgs.update_branch !== false; | ||
|
|
||
|
|
||
| if (!hasTitle && !hasBody && !hasUpdateBranch) { | ||
| throw { | ||
| code: -32602, | ||
| message: `${ERR_VALIDATION}: update_pull_request requires at least one of: 'title', 'body', 'update_branch' fields`, | ||
|
Contributor
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. [/diagnose] The Consider extracting a tiny shared predicate, or at minimum adding a cross-reference comment pointing to the exact line in |
||
| }; | ||
| } | ||
|
|
||
| return defaultHandler("update_pull_request")(safeArgs); | ||
|
Contributor
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. [/diagnose] The Is |
||
| }; | ||
|
|
||
| return { | ||
| defaultHandler, | ||
| uploadAssetHandler, | ||
|
|
@@ -1371,6 +1398,7 @@ function createHandlers(server, appendSafeOutput, config = {}) { | |
| addCommentHandler, | ||
| createPullRequestReviewCommentHandler, | ||
| submitPullRequestReviewHandler, | ||
| updatePullRequestHandler, | ||
| }; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1764,4 +1764,81 @@ describe("safe_outputs_handlers", () => { | |
| expect(() => handlers.submitPullRequestReviewHandler({ event: "COMMENT" })).toThrow(expect.objectContaining({ code: -32602, message: expect.stringContaining("review body is empty") })); | ||
| }); | ||
| }); | ||
|
|
||
| describe("updatePullRequestHandler", () => { | ||
| it("should throw MCP error when no fields are provided", () => { | ||
| expect(() => handlers.updatePullRequestHandler({})).toThrow( | ||
| expect.objectContaining({ | ||
| code: -32602, | ||
| message: expect.stringContaining("requires at least one of"), | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it("should throw MCP error when called with null/undefined args", () => { | ||
| expect(() => handlers.updatePullRequestHandler(null)).toThrow( | ||
| expect.objectContaining({ code: -32602 }) | ||
| ); | ||
| expect(() => handlers.updatePullRequestHandler(undefined)).toThrow( | ||
| expect.objectContaining({ code: -32602 }) | ||
| ); | ||
| }); | ||
|
|
||
| it("should throw MCP error when update_branch is explicitly false and no other fields", () => { | ||
| expect(() => handlers.updatePullRequestHandler({ update_branch: false })).toThrow( | ||
| expect.objectContaining({ code: -32602 }) | ||
| ); | ||
| }); | ||
|
|
||
| it("should write entry and return success when title is provided", () => { | ||
| const result = handlers.updatePullRequestHandler({ title: "New Title" }); | ||
| expect(result).toHaveProperty("content"); | ||
| const data = JSON.parse(result.content[0].text); | ||
| expect(data.result).toBe("success"); | ||
| expect(mockAppendSafeOutput).toHaveBeenCalledWith( | ||
| expect.objectContaining({ type: "update_pull_request", title: "New Title" }) | ||
| ); | ||
| }); | ||
|
|
||
| it("should write entry and return success when body is provided", () => { | ||
| const result = handlers.updatePullRequestHandler({ body: "Updated body" }); | ||
| expect(result).toHaveProperty("content"); | ||
| const data = JSON.parse(result.content[0].text); | ||
| expect(data.result).toBe("success"); | ||
| expect(mockAppendSafeOutput).toHaveBeenCalledWith( | ||
| expect.objectContaining({ type: "update_pull_request", body: "Updated body" }) | ||
| ); | ||
| }); | ||
|
|
||
| it("should write entry and return success when update_branch is true", () => { | ||
| const result = handlers.updatePullRequestHandler({ update_branch: true }); | ||
| expect(result).toHaveProperty("content"); | ||
| const data = JSON.parse(result.content[0].text); | ||
| expect(data.result).toBe("success"); | ||
| expect(mockAppendSafeOutput).toHaveBeenCalledWith( | ||
| expect.objectContaining({ type: "update_pull_request", update_branch: true }) | ||
| ); | ||
| }); | ||
|
|
||
| it("should write entry and return success when both title and body are provided", () => { | ||
| const result = handlers.updatePullRequestHandler({ title: "New Title", body: "New body" }); | ||
| expect(result).toHaveProperty("content"); | ||
| const data = JSON.parse(result.content[0].text); | ||
| expect(data.result).toBe("success"); | ||
| expect(mockAppendSafeOutput).toHaveBeenCalledWith( | ||
| expect.objectContaining({ type: "update_pull_request", title: "New Title", body: "New body" }) | ||
| ); | ||
| }); | ||
|
|
||
| it("error message should mention all required fields", () => { | ||
| try { | ||
| handlers.updatePullRequestHandler({}); | ||
|
Contributor
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. [/tdd] This test uses it('error message should mention all required fields', () => {
expect(() => handlers.updatePullRequestHandler({})).toThrow(
expect.objectContaining({
message: expect.stringMatching(/title.*body.*update_branch/s),
})
);
});Consistent style makes the test suite easier to scan and keeps coverage reporters accurate. |
||
| expect.fail("Should have thrown"); | ||
| } catch (err) { | ||
| expect(err.message).toContain("'title'"); | ||
| expect(err.message).toContain("'body'"); | ||
| expect(err.message).toContain("'update_branch'"); | ||
| } | ||
| }); | ||
| }); | ||
| }); | ||
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.
@copilot move inline helper to top level function and add tests
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.
Done in the latest commit. Extracted the validation logic into a top-level
hasUpdatePullRequestFields(args)function (placed betweenbuildIntentErrorResponseandcreateHandlers), exported it frommodule.exports, and added 12 direct unit tests for it in a new top-leveldescribe("hasUpdatePullRequestFields", ...)block — coveringnull,undefined,{},false/nullfield values, and all valid field combinations.