compiler: show full import chain for cycle errors#16554
Conversation
- Add ImportCycleError type with full chain tracking - Enhance topologicalSortImports to detect and report cycles - Add DFS-based cycle path finding with deterministic output - Format errors with multiline indented agent-friendly display - Add comprehensive tests for 2-file and 4-file cycles - Update existing TestCyclicImports to expect error (not silent handling) - Tests verify deterministic output and formatted error messages Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
- Use require.ErrorAs instead of assert.ErrorAs for error assertions - Use assert.Len instead of assert.Equal for length checks - All linting and tests now pass Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive import cycle detection and reporting for the workflow compiler, replacing silent cycle handling with detailed error messages showing the complete dependency chain.
Changes:
- Adds cycle detection to Kahn's topological sort algorithm with DFS-based path reconstruction
- Introduces ImportCycleError type with formatted multiline output showing full import chain and back-edge
- Updates existing tests to expect cycle errors instead of silent success, and adds comprehensive test coverage for 2-file, 4-file, and N-file cycles with deterministic output verification
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/parser/import_error.go | Defines ImportCycleError type and FormatImportCycleError function for user-friendly multiline error formatting |
| pkg/parser/import_processor.go | Enhances topologicalSortImports to detect cycles, adds findCyclePath and dfsForCycle functions for cycle path reconstruction with deterministic ordering |
| pkg/parser/import_cycle_test.go | Comprehensive test suite for cycle detection covering 2-file, 4-file, and multi-run deterministic scenarios, plus error formatting validation |
| pkg/workflow/compiler_orchestrator_engine.go | Integrates ImportCycleError handling with special formatting before returning to caller |
| pkg/workflow/imports_recursive_test.go | Updates TestCyclicImports to verify cycle detection errors instead of expecting silent success |
Comments suppressed due to low confidence (1)
pkg/parser/import_error.go:43
- The arrow symbol "→" used in the error message may not render consistently across all terminals and systems. Since the formatted error (FormatImportCycleError) uses "↳" for arrows, consider using a consistent symbol or ensuring both are ASCII-compatible for better cross-platform compatibility, especially in CI/CD environments that may have limited Unicode support.
return fmt.Sprintf("circular import detected: %s", strings.Join(e.Chain, " → "))
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| messageBuilder.WriteString(fmt.Sprintf("%s%s (starting point)\n", indent, file)) | ||
| } else if i == len(err.Chain)-1 { | ||
| // Last item is the back-edge - highlight it | ||
| messageBuilder.WriteString(fmt.Sprintf("%s↳ %s ⚠️ cycles back to %s\n", indent, file, err.Chain[0])) |
There was a problem hiding this comment.
The emoji character "
This issue also appears on line 43 of the same file.
| messageBuilder.WriteString(fmt.Sprintf("%s↳ %s ⚠️ cycles back to %s\n", indent, file, err.Chain[0])) | |
| messageBuilder.WriteString(fmt.Sprintf("%s↳ %s [!] cycles back to %s\n", indent, file, err.Chain[0])) |
|
|
||
| // Process imports from file A - should detect cycle | ||
| frontmatter := map[string]any{ | ||
| "imports": []string{"a.md"}, |
There was a problem hiding this comment.
The test creates a file A that imports B, and a file B that imports A, forming a cycle A→B→A. However, the frontmatter passed to ProcessImportsFromFrontmatterWithSource on line 43 uses "imports": []string{"a.md"}, which means it's importing A from A itself. This creates a self-import rather than testing the A→B→A cycle. The frontmatter should either be empty (testing from A's perspective) or should match what's actually in fileAContent (which imports B). Consider changing line 43 to use "imports": []string{"b.md"} to match the actual content of file A, or clarify the test intent.
| "imports": []string{"a.md"}, | |
| "imports": []string{"b.md"}, |
| for _, imp := range imports { | ||
| found := false | ||
| for _, processed := range result { | ||
| if processed == imp { | ||
| found = true | ||
| break | ||
| } | ||
| } | ||
| if !found { |
There was a problem hiding this comment.
The cycle node identification uses a nested loop with O(n*m) complexity where n is the number of imports and m is the number of processed results. This could be optimized by converting the result slice to a map for O(1) lookup, reducing complexity to O(n). Consider using a map[string]bool for faster lookups, especially for large import graphs.
| for _, imp := range imports { | |
| found := false | |
| for _, processed := range result { | |
| if processed == imp { | |
| found = true | |
| break | |
| } | |
| } | |
| if !found { | |
| // Build a set of processed imports for O(1) membership checks | |
| processedSet := make(map[string]bool, len(result)) | |
| for _, processed := range result { | |
| processedSet[processed] = true | |
| } | |
| for _, imp := range imports { | |
| if !processedSet[imp] { |
Import cycles were silently skipped during topological sort, forcing operators to debug blind. A 4-file cycle
A→B→C→D→Bwould compile successfully with incomplete output.Changes
Cycle detection
topologicalSortImportsto detect when Kahn's algorithm fails (all nodes have in-degree > 0)findCyclePathto construct full chain from cycle subgraphImportCycleErrorwith complete path including back-edgeError formatting
ImportCycleErrortype captures full chain:["b.md", "c.md", "d.md", "b.md"]FormatImportCycleErrorrenders multiline indented output with visual hierarchyTest coverage
TestCyclicImportsto expect errors instead of silent successExample output
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
https://api.github.com/graphql/usr/bin/gh /usr/bin/gh api graphql -f query=query($owner: String!, $name: String!) { repository(owner: $owner, name: $name) { hasDiscussionsEnabled } } -f owner=github -f name=gh-aw(http block)https://api.github.com/repos/actions/ai-inference/git/ref/tags/v1/usr/bin/gh gh api /repos/actions/ai-inference/git/ref/tags/v1 --jq .object.sha runs/20260218-131346-16603/test-254345567/.github/workflows -run /usr/bin/bash l ./pkg/workflow/ x_amd64/vet 0284250/b359/importcfg --no�� k/gh-aw/gh-aw/pkg/cli/access_log.go k/gh-aw/gh-aw/pkg/cli/actionlint.go /opt/hostedtoolcache/go/1.25.0/x64/pkg/tool/linux_amd64/vet g_.a --global /node /opt/hostedtoolcache/go/1.25.0/x64/pkg/tool/linux_amd64/vet(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v3/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v3 --jq .object.sha g_.a /tmp/go-build4099137013/b287/vet.cfg tartedAt,updatedAt,event,headBranch,headSha,displayTitle(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v4 --jq .object.sha ithub/workflows/agent-performance-analyzer.md scripts/**/*.js ache/go/1.25.0/x64/pkg/tool/linux_amd64/compile .prettierignore(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v4 --jq .object.sha echo "��� JavaScript files formatted"(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v4 --jq .object.sha user.name Test User /usr/bin/git on' --ignore-patgit(http block)https://api.github.com/repos/actions/checkout/git/ref/tags/v5/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha --noprofile(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha(http block)/usr/bin/gh gh api /repos/actions/checkout/git/ref/tags/v5 --jq .object.sha om/owner/repo.git rev-parse /usr/bin/git --noprofile .cfg x_amd64/cgo git conf�� user.email test@example.com /usr/bin/git ub/workflows --noheadings 0/x64/bin/sh git(http block)https://api.github.com/repos/actions/github-script/git/ref/tags/v8/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha --local pull.rebase ndor/bin/bash(http block)/usr/bin/gh gh api /repos/actions/github-script/git/ref/tags/v8 --jq .object.sha HEAD(http block)https://api.github.com/repos/actions/setup-go/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/setup-go/git/ref/tags/v4 --jq .object.sha --write scripts/**/*.js /usr/bin/bash .prettierignore(http block)https://api.github.com/repos/actions/setup-node/git/ref/tags/v4/usr/bin/gh gh api /repos/actions/setup-node/git/ref/tags/v4 --jq .object.sha 0284250/b123/_pkg_.a scripts/**/*.js 0284250/b123=> .prettierignore b/gh-aw/cmd/gh-arev-parse(http block)https://api.github.com/repos/github/gh-aw/actions/runs/1/artifacts/usr/bin/gh gh run download 1 --dir test-logs/run-1 9137013/b225/vet.cfg ode -n1 --format=format:rev-parse cf4abeacf031ce21--show-toplevel /usr/lib/systemd/systemd-executoconfig tion�� 69 --log-level x_amd64/link --log-target journal-or-kmsg -main/dist/ripgr--show-toplevel x_amd64/link(http block)https://api.github.com/repos/github/gh-aw/actions/runs/12345/artifacts/usr/bin/gh gh run download 12345 --dir test-logs/run-12345(http block)https://api.github.com/repos/github/gh-aw/actions/runs/12346/artifacts/usr/bin/gh gh run download 12346 --dir test-logs/run-12346(http block)https://api.github.com/repos/github/gh-aw/actions/runs/2/artifacts/usr/bin/gh gh run download 2 --dir test-logs/run-2 9137013/b223/vet.cfg x_amd64/compile -n1 --format=format:rev-parse cf4abeacf031 x_amd64/compile tion�� 9137013/b123/embedcfg(http block)https://api.github.com/repos/github/gh-aw/actions/runs/3/artifacts/usr/bin/gh gh run download 3 --dir test-logs/run-3(http block)https://api.github.com/repos/github/gh-aw/actions/runs/4/artifacts/usr/bin/gh gh run download 4 --dir test-logs/run-4 9137013/b224/vet.cfg ache/node/24.13.0/x64/bin/bash --merged cf4abeacf031ce21rev-parse(http block)https://api.github.com/repos/github/gh-aw/actions/runs/5/artifacts/usr/bin/gh gh run download 5 --dir test-logs/run-5 cf4abeacf031ce210136c5b52f2ecd56-w tions/setup/js/node_modules/.bin-buildmode=exe(http block)https://api.github.com/repos/github/gh-aw/actions/workflows/usr/bin/gh gh workflow list --json name,state,path w/js/**/*.json' --ignore-path origin x_amd64/vet(http block)/usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 100 er ./pkg/logger/slo/tmp/go-build2230284250/b377/_pkg_.a 64/pkg/tool/linu-trimpath --no�� th .prettierigno-p .cfg odules/npm/node_-lang=go1.25 ./pkg/logger/slogit %H %ct %D ./pkg/constants//tmp/gh-aw-test-runs/20260218-131346-16603/test-3341678157/.github/workflows ortcfg(http block)/usr/bin/gh gh run list --json databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle --workflow nonexistent-workflow-12345 --limit 6 gicalSortImportsrev-parse(http block)https://api.github.com/repos/github/gh-aw/git/ref/tags/v1.0.0/usr/bin/gh gh api /repos/github/gh-aw/git/ref/tags/v1.0.0 --jq .object.sha */*.json' '!../../../pkg/workflo-test.timeout=10m0s 9137013/b279/vet.cfg x_amd64/link(http block)https://api.github.com/repos/nonexistent/action/git/ref/tags/v999.999.999/usr/bin/gh gh api /repos/nonexistent/action/git/ref/tags/v999.999.999 --jq .object.sha */*.json' '!../../../pkg/workflo-p 9137013/b285/vet.cfg 64/pkg/tool/linux_amd64/link yBeforeUnloadChegit(http block)https://api.github.com/repos/nonexistent/repo/actions/runs/12345/usr/bin/gh gh run view 12345 --repo nonexistent/repo --json status,conclusion(http block)https://api.github.com/repos/owner/repo/actions/workflows/usr/bin/gh gh workflow list --json name,state,path --repo owner/repo 64/pkg/tool/linux_amd64/vet(http block)/usr/bin/gh gh workflow list --json name,state,path --repo owner/repo x_amd64/link(http block)https://api.github.com/repos/owner/repo/contents/file.md/tmp/go-build2230284250/b368/cli.test /tmp/go-build2230284250/b368/cli.test -test.testlogfile=/tmp/go-build2230284250/b368/testlog.txt -test.paniconexit0 -test.v=true -test.parallel=4 -test.timeout=10m0s -test.run=^Test -test.short=true g_adapter.go or_formatting_techeckout constants.go constants_test.go /pre�� = get && echo "******"; }; f get-errorsas = get && echo "******"; }; f get-ifaceassert 64/pkg/tool/linux_amd64/vet(http block)https://api.github.com/repos/test-owner/test-repo/actions/secrets/usr/bin/gh gh api /repos/test-owner/test-repo/actions/secrets --jq .secrets[].name set-url origin x_amd64/vet(http block)https://api.github.com/user/usr/bin/gh gh api user --jq .login(http block)/usr/bin/gh gh api user --jq .login g_.a --global x_amd64/vet(http block)Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.