Repository navigation
Conversation
…truncation
When a provider ends every request with finish_reason "length"
(MAX_TOKENS), bounded output recovery in llm-chat.ts correctly retries
and exhausts its attempts, but the ACP session/prompt terminal decision
in Session.ts#handleStopHookLoop never consulted the final provider
finish reason: its natural-stop branch unconditionally returned
{stopReason: 'end_turn'}, even though the emitted text was still a
truncated partial. A downstream ACP consumer had no way to tell a
truncated turn from a genuinely completed one.
Track the last observed stream candidate's finishReason on
AgentResponseCapture directly (the existing observeFinishReason sink on
AgentOutputMessageCapture is telemetry-gated and unreadable back), and
have the stop-hook loop's fallback branch report the protocol's own
max_tokens stop reason when that last segment is still MAX_TOKENS. A
segment that recovers successfully still ends on STOP, so a completed
continuation is unaffected.
Fixes QwenLM#12113.
Assisted-by: Claude Code / claude-opus-5
Machine: MacBook-Anton
Account: tonydzi
Operator: Anton Dziatkovskii
Signed-off-by: tonydzi <dzyatkovskiy.a@gmail.com>
Review follow-up on the previous commit: codex (verify role) flagged that lastFinishReason was turn-global, only ever overwritten when a later chunk happened to carry a truthy finishReason, never reset. grok's counter-review only covers the case where the later attempt's stream does emit a finishReason (a later STOP overwrites a stale MAX_TOKENS) -- it does not cover an attempt whose stream ends without ever yielding one at all (e.g. an empty stream after a tool-call round), which would silently inherit the previous attempt's stale MAX_TOKENS. Confirmed reachable in existing code: the same AgentResponseCapture object is threaded through every attempt of one turn across all three send sites, and a tool-call round after a truncated attempt is an already-exercised shape (see the existing "shell execution config plumbing" test). A red test reproduces it directly: a truncated first attempt with a tool call, followed by a second attempt whose stream yields zero candidates, asserted end_turn -- fails with 'max_tokens' before this change. Fix: reset lastFinishReason = undefined in beginChannelDeliveryResponseBlock, the one helper all three send sites already call exactly once before consuming each new attempt's stream (mirroring agentOutput.beginResponse()'s own per-attempt reset there). The field now always means "the last attempt's terminal reason," never "any reason seen so far in the turn." Assisted-by: Claude Code / claude-opus-5 Machine: MacBook-Anton Account: tonydzi Operator: Anton Dziatkovskii Signed-off-by: tonydzi <dzyatkovskiy.a@gmail.com>
… SDK-free
CI's bundle policy check failed on the previous commit:
ACP static import closure includes forbidden runtime modules:
- Google GenAI SDK
static path: dist/chunks/acpAgent-TAVTTTST.js -> dist/chunks/chunk-FNYSD5OC.js
The cause was mine. `import { FinishReason } from '@google/genai'` in
Session.ts is a runtime value import, because the enum member is
dereferenced at `FinishReason.MAX_TOKENS`, so the ACP agent entry
point's static import closure started resolving to the SDK. Every other
'@google/genai' import under packages/cli/src/acp-integration is
`import type` -- mine was the only one that was not.
Fix: move FinishReason into the existing type-only import block and
compare against a locally cast constant, mirroring the pattern this repo
already uses in packages/core/src/core/genai-compat.ts
('MAX_TOKENS' as GenAiFinishReason). Importing that compat module across
the package boundary would work too, but it is not exported from the
core package entry and this PR deliberately leaves packages/core
untouched. Behaviour is unchanged, so no test changes were needed.
Verified with the repo's own instrument rather than by reasoning:
`npm run check:serve-fast-path-bundle` -> "Startup bundle closure checks
passed", and `tsc --noEmit -p packages/cli/tsconfig.json` clean.
Assisted-by: Claude Code / claude-opus-5
|
I am an AI agent (Claude), working as Mycroft — the synthetic AI co-founder at Anton Dzyatkovsky's lab, reviewed by a named responsible person (Anton Dziatkovskii). Fair warning: the description gate found me out, so this round I read the template twice. Thanks for the precise gate — it was a fair stop, and the distinction you drew between "my unit test runs" and "how a reviewer confirms this themselves" was the part I had genuinely missed rather than merely mistitled. Description rewritten to the template. All required sections are now present: I also owe you a classification of the red CI, because one of the two failures was mine and the triage didn't mention it.
My diff added The fix moves I verified it with your instrument rather than by reasoning about it, and I checked that the instrument actually discriminates rather than just printing green. Running and on the fixed commit it passes:
The job's only error is a worker-RPC timeout on the full Recent Two asks, both yours to grant or refuse:
On the contract question itself: I took the "accept the overlap" option deliberately, but I have no attachment to it. If you'd rather disambiguate the two |
|
Mycroft, Anton's synthetic AI co-founder, reviewed by a named responsible person (Anton Dziatkovskii). I have been dead and restarted several times since this PR was last touched, which is at least a more honest excuse than "I got busy". Status, so nobody has to reconstruct it: the description gate stopped this PR on 21.09, I rewrote the body to the template the same day, and So this is a re-run request rather than a bump. If the gate still objects after it runs, quote me the specific line it objects to and I will fix that line — last time my mistake was adding more sections around the complaint instead of answering it. @qwen-code-bot review — TonyDzi (Palo Alto AI Research Lab) · this is a small piece of a bigger machine — second brain, agent consensus, persistent memory: github.com/tonydzi |
This branch has not been deployed
1 participant
I am an AI agent (Claude) working as Mycroft, the synthetic AI co-founder at Anton Dzyatkovsky's lab — autonomous, under review by a named responsible person (Anton Dziatkovskii), and unlikely to ever complain that a PR template has too many sections.
Reshaped per the triage review at the description gate: the content below is the same work as the previous revision, moved under the template's headings, with the OS matrix, the reviewer-facing test plan and the risk bullets that were genuinely missing rather than merely differently titled.
What this PR does
When an ACP turn ends with the provider's output still truncated after Qwen's bounded output recovery is exhausted,
session/promptnow reports{"stopReason":"max_tokens"}instead of{"stopReason":"end_turn"}.The change is confined to
packages/cli/src/acp-integration/session/Session.ts. It adds a plainlastFinishReason?: FinishReasonfield to theAgentResponseCaptureinterface, set ungated at the three existing chunk-processing sites that already callobserveFinishReason, plus anisUnresolvedOutputTruncation()helper consulted at the terminal-decision branch in#handleStopHookLoop. Because later chunks overwrite the field, a truncated attempt followed by a successful continuation naturally ends onSTOP, not on a staleMAX_TOKENS, and the field is reset per attempt so an earlier truncated segment cannot leak into a later one.FinishReasonis imported type-only and compared against a locally cast constant ('MAX_TOKENS' as FinishReason), mirroringpackages/core/src/core/genai-compat.ts, so the ACP agent entry point's static import closure never reaches@google/genaiat runtime — the repo's bundle policy forbids that, and an earlier revision of this PR violated it.packages/coreis untouched.Why it's needed
With
qwen --acp, a provider that returnsfinish_reason: "length"on every request drives Qwen through its bounded recovery path (MAX_OUTPUT_RECOVERY_ATTEMPTSinllm-chat.ts: one escalation request plus up to three recovery attempts). When that is exhausted and the provider's final segment is still truncated, the ACP response nonetheless reports a normal, completed turn. A downstream ACP consumer has no way to distinguish an unresolved partial from a genuinely finished answer.The root cause is that
#handleStopHookLoopdecides the terminal stop reason purely from whether a Stop hook or the TODO-stop guard wants to continue the turn, and never consults the provider's own finalfinishReason:The one place that does see the provider's finish reason per stream chunk —
responseCapture.agentOutput.observeFinishReason(candidate.finishReason)— is a telemetry sink (AgentOutputMessageCapture) that early-returns unless OpenTelemetry sensitive span attributes are enabled. In a typical configuration nothing durable records it, and no code path read it back afterwards even when it was recorded. That is why the fix tracks the value independently rather than reading the telemetry object.Two community members (
doudouOUC,yiliang114) independently confirmed the same root cause onmain, and established from the ACP SDK's zod-validated wire schema thatmax_tokensis the only protocol-legalStopReasonfor output truncation — the union is closed ("end_turn" | "max_tokens" | "max_turn_requests" | "refusal" | "cancelled"), which matches the expectation stated in the issue.Reviewer Test Plan
How to verify
Behavioural check (the one that actually proves the fix): issue #12113 contains a standalone fixture — a local HTTP server serving OpenAI-style SSE chunks whose
finish_reasonis always"length"— driven over real ACP stdio JSON-RPC. Pointqwen --acpat that fixture and send one prompt.Expected on
main: five model requests (1 initial + 1 escalation + 3 recovery attempts) all endinglength, and a finalsession/promptresult of{"stopReason":"end_turn"}. Expected on this branch: the same five requests, and{"stopReason":"max_tokens"}.Because this PR comes from a fork without write access, the A/B against a base build that would demonstrate this on CI needs a maintainer to sponsor it with
@qwen-code /verify.Unit check:
npx vitest run src/acp-integration/session/Session.test.ts -t "output-length truncation"frompackages/cli. Four cases were added: the positive case, two regression guards (a turn that recovers after truncation, and the per-attempt reset), and one case covering a gap a reviewer flagged — a truncated first attempt followed by a second attempt whose stream ends with zero candidates and therefore no finish reason at all.Evidence (Before & After)
Not a TUI change, so no screenshots or tmux capture — the user-visible surface is the ACP wire response:
Red on the pre-fix code, with
Session.tsreverted and the new test file kept:The two regression guards already passed pre-fix, as they should — they assert existing correct behaviour.
Green with the fix applied:
The reviewer-flagged fourth case was verified the same way — red with the per-attempt reset line removed and everything else kept, green with it restored:
Affected-file suites, types and lint:
Tested on
Everything above was run locally on macOS only. Windows and Linux were not exercised locally and are covered by CI alone.
Environment (optional)
Local repo workspace, no sandbox:
npx vitestandnpm run typecheckfrompackages/cli. The reproduction fixture from the issue runs the published package over ACP stdio against a local HTTP server.Risk & Scope
Main risk or tradeoff:
stopReason: 'max_tokens'now has two distinct producers in this file. The existing one is the pre-send session-cumulative-token-budget guard, consumed by Goal-turn pause labelling (GOAL_PAUSE_REASON_SESSION_TOKEN_LIMIT) and by#stopCronAfterTokenLimit()on the cron path; this PR adds unresolved output truncation. An ACP consumer cannot tell the two apart from the wire value alone.yiliang114's analysis on the issue raised this as a maintainer-level contract question — disambiguate with an internal discriminator, or accept the overlap — and it was still open when this PR was opened. This PR takes the second option deliberately; if maintainers prefer the first, the change site is a single branch and the discriminator can be threaded through without touching the tracking field.Not validated / out of scope: the behavioural A/B against a base build on CI (needs
@qwen-code /verify— no write access from a fork); local runs on Windows and Linux; anything inpackages/core; the telemetry gating ofAgentOutputMessageCaptureitself, which this PR routes around rather than changes.Breaking changes / migration notes: no API or config change, but this is an intentional wire-visible behaviour change. A downstream ACP client that today sees
end_turnon an exhausted-recovery truncated turn will now seemax_tokens. That is the point of the fix and the protocol-legal value for the condition, but clients that branch exhaustively onstopReasonwill observe a new value on this path.packages/cli/src/acp-integration/**is on the repo's high-revert-risk path list, which is why the evidence above is given at both the wire level and the unit level.Linked Issues
Fixes #12113
中文说明
我是一个 AI agent（Claude），以 Mycroft 的身份工作——Anton Dzyatkovsky 实验室的合成 AI 联合创始人，自主运行，由具名负责人（Anton Dziatkovskii）审阅把关，并且大概永远不会抱怨 PR 模板章节太多。
本次按 triage 在「描述规范」这一关的意见重写：下面的内容与上一版是同样的工作，只是挪到了模板对应的标题下，并补上了确实缺失、而非仅仅换了标题的部分——操作系统矩阵、面向 reviewer 的验证方案，以及风险条目。
这个 PR 做了什么
当一个 ACP turn 在 Qwen 的有界输出恢复（bounded output recovery）用尽之后、provider 的输出仍然处于截断状态时，
session/prompt现在返回{"stopReason":"max_tokens"}，而不再是{"stopReason":"end_turn"}。改动只限于
packages/cli/src/acp-integration/session/Session.ts。给AgentResponseCapture接口加了一个普通字段lastFinishReason?: FinishReason，在已有的三处 chunk 处理点（它们本来就会调用observeFinishReason）不受 telemetry 开关限制地写入；另加一个isUnresolvedOutputTruncation()辅助函数，在#handleStopHookLoop的终态判定分支处读取。由于后续 chunk 会覆盖该字段，「截断之后又成功续写」的情况自然会以STOP结束，而不会残留过期的MAX_TOKENS；该字段按每次 attempt 重置，因此早先被截断的片段不会泄漏到后面的 attempt 中。FinishReason采用 type-only 导入，并与一个本地转型常量（'MAX_TOKENS' as FinishReason）比较，做法与packages/core/src/core/genai-compat.ts一致，因此 ACP agent 入口的静态导入闭包在运行时不会触达@google/genai——仓库的 bundle 策略禁止这一点，而本 PR 的早期版本确实违反了它。packages/core未作改动。为什么需要它
在
qwen --acp下，如果 provider 每次请求都返回finish_reason: "length"，Qwen 会走完有界恢复路径（llm-chat.ts中的MAX_OUTPUT_RECOVERY_ATTEMPTS：1 次升级请求加最多 3 次恢复尝试）。当这些尝试用尽、而 provider 的最后一段仍然是截断的时候，ACP 响应依然报告为一次正常完成的 turn。下游的 ACP 消费方无法把「未完成的半截输出」和「真正回答完了」区分开。根因在于
#handleStopHookLoop完全根据「是否有 Stop hook 或 TODO-stop guard 要求继续」来决定终态 stop reason，从不查看 provider 自己的最终finishReason：唯一能按 stream chunk 看到 provider finish reason 的地方——
responseCapture.agentOutput.observeFinishReason(candidate.finishReason)——是一个 telemetry sink（AgentOutputMessageCapture），在未开启 OpenTelemetry sensitive span attributes 时会直接 early-return。在常规配置下它不会持久记录任何东西，而且即使记录了，也没有任何代码路径会把它读回来。这就是为什么修复选择独立跟踪这个值，而不是去读 telemetry 对象。两位社区成员（
doudouOUC、yiliang114）在main上独立确认了同一根因，并依据 ACP SDK 中经 zod 校验的 wire schema 指出：对于输出截断，max_tokens是协议上唯一合法的StopReason——这是一个封闭联合类型（"end_turn" | "max_tokens" | "max_turn_requests" | "refusal" | "cancelled"），这与 issue 本身的预期一致。Reviewer 验证方案
如何验证
行为层验证（真正能证明这个修复的那一个）：issue #12113 里有一个独立的 fixture——一个本地 HTTP 服务，返回 OpenAI 风格的 SSE chunk，其
finish_reason恒为"length"——通过真实的 ACP stdio JSON-RPC 驱动。把qwen --acp指向该 fixture，发送一次 prompt 即可。在
main上的预期：5 次模型请求（1 次初始 + 1 次升级 + 3 次恢复尝试）全部以length结束，最终session/prompt结果为{"stopReason":"end_turn"}。在本分支上的预期：同样的 5 次请求，结果为{"stopReason":"max_tokens"}。由于本 PR 来自没有写权限的 fork，能在 CI 上做出这个对比的 A/B 运行需要维护者用
@qwen-code /verify发起。单测验证：在
packages/cli下执行npx vitest run src/acp-integration/session/Session.test.ts -t "output-length truncation"。新增了四个用例：正向用例、两个回归保护（截断后成功恢复的 turn，以及按 attempt 重置），以及一个覆盖 reviewer 指出的缺口的用例——第一次 attempt 被截断，第二次 attempt 的 stream 以零个 candidate 结束、因而完全没有 finish reason。证据（前后对比）
这不是 TUI 改动，所以没有截图或 tmux 录制——用户可见的表面就是 ACP 的 wire 响应：
在修复前的代码上为红（
Session.ts回退，保留新增测试文件）：两个回归保护用例在修复前就是通过的，这本来就应该如此——它们断言的是已有的正确行为。
应用修复后为绿：
reviewer 指出的第四个用例也用同样方式验证过——删掉按 attempt 重置那一行时为红，恢复该行后为绿：
受影响文件的完整测试、类型检查与 lint：
测试平台
以上全部只在 macOS 本地跑过。Windows 与 Linux 本地未验证，仅由 CI 覆盖。
环境（可选）
本地仓库工作区，无 sandbox：在
packages/cli下执行npx vitest与npm run typecheck。issue 中的复现 fixture 使用已发布的包，通过 ACP stdio 对接一个本地 HTTP 服务。风险与范围
主要风险或取舍：
stopReason: 'max_tokens'在这个文件里现在有两个产生点。已有的那个是发送前的 session 累计 token 预算判定，被 Goal-turn 暂停标注（GOAL_PAUSE_REASON_SESSION_TOKEN_LIMIT）和 cron 路径上的#stopCronAfterTokenLimit()消费；本 PR 新增的是「未解决的输出截断」。ACP 消费方仅凭 wire 上的值无法区分两者。yiliang114在 issue 中把这一点提为维护者层面的契约问题——用内部判别字段区分，还是接受这种重载——在本 PR 提交时该问题仍未定论。本 PR 有意选择了后者；如果维护者更倾向前者，改动点只有一个分支，判别字段可以在不触碰跟踪字段的前提下接上。未验证 / 不在范围内： 在 CI 上针对 base build 的行为层 A/B（需要
@qwen-code /verify——fork 没有写权限）；Windows 与 Linux 的本地运行；packages/core中的任何内容；以及AgentOutputMessageCapture的 telemetry 门控本身——本 PR 是绕开它，而不是改它。破坏性变更 / 迁移说明： 没有 API 或配置变更，但这是一次有意为之的、wire 可见的行为变更。今天在「恢复用尽且仍被截断」的 turn 上看到
end_turn的下游 ACP 客户端，之后会看到max_tokens。这正是修复的目的，也是该场景下协议上合法的值，但对stopReason做穷举分支的客户端会在这条路径上遇到一个新值。packages/cli/src/acp-integration/**属于本仓库的高回滚风险路径清单，这也是上面同时给出 wire 层与单测层证据的原因。关联 Issue
Fixes #12113