fix: reject create_pull_request/push_to_pull_request_branch when branch equals base_branch after detection#34138
Conversation
…branch equals base_branch after detection Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR makes safe output branch handling fail fast when the resolved feature branch and base branch are the same, preventing malformed PR/push intents from reaching the safe outputs job.
Changes:
- Adds self-targeting branch rejection to
create_pull_request. - Adds the same rejection to
push_to_pull_request_branch. - Adds regression tests for both handlers using confused GitHub branch env vars.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/safe_outputs_handlers.cjs |
Adds post-detection validation that rejects branch === base_branch with clear errors. |
actions/setup/js/safe_outputs_handlers.test.cjs |
Adds coverage for create PR and push-to-PR-branch rejection scenarios. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
|
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR #34138 does not have the 'implementation' label and has 0 new lines of code in default business logic directories (src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/). |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer failed during the skills-based review. |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Test Classification Details
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. Analysis HighlightsBoth new tests demonstrate excellent test quality:
What makes these design tests?
|
|
|
When
getBaseBranch()resolves to the feature branch itself (e.g.GITHUB_BASE_REFset to the feature branch due to a confused event context), the existing auto-detection viagetCurrentBranch()also returns the same value — leavingentry.branch === entry.base_branch. The malformed safe output was written silently, and thesafe_outputsjob then failed with a crypticgit exit code 1trying to fetch the non-existent remote ref during checkout.Changes
safe_outputs_handlers.cjs— bothcreatePullRequestHandlerandpushToPullRequestBranchHandlernow guard against this after the detection step:Instead of propagating a self-targeting safe output, the agent receives a clear, actionable error at tool-call time.
safe_outputs_handlers.test.cjs— two new tests cover the failing scenario for both handlers, simulatingGITHUB_BASE_REFincorrectly set to the feature branch.