Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions crates/tui/src/core/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1091,7 +1091,7 @@ impl Engine {
} else if messages.is_empty() && system_prompt.is_none() {
self.session.id = uuid::Uuid::new_v4().to_string();
}
self.session.messages = messages;
self.session.messages = messages.into();
self.session.compaction_summary_prompt =
extract_compaction_summary_prompt(system_prompt.clone());
self.session.system_prompt = system_prompt;
Expand Down Expand Up @@ -1182,7 +1182,7 @@ impl Engine {
.tx_event
.send(Event::SessionUpdated {
session_id: self.session.id.clone(),
messages: self.session.messages.clone(),
messages: self.session.messages.clone().into(),
system_prompt: self.session.system_prompt.clone(),
model: self.session.model.clone(),
workspace: self.session.workspace.clone(),
Expand Down Expand Up @@ -1673,7 +1673,7 @@ impl Engine {
Ok(result) => {
if !result.messages.is_empty() || self.session.messages.is_empty() {
let messages_after = result.messages.len();
self.session.messages = result.messages;
self.session.messages = result.messages.into();
self.merge_compaction_summary(result.summary_prompt);
self.emit_session_updated().await;
let removed = messages_before.saturating_sub(messages_after);
Expand Down Expand Up @@ -1769,7 +1769,7 @@ impl Engine {
{
Ok(result) => {
let messages_after = result.messages.len();
self.session.messages = result.messages;
self.session.messages = result.messages.into();
self.emit_session_updated().await;

let summary = format!(
Expand Down Expand Up @@ -1864,7 +1864,7 @@ impl Engine {
{
Ok(result) => {
retries_used = result.retries_used;
compacted_messages = result.messages;
compacted_messages = result.messages.into();
summary_prompt = result.summary_prompt;
}
Err(err) => {
Expand Down Expand Up @@ -1952,7 +1952,7 @@ impl Engine {
self.session.model.clone(),
self.session.workspace.clone(),
self.session.system_prompt.clone(),
self.session.messages.clone(),
self.session.messages.clone().into(),
))
.with_cancel_token(self.cancel_token.clone())
.with_trusted_external_paths(trusted_external_paths);
Expand Down Expand Up @@ -2310,7 +2310,7 @@ impl Engine {
);

// 5. Atomic swap.
self.session.messages = seed_messages;
self.session.messages = seed_messages.into();
self.session.cycle_count = to;
self.session.current_cycle_started = now;
self.session.cycle_briefings.push(briefing.clone());
Expand Down
2 changes: 1 addition & 1 deletion crates/tui/src/core/engine/capacity_flow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ impl Engine {
{
Ok(result) => {
if !result.messages.is_empty() || self.session.messages.is_empty() {
self.session.messages = result.messages;
self.session.messages = result.messages.into();
self.merge_compaction_summary(result.summary_prompt);
refreshed = true;
}
Expand Down
4 changes: 2 additions & 2 deletions crates/tui/src/core/engine/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2104,7 +2104,7 @@ fn messages_with_turn_metadata_preserves_stored_messages_for_prefix_cache() {
let first_user = engine.user_text_message_with_turn_metadata("inspect src/lib.rs".to_string());
engine.session.add_message(first_user.clone());
let first_request = engine.messages_with_turn_metadata();
assert_eq!(first_request, engine.session.messages);
assert_eq!(&first_request, &*engine.session.messages);

engine.session.add_message(Message {
role: "assistant".to_string(),
Expand All @@ -2121,7 +2121,7 @@ fn messages_with_turn_metadata_preserves_stored_messages_for_prefix_cache() {
engine.session.add_message(second_user);

let second_request = engine.messages_with_turn_metadata();
assert_eq!(second_request, engine.session.messages);
assert_eq!(&second_request, &*engine.session.messages);
assert_eq!(second_request.first(), Some(&first_user));
}

Expand Down
4 changes: 2 additions & 2 deletions crates/tui/src/core/engine/turn_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ impl Engine {
// Only update if we got valid messages (never corrupt state)
if !result.messages.is_empty() || self.session.messages.is_empty() {
let auto_messages_after = result.messages.len();
self.session.messages = result.messages;
self.session.messages = result.messages.into();
self.merge_compaction_summary(result.summary_prompt);
self.emit_session_updated().await;
let removed = auto_messages_before.saturating_sub(auto_messages_after);
Expand Down Expand Up @@ -2219,7 +2219,7 @@ impl Engine {
// appended. Do not rewrite historical messages at request time: doing
// so makes the API prefix differ from the bytes sent in earlier turns
// and destroys DeepSeek's KV prefix cache reuse.
self.session.messages.clone()
self.session.messages.clone().into_inner()
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.

P2 clone().into_inner() allocates a full AppendLog clone just to immediately unwrap it into Vec<Message>. Since Deref<Target = Vec<Message>> is now implemented, the inner Vec can be cloned directly without constructing the intermediate AppendLog.

Suggested change
self.session.messages.clone().into_inner()
(*self.session.messages).clone()

Fix in Codex Fix in Claude Code Fix in Cursor

}
}

Expand Down
8 changes: 4 additions & 4 deletions crates/tui/src/core/session.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ use crate::cycle_manager::CycleBriefing;
use crate::models::{Message, SystemPrompt, Usage};
use crate::prefix_cache::PrefixStabilityManager;
use crate::project_context::{ProjectContext, load_project_context_with_parents};
use crate::prompt_zones::FrozenPrefix;
use crate::prompt_zones::{AppendLog, FrozenPrefix};
use crate::tui::approval::ApprovalMode;
use crate::working_set::WorkingSet;
use chrono::{DateTime, Utc};
Expand Down Expand Up @@ -42,8 +42,8 @@ pub struct Session {
/// Persisted summary blocks generated by context compaction.
pub compaction_summary_prompt: Option<SystemPrompt>,

/// Conversation history (API format)
pub messages: Vec<Message>,
/// Conversation history (API format), backed by AppendLog (#2264).
pub messages: AppendLog,

/// Total tokens used in this session
pub total_usage: SessionUsage,
Expand Down Expand Up @@ -152,7 +152,7 @@ impl Session {
system_prompt: None,
system_prompt_override: false,
compaction_summary_prompt: None,
messages: Vec::new(),
messages: AppendLog::new(),
total_usage: SessionUsage::default(),
allow_shell,
trust_mode,
Expand Down
46 changes: 29 additions & 17 deletions crates/tui/src/prompt_zones.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,16 +195,15 @@ impl std::fmt::Display for PrefixDrift {

// ── AppendLog ──────────────────────────────────────────────────────────

/// Append-only conversation history. Only exposes `push`-style mutations.
/// Append-only conversation history. Derefs to [`Vec<Message>`] for
/// transparent read access; mutations go through `push()` / `From`.
///
/// **Phase 1 scaffolding** — not yet wired into the engine request path.
#[allow(dead_code)]
/// Phase 4: backing store for `Session.messages` (#2264).
#[derive(Debug, Clone)]
pub struct AppendLog {
messages: Vec<Message>,
}

#[allow(dead_code)]
impl AppendLog {
pub fn new() -> Self {
Self {
Expand All @@ -216,33 +215,47 @@ impl AppendLog {
Self { messages }
}

/// Append a message to the log.
pub fn push(&mut self, message: Message) {
self.messages.push(message);
}

/// Consume and return the inner `Vec<Message>`.
#[must_use]
pub fn len(&self) -> usize {
self.messages.len()
pub fn into_inner(self) -> Vec<Message> {
self.messages
}
}

#[must_use]
pub fn is_empty(&self) -> bool {
self.messages.is_empty()
impl Default for AppendLog {
fn default() -> Self {
Self::new()
}
}

impl From<Vec<Message>> for AppendLog {
fn from(messages: Vec<Message>) -> Self {
Self { messages }
}
}

pub fn iter(&self) -> impl Iterator<Item = &Message> {
self.messages.iter()
impl From<AppendLog> for Vec<Message> {
fn from(log: AppendLog) -> Self {
log.messages
}
}

#[must_use]
pub fn as_slice(&self) -> &[Message] {
impl std::ops::Deref for AppendLog {
type Target = Vec<Message>;

fn deref(&self) -> &Self::Target {
&self.messages
}
}

impl Default for AppendLog {
fn default() -> Self {
Self::new()
impl std::ops::DerefMut for AppendLog {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.messages
}
}
Comment on lines +256 to 260
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.

P2 DerefMut exposes the full Vec<Message> mutation API, silently defeating the append-only contract. Callers in the current codebase already reach through it to call .remove(0) (engine.rs:1821), .truncate(idx) (engine.rs:1139), .clear() (capacity_flow.rs:690), last_mut() (turn_loop.rs:1127), and .extend_from_slice() (session_manager.rs:745). The type's name and doc-comment still say "append-only", but there is no compile-time barrier left to prevent arbitrary removals or rewrites — any future call site can silently violate the invariant without the compiler warning. If the intent is for AppendLog to enforce the constraint, DerefMut should be removed and explicit methods added for each needed mutation. If unrestricted mutation is acceptable, the "append-only" branding in the doc-comment should be updated to avoid misleading future readers.

Suggested change
impl std::ops::DerefMut for AppendLog {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.messages
}
}
// NOTE: DerefMut is intentionally omitted. Exposing &mut Vec<Message>
// would allow callers to call .remove() / .truncate() / .clear() etc.,
// defeating the append-only invariant. Add explicit methods for each
// permitted mutation instead (e.g. `truncate`, `clear`, `remove_first`).
//
// impl std::ops::DerefMut for AppendLog { … } ← deliberately absent

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

Fix in Codex Fix in Claude Code Fix in Cursor


Expand Down Expand Up @@ -525,7 +538,6 @@ mod tests {
let msgs = vec![make_message("user", "a"), make_message("assistant", "b")];
let log = AppendLog::from_messages(msgs);
assert_eq!(log.len(), 2);
assert_eq!(log.as_slice().len(), 2);
}

// ── TurnScratch ───────────────────────────────────────────────
Expand Down
Loading