fix: follow symlinks when generating sparse checkout for .github subdirs#43909
Conversation
… dirs Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…xtraPaths Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ Test Quality Sentinel completed test quality analysis. |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a sparse-checkout edge case in activation-job generation: when well-known .github/* directories (agents/skills/prompts) are symlinks that point elsewhere in the repo (e.g. ../.ai/agents), the generated sparse checkout should include the symlink target directory so runtime imports don’t break.
Changes:
- Add
resolveSymlinkExtraPathsto detect symlinks under.github/and append their resolved repo-relative targets to the activation sparse-checkout extra paths. - Call
resolveSymlinkExtraPathsduring activation checkout step generation. - Add unit tests for symlink resolution, escape handling, missing paths, and deduplication.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/compiler_activation_job.go |
Adds symlink target discovery and appends resolved paths to activation-job sparse checkout. |
pkg/workflow/compiler_activation_job_test.go |
Adds tests validating symlink resolution behavior and deduplication. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 5
- Review effort level: Low
| // Resolve relative to the parent directory of the symlink | ||
| resolved := filepath.Clean(filepath.Join(filepath.Dir(candidate), target)) | ||
| // Reject paths that escape the repository root (e.g. ../../etc) | ||
| if strings.HasPrefix(resolved, "..") { | ||
| compilerActivationJobLog.Printf("Ignoring symlink %s -> %s: resolved path %q escapes the repository root", candidate, target, resolved) | ||
| continue | ||
| } |
| for _, candidate := range candidates { | ||
| info, err := os.Lstat(candidate) | ||
| if err != nil || info.Mode()&os.ModeSymlink == 0 { | ||
| continue // not a symlink or doesn't exist |
ADR generated by design-decision-gate to document the architectural decision made in PR #43909 to follow symlinks when generating sparse checkout for .github subdirectories. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (115 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
|
🧪 Test Quality Sentinel Report✅ Test Quality Score: 94/100 — Excellent
📊 Metrics (5 scenarios in 1 function)
Verdict
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "awmgmcpg"See Network Configuration for more information.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
\u2705 Test Quality Sentinel: 94/100. 0% implementation tests (threshold: 30%). All 5 new test scenarios are high-value design tests covering the core symlink-resolution behavioral contract, including a security boundary (escape detection) and idempotency invariant.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on a correctness gap in the escape check and test fragility.
📋 Key Themes & Highlights
Key Themes
-
Absolute symlink targets not handled (line 567): Go's
filepath.Joindoes not treat absolute second args as absolute — an absolute symlink target like/etc/passwdresolves to.github/etc/passwd, passes the".."escape check, and gets added to sparse checkout as a wrong path. Afilepath.IsAbs(target)guard is needed before the join. -
Escape-root check is fragile (line 569):
strings.HasPrefix(resolved, "..")misses the edge case whereresolved == ".."exactly (whichfilepath.Cleanproduces for a one-level escape).resolved == ".." || strings.HasPrefix(resolved, "../")is more precise. -
CWD-implicit behaviour (line 558):
os.Lstat(candidate)silently skips all candidates if the process CWD is not the repo root. Accepting arepoRootparameter would make this explicit and testable withoutos.Chdir. -
Test fragility (line 1150):
os.Chdiris process-global; the single parent-level cleanup is not resilient to sub-test panics. Per-sub-test cleanups (ort.Chdirin Go 1.24+) are safer.
Positive Highlights
- ✅ Root cause is correctly identified and the fix is precise:
os.Lstat/os.Readlinkrather than blindly resolving every path. - ✅ Deduplication via the
existingmap is clean and efficient. - ✅ Five test cases cover the main scenarios well; the test structure is clear.
- ✅ Logging at each decision point makes debugging future failures straightforward.
- ✅ Candidates list is kept small and explicit — easy to extend.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 90.5 AIC · ⌖ 8.39 AIC · ⊞ 6.7K
Comment /matt to run again
| continue | ||
| } | ||
| // Resolve relative to the parent directory of the symlink | ||
| resolved := filepath.Clean(filepath.Join(filepath.Dir(candidate), target)) |
There was a problem hiding this comment.
[/diagnosing-bugs] Absolute symlink targets are silently mishandled — filepath.Join(".github", "/abs/path") returns .github/abs/path in Go (it does not honour the absolute second arg), so the strings.HasPrefix(resolved, "..") escape check passes and an incorrect path gets appended to the sparse checkout instead of resolving the actual dangling symlink.
💡 Suggested fix
Add an explicit guard for absolute targets before the join:
if filepath.IsAbs(target) {
compilerActivationJobLog.Printf("Ignoring symlink %s -> %s: absolute target not supported", candidate, target)
continue
}
resolved := filepath.Clean(filepath.Join(filepath.Dir(candidate), target))Most git symlinks use relative targets for portability, so rejecting absolute ones is the safest default.
@copilot please address this.
| existing[p] = struct{}{} | ||
| } | ||
| for _, candidate := range candidates { | ||
| info, err := os.Lstat(candidate) |
There was a problem hiding this comment.
[/diagnosing-bugs] os.Lstat(candidate) uses a relative path, so it is implicitly CWD-dependent — the function works only if the compiler process is already at the repo root. If cwd ever differs (e.g., during testing or if the compiler is invoked from a sub-directory), all candidates are silently skipped rather than raising an error.
💡 Suggested improvement
Accept a repoRoot string parameter (or derive it from the caller) and use filepath.Join(repoRoot, candidate) for all os.Lstat/os.Readlink calls. This makes the dependency explicit and easier to test without os.Chdir:
func resolveSymlinkExtraPaths(repoRoot string, extraPaths []string) []string {
...
info, err := os.Lstat(filepath.Join(repoRoot, candidate))
...
target, err := os.Readlink(filepath.Join(repoRoot, candidate))
...
}@copilot please address this.
| require.NoError(t, os.MkdirAll(filepath.Join(dir, ".ai", "agents"), 0o755)) | ||
| // Create .github/agents as a symlink -> ../.ai/agents | ||
| require.NoError(t, os.MkdirAll(filepath.Join(dir, ".github"), 0o755)) | ||
| require.NoError(t, os.Symlink(filepath.Join("..", ".ai", "agents"), filepath.Join(dir, ".github", "agents"))) |
There was a problem hiding this comment.
[/tdd] The "symlink resolved" test only asserts .ai/agents is in the result, but does not assert the full slice length. If a second, unexpected path were also added (e.g., due to a bug), the test would still pass.
💡 Suggested addition
Add a length check after the Contains assertion to make the test a precise specification:
result := resolveSymlinkExtraPaths(nil)
assert.Contains(t, result, ".ai/agents", "symlink target should be appended")
assert.Len(t, result, 1, "only the resolved target should be in the result")@copilot please address this.
| // Resolve relative to the parent directory of the symlink | ||
| resolved := filepath.Clean(filepath.Join(filepath.Dir(candidate), target)) | ||
| // Reject paths that escape the repository root (e.g. ../../etc) | ||
| if strings.HasPrefix(resolved, "..") { |
There was a problem hiding this comment.
[/diagnosing-bugs] The escape-root check strings.HasPrefix(resolved, "..") is fragile — it only catches paths that literally begin with .., but Windows-style paths or normalisation edge cases (e.g., after clean) could produce paths like ../foo on some hosts but just foo on others. More importantly, a symlink target of exactly .. (resolves to .) would pass the check and add . to the sparse checkout list, which is likely unintended.
💡 Suggested improvement
Use a more robust check: verify the resolved path does not escape the current working directory by confirming it is within a known root:
// Use strings.HasPrefix with slash to avoid false negatives on paths like "..foo"
if resolved == ".." || strings.HasPrefix(resolved, "../") {
...
continue
}Or if repoRoot is passed as a parameter, use filepath.Rel to confirm containment.
@copilot please address this.
There was a problem hiding this comment.
Two correctness issues block merge
The symlink-resolution idea is sound, but two problems need to be fixed.
1. Implicit CWD contract (high) — resolveSymlinkExtraPaths calls os.Lstat(".github/agents") with a bare relative path, silently no-oppping whenever the process CWD is not the repo root. The tests reveal this by calling os.Chdir before every case. The function should accept the repo root as an explicit parameter.
2. Absolute symlink targets bypass the escape guard (medium) — strings.HasPrefix(resolved, "..") only stops relative traversal. When a symlink target is an absolute path, Go silently strips the leading / in filepath.Join, so the guard passes and a wrong path enters the sparse-checkout list. Add filepath.IsAbs(resolved) to the rejection condition.
🔎 Code quality review by PR Code Quality Reviewer · 158.5 AIC · ⌖ 5.43 AIC · ⊞ 5.5K
Comment /review to run again
| for _, p := range extraPaths { | ||
| existing[p] = struct{}{} | ||
| } | ||
| for _, candidate := range candidates { |
There was a problem hiding this comment.
Silent no-op when CWD ≠ repo root: os.Lstat(candidate) resolves the bare relative path against the process working directory. If the compiler is invoked from any directory other than the repo root, symlink detection silently skips every candidate and the fix never fires.
💡 Suggested fix
Pass the repo root as an explicit parameter instead of relying on CWD:
func resolveSymlinkExtraPaths(repoRoot string, extraPaths []string) []string {
// ...
for _, candidate := range candidates {
fullPath := filepath.Join(repoRoot, candidate)
info, err := os.Lstat(fullPath)
if err != nil || info.Mode()&os.ModeSymlink == 0 {
continue
}
target, err := os.Readlink(fullPath)
if err != nil {
continue
}
resolved := filepath.Clean(filepath.Join(filepath.Dir(fullPath), target))
// Reject anything that escapes repoRoot
if !strings.HasPrefix(resolved, filepath.Clean(repoRoot)+string(os.PathSeparator)) {
continue
}
rel, _ := filepath.Rel(repoRoot, resolved)
// ...
}
}The caller already has access to the repo path; the hidden CWD contract makes the function fragile in library/test/embedded invocations. The tests themselves are forced to call os.Chdir as a workaround, which is the clearest sign this contract needs to be explicit.
| target, err := os.Readlink(candidate) | ||
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Absolute symlink target bypasses escape check: strings.HasPrefix(resolved, "..") only guards against relative traversal. If the symlink points to an absolute path (e.g. /data/shared-agents), Go's filepath.Join(".github", "/data/shared-agents") produces .github/data/shared-agents — it silently strips the leading / — so the resolved path doesn't start with .." and passes the guard, appending a meaningless path to extraPaths`.
💡 Suggested fix
Tighten the guard to cover both relative and absolute escape:
// After resolving:
resolved := filepath.Clean(filepath.Join(filepath.Dir(candidate), target))
// Reject absolute paths and relative-traversal paths
if filepath.IsAbs(resolved) || strings.HasPrefix(resolved, "..") {
compilerActivationJobLog.Printf("Ignoring symlink %s -> %s: resolved path %q is outside the repository root", candidate, target, resolved)
continue
}Note: even without this fix the impact is low (a wrong path in sparse-checkout is a non-operation), but it's an inconsistency between the documented invariant ("rejects paths that escape the root") and the actual behavior.
| // TestResolveSymlinkExtraPaths verifies that symlinks under .github/ are resolved and | ||
| // their targets appended to the extraPaths list, while non-symlinks and escape paths are ignored. | ||
| func TestResolveSymlinkExtraPaths(t *testing.T) { | ||
| origDir, err := os.Getwd() |
There was a problem hiding this comment.
Use t.Chdir instead of manual os.Chdir/t.Cleanup: the manual save/restore pattern is process-global and unsafe if any test in the package ever adopts t.Parallel(). Each subtest calls os.Chdir(dir) with no mutual exclusion between subtests.
💡 Suggested fix
t.Chdir (Go 1.24+) handles per-subtest cleanup automatically and is already the established pattern in this package (see compiler_shared_cache_test.go and resolve_test.go):
// Replace this:
origDir, err := os.Getwd()
require.NoError(t, err)
t.Cleanup(func() { _ = os.Chdir(origDir) })
// ...
require.NoError(t, os.Chdir(dir))
// With this (in each subtest):
t.Chdir(dir)This is a low-effort change that removes the fragile manual pattern and aligns with the rest of the test file.
There was a problem hiding this comment.
Review: fix: follow symlinks when generating sparse checkout for .github subdirs
The fix correctly solves the dangling-symlink problem by resolving .github/agents, .github/skills, and .github/prompts symlinks and adding their targets to extraPaths before the sparse-checkout step is generated. The approach is sound and well-tested.
One blocking issue was found in the escape guard:
Absolute symlink targets bypass the ..-prefix guard
Go's filepath.Join(base, absPath) on Linux does not discard base when absPath is absolute. For example:
filepath.Join(".github", "/etc/passwd") → ".github/etc/passwd"
That resolved path doesn't start with .., so the guard on line 569 silently passes and ".github/etc/passwd" is appended to extraPaths. This is practically harmless (the path won't exist in the checkout), but it breaks the documented security contract.
Fix: add filepath.IsAbs(resolved) to the guard, and add a test case with an absolute-path symlink target.
See inline comment for the exact change needed.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
proxy.golang.org
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 99.5 AIC · ⌖ 6.24 AIC · ⊞ 4.9K
| // Resolve relative to the parent directory of the symlink | ||
| resolved := filepath.Clean(filepath.Join(filepath.Dir(candidate), target)) | ||
| // Reject paths that escape the repository root (e.g. ../../etc) | ||
| if strings.HasPrefix(resolved, "..") { |
There was a problem hiding this comment.
Absolute symlink targets bypass the escape guard.
Go's filepath.Join does not discard the base when the second argument is an absolute path on Linux: filepath.Join(".github", "/etc/passwd") → ".github/etc/passwd". That resolved path does not start with .., so the guard on this line passes silently, and ".github/etc/passwd" would be appended to extraPaths.
This is benign in practice (the path just won't exist in the sparse checkout), but it violates the security contract the comment on line 568 promises.
Suggested fix:
if filepath.IsAbs(resolved) || strings.HasPrefix(resolved, "..") {
compilerActivationJobLog.Printf("Ignoring symlink %s -> %s: resolved path %q is unsafe", candidate, target, resolved)
continue
}Also add a test case that uses an absolute-path symlink target (e.g. os.Symlink("/etc/passwd", ...)) to cover this branch.
@copilot please address this.
🤖 PR Triage — §28848998303
Fix: follow symlinks for sparse checkout of
|
|
@copilot please run the
|
… skip symlink tests on unsupported platforms Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Done. The following changes were made in commits
Local |
|
🎉 This pull request is included in a new release. Release: |
When
.github/agents(or.github/skills/.github/prompts) is a git symlink to a directory outside.github/(e.g.../.ai/agents), the compiled sparse checkout only fetched the symlink blob — leaving a dangling symlink at runtime and breaking any{{#runtime-import .github/agents/<agent>.md}}expression.Changes
pkg/workflow/compiler_activation_job.go— addsresolveSymlinkExtraPaths, called just before bothGenerateGitHubFolderCheckoutStepinvocations ingenerateCheckoutGitHubFolderForActivation. For each of.github/agents,.github/skills,.github/prompts, it usesos.Lstat/os.Readlinkto detect symlinks, resolves the target relative to the repo root viafilepath.Clean, rejects targets that escape the root (..-prefixed paths), deduplicates, and appends qualifying paths toextraPaths.pkg/workflow/compiler_activation_job_test.go— addsTestResolveSymlinkExtraPathscovering: symlink resolved to inner path, regular directory, missing path, escape-root symlink ignored, and deduplication.Example
Given
.github/agents → ../.ai/agents, the generated sparse checkout becomes: