Skip to content

perf: eliminate repeated O(n) action pin scans and redundant permissions parsing in MCP workflow compilation #24256

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 2 commits into from
Apr 3, 2026
+69 −60
Merged

perf: eliminate repeated O(n) action pin scans and redundant permissions parsing in MCP workflow compilation #24256

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
70d2df8
Initial plan
Copilot Apr 3, 2026
6f521c6
perf: optimize action pin lookup and reduce redundant permissions par…
Copilot Apr 3, 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
99 changes: 58 additions & 41 deletions pkg/workflow/action_pins.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ import (

"github.com/github/gh-aw/pkg/console"
"github.com/github/gh-aw/pkg/logger"
"github.com/github/gh-aw/pkg/sliceutil"
)

var actionPinsLog = logger.New("workflow:action_pins")
Expand Down Expand Up @@ -48,6 +47,9 @@ type ActionPinsData struct {
var (
// cachedActionPins holds the parsed and sorted action pins
cachedActionPins []ActionPin
// cachedActionPinsByRepo maps repo name to its sorted (version-descending) pins.
// Built alongside cachedActionPins to enable O(1) repo lookups without linear scans.
cachedActionPinsByRepo map[string][]ActionPin
// actionPinsOnce ensures the action pins are loaded only once
actionPinsOnce sync.Once
)
Expand Down Expand Up @@ -106,11 +108,42 @@ func getActionPins() []ActionPin {

actionPinsLog.Printf("Successfully unmarshaled and sorted %d action pins from JSON", len(pins))
cachedActionPins = pins

// Build per-repo index for O(1) lookups.
// Each repo's slice is sorted by semantic version (descending) so callers can
// pick the highest compatible version without an additional sort pass.
byRepo := make(map[string][]ActionPin, len(pins))
for _, pin := range pins {
byRepo[pin.Repo] = append(byRepo[pin.Repo], pin)
}
// The global sort above orders all pins together, so grouping them by repo
// does not guarantee per-repo ordering. Re-sort each repo's slice using the
// semantic comparator (compareVersions) so that the index matches what
// sortPinsByVersion would return for any subset of pins.
for repo, repoPins := range byRepo {
sort.Slice(repoPins, func(i, j int) bool {
v1 := strings.TrimPrefix(repoPins[i].Version, "v")
v2 := strings.TrimPrefix(repoPins[j].Version, "v")
return compareVersions(v1, v2) > 0
})
byRepo[repo] = repoPins
}
cachedActionPinsByRepo = byRepo
actionPinsLog.Printf("Built per-repo action pin index for %d repos", len(byRepo))
})

return cachedActionPins
}

// getActionPinsByRepo returns the sorted (version-descending) list of action pins
// for the given repository, initialising the cache on first call.
// Returns nil if the repo has no pins.
func getActionPinsByRepo(repo string) []ActionPin {
// Ensure the cache is initialised (both slices are populated in one sync.Once call).
getActionPins()
return cachedActionPinsByRepo[repo]
}

// sortPinsByVersion sorts action pins by version in descending order (highest first).
// This function returns a new sorted slice without modifying the input.
// This is an immutable operation for better safety and clarity.
Expand All @@ -135,23 +168,16 @@ func sortPinsByVersion(pins []ActionPin) []ActionPin {
// If no pin is found, it returns an empty string
// The returned reference includes a comment with the version tag (e.g., "repo@sha # v1")
func GetActionPin(actionRepo string) string {
actionPins := getActionPins()

// Find all pins matching the repo - using functional filter
matchingPins := sliceutil.Filter(actionPins, func(pin ActionPin) bool {
return pin.Repo == actionRepo
})
// Use the pre-built per-repo index for O(1) lookup (avoids scanning all pins).
matchingPins := getActionPinsByRepo(actionRepo)

if len(matchingPins) == 0 {
// If no pin exists, return empty string to signal that this action is not pinned
return ""
}

// Sort matching pins by version (descending - latest first) - immutable operation
sortedPins := sortPinsByVersion(matchingPins)

// Return the latest version (first after sorting)
latestPin := sortedPins[0]
// The per-repo slice is already sorted by version descending; pick the first entry.
latestPin := matchingPins[0]
return formatActionReference(actionRepo, latestPin.SHA, latestPin.Version)
}

Expand Down Expand Up @@ -187,23 +213,18 @@ func GetActionPinWithData(actionRepo, version string, data *WorkflowData) (strin
}
}

// Dynamic resolution failed, try hardcoded pins
// Dynamic resolution failed, try hardcoded pins.
// Use the pre-built per-repo index for O(1) lookup (avoids scanning all pins).
actionPinsLog.Printf("Falling back to hardcoded pins for %s@%s", actionRepo, version)
actionPins := getActionPins()

// Find all pins matching the repo - using functional filter
matchingPins := sliceutil.Filter(actionPins, func(pin ActionPin) bool {
return pin.Repo == actionRepo
})
matchingPins := getActionPinsByRepo(actionRepo)

