Skip to content

chore(core): cover every hook event in the settings schema and hook bus - #11620

Merged
qqqys merged 5 commits into
QwenLM:mainfrom
qqqys:chore/hook-bus-and-schema-events
Sep 11, 2026
Merged

chore(core): cover every hook event in the settings schema and hook bus#11620
qqqys merged 5 commits into
QwenLM:mainfrom
qqqys:chore/hook-bus-and-schema-events

Conversation

@qqqys

@qqqys qqqys commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

The settings schema and the hook execution message bus now cover every hook event. The hooks setting gains entries for PostCompact, PermissionDenied, TodoCreated, TodoCompleted and InstructionsLoaded, so editors that validate against the generated JSON schema stop flagging them as unknown keys and show a description. Like the other 17 events, the new entries use the CONCAT merge strategy, so the merged hooks setting keeps these five events' definitions from every scope instead of letting the highest-precedence scope replace the others. The bus dispatch gains cases for SessionStart, SessionEnd, SessionDelete, PreCompact, PostCompact, InstructionsLoaded, StopFailure, TodoCreated and TodoCompleted, which previously fell through to "Unknown hook event". For the last three, whose fire methods return an aggregated result, the bus replies with the final hook output as is: for TodoCreated and TodoCompleted that is what their direct callers read, and for StopFailure the aggregator discards hook outputs, so the reply is always empty.

A schema test now asserts that the hooks setting declares exactly the events in the HookEventName enum and that every one of them concatenates across scopes, and a settings loading test pins the concatenation, so a future event cannot be added to one without the other or silently fall back to replacing. A bus test iterates the same enum and requires every event to reach a hook system method, so a new event cannot fall through to the unknown-event default either.

Why it's needed

The runtime already accepted all 22 events in settings, but the schema listed 17, so valid configurations looked wrong in editors and the settings docs generator had nothing to describe for the other five. Because the settings merge looks up each key's merge strategy in the schema, the five missing events also merged differently from the rest: the highest-precedence scope replaced the others. The bus handled 14 events, so anything that dispatches hooks through it, rather than calling the hook system directly, could not fire the other eight at all. No current caller relies on the new bus cases; they close the gap so the two paths stay interchangeable.

Reviewer Test Plan

How to verify

  • cd packages/cli && npx vitest run src/config/settingsSchema.test.ts includes the checks that the schema's hook events equal the enum and that every hook event uses CONCAT.
  • cd packages/cli && npx vitest run src/config/settings.test.ts -t "concatenate hook definitions" loads user and workspace settings that both define PostCompact and TodoCreated hooks and checks that the merged settings keep both scopes' definitions in order.
  • cd packages/core && npx vitest run src/config/config.test.ts -t "hook execution bridge" routes every hook event through the bus against a stubbed hook system, and for the nine new cases checks the forwarded arguments and abort signal, the reply when no hook is configured, the merged final output for TodoCreated and TodoCompleted, and the empty reply for StopFailure.
  • npm run generate:settings-schema reproduces the committed JSON schema.

Evidence (Before & After)

The user-visible change is the settings merge for the five new schema keys; the rest affects editor validation and bus dispatch that no current caller uses. The evidence is the unit tests and the regenerated schema.

$ cd packages/cli && npx vitest run src/config/settingsSchema.test.ts src/config/settings.test.ts
 Test Files  2 passed (2)
      Tests  255 passed (255)

$ cd packages/core && npx vitest run src/config/config.test.ts -t "hook execution bridge"
 Tests  50 passed | 669 skipped (719)

Each new bus assertion was checked by breaking the code it pins, then restoring it: adding an enum member without a bus case fails the coverage test, forwarding a different signal field fails 15 cases, and normalizing an empty SessionEnd result, replying with the first Todo hook's output instead of the merged one, publishing an output for StopFailure, and a non-AgentType value in the SessionStart example each fail their case. The full config.test.ts passes (719 tests).

With the CONCAT strategy removed from the PostCompact and TodoCreated schema entries, which is how main merges them, both new tests fail:

 × SettingsSchema > getSettingsSchema > should concatenate every hook event across settings scopes
 × Settings Loading and Merging > loadSettings > should concatenate hook definitions from user and workspace scopes
 Tests  2 failed | 253 passed (255)

The generated settings.schema.json differs from main only by the five new keys under hooks: InstructionsLoaded, PermissionDenied, PostCompact, TodoCompleted, TodoCreated. With config.ts and settingsSchema.ts reverted to main, the nine new dispatch cases and the schema coverage check fail.

Tested on

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

Environment (optional)

N/A, unit tests only.

Risk & Scope

  • Main risk or tradeoff: the merged hooks setting now concatenates PostCompact, PermissionDenied, TodoCreated, TodoCompleted and InstructionsLoaded across scopes, where before the highest-precedence scope replaced the others; this matches the other 17 events. Hook registration reads user and workspace hooks per scope, so a setup where both scopes define hooks is unchanged: each scope already registered on its own, and identical hooks (same name and command or URL) are collapsed when the hook plan is built. The merged value is only a fallback for a scope that has no hooks object at all. Through that fallback, definitions of these five events from lower-precedence scopes that the winning scope used to replace now also register and run, for example a system-defaults TodoCreated hook alongside a system settings one. PermissionDenied, TodoCreated and TodoCompleted hooks can block. Existing direct callers of the nine bus events are unchanged.
  • Not validated / out of scope: routing the existing direct callers through the bus, and the hooks guide's event catalog, which is updated separately.
  • Breaking changes / migration notes: none for user and workspace settings that both define hooks. If a system or system-defaults settings file defines one of these five events and you relied on a higher-precedence scope replacing that definition while your user or workspace settings have no hooks object, remove the definition you do not want.

Linked Issues

Part of #11610

中文说明

这个 PR 做了什么

设置 schema 和 hook 执行消息总线现在覆盖所有 hook 事件。hooks 设置新增 PostCompactPermissionDeniedTodoCreatedTodoCompletedInstructionsLoaded 的条目,按生成的 JSON schema 做校验的编辑器不再把它们标为未知键,并能显示说明。新条目和其余 17 个事件一样使用 CONCAT 合并策略,所以合并后的 hooks 设置会保留各作用域对这五个事件的定义,不再由优先级最高的作用域替换其它作用域。总线分发新增 SessionStartSessionEndSessionDeletePreCompactPostCompactInstructionsLoadedStopFailureTodoCreatedTodoCompleted 的分支,这些事件以前会落到 "Unknown hook event"。后三个事件的 fire 方法返回聚合结果,总线直接回复其最终输出:对 TodoCreatedTodoCompleted 来说这正是直接调用方读取的内容;StopFailure 的聚合器会丢弃 hook 输出,所以回复总是空的。

新增的 schema 测试断言 hooks 设置声明的事件与 HookEventName 枚举完全一致,并且每个事件都跨作用域拼接;另有一个设置加载测试钉住拼接行为。以后新增事件时,两处不会只改其一,也不会悄悄退回替换。另有一个总线测试遍历同一个枚举,要求每个事件都能到达 hook 系统的某个方法,所以新事件也不会落到 unknown-event 默认分支。

为什么需要

运行时本来就接受设置里的全部 22 个事件,但 schema 只列了 17 个,合法配置在编辑器里看起来是错的,设置文档生成器也无从描述另外五个。设置合并按 schema 查每个键的合并策略,所以缺失的五个事件合并方式也和其余事件不同:优先级最高的作用域会替换其它作用域。总线只处理 14 个事件,任何通过总线而不是直接调用 hook 系统来分发 hook 的地方,都无法触发另外八个。目前没有调用方依赖新增的总线分支;这里补齐缺口,让两条路径保持可互换。

评审测试计划

如何验证

  • cd packages/cli && npx vitest run src/config/settingsSchema.test.ts:包含两项检查,schema 中的 hook 事件与枚举一致,且每个 hook 事件都使用 CONCAT
  • cd packages/cli && npx vitest run src/config/settings.test.ts -t "concatenate hook definitions":加载用户级和工作区设置,两者都定义了 PostCompactTodoCreated hook,检查合并结果按顺序保留了两个作用域的定义。
  • cd packages/core && npx vitest run src/config/config.test.ts -t "hook execution bridge":对桩掉的 hook 系统,通过总线分发每一个 hook 事件;对新增的九个分支,检查转发的参数和 abort signal、未配置 hook 时的回复、TodoCreatedTodoCompleted 合并后的最终输出,以及 StopFailure 的空回复。
  • npm run generate:settings-schema 能复现提交的 JSON schema。

证据(前后对比)

用户可见的变化是五个新 schema 键的设置合并方式;其余部分只影响编辑器校验,以及目前没有调用方使用的总线分发。证据是单元测试和重新生成的 schema。

  • settingsSchema.test.tssettings.test.ts 共 255 个用例全部通过;config.test.ts 中 "hook execution bridge" 相关用例 50 个通过(跳过 669 个,共 719 个);整个文件 719 个用例全部通过。
  • 每条新增的总线断言都用破坏被钉住的代码再恢复的方式验证过:加一个没有总线分支的枚举成员,覆盖测试失败;转发另一个 signal 字段,15 个用例失败;把空的 SessionEnd 结果归一化、Todo 分支改为回复第一个 hook 的输出、StopFailure 分支发布输出、SessionStart 示例用非 AgentType 的值,都会让各自的用例失败。
  • PostCompactTodoCreated 条目上的 CONCAT 去掉(即 main 的合并方式)后,两个新测试都失败(2 failed | 253 passed)。
  • 生成的 settings.schema.jsonmain 相比只在 hooks 下多了五个键:InstructionsLoadedPermissionDeniedPostCompactTodoCompletedTodoCreated
  • config.tssettingsSchema.ts 换回 main 版本后,新增的九个分发用例和 schema 覆盖检查都会失败。

测试平台

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

环境

不适用,仅单元测试。

风险与范围

  • 主要风险或权衡:合并后的 hooks 设置现在对 PostCompactPermissionDeniedTodoCreatedTodoCompletedInstructionsLoaded 跨作用域拼接,以前由优先级最高的作用域替换其它作用域;这与其余 17 个事件一致。注册 hook 时按作用域分别读取用户级和工作区设置,所以两个作用域都定义了 hook 的配置不受影响:各作用域本来就分别注册,完全相同的 hook(名称与命令或 URL 相同)在构建执行计划时会合并为一个。合并值只在某个作用域完全没有 hooks 对象时作为回退使用。经由这条回退,以前被胜出作用域替换掉的低优先级作用域对这五个事件的定义,现在也会注册并执行,例如 system-defaults 里的 TodoCreated hook 会与 system 设置里的一起执行。PermissionDeniedTodoCreatedTodoCompleted 的 hook 可以阻断操作。九个总线事件现有的直接调用方不受影响。
  • 未验证 / 不在范围内:把现有的直接调用改为经由总线,以及 hooks 指南中的事件目录(另行更新)。
  • 破坏性变更 / 迁移说明:用户级与工作区设置都定义了 hook 的配置无需改动。如果 system 或 system-defaults 设置文件定义了这五个事件之一,而你依赖更高优先级的作用域在用户级或工作区设置没有 hooks 对象时替换掉它,请删掉不想要的那条定义。

关联 Issue

Part of #11610

The settings schema listed 17 of the 22 hook events the runtime accepts,
so PostCompact, PermissionDenied, TodoCreated, TodoCompleted and
InstructionsLoaded looked like unknown keys in editors. The hook
execution bus handled 14 events and logged "Unknown hook event" for the
rest.

Add the five schema entries and regenerate the JSON schema, with a test
that the schema's hook events equal the HookEventName enum. Add bus cases
for SessionStart, SessionEnd, SessionDelete, PreCompact, PostCompact,
InstructionsLoaded, StopFailure, TodoCreated and TodoCompleted; the last
three return aggregated results, so the bus replies with their final
output like every other event.

Part of QwenLM#11610
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required headings present, test plan filled in, Chinese translation complete.

Problem: observed, and I confirmed it in the tree rather than taking the description's word for it. HookEventName has 22 members. settingsSchema.ts declares 17 of them under hooks.properties — missing exactly PostCompact, PermissionDenied, TodoCreated, TodoCompleted and InstructionsLoaded. The message-bus switch in config.ts carries 13 case labels plus a default that warns "Unknown hook event". The other half of the claim holds too: hookRegistry.isValidEventName() validates against Object.values(HookEventName), so the runtime already accepted all 22 from settings — the schema was the only thing out of step. One correction: the description says the bus handled 14 events; it handles 13. 13 + 9 = 22, so the coverage conclusion is still right.

Direction: aligned. This is precisely the "Reconcile the settings schema and MessageBus dispatch with the event enum" item from #11610, and it stays scoped to that item — the matcher, timeout and stdout items are left to their own PRs. CHANGELOG: no direct reference, but the hooks area is clearly active.

Size: core paths touched (packages/core/src/config/**, packages/cli/src/config/**). Production logic 157 lines (config.ts +97, settingsSchema.ts +60) · tests 152 (config.test.ts +145, settingsSchema.test.ts +7) · generated/schema 560 (settings.schema.json, excluded from the count). Below both the 500-line escalation threshold and the 1000-line advisory. Tier 2 still applies, because it is a core config change — see the risk note at the end.

Approach: the scope feels right, and I checked the parts that would fail silently. All nine new case branches match the real HookSystem signatures exactly — argument order, optionality, and which three return AggregatedHookResult and therefore need .finalOutput (StopFailure, TodoCreated, TodoCompleted) versus the six that already return DefaultHookOutput | undefined. All ten new type imports are genuinely exported from ../hooks/types.js and all ten are used; PermissionMode and InstructionLoadReason were already imported, so nothing is duplicated. The committed JSON is one contiguous +560/-0 hunk adding exactly the five expected keys, and CI regenerates that file and fails on drift, so it is independently checked rather than trusted.

For what it's worth, my own first instinct was to derive the schema keys from the enum so drift becomes impossible by construction — but per-event descriptions differ, so that needs a description map plus a completeness check anyway, and the test you added is the cheaper equivalent. A table-driven bus dispatch would be a much bigger rewrite of a ~150-line switch and would cut against the minimal-change rule, so enumerating is the right call here. Your approach matches what I'd have done.

Two things I'd push back on. Neither is a correctness bug, but both need a maintainer's call rather than mine, so I've written them up in the review comment: adding the schema entries also flips those five keys from REPLACE to CONCAT during the settings merge, which is a real runtime change that the Risk section currently denies; and the nine new bus cases have no producer today, so they land as unreachable code.

Risk: no elevated risk signals — none of the changed files match the revert-correlated path list.

Flagging these for discussion before this merges. ⏸️

中文说明

感谢贡献!

模板完整 ✓ —— 所有必需标题都在,测试计划已填写,中文翻译完整。

问题: 是已观测到的问题,我没有只采信 PR 描述,而是在代码树里核对过。HookEventName 有 22 个成员;settingsSchema.tshooks.properties 下只声明了 17 个,缺的正好是 PostCompactPermissionDeniedTodoCreatedTodoCompletedInstructionsLoadedconfig.ts 里的消息总线 switch 有 13 个 case 分支,其余落到 default 并打印 "Unknown hook event"。描述的另一半也成立:hookRegistry.isValidEventName() 是用 Object.values(HookEventName) 校验的,所以运行时本来就接受全部 22 个事件,只有 schema 没跟上。一处更正:描述说总线处理 14 个事件,实际是 13 个;13 + 9 = 22,所以"覆盖完整"的结论依然正确。

方向: 对齐。这正是 #11610 中"让设置 schema 与 MessageBus 分发和事件枚举保持一致"这一项,而且范围就停在这一项 —— matcher、timeout、stdout 等留给各自的 PR。CHANGELOG 没有直接对应条目,但 hooks 这块明显是活跃区域。

规模: 触及核心路径(packages/core/src/config/**packages/cli/src/config/**)。生产代码 157 行(config.ts +97、settingsSchema.ts +60)· 测试 152 行(config.test.ts +145、settingsSchema.test.ts +7)· 生成/schema 560 行(settings.schema.json,不计入规模)。低于 500 行的升级阈值,也低于 1000 行的大 PR 建议线。但因为改的是核心 config,Tier 2 仍然适用 —— 见文末风险说明。

方案: 范围合理,我重点核对了那些出错也不会报错的地方。九个新增 case 分支与真实的 HookSystem 签名完全一致 —— 参数顺序、可选性,以及哪三个返回 AggregatedHookResult 因而需要 .finalOutputStopFailureTodoCreatedTodoCompleted),哪六个本来就返回 DefaultHookOutput | undefined。十个新增类型导入确实都由 ../hooks/types.js 导出且全部被用到;PermissionModeInstructionLoadReason 原本就已导入,没有重复。提交的 JSON 是一整块连续的 +560/-0 改动,只新增了预期的五个键,而 CI 会重新生成该文件并在漂移时失败,所以这个文件是被独立校验的,不是靠采信。

顺带说一下我自己的第一反应:本来倾向于直接从枚举派生 schema 的键,让漂移在结构上不可能发生 —— 但每个事件的描述不同,那样还是得配一张描述表加一个完整性检查,你新增的测试是更省的等价做法。至于总线,改成表驱动分发要重写那个约 150 行的 switch,与最小改动原则相悖,所以这里逐个列举是对的。你的方案和我会做的一致。

有两点我想提出异议。都不是正确性 bug,但需要维护者来判断而不是我来定,所以写在评审评论里了:新增 schema 条目同时会让这五个键在设置合并时从 REPLACE 变成 CONCAT,这是真实的运行时变化,而 Risk 部分目前否认了它;另外九个新增总线分支目前没有任何生产者,落地即是不可达代码。

风险: 无升级风险信号 —— 改动文件都不在"与回滚相关"的路径清单里。

先提出来讨论,再决定是否合入。 ⏸️

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

Reviewed at 4dce98ae1de9fb4d308673e5025ddfd958cd26e2 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

Code review

I read the diff against the real signatures on main rather than against the description. The mapping work is correct — all nine new bus cases line up with HookSystem exactly, including which three return AggregatedHookResult. Four things came up, ordered by how much they matter.

1. The schema entries change settings-merge behaviour, and the Risk section says they don't. This is the one I'd want a maintainer to look at.

getMergeStrategyForPath() (packages/cli/src/config/settings.ts:80) resolves a merge strategy by walking the schema tree segment by segment, and returns undefined as soon as a segment is missing. customDeepMerge() then uses it at settings.ts:616 to fold system defaults → user → workspace → system. In mergeRecursively (packages/cli/src/utils/deepMerge.ts:46-79), an array value only concatenates when the strategy is CONCAT; with undefined it falls through to the final else and does target[key] = srcValue — a straight replace.

So for path ['hooks','PostCompact']:

  • before — the segment is absent from the schema, the walk returns undefined, and the highest-precedence scope silently replaces the others;
  • after — the walk finds your new entry, returns MergeStrategy.CONCAT, and hooks from every scope run.

That applies to all five new keys. It is almost certainly the intended semantics — the other 17 events all declare CONCAT, and the old behaviour was itself an artefact of the missing entry. But it is a runtime change, not a documentation change: someone who defines TodoCreated or PermissionDenied hooks in both user and project settings goes from running one scope's hooks to running both, and per your own schema descriptions those two can block the operation. A hook duplicated across scopes now fires twice.

"Main risk or tradeoff: none at runtime for existing callers" and "Breaking changes / migration notes: none" both read as accurate for the bus half of the PR and inaccurate for the schema half. Please name this in Risk & Scope, and ideally pin it with a merge test — nothing in the current suite touches customDeepMerge, so a future schema edit could flip these five back to REPLACE with everything still green.

2. The new test is not prettier-formatted, so the lint gate should fail. In the dispatch helper in packages/core/src/config/config.test.ts, this line is 118 characters against a printWidth of 80:

>({ type: MessageBusType.HOOK_EXECUTION_REQUEST, eventName, input }, MessageBusType.HOOK_EXECUTION_RESPONSE);

Prettier can and will break it — the sibling call in the existing Stop-bridge suite a few lines below is already split across lines. scripts/lint.js:309 runs prettier --experimental-cli --check ., which is the Lint & Static job, still in flight at the time of writing. npm run format fixes it.

For clarity, the four long description strings you added in settingsSchema.ts are not a problem — prettier does not split string literals, and main already carries longer ones.

3. Nine unreachable branches — a question for the maintainer, not an objection. I traced every producer of HOOK_EXECUTION_REQUEST: toolHookTriggers.ts (six), client.ts (two), message-display-dispatcher.ts (one) and acp-integration/session/Session.ts (two). Between them they send nine distinct event names — PreToolUse, PostToolUse, PostToolUseFailure, PostToolBatch, Notification, PermissionRequest, UserPromptSubmit, Stop, MessageDisplay. None of the nine you added is among them, which matches what your description says.

Worth knowing: main already carries four caller-less cases (UserPromptExpansion, PermissionDenied, SubagentStart, SubagentStop), so this is not a new pattern — but it does take caller-less cases from 4 to 13 of 22. eventName is typed string, so an out-of-tree or ACP producer could reach them, and #11610 lists this reconciliation as an accepted item. I think carrying them is defensible; AGENTS.md's Simplicity First is the reason it is not obviously so, and that is a maintainer's call rather than mine.

4. Minor: the bus now has two conventions for aggregated results. Stop and MessageDisplay re-wrap via createHookOutput('Stop', …); your three new aggregated cases pass finalOutput through raw. Raw is actually the better match here — todoWrite.ts:438 reads result.finalOutput?.decision directly, so the bus now agrees with the direct path — but the comment "the bus carries its final output like every other event" isn't literally true of the other aggregated events. A word in that comment would save the next reader the same detour I took.

Everything else checked out and I want to say so plainly, because these are the failure modes a diff like this invites: the ten new type imports all exist in ../hooks/types.js and all ten are used (noUnusedLocals would have caught a stray); PermissionMode and InstructionLoadReason were already imported so nothing is duplicated; the argument order in all nine cases matches the real signatures including the options object for InstructionsLoaded; and the generated JSON is one contiguous +560/-0 hunk containing exactly the five expected keys at 112 lines each, with nothing else touched.

I also checked the "no user-visible behaviour" claim instead of accepting it, since schema entries can leak into the TUI. They don't: /config listing, completion and fuzzy-match all filter on SETTABLE_TYPES = {boolean, string, number, enum} and your entries are type: 'array'; the settings dialog filters on showInDialog === true and yours are false; and isRestartRequiredKey already resolved hooks.PostCompact to false via its hooks prefix, so that is unchanged. Schema defaults are never materialised into merged settings either — hookRegistry reads the raw settings.hooks — so default: [] is inert. That claim holds.

CI test evidence

Unattended CI run — per the triage rules I did not build or execute any PR code. This is the PR's own CI on the reviewed commit, fetched through the API. No check has failed; the two that matter most for this diff (Test, which runs your new suites, and Lint & Static, which runs the prettier gate in finding 2) are both still running, so this is a partial picture rather than a green one.

Final CI results for 4dce98a (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,失败项排在最前。

Two notes on what this signal can and cannot tell us. The settings.schema.json drift gate in ci.yml regenerates the file and fails on any difference — it lives in the still-running Lint & Static job, so the generated 560 lines are not yet independently confirmed, only structurally plausible. And the unit suites you cite in the description are the author's own claim; I have not re-run them and cannot on this path.

Not verified: the merge-behaviour change in finding 1. Nothing in the PR's tests exercises customDeepMerge, so the suite passes identically whether or not those five keys concatenate — a green run here says nothing about it.

Sandboxed verification would settle exactly that: @qwen-code /verify — whether a hooks.PostCompact (or TodoCreated) definition present in both user and workspace scope now concatenates into two hooks instead of the workspace scope replacing the user scope. That is the PR's own "none at runtime" claim, it is not observable from the diff, and the unit suite cannot distinguish the two outcomes. You have write access, so /tmux is also available, but I don't think it earns its keep here — I checked the TUI surfaces above and none of them render these keys.

Real-scenario testing: N/A on this path. This is an unattended CI run, so no tmux capture was driven; the live-behaviour lane is the /verify trigger named above.

中文说明

代码评审

我是拿 diff 对着 main 上的真实签名核对的,不是对着 PR 描述。映射部分是对的 —— 九个新增总线分支与 HookSystem 完全吻合,包括哪三个返回 AggregatedHookResult。有四点,按重要程度排列。

1. schema 条目改变了设置合并行为,而 Risk 部分说没有。 这是我最希望维护者过目的一点。

getMergeStrategyForPath()packages/cli/src/config/settings.ts:80)按路径逐段走 schema 树来解析合并策略,一旦某段缺失就返回 undefinedcustomDeepMerge()settings.ts:616 用它把 system defaults → user → workspace → system 依次折叠。在 mergeRecursivelypackages/cli/src/utils/deepMerge.ts:46-79)里,数组只有在策略为 CONCAT 时才拼接;策略是 undefined 时会落到最后的 else,执行 target[key] = srcValue,即直接替换。

所以对路径 ['hooks','PostCompact'] 而言:改动前,schema 里没有这一段,解析返回 undefined,优先级最高的作用域会静默替换掉其他作用域;改动后,解析命中你新增的条目,返回 MergeStrategy.CONCAT,各作用域的 hook 都会执行。五个新增键都是如此。

这几乎肯定就是预期语义 —— 另外 17 个事件都声明了 CONCAT,原先的替换行为本身正是条目缺失造成的。但它确实是运行时变化,不是文档变化:同时在用户级和项目级配置了 TodoCreatedPermissionDenied hook 的人,会从只执行一个作用域变成两个都执行,而按你自己写的 schema 描述,这两个事件是可以阻断操作的。跨作用域重复配置的同一个 hook 现在会触发两次。

"主要风险或权衡:对现有调用方没有运行时影响"和"破坏性变更 / 迁移说明:无"这两句,对总线那一半是准确的,对 schema 那一半不准确。请在 Risk & Scope 中写明,最好再补一个合并测试把它钉住 —— 现有测试完全没有触及 customDeepMerge,所以将来一次 schema 改动把这五个键退回 REPLACE,测试也依然全绿。

2. 新增测试没有经过 prettier 格式化,lint 门槛应该会失败。 packages/core/src/config/config.test.tsdispatch 辅助函数里,下面这行有 118 个字符,而 printWidth 是 80:

>({ type: MessageBusType.HOOK_EXECUTION_REQUEST, eventName, input }, MessageBusType.HOOK_EXECUTION_RESPONSE);

prettier 可以也会把它拆开 —— 下方几行、既有 Stop 桥接测试里的同类调用本来就是跨行写的。scripts/lint.js:309 执行 prettier --experimental-cli --check .,也就是 Lint & Static 任务,撰写时仍在运行。npm run format 即可修复。

说明一下:你在 settingsSchema.ts 里新增的四条较长 description 字符串不是问题 —— prettier 不会拆分字符串字面量,main 上本来就有更长的。

3. 九个不可达分支 —— 这是给维护者的问题,不是反对意见。 我追踪了 HOOK_EXECUTION_REQUEST 的全部生产者:toolHookTriggers.ts(六处)、client.ts(两处)、message-display-dispatcher.ts(一处)、acp-integration/session/Session.ts(两处)。它们一共只发送九个不同的事件名 —— PreToolUsePostToolUsePostToolUseFailurePostToolBatchNotificationPermissionRequestUserPromptSubmitStopMessageDisplay,你新增的九个都不在其中,这与描述一致。

需要知道的是:main 上本来就有四个无调用方的分支(UserPromptExpansionPermissionDeniedSubagentStartSubagentStop),所以这不是新出现的模式 —— 但无调用方的分支确实从 22 个里的 4 个变成了 13 个。eventName 的类型是 string,因此树外或 ACP 的生产者有可能触达它们,而且 #11610 把这项对齐列为已接受的清单项。我认为保留它们说得通;只是 AGENTS.md 的 Simplicity First 让它并非显然说得通,这该由维护者判断,而不是我。

4. 小问题:总线现在对聚合结果有两套写法。 StopMessageDisplay 会用 createHookOutput('Stop', …) 重新包装,而你新增的三个聚合分支直接透传 finalOutput。透传在这里其实更合适 —— todoWrite.ts:438 就是直接读 result.finalOutput?.decision,所以总线现在与直接调用路径一致 —— 但注释里那句"总线与其它事件一样回复最终的 hook 输出",对其他聚合事件并不字面成立。在注释里补一句,能让下一位读者少走我走的这段弯路。

其余部分都核对通过,我想明确说出来,因为这些正是这类 diff 最容易出错的地方:十个新增类型导入都确实存在于 ../hooks/types.js 且全部被用到(多一个 noUnusedLocals 就会报错);PermissionModeInstructionLoadReason 原本已导入,没有重复;九个分支的参数顺序与真实签名一致,包括 InstructionsLoadedoptions 对象;生成的 JSON 是一整块连续的 +560/-0 改动,恰好是预期的五个键、每个 112 行,没有动到其他任何内容。

我也没有直接接受"没有用户可见行为"这个说法,而是去验证了,因为 schema 条目有可能渗到 TUI 里。结论是没有:/config 的列表、补全和模糊匹配都以 SETTABLE_TYPES = {boolean, string, number, enum} 过滤,而你的条目是 type: 'array';设置对话框以 showInDialog === true 过滤,而你的是 falseisRestartRequiredKey 本来就会通过 hooks 前缀把 hooks.PostCompact 解析为 false,所以没有变化。schema 的默认值也不会被实体化进合并后的设置 —— hookRegistry 读的是原始的 settings.hooks —— 因此 default: [] 是惰性的。这个说法成立。

CI 测试证据

无人值守 CI 运行 —— 按 triage 规则,我没有构建或执行任何 PR 代码。以上是被评审提交上 PR 自身的 CI 结果,通过 API 获取。没有任何检查失败;但对本次 diff 最关键的两项(Test,会跑你新增的测试;Lint & Static,会跑第 2 点里的 prettier 门槛)都仍在运行,所以这是局部图景,不是一片绿。

关于这个信号能与不能说明什么,有两点。ci.yml 里的 settings.schema.json 漂移门槛会重新生成该文件并在出现差异时失败 —— 它位于仍在运行的 Lint & Static 任务中,所以那 560 行生成内容目前只是结构上合理,尚未被独立确认。另外,描述里引用的单元测试结果是作者自述,我没有重跑,在这条路径上也无法重跑。

未验证:第 1 点里的合并行为变化。PR 的测试没有任何一处触及 customDeepMerge,所以无论这五个键是否拼接,测试结果都完全相同 —— 这里的绿灯说明不了它。

沙箱验证正好可以敲定这一点:@qwen-code /verify —— 验证同时在用户级和工作区级定义的 hooks.PostCompact(或 TodoCreated)现在是否会拼接成两个 hook,而不是由工作区级替换用户级。这正是 PR 自己那句"运行时没有影响"的断言,从 diff 看不出来,单元测试也无法区分两种结果。你有写权限,所以 /tmux 同样可用,但我认为这里不值得动用 —— 上面我已经核对过 TUI 各处,都不会渲染这些键。

真实场景测试:本路径下 N/A。这是无人值守 CI 运行,没有驱动 tmux 抓取;实时行为的通道是上面点名的 /verify

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

Reviewed at 4dce98ae1de9fb4d308673e5025ddfd958cd26e2 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — the code itself is accurate and I could not break it, but it carries an undisclosed runtime change to settings merging on a core config path, and that is a maintainer's call, not mine.

Stepping back: this is a competent, well-scoped PR from someone who clearly read the code before writing. Every one of the nine bus cases matches the real HookSystem signature, the aggregated-versus-wrapped distinction is drawn correctly, the generated schema is exactly what the generator would emit, and the "no user-visible behaviour" claim survives the checking I did on it — I went looking for a TUI leak and there isn't one. Against my own independent proposal it lands in the same place: I would have preferred deriving the schema keys from the enum, but with per-event descriptions that collapses to the completeness test they wrote anyway, and a table-driven bus dispatch would have been a much larger change for no practical gain.

So why not 4 or 5. Two reasons, and only the first is substantive.

The schema half of this PR is not descriptive-only. Adding those five entries makes getMergeStrategyForPath resolve them to CONCAT, where today it resolves to undefined and the merge silently replaces. Anyone with the same hook defined in two scopes starts running both instead of one — including TodoCreated and PermissionDenied, which can block. I believe that is the correct end state; the other 17 events already work this way and the old behaviour was an accident of the missing entry. But the PR asserts "none at runtime" and "Breaking changes: none", which means nobody has weighed that transition, and nothing in the test suite would notice if it regressed later. That is exactly the kind of quiet semantic change on a core config path where I should not be the one signing off.

Second, and lighter: nine new dispatch branches that no producer can reach. main already has four such branches, so this isn't a new sin, and #11610 lists the reconciliation as an accepted item — but it takes caller-less cases from 4 to 13 of 22, and AGENTS.md's Simplicity First is the reason that deserves a human's explicit yes rather than a bot's shrug.

Would I maintain this in six months without cursing the author? Yes. The code is clear, the test is a real completeness guard rather than a coverage filler, and the comment explaining the three aggregated events shows the author was thinking about the reader. The prettier slip in the new test will fail Lint & Static and is a one-command fix.

Deferring, not rejecting. Nothing here is wrong enough to request changes over, and the direction is right — I just can't certify a core-config behaviour change that the PR's own risk assessment denies exists.

⏸️ Deferring to @zjunothing, who is already the assigned owner on this PR. Two specific questions I can't settle from the diff, tests and description:

  1. Is the REPLACE → CONCAT transition for PostCompact, PermissionDenied, TodoCreated, TodoCompleted and InstructionsLoaded intended to land silently in a chore PR, or should it be called out in Risk & Scope (and possibly in hooks: align the hook contract with Claude Code (plain-text stdout, stop_hook_active, timeout unit, matchers, common input) #11610) as a behaviour change with a migration note? Either answer is fine; the current description says the opposite of what the code does.
  2. Do we want the nine caller-less bus cases carried now, on the argument that eventName is an open string and the two paths should stay interchangeable — or deferred until a producer exists?

No approval posted in this run. Separately, Test and Lint & Static were still in flight on the reviewed commit, so there was nothing green to approve against yet either.

中文说明

置信度:3/5 —— 代码本身是准确的,我没能找出破绽;但它在核心 config 路径上带来了一处未披露的运行时变化(设置合并语义),这该由维护者判断,而不是我。

退一步看整体:这是一个胜任、范围克制的 PR,作者显然在动手前读过代码。九个总线分支每一个都与真实的 HookSystem 签名吻合,聚合结果与包装结果的区分画得正确,生成的 schema 正是生成器会产出的内容,而"没有用户可见行为"这个断言也经得住我的核查 —— 我专门去找 TUI 泄漏点,没有找到。与我读 diff 之前独立设想的方案相比,落点一致:我本来更倾向从枚举派生 schema 的键,但在每个事件描述各不相同的前提下,那最终会退化成他所写的那个完整性测试;而表驱动的总线分发会是一次大得多的改动,实际收益为零。

那为什么不是 4 或 5。两个原因,只有第一个是实质性的。

这个 PR 的 schema 那一半并非纯描述性。新增那五个条目会让 getMergeStrategyForPath 把它们解析为 CONCAT,而今天解析结果是 undefined、合并时静默替换。同时在两个作用域配置了同一个 hook 的人,会从只执行一个变成两个都执行 —— 其中包括可以阻断操作的 TodoCreatedPermissionDenied。我相信这就是正确的最终状态:另外 17 个事件本来就是这样,原先的行为只是条目缺失造成的意外。但 PR 断言"运行时没有影响""破坏性变更:无",这意味着没有人权衡过这个过渡,而且将来它若回退,测试套件也不会察觉。这正是在核心 config 路径上一处安静的语义变化,不该由我来签字。

第二点,分量较轻:九个没有生产者能触达的新分发分支。main 上本来就有四个这样的分支,所以这不是新出现的问题,而且 #11610 把这项对齐列为已接受的清单项 —— 但它让无调用方的分支从 22 个里的 4 个变成 13 个,而 AGENTS.md 的 Simplicity First 正是"这需要一个人类明确说 yes、而不是机器人耸肩放过"的理由。

六个月后我来维护这份代码,会骂作者吗?不会。代码清晰,那个测试是真正的完整性守卫而不是凑覆盖率,解释三个聚合事件的注释也说明作者在替读者考虑。新测试里的 prettier 疏漏会让 Lint & Static 失败,一条命令就能修。

是推迟,不是否决。这里没有严重到需要 request changes 的问题,方向也是对的 —— 只是我无法为一处 PR 自身风险评估都否认其存在的核心 config 行为变化背书。

⏸️ 转交 @zjunothing,他已经是本 PR 的指定负责人。有两个我无法从 diff、测试和描述中敲定的具体问题:

  1. PostCompactPermissionDeniedTodoCreatedTodoCompletedInstructionsLoaded 从 REPLACE 到 CONCAT 的过渡,是打算在一个 chore PR 里静默落地,还是应当在 Risk & Scope(也许还有 hooks: align the hook contract with Claude Code (plain-text stdout, stop_hook_active, timeout unit, matchers, common input) #11610)中作为带迁移说明的行为变化写清楚?两种答复都可以接受;但目前的描述与代码实际所做的事相反。
  2. 这九个无调用方的总线分支,是现在就保留(理由是 eventName 是开放的 string,两条路径应保持可互换),还是等有生产者出现再补?

本次运行未提交 approve。另外,在被评审的提交上 TestLint & Static 仍在运行,所以即便要批,当时也没有一片绿可作为依据。

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

Reviewed at 4dce98ae1de9fb4d308673e5025ddfd958cd26e2 · re-run with @qwen-code /triage

The five hook events this PR adds to the settings schema also gain the
CONCAT merge strategy, so a definition in workspace settings no longer
replaces the user's for PostCompact, PermissionDenied, TodoCreated,
TodoCompleted and InstructionsLoaded. Pin that with a loadSettings merge
test and a schema check that every hook event concatenates, and reword the
bus comment about the three aggregated events: they reply with the final
output as is, unlike Stop and MessageDisplay.
@qqqys

qqqys commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

评审意见处理,已推 0363b80(追加提交):

  1. schema 条目改变了设置合并行为:已修,这一条说得对。CLI 主路径的 userHooks 在没有分作用域配置时回退到合并后的 settings.hooks,而合并按 schema 查每个键的策略,所以这五个事件确实从"高优先级作用域替换"变成了"各作用域拼接"。这正是想要的最终语义(和其余 17 个事件一致),但属于运行时变化:

    • PR 正文的 What / Why / Evidence / Risk & Scope 和中文说明都已改写,写明了"同一 hook 写在两个作用域会触发两次、其中三个事件可阻断",并给出迁移说明。
    • 测试钉了两层:settings.test.ts 新增 "should concatenate hook definitions from user and workspace scopes",用 loadSettings 加载用户级和工作区都定义了 PostCompactTodoCreated 的配置,断言合并结果按顺序保留两边的定义;settingsSchema.test.ts 新增断言,要求每个 HookEventName 的 schema 条目都是 CONCAT
    • 变异验证:去掉 PostCompactTodoCreated 上的 CONCAT(即 main 的合并方式)后,两条新测试都失败(2 failed | 253 passed),恢复后 255/255 通过。
  2. 测试代码未经 prettier 格式化:不成立。那一行在类型参数 >( 之后,prettier 3 的输出本来就是这个形状:用仓库配置对该文件跑 prettier 输出与原文件逐字一致;Lint & Static4dce98a 上也已通过。

  3. 九个没有调用方的总线分支:保留。hooks: align the hook contract with Claude Code (plain-text stdout, stop_hook_active, timeout unit, matchers, common input) #11610 把"总线覆盖全部事件"列为已接受的清单项,理由是 eventName 是开放的 string,ACP、SDK 和进程外的生产者可以按事件名经总线触发,两条路径应当可以互换。以前事件落到 "Unknown hook event" 时只打一条 debug 日志,调用方收到 success: true、输出为 undefined,会静默丢失 hook。每个分支都是对 HookSystem 现有方法的直接转发,新增的桥接用例逐个核对了参数和返回值,维护成本低。

  4. "like every other event" 的注释不准确:已改。注释现在写明这三个事件直接回复聚合结果的 finalOutput,这正是直接调用方读取的内容(todoWritefinalOutput.decision),并注明 StopMessageDisplay 会用 createHookOutput 包装。PR 正文同一句也同步改了。

另外回应 stage 3 的两个问题(我是本仓维护者):

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agent-assisted review at dcf82223243c6291c78bd4d606123884aa26248e — no confirmed new Critical findings in the full six-file static scope.

Reviewed against base 35cc3f5f86a97250c81196722ce8780ff396cd9d: schema definitions, generated JSON, merge behavior, nine bus dispatch cases, their HookSystem/HookEventHandler signatures and result shapes, and added test assertions.

The five missing schema entries use the shared hook definition and CONCAT strategy. The settings merge still drops untrusted workspace settings (settings.ts:595-622); adding schema entries does not bypass that boundary. The bus forwards event-specific inputs and cancellation to the Config-owned HookSystem (config.ts:3585-3672), rather than choosing a workspace/runtime from payload data. Base session/cwd input continues to come from that HookSystem. StopFailure/Todo aggregate results are unwrapped to finalOutput; this agrees with the existing todo validation reader (todoWrite.ts:437-477), including its block decision. Existing lifecycle, compaction, instructions and todo producers still use direct methods; these new switch cases add bus parity, not a conversion of every producer to the bus. Missing-system, pre-abort and exception response paths remain available. No daemon route or new ownership selector is added.

History: no previous reviews or threads existed when collected. No new Suggestions. Core gate: 99 changed non-schema production lines; this is not a large external refactor. Write access is verified for the author, but maintainer status is not, and no exemption is assumed.

Validation: static inspection only, plus an independent Python JSON check that all 22 generated event definitions have identical hook-item schemas. The new tests cover event-key completeness, CONCAT parity and dispatch/result forwarding, but were not executed. No live hook process, integration suite or security certification is claimed; payload casts are not runtime input validation.

Comment only — no approval implied.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partially reviewed — gaps disclosed. Suggestions are inline.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 1)": tsc --noEmit for packages/core (only packages/cli was typechecked; the ten new core imports were verified used by grep instead).; "agent reverse-audit (round 2)": did not open the module that defines WORKSPACE_RESTRICTED_SETTINGS or the body of stripWorkspaceLoosenings ( settings.ts:575 ); I confirmed via grep that ne…; "agent reverse-audit (round 2)": did not execute npx vitest run on packages/cli/src/config/settings.test.ts , settingsSchema.test.ts , or packages/core/src/config/config.test.ts at the re….

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

Test Plan (not a blocker): Tests 255 passed — this review observed 30770, 25313, 535, 2025, 1016, 1997, 7341 passed; Tests 21 passed — this review observed 30770, 25313, 535, 2025, 1016, 1997, 7341 passed; 253 passed — this review observed 30770, 25313, 535, 2025, 1016, 1997, 7341 passed.

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 1)"tsc --noEmit for packages/core (only packages/cli was typechecked; the ten new core imports were verified used by grep instead)."agent reverse-audit (round 2)"did not open the module that defines WORKSPACE_RESTRICTED_SETTINGS or the body of stripWorkspaceLoosenings ( settings.ts:575 ); I confirmed via grep that ne…"agent reverse-audit (round 2)"did not execute npx vitest run on packages/cli/src/config/settings.test.ts , settingsSchema.test.ts , or packages/core/src/config/config.test.ts at the re…

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

Test Plan(非阻断):Tests 255 passed — this review observed 30770, 25313, 535, 2025, 1016, 1997, 7341 passed; Tests 21 passed — this review observed 30770, 25313, 535, 2025, 1016, 1997, 7341 passed; 253 passed — this review observed 30770, 25313, 535, 2025, 1016, 1997, 7341 passed

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

Comment thread packages/cli/src/config/settingsSchema.ts
Comment thread packages/core/src/config/config.test.ts
Comment thread packages/core/src/config/config.test.ts Outdated
Comment thread packages/core/src/config/config.test.ts Outdated
Comment thread packages/core/src/config/config.test.ts
Comment thread packages/core/src/config/config.test.ts Outdated
Comment thread packages/core/src/config/config.ts
@yiliang114

Copy link
Copy Markdown
Collaborator

Reviewed at dcf8222324 (+931/−0, 6 files).

Verdict

Approve — no blocking findings. Purely additive, and it matches the issue item: the settings schema and the message-bus dispatch now cover every hook event.

Verified at this commit:

  • The dispatch switch has an arm for all 22 HookEventName values (counted at config.ts:3437:3662). The new arms pass the positional arguments the real hookSystem.ts methods take, unwrap .finalOutput where the handler returns an aggregated result, assign directly where it returns DefaultHookOutput | undefined, and InstructionLoadReason is already imported.
  • The schema-vs-enum drift test (toEqual([...Object.values(HookEventName)].sort())) is the guard worth having: it fails when a hook event is added without a schema entry, instead of pinning today's list.
  • Merge semantics are covered end to end — the CONCAT strategy resolves through settings.tsgetMergeStrategyForPathdeepMerge.ts.
  • The regenerated schema copies carry the five new entries and nothing else.

Optional gaps, neither blocking

  1. The surviving default: arm is untested; one case with an unrecognized eventName would pin it.
  2. Only PostCompact and TodoCreated of the five new events are exercised end to end, so a copy-paste slip in PermissionDenied, TodoCompleted or InstructionsLoaded would not be caught by a test. The drift guard still catches a missing schema entry, so the exposure is limited to wrong arguments or a wrong unwrap.

… bus

The schema already fails when a hook event lacks an entry; the bus switch
had no such guard, so a new event could fall through to the unknown-event
default with the suite green. Iterate HookEventName and require each event to
reach a hook system method.

The direct-call bridge tests now send a real abort signal and assert the
same signal is forwarded, cover the no-hook-configured path that replies
with no output, use a declared AgentType in the SessionStart example, and
give the Todo events two distinct outputs so a first-output projection is
caught. StopFailure gets its own case with the aggregate shape the
aggregator really returns, and the comment on its arm now says it is
fire-and-forget and always replies with no output.
@qqqys

qqqys commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

@yiliang114 感谢 approve,两条可选缺口的处理如下(58f72d2dea):

  1. default: 分支没有用例:本轮新增的 every hook event through the hook execution bridgeHookEventName 逐个经总线分发,断言不会落到 unknown-event 默认分支。已验证:给枚举追加一个成员而不加分支时该用例失败。枚举之外的事件名仍按原样回复 success,这是为 ACP/SDK 等进程外调用方保留的开放契约,所以没有再单独钉“未知事件的回复”。

  2. 五个新事件只有两个走了端到端:schema 一侧,settingsSchema.test.ts 逐个断言每个事件都有条目且策略为 CONCAT,五个事件里任何一个策略写错都会失败;设置合并由策略驱动,所以 loadSettings 合并用例只取两个事件做代表。总线一侧,TodoCompletedInstructionsLoaded 的转发参数与 signal 在桥接用例里逐个钉住;PermissionDenied 的总线分支本 PR 之前就存在。因此没有再加端到端行。

同一次提交还处理了机器人这一轮 7 条意见(已逐条回复并 resolve),其中 Risk & Scope 与迁移说明按注册实际读取的作用域重写了。验证:config.test.ts 整个文件 719 个用例全部通过;core tsc --noEmit 的报错与合并 main 后的基线一致,只剩 code-mode/host.ts 缺依赖的环境报错。

@qqqys
qqqys enabled auto-merge September 11, 2026 17:41

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE — no Critical. CI is green at 58f72d2d, all 7 review threads are resolved, and the round-1 ledger carries only a Suggestion. This is a pure-addition diff (+1074/-0) whose one real behaviour change is disclosed precisely.

What I checked

The five new schema entries are structurally identical to the seventeen existing ones. settingsSchema.ts:3903+ adds PostCompact, PermissionDenied, TodoCreated, TodoCompleted and InstructionsLoaded with the same shape as their siblings — type: 'array', category: 'Advanced', requiresRestart: false, default: [], showInDialog: false, items: HOOK_DEFINITION_ITEMS, and mergeStrategy: MergeStrategy.CONCAT. Uniformity is the point: because the settings merge looks up each key's strategy in the schema, a missing entry silently fell back to replace, so these five merged differently from every other event. Adding them removes that asymmetry rather than introducing a new one.

The behaviour change is real, narrow, and correctly scoped in the description. The risk section does not just say "now concatenates" — it establishes why the common case is untouched (hook registration already reads user and workspace hooks per scope, each scope registers on its own, identical hooks collapse when the plan is built, and the merged value is only a fallback for a scope with no hooks object at all), then names the exact configuration that is affected: a system-defaults definition of one of these events alongside a system-settings one, where the winning scope used to replace it. It also flags that PermissionDenied, TodoCreated and TodoCompleted hooks can block, which is the part that matters — a newly-registering hook on one of those three can now block an operation that previously was not blocked. That is the right thing to surface, and the migration note tells the affected reader which definition to remove.

The nine new bus cases cannot change current behaviour. They are added arms in a dispatch switch keyed on event name, so they only affect callers that pass those names, and the description states no current caller does — the direct callers of those events bypass the bus. The value is that the two paths become interchangeable instead of one silently answering "Unknown hook event". The three aggregated-result events are handled honestly too: TodoCreated/TodoCompleted reply with the merged final output because that is what their direct callers read, and StopFailure replies empty because its aggregator discards hook outputs — stating that the reply is always empty there, rather than implying it carries something.

The invariant tests are the strongest part of this change. Asserting that the schema declares exactly the HookEventName enum members and that every one of them concatenates, plus a bus test that iterates the same enum and requires each to reach a hook-system method, closes the failure mode that produced this gap in the first place: a new event added to the enum but not to the schema or the bus. Without those, the same silent drift would recur on the next event.

Non-blocking

The description notes the hooks guide's event catalog is updated separately. Worth confirming that follow-up lands, since the schema descriptions are now the only in-repo statement of what these five events do, and showInDialog: false means the settings UI will not surface them.

The round-1 Suggestion about the migration note naming a configuration the change cannot reach reads as addressed by the current wording, which distinguishes the per-scope registration path from the merged-value fallback explicitly. Flagging only so it is on record that I checked it rather than assumed it.

@qwen-code-dev-bot qwen-code-dev-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE

已核对 head 58f72d2dea96c8ed09ce2a3cbf70947e9147cef8(vs origin/main = merge-base 28df8b8a78)。

历史项:本 PR 的 7 条线程全是 Suggestion、无 Critical,且都在当前 head 关闭(settingsSchema.ts:3905config.ts:3652config.test.ts:13784/13881 四条为作者在本 head 补/改,另三条 outdated 但已按回复落地)。逐条对过当前 head,没有仍站得住的历史项。

独立复查未发现 Critical,具体核过:

  • 覆盖度是闭合的:HookEventName 共 22 个成员,config.ts 的响应器 case 现有 22 个(:3438:3674),default: 只剩真正的未知事件;settingsSchema 一侧由 settingsSchema.test.ts:49-53 用集合相等(Object.keys(...hooks.properties) vs Object.values(HookEventName))钉住,另有逐事件 mergeStrategy 的 CONCAT 钉法,二者都不是能空转的断言。
  • 新增 9 个分支的实参顺序与 hookSystem.ts 签名逐个对得上(fireSessionStartEvent(source, model, permissionMode?, agentType?, signal?)fireInstructionsLoadedEvent(filePath, memoryType, loadReason, options, signal?)fireTodoCompletedEvent(todoId, todoContent, previousStatus, allTodos, phase, signal?) 等),没有同类型参数互换的空间;duration_ms 那两处沿用 #11618 已有的形参位置。
  • 分支返回值形态与注释一致:TodoCreated/TodoCompletedfinalOutput(直连调用方 todoWrite 读的就是 finalOutput.decision),StopFailurefinalOutputhookAggregator.ts:49-61 确实把它硬编码成 undefined,所以「等待执行但不回输出」的说法成立。
  • 不会双触发:SessionStart/InstructionsLoaded/TodoCreated 的直连调用方(client.ts:2179/2186instructionsLoadedCallback.ts:30todoWrite.ts:426/525)直接走 hookSystem,不经总线;总线的发布方是 Session.tsclient.tsmessage-display-dispatcher.tstoolHookTriggers.ts,同一事件不会被两条路各发一次。
  • 生成的 packages/vscode-ide-companion/schemas/settings.schema.json 五个新键(PostCompact/PermissionDenied/TodoCreated/TodoCompleted/InstructionsLoaded)都已出现,CI 的 “Check settings schema is up-to-date” 步骤在 Lint & Static 作业内且已通过。

CI:required 全部完成且成功 —— Test (ubuntu-latest, Node 22.x)Lint & Static (ubuntu-latest, Node 22.x)Integration Tests (no-AK, No Sandbox)web-shell E2E Smoke (ubuntu-latest, Node 22.x)(另有 TUI parity snapshotsOpenTUI no-flicker gate 亦 success),无失败、无 pending(review-pr 属评审机器人自身档,非门禁项)。

一处环境说明packages/core/src/config/config.test.ts 在本机无法收集(本地 node_modules 缺 @modelcontextprotocol/client,与本 PR 无关),因此该文件的绿色以 CI 的 Test 档为准;上面结论均来自静态核对与 schema/聚合器代码本身。

@qqqys
qqqys added this pull request to the merge queue Sep 11, 2026
Merged via the queue into QwenLM:main with commit fc15666 Sep 11, 2026
265 of 272 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants