Fix local Codex queued messages getting stuck#2185
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0093fedd79
ℹ️ 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".
| }); | ||
| if (isTurnCompleteEvent(acpMsg)) { | ||
| const session = sessionStoreSetters.getSessions()[taskRunId]; | ||
| if (session?.isCloud) { |
There was a problem hiding this comment.
When a user cancels a local Codex turn that has queued follow-up messages, Codex emits _posthog/turn_complete before the matching JSON-RPC prompt response, but this new session?.isCloud guard leaves the old currentPromptId in place for local sessions. Since cancelPrompt() only clears isPromptPending, the later cancelled response still matches the stale id in handleSessionEvent and drains the queued messages, even though the turn was cancelled; previously the turn-complete event cleared the id so that response was ignored as stale.
Useful? React with 👍 / 👎.
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/code/src/renderer/features/sessions/service/service.test.ts:2304-2379
**Test accesses private method via type cast — first such pattern in this file**
The test casts `service as unknown as { handleSessionEvent: ... }` to call a `private` method directly. This is the first occurrence of this pattern in the test file, and it makes the test brittle to any rename or visibility change on `handleSessionEvent`. The existing approach for similar event-ordering tests is to capture the `onData` callback from `mockTrpcAgent.onSessionEvent.subscribe` and call it directly in sequence, which exercises the actual public subscription path. The test is also placed in `describe("sendPrompt")` despite not calling `sendPrompt` at all; it fits better alongside the cloud queue-flush test under `watchCloudTask` or in a dedicated queue-management block.
Reviews (1): Last reviewed commit: "Fix local Codex queued messages getting ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it("drains local queued messages when Codex turn_complete arrives before prompt response", async () => { | ||
| const service = getSessionService() as unknown as { | ||
| handleSessionEvent: ( | ||
| taskRunId: string, | ||
| acpMsg: { | ||
| type: "acp_message"; | ||
| ts: number; | ||
| message: Record<string, unknown>; | ||
| }, | ||
| ) => void; | ||
| }; | ||
| const queuedSession = createMockSession({ | ||
| taskRunId: "run-123", | ||
| taskId: "task-123", | ||
| status: "connected", | ||
| isPromptPending: true, | ||
| currentPromptId: 42, | ||
| messageQueue: [ | ||
| { id: "q-1", content: "investigate another", queuedAt: 1700000001 }, | ||
| ], | ||
| }); | ||
| const sessions: Record<string, AgentSession> = { | ||
| "run-123": queuedSession, | ||
| }; | ||
| mockSessionStoreSetters.getSessions.mockImplementation(() => sessions); | ||
| mockSessionStoreSetters.getSessionByTaskId.mockImplementation( | ||
| (taskId: string) => | ||
| taskId === "task-123" ? sessions["run-123"] : undefined, | ||
| ); | ||
| mockSessionStoreSetters.updateSession.mockImplementation( | ||
| (taskRunId: string, updates: Partial<AgentSession>) => { | ||
| const existing = sessions[taskRunId]; | ||
| if (!existing) return; | ||
| sessions[taskRunId] = { ...existing, ...updates }; | ||
| }, | ||
| ); | ||
| mockSessionStoreSetters.dequeueMessagesAsText.mockReturnValue( | ||
| "investigate another", | ||
| ); | ||
| mockTrpcAgent.prompt.mutate.mockResolvedValue({ stopReason: "end_turn" }); | ||
|
|
||
| service.handleSessionEvent("run-123", { | ||
| type: "acp_message", | ||
| ts: 1700000002, | ||
| message: { | ||
| jsonrpc: "2.0", | ||
| method: "_posthog/turn_complete", | ||
| params: { sessionId: "acp-session", stopReason: "end_turn" }, | ||
| }, | ||
| }); | ||
| expect(sessions["run-123"].currentPromptId).toBe(42); | ||
|
|
||
| service.handleSessionEvent("run-123", { | ||
| type: "acp_message", | ||
| ts: 1700000003, | ||
| message: { | ||
| jsonrpc: "2.0", | ||
| id: 42, | ||
| result: { stopReason: "end_turn" }, | ||
| }, | ||
| }); | ||
|
|
||
| await vi.waitFor(() => { | ||
| expect( | ||
| mockSessionStoreSetters.dequeueMessagesAsText, | ||
| ).toHaveBeenCalledWith("task-123"); | ||
| expect(mockTrpcAgent.prompt.mutate).toHaveBeenCalledWith({ | ||
| sessionId: "run-123", | ||
| prompt: [{ type: "text", text: "investigate another" }], | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| it("queues message when compaction is in progress", async () => { | ||
| const service = getSessionService(); | ||
| mockSessionStoreSetters.getSessionByTaskId.mockReturnValue( |
There was a problem hiding this comment.
The test casts service as unknown as { handleSessionEvent: ... } to call a private method directly. This is the first occurrence of this pattern in the test file, and it makes the test brittle to any rename or visibility change on handleSessionEvent. The existing approach for similar event-ordering tests is to capture the onData callback from mockTrpcAgent.onSessionEvent.subscribe and call it directly in sequence, which exercises the actual public subscription path. The test is also placed in describe("sendPrompt") despite not calling sendPrompt at all; it fits better alongside the cloud queue-flush test under watchCloudTask or in a dedicated queue-management block.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/code/src/renderer/features/sessions/service/service.test.ts
Line: 2304-2379
Comment:
**Test accesses private method via type cast — first such pattern in this file**
The test casts `service as unknown as { handleSessionEvent: ... }` to call a `private` method directly. This is the first occurrence of this pattern in the test file, and it makes the test brittle to any rename or visibility change on `handleSessionEvent`. The existing approach for similar event-ordering tests is to capture the `onData` callback from `mockTrpcAgent.onSessionEvent.subscribe` and call it directly in sequence, which exercises the actual public subscription path. The test is also placed in `describe("sendPrompt")` despite not calling `sendPrompt` at all; it fits better alongside the cloud queue-flush test under `watchCloudTask` or in a dedicated queue-management block.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
fredluz
force-pushed
the
fix/codex-queued-messages-stuck
branch
from
0093fed to
a05668a
Compare
|
Thanks — updated. Cloud sessions still use I also added I manually tested the local Codex flow before and after the fix. I can’t manually run cloud tasks from my current local setup, but the existing session-service cloud queue/hydration coverage still passes. |
|
duplicate PR #2197 |
2 participants
Problem
Local Codex queued follow-up messages can remain stuck in the conversation after the active turn completes.
Closes #2182
Changes
_posthog/turn_completecan arrive through the streamed ACP subscription before the matching JSON-RPCsession/promptresponse for local Codex sessions. The renderer was clearingcurrentPromptIdfor all sessions when that notification arrived. When the later prompt response arrived, it was treated as stale and returned before the local queue-drain path could run.This changes
turn_completeprompt-state clearing to apply only to cloud sessions. Local sessions keep using the matching prompt response as the queue-drain boundary.Added a regression test for the local Codex ordering case: queued message present,
turn_completearrives first, then the matching prompt response drains the queue.How did you test this?
Manual:
Automated:
Also run by pre-commit hook:
Publish to changelog?
no