Skip to content

fix(acp): 对齐 ACP session ID 与全局会话状态#1117

Merged
claude-code-best merged 1 commit into
claude-code-best:mainfrom
xuzhongpeng:fix/acp-session-id-alignment
May 12, 2026
Merged

fix(acp): 对齐 ACP session ID 与全局会话状态#1117
claude-code-best merged 1 commit into
claude-code-best:mainfrom
xuzhongpeng:fix/acp-session-id-alignment

Conversation

@xuzhongpeng
Copy link
Copy Markdown
Contributor

@xuzhongpeng xuzhongpeng commented May 12, 2026

问题描述:
使用--acp启动后,使用newSession协议返回的sessionId和Claude Code本身的sessionId不一致,导致后续对话通过该返回的sessionId对话都是全新会话,无法继续之前会话。

解决方案:
在 newSession/resumeSession/loadSession 中调用 switchSession, 确保 transcript 持久化、analytics 与 cost tracking 使用 ACP session ID, 而非内部默认 session ID。

  • newSession 生成 sessionId 后立即对齐全局状态,确保Claude Code sessionId与ACP返回的sesssionId一致
  • resumeSession 命中 fingerprint 缓存路径也对齐
  • loadSession 在 sessionIdExists() 检查前对齐(lookup 依赖 getSessionId)
  • 补充 5 个测试覆盖上述路径,以及 prompt 不触发额外 switchSession

View in Codesmith
Need help on this PR? Tag @codesmith with what you need.

  • Let Codesmith autofix CI failures and bot reviews

Summary by CodeRabbit

  • Tests

    • Added test suite validating session ID alignment with application state across session creation, resuming, and loading operations.
  • Bug Fixes

    • Enhanced session state synchronization to ensure the global state properly reflects the active session across all operations, preventing state inconsistencies.

Review Change Stack

在 newSession/resumeSession/loadSession 中调用 switchSession,
确保 transcript 持久化、analytics 与 cost tracking 使用 ACP session ID,
而非内部默认 session ID。

- newSession 生成 sessionId 后立即对齐全局状态
- resumeSession 命中 fingerprint 缓存路径也对齐
- loadSession 在 sessionIdExists() 检查前对齐(lookup 依赖 getSessionId)
- 补充 5 个测试覆盖上述路径,以及 prompt 不触发额外 switchSession
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 12, 2026

📝 Walkthrough

Walkthrough

AcpAgent now imports switchSession() from bootstrap state and calls it at three critical points in the session lifecycle: during session creation, when reusing an existing session with matching fingerprint, and before session existence checks during resumption. Tests verify switchSession is invoked with the correct session IDs at these points and confirm it does not trigger during multi-session prompt operations.

Changes

Session State Alignment

Layer / File(s) Summary
Session state synchronization implementation
src/services/acp/agent.ts
Imports switchSession() and SessionId type, then invokes switchSession() at three session lifecycle points: before setting up a new session in createSession, before returning an existing session when fingerprints match, and before session history lookups in getOrCreateSession to ensure global context aligns with ACP session operations.
Test coverage for session state alignment
src/services/acp/__tests__/agent.test.ts
Mocks switchSession, clears call history in beforeEach, and introduces a test suite verifying switchSession is called with generated/requested session IDs on newSession, unstable_resumeSession, and loadSession, including reuse of existing sessions, and confirming prompt does not trigger additional calls in multi-session scenarios.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Poem

🐰 A session now dances with global state so grand,
Aligned by a switch at each lifecycle command—
Create, reuse, resume—the trio's in sync,
Tests lock down the calls with precision's own wink!
✨ State harmony achieved! 🎪

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title describes the main change: aligning ACP session ID with global session state. The title is concise, specific, and directly relates to the primary objective of the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
src/services/acp/__tests__/agent.test.ts (1)

1184-1194: ⚡ Quick win

Assert call order for loadSession alignment path, not just argument.

This test checks switchSession(requestedId) but does not prove it runs before sessionIdExists(), which is the regression-sensitive part of this fix.

✅ Suggested test hardening
     test('loadSession calls switchSession with the requested sessionId', async () => {
       const agent = new AcpAgent(makeConn())
       const requestedId = 'load-test-session-id'
+      const callOrder: string[] = []
+      mockSwitchSession.mockImplementationOnce(() => {
+        callOrder.push('switch')
+      })
+      mockSessionIdExists.mockImplementationOnce(() => {
+        callOrder.push('exists')
+        return false
+      })
       await agent.loadSession({
         sessionId: requestedId,
         cwd: '/tmp',
         mcpServers: [],
       } as any)

       expect(mockSwitchSession).toHaveBeenCalledWith(requestedId)
+      expect(callOrder.slice(0, 2)).toEqual(['switch', 'exists'])
     })
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/services/acp/__tests__/agent.test.ts` around lines 1184 - 1194, The test
asserts switchSession received the right argument but not that it ran before
sessionIdExists; update the test for loadSession to assert call order by
comparing the jest mock invocation order of mockSwitchSession and
mockSessionIdExists (use mock.fn().mock.invocationCallOrder indexes) so the
assertion ensures mockSwitchSession was invoked earlier than
mockSessionIdExists, while keeping the existing argument check for requestedId
on switchSession.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/services/acp/__tests__/agent.test.ts`:
- Around line 1184-1194: The test asserts switchSession received the right
argument but not that it ran before sessionIdExists; update the test for
loadSession to assert call order by comparing the jest mock invocation order of
mockSwitchSession and mockSessionIdExists (use
mock.fn().mock.invocationCallOrder indexes) so the assertion ensures
mockSwitchSession was invoked earlier than mockSessionIdExists, while keeping
the existing argument check for requestedId on switchSession.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5edac5e2-a79e-4426-b1fc-4fab90bf88d6

📥 Commits

Reviewing files that changed from the base of the PR and between ea51474 and 2c8a22d.

📒 Files selected for processing (2)
  • src/services/acp/__tests__/agent.test.ts
  • src/services/acp/agent.ts

@claude-code-best claude-code-best merged commit 3d7b32f into claude-code-best:main May 12, 2026
3 checks passed
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.

2 participants