Fix nested remote imports to resolve against parent workflowspec base path#16550
Fix nested remote imports to resolve against parent workflowspec base path#16550
Conversation
|
@copilot: clean paths, add logging, add tests |
…solution Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
@copilot validate full URLs and enterprise domains are supported |
…Path - Clean BasePath using path.Clean() to normalize paths (remove ./ and redundant slashes) - Add detailed logging for BasePath derivation with spec, owner, repo, ref - Add logging when no BasePath can be derived (file at repo root) - Clean basePath before use in nested import resolution - Add tests for path cleaning (./ components, redundant slashes, complex paths) - Add tests for URL-like paths (document current behavior) - Add test for enterprise domain workflowspecs - Fix BasePath derivation to include full directory path (not just parent dir) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in commit d1227cc:
Validated in commit d1227cc:
|
|
These are complementary fixes solving different problems: PR #16372 (
My PR (
Both needed:
They work at different layers and solve orthogonal problems. 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:
If you need me to access, download, or install something from one of these locations, you can either:
|
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where nested remote imports from workflowspecs outside .github/workflows/ were incorrectly resolved against a hardcoded .github/workflows/ base path instead of the parent workflowspec's actual base directory. This caused cache path inconsistencies and security scanner warnings for non-existent files.
Changes:
- Added
BasePathfield toremoteImportOriginto track the parent workflowspec's base directory path - Updated
parseRemoteOrigin()to deriveBasePathfrom the workflowspec by extracting the directory path containing the file - Modified nested import resolution logic to use the parent's
BasePathinstead of hardcoded.github/workflows/, with fallback for backward compatibility - Added comprehensive test coverage including path cleaning, edge cases, and documentation of URL-like path behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pkg/parser/import_processor.go | Added BasePath field to remoteImportOrigin struct, implemented BasePath derivation in parseRemoteOrigin(), and updated nested import resolution to use parent's BasePath instead of hardcoded .github/workflows/ |
| pkg/parser/import_remote_nested_test.go | Updated all existing tests to verify BasePath field, added new tests for path cleaning scenarios (./components, redundant slashes), URL-like path behavior documentation, and enterprise domain support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| // Derive BasePath: everything between owner/repo and the last component (filename) | ||
| // Since imports are always 2-level (dir/file.md), the base is everything before the filename |
There was a problem hiding this comment.
The comment "Since imports are always 2-level (dir/file.md)" is inaccurate. The test cases demonstrate that imports can have arbitrary depth (e.g., "owner/repo/a/b/c/d/e/file.md" has BasePath "a/b/c/d/e" which is 5 levels deep). The comment should be updated to reflect that the BasePath is the directory path containing the file, which can have any number of levels.
| // Since imports are always 2-level (dir/file.md), the base is everything before the filename | |
| // Imports can have arbitrary directory depth; BasePath is the directory path containing the file |
Nested relative imports from remote workflowspecs outside
.github/workflows/were resolving against a hardcoded.github/workflows/base instead of the parent's actual base directory. This caused cache path inconsistencies and security scanner warnings for non-existent files.Example:
Changes:
BasePathfield toremoteImportOrigintracking the parent workflowspec's base directoryparseRemoteOrigin()to derive base path from workflowspec (full directory path containing the file)BasePathinstead of hardcoded.github/workflows/.github/workflowswhenBasePathis empty (backward compatibility)path.Clean()to normalize BasePath (removes./components and redundant slashes)Path Cleaning:
Paths are normalized using
path.Clean()to handle edge cases:Enterprise Domain Support:
Enterprise GitHub instances use the same
owner/repo/pathworkflowspec format. The GitHub domain is handled by theGH_HOSTenvironment variable, not in the workflowspec itself.Testing:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.