Skip to content

perf: eliminate reflection in validateSafeOutputsMax (4.3x faster) #30701

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 3 commits into from
May 7, 2026
+273 −48
Merged

perf: eliminate reflection in validateSafeOutputsMax (4.3x faster) #30701

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ddb2533
Initial plan
Copilot May 6, 2026
698525e
fix: replace reflection in validateSafeOutputsMax with direct field a…
Copilot May 6, 2026
4e7eaac
fix: use f-variants of testify assertions in coverage test
Copilot May 6, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
276 changes: 228 additions & 48 deletions pkg/workflow/safe_outputs_max_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,26 +2,13 @@ package workflow

import (
"fmt"
"reflect"
"sort"
"strconv"
"strings"
)

var safeOutputsMaxValidationLog = newValidationLogger("safe_outputs_max")

// sortedSafeOutputMaxFieldNames is the pre-sorted list of safeOutputFieldMapping keys used
// by validateSafeOutputsMax for deterministic error reporting. Pre-computing this slice at
// init time avoids a make+sort allocation on every validateSafeOutputsMax call.
var sortedSafeOutputMaxFieldNames = func() []string {
names := make([]string, 0, len(safeOutputFieldMapping))
for fieldName := range safeOutputFieldMapping {
names = append(names, fieldName)
}
sort.Strings(names)
return names
}()

