Skip to content

feat(goal): account the tokens a Goal spends - #9583

Merged
wenshao merged 2 commits into
QwenLM:mainfrom
qqqys:goal/a2-recorder-token-usage
Aug 22, 2026
Merged

feat(goal): account the tokens a Goal spends#9583
wenshao merged 2 commits into
QwenLM:mainfrom
qqqys:goal/a2-recorder-token-usage

Conversation

@qqqys

@qqqys qqqys commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

GoalRecord gains a tokensUsed field, summed across the Goal's turns by reduceGoalTurnFinished, and get_goal reports it in the unpermitted lastGoal summary beside the turn count. The figure comes from the chat recorder: recordAssistantTurn already receives each assistant turn's usage together with the Goal permit that produced it, so the recorder accumulates the open Goal turn's spend as it records it, and the Goal runtime asks for one turn by id when that 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 what a user actually asks about a long autonomous run, which is what it is costing. turnCount is a poor proxy: ten expensive turns and a thousand cheap ones are indistinguishable in it. It is also the unit every future limit has to be expressed in, so counting has to land before anything can bound it.

Attribution is settled where the spend is recorded rather than reconstructed afterwards. That matters more than it first appears. A per-session total would have to be sampled before and after each Goal turn and differenced, which attributes anything else happening in the session to whichever Goal turn was open — an interleaved user turn, or the stored telemetry a /resume replays into the session's totals. Reading the permit-stamped records instead makes a Goal turn's spend exactly the model calls that turn made, and removes any need for the accounting to coordinate with session swaps at all.

Reviewer Test Plan

How to verify

Run a Goal for several turns and read tokensUsed back — through get_goal once the Goal stops issuing permits, or from the persisted snapshot. Confirm it grows monotonically, that a turn's growth matches the usage of that turn's own model calls, and that it survives a session resume. Confirm that work outside the Goal is not billed to it: send a user turn between two Goal turns and check that its tokens land on neither. Then confirm the degenerate paths keep the Goal running rather than failing it — a runtime constructed with no ledger, a ledger that throws, and a turn that made no model calls all bill zero. Finally confirm migration: a Goal recovered from a transcript written before this field existed restores with tokensUsed: 0 rather than being rejected as malformed.

Twelve tests are added across three layers. The reducer gets accumulation across turns, a missing and a negative reading each adding nothing, migration of a snapshot persisted without the field, and rejection of negative spend. The runtime gets the per-turn figure landing on the record across two consecutive turns, an assertion that the ledger is asked for the finishing turn's id and nothing else, a runtime with no ledger, and a throwing ledger that still resolves the turn. The recorder gets summation and single-consumption, isolation between two turn ids, rejection of unusable totals, and — the one that matters most — an end-to-end case driving recordAssistantTurn itself, since a ledger that is never fed would report every Goal turn as free.

That last test was mutation-checked rather than merely observed to pass: deleting the accumulation call inside recordAssistantTurn fails exactly that test and leaves the other 106 in the suite green. It was restored afterwards and the suite re-run clean.

Numbers: 1027 tests pass across 17 core files (goals/, chatRecordingService, config), and 144 across four CLI Goal suites. npx tsc --noEmit exits clean in both packages/core and packages/cli after a full workspace build. prettier and eslint are clean on every changed file.

Evidence (Before & After)

Before, get_goal on 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 additionally carries "tokensUsed":<n>, so the run's spend is readable from the same place as its turn count.

Tested on

OS Status
🍏 macOS N/A
🪟 Windows ⚠️
🐧 Linux

Environment (optional)

Linux, Node.js 22, unit tests only.

Risk & Scope

  • Main risk or tradeoff: tokensUsed is required on GoalRecord rather than optional, which is why this touches twenty-five test files for one fixture line each. The alternative — an optional number defaulted at every read — keeps the diff small and pushes an undefined check 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 that cost in one place, once.
  • Not validated / out of scope: nothing enforces a limit against tokensUsed, and no UI surface displays it beyond the get_goal summary. The ledger counts totalTokenCount per assistant record, matching how the session's own metrics are summed, so a turn's several requests each count their resent prompt — that is what is billed, but it is not the same as "new tokens produced". Side queries that never reach recordAssistantTurn — the checkpoint and terminal verifiers among them — are not billed to the Goal. Windows and macOS were not exercised locally and remain covered by CI.
  • Breaking changes / migration notes: none at runtime. Snapshots persisted before this field existed parse to tokensUsed: 0 rather than being rejected, and a snapshot carrying a negative value is rejected. Scope for the core triage gate: 101 added and 2 deleted production lines across seven files, the largest being 40 lines of ledger on the recorder and 30 on the runtime; config.ts changes by four lines. The change is cross-package only in that twenty CLI test files each gain one fixture field.

Linked Issues

Supersedes #9301, which measured the same figure from the session's telemetry aggregate and consequently pulled a session-swap replay overhaul in with it. That replay work is now #9582, on its own merits.

中文说明

本 PR 做了什么

