Skip to content

feat(core): add permission_mode, agent_id and prompt_id to every hook input - #11618

Merged
qqqys merged 3 commits into
QwenLM:mainfrom
qqqys:feat/hook-common-input-fields
Sep 11, 2026
Merged

qqqys merged 3 commits into
QwenLM:mainfrom
qqqys:feat/hook-common-input-fields

Conversation

@qqqys

@qqqys qqqys commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Every hook input now carries three more common fields. permission_mode is the session's approval mode, and events that already reported the mode that applied to them (tool events, subagent events, SessionStart) keep their own value. agent_id is added when the event fires inside a subagent, and omitted on the main agent. prompt_id is added when the event belongs to a model turn whose prompt id is known. PostToolUse and PostToolUseFailure also gain duration_ms, the time the tool spent executing, measured from the moment execution starts so that validation and approval time are excluded; it is omitted when a tool is cancelled before it starts. Both the core scheduler used by the terminal UI and headless runs and the ACP session report it.

The three copies of the approval-mode to permission-mode mapping, in the core client, the agent tool and background agent resume, are replaced by one shared helper that the hook event handler also uses.

Why it's needed

Only some events carried permission_mode, so a script shared across several events could not branch on it, and agent_id was only present on SubagentStart and SubagentStop even though the guide said subagent hooks receive it. The Claude Code hook contract puts these fields on every event's input, so scripts written against it read them unconditionally. duration_ms on the post-tool events lets audit and metrics hooks record tool latency without timing it themselves.

agent_type is not added to the common input. The agent context available at fire time only tracks the agent id, and threading the type through every place that starts an agent is a separate change; SubagentStart and SubagentStop keep reporting it.

Reviewer Test Plan

How to verify

Unit tests:

  • cd packages/core && npx vitest run src/hooks/permission-mode.test.ts src/hooks/hookEventHandler.test.ts covers the mapping, the session approval mode on an event without its own mode, an explicit event mode winning, agent_id and prompt_id present only inside an agent context and a prompt context, and duration_ms present only when given.
  • npx vitest run src/hooks/hookSystem.test.ts src/core/toolHookTriggers.test.ts src/core/coreToolScheduler.test.ts src/core/client.test.ts src/tools/agent/agent.test.ts src/agents/background-agent-resume.test.ts src/goals/goalLoop.integration.test.ts checks the callers of the moved mapping and the new trigger parameter.

End to end: a PostToolUse hook that copies its stdin to a file, then qwen -p "Run the shell command: sleep 1 && echo done" --approval-mode yolo. The captured input shows permission_mode: "yolo", a prompt_id, and a duration_ms a little over 1000.

Evidence (Before & After)

Both runs use the same isolated user settings with two capture hooks that copy their stdin to a file, one on UserPromptSubmit and one on PostToolUse for the shell tool. The fields below are picked out of the captured JSON; null means the key was absent. The baseline is the installed release, which builds hook input like main.

qwen -p "Reply with the word ok." --approval-mode auto-edit, captured UserPromptSubmit input:

installed qwen 0.22.3                      this branch
"permission_mode": null                    "permission_mode": "auto_edit"
"agent_id": null                           "agent_id": null
"prompt_id": null                          "prompt_id": "eb5ebcf8-4b56-4569-8115-1d27ea720b0d########0"

qwen -p "Run the shell command: sleep 1 && echo done" --approval-mode yolo, captured PostToolUse input:

installed qwen 0.22.3                      this branch
"permission_mode": "yolo"                  "permission_mode": "yolo"
"prompt_id": null                          "prompt_id": "72ee7ecb-8cba-4cc1-85dc-524bc0560e36########0"
"duration_ms": null                        "duration_ms": 1071

agent_id stays absent on the main agent, as intended. Unit tests on this branch: permission-mode.test.ts, hookEventHandler.test.ts, hookSystem.test.ts and toolHookTriggers.test.ts pass; coreToolScheduler.test.ts, client.test.ts, agent.test.ts, background-agent-resume.test.ts and goalLoop.integration.test.ts 1143 passed; the hook tests in Session.test.ts 82 passed. With the event handler change reverted, the four new common-input cases fail.

Tested on

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

Environment (optional)

Local build of this branch (npm run build && npm run bundle) with an isolated QWEN_HOME.

Risk & Scope

  • Main risk or tradeoff: hook input grows by up to four optional fields. The guide already asks consumers to ignore unknown fields, but a strict decoder that rejects unknown properties must allow them before upgrading.
  • Not validated / out of scope: agent_type on every event, duration_ms on events other than the two post-tool events, and the ACP path end to end (covered by the shared trigger and by unit tests only).
  • Breaking changes / migration notes: none for consumers that ignore unknown fields.

Linked Issues

Part of #11610

中文说明

这个 PR 做了什么

每个 hook 输入现在多带三个公共字段。permission_mode 是会话的审批模式;原本就上报自身适用模式的事件(工具事件、子代理事件、SessionStart)保留自己的值。事件在子代理内触发时会带上 agent_id,主代理上则不带。事件属于一个已知 prompt id 的模型轮次时会带上 prompt_idPostToolUsePostToolUseFailure 还新增 duration_ms,即工具执行耗时,从执行真正开始时计时,不包含校验和审批时间;工具在开始执行前被取消时不带该字段。终端 UI 与无头运行所用的 core 调度器,以及 ACP 会话都会上报它。

审批模式到权限模式的映射原本在 core 客户端、agent 工具和后台 agent 恢复里各有一份,现在替换为一个共享帮助函数,hook 事件处理器也使用它。

为什么需要

以前只有部分事件携带 permission_mode,跨事件共用的脚本无法据此分流;agent_id 也只出现在 SubagentStartSubagentStop 上,尽管指南说子代理里的 hook 会收到它。Claude Code 的 hook 约定把这些字段放在每个事件的输入里,按该约定编写的脚本会无条件读取它们。post-tool 事件上的 duration_ms 让审计和指标类 hook 无需自行计时即可记录工具延迟。

公共输入没有加 agent_type。触发时可用的 agent 上下文只记录了 agent id,把类型穿过所有启动 agent 的地方是另一项改动;SubagentStartSubagentStop 继续上报它。

评审测试计划

如何验证

单元测试:

  • cd packages/core && npx vitest run src/hooks/permission-mode.test.ts src/hooks/hookEventHandler.test.ts:覆盖映射本身;没有自身模式的事件上报会话审批模式;事件显式上报的模式优先;agent_idprompt_id 只在 agent 上下文和 prompt 上下文中出现;duration_ms 只在给定时出现。
  • npx vitest run src/hooks/hookSystem.test.ts src/core/toolHookTriggers.test.ts src/core/coreToolScheduler.test.ts src/core/client.test.ts src/tools/agent/agent.test.ts src/agents/background-agent-resume.test.ts src/goals/goalLoop.integration.test.ts:检查被迁移映射的调用方以及新增的触发参数。

端到端:配置一个把 stdin 复制到文件的 PostToolUse hook,然后运行 qwen -p "Run the shell command: sleep 1 && echo done" --approval-mode yolo。捕获的输入里有 permission_mode: "yolo"prompt_id,以及略大于 1000 的 duration_ms

证据(前后对比)

两次运行使用同一份隔离的用户设置,配置了两个把 stdin 复制到文件的捕获 hook,一个挂在 UserPromptSubmit,一个挂在 shell 工具的 PostToolUse。下面的字段取自捕获到的 JSON,null 表示该键不存在。基线是已安装的发行版,与 main 构造 hook 输入的方式相同。

UserPromptSubmit--approval-mode auto-edit):已安装的 0.22.3 上没有 permission_modeprompt_id;本分支上 permission_modeauto_edit,并带有当前轮的 prompt_idPostToolUsesleep 1 && echo done--approval-mode yolo):两者都有 permission_mode: "yolo";本分支额外带有 prompt_idduration_ms 为 1071。主代理上 agent_id 如预期不出现。完整对比见上文英文部分。

本分支单元测试:permission-mode.test.tshookEventHandler.test.tshookSystem.test.tstoolHookTriggers.test.ts 通过;coreToolScheduler.test.tsclient.test.tsagent.test.tsbackground-agent-resume.test.tsgoalLoop.integration.test.ts 共 1143 个通过;Session.test.ts 中的 hook 测试 82 个通过。撤回事件处理器的改动后,新增的四个公共输入用例失败。

测试平台

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

环境

本分支的本地构建(npm run build && npm run bundle),使用隔离的 QWEN_HOME

风险与范围

  • 主要风险或权衡:hook 输入最多增加四个可选字段。指南已经要求消费方忽略未知字段,但拒绝未知属性的严格解码器需要在升级前放行它们。
  • 未验证 / 不在范围内:所有事件上的 agent_type、两个 post-tool 事件以外事件上的 duration_ms,以及 ACP 路径的端到端验证(仅由共享触发函数和单元测试覆盖)。
  • 破坏性变更 / 迁移说明:对忽略未知字段的消费方没有。

关联 Issue

Part of #11610

… input

Only tool, subagent and session-start events carried permission_mode, and
agent_id only appeared on SubagentStart and SubagentStop. Scripts shared
across events could not branch on either.

The common hook input now includes permission_mode (the session approval
mode, unless the event reports its own), agent_id when the event fires
inside a subagent, and prompt_id when the model turn is known.
PostToolUse and PostToolUseFailure also report duration_ms, measured from
the start of tool execution in both the core scheduler and ACP sessions.

The three copies of the approval-mode to permission-mode mapping are
replaced by one shared helper.

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 ✓

Problem: observed, not theoretical. This is item 5 of the hook-contract audit in #11610, and the description carries a real before/after capture of hook stdin: installed 0.22.3 yields permission_mode: null on UserPromptSubmit and no duration_ms on PostToolUse, this branch yields auto_edit and 1071 for a sleep 1 tool. There is also a pre-existing doc/code mismatch behind it — the guide already promised agent_id on subagent hook events, and only SubagentStart/SubagentStop delivered it.

Direction: aligned, with upstream evidence. Claude Code's changelog is a direct match on two of the four fields: 2.1.119 added duration_ms to PostToolUse/PostToolUseFailure as "tool execution time, excluding permission prompts and PreToolUse hooks" — the same exclusion this PR implements — and 2.1.69 added agent_id "(for subagents)", which is exactly the conditional population here. Worth correcting one thing from the earlier triage of #11610: it recorded duration_ms as not part of the Claude Code contract, so this field is alignment, not an enhancement.

Two honest notes on direction, neither a block. The need-discussion label on #11610 was applied by this bot's own issue triage, not by a maintainer, so it is not a hold — but the hook input payload is a public contract that every user hook script reads, and this is one of six PRs split out of that issue the same day, so a maintainer's explicit yes on the field set is worth having before merge. And the earlier issue triage warned that putting all four fields on the base HookInput would hand 20 events a meaningless agent_id/agent_type; this PR answers that by adding three, populating agent_id only inside an agent context, and leaving agent_type out with a stated reason. That concern looks handled.

Size: core paths are touched (packages/core/src/** plus one ACP session file), so the breakdown — 161 production lines, 13 docs lines, 148 test lines, 0 generated/schema, of 322 total. Well under the 500-line threshold, so no size escalation. 56 of those 161 production lines are deletions: the three copies of the approval-mode mapping this PR collapses into one helper.

Approach: close to the minimal shape, and I independently arrived at the same one before reading the diff — createBaseInput() is the single place common input is assembled, so the three fields belong there once rather than at 22 call sites. The mapping dedup is not a drive-by refactor: the new call site needs that mapping, and a fourth copy would be worse than extracting one. durationMs as a trailing optional parameter through the existing trigger chain is the least invasive way to thread it. Scoping agent_type out instead of threading agent type through every agent start site is the right cut. Nothing here I would ask you to remove.

Risk: Stage 1e flags packages/cli/src/acp-integration/session/Session.tsacp-integration is on the revert-correlated path list, so this gets full review depth and CI evidence is required before approval. In fairness, the change there is 11 added lines (one timestamp plus three call-site arguments), which is about as small as that path gets.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题: 是已观测到的问题,不是理论性加固。这是 #11610 hook 契约审计中的第 5 项,PR 描述里带了真实的 before/after hook stdin 捕获:已安装的 0.22.3 在 UserPromptSubmit 上给出 permission_mode: null、在 PostToolUse 上没有 duration_ms;本分支给出 auto_edit,以及 sleep 1 工具对应的 1071。背后还有一个既有的文档与代码不一致——指南早就承诺子代理里的 hook 事件会带 agent_id,但只有 SubagentStart/SubagentStop 真的带了。

方向: 对齐,且有上游证据。Claude Code 的 changelog 在四个字段中有两个是直接对应的:2.1.119PostToolUse/PostToolUseFailure 加入了 duration_ms,描述是 "tool execution time, excluding permission prompts and PreToolUse hooks",与本 PR 实现的排除口径一致;2.1.69 加入了 agent_id("for subagents"),正是这里的条件性填充。有一点需要更正此前对 #11610 的 triage 记录:它把 duration_ms 记为不属于 Claude Code 契约,因此这个字段是对齐,而不是额外增强。

关于方向有两点如实说明,都不构成阻塞。#11610 上的 need-discussion 标签是本机器人自己的 issue triage 打的,不是 maintainer 打的,所以它不是一道"暂缓"闸门——但 hook 输入载荷是一份公共契约,每个用户 hook 脚本都会读它,而本 PR 是同日从该 issue 拆出的六个 PR 之一,因此合并前值得让 maintainer 对字段集合明确点头。另外,此前的 issue triage 曾警告把四个字段全塞进基础 HookInput 会让 20 个事件带上无意义的 agent_id/agent_type;本 PR 的回应是只加三个、仅在 agent 上下文内填充 agent_id、并给出明确理由不加 agent_type。这个顾虑看起来已经处理了。

规模: 触及了核心路径(packages/core/src/** 外加一个 ACP session 文件),因此给出拆分——生产代码 161 行、文档 13 行、测试 148 行、生成/schema 0 行,共 322 行。远低于 500 行阈值,不触发规模升级。这 161 行生产代码里有 56 行是删除:即本 PR 收敛为一个共享函数的三份审批模式映射副本。

方案: 接近最小实现,而且我在看 diff 之前独立想到的也是同一个方案——createBaseInput() 是公共输入唯一的组装点,所以三个字段应该在这里加一次,而不是在 22 个调用点各加一遍。映射函数的合并不是顺手重构:新的调用点本来就需要这个映射,再抄第四份比抽出一份更糟。把 durationMs 作为现有触发链末尾的可选参数串下去,是侵入性最小的做法。选择把 agent_type 排除在外、而不是把 agent 类型穿过所有启动 agent 的地方,是正确的取舍。这里没有我会要求你删掉的东西。

风险: Stage 1e 命中 packages/cli/src/acp-integration/session/Session.ts——acp-integration 在与 revert 相关的路径清单上,因此本次按完整深度审查,且批准前必须有 CI 证据。公平地说,该文件里的改动只有 11 行新增(一个时间戳加三处调用点参数),在这条路径上已经算很小的改动了。

进入代码审查 🔍

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

Reviewed at 1d236c30e021976ac8f0355e56a10d3f0842c9ba · 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

Independent proposal first. Before opening the diff I worked only from the title and the "Why it's needed" section, and the shape I landed on was: put the three fields in createBaseInput() because that is the single assembly point for common input; source permission_mode from Config.getApprovalMode() through one shared mapping helper rather than a fourth copy; read agent_id and prompt_id from the AsyncLocalStorage contexts that already exist; thread durationMs as a trailing optional parameter through the existing trigger chain; stamp the start timestamp at the invocation boundary so approval time is excluded. The PR does all five. I did not find a simpler path it missed, and I have nothing to ask you to cut.

No critical blockers. The things most likely to be wrong in a change shaped like this all check out:

  • Field precedence. The base input now carries permission_mode, so any event that reports its own mode must spread the base first. All eight do — SessionStart, PreToolUse, PostToolUse, PostToolUseFailure, PostToolBatch, PermissionRequest, SubagentStart, SubagentStop each set permission_mode on the line after the spread, and SubagentStart/SubagentStop likewise set agent_id after it. The event-specific value wins everywhere, so no event silently regressed to the session mode.
  • Mapping semantics preserved. The deleted background-agent-resume copy matched the bare strings 'yolo'/'auto-edit'/'auto'/'plan'/'default', which are exactly the ApprovalMode enum values the shared helper switches on, so all three migrated call sites keep their prior behavior. The signature widening to string | undefined is what lets one helper serve both the enum and the persisted string.
  • Import hygiene. PermissionMode genuinely stops being a value in client.ts once toPermissionMode goes, and genuinely remains one in agent.ts, so dropping it in the first and keeping it in the second is right; background-agent-resume.ts keeps only type positions, which is why the type-only import works.
  • duration_ms coverage is complete, not partial. All five production fire sites pass it — the two MessageBus cases in config.ts, both scheduler paths, and all three ACP Session.ts sites. Both invocation.execute branches in the scheduler (the promotable-shell branch and the normal one) stamp executionStartedAt, as does the single execute site in Session.ts. Nothing was left reporting a missing duration on one path and a real one on another.
  • Scope of the new timestamp. It is declared inside _executeToolCallBody, which spans the whole method containing every fire site, and the method runs once per scheduled call — so the value cannot leak between tool calls.
  • getApprovalMode() in the common path. HookSystem is only ever constructed as new HookSystem(this) from Config, and createBaseInput already calls getSessionId/getTranscriptPath/getWorkingDir unguarded, so this adds no new failure mode.
  • No new import cycle. This was my main structural worry, since hooks/ now reaches into agents/runtime/. promptIdContext is a leaf, and agent-context's value graph (team/identity, tools/agent/fork-subagentsubagents/types, tool-names, core/environmentContext) never routes back into hooks/. Clear.
  • Types and docs agree. The eight event interfaces that declare permission_mode required stay compatible with the new optional on HookInput, and the documented default | plan | auto_edit | auto | yolo matches the PermissionMode enum exactly.

Two suggestions, neither blocking:

  1. The rewritten doc line says agent_type "is reported on SubagentStart and SubagentStop", but SessionStart reports it too — fireSessionStartEvent sets agent_type: agentType and SessionStartInput declares agent_type?: AgentType. The sentence you replaced was inaccurate in a different direction, so this is not a regression you introduced so much as a chance to get it right: three events, not two. Related, and already stated honestly in your Risk section — Claude Code 2.1.69 added agent_type to hook events generally "for subagents and --agent", so deferring it leaves a real parity gap open under hooks: align the hook contract with Claude Code (plain-text stdout, stop_hook_active, timeout unit, matchers, common input) #11610 item 5. Using "Part of" rather than a closing keyword is the correct call for that reason.
  2. elapsedExecutionMs() uses Date.now(), so an NTP step during a long-running tool would put a negative or inflated duration_ms into an audit hook's payload. A monotonic clock would be sturdier. Small enough that I would not hold the PR for it.
sequenceDiagram
    participant P1 as Scheduler (core or ACP session)
    participant P2 as Tool invocation
    participant P3 as Agent and prompt contexts
    participant P4 as HookEventHandler
    participant P5 as Config
    participant P6 as User hook script
    P1->>P3: enter promptIdContext for this turn
    P1->>P1: stamp executionStartedAt at the invocation boundary
    P1->>P2: execute, after validation and approval
    P2-->>P1: result, or failure
    P1->>P4: fire PostToolUse with durationMs from the stamp
    P4->>P3: read current agent id and prompt id
    P4->>P5: getApprovalMode for the session mode
    P4->>P4: build base input, then spread event fields last
    P4->>P6: payload with permission_mode, agent_id, prompt_id, duration_ms
Loading
Files changed (16 of 16)
File Lines What changed
docs/users/features/hooks.md +9/-4 Adds the three common fields and both duration fields to the payload docs. The agent_type sentence needs the SessionStart correction above.
packages/cli/src/acp-integration/session/Session.ts +11/-0 Stamps execution start immediately before the single execute call and passes the elapsed value at all three hook sites. High-risk path per Stage 1e, but the change is small and mechanical.
packages/core/src/agents/background-agent-resume.ts +2/-17 Drops the local string-keyed mapping copy for the shared helper. Semantics identical.
packages/core/src/config/config.ts +6/-0 Forwards duration_ms through the two MessageBus post-tool cases, with a typeof guard so a non-numeric value stays absent.
packages/core/src/core/client.ts +4/-20 Deletes the private toPermissionMode method and calls the shared helper at both SessionStart sites.
packages/core/src/core/coreToolScheduler.ts +16/-0 Adds the start stamp plus elapsed helper and threads it into the success path and all four failure paths.
packages/core/src/core/toolHookTriggers.ts +4/-0 Trailing optional durationMs on both triggers, spread into the payload only when defined.
packages/core/src/goals/goalLoop.integration.test.ts +1/-0 Test Config stub gains getApprovalMode, which the common path now reads.
packages/core/src/hooks/hookEventHandler.test.ts +113/-0 The four new common-input cases, including that an explicit event mode beats the session mode and that agent_id and prompt_id are absent outside their contexts.
packages/core/src/hooks/hookEventHandler.ts +16/-0 The substance of the change: three fields plus two optional duration fields, assembled in createBaseInput with the event spread coming after.
packages/core/src/hooks/hookSystem.test.ts +8/-0 Existing calls updated for the new trailing parameter.
packages/core/src/hooks/hookSystem.ts +4/-0 Passes durationMs through to the event handler.
packages/core/src/hooks/permission-mode.test.ts +26/-0 New. Pins all five enum mappings plus undefined, empty and unknown falling back to default.
packages/core/src/hooks/permission-mode.ts +30/-0 New shared helper replacing three copies. kebab-case filename and license header both correct.
packages/core/src/hooks/types.ts +11/-0 Three optional fields on HookInput, one on each post-tool input, all documented.
packages/core/src/tools/agent/agent.ts +1/-19 Drops the third mapping copy for the shared helper. resolveSubagentApprovalMode keeps its explicit bubble handling, which the helper's default branch would otherwise swallow.

Testing

This was an unattended CI run, so I did not build, run, or execute anything from this PR — the static-review rule applies and the evidence below is the PR's own CI, read through the API, plus the source reads described above. No check is red at the time of writing.

The two checks that actually pin this change — Test (ubuntu-latest, Node 22.x) and Lint & Static (ubuntu-latest, Node 22.x) — were still running when I fetched this snapshot, so I cannot report a result for them and I have not guessed one. Integration Tests (no-AK, No Sandbox) and Real daemon E2E / Java 11 had landed green by then. Test (macos-latest, …) and Test (windows-latest, …) are skipped on this PR, so there is no cross-platform signal either way. The table below is wrapped in region markers and the finalize job rewrites it in place once CI settles.

Not verified, and worth saying plainly: the before/after payload capture in the description (installed 0.22.3 versus this branch, duration_ms: 1071 for a sleep 1 tool) is the author's own measurement on Linux, not something I re-ran. The unit tests do pin the field wiring, including that duration_ms is absent when not given.

Sandboxed verification would settle the part the suite cannot: @qwen-code /verify — that duration_ms really excludes validation and approval time rather than merely being present. The stamp sits after the approval gate in the source, but nothing in the diff or the tests proves the reported number is execution-only for a tool that spent time waiting on a prompt, and the ACP path is unit-covered only, as the Risk section concedes.

Final CI results for 1d236c3 (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
macos-latest / Java 21 ✅ success
OpenTUI no-flicker gate ✅ success
Real daemon E2E / Java 11 ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
TUI parity snapshots (ink vs opentui) ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
windows-latest / Java 21 ✅ success

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

中文说明

代码审查。 先说独立方案:在读 diff 之前,我只根据标题和"为什么需要"推出的做法是——三个字段放进 createBaseInput()(公共输入唯一的组装点);permission_modeConfig.getApprovalMode() 经一个共享映射函数取得,而不是再抄第四份;agent_idprompt_id 直接读已有的 AsyncLocalStorage 上下文;durationMs 作为现有触发链末尾的可选参数串下去;在调用边界打时间戳,从而排除审批耗时。这个 PR 五条全都做到了。我没有找到它漏掉的更简路径,也没有要请你删掉的东西。

没有阻塞性问题。 这类改动最容易出错的几处都核对过了:八个自带模式的事件都是先展开 base、后写自己的 permission_mode,因此事件自身的值始终优先,没有事件被静默降级成会话模式;被删除的 background-agent-resume 副本匹配的字面量与共享函数 switch 的 ApprovalMode 枚举值完全一致,三个迁移点语义未变;client.tsPermissionMode 确实不再作为值使用、agent.ts 里确实仍在用,所以一边删一边留是对的;duration_ms 的覆盖是完整的而非部分——五个生产触发点全部传入,调度器的两个 invocation.execute 分支和 Session.ts 唯一的那个都打了时间戳;新变量声明在 _executeToolCallBody 内,该方法每次调度只进入一次,不会在工具调用之间串值;HookSystem 只由真实 Config 构造,且 createBaseInput 本来就在无保护地调用其他 getter,所以没有引入新的失败模式。我最担心的结构问题是 hooks/ 反向依赖 agents/runtime/ 会不会形成循环引用——promptIdContext 是叶子模块,agent-context 的值依赖图(team/identitytools/agent/fork-subagentsubagents/typestool-namescore/environmentContext)不会绕回 hooks/,确认无环。类型与文档也一致:八个事件接口里必填的 permission_modeHookInput 上新的可选声明兼容,文档写的五个取值与 PermissionMode 枚举逐字对应。

两条建议,均不阻塞。 其一,改写后的文档说 agent_type "在 SubagentStartSubagentStop 上报",但 SessionStart 也上报——fireSessionStartEvent 会设置 agent_type: agentTypeSessionStartInput 也声明了该字段。你替换掉的那句话本来也不准确,所以这不算你引入的回归,而是顺手改对的机会:是三个事件,不是两个。相关的一点你在风险部分已经如实写了——Claude Code 2.1.69 是把 agent_type 加到 hook 事件上的("for subagents and --agent"),因此推迟它确实留下了 #11610 第 5 项的一个真实差异;正因为如此,用 "Part of" 而不是自动关闭关键字是正确的。其二,elapsedExecutionMs() 用的是 Date.now(),长任务执行期间若发生 NTP 校时,审计类 hook 会拿到负数或异常偏大的 duration_ms;单调时钟更稳。这一点很小,我不会为此卡住 PR。

测试。 本次是无人值守的 CI 运行,因此我没有构建、运行或执行本 PR 的任何代码——静态审查规则适用。下面的证据来自通过 API 读取的 PR 自身 CI,以及上文的源码核对。撰写时没有任何检查是红的。

真正能钉住这次改动的两项检查——Test (ubuntu-latest, Node 22.x)Lint & Static (ubuntu-latest, Node 22.x)——在我抓取快照时仍在运行,所以我无法给出结论,也没有去猜测。当时 Integration Tests (no-AK, No Sandbox)Real daemon E2E / Java 11 已经通过。Test (macos-latest, …)Test (windows-latest, …) 在本 PR 上是 skipped,因此跨平台信号两边都没有。下方表格用区域标记包裹,CI 结束后 finalize 任务会就地重写它。

需要如实说明的未验证项:描述里的 before/after 载荷捕获(已安装的 0.22.3 对比本分支,sleep 1 工具得到 duration_ms: 1071)是作者本人在 Linux 上的测量,不是我复跑的结果。单元测试确实钉住了字段接线,包括未传入时 duration_ms 不出现。

沙箱验证可以补上测试套件补不了的那部分:@qwen-code /verify——用来确认 duration_ms 真的排除了校验与审批时间,而不只是"字段存在"。源码里时间戳确实打在审批闸门之后,但 diff 和测试都无法证明一个曾在权限提示上等待过的工具,其上报数值只包含执行时间;ACP 路径按你风险部分的说明也只有单元测试覆盖。

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

Reviewed at 1d236c30e021976ac8f0355e56a10d3f0842c9ba · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — the implementation is clean and I would not change it, but this edits a public contract and lands as one of six sibling PRs, so the field-set decision needs a human yes rather than a bot approval.

Stepping back: my independent proposal and this PR converged on the same design before I read a line of the diff, which is usually a good sign — it means the change follows the shape of the code instead of fighting it. createBaseInput() is where common input is assembled, so that is where the fields go; the mapping dedup is not scope creep because the new call site needs the mapping and a fourth copy would have been the actual smell. 56 of the 161 production lines are deletions. If I inherited this in six months I would thank the author, not curse them: one mapping helper instead of three drifting copies is a net simplification, and duration_ms arriving as a trailing optional parameter means no existing caller had to change shape.

The problem is real, and I want to be precise about why I believe that rather than just accepting the PR's framing. The guide already promised agent_id on subagent hook events and only two of twenty-two delivered it — that is a live doc/code mismatch, not a hypothetical. permission_mode on eight events makes a hook script shared across events impossible to write. And upstream added both duration_ms (2.1.119, with the same "excluding permission prompts" carve-out) and subagent agent_id (2.1.69). This is closing a gap, not inventing one.

So why not approve? Three reasons, and only the first two are about the code.

The change writes to a payload that every user hook script parses. It is additive and optional, and the docs already declare hook input forward-extensible with a duty to ignore unknown fields — so the contract is designed to absorb exactly this. But "designed to absorb it" is not the same as "a bot should decide it", and I cannot name the external consumers, which is the specific test the core-change gate sets. The PR's own Risk section makes the same point more sharply than I would: a strict decoder that rejects unknown properties has to be updated before upgrading, and for a security-sensitive hook a decoder failure can flip fail-open to fail-closed. That deserves a maintainer's explicit sign-off on the field set.

Second, this is one of six PRs opened against #11610 the same day. Splitting a tracking issue into focused PRs is good practice and I evaluated this one on its own merits — but the six share one underlying decision about how far to move the hook contract toward Claude Code's, and one of them (#11619) is a fork refactor that cannot be auto-approved under any circumstances. Signing off on the field set once, across the set, is cheaper and more coherent than six independent approvals.

Third, and merely procedural: Test (ubuntu-latest, Node 22.x) and Lint & Static (ubuntu-latest, Node 22.x) were still running when I reviewed, so there was no settled CI to approve against anyway. I have deliberately not left an approve-on-green instruction behind, because the first two reasons mean a human should make this call rather than a green checkmark.

Two small things for the author, neither a reason to hold the PR: the doc sentence about agent_type misses SessionStart, and elapsedExecutionMs() reads the wall clock. Details in the review above.

⏸️ Deferring to @yiliang114 — you are already the assigned owner on this one. The code needs nothing from you; what needs a human call is whether these four fields become part of the documented hook input contract, and whether the six PRs off #11610 should be reviewed as a set. @qwen-code /verify would settle the only claim I could not: that duration_ms genuinely excludes approval time rather than merely being present.

中文说明

置信度:3/5 —— 实现是干净的,我不会去改它;但这次改动动的是一份公共契约,而且是六个同源 PR 中的一个,因此字段集合这个决定需要人来点头,而不是由机器人批准。

退一步看:在读 diff 之前,我独立想到的方案和这个 PR 收敛到了同一个设计,这通常是个好信号——说明改动是顺着代码本身的形状走的,而不是硬掰。createBaseInput() 是公共输入的组装点,字段就该加在那里;映射函数的合并也不是范围蔓延,因为新的调用点本来就需要这个映射,再抄第四份才是真正的问题。161 行生产代码里有 56 行是删除。如果半年后由我来维护,我会感谢作者而不是骂他:一个共享映射函数取代三份会逐渐漂移的副本是净简化;duration_ms 以末尾可选参数的形式加入,意味着没有任何既有调用方需要改变形状。

问题是真实存在的,我想说清楚我为什么相信这一点,而不是照单接受 PR 的说法。指南早就承诺子代理里的 hook 事件会带 agent_id,而二十二个事件里只有两个真的带了——这是当下就存在的文档与代码不一致,不是假设。permission_mode 只出现在八个事件上,导致跨事件共用的 hook 脚本根本没法写。而且上游已经加了这两个字段:duration_ms(2.1.119,同样带 "excluding permission prompts" 的排除口径)和子代理的 agent_id(2.1.69)。这是在补齐差异,不是在凭空造需求。

那为什么不批准?三个理由,其中只有前两个和代码有关。

这次改动写入的是每个用户 hook 脚本都要解析的载荷。它是新增且可选的,文档也已经声明 hook 输入是前向可扩展的、消费方有义务忽略未知字段——所以这份契约本来就是为了吸收这类改动而设计的。但"设计上能吸收"不等于"该由机器人来决定",而我无法点名外部消费方,这正是核心改动闸门设定的判据。PR 自己的风险部分把这一点讲得比我还尖锐:拒绝未知属性的严格解码器必须在升级前放行这些字段,而对安全敏感的 hook 来说,解码失败可能把 fail-open 翻转成 fail-closed。这值得 maintainer 对字段集合明确签字。

第二,这是同一天针对 #11610 开出的六个 PR 之一。把一个跟踪 issue 拆成聚焦的 PR 是好实践,我也是就这一个 PR 本身来评估的——但这六个共享同一个底层决定,即 hook 契约要向 Claude Code 靠拢到什么程度,而其中 #11619 是一个 fork 的 refactor,在任何情况下都不能自动批准。对字段集合签一次字、覆盖整组,比六次独立批准更省也更一致。

第三,纯属程序性:我审查时 Test (ubuntu-latest, Node 22.x)Lint & Static (ubuntu-latest, Node 22.x) 还在跑,所以本来也没有一个已确定的 CI 结果可供批准。我特意没有留下 approve-on-green 指令,因为前两个理由意味着这个决定该由人来做,而不是由一个绿色的勾来做。

给作者两个小点,都不构成卡住 PR 的理由:关于 agent_type 的文档句子漏了 SessionStartelapsedExecutionMs() 读的是墙上时钟。细节见上面的审查。

⏸️ 转交 @yiliang114 —— 你已经是这个 PR 的指派负责人。代码层面不需要你做什么;需要人来定的是这四个字段是否成为 hook 输入契约的正式一部分,以及 #11610 拆出的六个 PR 是否应当作为一组来审。@qwen-code /verify 可以补上我唯一没能确认的那点:duration_ms 是否真的排除了审批时间,而不只是"字段存在"。

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

Reviewed at 1d236c30e021976ac8f0355e56a10d3f0842c9ba · re-run with @qwen-code /triage

duration_ms on PostToolUse and PostToolUseFailure was the difference of two
Date.now() readings, so a system clock step while a long tool ran could
report a negative or inflated duration to audit hooks. Both the core
scheduler and the ACP session now read performance.now() and round the
elapsed time. The hooks guide also lists SessionStart among the events that
report agent_type.
@qqqys

qqqys commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

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

  1. agent_type 文档漏了 SessionStart:已修。fireSessionStartEvent 确实会设置 agent_type,指南那句改成了"在 SessionStartSubagentStartSubagentStop 上报"。

  2. elapsedExecutionMs() 用墙上时钟:已改。核心调度器和 ACP Session.ts 的起点与差值都改用 performance.now(),差值取整后作为 duration_ms

    • 新增调度器用例 "reports PostToolUse duration_ms from a clock that system time changes cannot move":工具执行期间把 Date.now 拨回 0,断言 PostToolUse 输入里的 duration_ms 仍是非负数。
    • 变异验证:把调度器改回 Date.now() 后,只有这条用例失败(1 failed | 408 passed),恢复后通过。
    • ACP 路径是同样的三行改动,没有单独的时钟用例;Session.test.ts 920/920 通过,core tsc --noEmit 通过。

关于 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 73087750f152d5e9b6c73817218d0c788d953da9 — Partial review — coverage gaps; no confirmed Critical found in the inspected paths.

Reviewed the full 17-file diff and traced the added fields beyond it: session approval mode through the shared converter and event-specific overrides; agent identity through the agent AsyncLocalStorage frame; prompt identity through interactive, non-interactive and ACP producers; duration from both execution boundaries through tool triggers, MessageBus dispatch, HookSystem and HookEventHandler. The MessageBus uses synchronous EventEmitter dispatch, so the common-field read does not inherently lose the publishing async context. The hook runners serialize the resulting input rather than projecting away these fields. Existing explicit subagent identity/mode overrides remain after the common-field spread. The duration clock is monotonic and starts after approval rather than at scheduling.

Suggestion: supplement the helper-level context tests with a real submission/tool-hook path, including an approval-delayed or resumed subagent. The new test manually installs both async-local frames; it does not establish which identity survives a production queue/resume boundary. This is a coverage recommendation, not a confirmed identity defect.

Limitations: I have not exhaustively established async-frame ownership across every detached/background completion, deferred approval and resume path, or measured all failure-path duration boundaries. Thus I cannot make the core gate's 100%-confidence claim. This is a small feature change (163 changed production-source lines by the test/generated-excluding count), not a size-blocked refactor; maintainer review is still needed for the remaining core/cross-package coverage. No changed daemon route was found: the ACP change is within the owning Session's tool execution, not workspace selection or primary-runtime fallback.

There were no earlier reviews/threads in the fetched history to reconcile. Read the added/modified regression tests, but ran no build, unit tests or E2E; no PR code was executed. Comment only; no approval implied.

@yiliang114

Copy link
Copy Markdown
Collaborator

Reviewed at 73087750f1 (17 files, +295/−60). Full read of the diff, plus the call sites in hookEventHandler.ts, coreToolScheduler.ts, Session.ts and the bus path at this commit.

Verdict

Approve — no blocking findings. Scope matches the issue item exactly: permission_mode / agent_id / prompt_id on the common input, duration_ms on PostToolUse and PostToolUseFailure, and agent_type left where it already was.

Verified:

  • All 22 events build their input by spreading createBaseInput (hookEventHandler.ts:185:800), so the three common fields exist at every dispatch site. The eight events that report their own mode (SessionStart, the four tool events, PermissionRequest, SubagentStart, SubagentStop) assign permission_mode after the spread and therefore win, which is what the comment above the field says.
  • Enum and docs agree: PermissionMode is default | plan | auto_edit | auto | yolo and the new doc block uses exactly those strings. The shared mapper switches on ApprovalMode (auto-editauto_edit) and falls back to Default for unknown, empty and undefined — identical to the three local copies it replaces, in client.ts, agent.ts and background-agent-resume.ts.
  • agent_id and prompt_id are populated on the real paths, not only in tests. The TUI wraps the submission in promptIdContext.run (use-llm-stream.ts:3761), headless does the same (nonInteractiveCli.ts:522), and ACP uses promptIdContext.enterWith (Session.ts:5281, :11002). The bus does not break the chain: MessageBus.request publishes synchronously inside the promise executor, and config.ts dispatches from that same subscriber.
  • duration_ms is measured from performance.now() taken immediately before the executing transition (both invocation paths in coreToolScheduler.ts, one site in Session.ts), so validation and approval time are excluded, and the value is omitted when execution never started — matching the failure event's documented wording. The clock choice is pinned by a test that steps Date.now backwards mid-execution and asserts a non-negative number.
  • Serialization omits the key when undefined, consistent with the neighbouring tool_call_id, and the tests assert absence (not.toHaveProperty) rather than a null.
  • All callers are updated: five trigger sites in coreToolScheduler.ts, three in Session.ts, the hookSystem.ts pass-throughs, and the bus dispatch reading typeof input['duration_ms'] === 'number'. Nothing on the SDK side duplicates the hook-input shape (sdk-typescript only has MCP duration_ms fields), so there is no consumer schema to keep in sync.

Notes, neither blocking

  1. elapsedExecutionMs() is evaluated where the hook fires rather than where execution settled, so the reported value also includes the result post-processing in between. If the intent is exact tool latency, capture the duration at settle time and pass that value; if the small overhead is acceptable, the current wording is fine.
  2. Hooks fired by the scheduler read the ambient promptIdContext, while execute() is scoped with promptIdContext.run(request.prompt_id, …). Those agree for today's callers, since a request's prompt_id comes from its turn. A call whose request.prompt_id differed from the enclosing turn would report the outer id, so a one-line comment stating the assumption would help the next reader.

@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 fully green at 73087750, there are no review threads, and @doudouOUC's pass found no Critical either. This is a well-scoped change: 163 production lines, and the riskiest part of it is a three-way consolidation, which is exactly where a behaviour change likes to hide — so that is what I checked hardest.

The consolidation is genuinely behaviour-preserving

The three removed copies did not all switch on the same kind of value, which is the thing worth confirming:

  • client.ts's toPermissionMode switched on ApprovalMode enum members.
  • tools/agent/agent.ts's approvalModeToPermissionMode switched on ApprovalMode enum members.
  • agents/background-agent-resume.ts's copy switched on bare string literals'yolo', 'auto-edit', 'auto', 'plan', 'default'.

The new shared helper switches on enum members, so it is only equivalent to the third copy if those literals are the enum's values. They are: config/approval-mode.ts:7-13 defines PLAN = 'plan', DEFAULT = 'default', AUTO_EDIT = 'auto-edit', AUTO = 'auto', YOLO = 'yolo'. So case ApprovalMode.AUTO_EDIT and case 'auto-edit' are the same case, and all three copies collapse to the same function without a behaviour change on any path. Folding ApprovalMode.DEFAULT into default is also equivalent, since both returned PermissionMode.Default.

Worth naming the trap this helper now owns, because it is the reason three copies drifting was a real risk: the two enums spell the same mode differently — ApprovalMode.AUTO_EDIT = 'auto-edit' (hyphen) maps to PermissionMode.AutoEdit = 'auto_edit' (underscore, hooks/types.ts:1033). The docs update gets this right, listing permission_mode as default | plan | auto_edit | auto | yolo. Having one place responsible for that translation is the actual value of the change.

Common-field merge order is correct

createBaseInput emits permission_mode from the session's approval mode plus conditional agent_id/prompt_id, and every event that reports its own value spreads the base first and sets its own afterPostToolUse at hookEventHandler.ts:433-435, SubagentStart at :656-660. So event-specific mode wins, as the body claims, and SubagentStart's explicit agent_id correctly overrides the ambient getCurrentAgentId() frame, which at that point is the parent, not the child being announced. That ordering is load-bearing and it is right.

duration_ms excludes the right interval and degrades correctly

executionStartedAt is stamped from performance.now() at the invocation boundary — inside promptIdContext.run, immediately before setStatusInternal(callId, 'executing') (coreToolScheduler.ts:5271 and :5315, both execution branches) — so validation and approval time are excluded as documented. Monotonic clock means a system clock step during a long tool cannot skew it. elapsedExecutionMs() returns undefined when execution never started, and every consumer uses ...(durationMs === undefined ? {} : { duration_ms: durationMs }), so a tool cancelled before it starts omits the key entirely rather than emitting 0 or null — a hook can distinguish "never ran" from "ran instantly". It is wired at all five scheduler call sites (:5539, :5631, :6086, :6374, :6417), and acp-integration/session/Session.ts mirrors the identical helper at :12105, stamps at :13797 and passes it at :14089, :14158 and :14396 — so the ACP path reports the same field on the same definition rather than a second approximation. The replay path in config.ts:3501 and :3516 type-guards with typeof input['duration_ms'] === 'number' instead of casting, which is the right treatment for a value arriving from serialized input.

Non-blocking

  1. @doudouOUC's coverage suggestion is the right one and I'd second it. hookEventHandler.test.ts installs both async-local frames by hand, which pins that the helper reads them but not which identity actually survives a production queue, a deferred approval, or a background-agent resume. Those are precisely the boundaries where an AsyncLocalStorage frame is most likely to be lost, and a lost frame fails silently — the field just goes absent, and since agent_id is documented as "only when the event fires inside a subagent", an absent field on a subagent event is indistinguishable from a main-agent event. One integration test through a real submission or tool-hook path with an approval-delayed subagent would close it. Not a confirmed defect, and not a reason to hold this.

  2. The helper's parameter widened from ApprovalMode to string | undefined. That is necessary for the persisted background-agent value, but it means a mistyped literal at a future call site silently maps to PermissionMode.Default instead of failing typecheck. ApprovalModeValue (`${ApprovalMode}`, approval-mode.ts:15) is the same string union, so typing the parameter ApprovalModeValue | undefined would keep the persisted path working and restore the compile-time check at the enum-typed call sites. Minor.

  3. The hyphen/underscore asymmetry deserves a line in the helper's docblock. It is now the single place that knows auto-edit becomes auto_edit, and nothing in the file says so. That is the fact a future editor is most likely to break.

The before/after evidence in the description — captured stdin from real qwen -p runs against the installed 0.22.3 release rather than against assertions, with agent_id correctly absent on the main agent — is the right way to demonstrate a hook-contract change.

@chiga0 chiga0 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.

No blocking findings.
Approval blockers: none.

Checked:

  • permission_mode override ordering across all 22 event types. All eight events that carry their own mode (SessionStart, PreToolUse, PostToolUse, PostToolUseFailure, PostToolBatch, PermissionRequest, SubagentStart, SubagentStop) spread createBaseInput() first and then assign permission_mode — the event-specific value wins in every case. Verified from the live file at head.
  • ApprovalMode enum values ('plan' | 'default' | 'auto-edit' | 'auto' | 'yolo') match the raw string literals used in the three removed local copies (client.ts, agent.ts, background-agent-resume.ts). No regression from the consolidation.
  • elapsedExecutionMs() returns undefined when executionStartedAt is unset (pre-execution failures) — duration_ms is correctly omitted in those cases via the spread guard.
  • Two executionStartedAt = performance.now() sites in coreToolScheduler.ts are two execution branches of the same invocation, not sequential calls. The single elapsedExecutionMs closure captures the shared let variable correctly.
  • Duration test (mock Date.now to 0 mid-execution, assert duration_ms ≥ 0): correctly discriminates between Date.now() and performance.now() because Date.now() would yield a large negative number at that point.

Not covered: macOS and Windows not tested (author-disclosed); ACP path unit-only (author-disclosed); no terminal-dependent behavior changed so rung 3 not run.

Cross-check vs existing reviews: yiliang114's review at this head reached the same verdict with full coverage of call sites and promptId chain through the bus. No diverging finding found. The minor note about elapsedExecutionMs() capturing post-settle processing time is noted and non-blocking.

Reviewed with AI assistance.

@qqqys
qqqys added this pull request to the merge queue Sep 11, 2026
Merged via the queue into QwenLM:main with commit 28df8b8 Sep 11, 2026
87 checks passed

@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 73087750f152d5e9b6c73817218d0c788d953da9(vs merge-base 35cc3f5f86)。无 review 线程,历史记录里没有待修的 Critical/必要 Suggestion;独立复查未发现新 Critical。

三处映射合并确认行为不变client.tstools/agent/agent.ts 删掉的两份按 ApprovalMode 枚举分支,background-agent-resume.ts 删掉的那份按裸字符串分支;新的 hooks/permission-mode.ts 按枚举分支,而 config/approval-mode.ts:7-13 的值正是 'plan' | 'default' | 'auto-edit' | 'auto' | 'yolo',因此 'auto-edit'ApprovalMode.AUTO_EDIT 是同一分支,三份等价(PermissionMode.AutoEdit 落地为 'auto_edit',与文档里写的取值一致)。git grep toPermissionMode 在该 head 已无残留引用。

公共字段不会盖掉事件自带的值hookEventHandler.ts 里 22 个事件字面量全部先展开 createBaseInput(...),自带 permission_mode 的 8 处(:326/:401/:435/:476/:534/:597/:658/:689,即 SessionStart/PreToolUse/PostToolUse/PostToolUseFailure/PostToolBatch/PermissionRequest/SubagentStart/SubagentStop)在其后赋值;SubagentStart/SubagentStop:659/:691 覆写自己的 agent_idagent_id/prompt_id 用条件展开,未知时不产生 undefined 键。duration_ms 一律追加在形参末尾(无位置错位风险),config.ts 的再触发处用 typeof === 'number' 收敛。

本地验证packages/corehookEventHandler.test.ts + hookSystem.test.ts + permission-mode.test.tsTest Files 3 passed (3) / Tests 251 passed (251)。变异复核(每个单独注入,/tmp 隔离副本):删掉 createBaseInput 里的 permission_modereports the session approval mode on events without their own mode 红;删掉 PreToolUse 自带的 permission_modekeeps the permission mode an event reports itselfshould include all parameters in the hook input 红;把 agent_idprompt_id 改成无条件写入 → adds agent_id and prompt_id only when they are known 红。新契约四个方向都有能变红的见证。

CI:required 全部完成且成功 —— Test (ubuntu-latest, Node 22.x)Lint & StaticIntegration Tests (no-AK, No Sandbox)web-shell E2E Smoke,另有 TUI parity snapshotsOpenTUI no-flicker gate 亦 success。

记录一条非阻塞建议(doudouOUC 提出,不要求本 PR 改):现有上下文用例是手工装上两个 AsyncLocalStorage 帧,建议后续补一条走真实提交/工具钩子路径(含审批延迟或被 resume 的子代理)的用例,以证明身份字段在真实链路上也能存活。

@qqqys

qqqys commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

@yiliang114 @doudouOUC 感谢两轮核对,三条都不改代码,理由如下:

  1. duration_ms 取值点在 hook 触发处而不是执行结束处(yiliang114 注 1):保持现状。执行结束到触发 PostToolUse 之间只有同步的结果整理(拼 llmContentreturnDisplay),不含审批、不含 I/O 等待,量级远小于工具本身耗时。指南写的是"工具执行耗时,不含审批",这个表述在包含这段整理时依然成立。hook 需要的是"这次工具大概跑了多久",不是精确到毫秒的内核计时,把计时点再往前挪一层要把值穿过调度器的结果对象,改动面比收益大。

  2. 调度器触发的 hook 读的是外层 promptIdContext(yiliang114 注 2):前提成立。调度器里每个请求的 prompt_id 都来自它所属的回合,execute()promptIdContext.run(request.prompt_id, …) 包的也是同一个值,两者今天不会分叉。若以后出现跨回合复用请求的调用方,prompt_id 与外层回合不一致,这时 hook 会拿到外层 id;这属于那个新调用方要处理的契约,届时再在调用点加注释或显式传参。

  3. 补一条真实提交路径的身份用例(doudouOUC):作为覆盖建议记下,不在本 PR 加。本 PR 新增字段都从同一个 createBaseInput 读取两个 AsyncLocalStorage(agent 与 prompt),生产侧的写入点(TUI use-llm-stream.tspromptIdContext.run、headless nonInteractiveCli.ts、ACP Session.tsenterWith)都是现有代码、本 PR 未改;审批延迟和后台子代理恢复路径上的上下文归属是这两个存储自身的问题,不是 hook 输入新引入的,适合在 agent 上下文那条线里单独覆盖。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.4.

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