if len(matchingPins) == 0 {
// No pins found for this repo, will handle below
actionPinsLog.Printf("No hardcoded pins found for %s", actionRepo)
} else {
actionPinsLog.Printf("Found %d hardcoded pin(s) for %s", len(matchingPins), actionRepo)

// Sort matching pins by version (descending - highest first) - immutable operation
matchingPins = sortPinsByVersion(matchingPins)
// The per-repo slice is already sorted by version descending; no extra sort needed.

// First, try to find an exact version match (for version tags)
for _, pin := range matchingPins {
Expand Down Expand Up @@ -231,18 +252,21 @@ func GetActionPinWithData(actionRepo, version string, data *WorkflowData) (strin
// Semver compatibility means respecting major version boundaries
// (e.g., v5 -> highest v5.x.x, not v6.x.x)
if !data.StrictMode && len(matchingPins) > 0 {
// Filter for semver-compatible pins (matching major version) - using functional filter
compatiblePins := sliceutil.Filter(matchingPins, func(pin ActionPin) bool {
return isSemverCompatible(pin.Version, version)
})

// If we found compatible pins, use the highest one (first after sorting)
// Otherwise fall back to the highest overall pin
// Find the first semver-compatible pin (already sorted descending, so first match is highest)
var selectedPin ActionPin
if len(compatiblePins) > 0 {
selectedPin = compatiblePins[0]
foundCompatible := false
for _, pin := range matchingPins {
if isSemverCompatible(pin.Version, version) {
selectedPin = pin
foundCompatible = true
break
}
}

if foundCompatible {
actionPinsLog.Printf("No exact match for version %s, using highest semver-compatible version: %s", version, selectedPin.Version)
} else {
// Fall back to the highest overall pin
selectedPin = matchingPins[0]
actionPinsLog.Printf("No exact match for version %s, no semver-compatible versions found, using highest available: %s", version, selectedPin.Version)
}
Expand Down Expand Up @@ -409,20 +433,13 @@ func ApplyActionPinsToTypedSteps(steps []*WorkflowStep, data *WorkflowData) []*W
// GetActionPinByRepo returns the ActionPin for a given repository, if it exists
// When multiple versions exist for the same repo, it returns the latest version by semver
func GetActionPinByRepo(repo string) (ActionPin, bool) {
actionPins := getActionPins()

// Find all pins matching the repo - using functional filter
matchingPins := sliceutil.Filter(actionPins, func(pin ActionPin) bool {
return pin.Repo == repo
})
// Use the pre-built per-repo index for O(1) lookup (avoids scanning all pins).
matchingPins := getActionPinsByRepo(repo)

if len(matchingPins) == 0 {
return ActionPin{}, false
}

// Sort matching pins by version (descending - latest first) - immutable operation
sortedPins := sortPinsByVersion(matchingPins)

// Return the latest version (first after sorting)
return sortedPins[0], true
// The per-repo slice is already sorted by version descending; pick the first entry.
return matchingPins[0], true
}
30 changes: 11 additions & 19 deletions pkg/workflow/compiler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 +319,11 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
return err
}

// Parse permissions once for reuse in the three validation checks below.
// WorkflowData.Permissions contains the raw YAML string (including "permissions:" prefix).
// Always produces a non-nil *Permissions (ToPermissions never returns nil).
cachedPermissions := NewPermissionsParser(workflowData.Permissions).ToPermissions()

// Validate permissions against GitHub MCP toolsets
log.Printf("Validating permissions for GitHub MCP toolsets")
if workflowData.ParsedTools != nil && workflowData.ParsedTools.GitHub != nil {
Expand All @@ -334,12 +339,8 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath
if hasPermissions && !workflowData.HasExplicitGitHubTool {
log.Printf("Skipping permission validation: permissions exist but tools.github not explicitly configured")
} else {
// Parse permissions from the workflow data
// WorkflowData.Permissions contains the raw YAML string (including "permissions:" prefix)
permissions := NewPermissionsParser(workflowData.Permissions).ToPermissions()

// Validate permissions using the typed GitHub tool configuration
validationResult := ValidatePermissions(permissions, workflowData.ParsedTools.GitHub)
validationResult := ValidatePermissions(cachedPermissions, workflowData.ParsedTools.GitHub)

if validationResult.HasValidationIssues {
// Format the validation message
Expand All @@ -361,18 +362,12 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath

// Emit warning if id-token: write permission is detected
log.Printf("Checking for id-token: write permission")
if workflowData.Permissions != "" {
permissions := NewPermissionsParser(workflowData.Permissions).ToPermissions()
if permissions != nil {
level, exists := permissions.Get(PermissionIdToken)
if exists && level == PermissionWrite {
warningMsg := `This workflow grants id-token: write permission
if level, exists := cachedPermissions.Get(PermissionIdToken); exists && level == PermissionWrite {
warningMsg := `This workflow grants id-token: write permission
OIDC tokens can authenticate to cloud providers (AWS, Azure, GCP).
Ensure proper audience validation and trust policies are configured.`
fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", warningMsg))
c.IncrementWarningCount()
}
}
fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", warningMsg))
c.IncrementWarningCount()
}

// Validate GitHub tools against enabled toolsets
Expand All @@ -399,11 +394,8 @@ Ensure proper audience validation and trust policies are configured.`
// Validate permissions for agentic-workflows tool
log.Printf("Validating permissions for agentic-workflows tool")
if _, hasAgenticWorkflows := workflowData.Tools["agentic-workflows"]; hasAgenticWorkflows {
// Parse permissions from the workflow data
permissions := NewPermissionsParser(workflowData.Permissions).ToPermissions()

// Check if actions: read permission exists
actionsLevel, hasActions := permissions.Get(PermissionActions)
actionsLevel, hasActions := cachedPermissions.Get(PermissionActions)
if !hasActions || actionsLevel == PermissionNone {
// Missing actions: read permission
message := "ERROR: Missing required permission for agentic-workflows tool:\n"
Expand Down
Loading