From ac17ab8bf448eb757408712bea3dedc7ba3fff86 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:02:43 +0000 Subject: [PATCH 01/12] Initial plan From d21bb0040bbbfeadb8fc538a0676a3af2a8234f1 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:11:17 +0000 Subject: [PATCH 02/12] Refactor safe output repo target accessors Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .../safe_outputs_tools_generation_test.go | 15 + .../safe_outputs_tools_repo_params.go | 338 ++++++++++-------- 2 files changed, 204 insertions(+), 149 deletions(-) diff --git a/pkg/workflow/safe_outputs_tools_generation_test.go b/pkg/workflow/safe_outputs_tools_generation_test.go index 3856f9e5514..fbf18e2eb6d 100644 --- a/pkg/workflow/safe_outputs_tools_generation_test.go +++ b/pkg/workflow/safe_outputs_tools_generation_test.go @@ -234,6 +234,21 @@ func TestAddRepoParameterIfNeededClosePullRequestWithAllowedRepos(t *testing.T) assert.Contains(t, repoProp["description"].(string), "org/default-repo", "description should include default repo") } +func TestRepoTargetAccessorsCoverRepoTargetTools(t *testing.T) { + for _, toolName := range []string{ + "create_issue", "create_discussion", "add_comment", "create_pull_request", + "create_pull_request_review_comment", "reply_to_pull_request_review_comment", + "dismiss_pull_request_review", "create_agent_session", "close_issue", "update_issue", + "close_discussion", "update_discussion", "close_pull_request", "update_pull_request", + "merge_pull_request", "add_labels", "remove_labels", "replace_label", "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", + } { + assert.Contains(t, repoTargetAccessors, toolName) + } +} + func TestParseUpdateIssuesConfigWithWildcardTargetRepo(t *testing.T) { compiler := &Compiler{} outputMap := map[string]any{ diff --git a/pkg/workflow/safe_outputs_tools_repo_params.go b/pkg/workflow/safe_outputs_tools_repo_params.go index 57883598ff2..f453b19ec21 100644 --- a/pkg/workflow/safe_outputs_tools_repo_params.go +++ b/pkg/workflow/safe_outputs_tools_repo_params.go @@ -2,6 +2,184 @@ package workflow import "fmt" +type repoTargetConfig struct { + allowedRepos []string + targetRepoSlug string +} + +type repoTargetAccessor func(*SafeOutputsConfig) *repoTargetConfig + +var repoTargetAccessors = map[string]repoTargetAccessor{ + "create_issue": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.CreateIssues; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "create_discussion": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.CreateDiscussions; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "add_comment": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.AddComments; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "create_pull_request": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.CreatePullRequests; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "create_pull_request_review_comment": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.CreatePullRequestReviewComments; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "reply_to_pull_request_review_comment": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.ReplyToPullRequestReviewComment; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "dismiss_pull_request_review": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.DismissPullRequestReview; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "create_agent_session": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.CreateAgentSessions; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "close_issue": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.CloseIssues; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "update_issue": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.UpdateIssues; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "close_discussion": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.CloseDiscussions; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "update_discussion": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.UpdateDiscussions; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "close_pull_request": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.ClosePullRequests; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "update_pull_request": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.UpdatePullRequests; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "merge_pull_request": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.MergePullRequest; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "add_labels": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.AddLabels; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "remove_labels": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.RemoveLabels; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "replace_label": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.ReplaceLabel; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "hide_comment": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.HideComment; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "link_sub_issue": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.LinkSubIssue; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "mark_pull_request_as_ready_for_review": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.MarkPullRequestAsReadyForReview; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "add_reviewer": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.AddReviewer; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "assign_milestone": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.AssignMilestone; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "assign_to_agent": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.AssignToAgent; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "assign_to_user": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.AssignToUser; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "unassign_from_user": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.UnassignFromUser; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "set_issue_type": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.SetIssueType; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, + "set_issue_field": func(config *SafeOutputsConfig) *repoTargetConfig { + if output := config.SetIssueField; output != nil { + return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + } + return nil + }, +} + // addRepoParameterIfNeeded adds a "repo" parameter to the tool's inputSchema // if the safe output configuration has allowed-repos entries or a wildcard "*" target-repo func addRepoParameterIfNeeded(tool map[string]any, toolName string, safeOutputs *SafeOutputsConfig) { @@ -10,155 +188,17 @@ func addRepoParameterIfNeeded(tool map[string]any, toolName string, safeOutputs return } - // Determine if this tool should have a repo parameter based on allowed-repos and target-repo configuration (including wildcard "*") - var hasAllowedRepos bool - var targetRepoSlug string - - switch toolName { - case "create_issue": - if config := safeOutputs.CreateIssues; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "create_discussion": - if config := safeOutputs.CreateDiscussions; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "add_comment": - if config := safeOutputs.AddComments; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "create_pull_request": - if config := safeOutputs.CreatePullRequests; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "create_pull_request_review_comment": - if config := safeOutputs.CreatePullRequestReviewComments; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "reply_to_pull_request_review_comment": - if config := safeOutputs.ReplyToPullRequestReviewComment; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "dismiss_pull_request_review": - if config := safeOutputs.DismissPullRequestReview; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "create_agent_session": - if config := safeOutputs.CreateAgentSessions; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "close_issue", "update_issue": - if config := safeOutputs.CloseIssues; config != nil && toolName == "close_issue" { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } else if config := safeOutputs.UpdateIssues; config != nil && toolName == "update_issue" { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "close_discussion", "update_discussion": - if config := safeOutputs.CloseDiscussions; config != nil && toolName == "close_discussion" { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } else if config := safeOutputs.UpdateDiscussions; config != nil && toolName == "update_discussion" { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "close_pull_request", "update_pull_request": - if config := safeOutputs.ClosePullRequests; config != nil && toolName == "close_pull_request" { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } else if config := safeOutputs.UpdatePullRequests; config != nil && toolName == "update_pull_request" { - 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", "replace_label", "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": - // These use SafeOutputTargetConfig - check the appropriate config - switch toolName { - case "add_labels": - if config := safeOutputs.AddLabels; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "remove_labels": - if config := safeOutputs.RemoveLabels; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "replace_label": - if config := safeOutputs.ReplaceLabel; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "hide_comment": - if config := safeOutputs.HideComment; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "link_sub_issue": - if config := safeOutputs.LinkSubIssue; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "mark_pull_request_as_ready_for_review": - if config := safeOutputs.MarkPullRequestAsReadyForReview; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "add_reviewer": - if config := safeOutputs.AddReviewer; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "assign_milestone": - if config := safeOutputs.AssignMilestone; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "assign_to_agent": - if config := safeOutputs.AssignToAgent; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "assign_to_user": - if config := safeOutputs.AssignToUser; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "unassign_from_user": - if config := safeOutputs.UnassignFromUser; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "set_issue_type": - if config := safeOutputs.SetIssueType; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - case "set_issue_field": - if config := safeOutputs.SetIssueField; config != nil { - hasAllowedRepos = len(config.AllowedRepos) > 0 - targetRepoSlug = config.TargetRepoSlug - } - } + accessor, ok := repoTargetAccessors[toolName] + if !ok { + return + } + targetConfig := accessor(safeOutputs) + if targetConfig == nil { + return } // Only add repo parameter if allowed-repos has entries or target-repo is wildcard ("*") - if !hasAllowedRepos && targetRepoSlug != "*" { + if len(targetConfig.allowedRepos) == 0 && targetConfig.targetRepoSlug != "*" { safeOutputsConfigLog.Printf("Skipping repo parameter for tool %s: no allowed-repos and target-repo is not wildcard", toolName) return } @@ -176,10 +216,10 @@ func addRepoParameterIfNeeded(tool map[string]any, toolName string, safeOutputs // Build repo parameter description var repoDescription string - if targetRepoSlug == "*" { + if targetConfig.targetRepoSlug == "*" { repoDescription = "Target repository for this operation in 'owner/repo' format. Any repository can be targeted." - } else if targetRepoSlug != "" { - repoDescription = fmt.Sprintf("Target repository for this operation in 'owner/repo' format. Default is %q. Must be the target-repo or in the allowed-repos list.", targetRepoSlug) + } else if targetConfig.targetRepoSlug != "" { + repoDescription = fmt.Sprintf("Target repository for this operation in 'owner/repo' format. Default is %q. Must be the target-repo or in the allowed-repos list.", targetConfig.targetRepoSlug) } else { repoDescription = "Target repository for this operation in 'owner/repo' format. Must be the target-repo or in the allowed-repos list." } From 773023e0510c1f2143e6040064b40c2ffc616df6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:13:30 +0000 Subject: [PATCH 03/12] Strengthen repo target registry coverage Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/safe_outputs_tools_generation_test.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/workflow/safe_outputs_tools_generation_test.go b/pkg/workflow/safe_outputs_tools_generation_test.go index fbf18e2eb6d..713d9fa12ae 100644 --- a/pkg/workflow/safe_outputs_tools_generation_test.go +++ b/pkg/workflow/safe_outputs_tools_generation_test.go @@ -235,7 +235,7 @@ func TestAddRepoParameterIfNeededClosePullRequestWithAllowedRepos(t *testing.T) } func TestRepoTargetAccessorsCoverRepoTargetTools(t *testing.T) { - for _, toolName := range []string{ + expectedTools := []string{ "create_issue", "create_discussion", "add_comment", "create_pull_request", "create_pull_request_review_comment", "reply_to_pull_request_review_comment", "dismiss_pull_request_review", "create_agent_session", "close_issue", "update_issue", @@ -244,7 +244,9 @@ func TestRepoTargetAccessorsCoverRepoTargetTools(t *testing.T) { "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", - } { + } + assert.Len(t, repoTargetAccessors, len(expectedTools)) + for _, toolName := range expectedTools { assert.Contains(t, repoTargetAccessors, toolName) } } From 987cc12677ed6fca8f569739fc3f58766e228237 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Wed, 19 Aug 2026 10:10:51 +0000 Subject: [PATCH 04/12] Add draft ADR for registry-based repo target accessor pattern (PR #53838) Co-Authored-By: Claude Sonnet 4.6 --- ...stry-based-repo-target-accessor-pattern.md | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 docs/adr/53838-registry-based-repo-target-accessor-pattern.md diff --git a/docs/adr/53838-registry-based-repo-target-accessor-pattern.md b/docs/adr/53838-registry-based-repo-target-accessor-pattern.md new file mode 100644 index 00000000000..28281e494db --- /dev/null +++ b/docs/adr/53838-registry-based-repo-target-accessor-pattern.md @@ -0,0 +1,44 @@ +# ADR-53838: Registry-Based Repo Target Accessor Pattern + +**Date**: 2026-08-19 +**Status**: Draft +**Deciders**: Unknown + +--- + +### Context + +`pkg/workflow/safe_outputs_tools_repo_params.go` contained a single function (`addRepoParameterIfNeeded`) with a 28-case `switch` statement that duplicated the same `AllowedRepos` / `TargetRepoSlug` extraction pattern for every safe-output tool. Adding a new tool required inserting identical boilerplate into multiple branches of that switch with no compile-time or test-time guard against omissions, making the code easy to implement inconsistently. + +### Decision + +We will replace the monolithic switch with a `repoTargetAccessors` registry: a package-level `map[string]repoTargetAccessor` that maps each tool name to a one-liner function returning a `*repoTargetConfig`. `addRepoParameterIfNeeded` performs a single map lookup and delegates to the accessor, reducing the function from ~160 lines to ~10. A dedicated test (`TestRepoTargetAccessorsCoverRepoTargetTools`) asserts that the registry contains exactly the expected set of tool names, acting as a coverage guard for future additions. + +### Alternatives Considered + +#### Alternative 1: Keep the switch with a coverage test + +Add the `TestRepoTargetAccessorsCoverRepoTargetTools`-style test against the existing switch (e.g., by enumerating all `case` labels via reflection or a maintained list), leaving the implementation unchanged. This would surface omissions at test time but would not reduce the per-tool duplication or shorten `addRepoParameterIfNeeded`. + +#### Alternative 2: Interface-based tool registry + +Define a `RepoTargetProvider` interface and have each tool's config struct implement it, removing the accessor functions entirely. This would eliminate all per-tool boilerplate in the registry at the cost of touching every config struct and coupling the config layer to the repo-parameter generation logic. + +### Consequences + +#### Positive +- Adding a new tool now requires a single, uniform registry entry instead of a switch case in an already-large function. +- The coverage test fails fast if a tool is added to the tool list but omitted from the registry. +- `addRepoParameterIfNeeded` is reduced from ~160 lines to ~10, making it easy to understand at a glance. + +#### Negative +- Each of the 28 registry entries still contains near-identical boilerplate (`if output := config.X; output != nil { return &repoTargetConfig{...} }`), so per-entry verbosity is unchanged. +- The registry is a package-level `var`, which is initialized at program startup; errors in registry construction surface only at runtime, not at compile time. + +#### Neutral +- The `repoTargetConfig` struct is now the canonical representation of per-tool repo-target state, which is a minor new abstraction other callers could reuse. +- The test coverage guard encodes the full list of repo-target tools in the test file; this list must be kept in sync when tools are added or removed. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* From ec1120a74c4ca5cd414ff73f15928e6805913ffb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 19 Aug 2026 11:02:05 +0000 Subject: [PATCH 05/12] Derive repo target accessors from handler metadata Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .../safe_outputs_tools_generation_test.go | 39 +-- .../safe_outputs_tools_repo_params.go | 226 +++++------------- 2 files changed, 83 insertions(+), 182 deletions(-) diff --git a/pkg/workflow/safe_outputs_tools_generation_test.go b/pkg/workflow/safe_outputs_tools_generation_test.go index 713d9fa12ae..d5cf62a6a46 100644 --- a/pkg/workflow/safe_outputs_tools_generation_test.go +++ b/pkg/workflow/safe_outputs_tools_generation_test.go @@ -5,6 +5,7 @@ package workflow import ( "os" "path/filepath" + "reflect" "testing" "github.com/stretchr/testify/assert" @@ -234,20 +235,30 @@ func TestAddRepoParameterIfNeededClosePullRequestWithAllowedRepos(t *testing.T) assert.Contains(t, repoProp["description"].(string), "org/default-repo", "description should include default repo") } -func TestRepoTargetAccessorsCoverRepoTargetTools(t *testing.T) { - expectedTools := []string{ - "create_issue", "create_discussion", "add_comment", "create_pull_request", - "create_pull_request_review_comment", "reply_to_pull_request_review_comment", - "dismiss_pull_request_review", "create_agent_session", "close_issue", "update_issue", - "close_discussion", "update_discussion", "close_pull_request", "update_pull_request", - "merge_pull_request", "add_labels", "remove_labels", "replace_label", "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", - } - assert.Len(t, repoTargetAccessors, len(expectedTools)) - for _, toolName := range expectedTools { - assert.Contains(t, repoTargetAccessors, toolName) +func TestRepoTargetAccessorsMatchHandlerMetadata(t *testing.T) { + accessors := getRepoTargetAccessors() + for _, handler := range safeOutputHandlers { + if !isRepoTargetHandler(handler) { + continue + } + + t.Run(handler.ToolName, func(t *testing.T) { + config := &SafeOutputsConfig{} + output := reflect.ValueOf(config).Elem().FieldByName(handler.StructField) + require.True(t, output.IsValid(), "handler struct field must exist") + + output.Set(reflect.New(output.Type().Elem())) + output = output.Elem() + output.FieldByName("AllowedRepos").Set(reflect.ValueOf([]string{handler.ToolName + "/allowed"})) + output.FieldByName("TargetRepoSlug").SetString(handler.ToolName + "/target") + + accessor, ok := accessors[handler.ToolName] + require.True(t, ok, "repo target handler must have an accessor") + targetConfig := accessor(config) + require.NotNil(t, targetConfig) + assert.Equal(t, []string{handler.ToolName + "/allowed"}, targetConfig.allowedRepos) + assert.Equal(t, handler.ToolName+"/target", targetConfig.targetRepoSlug) + }) } } diff --git a/pkg/workflow/safe_outputs_tools_repo_params.go b/pkg/workflow/safe_outputs_tools_repo_params.go index f453b19ec21..eba3905e849 100644 --- a/pkg/workflow/safe_outputs_tools_repo_params.go +++ b/pkg/workflow/safe_outputs_tools_repo_params.go @@ -1,6 +1,10 @@ package workflow -import "fmt" +import ( + "fmt" + "reflect" + "sync" +) type repoTargetConfig struct { allowedRepos []string @@ -9,175 +13,61 @@ type repoTargetConfig struct { type repoTargetAccessor func(*SafeOutputsConfig) *repoTargetConfig -var repoTargetAccessors = map[string]repoTargetAccessor{ - "create_issue": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.CreateIssues; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "create_discussion": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.CreateDiscussions; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "add_comment": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.AddComments; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "create_pull_request": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.CreatePullRequests; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "create_pull_request_review_comment": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.CreatePullRequestReviewComments; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "reply_to_pull_request_review_comment": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.ReplyToPullRequestReviewComment; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "dismiss_pull_request_review": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.DismissPullRequestReview; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "create_agent_session": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.CreateAgentSessions; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "close_issue": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.CloseIssues; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "update_issue": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.UpdateIssues; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "close_discussion": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.CloseDiscussions; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "update_discussion": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.UpdateDiscussions; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "close_pull_request": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.ClosePullRequests; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "update_pull_request": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.UpdatePullRequests; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "merge_pull_request": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.MergePullRequest; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "add_labels": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.AddLabels; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "remove_labels": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.RemoveLabels; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "replace_label": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.ReplaceLabel; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "hide_comment": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.HideComment; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "link_sub_issue": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.LinkSubIssue; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "mark_pull_request_as_ready_for_review": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.MarkPullRequestAsReadyForReview; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "add_reviewer": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.AddReviewer; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "assign_milestone": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.AssignMilestone; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "assign_to_agent": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.AssignToAgent; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "assign_to_user": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.AssignToUser; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} - } - return nil - }, - "unassign_from_user": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.UnassignFromUser; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} +var ( + repoTargetAccessors map[string]repoTargetAccessor + repoTargetAccessorsOnce sync.Once +) + +func getRepoTargetAccessors() map[string]repoTargetAccessor { + repoTargetAccessorsOnce.Do(func() { + repoTargetAccessors = buildRepoTargetAccessors() + }) + return repoTargetAccessors +} + +func buildRepoTargetAccessors() map[string]repoTargetAccessor { + accessors := make(map[string]repoTargetAccessor) + for _, handler := range safeOutputHandlers { + if !isRepoTargetHandler(handler) { + continue } - return nil - }, - "set_issue_type": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.SetIssueType; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + + accessors[handler.ToolName] = newRepoTargetAccessor(handler.StructField) + } + return accessors +} + +func isRepoTargetHandler(handler safeOutputHandlerDescriptor) bool { + if handler.NewConfig == nil { + return false + } + configType := reflect.TypeOf(handler.NewConfig()) + if configType == nil { + return false + } + outputField, hasOutputField := reflect.TypeFor[SafeOutputsConfig]().FieldByName(handler.StructField) + if !hasOutputField || outputField.Type != configType { + return false + } + allowedRepos, hasAllowedRepos := configType.Elem().FieldByName("AllowedRepos") + targetRepoSlug, hasTargetRepoSlug := configType.Elem().FieldByName("TargetRepoSlug") + return hasAllowedRepos && hasTargetRepoSlug && + allowedRepos.Type == reflect.TypeFor[[]string]() && + targetRepoSlug.Type.Kind() == reflect.String +} + +func newRepoTargetAccessor(structField string) repoTargetAccessor { + return func(config *SafeOutputsConfig) *repoTargetConfig { + output := reflect.ValueOf(config).Elem().FieldByName(structField) + if !output.IsValid() || output.IsNil() { + return nil } - return nil - }, - "set_issue_field": func(config *SafeOutputsConfig) *repoTargetConfig { - if output := config.SetIssueField; output != nil { - return &repoTargetConfig{output.AllowedRepos, output.TargetRepoSlug} + output = output.Elem() + return &repoTargetConfig{ + allowedRepos: output.FieldByName("AllowedRepos").Interface().([]string), + targetRepoSlug: output.FieldByName("TargetRepoSlug").String(), } - return nil - }, + } } // addRepoParameterIfNeeded adds a "repo" parameter to the tool's inputSchema @@ -188,7 +78,7 @@ func addRepoParameterIfNeeded(tool map[string]any, toolName string, safeOutputs return } - accessor, ok := repoTargetAccessors[toolName] + accessor, ok := getRepoTargetAccessors()[toolName] if !ok { return } From 4161388a5c61af4314b96d6584b00b07fe588604 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Aug 2026 04:57:11 +0000 Subject: [PATCH 06/12] Investigate reported CI failure Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- .github/skills/agentic-workflows/SKILL.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index d0c9af823a8..3af44da06f2 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -41,6 +41,7 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/github-mcp-server-pagination.md` - `.github/aw/github-mcp-server.md` - `.github/aw/instructions.md` +- `.github/aw/jobs.md` - `.github/aw/linter-workflows.md` - `.github/aw/llms.md` - `.github/aw/loop.md` From dc4b0d50266de7aaaf34b3791b9d43284168e347 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Aug 2026 05:04:04 +0000 Subject: [PATCH 07/12] Fix governance formal test lint assertions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/intent/governance_formal_test.go | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/pkg/intent/governance_formal_test.go b/pkg/intent/governance_formal_test.go index 7ae6c8a5499..1e942af2360 100644 --- a/pkg/intent/governance_formal_test.go +++ b/pkg/intent/governance_formal_test.go @@ -3,7 +3,6 @@ package intent_test import ( - "errors" "testing" "github.com/stretchr/testify/assert" @@ -116,7 +115,7 @@ func TestAuthorizeTool_DeniedWins(t *testing.T) { } err := intent.Authorizer{}.AuthorizeTool(policy, "write") require.Error(t, err, "P9: denied tool must be rejected") - assert.True(t, errors.Is(err, intent.ErrToolDenied), + assert.ErrorIs(t, err, intent.ErrToolDenied, "P9: denied tool must return ErrToolDenied") } @@ -126,7 +125,7 @@ func TestAuthorizeTool_AllowlistGate(t *testing.T) { policy := intent.ExecutionPolicy{AllowedTools: []string{"read"}} err := intent.Authorizer{}.AuthorizeTool(policy, "exec") require.Error(t, err, "P10: tool absent from a restricted allow list must be rejected") - assert.True(t, errors.Is(err, intent.ErrToolNotAllowed), + require.ErrorIs(t, err, intent.ErrToolNotAllowed, "P10: tool absent from allow list must return ErrToolNotAllowed") require.NoError(t, intent.Authorizer{}.AuthorizeTool(policy, "read"), @@ -145,7 +144,7 @@ func TestAuthorizeTool_UnrestrictedWhenAllowedToolsNil(t *testing.T) { err := intent.Authorizer{}.AuthorizeTool(policy, "exec") require.Error(t, err, "P11: an explicit deny must still be rejected even when unrestricted") - assert.True(t, errors.Is(err, intent.ErrToolDenied)) + assert.ErrorIs(t, err, intent.ErrToolDenied) } // TestAuthorizeTool_EmptyAllowedToolsDeniesAll (P12 — AuthorizeToolEmptyDenyAll) @@ -154,7 +153,7 @@ func TestAuthorizeTool_EmptyAllowedToolsDeniesAll(t *testing.T) { policy := intent.ExecutionPolicy{AllowedTools: []string{}} err := intent.Authorizer{}.AuthorizeTool(policy, "read") require.Error(t, err, "P12: non-nil empty AllowedTools must deny all tools") - assert.True(t, errors.Is(err, intent.ErrToolNotAllowed)) + assert.ErrorIs(t, err, intent.ErrToolNotAllowed) } // TestSafestDefaultPolicy_FailClosedForIndeterminateStatus (P13 — SafestDefaultFailClosed) From 903c288fb08a985740593997afe264141e2aaa95 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Aug 2026 07:28:18 +0000 Subject: [PATCH 08/12] Restore generated workflow skill source Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .github/skills/agentic-workflows/SKILL.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 3af44da06f2..d0c9af823a8 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -41,7 +41,6 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/github-mcp-server-pagination.md` - `.github/aw/github-mcp-server.md` - `.github/aw/instructions.md` -- `.github/aw/jobs.md` - `.github/aw/linter-workflows.md` - `.github/aw/llms.md` - `.github/aw/loop.md` From 4c4f39a2faadf12e97727dbdf30b90cbed5bb09c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Aug 2026 09:30:07 +0000 Subject: [PATCH 09/12] chore: begin maintainer finish pass Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .github/skills/agentic-workflows/SKILL.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index d0c9af823a8..3af44da06f2 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -41,6 +41,7 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/github-mcp-server-pagination.md` - `.github/aw/github-mcp-server.md` - `.github/aw/instructions.md` +- `.github/aw/jobs.md` - `.github/aw/linter-workflows.md` - `.github/aw/llms.md` - `.github/aw/loop.md` From 67cd33d0195fd4e39948753cb7409ed7dc06157a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:40:38 +0000 Subject: [PATCH 10/12] Verify latest PR readiness Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .github/skills/agentic-workflows/SKILL.md | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 3af44da06f2..4109b7d1e56 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -39,6 +39,7 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/experiments.md` - `.github/aw/github-agentic-workflows.md` - `.github/aw/github-mcp-server-pagination.md` +- `.github/aw/github-mcp-server-tools.md` - `.github/aw/github-mcp-server.md` - `.github/aw/instructions.md` - `.github/aw/jobs.md` From aa9257fc490b2d3da1ae54b48d3bb85fc7814e38 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:41:29 +0000 Subject: [PATCH 11/12] Revert unrelated skill-list change Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- .github/skills/agentic-workflows/SKILL.md | 1 - 1 file changed, 1 deletion(-) diff --git a/.github/skills/agentic-workflows/SKILL.md b/.github/skills/agentic-workflows/SKILL.md index 4109b7d1e56..3af44da06f2 100644 --- a/.github/skills/agentic-workflows/SKILL.md +++ b/.github/skills/agentic-workflows/SKILL.md @@ -39,7 +39,6 @@ Load these files from `github/gh-aw` (they are not available locally). - `.github/aw/experiments.md` - `.github/aw/github-agentic-workflows.md` - `.github/aw/github-mcp-server-pagination.md` -- `.github/aw/github-mcp-server-tools.md` - `.github/aw/github-mcp-server.md` - `.github/aw/instructions.md` - `.github/aw/jobs.md` From 699abbc05bf58556e0cb5e980f3a120718176506 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 20 Aug 2026 16:52:37 +0000 Subject: [PATCH 12/12] Guard reflected repo target assertion Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/safe_outputs_tools_repo_params.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/pkg/workflow/safe_outputs_tools_repo_params.go b/pkg/workflow/safe_outputs_tools_repo_params.go index eba3905e849..988028ce8c2 100644 --- a/pkg/workflow/safe_outputs_tools_repo_params.go +++ b/pkg/workflow/safe_outputs_tools_repo_params.go @@ -63,8 +63,12 @@ func newRepoTargetAccessor(structField string) repoTargetAccessor { return nil } output = output.Elem() + allowedRepos, ok := output.FieldByName("AllowedRepos").Interface().([]string) + if !ok { + return nil + } return &repoTargetConfig{ - allowedRepos: output.FieldByName("AllowedRepos").Interface().([]string), + allowedRepos: allowedRepos, targetRepoSlug: output.FieldByName("TargetRepoSlug").String(), } }