diff --git a/pkg/workflow/action_pins.go b/pkg/workflow/action_pins.go index 043efc10d5..b0123a2ebe 100644 --- a/pkg/workflow/action_pins.go +++ b/pkg/workflow/action_pins.go @@ -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") @@ -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 ) @@ -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. @@ -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) } @@ -187,14 +213,10 @@ 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 @@ -202,8 +224,7 @@ func GetActionPinWithData(actionRepo, version string, data *WorkflowData) (strin } 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 { @@ -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) } @@ -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 } diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 3177f9b449..37caf1d84b 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -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 { @@ -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 @@ -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 @@ -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"