fix: trial command missing remote dependency fetching for imports#23165
fix: trial command missing remote dependency fetching for imports#23165
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…spatch workflows, and resources The trial command only called fetchAndSaveRemoteIncludes but not fetchAndSaveRemoteFrontmatterImports, fetchAndSaveRemoteDispatchWorkflows, or fetchAndSaveRemoteResources. This caused 'import file not found' errors when running 'gh aw trial' with workflows that use frontmatter imports (e.g. shared/formatting.md), while 'gh aw add' worked fine. Extract a shared fetchAllRemoteDependencies function in includes.go that both add_command.go and trial_repository.go now call, ensuring parity and preventing future drift.
There was a problem hiding this comment.
Pull request overview
Fixes gh aw trial failing with import file not found for workflows that use frontmatter imports: by aligning trial-mode dependency fetching with the add command’s behavior.
Changes:
- Added a shared
fetchAllRemoteDependencies()helper to fetch includes, frontmatter imports, dispatch-workflow references, and resources. - Updated
addto call the shared helper instead of duplicating dependency-fetch logic inline. - Updated trial install flow to call the shared helper so remote workflows fetch the same dependency types as
add.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/cli/includes.go | Introduces fetchAllRemoteDependencies() to centralize remote dependency fetching. |
| pkg/cli/add_command.go | Replaces inline dependency-fetch calls with fetchAllRemoteDependencies(). |
| pkg/cli/trial_repository.go | Uses fetchAllRemoteDependencies() in trial installs to prevent dependency-fetch drift. |
| actions/setup/js/extra_empty_commit.cjs | Improves logging for git fetch/reset failures by including the underlying error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const syncErrorMessage = error instanceof Error ? error.message : String(error); | ||
| core.warning(`Could not sync local branch with remote ${branchName} - will attempt push with local HEAD. Underlying error: ${syncErrorMessage}`); | ||
| } |
There was a problem hiding this comment.
The warning logs the underlying git fetch/reset error message while the temporary remote URL contains the auth token (https://x-access-token:@...). Some git/exec errors can include the remote URL or other sensitive output, which would risk leaking the token into Actions logs. Please redact the token/remote URL from the error before logging, or avoid logging the raw error message here (e.g., log a generic warning and only include sanitized details).
| // fetchAllRemoteDependencies fetches all remote dependencies for a workflow: | ||
| // includes (@include directives), frontmatter imports, dispatch workflows, and resources. | ||
| // This is the single entry point shared by both the add and trial commands. | ||
| func fetchAllRemoteDependencies(ctx context.Context, content string, spec *WorkflowSpec, targetDir string, verbose bool, force bool, tracker *FileTracker) error { | ||
| // Fetch and save @include directive dependencies | ||
| if err := fetchAndSaveRemoteIncludes(content, spec, targetDir, verbose, force, tracker); err != nil { | ||
| if verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch include dependencies: %v", err))) | ||
| } | ||
| } |
There was a problem hiding this comment.
The docstring says this function “fetches all remote dependencies”, but the implementation intentionally swallows errors from includes and frontmatter imports (only emitting a warning when verbose). This mismatch can confuse callers (e.g., trial wraps the returned error as if all dependency fetch failures would surface). Either update the comment/name to make the best-effort semantics explicit, or change the function to propagate/aggregate include/import errors when they occur.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
The test was timing out at 10s default. Increase to 30s to handle slow API responses.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal!
💥 [THE END] — Illustrated by Smoke Claude
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to fetch resource dependencies: %v", err))) | ||
| } | ||
| return fmt.Errorf("failed to fetch resource dependencies: %w", err) | ||
| } |
There was a problem hiding this comment.
This function provides a clean, unified entry point for all remote dependency fetching. Smoke test review comment — Run 23617381060.
| if err := fetchAllRemoteDependencies(ctx, string(content), parsedSpec, result.WorkflowsDir, opts.Verbose, true, nil); err != nil { | ||
| return fmt.Errorf("failed to fetch remote dependencies: %w", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Good refactor — using fetchAllRemoteDependencies here ensures trial mode stays in sync with add command parity. Smoke test review comment — Run 23617381060.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@copilot merge main and recompile |
…e-imports # Conflicts: # actions/setup/js/extra_empty_commit.cjs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done — merged main (resolved conflict in 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:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Hey
If you'd like a hand, you can assign this prompt to your coding agent:
|
|
@copilot review comments, add tests |
…tring and add tests Agent-Logs-Url: https://github.com/github/gh-aw/sessions/78861777-a15b-4057-82d1-a28adf6deaba Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Addressed the review comments and added tests in
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:
|
githubnext/agentics#266
Problem
gh aw trialfails withimport file not founderrors when the workflow uses frontmatterimports:(e.g.shared/formatting.md), while the equivalentgh aw addworks fine.Root Cause
installWorkflowInTrialModeintrial_repository.goonly calledfetchAndSaveRemoteIncludes(for@includedirectives in the markdown body) but was missing three other dependency fetchers thatAddWorkflowsinadd_command.gocalled:fetchAndSaveRemoteFrontmatterImports— frontmatterimports:fieldfetchAndSaveRemoteDispatchWorkflows—safe-outputs.dispatch-workflowreferencesfetchAndSaveRemoteResources—resources:frontmatter fieldFix
Extract a shared
fetchAllRemoteDependencies()function inincludes.gothat bothadd_command.goandtrial_repository.gonow call. This ensures parity between the two code paths and prevents future drift when new dependency types are added.Changes
pkg/cli/includes.go: NewfetchAllRemoteDependencies()functionpkg/cli/add_command.go: Replaced inline dependency fetching with shared functionpkg/cli/trial_repository.go: Replaced incomplete dependency fetching with shared function✨ PR Review Safe Output Test - Run 23617381060