Skip to content

fix(core): inject current date on every user query to prevent stale date#4798

Merged
tanzhenxin merged 7 commits into
QwenLM:mainfrom
Alex-ai-future:fix/stale-date-in-long-conversations
Jun 8, 2026
Merged

fix(core): inject current date on every user query to prevent stale date#4798
tanzhenxin merged 7 commits into
QwenLM:mainfrom
Alex-ai-future:fix/stale-date-in-long-conversations

Conversation

@Alex-ai-future

@Alex-ai-future Alex-ai-future commented Jun 5, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Injects the current date as a <system-reminder> tag on every UserQuery turn so the model always receives up-to-date temporal context, even during long-running conversations that span hours or days.

Why it's needed

The date in the startup context is injected once at session start via getEnvironmentContext() into the first chat history entry and never refreshed. In long-running conversations, the model only sees this frozen startup date and reports stale time when asked. Modeled after Cline's approach of re-resolving CURRENT_DATE at send time in PromptBuilder.preparePlaceholders().

Reviewer Test Plan

How to verify

  1. Build the CLI: npm install && npm run build && npm run bundle
  2. Start a conversation: ./dist/cli.js
  3. After a few turns, ask the model "What is the current date?"
  4. Confirm the model reports today's date, not the date when the session started

Evidence (Before & After)

Before: Model only sees "Today's date is Friday, June 5, 2026" from the startup context frozen at session start. After a conversation spanning multiple days, the model still reports the old date.

After: Every UserQuery includes a fresh <system-reminder> prepended to the request: "The current date is: [current date]. Note: This is the authoritative current date — it may differ from the 'Today's date' mentioned earlier in the conversation startup context."

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment

Local runtime: npm run dev

Risk & Scope

  • Main risk or tradeoff: Adds ~50 tokens per UserQuery to the system reminder block. Negligible compared to typical conversation context sizes. Uses a lastInjectedDate cache to avoid injecting duplicate dates within the same session (prevents conflicting dates when a session spans midnight).
  • Not validated / out of scope: ToolResult and Cron turns intentionally do not inject date — they are system-driven re-entries where the startup-context date is still current.
  • Breaking changes / migration notes: None. Purely additive change to the request payload.

Linked Issues

None filed — this fix was identified through direct investigation of stale time behavior during long conversations.

中文说明

该 PR 在每次用户发起对话时,将当前日期作为 <system-reminder> 系统提醒注入到请求中,确保模型始终能获取最新的时间信息,即使是在持续数小时或数天的长时间对话中也是如此。

当前日期仅在会话启动时通过 getEnvironmentContext() 注入到对话历史的第一条消息中。在长时间运行的对话中,模型只能看到这个冻结的启动日期,当被询问时间时会报告过时的信息。该方案参考了 Cline 的解决方式——在 PromptBuilder.preparePlaceholders() 中每次发送请求时重新解析 CURRENT_DATE。

验证方法:构建 CLI 后启动对话,询问模型当前日期,确认模型报告的是今天的日期而非会话启动时的日期。改动仅在 UserQuery 时增加约 50 tokens 的系统提醒文本,使用 lastInjectedDate 缓存避免同一天内重复注入,无破坏性变更。

@qwen-code-ci-bot qwen-code-ci-bot added category/core Core engine and logic type/bug Something isn't working as expected labels Jun 5, 2026

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, @Alex-ai-future!

Unfortunately the PR body is missing the required headings from the PR template. The current description is a single paragraph, but the template asks for structured sections: What this PR does, Why it's needed, Reviewer Test Plan (with How to verify, Evidence Before & After, Tested on), Risk & Scope, Linked Issues, and a 中文说明.

Could you update the PR body to follow the template? This helps reviewers understand the change and verify it works. Without it, review may be delayed.

The idea itself — refreshing the date on every user turn — seems reasonable and well-scoped (+17 lines in one file). Happy to dig into the code once the template is filled in. 🔧

中文说明

感谢提交 PR,@Alex-ai-future

目前 PR 描述缺少 PR 模板 中要求的各个标题。当前只有一段话,但模板要求包含以下结构化章节:What this PR doesWhy it's neededReviewer Test Plan(含验证步骤、前后对比证据、测试平台)、Risk & ScopeLinked Issues,以及中文说明。

请按照模板更新 PR 描述,这有助于评审者理解和验证改动。缺少模板可能导致评审延迟。

方案本身——每次用户对话时刷新日期——看起来合理且范围可控(仅一个文件 +17 行)。模板补齐后会继续深入审查代码。🔧

Qwen Code · qwen3.7-max

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] 3 existing tests in client.test.ts (lines 2482, 3188, 3224) fail because the new date reminder is prepended to the messages array on every UserQuery turn, but the toHaveBeenCalledWith assertions were not updated. For example, the test at line 3188 expects ['Quick question'] but now receives ['The current date and time is: ...', 'Quick question']. These tests must be updated to account for the new prepended element (e.g., using expect.stringMatching(/^The current date and time is:/) as the first array element).

[Suggestion] No test verifies that the date/time system reminder is actually injected on UserQuery turns. The core new behavior — new Date().toLocaleDateString(...) formatted and pushed into systemReminders — has zero positive test assertion. Consider adding a test that uses vi.setSystemTime() and asserts the request contains the expected date string.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/client.ts Outdated
day: 'numeric',
});
systemReminders.push(
`The current date and time is: ${now}. Note: This is the authoritative current time — it may differ from the "Today's date" mentioned earlier in the conversation startup context.`,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The reminder says "The current date and time is:" and "This is the authoritative current time" but toLocaleDateString() produces only a date (e.g., "Friday, June 5, 2026") with no time component. The model is told it has time information when it doesn't, which can lead to hallucinated time-of-day answers.

Suggested change
`The current date and time is: ${now}. Note: This is the authoritative current time — it may differ from the "Today's date" mentioned earlier in the conversation startup context.`,
systemReminders.push(
`The current date is: ${now}. Note: This is the authoritative current date — it may differ from the "Today's date" mentioned earlier in the conversation startup context.`,
);

Alternatively, use toLocaleString() with hour/minute options if time-of-day awareness is genuinely desired.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/client.ts Outdated
) {
const systemReminders = [];

// Inject the current date/time on every UserQuery turn so the model

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The date reminder is injected into requestToSend, which gets permanently persisted in chat history via Turn.run()geminiChat.sendMessageStream(). On every subsequent turn, the full history (including ALL prior date reminders) is re-sent to the API. In a long-running session spanning midnight, the model will accumulate multiple mutually-contradictory "authoritative" dates (e.g., "Thursday, June 4... authoritative" at turn 50 and "Friday, June 5... authoritative" at turn 51).

Consider either (a) injecting only when the formatted date actually differs from the last-injected date (cache the string and skip if unchanged), or (b) stripping prior date-reminder strings from older history entries before sending.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/client.ts
The date in the startup context is injected once at session start and never
refreshed, causing the model to report outdated time during long-running
conversations.

Inject the current date as a system reminder on every UserQuery turn,
modeled after Cline's approach of re-resolving CURRENT_DATE at send time.
Uses a lastInjectedDate cache to avoid injecting duplicate dates within
the same session (prevents conflicting dates when a session spans midnight).

- packages/core/src/core/client.ts: add lastInjectedDate field, inject
  date only on UserQuery turns and only when the date has changed
- packages/core/src/core/client.test.ts: fix 3 existing tests that now
  expect a date prefix, add 2 new tests for date injection and dedup

Signed-off-by: Alex <alex.tech.lab@outlook.com>
@Alex-ai-future Alex-ai-future force-pushed the fix/stale-date-in-long-conversations branch from 842afba to c3dd7ae Compare June 5, 2026 12:51
@Alex-ai-future Alex-ai-future changed the title fix(core): inject current time on every user query to prevent stale date fix(core): inject current date on every user query to prevent stale date Jun 5, 2026
Comment thread packages/core/src/core/client.test.ts Outdated
expect.any(AbortSignal),
);

vi.setSystemTime(undefined);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] vi.setSystemTime(undefined) causes TS2345: undefined is not assignable to string | number | Date. The intent is to restore real timers after the test, but setSystemTime requires a concrete value.

Suggested change
vi.setSystemTime(undefined);
vi.useRealTimers();

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/client.test.ts Outdated
const secondCall = mockTurnRunFn.mock.calls[1];
expect(secondCall[1][0]).toBe('Second question');

vi.setSystemTime(undefined);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] Same TS2345 issue as above. Use vi.useRealTimers() to restore the real system clock.

Suggested change
vi.setSystemTime(undefined);
vi.useRealTimers();

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/client.ts Outdated
// Inject fresh date on UserQuery turns only; Cron and ToolResult turns
// reuse the same session and the startup-context date is still current.
if (messageType === SendMessageType.UserQuery) {
const today = new Date().toLocaleDateString(undefined, {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] toLocaleDateString(undefined, ...) delegates to the system locale, producing non-deterministic output: "June 5, 2026" (en-US) vs "5 June 2026" (en-GB) vs fully localized strings. This breaks the two new tests (which hardcode US format) on any non-US-locale CI runner or developer machine.

Pin the locale to produce consistent, portable output:

Suggested change
const today = new Date().toLocaleDateString(undefined, {
const today = new Date().toLocaleDateString('en-US', {

— qwen3.7-max via Qwen Code /review

- Pin toLocaleDateString to 'en-US' for consistent output across locales
- Replace vi.setSystemTime(undefined) with vi.useRealTimers() to fix TS2345

Signed-off-by: Alex <alex.tech.lab@outlook.com>
// This prevents accumulating conflicting dates when a session
// spans midnight.
if (today !== this.lastInjectedDate) {
systemReminders.push(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The date reminder is pushed as a bare string, but every other named system reminder in the same systemReminders array wraps its content in <system-reminder>...</system-reminder> tags — getPlanModeSystemReminder(), getArenaSystemReminder(), and wrapIdeContext() all do so. The system prompt (prompts.ts:288) explicitly instructs the model to treat <system-reminder> tags as authoritative system content, not user input. Without tags, the model may assign lower weight to the date correction.

Suggested change
systemReminders.push(
systemReminders.push(
`<system-reminder>\nThe current date is: ${today}. Note: This is the authoritative current date — it may differ from the \"Today's date\" mentioned earlier in the conversation startup context.\n</system-reminder>`,
);

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/client.ts Outdated
// Inject fresh date on UserQuery turns only; Cron and ToolResult turns
// reuse the same session and the startup-context date is still current.
if (messageType === SendMessageType.UserQuery) {
const today = new Date().toLocaleDateString('en-US', {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The injection uses toLocaleDateString('en-US', ...) but the startup context in environmentContext.ts:53 uses toLocaleDateString(undefined, ...) (system locale). On non-US systems (e.g., de-DE, zh-CN), these produce different format strings for the same date — the startup context shows Freitag, 5. Juni 2026 while the injected reminder shows Friday, June 5, 2026. The caveat "it may differ from the 'Today's date' mentioned earlier" becomes always true, not just at midnight crossings.

Consider either (a) pinning environmentContext.ts:53 to 'en-US' as well, or (b) extracting a shared date-formatting function so both sites stay in sync.

— qwen3.7-max via Qwen Code /review

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Downgraded from Approve to Comment: CI still running.

R3 review: the two Critical issues from R2 (vi.setSystemTime(undefined) type errors and toLocaleDateString locale pinning) are both fixed. 145 tests pass. No new Critical findings — two Suggestions below.

— qwen3.7-max via Qwen Code /review

* Only UserQuery turns inject dates; Cron/ToolResult turns reuse the
* startup-context date which is still current within the same session.
*/
private lastInjectedDate: string | undefined;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] lastInjectedDate is never cleared at session boundaries. resetChat() (line 590) clears cachedGitStatus, lastApiCompletionTimestamp, surfacedRelevantAutoMemoryPaths, file read cache, and deferred tools — but not lastInjectedDate. After /clear or auto-compaction on the same calendar day, the dedup guard today !== this.lastInjectedDate suppresses re-injection.

The cleanest fix is in startChat() (~line 780), which is the common entry point for both /clear and compression:

Suggested change
private lastInjectedDate: string | undefined;
async startChat(
extraHistory?: Content[],
sessionStartSource = extraHistory
? SessionStartSource.Resume
: SessionStartSource.Startup,
): Promise<GeminiChat> {
this.forceFullIdeContext = true;
this.lastInjectedDate = undefined;

— qwen3.7-max via Qwen Code /review

});
});

it('should inject the current date on every UserQuery turn', async () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The midnight rollover branch of the dedup conditional is untested. The existing "should not inject duplicate date on the same day" test only covers the skip path (today === lastInjectedDate). No test advances the clock past midnight to verify that a changed date triggers re-injection and updates lastInjectedDate — which is the primary scenario the dedup logic was designed for.

Consider adding a test that freezes time at 23:59, sends a query, advances to 00:01, sends another query, and asserts the new date is injected.

— qwen3.7-max via Qwen Code /review

…date format, session reset, midnight test

- Wrap date reminder in <system-reminder> tags (consistent with other reminders)
- Extract formatDateForContext() shared function in environmentContext.ts
- Use shared function in both client.ts and environmentContext.ts (pin 'en-US')
- Reset lastInjectedDate in startChat() for session boundary correctness
- Add midnight rollover test (date changes trigger re-injection)
- Update 3 existing tests that asserted not.toContain('<system-reminder>')
  to check for IDE-specific content instead (date reminder now uses tags)
- Update mock to use importOriginal so formatDateForContext is real

Signed-off-by: Alex <alex.tech.lab@outlook.com>

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] environmentContext.ts:73 — The startup context template still says "(formatted according to the user's locale)" but formatDateForContext() now hardcodes 'en-US'. The text is factually incorrect after this PR's locale pinning. environmentContext.test.ts:115 also asserts this now-wrong string.

Fix: drop the parenthetical — Today's date is ${today}. — and update the test accordingly.

(Posted as a body-level comment because the affected line is not in the PR diff hunks.)

Comment thread packages/core/src/core/client.test.ts Outdated
}));
vi.mock('../utils/environmentContext', async (importOriginal) => {
const actual =
await importOriginal<typeof import('../utils/environmentContext')>();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Critical] TS2835: Missing .js extension in typeof import() type query. With --moduleResolution nodenext, relative import paths require explicit file extensions.

Every other importOriginal<typeof import(...)> in the codebase uses .js for relative imports (e.g., '../utils/fileUtils.js', '../telemetry/index.js').

Suggested change
await importOriginal<typeof import('../utils/environmentContext')>();
await importOriginal<typeof import('../utils/environmentContext.js')>();

— qwen3.7-max via Qwen Code /review

…nd fix TS2835

- Remove '(formatted according to the user's locale)' from startup context
  template since formatDateForContext now hardcodes 'en-US'
- Remove corresponding test assertion in environmentContext.test.ts
- Add missing .js extension in typeof import() to fix TS2835

Signed-off-by: Alex <alex.tech.lab@outlook.com>
Comment thread packages/core/src/core/client.test.ts Outdated
expect.any(AbortSignal),
);

vi.useRealTimers();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] vi.useRealTimers() is called at the end of each of the 3 new date-injection test bodies (lines 3336, 3402, 3470) but not in the shared afterEach at line 456. If any expect() throws before reaching vi.useRealTimers(), fake timers and the frozen clock leak to every subsequent test in the suite — vi.restoreAllMocks() does not restore real timers.

Add vi.useRealTimers() to the top-level afterEach at line 456:

afterEach(() => {
  vi.useRealTimers();
  vi.restoreAllMocks();
  __resetActiveGoalStoreForTests();
});

Then remove the per-test vi.useRealTimers() calls.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/client.test.ts Outdated
// Reset date injection state from prior tests
client['lastInjectedDate'] = undefined;
// Freeze time to a specific date
vi.setSystemTime(new Date('2026-06-05T10:00:00Z'));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] This timestamp (T10:00:00Z) and the one at line 3345 are 10:00 UTC, but formatDateForContext() uses toLocaleDateString('en-US', ...) which applies the local timezone. In UTC+14, 2026-06-05T10:00:00Z is June 6 locally — the assertion for "June 5, 2026" would fail. The midnight-rollover test wisely uses T12:00:00Z (noon), but these two tests don't.

Suggested change
vi.setSystemTime(new Date('2026-06-05T10:00:00Z'));
vi.setSystemTime(new Date('2026-06-05T12:00:00Z'));

(Also update line 3345 to use noon UTC.)

— qwen3.7-max via Qwen Code /review

…fix timezone edge case

- Add vi.useRealTimers() to top-level afterEach to prevent fake timer leaks
- Remove per-test vi.useRealTimers() calls from 3 date injection tests
- Change T10:00:00Z to T12:00:00Z in date tests to avoid timezone edge cases
  (10:00 UTC could be next day in UTC+14)

Signed-off-by: Alex <alex.tech.lab@outlook.com>

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R6 Review Summary

All prior findings (R1–R5) have been addressed. The incremental commit ab27ee4b7 correctly centralizes vi.useRealTimers() in the shared afterEach hook and shifts timestamps from T10:00:00Z to T12:00:00Z for better timezone coverage. Deterministic checks pass: tsc 0 errors, eslint 0 errors, 158 tests pass.

Suggestions

Test coverage gaps — Three negative/boundary paths lack explicit test coverage:

  1. No negative test for Cron/ToolResult date non-injection (client.ts:1628): The production code correctly guards date injection behind messageType === SendMessageType.UserQuery, but no test sends a Cron or ToolResult turn and asserts the date <system-reminder> is absent. If someone later relaxes this guard, no test would catch it.

  2. No test for resetChat() clearing lastInjectedDate (client.ts:783): startChat() resets the field, but the existing resetChat test suite doesn't verify this. A future refactor could accidentally remove the reset without detection.

  3. No formatDateForContext() unit test (environmentContext.ts:16-22): The function pins locale to 'en-US' — a deliberate design choice. A direct unit test (e.g., formatDateForContext(new Date('2026-06-05'))'Friday, June 5, 2026') would lock in this contract and catch locale regressions.

Needs Human Review

Possibly: Test timezone sensitivity — The tests use vi.setSystemTime(new Date('2026-06-05T12:00:00Z')) and assert "June 5, 2026", but toLocaleDateString('en-US', ...) formats in the system's local timezone. In UTC+12+ (New Zealand, Fiji), noon UTC is already June 6 locally. This is a known tradeoff from the R5 timezone fix (shifting from T10 to T12), but tests would still fail in those rare timezones. Consider mocking formatDateForContext in tests or setting process.env.TZ = 'UTC'.


— qwen3.7-max via Qwen Code /review

…t test, formatDateForContext test

- Set process.env.TZ=UTC in client.test.ts for consistent toLocaleDateString
  output regardless of developer timezone
- Add Cron non-injection negative test (date not injected on Cron turns)
- Add resetChat() clearing lastInjectedDate test
- Add formatDateForContext() unit tests (en-US locale contract)
- Fix Cron test: pass { type: SendMessageType.Cron } not SendMessageType.Cron

Signed-off-by: Alex <alex.tech.lab@outlook.com>
@wenshao

wenshao commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Local Verification Report

PR: #4798 fix/stale-date-in-long-conversationsmain
Scope: 4 files, +337/−29 (client.ts, environmentContext.ts + tests)

Test Results

Check Result Details
Core tests (full) ✅ 10061/10066 passed 1 pre-existing failure, 4 skipped
PR-specific tests ✅ 162/162 passed client.test.ts (148) + environmentContext.test.ts (14)
Core typecheck ✅ 0 errors Clean
CLI typecheck ✅ 0 new errors 113 total, all pre-existing (108 on base + 5 from main drift)
ESLint ✅ Clean --max-warnings 0 on all 4 changed files
Whitespace ✅ Clean git diff --check passed

Pre-existing Failure (not from this PR)

  • anthropicContentGenerator.test.ts:184 — User-Agent header expects QwenCode/1.2.3 but gets claude-cli/1.2.3 (external, cli). Confirmed same failure on main base.

Typecheck Delta Analysis

CLI typecheck shows 113 errors (vs 108 on local main). The +5 delta comes from BaseTextInput.tsx (ink/dom module) and useGeminiStream.ts (FinishReason enum) — files not touched by this PR. Caused by local main being 83 commits behind origin/main; the PR branch is rebased on newer main. No typecheck errors originate from the 4 changed files.

Verdict

✅ PASS — All PR-specific tests pass. No new test failures, no new typecheck errors, no lint issues.


Verified locally by wenshao

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R6 Review Summary

All prior findings (R1–R5) have been addressed. The incremental commit ab27ee4b7 correctly centralizes vi.useRealTimers() in the shared afterEach hook and shifts timestamps from T10:00:00Z to T12:00:00Z for better timezone coverage. Deterministic checks pass: tsc 0 errors, eslint 0 errors, 158 tests pass.

Suggestions

Test coverage gaps — Three negative/boundary paths lack explicit test coverage:

  1. No negative test for Cron/ToolResult date non-injection (client.ts:1628): The production code correctly guards date injection behind messageType === SendMessageType.UserQuery, but no test sends a Cron or ToolResult turn and asserts the date <system-reminder> is absent. If someone later relaxes this guard, no test would catch it.

  2. No test for resetChat() clearing lastInjectedDate (client.ts:783): startChat() resets the field, but the existing resetChat test suite doesn't verify this. A future refactor could accidentally remove the reset without detection.

  3. No formatDateForContext() unit test (environmentContext.ts:16-22): The function pins locale to 'en-US' — a deliberate design choice. A direct unit test (e.g., formatDateForContext(new Date('2026-06-05'))'Friday, June 5, 2026') would lock in this contract and catch locale regressions.

Needs Human Review

Possibly: Test timezone sensitivity — The tests use vi.setSystemTime(new Date('2026-06-05T12:00:00Z')) and assert "June 5, 2026", but toLocaleDateString('en-US', ...) formats in the system's local timezone. In UTC+12+ (New Zealand, Fiji), noon UTC is already June 6 locally. This is a known tradeoff from the R5 timezone fix (shifting from T10 to T12), but tests would still fail in those rare timezones. Consider mocking formatDateForContext in tests or setting process.env.TZ = 'UTC'.


— qwen3.7-max via Qwen Code /review

@wenshao wenshao left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

R6 Review Summary

All prior findings (R1–R5) have been addressed. The incremental commit ab27ee4b7 correctly centralizes vi.useRealTimers() in the shared afterEach hook and shifts timestamps from T10:00:00Z to T12:00:00Z for better timezone coverage. Deterministic checks pass: tsc 0 errors, eslint 0 errors, 158 tests pass.

Suggestions

Test coverage gaps — Three negative/boundary paths lack explicit test coverage:

  1. No negative test for Cron/ToolResult date non-injection (client.ts:1628): The production code correctly guards date injection behind messageType === SendMessageType.UserQuery, but no test sends a Cron or ToolResult turn and asserts the date <system-reminder> is absent. If someone later relaxes this guard, no test would catch it.

  2. No test for resetChat() clearing lastInjectedDate (client.ts:783): startChat() resets the field, but the existing resetChat test suite doesn't verify this. A future refactor could accidentally remove the reset without detection.

  3. No formatDateForContext() unit test (environmentContext.ts:16-22): The function pins locale to 'en-US' — a deliberate design choice. A direct unit test (e.g., formatDateForContext(new Date('2026-06-05'))'Friday, June 5, 2026') would lock in this contract and catch locale regressions.

Needs Human Review

Possibly: Test timezone sensitivity — The tests use vi.setSystemTime(new Date('2026-06-05T12:00:00Z')) and assert "June 5, 2026", but toLocaleDateString('en-US', ...) formats in the system's local timezone. In UTC+12+ (New Zealand, Fiji), noon UTC is already June 6 locally. This is a known tradeoff from the R5 timezone fix (shifting from T10 to T12), but tests would still fail in those rare timezones. Consider mocking formatDateForContext in tests or setting process.env.TZ = 'UTC'.


— qwen3.7-max via Qwen Code /review

@Alex-ai-future Alex-ai-future requested a review from wenshao June 8, 2026 01:44
wenshao
wenshao previously approved these changes Jun 8, 2026
@tanzhenxin

Copy link
Copy Markdown
Collaborator

Hi @Alex-ai-future — the change itself looks good on review. Two things are blocking the merge right now, both quick:

  1. Merge conflicts — the branch is currently showing conflicts with main (GitHub marks it not mergeable). Could you rebase on the latest main and resolve them?
  2. PR description — the triage bot flagged that the description is missing the required headings from the PR template. Filling those in will clear its "changes requested" status.

Once it's conflict-free and the template is filled in, I'll approve. Thanks for working through all the review rounds on this!

@Alex-ai-future

Copy link
Copy Markdown
Contributor Author

@tanzhenxin thank you a lot, I have already addressed all.

@tanzhenxin tanzhenxin left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review

Refreshes the current date on every user turn via a <system-reminder>, deduplicated to at most one injection per calendar day so history never accumulates conflicting dates. Gating is correct — only UserQuery turns inject, while tool-result, cron, retry, hook, and notification turns skip — and there is no prompt-cache regression since the reminder is appended after the cacheable prefix. The shared date formatter keeps the startup context and the per-turn reminder textually identical. The earlier conflict with main has been resolved and the change is unchanged in scope from my prior read.

A couple of low-severity notes remain, none blocking: a long-lived cron run can still report the startup date across day boundaries because cron turns intentionally don't inject, and the very first user turn shows both the seeded startup date and an identical per-turn reminder. Neither affects correctness.

Verdict

APPROVE — bounded, cache-safe, and correctly gated; only minor design notes remain.

@tanzhenxin tanzhenxin dismissed qwen-code-ci-bot’s stale review June 8, 2026 08:42

PR body has been updated to follow the required template — all requested sections (What this PR does, Why it's needed, Reviewer Test Plan, Risk & Scope, Linked Issues, 中文说明) are now present. Dismissing this stale triage review.

@tanzhenxin tanzhenxin enabled auto-merge (squash) June 8, 2026 08:44
@tanzhenxin tanzhenxin merged commit 6a99ed1 into QwenLM:main Jun 8, 2026
3 of 7 checks passed
@wenshao

wenshao commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Verification Report — PR #4798 (inject current date on every user query)

Verdict: FAIL as-submitted — merge-blocking build break. The date-injection feature itself is correct and fully verified at runtime (see below), but PR head 22cad98e does not compile, so it cannot be merged until a one-line syntax error is fixed.

结论:当前 HEAD(22cad98e无法构建 —— packages/core/src/utils/environmentContext.test.ts 第 401/402 行缺少两个闭合 });(merge 冲突遗留),导致 npm run build 失败并连锁使 npm run bundle 无法产出 dist/cli.js,三平台 CI 全红。修掉这个语法错误后,功能本身经真实运行验证完全正确(注入 / 去重 / /clear 重置 / 启动上下文 en-US 全部通过)。建议作者补上闭合括号后再合并。


🚫 Blocker: PR head fails to build

npm run build (PR test-plan step 1) fails:

packages/core/src/utils/environmentContext.test.ts(599,1): error TS1005: '}' expected.
Error: Command failed: npm run build --workspace=packages/core

Because npm run build compiles every workspace package and stops at packages/core, the downstream packages (channel-base, channel-weixin, web-templates) never emit their dist/, so npm run bundle then fails too and no dist/cli.js is produced. CI confirms it independently — Lint + Test on macOS / Ubuntu / Windows are all red.

Root cause (exact): in environmentContext.test.ts, the it('should use current date when no date provided', …) block is never closed — line 401 is immediately followed by the next top-level describe on line 402. A merge that pulled describe('startup reminder builders', …) from main dropped the two closing lines of the formatDateForContext block.

     expect(result.length).toBeGreaterThan(0);
+  });
+});
+
 describe('startup reminder builders', () => {

Verified: inserting those }); }); at line 402 takes the TypeScript parser from 1 error → 0 errors, and npm run build && npm run bundle then succeed. (I applied only this test-file fix — it touches no production/bundled code — to unblock the runtime verification below.)


✅ Feature verification (runtime, after the 1-line fix)

Method: built the PR branch (node dist/cli.js), pointed it at a local mock OpenAI server (OPENAI_BASE_URL) that records the exact request payload sent to the model, and drove a real multi-turn TUI session in tmux. Evidence is the actual on-the-wire request the CLI sends.

1. ✅ Injection on a UserQuery turn. The model request for turn 1 carries a dedicated content part with today's real date, en-US formatted:

<system-reminder>
The current date is: Monday, June 8, 2026. Note: This is the authoritative current date — it may differ from the "Today's date" mentioned earlier in the conversation startup context.
</system-reminder>

(System date today = Monday, June 8, 2026 — exact match. Placed immediately before the user's typed text.)

2. ✅ Same-day dedup + ✅ /clear reset + ✅ non-UserQuery turns don't inject. Captured request payloads across a real session (each "main" turn also spawns a background memory-extraction call, shown for completeness):

Captured request Newest user message Date re-injected in the new turn?
turn 1 (req-3) "What day is it?" yes (first query of the day)
turn 2, same day (req-5) "And now?" no — deduped; the turn-1 reminder still rides along in history exactly once
after /clear (req-7) "Day three question" yes/clear → resetChat() cleared lastInjectedDate, so it re-injects (request also drops back to 2 messages = history cleared)
background memory tasks (req-4/6/8) (memory extraction) no — not a UserQuery turn

This exercises all three client.ts behaviors end-to-end: inject-once-per-day, the lastInjectedDate cache, and the resetChat() reset (line 867).

3. ✅ Startup-context change. The startup env line is now "Today's date is Monday, June 8, 2026." — pinned en-US via the shared formatDateForContext, and the old "(formatted according to the user's locale)" parenthetical is gone, as intended.


Findings

  • ⚠️ Merge-blocking (must fix): the unclosed it/describe braces above. One-line-ish fix; CI is red until then.
  • 🔍 Dedup persists the reminder in history (works as intended). On turn 2 the date reminder is not re-sent as a fresh part, but turn 1's reminder remains in the conversation history (count stays 1), so the model never loses the date — confirmed in req-5.
  • 🔎 Minor, across-midnight (not runtime-exercised): when a session crosses midnight, a new reminder is injected while the previous day's reminder is still present earlier in history. The newest (authoritative) one is last, so a capable model uses it — but two different "current date is" lines coexist in context. The Note: text only reconciles against the startup context, not a stale in-history reminder. Low impact; the midnight path itself is covered by the PR's unit tests (should re-inject date when session spans midnight), which I did not re-run (CI's job).
  • ℹ️ Token cost matches the PR's ~50-token estimate (the reminder is ~220 chars).

Recommendation

Hold merge until the environmentContext.test.ts braces are fixed (CI will go green). The feature logic is sound and fully verified — no changes needed to the production code in client.ts / environmentContext.ts.

Verified locally on Linux: built PR 22cad98e (with the trivial test-brace fix) → mock OpenAI endpoint + tmux multi-turn TUI → inspected captured request payloads. Build break reproduced via npm run build directly.

yiliang114 added a commit that referenced this pull request Jun 8, 2026
…est.ts

PR #4798 added a new vi.mock with importOriginal but left the old
manual mock in place. Vitest uses the last vi.mock call, so the old mock
won without formatDateForContext, causing 77 test failures. Merge both
into a single importOriginal-based mock that preserves all overrides.
tanzhenxin pushed a commit that referenced this pull request Jun 8, 2026
…ock (#4863)

* fix(core): add missing closing braces in formatDateForContext test block

PR #4798 introduced a `describe('formatDateForContext')` test block in
environmentContext.test.ts but omitted the closing `});` for both the
`it` case and the `describe` block, causing a TS1005 syntax error that
broke `tsc --build` and failed CI on all three platforms (ubuntu, macos,
windows) plus lint.

* fix(core): merge duplicate vi.mock for environmentContext in client.test.ts

PR #4798 added a new vi.mock with importOriginal but left the old
manual mock in place. Vitest uses the last vi.mock call, so the old mock
won without formatDateForContext, causing 77 test failures. Merge both
into a single importOriginal-based mock that preserves all overrides.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category/core Core engine and logic type/bug Something isn't working as expected

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants