feat(acp): add session persistence and history replay#5153
feat(acp): add session persistence and history replay#5153TylerBarnes wants to merge 1 commit intoanomalyco:devanomalyco/opencode:devfrom TylerBarnes:fix/acp-session-persistenceTylerBarnes/opencode:fix/acp-session-persistenceCopy head branch name to clipboard
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
00637c0 to
71e0ba2
Compare
f1ae801 to
08fa7f7
Compare
|
Closing this pull request because it has had no updates for more than 60 days. If you plan to continue working on it, feel free to reopen or open a new PR. |
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.