Refactor safe-output dispatch logic to use a canonical handler descriptor table#32489
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Consolidates safe-output handler metadata (key, struct field, tool name, constructor, permission builder, builtin marker) into a single descriptor table and routes existing dispatch paths through it via reflection helpers, replacing several parallel switch/if cascades.
Changes:
- Adds
safe_output_handlers.gowith a canonicalsafeOutputHandlersdescriptor list plus shared lookup/reflection helpers. - Rewrites
hasSafeOutputType,SafeOutputsConfigFromKeys,mergeSafeOutputConfig, andComputePermissionsForSafeOutputsto drive off the descriptor table while preserving special-case merge/permission semantics. - Replaces the hand-maintained
safeOutputFieldMappingmap with a descriptor-derived builder and updatesTestHasSafeOutputTypeNewKeysto validate legacy switch keys against the descriptor table.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/safe_output_handlers.go | New canonical handler descriptor table and reflection helpers. |
| pkg/workflow/imports.go | hasSafeOutputType and mergeSafeOutputConfig now drive off the descriptor table; special-case merges retained. |
| pkg/workflow/safe_outputs_permissions.go | ComputePermissionsForSafeOutputs iterates descriptor permission builders; SafeOutputsConfigFromKeys uses descriptor constructors. |
| pkg/workflow/safe_outputs_state.go | safeOutputFieldMapping now derived from descriptors; maintenance NOTE replaced with new descriptor-centric note. |
| pkg/workflow/safe_outputs_fix_test.go | Test rewritten as a descriptor-parity check against the legacy switch key set. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
pkg/workflow/safe_output_handlers.go:495
- The
Keyfor the dispatch_repository handler uses underscores ("dispatch_repository"), while every other descriptor in this table uses kebab-case (e.g."dispatch-workflow","create-issue","update-release"). Although the JSON schema uses the underscore form, the inconsistent style here makes lookups bygetSafeOutputHandlerByKey("dispatch-repository")silently miss this entry — which matters becauseparseDispatchRepositoryConfigexplicitly accepts bothdispatch_repositoryanddispatch-repositoryas aliases. Consider either making the canonical Key"dispatch-repository"withAliases: []string{"dispatch_repository"}, or addingAliases: []string{"dispatch-repository"}to match the alias support that already exists at parse time. As-is, any caller routing through the descriptor with the dash form will not find this handler.
{
Key: "dispatch_repository",
StructField: "DispatchRepository",
ToolName: "dispatch_repository",
NewConfig: func() any { return &DispatchRepositoryConfig{} },
},
- Files reviewed: 5/5 changed files
- Comments generated: 5
| permissions := NewPermissionsContentsReadProjectsWrite() | ||
| if current, exists := permissions.Get(PermissionIssues); !exists || current != PermissionWrite { | ||
| permissions.Set(PermissionIssues, PermissionRead) | ||
| } | ||
| return permissions | ||
| }, | ||
| }, | ||
| { | ||
| Key: "create-project", | ||
| StructField: "CreateProjects", | ||
| ToolName: "create_project", | ||
| NewConfig: func() any { return &CreateProjectsConfig{} }, | ||
| PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { | ||
| if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CreateProjects") { | ||
| return nil | ||
| } | ||
| permissions := NewPermissionsContentsReadProjectsWrite() | ||
| if current, exists := permissions.Get(PermissionIssues); !exists || current != PermissionWrite { | ||
| permissions.Set(PermissionIssues, PermissionRead) | ||
| } | ||
| return permissions |
| // 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) | ||
| } |
| constructorType := reflect.ValueOf(handler.NewConfig()).Type() | ||
| require.True(t, constructorType.AssignableTo(field.Type()), | ||
| "descriptor constructor for key %q returns %v, expected assignable to %v", key, constructorType, field.Type()) | ||
| } | ||
| field.Set(reflect.New(field.Type().Elem())) |
| handler, ok := getSafeOutputHandlerByKey(key) | ||
| if !ok || handler.NewConfig == nil { | ||
| continue | ||
| } | ||
| if hasSafeOutputFieldSet(config, handler.StructField) { | ||
| continue | ||
| } | ||
| _ = setSafeOutputField(config, handler.StructField, handler.NewConfig()) | ||
| } |
| // NOTE: Most safe output dispatch behavior is driven by safeOutputHandlers in | ||
| // safe_output_handlers.go. The two functions below intentionally remain direct | ||
| // nil-check cascades because they are hot-path checks executed on every compile. | ||
|
|
||
| // safeOutputFieldMapping maps SafeOutputsConfig struct field names to their tool names. | ||
| // This map is used by imports, prompt generation, and other metadata operations. | ||
| // It is NOT used for existence checks — see hasAnySafeOutputEnabled and | ||
| // hasNonBuiltinSafeOutputsEnabled for the performance-critical direct-field versions. | ||
| var safeOutputFieldMapping = map[string]string{ | ||
| "CreateIssues": "create_issue", | ||
| "CreateAgentSessions": "create_agent_session", | ||
| "CreateDiscussions": "create_discussion", | ||
| "UpdateDiscussions": "update_discussion", | ||
| "CloseDiscussions": "close_discussion", | ||
| "CloseIssues": "close_issue", | ||
| "ClosePullRequests": "close_pull_request", | ||
| "AddComments": "add_comment", | ||
| "CreatePullRequests": "create_pull_request", | ||
| "CreatePullRequestReviewComments": "create_pull_request_review_comment", | ||
| "SubmitPullRequestReview": "submit_pull_request_review", | ||
| "ReplyToPullRequestReviewComment": "reply_to_pull_request_review_comment", | ||
| "ResolvePullRequestReviewThread": "resolve_pull_request_review_thread", | ||
| "CreateCodeScanningAlerts": "create_code_scanning_alert", | ||
| "AutofixCodeScanningAlert": "autofix_code_scanning_alert", | ||
| "AddLabels": "add_labels", | ||
| "RemoveLabels": "remove_labels", | ||
| "AddReviewer": "add_reviewer", | ||
| "AssignMilestone": "assign_milestone", | ||
| "AssignToAgent": "assign_to_agent", | ||
| "AssignToUser": "assign_to_user", | ||
| "UnassignFromUser": "unassign_from_user", | ||
| "UpdateIssues": "update_issue", | ||
| "UpdatePullRequests": "update_pull_request", | ||
| "MergePullRequest": "merge_pull_request", | ||
| "PushToPullRequestBranch": "push_to_pull_request_branch", | ||
| "UploadAssets": "upload_asset", | ||
| "UploadArtifact": "upload_artifact", | ||
| "UpdateRelease": "update_release", | ||
| "UpdateProjects": "update_project", | ||
| "CreateProjects": "create_project", | ||
| "CreateProjectStatusUpdates": "create_project_status_update", | ||
| "LinkSubIssue": "link_sub_issue", | ||
| "HideComment": "hide_comment", | ||
| "DispatchWorkflow": "dispatch_workflow", | ||
| "DispatchRepository": "dispatch_repository", | ||
| "CallWorkflow": "call_workflow", | ||
| "MissingTool": "missing_tool", | ||
| "MissingData": "missing_data", | ||
| "SetIssueType": "set_issue_type", | ||
| "SetIssueField": "set_issue_field", | ||
| "NoOp": "noop", | ||
| "MarkPullRequestAsReadyForReview": "mark_pull_request_as_ready_for_review", | ||
| } | ||
| var safeOutputFieldMapping = buildSafeOutputFieldMapping() |
🧪 Test Quality Sentinel ReportTest Quality Score: 100/100✅ Excellent test quality
Test Classification Details
The other three test functions ( Test Analysis
This is a high-value design test: it enforces the invariant that the refactored descriptor table is a complete replacement for the old switch statement, and would catch any future key omission. All assertions include descriptive messages ✅. No mocks ✅. Build tag Test Inflation Check
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §25944925628
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The refactored TestHasSafeOutputTypeNewKeys is a high-value design test enforcing descriptor table parity, field validity, constructor type-safety, and bidirectional hasSafeOutputType behavior.
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (777 new lines across AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: The draft has been committed to this branch (status: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the recorded decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. A descriptor-table dispatch is exactly the kind of structural decision future contributors need recorded — without it, the next person who sees the reflection helpers will wonder whether they should add another parallel switch instead. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in References:
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /improve-codebase-architecture and /zoom-out as the primary skills (large refactor introducing a new canonical module) with /tdd for test coverage concerns.
Key Themes
1. Internal drift not fully eliminated (/improve-codebase-architecture)
The most important issue: each PermissionBuilder closure repeats the StructField string (isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CreateIssues")) instead of referencing handler.StructField. The descriptor was designed to be the single source of truth, but the permission guard introduces a second copy of the same string for every handler. Renaming a StructField would require updating both places — the same drift the PR set out to remove.
2. Two Builtin truths remain (/zoom-out)
hasNonBuiltinSafeOutputsEnabled is a hardcoded nil-check cascade that must stay in sync with the Builtin: true flags in the descriptor table. The Builtin field is stored but currently unused by the hot-path function. The old NOTE comment that explicitly listed both remaining manual-update locations was removed — which is a contributor experience regression.
3. Reflection primitives lack direct tests (/tdd)
setSafeOutputField, hasSafeOutputFieldSet, and mergeSafeOutputFieldIfNil are the load-bearing new code, but they have no unit tests. Silent false returns on unknown field names are the main risk; the parity test catches descriptor mismatches but doesn't cover the helper behavior itself. The discarded _ = setSafeOutputField(...) return amplifies this concern.
4. NewConfig func() any weakens the interface (/improve-codebase-architecture)
Returning any means the descriptor type relation between NewConfig and StructField is only validated at test time, not compile time. A comment or interface documenting the invariant would help future contributors.
Positive Highlights
- ✅ Excellent problem framing in the PR description — the motivation (shotgun edits, drift risk) is clearly articulated and the solution follows directly from it.
- ✅ Hot-path preservation is correct — keeping
hasAnySafeOutputEnabledandhasNonBuiltinSafeOutputsEnabledas direct nil-check cascades is the right performance call; the reasoning is documented. - ✅
safeOutputHandlersByKeymap built at package init — avoids repeated linear scans; clean. - ✅ The parity test is a great idea — validating all legacy keys against the descriptor table at test time is exactly the right guardrail for this kind of refactor.
- ✅ Net deletion of ~200 lines — the reduction in duplicated switch arms is the core value and it delivers cleanly.
Verdict
The architecture direction is sound and the PR delivers real value. The issues above are mostly refinements rather than blockers — the most impactful fix would be eliminating the repeated StructField string in PermissionBuilder closures and restoring the explicit NOTE for the two remaining manual-update locations. Happy to approve once those are addressed.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · ● 14.6M
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reviewed and addressed the actionable review threads in commits Implemented:
Validation run:
|
|
Please resolve the open review threads, especially the parity test gap for hasAnySafeOutputEnabled / hasNonBuiltinSafeOutputsEnabled, and then rerun checks.
|
|
@copilot merge main and recompile |
…-outputs-dispatch-tables Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Merged |
Safe-output handler behavior was spread across multiple parallel switch/if tables, making every new handler a multi-file shotgun edit with high drift risk (conflict detection, merge behavior, and permission derivation could diverge). This change introduces one canonical descriptor source and routes key dispatch paths through it.
Canonical handler metadata
safe_output_handlers.gowith a singlesafeOutputHandlersdescriptor list (Key, aliases, struct field, tool name, constructor, permission builder, builtin marker).Dispatch-site consolidation
hasSafeOutputTypenow resolves by descriptor key + reflected field check (including alias support).SafeOutputsConfigFromKeysnow constructs config via descriptor constructors instead of a large switch.mergeSafeOutputConfignow performs descriptor-driven field merge for standard handlers, while preserving existing special-case semantics (e.g., protected-files set merges, auto-default override behavior).Permission derivation consolidation
ComputePermissionsForSafeOutputsnow iterates descriptor permission builders for handler-managed permissions, reducing repeated per-handler branches while preserving per-handler semantics.Existing hot-path behavior preserved
hasAnySafeOutputEnabled/hasNonBuiltinSafeOutputsEnabledremain direct nil-check cascades (no reflection on compile hot path).safeOutputFieldMappingis now derived from descriptor metadata.Parity guardrails
TestHasSafeOutputTypeNewKeysto validate legacy key parity against the descriptor table and ensure constructor/field type compatibility.