fix(opencode): move volatile date out of cached system prefix#950
fix(opencode): move volatile date out of cached system prefix#950sumleo wants to merge 3 commits into
Conversation
environment() emitted the daily-changing date as the first entry in the system[] array, which applyCaching() marks with cacheControl. A session crossing midnight within the cache TTL invalidated the whole system prefix. Carry the date in the trailing user message instead, past the cache breakpoint, so the cached system bytes are date-invariant. Fixes AltimateAI#949
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesDate placement fix for prompt caching
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This PR doesn't fully meet our contributing guidelines and PR template. What needs to be fixed:
Please edit this PR description to address the above within 2 hours, or it will be automatically closed. If you believe this was flagged incorrectly, please let a maintainer know. |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR moves the volatile “today’s date” string out of the cache-controlled system prompt prefix to avoid unnecessary cache invalidation for sessions that cross midnight.
Changes:
- Removed the current date line from
SystemPrompt.environment()(cached system prefix). - Added
SystemPrompt.currentDate()and appended it to the trailing user message inSessionPrompt. - Added a regression test to ensure the environment prefix stays date-free while
currentDate()still reports today.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/opencode/src/session/system.ts | Removes volatile date from cached environment() and introduces currentDate() helper. |
| packages/opencode/src/session/prompt.ts | Appends the date to the last user message to keep it out of the cached system prefix. |
| packages/opencode/test/session/system.test.ts | Adds coverage asserting the cached env prefix doesn’t include date text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| test("environment() keeps the volatile date out of the cached system prefix", async () => { | ||
| await using tmp = await tmpdir({ git: true }) | ||
| await Instance.provide({ | ||
| directory: tmp.path, | ||
| fn: async () => { | ||
| const [env] = await SystemPrompt.environment(makeModel({ apiId: "claude-3-7-sonnet" })) | ||
| expect(env).toMatch(/<env>/) | ||
| expect(env).not.toMatch(/Today's date/) | ||
| expect(env).not.toContain(new Date().toDateString()) | ||
| expect(SystemPrompt.currentDate()).toContain(new Date().toDateString()) | ||
| }, | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Good catch - captured the date once at the top of the test so all three assertions compare the same value.
| const lastUserForDate = msgs.findLast((m) => m.info.role === "user") | ||
| if (lastUserForDate) { | ||
| lastUserForDate.parts = [ | ||
| ...lastUserForDate.parts, | ||
| { | ||
| type: "text" as const, | ||
| id: PartID.ascending(), | ||
| sessionID, | ||
| messageID: lastUserForDate.info.id, | ||
| text: `\n\n${SystemPrompt.currentDate()}`, | ||
| synthetic: true, | ||
| }, | ||
| ] | ||
| } |
There was a problem hiding this comment.
loop() already throws "No user message found" earlier if msgs has no user entry, so there's always a trailing user message to append to at this point. The date can't be silently dropped.
|
|
||
| // altimate_change start — upstream_fix: carry the date in the trailing user message | ||
| // so it stays out of the cache-controlled system prefix (see session/system.ts). | ||
| const lastUserForDate = msgs.findLast((m) => m.info.role === "user") |
There was a problem hiding this comment.
findLast is already used elsewhere in this file (the reminder-wrapping code just above), so the build target already supports it - kept it for consistency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/src/session/system.ts (1)
78-83: 💤 Low valueConsider adding a more specific cross-reference to the injection point.
The comment mentions "see session/prompt.ts" but doesn't specify the exact location where the date is now appended. Consider adding a reference to the specific function or line range (e.g., "see SessionPrompt.loop() at lines 972-988") to make future maintenance easier.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/session/system.ts` around lines 78 - 83, The comment block describing the date caching issue mentions that the date is appended to the trailing user message in session/prompt.ts, but does not specify the exact location within that file. Update the comment to include a specific cross-reference to the function name and line range in session/prompt.ts where the date is actually appended (for example, a specific function like SessionPrompt.loop() with its line range) instead of just the generic file reference, to aid future maintenance and code navigation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/src/session/system.ts`:
- Around line 78-83: The comment block describing the date caching issue
mentions that the date is appended to the trailing user message in
session/prompt.ts, but does not specify the exact location within that file.
Update the comment to include a specific cross-reference to the function name
and line range in session/prompt.ts where the date is actually appended (for
example, a specific function like SessionPrompt.loop() with its line range)
instead of just the generic file reference, to aid future maintenance and code
navigation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 00ce01c9-b57b-43c7-8be0-03624a70b55b
📒 Files selected for processing (3)
packages/opencode/src/session/prompt.tspackages/opencode/src/session/system.tspackages/opencode/test/session/system.test.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Quick heads-up on the one red check: anti-slop is failing only on |
❌ Tests — Failures DetectedTypeScript — 15 failure(s)
Next StepPlease address the failing cases above and re-run verification. cc @sumleo |
|
Thanks for the ping. I dug into the 15 cases and they're pre-existing test-isolation flakiness in the parallel runner, not something this PR introduces. What I ran (Bun 1.3.x,
Net: these aren't caused by the date move. Happy to take a look at the simulation-suite / MCP isolation flakiness separately if it'd help, but I'd rather not pull unrelated changes into this PR. Let me know if you'd like me to re-trigger the run. |
|
@sahrizvi Could I get a comment on this? Thanks! |
|
Hi @saravmajestic, gentle nudge on this when you have a moment. It's a small, self-contained prompt-caching fix, and I'm happy to rebase or tweak anything if that would make review easier. Thanks for the project and your time! |
🤖 Code Review — OpenCodeReview (Gemini) — No Issues FoundNo supported files changed. |
Fixes #949.
Summary
The environment system prompt put
Today's date: …first in the system message array, andapplyCachingmarks the first two system messages withcacheControl. So the one token in that block that changes daily lived inside the cached prefix. A session that crosses local midnight while still within the cache TTL gets a different prefix on the next request, missing the cache and re-billing the whole system block as a write.The fix drops the date line from
environment()and appends it to the trailing user message, past the cache breakpoint, via a smallcurrentDatehelper. It reuses the existing synthetic-text-part pattern already used inloop(). The model still sees the date; the cached system bytes are now date-invariant. Both edits are wrapped inaltimate_change/upstream_fixmarkers since the touched code is inherited from upstream.Test Plan
Extended
test/session/system.test.tsto assert theenvironment()output no longer contains the date string and that the new helper does. The test freezes system time so the date assertions are deterministic across a midnight boundary.bun test test/session/system.test.tspasses (8);bun run typecheckis clean.Checklist
Summary by CodeRabbit
Bug Fixes
Tests