test(web-shell): cover restored-history pagination retries - #7657
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: observed need — issue #7084 explicitly tracks deferred test coverage from the #7064 review rounds, and this PR addresses two of the three items listed there (URL-encoding of the Direction: aligned — regression coverage for the pagination contract introduced by #7064, scoped to what the follow-up issue asks for. Size: not applicable — no core paths touched. Both changed files are test files ( Approach: the scope feels right. Two small, focused additions: one new SDK-level test for query encoding, and strengthened assertions on an existing WebUI retry test. No unrelated changes, no scope creep. The third item in #7084 (replay production stamps / edge coverage) is not covered here, but the PR title scopes itself honestly to "pagination retries." Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已确认的实际需求——issue #7084 明确跟踪了 #7064 审查中推迟的测试覆盖，本 PR 覆盖了其中两项（ 方向：对齐——为 #7064 引入的分页契约补充回归覆盖，范围与后续 issue 一致。 规模：不适用——未触及核心路径。两个改动文件均为测试文件。 方案：范围合理。两个小而聚焦的新增：一个 SDK 层查询编码测试，以及对现有 WebUI 重试测试的断言加强。无无关改动，无范围蔓延。#7084 中的第三项（replay 生产标记/边界覆盖）未在此覆盖，但 PR 标题诚实地限定为 "pagination retries"。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code ReviewIndependent proposal: for the two coverage items this PR tackles from #7084, I would (1) add a SDK-level test calling Comparison with the diff: the PR does exactly this. No simpler path missed. Two additions, both clean:
No critical blockers. No convention violations. No unrelated changes. Test ResultsSDK:
|
|
Confidence: 5/5 — clean test-only PR that does exactly what #7084 asked for, nothing more. This is the kind of follow-up that keeps a pagination contract honest. The existing retry test proved the UI recovered from a transient failure, but never checked whether the retry actually sent the same Both additions use the existing test helpers and follow the neighboring tests' structure. 39 lines, two files, zero production changes. All 464 tests in the touched files pass. Nothing to maintain, nothing to curse in six months. 中文说明置信度：5/5——干净的纯测试 PR，完全按照 #7084 的要求补充覆盖，没有多余内容。 这类后续补充让分页契约保持可靠。现有的重试测试证明了 UI 能从瞬态故障中恢复，但从未检查重试是否实际发送了相同的 两个新增都使用现有测试辅助函数，遵循相邻测试的结构。39 行，两个文件，零生产改动。触及文件中的全部 464 个测试通过。 — 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. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / after✅ No screenshot changes against the PR base. Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Maintainer local verification — real test runs on macOSI built and ran this PR locally in isolated worktrees. For a test-only PR the interesting question isn't "does it pass" (CI already answered that) — it's do these assertions actually catch anything? So I ran a mutation matrix: break the production code in eight specific ways and see which side goes red. Verdict: ✅ merge-ready. Both new assertions are load-bearing, each guarding a distinct regression that nothing else in the repo currently catches. One non-blocking note on the This also covers the 1. Baseline, merge simulation, and repo gates
Full disclosure: one unrelated file ( 2. Mutation matrix — are the new assertions load-bearing?Each row breaks production code one way, then runs the affected suite twice: BASE =
Two of eight mutations flip green → red only once this PR is applied — one per new assertion. In both cases the rest of the suite stays green, so each failure points precisely at the contract it guards.
3. The two regressions only this PR catchesM1b — I deliberately kept M3 — I added
4. Notes (none blocking)a.
Merging with b. The SDK test's unique value is narrower than it looks — that's fine. The path-segment half ( c. Environment: macOS 15 (Darwin 24.6.0), Node v22.23.1, isolated git worktrees, PR head 中文说明维护者本地验证 — macOS 上的真实测试运行我在隔离的 git worktree 中本地构建并运行了这个 PR。对于一个纯测试 PR，真正值得问的不是"能不能通过"（CI 已经回答了），而是：这些断言到底能不能抓住东西？ 所以我做了一个变异矩阵：用八种具体方式破坏生产代码，看哪一侧会变红。 结论：✅ 可以合并。 两条新断言都是"承重"的，各自守护一个当前仓库中没有任何其他测试能捕获的回归。关于 这同时也补上了 PR 描述中 1. 基线、合并模拟与仓库门禁
如实说明：有一个无关文件（ 2. 变异矩阵 —— 新断言是否承重？每一行以一种方式破坏生产代码，然后把受影响的套件跑两遍：BASE = 不含本 PR 这 39 行的
八个变异中有两个是"只有应用本 PR 之后才由绿转红"—— 恰好每条新断言各一个。 两种情况下套件其余部分都保持绿色，因此每次失败都精确指向它所守护的那条契约。 3. 只有本 PR 能抓住的两个回归M1b —— 我刻意保持 M3 —— 我在 4. 说明（均不阻塞合并）a.
以 b. SDK 测试的独有价值比表面看起来要窄 —— 但这没问题。 路径段那一半（ c. 环境： macOS 15（Darwin 24.6.0）、Node v22.23.1、隔离 git worktree、PR head |
I have finished the modification，thanks for your reviewing~ |
Post-merge verification — re-ran the real tests on
|
| # | Prior finding | Status now |
|---|---|---|
| (a) | Fixes #7084 over-claims — would auto-close an issue with 2 of 3 items still undone |
✅ Resolved. Body changed to Refs #7084 before merge; #7084 is still OPEN, so the other two items survive. Thanks @jay666mnj. |
| (b) | SDK test's unique value is narrower than it looks (path-segment half already covered) | ✅ Still accurate — and now bounded precisely, see M2b below. |
| (c) | toHaveBeenNthCalledWith(1, …) is redundant but worth keeping |
✅ Unchanged; still no mutation caught by nth(1) alone. |
1. The merge itself
| Check | Result |
|---|---|
| Merged commit | 2049d5082, 2 files, 39 insertions, 0 deletions |
Added lines on main vs added lines on the PR branch |
✅ byte-identical (39 = 39, diff clean) |
Full @qwen-code/sdk suite at merged tip |
✅ 31 files / 1436 passed |
Full @qwen-code/webui suite at merged tip |
✅ 26 files / 368 passed |
prettier --check on both merged files |
✅ clean |
tsc --noEmit (webui) |
✅ clean |
2. Mutation matrix, re-run at the merged tip
Same method as before, now anchored on shipped main: BASE = merged main with this PR's 39 lines reverse-applied, PR = main as shipped. 8 mutations × 2 sides + 4 control runs = 20 real suite runs. Only a BASE-green / PR-red cell proves an assertion adds coverage nothing else provides.
| ID | Production regression simulated | suite | BASE | PR | Verdict |
|---|---|---|---|---|---|
| — | none (control) | sdk | 🟢 297 | 🟢 298 | control |
| — | none (control) | webui | 🟢 171 | 🟢 171 | control |
| M1b | only beforeRecordId loses percent-encoding |
sdk | 🟢 297 | 🔴 1 | UNIQUE ✅ |
| M3 | failure path clears history.beforeRecordId → retry loses boundary |
webui | 🟢 171 | 🔴 1 | UNIQUE ✅ |
| M1 | whole query hand-concatenated (no URLSearchParams) |
sdk | 🔴 2 | 🔴 3 | already covered |
| M2 | sessionId path segment not urlEncode()-d (DaemonClient) |
sdk | 🔴 1 | 🔴 2 | already covered |
| M2b | sessionId path segment not urlEncode()-d (workspace twin) |
sdk | 🔴 1 | 🔴 1 | covered elsewhere |
| M4 | request omits clientId |
webui | 🔴 3 | 🔴 4 | already covered |
| M5 | configured historyPageSize ignored |
webui | 🔴 3 | 🔴 4 | already covered |
| M6 | boundary sent under the wrong key (cursor) |
webui | 🔴 2 | 🔴 3 | already covered |
Both UNIQUE cells reproduce exactly as they did pre-merge, and the failing test in each case is the one this PR touched — not an unrelated test tripping over the mutation:
- M1b →
DaemonClient > getSessionTranscriptPage > encodes a before-record transcript boundary - M3 →
DaemonSessionProvider > keeps transient transcript page failures retryable
3. New this round: M2b, and why the anchor guard earned its keep
Every mutation aborts unless its anchor matches exactly once. That guard fired on M2: the naive anchor matched twice, because `/session/${urlEncode(sessionId)}/transcript${transcriptPageSuffix(opts)}` is built at two sites — DaemonClient.getSessionTranscriptPage (~line 2320) and a workspace-scoped twin (~line 5001) that routes through workspaceJsonRequest. Without the guard I'd have mutated whichever site String.replace hit first and drawn a conclusion about the wrong one.
So I split them and asked the open question: is the workspace twin pinned by anything? Yes — M2b is red on both sides (1 failure each), caught by workspace transcript paging forces direct REST transport. The count stays at 1 on the PR side, i.e. this PR's new test correctly does not fire there. No coverage gap, no follow-up needed — this just bounds finding (b) precisely: the new SDK test's contribution is exactly the beforeRecordId query-parameter encoding on DaemonClient, nothing more and nothing less.
4. Correction to my earlier report
My pre-merge comment disclosed that approval-mode-drift.test.ts failed on both sides in my sandbox, blamed on a stale ajv@6. That was a sandbox artifact, and it does not reproduce here — with a freshly linked dependency tree the full SDK suite is 1436/1436 green. The earlier caveat should be disregarded; there was never anything wrong with that test.
One artifact remains and is genuinely environmental: tsc --noEmit in packages/sdk-typescript reports 4 errors about @qwen-code/qwen-code-core/transcriptRecords and @qwen-code/acp-bridge/transcriptReplay. Those are dist-only subpath exports whose .d.ts files aren't built in my sandbox. I A/B'd it: 4 errors on BASE, 4 errors on PR — identical, in a file this PR never touches. CI is green.
Environment: macOS 15 (Darwin 24.6.0), Node v22.23.1, isolated git worktree at merged main 2049d5082.
中文说明
合并后验证 —— 在 main 上重跑真实测试
这是对我合并前报告的后续。我在重跑期间本 PR 已作为 2049d5082 合入，因此这不再是合并前的把关，而是在 main 上的合并后确认：合并是干净的，两条新断言在合并后的提交上依然"承重"。
结论：✅ 在 main 上确认良好。 无需后续动作。文末有一条对我自己先前报告的更正。
上轮结论 → 在合并后提交上的状态
| # | 上轮结论 | 当前状态 |
|---|---|---|
| (a) | Fixes #7084 过度声明 —— 会自动关闭一个尚有 2/3 项未完成的 issue |
✅ 已解决。 合并前正文已改为 Refs #7084；#7084 仍为 OPEN，另外两项得以保留。感谢 @jay666mnj。 |
| (b) | SDK 测试的独有价值比表面看起来更窄（路径段部分已被覆盖） | ✅ 依然准确 —— 且现在被精确界定，见下文 M2b。 |
| (c) | toHaveBeenNthCalledWith(1, …) 冗余但值得保留 |
✅ 不变；仍没有任何变异是仅靠 nth(1) 捕获的。 |
1. 合并本身
| 检查项 | 结果 |
|---|---|
| 合并提交 | 2049d5082，2 个文件，39 行新增，0 行删除 |
main 上的新增行 vs PR 分支上的新增行 |
✅ 逐字节一致（39 = 39，diff 干净） |
合并后提交上的 @qwen-code/sdk 完整套件 |
✅ 31 个文件 / 1436 通过 |
合并后提交上的 @qwen-code/webui 完整套件 |
✅ 26 个文件 / 368 通过 |
对两个合并文件执行 prettier --check |
✅ 干净 |
tsc --noEmit（webui） |
✅ 干净 |
2. 在合并后提交上重跑变异矩阵
方法与上轮相同，但基准换成已发布的 main：BASE = 将本 PR 的 39 行反向应用后的合并版 main，PR = 已发布的 main。9 个变异 × 2 侧 = 18 次真实套件运行。只有 BASE 绿 / PR 红 的格子才能证明该断言提供了其他测试所没有的覆盖。
| ID | 模拟的生产回归 | 套件 | BASE | PR | 判定 |
|---|---|---|---|---|---|
| — | 无（对照） | sdk | 🟢 297 | 🟢 298 | 对照 |
| — | 无（对照） | webui | 🟢 171 | 🟢 171 | 对照 |
| M1b | 仅 beforeRecordId 丢失百分号编码 |
sdk | 🟢 297 | 🔴 1 | 独有 ✅ |
| M3 | 失败分支清空 history.beforeRecordId → 重试丢失边界 |
webui | 🟢 171 | 🔴 1 | 独有 ✅ |
| M1 | 整个查询串手工拼接（不用 URLSearchParams） |
sdk | 🔴 2 | 🔴 3 | 已被覆盖 |
| M2 | sessionId 路径段未 urlEncode()（DaemonClient） |
sdk | 🔴 1 | 🔴 2 | 已被覆盖 |
| M2b | sessionId 路径段未 urlEncode()（workspace 孪生实现） |
sdk | 🔴 1 | 🔴 1 | 由其他测试覆盖 |
| M4 | 请求遗漏 clientId |
webui | 🔴 3 | 🔴 4 | 已被覆盖 |
| M5 | 忽略已配置的 historyPageSize |
webui | 🔴 3 | 🔴 4 | 已被覆盖 |
| M6 | 边界以错误的键（cursor）发送 |
webui | 🔴 2 | 🔴 3 | 已被覆盖 |
两个"独有"格子与合并前完全一致复现，且每种情况下失败的都是本 PR 改动的那个测试，而不是被变异误伤的无关测试：
- M1b →
DaemonClient > getSessionTranscriptPage > encodes a before-record transcript boundary - M3 →
DaemonSessionProvider > keeps transient transcript page failures retryable
3. 本轮新增：M2b，以及锚点校验为何值得
每个变异都要求锚点恰好匹配一次，否则中止。这个保护在 M2 上触发了：朴素锚点匹配了两次，因为 `/session/${urlEncode(sessionId)}/transcript${transcriptPageSuffix(opts)}` 在两处被构造 —— DaemonClient.getSessionTranscriptPage（约 2320 行）以及一个走 workspaceJsonRequest 的 workspace 作用域孪生实现（约 5001 行）。若没有该保护，我会改到 String.replace 先命中的那一处，并对错误的目标下结论。
于是我把两者拆开，并提出了那个悬而未决的问题：workspace 孪生实现是否被任何测试固定？是的 —— M2b 在两侧都是红（各 1 个失败），由 workspace transcript paging forces direct REST transport 捕获。PR 侧的计数仍为 1，即本 PR 的新测试正确地不会在那里触发。没有覆盖缺口，无需后续动作 —— 这只是精确界定了结论 (b)：新 SDK 测试的贡献恰好是 DaemonClient 上 beforeRecordId 查询参数的编码，不多也不少。
4. 对我先前报告的更正
我在合并前的评论中披露 approval-mode-drift.test.ts 在两侧同样失败，并归因于过旧的 ajv@6。那是沙箱假象，本轮未能复现 —— 在重新链接依赖树后，SDK 完整套件为 1436/1436 全绿。先前那条附注应予忽略，那个测试从来没有问题。
另有一个确属环境性的假象：packages/sdk-typescript 的 tsc --noEmit 报告 4 个关于 @qwen-code/qwen-code-core/transcriptRecords 与 @qwen-code/acp-bridge/transcriptReplay 的错误。它们是仅存在于 dist 的子路径导出，而其 .d.ts 在我的沙箱中未构建。我做了 A/B：BASE 4 个错误，PR 4 个错误 —— 完全相同，且位于本 PR 从未触及的文件中。CI 是绿的。
环境： macOS 15（Darwin 24.6.0）、Node v22.23.1、位于合并后 main 2049d5082 的隔离 git worktree。
|
Released in v0.21.1. |
3 participants
What this PR does
Adds regression coverage for restored-session transcript pagination. It verifies that a
beforeRecordIdboundary is URL-encoded when requesting a transcript page and that retrying a transient initial-page failure preserves that boundary and the session client identifier.Why it's needed
Restored sessions with truncated replay use their first persisted record as the boundary for loading older transcript history. A regression in query encoding or retry state could request the wrong page, skip history, or make the retry inconsistent. These tests preserve the pagination contract introduced by #7064.
Reviewer Test Plan
How to verify
Run the focused SDK and WebUI transcript-pagination tests. Confirm that
record/1is encoded asrecord%2F1in the daemon request URL, then simulate a transient first-page failure and confirm that both the failed request and its retry sendbeforeRecordId: 'record-2',limit: 25, and the active session client identifier.Evidence (Before & After)
N/A - this PR adds non-user-visible regression coverage.
Tested on
Environment (optional)
Windows 11, Node.js v22.11.0, npm 11.12.0.
Risk & Scope
Linked Issues
Refs #7084
中文说明
此 PR 的内容
为已恢复会话的转录分页添加回归覆盖。它验证请求转录页时会对
beforeRecordId边界进行 URL 编码，并验证首次页面请求发生瞬态失败后的重试会保留该边界和会话客户端标识符。为什么需要它
回放被截断的已恢复会话会使用其第一条持久化记录作为加载较旧转录历史的边界。查询编码或重试状态的回归可能请求错误页面、跳过历史记录，或使重试不一致。这些测试保护由 #7064 引入的分页契约。
审阅者测试计划
如何验证
运行聚焦的 SDK 和 WebUI 转录分页测试。确认 daemon 请求 URL 中的
record/1被编码为record%2F1，然后模拟一次瞬态的首页面失败，并确认失败请求及其重试均发送beforeRecordId: 'record-2'、limit: 25和活动会话客户端标识符。证据（前后对比）
不适用 - 此 PR 添加非用户可见的回归覆盖。
测试环境
环境（可选）
Windows 11、Node.js v22.11.0、npm 11.12.0。
风险与范围
关联 Issue
Refs #7084