Skip to content

feat(goal): stop a Goal at a turn or an active-time budget - #11457

Merged
wenshao merged 9 commits into
QwenLM:mainfrom
qqqys:feat/goal-turn-time-budget
Sep 11, 2026
Merged

feat(goal): stop a Goal at a turn or an active-time budget#11457
wenshao merged 9 commits into
QwenLM:mainfrom
qqqys:feat/goal-turn-time-budget

Conversation

@qqqys

@qqqys qqqys commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

A Goal can now be told to stop after a number of turns, or after an amount of time spent running, in addition to the token budget it already has. Two settings arm the ceilings: model.goalMaxTurns counts finished Goal turns, and model.goalMaxActiveMinutes counts the wall time the Goal spends active. Both are off by default, so no Goal stops for a new reason unless one is set. One thing does change with both unset: restoring an active Goal now rebases its in-flight clock, so the elapsed figure the footer pill, get_goal and the legacy durationMs projection report no longer counts time the process was down, and a restored Goal reads lower than it used to.

A Goal that reaches either ceiling takes the same path a spent token budget already takes: it gets exactly one wind-down turn to hand off what it has, then settles as usage_limited and waits. /goal resume authorizes another window on top of what has already been used, and moves only the ceiling that actually ran out. The stop carries a new limitKindturn_budget or time_budget — beside the reason prose that names the budget.

Ceilings are read at the continuation boundary, never mid-turn, so the turn that crosses one still runs to completion. When a single turn crosses more than one ceiling the Goal stops with one reason, token first, because that is the ceiling armed by default. Time is active time: a Goal paused overnight resumes with the window it had.

The hand-off prompt no longer names the token budget specifically, since it cannot know which ceiling was reached — the hosts carry a plain windDown flag. It now points at the budget line above it, and that line grew to report turns against their ceiling and, when a time ceiling is armed, active minutes against theirs.

Why it's needed

The token budget is a runaway-spend guard, sized for that job: 30,000,000 tokens by default, which a healthy long run reaches late. It is not what a user reaches for when they want to keep a Goal short. Asked to bound a Goal, people say "at most twenty turns" or "at most half an hour", not "at most 1.2 million tokens".

Today neither has any effect, and the product invites the first phrasing anyway: the objective template in docs/users/features/goals.md suggests Budget: stop as blocked after 20 turns, and the same page then has to explain that writing it configures nothing — it is an instruction the model may or may not honour, with no runtime timer behind it. That sentence is now false, which is the point of the change.

The meters were already on the record. turnCount is incremented on every finished turn and activeTimeMs is committed on every transition. Only the ceilings and the stop were missing.

Reviewer Test Plan

How to verify

cd packages/core && npx vitest run src/goals src/config/config.test.ts
cd packages/cli && npx vitest run
cd packages/web-shell && npx vitest run client/daemon/session/mappers.test.ts

Run the CLI package whole rather than by file. This change rewrites text that six assertions across three CLI suites pin verbatim, and adds a module-scope constant read that two more suites' hand-enumerated core mocks must carry; a filtered run reaches none of them.

The interesting cases are the ones where a ceiling must not fire or must not move: an unspent ceiling left alone by a resume, no ceiling retrofitted onto a Goal created without one, only the spent ceiling re-armed when two are configured, and a spent cadence budget outranking the no-progress pause so the Goal still gets its hand-off.

Two regressions worth reading rather than just running. The reducer's budget-resume branch keyed off the literal token_budget; left alone, a turn-stopped Goal would resume still rendering "ran its turn budget" as the reason it is currently active. And the Web Shell's getGoalState rebuilds the goal record from an explicit field whitelist, so both new fields and both new limitKind values had to be named there or they would be dropped on the live path while every component test still passed.

Evidence (Before & After)

Before. The settings do not exist, so they are dropped from settings.json and a Goal runs until the token budget or another bound stops it. The user doc says as much:

$ grep -n 'does not configure a runtime timer' docs/users/features/goals.md
59:`Budget` is an instruction to the model ... Writing a turn count or time limit in the objective does not configure a runtime timer or change the Goal's token budget.

After — the turn ceiling, unit-level trace of the full path. With turnBudgetGrant: 2, two ordinary continuations run, the third is flagged as the hand-off, and the Goal then settles:

turn 1 -> ordinary continuation
turn 2 -> ordinary continuation
turn 3 -> windDown: true
journal causes: create, turn_finished, turn_finished, turn_finished, usage_limited
final record: { status: 'usage_limited', limitKind: 'turn_budget', turnCount: 3, turnBudget: 2 }
lastReason:   "The Goal ran its autonomous turn budget (2 turns). Resume the Goal to authorize another window of turns, or clear it."
after resume: { status: 'active', turnCount: 3, turnBudget: 5 }   // ceiling moves, count is not reset

After — the continuation prompt's budget line, which is what the model reads:

Budget: 1,234 of 30,000,000 tokens used, 29,998,766 remaining; 4 of 20 Goal turns finished.
Budget: 900 tokens used, with no token budget on this Goal; 2 Goal turns finished; 12.3 of 30 active minutes used.

A Goal with no time ceiling reports no minutes at all, rather than an elapsed figure with nothing to measure it against.

After — startup rejection, so a misplaced zero is a message rather than a Goal that silently runs unbounded:

settings.json: model.goalMaxTurns must be > 0 (or -1 for no ceiling); got 0. Use -1 to disable, not 0.
settings.json: model.goalMaxActiveMinutes 10081 exceeds the supported ceiling (10080 minutes). Use a smaller value or -1 for no ceiling.

Tested on

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

Environment (optional)

Linux, npm run build, package-local vitest for core, cli, and web-shell.

Risk & Scope

  • Main risk or tradeoff: both settings default to no ceiling, so with neither set no Goal carries either field and the spent-budget reader reduces to the existing token check — no new stop is reachable without opting in; the one change that applies with both settings unset is the restore rebase described above. The judgement calls worth arguing with are that the ceilings are absolute rather than per-window (so a resume moves the ceiling instead of resetting the meter, matching the token budget), that a turn crossing two ceilings reports only the token one, and that active time accrues between turns as well as during them, so a Goal left active with no host to continue it does spend its time window. That last one is what activeTimeMs has always measured and what the status card already displays.
  • Not validated / out of scope: get_goal's lastGoal summary reports all three ceilings, but no display surface shows the new two yet — the footer pill, the status cards, the headless Usage: line and the Web Shell strip still show only the token pair, and that follow-up is listed in the design doc. The end-to-end plan against a live model is written up under .qwen/e2e-tests/2026-09-09-goal-turn-and-time-budget.md but has not been run on this branch. tokensUsed's own scope is unchanged.
  • Breaking changes / migration notes: no breaking change to settings, the wire or persisted records. Both record fields are optional, a Goal persisted before them restores unbounded, and both new limitKind values were added to the SDK's hand-copied union and the Web Shell's mapper whitelist in this PR, so an older daemon that never sends them behaves as before. One visible change needs no migration but belongs in the release note: a restored active Goal's elapsed figure no longer includes offline time.

Design doc: docs/design/2026-09-09-goal-turn-and-time-budget.md · docs/design/2026-09-09-goal-turn-and-time-budget.zh-CN.md

Linked Issues

Part of #4228.

中文说明

这个 PR 做了什么

现在除了已有的 token 预算,还可以让一个 Goal 在跑满若干轮之后、或者在运行了一段时间之后停下。两个设置分别武装这两道上限:model.goalMaxTurns 数的是已完成的 Goal 轮次,model.goalMaxActiveMinutes 数的是 Goal 处于活跃状态的墙钟时长。两者默认都关闭,因此不设置时不会有 Goal 因新的理由停下。但有一处在两者都不设时也会变:恢复一个 active 的 Goal 时会重置其进行中的时钟,所以底栏 pill、get_goal 与 legacy 的 durationMs 投影报告的已耗时长不再计入进程停机的时间,被恢复的 Goal 读数会比以前低。

到达任一上限的 Goal 走的是 token 预算花完时本来就走的那条路:恰好再获得一个 wind-down 轮把手上的东西交接出去,随后结算为 usage_limited 并等待。/goal resume 会在已经用掉的量之上再授一个窗口,并且只前移真正用尽的那一道上限。停机带上一个新的 limitKind——turn_budgettime_budget——与点名该预算的原因文案并列。

上限只在续跑边界读取,绝不在轮中打断,所以跨过上限的那一轮仍然会跑完。当同一轮跨过多道上限时,Goal 只报一个原因,token 优先,因为那是默认武装的那一道。时间指的是活跃时长:隔夜 pause 的 Goal 恢复时窗口不变。

交接轮的提示词不再单独点名 token 预算,因为它无从知道是哪道上限到了——三个 host 传的只是一个朴素的 windDown 标志。它现在指向上面那条预算行,而那条行也随之扩展:报出轮数与其上限,以及在武装了时长上限时报出活跃分钟数与其上限。

为什么需要

token 预算是一道失控花费的护栏,也是按这个职责定的量:默认 30,000,000 token,一次健康的长跑很晚才会碰到它。当用户想把 Goal 限短时,他要的不是这道上限。让人给 Goal 划个界,他说的是"最多二十轮"或"最多半小时",不会说"最多 120 万 token"。

而今天这两种说法都不起作用,产品本身却还在鼓励第一种:docs/users/features/goals.md 里的目标模板建议写 Budget: stop as blocked after 20 turns,同一页随后只好解释这么写什么也不配置——它只是一条模型可能遵守也可能不遵守的指令,背后没有任何运行时计时器。那句话现在不成立了,而这正是本次改动的意义。

两个计量器本来就在记录上。turnCount 在每轮结束时加一,activeTimeMs 在每次状态转换时落账。缺的只是上限本身与到达上限时的停机。

评审验证方式

如何验证

cd packages/core && npx vitest run src/goals src/config/config.test.ts
cd packages/cli && npx vitest run
cd packages/web-shell && npx vitest run client/daemon/session/mappers.test.ts

CLI 包请整包跑,不要按文件跑。本改动重写了三个 CLI 测试套里被六处断言逐字钉住的文本,又新增了一处模块作用域的常量读取——另外两个测试套手工枚举的 core mock 必须带上它;按文件过滤的跑法一个都碰不到。

值得关注的用例是那些上限该触发、或该移动的情况:resume 不动未花完的上限、不给创建时没有上限的 Goal 追加上限、配置了两道上限时只再授已花完的那道,以及已花完的节奏预算优先于空转 pause,从而让 Goal 仍然拿到它的交接轮。

有两处回归值得读而不只是跑。reducer 的预算 resume 分支原本按字面量 token_budget 判断;不改的话,一个按轮数停下的 Goal 在 resume 之后仍会把"ran its turn budget"当作它当前处于活跃状态的原因显示出来。另外 Web Shell 的 getGoalState 按一份明确的字段白名单重建 goal 记录,所以两个新字段与两个新 limitKind 取值都必须在那里点名,否则它们会在实时链路上被丢掉,而所有组件测试照样通过。

证据(前后对比)

改动前。 这两个设置并不存在,所以会被从 settings.json 里丢掉,Goal 会一直跑到 token 预算或别的界限把它停下。用户文档就是这么写的:

$ grep -n 'does not configure a runtime timer' docs/users/features/goals.md
59:`Budget` is an instruction to the model ... Writing a turn count or time limit in the objective does not configure a runtime timer or change the Goal's token budget.

改动后——轮数上限,完整路径的单测追踪。turnBudgetGrant: 2 下,两个普通续跑轮先跑,第三轮被标记为交接轮,随后 Goal 结算:

turn 1 -> ordinary continuation
turn 2 -> ordinary continuation
turn 3 -> windDown: true
journal causes: create, turn_finished, turn_finished, turn_finished, usage_limited
final record: { status: 'usage_limited', limitKind: 'turn_budget', turnCount: 3, turnBudget: 2 }
lastReason:   "The Goal ran its autonomous turn budget (2 turns). Resume the Goal to authorize another window of turns, or clear it."
after resume: { status: 'active', turnCount: 3, turnBudget: 5 }   // 上限前移,计数不清零

改动后——续跑提示词里的预算行,也就是模型真正读到的东西:

Budget: 1,234 of 30,000,000 tokens used, 29,998,766 remaining; 4 of 20 Goal turns finished.
Budget: 900 tokens used, with no token budget on this Goal; 2 Goal turns finished; 12.3 of 30 active minutes used.

没有时长上限的 Goal 完全不报分钟数,而不是报一个没有上限可比的已耗时长。

改动后——启动即拒绝,这样一个多打的零会表现为一条消息,而不是一个悄悄无上限运行的 Goal:

settings.json: model.goalMaxTurns must be > 0 (or -1 for no ceiling); got 0. Use -1 to disable, not 0.
settings.json: model.goalMaxActiveMinutes 10081 exceeds the supported ceiling (10080 minutes). Use a smaller value or -1 for no ceiling.

测试环境

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

运行环境(可选)

Linux,npm run build,core、cli、web-shell 的包内 vitest。

风险与范围

  • 主要风险或权衡:两个设置默认都是无上限,所以两者都不设时没有任何 Goal 会带上这两个字段,而"已花完预算"的读取器退化为现有的 token 检查——不主动开启就不会出现新的停机;两个设置都不设时唯一生效的变化,是上文所述的恢复时重置。真正值得商量的判断有三个:上限是绝对值而不是按窗口计数(所以 resume 前移上限而不是把计量清零,与 token 预算一致);一轮跨两道上限时只报 token 那一个;以及活跃时长在轮次之间也照样累积,所以一个处于 active 却没有 host 来续跑它的 Goal 确实会花掉它的时间窗口。最后这一点正是 activeTimeMs 一贯度量的东西,也是状态卡片已经在显示的数字。
  • 未验证 / 不在范围内:get_goallastGoal 摘要会报出三道上限,但目前还没有任何展示界面显示这两道新上限——底栏 pill、状态卡片、headless 的 Usage: 行与 Web Shell 状态条都仍然只显示 token 那一对,这项后续工作已列在设计文档里。针对真实模型的端到端计划写在 .qwen/e2e-tests/2026-09-09-goal-turn-and-time-budget.md,但尚未在本分支上跑过。tokensUsed 自身的口径未变。
  • 破坏性变更 / 迁移说明:设置、线协议与持久化记录均无破坏性变更。两个记录字段都是可选的,在它们存在之前持久化的 Goal 恢复为无上限,而两个新的 limitKind 取值已在本 PR 中同时加进 SDK 手抄的联合类型与 Web Shell 的 mapper 白名单,所以从不发送它们的老 daemon 行为不变。有一处可见变化无需迁移、但应写进发布说明:被恢复的 active Goal 的已耗时长不再包含离线时间。

设计文档:docs/design/2026-09-09-goal-turn-and-time-budget.md · docs/design/2026-09-09-goal-turn-and-time-budget.zh-CN.md

关联 Issue

Part of #4228.

The token budget is a runaway-spend guard, sized for that job: 30,000,000
tokens by default, which a healthy long run reaches late. It is not what a
user reaches for to keep a Goal short -- people say "at most twenty turns"
or "at most half an hour". Neither had any effect, and the objective
template invites the first phrasing while the user doc has to explain that
writing it configures nothing.

Add two ceilings beside the token one, both off by default:
`model.goalMaxTurns` counts finished Goal turns and `model.goalMaxActiveMinutes`
counts the wall time the Goal spends active. The meters were already on the
record; only the ceilings and the stop were missing.

A Goal that reaches either takes the path a spent token budget already
takes: one wind-down turn to hand off, then `usage_limited` with a new
`limitKind` (`turn_budget` or `time_budget`) and a reason naming the budget.
A resume authorizes another window on top of what was used and moves only
the ceiling that ran out. Ceilings are read at the continuation boundary, so
the turn that crosses one still finishes; a turn crossing several reports one
reason, token first.

- `spentBudget` replaces the runtime's direct token check, so the
  continuation gate, the settle, and the no-progress bound cannot disagree
  about whether a Goal is out of allowance. The no-progress pause yields to
  every spent budget, not just the token one, so a cadence stop still gets
  its hand-off.
- The reducer's budget-resume branch keyed off the literal `token_budget`;
  it now keys off `isGoalBudgetLimitKind`, or a turn-stopped Goal would
  resume still rendering "ran its turn budget" as the reason it is active.
- The wind-down line stops naming the token budget, since the hosts carry a
  plain `windDown` flag and cannot say which ceiling was reached. It points
  at the budget line above it, which grew to report turns against their
  ceiling and, when one is armed, active minutes against theirs.
- Both new `limitKind` values reach the two whitelists outside core: the
  SDK's hand-copied union and the Web Shell's `getGoalState`, which rebuilds
  the record field by field and would otherwise drop them on the live path.

Design: docs/design/2026-09-09-goal-turn-and-time-budget.md and its
.zh-CN.md counterpart.
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 9, 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 9e734b1. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

ℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:

  • packages/web-shell/client/i18n.tsx

Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to packages/web-shell/client/e2e/visuals/screenshots.spec.ts that seeds whatever state the UI is gated on; it then appears here as a head-only (NEW) capture.

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

Qwen Code · web-shell visuals

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Thanks for the PR! This is a re-run on an existing thread, so I've updated the three stage comments in place instead of adding new ones.

Template looks good ✓ — every required heading is present and filled in, Tested on is honest rather than optimistic (Linux ✅, macOS ⚠️, Windows ⚠️), and the design doc pair carries nine matching sections in both languages (Problem/问题, Current state/现状, Goals and non-goals/目标与非目标, Design/设计, Scope/影响范围, Constraints and risks/约束与风险, Validation/验证, Acceptance criteria/验收标准, Follow-up/后续工作).

Problem — observed, not theoretical, and the evidence is in the repo rather than only in the description. At the merge base docs/users/features/goals.md told users flatly that "Writing a turn count or time limit in the objective does not configure a runtime timer or change the Goal's token budget" — while the objective template on the same page, and propose_goal's own tool description, both suggested writing Budget: stop as blocked after 20 turns. The product advertised an affordance it never implemented. This PR makes that sentence false and then rewrites it, which is the right order. A gap that is documentary does not need a reproduction.

Direction — aligned. It completes a documented-but-unenforced affordance inside an existing feature instead of adding a new surface, and it does so by extending the token budget's machinery rather than building a parallel one. Part of #4228, referenced without a closing keyword so nothing auto-closes — correct for a partial. I did not go looking for a CHANGELOG signal; the area is plainly relevant on its own.

Size — the 2,664 changed lines break down as 668 production logic (14 files), 1,421 test (18 files), 553 docs (6 files) and 22 generated schema (1 file). It touches core paths (packages/core/src/goals/**, packages/core/src/config/config.ts, packages/cli/src/config/**) and spans five packages, so the Stage 0 gate applies. Two notes on how I resolved it:

  • The title is feat, not refactor, so the large-core-refactor hard block does not apply at any size.
  • 668 production lines is past the 500-line mark that normally escalates a fork PR to a maintainer for awareness. That escalation exists to supply architectural context the gate cannot judge, and here it is already supplied twice over: the author is a packages/core/ CODEOWNER with write access (.github/CODEOWNERS:16), so this is a maintainer-authored PR that AGENTS.md exempts from the external-PR gate; and a second core CODEOWNER has since reviewed, approved and independently verified it end to end. I'm recording the number and the reasoning rather than skipping it silently — if you read that exemption differently, this is the line to push back on. It is under the 1,000-line large-PR advisory, so no split suggested.

Approach — the scope feels right and I did not find a materially simpler path. I wrote my own proposal down before reading the diff: no new meters (the record already carries turnCount and activeTimeMs), two optional ceiling fields mirroring tokenBudget, two default-off settings, one central reader so the continuation gate, the settle and the no-progress bound cannot disagree, reuse of the existing wind-down path verbatim, resume moving only the ceiling that was spent, then an audit of every limitKind and record consumer. That is what the PR does. The single spentBudget reader is the load-bearing decision and the right one — the alternative, three separate isXSpent checks at each of four call sites, is precisely how a gate, a settle and a pause come to disagree with each other. Nothing reads as scope creep either: the two edits that look drive-by (propose_goal's objective example and the Web Shell placeholder both gaining "as model guidance") are required to keep the advisory framing honest once an enforced path exists.

Risk — no elevated risk signals. Stage 1e matched none of the revert-correlated paths; the two acp-integration files in the diff are tests, which that check excludes before matching.

Moving on to code review. 🔍

中文说明

感谢贡献!这是一次 re-run,所以我把三条 stage 评论原地更新了,没有新增。

模板完整 ✓ —— 所有必需标题都在且都填了内容,Tested on 是诚实的而不是乐观的(Linux ✅、macOS ⚠️、Windows ⚠️),设计文档中英文两版各有九个小节且一一对应(Problem/问题、Current state/现状、Goals and non-goals/目标与非目标、Design/设计、Scope/影响范围、Constraints and risks/约束与风险、Validation/验证、Acceptance criteria/验收标准、Follow-up/后续工作)。

问题 —— 是已观测到的,不是理论性的,而且证据在仓库里而不只在描述里。在 merge base 上,docs/users/features/goals.md 明确告诉用户"在 objective 里写轮数或时间限制不会配置任何运行时计时器,也不会改变 Goal 的 token 预算"——而同一页的 objective 模板、以及 propose_goal 自己的工具描述,都建议写 Budget: stop as blocked after 20 turns。产品宣传了一个自己从未实现的用法。本 PR 先让那句话失效、再把它改写掉,顺序是对的。一个有文档证据的缺口不需要复现。

方向 —— 对齐。它是在一个既有功能里补上一条"已文档化但未强制"的用法,而不是新增一个界面;而且是扩展 token 预算已有的机制,不是另建一套平行机制。Part of #4228,用了非关闭关键字所以不会自动关闭 issue —— 对于"部分完成"这是正确的写法。我没有去查 CHANGELOG 信号;这个领域本身就明显相关。

规模 —— 2,664 行改动拆解为:生产逻辑 668 行(14 个文件)、测试 1,421 行(18 个文件)、文档 553 行(6 个文件)、生成 schema 22 行(1 个文件)。它触及核心路径(packages/core/src/goals/**packages/core/src/config/config.tspackages/cli/src/config/**)并横跨五个 package,所以 Stage 0 闸门适用。两点说明我是怎么判的:

  • 标题是 feat 而不是 refactor,所以"大规模核心重构"的硬性拦截在任何体量下都不适用。
  • 668 行生产逻辑超过了那条通常会把 fork PR 上报给维护者知会的 500 行线。设置这条上报,是为了补上闸门自己判断不了的架构上下文,而这里这个上下文已经被补了两次:作者是 packages/core/ 的 CODEOWNER 且有 write 权限(.github/CODEOWNERS:16),所以这是维护者自己提的 PR,AGENTS.md 明确将其排除在"外部 PR 两级闸门"之外;而且另一位核心 CODEOWNER 已经评审、批准并独立做了端到端验证。我把数字和推理都写出来,而不是悄悄跳过 —— 如果你对这个豁免有不同看法,该反驳的就是这一段。它低于 1,000 行的大 PR 建议线,所以不建议拆分。

方案 —— 范围合理,我没有找到明显更简的路径。在看 diff 之前我先写下了自己的方案:不加新计量器(记录上本来就有 turnCountactiveTimeMs)、加两个与 tokenBudget 同形状可选上限字段、两个默认关闭的设置、一个中心读取器使续跑闸门/结算/空转界限不可能给出互相矛盾的结论、原样复用既有的 wind-down 路径、resume 只前移真正花完的那道上限,然后把 limitKind 与记录的每一个消费点都审一遍。PR 做的正是这个。那个单一的 spentBudget 读取器是承重的那处判断,而且判断正确 —— 另一种写法(在四个调用点各自做三次 isXSpent 检查)恰恰就是闸门、结算与 pause 最终互相矛盾的原因。也没有夹带无关改动:两处看起来像顺手改的地方(propose_goal 的 objective 示例与 Web Shell 的 placeholder 都加上了 "as model guidance")是必须的 —— 一旦存在了强制路径,就得让"仅供参考"这个说法保持诚实。

风险 —— 无升级风险信号。Stage 1e 没有命中任何与 revert 相关的路径;diff 里那两个 acp-integration 文件是测试,该检查在匹配之前就把测试排除了。

进入代码审查 🔍

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

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

No Critical findings. I went in expecting to break this and could not. What follows is what I actually checked, then the non-blocking residue.

The load-bearing arithmetic is the re-arm, because a window that is one in-flight increment wide is the kind of bug that passes every unit test and then stops a Goal a turn early. It holds. rearmedActiveTimeBudget computes elapsedActiveTime(current, now) and transitionGoal commits activeTimeMs: elapsedActiveTime(goal, now) — the same function, the same now, all four call sites pass transition.now to both — so the new ceiling is exactly committedElapsed + grant with nothing lost or double-counted between the two reads. And because elapsedActiveTime only extends the clock while status === 'active', a stopped Goal's elapsed figure is its committed activeTimeMs, which is what makes the re-arm well defined at all rather than racing a clock that is still running.

The restore rebase is the one behaviour that changes with both settings unset, so I read it for blast radius rather than trusting the description. It mutates recoveredSnapshot.goal.updatedAt on an object produced by structuredClone(recovery.payload.snapshot), so the persisted journal payload is untouched, and it is gated on status === 'active' — exactly the set of Goals whose in-flight clock elapsedActiveTime would otherwise extend across the whole outage. Minimal and correctly scoped.

The wind-down gate generalises without a new branch, which is the thing I most wanted to see: queueContinuation asks the one reader whether anything is spent, then grants a hand-off if windDownTurnId is unset and stops if it is set. "Exactly one hand-off then stop" is therefore a property of the record, not of the token budget, and the three ceilings inherit it for free. Every re-arm branch clears windDownTurnId, so a resumed Goal cannot be denied the hand-off it is owed.

Per AGENTS.md the bar for a core change is naming every downstream consumer, so I enumerated them repo-wide rather than accepting the description's list. All sixteen:

Consumer Needed a change This PR
core goal-protocol.ts — union + isGoalLimitKind yes ✓ widened, isGoalBudgetLimitKind factored out
core goal-reducer.ts — resume branch keyed on 'token_budget' yes ✓ now isGoalBudgetLimitKind
core goal-reducer.ts — parse whitelist + field validators yes ✓ both fields, integer for turns / number for ms
core goal-reducer.tstransitionGoal undefined-key deletes yes ✓ both, so neither reaches the JSON journal
core goal-runtime.ts — hardcoded 'token_budget' settle yes ✓ threads kind from the reader
core goal-runtime.ts — continuation gate + no-progress gate yes ✓ both call spentBudget
core goal-runtime.ts — restore rebase yes
core goal-tools.tsLastGoalSummary + tool description yes ✓ (this was missing at the first review round)
sdk-typescript hand-copied GoalLimitKind union yes
web-shell mappers.ts field whitelist + limitKind whitelist yes ✓ both
core goalLimitKindForReason no — maps only the two evidence sentinel strings, never mapped token_budget correctly untouched
core isEvidenceLimited no — an inclusion list over disjoint kinds correctly untouched
sdk-typescript/daemon/index.ts re-export no — type-only correctly untouched
cli event-adapter.ts GoalSnapshotLike no — a loose read-side display type, not a whitelist that strips fields correctly untouched
five display surfaces (footer pill, status card, GoalStatusStrip, headless Usage:, live-session-model) not required disclosed follow-up
docs/design/2026-09-07-goal-no-progress-pause.md stale prose yes

The four "correctly untouched" rows are the ones worth reading, because they are where a reviewer either satisfies themselves or finds the miss. I also grepped every display surface for a switch on limitKind: there are none, across packages/cli/src/ui, packages/web-shell/client/components and packages/acp-bridge/src. So a new kind cannot reach an unhandled branch in any UI — the only two places that validate the value are core's predicate and the Web Shell mapper, and both were updated.

One more AGENTS.md rule I applied: every added field gets its read sites grepped, because a declared-and-never-set option is a dead switch. excludedValues on SettingDefinition is live — set by both new settings, and read in two independent places, the runtime validator (settingsUtils.ts, where the includes is deliberately SameValueZero so a [0] exclusion also catches -0) and the schema generator (not: { enum: [...] }). Two readers of one declaration is the point: it is what keeps the editor JSON Schema and the runtime from drifting.

sequenceDiagram
    participant P1 as Goal turn host
    participant P2 as queueContinuation
    participant P3 as spentBudget reader
    participant P4 as goal record
    participant P5 as user
    P1->>P2: turn finished, ask for the next continuation
    P2->>P3: is any ceiling spent at this boundary
    P3->>P4: read tokensUsed, turnCount, elapsed active time
    P4-->>P3: meters plus the three armed ceilings
    P3-->>P2: token first, then turn, then time, else nothing
    alt nothing spent
        P2->>P1: ordinary continuation
    else spent and no hand-off recorded yet
        P2->>P1: one wind-down continuation via the windDown flag
        P1->>P4: hand-off finished, windDownTurnId recorded
    else spent and hand-off already recorded
        P2->>P4: settle usage_limited with limitKind and lastReason
        P4-->>P5: Goal stops and waits
        P5->>P4: goal resume
        P4->>P4: rearmedBudgets moves only the ceiling that was spent
    end
Loading
Files changed (14 production files, 25 more summarised)
File What changed
packages/core/src/goals/goal-reducer.ts The re-arm: three independent per-budget rearmers behind one entry point, plus parse/validate/delete support for the two new fields
packages/core/src/config/config.ts The two caps, the two validity predicates, the two normalisers, the two Config grants and their getters
packages/core/src/goals/goal-protocol.ts The two spent predicates, the two reason strings, the widened union and the factored-out budget-kind test
packages/core/src/goals/goal-runtime.ts The single spentBudget reader, the gate and settle rewired onto it, the prompt usage payload, the restore rebase
packages/cli/src/utils/runBudget.ts One shared startup validator for both cadence settings, so the two cannot drift on what a typo is
packages/core/src/goals/goal-continuation-prompt.ts Budget line rebuilt from segments; wind-down copy no longer names the token budget
packages/cli/src/config/settingsSchema.ts Both settings, and the new excludedValues concept on SettingDefinition
packages/cli/src/config/config.ts Reads both settings and fails startup with a settings.json-prefixed message
packages/sdk-typescript/src/daemon/types.ts The hand-copied union and the two record fields, with the older-daemon note
packages/core/src/goals/goal-tools.ts get_goal's summary carries both ceilings; propose_goal's example marked advisory
packages/web-shell/client/i18n.tsx New-goal hint points at the enforced settings; placeholder marked as guidance (EN and ZH)
packages/cli/src/config/settingsUtils.ts Enforces excludedValues, SameValueZero so signed zero is caught
packages/web-shell/client/daemon/session/mappers.ts The field and limitKind whitelists that decide what survives to the client
scripts/generate-settings-schema.ts Emits not-enum for excludedValues into the checked-in editor schema
…and 18 test files 1,421 lines: reducer, runtime, protocol, prompt, config, settings, mapper, runBudget
…and 6 docs files 553 lines: the EN and zh-CN design doc pair, settings.md, goals.md, the no-progress design note, the bundled goal-draft skill
…and 1 generated file 22 lines: the regenerated vscode settings.schema.json

Testing

What this comment carries: the PR's own CI, read through the API. This is an unattended CI run, so per the gate's rules I did not build, run or execute anything from this branch — the agent environment holds a write PAT and PR code is untrusted input. I ran no tests of my own and re-ran none of the author's.

CI is fully green at the reviewed commit. The prior review round on this thread was a 2/5 request-changes for exactly one reason: Test (ubuntu-latest, Node 22.x) was red with eight failures the PR had caused — six CLI assertions pinning the prompt text this PR rewrites, and two CLI suites whose hand-enumerated core mocks did not carry the new module-scope constant, which failed at collection and so silently skipped every case in those files. That is resolved. Test (ubuntu-latest, Node 22.x) is now success, and so is every other check that ran.

The four cancelled route checks are Qwen Autofix bot orchestration superseded by a later run of the same workflow, not PR CI — three of the seven Autofix runs on this head were cancelled and four succeeded. Everything else that is not success is skipped, and the large majority of those are bot command jobs that never had reason to run.

Two skips are worth naming rather than burying: Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) did not run, and neither did Integration Tests (CLI, No Sandbox). So the unit-suite evidence on this head is ubuntu-only, which lines up with the Tested on table marking both ⚠️. The change is pure TypeScript logic with no path, fs or process handling, so I would not expect platform divergence — but that is a judgement about likelihood, not a measurement, and it is the one real gap in the evidence below.

Check Conclusion
Qwen Code CI success
Test (ubuntu-latest, Node 22.x) success
Test (macos-latest, Node 22.x) skipped
Test (windows-latest, Node 22.x) skipped
Lint & Static (ubuntu-latest, Node 22.x) success
Integration Tests (no-AK, No Sandbox) success
Integration Tests (CLI, No Sandbox) skipped
Serve A/B (ubuntu-latest, Node 22.x) success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) success
Capture web-shell visuals (ubuntu-latest, Node 22.x) success
TUI parity snapshots (ink vs opentui) success
OpenTUI no-flicker gate success
Desktop Shell (ubuntu-22.04) success
Desktop Shell (windows-2022) success
Qwen Live Host CI success
Live Host (macos-latest) success
Real daemon E2E / Java 11 success
SDK Java (ubuntu 11/17/21, macos 21, windows 21) success
Classify PR · assign · label · authorize · precheck-pr · review-pr success
route × 4 (Qwen Autofix orchestration, superseded) cancelled

Totals on this head: 274 checks — 29 success, 241 skipped, 4 cancelled, 0 failure. Skips are overwhelmingly bot command jobs; the three that matter are named above.

Separately, and attributed rather than adopted: @wenshao posted an independent local end-to-end verification of this exact head, driving a real qwen serve daemon against a real Goal runtime with nothing in packages/core/src/goals mocked, and — the part that makes it worth more than a green suite — a negative control per behaviour, neutralising the relevant line in the built bundle and confirming the behaviour disappears. That is what establishes the new lines are load-bearing rather than decorative. It is a maintainer's result, not mine; I did not re-run it and could not have from this lane.

Sandboxed verification, and what is still unsettled. A /verify lane is already in flight for this PR (the qwen-triage:verify comment above, run 34534561125) and will post its own report; I am not treating it as evidence until it lands. What neither it nor the table above closes is the platform gap: the suite ran on ubuntu only, so the two skipped Test jobs are the unsubstantiated claim here. @qwen-code /tmux would additionally settle the TUI half of the disclosed follow-up — that the footer pill, the /goal status card and the OpenTUI strip still render only the token pair, so a user who sets model.goalMaxTurns gets no on-screen signal until the stop itself.

Non-blocking residue

Three review threads are still unresolved, all [Suggestion], none a correctness defect. This PR is at round five, and AGENTS.md says that past roughly five rounds only Critical fixes should land and the remaining Suggestions go to a follow-up — so my recommendation is to defer all three, not to open another round.

  1. SETTABLE_TYPES omits 'integer' (packages/cli/src/ui/commands/config-command.ts:26). I verified the premise against the code rather than the thread text: the set at main is ['boolean', 'string', 'number', 'enum'], and both new settings are type: 'integer'. So the bare /config listing, key completion and the "Did you mean" suggestion all hide them, while the set path itself works. This PR's own new-goal hint is the only product string that tells a user to type /config <key>=<value>, and it names both keys — which is what makes a pre-existing gap newly reachable. It is not a regression: thirteen integer settings are already hidden at main, including goalTokenBudget, this setting's direct sibling, and model.maxSessionTurns. The fix is one word in a file this PR does not touch, so it belongs in a follow-up with a test asserting the listing contains model.goalMaxTurns.
  2. The new-goal hint asks for a daemon restart that the new-goal flow does not need — that flow allocates a fresh session whose Config re-reads settings from disk, so the ceiling is already armed on the Goal the user creates next, while the restart the copy requests drops every live session the daemon hosts. Copy accuracy in EN and its ZH twin.
  3. docs/users/features/goals.md omits the admission caveat that the other three surfaces describing this setting carry — a user turn is still admitted at the ceiling and is what turns the next autonomous continuation into the wind-down. I confirmed the asymmetry: settings.md, the schema description and the turnBudget JSDoc all say it, the guide does not. Docs-only.

Two description-accuracy items I confirmed independently, since a reviewer will hit both:

  • The description says the end-to-end plan "is written up under .qwen/e2e-tests/2026-09-09-goal-turn-and-time-budget.md". That path does not exist at this head. Worth being precise about why, because the obvious explanation is wrong: .qwen/* is git-ignored, but 26 e2e-test plans are nevertheless force-added and tracked at this commit, so committing it is the established convention here and this PR simply does not. Either add it or reword to "planned, not written up".
  • The evidence block quotes lastReason as "ran its autonomous turn budget"; the string the code ships is "ran its Goal-turn budget". One-line fix so the body matches the merged behaviour.

One prompt-fidelity nit worth a look if anyone is in that file: renderActiveMinutes uses maximumFractionDigits: 1, which rounds half-up, so a Goal at 57.9 s of a 60 s window prints 1 of 1 active minutes used on an ordinary turn and the identical figures on the wind-down that follows. Two consecutive turns read as fully spent with different meanings, and a model told 1 of 1 has a fair reason to start handing off a turn early. The runtime gate decides the stop, not this prose, and the explicit wind-down instruction arrives on its own line — so this is a fidelity wart, not a bug. Flooring the elapsed side, or two decimals, removes it.

Finally, the disclosed gap I would keep visible rather than let the approval bury: no display surface carries the new ceilings yet. get_goal reports all three, but the footer pill, the status cards, the headless Usage: line and the Web Shell strip still show only the token pair. For a default-off feature with the follow-up already listed in the design doc that is a reasonable place to land, and it is the honest reason this is a 4 and not a 5.

中文说明

代码审查

没有 Critical 问题。 我是抱着把它弄坏的心态看的,没成功。下面是我真正查过的东西,以及剩下的非阻塞项。

承重的算术在"重新武装"这一段,因为一个差了一个在途增量的窗口,正是那种能通过每一个单测、却让 Goal 提前一轮停下的 bug。它是对的。rearmedActiveTimeBudget 算的是 elapsedActiveTime(current, now),而 transitionGoal 落账的是 activeTimeMs: elapsedActiveTime(goal, now) —— 同一个函数、同一个 now,四个调用点传给两者的都是 transition.now —— 所以新上限恰好等于 已落账的 elapsed + grant,两次读取之间既没丢也没重复计。而且因为 elapsedActiveTime 只在 status === 'active' 时才延展时钟,一个已停下的 Goal 其 elapsed 就是已落账的 activeTimeMs;这正是"重新武装"有良定义、而不是在跟一个仍在跑的时钟赛跑的原因。

恢复时重置时钟,是两个设置都不设时唯一会变的行为,所以我按影响面去读它,而不是采信描述。它改的是 recoveredSnapshot.goal.updatedAt,而这个对象来自 structuredClone(recovery.payload.snapshot),所以持久化的 journal payload 没有被碰到;而且它以 status === 'active' 为门 —— 恰好就是"在途时钟否则会被整段停机时间延展"的那一批 Goal。改动最小且范围正确。

wind-down 闸门在没有新增分支的前提下完成了泛化,这是我最想看到的:queueContinuation 只问那个唯一的读取器"有没有花完",然后按 windDownTurnId 是否已记录来决定发交接轮还是停机。于是"恰好一个交接轮然后停下"成了记录本身的性质,而不是 token 预算的性质,三道上限免费继承。每一个重新武装分支都会清掉 windDownTurnId,所以被恢复的 Goal 不会被赖掉它应得的交接轮。

按 AGENTS.md,核心改动的门槛是点出每一个下游消费点,所以我是全仓库枚举的,而不是采信描述里那份清单。一共十六处,见上表。真正值得读的是那四行"正确地未改动",因为审查者要么在那里说服自己、要么在那里发现漏改。我还 grep 了所有展示界面有没有对 limitKindswitchpackages/cli/src/uipackages/web-shell/client/componentspackages/acp-bridge/src 全都没有。所以一个新的 kind 不可能在任何 UI 里落到未处理的分支 —— 真正会校验这个值的只有 core 的谓词与 Web Shell 的 mapper,两处都改了。

另外我用了 AGENTS.md 的一条规则:每个新增字段都要 grep 它的读取点,因为"声明了却从没人设值"的选项就是一个死开关。SettingDefinition 上的 excludedValues 是活的 —— 两个新设置都为它赋了值,并且有两处独立读取:运行时校验器(settingsUtils.ts,那里的 includes 是刻意选 SameValueZero 的,所以 [0] 这条排除也能抓到 -0),以及 schema 生成器(not: { enum: [...] })。一份声明被两处读取正是重点:这就是让编辑器 JSON Schema 与运行时不会各自漂移的机制。

测试

本评论携带的证据是:PR 自己的 CI,通过 API 读取。 这是一次无人值守的 CI 运行,所以按闸门规则我没有构建、运行或执行本分支的任何东西 —— agent 环境里带着 write PAT,而 PR 代码是不可信输入。我没有自己跑测试,也没有复跑作者的。

在被审的这个 commit 上 CI 全绿。本线程上一轮的结论是 2/5 并要求修改,原因只有一条:Test (ubuntu-latest, Node 22.x) 是红的,八处失败都由本 PR 造成 —— 六处 CLI 断言钉住了本 PR 重写掉的提示词文本,两个 CLI 测试套手工枚举的 core mock 没带上新增的模块作用域常量,因此在收集阶段就失败、把这两个文件里的每个用例都静默跳过了。这条已经解决:Test (ubuntu-latest, Node 22.x) 现在是 success,其他所有真正跑起来的检查也都是。

四个 cancelledroute 检查是 Qwen Autofix 的机器人编排被同一 workflow 的后续运行取代,不是 PR CI —— 这个 head 上七次 Autofix 运行里有三次被取消、四次成功。其余非 success 的都是 skipped,其中绝大多数是从没有理由运行的机器人命令任务。

有两个 skip 值得点名而不是埋掉:Test (macos-latest, Node 22.x)Test (windows-latest, Node 22.x) 没有跑Integration Tests (CLI, No Sandbox) 也没有。所以这个 head 上的单测证据只有 ubuntu,这与 Tested on 表里两行 ⚠️ 是一致的。改动是纯 TypeScript 逻辑,不涉及路径、文件系统或进程处理,所以我不预期平台差异 —— 但那是关于可能性的判断,不是测量,也是下面证据里唯一真正的缺口。

另外,明确注明出处而不当作自己的结论:@wenshao 针对这同一个 head 发了一份独立的本地端到端验证,用真实 qwen serve daemon 驱动真实 Goal 运行时,packages/core/src/goals 里没有任何东西被 mock;而更有价值的是每项行为都配了一个反向对照 —— 在构建产物里把对应那行禁用掉,确认该行为随之消失。这才证明了新增的代码行是承重的而不是装饰。那是维护者的结论,不是我的;我没有复跑,在这个通道里也无法复跑。

沙箱验证,以及仍未落定的部分。 本 PR 已经有一个 /verify 通道在跑(上面那条 qwen-triage:verify 评论,run 34534561125),它会自己发报告;在它落地之前我不把它当证据。它和上面那张表都没有闭合的是平台缺口:套件只在 ubuntu 上跑过,所以那两个被跳过的 Test 任务就是此处未被证实的部分。@qwen-code /tmux 则还能落定已披露的后续工作中 TUI 的那一半 —— 底栏 pill、/goal 状态卡片与 OpenTUI 状态条仍然只显示 token 那一对,所以设置了 model.goalMaxTurns 的用户在停机之前得不到任何屏幕信号。

非阻塞的遗留项

三条 review thread 仍未 resolve,全部是 [Suggestion],没有一条是正确性缺陷。本 PR 已经到了第五轮,而 AGENTS.md 说超过约五轮之后只应落 Critical 修复、其余 Suggestion 转到后续 issue 或 PR —— 所以我的建议是三条都延后,不要再开一轮。

  1. SETTABLE_TYPES 里没有 'integer'packages/cli/src/ui/commands/config-command.ts:26)。这个前提我是对着代码验的,不是照着 thread 的文字:main 上那个集合是 ['boolean', 'string', 'number', 'enum'],而两个新设置都是 type: 'integer'。于是裸 /config 列表、键补全、"Did you mean" 建议都把这两个键藏起来了,而设置路径本身是通的。本 PR 自己新加的"新建目标"提示,是产品里唯一一条告诉用户去输入 /config <key>=<value> 的文案,而它点名的正是这两个键 —— 这就是一个既有缺口为何变得新可达。但这不是回归:main 上已经有十三个 integer 设置同样被藏着,其中就包括 goalTokenBudget(本设置的直接同类)和 model.maxSessionTurns。修法是在一个本 PR 未触及的文件里改一个词,所以它属于后续工作,并应补一条断言"裸列表包含 model.goalMaxTurns"的测试。
  2. "新建目标"提示要求的那次 daemon 重启,是这个流程本身并不需要的 —— 该流程会分配一个全新会话,其 Config 会从磁盘重读设置,所以下一个创建的 Goal 已经带上了这道上限;而文案要求的那次重启会连带终止 daemon 上托管的每一个在线会话。中英文两版都是文案准确性问题。
  3. docs/users/features/goals.md 漏掉了那条放行限定,而描述该设置的另外三处界面都带着它 —— 用户轮次在上限已到之后仍会被放行,而正是它把下一次自主续跑变成交接轮。这个不对称我确认过:settings.md、schema 描述、turnBudget 的 JSDoc 都写了,这份指南没写。纯文档。

另有两条描述准确性问题,我独立确认过,因为审查者一定会撞上:

  • 描述说端到端计划"写在 .qwen/e2e-tests/2026-09-09-goal-turn-and-time-budget.md"。该路径在这个 head 上不存在。这里值得把原因说准,因为最直觉的解释是错的:.qwen/* 确实被 git-ignore,但在这个 commit 上有 26 份 e2e 测试计划是被强制加入并已跟踪的,所以"提交它"才是本仓库既有的惯例,而本 PR 没有提交。要么补上文件,要么改成"已规划但尚未写出"。
  • 证据块里把 lastReason 引作 "ran its autonomous turn budget";代码实际发出的是 "ran its Goal-turn budget"。改一行,让正文与合入后的行为一致。

还有一处提示词保真度的小问题,如果有人正好要动那个文件可以顺手看:renderActiveMinutes 用了 maximumFractionDigits: 1,会向上取整,所以 60 s 窗口里跑到 57.9 s 的 Goal 会在一个普通轮上打印 1 of 1 active minutes used,而紧随其后的交接轮打印的数字一模一样。连续两轮都读作"已用尽"却含义不同,而一个被告知 1 of 1 的模型有充分理由提前一轮开始交接。决定停机的是运行时闸门而不是这段文案,且明确的交接指令是单独一行下发的 —— 所以这是保真度上的瑕疵,不是 bug。对已耗侧向下取整、或者保留两位小数,就能消除。

最后,那个已披露的缺口我希望它保持可见,而不是被这次批准埋掉:目前还没有任何展示界面带上这两道新上限。 get_goal 会报出三道,但底栏 pill、状态卡片、headless 的 Usage: 行与 Web Shell 状态条仍然只显示 token 那一对。对一个默认关闭、且后续工作已写进设计文档的功能来说,这是一个合理的落点 —— 而这也正是我给 4 分而不是 5 分的诚实理由。

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

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — solid, and I would merge it; the missing point is that the feature is enforced but not yet visible, plus three Suggestion-level copy items I am recommending be deferred rather than fixed in a sixth round.

Stepping back. My last pass on this thread was a 2/5 request-changes, and it is worth being precise about what changed, because it was not my opinion of the code. Even then I wrote that the design and the production logic were mergeable and that the failure was mechanical: a red Test check with eight PR-caused failures. Four rounds later the same check is green at this head, the six stale prompt-text pins are updated, and both hand-enumerated core mocks carry the new constants. So this is not a reversal under pressure — the one blocking fact was resolved and the rest of my assessment stands where it was.

Going back to the proposal I wrote before reading the diff: the PR matches it essentially point for point, and beats it in one place. I had assumed a single spent-budget reader; the PR additionally makes the reason string and the limit kind come out of that same reader as one value, so the prose a user reads and the kind a client branches on cannot be derived from different ceilings. That is a better shape than what I sketched, and it is why the reducer's resume branch could stop keying off the literal token_budget — the regression the description calls out, which would otherwise have left a turn-stopped Goal rendering "ran its turn budget" as the reason it is currently active.

On the questions I am supposed to ask myself here. Does it solve something users care about: yes, and unusually the evidence is the product's own documentation contradicting itself — a template that suggested Budget: stop as blocked after 20 turns next to a sentence saying that writing it configures nothing. Is the code trying too hard: no. 668 production lines against 1,421 lines of test is the ratio you want on a change to a state machine that decides when autonomous work stops, and the production side is mostly the two new ceilings walking the exact path the token budget already cut. Is every change necessary: yes — I looked hard for scope creep and the two "as model guidance" edits are the only candidates, and both are required to keep the advisory framing honest now that an enforced path exists. Would I curse the author in six months: no, I would thank them, because the next ceiling gets added by extending one reader rather than by finding four call sites and hoping.

Am I being a pushover? I checked that against the thing that should make me most suspicious, which is that this PR arrives with a maintainer's approval and a thorough verification already attached. Volume and advocacy both wear a reviewer down, so I did not take the consumer list on the description's word — I enumerated all sixteen myself, and the interesting result was the four that are correctly untouched, since that is where a real miss would hide. I also verified the SETTABLE_TYPES premise against main rather than against the thread text, and verified that the missing e2e-test plan is genuinely absent from a directory where 26 comparable plans are tracked. Both held up. The re-arm arithmetic I checked independently because it is the one place a subtle off-by-one would pass every test in the suite.

What I am not claiming. I ran nothing — no build, no test, no tmux; this is an unattended CI lane and PR code is untrusted input in an environment holding a write PAT. My evidence is the CI table plus static reading. The suite ran on ubuntu only, so macOS and Windows are unmeasured, and my "pure TypeScript logic, no platform surface" reasoning is a judgement about likelihood rather than a result. The behavioural claim rests on @wenshao's negative-control verification, which I have attributed rather than adopted, and on a /verify run still in flight that I have not counted. None of that is a reason to hold the PR: the platform gap is low-risk for logic with no fs or process surface, and the behaviour is the best-substantiated part of this PR, not the weakest.

So: approve, with three things I would ask for as follow-ups rather than as another round on this branch — the 'integer' gap in SETTABLE_TYPES (pre-existing, thirteen settings already affected including this one's sibling, but this PR's hint copy is what makes it user-visible), the two description corrections, and the display surfaces that still show only the token pair. The last one is the reason a user who sets model.goalMaxTurns gets no signal until the stop, and it is already on the design doc's follow-up list, so it is tracked rather than lost.

中文说明

信心度:4/5 —— 扎实,我会合入。少的那一分在于:这个功能已经能强制生效、但还不可见;另外三条 Suggestion 级的文案问题,我建议延后处理,而不是再开第六轮。

退一步看。我在这个线程上一次的结论是 2/5 并要求修改,这里值得把"变了什么"说准,因为变的不是我对代码的看法。即便在那一轮我也写了:设计与生产逻辑是可以合的,失败是机械性的 —— 一个红的 Test 检查,八处失败都由本 PR 造成。四轮之后,同一个检查在这个 head 上是绿的,六处过期的提示词固定断言已更新,两个手工枚举 core 导出的 mock 也都带上了新常量。所以这不是在压力下改口:唯一那条阻塞事实被解决了,而我其余的判断仍停在原处。

回到我在看 diff 之前写下的方案:PR 与它几乎逐点吻合,并且在一处胜过它。我原本只假设了一个"预算是否花完"的中心读取器;PR 还让原因文案limit kind 从同一个读取器里作为一个值一起出来,于是用户读到的散文与客户端据以分支的 kind 不可能来自两道不同的上限。这比我勾画的形状更好,也正是 reducer 的 resume 分支能够不再按字面量 token_budget 判断的原因 —— 那就是描述点出的那个回归,否则一个按轮数停下的 Goal 在 resume 之后仍会把"ran its turn budget"当作它当前处于活跃状态的理由。

关于我在这个阶段该问自己的问题。是否解决了用户真的在意的事:是,而且不寻常的是,证据是产品自己的文档在自相矛盾 —— 一份建议写 Budget: stop as blocked after 20 turns 的模板,紧挨着一句说这么写什么也不配置。代码是否用力过猛:没有。668 行生产逻辑对 1,421 行测试,正是一处"决定自主工作何时停下"的状态机该有的比例,而生产侧大部分只是让两道新上限沿着 token 预算已经开好的那条路走。每处改动是否必要:是 —— 我认真找过夹带,唯二的候选是那两处 "as model guidance",而一旦存在了强制路径,这两处就是让"仅供参考"的说法保持诚实所必需的。半年后维护它我是否会骂作者:不会,我会感谢他,因为下一道上限的加法是扩展一个读取器,而不是找齐四个调用点然后祈祷。

我是不是太好说话了?我拿最容易让我起疑的那件事来检验过 —— 这个 PR 到手时就已带着维护者的批准和一份详尽的验证。数量和背书都会磨软审查者,所以我没有采信描述里那份消费点清单:十六处是我自己枚举的,而有意思的结果恰恰是那四处正确地未改动,因为真正的漏改就藏在那里。SETTABLE_TYPES 那个前提我也是对着 main 验的,不是照着 thread 的文字;那份缺失的 e2e 测试计划,我也确认了它确实不在——而同目录下有 26 份同类计划是被跟踪的。两处都站得住。重新武装那段算术我是独立查的,因为它是全套测试都可能放过的、微妙差一的最可能藏身之处。

没有主张的部分。我什么都没跑 —— 没有构建、没有测试、没有 tmux;这是无人值守的 CI 通道,而在一个带着 write PAT 的环境里,PR 代码就是不可信输入。我的证据是那张 CI 表加静态阅读。套件只在 ubuntu 上跑过,所以 macOS 与 Windows 是未测量的,而我那句"纯 TypeScript 逻辑、没有平台面"是关于可能性的判断,不是结果。行为层面的主张依靠的是 @wenshao 带反向对照的验证 —— 我注明了出处而没有据为己有 —— 以及一个仍在跑、我没有计入的 /verify。这些都不是扣住这个 PR 的理由:对一个没有文件系统或进程面的逻辑改动来说,平台缺口风险很低;而行为恰恰是本 PR 被证实得最充分的部分,不是最弱的部分。

所以:批准,同时有三件事我会作为后续工作提出,而不是在这个分支上再走一轮 —— SETTABLE_TYPES 里的 'integer' 缺口(既有问题,已有十三个设置受影响、包括本设置的同类,但让它在用户面前显形的是本 PR 的提示文案)、两处描述更正、以及仍然只显示 token 那一对的展示界面。最后这一条正是"设置了 model.goalMaxTurns 的用户在停机前得不到任何信号"的原因,而它已经在设计文档的后续工作清单上,所以是被跟踪的、不是丢了的。

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

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The design and the production logic are good — I could not find a correctness defect in them, and I checked every downstream consumer of GoalLimitKind and GoalRecord rather than trusting the "no host changes" claim. But the required Test (ubuntu-latest, Node 22.x) check is red because of this PR: 5 test files failed, 6 tests failed, from two independent causes. Full detail with file and line for all eight sites is in my review comment above; the short version:

1. Six CLI assertions still pin the prompt text this PR rewrote. Renaming Token budget: to Budget: and rewording the wind-down line changes rendered output that four files pin verbatim. Core's own goal-continuation-prompt.test.ts was updated; these three were not:

  • packages/cli/src/nonInteractiveCli.test.ts:1327 and :1507
  • packages/cli/src/acp-integration/session/Session.test.ts:24885 and :24753
  • packages/cli/src/ui/hooks/use-llm-stream.test.tsx:656 and :562

2. Two CLI test files cannot collect at all, because settingsSchema.ts:1657 now reads GOAL_MAX_TURNS_CAP at module scope and these two build their @qwen-code/qwen-code-core mock by enumerating exports rather than using importOriginal:

  • packages/cli/src/acp-integration/acpAgent.worktree.test.ts:153
  • packages/cli/src/serve/workspace-service/__tests__/facade.test.ts:76

Both already carry GOAL_CHECKPOINT_TIMEOUT_SECONDS_CAP: 900, in that list, so add GOAL_MAX_TURNS_CAP and GOAL_MAX_ACTIVE_MINUTES_CAP beside it.

Both causes were out of reach of the verification steps in the PR description, which run only src/utils/runBudget.test.ts and src/config/config.test.ts in the CLI package — that is why a green local run coexisted with a red CI. cd packages/cli && npx vitest run catches all eight.

This is a fix-and-return, not a rethink: nothing about the direction, the scope, the budget semantics or the design needs to change. Two non-blocking notes are in the review comment (get_goal's LastGoalSummary does not project the new ceilings, and one doc comment says "positive integer" where the predicate also accepts 0) — take them only if you are already in those files.

中文说明

设计与生产逻辑都是好的——我没能找出正确性缺陷,而且我没有采信"host 侧零改动"这句话,是把 GoalLimitKindGoalRecord 的每个下游消费点都查了一遍。但必需的 Test (ubuntu-latest, Node 22.x) 检查因为这个 PR 而变红:5 个测试文件失败、6 个用例失败,来自两个互相独立的成因。全部八处的文件与行号都在上面我的审查评论里,简述如下:

1. 六处 CLI 断言仍钉着本 PR 重写掉的提示词文本。Token budget: 改名为 Budget:、并改写 wind-down 那一句,改变的是被四个文件逐字钉住的渲染输出。core 自己的 goal-continuation-prompt.test.ts 更新了,以下三个没有(六个位置见上方英文列表)。

2. 两个 CLI 测试文件连收集都完不成,因为 settingsSchema.ts:1657 现在在模块作用域读取 GOAL_MAX_TURNS_CAP,而这两个文件是靠枚举导出来构造 @qwen-code/qwen-code-core mock 的(而不是用 importOriginal)。两个文件在那份清单里本来就带着 GOAL_CHECKPOINT_TIMEOUT_SECONDS_CAP: 900,,所以在旁边补上 GOAL_MAX_TURNS_CAPGOAL_MAX_ACTIVE_MINUTES_CAP 即可。

这两个成因都在 PR 描述的验证步骤之外——那些步骤在 CLI 包里只跑了 src/utils/runBudget.test.tssrc/config/config.test.ts,这就是本地全绿与 CI 变红能够并存的原因。cd packages/cli && npx vitest run 能把这八处全部抓到。

这是修完再回来,不是重做:方向、范围、预算语义与设计都不需要改。两条不阻塞的意见在审查评论里(get_goalLastGoalSummary 没有投影两道新上限;有一处文档注释写着"正整数"而谓词其实也接受 0)——只有在你正好要动那些文件时才顺手处理。

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

Six CLI assertions pinned the continuation prompt verbatim, and renaming
the budget line's prefix and rewording the wind-down line moved that text
out from under them. Two more CLI suites could not collect at all: they
build their `@qwen-code/qwen-code-core` mock by enumerating exports, and
`settingsSchema.ts` now reads two new caps at module scope.

Neither was reachable from the verification steps this change shipped
with, which ran only `runBudget.test.ts` and `config.test.ts` in the CLI
package. `cd packages/cli && npx vitest run` reaches all eight.

Also, while in these files:

- `get_goal`'s `lastGoal` summary already reported `tokenBudget`, so a
  Goal stopped by a cadence ceiling could be inspected and show only the
  allowance that did not run out. It now reports all three, and the tool
  description says so.
- Two validator doc comments said "-1 for no ceiling, or a positive
  integer"; the predicates accept `0` as the same opt-out, the way the
  token budget's sibling already documents it.
- `settingsSchema.test.ts` now pins both new entries: integer, no
  default, bounded by their caps, no restart.
@qqqys

qqqys commented Sep 9, 2026

Copy link
Copy Markdown
Collaborator Author

Both causes fixed in 0f98f80, and you were right that neither was reachable from the verification steps I shipped: they ran two CLI files, and the breakage was in three others plus two that could not collect. I have replaced that section of the description with cd packages/cli && npx vitest run.

1. The six pinned assertions. Updated to the rewritten text at all six sites. Verified by running the three files whole rather than filtered: nonInteractiveCli.test.ts 162 passed, use-llm-stream.test.tsx 280 passed, Session.test.ts 861 passed.

2. The two enumerated mocks. GOAL_MAX_TURNS_CAP and GOAL_MAX_ACTIVE_MINUTES_CAP added beside the checkpoint cap they already carried, in both files. acpAgent.worktree.test.ts 3 passed, facade.test.ts 132 passed.

I took both non-blocking notes as well, because the first one turned out to be a real gap rather than a cosmetic one:

  • get_goal's lastGoal already reported tokenBudget. So a Goal stopped by a turn or time ceiling could be inspected and would show only the allowance that did not run out, with the reason prose as the sole hint. It now reports all three ceilings, the tool description names them, and the existing lastGoal test pins them.
  • The "positive integer" wording. Both predicates accept 0 as the same opt-out -1 is an alias for, which is how the token budget's sibling already documents it. Corrected in both doc comments.

One thing I did not do: surface the new ceilings in the pill, the status cards, the headless Usage: line or the Web Shell strip. That is still listed as follow-up in the design doc — this PR deliberately stops at the runtime and the model-facing read paths.

settingsSchema.test.ts now also pins both new entries (integer, no default, bounded by their caps, no restart), so the caps cannot drift out of the schema silently.

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

Built the PR base vs this PR head 9e734b1, drove a fixed endpoint set against each, and diffed the JSON responses. Only fields that changed are shown.

health-deep-with-session

field PR base (before) this PR (after)
activeWorkStaleMs 8 6

Qwen Code · serve A/B

Comment thread packages/core/src/goals/goal-runtime.ts
Comment thread packages/cli/src/config/settingsSchema.ts
Comment thread packages/cli/src/config/settingsSchema.ts Outdated
Comment thread docs/users/features/goals.md
Comment thread docs/design/2026-09-09-goal-turn-and-time-budget.md Outdated
Comment thread packages/core/src/goals/goal-reducer.ts
Comment thread packages/core/src/goals/goal-runtime.test.ts
Comment thread packages/core/src/goals/goal-runtime.test.ts
Comment thread packages/core/src/goals/goal-runtime.test.ts
Comment thread docs/design/2026-09-09-goal-turn-and-time-budget.md

@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 — web-shell E2E Smoke (ubuntu-latest, Node 22.x) is red at the reviewed head and could not be attributed: the job log was unreadable through the API, and the scoped local run does not execute the Playwright smoke suite.

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

Not explored to full depth (tool budget reached): "agent reverse-audit (round 2)": attributing the red web-shell E2E Smoke (ubuntu-latest, Node 22.x) check on the reviewed head 014ce407 — it passed on the two preceding PR commits (71b5daec, …; "agent reverse-audit (round 2)": none of my planned checks were cut short, but two things I verified only as far as my chunk allows: I did not walk settingsSchema.test.ts past diff line 746 (….

Not reviewed: reverse audit — stopped before round 5 by the review time budget.

Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:

  • docs/design/2026-09-09-goal-turn-and-time-budget.md:116 — [probe] 2026-09-07-goal-continuation-budget.md contradicts the…
  • packages/cli/src/config/config.test.ts:1355 — [probe] Accept-path test covers turns but not active minutes
  • packages/cli/src/config/settingsSchema.ts:1649 — [probe] A trusted repo can silently loosen an operator-set Goal…
  • packages/core/src/goals/goal-reducer.test.ts:1870 — [probe] Fourth rearmedBudgets call site has no cadence test
  • packages/core/src/goals/goal-reducer.test.ts:1879 — [probe] Two cadence ceilings spent at once is untested in the…
  • packages/core/src/goals/goal-runtime.test.ts:6709 — [probe] Only the paused leg of the accrual guard is tested
  • packages/core/src/goals/goal-runtime.test.ts:6878 — [probe] Time-ceiling precedence over the no-progress bound is…
  • packages/core/src/goals/goal-tools.ts:224 — [probe] get_goal understates an active Goal's used window
中文说明

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

未审查(原文为英文):build-and-test — web-shell E2E Smoke (ubuntu-latest, Node 22.x) is red at the reviewed head and could not be attributed: the job log was unreadable through the API, and the scoped local run does not execute the Playwright smoke suite.

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 2)"attributing the red web-shell E2E Smoke (ubuntu-latest, Node 22.x) check on the reviewed head 014ce407 — it passed on the two preceding PR commits (71b5daec, …"agent reverse-audit (round 2)"none of my planned checks were cut short, but two things I verified only as far as my chunk allows: I did not walk settingsSchema.test.ts past diff line 746 (…

未审查:反向审计——评审时间预算不足,未能开始第 5 轮。

收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 8 条(原文未翻译,列表见上方英文部分)。

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

Comment thread packages/cli/src/config/settingsUtils.ts Outdated
Comment thread docs/design/2026-09-07-goal-no-progress-pause.md
Comment thread docs/design/2026-09-07-goal-no-progress-pause.md
Comment thread docs/users/configuration/settings.md Outdated
Comment thread docs/users/configuration/settings.md Outdated
Comment thread packages/core/src/skills/bundled/goal-draft/SKILL.md Outdated
Comment thread packages/core/src/goals/goal-tools.ts Outdated
Comment thread packages/core/src/goals/goal-runtime.test.ts
Comment thread packages/core/src/goals/goal-runtime.test.ts
Comment thread packages/core/src/goals/goal-runtime.ts
…y what the ceilings actually do

The `excludedValues` guard added last round compared with `Object.is`, which
tells `+0` from `-0`. `Number()` maps `-0`, `-0.0`, `-.0` and `-0e0` to `-0`,
`Number.isInteger(-0)` is true and `-0 < -1` is false, so `/config
model.goalMaxTurns=-0` walked past the guard, reported `Set ... = 0` because
`String(-0)` is `"0"`, and `JSON.stringify` persisted the excluded value.
Every later start in that scope then aborted on `must be > 0`, with `/config`
unreachable to repair it -- the same lockout the guard was added to prevent.
Compare numbers with `===` so signed zero matches, keeping identity for the
string half of the field's type.

The documentation of both ceilings overclaimed in three ways, each fixed
wherever the claim is published -- the two schema descriptions and their
generated mirror, the settings table, the user guide, both locales of the
Web Shell placeholder, three places in the goal-draft skill, and the
`propose_goal` objective example:

- `-1` was described as running Goals with no ceiling. It only removes a
  ceiling a Goal has already spent, on the resume or edit that follows; a Goal
  still under its ceiling keeps it.
- Neither ceiling is retrofitted onto a Goal already on the record, so the
  only way to bound one is to replace it. Nothing said so.
- The minutes row read as a wall-clock cap. There is no timer: the ceiling is
  read between turns, so a turn already running is never interrupted and the
  window can be exceeded by that turn plus the hand-off.

The design doc's "no behavioural change when off" bullet and its first
acceptance criterion were also too strong, in both languages. Restoring an
`active` Goal rebases `updatedAt` unconditionally, so the elapsed figure a
restored Goal reports drops offline time for everyone, settings or not -- and
drops with it any active time in an interrupted turn that no journal write had
committed. Both are now stated rather than implied, with the reason the record
cannot separate the two.

Two promises the tests did not hold:

- Three surfaces say user turns are still admitted at the ceiling, and the only
  case covering it admitted the turn one step below the limit. A case now
  reserves a user turn with the ceiling already spent and the hand-off in
  flight; adding a `spentBudget` gate to `beginTurn` reds it.
- The active-time ceiling is the only one whose spent state can change between
  queueing a continuation and delivering it. A case now queues under the
  ceiling with no host and binds past it, asserting the hand-off; making
  `bindHost` flush directly reds it.
@qqqys

qqqys commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Round 2 addressed in 060bd0e. The Critical and thirteen of the fifteen Suggestions are fixed; two are answered below rather than changed, one of them with a correction to its premise.

R2-1 (Critical) — signed zero walks past excludedValues. Confirmed end to end before fixing, not taken on trust:

Object.is(0,-0) = false    0 === -0 = true
Number('-0') = -0          Number.isInteger(-0) = true      -0 < -1 = false
String(-0) = '0'           JSON.stringify({v:-0}) = {"v":0}

Every link of your chain holds: the guard misses, the integer branch passes, the reply reads Set … = 0, the excluded value is persisted, and the next start aborts with /config unreachable. Fixed with the numeric comparison, keeping identity for the string half of the field's type. Your acceptance criterion is in: expect(validateSettingValue(definition, -0)).toBe('Value must not be 0') inside the existing loop, and the mutation you asked for — reverting to Object.is — reds it. settingsUtils.test.ts 65, config-command.test.ts 42, settingsSchema.test.ts 54 all pass.

R2-4, R2-5, R2-6, R2-7, R2-8, R2-9, R2-10, R2-11, R2-12, R2-13 — the documentation overclaimed in three ways. All ten are the same three defects published on nine surfaces, and all three are real; I checked each against the code that contradicts it. Fixed at every source, with the generated mirror regenerated rather than hand-edited:

claim what the code does
-1 "runs Goals with no turn ceiling" rearmedTurnBudget/rearmedActiveTimeBudget guard on !isGoalTurnBudgetSpent(current), so the opt-out only removes a ceiling already spent, on the resume or edit that follows
nothing said when a ceiling arms armedBudget stamps at create/replace only; a Goal on the record is never retrofitted, so bounding it means replacing it
minutes read as a wall-clock cap no timer exists; spentBudget is read at three sites and a running turn is never interrupted, so the window can be exceeded by that turn plus the hand-off

Surfaces updated: settingsSchema.ts ×2 → regenerated settings.schema.json, settings.md ×2, goals.md, i18n.tsx both locales, SKILL.md ×3, and the propose_goal objective example. Your acceptance criterion for the skill is in — SKILL.test.ts now pins 'takes effect after a restart and only for Goals created afterwards'.

R2-16 — the rebase discards the in-flight window, not only the offline gap. Correct, and I took the documentation option rather than a dispose-time write, for the reason you name: dispose() is synchronous while every journal write is awaited inside enqueue, so the commit would have to be fire-and-forget during process exit — unreliable exactly when it matters. Gating the rebase on activeTimeBudgetMs !== undefined is worse still: it would make one meter mean two things depending on whether a budget is armed.

So both halves are now stated rather than implied, in both languages: the elapsed figure a restored active Goal reports drops offline time for everyone including with the settings unset, and drops with it any active time in an interrupted turn no journal write had committed. The design doc's default-off bullet and its first acceptance criterion are narrowed to match, a second Constraints bullet explains why the record cannot separate offline time from the in-process tail without an exit timestamp, and the two user-facing sentences now say active time is counted between recorded transitions. Your severity call reads right to me: the tail is the smaller error and it fails safe — the ceiling under-counts rather than stopping a Goal for time nobody spent.

R2-14 and R2-15 — two promises nothing held. Both real, both fixed, both mutation-verified rather than assumed:

new case mutation that must red it result
a user turn reserved with the ceiling already spent and the hand-off in flight if (spentBudget(...)) return undefined; at the top of beginTurn red
a continuation queued under the time ceiling with no host, bound past it bindHost calling flushContinuation() instead of queueContinuation() red

R2-14's old case did admit the turn one step below the limit, as you said. The new one reserves it while turnCount >= turnBudget, so the documented promise is now held by a test that fails when it stops being true.

R2-3 — two statements the widened precedence left false. Both fixed in place. §Problem's "the token budget is the only bound" is now marked as the state at the time of writing and forward-points at the new bilingual pair; the "goals.* settings family is the subject of separate work" line now names where that work actually landed, model.goalMaxTurns and model.goalMaxActiveMinutes under category: 'Model', and why not goals.*.

R2-2 — the missing Chinese twin: recorded as a deferral, not done. The rule is real and I am not disputing it. My reasons for not translating the 199-line file in this PR: the substance of the widening is already documented in both languages in the new pair this PR ships, the English edits to that file are now two sentences of correction rather than new design, and it is one of roughly 401 English-only design docs — as you note, 4 of ~405 have twins, so this is a repo-wide documentation task rather than something this branch introduced. Translating it here would put an unrelated 199-line doc into a feature PR. Recorded here so it is not silently dropped, per AGENTS.md § Submitting PRs.

On the red web-shell E2E Smoke you could not attribute. It is not this PR. The failing case is web-shell.history-viewport.spec.ts:189:3 › global turn navigation preserves the reading row across bounded 200-record pages @smoke, and the identical case is failing on two other unrelated open PRs at different times — #11501 (job 102677836377, 23:10) and #11486 (job 102503215956, 14:27). Repo-wide, not attributable to this branch. The 17 red route entries are cancelled bot-orchestration jobs, not tests.

Suites after the change: packages/core/src/goals 19 files all green, SKILL.test.ts 12, and the three CLI settings suites 161. ESLint and Prettier clean on every touched file.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline.

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

  • R2-2 missing zh-CN twin for docs/design/2026-09-07-goal-no-progress-pause.md — already reported (comment 3972706646), author declined with reasons recorded in the thread

Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:

  • packages/core/src/skills/bundled/goal-draft/SKILL.test.ts:239 — [probe] Adjacent not.toContain('minutes') now passes only on the…
  • packages/core/src/goals/goal-runtime.test.ts:6545 — [probe] Third identical copy of the mark-delivered-then-finish…

Convergence: round 3 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 16 (16 new). Findings keep coming back to the same files: docs/users/features/goals.md (findings in round 2; 1 more now); packages/cli/src/config/settingsUtils.ts (findings in round 2; 1 more now); packages/core/src/skills/bundled/goal-draft/SKILL.md (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

已审查。 建议见行内评论。

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

收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 3 轮发布了 4 条行内评论,其中 4 条是首次提出;上一轮发布了 16 条(其中 16 条首次提出)。发现反复回到同一批文件:docs/users/features/goals.md(第 2 轮已出过发现,本轮又有 1 条);packages/cli/src/config/settingsUtils.ts(第 2 轮已出过发现,本轮又有 1 条);packages/core/src/skills/bundled/goal-draft/SKILL.md(第 2 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)

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

Comment thread docs/design/2026-09-09-goal-turn-and-time-budget.md
Comment thread docs/users/features/goals.md Outdated
Comment thread packages/cli/src/config/settingsUtils.ts Outdated
Comment thread packages/core/src/skills/bundled/goal-draft/SKILL.md Outdated

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline.

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

  • R2-2 missing zh-CN twin for docs/design/2026-09-07-goal-no-progress-pause.md — already reported (comment 3972706646), author declined with reasons recorded in the thread

Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:

  • packages/core/src/skills/bundled/goal-draft/SKILL.test.ts:239 — [probe] Adjacent not.toContain('minutes') now passes only on the…
  • packages/core/src/goals/goal-runtime.test.ts:6545 — [probe] Third identical copy of the mark-delivered-then-finish…

Convergence: round 3 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 16 (16 new). Findings keep coming back to the same files: docs/users/features/goals.md (findings in round 2; 1 more now); packages/cli/src/config/settingsUtils.ts (findings in round 2; 1 more now); packages/core/src/skills/bundled/goal-draft/SKILL.md (findings in round 2; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

已审查。 建议见行内评论。

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

收敛姿态下延后(第 3 轮,非阻断)——已记录,本轮不要求修改:共 2 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 3 轮发布了 4 条行内评论,其中 4 条是首次提出;上一轮发布了 16 条(其中 16 条首次提出)。发现反复回到同一批文件:docs/users/features/goals.md(第 2 轮已出过发现,本轮又有 1 条);packages/cli/src/config/settingsUtils.ts(第 2 轮已出过发现,本轮又有 1 条);packages/core/src/skills/bundled/goal-draft/SKILL.md(第 2 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)

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

Comment thread docs/design/2026-09-09-goal-turn-and-time-budget.md
Comment thread docs/users/features/goals.md Outdated
Comment thread packages/cli/src/config/settingsUtils.ts Outdated
Comment thread packages/core/src/skills/bundled/goal-draft/SKILL.md Outdated
…of objective text

Round-three review, four findings.

Replacing a Goal is the only way to bound one already on the record, and the
docs offered it without the cost. `/goal set` goes through `createGoal`: a new
Goal at revision 1, turn, token and active-time meters at zero, and an
evidence cursor at the replacement, so everything the old Goal recorded drops
out of the citable catalog. Said so on every surface that offers the remedy --
the user guide, both settings rows and both schema descriptions, regenerated.

The ceiling settings were pointed at from two opposite places: the goal-draft
template slot said "outside the objective", while its exemplar, the user
guide's Budget row and strong example, the `propose_goal` objective example
and the Web Shell placeholder all wrote the pointer into objective text -- text
re-sent every turn to a model that cannot act on it, advising a setting that by
its own wording never applies to the Goal carrying it. One placement now:
never in objective text. The rules-of-thumb bullet names the settings, the
user guide's Budget row says to set them in settings, and the Web Shell pointer
moves from the placeholder into the hint rendered under the same field.
`SKILL.test.ts` pins both halves -- the settings are named in the skill body
and absent from the template and the exemplar -- so they cannot drift apart.
The strong example also gains the `On block:` clause the template asks for.

The signed-zero guard is one `includes` call: SameValueZero already matches
`-0` against a `[0]` exclusion, so the typeof-split ternary carried an arm no
setting can reach. The `-0` assertion from last round still pins it.

The settings table rows are trimmed to fit the widest existing row, so the
table is not re-padded, and one shortening that would have misdescribed which
values are rejected is reverted to the accurate list.
@qqqys

qqqys commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Round three is addressed in 1afb934, with one reply per thread. On the two red checks at the previous head, 060bd0e, neither is caused by this PR.

Test (ubuntu-latest, Node 22.x) had two failures, both in src/serve/run-qwen-serve.test.ts › runQwenServe Web Shell signals on RunHandle: the loader env scrub cases.

The failure looks environment-dependent. The push re-runs it.

web-shell E2E Smoke failed on web-shell.history-viewport.spec.ts:189 › global turn navigation preserves the reading row across bounded 200-record pages. The same case fails on the unrelated #11501 and #11486, so it is repo-wide.

The sixteen cancelled route entries are bot orchestration jobs superseded by later runs, not tests.

@qqqys

qqqys commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Merged main into the branch in 6dc3f2d to clear the red Lint & Static at 1afb934.

The failing step was Check lint gate freshness, not a lint finding. The step fails when main has changed a lint gate file since the branch forked and the branch does not contain that change. main changed one gate file after the branch's base, .github/workflows/ci.yml, in #11522. The same step was green at 060bd0e, before that change landed.

The merge had no conflicts. Ten files had changed on both sides, so I checked the merged tree rather than trusting the auto-merge:

check result
npm run generate:settings-schema against the merged settingsSchema.ts identical to the auto-merged settings.schema.json
npm run check-i18n passes
core: src/goals and the goal-draft SKILL.test.ts 19 files, 610 passed
core: config.test.ts -t Goal 26 passed
cli: settingsSchema, settingsUtils, config-command, runBudget 248 passed
cli: config.test.ts -t goalMax 14 passed
web-shell: mappers.test.ts 65 passed

The merge also brings a new core export, so core had to be rebuilt locally before its package tests would run. CI builds from scratch and is unaffected.

@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. Suggestions are inline.

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

  • R2-2 missing zh-CN twin for docs/design/2026-09-07-goal-no-progress-pause.md — already reported (comment 3972706646); the author recorded a deferral per AGENTS.md § Submitting PRs with three reasons

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

Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": opening goal-runtime.test.ts and goal-continuation-prompt.test.ts to confirm each individual assertion the zh-CN 验证 bullets list actually exists (file exist…; "agent reverse-audit (round 2)": whether a persisted (pre-rebase) goal record can reach the client's getGoalActiveTimeMs before the recovered snapshot is published — I traced the client's ini…; "agent reverse-audit (round 2)": whether loadSettingsCached ( packages/cli/src/config/settings-cache.ts:161 , used by ACP newSession at acpAgent.ts:5026 ) invalidates on a settings.json ….

Not reviewed: reverse audit — stopped before round 4 by the review time budget.

Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:

  • packages/core/src/goals/goal-tools.ts:225 — [probe] get_goal pairs committed activeTimeMs with the ceiling
  • packages/core/src/goals/goal-runtime.test.ts:6950 — [probe] the no-progress stand-down's time leg has no witness
  • packages/core/src/goals/goal-protocol.ts:73 — [probe] the PR description quotes a lastReason the code never emits
  • docs/design/2026-09-09-goal-turn-and-time-budget.md:140 — [review] the design doc's Scope omits the get_goal widening
  • docs/design/2026-09-09-goal-turn-and-time-budget.md:190 — [probe] the rebase's blast-radius list is incomplete and mislabels two surfaces
  • packages/cli/src/config/config.ts:2308 — [probe] a workspace settings file can lift an operator's Goal ceiling
  • packages/core/src/config/config.ts:1074 — [probe] the core contract says in this process; the budget is whole-of-life
  • packages/core/src/goals/goal-continuation-prompt.ts:177 — [probe] the wind-down line claims the budget line says which ceiling
  • packages/sdk-typescript/src/daemon/types.ts:81 — [probe] the SDK documents the time ceiling against the committed figure
  • docs/design/2026-09-09-goal-turn-and-time-budget.md:143 — [probe] the sibling continuation-budget design doc is left stale with no pointer
  • packages/core/src/goals/goal-runtime.test.ts:6985 — [probe] the cadence settle-failure test cannot tell failure from success

Convergence: round 4 posted 5 inline comment(s), 5 of them reported for the first time; the previous round posted 4 (4 new). Findings keep coming back to the same files: docs/users/features/goals.md (findings in round 2; 1 more now); packages/core/src/skills/bundled/goal-draft/SKILL.md (findings in round 3; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

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

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未探索到全部深度(达到工具调用预算):"agent reverse-audit (round 3)"opening goal-runtime.test.ts and goal-continuation-prompt.test.ts to confirm each individual assertion the zh-CN 验证 bullets list actually exists (file exist…"agent reverse-audit (round 2)"whether a persisted (pre-rebase) goal record can reach the client's getGoalActiveTimeMs before the recovered snapshot is published — I traced the client's ini…"agent reverse-audit (round 2)"whether loadSettingsCached ( packages/cli/src/config/settings-cache.ts:161 , used by ACP newSession at acpAgent.ts:5026 ) invalidates on a settings.json …

未审查:反向审计——评审时间预算不足,未能开始第 4 轮。

收敛姿态下延后(第 4 轮,非阻断)——已记录,本轮不要求修改:共 11 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 4 轮发布了 5 条行内评论,其中 5 条是首次提出;上一轮发布了 4 条(其中 4 条首次提出)。发现反复回到同一批文件:docs/users/features/goals.md(第 2 轮已出过发现,本轮又有 1 条);packages/core/src/skills/bundled/goal-draft/SKILL.md(第 3 轮已出过发现,本轮又有 1 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,或将本 PR 的评审降到 --severity-floor critical,可以避免循环反复推导同一组发现。(仅为观察——本轮评审未因此扣留任何内容。)

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

Comment thread packages/core/src/skills/bundled/goal-draft/SKILL.md Outdated
Comment thread packages/cli/src/config/settingsSchema.ts Outdated
Comment thread packages/web-shell/client/i18n.tsx Outdated
Comment thread docs/users/configuration/settings.md Outdated
Comment thread docs/users/features/goals.md Outdated
Round-four review, five findings, all documentation precision. Each one is
fixed by narrowing a claim rather than qualifying it further, so the surface
the next round can find fault with gets smaller.

- Bounding a Goal already on the record: "replacing it with /goal set or
  clearing it, which starts a new Goal" said clearing starts a Goal. Clearing
  leaves none. Only the replacement starts a new Goal at revision 1; clearing
  is followed by starting again. Both settings rows and both schema
  descriptions now say so, matching the user guide.
- "Time while the process is not running does not count" overclaimed. The
  only exclusion is the rebase a restart performs; a suspended process, such
  as a sleeping laptop, is charged. The claim now says exactly that.
- The quantified overshoot, "that turn plus the hand-off turn", holds only
  when a turn is running as the window runs out. A window that runs out while
  no turn runs is not noticed until the next turn ends. The bound is dropped
  in favour of saying the Goal can run well past the window.
- The Web Shell hint sent readers to settings for two keys that client's
  settings surface cannot reach. It now names `/config` in the chat, which
  runs in daemon sessions.
- The second weak-to-strong exemplar still wrote a bare turn count into its
  Budget. It carries the advisory marker like the first, and the skill test now
  checks every exemplar row instead of one.
@qqqys

qqqys commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator Author

Round four is addressed in 9e734b1, with one reply per thread, and main is merged again in 3e3c6da.

Why main was merged again. The red Test (ubuntu-latest, Node 22.x) at 6dc3f2d was eight failures in WorkspaceSessionProvider.loading.test.tsx and WebShellSidebar.brand.test.tsx. #11244 broke those suites on main, and #11530 fixed them after my previous merge point. vitest had already retried each case twice, so a rerun could not have cleared it. The merge brings #11530 in and had no conflicts.

Round four. All five findings are documentation precision; the last two rounds found no code defect. Each finding was about a sentence an earlier round had added, and each such sentence is published on four copies — the settings row, the schema description, the generated schema and the user guide — so one imprecise claim turns into several findings. I fixed them by narrowing or removing the claims rather than adding qualifiers.

Verification on the merged tree:

suite result
core src/goals and SKILL.test.ts 19 files, 610 passed
core config.test.ts -t Goal 26 passed
cli settingsSchema, settingsUtils, config-command 162 passed
cli config.test.ts -t goalMax 14 passed
cli runBudget.test.ts 86 passed
web-shell WorkspaceSessionProvider.loading and WebShellSidebar.brand 16 passed
generate:settings-schema, check-i18n, Prettier, ESLint clean

The two web-shell suites failed locally at first only because the local sdk-typescript dist predated #11244, which added the GET /brand call to DaemonClient. After rebuilding that dist they pass. CI builds from scratch and is unaffected.

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review — head 9e734b163d

Scope: feat(goal), +2569/−95 across 39 files, centered on packages/core/src/goals/** and packages/core/src/config/**. Under the AGENTS.md two-tier gate this is a core-infrastructure feat — not size-blocked, but it escalates to a maintainer for awareness, which this review serves as.

This PR is at ~30 review rounds. Per AGENTS.md I am reporting Criticals only.

C=0, defended below.

Budget arithmetic

This is where a bug would actually hurt — a ceiling that re-arms wrong either strands a Goal that should continue or lets one run unbounded. I read goal-reducer.ts at this commit.

The representation choice is right. armedBudget at :491-498 refuses to persist a non-finite grant:

return grant !== undefined && Number.isFinite(grant)
  ? { [field]: grant }
  : {};

"Unbounded" is spelled as an absent field rather than Infinity, because Infinity does not survive the JSON journal — it would come back as null and then compare in ways nobody intended. Getting this wrong is a classic source of silently-unbounded runs, and it is handled.

Ceilings are stored as absolute values, not remaining allowances: rearmedTurnBudget at :566-577 writes current.turnCount + grant, and rearmedTokenBudget at :552-563 writes current.tokensUsed + grant. That means a resume cannot accidentally reset consumption to zero, which is the failure mode I was looking for.

rearmedActiveTimeBudget at :585-598 is the one with a genuine hazard — it needs to measure against the same elapsed figure the transition is about to commit, not a fresh clock read, or the new window starts at a value the record never held and the two disagree by the transition's own duration. It computes elapsedActiveTime(current, now) once and uses that same elapsed for both the spent check and the new ceiling. Correct.

Each ceiling re-arms independently (rearmedBudgets at :604-613), and an unspent one is left untouched, so a resume granted for the turn bound cannot silently widen the token bound. That is the right isolation property and it is stated explicitly in the comment.

Backward compatibility

isEvidenceLimited at :535-543 matches on limitKind by kind, and falls back to the lastReason prose only for records persisted before limitKind existed. A Goal from before budgets existed stays unbounded (:754). Both are correct readings of old journal entries rather than crashes or accidental re-interpretation.

Dead switches

I grepped read and write sites for the fields added to GoalRecord, GoalBudgetGrants, and the protocol surface in goal-protocol.ts. Each is populated by a real caller — no declared-and-read-but-never-set switch.

Bilingual design docs are present and linked (2026-09-09-goal-turn-and-time-budget.md / .zh-CN.md).

Limitations

I did not run the suite locally. CI on this head shows 25 passing, 1 pending, 25 skipping. Both Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) report skipping, so the only unit-test evidence is from the Linux lane. For this change that is a milder gap than usual — the logic is arithmetic and journal-shape handling rather than platform-dependent — but it is worth stating rather than reading 25 green checkmarks as cross-platform coverage.

@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. Suggestions are inline.

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

  • R2-2 missing zh-CN twin for docs/design/2026-09-07-goal-no-progress-pause.md — already reported (comment 3972706646), author deferral recorded in the thread
  • the wind-down line's budget-line referent at packages/core/src/goals/goal-continuation-prompt.ts:182 — already recorded in round 4's deferral list (review 5166733188)
  • get_goal pairs committed activeTimeMs with the ceiling at packages/core/src/goals/goal-tools.ts:225 — already recorded in round 4's deferral list (review 5166733188)
  • the no-progress stand-down's time leg has no witness at packages/core/src/goals/goal-runtime.test.ts:6950 — already recorded in round 4's deferral list (review 5166733188)
  • the sibling continuation-budget design doc is left stale at docs/design/2026-09-07-goal-continuation-budget.md:72 — already recorded in round 4's deferral list (review 5166733188)

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

Not explored to full depth (tool budget reached): "agent 1b": none — I did not run npm run generate:settings-schema to prove the committed packages/vscode-ide-companion/schemas/settings.schema.json is byte-fresh agains….

Not reviewed: reverse audit — stopped before round 4 by the review time budget.

Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:

  • docs/design/2026-09-09-goal-turn-and-time-budget.md:82 — [review] Design section misstates the pre-change resume branch as only clearing prose, in both language versions
  • packages/core/src/goals/goal-tools.ts:936 — [probe] propose_goal's objective exemplar gained the advisory marker with no test pin, while the skill's twin is pinned hard
  • scripts/generate-settings-schema.ts:205 — [probe] The generator's new excludedValues to JSON Schema not branch has no test anywhere
  • packages/core/src/skills/bundled/goal-draft/SKILL.md:83 — [probe] 'only for Goals created afterwards' omits the spent-ceiling resume/edit carve-out the two doc surfaces carry
  • packages/core/src/skills/bundled/goal-draft/SKILL.md:69 — [probe] The normative objective template's Budget slot still emits an unmarked turn count
  • docs/design/2026-09-09-goal-turn-and-time-budget.md:88 — [probe] 'Each budget re-arms independently' omits the coincidence case, where one resume moves every spent ceiling
  • docs/design/2026-09-09-goal-turn-and-time-budget.md:72 — [review] Design record still claims a one-turn overshoot bound; the polled time ceiling's overshoot is unbounded
  • packages/core/src/goals/goal-runtime.test.ts:6838 — [probe] No test resumes a Goal whose stop coincided across two ceilings
  • packages/core/src/goals/goal-runtime.test.ts:6784 — [probe] The windDown assertion passes vacuously if the resume mints no continuation
  • packages/core/src/skills/bundled/goal-draft/SKILL.md:83 — [review] The ceiling bullet omits the 'read between turns, not by a timer' caveat four other surfaces carry
  • packages/cli/src/ui/hooks/use-llm-stream.test.tsx:509 — [probe] Host wind-down fixtures pin a prompt the runtime cannot produce (windDown with no usage)

Convergence: round 5 posted 3 inline comment(s), 3 of them reported for the first time; the previous round posted 5 (5 new). Findings keep coming back to the same files: packages/web-shell/client/i18n.tsx (findings in round 4; 2 more now); docs/users/features/goals.md (findings in round 4; 1 more now). A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. (Observation only — nothing was withheld from this review because of this observation.)

中文说明

仅完成部分审查,审查缺口已披露。 建议见行内评论。

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

未审查(原文为英文):build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.

未探索到全部深度(达到工具调用预算):"agent 1b"none — I did not run npm run generate:settings-schema to prove the committed packages/vscode-ide-companion/schemas/settings.schema.json is byte-fresh agains…

未审查:反向审计——评审时间预算不足,未能开始第 4 轮。

收敛姿态下延后(第 5 轮,非阻断)——已记录,本轮不要求修改:共 11 条(原文未翻译,列表见上方英文部分)。

收敛情况:第 5 轮发布了 3 条行内评论,其中 3 条是首次提出;上一轮发布了 5 条(其中 5 条首次提出)。发现反复回到同一批文件:packages/web-shell/client/i18n.tsx(第 4 轮已出过发现,本轮又有 2 条);docs/users/features/goals.md(第 4 轮已出过发现,本轮又有 1 条)。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。(仅为观察——本轮评审未因此扣留任何内容。)

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

'goals.new': 'New goal',
'goals.newHint':
'The goal starts in a new session and runs until its condition holds.',
'The goal starts in a new session and runs until its condition holds. For an enforced turn or time ceiling, run /config model.goalMaxTurns=20 or /config model.goalMaxActiveMinutes=30 in the chat; it applies after the daemon restarts, to Goals created afterwards.',

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] R4-2: (fix-induced) The hint rewritten to close R4-2 now tells the reader the ceiling "applies after the daemon restarts", but the New-goal flow this hint is rendered in allocates a fresh session whose Config re-reads settings from disk — so the ceiling is already armed on the Goal the user creates next, and the restart the copy asks for drops every live session the daemon hosts.

A Web Shell user opens the Goals dialog (GoalsDialog.tsx:301 renders goals.newHint), runs /config model.goalMaxTurns=20 in the chat — which persists at User scope (config-command.ts:355) — then reads this hint and restarts qwen serve to make it apply. The restart is unnecessary: newSession loads settings per request through loadSettingsCached(cwd) (acpAgent.ts:5229), whose freshness fingerprint includes getUserSettingsPath() (settings-cache.ts:107-115), and builds a fresh Config via loadCliConfig (acpAgent.ts:14265), then resolveGoalMaxTurns (cli/config.ts:2308) and normalizeGoalMaxTurns (core/config.ts:2843); the dialog's create path allocates exactly such a session (createGoalForAllocatedSession, App.tsx:17896). A user who cannot restart instead concludes the ceiling cannot be armed and falls back to writing "stop as blocked after 20 turns" into the objective — the advisory-only prose this same diff demotes in goals.conditionPlaceholder. The ZH twin at line 5879 repeats the claim and needs the same edit.

Witness:

probe (same process, QWEN_HOME isolated to a temp dir; the two loadSettingsCached calls
are what acpAgent.ts:5229 does per session/new):
  session 1 settings read : model.goalMaxTurns = undefined -> grant Infinity
  on-disk user settings   : { "$version": 4, "model": { "goalMaxTurns": 20 } }
  session 2 settings read : model.goalMaxTurns = 20 -> grant 20
  cache served the stale instance? false
Suggested change
'The goal starts in a new session and runs until its condition holds. For an enforced turn or time ceiling, run /config model.goalMaxTurns=20 or /config model.goalMaxActiveMinutes=30 in the chat; it applies after the daemon restarts, to Goals created afterwards.',
'The goal starts in a new session and runs until its condition holds. For an enforced turn or time ceiling, run /config model.goalMaxTurns=20 or /config model.goalMaxActiveMinutes=30 in the chat; it applies to Goals created in a session started after the change — the goal you create here starts a new session, so it is armed on that one. A goal set in the session where you ran /config keeps the ceiling that session started with.',

The corrected copy must not claim the ceiling reaches a Goal set in the session where /config ran: the grant is captured once at Config construction (this.goalTurnBudgetGrant = normalizeGoalMaxTurns(params.goalMaxTurns), packages/core/src/config/config.ts:2843) and closed over by the runtime (const turnBudgetGrant = options.turnBudgetGrant ?? Number.POSITIVE_INFINITY, packages/core/src/goals/goal-runtime.ts:405) — which is also why requiresRestart: true on the schema entry stays correct for the running session. No test or snapshot pins goals.newHint in either locale, so there is no existing assertion this fix turns red; if you add one, pin the EN and ZH strings together.

中文说明

为关闭 R4-2 而重写的这条提示,现在告诉读者上限"daemon 重启后生效";但渲染这条提示的"新建目标"流程本身会分配一个全新会话,而新会话的 Config 会从磁盘重新读取设置——所以下一个创建的 Goal 已经带上了这道上限,而文案要求的那次重启会连带终止 daemon 上托管的每一个在线会话。

一位 Web Shell 用户打开 Goals 对话框(GoalsDialog.tsx:301 渲染 goals.newHint),在对话里执行 /config model.goalMaxTurns=20(由 config-command.ts:355 以 User 作用域落盘),然后读到这条提示,于是重启 qwen serve 让它生效。这次重启是多余的:newSession 每次请求都通过 loadSettingsCached(cwd) 读取设置(acpAgent.ts:5229),其新鲜度指纹包含 getUserSettingsPath()settings-cache.ts:107-115),随后经 loadCliConfigacpAgent.ts:14265)→ resolveGoalMaxTurnscli/config.ts:2308)→ normalizeGoalMaxTurnscore/config.ts:2843)构造出一个全新的 Config;而对话框的创建路径分配的正是这样一个会话(createGoalForAllocatedSessionApp.tsx:17896)。无法重启的用户则会得出"这道上限没法武装"的结论,转而在 objective 里写 "stop as blocked after 20 turns"——也就是本 diff 在 goals.conditionPlaceholder 里刚刚降级为"仅供参考"的那种写法。第 5879 行的中文副本重复了同一说法,需要同步修改。

约束:修正后的文案不得声称上限会作用于"在执行 /config 的那个会话里设置的 Goal"——grant 在 Config 构造时只捕获一次(this.goalTurnBudgetGrant = normalizeGoalMaxTurns(params.goalMaxTurns)packages/core/src/config/config.ts:2843),并被 runtime 闭包持有(const turnBudgetGrant = options.turnBudgetGrant ?? Number.POSITIVE_INFINITYpackages/core/src/goals/goal-runtime.ts:405);这也正是 schema 条目上的 requiresRestart: true 对当前会话依然成立的原因。两个语言版本都没有测试或快照钉住 goals.newHint,因此这项修复没有会变红的既有断言;若要补一条,请把中英文字符串一起钉住。

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

'goals.new': 'New goal',
'goals.newHint':
'The goal starts in a new session and runs until its condition holds.',
'The goal starts in a new session and runs until its condition holds. For an enforced turn or time ceiling, run /config model.goalMaxTurns=20 or /config model.goalMaxActiveMinutes=30 in the chat; it applies after the daemon restarts, to Goals created afterwards.',

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] R5-1: This hint sends users to /config with two integer-typed keys, but SETTABLE_TYPES (packages/cli/src/ui/commands/config-command.ts:26) omits 'integer', so both keys are hidden from every discovery surface /config has — the bare listing, key completion, and the "Did you mean" suggestion — even though the set path itself works.

A user reads the hint and types /config model.goalMaxTurn (one character short). findClosestKey skips integer definitions (config-command.ts:64), so the distance-1 match is filtered out and the reply is a bare Unknown setting key: "model.goalMaxTurn". with no suggestion. The same filter drops both keys from the bare /config listing (config-command.ts:221) and from completion (config-command.ts:404), so after setting the ceiling the user cannot see it among the other settings. In the Web Shell there is no argument completion at all, so this hint's exact spelling is the user's only guide and a typo dead-ends. These two strings are, per a repo-wide grep, the only product strings that tell a user to type /config <key>=<value>, and both name integer keys — which is what makes a pre-existing gap in an untouched file newly reachable through this diff.

Witness:

sweep over the real population (oracle: the repo's own getFlattenedSchema() plus the literal
SETTABLE_TYPES), driven against the built CLI:
  flattened keys total: 328 | by type: {"array":48,"boolean":89,"enum":22,"integer":12,
                                        "number":36,"object":71,"string":50}
  hidden from /config total: 131, of which integer: 12
  completion "model.goalMax"       -> []
  completion "model.maxWallTime"   -> ["model.maxWallTimeSeconds"]  <- a `number` sibling DOES complete
  action "model.goalMaxTurn"       -> error 'Unknown setting key: "model.goalMaxTurn".' (no "Did you mean")
  action "model.maxWallTimeSecond" -> error '... Did you mean "model.maxWallTimeSeconds"?'
  action "model.goalMaxTurns=20"   -> info 'Set model.goalMaxTurns = 20 / (This setting requires a restart to take effect.)'

The fix is one word in a file this diff does not touch, so a plain block rather than an applicable suggestion:

// packages/cli/src/ui/commands/config-command.ts:26
const SETTABLE_TYPES = new Set([
  'boolean',
  'string',
  'number',
  'integer',
  'enum',
]);

If integers are deliberately meant to stay out of /config, the alternative is to point this hint (and its ZH twin at line 5879) at settings.json instead — but that would undo the reachable-path remedy R4-2 asked for. Surfacing integers loosens no validation: coerceValue already validates them, and the measured boundaries hold — =0 gives Value must not be 0 (excludedValues: [0] at packages/cli/src/config/settingsSchema.ts:1692, enforced at packages/cli/src/config/settingsUtils.ts:252), =-2 gives Value must be >= -1, =1.5 gives Value must be an integer, =10001 gives Value must be <= 10000. packages/cli/src/ui/commands/config-command.test.ts has no integer case at all today; asserting there that the bare /config listing contains model.goalMaxTurns, and that /config model.goalMaxTurn returns the Did you mean "model.goalMaxTurns"? error, is the test that must go red if 'integer' is removed from SETTABLE_TYPES again — please add it and confirm it reds with the word taken back out.

中文说明

这条提示把用户引向 /config 去设置两个 integer 类型的键,但 SETTABLE_TYPESpackages/cli/src/ui/commands/config-command.ts:26)里没有 'integer',于是这两个键在 /config 所有的发现界面上都是隐形的——裸列表、键补全、以及 "Did you mean" 建议——尽管设置路径本身是通的。

用户照着提示输入 /config model.goalMaxTurn(少一个字符)。findClosestKey 会跳过 integer 类型的定义(config-command.ts:64),于是距离为 1 的那个匹配被过滤掉,回复只有一句 Unknown setting key: "model.goalMaxTurn".,没有任何建议。同一个过滤器也让这两个键从裸 /config 列表(config-command.ts:221)和补全(config-command.ts:404)里消失,所以用户设完上限之后,在其他设置中间看不到它。Web Shell 里完全没有参数补全,因此这条提示给出的拼写是用户唯一的依据,打错就走投无路。按全仓库 grep,这两处字符串是产品里唯一告诉用户去输入 /config <key>=<value> 的文案,而它们点名的都是 integer 键——这正是本 diff 让一个位于未改动文件里的既有缺口变得新可达的原因。

修法是在一个本 diff 未触及的文件里改一个词,所以用普通代码块而不是可一键应用的 suggestion。如果 integer 是有意不进入 /config 的,另一种做法是把这条提示(以及第 5879 行的中文副本)改为指向 settings.json——但那会撤销 R4-2 所要求的"点出一条可达路径"的修复。放开 integer 不会削弱任何校验:coerceValue 本来就会校验它们,实测边界均成立——=0 返回 Value must not be 0excludedValues: [0]packages/cli/src/config/settingsSchema.ts:1692,在 packages/cli/src/config/settingsUtils.ts:252 生效),=-2 返回 Value must be >= -1=1.5 返回 Value must be an integer=10001 返回 Value must be <= 10000packages/cli/src/ui/commands/config-command.test.ts 目前完全没有 integer 用例;在那里断言裸 /config 列表包含 model.goalMaxTurns、且 /config model.goalMaxTurn 返回 Did you mean "model.goalMaxTurns"? 错误,就是把 'integer' 再次从 SETTABLE_TYPES 移除时必须变红的测试——请补上它,并确认把这个词去掉后它确实变红。

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


Once a Goal has billed a turn, the footer pill and every status card show what it has spent against the window it is allowed, as `1.2k/30.0m`. The figure counts the model calls the Goal makes in its own turns; subagents and the verifier's own checks are not included. The window is set by [`model.goalTokenBudget`](../configuration/settings.md); resuming a Goal that has spent its window grants another one on top of what it has already spent, so the figure reads `30.0m/60.0m` rather than starting over. A Goal with no budget shows only what it has spent. A Goal that has not billed a turn yet shows no figures at all.

Two more windows are available and off by default: [`model.goalMaxTurns`](../configuration/settings.md) caps how many turns a Goal may finish, including user-driven Goal turns, and [`model.goalMaxActiveMinutes`](../configuration/settings.md) caps wall time while it remains active in a running process, including waits and idle time between turns. Time while the Goal is paused, blocked or stopped does not count against the second one, nor does downtime across a restart; a process that is merely suspended is still charged. Whichever window runs out first, the Goal gets the same single wind-down turn to hand off and then stops until you resume it, and resuming authorizes another window on top of what has already been used. Only the window that ran out moves; the others stay where they were, and the `-1` opt-out likewise only takes a ceiling off a Goal that has already spent it. The time window is read between turns rather than enforced by a timer, so a Goal can run well past it before it stops: a turn already running is never interrupted, and a window that runs out while no turn is running is not noticed until the next turn ends. Active time is counted between recorded transitions, so time in a turn a restart interrupted is not charged. Changes to either setting take effect after restart. Both windows arm only on a Goal created after the change, so bounding a Goal already on the record means replacing it with `/goal set` -- a new Goal at revision 1, with its turn, token and active-time meters reset and its evidence window starting at the replacement, so evidence the old Goal recorded is no longer citable -- or clearing it and starting again. A resume or an edit never adds a ceiling that was not armed at creation.

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] R5-2: The new paragraph tells the user their turns count toward model.goalMaxTurns but omits the admission caveat three of the four surfaces describing this setting carry — a user turn is still admitted at the ceiling, and it is what turns the next autonomous continuation into the wind-down.

A user sets model.goalMaxTurns: 20 and reads only this guide. At turn 20 they type a steering message, relying on line 33 of this same file ("Typing a message while a Goal is active does not pause it. Your message runs as the next Goal turn, so use it to steer the work"). promoteQueuedUserTurn (packages/core/src/goals/goal-runtime.ts:842-846) never consults spentBudget, so the message is admitted as turn 21; the following queueContinuation (goal-runtime.ts:738) sees the ceiling spent and grants the wind-down instead of more work, and the Goal settles usage_limited. Their steering buys one turn and then a hand-off, and only /goal resume authorizes another window — none of which the paragraph says, because its only mention of user turns is the counting clause and its stop clause reads "then stops until you resume it".

Witness:

sweep of the four prose surfaces describing goalMaxTurns x user turns:
  docs/users/configuration/settings.md:187        carries "user turns are still admitted at the
                                                  ceiling, but they can make the next autonomous
                                                  continuation a wind-down"
  packages/cli/src/config/settingsSchema.ts:1688  same clause
  packages/core/src/goals/goal-protocol.ts        turnBudget doc: "although those turns are not
                                                  rejected at the ceiling"
  docs/users/features/goals.md:21                 ABSENT   <- 3 of 4 carry it
behaviour pinned by this PR's own test 'still admits a user turn once the ceiling is
already spent' (packages/core/src/goals/goal-runtime.test.ts:6624)

Extend the counting clause to match the other three surfaces (a plain block, since the replacement sits inside a 1,738-character paragraph):

[`model.goalMaxTurns`](../configuration/settings.md) caps how many turns a Goal may finish, including user-driven Goal turns — those are still admitted once the ceiling is reached, but they make the next autonomous continuation the wind-down — and ...

The wording must keep the user turn admitted at the ceiling rather than refused: promoteQueuedUserTurn gates only on !nextTurnKey || currentPermit || snapshot.goal?.status !== 'active' and never calls spentBudget, and stopForSpentBudget's JSDoc (packages/core/src/goals/goal-runtime.ts:523-524) states "User-driven turns never pass through here and are never blocked by the budget." This is a documentation-only change, so there is no existing assertion it turns red.

中文说明

新增的这一段告诉用户他们的轮次会计入 model.goalMaxTurns,但漏掉了描述该设置的四处界面中有三处都带着的那条限定——用户轮次在上限已到之后仍会被放行,而正是它把下一次自主续跑变成交接轮。

一位用户设置 model.goalMaxTurns: 20 并且只读这份指南。到第 20 轮时他输入一条引导消息,依据的是同一文件第 33 行("Typing a message while a Goal is active does not pause it. Your message runs as the next Goal turn, so use it to steer the work")。promoteQueuedUserTurnpackages/core/src/goals/goal-runtime.ts:842-846)从不查询 spentBudget,所以这条消息作为第 21 轮被放行;紧接着 queueContinuationgoal-runtime.ts:738)发现上限已花完,于是发出交接轮而不是继续工作,Goal 结算为 usage_limited。他的引导只换来一轮,随后就是交接,而只有 /goal resume 才会再授一个窗口——这些这一段都没有说,因为它对用户轮次的唯一提及就是那句计数从句,而它的停机从句写的是 "then stops until you resume it"。

约束:措辞必须保持"用户轮次在上限处仍被放行",而不是被拒绝——promoteQueuedUserTurn 只判断 !nextTurnKey || currentPermit || snapshot.goal?.status !== 'active',从不调用 spentBudgetstopForSpentBudget 的 JSDoc(packages/core/src/goals/goal-runtime.ts:523-524)也写明 "User-driven turns never pass through here and are never blocked by the budget."。这是纯文档修改,因此没有会变红的既有断言。

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

@wenshao

wenshao commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

Local end-to-end verification — merge-ready

I built the branch and drove it as a real qwen serve daemon against a real Goal runtime at head 9e734b1, because the description lists the live end-to-end plan as not run on this branch. That gap is now closed on Linux: both ceilings stop a real Goal, the hand-off is granted exactly once per window, /goal resume moves only the ceiling that was actually spent, and with neither setting present nothing changes. Every claim below was re-checked against a control arm in which the corresponding line is disabled.

Verdict: no blocking finding. Four non-blocking items at the end; two of them are description accuracy rather than code.

How it was verified

Build the branch's own bundle — node esbuild.config.jsdist/cli.js (no npm ci; per-entry node_modules farm from a sibling worktree)
Runtime real qwen serve daemon, real qwen --acp child, real Goal runtime. Nothing in packages/core/src/goals is mocked.
Model a scripted OpenAI-compatible server on loopback (OPENAI_BASE_URL), one glob call + one text turn per Goal turn, so every turn records evidence and finishes
Driven through POST /session, POST /session/:id/goal, GET /session/:id/goal, GET /session/:id/events
Also exercised the real TUI (dist/cli.js --yolo, xterm screenshots), the real Web Shell served by the daemon, and the real client mapper replayed on a captured SSE frame
Settings written into a real ~/.qwen/settings.json in an isolated HOME

1. The turn ceiling, in the real product

model.goalMaxTurns: 3 → three ordinary turns, exactly one wind-down turn, then usage_limited / turn_budget. /goal resume moves the ceiling to turnCount + 3 and leaves the meter alone.

TUI, turn budget stop

TUI, resume re-arms

The second shot is the interesting half: after /goal resume the status reads Goal running · 4 turns — the count is not reset — three more turns run, and the Goal stops again at the moved ceiling.

2. The time ceiling, in the real product

model.goalMaxActiveMinutes: 1 → the Goal stopped at 63,925 ms of active time with limitKind: time_budget, and the resume re-armed from the elapsed figure it stopped at (63,925 + 60,000 = 123,925), not from zero and not from the wall clock.

TUI, time budget stop

3. The state ladder, straight off GET /session/:id/goal

state ladders

Pane C is the case the description calls out as the interesting one, and it holds: with both ceilings armed the turn ceiling fires, the resume moves turnBudget 3 → 7, and the unspent activeTimeBudgetMs stays at exactly 1800000.

4. What the model actually reads

Captured from the wire, not from a unit test's expectation. All three shapes of the rewritten budget line render correctly, including the singular 1 Goal turn finished, and the wind-down instruction is delivered exactly once per window.

continuation prompts

5. Negative controls

Nothing above is claimed on a green run alone. For each behaviour I built a control arm where the corresponding line is neutralised in the built bundle (or in mappers.ts) and re-ran the same harness.

negative controls

  • Restore rebase — the one thing that changes with both settings unset. Kill the daemon mid-Goal, wait ~50 s, restart, POST /session/:id/resume. Head charges 2,141 ms across a 52,310 ms outage; with recoveredSnapshot.goal.updatedAt = Date.now() disabled the same run charges 52,372 ms, i.e. the whole outage. The change does exactly what the description says, and the release note about a restored Goal reading lower is warranted.
  • The stopif (isGoalTurnBudgetSpent(goal))if (false && …): turnBudget: 3 is still stamped on the record, but the Goal runs to 40 turns and stops only at the pre-existing evidence_catalog bound. Identical to the no-ceiling control arm.
  • The re-armrearmedTurnBudget's guard forced to always return {}: the resume flips the Goal active, the ceiling never moves off 3, and it instantly re-stops with turnCount frozen at 4. This is the failure the re-arm exists to prevent, and it is reachable.
  • The Web Shell whitelist — I captured a real _meta.goalState SSE frame from the run in §1 and replayed it through the real client mapper. Head carries turnBudget: 7 and limitKind: "turn_budget" through; with the two whitelist hunks reverted both fields are silently dropped while every component test still passes. The description's R2 note about getGoalState is accurate and the fix is load-bearing.

6. Settings, validated twice and independently

The runtime validator and the checked-in editor JSON Schema agree on 16 of 16 values, including signed zero. scripts/generate-settings-schema.ts regenerates settings.schema.json byte-identical to what is checked in.

settings validation

The excludedValues"not": {"enum": [0]} work also lands on the /config path in the real TUI:

config validation

get_goal outside a permitted Goal turn reports all three ceilings, as claimed (pane C of the settings figure): "tokenBudget":30000000,"turnBudget":2,"activeTimeBudgetMs":1800000.

7. Test suites

Suite Result
packages/coresrc/goals src/config/config.test.ts 19 files / 1273 tests pass
packages/web-shellclient/daemon/session/mappers.test.ts 65 pass
packages/cli — whole package 1036 files, 30,225 pass, 10 fail

The 10 CLI failures are not this PR. I ran the whole package on the branch's own merge base (779cfe9, which is the exact main this branch merged) with the same farm and the same machine: base fails 13, and head's 10 are a strict subset of them. Six are chmod-based EACCES injection (cleanup, workspace-agents, workspace-memory, workspace-file-system, session-archive) which cannot fail for a root user; four are ink-cursor-rendering.test.tsx, a local ink artifact. Base's three extra are run-qwen-serve-live.test.ts discovery cases, which I most likely disturbed myself: a daemon of mine was bound to port 4188 on this host while that run was in flight. No file the PR touches fails on either arm.

8. Non-blocking observations

  1. The description points at a file that is not in the branch. "The end-to-end plan against a live model is written up under .qwen/e2e-tests/2026-09-09-goal-turn-and-time-budget.md" — that path does not exist at 9e734b1 (ls .qwen/e2e-tests/ has no goal entry). Either add the file or reword to "planned but not written up".
  2. A stale string in the description's own evidence block. It quotes lastReason as "The Goal ran its autonomous turn budget (2 turns)"; the string the code ships, and the one a real run produces, is "The Goal ran its Goal-turn budget (2 turns)". Worth a one-line fix so the PR body matches the merged behaviour.
  3. renderActiveMinutes rounds up to the ceiling before the ceiling is reached. With maximumFractionDigits: 1, a Goal at 57.9 s of a 60 s window prints 1 of 1 active minutes used on an ordinary turn, and the next turn prints the identical figures as the wind-down. Two consecutive turns show the same "spent" reading with different meaning, and a model told 1 of 1 has a fair reason to start winding down a turn early. Two decimals, or flooring the elapsed side, would remove the ambiguity. (Visible in §4 pane B, turn 19 vs turn 20.)
  4. No display surface carries the new ceilings yet — confirmed, as disclosed. The TUI pill, the /goal status card and the Web Shell Goals card all still show only the token pair (9.6k/30.0m), so the ceiling that actually stopped the Goal reaches the user only through lastReason prose. That matches what the PR lists as follow-up; I am recording it because a user who sets goalMaxTurns and then watches the footer gets no signal until the stop.

Web Shell Goals page

One code-reading note that is not a finding: armedBudget treats a finite 0 grant as a real ceiling, so a direct createGoalRuntime({ turnBudgetGrant: 0 }) would arm a budget that is spent at creation. It is unreachable through Config (normalizeGoalMaxTurns maps 0Infinity) and the CLI rejects 0 at startup, and it is the same shape tokenBudget already had, so it changes nothing — but grant > 0 would close it for free.

What I did not verify

Linux only; I did not run macOS or Windows (the change is pure TypeScript logic with no path, fs or process handling, so I would not expect platform divergence, but the ⚠️ rows stand). I did not run a real provider — the model is scripted. And I did not independently drive a user turn into a Goal that is under its turn ceiling; that "user-driven turns count too" behaviour is covered by the branch's own unit test and stated in the setting description, but it is not in my end-to-end evidence.

中文版

本地端到端验证 —— 可以合入

我构建了这个分支,并在 head 9e734b1 上以真实的 qwen serve daemon + 真实 Goal 运行时跑了一遍,因为 PR 描述里写着针对真实模型的端到端计划"尚未在本分支上跑过"。这个缺口现在在 Linux 上补上了:两道上限都能真的把 Goal 停下,交接轮每个窗口恰好授予一次,/goal resume 只前移真正花完的那道上限,两个设置都不设时什么也不变。下面每一条结论都对照了一个"把对应那行禁用掉"的对照臂。

结论:无阻塞问题。 末尾有 4 条非阻塞项,其中两条是描述准确性而非代码。

验证方式

构建 分支自带的 bundle —— node esbuild.config.jsdist/cli.js(不跑 npm ci,用兄弟 worktree 的逐条 node_modules symlink)
运行时 真实 qwen serve daemon、真实 qwen --acp 子进程、真实 Goal 运行时。packages/core/src/goals 里没有任何东西被 mock。
模型 loopback 上的脚本化 OpenAI 兼容服务(OPENAI_BASE_URL),每个 Goal 轮一次 glob 调用 + 一次文本轮,保证每轮都记录证据并结束
驱动接口 POST /sessionPOST /session/:id/goalGET /session/:id/goalGET /session/:id/events
另外覆盖 真实 TUI(dist/cli.js --yolo + xterm 截图)、daemon 自己提供的真实 Web Shell、以及把抓到的真实 SSE 帧回放进真实客户端 mapper
设置 写进隔离 HOME 下真实的 ~/.qwen/settings.json

1. 轮数上限(真实产品里)

model.goalMaxTurns: 3 → 三个普通轮、恰好一个交接轮,然后 usage_limited / turn_budget/goal resume 把上限前移到 turnCount + 3,计量不清零。

第二张截图是更关键的一半:/goal resume 之后状态显示 Goal running · 4 turns——计数没有被重置——再跑三轮,Goal 在移动后的上限处再次停下。

2. 时长上限(真实产品里)

model.goalMaxActiveMinutes: 1 → Goal 在活跃时长 63,925 ms 处停下,limitKind: time_budget;resume 从它停下时的已耗时长重新武装(63,925 + 60,000 = 123,925),既不是从零开始,也不是按墙钟。

3. 状态阶梯(直接来自 GET /session/:id/goal

图中 C 面板正是描述点名的"最值得看"的用例,结论成立:两道上限都武装时,轮数上限触发,resume 把 turnBudget 从 3 移到 7,而未花完activeTimeBudgetMs 原封不动停在 1800000

4. 模型真正读到的东西

这些是从线上抓的,不是单测里的期望值。重写后的预算行三种形态都渲染正确,包括单数的 1 Goal turn finished,而交接指令每个窗口恰好下发一次

5. 反向对照

上面的结论没有一条只靠"跑绿了"。每一项行为我都做了一个把对应代码行在构建产物(或 mappers.ts)里禁用掉的对照臂,再跑同一套 harness。

  • 恢复时重置时钟 —— 这是两个设置都不设时唯一会变的东西。Goal 跑到一半 kill 掉 daemon,等 ~50 s,重启,POST /session/:id/resume。Head 在 52,310 ms 的停机上只计入 2,141 ms;把 recoveredSnapshot.goal.updatedAt = Date.now() 禁用后,同样的跑法计入 52,372 ms,也就是整段停机。改动确实做到了描述所说的事,发布说明里"被恢复的 Goal 读数更低"这句是有必要的。
  • 停机判定 —— if (isGoalTurnBudgetSpent(goal))if (false && …)turnBudget: 3 仍然写在记录上,但 Goal 一路跑到 40 轮,只在既有的 evidence_catalog 界限处停下,与"无上限"对照臂完全一致。
  • 重新武装 —— 把 rearmedTurnBudget 的判断强制成永远返回 {}:resume 把 Goal 翻成 active,上限始终停在 3,随即立刻再次停下,turnCount 冻结在 4。这正是重新武装这段代码要防的故障,而且它是可达的。
  • Web Shell 白名单 —— 我把 §1 那次真实运行中抓到的 _meta.goalState SSE 帧回放进真实客户端 mapper。Head 能把 turnBudget: 7limitKind: "turn_budget" 带过去;把两处白名单改动回退后两个字段都被悄悄丢掉,而所有组件测试照样通过。描述里关于 getGoalState 的 R2 说明是准确的,这个修复是有承重作用的。

6. 设置:两条独立路径各验一遍

运行时校验器与仓库里checked-in 的编辑器 JSON Schema 在 16/16 个取值上判断一致,包括负零。scripts/generate-settings-schema.ts 重新生成的 settings.schema.json 与仓库里的逐字节相同。

excludedValues"not": {"enum": [0]} 这套东西在真实 TUI 的 /config 路径上也生效。

get_goal 在非许可 Goal 轮之外确实报出全部三道上限(设置图 C 面板):"tokenBudget":30000000,"turnBudget":2,"activeTimeBudgetMs":1800000

7. 测试套件

套件 结果
packages/core —— src/goals src/config/config.test.ts 19 文件 / 1273 用例全通过
packages/web-shell —— client/daemon/session/mappers.test.ts 65 通过
packages/cli —— 整包 1036 文件,30,225 通过,10 失败

这 10 个 CLI 失败与本 PR 无关。 我在这个分支自己的 merge base(779cfe9,正是它合进来的那个 main)上,用同一套依赖、同一台机器跑了整包:base 失败 13 个,而 head 的 10 个是它的真子集。其中 6 个靠 chmod 注入 EACCES(cleanupworkspace-agentsworkspace-memoryworkspace-file-systemsession-archive),对 root 用户不可能失败;4 个是 ink-cursor-rendering.test.tsx,是本地 ink 的产物。base 多出的 3 个是 run-qwen-serve-live.test.ts 的发现用例,很可能是我自己干扰的:那次跑的时候我有一个 daemon 绑在本机 4188 端口上。两个臂上都没有本 PR 触及的文件失败。

8. 非阻塞观察

  1. 描述指向了一个不在分支里的文件。 "The end-to-end plan against a live model is written up under .qwen/e2e-tests/2026-09-09-goal-turn-and-time-budget.md" —— 该路径在 9e734b1 上并不存在(ls .qwen/e2e-tests/ 里没有 goal 相关条目)。要么补上文件,要么改成"已规划但尚未写出"。
  2. 描述自己的证据块里有一处过期字符串。 里面把 lastReason 写成 "The Goal ran its autonomous turn budget (2 turns)";代码实际发出、真实运行也确实产出的是 "The Goal ran its Goal-turn budget (2 turns)"。值得改一行,让 PR 正文与合入行为一致。
  3. renderActiveMinutes 会在到达上限之前就四舍五入到上限。 maximumFractionDigits: 1 之下,60 s 窗口里跑到 57.9 s 的 Goal 会在一个普通轮上打印 1 of 1 active minutes used,而下一轮的交接轮打印的数字一模一样。连续两轮显示同样的"已用尽"读数却含义不同,模型看到 1 of 1 有充分理由提前一轮开始收尾。保留两位小数、或对已耗侧向下取整,就能消除这个歧义。(见 §4 的 B 面板,第 19 轮与第 20 轮。)
  4. 确实还没有任何展示界面显示这两道新上限——与描述披露的一致。 底栏 pill、/goal 状态卡片、Web Shell 的 Goals 卡片都仍然只显示 token 那一对(9.6k/30.0m),所以真正让 Goal 停下的那道上限只能通过 lastReason 的文案传达给用户。这与 PR 里列的后续工作一致;我记下来是因为:一个设置了 goalMaxTurns 然后盯着底栏看的用户,在停下之前得不到任何信号。

未验证的部分

只跑了 Linux,没跑 macOS 和 Windows(改动是纯 TypeScript 逻辑,不涉及路径、文件系统或进程处理,我不预期平台差异,但那两行 ⚠️ 仍然成立)。没有接真实 provider——模型是脚本化的。另外我没有独立地在"尚未用完轮数上限"的 Goal 上驱动一个用户轮;"用户驱动的轮次也计入"这条行为由分支自带的单测覆盖、设置说明里也写了,但不在我的端到端证据里。


Verified locally with Claude Code (Opus 5, 1M context). Assets: wenshao/qwen-code@assets-pr11457.

@wenshao

wenshao commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

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: 209 passed · 0 failed · 209 total

Flakiness gate: ⚠️ timeout — only 3 of 5 rounds fit the 15-minute budget; the completed rounds agreed

中文 — 判定:✅ 通过 · 可合入(agent 判定)

沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码(与 base 构建 A/B 对照、无 mock harness 断言、定向门禁)。仅作为评审证据,不构成评审、批准或 CI 检查

脚本断言:209 通过 · 0 失败 · 209 总计

抖动门:⚠️ timeout — only 3 of 5 rounds fit the 15-minute budget; the completed rounds agreed

Verification report

PR 11457 — deep verification

Verdict: merge-ready · assertions 209 pass / 0 fail / 209 total · verified head 9e734b163da836b318182cdf2990cf71c9840987 (HEAD^2) against base 779cfe913bd1dd8063cc822fecd5128ccacc2769 (HEAD^1 = the snapshot's baseRefOid, so the merge-ref contract holds).

The central claim is load-bearing: a turn ceiling and an active-time ceiling each stop a Goal through one wind-down turn, and neither exists on base. The one change that applies with both settings unset — the restore rebase — is A/B'd, is pinned by a named test, and moves the reported elapsed figure from 600001 ms to 0 ms for a Goal restored after ten minutes of downtime. No blocking finding. Four non-blocking observations are listed under Findings; three of them are pre-existing patterns or cosmetic, and the fourth is an unnamed read site of the rebased field whose direction I could not make wrong.

中文摘要

结论:merge-ready,脚本化断言 209 通过 / 0 失败。验证头提交 9e734b16,基线 779cfe91

A/B 结论:中心主张成立。轮数上限与活跃时长上限各自都能让 Goal 经过恰好一个 wind-down 轮后停下(limitKind 分别为 turn_budget / time_budget),而基线构建对同样的 grant 完全无反应——不武装任何字段、不打 wind-down 标志、继续无限续跑。两者都不设时,头与基线行为一致(不武装任何节奏字段)。唯一在默认路径上生效的变化是恢复时重置时钟:同样停机 10 分钟后恢复,基线报出 elapsed=600001,头报出 elapsed=0,并且下一次落账的记录也不会把停机时间重新计入。详见下文 A/B 单元表01-ab-turn-time-budget-head-vs-base.png

Findings(均非阻塞)/config 的补全白名单不含 integer 类型,因此两个新设置不会被 tab 补全或拼写纠错建议——但基线已有 16 个 integer 设置同样如此,属既有问题,且 /config model.goalMaxTurns=20 本身可用(已由 config-command.test.ts 42/42 证实);恢复时重置的 updatedAt 还会流入 Web Shell 帧序比较这一未被 PR 说明点名的读取点,方向经分析为良性、我未能构造出错误结果;该比较本身有充分测试覆盖,未被钉住的是跨包耦合——mapper 测试手工构造快照,看不到 core 侧写入的值;停机原因文案在 60–150 秒区间会把 90 秒说成 "2 minutes";直接嵌入方传入负的 grant 会被原样盖章,而恢复时校验会拒绝负值——与既有 tokenBudget 行为完全同形。

未覆盖范围:逐提交归因不可达(浅克隆本地只有 1 个提交,元数据列 9 个);未跑真实模型的端到端(PR 自己也标注未跑);未跑仓库级 lint/typecheck/全量测试;展示层(底栏 pill、状态卡片、headless Usage: 行、Web Shell 状态条)不显示两道新上限,PR 已声明为后续工作。

Scope selection

Central claim. A Goal can be stopped by a turn ceiling (model.goalMaxTurns) or an active-time ceiling (model.goalMaxActiveMinutes), taking the path a spent token budget already takes: exactly one wind-down turn, then settle as usage_limited with a new limitKind, and /goal resume re-arms only the ceiling that ran out.

Secondary claims chosen for budget. (1) With neither setting armed, the only behaviour change is the restore rebase — the elapsed figure no longer counts process downtime. This is the highest-risk claim in the PR because it is the one that reaches users who opted into nothing. (2) Ceilings are read at the continuation boundary, never mid-turn, and a turn crossing several ceilings reports one reason in the order token → turn → time.

Out of scope, listed under Not covered. Display surfaces for the two new ceilings, live-model E2E, repo-wide gates, per-commit attribution.

Central claim — A/B

Control build: git worktree add tmp/base-tree HEAD^1, then packages/core rebuilt inside it (node ../../scripts/build_package.js with the root node_modules/.bin on PATH). The PR touches no package.json or lockfile, so the dependency tree is identical on both arms and sharing the root node_modules is a clean control rather than a confound.

Two contamination checks were run before any cell, because the root node_modules/@qwen-code/* entries are symlinks into the head tree (readlink -f asserted: @qwen-code/qwen-code-core -> /__w/qwen-code/qwen-code/packages/core). The harness therefore imports base by absolute path into the base tree's own dist/, and the four goals modules under test import nothing from @qwen-code/* — only @google/genai and node:crypto — so no arm crosses a workspace boundary. Identity was then asserted on the compiled output: base dist/src/goals/ contains 0 occurrences of turnBudgetGrant, spentBudget, isGoalTurnBudgetSpent, activeTimeBudgetMs, goalTurnBudgetReason (head: 24/5/9/57/10), contains all five pre-existing symbols it must (createGoalRuntime, reduceGoalControl, elapsedActiveTime, isGoalTokenBudgetSpent, goalTokenBudgetReason), and has the same 36 .js files as head. Each harness run re-asserts this per arm and aborts if the arms are swapped or contaminated.

Both arms are in 01-ab-turn-time-budget-head-vs-base.png; raw per-cell output is logs/ab-run.log.

cell arm scenario oracle result
A head turnBudgetGrant: 2, drive real continuations final status / limitKind / turnCount / windDownTurnId, journal cause sequence, continuation count usage_limited / turn_budget / 3 / set; causes create, turn_finished×3, usage_limited; no 4th continuation
A head wind-down shape host.inputs[2].windDown true, and turns 1–2 carry no such flag
A head resume after the stop ceiling vs meter turnBudget 2 → 5, turnCount stays 3, limitKind/windDownTurnId/lastReason cleared
A base same grant same oracles control: no turnBudget field armed, 5 continuations run, never usage_limited, no wind-down flag
B head activeTimeBudgetGrantMs: 120, real wall-clock sleeps (no clock fakery) limitKind, activeTimeMs ≥ budget at stop, resume time_budget; resume moves the ceiling to elapsed + grant (±250 ms)
B head mid-turn immunity continuations started while one is in flight; flag on the in-flight turn no new turn started, in-flight turn not retro-flagged, wind-down lands on the next grant
B base same grant, same sleeps same oracles control: no activeTimeBudgetMs armed, keeps granting continuations past the elapsed window, no wind-down
C both neither grant set (the default) presence of the two fields; 4 continuations parity: neither arm arms a cadence field; token budget still armed; stays active; no wind-down
D head token and turn crossed by one turn (real ledger spend) limitKind token_budget — token wins the tie
E head two ceilings armed, only turns spent both ceilings after resume spent turn ceiling moves (1 → turnCount+1); unspent time ceiling byte-identical (3600000); no token ceiling retrofitted
F head restore an active Goal whose persisted updatedAt is 600 000 ms old elapsedActiveTime(goal, now) 0 — downtime excluded; committed activeTimeMs not rewritten; the next persisted record also excludes it
F base identical records identical oracle control: 600001 — downtime charged; updatedAt left as persisted
G head restore a Goal 55 s into a 60 s time ceiling after 10 min downtime elapsed vs budget, status 55000 < 60000, still active — offline time is not charged against the new ceiling

Cell F is the load-bearing pair for the default-path claim: same real journal records produced by a real runtime, aged identically, restored into a fresh runtime, and the two arms report 0 versus 600001. Cells D, E and G have no base arm because base arms no cadence ceiling at all — recorded as skipped in the harness output rather than silently dropped.

Two ordering siblings the description does not name were driven separately (04-m11-adjudication-and-ceiling-ordering.png, logs/followup.log): turn outranks time when one turn crosses both (limitKind: turn_budget, exactly one reason string, no mention of active time), and when all three cross, token still wins. The same run showed the resume then re-arms both spent ceilings (turnBudget 1 → 3, activeTimeBudgetMs 60 → 211), which is what rearmedBudgets' per-budget independence implies.

The prompt the model actually reads

The description quotes two budget lines. Both were rendered from the compiled head module and compared verbatim (02-prompt-text-and-settings-ladder.png):

Budget: 1,234 of 30,000,000 tokens used, 29,998,766 remaining; 4 of 20 Goal turns finished.
Budget: 900 tokens used, with no token budget on this Goal; 2 Goal turns finished; 12.3 of 30 active minutes used.

Base renders Token budget: 1,234 of 30,000,000 tokens used, 29,998,766 remaining; 4 Goal turns finished. — the prefix rename and the turn segment are both attributable to this PR. "A Goal with no time ceiling reports no minutes at all" holds in both directions tested: no minutes without a ceiling, and no minutes when only an elapsed figure is supplied without one.

The wind-down rewrite says "the budget line above says which". That is a structural claim, so it was checked structurally: in the rendered prompt the budget line is index 11 and the wind-down line index 12, and the wind-down turn still carries the figures it refers the model to (3 of 2 Goal turns finished). Base's wind-down still names the token budget specifically.

Settings and the knob trace

Both startup rejection messages quoted in the description were reproduced verbatim from the compiled CLI validators, including the settings.json: wrapper the CLI adds:

settings.json: model.goalMaxTurns must be > 0 (or -1 for no ceiling); got 0. Use -1 to disable, not 0.
settings.json: model.goalMaxActiveMinutes 10081 exceeds the supported ceiling (10080 minutes). Use a smaller value or -1 for no ceiling.

A 20-value ladder was run against each validator: -1, 1, 20, cap-exact, 0, -0, -2, 1.5, cap+1, NaN, ±Infinity, "20", null, true, [20], {v:20}, 2**53. Every accept/reject matched, and accepted values round-trip unchanged. Signed zero is rejected by both paths — the startup validator via value <= 0, and the /settings API via excludedValues ([0].includes(-0) is true under SameValueZero, as the code comment claims).

The knob was then traced to an observable effect rather than assumed: validateGoalMaxTurns(20) → 20normalizeGoalMaxTurns(20) → 20 → a real Config built with goalMaxTurns: 20 returns getGoalTurnBudgetGrant() === 20 (30 minutes → 1800000 ms) → and cell A proves createGoalRuntime({turnBudgetGrant}) arms the field and stops. Unset, -1, 0, over-cap, fractional, string and negative all resolve to Infinity, i.e. no ceiling; the token budget's numeric default is unchanged. The single link shown by inspection rather than execution is config.ts passing turnBudgetGrant: this.goalTurnBudgetGrant into createGoalRuntime — one line, read in the diff.

Corrections

  • The CLI package is not green in this container, and the test plan does not say so. cd packages/cli && npx vitest run reports 13 failed / 30229 passed / 103 skipped (30345), all 13 in src/config/settings.test.ts. This is not caused by the PR — see the A/A attribution under Findings — but a reviewer following the plan literally will see red and needs the attribution up front. The other two commands in the plan are green as written.
  • The plan's own guidance is correct and worth keeping. "Run the CLI package whole rather than by file" is load-bearing: the PR's text rewrites are pinned verbatim by assertions in three CLI suites, and the whole-package run is what surfaced the pre-existing settings.test.ts failures that a filtered run would have hidden. No step of the Reviewer Test Plan was unperformable.
  • The description's account of the rebase is accurate but incomplete about its reach. It scopes the change to "the elapsed figure the footer pill, get_goal and the legacy durationMs projection". The rebased updatedAt also feeds a Web Shell frame-ordering comparison (Finding 2). This is a correction to the description's blast radius, not a request to change the code.

Findings

None blocking. Ordered by how much a reviewer should care.

1. integer settings are absent from /config completion and typo suggestions — pre-existing, not introduced here

packages/cli/src/ui/commands/config-command.ts:26 declares SETTABLE_TYPES = new Set(['boolean', 'string', 'number', 'enum']), which omits 'integer' — the type of both new settings. That set is consulted by completion() and by findClosestKey(), but not by action(), and coerceValue() does handle case 'integer'. So the instruction the PR adds to the Web Shell hint works: /config model.goalMaxTurns=20 succeeds. config-command.test.ts passes 42/42 including the new "shows restart warning for Goal cadence settings" case, which asserts setValues is called with {scope: 'User', key: 'model.goalMaxTurns', value: 20}.

What does not work is discovery: the key is never offered by tab-completion and never suggested when mistyped. This is pre-existing — base already had 16 integer settings (goalTokenBudget, maxSessionTurns, maxToolCallsPerTurn, …); the PR takes the count to 18 and inherits the gap rather than creating it. Named separately so the author is not asked to fix a 16-setting-wide issue in this diff. If it is worth fixing, adding 'integer' to SETTABLE_TYPES is the whole change, and it belongs in its own PR.

2. The rebased updatedAt reaches a read site the description does not name, and the coupling is unpinned

packages/web-shell/client/daemon/session/mappers.ts:498 breaks ties between a held snapshot and an incoming "goal cleared" frame with current.goal.updatedAt > clearedGoal.updatedAt, consulted only when goalId matches and revision is equal. The restore rebase sets updatedAt = Date.now(), so it moves exactly the value that comparison reads.

The direction is benign and I could not make it wrong. The comparison exists to ignore stale out-of-order frames, so preferring the fresher frame is its purpose; and the interleaving that would matter — a clear that predates the restore beating a live Goal — cannot arise, because recovery walks the records backwards and returns the newest parseable goal_state payload, so a Goal cleared before the restart is not restored as active in the first place.

The comparison itself is well covered: mappers.test.ts references selectGoalState 19 times and drives tombstone cases with explicit updatedAt values (e.g. clearedGoal: { goalId: 'goal-new', revision: 2, updatedAt: 30 }), which is why mutants M13/M14 died there. What is unpinned is the cross-package coupling, not the comparison: those tests hand-build the snapshots they compare, so they cannot observe what the core-side rebase writes into updatedAt, and no test on either side asserts that a rebased value is a legal input to that tiebreak. A test that restores an active Goal through the real runtime and then feeds the resulting snapshot a stale cleared frame would close it. Reported as an unnamed reach of the rebase, not as a defect — the description scopes the change to the elapsed figure, and this is a second consumer of the same field.

3. The stop reason rounds minutes, so a 90-second ceiling reports "2 minutes"

formatGoalActiveTimeBudget uses Math.round(minutes) above one minute. Measured from the compiled module: 90000 ms → "(2 minutes)", 59999 ms → "(60 seconds)", 1843000 ms → "(31 minutes)". Unreachable through the setting, which takes whole minutes ≥ 1; reachable only for a direct activeTimeBudgetGrantMs embedder in the 60–150 s band, and for post-resume ceilings (which is arguably correct — the ceiling really is 31 minutes by then, and tokenBudget reports its absolute ceiling the same way). Cosmetic; the sub-minute branch itself is pinned (mutant M12 killed a test).

4. A negative grant handed straight to the runtime is stamped verbatim, then rejected on restore

armedBudget gates on Number.isFinite only, so createGoalRuntime({turnBudgetGrant: -5}) produces a record with turnBudget: -5, while parseGoalRecord validates isNonNegativeInteger(turnBudget) and would refuse to restore it — a Goal that exists until the first restart. Not reachable through any in-repo caller: the CLI validator rejects negatives and normalizeGoalMaxTurns maps them to Infinity. This is the pre-existing tokenBudget behaviour faithfully extended (base's createGoal had the same Number.isFinite gate, and parseGoalRecord already rejected a negative tokenBudget), so it is parity rather than a regression. Recorded because the boundary was probed, not because it needs fixing here.

The same probe established the safety property that matters more: a Goal born spent (turnBudget: 0, reachable only the same way) gets its hand-off immediately and then stops as turn_budget — no infinite continuation loop, no silent unbounded run — and a zero ceiling survives the journal round-trip. Non-finite grants (±Infinity, NaN) arm neither ceiling. See 05-boundary-born-spent-and-grant-edges.png.

Mutation matrix and vacuity

16 single-point mutants of the production files this PR changed, each run against the suite that should pin it, each reverted afterwards (git status clean confirmed by the runner). 03-mutation-matrix.png, logs/mutation-run.log.

mutant guard under test suite result
M0 (control) off-by-one in isGoalTurnBudgetSpent core src/goals killed, 12 tests red
M1 spentBudget ordering (turn checked first) core src/goals killed, 18 red
M2 runtime turn-ceiling stop removed core src/goals killed, 6 red
M3 runtime time-ceiling stop removed core src/goals killed, 2 red
M4 resume branch reverted to === 'token_budget' — the exact regression the description says it fixed core src/goals killed, 4 red
M5 turn ceiling re-armed even when unspent core src/goals killed, 2 red
M6 time ceiling re-armed even when unspent core src/goals killed, 3 red
M7 armedBudget non-finite opt-out removed core src/goals killed, 11 red
M8 restore rebase removed (the default-path change) core src/goals killed, 1 red — goal runtime > does not charge offline time to a restored active Goal
M9 no-progress bound reverted to isGoalTokenBudgetSpent core src/goals killed, 1 red
M10 parseGoalRecord field whitelist narrowed core src/goals killed, 2 red
M11 prompt's activeTimeMs !== undefined half of pair-or-nothing core src/goals SURVIVED, 598/598 green
M12 sub-minute reason branch collapsed to "0 minutes" core src/goals killed, 1 red
M13 Web Shell field whitelist drops both new fields mappers.test.ts killed, 2 red
M14 Web Shell limitKind union drops both new values mappers.test.ts killed, 2 red
M15 excludedValues check removed from validateSettingValue settingsUtils.test.ts killed, 1 red

15 killed / 1 survived. The positive control M0 is in the same file and the same suite as the goal-runtime mutants and turned 12 tests red, so the harness demonstrably can fail this suite — the survivors are not an artifact of a runner that never executed. M8 answers the vacuity question for the riskiest hunk in the PR by name: the default-path rebase is pinned by a test whose failure message is the intended behavioural mismatch, not a broken import.

M11 adjudicated as redundant defence, not a coverage gap. The mutation drops the activeTimeMs !== undefined half of the conjunction, which would render NaN of 30 active minutes used if a ceiling ever arrived without an elapsed figure. Rather than argue that from the types, I captured the real usage object the runtime hands its hosts, in four shapes: no time ceiling → {tokensUsed, turnCount}; time ceiling armed → both fields; turn ceiling only → {tokensUsed, turnCount, turnBudget}; wind-down turn → both fields. The pair is emitted atomically by the sole producer (goal-runtime.ts:644, one spread guarded on activeTimeBudgetMs === undefined), and all three hosts (nonInteractiveCli.ts, use-llm-stream.ts, acp-integration/session/Session.ts) pass the turn straight through to buildGoalContinuationParts / renderGoalContinuationTurn without constructing usage themselves. No caller in this repository can make the guard false, so nothing can observe its removal — the clause is correct exactly as it stands. The other half of the conjunction (elapsed without a ceiling renders nothing) is reachable and is asserted. Bounded as usual: this holds for callers in this repository; a third-party embedder constructing GoalContinuationUsage by hand is exactly what the guard defends against, which is a reason to keep it, not to test it.

Not covered

  • Per-commit attribution. The checkout is shallow (git rev-parse --is-shallow-repositorytrue) and git rev-list HEAD^1..HEAD^2 returns 1 commit while the metadata snapshot lists 9 (including 3 merges) — the plausible-number trap rather than an error. Only the aggregate HEAD^1..HEAD diff was verified; no per-commit table is claimed.
  • Live-model E2E. Not run. The description says the same of its own plan (.qwen/e2e-tests/2026-09-09-goal-turn-and-time-budget.md, which is git-ignored and absent here). Everything above drives the real compiled runtime through its real public API with a real recording journal and a real turn host, so this reproduces the mechanism, not a model actually choosing to hand off when told to. Whether a model honours the wind-down instruction is untested by this round and by the PR.
  • Display surfaces. The footer pill, status cards, headless Usage: line and Web Shell strip still show only the token pair. The description declares this out of scope and defers it to the design doc; not independently audited beyond confirming GoalPill.tsx was not modified.
  • Repo-wide gates. No repo-wide lint, format, typecheck or full monorepo test run. Typecheck is covered transitively: the workflow's npm run build at HEAD is tsc --build, and it succeeded. The base control build left one unrelated tsc error (TS7016, @lydell/node-pty typings in src/services/shellExecutionService.ts) from the worktree lacking the head resolution context; 0 errors in src/goals/ or src/config/, and tsc emitted regardless, so the base goals output is trustworthy — asserted above by symbol census rather than assumed.
  • packages/cli/src/config/settings.test.ts was not made green. Its 13 failures are attributed, not fixed (see below).
  • Replay calibration is not applicable — this PR touches no workflow or CI script, so there is no embedded bash/jq to extract and no emitted artifact to calibrate against.
  • Concurrency. Goals are single-runtime per session here; no probe was made at two hosts racing one Goal's resume against its settle.

Attribution of the 13 CLI failures (A/A control)

src/config/settings.test.ts is not in the PR's diff, and neither is the src/config/settings.ts it exercises — git diff --stat HEAD^1..HEAD over both paths is empty. The two files the PR did change in that directory (settingsSchema.ts, settingsUtils.ts) were reverted to their HEAD^1 versions in place, the identical test file re-run, and the head versions restored (git status --porcelain clean afterwards, goalMaxTurns present again):

arm result failing names
head, whole CLI package 13 failed / 30229 passed / 103 skipped 13
head, settings.test.ts in isolation 13 failed / 184 passed (197) 13
base versions of the two changed files 13 failed / 184 passed (197) 13

diff of the two sorted, ANSI-stripped name lists is empty — byte-identical (aa-head-names.txt, aa-base-names-n.txt). All 13 concern ~/.env / ~/.qwen/.env resolution, ${VAR} substitution, home-directory workspace scope and QWEN_HOME/QWEN_RUNTIME_DIR isolation (e.g. should never set QWEN_HOME or QWEN_RUNTIME_DIR from settings.env fails on expect(process.env['HARMLESS_VAR']).toEqual('ok')undefined), none of which reads excludedValues or either new setting. Isolation reproduces it, so it is not cross-file pollution; base reproduces it, so it is not this PR. Conclusion: pre-existing/environmental in this container, and the CLI package's contribution to this PR is green.

Methodology

Environment: the workflow's node:22-bookworm container, Node v22.23.2, merge-ref checkout at depth 2, npm ci and npm run build already complete at HEAD. Head code was exercised through the compiled packages/core/dist and packages/cli/dist output, never through source or a stub of the unit under test. Four harnesses (ab-harness.mjs, prompt-settings-harness.mjs, followup-harness.mjs, boundary-harness.mjs, sharing harness-lib.mjs) drive the real createGoalRuntime through its real public API — dispatch, bindHost, beginTurn, markTurnDelivered, finishTurn, restore — against a journal that really stores the payloads the runtime writes and a turn host that really receives the continuation inputs; the only fakes are those two collaborators, which the runtime requires by contract. The active-time cells use real setTimeout sleeps, not fake timers, so no clock is intercepted anywhere. Restore cells are seeded with journal records a real runtime produced, aged by rewriting only updatedAt/activeTimeMs in a structuredClone. Every expectation is encoded in the harness as a scripted comparison, including the base control cells, so a base arm that behaves as base is predicted to behave counts as a pass; fail would mean an unexpected outcome and there were none. The mutation runner (mutate.py) applies one anchored single-point edit at a time (anchor uniqueness dry-run for all 16 first), runs the targeted suite, and reverts via git checkout, asserting tree cleanliness at the end. Gates: core src/goals + src/config/config.test.ts 1273 passed / 19 files; CLI whole package 30229 passed, 13 failed, 103 skipped / 1036 files; mappers.test.ts 65/65; config-command.test.ts 42/42; the goals suite alone is 598 tests, the denominator for the matrix. The committed packages/vscode-ide-companion/schemas/settings.schema.json was regenerated with npm run generate:settings-schema and is byte-identical to what was committed (sha256 c99a6b8b…), so the artifact was genuinely generated rather than hand-edited. The design doc ships linked English and Chinese versions with 10 parallel headings each and reciprocal language links. Raw per-cell stdout/stderr and both build logs are under logs/; images are under evidence/. The scratch base worktree was removed once every A/B cell was captured (git worktree remove --force tmp/base-tree, then git worktree prune); the two node_modules symlinks pointing from it into the head tree were deleted first, so the removal could not recurse into the head's dependencies — verified intact afterwards (core 11 entries, cli 18, root @qwen-code 24) with git status --porcelain empty and git worktree list showing only the main checkout.

Flakiness gate log

rounds=5 files=18 skipped=0
file packages/cli/src/acp-integration/acpAgent.worktree.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/acpAgent.worktree.test.ts
file packages/cli/src/acp-integration/session/Session.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/Session.test.ts
file packages/cli/src/config/config.test.ts: (cd packages/cli) npx --no-install vitest run ./src/config/config.test.ts
file packages/cli/src/config/settingsSchema.test.ts: (cd packages/cli) npx --no-install vitest run ./src/config/settingsSchema.test.ts
file packages/cli/src/config/settingsUtils.test.ts: (cd packages/cli) npx --no-install vitest run ./src/config/settingsUtils.test.ts
file packages/cli/src/nonInteractiveCli.test.ts: (cd packages/cli) npx --no-install vitest run ./src/nonInteractiveCli.test.ts
file packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/workspace-service/__tests__/facade.test.ts
file packages/cli/src/ui/commands/config-command.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/commands/config-command.test.ts
file packages/cli/src/ui/hooks/use-llm-stream.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/hooks/use-llm-stream.test.tsx
file packages/cli/src/utils/runBudget.test.ts: (cd packages/cli) npx --no-install vitest run ./src/utils/runBudget.test.ts
file packages/core/src/config/config.test.ts: (cd packages/core) npx --no-install vitest run ./src/config/config.test.ts
file packages/core/src/goals/goal-continuation-prompt.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-continuation-prompt.test.ts
file packages/core/src/goals/goal-protocol.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-protocol.test.ts
file packages/core/src/goals/goal-reducer.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-reducer.test.ts
file packages/core/src/goals/goal-runtime.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-runtime.test.ts
file packages/core/src/goals/goal-tools.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-tools.test.ts
file packages/core/src/skills/bundled/goal-draft/SKILL.test.ts: (cd packages/core) npx --no-install vitest run ./src/skills/bundled/goal-draft/SKILL.test.ts
file packages/web-shell/client/daemon/session/mappers.test.ts: (cd packages/web-shell) npx --no-install vitest run ./client/daemon/session/mappers.test.ts


per-file results (P=pass F=fail I=infra-exit, one letter per run):
  packages/cli/src/acp-integration/acpAgent.worktree.test.ts: PPPP
  packages/cli/src/acp-integration/session/Session.test.ts: PPPP
  packages/cli/src/config/config.test.ts: PPPP
  packages/cli/src/config/settingsSchema.test.ts: PPP
  packages/cli/src/config/settingsUtils.test.ts: PPP
  packages/cli/src/nonInteractiveCli.test.ts: PPP
  packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: PPP
  packages/cli/src/ui/commands/config-command.test.ts: PPP
  packages/cli/src/ui/hooks/use-llm-stream.test.tsx: PPP
  packages/cli/src/utils/runBudget.test.ts: PPP
  packages/core/src/config/config.test.ts: PPP
  packages/core/src/goals/goal-continuation-prompt.test.ts: PPP
  packages/core/src/goals/goal-protocol.test.ts: PPP
  packages/core/src/goals/goal-reducer.test.ts: PPP
  packages/core/src/goals/goal-runtime.test.ts: PPP
  packages/core/src/goals/goal-tools.test.ts: PPP
  packages/core/src/skills/bundled/goal-draft/SKILL.test.ts: PPP
  packages/web-shell/client/daemon/session/mappers.test.ts: PPP

verdict: timeout
summary: only 3 of 5 rounds fit the 15-minute budget; the completed rounds agreed

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/acp-integration/acpAgent.worktree.test.ts: P (exit 0)
round 1 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 1 · packages/cli/src/config/config.test.ts: P (exit 0)
round 1 · packages/cli/src/config/settingsSchema.test.ts: P (exit 0)
round 1 · packages/cli/src/config/settingsUtils.test.ts: P (exit 0)
round 1 · packages/cli/src/nonInteractiveCli.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/commands/config-command.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/hooks/use-llm-stream.test.tsx: P (exit 0)
round 1 · packages/cli/src/utils/runBudget.test.ts: P (exit 0)
round 1 · packages/core/src/config/config.test.ts: P (exit 0)
round 1 · packages/core/src/goals/goal-continuation-prompt.test.ts: P (exit 0)
round 1 · packages/core/src/goals/goal-protocol.test.ts: P (exit 0)
round 1 · packages/core/src/goals/goal-reducer.test.ts: P (exit 0)
round 1 · packages/core/src/goals/goal-runtime.test.ts: P (exit 0)
round 1 · packages/core/src/goals/goal-tools.test.ts: P (exit 0)
round 1 · packages/core/src/skills/bundled/goal-draft/SKILL.test.ts: P (exit 0)
round 1 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/acpAgent.worktree.test.ts: P (exit 0)
round 2 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 2 · packages/cli/src/config/config.test.ts: P (exit 0)
round 2 · packages/cli/src/config/settingsSchema.test.ts: P (exit 0)
round 2 · packages/cli/src/config/settingsUtils.test.ts: P (exit 0)
round 2 · packages/cli/src/nonInteractiveCli.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/commands/config-command.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/hooks/use-llm-stream.test.tsx: P (exit 0)
round 2 · packages/cli/src/utils/runBudget.test.ts: P (exit 0)
round 2 · packages/core/src/config/config.test.ts: P (exit 0)
round 2 · packages/core/src/goals/goal-continuation-prompt.test.ts: P (exit 0)
round 2 · packages/core/src/goals/goal-protocol.test.ts: P (exit 0)
round 2 · packages/core/src/goals/goal-reducer.test.ts: P (exit 0)
round 2 · packages/core/src/goals/goal-runtime.test.ts: P (exit 0)
round 2 · packages/core/src/goals/goal-tools.test.ts: P (exit 0)
round 2 · packages/core/src/skills/bundled/goal-draft/SKILL.test.ts: P (exit 0)
round 2 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/acpAgent.worktree.test.ts: P (exit 0)
round 3 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 3 · packages/cli/src/config/config.test.ts: P (exit 0)
round 3 · packages/cli/src/config/settingsSchema.test.ts: P (exit 0)
round 3 · packages/cli/src/config/settingsUtils.test.ts: P (exit 0)
round 3 · packages/cli/src/nonInteractiveCli.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/workspace-service/__tests__/facade.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/commands/config-command.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/hooks/use-llm-stream.test.tsx: P (exit 0)
round 3 · packages/cli/src/utils/runBudget.test.ts: P (exit 0)
round 3 · packages/core/src/config/config.test.ts: P (exit 0)
round 3 · packages/core/src/goals/goal-continuation-prompt.test.ts: P (exit 0)
round 3 · packages/core/src/goals/goal-protocol.test.ts: P (exit 0)
round 3 · packages/core/src/goals/goal-reducer.test.ts: P (exit 0)
round 3 · packages/core/src/goals/goal-runtime.test.ts: P (exit 0)
round 3 · packages/core/src/goals/goal-tools.test.ts: P (exit 0)
round 3 · packages/core/src/skills/bundled/goal-draft/SKILL.test.ts: P (exit 0)
round 3 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/acpAgent.worktree.test.ts: P (exit 0)
round 4 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 4 · packages/cli/src/config/config.test.ts: P (exit 0)

Evidence images

01-ab-turn-time-budget-head-vs-base

02-prompt-text-and-settings-ladder

03-mutation-matrix

04-m11-adjudication-and-ceiling-ordering

05-boundary-born-spent-and-grant-edges

Harness scripts and raw logs are in the workflow run artifacts (7-day retention).

Qwen Code · sandboxed verification

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

LGTM, looks ready to ship. ✅

@wenshao
wenshao added this pull request to the merge queue Sep 11, 2026
Merged via the queue into QwenLM:main with commit 5a9c9b3 Sep 11, 2026
289 of 293 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants