Skip to content

feat(serve): add pollable daemon turn status - #9080

Merged
BenGuanRan merged 15 commits into
QwenLM:mainfrom
BenGuanRan:feat/daemon-turn-status-polling-v2
Aug 18, 2026
Merged

feat(serve): add pollable daemon turn status#9080
BenGuanRan merged 15 commits into
QwenLM:mainfrom
BenGuanRan:feat/daemon-turn-status-polling-v2

Conversation

@BenGuanRan

@BenGuanRan BenGuanRan commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This PR adds an always-on session_turn_status capability and two read-only, live-Session routes: GET /session/:id/turns/current and GET /session/:id/turns/:promptId. Callers can poll idle, queued, running, completed, cancelled, or error without maintaining an SSE subscription.

The exact prompt route returns the raw final parent-model answer from the last tool-free response block. Text before a tool call, tool output, thought text, subagent updates, diagnostics, background output, and optional rewritten presentation are not reported as resultText. Results are bounded at 32,768 UTF-16 code units and expose resultTruncated: true plus RESULT_TEXT_TRUNCATED when that bound is reached.

Live state comes from the owning bridge. Recent terminals use a fixed 64-entry in-process overlay while settled turns are appended once, best-effort, by the Session recorder and read from a bounded active-transcript window. A successful rewind clears the overlay, failed rewind keeps it, and forks do not inherit source prompt identities.

Why it's needed

Automation clients such as AgentRun receive a promptId from non-blocking prompt admission but currently need to keep an SSE stream open to learn the final state and main answer. This makes short-lived or reconnecting callers unnecessarily complex. The polling surface lets them recover the result for a live Session while preserving workspace ownership and client authorization.

This PR intentionally supersedes #8682 instead of extending it. That PR validated the problem and several important correctness cases, but after many review rounds its diff grew into strict teardown persistence, crash/shutdown transcript backfill, rewind indexing, rewrite-pipeline changes, and repeated conflict resolution. Those changes exceeded the requested polling contract and made review convergence harder. This is a clean rebuild from the latest main: it retains the validated API, final-answer semantics, bounded live/persisted lookup, and critical race fixes, while explicitly leaving crash durability and permanent result storage out of scope.

Reviewer Test Plan

How to verify

  1. Start the built daemon without additional flags and confirm /capabilities contains session_turn_status.
  2. Create a live Session, submit a non-blocking prompt, and poll its returned promptId; expect queued/running while live and a settled terminal afterward.
  3. Use a model response that emits visible text plus a tool call, then a final answer; expect resultText to contain only the answer after the tool boundary.
  4. Return more than 32,768 UTF-16 code units; expect the bounded prefix, resultTruncated: true, and resultCode: "RESULT_TEXT_TRUNCATED".
  5. Query an unknown promptId; expect 404 prompt_not_found. Confirm this is a bounded not-found result, not proof that the prompt never existed.
  6. Rewind a Session successfully and confirm discarded overlay results are not returned; force rewind failure and confirm the overlay is retained.
  7. Confirm a fork cannot query the source Session's promptIds and that a Session owned by another workspace runtime is never resolved through the primary runtime.

Evidence (Before & After)

Before: the installed global qwen 0.18.5 does not advertise session_turn_status and has no turn polling route.

After: a built daemon backed by the repository's fake OpenAI server passed capability discovery, the tool-boundary final-answer E2E, and the 32,768-code-unit truncation E2E. Full changed-file suites passed: Session 611/611, ACP agent 400/400, ACP bridge 593/593, core recording/session service 215/215, conversation branches 21/21, and serve server 938/938. Repository build, bundle, lint, and workspace typecheck also passed.

Tested on

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

Environment (optional)

macOS, Node.js 22-compatible repository toolchain, no sandbox, real qwen serve process with the local fake OpenAI-compatible server.

Risk & Scope

  • Main risk or tradeoff: settled lookup is deliberately bounded and best-effort. The live bridge overlay holds 64 terminals; transcript lookup scans at most 10 backward pages of 500 active records.
  • Deadline consistency boundary: the 64-entry live overlay's caller-facing deadline terminal is authoritative until the child settles. Once the child has persisted a non-error execution outcome for the same promptId, the poll surface returns that persisted outcome instead of the deadline error (the exactly-once turn_error event was already delivered at deadline time), so polls never emit an error state enriched with a successful resultText. This PR intentionally does not add durable terminal reconciliation or a permanent task-result ledger.
  • Not validated / out of scope: no permanent or exactly-once result store; no daemon-side transcript writer, strict close/kill barrier, crash/shutdown backfill, offline Session lookup, deleted-JSONL recovery, or message-rewrite refactor. Restart lookup requires recording to be enabled, the append to succeed, the result to remain active and inside the bounded window, and the Session to be loaded live again.
  • Breaking changes / migration notes: none. The capability and routes are additive and require no configuration.

Linked Issues

Related to #8680

Supersedes #8682

中文说明

本 PR 做了什么

本 PR 新增默认开启的 session_turn_status 能力点,以及两个只读、仅面向存活 Session 的接口:GET /session/:id/turns/currentGET /session/:id/turns/:promptId。调用方无需保持 SSE 订阅,即可轮询 idlequeuedrunningcompletedcancellederror 状态。

精确 prompt 接口返回父模型最后一个不含工具调用的响应块中的原始最终回答。工具调用前的文本、工具输出、思考文本、subagent 更新、诊断信息、后台输出以及可选的改写展示文本都不会进入 resultText。结果上限为 32,768 个 UTF-16 code units,达到上限时返回 resultTruncated: trueRESULT_TEXT_TRUNCATED

实时状态来自 Session 所属的 bridge。最近的终态使用固定 64 条的进程内 overlay;已结束的 turn 由 Session recorder 单次、best-effort 写入,并从有界的 active transcript 窗口读取。rewind 成功后清空 overlay,rewind 失败时保留;fork 不会继承源 Session 的 prompt 身份。

为什么需要

AgentRun 等自动化调用方从非阻塞 prompt admission 获得 promptId 后,目前必须持续保持 SSE 才能获知最终状态和主回答,这让短生命周期或需要重连的调用方承担了不必要的复杂度。新增轮询接口让它们可以在 live Session 范围内恢复结果,同时保持工作空间归属与 client 授权边界。

本 PR 有意替代 #8682,而不是继续扩展它。#8682 已验证问题和多项重要正确性场景,但经过多轮评审后,其 diff 逐步扩展到了严格 teardown 持久化、crash/shutdown transcript 回填、rewind 索引、rewrite pipeline 改造以及反复的冲突处理。这些内容已经超出本次轮询契约,并提高了评审收敛难度。因此本 PR 基于最新 main 重新最小实现:保留已验证的 API、最终回答语义、有界实时/持久化查询和关键竞态修复,同时明确不处理 crash durability 和永久结果存储。

Reviewer 测试计划

如何验证

  1. 不增加任何参数启动构建后的 daemon,确认 /capabilities 包含 session_turn_status
  2. 创建 live Session,提交非阻塞 prompt,使用返回的 promptId 轮询;执行期间应看到 queued/running,结束后应看到终态。
  3. 让模型先输出可见文本并调用工具,再输出最终回答;resultText 应只包含工具边界之后的回答。
  4. 返回超过 32,768 个 UTF-16 code units;应获得有界前缀、resultTruncated: trueresultCode: "RESULT_TEXT_TRUNCATED"
  5. 查询未知 promptId;应返回 404 prompt_not_found。该结果仅表示在有界范围内未找到,并不证明 prompt 从未存在。
  6. 成功 rewind Session 后,已丢弃的 overlay 结果不应再返回;模拟 rewind 失败时,overlay 应保留。
  7. 确认 fork 无法查询源 Session 的 promptId,并确认属于其他 workspace runtime 的 Session 不会回退到 primary runtime 查询。

证据(Before & After)

Before:本机全局安装的 qwen 0.18.5 不会发布 session_turn_status,也没有 turn 轮询接口。

After:构建后的真实 daemon 使用仓库内 fake OpenAI server,已通过能力点发现、“工具边界后最终回答”E2E 和“32,768 code units 截断状态”E2E。完整变更文件测试均通过:Session 611/611、ACP agent 400/400、ACP bridge 593/593、core recording/session service 215/215、conversation branches 21/21、serve server 938/938。仓库 build、bundle、lint 和 workspace typecheck 也全部通过。

测试系统

OS 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

macOS,兼容 Node.js 22 的仓库工具链,无 sandbox,真实 qwen serve 进程配合本地 fake OpenAI-compatible server。

风险与范围

  • 主要风险或取舍:settled 查询有意设计为有界且 best-effort。live bridge overlay 保存 64 条终态;transcript 查询最多向后扫描 10 页、每页 500 条 active records。
  • Deadline 一致性边界:在 child 落盘之前,64 条 live overlay 中面向调用方的 deadline 终态是权威结果。一旦 child 为同一 promptId 落盘了非 error 的执行结果,轮询面即返回该 persisted 结果而不是 deadline error(exactly-once 的 turn_error 事件已在 deadline 时发出),因此轮询永远不会输出带成功 resultText 的 error 状态。本 PR 有意不引入 durable terminal reconciliation 或永久任务结果账本。
  • 未验证 / 不在范围内:不提供永久或 exactly-once 结果存储;不新增 daemon transcript writer、严格 close/kill barrier、crash/shutdown 回填、offline Session 查询、JSONL 删除恢复或 message rewrite 重构。重启后查询要求 recording 已开启、append 成功、结果仍在 active branch 且位于有界窗口内,并且 Session 已重新加载为 live。
  • 破坏性变更 / 迁移说明:无。能力点与接口都是新增项,无需配置。

关联 Issue

Related to #8680

Supersedes #8682

Add GET /session/:id/turns/current and GET /session/:id/turns/:promptId
so external callers can poll a turn's lifecycle state (queued / running /
completed / cancelled / error) and result instead of holding the SSE
stream for the whole turn lifetime.

- Live state comes from the bridge's pending prompt queue; settled
  outcomes from persisted turn_result transcript records, so results
  survive daemon restarts and the daemon keeps no per-turn memory
- Each prompt captures its own recording and settles exactly that one,
  so overlapping turns (DAEMON-003 deadline overlap) can never
  misattribute one turn's outcome to another promptId
- Enforces the same client authorization as POST /session/:id/prompt

Refs QwenLM#8680
@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

E2E test report

Tested the final bundled CLI on macOS with QWEN_SANDBOX=false, a real qwen serve child process, and the repository's local fake OpenAI-compatible server.

  • Capability discovery: session_turn_status is advertised without a flag or setting.
  • Final-answer contract: a model response containing visible pre-tool text plus read_file, followed by a final model response, completed with resultText equal to only The strict final answer is 42..
  • Result bound: a response longer than 32,768 UTF-16 code units completed with a 32,768-code-unit resultText, resultTruncated: true, and resultCode: RESULT_TEXT_TRUNCATED.
  • Focused integration result: 2 passed, 0 failed.

Additional verification on the final source:

  • Session: 611 passed.
  • ACP agent: 400 passed.
  • ACP bridge: 593 passed.
  • Core recording/session service: 215 passed.
  • Conversation branches: 21 passed.
  • Serve server: 938 passed.
  • Build, bundle, lint, workspace typecheck, and git diff --check: passed.

Known boundary: this report does not claim crash/shutdown backfill, deleted-JSONL recovery, offline Session lookup, or permanent result retention. Those are explicitly outside this PR.

@github-actions github-actions Bot added the review/self-reported The linked issue was opened by the PR author (self-reported) label Aug 13, 2026
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — and for the disciplined scope reduction compared to PR 8682.

Template looks good ✓

Problem: real and grounded. Since POST /session/:id/prompt became non-blocking (202 + promptId, PR 4585), callers have had no way to learn a turn's terminal state and final answer without holding the SSE stream open. Linked issue #8680 (already triaged, labeled roadmap/background-automation) documents the gap with a concrete response contract, and superseded PR 8682 validated it through many review rounds. This is an observed API-surface gap, not theoretical hardening.

Direction: aligned. Issue #8680 was accepted for exploration in triage; a polling surface is the direct complement of the merged non-blocking admission — read-only and additive, no behavior change for existing clients. CHANGELOG has no direct turn-status precedent, but the background-automation direction is active and this fits it.

Size: core paths are touched (core services/utils + cli serve/acp-integration + acp-bridge, cross-package). Roughly 1,076 production lines vs ~1,931 test lines vs ~46 doc lines. That crosses both the 500-line maintainer-awareness bar and the 1,000-line large-PR advisory, so this is flagged for maintainer attention. Mitigating context: this is the deliberately minimal rebuild after PR 8682 grew to ~7,900 lines, with crash durability and permanent result storage explicitly cut. Splitting further doesn't look natural — the Session settle hooks, bridge overlay, and routes form one contract.

Approach: matches what I'd propose independently — capability flag, two GET routes scoped to the live owning runtime with the same client-id auth as /prompt, live queue plus a bounded 64-entry terminal overlay in the bridge, best-effort bounded backward transcript scan for settled turns, and final answer = last tool-free parent-model response block capped at 32,768 UTF-16 code units. Two housekeeping points: (1) PR 8682 is still open — it should be closed in favor of this one; (2) the getPendingPrompts filter gains a !terminalPublished condition, a small behavior change to an existing route — it looks consistent with "pending" semantics and is detailed in the code review.

Risk: Stage 1e matches the acp-integration paths (acpAgent.ts, session/Session.ts), which this repo's revert history flags as elevated-risk. Not a blocker, but review depth is full, CI evidence is required before approval, and a sandboxed lane should be named before merge.

Moving on to code review. 🔍

中文说明

感谢贡献——也感谢相比 PR 8682 所做的严格的范围收敛。

模板完整 ✓

问题:真实且有依据。自 POST /session/:id/prompt 改为非阻塞(202 + promptId,PR 4585)之后,调用方若不保持 SSE 长连接就无法获知轮次终态与最终回答。关联 issue #8680(已完成 triage,带 roadmap/background-automation 标签)给出了具体的响应契约,被替代的 PR 8682 也经过多轮评审验证了该问题。这是已观测到的 API 面缺口,而非理论性加固。

方向:对齐。issue #8680 在 triage 中已"接受探索";轮询接口是已合入的非阻塞 admission 的直接补充——只读、增量,不改变现有客户端行为。CHANGELOG 没有完全对应的先例,但 background-automation 方向是活跃的,本 PR 契合该方向。

规模:触及核心路径(core services/utils + cli serve/acp-integration + acp-bridge,跨包)。约 1,076 生产行,对比约 1,931 测试行、约 46 文档行。同时越过 500 行"维护者关注"线与 1,000 行大 PR 建议线,因此标记请维护者关注。缓解背景:本 PR 是在 PR 8682 膨胀到约 7,900 行之后刻意做的最小重建,crash durability 与永久结果存储已被明确砍掉。进一步拆分看起来不自然——Session settle 钩子、bridge overlay 与路由共同构成一个契约。

方案:与我独立的设想一致——能力点、两个仅面向 live owning runtime 且复用 /prompt client-id 鉴权的 GET 路由、bridge 内实时队列 + 有界 64 条终态 overlay、对已落盘终态做有界的 transcript 回扫,以及"最后一个不含工具调用的父模型响应块"作为最终回答、上限 32,768 个 UTF-16 code units。两个事务性提醒:(1) PR 8682 仍处于 open 状态——应关闭它以让位给本 PR;(2) getPendingPrompts 的过滤条件新增了 !terminalPublished,属于对现有路由的小行为变更——看起来与 "pending" 语义一致,详见代码审查。

风险:Stage 1e 命中 acp-integration 路径(acpAgent.tssession/Session.ts),在本仓库的 revert 历史中属于较高风险区。不构成阻塞,但 review 深度为全量、批准前需要 CI 证据,且合入前应指定沙箱验证通道。

进入代码审查 🔍

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Code review

I formed an independent proposal before reading the diff (capability flag, two live-runtime-scoped GET routes, live queue + bounded terminal overlay, bounded transcript scan for settled turns, last-tool-free-block final answer). The PR matches it, and goes further than I would have on the hard parts: the bridge re-reads the overlay after every awaited child read — including failed ones — so a terminal published mid-lookup can never regress to queued or 404, and terminal publication is first-writer-wins. Those races have dedicated tests, not just assertions in prose.

No critical blockers found. What I verified against the surrounding code:

  • Route registration order is correct (/turns/current before /turns/:promptId), resolution goes through the live owning runtime with no primary-runtime fallback, and client-id authorization reuses the same resolveTrustedClientId path as /prompt.
  • The backward transcript scan reads pages newest-first and iterates each page from the tail, so the first turn_result match really is the newest; structured transcript errors (invalid cursor, snapshot unavailable, oversized page/snapshot) stay structured instead of collapsing into prompt_not_found.
  • Session is the only transcript writer; recordTurnResult uses the non-strict append path and never perturbs the turn lifecycle. normalizeTurnResultError reads message/code through guarded property access, so hostile getters can't throw during settlement.
  • The riskiest hunk is the Session.prompt() restructure needed to settle the recording on every exit path. I walked old vs new path by path: semantics are preserved (cleanup errors still override a successful result, releasePendingSend call sites unchanged), and the new settle hooks cover validation failures, admission cancellation, goal-reservation failures, thrown errors, and user-cancel classification.
  • Forks exclude turn_result records, so a forked Session can't inherit source prompt identities; turn_result joins NEUTRAL_TAIL_SUBTYPES so tail records don't skew branch classification.

Two observations worth a maintainer's eye, neither blocking:

  • Drive-by on an existing route: getPendingPrompts now also filters out entries whose terminal was already published. That looks consistent with "pending" semantics (and matches the new liveTurnStatus filter), but it does change what webui/web-shell queue reconciliation sees. It's covered indirectly by the existing bridge tests, just calling it out explicitly.
  • Non-blocking notes: the 6-line truncateTurnText helper is duplicated in bridge and Session (acceptable at this size); no SDK client helper yet — integration tests use raw fetch, which is fine for now; truncation slices at UTF-16 code units, so an astral character straddling the 32,768 boundary splits — documented contract, cosmetic.

The flow being added, for reviewers navigating the diff:

sequenceDiagram
    participant P1 as Client
    participant P2 as serve route
    participant P3 as Bridge
    participant P4 as ACP child
    participant P5 as Transcript JSONL
    P1->>P2: GET turns by promptId or current
    P2->>P3: getSessionTurnStatus (client-id auth)
    P3->>P3: check live queue and 64-entry terminal overlay
    alt not resolved live
        P3->>P4: ext sessionTurnStatus
        P4->>P5: bounded backward scan (10 pages of 500)
        P5-->>P4: turn_result record or null
        P4-->>P3: payload
    end
    P3->>P3: re-check overlay, concurrent terminal wins
    P3-->>P2: status or not found
    P2-->>P1: 200 status or 404 prompt_not_found
Loading
Files changed (25 of 25 shown)
File What changed
docs/design/daemon-turn-status-endpoint.md New design doc: scope, result semantics, live vs persisted sources, non-goals
docs/developers/qwen-serve-protocol.md Protocol doc paragraph for the two polling routes
integration-tests/cli/qwen-serve-routes.test.ts Capability envelope now expects session_turn_status
integration-tests/cli/qwen-serve-streaming.test.ts Real-daemon E2Es: tool-boundary final answer and 32,768 truncation
packages/acp-bridge/src/bridge.test.ts 733 lines covering overlay, races, queued/running, rewind, auth
packages/acp-bridge/src/bridge.ts getSessionTurnStatus, terminal overlay (64), startedAt, rewind clears overlay
packages/acp-bridge/src/bridgeTypes.ts BridgeTurnStatus type, startedAt on PendingPromptEntry, bridge method
packages/acp-bridge/src/status.ts New ext method name qwen/control/session/turn_status
packages/cli/src/acp-integration/acpAgent.test.ts Tests for the child-side ext handler and bounded scan
packages/cli/src/acp-integration/acpAgent.ts Ext handler: flush recorder, bounded backward scan, structured errors
packages/cli/src/acp-integration/session/Session.test.ts 575 lines on turn_result recording across settle paths
packages/cli/src/acp-integration/session/Session.ts Turn recording hooks, final-answer capture reuse, prompt() settle restructure
packages/cli/src/serve/acp-session-bridge.ts Re-exports BridgeTurnStatus
packages/cli/src/serve/capabilities.ts Registers session_turn_status capability
packages/cli/src/serve/routes/session.ts The two GET routes: resolution, auth, 404 prompt_not_found
packages/cli/src/serve/server.test.ts Route tests: 200 current, 200 by id, 404 unknown prompt/session, 400 bad client
packages/cli/src/serve/server/telemetry-catalog.test.ts Drift guard count 54 to 56
packages/cli/src/serve/server/telemetry.ts Registers both routes in the telemetry catalog
packages/core/src/services/chatRecordingService.test.ts recordTurnResult, hostile error normalization, payload validation
packages/core/src/services/chatRecordingService.ts turn_result subtype, bounded payload type, validator, best-effort append
packages/core/src/services/sessionService.test.ts Fork must not copy source turn_result identities
packages/core/src/services/sessionService.ts Fork filter drops turn_result records
packages/core/src/utils/conversation-branches.test.ts turn_result treated as neutral tail
packages/core/src/utils/conversation-branches.ts Adds turn_result to NEUTRAL_TAIL_SUBTYPES
packages/core/src/utils/transcript-records.ts Registers turn_result as a known subtype

Test evidence

The PR's own CI at the reviewed commit, fetched via API (per the static-review rule I did not build or run any PR code; the live-behavior lane is the sandboxed trigger below). Real daemon E2E / Java 11 passing is the meaningful early signal — it exercises a real daemon harness. The Node unit suite and the Serve A/B integration run (which is where the new qwen-serve-streaming E2Es execute) are still in progress; the finalize workflow updates the table below in place once CI settles. macOS/Windows unit jobs are skipped by the matrix on this run — noted as-is, not a finding.

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

Check Conclusion
Test (ubuntu-latest, Node 22.x) ❌ failure
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
macos-latest / Java 21 ✅ success
Real daemon E2E / Java 11 ✅ success
Serve A/B (ubuntu-latest, Node 22.x) ✅ success
ubuntu-latest / Java 11 ✅ 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,失败项排在最前。

The E2E report in this thread (capability discovery, tool-boundary final answer, truncation bound) is the author's self-reported result on macOS — attributed as a claim, not re-run here.

Sandboxed verification would settle this: @qwen-code /verify — the central claims are behavioural (resultText returns only the post-tool-boundary parent answer, the 32,768 truncation bound and RESULT_TEXT_TRUNCATED code hold end-to-end, overlay/rewind semantics survive a real daemon), and while the PR's own suite covers these against a fake OpenAI server, a suite can pass with the very code it was written next to; an A/B run against the base build would prove the new routes actually pin the behaviour. The author has write access, so a maintainer (or the author) can trigger it directly on this head.

中文说明

代码审查

读 diff 之前我先独立写了方案(能力点、两个仅限 live runtime 的 GET 路由、实时队列 + 有界终态 overlay、有界 transcript 回扫、"最后一个无工具调用响应块"作为最终回答)。PR 与之吻合,而且在最难的地方做得更细:bridge 在每次 await 子进程读取之后(包括读取失败时)重新检查 overlay,因此 lookup 中途出现的终态不会回退成 queued404;终态发布是 first-writer-wins。这些竞态都有专门的测试。

未发现关键阻塞项。对照周边代码核实过的点:

  • 路由注册顺序正确(/turns/current 先于 /turns/:promptId),解析走 live owning runtime、不回退 primary runtime,client-id 鉴权复用 /prompt 的 resolveTrustedClientId
  • transcript 回扫按页从新到旧、页内从尾向头迭代,首个匹配的 turn_result 确实是最新的;结构化 transcript 错误(非法 cursor、快照不可用、页/快照超限)保持结构化,不会塌缩成 prompt_not_found
  • Session 是唯一的 transcript 写入者;recordTurnResult 走非严格追加路径,绝不影响轮次生命周期。normalizeTurnResultError 用受保护的属性读取,恶意 getter 不会在 settle 时抛错。
  • 风险最高的是 Session.prompt() 为在所有退出路径上 settle 记录而做的重构。逐路径对比新旧语义:保持一致(清理错误仍然会覆盖成功结果,releasePendingSend 调用点不变),新的 settle 钩子覆盖校验失败、admission 取消、goal 预留失败、抛错与用户取消分类。
  • fork 排除 turn_result 记录,fork 出的 Session 不会继承源 prompt 身份;turn_result 加入 NEUTRAL_TAIL_SUBTYPES,尾部记录不影响分支分类。

两点提请维护者留意,均不阻塞:

  • 对现有路由的顺手变更getPendingPrompts 现在还会过滤掉终态已发布的条目。与 "pending" 语义一致(也与新的 liveTurnStatus 过滤一致),但确实改变了 webui/web-shell 队列对账看到的内容。现有 bridge 测试间接覆盖,此处显式指出。
  • 非阻塞备注:6 行的 truncateTurnText 在 bridge 与 Session 各有一份(此规模可接受);SDK 暂无对应客户端方法——集成测试直接用 fetch,当前可接受;截断按 UTF-16 code units 切片,恰好跨 32,768 边界的 astral 字符会被切开——契约已注明,属外观问题。

新增流程(对应英文版时序图):客户端 → serve 路由 → bridge:先查实时队列与 64 条终态 overlay;未命中则经 ext 方法调 ACP 子进程做有界回扫(10 页、每页 500 条);读取返回后重查 overlay(并发终态优先),最后返回状态或 404。

测试证据

以上为被审提交在 CI 上的真实状态(API 拉取;按静态审查规则未构建/运行任何 PR 代码)。Real daemon E2E / Java 11 通过是早期最有意义的信号。Node 单测套件与 Serve A/B 集成(新 E2E 所在)仍在进行,finalize 工作流会在 CI 结束后就地更新表格。macOS/Windows 单测本次被矩阵跳过,如实记录、不作为发现项。

线程中的 E2E 报告(能力点发现、工具边界最终回答、截断上限)是作者自述的 macOS 结果——作为声明引用,未在此复核。

沙箱验证可以定案:@qwen-code /verify —— 核心主张是行为性的(resultText 只返回工具边界之后的父模型回答、32,768 截断与 RESULT_TEXT_TRUNCATED 端到端成立、overlay/rewind 语义在真实 daemon 上成立);PR 自带测试基于 fake OpenAI server,而测试与代码同批编写可能同错,对 base 构建的 A/B 运行才能证明新路由真正钉住了行为。作者有写权限,维护者(或作者)可在当前 head 上直接触发。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 3/5 — clean review and unusually thorough tests, but this is a 1,000+ production-line feature across core session/recording internals, so the Stage 0 maintainer-awareness escalation caps auto-approval; it needs a human sign-off, not bot doubt.

Stepping back: this is the version of PR 8682 that should have landed. The rebuild restored scope discipline — 7.9k lines down to 3k, explicit non-goals instead of teardown persistence and crash backfill creeping in — and the result is a contract a reviewer can actually hold in their head. The approach matches my independent proposal and exceeds it exactly where daemon code usually hurts: the races between polling, settlement, and transcript visibility are handled deliberately and tested individually. If I were maintaining this in six months I'd thank the author, not curse them — the design doc, the bounded everything, and the first-writer-wins publication are the kind of care that ages well.

Why not approve, then:

  • Policy cap, not findings: ~1,076 production lines across packages/core services, cli acp-integration/serve, and acp-bridge puts this past the maintainer-awareness bar, so the call belongs to a maintainer regardless of how clean the review reads.
  • CI is not settled yet: the Node unit suite and the Serve A/B integration run (where the new serve E2Es execute) are still in progress at the reviewed commit; Real daemon E2E and precheck are green so far.
  • Elevated-risk paths: acp-integration (acpAgent.ts, session/Session.ts) is correlated with post-merge reverts in this repo's history, and the Session.prompt() settle restructure — faithful as it is — concentrates that risk.

⏸️ Deferring to @wenshao — the review itself found no blockers (Stage 2), but a core-surface feature of this size warrants a maintainer's sign-off on the contract before merge. Two housekeeping asks for @BenGuanRan: close PR 8682 in favor of this one, and consider running @qwen-code /verify on this head to settle the behavioural claims (final-answer boundary, truncation, overlay/rewind) with A/B evidence. Once CI is green and a maintainer has weighed in, this looks ready.

中文说明

置信度:3/5 —— review 干净、测试异常充分,但这是一个横跨 core session/recording 内部、1,000+ 生产行的 feature,Stage 0 的"维护者关注"升级决定了不能自动批准;这是流程要求,而非 review 存疑。

整体来看:这才是 PR 8682 本该落地的形态。重建恢复了范围纪律——从 7.9k 行收敛到 3k 行,用明确的 non-goals 取代了逐步膨胀的 teardown 持久化与 crash 回填——最终契约是评审者能完整把握的。方案与我的独立设想一致,并且恰好在 daemon 代码最容易出问题的地方做得更好:轮询、settle 与 transcript 可见性之间的竞态被刻意处理并逐一测试。半年后维护这段代码,只会感谢作者——设计文档、处处有界、first-writer-wins 发布,都是经得起时间的细致。

为什么不直接批准:

  • 流程上限,而非发现项:约 1,076 生产行横跨 packages/core services、cli acp-integration/serveacp-bridge,越过维护者关注线,合入与否应由 maintainer 决定。
  • CI 尚未收敛:被审提交上 Node 单测套件与 Serve A/B 集成(新 serve E2E 所在)仍在运行;Real daemon E2E 与 precheck 已通过。
  • 高风险路径acp-integrationacpAgent.tssession/Session.ts)与本仓库合入后 revert 的历史相关;Session.prompt() 的 settle 重构虽然语义忠实,仍是风险集中点。

⏸️ 转交 @wenshao —— review 本身未发现阻塞项(见 Stage 2),但此规模的核心面 feature 在合入前应有 maintainer 对契约的确认。请 @BenGuanRan 处理两件事务:关闭 PR 8682 让位给本 PR;考虑在当前 head 上运行 @qwen-code /verify,用 A/B 证据定案行为性主张(最终回答边界、截断、overlay/rewind)。CI 转绿且维护者确认后,本 PR 看起来可以合入。

Qwen Code · qwen3.8-max

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

@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

🩺 serve daemon A/B

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

capabilities

field PR base (before) this PR (after)
features[] "session_turn_status"

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.

Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite (integration-tests/cli, incl. the new turn-status E2Es) did not run locally.

Not explored to full depth (tool budget reached): "This PR adds pollable daemon turn-status routes to qwen…": none — all checks above completed within budget.; "This PR adds pollable daemon turn-status routes to qwen…": none — all checks above completed within budget.; "This PR adds pollable daemon turn-status routes to qwen…": did not benchmark the record-count at which finding 1's collapse becomes reachable in a real session transcript (mechanism verified by code trace only).; "This PR adds pollable daemon turn-status routes to qwen…": I did not benchmark cold index build time against the 10s budget on a real transcript (no node_modules in the worktree / out of budget) — this is why Finding 1 …; "This PR adds pollable daemon turn-status routes to qwen…": did not benchmark cold buildIndex wall-time against the 10s budget on a real large transcript (worktree has no installed deps) — this is why the finding below…, and 21 more.

中文说明

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

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite (integration-tests/cli, incl. the new turn-status E2Es) did not run locally。

未探索到全部深度(达到工具调用预算):"This PR adds pollable daemon turn-status routes to qwen…"none — all checks above completed within budget."This PR adds pollable daemon turn-status routes to qwen…"none — all checks above completed within budget."This PR adds pollable daemon turn-status routes to qwen…"did not benchmark the record-count at which finding 1's collapse becomes reachable in a real session transcript (mechanism verified by code trace only)."This PR adds pollable daemon turn-status routes to qwen…"I did not benchmark cold index build time against the 10s budget on a real transcript (no node_modules in the worktree / out of budget) — this is why Finding 1 …"This PR adds pollable daemon turn-status routes to qwen…"did not benchmark cold buildIndex wall-time against the 10s budget on a real large transcript (worktree has no installed deps) — this is why the finding below…,另有 21 条。

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

Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/acp-bridge/src/bridge.ts Outdated
Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
Comment thread packages/acp-bridge/src/bridge.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
Comment thread packages/acp-bridge/src/bridge.test.ts
Comment thread packages/acp-bridge/src/bridge.test.ts
Comment thread packages/cli/src/acp-integration/acpAgent.ts
Comment thread packages/cli/src/acp-integration/acpAgent.test.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
@wenshao

wenshao commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /resolve

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

Qwen Code resolved the merge conflicts and pushed the branch update.

Merge resolution for PR #9080

Root cause

Main's c0e649b53cperf(serve): Restore large sessions selectively (#9055) — rewrote the replay path in acpAgent.ts, deleting the bulk-replay helpers (relocated to session/history-replay-page.ts) and their core imports findBoundaryAtOrBefore / isReplayTurnStartType. This PR inserted isTurnResultRecordPayload between those same two import lines for its findSettledTurnResult scanner. One conflict, in that import list; everything else auto-merged.

Textual or semantic

Semantic in the import list: both sides edited the same four lines with opposing intent (main deleted two lines the PR kept as context; the PR added a line between them). Resolution — keep the PR's import, drop the two whose only users #9055 deleted:

  encodeSessionTranscriptCursor,
  isTurnResultRecordPayload,
  subagentGenerator,

Dropping the pair is mandatory: no usage of either remains in the merged file, so keeping them would fail noUnusedLocals. isTurnResultRecordPayload is still used in findSettledTurnResult (~L441) and stays exported via export * from chatRecordingService.js in the core index.

What is load-bearing

  • The merged acpAgent.ts is the exact union of both sides: vs origin/main it diffs by precisely the PR's +137/−0 (the import, type TurnResultRecordPayload, findSettledTurnResult, and the SERVE_CONTROL_EXT_METHODS.sessionTurnStatus case); vs the PR head it contains only perf(serve): Restore large sessions selectively #9055's deletions. Any future edit that re-adds the replay-helper imports or removes the core export breaks this file.
  • Turn-status wiring order: bridge getSessionTurnStatus checks live/terminal status before and after the ACP ext call sessionTurnStatus, which scans persisted transcript pages. The GET /session/:id/turns/current route must stay registered before turns/:promptId (comment in routes/session.ts).
  • Telemetry catalog counts: legacySessionTelemetryRoutes must contain both PR turns routes — merged tree has 56 routes, 49 handler_resolved / 7 pre_resolved, matching the test the PR updated. Main never touched telemetry.ts since the PR base, so the counts survive.

What I could not verify

No build, typecheck, or tests were run here. Both #9055 and this PR modify Session.ts, bridge.ts, and chatRecordingService.ts; those auto-merged cleanly and I spot-checked the turn-status call chain, but the runtime interaction between #9055's selective-restore lease gating and this PR's transcript scan is only provable by the PR's CI. Only the one conflicted file was edited; all other changes are git's auto-merge.

中文说明

冲突根因:main 上的 c0e649b53c#9055 大会话选择性恢复)重写了 acpAgent.ts 的回放路径,删除了批量回放辅助函数及其仅有的两个 core 导入 findBoundaryAtOrBefore / isReplayTurnStartType;而本 PR 恰好在这两行导入之间插入了新导入 isTurnResultRecordPayload,导致同一导入列表冲突。

语义冲突及解决:保留 PR 的新导入(findSettledTurnResult 仍在使用,core 仍通过 export * 导出),删除 main 已移除使用方的两个导入——保留它们会因未使用而无法通过编译。

关键点:合并后的 acpAgent.ts 是两侧的精确并集(相对 main 恰为 PR 的 +137 行,相对 PR 头部仅含 #9055 的删除)。turns/current 路由必须先于 turns/:promptId 注册;遥测目录计数为 56 条路由(49/7 归属拆分),与 PR 更新后的测试一致。

未能验证:本次未运行构建或测试。#9055 与本 PR 同时修改了 Session.tsbridge.tschatRecordingService.ts,均自动合并且抽查了调用链,但选择性恢复的租约门控与本 PR 转录扫描之间的运行时交互需由 PR 自身 CI 证明。除冲突文件外未改动任何其他文件。

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

Not explored to full depth (tool budget reached): "PR 9080 reverse audit round 5 (cap round): hunt only gaps…": none — finished within budget, no check left incomplete.; "PR 9080 reverse audit round 5 (cap round): hunt only gaps…": none — all checks above completed within budget.; "PR 9080 reverse audit round 5 (cap round): hunt only gaps…": none — finished within budget.; "Second-round reverse audit of PR 9080 (pollable daemon…": none — all checks above completed within budget.; "PR 9080 reverse audit round 3: hunt only gaps all prior…": did not quantify real-world throw rates of #settleGoalTurn / releaseTurn / refreshSystemInstruction beyond confirming they are unguarded awaits with throwing …, and 19 more.

中文说明

未探索到全部深度(达到工具调用预算):"PR 9080 reverse audit round 5 (cap round): hunt only gaps…"none — finished within budget, no check left incomplete."PR 9080 reverse audit round 5 (cap round): hunt only gaps…"none — all checks above completed within budget."PR 9080 reverse audit round 5 (cap round): hunt only gaps…"none — finished within budget."Second-round reverse audit of PR 9080 (pollable daemon…"none — all checks above completed within budget."PR 9080 reverse audit round 3: hunt only gaps all prior…"did not quantify real-world throw rates of #settleGoalTurn / releaseTurn / refreshSystemInstruction beyond confirming they are unguarded awaits with throwing …,另有 19 条。

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

Comment thread packages/acp-bridge/src/bridge.ts
Comment thread packages/acp-bridge/src/bridge.ts Outdated
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread packages/cli/src/acp-integration/session/Session.ts Outdated
Comment thread packages/cli/src/acp-integration/session/Session.ts
Comment thread docs/design/daemon-turn-status-endpoint.md
Comment thread packages/acp-bridge/src/bridge.test.ts Outdated
Comment thread packages/acp-bridge/src/bridge.test.ts
Comment thread packages/cli/src/acp-integration/session/Session.test.ts
Comment thread packages/acp-bridge/src/bridge.ts
@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

Updated this existing PR to bbf24434c3, based on current main 8517fa9d47. This carries the verified review fixes while keeping the bounded polling contract; it does not add permanent result storage, strict close/kill persistence, crash/shutdown transcript backfill, offline workspace scanning, a promptId index, or rewrite-pipeline refactoring.

Post-merge verification on the exact pushed head: Session 631/631, ACP bridge 661/661, recording service 80/80, Session service 139/139; workspace build, typecheck, lint, bundle, and diff check passed. Bundled daemon E2E passed 3/3 for final parent answer after a tool boundary, stable truncation status, and normal Session reload lookup; capability E2E passed 1/1.

The supported restart boundary remains explicit: recording must be enabled, append must succeed, the result must remain on the active branch and within the bounded scan window, and the Session must be loaded live again. Deleted JSONL, disabled/failed recording, unexpected process crash, daemon shutdown, or results outside the window may return prompt_not_found.

@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

Exact-head E2E test report

Tested commit: bbf24434c390c6ee1fd1856013937196ae0f60d1

Environment: macOS, Node.js 22-compatible repository toolchain, QWEN_SANDBOX=false, the built dist/cli.js, a real qwen serve process, and the repository fake OpenAI-compatible server.

  • Repository build: passed.
  • Bundle and asset copy: passed.
  • Pollable turn-result E2E: 3/3 passed.
    • A response with visible pre-tool text and a tool call returned only the final parent answer after the tool boundary.
    • A result above 32,768 UTF-16 code units returned the bounded text, resultTruncated: true, and RESULT_TEXT_TRUNCATED.
    • A normally recorded settled result remained queryable after closing and loading the Session again.
  • Capability E2E: 1/1 passed; session_turn_status is advertised without an additional flag or setting.

The generic Integration Tests (CLI, No Sandbox) CI job is skipped by the workflow for this PR; the focused scenarios above were run locally against the exact-head built bundle. This evidence does not claim deleted-JSONL recovery, recording-failure recovery, crash/shutdown backfill, offline Session lookup, or permanent result retention; those remain explicitly outside this PR.

@github-actions github-actions Bot removed the review/self-reported The linked issue was opened by the PR author (self-reported) label Aug 14, 2026
@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

Maintainer handoff

Current head: bbf24434c390c6ee1fd1856013937196ae0f60d1.

  • All previously reported correctness fixes have been verified against the current head and replied to in their original threads.
  • 48 of 50 review threads are resolved. The only two open threads are duplicate R1-4 reports for the same deadline overlay vs late child-settlement boundary. The disagreement is real and is now disclosed in the PR Risk & Scope; eliminating it would require durable terminal reconciliation or a permanent task-result ledger. Maintainer confirmation of the bounded contract is requested.
  • The PR now uses Related to #8680 rather than claiming to close the stronger issue contract.
  • Exact-head repository build and bundle passed. Exact-head built-bundle real-daemon E2E passed 4/4: capability discovery 1/1 and pollable turn results 3/3 (tool-boundary final answer, stable truncation signaling, and normal Session reload).
  • Current required CI checks that have completed are green, including Ubuntu Test, Serve A/B, Real daemon E2E, and web-shell E2E. The automated review-pr and route workflow are still pending.

Under the documented bounded, best-effort contract, there is no known remaining production-code blocker. Please confirm whether the deadline boundary is acceptable for this PR; if stronger cross-restart terminal consistency is required, it should be designed as a separate durable task-result subsystem rather than extending this polling diff.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite (integration-tests/cli, incl. the new turn-status E2Es) did not run locally.

Not explored to full depth (tool budget reached): chunk 4: none — all checks I needed completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks above completed within budget.; "You are review agent reverse-audit — Reverse audit agent…": none — all checks I started were completed within budget., and 8 more.

中文说明

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

未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite (integration-tests/cli, incl. the new turn-status E2Es) did not run locally。

未探索到全部深度(达到工具调用预算):chunk 4:none — all checks I needed completed within budget."You are review agent reverse-audit — Reverse audit agent…"none — all checks I started were completed within budget."You are review agent reverse-audit — Reverse audit agent…"none — all checks I started completed within budget."You are review agent reverse-audit — Reverse audit agent…"none — all checks above completed within budget."You are review agent reverse-audit — Reverse audit agent…"none — all checks I started were completed within budget.,另有 8 条。

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

Comment thread packages/acp-bridge/src/bridge.ts
Comment thread packages/acp-bridge/src/bridge.ts
Comment thread packages/acp-bridge/src/bridge.test.ts
Comment thread packages/cli/src/acp-integration/session/Session.test.ts
Comment thread packages/acp-bridge/src/bridge.ts
Comment thread packages/core/src/services/chatRecordingService.ts Outdated
Comment thread packages/cli/src/acp-integration/session/Session.test.ts
Comment thread packages/cli/src/acp-integration/session/Session.test.ts
Comment thread packages/cli/src/acp-integration/session/Session.test.ts
Comment thread packages/acp-bridge/src/bridge.test.ts
When the prompt-deadline path latches an error terminal in the overlay and the child later settles and persists a non-error turn_result for the same promptId, the poll surface previously kept the overlay error while enriching it with the successful resultText, and flipped to completed only after overlay eviction or restart. Merge via mergeTerminalWithPersisted at the two enrich call sites so the persisted outcome supersedes a bridge-synthesized error terminal once it exists; the exactly-once turn_error event publication and FIFO release are unchanged. The different-promptId endedAt tie-break is intentionally untouched.
@BenGuanRan

BenGuanRan commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Update: R1-4 deadline-consistency fix pushed

New head: 599543fe8d (on top of bbf24434c3, which was based on current main).

This supersedes the handoff's "maintainer confirmation of the bounded contract" ask for the R1-4 deadline boundary: instead of leaving the disagreement disclosed, the merge now prefers the child's persisted outcome on the poll surface once it exists and is non-error, so polls never emit an error state enriched with a successful resultText, and the timing-dependent error→completed flip is gone. The exactly-once turn_error event and FIFO release semantics are unchanged.

  • Scope: 1 helper + 2 merge call sites in the bridge, 2 doc edits, 3 regression tests.
  • Verification at this head: bridge suite 664/664, repo typecheck and build green, prettier/eslint clean. Two independent pre-commit code reviews (correctness + house-style/regression scope) both passed.
  • The 13 Suggestion items from the latest review round are intentionally deferred to keep this diff bounded, per the repo's review-round guidance.

The three R1-4 threads have been replied to with details.

…us-polling-v2

# Conflicts:
#	packages/cli/src/serve/server/telemetry.test.ts
@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

Heads-up on the new head: 565ec771cc is a main-merge-only commit — main advanced past our last merge and produced one conflict in the route-census test (telemetry.test.ts), resolved as the union of both sides (56 routes, 54/2 attribution split; verified by the catalog test itself). No functional changes relative to 67a4cb8db9, so the round-5 outcome stands unchanged: zero Criticals, four Suggestions deferred to #9240.

中文说明

新 head 565ec771cc 仅为合并 main:main 前进后与路由普查测试(telemetry.test.ts)产生一处冲突,已按两侧并集解决(56 条路由、54/2 归属拆分,由普查测试自身验证)。相对 67a4cb8db9 无功能变更,第五轮结论不变:零 Critical,四条 Suggestion 已 defer 至 https://github.com/QwenLM/qwen-code/issues/9240。

R3-3: cap promptId, stopReason, and originatorClientId at 256 chars in isTurnResultRecordPayload, closing the unbounded echo of corrupted-transcript values through GET /session/:id/turns/:promptId; recordTurnResult now validates payloads against the same contract before appending, so type-correct but invalid shapes (error state without error, error on non-error states) can no longer produce records invisible to the restart scan.

Also lands the four round-5 test assertions: merged-payload error-leak pin, multi-model-call settle count, successor attribution in the superseded-throws test, and the early session-mismatch guard pin.
@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

Final round: remaining six findings fixed — zero unresolved threads

The six threads left open after the round-5 closeout were all resolved by fixing them directly in 82d689b60b (re-evaluated cost: all low), each verified by two independent reviews (mechanism + tests/mutation-sensitivity) before push:

  • R3-3 (both sides): isTurnResultRecordPayload now caps promptId/stopReason/originatorClientId at 256 chars (closes the unbounded echo of corrupted-transcript values through the poll endpoint), and recordTurnResult validates payloads against the same contract before appending (no more type-correct-but-invisible records on the restart-scan path)
  • Round-5 test pins (4): merged-payload error-leak assertion, multi-model-call settle count, successor attribution in the superseded-throws test, and the early session-mismatch guard pin

Verification: chatRecordingService 100/100, Session 644/644, bridge 698/698, full typecheck clean, lint/format clean.

Current state: all review threads resolved (0 open), MERGEABLE, awaiting maintainer re-review to clear the stale CHANGES_REQUESTED (from the round-3 review; every finding raised since has been fixed or resolved with evidence). Not pinging anyone — leaving for maintainer triage.

中文说明

R5 收口后剩余的 6 个线程已全部直接修复于 82d689b60b(重新评估成本后均不高),推送前经双独立审核(机制 + 测试变异敏感性):R3-3 两侧(三字段 256 上限 + 写前契约校验)、R5 四条测试钉扎。验证:100/644/698 全绿、typecheck/lint/格式干净。当前 0 未解决线程、可合并,等 maintainer 重审解除旧 CHANGES_REQUESTED(那是第 3 轮遗留;此后所有发现均已修复或有证据地解决)。不 @ 任何人。

@qqqys

qqqys commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Review: pollable daemon turn status

I read the non-test diff hunk by hunk against upstream/main, plus the surrounding code it depends on — the bridge FIFO/terminal latch, the Session.prompt send loop, transcript reader paging, the fork/branch record filters, and Config.startNewSession. One high-severity finding, two medium, two nits.

1. (high) The new outer catch masks real turn failures as cancelled

packages/cli/src/acp-integration/session/Session.ts:3549

The wrapper's catch returns {stopReason:'cancelled'} whenever turnRecording.abortController is aborted with USER_CANCEL / NEW_PROMPT / SESSION_DISPOSE — regardless of what actually threw.

That reverses a deliberate decision made by the inner send-error handler in the same file (~line 4721), which explicitly excludes NEW_PROMPT_ABORT_REASON with the comment:

Other AbortErrors still surface so infrastructure failures are not hidden as cancellations

The new wrapper applies to every prompt carrying an invocationContext, i.e. every daemon prompt.

Concrete scenario:

  1. Prompt A exceeds the bridge deadline; the bridge releases the FIFO while A keeps running.
  2. Prompt B dispatches and calls this.pendingPrompt?.abort(NEW_PROMPT_ABORT_REASON) on A.
  3. A then fails with a genuine API/RequestError.

Previously that error propagated and the bridge published turn_error. Now A returns cancelled, writes a cancelled turn_result, and — through mergeTerminalWithPersisted — that persisted non-error outcome supersedes the deadline error on the poll surface. The client is told the turn was cancelled when it actually failed, which is exactly the outcome the inner handler's comment was written to prevent.

Suggested fix: gate the conversion on the same reasons the inner handler accepts (or on error actually being an AbortError), rather than on the abort flag alone.

2. (medium) Every poll pays a full child transcript scan; an unknown promptId pays the worst case

packages/acp-bridge/src/bridge.ts:10252

getSessionTurnStatus consults the in-process overlay only after the awaited requestSessionStatus. So even when the terminal is already sitting in entry.terminalTurnStatuses, the child still flushes the recorder and walks up to 10 pages × 500 records (4 MiB each) through findSettledTurnResult.

For a promptId that does not exist — or has scrolled out of the window — there is no early exit at all. GET /session/:id/turns/<random-uuid> forces the maximum scan on every request, so a cheap authorized request amplifies into a 5000-record disk read, and it is trivially loopable.

Compounding this: requestSessionStatus is called with the default initTimeoutMs (10 s, DEFAULT_INIT_TIMEOUT_MS). On a large transcript a legitimate poll hits withTimeout, the bridge's fallback finds nothing live and rethrows, and the route returns an error instead of a status.

Suggested fix: check the overlay before the child read when it already answers the query, and give the scan its own (larger) timeout.

3. (medium) The terminal reported for a given promptId is not stable across polls

packages/acp-bridge/src/bridge.ts:2161

mergeTerminalWithPersisted lets a persisted non-error outcome supersede an overlay error. But the persisted record is only visible while it stays inside the bounded 10-page window, and the overlay only holds 64 terminals.

A client polling a deadline-exceeded prompt therefore observes:

error (before settle) → completed (after settle, while in-window) → error again (overlay entry still present, transcript record scrolled out) → 404 (both gone)

Fields disappear rather than stabilize too: once the overlay entry is evicted, the persisted-only status drops queuedAt, and promptTextTruncated if it came from the overlay.

The design doc frames the boundedness as a lookup-miss tradeoff, but the observable effect is a terminal state that moves backwards, and polling clients will read that as a real state change. At minimum this belongs in docs/design/daemon-turn-status-endpoint.md as an explicit non-monotonicity note.

4. (nit) The promptDisplayText rewrite changes persisted/traced text outside this feature's surface

packages/acp-bridge/src/bridge.ts:8064

Changing const promptDisplayText = channelDisplayText to channelDisplayText === undefined ? undefined : pendingText only differs when channelDisplayText === '' and the prompt carries an image block — pendingText is then '[image]'.

That value is forwarded as DAEMON_PROMPT_DISPLAY_TEXT_META_KEY and consumed in Session.ts at ~4298 for recorder.recordUserMessage(..., { displayText }) and at ~4233 for addAgentInputMessageAttributes. So an image-only channel prompt whose worker supplied an empty display text now renders as [image] in the transcript/UI and in telemetry, where it previously rendered empty.

If the goal was only to make turn_result.promptText say [image], that belongs in #beginTurnRecording — its promptDisplayText ?? extractTurnPromptText(...) already produces [image], it just needs to treat '' as absent — rather than in the bridge's display-text forwarding.

5. (nit) Every daemon prompt now accumulates the full streamed response in memory

packages/cli/src/acp-integration/session/Session.ts:1263

beginChannelDeliveryResponseBlock previously returned undefined (no array, no accumulation) unless the turn had a channelDelivery. It now returns [] whenever capture.turnResult is set — i.e. for every prompt with an invocationContext.

Each streamed text part is pushed into that array (~4642, ~5661) and only truncated to 32 K at #settleTurnRecording. A multi-megabyte model answer is therefore fully retained per in-flight daemon turn, across all concurrent sessions, purely to keep a 32 K prefix. Truncating on push — stop appending once the block exceeds TURN_RESULT_TEXT_MAX_CHARS — gives the same result with a bounded footprint.


Things I checked that are correct

Worth stating explicitly, since several of these are the parts that would be easy to get wrong:

  • turn_result is a conversation record, so it lands in replayUuids and is reachable by the backward scan.
  • Backward cursor paging keeps direction and pins the snapshot, so multi-page scans stay consistent.
  • isTurnResultRecordPayload bounds match what the writer emits.
  • The fork filter, NEUTRAL_TAIL_SUBTYPES, and the KNOWN_RECORD_SUBTYPES additions are all needed and mutually consistent.
  • branch_checkpoint is written before turn_result, so the activeChain[i-1] === checkpoint.parentUuid invariant still holds.
  • Queued-removal splices and publishes a terminal, so the overlay is populated.
  • pinSessionIdentity is guarded by the same binding === undefined condition that gates startNewSession.
  • Route ordering (/turns/current before /turns/:promptId) is correct.

One thing that looked like a regression and is not: hoisting the invocationContext.sessionId !== config.getSessionId() check into prompt() reads like a /clear-rotation bug, but it duplicates the pre-existing check in #executePrompt, so the behaviour is unchanged.

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Second-pass verification of the open findings against head 82d689b.

Finding 1 (high) — confirmed at code level. The outer catch in prompt() (~L3549) converts any thrown error to {stopReason:'cancelled'} whenever the abort controller carries USER_CANCEL / NEW_PROMPT / SESSION_DISPOSE. The inner send-error handler (~L4717) deliberately excludes NEW_PROMPT from controlled cancellation, with the explicit comment "Other AbortErrors still surface so infrastructure failures are not hidden as cancellations." The scenario is real: a prompt aborted by a newer prompt that then fails with a genuine API error now settles as cancelled, and via mergeTerminalWithPersisted that non-error terminal supersedes the real failure on the poll surface. Agree it should be gated on the same reasons the inner handler accepts.

Finding 2 (medium) — substantively valid, one nuance. getSessionTurnStatus does consult live status before the child read (liveBeforeRead, ~L10243), but the terminal overlay (entry.terminalTurnStatuses) is only consulted after requestSessionStatus resolves or fails. An already-settled promptId therefore still pays the full child transcript scan, and an unknown promptId pays the worst case with no early exit.

Routes/auth check (my own pass). Both new GET routes resolve through the shared requireSessionRuntime gate and the bridge calls resolveTrustedClientId before any read; no authorization gap found. Route ordering (/turns/current registered before /turns/:promptId) is correct.

Holding approval until finding 1 is addressed.

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

Not reviewed: 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/cli/src/acp-integration/session/Session.test.ts:4901 — [probe] error/cancelled settle tests lack a toHaveBeenCalledTimes(1) pin against double-settle regressions
  • docs/developers/qwen-serve-protocol.md:2473 — [review] protocol doc overstates deadline supersede — an error settle keeps the deadline error
  • packages/core/src/services/chatRecordingService.ts:623 — [probe] UTF-16 truncation can split a surrogate pair; strict JSON decoders reject the lone surrogate
中文说明

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

未审查:reverse audit — did not converge within the reverse-audit round cap of 5。

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

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

- Session: settle a successor-aborted turn as cancelled only when the
  thrown error is the abort itself; genuine failures after a NEW_PROMPT
  abort surface as error, matching the send-loop contract
- bridge: serve repeat polls of a settled promptId from the enriched
  overlay instead of re-scanning the child transcript, and give the
  turn-status read the transcript timeout instead of the 10s init default
- bridge: forward the channel display text unchanged; Session treats an
  empty display text as absent for the turn record ([image] fallback)
- Session: cap streamed-response accumulation for turns without a
  channel delivery at the turn-result bound
- docs: document the bounded non-monotonicity of poll terminals
@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

Round-6 review fixes — head 0b5c336dac

All five findings from the latest review pass are addressed in 0b5c336dac (on top of 82d689b6). Point by point:

1. (high) Outer catch masks real turn failures as cancelled — fixed. The conversion is now gated exactly as suggested: USER_CANCEL / SESSION_DISPOSE always convert, and NEW_PROMPT converts only when the thrown error is actually an AbortError (Session.ts:3605). A genuine API/RequestError landing on a prompt that a successor aborted now settles the turn as error and rethrows, so the bridge publishes turn_error and the persisted non-error outcome no longer supersedes the real failure — the inner send-error handler's contract ("Other AbortErrors still surface so infrastructure failures are not hidden as cancellations") is restored.

  • Tests: the supersede-handoff pins are kept, plus a new surfaces a genuine failure after a successor abort test that reproduces the exact scenario from the finding.
  • Note on the alternative: gating on abort reasons alone (dropping NEW_PROMPT entirely) would break the pinned supersede-handoff cases where a successor's AbortError under NEW_PROMPT legitimately settles as cancelled; the AbortError gate is the variant the second-pass verification endorsed.

2. (medium) Every poll pays a full transcript scan — fixed.

  • Overlay-first: once a terminal has been enriched (whether merged with live status or persisted-only), it is written back into the in-process terminal overlay (rememberEnrichedTerminalTurnStatus, tracked by enrichedTerminalPromptIds, cleaned on rewind/eviction). Subsequent polls for the same promptId early-return before any child read.
  • Timeout: the transcript scan now runs under a dedicated SESSION_TRANSCRIPT_TIMEOUT_MS (60 s) instead of the 10 s default init timeout.
  • Tests: a cache-hit test proving the second poll is served from the overlay with no additional child read, and a negative test proving a bare (non-enriched) terminal still rescans until the transcript record becomes visible.
  • Scope note: the unknown-promptId worst-case scan is now bounded by the dedicated scan timeout; a negative cache was considered beyond the suggested fix and is recorded as a follow-up.

3. (medium) Terminal for a promptId not stable across polls — documented. docs/design/daemon-turn-status-endpoint.md now carries an explicit non-monotonicity note describing the observable error → completed → error → 404 sequence and the field drops after overlay eviction, plus the write-back behaviour for both merged and persisted-only terminals.

4. (nit) [image] rewrite leaking into transcript/telemetry — fixed. The bridge forwards channelDisplayText unchanged again (byte-identical to pre-PR behaviour); the [image] fallback now lives solely in #beginTurnRecording, which treats an empty display text as absent.

5. (nit) Unbounded streamed-response accumulation — fixed. Turns without a channel delivery now accumulate through a { parts, chars, capChars } block that stops appending once TURN_RESULT_TEXT_MAX_CHARS + 1 chars are reached (Session.ts:1288) — the +1 keeps the exactly-at-limit case from being flagged truncated. Memory per in-flight daemon turn is now bounded by the 32 K result bound regardless of model answer size; channel-delivery turns stay uncapped as before.

  • Tests: the multi-chunk delivery guard was strengthened so applying the cap to delivery text makes the test fail.

Verification at 0b5c336dac: npm run build and npm run typecheck clean, ESLint clean, unit tests Session.test.ts 647/647, bridge.test.ts 700/700, acpAgent.test.ts 435/435, chatRecordingService.test.ts 100/100. Two independent review passes over the fix (mechanism review and test review, including mutation checks on the new pins) both approved.

Deferred (recorded, not dropped): internal test-review hardening suggestions (cap-memory observability, rewind+cap combination, enriched-marker cleanup, SESSION_DISPOSE turn-recording pin) and the unknown-promptId negative cache from finding 2 — kept out of this round to avoid scope growth.

中文摘要

最新一轮评审的 5 个问题已在 0b5c336dac 全部处理:

  1. (高)外层 catch 把真实失败误报为 cancelled:按建议用 AbortError 门控——USER_CANCEL/SESSION_DISPOSE 照常转换,NEW_PROMPT 仅当抛出的错误确为 AbortError 时才转换;后继 prompt 中止后发生真实 API 错误的场景现在会落 error 并重新抛出,桥接层发布 turn_error,不再用"取消"覆盖真实失败。原有 supersede 交接测试全部保留,并新增复现该场景的测试。
  2. (中)每次轮询全量扫描转录:已富化的终态(合并或仅持久化)写回进程内 overlay,同一 promptId 的后续轮询在读子进程之前提前返回;转录扫描改用专用 60s 超时替代默认 10s。附缓存命中与"未富化仍需重扫"两个测试。未知 promptId 的负缓存超出建议范围,记录为后续项。
  3. (中)终态跨轮询不稳定:设计文档新增显式的非单调性说明(error → completed → error → 404 序列及 overlay 逐出后的字段缺失)和写回条款。
  4. (小)[image] 改写泄漏到转录/遥测:桥接层恢复原样转发 channelDisplayText(与 PR 前逐字节一致),[image] 兜底收敛到 #beginTurnRecording(空串视为缺省)。
  5. (小)流式响应无限累积:无渠道投递的 turn 改用带上限的累积块,达到 TURN_RESULT_TEXT_MAX_CHARS + 1 即停止追加(+1 保证恰好到限长不误报截断);单 turn 内存被 32K 结果上限约束,渠道投递 turn 维持不设上限。

验证:build/typecheck/ESLint 全部干净;单测 Session 647/647、bridge 700/700、acpAgent 435/435、chatRecordingService 100/100;两轮独立子评审(机制 + 测试,含对新测试的变异检查)均已通过。测试加固类建议(G3–G6)与未知 promptId 负缓存按"控制 PR 膨胀"原则记录为后续项。

@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

Merged origin/main (through c0791f0450, incl. #9310 session media references) to resolve the conflicts introduced by upstream movement — head is now ce60995ea6.

Conflict resolution notes:

  • The capability/route-catalog conflicts were union resolutions: session_turn_status (this PR) and session_media (feat: support session media references end-to-end #9310) are both kept; the legacy telemetry route catalog count moves 56/57 → 59 (54 base + 2 turn-status + 3 media) with the audited attribution split 57/2.
  • The bridge.ts and Session.ts conflicts were adjacent-addition conflicts (turn-status helpers vs. media helpers); both sides are kept intact with no behavioural interleaving.

Re-verified at the merge commit: npm run build + npm run typecheck clean, ESLint + Prettier clean, integration-tests typecheck clean; unit tests bridge.test.ts 726/726, serve tests (server.test.ts + telemetry) 1059/1059, Session.test.ts 653/653, acpAgent.test.ts 435/435, chatRecordingService.test.ts 102/102. The round-6 fixes from 0b5c336dac are unchanged and their pins still pass.

中文摘要

已合并 origin/main(至 c0791f0450,含 #9310 会话媒体引用),解决上游前移产生的冲突,新 head 为 ce60995ea6。冲突均为"两侧同位置各自新增":能力/路由目录取并集(session_turn_statussession_media 均保留,遥测路由目录数 56/57 → 59,归属拆分 57/2);bridge.ts/Session.ts 为相邻新增,两侧函数原样保留、无行为交织。合并提交上重新验证:build/typecheck/ESLint/Prettier/集成测试类型检查全干净;单测 bridge 726/726、serve 1059/1059、Session 653/653、acpAgent 435/435、chatRecordingService 102/102。0b5c336dac 的第 6 轮修复未变动且测试钉仍然通过。

@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

@qqqys @yiliang114 — requesting a re-verification at the new head ce60995ea6.

Why the head moved: origin/main advanced (through c0791f0450, incl. #9310 session media references) and conflicted with this branch, so main was merged in. The round-6 fixes for all five findings (0b5c336dac) are unchanged — the merge sits on top of that commit, and none of the conflict resolutions touch the fixed logic:

  • Capability/route-catalog conflicts were union resolutions (session_turn_status + session_media both kept; legacy telemetry route catalog count 59 = 54 base + 2 turn-status + 3 media, attribution split 57/2).
  • bridge.ts / Session.ts conflicts were adjacent additions (turn-status helpers vs. media helpers); both sides kept intact with no behavioural interleaving.

Re-verified at ce60995ea6: npm run build + npm run typecheck clean, ESLint + Prettier clean, integration-tests typecheck clean; unit tests bridge.test.ts 726/726, serve tests 1059/1059, Session.test.ts 653/653, acpAgent.test.ts 435/435, chatRecordingService.test.ts 102/102 — all round-6 pins still green. CI is running on the new head.

Suggested focus: the catalog union counts above, and that the two feature sets coexist cleanly in bridge.ts/Session.ts.

中文

@qqqys @yiliang114 请在新 head ce60995ea6 上复验。head 变动原因:origin/main 前移(至 c0791f0450,含 #9310 会话媒体引用)与本分支产生冲突,已合入 main。针对 5 个 finding 的第 6 轮修复(0b5c336dac未变动——合并位于该提交之上,且冲突解决不触及修复逻辑:能力/路由目录为并集解决(session_turn_statussession_media 均保留;遥测路由目录 59 = 54 基线 + 2 turn-status + 3 media,归属拆分 57/2);bridge.ts/Session.ts 为相邻新增,两侧原样保留、无行为交织。合并提交上复验:build/typecheck/ESLint/Prettier/集成测试类型检查全干净;单测 bridge 726/726、serve 1059/1059、Session 653/653、acpAgent 435/435、chatRecordingService 102/102,第 6 轮测试钉全绿。CI 正在新 head 上运行。建议重点:上述目录并集计数,以及两套功能在 bridge.ts/Session.ts 中的共存。

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Partially reviewed — gaps disclosed.

Not reviewed: reverse audit — stopped at the 5-round cap without converging (round 5 still reporting).

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

  • packages/acp-bridge/src/bridge.ts:10569 — [review] exact-promptId path never checks persisted.promptId === promptId (defense-in-depth across the ACP boundary)
  • packages/cli/src/acp-integration/session/Session.ts:1391 — [review] single oversized part defeats the capChars memory bound in appendChannelDeliveryResponseText
  • packages/acp-bridge/src/bridge.ts:10570 — [probe] write-back cache never warms for the current route; every current poll pays a full child scan; design doc overpromises
中文说明

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

未审查:reverse audit — stopped at the 5-round cap without converging (round 5 still reporting)。

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

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

Comment thread packages/acp-bridge/src/bridge.ts
Comment thread packages/acp-bridge/src/bridge.ts Outdated
qqqys
qqqys previously approved these changes Aug 17, 2026
…us-polling-v2

# Conflicts:
#	packages/acp-bridge/src/bridge.ts
#	packages/cli/src/serve/acp-session-bridge.ts
… trusted prompt projection

A successful rewind that completes while a getSessionTurnStatus child
transcript scan is in flight could let the pre-rewind record be cached
into the freshly cleared overlay and served forever. Track a per-session
rewind generation captured before the scan and discard the scanned
outcome when it moved.

enrichTerminalTurnStatus and the deadline-supersede merge returned the
child-recorded promptText ahead of the bridge's trusted display
projection, leaking hidden channel context on the poll surface. Make
promptText/promptTextTruncated backfill-only and keep the terminal's
projection in the supersede path. Make the pinning test adversarial and
correct a false comment about the child's ''-as-absent fallback.
@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

Round-7 closeout — both Criticals fixed, head f7f155dfbb

Both R7 findings are fixed in f7f155dfbb, on top of merge 6a2f6ca78a (origin/main through 5c56b67182; the two import-list conflicts in bridge.ts / acp-session-bridge.ts were resolved as unions).

R7-1 — rewind race on the poll success path. A per-session rewindGeneration is captured before the child transcript scan and bumped alongside the overlay clear after a successful rewind only. When the generation moved during the read, the scanned record is discarded at the single shared persisted computation, covering the with-promptId write-back branches and both no-promptId branches at once. New regression test beside the R1-2 twin: a late pre-rewind record after a successful rewind → the in-flight poll and a follow-up poll both resolve undefined (nothing cached into the enriched fast path).

R7-3 — prompt display projection leak. promptText/promptTextTruncated are now backfill-only in enrichTerminalTurnStatus, and the deadline-supersede merge keeps the terminal's trusted projection while the persisted non-error outcome stays authoritative (R1-4 intact). The pinning test is now adversarial (channel session, persisted text differs from the projection, enriched fast path included), a supersede-path test was added, and the false [image] comment was corrected.

Verification (at f7f155dfbb)

  • packages/acp-bridge bridge.test.ts 740/740; full package 1570/1570
  • packages/cli serve server.test.ts 1009/1009
  • npm run build + npm run typecheck clean; eslint/prettier clean on changed files
  • All three new tests verified red against the pre-fix code (mutation-sensitive)

Two non-blocking reviewer observations worth recording: a conservative promptTextTruncated flag/text mismatch is possible when hidden context alone crosses the truncation cap (no leak, display-only), and the persisted-only branch after 64-entry overlay eviction serves the child's text as-is by design (documented bounded-window degradation, no bridge projection exists there to protect).

Status: all review threads resolved; reviewDecision still shows the earlier CHANGES_REQUESTED pending maintainer re-review. Re-verification is welcome at head f7f155dfbb.

中文说明

R7 的两个 Critical 均已在 f7f155dfbb 修复,其下为合并提交 6a2f6ca78a(合并 origin/main 至 5c56b67182bridge.ts / acp-session-bridge.ts 的两处导入列表冲突按并集解决)。

R7-1(rewind 竞态,轮询成功路径):新增 per-session rewindGeneration,在子进程 transcript 扫描前捕获、仅在成功 rewind 清空 overlay 的同步块里自增;读取期间 generation 变化时,在 persisted 的唯一计算处丢弃扫描结果,一次性覆盖带 promptId 的两个写回分支与不带 promptId 的两个分支。回归测试与 R1-2 孪生测试并列:成功 rewind 后迟到的 pre-rewind 记录 → 在途轮询与后续轮询均返回 undefined(未缓存进 enriched 快路径)。

R7-3(展示投影泄漏)enrichTerminalTurnStatuspromptText/promptTextTruncated 改为仅回填;deadline supersede 合并保留 terminal 的可信投影,持久化非 error 结果仍权威(R1-4 不变)。钉扎测试改为对抗性(channel 会话、持久化文本与投影不同、含 enriched 快路径二次轮询),新增 supersede 路径测试,并修正错误的 [image] 注释。

验证(f7f155dfbb:bridge.test.ts 740/740;acp-bridge 全包 1570/1570;cli serve server.test.ts 1009/1009;build + typecheck 干净;改动文件 eslint/prettier 干净;三个新测试在无修复代码上均红(变异敏感)。

两点非阻塞的审核观察记录在案:隐藏上下文单独越过截断上限时可能出现 promptTextTruncated 标志与文本的保守不一致(无泄漏,仅展示层面);64 条 overlay 驱逐后的 persisted-only 分支按设计直接返回子进程文本(文档化的有界窗口退化,该处本就没有可保护的 bridge 投影)。

状态:所有评审线程已解决;reviewDecision 仍为此前遗留的 CHANGES_REQUESTED,等待 maintainer 重审。欢迎在 f7f155dfbb 复验。

@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

Real-model E2E at head f7f155dfbb — poll surface verified end to end

Ran the built bundle as a real daemon (loopback, default auth) against a live model (qwen3.7-plus, Bailian Token Plan, OpenAI-compatible endpoint).

Live turn — session 7a61550c-…, prompt 45e23116-…

  • capabilities advertises session_turn_status
  • POST /session/:id/prompt → 202 + server-assigned promptId
  • In-flight poll (GET /session/:id/turns/:promptId): state: running with the trusted promptText projection plus queuedAt/startedAt
  • Settled poll (~6s): state: completed, stopReason: end_turn, resultText: "E2E_TURN_STATUS_OK." — the model's actual final answer only
  • GET /session/:id/turns/current returns the identical record
  • Enriched fast-path re-poll serves the same merged record from the overlay
  • Unknown promptId → 404 with code: prompt_not_found

Restart persistence (persisted-only branch)
After restarting the daemon, POST /session/:id/load reloads the recorded session; polling the same promptId returns the identical completed result from the child's persisted turn_result (child-side timestamps differ from the overlay's, proving the record came from the transcript scan, not in-memory state).

Error path (incidental)
Two earlier runs with rejected model credentials surfaced the real model 401 as a persisted error terminal (Internal error, code -32603), consistently served by both poll routes.

中文说明

在 head f7f155dfbb 上用构建产物起了真实 daemon(loopback、默认鉴权),对接真实模型(qwen3.7-plus,百炼 Token Plan,OpenAI 兼容端点)验证轮询面:

实时 turn:capabilities 广告 session_turn_status;POST prompt 返回 202 与服务端分配的 promptId;在途轮询返回 state: running 与可信 promptText 投影及 queuedAt/startedAt;约 6 秒后终态 state: completedstopReason: end_turnresultText: "E2E_TURN_STATUS_OK."(即模型真实最终回答);/turns/current 与精确 promptId 轮询完全一致;enriched 快路径二次轮询返回同一合并记录;未知 promptId 返回 404 prompt_not_found

重启持久化(persisted-only 分支):daemon 重启后 POST /session/:id/load 重新加载会话,轮询同一 promptId 从子进程持久化 turn_result 返回相同 completed 结果(时间戳为子进程侧记录,证明来自 transcript 扫描而非内存态)。

错误路径(顺带):此前两次凭据被拒的运行中,真实模型 401 以持久化 error terminal(Internal error,code -32603)呈现,两条轮询路由返回一致。

@BenGuanRan

Copy link
Copy Markdown
Collaborator Author

@qqqys @yiliang114 — requesting a re-review at the new head f7f155dfbb.

Since the last request (ce60995ea6):

  • Both R7 Criticals are fixed in f7f155dfbb (rewind-generation guard for the poll success path; backfill-only trusted prompt projection, including the deadline-supersede merge) — details in the thread replies above
  • origin/main merged through 5c56b67182; GitHub reports the branch MERGEABLE
  • All review threads resolved; bridge 740/740, acp-bridge package 1570/1570, serve server.test.ts 1009/1009, build/typecheck/eslint clean; the three new tests are mutation-verified red without the fix
  • A real-model E2E at this head is posted above (live running → completed resultText, restart persistence through the persisted-only branch, 404 contract)

reviewDecision still carries the earlier CHANGES_REQUESTED; a fresh look would clear it if the changes are acceptable.

中文说明

请在新 head f7f155dfbb 上复验。相对上次请求(ce60995ea6)的变化:R7 的两个 Critical 已在 f7f155dfbb 修复(轮询成功路径的 rewind generation 守卫;展示投影改为仅回填,含 deadline supersede 合并),详见上方线程回复;已合并 origin/main 至 5c56b67182,GitHub 显示 MERGEABLE;所有评审线程已解决,测试与构建证据见上;本 head 的真实模型 E2E 报告也已贴上(在途→completed resultText、重启后 persisted-only 分支持久化、404 契约)。reviewDecision 仍遗留此前的 CHANGES_REQUESTED,若改动可接受,复验后可解除。

@BenGuanRan
BenGuanRan requested a review from yiliang114 August 18, 2026 06:19

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approving at head f7659d6 (merge of main; PR files unchanged by the merge).

  • Round-7 Criticals verified at f7f155d: the rewind-generation counter discards a child-transcript scan outcome when a successful rewind completes mid-scan (neither served nor cached), and promptText/promptTextTruncated are now backfill-only with the deadline-supersede path keeping the bridge's trusted projection — hidden channel context can no longer surface on the poll routes. Both have adversarial regression tests.
  • Prior review rounds' findings all carry through at this head; all 70 review threads are resolved.
  • Test suites were green at the pre-merge head f7f155d; CI on the merged head is still running and merge gating will hold until it lands.

@BenGuanRan
BenGuanRan dismissed qwen-code-ci-bot’s stale review August 18, 2026 06:35

Both R7 Criticals fixed at f7f155d per the bot's own suggestions; threads resolved, tests green, two write-access approvals at new head.

@BenGuanRan
BenGuanRan added this pull request to the merge queue Aug 18, 2026
Merged via the queue into QwenLM:main with commit 9a64c0a Aug 18, 2026
52 of 53 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.21.14.

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.

6 participants