Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
94 changes: 94 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", "github/docs"},
"github-token": "${{ secrets.TEMP_USER_PAT }}",
},
},
expectedTarget: "*",
expectedRepo: "githubnext/gh-aw-side-repo",
expectedRepos: []string{"githubnext/gh-aw-side-repo", "github/docs"},
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,45 @@ 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{
Target: "*",
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, "*", mergePR["target"], "target should be in handler config")

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
5 changes: 5 additions & 0 deletions pkg/workflow/safe_outputs_tools_repo_params.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ func addRepoParameterIfNeeded(tool map[string]any, toolName string, safeOutputs
hasAllowedRepos = len(config.AllowedRepos) > 0
targetRepoSlug = config.TargetRepoSlug
}
case "merge_pull_request":
if config := safeOutputs.MergePullRequest; config != nil {
hasAllowedRepos = len(config.AllowedRepos) > 0
targetRepoSlug = config.TargetRepoSlug
}
case "add_labels", "remove_labels", "hide_comment", "link_sub_issue", "mark_pull_request_as_ready_for_review",
"add_reviewer", "assign_milestone", "assign_to_agent", "assign_to_user", "unassign_from_user",
"set_issue_type", "set_issue_field":
Expand Down
3 changes: 3 additions & 0 deletions pkg/workflow/safe_outputs_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,9 @@ func validateSafeOutputsTarget(config *SafeOutputsConfig) error {
if config.PushToPullRequestBranch != nil {
configs = append(configs, targetConfig{"push-to-pull-request-branch", config.PushToPullRequestBranch.Target})
}
if config.MergePullRequest != nil {
configs = append(configs, targetConfig{"merge-pull-request", config.MergePullRequest.Target})
}
// Validate each target field
for _, cfg := range configs {
if err := validateTargetValue(cfg.name, cfg.target); err != nil {
Expand Down
Loading