Analyze: update_pull_request with empty args silently drops safe outputs#33134
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot Do the work suggested in the description |
…nd update schema description Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…mment Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Three gaps addressed across two commits:
The |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 6M
| if (!hasTitle && !hasBody && !hasUpdateBranch) { | ||
| throw { | ||
| code: -32602, | ||
| message: `${ERR_VALIDATION}: update_pull_request requires at least one of: 'title', 'body', 'update_branch' fields`, |
There was a problem hiding this comment.
[/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); |
There was a problem hiding this comment.
[/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.
|
|
||
| it("error message should mention all required fields", () => { | ||
| try { | ||
| handlers.updatePullRequestHandler({}); |
There was a problem hiding this comment.
[/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.
There was a problem hiding this comment.
Pull request overview
This PR addresses a safe-outputs validation gap where update_pull_request({}) previously returned success while silently producing an invalid/empty NDJSON entry downstream, causing safe output types to be dropped and jobs to be skipped.
Changes:
- Add a dedicated
update_pull_requestMCP handler that rejects invocations with no meaningful update fields, returning an MCP invalid-params error immediately. - Register the new handler in the tool loader and add unit tests covering the new validation behavior.
- Update tool descriptions to warn that at least one of
title/body/update_branchis required, and tweak a Datadog MCP header env var placeholder in a locked workflow.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/js/safe_outputs_tools.json | Updates update_pull_request tool description to document the at-least-one-field requirement. |
| actions/setup/js/safe_outputs_tools.json | Same description update for the setup action’s tool manifest. |
| actions/setup/js/safe_outputs_tools_loader.cjs | Registers a dedicated handler for update_pull_request instead of falling through to defaultHandler. |
| actions/setup/js/safe_outputs_handlers.cjs | Implements updatePullRequestHandler to enforce requires-one-of validation at invocation time. |
| actions/setup/js/safe_outputs_handlers.test.cjs | Adds test coverage for updatePullRequestHandler error/success paths. |
| .github/workflows/smoke-otel-backends.lock.yml | Changes Datadog MCP header placeholder from ${DD_APPLICATION_KEY} to ${DD_APP_KEY}. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/6 changed files
- Comments generated: 4
| "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": { |
| "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": { |
| 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; |
| "headers": { | ||
| "DD_API_KEY": "\${DD_API_KEY}", | ||
| "DD_APPLICATION_KEY": "\${DD_APPLICATION_KEY}", | ||
| "DD_APPLICATION_KEY": "\${DD_APP_KEY}", |
🧪 Test Quality Sentinel ReportTest Quality Score: 70/100
Verdict
|
There was a problem hiding this comment.
✅ Test Quality Sentinel: 70/100. Test quality is acceptable — 0% of new tests are implementation tests (threshold: 30%). No coding-guideline violations detected. Minor suggestions: consolidate 4 near-identical happy-path tests into a table-driven it.each pattern to reduce duplication and the 2.75:1 test inflation ratio.
|
|
|
@copilot review all comments |
…s 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>
…moke-otel-backends.lock.yml Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All four review comments addressed:
|
|
@copilot ensure claude/codex support anyOf. Otherwise encode it in the tool description. |
… for OpenAI/Codex which ignores anyOf OpenAI function calling does not enforce anyOf in JSON Schema. The anyOf remains for clients that do (Claude and others), while the description now explicitly states the REQUIRED constraint and that update_branch:false does not count, giving all LLMs an upfront signal regardless of anyOf support. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
OpenAI/Codex does not enforce
The |
| * 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 => { |
There was a problem hiding this comment.
@copilot move inline helper to top level function and add tests
There was a problem hiding this comment.
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.
…th tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
When the agent calls
update_pull_request({})(nobody,title, orupdate_branch), the safe outputs MCP server returns{"result":"success"}, but the entry is rejected downstream bycollect_ndjson_output.cjsviarequiresOneOf:title,body,update_branch— causingoutput_typesto be empty, which cascades to bothdetectionandsafe_outputsjobs being skipped. No actual PR update ever executes.Root cause
update_pull_requesthas no dedicated handler inattachHandlers— falls through todefaultHandler, which writes{type: "update_pull_request"}(empty) tosafeoutputs.jsonland returns successrequiresOneOfvalidation only runs incollect_ndjson_output.cjspost-hoc; agent receives no error signal and cannot retryrequiredfields, giving the LLM no upfront signal that at least one oftitle/body/update_branchis neededWhat this analysis identifies (no code changes in this session)
update_pull_requestreceives no meaningful fieldsrequiresOneOfconstraint so models see it before callingcollect_ndjson_output.cjsenforcement are out of sync for this tool typepr-sous-chef: refreshed branch via https://github.com/github/gh-aw/actions/runs/26056882797