chore(core): remove the unused trusted hooks manager and implement unregisterSkillHooks

[qqqys]

What this PR does

Two pieces of hook code that looked functional but were not are fixed. TrustedHooksManager and its tests are removed: it was never constructed anywhere, and project hooks are already gated by folder trust when settings load and again when a project skill's hook fires. unregisterSkillHooks now does what its name says. It removes the session hooks a skill registered, identified by the skill's root directory, and returns how many it removed. It previously logged a message and returned 0, and a skill without a root directory still returns 0 so another skill's hooks are never removed by mistake.

Why it's needed

A trust manager that is exported, tested and documented by a comment elsewhere suggests hooks go through a per-hook approval step that does not exist, which misleads anyone auditing how project hooks are trusted. unregisterSkillHooks is exported from the hooks module, so a caller that unloads a skill and relies on it would keep that skill's hooks firing for the rest of the session.

Reviewer Test Plan

How to verify

• cd packages/core && npx vitest run src/hooks/registerSkillHooks.test.ts src/hooks/hookRegistry.test.ts. The new case registers two skills, unregisters one, checks that only its hook is gone and the other skill's hook remains, and that the first skill can be registered again. A second new case checks that a skill without a root directory removes nothing.
• git grep -n -i -E "TrustedHooksManager|trustedHooks" -- . returns nothing; the deleted file's entry in the legacy-filename lint allowlist is removed too.

Evidence (Before & After)

Neither change has user-visible behaviour to capture, so the evidence is the tests and the reference search.

$ cd packages/core && npx vitest run src/hooks/registerSkillHooks.test.ts src/hooks/hookRegistry.test.ts src/hooks/sessionHooksManager.test.ts
 Tests  90 passed (90)

$ git grep -n -i -E "TrustedHooksManager|trustedHooks" -- .
(no output)

The legacy-filename lint allowlist entry for the deleted file is removed as well. With registerSkillHooks.ts reverted to main, the new case that unregisters one of two skills fails, and the rootless-skill case passes because the old function also returned 0.

Tested on

OS	Status
🍏 macOS	⚠️
🪟 Windows	⚠️
🐧 Linux	✅

Environment (optional)

N/A, unit tests only.

Risk & Scope

• Main risk or tradeoff: none at runtime. Nothing constructed the removed class, and no current caller invokes unregisterSkillHooks.
• Not validated / out of scope: calling unregisterSkillHooks when a skill body is unloaded, and the if field on HTTP hooks, which is declared but not evaluated yet and stays because it is part of the daemon status and SDK types.
• Breaking changes / migration notes: TrustedHooksManager is no longer exported from its module file; it was not re-exported from the package entry.

Linked Issues

Part of #11610

中文说明

这个 PR 做了什么

修正了两处看起来能用、实际不起作用的 hook 代码。删除 TrustedHooksManager 及其测试：它从未在任何地方被构造，而项目 hook 已经在加载设置时、以及项目 skill 的 hook 触发时按文件夹信任状态做了门控。unregisterSkillHooks 现在名副其实：按 skill 的根目录识别并移除该 skill 注册的会话 hook，返回移除的数量。它以前只打一条日志并返回 0；没有根目录的 skill 仍然返回 0，以免误删其它 skill 的 hook。

为什么需要

一个被导出、有测试、还在别处注释里被提到的信任管理器，会让人以为 hook 要经过逐个审批，而这一步并不存在，这会误导审查项目 hook 信任机制的人。unregisterSkillHooks 从 hooks 模块导出，若有调用方在卸载 skill 时依赖它，该 skill 的 hook 会在会话剩余时间里继续触发。

评审测试计划

如何验证

• cd packages/core && npx vitest run src/hooks/registerSkillHooks.test.ts src/hooks/hookRegistry.test.ts。新用例注册两个 skill、注销其中一个，检查只有它的 hook 被移除、另一个 skill 的 hook 仍在，并且第一个 skill 可以再次注册。另一个新用例检查没有根目录的 skill 不会移除任何 hook。
• git grep -n -i -E "TrustedHooksManager|trustedHooks" -- . 没有结果；已删除文件在旧文件名 lint 白名单里的条目也一并移除。

证据（前后对比）

两处改动都没有用户可见的行为可截取，证据是测试和引用搜索：registerSkillHooks.test.ts、hookRegistry.test.ts、sessionHooksManager.test.ts 共 90 个通过；git grep 搜不到 TrustedHooksManager 或 trustedHooks。已删除文件在旧文件名 lint 白名单里的条目也一并移除。把 registerSkillHooks.ts 换回 main 版本后，注销两个 skill 之一的新用例失败；无根目录 skill 的用例仍通过，因为旧函数本来就返回 0。

测试平台

仅在 Linux 上本地验证；macOS 和 Windows 未测试。

环境

不适用，仅单元测试。

风险与范围

• 主要风险或权衡：没有运行时影响。被删除的类没有任何构造点，目前也没有调用方调用 unregisterSkillHooks。
• 未验证 / 不在范围内：在卸载 skill 正文时调用 unregisterSkillHooks，以及 HTTP hook 上已声明但尚未求值的 if 字段；后者属于 daemon 状态和 SDK 类型的一部分，因此保留。
• 破坏性变更 / 迁移说明：TrustedHooksManager 不再从其模块文件导出；它原本也没有从包入口再导出。

关联 Issue

Part of #11610

[qwen-code-ci-bot]

✅ Qwen Triage finished — CI landed green on ae336e9 and the deferred approval was posted. finalize run

✅ Qwen Triage 已完成 —— ae336e9 的 CI 全绿，延迟审批已提交。查看 finalize 运行

[qwen-code-ci-bot]

Thanks for the PR — this is the dead-code item from the #11610 plan, and it holds up under checking.

Template ✓ — every section filled in, including the Tested-on table and a complete Chinese translation.

Problem — real, and I verified it rather than taking the description's word for it. TrustedHooksManager has zero production consumers repo-wide: the only references anywhere are its own file, its own test, and an inert vi.mock('./trustedHooks.js') in hookRegistry.test.ts (hookRegistry.ts never imported the module, so the mock was decorating nothing). unregisterSkillHooks has zero callers and returned 0 unconditionally. Both claims confirmed.

Worth being plain about what this is, though: neither half changes runtime behaviour today, so this is code-honesty work rather than a user-facing bug fix. That is how #11610 scoped it, so it is fine — it just should not be read as a fix for something users hit.

The security framing also checks out, which mattered to me here since this is the hook-trust subsystem. Removing the trust manager removes no enforcement: folder trust still gates project-skill hooks at registration (skill-utils.ts:282) and again at fire time (hookEventHandler.ts:857-870 re-reads Config.isTrustedFolder() per event, so a mid-session revocation silences the hook). I read both paths. What the deletion removes is a misleading impression of a per-hook approval step that never existed — an exported, unit-tested trust manager sitting in hooks/ is exactly the thing an auditor would burn an afternoon on.

Direction — aligned. It is a pre-declared plan item on a maintainer-triaged P1 umbrella ("Remove the unused trusted hooks manager and implement unregisterSkillHooks: #11621"), not a drive-by. Claude Code's CHANGELOG has no trusted-hooks manager and no unregister API to diverge from; the nearest signal is upstream's "Fixed skill hooks firing twice per event when a hooks-enabled skill is invoked by the model", which this repo already covers with the dedup guard in registerSkillHooks. No public-contract break either: unregisterSkillHooks lives in the hooks/index.ts barrel but is not re-exported from the package entry packages/core/src/index.ts, and nothing outside hooks/ references trustedHooks in docs, schema, or config — so no documentation is left stale.

Size — core paths are touched (packages/core/src/hooks/**, plus a cross-package comment edit in packages/cli), so the two-tier gate applies. The title type is chore, so the large-core-refactor hard block does not, and the diff genuinely is cleanup rather than a restructure wearing a chore label. Breakdown: 170 production logic lines (132 of them the single deleted trustedHooks.ts, 35 in registerSkillHooks.ts, 2 comment-only in supervisor-store.ts, 1 lint allowlist) vs 188 test lines and 0 generated/schema, across 7 files. Under both the 500-line maintainer-awareness threshold and the 1000-line advisory. Tier 2 asks whether I can name every downstream consumer — I can, and the enumeration is in the code review comment — so this proceeds instead of escalating.

Approach — the deletion half is exactly the minimal change, right down to the lint allowlist entry and the now-dangling trustedHooks mention in a supervisor-store.ts comment. The implementation half is where I'd push back a little, as a question rather than a blocker: the argument you use to justify deleting TrustedHooksManager — unused, not in the package entry — applies equally to unregisterSkillHooks, and the reload problem it would exist to solve is already solved by the dedup guard, whose own comment says unloading a skill body "never unregisters its session hooks" by design. Deleting the stub would come out roughly 30 lines smaller than implementing and testing it. I can see the counter-argument (an exported function that lies about what it does is its own defect, and the umbrella plan says "implement"), so I am not asking you to change it — but if a maintainer would rather see it gone than filled in, that is the cheaper PR and this is the moment to say so.

Risk — no high-risk path match. The revert-correlated set (geminiChat, shell.ts, shellExecutionService, mcp-client, acp-integration, sandbox.ts, relaunch.ts, the streaming parsers) is untouched, so no elevated review depth is required.

Moving on to code review. 🔍

中文说明

感谢贡献！这是 #11610 计划里的死代码清理项，核查之后站得住。

模板 ✓ —— 各部分都填了，包括测试平台表格和完整的中文翻译。

问题 —— 真实存在，而且是我自己核过的，不是照抄描述。TrustedHooksManager 在全仓库没有任何生产代码消费者：唯一的引用是它自己的文件、它自己的测试，以及 hookRegistry.test.ts 里一处失效的 vi.mock('./trustedHooks.js')（hookRegistry.ts 从未 import 这个模块，所以那个 mock 什么也没拦住）。unregisterSkillHooks 没有任何调用方，且无条件返回 0。两条结论均已确认。

不过要说清楚这是什么：两半改动今天都不改变运行时行为，所以这是"让代码诚实"的工作，不是用户会踩到的 bug 修复。#11610 本来就是这么定范围的，因此没问题——只是不要把它当成用户可见的修复来读。

安全层面的说法也成立，这点我很在意，因为这是 hook 信任子系统。删掉这个信任管理器不会削弱任何约束：文件夹信任仍然在注册时（skill-utils.ts:282）和触发时（hookEventHandler.ts:857-870 每次事件重新读取 Config.isTrustedFolder()，会话中途撤销信任即可让 hook 静默）双重门控项目 skill 的 hook。这两条路径我都读了。删除真正去掉的是一种误导性的印象——一个逐 hook 审批步骤其实从来不存在；一个被导出、有单测的信任管理器放在 hooks/ 里，正是审计者会白白花掉一下午的东西。

方向 —— 对齐。它是维护者已定级 P1 的总纲 issue 中预先声明的计划项（"Remove the unused trusted hooks manager and implement unregisterSkillHooks: #11621"），不是顺手夹带。Claude Code 的 CHANGELOG 里没有 trusted hooks manager、也没有 unregister API 可供"对齐/背离"；最接近的信号是上游那条 "Fixed skill hooks firing twice per event when a hooks-enabled skill is invoked by the model"，而本仓库已经用 registerSkillHooks 里的去重逻辑覆盖了。也不构成公共契约破坏：unregisterSkillHooks 在 hooks/index.ts 这个 barrel 里，但没有从包入口 packages/core/src/index.ts 再导出；hooks/ 之外没有任何文档、schema 或配置引用 trustedHooks，所以不会留下过期文档。

规模 —— 触及核心路径（packages/core/src/hooks/**，外加 packages/cli 里一处跨包注释修改），因此适用两级门禁。标题类型是 chore，所以大规模核心 refactor 硬阻断不适用；而且 diff 确实是清理，不是穿着 chore 外衣的结构重构。明细：生产逻辑 170 行（其中 132 行是单个被删的 trustedHooks.ts，35 行在 registerSkillHooks.ts，2 行是 supervisor-store.ts 的纯注释，1 行是 lint 白名单），测试 188 行，生成/schema 0 行，共 7 个文件。低于 500 行的维护者关注阈值，也低于 1000 行的大 PR 建议阈值。Tier 2 要求能点名每一个下游消费者——我能（完整清单在代码审查评论里），因此继续推进而不是升级。

方案 —— 删除那一半就是最小改动，连 lint 白名单条目和 supervisor-store.ts 里那句已经失效的 trustedHooks 注释都一并处理了。实现那一半我想稍微 push back 一下，是提问而不是阻断：你用来论证删除 TrustedHooksManager 的理由——没人用、不在包入口——对 unregisterSkillHooks 同样成立；而它要解决的"重新加载"问题已经由去重逻辑解决了，去重那段注释本身就写着卸载 skill 正文"从不注销其会话 hook"是有意设计。直接删掉这个桩函数，比实现并测试它大约少 30 行。反方理由我也理解（一个导出函数名不副实本身就是缺陷，而且总纲计划写的是 "implement"），所以我不要求你改——但如果维护者更希望看到它被删掉而不是被补全，那是更省的 PR，现在正是提出来的时机。

风险 —— 未命中高风险路径。与回滚相关的那组文件（geminiChat、shell.ts、shellExecutionService、mcp-client、acp-integration、sandbox.ts、relaunch.ts、流式解析器）都没有触及，因此不需要提升 review 深度。

进入代码审查 🔍

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at ae336e9f1dcff89465ca2fbc310e06172525855d · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Code review

My independent proposal (formed from the title and the "Why it's needed" section, before reading the diff): delete trustedHooks.ts and its test, drop the lint allowlist entry, fix any comment left pointing at it — and for unregisterSkillHooks, delete that too, on the same unused-and-not-in-the-package-entry argument, since the reload problem is already handled by the dedup guard in registerSkillHooks. If it is kept instead, key the removal on skillRoot and return early when the skill has no root.

Versus the diff: the deletion half is identical to my proposal, including the two hygiene details I'd have had to remind myself about (the allowlist entry, the dangling trustedHooks mention in a supervisor-store.ts comment). The unregisterSkillHooks half keeps the function rather than deleting it — the divergence I raised in the gate comment — but the implementation is what I would have written, guard included. No correctness blockers found.

What I checked, since this is core and the gate has to name every consumer:

• Iteration safety. getAllSessionHooks builds and returns a fresh array (allHooks.push(...)), while removeHook splices the per-event arrays inside the manager. So removing during the for…of cannot skip entries. Had it handed back the live arrays, this loop would have silently left every second matching hook behind — worth knowing, because that is the kind of bug a passing two-skill test would not catch.
• The !skill.skillRoot early return is load-bearing, not defensive padding. Without it, entry.skillRoot === skill.skillRoot degenerates to undefined === undefined and matches every rootless session hook — including /goal's Stop hook, which goalHook.ts:345 registers via addFunctionHook with no skillRoot. Unregistering a rootless skill would then quietly kill the goal loop's continuation check. The guard is correct and the test pins it.
• skillRoot is a sound identity. It is path.dirname of the skill's SKILL.md (skill-load.ts:205, extension/agent-plugins-v1/skills.ts:113), i.e. one directory per skill, so removal cannot reach into a neighbouring skill. And registerSkillHooks:102 is currently the only producer of skillRoot-tagged session hooks, so the match set equals that skill's own hooks.
• The count is honest. removeHook scans all events and returns a boolean; removed increments only on a true, so the return value equals entries actually removed.
• Nothing was orphaned. getHookKey, which trustedHooks.ts imported, survives via hookPlanner.ts:328 — the deletion does not leave a dead export behind.
• No lint/typecheck surprise. The removed vi.mock block was the only reason a reader might expect hookRegistry.ts to depend on trustedHooks.js; it never did. vi keeps 52 other uses in that file, so the import does not go unused. The allowlist feeds eslint.config.js:369 as ignores, so dropping the deleted file's entry is correct and cannot break lint either way.
• Consumers, exhaustively: TrustedHooksManager — zero production references. unregisterSkillHooks — zero callers; exported from the hooks/index.ts barrel, not from the package entry packages/core/src/index.ts. trusted_hooks.json — no remaining reader or writer, and since nothing ever constructed the manager, no on-disk state to migrate. The TRUSTED_HOOKS debug namespace — still created independently by six other files, unaffected. No docs, schema, or config outside hooks/ mention trusted hooks.

Test quality. The new cases assert the three things that matter — the return count, the identity of the surviving entry (['/skills/b'], not just a length), and the round-trip that re-registering the unregistered skill returns 1 rather than being swallowed by the dedup guard. That last assertion is what makes the test non-tautological. I also appreciate the PR saying outright that the rootless case passes on main too, instead of letting it look like new coverage.

Suggestions — neither blocks this:

1. The scope question from the gate comment stands: implement unregisterSkillHooks or delete it. Deleting is smaller and matches the argument this PR makes for deleting TrustedHooksManager. Keeping it is defensible; it is a maintainer's call, not mine.
2. No test pins the property the rootless guard actually protects — that unregistering a skill leaves a non-skill rootless session hook (a /goal Stop hook is the live example) untouched. The guard makes it true today; a test would keep it true if someone later "simplifies" the !skill.skillRoot condition away. That failure mode is silent and hits the goal loop, so it is the one assertion I would most want added.

No sequence diagram or changed-files table here — seven files, one deleted class, one filled-in stub, and no new runtime flow to trace. A diagram would be noise.

Test evidence

Stating plainly what this carries: static verification plus the PR's own CI. I read the base tree at ae336e9f1dcff89465ca2fbc310e06172525855d in an isolated worktree and the diff through the API. No PR-derived code was built, run, or tested by me — this is an unattended CI run, so executing fork code is out of bounds by rule. Nothing below is the author's self-report presented as evidence; where I cite the author's test counts, they are labelled as theirs.

The two claims that decide this PR are statically decidable, and I decided them by reading: "nothing constructs TrustedHooksManager" and "nothing calls unregisterSkillHooks" are repo-wide reference searches, both confirmed empty. There is no user-visible surface for a sandboxed lane to drive — no caller, no TUI change, no wire-format change — and an A/B against the base build would show no difference by construction. So the live-behaviour gap that normally needs /verify or /tmux does not exist here. What remains unsettled is only compile + unit + integration, which is exactly what CI covers.

At the time of this fetch: 0 failures, 0 cancellations. Green includes Integration Tests (no-AK, No Sandbox), both Desktop Shell jobs, TUI parity snapshots (ink vs opentui), OpenTUI no-flicker gate, and precheck-pr / precheck. Still in flight: Test (ubuntu-latest, Node 22.x) and Lint & Static (ubuntu-latest, Node 22.x) — the two jobs that actually matter for a deletion like this, since they are what would catch a dangling import or an unused-symbol error. Test (macos-latest) and Test (windows-latest) are skipped by workflow config, not failing. I did not poll; the table below is rewritten in place once CI settles.

For attribution, the author reports 90 passed (90) across registerSkillHooks.test.ts, hookRegistry.test.ts and sessionHooksManager.test.ts on Linux, and that reverting registerSkillHooks.ts to main makes the two-skill unregister case fail. That is their claim, not something I re-ran; the ubuntu Test job is what will confirm it.

Final CI results for ae336e9 (auto-updated by the triage finalize job after CI completed):

Check	Conclusion
Classify PR	✅ success
Desktop Shell (ubuntu-22.04)	✅ success
Desktop Shell (windows-2022)	✅ success
Integration Tests (no-AK, No Sandbox)	✅ success
Lint & Static (ubuntu-latest, Node 22.x)	✅ success
OpenTUI no-flicker gate	✅ success
Test (ubuntu-latest, Node 22.x)	✅ success
TUI parity snapshots (ink vs opentui)	✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x)	✅ success

_{One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。}

中文说明

代码审查

我的独立方案（只看标题和"为什么需要"就想好的，没有先看 diff）：删掉 trustedHooks.ts 及其测试、去掉 lint 白名单条目、修正残留的注释引用；至于 unregisterSkillHooks，同样按"没人用、不在包入口"的理由一起删掉，因为重新加载的问题已经由 registerSkillHooks 里的去重逻辑解决了。如果要保留，就按 skillRoot 匹配移除，并在 skill 没有根目录时提前返回。

与 diff 对比： 删除那一半和我的方案完全一致，连我本来还得提醒自己的两处细节（白名单条目、supervisor-store.ts 里那句失效的 trustedHooks 注释）都处理了。unregisterSkillHooks 那一半选择保留并实现而不是删除——就是我在门禁评论里提的那个分歧——但实现方式正是我会写的样子，包括那个提前返回的守卫。未发现正确性阻断问题。

因为这是核心路径，门禁必须点名每一个消费者，所以我逐项核过：

• 迭代安全。 getAllSessionHooks 构造并返回一个全新数组（allHooks.push(...)），而 removeHook 是在管理器内部对每个事件的数组做 splice。因此在 for…of 中移除不会漏掉任何条目。如果它返回的是活数组，这个循环会静默地每隔一个就漏掉一个匹配项——值得知道，因为这种 bug 一个"注册两个 skill"的通过测试是抓不到的。
• !skill.skillRoot 提前返回是承重的，不是防御性冗余。 没有它，entry.skillRoot === skill.skillRoot 会退化成 undefined === undefined，从而匹配每一个没有根目录的会话 hook——包括 /goal 的 Stop hook（goalHook.ts:345 通过 addFunctionHook 注册，不带 skillRoot）。那样一来，注销一个无根 skill 会悄悄干掉 goal 循环的续跑判定。守卫写法正确，测试也钉住了。
• skillRoot 是可靠的身份标识。 它是 skill 的 SKILL.md 的 path.dirname（skill-load.ts:205、extension/agent-plugins-v1/skills.ts:113），即每个 skill 一个目录，所以移除不会波及相邻 skill。而 registerSkillHooks:102 是目前唯一产生带 skillRoot 标记会话 hook 的地方，因此匹配集合就等于该 skill 自己的 hook。
• 计数是诚实的。 removeHook 扫描所有事件并返回布尔值；removed 只在 true 时自增，所以返回值等于实际移除的条目数。
• 没有留下孤儿导出。 trustedHooks.ts 引入的 getHookKey 仍被 hookPlanner.ts:328 使用，删除不会造成死导出。
• 没有 lint/类型意外。 被删掉的那个 vi.mock 块，是唯一会让人以为 hookRegistry.ts 依赖 trustedHooks.js 的东西；它从来没有依赖过。该文件里 vi 还有 52 处其他使用，import 不会变成未使用。白名单被 eslint.config.js:369 当作 ignores 使用，所以删掉已删除文件的条目是正确的，也不会以任何方式弄坏 lint。
• 消费者清单（穷尽）： TrustedHooksManager —— 生产代码零引用。unregisterSkillHooks —— 零调用方；从 hooks/index.ts barrel 导出，但没有从包入口 packages/core/src/index.ts 导出。trusted_hooks.json —— 已无读写方；而且既然从来没有代码构造过这个管理器，也就没有磁盘状态需要迁移。TRUSTED_HOOKS 调试命名空间 —— 另有六个文件各自独立创建，不受影响。hooks/ 之外没有任何文档、schema 或配置提到 trusted hooks。

测试质量。 新用例断言了三件真正重要的事——返回计数、存活条目的身份（['/skills/b']，而不只是长度）、以及"注销后重新注册返回 1 而没有被去重逻辑吞掉"这个往返性质。最后这条断言才是让测试不至于同义反复的关键。我也很欣赏 PR 直接说明无根 skill 那个用例在 main 上同样通过，而不是让它看起来像新增覆盖。

建议——两条都不阻断本 PR：

1. 门禁评论里的范围问题依然成立：unregisterSkillHooks 是实现它还是删掉它。删掉更小，也更符合本 PR 用来论证删除 TrustedHooksManager 的理由。保留也说得通；这是维护者的判断，不是我的。
2. 没有测试钉住无根守卫真正保护的那个性质——注销一个 skill 时，非 skill 的无根会话 hook（现成的例子就是 /goal 的 Stop hook）应保持不动。守卫让它今天成立；一个测试能让它在将来有人"顺手简化"掉 !skill.skillRoot 条件时仍然成立。那种失效是静默的、而且打在 goal 循环上，所以这是我最希望补上的一条断言。

这里没有放时序图或改动文件总览——七个文件、一个被删的类、一个被补全的桩函数，没有新的运行时流程需要追踪。放图只会是噪音。

测试证据

明确说明本节承载什么：静态核查 + PR 自身的 CI。 我在隔离 worktree 中读取了 ae336e9f1dcff89465ca2fbc310e06172525855d 的基线代码树，diff 通过 API 获取。我没有构建、运行或测试任何来自 PR 的代码——这是无人值守的 CI 运行，按规则禁止执行 fork 代码。下文没有任何一处是把作者自述当作证据呈现；凡引用作者的测试数字，都已标注为作者的声明。

决定这个 PR 的两条主张是静态可判定的，我也是靠阅读判定的："没有任何代码构造 TrustedHooksManager"和"没有任何代码调用 unregisterSkillHooks"都是全仓库引用搜索，结果均为空。不存在可供沙箱验证通道驱动的用户可见面——没有调用方、没有 TUI 改动、没有线格式改动——与基线构建做 A/B 对比在构造上就不会有差异。因此通常需要 /verify 或 /tmux 来填补的"真实行为"缺口在这里并不存在。剩下未定的只有编译、单测和集成测试，而这正是 CI 覆盖的范围。

抓取时刻的状态：0 失败、0 取消。 已通过包括 Integration Tests (no-AK, No Sandbox)、两个 Desktop Shell 作业、TUI parity snapshots (ink vs opentui)、OpenTUI no-flicker gate 和 precheck-pr / precheck。仍在运行：Test (ubuntu-latest, Node 22.x) 和 Lint & Static (ubuntu-latest, Node 22.x)——对这种删除类改动，这两个作业才是真正关键的，因为悬空 import 或未使用符号的错误只会被它们抓到。Test (macos-latest) 和 Test (windows-latest) 是工作流配置跳过，不是失败。我没有轮询；CI 稳定后下方表格会被就地改写。

关于出处：作者报告在 Linux 上 registerSkillHooks.test.ts、hookRegistry.test.ts、sessionHooksManager.test.ts 共 90 passed (90)，并称把 registerSkillHooks.ts 换回 main 后"两个 skill 注销其一"的用例会失败。这是作者的声明，不是我自己复跑的结果；ubuntu 的 Test 作业才是确认它的依据。

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at ae336e9f1dcff89465ca2fbc310e06172525855d · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

Confidence: 4/5 — clean at every stage; the two open items are a scope question and a missing test, and neither blocks.

Going back to my independent proposal: the deletion half is exactly what I would have written, and the implementation half is what I would have written if the function were being kept. The one place I'd have chosen differently is whether to keep it at all, and that is a maintainer's call, not a defect.

What I like about this diff is what it did not do. The obvious over-engineered version of unregisterSkillHooks is a per-skill hook-ID registry — new state on SessionHooksManager, new bookkeeping in registerSkillHooks, new invariants to keep in sync. Instead it reuses the skillRoot field already sitting on SessionHookEntry and two methods that already existed. Twenty-two net lines in the live file, no new abstraction, no new state. That is the right instinct and it is not the common one.

The diff is also minimal in the boring sense: no drive-by refactors, no formatting churn, no unrelated files. The two hygiene edits it does carry — the lint allowlist entry and the stale trustedHooks mention in a comment two packages over — are things most authors would have left for someone else to trip over.

On the "did I verify the problem exists" question: yes, by search rather than by trusting the framing. Both claims reduce to repo-wide reference searches, and both came back empty. That is about as solid as a dead-code claim gets.

On volume, since I should say it out loud: this author has 17 open PRs, 8 of them filed today against packages/core from the same #11610 umbrella. I checked whether that was wearing down my judgement on this one, and I don't think it was — this PR's claims are falsifiable by grep rather than by taste, and its runtime blast radius is zero because nothing calls the function it changes and nothing constructed the class it deletes. Judging it on its own diff is the right call. But a maintainer may reasonably want to sequence the umbrella rather than take eight core PRs in a day, and #11610 does carry need-discussion. That is a comment about the batch, not a reservation about this PR.

What I'd still like, non-blocking: a test that unregistering a skill leaves a non-skill rootless session hook alone — a /goal Stop hook is the live example. The !skill.skillRoot guard makes that true today, and it is the single condition whose removal would silently break the goal loop. Everything else is a scope preference.

CI status at the time of writing: Test (ubuntu-latest, Node 22.x) and Lint & Static (ubuntu-latest, Node 22.x) are still running; 0 failures so far. Those two are the jobs that would catch a dangling import or an unused symbol after a file deletion, so I am not approving on a partial result. Approval is deferred until CI lands green on ae336e9f1dcff89465ca2fbc310e06172525855d, and it will be recorded against that exact commit.

中文说明

置信度：4/5 —— 各阶段都干净；两个待议项是一个范围问题和一个缺失的测试，都不构成阻断。

回到我的独立方案：删除那一半正是我会写的样子；实现那一半，如果决定保留这个函数，也正是我会写的样子。我唯一会做不同选择的地方是"要不要保留它"，而这是维护者的判断，不是缺陷。

这个 diff 让我欣赏的是它没有做什么。unregisterSkillHooks 显而易见的过度设计版本是一套按 skill 记录 hook ID 的注册表——在 SessionHooksManager 上加新状态、在 registerSkillHooks 里加新记账、再加一堆需要同步维护的不变量。而它选择复用 SessionHookEntry 上本来就有的 skillRoot 字段和两个已存在的方法。生产文件净增 22 行，没有新抽象，没有新状态。这是对的直觉，而且不是常见的直觉。

diff 在"无聊"的意义上也是最少的：没有顺手重构，没有格式化噪音，没有无关文件。它确实带上的两处清理——lint 白名单条目，以及跨了两个包、注释里那句失效的 trustedHooks——正是多数作者会留给别人去踩的东西。

关于"我是否核实了问题真的存在"：核实了，靠搜索而不是靠相信 PR 的叙述。两条主张都归结为全仓库引用搜索，结果都是空。死代码类的主张能确证到这个程度，已经接近上限。

关于数量，我应该直说： 这位作者有 17 个开着的 PR，其中 8 个是今天针对 packages/core 提交的，都出自同一个 #11610 总纲。我检查了这是否在削弱我对本 PR 的判断，我认为没有——本 PR 的主张可以用 grep 证伪，而不是靠品味；它的运行时影响面为零，因为它改动的那个函数没有调用方，它删除的那个类没有构造点。按它自己的 diff 来评判是正确的做法。但维护者完全可能希望把这个总纲的 PR 排个序，而不是一天收八个核心 PR，而且 #11610 本身带着 need-discussion 标签。这是针对"这一批"的看法，不是对本 PR 的保留意见。

仍然希望补上、但不阻断： 一个测试，验证注销某个 skill 时非 skill 的无根会话 hook 保持不动——现成的例子就是 /goal 的 Stop hook。!skill.skillRoot 这个守卫让它今天成立，而它是唯一一个被去掉后会静默打断 goal 循环的条件。其余都属于范围偏好。

撰写时的 CI 状态： Test (ubuntu-latest, Node 22.x) 和 Lint & Static (ubuntu-latest, Node 22.x) 仍在运行；目前 0 失败。删除文件之后，能抓到悬空 import 或未使用符号的正是这两个作业，所以我不会基于部分结果批准。批准将推迟到 CI 在 ae336e9f1dcff89465ca2fbc310e06172525855d 上全绿之后，并且会记录在这个确切的提交上。

— Qwen Code · qwen3.8-max-2026-09-02

_{Reviewed at ae336e9f1dcff89465ca2fbc310e06172525855d · re-run with @qwen-code /triage}

[qwen-code-ci-bot]

LGTM, looks ready to ship — CI landed green after the review. ✅

[qwen-code-ci-bot]

Reviewed — no blockers. Suggestions are inline.

Test Plan (not a blocker): Tests 90 passed — this review observed 30525, 25020, 2018, 1016, 1984, 535, 7319 passed.

中文说明

已审查——无阻断问题。 建议见行内评论。

Test Plan（非阻断）：Tests 90 passed — this review observed 30525, 25020, 2018, 1016, 1984, 535, 7319 passed。

— qwen3.8-max via Qwen Code /review (v0.23.3)

[yiliang114]

Reviewed at head 6bf38952 (7 files, +124/-286). No blocking finding. Approving.

For a PR that deletes a module and implements a previously missing function, the two things that can go wrong are a dangling reference and a mutation-during-iteration bug. I checked both rather than assuming.

The deletion is clean. git grep -n "trustedHooks\|TrustedHooks" at head across packages/, scripts/ and eslint.legacy-filenames.mjs returns zero hits, so removing trustedHooks.ts (-132) and its test (-129) leaves nothing importing, re-exporting or lint-allowlisting it. The eslint.legacy-filenames.mjs entry went with the file (-1), which is the detail usually missed.

unregisterSkillHooks collects before it mutates, and says why. registerSkillHooks.ts:174-196 early-returns 0 when !skill.skillRoot, then materialises the id list first — const hookIds = sessionHooksManager.getAllSessionHooks(sessionId).filter((entry) => entry.skillRoot === skill.skillRoot).map((entry) => entry.hookId); — with the comment "Collect the ids first: removeHook splices the stored per-event arrays." That is the correct order: filtering a live array while removeHook splices it would skip entries and silently leave hooks registered, which for an unregister function is the one failure that matters. It then counts actual removals rather than assumed ones (if (sessionHooksManager.removeHook(sessionId, hookId)) removed++) and returns the count, so a caller can tell "nothing was registered" from "removed N". Matching by skillRoot rather than by skill name is also the right key, since it is what registration is scoped by.

What I did not verify: the +96 test lines' assertions, and whether every caller of the old trusted-hooks manager had already stopped using it before this PR (the zero-grep result makes a dangling reference impossible, but not a caller that was silently relying on behaviour the manager provided). packages/cli/src/agent-view/supervisor-store.ts (+3/-2) is the only non-hooks production file touched and I did not read it. CI at head: green apart from review-pr. No review threads. This touches /packages/core/, so this is the code-owner vote.

中文说明

在 head 6bf38952 上评审（7 个文件，+124/-286）。没有阻塞项。 批准。删除类 PR 会出错的地方就两处：悬空引用，以及边遍历边修改。我都查了。删除是干净的：head 上对 trustedHooks|TrustedHooks 在 packages/、scripts/、eslint.legacy-filenames.mjs 下 grep 零命中，且 eslint.legacy-filenames.mjs 里的条目随文件一起删掉了（-1，这通常是被漏掉的细节）。unregisterSkillHooks 先收集再修改，并写明了原因：registerSkillHooks.ts:174-196 在 !skill.skillRoot 时返回 0，然后先把 id 物化成数组，注释是「先收集 id：removeHook 会对按事件存储的数组做 splice」——顺序是对的，因为一边 splice 一边过滤活数组会跳过条目、静默留下已注册的 hook，而对一个注销函数来说这正是唯一要紧的失败。它随后统计实际移除数而不是假定数，并返回该计数，所以调用方能区分「本来没注册」与「移除了 N 个」。按 skillRoot 而非技能名匹配也是正确的键。未验证：+96 行测试的断言，以及旧 manager 的每个调用方是否在本 PR 之前就已停用（零 grep 排除了悬空引用，但排除不了某个调用方静默依赖该 manager 提供的行为）；supervisor-store.ts（+3/-2）是唯一被触碰的非 hooks 生产文件，我没有读。CI 除 review-pr 外全绿，无 review 线程。本 PR 触及 /packages/core/，所以这一票是 code-owner 票。

[qwen-code-dev-bot]

Approved at head 6bf38952.

Required CI is green — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox) and web-shell E2E Smoke completed successfully; only review-pr (the reviewer pipeline's own job) is outstanding. All three review threads are resolved, none a Critical.

I ran the package locally: packages/core src/hooks/*.test.ts at this head passes 24 files / 790 tests, including the new two-skill case that unregisters one root and checks the other skill's hook survives and the first can be registered again.

The replacement behavior is right for the reason that matters. Removal now keys on skillRoot alone, which is what makes it still work when the SkillConfig handed in no longer lists the hooks it registered earlier — keying on skill.hooks would leave exactly the case a caller needs unremovable. Registration does pass the root (registerSkillHooks.ts:102, { skillRoot, trustGated }), so the filter has real data to match against rather than being a well-written no-op, and a skill with no root returns 0 instead of deleting another skill's hooks. Collecting the ids before the removal loop is necessary rather than tidy: removeHook splices the stored per-event arrays, so filtering and deleting in one pass would skip entries.

The deletion side is clean: git grep -i trustedhooks at this head returns nothing anywhere in the repository, where the base carried it in five places — the module, its 129 lines of tests, a reference in hookRegistry.test.ts, the agent-view comment that cited it as a credential-writer precedent, and its entry in the legacy-filename lint allowlist. It was never re-exported from the hooks barrel, which is the proof that nothing depended on it. The comment that pointed at it now names writers that actually exist (supervisor-store.ts:697), which is the right resolution of the round-one note about that sentence going ungrammatical: the reference is replaced, not dropped.

No new Critical found. Two small things worth an answer rather than a round:

1. Both new cases register one hook on one event per skill, so the tests would still pass against an implementation that only scans a single event or stops after the first match. The code is broader than that — it filters every entry from getAllSessionHooks — but a case with one skill owning two events × two hooks is what pins "all of them", and it is cheap.
2. The !skill.skillRoot early return is now the only guard, and its pair — the !skill.hooks half — is gone rather than merged into the new path. That is intentional per the doc comment, and the no-root case is tested; worth keeping in mind if a caller ever starts passing a root-less skill expecting a no-op that is also reported as a no-op, since the return value is now the only signal either way.

[qqqys]

两条非阻塞说明的处理：

1. 多事件、多 hook 覆盖：已有用例覆盖了这一点，不需要再补。registerSkillHooks.test.ts 里的 "removes every hook the skill registered across events" 让同一个技能在 PreToolUse 和 PostToolUse 各注册一个 hook，另一个技能在 PreToolUse 也注册一个，然后断言：

   • unregisterSkillHooks 返回 2；
   • 两个事件里只剩另一个技能的 hook。

   只扫描单个事件的实现会让 PostToolUse 残留并返回 1，遇到第一个匹配就停的实现也返回 1，这两种都会被这条用例挡住。实现是先对 getAllSessionHooks 拉平的整张表按 skillRoot 过滤、收集 id，再逐个删除，不存在按事件分别计数的分支，所以同一事件放两个 hook 走的是同一条路径。首版 ae336e9 的两条用例确实都只在一个事件上注册一个 hook，首轮修复 6bf3895 补上了这条跨事件用例，所以评审时的 head 上已经有了。

2. !skill.skillRoot 早返回：这是有意的。没有根目录的技能注册出来的 hook 不带 skillRoot，没法和其它无根技能的 hook 区分，所以返回 0、什么都不删，这比误删别人的 hook 安全。"removes nothing for a skill without a root directory" 钉住了这个行为。目前 unregisterSkillHooks 在生产代码里还没有调用方，只从 hooks 入口导出。等将来技能卸载或重载路径接上它时，如果调用方需要区分"本来没注册"和"无法识别"，再在那个 PR 里调整返回值。

[qwen-code-ci-bot]

Released in v0.23.4.