feat(cli): reload hook registry when /hooks opens - #11904
Conversation
Hooks were captured once at startup, so a hook added, changed or removed in a settings file had no effect until restart. Opening the /hooks menu now re-reads the settings files, hands the resolved hook fields to Config via a new setHooksFromSettings, and reloads the hook registry before the menu renders. The hook fields are resolved by one helper shared with loadCliConfig, so the reload applies the startup rules: bare and safe mode load no hooks, user hooks fall back to the merged hooks setting, and project hooks load only in a trusted folder. All three Config fields are replaced together so a stale legacy snapshot cannot come back through the getter fallback. Settings are re-read without reloading the environment or consuming the startup corruption markers. A failed reload is logged and the menu still opens. The non-interactive /hooks list is unchanged.
|
Thanks for the PR — the write-up is unusually thorough, and the mutation table is a nice touch. Template looks good ✓ — every section filled in, the OS table honestly marked (Linux ✅, macOS/Windows Problem: real, and I confirmed it in the base tree rather than taking the description's word for it. Direction: the user-facing goal is squarely in scope — needing a restart to pick up a hook edit is a real papercut, and Size: core paths are touched ( Approach: the plumbing is right; the trigger is the question I can't settle from the diff.
But the repo already holds most of a hands-free version of this:
The The counter-argument is real and I want to state it fairly rather than let it read as an oversight: hooks are arbitrary command execution, and the MCP path needed approval gating ( Two smaller notes:
Risk: no elevated risk signals — none of the changed files match the revert-correlated high-risk paths. One neighbour worth a clause: Moving on to code review, and flagging the trigger question for a second human rather than settling it here. 🔍 中文说明感谢贡献!PR 描述写得非常扎实,定点变异(mutation)表格尤其加分。 模板完整 ✓ —— 各必填小节都写了,测试平台表格标注诚实(Linux ✅,macOS/Windows 问题: 真实存在,我在 base 代码里核对过,不是仅凭描述判断。 方向: 用户价值明确在范围内——改个 hook 要重启确实是痛点, 规模: 触及核心路径( 方案: 管道部分是对的;触发时机是我无法只凭 diff 判定的问题。
但仓库里其实已经有大半个「无需手势」的版本:
本 PR 新增的 反方理由同样真实,我想公平地说出来,而不是让它看起来像疏漏:hook 是任意命令执行,MCP 那条路径正是先有了审批闸门( 另外两点小提醒:
风险: 无升级风险信号——改动文件均未命中与回滚相关的高风险路径。有一个相邻点值得加一句限定: 进入代码审查;触发时机这个问题我不在此处定论,转交另一位维护者看一眼。🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo critical blockers. I traced the change end to end against the base tree and the core claims in the description hold up:
I also checked and dismissed two things that looked suspicious at first: Three notes, none blocking: 1. Two settings trees per menu open. The command calls a fresh 2. The menu line over-promises slightly. "Hooks are reloaded from settings files each time this menu opens" is true for the hook maps, but 3. Pre-existing, not introduced here — flagging only because this PR creates the place to fix it. The ordering invariant is the subtle part of this change, so here's the path it has to hold on: sequenceDiagram
participant P1 as User
participant P2 as hooksCommand action
participant P3 as loadSettings
participant P4 as Config
participant P5 as HookRegistry
P1->>P2: opens the hooks menu
P2->>P4: getHookSystem, early return when absent
P2->>P3: re-read settings, skipLoadEnvironment true
P3-->>P2: fresh user, project and merged hooks
P2->>P4: setHooksFromSettings, all three fields together
P2->>P5: reload through HookSystem
P5->>P4: getUserHooks and getProjectHooks, gates re-applied
P5-->>P5: keep agent-scoped entries, restore enabled state
P2-->>P1: menu opens even if any step threw
Test evidenceThis is an unattended CI run, so per the gate's rules I did not build, run, or test any of this PR's code — no Nothing is red. Lint & Static, both integration-test lanes that ran, TUI parity snapshots, the OpenTUI no-flicker gate, both Desktop Shell jobs and the daemon E2E all completed green. The only unfinished item at fetch time was the ubuntu unit suite ( One gap worth naming: Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 What the tests do and don't settle. The new suites are good at what they pin: Sandboxed verification would settle it, and both lanes are available since the author has write access:
中文说明代码审查没有发现阻断性问题。我对着 base 代码把改动端到端追了一遍,描述里的核心论断都成立:
另外有两处一开始看着可疑,我核对后排除了: 三点提醒,均不阻断: 1. 每次开菜单读两棵设置树。 命令里新调了一次 2. 菜单文案略微超出实际。 「Hooks are reloaded from settings files each time this menu opens」对 hook 映射是成立的,但 3. 既有问题,非本 PR 引入——之所以提,是因为本 PR 恰好造出了修它的地方。 顺序不变式是这次改动里最微妙的部分,上面用时序图画出了它必须成立的路径: 测试证据这是一次无人值守的 CI 运行,因此按门禁规则,我没有构建、运行或测试本 PR 的任何代码——没有 没有红色项。Lint & Static、两个实际运行的集成测试通道、TUI parity 快照、OpenTUI 无闪烁闸门、两个 Desktop Shell 作业以及 daemon E2E 全部绿色完成。取数时唯一未完成的是 ubuntu 单元测试( 有一个缺口值得点名: 测试能证明什么、不能证明什么。 新增用例在它们钉住的范围内是好的: 沙箱化验证可以定论,且作者有写权限,两条通道都可用:
— Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — the code is clean and I found no blockers; one direction question I can't settle from the diff, and CI hasn't landed. Stepping back: this is good work, and I want to be clear that the 3/5 is not a judgement on code quality. My independent proposal before reading the diff was essentially what you built — a setter on If I had to maintain this in six months I'd thank whoever wrote it. The helper is small, the command is a try/catch around one call, and the comment explaining why So here's the honest reservation, and it's the only thing between this and an approve. Six months from now, someone will ask "why do hooks reload when I open a menu?" and the answer won't be in the code or the description. I don't think that makes the current shape wrong. I said it in Stage 1 and I'll repeat it because it's the strongest argument for your design: hooks are arbitrary command execution, and the MCP path only became safe to auto-reconcile once approval gating existed. Auto-arming on file save means a Two smaller things I'd want before merge, neither blocking on its own: the menu line promises a reload that Where that leaves the verdict: deferring, not requesting changes. I'm not approving in this run for two reasons — the direction question above, and To be concrete about how this unblocks, since it's a short path: if the menu-open trigger is deliberate — which I suspect — one or two sentences in the description saying so, and why the watcher route was rejected, resolves my only substantive concern. Add the qualifier to the menu string and pick an owner for the docs section, and a re-run of One thing I'd flag for whoever picks this up: the behavioural claim at the centre of the PR — a hook added after startup fires once Escalating to @BenGuanRan for the direction call. How I resolved the mention: 中文说明Confidence: 3/5 —— 代码干净,我没找到阻断性问题;但有一个我无法只凭 diff 判定的方向问题,而且 CI 还没跑完。 退一步看整体:这是一份质量很高的工作,我想说清楚 3/5 并不是对代码质量的评价。在读 diff 之前我自己设想的方案,基本上就是你写出来的东西——在 如果半年后由我来维护这段代码,我会感谢写它的人。helper 很小,命令部分就是一个 try/catch 包一次调用,而解释为什么要传 所以,下面是我真实的保留意见,也是本 PR 与 approve 之间唯一的东西。 半年之后有人会问「为什么 hook 是在我打开菜单时重新加载的?」,而答案既不在代码里,也不在描述里。schema 里 我并不认为现在的形态是错的。Stage 1 里我说过,这里再说一遍,因为这是支持你设计的最强论据:hook 是任意命令执行,MCP 那条路径是在有了审批闸门之后才敢自动对账的。文件保存即自动装载,意味着一次 合入前我还希望处理两件较小的事,单独看都不阻断:菜单文案承诺了一次 结论落在哪里:转交(defer),不是要求修改(request changes)。 本次运行我不 approve,有两个原因——上面的方向问题,以及我取数时 具体说清楚怎么解围,因为路径很短:如果打开菜单这个触发时机是刻意为之(我猜是的),在描述里加一两句说明为什么、以及为什么否掉了 watcher 方案,就能解决我唯一的实质顾虑。再给菜单文案加上限定、给文档小节定下归属,那么在 CI 变绿后重跑一次 有一件事我要提醒接手的人:本 PR 核心的行为论断——启动之后新增的 hook,在打开过一次 方向决策转交 @BenGuanRan。说明这个 mention 是怎么定下来的: — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued for an available runner; follow the workflow run for progress. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R1-7 reload drops the freshly re-read disableAllHooks kill switch (packages/cli/src/ui/commands/hooksCommand.ts:54) — already reported (comment 5674232502)
- R1-8 reload hangs off a menu call site instead of the SettingsWatcher hot-reload plumbing (packages/cli/src/ui/commands/hooksCommand.ts:247) — already reported and escalated to a maintainer (comment 5674301136)
Test Plan (not a blocker): code-mode/host.ts — no such file or directory; 5 passed — this review observed 31333, 26407, 2108, 1028, 2040, 587, 8138 passed; 30 passed — this review observed 31333, 26407, 2108, 1028, 2040, 587, 8138 passed; 41 passed — this review observed 31333, 26407, 2108, 1028, 2040, 587, 8138 passed.
中文说明
本轮确认的 2 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。
Test Plan(非阻断):code-mode/host.ts — no such file or directory; 5 passed — this review observed 31333, 26407, 2108, 1028, 2040, 587, 8138 passed; 30 passed — this review observed 31333, 26407, 2108, 1028, 2040, 587, 8138 passed; 41 passed — this review observed 31333, 26407, 2108, 1028, 2040, 587, 8138 passed。
— qwen3.8-max via Qwen Code /review (v0.23.3)
|
Repair validation at
The two earlier triage suggestions are addressed in the updated description and guide: reload deliberately requires opening the menu rather than arming arbitrary commands on a file-save watcher; The pre-existing Ink right-border clipping remains outside this text repair; the new notice itself was verified fully visible. Fresh hosted CI and review are still separate from these local checks. |
doudouOUC
left a comment
There was a problem hiding this comment.
Traced the reload path and its side-effect surface at the PR head. No blocking issues found.
- The reload reuses
resolveHookSettingsForConfig, the same resolver startup uses, so the/hooksview and the running session cannot disagree on the bare/safe-mode rule or on the user-hooks fallback to the mergedhookssetting.Config.setHooksFromSettingsreplaces all three maps together, which is what stops a stale legacyhookssnapshot from resurfacing through the getters' fallback — the same invariant the constructor establishes. - The load is deliberately side-effect-free:
loadSettingsis called withskipLoadEnvironment: trueandconsumeCorruptionEnvVars: false, and I verified both options are real and honoured at the head (settings.ts:1265guards the environment load,:1061guards the corruption-marker consumption), so opening the menu cannot re-run the.envbootstrap or consume a one-shot startup marker.preResolveHomeEnvOverrides()still runs, but it is idempotent by construction — it re-applies a resolution the process already performed. - Trust is enforced in depth: project hooks come from
settings.getProjectHooks()(withheld for an untrusted folder at the source) and theConfiggetters still apply their own bare/safe-mode and folder-trust gates, so a reload cannot widen the surface for an untrusted workspace. - Failure is contained: the reload is wrapped so a malformed settings file logs and the menu still opens on whatever is loaded, and session-scoped hooks (skills, SDK,
/goal) live outside the registry and are correctly documented as unaffected.
The notice was one locale key whose English value carried a newline so the menu would break the line. `check-i18n` requires the English value to match its key byte for byte, so CI failed, and every translation encoded its own line breaks, which made layout part of the message data. Split it into two sentence-level keys and let the views place the break: Ink renders one Text per sentence, and the OpenTUI dialog renders one text row per line of its notice prop.
|
Fixed at
Verified locally on Node 22: |
yiliang114
left a comment
There was a problem hiding this comment.
LGTM at d2a2354 — no blocking issues. Both Criticals addressed at this head and spot-verified: the /hooks reload now consumes the session's LoadedSettings (hooksCommand.ts takes LoadedSettings from CommandContext) instead of a fresh loadSettings() that would silently recover over a corrupt file or read the wrong roots in a --worktree session. All threads resolved. CI in flight; merge on green.
qwen-code-dev-bot
left a comment
There was a problem hiding this comment.
APPROVE
核对基线:head d2a235474522136a51fcd56415de046c5e5b5fd3(base 3fc1133d,25 个文件 +831/-27)。required 档全部完成且成功(Test (ubuntu-latest, Node 22.x)、Lint & Static、Integration Tests (no-AK, No Sandbox)、TUI parity snapshots、OpenTUI no-flicker gate 等 13 项 pass,其余按改动面跳过),无 pending、无失败。6 条线程 0 open,其中 2 条 [Critical] 我按当前 head 复核。
两条 Critical 都是真改,不是补文案:
- R1-1(把
loadSettings()当只读用):这条路径现在完全不碰它 ——hooksCommand.ts与hook-settings.ts里loadSettings/getWorkingDir的引用数为 0,reload 走settings.reloadScopesFromDiskAtomically([User, Workspace])。那个方法(config/settings.ts:813)先把两个作用域的settings/originalSettings/rawJson做structuredClone快照,任一作用域重载失败就整体回滚并返回false,调用方随后抛出一条「Settings could not be read; the previous hooks are still active.」,Config与注册表都没被动过;磁盘上的坏字节与.corrupted状态都不由这条路径产生。 - R1-2(
--worktree会话读错目录,首开就把整批项目 hook 丢掉):reload 改用会话里那份LoadedSettings,不再按process.cwd()重启设置,因此保留的是启动时的原始路径;hooks-command-reload.test.ts用真实文件把 worktree cwd 设成别的目录,验证原项目文件里的 hook 确实替换了活动的那份。
其余我关心的面也过了一遍:resolveHookSettingsForConfig 与启动期同一套解析(bare/safe mode 不加载、项目 hook 只在受信目录加载),skills/SDK//goal 注册的会话级 hook 在注册表之外、不受 reload 影响;两份提示文案(重开菜单才会重载、HTTP 安全项需重启)在 ink 与 OpenTUI 两侧都存在且各有正反用例(HooksListStep.test.tsx:156-169、opentui-dialog-mount.test.tsx:329),不是只在一边加话。没有发现新的阻塞问题。
What this PR does
Opening the interactive
/hooksmenu refreshes hook definitions from the user and workspace settings files used by the current session, then rebuilds the running registry. Adding, editing or removing a hook therefore takes effect after opening the menu, without restarting. Ink and OpenTUI show the same qualified reload notice only when the session has a hook system. The non-interactive listing remains a read of the current registry.The reload uses the session's existing settings instance, so a
--worktreesession still reads its original settings paths. User and workspace scopes refresh atomically: if either file cannot be read or parsed, the previous settings and running hooks remain active, the file is not rewritten, and an error is shown. The Ink menu reads that same settings instance rather than loading the files a second time.Why it's needed
The menu could show a settings edit while the executing registry still held its startup snapshot. Opening the menu now updates both views together. The explicit menu-open trigger is intentional: hook definitions can execute arbitrary commands, so saving a file, pulling changes or switching branches should not silently arm new commands. This change reuses the existing atomic settings reader without adding a watcher that activates hooks on save.
The startup rules still apply: bare and safe modes load no hooks, and project hooks are withheld in untrusted folders. The reload covers hook definitions only. Changes to
hooks.disableAllHooks,hooks.stopHookBlockingCap,security.allowedHttpHookUrlsandsecurity.allowPrivateNetworkHooksretain their startup behavior and require a restart. Session hooks registered by skills or the SDK are unaffected.Reviewer Test Plan
How to verify
/hooks. The new hook should run at its next matching event. Editing or removing a definition should update the running registry on the next menu open.--worktreesession, editing the original workspace's settings. The edit should load from that original path./hooksshould report an error, retain both previous settings snapshots and active hooks, preserve the invalid file byte-for-byte, and create no.corruptedsibling./hooks listshould display the current registry without refreshing settings. In an interactive terminal,/hooks listopens the same menu as/hooks.Evidence (Before & After)
Real local settings, configuration and hook-system probes reproduced both blockers before the repair. In a worktree, the old reload lost the original project hook and missed its edit. Malformed user or workspace JSON was overwritten with
{}, and the running hook set changed without an error notice.The repaired path preserves the malformed bytes, creates no corruption backup, performs no hook-field update on failure, and emits an error. Executable local marker hooks confirm that valid edits and worktree edits activate the new hook, while malformed edits retain the original executing hook. These checks use temporary settings directories and local commands, with no model request.
Node 22 validation: 281 relevant unit tests passed, including real-file reload regressions, startup hook gating, registry/system tests, Ink rendering, OpenTUI notice routing and locale coverage. Repository build, typecheck, lint and read-only formatting checks passed. After synchronizing the latest main, the six directly affected CLI suites passed again (62 tests).
A real tmux session running the candidate v0.23.4 bundle showed one configured hook, retained that hook and displayed the reload error after a malformed edit, then showed two hooks after the file was corrected. Escape closed the menu and it reopened normally. A disabled session showed the disabled view with no reload notice. At 120×48, both lines of the final notice are fully visible, including the restart requirement; all nine translations use explicit short lines. The pre-existing Ink right-border clipping was observed and remains outside this text repair.
Tested on
Environment (optional)
macOS, Node 22.17.0, isolated settings and worktree. Hosted CI and fresh review results are tracked separately from local verification.
Risk & Scope
Linked Issues
Part of #11610
中文说明
打开交互式
/hooks菜单会从会话原有的用户与项目设置路径原子重读 hook 定义,再更新执行中的注册表。--worktree改变运行目录后仍读取原设置文件。任一文件读取或解析失败时,两份设置快照和原 hooks 保持不变,不覆盖文件、不创建.corrupted,并显示错误;Ink 菜单复用同一份会话设置,不再二次加载文件。菜单打开是有意保留的显式激活动作:保存文件、拉取代码或切换分支不会自动启用新增命令。重载仅覆盖 hook 定义;总开关、Stop 阻断次数和 HTTP 安全设置仍需重启。非交互
/hooks list只显示当前注册表。Ink 与 OpenTUI 均仅在 HookSystem 存在时显示同一条限定说明。已用真实文件与实际执行的本地标记 hook 验证 worktree 编辑生效、损坏文件原样保留和旧 hooks 继续运行;281 个相关测试通过,完整构建、类型检查、lint 和格式检查通过。同步最新 main 后,直接相关的六个 CLI 测试文件再次通过(62 个测试)。远端 CI 与新 review 状态单独跟踪。