Skip to content

Commit 3146b06

Browse files
Copilotdsyme
andauthored
safe-outputs: add merge-pull-request schema parity for samples and cross-repo targeting (#39767)
* Initial plan * Plan schema parity fix for merge-pull-request Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com> * Allow merge-pull-request samples and cross-repo target config Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com> * Address review: validate merge-pull-request target, add repo-param metadata, strengthen tests --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: dsyme <7204669+dsyme@users.noreply.github.com> Co-authored-by: Don Syme <dsyme@github.com>
1 parent 6be6637 commit 3146b06

7 files changed

Lines changed: 159 additions & 5 deletions

pkg/parser/schema_safe_outputs_target_test.go

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,6 +269,21 @@ func TestMainWorkflowSchema_SafeOutputsTargetProperties(t *testing.T) {
269269
},
270270
},
271271
},
272+
{
273+
name: "merge-pull-request with target, target-repo, and samples",
274+
safeOutputs: map[string]any{
275+
"merge-pull-request": map[string]any{
276+
"target": "*",
277+
"target-repo": "github/github",
278+
"allowed-repos": []any{"github/docs"},
279+
"samples": []any{
280+
map[string]any{
281+
"merge_method": "squash",
282+
},
283+
},
284+
},
285+
},
286+
},
272287
{
273288
name: "dispatch-workflow with target-repo and allowed-repos",
274289
safeOutputs: map[string]any{

pkg/parser/schemas/main_workflow_schema.json

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8507,6 +8507,21 @@
85078507
},
85088508
"description": "Glob patterns for allowed source branch names (pull request head ref). The PR's branch must match at least one pattern."
85098509
},
8510+
"target": {
8511+
"type": "string",
8512+
"description": "Target for merging: 'triggering' (default, current PR), or '*' (any PR with pull_request_number field)"
8513+
},
8514+
"target-repo": {
8515+
"type": "string",
8516+
"description": "Target repository in format 'owner/repo' for cross-repository operations. Takes precedence over trial target repo settings."
8517+
},
8518+
"allowed-repos": {
8519+
"type": "array",
8520+
"items": {
8521+
"type": "string"
8522+
},
8523+
"description": "List of additional repositories in format 'owner/repo' that pull requests can be merged in. The target repository is always implicitly allowed."
8524+
},
85108525
"github-token": {
85118526
"$ref": "#/$defs/github_token",
85128527
"description": "GitHub token to use for this specific output type. Overrides global github-token if specified."
@@ -8516,6 +8531,22 @@
85168531
"description": "If true, evaluate merge gates and emit preview results without executing the merge API call.",
85178532
"examples": [true, false]
85188533
},
8534+
"samples": {
8535+
"description": "Internal hidden feature. Optional list of declarative sample payloads that exercise this safe-output handler. Used by the hidden `gh aw compile --use-samples` flag to replace the agentic step with a deterministic replay through the safe-outputs MCP server. Each entry should conform to the corresponding MCP tool inputSchema; recognized sidecar keys (currently `patch` for create-pull-request and push-to-pull-request-branch) are stripped before schema validation and consumed by the replay driver.",
8536+
"oneOf": [
8537+
{
8538+
"type": "array",
8539+
"items": {
8540+
"type": "object",
8541+
"additionalProperties": true
8542+
}
8543+
},
8544+
{
8545+
"type": "object",
8546+
"additionalProperties": true
8547+
}
8548+
]
8549+
},
85198550
"required-title-prefix": {
85208551
"type": "string",
85218552
"description": "The target item's title must start with this prefix for this operation to proceed"

pkg/workflow/merge_pull_request.go

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,12 @@ var mergePullRequestLog = logger.New("workflow:merge_pull_request")
66

77
// MergePullRequestConfig holds configuration for merging pull requests with policy checks.
88
type MergePullRequestConfig struct {
9-
BaseSafeOutputConfig `yaml:",inline"`
10-
RequiredLabels []string `yaml:"required-labels,omitempty"` // Labels that must ALL be present on the PR
11-
AllowedBranches []string `yaml:"allowed-branches,omitempty"` // Glob patterns for source branch names; PR branch must match one
12-
RequiredTitlePrefix string `yaml:"required-title-prefix,omitempty"` // Title prefix the PR must have
13-
AllowedLabels []string `yaml:"allowed-labels,omitempty"` // Deprecated: use required-labels
9+
BaseSafeOutputConfig `yaml:",inline"`
10+
SafeOutputTargetConfig `yaml:",inline"`
11+
RequiredLabels []string `yaml:"required-labels,omitempty"` // Labels that must ALL be present on the PR
12+
AllowedBranches []string `yaml:"allowed-branches,omitempty"` // Glob patterns for source branch names; PR branch must match one
13+
RequiredTitlePrefix string `yaml:"required-title-prefix,omitempty"` // Title prefix the PR must have
14+
AllowedLabels []string `yaml:"allowed-labels,omitempty"` // Deprecated: use required-labels
1415
}
1516

1617
// parseMergePullRequestConfig handles merge-pull-request configuration.
@@ -28,6 +29,8 @@ func (c *Compiler) parseMergePullRequestConfig(outputMap map[string]any) *MergeP
2829
// Deprecated: allowed-labels is migrated to required-labels by the codemod
2930
cfg.RequiredLabels = ParseStringArrayFromConfig(configMap, "allowed-labels", mergePullRequestLog)
3031
}
32+
targetConfig, _ := ParseTargetConfig(configMap)
33+
cfg.SafeOutputTargetConfig = targetConfig
3134
cfg.AllowedBranches = ParseStringArrayFromConfig(configMap, "allowed-branches", mergePullRequestLog)
3235
cfg.RequiredTitlePrefix = extractStringFromMap(configMap, "required-title-prefix", mergePullRequestLog)
3336
c.parseBaseSafeOutputConfig(configMap, &cfg.BaseSafeOutputConfig, 1)

pkg/workflow/safe_outputs_cross_repo_config_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -238,6 +238,61 @@ func TestPushToPullRequestBranchConfigTargetRepo(t *testing.T) {
238238
}
239239
}
240240

241+
// TestMergePullRequestConfigTargetRepo verifies that merge-pull-request
242+
// correctly parses target-repo and allowed-repos fields.
243+
func TestMergePullRequestConfigTargetRepo(t *testing.T) {
244+
compiler := NewCompiler()
245+
246+
tests := []struct {
247+
name string
248+
configMap map[string]any
249+
expectedRepo string
250+
expectedRepos []string
251+
expectedToken string
252+
expectedTarget string
253+
}{
254+
{
255+
name: "target, target-repo and allowed-repos configured",
256+
configMap: map[string]any{
257+
"merge-pull-request": map[string]any{
258+
"target": "*",
259+
"target-repo": "githubnext/gh-aw-side-repo",
260+
"allowed-repos": []any{"githubnext/gh-aw-side-repo", "github/docs"},
261+
"github-token": "${{ secrets.TEMP_USER_PAT }}",
262+
},
263+
},
264+
expectedTarget: "*",
265+
expectedRepo: "githubnext/gh-aw-side-repo",
266+
expectedRepos: []string{"githubnext/gh-aw-side-repo", "github/docs"},
267+
expectedToken: "${{ secrets.TEMP_USER_PAT }}",
268+
},
269+
{
270+
name: "no cross-repo config",
271+
configMap: map[string]any{
272+
"merge-pull-request": map[string]any{
273+
"required-labels": []any{"safe-to-merge"},
274+
},
275+
},
276+
expectedTarget: "",
277+
expectedRepo: "",
278+
expectedRepos: nil,
279+
expectedToken: "",
280+
},
281+
}
282+
283+
for _, tt := range tests {
284+
t.Run(tt.name, func(t *testing.T) {
285+
cfg := compiler.parseMergePullRequestConfig(tt.configMap)
286+
287+
require.NotNil(t, cfg, "config should not be nil")
288+
assert.Equal(t, tt.expectedTarget, cfg.Target, "Target mismatch")
289+
assert.Equal(t, tt.expectedRepo, cfg.TargetRepoSlug, "TargetRepoSlug mismatch")
290+
assert.Equal(t, tt.expectedRepos, cfg.AllowedRepos, "AllowedRepos mismatch")
291+
assert.Equal(t, tt.expectedToken, cfg.GitHubToken, "GitHubToken mismatch")
292+
})
293+
}
294+
}
295+
241296
// TestUpdateIssueConfigGitHubToken verifies that update-issue correctly parses the github-token field.
242297
func TestUpdateIssueConfigGitHubToken(t *testing.T) {
243298
compiler := NewCompiler()
@@ -464,6 +519,45 @@ func TestUpdateIssueGitHubTokenInHandlerConfig(t *testing.T) {
464519
assert.Contains(t, allowedRepos, "githubnext/gh-aw-side-repo", "allowed_repos should contain the repo")
465520
}
466521

522+
// TestMergePullRequestCrossRepoInHandlerConfig verifies that target-repo and allowed-repos
523+
// are included in the handler manager config JSON for merge-pull-request.
524+
func TestMergePullRequestCrossRepoInHandlerConfig(t *testing.T) {
525+
compiler := NewCompiler()
526+
527+
workflowData := &WorkflowData{
528+
Name: "Test",
529+
SafeOutputs: &SafeOutputsConfig{
530+
MergePullRequest: &MergePullRequestConfig{
531+
BaseSafeOutputConfig: BaseSafeOutputConfig{
532+
GitHubToken: "${{ secrets.TEMP_USER_PAT }}",
533+
},
534+
SafeOutputTargetConfig: SafeOutputTargetConfig{
535+
Target: "*",
536+
TargetRepoSlug: "githubnext/gh-aw-side-repo",
537+
AllowedRepos: []string{"githubnext/gh-aw-side-repo"},
538+
},
539+
},
540+
},
541+
}
542+
543+
var steps []string
544+
compiler.addHandlerManagerConfigEnvVar(&steps, workflowData)
545+
546+
require.NotEmpty(t, steps)
547+
handlerConfig := extractHandlerConfig(t, strings.Join(steps, ""))
548+
549+
mergePR, ok := handlerConfig["merge_pull_request"]
550+
require.True(t, ok, "merge_pull_request config should be present")
551+
552+
assert.Equal(t, "*", mergePR["target"], "target should be in handler config")
553+
554+
assert.Equal(t, "githubnext/gh-aw-side-repo", mergePR["target-repo"], "target-repo should be in handler config")
555+
556+
allowedRepos, ok := mergePR["allowed_repos"]
557+
require.True(t, ok, "allowed_repos should be present")
558+
assert.Contains(t, allowedRepos, "githubnext/gh-aw-side-repo", "allowed_repos should contain the repo")
559+
}
560+
467561
// TestPushToPullRequestBranchCrossRepoInHandlerConfig verifies that target-repo and allowed-repos
468562
// are included in the handler manager config JSON for push-to-pull-request-branch.
469563
func TestPushToPullRequestBranchCrossRepoInHandlerConfig(t *testing.T) {

pkg/workflow/safe_outputs_handler_registry.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -554,7 +554,10 @@ var handlerRegistry = map[string]handlerBuilder{
554554
c := cfg.MergePullRequest
555555
return newHandlerConfigBuilder().
556556
AddTemplatableInt("max", c.Max).
557+
AddIfNotEmpty("target", c.Target).
557558
AddStringSlice("required_labels", c.RequiredLabels).AddIfNotEmpty("required_title_prefix", c.RequiredTitlePrefix).AddStringSlice("allowed_branches", c.AllowedBranches).
559+
AddIfNotEmpty("target-repo", c.TargetRepoSlug).
560+
AddStringSlice("allowed_repos", c.AllowedRepos).
558561
AddIfNotEmpty("github-token", c.GitHubToken).
559562
AddIfTrue("staged", c.Staged).
560563
Build()

pkg/workflow/safe_outputs_tools_repo_params.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,11 @@ func addRepoParameterIfNeeded(tool map[string]any, toolName string, safeOutputs
7474
hasAllowedRepos = len(config.AllowedRepos) > 0
7575
targetRepoSlug = config.TargetRepoSlug
7676
}
77+
case "merge_pull_request":
78+
if config := safeOutputs.MergePullRequest; config != nil {
79+
hasAllowedRepos = len(config.AllowedRepos) > 0
80+
targetRepoSlug = config.TargetRepoSlug
81+
}
7782
case "add_labels", "remove_labels", "hide_comment", "link_sub_issue", "mark_pull_request_as_ready_for_review",
7883
"add_reviewer", "assign_milestone", "assign_to_agent", "assign_to_user", "unassign_from_user",
7984
"set_issue_type", "set_issue_field":

pkg/workflow/safe_outputs_validation.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,9 @@ func validateSafeOutputsTarget(config *SafeOutputsConfig) error {
129129
if config.PushToPullRequestBranch != nil {
130130
configs = append(configs, targetConfig{"push-to-pull-request-branch", config.PushToPullRequestBranch.Target})
131131
}
132+
if config.MergePullRequest != nil {
133+
configs = append(configs, targetConfig{"merge-pull-request", config.MergePullRequest.Target})
134+
}
132135
// Validate each target field
133136
for _, cfg := range configs {
134137
if err := validateTargetValue(cfg.name, cfg.target); err != nil {

0 commit comments

Comments
 (0)