From 090a09205aea8cec6519f59fd2213487f4bf7ede Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Apr 2026 19:44:29 +0000 Subject: [PATCH 1/4] Initial plan From 6cba0d9135dd6746bdb1cae8db1f406fc2cf4182 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Apr 2026 20:26:01 +0000 Subject: [PATCH 2/4] perf: eliminate redundant import processing for string-form builtin engines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a workflow uses a string-form engine (e.g. `engine: claude`) without user-specified imports, skip the builtin engine .md import injection entirely. All builtin engine .md files contain only an engine definition—no tools, steps, or runtimes—so their content is already available in the engine catalog (populated at startup by loadBuiltinEngineDefinitions). The full ProcessImportsFromFrontmatterWithSource round-trip is therefore redundant in this common case. When user imports ARE present, the builtin is still injected so it participates correctly in duplicate-engine detection. The engine setting and config are recovered from the pre-known `builtinInjectedEngineID` via a fast path that also avoids the JSON re-parsing overhead of both `validateSingleEngineSpecification` and `extractEngineConfigFromJSON`. `builtinSkippedInjection` is used to pass the engine ID to `validateSingleEngineSpecification` as the `mainEngineSetting` in the conflict detection path, so a workflow with `engine: claude` AND an `@include` that specifies another engine still raises the expected "multiple engine fields found" error. Benchmark improvement (BenchmarkParseWorkflow): Before: 184,226 ns/op 1,370 allocs/op After: ~155,000 ns/op 1,175 allocs/op (~16% faster, 195 fewer allocs/op) Fixes the 17.1% regression reported in the Daily CLI Performance workflow. Agent-Logs-Url: https://github.com/github/gh-aw/sessions/daa722df-c5fe-41ff-9396-31a84efed073 Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/workflow/compiler_orchestrator_engine.go | 122 ++++++++++++++----- 1 file changed, 92 insertions(+), 30 deletions(-) diff --git a/pkg/workflow/compiler_orchestrator_engine.go b/pkg/workflow/compiler_orchestrator_engine.go index 6d47c6ebfb8..eaaf28ba61e 100644 --- a/pkg/workflow/compiler_orchestrator_engine.go +++ b/pkg/workflow/compiler_orchestrator_engine.go @@ -126,12 +126,44 @@ 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 builtinInjectedEngineID 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 builtinInjectedEngineID 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) + _, hadUserImports := result.Frontmatter["imports"] + if hadUserImports { + // 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") + builtinInjectedEngineID = engineSetting engineSetting = "" engineConfig = nil } @@ -216,38 +248,68 @@ 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 (builtinInjectedEngineID) 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 the injected builtin in imports + // (> 1 means user imports also had an engine field → conflict) + // + // 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 builtinInjectedEngineID != "" && len(includedEngines) == 0 && len(importsResult.MergedEngines) <= 1 { + // Common case: single builtin engine, no @include engines, no conflicting imports. + engineSetting = builtinInjectedEngineID + engineConfig = &EngineConfig{ID: builtinInjectedEngineID} + 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 = builtinInjectedEngineID + } + 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 + } + if finalEngineSetting != "" { + engineSetting = finalEngineSetting } - 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 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) } } From 727531b2d9bb93fd43e8c56916f38c10c5b51e0a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 27 Apr 2026 20:28:04 +0000 Subject: [PATCH 3/4] perf: clarify fast-path condition comments in setupEngineAndImports Agent-Logs-Url: https://github.com/github/gh-aw/sessions/daa722df-c5fe-41ff-9396-31a84efed073 Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com> --- pkg/workflow/compiler_orchestrator_engine.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/pkg/workflow/compiler_orchestrator_engine.go b/pkg/workflow/compiler_orchestrator_engine.go index eaaf28ba61e..98e64f4b57f 100644 --- a/pkg/workflow/compiler_orchestrator_engine.go +++ b/pkg/workflow/compiler_orchestrator_engine.go @@ -258,8 +258,11 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean // // The fast path is safe when: // - len(includedEngines) == 0: no @include directives contributed engine specs - // - len(importsResult.MergedEngines) <= 1: at most the injected builtin in imports - // (> 1 means user imports also had an engine field → conflict) + // - 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. @@ -267,6 +270,9 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean // known engine ID as mainEngineSetting so the validator counts it correctly. if builtinInjectedEngineID != "" && 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 = builtinInjectedEngineID engineConfig = &EngineConfig{ID: builtinInjectedEngineID} orchestratorEngineLog.Printf("Fast path: reusing pre-known builtin engine setting %s", engineSetting) From 32f9b2efeef6b76e3fe9be505545d95e8241bc99 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 28 Apr 2026 01:11:07 +0000 Subject: [PATCH 4/4] perf: address review comments and merge main - Rename builtinInjectedEngineID -> builtinEngineID (clearer: tracks the pre-known engine ID regardless of whether injection was skipped) - Replace hadUserImports key-existence check with frontmatterHasUserImports() helper that correctly treats empty imports (imports: [], imports: {aw: []}, imports: '') as having no user imports, so the optimisation fires in those cases too - Merge origin/main and recompile all 204 workflow lock files Agent-Logs-Url: https://github.com/github/gh-aw/sessions/9b66f8d4-2619-42f5-bfec-d588d963e7ca Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/workflow/compiler_orchestrator_engine.go | 57 +++++++++++++++----- 1 file changed, 45 insertions(+), 12 deletions(-) diff --git a/pkg/workflow/compiler_orchestrator_engine.go b/pkg/workflow/compiler_orchestrator_engine.go index 98e64f4b57f..8ff63ea7ff3 100644 --- a/pkg/workflow/compiler_orchestrator_engine.go +++ b/pkg/workflow/compiler_orchestrator_engine.go @@ -134,8 +134,7 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean // 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 builtinInjectedEngineID so the fast path below can restore it without - // JSON re-parsing. + // 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, @@ -144,13 +143,12 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean // 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 builtinInjectedEngineID string + var builtinEngineID string var builtinSkippedInjection bool if c.engineOverride == "" && isStringFormEngine(result.Frontmatter) && engineSetting != "" { builtinPath := builtinEnginePath(engineSetting) if parser.BuiltinVirtualFileExists(builtinPath) { - _, hadUserImports := result.Frontmatter["imports"] - if hadUserImports { + 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) @@ -163,7 +161,7 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean builtinSkippedInjection = true } delete(result.Frontmatter, "engine") - builtinInjectedEngineID = engineSetting + builtinEngineID = engineSetting engineSetting = "" engineConfig = nil } @@ -250,7 +248,7 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean // Resolve engine setting and config from the available engine specifications. // - // Fast path: when we know the engine ID (builtinInjectedEngineID) and there are + // 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 @@ -268,13 +266,13 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean // 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 builtinInjectedEngineID != "" && len(includedEngines) == 0 && len(importsResult.MergedEngines) <= 1 { + 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 = builtinInjectedEngineID - engineConfig = &EngineConfig{ID: builtinInjectedEngineID} + 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. @@ -282,7 +280,7 @@ func (c *Compiler) setupEngineAndImports(result *parser.FrontmatterResult, clean // the "main engine" so the duplicate check accounts for it correctly. mainEngineForValidation := engineSetting if builtinSkippedInjection { - mainEngineForValidation = builtinInjectedEngineID + mainEngineForValidation = builtinEngineID } orchestratorEngineLog.Printf("Validating single engine specification") finalEngineSetting, err := c.validateSingleEngineSpecification(mainEngineForValidation, allEngines) @@ -449,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".