Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

fix(session): align fork/undo context truncation to wire turns#2520

Open
Nas01010101 wants to merge 3 commits into
MoonshotAI:mainMoonshotAI/kimi-cli:mainfrom
Nas01010101:fix-fork-context-alignmentNas01010101/kimi-cli:fix-fork-context-alignmentCopy head branch name to clipboard
Open

fix(session): align fork/undo context truncation to wire turns#2520
Nas01010101 wants to merge 3 commits into
MoonshotAI:mainMoonshotAI/kimi-cli:mainfrom
Nas01010101:fix-fork-context-alignmentNas01010101/kimi-cli:fix-fork-context-alignmentCopy head branch name to clipboard

Conversation

@Nas01010101

Copy link
Copy Markdown

Related Issue

Resolve #2517

Also resolves #1974 (wire-only slash turns shifting the undo cut — covered by a dedicated regression test) and likely the root cause of #2049 (history mismatch after forks/undos).

Relation to open PR #2386: that PR maps wire turns to context turns for the slash-command case (credit to its author for identifying that direction). It does not handle synthetic user-role records in the context — compaction summaries, steers, notifications — so compacted/steered sessions still cut at the wrong record, and the compaction case leaks "undone" turns into the model's context. This PR covers both directions with tests for each; happy to instead rebase the additional coverage on top of #2386 if the maintainers prefer to land that one first.

Description

truncate_context_at_turn() counts non-checkpoint role="user" records to find the cut point for /undo and /fork, but the context file is not 1:1 with wire turns: compaction rewrites earlier turns into one user-role summary record, steers and notifications inject extra user-role records, and slash-command turns produce no context record at all. After any of these, the context is truncated at the wrong record:

Fix: when the wire turn texts are available (from enumerate_turns, which fork_session already computes for wire truncation), align context user records to wire turns by matching extracted user text, consuming turns in order. Records that match no remaining wire turn (summaries, steers, notifications) no longer advance the turn counter, and turns with no context record (slash commands, compacted-away turns) no longer consume a user record. Positional counting is kept as an explicit fallback when wire texts are unavailable, preserving the existing best-effort semantics.

Real-behavior proof (deterministic, local mock provider)

Build a real session: turn oneturn two/compactturn three; then fork_session(dir, work_dir, turn_index=2) (the exact /undo code path):

  • on main: forked wire shows turns 0–2, but forked context.jsonl still contains user: "turn three" and its assistant reply
  • on this branch: forked context ends after the turn two block ✅

Verification

  • New tests in tests/core/test_session_fork.py::TestTruncateContextAlignment (4 fail on main):
    compaction summary not counted as a turn; steer not counted; duplicate turn texts align in order; end-to-end fork_session after compaction drops the undone turn; positional fallback unchanged.
  • uv run pytest tests/core/test_session_fork.py → 34 passed; full tests/ suite green.
  • ruff / pyright clean.

Checklist

  • I have read the CONTRIBUTING document.
  • I have linked the related issue (file the draft issue first).
  • I have added tests that prove my fix is effective.
  • Changelog updated (manual ## Unreleased entry; make gen-changelog needs kimi API auth).
  • make gen-docs — N/A: no user-facing docs change.

truncate_context_at_turn counted every non-checkpoint user-role record
as a turn boundary, but the context file is not 1:1 with wire turns:
compaction rewrites earlier turns into a single user-role summary
record, steers inject extra user-role records mid-turn, and slash
command turns never touch the context. After any of these, /undo and
/fork truncated the context at the wrong record — e.g. rewinding past a
post-compaction turn kept that turn in the model's context while the
UI no longer showed it. Align context user records to wire turns by
matching user text (positional counting kept as fallback when wire
texts are unavailable).
Copilot AI review requested due to automatic review settings July 19, 2026 22:19

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI left a comment

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Nas01010101

Copy link
Copy Markdown
Author

For reviewer transparency — two edge cases the text-alignment approach cannot fully resolve (now documented in the truncate_context_at_turn docstring, pushed as a docs commit):

  1. Context-mutating slash turns (e.g. /init appends a user-role instruction record): the injected record's text differs from the wire turn text ("/init"), so it cannot be attributed to its turn and is conservatively kept even when that turn is rewound past.
  2. Exact-text collision: a steer whose text is exactly equal to a later turn's user text is indistinguishable from that turn's start and aligns to it, cutting slightly early.

Both predate this PR — positional counting handled them strictly worse (every compaction/steer/slash turn corrupted the cut, not just these collisions) — and both are inherently ambiguous for any content-based heuristic: I checked LCS/sequence alignment (difflib) and it resolves the collision case identically, and substring-matching slash injections trades a rare false-negative for likelier false-positives on user text that merely mentions a slash command.

The clean resolution for both is the explicit _turn marker record in context.jsonl proposed as the protocol-level option in #2517 — happy to implement that as a follow-up if you'd prefer it over (or on top of) this alignment.

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

Labels

None yet

Projects

None yet

2 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.