diff --git a/docs/guide/separate-input-root.md b/docs/guide/separate-input-root.md index 0492890eb..aa0c1a501 100644 --- a/docs/guide/separate-input-root.md +++ b/docs/guide/separate-input-root.md @@ -55,4 +55,4 @@ rulesync generate --input-root ~/.aiglobal --global --targets claudecode --featu Rulesync follows symbolic links during file discovery. A symlink inside `.rulesync/` that points outside the directory will be followed transparently, and the resolved file content will be copied into the generated output. This is intentional: it lets you centralize shared skills or rules in one place and reference them via symlinks from multiple project directories without duplication. -The trust boundary is the directory you point Rulesync at. `--input-root` is `resolve()`-ed to an absolute path before use, but there is no `realpath`-based boundary check on individual symlinks inside it. Only run Rulesync against trees you control. +The trust boundary is the directory you point Rulesync at. `--input-root` is `resolve()`-ed to an absolute path before use, but there is no `realpath`-based boundary check on individual symlinks inside it. Only run Rulesync against trees you control. Directory symlink cycles are handled safely — discovery results are deduplicated by real path, so a cycle does not produce duplicated output. See the [File Formats § Symlinks](../reference/file-formats.md#symlinks) note for the behavior that applies across all features. diff --git a/docs/reference/file-formats.md b/docs/reference/file-formats.md index 6d4cfccc4..a99caea68 100644 --- a/docs/reference/file-formats.md +++ b/docs/reference/file-formats.md @@ -1,5 +1,11 @@ # File Formats +## Symlinks + +Rulesync follows symbolic links when it discovers source files, whether you use a plain `.rulesync/` directory or a separate `--input-root`. Glob-based discovery (rules, commands, subagents, skills) follows symlinked files and directories; single fixed-path files such as `.rulesyncignore`, `.rulesync/mcp.json`, and `.rulesync/permissions.json` are likewise resolved transparently by the OS when read. A symlink inside the input tree that points elsewhere is followed transparently, and the resolved file content is copied into the generated output. This is intentional: it lets you centralize shared skills or rules in one place and reference them via symlinks without duplication (see [issue #1707](https://github.com/dyoshikawa/rulesync/issues/1707)). + +The trust boundary is the directory you point Rulesync at. There is **no** `realpath`-based containment check on individual symlinks, so a link may resolve to a target outside the input root — enforcing containment would break the shared-file use case above. Only run Rulesync against trees you control. Directory symlink **cycles** are handled safely: results are deduplicated by real path, so a cycle does not produce duplicated output. Note that the remote-fetch path (`rulesync fetch` from a Git repository) is a separate, hardened code path that **skips** symlinks entirely, so untrusted remote content never has its symlinks followed. + ## `rulesync/rules/*.md` Example: diff --git a/skills/rulesync/file-formats.md b/skills/rulesync/file-formats.md index 6d4cfccc4..a99caea68 100644 --- a/skills/rulesync/file-formats.md +++ b/skills/rulesync/file-formats.md @@ -1,5 +1,11 @@ # File Formats +## Symlinks + +Rulesync follows symbolic links when it discovers source files, whether you use a plain `.rulesync/` directory or a separate `--input-root`. Glob-based discovery (rules, commands, subagents, skills) follows symlinked files and directories; single fixed-path files such as `.rulesyncignore`, `.rulesync/mcp.json`, and `.rulesync/permissions.json` are likewise resolved transparently by the OS when read. A symlink inside the input tree that points elsewhere is followed transparently, and the resolved file content is copied into the generated output. This is intentional: it lets you centralize shared skills or rules in one place and reference them via symlinks without duplication (see [issue #1707](https://github.com/dyoshikawa/rulesync/issues/1707)). + +The trust boundary is the directory you point Rulesync at. There is **no** `realpath`-based containment check on individual symlinks, so a link may resolve to a target outside the input root — enforcing containment would break the shared-file use case above. Only run Rulesync against trees you control. Directory symlink **cycles** are handled safely: results are deduplicated by real path, so a cycle does not produce duplicated output. Note that the remote-fetch path (`rulesync fetch` from a Git repository) is a separate, hardened code path that **skips** symlinks entirely, so untrusted remote content never has its symlinks followed. + ## `rulesync/rules/*.md` Example: diff --git a/skills/rulesync/separate-input-root.md b/skills/rulesync/separate-input-root.md index 0492890eb..aa0c1a501 100644 --- a/skills/rulesync/separate-input-root.md +++ b/skills/rulesync/separate-input-root.md @@ -55,4 +55,4 @@ rulesync generate --input-root ~/.aiglobal --global --targets claudecode --featu Rulesync follows symbolic links during file discovery. A symlink inside `.rulesync/` that points outside the directory will be followed transparently, and the resolved file content will be copied into the generated output. This is intentional: it lets you centralize shared skills or rules in one place and reference them via symlinks from multiple project directories without duplication. -The trust boundary is the directory you point Rulesync at. `--input-root` is `resolve()`-ed to an absolute path before use, but there is no `realpath`-based boundary check on individual symlinks inside it. Only run Rulesync against trees you control. +The trust boundary is the directory you point Rulesync at. `--input-root` is `resolve()`-ed to an absolute path before use, but there is no `realpath`-based boundary check on individual symlinks inside it. Only run Rulesync against trees you control. Directory symlink cycles are handled safely — discovery results are deduplicated by real path, so a cycle does not produce duplicated output. See the [File Formats § Symlinks](../reference/file-formats.md#symlinks) note for the behavior that applies across all features. diff --git a/src/features/skills/skills-processor.test.ts b/src/features/skills/skills-processor.test.ts index 1d550308b..fca1ef4be 100644 --- a/src/features/skills/skills-processor.test.ts +++ b/src/features/skills/skills-processor.test.ts @@ -1,3 +1,4 @@ +import { symlink } from "node:fs/promises"; import { join } from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -424,6 +425,38 @@ Invalid content`; await expect(processor.loadRulesyncDirs()).rejects.toThrow(); }); + // End-to-end coverage for issue #1707: a skill directory under .rulesync/skills/ that is + // a symlink to a real directory elsewhere must be loaded like a regular skill. fs.symlink + // needs admin/Developer Mode on Windows, so this is skipped there (issue #1808 #5). + it.skipIf(process.platform === "win32")( + "should load a skill directory that is a symbolic link", + async () => { + const skillsDir = join(testDir, RULESYNC_SKILLS_RELATIVE_DIR_PATH); + await ensureDir(skillsDir); + + // The real skill lives outside .rulesync/skills/, shared via a symlink. + const sharedSkillDir = join(testDir, "shared", "linked-skill"); + await ensureDir(sharedSkillDir); + await writeFileContent( + join(sharedSkillDir, "SKILL.md"), + `--- +name: linked-skill +description: Skill shared via a symbolic link +--- +Linked skill content`, + ); + + await symlink(sharedSkillDir, join(skillsDir, "linked-skill")); + + const rulesyncDirs = await processor.loadRulesyncDirs(); + + expect(rulesyncDirs).toHaveLength(1); + const rulesyncSkill = rulesyncDirs[0] as RulesyncSkill; + expect(rulesyncSkill.getFrontmatter().name).toBe("linked-skill"); + expect(rulesyncSkill.getFrontmatter().description).toBe("Skill shared via a symbolic link"); + }, + ); + it("should throw error when directory without SKILL.md file is found", async () => { const skillsDir = join(testDir, RULESYNC_SKILLS_RELATIVE_DIR_PATH); await ensureDir(skillsDir); diff --git a/src/utils/file.test.ts b/src/utils/file.test.ts index db606f36b..de3c4e264 100644 --- a/src/utils/file.test.ts +++ b/src/utils/file.test.ts @@ -1,4 +1,4 @@ -import { symlink } from "node:fs/promises"; +import { realpath, symlink } from "node:fs/promises"; import { join, resolve } from "node:path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; @@ -551,7 +551,9 @@ describe("file utilities", () => { }); }); - describe("symlink support", () => { + // fs.symlink with the default/file type needs admin or Developer Mode on Windows, so + // these tests are skipped there (CI unit tests run on ubuntu). See issue #1808 #5. + describe.skipIf(process.platform === "win32")("symlink support", () => { it("should include a symlinked file in results", async () => { const realFile = join(testDir, "real.md"); const linkedFile = join(testDir, "linked.md"); @@ -581,6 +583,40 @@ describe("file utilities", () => { }); expect(fileResults).toContain(join(linkedDir, "SKILL.md")); }); + + it("should not produce duplicated entries when a directory symlink cycle exists", async () => { + // skills/a contains a real file and a link back to skills/, forming a cycle that + // globby follows up to the kernel ELOOP limit. Deduplication by real path collapses it. + const skillsDir = join(testDir, "skills"); + const skillA = join(skillsDir, "a"); + await ensureDir(skillA); + await writeFileContent(join(skillA, "SKILL.md"), "skill content"); + await symlink(skillsDir, join(skillA, "loop")); + + const fileResults = await findFilesByGlobs(join(skillsDir, "**", "*.md"), { + type: "file", + }); + + // Exactly one entry survives per real file despite the cycle (no ~40x blowup). + const uniqueRealPaths = new Set(await Promise.all(fileResults.map((p) => realpath(p)))); + expect(uniqueRealPaths.size).toBe(fileResults.length); + expect(fileResults.length).toBeLessThan(5); + }); + + it("should keep only one path per real file when a link and its target both match", async () => { + const realFile = join(testDir, "real.md"); + const linkedFile = join(testDir, "linked.md"); + await writeFileContent(realFile, "content"); + await symlink(realFile, linkedFile); + + const results = await findFilesByGlobs(join(testDir, "*.md"), { type: "file" }); + + const realPaths = await Promise.all(results.map((p) => realpath(p))); + expect(new Set(realPaths).size).toBe(results.length); + // The sorted-first path (linked.md < real.md) survives; its target is dropped. + expect(results).toContain(linkedFile); + expect(results).not.toContain(realFile); + }); }); }); }); diff --git a/src/utils/file.ts b/src/utils/file.ts index 22e6164a8..218f1e0e0 100644 --- a/src/utils/file.ts +++ b/src/utils/file.ts @@ -1,4 +1,14 @@ -import { lstat, mkdir, mkdtemp, readdir, readFile, rm, stat, writeFile } from "node:fs/promises"; +import { + lstat, + mkdir, + mkdtemp, + readdir, + readFile, + realpath, + rm, + stat, + writeFile, +} from "node:fs/promises"; import os from "node:os"; import { dirname, isAbsolute, join, relative, resolve } from "node:path"; @@ -220,18 +230,40 @@ export async function findFilesByGlobs( ? globs.map((g) => g.replaceAll("\\", "/")) : globs.replaceAll("\\", "/"); // followSymbolicLinks: true lets callers use symlinks to share skills/rules across - // directories without duplication. Callers are responsible for passing globs rooted - // inside a trusted directory (inputRoot/outputRoot); this function has no path-boundary - // enforcement. Note: git-client.ts intentionally skips symlinks during remote fetch - // because unresolved symlinks in a bare clone are meaningless — that is a distinct code - // path with different trust assumptions. + // directories without duplication (see issue #1707). Callers operate on user-specified + // local trees that are inside the trust boundary, so symlinks are honored — including + // ones whose targets resolve outside the glob root. This is an intentional trade-off: + // enforcing realpath containment against a single root would break the #1707 use case of + // sharing files that live elsewhere in the same repository. Untrusted remote content is a + // separate code path: git-client.ts (`walkDirectory`) skips symlinks entirely as a + // security hardening for fetched repositories (commit 51bf0443), so this relaxed handling + // never applies to remote input. const results = globbySync(normalizedGlobs, { absolute: true, followSymbolicLinks: true, ...globbyOptions, }); - // Sort for consistent ordering across different glob implementations - return results.toSorted(); + // Deduplicate by real path so that directory symlink cycles (which globby follows up to + // the kernel ELOOP limit, ~40 levels) do not yield ~40x duplicated entries that would be + // read and re-emitted. Keep the first path per real file in sorted order for determinism. + const seenRealPaths = new Set(); + const deduped: string[] = []; + for (const result of results.toSorted()) { + let realResult: string; + try { + realResult = await realpath(result); + } catch { + // realpath can fail on a broken link or race; fall back to the literal path so the + // entry is still considered (and still deduplicated against identical literals). + realResult = result; + } + if (seenRealPaths.has(realResult)) { + continue; + } + seenRealPaths.add(realResult); + deduped.push(result); + } + return deduped; } export async function findRuleFiles(aiRulesDir: string): Promise {