Skip to content

fix(daemon): move connected-MCP directive out of the cached system prompt - #5336

Merged
AmyShang-alt merged 4 commits into
mainfrom
fix/prompt-cache-mcp-drift
Jul 10, 2026
Merged

fix(daemon): move connected-MCP directive out of the cached system prompt#5336
AmyShang-alt merged 4 commits into
mainfrom
fix/prompt-cache-mcp-drift

Conversation

@lefarcen

@lefarcen lefarcen commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Why

The ## External MCP servers — already authenticated directive is composed inside daemonSystemPrompt, one of the three parts that make up the cacheable system prefix sent to the model (and hashed into the stable-instruction fingerprint). Its membership is driven by live OAuth Bearer validity (isTokenExpired, mcp-tokens.ts): a server drops out of the list the moment its token crosses expiry, and comes back on refresh/reconnect.

That makes the directive a mid-conversation drift source in the cached prefix. On Bedrock/Anthropic prompt caching the cache is prefix-matched, and the conversation history (which AMR/Claude-on-Bedrock caches to avoid re-billing 60–85% of the prompt every turn — the basis for the recently-landed vela link history caching) sits after the system prompt. So any byte change in the system prompt — including this directive flipping as a token expires — invalidates the whole downstream prefix, history cache included.

Production telemetry backs this up. run_finished.stable_prompt_cache_miss_reason='stable-prompt-changed' fires on ~10% of AMR resume turns (and 20–31% for other native-resume runtimes), and the rate rises with conversation length (2% at turn 0 → ~10% by turn 5–6), exactly the signature of tokens crossing expiry over a longer session. Root-causing the fingerprint parts pinned this directive (plus the auto-extracted memory block) as the dominant cause. The cost impact today is modest (drifted turns still hit ~0.87 vs ~0.95), but it's pure waste and grows as the history-cache reliance grows.

What users will see

Nothing visible. This is an internal prompt-cache-stability change. The model still receives the exact same "these external MCP servers are already authenticated, don't chase the synthetic *_authenticate tools" guidance — it just arrives in the per-turn instruction slice (next to the cwd / run-context hints) instead of the cached system prompt. Functionally it's actually a touch more correct: the directive now reflects the live MCP auth state on every turn, instead of the snapshot frozen at the turn the session was seeded.

