Skip to content

fix(goal): persist why a checkpoint failed and show it before the Goal stops - #11576

Merged
qqqys merged 6 commits into
QwenLM:mainfrom
qqqys:fix/goal-checkpoint-failure-cause
Sep 11, 2026
Merged

fix(goal): persist why a checkpoint failed and show it before the Goal stops#11576
qqqys merged 6 commits into
QwenLM:mainfrom
qqqys:fix/goal-checkpoint-failure-cause

Conversation

@qqqys

@qqqys qqqys commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

When an evidence checkpoint check fails, the Goal record now keeps a one-line diagnostic of what the check ran into, next to the existing stall streak. The diagnostic is set by every failed check (whether or not it spends a stall), cleared by a check that succeeds, left alone by a check that proves nothing, and cleared together with the streak by edit and by the resume of an evidence-limited Goal. It is written display-safe: terminal control sequences and bidi overrides are removed, whitespace (line breaks included) is collapsed to single spaces, and the result is capped at 500 code points. A stop at another checkpoint bound scopes the diagnostic to that stop: a too-large checkpoint request records its own failure, and the other bounds clear whatever an earlier check left.

When three stalled checks stop the Goal, the stop reason follows the check that spent the last stall instead of always advising a narrower objective:

  • Capacity (a full claim list that still left evidence behind, or well-formed claims over the claim count, byte budget or per-claim length) keeps the advice to narrow the objective.
  • Unusable output (an answer that cannot be folded into claims) says the checkpoint model is not returning usable structured output, and that narrowing does not fix it.
  • No answer says the recorded failure tells which of an unreachable or rate-limited provider, a check that did not finish within model.goalCheckpointTimeoutSeconds, or an error in the check itself, and lists the remedies for each.

All three still record limitKind: 'evidence_catalog', so resume behaves exactly as before and starts a fresh evidence window. The verifier now reports its own timeout when a provider SDK answers the aborted request with a generic "Request was aborted." error, so a timed-out check says it timed out, and a checkpoint request too large to send names its measured size.

The streak and the diagnostic are visible before the stop, under one rule shared by every surface: never on a completed Goal, always while a stall streak runs, the failure that stopped a Goal whose checkpoint request was too large, and any other failure that spent no stall only while the Goal is active. While an active Goal's streak runs, the Ink footer pill switches to checkpoint N/3 stalled, and the web shell's Goal status strip shows the count whatever the status. Whenever a Goal status card renders, in the Ink TUI and in OpenTUI, it shows a Checkpoint: line; the web shell's Goals dialog shows the same line with the full text as a tooltip, and headless /goal text output prints it too. The terminal surfaces word that line through one core helper, and the web shell's copy of the visibility rule is pinned to core's by a drift test. get_goal reports both fields, including in the lastGoal summary of a stopped Goal. The checkpointRequired hint and the update_goal description tell the model to retry right after get_goal, before running other tools, because every new tool result can push a cited entry out of the bounded catalog.

Why it's needed

A stalled-checkpoint stop could not say which failure caused it, and nothing showed a failing checkpoint until the Goal stopped (#11326). In a daemon session on 2026-09-10 a single Goal turn of about 60 tool calls overflowed the evidence catalog. Three consecutive checkpoint checks then failed within about 40 seconds each, the cursor never moved, and the Goal stopped as usage_limited after roughly 7.6M tokens with the fixed advice to narrow the objective. The objective was not too broad, the persisted record did not say what the checks had returned, and during the whole run every surface showed the Goal as working. The model also lost one retry to invalidEvidenceRefs because it re-ran a verification command before retrying update_goal.

Reviewer Test Plan

How to verify

  1. cd packages/core && npx vitest run src/goals — the stall-breaker tests assert the stop reason per failure shape and the diagnostic on the record at every step: a full claim list and a claim-count, claim-budget or claim-length overrun keep GOAL_CHECKPOINT_STALLED_REASON, a verifier timeout or provider error ends with GOAL_CHECKPOINT_UNREACHABLE_REASON, unusable output (including any other subclass of InvalidGoalCheckpointError) ends with GOAL_CHECKPOINT_UNUSABLE_REASON. A check with room records its failure without spending a stall, a check that proves nothing keeps the diagnostic, a check that finds room clears it, the recorded value is capped and one line, a too-large request records its own failure (visible with no streak), and a checkpoint stop for another reason clears the diagnostic while keeping the streak. goal-protocol.test.ts pins the reasons, the one-line cleanup and the visibility rule; goal-tools.test.ts pins the lastGoal summary and the retry hint; goal-checkpoint-verifier.test.ts pins the timeout reason.
  2. cd packages/cli && npx vitest run src/ui/components/GoalPill.test.tsx src/ui/components/messages/GoalStatusMessage.test.tsx src/ui/opentui/live-session-model.test.ts src/ui/commands/goal-checkpoint-stall-limit-wire-key.test.ts src/nonInteractiveCli.test.ts — the pill label, the card line under the visibility rule (including a bare streak), terminal sanitization, the headless /goal checkpoint line, and the core/SDK stall-limit parity.
  3. cd packages/web-shell && npx vitest run client/components/dialogs/GoalsDialog.test.tsx client/components/GoalStatusStrip.test.tsx client/daemon/session/mappers.test.ts client/utils/goalGate.drift.test.ts — the mapper keeps both fields, the dialog renders the line (stopped card, tooltip, sanitized text, visibility rule) and the strip shows the streak. The new copy is added to both the English and Chinese message tables; no automated test checks key parity between them.

Evidence (Before & After)

Before: the incident session's stopped record carried checkpointStalls: 3, limitKind: 'evidence_catalog' and the single fixed stall reason ending in "Edit or replace the Goal with a narrower objective before resuming it." Nothing in the record said what the three checks had returned, and the card, the pill and get_goal showed no checkpoint state while the streak was running.

After: a script drives the built runtime through three turns on an overflowing window (101 records) with three checkpoint verifiers, one that overruns the claim byte budget, one that answers { "claims": [] }, and one that throws a provider error carrying a multi-line body, and prints the record after each turn:

## claims over the byte budget
{"turn":1,"status":"active","checkpointStalls":1,"lastCheckpointFailure":"GoalCheckpointClaimBudgetError: Goal checkpoint claims total 20000 bytes, over the 16000-byte budget"}
{"turn":2,"status":"active","checkpointStalls":2,"lastCheckpointFailure":"GoalCheckpointClaimBudgetError: Goal checkpoint claims total 20000 bytes, over the 16000-byte budget"}
{"turn":3,"status":"usage_limited","checkpointStalls":3,"lastCheckpointFailure":"GoalCheckpointClaimBudgetError: Goal checkpoint claims total 20000 bytes, over the 16000-byte budget","limitKind":"evidence_catalog","lastReason":"The current Goal revision ran three consecutive evidence checkpoints without relief: the evidence window overflowed every time, and the last check could not fit it within the checkpoint claim bounds, so every turn paid a checkpoint call and lost uncatalogued evidence. Automatic retries cannot recover. Edit or replace the Goal with a narrower objective before resuming it."}

## checkpoint verifier answers with no claims
{"turn":1,"status":"active","checkpointStalls":1,"lastCheckpointFailure":"InvalidGoalCheckpointError: Goal checkpoint must contain between 1 and 32 claims"}
{"turn":2,"status":"active","checkpointStalls":2,"lastCheckpointFailure":"InvalidGoalCheckpointError: Goal checkpoint must contain between 1 and 32 claims"}
{"turn":3,"status":"usage_limited","checkpointStalls":3,"lastCheckpointFailure":"InvalidGoalCheckpointError: Goal checkpoint must contain between 1 and 32 claims","limitKind":"evidence_catalog","lastReason":"The current Goal revision ran three consecutive evidence checkpoints without relief: the evidence window overflowed every time, and the last check answered with output that could not be folded into claims. Narrowing the objective does not fix this. Check that the checkpoint model returns the structured JSON it is asked for, or switch models, then resume the Goal; resuming starts a fresh evidence window."}

## provider error carrying a multi-line body
{"turn":1,"status":"active","checkpointStalls":1,"lastCheckpointFailure":"Error: Failed to generate text content: 429 { \"error\": \"rate limited\", \"retry_after\": 30 }"}
{"turn":2,"status":"active","checkpointStalls":2,"lastCheckpointFailure":"Error: Failed to generate text content: 429 { \"error\": \"rate limited\", \"retry_after\": 30 }"}
{"turn":3,"status":"usage_limited","checkpointStalls":3,"lastCheckpointFailure":"Error: Failed to generate text content: 429 { \"error\": \"rate limited\", \"retry_after\": 30 }","limitKind":"evidence_catalog","lastReason":"The current Goal revision ran three consecutive evidence checkpoints without relief: the evidence window overflowed every time, and the last check failed before the checkpoint verifier returned an answer. The recorded checkpoint failure says why: an unreachable or rate-limited provider, a check that did not finish within model.goalCheckpointTimeoutSeconds, or an error in the check itself. Fix the provider, raise that timeout, or narrow the objective so the window checkpoints in time, then resume the Goal; resuming starts a fresh evidence window."}

Local runs on this branch after merging main:

Command Result
core vitest run src/goals 18 files, 655 tests passed
cli Goal pill, status card, OpenTUI model, headless formatter, core/SDK wire-contract tests 5 files, 259 tests passed (1 skipped)
web-shell Goals dialog, status strip, goal gate and its drift test 4 files, 67 tests passed
core build and cli tsc --noEmit, web-shell typecheck no errors outside packages/core/src/code-mode/host.ts, whose quickjs dependencies from #10607 are not installed in this worktree
prettier and eslint on every changed file clean
mutations: room arm stops clearing / cap removed at the call site / completed-Goal gate removed each turns a test red
round-two mutations: join without .filter(Boolean) (Ink, OpenTUI) / headless line removed / count or length capacity operand removed / other-stop default keeps the diagnostic / measured size dropped / checkpoint_request visibility removed (core, dialog) / summary gated on the streak / strip pill active-only / dialog gates on sanitized text / dialog complete clause removed each of the 13 turns a test red
round-three mutations: each CLI site stops using the helper / helper ignores the cleaner / headless or Ink stop reason unsanitized / core rule or web-shell copy gains a branch / dialog bypasses the shared gate / strip tooltip removed each of the 10 turns a test red

Tested on

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

Environment (optional)

Unit tests, type checks, and a script against the built core runtime. No interactive session was run in this round; see wenshao's end-to-end verification on the first head for the TUI, daemon and Web Shell.

Risk & Scope

  • Main risk or tradeoff: the Goal record gains an optional lastCheckpointFailure field. A build older than this one cannot parse a transcript record that carries it; on a downgrade, recovery walks back to the newest record it can parse, so a Goal that had stopped can come back active with its stall streak reset. checkpointStalls already had this property, but the new field is written by every failed check, including checks that spend no stall, so the window is wider. The SDK gains a duplicated GOAL_CHECKPOINT_STALL_LIMIT export, following the existing GOAL_PAUSE_REASON_COMMAND pattern, now pinned to core's value by a parity test.
  • Not validated / out of scope: this does not change when a Goal stops, what counts as a stall, or how checkpoints are built. A diagnostic that spent no stall stays on the record after a pause or another stop; only the surfaces hide it. The claim-count overrun error from fix(goal): strip fences and retry the checkpoint verifier on the shapes a note can fix #11578, now on main, is read as a capacity failure here. Folding an overflowing window in smaller batches is tracked in bug(goal): a checkpoint that fails on an overflowing window is retried with the identical request until the Goal stops #11577.
  • Breaking changes / migration notes: none. The stall-stop wording for the capacity case was generalized and the no-answer wording no longer blames the provider; nothing keys off their text (goalLimitKindForReason never mapped them, and resume reads limitKind).

Linked Issues

Closes #11326

中文说明

这个 PR 做了什么

evidence checkpoint 检查失败时,Goal 记录现在会在已有的停滞计数旁边保存一行诊断,说明这次检查遇到了什么。每次失败的检查都会写入它,不论是否计入停滞;成功的检查会清掉它;什么也没证明的检查不动它;edit 和 evidence 受限 Goal 的 resume 会连同停滞计数一起清掉它。诊断在写入时就做了清理:去掉终端控制序列和 bidi 覆盖符,把空白(含换行)折叠为单个空格,再截断到 500 个码点。其他 checkpoint 上限导致的停机会把诊断限定在这次停机上:checkpoint 请求过大时记录它自己的失败,其余上限清掉更早检查留下的内容。

三次停滞检查让 Goal 停下时,停机原因跟随最后一次计入停滞的那次检查,而不是一律建议收窄目标:

  • 容量(满额 claim 列表仍留下未收录证据,或格式正确但超出条数、字节或单条长度预算)保留收窄目标的建议。
  • 不可用输出(返回的内容无法折叠成 claim)说明 checkpoint 模型没有返回可用的结构化输出,并说明收窄目标解决不了。
  • 没有回应:记录下来的失败会说明是 provider 不可达或限流、检查没在 model.goalCheckpointTimeoutSeconds 内完成,还是检查本身出错,并分别列出补救。

三种情况仍然都记录 limitKind: 'evidence_catalog',resume 的行为与之前完全一致,都从新的 evidence 窗口开始。当 provider SDK 用通用的 "Request was aborted." 回应被中止的请求时,校验器现在会报告它自己的超时,所以超时的检查会写明是超时;checkpoint 请求过大时,错误里带实测字节数。

停滞计数与诊断在停机之前就可见,所有界面共用同一条规则:完成态不显示;停滞计数存在时一律显示;checkpoint 请求过大导致的停机显示那次失败;其他没有计入停滞的失败只在 Goal 处于 active 时显示。active 的 Goal 停滞期间,Ink 底栏 pill 切换为 checkpoint N/3 stalled;web shell 的 Goal 状态条不论状态都显示计数。Goal 状态卡在 Ink TUI 和 OpenTUI 中渲染时会显示 Checkpoint: 行;web shell 的 Goals 对话框显示同样的一行,并把完整文本放在 tooltip 里;headless /goal 文本输出也会打印这一行。终端各界面的这一行由 core 的同一个函数生成,web shell 复制的可见性规则由 drift 测试与 core 对齐。get_goal 会报告这两个字段,已停止 Goal 的 lastGoal 摘要也包含它们。checkpointRequired 提示和 update_goal 描述都要求模型在 get_goal 之后立即重试、不要先运行其他工具,因为每条新的工具结果都可能把已引用的条目挤出有界目录。

为什么需要

停滞停机无法说明是哪种失败导致的,而且在 Goal 停下之前没有任何界面显示 checkpoint 正在失败(#11326)。2026-09-10 的一次 daemon 会话里,一个约 60 次工具调用的 Goal 回合撑满了 evidence 目录。随后连续三次 checkpoint 检查各自在约 40 秒内失败,游标始终没有移动,Goal 在消耗约 760 万 token 后以 usage_limited 停下,并给出固定的"收窄目标"建议。实际上目标并不宽,持久化记录没有说明检查返回了什么,而整个过程中所有界面都显示 Goal 在正常工作。模型还因为在重试 update_goal 之前先重跑了一条验证命令,白白损失了一次 invalidEvidenceRefs 重试。

评审测试计划

如何验证

  1. cd packages/core && npx vitest run src/goals:停滞熔断测试按失败形状断言停机原因,并在每一步断言记录上的诊断。满额 claim 列表以及 claim 条数、字节、单条长度超限保持 GOAL_CHECKPOINT_STALLED_REASON;校验器超时或 provider 错误最终为 GOAL_CHECKPOINT_UNREACHABLE_REASON;不可用输出(包括 InvalidGoalCheckpointError 的任何其他子类)最终为 GOAL_CHECKPOINT_UNUSABLE_REASON。窗口有余量时的失败记录诊断但不计停滞;什么也没证明的检查保留诊断;发现有余量的检查清掉诊断;记录的值被截断且为单行;请求过大时记录它自己的失败(没有停滞计数时也可见);其他原因的 checkpoint 停机清掉诊断、保留停滞计数。goal-protocol.test.ts 钉住文案、单行清理与可见性规则;goal-tools.test.ts 钉住 lastGoal 摘要与重试提示;goal-checkpoint-verifier.test.ts 钉住超时原因。
  2. cd packages/cli && npx vitest run src/ui/components/GoalPill.test.tsx src/ui/components/messages/GoalStatusMessage.test.tsx src/ui/opentui/live-session-model.test.ts src/ui/commands/goal-checkpoint-stall-limit-wire-key.test.ts src/nonInteractiveCli.test.ts:pill 标签、按可见性规则渲染的卡片行(含只有停滞计数的情况)、终端清理、headless /goal 的 checkpoint 行,以及 core 与 SDK 停滞上限一致。
  3. cd packages/web-shell && npx vitest run client/components/dialogs/GoalsDialog.test.tsx client/components/GoalStatusStrip.test.tsx client/daemon/session/mappers.test.ts client/utils/goalGate.drift.test.ts:映射保留两个字段;对话框渲染这一行(停机卡片、tooltip、清理后的文本、可见性规则);状态条显示停滞计数。新文案已加入中英两张消息表,但没有自动化测试检查两边键是否对齐。

证据(前后对比)

修改前:事故会话停机时的记录带有 checkpointStalls: 3limitKind: 'evidence_catalog',以及唯一一条固定的停滞原因,结尾是 "Edit or replace the Goal with a narrower objective before resuming it."。记录里没有任何内容说明三次检查返回了什么;停滞计数累积期间,状态卡、pill 和 get_goal 都不显示 checkpoint 状态。

修改后:用脚本驱动构建后的运行时,在溢出窗口(101 条记录)上跑三个回合,分别使用三种 checkpoint 校验器:claim 超出字节预算、返回 { "claims": [] }、抛出带多行响应体的 provider 错误,并在每个回合后打印记录。输出见上方英文部分。

本分支合并 main 之后的本地运行结果:

core 的 vitest run src/goals 共 18 个文件、655 个用例通过;cli 的 pill、状态卡、OpenTUI 模型、headless 格式化与 core/SDK 常量同步测试共 5 个文件、259 个用例通过(1 个跳过);web-shell 的 Goals 对话框、状态条、goalGate 及其 drift 测试共 4 个文件、67 个用例通过;core 构建、cli tsc --noEmit 与 web-shell typecheckpackages/core/src/code-mode/host.ts 外无错误(该文件依赖 #10607 引入的 quickjs 包,本地 worktree 未安装);全部改动文件的 prettier 与 eslint 检查通过。另做了三个变异:room 分支不再清除诊断、调用处去掉截断、去掉完成态判断,每个都会让对应测试变红。第二轮又做了 13 个:两处去掉 .filter(Boolean)、去掉 headless 行、去掉条数或长度 capacity 判断、其他停机默认保留诊断、去掉实测字节数、去掉 core 与对话框的 checkpoint_request 可见规则、摘要改按停滞计数判断、状态条只在 active 显示、对话框按清理后文本判断、去掉对话框完成态判断,每个都会让对应测试变红。第三轮再做 10 个:三处调用各自不用 helper、helper 忽略 cleaner、headless 与 ink 的 Reason 不清理、core 与 web-shell 副本各加一个分支、对话框绕过共享门、去掉状态条 tooltip,每个都会让对应测试变红。

测试平台

仅在 Linux 上运行;macOS 与 Windows 未测试。

环境(可选)

单元测试、类型检查,以及针对构建后 core 运行时的脚本。本轮没有运行交互式会话;TUI、daemon 与 Web Shell 的端到端情况见 wenshao 在第一个 head 上的验证。

风险与范围

  • 主要风险或取舍:Goal 记录新增可选字段 lastCheckpointFailure。早于本版本的构建无法解析带有该字段的 transcript 记录;降级时,恢复流程会回退到最近一条能解析的记录,于是一个已经停下的 Goal 可能以 active 状态回来,停滞计数被清零。checkpointStalls 本来就有这个特点,但新字段在每次失败检查时都会写入(包括不计入停滞的检查),所以这个窗口更宽。SDK 新增一个复制的 GOAL_CHECKPOINT_STALL_LIMIT 导出,沿用已有的 GOAL_PAUSE_REASON_COMMAND 做法,现在由一条同步测试钉住它与 core 的值一致。
  • 未验证 / 不在范围内:不改变 Goal 何时停止、什么算停滞、checkpoint 如何构建。未计入停滞的诊断在暂停或其他停机后仍保留在记录上,只是各界面不显示。fix(goal): strip fences and retry the checkpoint verifier on the shapes a note can fix #11578(已合入 main)新增的 claim 条数超限错误在本 PR 中按容量类失败处理。把溢出窗口分小批折叠由 bug(goal): a checkpoint that fails on an overflowing window is retried with the identical request until the Goal stops #11577 跟踪。
  • 破坏性变更 / 迁移说明:无。容量情形的停机文案做了泛化,"没有回应"的文案不再断定是 provider 的问题;没有任何逻辑依赖它们的文本(goalLimitKindForReason 从未映射它们,resume 读取的是 limitKind)。

关联 Issue

Closes #11326

…l stops

A stalled-checkpoint stop always recorded the same reason and advised
narrowing the objective, whatever the three failed checks had run into,
and nothing showed a failing checkpoint until the Goal stopped.

- Keep a capped one-line lastCheckpointFailure on the Goal record: set by
  every failed check, cleared by a check that succeeds, left alone by a
  check that proves nothing, and cleared with the stall streak on edit and
  on the resume of an evidence-limited Goal.
- Pick the stall-stop reason from the check that spent the last stall: a
  full claim list, an unusable verifier answer, or a verifier that never
  answered. limitKind stays evidence_catalog, so resume is unchanged.
- Show the streak and the failure on the Ink and OpenTUI Goal cards, the
  footer pill, the web shell Goals dialog, and get_goal's lastGoal summary.
- Tell the model to retry update_goal right after get_goal when a
  checkpoint is required, before running other tools.

Closes QwenLM#11326
@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Sep 10, 2026
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

⚠️ Qwen Triage ended earlyview run. It stopped before finishing; check the run log.

⚠️ Qwen Triage 提前结束 —— 查看运行。未跑完,请查看运行日志。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — every required heading is present, the Before/After evidence is concrete, and the Chinese section is a full translation rather than a summary.

Problem: observed, and verifiable straight from main without trusting the report. settleIfCheckpointStalled passes a single constant into the settle, so three genuinely different failures write a byte-identical usage_limited record, and the advice that constant ends with ("narrow the objective") is only correct for one of them. The second half of #11326 is equally checkable: nothing carries checkpoint health to any surface before the breaker fires. This is a diagnosability gap with a named incident behind it, not theoretical hardening.

Direction: aligned. #11326 asks for exactly these two halves and explicitly says both are additive and neither changes when a Goal stops — which is what this does. Worth calling out one place where the PR is more right than the issue sketch: the issue grouped "a result that could not be folded into claims" together with "a full claim list" as the compaction shape, but a verifier returning invalid JSON is a model-output problem, and telling that user to rewrite their objective is the same mistake in a different costume. Splitting it three ways is the better read.

Size: core paths are touched (packages/core/src/goals/, plus cross-package into cli, sdk-typescript and web-shell), so the two-tier gate applies. The title is a fix, so the Tier 1 refactor block does not. 364 production lines / 443 test lines / 0 generated or schema lines (807 total) — under the 500-line escalation threshold and under the 1000-line advisory. More test than production code, which is the right shape for a change whose whole job is getting wording and state transitions right.

Approach: the scope is right, and the file count is misleading. 25 files sounds sprawling, but the width is forced by the existing multi-frontend architecture, not by ambition: Goal state is rendered by the Ink pill, the Ink card, the OpenTUI card, the web shell dialog and get_goal, and a diagnostic that reaches only some of them is worse than one that reaches none — the repo runs a tui-parity gate precisely because those two frontends are not allowed to drift. Per surface the change is 2–31 lines. I considered whether the persisting half could ship without the surfacing half and concluded no: "nothing shows a failing checkpoint until the Goal stops" is half the reported bug, so splitting would leave the gap open. The one drive-by is the checkpointRequired hint rewording in update_goal, which is unrelated to checkpoints-failing but is two lines of prose aimed at the same incident (the model burned a retry by running a tool between get_goal and update_goal) — fine to keep, worth a sentence in the description if a maintainer asks.

Risk: no elevated risk signals — none of the changed files match the revert-correlated paths. The one real tradeoff is disclosed in Risk & Scope and I confirmed it is what the author says it is: parseGoalRecord uses an allowlist, so a build older than this one rejects a journal record carrying the new field. That is the same one-way door checkpointStalls and noProgressTurns already walked through, and the blast radius is contained — every caller of parseGoalStateRecordPayloadV2 treats a parse failure as "no Goal state", so a downgrade loses the Goal, not the session.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 所有必需小节都在,Before/After 证据具体,中文部分是完整翻译而非摘要。

问题: 已观测,且不依赖报告本身就能在 main 上验证。settleIfCheckpointStalled 把单一常量传进 settle,于是三种性质不同的失败写出逐字节相同的 usage_limited 记录,而该常量结尾的建议("收窄目标")只对其中一种成立。#11326 的后半段同样可验证:熔断触发之前,没有任何界面承载 checkpoint 健康度。这是一个有具体事故支撑的可诊断性缺口,不是理论性加固。

方向: 对齐。#11326 要的正是这两半,并明确说明两者都是增量、都不改变 Goal 何时停止 —— 本 PR 就是这样做的。有一处值得单独指出:PR 比 issue 的草图更准确。issue 把"无法折叠成 claim 的结果"和"满额 claim 列表"一起归入压缩形状,但校验器返回非法 JSON 属于模型输出问题,让这样的用户去重写目标,是同一个错误的另一种形式。拆成三类是更好的判断。

规模: 触及核心路径(packages/core/src/goals/,并跨包进入 cli、sdk-typescript、web-shell),因此适用两级门禁。标题是 fix,所以 Tier 1 的 refactor 硬阻断不适用。生产代码 364 行 / 测试 443 行 / 生成与 schema 0 行(合计 807)—— 低于 500 行的升级阈值,也低于 1000 行的大 PR 建议阈值。测试多于生产代码,对一个全部工作就是把措辞与状态迁移做对的改动来说,这个比例是合理的。

方案: 范围合理,文件数具有误导性。25 个文件看起来很宽,但这个宽度来自现有的多前端架构,而不是来自野心:Goal 状态由 Ink pill、Ink 状态卡、OpenTUI 状态卡、web shell 对话框和 get_goal 共同渲染,只覆盖其中一部分的诊断比一个都不覆盖更糟 —— 仓库专门跑 tui-parity 门禁,正是因为这两个前端不允许漂移。单个界面上的改动只有 2–31 行。我考虑过能否只做持久化而不做界面暴露,结论是不行:"Goal 停下之前没有任何界面显示 checkpoint 失败"本身就是所报缺陷的一半,拆开做等于缺口仍然开着。唯一顺手带的改动是 update_goalcheckpointRequired 提示的措辞,它与"checkpoint 失败"无关,但只有两行文字,且指向同一次事故(模型在 get_goalupdate_goal 之间跑了别的工具,白白损失一次重试)—— 保留没问题,若维护者问起,值得在描述里补一句。

风险: 无升级风险信号 —— 改动文件都没有命中与 revert 相关的路径。唯一真实的取舍已在 Risk & Scope 中披露,我确认它与作者所述一致:parseGoalRecord 使用白名单,因此比本次更旧的构建会拒绝携带新字段的 journal 记录。这与 checkpointStallsnoProgressTurns 已经走过的单向门相同,且影响范围可控 —— parseGoalStateRecordPayloadV2 的所有调用方都把解析失败当作"没有 Goal 状态"处理,所以降级会丢掉 Goal,而不是丢掉整个会话。

进入代码审查 🔍

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

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

No Critical blockers. I read the diff against main and, because this touches packages/core/src/goals/ and crosses into three other packages, I went looking for every consumer of the surface it changes rather than trusting the description. The claims that mattered all held:

  • The reason-constant rewrite is safe. GOAL_CHECKPOINT_STALLED_REASON's prose changes, which is the kind of edit that silently breaks an identity comparison. It doesn't here: goalLimitKindForReason compares with === against two other constants and never against any stall reason, so all three reasons — old and new — return undefined, and resume stays keyed on limitKind: 'evidence_catalog'. I grepped for the old wording across the whole repo and it exists in exactly one place, its own definition — no test, snapshot, prompt or doc hardcodes a copy. The new test that pins all three reasons to undefined is the right guard.
  • Every consumer of the new field is accounted for. parseGoalRecord's allowlist is the only strict validator of the record shape, and it's updated; both places that clear checkpointStalls (edit, and resume of an evidence-limited Goal) clear the new field beside it, and the paused-resume arm correctly keeps both. The web shell's field-by-field mapper rebuild and the OpenTUI GoalSnapshotLike structural type were the two spots that would have silently dropped it — both updated. The SDK re-declares the record independently and is updated too.
  • The one control-flow reader was correctly left alone. The no-progress pause suppresses itself when a stall streak is running, and it reads checkpointStalls only. That's right: the new field is diagnostic and never gates anything, so a failure that spends no stall must not suppress that pause — and it doesn't.
  • The failure classification matches the real throw sites. InvalidGoalCheckpointError is what parseGoalCheckpointVerifierText throws for invalid JSON and invalid claims, and it's the base of the claim-budget and claim-length errors, so instanceof covers them; a timeout or provider error is a plain Error. GoalCheckpointVerifierInputTooLargeError is not a subclass, but it returns in its own arm before classification, so it can't be mislabelled as unreachable. The three shapes line up with what the code actually throws.
  • Persistence is real, not just in-memory. settledSnapshot spreads the record, journalUsageLimitedSettle writes that exact object and returns it, and commitUsageLimitedSettle clones the same object into the snapshot — so the tests asserting the field after a stop are asserting the journaled payload, and the reducer test that round-trips a record through parseGoalSnapshotV2 closes the replay half.
  • Conventions held. The pill's ! with theme.status.warning is exactly what the existing paused and usage_limited arms use; the i18n entry is a function-valued message matching the existing git.currentBranch pattern, and t already forwards vars; the barrel import in the Ink card resolves because goals/index.ts star-exports goal-protocol.js.

Two non-blocking notes:

  1. The test plan's step 3 oversells client/i18n.test.ts. It says that file confirms "both locales carry the new keys", but the PR doesn't touch it and it currently pins only two follow-up-suggestion keys. Nothing asserts EN/ZH parity for these three: Messages is Record<string, MessageValue>, so a missing ZH key would silently fall back to English through messages[key] ?? EN[key] ?? key. The keys are added to both locales in this diff, so the behaviour is correct — a reviewer following the plan just won't find the coverage they were promised.
  2. The SDK's duplicated GOAL_CHECKPOINT_STALL_LIMIT has a "must match" comment and nothing enforcing it. If core ever changes the limit, the web shell keeps rendering N/3. The duplication itself is right — the SDK is published separately and can't import core, and GOAL_PAUSE_REASON_COMMAND sets the precedent — but a one-line assertion pinning both sides to the same number would make the comment self-enforcing.

One design consequence worth stating, since it's deliberate and I want it on the record rather than discovered later: the stop reason follows only the check that spent the last stall. A mixed streak — two compaction failures then one timeout — reports the timeout shape and advises waiting for the provider. I think that's the right call (the last check is the freshest evidence about the current state, and the alternative is majority-voting three failures), and lastCheckpointFailure sits on the same record and the same card with the actual error, so nothing is lost. Just flagging that the reason describes one check, not the streak.

sequenceDiagram
    participant P1 as Checkpoint check
    participant P2 as Classify failure
    participant P3 as Goal record health
    participant P4 as Stall breaker
    participant P5 as Journal and surfaces
    P1->>P2: check threw
    P2-->>P3: shape plus capped one-line detail
    P1->>P3: check succeeded (clear diagnostic)
    P1->>P3: check proved nothing (keep previous)
    P3->>P4: streak count
    alt streak below the limit
        P4-->>P5: persist record, Goal stays active
    else streak reaches the limit
        P4->>P4: pick stop reason from the last stalled shape
        P4-->>P5: usage_limited, limitKind evidence_catalog, diagnostic kept
    end
Loading
Files changed (all 24 shown)
File What changed
packages/core/src/goals/goal-protocol.ts The heart of it: two new stop reasons, a three-way failure-shape type with a total mapper, a 500-code-point cap helper that slices on code points so an astral character can't leave a lone surrogate in the journal, and the new optional record field
packages/core/src/goals/goal-runtime.ts Classifies a thrown check, threads the result through the health writer and the stall settle, and picks the stop reason from the shape that spent the last stall
packages/core/src/goals/goal-reducer.ts Accepts the field in the strict parser, rejects an empty or non-string one, and clears it wherever the streak is cleared
packages/core/src/goals/goal-tools.ts Adds both fields to the stopped-Goal summary, documents them in the tool description, and rewords the checkpoint-required retry hint
packages/cli/src/ui/components/GoalPill.tsx Footer pill switches to a warning-coloured stalled-checkpoint label while a streak runs
packages/cli/src/ui/components/messages/GoalStatusMessage.tsx Ink status card gains a Checkpoint line, kept on the card of a stopped Goal
packages/cli/src/ui/opentui/live-session-model.ts Same line for the OpenTUI card, worded to match Ink for the parity gate
packages/cli/src/ui/opentui/transcript-view.tsx Renders that line, through the existing terminal-text sanitizer
packages/cli/src/ui/opentui/event-adapter.ts Two fields added to the structural snapshot type so they survive the adapter
packages/web-shell/client/components/dialogs/GoalsDialog.tsx Goals dialog renders the line from the SDK's duplicated stall limit
packages/web-shell/client/daemon/session/mappers.ts Stops dropping both fields in the field-by-field wire rebuild
packages/web-shell/client/i18n.tsx Three keys in each of EN and ZH
packages/sdk-typescript/src/daemon/types.ts Both fields on the SDK record, plus the duplicated stall limit with a must-match note
packages/sdk-typescript/src/daemon/index.ts Exports that constant
docs/users/features/goals.md One paragraph: what a failing checkpoint looks like before the stop, and which advice fits which shape
packages/core/src/goals/goal-protocol.test.ts Reason per shape, all three reasons mapping to no limit kind, and the code-point cap
packages/core/src/goals/goal-reducer.test.ts Replay round-trip, malformed values rejected, cleared on edit and evidence-limited resume, kept on paused resume
packages/core/src/goals/goal-runtime.test.ts Per-shape stop reason, diagnostic at every step, set without spending a stall, kept by an inconclusive check, cleared by success, and the restore replay case
packages/core/src/goals/goal-tools.test.ts Both fields in the stopped-Goal summary
packages/cli/src/ui/components/GoalPill.test.tsx Pill label while stalled, and quiet labels when nothing has stalled
packages/cli/src/ui/components/messages/GoalStatusMessage.test.tsx Card line active and stopped, and absent when healthy
packages/cli/src/ui/opentui/live-session-model.test.ts OpenTUI card matching the Ink wording
packages/web-shell/client/components/dialogs/GoalsDialog.test.tsx Dialog line for both the stalled and the no-stall-failure case
packages/web-shell/client/daemon/session/mappers.test.ts Pins the mapper passthrough from the wire

Testing evidence

This is an unattended CI run, so I did not build or execute anything from this PR — the evidence below is the PR's own CI on the reviewed commit, read through the API. Zero checks are red at the time of writing; the unit suite and the web-shell visual capture are still running, so this is a partial picture and the approval is deferred rather than given.

What has already landed is directly relevant, not incidental: Lint & Static is green, which settles the type-level questions a static review can only argue about (the barrel import in the Ink card, the function-valued i18n message, the SDK export). TUI parity snapshots (ink vs opentui) and the OpenTUI no-flicker gate are green, which matters because this PR adds a line to both Goal cards and those two are exactly the gates that catch the frontends drifting apart. Integration Tests (no-AK, No Sandbox), Live Host (macos-latest) and both Desktop Shell jobs are green.

Still pending, and the one that carries the weight here: Test (ubuntu-latest, Node 22.x) — the unit suite that runs the 443 new test lines. Until it lands, the per-shape stop reasons, the record round-trip and the surface assertions are unverified by anything I can point to.

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

Check Conclusion
Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ success
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Lint & Static (ubuntu-latest, Node 22.x) ✅ success
Live Host (macos-latest) ✅ success
OpenTUI no-flicker gate ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
TUI parity snapshots (ink vs opentui) ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

The Before/After block in the description is the author's own claim, not independently re-run here: a script driving the built runtime through three turns on a 101-record overflowing window, once with a verifier that always times out and once with one that always answers with no claims. It reads plausibly and matches what the code does, and the description is honest that no interactive session was run — but it is the author's output on the author's machine, and I am not presenting it as evidence.

Sandboxed verification would settle the remaining gap: @qwen-code /tmux — that the new Checkpoint: line and the checkpoint N/3 stalled pill actually render in a live session is not observable from this diff or from the unit suite, because every surface test feeds the components a synthetic snapshot rather than a real Goal whose checkpoints are failing. The author has write access, so the job can be triggered directly. This is the honest residual: the state-machine half is well pinned by tests, the pixels half is pinned only by parity snapshots of components fed synthetic data.

中文说明

代码审查

无 Critical 阻断项。我把 diff 对着 main 读了一遍;由于它触及 packages/core/src/goals/ 并跨到另外三个包,我没有采信描述,而是去把它改动的每一个下游消费方都找了出来。关键的几条都成立:

  • 改写 reason 常量是安全的。 GOAL_CHECKPOINT_STALLED_REASON 的文字变了,这类改动最容易悄悄破坏身份比较。这里没有:goalLimitKindForReason=== 只比较另外两个常量,从不比较任何停滞 reason,所以三个 reason(新旧都一样)返回 undefined,resume 仍然只依赖 limitKind: 'evidence_catalog'。我在整个仓库搜了旧措辞,只在一个地方出现,就是它自己的定义 —— 没有任何测试、快照、提示词或文档硬编码副本。把三个 reason 都钉为 undefined 的新测试正是该有的防线。
  • 新字段的每个消费方都有着落。 parseGoalRecord 的白名单是记录形状唯一的严格校验器,已更新;两处清除 checkpointStalls 的地方(edit,以及 evidence 受限 Goal 的 resume)都在旁边清除新字段,而 paused 的 resume 分支正确地保留了两者。web shell 逐字段重建的 mapper 与 OpenTUI 的 GoalSnapshotLike 结构类型是两处会静默丢掉它的地方 —— 都已更新。SDK 独立声明了该记录,也已更新。
  • 唯一读它做控制流的地方被正确地没有改动。 no-progress 暂停在停滞计数存在时会让路,而它只读 checkpointStalls。这是对的:新字段纯粹是诊断,不参与任何门控,所以一次不计停滞的失败绝不能压掉那个暂停 —— 它也没有。
  • 失败分类与真实的抛出点吻合。 InvalidGoalCheckpointError 正是 parseGoalCheckpointVerifierText 在非法 JSON 与非法 claims 时抛出的,并且是 claim 预算与 claim 长度错误的基类,所以 instanceof 覆盖它们;超时或 provider 错误则是普通 ErrorGoalCheckpointVerifierInputTooLargeError 不是它的子类,但在分类之前就从自己的分支返回了,因此不会被误标为不可达。三种形状与代码实际抛出的东西对得上。
  • 持久化是真的,不只是内存态。 settledSnapshot 展开记录,journalUsageLimitedSettle 写入的正是这个对象并把它返回,commitUsageLimitedSettle 再把同一个对象克隆进快照 —— 所以停机后断言该字段的测试,断言的就是写进 journal 的载荷;而 reducer 里把记录经 parseGoalSnapshotV2 走一遍的测试补上了回放那一半。
  • 约定守住了。 pill 用 !theme.status.warning,与现有 pausedusage_limited 分支完全一致;i18n 条目是函数型消息,与已有的 git.currentBranch 模式相同,而 t 本来就会转发参数;Ink 卡片走 barrel 导入能成立,是因为 goals/index.tsgoal-protocol.js 做了 star export。

两条非阻断意见:

  1. 测试计划第 3 步高估了 client/i18n.test.ts 它说该文件确认"两种语言都带上了新键",但本 PR 没有改它,而它目前只钉了两个 follow-up suggestion 键。这三个新键的中英一致性没有任何断言:MessagesRecord<string, MessageValue>,缺少中文键会通过 messages[key] ?? EN[key] ?? key 静默回落到英文。本次 diff 确实在两种语言里都加了键,所以行为是对的 —— 只是照着计划去查的评审者找不到被承诺的覆盖。
  2. SDK 里重复的 GOAL_CHECKPOINT_STALL_LIMIT 只有一句"必须一致"的注释,没有任何东西强制它。 若 core 哪天改了上限,web shell 仍会渲染 N/3。重复本身是对的 —— SDK 独立发布,不能导入 core,而 GOAL_PAUSE_REASON_COMMAND 已有先例 —— 但加一行把两边钉在同一个数字上的断言,就能让那句注释自我执行。

还有一条设计后果值得写明,因为它是有意的,我希望它留在记录上而不是日后被发现:停机原因只跟随最后一次计入停滞的检查。混合的停滞序列 —— 两次压缩失败后接一次超时 —— 会报告超时形状并建议等 provider 恢复。我认为这是对的取舍(最后一次检查是关于当前状态的最新证据,替代方案是给三次失败投票),而且 lastCheckpointFailure 就在同一条记录、同一张卡片上,带着真实的错误,所以没有信息丢失。只是提醒:这段 reason 描述的是某一次检查,不是整个停滞序列。

测试证据

这是一次无人值守的 CI 运行,所以我没有构建或执行本 PR 的任何代码 —— 下面的证据是通过 API 读到的、该 PR 自己在被评审 commit 上的 CI 结果。撰写时没有任何检查是红的;单元测试套件与 web-shell 视觉截图仍在运行,所以这是一张局部图景,批准被延后而不是给出。

已经落地的部分与本 PR 直接相关,不是顺带的:Lint & Static 通过,这把静态审查只能靠论证的类型问题定了下来(Ink 卡片的 barrel 导入、函数型 i18n 消息、SDK 导出)。TUI parity snapshots (ink vs opentui)OpenTUI no-flicker gate 通过,这一点很要紧,因为本 PR 给两张 Goal 卡片都加了一行,而这两个正是抓前端漂移的门禁。Integration Tests (no-AK, No Sandbox)Live Host (macos-latest) 与两个 Desktop Shell 作业均通过。

仍未完成、而且在这里分量最重的是 Test (ubuntu-latest, Node 22.x) —— 跑那 443 行新测试的单元套件。在它落地之前,按形状区分的停机原因、记录往返、各界面断言,都没有我能指向的验证。

描述里的 Before/After 是作者自己的陈述,此处未独立复跑:一个脚本驱动已构建的 runtime,在 101 条记录的溢出窗口上跑三轮,一次用总是超时的校验器,一次用总是返回空 claims 的校验器。它读起来可信,也与代码行为一致,描述也老实说明了没有运行交互式会话 —— 但它是作者在自己机器上的输出,我不把它当作证据呈现。

沙箱验证可以补上剩下的缺口:@qwen-code /tmux —— 新的 Checkpoint: 行与 checkpoint N/3 stalled pill 是否真的在实时会话里渲染出来,从这份 diff 和单元套件都看不出来,因为所有界面测试喂给组件的都是合成快照,而不是一个 checkpoint 真的在失败的 Goal。作者有写权限,可以直接触发该作业。这是诚实的残余风险:状态机那一半被测试钉得很牢,像素那一半只由喂了合成数据的组件的 parity 快照钉着。

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

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 10, 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 f51ccf9. 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 4 render-shaping files:

  • packages/web-shell/client/components/GoalStatusStrip.module.css
  • packages/web-shell/client/components/GoalStatusStrip.tsx
  • packages/web-shell/client/components/dialogs/GoalsDialog.tsx
  • 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

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — solid and unusually well-pinned for a change to Goal state; the two nits I found are about coverage claims and an unenforced duplicate constant, not about the code being wrong.

Stepping back. My own proposal before reading the diff was the same shape as this: thread the failure into the settle, split the advice by cause, and put the streak on the record so surfaces can read it. The PR matches that and beats it in three places I'd have gotten wrong. I would have capped the diagnostic with .slice(0, 500) and shipped a lone-surrogate bug into a journaled record; it slices on code points and tests the astral case. I would have cleared the diagnostic on anything that wasn't a failure, which would have erased a real diagnostic on an inconclusive check that proved nothing; it distinguishes set, clear and preserve, and that distinction is the subtlest piece of state design in the diff. And I would have written an if/else chain on an error message; it names the three shapes as a union with an exhaustive switch, so a fourth shape is a compile error rather than a grep.

The reason constants each carry a comment saying why that shape gets that advice. In six months that is the difference between someone trusting the mapping and someone re-deriving it, and it's the reason I'd thank the author rather than curse them.

On need: this isn't a solution hunting for a problem. A real session burned roughly 7.6M tokens and stopped with advice that was wrong for the failure it hit, the record couldn't say what the three checks returned, and every surface said the Goal was working. The linked issue is a code-level gap filed out of a review thread on #11304 that deliberately deferred this half. Both halves of that issue are addressed, and the PR improves on the issue's own taxonomy rather than following it literally.

On scope: I asked whether 80% could be cut and the remaining 20% still fix it, and the answer is no. The persisting half without the surfacing half leaves "nothing shows a failing checkpoint until the Goal stops" — half the reported bug — untouched, and the five surfaces aren't optional extras: Goal state is rendered in five places and a diagnostic that reaches three of them is worse than one that reaches none. The 25-file spread is the architecture's cost, not scope creep, and it's mostly a few lines per surface. The single drive-by (the update_goal retry hint) is two lines of prose aimed at the same incident.

Two things a maintainer should know before merging, neither of which I'd block on:

One honesty note on process: this author has 14 open PRs and merged 8 in the last two days, so this is exactly the volume where a gate should get more skeptical rather than tired. I ran the full consumer enumeration I'd run for anyone touching packages/core/src/goals/ — every reader of the changed constant, every clear site, every structural re-declaration of the record, the one place checkpointStalls gates control flow — and it came back clean. The scrutiny was earned and it passed; the volume didn't move my read in either direction.

Verdict: approve, deferred until CI lands green. The unit suite (Qwen Code CI) is still running on the reviewed commit, and that is the check carrying the 443 new test lines — approving now would attest to a result that doesn't exist yet. Lint & Static, tui-parity, Web-shell Visuals and Qwen Live Host CI are already green on this commit. Approval is deferred until CI lands green on 108f07dc4291c66ff24a5601d150d70eaa0277b9; the finalize job posts the commit-pinned approval and withholds it if anything lands red or the head moves.

中文说明

Confidence: 4/5 —— 对一个改动 Goal 状态的 PR 来说,它扎实且钉得异常牢;我发现的两个小问题都关于覆盖声明与一个无强制的重复常量,而不是代码本身有错。

退一步看。我在读 diff 之前的方案与它形状相同:把失败穿进 settle,按原因拆开建议,把停滞序列放到记录上供各界面读取。本 PR 与此一致,并在三处我会做错的地方做得更好。我会用 .slice(0, 500) 来截断诊断,从而把一个孤立代理项 bug 写进 journal 记录;它按码点切分,并测试了 astral 字符。我会在任何"不是失败"的情况下清掉诊断,那会在一次什么也没证明的 inconclusive 检查上抹掉真实诊断;它区分了写入清除保留,而这个区分是整个 diff 里最微妙的状态设计。我还会基于错误消息写一串 if/else;它把三种形状命名为带穷尽 switch 的联合类型,于是第四种形状是编译错误而不是一次 grep。

三个 reason 常量各自带着注释,说明为什么该形状得到该建议。六个月后,这就是"有人信任这个映射"与"有人重新推导它"的区别,也是我会感谢作者而不是骂作者的原因。

关于必要性:这不是为找问题而造的解法。一次真实会话烧掉约 760 万 token,并带着对它所遇失败而言错误的建议停下,记录说不出三次检查返回了什么,而所有界面都显示 Goal 在正常工作。关联 issue 是从 #11304 的评审讨论串里提出的代码级缺口,那时刻意延后了这一半。该 issue 的两半都被解决了,而且 PR 改进了 issue 自己的分类,而非照字面执行。

关于范围:我问过能否砍掉 80% 而剩下的 20% 仍能解决问题,答案是不能。只做持久化而不做界面暴露,就等于放着"Goal 停下之前没有任何界面显示 checkpoint 失败"——所报缺陷的一半——不管;而那五个界面不是可选附加项:Goal 状态在五个地方渲染,只覆盖其中三个的诊断比一个都不覆盖更糟。25 个文件的铺开是架构的成本,不是范围蔓延,而且多数界面只有几行。唯一顺手带的改动(update_goal 的重试提示)是两行文字,指向同一次事故。

维护者在合并前应当知道两件事,我都不认为构成阻断:

流程上的一句实话:这位作者有 14 个开放 PR,最近两天合并了 8 个,所以这正是门禁应当变得更怀疑、而不是变得更疲惫的那种量级。我对任何触及 packages/core/src/goals/ 的人都会跑完整的消费方枚举 —— 被改常量的每个读取方、每个清除点、记录形状的每处结构性重声明、checkpointStalls 唯一参与控制流的地方 —— 结果是干净的。这份审视是应得的,它也通过了;数量没有让我的判断偏向任何一侧。

结论:批准,延后至 CI 变绿。 单元套件(Qwen Code CI)仍在被评审的 commit 上运行,而它正是承载那 443 行新测试的检查 —— 现在批准等于为一个尚不存在的结果背书。Lint & Statictui-parityWeb-shell VisualsQwen Live Host CI 在该 commit 上已经通过。批准延后至 CI 在该 commit 上全绿;finalize 作业会发布绑定 commit 的批准,若有任何检查变红或 head 移动则不予发布。

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

Reviewed at 108f07dc4291c66ff24a5601d150d70eaa0277b9 · 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.

LGTM, looks ready to ship — CI landed green after the review. ✅

@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 at head 108f07dc4291c66ff24a5601d150d70eaa0277b9 — verdict: no Criticals, 2 Suggestions.

Suggestions

  1. packages/sdk-typescript/src/types.ts re-declares GOAL_CHECKPOINT_STALL_LIMIT = 3, duplicating packages/core/src/goals/goal-protocol.ts. This repo already has a precedent for pinning exactly this kind of duplicate — packages/cli/src/ui/commands/goal-pause-reason-wire-key.test.ts exists solely to keep GOAL_PAUSE_REASON_COMMAND in sync — and this constant has no equivalent parity test, so the two can silently drift.
  2. capGoalCheckpointFailure bounds length (GOAL_CHECKPOINT_FAILURE_MAX_CHARACTERS = 500) but not control characters, while the OpenTUI twin runs sanitizeTerminalText in transcript-view.tsx. The Ink GoalStatusMessage card renders the diagnostic raw, so a model-authored reason containing escape sequences reaches the terminal on one surface but not the other. Mitigated and not new — the pre-existing lastReason field already carries arbitrary error text down the same path — but the asymmetry is worth closing.

Basis for the no-Critical verdict

  • Bookkeeping is consistent across all three transitions: cleared when the window has room and on success, recorded on failure with or without a stall, and left untouched when the attempt produced nothing new. A stale diagnostic cannot outlive its cause.
  • Grepped every read site of the two new optional GoalSnapshotLike fields and of GoalRecord.lastCheckpointFailure?: event-adapter.ts:755, plus GoalPill.tsx, GoalStatusMessage.tsx, GoalsDialog.tsx, live-session-model.ts and mappers.ts. Every added field is consumed on at least one surface — no dead switch.
  • Verified the root-package export chain for the CLI's import of GOAL_CHECKPOINT_STALL_LIMIT: packages/core/src/index.ts:791 export * from './goals/index.js'goals/index.ts:37 export * from './goal-protocol.js'. It compiles; an earlier suspicion of a break was wrong.
  • No new daemon routes and no workspace-scoped path handling; the wire changes are mapper-only, so no route-ownership or runtime-escape question arises.
  • i18n EN/ZH parity is present for all added strings, and reducer validation accepts the new optional field without rejecting older GoalSnapshotV2/GoalRecord payloads — forward and backward compatible.
  • Re-checked the existing bot APPROVE (C=0): no inline comments, nothing in the diff contradicts it.

Agent-assisted review. Findings were re-read in the file content at the exact head SHA above before filing. Posting as a comment only — no approval implied.

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

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

  • SDK duplicate of GOAL_CHECKPOINT_STALL_LIMIT has no cross-package parity pin — already reported (comment 5619883827)
  • Stall stop reason follows only the check that spent the last stall — already reported (comment 5619883827)

Not explored to full depth (tool budget reached): "agent 1b": I did not run npm run typecheck or the packages I did not touch ( acp-bridge , sdk-typescript ), so the SDK's duplicated GOAL_CHECKPOINT_STALL_LIMIT = 3 is….

Test Plan (not a blocker): 566 tests passed — this review observed 30346, 24919, 1842, 7225, 2013, 300, 519 passed; 86 tests passed — this review observed 30346, 24919, 1842, 7225, 2013, 300, 519 passed; 105 tests passed — this review observed 30346, 24919, 1842, 7225, 2013, 300, 519 passed.

中文说明

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

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

未探索到全部深度(达到工具调用预算):"agent 1b"I did not run npm run typecheck or the packages I did not touch ( acp-bridge , sdk-typescript ), so the SDK's duplicated GOAL_CHECKPOINT_STALL_LIMIT = 3 is…

Test Plan(非阻断):566 tests passed — this review observed 30346, 24919, 1842, 7225, 2013, 300, 519 passed; 86 tests passed — this review observed 30346, 24919, 1842, 7225, 2013, 300, 519 passed; 105 tests passed — this review observed 30346, 24919, 1842, 7225, 2013, 300, 519 passed

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

Comment thread packages/cli/src/ui/components/messages/GoalStatusMessage.tsx Outdated
Comment thread packages/cli/src/ui/components/messages/GoalStatusMessage.tsx
Comment thread packages/core/src/goals/goal-protocol.ts
Comment thread packages/core/src/goals/goal-protocol.ts Outdated
Comment thread packages/core/src/goals/goal-protocol.ts Outdated
Comment thread packages/web-shell/client/components/dialogs/GoalsDialog.test.tsx
Comment thread packages/web-shell/client/components/dialogs/GoalsDialog.tsx Outdated
Comment thread packages/web-shell/client/components/dialogs/GoalsDialog.tsx
Comment thread packages/web-shell/client/daemon/session/mappers.ts
Comment thread packages/web-shell/client/i18n.tsx Outdated
@wenshao

wenshao commented Sep 10, 2026

Copy link
Copy Markdown
Collaborator

PR #11576 — local end-to-end verification

Head 108f07dc42, base ac1edef974 (merge-base with main). Everything below was run
on Linux against the built CLI, not against test doubles: headless qwen -p, the real
Ink TUI under tmux, and a real qwen serve daemon with the Web Shell, all driven by an
OpenAI-compatible mock that can fail the Goal evidence-checkpoint verifier in each of the
shapes the PR distinguishes. The A/B arm is the same worktree with the PR diff
reverse-applied and rebuilt.

Verdict

Recommend merge. Every behavioural claim in the PR reproduces end to end. Four
findings below; none of them blocks, and F1 is the one worth a follow-up because it lands
on the failure shape the PR's own evidence block advertises.

1. The declared checks

Command Result
core npx vitest run src/goals 18 files, 566 passed — matches the PR
cli GoalPill + GoalStatusMessage + live-session-model 3 files, 86 passed — matches the PR
web-shell GoalsDialog + mappers + i18n 3 files, 105 passed — matches the PR
tsc --noEmit in core, cli, web-shell clean
prettier --check + eslint on all 24 changed files clean

2. The stop reason follows the failure shape (real CLI, A/B)

Each arm drives a real Goal until three consecutive checkpoints stall and the runtime
stops it. The mock fails the verifier a different way per arm; the ladder is read back out
of the session journal (*.ladder in this branch).

Arm lastCheckpointFailure on the stopped record stop reason
PR full claim list (32 claims) checkpoint came back with a full claim list (32 claims) while the evidence window overflowed STALLEDnarrow the objective
PR claim cites an unknown source InvalidGoalCheckpointError: Goal checkpoint claim 1 cites unknown source e1 UNUSABLEthe checkpoint model is not returning usable JSON
PR provider returns HTTP 500 Error: Failed to generate text content (side-query:goal-checkpoint-verifier): 500 mock upstream failure UNREACHABLEresume once the provider is reachable
PR verifier never answers (runtime timeout) Error: Request was aborted. UNREACHABLE
base, all four arms (absent) the single pre-PR reason, narrow the objective, in every case

All four PR arms still record limitKind: "evidence_catalog", so resume behaves exactly
as before. The base arms are byte-identical to each other — that is the bug #11326
describes.

A failure that spends no stall is recorded too. With a window that still had room, a
run reached turnCount 8 carrying lastCheckpointFailure and no checkpointStalls,
exactly as the PR describes.

3. It is visible before the Goal stops

footer pill and status card before the stop

At checkpointStalls = 2 the footer pill reads checkpoint 2/3 stalled, and /goal
renders Checkpoint: 2/3 stalled · Error: Request was aborted. — while the Goal is still
running.

The same moment on base, for comparison — a Goal two stalls from being stopped looks
completely healthy:

pill A/B

And the card after the stop, base above / PR below:

card A/B

4. The daemon and the Web Shell carry it

Real qwen serve on 127.0.0.1:8576, a session created over POST /session, the Goal
driven by POST /session/:id/prompt. GET /goals returns the two fields inside the
snapshot while the Goal is still active:

{"status": "active", "turnCount": 2, "checkpointStalls": 2,
 "lastCheckpointFailure": "Error: Request was aborted.", "limitKind": null}

The Goals view rendered from that daemon — mid-streak above, after the stop below:

web shell Goals dialog

5. get_goal and resume

get_goal called in the same session after the stop returns both fields in lastGoal:

{"active": false,
 "lastGoal": {"status": "usage_limited", "turnCount": 3, "checkpointStalls": 3,
              "lastCheckpointFailure": "Error: Request was aborted.",
              "lastReason": "…the last check failed before the checkpoint verifier answered…"}}

/goal resume on the evidence-limited Goal clears both fields together with limitKind
and lastReason, and the next card carries no Checkpoint: line:

resume clears the streak and the diagnostic

6. The new tests are not vacuous — 14/15 mutants killed

One shipped line edited per mutant, only the tests that should notice are run
(mutate.log in this branch).

mutant verdict
M1 classify every non-checkpoint error as unusable KILLED (3 failed)
M2 a check that found room no longer clears the diagnostic SURVIVED — see F3
M3 a check that proves nothing drops the previous diagnostic KILLED
M4 stop reason ignores the last failure shape (pre-PR behaviour) KILLED (4 failed)
M5 a successful non-stalled checkpoint stops clearing the diagnostic KILLED
M6 edit no longer clears the diagnostic KILLED
M7 the record parser rejects any record carrying the new field KILLED
M8 the cap measured in UTF-16 units instead of code points KILLED
M9 get_goal's lastGoal summary drops the diagnostic KILLED
M10 the footer pill never shows the stall streak KILLED
M11 the ink status card never shows the Checkpoint line KILLED (3 failed)
M12 the OpenTUI card never carries the Checkpoint line KILLED
M13 the web-shell mapper drops the diagnostic again KILLED
M14 the Goals dialog never renders the Checkpoint line KILLED
M15 the English goal.checkpointFailed string drifts KILLED

Findings

F1 — Important. A verifier timeout records Error: Request was aborted., not the timeout

The PR's Evidence block advertises

"lastCheckpointFailure": "Error: Goal checkpoint verifier timed out after 180000ms"

That is what a stubbed verifier throwing the timeout directly produces. Through a real
provider it does not survive. createGoalCheckpointVerifier aborts its own
timeoutController with that Error as the abort reason, but the abort reaches the
OpenAI SDK first, which throws its own APIUserAbortError with the default message; the
reason is dropped. The record — and the Checkpoint: line the user reads — ends up saying:

Checkpoint: 3/3 stalled · Error: Request was aborted.

Live, with model.goalCheckpointTimeoutSeconds: 8, every one of the three checks recorded
that string. It does not say the check timed out, does not name the ceiling it hit, and
does not name the verifier — for the one failure shape whose whole diagnosis is how long
it waited
. The classification is still right (unreachable, correct advice), and the 500
arm's diagnostic is excellent, so this is not a blocker; but it is the shape the linked
incident is about, and it is the shape the PR body promises to explain.

One-line fix, in the try that already has a finally { clearTimeout(timer) }:

} catch (error) {
  // The provider's own abort error erases the reason we aborted for.
  if (timeoutController.signal.aborted) throw timeoutController.signal.reason;
  throw error;
} finally {
  clearTimeout(timer);
}

Worth noting as a corollary: the diagnostic's ErrorName: prefix only carries information
when the thrown class sets .name. InvalidGoalCheckpointError does; most provider
errors do not, so they all render as a bare Error:.

F2 — Nit. "The Goal status card shows a Checkpoint: line" needs one qualifier

shouldDisplayGoalStateCause returns false for 'checkpoint' (unchanged by this PR, and
shared by Ink and OpenTUI), so a stalling checkpoint never pushes a card. In a Goal that
just works — no terminal proposal, no pause — nothing appears on the card between create
and the stop, which is what I observed: three stalls accumulated with no card in the
transcript.

The claim is still true, just on demand: /goal renders the card, and it carries
Checkpoint: 2/3 stalled · … mid-streak (screenshot in §3), as does any lifecycle card
(pause, resume, verifier_reject). The always-on before-the-stop surfaces are the
footer pill and the Web Shell, both verified. Suggest softening the sentence in the PR
body and in docs/users/features/goals.md to say the pill is what changes on its own and
the card shows it whenever it is rendered.

F3 — Nit. The 'room' arm's clearing is the one untested line (M2)

finishCheckpointCheck sets health = outcome === 'room' ? 'clear' : failure. Replacing
that whole expression with failure — so a window that turned out to have room keeps a
stale diagnostic instead of retiring it — passes all 171 goal-runtime tests.

The scenario is already set up: resets the stall streak when a check needs no checkpoint at all runs two stalled turns (which leave FULL_CLAIM_LIST_FAILURE on the record) and
then a quiet turn that takes the 'room' arm. It asserts the streak is gone but not the
diagnostic. One line kills the mutant:

expect(runtime.getSnapshot().goal).not.toHaveProperty('lastCheckpointFailure');

F4 — Nit. The duplicated GOAL_CHECKPOINT_STALL_LIMIT has no drift guard

packages/sdk-typescript/src/daemon/types.ts re-declares = 3 with a comment saying it
must match core. Nothing enforces it, and the Web Shell renders the N/3 denominator from
the SDK copy while the runtime stops at core's value — so a drift shows the user a wrong
limit rather than failing a build. The PR is right that this follows the existing
GOAL_PAUSE_REASON_COMMAND precedent, which has no guard either; a single equality
assertion would cover both.


Harness

harness/ in this branch, with a README covering the gotchas. Four are worth repeating:
a mock that keeps issuing tool calls never ends a Goal turn (the run dies on the per-turn
tool-call cap instead); the window has to overflow on every turn, because a successful
checkpoint advances the cursor and resets the streak; a claim must cite a real evidence
uuid and its real proofKind, or a full_claims arm silently becomes an unusable
one; and model.goalCheckpointTimeoutSeconds makes the timeout arm finish in seconds
rather than 3 × 180 s.


中文版报告

PR #11576 —— 本地端到端验证报告

Head 108f07dc42,base ac1edef974(与 main 的 merge-base)。以下所有结论都跑在 Linux 上、
针对构建产物而不是测试替身:headless qwen -p、tmux 里的真实 Ink TUI、以及带 Web Shell 的
真实 qwen serve daemon,全部由一个 OpenAI 兼容 mock 驱动,该 mock 能按 PR 区分的每一种形状
让 Goal evidence checkpoint 校验器失败。A/B 对照是同一个 worktree 反向应用 PR diff 后重新构建。

结论

建议合并。 PR 里每一条行为声明都端到端复现了。下面 4 条发现都不阻塞合并;其中 F1 值得一个
后续 PR,因为它正好落在 PR 自己的证据块所宣传的那种失败形状上。

1. PR 声明的检查

命令 结果
core npx vitest run src/goals 18 个文件、566 通过 —— 与 PR 一致
cli GoalPill + GoalStatusMessage + live-session-model 3 个文件、86 通过 —— 与 PR 一致
web-shell GoalsDialog + mappers + i18n 3 个文件、105 通过 —— 与 PR 一致
core、cli、web-shell 的 tsc --noEmit 无错误
全部 24 个改动文件的 prettier --check + eslint 通过

2. 停机原因跟随失败形状(真实 CLI,A/B)

每个 arm 都驱动一个真实 Goal,直到连续三次 checkpoint 停滞、runtime 把它停掉。mock 在每个 arm
里用不同方式让校验器失败;下面的 ladder 是从会话 journal 里读回来的(本分支的 *.ladder)。

Arm 停机记录上的 lastCheckpointFailure 停机原因
PR 满额 claim 列表(32 条) checkpoint came back with a full claim list (32 claims) while the evidence window overflowed STALLED —— 收窄目标
PR claim 引用了不存在的来源 InvalidGoalCheckpointError: Goal checkpoint claim 1 cites unknown source e1 UNUSABLE —— checkpoint 模型没有返回可用 JSON
PR provider 返回 HTTP 500 Error: Failed to generate text content (side-query:goal-checkpoint-verifier): 500 mock upstream failure UNREACHABLE —— provider 可达后再 resume
PR 校验器始终不回应(runtime 超时) Error: Request was aborted. UNREACHABLE
base,四个 arm 全部 (没有这个字段) 四种情况都是同一条 PR 前的文案:收窄目标

四个 PR arm 仍然都记录 limitKind: "evidence_catalog",所以 resume 的行为与之前完全一致。
四个 base arm 的停机文案彼此完全相同 —— 这正是 #11326 描述的问题。

不计停滞的失败也会被记录。 在窗口仍有余量的情况下,一次运行在 turnCount 8 时记录了
lastCheckpointFailure没有 checkpointStalls,与 PR 描述一致。

3. 停机之前就能看见

停机前的底栏 pill 与状态卡

checkpointStalls = 2 时,底栏 pill 显示 checkpoint 2/3 stalled/goal 渲染出
Checkpoint: 2/3 stalled · Error: Request was aborted. —— 此时 Goal 仍在运行。

同一时刻的 base 对照 —— 一个离被停机只差一次停滞的 Goal 看起来完全健康:

pill A/B

停机之后的状态卡,上为 base、下为 PR:

状态卡 A/B

4. daemon 与 Web Shell 也带上了这两个字段

真实 qwen serve 监听 127.0.0.1:8576,通过 POST /session 建会话、POST /session/:id/prompt
驱动 Goal。Goal 仍处于 active 时,GET /goals 返回的 snapshot 里已经带上两个字段:

{"status": "active", "turnCount": 2, "checkpointStalls": 2,
 "lastCheckpointFailure": "Error: Request was aborted.", "limitKind": null}

由该 daemon 渲染出的 Goals 页面 —— 上为停滞过程中、下为停机之后:

Web Shell Goals 对话框

5. get_goal 与 resume

停机后在同一会话里调用 get_goallastGoal 里两个字段都在:

{"active": false,
 "lastGoal": {"status": "usage_limited", "turnCount": 3, "checkpointStalls": 3,
              "lastCheckpointFailure": "Error: Request was aborted.",
              "lastReason": "…the last check failed before the checkpoint verifier answered…"}}

对 evidence 受限的 Goal 执行 /goal resume,两个字段连同 limitKindlastReason 一起被清掉,
之后的状态卡不再带 Checkpoint: 行:

resume 清掉停滞计数与诊断

6. 新增测试不是空转 —— 15 个变异杀掉 14 个

每个变异只改一行发布代码,只跑应该察觉到它的那些测试(本分支的 mutate.log)。

变异 结果
M1 把所有非 checkpoint 错误都归类为 unusable 杀掉(3 个用例失败)
M2 发现窗口有余量的检查不再清掉诊断 存活 —— 见 F3
M3 什么也没证明的检查丢掉此前的诊断 杀掉
M4 停机原因忽略最后一次失败的形状(PR 前行为) 杀掉(4 个用例失败)
M5 成功且未停滞的 checkpoint 不再清诊断 杀掉
M6 edit 不再清掉诊断 杀掉
M7 记录解析器拒绝任何带新字段的记录 杀掉
M8 上限按 UTF-16 单元而非码点计量 杀掉
M9 get_goallastGoal 摘要丢掉诊断 杀掉
M10 底栏 pill 不再显示停滞计数 杀掉
M11 Ink 状态卡不再显示 Checkpoint 杀掉(3 个用例失败)
M12 OpenTUI 卡片不再携带 Checkpoint 杀掉
M13 web-shell 映射重新丢掉诊断 杀掉
M14 Goals 对话框不再渲染 Checkpoint 杀掉
M15 英文 goal.checkpointFailed 文案漂移 杀掉

发现

F1 —— Important。校验器超时记录的是 Error: Request was aborted.,而不是超时信息

PR 的证据块宣传的是:

"lastCheckpointFailure": "Error: Goal checkpoint verifier timed out after 180000ms"

那是直接抛出超时错误的桩校验器产生的结果。走真实 provider 时它不会保留下来。
createGoalCheckpointVerifier 用那个 Error 作为 abort reason 中止自己的
timeoutController,但这个中止先到达 OpenAI SDK,SDK 抛出自己的 APIUserAbortError
并使用默认文案,reason 被丢弃。最终记录里 —— 以及用户读到的 Checkpoint: 行 —— 是:

Checkpoint: 3/3 stalled · Error: Request was aborted.

实测中把 model.goalCheckpointTimeoutSeconds 设为 8,三次检查记录的都是这条字符串。它没有说
检查超时了,没有给出触发的时限,也没有指明是哪个校验器 —— 而这恰恰是唯一一种「诊断本身就是
它等了多久」的失败形状。分类仍然是对的(unreachable,建议也正确),500 那一路的诊断也非常好,
所以这不阻塞合并;但它正是所关联事故讨论的那种形状,也正是 PR 正文承诺要解释清楚的那种形状。

一行修复,放在已经有 finally { clearTimeout(timer) } 的那个 try 上:

} catch (error) {
  // provider 自己的 abort 错误抹掉了我们中止的原因。
  if (timeoutController.signal.aborted) throw timeoutController.signal.reason;
  throw error;
} finally {
  clearTimeout(timer);
}

顺带一提:诊断的 ErrorName: 前缀只有在抛出的类设置了 .name 时才带信息。
InvalidGoalCheckpointError 设置了,多数 provider 错误没有,所以它们都渲染成裸的 Error:

F2 —— Nit。「Goal 状态卡会显示 Checkpoint: 行」需要加一个限定

shouldDisplayGoalStateCause'checkpoint' 返回 false(本 PR 未改动,Ink 与 OpenTUI 共用),
所以一次停滞的 checkpoint 永远不会主动推送一张卡片。在一个只是在干活的 Goal 里 —— 没有终态
提案、没有暂停 —— 从 create 到停机之间卡片上什么也不会出现,这也正是我观察到的:三次停滞
累积过程中,transcript 里没有出现任何卡片。

这条声明仍然成立,只是需要主动触发:/goal 会渲染卡片,并且在停滞过程中确实带着
Checkpoint: 2/3 stalled · …(截图见第 3 节),任何生命周期卡片(pauseresume
verifier_reject)也一样。真正「一直在线」的停机前提示面是底栏 pill 和 Web Shell,两者都已验证。
建议把 PR 正文和 docs/users/features/goals.md 里的措辞放软一些:pill 是会自己变化的那个,
卡片则是在被渲染时会带上这一行。

F3 —— Nit。'room' 分支的清除是唯一没有被测试钉住的一行(M2)

finishCheckpointCheck 里写的是 health = outcome === 'room' ? 'clear' : failure。把整个表达式
换成 failure —— 也就是让「窗口其实还有余量」的检查保留一条陈旧诊断而不是把它退休 ——
171 个 goal-runtime 用例全部照常通过。

场景其实已经搭好了:resets the stall streak when a check needs no checkpoint at all 先跑两个
停滞回合(会在记录上留下 FULL_CLAIM_LIST_FAILURE),再跑一个走 'room' 分支的安静回合。
它断言了停滞计数被清掉,却没断言诊断。加一行就能杀掉这个变异:

expect(runtime.getSnapshot().goal).not.toHaveProperty('lastCheckpointFailure');

F4 —— Nit。复制出来的 GOAL_CHECKPOINT_STALL_LIMIT 没有防漂移保护

packages/sdk-typescript/src/daemon/types.ts 重新声明了 = 3,注释说明它必须与 core 保持一致,
但没有任何东西强制这一点;而 Web Shell 渲染 N/3 里的分母用的是 SDK 这份副本,runtime 停机用的
却是 core 的值 —— 一旦漂移,用户看到的是错误的上限,而不是构建失败。PR 说这沿用了已有的
GOAL_PAUSE_REASON_COMMAND 做法,这没错,那一处同样没有保护;一条相等断言就能同时覆盖两者。


验证工具

在本分支的 harness/,README 里记了那些坑。有四条值得重复:一个不停发工具调用的 mock 永远
不会结束一个 Goal 回合(运行会死在单回合工具调用上限上);窗口必须每个回合都溢出,因为一次
成功的 checkpoint 会推进游标并把停滞计数清零;claim 必须引用真实的 evidence uuid 并且用它
真实的 proofKind,否则一个 full_claims arm 会悄悄变成 unusable arm;以及
model.goalCheckpointTimeoutSeconds 能让超时那一路在几秒内跑完,而不是 3 × 180 秒。


Full evidence, ladders and the harness: assets-pr11576 on the wenshao/qwen-code fork.

@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: ❌ not passed — findings reported (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: 183 passed · 1 failed · 184 total

Flakiness gate: ✅ 9 changed test file(s) x 5 identical rounds, no divergence

中文 — 判定:❌ 不通过 · 报告了发现(agent 判定)

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

脚本断言:183 通过 · 1 失败 · 184 总计

抖动门:✅ 9 changed test file(s) x 5 identical rounds, no divergence

Verification report

PR #11576 — fix(goal): persist why a checkpoint failed and show it before the Goal stops

Verdict: findings — 183/184 scripted assertions passed (A/B 140/140, classification boundaries 32/32, mutation matrix 11/12). The single failed assertion is a coverage gap (M8), not a behavioural defect; no finding below is merge-blocking. The central claim is proven load-bearing by an A/B against base. Verified head: 108f07dc4291c66ff24a5601d150d70eaa0277b9 (merge base 1961e9744a4c0c4f842a760fa5e32ecc975c1913).

中文摘要
  • 结论:findings(可合并,但有值得评审注意的问题)。184 条脚本化断言通过 183 条;唯一失败是 M8 覆盖缺口(complete 状态卡隐藏 Checkpoint 行的守卫在三个前端都没有测试钉住),不是行为缺陷。
  • A/B 结论(核心主张成立且为承重改动):以 base 源码为对照(packages/core/src/goals/ 是本 PR 在 core 中唯一改动的目录),同一 harness 在两个分支上驱动 10 个场景 × 3 回合。base 对全部 7 种停机形状只给出同一条 448 字符的固定建议;head 给出 3 条不同建议(full_claims 386 / unreachable 326 / unusable 406 字符),且混合场景跟随"最后一次计入停滞的检查"。lastCheckpointFailure 在 head 上从第 1 回合起就写在记录上(base 完全没有该字段)。停滞计数、limitKind: 'evidence_catalog'、停机时机两臂完全一致(resume 行为未变)。见 01-ab-stop-reason-base-vs-head.png
  • 降级兼容的实测后果与描述不完全一致(更正):PR 说"旧版本无法解析带该字段的记录",属实;但后果不是记录丢失,而是静默回滚——base 对 head 写出的 7 条 journal 记录只能解析 2 条,selectGoalRecoveryFromRecords 回退到旧记录,把已停机的 Goal 复活成 active 且清空停滞计数(turn 1/3)。
  • Findings(均不阻塞合并):① 字节预算超限被归入 unusable,停机建议却说"检查 checkpoint 模型是否返回结构化 JSON",与记录里的诊断自相矛盾;② 任何非 InvalidGoalCheckpointError(包括运行时自己的 TypeError)都会被告知"provider 不可达";③ "单行诊断"并非单行——500 码点上限保留了 8 个换行,Ink/OpenTUI 卡片会跨行渲染(候选修复已实测:三套测试全绿、24/28 观测逐字节不变);④ complete 卡片隐藏 Checkpoint 行的守卫可达但无测试钉住。
  • 未覆盖:未跑真实 TUI 会话;未用真实 socket 驱动 checkpoint 校验器(超时/供应商错误在 GoalCheckpointVerifier 接缝注入);update_goalcheckpointRequired 提示文案改动无测试、未实测;多行诊断的渲染后果由渲染代码推断而非截屏。

Central claim and A/B

Central claim: when three stalled evidence checkpoints stop a Goal, the stop reason follows what the last stalled check ran into (full claim list / unusable answer / no answer), and the record carries a capped lastCheckpointFailure from the first failed check onward.

Control construction. git diff --name-only HEAD^1..HEAD -- packages/core shows the PR touches only packages/core/src/goals/, so the control is base's src/goals/ as a sibling directory inside the head tree (src/zz-base-goals/, deleted after the run): identical node_modules, identical src/utils/src/config, differing only by the PR's own hunks. Verified pure: the control contains 0 occurrences of lastCheckpointFailure/GOAL_CHECKPOINT_UNREACHABLE_REASON while head's goal-protocol.ts has 2. The harness imports only relative paths, so no internal workspace symlink can leak head code into the control arm.

Harness. zz-ab-harness.test.ts (kept in this artifact dir) drives the real createGoalRuntime — real reducer, real checkpoint materialisation, real journal — with only the GoalJournal/GoalEvidenceSource/GoalTurnHost/GoalCheckpointVerifier collaborators faked, exactly the seam the runtime's own tests use. 10 scenarios × 3 overflowing-window turns each; observations in head-observations.jsonl / base-observations.jsonl; comparisons in ab-compare.mjs (140 assertions).

scenario (3 turns each) base stop reason head stop reason base diagnostic head diagnostic
full_claims 448 ch, fixed 386 ch full_claims ("narrower objective") absent 92 cp
unreachable_provider_error 448 ch, fixed 326 ch unreachable ("does not fix this") absent 22 cp
unreachable_timeout 448 ch, fixed 326 ch unreachable absent 56 cp
unusable_empty_claims 448 ch, fixed 406 ch unusable ("does not fix this") absent 80 cp
mixed full,full,unreachable 448 ch, fixed 326 ch — follows the last stall absent 22 cp
mixed unusable,full,unusable 448 ch, fixed 406 ch — follows the last stall absent 80 cp
multiline_provider_error 448 ch, fixed 326 ch unreachable absent 500 cp, 8 newlines
room_failure_spends_no_stall stalls 1→1, no diagnostic stalls 1→1, diagnostic replaced absent set, no stall spent
quiet_turn_preserves_diagnostic stalls preserved stalls + diagnostic preserved absent preserved
successful_check_clears_diagnostic both cleared both cleared absent cleared

Distinct stop reasons: head 3, base 1. Stall progression, status, and limitKind: 'evidence_catalog' are identical on both arms in every scenario (the resume contract did not move). Witness: 01-ab-stop-reason-base-vs-head.png.

Cross-arm record cells (real bytes each arm wrote, parsed by the other): head↔head and head←base parse and round-trip; base←base parses; base←head rejects (parseGoalSnapshotV2undefined). Recovery over the same 7-record head chain: head recovers from the newest record (usage_limited, stalls 3); base parses only 2 of 7 and rolls back to turn 1, resurrecting the stopped Goal as active with the streak erased. See Corrections C1.

Secondary claim — visibility before the stop. The field is on the record from the first failed check (table above), which is what every surface reads. The surfaces themselves are pinned by the PR's own new tests, proven non-vacuous by mutation: M9 (pill label, 1 red), M10 (OpenTUI Checkpoint: line, 1 red), M11 (web-shell mapper, 1 red), M12 (Goals dialog, 2 red). The permitted-path get_goal payload carries both fields by construction (projectWorkerViewstructuredClone(snapshot) / summarizeSnapshot, which spreads ...goal); I verified this by reading the projection, not by a scripted assertion (see Not covered).

Targeted gates (all match the PR's own claimed counts): core vitest run src/goals 18 files / 566 passed; cli 3 files / 86 passed; web-shell 3 files / 105 passed; tsc --noEmit clean in core, cli, web-shell, sdk-typescript; eslint and prettier --check clean on all 23 changed .ts/.tsx files. Logs: gate-core-goals.log, gate-cli.log, gate-web-shell.log, gate-typecheck.log, gate-lint.log, gate-prettier.log.

Mutation matrix

Baseline for the four changed core test files: 348 passed / 0 failed. Driver: mutation-matrix.mjs (applies one hunk, runs the suites, restores from git). Witness: 02-mutation-matrix-killed-vs-survived.png.

id mutation result classification
M1 stop reason always full_claims KILLED (4 red: "stops a Goal whose verifier never answers…", "…keeps returning unusable checkpoint results", …) pinned
M2 record never keeps the diagnostic KILLED (7 red) pinned
M3 capGoalCheckpointFailure stops capping KILLED (1 red: "caps a failure diagnostic on a code point boundary") pinned
M4 an inconclusive check clears the diagnostic KILLED (1 red: "keeps the stall streak when a turn records no evidence at all") pinned
M5 delete the 'full_claims' fallback in settleIfCheckpointStalled SURVIVED (348/0) unreachable defence, not a coverage gap — see below
M6 POSITIVE CONTROL: stall limit 3→4 KILLED (6 red) harness live
M7 CANDIDATE FIX: collapse newlines in the cap SURVIVED (core 348/0, cli 86/0, ws 105/0) nothing pins the one-line axis — see F3
M8 Ink card shows Checkpoint on a complete Goal SURVIVED (86/0) coverage gap on a reachable guard — F4
M9 pill never switches to the stalled label KILLED (1 red) pinned
M10 OpenTUI card drops the checkpoint line KILLED (1 red) pinned
M11 web-shell mapper drops lastCheckpointFailure KILLED (1 red) pinned
M12 Goals dialog never renders the line KILLED (2 red) pinned

M5 adjudication (why the survivor is correct as it stands). checkpointStalls has exactly two increment sites (goal-runtime.ts:1113 and :1228); both call settleIfCheckpointStalled before persisting, and the reducer only ever clears the field. A live record therefore cannot hold checkpointStalls >= 3 without a CheckpointFailure already in hand, so the fallback's 'full_claims' is unreachable in every reachable state — deleting it changes no observable outcome, which is what the survivor shows. It is defence against an invariant break, not a test to write. The positive control (M6) landed in the same file, so the survivor is not a dead harness.

Corrections

C1 — the downgrade consequence in the Risk section is understated. The body says "A build older than this one cannot parse a transcript record that carries it, the same one-way compatibility every earlier Goal record field has had." Parsability is exactly as stated (measured: base rejects the head-written record). The consequence is not a lost record: selectGoalRecoveryFromRecords walks the journal newest-first and returns the first parseable record, so on a downgrade the Goal silently rolls back — measured on a real 7-record chain from a stalled stop: base parses 2/7, recovers status: 'active', turnCount: 1, no streak, while head recovers usage_limited / 3 stalls from the newest record. A Goal that had stopped re-arms with its stall breaker reset to zero. This is the same class of tradeoff the PR acknowledges (and checkpointStalls already had it), but the head build now writes an unparseable field into strictly more records than base did — every failed check, including checks that spend no stall — so the rollback window is wider. Not blocking: it is a downgrade-only path and rolling back is arguably safer than dropping; the report should just say what happens.

C2 — a code comment the PR makes false. packages/core/src/goals/goal-checkpoint.ts:74-77 says of InvalidGoalCheckpointError: "Nothing in production branches on the class -- the stall breaker counts by window state, not error class -- so it is a diagnostic carrier". After this PR, describeCheckpointFailure in goal-runtime.ts branches on exactly that class to choose the user-facing stop advice. The comment now contradicts the code it sits on; it should say the class also selects the advice.

Findings

F1 (Suggestion) — a byte-budget overrun is told its JSON was unusable, and that narrowing cannot help

describeCheckpointFailure is a two-way split: error instanceof InvalidGoalCheckpointError ? 'unusable' : 'unreachable'. GoalCheckpointClaimBudgetError / GoalCheckpointClaimLengthError are subclasses, so a checkpoint whose claims were well-formed but too large lands in the same bucket as a malformed answer, and the stop says: "the last check answered with output that could not be folded into claims. Narrowing the objective does not fix this. Check that the checkpoint model returns the structured JSON it is asked for, or switch models." The record's own diagnostic contradicts the advice in the same breath: GoalCheckpointClaimBudgetError: Goal checkpoint claims total 99999 bytes, over the 16000-byte budget. The JSON was structured; it was too big — and a narrower objective shrinks the window that produced those bytes, so "narrowing does not fix this" is the one claim here that is likely false.

Reproduce: cp tmp/pr11576-verify-<ts>/zz-shape-boundary.test.ts packages/core/src/goals/ && cd packages/core && npx vitest run --config tmp/pr11576-verify-<ts>/vitest.ab.config.ts src/goals/zz-shape-boundary.test.ts, then node tmp/pr11576-verify-<ts>/boundary-compare.mjs (row claim_budget_overrun). Witness: 03-classification-boundaries.png.

Direction (not measured — I did not implement it): give the budget/length subclasses their own shape whose advice names the byte/character bound instead of the model's JSON discipline. Base is not better here (it advised narrowing for every shape), so this is residue of the new split, not a regression.

F2 (Suggestion) — any non-InvalidGoalCheckpointError, including an internal bug, is reported as an unreachable provider

Same expression, other side: a TypeError thrown inside the check path stops the Goal with "the last check failed before the checkpoint verifier answered. … Resume the Goal once the provider is reachable". Measured with new TypeError("Cannot read properties of undefined (reading 'x')"): the record says TypeError: … while the advice blames the provider. Same for a non-Error throw (throw 'verifier exploded' → diagnostic verifier exploded, advice "provider is reachable"). The diagnostic is honest in both cases; the advice is not. A third bucket (or advice that does not name a cause the runtime cannot know) would keep the record's honesty and the stop's honesty aligned. Reproduce as F1 (rows runtime_typeerror, non_error_throw).

F3 (Suggestion) — the "one-line diagnostic" keeps newlines, and both terminal cards render them

capGoalCheckpointFailure trims and caps at 500 code points but never collapses line breaks. A provider error carrying a response body — the exact case the PR's own risk note cites ("provider errors can carry a response body") — lands in the journaled record with 8 newlines inside the 500 cp (measured). GoalStatusMessage.tsx renders it unsanitized in <Text wrap="wrap">, and the OpenTUI card passes it through sanitizeTerminalText, whose C0 regex (/[\x00-\x08\x0b-\x1f\x7f-\x9f]/, textUtils.ts:327) deliberately excludes \n — so the body prints across lines with the Checkpoint: label and the card indent only on the first. The type doc and the PR body both promise "a one-line diagnostic".

Candidate fix, measured: .replace(/\s*\n\s*/g, ' ') on the trimmed text in capGoalCheckpointFailure. Suites green on both sides (core 348/0, cli 86/0, ws 105/0 — which is itself the signal that nothing pins this axis), and re-running the A/B harness with it changes exactly one value: the multiline diagnostic goes 8 newlines → 0 at the same 500 cp, while the other 24 observations are byte-identical (candidate-fix-collateral.txt, 04-multiline-diagnostic-candidate-fix.png; the 4th diff line is a harness input difference — that run had no AB_FOREIGN_CHAIN — not a behaviour change). The fixture that would pin it: expect(capGoalCheckpointFailure('Error: a\n b')).toBe('Error: a b') in goal-protocol.test.ts, plus one card-render assertion. Note the sibling exposure is pre-existing: Reason: already renders lastReason, which the runtime's outer catch fills from an arbitrary error.message — this PR widens that surface with a field far likelier to be multi-line.

F4 (nit) — the complete-status suppression of the Checkpoint line is reachable and unpinned in all three surfaces

M8 deleted goal.status === 'complete' || from GoalStatusMessage.tsx and left 86/86 cli tests green; the same clause in describeGoalCard and GoalsDialog.tsx has no complete case in any new test. It is reachable, not dead: the terminal snapshot is { ...acceptedGoal, status: attempt.proposal.status } (goal-runtime.ts:901-907) and nothing clears checkpointStalls/lastCheckpointFailure on that transition, so a Goal that completes while carrying a diagnostic (e.g. a room-failure check, then a verified proposal) persists both on its complete record — without the guard the "Goal complete" card would show "Checkpoint: 1/3 stalled · …". One complete-with-diagnostic case per surface would pin it. Not a merge condition.

F5 (nit) — C2 above; a stale comment now contradicts the code it documents.

Not covered

  • No real sockets. Timeouts and provider errors were injected at the GoalCheckpointVerifier seam (the same seam the runtime's own tests use), so a genuine timeout error object and a genuine provider error body were never observed. The multi-line fixture in F3 is synthesized to the shape the PR's risk note describes; whether a real provider's error text actually contains newlines is unmeasured.
  • No interactive/TUI session. The multi-line rendering consequence in F3 is inferred from the render code plus the measured string, not from a rendered frame; terminal-capture was not run.
  • The checkpointRequired hint change (the update_goal nextAction wording, secondary claim) has no test and was verified only by reading the string. The static get_goal tool description was not updated to match the new wording, so a model reading only the schema still gets the weaker instruction.
  • Permitted-path get_goal payload carries both fields by construction (projectWorkerView clones the whole goal record); no scripted assertion covers it.
  • The SDK's duplicated GOAL_CHECKPOINT_STALL_LIMIT = 3 is pinned only indirectly (GoalsDialog tests assert the literal 2/3); there is no equality check against core's constant.
  • Merge into current main was not attempted; the CI merge ref (HEAD^1 = 1961e97) is what the A/B used.
  • Windows/macOS not exercised (Linux container only), matching the PR's own tested-on table.
  • The full_claims fallback's unreachability (M5) is a static census of every writer of checkpointStalls, not an instrumented run.

Methodology

Single round on the CI merge-ref checkout (HEAD = a33904a, base tip HEAD^1 = 1961e97, PR head HEAD^2 = 108f07d, depth-2 shallow). The A/B control is base's src/goals/ as a sibling directory in the head tree, proven pure by symbol census and by the PR's core diff being confined to that directory; both arms ran the byte-identical harness zz-ab-harness.test.ts (plus zz-shape-boundary.test.ts for the classification boundaries) through vitest against real source, writing per-turn observations to head-observations.jsonl / base-observations.jsonl / boundary-*.jsonl; ab-compare.mjs and boundary-compare.mjs turn those into the 172 A/B and boundary assertions, and mutation-matrix.mjs produces the 12 matrix rows (each mutation applied to a scratch copy and restored with git checkout --). Gates ran on the unmodified tree after the scratch files were deleted; git status was clean at report time. Raw logs, harnesses, observation streams and the four captures are in this directory; the captures were produced with scripts/verify-capture.mjs.

Flakiness gate log

rounds=5 files=9 skipped=0
file packages/cli/src/ui/components/GoalPill.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/components/GoalPill.test.tsx
file packages/cli/src/ui/components/messages/GoalStatusMessage.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/components/messages/GoalStatusMessage.test.tsx
file packages/cli/src/ui/opentui/live-session-model.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/opentui/live-session-model.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/web-shell/client/components/dialogs/GoalsDialog.test.tsx: (cd packages/web-shell) npx --no-install vitest run ./client/components/dialogs/GoalsDialog.test.tsx
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/ui/components/GoalPill.test.tsx: PPPPP
  packages/cli/src/ui/components/messages/GoalStatusMessage.test.tsx: PPPPP
  packages/cli/src/ui/opentui/live-session-model.test.ts: PPPPP
  packages/core/src/goals/goal-protocol.test.ts: PPPPP
  packages/core/src/goals/goal-reducer.test.ts: PPPPP
  packages/core/src/goals/goal-runtime.test.ts: PPPPP
  packages/core/src/goals/goal-tools.test.ts: PPPPP
  packages/web-shell/client/components/dialogs/GoalsDialog.test.tsx: PPPPP
  packages/web-shell/client/daemon/session/mappers.test.ts: PPPPP

verdict: pass
summary: 9 changed test file(s) x 5 identical rounds, no divergence

--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/ui/components/GoalPill.test.tsx: P (exit 0)
round 1 · packages/cli/src/ui/components/messages/GoalStatusMessage.test.tsx: P (exit 0)
round 1 · packages/cli/src/ui/opentui/live-session-model.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/web-shell/client/components/dialogs/GoalsDialog.test.tsx: P (exit 0)
round 1 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 2 · packages/cli/src/ui/components/GoalPill.test.tsx: P (exit 0)
round 2 · packages/cli/src/ui/components/messages/GoalStatusMessage.test.tsx: P (exit 0)
round 2 · packages/cli/src/ui/opentui/live-session-model.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/web-shell/client/components/dialogs/GoalsDialog.test.tsx: P (exit 0)
round 2 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 3 · packages/cli/src/ui/components/GoalPill.test.tsx: P (exit 0)
round 3 · packages/cli/src/ui/components/messages/GoalStatusMessage.test.tsx: P (exit 0)
round 3 · packages/cli/src/ui/opentui/live-session-model.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/web-shell/client/components/dialogs/GoalsDialog.test.tsx: P (exit 0)
round 3 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 4 · packages/cli/src/ui/components/GoalPill.test.tsx: P (exit 0)
round 4 · packages/cli/src/ui/components/messages/GoalStatusMessage.test.tsx: P (exit 0)
round 4 · packages/cli/src/ui/opentui/live-session-model.test.ts: P (exit 0)
round 4 · packages/core/src/goals/goal-protocol.test.ts: P (exit 0)
round 4 · packages/core/src/goals/goal-reducer.test.ts: P (exit 0)
round 4 · packages/core/src/goals/goal-runtime.test.ts: P (exit 0)
round 4 · packages/core/src/goals/goal-tools.test.ts: P (exit 0)
round 4 · packages/web-shell/client/components/dialogs/GoalsDialog.test.tsx: P (exit 0)
round 4 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)
round 5 · packages/cli/src/ui/components/GoalPill.test.tsx: P (exit 0)
round 5 · packages/cli/src/ui/components/messages/GoalStatusMessage.test.tsx: P (exit 0)
round 5 · packages/cli/src/ui/opentui/live-session-model.test.ts: P (exit 0)
round 5 · packages/core/src/goals/goal-protocol.test.ts: P (exit 0)
round 5 · packages/core/src/goals/goal-reducer.test.ts: P (exit 0)
round 5 · packages/core/src/goals/goal-runtime.test.ts: P (exit 0)
round 5 · packages/core/src/goals/goal-tools.test.ts: P (exit 0)
round 5 · packages/web-shell/client/components/dialogs/GoalsDialog.test.tsx: P (exit 0)
round 5 · packages/web-shell/client/daemon/session/mappers.test.ts: P (exit 0)

Evidence images

01-ab-stop-reason-base-vs-head

02-mutation-matrix-killed-vs-survived

03-classification-boundaries

04-multiline-diagnostic-candidate-fix

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

Qwen Code · sandboxed verification

…-failure-cause

# Conflicts:
#	packages/core/src/goals/goal-protocol.test.ts
#	packages/core/src/goals/goal-tools.ts
#	packages/web-shell/client/daemon/session/mappers.test.ts
- Read claim-budget and claim-length overruns as capacity failures that
  keep the narrow-the-objective advice, and rename that shape to capacity.
- Reword the no-answer stop so it names provider, timeout and check errors
  instead of blaming the provider, and rethrow the verifier's own timeout
  when a provider SDK replaces it with a generic abort error.
- Write the diagnostic as one display-safe line (control sequences and
  bidi overrides removed, whitespace collapsed before the cap) and sanitize
  it again at the Ink card and the web Goals dialog.
- Share one visibility rule across every surface and lastGoal: never on a
  completed Goal, always during a stall streak, and a stall-free failure
  only while active; scope the diagnostic on the other checkpoint stops.
- Show the stall streak in the web Goal status strip, add a tooltip to the
  dialog row, and move the dialog copy away from "last check".
- Pin the SDK stall limit to core, the room-arm clear, the call-site cap,
  subclass classification and the retry hint in tests; restate the
  InvalidGoalCheckpointError contract and update the Goal docs.
@qqqys

qqqys commented Sep 11, 2026

Copy link
Copy Markdown
Collaborator Author

本轮评审与验证意见已在 a9e428c 处理,逐条线程的处置写在各自线程上。下面是不在线程里的几组意见。

@wenshao 本地端到端验证

  • F1(超时记录成 Request was aborted.)已修。 createGoalCheckpointVerifier 在自身超时触发时抛出超时原因,调用方主动中断时保留原错误,所以诊断会写明 Goal checkpoint verifier timed out after …ms。测试:goal-checkpoint-verifier.test.ts「reports the timeout when the provider answers the abort with its own error」,mock 的 provider 在 abort 时抛自己的 Request was aborted.ErrorName: 前缀对多数 provider 错误只是 Error: 这一点保留现状,消息本身已经足够说明。
  • F2(卡片是按需渲染的)已改文档。 docs/users/features/goals.md 现在写明:底栏 pill 和 web shell 状态条会自己变化;状态卡在被渲染时(/goal、暂停、恢复、校验器卡片)显示 Checkpoint: 行。PR 正文同步。
  • F3(room 分支清除没有测试)已补,见 R1-8 线程。
  • F4(SDK 复制常量没有防漂移)已补。 新增 packages/cli/src/ui/commands/goal-checkpoint-stall-limit-wire-key.test.ts,沿用 goal-pause-reason-wire-key.test.ts 的做法,断言 SDK 与 core 的 GOAL_CHECKPOINT_STALL_LIMIT 相等。

沙箱验证(C1、C2、F1–F5、未覆盖项)

  • C1(降级后会回滚)接受,已改 PR 正文。 Risk 部分现在写明:旧版本解析不了带新字段的记录,恢复时会退回到更早可解析的记录,已停机的 Goal 可能以 active、停滞计数清零的状态重新运行;新字段在每次失败检查时都会写,所以这个窗口比 checkpointStalls 更宽。只影响降级路径。
  • C2 / F5(goal-checkpoint.ts 注释失真)已改,见 R1-6 线程。
  • F1(字节预算超限被判为不可用输出)已改,见 R1-3 线程:预算与长度超限归入 capacity,保留收窄目标的建议。
  • F2(内部异常被说成 provider 不可达)已改,见 R1-4 线程:UNREACHABLE 文案不再断定是 provider,列出 provider、超时、检查自身出错三种原因。
  • F3(诊断里有换行)已改,见 R1-5 线程,落盘时折叠空白并去掉控制字符。
  • F4(完成态抑制没有测试)已补,见 R1-1 线程。
  • 未覆盖项里的 update_goal 工具描述已同步:静态描述现在也写明 checkpointRequired 之后先 get_goal、在运行其他工具之前重试;goal-tools.test.ts 的 nextAction 断言钉住了这句。permitted 路径的 get_goal 本来就克隆整个记录,这次没有另加断言。

@doudouOUC 评审

  • SDK 常量同步测试:已补,同上 F4。
  • Ink 卡片未清理控制字符:已改。落盘时统一清理,Ink 卡片渲染时再过 sanitizeTerminalText,web shell 对话框用 sanitizeControlChars,见 R1-2、R1-12 线程。

triage 第一轮的两点

  • 测试计划里 client/i18n.test.ts 的说法:该文件并不校验中英键齐全,PR 正文已改为只说新键在两种语言里都有,并注明没有自动化的键对齐检查。
  • 停机原因只跟随最后一次停滞:保持现状。最后一次检查是当前窗口最新的证据,诊断字段就在同一条记录上。

另外本次合入了最新 main,解决了与 #11457(回合与时长预算)在 goal-tools.tsgoal-protocol.test.tsmappers.test.ts 的冲突,两边字段都保留。

English summary

Addressed in a9e428c; per-thread dispositions are on each review thread. Outside the threads: wenshao F1 fixed (the verifier rethrows its own timeout reason when a provider SDK replaces it with "Request was aborted."), F2 docs now say the pill and the web status strip change on their own while the card shows the line whenever it renders, F3 and F4 pinned (room-arm clear assertion, new core/SDK stall-limit parity test). Sandbox C1 accepted and the Risk section now states the downgrade rollback; C2/F5 comment rewritten; F1 budget/length overruns now read as capacity; F2 the unreachable reason no longer blames the provider; F3 diagnostics are collapsed to one line with control characters removed; F4 completed-status suppression pinned; the static update_goal description matches the new nextAction wording. doudouOUC: parity test added and sanitization applied at write time and at the Ink and web render sites. Triage: the test plan no longer claims i18n.test.ts checks key parity; the stop reason still follows the last stall by design. Merged latest main and resolved the #11457 conflicts keeping both sides.

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

  • Mixed-streak stall stop advice follows only the last stalled check — already reported (issue comment 5619883827); kept by the author as design

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)": did not verify how packages/cli/src/ui/utils/export/export-transcript-document.ts renders a goal_state item (it only lists the type at line 418 and the diff….

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

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

  • packages/cli/src/ui/components/GoalPill.tsx:113 — [probe] footer-pill stall-label precedence over the non-active status labels is not pinned by any test
  • packages/cli/src/ui/opentui/transcript-view.tsx:542 — [probe] opentui render-layer sanitizeTerminalText on the checkpoint line has no test
  • packages/core/src/goals/goal-protocol.ts:127 — [probe] always-show streak rationale is falsified by the checkpoint_request stop whose resume clears the streak
  • packages/core/src/goals/goal-reducer.test.ts:939 — [probe] budget-resume keep of streak and diagnostic is exercised by no test carrying either field
  • packages/web-shell/client/daemon/session/mappers.test.ts:1106 — [probe] new mapper pin comment names the Goals dialog, which reads GET /goals and never this mapper
中文说明

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

本轮确认的 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 2)"did not verify how packages/cli/src/ui/utils/export/export-transcript-document.ts renders a goal_state item (it only lists the type at line 418 and the diff…

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

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

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

Comment thread packages/cli/src/ui/components/messages/GoalStatusMessage.tsx Outdated
Comment thread packages/core/src/goals/goal-protocol.ts
Comment thread packages/core/src/goals/goal-runtime.ts Outdated
Comment thread packages/core/src/goals/goal-runtime.ts
Comment thread packages/core/src/goals/goal-runtime.ts
Comment thread packages/web-shell/client/components/GoalStatusStrip.module.css Outdated
Comment thread packages/web-shell/client/components/GoalStatusStrip.test.tsx
Comment thread packages/web-shell/client/components/GoalStatusStrip.tsx Outdated
Comment thread packages/web-shell/client/components/dialogs/GoalsDialog.tsx Outdated
Comment thread packages/web-shell/client/components/dialogs/GoalsDialog.tsx Outdated
…t health surfaces

- describeCheckpointFailure reads GoalCheckpointClaimCountError as
  capacity, now that the count error has landed on main.
- goalCheckpointHealthVisible also shows the failure that stopped a Goal
  whose checkpoint request was too large; the Goals dialog copy follows,
  and gates on the raw diagnostic as core does.
- Headless /goal text output prints the checkpoint line.
- The request-too-large error names the measured size.
- Docs (get_goal description, record field, SDK type, goals.md, strip
  comment) state when the diagnostic is cleared and where the streak shows.
- The strip's stall pill shrinks on narrow panes instead of overlapping
  the actions.
- Tests pin the bare streak, the default clear on other checkpoint stops,
  every capacity subclass, the active stall-free summary, the stopped
  strip streak, and the dialog's complete and control-only cases.

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

Agent-assisted review at baa55da48de5e6ba47b82f1d836600133e2fea1f — no confirmed Criticals in the current static scope.

The head moved during review. I withheld the old review, fetched the new complete 33-file diff against 518f6795f9eda4b9911c2a1e8ea76ddc787551cb and refreshed pinned source/history, then inspected the production/test delta and its integration with the new base. Scope includes checkpoint verifier/runtime/reducer paths and the diagnostic's consumers in get_goal, headless TEXT, Ink, OpenTUI, SDK types and WebShell. This is a bugfix, not a policy-gated refactor. Author maintainer status was not independently established.

Prior review: our two Suggestions at 108f07d are addressed: the cross-package stall-limit parity test exists, and diagnostics are cleaned both on production and at terminal display. The fetched history contains Suggestions, not a standing Critical. Existing deferrals remain non-blocking; I am not asking for additional scope.

Key checks:

  • packages/core/src/goals/goal-runtime.ts distinguishes clear, preserve and replace; records failures on non-overflow/replay paths without spending a stall; journals health with the stopped snapshot; and retains the attempt/abort guards. Reducer controls clear health beside the stall count and the parser round-trips the optional string.
  • goal-checkpoint-verifier.ts:682-694 restores its own timeout reason only when the caller has not aborted. The refreshed runtime includes the new base's claim-count capacity subclass, and the oversized-request diagnostic now names its measured size. Mixed-streak advice intentionally follows the final check.
  • The refreshed visibility rule displays a stall-free checkpoint_request failure, while hiding stale health after completion. The headless formatter now carries the same line; WebShell's copied gate agrees for valid records. New optional-field producers/readers were traced in pinned source.
  • No new or changed daemon route is introduced: this extends the existing session-owned Goal snapshot, not workspace/service selection. The Goals listing forwards the snapshot intact.

Limitations: static review only; I ran no PR code, tests, builds, browser/terminal sessions or workflows. Parsing a persisted diagnostic checks its type, not a new security boundary or downgrade guarantee; older strict record parsers can reject the extended record. Third-party execution claims were not treated as my own test results.

Comment only; no approval implied.

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

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

Not reviewed: build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI for this fork PR; the suites ran on Linux only.

Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.

Test Plan (not a blocker): core/src/code-mode/host.tsno such file or directory; 652 tests passed — this review observed 30796, 25471, 1997, 7354, 2025, 1016, 535 passed; 257 tests passed — this review observed 30796, 25471, 1997, 7354, 2025, 1016, 535 passed; 127 tests passed — this review observed 30796, 25471, 1997, 7354, 2025, 1016, 535 passed.

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

  • packages/cli/src/ui/components/GoalPill.test.tsx:145 — [probe] test name contradicts the verifying-over-stall precedence it is the sole pin for
  • packages/core/src/goals/goal-checkpoint-verifier.test.ts:1089 — [probe] caller-abort case never reaches the !attemptSignal?.aborted clause, so deleting it stays green
  • packages/cli/src/ui/components/GoalPill.tsx:117 — [probe] wider stall label overflows the never-compressed footer right section at 80-83 columns
  • packages/core/src/goals/goal-checkpoint-verifier.ts:688 — [probe] new timeout catch discards the provider error it displaces, with no cause attached
  • packages/cli/src/ui/opentui/live-session-model.ts:663 — [probe] GoalCardView.checkpoint doc says 'when either is set' but the value is predicate-gated

Convergence: round 3 posted 6 inline comment(s), 6 of them reported for the first time; the previous round posted 16 (16 new). Findings keep coming back to the same files: packages/web-shell/client/components/dialogs/GoalsDialog.tsx (findings in rounds 1, 2; 1 more now); packages/sdk-typescript/src/daemon/types.ts (findings in round 2; 1 more now); packages/web-shell/client/components/GoalStatusStrip.module.css (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.)

中文说明

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

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

未审查(原文为英文):build-and-test — Test (macos-latest, Node 22.x) and Test (windows-latest, Node 22.x) were skipped in CI for this fork PR; the suites ran on Linux only.

未审查:反向审计——在 5 轮的反审轮数上限内未收敛。

Test Plan(非阻断):core/src/code-mode/host.tsno such file or directory; 652 tests passed — this review observed 30796, 25471, 1997, 7354, 2025, 1016, 535 passed; 257 tests passed — this review observed 30796, 25471, 1997, 7354, 2025, 1016, 535 passed; 127 tests passed — this review observed 30796, 25471, 1997, 7354, 2025, 1016, 535 passed

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

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

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

Comment thread packages/cli/src/nonInteractiveCli.ts Outdated
Comment thread packages/web-shell/client/components/dialogs/GoalsDialog.tsx Outdated
Comment thread docs/users/features/goals.md Outdated
Comment thread packages/cli/src/nonInteractiveCli.ts Outdated
Comment thread packages/sdk-typescript/src/daemon/types.ts Outdated
Comment thread packages/web-shell/client/components/GoalStatusStrip.module.css
…f its rule

- goalCheckpointHealthLine in core builds the checkpoint line the Ink card,
  the OpenTUI card and headless /goal text print, with an optional cleaner
  for callers that write straight to a terminal.
- The Web Shell's copy of goalCheckpointHealthVisible moves into goalGate,
  and a drift test runs core's own function body against it over every
  combination of the fields it reads.
- Headless /goal text and the Ink card sanitize the stop reason, which can
  embed a raw provider error.
- The status strip's stall label doubles as its tooltip, since it is
  ellipsized on narrow panes.
- The record field, SDK type and get_goal description say the diagnostic
  can also be the full-claim-list phrase; goals.md scopes the card claim to
  terminal cards.
@qqqys
qqqys enabled auto-merge September 11, 2026 17:40

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

APPROVE — no Critical. CI is green at f51ccf95, all 37 review threads are resolved, and the round-3 ledger carries only Suggestion-severity findings (shared-helper duplication at three CLI render sites, and the bare-streak shape in GoalStatusMessage.tsx). I did not re-review 1920 lines that three rounds have already walked; I read the two places where a defect would be silent and user-facing.

The diagnostic is sanitized at the write boundary, not trusted to each reader

capGoalCheckpointFailure (goal-protocol.ts:113-121) runs stripTerminalControlSequences then stripDisplayControlChars, collapses \s+ to a single space, trims, and caps by code point via [...oneLine] rather than .length — so the cap cannot land mid-surrogate-pair and leave a lone surrogate in a journaled string that every surface later renders. Collapsing before capping is the right order, and the docblock says why: the budget should spend its code points on the message rather than on a response body's indentation.

The architectural choice is the part worth calling out. The docblock states the value "is journaled, handed to the model, and rendered on every Goal surface, so it is cleaned once where it is written rather than trusted to each reader." For a string whose origin is a provider error body — i.e. content this process did not author — cleaning at the single write site is materially safer than N read sites each remembering to clean, and the terminal render sites clean again anyway (nonInteractiveCli.ts passes lastReason through sanitizeTerminalText before goalCheckpointHealthLine). Persistence growth is bounded by the same 500-code-point cap, one field overwritten per check rather than appended, so a long stall streak cannot grow the record.

The web strip's divergence from the shared gate is deliberate, and I want it on the record

GoalStatusStrip.tsx does not call goalCheckpointHealthVisible; it gates the stall badge on checkpointStalls > 0 at :115. That looks like a missed gate, so I traced it, and the two cannot disagree:

  • GoalStatusStrip.tsx:68 is if (!goal || goal.status === 'complete') return null; — the entire strip is absent for a completed Goal, which is exactly the case the gate excludes at goal-protocol.ts:141.
  • For every non-complete status with a streak, the gate returns true at :142 and the strip shows the badge. Same answer.
  • Where the strip is narrower — a lastCheckpointFailure with no stall, which the gate can show when limitKind === 'checkpoint_request' or while active — the in-code comment at :73-76 states the intent: the streak shows "where a daemon-session user is already looking", and "the failure text itself is left to the Goals dialog, which has room for it."

So this is a documented presentation narrowing, not a gate bypass. Recording it because the shape invites a well-meaning future edit that routes the strip through the shared gate and thereby adds failure text to a narrow pane, or that removes the :68 early return and actually creates the divergence this currently avoids. If the intent is ever "show it whatever the status, including complete", that belongs in core's gate so the three surfaces cannot drift — goalGate.drift.test.ts pins the dialog's copy but nothing pins the strip's complete behaviour.

Also checked

The interaction with #11578 is correct: the persisted detail comes from the error that survived the verifier's corrective retry, i.e. the post-stripping outcome, and recordCheckpoint's success arm sets health = 'clear', which drops both the streak and the diagnostic. A retry that later succeeds therefore cannot leave a stale reason on screen. The control-flow change is additive and non-blocking — settleIfCheckpointStalled builds the record with withCheckpointHealth and passes it plus goalCheckpointStalledReason(shape) into the pre-existing settleCheckpointFailure, so the diagnostic lands in the same snapshot as the terminal status and is emitted with the stop rather than after teardown; both helpers are pure and non-throwing, and shape falls back to 'capacity', so nothing new can prevent a Goal from stopping.

The new field degrades safely on an old or malformed record: it is optional in parseGoalRecord's key list and type-validated, with reducer tests pinning '' and 42.

Non-blocking

One thing I could not establish: whether timeoutController.abort(...) in goal-checkpoint-verifier.ts carries a descriptive reason. The new catch rethrows timeoutController.signal.reason, and the arming code is outside this diff, so if the timer aborts without a message the persisted detail degrades to a generic AbortError: This operation was aborted instead of naming the timeout. Harmless — GOAL_CHECKPOINT_UNREACHABLE_REASON already lists the timeout as a possible cause — but passing a reason to abort() would make the persisted diagnostic self-explanatory, which is the point of the change.

I'd second the round-3 Suggestion about the checkpoint label and join format being hand-built at three CLI render sites, for the ordinary reason: this PR's stated purpose is that every surface agrees, and three hand-built copies of the format are how surfaces stop agreeing.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

APPROVE

已核对 head f51ccf95f7f5ddaacc30a94ff5564a491f2a058d(vs merge-base 518f6795f9)。

历史项:本 PR 的 35 条线程全部是 Suggestion,无 Critical;最后一轮(R3-1…R3-5 与再锚定的 R1-4/R1-9/R1-10/R2-9)就落在当前 head f51ccf95f7,逐条对该 head 复核确认已落地:

  • R3-1/R1-4:core 新增 goalCheckpointHealthLine()goal-protocol.ts),终端侧(headless formatGoalStateCheckpoint: 行、GoalStatusMessageGoalPill)统一取这一份措辞,clean 参数由「直接写终端的调用方」传入 sanitizeTerminalText,自行清理渲染文本的调用方不传,避免二次转义。
  • R3-2:web-shell 的门控复制抽到 client/utils/goalGate.ts,并配 goalGate.drift.test.ts(用 core 自己的函数体跑这份拷贝,任一侧加分支而另一侧没加即失败)。
  • R3-3:docs/users/features/goals.md 的表述已限定为「终态 Goal 停止时」这一条件。
  • R3-4:headless 路径的 Reason:Checkpoint: 两行都做 sanitize,注释写明原因(暂停原因里可能带原始 provider 错误)。
  • R3-5/R2-8:SDK daemon/types.ts 与 core goal-protocol.ts 的字段/常量对齐,并有 goal-checkpoint-stall-limit-wire-key.test.ts 钉住线上传 key。

