From c19fe95da8f25606736daae56fbc6a3cc7d9b989 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 1 Jun 2026 14:41:04 +0000 Subject: [PATCH 1/5] Initial plan From 682b1c5a11372438bf53fea5f2e3d231f78da621 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:09:18 +0000 Subject: [PATCH 2/5] refactor workflow helpers for cache actions and validation Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/workflow/action_cache.go | 124 ++++--- pkg/workflow/action_reference.go | 249 ++++++------- pkg/workflow/action_sha_checker.go | 79 ++-- pkg/workflow/agent_validation.go | 76 ++-- pkg/workflow/cache.go | 436 +++++++++++++---------- pkg/workflow/call_workflow_validation.go | 175 ++++----- 6 files changed, 567 insertions(+), 572 deletions(-) diff --git a/pkg/workflow/action_cache.go b/pkg/workflow/action_cache.go index 6b2f5a0ead5..d89cdd78127 100644 --- a/pkg/workflow/action_cache.go +++ b/pkg/workflow/action_cache.go @@ -488,81 +488,91 @@ func (c *ActionCache) GetCachePath() string { // for each repo+SHA combination. For example, if both "actions/cache@v4" and "actions/cache@v4.3.0" // point to the same SHA and version, only "actions/cache@v4.3.0" is kept. func (c *ActionCache) deduplicateEntries() { - // Group entries by repo+SHA - type entryKey struct { - repo string - sha string - } - groups := make(map[entryKey][]string) - - for key, entry := range c.Entries { - ek := entryKey{repo: entry.Repo, sha: entry.SHA} - groups[ek] = append(groups[ek], key) - } - - // For each group with multiple entries, keep only the most precise one + groups := c.groupEntriesByRepoAndSHA() var toDelete []string - var deduplicationDetails []string // Track details for user-friendly message - + var deduplicationDetails []string for ek, keys := range groups { if len(keys) <= 1 { continue } - - // Truncate SHA for logging (handle short SHAs in tests) - shortSHA := ek.sha - if len(ek.sha) > 8 { - shortSHA = ek.sha[:8] + actionCacheLog.Printf("Found %d cache entries for %s with SHA %s", len(keys), ek.repo, truncateSHAForLog(ek.sha)) + keyInfos := buildDedupKeyInfos(keys) + if len(keyInfos) <= 1 { + continue } - actionCacheLog.Printf("Found %d cache entries for %s with SHA %s", len(keys), ek.repo, shortSHA) - - // Find the most precise version reference - // Extract the version reference from each key (format: "repo@versionRef") - type keyInfo struct { - key string - versionRef string + keepVersion, removedKeys, removedVersions := collectDedupRemovals(keyInfos) + for _, removedKey := range removedKeys { + toDelete = append(toDelete, removedKey) + actionCacheLog.Printf("Deduplicating: keeping %s, removing %s", keyInfos[0].key, removedKey) } - keyInfos := make([]keyInfo, len(keys)) - for i, key := range keys { - parts := strings.SplitN(key, "@", 2) - versionRef := "" - if len(parts) == 2 { - versionRef = parts[1] - } - keyInfos[i] = keyInfo{key: key, versionRef: versionRef} + deduplicationDetails = append(deduplicationDetails, fmt.Sprintf("%s: kept %s, removed %s", ek.repo, keepVersion, strings.Join(removedVersions, ", "))) + } + c.deleteDedupEntries(toDelete) + if len(toDelete) > 0 { + actionCacheLog.Printf("Deduplicated %d entries, %d entries remaining", len(toDelete), len(c.Entries)) + for _, detail := range deduplicationDetails { + actionCacheLog.Printf("Deduplication detail: %s", detail) } + } +} - // Sort by version precision (most precise first) - sort.Slice(keyInfos, func(i, j int) bool { - return isMorePreciseVersion(keyInfos[i].versionRef, keyInfos[j].versionRef) - }) - - // Keep the most precise version, mark others for deletion - keepVersion := keyInfos[0].versionRef - var removedVersions []string - for i := 1; i < len(keyInfos); i++ { - toDelete = append(toDelete, keyInfos[i].key) - removedVersions = append(removedVersions, keyInfos[i].versionRef) - actionCacheLog.Printf("Deduplicating: keeping %s, removing %s", keyInfos[0].key, keyInfos[i].key) +type deduplicationKey struct { + repo string + sha string +} + +type cacheKeyInfo struct { + key string + versionRef string +} + +func (c *ActionCache) groupEntriesByRepoAndSHA() map[deduplicationKey][]string { + groups := make(map[deduplicationKey][]string) + for key, entry := range c.Entries { + ek := deduplicationKey{repo: entry.Repo, sha: entry.SHA} + groups[ek] = append(groups[ek], key) + } + return groups +} + +func buildDedupKeyInfos(keys []string) []cacheKeyInfo { + keyInfos := make([]cacheKeyInfo, len(keys)) + for i, key := range keys { + parts := strings.SplitN(key, "@", 2) + versionRef := "" + if len(parts) == 2 { + versionRef = parts[1] } + keyInfos[i] = cacheKeyInfo{key: key, versionRef: versionRef} + } + sort.Slice(keyInfos, func(i, j int) bool { + return isMorePreciseVersion(keyInfos[i].versionRef, keyInfos[j].versionRef) + }) + return keyInfos +} - // Build user-friendly message - detail := fmt.Sprintf("%s: kept %s, removed %s", ek.repo, keepVersion, strings.Join(removedVersions, ", ")) - deduplicationDetails = append(deduplicationDetails, detail) +func collectDedupRemovals(keyInfos []cacheKeyInfo) (string, []string, []string) { + keepVersion := keyInfos[0].versionRef + removedKeys := make([]string, 0, len(keyInfos)-1) + removedVersions := make([]string, 0, len(keyInfos)-1) + for i := 1; i < len(keyInfos); i++ { + removedKeys = append(removedKeys, keyInfos[i].key) + removedVersions = append(removedVersions, keyInfos[i].versionRef) } + return keepVersion, removedKeys, removedVersions +} - // Delete the less precise entries +func (c *ActionCache) deleteDedupEntries(toDelete []string) { for _, key := range toDelete { delete(c.Entries, key) } +} - if len(toDelete) > 0 { - actionCacheLog.Printf("Deduplicated %d entries, %d entries remaining", len(toDelete), len(c.Entries)) - // Log detailed deduplication info at verbose level - for _, detail := range deduplicationDetails { - actionCacheLog.Printf("Deduplication detail: %s", detail) - } +func truncateSHAForLog(sha string) string { + if len(sha) > 8 { + return sha[:8] } + return sha } // PruneStaleGHAWEntries removes entries from the cache for the gh-aw-actions diff --git a/pkg/workflow/action_reference.go b/pkg/workflow/action_reference.go index 210a9668ea4..9d771838380 100644 --- a/pkg/workflow/action_reference.go +++ b/pkg/workflow/action_reference.go @@ -49,90 +49,77 @@ func resolveSetupActionRef(ctx context.Context, actionMode ActionMode, version s localPath := "./actions/setup" - // Dev mode - return local path if actionMode == ActionModeDev { actionRefLog.Printf("Dev mode: using local action path: %s", localPath) return localPath } - - // Action mode - use external gh-aw-actions repository with SHA pinning if possible if actionMode == ActionModeAction { - // Use actionTag if provided, otherwise fall back to version - tag := actionTag - if tag == "" { - tag = version - } - - // Check if tag is valid for action mode - if tag == "" || tag == "dev" { - actionRefLog.Print("WARNING: No release tag available in binary version (version is 'dev' or empty), falling back to local path") - return localPath - } - - // Construct the remote reference: /setup@tag - actionRepo := actionsOrgRepo + "/setup" - remoteRef := fmt.Sprintf("%s@%s", actionRepo, tag) - - // If a resolver is available, try to resolve the SHA - if resolver != nil { - sha, err := resolver.ResolveSHA(ctx, actionRepo, tag) - if err == nil && sha != "" { - pinnedRef := formatActionReference(actionRepo, sha, tag) - actionRefLog.Printf("Action mode: resolved %s to SHA-pinned reference: %s", remoteRef, pinnedRef) - return pinnedRef - } - if err != nil { - actionRefLog.Printf("Failed to resolve SHA for %s@%s: %v", actionRepo, tag, err) - } - } - - // If no resolver or SHA resolution failed, return tag-based reference - actionRefLog.Printf("Action mode: using tag-based external actions repo reference: %s (SHA will be resolved later)", remoteRef) - return remoteRef + return resolveSetupActionModeRef(ctx, actionTag, version, resolver, actionsOrgRepo, localPath) } - - // Release mode - convert to remote reference if actionMode == ActionModeRelease { - actionPath := strings.TrimPrefix(localPath, "./") - - // Use actionTag if provided, otherwise fall back to version - tag := actionTag - if tag == "" { - tag = version - } + return resolveSetupReleaseModeRef(ctx, actionTag, version, resolver, localPath) + } + actionRefLog.Printf("WARNING: Unknown action mode %s, defaulting to local path", actionMode) + return localPath +} - // Check if tag is valid for release mode - if tag == "" || tag == "dev" { - actionRefLog.Print("WARNING: No release tag available in binary version (version is 'dev' or empty), falling back to local path") - return localPath - } +func resolveSetupActionModeRef(ctx context.Context, actionTag string, version string, resolver SHAResolver, actionsOrgRepo string, localPath string) string { + tag, ok := resolveSetupTag(actionTag, version, localPath) + if !ok { + return localPath + } + actionRepo := actionsOrgRepo + "/setup" + remoteRef := fmt.Sprintf("%s@%s", actionRepo, tag) + ref := tryResolveSetupSHA(ctx, resolver, actionRepo, tag, remoteRef, "Action mode") + if ref != "" { + return ref + } + actionRefLog.Printf("Action mode: using tag-based external actions repo reference: %s (SHA will be resolved later)", remoteRef) + return remoteRef +} - // Construct the remote reference with tag: github/gh-aw/actions/setup@tag - actionRepo := fmt.Sprintf("%s/%s", GitHubOrgRepo, actionPath) - remoteRef := fmt.Sprintf("%s@%s", actionRepo, tag) - - // If a resolver is available, try to resolve the SHA - if resolver != nil { - sha, err := resolver.ResolveSHA(ctx, actionRepo, tag) - if err == nil && sha != "" { - pinnedRef := formatActionReference(actionRepo, sha, tag) - actionRefLog.Printf("Release mode: resolved %s to SHA-pinned reference: %s", remoteRef, pinnedRef) - return pinnedRef - } - if err != nil { - actionRefLog.Printf("Failed to resolve SHA for %s@%s: %v", actionRepo, tag, err) - } - } +func resolveSetupReleaseModeRef(ctx context.Context, actionTag string, version string, resolver SHAResolver, localPath string) string { + tag, ok := resolveSetupTag(actionTag, version, localPath) + if !ok { + return localPath + } + actionPath := strings.TrimPrefix(localPath, "./") + actionRepo := fmt.Sprintf("%s/%s", GitHubOrgRepo, actionPath) + remoteRef := fmt.Sprintf("%s@%s", actionRepo, tag) + ref := tryResolveSetupSHA(ctx, resolver, actionRepo, tag, remoteRef, "Release mode") + if ref != "" { + return ref + } + actionRefLog.Printf("Release mode: using tag-based remote action reference: %s (SHA will be resolved later)", remoteRef) + return remoteRef +} - // If no resolver or SHA resolution failed, return tag-based reference - // This is for backward compatibility with standalone workflow generators - actionRefLog.Printf("Release mode: using tag-based remote action reference: %s (SHA will be resolved later)", remoteRef) - return remoteRef +func resolveSetupTag(actionTag string, version string, localPath string) (string, bool) { + tag := actionTag + if tag == "" { + tag = version + } + if tag == "" || tag == "dev" { + actionRefLog.Printf("WARNING: No release tag available in binary version (version is 'dev' or empty), falling back to local path: %s", localPath) + return "", false } + return tag, true +} - // Unknown mode - default to local path - actionRefLog.Printf("WARNING: Unknown action mode %s, defaulting to local path", actionMode) - return localPath +func tryResolveSetupSHA(ctx context.Context, resolver SHAResolver, actionRepo, tag, remoteRef, modeLabel string) string { + if resolver == nil { + return "" + } + sha, err := resolver.ResolveSHA(ctx, actionRepo, tag) + if err == nil && sha != "" { + pinnedRef := formatActionReference(actionRepo, sha, tag) + actionRefLog.Printf("%s: resolved %s to SHA-pinned reference: %s", modeLabel, remoteRef, pinnedRef) + return pinnedRef + } + if err != nil { + actionRefLog.Printf("Failed to resolve SHA for %s@%s: %v", actionRepo, tag, err) + } + return "" } // resolveActionReference converts a local action path to the appropriate reference @@ -143,77 +130,20 @@ func resolveSetupActionRef(ctx context.Context, actionMode ActionMode, version s // For release mode: converts to SHA-pinned remote reference (e.g., "github/gh-aw/actions/create-issue@SHA # tag") // For action mode: converts to SHA-pinned reference in external repo if possible (e.g., "github/gh-aw-actions/create-issue@SHA # version") func (c *Compiler) resolveActionReference(localActionPath string, data *WorkflowData) string { - // Check if action-tag is specified in features - if so, override mode and use action mode behavior - hasActionTag := false - var frontmatterActionTag string - if data != nil && data.Features != nil { - if actionTagVal, exists := data.Features["action-tag"]; exists { - if actionTagStr, ok := actionTagVal.(string); ok && actionTagStr != "" { - hasActionTag = true - frontmatterActionTag = actionTagStr - actionRefLog.Printf("action-tag feature detected: %s - using action mode behavior", actionTagStr) - } - } - } + hasActionTag, frontmatterActionTag := getFrontmatterActionTag(data) // For ./actions/setup, check for compiler-level actionTag override first if localActionPath == "./actions/setup" { - // Use compiler actionTag if available, otherwise check features - var resolver SHAResolver - if data != nil && data.ActionResolver != nil { - resolver = data.ActionResolver - } - if c.actionTag != "" { - return resolveSetupActionRef(c.ctx, c.actionMode, c.version, c.actionTag, resolver, c.effectiveActionsRepo()) - } - if !hasActionTag { - return resolveSetupActionRef(c.ctx, c.actionMode, c.version, "", resolver, c.effectiveActionsRepo()) - } - // hasActionTag is true and no compiler actionTag: use action mode with the frontmatter tag - return resolveSetupActionRef(c.ctx, ActionModeAction, c.version, frontmatterActionTag, resolver, c.effectiveActionsRepo()) + return c.resolveSetupReference(data, hasActionTag, frontmatterActionTag) } - // Action mode - use external gh-aw-actions repository if c.actionMode == ActionModeAction || hasActionTag { return c.convertToExternalActionsRef(localActionPath, data) } - // Use release mode if c.actionMode == ActionModeRelease { - // Convert to tag-based remote reference for release - remoteRef := c.convertToRemoteActionRef(localActionPath, data) - if remoteRef == "" { - actionRefLog.Printf("WARNING: Could not resolve remote reference for %s", localActionPath) - return "" - } - - // Now resolve the tag to a SHA using action pins - // Extract repo and version from the remote reference (format: "repo/path@version") - actionRepo := extractActionRepo(remoteRef) - version := extractActionVersion(remoteRef) - - if actionRepo != "" && version != "" { - // Resolve the SHA using action pins - pinnedRef, err := getActionPinWithData(actionRepo, version, data) - if err != nil { - // In strict mode, getActionPinWithData returns an error - actionRefLog.Printf("Failed to pin action %s@%s: %v", actionRepo, version, err) - return "" - } - if pinnedRef != "" { - // Successfully resolved to SHA - actionRefLog.Printf("Release mode: resolved %s to SHA-pinned reference: %s", remoteRef, pinnedRef) - return pinnedRef - } - } - - // If we couldn't resolve to SHA, return the tag-based reference - // This happens in non-strict mode when no pin is available - actionRefLog.Printf("Release mode: using tag-based remote action reference: %s", remoteRef) - return remoteRef + return c.resolveReleaseActionReference(localActionPath, data) } - - // Dev mode - return local path if c.actionMode == ActionModeDev { actionRefLog.Printf("Dev mode: using local action path: %s", localActionPath) return localActionPath @@ -224,6 +154,61 @@ func (c *Compiler) resolveActionReference(localActionPath string, data *Workflow return localActionPath } +func getFrontmatterActionTag(data *WorkflowData) (bool, string) { + if data == nil || data.Features == nil { + return false, "" + } + actionTagVal, exists := data.Features["action-tag"] + if !exists { + return false, "" + } + actionTagStr, ok := actionTagVal.(string) + if !ok || actionTagStr == "" { + return false, "" + } + actionRefLog.Printf("action-tag feature detected: %s - using action mode behavior", actionTagStr) + return true, actionTagStr +} + +func (c *Compiler) resolveSetupReference(data *WorkflowData, hasActionTag bool, frontmatterActionTag string) string { + var resolver SHAResolver + if data != nil && data.ActionResolver != nil { + resolver = data.ActionResolver + } + if c.actionTag != "" { + return resolveSetupActionRef(c.ctx, c.actionMode, c.version, c.actionTag, resolver, c.effectiveActionsRepo()) + } + if !hasActionTag { + return resolveSetupActionRef(c.ctx, c.actionMode, c.version, "", resolver, c.effectiveActionsRepo()) + } + return resolveSetupActionRef(c.ctx, ActionModeAction, c.version, frontmatterActionTag, resolver, c.effectiveActionsRepo()) +} + +func (c *Compiler) resolveReleaseActionReference(localActionPath string, data *WorkflowData) string { + remoteRef := c.convertToRemoteActionRef(localActionPath, data) + if remoteRef == "" { + actionRefLog.Printf("WARNING: Could not resolve remote reference for %s", localActionPath) + return "" + } + actionRepo := extractActionRepo(remoteRef) + version := extractActionVersion(remoteRef) + if actionRepo == "" || version == "" { + actionRefLog.Printf("Release mode: using tag-based remote action reference: %s", remoteRef) + return remoteRef + } + pinnedRef, err := getActionPinWithData(actionRepo, version, data) + if err != nil { + actionRefLog.Printf("Failed to pin action %s@%s: %v", actionRepo, version, err) + return "" + } + if pinnedRef != "" { + actionRefLog.Printf("Release mode: resolved %s to SHA-pinned reference: %s", remoteRef, pinnedRef) + return pinnedRef + } + actionRefLog.Printf("Release mode: using tag-based remote action reference: %s", remoteRef) + return remoteRef +} + // convertToRemoteActionRef converts a local action path to a tag-based remote reference // that will be resolved to a SHA later in the release pipeline using action pins. // Uses the action-tag from WorkflowData.Features if specified (for testing), otherwise uses the version stored in the compiler binary. diff --git a/pkg/workflow/action_sha_checker.go b/pkg/workflow/action_sha_checker.go index 504b669a67d..53b13226d8e 100644 --- a/pkg/workflow/action_sha_checker.go +++ b/pkg/workflow/action_sha_checker.go @@ -44,59 +44,66 @@ func ExtractActionsFromLockFile(lockFilePath string) ([]ActionUsage, error) { if err := ValidateLockSchemaCompatibility(string(content), lockFilePath); err != nil { return nil, err } + if err := validateLockYAML(content); err != nil { + return nil, err + } + return extractActionsFromContent(string(content)), nil +} - // Parse YAML to extract actions from "uses" fields +func validateLockYAML(content []byte) error { var workflowData map[string]any if err := yaml.Unmarshal(content, &workflowData); err != nil { - return nil, fmt.Errorf("failed to parse lock file YAML: %w", err) + return fmt.Errorf("failed to parse lock file YAML: %w", err) } + return nil +} - // Convert to string and extract all uses fields - contentStr := string(content) +func extractActionsFromContent(content string) []ActionUsage { actions := make(map[string]ActionUsage) // Use map to deduplicate - matches := actionUsesPattern.FindAllStringSubmatch(contentStr, -1) + matches := actionUsesPattern.FindAllStringSubmatch(content, -1) for _, match := range matches { - if len(match) >= 3 { - repo := match[1] - sha := match[2] - - // Skip if we've already seen this action - if _, exists := actions[repo+"@"+sha]; exists { - continue - } - - // Extract version from comment if present (match[3]) - version := "" - if len(match) >= 4 && match[3] != "" { - version = match[3] - actionSHACheckerLog.Printf("Found action: %s@%s (version: %s)", repo, sha, version) - } else { - // Fallback: try to determine the version tag from action_pins.json - if pin, found := getLatestActionPinByRepo(repo); found { - version = pin.Version - actionSHACheckerLog.Printf("Found action: %s@%s (version from pins: %s)", repo, sha, version) - } else { - actionSHACheckerLog.Printf("Found action: %s@%s (no version)", repo, sha) - } - } - - actions[repo+"@"+sha] = ActionUsage{ - Repo: repo, - SHA: sha, - Version: version, - } + action, ok := parseActionUsageMatch(match) + if !ok { + continue + } + actionKey := action.Repo + "@" + action.SHA + if _, exists := actions[actionKey]; exists { + continue } + actions[actionKey] = action } - // Convert map to slice result := make([]ActionUsage, 0, len(actions)) for _, action := range actions { result = append(result, action) } actionSHACheckerLog.Printf("Extracted %d unique actions", len(result)) - return result, nil + return result +} + +func parseActionUsageMatch(match []string) (ActionUsage, bool) { + if len(match) < 3 { + return ActionUsage{}, false + } + repo := match[1] + sha := match[2] + version := resolveActionUsageVersion(repo, sha, match) + return ActionUsage{Repo: repo, SHA: sha, Version: version}, true +} + +func resolveActionUsageVersion(repo, sha string, match []string) string { + if len(match) >= 4 && match[3] != "" { + actionSHACheckerLog.Printf("Found action: %s@%s (version: %s)", repo, sha, match[3]) + return match[3] + } + if pin, found := getLatestActionPinByRepo(repo); found { + actionSHACheckerLog.Printf("Found action: %s@%s (version from pins: %s)", repo, sha, pin.Version) + return pin.Version + } + actionSHACheckerLog.Printf("Found action: %s@%s (no version)", repo, sha) + return "" } // CheckActionSHAUpdates checks if actions need updating by comparing with latest SHAs diff --git a/pkg/workflow/agent_validation.go b/pkg/workflow/agent_validation.go index 7b524a1d831..2e065e0284a 100644 --- a/pkg/workflow/agent_validation.go +++ b/pkg/workflow/agent_validation.go @@ -234,54 +234,54 @@ func (c *Compiler) validateBareModeSupport(frontmatter map[string]any, engine Co // validateWorkflowRunBranches validates workflow_run trigger requirements. // It enforces required workflows and branch restrictions guidance. func (c *Compiler) validateWorkflowRunBranches(workflowData *WorkflowData, markdownPath string) error { - // Fast path: skip expensive YAML parsing when the On field cannot possibly contain - // a workflow_run trigger (including when it is empty). This avoids yaml.Unmarshal - // on every validateWorkflowData call for the common case of non-workflow_run workflows. if !strings.Contains(workflowData.On, "workflow_run") { return nil } - agentValidationLog.Print("Validating workflow_run trigger requirements") + workflowRunMap, ok := parseWorkflowRunTrigger(workflowData.On) + if !ok { + return nil + } + if err := validateWorkflowRunHasWorkflows(workflowRunMap, markdownPath); err != nil { + return err + } + if _, hasBranches := workflowRunMap["branches"]; hasBranches { + if c.verbose { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("✓ workflow_run trigger has branch restrictions")) + } + return nil + } + return c.emitWorkflowRunMissingBranches(markdownPath) +} - // Parse the On field as YAML to check for workflow_run - // The On field is a YAML string that starts with "on:" key +func parseWorkflowRunTrigger(onYAML string) (map[string]any, bool) { var parsedData map[string]any - if err := yaml.Unmarshal([]byte(workflowData.On), &parsedData); err != nil { - // If we can't parse the YAML, skip this validation + if err := yaml.Unmarshal([]byte(onYAML), &parsedData); err != nil { agentValidationLog.Printf("Could not parse On field as YAML: %v", err) - return nil + return nil, false } - - // Extract the actual "on" section from the parsed data onData, hasOn := parsedData["on"] if !hasOn { - // No "on" key found, skip validation - return nil + return nil, false } - onMap, isMap := onData.(map[string]any) if !isMap { - // "on" is not a map, skip validation - return nil + return nil, false } - - // Check if workflow_run is present workflowRunVal, hasWorkflowRun := onMap["workflow_run"] if !hasWorkflowRun { - // No workflow_run trigger, no validation needed - return nil + return nil, false } + workflowRunMap, ok := workflowRunVal.(map[string]any) + return workflowRunMap, ok +} - workflowRunMap, isMap := workflowRunVal.(map[string]any) - if !isMap { - // workflow_run is not a map (unusual), skip validation +func validateWorkflowRunHasWorkflows(workflowRunMap map[string]any, markdownPath string) error { + workflowsVal, hasWorkflows := workflowRunMap["workflows"] + if hasWorkflows && hasNonEmptyWorkflowRunWorkflows(workflowsVal) { return nil } - - // workflow_run requires workflows to be present and non-empty. - workflowsVal, hasWorkflows := workflowRunMap["workflows"] - if !hasWorkflows || !hasNonEmptyWorkflowRunWorkflows(workflowsVal) { - message := `workflow_run trigger must include a non-empty workflows field. + message := `workflow_run trigger must include a non-empty workflows field. GitHub Actions requires on.workflow_run.workflows to reference at least one workflow. Without it, the compiled workflow is invalid and will be rejected. @@ -291,20 +291,10 @@ on: workflow_run: workflows: ["your-workflow"] types: [completed]` - return formatCompilerError(markdownPath, "error", message, nil) - } - - // Check if workflow_run has branches field - _, hasBranches := workflowRunMap["branches"] - if hasBranches { - // Has branch restrictions, validation passed - if c.verbose { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("✓ workflow_run trigger has branch restrictions")) - } - return nil - } + return formatCompilerError(markdownPath, "error", message, nil) +} - // workflow_run without branches - this is a warning or error depending on mode +func (c *Compiler) emitWorkflowRunMissingBranches(markdownPath string) error { message := "workflow_run trigger should include branch restrictions for security and performance.\n\n" + "Without branch restrictions, the workflow will run for workflow runs on ALL branches,\n" + "which can cause unexpected behavior and security issues.\n\n" + @@ -318,15 +308,11 @@ on: " - develop" if c.strictMode { - // In strict mode, this is an error return formatCompilerError(markdownPath, "error", message, nil) } - - // In normal mode, this is a warning formattedWarning := formatCompilerMessage(markdownPath, "warning", message) fmt.Fprintln(os.Stderr, formattedWarning) c.IncrementWarningCount() - return nil } diff --git a/pkg/workflow/cache.go b/pkg/workflow/cache.go index 201531442fd..a0b05be2d77 100644 --- a/pkg/workflow/cache.go +++ b/pkg/workflow/cache.go @@ -2,7 +2,9 @@ package workflow import ( "encoding/json" + "errors" "fmt" + "math" "os" "slices" "sort" @@ -125,175 +127,187 @@ func parseCacheMemoryEntry(cacheMap map[string]any, defaultID string) (CacheMemo ID: defaultID, Key: generateDefaultCacheKey(defaultID), } + if err := parseCacheMemoryIdentity(cacheMap, defaultID, &entry); err != nil { + return entry, err + } + parseCacheMemoryDescription(cacheMap, &entry) + if err := parseCacheMemoryRetentionDays(cacheMap, &entry); err != nil { + return entry, err + } + parseCacheMemoryRestoreOnly(cacheMap, &entry) + if err := parseCacheMemoryScope(cacheMap, &entry); err != nil { + return entry, err + } + if err := parseCacheMemoryAllowedExtensions(cacheMap, &entry); err != nil { + return entry, err + } + applyDefaultAllowedExtensions(&entry) + cacheLog.Printf("Parsed cache-memory entry: id=%s, scope=%s, restore-only=%v, retention-days=%v", entry.ID, entry.Scope, entry.RestoreOnly, entry.RetentionDays) + return entry, nil +} - // Parse ID (for array notation) - if id, exists := cacheMap["id"]; exists { - if idStr, ok := id.(string); ok { - if idStr != "default" && !isValidCacheID(idStr) { - return entry, fmt.Errorf("invalid cache-memory id %q: must contain only letters, digits, underscores, or hyphens (1-64 characters)", idStr) - } - entry.ID = idStr +func parseCacheMemoryIdentity(cacheMap map[string]any, defaultID string, entry *CacheMemoryEntry) error { + if idStr, ok := cacheMap["id"].(string); ok { + if idStr != "default" && !isValidCacheID(idStr) { + return fmt.Errorf("invalid cache-memory id %q: must contain only letters, digits, underscores, or hyphens (1-64 characters)", idStr) } + entry.ID = idStr } - // Update key if ID changed if entry.ID != defaultID { entry.Key = generateDefaultCacheKey(entry.ID) } + keyStr, ok := cacheMap["key"].(string) + if !ok { + return nil + } + if err := validateNoCacheKeyRunID(keyStr); err != nil { + return err + } + entry.Key = ensureCacheRunIDSuffix(keyStr) + return nil +} - // Parse custom key - if key, exists := cacheMap["key"]; exists { - if keyStr, ok := key.(string); ok { - if err := validateNoCacheKeyRunID(keyStr); err != nil { - return entry, err - } - entry.Key = keyStr - // Automatically append -${{ github.run_id }} if the key doesn't already end with it - runIdSuffix := "-${{ github.run_id }}" - if !strings.HasSuffix(entry.Key, runIdSuffix) { - entry.Key = entry.Key + runIdSuffix - } - } +func ensureCacheRunIDSuffix(key string) string { + runIdSuffix := "-${{ github.run_id }}" + if strings.HasSuffix(key, runIdSuffix) { + return key } + return key + runIdSuffix +} - // Parse description - if description, exists := cacheMap["description"]; exists { - if descStr, ok := description.(string); ok { - entry.Description = descStr - } +func parseCacheMemoryDescription(cacheMap map[string]any, entry *CacheMemoryEntry) { + if descStr, ok := cacheMap["description"].(string); ok { + entry.Description = descStr } +} + +func parseCacheMemoryRetentionDays(cacheMap map[string]any, entry *CacheMemoryEntry) error { + retentionDays, exists := cacheMap["retention-days"] + if !exists { + return nil + } + entry.RetentionDays = parseOptionalInt(retentionDays) + if entry.RetentionDays == nil { + return nil + } + return validateIntRange(*entry.RetentionDays, 1, 90, "retention-days") +} - // Parse retention days - if retentionDays, exists := cacheMap["retention-days"]; exists { - if retentionDaysInt, ok := retentionDays.(int); ok { - entry.RetentionDays = &retentionDaysInt - } else if retentionDaysFloat, ok := retentionDays.(float64); ok { - retentionDaysIntValue := int(retentionDaysFloat) - entry.RetentionDays = &retentionDaysIntValue - } else if retentionDaysUint64, ok := retentionDays.(uint64); ok { - retentionDaysIntValue := int(retentionDaysUint64) - entry.RetentionDays = &retentionDaysIntValue +// parseOptionalInt safely converts YAML numeric values (int, float64, uint64) to *int. +// +// It returns nil when the input cannot be represented as an integer for the current +// architecture, including: +// - NaN/Inf float64 values +// - fractional float64 values +// - float64 values outside the exact-integer range [-2^53, 2^53] +// - uint64 values larger than math.MaxInt +// - unsupported types +func parseOptionalInt(value any) *int { + // YAML unmarshaling can yield int, float64, or uint64 depending on parser/input. + if intValue, ok := value.(int); ok { + return &intValue + } + if floatValue, ok := value.(float64); ok { + if math.IsNaN(floatValue) || math.IsInf(floatValue, 0) { + return nil } - // Validate retention-days bounds - if entry.RetentionDays != nil { - if err := validateIntRange(*entry.RetentionDays, 1, 90, "retention-days"); err != nil { - return entry, err - } + if floatValue != math.Trunc(floatValue) { + return nil + } + // float64 can exactly represent integers only in [-2^53, 2^53]. + const maxExactFloatInt = float64(1 << 53) + if floatValue < -maxExactFloatInt || floatValue > maxExactFloatInt { + return nil } + intValue := int(floatValue) + return &intValue } - - // Parse restore-only flag - if restoreOnly, exists := cacheMap["restore-only"]; exists { - if restoreOnlyBool, ok := restoreOnly.(bool); ok { - entry.RestoreOnly = restoreOnlyBool + if uintValue, ok := value.(uint64); ok { + // Guard int conversion on 32-bit/64-bit architectures. + if uintValue > uint64(math.MaxInt) { + return nil } + intValue := int(uintValue) + return &intValue } + return nil +} - // Parse scope field - if scope, exists := cacheMap["scope"]; exists { - if scopeStr, ok := scope.(string); ok { - entry.Scope = scopeStr - } +func parseCacheMemoryRestoreOnly(cacheMap map[string]any, entry *CacheMemoryEntry) { + if restoreOnlyBool, ok := cacheMap["restore-only"].(bool); ok { + entry.RestoreOnly = restoreOnlyBool + } +} + +func parseCacheMemoryScope(cacheMap map[string]any, entry *CacheMemoryEntry) error { + if scopeStr, ok := cacheMap["scope"].(string); ok { + entry.Scope = scopeStr } - // Default to "workflow" scope if not specified if entry.Scope == "" { entry.Scope = "workflow" } - // Validate scope value - if !slices.Contains(validCacheMemoryScopes, entry.Scope) { - return entry, fmt.Errorf("invalid cache-memory scope %q: must be one of %v", entry.Scope, validCacheMemoryScopes) - } - - // Parse allowed-extensions field - if allowedExts, exists := cacheMap["allowed-extensions"]; exists { - if extArray, ok := allowedExts.([]any); ok { - entry.AllowedExtensions = make([]string, 0, len(extArray)) - for _, ext := range extArray { - if extStr, ok := ext.(string); ok { - // Validate: must be of the form ^\.[A-Za-z0-9]+$ to prevent YAML injection - // and ensure the shell sanitization script handles them correctly. - if !isValidFileExtension(extStr) { - return entry, fmt.Errorf("invalid allowed-extension %q: must start with '.' followed by alphanumeric characters only (e.g. .json)", extStr) - } - entry.AllowedExtensions = append(entry.AllowedExtensions, extStr) - } - } + if slices.Contains(validCacheMemoryScopes, entry.Scope) { + return nil + } + return fmt.Errorf("invalid cache-memory scope %q: must be one of %v", entry.Scope, validCacheMemoryScopes) +} + +func parseCacheMemoryAllowedExtensions(cacheMap map[string]any, entry *CacheMemoryEntry) error { + allowedExts, exists := cacheMap["allowed-extensions"] + if !exists { + return nil + } + extArray, ok := allowedExts.([]any) + if !ok { + return nil + } + entry.AllowedExtensions = make([]string, 0, len(extArray)) + for _, ext := range extArray { + extStr, ok := ext.(string) + if !ok { + continue + } + if !isValidFileExtension(extStr) { + return fmt.Errorf("invalid allowed-extension %q: must start with '.' followed by alphanumeric characters only (e.g. .json)", extStr) } + entry.AllowedExtensions = append(entry.AllowedExtensions, extStr) } - // Default to standard allowed extensions if not specified + return nil +} + +func applyDefaultAllowedExtensions(entry *CacheMemoryEntry) { if len(entry.AllowedExtensions) == 0 { entry.AllowedExtensions = constants.DefaultAllowedMemoryExtensions } - - cacheLog.Printf("Parsed cache-memory entry: id=%s, scope=%s, restore-only=%v, retention-days=%v", entry.ID, entry.Scope, entry.RestoreOnly, entry.RetentionDays) - return entry, nil } // extractCacheMemoryConfig extracts cache-memory configuration from tools section // Updated to use ToolsConfig instead of map[string]any func (c *Compiler) extractCacheMemoryConfig(toolsConfig *ToolsConfig) (*CacheMemoryConfig, error) { - // Check if cache-memory tool is configured if toolsConfig == nil || toolsConfig.CacheMemory == nil { return nil, nil } - cacheLog.Print("Extracting cache-memory configuration from ToolsConfig") - config := &CacheMemoryConfig{} cacheMemoryValue := toolsConfig.CacheMemory.Raw - - // Handle nil value (simple enable with defaults) - same as true - // This handles the case where cache-memory: is specified without a value if cacheMemoryValue == nil { - config.Caches = []CacheMemoryEntry{ - { - ID: "default", - Key: generateDefaultCacheKey("default"), - AllowedExtensions: constants.DefaultAllowedMemoryExtensions, - }, - } + config.Caches = defaultCacheMemoryEntries() return config, nil } - - // Handle boolean value (simple enable/disable) if boolValue, ok := cacheMemoryValue.(bool); ok { if boolValue { - // Create a single default cache entry - config.Caches = []CacheMemoryEntry{ - { - ID: "default", - Key: generateDefaultCacheKey("default"), - AllowedExtensions: constants.DefaultAllowedMemoryExtensions, - }, - } + config.Caches = defaultCacheMemoryEntries() } - // If false, return empty config (empty array means disabled) return config, nil } - - // Handle array of cache configurations if cacheArray, ok := cacheMemoryValue.([]any); ok { - cacheLog.Printf("Processing cache array with %d entries", len(cacheArray)) - config.Caches = make([]CacheMemoryEntry, 0, len(cacheArray)) - for _, item := range cacheArray { - if cacheMap, ok := item.(map[string]any); ok { - entry, err := parseCacheMemoryEntry(cacheMap, "default") - if err != nil { - return nil, err - } - config.Caches = append(config.Caches, entry) - } - } - - // Check for duplicate cache IDs - if err := validateNoDuplicateCacheIDs(config.Caches); err != nil { + entries, err := parseCacheMemoryEntries(cacheArray) + if err != nil { return nil, err } - + config.Caches = entries return config, nil } - - // Handle object configuration (single cache, backward compatible) - // Convert to array with single entry if configMap, ok := cacheMemoryValue.(map[string]any); ok { entry, err := parseCacheMemoryEntry(configMap, "default") if err != nil { @@ -306,6 +320,36 @@ func (c *Compiler) extractCacheMemoryConfig(toolsConfig *ToolsConfig) (*CacheMem return nil, nil } +func defaultCacheMemoryEntries() []CacheMemoryEntry { + return []CacheMemoryEntry{ + { + ID: "default", + Key: generateDefaultCacheKey("default"), + AllowedExtensions: constants.DefaultAllowedMemoryExtensions, + }, + } +} + +func parseCacheMemoryEntries(cacheArray []any) ([]CacheMemoryEntry, error) { + cacheLog.Printf("Processing cache array with %d entries", len(cacheArray)) + entries := make([]CacheMemoryEntry, 0, len(cacheArray)) + for _, item := range cacheArray { + cacheMap, ok := item.(map[string]any) + if !ok { + continue + } + entry, err := parseCacheMemoryEntry(cacheMap, "default") + if err != nil { + return nil, err + } + entries = append(entries, entry) + } + if err := validateNoDuplicateCacheIDs(entries); err != nil { + return nil, err + } + return entries, nil +} + // extractCacheMemoryConfigFromMap is a backward compatibility wrapper for extractCacheMemoryConfig // extractCacheMemoryConfigFromMap is a backward compatibility wrapper for extractCacheMemoryConfig // that accepts map[string]any instead of *ToolsConfig. This allows gradual migration of calling code. @@ -324,101 +368,107 @@ func generateCacheSteps(builder *strings.Builder, data *WorkflowData, verbose bo } cacheLog.Print("Generating cache steps from frontmatter cache configuration") - - // Add comment indicating cache configuration was processed builder.WriteString(" # Cache configuration from frontmatter processed below\n") - - // Parse cache configuration to determine if it's a single cache or array - var caches []map[string]any - - // Try to parse the cache YAML string back to determine structure - var topLevel map[string]any - if err := yaml.Unmarshal([]byte(data.Cache), &topLevel); err != nil { + caches, err := parseCacheStepConfigs(data.Cache) + if err != nil { if verbose { fmt.Fprintf(os.Stderr, "Warning: Failed to parse cache configuration: %v\n", err) } return } + for i, cache := range caches { + writeCacheStep(builder, cache, i, len(caches)) + } +} - // Extract the cache section from the top-level map +func parseCacheStepConfigs(cacheYAML string) ([]map[string]any, error) { + var topLevel map[string]any + if err := yaml.Unmarshal([]byte(cacheYAML), &topLevel); err != nil { + return nil, err + } cacheConfig, exists := topLevel["cache"] if !exists { - if verbose { - fmt.Fprintf(os.Stderr, "Warning: No cache key found in parsed configuration\n") - } - return + return nil, errors.New("no cache key found in parsed configuration") } - - // Handle both single cache object and array of caches if cacheArray, isArray := cacheConfig.([]any); isArray { - // Multiple caches cacheLog.Printf("Processing %d cache entries (array format)", len(cacheArray)) - for _, cacheItem := range cacheArray { - if cacheMap, ok := cacheItem.(map[string]any); ok { - caches = append(caches, cacheMap) - } - } - } else if cacheMap, isMap := cacheConfig.(map[string]any); isMap { - // Single cache + return normalizeCacheStepArray(cacheArray), nil + } + if cacheMap, isMap := cacheConfig.(map[string]any); isMap { cacheLog.Print("Processing single cache entry (object format)") - caches = append(caches, cacheMap) + return []map[string]any{cacheMap}, nil } + return nil, nil +} - // Generate cache steps - for i, cache := range caches { - stepName := "Cache" - if len(caches) > 1 { - stepName = fmt.Sprintf("Cache %d", i+1) - } - if nameVal, hasName := cache["name"]; hasName { - if nameStr, ok := nameVal.(string); ok && nameStr != "" { - stepName = nameStr - } - } else if key, hasKey := cache["key"]; hasKey { - if keyStr, ok := key.(string); ok && keyStr != "" { - stepName = fmt.Sprintf("Cache (%s)", keyStr) - } +func normalizeCacheStepArray(cacheArray []any) []map[string]any { + caches := make([]map[string]any, 0, len(cacheArray)) + for _, cacheItem := range cacheArray { + if cacheMap, ok := cacheItem.(map[string]any); ok { + caches = append(caches, cacheMap) } + } + return caches +} - fmt.Fprintf(builder, " - name: %s\n", stepName) - fmt.Fprintf(builder, " uses: %s\n", getActionPin("actions/cache")) - builder.WriteString(" with:\n") +func writeCacheStep(builder *strings.Builder, cache map[string]any, idx int, total int) { + stepName := resolveCacheStepName(cache, idx, total) + fmt.Fprintf(builder, " - name: %s\n", stepName) + fmt.Fprintf(builder, " uses: %s\n", getActionPin("actions/cache")) + builder.WriteString(" with:\n") + writeCacheStepValue(builder, "key", cache["key"]) + writeCachePath(builder, cache["path"]) + writeCacheRestoreKeys(builder, cache["restore-keys"]) + writeCacheStepValue(builder, "upload-chunk-size", cache["upload-chunk-size"]) + writeCacheStepValue(builder, "fail-on-cache-miss", cache["fail-on-cache-miss"]) + writeCacheStepValue(builder, "lookup-only", cache["lookup-only"]) +} - // Add required cache parameters - if key, hasKey := cache["key"]; hasKey { - fmt.Fprintf(builder, " key: %v\n", key) - } - if path, hasPath := cache["path"]; hasPath { - if pathArray, isArray := path.([]any); isArray { - builder.WriteString(" path: |\n") - for _, p := range pathArray { - fmt.Fprintf(builder, " %v\n", p) - } - } else { - fmt.Fprintf(builder, " path: %v\n", path) - } - } +func resolveCacheStepName(cache map[string]any, idx int, total int) string { + stepName := "Cache" + if total > 1 { + stepName = fmt.Sprintf("Cache %d", idx+1) + } + if nameStr, ok := cache["name"].(string); ok && nameStr != "" { + return nameStr + } + if keyStr, ok := cache["key"].(string); ok && keyStr != "" { + return fmt.Sprintf("Cache (%s)", keyStr) + } + return stepName +} - // Add optional cache parameters - if restoreKeys, hasRestoreKeys := cache["restore-keys"]; hasRestoreKeys { - if restoreArray, isArray := restoreKeys.([]any); isArray { - builder.WriteString(" restore-keys: |\n") - for _, key := range restoreArray { - fmt.Fprintf(builder, " %v\n", key) - } - } else { - fmt.Fprintf(builder, " restore-keys: %v\n", restoreKeys) - } - } - if uploadChunkSize, hasSize := cache["upload-chunk-size"]; hasSize { - fmt.Fprintf(builder, " upload-chunk-size: %v\n", uploadChunkSize) - } - if failOnMiss, hasFail := cache["fail-on-cache-miss"]; hasFail { - fmt.Fprintf(builder, " fail-on-cache-miss: %v\n", failOnMiss) +func writeCachePath(builder *strings.Builder, path any) { + if path == nil { + return + } + if pathArray, isArray := path.([]any); isArray { + builder.WriteString(" path: |\n") + for _, p := range pathArray { + fmt.Fprintf(builder, " %v\n", p) } - if lookupOnly, hasLookup := cache["lookup-only"]; hasLookup { - fmt.Fprintf(builder, " lookup-only: %v\n", lookupOnly) + return + } + fmt.Fprintf(builder, " path: %v\n", path) +} + +func writeCacheRestoreKeys(builder *strings.Builder, restoreKeys any) { + if restoreKeys == nil { + return + } + if restoreArray, isArray := restoreKeys.([]any); isArray { + builder.WriteString(" restore-keys: |\n") + for _, key := range restoreArray { + fmt.Fprintf(builder, " %v\n", key) } + return + } + fmt.Fprintf(builder, " restore-keys: %v\n", restoreKeys) +} + +func writeCacheStepValue(builder *strings.Builder, key string, value any) { + if value != nil { + fmt.Fprintf(builder, " %s: %v\n", key, value) } } diff --git a/pkg/workflow/call_workflow_validation.go b/pkg/workflow/call_workflow_validation.go index 3ce8c2d953d..3b4b03c09bc 100644 --- a/pkg/workflow/call_workflow_validation.go +++ b/pkg/workflow/call_workflow_validation.go @@ -42,23 +42,15 @@ func (c *Compiler) validateCallWorkflow(data *WorkflowData, workflowPath string) // Get the current workflow name for self-reference check currentWorkflowName := getCurrentWorkflowName(workflowPath) callWorkflowValidationLog.Printf("Current workflow name: %s", currentWorkflowName) - - // Collect all validation errors using ErrorCollector collector := NewErrorCollector(c.failFast) - for _, workflowName := range config.Workflows { callWorkflowValidationLog.Printf("Validating workflow: %s", workflowName) - - // Check for self-reference - if workflowName == currentWorkflowName { - selfRefErr := fmt.Errorf("call-workflow: self-reference not allowed (workflow '%s' cannot call itself)\n\nA workflow cannot call itself to prevent infinite loops.\nUse a separate worker workflow for the task instead", workflowName) - if returnErr := collector.Add(selfRefErr); returnErr != nil { + if err := validateNotSelfReference(workflowName, currentWorkflowName); err != nil { + if returnErr := collector.Add(err); returnErr != nil { return returnErr } continue } - - // Find the workflow file in multiple locations fileResult, err := findWorkflowFile(workflowName, workflowPath) if err != nil { findErr := fmt.Errorf("call-workflow: error finding workflow '%s': %w", workflowName, err) @@ -67,118 +59,83 @@ func (c *Compiler) validateCallWorkflow(data *WorkflowData, workflowPath string) } continue } - - // Check if any workflow file exists - if !fileResult.mdExists && !fileResult.lockExists && !fileResult.ymlExists { - currentDir := filepath.Dir(workflowPath) - githubDir := filepath.Dir(currentDir) - repoRoot := filepath.Dir(githubDir) - workflowsDir := filepath.Join(repoRoot, constants.GetWorkflowDir()) - - notFoundErr := fmt.Errorf("call-workflow: workflow '%s' not found in %s\n\nChecked for: %s.md, %s.lock.yml, %s.yml\n\nTo fix:\n1. Verify the workflow file exists in %s/\n2. Ensure the filename matches exactly (case-sensitive)\n3. Use the filename without extension in your configuration", workflowName, workflowsDir, workflowName, workflowName, workflowName, workflowsDir) - if returnErr := collector.Add(notFoundErr); returnErr != nil { + if err := validateWorkflowFileExists(fileResult, workflowName, workflowPath); err != nil { + if returnErr := collector.Add(err); returnErr != nil { return returnErr } continue } - - // Validate that the workflow supports workflow_call. - // Priority: .lock.yml > .yml > .md (same-batch compilation target) - if fileResult.lockExists { - workflowContent, readErr := os.ReadFile(fileResult.lockPath) // #nosec G304 -- lockPath is validated via isPathWithinDir() in findWorkflowFile() before being returned - if readErr != nil { - fileReadErr := fmt.Errorf("call-workflow: failed to read workflow file %s: %w", fileResult.lockPath, readErr) - if returnErr := collector.Add(fileReadErr); returnErr != nil { - return returnErr - } - continue - } - var workflow map[string]any - if err := yaml.Unmarshal(workflowContent, &workflow); err != nil { - parseErr := fmt.Errorf("call-workflow: failed to parse workflow file %s: %w", fileResult.lockPath, err) - if returnErr := collector.Add(parseErr); returnErr != nil { - return returnErr - } - continue - } - onSection, hasOn := workflow["on"] - if !hasOn { - onErr := fmt.Errorf("call-workflow: workflow '%s' does not have an 'on' trigger section", workflowName) - if returnErr := collector.Add(onErr); returnErr != nil { - return returnErr - } - continue - } - if !containsWorkflowCall(onSection) { - callErr := fmt.Errorf("call-workflow: workflow '%s' does not support workflow_call trigger (must include 'workflow_call' in the 'on' section)", workflowName) - if returnErr := collector.Add(callErr); returnErr != nil { - return returnErr - } - continue - } - } else if fileResult.ymlExists { - workflowContent, readErr := os.ReadFile(fileResult.ymlPath) // #nosec G304 -- ymlPath is validated via isPathWithinDir() in findWorkflowFile() before being returned - if readErr != nil { - fileReadErr := fmt.Errorf("call-workflow: failed to read workflow file %s: %w", fileResult.ymlPath, readErr) - if returnErr := collector.Add(fileReadErr); returnErr != nil { - return returnErr - } - continue - } - var workflow map[string]any - if err := yaml.Unmarshal(workflowContent, &workflow); err != nil { - parseErr := fmt.Errorf("call-workflow: failed to parse workflow file %s: %w", fileResult.ymlPath, err) - if returnErr := collector.Add(parseErr); returnErr != nil { - return returnErr - } - continue - } - onSection, hasOn := workflow["on"] - if !hasOn { - onErr := fmt.Errorf("call-workflow: workflow '%s' does not have an 'on' trigger section", workflowName) - if returnErr := collector.Add(onErr); returnErr != nil { - return returnErr - } - continue - } - if !containsWorkflowCall(onSection) { - callErr := fmt.Errorf("call-workflow: workflow '%s' does not support workflow_call trigger (must include 'workflow_call' in the 'on' section)", workflowName) - if returnErr := collector.Add(callErr); returnErr != nil { - return returnErr - } - continue - } - } else { - // Only .md exists — it may be a same-batch compilation target. - // Validate via the .md frontmatter so a second compile pass is not required. - mdHasCall, checkErr := mdHasWorkflowCall(fileResult.mdPath) - if checkErr != nil { - readErr := fmt.Errorf("call-workflow: failed to read workflow source %s: %w", fileResult.mdPath, checkErr) - if returnErr := collector.Add(readErr); returnErr != nil { - return returnErr - } - continue - } - if !mdHasCall { - callErr := fmt.Errorf("call-workflow: workflow '%s' does not support workflow_call trigger (must include 'workflow_call' in the 'on' section)", workflowName) - if returnErr := collector.Add(callErr); returnErr != nil { - return returnErr - } - continue + if err := validateWorkflowSupportsCallTrigger(workflowName, fileResult); err != nil { + if returnErr := collector.Add(err); returnErr != nil { + return returnErr } - // .md exists with workflow_call — valid same-batch compilation target. - callWorkflowValidationLog.Printf("Workflow '%s' is valid for call-workflow (found .md source at %s with workflow_call trigger)", workflowName, fileResult.mdPath) continue } - callWorkflowValidationLog.Printf("Workflow '%s' is valid for call-workflow", workflowName) } - callWorkflowValidationLog.Printf("Call workflow validation completed: error_count=%d, total_workflows=%d", collector.Count(), len(config.Workflows)) - return collector.FormattedError("call-workflow") } +func validateNotSelfReference(workflowName, currentWorkflowName string) error { + if workflowName == currentWorkflowName { + return fmt.Errorf("call-workflow: self-reference not allowed (workflow '%s' cannot call itself)\n\nA workflow cannot call itself to prevent infinite loops.\nUse a separate worker workflow for the task instead", workflowName) + } + return nil +} + +func validateWorkflowFileExists(fileResult *findWorkflowFileResult, workflowName, workflowPath string) error { + if fileResult.mdExists || fileResult.lockExists || fileResult.ymlExists { + return nil + } + currentDir := filepath.Dir(workflowPath) + githubDir := filepath.Dir(currentDir) + repoRoot := filepath.Dir(githubDir) + workflowsDir := filepath.Join(repoRoot, constants.GetWorkflowDir()) + return fmt.Errorf("call-workflow: workflow '%s' not found in %s\n\nChecked for: %s.md, %s.lock.yml, %s.yml\n\nTo fix:\n1. Verify the workflow file exists in %s/\n2. Ensure the filename matches exactly (case-sensitive)\n3. Use the filename without extension in your configuration", workflowName, workflowsDir, workflowName, workflowName, workflowName, workflowsDir) +} + +func validateWorkflowSupportsCallTrigger(workflowName string, fileResult *findWorkflowFileResult) error { + if fileResult.lockExists { + return validateYAMLWorkflowHasCallTrigger(fileResult.lockPath, workflowName) + } + if fileResult.ymlExists { + return validateYAMLWorkflowHasCallTrigger(fileResult.ymlPath, workflowName) + } + return validateMarkdownWorkflowHasCallTrigger(fileResult.mdPath, workflowName) +} + +func validateYAMLWorkflowHasCallTrigger(path, workflowName string) error { + workflowContent, err := os.ReadFile(path) // #nosec G304 -- path is validated via isPathWithinDir() in findWorkflowFile() before being returned + if err != nil { + return fmt.Errorf("call-workflow: failed to read workflow file %s: %w", path, err) + } + var workflow map[string]any + if err = yaml.Unmarshal(workflowContent, &workflow); err != nil { + return fmt.Errorf("call-workflow: failed to parse workflow file %s: %w", path, err) + } + onSection, hasOn := workflow["on"] + if !hasOn { + return fmt.Errorf("call-workflow: workflow '%s' does not have an 'on' trigger section", workflowName) + } + if containsWorkflowCall(onSection) { + return nil + } + return fmt.Errorf("call-workflow: workflow '%s' does not support workflow_call trigger (must include 'workflow_call' in the 'on' section)", workflowName) +} + +func validateMarkdownWorkflowHasCallTrigger(path, workflowName string) error { + mdHasCall, checkErr := mdHasWorkflowCall(path) + if checkErr != nil { + return fmt.Errorf("call-workflow: failed to read workflow source %s: %w", path, checkErr) + } + if !mdHasCall { + return fmt.Errorf("call-workflow: workflow '%s' does not support workflow_call trigger (must include 'workflow_call' in the 'on' section)", workflowName) + } + callWorkflowValidationLog.Printf("Workflow '%s' is valid for call-workflow (found .md source at %s with workflow_call trigger)", workflowName, path) + return nil +} + // extractWorkflowCallInputs parses a workflow file and extracts the workflow_call inputs schema. // Returns a map of input definitions that can be used to generate MCP tool schemas. func extractWorkflowCallInputs(workflowPath string) (map[string]any, error) { From 44e8651104397c84faec334a52c5fd8e732dbd80 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 1 Jun 2026 17:04:23 +0000 Subject: [PATCH 3/5] Fix parseOptionalInt int-range handling and add coverage Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/cache.go | 7 ++++ pkg/workflow/cache_optional_int_test.go | 52 +++++++++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 pkg/workflow/cache_optional_int_test.go diff --git a/pkg/workflow/cache.go b/pkg/workflow/cache.go index a0b05be2d77..6fa5b0b4bc4 100644 --- a/pkg/workflow/cache.go +++ b/pkg/workflow/cache.go @@ -200,9 +200,13 @@ func parseCacheMemoryRetentionDays(cacheMap map[string]any, entry *CacheMemoryEn // - NaN/Inf float64 values // - fractional float64 values // - float64 values outside the exact-integer range [-2^53, 2^53] +// - float64 values outside the current architecture int range // - uint64 values larger than math.MaxInt // - unsupported types func parseOptionalInt(value any) *int { + maxInt := int(^uint(0) >> 1) + minInt := -maxInt - 1 + // YAML unmarshaling can yield int, float64, or uint64 depending on parser/input. if intValue, ok := value.(int); ok { return &intValue @@ -219,6 +223,9 @@ func parseOptionalInt(value any) *int { if floatValue < -maxExactFloatInt || floatValue > maxExactFloatInt { return nil } + if floatValue < float64(minInt) || floatValue > float64(maxInt) { + return nil + } intValue := int(floatValue) return &intValue } diff --git a/pkg/workflow/cache_optional_int_test.go b/pkg/workflow/cache_optional_int_test.go new file mode 100644 index 00000000000..894159d8f5e --- /dev/null +++ b/pkg/workflow/cache_optional_int_test.go @@ -0,0 +1,52 @@ +//go:build !integration + +package workflow + +import ( + "math" + "strconv" + "testing" +) + +func TestParseOptionalInt(t *testing.T) { + t.Run("integer float is accepted", func(t *testing.T) { + value := parseOptionalInt(7.0) + if value == nil || *value != 7 { + t.Fatalf("expected 7, got %v", value) + } + }) + + t.Run("fractional float is rejected", func(t *testing.T) { + if value := parseOptionalInt(7.5); value != nil { + t.Fatalf("expected nil, got %d", *value) + } + }) + + t.Run("nan and infinity are rejected", func(t *testing.T) { + if value := parseOptionalInt(math.NaN()); value != nil { + t.Fatalf("expected nil for NaN, got %d", *value) + } + if value := parseOptionalInt(math.Inf(1)); value != nil { + t.Fatalf("expected nil for +Inf, got %d", *value) + } + }) + + t.Run("uint64 above max int is rejected", func(t *testing.T) { + if value := parseOptionalInt(uint64(math.MaxInt) + 1); value != nil { + t.Fatalf("expected nil, got %d", *value) + } + }) + + t.Run("large exact float is architecture aware", func(t *testing.T) { + value := parseOptionalInt(1e12) + if strconv.IntSize == 32 { + if value != nil { + t.Fatalf("expected nil on 32-bit, got %d", *value) + } + return + } + if value == nil || *value != 1000000000000 { + t.Fatalf("expected 1000000000000 on 64-bit, got %v", value) + } + }) +} From 546e94ae721586ab015f8222ce15c1c2cc27c442 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 1 Jun 2026 17:06:38 +0000 Subject: [PATCH 4/5] Harden parseOptionalInt bounds checks and improve tests Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/cache.go | 5 +--- pkg/workflow/cache_optional_int_test.go | 33 +++++++++---------------- 2 files changed, 13 insertions(+), 25 deletions(-) diff --git a/pkg/workflow/cache.go b/pkg/workflow/cache.go index 6fa5b0b4bc4..361c3c6f71f 100644 --- a/pkg/workflow/cache.go +++ b/pkg/workflow/cache.go @@ -204,9 +204,6 @@ func parseCacheMemoryRetentionDays(cacheMap map[string]any, entry *CacheMemoryEn // - uint64 values larger than math.MaxInt // - unsupported types func parseOptionalInt(value any) *int { - maxInt := int(^uint(0) >> 1) - minInt := -maxInt - 1 - // YAML unmarshaling can yield int, float64, or uint64 depending on parser/input. if intValue, ok := value.(int); ok { return &intValue @@ -223,7 +220,7 @@ func parseOptionalInt(value any) *int { if floatValue < -maxExactFloatInt || floatValue > maxExactFloatInt { return nil } - if floatValue < float64(minInt) || floatValue > float64(maxInt) { + if floatValue < float64(math.MinInt) || floatValue > float64(math.MaxInt) { return nil } intValue := int(floatValue) diff --git a/pkg/workflow/cache_optional_int_test.go b/pkg/workflow/cache_optional_int_test.go index 894159d8f5e..e8088395402 100644 --- a/pkg/workflow/cache_optional_int_test.go +++ b/pkg/workflow/cache_optional_int_test.go @@ -6,47 +6,38 @@ import ( "math" "strconv" "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestParseOptionalInt(t *testing.T) { t.Run("integer float is accepted", func(t *testing.T) { value := parseOptionalInt(7.0) - if value == nil || *value != 7 { - t.Fatalf("expected 7, got %v", value) - } + require.NotNil(t, value) + assert.Equal(t, 7, *value) }) t.Run("fractional float is rejected", func(t *testing.T) { - if value := parseOptionalInt(7.5); value != nil { - t.Fatalf("expected nil, got %d", *value) - } + assert.Nil(t, parseOptionalInt(7.5)) }) t.Run("nan and infinity are rejected", func(t *testing.T) { - if value := parseOptionalInt(math.NaN()); value != nil { - t.Fatalf("expected nil for NaN, got %d", *value) - } - if value := parseOptionalInt(math.Inf(1)); value != nil { - t.Fatalf("expected nil for +Inf, got %d", *value) - } + assert.Nil(t, parseOptionalInt(math.NaN())) + assert.Nil(t, parseOptionalInt(math.Inf(1))) }) t.Run("uint64 above max int is rejected", func(t *testing.T) { - if value := parseOptionalInt(uint64(math.MaxInt) + 1); value != nil { - t.Fatalf("expected nil, got %d", *value) - } + assert.Nil(t, parseOptionalInt(uint64(math.MaxInt)+1)) }) t.Run("large exact float is architecture aware", func(t *testing.T) { value := parseOptionalInt(1e12) if strconv.IntSize == 32 { - if value != nil { - t.Fatalf("expected nil on 32-bit, got %d", *value) - } + assert.Nil(t, value) return } - if value == nil || *value != 1000000000000 { - t.Fatalf("expected 1000000000000 on 64-bit, got %v", value) - } + require.NotNil(t, value) + assert.Equal(t, 1000000000000, *value) }) } From a11240b8372508979e0d4f10cfee0d9d240bc7e9 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 1 Jun 2026 17:08:31 +0000 Subject: [PATCH 5/5] Clarify parseOptionalInt float validation order Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/cache.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/workflow/cache.go b/pkg/workflow/cache.go index 361c3c6f71f..67f5d167482 100644 --- a/pkg/workflow/cache.go +++ b/pkg/workflow/cache.go @@ -215,14 +215,14 @@ func parseOptionalInt(value any) *int { if floatValue != math.Trunc(floatValue) { return nil } + if floatValue < float64(math.MinInt) || floatValue > float64(math.MaxInt) { + return nil + } // float64 can exactly represent integers only in [-2^53, 2^53]. const maxExactFloatInt = float64(1 << 53) if floatValue < -maxExactFloatInt || floatValue > maxExactFloatInt { return nil } - if floatValue < float64(math.MinInt) || floatValue > float64(math.MaxInt) { - return nil - } intValue := int(floatValue) return &intValue }