GoalRecord 新增 tokensUsed 字段,由 reduceGoalTurnFinished 在 Goal 的各轮之间累加;get_goal 在无 permit 的 lastGoal 摘要里与轮数并列返回它。这个数字来自 chat recorder:recordAssistantTurn 本来就同时收到每个 assistant 轮的用量和产生它的 Goal permit,因此 recorder 在记录的当下就累加当前打开的那个 Goal 轮的花费,而 Goal 运行时在某一轮结束时按 turn id 取走它。本 PR 不引入任何上限——它只负责计数。

为什么需要

Goal 会报告自己跑了多少轮、活跃了多久,而这两者都回答不了用户面对一次长自主运行时真正会问的问题:它花了多少。turnCount 是很差的代理——十轮昂贵的和一千轮便宜的在它里面无法区分。它同时也是未来任何上限必须使用的计量单位,所以「计数」必须先于「设限」落地。

归因在花费被记录的那一刻就确定,而不是事后重建,这一点比乍看之下更重要。若改用「按会话的总量」,就必须在每个 Goal 轮前后各采样一次再作差,于是会话里发生的其他一切都会被算到当时打开的那个 Goal 轮上——交错进来的用户轮次,或是 /resume 重放进会话总量的历史遥测。改为读取带 permit 印记的记录之后,一个 Goal 轮的花费恰好就是该轮自己发起的那些模型调用,并且整套记账完全不需要与会话切换做任何协调。

评审者测试计划

如何验证

让一个 Goal 跑若干轮,然后读回 tokensUsed——Goal 停止发放 permit 后通过 get_goal,或直接从持久化快照读。确认它单调增长、某一轮的增量与该轮自身模型调用的用量一致,并能在会话恢复后保留。确认 Goal 之外的工作不会记到它头上:在两个 Goal 轮之间发一条用户消息,检查它的 token 两边都不落。然后确认退化路径只会让 Goal 少一个花费数字、而不会让轮次失败——未配置 ledger 的运行时、会抛异常的 ledger、以及没有任何模型调用的轮次,都计零。最后确认迁移:从该字段出现之前写入的转录中恢复的 Goal,会以 tokensUsed: 0 恢复,而不是被判为格式非法。

新增十二个测试,分布在三层。reducer 层:跨轮累加、缺失读数与负数读数都不增加、对未持久化该字段的快照做迁移、拒绝负数花费。runtime 层:连续两轮的每轮数字正确落到记录上、断言 ledger 只会被问及正在结束的那个 turn id、未配置 ledger 的运行时、以及抛异常的 ledger 仍能让轮次正常结束。recorder 层:求和与一次性取走、两个 turn id 之间互不串账、拒绝不可用的总量,以及最关键的一个——直接驱动 recordAssistantTurn 的端到端用例,因为一个从未被喂过的 ledger 会把每个 Goal 轮都报成免费。

最后那个测试做了变异检验,而不只是「跑通了」:删掉 recordAssistantTurn 内的累加调用,恰好只有它失败,套件里其余 106 个仍绿。随后已还原并重跑干净。

数字:core 侧 17 个文件(goals/chatRecordingServiceconfig)共 1027 个测试通过,CLI 侧四个 Goal 套件共 144 个通过。完成一次完整 workspace 构建后,npx tsc --noEmitpackages/corepackages/cli 均干净退出。所有改动文件的 prettiereslint 均干净。

证据(修复前后)

修复前,对一个已停止的 Goal 调用 get_goal{"active":false,"lastGoal":{"goalId":"…","revision":3,"status":"usage_limited","turnCount":27,"activeTimeMs":1763705,"lastReason":"…"}} —— 有 27 轮,没有花费。

修复后,同一次调用额外携带 "tokensUsed":<n>,于是这次运行的花费与它的轮数可以从同一个地方读到。

测试平台

操作系统 状态
🍏 macOS N/A
🪟 Windows ⚠️
🐧 Linux

环境(可选)

Linux、Node.js 22,仅单元测试。

风险与范围

  • 主要风险或取舍:tokensUsedGoalRecord 上是必填而非可选,这正是本 PR 触及二十五个测试文件、每个各加一行 fixture 的原因。另一种做法——设为可选并在每个读取点补默认值——会让 diff 变小,但会把 undefined 判断推给未来的每一个消费者,包括本 PR 正在为之铺路的那个预算比较。把字段做成完整的,并在解析边界把缺失值迁移为零,使这份成本只在一处付出一次。
  • 未验证/不在范围:目前没有任何东西依据 tokensUsed 执行上限,除 get_goal 摘要外也还没有 UI 展示它。ledger 按每条 assistant 记录累加 totalTokenCount,与会话自身指标的求和口径一致,因此一轮内的多次请求各自都会计入其重发的 prompt——这就是实际计费的口径,但与「新产生的 token」不是一回事。不经过 recordAssistantTurn 的 side query(其中包括 checkpoint 与终审 verifier)不计入 Goal。Windows 和 macOS 未在本地验证,仍由 CI 覆盖。
  • 破坏性变更/迁移说明:运行时无破坏性变更。该字段出现之前持久化的快照会解析为 tokensUsed: 0 而不是被拒绝;携带负值的快照会被拒绝。供 core triage gate 参考的规模:七个文件、新增 101 行、删除 2 行生产代码,其中最大的两块是 recorder 上 40 行 ledger 与 runtime 上 30 行;config.ts 只变动四行。跨包之处仅在于二十个 CLI 测试文件各多一行 fixture 字段。

