fix(omni): intercept SendMessage(to: omni) in SDK executor → NATS reply path - #1091
Conversation
…ly path
Agents spawned via the omni bridge SDK executor (eugenia-seller, etc.)
call `SendMessage(recipient: "omni", message: "...")` to send replies,
mirroring the tmux mode contract. In SDK mode the call was a no-op:
the `done` MCP tool was the only NATS publish path, and the agent's
SendMessage calls fell through with no interception.
Fix: add a PreToolUse hook in `_processDelivery()` that fires only when
OMNI_INSTANCE is set in the executor env. The hook intercepts
`SendMessage` to recipient "omni", side-effect publishes the body to
`omni.reply.{instance}.{chatId}` (mirrors `handleDoneTool`'s text
action), and returns deny + reason "Message delivered to user via omni
bridge." The deny reason becomes the tool result the agent reads, so
the agent treats it as a successful send.
Defensive on field shape: accepts both `recipient`/`to` and
`message`/`content` (matches identity-inject's pattern).
Also updates `turn-based-prompt.ts` to teach SendMessage as the
canonical reply verb — the old prompt referenced `omni say` CLI verbs
that don't exist in SDK mode (no shell, in-process query).
Tests: 8 new cases covering publish path, alternate field shapes,
non-omni passthrough, non-SendMessage passthrough, bridge-unavailable
deny, OMNI_INSTANCE-absent passthrough, and wiring proof in both
directions. Full suite: 2251 pass / 0 fail.
Closes #1088
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to intercept SendMessage tool calls within the Claude SDK executor, specifically routing messages directed to the 'omni' recipient through a NATS bridge. It includes a new PreToolUse hook, createSendMessageOmniHook, which handles the interception and publication of these messages. Additionally, the turn-based system prompt has been updated to promote SendMessage as the primary reply method, replacing the legacy omni say command. Comprehensive unit tests have been added to verify the routing logic and hook integration. I have no feedback to provide.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e65e35a42
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| natsPublish( | ||
| `omni.reply.${instanceId}.${chatId}`, | ||
| buildReplyPayload(agent, chatId, instanceId, { content: body ?? '' }), |
There was a problem hiding this comment.
Avoid publishing empty omni replies on malformed SendMessage
When SendMessage targets recipient: "omni" but omits/invalidates the message field (for example, the model emits text instead of message/content), this hook still publishes content: "" and returns a success-like deny reason. In that scenario the agent believes delivery succeeded and won’t retry, so users can receive a blank message (or no usable reply) instead of an explicit tool error. Guarding on a non-empty parsed body before publishing would prevent this silent failure mode.
Useful? React with 👍 / 👎.
Address Codex P2 on #1091. Previously, if the model emitted SendMessage(recipient: 'omni') with the wrong field name (e.g. `text` instead of `message`/`content`) or an empty/whitespace string, the hook still published `content: ""` to the omni reply path AND returned a deny reason that read like success. The agent believed delivery succeeded and would not retry, leaving the user with a blank reply or no reply at all. The hook now rejects empty/whitespace-only bodies with an explicit error reason that prompts the model to retry with a real payload, BEFORE the NATS publish. - Two new tests cover the missing-field and whitespace-only cases. - All 34 claude-sdk tests pass; full gate green (2253/2253). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary
Fixes #1088. Agents spawned via the omni bridge SDK executor (eugenia-seller, etc.) call
SendMessage(recipient: "omni", message: "...")to send replies. In SDK mode this was a silent no-op — thedoneMCP tool was the only NATS publish path, and SendMessage calls fell through with no interception. Bridge logs showed sessions spawning and messages arriving, but replies never reachedomni.reply.{instance}.{chatId}and tests timed out.Approach
Added a
PreToolUsehook in_processDelivery()that fires only whenOMNI_INSTANCEis set in the executor env (non-bridge SDK sessions are unaffected). The hook:SendMessageto recipient"omni"omni.reply.{instance}.{chatId}— mirrorshandleDoneTool's text action exactlypermissionDecision: 'deny'with reason"Message delivered to user via omni bridge."— the deny reason becomes the tool result the agent reads, so the agent treats it as a successful sendThe provider's existing
mergeHooks()deep-merges this with the permission gate hook, so both run on every PreToolUse.Why not an MCP tool replacement? Namespacing (
mcp__genie-omni-tools__SendMessage) would break the agent's expectation thatSendMessageis the bare tool name — the same name it uses in tmux mode. A hook keeps the call site identical across transports.Field shape: defensive — accepts both
recipient/toandmessage/content(matchesidentity-inject.ts's pattern).Bonus fix
turn-based-prompt.tspreviously taught agents to useomni say/omni speakCLI verbs. These don't exist in SDK mode (no shell, in-process query) — so even though the issue title is about SendMessage interception, the prompt was steering agents toward a dead path. Updated the prompt to makeSendMessage(recipient: "omni", ...)the canonical reply verb, withomni donestill serving the turn-close protocol via the existing MCP tool.Test plan
bun run typecheckpassesbunx biome checkclean on all touched filesbun test src/services/executors/__tests__/claude-sdk.test.ts— 32 pass / 0 fail (24 existing + 8 new)bun testfull suite — 2251 pass / 0 failNew test coverage (
SendMessage omni interceptiondescribe block)recipient === 'omni'to/contentfield shape acceptednatsPublishis nullOMNI_INSTANCEis absent (non-bridge session)SendMessagematcher present inrunQueryoptions when env setFiles
src/services/executors/claude-sdk.ts(+88 -2) —parseSendMessageInput(),createSendMessageOmniHook(),_processDeliverywiringsrc/services/executors/turn-based-prompt.ts(+14 -10) — SendMessage as canonical reply verbsrc/services/executors/__tests__/claude-sdk.test.ts(+180 -3) — 8 new tests + 1 prompt assertion updateSibling work
#1089 (omni-bridge missing
omni.session.reset.*subscription) is the matching reset-path fix — different file, different abstraction, lands as a separate PR.🤖 Generated with Claude Code