From b34224011288455a5043082c417c3fe343c64ade Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 18:40:38 +0000 Subject: [PATCH 1/4] Initial plan From 7d6f096f3d24daa16f7c335435585832992edc90 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 19:07:39 +0000 Subject: [PATCH 2/4] refactor: decompose validateWorkflowData into 4 focused validators - Create compiler_validators.go with validateExpressions, validateFeatureConfig, validatePermissions, and validateToolConfiguration (each ~100 lines, independently testable) - Reduce validateWorkflowData from 387 lines to a 25-line orchestrator - Add fmt.Errorf/isFormattedCompilerError defensive wrapping for the bare return err calls at CompileWorkflowData lines 775 and 789 - Add compiler_validators_test.go with tests for each new validator - Remove now-unused 'slices' import from compiler.go Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7c5f96e4-396a-4b5a-b1df-52d8c72b86d7 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler.go | 410 ++-------------------- pkg/workflow/compiler_validators.go | 425 +++++++++++++++++++++++ pkg/workflow/compiler_validators_test.go | 262 ++++++++++++++ 3 files changed, 713 insertions(+), 384 deletions(-) create mode 100644 pkg/workflow/compiler_validators.go create mode 100644 pkg/workflow/compiler_validators_test.go diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 7031e16d7d..6b841ce5bc 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -6,7 +6,6 @@ import ( "fmt" "os" "path/filepath" - "slices" "strings" "time" @@ -75,396 +74,29 @@ func (c *Compiler) CompileWorkflow(markdownPath string) error { return c.CompileWorkflowData(workflowData, markdownPath) } -// validateWorkflowData performs comprehensive validation of workflow configuration -// including expressions, features, permissions, and configurations. +// validateWorkflowData orchestrates all validation of workflow configuration by +// delegating to four focused validators. Each validator is independently testable +// and covers a distinct concern: +// +// - validateExpressions: expression safety and runtime-import file checks +// - validateFeatureConfig: feature flags and action-mode override +// - validatePermissions: permissions parsing, MCP tool constraints, workflow_run security +// - validateToolConfiguration: safe-outputs, GitHub tools, dispatches, and resources func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath string) error { - // Validate expression safety - check that all GitHub Actions expressions are in the allowed list - if strings.Contains(workflowData.MarkdownContent, "${{") { - log.Printf("Validating expression safety") - if err := validateExpressionSafety(workflowData.MarkdownContent); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - } - - // Validate expressions in runtime-import files at compile time - if strings.Contains(workflowData.MarkdownContent, "{{#runtime-import") { - log.Printf("Validating runtime-import files") - // Go up from .github/workflows/file.md to repo root - workflowDir := filepath.Dir(markdownPath) // .github/workflows - githubDir := filepath.Dir(workflowDir) // .github - workspaceDir := filepath.Dir(githubDir) // repo root - if err := validateRuntimeImportFiles(workflowData.MarkdownContent, workspaceDir); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - } - - // Validate feature flags - log.Printf("Validating feature flags") - if err := validateFeatures(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Check for action-mode feature flag override - if workflowData.Features != nil { - if actionModeVal, exists := workflowData.Features["action-mode"]; exists { - if actionModeStr, ok := actionModeVal.(string); ok && actionModeStr != "" { - mode := ActionMode(actionModeStr) - if !mode.IsValid() { - return formatCompilerError(markdownPath, "error", fmt.Sprintf("invalid action-mode feature flag '%s'. Must be 'dev', 'release', or 'script'", actionModeStr), nil) - } - log.Printf("Overriding action mode from feature flag: %s", mode) - c.SetActionMode(mode) - } - } - } - - // Parse permissions once for all permission-related validation checks below. - // WorkflowData.Permissions contains the raw YAML string (including "permissions:" prefix). - // Parsing once here avoids redundant YAML parsing in each validator. - workflowPermissions := NewPermissionsParser(workflowData.Permissions).ToPermissions() - - // Validate dangerous permissions - log.Printf("Validating dangerous permissions") - if err := validateDangerousPermissions(workflowData, workflowPermissions); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate GitHub App-only permissions require a GitHub App to be configured - log.Printf("Validating GitHub App-only permissions") - if err := validateGitHubAppOnlyPermissions(workflowData, workflowPermissions); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate tools.github.github-app.permissions does not use "write" - log.Printf("Validating GitHub MCP app permissions (no write)") - if err := validateGitHubMCPAppPermissionsNoWrite(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Warn when github-app.permissions is set in contexts that don't support it - warnGitHubAppPermissionsUnsupportedContexts(workflowData) - - // Validate agent file exists if specified in engine config - log.Printf("Validating agent file if specified") - if err := c.validateAgentFile(workflowData, markdownPath); err != nil { + if err := c.validateExpressions(workflowData, markdownPath); err != nil { return err } - // Validate sandbox configuration - log.Printf("Validating sandbox configuration") - if err := validateSandboxConfig(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate safe-outputs target configuration - log.Printf("Validating safe-outputs target fields") - if err := validateSafeOutputsTarget(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate safe-outputs max configuration - log.Printf("Validating safe-outputs max fields") - if err := validateSafeOutputsMax(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate safe-outputs allowed-domains configuration - log.Printf("Validating safe-outputs allowed-domains") - if err := c.validateSafeOutputsAllowedDomains(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate safe-outputs merge-pull-request configuration - log.Printf("Validating safe-outputs merge-pull-request") - if err := validateSafeOutputsMergePullRequest(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate safe-outputs needs declarations - log.Printf("Validating safe-outputs needs declarations") - if err := validateSafeOutputsNeeds(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate on.needs declarations and on.github-app needs expressions - log.Printf("Validating on.needs declarations") - if err := c.validateOnNeeds(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate safe-job needs: declarations against known generated job IDs - log.Printf("Validating safe-job needs declarations") - if err := validateSafeJobNeeds(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Emit warnings for push-to-pull-request-branch misconfiguration - log.Printf("Validating push-to-pull-request-branch configuration") - c.validatePushToPullRequestBranchWarnings(workflowData.SafeOutputs, workflowData.CheckoutConfigs) - - // Validate network allowed domains configuration - log.Printf("Validating network allowed domains") - if err := c.validateNetworkAllowedDomains(workflowData.NetworkPermissions); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate network firewall configuration - log.Printf("Validating network firewall configuration") - if err := validateNetworkFirewallConfig(workflowData.NetworkPermissions); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate safe-outputs allow-workflows requires GitHub App - log.Printf("Validating safe-outputs allow-workflows") - if err := validateSafeOutputsAllowWorkflows(workflowData.SafeOutputs); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate labels configuration - log.Printf("Validating labels") - if err := validateLabels(workflowData); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Validate workflow-level concurrency group expression - log.Printf("Validating workflow-level concurrency configuration") - if workflowData.Concurrency != "" { - // Extract the group expression from the concurrency YAML - // The Concurrency field contains the full YAML (e.g., "concurrency:\n group: \"...\"") - // We need to extract just the group value - groupExpr := extractConcurrencyGroupFromYAML(workflowData.Concurrency) - if groupExpr != "" { - if err := validateConcurrencyGroupExpression(groupExpr); err != nil { - return formatCompilerError(markdownPath, "error", "workflow-level concurrency validation failed: "+err.Error(), err) - } - } - } - - // Validate concurrency.job-discriminator expression - if workflowData.ConcurrencyJobDiscriminator != "" { - if err := validateConcurrencyGroupExpression(workflowData.ConcurrencyJobDiscriminator); err != nil { - return formatCompilerError(markdownPath, "error", "concurrency.job-discriminator validation failed: "+err.Error(), err) - } - } - - // Validate engine-level concurrency group expression - log.Printf("Validating engine-level concurrency configuration") - if workflowData.EngineConfig != nil && workflowData.EngineConfig.Concurrency != "" { - // Extract the group expression from the engine concurrency YAML - groupExpr := extractConcurrencyGroupFromYAML(workflowData.EngineConfig.Concurrency) - if groupExpr != "" { - if err := validateConcurrencyGroupExpression(groupExpr); err != nil { - return formatCompilerError(markdownPath, "error", "engine.concurrency validation failed: "+err.Error(), err) - } - } - } - - // Validate safe-outputs concurrency group expression - if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.ConcurrencyGroup != "" { - if err := validateConcurrencyGroupExpression(workflowData.SafeOutputs.ConcurrencyGroup); err != nil { - return formatCompilerError(markdownPath, "error", "safe-outputs.concurrency-group validation failed: "+err.Error(), err) - } - } - - // Warn when the user has specified custom workflow-level concurrency with cancel-in-progress: true - // AND the workflow has the bot self-cancel risk combination (issue_comment triggers + GitHub App - // safe-outputs). In this case the auto-generated bot-actor isolation cannot be applied because the - // user's concurrency expression is preserved as-is. The user must add the bot-actor isolation - // themselves (e.g. prepend `contains(github.actor, '[bot]') && github.run_id ||` to their group key). - if workflowData.Concurrency != "" && - strings.Contains(workflowData.Concurrency, "cancel-in-progress: true") && - hasBotSelfCancelRisk(workflowData) { - fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", - "Custom workflow-level concurrency with cancel-in-progress: true may cause self-cancellation.\n"+ - "safe-outputs.github-app can post comments that re-trigger this workflow via issue_comment,\n"+ - "and those passive bot-authored runs can collide with the primary run's concurrency group.\n"+ - "Add `contains(github.actor, '[bot]') && github.run_id ||` at the start of your concurrency\n"+ - "group expression to route bot-triggered runs to a unique key and prevent self-cancellation.\n"+ - "See: https://gh.io/gh-aw/reference/concurrency for details.")) - c.IncrementWarningCount() - } - - // Emit warning for sandbox.agent: false (disables agent sandbox firewall) - if isAgentSandboxDisabled(workflowData) { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("⚠️ WARNING: Agent sandbox disabled (sandbox.agent: false). This removes firewall protection. The AI agent will have direct network access without firewall filtering. The MCP gateway remains enabled. Only use this for testing or in controlled environments where you trust the AI agent completely.")) - c.IncrementWarningCount() - } - - // Validate: threat detection requires sandbox.agent to be enabled (detection runs inside AWF) - if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.ThreatDetection != nil && isAgentSandboxDisabled(workflowData) { - return formatCompilerError(markdownPath, "error", "threat detection requires sandbox.agent to be enabled. Threat detection runs inside the agent sandbox (AWF) with fully blocked network. Either enable sandbox.agent or use 'threat-detection: false' to disable the threat-detection configuration in safe-outputs.", errors.New("threat detection requires sandbox.agent")) - } - - // Emit warning when assign-to-agent is used with github-app: but no explicit github-token:. - // GitHub App tokens are rejected by the Copilot assignment API — a PAT is required. - // The token fallback chain (GH_AW_AGENT_TOKEN || GH_AW_GITHUB_TOKEN || GITHUB_TOKEN) is used automatically. - if workflowData.SafeOutputs != nil && - workflowData.SafeOutputs.AssignToAgent != nil && - workflowData.SafeOutputs.GitHubApp != nil && - workflowData.SafeOutputs.AssignToAgent.GitHubToken == "" { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage( - "assign-to-agent does not support GitHub App tokens. "+ - "The Copilot assignment API requires a fine-grained PAT. "+ - "The token fallback chain (GH_AW_AGENT_TOKEN || GH_AW_GITHUB_TOKEN || GITHUB_TOKEN) will be used automatically. "+ - "Add github-token: to your assign-to-agent config to specify a different token.")) - c.IncrementWarningCount() - } - - // Emit experimental warning for rate-limit feature - if workflowData.RateLimit != nil { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: rate-limit")) - c.IncrementWarningCount() - } - - // Emit experimental warning for dispatch_repository feature - if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.DispatchRepository != nil { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: dispatch_repository")) - c.IncrementWarningCount() - } - - // Emit experimental warning for merge-pull-request feature - if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.MergePullRequest != nil { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: merge-pull-request")) - c.IncrementWarningCount() - } - - // Inform users when this workflow is a redirect stub for updates. - if workflowData.Redirect != "" { - fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "info", - "workflow redirect configured: updates move to "+workflowData.Redirect)) - } - - // Validate workflow_run triggers have branch restrictions - log.Printf("Validating workflow_run triggers for branch restrictions") - if err := c.validateWorkflowRunBranches(workflowData, markdownPath); err != nil { + if err := c.validateFeatureConfig(workflowData, markdownPath); err != nil { return err } - // Reuse the permissions parsed earlier (workflowPermissions) for all subsequent checks. - // This avoids an additional NewPermissionsParser call here. - cachedPermissions := workflowPermissions - - // Validate permissions against GitHub MCP toolsets - log.Printf("Validating permissions for GitHub MCP toolsets") - if workflowData.ParsedTools != nil && workflowData.ParsedTools.GitHub != nil { - // Check if GitHub tool was explicitly configured in frontmatter - // If permissions exist but tools.github was NOT explicitly configured, - // skip validation and let the GitHub MCP server handle permission issues - hasPermissions := workflowData.Permissions != "" - - log.Printf("Permission validation check: hasExplicitGitHubTool=%v, hasPermissions=%v", - workflowData.HasExplicitGitHubTool, hasPermissions) - - // Skip validation if permissions exist but GitHub tool was auto-added (not explicit) - if hasPermissions && !workflowData.HasExplicitGitHubTool { - log.Printf("Skipping permission validation: permissions exist but tools.github not explicitly configured") - } else { - // Validate permissions using the typed GitHub tool configuration - validationResult := ValidatePermissions(cachedPermissions, workflowData.ParsedTools.GitHub) - - if validationResult.HasValidationIssues { - // Format the validation message - message := FormatValidationMessage(validationResult, c.strictMode) - - if len(validationResult.MissingPermissions) > 0 { - downgradeToWarning := c.strictMode && shouldDowngradeDefaultToolsetPermissionError(workflowData.ParsedTools.GitHub) - if c.strictMode && !downgradeToWarning { - // In strict mode, missing permissions are errors - return formatCompilerError(markdownPath, "error", message, nil) - } - - if downgradeToWarning { - message += "\n\n" + missingPermissionsDefaultToolsetWarning - } - - // In non-strict mode, missing permissions are warnings. - // In strict mode with default-only toolsets, this is intentionally downgraded to warning. - fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", message)) - c.IncrementWarningCount() - } - } - } - } - - // Emit warning if id-token: write permission is detected - log.Printf("Checking for 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() - } - - // Validate GitHub tools against enabled toolsets - log.Printf("Validating GitHub tools against enabled toolsets") - if workflowData.ParsedTools != nil && workflowData.ParsedTools.GitHub != nil { - // Extract allowed tools and enabled toolsets from ParsedTools - allowedTools := workflowData.ParsedTools.GitHub.Allowed.ToStringSlice() - enabledToolsets := ParseGitHubToolsets(strings.Join(workflowData.ParsedTools.GitHub.Toolset.ToStringSlice(), ",")) - - // Validate that all allowed tools have their toolsets enabled - if err := ValidateGitHubToolsAgainstToolsets(allowedTools, enabledToolsets); err != nil { - return formatCompilerError(markdownPath, "error", err.Error(), err) - } - - // Print informational message if "projects" toolset is explicitly specified - // (not when implied by "all", as users unlikely intend to use projects with "all") - originalToolsets := workflowData.ParsedTools.GitHub.Toolset.ToStringSlice() - if slices.Contains(originalToolsets, "projects") { - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("The 'projects' toolset requires additional authentication.")) - fmt.Fprintln(os.Stderr, console.FormatInfoMessage("See: https://github.github.com/gh-aw/reference/auth-projects/")) - } - } - - // Validate permissions for agentic-workflows tool - log.Printf("Validating permissions for agentic-workflows tool") - if _, hasAgenticWorkflows := workflowData.Tools["agentic-workflows"]; hasAgenticWorkflows { - // Check if actions: read permission exists - actionsLevel, hasActions := cachedPermissions.Get(PermissionActions) - if !hasActions || actionsLevel == PermissionNone { - // Missing actions: read permission - message := "ERROR: Missing required permission for agentic-workflows tool:\n" - message += " - actions: read\n\n" - message += "The agentic-workflows tool requires actions: read permission to access GitHub Actions data.\n\n" - message += "Suggested fix: Add the following to your workflow frontmatter:\n" - message += "permissions:\n" - message += " actions: read" - - return formatCompilerError(markdownPath, "error", message, nil) - } - } - - // Validate resources field — GitHub Actions expression syntax is not allowed. - log.Printf("Validating resources field") - if workflowData.ParsedFrontmatter != nil { - for _, r := range workflowData.ParsedFrontmatter.Resources { - if strings.Contains(r, "${{") { - return formatCompilerError(markdownPath, "error", - fmt.Sprintf("resources entry %q contains GitHub Actions expression syntax (${{) which is not allowed; use static paths only", r), nil) - } - } - } - - // Validate dispatch-workflow configuration (independent of agentic-workflows tool) - log.Print("Validating dispatch-workflow configuration") - if err := c.validateDispatchWorkflow(workflowData, markdownPath); err != nil { - return formatCompilerError(markdownPath, "error", fmt.Sprintf("dispatch-workflow validation failed: %v", err), err) - } - - // Validate dispatch_repository configuration (independent of agentic-workflows tool) - log.Print("Validating dispatch_repository configuration") - if err := c.validateDispatchRepository(workflowData, markdownPath); err != nil { - return formatCompilerError(markdownPath, "error", fmt.Sprintf("dispatch_repository validation failed: %v", err), err) - } - - // Validate call-workflow configuration (independent of agentic-workflows tool) - log.Print("Validating call-workflow configuration") - if err := c.validateCallWorkflow(workflowData, markdownPath); err != nil { - return formatCompilerError(markdownPath, "error", fmt.Sprintf("call-workflow validation failed: %v", err), err) + cachedPermissions, err := c.validatePermissions(workflowData, markdownPath) + if err != nil { + return err } - return nil + return c.validateToolConfiguration(workflowData, markdownPath, cachedPermissions) } // shouldDowngradeDefaultToolsetPermissionError returns true when strict-mode @@ -772,7 +404,12 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // Validate workflow data if err := c.validateWorkflowData(workflowData, markdownPath); err != nil { - return err + // validateWorkflowData always returns formatCompilerError results; pass through directly. + // If an unformatted error somehow slips through, wrap it with context. + if isFormattedCompilerError(err) { + return err + } + return fmt.Errorf("workflow validation: %w", err) } // Note: Markdown content size is now handled by splitting into multiple steps in generatePrompt @@ -786,7 +423,12 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // safe update enforcement. yamlContent, bodySecrets, bodyActions, err := c.generateAndValidateYAML(workflowData, markdownPath, lockFile) if err != nil { - return err + // generateAndValidateYAML always returns formatCompilerError results; pass through directly. + // If an unformatted error somehow slips through, wrap it with context. + if isFormattedCompilerError(err) { + return err + } + return fmt.Errorf("YAML generation: %w", err) } // Enforce safe update mode: emit a warning prompt (not a hard error) when unapproved diff --git a/pkg/workflow/compiler_validators.go b/pkg/workflow/compiler_validators.go new file mode 100644 index 0000000000..529c6471a2 --- /dev/null +++ b/pkg/workflow/compiler_validators.go @@ -0,0 +1,425 @@ +package workflow + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "slices" + "strings" + + "github.com/github/gh-aw/pkg/console" +) + +// validateExpressions checks expression safety and runtime-import file references +// embedded in the workflow's markdown content. It is the first validator called in +// validateWorkflowData and guards against unsafe GitHub Actions expressions. +func (c *Compiler) validateExpressions(workflowData *WorkflowData, markdownPath string) error { + // Validate expression safety - check that all GitHub Actions expressions are in the allowed list + if strings.Contains(workflowData.MarkdownContent, "${{") { + log.Printf("Validating expression safety") + if err := validateExpressionSafety(workflowData.MarkdownContent); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + } + + // Validate expressions in runtime-import files at compile time + if strings.Contains(workflowData.MarkdownContent, "{{#runtime-import") { + log.Printf("Validating runtime-import files") + // Go up from .github/workflows/file.md to repo root + workflowDir := filepath.Dir(markdownPath) // .github/workflows + githubDir := filepath.Dir(workflowDir) // .github + workspaceDir := filepath.Dir(githubDir) // repo root + if err := validateRuntimeImportFiles(workflowData.MarkdownContent, workspaceDir); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + } + + return nil +} + +// validateFeatureConfig validates feature flags declared in the workflow frontmatter +// and applies any action-mode override specified via the "action-mode" feature flag. +func (c *Compiler) validateFeatureConfig(workflowData *WorkflowData, markdownPath string) error { + // Validate feature flags + log.Printf("Validating feature flags") + if err := validateFeatures(workflowData); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Check for action-mode feature flag override + if workflowData.Features != nil { + if actionModeVal, exists := workflowData.Features["action-mode"]; exists { + if actionModeStr, ok := actionModeVal.(string); ok && actionModeStr != "" { + mode := ActionMode(actionModeStr) + if !mode.IsValid() { + return formatCompilerError(markdownPath, "error", fmt.Sprintf("invalid action-mode feature flag '%s'. Must be 'dev', 'release', or 'script'", actionModeStr), nil) + } + log.Printf("Overriding action mode from feature flag: %s", mode) + c.SetActionMode(mode) + } + } + } + + return nil +} + +// validatePermissions validates all permission-related configuration: dangerous +// permissions, GitHub App-only constraints, MCP app write restrictions, workflow_run +// branch security, GitHub MCP toolset permissions, and the id-token write warning. +// It returns the parsed *Permissions for reuse in subsequent validation steps. +func (c *Compiler) validatePermissions(workflowData *WorkflowData, markdownPath string) (*Permissions, error) { + // Parse permissions once for all permission-related validation checks below. + // WorkflowData.Permissions contains the raw YAML string (including "permissions:" prefix). + // Parsing once here avoids redundant YAML parsing in each validator. + workflowPermissions := NewPermissionsParser(workflowData.Permissions).ToPermissions() + + // Validate dangerous permissions + log.Printf("Validating dangerous permissions") + if err := validateDangerousPermissions(workflowData, workflowPermissions); err != nil { + return nil, formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate GitHub App-only permissions require a GitHub App to be configured + log.Printf("Validating GitHub App-only permissions") + if err := validateGitHubAppOnlyPermissions(workflowData, workflowPermissions); err != nil { + return nil, formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate tools.github.github-app.permissions does not use "write" + log.Printf("Validating GitHub MCP app permissions (no write)") + if err := validateGitHubMCPAppPermissionsNoWrite(workflowData); err != nil { + return nil, formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Warn when github-app.permissions is set in contexts that don't support it + warnGitHubAppPermissionsUnsupportedContexts(workflowData) + + // Validate workflow_run triggers have branch restrictions + log.Printf("Validating workflow_run triggers for branch restrictions") + if err := c.validateWorkflowRunBranches(workflowData, markdownPath); err != nil { + return nil, err + } + + // Validate permissions against GitHub MCP toolsets + log.Printf("Validating permissions for GitHub MCP toolsets") + if workflowData.ParsedTools != nil && workflowData.ParsedTools.GitHub != nil { + // Check if GitHub tool was explicitly configured in frontmatter + // If permissions exist but tools.github was NOT explicitly configured, + // skip validation and let the GitHub MCP server handle permission issues + hasPermissions := workflowData.Permissions != "" + + log.Printf("Permission validation check: hasExplicitGitHubTool=%v, hasPermissions=%v", + workflowData.HasExplicitGitHubTool, hasPermissions) + + // Skip validation if permissions exist but GitHub tool was auto-added (not explicit) + if hasPermissions && !workflowData.HasExplicitGitHubTool { + log.Printf("Skipping permission validation: permissions exist but tools.github not explicitly configured") + } else { + // Validate permissions using the typed GitHub tool configuration + validationResult := ValidatePermissions(workflowPermissions, workflowData.ParsedTools.GitHub) + + if validationResult.HasValidationIssues { + // Format the validation message + message := FormatValidationMessage(validationResult, c.strictMode) + + if len(validationResult.MissingPermissions) > 0 { + downgradeToWarning := c.strictMode && shouldDowngradeDefaultToolsetPermissionError(workflowData.ParsedTools.GitHub) + if c.strictMode && !downgradeToWarning { + // In strict mode, missing permissions are errors + return nil, formatCompilerError(markdownPath, "error", message, nil) + } + + if downgradeToWarning { + message += "\n\n" + missingPermissionsDefaultToolsetWarning + } + + // In non-strict mode, missing permissions are warnings. + // In strict mode with default-only toolsets, this is intentionally downgraded to warning. + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", message)) + c.IncrementWarningCount() + } + } + } + } + + // Emit warning if id-token: write permission is detected + log.Printf("Checking for id-token: write permission") + if level, exists := workflowPermissions.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() + } + + return workflowPermissions, nil +} + +// validateToolConfiguration validates safe-outputs settings, on.needs and safe-job +// declarations, network configuration, labels, concurrency expressions, sandbox +// security constraints, GitHub tool-to-toolset alignment, the agentic-workflows +// permission requirement, and dispatch/call-workflow configurations. +// cachedPermissions is the *Permissions value returned by validatePermissions. +func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdownPath string, cachedPermissions *Permissions) error { + // Validate agent file exists if specified in engine config + log.Printf("Validating agent file if specified") + if err := c.validateAgentFile(workflowData, markdownPath); err != nil { + // validateAgentFile always returns formatCompilerError results; pass through directly. + return err + } + + // Validate sandbox configuration + log.Printf("Validating sandbox configuration") + if err := validateSandboxConfig(workflowData); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate safe-outputs target configuration + log.Printf("Validating safe-outputs target fields") + if err := validateSafeOutputsTarget(workflowData.SafeOutputs); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate safe-outputs max configuration + log.Printf("Validating safe-outputs max fields") + if err := validateSafeOutputsMax(workflowData.SafeOutputs); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate safe-outputs allowed-domains configuration + log.Printf("Validating safe-outputs allowed-domains") + if err := c.validateSafeOutputsAllowedDomains(workflowData.SafeOutputs); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate safe-outputs merge-pull-request configuration + log.Printf("Validating safe-outputs merge-pull-request") + if err := validateSafeOutputsMergePullRequest(workflowData.SafeOutputs); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate safe-outputs needs declarations + log.Printf("Validating safe-outputs needs declarations") + if err := validateSafeOutputsNeeds(workflowData); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate on.needs declarations and on.github-app needs expressions + log.Printf("Validating on.needs declarations") + if err := c.validateOnNeeds(workflowData); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate safe-job needs: declarations against known generated job IDs + log.Printf("Validating safe-job needs declarations") + if err := validateSafeJobNeeds(workflowData); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Emit warnings for push-to-pull-request-branch misconfiguration + log.Printf("Validating push-to-pull-request-branch configuration") + c.validatePushToPullRequestBranchWarnings(workflowData.SafeOutputs, workflowData.CheckoutConfigs) + + // Validate network allowed domains configuration + log.Printf("Validating network allowed domains") + if err := c.validateNetworkAllowedDomains(workflowData.NetworkPermissions); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate network firewall configuration + log.Printf("Validating network firewall configuration") + if err := validateNetworkFirewallConfig(workflowData.NetworkPermissions); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate safe-outputs allow-workflows requires GitHub App + log.Printf("Validating safe-outputs allow-workflows") + if err := validateSafeOutputsAllowWorkflows(workflowData.SafeOutputs); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate labels configuration + log.Printf("Validating labels") + if err := validateLabels(workflowData); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Validate workflow-level concurrency group expression + log.Printf("Validating workflow-level concurrency configuration") + if workflowData.Concurrency != "" { + // Extract the group expression from the concurrency YAML + // The Concurrency field contains the full YAML (e.g., "concurrency:\n group: \"...\"") + // We need to extract just the group value + groupExpr := extractConcurrencyGroupFromYAML(workflowData.Concurrency) + if groupExpr != "" { + if err := validateConcurrencyGroupExpression(groupExpr); err != nil { + return formatCompilerError(markdownPath, "error", "workflow-level concurrency validation failed: "+err.Error(), err) + } + } + } + + // Validate concurrency.job-discriminator expression + if workflowData.ConcurrencyJobDiscriminator != "" { + if err := validateConcurrencyGroupExpression(workflowData.ConcurrencyJobDiscriminator); err != nil { + return formatCompilerError(markdownPath, "error", "concurrency.job-discriminator validation failed: "+err.Error(), err) + } + } + + // Validate engine-level concurrency group expression + log.Printf("Validating engine-level concurrency configuration") + if workflowData.EngineConfig != nil && workflowData.EngineConfig.Concurrency != "" { + // Extract the group expression from the engine concurrency YAML + groupExpr := extractConcurrencyGroupFromYAML(workflowData.EngineConfig.Concurrency) + if groupExpr != "" { + if err := validateConcurrencyGroupExpression(groupExpr); err != nil { + return formatCompilerError(markdownPath, "error", "engine.concurrency validation failed: "+err.Error(), err) + } + } + } + + // Validate safe-outputs concurrency group expression + if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.ConcurrencyGroup != "" { + if err := validateConcurrencyGroupExpression(workflowData.SafeOutputs.ConcurrencyGroup); err != nil { + return formatCompilerError(markdownPath, "error", "safe-outputs.concurrency-group validation failed: "+err.Error(), err) + } + } + + // Warn when the user has specified custom workflow-level concurrency with cancel-in-progress: true + // AND the workflow has the bot self-cancel risk combination (issue_comment triggers + GitHub App + // safe-outputs). In this case the auto-generated bot-actor isolation cannot be applied because the + // user's concurrency expression is preserved as-is. The user must add the bot-actor isolation + // themselves (e.g. prepend `contains(github.actor, '[bot]') && github.run_id ||` to their group key). + if workflowData.Concurrency != "" && + strings.Contains(workflowData.Concurrency, "cancel-in-progress: true") && + hasBotSelfCancelRisk(workflowData) { + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", + "Custom workflow-level concurrency with cancel-in-progress: true may cause self-cancellation.\n"+ + "safe-outputs.github-app can post comments that re-trigger this workflow via issue_comment,\n"+ + "and those passive bot-authored runs can collide with the primary run's concurrency group.\n"+ + "Add `contains(github.actor, '[bot]') && github.run_id ||` at the start of your concurrency\n"+ + "group expression to route bot-triggered runs to a unique key and prevent self-cancellation.\n"+ + "See: https://gh.io/gh-aw/reference/concurrency for details.")) + c.IncrementWarningCount() + } + + // Emit warning for sandbox.agent: false (disables agent sandbox firewall) + if isAgentSandboxDisabled(workflowData) { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage("⚠️ WARNING: Agent sandbox disabled (sandbox.agent: false). This removes firewall protection. The AI agent will have direct network access without firewall filtering. The MCP gateway remains enabled. Only use this for testing or in controlled environments where you trust the AI agent completely.")) + c.IncrementWarningCount() + } + + // Validate: threat detection requires sandbox.agent to be enabled (detection runs inside AWF) + if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.ThreatDetection != nil && isAgentSandboxDisabled(workflowData) { + return formatCompilerError(markdownPath, "error", "threat detection requires sandbox.agent to be enabled. Threat detection runs inside the agent sandbox (AWF) with fully blocked network. Either enable sandbox.agent or use 'threat-detection: false' to disable the threat-detection configuration in safe-outputs.", errors.New("threat detection requires sandbox.agent")) + } + + // Emit warning when assign-to-agent is used with github-app: but no explicit github-token:. + // GitHub App tokens are rejected by the Copilot assignment API — a PAT is required. + // The token fallback chain (GH_AW_AGENT_TOKEN || GH_AW_GITHUB_TOKEN || GITHUB_TOKEN) is used automatically. + if workflowData.SafeOutputs != nil && + workflowData.SafeOutputs.AssignToAgent != nil && + workflowData.SafeOutputs.GitHubApp != nil && + workflowData.SafeOutputs.AssignToAgent.GitHubToken == "" { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage( + "assign-to-agent does not support GitHub App tokens. "+ + "The Copilot assignment API requires a fine-grained PAT. "+ + "The token fallback chain (GH_AW_AGENT_TOKEN || GH_AW_GITHUB_TOKEN || GITHUB_TOKEN) will be used automatically. "+ + "Add github-token: to your assign-to-agent config to specify a different token.")) + c.IncrementWarningCount() + } + + // Emit experimental warning for rate-limit feature + if workflowData.RateLimit != nil { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: rate-limit")) + c.IncrementWarningCount() + } + + // Emit experimental warning for dispatch_repository feature + if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.DispatchRepository != nil { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: dispatch_repository")) + c.IncrementWarningCount() + } + + // Emit experimental warning for merge-pull-request feature + if workflowData.SafeOutputs != nil && workflowData.SafeOutputs.MergePullRequest != nil { + fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Using experimental feature: merge-pull-request")) + c.IncrementWarningCount() + } + + // Inform users when this workflow is a redirect stub for updates. + if workflowData.Redirect != "" { + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "info", + "workflow redirect configured: updates move to "+workflowData.Redirect)) + } + + // Validate GitHub tools against enabled toolsets + log.Printf("Validating GitHub tools against enabled toolsets") + if workflowData.ParsedTools != nil && workflowData.ParsedTools.GitHub != nil { + // Extract allowed tools and enabled toolsets from ParsedTools + allowedTools := workflowData.ParsedTools.GitHub.Allowed.ToStringSlice() + enabledToolsets := ParseGitHubToolsets(strings.Join(workflowData.ParsedTools.GitHub.Toolset.ToStringSlice(), ",")) + + // Validate that all allowed tools have their toolsets enabled + if err := ValidateGitHubToolsAgainstToolsets(allowedTools, enabledToolsets); err != nil { + return formatCompilerError(markdownPath, "error", err.Error(), err) + } + + // Print informational message if "projects" toolset is explicitly specified + // (not when implied by "all", as users unlikely intend to use projects with "all") + originalToolsets := workflowData.ParsedTools.GitHub.Toolset.ToStringSlice() + if slices.Contains(originalToolsets, "projects") { + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("The 'projects' toolset requires additional authentication.")) + fmt.Fprintln(os.Stderr, console.FormatInfoMessage("See: https://github.github.com/gh-aw/reference/auth-projects/")) + } + } + + // Validate permissions for agentic-workflows tool + log.Printf("Validating permissions for agentic-workflows tool") + if _, hasAgenticWorkflows := workflowData.Tools["agentic-workflows"]; hasAgenticWorkflows { + // Check if actions: read permission exists + actionsLevel, hasActions := cachedPermissions.Get(PermissionActions) + if !hasActions || actionsLevel == PermissionNone { + // Missing actions: read permission + message := "ERROR: Missing required permission for agentic-workflows tool:\n" + message += " - actions: read\n\n" + message += "The agentic-workflows tool requires actions: read permission to access GitHub Actions data.\n\n" + message += "Suggested fix: Add the following to your workflow frontmatter:\n" + message += "permissions:\n" + message += " actions: read" + + return formatCompilerError(markdownPath, "error", message, nil) + } + } + + // Validate resources field — GitHub Actions expression syntax is not allowed. + log.Printf("Validating resources field") + if workflowData.ParsedFrontmatter != nil { + for _, r := range workflowData.ParsedFrontmatter.Resources { + if strings.Contains(r, "${{") { + return formatCompilerError(markdownPath, "error", + fmt.Sprintf("resources entry %q contains GitHub Actions expression syntax (${{) which is not allowed; use static paths only", r), nil) + } + } + } + + // Validate dispatch-workflow configuration (independent of agentic-workflows tool) + log.Print("Validating dispatch-workflow configuration") + if err := c.validateDispatchWorkflow(workflowData, markdownPath); err != nil { + return formatCompilerError(markdownPath, "error", fmt.Sprintf("dispatch-workflow validation failed: %v", err), err) + } + + // Validate dispatch_repository configuration (independent of agentic-workflows tool) + log.Print("Validating dispatch_repository configuration") + if err := c.validateDispatchRepository(workflowData, markdownPath); err != nil { + return formatCompilerError(markdownPath, "error", fmt.Sprintf("dispatch_repository validation failed: %v", err), err) + } + + // Validate call-workflow configuration (independent of agentic-workflows tool) + log.Print("Validating call-workflow configuration") + if err := c.validateCallWorkflow(workflowData, markdownPath); err != nil { + return formatCompilerError(markdownPath, "error", fmt.Sprintf("call-workflow validation failed: %v", err), err) + } + + return nil +} diff --git a/pkg/workflow/compiler_validators_test.go b/pkg/workflow/compiler_validators_test.go new file mode 100644 index 0000000000..04f55dbdc1 --- /dev/null +++ b/pkg/workflow/compiler_validators_test.go @@ -0,0 +1,262 @@ +//go:build !integration + +package workflow + +import ( + "path/filepath" + "testing" + + "github.com/github/gh-aw/pkg/testutil" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestValidateExpressions tests expression safety and runtime-import validation. +func TestValidateExpressions(t *testing.T) { + tests := []struct { + name string + markdown string + shouldError bool + errorContains string + }{ + { + name: "no expressions", + markdown: "# Hello\n\nNo expressions here.", + shouldError: false, + }, + { + name: "safe expression", + markdown: "# Hello\n\n${{ github.event.issue.number }}", + shouldError: false, + }, + { + name: "unsafe expression in markdown", + markdown: "# Hello\n\n${{ github.event.issue.body }}", + shouldError: true, + errorContains: "unauthorized expressions found", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "expr-test") + markdownPath := filepath.Join(tmpDir, "test.md") + + compiler := NewCompiler() + workflowData := &WorkflowData{ + Name: "Test", + MarkdownContent: tt.markdown, + AI: "copilot", + } + + err := compiler.validateExpressions(workflowData, markdownPath) + if tt.shouldError { + require.Error(t, err, "Expected validateExpressions to return an error") + if tt.errorContains != "" { + assert.Contains(t, err.Error(), tt.errorContains, "Error should contain expected message") + } + } else { + assert.NoError(t, err, "validateExpressions should not return an error") + } + }) + } +} + +// TestValidateFeatureConfig tests feature flag and action-mode validation. +func TestValidateFeatureConfig(t *testing.T) { + tests := []struct { + name string + features map[string]any + shouldError bool + errorContains string + }{ + { + name: "no features", + features: nil, + shouldError: false, + }, + { + name: "valid action-mode dev", + features: map[string]any{ + "action-mode": "dev", + }, + shouldError: false, + }, + { + name: "valid action-mode release", + features: map[string]any{ + "action-mode": "release", + }, + shouldError: false, + }, + { + name: "invalid action-mode", + features: map[string]any{ + "action-mode": "invalid-mode", + }, + shouldError: true, + errorContains: "invalid action-mode feature flag", + }, + { + name: "empty action-mode is ignored", + features: map[string]any{ + "action-mode": "", + }, + shouldError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "feature-test") + markdownPath := filepath.Join(tmpDir, "test.md") + + compiler := NewCompiler() + workflowData := &WorkflowData{ + Name: "Test", + MarkdownContent: "# Test", + AI: "copilot", + Features: tt.features, + } + + err := compiler.validateFeatureConfig(workflowData, markdownPath) + if tt.shouldError { + require.Error(t, err, "Expected validateFeatureConfig to return an error") + if tt.errorContains != "" { + assert.Contains(t, err.Error(), tt.errorContains, "Error should contain expected message") + } + } else { + assert.NoError(t, err, "validateFeatureConfig should not return an error") + } + }) + } +} + +// TestValidatePermissions tests permission parsing and MCP tool constraint validation. +func TestValidatePermissions(t *testing.T) { + tests := []struct { + name string + workflowData *WorkflowData + strictMode bool + shouldError bool + errorContains string + wantPermissions bool // whether the returned *Permissions should be non-nil + }{ + { + name: "no permissions returns empty Permissions", + workflowData: &WorkflowData{ + Name: "Test", + MarkdownContent: "# Test", + AI: "copilot", + Permissions: "", + }, + shouldError: false, + wantPermissions: true, + }, + { + name: "valid permissions parses successfully", + workflowData: &WorkflowData{ + Name: "Test", + MarkdownContent: "# Test", + AI: "copilot", + Permissions: "permissions:\n contents: read\n", + }, + shouldError: false, + wantPermissions: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "perms-test") + markdownPath := filepath.Join(tmpDir, "test.md") + + compiler := NewCompiler() + compiler.strictMode = tt.strictMode + + perms, err := compiler.validatePermissions(tt.workflowData, markdownPath) + if tt.shouldError { + require.Error(t, err, "Expected validatePermissions to return an error") + if tt.errorContains != "" { + assert.Contains(t, err.Error(), tt.errorContains, "Error should contain expected message") + } + } else { + require.NoError(t, err, "validatePermissions should not return an error") + if tt.wantPermissions { + assert.NotNil(t, perms, "validatePermissions should return a non-nil *Permissions") + } + } + }) + } +} + +// TestValidateToolConfiguration tests safe-outputs, GitHub tools, and dispatch validation. +func TestValidateToolConfiguration(t *testing.T) { + tests := []struct { + name string + workflowData *WorkflowData + permissions string // raw permissions YAML to parse + shouldError bool + errorContains string + }{ + { + name: "minimal workflow passes", + workflowData: &WorkflowData{ + Name: "Test", + MarkdownContent: "# Test", + AI: "copilot", + }, + permissions: "", + shouldError: false, + }, + { + name: "agentic-workflows tool requires actions read permission", + workflowData: &WorkflowData{ + Name: "Test", + MarkdownContent: "# Test", + AI: "copilot", + Tools: map[string]any{ + "agentic-workflows": map[string]any{}, + }, + Permissions: "", + }, + permissions: "", + shouldError: true, + errorContains: "Missing required permission for agentic-workflows tool", + }, + { + name: "agentic-workflows tool with actions read succeeds", + workflowData: &WorkflowData{ + Name: "Test", + MarkdownContent: "# Test", + AI: "copilot", + Tools: map[string]any{ + "agentic-workflows": map[string]any{}, + }, + Permissions: "permissions:\n actions: read\n", + }, + permissions: "permissions:\n actions: read\n", + shouldError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := testutil.TempDir(t, "tool-test") + markdownPath := filepath.Join(tmpDir, "test.md") + + compiler := NewCompiler() + cachedPermissions := NewPermissionsParser(tt.permissions).ToPermissions() + + err := compiler.validateToolConfiguration(tt.workflowData, markdownPath, cachedPermissions) + if tt.shouldError { + require.Error(t, err, "Expected validateToolConfiguration to return an error") + if tt.errorContains != "" { + assert.Contains(t, err.Error(), tt.errorContains, "Error should contain expected message") + } + } else { + assert.NoError(t, err, "validateToolConfiguration should not return an error") + } + }) + } +} From c15b30f9c8a2a9fe07433e7ca021c28a45b8af80 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 19:10:55 +0000 Subject: [PATCH 3/4] refactor: rename cachedPermissions to workflowPermissions for clarity Address code review feedback: rename the parameter and local variable from 'cachedPermissions' to 'workflowPermissions' in validateToolConfiguration, validateWorkflowData, and the test file to better reflect it is a freshly parsed permissions object rather than a performance cache. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/7c5f96e4-396a-4b5a-b1df-52d8c72b86d7 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler.go | 4 ++-- pkg/workflow/compiler_validators.go | 6 +++--- pkg/workflow/compiler_validators_test.go | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index 6b841ce5bc..a317cdc320 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -91,12 +91,12 @@ func (c *Compiler) validateWorkflowData(workflowData *WorkflowData, markdownPath return err } - cachedPermissions, err := c.validatePermissions(workflowData, markdownPath) + workflowPermissions, err := c.validatePermissions(workflowData, markdownPath) if err != nil { return err } - return c.validateToolConfiguration(workflowData, markdownPath, cachedPermissions) + return c.validateToolConfiguration(workflowData, markdownPath, workflowPermissions) } // shouldDowngradeDefaultToolsetPermissionError returns true when strict-mode diff --git a/pkg/workflow/compiler_validators.go b/pkg/workflow/compiler_validators.go index 529c6471a2..9de60980ea 100644 --- a/pkg/workflow/compiler_validators.go +++ b/pkg/workflow/compiler_validators.go @@ -160,8 +160,8 @@ Ensure proper audience validation and trust policies are configured.` // declarations, network configuration, labels, concurrency expressions, sandbox // security constraints, GitHub tool-to-toolset alignment, the agentic-workflows // permission requirement, and dispatch/call-workflow configurations. -// cachedPermissions is the *Permissions value returned by validatePermissions. -func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdownPath string, cachedPermissions *Permissions) error { +// workflowPermissions is the *Permissions value returned by validatePermissions. +func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdownPath string, workflowPermissions *Permissions) error { // Validate agent file exists if specified in engine config log.Printf("Validating agent file if specified") if err := c.validateAgentFile(workflowData, markdownPath); err != nil { @@ -378,7 +378,7 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow log.Printf("Validating permissions for agentic-workflows tool") if _, hasAgenticWorkflows := workflowData.Tools["agentic-workflows"]; hasAgenticWorkflows { // Check if actions: read permission exists - actionsLevel, hasActions := cachedPermissions.Get(PermissionActions) + actionsLevel, hasActions := workflowPermissions.Get(PermissionActions) if !hasActions || actionsLevel == PermissionNone { // Missing actions: read permission message := "ERROR: Missing required permission for agentic-workflows tool:\n" diff --git a/pkg/workflow/compiler_validators_test.go b/pkg/workflow/compiler_validators_test.go index 04f55dbdc1..1093826b38 100644 --- a/pkg/workflow/compiler_validators_test.go +++ b/pkg/workflow/compiler_validators_test.go @@ -246,9 +246,9 @@ func TestValidateToolConfiguration(t *testing.T) { markdownPath := filepath.Join(tmpDir, "test.md") compiler := NewCompiler() - cachedPermissions := NewPermissionsParser(tt.permissions).ToPermissions() + parsedPermissions := NewPermissionsParser(tt.permissions).ToPermissions() - err := compiler.validateToolConfiguration(tt.workflowData, markdownPath, cachedPermissions) + err := compiler.validateToolConfiguration(tt.workflowData, markdownPath, parsedPermissions) if tt.shouldError { require.Error(t, err, "Expected validateToolConfiguration to return an error") if tt.errorContains != "" { From d0cd18c7100fa4de26dd762105af59f3f1628b59 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 23 Apr 2026 21:01:29 +0000 Subject: [PATCH 4/4] fix: address review feedback on compiler_validators and compiler error formatting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Replace console.FormatWarningMessage("⚠️ WARNING: ...") with formatCompilerMessage(markdownPath, "warning", ...) for the sandbox disabled warning to avoid duplicate icon/prefix and add file context - Replace fmt.Errorf fallback in CompileWorkflowData with formatCompilerError calls so unformatted errors still get proper compiler diagnostics context with file path Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c3ba6830-0d95-4be1-878f-3ea9dc53b5c2 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler.go | 8 ++++---- pkg/workflow/compiler_validators.go | 6 +++++- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index a317cdc320..4224243235 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -405,11 +405,11 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath // Validate workflow data if err := c.validateWorkflowData(workflowData, markdownPath); err != nil { // validateWorkflowData always returns formatCompilerError results; pass through directly. - // If an unformatted error somehow slips through, wrap it with context. + // If an unformatted error somehow slips through, wrap it with compiler context. if isFormattedCompilerError(err) { return err } - return fmt.Errorf("workflow validation: %w", err) + return formatCompilerError(markdownPath, "error", "workflow validation: "+err.Error(), err) } // Note: Markdown content size is now handled by splitting into multiple steps in generatePrompt @@ -424,11 +424,11 @@ func (c *Compiler) CompileWorkflowData(workflowData *WorkflowData, markdownPath yamlContent, bodySecrets, bodyActions, err := c.generateAndValidateYAML(workflowData, markdownPath, lockFile) if err != nil { // generateAndValidateYAML always returns formatCompilerError results; pass through directly. - // If an unformatted error somehow slips through, wrap it with context. + // If an unformatted error somehow slips through, wrap it with compiler context. if isFormattedCompilerError(err) { return err } - return fmt.Errorf("YAML generation: %w", err) + return formatCompilerError(markdownPath, "error", "YAML generation: "+err.Error(), err) } // Enforce safe update mode: emit a warning prompt (not a hard error) when unapproved diff --git a/pkg/workflow/compiler_validators.go b/pkg/workflow/compiler_validators.go index 9de60980ea..5167c9636d 100644 --- a/pkg/workflow/compiler_validators.go +++ b/pkg/workflow/compiler_validators.go @@ -305,7 +305,11 @@ func (c *Compiler) validateToolConfiguration(workflowData *WorkflowData, markdow // Emit warning for sandbox.agent: false (disables agent sandbox firewall) if isAgentSandboxDisabled(workflowData) { - fmt.Fprintln(os.Stderr, console.FormatWarningMessage("⚠️ WARNING: Agent sandbox disabled (sandbox.agent: false). This removes firewall protection. The AI agent will have direct network access without firewall filtering. The MCP gateway remains enabled. Only use this for testing or in controlled environments where you trust the AI agent completely.")) + fmt.Fprintln(os.Stderr, formatCompilerMessage(markdownPath, "warning", + "Agent sandbox disabled (sandbox.agent: false). This removes firewall protection. "+ + "The AI agent will have direct network access without firewall filtering. "+ + "The MCP gateway remains enabled. Only use this for testing or in controlled "+ + "environments where you trust the AI agent completely.")) c.IncrementWarningCount() }