diff --git a/pkg/workflow/compiler_orchestrator_engine.go b/pkg/workflow/compiler_orchestrator_engine.go index 6d47c6ebfb..8ff63ea7ff 100644 --- a/pkg/workflow/compiler_orchestrator_engine.go +++ b/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".