关联 Issue

取代 #9301 —— 那个 PR 从会话遥测聚合中测量同一个数字,因而把一整套会话切换重放大修一并拖了进来。那部分重放工作现已独立为 #9582,按自身价值评审。

A Goal reported how many turns it had run and how long it had been active, but
never what it cost. That is the number a user needs to judge whether a long
autonomous run is worth continuing, and the one 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 beside the turn count.

The figure comes from the chat recorder, which already receives every assistant
turn's usage stamped with the Goal permit that produced it. Attribution is
therefore settled where the spend is recorded rather than reconstructed
afterwards from session totals: a user turn interleaved with an autonomous run
belongs to no Goal turn, and a resumed session's replayed history is not a Goal
turn's spend either. The runtime asks the ledger for one turn by id when that
turn finishes, which is also why the accounting needs no coordination with
session swaps.

A runtime with no ledger, a ledger that throws, and a turn that made no model
calls all bill zero rather than guessing, and none of them fails the turn.
Goals recovered from a transcript written before the field existed restore with
zero spend. No limit is introduced here — this only counts.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — this is a well-scoped one.

Template looks good ✓

Problem: real and observable. Today get_goal on a stopped Goal reports turnCount and activeTimeMs but nothing about cost — the author shows the current output shape as evidence. Token spend is the number users actually ask about after a long autonomous run, and it's also the unit any future budget would be expressed in, so counting has to land before bounding can.

Direction: aligned. Goals are a first-class feature here, and spend/usage accounting is squarely within mission — the reference agent's changelog shows the same area getting active investment (usage spend rows, gateway spend limits). No escalation triggers touched (the get_goal surface change is additive).

Size: core paths are touched (goals/, chatRecordingService, config), so the two-tier gate applies: 103 production lines (101 added, 2 deleted across 7 files) vs 336 test lines (25 files, almost all one fixture line each). Well under every size threshold; Tier 2's full-confidence bar applies to the review instead.

Approach: the scope feels right, and the central design choice is the correct one — attributing spend where it is recorded (permit-stamped records in recordAssistantTurn) rather than differencing session totals before/after each Goal turn. The differencing route would misattribute interleaved user turns and /resume replayed telemetry, exactly as the description says. I checked the single-entry ledger's premise against the runtime: every permit-issuance path (flushContinuation, promoteQueuedUserTurn, the re-issue paths) is guarded on currentPermit being clear, and takeTurnTokens runs inside finishTurn before that clear — so a record stamped with a different turnId genuinely means the prior turn is over. Making the field required on GoalRecord with migration at the parse boundary (instead of an optional field defaulting at every read site) is a defensible tradeoff, and the PR names it honestly. No drive-by changes spotted; supersedes-#9301 and the split-out #9582 are linked.

Risk: no elevated risk signals — no revert-correlated paths among the production files.

Moving on to code review. 🔍

中文说明

感谢贡献!这个 PR 的范围控制得很好。

模板完整 ✓

问题:真实且可观测。目前对一个已停止的 Goal 调用 get_goal 只会返回 turnCountactiveTimeMs,完全没有花费信息——作者展示了当前的输出形状作为证据。token 花费是用户在一次长自主运行后真正会问的数字,也是未来任何预算上限必须使用的计量单位,所以必须先有计数才能谈设限。

方向:对齐。Goal 是本仓库的一等特性,花费/用量记账完全在核心使命之内——参考 agent 的 changelog 显示同一领域正在被持续投入(usage 花费行、gateway 花费上限)。未触及任何升级项(get_goal 的表面变更是纯增量的)。

规模:触及核心路径(goals/chatRecordingServiceconfig),两层门禁适用:103 行生产代码(7 个文件,新增 101、删除 2)对 336 行测试(25 个文件,几乎每个只加一行 fixture)。远低于所有规模阈值;改由 Tier 2 的全置信标准在 review 中把关。

方案:范围合理,核心设计选择也是对的——在花费被记录的那一刻归因(recordAssistantTurn 中带 permit 印记的记录),而不是在每个 Goal 轮前后对会话总量作差。作差方案会把交错的用户轮次和 /resume 重放的遥测错误地算进 Goal 轮,正如 PR 描述所说。我对照运行时核实了单条 ledger 的前提:所有 permit 发放路径(flushContinuationpromoteQueuedUserTurn、重发路径)都以 currentPermit 已清空为前提,而 takeTurnTokensfinishTurn 内部、清空之前执行——因此一条盖着不同 turnId 印记的记录确实意味着上一轮已经结束。把字段设为 GoalRecord 上必填、并在解析边界做迁移(而不是设为可选、在每个读取点补默认值)是一个站得住的取舍,PR 也诚实地说明了这一点。未发现夹带的无关改动;对 #9301 的取代和拆分出的 #9582 都已关联。

