perf: fix 17.1% regression in BenchmarkParseWorkflow

pkg/workflow/compiler_orchestrator_engine.go

@@ -126,12 +126,42 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean
 	// import. This makes "engine: copilot" syntactic sugar for importing the builtin
 	// copilot.md, which carries the full engine definition. The engine field is removed
 	// from the frontmatter so the definition comes entirely from the import.
+	//
+	// Performance optimisation: if the workflow has no user-specified imports, the
+	// builtin engine .md would be the only import. All builtin engine .md files
+	// contain only an engine definition (no tools, steps, runtimes, or network
+	// config), so importing them produces nothing that is not already available in
+	// the engine catalog (populated at startup by loadBuiltinEngineDefinitions).
+	// In this common case we skip the import injection entirely — avoiding a full
+	// ProcessImportsFromFrontmatterWithSource round-trip — and record the engine ID
+	// in builtinEngineID so the fast path below can restore it without JSON re-parsing.
+	//
+	// When the workflow DOES have user-specified imports we still inject the builtin
+	// so that the engine definition is part of the import chain (for correctness,
+	// e.g. duplicate-engine detection across all imported files).
+	//
+	// builtinSkippedInjection is true when the optimisation fired and the builtin
+	// was NOT added to the imports list. It is used later to represent the engine
+	// in duplicate-engine conflict detection even though it is absent from allEngines.
+	var builtinEngineID string
+	var builtinSkippedInjection bool
 	if c.engineOverride == "" && isStringFormEngine(result.Frontmatter) && engineSetting != "" {
 		builtinPath := builtinEnginePath(engineSetting)
 		if parser.BuiltinVirtualFileExists(builtinPath) {
-			orchestratorEngineLog.Printf("Injecting builtin engine import: %s", builtinPath)
-			addImportToFrontmatter(result.Frontmatter, builtinPath)
+			if frontmatterHasUserImports(result.Frontmatter) {
+				// User already specified imports: inject normally so the engine
+				// definition participates in the full import processing chain.
+				orchestratorEngineLog.Printf("Injecting builtin engine import: %s", builtinPath)
+				addImportToFrontmatter(result.Frontmatter, builtinPath)
+			} else {
+				// No user imports: skip the injection to avoid the import
+				// processing overhead. The engine catalog already has the full
+				// definition for this builtin engine.
+				orchestratorEngineLog.Printf("Skipping builtin engine import injection (no user imports): %s", builtinPath)
+				builtinSkippedInjection = true
+			}
 			delete(result.Frontmatter, "engine")
+			builtinEngineID = engineSetting
 			engineSetting = ""
 			engineConfig = nil
 		}
@@ -216,38 +246,74 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean
 	// Combine imported engines with included engines
 	allEngines := append(importsResult.MergedEngines, includedEngines...)
 
-	// Validate that only one engine field exists across all files
-	orchestratorEngineLog.Printf("Validating single engine specification")
-	finalEngineSetting, err := c.validateSingleEngineSpecification(engineSetting, allEngines)
-	if err != nil {
-		orchestratorEngineLog.Printf("Engine specification validation failed: %v", err)
-		return nil, err
-	}
-	if finalEngineSetting != "" {
-		engineSetting = finalEngineSetting
-	}
-
-	// If engineConfig is nil (engine was in an included file), extract it from the included engine JSON
-	if engineConfig == nil && len(allEngines) > 0 {
-		orchestratorEngineLog.Printf("Extracting engine config from included file")
-		extractedConfig, err := c.extractEngineConfigFromJSON(allEngines[0])
+	// Resolve engine setting and config from the available engine specifications.
+	//
+	// Fast path: when we know the engine ID (builtinEngineID) and there are
+	// no engine specs from @include directives or conflicting user imports, skip the
+	// JSON parsing overhead of validateSingleEngineSpecification and
+	// extractEngineConfigFromJSON (saves ~15µs per ParseWorkflowFile call for the
+	// common case of a simple string-form builtin engine with no imports).
+	//
+	// The fast path is safe when:
+	//  - len(includedEngines) == 0: no @include directives contributed engine specs
+	//  - len(importsResult.MergedEngines) <= 1: at most one engine entry from imports.
+	//    When builtinSkippedInjection=true (no user imports), len == 0 always.
+	//    When builtinSkippedInjection=false (builtin was injected), len == 1 means
+	//    only the injected builtin contributed; len > 1 means user imports also had
+	//    an engine field → fall through to the conflict-detection general path.
+	//
+	// General path: when @include/import engines are present, run full validation to
+	// detect and report duplicate engine specifications with a helpful error message.
+	// When the builtin injection was skipped (builtinSkippedInjection), pass the
+	// known engine ID as mainEngineSetting so the validator counts it correctly.
+	if builtinEngineID != "" && len(includedEngines) == 0 && len(importsResult.MergedEngines) <= 1 {
+		// Common case: single builtin engine, no @include engines, no conflicting imports.
+		// EngineConfig only needs the ID here; downstream code (engineCatalog.Resolve)
+		// retrieves the full engine definition (runtime, auth, etc.) from the catalog
+		// that was populated at startup by loadBuiltinEngineDefinitions.
+		engineSetting = builtinEngineID
+		engineConfig = &EngineConfig{ID: builtinEngineID}
+		orchestratorEngineLog.Printf("Fast path: reusing pre-known builtin engine setting %s", engineSetting)
+	} else {
+		// General path: validate that only one engine field exists across all files.
+		// When the builtin injection was skipped, inject the pre-known engine ID as
+		// the "main engine" so the duplicate check accounts for it correctly.
+		mainEngineForValidation := engineSetting
+		if builtinSkippedInjection {
+			mainEngineForValidation = builtinEngineID
+		}
+		orchestratorEngineLog.Printf("Validating single engine specification")
+		finalEngineSetting, err := c.validateSingleEngineSpecification(mainEngineForValidation, allEngines)
 		if err != nil {
-			orchestratorEngineLog.Printf("Failed to extract engine config: %v", err)
-			return nil, fmt.Errorf("failed to extract engine config from included file: %w", err)
+			orchestratorEngineLog.Printf("Engine specification validation failed: %v", err)
+			return nil, err
 		}
-		engineConfig = extractedConfig
-
-		// If the imported engine is an inline definition (engine.runtime sub-object),
-		// validate and register it in the catalog. This mirrors the handling for inline
-		// definitions declared directly in the main workflow (above).
-		if engineConfig != nil && engineConfig.IsInlineDefinition {
-			if err := c.validateEngineInlineDefinition(engineConfig); err != nil {
-				return nil, err
+		if finalEngineSetting != "" {
+			engineSetting = finalEngineSetting
+		}
+
+		// If engineConfig is nil (engine was in an included file), extract it from the included engine JSON
+		if engineConfig == nil && len(allEngines) > 0 {
+			orchestratorEngineLog.Printf("Extracting engine config from included file")
+			extractedConfig, err := c.extractEngineConfigFromJSON(allEngines[0])
+			if err != nil {
+				orchestratorEngineLog.Printf("Failed to extract engine config: %v", err)
+				return nil, fmt.Errorf("failed to extract engine config from included file: %w", err)
 			}
-			if err := c.validateEngineAuthDefinition(engineConfig); err != nil {
-				return nil, err
+			engineConfig = extractedConfig
+
+			// If the imported engine is an inline definition (engine.runtime sub-object),
+			// validate and register it in the catalog. This mirrors the handling for inline
+			// definitions declared directly in the main workflow (above).
+			if engineConfig != nil && engineConfig.IsInlineDefinition {
+				if err := c.validateEngineInlineDefinition(engineConfig); err != nil {
+					return nil, err
+				}
+				if err := c.validateEngineAuthDefinition(engineConfig); err != nil {
+					return nil, err
+				}
+				c.registerInlineEngineDefinition(engineConfig)
 			}
-			c.registerInlineEngineDefinition(engineConfig)
 		}
 	}
 
@@ -381,7 +447,42 @@ func isStringFormEngine(frontmatter map[string]any) bool {
 	return isString
 }
 
-// addImportToFrontmatter appends importPath to the "imports" slice in frontmatter.
+// frontmatterHasUserImports reports whether the "imports" field in frontmatter
+// contains at least one user-specified import path. It returns false for a missing
+// "imports" key, an empty slice/array (`imports: []`), and empty object forms
+// (`imports: {}` or `imports: {aw: []}`), which all produce zero import specs.
+// This mirrors the early-exit logic in processImportsFromFrontmatterWithManifestAndSource
+// so the optimisation fires whenever that function would return an empty result.
+func frontmatterHasUserImports(frontmatter map[string]any) bool {
+	raw, exists := frontmatter["imports"]
+	if !exists {
+		return false
+	}
+	switch v := raw.(type) {
+	case []any:
+		return len(v) > 0
+	case []string:
+		return len(v) > 0
+	case string:
+		return v != ""
+	case map[string]any:
+		// Object form {aw: [...]}
+		if awAny, hasAW := v["aw"]; hasAW {
+			switch aw := awAny.(type) {
+			case []any:
+				return len(aw) > 0
+			case []string:
+				return len(aw) > 0
+			}
+		}
+		return false
+	default:
+		// Unknown type — treat conservatively as "has imports" so the builtin
+		// injection still fires and the downstream parser can validate it.
+		return true
+	}
+}
+
 // It handles the case where "imports" may be absent, a []any, a []string, or a
 // single string (which is converted to a two-element slice preserving the original value).
 // When "imports" is an object (map) with an "aw" subfield, the path is appended to "aw".