// isInvalidMaxValue returns true if n is not a valid max field value.
// Valid values are positive integers (n > 0) or -1 (unlimited).
// Invalid values are 0 and negative integers except -1.
Expand All @@ -35,58 +22,251 @@ func isInvalidMaxValue(n int) bool {
// maxInvalidErrSuffix is the common suffix of max validation error messages.
const maxInvalidErrSuffix = "\n\nThe max field controls how many times this safe output can be triggered.\nProvide a positive integer (e.g., max: 1 or max: 5) or -1 for unlimited"

// checkMaxField validates a single safe-output max field value.
// Returns an error if the max value is invalid (0 or negative, except -1).
// Returns nil if the max pointer is nil, the value is an expression, or is valid.
func checkMaxField(toolName string, maxPtr *string) error {
if maxPtr == nil || isExpression(*maxPtr) {
return nil
}
n, err := strconv.Atoi(*maxPtr)
if err != nil {
return nil
}
if isInvalidMaxValue(n) {
toolDisplayName := strings.ReplaceAll(toolName, "_", "-")
safeOutputsMaxValidationLog.Printf("Invalid max value %d for %s", n, toolDisplayName)
return fmt.Errorf(
"safe-outputs.%s: max must be a positive integer or -1 (unlimited), got %d%s",
toolDisplayName, n, maxInvalidErrSuffix,
)
}
return nil
}

// validateSafeOutputsMax validates that all max fields in safe-outputs configs hold valid values.
// Valid values are positive integers (n > 0) or -1 (unlimited per spec).
// 0 and other negative values are rejected.
// GitHub Actions expressions (e.g. "${{ inputs.max }}") are not evaluable at compile time
// and are therefore skipped.
//
// This function uses direct struct field access instead of reflection for performance;
// it is on the hot path and called on every compilation. The field ordering matches
// the sorted safeOutputFieldMapping keys for deterministic error reporting.
func validateSafeOutputsMax(config *SafeOutputsConfig) error {
if config == nil {
return nil
}

safeOutputsMaxValidationLog.Print("Validating safe-outputs max fields")

val := reflect.ValueOf(config).Elem()

// Iterate over sorted field names for deterministic error reporting.
// sortedSafeOutputMaxFieldNames is pre-computed at init time to avoid a
// make+sort allocation on every call (performance-critical hot path).
for _, fieldName := range sortedSafeOutputMaxFieldNames {
toolName := safeOutputFieldMapping[fieldName]
field := val.FieldByName(fieldName)
if !field.IsValid() || field.IsNil() {
continue
// Direct field access — no reflection, no heap allocation.
// Fields are checked in the alphabetical order of their struct field names,
// matching the sort order of safeOutputFieldMapping keys for deterministic
// error reporting.
if config.AddComments != nil {
if err := checkMaxField("add_comment", config.AddComments.Max); err != nil {
return err
}

elem := field.Elem()
baseCfgField := elem.FieldByName("BaseSafeOutputConfig")
if !baseCfgField.IsValid() {
continue
}
if config.AddLabels != nil {
if err := checkMaxField("add_labels", config.AddLabels.Max); err != nil {
return err
}

maxField := baseCfgField.FieldByName("Max")
if !maxField.IsValid() || maxField.IsNil() {
continue
}
if config.AddReviewer != nil {
if err := checkMaxField("add_reviewer", config.AddReviewer.Max); err != nil {
return err
}

maxPtr, ok := maxField.Interface().(*string)
if !ok || maxPtr == nil || isExpression(*maxPtr) {
continue
}
if config.AssignMilestone != nil {
if err := checkMaxField("assign_milestone", config.AssignMilestone.Max); err != nil {
return err
}

n, err := strconv.Atoi(*maxPtr)
if err != nil {
continue
}
if config.AssignToAgent != nil {
if err := checkMaxField("assign_to_agent", config.AssignToAgent.Max); err != nil {
return err
}

if isInvalidMaxValue(n) {
toolDisplayName := strings.ReplaceAll(toolName, "_", "-")
safeOutputsMaxValidationLog.Printf("Invalid max value %d for %s", n, toolDisplayName)
return fmt.Errorf(
"safe-outputs.%s: max must be a positive integer or -1 (unlimited), got %d%s",
toolDisplayName, n, maxInvalidErrSuffix,
)
}
if config.AssignToUser != nil {
if err := checkMaxField("assign_to_user", config.AssignToUser.Max); err != nil {
return err
}
}
if config.AutofixCodeScanningAlert != nil {
if err := checkMaxField("autofix_code_scanning_alert", config.AutofixCodeScanningAlert.Max); err != nil {
return err
}
}
if config.CallWorkflow != nil {
if err := checkMaxField("call_workflow", config.CallWorkflow.Max); err != nil {
return err
}
}
if config.CloseDiscussions != nil {
if err := checkMaxField("close_discussion", config.CloseDiscussions.Max); err != nil {
return err
}
}
if config.CloseIssues != nil {
if err := checkMaxField("close_issue", config.CloseIssues.Max); err != nil {
return err
}
}
if config.ClosePullRequests != nil {
if err := checkMaxField("close_pull_request", config.ClosePullRequests.Max); err != nil {
return err
}
}
if config.CreateAgentSessions != nil {
if err := checkMaxField("create_agent_session", config.CreateAgentSessions.Max); err != nil {
return err
}
}
if config.CreateCodeScanningAlerts != nil {
if err := checkMaxField("create_code_scanning_alert", config.CreateCodeScanningAlerts.Max); err != nil {
return err
}
}
if config.CreateDiscussions != nil {
if err := checkMaxField("create_discussion", config.CreateDiscussions.Max); err != nil {
return err
}
}
if config.CreateIssues != nil {
if err := checkMaxField("create_issue", config.CreateIssues.Max); err != nil {
return err
}
}
if config.CreateProjectStatusUpdates != nil {
if err := checkMaxField("create_project_status_update", config.CreateProjectStatusUpdates.Max); err != nil {
return err
}
}
if config.CreateProjects != nil {
if err := checkMaxField("create_project", config.CreateProjects.Max); err != nil {
return err
}
}
if config.CreatePullRequestReviewComments != nil {
if err := checkMaxField("create_pull_request_review_comment", config.CreatePullRequestReviewComments.Max); err != nil {
return err
}
}
if config.CreatePullRequests != nil {
if err := checkMaxField("create_pull_request", config.CreatePullRequests.Max); err != nil {
return err
}
}
if config.DispatchWorkflow != nil {
if err := checkMaxField("dispatch_workflow", config.DispatchWorkflow.Max); err != nil {
return err
}
}
if config.HideComment != nil {
if err := checkMaxField("hide_comment", config.HideComment.Max); err != nil {
return err
}
}
if config.LinkSubIssue != nil {
if err := checkMaxField("link_sub_issue", config.LinkSubIssue.Max); err != nil {
return err
}
}
if config.MarkPullRequestAsReadyForReview != nil {
if err := checkMaxField("mark_pull_request_as_ready_for_review", config.MarkPullRequestAsReadyForReview.Max); err != nil {
return err
}
}
if config.MergePullRequest != nil {
if err := checkMaxField("merge_pull_request", config.MergePullRequest.Max); err != nil {
return err
}
}
if config.MissingData != nil {
if err := checkMaxField("missing_data", config.MissingData.Max); err != nil {
return err
}
}
if config.MissingTool != nil {
if err := checkMaxField("missing_tool", config.MissingTool.Max); err != nil {
return err
}
}
if config.NoOp != nil {
if err := checkMaxField("noop", config.NoOp.Max); err != nil {
return err
}
}
if config.PushToPullRequestBranch != nil {
if err := checkMaxField("push_to_pull_request_branch", config.PushToPullRequestBranch.Max); err != nil {
return err
}
}
if config.RemoveLabels != nil {
if err := checkMaxField("remove_labels", config.RemoveLabels.Max); err != nil {
return err
}
}
if config.ReplyToPullRequestReviewComment != nil {
if err := checkMaxField("reply_to_pull_request_review_comment", config.ReplyToPullRequestReviewComment.Max); err != nil {
return err
}
}
if config.ResolvePullRequestReviewThread != nil {
if err := checkMaxField("resolve_pull_request_review_thread", config.ResolvePullRequestReviewThread.Max); err != nil {
return err
}
}
if config.SetIssueType != nil {
if err := checkMaxField("set_issue_type", config.SetIssueType.Max); err != nil {
return err
}
}
if config.SubmitPullRequestReview != nil {
if err := checkMaxField("submit_pull_request_review", config.SubmitPullRequestReview.Max); err != nil {
return err
}
}
if config.UnassignFromUser != nil {
if err := checkMaxField("unassign_from_user", config.UnassignFromUser.Max); err != nil {
return err
}
}
if config.UpdateDiscussions != nil {
if err := checkMaxField("update_discussion", config.UpdateDiscussions.Max); err != nil {
return err
}
}
if config.UpdateIssues != nil {
if err := checkMaxField("update_issue", config.UpdateIssues.Max); err != nil {
return err
}
}
if config.UpdateProjects != nil {
if err := checkMaxField("update_project", config.UpdateProjects.Max); err != nil {
return err
}
}
if config.UpdatePullRequests != nil {
if err := checkMaxField("update_pull_request", config.UpdatePullRequests.Max); err != nil {
return err
}
}
if config.UpdateRelease != nil {
if err := checkMaxField("update_release", config.UpdateRelease.Max); err != nil {
return err
}
}
if config.UploadArtifact != nil {
if err := checkMaxField("upload_artifact", config.UploadArtifact.Max); err != nil {
return err
}
}
if config.UploadAssets != nil {
if err := checkMaxField("upload_asset", config.UploadAssets.Max); err != nil {
return err
}
}

Expand Down
45 changes: 45 additions & 0 deletions pkg/workflow/safe_outputs_max_validation_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package workflow

import (
"reflect"
"testing"

"github.com/stretchr/testify/assert"
Expand Down Expand Up @@ -164,6 +165,50 @@ func TestValidateSafeOutputsMax(t *testing.T) {
})
}

// TestValidateSafeOutputsMaxFieldCoverage verifies that validateSafeOutputsMax detects
// invalid max values for every field listed in safeOutputFieldMapping (except
// DispatchRepository, which has a different map-of-tools structure and is validated
// separately). This acts as a regression guard to ensure that when a new safe output
// type is added to safeOutputFieldMapping the developer also adds a direct-access
// check to validateSafeOutputsMax.
func TestValidateSafeOutputsMaxFieldCoverage(t *testing.T) {
invalidMax := strPtr("0") // 0 is always an invalid max value

for fieldName, toolName := range safeOutputFieldMapping {
if fieldName == "DispatchRepository" {
// DispatchRepository uses a map-of-tools structure and is validated
// separately at the end of validateSafeOutputsMax.
continue
}

t.Run(fieldName, func(t *testing.T) {
cfg := &SafeOutputsConfig{}
val := reflect.ValueOf(cfg).Elem()
field := val.FieldByName(fieldName)
require.Truef(t, field.IsValid(),
"safeOutputFieldMapping references unknown struct field %q", fieldName)
require.Equalf(t, reflect.Ptr, field.Kind(),
"safeOutputFieldMapping field %q is expected to be a pointer type", fieldName)

// Create a zero-value instance of the field's element type and set Max to an invalid value.
elem := reflect.New(field.Type().Elem())
baseCfgField := elem.Elem().FieldByName("BaseSafeOutputConfig")
require.Truef(t, baseCfgField.IsValid(),
"field %q does not embed BaseSafeOutputConfig — add a direct check in validateSafeOutputsMax", fieldName)
maxField := baseCfgField.FieldByName("Max")
require.Truef(t, maxField.IsValid(), "BaseSafeOutputConfig.Max field not found for field %q", fieldName)
maxField.Set(reflect.ValueOf(invalidMax))
field.Set(elem)

err := validateSafeOutputsMax(cfg)
require.Errorf(t, err,
"validateSafeOutputsMax should detect invalid max for field %q (tool: %q); add a direct-access check", fieldName, toolName)
assert.Containsf(t, err.Error(), "max must be a positive integer or -1",
"error for field %q should explain valid values", fieldName)
})
}
}

func TestValidateSafeOutputsMaxIntegration(t *testing.T) {
compiler := &Compiler{}

Expand Down