风险:无升级风险信号——生产文件未命中任何与 revert 相关的高风险路径。

进入代码审查 🔍

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

My independent proposal for "account the tokens a Goal spends" was exactly the shape this PR takes: a required tokensUsed on GoalRecord (zero at creation and at legacy migration), accumulation at the one place that already sees both the usage and the permit — recordAssistantTurn — a take-by-turn-id consumed in finishTurn and carried through reduceGoalTurnFinished, additive exposure in the get_goal lastGoal summary, and degenerate paths that cost the Goal its figure, never the turn. The PR matches that baseline; I didn't find a simpler path it missed.

What I verified by reading the runtime (not just the diff):

  • Single-entry ledger is safe. Every permit-issuance path (flushContinuation, promoteQueuedUserTurn, the start-failure re-issue) is guarded on currentPermit being clear, and takeTurnTokens runs inside finishTurn's enqueued body before that clear — so records stamped with a different turnId can only appear after the prior turn's total was already taken or abandoned. A stale permit (e.g. after a revision bump) that retries finishTurn gets 0, never another turn's total.
  • Attribution is airtight for the cases named. Non-Goal records never touch the ledger; an interleaved user turn without goalContext neither adds to nor resets it; a record with a usable total but a new turnId resets before adding, so abandoned turn spend is dropped, not carried over.
  • Clamping is doubled but harmless — runtime clamps to finite ≥ 0 and the reducer clamps again. Parse boundary rejects negative persisted values and migrates absent ones to 0 (hasOnlyKeys is an allow-list, so pre-field snapshots still parse).
  • One production wiring site (Config), and it's wired; the ledger stays optional so test/standalone runtimes degrade to billing zero.
  • tokensUsed surfaces only through the one Pick<GoalRecord> in the repo (LastGoalSummary), updated here with the tool description kept in sync.

One non-blocking nit:

  • In chatRecordingService.ts, the new goalTurnSpend field and its two methods are inserted between recordAssistantTurn's existing JSDoc block and the method itself. The @param data.* docs now attach to the private goalTurnSpend field, and recordAssistantTurn loses its docs. Moving the new members above that doc block fixes it — worth doing before merge, but not blocking.

The 25 test-file churn is one fixture line each, the mechanical consequence of making the field required — acceptable, and the alternative (optional + default at every read) is the worse tradeoff for the budget comparison this is groundwork for. The author's mutation-testing claim on the end-to-end ledger test is their report, not independently re-run here (static review only; this run never executes PR code).

sequenceDiagram
    participant P1 as Goal turn host
    participant P2 as ChatRecordingService
    participant P3 as goal-runtime finishTurn
    participant P4 as goal reducer
    P1->>P2: recordAssistantTurn usage plus permit
    P2->>P2: accumulate spend for open turnId
    P1->>P3: finishTurn permit
    P3->>P2: takeGoalTurnTokens turnId
    P2-->>P3: tokens, consumed once
    P3->>P4: reduceGoalTurnFinished tokensUsed
    P4-->>P3: GoalRecord with accumulated spend
Loading

Test evidence (PR's own CI, via API — this run executes no PR code)

CI on the reviewed commit is still settling: the main unit-suite workflow (Qwen Code CI) and Serve A/B are in progress; the table below is updated in place by the finalize job once they land. One red check: SDK Java failed in the packages/sdk-java Maven surefire on the Java 11 cell while the Java 17/21 cells and Real daemon E2E / Java 11 passed. Classification: not caused by this PR — the diff contains zero files under packages/sdk-java (it is entirely TypeScript), so a Java SDK test failure cannot be attributed to it; treat as pre-existing infra noise. The author's local numbers (1027 core + 144 CLI tests) are their claim; CI is the evidence that counts here. Security Checks (incl. Dependency CVE audit, Secret scan, Desktop Shell) completed green.

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

Check Conclusion
ubuntu-latest / Java 11 ❌ failure
Classify PR ✅ success
Dependency CVE audit ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
macos-latest / Java 21 ✅ success
Real daemon E2E / Java 11 ✅ success
Secret scan (TruffleHog) ✅ success
Serve A/B (ubuntu-latest, Node 22.x) ✅ success
Test (ubuntu-latest, Node 22.x) ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
web-shell E2E Smoke (ubuntu-latest, Node 22.x) ✅ success
windows-latest / Java 21 ✅ success

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

Sandboxed verification would settle the remaining live-behavior gap: @qwen-code /verify — that tokensUsed on a real multi-turn Goal grows with the turn's own model calls and survives resume is asserted by the added tests but not observed live in this static run; as a fork author without write access this would be a sponsored run (a maintainer posts the trigger, which approves the head it runs against; the run carries a pre-execution risk screen and a full workspace wipe — read its report with the same skepticism as the fork's own CI logs).

中文说明

代码审查

