Support allowed-branches enforcement for create-pull-request safe output#33610
Conversation
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
allowed-branches enforcement for create-pull-request safe output
There was a problem hiding this comment.
Pull request overview
Adds support for enforcing a configured source-branch allowlist (safe-outputs.create-pull-request.allowed-branches) during create_pull_request safe-output collection, and plumbs the configuration through the workflow compiler into the JS runtime handler config.
Changes:
- Extend
CreatePullRequestsConfigand compiler handler-config generation to includeallowed-branches→allowed_branches. - Enforce
allowed_branchesin thecreatePullRequestHandler, including when the branch is resolved from the current checkout. - Add/extend Go and JS tests to cover expression parsing/config emission and collector-time accept/reject behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/create_pull_request.go | Adds AllowedBranches and treats allowed-branches as a templatable expression-array field. |
| pkg/workflow/config_parsing_helpers_test.go | Updates tests to ensure allowed-branches supports expression parsing and rejects invalid bare strings. |
| pkg/workflow/compiler_safe_outputs_handlers.go | Emits allowed_branches into the runtime handler config for create_pull_request. |
| pkg/workflow/compiler_safe_outputs_config_test.go | Verifies allowed_branches is included/omitted in generated handler config JSON as expected. |
| actions/setup/js/types/safe-outputs-config.d.ts | Adds CreatePullRequestConfig["allowed-branches"] typing. |
| actions/setup/js/safe_outputs_handlers.test.cjs | Adds collector-time tests for allowed-branch enforcement (resolved branch acceptance + rejection). |
| actions/setup/js/safe_outputs_handlers.cjs | Implements parsing and glob matching for allowed_branches and rejects non-matching branches during collection. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 6/7 changed files
- Comments generated: 1
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com>
…upport-allowed-branches-create-pull-request
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Quality Review
I've reviewed the changes for allowed-branches enforcement in create-pull-request safe output. The implementation is solid overall with good test coverage and consistent patterns, but there's one critical inconsistency that needs to be fixed.
Critical Issue
Wildcard handling inconsistency — The new isAllowedBranch() function doesn't handle the literal "*" pattern as a true allow-all, which is inconsistent with existing policy checks in this codebase (isBaseBranchAllowed() in create_pull_request.cjs and similar functions in repo_helpers.cjs). This means allowed_branches: ["*"] would reject common branch names like feature/foo.
What Was Done Well
✅ Comprehensive test coverage — Tests for both accepted and rejected branches
✅ Schema updates — Proper updates to main_workflow_schema.json matching allowed-base-branches style
✅ Documentation — Clear docs in safe-outputs.md and reference docs
✅ Type safety — Updated TypeScript definitions
✅ Consistent error messages — Clear, actionable error when branch doesn't match patterns
✅ Expression support — Properly handles both array and expression forms
Recommendation
Add the pattern === "*" fast-path to isAllowedBranch() to match the existing codebase pattern for wildcard handling. This is a small fix that maintains consistency across all policy enforcement functions.
🔎 Code quality review by PR Code Quality Reviewer · ● 4.7M
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs based on this being a security feature addition with comprehensive test coverage and documentation updates.
Key Themes
Test Coverage Gaps (/tdd)
- Missing edge case tests for empty
allowed_branchesarray vs undefined - Glob pattern behavior (especially
**vs*) not explicitly tested - The core logic is well-tested, but boundary conditions deserve explicit coverage
Terminology Consistency (/grill-with-docs)
- Error messages and tool descriptions have minor inconsistencies between kebab-case (user-facing YAML) and snake_case (internal implementation)
- Tool description doesn't fully reflect that branch validation happens after detection/fallback resolution
- Struct field comments could use more parallel phrasing with related fields
Positive Highlights
✅ Excellent test structure — the two new test cases cleanly demonstrate the happy path (matching pattern) and error path (non-matching pattern)
✅ Consistent architecture — the implementation follows the existing pattern for allowed-base-branches, making the codebase more uniform
✅ Complete plumbing — compiler, schema, handlers, docs all updated together (no missing pieces)
✅ Security-first design — enforcement happens at collection time, not just apply time, which is the right defensive layer
Verdict
Commenting with suggested improvements. The additions are well-structured and secure, but the edge case test coverage and terminology alignment would strengthen the implementation before merge.
Resources:
/tddskill: Focus on red-green-refactor, edge cases, and regression protection/grill-with-docsskill: Ensure domain language consistency and clear user-facing terminology
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 6.8M
Comments that could not be inline-anchored
actions/setup/js/safe_outputs_handlers.cjs:67
[/tdd] The isAllowedBranch function has a subtle edge case that isn't covered by tests: what happens when allowedPatterns is an empty array but still truthy?
Consider adding a test case:
it("should allow any branch when allowed_branches is empty array", async () => {
handlers = createHandlers(mockServer, mockAppendSafeOutput, {
create_pull_request: {
allow_empty: true,
allowed_branches: [], // explicitly empty
},
});
const result = await handle…
</details>
<details><summary>actions/setup/js/safe_outputs_handlers.cjs:62</summary>
**[/tdd]** The glob matching logic uses `pathMode: true` for branch names, but branch names aren't filesystem paths. This works, but the test coverage doesn't verify the glob behavior with different patterns.
Consider adding tests for common glob patterns:
```javascript
it("should match branch with double-star glob pattern", async () => {
handlers = createHandlers(mockServer, mockAppendSafeOutput, {
create_pull_request: {
allow_empty: true,
allowed_branches: ["feature/**"], …
</details>
<details><summary>actions/setup/js/safe_outputs_handlers.cjs:89</summary>
**[/grill-with-docs]** The error message uses "allowed-branches" (kebab-case) but the internal config field is `allowed_branches` (snake_case). This inconsistency might confuse users.
Consider matching the user-facing YAML terminology:
```javascript
error: `Branch '${entry.branch}' does not match allowed-branches patterns. Allowed: ${allowedBranches.join(", ")}`This keeps the error message aligned with the frontmatter field name users write in their workflow config.
actions/setup/js/safe_outputs_tools.json:174
[/grill-with-docs] The updated description says branch name "MUST be given" when allowed-branches is configured, but looking at the handler code (line 81-95 in safe_outputs_handlers.cjs), it enforces the resolved branch including fallback.
The description should clarify:
"Source branch name containing the changes. If omitted, uses the current working branch. When safe-outputs.create-pull-request.allowed-branches is configured, the effective branch (agent-provided or detected fro…
</details>
<details><summary>pkg/workflow/create_pull_request.go:415</summary>
**[/grill-with-docs]** The field comment uses slightly different terminology than `AllowedBaseBranches` above it. Compare:
- Line 414: "Enables agent-provided `base` override when configured."
- Line 416: "Branch in create_pull_request payload must match when configured."
Consider parallel phrasing:
```go
AllowedBranches []string `yaml:"allowed-branches,omitempty"` // List of allowed source branch globs (e.g. "feature/*"). Enforces agent-provided or detected `branch` when configured.
Th…
🏗️ Design Decision Gate — ADR RequiredThis PR adds ~187 new lines in business-logic directories ( AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once the ADR is linked from the PR body, this gate will re-run and verify the implementation matches the recorded decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you for explaining why source-branch policy is enforced in the MCP collector rather than only at apply time. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs in this repo are stored in
References:
|
…https://github.com/github/gh-aw into copilot/support-allowed-branches-create-pull-request
🧪 Test Quality Sentinel ReportTest Quality Score: 91/100✅ Excellent test quality
📋 Test Classification Details
Test Quality HighlightsJavaScript Tests (vitest):
Go Tests:
Test Inflation Analysis:
Language SupportTests analyzed:
Verdict
Score Breakdown:
📖 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.
|
|
Progress update: checks are green, the branch is behind main, and a base update was attempted but could not be pushed here.
|
This change adds branch-policy enforcement for
safe-outputs.create-pull-request.allowed-branches: thecreate_pull_requestcollector now rejects payloads whose selected branch does not match configured branch globs. It also ensures the chosen payload branch remains the branch used in apply-time PR creation flow.Problem scope
Collector/runtime enforcement (
actions/setup/js)allowed_branchesparsing (array or comma-separated string) insafe_outputs_handlers.cjs.entry.branch(including branch resolved from current checkout when agent input is empty/base).Compiler/config plumbing (
pkg/workflow)CreatePullRequestsConfigwithallowed-branches.allowed-branchesin templatable expression-array preprocessing.allowed_branchesin handler config generation so runtime receives and enforces policy.Type definitions
CreatePullRequestConfig["allowed-branches"].Focused test coverage
allowed_branchesallowed-branchesallowed_branchesemission in handler configExample
If the agent emits:
{ "type": "create_pull_request", "branch": "hotfix/urgent", "title": "...", "body": "..." }the collector returns an error and does not record the safe output, because
hotfix/urgentdoes not match configuredallowed-branches.