Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
2 changes: 1 addition & 1 deletion .github/workflows/smoke-otel-backends.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

28 changes: 28 additions & 0 deletions actions/setup/js/safe_outputs_handlers.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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 => {

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Contributor Author

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 between buildIntentErrorResponse and createHandlers), exported it from module.exports, and added 12 direct unit tests for it in a new top-level describe("hasUpdatePullRequestFields", ...) block — covering null, undefined, {}, false/null field values, and all valid field combinations.

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`,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] The update_branch: false exclusion rule is correctly implemented here and mirrors safe_output_type_validator.cjs, but the shared semantic is captured only in a comment with no shared constant or utility. If the downstream validator's logic ever changes (e.g., treating 0 or null the same as false), this handler will silently diverge.

Consider extracting a tiny shared predicate, or at minimum adding a cross-reference comment pointing to the exact line in safe_output_type_validator.cjs where the parallel logic lives, so future maintainers update both places together.

};
}

return defaultHandler("update_pull_request")(safeArgs);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/diagnose] The draft field is present in the inputSchema and can legitimately represent update intent (converting a PR from draft → ready or vice-versa). A call like update_pull_request({ draft: true }) will pass this handler's validation (none of title/body/update_branch are set), get written to NDJSON, but may then fail downstream if requiresOneOf is the only gating mechanism.

Is draft intentionally excluded from the "at least one of" constraint? If it's a valid standalone update, the description should say so; if not, the handler should gate on it too.

};

return {
defaultHandler,
uploadAssetHandler,
Expand All @@ -1371,6 +1398,7 @@ function createHandlers(server, appendSafeOutput, config = {}) {
addCommentHandler,
createPullRequestReviewCommentHandler,
submitPullRequestReviewHandler,
updatePullRequestHandler,
};
}

Expand Down
77 changes: 77 additions & 0 deletions actions/setup/js/safe_outputs_handlers.test.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -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({});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[/tdd] This test uses try/catch + expect.fail instead of Jest's toThrow matcher, which is the pattern used everywhere else in this file (e.g., line 1764). Consider:

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'");
}
});
});
});
2 changes: 1 addition & 1 deletion actions/setup/js/safe_outputs_tools.json
Original file line number Diff line number Diff line change
Expand Up @@ -799,7 +799,7 @@
},
{
"name": "update_pull_request",
"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.",
"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.",
"inputSchema": {
"type": "object",
"properties": {
Comment on lines 801 to 806
Expand Down
1 change: 1 addition & 0 deletions actions/setup/js/safe_outputs_tools_loader.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ function attachHandlers(tools, handlers) {
add_comment: handlers.addCommentHandler,
create_pull_request_review_comment: handlers.createPullRequestReviewCommentHandler,
submit_pull_request_review: handlers.submitPullRequestReviewHandler,
update_pull_request: handlers.updatePullRequestHandler,
};

tools.forEach(tool => {
Expand Down
2 changes: 1 addition & 1 deletion pkg/workflow/js/safe_outputs_tools.json
Original file line number Diff line number Diff line change
Expand Up @@ -943,7 +943,7 @@
},
{
"name": "update_pull_request",
"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.",
"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.",
"inputSchema": {
"type": "object",
"properties": {
Comment on lines 945 to 949
Expand Down
Loading