Websocket followup

[shivammittal274]

No description provided.

[greptile-apps]

Greptile Overview

Greptile Summary

Implemented cancellation support for websocket connections allowing clients to abort agent execution mid-stream. The implementation adds abort() and isExecuting() methods to agents, introduces a cancel message type in the protocol, and implements proper cleanup handling.

Key changes:

• Extended BaseAgent with abstract abort() and isExecuting() methods
• Implemented cancellation in both ClaudeSDKAgent and CodexSDKAgent using AbortController
• Added thread reuse logic in CodexSDKAgent with proper null checks to prevent race conditions
• Created cancelExecution() in SessionManager that correctly avoids marking session idle (preventing race conditions)
• Extended websocket protocol with cancel message type and cancelled event using discriminated unions
• Changed iterator cleanup to fire-and-forget pattern to avoid blocking

Issues found:

• Syntax error in CodexSDKAgent.ts:343 - accessing .id on potentially null object
• Inconsistent iterator cleanup patterns between fire-and-forget and await
• Commented code in server.ts that should be removed per custom rules

Confidence Score: 4/5

• Safe to merge after fixing the null pointer issue in CodexSDKAgent
• Score reflects one critical syntax error that will cause runtime crashes when currentThread is null, plus style issues with commented code and inconsistent cleanup patterns. The core cancellation logic is well-designed with proper race condition prevention.
• Pay close attention to packages/agent/src/agent/CodexSDKAgent.ts line 343 - fix the null pointer bug before merging

Important Files Changed

File Analysis

Filename	Score	Overview
packages/agent/src/agent/CodexSDKAgent.ts	4/5	Added thread reuse, abort methods, and proper cleanup - thread ID null check prevents race condition
packages/agent/src/session/SessionManager.ts	5/5	Added cancelExecution() with defensive checks - correctly avoids marking idle to prevent race conditions
packages/agent/src/websocket/server.ts	4/5	Added cancel message handler and commented out idle cleanup - functional but contains commented code that should be removed

Sequence Diagram

sequenceDiagram
    participant Client
    participant WebSocketServer
    participant SessionManager
    participant Agent
    participant AbortController

    Note over Client,Agent: Regular Message Flow
    Client->>WebSocketServer: message (type: 'message')
    WebSocketServer->>SessionManager: markProcessing(sessionId)
    SessionManager-->>WebSocketServer: true
    WebSocketServer->>SessionManager: getAgent(sessionId)
    SessionManager-->>WebSocketServer: agent
    WebSocketServer->>Agent: execute(message)
    Agent->>AbortController: new AbortController()
    loop Event Streaming
        Agent-->>WebSocketServer: yield FormattedEvent
        WebSocketServer-->>Client: send event
    end
    Agent->>Agent: completeExecution()
    WebSocketServer->>SessionManager: markIdle(sessionId)

    Note over Client,Agent: Cancel Flow (New)
    Client->>WebSocketServer: message (type: 'cancel')
    WebSocketServer->>SessionManager: cancelExecution(sessionId)
    SessionManager->>Agent: isExecuting()
    Agent-->>SessionManager: true
    SessionManager->>Agent: abort()
    Agent->>AbortController: abort()
    Note over Agent: Breaks event loop
    Agent->>Agent: Clear currentThread
    SessionManager-->>WebSocketServer: true
    WebSocketServer-->>Client: cancelled event
    Note over Agent: Original execute() completes
    WebSocketServer->>SessionManager: markIdle(sessionId)

Loading

_{6 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

syntax: The condition checks both !this.currentThread and !this.currentThread.id, but if this.currentThread is null, accessing .id will throw. Should use optional chaining:

Suggested change

	// CRITICAL: Check both existence AND thread ID (ID is null if cancelled before thread.started event)
	if (!this.currentThread || !this.currentThread.id) {
	if (!this.currentThread || !this.currentThread?.id) {

Prompt To Fix With AI

This is a comment left during a code review.
Path: packages/agent/src/agent/CodexSDKAgent.ts
Line: 343:344

Comment:
**syntax:** The condition checks both `!this.currentThread` and `!this.currentThread.id`, but if `this.currentThread` is null, accessing `.id` will throw. Should use optional chaining:

```suggestion
      if (!this.currentThread || !this.currentThread?.id) {
```

How can I resolve this? If you propose a fix, please make it concise.