Repository navigation
fix(serve): preserve session creation failure diagnostics - #12331
Conversation
Deterministic creation-diagnostics validationVerified on macOS / Node.js 22.17.0, without a model call.
Evidence boundaries: the installed global CLI ( 中文验证说明macOS / Node.js 22.17.0，无模型调用。最终编译产物完成真实无 prompt 创建（200、 验证了嵌套 cause 身份、清理失败与隔离结果、child/parent 身份分离、缺失/null/非法父 ID 拒绝、回滚不关闭健康兄弟会话，以及 recording 不可用/写未确认/旧版负确认/非法响应/真实本地超时/断连/RPC 拒绝。冷恢复和在线来源补全的 boolean 契约不变。秘密标记不进入新增诊断和 HTTP 投影，telemetry 使用无 cause Error，诊断输出端抛错不替换创建失败。 Build、typecheck、bundle、改动文件 ESLint/Prettier 和 2,055 项回归测试通过。全局旧版 CLI 不支持该路由（404），失败验收使用真实源码组合及注入的 transport/storage 边界；bridge 超时/断连使用真实内存 ACP transport。不据此宣称部署事故根因、真实文件系统故障、provider 推理或 Windows/Linux 验收。 |
|
我看了 session creation failure 的 error propagation 和 source persistence classification，这个设计把 phase / dispatch state / cleanup outcome 分开记录，避免了 rollback 后丢失 root cause。一个小问题是未来如果新增更多 wrapper error，cause extraction 是否考虑统一封装，避免新路径遗漏。 |
|
Fixed in
Validation: build, typecheck, bundle, changed-file ESLint/Prettier and 177 focused tests passed; two clean follow-up diff audit passes. Binary fault-injection evidence is separate from source-level regression coverage and does not claim an external provider or real storage-device fault. The capacity-specific safe reason is left as an optional diagnostic enhancement; the existing public capacity payload is unchanged. A generalized wrapper-unwrapping abstraction is deferred until another concrete wrapper requires it, to keep this PR scoped. 中文说明已处理有效评审意见：仅对带创建诊断的错误做无 cause 投射，保留原始安全的 name/code/stack；删除、恢复等其他错误保持原对象。已检查本地 OTel 实现，它不遍历 cause，因此文档将该投射明确为窄范围纵深防护。两条回归用例在旧 head 失败、修复后通过，并断言 cause/私有载荷不进入投射且原错误未被修改。 新增 runtime 获取失败、持久化确认后的 durable validation 失败测试，并在既有定时 child 模型失败回滚用例中增加 model_selection、父子关联、派发和清理结果断言。中英文档同步收窄来源诊断覆盖声明，明确 branch/default 绕过 helper 的路径不在此次改动内。 对真实 dist/cli.js 用隔离 preload 仅拦截指定 session 的 child RPC，分别造成真实等待超时和子进程 transport 关闭；分类由 daemon 原有处理产生，没有注入诊断结果。真实 daemon 日志文件记录 rpc_timeout/transport_closed，以及关联的 source_persistence/dispatched/rolled_back；两次 HTTP 均保持 500 rollback 和 Retry-After:5，没有 prompt RPC 或模型调用。超时场景的健康兄弟会话保持在线（detach 204）；杀死共享 child 必然移除两者的在线注册，断连场景只证明兄弟会话持久数据仍可读（GET 200）且删除成功，不宣称在线连接存活。 Build、typecheck、bundle、改动文件 ESLint/Prettier 和本轮 177 项测试通过，两轮补丁自审干净。二进制故障注入与源码回归分开报告，不宣称外部 provider 或真实存储设备故障验收。容量专用 reason 保留为可选增强，公共 capacity 载荷不变；在出现另一个实际 wrapper 需求前，不引入通用解包抽象。 |
Maintainer verification round 1 (local, Linux aarch64)Verdict: merge-ready — 65/65 scripted assertions passed, 0 failed. Verified head 中文摘要结论:merge-ready(可合并) — 65/65 项脚本断言全部通过,0 失败。验证对象为 PR head A/B 结论(核心声明成立):6 个会话创建失败场景(运行时获取失败、派发前 spawn 失败、来源持久化未确认、父会话校验失败、持久化校验失败→隔离、绑定两次提交失败)在 head 上各自产生恰好一条带目标 sessionId、phase、reason、dispatchState、cleanupOutcome 的关联诊断( 测试非空虚:突变 M1(删除 catch 中的 门禁:merged head 上 发现:无阻塞问题。注记:①main 上 3 个 acpAgent 测试在此 Linux 环境失败(既有问题,建议另行跟进);②PR 描述称 Linux 未测试,本轮在 Linux aarch64 上全部相关门禁通过;③main 已切换 pnpm(#11859 移除了 package-lock.json),merged 树需用 pnpm 安装——PR 分支本身基于旧 main,合入后无锁文件冲突。 未覆盖:真实 provider 推理与模型调用(按 PR 声明的范围外);Windows 平台; Central claimWhen standalone session creation fails, the failure produces one correlated diagnostic carrying the target session ID, the failed phase, dispatch state and cleanup outcome, while the original cause survives in memory on the error chain — and public HTTP behavior (codes, messages, A/B: merged head vs current main (the PR diff is the only variable)Identical observation harness (
Reviewer Test Plan walk: all four bullets exercised — pre-dispatch + unconfirmed source write through the real route (rows 2–3); direct child against missing parent (row 4); cleanup failure / double binding-commit failure with cause chain and sibling survival (rows 5–6); secret sentinels checked in daemon warn, route warn, trace arguments and HTTP output (rows 2, 5, 6). Test non-vacuity (mutation matrix)Witness:
Unmutated controls green (122/122, 58/58, 51/51 source-filtered), so the kills are attributable. Both mutations reverted afterward; working tree clean. Gates (merged head)Witness:
The 3 FindingsNone blocking. Notes for the record:
Not covered
MethodologyOrange Pi host, Linux aarch64, Node 24.14.0, pnpm 11.24.0. Head tree = PR head Evidence
|
|
@qwen-code /review |
|
Qwen Code review request accepted. Review is queued for an available runner; follow the workflow run for progress. A command-triggered review is not listed under the checks of this PR; the result is posted here as a review when it finishes. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed at e3a66436 (base 615fcfcd). Approving — no P0/P1 found.
Verified the change is diagnostics-only where it claims to be: the failure control flow (status codes, retryable flags, rollback/quarantine/close outcomes) is preserved branch-for-branch, and what changed is that each terminal error now carries its cause plus a CreationDiagnostic (phase / reason / dispatchState / cleanupOutcome) attached in recordCreationFailure. The parent-validation move into createInternal keeps the same conflict check and now attributes the phase correctly, and the child's own session id stays distinct from the parent-validation error's id.
The leak boundary is the part I checked hardest, and it is done right: the HTTP response payload is unchanged (err.message only), and for telemetry sendBridgeError records a sanitized copy when a creation diagnostic is present — cause and ad-hoc properties are stripped, with a test that pins SECRET_CAUSE/SECRET_PAYLOAD absence. The in-memory cause survives for server-side logs; emitDaemonLog/recordDaemonError/daemonLog.warn are all wrapped best-effort so diagnostics can never break creation. The swallowed-rejection fix in admitInitialPrompt (admissionFailure attached as cause) is a genuine diagnostics win.
Two nits, not worth a roundtrip: the phase = 'parent_validation' assignment is duplicated a few lines apart in createInternal, and client-caused 4xx (e.g. standalone_session_conflict) now also emit daemon-error telemetry — slightly noisy, but harmless. CI green at this head; no review threads.
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
PR 主旨：这是会话创建失败路径的纯诊断增强。给 standalone 会话创建的各阶段（runtime / parent_validation / durable_validation / directory_prepare / spawn_pre_dispatch / spawn_dispatched / source_persistence / model_selection / binding / initial_prompt）打上一次性 CreationAttempt 标注，失败时经 recordCreationFailure 产出一条固定文案的 telemetry（recordDaemonError + emitDaemonLog）与一条 daemonLog.warn，属性是枚举过的白名单标量；原始 cause 只在内存里挂在 Error 上供链式排查，绝不进 telemetry/日志/HTTP。同时给私有 source ack 补了可选 reason（recording_unavailable / write_not_confirmed），bridge 侧把未确认写入分类为 negative_ack/invalid_ack/rpc_timeout/transport_closed/rpc_rejected。公共契约（HTTP 状态码、body、Retry-After: 5、sourcePersisted）保持不变。
我按 head 静态复核，结论：无 Critical、无 Important。要点：
- 规范化等价：
parseRequiredSessionId(x).sessionId与normalizeSessionIdForLookup(x)在合法 UUID 上走同一正则+小写；非法 child 仍在createInternal首行抛invalid_request（与旧代码同位置同行为），自冲突判定语义不变，父会话仍是强制校验（commit「retain required parent validation」属实）。 recordCreationFailure只在createInternal的 catch 里触发，transcript deletion / operation_failed 等路径不经过它，也不会带上 diagnostic。- CI 全绿，与你的实现一致：cause 不外泄在测试里对
SECRET_SENTINEL/SECRET_PARENT/SECRET_PROMPT/SECRET_CAUSE/SECRET_PAYLOAD均有JSON.stringify(...).not.toContain(...)断言。
顺带复核了 ci-bot 的两条疑问，在最终 head 上都不成立：(1)「error-response.ts 把剥 cause 扩大到了每个 500」——其实 safeError 被 err.creationDiagnostic 严格门控，非创建类 500（transcript_deletion_failed / working_directory_recovery_failed）没有 diagnostic，仍原样上报；(2)「runtime / durable_validation / model_selection 三个阶段无测试断言」——本 head 的测试分别断言了这三个 phase。文档里「every unsuccessful source operation」对未走 persistSessionSource 的分支/默认会话略有夸大，属非阻塞的措辞问题，bot 已提，留作后续即可。
APPROVE。
What this PR does: A diagnostics-only enhancement to the standalone session-creation failure paths. A one-shot CreationAttempt is threaded through each phase (runtime / parent_validation / durable_validation / directory_prepare / spawn_pre_dispatch / spawn_dispatched / source_persistence / model_selection / binding / initial_prompt); on failure recordCreationFailure emits one fixed-wording telemetry record (recordDaemonError + emitDaemonLog) plus a daemonLog.warn line whose attributes are allowlisted scalars. The raw cause is kept only in memory, chained onto the Error for post-hoc debugging, and never serialized into telemetry/logs/HTTP. The private source ack gains an optional reason (recording_unavailable / write_not_confirmed), and the bridge classifies unconfirmed writes as negative_ack/invalid_ack/rpc_timeout/transport_closed/rpc_rejected. The public contract (HTTP status codes, body, Retry-After: 5, sourcePersisted) is unchanged.
Static re-review against the head; verdict: no Critical, no Important. Highlights:
- Normalization equivalence:
parseRequiredSessionId(x).sessionIdandnormalizeSessionIdForLookup(x)share the same regex + lowercasing for valid UUIDs; an invalid child still throwsinvalid_requeston the first line ofcreateInternal(same spot/behavior as before), so the self-conflict check keeps its meaning and parent validation remains required (the "retain required parent validation" commit holds). recordCreationFailurefires only fromcreateInternal's catch, so transcript-deletion / operation_failed paths never route through it and never carry a diagnostic.- CI is green, and cause non-leakage is asserted in tests via
JSON.stringify(...).not.toContain(...)over theSECRET_*/SECRET_PAYLOADsentinels.
I also re-checked the two questions raised by ci-bot; neither holds on the final head: (1) "the cause-stripping in error-response.ts now applies to every 500" — the safeError projection is strictly gated by err.creationDiagnostic, so non-creation 500s (transcript_deletion_failed / working_directory_recovery_failed) have no diagnostic and are recorded unchanged; (2) "phases runtime / durable_validation / model_selection are untested" — this head's tests assert all three phases. The design doc's "every unsuccessful source operation" slightly overclaims for the branched-session / default-session writes that don't go through persistSessionSource; non-blocking wording, already flagged by the bot, fine as a follow-up.
APPROVE.
Several earlier merges of main into this branch resolved files with the branch side and silently lost main's changes. They merged without conflicts, so only tests and a line-level audit of each merge found them. - acp-bridge: bring back main's split between bridge.ts, the session control plane and the channel harness (QwenLM#11916), which had been replaced by the pre-split monolith, and port the branch's paired execution engines, receipt validation, Managed Session store binding, prompt admission and teardown reporting into it. Main's runtime stop (QwenLM#12008), shared channel startup and source-persistence classification (QwenLM#12331) work again. - core: restore the credential check that kept a parent API key from being sent to another endpoint, reasoning overrides on user turns, the fixed_policy bypass and permission-flow signal, the container execution guard for in-process agents, the sandbox guard for headless subagents, the omni_recall record subtype, tool span attributes and the auto-mode fallback message rule, and main's Managed Session record validation that the branch's writers never trip. - cli: restore the ACP policy-artifact collection, omni media normalization and structured shell result diagnostics in Session, the concurrent SessionEnd wait, the default factory's child process registry and idle reclaim wiring, and the frozen Hosted Harness contract. - web-shell: restore 50 capacity, footnote and daemon strings in both dictionaries, and keep the deep-linked Connections settings open when project features are unavailable. - packaging: keep main's codeModeHost and bwrap sandbox worker entries in the standalone and npm package lists next to the managed runtime worker.
5 participants
What this PR does
Preserves correlated diagnostics when standalone session creation fails, including the original in-memory cause, failed phase, dispatch state and cleanup outcome. The target child ID remains distinct from a parent-validation error's ID. A dedicated service-boundary record covers HTTP and direct child/side-task callers, while source-persistence diagnostics distinguish private negative acknowledgements, legacy or malformed replies, local timeouts, transport closure and RPC rejection.
Why it's needed
A failure before dispatch and a failure to confirm source persistence currently produce the same rollback response and warnings without the collection request's session ID. Operators cannot distinguish those paths, and rollback or quarantine can discard the initiating cause. This change makes those failures diagnosable without changing public HTTP codes, messages, retry headers or
sourcePersisted, and without logging raw causes or request payloads.Reviewer Test Plan
How to verify
Retry-After: 5, while each produces one dedicated diagnostic with the target session ID and its distinct phase/dispatch state. No prompt is admitted.Evidence (Before & After)
Before: deterministic real route + service + serializer fault injection reproduced identical uncorrelated warnings and a lost cause for distinct creation failures. The installed global CLI is too old for this route (404), so that binary is not claimed as a supported baseline.
After: focused regression tests verify exact HTTP compatibility, distinct correlated diagnostics, cause-chain identity, cleanup failure handling, direct-child identity, throwing sinks and privacy. The source transport tests use an actual unanswered RPC/closed channel rather than matching remote exception text. The initial implementation passed 2,055 focused tests on macOS. The CR follow-up additionally passed 177 focused tests, build, typecheck, bundle and changed-file lint/format checks, and preserves original exception stacks/types while limiting cause-free projection to creation errors. Two complete local diff self-audit passes found no remaining actionable issue; the local review used medium effort and does not replace maintainer review.
Tested on
Environment (optional)
Node.js 22.17.0. Deterministic local fault injection without model invocation; the compiled candidate completed real creation with
sourcePersisted:true, detach and deletion without errors. Failure acceptance also includes actual source-RPC timeout and child transport closure against the bundle, with classifications read from the real daemon log file; details and sibling-survival boundaries are in the follow-up test report.Risk & Scope
Design: English · 简体中文.
Linked Issues
Refs #11944. This PR does not close the four-part tracker.
中文说明
本 PR 的改动
保留 standalone 会话创建失败的关联诊断，包括内存中的原始 cause、失败阶段、派发状态和清理结果。目标 child ID 与父会话校验错误中的 ID 分开记录。服务边界的专用记录覆盖 HTTP 和直接 child/side-task 调用；来源持久化诊断区分私有负确认、旧版或非法响应、本地超时、transport 关闭和 RPC 拒绝。
为什么需要
派发前失败和来源持久化未确认目前返回相同的 rollback 响应，集合请求的 warning 缺少 session ID。运维无法区分两条路径，回滚或隔离还可能丢失最初 cause。本改动让这些失败可诊断，同时保持公共 HTTP code、文案、重试响应头和
sourcePersisted不变，不记录原始 cause 或请求载荷。Reviewer 测试计划
如何验证
Retry-After: 5，但各产生一条带目标 session ID 和不同阶段/派发状态的专用诊断，不准入 prompt。修改前后证据
修改前：真实 route + service + serializer 的确定性故障注入复现了不同创建失败产生相同、无关联 ID 的 warning，且 cause 丢失。全局 CLI 过旧，该路由返回 404，因此不将该二进制作为支持能力的基线证据。
修改后：聚焦回归测试验证精确 HTTP 兼容、不同关联诊断、cause 链引用身份、清理失败、直接 child 身份、抛错输出端和隐私。来源 transport 测试使用真正未响应的 RPC/关闭的 channel，不匹配远端异常文本。初版在 macOS 上通过 2,055 项聚焦测试；CR 修复另外通过本轮 177 项测试、build、typecheck、bundle 和改动文件 lint/格式检查，并保留原始异常栈与类型，只对创建错误做无 cause 投射。两轮完整本地 diff 自审无待处理问题；本地 review 使用 medium effort，不替代维护者评审。
测试平台
环境
Node.js 22.17.0。确定性本地故障注入，无模型调用；编译产物完成真实创建并返回
sourcePersisted:true，随后 detach 和删除均无错误。失败验收还包括对 bundle 注入真实来源 RPC 超时及 child transport 关闭，并从实际 daemon 日志文件读取分类；详情和兄弟会话存活的证据边界见后续测试报告。风险与范围
设计：English · 简体中文。
关联 Issue
Refs #11944。本 PR 不关闭四项 tracker。