Refactor safe-output dispatch logic to use a canonical handler descriptor table

docs/adr/32489-canonical-descriptor-table-for-safe-output-dispatch.md

@@ -0,0 +1,86 @@
+# ADR-32489: Canonical Descriptor Table for Safe-Output Dispatch
+
+**Date**: 2026-05-15
+**Status**: Draft
+**Deciders**: pelikhan, Copilot
+
+---
+
+## Part 1 — Narrative (Human-Friendly)
+
+### Context
+
+Safe-output handler behavior in `pkg/workflow` was spread across several parallel switch/if tables: `hasSafeOutputType` performed key-to-field checks, `SafeOutputsConfigFromKeys` constructed concrete handler config structs via a large switch, `mergeSafeOutputConfig` enumerated per-handler merge logic, and `ComputePermissionsForSafeOutputs` repeated per-handler permission derivation branches. Every new handler required a coordinated multi-file edit, and the parallel tables could drift independently — most notably in alias handling, permission derivation, and merge semantics — without any compile-time check catching the divergence. The result was high cognitive load when adding or auditing a handler, and a real risk of subtle bugs when one of the tables fell out of sync.
+
+### Decision
+
+We will introduce a single canonical `safeOutputHandlers` descriptor list in `pkg/workflow/safe_output_handlers.go`, where each entry carries the handler's `Key`, optional `Aliases`, `StructField` name on `SafeOutputsConfig`, `ToolName`, `Builtin` flag, a `NewConfig` constructor, and a `PermissionBuilder`. The dispatch sites (`hasSafeOutputType`, `SafeOutputsConfigFromKeys`, `mergeSafeOutputConfig`, `ComputePermissionsForSafeOutputs`, and the `safeOutputFieldMapping`) are routed through this descriptor table using shared lookup and reflection helpers, so adding or modifying a handler becomes a single-entry change in the descriptor list. Special-case semantics that don't fit the generic shape (e.g., the protected-files set-merge, auto-default override behavior, and the nil-check fast paths on `hasAnySafeOutputEnabled` / `hasNonBuiltinSafeOutputsEnabled`) are preserved explicitly rather than forced into the generic path.
+
+### Alternatives Considered
+
+#### Alternative 1: Keep the Parallel Switch/If Tables
+
+The simplest option is to leave the existing parallel dispatch tables in place and rely on code review to keep them in sync. This was rejected because the number of handlers and the number of dispatch concerns (existence check, construction, merge, permission derivation, tool name mapping, alias resolution) make manual synchronization fragile, and the parallel tables provide no compile-time guarantee that all dispatch concerns have been considered for a new handler.
+
+#### Alternative 2: Code-Generate the Dispatch Tables from a Schema
+
+The descriptor information could have been expressed as a YAML/JSON schema and code-generated into Go at build time, eliminating reflection entirely. This was rejected as overkill for the current handler count: it introduces a build-time generator, a schema-to-Go pipeline, and review overhead for generated files, in exchange for marginal runtime gains versus a runtime descriptor list with localized reflection.
+
+#### Alternative 3: Force All Handlers Through a Single Generic Path
+
+The descriptor could have been pushed further so that *every* dispatch concern — including special cases like protected-files set merging and auto-default overrides — flowed through a single generic mechanism via extra descriptor fields or strategy hooks. This was rejected because the special-case semantics are genuinely irregular; encoding them as descriptor extensions would have moved the complexity into the descriptor type rather than removing it, and would have obscured the intent of those special cases.
+
+### Consequences
+
+#### Positive
+- Adding a new safe-output handler becomes a single-entry change in `safeOutputHandlers` plus the corresponding `SafeOutputsConfig` field, instead of edits to four or five parallel tables.
+- Alias resolution, permission derivation, and tool-name mapping are guaranteed consistent across dispatch sites because they read from the same descriptor.
+- `safeOutputFieldMapping` is derived from the descriptor table rather than maintained as a separate constant, removing one drift surface.
+
+#### Negative
+- Generic dispatch paths now use reflection (`reflect`) to read and write `SafeOutputsConfig` fields, introducing a small runtime cost and a class of errors (typo'd `StructField`) that would previously have been caught at compile time.
+- The split between "generic descriptor-driven path" and "preserved special-case semantics" creates a second mental model — contributors must know which handlers go through the descriptor and which retain bespoke logic in `mergeSafeOutputConfig`.
+- The descriptor list is now a centralized hotspot: any change to the descriptor type ripples across all handlers and all dispatch sites.
+
+#### Neutral
+- Hot-path predicates (`hasAnySafeOutputEnabled`, `hasNonBuiltinSafeOutputsEnabled`) intentionally remain as direct nil-check cascades rather than reflection loops, so compile-time performance is unchanged on the most frequently executed checks.
+- `TestHasSafeOutputTypeNewKeys` is updated to validate legacy key parity against the descriptor table and to assert constructor/field type compatibility, providing a guardrail against future drift.
+- The descriptor approach is internal to `pkg/workflow`; no public API surface changes.
+
+---
+
+## Part 2 — Normative Specification (RFC 2119)
+
+> The key words **MUST**, **MUST NOT**, **REQUIRED**, **SHALL**, **SHALL NOT**, **SHOULD**, **SHOULD NOT**, **RECOMMENDED**, **MAY**, and **OPTIONAL** in this section are to be interpreted as described in [RFC 2119](https://www.rfc-editor.org/rfc/rfc2119).
+
+### Descriptor Source of Truth
+
+1. The `safeOutputHandlers` list in `pkg/workflow/safe_output_handlers.go` **MUST** be the single source of truth for the per-handler tuple of `Key`, `Aliases`, `StructField`, `ToolName`, `Builtin`, `NewConfig`, and `PermissionBuilder`.
+2. New safe-output handlers **MUST** be registered by adding an entry to `safeOutputHandlers` and **MUST NOT** be introduced solely by editing dispatch-site switches.
+3. Handler key strings and aliases **MUST NOT** be hardcoded in dispatch sites in a way that bypasses descriptor lookup; dispatch sites **SHALL** resolve keys via the shared descriptor lookup helper.
+
+### Dispatch-Site Routing
+
+1. `hasSafeOutputType` **MUST** resolve handler existence by descriptor key (including aliases) and **MUST** verify the corresponding `StructField` is set via the shared reflection helper.
+2. `SafeOutputsConfigFromKeys` **MUST** construct handler config values via the descriptor's `NewConfig` constructor and **MUST NOT** reintroduce a parallel per-handler switch for construction.
+3. `mergeSafeOutputConfig` **MUST** perform descriptor-driven field merge for standard handlers, and **MAY** retain explicit special-case branches for handlers whose merge semantics are not generic (e.g., protected-files set merging, auto-default override behavior).
+4. `ComputePermissionsForSafeOutputs` **MUST** iterate descriptor `PermissionBuilder` functions to derive handler-managed permissions, and **MUST NOT** duplicate handler-specific permission logic outside the descriptor table.
+5. `safeOutputFieldMapping` **MUST** be derived from descriptor metadata rather than maintained as an independent constant.
+
+### Hot-Path Preservation
+
+1. `hasAnySafeOutputEnabled` and `hasNonBuiltinSafeOutputsEnabled` **MUST** remain direct nil-check cascades over `SafeOutputsConfig` fields and **MUST NOT** be rewritten to perform reflection or descriptor iteration on the compile hot path.
+2. Implementations **MAY** generate or maintain these nil-check cascades from the descriptor list at build time, provided the runtime cost is equivalent to the original direct checks.
+
+### Parity Guardrails
+
+1. The descriptor table **MUST** be covered by a parity test (currently `TestHasSafeOutputTypeNewKeys`) that asserts: every legacy handler key resolves via the descriptor table, every descriptor `StructField` exists on `SafeOutputsConfig`, and every `NewConfig` constructor returns a type assignable to the corresponding `StructField`.
+2. Removing or weakening the parity test **MUST NOT** be done without an accompanying ADR update.
+
+### Conformance
+
+An implementation is considered conformant with this ADR if it satisfies all **MUST** and **MUST NOT** requirements above. Failure to meet any **MUST** or **MUST NOT** requirement constitutes non-conformance. The primary conformance checks are: (a) `safeOutputHandlers` is the sole place where the descriptor tuple is declared, (b) the four named dispatch sites route through the descriptor table as specified, (c) `hasAnySafeOutputEnabled` / `hasNonBuiltinSafeOutputsEnabled` retain direct nil-check cascades, and (d) the parity test continues to validate descriptor/field/constructor coherence.
+
+---
+
+*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/25944925610) workflow. The PR author must review, complete, and finalize this document before the PR can merge.*

pkg/workflow/imports.go

@@ -323,100 +323,12 @@ func hasSafeOutputType(config *SafeOutputsConfig, key string) bool {
 		return false
 	}
 
-	switch key {
-	case "create-issue":
-		return config.CreateIssues != nil
-	case "create-discussion":
-		return config.CreateDiscussions != nil
-	case "close-discussion":
-		return config.CloseDiscussions != nil
-	case "close-issue":
-		return config.CloseIssues != nil
-	case "close-pull-request":
-		return config.ClosePullRequests != nil
-	case "add-comment":
-		return config.AddComments != nil
-	case "create-pull-request":
-		return config.CreatePullRequests != nil
-	case "create-pull-request-review-comment":
-		return config.CreatePullRequestReviewComments != nil
-	case "submit-pull-request-review":
-		return config.SubmitPullRequestReview != nil
-	case "reply-to-pull-request-review-comment":
-		return config.ReplyToPullRequestReviewComment != nil
-	case "resolve-pull-request-review-thread":
-		return config.ResolvePullRequestReviewThread != nil
-	case "create-code-scanning-alert":
-		return config.CreateCodeScanningAlerts != nil
-	case "add-labels":
-		return config.AddLabels != nil
-	case "remove-labels":
-		return config.RemoveLabels != nil
-	case "add-reviewer":
-		return config.AddReviewer != nil
-	case "assign-milestone":
-		return config.AssignMilestone != nil
-	case "assign-to-agent":
-		return config.AssignToAgent != nil
-	case "update-issue":
-		return config.UpdateIssues != nil
-	case "update-pull-request":
-		return config.UpdatePullRequests != nil
-	case "merge-pull-request":
-		return config.MergePullRequest != nil
-	case "push-to-pull-request-branch":
-		return config.PushToPullRequestBranch != nil
-	case "upload-asset":
-		return config.UploadAssets != nil
-	case "upload-artifact":
-		return config.UploadArtifact != nil
-	case "update-release":
-		return config.UpdateRelease != nil
-	case "create-agent-session":
-		return config.CreateAgentSessions != nil
-	case "create-agent-task": // Backward compatibility
-		return config.CreateAgentSessions != nil
-	case "update-project":
-		return config.UpdateProjects != nil
-	case "update-discussion":
-		return config.UpdateDiscussions != nil
-	case "mark-pull-request-as-ready-for-review":
-		return config.MarkPullRequestAsReadyForReview != nil
-	case "autofix-code-scanning-alert":
-		return config.AutofixCodeScanningAlert != nil
-	case "assign-to-user":
-		return config.AssignToUser != nil
-	case "unassign-from-user":
-		return config.UnassignFromUser != nil
-	case "create-project":
-		return config.CreateProjects != nil
-	case "create-project-status-update":
-		return config.CreateProjectStatusUpdates != nil
-	case "link-sub-issue":
-		return config.LinkSubIssue != nil
-	case "hide-comment":
-		return config.HideComment != nil
-	case "set-issue-type":
-		return config.SetIssueType != nil
-	case "set-issue-field":
-		return config.SetIssueField != nil
-	case "dispatch-workflow":
-		return config.DispatchWorkflow != nil
-	case "call-workflow":
-		return config.CallWorkflow != nil
-	case "missing-data":
-		return config.MissingData != nil
-	case "missing-tool":
-		return config.MissingTool != nil
-	case "noop":
-		return config.NoOp != nil
-	case "report-incomplete":
-		return config.ReportIncomplete != nil
-	case "threat-detection":
-		return config.ThreatDetection != nil
-	default:
+	handler, ok := getSafeOutputHandlerByKey(key)
+	if !ok {
 		return false
 	}
+
+	return hasSafeOutputFieldSet(config, handler.StructField)
 }
 
 // mergeSafeOutputConfig merges a single imported config map into the result SafeOutputsConfig
@@ -434,31 +346,24 @@ func mergeSafeOutputConfig(result *SafeOutputsConfig, config map[string]any, c *
 		return result, nil
 	}
 
-	// Merge each safe output type (only set if nil in result)
-	if result.CreateIssues == nil && importedConfig.CreateIssues != nil {
-		result.CreateIssues = importedConfig.CreateIssues
-	}
-	if result.CreateDiscussions == nil && importedConfig.CreateDiscussions != nil {
-		result.CreateDiscussions = importedConfig.CreateDiscussions
-	}
-	if result.UpdateDiscussions == nil && importedConfig.UpdateDiscussions != nil {
-		result.UpdateDiscussions = importedConfig.UpdateDiscussions
-	}
-	if result.CloseDiscussions == nil && importedConfig.CloseDiscussions != nil {
-		result.CloseDiscussions = importedConfig.CloseDiscussions
-	}
-	if result.CloseIssues == nil && importedConfig.CloseIssues != nil {
-		result.CloseIssues = importedConfig.CloseIssues
+	// Merge each safe output type (only set if nil in result).
+	// Types with custom merge semantics are handled below.
+	specialMergeFields := map[string]bool{
+		"CreatePullRequests":      true,
+		"PushToPullRequestBranch": true,
+		"MissingTool":             true,
+		"MissingData":             true,
+		"NoOp":                    true,
+		"ReportIncomplete":        true,
+		"ThreatDetection":         true,
 	}
-	if result.ClosePullRequests == nil && importedConfig.ClosePullRequests != nil {
-		result.ClosePullRequests = importedConfig.ClosePullRequests
-	}
-	if result.MarkPullRequestAsReadyForReview == nil && importedConfig.MarkPullRequestAsReadyForReview != nil {
-		result.MarkPullRequestAsReadyForReview = importedConfig.MarkPullRequestAsReadyForReview
-	}
-	if result.AddComments == nil && importedConfig.AddComments != nil {
-		result.AddComments = importedConfig.AddComments
+	for _, handler := range safeOutputHandlers {
+		if specialMergeFields[handler.StructField] {
+			continue
+		}
+		mergeSafeOutputFieldIfNil(result, importedConfig, handler.StructField)
 	}
+
 	if result.CreatePullRequests == nil && importedConfig.CreatePullRequests != nil {
 		result.CreatePullRequests = importedConfig.CreatePullRequests
 	} else if result.CreatePullRequests != nil && importedConfig.CreatePullRequests != nil {
@@ -469,51 +374,6 @@ func mergeSafeOutputConfig(result *SafeOutputsConfig, config map[string]any, c *
 			importedConfig.CreatePullRequests.ProtectedFilesExclude...,
 		)
 	}
-	if result.CreatePullRequestReviewComments == nil && importedConfig.CreatePullRequestReviewComments != nil {
-		result.CreatePullRequestReviewComments = importedConfig.CreatePullRequestReviewComments
-	}
-	if result.SubmitPullRequestReview == nil && importedConfig.SubmitPullRequestReview != nil {
-		result.SubmitPullRequestReview = importedConfig.SubmitPullRequestReview
-	}
-	if result.ReplyToPullRequestReviewComment == nil && importedConfig.ReplyToPullRequestReviewComment != nil {
-		result.ReplyToPullRequestReviewComment = importedConfig.ReplyToPullRequestReviewComment
-	}
-	if result.ResolvePullRequestReviewThread == nil && importedConfig.ResolvePullRequestReviewThread != nil {
-		result.ResolvePullRequestReviewThread = importedConfig.ResolvePullRequestReviewThread
-	}
-	if result.CreateCodeScanningAlerts == nil && importedConfig.CreateCodeScanningAlerts != nil {
-		result.CreateCodeScanningAlerts = importedConfig.CreateCodeScanningAlerts
-	}
-	if result.AutofixCodeScanningAlert == nil && importedConfig.AutofixCodeScanningAlert != nil {
-		result.AutofixCodeScanningAlert = importedConfig.AutofixCodeScanningAlert
-	}
-	if result.AddLabels == nil && importedConfig.AddLabels != nil {
-		result.AddLabels = importedConfig.AddLabels
-	}
-	if result.RemoveLabels == nil && importedConfig.RemoveLabels != nil {
-		result.RemoveLabels = importedConfig.RemoveLabels
-	}
-	if result.AddReviewer == nil && importedConfig.AddReviewer != nil {
-		result.AddReviewer = importedConfig.AddReviewer
-	}
-	if result.AssignMilestone == nil && importedConfig.AssignMilestone != nil {
-		result.AssignMilestone = importedConfig.AssignMilestone
-	}
-	if result.AssignToAgent == nil && importedConfig.AssignToAgent != nil {
-		result.AssignToAgent = importedConfig.AssignToAgent
-	}
-	if result.AssignToUser == nil && importedConfig.AssignToUser != nil {
-		result.AssignToUser = importedConfig.AssignToUser
-	}
-	if result.UpdateIssues == nil && importedConfig.UpdateIssues != nil {
-		result.UpdateIssues = importedConfig.UpdateIssues
-	}
-	if result.UpdatePullRequests == nil && importedConfig.UpdatePullRequests != nil {
-		result.UpdatePullRequests = importedConfig.UpdatePullRequests
-	}
-	if result.MergePullRequest == nil && importedConfig.MergePullRequest != nil {
-		result.MergePullRequest = importedConfig.MergePullRequest
-	}
 	if result.PushToPullRequestBranch == nil && importedConfig.PushToPullRequestBranch != nil {
 		result.PushToPullRequestBranch = importedConfig.PushToPullRequestBranch
 	} else if result.PushToPullRequestBranch != nil && importedConfig.PushToPullRequestBranch != nil {
@@ -524,45 +384,6 @@ func mergeSafeOutputConfig(result *SafeOutputsConfig, config map[string]any, c *
 			importedConfig.PushToPullRequestBranch.ProtectedFilesExclude...,
 		)
 	}
-	if result.UploadAssets == nil && importedConfig.UploadAssets != nil {
-		result.UploadAssets = importedConfig.UploadAssets
-	}
-	if result.UploadArtifact == nil && importedConfig.UploadArtifact != nil {
-		result.UploadArtifact = importedConfig.UploadArtifact
-	}
-	if result.UpdateRelease == nil && importedConfig.UpdateRelease != nil {
-		result.UpdateRelease = importedConfig.UpdateRelease
-	}
-	if result.CreateAgentSessions == nil && importedConfig.CreateAgentSessions != nil {
-		result.CreateAgentSessions = importedConfig.CreateAgentSessions
-	}
-	if result.UpdateProjects == nil && importedConfig.UpdateProjects != nil {
-		result.UpdateProjects = importedConfig.UpdateProjects
-	}
-	if result.CreateProjects == nil && importedConfig.CreateProjects != nil {
-		result.CreateProjects = importedConfig.CreateProjects
-	}
-	if result.CreateProjectStatusUpdates == nil && importedConfig.CreateProjectStatusUpdates != nil {
-		result.CreateProjectStatusUpdates = importedConfig.CreateProjectStatusUpdates
-	}
-	if result.LinkSubIssue == nil && importedConfig.LinkSubIssue != nil {
-		result.LinkSubIssue = importedConfig.LinkSubIssue
-	}
-	if result.HideComment == nil && importedConfig.HideComment != nil {
-		result.HideComment = importedConfig.HideComment
-	}
-	if result.SetIssueType == nil && importedConfig.SetIssueType != nil {
-		result.SetIssueType = importedConfig.SetIssueType
-	}
-	if result.SetIssueField == nil && importedConfig.SetIssueField != nil {
-		result.SetIssueField = importedConfig.SetIssueField
-	}
-	if result.DispatchWorkflow == nil && importedConfig.DispatchWorkflow != nil {
-		result.DispatchWorkflow = importedConfig.DispatchWorkflow
-	}
-	if result.CallWorkflow == nil && importedConfig.CallWorkflow != nil {
-		result.CallWorkflow = importedConfig.CallWorkflow
-	}
 	// missing-tool, missing-data, noop, and report-incomplete are auto-defaulted by
 	// extractSafeOutputsConfig whenever any safe-outputs are present, even when the user
 	// has not explicitly configured those types. This means result.X can be non-nil (the