Skip to content

Fix/edit tool calls#103

Open
i1bro wants to merge 15 commits intomainfrom
fix/edit-tool-calls
Open

Fix/edit tool calls#103
i1bro wants to merge 15 commits intomainfrom
fix/edit-tool-calls

Conversation

@i1bro
Copy link
Copy Markdown
Collaborator

@i1bro i1bro commented Apr 20, 2026

Summary

  • capture file-change context from app-server events for later approval and tool-call updates
  • use edited file paths as edit tool-call titles instead of the generic "Editing files"
  • attach file locations and diff data to edit tool calls, with a turn-diff fallback when needed
  • serialize session updates so the initial tool call is emitted before its completion update

Why

This makes edit tool calls understandable in the UI and avoids races where a completion update can arrive before the corresponding tool call exists.

Testing

  • npm test

Comment thread src/CodexAppServerClient.ts Outdated
Comment thread src/CodexAppServerClient.ts Outdated
Comment thread src/CodexAcpClient.ts Outdated
Comment thread src/CodexAppServerClient.ts Outdated
}
}

async flushServerNotifications(sessionId: string): Promise<void> {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why do we need a queue and then why we need to flush it instead of handling events?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because sendPrompt() must not return until all earlier events have been handled.

Comment thread src/CodexApprovalHandler.ts Outdated
Comment thread src/CodexAcpServer.ts Outdated
Comment thread src/CodexEventHandler.ts Outdated
Comment thread src/CodexAcpClient.ts Outdated
Comment thread src/CodexAcpClient.ts Outdated
Comment thread src/CodexToolCallMapper.ts Outdated
@AlexandrSuhinin AlexandrSuhinin self-requested a review April 28, 2026 13:09
Comment thread src/CodexAcpClient.ts Outdated

type SessionHandlerState = {
handler: SessionHandler;
pending: Promise<void>;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you clarify why do we need state management to handle notifications instead of calling "handler" when event arrives?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the per-session pending promise because notification handling is async: handleNotification() awaits createUpdateEvent() and session.update(), and file-change events may read files to reconstruct ACP diffs. The stored promise chain preserves event order and lets awaitSessionIdle() wait for all pending ACP updates before prompt completion.

}, FILE_HEADERS_ONLY);
}

function createDiffContentFromParsedPatch(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we manually create patch lines? Don't we have it already in change?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To convert the diff to ACP format and return as ACP "diff" content block. It needs oldText/newText

Comment thread src/CodexToolCallMapper.ts Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants