Skip to content

fix(transcript): mark dangling tool history incomplete - #7340

Merged
ytahdn merged 2 commits into
QwenLM:mainfrom
cxruan:fix/chat-record-replay-followups
Jul 21, 2026
Merged

fix(transcript): mark dangling tool history incomplete#7340
ytahdn merged 2 commits into
QwenLM:mainfrom
cxruan:fix/chat-record-replay-followups

Conversation

@cxruan

@cxruan cxruan commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

What this PR does

This follow-up makes incomplete offline ChatRecord history explicit when a persisted tool call has no matching result.

Why it's needed

The audit after #6999 found that replay already synthesizes a failed tool result for a dangling call, but the public projection still reports the transcript as complete and provides no diagnostic identifying the missing persisted result.

Why the other audit findings are out of scope

These findings are therefore not regressions introduced by the real #6999 ChatRecord projection path and are not required in this focused follow-up. They can be tracked separately if their existing behavior needs to change.

Reviewer Test Plan

How to verify

  • Project a transcript containing a tool call with no result. Confirm the failed tool block remains visible, complete is false, and the missing_tool_result diagnostic identifies the source record.

Evidence (Before & After)

Before: a dangling call rendered as failed while its transcript was still reported as complete. After: focused ACP and SDK regression suites cover the completeness diagnostic and public projection result. No TUI surface changes.

Tested on

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

Environment (optional)

Node.js workspace tests and builds without a sandbox.

Risk & Scope

  • Main risk or tradeoff: consumers that treat complete: false as a hard failure will now correctly reject dangling persisted history.
  • Not validated / out of scope: the pre-existing or unreachable audit findings described above are intentionally unchanged.
  • Breaking changes / migration notes: none.

Linked Issues

Follow-up to #6999.

中文说明

本 PR 做了什么

这个 follow-up 会在持久化工具调用缺少匹配结果时,明确标记离线 ChatRecord 历史不完整。

为什么需要

#6999 合并后的审计发现,回放虽然已经会为悬空调用生成失败工具结果,但公共投影仍将 transcript 报告为完整,也没有 diagnostic 指出缺失的持久化结果。

为什么其他审计项不在本 PR 范围内

因此,这些发现都不是实际 #6999 ChatRecord 投影链路引入的回归,也不是本次聚焦 follow-up 的必要修复。如果需要改变这些既有行为,可以单独跟踪。

Reviewer 测试计划

如何验证

  • 投影一个只有工具调用、没有结果的 transcript。确认失败工具块仍然可见、complete 为 false,并且 missing_tool_result 诊断指向源记录。

证据(修复前后)

修复前:悬空调用显示为失败,但 transcript 仍被报告为完整。修复后:聚焦的 ACP 和 SDK 回归测试覆盖完整性 diagnostic 与公共投影结果。没有 TUI 表面变化。

测试平台

OS 状态
🍏 macOS
🪟 Windows N/A
🐧 Linux N/A

环境(可选)

Node.js workspace 测试与构建,未启用 sandbox。

风险与范围

  • 主要风险或取舍:将 complete: false 视为硬失败的消费者现在会正确拒绝悬空的持久化历史。
  • 未验证 / 范围外:上述既有或不可达的审计项有意保持不变。
  • 破坏性变更 / 迁移说明:无。

关联 Issue

#6999 的 follow-up。

@cxruan
cxruan marked this pull request as ready for review July 20, 2026 12:28
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: observed bug from the #6999 audit — finalize() already synthesizes a failed tool result for a dangling call, but the public projection still reports complete: true and emits no diagnostic identifying the missing persisted result. The test changes demonstrate the before/after clearly.

Direction: aligned. Correctly reporting completeness is fundamental to the SDK's transcript projection API. A consumer that trusts complete: true would silently accept an incomplete history.

Size: not applicable — no core paths touched. 5 production lines (one this.report() call), 1 doc table row, 18 test lines.

Approach: the scope feels right. Uses the existing report() diagnostic infrastructure with a new missing_tool_result code, updates the design doc's diagnostic table, and adds assertions at both the machine level (acp-bridge) and the projection level (sdk-typescript). Nothing to cut.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题:来自 #6999 审计的已观测 bug——finalize() 已经会为悬空调用合成失败工具结果,但公共投影仍报告 complete: true,也没有 diagnostic 指出缺失的持久化结果。测试变更清楚展示了修复前后行为。

方向:对齐。正确报告完整性是 SDK transcript 投影 API 的基础能力。信任 complete: true 的消费者会静默接受不完整历史。

规模:不适用——未触及核心路径。5 行生产代码(一个 this.report() 调用)、1 行文档表格、18 行测试。

方案:范围合理。使用现有 report() diagnostic 基础设施,新增 missing_tool_result 代码,更新设计文档 diagnostic 表,并在 machine 层(acp-bridge)和投影层(sdk-typescript)都添加了断言。没有可砍的部分。

进入代码审查 🔍

Qwen Code · qwen3.7-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: given the problem (dangling tool calls get synthetic failed results but complete stays true), I would add a diagnostic emission in finalize() using the existing this.report() mechanism, with affectsCompleteness: true (the default), so the SDK projection's complete computation picks it up. Update the design doc's diagnostic table. Add test assertions at both the machine and projection levels.

Comparison with the diff: the PR does exactly this. The report() call is placed before the yield in the finalize() loop, which is correct — the diagnostic is emitted regardless of whether anyone consumes the emission. The diagnostic code missing_tool_result is consistent with the existing naming convention (ambiguous_tool_call_correlation, unknown_record_or_part, etc.). The recordId is pending.sourceRecordId, correctly identifying the record containing the dangling call.

The diagnostic flows through the established path: this.report()onDiagnostic callback → addDiagnostic in chat-record-transcript.tsdiagnostics array → complete = !truncated && !diagnostics.some(d => d.affectsCompleteness). No new plumbing needed.

No critical blockers. No convention violations.

Testing

Unit tests (applied PR diff to worktree, ran both changed test files):

✓ src/transcript-replay.test.ts (6 tests) 8ms
  Test Files  1 passed (1)
       Tests  6 passed (6)

✓ test/unit/daemon-transcript-projection.test.ts (14 tests) 16ms
  Test Files  1 passed (1)
       Tests  14 passed (14)

Typecheck (tsc --noEmit on acp-bridge): clean.

Tmux smoke test (non-TUI change — verified build and CLI startup):

$ node packages/cli/dist/index.js --version
0.20.0

$ node packages/cli/dist/index.js --model qwen3-coder-plus

   ▄▄▄▄▄▄  ▄▄     ▄▄ ▄▄▄▄▄▄▄ ▄▄▄    ▄▄   ┌──────────────────────────────────────────────────────────┐
  ██╔═══██╗██║    ██║██╔════╝████╗  ██║  │ >_ Qwen Code (v0.20.0)                                   │
  ██║   ██║██║ █╗ ██║█████╗  ██╔██╗ ██║  │                                                          │
  ██║▄▄ ██║██║███╗██║██╔══╝  ██║╚██╗██║  │ API Key | qwen3-coder-plus (/model to change)            │
  ╚██████╔╝╚███╔███╔╝███████╗██║ ╚████║  │ ~/.../_work/qwen-code/qwen-code/.qwen/worktrees/triage   │
   ╚══▀▀═╝  ╚══╝╚══╝ ╚══════╝╚═╝  ╚═══╝  └──────────────────────────────────────────────────────────┘

  > /quit
  Agent powering down. Goodbye!
中文说明

代码审查

独立方案: 针对该问题(悬空调用获得合成失败结果但 complete 仍为 true),我会在 finalize() 中使用现有 this.report() 机制添加 diagnostic 发射,affectsCompletenesstrue(默认值),使 SDK 投影的 complete 计算能捕获它。更新设计文档 diagnostic 表。在 machine 层和投影层都添加测试断言。

与 diff 对比: PR 完全这样做了。report() 调用放在 finalize() 循环的 yield 之前,这是正确的——无论是否有人消费 emission,diagnostic 都会被发射。diagnostic 代码 missing_tool_result 与现有命名约定一致。recordIdpending.sourceRecordId,正确标识包含悬空调用的记录。

diagnostic 通过既有路径流转:this.report()onDiagnostic 回调 → chat-record-transcript.ts 中的 addDiagnosticdiagnostics 数组 → complete = !truncated && !diagnostics.some(d => d.affectsCompleteness)。无需新的管道代码。

无关键阻塞项。无约定违反。

测试

单元测试(将 PR diff 应用到 worktree,运行两个变更的测试文件):全部通过(6 + 14 = 20 个测试)。

类型检查(acp-bridge 的 tsc --noEmit):干净。

Tmux 冒烟测试(非 TUI 变更——验证构建和 CLI 启动):CLI 正常启动和退出。

Qwen Code · qwen3.7-max

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 5/5

Clean, focused correctness fix. One this.report() call in finalize() closes the gap where a dangling tool call produced a synthetic failed result but the projection still claimed complete: true. The diagnostic flows through the existing infrastructure, the design doc table is updated, and both the machine-level and projection-level tests verify the new behavior. No new abstractions, no scope creep, no downstream consumers at risk — the only behavioral change is that complete now correctly reflects reality.

LGTM, approving. ✅

中文说明

置信度:5/5

干净、聚焦的正确性修复。finalize() 中的一个 this.report() 调用填补了悬空调用产生合成失败结果但投影仍声称 complete: true 的缺口。diagnostic 通过现有基础设施流转,设计文档表格已更新,machine 层和投影层测试都验证了新行为。无新抽象、无范围蔓延、无下游消费者风险——唯一的行为变化是 complete 现在正确反映实际情况。

LGTM,批准。✅

Qwen Code · qwen3.7-max

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

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

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM, looks ready to ship. ✅

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

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

— qwen3.7-max via Qwen Code /review

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

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

— qwen3.7-max via Qwen Code /review

@ytahdn ytahdn left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed. Changes look good — minimal fix for dangling tool call completeness diagnostic, with regression tests at both ACP and SDK layers.

@ytahdn
ytahdn added this pull request to the merge queue Jul 21, 2026
Merged via the queue into QwenLM:main with commit c8b2322 Jul 21, 2026
45 checks passed
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Released in v0.20.1.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants