Repository navigation
fix(session): keep the cached system prompt prefix stable across steps - #1631
RaghwendraSingh wants to merge 5 commits into
Conversation
Two pieces of text in the cached system prompt changed between requests of the same session, so the provider prompt cache missed and the whole prefix (system prompt plus tool schemas) was written again at the full input rate: - The Altimate Workspace section rendered one sentence for a fresh link check and another for a memoised one, so it flipped on the 30 s memo timer. A re-check that outlasted the 1.5 s deadline also marked the previous outcome stale, which dropped the workspace contents and memory line. The section now renders one hedged sentence per link state, and a slow re-check reuses the previous outcome as it was. Only the resolver marks a link stale. - Hoisted reminders (the post-compaction task pin, the plan switch) were appended to the system array on non-Anthropic-like models, which includes the Altimate gateway model. They now ride in the trailing environment message, after the cache breakpoint, and still reach the model as system text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: true
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. |
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
Request-body capture:
|
| scenario | build | agent requests | distinct first-system hashes | changes |
|---|---|---|---|---|
| 9 steps, 50 s apart (crosses the 5-min miss memo) | main |
9 | 2 | 2 (requests 7 and 8) |
| this branch | 9 | 1 | 0 | |
| compaction on step 2 | main |
5 | 2 | 1 (first request after compaction) |
| this branch | 5 | 1 | 0 |
Timer, main. At t≈301 s the miss memo expires and that step's resolve asks the server itself, so it renders the fresh wording; 50 s later the step reads the memoised miss again and renders the other wording:
-No Altimate Workspace was linked to this project as of the last check, up to five minutes ago; a link made elsewhere since then would not show yet.
+No Altimate Workspace is linked to this project.and back at request 8. That is two full-prefix cache writes every five minutes on an unlinked project. On this branch all nine requests have the same first system message.
Compaction, main. The first request after the compaction adds the hoisted Original task — authoritative over any summary… reminder to the first system message. On this branch that message is unchanged, and the reminder is in the trailing system message after the breakpoint.
On both builds the tools hash and the number of cache-marked system messages (2) are constant within each run.
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
sahrizvi
left a comment
sahrizvi
left a comment
There was a problem hiding this comment.
Requesting changes: two timer-driven flips remain in the section this PR stabilizes, so it does not yet do what it claims.
The two changes in the diff are correct for what they touch. The pin reaches the model as system text on every provider I probed, with the first message unchanged on the explicit-breakpoint path, and fresh and memoised wording are identical. test/session, test/tool, test/agent and the touched files show no regressions (0 failures at base and at head).
Blocking:
lastKnownkeeps only aboundanswer, so an unlinked project with a slow re-check still flips (identity.ts:401, one-line fix suggested).- The team-memory line expires 60 s after each check (
identity.ts:372).
Both also happen on main, so they are incomplete fixes rather than regressions.
Three minors are inline and one finding that has no diff line, with the nits, is in a separate PR comment. None of those block. The gateway cache_read A/B is worth running before this leaves draft, including a linked project and a project with lessons. The per-request hash counter is fine as a follow-up.
| // Marking it stale here dropped the workspace contents and memory line from the cached | ||
| // system prefix whenever a resolve outlasted the deadline, re-creating the prefix at full | ||
| // input rate. Only the resolver itself marks a binding stale. | ||
| if (previous?.status === "bound") return previous |
There was a problem hiding this comment.
MAJOR: a slow re-check on an unlinked project still switches text
This keeps the previous answer only for bound. A previous unbound falls through to the pin check and the local cache, finds no binding, and returns unknown.
- An unlinked project gets a 404 on the first step.
- After about 5 minutes the miss memo and the outcome memo have both expired, so the step asks the server.
- If that takes more than
RESOLVE_DEADLINE_MS, the step renders "Whether this project is linked to an Altimate Workspace could not be verified just now." - The next step reads the settled miss and flips back.
That is two full prefix writes per occurrence. The PR body and #1633 both say a slow re-check reuses the previous outcome as it was. The mock table is this project type, but its mock answers instantly, so it cannot show this.
I reproduced it with Date.now advanced 301 s and a hanging lookup. The behaviour is the same on main, so this is an incomplete fix rather than a regression.
| if (previous?.status === "bound") return previous | |
| if (previous && previous.status !== "unknown") return previous |
With this line, that probe is byte-identical and identity.test.ts plus identity-section.test.ts stay at 69 of 69.
There was a problem hiding this comment.
Fixed in 51ea805: lastKnown now returns any previous non-unknown outcome, and a stepped-clock test (301 s, hanging lookup) asserts one hash across six steps on an unlinked project.
| // A stale outcome is a link the server could not confirm: promising that a save | ||
| // syncs to that workspace would not be true of it. | ||
| if (outcome.stale) return {} | ||
| return { teamMemory: memoryEnabledCached(outcome.binding) === "enabled", memoryTools } |
There was a problem hiding this comment.
MAJOR: the team-memory line still flips on a 60-second timer
memoryEnabledCached runs on every step and answers "enabled" only within MEMORY_ENABLED_TTL_MS (60 s) of the last enablement check, by the real Date.now(). That cache is warmed only by a memory mirror or delete, a backfill, or the 5-minute poll in manage.ts.
On a linked project with memory on, the "Team memory:" line is present at T+30, gone at T+61, and back after the next check. With the TUI poll, that is at most 60 s of every 5 minutes and two full prefix writes per cycle. I reproduced the line disappearing at t+61 s, on main and on this branch.
The PR says what remains changes "on events rather than a timer", which is not true of this line, and #1633 names losing it as a symptom. The new test "a verified link whose re-check outlasts the deadline renders byte-identical" misses it because setClockForTests steps only the identity memo clock, not Date.now(). The mock proof covered unlinked projects only. The model also lacks the memory guidance for most of a session.
Suggested fix: base this line on a value that does not expire on read, for example the Contents summary's memoryEnabled (it keeps the previous value while refreshing), or a last-known answer per workspace refreshed in the background.
There was a problem hiding this comment.
Fixed in 51ea805: the line now reads memoryEnabledLastKnown (last answer any check gave, per tenant/URL/workspace, no expiry on read), with a stepped-clock test asserting the line and one hash at T+30, T+61, T+120 and two slow re-checks past 5 min.
| // A stale outcome is a link the server could not confirm and that may have changed. Listing | ||
| // that workspace's contents would present it as current, so skip them. | ||
| if (!identity || outcome.status !== "bound" || outcome.stale) return identity | ||
| const contents = await Contents.section(directory, outcome.binding.datamateId).catch(() => "") |
There was a problem hiding this comment.
MINOR (not blocking, pre-existing): the contents block still depends on request timing
- A cold summary slower than
SUMMARY_WAIT_MS(300 ms): step 1 lacks the Integrations, Workspace memory and Knowledge lines and step 2 has them. One extra full write per process on a linked project. - A failed refresh at the 5-minute TTL overwrites the good cached value with
UNKNOWN(contents.ts:258), so those lines vanish for that step. Keepinghit.valueon a failed fetch fixes this one. A merely slow refresh already keeps the old value. - The skills read races
SKILLS_WAIT_MS. Under heavy machine load it rendered "could not be read just now".
I reproduced the first two with a stubbed snapshot and summary, and saw the third in a load-affected run. This runs on every step, so the PR does not introduce it. It is safe to do in a follow-up, but it belongs in the list of what still moves the prefix.
There was a problem hiding this comment.
Failed-refresh case fixed in 36ec951 (keeps hit.value, retries next step, with a test); the cold-summary and skills-read races are left as follow-ups and listed in the PR body as remaining prefix movers.
| ? `${subjectPast} Altimate Workspace id ${id}; ${named}. ` + | ||
| `The ${verifyNoun} could not be re-verified just now, so it may since have changed.` | ||
| : `${subject} Altimate Workspace id ${id}; ${named}.`) + | ||
| `${subject} Altimate Workspace id ${id}; ${named}. ${lastCheckNote(verifyNoun)}` + |
There was a problem hiding this comment.
MINOR (not blocking, edge input): an unconfirmed link is now stated as fact
A resolver-stale outcome means the check failed, but this sentence now reads "This project is linked to … The link is re-checked every few minutes". Main said "was last known … could not be re-verified just now".
If a user asks which workspace this is during an outage, after an unlink elsewhere, the model answers flatly with the old one while /workspace says "Last known link". Tools and sync read the resolver, so this is text only.
Byte equality buys nothing for this state, because contents and the memory line already differ when outcome.stale is set. You could keep a separate sentence for stale bound outcomes at no cache cost.
There was a problem hiding this comment.
Fixed in 51ea805: a stale bound outcome renders the "was last known to be linked … could not be re-verified just now" sentence again; fresh and memoised verified links stay byte-identical.
| expect(ProviderTransform.usesExplicitCacheBreakpoints(gatewayModel)).toBe(true) | ||
| }) | ||
|
|
||
| test("a hoisted task pin rides in the trailing system message; the marked prefix is unchanged", async () => { |
There was a problem hiding this comment.
MINOR (not blocking, test-only): this test passes on main
Copied to the base commit, this test passes, because it builds environment by hand and llm.ts is unchanged. Only the source-text guard at line 149 fails on main in this file, so the move itself is protected by a string match.
There is also no prefix-hash test across N steps with a stepped clock, either through SessionPrompt.loop or on systemSection with Date.now advanced. Such a test would have caught both majors on identity.ts.
|
Findings from the review that have no diff line, plus nits. MINOR (not blocking, pre-existing): a compaction can still re-create the prefix for other reasons (
The mock proof has neither (no lessons, no removed tools), so the "0 changes after compaction" row holds for that setup only. Both are worth checking in the gateway A/B. Nits
Sibling PRs |
…s and the memory TTL - A re-check that outlasts the deadline now reuses a previous `unbound` outcome as well as a `bound` one, so an unlinked project no longer renders "could not be verified" for one step. - The team-memory line reads the last answer an enablement check gave (`memoryEnabledLastKnown`, per tenant, API URL and workspace) instead of the 60 s positive TTL, so it no longer drops a minute after a check. - A link the server could not confirm keeps the hedged "last known" sentence instead of being stated as fact. - Prefix-hash tests over steps with a stepped clock (`setSystemTime`) for an unlinked and a linked project. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A refresh at the 5-minute TTL that fails now keeps the previous summary (and retries next step) instead of replacing it with "not known", which dropped the integrations, memory and knowledge lines for that step. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Replaces the hand-built `LLM.stream` test and the source-text guard (both passed on main) with a loop-level test against a mock OpenAI-compatible server: a builder turn after a plan turn carries the hoisted plan-to-build reminder as system text, and the first system message is byte-identical to the earlier builder turn. Fails with the previous `prompt.ts`. Drops the empty `altimate_change` pair. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Addressed review: slow re-check on an unlinked project keeps the previous outcome (51ea805); team-memory line reads the last enablement answer, not the 60 s TTL (51ea805); unconfirmed link hedged as "last known" again (51ea805); failed summary refresh keeps cached contents (36ec951); stepped-clock prefix-hash tests on |
Issue for this PR
Closes #1633
Type of change
What does this PR do?
The system prompt is the cached prefix of every request, so any change to it between two requests of one session is a full cache miss: the gateway bills the whole prefix (system prompt plus tool schemas, about 75K tokens on a typical repo) again as cache writes at the full input rate, instead of as cache reads at 0.1×. Two parts of the prompt changed for reasons unrelated to the task.
1. The Altimate Workspace section flipped on a timer (
altimate/workspace/identity.ts).It rendered one sentence for a fresh link check and another for a memoised one. For an unlinked repo that was "No Altimate Workspace is linked to this project." vs "…was linked … as of the last check, up to five minutes ago…". The outcome memo is 30 s and the miss memo 5 minutes, so a long session alternated between the two.
Separately, when a re-check outlasted the 1.5 s deadline,
lastKnownmarked the previous (fresh) outcome stale. For a linked repo that removed the workspace contents and the team-memory line from the prompt for that step.The "Team memory:" line was read from the 60 s positive enablement cache, so on a linked project it disappeared a minute after each enablement check and came back on the next one.
Fix: one hedged sentence per verified link state, true whether or not the last check is fresh ("…as of the last check. The link is re-checked every few minutes, so a change made elsewhere may not show yet."), and a slow re-check reuses the previous outcome as it was,
boundorunbound. The team-memory line reads the last answer any enablement check gave (memoryEnabledLastKnown, keyed by tenant, API URL and workspace id, no expiry on read; a failed check keeps it). Only the resolver marks a link stale (the server could not confirm it); that case keeps its own "was last known to be linked … could not be re-verified just now" sentence and still withholds contents and the memory line, as before. A summary refresh that fails at its 5-minute TTL keeps the previous contents instead of replacing them with "not known".2. Hoisted reminders went into the cached system message (
session/prompt.ts).After a compaction, the original-task pin (and the plan-switch reminders) are hoisted out of the user turn for models that are not Anthropic-like (#887/#888). The
altimate-backend/altimate-defaultmodel is registered as OpenAI-compatible, so it takes that path, and the hoisted text was appended to the system array, which is the message carrying the cache breakpoint. The first request after every compaction therefore missed on the whole prefix, not just on the replaced history.Fix: the hoisted text goes with the trailing environment system message, after the breakpoint (
llm.tsalready splits that message out for providers with explicit breakpoints). It is still sent as system text, so the #887 behaviour is unchanged.Behaviour change to review: a verified link reads the same whether its memo is fresh or memoised ("as of the last check"); an unconfirmed (resolver-stale) link still says "last known". The team-memory line now stays on after the last enablement check said "enabled", until a later check says otherwise.
Not in this PR (limits):
lessons.compactre-rendersteamRulesonce a request- or file-tier lesson has been shown, andaddHistoricalToolStubschanges the tool list when a compaction removes the last reference to a gone tool. The mock capture below has neither.How did you verify your code works?
test/session/system-prefix-stability.test.ts(rewritten after review): through the real prompt loop against a mock OpenAI-compatible server on analtimate-backendmodel, a builder turn after a plan turn carries the hoisted plan-to-build reminder as system text, and its first system message is byte-identical to the earlier builder turn's. Fails with the previousprompt.ts. Replaces the hand-builtLLM.streamtest and source-text guard, which passed onmain.identity-section.test.ts, "prefix hash over steps with a stepped clock" (setSystemTimemovesDate.nowfor every module): an unlinked project over six steps including a hanging re-check past the 5-minute miss memo, and a linked project with memory on over six steps (T+30 s, T+61 s, T+2 min, two hanging re-checks past 5 min). One hash each, and the team-memory line on every step. Both failed before the fix (2 hashes; line gone at T+61).contents.test.ts: a failed refresh past the TTL keeps the cached summary and retries next step (failed before the fix).identity.test.ts: memoised and fresh misses render identically; an unconfirmed link is hedged as "last known" (pinned and unpinned).The earlier stale/fresh render tests and the v0.12.1 release-contract tests are updated to the wording above.
No-regression run,
bun test test/altimate/workspace test/session test/skill(packages/opencode), on the merged head before the review fixes (374bfb80e9) and after (3d2a473812):bun test test/altimate test/memory: 10,975 pass, 1 fail,tool CLI > the daemon releases a DuckDB file between calls, which fails the same way with these changes stashed (DuckDB native binding).Typecheck clean;
script/check-tracker-leaks.tsandanalyze.ts --markers --strictpass.Request-body capture of both builds against a local mock gateway (comment): on
mainthe first system message changed twice in a 7-minute session on an unlinked project, and once after a compaction; on this branch it was byte-identical across every request in both scenarios.Still to add: a short gateway A/B (
cache_readon the request after a timer flip and after a compaction, including a linked project and one with lessons).Cost side effects
Reduces cost; adds no model calls, queries or resources. In a headless evaluation of about 5,400 requests, the system-prompt hash changed 785 times; 315 of those were full mid-session cache misses, and prefix re-writes were about 43% of billed tokens. This PR removes the two causes identified there; the remaining misses need the per-request hash log to attribute.
Screenshots / recordings
N/A
Checklist
🤖 Generated with Claude Code