fix: maintain conversation history across ACP session/prompt turns#3372
fix: maintain conversation history across ACP session/prompt turns#3372xulongzhe wants to merge 2 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Thanks @xulongzhe for taking the time to contribute. This repository is observing a maintainer-managed PR intake gate in dry-run mode, so this pull request is staying open. This note helps maintainers prepare the allowlist before any enforcement is considered. Please read |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces session-based message history tracking in AcpServer by adding a messages list to AcpSession. This allows sending the full conversation history to the LLM instead of just the single latest prompt. The feedback suggests combining two consecutive lookups of the session in the sessions map into a single mutable lookup to avoid redundant map queries and improve performance.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // Append user message to session history | ||
| { | ||
| let session = self | ||
| .sessions | ||
| .get_mut(&session_id) | ||
| .ok_or_else(|| AcpError::invalid_params("unknown sessionId"))?; | ||
| session.messages.push(Message { | ||
| role: "user".to_string(), | ||
| content: vec![ContentBlock::Text { | ||
| text: prompt, | ||
| cache_control: None, | ||
| }], | ||
| }); | ||
| } | ||
|
|
||
| // Clone messages for the LLM call (avoids borrowing self across await) | ||
| let (messages, cwd) = { | ||
| let session = self | ||
| .sessions | ||
| .get(&session_id) | ||
| .ok_or_else(|| AcpError::invalid_params("unknown sessionId"))?; | ||
| (session.messages.clone(), session.cwd.clone()) | ||
| }; |
There was a problem hiding this comment.
The session is looked up twice in the sessions map: first mutably to append the user message, and then immutably to clone the messages and cwd. These two lookups can be combined into a single mutable lookup to avoid redundant map queries and improve performance.
| // Append user message to session history | |
| { | |
| let session = self | |
| .sessions | |
| .get_mut(&session_id) | |
| .ok_or_else(|| AcpError::invalid_params("unknown sessionId"))?; | |
| session.messages.push(Message { | |
| role: "user".to_string(), | |
| content: vec![ContentBlock::Text { | |
| text: prompt, | |
| cache_control: None, | |
| }], | |
| }); | |
| } | |
| // Clone messages for the LLM call (avoids borrowing self across await) | |
| let (messages, cwd) = { | |
| let session = self | |
| .sessions | |
| .get(&session_id) | |
| .ok_or_else(|| AcpError::invalid_params("unknown sessionId"))?; | |
| (session.messages.clone(), session.cwd.clone()) | |
| }; | |
| // Append user message to session history and clone for the LLM call (avoids borrowing self across await) | |
| let (messages, cwd) = { | |
| let session = self | |
| .sessions | |
| .get_mut(&session_id) | |
| .ok_or_else(|| AcpError::invalid_params("unknown sessionId"))?; | |
| session.messages.push(Message { | |
| role: "user".to_string(), | |
| content: vec![ContentBlock::Text { | |
| text: prompt, | |
| cache_control: None, | |
| }], | |
| }); | |
| (session.messages.clone(), session.cwd.clone()) | |
| }; |
- Add messages: Vec<Message> to AcpSession for per-session history - Append user message before LLM call, assistant response after - Pass full conversation history to LLM instead of single-message request - Add unit tests for session history accumulation - Add Debug derive to AcpError for test compatibility - Run cargo fmt
xulongzhe
force-pushed
the
fix/acp-multi-turn-context
branch
from
cc3a373 to
a37038d
Compare
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
Merge get_mut + get into a single get_mut in prompt(), as suggested by Gemini Code Assist review.
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Thanks @xulongzhe — I carried this into the v0.8.64 integration branch with your two commits cherry-picked and original authorship preserved:
Verified on the integration branch with:
I am keeping this PR open until the integration branch lands so the public repo state stays accurate. Appreciate the focused ACP repro and fix. |
2 participants
Summary
Fix multi-turn conversation context loss in ACP server. The
AcpSessionstruct only storedcwdwith no conversation history, causingrun_prompt()to always construct a single-message request. Eachsession/promptcall was treated as an independent conversation, losing all prior context.Changes:
messages: Vec<Message>toAcpSessionfor per-session conversation historyDebugderive toAcpErrorfor test compatibilityReproduction:
session/new"1+1等于几？只回答数字"→ LLM answers"2"✓"再加一等于几？只回答数字"→ LLM answers"2"✗ (should be"3")Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-features3-turn arithmetic chain integration test (1+1→2, add one→3, add one→4):
Unit tests added in
acp_server.rs:new_session_starts_with_empty_messagesprompt_appends_user_and_assistant_messages_to_historydifferent_sessions_have_independent_historyChecklist