Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/grumpy-reviewer.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/mattpocock-skills-reviewer.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/pr-code-quality-reviewer.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/pr-nitpick-reviewer.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/pr-triage-agent.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/refiner.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/security-review.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/smoke-claude.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/smoke-copilot-aoai-apikey.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/smoke-copilot-aoai-entra.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/smoke-copilot-arm.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/smoke-copilot.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion .github/workflows/test-quality-sentinel.lock.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

15 changes: 15 additions & 0 deletions pkg/parser/schema_safe_outputs_target_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,21 @@ func TestMainWorkflowSchema_SafeOutputsTargetProperties(t *testing.T) {
},
},
},
{
name: "merge-pull-request with target, target-repo, and samples",
safeOutputs: map[string]any{
"merge-pull-request": map[string]any{
"target": "*",
"target-repo": "github/github",
"allowed-repos": []any{"github/docs"},
"samples": []any{
map[string]any{
"merge_method": "squash",
},
},
},
},
},
{
name: "dispatch-workflow with target-repo and allowed-repos",
safeOutputs: map[string]any{
Expand Down
31 changes: 31 additions & 0 deletions pkg/parser/schemas/main_workflow_schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -8499,6 +8499,21 @@
},
"description": "Glob patterns for allowed source branch names (pull request head ref). The PR's branch must match at least one pattern."
},
"target": {
"type": "string",
"description": "Target for merging: 'triggering' (default, current PR), or '*' (any PR with pull_request_number field)"
Comment thread
dsyme marked this conversation as resolved.
},
"target-repo": {
"type": "string",
"description": "Target repository in format 'owner/repo' for cross-repository operations. Takes precedence over trial target repo settings."
},
"allowed-repos": {
"type": "array",
"items": {
"type": "string"
},
"description": "List of additional repositories in format 'owner/repo' that pull requests can be merged in. The target repository is always implicitly allowed."
},
"github-token": {
"$ref": "#/$defs/github_token",
"description": "GitHub token to use for this specific output type. Overrides global github-token if specified."
Expand All @@ -8508,6 +8523,22 @@
"description": "If true, evaluate merge gates and emit preview results without executing the merge API call.",
"examples": [true, false]
},
"samples": {
"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.",
"oneOf": [
{
"type": "array",
"items": {
"type": "object",
"additionalProperties": true
}
},
{
"type": "object",
"additionalProperties": true
}
]
},
"required-title-prefix": {
"type": "string",
"description": "The target item's title must start with this prefix for this operation to proceed"
Expand Down
13 changes: 8 additions & 5 deletions pkg/workflow/merge_pull_request.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,12 @@ var mergePullRequestLog = logger.New("workflow:merge_pull_request")

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

// parseMergePullRequestConfig handles merge-pull-request configuration.
Expand All @@ -28,6 +29,8 @@ func (c *Compiler) parseMergePullRequestConfig(outputMap map[string]any) *MergeP
// Deprecated: allowed-labels is migrated to required-labels by the codemod
cfg.RequiredLabels = ParseStringArrayFromConfig(configMap, "allowed-labels", mergePullRequestLog)
}
targetConfig, _ := ParseTargetConfig(configMap)
cfg.SafeOutputTargetConfig = targetConfig
Comment on lines +32 to +33
cfg.AllowedBranches = ParseStringArrayFromConfig(configMap, "allowed-branches", mergePullRequestLog)
cfg.RequiredTitlePrefix = extractStringFromMap(configMap, "required-title-prefix", mergePullRequestLog)
c.parseBaseSafeOutputConfig(configMap, &cfg.BaseSafeOutputConfig, 1)
Expand Down
91 changes: 91 additions & 0 deletions pkg/workflow/safe_outputs_cross_repo_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,61 @@ func TestPushToPullRequestBranchConfigTargetRepo(t *testing.T) {
}
}

// TestMergePullRequestConfigTargetRepo verifies that merge-pull-request
// correctly parses target-repo and allowed-repos fields.
func TestMergePullRequestConfigTargetRepo(t *testing.T) {
compiler := NewCompiler()

tests := []struct {
name string
configMap map[string]any
expectedRepo string
expectedRepos []string
expectedToken string
expectedTarget string
}{
{
name: "target, target-repo and allowed-repos configured",
configMap: map[string]any{
"merge-pull-request": map[string]any{
"target": "*",
"target-repo": "githubnext/gh-aw-side-repo",
"allowed-repos": []any{"githubnext/gh-aw-side-repo"},
Comment thread
dsyme marked this conversation as resolved.
Outdated
Comment thread
dsyme marked this conversation as resolved.
Outdated
"github-token": "${{ secrets.TEMP_USER_PAT }}",
},
},
expectedTarget: "*",
expectedRepo: "githubnext/gh-aw-side-repo",
expectedRepos: []string{"githubnext/gh-aw-side-repo"},
expectedToken: "${{ secrets.TEMP_USER_PAT }}",
},
{
name: "no cross-repo config",
configMap: map[string]any{
"merge-pull-request": map[string]any{
"required-labels": []any{"safe-to-merge"},
},
},
expectedTarget: "",
expectedRepo: "",
expectedRepos: nil,
expectedToken: "",
},
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
cfg := compiler.parseMergePullRequestConfig(tt.configMap)

require.NotNil(t, cfg, "config should not be nil")
assert.Equal(t, tt.expectedTarget, cfg.Target, "Target mismatch")
assert.Equal(t, tt.expectedRepo, cfg.TargetRepoSlug, "TargetRepoSlug mismatch")
assert.Equal(t, tt.expectedRepos, cfg.AllowedRepos, "AllowedRepos mismatch")
assert.Equal(t, tt.expectedToken, cfg.GitHubToken, "GitHubToken mismatch")
})
}
}

// TestUpdateIssueConfigGitHubToken verifies that update-issue correctly parses the github-token field.
func TestUpdateIssueConfigGitHubToken(t *testing.T) {
compiler := NewCompiler()
Expand Down Expand Up @@ -464,6 +519,42 @@ func TestUpdateIssueGitHubTokenInHandlerConfig(t *testing.T) {
assert.Contains(t, allowedRepos, "githubnext/gh-aw-side-repo", "allowed_repos should contain the repo")
}

// TestMergePullRequestCrossRepoInHandlerConfig verifies that target-repo and allowed-repos
// are included in the handler manager config JSON for merge-pull-request.
func TestMergePullRequestCrossRepoInHandlerConfig(t *testing.T) {
compiler := NewCompiler()

workflowData := &WorkflowData{
Name: "Test",
SafeOutputs: &SafeOutputsConfig{
MergePullRequest: &MergePullRequestConfig{
BaseSafeOutputConfig: BaseSafeOutputConfig{
GitHubToken: "${{ secrets.TEMP_USER_PAT }}",
},
SafeOutputTargetConfig: SafeOutputTargetConfig{
TargetRepoSlug: "githubnext/gh-aw-side-repo",
AllowedRepos: []string{"githubnext/gh-aw-side-repo"},
Comment thread
dsyme marked this conversation as resolved.
},
},
},
}

var steps []string
compiler.addHandlerManagerConfigEnvVar(&steps, workflowData)

require.NotEmpty(t, steps)
handlerConfig := extractHandlerConfig(t, strings.Join(steps, ""))

mergePR, ok := handlerConfig["merge_pull_request"]
require.True(t, ok, "merge_pull_request config should be present")

assert.Equal(t, "githubnext/gh-aw-side-repo", mergePR["target-repo"], "target-repo should be in handler config")

allowedRepos, ok := mergePR["allowed_repos"]
require.True(t, ok, "allowed_repos should be present")
assert.Contains(t, allowedRepos, "githubnext/gh-aw-side-repo", "allowed_repos should contain the repo")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test coverage gap: AddIfNotEmpty("target", c.Target) is a new addition in this diff, but no test ever sets Target or asserts that "target" appears in the emitted handler config — a rename or omission of that key would pass undetected.

💡 Suggested fix

Add a sub-case that sets Target: "*" and asserts the key appears in the output:

SafeOutputTargetConfig: SafeOutputTargetConfig{
    Target:         "*",
    TargetRepoSlug: "githubnext/gh-aw-side-repo",
    AllowedRepos:   []string{"githubnext/gh-aw-side-repo"},
},
...
assert.Equal(t, "*", mergePR["target"], "target should be in handler config")

Without this, the AddIfNotEmpty("target", c.Target) line added in safe_outputs_handler_registry.go has zero handler-config test coverage.

}

// TestPushToPullRequestBranchCrossRepoInHandlerConfig verifies that target-repo and allowed-repos
// are included in the handler manager config JSON for push-to-pull-request-branch.
func TestPushToPullRequestBranchCrossRepoInHandlerConfig(t *testing.T) {
Expand Down
3 changes: 3 additions & 0 deletions pkg/workflow/safe_outputs_handler_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,10 @@ var handlerRegistry = map[string]handlerBuilder{
c := cfg.MergePullRequest
return newHandlerConfigBuilder().
AddTemplatableInt("max", c.Max).
AddIfNotEmpty("target", c.Target).
AddStringSlice("required_labels", c.RequiredLabels).AddIfNotEmpty("required_title_prefix", c.RequiredTitlePrefix).AddStringSlice("allowed_branches", c.AllowedBranches).
AddIfNotEmpty("target-repo", c.TargetRepoSlug).
AddStringSlice("allowed_repos", c.AllowedRepos).
Comment on lines 556 to +560
AddIfNotEmpty("github-token", c.GitHubToken).
AddIfTrue("staged", c.Staged).
Build()
Expand Down
Loading