Conversation
Contributor
|
@dsyme any way to add integrations tests for this in ci.yml? |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines import path detection to fix a bug where three-part relative paths (e.g., shared/mcp/arxiv.md) were incorrectly treated as workflowspecs. The fix simplifies the detection logic to only consider paths with @ version separators as workflowspecs, and adds comprehensive test coverage. Additionally, the PR introduces spinner support for git clone operations to improve user feedback during repository operations.
Changes:
- Simplified
isWorkflowSpecFormat()to strictly check for@version separator, fixing incorrect detection of three-part relative paths - Added comprehensive unit and integration tests for import path detection
- Introduced
RunGit()andRunGitCombined()helpers inpkg/workflow/git_helpers.gofor git operations with spinner feedback - Updated git clone operations in trial repository and package download flows to use spinner-enabled git helpers
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/cli/imports.go |
Simplified isWorkflowSpecFormat() to only check for @ separator, removing complex path part counting logic |
pkg/cli/imports_test.go |
Added comprehensive test coverage for import path detection including edge cases |
pkg/workflow/git_helpers.go |
Added RunGit() and RunGitCombined() helpers with spinner support for interactive terminals |
pkg/cli/trial_repository.go |
Updated clone operations to use workflow.RunGitCombined() for spinner feedback |
pkg/cli/packages.go |
Added manual spinner management for git clone operations (inconsistent with trial_repository.go approach) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
isWorkflowSpecFormat()to strictly check for workflow specifications using@version separatorKey Changes
@as workflow specificationsimports_test.go✨ PR Review Safe Output Test - Run 22075674041