我对"统计 Goal 花费的 token"的独立方案与本 PR 完全一致:在 GoalRecord 上设必填的 tokensUsed(创建与旧数据迁移时为零),在唯一同时能看到用量和 permit 的地方——recordAssistantTurn——做累加,按 turn id 一次性取走,在 finishTurn 中经由 reduceGoalTurnFinished 入账,增量暴露到 get_goallastGoal 摘要,退化路径只让 Goal 少一个数字、不失败轮次。PR 与该基线一致,没有发现更简的遗漏路径。

对照运行时核实(不只看 diff):

  • 单条 ledger 是安全的。 所有 permit 发放路径都以 currentPermit 已清空为前提,而 takeTurnTokensfinishTurn 入队体内、清空之前执行——盖着不同 turnId 的记录只可能出现在上一轮的数字已被取走或放弃之后。过期 permit(如 revision 抬升后)重试 finishTurn 只会拿到 0,永远不会拿到别的轮次的总量。
  • 归因对所述场景无漏洞。 非 Goal 记录不碰 ledger;不带 goalContext 的交错用户轮次既不增加也不重置;带可用总量但 turnId 不同的记录先重置再累加,被放弃轮次的花费会丢弃而非串账。
  • 双重钳位但无害——运行时钳位为有限且 ≥0,reducer 再钳一次。解析边界拒绝持久化的负值、把缺失迁移为 0(hasOnlyKeys 是允许名单,旧快照仍可解析)。
  • 唯一的生产装配点Config)已接线;ledger 保持可选,测试/独立运行时退化为计零。
  • tokensUsed 只经由仓库中唯一的 Pick<GoalRecord>LastGoalSummary)暴露,此处已更新,工具描述同步。

一个非阻塞问题:

  • chatRecordingService.ts 中,新的 goalTurnSpend 字段与两个方法被插在 recordAssistantTurn 既有 JSDoc 块和方法之间,导致 @param data.* 文档挂到了私有字段上、方法本身失去文档。把新成员移到该文档块上方即可——合并前值得修,但不阻塞。

25 个测试文件的改动每个只有一行 fixture,是字段必填化的机械结果——可以接受;备选方案(可选字段 + 每个读取点补默认)对正在为之铺路的预算比较而言更糟。作者关于端到端 ledger 测试的变异检验说法是其自述,本次未独立重跑(静态审查;本流程从不执行 PR 代码)。

测试证据(PR 自身的 CI,经 API 获取——本次审查不执行任何 PR 代码)

被审 commit 上的 CI 尚未落定:主单测工作流(Qwen Code CI)与 Serve A/B 仍在运行,下方表格由 finalize 任务在结果落地后原位更新。一个红色检查:SDK Java 在 Java 11 单元的 packages/sdk-java Maven surefire 上失败,而 Java 17/21 单元与 Real daemon E2E / Java 11 通过。判定:非本 PR 所致——diff 中没有任何 packages/sdk-java 下的文件(全部是 TypeScript),Java SDK 测试失败无法归因于它,按既有基础设施噪音处理。作者本地数字(core 1027 + CLI 144 个测试)是其自述;这里以 CI 为准。Security Checks(含依赖 CVE 审计、密钥扫描、Desktop Shell)已通过。

沙箱验证可以补足剩余的实运行缺口:@qwen-code /verify——多轮 Goal 运行中 tokensUsed 随该轮自身模型调用增长且在 resume 后保留,这一行为由新增测试断言、但未在本静态流程中实际观测;由于作者是 fork 贡献者(无写权限),这将是受助运行:由 maintainer 发出触发指令(即批准其运行所针对的 head),运行前有执行风险筛查与全工作区清理——请对该报告保持与 fork 自身 CI 日志同等的审慎。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean, minimal, well-reasoned change that does exactly one thing; the one nit (orphaned JSDoc in chatRecordingService.ts) is cosmetic, and CI is still running.

Stepping back: this is what a good fork PR looks like. The problem is real (Goal runs report turns and wall-time but never cost), the design choice that matters — attribute spend at record time via permit-stamped records instead of differencing session totals — is the right one and the description explains why it matters (interleaved user turns, /resume replayed telemetry), the scope stopped at counting and deliberately didn't drag a budget mechanism along, and the superseded predecessor (#9301) was honestly decomposed rather than patched. Every line in the diff earns its place; the 25-file fixture churn is the mechanical price of a total field, paid once, at the parse boundary. If I'm maintaining this in six months, the single-entry ledger with its single-permit invariant is easy to hold in my head, and the degenerate paths (no ledger, throwing ledger, zero-spend turn, pre-field snapshot) all fail toward "Goal keeps running", which is the correct direction for bookkeeping.

The reason this is 4 and not 5: the unit suite hasn't landed yet, so the green that would pin the twelve new tests is still an expectation, not a fact — and the JSDoc placement nit is worth a follow-up commit. Neither is a reason to hold the PR up beyond CI.

