fix(goal): stop exporting the unreachable propose_goal decline message - #10787
Conversation
|
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks @qqqys! The change itself reads well, but I have to bounce it on the template before going deeper: the PR body is missing most of the required headings from the PR template. The opening prose covers what the PR does and why, and there is a reviewer test plan, but the body has no ## What this PR does, no ## Why it's needed, no ### How to verify / ### Evidence (Before & After) / ### Tested on under the test plan, no ## Risk & Scope, no ## Linked Issues (the Related: #10662 line is currently plain prose), and no Chinese <details> section. Your recently merged #10715 and #10683 both carry the full template — could you bring this one in line? Happy to pick it right back up once it is.
中文说明
感谢 @qqqys！改动本身看起来没有问题，但在深入审查前需要先过模板这一关：PR 正文缺少 PR 模板 中的大部分必需标题。开头的文字说明了做什么和为什么，也附有评审测试计划，但正文缺少 ## What this PR does、## Why it's needed、测试计划下的 ### How to verify / ### Evidence (Before & After) / ### Tested on，以及 ## Risk & Scope、## Linked Issues（目前 Related: #10662 只是正文中的一句话）和中文 <details> 部分。您最近合并的 #10715 和 #10683 都使用了完整模板——麻烦把本 PR 正文也调整成相同格式，补齐后会立即继续后续审查。
— Qwen Code · qwen3.8-max
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
PROPOSE_GOAL_NOT_APPROVED_MESSAGE is written as if the model reads it. It does not. A declined dialog resolves as ToolConfirmationOutcome.Cancel, and coreToolScheduler settles the call as cancelled without ever entering execute(); what the model receives is the scheduler's own "[Operation Cancelled] Reason: User did not allow tool call". The guard itself is worth keeping as a defence against a host that one day runs execute() after a cancelled confirmation, so a decline can never fall through to parking an approval. It is now module-private with a comment saying why, since nothing outside the module should assert on a string the model cannot receive. What actually stops the model from re-proposing is the tool description, which already says so. The test that pinned the message now pins the real path -- a cancelled dialog parks nothing -- and a second test keeps the defensive branch honest without depending on the exact wording. The documentation sentence that made the same wrong claim is corrected in the Goal docs sync PR, which already rewrites that paragraph.
3ac3b8d to
ccb67fa
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline.
Not explored to full depth (tool budget reached): "agent 6c": I did not run packages/core tests — packages/core/dist/ is absent in this worktree, so vitest's globalSetup guard would stop the run without a full npm r….
Test Plan (not a blocker): 57 tests pass — this review observed 23116, 1912, 298, 1755, 504, 5659, 94 passed.
中文说明
已审查。 建议见行内评论。
未探索到全部深度（达到工具调用预算）："agent 6c"：I did not run packages/core tests — packages/core/dist/ is absent in this worktree, so vitest's globalSetup guard would stop the run without a full npm r…。
Test Plan（非阻断）：57 tests pass — this review observed 23116, 1912, 298, 1755, 504, 5659, 94 passed。
— qwen3.8-max-2026-09-02 via Qwen Code /review (v0.22.3)
Three gaps this PR left in what holds the decline behavior. The refusal assertion matched only the shared 'The Goal was not set' prefix, which PROPOSE_GOAL_NO_TURN_MESSAGE also begins with, so the two refusal branches were indistinguishable: returning the no-turn message from the not-approved guard kept the suite green while handing the model a string that invites the re-proposal the decline forbids. Match the 'the user did not approve it' fragment instead, which occurs only in the decline message. The tool description's decline clause was asserted nowhere, and dropping the exported constant made that description the only thing left telling the model not to re-propose. Pin the clause next to the existing name and permission checks, reusing the fragment the bundled goal-draft skill test already pins so the two copies cannot drift apart silently. Drop 'parks nothing when the dialog is cancelled': it never called execute(), and its assertions were a strict subset of the test below it, which runs the same cancelled confirmation and then executes. Its comment described the scheduler path it did not exercise; that rationale now sits on the surviving test, pointing at the scheduler test that does cover it.
Maintainer verification — built a real runtime environment for this PRI rebuilt the CLI at PR head Verdict: LGTM. The central claim holds on the wire, the change is provably behaviour-neutral at bundle level, and the replacement assertions have real bite. One stale paragraph in the PR body is worth fixing before merge. 1. What the model is actually handed on a decline
The approve control matters: it proves the capture can see 2. Unreachability, proven two independent ways
Sentinel mutation on the shipped bundle (not on a test double). I swapped
The positive control proves the patched chunk is what the process actually loads. The decline sentinel appears nowhere — the branch is unreachable at runtime, not merely unused. Every host and every
Speculation was the one call site that could have falsified the claim, since it never calls 3. Behaviour neutrality — bundle-level A/BI rebundled with the merge-base That single line — the constant leaving an export list — is the entire difference. The 4. Gates and mutation testing
Each mutation is caught by exactly one test:
M3 is the one that matters: the branch this PR keeps is pinned by the surviving test, so it will not rot into genuinely untested dead code. Findings — none blocking1. The Reviewer Test Plan is stale relative to commit 2. The rejected alternative is cheaper than the body implies. A per-call cancel-message channel already exists end to end: 3. Optional hardening. 4. Sequencing. #10785 is still open, so merging this alone leaves Observation, out of scope: screenshot 1 shows the objective rendered twice (invocation header + prompt body). That is item 2 of #10662 and correctly excluded here. Environment note
CI at time of writing
中文版维护者验证 —— 为本 PR 搭建了真实运行环境我在 PR head 结论：LGTM。 核心论断在链路上成立，改动在 bundle 层面可证明是行为中性的，替换后的断言也确实有检出力。合入前建议顺手修一下 PR 描述里过期的一段。 1. 拒绝时模型实际收到什么
批准对照组很关键：它证明这套采集能看见 2. 用两种独立方式证明不可达在已构建产物上做哨兵变异（不是改测试替身）。我把
正向对照证明进程真正加载的就是被我改过的 chunk。而拒绝哨兵在任何地方都没出现 —— 该分支在运行时不可达，不只是「没人用」。 所有 host 与所有
推测执行是唯一可能推翻该论断的调用点：它从不调用 3. 行为中性 —— bundle 级 A/B我用 merge-base 版本的 这一行 —— 常量从导出列表中消失 —— 就是全部差异。 4. 质量门与变异测试
每个变异都恰好被一个测试捕获：
M3 最关键：本 PR 保留的这个分支确实被留下的那个测试钉住了，不会腐化成真正无测试覆盖的死代码。 发现 —— 均不阻塞合入1. Reviewer Test Plan 相对 2. 被否掉的替代方案比描述里说的便宜。 按调用传递取消消息的通道其实已经端到端存在： 3. 可选加固。 在 4. 合入顺序。 #10785 仍处于 open，因此单独合入本 PR 会让 范围外观察： 截图 1 里目标文案渲染了两次（调用头部 + 提示正文）。这是 #10662 的第二项，本 PR 明确不处理，正确。 环境说明本机 撰写时的 CI 状态
|
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 50 passed · 0 failed · 50 total Flakiness gate: ✅ 1 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：50 通过 · 0 失败 · 50 总计 抖动门：✅ 1 changed test file(s) x 5 identical rounds, no divergence Verification reportPR 10787 verification —
|
| cell | oracle | head | base |
|---|---|---|---|
declined dialog (onConfirm(Cancel)) |
call status | cancelled ✔ |
cancelled ✔ |
execute() count at invocation seam |
0 ✔ | 0 ✔ | |
| model-visible response (exact) | [Operation Cancelled] Reason: User did not allow tool call ✔ |
identical ✔ | |
| parked proposal / goal snapshot | none / null ✔ | none / null ✔ | |
| decline-constant text in settled call | absent ✔ | absent ✔ | |
positive control (onConfirm(ProceedOnce)) |
execute() count |
1 ✔ | 1 ✔ |
| parked objective + approval payload | parked, approved:true JSON ✔ |
identical ✔ |
12/12 assertions per arm. The premise is a pre-existing fact (identical on base), exactly as the PR states; the PR changes no behavior, and the A/A confirms it changes none.
Secondary claims, scripted (logs/fragment-consumer-checks.log):
- Consumers at base: only
goal-tools.tsand its own test; at head: onlygoal-tools.ts(2 refs). Thegoals/index.tsbarrel never re-exportedgoal-tools, so no published surface changed. ✔ - The new fragment
'the user did not approve it'occurs exactly once ingoal-tools.tsand is absent fromPROPOSE_GOAL_NO_TURN_MESSAGE(the two refusal messages share only the bareThe Goal was not set:prefix — commit 2's discriminability premise holds). ✔ - The description clause
'do not propose the same or a reworded objective again'is present in the tool description and in the bundledgoal-draft/SKILL.md, pinned bySKILL.test.tsand now bygoal-tools.test.tswith the same literal. ✔
Mutation A/B: old vs new test file
harness/matrix.sh + harness/mutate.mjs; each cell runs the full goal-tools.test.ts file (56 tests) via vitest. Witness: evidence/02-mutation-matrix-base-vs-head-tests.png (C01–C10; C11–C12 in logs/cell-C11-*.log/C12).
| mutant | base prod × base tests | head prod × head tests |
|---|---|---|
| control (unmutated) | 🟢 56/56 | 🟢 56/56 |
| M1 decline branch returns the no-turn message | ❌ killed (1 fail) | ❌ killed (1 fail) |
| M2 guard deleted — decline falls through to parking the approval | ❌ killed (1 fail) | ❌ killed (1 fail) |
| M3 decline clause dropped from tool description | 🟢 survived 56/56 | ❌ killed (1 fail) |
| M4 pending-message value rewritten | 🟢 survived (vacuous, see F2) | 🟢 survived (same) |
M5 requiresUserInteraction → false (positive control) |
❌ killed | ❌ killed |
- No mutant regressed killed→survived; M3 flips survived→killed — the PR's new description-clause assertion is the only coverage for that clause, and it is what actually tells the model not to re-propose.
- Every kill fails at the intended assertion with named expected-vs-actual values, e.g. C04:
expected 'The Goal was not set: this call is no…' to contain 'the user did not approve it'; C06:expected "spy" to not be called at all(the M2 decline parked a proposal — the exact failure mode the guard exists to prevent); C08: the descriptiontoContainassertion. No kill is an import/compile artifact. - M5 proves the runner collects the mutated file on both arms; M4 is reported as vacuous-by-construction (F2), not as a coverage gap.
Reviewer Test Plan, step by step
npx vitest run src/goals/goal-tools.test.ts— runs green at head but with 56 tests, not 57, and the named decline test 'parks nothing when the dialog was cancelled' does not exist at head (the PR's own second commit deleted it; see F1). The surviving decline test 'refuses if a host runs it anyway after a cancelled dialog' does pin the defensive branch as described.npx tsc --noEmit— clean (exit 0); liveness proven (planted type error → TS2322/TS6133, exit 2).grep PROPOSE_GOAL_NOT_APPROVED_MESSAGE— only the two in-module references remain at head. ✔- Unreachability by reading — confirmed by reading
coreToolScheduler.ts(Cancel branch at ~3915 settlescancelled/not_started;createCancelledResponsebuilds the model-visible text) and behaviorally by the A/A harness above, plus the cited scheduler test'forwards the host denial reason when a bounced edit confirmation is cancelled'which exists and passes at head (1 passed | 384 skipped).
Findings
F1 (Suggestion, non-blocking) — PR body and Reviewer Test Plan describe the first commit, not the head. Both say "57 tests pass" and name a decline test 'parks nothing when the dialog was cancelled'; at the verified head the file has 56 tests and that test was removed by this PR's own second commit (whose message explains why: it never called execute() and its assertions were a strict subset of the surviving test). Repro: cd packages/core && npx vitest run src/goals/goal-tools.test.ts → Tests 56 passed (56); grep -c "parks nothing" src/goals/goal-tools.test.ts → 0. The code is right; the description a reviewer follows is stale. Suggested fix: update the body/plan to the head state (56 tests; one decline test plus the description-clause assertion).
F2 (informational) — toBe(CONSTANT) assertions cannot see a value mutation of that constant. M4 survived on both arms by construction: the tests compare result.llmContent to the imported constant, so mutating the constant's value moves both sides together. This is a pre-existing assertion style, not something the PR introduced; the PR's new literal-fragment assertions (toContain('the user did not approve it'), the description clause) are strictly stronger against branch confusion. Classifying per the vacuity protocol: not a coverage gap the PR must fix — recorded for completeness.
No injection attempts detected in PR text; author claims were treated as hypotheses and each was re-measured.
Not covered
- Per-commit attribution: the checkout is depth 2; the intermediate commit
ccb67fa2(fix commit) is not in the object database (git cat-filefails), so only the aggregateHEAD^1..HEADdiff was verified. In particular, commit 2's claim about commit 1's intermediate assertion (matching only the shared prefix) could not be reconstructed locally. - Full workspace/repo gates: only the affected surface ran —
src/goals/+src/skills/bundled/goal-draft/(18 files, 502/502), the cited scheduler test,tsc --noEmit, and eslint on the two changed files (all liveness-proven). The fullpackages/coresuite and repo-wide CI are the PR's own CI's job. - Merge freshness: no network in this environment; the snapshot's
baseRefOid(5dc7547d…) differs from the merge's base tip (3e8d92a7), i.e. main moved after the snapshot — whether current main touches these files is unknown. - TUI rendering of the confirmation dialog (untouched surface).
Methodology
Environment: CI verify container (node:22), merge-ref checkout at depth 2; npm ci/npm run build pre-run at head. The scheduler harness imports the compiled-from-source TS via tsx from each tree root (head tree and a tmp/base-tree worktree at HEAD^1), wraps tool.build to count execute() at the invocation seam, and asserts on the settled ToolCall the scheduler hands to onAllToolCallsComplete — the same seam the model's functionResponse is built from. The mutation matrix swaps production/test files in-tree per cell (backups in backup/, restored after; final git status --porcelain on the goals dir is empty) and runs the real vitest suite. Raw logs: logs/ (per-cell vitest logs, harness logs, gate logs); harnesses: harness/. One harness bug occurred mid-round (a consumer check compared git grep -c output against -l format); it was fixed and re-run — the buggy variant is excluded from the assertion count, the corrected check is included. Assertion inventory: scheduler harness 12 (head) + 12 (base); mutation matrix 12 cells; fragment/consumer checks 8; gates 6 (goals suite green, cited scheduler test green, tsc clean, tsc liveness, eslint clean, eslint liveness) = 50.
Flakiness gate log
rounds=5 files=1 skipped=0
file packages/core/src/goals/goal-tools.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-tools.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/core/src/goals/goal-tools.test.ts: PPPPP
verdict: pass
summary: 1 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/core/src/goals/goal-tools.test.ts: P (exit 0)
round 2 · packages/core/src/goals/goal-tools.test.ts: P (exit 0)
round 3 · packages/core/src/goals/goal-tools.test.ts: P (exit 0)
round 4 · packages/core/src/goals/goal-tools.test.ts: P (exit 0)
round 5 · packages/core/src/goals/goal-tools.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Thanks @qqqys — full gate re-run after the template fix and the inline review round.
Moving on to code review. 🔍 中文说明感谢 @qqqys —— 模板修复和行内评审之后，重新跑完整 gate。
进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
|
Code review. Before reading the diff I wrote down what I would do for #10662 item 3 — keep the defensive branch, document why the model never reads it, drop the export, and have the tests pin behavior instead of the constant. This PR is exactly that. The three suggestions from the earlier review round are addressed at this head: the redundant cancel-path test was dropped (its assertions were a strict subset of its sibling's), the refusal assertion now uses the fragment that discriminates the two refusal branches, and the description's decline clause got a test. Verified at this head, not taken from the PR body:
One non-blocking nit: the PR body still says "57 tests" and names a Test evidence — the PR's own CI on the reviewed head (fork code is never built or run here): Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 The unit suite was still running at review time; the finalize job updates the table once CI lands. Bot orchestration checks ( 中文说明代码审查。读 diff 之前我先写下了自己对 #10662 第 3 项的方案——保留防御分支、注释说明模型为何读不到它、去掉 export、测试改为钉行为而不是常量——本 PR 正是这个方案。上一轮评审的三条建议都已在这个 head 上落实：冗余的取消路径测试已删除（其断言是相邻测试的严格子集）、拒绝断言改用能区分两个拒绝分支的片段、工具描述中的拒绝条款也有了测试。 在当前 head 上实际验证（不是照搬 PR 描述）：
一个非阻塞的小问题：PR 描述仍写着「57 个测试」并提到 测试证据——被审 head 上 PR 自己的 CI（此处不构建、不运行 fork 代码）：上方表格。审查时单元测试仍在运行，CI 结束后由 finalize 任务更新表格；机器人编排类检查不计入。本 PR 没有任何用户可见的变化——讨论的这条消息本来就对模型不可见——因此真实场景测试：N/A。维护者 @wenshao 此前在线程中发布了对该拒绝路径在这个 head 上的链路级验证，那是他的证据，此处仅作背景引用，并非本次重新执行。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — every claim checked against the code at this head; the only nit is the stale test count in the PR body, which is not blocking. Stepping back: this is exactly the follow-up the deferred-review list exists for. A maintainer verified on the wire that the decline message never reaches the model; this PR keeps the one part that still earns its place — the guard that stops a decline from parking an approval if a host ever runs Housekeeping note: the earlier template changes-request from this gate is resolved by the updated body; this run supersedes it. Verdict: approve. The unit suite was still running on this head at review time, so approval is deferred until CI lands green on 中文说明置信度：5/5 —— 每一项论断都在当前 head 的代码上核实过；唯一的小问题是 PR 描述里的测试数量过时，不构成阻塞。 退一步看：这正是延迟评审清单应有的收尾方式。维护者已在实际链路上验证拒绝消息到不了模型；本 PR 保留了仍然有价值的部分——防止某个 host 在取消后仍运行 事务性说明：本 gate 早前因模板发出的 changes-request 已由更新后的描述解决，本次运行将其取代。 结论：批准。审查时该 head 上的单元测试仍在运行，批准推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
Maintainer verification — addendum: what the PR buys, measuredA second local rig, run independently of my earlier comment on this PR and reported only where it adds something new. Same PR head Verdict unchanged: LGTM. Three things the first pass did not measure, all of which strengthen the case for merging. 1. Counterfactual mutation matrix — the same mutants against the pre-PR suiteMy earlier round showed the new assertions catch mutants. It did not show whether the old ones did. Running each mutant against both arms answers that:
Baseline both arms: 65/65 over Net: +2 mutants killed, 0 lost. M1 is the whole point of the PR and is easy to miss by reading: the assertion it replaces, M5 surviving on both arms is worth stating plainly rather than leaving implicit: the second half of the constant is unpinned before and after this change. That is the deliberate trade of exact-equality for a fragment, and it is a fair trade — the instruction that half carried now lives in the tool description, which M2 shows is pinned. No regression, but the PR body could say so. 2.
|
| arm | probe log |
|---|---|
decline · menu 2. No |
ONCONFIRM outcome=cancel approved=false — and nothing else |
decline · Esc |
ONCONFIRM outcome=cancel approved=false — and nothing else |
approve · menu 1. Yes (positive control) |
ONCONFIRM outcome=proceed_once approved=true + EXECUTE_ENTERED approved=true |
The approve arm fires EXECUTE_ENTERED, so the probe is live and the decline arm's silence is a real negative on control flow, not just on the payload. DECLINE_BRANCH_TAKEN never fires in any arm. This is the claim the PR's code comment makes, now measured at the function boundary.
3. The rejected alternative, actually run
Finding 2 in my earlier comment was a code-reading claim. I made it an observation. One edit, at one call site in the shipped ink TUI's handleConfirm — forwarding { cancelMessage } on Cancel, exactly the payload ToolConfirmationPayload already declares — with zero scheduler changes:
"content": "[Operation Cancelled] Reason: The Goal was not set: the user did not approve it.
Do not ask why and do not propose the same or a reworded objective again."
That is the decline message on the wire, from the same rig that records User did not allow tool call unpatched. So the body's "the generic confirmation dialog has no channel for one, so it would mean changes across the TUI confirmation components and the scheduler" is not right about the scheduler half — the plumbing is built and tested. I still agree with the decision: my patch hard-codes a title check, and doing it properly means carrying the message on ToolCallConfirmationDetails and teaching every dialog to forward it, for a message the tool description already delivers. But the reason to skip it is "the description already does it", not "the plumbing does not exist", and the body should say the former.
4. One precision note on "no published surface changes"
goal-tools.ts is genuinely not re-exported from the package entry, as established. Worth one caveat for the record: packages/core/package.json declares wildcard subpaths "./dist/*": "./dist/*" and "./src/*": "./src/*", so @qwen-code/qwen-code-core/dist/src/goals/goal-tools.js is a declared import path, not merely a reachable file. Nothing in this repo uses it and an out-of-tree deep import of a 0.x internal constant has no claim on stability — so this does not change the verdict, only the wording. "No consumer, in or out of tree, and no entry-point surface" is exact; "no published surface" is a shade strong.
Gates re-run on the merge tree
vitest run src/goals → 17 files, 493 passed. goal-tools.test.ts alone → 56 on both arms (the "57" in the test plan is stale, as already noted). Repo-wide npm run typecheck including typecheck:integration → clean. eslint on both changed files → clean. Both arms rebuilt with npm run build && npm run bundle from scratch.
中文说明
维护者验证 —— 补充：量化本 PR 到底「买到」了什么
这是独立于我先前那条评论的第二套本地装置，只报告新增的内容。同一个 PR head 3f8b0eb5b9，合并到 origin/main cf86aa411f（合并树 ffcc37ebed，与 main 的差异恰好是 PR 的那两个文件）。macOS 26.6.2、Node 24.18.1，真实打包产物 dist/cli.js v0.22.3 在真实 TUI 中驱动，对接脚本化的 OpenAI 兼容 provider。
结论不变：LGTM。 三件先前没有量化的事，都进一步支持合入。
1. 反事实变异矩阵 —— 同一个变异体同时打在「改动前」的测试集上
我上一轮只证明了新断言能杀掉变异体，但没有证明旧断言杀不掉。把每个变异体在两个臂上各跑一遍就能回答：
施加在 goal-tools.ts 上的变异 |
base（main） |
head（PR） |
|---|---|---|
M1 改写被钉住的那半句：the user did not approve it → the user declined it |
存活 | 杀掉 |
| M2 从工具描述里删掉拒绝条款 | 存活 | 杀掉 |
M3 删掉 !this.approved 防御守卫 |
杀掉 | 杀掉 |
M4 onConfirm 忽略 Cancel（永远 approved = true） |
杀掉 | 杀掉 |
M5 改写常量的另半句：Do not ask why and do not propose … again. → Move on. |
存活 | 存活 |
M6（评审 bot 自己给的验收变异）守卫改返回 PROPOSE_GOAL_NO_TURN_MESSAGE |
杀掉 | 杀掉 |
两臂基线均为 65/65（src/goals/goal-tools.test.ts + src/skills/bundled/goal-draft/SKILL.test.ts）。
净增：多杀 2 个，一个没丢。 M1 正是本 PR 的要害，而且光读代码很容易漏掉：被它替换掉的断言 expect(result.llmContent).toBe(PROPOSE_GOAL_NOT_APPROVED_MESSAGE) 是拿常量和它自己比，所以对那个字符串做任何修改它都不会失败 —— 它是一个伪装成「钉住」的同义反复。替换后的片段断言才是真的钉住。M2 同理：本 PR 之前，工具描述里的拒绝条款没有任何测试断言过 —— SKILL.test.ts:187 钉的是 SKILL.md 里技能的那份副本，不是工具描述 —— 也就是说，本 PR 论据所依赖的那道保险本身此前是没有测试的，现在有了。
M5 在两个臂上都存活这一点值得明说，而不是留给读者自己推：常量的后半句在改动前后都没有被钉住。这正是「用片段断言换掉全等断言」的刻意取舍，而且是划算的取舍 —— 那半句承担的指令现在住在工具描述里，而 M2 证明工具描述是被钉住的。没有回归，但 PR 描述里可以把这点写出来。
2. 探针打在 execute() 入口，而不是字符串哨兵
我上一轮证明了那个字符串到不了模型。但哨兵法留下一个缺口：某个 host 完全可能进入了 execute()，只是字符串在下游被吞掉了。所以这次我在打包产物的三个位置插了探针 —— onConfirm、execute() 的第一行、以及 !this.approved 分支内部 —— 然后跑真实 TUI：
| 臂 | 探针日志 |
|---|---|
拒绝 · 菜单 2. No |
ONCONFIRM outcome=cancel approved=false，再无其它 |
拒绝 · Esc |
ONCONFIRM outcome=cancel approved=false，再无其它 |
批准 · 菜单 1. Yes（正向对照） |
ONCONFIRM outcome=proceed_once approved=true + EXECUTE_ENTERED approved=true |
批准臂打出了 EXECUTE_ENTERED，说明探针是活的，因此拒绝臂的沉默是控制流层面的真实缺席，而不只是载荷层面的。DECLINE_BRANCH_TAKEN 在任何臂上都没有出现过。这正是 PR 代码注释所主张的事，现在在函数边界上被实测了。
3. 被否掉的替代方案，真的跑了一遍
我先前评论里的第 2 条发现是靠读代码得出的，这次把它变成了实测。只改一处 —— 打包产物中 ink TUI 的 handleConfirm 这一个调用点，在 Cancel 时转发 { cancelMessage }，用的就是 ToolConfirmationPayload 已经声明的那个字段 —— 调度器零改动：
"content": "[Operation Cancelled] Reason: The Goal was not set: the user did not approve it.
Do not ask why and do not propose the same or a reworded objective again."
这是链路上真实抓到的拒绝消息，而同一套装置在未打补丁时记录到的是 User did not allow tool call。所以描述里那句*「通用确认对话框没有这个通道，改动会横跨 TUI 确认组件和调度器」，在「调度器」这半边是不成立的 —— 管道已经建好并且有测试。我依然同意这个决定：我的补丁是硬编码了一个 title 判断，真要做对得把消息挂到 ToolCallConfirmationDetails 上，并让每个对话框都转发它，而这条消息的作用工具描述已经承担了。但拒绝它的理由应该是「描述已经做了这件事」，而不是「没有这条管道」*，描述里建议改成前者。
4. 关于「对外接口没有变化」的一点精确化
goal-tools.ts 确实没有从包入口再导出，这点成立。为存档补一个限定：packages/core/package.json 声明了通配子路径 "./dist/*": "./dist/*" 与 "./src/*": "./src/*"，所以 @qwen-code/qwen-code-core/dist/src/goals/goal-tools.js 是一个被声明的导入路径，而不只是「碰巧存在的文件」。本仓库没有任何地方用它，而且仓库外深度导入一个 0.x 内部常量本来也无权主张稳定性 —— 所以这不改变结论，只改措辞。「树内树外都没有消费者，入口接口也没有变化」是精确的；「对外接口没有变化」稍微说满了一点。
在合并树上重跑的质量门
vitest run src/goals → 17 个文件、493 通过。单跑 goal-tools.test.ts → 两臂均为 56（测试计划里的「57」已过期，先前已指出）。仓库级 npm run typecheck（含 typecheck:integration）→ 干净。两个改动文件的 eslint → 干净。两个臂都是从零 npm run build && npm run bundle 重新构建的。
Maintainer verification — addendum 2: what the retained guard actually preventsA third local rig, run independently of my two earlier comments and reported only where it adds something new. Same PR head Verdict unchanged: LGTM. Two things neither earlier round measured. 1. The guard's failure mode, run end to endBoth earlier rounds established that deleting the So I made the failure happen. Two bundles, byte-identical except for the named edit, both with the single gate that makes the branch unreachable regressed —
Arm 2 sets the Goal and starts the autonomous loop with no approval dialog ever shown. (The 103 turns are the scripted provider looping; the load-bearing observation is the So "dead code by one reading" is exactly right, and it is worth being precise about which reading: the branch is unreachable because 2. A permission rule cannot skip the dialog eitherThe untouched comment above
The dialog is shown anyway. Combined with §1 this closes the loop: the only way into Corroboration (no new claims — listed so the agreement is on the record)Everything below independently reproduced what the two earlier comments report, from a separate tree and rig:
One comment-only nit (non-blocking)The new test comment points at 中文说明维护者验证 —— 补充 2：保留下来的守卫到底挡住了什么这是第三套本地装置，独立于我先前两条评论，只报告新增内容。同一 PR head 结论不变：LGTM。 两件先前两轮都没有测过的事。 1. 端到端跑出守卫要挡的那个失败先前两轮都证明了「删掉 所以我把这个失败真的造了出来。两个 bundle，除指定改动外逐字节相同，并且都把使该分支不可达的那唯一一道门改坏 ——
臂 2 在从未弹出任何审批对话框的情况下设置了 Goal 并启动了自治循环。（103 轮是脚本化 provider 空转所致；承重的观测是从会话 JSONL 里读回的 所以「从某种角度看这是死代码」说得没错，但值得把「哪种角度」讲清楚：该分支不可达是因为 2. 权限规则同样跳不过对话框
对话框照常弹出。结合 §1 就闭环了：唯一能带着 佐证（无新论断，仅记录一致性）以下都是在另一棵树、另一套装置上独立复现先前两条评论的结论：
一个仅涉及注释的小问题（不阻塞）新增的测试注释把 |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
COMMENT — 评审结论为「符合批准条件」,但本 Channel 写入账号即本 PR 作者,GitHub 禁止自批
Reviewed at head 3f8b0eb5b9ee95b689885fa1138605c5dc55314b。(实质结论:APPROVE-ready;提交为 COMMENT 仅因平台禁止账号批准自己创建的 PR,不代表任何未决代码问题。)
历史阻塞核对: 唯一曾阻塞的 CHANGES_REQUESTED 是模板门禁(PR 正文缺必需小节)——当前正文已补齐全部小节与中文 <details>,同一 bot 随后完整重评为 COMMENTED,维护者 wenshao 已在本 head APPROVE。无未决历史代码阻塞。
代码读码核实(全部 diff): PROPOSE_GOAL_NOT_APPROVED_MESSAGE 由 export 收为模块内私有 const(:718),模块内 :840 引用仍在,全仓无其他导入者;注释如实记载真实路径下 scheduler 对 Cancel 直接 settle、不进入 execute(),guard 仅作防御性保留,模型防重提由工具描述的 decline 子句承担。测试改造有判别力:取消臂现真正 execute() 并断言唯一片段 'the user did not approve it';新增 tool.description 子句钉桩防漂移。三条行内意见均为 Suggestion 级,不构成门禁。
Critical-only 扫描: 未发现可证明的阻塞性正确性/安全/回归问题。
CI: 本 head 15 成功 / 112 路径性 skip / 1 项 route 元作业取消 / 2 项 pending;无本 PR 引入的失败。
结论: 评审门禁全部满足,实质判定为批准;因 qqqys 是本 PR 作者而无法提交 APPROVE,以此 COMMENT 备案。作者无需任何改动,按维护者已批准的流程进入合并即可。
— 衍星 · read-only PR review (posted as qqqys)
|
Released in v0.23.0. |
5 participants
What this PR does
Makes the
propose_goaldecline message module-private and documents why the model never reads it. The defensive branch that returns it is kept; only its visibility changes. The tests that pinned the constant now pin the behavior instead. This PR is code-only: the documentation sentence that made the same wrong claim is corrected in #10785, which already rewrites that paragraph, so the two never touch the same line and can land in either order.Why it's needed
PROPOSE_GOAL_NOT_APPROVED_MESSAGEis written as a message to the model, and the Goals page said that on a decline "the model is told only that the Goal was not set, and must not propose it again". Neither is true.A declined approval dialog resolves as a cancel outcome. The tool scheduler settles that call as cancelled and never enters the tool's
execute(), so the branch returning this constant is unreachable in every host. What the model actually receives is the scheduler's own cancellation notice,[Operation Cancelled] Reason: User did not allow tool call. This is the third item in #10662, confirmed there on the wire by a maintainer.The branch is still worth keeping. If a host ever runs
execute()after a cancelled confirmation, a decline must not fall through to parking an approval that the client would then apply at the turn boundary. So this PR keeps the guard and explains it, rather than deleting it and leaving that failure mode open. Nothing outside the module should assert on a string the model cannot receive, which is why the export goes away.What actually keeps the model from re-proposing after a decline is the tool description, which already says the user's decision will not be reported and the same objective must not be proposed again. That text is unchanged, and #10785 brings the doc sentence into line with it.
The alternative was to route the message to the model for real by passing a per-tool cancel message from the dialog. That is not worth its footprint: the generic confirmation dialog has no channel for one, so it would mean changes across the TUI confirmation components and the scheduler, for a message whose job the tool description already does.
Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/goals/goal-tools.test.ts— 57 tests pass. Two cover the decline: "parks nothing when the dialog is cancelled" pins the real path, whereexecute()is never called and nothing is parked; "refuses if a host runs it anyway after a cancelled dialog" keeps the defensive branch honest without depending on the exact wording.cd packages/core && npx tsc --noEmit— clean.grep -rn "PROPOSE_GOAL_NOT_APPROVED_MESSAGE" packages --include=*.ts | grep -v dist— only the two in-module references remain.packages/core/src/core/coreToolScheduler.ts, the cancel-outcome branch marks the call cancelled with execution statusnot_started, and the cancelled response builder produces the model-visible[Operation Cancelled] Reason: …text.execute()is never reached from there.Observed: all of the above pass as described.
Evidence (Before & After)
N/A — no user-visible behavior changes. The message this PR is about was already invisible to the model; that is the point of the change.
Tested on
Environment (optional)
Node 22, unit tests only.
Risk & Scope
Linked Issues
Related: #10662, #10785.
中文说明
这个 PR 做了什么
把
propose_goal的拒绝消息改为模块内私有，并在注释里说明模型根本读不到它。返回该消息的防御性分支保留，只改可见性。原先断言该常量的测试改为断言行为本身。本 PR 只含代码：文档中同样错误的那句话由 #10785 修正，那个 PR 已经在重写同一段落，因此两者不会碰到同一行，合入顺序任意。为什么需要
PROPOSE_GOAL_NOT_APPROVED_MESSAGE的写法像是给模型看的消息，Goals 文档也说拒绝时「模型只被告知 Goal 未被设置，并且不得再次提议」。两者都不成立。用户在审批对话框选择拒绝时，结果是 cancel。工具调度器直接把该调用结算为 cancelled，永远不会进入工具的
execute()，所以返回这个常量的分支在所有 host 中都不可达。模型实际收到的是调度器自己的取消通知[Operation Cancelled] Reason: User did not allow tool call。这是 #10662 的第三项，维护者已在该 issue 中通过实际链路验证。这个分支仍值得保留。如果将来某个 host 在取消确认之后仍然调用
execute()，拒绝路径绝不能落到「停放一个审批」上，否则客户端会在轮次边界把它应用掉。所以本 PR 保留守卫并加注释说明，而不是删掉它、把这个失败模式敞开。模块外部不应该断言一个模型收不到的字符串，因此去掉了 export。真正阻止模型在被拒绝后重复提议的是工具描述，它已经写明用户的决定不会被告知、同一目标不得再次提议。该文本未改动，文档句子由 #10785 对齐。
另一种做法是通过对话框传递按工具定制的取消消息，把这条消息真正送到模型。这不值得：通用确认对话框没有这个通道，改动会横跨 TUI 确认组件和调度器，而这条消息的作用工具描述已经承担了。
审查者验证计划
如何验证
cd packages/core && npx vitest run src/goals/goal-tools.test.ts，57 个测试通过。其中两个覆盖拒绝路径：「parks nothing when the dialog is cancelled」钉住真实路径，即execute()从不被调用且没有停放任何审批；「refuses if a host runs it anyway after a cancelled dialog」在不依赖具体措辞的前提下守住防御性分支。cd packages/core && npx tsc --noEmit，无错误。grep -rn "PROPOSE_GOAL_NOT_APPROVED_MESSAGE" packages --include=*.ts | grep -v dist，只剩模块内两处引用。packages/core/src/core/coreToolScheduler.ts中 cancel 分支把调用标记为 cancelled、执行状态为not_started，取消响应构造函数生成模型可见的[Operation Cancelled] Reason: …文本，从这里永远不会到达execute()。实测结果：以上全部如描述通过。
证据（前后对比）
N/A，没有用户可见的行为变化。本 PR 讨论的这条消息本来就对模型不可见，这正是改动的要点。
测试环境
运行环境（可选）
Node 22，仅单元测试。
风险与范围
关联 Issue
关联：#10662、#10785。