safeoutputs: prohibit concurrency-control fields on push_to_pull_request_branch#45822
Conversation
…_pull_request_branch Add explicit prohibition sentence to the `push_to_pull_request_branch` description in both the source and runtime safe_outputs_tools.json files, explaining that the tool auto-pins the PR branch HEAD before pushing and therefore takes no concurrency-control / compare-and-swap parameter. Agents must NOT pass `expected_head_sha`, `head_sha`, or `base_sha`. Closes #45821 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
🤖 PR Triage
Score breakdown: Impact 10 + Urgency 8 + Quality 10 DRAFT. Updates Run §29462479491
|
There was a problem hiding this comment.
Pull request overview
Updates the safe-outputs tool documentation to prevent agents from supplying unsupported concurrency-control fields when using push_to_pull_request_branch, reducing runtime rejections caused by invented CAS parameters.
Changes:
- Appends an explicit prohibition note to the
push_to_pull_request_branchtool description clarifying it auto-pins PR HEAD and must not receiveexpected_head_sha,head_sha, orbase_sha. - Keeps both JSON copies of the safe-outputs tool definitions in sync (
pkg/workflow/js/...andactions/setup/js/...).
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/js/safe_outputs_tools.json | Adds a clarification/prohibition note to push_to_pull_request_branch tool description about unsupported concurrency-control fields. |
| actions/setup/js/safe_outputs_tools.json | Mirrors the same description update to keep the action-bundled tools JSON consistent. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Low
| { | ||
| "name": "push_to_pull_request_branch", | ||
| "description": "Push committed changes to a pull request's branch. APPEND-ONLY: this tool adds new commits on top of the existing PR branch \u2014 force-push is NOT supported and will be rejected. Use this to add follow-up commits to an existing PR, such as addressing review feedback or fixing issues. This is a write-once declaration for a real intended PR branch update, not a sandbox or probe: do not call it with probe branches, placeholder commit messages, or auth experiments. If you are not ready to push the real update, use noop or report_incomplete instead. Changes must be committed locally before calling this tool. IMPORTANT: always supply the 'branch' argument with the local branch name you committed to. In batch workflows that process multiple PRs, this is required \u2014 if omitted, the branch is inferred from the current git HEAD, which will produce wrong results if the workspace has been checked out to a different branch between commit and tool-call time. IMPORTANT: do NOT use 'git merge' to update the branch against another branch \u2014 merge commits cannot be signed; the action will attempt to squash them into a single linear commit before pushing, but this rewrites history. Use 'git rebase' instead to avoid the rewrite.", | ||
| "description": "Push committed changes to a pull request's branch. APPEND-ONLY: this tool adds new commits on top of the existing PR branch \u2014 force-push is NOT supported and will be rejected. Use this to add follow-up commits to an existing PR, such as addressing review feedback or fixing issues. This is a write-once declaration for a real intended PR branch update, not a sandbox or probe: do not call it with probe branches, placeholder commit messages, or auth experiments. If you are not ready to push the real update, use noop or report_incomplete instead. Changes must be committed locally before calling this tool. IMPORTANT: always supply the 'branch' argument with the local branch name you committed to. In batch workflows that process multiple PRs, this is required \u2014 if omitted, the branch is inferred from the current git HEAD, which will produce wrong results if the workspace has been checked out to a different branch between commit and tool-call time. IMPORTANT: do NOT use 'git merge' to update the branch against another branch \u2014 merge commits cannot be signed; the action will attempt to squash them into a single linear commit before pushing, but this rewrites history. Use 'git rebase' instead to avoid the rewrite. This tool auto-pins the PR branch HEAD before pushing, so it takes no concurrency-control / compare-and-swap parameter \u2014 do NOT pass expected_head_sha, head_sha, or base_sha; the only accepted fields are message, branch, and pull_request_number.", |
| { | ||
| "name": "push_to_pull_request_branch", | ||
| "description": "Push committed changes to a pull request's branch. APPEND-ONLY: this tool adds new commits on top of the existing PR branch \u2014 force-push is NOT supported and will be rejected. Use this to add follow-up commits to an existing PR, such as addressing review feedback or fixing issues. This is a write-once declaration for a real intended PR branch update, not a sandbox or probe: do not call it with probe branches, placeholder commit messages, or auth experiments. If you are not ready to push the real update, use noop or report_incomplete instead. Changes must be committed locally before calling this tool. IMPORTANT: always supply the 'branch' argument with the local branch name you committed to. In batch workflows that process multiple PRs, this is required \u2014 if omitted, the branch is inferred from the current git HEAD, which will produce wrong results if the workspace has been checked out to a different branch between commit and tool-call time. IMPORTANT: do NOT use 'git merge' to update the branch against another branch \u2014 merge commits cannot be signed; the action will attempt to squash them into a single linear commit before pushing, but this rewrites history. Use 'git rebase' instead to avoid the rewrite.", | ||
| "description": "Push committed changes to a pull request's branch. APPEND-ONLY: this tool adds new commits on top of the existing PR branch \u2014 force-push is NOT supported and will be rejected. Use this to add follow-up commits to an existing PR, such as addressing review feedback or fixing issues. This is a write-once declaration for a real intended PR branch update, not a sandbox or probe: do not call it with probe branches, placeholder commit messages, or auth experiments. If you are not ready to push the real update, use noop or report_incomplete instead. Changes must be committed locally before calling this tool. IMPORTANT: always supply the 'branch' argument with the local branch name you committed to. In batch workflows that process multiple PRs, this is required \u2014 if omitted, the branch is inferred from the current git HEAD, which will produce wrong results if the workspace has been checked out to a different branch between commit and tool-call time. IMPORTANT: do NOT use 'git merge' to update the branch against another branch \u2014 merge commits cannot be signed; the action will attempt to squash them into a single linear commit before pushing, but this rewrites history. Use 'git rebase' instead to avoid the rewrite. This tool auto-pins the PR branch HEAD before pushing, so it takes no concurrency-control / compare-and-swap parameter \u2014 do NOT pass expected_head_sha, head_sha, or base_sha; the only accepted fields are message, branch, and pull_request_number.", |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #45822 does not have the 'implementation' label and has only 1 new line of code in business logic directories (threshold is 100). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /grill-with-docs — requesting one change before merge.
📋 Key Themes & Highlights
Key Themes
- Inaccurate field enumeration: The new sentence says the
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 12.1 AIC · ⌖ 4.27 AIC · ⊞ 6.7K
Comment /matt to run again
There was a problem hiding this comment.
Review
The change correctly documents the prohibition on concurrency-control fields (expected_head_sha, head_sha, base_sha) for push_to_pull_request_branch, following the established "named X, NOT Y" prohibition style. Both JSON copies are updated identically.
Minor inaccuracy (non-blocking): The new sentence claims "the only accepted fields are message, branch, and pull_request_number" but the inputSchema also accepts secrecy and integrity metadata fields. This could mislead agents into omitting those standard safe-outputs fields. Consider narrowing the wording to explicitly name only the forbidden concurrency-control fields rather than enumerating all allowed fields.
Overall the PR is correct and safe to merge.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 13.7 AIC · ⌖ 4.33 AIC · ⊞ 5K
There was a problem hiding this comment.
Factual inaccuracy in the new prohibition sentence
The added sentence claims the only accepted fields are , , and , but the tool's own also declares and as valid properties. As written, this would cause agents to omit those metadata fields even when they are needed.
💡 Suggested fix
Replace:
the only accepted fields are
message,branch, andpull_request_number
With a narrower prohibition that targets only the concurrency-control fields:
do NOT pass
expected_head_sha,head_sha,base_sha, or any other compare-and-swap field; the only concurrency-control accepted fields aremessage,branch, andpull_request_number
Or simply drop the exhaustive enumeration and end at the prohibition:
do NOT pass
expected_head_sha,head_sha, orbase_sha
This avoids implying that secrecy and integrity are disallowed.
The rest of the change — the auto-pin disclosure and the prohibition of the three specific fields — is accurate and fixes a real agent confusion pattern. The fix is straightforward: scope the "only accepted fields" claim to just the concurrency-control context, or remove the exhaustive enumeration.
🔎 Code quality review by PR Code Quality Reviewer · 31.1 AIC · ⌖ 4.36 AIC · ⊞ 5.6K
Comment /review to run again
|
🎉 This pull request is included in a new release. Release: |
The
push_to_pull_request_branchtool description didn't explain that it auto-pins the PR branch HEAD, causing agents to invent concurrency-control fields (expected_head_sha,head_sha,base_sha) that get rejected at runtime. Same field, same workflow, same engine — second occurrence in three weeks, meeting the escalation threshold.Changes
pkg/workflow/js/safe_outputs_tools.json/actions/setup/js/safe_outputs_tools.json— appended to thepush_to_pull_request_branchdescription:Follows the established "named X, NOT Y" prohibition style already used for
message/commit_messagedisambiguation. Both copies kept in sync (TestSafeOutputsToolsJSONInSyncvalidates this).