-
Notifications
You must be signed in to change notification settings - Fork 456
fix: follow symlinks when generating sparse checkout for .github subdirs #43909
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
67202ab
d6f1415
8702b96
9caa6c6
854efe4
e0676f5
edcac7a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| # ADR-43909: Follow Symlinks in Sparse Checkout for .github Subdirectories | ||
|
|
||
| **Date**: 2026-07-07 | ||
| **Status**: Draft | ||
| **Deciders**: Unknown | ||
|
|
||
| --- | ||
|
|
||
| ### Context | ||
|
|
||
| The workflow compiler builds sparse-checkout configurations for `.github` subdirectories (`.github/agents`, `.github/skills`, `.github/prompts`) when generating activation jobs. Some repositories use git symlinks at these paths to point to directories elsewhere in the repo (e.g. `.github/agents → ../.ai/agents`). Previously, the compiler added only the symlink blob path to the sparse-checkout manifest; at runtime this produced a dangling symlink, causing any `{{#runtime-import .github/agents/<agent>.md}}` expression to fail silently. No mechanism existed to detect this case or include the symlink's target directory in the checkout. | ||
|
|
||
| ### Decision | ||
|
|
||
| We will add a `resolveSymlinkExtraPaths` function that inspects `.github/agents`, `.github/skills`, and `.github/prompts` at compile time using `os.Lstat` and `os.Readlink`. When a candidate path is a symlink, the resolved target (relative to the repository root via `filepath.Clean(filepath.Join(dir, target))`) is appended to `extraPaths` before the sparse-checkout step is generated. Symlink targets whose resolved path begins with `..` are silently rejected to prevent path traversal. Paths already present in `extraPaths` are deduplicated via a map. | ||
|
|
||
| ### Alternatives Considered | ||
|
|
||
| #### Alternative 1: Require explicit user configuration | ||
|
|
||
| Repository owners could be required to enumerate symlink targets explicitly in a configuration file or in their sparse-checkout definition. This pushes the responsibility to every affected repository and is error-prone: users must know the internal structure of the compiled sparse-checkout and keep their configuration in sync as symlink targets change. Given that the compiler already resolves extra paths automatically for other cases, silent auto-detection is the lower-friction approach. | ||
|
|
||
| #### Alternative 2: Resolve all symlinks generically across extraPaths | ||
|
|
||
| Instead of inspecting a fixed set of three candidates, the compiler could recursively follow symlinks for every entry in `extraPaths`. This would cover any future well-known paths without code changes. However, unbounded symlink traversal increases the risk of accidentally including unintended directories, makes the behavior harder to reason about, and complicates path-escape validation. Limiting resolution to the three known subdirectories keeps the scope narrow and the security surface small. | ||
|
|
||
| ### Consequences | ||
|
|
||
| #### Positive | ||
| - `runtime-import` expressions that reference `.github/agents`, `.github/skills`, or `.github/prompts` now work correctly when those paths are git symlinks. | ||
| - Path traversal is explicitly blocked: symlink targets resolving outside the repository root are ignored with a log message, preserving security guarantees. | ||
| - Deduplication ensures the generated sparse-checkout manifest does not contain redundant entries when a target is already present. | ||
|
|
||
| #### Negative | ||
| - `resolveSymlinkExtraPaths` uses relative paths (`os.Lstat(".github/agents")`) and implicitly assumes the compiler's working directory is the repository root at the time of compilation. If the working directory differs, all three `os.Lstat` calls fail silently and no symlink targets are added — the bug reappears without any error surfaced to the caller. | ||
| - Only the three hard-coded candidate paths are inspected. Other symlinked `.github` subdirectories (e.g. `.github/workflows` or custom subdirectories) are not covered and would require separate changes. | ||
|
|
||
| #### Neutral | ||
| - The function is append-only: it does not modify or remove existing `extraPaths` entries, preserving backward compatibility with all callers. | ||
| - The `resolveSymlinkExtraPaths` call is placed immediately before both `GenerateGitHubFolderCheckoutStep` invocations via the single call-site in `generateCheckoutGitHubFolderForActivation`, so all callers inherit the fix without further changes. | ||
|
|
||
| --- | ||
|
|
||
| *ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,8 @@ package workflow | |
|
|
||
| import ( | ||
| "fmt" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
|
|
||
| "github.com/goccy/go-yaml" | ||
|
|
@@ -434,6 +436,17 @@ func (c *Compiler) generateCheckoutGitHubFolderForActivation(data *WorkflowData) | |
| } | ||
| compilerActivationJobLog.Printf("Adding %d engine-specific dirs to sparse-checkout: %v", len(extraPaths), extraPaths) | ||
|
|
||
| // Detect symlinks for well-known .github sub-paths and add their resolved targets | ||
| // so that sparse checkout fetches the target directory, not just the symlink blob. | ||
| // Use c.gitRoot so detection works regardless of the process CWD. | ||
| repoRoot := c.gitRoot | ||
| if repoRoot == "" { | ||
| if cwd, err := os.Getwd(); err == nil { | ||
| repoRoot = cwd | ||
| } | ||
| } | ||
| extraPaths = resolveSymlinkExtraPaths(repoRoot, extraPaths) | ||
|
|
||
| cm := NewCheckoutManager(nil) | ||
| activationToken := c.resolveActivationToken(data) | ||
| if data != nil && hasWorkflowCallTrigger(data.On) && !data.InlinedImports { | ||
|
|
@@ -531,3 +544,68 @@ func injectIfConditionAfterName(step, condition string) string { | |
| newLines = append(newLines, lines[nameLineIdx+1:]...) | ||
| return strings.Join(newLines, "\n") | ||
| } | ||
|
|
||
| // resolveSymlinkExtraPaths inspects well-known .github sub-paths that are commonly | ||
| // symlinked (e.g. .github/agents -> ../.ai/agents). When a path is a symlink, the | ||
| // resolved target directory (relative to the repo root) is appended to extraPaths so | ||
| // that the sparse-checkout step fetches the target files, not just the dangling symlink | ||
| // blob. Symlink targets that escape the repository root are silently ignored to prevent | ||
| // path traversal. Already-present paths are not duplicated. | ||
| // repoRoot must be an absolute path to the repository root; when empty, the function is a no-op. | ||
| func resolveSymlinkExtraPaths(repoRoot string, extraPaths []string) []string { | ||
| if repoRoot == "" { | ||
| return extraPaths | ||
| } | ||
| candidates := []string{ | ||
| ".github/agents", | ||
| ".github/skills", | ||
| ".github/prompts", | ||
| } | ||
| existing := make(map[string]struct{}, len(extraPaths)) | ||
| for _, p := range extraPaths { | ||
| existing[p] = struct{}{} | ||
| } | ||
| for _, candidate := range candidates { | ||
| absCandidate := filepath.Join(repoRoot, candidate) | ||
| info, err := os.Lstat(absCandidate) | ||
| if err != nil || info.Mode()&os.ModeSymlink == 0 { | ||
| continue // not a symlink or doesn't exist | ||
|
Comment on lines
+568
to
+572
|
||
| } | ||
| target, err := os.Readlink(absCandidate) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| // Resolve target to an absolute path. | ||
| // When the symlink target is absolute (e.g. /etc/passwd), use it directly. | ||
| // When relative, resolve it against the directory that contains the symlink. | ||
| var absResolved string | ||
| if filepath.IsAbs(target) { | ||
| absResolved = filepath.Clean(target) | ||
| } else { | ||
| absResolved = filepath.Clean(filepath.Join(filepath.Dir(absCandidate), target)) | ||
| } | ||
| rel, err := filepath.Rel(repoRoot, absResolved) | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Absolute symlink target bypasses escape check: 💡 Suggested fixTighten 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. |
||
| // Reject paths that escape the repository root using a segment-aware check. | ||
| // strings.HasPrefix would incorrectly reject names like "..foo". | ||
| firstSeg := rel | ||
| if i := strings.IndexByte(rel, filepath.Separator); i >= 0 { | ||
| firstSeg = rel[:i] | ||
| } | ||
| if firstSeg == ".." { | ||
| compilerActivationJobLog.Printf("Ignoring symlink %s -> %s: resolved path %q escapes the repository root", candidate, target, rel) | ||
| continue | ||
| } | ||
| // Normalize to forward slashes for use in YAML sparse-checkout paths. | ||
| rel = filepath.ToSlash(rel) | ||
| if _, alreadyPresent := existing[rel]; alreadyPresent { | ||
| continue | ||
| } | ||
| compilerActivationJobLog.Printf("Symlink detected: %s -> %s (adding %s to sparse checkout)", candidate, target, rel) | ||
| extraPaths = append(extraPaths, rel) | ||
| existing[rel] = struct{}{} | ||
| } | ||
| return extraPaths | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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.Chdiras a workaround, which is the clearest sign this contract needs to be explicit.