feat(reactions): notify from reactions + repair the bot-delivery path#1064
Merged
Conversation
buremba
force-pushed
the
feat/reaction-notifications
branch
from
c140740 to
a3d701c
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a notifications namespace to ReactionClient, exposes it in the sandbox ClientSDK, documents the method, updates example reactions to send watcher-attributed notifications, and implements DB-driven bot delivery via the chat instance manager with tests. ChangesNotifications SDK Feature
Sequence DiagramsequenceDiagram
participant ReactionScript as Reaction Script
participant ClientSDK as client.notifications
participant ActionCaller as createActionCaller
participant NotifyTool as notify
participant NotificationsService as notifications.service
participant ChatManager as ChatInstanceManager
participant Channel as Channel
ReactionScript->>ClientSDK: send({ title, body, recipients, watcher_source })
ClientSDK->>ActionCaller: dispatch "send" action
ActionCaller->>NotifyTool: invoke notify.send with input
NotifyTool->>NotificationsService: resolveBotDeliveryTargets(orgId, connectionId?)
NotificationsService->>ChatManager: postNotificationToChannel(connectionId, channelKey, text)
ChatManager->>Channel: post({ raw: text, files: [] })
Channel-->>ChatManager: post result
ChatManager-->>NotificationsService: delivery result
NotificationsService-->>NotifyTool: aggregate results
NotifyTool-->>ActionCaller: { notified_count }
ActionCaller-->>ClientSDK: return result
ClientSDK-->>ReactionScript: { notified_count }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Member
Author
|
bug_free 48, simplicity 78, slop 8, bugs 1, 1 blockers Script-run typecheck/unit/integration all passed. Exploratory: copied connector-sdk d.ts to a temp package without chat and ran tsc; it failed TS2307 on the new chat type import. Did not boot server. Blockers
Suggested fixes
Full verdict JSON{
"bug_free_confidence": 48,
"bugs": 1,
"slop": 8,
"simplicity": 78,
"blockers": [
"published @lobu/connector-sdk declarations now import optional peer `chat`; an external TS compile without `chat` installed fails with TS2307"
],
"change_type": "feat",
"behavior_change_risk": "medium",
"tests_adequate": false,
"suggested_fixes": [
{
"file": "packages/connector-sdk/package.json",
"line": 57,
"change": "Make `chat` a normal dependency (and update bun.lock), or remove the public `import type { CardElement } from \"chat\"` so SDK consumers do not need an optional peer for typechecking."
},
{
"file": "packages/server/src/gateway/__tests__/chat-instance-manager-slack.test.ts",
"line": 212,
"change": "Remove unused `Section` from the dynamic import and delete `void Section;` on line 226."
}
],
"notes": "Script-run typecheck/unit/integration all passed. Exploratory: copied connector-sdk d.ts to a temp package without chat and ran tsc; it failed TS2307 on the new chat type import. Did not boot server.",
"categories": {
"src": 373,
"tests": 242,
"docs": 0,
"config": 0,
"deps": 28,
"migrations": 0,
"ci": 0,
"generated": 0
}
}Local review gate — branch protection can require the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/sandbox/method-metadata.ts`:
- Around line 208-223: The usageExample for the "notifications.send" method
contains an escaped newline literal `\\n` inside the template literal, so change
that to a single `\n` so the body string contains a real newline; update the
usageExample string in the notifications.send metadata (the usageExample
property for "notifications.send") to use `\n` instead of `\\n` in the
notification body passed to client.notifications.send.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b139b0c5-21d8-42c5-b67c-14dbdafd2f61
📒 Files selected for processing (8)
examples/lobu-crm/funnel-digest.reaction.tsexamples/lobu-crm/inbound-triage.reaction.tspackages/connector-sdk/src/reaction-client-types.tspackages/server/src/__tests__/integration/sandbox/run-script-runtime.test.tspackages/server/src/sandbox/client-sdk.tspackages/server/src/sandbox/method-metadata.tspackages/server/src/sandbox/namespaces/index.tspackages/server/src/sandbox/namespaces/notifications.ts
Comment on lines
+208
to
+223
| // notifications | ||
| "notifications.send": { | ||
| summary: | ||
| "Send a notification to org users. Writes an `agent_message` notification (in-app inbox) and fans it out to the org's active bot connections (Slack/Telegram) — the way a watcher reaction surfaces its digest to a chat channel. Pass `watcher_source` when firing from a reaction.", | ||
| access: "write", | ||
| example: | ||
| "await client.notifications.send({ title: 'Weekly funnel digest', body: '3 new leads...', watcher_source: { watcher_id: ctx.window.watcher_id, window_id: ctx.window.id } });", | ||
| usageExample: `// Push a watcher digest to the org's Slack/Telegram connections + inbox. | ||
| export default async (ctx, client) => { | ||
| await client.notifications.send({ | ||
| title: 'Weekly funnel digest', | ||
| body: 'Top action: send the Acme pilot offer\\nNew leads: 3', | ||
| watcher_source: { watcher_id: ctx.window.watcher_id, window_id: ctx.window.id }, | ||
| }); | ||
| };`, | ||
| }, |
There was a problem hiding this comment.
Fix escaped newline in the usageExample.
Line 219 uses \\n inside a template literal, which will render as a literal \n string rather than a newline character. Since the usageExample is wrapped in backticks (template literal), use a single backslash \n to produce an actual newline in the notification body.
📝 Proposed fix
body: 'Top action: send the Acme pilot offer\\nNew leads: 3',
+ body: 'Top action: send the Acme pilot offer\nNew leads: 3',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/sandbox/method-metadata.ts` around lines 208 - 223, The
usageExample for the "notifications.send" method contains an escaped newline
literal `\\n` inside the template literal, so change that to a single `\n` so
the body string contains a real newline; update the usageExample string in the
notifications.send metadata (the usageExample property for "notifications.send")
to use `\n` instead of `\\n` in the notification body passed to
client.notifications.send.
Watcher reactions could persist knowledge but had no way to push a notification — so digests landed in the feed and never reached Slack. The legacy ReactionSDK had `notify`; PR #348 consolidated reactions onto the typed ClientSDK and carried `notify` over only as an agent-facing MCP tool, dropping it from the reaction surface. Restore it as a `notifications` namespace that reuses the existing `notify` tool handler -> createNotificationForUsers -> deliverToBotConnections, so a reaction can fan a digest out to the org's active bot connections (Slack/ Telegram) and the in-app inbox. `watcher_source` attributes the send to the firing window. - sandbox: new `notifications.send` namespace + method-metadata (write) + ClientSDK wiring; exposed on connector-sdk ReactionClient type. - examples/lobu-crm: funnel-digest + inbound-triage reactions now call client.notifications.send after persisting knowledge. - test: reaction script dispatching client.notifications.send through the isolate (run-script-runtime).
deliverToBotConnections fetched GET /api/internal/connections (removed in #846) and POST /api/v1/messaging/send (also gone), so it hit `if (!connRes.ok) return` and silently delivered nothing — notifications never reached Slack/Telegram for any notification type. Rewrite it to resolve active chat connections + their channel bindings from Postgres and post in-process via a new ChatInstanceManager.postNotificationToChannel (a one-shot outbound post, not an inbound message that triggers an agent run). Every app pod loads every active connection at boot, so the locally-held instance can post regardless of which pod fired the notification — correct under N>1 replicas, no cross-pod routing. Best-effort per connection. Together with the reaction `notifications.send` namespace, a watcher reaction digest now reaches the agent's bound Slack channel. - chat-instance-manager: new postNotificationToChannel + 2 unit tests. - notifications/service: DB+manager delivery; drop dead HTTP path + unused getLobuServiceToken import.
Extract resolveBotDeliveryTargets (connections JOIN channel bindings) and cover it against a real DB: bound channel resolved, no-binding/inactive omitted, bare channel id prefixed, connectionId filter honored. Addresses the pi review's tests_adequate flag for the bot fan-out path. Also trims the notifications namespace header comment.
buremba
force-pushed
the
feat/reaction-notifications
branch
from
2344039 to
50fb402
Compare
…-replica) pi review caught a real N>1 hole: postNotificationToChannel only read this pod's in-memory instance map. A connection created or restarted on another replica is found in Postgres but has no live instance locally, so the post was skipped. Call ensureConnectionRunning() first — idempotent when already running, won't revive a stopped connection — so any pod that fires the notification can deliver. Adds lazy-start + stopped-connection unit tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/server/src/__tests__/integration/notifications/bot-delivery.test.ts (1)
48-139: ⚡ Quick winAdd a two-workspace Slack isolation test case.
Please add one case with two active Slack connections for the same agent but different
metadata.teamId, and assert each binding resolves only to its matching connection. This will lock in the fan-out safety contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/server/src/__tests__/integration/notifications/bot-delivery.test.ts` around lines 48 - 139, Add a new test in the resolveBotDeliveryTargets suite that seeds two active Slack connections for the same agent with different metadata.teamId values (use seedSlackConnection with connectionId 'conn-a' and 'conn-b' and metadata.teamId 'TAAA' and 'TBBB'), then seed two bindings for the same agent each pointing to a channel for one team (channelId 'slack:C_TAAA' and 'slack:C_TBBB'); call resolveBotDeliveryTargets(org.id) and assert the result contains exactly two targets with matching connectionId -> channelKey pairs (e.g., 'conn-a' -> 'slack:C_TAAA' and 'conn-b' -> 'slack:C_TBBB'), ensuring each binding resolves only to its corresponding connection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/server/src/notifications/service.ts`:
- Around line 75-79: The JOIN between agent_channel_bindings (alias b) and agent
connections (alias ac) only matches on organization_id, agent_id and platform,
allowing a binding to pair with the wrong Slack connection; update the JOIN
condition in the SQL to also constrain the workspace/team identifier by adding
an equality check between b.team_id and ac.team_id (or the appropriate Slack
workspace field on ac) so bindings only match connections for the same Slack
team/workspace.
---
Nitpick comments:
In
`@packages/server/src/__tests__/integration/notifications/bot-delivery.test.ts`:
- Around line 48-139: Add a new test in the resolveBotDeliveryTargets suite that
seeds two active Slack connections for the same agent with different
metadata.teamId values (use seedSlackConnection with connectionId 'conn-a' and
'conn-b' and metadata.teamId 'TAAA' and 'TBBB'), then seed two bindings for the
same agent each pointing to a channel for one team (channelId 'slack:C_TAAA' and
'slack:C_TBBB'); call resolveBotDeliveryTargets(org.id) and assert the result
contains exactly two targets with matching connectionId -> channelKey pairs
(e.g., 'conn-a' -> 'slack:C_TAAA' and 'conn-b' -> 'slack:C_TBBB'), ensuring each
binding resolves only to its corresponding connection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 53b60b5b-3d1a-4a7b-baff-da95f24bfb83
📒 Files selected for processing (13)
examples/lobu-crm/funnel-digest.reaction.tsexamples/lobu-crm/inbound-triage.reaction.tspackages/connector-sdk/src/index.tspackages/connector-sdk/src/reaction-client-types.tspackages/server/src/__tests__/integration/notifications/bot-delivery.test.tspackages/server/src/__tests__/integration/sandbox/run-script-runtime.test.tspackages/server/src/gateway/__tests__/chat-instance-manager-slack.test.tspackages/server/src/gateway/connections/chat-instance-manager.tspackages/server/src/notifications/service.tspackages/server/src/sandbox/client-sdk.tspackages/server/src/sandbox/method-metadata.tspackages/server/src/sandbox/namespaces/index.tspackages/server/src/sandbox/namespaces/notifications.ts
Comment on lines
+75
to
+79
| JOIN agent_channel_bindings b | ||
| ON b.organization_id = ac.organization_id | ||
| AND b.agent_id = ac.agent_id | ||
| AND b.platform = ac.platform | ||
| WHERE ac.organization_id = ${organizationId} |
There was a problem hiding this comment.
Prevent cross-workspace misdelivery by constraining Slack joins with team_id.
Line 75-Line 79 joins bindings to connections only by org/agent/platform. With multiple active Slack connections for the same agent, one binding can match the wrong connection and post into the wrong workspace channel.
Suggested fix
JOIN agent_channel_bindings b
ON b.organization_id = ac.organization_id
AND b.agent_id = ac.agent_id
AND b.platform = ac.platform
+ AND (
+ ac.platform <> 'slack'
+ OR b.team_id IS NULL
+ OR b.team_id = ac.metadata->>'teamId'
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| JOIN agent_channel_bindings b | |
| ON b.organization_id = ac.organization_id | |
| AND b.agent_id = ac.agent_id | |
| AND b.platform = ac.platform | |
| WHERE ac.organization_id = ${organizationId} | |
| JOIN agent_channel_bindings b | |
| ON b.organization_id = ac.organization_id | |
| AND b.agent_id = ac.agent_id | |
| AND b.platform = ac.platform | |
| AND ( | |
| ac.platform <> 'slack' | |
| OR b.team_id IS NULL | |
| OR b.team_id = ac.metadata->>'teamId' | |
| ) | |
| WHERE ac.organization_id = ${organizationId} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/server/src/notifications/service.ts` around lines 75 - 79, The JOIN
between agent_channel_bindings (alias b) and agent connections (alias ac) only
matches on organization_id, agent_id and platform, allowing a binding to pair
with the wrong Slack connection; update the JOIN condition in the SQL to also
constrain the workspace/team identifier by adding an equality check between
b.team_id and ac.team_id (or the appropriate Slack workspace field on ac) so
bindings only match connections for the same Slack team/workspace.
Rename postNotificationToChannel -> postMessageToChannel (it's a generic
outbound-post primitive; 'notification' is the caller's concern) and post
{ markdown } instead of { raw } so digests render in each platform's native
format (Slack markdown_text) rather than being HTML-escaped.
Let notifications carry a rich card, not just markdown: notifications.send /
the notify tool accept an optional `card` typed as the `chat` CardElement
(reused, not a bespoke DSL), and postMessageToChannel takes chat's
AdapterPostableMessage and posts it directly — no converter. The Chat SDK
renders one card to each platform's native format (Slack Block Kit, Teams
Adaptive Cards, Google Chat Cards), so it's portable everywhere.
connector-sdk re-exports the CardElement type (chat as optional peer/dev dep,
type-only — no runtime bloat). Adds a manager test that builds a card with the
real chat primitives and asserts it posts as { card }.
Importing chat's CardElement into connector-sdk's public .d.ts made an external typecheck fail (TS2307) unless the consumer also installed chat — and making chat a hard dep would bloat every connector install. Keep the real chat types server-side (where the card is validated + rendered) and type the author-facing `card` as a plain serializable object. Drop the chat dep from connector-sdk; remove the unused Section import in the card test.
Member
Author
|
Review verdict (pi via |
This was referenced May 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes watcher reactions able to push a digest to Slack/Telegram — two layered gaps, both fixed here.
Gap 1 — reactions can't notify
Reaction scripts run in an
isolated-vmsandbox whose only capability is the typedClientSDK(knowledge,entities,query,log) — no notify. The legacyReactionSDKhadnotify; PR #348 ("swap reactions to ClientSDK") carried it over only as an agent-facing MCP tool. So a reaction can persist a digest but not deliver it.Fix: restore it as a
notificationsnamespace on the ClientSDK, reusing the existingnotifytool handler →createNotificationForUsers(normal Lobu inbox notification + bot fan-out).watcher_sourceattributes the send to the firing window. Exposed on the publicReactionClienttype.Gap 2 — the bot fan-out is dead
deliverToBotConnections(the path that turns a notification into a Slack/Telegram message) fetchedGET /api/internal/connections(removed in #846) andPOST /api/v1/messaging/send(also gone) — so it hitif (!connRes.ok) returnand silently no-op'd. Every notification type has been unable to reach a bot channel since #846.Fix: rewrite it to resolve active connections + their channel bindings from Postgres and post in-process via a new
ChatInstanceManager.postNotificationToChannel(a one-shot outbound post, not an inbound message that triggers an agent run). Every app pod loads every active connection at boot, so the locally-held instance can post regardless of which pod fired the notification — correct under N>1 replicas, no cross-pod routing.Changes
sandbox:notifications.sendnamespace + method-metadata (write) + ClientSDK wiring.connector-sdk:notifications+NotificationsSendInputon the publicReactionClienttype.chat-instance-manager:postNotificationToChannel(+ 2 unit tests).notifications/service: DB + in-process delivery; removed the dead HTTP path.examples/lobu-crm:funnel-digest+inbound-triagereactions callclient.notifications.send(...).Validation
client.notifications.send(...)dispatches through the isolate (run-script-runtime) — 6 isolate tests pass under Node 22.postNotificationToChannelunit tests (posts to resolved channel; throws with no instance).tsc --noEmitclean for server, connector-sdk, examples/lobu-crm.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Examples
Tests