Split oversized engine_validation.go (521 lines) into domain-focused files

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.*

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)
+	}
+}