Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions pkg/parser/schema_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,49 @@ func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_WorkflowDispatchNu
}
}

func TestValidateMainWorkflowFrontmatterWithSchemaAndLocation_EngineDriverPattern(t *testing.T) {
t.Parallel()

validFrontmatter := map[string]any{
"on": "push",
"engine": map[string]any{
"id": "claude",
"driver": "custom_driver.cjs",
},
}

err := ValidateMainWorkflowFrontmatterWithSchemaAndLocation(validFrontmatter, "/tmp/gh-aw/engine-driver-valid-pattern-test.md")
if err != nil {
t.Fatalf("expected valid engine.driver pattern to pass schema validation, got: %v", err)
}

invalidFrontmatter := map[string]any{
"on": "push",
"engine": map[string]any{
"id": "claude",
"driver": "../driver.cjs",
},
}

err = ValidateMainWorkflowFrontmatterWithSchemaAndLocation(invalidFrontmatter, "/tmp/gh-aw/engine-driver-invalid-pattern-test.md")
if err == nil {
t.Fatal("expected invalid engine.driver pattern to fail schema validation")
}

invalidFlagLikeFrontmatter := map[string]any{
"on": "push",
"engine": map[string]any{
"id": "claude",
"driver": "-driver.cjs",
},
}

err = ValidateMainWorkflowFrontmatterWithSchemaAndLocation(invalidFlagLikeFrontmatter, "/tmp/gh-aw/engine-driver-invalid-flaglike-pattern-test.md")
if err == nil {
t.Fatal("expected flag-like engine.driver pattern to fail schema validation")
}
}

func TestMainWorkflowSchema_WorkflowDispatchNumberTypeDocumentation(t *testing.T) {
t.Parallel()

Expand Down
5 changes: 5 additions & 0 deletions pkg/parser/schemas/main_workflow_schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -9497,6 +9497,11 @@
"type": "string",
"description": "Custom executable path for the AI engine CLI. When specified, the workflow will skip the standard installation steps and use this command instead. The command should be the full path to the executable or a command available in PATH."
},
"driver": {
"type": "string",
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Schema text says engine.driver "must end with .js, .cjs, or .mjs", but the schema itself doesn’t enforce that constraint (e.g., via pattern). Adding a regex pattern here (and ideally also restricting to a safe basename) would keep schema validation consistent with validateEngineDriverScript and help catch invalid/unsafe values earlier.

Suggested change
"type": "string",
"type": "string",
"pattern": "^(?!\\.)(?!.*[\\\\/])[A-Za-z0-9._-]+\\.(?:js|cjs|mjs)$",

Copilot uses AI. Check for mistakes.
"pattern": "^[A-Za-z0-9_][A-Za-z0-9._-]*\\.(?:js|cjs|mjs)$",
"description": "Custom Node.js driver script filename for an agentic engine. This replaces the engine's built-in driver wrapper (when the engine supports one) and must end with .js, .cjs, or .mjs."
},
"env": {
"type": "object",
"description": "Custom environment variables to pass to the AI engine, including secret overrides (e.g., OPENAI_API_KEY: ${{ secrets.CUSTOM_KEY }})",
Expand Down
83 changes: 59 additions & 24 deletions pkg/workflow/action_pins_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,21 @@ import (
"github.com/github/gh-aw/pkg/testutil"
)

const setupNodeV6ExpectedUsesPlaceholder = "__setup_node_v6__"

func expectedPinnedUses(t *testing.T, repo, version string) string {
t.Helper()

result, err := getActionPinWithData(repo, version, &WorkflowData{})
if err != nil {
t.Fatalf("getActionPinWithData(%s, %s) returned error: %v", repo, version, err)
}
if result == "" {
t.Fatalf("getActionPinWithData(%s, %s) returned empty result", repo, version)
}
return result
}

// TestActionPinsExist verifies that all action pinning entries exist
func TestActionPinsExist(t *testing.T) {
// Read action pins from JSON file instead of hardcoded list
Expand Down Expand Up @@ -215,7 +230,7 @@ func TestApplyActionPinToStep(t *testing.T) {
},
},
expectPinned: true,
expectedUses: "actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6",
expectedUses: setupNodeV6ExpectedUsesPlaceholder,
},
{
name: "step with unpinned action",
Expand Down Expand Up @@ -274,8 +289,12 @@ func TestApplyActionPinToStep(t *testing.T) {
return
}

if usesStr != tt.expectedUses {
t.Errorf("applyActionPinToTypedStep uses = %q, want %q", usesStr, tt.expectedUses)
expectedUses := tt.expectedUses
if expectedUses == setupNodeV6ExpectedUsesPlaceholder {
expectedUses = expectedPinnedUses(t, "actions/setup-node", "v6")
}
if usesStr != expectedUses {
t.Errorf("applyActionPinToTypedStep uses = %q, want %q", usesStr, expectedUses)
}

// Verify other fields are preserved (check length and keys)
Expand Down Expand Up @@ -344,22 +363,23 @@ func TestGetActionPinsSorting(t *testing.T) {
// TestGetActionPinByRepo tests the getActionPinByRepo function
func TestGetActionPinByRepo(t *testing.T) {
tests := []struct {
repo string
expectExists bool
expectRepo string
expectVer string
repo string
expectExists bool
expectRepo string
expectVersion string
expectVersionPrefix string
}{
{
repo: "actions/checkout",
expectExists: true,
expectRepo: "actions/checkout",
expectVer: "v6.0.2",
repo: "actions/checkout",
expectExists: true,
expectRepo: "actions/checkout",
expectVersion: "v6.0.2",
},
{
repo: "actions/setup-node",
expectExists: true,
expectRepo: "actions/setup-node",
expectVer: "v6.3.0",
repo: "actions/setup-node",
expectExists: true,
expectRepo: "actions/setup-node",
expectVersionPrefix: "v6.",
},
{
repo: "unknown/action",
Expand All @@ -373,6 +393,10 @@ func TestGetActionPinByRepo(t *testing.T) {

for _, tt := range tests {
t.Run(tt.repo, func(t *testing.T) {
if tt.expectVersion != "" && tt.expectVersionPrefix != "" {
t.Fatalf("invalid test case: expectVersion and expectVersionPrefix are mutually exclusive")
}

pin, exists := getActionPinByRepo(tt.repo)

if exists != tt.expectExists {
Expand All @@ -383,8 +407,11 @@ func TestGetActionPinByRepo(t *testing.T) {
if pin.Repo != tt.expectRepo {
t.Errorf("getActionPinByRepo(%s) repo = %s, want %s", tt.repo, pin.Repo, tt.expectRepo)
}
if pin.Version != tt.expectVer {
t.Errorf("getActionPinByRepo(%s) version = %s, want %s", tt.repo, pin.Version, tt.expectVer)
if tt.expectVersion != "" && pin.Version != tt.expectVersion {
t.Errorf("getActionPinByRepo(%s) version = %s, want %s", tt.repo, pin.Version, tt.expectVersion)
}
if tt.expectVersionPrefix != "" && !strings.HasPrefix(pin.Version, tt.expectVersionPrefix) {
t.Errorf("getActionPinByRepo(%s) version = %s, want prefix %s", tt.repo, pin.Version, tt.expectVersionPrefix)
}
if !isValidSHA(pin.SHA) {
t.Errorf("getActionPinByRepo(%s) has invalid SHA: %s", tt.repo, pin.SHA)
Expand Down Expand Up @@ -421,7 +448,7 @@ func TestApplyActionPinToTypedStep(t *testing.T) {
},
},
expectPinned: true,
expectedUses: "actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6",
expectedUses: setupNodeV6ExpectedUsesPlaceholder,
},
{
name: "step with unpinned action",
Expand Down Expand Up @@ -484,8 +511,12 @@ func TestApplyActionPinToTypedStep(t *testing.T) {
}

// Check uses field
if result.Uses != tt.expectedUses {
t.Errorf("applyActionPinToTypedStep() uses = %q, want %q", result.Uses, tt.expectedUses)
expectedUses := tt.expectedUses
if expectedUses == setupNodeV6ExpectedUsesPlaceholder {
expectedUses = expectedPinnedUses(t, "actions/setup-node", "v6")
}
if result.Uses != expectedUses {
t.Errorf("applyActionPinToTypedStep() uses = %q, want %q", result.Uses, expectedUses)
}

// Verify other fields are preserved
Expand Down Expand Up @@ -1314,7 +1345,7 @@ func TestMapToStepWithActionPinning(t *testing.T) {
},
},
wantErr: false,
expectedUses: "actions/setup-node@53b83947a5a98c8d113130e565377fae1a50d02f # v6",
expectedUses: setupNodeV6ExpectedUsesPlaceholder,
},
}

Expand All @@ -1338,9 +1369,13 @@ func TestMapToStepWithActionPinning(t *testing.T) {
}

// Verify the result
if tt.expectedUses != "" {
if pinnedStep.Uses != tt.expectedUses {
t.Errorf("pinnedStep.Uses = %q, want %q", pinnedStep.Uses, tt.expectedUses)
expectedUses := tt.expectedUses
if expectedUses == setupNodeV6ExpectedUsesPlaceholder {
expectedUses = expectedPinnedUses(t, "actions/setup-node", "v6")
}
if expectedUses != "" {
if pinnedStep.Uses != expectedUses {
t.Errorf("pinnedStep.Uses = %q, want %q", pinnedStep.Uses, expectedUses)
}
}

Expand Down
5 changes: 5 additions & 0 deletions pkg/workflow/compiler_orchestrator_workflow.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,11 @@ func (c *Compiler) ParseWorkflowFile(markdownPath string) (*WorkflowData, error)
return nil, fmt.Errorf("%s: %w", cleanPath, err)
}

// Validate optional custom engine driver script configuration.
if err := c.validateEngineDriverScript(workflowData); err != nil {
return nil, fmt.Errorf("%s: %w", cleanPath, err)
}

// Validate that inlined-imports is not used with agent file imports.
// Agent files require runtime access and cannot be resolved without sources.
if workflowData.InlinedImports && engineSetup.importsResult.AgentFile != "" {
Expand Down
3 changes: 3 additions & 0 deletions pkg/workflow/copilot_engine_execution.go
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,9 @@ func (e *CopilotEngine) GetExecutionSteps(workflowData *WorkflowData, logFile st
// - Fall back to `command -v node` if GH_AW_NODE_BIN points to a non-mounted toolcache path.
// This prevents agent startup failures when host toolcache paths are not present in the AWF container.
driverScriptName := e.GetDriverScriptName()
if workflowData.EngineConfig != nil && workflowData.EngineConfig.DriverScript != "" {
driverScriptName = workflowData.EngineConfig.DriverScript
}
var execPrefix string
if driverScriptName != "" {
// Driver wraps the copilot subprocess; ${RUNNER_TEMP} and ${GH_AW_NODE_BIN} expand in the shell context.
Expand Down
25 changes: 25 additions & 0 deletions pkg/workflow/copilot_engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1729,6 +1729,31 @@ func TestCopilotEngineDriverScript(t *testing.T) {
}
})

t.Run("Execution step uses configured custom driver instead of built-in", func(t *testing.T) {
workflowData := &WorkflowData{
Name: "test-workflow",
EngineConfig: &EngineConfig{
ID: "copilot",
DriverScript: "custom_copilot_driver.cjs",
},
Tools: make(map[string]any),
}

steps := engine.GetExecutionSteps(workflowData, "/tmp/gh-aw/agent-stdio.log")
if len(steps) == 0 {
t.Fatal("Expected at least one step")
}

stepContent := strings.Join([]string(steps[0]), "\n")

if !strings.Contains(stepContent, "custom_copilot_driver.cjs") {
t.Errorf("Expected custom driver in execution step, got:\n%s", stepContent)
}
if strings.Contains(stepContent, "actions/copilot_driver.cjs") {
t.Errorf("Expected built-in driver to be replaced, got:\n%s", stepContent)
}
})

t.Run("CopilotEngine implements DriverProvider interface", func(t *testing.T) {
var _ DriverProvider = engine
})
Expand Down
8 changes: 8 additions & 0 deletions pkg/workflow/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ type EngineConfig struct {
Concurrency string // Agent job-level concurrency configuration (YAML format)
UserAgent string
Command string // Custom executable path (when set, skip installation steps)
DriverScript string // Custom Node.js driver script filename (replaces engine default driver script when supported)
Env map[string]string
Config string
Args []string
Expand Down Expand Up @@ -245,6 +246,13 @@ func (c *Compiler) ExtractEngineConfig(frontmatter map[string]any) (string, *Eng
}
}

// Extract optional 'driver' field (string - validated separately)
if driver, hasDriver := engineObj["driver"]; hasDriver {
if driverStr, ok := driver.(string); ok {
config.DriverScript = driverStr
}
}

// Extract optional 'env' field (object/map of strings)
if env, hasEnv := engineObj["env"]; hasEnv {
if envMap, ok := env.(map[string]any); ok {
Expand Down
15 changes: 15 additions & 0 deletions pkg/workflow/engine_config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,17 @@ func TestExtractEngineConfig(t *testing.T) {
expectedEngineSetting: "codex",
expectedConfig: &EngineConfig{ID: "codex", UserAgent: "my-custom-agent-hyphen"},
},
{
name: "object format - with copilot driver script",
frontmatter: map[string]any{
"engine": map[string]any{
"id": "copilot",
"driver": "custom_copilot_driver.cjs",
},
},
expectedEngineSetting: "copilot",
expectedConfig: &EngineConfig{ID: "copilot", DriverScript: "custom_copilot_driver.cjs"},
},
{
name: "object format - complete with user-agent",
frontmatter: map[string]any{
Expand Down Expand Up @@ -264,6 +275,10 @@ func TestExtractEngineConfig(t *testing.T) {
t.Errorf("Expected config.UserAgent '%s', got '%s'", test.expectedConfig.UserAgent, config.UserAgent)
}

if config.DriverScript != test.expectedConfig.DriverScript {
t.Errorf("Expected config.DriverScript '%s', got '%s'", test.expectedConfig.DriverScript, config.DriverScript)
}

if len(config.Env) != len(test.expectedConfig.Env) {
t.Errorf("Expected config.Env length %d, got %d", len(test.expectedConfig.Env), len(config.Env))
} else {
Expand Down
32 changes: 32 additions & 0 deletions pkg/workflow/engine_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,8 @@ import (
"encoding/json"
"fmt"
"os"
"path/filepath"
"regexp"
"strings"

"github.com/github/gh-aw/pkg/console"
Expand All @@ -45,6 +47,7 @@ import (
)

var engineValidationLog = newValidationLogger("engine")
var safeDriverScriptPattern = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9._-]*$`)

// validateEngineVersion warns (non-strict) or errors (strict) when the workflow
// explicitly pins the engine CLI to "latest". Unpinned "latest" versions change
Expand Down Expand Up @@ -75,6 +78,35 @@ func (c *Compiler) validateEngineVersion(workflowData *WorkflowData) error {
return nil
}

// validateEngineDriverScript validates optional engine.driver configuration.
// engine.driver must point to a Node.js script.
func (c *Compiler) validateEngineDriverScript(workflowData *WorkflowData) error {
if workflowData == nil || workflowData.EngineConfig == nil || workflowData.EngineConfig.DriverScript == "" {
return nil
}

driverScript := workflowData.EngineConfig.DriverScript
if strings.TrimSpace(driverScript) != driverScript {
return fmt.Errorf("engine.driver must be a safe basename without leading/trailing whitespace (found: %s).\n\nSee: %s", workflowData.EngineConfig.DriverScript, constants.DocsEnginesURL)
}

if filepath.IsAbs(driverScript) ||
strings.Contains(driverScript, "/") ||
strings.Contains(driverScript, `\`) ||
strings.Contains(driverScript, "..") ||
!safeDriverScriptPattern.MatchString(driverScript) {
return fmt.Errorf("engine.driver must be a safe basename (no path separators, '..', or shell metacharacters) ending with .js, .cjs, or .mjs (found: %s).\n\nSee: %s", workflowData.EngineConfig.DriverScript, constants.DocsEnginesURL)
}

ext := strings.ToLower(filepath.Ext(driverScript))
switch ext {
case ".js", ".cjs", ".mjs":
return nil
default:
return fmt.Errorf("engine.driver must be a Node.js script ending with .js, .cjs, or .mjs (found: %s).\n\nSee: %s", workflowData.EngineConfig.DriverScript, constants.DocsEnginesURL)
}
Comment on lines +81 to +107
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current validation only checks the file extension. Since engine.driver is later used to build shell commands and file paths, it should also reject unsafe values (path separators, .., absolute paths, whitespace, shell metacharacters) to prevent command breakage/injection and directory traversal. Tightening validation to a safe filename pattern (and possibly documenting that it must be a basename located under the setup actions directory) would make this feature safer and more predictable.

Copilot uses AI. Check for mistakes.
}

// validateEngineInlineDefinition validates an inline engine definition parsed from
// engine.runtime + optional engine.provider in the workflow frontmatter.
// Returns an error if:
Expand Down
Loading
Loading