Preserve declared imports: order in compiled prompt assembly#36825
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
imports: order in compiled prompt assembly
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
🧪 Test Quality Sentinel completed test quality analysis. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Pull request overview
This PR updates the workflow compiler pipeline to preserve the declared imports: order when assembling the compiled prompt, even when some imports are emitted as inline markdown (parameterized imports) and others are emitted as {{#runtime-import ...}} macros.
Changes:
- Introduces an ordered per-import prompt contribution model (
parser.PromptImportEntry) and threads it through compiler structs. - Updates prompt generation (
generatePrompt) to interleave runtime-import macros and inlined markdown in declaration order, with a legacy fallback path. - Adds a regression test covering a mixed runtime + parameterized import ordering case.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/workflow_builder.go | Wires ordered prompt import entries into WorkflowData. |
| pkg/workflow/imports_inputs_test.go | Adds regression test ensuring mixed imports preserve declared ordering. |
| pkg/workflow/compiler_yaml.go | Updates prompt assembly to consume ordered import entries and interleave output. |
| pkg/workflow/compiler_types.go | Extends WorkflowData with PromptImports. |
| pkg/workflow/compiler_orchestrator_tools.go | Threads ordered import entries through markdown/tool processing artifacts. |
| pkg/parser/import_processor.go | Defines PromptImportEntry and adds it to ImportsResult. |
| pkg/parser/import_field_extractor.go | Records ordered prompt contributions while processing imports. |
| pkg/parser/import_bfs.go | Records ordered prompt contributions for agent-file imports. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 2
| mainWorkflowMarkdown := markdownContent | ||
| orchestratorToolsLog.Printf("Main workflow markdown: %d bytes", len(mainWorkflowMarkdown)) | ||
| importPaths := append([]string{}, importsResult.ImportPaths...) | ||
| promptImports := append([]parser.PromptImportEntry{}, importsResult.PromptImports...) | ||
| if len(importPaths) > 0 { |
| // Step 1a/1b: Process imports in declaration order, interleaving: | ||
| // - compile-time inlined markdown (imports with inputs) | ||
| // - runtime-import macros (imports without inputs) | ||
| // In older workflow data (without PromptImports), fall back to legacy grouped handling. | ||
| if data.PromptImports != nil { | ||
| compilerYamlLog.Printf("Processing %d ordered prompt import entries", len(data.PromptImports)) |
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (191 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs MatterADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. This change introduces a new ordered prompt-import data model ( 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
🧪 Test Quality Sentinel Report
📊 Metrics & Test Classification (1 test analyzed)
Test Classification Details
Language SupportTests analyzed:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnose, /tdd, and /zoom-out — the fix is sound and the overall approach is clean. Leaving a COMMENT review with a few non-blocking suggestions around observability, test coverage, and assertion strength.
📋 Key Themes & Highlights
Key Themes
- Silent secondary bug fix: The old code had
expressionMappings = exprMaps(overwrite) for theImportedMarkdowncase. The newappendis correct, but this implicit fix is unacknowledged and untested. - Test assertion strength: The ordering test uses
strings.Indexfor relative position, which passes even with unrelated content between the two markers. A regex or tighter window would catch real regressions more reliably. - Uncovered
inlinedImportscode path: The new ordered loop handlesinlinedImports: truemode, but no test exercises it. - Observability regression: The
Inlined import without inputslog line was removed without replacement in the new path.
Positive Highlights
- ✅ Clean
PromptImportEntryunion type — simple, purpose-fit, well-commented - ✅ Legacy fallback (
data.PromptImports == nil) is a good backward-compatibility hedge - ✅ Regression test that directly encodes the bug scenario is exactly right — good instinct
- ✅ Data flow threading across parser → orchestrator → compiler_types → compiler_yaml is tidy and consistent
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 1.6M
| } | ||
| chunks, exprMaps := extractPromptChunksFromMarkdown(cleaned) | ||
| userPromptChunks = append(userPromptChunks, chunks...) | ||
| expressionMappings = append(expressionMappings, exprMaps...) |
There was a problem hiding this comment.
[/diagnose] Silent bug fix for expressionMappings — the old code used expressionMappings = exprMaps (overwrite), which would have dropped mappings from any previously processed import. The new append is correct, but this implicit fix is untested.
💡 Suggested regression test addition
Consider adding a test case with multiple parameterized imports (uses: ... with: ...) and verifying that expressionMappings from all of them are preserved in the compiled output. Without such a test, a future refactor could silently reintroduce the overwrite.
// two parameterized imports → both expression mappings must survive
imports:
- uses: shared/a.md
with: {x: "${{github.event.issue.number}}"}
- uses: shared/b.md
with: {y: "${{github.event.issue.title}}"}| expressionMappings = append(expressionMappings, exprMaps...) | ||
| continue | ||
| } | ||
| if entry.ImportPath == "" { |
There was a problem hiding this comment.
[/diagnose] Zero-value PromptImportEntry{} (both ImportPath and Markdown empty) is silently skipped with no log output. This could hide data flow bugs — e.g., a missed append site in the parser producing empty entries.
💡 Suggestion
Add a warning log before the continue:
if entry.ImportPath == "" {
compilerYamlLog.Printf("Warning: skipping empty PromptImportEntry (both fields unset)")
continue
}This costs nothing at runtime and makes unexpected zero-value entries immediately visible during debugging.
| userPromptChunks = append(userPromptChunks, chunks...) | ||
| expressionMappings = append(expressionMappings, exprMaps...) | ||
| compilerYamlLog.Printf("Inlined import without inputs: %s", importPath) | ||
| continue |
There was a problem hiding this comment.
[/diagnose] The successful inlined-import log line (Inlined import without inputs: %s) was removed in the new path but not replaced. This makes it harder to trace which imports were resolved from disk vs emitted as runtime-import macros.
💡 Suggestion
Restore the log after the successful inline:
compilerYamlLog.Printf("Inlined import without inputs (ordered): %s", importPath)
continue| if parameterizedIdx == -1 { | ||
| t.Fatalf("Expected lock file to contain substituted parameterized import content %q", parameterizedContent) | ||
| } | ||
| if runtimeIdx > parameterizedIdx { |
There was a problem hiding this comment.
[/tdd] The ordering assertion uses strings.Index which only checks relative substring position — it passes even if main workflow content appears between the two imports, or if either string appears in an unrelated part of the lock file (e.g., comments). This makes the test weaker than it looks.
💡 Stronger assertion approach
Consider splitting the lock file on a known separator or searching within a specific YAML block, then asserting the exact sequence. Alternatively, assert a tight window:
// Assert the two markers appear with only whitespace/newlines between them
pattern := regexp.MustCompile(
`\{\{#runtime-import shared/infra\.md\}\}[\s\S]*?Use mode: strict\.`,
)
if !pattern.MatchString(lockContent) {
t.Fatalf("expected runtime-import to immediately precede parameterized content")
}This validates both order and adjacency.
| orchestratorToolsLog.Printf("Found %d body-level {{#runtime-import}} directive(s) to promote to lock-file macros", len(bodyImports)) | ||
| for _, bodyImport := range bodyImports { | ||
| importPaths = append(importPaths, bodyImport.Path) | ||
| promptImports = append(promptImports, parser.PromptImportEntry{ImportPath: bodyImport.Path}) |
There was a problem hiding this comment.
[/zoom-out] Body-level {{#runtime-import}} directives are always appended to promptImports after frontmatter import entries, so they will always appear at the end of the ordered import sequence. If a workflow body intentionally mixes {{#runtime-import}} macros between paragraphs of its own markdown, this relative ordering may diverge from what the author sees in the source file.
📋 Context
This behaviour is consistent with how importPaths was handled before this PR, so it is not a regression. However, now that promptImports is the canonical ordering mechanism, it is worth a brief comment here documenting the deliberate decision ("body-level macros always follow frontmatter imports") so future contributors don't accidentally change it.
| } | ||
| } | ||
|
|
||
| func TestImportPromptOrderInterleavesRuntimeAndParameterizedImports(t *testing.T) { |
There was a problem hiding this comment.
[/tdd] The new test covers only the inlinedImports: false (normal runtime-import macro) mode. The inlinedImports: true code path in the new ordered loop (lines 512–526 of compiler_yaml.go) is untested — a file-not-found fallback in that path would silently emit a runtime-import macro instead of inlining.
💡 Suggestion
Add a second test case that sets inlined-imports: true in the workflow frontmatter and asserts that the parameterized import content is actually embedded verbatim in the lock file, and that the runtime-import-able file is also embedded (not emitted as a macro).
|
@copilot review all comments |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed in 682d37e. I reviewed the in-scope comments and applied both requested fallback fixes: preserving nil semantics when cloning |
|
@copilot merge main, recompile, add tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
The compiler currently reorders effective prompt content when
imports:mixes prompt-only files and files that require runtime import handling (e.g., files withsteps:), causing agent context to diverge from declared import order. This change preserves import declaration order regardless of whether each import is emitted as inline markdown or{{#runtime-import ...}}.Ordered import representation
PromptImportEntry) to capture per-import prompt contribution in declaration order.ImportPath→ emit{{#runtime-import ...}}Markdown→ inline at compile time (e.g., parameterized imports)Compiler data flow updates
ImportPaths,MergedMarkdown) for compatibility and fallback behavior.nilsemantics when cloningPromptImportslen(PromptImports) > 0Prompt generation behavior
generatePromptto consume ordered entries and interleave runtime-import macros and inlined markdown exactly in declared order.Regression coverage
ImportedMarkdown/ImportPathsbehaviorresolveMarkdownArtifactspreservesnilPromptImportswhen source data is unavailableExpected compiled prompt ordering (schematic):
{{#runtime-import shared/infra.md}} ...inlined content from shared/instructions.md with substitutions... {{#runtime-import .github/workflows/<workflow>.md}}