Verdict: approve — the review is done and I'm confident in the change. Because Qwen Code CI and Serve A/B are still in progress on the reviewed commit, approval is deferred until CI lands green on 9cc705b9222cbff7851e3c76770ae5ba8e1e7156; the finalize job performs the commit-pinned approval once every check completes. One caveat: the unrelated SDK Java check is red on this commit (classified pre-existing above — the diff contains no Java files). If it stays red, finalize will withhold the approval and flag it; that is the point for a maintainer's human call (or a re-run of that workflow), not a reflection on this PR.

中文说明

回顾整体:这是一个优质 fork PR 的样子。问题真实(Goal 运行只报告轮次和时长,从不报告花费),关键设计选择——在记录时通过带 permit 印记的记录归因花费,而不是对会话总量作差——是正确的,描述也解释了为什么重要(交错的用户轮次、/resume 重放的遥测);范围止步于计数,没有顺手拖入预算机制;被取代的前作(#9301)被诚实地分解而非修补。diff 中每一行都物有所值;25 个文件的 fixture 改动是字段必填化的一次性机械代价,付在解析边界。六个月后维护这段代码时,单条 ledger 与其单 permit 不变量很容易装进脑子,退化路径(无 ledger、抛异常的 ledger、零花费轮次、旧快照)全部朝"Goal 继续运行"方向失败——对记账而言这是正确的方向。

打 4 分而非 5 分的原因:单测套件尚未落定,钉住十二个新测试的绿灯目前还是预期而非事实;JSDoc 位置问题值得补一个 commit。两者都不构成在 CI 之外继续卡住 PR 的理由。

结论:approve——审查已完成,我对这个改动有信心。由于被审 commit 上 Qwen Code CIServe A/B 仍在运行,批准推迟到 CI 在 9cc705b9222cbff7851e3c76770ae5ba8e1e7156 上全绿后由 finalize 任务执行(绑定到被审 commit)。一个提醒:无关的 SDK Java 检查在该 commit 上是红的(上面已判定为既有问题——diff 不含任何 Java 文件);若它保持红色,finalize 会暂缓批准并标记,届时由 maintainer 人工决断(或重跑该工作流),与本 PR 的质量无关。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

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

No response changes against the PR base across 4 scenario(s).

Qwen Code · serve A/B

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Suggestions are inline.

中文说明

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

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

Comment on lines +1931 to +1932
/**
* Tokens billed to the Goal turn that is currently open.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] The new ledger block (goalTurnSpend, accumulateGoalTurnTokens, takeGoalTurnTokens) was inserted between recordAssistantTurn's existing JSDoc and the method itself, orphaning that JSDoc: recordAssistantTurn — the method the new accumulation call hooks into — is left without documentation, and the dangling @param data.message/model/tokens/… block now sits directly above the unrelated private goalTurnSpend field, where IDE hover / doc tooling and the next reader will take it as describing the field. A compiler-API probe over this file confirms it: after the insertion recordAssistantTurn has 0 leading JSDoc blocks, while goalTurnSpend has 2, the first being the orphaned block. Moving the three new members to after the recordAssistantTurn method body (or above its JSDoc) keeps the existing JSDoc adjacent to the method it documents.

probe (tsc compiler API): recordAssistantTurn → 0 leading JSDoc blocks; goalTurnSpend → 2 (first = orphaned "@param" block)
中文说明

新的 ledger 代码块(goalTurnSpendaccumulateGoalTurnTokenstakeGoalTurnTokens)被插入在 recordAssistantTurn 已有的 JSDoc 与方法本身之间,导致该 JSDoc 孤立:recordAssistantTurn(新的累加调用所嵌入的方法)失去了文档,而悬空的 @param data.message/model/tokens/… 块现在恰好位于无关的私有字段 goalTurnSpend 上方,IDE 悬停 / 文档工具和后续读者会把它理解为在描述该字段。编译器 API 探针证实了这一点:插入后 recordAssistantTurn 的前置 JSDoc 块为 0 个,而 goalTurnSpend 有 2 个,第一个正是被孤立的那个块。建议把这三个新成员移到 recordAssistantTurn 方法体之后(或其 JSDoc 之前),使现有 JSDoc 保持与它所 documenting 的方法相邻。

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in e292e3b. The assistant-turn JSDoc is attached to its method again. Validation: root build and typecheck, targeted ESLint and Prettier passed.

Comment on lines +7891 to +7894
// The recorder already sees every assistant turn's usage stamped with
// the Goal permit that produced it, so the spend is Goal-scoped at the
// point it is recorded rather than reconstructed from session totals.
tokenLedger: recorder,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] tokenLedger: recorder is the only production site connecting the ChatRecordingService ledger to the Goal runtime, and no test covers it: the goal-runtime tests use only fake ledgers (and explicitly bless the no-ledger case), the chatRecordingService tests exercise the ledger methods in isolation, and every get_goal fixture asserts zero. A mutation probe confirms the exposure: removing this line leaves 1049 tests across the goals, chatRecordingService, config, and client-goal suites passing — while in production every Goal would report tokensUsed: 0 for its entire life with the whole suite green. Consider adding a Config-level test: build a Config with a chat recording service, initialize the Goal runtime, record an assistant turn stamped with a Goal permit, finish the turn, and assert the snapshot's tokensUsed is non-zero (or spy on takeGoalTurnTokens to prove the runtime queries the recorder instance).

mutation probe: removed tokenLedger: recorder → 1049 tests still pass
中文说明

tokenLedger: recorder 是把 ChatRecordingService 账本连接到 Goal 运行时的唯一生产代码位置,但没有任何测试覆盖它:goal-runtime 测试只用假账本(且明确认可无账本的情况),chatRecordingService 测试只单独验证账本方法,而所有 get_goal 夹具都断言零值。变异探针证实了这个暴露面:删掉这一行后,goals、chatRecordingService、config、client-goal 套件中的 1049 个测试仍然全部通过——而生产中每个 Goal 将在其整个生命周期内报告 tokensUsed: 0,整套测试却仍是绿的。建议增加一个 Config 层测试:构建带 chat recording service 的 Config,初始化 Goal 运行时,记录一条带 Goal permit 印记的 assistant 轮,结束该轮,并断言快照的 tokensUsed 非零(或 spy takeGoalTurnTokens,证明运行时确实查询了该 recorder 实例)。

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in e292e3b. Added an observable Config-level regression covering real recorder-to-runtime Goal token accounting. Validation: focused Core tests passed 570/570, the three-file set passed 677/677, and root build/typecheck passed.

Comment on lines +175 to +177
turnCount: 27,
activeTimeMs: 1_763_705,
tokensUsed: 0,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] All three GetGoalTool lastGoal-summary tests pin tokensUsed: 0, so nothing proves non-zero spend is actually reported — a mutation probe confirms the hole: changing tokensUsed: goal.tokensUsedtokensUsed: 0 in goal-tools.ts's lastGoal() leaves this suite passing 35/35, so a regression that hardcodes or drops the spend when rebuilding the summary ships green and get_goal reports a Goal as free while the underlying GoalRecord holds real spend. Giving one fixture non-zero spend (and asserting it in the expected lastGoal object) closes the hole; the suggestion below updates this fixture — remember to update the matching expectation to tokensUsed: 4_500 as well.

mutation probe: tokensUsed: goal.tokensUsed → tokensUsed: 0 → goal-tools.test.ts still passes 35/35
Suggested change
turnCount: 27,
activeTimeMs: 1_763_705,
tokensUsed: 0,
turnCount: 27,
activeTimeMs: 1_763_705,
tokensUsed: 4_500,
中文说明

GetGoalTool 的三个 lastGoal 摘要测试都把 tokensUsed 固定为 0,因此没有任何测试证明非零花费会被真正报告——变异探针证实了这个漏洞:把 goal-tools.ts lastGoal() 中的 tokensUsed: goal.tokensUsed 改成 tokensUsed: 0,该套件仍以 35/35 通过,意味着重建摘要时把花费硬编码或丢掉的回归会绿灯上线,get_goal 会把实际持有花费的 Goal 报告为免费。把其中一个夹具改为非零花费(并在期望的 lastGoal 对象中断言它)即可补上漏洞;下面的 suggestion 更新了该夹具——请同时把对应的期望值更新为 tokensUsed: 4_500

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

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in e292e3b. The last-Goal fixture now carries and asserts non-zero token usage. Validation: focused goal-tools and Config tests passed 570/570.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

No blocking issues. LGTM! ✅

Test Plan (not a blocker): 1027 tests pass — this review observed 1638, 22570, 20725, 1616, 494, 3860, 556 passed.

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

  • packages/core/src/services/chatRecordingService.test.ts:3092 — [review] triplicated private-method cast for accumulateGoalTurnTokens in the new ledger tests
  • packages/core/src/services/chatRecordingService.ts:1924 — [review] single-slot ledger silently discards spend of turns ended by permit invalidation (cancel/pause/replace/dispose)
  • packages/core/src/goals/goal-reducer.ts:460 — [probe] no test round-trips a non-zero tokensUsed through parseGoalRecord (mutation survives the whole suite)
  • packages/core/src/goals/goal-runtime.ts:277 — [review] ledger failure modes collapse to tokensUsed: 0 with zero logging
中文说明

无阻断问题。LGTM!✅

Test Plan(非阻断):1027 tests pass — this review observed 1638, 22570, 20725, 1616, 494, 3860, 556 passed

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

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

@wenshao
wenshao added this pull request to the merge queue Aug 22, 2026
Merged via the queue into QwenLM:main with commit bd24712 Aug 22, 2026
146 of 148 checks passed
@wenshao

wenshao commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

🧪 Local deep verification — verdict: merge-ready (31/31 scripted assertions, 0 unexpected failures)

Ran a local maintainer verification round on a real build: PR head e292e3b trial-merged into current main tip 7c039e01 (clean merge — main had advanced 75 commits and touched this PR's own files, so the merge is what was verified), against main tip as the control arm. Both arms built from the same untouched lockfile; the harness drove each tree's built packages/core dist with the runtime wired exactly as config.ts wires it.

A/B — the central claim is load-bearing

One identical Goal scenario on both arms: two Goal turns making 900+100 then 700 tokens, with an interleaved 5000-token non-Goal record and a NaN reading as decoys.

Cell base (main tip) head (trial merge)
Recorder turn-token ledger absent present
Turn 1 bills its own records only (900+100; 5000 non-Goal and NaN excluded) undefined 1000
Turn 2 accumulates (+700) undefined 1700
Re-take of a taken turn n/a 0
Pre-field snapshot migration parses, no field parses, tokensUsed: 0
Negative spend snapshot rejected rejected
No ledger / throwing ledger / zero-call turn n/a turn finishes, bills 0

Head 12/12, base 8/8 (base cells assert the predicted absence — the control reds). The number exists only at head and is Goal-scoped, not session-scoped.

A/B output, base vs head

Ordering check (a manual record→take order in tests could hide an inverted production order ⇒ systematic undercount): accumulateGoalTurnTokens runs synchronously inside recordAssistantTurn during stream consumption, and all seven production finishTurn call sites (client.ts:4048, client.ts:2512/2693, nonInteractiveCli.ts:705/747, Session.ts:2346, useGeminiStream.ts:614/4978, live-session-coordinator.ts:817) run after the finishing turn's records are already accumulated. No take-before-record path exists.

Mutation matrix — 4/4 killed, no survivors

Mutation (one guard deleted) Suite Result
accumulate call in recordAssistantTurn recorder suite killed — exactly 1 failed / 118 passed
ledger take wiring in runtime finishTurn goal-runtime suite killed — 2 / 107
tokenLedger: recorder in config.ts (production wiring) goals + recorder + config (1047) killed — 1 / 1046
reducer accumulation goal-reducer suite killed — 1 / 70

The config-wiring row matters most: the PR ships a real-Config end-to-end test (bills Goal turns through the canonical chat recorder) that pins the production seam behaviorally. Every layer — recorder feed, runtime take, config wiring, reducer accumulate — has a test that fails when its guard is deleted. The author's own mutation claim (deleting the accumulate call fails exactly one test) is independently confirmed.

Mutation matrix output

Gates

core affected suites 1167/1167 · cli goal suites 132/132 · acp-bridge transcript-replay 39/39 · tsc --noEmit clean in core and cli · base-arm core suites 493/493 (no pre-existing failures to attribute).

Findings

No blockers, no coverage gaps. One low-severity note, design-consistent: goalTurnSpend is in-memory, so a mid-turn process crash loses that turn's accumulated-but-untaken spend (recovery bills it 0). Undercount-only, matching the PR's "bill zero rather than guess" semantics — worth knowing for the future limit work.

Not covered: Windows; UI rendering beyond the JSON summary; retry re-billing semantics (declared as the billing basis); limit enforcement (doesn't exist yet); full serve/ACP end-to-end Goal runs (no in-repo harness; CI serve-ab already covered daemon routes).

Full report, harnesses, and raw logs: run artifacts from the local round (maintainer's machine, darwin, Node 24).

中文摘要(点开)

结论:merge-ready —— 31/31 断言全过,无阻塞问题。

  • 验证方式:本地维护者轮次,真实构建环境。head = PR head e292e3b 试合并进当前 main tip 7c039e01(干净合并;main 已领先 75 个提交且动过本 PR 的文件,故验证的是合并后的树),base = main tip 对照。两臂同一 lockfile,harness 驱动各自构建出的 packages/core dist,并按 config.ts 的方式接线。
  • A/B 结论:同一场景下 base 无 tokensUsed;head 第 1 轮计 1000(900+100,穿插的 5000 非 Goal 记录与 NaN 均排除)、第 2 轮累计 1700、重复取为 0;旧快照迁移 0、负值拒绝;无 ledger / 抛异常 ledger / 零调用轮均计 0 且轮次照常完成。12/12 vs 8/8。该数字只在 head 存在,且按 Goal 归因、非会话总量。
  • 顺序核查:accumulate 在流消费期间同步完成,全部 7 个生产 finishTurn 调用点都在记录之后——不存在 take 早于 record 的路径,无系统性少计。
  • 变异矩阵 4/4 全灭:recorder 喂数、runtime 取数、config 生产接线、reducer 累加四层各有测试 pin 住;config 层由 PR 自带的真实 Config 端到端测试按行为 pin 住。作者"删累加调用恰好失败一个测试"的声明独立复核一致。
  • 门禁:core 1167/1167、cli 132/132、acp-bridge 39/39、双 tsc --noEmit 干净、base 493/493(无预存失败)。
  • 唯一备注(低危,设计内):进程在 Goal 轮中途崩溃会丢该轮已累积未取的账目(恢复后计 0,只少计不多计),与 PR"宁可计零不猜测"语义一致;未来做限额时需知此口径。
  • 未覆盖:Windows;UI 呈现;重试重发 prompt 的计费口径(PR 已声明);限额 enforcement;完整 serve/ACP 端到端(仓库无现成 harness,CI serve-ab 已覆盖 daemon 路由)。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.22.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants