Skip to content

Commit 8c4c500

Browse files
Copilotpelikhan
andauthored
fix(update_pull_request): strengthen validation and schema constraints per review feedback
- Use typeof/=== true checks in handler (rejects null title/body, null/false update_branch) - Add anyOf JSON Schema constraint to both safe_outputs_tools.json copies so {} is rejected at schema level - Add tests for null title, null body, null update_branch inputs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
1 parent d58eb03 commit 8c4c500

7 files changed

Lines changed: 125 additions & 37 deletions

File tree

.github/workflows/mcp-inspector.lock.yml

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

.github/workflows/pr-sous-chef.lock.yml

Lines changed: 82 additions & 29 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

.github/workflows/smoke-otel-backends.lock.yml

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

actions/setup/js/safe_outputs_handlers.cjs

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1369,12 +1369,12 @@ function createHandlers(server, appendSafeOutput, config = {}) {
13691369
*/
13701370
const updatePullRequestHandler = args => {
13711371
const safeArgs = args || {};
1372-
const hasTitle = safeArgs.title !== undefined;
1373-
const hasBody = safeArgs.body !== undefined;
1374-
// update_branch: false is treated as not provided because it carries no update intent
1375-
// (it's the default behaviour). This mirrors the downstream requiresOneOf validator in
1376-
// safe_output_type_validator.cjs which also excludes field === false from the count.
1377-
const hasUpdateBranch = safeArgs.update_branch !== undefined && safeArgs.update_branch !== false;
1372+
const hasTitle = typeof safeArgs.title === "string";
1373+
const hasBody = typeof safeArgs.body === "string";
1374+
// update_branch must be exactly true to carry update intent; false/null/undefined do not.
1375+
// This mirrors the downstream requiresOneOf validator in safe_output_type_validator.cjs
1376+
// which also excludes field === false from the count.
1377+
const hasUpdateBranch = safeArgs.update_branch === true;
13781378

13791379
if (!hasTitle && !hasBody && !hasUpdateBranch) {
13801380
throw {

actions/setup/js/safe_outputs_handlers.test.cjs

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1790,6 +1790,24 @@ describe("safe_outputs_handlers", () => {
17901790
);
17911791
});
17921792

1793+
it("should throw MCP error when title is null", () => {
1794+
expect(() => handlers.updatePullRequestHandler({ title: null })).toThrow(
1795+
expect.objectContaining({ code: -32602 })
1796+
);
1797+
});
1798+
1799+
it("should throw MCP error when body is null", () => {
1800+
expect(() => handlers.updatePullRequestHandler({ body: null })).toThrow(
1801+
expect.objectContaining({ code: -32602 })
1802+
);
1803+
});
1804+
1805+
it("should throw MCP error when update_branch is null", () => {
1806+
expect(() => handlers.updatePullRequestHandler({ update_branch: null })).toThrow(
1807+
expect.objectContaining({ code: -32602 })
1808+
);
1809+
});
1810+
17931811
it("should write entry and return success when title is provided", () => {
17941812
const result = handlers.updatePullRequestHandler({ title: "New Title" });
17951813
expect(result).toHaveProperty("content");

actions/setup/js/safe_outputs_tools.json

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -799,6 +799,7 @@
799799
},
800800
{
801801
"name": "update_pull_request",
802+
802803
"description": "Update an existing GitHub pull request's title or body. Supports replacing, appending to, or prepending content to the body. Title is always replaced. Only the fields you specify will be updated; other fields remain unchanged. IMPORTANT: At least one of 'title', 'body', or 'update_branch' must be provided; calling this tool with no fields will return an error.",
803804
"inputSchema": {
804805
"type": "object",
@@ -845,6 +846,14 @@
845846
"description": "Trustworthiness level of the message source (e.g., \"low\", \"medium\", \"high\")."
846847
}
847848
},
849+
"anyOf": [
850+
{ "required": ["title"] },
851+
{ "required": ["body"] },
852+
{
853+
"required": ["update_branch"],
854+
"properties": { "update_branch": { "const": true } }
855+
}
856+
],
848857
"additionalProperties": false
849858
}
850859
},

pkg/workflow/js/safe_outputs_tools.json

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1002,6 +1002,14 @@
10021002
"description": "Trustworthiness level of the message source (e.g., \"low\", \"medium\", \"high\")."
10031003
}
10041004
},
1005+
"anyOf": [
1006+
{ "required": ["title"] },
1007+
{ "required": ["body"] },
1008+
{
1009+
"required": ["update_branch"],
1010+
"properties": { "update_branch": { "const": true } }
1011+
}
1012+
],
10051013
"additionalProperties": false
10061014
}
10071015
},

0 commit comments

Comments
 (0)