Refactor safe-output repo target accessors

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.*

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{

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."
 	}