fix(file): dedup symlink-cycle glob results and clarify symlink trust model - #1811
Merged
Merged
Conversation
… model Follow-ups from PR #1802 review (issue #1808): - Deduplicate findFilesByGlobs results by real path so directory symlink cycles (which globby follows up to the kernel ELOOP limit) no longer yield ~40x duplicated entries that get read and re-emitted (#1) - Rewrite the findFilesByGlobs comment: git-client.ts skips symlinks as a security hardening for untrusted remote content (commit 51bf044), not because a bare clone makes them meaningless — it does a non-bare checkout (#2) - Document the intentional trade-off of not enforcing realpath containment (it would break the issue #1707 shared-file use case); record the decision in the comment and docs rather than adding a containment check (#3) - Add a skills-processor test loading a symlinked skill directory (#4) - Guard symlink unit tests with skipIf(win32) since fs.symlink needs admin / Developer Mode on Windows (#5) - Add a feature-wide Symlinks section to docs/reference/file-formats.md and cross-link it from the input-root guide; sync skills/rulesync (#6) Closes #1808 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Address low-severity review findings on PR #1811: - Narrow the Symlinks docs wording: glob-based discovery (rules, commands, subagents, skills) follows symlinks; mcp/ignore/permissions load a single fixed-path file that the OS resolves transparently on read (no glob discovery) - Strengthen the link-and-target dedup test to assert the sorted-first path (linked.md) survives and its target (real.md) is dropped Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Owner
Author
|
@dyoshikawa Thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background
Issue #1808 consolidated the follow-up findings from the code and security reviews of PR #1802 (
fix(file): follow symbolic links in findFilesByGlobs, resolving #1707). None blocked that PR, but they were tracked so they would not be lost. Each was re-validated against the current code before this fix.Changes
findFilesByGlobsnow deduplicates results by real path (realpath), keeping the first path per real file in sorted order. A directory symlink cycle (which globby follows up to the kernel ELOOP limit, ~40 levels) no longer yields ~40× duplicated entries thatAiDir.collectOtherFileswould read and re-emit. Added a circular-symlink test and a link-and-target-collapse test.src/lib/git-client.tsdoes a non-bare clone (--no-checkout --filter=blob:nonethengit checkout), so symlinks are resolvable — the old comment ("unresolved symlinks in a bare clone are meaningless") was factually wrong. Rewrote it to state the real reason:walkDirectoryskips symlinks as a security hardening for untrusted remote content (commit51bf0443), a distinct code path from the local, trustedfindFilesByGlobs.realpathcontainment against a single root would break issue Support symlinked shared skills in multi-directory repositories #1707's use case of sharing files that live elsewhere in the same repository (and--input-roottrees legitimately point outside themselves). The remote-fetch path already skips symlinks, so untrusted content is not an attack vector. Documented in the code comment and the new docs section.skills-processor.test.tscase that loads a skill directory which is a symlink to a real directory elsewhere.describe.skipIf(process.platform === "win32")/it.skipIf(...)sincefs.symlinkneeds admin or Developer Mode on Windows.docs/reference/file-formats.md(covering all features and plain.rulesync/usage, not just--input-root), cross-linked it from the input-root guide, and syncedskills/rulesync/.pnpm cicheckpasses locally (6194 tests; the content sync-check passes once the synced skills files are committed).Closes #1808
🤖 Generated with Claude Code