Skip to content

feat(goal): let the model propose a Goal the user approves in a dialog - #10171

Open
qqqys wants to merge 6 commits into
QwenLM:mainfrom
qqqys:feat/propose-goal-tool
Open

feat(goal): let the model propose a Goal the user approves in a dialog#10171
qqqys wants to merge 6 commits into
QwenLM:mainfrom
qqqys:feat/propose-goal-tool

Conversation

@qqqys

@qqqys qqqys commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Adds a propose_goal core tool: the model proposes a session Goal, the user sees the full objective in an approval dialog, and only their approval sets it — exactly as /goal set would. Declining sets nothing; the model is told only that the Goal was not set and must not propose it again. The tool is registered beside get_goal / update_goal (so never for subagents), only when the host can show a dialog (interactive TUI, ACP, stream-json — never plain headless) and only while the new goals.modelProposed setting is not "disabled".

The dialog reuses the generic info confirmation, so no host needs a new component: the objective is in the invocation description (the one field every host forwards — the Web Shell does not render an info prompt) and in the plain-text prompt together with a one-sentence explanation of what approving does. The invocation declares requiresUserInteraction(), so no permissions.allow rule, no skill allowedTools grant, and no approval mode — YOLO included, and AUTO_EDIT, which auto-approves info confirmations — can skip the dialog. Preconditions are checked before the dialog and again in execute(), because /goal can change the session while the dialog is open: plan mode, an untrusted folder, a session without Goal persistence, and an active Goal all refuse with guidance. An active Goal is never replaced from the tool (the model is told to hand over a /goal edit … / /goal set … line instead); a stopped Goal (paused, blocked, complete, usage-limited) is replaced through replace with its expected version, so a Goal that changed under the dialog is rejected rather than clobbered. The tool never dispatches from inside the turn: creating the Goal mid-turn would leave the proposing turn's remaining tool-result continuations without a Goal permit (the first end-to-end run surfaced exactly that as an An active Goal requires an exact turn permit error card). The approval is parked on Config and the client applies it at the boundary a typed /goal set takes effect — once the model has stopped calling tools, before Stop hooks — so the runtime's broadcast renders the Goal card and starts the first Goal turn right after the model's one-sentence acknowledgement. An approval still parked when the next real user query starts belongs to a cancelled turn and is discarded.

goals.modelProposed ("alwaysAsk" default, "disabled") is added to the settings schema and to WORKSPACE_RESTRICTED_SETTINGS, so a repository's .qwen/settings.json cannot switch the tool on; the CLI normalizes the value to the closed enum before it reaches core. The bundled goal-draft skill now hands off through propose_goal when it is available and no Goal is active, and keeps its printed /goal set line for headless runs, the disabled setting, and the active-Goal case. Docs (goals.md, settings.md, the design note) and the Web Shell tool-name tables are updated.

Why it's needed

#10002 shipped /goal-draft, which ends by printing a /goal set … line for the user to copy and run — because built-in commands are not model-invocable, and deliberately so: starting an unattended loop is the user's decision. That hand-off works but is clumsy (a 1,000-character line to paste, and on some terminals the backticks get escaped), and it is the one place where the model's careful draft can be lost. Claude Code solves this with ProposeGoal + a modelProposedGoals setting: the model proposes, the user approves with one keypress. This PR is the qwen-code equivalent, with two deliberate differences: there is no auto mode that lets the model skip the dialog, and the tool never replaces a running Goal.

Reviewer Test Plan

How to verify

  1. cd packages/core && npx vitest run src/goals src/core/client-goal.test.ts src/config/config.test.ts src/skills/bundled/goal-draft/SKILL.test.ts and cd packages/cli && npx vitest run src/config/settings.test.ts src/config/config.test.ts — all pass. New cases: ProposeGoalTool (dialog contents, approval parks the proposal and applyPendingGoalProposal then sets the Goal and the runtime starts the first Goal turn, cancel parks nothing, refuses in plan mode / untrusted / no persistence / active Goal, replaces a paused Goal, refuses over a Goal that became active meanwhile, reports a moved version as a conflict, validation), the client boundary (a parked approval is applied once a user turn ends without tool calls; one still parked at the next user query is discarded), registration (present in interactive/ACP/stream-json, absent headless and when disabled), goals.modelProposed scope (honored from user scope, stripped from workspace scope with a warning), and the CLI normalizer.
  2. TUI, Ask mode, in a trusted project with a test script: /goal-draft make the auth tests pass and raise coverage. Expected: after drafting, a confirmation card Propose Goal: Outcome: … with the objective and "Set this as the session Goal? Approving sets it like /goal set …", options "Yes, allow once" / "No" only (no "Always allow"). Enter → Goal card create, the footer Goal pill appears, and the first Goal turn starts on its own; /goal shows revision 1 with the proposed objective.
  3. Same, but press Esc on the dialog: no Goal is set, /goal reports none, and the model does not re-propose.
  4. YOLO (-y) interactive: the dialog still appears. Plan mode: the tool returns "Keep planning; propose the Goal after the plan is approved." With a Goal active: the model prints a /goal edit … line instead.
  5. qwen -p "/goal-draft …" (headless): unchanged — propose_goal is not registered and the /goal set line is printed.
  6. Put "goals": {"modelProposed": "disabled"} in ~/.qwen/settings.json: the tool disappears (the skill falls back to the printed line). Put the same in .qwen/settings.json of a project: a startup warning says the value is ignored.

Evidence (Before & After)

TUI in YOLO mode (this machine's default), trusted throwaway project with test / coverage scripts, driven in tmux (160×48) against DashScope qwen3.8-max; screenshots are the tmux pane rendered with its own colors.

Before (main + #10002): /goal-draft ends with a /goal set … line to copy; there is no propose_goal tool.

After — 1. The skill's clarifying questions still come as a real dialog in YOLO (that is #10160), and the answers feed the draft.

After — 2. propose_goal renders the objective and the one-sentence explanation in the approval dialog. Only "Yes, allow once" / "No" are offered — no "Always allow", even in YOLO.

After — 3. Enter → the tool card reports Goal approved ·, the model acknowledges in one sentence and stops, and at that boundary the runtime sets the Goal: the Goal active card appears and the footer pill reads /goal active. No An active Goal requires an exact turn permit error card (the first end-to-end run, which set the Goal from inside the tool, produced one right here — hence the boundary application).

After — 4. The runtime drives the first Goal turn on its own (here: diagnosing the failing node --test test/ invocation) while the pill counts up.

On the first end-to-end run the same loop ran to the end: the runtime installed c8, added the missing-branch tests, pasted both check lines, and the independent verifier accepted the completion (Goal complete · 1 turn · 3m 57s). Cancelling the dialog with Esc leaves /goal empty (unit-tested; the scheduler cancels the call before execute, and execute itself parks nothing without approval).

Tested on

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

Environment (optional)

Linux, Node 22, npm ci + npm run build + npm run bundle; TUI driven in tmux (160×50) against DashScope qwen3.8-max.

Risk & Scope

  • Main risk or tradeoff: the dialog is the generic info confirmation, so its buttons read "Yes, allow once" / "No, suggest changes" in the TUI and "Allow" / "Reject" in the Web Shell and ACP clients rather than "Set this goal" / "Not now"; the prompt text explains what approving does. A dedicated confirmation type would need rendering in all three hosts and is left for later. The tool never replaces an active Goal — replacing mid-run would preempt the running Goal turn from inside one of its own tool calls.
  • Not validated / out of scope: Web Shell and ACP dialogs are covered by the existing info path and unit tests, not by a new screenshot; parseGoalCommand still flattens newlines (separate follow-up); no auto mode.
  • Breaking changes / migration notes: none. A new setting with a safe default; the skill's printed hand-off remains wherever the tool is absent.

Linked Issues

Follow-up to #10002 (P2 of the goal-draft design note).

中文说明

这个 PR 做了什么

新增核心工具 propose_goal:模型提议一个会话 Goal,用户在审批对话框里看到完整 objective,只有用户批准才会设定——效果与 /goal set 完全一致。拒绝则什么都不设;模型只被告知"Goal 未设定",且不得再次提议。该工具与 get_goal / update_goal 一起注册(因此子 agent 永远没有),仅在宿主能显示对话框时(交互式 TUI、ACP、stream-json——普通 headless 没有)、且新设置 goals.modelProposed 不为 "disabled" 时才注册。

对话框复用通用的 info 确认类型,三端都不需要新组件:objective 同时放在调用描述里(这是每个宿主都会转发的唯一字段——Web Shell 不渲染 info 的 prompt)和纯文本 prompt 里,并附一句"批准意味着什么"的说明。调用声明了 requiresUserInteraction(),所以任何 permissions.allow 规则、skill 的 allowedTools 授权、任何审批模式——包括 YOLO,以及会自动批准 info 确认的 AUTO_EDIT——都跳不过对话框。前置条件在弹窗前检查一次、execute() 里再检查一次,因为对话框打开期间 /goal 可能改变会话:plan 模式、不受信任的目录、没有 Goal 持久化的会话、以及已有活跃 Goal,都会带指引拒绝。活跃 Goal 永远不会被这个工具替换(模型被要求改为给出 /goal edit … / /goal set … 行);已停止的 Goal(paused、blocked、complete、usage-limited)通过带期望版本的 replace 替换,所以在对话框下被改动过的 Goal 会被拒绝而不是被覆盖。工具从不在 turn 内部 dispatch:在 turn 中途创建 Goal 会让本 turn 剩余的工具结果续跑失去 Goal permit(第一次端到端运行正是暴露了 An active Goal requires an exact turn permit 这个错误卡片)。批准被暂存在 Config 上,由 client 在与手动 /goal set 相同的边界——模型停止调用工具之后、Stop hook 之前——统一应用,于是 runtime 的广播渲染 Goal 卡片,并在模型的一句话确认之后紧接着启动首个 Goal turn。下一条真实用户消息开始时仍暂存的批准属于被取消的 turn,会被丢弃。

goals.modelProposed(默认 "alwaysAsk",可选 "disabled")加入设置 schema 和 WORKSPACE_RESTRICTED_SETTINGS,仓库的 .qwen/settings.json 无法打开这个工具;CLI 会先把值规范到封闭枚举再传给 core。内置 goal-draft skill 现在在工具可用且没有活跃 Goal 时通过 propose_goal 交接,在 headless、设置禁用、或已有活跃 Goal 时保留打印 /goal set 行的方式。文档(goals.mdsettings.md、设计说明)和 Web Shell 的工具名表已更新。

为什么需要

#10002 交付的 /goal-draft 以打印一行 /goal set … 让用户复制运行收尾——因为内置命令不可被模型调用,而且这是刻意的:启动无人值守循环应由用户决定。这个交接能用但笨拙(一行 1000 字符要粘贴,某些终端还会转义反引号),也是模型精心起草的内容最容易丢失的地方。Claude Code 用 ProposeGoal + modelProposedGoals 设置解决:模型提议,用户一键批准。本 PR 是 qwen-code 的对应实现,有两处刻意的差异:没有让模型跳过对话框的 auto 模式;工具永远不替换运行中的 Goal。

Reviewer 测试计划

如何验证

  1. cd packages/core && npx vitest run src/goals src/core/client-goal.test.ts src/config/config.test.ts src/skills/bundled/goal-draft/SKILL.test.tscd packages/cli && npx vitest run src/config/settings.test.ts src/config/config.test.ts——全部通过。新增用例:ProposeGoalTool(对话框内容、批准后暂存提案且 applyPendingGoalProposal 随后设定 Goal 并由 runtime 启动首个 Goal turn、取消不暂存、plan 模式 / 未信任 / 无持久化 / 活跃 Goal 时拒绝、替换 paused Goal、期间变为活跃则拒绝、版本移动报冲突、参数校验),client 边界(用户 turn 无工具调用结束时应用暂存的批准;下一条用户消息开始时仍暂存的被丢弃),注册(交互/ACP/stream-json 存在,headless 与禁用时不存在),goals.modelProposed 作用域(user 层生效,workspace 层剥离并告警),以及 CLI 的规范化函数。
  2. TUI、Ask 模式、受信任且带 test 脚本的项目:/goal-draft make the auth tests pass and raise coverage。预期:起草后出现确认卡片 Propose Goal: Outcome: …,带 objective 和"Set this as the session Goal? Approving sets it like /goal set …",选项只有 "Yes, allow once" / "No"(没有 "Always allow")。回车 → Goal 卡片 create、底部 Goal pill 出现、首个 Goal turn 自动开始;/goal 显示 revision 1 与提议的 objective。
  3. 同上但在对话框按 Esc:没有 Goal 被设定,/goal 报告无,模型不会再提议。
  4. YOLO(-y)交互:对话框仍然出现。Plan 模式:工具返回 "Keep planning; propose the Goal after the plan is approved."。已有活跃 Goal:模型改为打印 /goal edit … 行。
  5. qwen -p "/goal-draft …"(headless):不变——propose_goal 未注册,仍打印 /goal set 行。
  6. ~/.qwen/settings.json"goals": {"modelProposed": "disabled"}:工具消失(skill 回退到打印行)。在项目 .qwen/settings.json 写同样内容:启动时告警该值被忽略。

证据(Before & After)

Before:/goal-draft 以一行待复制的 /goal set … 结束;没有 propose_goal 工具。

After:见上方英文部分的 5 张 tmux 截图——skill 提问对话框(YOLO 下仍弹出)、propose_goal 审批对话框(只有 Yes/No,无 Always allow)、批准后 Goal approved · → 模型一句话确认 → 边界处 Goal active 卡片与底部 /goal active pill、以及 runtime 自行驱动首个 Goal turn;没有再出现 An active Goal requires an exact turn permit 错误卡片。首次端到端运行中同一循环跑到了 Goal complete · 1 turn · 3m 57s

测试平台

Linux ✅;macOS、Windows ⚠️ 未测试。

环境(可选)

Linux,Node 22,npm ci + npm run build + npm run bundle;TUI 在 tmux(160×50)中针对 DashScope qwen3.8-max 驱动。

风险与范围

  • 主要风险/取舍:对话框是通用 info 确认,TUI 里按钮文案是 "Yes, allow once" / "No, suggest changes",Web Shell 与 ACP 客户端里是 "Allow" / "Reject",而不是 "Set this goal" / "Not now";prompt 文本解释了批准的含义。专用确认类型需要三端各自渲染,留待后续。工具永远不替换活跃 Goal——运行中替换会从 Goal 自己的工具调用内部抢占正在运行的 Goal turn。
  • 未验证/范围外:Web Shell 与 ACP 的对话框由现有 info 路径和单元测试覆盖,没有新截图;parseGoalCommand 仍会压平换行(另行跟进);没有 auto 模式。
  • 破坏性变更/迁移说明:无。新设置有安全默认值;工具不存在的地方 skill 仍打印交接行。

关联 Issue

#10002 的后续(goal-draft 设计说明的 P2)。

https://claude.ai/code/session_01FV7i3w7egJ2kMw4AhQC38Z

Adds a propose_goal core tool: the model proposes a session Goal, the
user sees the full objective in an approval dialog, and only their
approval sets it, exactly as /goal set would. Declining sets nothing and
the model is told only that the Goal was not set. The tool is registered
beside get_goal/update_goal (never for subagents), only where a dialog
can be shown (interactive, ACP, stream-json; never plain headless) and
only while goals.modelProposed is not "disabled".

The dialog reuses the generic info confirmation, so no host needs a new
component: the objective is in the invocation description (the one field
every host forwards) and in the plain-text prompt with a one-sentence
explanation. requiresUserInteraction() is true, so no allow rule, skill
grant, YOLO, or AUTO_EDIT (which auto-approves info confirmations) can
skip the dialog. Preconditions are checked before the dialog and again in
execute(), because /goal can change the session while it is open: plan
mode, an untrusted folder, no Goal persistence, and an active Goal refuse
with guidance. An active Goal is never replaced from the tool; a stopped
Goal is replaced through `replace` with its expected version. The tool
only dispatches: the runtime's broadcast renders the card and starts the
first Goal turn after the current turn ends.

goals.modelProposed (alwaysAsk | disabled) joins WORKSPACE_RESTRICTED_SETTINGS
so a repository cannot switch the tool on; the CLI normalizes it to the
closed enum. The goal-draft skill hands off through propose_goal when it is
available and no Goal is active, and keeps its printed /goal set line for
headless runs, the disabled setting, and the active-Goal case.

Follow-up to QwenLM#10002 (P2 of the goal-draft design note).

Claude-Session: https://claude.ai/code/session_01FV7i3w7egJ2kMw4AhQC38Z
@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Deferred approval withheld — 1 PR CI workflow run(s) on e2c77fd did not finish green; see the updated table in the Stage 2 comment. Re-run @qwen-code /triage after fixes. finalize run

⚠️ 延迟审批已搁置 —— e2c77fd 有 1 个 PR CI workflow 未以绿色完成,详见 Stage 2 评论中已更新的表格。修复后可重新运行 @qwen-code /triage查看 finalize 运行

… columns

The first cut used a quoted default that widened the Default column, so
prettier re-aligned 46 unrelated rows. The row now fits the existing
widths and the table diff is the one added line.

Claude-Session: https://claude.ai/code/session_01FV7i3w7egJ2kMw4AhQC38Z
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: real and observed. #10002 shipped /goal-draft with a paste-a-1,000-character-line hand-off, and the merged design doc for it explicitly lists this propose_goal tool as the next phase. The PR also includes a real before/after tmux session, not a hypothetical.

Direction: aligned. This is the planned P2 of the goal-draft design note, and Claude Code's /goal + ProposeGoal is the established analogue (its changelog shows /goal under active iteration; no direct ProposeGoal entry to cite). The two deliberate deviations — no auto mode, never replacing a running Goal — are the right conservative calls for a consent surface that starts an unattended loop.

Size: core paths touched (packages/core/src/goals, config, tools, permissions + CLI settings). Breakdown: ~352 production lines, ~339 test lines, ~14 schema, ~25 docs/skill content — under the 500-line maintainer-awareness bar. 19 files, but every one of them serves the feature (tool, registration, setting, scope restriction, skill hand-off, display names, docs). No drive-by changes spotted.

Approach: scope feels right — it matches what the design doc scoped. Reusing the generic info confirmation instead of adding a per-host dialog component is the simple path, and the tradeoff (generic button labels) is called out honestly in the PR body. Keeping the printed /goal set fallback for headless/disabled/active-Goal cases preserves every existing path.

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

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:真实存在。#10002 交付的 /goal-draft 以"粘贴一行 1000 字符命令"收尾,其已合并的设计文档明确把 propose_goal 工具列为下一阶段。PR 还附了真实的 before/after tmux 记录,不是假设性问题。

方向:对齐。这是 goal-draft 设计说明里规划好的 P2;Claude Code 的 /goal + ProposeGoal 是已确立的同类方案(其 changelog 显示 /goal 在持续迭代,没有直接的 ProposeGoal 条目可引用)。两处刻意差异——没有 auto 模式、永不替换运行中的 Goal——对于"启动无人值守循环"这种同意面来说是正确的保守选择。

规模:触及核心路径(packages/core/src/goalsconfigtoolspermissions 及 CLI 设置)。拆分:约 352 行生产代码、约 339 行测试、约 14 行 schema、约 25 行文档/skill 内容——低于 500 行维护者关注线。共 19 个文件,但每个都服务于该功能本身(工具、注册、设置、作用域限制、skill 交接、显示名、文档),未发现顺手改动。

方案:范围合理,与设计文档圈定的范围一致。复用通用 info 确认而不是给每个宿主加新对话框组件,是更简单的路径;代价(按钮文案是通用的)在 PR 正文里也如实说明了。headless/禁用/已有活跃 Goal 时保留打印 /goal set 行的回退,现有路径全部保留。

风险:无升级风险信号——改动文件均未命中与 revert 相关的高风险路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

Code review

The approach matches what I'd have proposed independently — new tool beside get_goal / update_goal, reuse the existing info confirmation so no host needs new UI, gate registration on host capability and the opt-out setting, user-scope-only consent, skill hand-off with the text fallback kept. I verified the load-bearing claims against the base code rather than taking the PR's word for them:

  • The dialog cannot be skipped. requiresUserInteraction() is an established mechanism (used by exit_plan_mode) and I checked every bypass route in the scheduler: permissionFlow forces ask over allow rules when it is set; needsConfirmation returns true before the YOLO override; the AUTO-mode three-layer filter, the AUTO_EDIT auto-approval of info dialogs, and the PermissionRequest hook's allow path are all guarded by !requiresUserInteraction (hooks can still deny). The scheduler also sets hideAlwaysAllow, which matches the evidence showing only "Yes, allow once" / "No". Deny rules still work, as they should.
  • Registration gating is structural, not advisory. The registration sits inside registerGoalWorkerTools, which early-returns for subagent contexts; the new gate checks resolveInteractionMode !== 'headless' and the setting. Headless, subagents, and the disabled setting all miss the tool; the registration-matrix tests pin this.
  • TOCTOU handling is real. Preconditions run before the dialog and again in execute(); a replace carries expectedGoalId / expectedRevision and the goal reducer's version assert turns a concurrent /goal change into a friendly refusal instead of a clobber. Without an onConfirm approval the invocation fails closed (approved stays false).
  • Scope restriction works through the existing generic machinery. WORKSPACE_RESTRICTED_SETTINGS strips workspace values and warns; the Settings type is schema-derived so the new goals section type-checks end to end; the CLI normalizer closes the enum before it reaches core. Same pattern as agents.crossSession*.
  • renderPromptAsPlainText and the info confirmation are established surfaces (the Workflow tool uses both), so no host renders anything new.

One non-blocking nit: propose_goal isn't added to the two existing goal-tool enumerations — the chat-recording goal_runtime provenance tagging (coreToolScheduler.ts ~6294, useGeminiStream.ts ~3936/~4878) and the tool-use summarizer exclusion (useGeminiStream.ts ~5052). Practical impact is small: the tool refuses while a Goal is active, so it effectively never carries a goalContext, and the exclusion only matters because a fast-model summary could echo beside the tool's own "Goal set · …" display. Worth adding for consistency in a follow-up or a quick amend — not a merge blocker.

sequenceDiagram
    participant P1 as Model
    participant P2 as Tool scheduler
    participant P3 as ProposeGoalInvocation
    participant P4 as User dialog
    participant P5 as Goal runtime
    P1->>P2: calls propose_goal with objective
    P2->>P3: permission flow forces ask and hides always-allow
    P3->>P3: checks plan mode, trust, persistence, active Goal
    P3->>P4: info dialog with the full objective
    P4-->>P3: approve or decline
    P3->>P5: dispatch create or versioned replace
    P5-->>P3: Goal record or conflict error
    P3-->>P1: result telling it to acknowledge and end the turn
    P5->>P5: broadcast renders Goal card and starts first Goal turn
Loading
Files changed (19 of 19 shown)
File What changed
docs/design/2026-08-25-goal-draft-skill.md Design doc: adds the Phase 2 section, moves propose_goal out of Later phases
docs/users/configuration/settings.md Docs: goals.modelProposed settings table row
docs/users/features/goals.md Docs: user guide for the approval-dialog flow
packages/cli/src/config/config.test.ts CLI tests: enum normalizer
packages/cli/src/config/config.ts CLI: normalizer closes the enum before it reaches core
packages/cli/src/config/settings.test.ts CLI tests: user scope honored, workspace scope stripped with warning
packages/cli/src/config/settingsSchema.ts CLI: settings schema entry for goals.modelProposed
packages/cli/src/config/settingsUtils.ts CLI: adds goals.modelProposed to the workspace-restricted list
packages/core/src/config/config.test.ts Core tests: registration matrix (interactive, ACP, stream-json, disabled, headless)
packages/core/src/config/config.ts Core: Config param, getter, and gated lazy registration
packages/core/src/goals/goal-tools.test.ts Core tests: ProposeGoalTool behavior suite
packages/core/src/goals/goal-tools.ts Core: the ProposeGoalTool and its invocation
packages/core/src/permissions/rule-parser.ts Core: tool-name aliases for permission rules
packages/core/src/skills/bundled/goal-draft/SKILL.md Skill: tool-first hand-off with the printed-line fallback
packages/core/src/skills/bundled/goal-draft/SKILL.test.ts Skill tests: pins hand-off wording, no allowedTools grant
packages/core/src/tools/tool-names.ts Core: name and display-name constants
packages/vscode-ide-companion/schemas/settings.schema.json VS Code companion: settings schema mirror
packages/web-shell/client/components/messages/toolFormatting.ts Web Shell: display-name mapping
packages/web-shell/client/i18n.tsx Web Shell: zh tool-name string

Testing evidence (the PR's own CI — per policy this review does not run PR code)

The ubuntu unit suite and the web-shell visual capture are still running as of this review; the windows/macos unit jobs and the CLI integration job were skipped by workflow conditions (not failures). The Desktop Shell jobs on both platforms passed — those compile the whole monorepo including this diff, which is a real build signal. No red checks on this commit at review time.

Final CI results for e2c77fd (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Test (ubuntu-latest, Node 22.x) ❌ failure
Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ success
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Secret scan (TruffleHog) ✅ 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,失败项排在最前。

The remaining checks on the commit are bot orchestration (review-pr, label, authorize, …). Not verified: live TUI/Web Shell/ACP rendering of the dialog — the author's tmux capture below is their evidence, not an independent re-run:

Sandboxed verification would settle this: @qwen-code /tmux — that the approval dialog actually renders with the full objective under YOLO and that approving it starts the first Goal turn currently rests on the author's tmux capture (Linux only); the unit suite pins the gating logic but not host rendering. (@qwen-code /verify would settle the A/B behavioural side.) The author has write access, so a maintainer trigger is all it takes.

中文说明

代码审查

方案与我独立给出的设想一致——在 get_goal / update_goal 旁新增工具、复用现有 info 确认(三端都不需要新 UI)、按宿主能力和退出设置门控注册、同意只来自 user 作用域、skill 交接保留文本回退。我对关键断言逐一对照了基线代码:

  • 对话框无法被跳过。 requiresUserInteraction() 是既有机制(exit_plan_mode 在用),我核对了调度器里所有旁路:设置后 permissionFlow 会把 allow 规则强制为 ask;needsConfirmation 在 YOLO 豁免之前返回 true;AUTO 三层过滤、AUTO_EDIT 对 info 的自动批准、PermissionRequest hook 的 allow 分支都有 !requiresUserInteraction 守卫(hook 仍可拒绝);调度器同时设置 hideAlwaysAllow,与证据里只有 "Yes, allow once" / "No" 一致。deny 规则仍然有效。
  • 注册门控是结构性的。 注册位于 registerGoalWorkerTools 内(子 agent 上下文提前返回);新增条件检查 resolveInteractionMode !== 'headless' 与设置值。headless、子 agent、禁用设置都拿不到该工具;注册矩阵测试已固定。
  • TOCTOU 处理是真实的。 前置条件在弹窗前与 execute() 中各检查一次;replace 携带 expectedGoalId / expectedRevision,reducer 的版本断言把并发的 /goal 变更变成友好拒绝而非覆盖;未获批准时保持失败关闭。
  • 作用域限制走既有通用机制。 WORKSPACE_RESTRICTED_SETTINGS 剥离 workspace 值并告警;Settings 类型由 schema 推导,新增 goals 节类型全链路通过;CLI 规范化函数把枚举封闭后再传给 core。与 agents.crossSession* 同构。
  • renderPromptAsPlainTextinfo 确认都是既有面(Workflow 工具两者都用),宿主无需渲染任何新东西。

一个非阻塞小项: 两处既有的 goal 工具枚举没有加入 propose_goal——chat recording 的 goal_runtime 来源标记(coreToolScheduler.ts ~6294、useGeminiStream.ts ~3936/~4878)与工具使用摘要的排除列表(useGeminiStream.ts ~5052)。实际影响很小:该工具在 Goal 活跃时拒绝,基本不会携带 goalContext;排除列表只影响快速模型摘要可能与工具自身的 "Goal set · …" 展示并存。建议顺手补齐,不构成合并阻塞。

测试证据(引自 PR 自身的 CI——按策略本审查不运行 PR 代码)

截至审查时 ubuntu 单测与 web-shell 视觉截图仍在运行;windows/macos 单测与 CLI 集成测试被工作流条件跳过(不是失败)。两平台的 Desktop Shell 任务已通过——它们连同本 diff 编译整个 monorepo,是真实的构建信号。审查时该提交无红色检查。上方表格区域会由 finalize 流程在 CI 落定后就地更新。

未验证:TUI/Web Shell/ACP 对话框的实际渲染——作者在 Linux 上的 tmux 记录是其自述证据,不是独立复跑。维护者可触发 @qwen-code /tmux(TUI 面)或 @qwen-code /verify(A/B 行为面)来闭环;作者有 write 权限,维护者直接触发即可。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, minimal, direction pre-approved in the merged design doc; the only open items are the non-blocking enumeration nit and CI still in flight.

Stepping back: this is exactly the follow-up phase the goal-draft design note promised, and it lands the hard parts right. My independent proposal for this feature and the PR's implementation converge on the same shape, and where the PR goes further — the versioned replace, the double precondition check across the dialog window, the closed-enum normalizer, the workspace-scope lockdown — each addition earns its place. Nothing in the diff is unrelated, nothing feels over-engineered, and the security model (consent for starting an autonomous loop must come from a human at a dialog that no rule or mode can skip) verifies cleanly against the scheduler code, not just against the PR's description. The test suite pins the behaviors that matter rather than the implementation details.

Reservations, plainly stated: the cross-host rendering of the dialog (TUI buttons, Web Shell description-only approval, ACP) is covered by unit tests and the author's tmux session but not independently exercised here, and the ubuntu unit suite had not finished when this review ran. Neither is a reason to hold the PR — they are what the green-CI precondition and the sandboxed lanes above are for.

Verdict: approve, with approval deferred until CI lands green on e2c77fdae97ee70cc33ad5bd2b9ecd3f4388044b (the ubuntu unit suite and web-shell visuals are still in flight). If anything lands red, the finalize step flags it instead of approving.

中文说明

置信度:4/5 —— 干净、最小化,方向在已合并的设计文档中已预先确认;唯一未决项是上面那个非阻塞枚举小项,以及 CI 仍在运行。

退一步看:这正是 goal-draft 设计说明承诺的后续阶段,而且难点都做对了。我独立给出的方案与 PR 的实现收敛到同一形状;PR 更进一步的地方——带版本的 replace、跨对话框窗口的双重前置条件检查、封闭枚举的规范化、workspace 作用域锁定——每一处都物有所值。diff 中没有无关改动,没有过度设计;安全模型(启动无人值守循环的同意必须来自对话框里的真人,任何规则或模式都不能跳过)在调度器代码层面得到验证,而不只是 PR 描述里的说法。测试套件固定的是行为而非实现细节。

坦率地说保留意见:对话框的跨宿主渲染(TUI 按钮、Web Shell 仅描述批准、ACP)由单测和作者的 tmux 记录覆盖,但这里没有独立复跑;审查时 ubuntu 单测尚未结束。这两点都不是扣住 PR 的理由——绿色 CI 前置条件和上面的沙箱通道正是为此而设。

结论:批准,但批准延迟到 CI 在 e2c77fdae97ee70cc33ad5bd2b9ecd3f4388044b 上全绿后生效(ubuntu 单测与 web-shell 视觉仍在运行)。若有红色结果,finalize 环节会标记而不是批准。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head 10c6fab. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

⚠️ No preview: one or more scenarios failed to render on this head — see the workflow run. This is not "no visual change" — a scenario that times out or throws produces no image. Fix the failing scenario (or a genuine regression it caught) and the preview returns on the next push.

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

@qqqys

qqqys commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator Author

已修复 ProposeGoal 的 CLI 工具名翻译覆盖,补齐 en、zh、zh-TW。验证:CLI i18n 31/31、全仓 typecheck、完整 build/bundle 均通过。

qqqys added 2 commits August 27, 2026 11:32
… turn ends

The first end-to-end run set the Goal from inside propose_goal's execute().
That works for the Goal but not for the turn that proposed it: with a Goal
now active, the tool-result continuation of that turn has no Goal permit
and client.ts rejects it with "An active Goal requires an exact turn
permit", which surfaces as an error card before the runtime's own Goal
turn takes over.

The tool now parks the approval on Config (setPendingGoalProposal) and the
client applies it at the boundary a typed /goal set takes effect: once the
model has stopped calling tools, before Stop hooks (settlePendingGoalProposal
-> applyPendingGoalProposal). The runtime's broadcast then renders the Goal
card and starts the first Goal turn right after the model's acknowledgement.
An approval still parked when the next real user query starts belongs to a
cancelled turn and is discarded; a Goal that became active meanwhile is never
replaced, and a moved expected version is reported as a conflict instead of
thrown.

Claude-Session: https://claude.ai/code/session_01FV7i3w7egJ2kMw4AhQC38Z
qqqys added a commit to qqqys/qwen-code that referenced this pull request Aug 27, 2026

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — the 'Integration Tests (CLI, No Sandbox)' check was skipped in CI and its suite did not run locally.

Not explored to full depth (tool budget reached): "agent reverse-audit (round 5)": none — the walk completed inside budget; no check was cut short..

中文说明

仅完成部分审查,审查缺口已披露。

未审查:build-and-test — the 'Integration Tests (CLI, No Sandbox)' check was skipped in CI and its suite did not run locally。

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 5)"none — the walk completed inside budget; no check was cut short.

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

Comment thread packages/core/src/core/client.ts Outdated
Comment on lines +3906 to +3910
await this.settlePendingGoalProposal(
turn.pendingToolCalls.length === 0,
signal,
loadGoalRuntime,
);

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.

[Critical] R1-1: settlePendingGoalProposal applies the parked propose_goal approval at the end of the stream loop, but before the same outer turn's tail continuations run: the queued steer-input drain (~3922/3927), the blocking user Stop-hook continuation (~4115/4249), and the next-speaker recursion (~4379, when skipNextSpeakerCheck is false). The proposing turn was admitted before any Goal existed, so it never acquired a goalPermit (the rebuild at ~3117 only runs if (goalPermit)), and those recursions re-enter sendMessageStream with goalPermit === undefined. With the Goal now active, admission reaches the goalRequiresPermit branch (~3114) and throws.

Concretely: the user approves the dialog, the model acknowledges without tool calls, settle dispatches create mid-turn — then a steer message queued during the turn (Ctrl+Q), or a user-configured blocking Stop hook, or a next-speaker model turn, recurses permit-less, and the turn the user just approved ends with ✕ [API Error: An active Goal requires an exact turn permit]; the steer message is restored to the queue unanswered and the Stop-hook continuation is dropped. Pre-PR no path could make a Goal active after turn admission, so this diff makes the state newly reachable. The settle docstring's own rationale ("creating the Goal earlier would leave those continuations without a permit") argues for tool-result continuations only — it does not argue for the steer/hook/next-speaker continuations it now precedes.

Witness (probe on the unmodified commit, fix flipped in a scratch tree):

A-steer:   error="An active Goal requires an exact turn permit", dispatch=1x create, steerAcceptCalls=0, modelTurnRuns=1
B-stophook: error="An active Goal requires an exact turn permit", dispatch=1x create
with settle moved past the steer block: A-steer error=undefined, modelTurnRuns=2, dispatch still exactly 1x

Suggested fix: apply the proposal only when the outer turn truly ends — move the settle call past the steer block and the Stop-hook continuation branches (to the points that return turn with no continuation pending), or skip applying when a steer input / blocking Stop hook continuation will follow, leaving the proposal parked for the continuation's own boundary.

Fix witness: a client-goal.test.ts variant of 'sets an approved propose_goal proposal once the turn ends without tool calls' with a queued steer input — the stream must not throw the permit error, the continuation turn must run, and runtime.dispatch must still be called exactly once; removing the relocation must turn this test red.

中文说明

settlePendingGoalProposal 在流循环结束时应用暂存的 propose_goal 批准,但位置在同一个外层 turn 的尾部延续之前:排队 steer 输入的消费(~3922/3927)、阻塞型用户 Stop hook 的延续(~4115/4249)、以及 next-speaker 递归(~4379,当 skipNextSpeakerCheck 为 false 时)。提议 turn 进入时还没有任何 Goal,因此从未获取 goalPermit(~3117 处的 options 重建只在 if (goalPermit) 时执行),这些递归以 goalPermit === undefined 重新进入 sendMessageStream;此时 Goal 已激活,准入走到 goalRequiresPermit 分支(~3114)并抛错。具体场景:用户批准对话框、模型不再调用工具、settle 在 turn 中途 dispatch create——随后 turn 中排队的 steer 消息(Ctrl+Q)、用户配置的阻塞 Stop hook、或 next-speaker 模型 turn 以无 permit 状态递归,用户刚批准的 turn 以 ✕ [API Error: An active Goal requires an exact turn permit] 结束;steer 消息被放回队列无人应答,Stop hook 延续被丢弃。本 PR 之前没有任何路径能在 turn 准入之后激活 Goal,因此这个状态是本 diff 新引入的。settle 注释里"更早创建 Goal 会让那些延续没有 permit"的理由只对 tool-result 延续成立,不适用于它现在排在前面的 steer/hook/next-speaker 延续。

建议修复:只在外层 turn 真正结束时应用提议——把 settle 调用移到 steer 块与 Stop hook 延续分支之后(即那些不带任何待处理延续、直接 return turn 的点),或在即将出现 steer/阻塞 Stop hook 延续时跳过应用,把提议继续暂存、留给延续自己的 turn 边界。

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

已在 10c6fab8e6 修复:proposal 现在会保持暂存,直到 queued steer、Stop hook 和 next-speaker 尾续跑全部结束后才 settle。回归覆盖 queued steer;client-goal.test.ts 28/28 通过。

Comment on lines +3085 to +3087
// turn the user cancelled; starting a loop from it now would
// surprise them.
this.config.takePendingGoalProposal?.();

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.

[Critical] R1-2: The parked approval is discarded only when the next message is a UserQuery. A proposing turn that exits through an error path skips both settlePendingGoalProposal (called only at ~3906, after the stream loop) and this discard — the GeminiEventType.Error early return (~3885), the LoopDetected returns (~3769/3801), max-turns (~3347), session-token-limit (~3420) and arena-control-signal (~3468) exits all leave the approval parked. With the session idle, a later automated turn — SendMessageType.Cron, Notification, or Teammate (when agents.crossSessionMessaging is on) — ends cleanly, and settle runs unconditionally for every message type: the stale approval is applied and the autonomous Goal loop starts underneath a turn the user never initiated, contradicting this comment's premise that anything still parked "belongs to a turn the user cancelled". If the user's next query arrives first instead, the approval is silently discarded and the Goal they approved never materializes.

Witness (probe on the unmodified commit, fix flipped in a scratch tree):

turn 1 (UserQuery) driven through the Error early return; turn 2 (Notification) ends cleanly:
take calls after error turn: 1   <- only the entry discard; settle skipped on turn 1
dispatch calls final: [[{"action":"create","objective":"stale-approval"}]]   <- stale approval applied under the automated turn
with takePendingGoalProposal() added to the Error early-return path: dispatch calls [] 

Suggested fix: settle or drop the parked proposal on every exit path of the proposing turn — e.g. call this.config.takePendingGoalProposal?.() in the GeminiEventType.Error early-return path next to finalizeInterruptedGoalTurn() — or apply the discard at every turn start rather than only UserQuery.

Fix witness: a client-goal.test.ts case — park a proposal, drive the proposing turn through an error exit, then run a SendMessageType.Notification turn (the helper already exercises this type) and assert runtime.dispatch was NOT called with the parked objective; removing the new guard must turn it red.

中文说明

暂存的批准只在下一条消息是 UserQuery 时被丢弃。提议 turn 若从错误路径退出,则既不会走到 settlePendingGoalProposal(只在 ~3906、流循环之后调用),也不会走到这里的丢弃——API 错误的提前 return(~3885)、LoopDetected(~3769/3801)、max-turns(~3347)、session-token-limit(~3420)、arena-control-signal(~3468)都会让批准继续暂存。会话空闲时,一个稍后的自动 turn(CronNotificationTeammate——当 agents.crossSessionMessaging 开启时)正常结束,而 settle 对每种消息类型都无条件执行:过期批准被应用,自主 Goal 循环在用户从未发起的 turn 下启动,与本注释"仍暂存的批准属于用户取消的 turn"的前提矛盾。若用户的下一条查询先到,批准则被静默丢弃,用户批准过的 Goal 永远不会生效。

建议修复:在提议 turn 的每条退出路径上 settle 或丢弃暂存批准——例如在 GeminiEventType.Error 的提前 return 处、finalizeInterruptedGoalTurn() 旁调用 this.config.takePendingGoalProposal?.();或在每种 turn 开始时都执行丢弃,而不只是 UserQuery

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

已在 10c6fab8e6 修复:任何异常退出都会消费并丢弃该 turn 暂存的 proposal,后续 Notification/Cron 不会误启动旧 Goal。provider error 回归通过;client-goal.test.ts 28/28。

Comment on lines +8942 to +8945
if (
this.getModelProposedGoals() !== 'disabled' &&
resolveInteractionMode(this) !== 'headless'
) {

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.

[Critical] R1-3: propose_goal is registered whenever resolveInteractionMode(this) !== 'headless', which includes acp — Zed's --acp and stream-json daemon sessions bridged through acp-bridge both resolve to it, and the new registration it.each even asserts the 'ACP' case. But the only code that ever applies or discards the parked approval (settlePendingGoalProposal / takePendingGoalProposal) lives in GeminiClient.sendMessageStream, which the ACP surface deliberately bypasses — Session.ts drives chat.sendMessageStream (Session.ts:7286) with the file's own comment: "The ACP surface consumes GeminiChat's raw stream directly rather than going through GeminiClient.sendMessageStream". Grep over packages/cli/src/acp-integration/** for the settle/take machinery returns zero matches.

In ACP sessions an approved proposal is therefore parked forever: the model proposes, Session.ts's permission flow honors requiresUserInteraction and relays the dialog, the user approves, execute() parks the proposal and tells the model "It is set the moment this turn ends" — the turn ends inside Session.ts's own loop, and nothing ever applies or discards the approval. No Goal card, no Goal turns, no error, while the model's acknowledgement tells the user it was set. The user consented to an autonomous loop that silently never starts. (Goals otherwise work in ACP — Session.ts passes goalPermit and uses getGoalRuntimeReady — so this is a gap specific to the parked-proposal boundary.)

Witness (run in the verifier's scratch tree): the PR's own registration test — vitest run src/config/config.test.ts -t "registers propose_goal beside the Goal worker tools"Tests 3 passed | 581 skipped (interactive, ACP, stream-json all register the tool) — combined with the zero-match grep over acp-integration.

Suggested fix: either tighten the gate to the surface that actually settles — resolveInteractionMode(this) === 'interactive' (excluding 'acp'), flipping the 'ACP' case of the new registration test — or mirror the turn-boundary settle/discard into the ACP Session loop (#sendMessageStreamWithAutoCompression turn end and new-user-prompt entry).

Fix witness: for exclusion, the inverted 'ACP' registration case pins the gate; for ACP settle, a Session-level test asserting a parked proposal is applied (dispatch create) when the ACP turn ends — removing the settle call must fail it.

中文说明

propose_goalresolveInteractionMode(this) !== 'headless' 时注册,这包含 acp——Zed 的 --acp 与经 acp-bridge 桥接的 stream-json 守护会话都解析为该模式,新的注册 it.each 甚至断言了 'ACP' 分支。但唯一应用或丢弃暂存批准的代码(settlePendingGoalProposal / takePendingGoalProposal)位于 GeminiClient.sendMessageStream,而 ACP 面刻意绕过了它——Session.ts 直接驱动 chat.sendMessageStream(Session.ts:7286),文件自己的注释写明"ACP 面直接消费 GeminiChat 的原始流,不经过 GeminiClient.sendMessageStream"。对 packages/cli/src/acp-integration/** grep settle/take 机制:零匹配。

因此在 ACP 会话里,被批准的提议会永远暂存:模型提议、Session.ts 的权限流尊重 requiresUserInteraction 并转发对话框、用户批准、execute() 暂存提议并告诉模型"It is set the moment this turn ends"——turn 在 Session.ts 自己的循环里结束,没有任何代码应用或丢弃这个批准。没有 Goal 卡片、没有 Goal turn、没有错误,而模型的确认语告诉用户 Goal 已设定。用户同意启动的自主循环静默地从未开始。(ACP 里 Goal 其余功能是正常的——Session.ts 传 goalPermit、用 getGoalRuntimeReady——所以这是暂存提议边界特有的缺口。)

建议修复:把门控收紧到真正会 settle 的面——resolveInteractionMode(this) === 'interactive'(排除 'acp'),并把新注册测试的 'ACP' 分支反转;或在 ACP Session 循环里(#sendMessageStreamWithAutoCompression 的 turn 结束与新用户输入入口)镜像同样的 turn 边界 settle/丢弃。

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

已在 10c6fab8e6 修复:在 ACP 与 stream-json 尚无等价 turn-boundary settle 前,不再注册 propose_goal;interactive 保持可用。config.test.ts 584/584 通过。

Comment on lines +648 to +650
const objective = proposal.objective.trim();
const current = runtime.getSnapshot().goal;
if (current?.status === 'active') {

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.

[Critical] R1-4: applyPendingGoalProposal's active-Goal check runs outside the runtime's serialized dispatch queue, and resume activates a Goal without bumping goalId/revision (the reducer's resume paths never bump revision — only edit does). So a concurrently queued resume lets this proposal's replace pass assertExpectedVersion and swap the Goal that is active by the time the reducer applies it — contradicting the invariant in this function's docstring ("an active Goal is never replaced") and silently destroying a Goal the user explicitly resumed.

Failure scenario: a Goal is paused (goalId X, revision R); the user approves a propose_goal dialog during a turn; near the turn's end the user resumes the paused Goal (/goal resume or the web-shell goal controls). That dispatch is in the runtime queue with its journal write still in flight, so getSnapshot() here still reports paused; replace {expectedGoalId: X, expectedRevision: R} is enqueued after it. The resume commits (status→active, same goalId/revision) and queueContinuation() starts driving the resumed Goal; then the replace executes, passes the version assertion, and swaps the now-active Goal for the proposal — invalidating the permit and preempting the host mid-flight. The existing tests only pin the ordering-safe variants ("became active" visible at snapshot-read time; stale-revision conflict, which resume does not produce).

Witness (probe on the unmodified commit, fix flipped in a scratch tree):

W4-A resume result goal: {"goalId":"302f57df-...","revision":1,"status":"active"}   <- resume committed, same goalId/revision
W4-A applied: {"applied":true, ...,"replacedGoalId":"302f57df-..."}
W4-A final goal: {"goalId":"75478b48-...","status":"active","objective":"Proposed by model"}   <- resumed Goal destroyed
flip (resume bumps revision): applied {"applied":false,"reason":"Goal version does not match the current session Goal"}, final goal stays "Resumed by user"

Production frequency is a race window — but a window equal to every resume's journal write, in a state this feature explicitly supports (the dialog offers "Replace the paused Goal" while a Goal is paused, and resume can be dispatched mid-turn).

Suggested fix: move the guard inside the queue — extend the replace request with a refuseIfActive-style flag (or a dedicated "apply proposal" operation) that reduceGoalControl/dispatch checks at execution time; the snapshot pre-check alone cannot cover the queue window.

Fix witness: a variant of 'does not set a parked approval over a Goal that became active meanwhile' using the existing stale-wrapper pattern — a stub whose getSnapshot() reports the paused goal while dispatch runs against a runtime whose goal is active with the same goalId/revision; today applied.applied is true, with the fix it must be false.

中文说明

applyPendingGoalProposal 的活跃 Goal 检查运行在 runtime 串行 dispatch 队列之外,而 resume 激活 Goal 时不改变 goalId/revision(reducer 的 resume 路径从不递增 revision——只有 edit 会)。因此一个并发排队的 resume 能让本提议的 replace 通过 assertExpectedVersion,在 reducer 真正执行时换掉已经处于活跃状态的 Goal——与本函数文档中"活跃 Goal 永不被替换"的不变量矛盾,并静默销毁用户明确恢复的 Goal。

失败场景:Goal 处于 paused(goalId X,revision R);用户在某个 turn 中批准 propose_goal 对话框;临近 turn 结束时用户恢复该 paused Goal(/goal resume 或 web-shell 的 Goal 控件)。该 dispatch 已入队、日志写入尚未落盘,getSnapshot() 此时仍报告 pausedreplace {expectedGoalId: X, expectedRevision: R} 排在其后。resume 先提交(status→active,goalId/revision 不变),queueContinuation() 开始驱动恢复的 Goal;随后 replace 执行、通过版本断言、把此刻活跃的 Goal 换成提议——使 permit 失效并在运行中抢占宿主。现有测试只固定了顺序安全的变体(快照读取时已活跃;以及 resume 不会产生的旧版本冲突)。

建议修复:把守卫移进队列内——给 replace 请求扩展一个 refuseIfActive 语义的标志(或专门的"应用提议"操作),由 reduceGoalControl/dispatch 在执行时检查;快照预检查无法覆盖队列窗口。

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

已在 10c6fab8e6 修复:proposal replace 的 active 检查移入 GoalRuntime.dispatch 串行队列,queued resume 先落地时 replace 会拒绝。真实 runtime 竞争回归通过;goal-tools.test.ts 51/51。

Comment thread packages/core/src/core/client.ts Outdated
Comment on lines +3906 to +3910
await this.settlePendingGoalProposal(
turn.pendingToolCalls.length === 0,
signal,
loadGoalRuntime,
);

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.

[Critical] R1-5: Settle's abort decision is check-then-act: settlePendingGoalProposal tests signal.aborted once at entry (833-837), but the two awaits that follow — loadGoalRuntime(false) and applyPendingGoalProposal — never re-check it, so an abort landing mid-settle still applies the approval. The autonomous Goal loop then starts immediately after the user cancelled, contradicting the method's own documented semantics ("An aborted turn drops the approval instead of starting a loop the user just cancelled").

The windows are real I/O, not just microtasks: getGoalRuntimeReady waits on the session-writer lease restore (config.ts:3484) — plausibly incomplete on the early turns where /goal-draft typically proposes — and dispatch serializes through the queue and awaits the journal write. On create, dispatch sets the Goal active and calls queueContinuation()host.startGoalTurn(...), which takes no AbortSignal, so the spawned Goal turn does not inherit the aborted signal. The loop's signal.aborted branch then runs finalizeInterruptedGoalTurn, a no-op here because the proposing turn holds no permit for the just-created Goal; the TUI's Escape drain of queued Goal turns runs before settle applies, so the turn enqueued afterwards is submitted and the loop runs.

Witness (probe on the unmodified commit, fix flipped in a scratch tree): abort fired deterministically inside settle's readiness await, after the entry check (readyCalls === 2):

PR code: AssertionError: expected "spy" to not be called at all, but actually been called 1 times
         1st spy call: [{"action":"create","objective":"ship it"}]
with re-check after loadGoalRuntime: dispatch never called; the existing 24 client-goal tests still pass

Suggested fix: re-check signal.aborted immediately before dispatching (after loadGoalRuntime resolves) and drop the taken proposal if it fired; for full closure, if applyPendingGoalProposal returned applied: true but signal.aborted became true during apply, roll the Goal back (dispatch pause/clear with the applied goal's id/revision) and debug-log the discard — applyPendingGoalProposal's own await dispatch is a second window the entry re-check alone does not close.

Fix witness: a client-goal.test.ts case beside 'sets an approved propose_goal proposal once the turn ends without tool calls' — the mocked readiness loader aborts the controller while settle is in flight; assert runtime.dispatch was never called (or an applied Goal is rolled back); removing the re-check must turn it red.

中文说明

settle 的中止判断是"先检查后执行":settlePendingGoalProposal 只在入口检查一次 signal.aborted(833-837),其后的两个 await——loadGoalRuntime(false)applyPendingGoalProposal——都不再检查,因此落在 settle 中部的中止仍会让批准被应用:用户刚取消,自主 Goal 循环随即启动,与方法自身文档语义("被中止的 turn 丢弃批准,而不是启动用户刚取消的循环")矛盾。

窗口是真实 I/O 而非微任务:getGoalRuntimeReady 要等会话 writer 租约的恢复(config.ts:3484)——在 /goal-draft 通常提议的前几个 turn 里很可能尚未完成——dispatch 则串行入队并等待日志写入。create 时 dispatch 使 Goal 激活并调用 queueContinuation()host.startGoalTurn(...),后者不接受 AbortSignal,派生的 Goal turn 不继承已中止的信号。循环随后走 signal.aborted 分支执行 finalizeInterruptedGoalTurn,但此处是空操作(提议 turn 没有刚创建 Goal 的 permit);TUI 对排队 Goal turn 的 Escape 清空发生在 settle 应用之前,之后入队的 turn 会被提交,循环照常运行。

建议修复:在 dispatch 之前(loadGoalRuntime 解析后)立即重查 signal.aborted,若已中止则丢弃已取出的提议;若要完全闭环,当 applyPendingGoalProposal 返回 applied: true 而 apply 期间 signal.aborted 变真时,回滚该 Goal(以应用后的 goal id/revision dispatch pause/clear)并写 debug 日志——applyPendingGoalProposal 自身的 await dispatch 是入口重查无法覆盖的第二个窗口。

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

已在 10c6fab8e6 修复:runtime readiness 后再次检查取消;若取消落在 apply 期间,则立即暂停刚应用的 Goal。两个确定性取消窗口回归通过;client-goal.test.ts 28/28。

Comment on lines +2378 to +2380
modelProposedGoals: normalizeModelProposedGoals(
settings.goals?.modelProposed,
),

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.

[Suggestion] R1-11 (pattern: five new propose_goal lifecycle behaviors ship without any test crossing them — this is location 1 of 5): no test crosses this loadCliConfig wiring line. Delete or mis-key it and everything stays green — normalizeModelProposedGoals unit tests call the function directly, settings.test.ts tests loadSettings (not loadCliConfig), the core registration tests pass modelProposedGoals straight into new Config — and the core default 'alwaysAsk' applies, so a user who set goals.modelProposed: "disabled" keeps getting the propose_goal tool, with every test green. This is not hypothetical: the neighbouring loadCliConfig workflowsEnabled test carries a comment documenting exactly this dead-switch regression ("loadCliConfig never wrote it — so the setting was a dead switch"), yet this wiring skips that established pattern.

Suggested fix: add a loadCliConfig-level test beside the workflowsEnabled one — settings = { goals: { modelProposed: 'disabled' } }expect(config.getModelProposedGoals()).toBe('disabled') (and default → 'alwaysAsk').

Fix witness: the new test itself goes red under "delete the modelProposedGoals: line from loadCliConfig", which today ships green.

中文说明

R1-11(模式:五项新的 propose_goal 生命周期行为都没有任何测试穿过——这是 5 处中的第 1 处):没有测试穿过这行 loadCliConfig 接线。删除或写错键名,所有测试仍然是绿的——normalizeModelProposedGoals 单测直接调用函数,settings.test.ts 测的是 loadSettings(不是 loadCliConfig),core 注册测试直接把 modelProposedGoals 传给 new Config——于是 core 默认 'alwaysAsk' 生效,设置了 goals.modelProposed: "disabled" 的用户仍然会拿到 propose_goal 工具,且所有测试全绿。这不是假设:相邻的 loadCliConfig workflowsEnabled 测试的注释就记录了这个死开关回归("loadCliConfig never wrote it — so the setting was a dead switch"),而本 PR 的接线没有沿用这个既有模式。

建议修复:在 workflowsEnabled 旁补一个 loadCliConfig 层测试——settings = { goals: { modelProposed: 'disabled' } }expect(config.getModelProposedGoals()).toBe('disabled')(以及默认 → 'alwaysAsk')。

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

Comment on lines +833 to +836
if (signal.aborted) {
take.call(this.config);
return;
}

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.

[Suggestion] R1-11 (pattern: five new propose_goal lifecycle behaviors ship without any test crossing them — this is location 2 of 5): the signal.aborted discard branch — the only thing preventing an approved proposal from being applied to a turn the user cancelled (the call site at ~3906 is unconditional; the loop branch signal.aborted ? finalizeInterruptedGoalTurn() proves the path runs) — has no test; both new client-goal.test.ts tests use never-aborted signals. Remove this branch and no existing test goes red, so the regression — an autonomous Goal loop starting after the user pressed Escape on the proposing turn — would ship silently.

Suggested fix: add a client-goal.test.ts case — park a proposal, abort the AbortController before draining, assert runtime.dispatch is never called and the proposal is consumed (a subsequent takePendingGoalProposal returns undefined).

Fix witness: the new test goes red under "delete the signal.aborted branch" (settle would then take and apply the proposal on an aborted turn).

中文说明

R1-11(模式:五项新的 propose_goal 生命周期行为都没有任何测试穿过——这是 5 处中的第 2 处):signal.aborted 丢弃分支——防止已批准提议被应用到用户已取消的 turn 的唯一防线(~3906 的调用点无条件执行;循环里 signal.aborted ? finalizeInterruptedGoalTurn() 分支证明该路径会走到)——没有测试;两个新的 client-goal.test.ts 测试都使用未中止的信号。删除该分支,现有测试无一变红,因此"用户在提议 turn 按 Escape 后自主 Goal 循环仍启动"的回归会静默上线。

建议修复:补一个 client-goal.test.ts 用例——暂存提议,在 drain 前中止 AbortController,断言 runtime.dispatch 未被调用且提议已被消费(随后 takePendingGoalProposal 返回 undefined)。

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

Comment on lines +837 to +838
if (!turnEnded) return;
const proposal = take.call(this.config);

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.

[Suggestion] R1-11 (pattern: five new propose_goal lifecycle behaviors ship without any test crossing them — this is location 3 of 5): the if (!turnEnded) return; park gate — which keeps the proposal parked while the turn continues calling tools — is untested; both client tests use MockTurn defaults with pendingToolCalls: [] and emptyStream responses, so turnEnded is always true. Remove the gate and a turn that keeps calling tools after approval creates the Goal mid-tool-turn — the rest of the turn runs Goal-active without a permit and admission rejects the permit-less continuation — with no test going red.

Suggested fix: add a client test that mocks a turn response with non-empty pendingToolCalls after the proposal is parked and asserts runtime.dispatch is not called and the proposal stays parked for a later tool-free boundary.

Fix witness: the new test goes red under "delete the !turnEnded early return".

中文说明

R1-11(模式:五项新的 propose_goal 生命周期行为都没有任何测试穿过——这是 5 处中的第 3 处):if (!turnEnded) return; 暂存门——让提议在 turn 继续调用工具期间保持暂存——没有测试;两个 client 测试都用 MockTurn 默认的 pendingToolCalls: []emptyStream 响应,turnEnded 恒为 true。删除该门,批准后继续调用工具的 turn 会在工具 turn 中途创建 Goal——turn 余下部分在无 permit 的情况下带着活跃 Goal 运行,准入会拒绝无 permit 的延续——且没有测试变红。

建议修复:补一个 client 测试:提议暂存后 mock 一个 pendingToolCalls 非空的 turn 响应,断言 runtime.dispatch 未被调用、提议仍暂存、等待之后无工具的边界。

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

Comment on lines +8286 to +8287
// An approval belongs to the session that produced it.
this.pendingGoalProposal = undefined;

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.

[Suggestion] R1-11 (pattern: five new propose_goal lifecycle behaviors ship without any test crossing them — this is location 4 of 5): the initializeGoalRuntime guard that clears a parked approval when the Goal runtime is rebuilt — "an approval belongs to the session that produced it" — is untested; Config.setPendingGoalProposal/takePendingGoalProposal are never exercised directly either (the client tests replace takePendingGoalProposal with a vi.fn via Object.assign). Mid-session re-entry callers exist (writer-lease activation at config.ts:3474, rebaseGoalRuntimeFromActiveTranscript at 8223). Remove this line and a stale approval survives the rebuild and is applied against the replaced runtime at the next turn boundary — a Goal consented to in one session state starting in another — with every test green.

Suggested fix: add a core config.test.ts case — park a proposal via setPendingGoalProposal, drive a path that calls initializeGoalRuntime again, then assert takePendingGoalProposal() returns undefined (also pins take-once semantics).

Fix witness: the new test goes red under "delete the clear in initializeGoalRuntime".

中文说明

R1-11(模式:五项新的 propose_goal 生命周期行为都没有任何测试穿过——这是 5 处中的第 4 处):initializeGoalRuntime 在 Goal runtime 重建时清空暂存批准的守卫——"批准属于产生它的会话"——没有测试;Config.setPendingGoalProposal/takePendingGoalProposal 本身也从未被直接测过(client 测试用 Object.assigntakePendingGoalProposal 换成 vi.fn)。会话中期的重入调用方是存在的(config.ts:3474 的 writer-lease 激活、8223 的 rebaseGoalRuntimeFromActiveTranscript)。删除这行后,过期批准会在重建后存活,并在下一个 turn 边界应用到被替换的 runtime 上——在一个会话状态里同意的 Goal 在另一个状态里启动——而所有测试全绿。

建议修复:补一个 core config.test.ts 用例——用 setPendingGoalProposal 暂存提议,驱动一条再次调用 initializeGoalRuntime 的路径,断言 takePendingGoalProposal() 返回 undefined(同时固定 take-once 语义)。

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

Comment on lines +809 to +810
const blocker = this.blocker();
if (blocker) return this.errorResult(blocker.message, blocker.type);

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.

[Suggestion] R1-11 (pattern: five new propose_goal lifecycle behaviors ship without any test crossing them — this is location 5 of 5): execute()'s post-dialog re-check of preconditions — the design's "checked before the dialog and again in execute(), because /goal can change the session while the dialog is open" — is untested; every refusal case in goal-tools.test.ts rejects from getConfirmationDetails, and no test approves a dialog and then makes the session ineligible before execute(). Without this re-check, approving a stale dialog after /goal set would park the proposal and return "Goal approved"; only at the turn boundary would applyPendingGoalProposal refuse ('became active') — a refusal settlePendingGoalProposal swallows into debugLogger.debug, leaving the model believing a Goal is set while none is. The apply-time backstop is tested; this earlier guard is not.

Suggested fix: add a goal-tools.test.ts case — approve the dialog, dispatch a create on the runtime to make a Goal active, then execute() and assert EXECUTION_DENIED, no parked proposal, no active Goal replaced.

Fix witness: the new test goes red under "delete the blocker re-check in execute".

中文说明

R1-11(模式:五项新的 propose_goal 生命周期行为都没有任何测试穿过——这是 5 处中的第 5 处):execute() 在对话框之后对前置条件的复查——设计里"弹窗前检查一次、execute() 里再检查一次,因为对话框打开期间 /goal 可能改变会话"——没有测试;goal-tools.test.ts 里所有拒绝用例都从 getConfirmationDetails 抛出,没有用例先批准对话框、再在 execute() 前让会话变为不合格。若没有这个复查,在 /goal set 之后批准过期对话框会暂存提议并返回"Goal approved";直到 turn 边界 applyPendingGoalProposal 才拒绝('became active')——而该拒绝被 settlePendingGoalProposal 吞进 debugLogger.debug,模型以为 Goal 已设定,实际没有。apply 时的兜底有测试;这个更早的守卫没有。

建议修复:补一个 goal-tools.test.ts 用例——批准对话框,然后在 runtime 上 dispatch create 使 Goal 激活,再 execute(),断言 EXECUTION_DENIED、无暂存提议、活跃 Goal 未被替换。

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

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.

2 participants