diff --git a/docs/adr/32489-canonical-descriptor-table-for-safe-output-dispatch.md b/docs/adr/32489-canonical-descriptor-table-for-safe-output-dispatch.md new file mode 100644 index 00000000000..b4c7d266006 --- /dev/null +++ b/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.* diff --git a/pkg/workflow/imports.go b/pkg/workflow/imports.go index 8a99311ac38..69bc768c46a 100644 --- a/pkg/workflow/imports.go +++ b/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 diff --git a/pkg/workflow/safe_output_handlers.go b/pkg/workflow/safe_output_handlers.go new file mode 100644 index 00000000000..7f11cce44c5 --- /dev/null +++ b/pkg/workflow/safe_output_handlers.go @@ -0,0 +1,667 @@ +package workflow + +import "reflect" + +type safeOutputHandlerDescriptor struct { + Key string + Aliases []string + StructField string + ToolName string + Builtin bool + NewConfig func() any + PermissionBuilder func(*SafeOutputsConfig) *Permissions +} + +var safeOutputHandlers = []safeOutputHandlerDescriptor{ + { + Key: "create-issue", + StructField: "CreateIssues", + ToolName: "create_issue", + NewConfig: func() any { return &CreateIssuesConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CreateIssues") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "create-agent-session", + Aliases: []string{"create-agent-task"}, + StructField: "CreateAgentSessions", + ToolName: "create_agent_session", + NewConfig: func() any { return &CreateAgentSessionConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CreateAgentSessions") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "create-discussion", + StructField: "CreateDiscussions", + ToolName: "create_discussion", + NewConfig: func() any { return &CreateDiscussionsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CreateDiscussions") { + return nil + } + return NewPermissionsContentsReadIssuesWriteDiscussionsWrite() + }, + }, + { + Key: "update-discussion", + StructField: "UpdateDiscussions", + ToolName: "update_discussion", + NewConfig: func() any { return &UpdateDiscussionsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "UpdateDiscussions") { + return nil + } + return NewPermissionsContentsReadDiscussionsWrite() + }, + }, + { + Key: "close-discussion", + StructField: "CloseDiscussions", + ToolName: "close_discussion", + NewConfig: func() any { return &CloseDiscussionsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CloseDiscussions") { + return nil + } + return NewPermissionsContentsReadDiscussionsWrite() + }, + }, + { + Key: "close-issue", + StructField: "CloseIssues", + ToolName: "close_issue", + NewConfig: func() any { return &CloseIssuesConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CloseIssues") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "close-pull-request", + StructField: "ClosePullRequests", + ToolName: "close_pull_request", + NewConfig: func() any { return &ClosePullRequestsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "ClosePullRequests") { + return nil + } + return NewPermissionsContentsReadPRWrite() + }, + }, + { + Key: "mark-pull-request-as-ready-for-review", + StructField: "MarkPullRequestAsReadyForReview", + ToolName: "mark_pull_request_as_ready_for_review", + NewConfig: func() any { return &MarkPullRequestAsReadyForReviewConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "MarkPullRequestAsReadyForReview") { + return nil + } + return NewPermissionsContentsReadPRWrite() + }, + }, + { + Key: "add-comment", + StructField: "AddComments", + ToolName: "add_comment", + NewConfig: func() any { return &AddCommentsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "AddComments") { + return nil + } + return buildAddCommentPermissions(safeOutputs.AddComments) + }, + }, + { + Key: "comment-memory", + StructField: "CommentMemory", + NewConfig: func() any { return &CommentMemoryConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CommentMemory") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "create-pull-request", + StructField: "CreatePullRequests", + ToolName: "create_pull_request", + NewConfig: func() any { return &CreatePullRequestsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CreatePullRequests") { + return nil + } + if getFallbackAsIssue(safeOutputs.CreatePullRequests) { + permissions := NewPermissionsContentsWriteIssuesWritePRWrite() + if safeOutputs.CreatePullRequests.AllowWorkflows { + permissions.Set(PermissionWorkflows, PermissionWrite) + } + return permissions + } + permissions := NewPermissionsContentsWritePRWrite() + if safeOutputs.CreatePullRequests.AllowWorkflows { + permissions.Set(PermissionWorkflows, PermissionWrite) + } + return permissions + }, + }, + { + Key: "create-pull-request-review-comment", + StructField: "CreatePullRequestReviewComments", + ToolName: "create_pull_request_review_comment", + NewConfig: func() any { return &CreatePullRequestReviewCommentsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CreatePullRequestReviewComments") { + return nil + } + return NewPermissionsContentsReadPRWrite() + }, + }, + { + Key: "submit-pull-request-review", + StructField: "SubmitPullRequestReview", + ToolName: "submit_pull_request_review", + NewConfig: func() any { return &SubmitPullRequestReviewConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "SubmitPullRequestReview") { + return nil + } + return NewPermissionsContentsReadPRWrite() + }, + }, + { + Key: "reply-to-pull-request-review-comment", + StructField: "ReplyToPullRequestReviewComment", + ToolName: "reply_to_pull_request_review_comment", + NewConfig: func() any { return &ReplyToPullRequestReviewCommentConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "ReplyToPullRequestReviewComment") { + return nil + } + return NewPermissionsContentsReadPRWrite() + }, + }, + { + Key: "resolve-pull-request-review-thread", + StructField: "ResolvePullRequestReviewThread", + ToolName: "resolve_pull_request_review_thread", + NewConfig: func() any { return &ResolvePullRequestReviewThreadConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "ResolvePullRequestReviewThread") { + return nil + } + return NewPermissionsContentsReadPRWrite() + }, + }, + { + Key: "create-code-scanning-alert", + StructField: "CreateCodeScanningAlerts", + ToolName: "create_code_scanning_alert", + NewConfig: func() any { return &CreateCodeScanningAlertsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CreateCodeScanningAlerts") { + return nil + } + return NewPermissionsContentsReadSecurityEventsWrite() + }, + }, + { + Key: "autofix-code-scanning-alert", + StructField: "AutofixCodeScanningAlert", + ToolName: "autofix_code_scanning_alert", + NewConfig: func() any { return &AutofixCodeScanningAlertConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "AutofixCodeScanningAlert") { + return nil + } + return NewPermissionsContentsReadSecurityEventsWriteActionsRead() + }, + }, + { + Key: "add-labels", + StructField: "AddLabels", + ToolName: "add_labels", + NewConfig: func() any { return &AddLabelsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "AddLabels") { + return nil + } + return NewPermissionsContentsReadIssuesWritePRWrite() + }, + }, + { + Key: "remove-labels", + StructField: "RemoveLabels", + ToolName: "remove_labels", + NewConfig: func() any { return &RemoveLabelsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "RemoveLabels") { + return nil + } + return NewPermissionsContentsReadIssuesWritePRWrite() + }, + }, + { + Key: "add-reviewer", + StructField: "AddReviewer", + ToolName: "add_reviewer", + NewConfig: func() any { return &AddReviewerConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "AddReviewer") { + return nil + } + return NewPermissionsContentsReadPRWrite() + }, + }, + { + Key: "assign-milestone", + StructField: "AssignMilestone", + ToolName: "assign_milestone", + NewConfig: func() any { return &AssignMilestoneConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "AssignMilestone") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "assign-to-agent", + StructField: "AssignToAgent", + ToolName: "assign_to_agent", + NewConfig: func() any { return &AssignToAgentConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "AssignToAgent") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "assign-to-user", + StructField: "AssignToUser", + ToolName: "assign_to_user", + NewConfig: func() any { return &AssignToUserConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "AssignToUser") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "unassign-from-user", + StructField: "UnassignFromUser", + ToolName: "unassign_from_user", + NewConfig: func() any { return &UnassignFromUserConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "UnassignFromUser") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "update-issue", + StructField: "UpdateIssues", + ToolName: "update_issue", + NewConfig: func() any { return &UpdateIssuesConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "UpdateIssues") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "update-pull-request", + StructField: "UpdatePullRequests", + ToolName: "update_pull_request", + NewConfig: func() any { return &UpdatePullRequestsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "UpdatePullRequests") { + return nil + } + if safeOutputs.UpdatePullRequests.UpdateBranch != nil && *safeOutputs.UpdatePullRequests.UpdateBranch { + return NewPermissionsContentsWritePRWrite() + } + return NewPermissionsContentsReadPRWrite() + }, + }, + { + Key: "merge-pull-request", + StructField: "MergePullRequest", + ToolName: "merge_pull_request", + NewConfig: func() any { return &MergePullRequestConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "MergePullRequest") { + return nil + } + return NewPermissionsContentsWritePRWrite() + }, + }, + { + Key: "push-to-pull-request-branch", + StructField: "PushToPullRequestBranch", + ToolName: "push_to_pull_request_branch", + NewConfig: func() any { return &PushToPullRequestBranchConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "PushToPullRequestBranch") { + return nil + } + permissions := NewPermissions() + if getPushFallbackAsPullRequest(safeOutputs.PushToPullRequestBranch) { + permissions.Merge(NewPermissionsContentsWritePRWrite()) + } else { + permissions.Merge(NewPermissionsContentsWrite()) + } + if safeOutputs.PushToPullRequestBranch.AllowWorkflows { + permissions.Set(PermissionWorkflows, PermissionWrite) + } + if getCheckBranchProtection(safeOutputs.PushToPullRequestBranch) { + permissions.Set(PermissionAdministration, PermissionRead) + } + return permissions + }, + }, + { + Key: "upload-asset", + StructField: "UploadAssets", + ToolName: "upload_asset", + NewConfig: func() any { return &UploadAssetsConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "UploadAssets") { + return nil + } + return NewPermissionsContentsRead() + }, + }, + { + Key: "upload-artifact", + StructField: "UploadArtifact", + ToolName: "upload_artifact", + }, + { + Key: "update-release", + StructField: "UpdateRelease", + ToolName: "update_release", + NewConfig: func() any { return &UpdateReleaseConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "UpdateRelease") { + return nil + } + return NewPermissionsContentsWrite() + }, + }, + { + Key: "update-project", + StructField: "UpdateProjects", + ToolName: "update_project", + NewConfig: func() any { return &UpdateProjectConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "UpdateProjects") { + return nil + } + permissions := NewPermissionsContentsReadProjectsWrite() + 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() + permissions.Set(PermissionIssues, PermissionRead) + return permissions + }, + }, + { + Key: "create-project-status-update", + StructField: "CreateProjectStatusUpdates", + ToolName: "create_project_status_update", + NewConfig: func() any { return &CreateProjectStatusUpdateConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "CreateProjectStatusUpdates") { + return nil + } + return NewPermissionsContentsReadProjectsWrite() + }, + }, + { + Key: "link-sub-issue", + StructField: "LinkSubIssue", + ToolName: "link_sub_issue", + NewConfig: func() any { return &LinkSubIssueConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "LinkSubIssue") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "hide-comment", + StructField: "HideComment", + ToolName: "hide_comment", + NewConfig: func() any { return &HideCommentConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "HideComment") { + return nil + } + if safeOutputs.HideComment.Discussions != nil && !*safeOutputs.HideComment.Discussions { + return NewPermissionsContentsReadIssuesWrite() + } + return NewPermissionsContentsReadIssuesWriteDiscussionsWrite() + }, + }, + { + Key: "dispatch-workflow", + StructField: "DispatchWorkflow", + ToolName: "dispatch_workflow", + NewConfig: func() any { return &DispatchWorkflowConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "DispatchWorkflow") { + return nil + } + return NewPermissionsActionsWrite() + }, + }, + { + Key: "dispatch_repository", + StructField: "DispatchRepository", + ToolName: "dispatch_repository", + NewConfig: func() any { return &DispatchRepositoryConfig{} }, + }, + { + Key: "call-workflow", + StructField: "CallWorkflow", + ToolName: "call_workflow", + NewConfig: func() any { return &CallWorkflowConfig{} }, + }, + { + Key: "missing-tool", + StructField: "MissingTool", + ToolName: "missing_tool", + Builtin: true, + }, + { + Key: "missing-data", + StructField: "MissingData", + ToolName: "missing_data", + Builtin: true, + }, + { + Key: "set-issue-type", + StructField: "SetIssueType", + ToolName: "set_issue_type", + NewConfig: func() any { return &SetIssueTypeConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "SetIssueType") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "set-issue-field", + StructField: "SetIssueField", + ToolName: "set_issue_field", + NewConfig: func() any { return &SetIssueFieldConfig{} }, + PermissionBuilder: func(safeOutputs *SafeOutputsConfig) *Permissions { + if !isSafeOutputHandlerEnabledAndUnstaged(safeOutputs, "SetIssueField") { + return nil + } + return NewPermissionsContentsReadIssuesWrite() + }, + }, + { + Key: "noop", + StructField: "NoOp", + ToolName: "noop", + Builtin: true, + }, + { + Key: "report-incomplete", + StructField: "ReportIncomplete", + }, + { + Key: "threat-detection", + StructField: "ThreatDetection", + }, +} + +var safeOutputHandlersByKey = buildSafeOutputHandlersByKey() + +func buildSafeOutputHandlersByKey() map[string]safeOutputHandlerDescriptor { + result := make(map[string]safeOutputHandlerDescriptor, len(safeOutputHandlers)+1) + for _, handler := range safeOutputHandlers { + if handler.Key != "" { + result[handler.Key] = handler + } + for _, alias := range handler.Aliases { + result[alias] = handler + } + } + return result +} + +func buildSafeOutputFieldMapping() map[string]string { + mapping := make(map[string]string) + for _, handler := range safeOutputHandlers { + if handler.ToolName == "" { + continue + } + mapping[handler.StructField] = handler.ToolName + } + return mapping +} + +func getSafeOutputHandlerByKey(key string) (safeOutputHandlerDescriptor, bool) { + handler, ok := safeOutputHandlersByKey[key] + return handler, ok +} + +func safeOutputPointerFieldValue(config *SafeOutputsConfig, fieldName string) (reflect.Value, bool) { + if config == nil { + return reflect.Value{}, false + } + + value := reflect.ValueOf(config) + if value.Kind() != reflect.Ptr || value.IsNil() { + return reflect.Value{}, false + } + + field := value.Elem().FieldByName(fieldName) + if !field.IsValid() || field.Kind() != reflect.Ptr { + return reflect.Value{}, false + } + + return field, true +} + +func hasSafeOutputFieldSet(config *SafeOutputsConfig, fieldName string) bool { + field, ok := safeOutputPointerFieldValue(config, fieldName) + return ok && !field.IsNil() +} + +func setSafeOutputField(config *SafeOutputsConfig, fieldName string, value any) bool { + field, ok := safeOutputPointerFieldValue(config, fieldName) + if !ok { + return false + } + + newValue := reflect.ValueOf(value) + if !newValue.IsValid() || !newValue.Type().AssignableTo(field.Type()) { + return false + } + + field.Set(newValue) + return true +} + +func mergeSafeOutputFieldIfNil(result, imported *SafeOutputsConfig, fieldName string) { + resultField, ok := safeOutputPointerFieldValue(result, fieldName) + if !ok || !resultField.IsNil() { + return + } + + importedField, ok := safeOutputPointerFieldValue(imported, fieldName) + if !ok || importedField.IsNil() { + return + } + + resultField.Set(importedField) +} + +func isSafeOutputHandlerEnabledAndUnstaged(safeOutputs *SafeOutputsConfig, fieldName string) bool { + field, ok := safeOutputPointerFieldValue(safeOutputs, fieldName) + if !ok || field.IsNil() { + return false + } + + return !isHandlerStaged(safeOutputs.Staged, safeOutputHandlerStaged(field)) +} + +func safeOutputHandlerStaged(field reflect.Value) bool { + if !field.IsValid() || field.IsNil() { + return false + } + + elem := field.Elem() + if !elem.IsValid() || elem.Kind() != reflect.Struct { + return false + } + + if staged := elem.FieldByName("Staged"); staged.IsValid() && staged.Kind() == reflect.Bool { + return staged.Bool() + } + + baseConfig := elem.FieldByName("BaseSafeOutputConfig") + if !baseConfig.IsValid() || baseConfig.Kind() != reflect.Struct { + return false + } + + staged := baseConfig.FieldByName("Staged") + if !staged.IsValid() || staged.Kind() != reflect.Bool { + return false + } + + return staged.Bool() +} diff --git a/pkg/workflow/safe_outputs_fix_test.go b/pkg/workflow/safe_outputs_fix_test.go index f17bf23917e..789b8af066d 100644 --- a/pkg/workflow/safe_outputs_fix_test.go +++ b/pkg/workflow/safe_outputs_fix_test.go @@ -3,6 +3,7 @@ package workflow import ( + "reflect" "testing" "github.com/goccy/go-yaml" @@ -14,79 +15,82 @@ import ( // hasSafeOutputType new switch cases // ======================================== -// TestHasSafeOutputTypeNewKeys verifies that the 11 operation types added to hasSafeOutputType -// are correctly detected. These were previously silently returning false, causing import -// conflict detection to pass through conflicts for those types. +// TestHasSafeOutputTypeNewKeys validates that all previously-supported hasSafeOutputType switch keys +// are represented by the canonical safe output descriptor table. func TestHasSafeOutputTypeNewKeys(t *testing.T) { - tests := []struct { - name string - key string - config *SafeOutputsConfig - }{ - { - name: "update-discussion", - key: "update-discussion", - config: &SafeOutputsConfig{UpdateDiscussions: &UpdateDiscussionsConfig{}}, - }, - { - name: "mark-pull-request-as-ready-for-review", - key: "mark-pull-request-as-ready-for-review", - config: &SafeOutputsConfig{MarkPullRequestAsReadyForReview: &MarkPullRequestAsReadyForReviewConfig{}}, - }, - { - name: "autofix-code-scanning-alert", - key: "autofix-code-scanning-alert", - config: &SafeOutputsConfig{AutofixCodeScanningAlert: &AutofixCodeScanningAlertConfig{}}, - }, - { - name: "assign-to-user", - key: "assign-to-user", - config: &SafeOutputsConfig{AssignToUser: &AssignToUserConfig{}}, - }, - { - name: "unassign-from-user", - key: "unassign-from-user", - config: &SafeOutputsConfig{UnassignFromUser: &UnassignFromUserConfig{}}, - }, - { - name: "create-project", - key: "create-project", - config: &SafeOutputsConfig{CreateProjects: &CreateProjectsConfig{}}, - }, - { - name: "create-project-status-update", - key: "create-project-status-update", - config: &SafeOutputsConfig{CreateProjectStatusUpdates: &CreateProjectStatusUpdateConfig{}}, - }, - { - name: "link-sub-issue", - key: "link-sub-issue", - config: &SafeOutputsConfig{LinkSubIssue: &LinkSubIssueConfig{}}, - }, - { - name: "hide-comment", - key: "hide-comment", - config: &SafeOutputsConfig{HideComment: &HideCommentConfig{}}, - }, - { - name: "dispatch-workflow", - key: "dispatch-workflow", - config: &SafeOutputsConfig{DispatchWorkflow: &DispatchWorkflowConfig{}}, - }, - { - name: "missing-data", - key: "missing-data", - config: &SafeOutputsConfig{MissingData: &MissingDataConfig{}}, - }, + legacySwitchKeys := []string{ + "create-issue", + "create-discussion", + "close-discussion", + "close-issue", + "close-pull-request", + "add-comment", + "create-pull-request", + "create-pull-request-review-comment", + "submit-pull-request-review", + "reply-to-pull-request-review-comment", + "resolve-pull-request-review-thread", + "create-code-scanning-alert", + "add-labels", + "remove-labels", + "add-reviewer", + "assign-milestone", + "assign-to-agent", + "update-issue", + "update-pull-request", + "merge-pull-request", + "push-to-pull-request-branch", + "upload-asset", + "upload-artifact", + "update-release", + "create-agent-session", + "create-agent-task", + "update-project", + "update-discussion", + "mark-pull-request-as-ready-for-review", + "autofix-code-scanning-alert", + "assign-to-user", + "unassign-from-user", + "create-project", + "create-project-status-update", + "link-sub-issue", + "hide-comment", + "set-issue-type", + "set-issue-field", + "dispatch-workflow", + "call-workflow", + "missing-data", + "missing-tool", + "noop", + "report-incomplete", + "threat-detection", } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - // Should return true when the field is set - assert.True(t, hasSafeOutputType(tt.config, tt.key), "hasSafeOutputType should return true for key %q when field is set", tt.key) + for _, key := range legacySwitchKeys { + t.Run(key, func(t *testing.T) { + handler, ok := getSafeOutputHandlerByKey(key) + require.True(t, ok, "descriptor table must include hasSafeOutputType key %q", key) + + cfg := &SafeOutputsConfig{} + field := reflect.ValueOf(cfg).Elem().FieldByName(handler.StructField) + require.True(t, field.IsValid(), "descriptor references unknown field %q", handler.StructField) + require.Equal(t, reflect.Ptr, field.Kind(), "descriptor field %q must be a pointer", handler.StructField) + var configValue any + if handler.NewConfig != nil { + configValue = handler.NewConfig() + constructorType := reflect.ValueOf(configValue).Type() + require.True(t, constructorType.AssignableTo(field.Type()), + "descriptor constructor for key %q returns %v, expected assignable to %v", key, constructorType, field.Type()) + } else { + configValue = reflect.New(field.Type().Elem()).Interface() + } + require.True(t, setSafeOutputField(cfg, handler.StructField, configValue), + "failed to set field %q for key %q from descriptor constructor", handler.StructField, key) + assert.Equal(t, configValue, field.Interface(), + "field %q should hold the descriptor constructor value for key %q", handler.StructField, key) - // Should return false when the field is nil (empty config) - assert.False(t, hasSafeOutputType(&SafeOutputsConfig{}, tt.key), "hasSafeOutputType should return false for key %q when field is nil", tt.key) + assert.True(t, hasSafeOutputType(cfg, key), "hasSafeOutputType should return true for key %q when field is set", key) + assert.False(t, hasSafeOutputType(&SafeOutputsConfig{}, key), "hasSafeOutputType should return false for key %q when field is nil", key) }) } } diff --git a/pkg/workflow/safe_outputs_import_test.go b/pkg/workflow/safe_outputs_import_test.go index fbe25369f9b..3239d1fd2d7 100644 --- a/pkg/workflow/safe_outputs_import_test.go +++ b/pkg/workflow/safe_outputs_import_test.go @@ -564,6 +564,36 @@ func TestMergeSafeOutputsUnit(t *testing.T) { } } +func TestMergeSafeOutputsDescriptorMergedFieldsUnit(t *testing.T) { + compiler := NewCompiler(WithVersion("1.0.0")) + + t.Run("imports unassign-from-user", func(t *testing.T) { + result, err := compiler.MergeSafeOutputs(nil, []string{ + `{"unassign-from-user":{"max":1}}`, + }, nil) + require.NoError(t, err) + require.NotNil(t, result) + require.NotNil(t, result.UnassignFromUser) + assert.Equal(t, strPtr("1"), result.UnassignFromUser.Max) + }) + + t.Run("imports dispatch_repository", func(t *testing.T) { + result, err := compiler.MergeSafeOutputs(nil, []string{ + `{"dispatch_repository":{"trigger_ci":{"workflow":"ci.yml","event_type":"ci_trigger","repository":"org/target-repo","max":1}}}`, + }, nil) + require.NoError(t, err) + require.NotNil(t, result) + require.NotNil(t, result.DispatchRepository) + require.Len(t, result.DispatchRepository.Tools, 1) + tool := result.DispatchRepository.Tools["trigger_ci"] + require.NotNil(t, tool) + assert.Equal(t, "ci.yml", tool.Workflow) + assert.Equal(t, "ci_trigger", tool.EventType) + assert.Equal(t, "org/target-repo", tool.Repository) + assert.Equal(t, strPtr("1"), tool.Max) + }) +} + // TestMergeSafeOutputsMessagesUnit tests the MergeSafeOutputs function for messages field func TestMergeSafeOutputsMessagesUnit(t *testing.T) { compiler := NewCompiler(WithVersion("1.0.0")) diff --git a/pkg/workflow/safe_outputs_permissions.go b/pkg/workflow/safe_outputs_permissions.go index 20435d569cd..6b12b3a313c 100644 --- a/pkg/workflow/safe_outputs_permissions.go +++ b/pkg/workflow/safe_outputs_permissions.go @@ -84,197 +84,18 @@ func ComputePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig) *Permissio permissions := NewPermissions() - // Merge permissions for all handler-managed types. - // Staged handlers are skipped because they do not make real API calls. - if safeOutputs.CreateIssues != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CreateIssues.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for create-issue") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.CreateDiscussions != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CreateDiscussions.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for create-discussion") - permissions.Merge(NewPermissionsContentsReadIssuesWriteDiscussionsWrite()) - } - if safeOutputs.AddComments != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.AddComments.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for add-comment") - permissions.Merge(buildAddCommentPermissions(safeOutputs.AddComments)) - } - if safeOutputs.CommentMemory != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CommentMemory.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for comment-memory") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.CloseIssues != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CloseIssues.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for close-issue") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.CloseDiscussions != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CloseDiscussions.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for close-discussion") - permissions.Merge(NewPermissionsContentsReadDiscussionsWrite()) - } - if safeOutputs.AddLabels != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.AddLabels.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for add-labels") - permissions.Merge(NewPermissionsContentsReadIssuesWritePRWrite()) - } - if safeOutputs.RemoveLabels != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.RemoveLabels.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for remove-labels") - permissions.Merge(NewPermissionsContentsReadIssuesWritePRWrite()) - } - if safeOutputs.UpdateIssues != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.UpdateIssues.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for update-issue") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.UpdateDiscussions != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.UpdateDiscussions.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for update-discussion") - permissions.Merge(NewPermissionsContentsReadDiscussionsWrite()) - } - if safeOutputs.LinkSubIssue != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.LinkSubIssue.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for link-sub-issue") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.UpdateRelease != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.UpdateRelease.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for update-release") - permissions.Merge(NewPermissionsContentsWrite()) - } - if (safeOutputs.CreatePullRequestReviewComments != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CreatePullRequestReviewComments.Staged)) || - (safeOutputs.SubmitPullRequestReview != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.SubmitPullRequestReview.Staged)) || - (safeOutputs.ReplyToPullRequestReviewComment != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.ReplyToPullRequestReviewComment.Staged)) || - (safeOutputs.ResolvePullRequestReviewThread != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.ResolvePullRequestReviewThread.Staged)) { - safeOutputsPermissionsLog.Print("Adding permissions for PR review operations") - permissions.Merge(NewPermissionsContentsReadPRWrite()) - } - if safeOutputs.CreatePullRequests != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CreatePullRequests.Staged) { - // Check fallback-as-issue setting to determine permissions - if getFallbackAsIssue(safeOutputs.CreatePullRequests) { - safeOutputsPermissionsLog.Print("Adding permissions for create-pull-request with fallback-as-issue") - permissions.Merge(NewPermissionsContentsWriteIssuesWritePRWrite()) - } else { - safeOutputsPermissionsLog.Print("Adding permissions for create-pull-request") - permissions.Merge(NewPermissionsContentsWritePRWrite()) - } - // Add workflows: write when allow-workflows is true (GitHub App-only permission) - if safeOutputs.CreatePullRequests.AllowWorkflows { - safeOutputsPermissionsLog.Print("Adding workflows: write for create-pull-request (allow-workflows: true)") - permissions.Set(PermissionWorkflows, PermissionWrite) - } - } - if safeOutputs.PushToPullRequestBranch != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.PushToPullRequestBranch.Staged) { - if getPushFallbackAsPullRequest(safeOutputs.PushToPullRequestBranch) { - safeOutputsPermissionsLog.Print("Adding permissions for push-to-pull-request-branch with fallback-as-pull-request") - permissions.Merge(NewPermissionsContentsWritePRWrite()) - } else { - safeOutputsPermissionsLog.Print("Adding permissions for push-to-pull-request-branch without fallback-as-pull-request") - permissions.Merge(NewPermissionsContentsWrite()) - } - // Add workflows: write when allow-workflows is true (GitHub App-only permission) - if safeOutputs.PushToPullRequestBranch.AllowWorkflows { - safeOutputsPermissionsLog.Print("Adding workflows: write for push-to-pull-request-branch (allow-workflows: true)") - permissions.Set(PermissionWorkflows, PermissionWrite) - } - // Add administration: read when check-branch-protection is enabled (GitHub App-only permission) - // The branch protection API requires administration: read for GitHub App tokens. - // Standard GITHUB_TOKEN lacks this scope; set it so the minted app token includes it. - if getCheckBranchProtection(safeOutputs.PushToPullRequestBranch) { - safeOutputsPermissionsLog.Print("Adding administration: read for push-to-pull-request-branch (check-branch-protection enabled)") - permissions.Set(PermissionAdministration, PermissionRead) - } - } - if safeOutputs.UpdatePullRequests != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.UpdatePullRequests.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for update-pull-request") - if safeOutputs.UpdatePullRequests.UpdateBranch != nil && *safeOutputs.UpdatePullRequests.UpdateBranch { - safeOutputsPermissionsLog.Print("update-pull-request has update-branch enabled; requiring contents: write") - permissions.Merge(NewPermissionsContentsWritePRWrite()) - } else { - permissions.Merge(NewPermissionsContentsReadPRWrite()) - } - } - if safeOutputs.MergePullRequest != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.MergePullRequest.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for merge-pull-request") - permissions.Merge(NewPermissionsContentsWritePRWrite()) - } - if safeOutputs.ClosePullRequests != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.ClosePullRequests.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for close-pull-request") - permissions.Merge(NewPermissionsContentsReadPRWrite()) - } - if safeOutputs.MarkPullRequestAsReadyForReview != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.MarkPullRequestAsReadyForReview.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for mark-pull-request-as-ready-for-review") - permissions.Merge(NewPermissionsContentsReadPRWrite()) - } - if safeOutputs.HideComment != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.HideComment.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for hide-comment") - // Check if discussions permission should be excluded (discussions: false) - // Default (nil or true) includes discussions:write for GitHub Apps with Discussions permission - // Note: Hiding comments (issue/PR/discussion) only needs issues:write, not pull_requests:write - if safeOutputs.HideComment.Discussions != nil && !*safeOutputs.HideComment.Discussions { - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } else { - permissions.Merge(NewPermissionsContentsReadIssuesWriteDiscussionsWrite()) + for _, handler := range safeOutputHandlers { + if handler.PermissionBuilder == nil { + continue } - } - if safeOutputs.DispatchWorkflow != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.DispatchWorkflow.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for dispatch-workflow") - permissions.Merge(NewPermissionsActionsWrite()) - } - // Project-related types - if safeOutputs.CreateProjects != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CreateProjects.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for create-project") - permissions.Merge(NewPermissionsContentsReadProjectsWrite()) - if current, exists := permissions.Get(PermissionIssues); !exists || current != PermissionWrite { - permissions.Set(PermissionIssues, PermissionRead) + handlerPermissions := handler.PermissionBuilder(safeOutputs) + if handlerPermissions == nil { + continue } - } - if safeOutputs.UpdateProjects != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.UpdateProjects.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for update-project") - permissions.Merge(NewPermissionsContentsReadProjectsWrite()) - if current, exists := permissions.Get(PermissionIssues); !exists || current != PermissionWrite { - permissions.Set(PermissionIssues, PermissionRead) + if handler.Key != "" { + safeOutputsPermissionsLog.Printf("Adding permissions for %s", handler.Key) } - } - if safeOutputs.CreateProjectStatusUpdates != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CreateProjectStatusUpdates.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for create-project-status-update") - permissions.Merge(NewPermissionsContentsReadProjectsWrite()) - } - if safeOutputs.AssignToAgent != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.AssignToAgent.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for assign-to-agent") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.CreateAgentSessions != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CreateAgentSessions.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for create-agent-session") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.CreateCodeScanningAlerts != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.CreateCodeScanningAlerts.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for create-code-scanning-alert") - permissions.Merge(NewPermissionsContentsReadSecurityEventsWrite()) - } - if safeOutputs.AutofixCodeScanningAlert != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.AutofixCodeScanningAlert.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for autofix-code-scanning-alert") - permissions.Merge(NewPermissionsContentsReadSecurityEventsWriteActionsRead()) - } - if safeOutputs.AssignToUser != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.AssignToUser.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for assign-to-user") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.UnassignFromUser != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.UnassignFromUser.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for unassign-from-user") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.AssignMilestone != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.AssignMilestone.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for assign-milestone") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.SetIssueType != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.SetIssueType.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for set-issue-type") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.SetIssueField != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.SetIssueField.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for set-issue-field") - permissions.Merge(NewPermissionsContentsReadIssuesWrite()) - } - if safeOutputs.AddReviewer != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.AddReviewer.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for add-reviewer") - permissions.Merge(NewPermissionsContentsReadPRWrite()) - } - if safeOutputs.UploadAssets != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.UploadAssets.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for upload-asset") - permissions.Merge(NewPermissionsContentsRead()) + permissions.Merge(handlerPermissions) } // NoOp and MissingTool don't require write permissions beyond what's already included @@ -313,77 +134,19 @@ func ComputePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig) *Permissio func SafeOutputsConfigFromKeys(keys []string) *SafeOutputsConfig { config := &SafeOutputsConfig{} for _, key := range keys { - switch key { - case "create-issue": - config.CreateIssues = &CreateIssuesConfig{} - case "create-agent-session": - config.CreateAgentSessions = &CreateAgentSessionConfig{} - case "create-discussion": - config.CreateDiscussions = &CreateDiscussionsConfig{} - case "update-discussion": - config.UpdateDiscussions = &UpdateDiscussionsConfig{} - case "close-discussion": - config.CloseDiscussions = &CloseDiscussionsConfig{} - case "add-comment": - config.AddComments = &AddCommentsConfig{} - case "comment-memory": - config.CommentMemory = &CommentMemoryConfig{} - case "close-issue": - config.CloseIssues = &CloseIssuesConfig{} - case "close-pull-request": - config.ClosePullRequests = &ClosePullRequestsConfig{} - case "create-pull-request": - config.CreatePullRequests = &CreatePullRequestsConfig{} - case "create-pull-request-review-comment": - config.CreatePullRequestReviewComments = &CreatePullRequestReviewCommentsConfig{} - case "submit-pull-request-review": - config.SubmitPullRequestReview = &SubmitPullRequestReviewConfig{} - case "reply-to-pull-request-review-comment": - config.ReplyToPullRequestReviewComment = &ReplyToPullRequestReviewCommentConfig{} - case "resolve-pull-request-review-thread": - config.ResolvePullRequestReviewThread = &ResolvePullRequestReviewThreadConfig{} - case "create-code-scanning-alert": - config.CreateCodeScanningAlerts = &CreateCodeScanningAlertsConfig{} - case "autofix-code-scanning-alert": - config.AutofixCodeScanningAlert = &AutofixCodeScanningAlertConfig{} - case "add-labels": - config.AddLabels = &AddLabelsConfig{} - case "remove-labels": - config.RemoveLabels = &RemoveLabelsConfig{} - case "add-reviewer": - config.AddReviewer = &AddReviewerConfig{} - case "assign-milestone": - config.AssignMilestone = &AssignMilestoneConfig{} - case "assign-to-agent": - config.AssignToAgent = &AssignToAgentConfig{} - case "assign-to-user": - config.AssignToUser = &AssignToUserConfig{} - case "unassign-from-user": - config.UnassignFromUser = &UnassignFromUserConfig{} - case "update-issue": - config.UpdateIssues = &UpdateIssuesConfig{} - case "update-pull-request": - config.UpdatePullRequests = &UpdatePullRequestsConfig{} - case "merge-pull-request": - config.MergePullRequest = &MergePullRequestConfig{} - case "push-to-pull-request-branch": - config.PushToPullRequestBranch = &PushToPullRequestBranchConfig{} - case "upload-asset": - config.UploadAssets = &UploadAssetsConfig{} - case "update-release": - config.UpdateRelease = &UpdateReleaseConfig{} - case "hide-comment": - config.HideComment = &HideCommentConfig{} - case "link-sub-issue": - config.LinkSubIssue = &LinkSubIssueConfig{} - case "update-project": - config.UpdateProjects = &UpdateProjectConfig{} - case "create-project": - config.CreateProjects = &CreateProjectsConfig{} - case "create-project-status-update": - config.CreateProjectStatusUpdates = &CreateProjectStatusUpdateConfig{} - case "mark-pull-request-as-ready-for-review": - config.MarkPullRequestAsReadyForReview = &MarkPullRequestAsReadyForReviewConfig{} + handler, ok := getSafeOutputHandlerByKey(key) + if !ok || handler.NewConfig == nil { + continue + } + if hasSafeOutputFieldSet(config, handler.StructField) { + continue + } + if !setSafeOutputField(config, handler.StructField, handler.NewConfig()) { + safeOutputsPermissionsLog.Printf( + "Warning: failed to set safe-output field %q for key %q from descriptor constructor", + handler.StructField, + key, + ) } } return config diff --git a/pkg/workflow/safe_outputs_state.go b/pkg/workflow/safe_outputs_state.go index 456e65b3850..f136cc01a2c 100644 --- a/pkg/workflow/safe_outputs_state.go +++ b/pkg/workflow/safe_outputs_state.go @@ -13,62 +13,15 @@ import ( // hasNonBuiltinSafeOutputsEnabled use direct nil-checks instead of reflection // for performance (these functions are called on every compilation). // -// NOTE: When adding a new pointer field to SafeOutputsConfig that represents -// a user-facing safe output action, add it to ALL of the following locations: -// 1. safeOutputFieldMapping (below) — drives imports, prompt generation, etc. -// 2. hasAnySafeOutputEnabled — performance-critical hot path -// 3. hasNonBuiltinSafeOutputsEnabled — if it is NOT a builtin (noop/missing-*) -// 4. hasSafeOutputType in imports.go — used for conflict detection +// 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() // hasAnySafeOutputEnabled reports whether any safe output field is non-nil. // It uses direct struct-field nil checks instead of reflection for performance; diff --git a/pkg/workflow/safe_outputs_state_test.go b/pkg/workflow/safe_outputs_state_test.go index 74d8f1bbea3..04927f09768 100644 --- a/pkg/workflow/safe_outputs_state_test.go +++ b/pkg/workflow/safe_outputs_state_test.go @@ -11,39 +11,38 @@ import ( ) // TestSafeOutputStateFieldCoverage verifies that hasAnySafeOutputEnabled and -// hasNonBuiltinSafeOutputsEnabled cover every pointer field listed in -// safeOutputFieldMapping. This acts as a regression guard to ensure that when -// a new safe output type is added to safeOutputFieldMapping, the developer is -// reminded (via a failing test) to also update the two direct-check functions. +// hasNonBuiltinSafeOutputsEnabled cover every descriptor-managed pointer field. +// This acts as a regression guard to ensure that when a new safe output +// descriptor is added, the developer is reminded (via a failing test) to also +// update the two direct-check functions. func TestSafeOutputStateFieldCoverage(t *testing.T) { - // builtins excluded from hasNonBuiltinSafeOutputsEnabled - builtins := map[string]bool{ - "NoOp": true, - "MissingData": true, - "MissingTool": true, - } - - for fieldName := range safeOutputFieldMapping { - t.Run(fieldName, func(t *testing.T) { + for _, handler := range safeOutputHandlers { + if handler.StructField == "ReportIncomplete" || handler.StructField == "ThreatDetection" { + // These are auto-defaulted policy controls, not action handlers. They are + // intentionally excluded from hasAnySafeOutputEnabled/hasNonBuiltinSafeOutputsEnabled + // so defaults don't count as explicit user-enabled safe outputs. + continue + } + t.Run(handler.StructField, func(t *testing.T) { // Build a SafeOutputsConfig with only this one field set to a non-nil value. cfg := &SafeOutputsConfig{} val := reflect.ValueOf(cfg).Elem() - field := val.FieldByName(fieldName) + field := val.FieldByName(handler.StructField) require.True(t, field.IsValid(), - "safeOutputFieldMapping references unknown struct field %q; update the mapping or the struct", fieldName) + "safeOutputHandlers references unknown struct field %q; update descriptors or struct", handler.StructField) require.Equal(t, reflect.Ptr, field.Kind(), - "safeOutputFieldMapping field %q is expected to be a pointer type", fieldName) + "safeOutputHandlers field %q is expected to be a pointer type", handler.StructField) field.Set(reflect.New(field.Type().Elem())) - // hasAnySafeOutputEnabled must return true for every field in the mapping. + // hasAnySafeOutputEnabled must return true for every descriptor field. assert.True(t, hasAnySafeOutputEnabled(cfg), - "hasAnySafeOutputEnabled missing check for field %q; add it to the direct nil-check list", fieldName) + "hasAnySafeOutputEnabled missing check for field %q; add it to the direct nil-check list", handler.StructField) // hasNonBuiltinSafeOutputsEnabled must return true for every non-builtin field. - if !builtins[fieldName] { + if !handler.Builtin { assert.True(t, hasNonBuiltinSafeOutputsEnabled(cfg), - "hasNonBuiltinSafeOutputsEnabled missing check for non-builtin field %q; add it to the direct nil-check list", fieldName) + "hasNonBuiltinSafeOutputsEnabled missing check for non-builtin field %q; add it to the direct nil-check list", handler.StructField) } }) }