test(core): pin the archived-copy usage salvage in the conflict deletion test - #7604
Conversation
…ion test Post-merge review follow-up on QwenLM#7425: the both-copies-conflict test only asserted the unlink calls, so removing the archived-path salvage (easy to misread as redundant with the active-path one) would pass every existing test while silently re-opening the interrupted-archive data loss. The test now pins the salvage call with the archived path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Thanks for the PR! Template looks good ✓ Problem: observed gap — post-merge review on #7425 identified that the conflict-deletion test only asserts Direction: aligned — test-only follow-up to a merged fix, pinning behavior that was explicitly flagged in review. No product direction concerns. Size: not applicable — 9 lines added to a single test file, 0 production logic lines. Approach: the scope is exactly right — one assertion in the right test, with a comment explaining why it exists. Nothing to cut, nothing missing. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测到的覆盖缺口——#7425 合并后 review 指出冲突删除测试只断言了 方向：对齐——纯测试跟进，固定合并后 review 中明确标记的行为。无产品方向顾虑。 规模：不适用——单个测试文件新增 9 行，0 生产逻辑行。 方案：范围恰好——在正确的测试中补一条断言，附注释说明存在原因。无需删减，无遗漏。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: given the goal "pin the archived-copy usage salvage in the conflict deletion test", I'd add one Diff review: 9 lines — a 4-line comment and a 5-line assertion. The assertion follows the same TestingTest-only PR — verification is running the test suite and confirming the new assertion actually pins the behavior. Full suite (with PR changes): Mutation test — commented out The new assertion catches the regression exactly as intended. Mutation reverted after verification. 中文说明独立方案： 给定目标"在冲突删除测试中固定 archived 副本的 usage salvage"，我会添加一条对 Diff 审查： 9 行——4 行注释 + 5 行断言。断言遵循同一测试中已有的 测试： 纯测试 PR——验证方式是运行测试套件并确认新断言确实固定了行为。全套 121/121 通过。变异测试（注释掉冲突分支中的 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 5/5 — clean across every stage; would merge without hesitation. This is exactly the kind of follow-up that makes a codebase safer over time. The #7425 fix added the archived-copy salvage, but the test only pinned the Nine lines, zero production changes, follows existing patterns, verified end-to-end. Ship it. 中文说明置信度：5/5 — 每个阶段都干净，毫不犹豫地合并。 这正是让代码库随时间推移更安全的跟进。#7425 修复添加了 archived 副本的 salvage，但测试只固定了 九行，零生产改动，遵循既有模式，端到端验证。可以合并。 — Qwen Code · qwen3.8-max-preview 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. ✅
Local verification — real mutation A/B at head
|
Mutation in sessionService.ts |
base test file | PR test file (#7604) |
|---|---|---|
| none (baseline) | ✅ 121 / 121 | ✅ 121 / 121 |
L1364 delete salvage(activePath) |
❌ 1 failed — should remove session file | ❌ 1 failed (already pinned) |
L1373 delete salvage(archivedPath) — the line this PR pins |
❌ 1 failed — …when active and archived copies conflict | |
L1373 pass activePath instead (argument-precision control) |
n/a | ❌ 1 failed |
L1388 delete salvage(archivedPath) (archived-only branch) |
The L1373 row is the whole point of the PR, and it lands: on base the "redundant-looking" deletion ships completely green; with this PR it goes red. The argument-precision control matters too — it rules out a vacuous toHaveBeenCalled()-style assertion: the matcher rejects the call when the active path is passed instead, so it pins the archived path specifically, not merely "the spy ran".
2. The discriminating run, verbatim
The received-args diff is the tell — only the active path reached the salvage spy, so the archived copy (in the interrupted-archive case, the sole holder of the session's usage history) would have been unlinked unsalvaged. Exactly the #7425 edge, exactly the regression a future refactor would have introduced silently.
I also confirmed the failing test is the one that owns the new assertion (sessionService.test.ts:1615, inside should remove both JSONL files when active and archived copies conflict) — not a neighbouring test coincidentally reacting to the mutation. Mock hygiene checks out as well: persistUsageBeforeTranscriptDeletion is mockClear()-ed in beforeEach (test.ts:73), so the assertion cannot be satisfied by a call leaked from an earlier test.
3. Regression scope & hygiene
| Check | Result |
|---|---|
vitest run src/services/sessionService.test.ts |
✅ 121 / 121 |
vitest run src/services (whole dir) |
✅ 50 files, 1775 passed, 1 skipped |
prettier --check on the changed file |
✅ clean |
eslint on the changed file |
✅ clean |
| CI at head | ✅ all reported checks pass |
Two residual gaps (non-blocking, follow-up material)
(a) Ordering is not pinned. The new assertion pins that the archived salvage happens, not that it happens before the unlink. I swapped lines 1373/1374 so removeFileIfExists(archivedPath) runs first — salvage then reads an already-deleted file, i.e. the same data loss in a different shape — and the suite stayed at 121 passed. The sibling test should remove session file does pin this, at test.ts:1455:
expect(salvage.mock.invocationCallOrder[0]!).toBeLessThan(
unlinkSyncSpy.mock.invocationCallOrder[0]!,
);Adding the analogous ordering check here would make the pin match the description's wording ("before deletion") rather than a strict subset of it.
(b) The third call site (sessionService.ts:1388, archived-only branch) remains unguarded — the mutation matrix's last row. The existing test should remove archived session files and both worktree sidecars already exercises exactly that branch, so a mirror of this PR's assertion closes it. I applied and verified it locally:
// in 'should remove archived session files and both worktree sidecars'
expect(
vi.mocked(persistUsageBeforeTranscriptDeletion),
).toHaveBeenCalledWith(
expect.stringContaining(`/chats/archive/${sessionIdA}.jsonl`),
);| Configuration | Result |
|---|---|
| with the mirror, no mutation | ✅ 121 passed — does not over-constrain |
with the mirror, src:1388 deleted |
❌ 1 failed — gap closed |
PR as-is, src:1388 deleted |
Both (a) and (b) are additive and could equally land as a separate follow-up; I would not hold this PR for them.
Reproduce
git fetch origin pull/7604/head:pr-7604
git worktree add --detach /tmp/wt-7604 d8eaf9af
cd /tmp/wt-7604/packages/core
# baseline
npx vitest run src/services/sessionService.test.ts # 121/121
# the discriminating mutation: delete sessionService.ts:1373
# await this.salvageUsageBestEffort(archivedPath);
npx vitest run src/services/sessionService.test.ts # 1 failed (PR test file)
git show 4f7429a0b:packages/core/src/services/sessionService.test.ts \
> src/services/sessionService.test.ts
npx vitest run src/services/sessionService.test.ts # 121/121 (base test file — the gap)中文版本
本地验证 —— 在 head d8eaf9af 上做真实变异 A/B
以维护者身份在 macOS(Darwin 24.6.0)、PR head 的独立 worktree 中验证,vitest 3.2.4。
结论:✅ 可以合并。 描述中的说法完全复现,而且这条断言是有鉴别力的,不是摆设。验证过程中另外发现两个残留缺口,都属于纯增量的后续项,不构成本 PR 的阻塞。
1. 这条新断言真的能抓住它声称要抓的删除吗?
我把 removeSessionFiles() 里三处 salvageUsageBestEffort() 调用点逐个删掉,每种变异都跑两遍 sessionService.test.ts —— 一遍用 base 测试文件(4f7429a0b),一遍用 PR 的(d8eaf9af)。源码变异完全相同,只有测试文件不同。
sessionService.ts 中的变异 |
base 测试文件 | PR 测试文件 (#7604) |
|---|---|---|
| 无(基线) | ✅ 121 / 121 | ✅ 121 / 121 |
L1364 删除 salvage(activePath) |
❌ 1 failed —— should remove session file | ❌ 1 failed(本 PR 之前已被钉住) |
L1373 删除 salvage(archivedPath) —— 本 PR 要钉住的那一行 |
❌ 1 failed —— …when active and archived copies conflict | |
L1373 改传 activePath(参数精度对照组) |
n/a | ❌ 1 failed |
L1388 删除 salvage(archivedPath)(仅归档分支) |
L1373 这一行就是本 PR 的全部意义,结果站得住:在 base 上,那个"看起来冗余"的删除会全绿通过;有了本 PR 就变红。参数精度对照组同样重要 —— 它排除了"等价于 toHaveBeenCalled() 的空断言"这种可能:传 active 路径时匹配器会拒绝,说明它钉住的是归档路径本身,而不只是"spy 被调用过"。
2. 关键那一次运行的原始输出
收到参数的 diff 就是证据 —— 只有 active 路径进了 salvage spy,也就是说归档副本(在归档中断的场景下,它是该 session 用量历史的唯一持有者)会在未被抢救的情况下被 unlink。这正是 #7425 修的那个边缘,也正是未来重构会静默引入的那种回归。
我另外确认了失败的确实是拥有这条新断言的那个测试(sessionService.test.ts:1615,位于 should remove both JSONL files when active and archived copies conflict),而不是相邻测试碰巧对变异有反应。mock 卫生也没问题:persistUsageBeforeTranscriptDeletion 在 beforeEach 里做了 mockClear()(test.ts:73),所以这条断言不可能靠前面测试遗留的调用蒙混通过。
3. 回归范围与规范检查
| 检查项 | 结果 |
|---|---|
vitest run src/services/sessionService.test.ts |
✅ 121 / 121 |
vitest run src/services(整个目录) |
✅ 50 个文件,1775 通过,1 跳过 |
改动文件 prettier --check |
✅ 通过 |
改动文件 eslint |
✅ 通过 |
| head 上的 CI | ✅ 所有已报告的检查通过 |
两个残留缺口(不阻塞,可作后续)
(a) 顺序没有被钉住。 新断言钉住的是归档 salvage 发生了,而不是它发生在 unlink 之前。我把 1373/1374 两行对调,让 removeFileIfExists(archivedPath) 先执行 —— 此时 salvage 读的是一个已被删除的文件,本质上是同一种数据丢失换了个形态 —— 结果整套测试仍然 121 passed。兄弟测试 should remove session file 在 test.ts:1455 是显式钉住顺序的:
expect(salvage.mock.invocationCallOrder[0]!).toBeLessThan(
unlinkSyncSpy.mock.invocationCallOrder[0]!,
);这里补一条同样的顺序断言,才能让"钉住"与描述里的措辞("deletion 之前")完全对齐,而不是只覆盖其中一部分。
(b) 第三处调用点(sessionService.ts:1388,仅归档分支)仍然没有防护 —— 即变异矩阵的最后一行。现有测试 should remove archived session files and both worktree sidecars 恰好就走这条分支,所以照搬本 PR 的断言即可补上。我在本地实际打了这个补丁并验证:
| 配置 | 结果 |
|---|---|
| 加上镜像断言,不做变异 | ✅ 121 passed —— 不会过度约束 |
加上镜像断言,删除 src:1388 |
❌ 1 failed —— 缺口补上 |
PR 现状,删除 src:1388 |
(a) 和 (b) 都是纯增量的,完全可以另开 PR 处理;我不会为它们卡住本 PR。
3 participants
What this PR does
Adds one assertion to the existing
should remove both JSONL files when active and archived copies conflicttest:persistUsageBeforeTranscriptDeletionmust be called with the archived transcript path before deletion.Why it's needed
Post-merge review on #7425 flagged that the conflict test only asserted the
unlinkSynccalls — a future refactor removing the archived-path salvage (easy to misread as redundant with the active-path salvage above it) would pass every existing test while silently re-opening the interrupted-archive usage-loss edge that #7425 fixed. The dedup guard makes the archived call a no-op when the active copy already wrote, so the call itself is always expected.Reviewer Test Plan
How to verify
npx vitest run src/services/sessionService.test.ts(packages/core): 121/121; deleting thesalvageUsageBestEffort(archivedPath)line in the conflict branch now fails this test.Evidence (Before & After)
Test-only change; the pinned behavior itself shipped and was verified in #7425.
Tested on
Risk & Scope
Linked Issues
Follow-up to #7425 (issue #7384).
中文说明
#7425 合并后 review 指出：双副本冲突测试只断言了 unlink 调用——未来若有人把 archived 路径的 salvage 当作冗余删掉，所有既有测试仍会通过，而 #7425 修复的中断归档丢数据边缘会静默复活。本 PR 在该测试补一条断言：删除前必须以 archived 路径调用过 salvage（查重门保证 active 已写时该调用为 no-op，因此调用本身恒为预期）。纯测试改动，121/121。
🤖 Generated with Claude Code