Skip to content

refactor(core): stop routing Goal turns through the legacy Stop hook - #11458

Merged
qqqys merged 2 commits into
QwenLM:mainfrom
qqqys:refactor/goal-drop-legacy-core
Sep 9, 2026
Merged

refactor(core): stop routing Goal turns through the legacy Stop hook#11458
qqqys merged 2 commits into
QwenLM:mainfrom
qqqys:refactor/goal-drop-legacy-core

Conversation

@qqqys

@qqqys qqqys commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Core no longer routes any Goal through the first-generation Stop-hook implementation. The LlmClient turn loop drops the fallback that read a Goal out of the in-memory legacy store whenever a turn arrived without a v3 permit, along with everything that fallback fed: the duplicate-suppressing active_goal emitter, the blocking-cap abort that reached into the store, the recursion-budget exception a legacy Goal used to get, and the Stop-hook continuation rewrite that compared hook ids to decide whether a Goal had been cleared mid-continuation. The hook execution bridge drops the two response fields that existed only so that rewrite could tell a Goal's blocking Stop output from anyone else's.

The ActiveGoal interface moves out of the legacy store module into goal-legacy-projection.ts, because it is still the shape of a live compatibility surface: the headless active_goal stream event and the ServerLlmActiveGoalEvent that carries it. It sits beside the existing LegacyActiveGoal rather than merging with it — the two differ in which fields are required, so merging would change a wire type — and the core barrel keeps exporting the same name.

The legacy modules themselves (goalHook.ts, goalJudge.ts, activeGoalStore.ts) are untouched and still compile. Two CLI call sites keep them referenced, so deleting them needs a CLI-side change first; that is the next PR in this sequence.

Why it's needed

The two generations have coexisted for a while, and the duplication is the main thing that makes the Goal code hard to read: a reader cannot tell which judge is authoritative without tracing call graphs. It is also not a live/live split. registerGoalHook's only production caller is restoreGoalFromHistory, and that function has no callers of its own, so the legacy store is never populated in a real session and the legacy judge never runs. What remained was cost: dead branches in the hottest function in core, two response fields nothing else could use, and a comment block explaining a recursion hazard that can no longer occur.

Removing core's use of the dead generation is also the prerequisite for deleting it. The files stay referenced from the CLI until the sibling PR lands, so this one is deliberately scoped to "stop calling it" rather than "delete it".

Reviewer Test Plan

How to verify

cd packages/core && npx vitest run src/core/client-goal.test.ts src/core/client.test.ts src/config/config.test.ts src/goals/goal-legacy-projection.test.ts
cd packages/core && npx vitest run src/goals src/hooks
cd packages/core && npx tsc --noEmit -p tsconfig.json

The claim worth checking rather than taking on trust is that nothing live was cut. Three things had to survive, and all three are still there: projectActiveGoal and sameActiveGoalProjection, the takePendingGoalEvents projection that turns a v3 snapshot into the active_goal event, and the ActiveGoal type itself under its original exported name. The removed emitter was a second, separate suppressor that only ever saw store-sourced Goals; the v3 one is untouched.

Also worth confirming: with the legacy branch gone the Stop-hook recursion budget is unconditionally boundedTurns - 1, which is the arm every non-legacy turn already took.

Evidence (Before & After)

Non–user-visible refactor: N/A for screenshots. The observable evidence is that the legacy path is unreachable from core and the suites are unchanged in intent:

$ git grep -n "goalHook\|activeGoalStore" -- packages/core/src \
    | grep -v "goals/goalHook.ts\|goals/activeGoalStore.ts\|goals/index.ts\|\.test\."
(no matches)

$ cd packages/core && npx vitest run src/core/client-goal.test.ts src/core/client.test.ts src/config/config.test.ts src/goals/goal-legacy-projection.test.ts
 Test Files  4 passed (4)
      Tests  1089 passed (1089)

$ cd packages/core && npx vitest run src/goals src/hooks
 Test Files  43 passed (43)
      Tests  1338 passed (1338)

$ cd packages/core && npx tsc --noEmit -p tsconfig.json
(no output)

Tested on

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

Environment (optional)

Linux, package-local vitest and tsc --noEmit for packages/core.

Risk & Scope

  • Main risk or tradeoff: this is deletion inside LlmClient's Stop-hook handling, which is dense and shared with non-Goal hooks. The mitigation is that every removed branch was guarded by a legacy-store lookup that can only return undefined in a real session, and the tests that covered those branches were removed rather than weakened — nine in client.test.ts, two in client-goal.test.ts, and one describe in config.test.ts, each because the mechanism it asserted no longer exists. One test was rewritten instead of deleted (the abort-before-continuation case), because its surviving assertion is about Stop hooks generally rather than about Goals.
  • Not validated / out of scope: the legacy modules are still compiled and still tested by their own suites; deleting them, and the CLI call sites that keep them alive, is the next PR. projectLegacyActiveGoal, the active_goal stream event, _meta.goalStatus, BridgeSessionGoal.active and the pre-v3 goal_status transcript migration are all deliberately untouched.
  • Breaking changes / migration notes: HookExecutionResponse loses two optional fields, hasNonGoalBlockingStopHook and nonGoalBlockingStopReason. Nothing in the repository writes them except the code removed here, and nothing reads them at all. No wire format, persisted record, or user-visible behaviour changes.

Linked Issues

Part of #10795. Part of #4228.

中文说明

这个 PR 做了什么

core 不再让任何 Goal 走第一代 Stop-hook 实现。LlmClient 的轮次循环去掉了那条兜底逻辑——每当一个轮次没有带 v3 permit 时,就从内存里的 legacy store 读出一个 Goal——以及这条兜底所喂养的一切:做重复抑制的 active_goal 发射器、伸手进 store 的阻塞上限中止逻辑、legacy Goal 曾经享有的递归预算例外,以及那段通过比对 hook id 来判断 Goal 是否在续跑中途被清除的 Stop-hook 续跑重写。hook 执行桥同时去掉了两个响应字段——它们存在的唯一目的就是让那段重写能把 Goal 的阻塞 Stop 输出与别人的区分开。

ActiveGoal 接口从 legacy store 模块搬到了 goal-legacy-projection.ts,因为它仍然是一个存活的兼容面的形状:headless 的 active_goal 流事件,以及承载它的 ServerLlmActiveGoalEvent。它与已有的 LegacyActiveGoal 并列而不是合并——两者在哪些字段必填上不同,合并会改变一个线协议类型——而 core 的 barrel 继续导出同一个名字。

legacy 模块本身(goalHook.tsgoalJudge.tsactiveGoalStore.ts)未被触及,仍然参与编译。有两处 CLI 调用点让它们保持被引用,所以删除它们需要先做一次 CLI 侧改动;那是本序列的下一个 PR。

为什么需要

两代实现已经共存了一段时间,而这份重复正是让 Goal 代码难读的主要原因:读者不追调用图就分不清哪个判定者才是权威。它也不是"两个都活着"的分裂。registerGoalHook 在生产代码里唯一的调用者是 restoreGoalFromHistory,而后者自己没有任何调用者,所以 legacy store 在真实会话里从不被写入,legacy judge 也从不运行。剩下的只有代价:core 里最热的那个函数中的死分支、两个别处无法使用的响应字段,以及一段解释一种已经不可能发生的递归风险的注释。

移除 core 对这代死代码的使用,也是删除它的前提。在姊妹 PR 落地之前,这些文件仍被 CLI 引用,所以本 PR 刻意把范围收在"不再调用它"而不是"删除它"。

评审验证方式

如何验证

cd packages/core && npx vitest run src/core/client-goal.test.ts src/core/client.test.ts src/config/config.test.ts src/goals/goal-legacy-projection.test.ts
cd packages/core && npx vitest run src/goals src/hooks
cd packages/core && npx tsc --noEmit -p tsconfig.json

值得亲自核对而不是照单接受的论断是:没有任何存活的东西被砍掉。有三样必须存活,而它们都还在:projectActiveGoalsameActiveGoalProjection;把 v3 快照转成 active_goal 事件的 takePendingGoalEvents 投影;以及 ActiveGoal 类型本身,仍以原来导出的名字存在。被删掉的那个发射器是第二个、独立的抑制器,它只见过来自 store 的 Goal;v3 那个未被触及。

同样值得确认:legacy 分支移除后,Stop-hook 的递归预算无条件为 boundedTurns - 1,而这正是每一个非 legacy 轮次本来就走的那一支。

证据(前后对比)

不面向用户的重构:截图部分为 N/A。可观察的证据是 legacy 路径从 core 已不可达,且测试套的意图未变:

$ git grep -n "goalHook\|activeGoalStore" -- packages/core/src \
    | grep -v "goals/goalHook.ts\|goals/activeGoalStore.ts\|goals/index.ts\|\.test\."
(no matches)

$ cd packages/core && npx vitest run src/core/client-goal.test.ts src/core/client.test.ts src/config/config.test.ts src/goals/goal-legacy-projection.test.ts
 Test Files  4 passed (4)
      Tests  1089 passed (1089)

$ cd packages/core && npx vitest run src/goals src/hooks
 Test Files  43 passed (43)
      Tests  1338 passed (1338)

$ cd packages/core && npx tsc --noEmit -p tsconfig.json
(no output)

测试环境

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

运行环境(可选)

Linux,packages/core 的包内 vitest 与 tsc --noEmit

风险与范围

  • 主要风险或权衡:这是在 LlmClient 的 Stop-hook 处理里做删除,那段代码很密,而且与非 Goal 的 hook 共用。缓解在于:每一个被删掉的分支都由一次 legacy store 查询把门,而这个查询在真实会话里只可能返回 undefined;覆盖那些分支的测试是被删除而不是被弱化——client.test.ts 里九个、client-goal.test.ts 里两个、config.test.ts 里一个 describe,各自都是因为它断言的机制已不存在。只有一个测试是被改写而不是删除(中止先于续跑那个用例),因为它仍然成立的那条断言讲的是 Stop hook 的通用行为,而不是 Goal。
  • 未验证 / 不在范围内:legacy 模块仍在编译,仍由它们自己的测试套覆盖;删除它们以及让它们存活的那些 CLI 调用点,是下一个 PR。projectLegacyActiveGoalactive_goal 流事件、_meta.goalStatusBridgeSessionGoal.active 与 pre-v3 的 goal_status transcript 迁移都刻意未动。
  • 破坏性变更 / 迁移说明:HookExecutionResponse 少了两个可选字段,hasNonGoalBlockingStopHooknonGoalBlockingStopReason。仓库里除了本次被删掉的代码之外没有任何地方写入它们,也没有任何地方读取它们。线协议格式、持久化记录与用户可见行为均未改变。

关联 Issue

Part of #10795. Part of #4228.

Goal v3 (`goal-runtime`/`goal-reducer`/`goal-protocol`) owns every Goal
surface. The first-generation Stop-hook implementation is unreachable
from core: `registerGoalHook`'s only production caller is
`restoreGoalFromHistory`, which has no callers of its own.

Cut core's use of the legacy path so those modules become unreferenced:

- Re-home `interface ActiveGoal` from `goals/activeGoalStore.ts` into
  `goals/goal-legacy-projection.ts`, next to (but deliberately separate
  from) `LegacyActiveGoal` — the two differ in optionality, so merging
  them would change a wire type. `goals/index.ts` keeps exporting the
  name.
- `core/client.ts`: drop the permit-less `getActiveGoal` fallback and the
  `maybeEmitActiveGoalChange` duplicate-suppression helper it fed, the
  `abortGoalForStopHookCap` call on the blocking-cap path, and the
  `goalContinuationChanged`/`discardGoalContinuation` continuation
  rewrite. `projectActiveGoal`/`sameActiveGoalProjection` and the v3
  snapshot projection that backs the headless `active_goal` stream event
  are untouched.
- `config/config.ts`: drop the `GOAL_HOOK_ID_OUTPUT_KEY` special-casing
  of Stop hook outputs, along with the `hasNonGoalBlockingStopHook` and
  `nonGoalBlockingStopReason` response fields that only the legacy
  continuation rewrite read.

The legacy files themselves stay for a follow-up.
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 9, 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 ✓ — every heading filled in, and the Risk & Scope section is unusually honest about what was not validated.

Problem: this is dead-code removal rather than a bug fix, so the thing to verify is whether the code really is dead — and it is. I traced the chain independently rather than taking the description's word for it: qwenGoalHookId is written in exactly one place (the legacy hook callback in goalHook.ts), that callback is only installed by registerGoalHook, and registerGoalHook's sole production caller is restoreGoalFromHistory — which has no production callers left. The three CLI files that import restoreGoal.js take only collectGoalStatusItemsFromRecords and findGoalToRestore; the #restoreGoalOnResume its docstring still mentions no longer exists. So the legacy store is never populated in a real session. Corroborating this from the other direction, Session.ts already carries the comment "The legacy store is empty for daemon sessions, so the cap above stops nothing on its own."

Direction: clearly aligned. #10795 is an open roadmap issue titled "Retire the legacy Goal Stop-hook path now that Goal v3 owns every surface", and this is the first step of exactly that. #4228 is open too. No CHANGELOG signal needed — the tracking issue is the stronger one.

