feat(cli): tighten response timestamp consistency and tests - #5850
Conversation
) - Preserve timestamps for resumed assistant messages in convertToHistoryItems using ChatRecord.timestamp (ISO 8601 → epoch ms) - Add explicit hour/minute/second 2-digit options to toLocaleTimeString for guaranteed HH:MM:SS format across ICU variants - Add positive stream tests asserting commitItem attaches numeric timestamp to gemini items and does not attach one to user items - Update existing resumeHistoryUtils tests to include timestamp in mock data Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
…ems (QwenLM#5610) Review feedback: the previous test only asserted on user items, but the issue asks to verify timestamps are not attached to continuation items (gemini_content). Changed to assert on all non-gemini types. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Thanks for the PR, @ZijianZhang989! Template looks good ✓ — all required headings present, bilingual, test plan included. On direction: this is a solid follow-up to PR #5001. Resumed sessions showing timestamps on new turns but not historical ones is a genuine, user-visible inconsistency. Tightening the On approach: the scope feels right. 6 lines of production code, the rest is test coverage. Each change maps directly to a stated goal: One thing to flag (non-blocking): Moving on to code review and testing. 🔍 中文说明感谢贡献，@ZijianZhang989！ 模板完整 ✓ — 所有必需标题齐全，双语，含测试计划。 方向：这是 PR #5001 的一个合理后续。恢复的会话中，新 turn 有时间戳但历史 turn 没有，这是真实可见的不一致性。收紧 方案：范围合理。6 行生产代码，其余是测试覆盖。每项改动都直接对应一个声明的目标： 一个非阻塞提醒：如果 进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max |
Code ReviewIndependent proposal (before reading the diff): to fix the resumed-session timestamp inconsistency, I would:
The PR does exactly this. The implementation matches my proposal 1:1. Findings:
TestsAll targeted tests pass: Real-Scenario Testing (tmux)Attempted but could not complete — the CI environment has no API key configured, so Mitigation: the unit test coverage is strong. The two changed code paths are:
Terminal Output (attempted)中文说明代码审查独立方案（阅读 diff 前）：要修复恢复会话的时间戳不一致问题，我会：
PR 完全按照此方案实现，1:1 匹配。 发现：
测试所有目标测试通过：
真实场景测试（tmux）已尝试但无法完成——CI 环境无 API key， 缓解： 单元测试覆盖充分。两条修改的代码路径均有可靠的测试验证。 — Qwen Code · qwen3.7-max |
|
Stepping back: this is the kind of PR that's easy to approve because every line earns its place. Six lines of production code fix a visible inconsistency (resumed sessions missing timestamps) and tighten format portability (ICU digit-width). The remaining 111 lines are test coverage — both for the new resume path and for existing stream behavior that was previously untested. My independent proposal matched the PR's approach exactly. The The Couldn't run tmux real-scenario testing (no API key in CI), but the unit test coverage is thorough: 20 tests across both changed files, all passing. Typecheck and lint clean. Ships the fix cleanly, no scope creep, good test hygiene. LGTM. ✅ 中文说明退一步看：这个 PR 容易批准，因为每一行代码都物有所值。6 行生产代码修复了一个可见的不一致（恢复的会话缺少时间戳）并收紧了格式可移植性（ICU 数字宽度）。其余 111 行是测试覆盖——既覆盖新的恢复路径，也覆盖了之前未测试的现有流行为。 我的独立方案与 PR 的实现完全一致。代码库中已有的 唯一要提醒作者考虑的是无效 无法在 CI 中运行 tmux 真实场景测试（无 API key），但单元测试覆盖充分：两个改动文件共 20 个测试全部通过。Typecheck 和 lint 均干净。 干净地交付了修复，无范围蔓延，测试素养好。LGTM。✅ — Qwen Code · qwen3.7-max |
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. ✅
doudouOUC
left a comment
doudouOUC
left a comment
There was a problem hiding this comment.
[Suggestion] agentHistoryAdapter.ts:65 — Sub-agent gemini items miss timestamps. The agentMessagesToHistoryItems adapter creates { type: 'gemini', text: msg.content } without propagating msg.timestamp, even though AgentMessage.timestamp is a required number field. Sub-agent assistant responses will never show timestamps when showTimestamps is enabled, undermining the PR's consistency goal. Add timestamp: msg.timestamp to the gemini item construction.
[Suggestion] HistoryItemDisplay.test.tsx:457 — The timestamp format test fixture uses new Date('2026-01-15T14:30:45') which has naturally 2-digit time components. The regex /\[\d{2}:\d{2}:\d{2}\]/ would pass with or without the explicit 2-digit options, so the test can't catch a regression if someone removes them. Consider changing the fixture to new Date('2026-01-15T09:05:03') with single-digit components.
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| const geminiCalls = mockAddItem.mock.calls.filter( | ||
| (call: any[]) => call[0]?.type === 'gemini', |
There was a problem hiding this comment.
[Suggestion] Inconsistent (call: any[]) type annotations in the new filter callbacks. All other mockAddItem.mock.calls.filter callbacks in this file (e.g., lines 5934, 5991, 7468) use untyped (call) parameters. The explicit any[] suppresses type checking and breaks the file's convention.
| (call: any[]) => call[0]?.type === 'gemini', | |
| const geminiCalls = mockAddItem.mock.calls.filter( | |
| (call) => call[0]?.type === 'gemini', | |
| ); |
— qwen3.7-max via Qwen Code /review
| }); | ||
|
|
||
| describe('timestamp attachment', () => { | ||
| it('attaches a numeric timestamp to gemini items via commitItem', async () => { |
There was a problem hiding this comment.
[Suggestion] The test name says "via commitItem" but the timestamp is actually set by setPendingHistoryItem at useGeminiStream.ts:1133 (which creates the pending item with timestamp: Date.now()). By the time commitItem receives the item, !item.timestamp is already false, so the guard's value-assignment branch is never exercised.
The branch where commitItem itself assigns Date.now() — when a gemini item arrives without a pre-existing timestamp — is only reachable via the split path at useGeminiStream.ts:1163 (buffer exceeding STREAM_PENDING_ITEM_MAX_CHARS). No test triggers this path.
Consider adding a test that directly invokes commitItem with a gemini item lacking a timestamp, or triggers the split path, to validate the guard's assignment branch.
— qwen3.7-max via Qwen Code /review
✅ Local real-build verification (tmux) — PR #5850Maintainer verification before merge. Built the real CLI from this PR's head and from
1. Real TUI A/B —
|
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM. The remaining points look like non-blocking nits: mostly test naming/coverage precision and a small type annotation cleanup. Production behavior and scope look good to me.
5 participants
What this PR does
Follow-up to PR #5001 (optional response timestamps). Three consistency and hardening items: (1) resumed assistant messages now carry their original conversation record timestamp so historical turns display
[HH:MM:SS]consistently with newly streamed turns; (2) the time format now explicitly requests two-digit hour, minute, and second fields to guaranteeHH:MM:SSacross all ICU/runtime variants; (3) added positive stream tests that assertuseGeminiStreamattaches a numeric timestamp togeminiitems and does not attach one to any non-gemini item type.Why it's needed
With
output.showTimestampsenabled, old assistant turns from a restored session appeared without timestamps while newly streamed turns included them — a visible inconsistency. ThetoLocaleTimeStringoptions also lacked explicit digit-width hints, which could produce single-digit hours on some ICU variants. The stream-level timestamp behavior had no dedicated test coverage.Reviewer Test Plan
How to verify
npm run devoutput.showTimestampsin/settings[HH:MM:SS]appears before the responseshowTimestamps— next response should not have timestampEvidence (Before & After)
Before: Resumed sessions showed timestamps only on newly streamed turns; historical turns had none. Time format could vary across ICU versions.
After: All assistant turns (new and resumed) show
[HH:MM:SS]when the setting is enabled. Format is guaranteed two-digit across all platforms.Tested on
Environment
Node v22, macOS arm64, dev build (
npm run dev).Risk & Scope
ChatRecord.timestampis always a valid ISO 8601 string, sonew Date().getTime()is safe. Thetimestampfield onHistoryItemGeminiis optional, maintaining backward compatibility.false.Linked Issues
Closes #5610
Refs #4899
Refs #5001
中文说明
这个 PR 做了什么
PR #5001（可选响应时间戳）的后续工作。三项一致性和加固改动：(1) 恢复的助手消息现在携带原始会话记录的时间戳，使历史 turn 与新流式 turn 一样显示
[HH:MM:SS]；(2) 时间格式现在显式请求两位小时、分钟和秒字段，确保在所有 ICU/运行时变体上保证HH:MM:SS；(3) 添加了正向流测试，验证useGeminiStream为gemini类型 item 附加数字时间戳，不为任何非 gemini 类型 item 附加时间戳。为什么需要
启用
output.showTimestamps后，从恢复的会话中加载的旧助手 turn 没有时间戳，而新流式 turn 有时间戳——可见的不一致性。toLocaleTimeString选项也缺少显式的数字宽度提示，在某些 ICU 变体上可能产生单位数小时。流级别的时间戳行为没有专门的测试覆盖。Reviewer Test Plan
npm run dev/settings中启用output.showTimestamps[HH:MM:SS]showTimestamps— 下次回复不应有时间戳风险与范围
ChatRecord.timestamp始终是有效的 ISO 8601 字符串。HistoryItemGemini上的timestamp字段是可选的，保持向后兼容。false。关联 Issues
Closes #5610
Refs #4899
Refs #5001