Repository navigation
fix(session): keep prompt cache prefixes stable across working directories - #1517
anandgupta42 wants to merge 2 commits into
Conversation
…ories
- Pass the environment section (directory, workspace root, date) to `LLM.stream` separately and append it as a trailing system message after the cache-marked instructions
- Remove the instance directory from the `bash` tool `workdir` description and drop the no-op `${directory}` substitution, so serialized tool schemas are identical across directories
- Add regression tests for the bash schema, the prompt loop wiring, and Anthropic/Bedrock request assembly
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 65bda672-42b5-4edb-b25f-b9ae64ba8ece) |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
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. |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 29 seconds. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Linked Issues check |  |
The source changes satisfy the coding requirements that the reviewed evidence can establish for [#1490]. LLM.stream appends environment after plugin transformation, the environment remains in inpu… |
Provide a supported-provider validation with two sessions in different working directories and record cacheRead > 0 on the second session's first generation. The current evidence is sufficient for prefix and schema behavior but not for th… |
-
Check name Status Explanation Title check The title clearly and concisely describes the primary change: keeping prompt-cache prefixes stable across working directories. Description check The description includes the issue, change type, implementation details, verification results, screenshots status, and checklist information. It explains the root cause, fix, limitations, and test cov… Out of Scope Changes check The reviewed source changes and tests directly support [ #1490]. They move environment context out of the reusable system prefix, stabilize thebashschema, preserve runtime working-directory behavio…Docstring Coverage Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 6 files.
Full details: Linked Issues check
-
Explanation
The source changes satisfy the coding requirements that the reviewed evidence can establish for [
#1490].LLM.streamappendsenvironmentafter plugin transformation, the environment remains in input-token estimation, andBashToolno longer embedsInstance.directoryin its schema while execution still uses it as the default. The added tests check stable tool definitions, stable cache-marked system prefixes, and environment placement. The evidence does not establish the issue's provider-backed acceptance check that the second session's first generation reportscacheRead > 0; the PR explicitly reports no live cache-hit measurement.Resolution
Provide a supported-provider validation with two sessions in different working directories and record
cacheRead > 0on the second session's first generation. The current evidence is sufficient for prefix and schema behavior but not for the live cache result.
✨ Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit checks the prompt at dawn,
The changing path is neatly drawn.
The system prefix stays in line,
While dates and directories shine.
Bash finds its workdir when called,
And cache tests hop through them all.
Comment @coderabbitai help to get the list of available commands.
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (7 files)
Previous Review Summary (commit 01f4c63)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 01f4c63)Status: No Issues Found | Recommendation: Merge Files Reviewed (6 files)
Reviewed by gemini-4-argon · Input: 100K · Output: 2.5K · Cached: 441.6K Review guidance: REVIEW.md from base branch |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…akpoint providers - Add `ProviderTransform.usesExplicitCacheBreakpoints` (extracted from `message`) and use it in `LLM.stream` - Anthropic-style providers keep the trailing environment system message after the marked prefix; all other providers keep a single system message with the environment last - Pass the environment block through `experimental.chat.system.transform` again and keep it last after plugin additions - Extend request tests to OpenAI-style and Google-style providers Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: d956af07-b318-40f3-b686-7d1be44539b3) |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Addressed independent review in 994d11d:
|
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 994d11ddd7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
This branch has not been deployed
1 participant
Issue for this PR
Closes #1490
Type of change
What does this PR do?
Why: prompt caching only helps when the request prefix (system message and tool definitions) is byte-identical between sessions. Working directory and date text sat at the front of the system message and in the
bashtool schema, so opening the same project from a different directory (or on a different day) invalidated the cached prefix.Root cause:
SystemPrompt.environment()(directory, workspace root, date) was assembled before skills and project instructions in the single system message.bashtoolworkdirdescription interpolatedInstance.directory, changing the serialized tool schema per directory. (The${directory}replacement in the description was already a no-op.)Before -> after:
environmenttoLLM.streamand appended as a separate trailing system message after plugin transformation, so the cache markers sit before it and directory/date still reach the model. Output-budget estimation still counts it.bashworkdirdescribes its default generically; execution still resolvesparams.workdir || Instance.directory.Deferred / limits:
session/llm/request.tsunchanged.Upstream-shared files (
session/llm.ts,session/prompt.ts,tool/bash.ts) exist in the v1.4 baseline tree; all changes are wrapped inaltimate_changeupstream_fixmarkers. The analyzer reports nothing to check against upstream v2, so markers were audited manually.How did you verify your code works?
bun test test/tool test/session test/provider/transform.test.ts test/memory/injection.test.ts test/altimate/enhance-prompt.test.ts test/acp/tool.test.ts test/branding/build-integrity.test.ts(packages/opencode, unsandboxed): 2086 pass, 0 fail, 33 skip, 45 todo.bun run typecheck: clean.git diff --check: clean.Screenshots / recordings
N/A
Checklist
models-snapshot.ts🤖 Generated with Claude Code
Note
Medium Risk
Changes how system messages and tool schemas are serialized for every LLM turn; behavior is well-tested but any missed caller of
environmentor path-bearing instructions could still break cache stability.Overview
Fixes prompt cache invalidation when the same project is opened from a different cwd or on a new day. Directory, workspace root, and date used to live inside the reusable system-instructions prefix (and in the bash tool schema), so the byte-identical prefix providers cache on could change every session.
Session / LLM:
SystemPrompt.environment()is no longer merged into the mainsystemarray inprompt.ts. It is passed as optionalenvironmentonLLM.streamand appended as a trailing system segment after plugin system transforms, so Anthropic/Bedrock cache markers stay on the stable instruction prefix while ambient<env>context still reaches the model.Bash tool: Tool description and
workdirparameter text no longer embedInstance.directory; defaults are described generically and cwd is still resolved at execute time (params.workdir || Instance.directory).Tests: Coverage for identical system/tool prefixes across directories (including cache marker placement), prompt loop wiring of
environmentvssystem/messages, and stable bash definitions plus runtime workdir behavior.Reviewed by Cursor Bugbot for commit 01f4c63. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes prompt cache invalidation caused by working directory and date text appearing in the cached request prefix, so reusing the prefix across directories and days works as intended.
LLM.streamseparately and places it after the cached instructions: as a trailing system message for providers with explicit cache breakpoints (Anthropic, Bedrock, Alibaba) and appended to the single system message for others.usesExplicitCacheBreakpointsso the split only applies where cache breakpoints are explicit; plugin-transformed instructions stay ahead of the environment.bashtoolworkdirdescription and the no-op${directory}substitution; execution still resolves the default directory at runtime.Closes #1490.
Written for commit 994d11d. Summary will update on new commits.
Summary by CodeRabbit