fix(conversations): resume opencode provider sessions#2201
fix(conversations): resume opencode provider sessions#2201janburzinski wants to merge 2 commits into
Conversation
Greptile SummaryThis PR fixes OpenCode session resumption by replacing the global
Confidence Score: 4/5The change is safe to merge; the happy path is well-tested and the fallback behaviour (fresh start when no session ID is stored) is deliberately documented. The core logic is sound: session IDs are captured via the new plugin hook, stored idempotently, and consumed correctly on resume. The two minor concerns are a lenient regex that could technically store a bare src/shared/conversation-config.ts (regex) and src/main/core/conversations/resolve-agent-session-command.ts (silent fallback) are the only spots worth a second look.
|
| Filename | Overview |
|---|---|
| src/main/core/agent-hooks/opencode-notifications-plugin.js | Adds session.created handler that extracts root session ID; parentID guard correctly skips sub-sessions. |
| src/main/core/conversations/save-provider-session-id.ts | Persists OpenCode session ID with idempotency check; validation relies on a lenient /^ses_/ regex. |
| src/main/core/conversations/resolve-agent-session-command.ts | Resolves session args for OpenCode: uses stored ses_* ID when available, silently falls back to fresh start otherwise. |
| src/shared/conversation-config.ts | New module for structured config parse/serialize; typed extraction replaces ad-hoc JSON.parse usage. |
| src/shared/agent-provider-registry.ts | OpenCode entry updated: resumeFlag changed to --session, sessionIdFlag and sessionIdOnResumeOnly added. |
| src/main/core/conversations/impl/local-conversation.ts | Delegates session/resume args to resolveAgentSessionCommandArgs before building the command. |
| src/main/core/conversations/impl/ssh-conversation.ts | Mirrors local-conversation: uses resolved session args; resume flag also updated to agentSession.isResuming. |
Sequence Diagram
sequenceDiagram
participant U as User
participant LC as LocalConversation
participant RA as resolveAgentSessionCommandArgs
participant OC as OpenCode CLI
participant HS as HookServer
participant HH as handleProviderSessionHook
participant DB as Database
U->>LC: Resume conversation
LC->>DB: getConversation (reads providerSessionId from config)
LC->>RA: "resolveAgentSessionCommandArgs(conversation, isResuming=true)"
alt providerSessionId stored
RA-->>LC: "{ sessionId: ses_abc123, isResuming: true }"
LC->>OC: opencode --session ses_abc123
else no providerSessionId
RA-->>LC: "{ sessionId: conv-1, isResuming: false }"
LC->>OC: opencode (fresh start)
end
OC->>HS: POST /hook session.created (no parentID)
HS->>HH: handleProviderSessionHook(raw)
HH->>DB: saveProviderSessionId(conversationId, ses_abc123)
DB-->>HH: updated config
HS-->>OC: 200 OK
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/shared/conversation-config.ts:1
The `isOpenCodeProviderSessionId` guard accepts any string that starts with `ses_`, including `ses_` itself (4 characters, non-empty). A bare `ses_` prefix with no body would pass the `length === 0` check in `handleProviderSessionHook`, pass this regex, be stored in the DB, and then be forwarded as `opencode --session ses_` — which would likely cause OpenCode to fail with an unknown or malformed session. Adding a minimum-length requirement (e.g. `ses_` + at least one character) closes this gap without breaking any real IDs.
```suggestion
const OPENCODE_SESSION_ID_PATTERN = /^ses_.+/;
```
### Issue 2 of 2
src/main/core/conversations/resolve-agent-session-command.ts:11-12
**Silent fresh-start fallback has no observability**
When an OpenCode conversation is resumed but `providerSessionId` is not yet stored (e.g. the `session.created` webhook was never received), the function quietly downgrades to a fresh session. A user who clicks "Resume" would see a brand-new chat with no indication that resumption was skipped. Adding a `log.warn` here (similar to the warning in `saveProviderSessionId`) would make this case visible in logs without changing the user-facing behaviour.
Reviews (1): Last reviewed commit: "fix(conversations): resume opencode prov..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -0,0 +1,32 @@ | |||
| const OPENCODE_SESSION_ID_PATTERN = /^ses_/; | |||
There was a problem hiding this comment.
isOpenCodeProviderSessionId guard accepts any string that starts with ses_, including ses_ itself (4 characters, non-empty). A bare ses_ prefix with no body would pass the length === 0 check in handleProviderSessionHook, pass this regex, be stored in the DB, and then be forwarded as opencode --session ses_ — which would likely cause OpenCode to fail with an unknown or malformed session. Adding a minimum-length requirement (e.g. ses_ + at least one character) closes this gap without breaking any real IDs.
| const OPENCODE_SESSION_ID_PATTERN = /^ses_/; | |
| const OPENCODE_SESSION_ID_PATTERN = /^ses_.+/; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/shared/conversation-config.ts
Line: 1
Comment:
The `isOpenCodeProviderSessionId` guard accepts any string that starts with `ses_`, including `ses_` itself (4 characters, non-empty). A bare `ses_` prefix with no body would pass the `length === 0` check in `handleProviderSessionHook`, pass this regex, be stored in the DB, and then be forwarded as `opencode --session ses_` — which would likely cause OpenCode to fail with an unknown or malformed session. Adding a minimum-length requirement (e.g. `ses_` + at least one character) closes this gap without breaking any real IDs.
```suggestion
const OPENCODE_SESSION_ID_PATTERN = /^ses_.+/;
```
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| return { sessionId: conversation.id, isResuming: false }; |
There was a problem hiding this comment.
When an OpenCode conversation is resumed but providerSessionId is not yet stored (e.g. the session.created webhook was never received), the function quietly downgrades to a fresh session. A user who clicks "Resume" would see a brand-new chat with no indication that resumption was skipped. Adding a log.warn here (similar to the warning in saveProviderSessionId) would make this case visible in logs without changing the user-facing behaviour.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/main/core/conversations/resolve-agent-session-command.ts
Line: 11-12
Comment:
**Silent fresh-start fallback has no observability**
When an OpenCode conversation is resumed but `providerSessionId` is not yet stored (e.g. the `session.created` webhook was never received), the function quietly downgrades to a fresh session. A user who clicks "Resume" would see a brand-new chat with no indication that resumption was skipped. Adding a `log.warn` here (similar to the warning in `saveProviderSessionId`) would make this case visible in logs without changing the user-facing behaviour.
How can I resolve this? If you propose a fix, please make it concise.1 participant
summary