独立复查未发现 Critical

  • 分类顺序正确:claim-count / claim-budget / claim-length 三个子类在基类 InvalidGoalCheckpointError 之前判定(goal-runtime.tsdescribeCheckpointFailure),provider/传输类错误落在 unreachable,不会把 provider 故障说成「输出畸形」。
  • 记录可见性自洽:goalCheckpointHealthVisible 的四条规则(complete 永不显示;stall 计数 >0 总显示;checkpoint_request 显示那条失败;其余仅在 active 显示)与 withCheckpointHealth 的三种更新(clear / 失败详情 / undefined 保持原值)一致;settleIfCheckpointStalled 在 streak 到限但没有失败详情时回落 capacity
  • 日志/记录体积有界:capGoalCheckpointFailure 先去终端控制序列与默认忽略码位、再折叠空白、最后按码点截到 500,写入处清一次而非每个读方各自信任。
  • 持久化契约不变差:parseGoalRecordhasOnlyKeys 白名单已加入新字段,空串视为非法;旧 journal(无该字段)照旧可读。

本地验证packages/core/src/goals/** 在该 head Tests 582 passed (582),同一环境同一配置在 merge-base 518f6795f9554 passed (554),两边失败的测试文件完全相同(goal-tools.test.tsgoalLoop.integration.test.ts,本机 node_modules 缺 @modelcontextprotocol/client,与本 PR 无关)——即本 PR 净增 28 条用例全绿、未引入新失败。变异复核:把 settleIfCheckpointStalled 的形状写死成 capacity → 5 条失败;让 withCheckpointHealth 永不写入 detail → 14 条失败;把三个 claim 子类并入 unusablereads a claim-count/budget/length overrun as capacity, not as unusable output 三条具名用例失败。

CI:required 全部 success(Test (ubuntu-latest, Node 22.x)Lint & StaticIntegration Tests (no-AK, No Sandbox)web-shell E2E Smoke,另有 TUI parity snapshotsOpenTUI no-flicker gate),无失败无 pending(review-pr 为评审机器人自身档)。

一条留给后续的观察(不阻塞):journal 采用 hasOnlyKeys 严格白名单,因此带新字段的记录在降级到旧版本时会整条被判非法;这与该文件既有字段的处理方式一致,但降级路径的取舍值得单独记一笔。

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

Labels

review/self-reported The linked issue was opened by the PR author (self-reported)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug(goal): a stalled-checkpoint stop cannot say which failure stopped it, and no surface shows a failing checkpoint before it does

7 participants