feat(telemetry): align session lifecycle with OpenTelemetry - #8616
Conversation
Verification report / 验证报告EnglishImplemented the OpenTelemetry session lifecycle alignment requested in #8589.
Verified locally:
中文已实现 #8589 要求的 OpenTelemetry session lifecycle 对齐：
本地验证：定向 Vitest 578 项通过； |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Thanks for the PR, @zjunothing — and for including the design doc and the linked issue, both of which match what issue triage asked for on #8589.
Before review can proceed, the PR body needs to follow the pull request template. The current body uses free-form headings (Summary / Verification / OTel references) and is missing the required sections:
## What this PR does## Why it's needed## Reviewer Test Plan— including### How to verify,### Evidence (Before & After), and the### Tested onOS table## Risk & Scope## Linked Issues- The Chinese translation wrapped in a
<details>block (the## 中文说明section is already there — it just needs to be inside<details>per the template)
The content you've written is good — the Summary maps directly onto What this PR does / Why it's needed, and the Verification section maps onto How to verify, so this is a restructure rather than a rewrite. The genuinely new parts: the Tested on table (which OSes did you verify on locally?) and Risk & Scope (e.g. backward compatibility with the existing Qwen-specific telemetry events, and what is explicitly out of scope such as gen_ai.agent.id).
One drive-by note while you're editing: the linked issue #8589 is a feature request titled feat(telemetry): ..., but this PR is titled fix(telemetry): .... Since the change adds new standard lifecycle events rather than repairing broken behavior, feat is the accurate conventional-commit type — worth fixing so it lands in the right changelog bucket.
Once the body follows the template, push a new commit or re-trigger with @qwen-code /triage and the gate will run again.
中文说明
感谢提交 PR，@zjunothing——也感谢你提供了设计文档和关联 issue，两者都符合 #8589 issue triage 阶段的要求。
在进入审查之前，PR 正文需要遵循 PR 模板。当前正文使用了自由格式的标题（Summary / Verification / OTel references），缺少以下必需章节：
## What this PR does## Why it's needed## Reviewer Test Plan——包含### How to verify、### Evidence (Before & After)和### Tested on操作系统表格## Risk & Scope## Linked Issues- 包在
<details>块中的中文翻译（已有## 中文说明章节，只需按模板放入<details>中）
你写的内容质量很好——Summary 可以直接对应 What this PR does / Why it's needed，Verification 章节可以对应 How to verify，所以这只是重新组织结构，而不是重写。真正需要补充的是：Tested on 表格（你在本地验证了哪些操作系统？）和 Risk & Scope（例如与现有 Qwen 自定义 telemetry 事件的向后兼容性，以及明确超出范围的部分，如 gen_ai.agent.id）。
顺带一提：关联 issue #8589 是标题为 feat(telemetry): ... 的 feature request，而本 PR 标题是 fix(telemetry): ...。由于该改动是新增标准生命周期事件而非修复损坏行为，feat 才是准确的 conventional commit 类型——建议一并修正，以便进入正确的 changelog 分类。
正文遵循模板后，推送新 commit 或使用 @qwen-code /triage 重新触发，门禁会再次运行。
— Qwen Code · qwen3.8-max
zjunothing
changed the title
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 41 passed · 2 failed · 43 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：41 通过 · 2 失败 · 43 总计 Verification reportPR 8616 — feat(telemetry): align session lifecycle with OpenTelemetryVerdict: 中文摘要
Central claim + A/BCentral claim: with the OTel pipeline enabled, every session lifecycle transition emits standard General-Session LogRecords — Harness: mock-free, drives the compiled
Ordering asserted per switch: Reviewer Test Plan, step by step
FindingsF1 (medium) — initial
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
Test Plan (not a blocker): src/telemetry/session-events.test.ts — no such file or directory; src/telemetry/loggers.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory.
中文说明
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
Test Plan（非阻断）：src/telemetry/session-events.test.ts — no such file or directory; src/telemetry/loggers.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| logStartSession( | ||
| this, | ||
| new StartSessionEvent(this), | ||
| sessionData ? previousSessionId : undefined, | ||
| ); |
There was a problem hiding this comment.
[Critical] Resuming the currently-active session emits session.start with session.previous_id === session.id, violating the invariant this PR's own design doc states ("it is never equal to the new session.id"). The gate keys only on sessionData presence, never on id change. The /resume picker does not exclude the current session (DialogManager renders SessionPicker without disabledIds, unlike the delete dialog right below it), the active session is persisted live and sorts first by mtime, and handleResume has no same-id guard — Failure scenario: /resume + Enter on the first row (the session the user is already in) → session.start with session.previous_id === session.id, a self-loop for any consumer chaining sessions by previous_id.
| logStartSession( | |
| this, | |
| new StartSessionEvent(this), | |
| sessionData ? previousSessionId : undefined, | |
| ); | |
| logStartSession( | |
| this, | |
| new StartSessionEvent(this), | |
| sessionData && previousSessionId !== this.sessionId | |
| ? previousSessionId | |
| : undefined, | |
| ); |
中文说明
恢复当前正在使用的 session 时，会发出 session.previous_id === session.id 的 session.start，违反本 PR 设计文档中"previous_id 永不等于新 session.id"的约定。续接判断仅依据 sessionData 是否存在，未校验 id 是否变化。/resume 选择器没有排除当前 session（DialogManager 渲染 SessionPicker 时未传 disabledIds，而紧随其后的删除对话框传了），当前 session 被实时持久化且按 mtime 排第一，handleResume 也没有同 id 防护——触发路径：/resume 后直接回车选中第一行（即用户当前所在 session）→ 产生自环路的续接记录。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const currentSessionId = getCurrentSessionId(); | ||
| if (currentSessionId) { | ||
| emitSessionEnd(currentSessionId); | ||
| } |
There was a problem hiding this comment.
[Critical] This shutdown session.end is unconditional, but the initial session's session.start is gated behind isTelemetrySdkInitialized() inside logStartSession and is dropped when the SDK has not settled — probe-confirmed against the real SDK with a file exporter: replaying the interactive startup order (logStartSession before init settles → initializeTelemetry → shutdownTelemetry) exports only ["session.end"], while the eager-order control exports ["session.start", "session.end"]. In interactive mode this is deterministic, not a race: deferTelemetryInitialization defers SDK init to post-render, so the initial logStartSession in Config.initialize() always hits the early-return gate — Failure scenario: every telemetry-enabled interactive run exports a session.end with no matching session.start; if the user never runs /clear that is the only session, so lifecycle consumers see a session with no start time. Suggested fix: track which session ids actually emitted session.start and gate the end emissions on it, or emit a catch-up session.start when initializeTelemetry settles (the fix spans session-events.ts/sdk.ts, so no inline suggestion here).
中文说明
关闭时的 session.end 无条件发出，但初始 session 的 session.start 受 logStartSession 内的 isTelemetrySdkInitialized() 门控，SDK 未就绪时被丢弃——已用真实 SDK + 文件 exporter 探针验证：复现交互式启动顺序只导出 ["session.end"]，而先初始化再启动的对照组导出成对记录。交互模式下这是确定性的（遥测初始化被延迟到首屏渲染之后，Config.initialize() 中的首次 logStartSession 必然被门控拦截）——结果：每个启用遥测的交互式运行都会导出没有对应 start 的 end。建议：记录实际发出过 start 的 session id 并据此门控 end，或在 initializeTelemetry 完成时补发 start。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const currentSessionId = getCurrentSessionId(); | ||
| if (currentSessionId) { | ||
| emitSessionEnd(currentSessionId); | ||
| } |
There was a problem hiding this comment.
[Critical] In daemon mode (qwen serve) this ends the wrong session: the process-global context holds the synthetic daemon:<pid> id set by the daemon telemetry runtime config — an id that never had a session.start — while the actually-served sessions (which DID emit session.start via Config.initialize()) never emit session.end: every per-session teardown passes shutdownTelemetry: false and nothing else on those paths calls logSessionEnd — Failure scenario: OTel-enabled qwen serve serving sessions A, B: on SIGTERM exactly one session.end fires, for daemon:<pid>; the served sessions are never ended and the one end has no start, so generic backends cannot determine session boundaries in daemon mode — the observability gap issue #8589 was filed to close ("important when one daemon serves multiple sessions"). Suggested fix: emit logSessionEnd(config) in the per-session teardown paths, and/or gate this shutdown end on ids that actually emitted a start; state multi-session semantics in the design doc.
中文说明
daemon 模式（qwen serve）下结束的是错误的 session：进程级全局 context 持有 daemon 遥测运行时配置写入的合成 id daemon:<pid>（从未发过 session.start），而真正发出过 start 的被服务 session 从不发 end——每个会话的 teardown 都传 shutdownTelemetry: false，且这些路径上没有其他 logSessionEnd 调用。触发场景：启用 OTel 的 qwen serve 服务会话 A、B，SIGTERM 时只发出唯一一条针对 daemon:<pid> 的 end；被服务会话永远不结束、唯一的 end 又没有对应的 start，通用后端无法在 daemon 模式下确定会话边界——这正是 #8589 要补上的观测缺口。建议：在每个会话的 teardown 路径补发 logSessionEnd(config)，并将关闭时的 end 限制在实际发过 start 的 id 上。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const previousSessionId = this.sessionId; | ||
| logSessionEnd(this); | ||
| this.sessionId = sessionId ?? randomUUID(); |
There was a problem hiding this comment.
[Critical] This new logSessionEnd call breaks the existing packages/core/src/config/config-session-env.test.ts suite — measured against the merge base (base-tree + test-delta): the file passes on base and fails only with this PR. Its vi.mock('../telemetry/index.js', ...) factory predates the new export, so both tests exercising startNewSession throw [vitest] No "logSessionEnd" export is defined on the "../telemetry/index.js" mock — Failure scenario: npm test --workspace=packages/core (and thus CI) fails with exactly the +1 file / +2 tests delta measured here (the other 11 failing core files fail identically on base — pre-existing environment issues, not this PR). Fix: add logSessionEnd: vi.fn() to that mock factory, then re-run npx vitest run src/config/config-session-env.test.ts.
中文说明
新增的 logSessionEnd 调用破坏了现有 config-session-env.test.ts 测试套件——已对照合并基线测量（base-tree + test-delta）：该文件在基线上通过、仅在本 PR 上失败。其 vi.mock('../telemetry/index.js', ...) 工厂早于新导出存在，导致两个执行 startNewSession 的测试抛出 mock 缺少导出的错误。修复：在该 mock 工厂中补上 logSessionEnd: vi.fn()（其余 11 个失败的 core 测试文件在基线上同样失败，属既有环境问题，与本 PR 无关）。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const previousSessionId = this.sessionId; | ||
| logSessionEnd(this); | ||
| this.sessionId = sessionId ?? randomUUID(); |
There was a problem hiding this comment.
[Suggestion] Untested-wiring pattern (1/6 — the end-before-swap ordering): the efficacy probe (harness validated) reverted this hunk on its own and every affected test stayed green; 6 review agents independently concur the lifecycle wiring has no test. Sibling locations: the continuation gate below (~line 3845), the config.ts import (~132), the telemetry/index.ts export (~35), the loggers.ts import (~143), the sdk.ts shutdown emission (~144) — Failure scenario: a future refactor moving logSessionEnd(this) below the sessionId reassignment ends the NEW session instead of the outgoing one on every /clear//resume//branch, and every test stays green. Fix: add a startNewSession test asserting session.end carries the pre-swap id and is emitted before the swap.
中文说明
未测试的接线（模式 1/6——先结束再换 id 的顺序）：测试有效性探针（harness 已验证）单独回退该代码块后所有测试仍然通过；6 个审查 agent 独立确认生命周期接线无测试。同类位置：续接门控（约 3845 行）、config.ts 导入（约 132 行）、telemetry/index.ts 导出（约 35 行）、loggers.ts 导入（约 143 行）、sdk.ts 关闭时发射（约 144 行）。修复：新增 startNewSession 测试，断言 session.end 携带换 id 之前的旧 id 且先于换 id 发出。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const currentSessionId = getCurrentSessionId(); | ||
| if (currentSessionId) { | ||
| emitSessionEnd(currentSessionId); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Untested-wiring pattern (6/6 — this shutdown emission): deleting this block, or moving it below currentSdk.shutdown(), leaves the whole suite green — every session would be left unterminated at process exit (or the record emitted into an already-shut-down SDK and dropped), defeating the design doc's "Telemetry shutdown ends the currently active session" guarantee. sdk.test.ts has extensive shutdown coverage to host this — Fix: initialize telemetry, establish a session context, spy emitSessionEnd, assert it is called with the current session id before NodeSDK.prototype.shutdown; plus a no-session-context case asserting no emission.
中文说明
未测试的接线（模式 6/6——关闭时的发射）：删除该代码块或把它移到 currentSdk.shutdown() 之后，整个测试套件仍然通过——进程退出时所有会话都不会被结束，设计文档"遥测关闭时结束当前活跃会话"的保证形同虚设。sdk.test.ts 已有大量 shutdown 覆盖可承载该测试。修复：初始化遥测、建立会话上下文、spy emitSessionEnd，断言其在 SDK shutdown 之前以当前 session id 被调用，并补充无会话上下文时不发射的用例。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| const attributes: LogAttributes = { | ||
| 'event.name': EVENT_SESSION_START, | ||
| 'session.id': sessionId, | ||
| ...(previousSessionId ? { 'session.previous_id': previousSessionId } : {}), | ||
| }; |
There was a problem hiding this comment.
[Suggestion] These records omit event.timestamp, which every sibling emitter in loggers.ts sets explicitly (~30 sites, including the cli_config record emitted in the same logStartSession body), and which telemetry.md — edited in this same PR — states is universal: "All log records automatically include event.name and event.timestamp attributes". Nothing adds it downstream (plain BatchLogRecordProcessor) — Concrete cost: session.start/session.end become the only records without the attribute, the doc sentence becomes false, and any downstream query/filter keyed on event.timestamp silently drops both events. Apply the same addition in emitSessionEnd below.
| const attributes: LogAttributes = { | |
| 'event.name': EVENT_SESSION_START, | |
| 'session.id': sessionId, | |
| ...(previousSessionId ? { 'session.previous_id': previousSessionId } : {}), | |
| }; | |
| const attributes: LogAttributes = { | |
| 'event.name': EVENT_SESSION_START, | |
| 'event.timestamp': new Date().toISOString(), | |
| 'session.id': sessionId, | |
| ...(previousSessionId ? { 'session.previous_id': previousSessionId } : {}), | |
| }; |
中文说明
新记录缺少 event.timestamp：loggers.ts 中所有同类 emitter 都显式设置该字段（约 30 处，包括同一个 logStartSession 里发出的 cli_config 记录），且本 PR 同时编辑的 telemetry.md 声称"所有日志记录自动包含 event.name 和 event.timestamp"，而管道下游并不会自动补充。代价：这两个记录成为唯一缺少该字段的记录，文档表述变为错误，按 event.timestamp 过滤的下游查询会悄悄漏掉它们。下方的 emitSessionEnd 也需同样补充。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| The existing Qwen-specific `qwen-code.config`/`cli_config` and | ||
| `end_session` records remain available for compatibility. GenAI request spans | ||
| continue to use `gen_ai.conversation.id` for the same owning session ID. |
There was a problem hiding this comment.
[Suggestion] This sentence claims end_session records remain available, but no production code path emits them: EndSessionEvent is instantiated only in qwen-logger.test.ts, and QwenLogger.logEndSessionEvent has zero production callers. It also contradicts the design doc added in this same PR, which names the surviving events as "qwen-code.config / cli_config and RUM session_start" — Concrete cost: an operator following this doc queries their pipeline for end_session records and gets none; the two documents added by this PR disagree about which legacy events survive.
| The existing Qwen-specific `qwen-code.config`/`cli_config` and | |
| `end_session` records remain available for compatibility. GenAI request spans | |
| continue to use `gen_ai.conversation.id` for the same owning session ID. | |
| The existing Qwen-specific `qwen-code.config`/`cli_config` and RUM | |
| `session_start` records remain available for compatibility. GenAI request | |
| spans continue to use `gen_ai.conversation.id` for the same owning session ID. |
中文说明
该句声称 end_session 记录仍然可用，但生产代码没有任何路径发出它：EndSessionEvent 只在 qwen-logger.test.ts 中实例化，QwenLogger.logEndSessionEvent 没有生产调用方。此表述还与本 PR 同时新增的设计文档矛盾（设计文档称保留的是 qwen-code.config / cli_config 和 RUM session_start）。代价：按此文档查询 end_session 的运维人员将一无所获。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| Session lifecycle is also exported through the OpenTelemetry General Session | ||
| semantic conventions. When the OTel logs pipeline is enabled, Qwen Code emits | ||
| `session.start` and `session.end` log events with the required `session.id` |
There was a problem hiding this comment.
[Suggestion] The new LogRecord events are documented only as prose here in the Spans section, while every sibling log event has a bullet entry in the Logs catalog ("The following events are logged:" → "Core Session Events", ~line 556, where qwen-code.config etc. are listed) — Concrete cost: a consumer consulting that catalog to build a dashboard or query finds no session.start/session.end entries and concludes they don't exist. Suggested fix: add bullet entries there (with session.id and optional session.previous_id), keeping only a cross-reference in this section.
中文说明
新的 LogRecord 事件仅以散文形式记录在 Spans 章节，而 Logs 目录（"The following events are logged:" → "Core Session Events"，约 556 行）中每个同级日志事件都有条目。代价：按该目录构建仪表板的用户找不到 session.start/session.end，会以为它们不存在。建议：在目录中补充条目，此处仅保留交叉引用。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| export function logStartSession( | ||
| config: Config, | ||
| event: StartSessionEvent, | ||
| previousSessionId?: string, | ||
| ): void { |
There was a problem hiding this comment.
[Suggestion] This new parameter is supplied at only one of the two logStartSession call sites: the startup path (Config.initialize(), config.ts ~3039) never passes it, so qwen --resume X --fork-session — a persisted continuation that creates a NEW session id, the exact case session.previous_id exists for — emits session.start{session.id: F} with no lineage to source conversation X, while the in-process analogue /branch does emit previous_id for the identical shape — Concrete cost: telemetry consumers cannot link a forked session back to its source conversation; issue #8589's rollover criterion is unmet for the startup fork path. Suggested fix: plumb the fork's source session id through to the initial call (or document startup fork lineage as intentionally untracked).
中文说明
新参数只在两个 logStartSession 调用点之一被传入：启动路径（Config.initialize()）从不传入，因此 qwen --resume X --fork-session——创建新 session id 的持久化续接，正是 session.previous_id 存在的场景——发出的 session.start 不带源会话 X 的血缘信息，而进程内同形态的 /branch 却会发出 previous_id。代价：遥测消费方无法把 fork 会话关联回源会话。建议：把 fork 的源 session id 传入初始调用，或在文档中说明启动 fork 血缘有意不记录。
— qwen3.8-max via Qwen Code /review (v0.21.6)
Verification report — follow-upEnvironment
Reproduction and resultThe sandbox A/B verification identified two edge cases in the previous head:
The follow-up now emits the initial standard start after deferred SDK initialization and suppresses self-referential Tests executed
EvidenceThe new regression tests cover deferred initialization and unchanged-ID continuation at the SDK/Config integration points. This is a non-visual telemetry change, so screenshots are not applicable. 中文验证报告验证报告环境macOS、Node.js 22.x、 复现与结果针对 sandbox A/B 验证发现的两个问题已完成修复：延迟 TUI/ACP telemetry 初始化会补发初始 已执行测试
证据新增 SDK/Config 集成回归测试覆盖延迟初始化和相同 session ID 的恢复场景。这是非 UI 的 telemetry 变更，因此不适用截图。 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Not reviewed: reverse audit — stopped before round 3 by the review time budget. Test Plan (not a blocker): src/telemetry/session-events.test.ts — no such file or directory; src/telemetry/sdk.test.ts — no such file or directory; src/telemetry/loggers.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory.
中文说明
已审查。 未审查：反向审计——评审时间预算不足，未能开始第 3 轮。 Test Plan（非阻断）：src/telemetry/session-events.test.ts — no such file or directory; src/telemetry/sdk.test.ts — no such file or directory; src/telemetry/loggers.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.6)
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: reverse audit — stopped before round 4 by the review time budget. Test Plan (not a blocker): src/telemetry/session-events.test.ts — no such file or directory; src/telemetry/sdk.test.ts — no such file or directory; src/telemetry/loggers.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory.
中文说明
已审查。 建议见行内评论。 未审查：反向审计——评审时间预算不足，未能开始第 4 轮。 Test Plan（非阻断）：src/telemetry/session-events.test.ts — no such file or directory; src/telemetry/sdk.test.ts — no such file or directory; src/telemetry/loggers.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.6)
| it('emits the initial session start after deferred telemetry initialization', async () => { | ||
| const deferredConfig = { | ||
| ...mockConfig, | ||
| isTelemetryInitializationDeferred: () => true, | ||
| } as unknown as Config; |
There was a problem hiding this comment.
[Suggestion] This new test pins only the positive side of the catch-up guard: no test asserts that a NON-deferred initializeTelemetry does NOT emit the catch-up session.start. Measured mutation: removing the if (config.isTelemetryInitializationDeferred?.()) guard in sdk.ts survives the whole relevant suite (sdk.test.ts 64/64 pass with it applied).
Concrete cost: a future refactor that drops the guard double-emits session.start on every eager-path startup (catch-up at settle + Config.initialize's logStartSession once the SDK is up), and no test turns red.
Fix (belongs in an existing non-deferred init test, e.g. 'shares a single in-flight init across concurrent callers'):
expect(emitSessionStart).not.toHaveBeenCalled();中文说明
该新测试只钉住了补发守卫的正向一侧：没有测试断言非延迟（non-deferred）的 initializeTelemetry 不会发出补发的 session.start。实测变异：移除 sdk.ts 中的 if (config.isTelemetryInitializationDeferred?.()) 守卫后，相关套件全部通过（sdk.test.ts 64/64）。
具体代价：未来若重构删除该守卫，每次 eager 路径启动都会双发 session.start（SDK 就绪时补发一次 + SDK 就绪后 Config.initialize 的 logStartSession 再一次），且没有任何测试变红。
修复（应加在既有的非延迟初始化测试中，如 'shares a single in-flight init across concurrent callers'）：expect(emitSessionStart).not.toHaveBeenCalled();
— qwen3.8-max via Qwen Code /review (v0.21.6)
| attribute. A resumed persisted conversation includes `session.previous_id` on | ||
| its `session.start` event. `/clear` and other replacement flows intentionally |
There was a problem hiding this comment.
[Suggestion] This sentence overstates when session.previous_id is emitted. The common /resume case keeps the same session id, and the new same-id guard in Config.startNewSession intentionally suppresses previous_id there (pinned by this PR's own new test); cold-start resumptions (--resume/--continue/--fork-session via Config.initialize) also never carry it.
Concrete cost: an operator building resume-lineage queries on session.previous_id from this doc gets zero rows for ordinary same-id resumes and concludes telemetry is broken or mis-models continuation. The design doc in this same PR states the invariant correctly ("never equal to the new session.id"); this page does not.
| attribute. A resumed persisted conversation includes `session.previous_id` on | |
| its `session.start` event. `/clear` and other replacement flows intentionally | |
| attribute. A resumed persisted conversation includes `session.previous_id` on | |
| its `session.start` event only when the resumed session id differs from the | |
| current one. `/clear` and other replacement flows intentionally |
中文说明
该句对 session.previous_id 的发射条件表述过度。最常见的 /resume 场景保持同一 session id，而 Config.startNewSession 中新增的同 id 守卫在该场景下有意不发 previous_id（本 PR 新增测试已钉住此行为）；冷启动续接（--resume/--continue/--fork-session，经 Config.initialize）同样不携带。
具体代价：按此文档基于 session.previous_id 构建续接谱系查询的使用者，在普通同 id 续接上会得到零行结果，进而认为遥测损坏或对续接建模有误。同一 PR 的设计文档对该不变量表述正确（"永不等于新的 session.id"），本页没有。
— qwen3.8-max via Qwen Code /review (v0.21.6)
Runtime verification report (maintainer review)I built a real end-to-end environment for this PR instead of relying on the unit suite alone: the actual CLI built from this PR's head, the real OpenTelemetry SDK, and two independent capture surfaces — the shipped Bottom line: the feature itself works — Scenario matrix (real runs, both arms)
BEFORE arm (merge-base): zero ✅ What is confirmed workingReal OTLP/HTTP payloads captured off the wire for one interactive session (
{
"timeUnixNano": "1786120488084000000",
"body": { "stringValue": "Session started." },
"attributes": [
{ "key": "event.name", "value": { "stringValue": "session.start" } },
{ "key": "session.id", "value": { "stringValue": "0788fc52-e991-431e-a169-0adbdce821ab" } },
{ "key": "session.previous_id", "value": { "stringValue": "1d289ffd-6a41-46e6-9d3d-cf31b870e4ee" } }
],
"droppedAttributesCount": 0
}
❌ Finding 1 (blocking) — the interactive TUI emits
|
| # | Reverted hunk | Suite | |
|---|---|---|---|
| M1 | session-events.ts — drop the session.previous_id attribute |
1 failed | ✅ killed |
| M2 | config.ts — delete logSessionEnd(this) on session switch |
643 passed | ❌ survived |
| M3 | config.ts — always pass previousSessionId (no gate) |
1 failed | ✅ killed |
| M4 | config.ts — drop only the self-link (ids differ) guard |
1 failed | ✅ killed |
| M5 | config.ts — drop only the sessionData continuation gate |
643 passed | ❌ survived |
| M6 | sdk.ts — delete the deferred-init emitSessionStart |
1 failed | ✅ killed |
| M7 | sdk.ts — delete the shutdown emitSessionEnd |
643 passed | ❌ survived |
| M8 | loggers.ts — delete emitSessionStart in logStartSession |
643 passed | ❌ survived |
| M9 | loggers.ts — make logSessionEnd a no-op |
643 passed | ❌ survived |
Notably: every session.end emission point survives (M2/M7/M9), and the primary session.start wiring survives (M8) — in a PR whose stated goal is start/end symmetry. M5 is the one I'd fix regardless of the rest: deleting the sessionData && gate would make /clear claim continuation, which is the explicit non-goal in the design doc, and it ships green today.
The four killed mutants (M1/M3/M4/M6) map to the tests this follow-up added, and they are genuinely good — they pin exactly the two edge cases it was written for.
Candidate fix (validated on the same harness)
Not a requirement on how to fix it, just evidence that a small change closes both:
--- a/packages/core/src/telemetry/session-events.ts
+++ b/packages/core/src/telemetry/session-events.ts
+/**
+ * Session id whose `session.start` has already been emitted in this process.
+ * The deferred-init fallback in `sdk.ts` and `logStartSession()` can both run
+ * for the same session (the interactive TUI orders `config.initialize()` after
+ * the post-render telemetry init), so the emit must be idempotent per id.
+ */
+let startedSessionId: string | undefined;
+
export function emitSessionStart(
sessionId: string,
previousSessionId?: string,
): void {
+ if (startedSessionId === sessionId) return;
+ startedSessionId = sessionId;
const attributes: LogAttributes = {
export function emitSessionEnd(sessionId: string): void {
+ if (startedSessionId === sessionId) startedSessionId = undefined;
logs.getLogger(SERVICE_NAME).emit({
--- a/packages/cli/src/serve/run-qwen-serve.ts
+++ b/packages/cli/src/serve/run-qwen-serve.ts
getSessionId: () => daemonSessionId,
+ isTelemetryInitializationDeferred: () => true,
isInteractive: () => false,Re-run of the full matrix against the patched build:
Every scenario becomes start=1 end=1, including daemon:<pid>, and /resume still carries session.previous_id (regression control). One caveat worth knowing: session-events.test.ts reuses 'session-2' across two cases with no module-state reset, so the idempotence guard makes the second case fail until the ids are made distinct (or a test-only reset is exported) — that's the fix surfacing a latent test-isolation issue, not a defect in the guard.
Nit (non-blocking)
The OTel General Session convention does not define a body for session.start / session.end; this PR sets "Session started." / "Session ended.". Harmless for OTLP consumers, but worth a conscious decision since the design doc claims strict conformance.
Verdict
Design, semantics and documentation are sound, and the two edge cases this follow-up targets are genuinely fixed and genuinely tested. Finding 1 should be fixed before merge — it is deterministic on the primary interactive path and inverts the semantics of the very event being added. Finding 2 is a small, self-contained gap. Finding 3 is worth at least one wiring-level test for session.end and one for the /clear non-continuation rule.
Reproduction
# arms: PR head 716b243 vs merge-base 41f9b83, built into two packages/core/dist trees
# capture A: shipped file exporter
qwen --telemetry --telemetry-target local --telemetry-outfile /tmp/t.json …
# capture B: real OTLP/HTTP JSON receiver on 127.0.0.1:4318
qwen --telemetry --telemetry-otlp-protocol http --telemetry-otlp-endpoint http://127.0.0.1:4318
# daemon arm
QWEN_TELEMETRY_ENABLED=1 QWEN_TELEMETRY_TARGET=local QWEN_TELEMETRY_OUTFILE=/tmp/d.json \
qwen serve --port 18616 # then SIGTERMScenarios driven in a real pty (tmux) against a local mock OpenAI server; model traffic is irrelevant to the lifecycle records but keeps the session realistic. Pairing audit = group all session.start / session.end records by session.id and require exactly one of each.
中文版报告
PR 8616 真实环境验证报告
我为这个 PR 搭建了完整的真机验证环境，没有只依赖单测：使用由 PR head 实际构建出来的 CLI、真实的 OpenTelemetry SDK，并用两条独立的采集通道交叉验证 —— 仓库自带的 FileLogExporter（--telemetry-outfile），以及一个跑在 127.0.0.1:4318 的真实 OTLP/HTTP 接收端（记录完全一致的线上 payload）。A/B 基线取 PR 的 merge-base（41f9b83，已核对能复现 GitHub 的规范 diff：12 files, +232/−2），单独编译成第二份 packages/core/dist，切换时无需重新构建。
结论： 功能本身是对的 —— session.start / session.end / session.previous_id 都按设计出现在 wire 上，/clear 也正确地不声明 continuation。但有两条生命周期路径在运行时是错的，而且恰好都属于 PR 风险说明里声称已经规避的那一类缺陷。第一条建议合并前修掉。
场景矩阵（真实运行，双臂对比）
| 场景 | deferTelemetryInitialization |
start | end | 次数 | 结论 |
|---|---|---|---|---|---|
headless qwen -p '…' |
false |
1 | 1 | 5/5 | ✅ 正常 |
qwen -i '…' |
false |
1 | 1 | 1/1 | ✅ 正常 |
交互式 TUI（裸 qwen） |
true |
2 | 1 | 10/10 | ❌ session.start 重复 |
/clear（替换） |
— | 1（无 previous_id） |
1 | ✅ | ✅ 正常 |
/resume（续接） |
— | 1（带 session.previous_id） |
1 | ✅ | ✅ 正常 |
| ACP 子进程（daemon 拉起） | true |
1 | 1 | 2/2 | ✅ 正常 |
daemon qwen serve（daemon:<pid>） |
未实现 | 0 | 1 | 2/2 | ❌ 只有 end 没有 start |
BEFORE 臂（merge-base）在所有场景下 session.* 记录数均为 0，证明这套 harness 度量的确实是本 PR 的改动。
✅ 已确认正常的部分
- 每条记录的
event.name+session.id都在，文件导出和 OTLP wire 两条通道一致。 session.previous_id只出现在真正的/resume续接上；/clear发出的是不带previous_id的session.start。这是端到端验证的，不只是emitSessionStart()的单元级别。- 现有的
qwen-code.config、qwen-code.slash_command、gen_ai.*关联字段和 RUM 事件均无变化，所有运行都未观察到回归。 - PR 声明的定向测试套件确实是绿的：643 passed。
❌ 问题 1（建议阻塞合并）—— 交互式 TUI 每个 session 发两次 session.start
10 次交互式运行 10 次复现，在去掉插桩的干净构建上同样复现，并且在 OTLP wire 上可见（相隔 114 ms 的两条记录，session.id 完全相同）。
根因：deferTelemetryInitialization 是 isAcpMode || (interactive && !question)（packages/cli/src/config/config.ts:2201），对 ACP 和交互式 TUI 都是 true，但两条路径里 config.initialize() 相对延迟 telemetry 初始化的顺序不一样：
- ACP ——
initializeTelemetry()在config.initialize()返回之后才跑，此时logStartSession()已被isTelemetrySdkInitialized()挡掉，sdk.ts的兜底是唯一发射点。✅ 这正是 PR 想修的场景，确实修好了。 - 交互式 TUI ——
startPostRenderPrefetches()先排入initializeTelemetry()，AppContainer的 mount effect 之后才 awaitconfig.initialize()，于是两个发射点都触发：sdk.ts:92延迟兜底 →session.startAppContainer→config.initialize()→logStartSession()→loggers.ts:240→ 又一条session.start
不用任何插桩也能看出第二条确实跑了：TUI 的 telemetry 输出里存在 qwen-code.config，而这只有在 logStartSession() 越过 isTelemetrySdkInitialized() 闸门时才可能发生。
影响：任何按 session 计数、或用 start/end 配对算 session 时长的消费端，都会把每个交互式 session 重复计一次 —— 而交互式是主路径。作为对照，qwen -i '…'（同样是 TUI，但 deferred=false）只发一条，说明触发条件就是 deferred 分支。
❌ 问题 2 —— qwen serve 只发 session.end，没有 session.start
createDaemonTelemetryRuntimeConfig()（packages/cli/src/serve/run-qwen-serve.ts:479）自己构造 TelemetryRuntimeConfig，没有实现本 PR 新加的可选方法 isTelemetryInitializationDeferred?()，因此 config.isTelemetryInitializationDeferred?.() 为 undefined，兜底不会触发；daemon 也从不为自己的 daemon:<pid> session 构造 Config，所以 logStartSession() 同样不会跑。而 shutdownTelemetry() 却无条件用 getCurrentSessionId() 发 session.end —— 这个值 daemon 是设置过的。
SIGTERM 后的结果（2/2 复现）：
session.end session.id=daemon:2644179 <- 没有对应的 session.start
这正是 PR 风险说明里写着"避免出现只有 end 没有 start 的 session"的那种情况，只是这次只在 TUI/ACP 路径上被避免了。
⚠️ 问题 3 —— 发射链路的测试覆盖不足
我逐个回滚生产代码 hunk 并重跑 PR 声明的测试套件：
| # | 回滚内容 | 套件 | |
|---|---|---|---|
| M1 | session-events.ts 去掉 session.previous_id 属性 |
1 failed | ✅ 被杀 |
| M2 | config.ts 删掉切换时的 logSessionEnd(this) |
643 passed | ❌ 存活 |
| M3 | config.ts 无条件传 previousSessionId |
1 failed | ✅ 被杀 |
| M4 | config.ts 只去掉自引用（id 不同）守卫 |
1 failed | ✅ 被杀 |
| M5 | config.ts 只去掉 sessionData 续接闸门 |
643 passed | ❌ 存活 |
| M6 | sdk.ts 删掉延迟初始化的 emitSessionStart |
1 failed | ✅ 被杀 |
| M7 | sdk.ts 删掉 shutdown 的 emitSessionEnd |
643 passed | ❌ 存活 |
| M8 | loggers.ts 删掉 logStartSession 里的 emitSessionStart |
643 passed | ❌ 存活 |
| M9 | loggers.ts 把 logSessionEnd 改成空实现 |
643 passed | ❌ 存活 |
值得注意的是：所有 session.end 的发射点（M2/M7/M9）全部存活，主 session.start 链路（M8）也存活 —— 而这个 PR 的核心诉求恰恰是 start/end 对称。M5 我建议无论如何都补一下：删掉 sessionData && 之后 /clear 就会声明 continuation，而这是设计文档里明确写的非目标，今天却能绿着合进去。
被杀掉的四个 mutant（M1/M3/M4/M6）对应的正是这次跟进新增的测试，质量确实是好的 —— 它们精确锁住了这次要修的两个边界。
候选修复（已在同一套 harness 上验证）
不是在规定该怎么修，只是给出"小改动能同时闭合这两点"的证据：session-events.ts 里加一个按 session id 幂等的守卫，run-qwen-serve.ts 里给 daemon 的 runtime config 补上 isTelemetryInitializationDeferred: () => true（共 12 行）。打完补丁重跑整个矩阵，所有场景都变成 start=1 end=1（含 daemon:<pid>），/resume 仍然带 session.previous_id（回归对照）。
一个需要知道的副作用：session-events.test.ts 有两个用例复用了 'session-2' 且没有重置模块状态，所以幂等守卫会让第二个用例失败，直到把 id 改成不同的（或导出一个仅测试用的 reset）—— 这是修复暴露出的既有测试隔离问题，不是守卫本身的缺陷。
小建议（不阻塞）
OTel General Session 约定并未为 session.start / session.end 定义 body，本 PR 设置了 "Session started." / "Session ended."。对 OTLP 消费端无害，但设计文档声称严格遵循约定，值得明确决策一下。
结论
设计、语义和文档都是扎实的，这次跟进针对的两个边界确实修好了、也确实有测试锁住。问题 1 建议合并前修掉 —— 它在主交互路径上是确定性复现的，而且直接推翻了所引入事件本身的语义。问题 2 是个小而独立的缺口。问题 3 建议至少为 session.end 补一个链路级测试，再为 /clear 不声明 continuation 的规则补一个。
Verification reportEnvironment
Reproduction and resultThe maintainer reproduced two lifecycle defects: interactive TUI sessions emitted duplicate This follow-up:
Tests executed
EvidenceTelemetry behavior is non-visual; no new screenshot is needed. The maintainer's real-process report includes file-exporter and OTLP/HTTP evidence and the before/after scenario matrix. 中文验证报告验证报告环境
复现与结果维护者在 10/10 次交互式 TUI 运行中复现了重复 本次跟进：
已执行测试
证据这是非视觉 telemetry 行为，不需要新增截图。维护者的真实进程报告包含文件导出器、OTLP/HTTP 证据及修复前后场景矩阵。 |
🩺 serve daemon A/BBuilt the PR base vs this PR head ✅ No response changes against the PR base across 4 scenario(s). — Qwen Code · serve A/B |
|
@qwen-code /takeover |
qwen-code-dev-bot
added
the
autofix/takeover
|
🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. This is a fork PR, so the first round comes from the next scheduled scan (usually within minutes). Remove the 中文说明🤝 已接管：autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突，直到移除标签或达到轮次上限。本 PR 来自 fork，首轮处理将由下一次定时扫描执行（通常几分钟内）。移除 |
|
🔀 Base updated: red check(s) [label, Test (ubuntu-latest, Node 22.x)] pass on current main — merged current main via update-branch; CI will re-run. 中文说明🔀 已更新 base：红色检查 [label, Test (ubuntu-latest, Node 22.x)] 在当前 main 上通过 —— 已通过 update-branch 合入当前 main，CI 将重新运行。 |
Deep verification —
|
| Cell | Environment | Wire oracle (OTLP JSON @ /v1/logs) |
Result |
|---|---|---|---|
head 2b160e04 |
real Config + real SDK + real exporter | 6 session.* records in exact expected order |
6/6 — 11/11 assertions |
base bb8f2c01 (control) |
same, control tree only | 0 session.* records; 4 qwen-code.config records → probe live |
expected absence — 2/2 |
| head, init-order flipped (SDK first) | same | exactly one session.start per session id (dedupe) |
7/7 |
| base, init-order flipped | same | 0 records; probe live | 2/2 |
trial merge into main |
merge-tree of origin/main + head |
full 6-record lifecycle again | 6/6 — 11/11 |
Head wire sequence:
session.start id=initial-session <- deferred-init catch-up after SDK settle
session.end id=initial-session <- replacement transition
session.start id=replacement-1 <- no previous_id (replacement, not continuation)
session.end id=replacement-1 <- persisted-resume transition
session.start id=resumed-2 prev=replacement-1 <- previous_id only for genuine continuation
session.end id=resumed-2 <- shutdown emission, before SDK teardown
Same-id resume (startNewSession('resumed-2', …) twice) adds no record; session.end always precedes session.start on transitions; every record carries event.timestamp.
Findings
No blocking or major findings. Three notes:
- Environmental, non-blocking — typecheck: one TS7016 (
@lydell/node-pty) inshellExecutionService.ts, a file this PR does not touch. A/A comparison:tsc --noEmiterror set is byte-identical on base and head (logs/tsc-*.log), so it is a pre-existing local dependency-resolution condition; the PR's changed files are type-clean. - By design — disabled SDK emits nothing: both
logSessionEndand thesession.startemit insidelogStartSessionare gated onisTelemetrySdkInitialized(), consistent with all existing events. The gate does not consume the idempotency token (pinned by the newloggers.test.tssuppression test and the init-order harness). - Format: the shipped HTTP chain emits OTLP JSON; all
session.*attributes arrive as string attributes per the semantic-convention shape in the design doc.
Vacuity check (mutation matrix) — new tests are load-bearing
One-line source mutations on the head worktree; each kills exactly the test that pins it (raw output in logs/mutations.log):
| Mutation | Test that goes red | Verdict |
|---|---|---|
M1: session-events.ts — dedupe guard disabled |
does not emit session.start twice for the same session — "expected spy to be called 1 times, but got 2 times" |
pinned |
M2: config.ts — logSessionEnd forced on every startNewSession |
records no lifecycle transition when resuming the current session id — "expected logSessionEnd to not be called at all, but actually been called 1 times" |
pinned |
M3: sdk.ts — settle-time catch-up removed |
emits the initial session start after the SDK settles — "expected spy to be called with arguments: [ 'test-session' ]" |
pinned |
All mutations restored afterwards; each failure names expected-vs-actual, so none failed for a vacuous reason.
Targeted gates
- Focused suite at head (the PR's own test-plan command): 652/652 passed —
session-events5/5,sdk66/66,loggers78/78,config503/503 - ESLint on changed files: clean
- Typecheck: only the pre-existing A/A-identical TS7016 above
- Trial merge into current
main: conflict-free; merged tree re-ran lifecycle 11/11 and the telemetry suite green
Not covered
- Full interactive TUI/ACP startup (deferred-init ordering was driven at the Config+SDK level, both orders, not via a full interactive launch)
- gRPC OTLP protocol (HTTP wire only; gRPC chain untouched by this PR)
- QwenLogger (RUM) internals; repo-wide test suite; per-commit attribution of the 14-commit chain (aggregate head diff verified)
Evidence images
Maintainer-local round; full report, harnesses, and raw logs at tmp/pr8616-verify-20260809-065703/. Advisory evidence for humans — no merge action taken by this round.
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round: no action (PR #8616)No actionable feedback this round — no code changes were made.
No conflicts were reported ( 中文说明Autofix 轮次：无操作（PR #8616）本轮没有需要处理的反馈——未做任何代码改动。
未报告冲突（ Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
…await (QwenLM#8764) * fix(external-context): read the response body with a reader, not for-await Async-iterating a ReadableStream needs [Symbol.asyncIterator] on the TYPE, and whether it is there depends on which lib set the program resolves — @types/node's stream has it, the DOM lib's needs lib.dom.asynciterable. That resolution flipped underneath this file on 2026-08-08: QwenLM#8693 installed @types/jsdom at the root, vitest's types pull the jsdom types in wherever they exist, and jsdom's carry /// <reference lib="dom" />. QwenLM#8693 shipped the tsconfig `types` guard in the same commit, so main stayed green — but the guard travels with the BRANCH while node_modules travel with the TRUSTED BASE in the autofix verification build, so every managed branch behind QwenLM#8693 failed that build with TS2504 on this line. Two legs measured on run 31276008548: 63 minutes of accepted agent work discarded per round, 18 more minutes burned by a repair step that cannot fix a failure outside the PR's diff (QwenLM#8614 reached attempt 13 that way; QwenLM#8616 died identically). Reproduced locally in both directions before changing anything: @types/jsdom installed + guard removed = the gate's exact error, character for character; with the reader loop the same poisoned setup builds clean. The guard stays — belt and suspenders — but the build no longer depends on it, or on which lib set any future environment resolves. Behavior is unchanged and now pinned by tests the file never had: multi-chunk assembly, the exact MAX_RESPONSE_BYTES boundary (bound is strictly-greater), invalid-UTF-8 rejection, and the easy one to drop in this rewrite — cancelling the stream on early exit, which `for await` did implicitly via iterator return(). Mutation-tested: removing the cancel fails exactly that test against an endless producer. The package's other for-awaits iterate process.stdin (a Node stream, async-iterable in every lib set) and are untouched. * fix(external-context): await stream cancellation before rejecting the request On early exit from the reader loop (the oversize throw) cancellation was started fire-and-forget, so postJson() rejected while the stream's teardown was still settling — `for await` had awaited its implicit iterator return() before propagating. An immediate retry could overlap the previous response transport's unfinished cancellation. Await reader.cancel() before releaseLock(), and pin the sequencing with a deferred-cancel regression test that fails against the fire-and-forget form. Also cover read() rejecting after a partial chunk was received: the error maps to the request-did-not-complete transport error rather than EOF-then-parse of the partial JSON, and the reader lock is still released. * fix(external-context): drop the types guard the reader rewrite made obsolete The `"types": ["node"]` override existed solely to keep @types/jsdom's lib.dom out of this program while http-client.ts read the response body with `for await` — the DOM lib's ReadableStream is not async-iterable, and the flip broke the build with TS2504 (QwenLM#8693). The reader loop that replaced the `for await` types identically in every lib set, so the guard is no longer load-bearing: with it removed, lib.dom re-enters the program and the package still builds cleanly. Drop it with its stale comment instead of leaving maintainers two contradicting stories about whether it is needed. Also export MAX_RESPONSE_BYTES and import it in the boundary tests instead of re-declaring it locally, so the tests pin the real constant rather than a copy that can silently drift. * test(external-context): make the invalid-UTF-8 test pin fatal decoding --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
|
🤖 Could not produce a passing fix for this feedback (round 6/100). This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31322652532 🧠 Handled by Qwen Code · model/模型 |
|
🤖 Could not produce a passing fix for this feedback (round 7/100). This item now needs a human; the loop stays engaged and still picks up new feedback and base conflicts, but will not retry this item on its own. What I found before stopping: See the Qwen Autofix agent step logs for model/tool output. Run log: https://github.com/QwenLM/qwen-code/actions/runs/31323352713 🧠 Handled by Qwen Code · model/模型 |
…ession-lifecycle # Conflicts: # packages/core/src/telemetry/loggers.test.ts
|
🤖 Addressed the latest review feedback (round 8/100). What changed, and what I pushed back on: · 已处理最新评审反馈（第 8/100 轮）。改动内容与我反驳保留之处如下： Round summary — PR #8616 (Critical-only mode)This round ran in Critical-only mode after five change-producing rounds. All actionable feedback sections (reviews, inline comments, issue-level comments, failed checks, still-red checks) were empty, and the The round's in-scope work was the requested base-conflict resolution ( Feedback dispositions
Conflict resolution
All other files auto-merged. The seven files touched by both sides ( Resulting merge commit: Verification
中文说明轮次总结 — PR #8616（仅处理 Critical 模式）本轮在完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。所有可执行反馈区域（评审、行内评论、issue 级评论、失败检查、持续失败检查）均为空， 本轮范围内的工作是所请求的与 base 分支的冲突解决（ 反馈处置
冲突解决
其余文件全部自动合并成功。双方都触碰过的 7 个文件（ 产生的合并提交： 验证
Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: conflicted with main — resolved in this push. · 与 main 有冲突——已在本次推送中解决。 Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human. · 有空请复审；第 100 轮后本 bot 停止并将 PR 交给人工。 🧠 Handled by Qwen Code · model/模型 |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally. Not explored to full depth (tool budget reached): This PR adds OpenTelemetry General Session lifecycle even...: did not execute the core test suite to confirm the new tests pass (static review only).. Test Plan (not a blocker): src/telemetry/session-events.test.ts — no such file or directory; src/telemetry/sdk.test.ts — no such file or directory; src/telemetry/loggers.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory.
中文说明
已审查。 建议见行内评论。 未审查：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。 未探索到全部深度（达到工具调用预算）：This PR adds OpenTelemetry General Session lifecycle even...：did not execute the core test suite to confirm the new tests pass (static review only).。 Test Plan（非阻断）：src/telemetry/session-events.test.ts — no such file or directory; src/telemetry/sdk.test.ts — no such file or directory; src/telemetry/loggers.test.ts — no such file or directory; src/config/config.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.21.8)
| config.startNewSession('resumed-session-id', { | ||
| conversation: { messages: [] }, | ||
| } as unknown as ResumedSessionData); |
There was a problem hiding this comment.
[Suggestion] R6-2: The resume-path test asserts only the logStartSession lineage argument and never asserts that logSessionEnd is called for the outgoing session, leaving the resume transition's session.end emission unguarded — probe-verified: gating logSessionEnd on isSessionTransition && !sessionData (dropping the end on every persisted resume) leaves all tests in this file green, and adding the missing assertion flips red against that mutant and green against the original. — Failure scenario: a refactor making that one-line change would ship silently: every /resume of a different persisted session would stop closing the outgoing session's window, leaving an unbalanced session.start/session.end stream — the unterminated-window condition the design doc warns backends about. Fix: mirror the sibling replacement test —
const endedSessionIds: string[] = [];
vi.mocked(logSessionEnd).mockClear();
vi.mocked(logStartSession).mockClear();
vi.mocked(logSessionEnd).mockImplementationOnce((cfg: Config) => {
endedSessionIds.push(cfg.getSessionId());
});
config.startNewSession('resumed-session-id', {
conversation: { messages: [] },
} as unknown as ResumedSessionData);
expect(endedSessionIds).toEqual([outgoingSessionId]);
expect(vi.mocked(logSessionEnd).mock.invocationCallOrder[0]).toBeLessThan(
vi.mocked(logStartSession).mock.invocationCallOrder[0],
);中文说明
该 resume 路径测试只断言了 logStartSession 的谱系参数，从未断言对离任会话调用了 logSessionEnd，resume 切换时的 session.end 发射因此没有任何测试防护——已用探针验证：把生产代码门控改为 isSessionTransition && !sessionData（即恢复持久化会话时丢弃离任会话的 end）后，本文件全部测试仍为绿；补上缺失断言后，该变异红、原始代码绿（双向翻转）。触发场景：一次单行重构即可悄悄改变行为——此后每次 /resume 到不同的持久化会话都不再关闭离任会话的窗口，产生不配对的 session.start/session.end 流——正是设计文档提醒后端防范的"未终结窗口"。修复：对照同组的 replacement 测试补齐——切换前 mockClear() logSessionEnd，用 mockImplementationOnce 捕获 cfg.getSessionId()，断言 expect(endedSessionIds).toEqual([outgoingSessionId])，并加 end 先于 start 的 invocationCallOrder 检查。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round: no action needed (PR #8616)This round's actionable feedback is empty:
The PR is in Critical-only mode after five change-producing rounds. The deferred non-Critical items listed in this round's audit section (the automated reviewer's review No commits were made and the branch was not modified. 中文说明Autofix 轮次：无需处理（PR #8616）本轮可处理的反馈为空：
该 PR 在完成 5 个产生改动的轮次后已进入仅处理 Critical 的模式。本轮审计区域中列出的延后非 Critical 条目（自动化评审器的评审 未产生任何提交，分支未做改动。 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
…eak (QwenLM#8816) * feat(ci): A/B deterministic gate rejections against the pre-round ref A deterministic rejection in the autofix verification gate is only chargeable to the round if the same check passes without the round's commit. The gate charged every red to the fix unconditionally, and run 31276008548 measured what that costs when the premise is false: PR 8614's branch predated QwenLM#8693's tsconfig guard while node_modules came from the post-QwenLM#8693 trusted base, so `npm run build` was equally red at origin/<branch> — 63 minutes of accepted agent work discarded, an 18-minute repair burned on a failure the repair agent is forbidden to touch (it may only amend the round's own fix), thirteen rounds in a row, and the same again on the QwenLM#8616 leg. On rejection the gate now re-runs the failing check at origin/<branch> (the branch as pushed, before the round) in the same environment: - baseline green: today's path exactly — outcome=failed, retryable=true, the repair pass gets its chance. - baseline red too: outcome=failed with preexisting=true and NO retryable. The repair step keys on retryable and is skipped — it cannot reach a failure outside the round's diff by construction — and gate-rejection.md says outright that the branch needs a base update (merge main), which flows into the failure comment as-is. Fail-closed toward today's semantics: any A/B infrastructure problem (missing ref, checkout failure) charges the fix as before, and a restore failure after the baseline run rejects outright since the tree can no longer be trusted. The round's work is still not pushed — this changes the verdict's honesty and cost, not the push policy. Tested by executing the real script in a real two-remote git repo with an npm stub whose failures are keyed by commit SHA: round-caused red (baseline green), pre-existing red (both red), and the untouched green path. Mutation-tested, 3 of 3 caught: skipping the A/B, claiming pre-existing without measuring, and dropping the tree restore. * Address review: bound the A/B to checks it can honestly compare All seven findings verified before fixing; the three Criticals were each a way the A/B compared something other than the check that failed. R1-1 — the contracts check feeds on stdin, which its first run drains; the baseline leg re-ran against EOF and checked an empty file list. R1-3 — the schema check's verdict rides on packages/core/dist, which the core-rebuild guard built from ROUND sources and which, being gitignored, survives the detach. Both checks are now A/B-exempt (run_check_no_ab): their baseline verdicts prove nothing, and their rejections stay where the repair agent can actually act on them. R1-2 — a workspace the round ADDS does not exist at the baseline, and npm exits 1 there with "No workspaces found" (measured; --if-present forgives a missing script, not a missing workspace) — a round-caused failure misread as pre-existing, skipping the one repair that can fix the round's own package. The per-package loop now A/Bs only when the workspace exists at origin/<branch>. R1-4 — a chatty PASSING baseline used to flood the tail -c 3000 evidence window and push the actual failure text out of gate-rejection.md, the sole carrier into the repair feedback, the PR comment, and the next round's LAST_REJECTION. The baseline transcript now goes to a side log and only a FAILING tail is merged back, where it is the evidence. R1-5 — the pre-existing paragraph pushed gate-rejection.md past the report's head -c 3500 cap, truncating the closing fence for branch names past 44 characters. Cap raised to 3900, invariant comment updated with the new arithmetic. R1-6 — preexisting=true had no read site. It now flows verify → Finalize verification → the failure report, whose headline swaps the generic gate clause for "PRE-EXISTING failure … needs a base update (merge main)". R1-7 — the no-round-commit guard was unpinned (deleting it kept all tests green). Now exercised through the core-rebuild path, the one A/B-eligible check that runs before the commit gate. Four new behavioral scenarios (chatty baseline, no-commit round, A/B-exempt checks, round-added workspace) plus workflow pins for the forwarding, the clause, and the cap. Mutation-tested, 4 of 4 caught: schema back to A/B (3 tests), guard dropped, side log reverted, no-commit guard dropped. * Address review round 2: A/B only what it can prove, prove what it claims Ten findings across two rounds, each verified before fixing. The three deepest share one lesson: the A/B is only sound for a check whose inputs travel entirely with the git ref, and whose failure it can IDENTIFY, not merely observe. R2-1 — rc=1 at both legs does not make them the same failure: the branch can fail for reason A while the round fails for reason B, and a baseline infrastructure hiccup is a nonzero exit too. Pre-existing now requires a MATCHING failure identity — tsc diagnostics normalized to file + error code (positions shift with the round's edits), compared via comm(1) on a per-check transcript. No diagnostics on either side means identity cannot be established and the round stays charged. R2-2 / R2-7 — gitignored dist survives the detach carrying the ROUND's build, so any dist-consuming check A/Bs reverted sources against round-built artifacts: package tests (channel-base resolved through dist exports) and typecheck (sdk-typescript resolves core's d.ts — probe-verified three-arm flip). Both are now A/B-exempt, as is lint, leaving `npm run build` — the incident class, and the one check that rebuilds its own inputs from the checked-out sources — as the sole A/B candidate. The workspace-existence guard dissolves with it. R2-3 — the fixture inherited the caller's global git config; a failing global pre-commit hook broke all seven cases. The harness now isolates GIT_CONFIG_GLOBAL/SYSTEM for every git child, and the suite is proven green under a deliberately hostile hooksPath. R2-4 — Finalize verification now selects preexisting from the same attempt whose outcome it selects (repair verification included). R2-5 / R2-8 — the "merge main" advice is now conditional at both layers: the script paragraph states the measured fact and hedges the remedy; the report headline uses the compare the step already ran — behind/diverged gets the base-update clause, an up-to-date branch is told its own pre-round code needs attention. R2-6 — the rejection document now sizes its evidence tail against its preamble (floor 500 bytes, total under the 3900-byte render cap), so the closing fence can no longer be truncated off by a long branch name. R2-9 — dissolved by R2-2: package tests no longer A/B, the guard and its uncovered positive branch are gone. R2-10 — the baseline-evidence merge is now pinned: the pre-existing scenario asserts the baseline leg's own failure line (keyed by its SHA) reaches gate-rejection.md. Eight behavioral scenarios; mutation-tested 5 of 5: identity dropped, typecheck re-enrolled, package tests re-enrolled, evidence merge dropped, fixed tail restored. * Address review round 4: sharpen identity, stage the git failures, sync prose Nine findings, all refinements — the design held, the edges did not. Identity now keeps the diagnostic MESSAGE (file + code collide: two unrelated TS2339s in one file compared equal, skipping a repair that could have shipped — probe-reproduced by the review), and the fixture emits a SHIFTED position on the baseline leg so the position strip is load-bearing instead of decorative (deleting the sed survived every test before; it fails one now). vite/esbuild failures still yield an empty signature by design — documented as the fail-closed limit rather than half-widened. The fail_signature assignments take `|| true`: grep exits 1 on the normal no-match case and survives errexit today only because the caller sits in an if-condition — a future unconditional call site would crash the gate verdict-less. The restore-failure branch is now stageable and staged: the baseline leg recreates (untracked) a file the branch tracks, the checkout back refuses, and the test pins retryable-not-preexisting with the 'could not restore' label. Relaxing the branch to `|| true` fails it. Prose synced to the mechanisms that replaced it: the render-cap invariant restates against the dynamic tail budget (the old 3000-based arithmetic would misguide the next retune), the no-round-commit guard comment names the core rebuild (schema/contracts left the A/B last round), the describe wording counts both A/B-eligible builds, and the pre-existing clauses no longer claim "the repair pass was skipped" — with REPAIR_PREEXISTING forwarded, repair may have RUN; they now state the invariant that is true either way: repair may only amend the round's own fix, so it cannot reach this failure. Mutation-tested, 3 of 3 caught: position strip dropped, message dropped from the identity, restore rejection relaxed. * fix(ci): watchdog silent sandbox hangs and reap the containers they leak Four autofix rounds have died the same way (QwenLM#8663 twice, QwenLM#8761 r3, QwenLM#8763 r4): the agent's last output is the sandbox wrapper's "ContainerName (regular): …" line at docker container entry, then nothing — not one event — until the 2-hour absolute budget kills the round. Four different runners, two image versions: systemic, not a bad machine. Where exactly the container wedges is still unknown (that needs docker state on the runner); what is certain from the logs is the shape — a wedged sandbox produces NOTHING, and a legitimate run is never silent for long (the fleet's longest tolerated quiet is the review pipeline's 10-minute stream-idle window for thinking phases). Two mitigations, each aimed at a measured half of the damage: - run-agent.mjs gains an idle watchdog (QWEN_IDLE_TIMEOUT_MS, default 20 minutes = 2x that longest legitimate silence): zero output for the window kills the agent with a distinct "idle-timeout … the sandbox likely hung at startup" detail, so the failure comment names the right knob and a hung round costs 20 minutes instead of 120. Polled, not reset-per-chunk — a busy stream should not spend its time re-arming timers. - Both sandboxed jobs reap stale qwen-code-* containers at job start: a budget kill reaps the HOST-side docker client, not the container, so every killed sandbox keeps running on the persistent runner — observed directly when a later leg's container-name counter found qwen-code-0.21.8-0 already occupied and picked -1. One job per runner at a time makes any container alive at job start stale by definition. Tested by executing the real run-agent.mjs end to end with stub agents: the hang shape (one line, then silence) dies at the idle window naming the idle limit, and a slow-but-talking agent that outputs every 400ms across a 1500ms window survives to a clean exit — the test that distinguishes a watchdog from a disguised absolute timer. Mutation- tested, 3 of 3 caught: watchdog disabled, last-output tracking dropped (the disguised-timer regression), cleanup dropped from a job. * Address review round 5: the gate's verdict defects and the reaper's live kill Budget-warning round — the five Criticals from both reviewers, no suggestions (each deferred with a recorded reply). fail_signature: `[^\n]*` in an ERE bracket expression does not mean "rest of line" — in POSIX bracket expressions `\` is literal, so it matched "neither backslash nor the letter n" and truncated every tsc message at its first n. Nearly every real message has an early n ("Cannot find name", "is not assignable"), so distinct same-file failures collapsed into identical signatures and a round-caused failure could be labeled pre-existing, skipping the repair. grep is line-oriented: `.*` is exactly the rest of the line. New fixture: two messages differing only after their first n. Pre-existing verdict: the intersection test mislabeled in both directions. A round that ADDS a diagnostic sharing one normalized line with the baseline was called pre-existing (repair skipped for a round-caused, repairable failure); and `comm -12 | grep -q` under `set -eo pipefail` SIGPIPEs comm (exit 141) once the shared output outruns the pipe buffer, charging true pre-existing failures to the round — the exact 18-minute repair waste the gate exists to kill. Pre-existing now means the round's failing set is a SUBSET of the baseline's, and the difference is captured before testing. New fixture: a round adding a second diagnostic to a failing baseline. Restore failure after the baseline leg: was retryable=true with HEAD still detached at the baseline commit — the repair agent works in that very checkout and does no git recovery, so its commit would land on the baseline and be orphaned. Now rejected non-retryable (reject_fix grows a third arg); the next round starts clean from the trusted checkout. The restoreClash test pins the new semantics. Stale-container reap: the premise "a runner runs one job at a time, so any live qwen-code-* container is stale" holds per runner registration, but the filter queries the docker daemon, which is per host — and this pool runs several registrations on one OS. With per-issue/PR serialization only, a concurrent job's sandbox is a substring match away from `docker rm -f`. The reap now takes only provably-dead containers (--filter status=exited/dead, both jobs) and the comment says why a running one is left alone. Preamble printf: the `\`` escapes sat inside a single-quoted format where backslash is literal, so every pre-existing rejection rendered raw backticks instead of code spans (shellcheck SC2016). Backticks need no escaping there. Also syncs the side-log comment to the dynamic tail_budget it actually renders. Verified: scripts suite 140/140 (was 138; the two new fixtures and the rewritten restoreClash test all fail against the pre-fix script), npm run build / typecheck / lint pass, bash -n clean. * Address review round 6: reap the kill's own orphan, tolerate the reaper * Address review: hang-bound the reaper, unblock the kill path, pin the unpinned arms - Wrap every docker call in the stale-container reap with timeout 30: an alive-but-wedged daemon blocks docker ps indefinitely, and the existing || guards only catch nonzero exits, not hangs (R3-1). - Make the kill-path container removal async in run-agent.mjs: the spawnSync blocked the event loop between SIGTERM and the 10s SIGKILL backstop for up to its 30s timeout — in exactly the wedged-daemon scenario the watchdog exists for. The main flow awaits the removal so the leak warning stays deterministic (R3-6). - Split the pre-existing gate clause for an empty CMP_R: a transient compare-API failure is "never measured", not "measured not-behind", and must not assert the branch's own code is at fault (R3-7). - Swap the timeout breaker's closing remedy to the sandbox investigation when every counted timeout was idle, mirroring the round-level split (R3-11). - Tests: pin the budget kill path separately from the idle kill path (R3-3), parameterize the idle-window parse guard over -1/0/NaN (R3-5), add a stderr-only liveness case (R3-12), pin the strict-subset A/B arm via a baseline-superset fixture knob (R3-15), and pin the breaker's current-round idle increment (R3-18). --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-ci-bot <qwen-code-ci@service.alibaba.com> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 101 passed · 0 failed · 101 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：101 通过 · 0 失败 · 101 总计 Verification report<!-- qwen-triage:verify --> Sandboxed verification: ✅ passed (agent verdict) — 中文 — 判定：✅ 通过（agent 判定）
Verification reportPR 8616 (follow-up round) — feat(telemetry): align session lifecycle with OpenTelemetryVerdict: Previous-round finding status
Declined/deferred rows were re-measured, not diffed: every cell above was rebuilt and re-run at the new head; the input closure for the legacy-count comparison (loggers.ts legacy path, file-exporters, sdk-impl local target) is unchanged by this PR's diff except the additive Central claim + A/BCentral claim: with the OTel pipeline enabled, every session lifecycle transition emits standard General-Session LogRecords — Harness
Witnesses: Per-arm assertions included: initial start exactly once with no Reviewer Test Plan, step by step
Mutation matrix (vacuity + round-1 gap re-measure)Unmutated control green (658/658). Each mutant applied to a scratch edit, intended killer file run, restored via
6/6 killed, 0 survivors. The two round-1 coverage gaps (M2/M3 rows) are closed by the PR's new tests; attribution is exact (each mutant's first red is the test named for that behavior). FindingsNone new. Two observations, classified as non-findings with bounds:
Not covered
MethodologyEnvironment: CI verify container ( Evidence images
— Qwen Code · sandboxed verification Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Thanks for the PR, and for working through the many review rounds. Full gate re-run on Template ✓ — all required sections present, bilingual summary included. Problem: real and scoped. Linked issue #8589 (triaged with Direction: aligned. Qwen Code already invests in OTel (OTLP exporters, Size: core paths touched. Production logic 95 lines ( Approach: scope feels right. Additive records through the existing logger idiom, an idempotency guard covering the deferred-init catch-up, and an honest design doc that defers the daemon/ACP multi-session gap instead of faking coverage. No drive-by changes. The Risk: no Stage 1e high-risk path matches. The critical findings from earlier rounds (self-referential Moving on to code review. 🔍 中文说明感谢贡献，也感谢在多轮评审中的持续迭代。本次是对 模板 ✓ —— 各必需部分齐全，含中文摘要。 问题： 真实且范围明确。关联 issue #8589（已分诊： 方向： 对齐。Qwen Code 已在 OTel 上有持续投入（OTLP exporter、 规模： 触及核心路径。生产逻辑 95 行（ 方案： 范围合理。通过现有 logger 惯用法做增量记录，用幂等守卫覆盖延迟初始化的补发，设计文档诚实地将 daemon/ACP 多 session 缺口延后处理而不是假装覆盖。无夹带改动。 风险： Stage 1e 高风险路径无命中。前几轮的关键问题（自引用 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent proposal for this issue was: a small emitter module, wired at the single session-rotation choke point ( What I verified against the diff and the surrounding code:
No critical blockers, no convention violations. The outstanding non-Critical suggestions in the thread (naming symmetry The lifecycle flow, since emission ordering was the crux of the earlier criticals: sequenceDiagram
participant P1 as Session switch or startup
participant P2 as Config and SDK lifecycle
participant P3 as session-events guard
participant P4 as OTel logs pipeline
P1->>P2: startNewSession or initializeTelemetry
P2->>P3: end outgoing id (real id change only)
P2->>P3: start new id (previous_id on persisted continuation)
P3-->>P3: per-id token dedupes the settle catch-up race
P3->>P4: session.end then session.start LogRecords
P1->>P2: shutdownTelemetry
P2->>P3: end the current session context id
P3->>P4: session.end LogRecord
Files changed (13)
Testing evidenceEvidence carried: the PR's own CI on the reviewed commit, quoted below (unattended run — no PR code executed here). The macOS/Windows matrix legs and the integration suite are skipped on this fork PR, consistent with every earlier commit on this branch; the ubuntu unit leg is the one that runs and it is green.
On the behavioural claim (the records actually reach the wire): the unit suite pins the wiring but largely against mocked loggers. Two stronger signals exist — @wenshao's mock-free deep verification of the pre-merge head Not verified here: the skipped integration matrix legs (fork gating) — covered by the in-flight verify run rather than by this review. 中文说明代码审查我独立提出的方案是：一个小的事件发射模块，接在唯一的 session 轮换汇聚点（ 已对照 diff 与周边代码核实：所有轮换路径（ 无阻塞项、无规范违规。线程中遗留的非 Critical 建议（命名对称性、抑制路径的 debug 日志、为 ACP 缺口建跟踪 issue）可以合并后再处理。 测试证据本评论携带的是 PR 自身在受审提交上的 CI 结果（无人值守运行，未执行 PR 代码）。macOS/Windows 矩阵与集成测试在该 fork PR 上被跳过（与本分支此前所有提交一致）；实际运行的 ubuntu 单元测试为绿色。 行为性声明（记录真正到达链路）方面：单元测试固定了接线但多用 mock logger。有两个更强的信号——@wenshao 对合并前 head 此处未验证：被跳过的集成矩阵（fork 门控）——由进行中的 verify 运行覆盖，而非本评审。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal, and wire-verified; docking one point only because the integration matrix legs never ran on this fork and I'm relying on the in-flight Stepping back: this is what a good fork contribution looks like. The problem was filed and triaged before any code existed, the design doc is honest about what it defers (the daemon/ACP multi-session gap, cold-start lineage) instead of overclaiming, and the implementation is 95 lines of additive, best-effort telemetry that follows the house idiom to the letter. My independent proposal for this issue and the PR's approach landed on the same shape — small emitter module, single choke point at The history here also matters: five change-producing autofix rounds and three earlier critical findings (self-referential Approving, pinned to the reviewed commit. Two housekeeping notes for the maintainer, neither blocking:
中文说明置信度：4/5 —— 干净、最小化、且已做过线上验证；扣一分仅因为该 fork 的集成测试矩阵分支始终未运行，需要依靠进行中的 整体来看：这是一次高质量的外部贡献。问题在任何代码之前就已被提交并分诊；设计文档诚实地说明了延后项（daemon/ACP 多 session 缺口、冷启动血缘）而非夸大覆盖；实现是 95 行增量式、尽力而为的 telemetry，完全遵循项目惯用法。我对该 issue 的独立方案与 PR 的结构不谋而合——小的发射模块、 历史同样重要：五轮产生改动的 autofix 与三个早期 Critical（自引用 批准，固定在受审提交上。给维护者两条非阻塞的整理备注：
— Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix review round: no action neededNo changes were made in this round. Triage of this round's feedback:
The branch head remains at commit 中文说明Autofix 审查轮次：无需处理本轮未做任何改动。 本轮反馈的分类处理：
分支头部仍为提交 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. (
中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
…await (#8764) * fix(external-context): read the response body with a reader, not for-await Async-iterating a ReadableStream needs [Symbol.asyncIterator] on the TYPE, and whether it is there depends on which lib set the program resolves — @types/node's stream has it, the DOM lib's needs lib.dom.asynciterable. That resolution flipped underneath this file on 2026-08-08: #8693 installed @types/jsdom at the root, vitest's types pull the jsdom types in wherever they exist, and jsdom's carry /// <reference lib="dom" />. #8693 shipped the tsconfig `types` guard in the same commit, so main stayed green — but the guard travels with the BRANCH while node_modules travel with the TRUSTED BASE in the autofix verification build, so every managed branch behind #8693 failed that build with TS2504 on this line. Two legs measured on run 31276008548: 63 minutes of accepted agent work discarded per round, 18 more minutes burned by a repair step that cannot fix a failure outside the PR's diff (#8614 reached attempt 13 that way; #8616 died identically). Reproduced locally in both directions before changing anything: @types/jsdom installed + guard removed = the gate's exact error, character for character; with the reader loop the same poisoned setup builds clean. The guard stays — belt and suspenders — but the build no longer depends on it, or on which lib set any future environment resolves. Behavior is unchanged and now pinned by tests the file never had: multi-chunk assembly, the exact MAX_RESPONSE_BYTES boundary (bound is strictly-greater), invalid-UTF-8 rejection, and the easy one to drop in this rewrite — cancelling the stream on early exit, which `for await` did implicitly via iterator return(). Mutation-tested: removing the cancel fails exactly that test against an endless producer. The package's other for-awaits iterate process.stdin (a Node stream, async-iterable in every lib set) and are untouched. * fix(external-context): await stream cancellation before rejecting the request On early exit from the reader loop (the oversize throw) cancellation was started fire-and-forget, so postJson() rejected while the stream's teardown was still settling — `for await` had awaited its implicit iterator return() before propagating. An immediate retry could overlap the previous response transport's unfinished cancellation. Await reader.cancel() before releaseLock(), and pin the sequencing with a deferred-cancel regression test that fails against the fire-and-forget form. Also cover read() rejecting after a partial chunk was received: the error maps to the request-did-not-complete transport error rather than EOF-then-parse of the partial JSON, and the reader lock is still released. * fix(external-context): drop the types guard the reader rewrite made obsolete The `"types": ["node"]` override existed solely to keep @types/jsdom's lib.dom out of this program while http-client.ts read the response body with `for await` — the DOM lib's ReadableStream is not async-iterable, and the flip broke the build with TS2504 (#8693). The reader loop that replaced the `for await` types identically in every lib set, so the guard is no longer load-bearing: with it removed, lib.dom re-enters the program and the package still builds cleanly. Drop it with its stale comment instead of leaving maintainers two contradicting stories about whether it is needed. Also export MAX_RESPONSE_BYTES and import it in the boundary tests instead of re-declaring it locally, so the tests pin the real constant rather than a copy that can silently drift. * test(external-context): make the invalid-UTF-8 test pin fatal decoding --------- Co-authored-by: verify <verify@local> Co-authored-by: qwen-code-dev-bot <qwen-code-dev@service.alibaba.com>
Real-environment verification — head
|
| # | Scenario | Expected | Observed |
|---|---|---|---|
| 1 | Non-interactive qwen -p |
one session.start + one session.end, same id, no duplicate from the settle-time catch-up |
✅ exactly 2 records |
| 2 | Interactive TUI startup (deferred telemetry init) | initial session.start emitted after SDK settles (the edge case this follow-up fixed) |
✅ emitted once |
| 3 | /clear (A→B) |
end(A) then start(B), no session.previous_id |
✅ end-before-start, no previous_id |
| 4 | /resume A (persisted continuation B→A) |
end(B) then start(A) with session.previous_id=B; idempotency guard re-arms after step 3's end |
✅ previous_id = B |
| 5 | /resume A while A is already active (same-id) |
zero lifecycle records, no self-referential previous_id | ✅ 0 records (slash_command: resume confirms the command ran) |
| 6 | /quit |
shutdown ends the active session before SDK shutdown | ✅ end(A) |
| 7 | Cold start --resume A |
start(A) without previous_id (documented cold-start behavior), end(A) on quit |
✅ |
Before/after A/B
Identical non-interactive run against the same collector, bundle from merge-base 0a3d7bb5 vs PR head: 0 → 2 lifecycle records, and the qwen-code.* event list and counts are byte-identical — the PR is purely additive to the existing telemetry schema.
Gates on PR head
- Focused suite (
session-events,sdk,loggers,configtests, single-thread): 658 passed / 0 failed. npm run typecheck(all workspaces): pass.- CI
Test (ubuntu-latest, Node 22.x): green on this head.
Evidence
| Lifecycle records at the real collector | Before/after A/B |
|---|---|
 |
 |
TUI after /resume (session A history restored) |
Focused suite + typecheck |
|---|---|
 |
 |
Notes
- A TUI process killed hard (SIGKILL, no shutdown path) leaves a
session.startwithout a matchingsession.end— consistent with the documented best-effort semantics; backends should key on (session.id, start-timestamp) windows as the design doc says. - The daemon/ACP multi-session gap is documented as a known limitation in
docs/design/otel-session-lifecycle-design.mdand was not re-tested here (out of scope for this PR).
Verdict: merge-ready. All seven lifecycle scenarios behave exactly as specified against a real OTLP pipeline, and the change is a no-op for existing telemetry consumers.
中文版本（Chinese version）
真实环境验证 — head 33916e2 ✅ 可合并
2026-08-12 维护者验证轮，独立于此前的脚本化验证（我 08-08 的那轮在 autofix 第 6–8 轮之前的旧 head 上，因此本轮对当前 head 做了端到端复验）。
环境
- 隔离 clone 在 PR head
33916e2，真实产线 bundle（npm run bundle→dist/cli.js），Node v24.18.1，macOS。 - 真实 OTLP/HTTP collector（
127.0.0.1:43117）接收真实 logs 管道（telemetry.otlpProtocol: http）——以下所有断言均基于 collector 实际收到的 LogRecord，而非单测 mock。 - OpenAI 兼容 mock 模型服务（SSE 流式），保证真实对话轮次；隔离
QWEN_HOME。
生命周期矩阵（均为 collector 实收记录）
| # | 场景 | 预期 | 实测 |
|---|---|---|---|
| 1 | 非交互 qwen -p |
一对 session.start/session.end，同 id，settle 补发不产生重复 |
✅ 恰好 2 条 |
| 2 | TUI 启动（延迟初始化 telemetry） | SDK 就绪后补发首个 session.start（本次跟进修复的边界） |
✅ 补发且仅一次 |
| 3 | /clear（A→B） |
先 end(A) 后 start(B)，不带 previous_id |
✅ |
| 4 | /resume A（持久化续接 B→A） |
end(B) 后 start(A) 携带 previous_id=B；幂等 guard 在第 3 步 end 后正确重置 |
✅ |
| 5 | A 激活时再次 /resume A（同 id） |
零条生命周期记录，无自引用 previous_id | ✅ 0 条（slash_command: resume 证明命令确已执行） |
| 6 | /quit |
shutdown 在 SDK 关闭前结束当前会话 | ✅ end(A) |
| 7 | 冷启动 --resume A |
start(A) 不带 previous_id（文档化的冷启动行为），退出时 end(A) |
✅ |
Before/After 对照
同一非交互场景、同一 collector，merge-base 0a3d7bb5 的 bundle 对比 PR head：生命周期记录 0 → 2，且 qwen-code.* 事件列表与计数完全一致——对既有 telemetry schema 纯增量、零改动。
门禁（PR head）
- 定向测试套件（
session-events/sdk/loggers/config，单线程）：658 通过 / 0 失败。 npm run typecheck（全 workspace）：通过。- CI
Test (ubuntu-latest, Node 22.x)：本 head 已绿。
备注
- 被强杀（SIGKILL、无 shutdown 路径）的 TUI 进程会留下有
start无end的会话——与文档化的 best-effort 语义一致；后端应按设计文档所述以（session.id，start 时间戳）窗口为键。 - daemon/ACP 多会话缺口已在
docs/design/otel-session-lifecycle-design.md中列为已知限制，不在本 PR 范围内，本轮未复测。
结论：可合并。 七个生命周期场景在真实 OTLP 管道上的行为与设计完全一致，对既有 telemetry 消费方零影响。
|
🤖 Reviewed the latest feedback — no changes needed. Why, point by point: · 已审阅最新反馈——无需改动。逐点说明原因如下： Autofix round — no action taken (PR #8616)This round's feedback contains nothing actionable:
The only issue-level comment is a maintainer real-environment verification report (2026-08-12) for PR head
No code changes were made and no commit was created. The branch remains at 中文说明Autofix 轮次 — 无需处理（PR #8616）本轮反馈没有任何可执行项：
唯一一条 issue 级评论是维护者针对 PR head
未做任何代码改动，未创建提交。分支保持在 Deferred non-Critical feedbackCritical-only mode is active after 5 change-producing rounds. The workflow excluded the non-Critical feedback below from this round's actionable sections; the items remain open for human follow-up. Maintainer feedback is deferred only after its author has used 2 regular feedback batches in this window's Critical-only tail; authors at that budget, if any, are named below. ( 中文说明完成 5 个产生改动的轮次后进入仅处理 Critical 的模式。本轮可执行区域已排除下方非 Critical 反馈；这些条目保持开放，留待人工跟进。维护者反馈仅在其本人于本窗口 Critical-only 阶段已使用 2 批常规反馈预算后才会延后；达到预算的作者（如有）在下方点名。（评论 Base-conflict check · 基分支冲突检查: no conflict with main. · 与 main 无冲突。 🧠 Handled by Qwen Code · model/模型 |
|
Released in v0.21.11. |
4 participants
What this PR does
Adds OpenTelemetry General Session lifecycle events to Qwen Code. Each active session emits a standard
session.startandsession.endLogRecord withevent.nameandsession.id. A resumed persisted conversation also includessession.previous_id. Existing Qwen-specific telemetry events remain unchanged for compatibility.Why it's needed
Qwen Code already correlates GenAI spans with the session through
session.idandgen_ai.conversation.id, but it did not expose standard session lifecycle events. Adding the lifecycle records makes session creation, continuation, replacement, and shutdown observable using the OpenTelemetry General Session semantic conventions.Reviewer Test Plan
How to verify
Review the implementation and design document in
docs/design/otel-session-lifecycle-design.md.Run the focused telemetry/configuration tests:
Confirm that the tests cover:
session.startwithsession.id;session.endwithsession.id;session.startafter deferred TUI/ACP telemetry initialization;session.previous_idonly for a genuine persisted-session continuation;Run the package typecheck, changed-file lint, and Core build commands listed below.
Evidence (Before & After)
Non-user-visible telemetry and documentation change; UI screenshots are not applicable (N/A). Before this follow-up, sandbox A/B verification found two lifecycle edge cases: deferred TUI/ACP initialization could drop the first
session.start, and same-ID resume could emitsession.previous_id === session.id. After this follow-up, the focused suite passes with integration regression tests for both cases, and the emitted lifecycle records remain covered by the existing tests.Tested on
Environment (optional)
Risk & Scope
cli_config/end_sessionand RUM telemetry events are preserved; existingsession.idandgen_ai.conversation.idcorrelation fields are unchanged.session.startafter the SDK becomes ready, avoiding a session with an end but no start.session.previous_idis emitted only whenstartNewSession()receives persisted session data and the incoming session ID differs from the outgoing ID.gen_ai.agent.id, redesigning trace parentage, or changing existing custom telemetry schemas.Linked Issues
Closes #8589
中文说明
本 PR 为 Qwen Code 补充 OpenTelemetry General Session 生命周期事件。每个 session 会发送带有
event.name和session.id的标准session.start/session.endLogRecord；恢复持久化会话时额外携带session.previous_id。现有 Qwen 自定义 telemetry 事件保持不变。本次跟进还修复了两个边界问题：延迟初始化的 TUI/ACP 路径现在会在 SDK 就绪后补发初始
session.start；当恢复的 session ID 与当前 ID 相同时，不再生成自引用的session.previous_id。验证结果
packages/corebuild：通过。风险与范围
保留现有
session.id、gen_ai.conversation.id以及 Qwen 自定义事件；不改变gen_ai.agent.id、trace parentage 或现有自定义 telemetry schema，也不需要数据迁移。