test(cli): teach session-swap telemetry fakes the /branch title prerequisites - #9998
Conversation
…quisites QwenLM#9764 taught /branch to read the outgoing recorder's custom title and the source session's display name before forking. The QwenLM#9844 regression suite's fake environment predates that and lacks both methods, so on main every /branch scenario throws before the fork and three tests fail. Complete the fakes; no production change.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Scope: 1 file changed (test-only); +2 lines, both stub additions in makeFakeEnv(). No production code touched.
Checked:
- Stub return types match real implementations:
- chatRecordingService.ts:1077 getCurrentCustomTitle(): string | undefined (sync) -> mockReturnValue(undefined) correct
- sessionService.ts:3132 getSessionDisplayName(): Promise<string | undefined> (async) -> mockResolvedValue(undefined) correct
- Both stubs placed inside makeFakeEnv()'s sessionService / recorder fakes, consistent with surrounding mocks.
- useBranchCommand.ts:178-184: getSessionDisplayName is only called when sourceCustomTitle === undefined && name === undefined. With the new stub returning undefined, that path still calls through and resolves to undefined, so branch-title derivation falls through to the first-user-prompt fallback exactly as the scenario assertions expect.
- No logic change introduced; the two new stubs eliminate the "not a function" TypeError that crashed the three /branch scenarios once #9764 landed on main alongside #9844.
CI: Test (ubuntu-latest, Node 22.x) still in progress at review time -- ubuntu suite is the ground truth for these tests; macOS/Windows are SKIPPED (fork PR security model). precheck-pr SKIPPED is expected for fork PRs.
Reviewed with AI assistance.
|
Thanks for the quick follow-up! Template looks good ✓ Problem: real, and verified. #9764 (branch-title preservation, merged today 06:47 UTC) taught Direction: aligned — restoring a red unit suite on main is about as clearly in scope as it gets. Test-only; production telemetry code untouched. Size: not applicable — one test file, +2/-0 (0 production lines). Approach: the minimal fix. Two stubs with correct sync/async shapes ( Risk: no elevated risk signals — the only changed file is a Moving on to code review. 🔍 中文说明感谢快速跟进！ 模板完整 ✓ 问题：真实存在，且已核实。#9764（分支标题保留，今天 06:47 UTC 合入）让 /branch 在 fork JSONL 之前先读取切出方 recorder 的自定义标题和源会话的展示名；#9844 的回归套件（06:53 UTC 合入）分支基线早于该改动，其假环境缺这两个方法。在 main 上，假环境会在第一步标题查询处抛 TypeError: ... is not a function——已核对 branch hook 的调用点、测试假环境中缺失的 stub，以及两个 PR 的合入时间线，均与描述吻合。是已观测到的失败，不是理论问题。 方向：对齐——恢复 main 上变红的单测属于最明确的职责范围。纯测试改动，未触碰生产 telemetry 代码。 规模：不适用——单个测试文件，+2/-0（0 行生产代码）。 方案：最小修复。两个 stub，同步/异步形态正确（真实 recording service 的 getCurrentCustomTitle 是同步方法，真实 session service 的 getSessionDisplayName 返回 Promise），均返回 undefined，使各场景保持在预期的"无既有标题 → 回退到首条 prompt"路径。无可裁剪，无夹带改动。 风险：无升级风险信号——唯一改动文件是 .test.ts，未触碰任何与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewTwo lines, and they're the right two lines. The branch hook now reads the outgoing recorder's custom title (sync) and the source session's display name (async) before forking, and this adds exactly those two stubs to the test's fake env. I checked the shapes against the real services: With the stubs in place the fake env covers every method the /branch path touches — recorder: title lookup, finalize, flush; session service: display name, load, fork, rename, remove, and the title-prefix collision query via Testing evidence (this PR's own CI, read via API — PR code not executed here)The decisive check is
Not verified independently: the before/after counts ( 中文说明代码审查两行改动，而且正是需要的两行。branch hook 现在会在 fork 之前读取切出方 recorder 的自定义标题（同步）和源会话的展示名（异步），本 PR 恰好给测试的假环境补上了这两个 stub。已与真实服务核对形态： 补上后，假环境覆盖了 /branch 路径触及的全部方法——recorder：标题查询、finalize、flush；session service：展示名、load、fork、rename、remove，以及经由 测试证据（读取本 PR 自身 CI，未在此执行 PR 代码）决定性检查是 未独立验证：before/after 数字（ — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — a verified red on main, fixed by exactly the two stubs the code path needs, nothing more. Stepping back: this is the cleanest kind of follow-up. The problem is real and independently verified — #9764 added the two title lookups to the branch flow at 06:47 UTC, #9844's regression suite landed six minutes later from a base that predates them, and main's fake env has been missing both methods since. The fix is the minimal one: stub the two methods with The one thing not yet settled is CI itself: the Ubuntu unit job — the check that runs this exact file — was still in flight at review time, so approval is deferred until CI lands green on 中文说明置信度：5/5 —— main 上已核实的红灯，由该代码路径恰好需要的两个 stub 修复，多一分都没有。 退一步看：这是最干净的后续修复。问题真实且已独立核实——#9764 于 06:47 UTC 给 branch 流程加了两个标题查询，#9844 的回归套件六分钟后从早于它的基线合入，main 的假环境从此缺这两个方法。修复就是最小方案：把两个方法 stub 为 undefined，让所有场景保持在断言所预期的"无既有标题 → 从首条 prompt 推导"路径。我在看 diff 之前想到的修法与之完全一致，且不存在更小的版本。未触碰生产代码，stub 形态与真实服务及相邻测试文件的约定一致，diff 不含任何其他改动。 唯一未落定的是 CI 本身：运行该文件的 Ubuntu 单测 job 在审查时仍在执行，因此暂不批准——等 CI 在该提交上变绿后自动批准。若变绿，即可合入。 — Qwen Code · qwen3.8-max Reviewed at |
Carried in from main by the last merge: #9998 added the vi.fn variant of getCurrentCustomTitle to the getChatRecordingService fake while #9994's Live Host restore re-added the plain arrow variant in the same literal — each PR built green alone, the clean merge stacked both keys, and `tsc --build` fails the cli package with TS1117 inside `npm ci`'s prepare step, so this PR's Install dependencies step dies on fresh runners. Keep the vi.fn variant: a JS literal keeps the LAST key, so it is the one every run actually used — behaviour unchanged, suite 10/10. Same one-line fix as #10023 proposes for main; landing it here unblocks this PR's CI without waiting, and the two are byte-identical so neither merge order conflicts.
#9998 re-added getSessionDisplayName and getCurrentCustomTitle to the session-swap telemetry fakes, which already declared both. TS1117 fails tsc --build, breaking every workflow that runs npm run build on main.
…wenLM#10022) QwenLM#9998 re-added getSessionDisplayName and getCurrentCustomTitle to the session-swap telemetry fakes, which already declared both. TS1117 fails tsc --build, breaking every workflow that runs npm run build on main.
|
Released in v0.22.2. |
4 participants
What this PR does
Completes the fake environment used by the session-swap telemetry regression tests added in #9844: the fake recording service and session service gain the two title-related methods the real services have had since the branch-title work — the outgoing recorder's current custom title and the source session's display name. With those stubs in place, the three /branch scenarios drive the intended fork/swap/rollback paths again instead of dying at the first title lookup.
Why it's needed
The branch-title change that landed just before the regression suite taught /branch to read the outgoing recorder's custom title (and fall back to the source session's display name) before forking the JSONL. The regression PR branched from a base without that change, so its fake environment only stubs the recorder's finalize/flush and the session service's fork/load/remove/rename/title-prefix methods. Once both landed on main, every /branch scenario threw a "not a function" error before the fork, and main's CI fails three tests: a failed /branch restores the process-wide aggregate, a successful /branch keeps the replayed usage, and a failure after the UI re-key does not roll back or undo. The /resume scenarios never read those methods, which is why they stayed green. This PR completes the fakes only; no production code changes.
Reviewer Test Plan
How to verify
Run the session-swap telemetry regression file from packages/cli at main to observe the same three /branch failures as CI, then apply this change and rerun: the whole file passes (10/10). The /branch and /resume command suites stay green as well.
Evidence (Before & After)
Before:
Tests 3 failed | 7 passed (10)— identical to main's CI. After:Tests 10 passed (10).Tested on
Environment
N/A — unit tests only; CI covers the remaining platforms.
Risk & Scope
Linked Issues
Follow-up to #9844; the missing fake surface comes from #9764.
中文说明
这个 PR 做了什么
补全 #9844 新增的会话切换 telemetry 回归测试所用的假环境：给假 recording service 和假 session service 补上分支标题功能落地后真实服务早已具备的两个标题相关方法——切出方 recorder 的当前自定义标题、源会话的展示名。补上后，三个 /branch 场景重新走预期的 fork/切换/回滚路径，而不是在第一步标题查询处就抛错。
为什么需要
紧邻该回归套件之前合入的分支标题改动让 /branch 在 fork JSONL 之前先读取切出方 recorder 的自定义标题（并回退到源会话展示名）。回归 PR 的分支基线不含该改动，假环境只 mock 了 recorder 的 finalize/flush 和 session service 的 fork/load/remove/rename/标题前缀查询。两者在 main 上汇合后，所有 /branch 场景在 fork 之前就抛 “not a function”，main 的 CI 因此挂三个用例：a failed /branch restores the process-wide aggregate、a successful /branch keeps the replayed usage、a failure after the UI re-key does not roll back or undo。/resume 场景不读这两个方法，所以一直绿。本 PR 只补假环境，不改任何生产代码。
如何验证
在 packages/cli 下于 main 跑该回归文件，复现与 CI 一致的三个 /branch 失败；应用本改动后重跑，整文件通过（10/10），/branch 与 /resume 命令套件同样保持绿。
证据（改动前后）
改动前：
Tests 3 failed | 7 passed (10)，与 main CI 一致。改动后：Tests 10 passed (10)。测试平台
macOS ✅；Windows / Linux 交由 CI（⚠️ ）。
环境
N/A —— 纯单测。
风险与范围
关联 Issue
#9844 的后续修复；缺失的假接口来自 #9764。