feat(goal): account the tokens a Goal spends - #9301
Conversation
A Goal reported how many turns it had run and how long it had been active, but never what it cost. That is the one number a user needs to decide whether an autonomous run is worth continuing, and the one number every future limit has to be expressed in — a budget cannot be enforced against a figure nobody keeps. `GoalRecord` now carries `tokensUsed`, summed across the Goal's turns by `reduceGoalTurnFinished`, and `get_goal` reports it in the unpermitted `lastGoal` summary alongside the turn count. The figure is the same one `/stats` shows, read from the session's own model metrics, so a Goal's spend and the session's spend are one measurement rather than two definitions. The runtime pulls the reading rather than having hosts push it. `finishTurn` is called from three separate hosts on the normal path and from core on the interrupted paths, so a pushed count would have to be threaded through four call sites and would go missing wherever it was forgotten; a meter injected once at construction cannot be. A session with no meter, or a meter that throws, bills the turn zero rather than guessing, and never fails the turn. No limit is introduced here — this only counts. Goals recovered from a transcript written before the field existed restore with zero spend. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Thanks for the continued work on this! Template looks good ✓ Problem: unchanged from the first pass and still real — a Goal reports Direction: aligned. It reuses the session's existing model metrics rather than introducing a second accounting system, and reads telemetry without changing how telemetry is collected. Size: this is what changed since the last pass. Eight review rounds grew the PR from ~93 production lines to 552 production logic lines (540+ / 12−) across 11 production files, plus 1,280 test/fixture lines across 28 more. The growth is real scope, not churn — every increment lands a correctness fix the review surfaced: the meter must open on restored totals after Approach: the ripple is wide, but each wave breaks on a concrete failure mode with a test next to it, and I did not find a materially simpler construction that keeps the "one measurement, one wiring point" invariant. The deferred-activation and snapshot/rollback machinery is the price of making the number right across session swaps, and the PR pays it once per concern, in one place. Risk: no elevated risk signals — none of the changed production files match the revert-correlated paths. Moving on to code review. 🔍 中文说明感谢持续打磨！ 模板完整 ✓ 问题：与首轮判断一致，依然是真实缺口——Goal 报告 方向：对齐。复用会话已有的模型指标，不引入第二套记账系统；只读取遥测，不改变遥测的采集方式。 规模：这是与上一轮相比的变化所在。 经过八轮审查，PR 从约 93 行生产代码增长到 552 行生产逻辑（540 增 / 12 删）、11 个生产文件，另有 28 个文件共 1,280 行测试/fixture。增长是真实范围而非杂散改动——每一块增量都落实了审查中暴露的正确性修复： 方案：波及面广，但每一波都落在一个具体的失败模式上，且旁边就有测试；我没有找到在保持"一次测量、单点接线"不变量前提下明显更简单的构造。延迟激活与快照/回滚机制是让这个数字在会话切换下正确的代价，PR 在每个关注点上只付一次、且集中在一处。 风险：无升级风险信号——改动的生产文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal for this problem — reuse the session's existing model metrics, inject a meter once where the runtime is constructed, sample it at permit issue and turn finish, accumulate the delta in the reducer, fail closed to zero, migrate the field at the parse boundary — is exactly what the PR does, so this pass was about whether the wiring holds up after eight review rounds and eleven fix commits. On the current head, statically, it does:
Two informational nits, neither a change request: the "same figure The two paths that had to stay straight: sequenceDiagram
participant P1 as CLI resume and branch hooks
participant P2 as Config
participant P3 as UiTelemetryService
participant P4 as Goal runtime
Note over P1,P4: Swap path - resume or branch
P1->>P2: startNewSession - rebuilds runtime, clears marker
P1->>P3: snapshotForReplay, then replay stored events
P1->>P2: markUiTelemetryEventsReplayed - one-shot marker
P1->>P4: waitForGoalRuntime - meter opens on restored totals
P1->>P2: client initialize - consumes marker, skips second replay
Note over P2,P4: Cold start path - resume via CLI flag
P2->>P4: prepareRestore - activation held back
P2->>P3: client init replays stored telemetry
P2->>P4: startDeferredGoalRestore - first permit opens on restored totals
File map, in brief: five goal files carry the feature itself (protocol field, reducer accumulation + parse migration, runtime meter lifecycle, TestingThis is an unattended CI run — no PR code was built or executed here; the evidence below is the PR's own CI, fetched through the API. The build break that red-lined the last pass (
Sandboxed verification would settle this: Not verified: live telemetry wiring end-to-end (meter mocked in all unit tests); macOS/Windows behavior (merge-queue lanes, covered by CI policy once the PR queues). 中文说明代码审查我对这个问题的独立方案——复用会话已有的模型指标、在运行时构造处一次性注入 meter、在发放与结束轮次许可时采样、在 reducer 中累加差值、失败一律向零收敛、在解析边界迁移字段——与 PR 的做法完全一致，因此本轮审查聚焦于：经过八轮审查与十一个修复提交后，接线是否依然成立。在当前 head 上，静态审查结论是成立的：
两条仅供参考的小问题，均非修改要求："与 两条必须分清的路径见上方时序图（swap 路径与冷启动路径）。 文件分布简述：五个 goal 文件承载特性本体（协议字段、reducer 累加与解析迁移、运行时 meter 生命周期、 测试本轮为无人值守 CI 运行——此处未构建或执行任何 PR 代码；以下证据是 PR 自己的 CI，通过 API 获取。上一轮标红的构建失败（acp-bridge replay fixture 缺 沙箱验证可以收口这一点： 未验证：真实遥测接线的端到端行为（所有单测均 mock meter）；macOS/Windows 行为（合并队列流水线，待 PR 入队后由 CI 覆盖）。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 3/5 — clean review and green CI on the current head, but the Stage 0 core gate flags a Stepping back: this is genuinely good work, and it got better every round. The pull-vs-push case for the meter still reads as the right call, and the eleven fix commits since the first pass each closed a real hole rather than polishing prose — the replay double-count, the abandoned-swap leak, the cold-start first-turn overbilling, the rollback prototype re-arm. Fail-closed discipline everywhere, migration at the parse boundary, honest disclosure of the attribution caveat, and a test next to every failure mode the review surfaced. The previous pass's blocker — the incomplete fixture sweep that broke the workspace build — is fixed, and CI is green on every lane that runs on PRs. If I had to maintain this in six months, I would thank the author. Why not approve, then. Two reasons, and only one of them is policy. The policy one: 552 production lines across core telemetry, config, and the Goal runtime is past the 500-line mark where a So: no change requests — there is nothing left to ask of the code — and no approval, per the gate. Refreshing the PR description (it still quotes the original six-file footprint) before merge would help the next reader; that is hygiene, not a condition. 中文说明置信度：3/5 —— 当前 head 上审查干净、CI 全绿，但 Stage 0 核心门禁将触及核心路径、生产代码达 552 行的 退一步看：这是真正用心的工作，而且一轮比一轮好。meter 采用拉取而非推送的论证依然成立；首轮以来的十一个修复提交，每一个都堵上了真实的漏洞而非润色文字——replay 重复计数、放弃切换导致的泄漏、冷启动首轮超额记账、回滚重新武装的原型问题。处处保持失败向零收敛、在解析边界做迁移、坦诚披露归因局限、审查中暴露的每个失败模式旁都有测试。上一轮的阻断项——fixture 排查不完整导致 workspace 构建失败——已经修复，CI 在所有 PR 阶段运行的流水线上均为绿色。半年后维护这份代码，我会感谢作者。 那为什么不批准？两个原因，其中只有一个是策略性的。策略原因：跨核心遥测、config 与 Goal 运行时的 552 行生产代码超过了 500 行标记—— 因此：不提修改要求——代码层面已无可要求；也不批准——依据门禁策略。合并前更新一下 PR 描述（目前仍写着最初的六文件规模）会有助于下一位读者；这是卫生问题，不是条件。 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Build is broken on this commit: packages/acp-bridge/src/transcript-replay.test.ts is missing the now-required tokensUsed field, so tsc --build fails before any test runs (all three red checks share this root cause).
One-line fix plus a full-workspace npm run build before pushing — details in my notes above. The design itself is sound; see the review comments. 🙏
已被后续 commit 取代，当前 head 需重新 review
|
已修复：commit |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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 — Serve A/B (ubuntu-latest, Node 22.x) was cancelled in CI at the Node-setup stage before its suite ran and did not run locally.
Not explored to full depth (tool budget reached): "agent 6b": verify ordering of headless bindGoalHost() (nonInteractiveCli.ts:1206/1441) vs client.initialize() telemetry replay for the resume-race finding; "agent 6b": verify ordering of ACP Session host binding (Session.ts:1752) vs telemetry replay / finalizeSessionRestore for the resume-race finding.
Not reviewed: reverse audit — stopped before round 4 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：build-and-test — Serve A/B (ubuntu-latest, Node 22.x) was cancelled in CI at the Node-setup stage before its suite ran and did not run locally。
未探索到全部深度（达到工具调用预算）："agent 6b"：verify ordering of headless bindGoalHost() (nonInteractiveCli.ts:1206/1441) vs client.initialize() telemetry replay for the resume-race finding；"agent 6b"：verify ordering of ACP Session host binding (Session.ts:1752) vs telemetry replay / finalizeSessionRestore for the resume-race finding。
未审查：反向审计——评审时间预算不足，未能开始第 4 轮。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| readSessionTokens: () => | ||
| Object.values( | ||
| uiTelemetryService.getMetricsForSession(sessionId).models, | ||
| ).reduce((total, model) => total + model.tokens.total, 0), |
There was a problem hiding this comment.
[Suggestion] This "session total tokens" reduction is implemented inline here for the third time: computeUsageFromMetrics (packages/cli/src/utils/nonInteractiveHelpers.ts:119-122, session-scoped, byte-for-byte the same computation) and ArenaAgentClient.getStatsFromTelemetry (packages/core/src/agents/arena/ArenaAgentClient.ts:210-223, global-scoped loop variant) already exist — and usageHistoryService.metricsToUsageRecord (packages/core/src/services/usageHistoryService.ts:173-176) carries a fourth variant with a fallback (m.tokens.total || m.tokens.prompt + m.tokens.candidates + m.tokens.thoughts) for sessions where tokens.total alone comes up short. — Failure scenario: the meter's comment asserts "the same figure /stats reports", but that equality is maintained by convention across independent implementations and guaranteed by nothing → a future change to what counts as total tokens updates one copy and silently desyncs the Goal's tokensUsed from the session figure reported in the same session, with no failing test; the existing fallback divergence shows the copies already desync in exactly the case where this meter reports a smaller number. Concrete cost: the Goal's spend and /stats drift apart silently.
Suggested fix: extract one helper (e.g. sumSessionTokens(metrics: SessionMetrics) in packages/core/src/telemetry/uiTelemetry.ts) and call it from this meter and the other sites — the copies are not all scope-identical, so the helper must respect session- vs global-scoped callers.
中文说明
[建议] 这个"会话 token 总量"的归约在这里是第三次以内联方式实现：computeUsageFromMetrics（packages/cli/src/utils/nonInteractiveHelpers.ts:119-122，会话作用域，计算逐字节相同）与 ArenaAgentClient.getStatsFromTelemetry（packages/core/src/agents/arena/ArenaAgentClient.ts:210-223，全局作用域循环变体）已经存在——而 usageHistoryService.metricsToUsageRecord（packages/core/src/services/usageHistoryService.ts:173-176）还带着第四个变体，它有回退表达式（m.tokens.total || m.tokens.prompt + m.tokens.candidates + m.tokens.thoughts），专门应对 tokens.total 单独取不到完整数值的会话。— 失败场景：meter 的注释声称"与 /stats 报告的同一数字"，但该等价性仅靠约定在多个独立实现间维持，没有任何保证 → 未来对"什么算总 token"的修改只会更新其中一份拷贝，Goal 的 tokensUsed 与同一会话中报告的会话数字悄悄失同步，且没有任何测试失败；既有的回退差异表明这些拷贝已经在此 meter 报告较小数值的情形下失同步。具体代价：Goal 花费与 /stats 静默背离。
修复建议：抽取一个共享助手（例如 packages/core/src/telemetry/uiTelemetry.ts 中的 sumSessionTokens(metrics: SessionMetrics)），由本 meter 与其他调用点共同调用——这些拷贝的作用域并不全相同，助手需区分会话作用域与全局作用域的调用方。
— qwen3.8-max via Qwen Code /review (v0.21.13)
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/goals/goal-reducer.ts:460 — [probe] D2-1: non-zero tokensUsed restoration unpinned — the ?? 0 → 0 mutant survives all 585 parse-path tests (deferred by the code-age rule; goal-reducer.ts unchanged since the previous round)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 2 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
已被后续 commit 取代，当前 head 需重新 review
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: the linked-issue fidelity pass — its prompt was built, but no agent on record was launched with it.
Not reviewed: reverse audit — no auditor was launched with a prompt this skill builds — the pass that hunts what the rest of the review missed ran, if at all, without the method its brief carries.
中文说明
仅完成部分审查，审查缺口已披露。
未审查：关联 issue 一致性检查——它的 prompt 已构建，但没有任何 agent 有记录用它启动过。
未审查：反向审计——没有审计 agent 是用本 skill 构建的 prompt 启动的——负责搜寻评审其余部分遗漏问题的这道工序，即便运行过，也缺失了 brief 承载的方法。
— Qwen3-235B-A22B via Qwen Code /review (v0.21.10)
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
Two-stage code review result — ISSUES_FOUND (round 1 / deepseek-v4-flash)
PR head: d74015c81f3079771817743e41420b0904ff84e7 (unchanged)
Verdict: Comment — findings reported, no Approve.
Round 1 model: deepseek-v4-flash
Findings: 14 total — 0 Critical, 9 Suggestions, 5 Nice-to-Have
Cost: 467 model calls · 35.3 M input tokens (97 % cached) · 283 k output tokens · ~32 min wall
Key findings (Suggestions)
- Aborted-turn tokens silently dropped —
releaseTurnclearscurrentTurnTokensAtStartwithout recording the delta, so paused / interrupted turns lose token accounting. - Replay redundancy —
replayUiTelemetryEventsFromConversationis invoked twice in the branch / resume flow (hook +client.tsinitialize()), doing O(2n) work. - Silent error swallowing —
readSessionTokenscatches all exceptions from the token meter (including programming errors) with no logging. - Fragile invariant —
currentTurnTokensAtStartis maintained across 6 set + 5 clear sites with no compiler enforcement. - Global metrics inflation — replay adds historical events to global metrics, inflating
/statson each resume / branch in the same process. - Incomplete test gating —
goal-evidence.test.tsandgoal-legacy-projection.test.tspass with and without the source change.
Notes
ghCLI authentication was unavailable on the review runner, so the linked-issue fidelity pass could not access the PR discussion.- The
audio-capturebuild failure is a pre-existing Windows environment issue (missing Python for node-gyp), not caused by this PR. - Because issues were found in round 1, round 2 (qwen3.8-max) was skipped per the two-stage review policy.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
1 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- Session-total-tokens reduction implemented inline a third time (packages/core/src/config/config.ts:~7873) — already reported (comment 3794415100)
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Deferred under the convergence posture (round 3, not a blocker) — recorded, not requested in this round:
packages/core/src/goals/goal-reducer.ts:460 — [review] D3-1: non-zero tokensUsed restoration unpinned — the ?? 0 → 0 mutant survives all parse-path tests (rediscovery of round-2 D2-1; goal-reducer.ts unchanged since the previous round)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 1 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
收敛姿态下延后（第 3 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Round-3 review found the same Critical in two places (R3-1 in useResumeCommand.ts, R3-2 in useBranchCommand.ts): both hooks replay the stored UI telemetry, and `GeminiClient.initialize()` replays it again a few lines later. `uiTelemetryService.addEvent` accumulates into both the process-wide metrics and the per-session bucket, but the `resetSession` leading each replay clears only the bucket — so the aggregate ends up carrying one extra copy of the whole session history. At process exit or on /clear, `persistSessionUsage` writes that doubled figure out under the resumed session id, permanently inflating cross-session usage reports, while in-session /stats reads the session-keyed bucket and stays correct — which is why manual testing never surfaces it. For /branch the same mechanism can reach a third copy when the fork fails after the replay and the rollback's own initialize() replays the parent. Replay exactly once per swap. The hook-level call stays where it is — its position before waitForGoalRuntime is load-bearing, it is what makes the Goal meter open on the restored totals — and it now hands off to the client via a one-shot marker on Config. initialize() consumes the marker and restores only the token counts, so its chat seeding is unchanged. startNewSession() clears the marker, so resuming the same session again later still replays exactly once, and a rollback still replays for the session it rolls back to. Also addressed from the same round: - R3-3 / R3-4: the order tests pinned only the call order of replayUiTelemetryEventsFromConversation, never its arguments, so dropping the optional sessionId survived all 38 hook tests. Dropping it sends sessionService down the global reset() branch, clearing every live session's bucket and keying no history under the resumed session. Both tests now assert the argument and the new hand-off. - R2-1: the config meter fixture set output_token_count to 0, making tokens.prompt equal tokens.total so the test could not tell a prompt-only meter from a total meter. The fixture now splits the total. - R1-2: of the six turn-start sites that open a meter reading, only two ran under a meter in the tests. Added metered tests for the finishTurn promotion, the releaseTurn promotion, and the post-rejection promoteQueuedUserTurn path; all four sites the review named now fail a suite when their reading is deleted. - R1-5: every meter failure was swallowed with zero logging, making a persistent fault indistinguishable from "no API calls happened". Added one breadcrumb per runtime, covering both the throw and the non-finite reading. Every change above is mutation-verified: reverting it turns at least one named test red. Verified with vitest on client, sessionService, config and goal-runtime (1164 passed) plus the two hook suites (38 passed).
|
Round 3 addressed in R3-1 / R3-2 (Critical, double replay). Took the suggested shape: replay exactly once per session swap. The hook-level call stays where it is — its position before R3-3 / R3-4. Both order tests now assert the replay's R2-1. Fixture split to R1-2. Added metered tests for the three unpinned promotion paths — the R1-5. Added one breadcrumb per runtime covering both failure modes — the throw and the non-finite reading (the Verification:
|
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued in workflow run. |
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not explored to full depth (tool budget reached): "agent reverse-audit (round 3)": none — no checks were cut short.; "agent reverse-audit (round 1)": could not locate the interactive cold- --resume GeminiClient.initialize() call site and its ordering vs goal-restore activation/host binding — this bounds th…; "agent reverse-audit (round 1)": did not run the packages/core unit tests for the chunk's changed files (goal-reducer, client, config) to confirm they pass..
Deferred under the convergence posture (round 4, not a blocker) — recorded, not requested in this round:
packages/core/src/goals/goal-reducer.ts:460 — [probe] D4-1: non-zero tokensUsed restoration unpinned — the ?? 0 → 0 mutant survives all parse-path tests (rediscovery of deferred D2-1/D3-1; goal-reducer.ts unchanged since the previous round)packages/core/src/goals/goal-evidence.test.ts:92 — [test] D4-2: fixture edit is inert — all 29 tests pass with the PR's source change reverted; nothing in this file gates tokensUsed (file unchanged since the previous round)packages/core/src/goals/goal-legacy-projection.test.ts:23 — [test] D4-3: fixture edit is inert — all 13 tests pass with the PR's source change reverted; nothing in this file gates tokensUsed (file unchanged since the previous round)packages/core/src/config/config.test.ts:2648 — [probe] D4-4: the new metering test's mockImplementation leaks into every later test in the file (clearAllMocks clears call history only; anchor line unchanged since the previous round)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）："agent reverse-audit (round 3)"：none — no checks were cut short.；"agent reverse-audit (round 1)"：could not locate the interactive cold- --resume GeminiClient.initialize() call site and its ordering vs goal-restore activation/host binding — this bounds th…；"agent reverse-audit (round 1)"：did not run the packages/core unit tests for the chunk's changed files (goal-reducer, client, config) to confirm they pass.。
收敛姿态下延后（第 4 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| sessionTokens = 100; | ||
| await runtime.finishTurn(promoted!); | ||
|
|
||
| expect(runtime.getSnapshot().goal?.tokensUsed).toBe(70); |
There was a problem hiding this comment.
[Suggestion] R1-2 (re-check, round 4): narrowed — five of the six currentTurnTokensAtStart turn-start sites are now gated by metered tests (continuation start, beginTurn admit via the config test, and the three promotion paths this round added — this assertion is one of them). The remaining one is the promotion inside handleStartFailure (goal-runtime.ts:406, a queued user turn promoted after host.startGoalTurn throws/rejects): deleting its readSessionTokens() reading bills the promoted turn 0 with the whole suite green — the exact mutation shape the new tests' comments cite and close for the other three promotion sites. The existing start-failure promotion tests all construct createGoalRuntime without tokenMeter.
Failure scenario: Goal active, user input queued while a Goal turn is starting, and the host fails to start the turn — the promoted turn bills 0; a future edit dropping that reading ships with the whole suite green.
Witness (mutation probe):
MUTATED (reading at goal-runtime.ts:406 deleted): 16 files / 393 tests all pass
metered probe (startGoalTurn rejects while beginTurn('real-user') is queued; session tokens 100 → 175): AssertionError: expected +0 to be 75
REVERTED: probe passes, suite green
Suggested fix: add a metered goal-runtime test whose host's startGoalTurn rejects once while a beginTurn reservation is queued, asserting the promoted turn bills its own delta.
中文说明
[建议] R1-2（复查，第 4 轮）：范围收窄——六个 currentTurnTokensAtStart 轮次起始点中已有五个被带 meter 的测试门控（continuation 起始、经 config 测试覆盖的 beginTurn 准入、以及本轮新增的三条晋升路径——本断言即其一）。剩下的是 handleStartFailure 内部的晋升点（goal-runtime.ts:406，host.startGoalTurn 抛出/拒绝后晋升排队的用户轮）：删除该处的 readSessionTokens() 读数会让被晋升轮计费为 0 而整个测试套件仍全绿——正是新测试注释中引用、并为另外三个晋升点关闭的突变形态。现有的 start-failure 晋升测试构造 createGoalRuntime 时都不带 tokenMeter。
失败场景：Goal 活跃、Goal 轮启动时有用户输入排队、host 启动轮次失败——被晋升轮计费为 0；未来若有编辑删掉该读数，将在整套测试全绿的情况下发布。
证据（突变探针）：突变（删除 goal-runtime.ts:406 读数）：16 个文件 / 393 个测试全部通过；带 meter 的探针（startGoalTurn 在 beginTurn('real-user') 排队时拒绝一次；会话 token 100 → 175）：AssertionError: expected +0 to be 75；还原后：探针通过、套件全绿。
修复建议：新增一个带 meter 的 goal-runtime 测试：host 的 startGoalTurn 在一个 beginTurn 预约排队时拒绝一次，断言被晋升轮计费为自己的增量。
— qwen3.8-max via Qwen Code /review (v0.21.13)
…orrupt Round-4 review found four Critical defects, all in the window the replay this PR added opens between a session swap starting and the swap either committing or failing. R4-1 — the replay runs before the fallible steps (`waitForGoalRuntime`, `initialize()`) and the rollback never undid it, so a failed `/resume` or `/branch` leaked one full copy of the abandoned session's history into the process-wide aggregate for the life of the process, and `persistSessionUsage` wrote the inflated figure out. `UiTelemetryService` had no compensation: `resetSession` clears one bucket and the global `reset()` would take the surviving session's live data with it. Adds `snapshotForReplay` / `restoreFromReplaySnapshot` — a narrow undo that touches only the snapshotted session — and hands the snapshot back on both hooks' rollback paths, before the rollback's own re-init runs. R4-2, R4-3 — resuming the session that is already current replayed it into itself. Nothing upstream excluded it, and there the hand-off marker is never consumed because `GeminiClient.initialize()` early-returns for an already-initialized session id: the aggregate carried the session twice, and the `resetSession` leading the replay first wiped the live bucket and refilled it from a snapshot loaded before the recorder's queued writes landed — dropping not-yet-flushed events and the internal-prompt side-query tokens that `recordUiTelemetryEventToChat` never persists at all. `/resume` now skips both the replay and the mark when `sessionId === oldSessionId`, which is what the base tree did (it replayed nothing on that path). R4-4 — on TUI `--resume` of a session interrupted with an active Goal, the restore is kicked off from the Config constructor (or, under a session-writer lease, from `activateChatRecording()`), both strictly before `initializeInternal` reaches `geminiClient.initialize()`. The continuation permit was minted — and its opening meter reading latched — against an empty bucket, so the first restored turn billed the whole replayed history: a Goal resumed from a 200k-token session billed ~200k extra, persisted into `GoalRecord.tokensUsed`. Both cold entrances now use the same prepareRestore/activateRestoredWork split the session-restore-projection path already used, with activation released once the client-init step has run. `restoreActivationPending` already holds `queueContinuation` until then. Behavior flip: a cold-start restore no longer mints its permit at construction time. `getGoalRuntimeReady()` deliberately still resolves on preparation, not activation — the Goal state is recovered and no awaiter is held — so a resumed Config that never calls `initialize()` cannot hang; only the autonomous continuation waits. `config.test.ts`'s "rebinds the current Goal host to every replacement runtime" pinned the old ordering and now drives `initialize()`, asserting no turn starts before it. The old ordering was the defect. Also fixes R3-3: the branch test's sessionId assertion was self-referential (`expect.any(String)` on the mock's own argument), so keying both calls onto the parent id passed all 26 tests while double-counting in production. It now pins the forked id, exactly the reviewer's witness. Every fix is mutation-verified: reverting each one turns at least one test red (3 tests for the snapshot API, 1 each for the same-session guard, the two rollback paths, the cold-start deferral, and the sessionId pin). Deferred D4-4 is not fixed, but the test added here resets the shared meter mock it introduces rather than widening the leak.
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
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): chunk 7: run goal-tools.test.ts and sessionService.test.ts to confirm the new/updated tests pass — the review worktree and its parent checkout have no node_modules ….
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/goals/goal-evidence.test.ts:92 (+3 locations) — [test] D5-1: inert tokensUsed fixture additions gate no assertions — 29/29, 13/13, 56/56, 22/22 all green under revert (rediscovery of deferred D4-2/D4-3; all four files unch…
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 7：run goal-tools.test.ts and sessionService.test.ts to confirm the new/updated tests pass — the review worktree and its parent checkout have no node_modules …。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 1 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| currentPermitHost = host; | ||
| currentTurnTokensAtStart = readSessionTokens(); | ||
| currentTurnKey = nextTurnKey; | ||
| currentTurnFeedback = nextVerifierFeedback; |
There was a problem hiding this comment.
[Suggestion] R1-2 (re-check, round 5): still stands — of the six currentTurnTokensAtStart = readSessionTokens() turn-start sites (380, 406, 550, 1199, 1247, 1364), five are now gated by metered tests (continuation start, beginTurn admit via the config test, and the three promotion paths added this round). This one — the handleStartFailure promotion at 406, the fallback when host.startGoalTurn rejects while a user turn is queued — is the only permit-minting site with no metered test: deleting this line survives the whole suite. — Failure scenario: Goal active, a user turn sits in queuedTurnKey, and the host rejects the scheduled continuation's startGoalTurn while the continuation start is in flight; the fallback promotes the queued user turn; without the opening reading, takeTurnTokens() sees opened === undefined and bills that turn 0 even though the session spent tokens during it — tokensUsed silently under-reports. — Witness (mutation + flip probe on this commit): deleting line 406 leaves goal-runtime.test.ts 115/115 and config.test.ts 535/535 green; the flip probe (deferred startGoalTurn rejection while a user turn is queued) bills tokensUsed=40 under the mutation (AssertionError: expected 40 to be 95) vs tokensUsed=95 on the unmodified code. — Suggested fix: add a runtime test with a tokenMeter, a queued user turn, and a host whose startGoalTurn rejects once in flight; assert the promoted turn bills its own meter delta (same shape as 'bills the meter delta for a reservation promoted by finishTurn').
中文说明
[建议] R1-2（第 5 轮复核）：仍然成立——六个 currentTurnTokensAtStart = readSessionTokens() 轮次起始点（380、406、550、1199、1247、1364）中，已有五个被计量测试钉住（continuation 起始、经 config 测试覆盖的 beginTurn 准入、以及本轮新增的三条晋升路径）。此处——406 行 handleStartFailure 中的晋升（用户轮排队时 host.startGoalTurn 被拒绝的兜底）——是唯一没有计量测试的 permit 签发点：删除该行后整套测试依旧全绿。— 失败场景：Goal 活跃、queuedTurnKey 中有排队的用户轮，continuation 启动在途时 host 拒绝 startGoalTurn；兜底逻辑晋升排队的用户轮；由于缺少起始读数，takeTurnTokens() 看到 opened === undefined，给该轮计 0——尽管会话在该轮实际消耗了 token——tokensUsed 静默少报。— 证据（本 commit 上的突变 + 翻转探针）：删除 406 行后 goal-runtime.test.ts 115/115、config.test.ts 535/535 全绿；翻转探针（用户轮排队时延迟拒绝 startGoalTurn）在突变下计出 tokensUsed=40（AssertionError: expected 40 to be 95），未修改代码上为 tokensUsed=95。— 修复建议：新增一个 runtime 测试：注入 tokenMeter、排队一个用户轮、让 host 的 startGoalTurn 在途拒绝一次；断言被晋升的轮次计到自己的 meter 增量（形态同 'bills the meter delta for a reservation promoted by finishTurn'）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| debugLogger.warn( | ||
| `Goal token meter unreadable (${detail}); tokensUsed will report 0 ` + |
There was a problem hiding this comment.
[Suggestion] R1-5 (re-check, round 5): still stands — the one-shot breadcrumb added in answer to this thread is delivered solely via debugLogger.warn, which writes nothing unless QWEN_DEBUG_LOG_FILE is set (writeLog opens the file lazily from that env var). Without it — the default for every user — a persistently broken or mis-keyed meter still produces a silently stalled/diverged tokensUsed ("my Goal says 0 tokens but I was billed for millions") with no log line, telemetry event, or error to grep for — exactly the failure mode the breadcrumb was added to close. A shape drift (e.g. tokens.total renamed in a future metrics change) turns the config meter's reduce into NaN on every reading; readSessionTokens reports once and bills 0 forever, and nothing is written anywhere. — Suggested fix: deliver the one-per-runtime breadcrumb on a channel that is on by default — a single console.warn/console.error line (the codebase's existing one-shot warning pattern), or a telemetry/ui event — instead of (or in addition to) the debug-file logger.
中文说明
[建议] R1-5（第 5 轮复核）：仍然成立——为回应本线程而新增的一次性 breadcrumb 仅通过 debugLogger.warn 投递，而它在未设置 QWEN_DEBUG_LOG_FILE 时不写任何内容（writeLog 依据该环境变量惰性打开文件）。不设置该变量——即所有用户的默认状态——时，一个持续损坏或键名错误的 meter 依然会产生静默停滞/偏离的 tokensUsed（"我的 Goal 显示 0 token，但我被计费了数百万"），没有任何日志行、遥测事件或错误可供检索——这正是 breadcrumb 要消灭的失败形态。例如未来的指标改动把 tokens.total 改名，config meter 的 reduce 每次都得到 NaN；readSessionTokens 上报一次后永久计 0，且任何地方都没有留下记录。— 修复建议：把这份每 runtime 一次的 breadcrumb 换到默认开启的通道——一行 console.warn/console.error（代码库现有的一次性告警模式），或一条遥测/UI 事件——替代（或补充）debug 文件日志。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| * skips its replay. | ||
| */ | ||
| consumeUiTelemetryEventsReplayed(sessionId: string): boolean { | ||
| if (this.uiTelemetryReplayedSessionId !== sessionId) return false; |
There was a problem hiding this comment.
[Suggestion] R4-5 (re-check, round 5): still stands — the real replay-handoff marker (markUiTelemetryEventsReplayed / consumeUiTelemetryEventsReplayed / the startNewSession re-arm) has zero end-to-end coverage: the CLI hook tests mock markUiTelemetryEventsReplayed on a plain-object config, the client tests mock the config side, and the config tests never drive a real client initialize() against it. Every test on both sides of the handshake mocks its own end, so a drift on either side ships green. — Failure scenario: if consumeUiTelemetryEventsReplayed were changed to not clear the marker (or startNewSession to not re-arm it), the next same-process /resume would either skip a needed replay (under-counting the resumed history) or double-replay it (the exact aggregate inflation this marker exists to prevent) — and no test in this diff would fail. — Suggested fix: add one integration-shaped test that runs the real Config marker methods against a real (or thinly spied) GeminiClient.initialize(): mark via the hook-shaped call, initialize, assert the replay ran exactly once, then swap again and assert the re-armed marker requires a fresh replay.
中文说明
[建议] R4-5（第 5 轮复核）：仍然成立——真正的回放交接标记（markUiTelemetryEventsReplayed / consumeUiTelemetryEventsReplayed / startNewSession 的重新武装）没有任何端到端覆盖：CLI hook 测试在纯对象 config 上 mock 掉 markUiTelemetryEventsReplayed，client 测试 mock 掉 config 一侧，config 测试从未驱动真实 client 的 initialize() 与之配合。交接两端的每个测试都 mock 了自己这一端，因此任何一侧的漂移都会绿灯通过。— 失败场景：若 consumeUiTelemetryEventsReplayed 被改成不清除标记（或 startNewSession 不再重新武装它），同一进程内的下一次 /resume 要么跳过一次必要的回放（少计恢复的历史），要么二次回放（正是该标记要防止的全局聚合膨胀）——而本 diff 中没有任何测试会失败。— 修复建议：补一个集成形态的测试，让真实的 Config 标记方法与真实（或薄 spy 的）GeminiClient.initialize() 配合：按 hook 的形态标记、执行 initialize、断言回放恰好执行一次，随后再次切换会话并断言重新武装的标记要求一次新的回放。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // Replay before the Goal runtime opens its meter so the meter reads | ||
| // the restored totals, then tell the client it is already done — |
There was a problem hiding this comment.
[Suggestion] R5-4 (location 1 of 2 — sibling at useBranchCommand.ts:214): the swap-entrance invariant "the replay lands in the session bucket before the swapped-in Goal runtime can mint a permit" is held only by event-loop timing — the replay's synchronous adjacency to config.startNewSession(...) — and no added test pins that adjacency. startNewSession synchronously builds the goal runtime and, on a warm process (clientSessionTelemetryReplayed already true, never reset), starts runtime.restore() immediately; the restore's activation chain (enqueue → activateRestoredWork → queueContinuation → flushContinuation) mints the continuation permit and samples currentTurnTokensAtStart on microtasks. Today the synchronous replay lands first; nothing asserts it. The cold-start entrance is explicitly gated (shouldDeferGoalRestoreForTelemetryReplay) and pinned by the cold-resume test — the swap entrances are neither. — Failure scenario: any future edit inserting an await between config.startNewSession(...) and the replay (a flush, a load, a guard) drains the microtask queue first: the permit mints against the still-empty bucket, then the replay fills it, and the first restored turn bills the entire replayed history into GoalRecord.tokensUsed (a 200k-token session bills ~200k extra) — the exact cold-start defect the deferral machinery exists to close, re-opened on both swap paths. Every test added in this diff stays green under that mutation (config is a plain-object mock — no real meter — and the assertions pin only that replay ordered before the getGoalRuntimeReady call). — Witness (probe on this entrance, real Config + real UiTelemetryService): PR ordering tokensUsed = 500; replay moved behind one await → tokensUsed = 200500 (assertion flips). — Suggested fix: make the invariant structural (gate the swap-time restore on the same deferred-activation shape used for cold start) or add a config-level regression test mirroring the cold-resume test for this entrance; at minimum assert in the hook tests that the replay ran before any microtask successor of startNewSession can fire.
中文说明
[建议] R5-4（共 2 处，此处为第 1 处；另一处见 useBranchCommand.ts:214）：切换入口的不变量"回放先于换入的 Goal runtime 签发 permit 落入会话 bucket"仅靠事件循环时序维持——即回放与 config.startNewSession(...) 的同步紧邻——而新增测试没有任何一条钉住这一紧邻关系。startNewSession 同步构建 goal runtime，且在热进程中（clientSessionTelemetryReplayed 已为 true 且从不重置）会立即启动 runtime.restore()；恢复的激活链（enqueue → activateRestoredWork → queueContinuation → flushContinuation）在微任务上签发 continuation permit 并采样 currentTurnTokensAtStart。当前同步回放恰好先落地，但没有任何东西保证这一点。冷启动入口有显式门控（shouldDeferGoalRestoreForTelemetryReplay）并被冷恢复测试钉住——切换入口两者皆无。— 失败场景：未来任何在 config.startNewSession(...) 与回放之间插入 await 的改动（flush、加载、守卫）都会先排空微任务队列：permit 在仍为空的 bucket 上签发，随后回放才填充 bucket，恢复后的第一轮会把整个回放历史计入 GoalRecord.tokensUsed（20 万 token 的会话多计约 20 万）——正是延迟激活机制要封堵的冷启动缺陷，在两条切换路径上重新打开。该突变下本 diff 新增的所有测试依旧全绿（config 是纯对象 mock，无真实 meter，断言只钉住回放先于 getGoalRuntimeReady 调用）。— 证据（本入口探针，真实 Config + 真实 UiTelemetryService）：PR 时序 tokensUsed = 500；把回放挪到一个 await 之后 → tokensUsed = 200500（断言翻转）。— 修复建议：让该不变量结构化（切换时的恢复改用与冷启动相同的延迟激活形态），或为本入口补一个仿照冷恢复测试的 config 级回归测试；至少应在 hook 测试中断言回放先于 startNewSession 的任何微任务后继执行。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| telemetryReplaySnapshot = | ||
| uiTelemetryService.snapshotForReplay(newSessionId); |
There was a problem hiding this comment.
[Suggestion] R5-4 (location 2 of 2 — sibling at useResumeCommand.ts:175): same gap on the /branch entrance, where the tests pin even less: they assert only that replay ordered before the getGoalRuntimeReady CALL, which stays true no matter what is inserted before waitForGoalRuntime. The fork additionally makes the race reachable with real data: forkSession copies the parent's Goal records into the fork JSONL, so an active parent goal yields activateRestoredWork → flushContinuation on the fork, sampling the meter — verified by probing a real forkSession + recoverGoalFromRecords (returns kind:'v2', goal.status === 'active'). — Failure scenario: any await inserted between config.startNewSession(newSessionId, resumed) and this replay drains the microtask queue first: the fork's continuation permit mints against its still-empty bucket (getMetricsForSession fresh-zero), then the replay fills it, and the first continuation turn bills the entire inherited history into GoalRecord.tokensUsed (a fork of a 200k-token session bills ~200k extra on its first turn), persisted. Inserting await Promise.resolve(); at that spot leaves all 26 useBranchCommand tests green. — Witness (probe on this entrance, real Config + real service, fork sessionData with active goal + 200k telemetry): PROBE-C-EVIDENCE PR-ordering tokensUsed=500 | mutant tokensUsed=200500. — Suggested fix: same as the sibling comment — structural deferral, or a config-level regression test for the branch entrance; at minimum pin the adjacency in the hook tests.
中文说明
[建议] R5-4（共 2 处，此处为第 2 处；另一处见 useResumeCommand.ts:175）：/branch 入口存在相同缺口，且测试钉得更少：只断言 replay 的调用先于 getGoalRuntimeReady 的调用——无论 waitForGoalRuntime 之前插入什么，该断言都成立。fork 还让该竞态可以带真实数据触发：forkSession 会把父会话的 Goal 记录复制进 fork 的 JSONL，活跃的父 Goal 会在 fork 上触发 activateRestoredWork → flushContinuation 并采样 meter——已通过真实 forkSession + recoverGoalFromRecords 探针验证（返回 kind:'v2'、goal.status === 'active'）。— 失败场景：在 config.startNewSession(newSessionId, resumed) 与本回放之间插入任何 await 都会先排空微任务队列：fork 的 continuation permit 在仍为空的 bucket 上签发（getMetricsForSession 返回全新零值），随后回放才填充，第一个 continuation 轮会把整个继承历史计入 GoalRecord.tokensUsed（从 20 万 token 会话 fork 出的分支第一轮多计约 20 万）并被持久化。在该位置插入 await Promise.resolve(); 后 useBranchCommand 全部 26 个测试依旧全绿。— 证据（本入口探针，真实 Config + 真实服务，带活跃 Goal 与 20 万遥测的 fork sessionData）：PROBE-C-EVIDENCE PR-ordering tokensUsed=500 | mutant tokensUsed=200500。— 修复建议：同另一处评论——结构化延迟激活，或为 branch 入口补 config 级回归测试；至少在 hook 测试中钉住该紧邻关系。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| // ...and the bucket the replay created is gone rather than left empty. | ||
| expect(service.getMetricsForSession(SESSION_B).models).toEqual({}); |
There was a problem hiding this comment.
[Suggestion] R5-5: the "bucket is gone rather than left empty" assertions (here and at line ~1363) are non-discriminating: getMetricsForSession() returns #sessionMetrics.get(id) ?? createInitialMetrics(), so a deleted bucket and a leftover empty bucket both yield .models = {} — the delete branch of restoreFromReplaySnapshot (uiTelemetry.ts:340) is untestable through this API, and no bucket-existence assertion exists anywhere in the suite. The stated intent ("Drop it rather than leave an empty bucket that reads as a live session") is unfalsifiable. — Failure scenario: replacing this.#sessionMetrics.delete(snapshot.sessionId) with this.#sessionMetrics.set(snapshot.sessionId, createInitialMetrics()) leaves all 50 tests green (verified by mutation on this commit) — the regression accumulates phantom per-session buckets on every failed swap until any future consumer iterating #sessionMetrics (e.g. a per-session usage listing) enumerates dead sessions as live ones. — Witness: mutation → Tests 50 passed (50); the discriminating assertion below flips it — against the mutant AssertionError: expected { models: {}, ... } to be undefined, against the original it passes. — Suggested fix:
| // ...and the bucket the replay created is gone rather than left empty. | |
| expect(service.getMetricsForSession(SESSION_B).models).toEqual({}); | |
| // ...and the bucket the replay created is gone rather than left empty. | |
| expect(service.getMetricsForSession(SESSION_B).models).toEqual({}); | |
| expect(service.snapshotForReplay(SESSION_B).sessionMetrics).toBeUndefined(); |
(snapshotForReplay reads #sessionMetrics.get(sessionId) directly and yields undefined only when the bucket was deleted; add the same to the closed-session test at ~1363.)
中文说明
[建议] R5-5："bucket 被删除而非留空"的断言（此处与约 1363 行）不具备判别力：getMetricsForSession() 返回 #sessionMetrics.get(id) ?? createInitialMetrics()，因此被删除的 bucket 与遗留的空 bucket 都得到 .models = {}——restoreFromReplaySnapshot 的删除分支（uiTelemetry.ts:340）经该 API 无法被测试，整套测试中也不存在任何 bucket 存在性断言。注释声明的意图（"删除它，而不是留下一个读起来像活跃会话的空 bucket"）不可证伪。— 失败场景：把 this.#sessionMetrics.delete(snapshot.sessionId) 换成 this.#sessionMetrics.set(snapshot.sessionId, createInitialMetrics()) 后全部 50 个测试依旧全绿（本 commit 上经突变验证）——该回归会在每次切换失败时累积幽灵般的按会话 bucket，直到未来某个遍历 #sessionMetrics 的消费者（例如按会话列出用量）把已死会话当成活跃会话列出。— 证据：突变后 Tests 50 passed (50)；下方判别性断言可翻转之——对突变体 AssertionError: expected { models: {}, ... } to be undefined，对原始代码通过。— 修复建议：见上方 suggestion 块（snapshotForReplay 直接读 #sessionMetrics.get(sessionId)，仅当 bucket 被删除时得到 undefined；约 1363 行的 closed-session 测试同理补一条）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
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 explored to full depth (tool budget reached): chunk 7: could not execute goal-tools.test.ts / sessionService.test.ts to confirm green — the review worktree (and the parent checkout) has no node_modules install….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 5, not a blocker) — recorded, not requested in this round:
packages/core/src/goals/goal-reducer.ts:460 — [probe] D5-1: non-zero tokensUsed restoration unpinned — the ?? 0 → 0 mutant survives all 848 parse-path tests (rediscovery of deferred D2-1/D3-1/D4-1; goal-reducer.ts unchanged since the previo…packages/core/src/goals/goal-evidence.test.ts:92 — [test] D5-2: fixture edit is inert — all 29 tests pass with the PR's source change reverted; nothing in this file gates tokensUsed (rediscovery of deferred D4-2; file unchanged since the pr…packages/core/src/goals/goal-legacy-projection.test.ts:23 — [test] D5-3: fixture edit is inert — all 13 tests pass with the PR's source change reverted; nothing in this file gates tokensUsed (rediscovery of deferred D4-3; file unchanged sin…packages/core/src/goals/goal-runtime.ts:83 — [review] D5-4: interrupted/paused turn windows never bill, and the added GoalTokenMeter doc claims otherwise — deferred by the code-age rule (goal-runtime.ts unchanged since the previous round)
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 7：could not execute goal-tools.test.ts / sessionService.test.ts to confirm green — the review worktree (and the parent checkout) has no node_modules install…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 5 轮，非阻断）——已记录，本轮不要求修改：共 4 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| if (this.shouldDeferGoalRestoreForTelemetryReplay()) { | ||
| // Same cold-start window as the constructor path: this runs from | ||
| // `activateChatRecording()`, still ahead of the client's replay. |
There was a problem hiding this comment.
[Suggestion] R5-2: The writer-lease entrance of the telemetry-replay deferral (startPendingGoalRestore) has no test pinning it — deleting this branch keeps every test green. — Failure scenario: the only test calling startPendingGoalRestore uses a legacy paused goal, never calls initialize(), and asserts nothing about held activation; for a paused goal activateRestoredWork → queueContinuation is a no-op, so removing the branch is invisible. On a cold --resume with sessionWriterLeaseEnabled: true and an active goal, the lease-handed runtime would restore-and-activate before GeminiClient.initialize() replays, so the first continuation turn's meter opens on an empty bucket and bills the entire replayed history into tokensUsed — the exact double-billing the constructor-path test ("opens a cold-resumed Goal meter on the replayed totals, not an empty bucket") guards. Suggested fix: add a writer-lease variant of that test — hand the lease via startPendingGoalRestore(), assert no continuation permit is minted before initialize(), replay + await config.initialize(), then assert the first turn bills only its own spend.
中文说明
[建议] R5-2：遥测回放延迟机制的 writer-lease 入口（startPendingGoalRestore）没有任何测试钉住——删掉这个分支，所有测试依然全绿。— 失败场景：唯一调用 startPendingGoalRestore 的测试用的是 legacy paused goal，从不调用 initialize()，也不对「激活被挂起」做任何断言；对 paused goal，activateRestoredWork → queueContinuation 本就是空操作，所以删掉该分支不可见。在 sessionWriterLeaseEnabled: true 且恢复活跃 goal 的冷 --resume 下，拿到 lease 的 runtime 会在 GeminiClient.initialize() 回放之前恢复并激活，第一个 continuation 轮的 meter 会在空 bucket 上开启读数，把整段回放历史计费进 tokensUsed——正是构造函数路径测试（"opens a cold-resumed Goal meter on the replayed totals, not an empty bucket"）所防范的双重计费。修复建议：为该测试补一个 writer-lease 变体——经 startPendingGoalRestore() 交付 lease，断言 initialize() 之前不签发 continuation permit，回放并 await config.initialize() 后，断言第一轮只计费自身花费。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| expect(snapshotForReplay.mock.invocationCallOrder[0]).toBeLessThan( | ||
| replayUiTelemetryEventsMock.mock.invocationCallOrder[0], | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R5-3: This /branch rollback test never pins the session-id argument of snapshotForReplay — every assertion on it is call-order or self-referential (restoreFromReplaySnapshot called with snapshotForReplay.mock.results[0].value), unlike the /resume twin, which asserts toHaveBeenCalledWith('session-2'). — Failure scenario: a refactor keying the snapshot on the parent session id (snapshotForReplay(oldSessionId)) keeps this test green, while in production restoreFromReplaySnapshot sets/deletes the bucket under the snapshot's sessionId — clobbering the parent's live bucket and leaving the abandoned fork's replayed history in the process-wide aggregate that persistSessionUsage writes out.
| expect(snapshotForReplay.mock.invocationCallOrder[0]).toBeLessThan( | |
| replayUiTelemetryEventsMock.mock.invocationCallOrder[0], | |
| ); | |
| expect(snapshotForReplay).toHaveBeenCalledWith( | |
| replayUiTelemetryEventsMock.mock.calls[0][1], | |
| ); | |
| expect(snapshotForReplay.mock.invocationCallOrder[0]).toBeLessThan( | |
| replayUiTelemetryEventsMock.mock.invocationCallOrder[0], | |
| ); |
中文说明
[建议] R5-3：这个 /branch 回滚测试从未钉住 snapshotForReplay 的会话 id 参数——对它的所有断言要么是调用顺序、要么是自引用（restoreFromReplaySnapshot 传入 snapshotForReplay.mock.results[0].value），而 /resume 的孪生测试断言了 toHaveBeenCalledWith('session-2')。— 失败场景：把快照改为以父会话 id 为键（snapshotForReplay(oldSessionId)）的重构能让本测试继续全绿，但生产中 restoreFromReplaySnapshot 按快照里的 sessionId 设置/删除 bucket——会覆盖父会话的实时 bucket，并把被放弃分叉已回放的历史留在进程级全局聚合中，被 persistSessionUsage 写出。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| try { | ||
| config.startNewSession(oldSessionId, prevSessionData); | ||
| // Re-hydrate chat history against the restored session. Best- |
There was a problem hiding this comment.
[Suggestion] R5-4: When the fork's initialize(SessionStartSource.Branch) succeeded before a later pre-uiSwapped step threw (buildResumedHistoryItems, applyCollapsePolicyAndSummary, or a UI-swap callback), the rollback's re-init replays the parent history a second time into the process-wide aggregate. — Failure scenario: after a successful fork init, initializedSessionId is the fork's id, so the rollback's initialize() fails the same-session early return, finds no restore runtime, and consumeUiTelemetryEventsReplayed(oldSessionId) is false (the startNewSession above cleared the marker, and it was keyed to the fork id anyway) — it takes the full-replay branch and re-adds the parent's whole conversation to the aggregate that already carries one copy, so persistSessionUsage writes roughly double the parent's usage for the process lifetime. (When the fork's init failed, initializedSessionId is still the parent's and the re-init correctly early-returns — only the success-then-late-failure edge double-counts.) Suggested fix: set the hand-off for the rollback's own init — call config.markUiTelemetryEventsReplayed(oldSessionId) immediately after the rollback startNewSession(...) and before the rollback initialize(), so it skips the replay and only restores token counts (the parent bucket is already intact via the snapshot restore).
中文说明
[建议] R5-4：若分叉的 initialize(SessionStartSource.Branch) 已成功、随后某个 uiSwapped 之前的步骤才抛错（buildResumedHistoryItems、applyCollapsePolicyAndSummary 或 UI 切换回调），回滚路径的重新初始化会把父会话历史第二次回放进进程级全局聚合。— 失败场景：分叉 init 成功后 initializedSessionId 是分叉 id，回滚的 initialize() 不满足同会话早退条件，又找不到 restore runtime，且 consumeUiTelemetryEventsReplayed(oldSessionId) 为 false（上面的 startNewSession 已清掉标记，且标记本来就是分叉 id 为键）——于是走完整回放分支，把父会话整段对话再次累加进已含一份副本的全局聚合，persistSessionUsage 会在进程余生写出约双倍的父会话用量。（若分叉 init 失败，initializedSessionId 仍是父会话 id，回滚重初始化会正确早退——只有「成功后再失败」这一支路双重计数。）修复建议：为回滚自身的重初始化设置交接标记——在回滚的 startNewSession(...) 之后、回滚的 initialize() 之前调用 config.markUiTelemetryEventsReplayed(oldSessionId)，使其跳过回放、只恢复 token 计数（父会话 bucket 已由快照恢复保持完整）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| ).toBeLessThan(getGoalRuntimeReady.mock.invocationCallOrder[0]!); | ||
| // Hand the replay off to the client so initialize() does not replay the | ||
| // same history a second time into the process-wide usage aggregate. | ||
| expect(markUiTelemetryEventsReplayed).toHaveBeenCalledWith(forkedSessionId); |
There was a problem hiding this comment.
[Suggestion] R5-6: Neither hook test pins the one ordering that makes the hand-off marker work — markUiTelemetryEventsReplayed before initialize(); the tests pin replay<mark and replay<goalReady only (same gap in useResumeCommand.test.ts). — Failure scenario: moving markUiTelemetryEventsReplayed(...) below the initialize() call in either hook keeps every test green; initialize() then finds no marker (consumeUiTelemetryEventsReplayed is one-shot and the marker was set too late), takes the full-replay branch, and re-adds the entire stored history into the process-wide usage aggregate the hook's replay already populated — the exact double-count this PR exists to eliminate, persisted by persistSessionUsage. — Witness (mutation probe): mark moved below initialize in both hooks → Tests 39 passed (39) (suite blind); adding one ordering assertion fails under the mutation (AssertionError: expected 16 to be less than 15) and passes against restored code (13/13). Suggested fix: add expect(vi.mocked(config.markUiTelemetryEventsReplayed).mock.invocationCallOrder[0]).toBeLessThan(vi.mocked(geminiClient.initialize).mock.invocationCallOrder[0]!); to each happy-path test (lift this test's initialize mock into a named variable, as the rollback test already does).
中文说明
[建议] R5-6：两个 hook 测试都没有钉住让交接标记生效的那条顺序——markUiTelemetryEventsReplayed 必须在 initialize() 之前；测试只钉了 replay<mark 与 replay<goalReady（useResumeCommand.test.ts 同样缺失）。— 失败场景：把任一 hook 中的 markUiTelemetryEventsReplayed(...) 移到 initialize() 调用之下，所有测试仍全绿；随后 initialize() 找不到标记（consumeUiTelemetryEventsReplayed 是一次性的，而标记设置得太晚），走完整回放分支，把整段存储历史再次累加进 hook 回放已填充的进程级全局用量聚合——正是本 PR 要消除的双重计数，并被 persistSessionUsage 持久化。— 证据（变异探针）：两个 hook 均把 mark 移到 initialize 之后 → Tests 39 passed (39)（套件无感知）；补一条顺序断言后，变异下失败（AssertionError: expected 16 to be less than 15），恢复代码后通过（13/13）。修复建议：在每个 happy-path 测试中补 expect(vi.mocked(config.markUiTelemetryEventsReplayed).mock.invocationCallOrder[0]).toBeLessThan(vi.mocked(geminiClient.initialize).mock.invocationCallOrder[0]!);（把本测试里的 initialize mock 提升为具名变量，回滚测试已这样做）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| this.#lastPromptTokenCount = snapshot.lastPromptTokenCount; | ||
| this.#lastCachedContentTokenCount = snapshot.lastCachedContentTokenCount; |
There was a problem hiding this comment.
[Suggestion] R5-7: No test pins the lastCachedContentTokenCount half of restoreFromReplaySnapshot — the three new rollback tests set/assert only lastPromptTokenCount, and the hook-level rollback tests assert spy order/args, not service state. — Failure scenario: deleting the #lastCachedContentTokenCount restore line keeps uiTelemetry.test.ts 50/50 green (mutation run), and after any failed /resume or /branch, /context (which reads getLastCachedContentTokenCount for the API cached-tokens display) would show the abandoned session's cached-token figure against the session the user was returned to. — Witness (mutation probe): restore line removed → Tests 50 passed (50); an added cached-content round-trip assertion fails under the mutation (AssertionError: expected 888 to be 11) and passes against restored code. Caveat: nothing in today's swap window mutates the cached counter, so the restore line is currently defensive — the untested-invariant claim stands as filed. Suggested fix: in "restores closed-session state and prompt counts", also call setLastCachedContentTokenCount(3) before the snapshot and setLastCachedContentTokenCount(888) during the simulated replay, then assert getLastCachedContentTokenCount() is 3 after the restore.
中文说明
[建议] R5-7：没有任何测试钉住 restoreFromReplaySnapshot 中 lastCachedContentTokenCount 这一半——三个新的回滚测试只设置/断言 lastPromptTokenCount，hook 层回滚测试只断言 spy 的调用顺序/参数，不断言服务状态。— 失败场景：删掉 #lastCachedContentTokenCount 的恢复行，uiTelemetry.test.ts 依然 50/50 全绿（变异运行）；此后任何失败的 /resume 或 /branch 之后，/context（读取 getLastCachedContentTokenCount 展示 API 缓存 token）会在用户被带回的会话上显示被放弃会话的缓存 token 数。— 证据（变异探针）：删除恢复行 → Tests 50 passed (50)；补一条 cached-content 往返断言后，变异下失败（AssertionError: expected 888 to be 11），恢复后通过。说明：当前交换窗口内没有任何路径会改动 cached 计数器，因此该恢复行目前是防御性的——未测不变量这一问题按原样成立。修复建议：在 "restores closed-session state and prompt counts" 中，快照前调用 setLastCachedContentTokenCount(3)、模拟回放期间调用 setLastCachedContentTokenCount(888)，然后断言恢复后 getLastCachedContentTokenCount() 为 3。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| debugLogger.warn( | ||
| `Goal token meter unreadable (${detail}); tokensUsed will report 0 ` + | ||
| `for this runtime until the meter recovers. Reported once per runtime.`, |
There was a problem hiding this comment.
[Suggestion] R1-5: (re-check, round 5) still stands — the one-shot meter-failure breadcrumb is delivered solely via debugLogger.warn, which writes nothing unless QWEN_DEBUG_LOG_FILE is set (--debug), so it is silent for exactly the users the motivating comment says it exists for. — Failure scenario: the comment names the scenario — "my Goal says 0 tokens but I was billed for millions" must stay distinguishable from "no API calls happened". A user running without --debug (the default; extension/corruptFile.ts already documents the env var as unset for almost all users) who hits a persistent meter fault gets tokensUsed 0 reported and persisted with no log line — warn() returns early on isDebugLogFileEnabled() === false — so the two cases remain exactly as indistinguishable as the comment says a silent failure makes them. Suggested fix: emit the one-shot breadcrumb on a channel that does not require the env var (a console.warn, a uiTelemetry event, or a session-record entry), or at minimum document the QWEN_DEBUG_LOG_FILE gating in the comment.
中文说明
[建议] R1-5：（第 5 轮复查）依然存在——一次性的 meter 故障面包屑仅通过 debugLogger.warn 发出，而它在未设置 QWEN_DEBUG_LOG_FILE（--debug）时不写任何内容，因此对注释声称它为之存在的那部分用户恰恰是静默的。— 失败场景：注释已点名该场景——「我的 Goal 显示 0 token，但我被计费了数百万」必须与「没有发生任何 API 调用」可区分。一个不带 --debug 运行的用户（默认情况；extension/corruptFile.ts 已说明几乎所有用户都不设置该环境变量）遇到持续性 meter 故障时，tokensUsed 会报告并持久化为 0，且没有任何日志行——warn() 在 isDebugLogFileEnabled() === false 时直接返回——两种情况仍然像注释所说的那样无法区分。修复建议：把一次性面包屑改到不依赖该环境变量的通道（console.warn、uiTelemetry 事件或会话记录条目），或至少在注释中说明它受 QWEN_DEBUG_LOG_FILE 门控。
— qwen3.8-max via Qwen Code /review (v0.21.13)
| markUiTelemetryEventsReplayed(sessionId: string): void { | ||
| this.uiTelemetryReplayedSessionId = sessionId; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R4-5: (re-check, round 5) still stands — the real replay-handoff marker methods have zero test coverage; every test on both sides of the handshake mocks its own end. client.test.ts injects a fake consumeUiTelemetryEventsReplayed onto mockConfig, the hook tests pass a vi.fn() as markUiTelemetryEventsReplayed, and config.test.ts contains no reference to either method (grep-verified at this commit). — Failure scenario: a regression making consumeUiTelemetryEventsReplayed always return false, or markUiTelemetryEventsReplayed a no-op, or dropping the uiTelemetryReplayedSessionId = undefined re-arm in startNewSession, reintroduces exactly the double-replay this PR exists to fix — on /resume or /branch, initialize() replays the already-replayed history a second time into the process-wide aggregate — with every suite green. Suggested fix: add a config.test.ts unit test for the real methods: mark(X) → consume(X) is true once, false on a second call and for a mismatched id; startNewSession(Y) clears a surviving marker so a later consume(X) is false.
中文说明
[建议] R4-5：（第 5 轮复查）依然存在——真实的回放交接标记方法零测试覆盖；交接两侧的每个测试都 mock 了自己这一端。client.test.ts 向 mockConfig 注入假的 consumeUiTelemetryEventsReplayed，hook 测试以 vi.fn() 充当 markUiTelemetryEventsReplayed，而 config.test.ts 对这两个方法没有任何引用（已在本 commit 上 grep 验证）。— 失败场景：若回归使 consumeUiTelemetryEventsReplayed 恒返回 false、或使 markUiTelemetryEventsReplayed 成为空操作、或删掉 startNewSession 中 uiTelemetryReplayedSessionId = undefined 的重新置位，就会重新引入本 PR 要修复的双重回放——/resume 或 /branch 时 initialize() 把已回放的历史第二次回放进进程级全局聚合——而所有套件仍全绿。修复建议：在 config.test.ts 为真实方法增加单测：mark(X) → consume(X) 第一次为 true，第二次及 id 不匹配时为 false；startNewSession(Y) 清除残留标记，使之后的 consume(X) 为 false。
— qwen3.8-max via Qwen Code /review (v0.21.13)
R5-1 (Critical, round 5): `bySource` is built with `Object.create(null)`
precisely so a subagent named after an inherited `Object` member cannot
short-circuit the `!bySource[name]` check and hand back a prototype member as
the bucket — the comment above `createInitialModelMetrics` names that crash
class. `structuredClone` does not preserve that: it copies own properties onto
a fresh object carrying `Object.prototype`. So every `snapshotForReplay` and
every `restoreFromReplaySnapshot` silently re-armed the crash, permanently,
for the rest of the process.
Verified rather than inferred. `structuredClone({bySource: Object.create(null)})`
yields a clone whose prototype IS `Object.prototype`, with
`typeof clone.bySource['constructor'] === 'function'` and the truthiness check
passing — so `#getOrCreateSourceMetrics` returns the `Object` function itself
and `bucket.api.totalRequests++` throws
`TypeError: Cannot read properties of undefined (reading 'totalRequests')`,
exactly the reported witness. `constructor` is a valid subagent name per the
naming regex, and uiTelemetry.test.ts already covers it on the live path — the
three replay tests missed it only because their fixture sets no
`subagent_name` and so never probes a colliding key.
All four clone sites now go through one `cloneSessionMetrics` helper that
re-homes each model's `bySource` onto a null prototype after the clone.
Test: `keeps bySource prototype-free across a snapshot/restore round trip`
drives a `constructor`-named event through snapshot → resetSession → restore,
asserts the null prototype survives in both the aggregate and the restored
session bucket, and asserts a further colliding event accumulates instead of
throwing. Mutation-verified: reverting all four sites to `structuredClone`
turns exactly this test red.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
2 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- R6-3 replay-handoff marker state machine has no direct test (ledger R4-5, re-confirmed this round) — already reported (comment 3804629533)
- R6-12 meter-failure breadcrumb invisible outside --debug (ledger R1-5, re-confirmed this round) — already reported (comment 3804629506)
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): chunk 5: did not execute goal-reducer.test.ts / goal-runtime.test.ts under vitest — the review worktree has no node_modules and installing the full dependency tree….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 6, not a blocker) — recorded, not requested in this round:
packages/core/src/goals/goal-reducer.ts:163 — [probe] R6-1: reducer NaN guard half-closed — NaN passes ?? 0 and Math.max, null-serializes, and parseGoalRecord then rejects the snapshot (unreachable via today's production caller)packages/core/src/goals/goal-runtime.ts:1286 — [probe] R6-2: finishTurn consumes the meter reading before the journal write — a rejecting write + same-permit retry bills the whole turn 0 (probe: spent 500, retry billed 0)packages/core/src/config/config.ts:8083 — [probe] R6-5 (ledger R5-2 still stands): writer-lease deferral branch has no active-Goal test — branch deletion survives config Goal 11/11 and lease suite 87/87packages/core/src/goals/goal-evidence.test.ts:92 — [test] R6-8: inert fixture edit — 29/29 pass with the source change reverted; gates nothing (mitigated: reducer/persistence suites fail under revert)packages/core/src/goals/goal-legacy-projection.test.ts:23 — [test] R6-9: inert fixture edit — 13/13 pass with the source change reverted; gates nothing (same mitigation)packages/core/src/config/config.ts:8007 — [review] R6-10 (ledger R5-5 still stands): warm-swap Goal metering rests on an accidental async delay and this comment states the opposite order; no warm-swap metered testpackages/core/src/config/config.test.ts:2646 — [probe] R6-15: new metering test leaks its mockImplementation into later tests (clearAllMocks clears call history only) — probe observed impl=LEAKED total=100packages/core/src/telemetry/uiTelemetry.test.ts:1367 — [probe] R6-16: R5-1 prototype test asserts the null prototype on SESSION_A — a bucket never restored; plain-structuredClone mutant on the session leg survives 51/51packages/core/src/telemetry/uiTelemetry.ts:359 — [probe] R6-18: open-session #closedSessions.delete restore leg unpinned — delete→add mutant survives 51/51 + 39/39; probe 'expected 40 to be 45' under mutantpackages/core/src/goals/goal-runtime.ts:406 — [probe] R1-2 (still stands, narrowed): handleStartFailure's promotion reading is the only ungated currentTurnTokensAtStart site — deletion mutant survives 115/115 + 11/11
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 2 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 5：did not execute goal-reducer.test.ts / goal-runtime.test.ts under vitest — the review worktree has no node_modules and installing the full dependency tree…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 6 轮，非阻断）——已记录，本轮不要求修改：共 10 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.13)
Round 6 review of QwenLM#9301 confirmed no new findings, but re-confirmed the two Suggestions carried over from earlier rounds. Both are cleared here. R6-12 (ledger R1-5): the one-shot meter-failure breadcrumb was delivered solely through `debugLogger.warn`, which writes nothing unless QWEN_DEBUG_LOG_FILE is set — `writeLog` returns early otherwise and the module has no console fallback. So in the default configuration a persistent meter fault still produced no log line, telemetry event or error, which is exactly the indistinguishability ("my Goal says 0 tokens but I was billed for millions" vs "no API calls happened") the breadcrumb was added to remove. It now also writes a `[warn]` line to stderr, following `quarantineCorruptFile`'s house pattern, and keeps the debugLogger line as the verbose copy. Secondary, same finding: the message promised "until the meter recovers" while `meterFailureReported` was never reset, so one transient fault permanently silenced every distinct later one. A finite reading now re-arms the flag, which makes the wording true. Crash-loop repetition of the SAME fault is still reported once, because the flag only clears on a successful read. R6-3 (ledger R4-5): the replay-handoff marker had zero real coverage — `client.test.ts` injects a fake `consumeUiTelemetryEventsReplayed` onto its mockConfig and the resume/branch hooks pass a `vi.fn()` as `markUiTelemetryEventsReplayed`, so both ends of the handshake were mocked on their own side of the territory split. Added three `config.test.ts` tests against the real methods: mark→consume is true once and false on a second read, a mismatched session id neither consumes nor clears the marker, and `startNewSession` re-arms it. Both fixes are mutation-verified. Six mutants run, six killed: dropping the stderr write, dropping the recovery re-arm, making `consume` always return false, making `mark` a no-op, making the marker non-one-shot, and dropping the `startNewSession` re-arm each turn at least one of the new tests red. Verification: `cd packages/core && npx vitest run src/goals/goal-runtime.test.ts src/config/config.test.ts src/goals/goal-reducer.test.ts` — 725 passed. eslint and prettier clean on the three touched files.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
4 Suggestion-level finding(s) this review confirmed are already reported on this PR and are not repeated:
- F4 writer-lease defer branch has no active-Goal test (ledger R5-2, re-confirmed this round by regression mutant) — already reported (comment 3806865646)
- F5 handleStartFailure promotion reading is the only ungated turn-start site (ledger R1-2, re-confirmed this round by deletion mutant) — already reported (comment 3806907118)
- H2 warm-swap immediate-restore meter ordering has no metered test (ledger R5-5, re-confirmed this round by ordering mutant) — already reported (comment 3806907094)
- I1 lastCachedContentTokenCount restore unpinned (ledger R5-3/R5-7, re-confirmed this round by deletion mutant) — already reported (comment 3806865651)
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): chunk 4: executing the two new client.test.ts tests (worktree and parent checkout both lack node_modules ; running them requires a full npm ci + npm run build , wh….
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 7, not a blocker) — recorded, not requested in this round:
packages/cli/src/ui/hooks/useResumeCommand.ts:196 — [review] A->B->A re-resume replays again: aggregate double-count plus live-only bucket loss (residual edge; replay arithmetic predates this PR)packages/core/src/goals/goal-runtime.test.ts:448 — [probe] takeTurnTokens opened===undefined branch unpinned — recovered-reading mutant bills the whole session totalpackages/cli/src/ui/hooks/useBranchCommand.ts:297 — [probe] rollback restore gate pinned only in the positive direction — un-gating mutant survives the whole suitepackages/core/src/goals/goal-runtime.ts:293 — [probe] five meter-fault tests leak [warn] breadcrumb lines into real test-run stderrpackages/core/src/telemetry/uiTelemetry.test.ts:1364 — [probe] prototype round-trip test never routes a session bucket through the restore clone (rediscovery of deferred R6-16)packages/core/src/goals/goal-evidence.test.ts:92 — [test] inert tokensUsed fixture gates no assertions (mitigated: reducer/persistence suites fail under revert)packages/core/src/goals/goal-legacy-projection.test.ts:23 — [test] inert tokensUsed fixture gates no assertions (same mitigation)
中文说明
仅完成部分审查，审查缺口已披露。
本轮确认的 4 条建议级发现已在 PR 上报告过，不再重复发布（列表见上方英文部分）。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 4：executing the two new client.test.ts tests (worktree and parent checkout both lack node_modules ; running them requires a full npm ci + npm run build , wh…。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 7 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
…nfirmed Round 7 confirmed no new findings but re-counted four Suggestion-level findings as already-reported duplicates. All four are the same shape: a load-bearing line with no test behind it, each with a mutation witness showing the suite stays green when it is deleted. Clearing them means making the next round unable to re-derive them, so each gets the test its witness describes. R1-2 (goal-runtime.ts, `handleStartFailure` promotion reading) — the sixth and last of the `currentTurnTokensAtStart` turn-start readings with no test. All four start-failure tests build the runtime with no `tokenMeter`, and the three metered promotion tests never make `startGoalTurn` reject, so nothing in the repo combined the two. Deleting the reading left the promoted permit with `opened === undefined`, `takeTurnTokens()` returning 0, and the promoted user turn's real spend silently dropped from `tokensUsed` on every host start failure. Adds the fourth twin of the promotion meter tests. Mutation: deleting the line fails the new test with `expected +0 to be 25` -- the exact witness the finding predicted -- and no other test moves. R5-2 (config.ts, writer-lease entrance of the telemetry-replay deferral) — the deferral is load-bearing on this entrance too: `initializeOnce` awaits `activateChatRecording()` strictly before `initializeInternal` reaches `geminiClient.initialize()`, where the stored telemetry is replayed. But the only lease restore test uses a PAUSED Goal and never calls `initialize()`, so it never mints a permit and never reads the meter -- deleting the branch shipped green. Adds the lease twin of the cold-resume meter test: lease handed over via `startPendingGoalRestore()`, no permit before `initialize()`, 200k replayed into the bucket during it, then the first restored turn bills only its own 500. Mutation: deleting the branch fails the new test at `expect(permit) .toBeUndefined()` -- a permit minted on the empty bucket, which is the R4-4 regression reappearing on this entrance -- and exactly one test goes red. R5-3 (uiTelemetry.ts, `lastCachedContentTokenCount` restore) — the field rides the same replay snapshot as `lastPromptTokenCount` but had no assertion anywhere, so deleting its capture/restore pair left the suite 50/50 green while its sibling stayed guarded. `geminiChat` writes it on every live API response, so an in-flight response from the OUTGOING session landing inside a swap window is exactly what the rollback has to undo. Extends the existing sibling test rather than adding a near-duplicate. Mutation: zeroing the capture fails the test with `expected 1234 to be 42`. R5-5 (config.ts, `shouldDeferGoalRestoreForTelemetryReplay` doc) — the clause "a swap-time restore (`startNewSession`) runs after the caller already replayed" is backwards. Verified against the code: both `useResumeCommand` and `useBranchCommand` call `config.startNewSession(...)` FIRST (useResumeCommand .ts:173, useBranchCommand.ts:208), and `startNewSession` synchronously calls `initializeGoalRuntime()`. The path is safe for two different reasons -- `clientSessionTelemetryReplayed` is set once by the initial `initialize()` and never cleared, so the predicate is already false at a swap; and the hooks replay synchronously, with no `await` between `startNewSession()` and `markUiTelemetryEventsReplayed()`, so the restore's microtask activation cannot run until the replay has landed. Rewritten to state that, including the "no `await` may be inserted between those two calls" constraint a maintainer would otherwise violate on the strength of the old wording. Comment-only. Verification: - packages/core config.test.ts + goal-runtime.test.ts + uiTelemetry.test.ts: 708 tests green. - Each of the three code findings mutation-checked individually, above; every mutant turns exactly one test red, the new one. - eslint and prettier clean on all four files. The only non-test source change in this commit is the R5-5 comment. - `npm run typecheck --workspace packages/core` reports one error, `src/utils/image-view.ts` / `sharp` -- identical on the untouched branch, a dependency-typing skew in this worktree, not this change.
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/core/src/telemetry/uiTelemetry.ts:363 — [review] restoreFromReplaySnapshot's 'update' emission is untested — deleting it ships greenpackages/core/src/goals/goal-runtime.test.ts:2374 — [review] releaseTurn silently discards a started turn's accumulated meter delta, undocumented; production reaches it with real spend (ACP settleGoalTurn fallback)packages/core/src/goals/goal-runtime.ts:1545 — [probe] dispatch permit-invalidation and dispose clear the meter reading without billing — interrupted-turn spend is silently dropped and persisted shortpackages/core/src/config/config.test.ts:2756 — [review] First new Goal-metering test leaks its mockImplementation into later tests (rediscovery of deferred D4-4/R6-15)packages/core/src/telemetry/uiTelemetry.test.ts:1364 — [probe] R5-1 prototype test's session-bucket assertion targets SESSION_A — never restored; plain-structuredClone mutant on the session leg ships green (rediscovery of R6-16)packages/core/src/goals/goal-evidence.test.ts:92 — [test] Inert tokensUsed fixture — all 29 tests pass with the source change reverted; gates nothing (mitigated: reducer/persistence suites fail under revert)packages/core/src/goals/goal-legacy-projection.test.ts:23 — [test] Inert tokensUsed fixture — all 13 tests pass with the source change reverted; gates nothing (mitigated: reducer/persistence suites fail under revert)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：reverse audit — did not converge within the reverse-audit round cap of 5。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 7 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 8, not a blocker) — recorded, not requested in this round:
packages/core/src/config/config.test.ts:2756 — [probe] new Goal-metering test leaks its getMetricsForSession mockImplementation into later tests (rediscovery of deferred R6-15)packages/core/src/goals/goal-reducer.ts:460 — [probe] non-zero tokensUsed restoration unpinned — the ?? 0 → 0 mutant survives all 937 parse-path tests (rediscovery of deferred D2-1/D3-1/D4-1/D5-1/R6-1)packages/core/src/telemetry/uiTelemetry.test.ts:1364 — [probe] prototype round-trip test never routes a session bucket through the restore clone (rediscovery of deferred R6-16)packages/core/src/config/config.ts:8039 — [review] failed prepareRestore leaves the deferred-activation closure installed — misleading second error over the real restore failurepackages/core/src/goals/goal-runtime.ts:329 — [probe] paused/interrupted turns after model start drop their billed spend from tokensUsed; meter doc claims otherwise (D5-4 class)packages/core/src/goals/goal-evidence.test.ts:92 (+2 locations) — [test] inert tokensUsed fixture additions gate no assertions (goal-evidence 29/29, goal-legacy-projection 13/13 pass under revert; feature gated elsewhere)
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 8 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
`finishTurn` evaluated `tokensUsed: takeTurnTokens()` while building the record, which cleared `currentTurnTokensAtStart` strictly before `await journal.recordGoalState(...)`. That write can throw — a transient writer-lease unavailability or a disk error — without mutating any state, which is exactly the contract the sibling test 'keeps turn state and the dispatch mutex usable when turn persistence fails' pins: the permit stays current so the same turn retries. On that retry the opening reading was gone, so the turn persisted with `tokensUsed: 0` while `turnCount` still advanced — the turn's real spend dropped out of the Goal's usage figure with no breadcrumb, since the meter itself read fine. The interactive TUI runs this chain (useGeminiStream finishTurn catch -> failClosedGoalTurn -> a second finishTurn on the same permit). `takeTurnTokens` becomes the non-destructive `peekTurnTokens`; the reading is cleared on the post-write path, which already did so. The new test is the metered twin of the persistence-failure contract test (that one builds its runtime with no `tokenMeter`, which is why the ordering shipped green); mutation-verified that restoring the consume turns it red at `tokensUsed: 0`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@qwen-code /review |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
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): chunk 6: running packages/core goal-reducer.test.ts and goal-runtime.test.ts under vitest — the review worktree has no node_modules or built dist/ , and installing/….
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:
packages/core/src/config/config.ts:3102 — [probe] Deferred Goal-restore activation under skipGeminiInitialization has no testpackages/cli/src/ui/hooks/useResumeCommand.ts:196 — [probe] Swap-entrance no-await invariant (startNewSession -> replay/mark) is unobservable by any testpackages/core/src/goals/goal-evidence.test.ts:92 — [test] Inert tokensUsed fixture — the file gates no behavior of this PRpackages/core/src/goals/goal-legacy-projection.test.ts:23 — [test] Inert tokensUsed fixture — legacy projection of the new field is ungatedpackages/core/src/goals/goal-protocol.ts:114 — [review] Fixture sweep missed 8 Goal fixtures in two typecheck-excluded test filespackages/core/src/services/sessionService.test.ts:2753 — [probe] New replay test leaves 200k tokens in the process-wide uiTelemetryService singleton
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未探索到全部深度（达到工具调用预算）：chunk 6：running packages/core goal-reducer.test.ts and goal-runtime.test.ts under vitest — the review worktree has no node_modules or built dist/ , and installing/…。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改：共 6 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
Deferred under the convergence posture (round 9, not a blocker) — recorded, not requested in this round:
packages/core/src/goals/goal-reducer.ts:460 — [probe] D9-1: non-zero tokensUsed parse-restoration unpinned — the ?? 0 → 0 mutant survives all 926 parse-path tests (rediscovery of deferred D2-1/D3-1/D4-1/D5-1/R6-1)packages/core/src/config/config.ts:3103 — [probe] D9-2: skip-path deferred-Goal-restore release has no test — moving it inside the non-skip branch strands a resumed active Goal (mutant survives 1061 tests)packages/cli/src/ui/hooks/useResumeCommand.ts:199 (+2 locations) — [probe] D9-3: swap-time no-await replay invariant pinned only by a comment — an inserted await survives 39/39 hook tests and would bill the replayed history into the first r…packages/core/src/goals/goal-reducer.test.ts:535 — [probe] D9-4: pause/resume/edit tokensUsed preservation unpinned — a spend-reset mutant in reduceGoalControl survives all 935 goal testspackages/core/src/goals/goal-runtime.test.ts:2327 (+2 locations) — [review] D9-5: two new meter-test rationale comments cite takeTurnTokens, renamed to peekTurnTokens by this PR's own HEAD commit — grep finds the old name only in those comm…packages/cli/src/ui/hooks/useBranchCommand.test.ts:14 — [probe] D9-6: /branch rollback compensation pinned only at call-structure level — a re-snapshot-at-rollback mutant restores the post-replay aggregate and ships green (26/26)packages/cli/src/ui/hooks/useBranchCommand.test.ts:683 — [probe] R5-3 still stands: snapshotForReplay session-id unpinned in the /branch rollback test — a wrong-key mutant leaks the fork bucket and ships green (26/26)packages/core/src/goals/goal-evidence.test.ts:92 — [test] D9-7: inert tokensUsed fixture — 29/29 tests pass with the source change reverted; gates nothing (feature gated by goal-persistence/goal-reducer suites)packages/core/src/goals/goal-legacy-projection.test.ts:23 — [test] D9-8: inert tokensUsed fixture — 13/13 tests pass with the source change reverted; gates nothing
中文说明
仅完成部分审查，审查缺口已披露。
未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
收敛姿态下延后（第 9 轮，非阻断）——已记录，本轮不要求修改：共 9 条（原文未翻译，列表见上方英文部分）。
— qwen3.8-max via Qwen Code /review (v0.21.14)
Critical findings were fixed on the current head and their review threads are resolved; the current-head bot review reports no blocking findings. Dismissing this stale review before a fresh triage.
|
@qwen-code /triage |
|
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: 3263 passed · 1 failed · 3264 total Flakiness gate: 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：3263 通过 · 1 失败 · 3264 总计 抖动门： Verification reportPR #9301 — feat(goal): account the tokens a Goal spendsVerdict: The single failing assertion is Finding 1 below: a pre-existing test in a file the PR modifies is deterministically red at the verified head because its mock 中文摘要结论：
Central claim + A/BCentral claim: a Harness
Both arms exit 0 with their expectations encoded (base asserts absence, head asserts presence), so the flip "no accounting → exact deltas on record, on disk, and on the wire" is the A/B verdict. Secondary claim 1 — replay-once + rollback compensation. Harness
Secondary claim 2 — fail-safe meter + non-destructive peek. Cells C–G above: unmetered/throwing/NaN meters bill zero without failing turns; one FindingsF1 (high, CI-blocking) — pre-existing resume test red at HEAD: mock Config lacks
|
| # | mutation | killed by | harness also red? |
|---|---|---|---|
| M1 | peekTurnTokens consumes the opening reading (reverts final commit) |
goal-runtime.test.ts bills the retry after a failed turn write the tokens the turn spent (tokensUsed 0 vs 2000) |
yes — cell G5 got 0 |
| M2 | cloneSessionMetrics → plain structuredClone |
uiTelemetry.test.ts keeps bySource prototype-free across a snapshot/restore round trip |
yes — cells 4b/4c proto=[object Object] |
| M3 | reducer Math.max(0, …) clamp removed |
goal-reducer.test.ts adds nothing for a meter that went backwards |
— |
| M4 | migration default ?? 0 removed |
goal-reducer.test.ts migrates a snapshot persisted before spend was recorded |
— |
| M5 | negative-spend parse rejection clause removed | goal-reducer.test.ts rejects a snapshot carrying negative spend |
— |
| M6 | same-session replay guard disabled | useResumeCommand.test.ts does not replay telemetry when resuming the session already current |
— |
| M7 | consumeUiTelemetryEventsReplayed no longer clears the marker |
config.test.ts replays a marked session exactly once, then never again |
— |
| M8 | client ignores the hand-off (alreadyReplayed = false) |
client.test.ts does not replay telemetry a second time when the session swap already did |
— |
| M9 | shouldDeferGoalRestoreForTelemetryReplay → false |
config.test.ts opens a cold-resumed/lease-restored Goal meter on the replayed totals (both) |
— |
9/9 killed, 0 survivors. Unmutated controls green on both sides (core 1742/1742; harness 27/27 and 25/25), so the kills are attributable and the harnesses are proven able to fail.
Targeted gates
packages/coreaffected suites (goals, uiTelemetry, config, client, client-goal, sessionService, session-transcript-reader, chatRecordingService): 22 files, 1742/1742 passed.packages/cliPR-touched suites (14 files incl. both hooks, resumeHistoryUtils, goalCommand, GoalPill, HistoryItemDisplay, GoalStatusMessage, serve goals, StreamJsonOutputAdapter, 4 ACP files): 1442 passed, 1 failed — the failure is F1.- Repo-wide
npm run typecheck: exit 0 (core and cli included).
Not covered
- Per-commit attribution: the checkout is shallow (depth 2);
git rev-list --count HEAD^1..HEAD^2returns 1 while the metadata lists 12 commits, so individual commits (e.g. the round-3/4/5 fixes) were verified only as part of the aggregateHEAD^1..HEADdiff. - Full interactive /resume and /branch flows: hook logic is covered by their vitest suites and my service-level harness; I did not render the ink TUI end to end.
- Windows/macOS (PR states CI-only coverage there).
- sessionService / chatRecordingService / session-transcript-reader / client-goal suites ran green but received no planted mutation (completeness note; the PR's additions there are fixture-level).
- The PR's accepted tradeoff that the meter bills the whole session (interleaved user turns attributed to the open Goal turn) was not re-litigated; it is documented in the tool/protocol comments.
Methodology
Environment: CI verify container (node 22.23.2), merge-ref checkout — HEAD 6bfadccc4f, base tip HEAD^1 3b3818db87, PR head HEAD^2 0d5abe8ec4 (matches headRefOid in the metadata snapshot). Base control built in a scratch worktree tmp/base-tree via npx tsc --build in packages/core; its node_modules is a symlink to the head tree's packages/core/node_modules, which contains only external deps (ajv, diff, fdir, ignore, json-schema-traverse, mime, picomatch, undici — no @qwen-code/* links), pinned by the PR-unchanged lockfile, so the control differs from head by source only; each harness run prints realpath() of the loaded module to prove which tree supplied the code. Harnesses (01-ab-goal-token-meter.mjs, 02-replay-compensation.mjs, kept in this directory) import the compiled dist/ of each tree and drive the real runtime/service with fakes only at the interfaces the code defines. Mutations were scratch source edits with targeted vitest runs, restored via git checkout (worktree verified clean after each). Raw logs in logs/; evidence PNGs rendered by scripts/verify-capture.mjs.
Flakiness gate log
rounds=5 files=28 skipped=0
file packages/acp-bridge/src/transcript-replay.test.ts: (cd packages/acp-bridge) npx --no-install vitest run ./src/transcript-replay.test.ts
file packages/cli/src/acp-integration/acpAgent.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/acpAgent.test.ts
file packages/cli/src/acp-integration/session/Session.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/Session.test.ts
file packages/cli/src/acp-integration/session/emitters/MessageEmitter.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/emitters/MessageEmitter.test.ts
file packages/cli/src/acp-integration/session/history-replay-page.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/history-replay-page.test.ts
file packages/cli/src/acp-integration/session/recovered-goal-update.test.ts: (cd packages/cli) npx --no-install vitest run ./src/acp-integration/session/recovered-goal-update.test.ts
file packages/cli/src/nonInteractive/io/StreamJsonOutputAdapter.test.ts: (cd packages/cli) npx --no-install vitest run ./src/nonInteractive/io/StreamJsonOutputAdapter.test.ts
file packages/cli/src/serve/routes/goals.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/routes/goals.test.ts
file packages/cli/src/ui/commands/goalCommand.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/commands/goalCommand.test.ts
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/HistoryItemDisplay.test.tsx: (cd packages/cli) npx --no-install vitest run ./src/ui/components/HistoryItemDisplay.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/hooks/useBranchCommand.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/hooks/useBranchCommand.test.ts
file packages/cli/src/ui/hooks/useResumeCommand.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/hooks/useResumeCommand.test.ts
file packages/cli/src/ui/utils/resumeHistoryUtils.test.ts: (cd packages/cli) npx --no-install vitest run ./src/ui/utils/resumeHistoryUtils.test.ts
file packages/core/src/config/config.test.ts: (cd packages/core) npx --no-install vitest run ./src/config/config.test.ts
file packages/core/src/core/client-goal.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/client-goal.test.ts
file packages/core/src/core/client.test.ts: (cd packages/core) npx --no-install vitest run ./src/core/client.test.ts
file packages/core/src/goals/goal-evidence.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-evidence.test.ts
file packages/core/src/goals/goal-legacy-projection.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-legacy-projection.test.ts
file packages/core/src/goals/goal-persistence.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-persistence.test.ts
file packages/core/src/goals/goal-reducer.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-reducer.test.ts
file packages/core/src/goals/goal-runtime.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-runtime.test.ts
file packages/core/src/goals/goal-tools.test.ts: (cd packages/core) npx --no-install vitest run ./src/goals/goal-tools.test.ts
file packages/core/src/services/chatRecordingService.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/chatRecordingService.test.ts
file packages/core/src/services/session-transcript-reader.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/session-transcript-reader.test.ts
file packages/core/src/services/sessionService.test.ts: (cd packages/core) npx --no-install vitest run ./src/services/sessionService.test.ts
file packages/core/src/telemetry/uiTelemetry.test.ts: (cd packages/core) npx --no-install vitest run ./src/telemetry/uiTelemetry.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/acp-bridge/src/transcript-replay.test.ts: PPP
packages/cli/src/acp-integration/acpAgent.test.ts: PPP
packages/cli/src/acp-integration/session/Session.test.ts: PP
packages/cli/src/acp-integration/session/emitters/MessageEmitter.test.ts: PP
packages/cli/src/acp-integration/session/history-replay-page.test.ts: PP
packages/cli/src/acp-integration/session/recovered-goal-update.test.ts: PP
packages/cli/src/nonInteractive/io/StreamJsonOutputAdapter.test.ts: PP
packages/cli/src/serve/routes/goals.test.ts: PP
packages/cli/src/ui/commands/goalCommand.test.ts: PP
packages/cli/src/ui/components/GoalPill.test.tsx: PP
packages/cli/src/ui/components/HistoryItemDisplay.test.tsx: PP
packages/cli/src/ui/components/messages/GoalStatusMessage.test.tsx: PP
packages/cli/src/ui/hooks/useBranchCommand.test.ts: PP
packages/cli/src/ui/hooks/useResumeCommand.test.ts: FF
packages/cli/src/ui/utils/resumeHistoryUtils.test.ts: PP
packages/core/src/config/config.test.ts: PP
packages/core/src/core/client-goal.test.ts: PP
packages/core/src/core/client.test.ts: PP
packages/core/src/goals/goal-evidence.test.ts: PP
packages/core/src/goals/goal-legacy-projection.test.ts: PP
packages/core/src/goals/goal-persistence.test.ts: PP
packages/core/src/goals/goal-reducer.test.ts: PP
packages/core/src/goals/goal-runtime.test.ts: PP
packages/core/src/goals/goal-tools.test.ts: PP
packages/core/src/services/chatRecordingService.test.ts: PP
packages/core/src/services/session-transcript-reader.test.ts: PP
packages/core/src/services/sessionService.test.ts: PP
packages/core/src/telemetry/uiTelemetry.test.ts: PP
verdict: consistent-fail
summary: 1 of 28 changed test file(s) failed identically in every round — deterministic, so CI owns that signal
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/acp-bridge/src/transcript-replay.test.ts: P (exit 0)
round 1 · packages/cli/src/acp-integration/acpAgent.test.ts: P (exit 0)
round 1 · packages/cli/src/acp-integration/session/Session.test.ts: P (exit 0)
round 1 · packages/cli/src/acp-integration/session/emitters/MessageEmitter.test.ts: P (exit 0)
round 1 · packages/cli/src/acp-integration/session/history-replay-page.test.ts: P (exit 0)
round 1 · packages/cli/src/acp-integration/session/recovered-goal-update.test.ts: P (exit 0)
round 1 · packages/cli/src/nonInteractive/io/StreamJsonOutputAdapter.test.ts: P (exit 0)
round 1 · packages/cli/src/serve/routes/goals.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/commands/goalCommand.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/components/GoalPill.test.tsx: P (exit 0)
round 1 · packages/cli/src/ui/components/HistoryItemDisplay.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/hooks/useBranchCommand.test.ts: P (exit 0)
round 1 · packages/cli/src/ui/hooks/useResumeCommand.test.ts: F (exit 1)
--- output tail · round 1 · packages/cli/src/ui/hooks/useResumeCommand.test.ts ---
�[1m�[46m RUN �[49m�[22m �[36mv3.2.4 �[39m�[90m/__w/qwen-code/qwen-code/packages/cli�[39m
�[2mCoverage enabled with �[22m�[33mv8�[39m
�[31m❯�[39m src/ui/hooks/useResumeCommand.test.ts �[2m(�[22m�[2m14 tests�[22m�[2m | �[22m�[31m1 failed�[39m�[2m)�[22m�[32m 59�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22mshould initialize with dialog closed�[32m 13�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22mshould open the dialog when openResumeDialog is called�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22mshould close the dialog when closeResumeDialog is called�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22mshould maintain stable function references across renders�[32m 1�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22mhandleResume no-ops when config is null�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22mhandleResume closes the dialog immediately and restores session state�[32m 6�[2mms�[22m�[39m
�[31m �[31m�[31m useResumeCommand�[2m > �[22mhandleResume routes history replacement through the loadHistory override�[39m�[32m 8�[2mms�[22m�[39m
�[31m → expected "spy" to be called 1 times, but got 0 times�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22madds a recovery notice when resuming an interrupted tool turn�[32m 3�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22mapplies collapseOnResume policy when resuming a session�[32m 3�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22madds a recovered-background-agents notice when paused agents are restored�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22mblocks resume when the current session still has running background work�[32m 3�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22mblocks resume when the current session still has a running monitor�[32m 2�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22mrolls core back when persisted Goal state is malformed�[32m 7�[2mms�[22m�[39m
�[32m✓�[39m useResumeCommand�[2m > �[22mdoes not replay telemetry when resuming the session already current�[32m 2�[2mms�[22m�[39m
�[31m⎯⎯⎯⎯⎯⎯⎯�[39m�[1m�[41m Failed Tests 1 �[49m�[22m�[31m⎯⎯⎯⎯⎯⎯⎯�[39m
�[41m�[1m FAIL �[22m�[49m src/ui/hooks/useResumeCommand.test.ts�[2m > �[22museResumeCommand�[2m > �[22mhandleResume routes history replacement through the loadHistory override
�[31m�[1mAssertionError�[22m: expected "spy" to be called 1 times, but got 0 times�[39m
�[36m �[2m❯�[22m src/ui/hooks/useResumeCommand.test.ts:�[2m435:33�[22m�[39m
�[90m433| �[39m })�[33m;�[39m
�[90m434| �[39m
�[90m435| �[39m
...truncated -- full content in the run artifacts.
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
⏸️ Deferring to @qqqys — Stage 0 core-gate policy: a |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
|
Closing in favour of two focused PRs — this one had grown to 540 production lines across 11 files, of which only about 130 were the Goal accounting it is named for. The cause was the measurement, not the feature. Reading the Goal's spend from the session's telemetry aggregate meant sampling that aggregate before and after each turn, which made every path that writes to it — The split:
The remainder — the hook-side early replay, the replay hand-off marker, and the goal-restore deferral — existed only to make an aggregate-based meter correct across swaps. With the recorder-based ledger none of it is needed, so it is dropped rather than re-homed. Thanks to the review rounds here for surfacing the swap-ordering hazards; they are what made the measurement choice look wrong in the first place. |
4 participants
What this PR does
GoalRecordgains atokensUsedfield, summed across the Goal's turns byreduceGoalTurnFinished, andget_goalreports it in the unpermittedlastGoalsummary alongside the turn count. The figure is the one the session already keeps for/stats, read fromuiTelemetryServicesession model metrics, so a Goal's spend and the session's spend are one measurement rather than two definitions. The runtime obtains it through atokenMeterinjected once where the runtime is constructed: it takes a reading when a turn permit is issued and the difference when the turn finishes. No limit is introduced — this only counts.Why it's needed
A Goal reports how many turns it has run and how long it has been active, and neither answers the question a user actually asks about a long autonomous run, which is what it is costing.
turnCountis a poor proxy: ten expensive turns and a thousand cheap ones are indistinguishable in it. It is also the number every future limit has to be expressed in, and a budget cannot be enforced against a figure nobody keeps, so counting has to land before anything can bound it.The reading is pulled rather than pushed on purpose.
runtime.finishTurnis called from three separate hosts on the normal path — the interactive TUI, the ACP session, and the non-interactive CLI — and from core on the interrupted and cancelled paths. A count pushed in by the caller would have to be threaded through all four and would silently go missing wherever a future call site forgot it, which is the same shape of defect as a prompt assembled independently in each host. A meter injected once at construction cannot be forgotten and covers the interrupted paths for free.Reviewer Test Plan
How to verify
Run a Goal for several turns and read
tokensUsedback — throughget_goalonce the Goal stops issuing permits, or from the persisted snapshot. Confirm it grows monotonically, that the growth over a turn matches what the session's own token metrics moved by during that turn, and that it survives a session resume. Then confirm the degenerate paths do not fail the turn: a runtime constructed with notokenMeterbills every turn zero and keeps running, and a meter that throws bills zero and keeps running rather than propagating. Finally confirm migration: a Goal recovered from a transcript written before this field existed restores withtokensUsed: 0rather than being rejected as malformed.Automated coverage adds four reducer cases (accumulation across turns, an unmetered turn and a backwards meter both adding nothing, migration of a snapshot persisted without the field, and rejection of negative spend) and three runtime cases (the per-turn delta landing on the record across two consecutive turns, an unmetered runtime, and a throwing meter that still resolves the turn).
npx vitest runpasses 1136 tests across 19 core files and 161 tests across 6 CLI Goal files.npx tsc --noEmitis clean inpackages/core; inpackages/cliit reports no Goal-related error, only the unbuilt-sibling-workspace errors that are present on the unmodified head.prettierandeslintare clean on every changed file.Evidence (Before & After)
Before,
get_goalon a stopped Goal:{"active":false,"lastGoal":{"goalId":"…","revision":3,"status":"usage_limited","turnCount":27,"activeTimeMs":1763705,"lastReason":"…"}}— 27 turns, no cost.After, the same call: the summary additionally carries
"tokensUsed":<n>, so the run's spend is readable from the same place as its turn count.Tested on
Environment (optional)
Linux, Node.js 22, unit tests only.
Risk & Scope
tokensUsedis required onGoalRecordrather than optional, which is why this PR touches twenty test files. The alternative — an optional number defaulted at every read site — would have kept the diff small and pushed anundefinedcheck onto every future consumer, including the budget comparison this is groundwork for. Making the field total and migrating absent values to zero at the parse boundary puts the cost in one place, once.tokensUsed, and no UI surface displays it yet beyond theget_goalsummary. The meter measures the whole session rather than only the Goal's own requests, so a user turn interleaved with an autonomous run is attributed to the Goal turn that was open at the time; splitting that apart needs per-request attribution that does not exist today and is deliberately not attempted here. Windows and macOS were not exercised locally and remain covered by CI.tokensUsed: 0instead of being rejected, and a snapshot carrying a negative value is rejected. Scope for the core triage gate: 91 added and 2 deleted production lines across six files, with the remainder being fixture updates in twenty test files; the change is cross-package (packages/coreandpackages/cli) but each CLI file gains one field in a Goal fixture.Linked Issues
None.
中文说明
本 PR 做了什么
GoalRecord新增tokensUsed字段，由reduceGoalTurnFinished在 Goal 的各轮之间累加；get_goal在无 permit 的lastGoal摘要里与轮数一并返回它。这个数字就是会话本来就为/stats维护的那个，读自uiTelemetryService的会话模型指标，因此 Goal 的花费和会话的花费是同一次测量，而不是两套定义。运行时通过在构造处注入一次的tokenMeter取值：发放轮次 permit 时取一次读数，轮次结束时取差值。本 PR 不引入任何上限——它只负责计数。为什么需要
Goal 会报告自己跑了多少轮、活跃了多久，而这两者都回答不了用户面对一次长自主运行时真正会问的问题：它花了多少。
turnCount是个很差的代理——十轮昂贵的和一千轮便宜的在它里面无法区分。它同时也是未来任何上限必须使用的计量单位，而预算无法针对一个没人记录的数字来执行，所以「计数」必须先于「设限」落地。读数采用拉取而非推送，是有意为之。
runtime.finishTurn在正常路径上由三个不同的 host 调用——交互式 TUI、ACP session、非交互 CLI——并在中断与取消路径上由 core 调用。由调用方推入的计数需要贯穿这四处，并且会在未来任何一个忘记它的调用点上静默丢失，这与「提示词在每个 host 各自拼装」是同一形状的缺陷。在构造处注入一次的 meter 无法被忘记，并且顺带覆盖了中断路径。评审者测试计划
如何验证
让一个 Goal 跑若干轮，然后读回
tokensUsed——Goal 停止发放 permit 后通过get_goal，或直接从持久化快照读。确认它单调增长、某一轮的增量与该轮期间会话自身 token 指标的变化一致，并且能在会话恢复后保留。然后确认退化路径不会让轮次失败：构造时未提供tokenMeter的运行时对每轮计零并继续运行；会抛异常的 meter 同样计零并继续运行，而不是把异常抛出去。最后确认迁移：从该字段出现之前写入的转录中恢复的 Goal，会以tokensUsed: 0恢复，而不是被判定为格式非法。自动化覆盖新增四个 reducer 用例（跨轮累加、未计量轮次与倒退的 meter 都不增加、对未持久化该字段的快照做迁移、拒绝负数花费）和三个 runtime 用例（连续两轮的每轮增量正确落到记录上、未计量的运行时、以及抛异常的 meter 仍能让轮次正常结束）。
npx vitest run通过 core 侧 19 个文件共 1136 个测试，以及 CLI 侧 6 个 Goal 文件共 161 个测试。packages/core的npx tsc --noEmit干净；packages/cli没有任何与 Goal 相关的错误，只剩未构建的兄弟 workspace 报错，这些在未修改的 head 上同样存在。所有改动文件的prettier与eslint均干净。证据（修复前后）
修复前，对一个已停止的 Goal 调用
get_goal：{"active":false,"lastGoal":{"goalId":"…","revision":3,"status":"usage_limited","turnCount":27,"activeTimeMs":1763705,"lastReason":"…"}}—— 有 27 轮，没有花费。修复后，同一次调用：摘要中额外携带
"tokensUsed":<n>，于是这次运行的花费与它的轮数可以从同一个地方读到。测试平台
环境（可选）
Linux、Node.js 22，仅单元测试。
风险与范围
tokensUsed在GoalRecord上是必填而非可选，这正是本 PR 触及二十个测试文件的原因。另一种做法——设为可选并在每个读取点上补默认值——会让 diff 变小，但会把undefined判断推给未来的每一个消费者，包括本 PR 正在为之铺路的那个预算比较。把字段做成完整的，并在解析边界上把缺失值迁移为零，使这份成本只在一个地方付出一次。tokensUsed执行上限，除get_goal摘要外也还没有 UI 展示它。meter 度量的是整个会话而不仅是 Goal 自己的请求，因此与自主运行交错的用户轮次会被归到当时打开的那个 Goal 轮上；把两者拆开需要今天并不存在的按请求归因，本 PR 有意不去尝试。Windows 和 macOS 未在本地验证，仍由 CI 覆盖。tokensUsed: 0而不是被拒绝；携带负值的快照会被拒绝。供 core triage gate 参考的规模：六个文件、新增 91 行、删除 2 行生产代码，其余为二十个测试文件中的 fixture 更新；改动跨包（packages/core与packages/cli），但每个 CLI 文件只是在 Goal fixture 中多一个字段。关联 Issue
无。