Size: core paths only (packages/core/src/**). 254 production lines (additions + deletions) vs 752 test lines, 0 generated/schema. Under the 500-line threshold for the core-refactor hard block, so no size gate applies. For the record you have write access, so the maintainer exemption would have applied regardless.

Approach: the scoping is the best part of this. Stopping at "core stops calling it" instead of "delete it" is right, because the CLI still holds references — and keeping ActiveGoal as its own interface beside LegacyActiveGoal instead of merging them is the correct call, since nonInteractive/types.ts puts it on the wire as active_goal and the two differ in optionality. Two things I'd flag, neither blocking:

  • The body says "two CLI call sites keep them referenced". I count three files: Session.ts (getStopHookContinuationReason, abortGoalForStopHookCap), restoreGoal.ts (registerGoalHook, unregisterGoalHook), and MessageEmitter.ts (getActiveGoal, feeding an always-undefined _meta.stopHookLoop.goal). Worth correcting before the sibling PR sizes its own scope.
  • Once core stops calling it, activeGoalEquals has no production caller anywhere — only tests. Fine to leave here given the stated scope; just don't lose track of it in the deletion PR.

Risk: no high-risk-path match on the revert-history signal. One process note: this is a cross-repository PR with a refactor title, which trips our approval guardrail, so I won't be auto-approving it regardless of how the code review lands — a maintainer signs off on fork refactors. That's policy, not a judgement on the diff.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 每个小节都填了,Risk & Scope 部分对"哪些没有验证"写得相当坦诚。

问题: 这是删除死代码而不是修 bug,所以要核实的是"这些代码是否真的是死的"——确实是。我没有照抄 PR 描述,而是独立追了整条链:qwenGoalHookId 全仓只有一处写入(goalHook.ts 里的 legacy hook 回调),该回调只由 registerGoalHook 安装,而 registerGoalHook 在生产代码里唯一的调用者是 restoreGoalFromHistory——它自己已经没有生产调用者了。三个引用 restoreGoal.js 的 CLI 文件只取了 collectGoalStatusItemsFromRecordsfindGoalToRestore;其文档注释里提到的 #restoreGoalOnResume 已不存在。所以真实会话中 legacy store 从不被写入。反向印证:Session.ts 里本来就写着注释"legacy store 对 daemon 会话是空的,所以上面的 cap 自身停不掉任何东西"。

方向: 明确对齐。#10795 是一个开放中的 roadmap issue,标题就是"在 Goal v3 接管所有面之后退役 legacy Goal Stop-hook 路径",本 PR 正是这件事的第一步。#4228 也是开放的。不需要 CHANGELOG 信号——tracking issue 是更强的依据。

规模: 仅触及核心路径(packages/core/src/**)。生产代码 254 行(增+删)对比测试 752 行,生成/schema 0 行。低于核心重构 500 行硬阻断阈值,不触发规模门禁。另外说明一下:你有 write 权限,因此维护者豁免本来也适用。

方案: 范围切分是这个 PR 最好的地方。停在"core 不再调用它"而不是"删掉它"是对的,因为 CLI 还持有引用;把 ActiveGoalLegacyActiveGoal 并列保留而不合并也是正确判断——nonInteractive/types.ts 会把它作为 active_goal 放上线协议,而两者在必填性上不同。有两点想提出来,都不阻塞:

  • 正文写"两处 CLI 调用点让它们保持被引用"。我数到三个文件:Session.tsgetStopHookContinuationReasonabortGoalForStopHookCap)、restoreGoal.tsregisterGoalHookunregisterGoalHook)、MessageEmitter.tsgetActiveGoal,喂给一个永远为 undefined 的 _meta.stopHookLoop.goal)。建议在姊妹 PR 估算自身范围前先更正。
  • core 不再调用之后,activeGoalEquals 在全仓已无任何生产调用者——只剩测试。按既定范围留在这里没问题,只是删除 PR 里别把它漏掉。

风险: 未命中 revert 历史的高风险路径信号。一点流程说明:这是一个跨仓库(fork)且标题为 refactor 的 PR,触发了我们的审批护栏,所以无论代码审查结论如何我都不会自动批准——fork 重构需要维护者签字。这是策略,不是对 diff 的评价。

进入代码审查 🔍

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

Reviewed at 75f173d48e413f89c48fafc132d5eeb63c4c99d0 · re-run with @qwen-code /triage

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

I tried to break the central claim — that every removed branch was unreachable — and could not. The whole diff hangs on two conditions, and both are provably never true in production:

  • (a) getActiveGoal(sessionId) returns a goal. That needs setActiveGoal, whose only caller is registerGoalHook (goalHook.ts:379), whose only production caller is restoreGoalFromHistory — which has none.
  • (b) stopOutput.hookSpecificOutput['qwenGoalHookId'] is a string. Written in exactly one place, goalHook.ts:280, inside the same legacy callback.

Walking each removal against those:

  • getStopHookContinuationReason has two arms, and the goal arm ([stopReason, reason].filter(Boolean).join('\n')) is reachable only via (b). With (b) false it returns precisely stopReason || reason || 'No reason provided' — identical to what client.ts now inlines. Not "equivalent enough"; the same expression.
  • hookTurnBudget = activeGoal ? boundedTurns : boundedTurns - 1 collapses to boundedTurns - 1, the arm every non-legacy turn already took.
  • discardGoalContinuation required activeGoal !== undefined, so the !continuationReasonAfterSteer && !pendingSteer early-return was unreachable and continuationReasonAfterSteer was always continueReason. Dropping the ternary and always seeding continueRequest with one text part preserves behaviour exactly.
  • hasNonGoalBlockingStopHook was assigned only inside if (typeof goalHookId === 'string') — i.e. only via (b) — so it was always undefined, and both === false and === true comparisons in client.ts were always false. Grepping the whole repo across .ts/.tsx/.json/.md, neither removed field has a single consumer outside the lines this PR deletes, so shrinking HookExecutionResponse is safe.

The type move is handled carefully. ActiveGoal relocates verbatim — same seven fields, same optionality — and is deliberately not merged into LegacyActiveGoal, which is right because LegacyActiveGoal is readonly and differs in requiredness while ActiveGoal is genuinely on the wire (nonInteractive/types.ts types active_goal as ActiveGoal | null, and nonInteractiveCli.ts returns it from projectLegacyActiveGoal). The barrel keeps exporting the same name, the one deep import (turn.ts) is repointed, and the new activeGoalStore.tsgoal-legacy-projection.ts dependency is type-only and one-directional (that module imports only from goal-protocol.js), so no cycle.

I also confirmed the surviving emitter is the right one: takePendingGoalEvents projects active_goal from a v3 GoalSnapshotV2 through projectActiveGoal with sameActiveGoalProjection dedup, and this PR doesn't touch it. The deleted emitter was a second, separate suppressor that could only ever see store-sourced goals.

Tests. Twelve removals — nine in client.test.ts, two in client-goal.test.ts, one describe in config.test.ts — which matches the description exactly. Every one of them either calls setActiveGoal(...) in setup or mocks hasNonGoalBlockingStopHook directly, so each was constructing a state production cannot reach. Those were pins on dead code, not lost coverage. The single rewrite is good judgement: the abort-before-continuation case keeps its Goal-independent assertion (no continuation turn runs, no StopHookLoop announced) and gains expect(mockTurnRunFn).toHaveBeenCalledOnce().

Live non-Goal Stop-hook coverage survives and actually pins the changed lines — 'consumes input queued during a blocking Stop hook before its continuation' asserts the continuation text carries both the stop reason and the steer parts, which is exactly the continueRequest assembly being simplified here, and 'gives a blocking Stop hook continuation a fresh per-turn tool-call budget' covers the loopDetector.reset / hookTurnBudget path.

No blockers. Three non-blocking notes:

  1. The body undercounts the CLI's references. It says two call sites keep the legacy modules alive; there are three files — Session.ts (getStopHookContinuationReason, abortGoalForStopHookCap), restoreGoal.ts (registerGoalHook, unregisterGoalHook), and MessageEmitter.ts (getActiveGoal, feeding a _meta.stopHookLoop.goal that is therefore never populated). Worth correcting before the sibling PR sizes itself.
  2. activeGoalEquals loses its last production caller with this diff and is left exported for tests only. Consistent with the stated "stop calling it, don't delete it" scope — just don't lose track of it next PR.
  3. The equivalence proof has a shelf life. It rests on restoreGoalFromHistory staying uncalled, and that function's docstring still advertises "three of the four callers (the TUI ones)" plus an ACP #restoreGoalOnResume that no longer exists. Today the branches are dead; after this PR they're gone, so a future contributor who trusts that stale comment and re-wires the function gets a silent behaviour change instead of a dormant path. Fixing that comment is CLI-side and belongs in the next PR, but it's the thing I'd most want not to slip.

Cutting the obsolete recursion-hazard comment block rather than preserving it is the correct read of our comments convention — it described a mechanism this PR removes, so keeping it would mislead.

Files changed (12)
File What changed
packages/core/src/core/client.ts The real work: drops the legacy-store fallback, the duplicate active_goal emitter, the cap abort, the recursion-budget exception, and the hook-id continuation rewrite
packages/core/src/config/config.ts Stops computing the two goal-vs-non-goal Stop fields in the hook bridge
packages/core/src/confirmation-bus/types.ts Removes those two optional fields from HookExecutionResponse
packages/core/src/goals/goal-legacy-projection.ts New home for the ActiveGoal interface, with a comment on why it is not merged into LegacyActiveGoal
packages/core/src/goals/activeGoalStore.ts Interface lifted out; now imports the type back, plus a doc-comment reword
packages/core/src/goals/index.ts Barrel keeps exporting ActiveGoal, from the new module
packages/core/src/goals/goalHook.ts Import repoint only
packages/core/src/core/turn.ts Import repoint only
packages/core/src/core/client.test.ts Nine legacy-store-seeded tests removed, one rewritten to keep its Goal-independent assertion
packages/core/src/core/client-goal.test.ts Two tests removed (proposal settle on cleared Goal, recursion budget in a legacy chain)
packages/core/src/config/config.test.ts The bridge describe covering the two removed fields
packages/core/src/goals/activeGoalStore.test.ts Import repoint only

Testing

This is an unattended CI run, so I did not build or execute anything from this PR — the evidence below is the PR's own CI on the reviewed commit, read through the API, plus the static equivalence proof above.

Zero failures across all 71 check-runs on 75f173d48e413f89c48fafc132d5eeb63c4c99d0. Lint & Static, Integration Tests (no-AK, No Sandbox), and both Desktop Shell jobs are green. The Linux unit suite was still running when I fetched it, so the single most relevant check for a deletion-heavy refactor has not landed yet — I'm reporting that as pending rather than guessing at it. macOS and Windows unit jobs are skipped for this PR. review-pr and triage are bot orchestration, not PR CI.

Final CI results for 75f173d (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
Test (ubuntu-latest, Node 22.x) ✅ 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,失败项排在最前。

Not verified: the Linux unit suite result (still running at fetch time), and nothing here was executed locally by me. The author's own paste of 1089 + 1338 passing tests and a clean tsc --noEmit is their claim, not evidence I re-ran — the static proof above is what I'm actually relying on, and it doesn't depend on their numbers.

No sandboxed lane is named here because there is no behavioural claim to settle: this is a pure refactor whose entire assertion is that observable behaviour does not change, and an A/B run against a store that is never populated would show no difference trivially, without distinguishing "provably dead" from "my scenario never hit it". The equivalence argument above is strictly stronger evidence than a lane would be.

中文说明

代码审查

我试着去推翻它的核心论断——"每一条被删掉的分支都不可达"——没能推翻。整个 diff 依赖两个条件,而这两个条件在生产环境中可证明地永不成立:

  • (a) getActiveGoal(sessionId) 返回一个 goal。这需要 setActiveGoal,而它唯一的调用者是 registerGoalHookgoalHook.ts:379),后者在生产代码里唯一的调用者是 restoreGoalFromHistory——而它已经没有调用者了。
  • (b) stopOutput.hookSpecificOutput['qwenGoalHookId'] 是字符串。全仓只有一处写入,goalHook.ts:280,就在同一个 legacy 回调里。

对照这两个条件逐条走一遍删除:

  • getStopHookContinuationReason 有两个分支,goal 分支([stopReason, reason].filter(Boolean).join('\n'))只能经由 (b) 到达。(b) 为假时它返回的恰好是 stopReason || reason || 'No reason provided'——与 client.ts 现在内联的表达式完全一致。不是"大致等价",是同一个表达式。
  • hookTurnBudget = activeGoal ? boundedTurns : boundedTurns - 1 坍缩为 boundedTurns - 1,也就是每个非 legacy 轮次本来就走的那个分支。
  • discardGoalContinuation 要求 activeGoal !== undefined,所以 !continuationReasonAfterSteer && !pendingSteer 那个提前返回不可达,而 continuationReasonAfterSteer 恒等于 continueReason。去掉三元表达式、始终用一个 text part 起始构造 continueRequest,行为完全保持。
  • hasNonGoalBlockingStopHook 只在 if (typeof goalHookId === 'string') 内部赋值——即只能经由 (b)——所以它恒为 undefinedclient.ts=== false=== true 两处比较恒为假。在 .ts/.tsx/.json/.md 范围内全仓搜索,两个被删字段在本 PR 删掉的代码之外没有任何消费者,所以收窄 HookExecutionResponse 是安全的。

类型搬迁处理得很细致。ActiveGoal 原样迁移——七个字段、可选性都不变——并且刻意没有LegacyActiveGoal 合并。这是对的:LegacyActiveGoalreadonly 且必填性不同,而 ActiveGoal 确实在线协议上(nonInteractive/types.tsactive_goal 定义为 ActiveGoal | nullnonInteractiveCli.tsprojectLegacyActiveGoal 返回它)。barrel 继续导出同名类型,唯一的深引用(turn.ts)已改指向,而新增的 activeGoalStore.tsgoal-legacy-projection.ts 依赖是纯类型、单向的(该模块只从 goal-protocol.js 引入),所以没有循环。

我也确认了存活下来的那个发射器是对的:takePendingGoalEventsv3GoalSnapshotV2projectActiveGoal 投影出 active_goal,并用 sameActiveGoalProjection 去重,本 PR 没有碰它。被删掉的是第二个独立的抑制器,它只可能看到来自 store 的 goal。

测试。 十二处删除——client.test.ts 九处、client-goal.test.ts 两处、config.test.ts 一个 describe——与描述完全一致。每一处要么在 setup 里调用 setActiveGoal(...),要么直接 mock hasNonGoalBlockingStopHook,也就是都在构造生产环境到不了的状态。那些是钉在死代码上的钉子,不是丢失的覆盖。唯一那处改写判断得很好:abort-before-continuation 保留了与 Goal 无关的断言(不跑续跑轮次、不广播 StopHookLoop),并新增了 expect(mockTurnRunFn).toHaveBeenCalledOnce()

存活的非 Goal Stop-hook 覆盖确实钉住了被改动的代码——'consumes input queued during a blocking Stop hook before its continuation' 断言续跑文本同时包含 stop reason 和 steer 内容,正是这里被简化的 continueRequest 组装;'gives a blocking Stop hook continuation a fresh per-turn tool-call budget' 覆盖 loopDetector.reset / hookTurnBudget 路径。

无阻塞项。 三点非阻塞提醒:

  1. 正文少算了 CLI 的引用。 它说两处调用点让 legacy 模块保持存活;实际是三个文件——Session.tsgetStopHookContinuationReasonabortGoalForStopHookCap)、restoreGoal.tsregisterGoalHookunregisterGoalHook)、MessageEmitter.tsgetActiveGoal,喂给一个因此永不被填充的 _meta.stopHookLoop.goal)。建议在姊妹 PR 估算范围前先更正。
  2. activeGoalEquals 随本 diff 失去了最后一个生产调用者,只剩测试在用、仍被导出。这与"不再调用它、先不删它"的既定范围一致——只是下一个 PR 别把它漏掉。
  3. 这个等价性证明有保质期。 它依赖 restoreGoalFromHistory 保持无调用者,而该函数的文档注释仍写着"四个调用者中的三个(TUI 那些)"以及一个已不存在的 ACP #restoreGoalOnResume。今天这些分支是死的;本 PR 之后它们是消失的,所以未来若有人相信那段过期注释、把该函数重新接上,得到的是静默的行为变化而不是一条休眠路径。修这段注释属于 CLI 侧、应放在下一个 PR,但这是我最不希望被漏掉的一件事。

把已过期的递归风险注释块删掉而不是保留,是对我们注释约定的正确理解——它描述的机制正是本 PR 移除的,留着会误导。

测试

这是一次无人值守的 CI 运行,所以我没有构建或执行本 PR 的任何代码——下面的证据是 PR 自己在被审 commit 上的 CI(通过 API 读取),加上上面的静态等价性证明。

75f173d48e413f89c48fafc132d5eeb63c4c99d0 上全部 71 个 check-run 零失败。Lint & Static、Integration Tests (no-AK, No Sandbox)、两个 Desktop Shell 作业均为绿。我拉取时 Linux 单元测试仍在运行,所以对这种删除为主的重构最相关的那一项还没落地——我如实报告为 pending,而不是猜测结果。macOS 与 Windows 单元测试作业对本 PR 跳过。review-prtriage 是 bot 编排作业,不是 PR CI。

未验证:Linux 单元测试结果(拉取时仍在跑),并且我没有在本地执行任何东西。作者自己贴出的 1089 + 1338 个测试通过与 tsc --noEmit 干净,是作者的说法、不是我复跑的证据——我实际依赖的是上面的静态证明,而它不依赖这些数字。

这里没有点名沙箱验证通道,因为没有行为性论断需要落定:这是一个纯粹的重构,它全部的断言就是可观测行为不变;而对着一个从不被写入的 store 做 A/B,只会平凡地显示"没有差异",无法区分"可证明是死的"和"我的场景没走到"。上面的等价性论证是比任何通道都更强的证据。

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

Reviewed at 75f173d48e413f89c48fafc132d5eeb63c4c99d0 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — clean review; the cap is pure policy (cross-repository PR with a refactor title), not doubt about the code.

Stepping back: this is 892 net deletions and the only additive code is a sixteen-line interface relocation plus a comment rewrite. I went in expecting to argue about scope and there wasn't an argument to have — every line in the diff is load-bearing for the stated goal, there's no drive-by formatting, and the one comment block that gets cut is cut because it described the mechanism being removed. Deleting it is the right read of our convention, not a violation of it.

Against my own baseline proposal the PR came out ahead. I'd have sketched the same shape — prove the legacy store is never populated, delete the branches that guard on it, leave the modules compiling for the CLI — but I'd probably have left ActiveGoal where it was to keep the diff smaller. Moving it into goal-legacy-projection.ts is the better call: it means the eventual deletion of activeGoalStore.ts doesn't send a second wave of import churn through turn.ts and the barrel. Keeping it distinct from LegacyActiveGoal rather than collapsing two similar-looking interfaces is also the disciplined choice, and the code comment says why in one line.

The thing I'd want a human to weigh is not correctness but timing. The equivalence argument is airtight today and it is not permanent: it holds because restoreGoalFromHistory has no callers, and that is a fact about the current call graph, not a structural guarantee. Once these branches are deleted rather than dormant, re-wiring that function becomes a silent behaviour change. The mitigating news is that the sequence is real and already in flight — #11459 (refactor(cli): drop the legacy Goal Stop-hook plumbing) is open against main and rewrites restoreGoal.ts (+4/−224), removes the MessageEmitter.ts store read, and should carry the stale docstring with it. I checked that the two are order-independent: #11458 leaves every symbol the CLI still imports exported, and #11459 leaves core's dead branches harmless, so either can land first. That's a well-cut seam. What #11459 does not do is delete goalHook.ts / goalJudge.ts / activeGoalStore.ts, so a third PR is still needed before activeGoalEquals and friends stop being unused exports kept alive by tests.

On the honesty scale: this buys maintainability, not user-visible anything. The active_goal wire event, _meta.goalStatus, and the transcript migration are all untouched, and the payoff only lands once the deletion PR closes the sequence. For an open roadmap issue (#10795) whose whole point is retiring this path, that's a legitimate reason to ship — I'm just not going to dress it up as a user win.

If I picked this up in six months I'd thank them. The Stop-hook block in client.ts is the densest shared code in core, and it loses ~140 lines plus an entire "which of the two judges is authoritative" question.

Verification of the problem. For a dead-code PR the problem is the deadness, and I didn't take it on trust — three independent routes agree: the call graph bottoms out at a function with no callers; Session.ts already carries a committed comment saying the legacy store is empty for daemon sessions; and all twelve removed tests have to call setActiveGoal(...) by hand to construct the state they assert on.

Pattern. The author has thirteen PRs open, so I checked whether I was being worn down by volume. I don't think so: there's a consistent merge record behind them, I evaluated this one on its own diff, and the sibling PR existing is evidence of a plan rather than of churn.

⏸️ Deferring — not approving. Two reasons, and the second is procedural rather than a judgement on the work:

  1. Our approval guardrail blocks auto-approving a cross-repository PR whose title is a refactor, so this needs a human maintainer's sign-off no matter how the review landed.
  2. The Linux unit suite (Test (ubuntu-latest, Node 22.x)) was still running when I reviewed, and for a deletion-heavy refactor that's the check I'd least want to approve ahead of. Lint & Static, Integration Tests, and both Desktop Shell jobs were green, with zero failures across all 71 check-runs.

I could not resolve an owner to hand this to, so there's no @mention here rather than a guessed one: the PR currently carries no labels, and the area map routes on labels, so nothing matched. Worth naming the awkward part — packages/core/src/goals/ and packages/core/src/config/ both map to the PR's own author, who has write access and is excluded from owning their own PR, so the accountable reviewer has to come from elsewhere in the core area. main wants two approvals and the author can't supply one for their own change. Whoever picks this up: the review above is the whole argument, and the only open item is the unit suite landing green on 75f173d48e413f89c48fafc132d5eeb63c4c99d0.

中文说明

信心度:3/5 —— 审查本身是干净的;这个上限纯粹来自策略(跨仓库 PR 且标题为 refactor),不是对代码有疑虑。

退一步看整体:这是净删除 892 行,唯一新增的代码是一个十六行的接口搬迁加一段注释重写。我本来准备好要就范围争论一番,结果没什么可争的——diff 里每一行都为既定目标服务,没有顺手改格式,而唯一被删掉的那段注释之所以被删,正是因为它描述的机制被移除了。删掉它是对我们注释约定的正确理解,不是违反。

对照我自己的基线方案,这个 PR 做得更好。我会给出同样的形状——证明 legacy store 从不被写入、删掉以它为条件的分支、为 CLI 保留模块可编译——但我大概会把 ActiveGoal 留在原处以缩小 diff。搬到 goal-legacy-projection.ts 是更优选择:将来删除 activeGoalStore.ts 时,就不会再往 turn.ts 和 barrel 引发第二波 import 改动。把它与 LegacyActiveGoal 区分开、而不是合并两个看起来相似的接口,也是有纪律的判断,而且代码注释用一行就说清了原因。

我最希望人来权衡的不是正确性,而是时机。等价性论证今天是严密的,但它不是永久的:它成立是因为 restoreGoalFromHistory 没有调用者,而这是当前调用图的事实,不是结构性保证。一旦这些分支从"休眠"变成"被删除",重新接上那个函数就成了静默的行为变化。缓解消息是这个序列是真实存在且已在推进的——#11459refactor(cli): drop the legacy Goal Stop-hook plumbing)已基于 main 开启,重写了 restoreGoal.ts(+4/−224)、移除了 MessageEmitter.ts 的 store 读取,那段过期文档注释应该也会一并带走。我核对过两者互不依赖顺序:#11458 保留了 CLI 仍在 import 的每一个导出符号,#11459 则让 core 的死分支保持无害,所以谁先落地都可以。这是一条切得很好的缝。#11459 没有做的是删除 goalHook.ts / goalJudge.ts / activeGoalStore.ts,所以还需要第三个 PR,activeGoalEquals 之类才会停止作为"只由测试维持的未使用导出"存在。

坦白地说:这买到的是可维护性,不是任何用户可见的东西。active_goal 线协议事件、_meta.goalStatus、transcript 迁移都未被触及,而收益要等删除 PR 收尾后才落地。对于一个目的就是退役这条路径的开放 roadmap issue(#10795),这是合理的发布理由——我只是不打算把它包装成用户侧的收益。

如果六个月后由我接手这块代码,我会感谢作者。client.ts 里的 Stop-hook 区块是 core 中最密集的共享代码,它少了约 140 行,也少了"两个判定者哪个才权威"这整类问题。

问题核实。 对删除死代码的 PR 来说,"问题"就是"它是死的"这件事,我没有照单接受——三条独立路径结论一致:调用图最终落在一个没有调用者的函数上;Session.ts 里已经有一段已提交的注释说明 legacy store 对 daemon 会话是空的;而被删的十二个测试全都必须手动调用 setActiveGoal(...) 才能构造出它们所断言的状态。

模式。 作者有十三个开放 PR,所以我检查了自己是不是被数量磨软了。我认为没有:背后有稳定的合并记录,我是就这一个 diff 单独评估的,而姊妹 PR 的存在是"有计划"的证据,不是"刷量"的证据。

⏸️ 暂缓——不批准。 两个原因,第二个是流程性的,不是对这份工作的评价:

  1. 我们的审批护栏禁止自动批准跨仓库且标题为 refactor 的 PR,所以无论审查结论如何,都需要人类维护者签字。
  2. 我审查时 Linux 单元测试(Test (ubuntu-latest, Node 22.x))仍在运行,而对删除为主的重构,这恰恰是我最不愿意抢在它前面批准的一项。Lint & Static、Integration Tests、两个 Desktop Shell 作业均为绿,全部 71 个 check-run 零失败。

我没能解析出应该交给谁,所以这里没有 @提及,而不是猜一个:该 PR 目前没有标签,而 area 映射是按标签路由的,因此没有任何 area 命中。有一点值得点明其中的尴尬之处——packages/core/src/goals/packages/core/src/config/ 都映射到本 PR 作者本人,他有 write 权限,且被排除在"拥有自己的 PR"之外,所以负责的审阅者必须来自 core area 的其他人。main 需要两个批准,而作者无法为自己的改动提供其中一个。接手的人请注意:上面那份审查就是完整论证,唯一未决项是单元测试在 75f173d48e413f89c48fafc132d5eeb63c4c99d0 上落地为绿。

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

Reviewed at 75f173d48e413f89c48fafc132d5eeb63c4c99d0 · re-run with @qwen-code /triage

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

Reviewed — no blockers. Suggestions are inline.

1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:

Test Plan (not a blocker): Tests 1089 passed — this review observed 23949, 1983, 29459, 298, 1830, 515, 6854 passed; Tests 1338 passed — this review observed 23949, 1983, 29459, 298, 1830, 515, 6854 passed.

中文说明

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

本轮确认的 1 条建议级发现已在 PR 上报告过,不再重复发布(列表见上方英文部分)。

Test Plan(非阻断):Tests 1089 passed — this review observed 23949, 1983, 29459, 298, 1830, 515, 6854 passed; Tests 1338 passed — this review observed 23949, 1983, 29459, 298, 1830, 515, 6854 passed

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

Comment thread packages/core/src/config/config.test.ts
Comment thread packages/core/src/confirmation-bus/types.ts
Comment thread packages/core/src/core/client.ts Outdated
…top bridge tested

Two review findings from the deletion pass.

`client.ts` had inlined `stopReason || reason || 'No reason provided'`, which
is `DefaultHookOutput.getEffectiveReason()` verbatim -- the accessor the
sibling branch of the same `if`/`else` already calls on the same object.
Dropping the `goalHook.js` import only required dropping
`getStopHookContinuationReason`, not the accessor. Calling it again puts this
branch back under the coverage the rule already has (`hookAggregator.test.ts`
pins the `stopReason` precedence), instead of leaving a private copy in the
hottest function in core for the next change to that rule to miss.

Deleting the goal assertions from `Stop dispatch through the hook execution
bridge` also took the harness around them, which was the only thing in the
repository that drove `eventName: 'Stop'` through Config's real message-bus
handler -- while the sibling `UserPromptSubmit` and `MessageDisplay` bridge
suites were kept. Restore a goal-free pair beside them: one asserting the
branch forwards `stop_hook_active`, `last_assistant_message`, the computed
context usage and the signal positionally, wraps the final output, and
reports `allOutputs.length`; one asserting a Stop that produced no output
wraps nothing and counts zero.

`stopHookCount` is read off the bridge response rather than through a
consumer, because the field is optional and both consumers mask a missing
value with `?? 1`. The wrap is asserted on the `createHookOutput` call, since
this file replaces the hooks module with a bare mock. Verified as an
acceptance criterion: making `stopHookCount` a constant, dropping the wrap,
swapping the two positional inputs, and emptying the branch each turn the
suite red.
@qqqys

qqqys commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

All three taken, in 6296d36. I checked each one against the tree rather than taking it on trust, and all three held up.

R1-3 — the inlined fallback chain. Confirmed and fixed. getEffectiveReason() is this.stopReason || this.reason || 'No reason provided' at hooks/types.ts:425-427, StopHookOutput overrides only getStopReason, and the sibling branch of the same if/else already calls the accessor on the same object at client.ts:4387. Now both branches do. Worth adding to your reasoning: this is not only deduplication — hookAggregator.test.ts:255 feeds { continue: false, stopReason: 'first hook stopped' } and asserts getEffectiveReason() returns it, so calling the accessor puts this path back under the coverage the precedence rule already has. That is why I did not add the optional client.test.ts precedence case you offered: re-inlining the expression would now be the deliberate act, and the rule itself stays pinned at its source.

R1-1 — the Stop bridge harness. Confirmed, including the scope of "only". The remaining eventName: 'Stop' hits outside the deleted block do not reach that branch: Session.test.ts:30755 runs against mockConfig.getMessageBus = vi.fn().mockReturnValue(messageBus), so it asserts what the caller sends, never what Config does with it; the hooksCommand and daemon-status hits are UI and status fixtures.

Restored a goal-free pair beside the two surviving bridge suites. Two deviations from your sketch, both forced by the file:

  • The wrap is asserted on the createHookOutput call, not on response.output. This file replaces ../hooks/index.js wholesale with createHookOutput: vi.fn() (line 255), so the wrap returns undefined here no matter what the branch does. Asserting the call with ('Stop', blockingOutput) pins the same thing and cannot be fooled by that mock.
  • The forwarded signal is pinned by passing a real AbortController().signal in the request and asserting identity. expect.anything() does not match the undefined this path passes when the request carries no signal, which is how I found out.

stopHookCount is read off the bridge response, as you required — through a consumer the ?? 1 masks would hide a producer that stopped setting it.

I treated your "the restored case is its own acceptance criterion" as a requirement rather than a remark, and mutated the branch four ways to check it:

mutation result
stopHookCount = 1 instead of allOutputs.length 2 failed
drop the createHookOutput('Stop', …) wrap 1 failed
swap stop_hook_active and last_assistant_message 1 failed
empty the whole case 'Stop': body 2 failed

config.test.ts 641, client.test.ts 389, client-goal.test.ts 48 — 1078 passed.

R1-2 — the design doc. Confirmed, and recording it here as a deferral rather than editing in place, which is the option you flagged as cheaper. The reason it is cheaper than you knew: docs/design/goal-loop-input-control.md is not getting a status note, it is getting deleted. It opens with "An active /goal is implemented as a blocking Stop hook" and describes nothing else, so it is on the list for the documentation slice of this sequence, together with the Stop-hook paragraphs in four other design docs, the orphan i18n key, and the stale acp-bridge/src/status.ts comment. Adding a note now and deleting the file two PRs later would be churn, and it would also drag in authoring the missing Chinese counterpart for a file scheduled to disappear.

Deferred to the follow-up PRs in this sequence, so nothing is silently dropped:

  1. Delete goalHook.ts, goalJudge.ts and activeGoalStore.ts with their suites. Blocked on the CLI-side sibling (refactor(cli): drop the legacy Goal Stop-hook plumbing #11459), which removes the last two references.
  2. Delete docs/design/goal-loop-input-control.md, and correct the Stop-hook paragraphs in 2026-08-08-selective-session-restore.md, 2026-09-02-goal-pause-reasons.md, 2026-09-07-goal-no-progress-pause.md, 2026-09-07-goal-continuation-budget.md and 2026-08-25-goal-draft-skill.md. This is where R1-2 lands.
  3. Drop the abortGoalForStopHookCap guard in session/Session.ts, which its own comment says can only return false for daemon sessions. Behaviour-bearing, so it deserves its own commit rather than riding along.

@qqqys
qqqys enabled auto-merge September 9, 2026 12:50

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

Approving. Clean removal of the legacy Goal Stop-hook routing: client.ts drops the activeGoalAtTurnStart read-through and the goalHook/activeGoalStore imports, goalHook.ts drops the stop-hook-cap exports, and the new ActiveGoal interface in goal-legacy-projection.ts is deliberately kept separate from LegacyActiveGoal so the wire type doesn't silently change. Net −789 lines, tests green (1089/1338 passed), CI fully green. Bot review found no blockers — the one Suggestion (orphaned activeGoalEquals export) is non-blocking.

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

Scope: packages/core/src source and test changes only. NOT reviewed: CLI callers of the legacy modules (out of scope per PR description); Windows/macOS runtime behavior (no host).


What I checked

Class 1 — contract asymmetry (getStopHookContinuationReasongetEffectiveReason).
Old function had two branches: no-Goal output → stopReason || reason; Goal output → [stopReason, reason].join('\n'). New DefaultHookOutput.getEffectiveReason() always does stopReason || reason. The Goal-specific join branch was only reachable when the legacy store held an active goal, which is never the case in production (sole writer restoreGoalFromHistory has no callers). For every live Stop hook, old and new produce identical output. Clean.

Class 2 — API/export surface. ActiveGoal interface: moved from activeGoalStore.ts to goal-legacy-projection.ts, re-exported from goals/index.ts at the same name (confirmed at HEAD line 59). Callers importing from the barrel are unaffected. HookExecutionResponse field removals: no write site or read site outside the deleted code at HEAD. Clean.

Class 7 — zombie state / hookTurnBudget change. Old: activeGoal ? boundedTurns : boundedTurns - 1. New: unconditionally boundedTurns - 1. Since getActiveGoal(sessionId) returns undefined whenever the legacy store is empty — which is always in production — this always resolved to boundedTurns - 1. Simplification is correct. Clean.

continueRequest simplification. For the only live path (no legacy Goal in flight), old code set continuationReasonAfterSteer = continueReason unchanged and built [{ text: continueReason }]. New code does exactly the same. The early-return path (settle goal proposal and stop when both reason and steer are absent) was only entered when a legacy goal changed mid-continuation — dead in production. Clean.

Config test suite (surviving case 'Stop': branch). The old parametric it.each describe was replaced, not deleted: two new focused tests exercise the surviving bridge branch with positional assertions and a count check. Adequate.


Cross-check against existing reviews

Three prior bot suggestions; author reports all three addressed in 6296d36 (current HEAD):

Bot finding My check Outcome
Config test: surviving Stop branch coverage lost New describe has 2 tests covering case 'Stop': with positional + count assertions Confirmed resolved
types.ts: removed fields still named in design doc Nothing reads or writes the fields at HEAD; no wire format changes Confirmed resolved
client.ts: inlined fallback is a copy of getEffectiveReason() HEAD calls stopOutput.getEffectiveReason() directly Confirmed resolved

yiliang114 approval — independently reached the same conclusion (clean deletion of dead legacy Goal routing).


Unreviewed dimensions: CLI callers of registerGoalHook/restoreGoalFromHistory not reviewed (author scopes to packages/core; CLI-side deletion is the stated next PR). No execution rung run — working tree unavailable; not material to approval given the code paths are verifiably dead (legacy store never written in production).

Reviewed with AI assistance.

@qqqys
qqqys added this pull request to the merge queue Sep 9, 2026
Merged via the queue into QwenLM:main with commit 3733c8e Sep 9, 2026
58 of 59 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.

Approved at head 6296d362.

Required CI is green here — Test (ubuntu-latest, Node 22.x), Lint & Static, Integration Tests (no-AK, No Sandbox), web-shell E2E Smoke and both Desktop Shell lanes completed successfully; only review-pr (the reviewer's own job) is still running.

I did not take the "it was already dead" premise on trust, because the whole diff stands on it:

  • registerGoalHook has exactly one non-test caller (cli/src/ui/utils/restoreGoal.ts:369), and it lives inside restoreGoalFromHistory (:324), which has no non-test caller anywhere. Nothing else outside core/src/goals/ writes the store, so every removed getActiveGoal(...) arm evaluated to undefined in a real session. The one remaining non-test consumer of the legacy API, abortGoalForStopHookCap at cli/src/acp-integration/session/Session.ts:6606, still resolves — this PR only stops core from calling it.
  • Dropping getStopHookContinuationReason for stopOutput.getEffectiveReason() preserves the produced string on every reachable input: the two differ only for an output carrying qwenGoalHookId, and only the goal hook ever set that key. createHookOutput returns DefaultHookOutput (hooks/types.ts:359-387), which declares the accessor and is not overridden by StopHookOutput, so this also matches the sibling goal-permit branch that already called it at :4387.
  • The ActiveGoal move is complete: turn.ts, goalHook.ts and activeGoalStore.test.ts were repointed, goals/index.ts still exports the same name, and no module imports it from activeGoalStore.js any more.
  • boundedTurns - 1 is the arm every non-legacy turn already took, and LlmEventType.ActiveGoal is still produced by the surviving v3 projection (client.ts:2921-2940, through projectActiveGoal / sameActiveGoalProjection) — which client-goal.test.ts:1091, :1629 and :1679 still assert, so the duplicate-suppression contract did not lose its only observer.

Test coverage is preserved rather than narrowed: the Stop dispatch through the hook execution bridge describe at config.test.ts:13093 was rewritten goal-free in place instead of being dropped with the two response fields, and it still drives eventName: 'Stop' through the real message bus — asserting the positional fireStopEvent forwarding and reading stopHookCount (2 / 0) off the bridge response itself rather than through the ?? 1 consumers. So the surviving case 'Stop': branch keeps the only observer that can go red if it stops forwarding stop_hook_active / last_assistant_message, loses the createHookOutput('Stop', ...) wrap, or hard-codes the count.

No new Critical found. One non-blocking follow-up, which the earlier inline suggestion asked for and this head does not carry: docs/design/goal-loop-input-control.md:32-37 and its Verification bullet at :45-46 still specify the goal-vs-non-goal Stop separation and the "aggregated independent blocker" tests that this change retires. Worth a status note there, or an explicit deferral owned by the follow-up deletion PR, so the next reader doesn't go hunting for code that no longer exists.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.23.3.

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.

5 participants