feat(acp): add session persistence and history replay #5153
Conversation
- ACPSessionManager now persists sessions to Storage - loadSession properly loads existing sessions from disk - Restores user's model choice when loading a session - replaySessionHistory sends session/update notifications for conversation history - Added tests for session persistence
|
/review |
| modelID: string | ||
| } | ||
| modeId?: string | ||
| internalSessionId?: string |
There was a problem hiding this comment.
Suggestion: The internalSessionId field was added but appears unused in this PR. Consider removing it if not needed, or adding a comment explaining its intended future use.
| internalSessionId?: string | |
| modeId?: string |
| const model = existingSession.model ?? (await defaultModel(this.config, directory)) | ||
|
|
||
| this.setupEventSubscriptions(existingSession) | ||
|
|
There was a problem hiding this comment.
Potential Bug: Adding setupEventSubscriptions here means it will be called twice when creating new sessions, since newSession also calls setupEventSubscriptions(state) after calling loadSession. This could result in duplicate event handlers being registered.
You should either:
- Remove the
setupEventSubscriptionscall fromnewSession(sinceloadSessionnow handles it), OR - Only call
setupEventSubscriptionshere when this is NOT a newly created session (though this may be tricky to detect)
|
|
||
| // Verify session is stored in Storage | ||
| const stored = await Storage.read<any>(["acp_session", session.id]).catch(() => null) | ||
| expect(stored).not.toBeNull() |
There was a problem hiding this comment.
Suggestion: Consider adding cleanup for consistency with other tests in this file that do clean up after themselves.
| expect(stored).not.toBeNull() | |
| expect(stored?.id).toBe(session.id) | |
| expect(stored?.cwd).toBe("/test/cwd") | |
| // Clean up | |
| await Storage.remove(["acp_session", session.id]) | |
| }, |
| const sdk = createMockSDK() | ||
| const manager = new ACPSessionManager(sdk) | ||
|
|
||
| const session = await manager.create("/test/cwd", []) |
There was a problem hiding this comment.
Potential Race Condition: setModel persists asynchronously with .catch() (fire-and-forget), so this Storage.read immediately after might read stale data if the persist hasnt completed yet. Consider either:
- Making
setModelreturn the persist promise, or - Adding a small delay or polling in the test
Summary
This PR adds proper session persistence and history replay to the ACP implementation.
Changes
session/updatenotifications (as per ACP spec)Testing
Added tests for session persistence:
All 258 tests pass.