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.* diff --git a/pkg/workflow/safe_outputs_tools_generation_test.go b/pkg/workflow/safe_outputs_tools_generation_test.go index 3856f9e5514..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,6 +235,33 @@ func TestAddRepoParameterIfNeededClosePullRequestWithAllowedRepos(t *testing.T) assert.Contains(t, repoProp["description"].(string), "org/default-repo", "description should include default repo") } +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) + }) + } +} + 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..988028ce8c2 100644 --- a/pkg/workflow/safe_outputs_tools_repo_params.go +++ b/pkg/workflow/safe_outputs_tools_repo_params.go @@ -1,6 +1,78 @@ package workflow -import "fmt" +import ( + "fmt" + "reflect" + "sync" +) + +type repoTargetConfig struct { + allowedRepos []string + targetRepoSlug string +} + +type repoTargetAccessor func(*SafeOutputsConfig) *repoTargetConfig + +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 + } + + 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 + } + output = output.Elem() + allowedRepos, ok := output.FieldByName("AllowedRepos").Interface().([]string) + if !ok { + return nil + } + return &repoTargetConfig{ + allowedRepos: allowedRepos, + targetRepoSlug: output.FieldByName("TargetRepoSlug").String(), + } + } +} // addRepoParameterIfNeeded adds a "repo" parameter to the tool's inputSchema // if the safe output configuration has allowed-repos entries or a wildcard "*" target-repo @@ -10,155 +82,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 := getRepoTargetAccessors()[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 +110,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." }