fix(trial): fetch entire .github folder to include runtime-import dependencies - #48787
Conversation
…endencies
When `gh aw trial` installs a remote workflow, it only fetched individual
declared dependencies (frontmatter `imports:`, `@include` directives,
`dispatch-workflow`, etc.) but missed files referenced by
`{{#runtime-import ...}}` macros in the workflow body (e.g. skills under
`.github/skills/`).
Add `copySourceRepoGitHubFolder` which performs a sparse git checkout of
the entire `.github/` directory from the source repository at the exact
same ref as the workflow and merges it into the trial host directory.
Existing files (e.g. the trial-modified workflow with `source:` frontmatter
already injected) are preserved via a skip-if-exists strategy in the new
`mergeDirectory` helper.
`fetchAllRemoteDependencies` is kept to handle cross-repo `@include`,
dispatch-workflow, call-workflow, and resources dependencies.
Closes #47147
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
TriageCategory: bug (trial fix) | Risk: medium | Score: 48/100
Recommended action:
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ Test Quality Sentinel completed test quality analysis. |
There was a problem hiding this comment.
Pull request overview
Adds runtime-import dependencies to remote trial installations by copying source .github/ content.
Changes:
- Adds sparse checkout and directory merge logic.
- Adds merge behavior tests.
Show a summary per file
| File | Description |
|---|---|
pkg/cli/trial_repository.go |
Fetches and merges source .github/ content. |
pkg/cli/trial_repository_test.go |
Tests recursive merge behavior. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (4)
pkg/cli/trial_repository.go:658
- This remote URL is used by the later
git fetchcommands without any credential injection. The workflow itself can be downloaded from a private source through the authenticated GitHub API, but these plain HTTPS fetches then fail unless the user happened to configure a global Git credential helper. Obtain the credential forspec.Hostand pass it per command (without persisting it), or use the authenticated API/archive path.
repoURL := fmt.Sprintf("%s/%s.git", githubHost, spec.RepoSlug)
pkg/cli/trial_repository.go:322
- The fallback below does not fetch runtime imports, so treating this failure as non-fatal reintroduces the reported bug: trial installation succeeds, but activation later fails with a missing runtime-import file. Return this error immediately (and do so even when verbose mode is off) rather than silently continuing.
if err := copySourceRepoGitHubFolder(ctx, parsedSpec, tempDir, opts.Verbose); err != nil {
if opts.Verbose {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to copy .github folder from source: %v", err)))
}
// Non-fatal: fall through to individual dependency fetching below
}
pkg/cli/trial_repository.go:755
- Skipping every destination file preserves stale runtime imports from previous trial installations. When the source ref advances, an existing
.github/skills/.../SKILL.mdremains unchanged and compilation no longer represents the selected source revision. Preserve only files intentionally modified by trial installation (such as the installed workflow), and overwrite dependency files from the pinned source revision.
// Skip files that already exist in the destination (preserve trial-modified versions)
if _, statErr := os.Stat(dstPath); statErr == nil {
return nil
}
pkg/cli/trial_repository.go:757
WalkDirreports repository symlinks as non-directories, andCopyFileopens them normally, which dereferences the link. A source repository can therefore place a symlink such as.github/skills/x/SKILL.md -> /path/on/the-user-machineand cause local file contents to be copied and pushed to the trial host. Reject source symlinks (or copy links without dereferencing them) before callingCopyFile.
return fileutil.CopyFile(srcPath, dstPath)
- Files reviewed: 2/2 changed files
- Comments generated: 5
- Review effort level: Medium
| // Copy the entire .github/ folder from the source repo so that runtime-import | ||
| // files (e.g. .github/skills/*/SKILL.md) and other local dependencies are | ||
| // present in the trial host before compilation. Files already written to tempDir | ||
| // (such as the trial-modified workflow) are preserved. |
| // files (e.g. .github/skills/*/SKILL.md) and other local dependencies are | ||
| // present in the trial host before compilation. Files already written to tempDir | ||
| // (such as the trial-modified workflow) are preserved. | ||
| if err := copySourceRepoGitHubFolder(ctx, parsedSpec, tempDir, opts.Verbose); err != nil { |
| } | ||
| defer os.RemoveAll(tmpDir) | ||
|
|
||
| githubHost := getGitHubHostForRepo(spec.RepoSlug) |
| if _, statErr := os.Stat(dstPath); statErr == nil { | ||
| return nil | ||
| } |
| // For commit SHAs: attempt direct SHA fetch; fall back to full shallow fetch | ||
| fetchCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "fetch", "--depth", "1", "origin", ref) | ||
| if _, err := fetchCmd.CombinedOutput(); err != nil { | ||
| fetchCmd = exec.CommandContext(ctx, "git", "-C", tmpDir, "fetch", "--depth", "1", "origin") |
![github-actions[bot]](https://cdn.github.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(trial): fetch entire .github folder to include runtime-import dependencies
The fix is well-targeted and the sparse checkout + merge-skip-existing approach is correct. One non-blocking suggestion:
Silent failure on .github copy error — when copySourceRepoGitHubFolder errors and opts.Verbose is false, the failure is silently dropped. The user then sees the original Runtime import file not found error with no preceding hint. A non-verbose warning on stderr would improve debuggability. See inline comment.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 21.8 AIC · ⌖ 5.32 AIC · ⊞ 5K
| // (such as the trial-modified workflow) are preserved. | ||
| if err := copySourceRepoGitHubFolder(ctx, parsedSpec, tempDir, opts.Verbose); err != nil { | ||
| if opts.Verbose { | ||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to copy .github folder from source: %v", err))) |
There was a problem hiding this comment.
Non-fatal failure is silent by default.
When copySourceRepoGitHubFolder fails and opts.Verbose is false, the error is completely swallowed. The user then hits the original Runtime import file not found error with no indication that the .github folder copy failed earlier. Consider always emitting a warning on stderr, gating only the detailed error string on verbose:
fmt.Fprintln(os.Stderr, console.FormatWarningMessage("Failed to copy .github folder from source; runtime-import dependencies may be missing"))
if opts.Verbose {
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(err.Error()))
}@copilot please address this.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on two correctness issues and a test coverage gap.
📋 Key Themes & Highlights
Key Themes
- SHA fallback is unreliable — the fallback
git fetch --depth 1 origin(no ref) fetches only default-branch HEAD; if the pinned SHA is not in that history, checkout still fails, and the error is silently swallowed, reproducing the original bug. - Silent failure on copy error — the non-fatal swallowing of
copySourceRepoGitHubFoldererrors is guarded behind--verbose, so users get no indication when the step fails. - No tests for
copySourceRepoGitHubFolder— the git strategy logic (SHA vs branch, fallback) has zero test coverage;mergeDirectoryis well tested.
Positive Highlights
- ✅ Sparse checkout strategy is well-chosen — saves bandwidth by fetching only
.github/ - ✅
mergeDirectoryis thoroughly tested with 5 cases covering preservation, nesting, and edge cases - ✅
fileutil.ValidatePathWithinBasepath-traversal guard is exactly right here - ✅ Non-fatal design in
installWorkflowInTrialModeis appropriate — fall-through to individual dependency fetching is a good safety net
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 45.5 AIC · ⌖ 5.01 AIC · ⊞ 6.7K
Comment /matt to run again
| // For commit SHAs: attempt direct SHA fetch; fall back to full shallow fetch | ||
| fetchCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "fetch", "--depth", "1", "origin", ref) | ||
| if _, err := fetchCmd.CombinedOutput(); err != nil { | ||
| fetchCmd = exec.CommandContext(ctx, "git", "-C", tmpDir, "fetch", "--depth", "1", "origin") |
There was a problem hiding this comment.
[/diagnosing-bugs] The SHA fallback fetch --depth 1 origin (no ref) fetches only default-branch HEAD — if the target SHA is not in that history, the subsequent checkout <SHA> (line 694) will still fail. That error is silently swallowed by the non-fatal caller, so trial mode silently uses stale/wrong files rather than surfacing the failure.
💡 Suggested fix
Either remove the fallback and propagate the error, or use an unshallowed fetch:
// Unshallowed fetch gives git access to the full history and the SHA
fetchCmd = exec.CommandContext(ctx, "git", "-C", tmpDir, "fetch", "--unshallow", "origin")Propagating the error is safer: the non-fatal wrapper in installWorkflowInTrialMode already logs it when --verbose is set, and then falls back to individual dependency fetching.
@copilot please address this.
| // files (e.g. .github/skills/*/SKILL.md) and other local dependencies are | ||
| // present in the trial host before compilation. Files already written to tempDir | ||
| // (such as the trial-modified workflow) are preserved. | ||
| if err := copySourceRepoGitHubFolder(ctx, parsedSpec, tempDir, opts.Verbose); err != nil { |
There was a problem hiding this comment.
[/diagnosing-bugs] The copySourceRepoGitHubFolder error is swallowed silently unless --verbose is set. If the fetch fails (e.g. private repo, network issue, bad ref), trial mode will proceed without the .github/ files, reproduce the original Runtime import file not found bug, and give the user no indication that the copy step failed.
💡 Suggested improvement
Always print a warning (not just under --verbose), or surface the error level so users see it without needing to re-run with --verbose:
if err := copySourceRepoGitHubFolder(ctx, parsedSpec, tempDir, opts.Verbose); err != nil {
// Always warn, not just in verbose mode
fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to copy .github folder from source: %v", err)))
// Non-fatal: fall through
}@copilot please address this.
| } | ||
|
|
||
| // Fetch and checkout at the exact ref used by the workflow | ||
| isSHA := len(ref) == 40 && gitutil.IsHexString(ref) |
There was a problem hiding this comment.
[/tdd] copySourceRepoGitHubFolder has no unit tests — only mergeDirectory is covered. The git fetch/checkout branching logic (SHA vs branch, fallback path) is the highest-risk code and also the hardest to debug in CI.
💡 What to test
Add at least one test for the non-local path in installWorkflowInTrialMode or extract the fetch strategy to a testable function. At minimum, consider testing:
- ref is a 40-char SHA → uses SHA fetch path
- ref is a branch name → uses branch fetch path
spec.RepoSlugis empty → returns nil immediately (this is the easiest to add)
The empty-slug early-return is trivially testable and would anchor the function in the test suite:
func TestCopySourceRepoGitHubFolder_EmptySlug(t *testing.T) {
err := copySourceRepoGitHubFolder(context.Background(), &WorkflowSpec{}, t.TempDir(), false)
if err != nil {
t.Fatalf("expected nil for empty RepoSlug, got %v", err)
}
}@copilot please address this.
🧪 Test Quality Sentinel Report✅ Test Quality Score: 100/100 — Excellent
📊 Metrics (5 tests)
Quality Highlights✨ Excellent test design:
Verdict
|
There was a problem hiding this comment.
Verdict: Request changes
Correctness bug in SHA-pinned dependency fetching undermines this fix's primary use case; error visibility is also too weak.
Themes
- The SHA-ref fallback path (
git fetch --depth 1 originwithout a refspec) does not reliably make an arbitrary historical commit checkoutable — it only fetches the current branch tip at depth 1. Workflows pinned to older commit SHAs (a common supply-chain-pinning pattern, and exactly the scenariogh aw trialneeds to support) will silently fail this new step and fall back to the pre-existing per-file dependency fetch, which is the bug this PR is fixing. Worth verifying with an integration test against an older SHA. - Failures are only surfaced with
--verbose; by default users get no signal that.githubcopy failed, making regressions of this exact bug hard to notice. - Positive: sparse-checkout config and directory-merge logic follow the existing
downloadWorkflowContentViaGitClonepattern in the codebase, andmergeDirectorycorrectly guards against path traversal viafileutil.ValidatePathWithinBase. Test coverage formergeDirectoryis solid.
🔎 Code quality review by PR Code Quality Reviewer · sonnet50 · 51.4 AIC · ⌖ 4.36 AIC · ⊞ 7.4K
Comment /review to run again
| } | ||
| checkoutCmd := exec.CommandContext(ctx, "git", "-C", tmpDir, "checkout", ref) | ||
| if out, err := checkoutCmd.CombinedOutput(); err != nil { | ||
| return fmt.Errorf("failed to checkout commit %s: %w\nOutput: %s", ref, err, string(out)) |
There was a problem hiding this comment.
SHA-ref fallback only fetches whatever the default branch tip currently points to at depth 1 — it will not make an arbitrary historical/pinned commit reachable, so git checkout ref fails for typical supply-chain-pinned SHAs.
| // present in the trial host before compilation. Files already written to tempDir | ||
| // (such as the trial-modified workflow) are preserved. | ||
| if err := copySourceRepoGitHubFolder(ctx, parsedSpec, tempDir, opts.Verbose); err != nil { | ||
| if opts.Verbose { |
There was a problem hiding this comment.
A failed .github folder copy (e.g. private repo auth, bad ref, network error) is silently swallowed unless --verbose is passed, so users see no signal that runtime-import dependencies were not fetched — the exact failure mode this PR is meant to fix.
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (289 new lines in 📄 Draft ADR committed:
📋 What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. ❓ Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
gh aw trialwas fetching individually declared dependencies (frontmatterimports:,@include, dispatch/call workflows, resources) but silently skipping files referenced by{{#runtime-import ...}}macros in the workflow body. Trial activation would reach template interpolation and fail withRuntime import file not found.Changes
copySourceRepoGitHubFolder— sparse-checks out the entire.github/directory from the source repo at the exact same commit/ref as the workflow, then merges it into the trial host. Handles both SHA and branch/tag refs using the same fetch strategy asdownloadWorkflowContentViaGitClone.mergeDirectory— merge helper that copies files from source to destination but skips existing files, preserving trial-modified versions (e.g. the workflow file already hassource:frontmatter injected before this step runs).installWorkflowInTrialModecallscopySourceRepoGitHubFolderbeforefetchAllRemoteDependenciesfor all non-local workflows.fetchAllRemoteDependenciesis retained for cross-repo@include, dispatch, and resource dependencies.The effect: a workflow like this now gets its
.github/skills/dependency fetched automatically during trial install: