Skip to content

feat: skills registry - #172

Merged
yujiezhang-ops merged 33 commits into
mainfrom
feat/skills-registry
Aug 13, 2026
Merged

feat: skills registry#172
yujiezhang-ops merged 33 commits into
mainfrom
feat/skills-registry

Conversation

@Paulkm2006

Copy link
Copy Markdown
Collaborator

Summary

Implement skill discovery, sync and delete.

Related issue: #161

Verification

  • Tests were added or updated for behavior changes, or the reason they are unnecessary is explained.
  • Relevant Go, frontend, documentation, and release-compliance checks pass locally.
  • UI changes include screenshots or a short recording, or this change has no UI impact.

Change checklist

  • No API key, token, private configuration, or sensitive path is present in code, logs, screenshots, fixtures, or issue links.
  • No Wails DTO or service changed, or frontend/bindings, frontend/src/backend/wails.ts, and handwritten API types were synchronized.
  • No documented user-visible behavior changed, or README.md and README_ZH.md were updated together.
  • No distributed dependency or third-party mark was added, or NOTICE was updated.
  • Public documentation is in English; AGENTS.md and docs/internal/ remain in Chinese.

@Paulkm2006
Paulkm2006 requested a review from a team August 13, 2026 02:51
@Paulkm2006 Paulkm2006 added the enhancement New feature or request label Aug 13, 2026
@Paulkm2006 Paulkm2006 linked an issue Aug 13, 2026 that may be closed by this pull request
3 tasks
@Paulkm2006
Paulkm2006 force-pushed the feat/skills-registry branch from 6780418 to ddcd002 Compare August 13, 2026 02:52
@yujiezhang-ops

Copy link
Copy Markdown
Collaborator

审了一遍,安全设计的主体是扎实的:路径穿越、符号链接、以及「不能删用户自己写的 Skill」这三条防线我都实测过,都成立。有一个数据丢失问题建议合并前修掉。

下面每条都是在 pr172 上写测试跑出来的,不是读代码推断。

P0 — 重新发布 Skill 会静默删除用户在目标目录里的文件

internal/app/skill.go:436internal/skill/discovery.go:264-282

PublishTree整目录替换:暂存副本 → destination 改名为 rollback → stage 改名为 destination → RemoveAll(rollback)。替换前不检查目标目录里有什么。

复现(走真实入口 ApplySkills,非内部函数):

second apply: [{Agent:"codex", TargetUpdated:true, RegistryUpdated:true, Error:""}]
DATA LOSS: user file destroyed by re-publish:
  .../.codex/skills/review/my-notes.md: no such file or directory

场景:用户导入 Skill review 同步到 codex,之后在 ~/.codex/skills/review/ 里加了自己的 my-notes.md(这是最自然的工作位置),或者改了 SKILL.md。之后只要再 apply 一次 —— 包括只是在 UI 里多勾一个目标 Agent,因为那会向所有已选目标重新发布 —— 文件就没了,而结果报告是绿色的 Error:""

三点让它更值得修:

一、同一份代码里对同一风险的两种态度。 removeManagedSkillskill.go:891-896)在删除前先 HashTree 比对,不一致就拒绝并返回 "Skill target changed and was not removed"。我实测了这条保护确实生效。也就是说 delete 路径明确防的这件事,publish 路径直接做了。

二、与仓库既有约定相反。 internal/config/write.go:513 的注释写着「凡是它没点名的内容都原样透传」;internal/mcp/adapter.go 是按 id 逐条 patch 而不是替换整个 map。Skills 这里替换整个目录。

三、没有备份也没有警告。 CreateBackup 全仓库只在 skill.go:510UninstallSkill)被调用一次,publish 路径不走它。而这个 app 其他覆盖路径都有提示(i18n.tsx:341「覆盖前会创建带时间戳的备份」、:92「该操作无法撤销」),Skills 页面没有。

有个偶然的缓解:如果用户编辑后恰好触发过 ScanSkills(启动时会跑),改动会被当成新 variant 哈希进 ~/.oneagent/skills/<id>/variants/<hash>/,字节还在。但这是碰巧,不是设计,而且 UI 没有入口能取回。

建议:让 publish 与 delete 对称 —— 发布前 HashTree 目标,与 OneAgent 上次发布的 variant 不一致时,要么先备份要么拒绝并报冲突。就是 removeManagedSkill 已经在做的事。

P2 — 排队的删除会扩散到用户没见过的目标

internal/app/skill.go:390-394

Targets 为空的删除会在apply 时展开成 fact.Variants[idx].ManagedTargets,而不是排队时。实测:

delete touched 2 agents: [{claude-code, TargetUpdated:true}, {codex, TargetUpdated:true}]

先在只发布到 codex 时排一个删除,applied 之前又发布到了 claude-code,那次删除会把两个都清掉。

影响有限 —— removeManagedSkill 的哈希校验意味着只会删掉与 OneAgent 发布内容逐字节相同的目录,且 variant 仍在私有库里。但结果和 UI 当时展示的不一致。

P3 — 重复删除返回空结果,UI 无从展示

同一个删除执行第二次:variantIdx < 0targetIDs 为空,agent 循环不进入,Results: []。既不是成功也不是失败:

repeat delete results: []app.SkillAgentApplyResult{} (len=0)

P3 — Uninstall 的备份在 UI 里取不到

internal/binding/skill.go:97,107wails.ts:234-235 暴露了 ListBackups/RestoreBackup,但 SkillsPage.tsx 里零引用。UninstallSkill 建的安全网只能手工翻 ~/.oneagent/skill-backups/ 才能用到。

确认做对的部分

这些我都实测过,不是扫一眼:

路径穿越被正确拦下。 skill.ValidateID 是 apply 循环的第一条语句(skill.go:330),在任何路径拼接之前:

id="../victim"     -> invalid Skill ID
id="../../victim"  -> invalid Skill ID
id="/etc"          -> invalid Skill ID
id=".."            -> invalid Skill ID
id="./../victim"   -> invalid Skill ID
victim survived all traversal attempts

符号链接防线是分层的,每层都成立。 全 PR 没有 filepath.EvalSymlinksHashTreemodel.go:116)遇到任何符号链接就报错;copyDirdiscovery.go:320)拒绝;eligibleSkillAgentsskill.go:137)会把 skills 根是符号链接的 Agent 剔除;hasSymlinkComponentskill.go:410)守住目标父链。

扫描到的 Skill 不能被删除 —— 这条最关键。 扫描只赋予 ObservedAgents,而删除要求 ManagedTargetsskill.go:417)。所以用户自己写的 Skill 不会被 OneAgent 删掉,实测确认:

delete of a scanned-only Skill: []
user's own Skill survived

权限正确。 发布的文件 0600、目录 0700,故意设成 0644 的源文件会被收紧。注册表与备份元数据走 securefs.AtomicWrite

幂等性正确。 连续三次 apply 后注册表逐字节一致,无重复 variant,无残留 stage 目录。

启动同步的设计是对的。 main_wails.go 里只调 ScanMCP/ScanSkills(只读扫描,不写文件),且 FirstRun 时整段跳过。三个相关提交(preserve first-run onboarding before startup synckeep startup sync out of first-run status)说明这个边界是被认真处理过的。

测试覆盖评估

TestApplySkillsRefusesToDeleteChangedManagedTargetskill_test.go:114)是一个真正的数据丢失测试,断言用户文件在删除后仍存在 —— 这个写得很好。

但覆盖面是不对称的:

  • P0 没有测试,而原因是结构性的:所有 publish 测试都写进全新的空目标目录。上面那个测试是唯一一个往受管目录里放用户文件的,而它只断言 delete 路径。把它的对称版本(放文件 → 重新发布)加上,测试就会失败。
  • 穿越 ID 没有在 app 层测过model_test.go:11 单测 ValidateIDstore_test.go:254 覆盖 RemoveSkill,但没有测试断言 ApplySkills/UninstallSkill 会拒绝穿越 ID。守卫接在 skill.go:330 是对的,但目前这一点对测试套件是偶然成立的 —— 我得自己写探针才能确认。
  • 符号链接测试全部针对 ~/.oneagent/,没有一个把符号链接放进 Agent 目录再走 apply。hasSymlinkComponentskill.go:747)没有直接测试。

一处小提醒

skill.go:178oldFacts[id] = fact 是对 Variants 底层数组的别名,不是快照。我追了一遍,当前行为恰好正确(strip 只移除当前 agentID,而 addSorted 会重新加回),但离出问题只差一行改动,值得改成显式拷贝。


我的建议是 P0 合并前修。其余几条可以后续处理,其中「备份取不到」和「重复删除返回空」都是 UI 层的小补。

@yujiezhang-ops
yujiezhang-ops merged commit 871e712 into main Aug 13, 2026
4 checks passed
@yujiezhang-ops
yujiezhang-ops deleted the feat/skills-registry branch August 13, 2026 03:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Feature]: Skills registry

2 participants