Skip to content

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

Merged
Astro-Han merged 3 commits into
apache:mainfrom
Sun-GLiang:fix/4096-symlinked-skill-discovery
Aug 30, 2026
Merged

fix(runtime): discover contained symlinked skill directories#4116
Astro-Han merged 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 和后续读取路径,并对这些反馈负责。

Comment thread packages/runtime/src/skills-discovery.ts

@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 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;此前问题是否解决及最终批准决定由我确认。

@Astro-Han
Astro-Han merged commit c62980c into apache:main Aug 30, 2026
2 checks passed
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