fix(session): truncate branch context_messages at fork prefix (#5096 Bug A) - #5124
1 commit merged into
Conversation
Refs nesquena#5096 Co-authored-by: b3nw <b3nw@users.noreply.github.com>
| if len(ctx) == len(msgs): | ||
| return ctx[:keep] | ||
| if len(msgs) == 0: | ||
| return [] | ||
| # Context tail aligns with full display transcript; preserve leading-only rows. | ||
| prefix_len = max(0, len(ctx) - len(msgs)) |
There was a problem hiding this comment.
Unhandled alignment when
len(ctx) < len(msgs). When context_messages is shorter than the display transcript (e.g. after partial context pruning or a future compaction strategy that drops old LLM-only rows), prefix_len becomes 0, so the function returns ctx[:keep] treating context as front-aligned with msgs. But the code's own comment says "Context tail aligns with full display transcript", meaning the correct slice for the shorter case would be ctx[len(ctx) - min(len(ctx), keep):]. As written, a fork at keep=1 with a 2-item ctx and 3-item msgs returns ctx[0] instead of ctx[-2] (the entry that actually corresponds to msgs[0]). Adding an explicit branch or at minimum a guard assertion would prevent silent mistruncation if this case is ever reachable.
| if len(ctx) == len(msgs): | |
| return ctx[:keep] | |
| if len(msgs) == 0: | |
| return [] | |
| # Context tail aligns with full display transcript; preserve leading-only rows. | |
| prefix_len = max(0, len(ctx) - len(msgs)) | |
| if len(ctx) == len(msgs): | |
| return ctx[:keep] | |
| if len(msgs) == 0: | |
| return [] | |
| if len(ctx) < len(msgs): | |
| # Context is shorter than display — tail-aligned; slice from the aligned end. | |
| aligned_keep = max(0, keep - (len(msgs) - len(ctx))) | |
| return ctx[:aligned_keep] | |
| # Context tail aligns with full display transcript; preserve leading-only rows. | |
| prefix_len = len(ctx) - len(msgs) |
| # Context tail aligns with full display transcript; preserve leading-only rows. | ||
| prefix_len = max(0, len(ctx) - len(msgs)) | ||
| prefix = ctx[:prefix_len] | ||
| suffix = ctx[prefix_len:] | ||
| return prefix + suffix[:keep] |
There was a problem hiding this comment.
Compaction prefix always survives the fork regardless of
keep — When prefix_len > 0, prefix + suffix[:keep] always includes all leading compaction-only rows even if keep=1. If a compaction summary was generated after the fork point (i.e. it summarises turns that include post-fork content), those rows will silently leak post-fork knowledge into the branch, which is the exact class of bug this PR targets. The PR notes this as a follow-up for context_engine_state, but the same window exists in the compaction prefix. Consider at minimum adding a test that asserts the expected behaviour so the contract is visible and a future compaction-aware truncation can be slotted in without regression.
| ] | ||
| out = truncate_context_for_display_keep(ctx, msgs, 2) | ||
| assert len(out) == 3 | ||
| assert out[0]["content"] == "compaction-ref-only" No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file — most linters and
git diff warn about this.
| assert out[0]["content"] == "compaction-ref-only" | |
| assert out[0]["content"] == "compaction-ref-only" |
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!
e3271b0
…squena#5096 fork/edit/rewind context bundle nesquena#5124/nesquena#5125/nesquena#5126)
Fix #5096 (Bug A): truncate branch context_messages at fork prefix
Refs #5096 (does not close the full bundle).
Thinking Path
POST /api/session/branchslicedmessagesto the fork prefix but deep-copied the parent’s fullcontext_messages, so the model on the next turn still saw post-fork context (fix(session): rewind/fork leaves stale model context (branch, truncate, edit index) #5096 Bug A).context_messagesis longer thanmessages(compaction-only leading rows).context_messagesto the same semantic prefix as the forked display messages via a shared helper.What Changed
api/session_ops.py—truncate_context_for_display_keep(context_messages, full_messages, keep)aligns context length/shape withfull_messages[:keep].api/routes.py— branch handler setscontext_messagesfromforked_contextinstead of copying the entire parent context.tests/test_issue_branch_context_at_fork.py— unit tests for the helper (no parent-only tail after fork keep).No
CHANGELOG.mdedits.Why It Matters
Forking looked correct in the sidebar but the agent could answer using knowledge from turns the user had intentionally discarded. That breaks trust in branch-as-rewind and matches the failure mode described in #5096.
Verification
./scripts/test.sh tests/test_issue_branch_context_at_fork.py— passed locally after rebase onupstream/master.Risks / Follow-ups
context_engine_stateis still deep-copied from the parent (unchanged). If odd behavior persists after fork, trim engine state in a follow-up.Release note (for maintainers)
Fixed: Branching from a fork point no longer copies the parent’s full model context;
context_messagesare truncated to match the forked transcript prefix.Model Used
x-ai/grok-composer-2.5-fastCo-authored-by: b3nw b3nw@users.noreply.github.com