What changed

  • prompts/system.ts: composeSystemPrompt no longer takes connectedExternalMcp or emits the directive. renderConnectedExternalMcpDirective is exported and its leading --- separator dropped so it composes cleanly inside a ----joined slice.
  • server.ts: render the directive from the already-computed connectedExternalMcp and append it to clientInstructionParts (both the include-stable and resume branches, so it's sent every turn). The connectedExternalMcp param is dropped from the composeDaemonSystemPromptcomposeSystemPrompt chain.
  • Net effect: daemonSystemPrompt (the cached prefix) is now invariant to MCP token expiry → the stable fingerprint stops drifting from this cause → the upstream history cache stays warm across resumes.

Testing

  • pnpm --filter @open-design/daemon typecheck — clean (src + tests).
  • pnpm --filter @open-design/daemon exec vitest run tests/prompts/system.test.ts — 46 passed. The old connectedExternalMcp directive suite is rewritten: composeSystemPrompt now asserts the directive is never in the system prompt, and a new renderConnectedExternalMcpDirective suite covers the moved renderer (empty→'', server list, blank-id skip, no label dup, no leading separator).
  • pnpm guard — pass (78/0).
  • Verified the change cannot affect the unknown-agent failure path (fails at server.ts:4071, before the new code at 4691).

Follow-up (not in this PR)

The secondary drift source — the auto-extracted personal-memory block re-ordering by file mtime — is left for a separate change since it's more behavior-sensitive (it changes the fact order the model sees). Tracked via the AMR prompt-cache drift dashboard.

…ompt

The '## External MCP servers — already authenticated' directive was
composed into daemonSystemPrompt, part of the cacheable system prefix
sent upstream. Its membership is driven by live OAuth Bearer validity
(isTokenExpired), so a token expiring/refreshing/reconnecting mid-
conversation changes the directive -> changes the system prompt bytes ->
invalidates the whole downstream prompt-cache prefix, including the
conversation-history cache AMR/Claude-on-Bedrock relies on. Production
telemetry (stable_prompt_cache_miss_reason='stable-prompt-changed')
attributes the bulk of resume-turn prefix drift to this directive, rising
with conversation length as more tokens cross expiry.

Move it from composeSystemPrompt into the per-turn client instruction
slice (alongside runContext/cwd hints): re-sent uncached every turn,
landing after the cached system+history prefix. The model still sees the
current MCP auth state each turn (more accurately -- live state, not the
seed snapshot), while the cacheable prefix stays byte-stable across
resumes. renderConnectedExternalMcpDirective is exported and its leading
separator dropped so it composes cleanly in the joined slice;
composeSystemPrompt no longer takes connectedExternalMcp.
@lefarcen
lefarcen requested a review from nettee July 9, 2026 07:23
@lefarcen lefarcen added size/M PR changes 100-300 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix needs-validation Runtime change detected; needs human or /explore agent validation. labels Jul 9, 2026
@lefarcen

lefarcen commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Added needs-validation because this touches a live daemon/runtime path — if this regressed, users would feel it in resume/prompt behavior rather than just internal telemetry.

One small PR-body follow-up: could you tick ## Surface area and mark None here? The rest of the write-up and validation details are already clear.

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@lefarcen I traced the moved MCP directive through composeSystemPrompt, composeDaemonSystemPrompt, and the create/resume instruction assembly in server.ts; the changed flow keeps the stable prompt fingerprint free of live token drift while still sending the authenticated-server hint on every turn. The rewritten prompt tests also cover the extracted renderer behavior and the fact that the directive no longer lands in the cached system prompt.

I could not rerun the daemon checks in this prepared reviewer worktree because node_modules are absent here, but I do not see any actionable issues in the changed ranges. Nice separation of the volatile auth-state hint from the cacheable prefix.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen

lefarcen commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

🧪 Queued for QA validation now that review is in and CI is green.

One last PR-body follow-up before this wraps: please tick Surface area as None and add the bug-fix verification note for the cache-drift regression path you validated.

@lefarcen
lefarcen requested a review from AmyShang-alt July 9, 2026 07:40
…the per-turn slice

Drives a real two-turn codex session (native resume) with a connected
external MCP server (enabled config + live OAuth Bearer) through the
daemon HTTP boundary, capturing the exact prompt each turn reaches the
agent's stdin. Asserts the '## External MCP servers — already
authenticated' directive is present on turn 1 AND re-sent on the turn-2
clean resume — which only holds once the directive rides in the per-turn
instruction slice instead of the cached stable block. Verified red on
origin/main (turn-2 stdin lacks the directive because the cached block is
not re-sent on a clean resume) and green with the fix.
@lefarcen

lefarcen commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Added an end-to-end red-spec (per @request)

The unit tests only prove the composition (composeSystemPrompt no longer emits the directive). They don't cover the server.ts wiring — that the directive actually reaches the model in the per-turn slice at runtime, including on resume turns. Added a real daemon-HTTP-boundary e2e for exactly that:

apps/daemon/tests/codex-session-resume.test.ts"re-sends the connected-MCP directive in the per-turn slice on resume turns"

  • Drives a real 2-turn codex native-resume session through the daemon, with a connected external MCP server (enabled config + a live OAuth Bearer via writeMcpConfig + setToken), capturing the exact prompt each turn writes to the agent's stdin.
  • Turn 1 (seed): directive present. Turn 2 (clean resume): the cached stable block is not re-sent, but the MCP directive IS — which only holds once it rides in the per-turn slice.

Red-on-main / green-on-branch verified:

  • git checkout origin/main -- system.ts server.ts → test FAILS: resume.stdin is # Instructions … ## Design Files … with no External MCP servers — already authenticated (on main the directive sits in the cached stable block, which a clean resume doesn't re-send).
  • With the fix → passes.

So the fix's runtime behavior is now covered by a falsifiable spec, not just the composition-level unit tests.

@lefarcen
lefarcen requested a review from nettee July 9, 2026 08:14

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I reviewed the prompt move and the new resume-path coverage. The runtime wiring in server.ts looks consistent with the cache-stability goal, but I found one test-isolation issue in the new red-spec that is worth fixing before this suite grows more order-dependent.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Comment thread apps/daemon/tests/codex-session-resume.test.ts
@lefarcen

lefarcen commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Hey @lefarcen@nettee's isolation note on the new red-spec looks like the main remaining follow-up on this head: the test should avoid leaking MCP config/token state through the shared OD_DATA_DIR.

Once that cleanup is in, please also fold in the two PR-body nits already noted above (Surface area = None + the bug-fix verification note), and this should be ready for the next pass.

The new test writes an authenticated github MCP server into the
process-wide OD_DATA_DIR (tests/setup.ts shares one data root for the
whole daemon Vitest run) and the shared afterEach never reset it, so the
connected-MCP state could leak into later test files and make them
suite-order dependent. Reset the MCP config (empty servers) and clear the
github token in afterEach — both idempotent no-ops for the other tests in
this file.
@lefarcen
lefarcen requested a review from nettee July 9, 2026 11:35

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I traced the moved MCP directive through composeSystemPrompt, the create/resume instruction assembly in server.ts, and the new resume-path red-spec. The runtime change still looks aligned with the cache-stability goal, but there is one remaining test-cleanup issue in the new hermetic follow-up worth tightening.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Comment thread apps/daemon/tests/codex-session-resume.test.ts Outdated
Drop the unconditional .catch(() => {}) around the afterEach MCP reset.
writeMcpConfig always overwrites to an empty server list and clearToken
is a documented no-op when the entry is absent, so neither needs the
catch — and swallowing a real teardown failure would silently let MCP
state leak into later tests, defeating the isolation guarantee.
@lefarcen
lefarcen requested a review from nettee July 9, 2026 11:42

@nettee nettee left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@lefarcen I rechecked the final head after the follow-up test cleanup. The connected-MCP directive now stays out of the stable prompt fingerprint, is rendered from the live authenticated-server set in the per-turn instruction slice for both create and resume paths, and the updated prompt/unit coverage plus the resume-path red-spec line up with that behavior. I could not rerun the daemon test commands in this prepared reviewer worktree because node_modules are absent here, but the final teardown change now fails fast instead of masking MCP-state leakage, which was the remaining review concern on the prior head. Nice cleanup on the follow-up.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen

lefarcen commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Hey @lefarcen — on this head, @nettee's remaining cleanup note is the teardown path in codex-session-resume.test.ts: the MCP reset should fail fast rather than swallow real cleanup errors with broad .catch(() => {}) wrappers.

Once that lands, the two PR-body follow-ups already noted above are still worth folding in as well (Surface area = None + the bug-fix verification note).

@lefarcen
lefarcen added this pull request to the merge queue Jul 9, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 9, 2026
@AmyShang-alt AmyShang-alt added validated Runtime change validated (via /explore Pass or manual QA). and removed needs-validation Runtime change detected; needs human or /explore agent validation. labels Jul 10, 2026

@AmyShang-alt AmyShang-alt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

QA 验收记录

验收范围:

  • daemon prompt cache 稳定性:connected external MCP directive 从 cached system prompt 移到 per-turn instruction slice。
  • codex native resume 路径:clean resume turn 仍应收到 connected-MCP directive。

已验证:

  • PR head: c96993c
  • pnpm --filter @open-design/daemon typecheck 通过。
  • pnpm --filter @open-design/daemon exec vitest run tests/prompts/system.test.ts 通过,46 passed。
  • pnpm --filter @open-design/daemon exec vitest run tests/codex-session-resume.test.ts 通过,5 passed。
  • GitHub checks 当前为绿;scope 跳过项不是失败。

未验证:

  • 未做 Electron 手验;本 PR 无用户可见 UI,关键风险已由 daemon typecheck + resume-path Vitest 覆盖。
  • 未查 PostHog 指标回落;本 PR 未改 analytics capture,仅使用现有 telemetry 作为背景说明。

风险/关注点:

  • 这是 daemon/runtime prompt 路径,回归会影响 resume turn 的 MCP auth 指引或 prompt cache 命中。
  • 当前新增红规已覆盖核心风险:stable block 不重发时,MCP directive 仍在 per-turn slice 里发送。

结论:

  • 验收通过,可移除 needs-validation

@AmyShang-alt
AmyShang-alt added this pull request to the merge queue Jul 10, 2026
Merged via the queue into main with commit 9b5cdd8 Jul 10, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/M PR changes 100-300 lines type/bugfix Bug fix validated Runtime change validated (via /explore Pass or manual QA).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants