Skip to content

refs(#2264): Phase 4 — replace Session.messages: Vec<Message> with AppendLog#2579

Open
encyc wants to merge 1 commit into
Hmbown:mainfrom
encyc:feat/phase4-append-log
Open

refs(#2264): Phase 4 — replace Session.messages: Vec<Message> with AppendLog#2579
encyc wants to merge 1 commit into
Hmbown:mainfrom
encyc:feat/phase4-append-log

Conversation

@encyc
Copy link
Copy Markdown
Contributor

@encyc encyc commented Jun 2, 2026

Refs #2264 (Phase 4 — AppendLog backing store).

Summary

Replaces Session.messages: Vec<Message> with AppendLog, the
append-only wrapper introduced in Phase 1. AppendLog now implements
Deref<Target=Vec<Message>> + DerefMut + From/Into, making
it a transparent drop-in — all existing .len(), .iter(),
.push(), .remove(), .clone() calls work without change.

AppendLog changes

  • Add Deref/DerefMut for transparent Vec access
  • Add From<Vec<Message>> and Into<Vec<Message>> for conversions
  • Add into_inner() for explicit unwrapping
  • Remove explicit iter()/len()/is_empty()/as_slice() — Deref
    provides std::slice::Iter (not opaque impl Iterator)

Files

File Change
prompt_zones.rs +28/−25: Deref + From impls
session.rs +2/−2: messages: AppendLog
engine.rs +8/−8: .into() on assignments
turn_loop.rs +2/−2: .into() + .into_inner()
capacity_flow.rs +1/−1: .into()
tests.rs +2/−2: &* deref for assertions

Testing

  • 37 prompt_zones + prefix_cache tests pass
  • cargo check: clean (6 pre-existing schema_migration warnings)

Greptile Summary

This PR wires AppendLog (from Phase 1) as the backing store for Session.messages, replacing the raw Vec<Message>. The conversion is kept transparent by adding Deref<Target=Vec<Message>>, DerefMut, From/Into, and into_inner(), so all existing call sites continue to compile with minimal mechanical changes.

  • AppendLog in prompt_zones.rs gains Deref, DerefMut, and the From/Into pair; the redundant explicit methods (len, is_empty, iter, as_slice) are removed in favour of slice auto-deref.
  • Session.messages in session.rs is retyped to AppendLog; eight assignment sites in engine.rs, capacity_flow.rs, and turn_loop.rs add .into() to convert Vec<Message> results into AppendLog.
  • Two test assertions are updated to use &* to explicitly deref AppendLog to &Vec<Message> for comparison.

Confidence Score: 4/5

Safe to merge — the mechanical conversions are all correct and cargo check is clean. The only concern is a design gap in prompt_zones.rs that future contributors should be aware of.

All Vec to AppendLog assignment sites are converted correctly, type inference via From/Into is sound, and the test updates are accurate. The one open question is DerefMut: it fully exposes the inner Vec for arbitrary mutation (remove, truncate, clear, last_mut), so the append-only contract is documented but not enforced at the type level. Existing callers already depend on those mutations, so no behaviour changes with this PR, but the abstraction is now leaky and could mislead future contributors into thinking the type guards against removals or rewrites.

crates/tui/src/prompt_zones.rs — the DerefMut implementation and its interaction with the append-only doc-comment.

Important Files Changed

Filename Overview
crates/tui/src/prompt_zones.rs Adds Deref/DerefMut, From/Into, and into_inner() to AppendLog; removes the explicit iter/len/is_empty/as_slice methods. DerefMut fully exposes the inner Vec for arbitrary mutation, undermining the append-only invariant the type is named and documented for.
crates/tui/src/core/session.rs Changes Session.messages field from Vec to AppendLog and initialises it with AppendLog::new(); minimal and correct change.
crates/tui/src/core/engine.rs Updates eight assignment sites from direct Vec assignment to .into() conversions; all existing .remove(), .truncate(), .len(), and .iter() calls continue to work via DerefMut/Deref.
crates/tui/src/core/engine/turn_loop.rs Converts one assignment site to .into() and changes messages_with_turn_metadata() to use clone().into_inner(); the latter allocates an unnecessary intermediate AppendLog.
crates/tui/src/core/engine/capacity_flow.rs Single assignment converted to .into(); no other changes.
crates/tui/src/core/engine/tests.rs Updates two assertions to use &* deref to compare Vec with &*AppendLog; change is correct.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["result.messages: Vec<Message>"] -->|".into()"| B["AppendLog"]
    B -->|"Deref"| C["&Vec<Message> (read-only callers)"]
    B -->|"DerefMut"| D["&mut Vec<Message> (remove / truncate / clear / last_mut)"]
    B -->|".clone().into() or .clone().into_inner()"| E["Vec<Message> (SessionUpdated / API calls)"]
    B -->|"into_inner()"| F["Vec<Message> (consumed)"]
    G["Session.messages: AppendLog"] --- B
Loading

Fix All in Codex Fix All in Claude Code Fix All in Cursor

Reviews (1): Last reviewed commit: "refs(#2264): Phase 4 — replace Session.m..." | Re-trigger Greptile

Greptile also left 2 inline comments on this PR.

…ith AppendLog

AppendLog gains Deref<Target=Vec<Message>> + DerefMut + From/Into impls,
making it a transparent drop-in for Vec<Message>. All existing .len(),
.iter(), .push(), .remove(), .clone() calls work through Deref.

- AppendLog: add Deref/DerefMut, From<Vec>/Into<Vec>, into_inner()
- Remove explicit iter()/len()/is_empty()/as_slice() — Deref provides
  the correct return types (std::slice::Iter vs impl Iterator)
- Session.messages: Vec<Message> → AppendLog
- 10 assignment sites: add .into() or .into_inner()
- 2 test assertions: compare &Vec references via &* deref
@gemini-code-assist
Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 2, 2026

Thanks @encyc for taking the time to contribute.

This repository is currently observing a maintainer-managed contribution gate in dry-run mode, so this pull request is staying open. When enforcement is enabled, pull requests from contributors who are not listed in .github/APPROVED_CONTRIBUTORS will be closed automatically.

Please read CONTRIBUTING.md for the expected contribution shape. A maintainer can grant PR access by commenting /lgtm on a pull request.

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

// 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant