Skip to content

fix(runtime): discover contained symlinked skill directories - #4116

Open
Sun-GLiang wants to merge 3 commits into
apache:mainfrom
Sun-GLiang:fix/4096-symlinked-skill-discovery
Open

fix(runtime): discover contained symlinked skill directories#4116
Sun-GLiang wants to merge 3 commits into
apache:mainfrom
Sun-GLiang:fix/4096-symlinked-skill-discovery

Conversation

@Sun-GLiang

Copy link
Copy Markdown
Contributor

Summary

  • Discover symlinked Skill directories when their canonical targets remain inside the configured containment root, while preserving the link entry's id, ref, and path.
  • Diagnose escaping and dangling links with the existing discovery diagnostic contract instead of silently skipping them.
  • Pin inspection and reads to the validated canonical target so replacing the entry link cannot redirect discovery outside the containment root.

Fixes #4096

Verification

  • node --test packages/runtime/dist/__tests__/skills.test.js — 31 passed, 0 failed.
  • npm --workspace @maka/runtime run typecheck — passed.
  • npx biome lint packages/runtime/src/skills-discovery.ts packages/runtime/src/__tests__/skills.test.ts — passed.
  • npx biome format packages/runtime/src/skills-discovery.ts packages/runtime/src/__tests__/skills.test.ts — passed.
  • npm --workspace @maka/runtime test — 3,069 passed, 2 failed, 13 skipped. The same two unrelated failures reproduced on the clean base before this patch: the macOS Bash sandbox executable-root assertion and the deep-research SQLite schema fixture.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: OpenAI Codex analyzed the issue and repository contracts, implemented the Runtime fix and regression tests, ran verification, and performed an AI-assisted pre-review. Human review remains required for submission and merge.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Aug 29, 2026

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing the silent omission of symlinked Skills. The discovery change itself is appropriately scoped, and the main positive, escaping, and dangling-link cases are covered. I verified exact head 45558ba7e76902b3873a6ec066736d6254dd69b5; test and windows_recovery pass.

I found one blocking containment issue inline. It can be fixed narrowly by carrying the existing discoveryRoot authority into later reads.

Most of the diff is the test matrix. The cyclic-link, ENOTDIR, and regular-file-link cases are reasonable defensive coverage, but they are not all required for this issue; feel free to consolidate them if that makes the regression easier to maintain.

I’m leaving a Comment rather than approving this head.

Review analysis was assisted by Codex and an @reviewer; Astro-Han verified the affected discovery and downstream read paths and owns this feedback.

中文对照

谢谢修复 symlink Skill 被静默忽略的问题。Discovery 改动本身范围合理,主要的正常发现、越界和悬空 link 场景也都有覆盖。我验证了精确 head 45558ba7e76902b3873a6ec066736d6254dd69b5testwindows_recovery 均通过。

行内有一条阻塞性的 containment 问题。可以通过让后续读取继续使用现有的 discoveryRoot authority 来小范围修复。

Diff 的大部分来自测试矩阵。循环 link、ENOTDIR 和指向普通文件的 link 都是合理的防御性覆盖,但并非这个 Issue 全部必需;如果能让回归测试更易维护,可以考虑合并这些场景。

当前 head 暂不 Approve,只提交 Comment。

本次审查分析由 Codex 和一位 @reviewer 协助;Astro-Han 核对了 discovery 和后续读取路径,并对这些反馈负责。

}
if (!(await stat(skillReal)).isDirectory()) continue;
skillReadRoot = rootReal;
skillDirectory = skillReal;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for handling symlinked Skill directories while preserving the link identity. [P1, concurrent replacement or attacker-controlled workspace boundary] Discovery validates the target against the original containment root, but later catalog reads use skill.path—the mutable symlink alias—as their containment root. Replacing the link after discovery can therefore redirect a later read outside the originally validated root.

The smallest fix should be to use the existing skill.discoveryRoot as the containment authority for subsequent reads rather than introducing another path field. Please add one regression that replaces the link after discovery and verifies that the later read is blocked.

中文对照

谢谢在支持 symlink Skill directory 的同时保留 link identity。[P1,并发替换或攻击者可控的工作区边界] Discovery 会相对于原始 containment root 验证目标,但后续 catalog 读取却把 skill.path——一个可变的 symlink alias——当作 containment root。因而在 discovery 后替换 link,可以把后续读取重定向到原本验证范围之外。

最小修复不需要增加新的 path 字段,直接让后续读取使用现有的 skill.discoveryRoot 作为 containment authority 即可。请补一条回归测试:在 discovery 后替换 link,并验证后续读取会被阻止。

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 2f34c167d.

Subsequent Skill catalog artifact reads now keep skill.path as the target identity but use the existing skill.discoveryRoot as their containment authority. This covers governance lock/baseline reads, preview reads, and managed-update artifact reads without adding another path field.

Added the regression managed update blocks a symlink redirected outside its discovery root after scanning. It replaces the entry link after the repository snapshot: before the fix the later read followed the replacement and returned local_modified; after the fix it is blocked with metadata_error, and the outside content remains unchanged.

Local verification: Runtime Skills 31/31, Runtime Host repository 29/29, both workspace typechecks, Biome lint/format, and git diff --check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Symlinked skill directories are silently skipped during discovery

2 participants