fix(runtime): discover contained symlinked skill directories - #4116
Conversation
Generated-by: OpenAI Codex
Astro-Han
left a comment
There was a problem hiding this comment.
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 45558ba7e76902b3873a6ec066736d6254dd69b5;test 和 windows_recovery 均通过。
行内有一条阻塞性的 containment 问题。可以通过让后续读取继续使用现有的 discoveryRoot authority 来小范围修复。
Diff 的大部分来自测试矩阵。循环 link、ENOTDIR 和指向普通文件的 link 都是合理的防御性覆盖,但并非这个 Issue 全部必需;如果能让回归测试更易维护,可以考虑合并这些场景。
当前 head 暂不 Approve,只提交 Comment。
本次审查分析由 Codex 和一位 @reviewer 协助;Astro-Han 核对了 discovery 和后续读取路径,并对这些反馈负责。
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for completing the symlinked Skill discovery fix and addressing the earlier containment concern. The current design keeps the link spelling as the Skill's visible identity while using discoveryRoot as the single containment authority for preview, governance, and managed reads. That closes the link-swap escape without adding parallel canonical-path state.
I found no remaining P0 or P1 issues on exact head 402a1d9aefe5ad3064d1f83e92d385f9bd95e673. The focused regression confirms that redirecting the link outside its discovery root fails closed and leaves the outside file unchanged. Approving.
AI-assisted review: Codex helped rereview the production read paths, containment authority, regression coverage, and exact-head CI. I verified that the previous finding is resolved and made the final approval decision.
中文对照
谢谢完成软链接 Skill 的发现修复,并解决此前的 containment 问题。当前设计保留链接写法作为 Skill 的可见身份,同时把 discoveryRoot 作为 preview、governance 和 managed reads 的唯一 containment 权威;这关闭了替换链接后逃逸的路径,也没有增加平行的 canonical-path 状态。
精确 head 上没有剩余 P0/P1。针对性回归测试证明,把链接重定向到 discovery root 之外会 fail closed,且不会修改外部文件。批准。
本次评审使用 Codex 辅助复审生产读取路径、containment 权威、回归覆盖和精确 head CI;此前问题是否解决及最终批准决定由我确认。
Summary
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:
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
Does this PR entail a change in behavior?