Skip to content

fix(acp): replay loaded session history#2363

Open
huntharo wants to merge 3 commits into
MoonshotAI:mainfrom
huntharo:fix/acp-load-history-message-ids
Open

fix(acp): replay loaded session history#2363
huntharo wants to merge 3 commits into
MoonshotAI:mainfrom
huntharo:fix/acp-load-history-message-ids

Conversation

@huntharo
Copy link
Copy Markdown

@huntharo huntharo commented May 24, 2026

Summary

This PR builds on #2359 and includes all current commits from that PR plus the session-load follow-up:

  1. fix(acp): assign message ids to streamed content #2359: ACP SDK 0.10.0 support, streamed content messageId values, and terminal auth login invocation support.
  2. This follow-up: make ACP session/load replay restored history, including messageId fields on replayed user, assistant, and thought chunks, and expose restored session metadata.

I can rebase this PR after #2359 lands so it contains only the session-load replay change. Alternatively, if maintainers prefer to take both fixes together, this PR can be merged as the combined ACP fix and #2359 can be closed.

Changes

  • Return a LoadSessionResponse with initial modes and models from session/load.
  • Advertise transcript restore support as sessionCapabilities._meta.kimi.sessionHistoryReplay = true so clients do not have to infer replay behavior from loadSession alone.
  • Replay persisted wire.jsonl records as ACP session/update notifications on load.
  • Send messageId on restored user, assistant, and thought chunks using the message-id run tracking from fix(acp): assign message ids to streamed content #2359.
  • Emit standard ACP SessionInfoUpdate title/updatedAt updates after prompt completion, and during session/load/session/resume.
  • Return title/updatedAt metadata on session/load and session/resume under _meta.kimi.session.
  • Record ACP/direct KimiCLI.run() turns to the session wire log so future loads have a replay source.
  • Fall back to context text history for older sessions that do not have a wire log.
  • Cover wire replay, context fallback, restored messageId, advertised replay capability, and session title metadata with tests.

Validation

  • uv run pytest tests/acp tests/core/test_notifications.py -q
  • uv run ruff check src/kimi_cli/acp/session.py src/kimi_cli/acp/server.py src/kimi_cli/app.py tests/acp/test_protocol_v1.py tests/acp/test_session_notifications.py tests/acp/test_server_initialize.py tests/ui_and_conv/test_acp_server_auth.py
  • uv run pyright src/kimi_cli/acp/session.py src/kimi_cli/acp/server.py src/kimi_cli/app.py tests/acp/test_protocol_v1.py tests/acp/test_session_notifications.py tests/acp/test_server_initialize.py tests/ui_and_conv/test_acp_server_auth.py
  • git diff --check

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 5 additional findings in Devin Review.

Open in Devin Review

Comment on lines +356 to +358
case Notification():
await self._send_notification(msg)
replayed_updates += 1
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.

🔴 AssertionError when replaying Notification after TurnEnd in wire history

In replay_history, the Notification case (line 356-358) calls _send_notification_send_text_content_run_id("message") which asserts self._turn_state is not None (src/kimi_cli/acp/session.py:438). However, after a TurnEnd is processed during replay (line 325-329), self._turn_state is set to None. Notifications can appear after TurnEnd in the wire file because run_soul() at src/kimi_cli/soul/__init__.py:239 performs a final _deliver_notifications_to_wire_once() flush after the soul task finishes (i.e., after TurnEnd has been emitted) but before the wire shuts down. This means any session with late-flushed background-task notifications will crash with AssertionError when loaded via session/load.

Trace through the crash path
  1. Wire file contains: ... TurnEnd ... Notification (notification flushed after turn)
  2. replay_history processes TurnEnd → sets self._turn_state = None (line 329)
  3. replay_history processes Notification (line 356) → calls _send_notification
  4. _send_notification_send_text_content_run_id("message")assert self._turn_state is not NoneAssertionError
Prompt for agents
In replay_history(), the Notification case at lines 356-358 does not guard against self._turn_state being None. When a Notification appears in the wire file after TurnEnd (which happens due to the final notification flush in run_soul), self._turn_state is None and _send_notification -> _send_text -> _content_run_id will hit an assertion error.

The fix should either:
1. Guard the Notification case with a check like `if self._turn_state is not None`, similar to how ToolCallPart and ToolResult are guarded (lines 349, 353). If turn_state is None, either skip the notification or create a temporary turn state via _begin_replay_turn() before sending.
2. Or make _send_text / _content_run_id handle the case where _turn_state is None gracefully (e.g., generate a one-off message_id).

Option 1 is simpler and consistent with the existing pattern for ToolCallPart/ToolResult. The same concern technically applies to ThinkPart and TextPart cases (lines 333-338), though those are far less likely to appear outside a turn.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5e25b1d753

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +356 to +357
case Notification():
await self._send_notification(msg)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Guard replayed notifications when no turn is active

replay_history() can call _send_notification() after a TurnEnd (or before any TurnBegin) because wire logs may contain Notification records emitted outside prompt turns (e.g., background-task notifications flushed by the runtime). In that state self._turn_state is None, so _send_notification() -> _send_text() -> _content_run_id() hits the assert and aborts session/load for affected sessions instead of replaying history.

Useful? React with 👍 / 👎.

@huntharo huntharo force-pushed the fix/acp-load-history-message-ids branch 2 times, most recently from f86a8d4 to bea352f Compare May 24, 2026 17:44
@huntharo huntharo force-pushed the fix/acp-load-history-message-ids branch from bea352f to 9030439 Compare May 24, 2026 17:54
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9030439624

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +358 to +360
case Notification():
await self._send_notification(msg)
replayed_updates += 1
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reset content run around replayed notifications

In replay_history(), notifications are replayed via _send_notification() without clearing the current content run first, unlike the live prompt() path which resets before and after notifications. If a Notification is persisted between assistant text chunks in the same turn, replay reuses the same message_id for notification and adjacent assistant chunks, so clients that group by message_id will merge system notification text into the assistant message and lose original message boundaries.

Useful? React with 👍 / 👎.

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