diff --git a/docs/adr/48568-split-engine-validation-into-domain-focused-files.md b/docs/adr/48568-split-engine-validation-into-domain-focused-files.md new file mode 100644 index 00000000000..c72e2c0126b --- /dev/null +++ b/docs/adr/48568-split-engine-validation-into-domain-focused-files.md @@ -0,0 +1,48 @@ +# ADR-48568: Split engine_validation.go Into Domain-Focused Files + +**Date**: 2026-07-28 +**Status**: Draft +**Deciders**: Unknown (automated refactor by copilot-swe-agent) + +--- + +### Context + +`pkg/workflow/engine_validation.go` had grown to 521 lines by mixing three unrelated validation domains in a single file: engine driver and harness script filename safety checks, inline engine definition and auth strategy registration, and top-level engine settings validation (version pinning, MCP timeouts, multi-file specification consistency). The existing 300-line refactor threshold existed specifically to prevent files from mixing unrelated concerns, which makes individual domains harder to navigate, extend, and test in isolation. + +### Decision + +We will split `engine_validation.go` into three domain-focused files within the same `workflow` package: `engine_driver_validation.go` (driver/harness script path safety, ~171 lines), `engine_inline_definition_validation.go` (inline runtime/provider auth registration, ~177 lines), and a trimmed `engine_validation.go` (~255 lines, retaining version, MCP timeout, and multi-file specification validation). No logic changes, no API surface changes — this is a pure structural reorganization. Each file includes a header comment explaining its responsibility and directing contributors to the right file for future additions. + +### Alternatives Considered + +#### Alternative 1: Keep the monolithic file + +Leave `engine_validation.go` as-is and continue adding to it. This avoids merge risk and churn but the file will continue to grow — the three validation domains have independent growth trajectories (new driver runtimes, new auth strategies, new MCP settings). Rejected because the 521-line size already exceeds the repository's stated 300-line threshold, and mixing domains increases the risk that unrelated changes conflict in the same file. + +#### Alternative 2: Extract to a dedicated sub-package (`pkg/workflow/enginevalidation/`) + +Move the engine validation logic into its own sub-package. This would provide stronger encapsulation via Go's package visibility rules and prevent accidental access to internal types. Rejected for this PR because it would change import paths, may require exporting currently unexported types (breaking encapsulation in the other direction), and is a larger structural change with broader impact that warrants its own explicit decision. The within-package file split is the lower-risk, immediately actionable step. + +#### Alternative 3: Split only the largest domain (driver validation) + +Extract only `validateEngineDriver` and related helpers into a separate file, leaving inline definition and auth validation in `engine_validation.go`. This would partially address the size issue. Rejected because the inline definition and auth domain is equally unrelated to the top-level engine settings remaining in `engine_validation.go`, so a partial split would still leave the file mixing concerns and require a follow-up split anyway. + +### Consequences + +#### Positive +- Each file is under 260 lines and owns a single validation domain, reducing cognitive load when navigating engine validation logic. +- The new file headers explicitly state what to add where, creating a self-enforcing contribution convention for future engine validation work. +- Driver and inline definition validation can now be tested and reviewed independently without reading unrelated validation code. + +#### Negative +- The number of files in `pkg/workflow/` increases by two, adding some noise to directory listings. +- Future growth of any of the three new files may require further splitting; this reorganization does not eliminate the risk of future monolith accumulation. + +#### Neutral +- All function signatures are preserved and all existing tests pass unchanged — the split is transparent to callers. +- The `workflow` package boundary is preserved; no import path changes are required. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/workflow/engine_driver_validation.go b/pkg/workflow/engine_driver_validation.go new file mode 100644 index 00000000000..5fac6be0a18 --- /dev/null +++ b/pkg/workflow/engine_driver_validation.go @@ -0,0 +1,171 @@ +// This file provides engine driver and script validation for agentic workflows. +// +// # Engine Driver Validation +// +// This file validates engine.driver and engine.harness configuration fields, +// ensuring that script filenames and driver paths meet safety requirements. +// +// # Validation Functions +// +// - validateEngineScriptFilename() - Validates that a script filename is a safe Node.js basename +// - validateEngineHarnessScript() - Validates optional engine.harness configuration +// - validateEngineDriver() - Validates the shared engine.driver field +// - validateInlineEngineDriver() - Validates an inline (source-embedded) driver configuration +// +// # When to Add Validation Here +// +// Add validation to this file when: +// - It validates engine.driver or engine.harness field values +// - It checks script filename or path safety (no traversal, no metacharacters) +// - It validates file extensions for engine scripts +// - It validates inline driver source/runtime combinations +// +// For engine version and MCP timeout validation, see engine_validation.go. +// For inline engine definition and auth validation, see engine_inline_definition_validation.go. +// For general validation, see validation.go. +// For detailed documentation, see scratchpad/validation-architecture.md + +package workflow + +import ( + "fmt" + "path/filepath" + "regexp" + "strings" + + "github.com/github/gh-aw/pkg/constants" +) + +var safeHarnessScriptPattern = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9._-]*$`) + +// safeSDKDriverSegmentPattern allows path segments that may start with a dot followed by an +// alphanumeric/underscore (e.g. ".github"), but still rejects ".." traversals, leading hyphens, +// and shell metacharacters. +var safeSDKDriverSegmentPattern = regexp.MustCompile(`^(?:\.[A-Za-z0-9_]|[A-Za-z0-9_])[A-Za-z0-9._-]*$`) + +// validateEngineScriptFilename validates that a script name is a safe Node.js basename. +// The name must not contain path separators, '..', or shell metacharacters, and must +// end with one of the supported JavaScript extensions: .js, .cjs, or .mjs. +func validateEngineScriptFilename(fieldName, scriptName string) error { + if strings.TrimSpace(scriptName) != scriptName { + return fmt.Errorf("%s must be a safe basename without leading/trailing whitespace (found: %s).\n\nSee: %s", fieldName, scriptName, constants.DocsEnginesURL) + } + + if filepath.IsAbs(scriptName) || + strings.Contains(scriptName, "/") || + strings.Contains(scriptName, `\`) || + strings.Contains(scriptName, "..") || + !safeHarnessScriptPattern.MatchString(scriptName) { + return fmt.Errorf("%s must be a safe basename (no path separators, '..', or shell metacharacters) ending with .js, .cjs, or .mjs (found: %s).\n\nSee: %s", fieldName, scriptName, constants.DocsEnginesURL) + } + + ext := strings.ToLower(filepath.Ext(scriptName)) + switch ext { + case ".js", ".cjs", ".mjs": + return nil + default: + return fmt.Errorf("%s must be a Node.js script ending with .js, .cjs, or .mjs (found: %s).\n\nSee: %s", fieldName, scriptName, constants.DocsEnginesURL) + } +} + +// validateEngineHarnessScript validates optional engine.harness configuration. +// engine.harness must point to a Node.js script. +func (c *Compiler) validateEngineHarnessScript(workflowData *WorkflowData) error { + if workflowData == nil || workflowData.EngineConfig == nil || workflowData.EngineConfig.HarnessScript == "" { + return nil + } + + return validateEngineScriptFilename("engine.harness", workflowData.EngineConfig.HarnessScript) +} + +// validateEngineDriver validates the shared engine.driver field. +// engine.driver must be either: +// - a relative path (with safe segments separated by '/') ending with a supported +// extension for the engine in use, or +// - a bare command name without any extension (treated as an arbitrary executable in +// PATH for the copilot engine, or as a built-in driver for the pi engine). +// +// The allowed extensions depend on the engine: +// - copilot: .js, .cjs, .mjs (Node.js), .py (Python), .ts/.mts (TypeScript), .rb (Ruby) +// - all other engines (e.g. pi): .js, .cjs, .mjs only +// +// Absolute paths, backslashes, '..' components, and shell metacharacters are rejected. +func (c *Compiler) validateEngineDriver(workflowData *WorkflowData) error { + if workflowData == nil || workflowData.EngineConfig == nil || workflowData.EngineConfig.Driver == "" { + return nil + } + + if workflowData.EngineConfig.InlineDriver != nil { + return c.validateInlineEngineDriver(workflowData) + } + + name := workflowData.EngineConfig.Driver + isCopilotEngine := workflowData.EngineConfig.ID == "copilot" + + if strings.TrimSpace(name) != name { + return fmt.Errorf("engine.driver must be a safe path without leading/trailing whitespace (found: %s).\n\nSee: %s", name, constants.DocsEnginesURL) + } + + if filepath.IsAbs(name) || + strings.Contains(name, `\`) || + strings.Contains(name, "..") { + return fmt.Errorf("engine.driver must be a relative path (no absolute paths, '..', or backslashes) with a supported extension (found: %s).\n\nSee: %s", name, constants.DocsEnginesURL) + } + + // Each path segment must be safe (alphanumeric, underscore, dot, hyphen; may start with dot). + // Empty segments (consecutive slashes, leading/trailing slashes) are rejected. + for segment := range strings.SplitSeq(name, "/") { + if segment == "" { + return fmt.Errorf("engine.driver must not contain empty path segments (e.g. consecutive '/' or leading/trailing '/') (found: %s).\n\nSee: %s", name, constants.DocsEnginesURL) + } + if !safeSDKDriverSegmentPattern.MatchString(segment) { + return fmt.Errorf("engine.driver must not contain shell metacharacters (found unsafe segment %q in: %s).\n\nSee: %s", segment, name, constants.DocsEnginesURL) + } + } + + ext := strings.ToLower(filepath.Ext(name)) + switch ext { + case ".js", ".cjs", ".mjs": + return nil + case ".py", ".ts", ".mts", ".rb": + if isCopilotEngine { + return nil + } + return fmt.Errorf("engine.driver has unsupported extension %q for this engine (found: %s). Must be a JavaScript file ending with .js, .cjs, or .mjs, or a bare name without an extension.\n\nSee: %s", ext, name, constants.DocsEnginesURL) + case "": + // No extension — valid as a bare built-in driver name or arbitrary command in PATH. + return nil + default: + if isCopilotEngine { + return fmt.Errorf("engine.driver has unsupported extension %q (found: %s). Must be a script ending with .js, .cjs, .mjs, .py, .ts, .mts, or .rb, or a bare command name without an extension.\n\nSee: %s", ext, name, constants.DocsEnginesURL) + } + return fmt.Errorf("engine.driver has unsupported extension %q (found: %s). Must be a JavaScript file ending with .js, .cjs, or .mjs, or a bare name without an extension.\n\nSee: %s", ext, name, constants.DocsEnginesURL) + } +} + +func (c *Compiler) validateInlineEngineDriver(workflowData *WorkflowData) error { + if workflowData == nil || workflowData.EngineConfig == nil || workflowData.EngineConfig.InlineDriver == nil { + return nil + } + + inlineDriver := workflowData.EngineConfig.InlineDriver + + if inlineDriver.MultipleRuntime { + return fmt.Errorf("engine.driver: exactly one runtime key is allowed (node, python, go, java); found multiple.\n\nSee: %s", constants.DocsEnginesURL) + } + + if workflowData.EngineConfig.ID != "copilot" { + return fmt.Errorf("inline engine.driver sources are only supported for the copilot engine.\n\nSee: %s", constants.DocsEnginesURL) + } + + if strings.TrimSpace(inlineDriver.Source) == "" { + return fmt.Errorf("engine.driver.%s must not be empty.\n\nSee: %s", inlineDriver.Runtime, constants.DocsEnginesURL) + } + + switch inlineDriver.Runtime { + case "node", "python", "go", "java": + return nil + default: + return fmt.Errorf("engine.driver inline runtime %q is not supported. Use one of: node, python, go, java.\n\nSee: %s", inlineDriver.Runtime, constants.DocsEnginesURL) + } +} diff --git a/pkg/workflow/engine_driver_validation_test.go b/pkg/workflow/engine_driver_validation_test.go new file mode 100644 index 00000000000..c83829da73c --- /dev/null +++ b/pkg/workflow/engine_driver_validation_test.go @@ -0,0 +1,395 @@ +//go:build !integration + +package workflow + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestValidateEngineScriptFilename tests the validateEngineScriptFilename helper directly. +// The function validates that a script name is a safe Node.js basename (no paths, safe chars, +// correct extension). It is also exercised indirectly by TestValidateEngineHarnessScript. +func TestValidateEngineScriptFilename_ValidInputs(t *testing.T) { + tests := []struct { + name string + scriptName string + }{ + {name: "simple js", scriptName: "script.js"}, + {name: "simple cjs", scriptName: "driver.cjs"}, + {name: "simple mjs", scriptName: "index.mjs"}, + {name: "hyphenated name", scriptName: "my-driver.js"}, + {name: "underscore name", scriptName: "my_script.js"}, + {name: "dotted name", scriptName: "a.b.js"}, + {name: "uppercase", scriptName: "MyScript.js"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateEngineScriptFilename("engine.harness", tt.scriptName) + require.NoError(t, err, "expected %q to be valid", tt.scriptName) + }) + } +} + +func TestValidateEngineScriptFilename_InvalidInputs(t *testing.T) { + tests := []struct { + name string + scriptName string + errContains string + }{ + { + name: "leading whitespace", + scriptName: " script.js", + errContains: "leading/trailing whitespace", + }, + { + name: "trailing whitespace", + scriptName: "script.js ", + errContains: "leading/trailing whitespace", + }, + { + name: "absolute path", + scriptName: "/tmp/script.js", + errContains: "safe basename", + }, + { + name: "path separator slash", + scriptName: "subdir/script.js", + errContains: "safe basename", + }, + { + name: "path separator backslash", + scriptName: `sub\script.js`, + errContains: "safe basename", + }, + { + name: "directory traversal", + scriptName: "../script.js", + errContains: "safe basename", + }, + { + name: "unsupported extension py", + scriptName: "script.py", + errContains: ".js, .cjs, or .mjs", + }, + { + name: "unsupported extension ts", + scriptName: "script.ts", + errContains: ".js, .cjs, or .mjs", + }, + { + name: "no extension", + scriptName: "script", + errContains: ".js, .cjs, or .mjs", + }, + { + name: "shell metacharacter dollar", + scriptName: "$script.js", + errContains: "safe basename", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateEngineScriptFilename("engine.harness", tt.scriptName) + require.Error(t, err, "expected %q to be invalid", tt.scriptName) + assert.Contains(t, err.Error(), tt.errContains) + }) + } +} + +func TestValidateEngineHarnessScript(t *testing.T) { + tests := []struct { + name string + workflow *WorkflowData + expectError bool + errorSubstr string + }{ + { + name: "no engine config", + workflow: &WorkflowData{ + EngineConfig: nil, + }, + expectError: false, + }, + { + name: "no harness configured", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot"}, + }, + expectError: false, + }, + { + name: "valid cjs harness on copilot", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "custom_harness.cjs"}, + }, + expectError: false, + }, + { + name: "valid mjs harness on copilot", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "custom_harness.mjs"}, + }, + expectError: false, + }, + { + name: "invalid extension", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "harness.sh"}, + }, + expectError: true, + errorSubstr: "must be a Node.js script", + }, + { + name: "harness configured for any engine", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "claude", HarnessScript: "driver.cjs"}, + }, + expectError: false, + }, + { + name: "invalid path traversal", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "../driver.cjs"}, + }, + expectError: true, + errorSubstr: "safe basename", + }, + { + name: "invalid path separator", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "nested/driver.cjs"}, + }, + expectError: true, + errorSubstr: "safe basename", + }, + { + name: "invalid shell metacharacter", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "driver;rm -rf /.cjs"}, + }, + expectError: true, + errorSubstr: "safe basename", + }, + { + name: "invalid leading whitespace", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: " driver.cjs"}, + }, + expectError: true, + errorSubstr: "leading/trailing whitespace", + }, + { + name: "invalid leading hyphen", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "-driver.cjs"}, + }, + expectError: true, + errorSubstr: "safe basename", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + err := compiler.validateEngineHarnessScript(tt.workflow) + + if tt.expectError { + require.Error(t, err, "Expected validation error") + if tt.errorSubstr != "" { + require.ErrorContains(t, err, tt.errorSubstr, "Expected error substring mismatch") + } + return + } + + assert.NoError(t, err, "Expected harness validation to pass") + }) + } +} + +func TestValidateEngineCopilotSDKDriver_Copilot(t *testing.T) { + tests := []struct { + name string + workflow *WorkflowData + expectError bool + errorSubstr string + }{ + { + name: "no engine config", + workflow: &WorkflowData{ + EngineConfig: nil, + }, + expectError: false, + }, + { + name: "no copilot sdk driver configured", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot"}, + }, + expectError: false, + }, + { + name: "valid cjs copilot sdk driver", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "custom_copilot_sdk_driver.cjs"}, + }, + expectError: false, + }, + { + name: "valid mjs copilot sdk driver", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "custom_copilot_sdk_driver.mjs"}, + }, + expectError: false, + }, + { + name: "invalid extension", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "driver.sh"}, + }, + expectError: true, + errorSubstr: "unsupported extension", + }, + { + name: "invalid path traversal", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "../driver.cjs"}, + }, + expectError: true, + errorSubstr: "relative path", + }, + { + name: "invalid path separator leading slash", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "/abs/driver.cjs"}, + }, + expectError: true, + errorSubstr: "relative path", + }, + { + name: "valid relative path driver", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "nested/driver.cjs"}, + }, + expectError: false, + }, + { + name: "valid github drivers path driver", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: ".github/drivers/my_driver.py"}, + }, + expectError: false, + }, + { + name: "invalid shell metacharacter", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "driver;rm -rf /.cjs"}, + }, + expectError: true, + errorSubstr: "metacharacter", + }, + { + name: "invalid leading whitespace", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: " driver.cjs"}, + }, + expectError: true, + errorSubstr: "leading/trailing whitespace", + }, + { + name: "invalid leading hyphen", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "-driver.cjs"}, + }, + expectError: true, + errorSubstr: "metacharacter", + }, + { + name: "valid python driver", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "my_driver.py"}, + }, + expectError: false, + }, + { + name: "valid typescript driver", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "my_driver.ts"}, + }, + expectError: false, + }, + { + name: "valid mts typescript driver", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "my_driver.mts"}, + }, + expectError: false, + }, + { + name: "valid ruby driver", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "my_driver.rb"}, + }, + expectError: false, + }, + { + name: "valid arbitrary command no extension", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "my-copilot-driver"}, + }, + expectError: false, + }, + { + name: "valid arbitrary command underscore no extension", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "my_driver"}, + }, + expectError: false, + }, + { + name: "invalid java extension", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: "driver.java"}, + }, + expectError: true, + errorSubstr: "unsupported extension", + }, + { + name: "invalid consecutive slashes", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: ".github//drivers/driver.py"}, + }, + expectError: true, + errorSubstr: "empty path segments", + }, + { + name: "invalid trailing slash", + workflow: &WorkflowData{ + EngineConfig: &EngineConfig{ID: "copilot", Driver: ".github/drivers/"}, + }, + expectError: true, + errorSubstr: "empty path segments", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + compiler := NewCompiler() + err := compiler.validateEngineDriver(tt.workflow) + + if tt.expectError { + require.Error(t, err, "Expected validation error") + if tt.errorSubstr != "" { + require.ErrorContains(t, err, tt.errorSubstr, "Expected error substring mismatch") + } + return + } + + assert.NoError(t, err, "Expected driver validation to pass") + }) + } +} diff --git a/pkg/workflow/engine_inline_definition_validation.go b/pkg/workflow/engine_inline_definition_validation.go new file mode 100644 index 00000000000..d8429733a2a --- /dev/null +++ b/pkg/workflow/engine_inline_definition_validation.go @@ -0,0 +1,177 @@ +// This file provides inline engine definition and auth validation for agentic workflows. +// +// # Inline Engine Definition Validation +// +// This file validates inline engine definitions declared via engine.runtime and +// engine.provider in the workflow frontmatter. It also handles registration of +// validated definitions into the session engine catalog. +// +// # Validation Functions +// +// - validateEngineInlineDefinition() - Validates inline engine runtime.id against the registry +// - registerInlineEngineDefinition() - Registers a validated inline engine definition +// - validateEngineAuthDefinition() - Validates auth strategy fields for inline provider auth +// +// # When to Add Validation Here +// +// Add validation to this file when: +// - It validates engine.runtime or engine.provider fields +// - It validates inline engine auth strategy configurations +// - It registers or modifies the engine catalog for inline definitions +// - It checks provider, auth, or request shape fields +// +// For engine driver and script validation, see engine_driver_validation.go. +// For engine version and MCP timeout validation, see engine_validation.go. +// For general validation, see validation.go. +// For detailed documentation, see scratchpad/validation-architecture.md + +package workflow + +import ( + "errors" + "fmt" + "strings" + + "github.com/github/gh-aw/pkg/constants" + "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/parser" +) + +var engineInlineDefinitionLog = logger.New("workflow:engine_inline_definition_validation") + +// validateEngineInlineDefinition validates an inline engine definition parsed from +// engine.runtime + optional engine.provider in the workflow frontmatter. +// Returns an error if: +// - The required runtime.id field is missing +// - The runtime.id does not match a known runtime adapter +func (c *Compiler) validateEngineInlineDefinition(config *EngineConfig) error { + if !config.IsInlineDefinition { + return nil + } + + engineInlineDefinitionLog.Printf("Validating inline engine definition: runtimeID=%s", config.ID) + + if config.ID == "" { + return fmt.Errorf("inline engine definition is missing required 'runtime.id' field.\n\nExample:\nengine:\n runtime:\n id: codex\n\nSee: %s", constants.DocsEnginesURL) + } + + // Validate that runtime.id maps to a known runtime adapter. + if !c.engineRegistry.IsValidEngine(config.ID) { + // Try prefix match for backward compatibility (e.g. "codex-experimental") + if matched, err := c.engineRegistry.GetEngineByPrefix(config.ID); err == nil { + engineInlineDefinitionLog.Printf("Inline engine runtime.id %q matched via prefix to runtime %q", config.ID, matched.GetID()) + } else { + validEngines := c.engineRegistry.GetSupportedEngines() + suggestions := parser.FindClosestMatches(config.ID, validEngines, 1) + enginesStr := strings.Join(validEngines, ", ") + + errMsg := fmt.Sprintf("inline engine definition references unknown runtime.id: %s. Known runtime IDs are: %s.\n\nExample:\nengine:\n runtime:\n id: codex\n\nSee: %s", + config.ID, enginesStr, constants.DocsEnginesURL) + if len(suggestions) > 0 { + errMsg = fmt.Sprintf("inline engine definition references unknown runtime.id: %s. Known runtime IDs are: %s.\n\nDid you mean: %s?\n\nExample:\nengine:\n runtime:\n id: codex\n\nSee: %s", + config.ID, enginesStr, suggestions[0], constants.DocsEnginesURL) + } + return errors.New(errMsg) + } + } + + return nil +} + +// registerInlineEngineDefinition registers an inline engine definition in the session +// catalog. If the runtime ID already exists in the catalog (e.g. a built-in), the +// existing display name and description are preserved while provider overrides are applied. +func (c *Compiler) registerInlineEngineDefinition(config *EngineConfig) { + def := &EngineDefinition{ + ID: config.ID, + RuntimeID: config.ID, + DisplayName: config.ID, + Description: "Inline engine definition from workflow frontmatter", + } + + // Preserve display name and description from existing built-in entry if available. + if existing := c.engineCatalog.Get(config.ID); existing != nil { + def.DisplayName = existing.DisplayName + def.Description = existing.Description + def.Models = existing.Models + // Copy existing provider/auth as defaults; inline values below fully replace them + // when present (replacement, not merge). + def.Provider = existing.Provider + } + + // Apply inline provider overrides. + if config.InlineProviderID != "" { + def.Provider = ProviderSelection{Name: config.InlineProviderID} + } + + // Prefer the full AuthDefinition over the legacy simple-secret path. + if config.InlineProviderAuth != nil { + // Normalise strategy: treat empty strategy as api-key when a secret is set. + auth := config.InlineProviderAuth + if auth.Strategy == "" && auth.Secret != "" { + auth.Strategy = AuthStrategyAPIKey + } + def.Provider.Auth = auth + } + + if config.InlineProviderRequest != nil { + def.Provider.Request = config.InlineProviderRequest + } + + engineInlineDefinitionLog.Printf("Registering inline engine definition in session catalog: id=%s, runtimeID=%s, providerID=%s", + def.ID, def.RuntimeID, def.Provider.Name) + c.engineCatalog.Register(def) +} + +// validateEngineAuthDefinition validates AuthDefinition fields for an inline engine definition. +// Returns an error describing the first (or all, in non-fail-fast mode) validation problems found. +func (c *Compiler) validateEngineAuthDefinition(config *EngineConfig) error { + auth := config.InlineProviderAuth + if auth == nil { + return nil + } + + engineInlineDefinitionLog.Printf("Validating engine auth definition: strategy=%s", auth.Strategy) + + switch auth.Strategy { + case AuthStrategyOAuthClientCreds: + // oauth-client-credentials requires tokenUrl, clientId, clientSecret. + if auth.TokenURL == "" { + return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.token-url' to be set.\n\nExample:\nengine:\n runtime:\n id: codex\n provider:\n auth:\n strategy: oauth-client-credentials\n token-url: https://auth.example.com/oauth/token\n client-id: MY_CLIENT_ID_SECRET\n client-secret: MY_CLIENT_SECRET_SECRET\n\nSee: %s", constants.DocsEnginesURL) + } + if auth.ClientIDRef == "" { + return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.client-id' to be set.\n\nSee: %s", constants.DocsEnginesURL) + } + if auth.ClientSecretRef == "" { + return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.client-secret' to be set.\n\nSee: %s", constants.DocsEnginesURL) + } + // For oauth, header-name is required (the token must go somewhere). + if auth.HeaderName == "" { + return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.header-name' to be set (e.g. 'api-key' or 'Authorization').\n\nSee: %s", constants.DocsEnginesURL) + } + case AuthStrategyAPIKey: + // api-key requires a secret value and a header-name so the caller knows where to inject the key. + if auth.Secret == "" { + return fmt.Errorf("engine auth: strategy 'api-key' requires 'auth.secret' to be set.\n\nSee: %s", constants.DocsEnginesURL) + } + if auth.HeaderName == "" { + return fmt.Errorf("engine auth: strategy 'api-key' requires 'auth.header-name' to be set (e.g. 'api-key' or 'x-api-key').\n\nSee: %s", constants.DocsEnginesURL) + } + case AuthStrategyBearer, "": + // bearer strategy and unset strategy (simple backwards-compat secret) require a secret value. + if auth.Secret == "" { + return fmt.Errorf("engine auth: strategy 'bearer' (or unset) requires 'auth.secret' to be set.\n\nSee: %s", constants.DocsEnginesURL) + } + default: + validStrategies := []string{ + string(AuthStrategyAPIKey), + string(AuthStrategyOAuthClientCreds), + string(AuthStrategyBearer), + } + return fmt.Errorf("engine auth: unknown strategy %q. Valid strategies are: %s.\n\nSee: %s", + auth.Strategy, strings.Join(validStrategies, ", "), constants.DocsEnginesURL) + } + + engineInlineDefinitionLog.Printf("Engine auth definition is valid: strategy=%s", auth.Strategy) + return nil +} diff --git a/pkg/workflow/engine_validation.go b/pkg/workflow/engine_validation.go index bf3b0af3e1e..ad843a3f503 100644 --- a/pkg/workflow/engine_validation.go +++ b/pkg/workflow/engine_validation.go @@ -2,14 +2,17 @@ // // # Engine Validation // -// This file validates engine configurations used in agentic workflows. -// Validation ensures that engine IDs are supported and that only one engine -// specification exists across the main workflow and all included files. +// This file validates top-level engine configuration fields used in agentic workflows, +// including the engine version, MCP timeout settings, and multi-file engine specification +// consistency. // // # Validation Functions // -// - validateEngine() - Validates that a given engine ID is supported +// - validateEngineVersion() - Warns when engine.version is set to "latest" +// - validateEngineMCPSessionTimeout() - Validates engine.mcp.session-timeout duration +// - validateEngineMCPToolTimeout() - Validates engine.mcp.tool-timeout duration // - validateSingleEngineSpecification() - Validates that only one engine field exists across all files +// - EngineHasValidateSecretStep() - Reports whether an engine provides a validate-secret step // // # Validation Pattern: Engine Registry // @@ -22,11 +25,13 @@ // # When to Add Validation Here // // Add validation to this file when: -// - It validates engine IDs or engine configurations -// - It checks engine registry entries -// - It validates engine-specific settings -// - It validates engine field consistency across imports +// - It validates engine version or CLI pinning settings +// - It validates engine.mcp timeout values +// - It checks engine specification consistency across main and included files +// - It validates engine field presence or absence at the top level // +// For engine driver and script validation, see engine_driver_validation.go. +// For inline engine definition and auth validation, see engine_inline_definition_validation.go. // For engine configuration extraction, see engine.go. // For general validation, see validation.go. // For detailed documentation, see scratchpad/validation-architecture.md @@ -35,49 +40,17 @@ package workflow import ( "encoding/json" - "errors" "fmt" "os" - "path/filepath" - "regexp" "strings" "time" "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/constants" "github.com/github/gh-aw/pkg/logger" - "github.com/github/gh-aw/pkg/parser" ) var engineValidationLog = logger.New("workflow:engine_validation") -var safeHarnessScriptPattern = regexp.MustCompile(`^[A-Za-z0-9_][A-Za-z0-9._-]*$`) - -// safeSDKDriverSegmentPattern allows path segments that may start with a dot followed by an -// alphanumeric/underscore (e.g. ".github"), but still rejects ".." traversals, leading hyphens, -// and shell metacharacters. -var safeSDKDriverSegmentPattern = regexp.MustCompile(`^(?:\.[A-Za-z0-9_]|[A-Za-z0-9_])[A-Za-z0-9._-]*$`) - -func validateEngineScriptFilename(fieldName, scriptName string) error { - if strings.TrimSpace(scriptName) != scriptName { - return fmt.Errorf("%s must be a safe basename without leading/trailing whitespace (found: %s).\n\nSee: %s", fieldName, scriptName, constants.DocsEnginesURL) - } - - if filepath.IsAbs(scriptName) || - strings.Contains(scriptName, "/") || - strings.Contains(scriptName, `\`) || - strings.Contains(scriptName, "..") || - !safeHarnessScriptPattern.MatchString(scriptName) { - return fmt.Errorf("%s must be a safe basename (no path separators, '..', or shell metacharacters) ending with .js, .cjs, or .mjs (found: %s).\n\nSee: %s", fieldName, scriptName, constants.DocsEnginesURL) - } - - ext := strings.ToLower(filepath.Ext(scriptName)) - switch ext { - case ".js", ".cjs", ".mjs": - return nil - default: - return fmt.Errorf("%s must be a Node.js script ending with .js, .cjs, or .mjs (found: %s).\n\nSee: %s", fieldName, scriptName, constants.DocsEnginesURL) - } -} // validateEngineVersion warns when the workflow explicitly pins the engine CLI // to "latest". Unpinned "latest" versions change unpredictably and undermine @@ -104,108 +77,6 @@ func (c *Compiler) validateEngineVersion(workflowData *WorkflowData) error { return nil } -// validateEngineHarnessScript validates optional engine.harness configuration. -// engine.harness must point to a Node.js script. -func (c *Compiler) validateEngineHarnessScript(workflowData *WorkflowData) error { - if workflowData == nil || workflowData.EngineConfig == nil || workflowData.EngineConfig.HarnessScript == "" { - return nil - } - - return validateEngineScriptFilename("engine.harness", workflowData.EngineConfig.HarnessScript) -} - -// validateEngineDriver validates the shared engine.driver field. -// engine.driver must be either: -// - a relative path (with safe segments separated by '/') ending with a supported -// extension for the engine in use, or -// - a bare command name without any extension (treated as an arbitrary executable in -// PATH for the copilot engine, or as a built-in driver for the pi engine). -// -// The allowed extensions depend on the engine: -// - copilot: .js, .cjs, .mjs (Node.js), .py (Python), .ts/.mts (TypeScript), .rb (Ruby) -// - all other engines (e.g. pi): .js, .cjs, .mjs only -// -// Absolute paths, backslashes, '..' components, and shell metacharacters are rejected. -func (c *Compiler) validateEngineDriver(workflowData *WorkflowData) error { - if workflowData == nil || workflowData.EngineConfig == nil || workflowData.EngineConfig.Driver == "" { - return nil - } - - if workflowData.EngineConfig.InlineDriver != nil { - return c.validateInlineEngineDriver(workflowData) - } - - name := workflowData.EngineConfig.Driver - isCopilotEngine := workflowData.EngineConfig.ID == "copilot" - - if strings.TrimSpace(name) != name { - return fmt.Errorf("engine.driver must be a safe path without leading/trailing whitespace (found: %s).\n\nSee: %s", name, constants.DocsEnginesURL) - } - - if filepath.IsAbs(name) || - strings.Contains(name, `\`) || - strings.Contains(name, "..") { - return fmt.Errorf("engine.driver must be a relative path (no absolute paths, '..', or backslashes) with a supported extension (found: %s).\n\nSee: %s", name, constants.DocsEnginesURL) - } - - // Each path segment must be safe (alphanumeric, underscore, dot, hyphen; may start with dot). - // Empty segments (consecutive slashes, leading/trailing slashes) are rejected. - for segment := range strings.SplitSeq(name, "/") { - if segment == "" { - return fmt.Errorf("engine.driver must not contain empty path segments (e.g. consecutive '/' or leading/trailing '/') (found: %s).\n\nSee: %s", name, constants.DocsEnginesURL) - } - if !safeSDKDriverSegmentPattern.MatchString(segment) { - return fmt.Errorf("engine.driver must not contain shell metacharacters (found unsafe segment %q in: %s).\n\nSee: %s", segment, name, constants.DocsEnginesURL) - } - } - - ext := strings.ToLower(filepath.Ext(name)) - switch ext { - case ".js", ".cjs", ".mjs": - return nil - case ".py", ".ts", ".mts", ".rb": - if isCopilotEngine { - return nil - } - return fmt.Errorf("engine.driver has unsupported extension %q for this engine (found: %s). Must be a JavaScript file ending with .js, .cjs, or .mjs, or a bare name without an extension.\n\nSee: %s", ext, name, constants.DocsEnginesURL) - case "": - // No extension — valid as a bare built-in driver name or arbitrary command in PATH. - return nil - default: - if isCopilotEngine { - return fmt.Errorf("engine.driver has unsupported extension %q (found: %s). Must be a script ending with .js, .cjs, .mjs, .py, .ts, .mts, or .rb, or a bare command name without an extension.\n\nSee: %s", ext, name, constants.DocsEnginesURL) - } - return fmt.Errorf("engine.driver has unsupported extension %q (found: %s). Must be a JavaScript file ending with .js, .cjs, or .mjs, or a bare name without an extension.\n\nSee: %s", ext, name, constants.DocsEnginesURL) - } -} - -func (c *Compiler) validateInlineEngineDriver(workflowData *WorkflowData) error { - if workflowData == nil || workflowData.EngineConfig == nil || workflowData.EngineConfig.InlineDriver == nil { - return nil - } - - inlineDriver := workflowData.EngineConfig.InlineDriver - - if inlineDriver.MultipleRuntime { - return fmt.Errorf("engine.driver: exactly one runtime key is allowed (node, python, go, java); found multiple.\n\nSee: %s", constants.DocsEnginesURL) - } - - if workflowData.EngineConfig.ID != "copilot" { - return fmt.Errorf("inline engine.driver sources are only supported for the copilot engine.\n\nSee: %s", constants.DocsEnginesURL) - } - - if strings.TrimSpace(inlineDriver.Source) == "" { - return fmt.Errorf("engine.driver.%s must not be empty.\n\nSee: %s", inlineDriver.Runtime, constants.DocsEnginesURL) - } - - switch inlineDriver.Runtime { - case "node", "python", "go", "java": - return nil - default: - return fmt.Errorf("engine.driver inline runtime %q is not supported. Use one of: node, python, go, java.\n\nSee: %s", inlineDriver.Runtime, constants.DocsEnginesURL) - } -} - // validateEngineMCPSessionTimeout validates optional engine.mcp.session-timeout configuration. // The value must be a valid Go duration string of at least 5m (no upper bound). func (c *Compiler) validateEngineMCPSessionTimeout(workflowData *WorkflowData) error { @@ -254,143 +125,6 @@ func (c *Compiler) validateEngineMCPToolTimeout(workflowData *WorkflowData) erro return nil } -// validateEngineInlineDefinition validates an inline engine definition parsed from -// engine.runtime + optional engine.provider in the workflow frontmatter. -// Returns an error if: -// - The required runtime.id field is missing -// - The runtime.id does not match a known runtime adapter -func (c *Compiler) validateEngineInlineDefinition(config *EngineConfig) error { - if !config.IsInlineDefinition { - return nil - } - - engineValidationLog.Printf("Validating inline engine definition: runtimeID=%s", config.ID) - - if config.ID == "" { - return fmt.Errorf("inline engine definition is missing required 'runtime.id' field.\n\nExample:\nengine:\n runtime:\n id: codex\n\nSee: %s", constants.DocsEnginesURL) - } - - // Validate that runtime.id maps to a known runtime adapter. - if !c.engineRegistry.IsValidEngine(config.ID) { - // Try prefix match for backward compatibility (e.g. "codex-experimental") - if matched, err := c.engineRegistry.GetEngineByPrefix(config.ID); err == nil { - engineValidationLog.Printf("Inline engine runtime.id %q matched via prefix to runtime %q", config.ID, matched.GetID()) - } else { - validEngines := c.engineRegistry.GetSupportedEngines() - suggestions := parser.FindClosestMatches(config.ID, validEngines, 1) - enginesStr := strings.Join(validEngines, ", ") - - errMsg := fmt.Sprintf("inline engine definition references unknown runtime.id: %s. Known runtime IDs are: %s.\n\nExample:\nengine:\n runtime:\n id: codex\n\nSee: %s", - config.ID, enginesStr, constants.DocsEnginesURL) - if len(suggestions) > 0 { - errMsg = fmt.Sprintf("inline engine definition references unknown runtime.id: %s. Known runtime IDs are: %s.\n\nDid you mean: %s?\n\nExample:\nengine:\n runtime:\n id: codex\n\nSee: %s", - config.ID, enginesStr, suggestions[0], constants.DocsEnginesURL) - } - return errors.New(errMsg) - } - } - - return nil -} - -// registerInlineEngineDefinition registers an inline engine definition in the session -// catalog. If the runtime ID already exists in the catalog (e.g. a built-in), the -// existing display name and description are preserved while provider overrides are applied. -func (c *Compiler) registerInlineEngineDefinition(config *EngineConfig) { - def := &EngineDefinition{ - ID: config.ID, - RuntimeID: config.ID, - DisplayName: config.ID, - Description: "Inline engine definition from workflow frontmatter", - } - - // Preserve display name and description from existing built-in entry if available. - if existing := c.engineCatalog.Get(config.ID); existing != nil { - def.DisplayName = existing.DisplayName - def.Description = existing.Description - def.Models = existing.Models - // Copy existing provider/auth as defaults; inline values below fully replace them - // when present (replacement, not merge). - def.Provider = existing.Provider - } - - // Apply inline provider overrides. - if config.InlineProviderID != "" { - def.Provider = ProviderSelection{Name: config.InlineProviderID} - } - - // Prefer the full AuthDefinition over the legacy simple-secret path. - if config.InlineProviderAuth != nil { - // Normalise strategy: treat empty strategy as api-key when a secret is set. - auth := config.InlineProviderAuth - if auth.Strategy == "" && auth.Secret != "" { - auth.Strategy = AuthStrategyAPIKey - } - def.Provider.Auth = auth - } - - if config.InlineProviderRequest != nil { - def.Provider.Request = config.InlineProviderRequest - } - - engineValidationLog.Printf("Registering inline engine definition in session catalog: id=%s, runtimeID=%s, providerID=%s", - def.ID, def.RuntimeID, def.Provider.Name) - c.engineCatalog.Register(def) -} - -// validateEngineAuthDefinition validates AuthDefinition fields for an inline engine definition. -// Returns an error describing the first (or all, in non-fail-fast mode) validation problems found. -func (c *Compiler) validateEngineAuthDefinition(config *EngineConfig) error { - auth := config.InlineProviderAuth - if auth == nil { - return nil - } - - engineValidationLog.Printf("Validating engine auth definition: strategy=%s", auth.Strategy) - - switch auth.Strategy { - case AuthStrategyOAuthClientCreds: - // oauth-client-credentials requires tokenUrl, clientId, clientSecret. - if auth.TokenURL == "" { - return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.token-url' to be set.\n\nExample:\nengine:\n runtime:\n id: codex\n provider:\n auth:\n strategy: oauth-client-credentials\n token-url: https://auth.example.com/oauth/token\n client-id: MY_CLIENT_ID_SECRET\n client-secret: MY_CLIENT_SECRET_SECRET\n\nSee: %s", constants.DocsEnginesURL) - } - if auth.ClientIDRef == "" { - return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.client-id' to be set.\n\nSee: %s", constants.DocsEnginesURL) - } - if auth.ClientSecretRef == "" { - return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.client-secret' to be set.\n\nSee: %s", constants.DocsEnginesURL) - } - // For oauth, header-name is required (the token must go somewhere). - if auth.HeaderName == "" { - return fmt.Errorf("engine auth: strategy 'oauth-client-credentials' requires 'auth.header-name' to be set (e.g. 'api-key' or 'Authorization').\n\nSee: %s", constants.DocsEnginesURL) - } - case AuthStrategyAPIKey: - // api-key requires a secret value and a header-name so the caller knows where to inject the key. - if auth.Secret == "" { - return fmt.Errorf("engine auth: strategy 'api-key' requires 'auth.secret' to be set.\n\nSee: %s", constants.DocsEnginesURL) - } - if auth.HeaderName == "" { - return fmt.Errorf("engine auth: strategy 'api-key' requires 'auth.header-name' to be set (e.g. 'api-key' or 'x-api-key').\n\nSee: %s", constants.DocsEnginesURL) - } - case AuthStrategyBearer, "": - // bearer strategy and unset strategy (simple backwards-compat secret) require a secret value. - if auth.Secret == "" { - return fmt.Errorf("engine auth: strategy 'bearer' (or unset) requires 'auth.secret' to be set.\n\nSee: %s", constants.DocsEnginesURL) - } - default: - validStrategies := []string{ - string(AuthStrategyAPIKey), - string(AuthStrategyOAuthClientCreds), - string(AuthStrategyBearer), - } - return fmt.Errorf("engine auth: unknown strategy %q. Valid strategies are: %s.\n\nSee: %s", - auth.Strategy, strings.Join(validStrategies, ", "), constants.DocsEnginesURL) - } - - engineValidationLog.Printf("Engine auth definition is valid: strategy=%s", auth.Strategy) - return nil -} - // isModelOnlyEngineJSON reports whether engineJSON represents an engine object that // contains only preference settings (no 'id' or 'runtime' field). Such configs express // a preference (e.g., model size or MCP timeouts) without selecting a specific engine, diff --git a/pkg/workflow/engine_validation_test.go b/pkg/workflow/engine_validation_test.go index 5ec4f7a9f8c..916c05f9c21 100644 --- a/pkg/workflow/engine_validation_test.go +++ b/pkg/workflow/engine_validation_test.go @@ -338,299 +338,6 @@ func TestValidateEngineVersion(t *testing.T) { } } -func TestValidateEngineHarnessScript(t *testing.T) { - tests := []struct { - name string - workflow *WorkflowData - expectError bool - errorSubstr string - }{ - { - name: "no engine config", - workflow: &WorkflowData{ - EngineConfig: nil, - }, - expectError: false, - }, - { - name: "no harness configured", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot"}, - }, - expectError: false, - }, - { - name: "valid cjs harness on copilot", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "custom_harness.cjs"}, - }, - expectError: false, - }, - { - name: "valid mjs harness on copilot", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "custom_harness.mjs"}, - }, - expectError: false, - }, - { - name: "invalid extension", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "harness.sh"}, - }, - expectError: true, - errorSubstr: "must be a Node.js script", - }, - { - name: "harness configured for any engine", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "claude", HarnessScript: "driver.cjs"}, - }, - expectError: false, - }, - { - name: "invalid path traversal", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "../driver.cjs"}, - }, - expectError: true, - errorSubstr: "safe basename", - }, - { - name: "invalid path separator", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "nested/driver.cjs"}, - }, - expectError: true, - errorSubstr: "safe basename", - }, - { - name: "invalid shell metacharacter", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "driver;rm -rf /.cjs"}, - }, - expectError: true, - errorSubstr: "safe basename", - }, - { - name: "invalid leading whitespace", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: " driver.cjs"}, - }, - expectError: true, - errorSubstr: "leading/trailing whitespace", - }, - { - name: "invalid leading hyphen", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", HarnessScript: "-driver.cjs"}, - }, - expectError: true, - errorSubstr: "safe basename", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - compiler := NewCompiler() - err := compiler.validateEngineHarnessScript(tt.workflow) - - if tt.expectError { - require.Error(t, err, "Expected validation error") - if tt.errorSubstr != "" { - require.ErrorContains(t, err, tt.errorSubstr, "Expected error substring mismatch") - } - return - } - - assert.NoError(t, err, "Expected harness validation to pass") - }) - } -} - -func TestValidateEngineCopilotSDKDriver_Copilot(t *testing.T) { - tests := []struct { - name string - workflow *WorkflowData - expectError bool - errorSubstr string - }{ - { - name: "no engine config", - workflow: &WorkflowData{ - EngineConfig: nil, - }, - expectError: false, - }, - { - name: "no copilot sdk driver configured", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot"}, - }, - expectError: false, - }, - { - name: "valid cjs copilot sdk driver", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "custom_copilot_sdk_driver.cjs"}, - }, - expectError: false, - }, - { - name: "valid mjs copilot sdk driver", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "custom_copilot_sdk_driver.mjs"}, - }, - expectError: false, - }, - { - name: "invalid extension", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "driver.sh"}, - }, - expectError: true, - errorSubstr: "unsupported extension", - }, - { - name: "invalid path traversal", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "../driver.cjs"}, - }, - expectError: true, - errorSubstr: "relative path", - }, - { - name: "invalid path separator leading slash", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "/abs/driver.cjs"}, - }, - expectError: true, - errorSubstr: "relative path", - }, - { - name: "valid relative path driver", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "nested/driver.cjs"}, - }, - expectError: false, - }, - { - name: "valid github drivers path driver", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: ".github/drivers/my_driver.py"}, - }, - expectError: false, - }, - { - name: "invalid shell metacharacter", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "driver;rm -rf /.cjs"}, - }, - expectError: true, - errorSubstr: "metacharacter", - }, - { - name: "invalid leading whitespace", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: " driver.cjs"}, - }, - expectError: true, - errorSubstr: "leading/trailing whitespace", - }, - { - name: "invalid leading hyphen", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "-driver.cjs"}, - }, - expectError: true, - errorSubstr: "metacharacter", - }, - { - name: "valid python driver", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "my_driver.py"}, - }, - expectError: false, - }, - { - name: "valid typescript driver", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "my_driver.ts"}, - }, - expectError: false, - }, - { - name: "valid mts typescript driver", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "my_driver.mts"}, - }, - expectError: false, - }, - { - name: "valid ruby driver", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "my_driver.rb"}, - }, - expectError: false, - }, - { - name: "valid arbitrary command no extension", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "my-copilot-driver"}, - }, - expectError: false, - }, - { - name: "valid arbitrary command underscore no extension", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "my_driver"}, - }, - expectError: false, - }, - { - name: "invalid java extension", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: "driver.java"}, - }, - expectError: true, - errorSubstr: "unsupported extension", - }, - { - name: "invalid consecutive slashes", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: ".github//drivers/driver.py"}, - }, - expectError: true, - errorSubstr: "empty path segments", - }, - { - name: "invalid trailing slash", - workflow: &WorkflowData{ - EngineConfig: &EngineConfig{ID: "copilot", Driver: ".github/drivers/"}, - }, - expectError: true, - errorSubstr: "empty path segments", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - compiler := NewCompiler() - err := compiler.validateEngineDriver(tt.workflow) - - if tt.expectError { - require.Error(t, err, "Expected validation error") - if tt.errorSubstr != "" { - require.ErrorContains(t, err, tt.errorSubstr, "Expected error substring mismatch") - } - return - } - - assert.NoError(t, err, "Expected driver validation to pass") - }) - } -} - // TestValidateEngineMCPSessionTimeout tests the validateEngineMCPSessionTimeout function. func TestValidateEngineMCPSessionTimeout(t *testing.T) { tests := []struct { diff --git a/scratchpad/validation-architecture.md b/scratchpad/validation-architecture.md index 0c8a0b48648..54a087f66ac 100644 --- a/scratchpad/validation-architecture.md +++ b/scratchpad/validation-architecture.md @@ -117,15 +117,34 @@ Domain-specific validation is organized into separate files based on functional - ✅ Expression security validation - ✅ Injection prevention -#### 5. **Engine Validation**: `engine.go` +#### 5. **Engine Validation**: `engine_validation.go`, `engine_driver_validation.go`, `engine_inline_definition_validation.go` -**Location**: `pkg/workflow/engine.go` (383 lines) +**Locations**: +- `pkg/workflow/engine_validation.go` (~255 lines) — top-level engine settings, MCP timeouts, specification consistency +- `pkg/workflow/engine_driver_validation.go` (~171 lines) — engine.driver and engine.harness file path safety +- `pkg/workflow/engine_inline_definition_validation.go` (~175 lines) — inline runtime/provider definitions and auth **Purpose**: Validates AI engine configuration **Validation Functions**: -- `validateEngine()` - Validates engine ID is supported -- `validateSingleEngineSpecification()` - Ensures single engine per workflow + +`engine_validation.go`: +- `validateEngineVersion()` - Warns when engine.version is "latest" +- `validateEngineMCPSessionTimeout()` - Validates engine.mcp.session-timeout duration +- `validateEngineMCPToolTimeout()` - Validates engine.mcp.tool-timeout duration +- `validateSingleEngineSpecification()` - Ensures single engine per workflow across all files +- `EngineHasValidateSecretStep()` - Reports whether an engine provides a validate-secret step + +`engine_driver_validation.go`: +- `validateEngineScriptFilename()` - Validates that a script name is a safe Node.js basename +- `validateEngineHarnessScript()` - Validates optional engine.harness configuration +- `validateEngineDriver()` - Validates the shared engine.driver field +- `validateInlineEngineDriver()` - Validates an inline (source-embedded) driver configuration + +`engine_inline_definition_validation.go`: +- `validateEngineInlineDefinition()` - Validates inline engine runtime.id against the registry +- `registerInlineEngineDefinition()` - Registers a validated inline engine definition +- `validateEngineAuthDefinition()` - Validates auth strategy fields for inline provider auth **Pattern**: Configuration validation with backward compatibility @@ -133,6 +152,8 @@ Domain-specific validation is organized into separate files based on functional - ✅ Engine configuration parsing - ✅ Engine compatibility checks - ✅ Engine-specific feature validation +- ✅ Engine driver or script file path validation (→ engine_driver_validation.go) +- ✅ Inline engine runtime/provider auth validation (→ engine_inline_definition_validation.go) #### 6. **MCP Configuration**: `mcp-config.go`