feat(state): add parent_entry_id on the message table for fork support#2308
feat(state): add parent_entry_id on the message table for fork support#2308hongqitai wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces message branching and forking capabilities to the state store by adding current_leaf_id to threads and parent_entry_id to messages, alongside database schema migrations and recursive CTE queries. The code reviewer identified a critical bug where the recursive CTE query returns messages in reverse chronological order, and suggested handling limit == 0 early. Additionally, the reviewer pointed out that using created_at in migrations can cause issues due to duplicate timestamps and recommended using the auto-incrementing id instead. Other feedback includes simplifying an Option<String> type to String and updating test assertions to match chronological ordering.
| Ok(out) | ||
| } | ||
|
|
||
| pub fn set_current_leaf_id(&self, thread_id: &str, current_leaf_id: &str) -> Result<()> { |
There was a problem hiding this comment.
set_current_leaf_id accepts current_leaf_id as &str even though the underlying column is INTEGER and every other call-site works with i64. SQLite silently coerces the string, but callers are forced to call .to_string() on an integer they already hold, which is type-unsafe. The parameter should be i64.
| pub fn set_current_leaf_id(&self, thread_id: &str, current_leaf_id: &str) -> Result<()> { | |
| pub fn set_current_leaf_id(&self, thread_id: &str, current_leaf_id: i64) -> Result<()> { |
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!
…tion in init, update tests
|
fix all problem without coding style, because of I find other code in this crate use "&str" to take int value |
83b848f to
8cb6b91
Compare
…ill also clean current_leaf_id, update tests
8cb6b91 to
734127f
Compare
Summary
Refs #2082 add fork support to state crate, add parent_entry_id to message table and current_leaf_id to threads table
Change
Testing
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-featurescargo test --workspace --all-featuresChecklist
Greptile Summary
This PR adds fork support to the
statecrate by introducingparent_entry_idon themessagestable andcurrent_leaf_idon thethreadstable, enabling a message-tree structure where conversations can branch from any historical message.init_schemais now versioned viaPRAGMA user_version: on first open it runsCREATE TABLE IF NOT EXISTSfor all tables and immediatelyALTER TABLEto add the two new columns, setsuser_version = 1, and wraps everything in aBEGIN/COMMITto make the migration atomic.append_messageis rewritten as a transaction that readscurrent_leaf_id, inserts the new message with that value asparent_entry_id, then advancescurrent_leaf_id;list_messagesswitches from a flatORDER BY created_atto a recursive CTE that walks the parent chain fromcurrent_leaf_id.fork_at_messageinserts a child under an arbitrary existing message and sets it as the new current leaf;list_leaf_messagesreturns all tip nodes;set_current_leaf_idlets callers switch the active branch.Confidence Score: 3/5
The migration parent-chain reconstruction uses strict less-than on a seconds-precision timestamp, which silently truncates conversation history for any existing thread where two or more messages share the same second.
Any thread with rapid message exchanges — the common case for LLM tool-call/response cycles — will have messages with identical created_at values. The migration sets parent_entry_id to NULL for all of them, breaking the ancestor chain. list_messages then returns only the fragment from current_leaf_id to the first orphan, dropping older messages permanently and irreversibly.
crates/state/src/lib.rs — the UPDATE messages SET parent_entry_id subquery in the migration block (lines 243–250).
Important Files Changed
Comments Outside Diff (1)
crates/core/src/lib.rs, line 624-647 (link)persist_threadhard-codescurrent_leaf_id: None, wiping it on every resumepersist_threadbuilds aThreadMetadataliteral withcurrent_leaf_id: None. Becauseupsert_thread'sON CONFLICTclause includescurrent_leaf_id = excluded.current_leaf_id, any call topersist_threadon an existing thread writes NULL over the value thatappend_messagepainstakingly maintained.resume_thread_with_historycallspersist_threadbefore it conditionally appends history (line 502), so resuming a thread that has no new history clearscurrent_leaf_idpermanently — after whichlist_messagesreturns an empty list because the CTE base caseWHERE id = NULLnever matches.Reviews (5): Last reviewed commit: "fix: upsert_thread will not set current_..." | Re-trigger Greptile