Replies: 3 comments
|
continue:false 记录了但没真正 halt——hook 的"审批拒绝"语义失效,比没记录更危险(以为拦住了其实没拦)。 这类控制流问题插件开发时要注意:hook 返回值要真正作用于执行管线,不能只落日志。第 4 章插件扩展点有相关讨论:https://github.com/Electricitysheep/dsh-handbook/blob/main/docs/04-plugin-dev.md |
作者已给出完整根因 + 自带测试的补丁( 补一点定位价值:这属于 "hook 返回值落库了却没作用于执行管线" 的运行时控制流族,与 #1521(Stop hook 可强制无限续)是同一断面的两面——决策被记录但执行段不消费该决策。对插件作者是重要的语义契约:hook 的返回值必须由真正的执行入口消费,不能只写审计日志。该族无离线可判定的 manifest 指纹, 认可补丁方向( |
|
根因和补丁都很扎实( 一、这不只是控制流 bug,是审计记录不实你自己那句话我觉得应该提到摘要里,而且加重:
区别在哪:一个"hook 没生效"的 bug,后果是防护没起作用;而现在的行为是——会话日志里明明白白写着 也就是说日志肯定地记录了一件没有发生的事。这比缺失记录严重:
建议把标题或摘要从"does not halt agent runs"改成同时点出这一层,比如 " 二、同一个控制边界,在 Windows 上还有第二条独立的失效路径#2485 报的是 PowerShell 不把原生子进程的退出码当作自己的退出码,于是一个 那帖里 @sjh9714 还补了第三条: 把这三条并起来看:
一个用户配了 deny hook,今天有三条独立的路径能让他得到零强制,而三条都不出声。 我觉得这个对照比任何单条都更能说明该修的是什么——不是三个各自的 bug,是这个控制边界缺少"我的强制真的生效了吗"这个可验证性。 你的补丁( 三、给现在依赖 hook 做强制的人:一个能证伪的自查在修复合入之前,别假设你的 hook 生效了——上面三条里任何一条都会让它静默失效。#2485 那位用的办法值得固化: 准备一个无条件拦截的 hook(比如直接 值得在每次改 hook 配置之后、每次升级之后都跑一遍——因为这件事今天没有任何其它信号。(这和我们自己的一条规矩同源:装完必须回读装到的那份再和期望比对;任何"我以为它生效了"的环节,都值得有一个能证伪它的探针。) 边界与利益相关我们不修 DSH 自家组件——两个 hook bridge、 利益相关:我维护 pi2dsh(Pi 生态兼容层)。这条不推销,而且是原则性的:我们生态里有"第二个模型逐次审批工具调用"的插件,但在一个"用户明确表达的强制没有生效"的帖子上推它是错的形状——用户要的是自己写的规则被执行,不是再引入一个会做判断的东西。 |
Uh oh!
There was an error while loading. Please reload this page.
Summary
Both
@deepseek-ai/dsh-hooks-codexand@deepseek-ai/dsh-hooks-claude-codeparse and durably record a hook response containing{"continue": false}, but officialmasterdoes not act on the merged stop request. A PreToolUse hook can therefore request a hard halt, produce ahook/resultwith decisionstop, and still allow the tool and the rest of the turn to run.I reproduced the control-flow mismatch with failing regression tests and prepared a tested fix in a public fork. Since the upstream repository does not expose external pull-request creation, I am sharing the complete patch here according to the repository's contribution workflow.
Reproduction
Configure either bridge with a PreToolUse command hook that prints:
{"continue": false, "stopReason": "halt"}Then run an agent turn whose first model response calls the matched tool.
Before the fix, both bridge regressions failed on official
masterat the same assertion:The session log contained
hook/result.decision === "stop", yet the tool ran and the turn completed normally. The targeted failing commands were:Root cause
mergeHookOutputs()correctly folds the firstcontinue:falseintoMergedHookOutcome.stopand preservesstopReason. Each bridge then maps permission decisions and context but ignoresmerged.stop; the source carried an explicitTODO(hook-continue-false)saying that a run-level halt mechanism was still needed.The agent loop now exposes that mechanism through
Agent.cancel({ kind: 'hook', reason }), including a durable aborted turn reason. The bridge code had not been updated to use it.Impact
Hook authors commonly use
continue:falsefor policy, safety, or automation stop conditions. Recording the request without enforcing it can execute a tool the hook explicitly intended to prevent, issue another model request after a post-tool halt, or force continuation from a Stop hook whose result also requested termination. The audit log and actual behavior disagree at a security-sensitive control boundary.Proposed fix
The patch adds a shared
createHookHaltController()indsh-hook-protocoland applies it consistently in both bridges:Agent.cancel().stopReasonbecomes the durable hook cancellation reason, withstopped by <Event> hookas a deterministic fallback.continue:falsetakes precedence over deny/ask/context output at the same point. Integration coverage verifies all active boundaries, preserved post-tool results, absence of subsequent model requests, the SessionStart timing path, scoped subagent cancellation, and the exact aborted turn reason. The shared idle latch also has focused unit coverage.Patch
8e79b5390632a40abb31b8d3b2070346d3bd151dagent/hooks-honor-continue-falseValidation
pnpm exec vitest run packages/hooks/hook-protocol/tests packages/hooks/hooks-codex/tests packages/hooks/hooks-claude-code/tests— 19 files, 217 tests passed.pnpm run typecheck— passed, including the pre-push rerun.pnpm run lint/ finalpnpm run lint:contracts-ready— passed.pnpm run duplication— zero clones.pnpm run doc-sync— 28/28 gates passed.I would appreciate maintainer feedback on whether this cancellation mapping matches the intended Codex and Claude Code compatibility semantics.
All reactions