fix: handle uses:/path: map imports in fetchFrontmatterImportsRecursive#23439
fix: handle uses:/path: map imports in fetchFrontmatterImportsRecursive#23439
Conversation
…ve and extractImportPaths
The fetchFrontmatterImportsRecursive function in pkg/cli/includes.go only handled
string items in the imports array, silently skipping map items using the uses:/with:
object form (GitHub Actions reusable workflow syntax). This caused transitive imports
to not be downloaded when a nested shared file used the uses: form.
For example, shared/mcp/serena-go.md imports shared/mcp/serena.md using:
imports:
- uses: shared/mcp/serena.md
with:
languages: ["go"]
Since serena.md was never downloaded to the temp dir, compilation of smoke-copilot.md
would fail with "import file not found" when it tried to compile serena-go.md's imports.
Fix:
- Update fetchFrontmatterImportsRecursive to extract paths from map items with uses: or path: keys
- Update extractImportPaths in import_topological.go to also handle uses: key (for consistency)
- Add a unit test TestFetchAndSaveRemoteFrontmatterImports_UsesFormSkippedWhenWorkflowSpec
This fixes TestAddWorkflowWithDispatchWorkflowFromSharedImport which downloads
smoke-copilot.md@531594a (which has serena-go.md as an import).
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4321d63a-b318-48fb-81cb-9bcdd2d7a03b
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Fixes recursive downloading of frontmatter imports when import entries use the GitHub Actions reusable-workflow object form (uses:/with:), which was causing transitive imports to be skipped and CI to fail.
Changes:
- Extend frontmatter import-path extraction to recognize map/object items with
uses:(andpath:) in the CLI recursive fetcher. - Extend topological import dependency extraction to recognize
uses:for consistency. - Add a unit test intended to cover
uses:-form imports in the remote frontmatter fetch flow.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/cli/includes.go | Updates recursive frontmatter import fetching to extract paths from imports map items (uses/path). |
| pkg/parser/import_topological.go | Makes import dependency extraction recognize uses in addition to path. |
| pkg/cli/remote_workflow_test.go | Adds a unit test covering uses:-form imports for the remote frontmatter import fetcher. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if pathValue, hasPath := importItem["path"]; hasPath { | ||
| if pathStr, ok := pathValue.(string); ok { | ||
| imports = append(imports, pathStr) | ||
| } | ||
| } else if usesValue, hasUses := importItem["uses"]; hasUses { |
There was a problem hiding this comment.
The comment says imports objects are "with path", but this code now also supports the uses alias. Please update the nearby comment to reflect that both path and uses are accepted so it stays consistent with the implementation.
| // TestFetchAndSaveRemoteFrontmatterImports_UsesFormSkippedWhenWorkflowSpec verifies that | ||
| // an import written in the uses:/with: object form (GitHub Actions reusable workflow syntax) | ||
| // that points to an already-pinned workflowspec path is also skipped — exactly as the | ||
| // equivalent string-form import would be. This exercises the map-item branch added to | ||
| // handle uses:/path: object imports. | ||
| func TestFetchAndSaveRemoteFrontmatterImports_UsesFormSkippedWhenWorkflowSpec(t *testing.T) { | ||
| content := `--- | ||
| engine: copilot | ||
| imports: | ||
| - uses: github/gh-aw/.github/workflows/shared/mcp/serena.md@abc123 | ||
| with: | ||
| languages: ["go"] | ||
| --- | ||
|
|
||
| # Workflow with uses: form import | ||
| ` | ||
| spec := &WorkflowSpec{ | ||
| RepoSpec: RepoSpec{ | ||
| RepoSlug: "github/gh-aw", | ||
| Version: "main", | ||
| }, | ||
| WorkflowPath: ".github/workflows/ci-coach.md", | ||
| } | ||
|
|
||
| tmpDir := t.TempDir() | ||
| // The uses: import path is in workflowspec format so it should be skipped, | ||
| // just like a string-form workflowspec import would be. | ||
| err := fetchAndSaveRemoteFrontmatterImports(content, spec, tmpDir, false, false, nil) | ||
| require.NoError(t, err, "should not error for uses: form workflowspec imports") | ||
|
|
||
| entries, readErr := os.ReadDir(tmpDir) | ||
| require.NoError(t, readErr) | ||
| assert.Empty(t, entries, "uses: form workflowspec imports should not be downloaded") | ||
| } |
There was a problem hiding this comment.
This new test doesn’t actually exercise the newly-added uses: map-item parsing: before this PR, the uses: entry would be silently ignored, importPaths would be empty, and the temp dir would still be empty (so the assertion would pass). Please adjust the test so it fails without the fix—e.g., use a relative uses: import and assert an observable side effect that only happens when the map item is parsed (such as the recursive helper’s seen set being populated, or a pre-existing local file being detected/skipped).
Summary
Fixes the
TestAddWorkflowWithDispatchWorkflowFromSharedImportCI failure (job 69062185259).Root Cause
fetchFrontmatterImportsRecursiveinpkg/cli/includes.goonly extracted string items from theimportsarray, silently skipping map items using theuses:/with:object form (GitHub Actions reusable workflow syntax).This meant transitive
uses:form imports were never downloaded. When the test downloadssmoke-copilot.md@531594a, it fetchesshared/mcp/serena-go.md(a plain string import). Butserena-go.mditself importsshared/mcp/serena.mdvia:Since the recursive fetcher didn't handle the
uses:form,serena.mdwas never downloaded to the temp dir. Compilation ofsmoke-copilot.mdthen failed with:Changes
pkg/cli/includes.go: updatefetchFrontmatterImportsRecursiveto extract import paths from map items withuses:orpath:keys, mirroringparseImportSpecsFromArrayin the compilerpkg/parser/import_topological.go: updateextractImportPathsto also handle theuses:key for consistency (previously only handledpath:)pkg/cli/remote_workflow_test.go: add unit testTestFetchAndSaveRemoteFrontmatterImports_UsesFormSkippedWhenWorkflowSpecverifyinguses:form imports are recognized