diff --git a/pkg/parser/import_bfs.go b/pkg/parser/import_bfs.go index b2dc7373870..3667e996943 100644 --- a/pkg/parser/import_bfs.go +++ b/pkg/parser/import_bfs.go @@ -302,19 +302,25 @@ func processImportsFromFrontmatterWithManifestAndSource(frontmatter map[string]a result, err = ExtractFrontmatterFromContent(string(content)) } - // When the import provides 'with' inputs, apply expression substitution before - // discovering nested imports. This resolves ${{ github.aw.import-inputs.* }} + // Apply import-schema defaults before discovering nested imports, even when no + // explicit 'with:' inputs were provided. This resolves ${{ github.aw.import-inputs.* }} // expressions that appear in the 'with' values of nested imports, enabling // multi-level workflow composition. // We reuse the already-parsed frontmatter to extract import-schema defaults, // avoiding a second YAML parse inside applyImportSchemaDefaults. - if err == nil && result != nil && len(item.inputs) > 0 { + if err == nil && result != nil { inputsWithDefaults := applyImportSchemaDefaultsFromFrontmatter(result.Frontmatter, item.inputs) - substituted := substituteImportInputsInContent(string(content), inputsWithDefaults) - // Re-parse the substituted content so that nested-import discovery sees - // the resolved 'with' values instead of literal expression strings. - if reparse, rerr := ExtractFrontmatterFromContent(substituted); rerr == nil { - result = reparse + if len(inputsWithDefaults) > 0 { + origContent := string(content) + substituted := substituteImportInputsInContent(origContent, inputsWithDefaults) + // Only re-parse when substitution actually changed the content. + // If no ${{ github.aw.import-inputs.* }} expressions were present, + // the content is unchanged and a YAML reparse would be wasteful. + if substituted != origContent { + if reparse, rerr := ExtractFrontmatterFromContent(substituted); rerr == nil { + result = reparse + } + } } } if err != nil { diff --git a/pkg/parser/import_field_extractor.go b/pkg/parser/import_field_extractor.go index aa417018dfd..0bec0c6e6fc 100644 --- a/pkg/parser/import_field_extractor.go +++ b/pkg/parser/import_field_extractor.go @@ -19,7 +19,7 @@ import ( type importAccumulator struct { toolsBuilder strings.Builder mcpServersBuilder strings.Builder - markdownBuilder strings.Builder // Only used for imports WITH inputs (compile-time substitution) + markdownBuilder strings.Builder // imports with substituted inputs or schema defaults (compile-time substitution) importPaths []string // Import paths for runtime-import macro generation stepsBuilder strings.Builder copilotSetupStepsBuilder strings.Builder // Steps from copilot-setup-steps.yml (inserted at start) @@ -82,26 +82,47 @@ func newImportAccumulator() *importAccumulator { func (acc *importAccumulator) extractAllImportFields(content []byte, item importQueueItem, visited map[string]bool) error { log.Printf("Extracting all import fields: path=%s, section=%s, inputs=%d, content_size=%d bytes", item.fullPath, item.sectionName, len(item.inputs), len(content)) - // When the import provides 'with' inputs, apply expression substitution to the raw - // content before any YAML or markdown processing. This enables ${{ github.aw.import-inputs.* }} - // expressions in the imported workflow's frontmatter fields (tools, runtimes, etc.) - // as well as in the markdown body. Array and map values are serialized as JSON so they - // produce valid YAML inline syntax (e.g. ["go","typescript"]). - rawContent := string(content) - if len(item.inputs) > 0 { - // Apply import-schema defaults for any optional parameters not supplied by the caller, - // so that ${{ github.aw.import-inputs. }} expressions for defaulted parameters - // are replaced with their declared default values rather than left as literal strings. - inputsWithDefaults := applyImportSchemaDefaults(rawContent, item.inputs) - rawContent = substituteImportInputsInContent(rawContent, inputsWithDefaults) + // Parse frontmatter once from the original content. This parse is reused for + // import-schema default extraction and schema validation, avoiding redundant YAML parsing. + // For builtin files we use the process-level cache. + origContent := string(content) + var origParsed *FrontmatterResult + var origParseErr error + if strings.HasPrefix(item.fullPath, BuiltinPathPrefix) { + origParsed, origParseErr = ExtractFrontmatterFromBuiltinFile(item.fullPath, content) + } else { + origParsed, origParseErr = ExtractFrontmatterFromContent(origContent) } + var origFm map[string]any + if origParseErr == nil { + origFm = origParsed.Frontmatter + } else { + origFm = make(map[string]any) + } + + // Apply import-schema defaults before any YAML or markdown processing, even when no + // explicit 'with:' inputs were provided by the importing workflow. This enables + // ${{ github.aw.import-inputs.* }} expressions in the imported workflow's frontmatter + // fields (engine, safe-outputs, tools, runtimes, etc.) and markdown body to resolve + // to their declared default values rather than remaining as literal strings. + // Array and map values are serialized as JSON so they produce valid YAML inline syntax. + // We reuse the already-parsed frontmatter to avoid a second YAML parse. + inputsWithDefaults := applyImportSchemaDefaultsFromFrontmatter(origFm, item.inputs) + rawContent := origContent + if len(inputsWithDefaults) > 0 { + rawContent = substituteImportInputsInContent(origContent, inputsWithDefaults) + // Add resolved defaults to acc.importInputs so the compile-time markdown + // substitution pass (generatePrompt) also has access to them. + maps.Copy(acc.importInputs, inputsWithDefaults) + } + wasSubstituted := rawContent != origContent // Extract tools from imported file. - // When inputs are present we use the already-substituted content (to pick up any - // ${{ github.aw.import-inputs.* }} expressions in the tools/mcp-servers frontmatter) - // rather than re-reading the original file from disk. + // When content was modified by substitution (either explicit inputs or schema defaults), + // we use the already-substituted content (to pick up any ${{ github.aw.import-inputs.* }} + // expressions in the tools/mcp-servers frontmatter) rather than re-reading the original file. var toolsContent string - if len(item.inputs) > 0 { + if wasSubstituted { var err error toolsContent, err = extractToolsFromContent(rawContent) if err != nil { @@ -116,21 +137,22 @@ func (acc *importAccumulator) extractAllImportFields(content []byte, item import } acc.toolsBuilder.WriteString(toolsContent + "\n") - // Track import path for runtime-import macro generation (only if no inputs). - // Imports with inputs must be inlined for compile-time substitution. + // Track import path for runtime-import macro generation (only if no substitution happened). + // Imports with substituted inputs (explicit or via schema defaults) must be inlined for + // compile-time substitution so that ${{ github.aw.import-inputs.* }} expressions are resolved. // Builtin paths (@builtin:…) are pure configuration — they carry no user-visible // prompt content and must not generate runtime-import macros. importRelPath := computeImportRelPath(item.fullPath, item.importPath) - if len(item.inputs) == 0 && !strings.HasPrefix(importRelPath, BuiltinPathPrefix) { - // No inputs and not a builtin - use runtime-import macro + if !wasSubstituted && !strings.HasPrefix(importRelPath, BuiltinPathPrefix) { + // No substitution happened and not a builtin - use runtime-import macro acc.importPaths = append(acc.importPaths, importRelPath) log.Printf("Added import path for runtime-import: %s", importRelPath) - } else if len(item.inputs) > 0 { - // Has inputs - must inline for compile-time substitution. + } else if wasSubstituted { + // Content was modified by substitution - inline for compile-time substitution. // Extract markdown from the already-substituted content so that import-inputs // expressions embedded in the markdown body are resolved here. - log.Printf("Import %s has inputs - will be inlined for compile-time substitution", importRelPath) + log.Printf("Import %s has substituted inputs - will be inlined for compile-time substitution", importRelPath) markdownContent, err := ExtractMarkdownContent(rawContent) if err != nil { return fmt.Errorf("failed to extract markdown from imported file '%s': %w", item.fullPath, err) @@ -148,47 +170,29 @@ func (acc *importAccumulator) extractAllImportFields(content []byte, item import } } - // Parse frontmatter once to avoid redundant YAML parsing for each field extraction. - // All subsequent field extractions use the pre-parsed result. - // When inputs are present we parse the already-substituted content so that all - // frontmatter fields (runtimes, mcp-servers, etc.) reflect the resolved values. - // For builtin files without inputs, use the process-level cache to avoid redundant - // YAML re-parsing (processIncludedFileWithVisited already populated this cache). - // Builtin files WITH inputs must skip the cache because input substitution modifies - // the content, so the cached (unsubstituted) result would be stale. - var parsed *FrontmatterResult - var err error - if strings.HasPrefix(item.fullPath, BuiltinPathPrefix) && len(item.inputs) == 0 { - parsed, err = ExtractFrontmatterFromBuiltinFile(item.fullPath, content) - } else { - parsed, err = ExtractFrontmatterFromContent(rawContent) - } + // Parse frontmatter from the (possibly substituted) content for field extraction. + // All subsequent field extractions use this pre-parsed result. + // When substitution changed the content, reparse from rawContent so that all + // frontmatter fields (runtimes, mcp-servers, engine, safe-outputs, etc.) reflect + // the resolved values. When content is unchanged we reuse origFm, which was already + // parsed above — for builtin files the cache also applies. var fm map[string]any - if err == nil { - fm = parsed.Frontmatter + if wasSubstituted { + if reparsed, rerr := ExtractFrontmatterFromContent(rawContent); rerr == nil { + fm = reparsed.Frontmatter + } else { + fm = make(map[string]any) + } } else { - fm = make(map[string]any) + fm = origFm } // Validate 'with'/'inputs' values against the imported workflow's 'import-schema' (if present). - // Run validation even when inputs is nil/empty so required fields can be detected. - // Use the ORIGINAL (unsubstituted) frontmatter for schema lookup so the import-schema + // Always use the ORIGINAL (unsubstituted) frontmatter for schema lookup so the import-schema // declaration itself is not affected by expression substitution. - if len(item.inputs) > 0 || string(content) != rawContent { - // When substitution happened, reload the original frontmatter for schema validation. - origParsed, origErr := ExtractFrontmatterFromContent(string(content)) - if origErr == nil { - if _, hasSchema := origParsed.Frontmatter["import-schema"]; hasSchema { - if err := validateWithImportSchema(item.inputs, origParsed.Frontmatter, item.importPath); err != nil { - return err - } - } - } - } else { - if _, hasSchema := fm["import-schema"]; hasSchema { - if err := validateWithImportSchema(item.inputs, fm, item.importPath); err != nil { - return err - } + if _, hasSchema := origFm["import-schema"]; hasSchema { + if err := validateWithImportSchema(item.inputs, origFm, item.importPath); err != nil { + return err } } @@ -694,18 +698,6 @@ func validateImportInputType(name string, value any, declaredType string, paramD return nil } -// applyImportSchemaDefaults reads the import-schema from rawContent and returns a copy -// of inputs augmented with default values for any schema parameters that are declared -// with a "default" field but not present in the provided inputs map. Parameters that -// are already in inputs are left unchanged. -func applyImportSchemaDefaults(rawContent string, inputs map[string]any) map[string]any { - parsed, err := ExtractFrontmatterFromContent(rawContent) - if err != nil { - return inputs - } - return applyImportSchemaDefaultsFromFrontmatter(parsed.Frontmatter, inputs) -} - // applyImportSchemaDefaultsFromFrontmatter applies import-schema defaults from an // already-parsed frontmatter map, avoiding a redundant YAML parse when the caller // has already extracted the frontmatter. Returns a copy of inputs augmented with diff --git a/pkg/workflow/import_schema_test.go b/pkg/workflow/import_schema_test.go index e141b08211d..d870733ab55 100644 --- a/pkg/workflow/import_schema_test.go +++ b/pkg/workflow/import_schema_test.go @@ -658,3 +658,190 @@ imports: } }) } + +// TestImportSchemaDefaultsNoExplicitInputs tests that import-schema default values are +// applied when no explicit 'with:' inputs are provided by the importing workflow. +// This verifies the fix where ${{ github.aw.import-inputs.* }} expressions in 'engine' +// and 'safe-outputs' sections were not resolved to their default values when no inputs +// were passed. +func TestImportSchemaDefaultsNoExplicitInputs(t *testing.T) { + tempDir := testutil.TempDir(t, "test-import-defaults-no-inputs-*") + + sharedDir := filepath.Join(tempDir, "shared") + if err := os.MkdirAll(sharedDir, 0755); err != nil { + t.Fatalf("Failed to create shared directory: %v", err) + } + + t.Run("engine section with schema default", func(t *testing.T) { + sharedPath := filepath.Join(sharedDir, "engine-defaults.md") + sharedContent := `--- +import-schema: + claude-max-turns: + type: integer + default: 10000 + description: Maximum number of chat iterations per run + +engine: + id: claude + max-turns: ${{ github.aw.import-inputs.claude-max-turns }} +--- + +# Shared Engine Workflow +` + if err := os.WriteFile(sharedPath, []byte(sharedContent), 0644); err != nil { + t.Fatalf("Failed to write shared file: %v", err) + } + + workflowPath := filepath.Join(tempDir, "main-engine.md") + workflowContent := `--- +on: issues +permissions: + contents: read + issues: read +imports: + - shared/engine-defaults.md +--- + +# Main Workflow +` + if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + compiler := workflow.NewCompiler() + if err := compiler.CompileWorkflow(workflowPath); err != nil { + t.Fatalf("CompileWorkflow failed: %v", err) + } + + lockFilePath := stringutil.MarkdownToLockFile(workflowPath) + lockFileContent, err := os.ReadFile(lockFilePath) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + lockContent := string(lockFileContent) + + // The engine max-turns should be resolved to the default value (10000). + // In the compiled lock file the value appears as the GH_AW_MAX_TURNS env var. + if strings.Contains(lockContent, "github.aw.import-inputs.claude-max-turns") { + t.Error("Lock file should not contain unresolved github.aw.import-inputs.claude-max-turns expression") + } + if !strings.Contains(lockContent, "GH_AW_MAX_TURNS: 10000") { + t.Errorf("Lock file should contain GH_AW_MAX_TURNS: 10000 from the schema default; got:\n%s", lockContent) + } + }) + + t.Run("safe-outputs section with schema default", func(t *testing.T) { + sharedPath := filepath.Join(sharedDir, "safe-outputs-defaults.md") + sharedContent := `--- +import-schema: + expires: + type: string + default: "3d" + description: "How long to keep discussions before expiry" + +safe-outputs: + create-discussion: + expires: ${{ github.aw.import-inputs.expires }} +--- + +# Shared Safe Outputs Workflow +` + if err := os.WriteFile(sharedPath, []byte(sharedContent), 0644); err != nil { + t.Fatalf("Failed to write shared file: %v", err) + } + + workflowPath := filepath.Join(tempDir, "main-safe-outputs.md") + workflowContent := `--- +on: issues +permissions: + contents: read + issues: read +engine: copilot +imports: + - shared/safe-outputs-defaults.md +--- + +# Main Workflow +` + if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + compiler := workflow.NewCompiler() + if err := compiler.CompileWorkflow(workflowPath); err != nil { + t.Fatalf("CompileWorkflow failed: %v", err) + } + + lockFilePath := stringutil.MarkdownToLockFile(workflowPath) + lockFileContent, err := os.ReadFile(lockFilePath) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + lockContent := string(lockFileContent) + + // The safe-outputs expires should be resolved to the default value ("3d" = 72 hours). + // In the compiled lock file the value appears as JSON in the safe-outputs config. + if strings.Contains(lockContent, "github.aw.import-inputs.expires") { + t.Error("Lock file should not contain unresolved github.aw.import-inputs.expires expression") + } + if !strings.Contains(lockContent, `"expires":72`) { + t.Errorf("Lock file should contain expires:72 (3d = 72h) from the schema default; got:\n%s", lockContent) + } + }) + + t.Run("uses/with syntax with no with block", func(t *testing.T) { + sharedPath := filepath.Join(sharedDir, "uses-defaults.md") + sharedContent := `--- +import-schema: + model: + type: string + default: "claude-sonnet-4-5" + description: "AI model to use" + +engine: + id: claude + model: ${{ github.aw.import-inputs.model }} +--- + +# Shared Uses Workflow +` + if err := os.WriteFile(sharedPath, []byte(sharedContent), 0644); err != nil { + t.Fatalf("Failed to write shared file: %v", err) + } + + workflowPath := filepath.Join(tempDir, "main-uses.md") + workflowContent := `--- +on: issues +permissions: + contents: read + issues: read +imports: + - uses: shared/uses-defaults.md +--- + +# Main Workflow +` + if err := os.WriteFile(workflowPath, []byte(workflowContent), 0644); err != nil { + t.Fatalf("Failed to write workflow file: %v", err) + } + + compiler := workflow.NewCompiler() + if err := compiler.CompileWorkflow(workflowPath); err != nil { + t.Fatalf("CompileWorkflow failed: %v", err) + } + + lockFilePath := stringutil.MarkdownToLockFile(workflowPath) + lockFileContent, err := os.ReadFile(lockFilePath) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + lockContent := string(lockFileContent) + + if strings.Contains(lockContent, "github.aw.import-inputs.model") { + t.Error("Lock file should not contain unresolved github.aw.import-inputs.model expression") + } + if !strings.Contains(lockContent, "ANTHROPIC_MODEL: claude-sonnet-4-5") { + t.Errorf("Lock file should contain ANTHROPIC_MODEL: claude-sonnet-4-5 from the schema default; got:\n%s", lockContent) + } + }) +}