Uh oh!
There was an error while loading. Please reload this page.
Support explicit auto-merge strategies in safe-outputs.create-pull-request - #49412
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
safe-outputs.create-pull-requestpelikhan
commented
Aug 1, 2026
@copilot auto-merge true should be squash by default |
✅ Test Quality Sentinel completed test quality analysis. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
✅ PR Code Quality Reviewer completed the code quality review. Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. |
There was a problem hiding this comment.
Pull request overview
Adds explicit auto-merge strategies while retaining boolean behavior.
Changes:
- Accepts
squash,merge, andrebase. - Maps strategies to GraphQL merge methods.
- Adds schema, documentation, and test coverage.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/create_pull_request.go | Parses and validates strategies. |
pkg/workflow/compile_outputs_pr_test.go | Tests compilation. |
pkg/workflow/safe_outputs_config_generation_test.go | Tests serialization. |
pkg/parser/schemas/main_workflow_schema.json | Extends the schema. |
pkg/parser/schema_test.go | Tests schema structure. |
actions/setup/js/create_pull_request.cjs | Applies GraphQL merge methods. |
actions/setup/js/create_pull_request.test.cjs | Tests runtime invocation. |
.github/aw/safe-outputs-content.md | Documents strategy configuration. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
| switch (normalized) { | ||
| case "squash": | ||
| return { enabled: true, mergeMethod: "SQUASH" }; | ||
| case "merge": | ||
| return { enabled: true, mergeMethod: "MERGE" }; | ||
| case "rebase": | ||
| return { enabled: true, mergeMethod: "REBASE" }; | ||
| default: | ||
| return { enabled: parseBoolTemplatable(value, false) }; | ||
| } |
There was a problem hiding this comment.
This concern is addressed in commit 2e1fa40. The parseAutoMergeConfig function now handles "true" explicitly (mapping it to SQUASH), and any unrecognized value triggers core.warning(...) and returns { enabled: false } — so typos like sqaush fail closed with a visible warning instead of silently enabling auto-merge.
| "type": "boolean", | ||
| "description": "Enable auto-merge for the pull request. When enabled, the PR will be automatically merged once all required checks pass and required approvals are met. Defaults to false.", | ||
| "default": false | ||
| "description": "Enable auto-merge for the pull request. Accepts true/false or an explicit merge method string (squash, merge, rebase). When enabled, the PR will be automatically merged once all required checks pass and required approvals are met. Defaults to false.", |
There was a problem hiding this comment.
Regenerated docs/src/content/docs/reference/frontmatter-full.md via node scripts/generate-schema-docs.js — the reference now documents auto-merge with boolean, string (squash/merge/rebase), and GitHub Actions expression formats. Committed in 76ea9f1.
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review: Support explicit auto-merge strategies
The implementation is well-structured and correct.
✅ Correctness
parseAutoMergeConfigcorrectly handles all documented input forms: booleanfalse/true, string strategy values, and templatable expressions.- The GraphQL mutation correctly passes
mergeMethodas an optional variable — GitHub's API acceptsnullformergeMethod(booleantruepath), preserving existing behavior. - Go compile-time validation correctly removes
auto-mergefromBoolFieldsand adds explicit validation for the expanded value set. - Schema uses
oneOfwith three variants (boolean, enum string, expression pattern) — semantics are sound.
✅ Backward compatibility
- Existing
auto-merge: true/falseconfigurations compile and behave identically to before.
✅ Test coverage
- JS tests cover explicit strategy and boolean-without-method paths.
- Go tests cover schema validation, config parsing, and compiled lock file content.
No blocking issues found.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 19.4 AIC · ⊞ 5.3K
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (142 new lines across 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the 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. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Skills-Based Review 🧠
Applied /tdd and /grill-with-docs — requesting changes on two actionability and documentation gaps.
📋 Key Themes & Highlights
Key Themes
- Silent runtime failure:
parseAutoMergeConfig'sdefaultbranch quietly disables auto-merge on unrecognized input (typos, future values). Acore.warningwould surface this in the Actions log. - Unit test coverage:
parseAutoMergeConfigis only tested end-to-end viamain(). Direct unit tests for the"true"/"false"string path, null, and unknown values would make regressions immediately obvious. - Documentation gaps: The schema
descriptiondoesn't mention expression support, and the reference doc line dropped the explicitfalsedefault.
Positive Highlights
- ✅ Clean layered approach: schema → compile-time validation → runtime parsing → GraphQL mutation
- ✅ Backward compatibility is well-preserved and explicitly tested
- ✅ Boolean normalization to strings at compile time keeps the runtime handler simple
- ✅ Comprehensive test coverage across all three layers (schema, Go compile, JS runtime)
- ✅ GraphQL variable typed as nullable (
PullRequestMergeMethod) soundefinedcorrectly omits the argument forauto-merge: true
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 37 AIC · ⊞ 7K
Comment /matt to run again
| it("passes an explicit auto-merge method to GraphQL", async () => { | ||
| const { main } = require("./create_pull_request.cjs"); | ||
| const handler = await main({ auto_merge: "rebase", allow_empty: true }); |
There was a problem hiding this comment.
[/tdd] The "rebase" string case is tested via main(), but parseAutoMergeConfig has no direct unit tests for the "true" string, "false" string, null, or empty inputs — these fall through to parseBoolTemplatable via the default branch and are easy to regress.
💡 Suggested additions
Export parseAutoMergeConfig and add focused unit tests:
it('treats string "true" as enabled with no method',()=>{expect(parseAutoMergeConfig('true')).toEqual({enabled: true});});it('treats string "false" as disabled',()=>{expect(parseAutoMergeConfig('false')).toEqual({enabled: false});});it('treats null as disabled',()=>{expect(parseAutoMergeConfig(null)).toEqual({enabled: false});});it('treats unknown string as disabled',()=>{expect(parseAutoMergeConfig('unknown')).toEqual({enabled: false});});The default branch passes the original value (not the lowercased normalized) to parseBoolTemplatable — correct for expression strings, but worth a comment.
@copilot please address this.
There was a problem hiding this comment.
Exported parseAutoMergeConfig and added a dedicated "parseAutoMergeConfig unit tests" describe block in create_pull_request.test.cjs covering: true (bool and string → SQUASH), false (bool and string → disabled), null, undefined, each explicit strategy, an unrecognized string (warns + disabled), and empty string. Committed in 76ea9f1.
| case "rebase": | ||
| return { enabled: true, mergeMethod: "REBASE" }; | ||
| default: | ||
| return { enabled: parseBoolTemplatable(value, false) }; |
There was a problem hiding this comment.
[/tdd] The default branch of parseAutoMergeConfig silently returns { enabled: false } for any unrecognized string (e.g. a typo like "squahs"). This makes misconfiguration invisible at runtime — the PR just won't auto-merge with no warning or error.
💡 Suggested fix
Log a warning for unrecognized non-boolean, non-expression strings so authors can detect misconfiguration:
default:
if(!/^\$\{\{.*\}\}$/.test(normalized)){// Not a boolean and not an expression — likely a typoconsole.warn(`[auto-merge] Unrecognized value "${normalized}", disabling auto-merge`);}return{enabled: parseBoolTemplatable(value,false)};Alternatively, emit a core.warning so it shows up in the Actions log.
@copilot please address this.
There was a problem hiding this comment.
Fixed in commit 2e1fa40. The default branch now calls core.warning(\Unrecognized auto-merge value "${value}". Expected true, false, "squash", "merge", or "rebase". Auto-merge will be disabled.`)and returns{ enabled: false }`, making typos clearly visible in the Actions log rather than silently disabling auto-merge.
| ] | ||
| }, | ||
| "base-branch": { | ||
| "type": "string", |
There was a problem hiding this comment.
[/grill-with-docs] The oneOf for auto-merge includes a GitHub Actions expression variant (pattern: ^\\$\{\{.*\}\}\$), but the description field only mentions true/false or an explicit merge method string. A user relying solely on the schema description won't know expressions are supported.
💡 Suggested fix
Update the description to mention expression support:
"description": "Enable auto-merge for the pull request. Accepts true/false, an explicit merge method (squash, merge, rebase), or a GitHub Actions expression. When enabled, the PR is automatically merged once all required checks pass.",@copilot please address this.
There was a problem hiding this comment.
Updated the auto-merge description in pkg/parser/schemas/main_workflow_schema.json to: "Accepts true/false, an explicit merge method (squash, merge, rebase), or a GitHub Actions expression resolving to any of those values." Schema docs regenerated to reflect the change. Committed in 76ea9f1.
| allow-empty: false # Optional: create PR with empty branch, no changes required (default: false) | ||
| expires: 7 # Optional: auto-close after 7 days (supports: 2h, 7d, 2w, 1m, 1y; min: 2h) | ||
| auto-merge: false# Optional: enable auto-merge when checks pass (default: false) | ||
| auto-merge: squash # Optional: false/true or merge method squash|merge|rebase for native auto-merge |
There was a problem hiding this comment.
[/grill-with-docs] The inline comment now says squash as the example value but no longer shows the false default — readers can't tell the default value from this line alone.
💡 Suggested wording
auto-merge: squash # Optional: false (default), true, or merge method: squash|merge|rebaseThis makes the default explicit and matches the convention used by neighbouring fields like allow-empty and draft.
@copilot please address this.
There was a problem hiding this comment.
Updated the inline comment to # Optional: false (default), true, or merge method: squash|merge|rebase, making the default explicit and matching the convention used by neighbouring fields. Committed in 76ea9f1.
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. 🧪 Test Quality Sentinel Report
📊 Metrics (6 tests)
|
There was a problem hiding this comment.
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Review verdict: COMMENT
Solid, well-tested extension of auto-merge to support explicit merge strategies (squash/merge/rebase) with backward-compatible boolean handling. One maintainability/consistency concern worth addressing before or shortly after merge.
Themes and highlights
- Schema change (
oneOfboolean/enum/expression-string) follows the existing templatable-field pattern used elsewhere in the schema and is correctly reflected in new schema tests. - JS runtime
parseAutoMergeConfigcorrectly maps the new string values to GraphQLmergeMethod, and the GraphQL call site relies onundefinedbeing omitted for the boolean/no-strategy case — this is standard Octokit/GraphQL behavior and is covered by a test assertingmergeMethod: undefinedis accepted. - Go-side validation of
auto-merge(create_pull_request.go ~113-130) diverges from the siblingvalidateStringEnumFieldpattern used forprotected-files/patch-formatin the same function: instead of silently dropping just the bad field, an invalidauto-mergevalue aborts the entirepreUnmarshalcallback, which discards the wholecreate-pull-requestconfig (seeparseCreateEntityConfig, create_entity_helpers.go:50-52). This is a meaningfully different (and more severe) failure mode than its neighbors, with no test exercising it. Flagged inline. - Minor, non-blocking: no test found for an actually-invalid
auto-mergestring (e.g., a typo like "sqaush") to confirm/document the abort behavior is intentional.
No sub-agent (grumpy-coder) findings were incorporated — analysis here is from an independent manual second pass over the diff and surrounding code paths (config parsing, schema validation ordering, handler serialization).
🔎 Code quality review by PR Code Quality Reviewer · auto · 148.9 AIC · ⊞ 7.8K
Comment /review to run again
| if val, exists := configData["auto-merge"]; exists { | ||
| switch v := val.(type) { | ||
| case bool: | ||
| if v { | ||
| configData["auto-merge"] = "true" | ||
| } else { | ||
| configData["auto-merge"] = "false" | ||
| } | ||
| case string: | ||
| if !isExpression(v) && v != "true" && v != "false" && v != "squash" && v != "merge" && v != "rebase" { |
There was a problem hiding this comment.
Inline validation duplicates validateStringEnumField's job but with a stricter, inconsistent failure mode.
There was a problem hiding this comment.
Replaced the inline enum validation switch with behavior consistent with validateStringEnumField: invalid string values and unexpected types now delete the auto-merge key and log a debug message rather than returning false. Committed in 76ea9f1.
| if val, exists := configData["auto-merge"]; exists { | ||
| switch v := val.(type) { | ||
| case bool: | ||
| if v { | ||
| configData["auto-merge"] = "true" | ||
| } else { | ||
| configData["auto-merge"] = "false" | ||
| } | ||
| case string: | ||
| if !isExpression(v) && v != "true" && v != "false" && v != "squash" && v != "merge" && v != "rebase" { | ||
| createPRLog.Printf("Invalid auto-merge value %q", v) | ||
| return false | ||
| } | ||
| default: | ||
| createPRLog.Printf("Invalid auto-merge value type %T", val) | ||
| return false |
There was a problem hiding this comment.
This inline switch reimplements enum validation but aborts the entire create-pull-request config on failure, unlike sibling fields.
💡 Inconsistent failure severity vs. validateStringEnumField
Compare with lines 108/111 which use validateStringEnumField for protected-files and patch-format: on an invalid value, that helper just deletes the single bad key and logs a debug message — the rest of the create-pull-request config still parses normally.
This new auto-merge block instead returns false from the preUnmarshal callback (line 124/128), which per parseCreateEntityConfig (create_entity_helpers.go:50-52) causes the whole config to be discarded (return nil). A single typo like auto-merge: sqaush silently disables the entire create-pull-request safe-output, not just auto-merge — a much more severe and surprising blast radius than its sibling enum fields in the same function, with no test covering this abort path.
Suggested fix: reuse validateStringEnumField-style behavior (drop only the bad field with a debug log) instead of aborting, or if aborting is intentional, add an explicit compile-time error surfaced to the user (not just a debug log) plus a regression test asserting the behavior.
There was a problem hiding this comment.
Fixed the inconsistent blast radius: the invalid-value cases now call delete(configData, "auto-merge") and log the issue instead of return false. This matches validateStringEnumField semantics — a bad auto-merge value silently drops only that field, leaving the rest of the create-pull-request config intact. Committed in 76ea9f1.
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
… values Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. Great work on extending auto-merge support! 🎯 This PR adds explicit merge strategy control ( What looks solid:
The PR is well-focused, thoroughly tested, and ready for maintainer review. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "patchdiff.githubusercontent.com"See Network Configuration for more information.
|
…nt failure mode, and unit tests Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
All outstanding review feedback addressed:
|
gh-aw-bot
commented
Aug 1, 2026
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
|
Here is the full pr-finisher pass: Checks (local) ✅ — Branch ✅ — already up-to-date with Review threads ✅ — all 8 in-scope threads now have substantive Copilot replies. Two threads that were missing replies ( CHANGES_REQUESTED reviews — both stale reviews (Matt Pocock Skills Reviewer, Test Quality Sentinel) were submitted against the old commit Checks (CI) ❓ — stale after agent push; the agent cannot re-trigger CI. A human re-trigger (close/reopen the PR or push from a maintainer) is needed for fresh check results. Hand-off: all agent-actionable work is complete. To merge: dismiss the two stale |
gh-aw-bot
commented
Aug 1, 2026
Warning threat detection engine error DetailsThe threat detection engine failed to produce results. Review the workflow run logs for details. @copilot please run the
|
All agent-actionable work is already complete. Here's the confirmed state: Thread resolution — All 8 threads have substantive Copilot replies. Attempted to resolve them via GraphQL and the GitHub web UI, but both fail with 422/permission errors. Resolving review threads requires maintainer write access that the Copilot agent does not have. A maintainer must resolve them from the PR Files tab. Thread IDs ready to resolve:
Stale blocking reviews — The two Branch — up-to-date with Required human actions to unblock merge:
|
🎉 This pull request is included in a new release. Release: |
safe-outputs.create-pull-requestcould enable auto-merge, but only as a boolean and without controlling the merge strategy. This change allows workflows to request GitHub native auto-merge with an explicit method while keeping existingtrue/falsebehavior intact.Config surface
safe-outputs.create-pull-request.auto-mergeto accept:true/falsesquashmergerebaseCompile-time handling
Runtime behavior
PullRequestMergeMethod.mergeMethodtoenablePullRequestAutoMergeonly when a strategy is specified.auto-merge: truebehavior unchanged so repositories still use their default merge configuration.Coverage
Example:
This compiles to a handler config that preserves the selected strategy and enables native auto-merge with
SQUASHfor the created PR.Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
run: https://github.com/github/gh-aw/actions/runs/30676859249
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Run details: https://github.com/github/gh-aw/actions/runs/30678610470
Warning
threat detection engine error
The threat detection engine encountered an error and could not complete analysis. This is a tooling failure, not a security finding.
Details
The threat detection engine failed to produce results.
Review the workflow run logs for details.
Run: https://github.com/github/gh-aw/actions/runs/30680808859