fix(serve): Harden standalone conversation primitives - #9512
Conversation
🩺 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 |
ec845c4 to
240a196
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
doudouOUC
changed the title
|
@qwen-code /triage |
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen 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: 71 passed · 0 failed · 71 total Flakiness gate: ✅ 3 changed test file(s) x 5 identical rounds, no divergence 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：71 通过 · 0 失败 · 71 总计 抖动门：✅ 3 changed test file(s) x 5 identical rounds, no divergence Verification reportPR #9512 — fix(serve): Harden standalone conversation primitivesVerdict: 中文摘要
ScopeCentral claim (item 4): the integrity-aware JSONL head reader measures its budget in physical non-empty lines, so its fail-closed verdict covers a fixed line prefix even when a glued Secondary claims: (item 3) raced standalone directories are adopted only through a re-inspection even without an expected identity, and first-ever creation reports A/B resultsEnvironment per cell: head = compiled Central claim — JSONL budgets (harness
|
| cell | fixture (count) | base oracle | head oracle | result |
|---|---|---|---|---|
| C1 central | {"i":1}{"i":2}\n{"i":\n (2) |
complete: true — corruption missed |
complete: false |
base defect / head fixed |
| C2 | glued pair + clean + {"i":4}garbage (3) |
complete: true — corruption missed |
complete: false, returns all 4 recovered |
base defect / head fixed |
| C3 | glued pair + clean (1) | 1 record (record budget) | both records of the scanned line | contract split as documented |
| C8 | 3-glued + clean (2) | 2 records | 4 records (whole scan window) | documented tradeoff holds |
| C4–C7, C9 | blanks / clean / short / count=0 / scalar | parity | parity | 5/5 parity |
| P1–P6 | plain reader: glued, zero-record lines, overflow, blanks, scalars, count=1 | parity | parity (slice never truncates) | 6/6 parity |
15/15 cells pass per arm. The PR's own new tests pin exactly this: mutant M1 (revert to record budget) is killed by the 3 budget tests with the behavioral mismatch complete: true vs false (05-mutation-matrix.png).
Consumer-level proof (harness 04-consumer-ab.mjs, witness 04-consumer-ab-base-vs-head.png)
Real SessionService.readCreationMetadataIfReadable driven on a session file whose first 5 physical lines carry 10 glued records and whose line 9 is truncated:
| cell | base | head |
|---|---|---|
| S1 corruption at line 9 behind a full record budget | returns {parentSessionId: 'parent-0001', sourceType: 'default', …} — reads creation metadata from a corrupt transcript |
undefined — fails closed |
| S2 clean head, 12 records in the 10-line scan window | identical metadata | identical metadata (consumer tolerates the wider scan window) |
Raced adoption + status semantics (harness 02-ensure-ab.mjs, witness 02-ensure-ab-base-vs-head.png)
| cell | base | head |
|---|---|---|
| E1 first-ever creation, no expected | recreated (mislabel) |
created |
| E2 raced clean dir, no expected | blind ready, 1 inspection |
ready after re-inspection (2 calls) |
| E6 double-vanish inside the race window | blind ready |
compromised/identity_changed (fail-closed) |
| E3/E4/E5 | parity (wrong-mode rejected at materialize; stale expected → unexpected_identity; vanished known identity → recreated) |
parity |
6/6 per arm. Mutants M4 (restore && expected) and M5 (always recreated) are each killed by the corresponding new tests; M4's failure shows the compromised verdict silently replaced by created on base's guard.
Cause serialization (harness 03-cause-ab.mjs, witness 03-cause-ab-base-vs-head.png)
| cell | base | head |
|---|---|---|
| K1 no cause | 'cause' in e = true, enumerable, in Object.keys |
no own cause key at all |
K3 explicit undefined cause |
class-field leak | no key |
K7 {...e} spread |
leaks cause key |
clean |
| K2/K4/K5/K6 | parity (readable non-enumerable cause; scalar cause; identity; liveIdentityError unwrap seam) |
parity |
7/7 per arm. Mutant M6 (re-add the class field) is killed by the new cause test and by the pre-existing preserves Live filesystem errors… unwrap test — the cause semantics has a live consumer.
Mutation matrix (scripted adjudication 05-matrix-check.mjs, witness 05-mutation-matrix.png)
| mutant | guard | suite | outcome | classification |
|---|---|---|---|---|
| M1 revert integrity budget to records | line budget | core jsonl+corruption | killed 3/55 | load-bearing, pinned |
M2 drop .slice(0, count) |
plain-reader cap | core jsonl | survived 40/40 | dead clause (guards already enforce ≤ count; base without slice is byte-identical on all 6 P cells) — completeness reporting, not a merge condition |
M3 scannedLines > count |
off-by-one | core jsonl | killed 2/40 | pinned |
M4 && expected restore |
race re-inspection | cli | killed 2/29 | pinned |
M5 always recreated |
status mapping | cli | killed 1/29 | pinned |
M6 restore cause field |
options bag | cli | killed 2/29 | pinned (incl. downstream consumer) |
Positive control: unmutated suites green (231 + 29). No mutant regressed killed→survived relative to the PR's intent; the single survivor (M2) is dead code, not a coverage gap.
Targeted gates (witness 06-targeted-gates.png)
packages/core:jsonl-utils.test.ts+sessionService.test.ts+sessionService.corruption.test.ts— 231 passed (40/176/15; PR stated 226, actual 231).packages/cli: both suites — 29 passed, matching the PR's claim.tsc --noEmitforpackages/coreandpackages/cli— clean.
Reviewer Test Plan walk-through
All five steps executable and verified: raced adoption without expected (E2/E6 + M4), status semantics (E1/E5), integrity budget (C1/C2 + S1), plain-reader compatibility (P1–P6 + M2), cause behavior (K1–K7 + M6). No unreachable step.
Findings
No blocking findings. Informational, non-blocking:
- Dead defensive clause —
readLines's.slice(0, count)can never truncate: withbudget='records'the outer line-loop guard and the inner per-record guard makeresults.length ≤ countunconditional. Proven by M2 surviving 40/40 and by base (no slice) producing byte-identical output on every plain-reader cell. The comment overstates what the slice does; the contract it names is enforced by the guards. Leave or delete — behavior is identical either way. - Pre-existing semantic looseness, unchanged —
ensureStandaloneDirectory(id, expected)with a fabricatedexpectedfor a session that never had a directory reportsrecreatedafter creating it (the expected identity is only validated when the directory exists). Identical on base and head; callers are trusted daemon internals and the next inspection surfaces the mismatch. Not introduced by this PR.
Not covered
- Per-commit attribution: the checkout is depth-2; the metadata lists 4 commits but only the PR head is locally reachable (
git rev-list HEAD^1..HEAD^2= 1). The aggregateHEAD^1..HEADdiff was verified; per-commit claims were not individually exercised. - Non-Linux filesystem semantics (inode-less FAT/exFAT/SMB fallbacks) — container is Linux ext4/overlayfs only.
- Whole-workspace build/typecheck — only the two affected workspaces were gated; the PR itself defers the workspace-wide signal to CI.
- PR2A follow-ups: enforcement breadth, lock-key folding, directory race adoption, integrity budget, error serialization #9490 items 1–2 — explicitly out of the PR's scope.
- The base control was compiled out-of-tree with the repo's TypeScript; the full-package base build shows 16–19 pre-existing type errors in unrelated files (dependency-resolution mismatches of this container, e.g.
mime/lite,fdir,ignore,ajv) — none in the tested closures; emit was complete and every base module was runtime-smoke-verified before use.
Methodology
Ran in the CI verify container (node v22.23.2, merge-ref checkout). Head side = the CI-built packages/*/dist; base side = scratch git worktree at HEAD^1 compiled with the same tsc (core full-package emit into the worktree's dist, CLI as a 3-file closure conversation-workspace.ts + conversation-directory-identity.ts + session-id.ts), with the shared per-package node_modules linked in for the consumer harness. Harnesses are plain .mjs files in this directory that import each variant's compiled modules by absolute path and drive real filesystems (mkdtemp homes, chmod/rename races) and the real SessionService; the only instrumentation is at the environment seam (forcing the first inspection verdict, counting inspection calls), leaving the code under test unmodified. Control purity: grep '@qwen-code/' over the tested base closures returns nothing, and the PR leaves package.json/package-lock.json untouched, so both arms share identical external dependencies. Mutants were applied to the HEAD source one at a time via mutate.mjs, suites re-run, and the tree restored (git status --porcelain empty afterwards). Raw per-cell output in logs/; witnesses in evidence/ (01–06).
Flakiness gate log
rounds=5 files=3 skipped=0
file packages/cli/src/serve/conversations/conversation-workspace.test.ts: (cd packages/cli) npx --no-install vitest run ./src/serve/conversations/conversation-workspace.test.ts
file packages/cli/src/utils/conversation-directory-identity.test.ts: (cd packages/cli) npx --no-install vitest run ./src/utils/conversation-directory-identity.test.ts
file packages/core/src/utils/jsonl-utils.test.ts: (cd packages/core) npx --no-install vitest run ./src/utils/jsonl-utils.test.ts
per-file results (P=pass F=fail I=infra-exit, one letter per run):
packages/cli/src/serve/conversations/conversation-workspace.test.ts: PPPPP
packages/cli/src/utils/conversation-directory-identity.test.ts: PPPPP
packages/core/src/utils/jsonl-utils.test.ts: PPPPP
verdict: pass
summary: 3 changed test file(s) x 5 identical rounds, no divergence
--- per-invocation detail (full copy in the artifact) ---
round 1 · packages/cli/src/serve/conversations/conversation-workspace.test.ts: P (exit 0)
round 1 · packages/cli/src/utils/conversation-directory-identity.test.ts: P (exit 0)
round 1 · packages/core/src/utils/jsonl-utils.test.ts: P (exit 0)
round 2 · packages/cli/src/serve/conversations/conversation-workspace.test.ts: P (exit 0)
round 2 · packages/cli/src/utils/conversation-directory-identity.test.ts: P (exit 0)
round 2 · packages/core/src/utils/jsonl-utils.test.ts: P (exit 0)
round 3 · packages/cli/src/serve/conversations/conversation-workspace.test.ts: P (exit 0)
round 3 · packages/cli/src/utils/conversation-directory-identity.test.ts: P (exit 0)
round 3 · packages/core/src/utils/jsonl-utils.test.ts: P (exit 0)
round 4 · packages/cli/src/serve/conversations/conversation-workspace.test.ts: P (exit 0)
round 4 · packages/cli/src/utils/conversation-directory-identity.test.ts: P (exit 0)
round 4 · packages/core/src/utils/jsonl-utils.test.ts: P (exit 0)
round 5 · packages/cli/src/serve/conversations/conversation-workspace.test.ts: P (exit 0)
round 5 · packages/cli/src/utils/conversation-directory-identity.test.ts: P (exit 0)
round 5 · packages/core/src/utils/jsonl-utils.test.ts: P (exit 0)
Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention).
— Qwen Code · sandboxed verification
|
Thanks for the follow-up! Template looks good ✓ Problem: observed, not theoretical. All three defects were reported by @wenshao during the review of #9341 and are tracked in #9490 (items 3, 4, 5) with concrete mechanisms. One of them already touches production: Direction: aligned. This is review-directed follow-up on the standalone isolation primitives merged in #9341, and #9490 explicitly scopes these items as the remaining work. The integrity reader serves session-head classification today; the directory primitives feed PR2B. Size: core path touched ( Approach: minimal — each fix is exactly what #9490 prescribes, the diff carries nothing unrelated, and the plan doc is updated to match the new Risk: no elevated-risk paths matched. Moving on to code review. 🔍 中文说明感谢这个后续 PR！ 模板完整 ✓ 问题：已观测到，不是理论问题。三个缺陷都是 @wenshao 在 #9341 评审中报告、由 #9490（item 3、4、5）追踪的，机制描述具体。其中一项已影响生产路径： 方向：对齐。这是 #9341 合并后、由评审驱动的后续修复，#9490 明确将这些项列为遗留工作。完整性读取器今天就服务于会话头部归类；目录原语将在 PR2B 中使用。 规模：触及核心路径（ 方案：最小化——每个修复都严格对应 #9490 的处方，diff 不含无关改动，计划文档也同步更新为新的 风险：未命中高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewNo blockers. All three fixes do exactly what #9490 prescribed, and I verified the downstream consumers rather than taking the description at face value: Raced directory adoption (item 3). The re-inspection in Integrity vs. record budget (item 4). The shared internal reader gains a Error cause (item 5). Passing cause through the native ES2022 options bag is the textbook fix: no The added and updated tests pin the changed behaviors — traced against the pre-PR logic, the raced-adoption, line-budget, and cause-enumerability tests all fail without the fix — and a compatibility test pins that the plain reader's record-budget contract survives. The updated stream-cleanup expectation is a consequence of the budget change, not an unrelated edit. CI evidence (the PR's own checks on the reviewed commit)No failures. The main unit suite and Serve A/B are still running on this head; everything completed so far is green. The cancelled Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 Verification gapsNot verified: the author's local runs were macOS-only unit tests (their claim, not re-run here); the CI matrix above is the cross-platform signal, and the unit suite for this head is still in flight. Sandboxed verification would settle the behavioural claim end-to-end: Real-scenario (tmux) testing: N/A — internal serve primitives with no user-visible surface, and this is an unattended run, so no PR code is executed here regardless. 中文说明代码审查无阻塞问题。三处修复都严格对应 #9490 的处方，且我逐一核实了下游消费者，而不是只看描述： 竞态目录收养（item 3）。 完整性预算 vs 记录预算（item 4）。 共享的内部读取器新增 错误 cause（item 5）。 通过 ES2022 原生 options bag 传递 cause 是标准修法：未提供 cause 时不存在 新增与更新的测试钉住了行为变化——对照 PR 前逻辑逐条推演，竞态收养、按行预算、cause 可枚举性测试在缺少修复时都会失败——另有一个兼容性测试钉住普通读取器的记录预算契约不变。流清理测试期望值的更新是预算变化的结果，不是无关改动。 CI 证据（该 commit 上 PR 自己的检查）无失败。主单元测试与 Serve A/B 仍在运行；已完成的全部为绿。cancelled 的 （CI 表格见上方机器可读区域） 验证缺口未验证：作者本地仅跑了 macOS 单元测试（作者自述，未在此重跑）；跨平台信号看 CI 矩阵，本 head 的单元测试仍在进行中。沙箱验证可以端到端坐实行为声明： 真实场景（tmux）测试：N/A —— 内部 serve 原语，无用户可见界面；且本次为无人值守运行，无论如何都不会执行 PR 代码。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — every stage checks out and the fixes are exactly the prescribed ones; withholding the last point only because this head's unit suite and Serve A/B are still in flight. This is what follow-up work should look like: three review findings from #9341, each fixed with the minimal prescribed change, each pinned by a test that fails without the fix, and nothing else in the diff. Before believing the "no production consumers" claim I went looking for downstream damage — it holds for the directory primitives, and the integrity reader's single consumer ( Independent-proposal check: my own plan for items 3/4/5 (unconditional re-inspection plus the Verdict: approve, deferred until CI lands green on this commit — the approval instruction travels with the marker below, and the finalize job will withhold it if anything lands red or the head moves. 中文说明置信度：4/5 —— 各阶段均通过，修复也严格对应处方；扣掉的一分只因为该 head 的单元测试与 Serve A/B 尚未出结果。 这是后续修复应有的样子：#9341 的三个评审问题，每一个都用最小处方修复，每一个都有"缺少修复就会失败"的测试钉住，diff 里没有别的东西。在相信"没有生产消费者"之前我先去找了下游影响——目录原语确实如此，完整性读取器唯一的生产消费者（ 独立方案对照：我对 item 3/4/5 的设想（无条件重检加 结论：批准，但等该 commit 的 CI 变绿后生效——批准指令随下方标记发出；若有检查变红或 head 移动，finalize 任务会撤回批准。 — Qwen Code · qwen3.8-max Reviewed at |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
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 — CI landed green after the review. ✅
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. CI is green and the change is a focused followup that closes the leftover items from the standalone-session review:
ensureStandaloneDirectorynow reportscreated(first-ever, no expected) vsrecreated(re-created after a captured identity vanished), and re-inspects the race window even without an expected identity instead of blindly adopting a raced creator's directory.readLinesWithIntegritymeasures completeness against a line budget (not a record budget) so fail-closed coverage does not shrink on}{-glued lines;readLineskeeps its record-budget contract via.slice(0, count).ConversationDirectoryIdentityErrorcarriescauseas a non-enumerable property only when provided, via the Error options bag.
Tests cover each semantic change.
|
Released in v0.21.15. |
3 participants
What this PR does
Fixes items 3, 4, and 5 tracked in #9490 from the review of the standalone conversation isolation primitives:
created, whilerecreatedis reserved for a directory that vanished after its identity was captured.cause: undefined, while an actual cause remains readable and non-enumerable.Why it's needed
These defects were reported during the review of #9341 and deferred to keep that PR bounded; #9490 tracks the follow-up work. The directory ensure and inspection primitives do not yet have production consumers, but PR2B will build directly on their race and status contracts. The integrity reader already gates session-head classification, so its coverage must be deterministic without changing the long-standing behavior of the plain record-limited reader.
Reviewer Test Plan
How to verify
created; recreation after a captured identity disappears reportsrecreated.causekey; with a cause the value is readable but absent fromObject.keysandJSON.stringify.The targeted suites pass locally:
cd packages/cli && npx vitest run src/utils/conversation-directory-identity.test.ts src/serve/conversations/conversation-workspace.test.ts(29 tests) andcd packages/core && npx vitest run src/utils/jsonl-utils.test.ts src/services/sessionService.test.ts src/services/sessionService.corruption.test.ts(226 tests).npm run typecheck --workspace=packages/core, Prettier, ESLint, andgit diff --checkalso pass for the changed scope.Evidence (Before & After)
N/A — non-UI behavioral fixes.
Tested on
Environment (optional)
macOS 26.4.1, Node.js v22.22.3, npm 10.9.8; unit tests only.
Risk & Scope
createdmember, but that primitive has no production consumers yet. The integrity-aware reader can return more records than its numeric line budget when one physical line contains glued records; its single production consumer treats the result as a scan window, while the plain reader retains an explicit record cap.Linked Issues
中文说明
本 PR 做了什么
修复 #9490 中追踪的 item 3、4、5，这些问题来自对 standalone conversation 隔离原语的评审：
created，recreated只用于身份已捕获后目录消失并被重建的场景。cause: undefined，实际 cause 则保持可读且不可枚举。为什么需要
这些缺陷在 #9341 评审期间被报告，为控制该 PR 的范围而推迟，由 #9490 跟踪后续工作。目录 ensure 和 inspection 原语尚无生产消费者，但 PR2B 将直接依赖其竞态和状态契约。完整性读取器已经用于 session head 分类，因此其覆盖范围必须保持确定，同时不能改变普通记录限额读取器的长期行为。
审阅者测试计划
如何验证
created；已捕获 identity 消失后的重建报告recreated。cause键；传入 cause 时值可读，但不会出现在Object.keys和JSON.stringify中。定向测试在本地通过：
cd packages/cli && npx vitest run src/utils/conversation-directory-identity.test.ts src/serve/conversations/conversation-workspace.test.ts（29 个测试）和cd packages/core && npx vitest run src/utils/jsonl-utils.test.ts src/services/sessionService.test.ts src/services/sessionService.corruption.test.ts（226 个测试）。变更范围内的npm run typecheck --workspace=packages/core、Prettier、ESLint 和git diff --check也均通过。证据（Before & After）
N/A —— 非 UI 行为修复。
测试平台
环境（可选）
macOS 26.4.1、Node.js v22.22.3、npm 10.9.8；仅运行单元测试。
风险与范围
created成员，但该原语尚无生产消费者。当一个物理行包含粘连记录时，完整性感知读取器返回的记录数可能超过数值形式的行预算；它唯一的生产消费者将结果视为扫描窗口，普通读取器则继续保留明确的记录上限。关联 Issue