fix(skills): follow symlinked entries in SkillStore.readEntry (#2104) by nianyi778 · Pull Request #2124 · esengine/DeepSeek-Reasonix

nianyi778 · 2026-05-28T05:05:02Z

Problem

Closes #2104. SkillStore.readEntry() classified each entry with Dirent.isDirectory() / isFile(). Both return false for symlinks even when the target is a directory or file — so a skills.sh-style ~/.agents/skills/ layout that symlinks every skill into a central repo (e.g. ~/.sra/<skill> → ~/.agents/skills/<skill>) silently saw zero entries, with no error or warning.

The reporter's case: 22/22 skills under ~/.agents/skills/ were symlinks, all silently ignored.

Change

src/skills.ts readEntry now resolves symlinks via statSync before the type branch:

let isDir = entry.isDirectory();
let isFile = entry.isFile();
if (entry.isSymbolicLink()) {
  try {
    const target = statSync(join(dir, entry.name));
    isDir = target.isDirectory();
    isFile = target.isFile();
  } catch {
    return null; // broken symlink — skip silently rather than abort the scan
  }
}

So a symlinked directory containing SKILL.md and a symlinked flat <name>.md are both discovered exactly like the real entries. Broken symlinks no longer crash the scan; they're just skipped (parity with how the rest of list() handles unreadable roots).

Out of scope

The issue mentions #1464 (same root cause for the @-mention directory listing). That's a separate code path; happy to tackle it in a follow-up if useful, but keeping this PR focused.

Verification

Three new tests/skills.test.ts cases (using fs.symlinkSync):

symlinked directory containing SKILL.md shows up in list(),
symlinked flat <name>.md shows up,
a broken symlink is dropped without crashing — sibling real skills still load.

npm run verify (build + lint + typecheck + full suite) passes.

nianyi778 · 2026-05-28T05:27:26Z

Pushed 02467c46 with two self-review fixes:

Windows CI: symlinkSync now passes the 'dir' / 'file' type arg (Windows defaults to 'file' even for directory targets and would produce a broken link — ignored on POSIX). The broken-symlink test is gated with it.skipIf(process.platform === 'win32') because creating a nonexistent-target symlink on Windows throws EPERM without Developer Mode / admin — unrelated to the readEntry behavior under test.
Tmpdir cleanup: the two tests that create an out-of-scope realRoot now wrap the body in try { … } finally { rmSync(realRoot, …) } so a failing assertion doesn't leak /tmp/reasonix-skills-real-*.

Out of scope (kept the PR focused): src/server/api/skills.ts listSkills has the same Dirent-blind-to-symlinks bug. CLI now discovers symlinked skills (this PR) while the dashboard's API still misses them, so the two listings disagree until that's mirrored. Happy to tackle it in a follow-up — let me know if you'd rather it come in here.

@codex review

chatgpt-codex-connector · 2026-05-28T05:29:39Z

Codex Review: Didn't find any major issues. More of your lovely PRs please.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

esengine

Good catch on the root cause, and the broken-symlink test is the right instinct. But the landscape shifted: #2137 (merged) already shipped the symlinked-directory half of this on main, via isSymlinkDirectory (src/skills.ts:252). So this PR now conflicts and the directory branch duplicates what's already there.

The part that's still genuinely missing on main and worth keeping: symlinked flat <name>.md files. Line 260 still does a bare entry.isFile(), which is false for a symlink, so flat-file symlinks are still dropped.

Please:

Rebase onto current main.
Drop the now-redundant directory logic — reuse the existing isSymlinkDirectory helper rather than reintroducing the let isDir = ... block.
Keep just the flat-.md symlink resolution + the broken-symlink skip.
Trim the tests to the two that aren't already on main: the flat-.md symlink case and the broken-symlink case (the symlinked-directory test already landed with #2137 — drop the duplicate to avoid a name clash).

The flat-.md fix is small but real, so it's worth carrying across the rebase rather than closing.

nianyi778 · 2026-05-29T01:14:14Z

Done — rebased onto current main (47e4deb6).

Dropped the now-redundant directory symlink logic — the directory branch now uses the existing isSymlinkDirectory helper from main (fix: load skills from symlinked directories (#2104) #2137) untouched.
Added a parallel isSymlinkFile private method (same pattern: statSync → .isFile(), returns false for broken symlinks) and wires it into readEntry for the flat <name>.md branch.
Dropped the symlinked-directory test (already on main via fix: load skills from symlinked directories (#2104) #2137); kept the two that are genuinely new: symlinked flat .md file and broken-symlink skip.

@codex review

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 47e4deb6cd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-05-29T01:16:33Z

+      writeFileSync(realFile, "---\ndescription: flat via symlink\n---\nbody\n", "utf8");
+      const scannedRoot = join(home, ".reasonix", "skills");
+      mkdirSync(scannedRoot, { recursive: true });
+      symlinkSync(realFile, join(scannedRoot, "shipit.md"), "file");


Guard flat symlink test on Windows privilege failures

When npm run test:coverage runs on the Windows job (.github/workflows/ci.yml includes windows-latest), this symlinkSync(..., "file") can throw EPERM on hosts without Developer Mode/admin, the same privilege issue already handled by the broken-symlink test below and by the existing at-mentions symlink test. Because this call is unguarded, the skills suite can fail before exercising readEntry; wrap it in a capability guard or skip it on win32.

Useful? React with 👍 / 👎.

nianyi778 · 2026-05-29T01:34:57Z

Fixed in 1e706dc9 — added it.skipIf(process.platform === 'win32') to the flat-symlink test, matching the same guard already on the broken-symlink test below it.

@codex review

nianyi778 · 2026-05-29T01:40:59Z

Note on the CI failures: the 3 failing tests (slash.test.ts, ui-slash-suggestions.test.tsx, auto-git-rollback.test.ts) are also failing on main itself right now — they were broken by the #2173 unified-diff PR and the Telegram PR merged after my rebase. My changes (src/skills.ts + tests/skills.test.ts) don't touch any of those files.

Verified: all skills tests still pass on my branch; the 3 failing tests fail identically on the current main HEAD (8131e241).

…ne#2104) `readEntry` used `Dirent.isDirectory()/isFile()` to classify entries, but both return false for symlinks even when the target is a directory or file — so a `skills.sh`-style `~/.agents/skills/` layout that symlinks every skill into a central repo silently saw zero entries with no error or warning. Resolve symlinks via `statSync` to get the real target type before branching; a broken symlink is skipped silently instead of aborting the whole scan.

esengine

Re-reviewed — exactly the trim + rebase I asked for. It now reuses the merged isSymlinkDirectory (#2137) for the directory case and adds only the missing piece: symlinked flat <name>.md files, via a parallel isSymlinkFile helper, with isFile = entry.isFile() || (entry.isSymbolicLink() && !isDir && this.isSymlinkFile(...)). The !isDir guard correctly prevents a dir-symlink from also matching the file branch. No duplication of what's already on main, tests cover it, CLEAN + CI green. This was the genuinely-missing case (main's bare entry.isFile() skips flat-md symlinks). Merging.

…dmin) (#2239) The two symlinked-skill tests added in #2137 — "loads skills from symlinked directories (#2104)" and "skips broken symlinks gracefully" — call symlinkSync with no Windows guard, so `npm run verify` fails with EPERM for contributors on Windows that lack Developer Mode / admin. CI's Windows runner has the privilege so it slipped through. Add `.skipIf(process.platform === "win32")`, matching the #2124 symlink tests already in this file. Co-authored-by: yhh <yhh@yhhdeMac-mini.local>

nianyi778 force-pushed the fix/skills-symlink-2104 branch from c75487d to 02467c4 Compare May 28, 2026 05:27

esengine mentioned this pull request May 28, 2026

fix: SkillStore.readEntry now handles symlinked skill directories #2112

Closed

esengine requested changes May 28, 2026

View reviewed changes

nianyi778 force-pushed the fix/skills-symlink-2104 branch from 02467c4 to 47e4deb Compare May 29, 2026 01:14

chatgpt-codex-connector Bot reviewed May 29, 2026

View reviewed changes

nianyi778 force-pushed the fix/skills-symlink-2104 branch from 47e4deb to 1e706dc Compare May 29, 2026 01:34

nianyi778 mentioned this pull request May 29, 2026

fix(tests): sync assertions broken by #2173 and #2168 #2200

Merged

nianyi778 force-pushed the fix/skills-symlink-2104 branch from 1e706dc to a32db69 Compare May 29, 2026 02:29

esengine approved these changes May 29, 2026

View reviewed changes

esengine merged commit 0711fad into esengine:main May 29, 2026
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(skills): follow symlinked entries in SkillStore.readEntry (#2104)#2124

fix(skills): follow symlinked entries in SkillStore.readEntry (#2104)#2124
esengine merged 1 commit into
esengine:mainfrom
nianyi778:fix/skills-symlink-2104

nianyi778 commented May 28, 2026

Uh oh!

nianyi778 commented May 28, 2026

Uh oh!

chatgpt-codex-connector Bot commented May 28, 2026

Uh oh!

esengine left a comment

Uh oh!

nianyi778 commented May 29, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot May 29, 2026

Uh oh!

nianyi778 commented May 29, 2026

Uh oh!

nianyi778 commented May 29, 2026

Uh oh!

esengine left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

nianyi778 commented May 28, 2026

Problem

Change

Out of scope

Verification

Uh oh!

nianyi778 commented May 28, 2026

Uh oh!

chatgpt-codex-connector Bot commented May 28, 2026

Uh oh!

esengine left a comment

Choose a reason for hiding this comment

Uh oh!

nianyi778 commented May 29, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot May 29, 2026

Choose a reason for hiding this comment

Uh oh!

nianyi778 commented May 29, 2026

Uh oh!

nianyi778 commented May 29, 2026

Uh oh!

esengine left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants