Skip to content

feat(agent,websocket,status,channels): WebSocket tooling, session lifecycle, reasoning, chat_id#3179

Closed
JackLuguibin wants to merge 1 commit intoHKUDS:nightlyfrom
JackLuguibin:feat/enhance_ws_tool_event_ex
Closed

feat(agent,websocket,status,channels): WebSocket tooling, session lifecycle, reasoning, chat_id#3179
JackLuguibin wants to merge 1 commit intoHKUDS:nightlyfrom
JackLuguibin:feat/enhance_ws_tool_event_ex

Conversation

@JackLuguibin
Copy link
Copy Markdown
Contributor

Enhance tool event handling and configuration
Session turn lifecycle notifications
Reasoning content handling for WebSocket channels; streamlined attachment across channels
Status and context commands JSON support
chat_id for WebSocket session resumption
OutboundMessage data field for additional payloads; docs and tests updated

@JackLuguibin
Copy link
Copy Markdown
Contributor Author

@Re-bin

Copy link
Copy Markdown
Collaborator

@Re-bin Re-bin left a comment

Choose a reason for hiding this comment

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

I like the direction of the WebSocket work here, but I don’t think this is ready to merge as-is.

The good part is clear:

  • resumable chat_id sessions
  • explicit chat_start / chat_end
  • structured tool_event payloads
  • a more frontend-friendly WebSocket protocol

That all makes sense.

Where this PR loses its boundary is that it also generalizes structured JSON command output and reasoning delivery across channels, but without a complete delivery contract outside WebSocket.

Two concrete problems stand out:

  1. /status_json and /context_json now put their payload into OutboundMessage.data while leaving content empty. WebSocket handles data, but CLI and most other channels still render from content, so these commands effectively become empty responses outside WebSocket.

  2. reasoning_content is now attached as a streamed follow-up for all channels when send_reasoning_content is enabled, but only WebSocket actually understands the _reasoning_only path. That means behavior is inconsistent across channels: some drop it, some send blank messages, and only WebSocket renders it correctly.

So the WebSocket-specific changes are promising, but the PR overreaches when it turns those transport-specific ideas into cross-channel behavior without fully implementing the other side.

I pulled the branch locally, reviewed the origin/nightly...HEAD diff, and reran:

  • focused websocket / command tests
  • the full test suite

Everything passes locally, but the functional boundary still isn’t clean enough.

My recommendation:

  • either scope this PR down to WebSocket-only behavior
  • or fully implement and document the cross-channel contract for data and reasoning delivery before merging

From my side, this is not merge-ready yet.

@JackLuguibin JackLuguibin force-pushed the feat/enhance_ws_tool_event_ex branch from 4e2cd8a to 91a67de Compare April 16, 2026 10:45
…ecycle, reasoning, chat_id

- Enhance tool event handling and configuration; session turn lifecycle notifications
- Reasoning content handling for WebSocket channels; streamlined attachment across channels
- Status and context commands JSON support; OutboundMessage data field for payloads
- chat_id for WebSocket session resumption; docs and tests updated
- fix(discord): register status_json, context, context_json slash commands
- refactor(builtin): streamline context command response handling
- chore(builtin): remove unused json import
@JackLuguibin JackLuguibin force-pushed the feat/enhance_ws_tool_event_ex branch from 91a67de to 8602904 Compare April 16, 2026 10:48
@Re-bin
Copy link
Copy Markdown
Collaborator

Re-bin commented Apr 16, 2026

Closed due to #3216.

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