feat: implement channel system with various channel types and tools f…#340
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a Channels subsystem: channel metadata/decorators, runtime instances and registry, event bus, notification delivery (subscriptions, replay buffer, two‑way replies), scope/transport wiring, flows/tools, extensive docs, an E2E demo app with tests, and version bumps across packages. Changes
Sequence Diagram(s)sequenceDiagram
participant External as External Source
participant EventBus as ChannelEventBus
participant Registry as ChannelRegistry
participant Channel as ChannelInstance
participant Notifier as ChannelNotificationService
participant Session as MCP Session (Client)
External->>EventBus: emit(eventName, payload)
EventBus->>Registry: route to ChannelInstance.handleEvent(payload)
Registry->>Channel: create/obtain context -> onEvent()
Channel->>Channel: (optional) append to replay buffer
Channel->>Notifier: pushNotification(content, meta, targetSessionId?)
Notifier->>Session: send "notifications/claude/channel" (filtered by capability/subscription)
Session-->>Registry: call tool "channel-reply"(channel_name, text, meta)
Registry->>Channel: handleReply(text, meta)
Channel->>External: onReply() forwards reply to external system
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/sdk/src/common/metadata/app.metadata.ts (1)
19-34:⚠️ Potential issue | 🟡 MinorUpdate SDK documentation to include the new
channelsfield inLocalAppMetadata.The new
channels?: ChannelType[]field onLocalAppMetadatais a public SDK surface that requires documentation. Updatedocs/frontmcp/sdk-reference/decorators/app.mdxto addchannelsto the "Optional Properties" or "Components" table of the@Appdecorator configuration options. Also verify thatdocs/frontmcp/servers/apps.mdxincludes this field in its "App options" table.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/metadata/app.metadata.ts` around lines 19 - 34, Add the new public SDK field channels?: ChannelType[] to the `@App` decorator docs and server app options docs: update the SDK reference page that lists LocalAppMetadata/@App optional properties to include an entry "channels?: ChannelType[] — list of channel types the app supports" and ensure the servers/apps documentation's "App options" table also lists this property; reference the LocalAppMetadata type and ChannelType symbol in the description and mark it as optional in both tables so consumers see it in the public SDK surface.
🟠 Major comments (18)
libs/skills/catalog/frontmcp-channels/examples/channel-two-way/whatsapp-bridge.md-11-12 (1)
11-12:⚠️ Potential issue | 🟠 MajorWebhook signature validation is declared but not implemented in the example.
The page advertises signature validation, but the code path accepts and processes webhook payloads without validating provider signatures first. For a chat bridge, this weakens sender trust and allows spoofed inbound events.
As per coding guidelines (referenced
libs/skills/catalog/frontmcp-channels/SKILL.md): "for sources like chat bridges enforce sender authentication/allowlisting to prevent prompt injection."Also applies to: 20-95
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/skills/catalog/frontmcp-channels/examples/channel-two-way/whatsapp-bridge.md` around lines 11 - 12, The example advertises webhook signature validation but never performs it; add a validateWebhookSignature function and call it at the start of the webhook handler (e.g., in the handler that currently processes inbound events such as handleWebhookEvent or processInboundEvent) to verify provider signatures or HMACs and reject requests that fail validation before any parsing or processing; implement signature extraction/verification logic consistent with the provider (e.g., compare computed HMAC of the raw body to the X-Signature header or check a JWT) and return a 401/403 on failure, and annotate the example with the required environment/config values (signing secret/header name) so the new validation is clearly referenced in the flow.libs/sdk/src/channel/channel.types.ts-1-16 (1)
1-16: 🛠️ Refactor suggestion | 🟠 MajorMove
IndexedChanneltocommon/recordsto match SDK record-type placement.This index-row shape is a record-type artifact and should be centralized instead of living under a feature module path.
As per coding guidelines, "Centralize record types in common/records directory instead of scattering across module-specific files".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel.types.ts` around lines 1 - 16, The IndexedChannel interface is a record-type that should be moved to the centralized records area: create a new exported type file in the common/records module that declares export interface IndexedChannel { instance: ChannelInstance; owner: EntryOwnerRef; resolvedName: string } and import the referenced types ChannelInstance and EntryOwnerRef from their existing modules, then update all usages/imports of IndexedChannel throughout the codebase to import from the new common/records file and remove the original IndexedChannel declaration from the channel module (or replace it with a re-export if needed for compatibility).libs/sdk/src/front-mcp/front-mcp.ts-482-488 (1)
482-488:⚠️ Potential issue | 🟠 MajorAdd
scope.notifications.unregisterServer(sessionId)to stdio shutdown handlers to prevent resource leak.The stdio session is registered and auto-subscribed to channels (lines 483-488), but the SIGINT/SIGTERM handlers (lines 491-506) only close the server without cleanup. This leaves the session entry in the servers, subscriptions, and channelSubscriptions maps indefinitely. Compare with
in-memory-server.ts(line 173), which properly callsunregisterServer()before server close:Correct cleanup pattern
close: async () => { scope.notifications.unregisterServer(sessionId); await mcpServer.close(); }Apply the same pattern to both SIGINT and SIGTERM handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/front-mcp/front-mcp.ts` around lines 482 - 488, The stdio shutdown handlers currently only close mcpServer and leak session state; update both SIGINT and SIGTERM handlers to first call scope.notifications.unregisterServer(sessionId) to remove the session from servers/subscriptions/channelSubscriptions, then await mcpServer.close(); reference the existing symbols sessionId, mcpServer and scope.notifications.unregisterServer in the SIGINT and SIGTERM handler functions so the unregister runs before closing the server.libs/skills/catalog/frontmcp-channels/references/channel-sources.md-124-141 (1)
124-141:⚠️ Potential issue | 🟠 MajorThis
twoWayexample is incomplete withoutonReply().With
twoWay: true, readers will getchannel-replyregistered, but this sample never shows how replies are forwarded back to the service. As written, it demonstrates a feature that will just hit the default warning path.As per coding guidelines,
For two-way support, set twoWay: true so channel-reply is available, and implement onReply(reply, meta?).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/skills/catalog/frontmcp-channels/references/channel-sources.md` around lines 124 - 141, The example sets twoWay: true on WhatsAppChannel (extending ChannelContext) but omits the required onReply(reply, meta?) implementation that forwards inbound replies to the underlying service; add an async onReply(reply: string, meta?: unknown): Promise<void> method on WhatsAppChannel that transforms the reply into the service payload and calls the WhatsApp client/send API (use the same client used in onConnect, e.g., this.client) to deliver the message so the registered channel-reply handler actually forwards replies instead of hitting the default warning path.libs/sdk/src/channel/sources/agent-completion.source.ts-42-44 (1)
42-44:⚠️ Potential issue | 🟠 Major
agentIdsshould not match display names.Matching
event.agentNamehere broadens the documented contract and can notify the wrong channel when names collide or change. If name-based filtering is intentional, it should be a separate option.As per coding guidelines,
`agentIds` | `string[]` | Optional filter — only these agent IDs trigger the channel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/sources/agent-completion.source.ts` around lines 42 - 44, The filter currently checks both filterAgentIds and event.agentName which violates the documented contract that filterAgentIds only matches agent IDs; update the logic in the agent-completion source (where filterAgentIds is used) to only compare filterAgentIds.includes(event.agentId) and remove the includes(event.agentName) branch, and if name-based filtering is desired introduce a separate option (e.g., filterAgentNames) instead of reusing filterAgentIds.apps/e2e/demo-e2e-channels/e2e/channels.e2e.spec.ts-56-92 (1)
56-92:⚠️ Potential issue | 🟠 MajorThese cases only validate the helper tool response, not channel delivery.
emit-app-eventandsend-channel-notificationcan both return success even if nonotifications/claude/channelmessage ever reaches a capable session. The new session capture tools in this PR are a better fit here.As per coding guidelines,
Channel notifications are server-initiated and delivered only to MCP sessions that advertise experimental: { 'claude/channel': {} }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-channels/e2e/channels.e2e.spec.ts` around lines 56 - 92, The tests currently only assert the tool responses from server.callTool('emit-app-event') and server.callTool('send-channel-notification') but do not verify actual channel delivery; update the two specs ("App Event Source" / emit-app-event and "Manual Channel Push" / send-channel-notification) to use the new session-capture utilities provided in this PR to capture sessions that advertise experimental: { 'claude/channel': {} } and assert that a captured session actually received the expected channel message (e.g., contains the event payload or notification with channelName/status), rather than only parsing the tool response; keep the existing tool calls but add capture/await logic to validate delivery to ChannelEventBus/ChannelNotificationService recipients.apps/e2e/demo-e2e-channels/e2e/channels.e2e.spec.ts-43-50 (1)
43-50:⚠️ Potential issue | 🟠 MajorThis test never checks the advertised channel capability.
Finding
channel-replyinlistTools()only proves channel registration ran. A brokeninitializepayload would still make every Claude client missexperimental['claude/channel'], and this test would stay green.As per coding guidelines,
Capability handshake: server advertises experimental: { "claude/channel": {} }.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-channels/e2e/channels.e2e.spec.ts` around lines 43 - 50, The test currently only checks server.listTools() for 'channel-reply' which doesn't verify the initialize handshake; update the spec to call the server's initialize/get-initialize response (e.g. server.initialize() or server.getInitializeResponse()) and assert that the returned capabilities include experimental['claude/channel'] (non-null/object), while keeping the existing tools check for 'channel-reply' to ensure registration also ran.libs/sdk/src/channel/sources/app-event.source.ts-45-49 (1)
45-49:⚠️ Potential issue | 🟠 MajorThe unsubscribe callback can tear down a newer subscription set.
Because the closure mutates the
Setcaptured at subscribe time, calling an old unsubscribe afterclear()/re-registration can delete the currentthis.handlers[event]entry as soon as the old set becomes empty. Guard this against a replaced set before mutating the map.Suggested fix
return () => { - handlers!.delete(handler); - if (handlers!.size === 0) { + const current = this.handlers.get(event); + if (!current || current !== handlers) { + return; + } + current.delete(handler); + if (current.size === 0) { this.handlers.delete(event); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/sources/app-event.source.ts` around lines 45 - 49, The unsubscribe closure currently mutates the captured Set (handlers) and can remove a newly registered set if an old unsubscribe runs after re-registration; fix by reading the current set from this.handlers for the same event inside the returned function and only modify the map when the current set is the same Set captured by subscribe and its size becomes 0. In other words, inside the returned () => { ... } handler, guard with a check like currentSet === handlers before deleting this.handlers.delete(event), and also null-check handlers/currentSet before calling delete on the Set to avoid touching a replaced set.apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts-193-195 (1)
193-195:⚠️ Potential issue | 🟠 MajorReturn a real tool error when the channel is missing.
A plain
{ error: ... }payload still looks like a successful tool invocation to MCP clients, so callers checkingresult.isErrorwill miss this failure path. Throw the same tool/MCP error type the rest of the demo uses instead of encoding the failure in the payload.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts` around lines 193 - 195, Replace the plain return payload with a thrown tool/MCP error so callers see a real failure: instead of `return { error: \`Channel "${input.channelName}" not found\` };` throw the same demo error type used elsewhere (e.g. `throw new ToolError(\`Channel "${input.channelName}" not found\`)` or the project's MCP error class), import that error class into session-tools.tool.ts, and use it where `channel` is falsy so callers checking result.isError will detect the failure.libs/sdk/src/channel/channel-scope.helper.ts-51-62 (1)
51-62:⚠️ Potential issue | 🟠 Major
channelsConfig.defaultMetanever leaves config.
RegisterChannelCapabilitiesArgsacceptschannelsConfig, andChannelsConfigOptionsdocumentsdefaultMeta, but this helper drops it before constructingChannelNotificationService. Server-level default channel metadata is dead config right now.Also applies to: 70-71
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel-scope.helper.ts` around lines 51 - 62, The helper is stripping channelsConfig.defaultMeta from the config so server-level default channel metadata never gets passed into the notification service; update the destructuring and construction so defaultMeta is preserved and forwarded: when unpacking RegisterChannelCapabilitiesArgs avoid dropping channelsConfig.defaultMeta (or capture it as defaultMeta) and include that defaultMeta in the ChannelNotificationService instantiation (and any other places referenced at the same spots around lines 70-71), ensuring ChannelsConfigOptions.defaultMeta flows through to ChannelNotificationService.libs/sdk/src/channel/channel.registry.ts-71-85 (1)
71-85:⚠️ Potential issue | 🟠 MajorFail fast on duplicate channel names.
The registry keys instances by DI token, but every delivery path here is name-based (
findByName(), subscriptions,meta.source). Two different channel definitions that resolve to the sameinstance.namewill be ambiguous at runtime; reject duplicates during initialization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel.registry.ts` around lines 71 - 85, During initialize(), detect duplicate channel names and fail fast: after constructing ChannelInstance (symbol ChannelInstance and variable instance) but before adding to this.instances and this.localRows, check whether instance.name has already been seen (track names with a Set or Map) and if so throw an error identifying the duplicate name and the conflicting tokens/definitions; otherwise record the name and proceed to set this.instances and push the localRows entry. This ensures name-based lookup methods like findByName() won't be ambiguous.docs/frontmcp/servers/channels.mdx-302-307 (1)
302-307:⚠️ Potential issue | 🟠 MajorClarify that replay buffers only global events.
Step 2 currently reads like every notification is stored. The rest of the channel docs/security model says replay is only for global events, so this wording makes targeted agent/job results sound replayable to future sessions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/servers/channels.mdx` around lines 302 - 307, The wording in step 2 is misleading: only global events are stored in the replay buffer, not every notification; update the docs around onEvent(), the in-memory ring buffer description, and replayBufferedEvents(sessionId) to state explicitly that only global (non-targeted) events are buffered and replayed, and clarify that replayed events include meta.replayed="true" while targeted agent/job results are excluded from the buffer and will not be replayed to future sessions.libs/sdk/src/common/interfaces/channel.interface.ts-111-119 (1)
111-119:⚠️ Potential issue | 🟠 MajorDon't silently drop replies from
channel-reply.Any
twoWaychannel gets reply support registered, but the defaultonReply()only logs a warning. That makes reply tool calls look successful while discarding the outbound message. Throw an explicit error here or rejecttwoWay: trueduring registration whenonReply()is not overridden.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/interfaces/channel.interface.ts` around lines 111 - 119, The default onReply implementation currently only logs a warning and silently discards replies; change it to fail loudly by throwing an explicit Error from the onReply(reply, meta?) method (include channelName in the message) so reply tool calls cannot appear successful when unanswered. Alternatively (or additionally), add a validation where channels are registered (check the registration code that reads the twoWay flag) to reject/throw if twoWay === true but the subclass has not overridden onReply, using a runtime check like comparing the prototype implementation to ensure subclasses implemented onReply. Ensure references to onReply, twoWay, and channelName are used in the error/validation messages to make the failure clear.libs/skills/catalog/frontmcp-channels/examples/channel-sources/service-connector.md-61-67 (1)
61-67:⚠️ Potential issue | 🟠 MajorEither implement
onReply()here or droptwoWay: true.This snippet auto-registers
channel-reply, but the class never forwards replies back to WhatsApp. As copied, readers get a reply tool that silently does nothing.Also applies to: 69-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/skills/catalog/frontmcp-channels/examples/channel-sources/service-connector.md` around lines 61 - 67, The Channel configuration sets twoWay: true which auto-registers the channel-reply handler but the class does not implement onReply(), so replies are never forwarded; either remove twoWay: true or implement an onReply(payload) method on the channel class that forwards incoming replies to the WhatsApp service by invoking the SendWhatsAppTool (or the equivalent send method) and handling errors/acknowledgements; reference the Channel decorator configuration (name: 'whatsapp', twoWay: true), the generated channel-reply behavior, and the SendWhatsAppTool/onReply method to locate where to add the forwarding logic.docs/frontmcp/servers/channels.mdx-444-446 (1)
444-446:⚠️ Potential issue | 🟠 MajorUse the public notification service in this warning.
channel.pushNotification()looks like an internal instance hook, while the public surface in this PR isscope.channelNotifications.send()/sendToSession(). Point readers at that API so copied examples stay on the supported path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/frontmcp/servers/channels.mdx` around lines 444 - 446, Update the warning to reference the public notification API instead of the internal instance hook: replace mentions of channel.pushNotification() with the public surface scope.channelNotifications.send() and/or scope.channelNotifications.sendToSession() (and note passing targetSessionId where session-scoped delivery is required) so examples point to the supported API; update the text in the Warning block to call out these methods by name and indicate which one enforces session isolation.libs/sdk/src/channel/channel-notification.service.ts-119-123 (1)
119-123:⚠️ Potential issue | 🟠 MajorDon't let
additionalMeta.sourceoverride the selected channel.
send('deploy-alerts', ..., { source: 'error-alerts' })currently routes toerror-alertssubscribers because the spread happens after the explicitsource. That breaks the method contract and can leak a manual push into the wrong channel.🛠️ Suggested fix
const meta: Record<string, string> = { - source: channelName, ...(additionalMeta ?? {}), + source: channelName, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel-notification.service.ts` around lines 119 - 123, The send method currently lets additionalMeta.source override the intended channel because source is set before spreading additionalMeta; update the merge so the explicit channelName wins (e.g., merge additionalMeta first and then set source to channelName, or remove any source key from additionalMeta before merging) in the send function of channel-notification.service.ts so send(channelName, ...) cannot be hijacked by additionalMeta.source.libs/sdk/src/channel/channel.instance.ts-91-102 (1)
91-102:⚠️ Potential issue | 🟠 MajorOnly publish the service context after
onConnect()succeeds.
_serviceContextis assigned beforeonConnect()runs. IfonConnect()throws, the instance still looks connected and teardown/follow-up logic will act on a half-initialized connector.Suggested change
- this._serviceContext = ctx; - try { await ctx.onConnect(); + this._serviceContext = ctx; const sourceLabel = this.metadata.source.type === 'service' ? (this.metadata.source as { service: string }).service : this.metadata.source.type; logger.info(`Channel "${this.name}" connected (${sourceLabel})`); } catch (err) { + this._serviceContext = undefined; logger.error(`Channel "${this.name}" service failed to connect`, { error: err }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel.instance.ts` around lines 91 - 102, The instance sets this._serviceContext before awaiting ctx.onConnect(), so a thrown error leaves the channel appearing connected; move the assignment to after await ctx.onConnect() (or assign to a temp variable and set this._serviceContext only on success) so that onConnect() must complete without throwing before publishing the service context; ensure any existing catch block does not leave this._serviceContext set when ctx.onConnect() fails and keep the existing logging (use this.name and metadata for context).libs/sdk/src/channel/channel.instance.ts-181-187 (1)
181-187:⚠️ Potential issue | 🟠 MajorKeep
meta.sourceauthoritative.The current merge order lets
staticMetaor per-eventmetaoverridesource, so a channel can mislabel its own notifications. Anything downstream that keys offmeta.sourcewill then see the wrong channel.Suggested change
const mergedMeta: Record<string, string> = { - source: this.name, ...(this.staticMeta ?? {}), ...(meta ?? {}), + source: this.name, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel.instance.ts` around lines 181 - 187, The merge currently allows staticMeta or the per-notification meta to override the channel's source; in pushNotification, ensure the channel's source (this.name) is authoritative by applying it after merging staticMeta and meta (or explicitly setting mergedMeta.source = this.name) so mergedMeta.source cannot be overridden by this.staticMeta or the meta parameter.
🟡 Minor comments (8)
libs/skills/catalog/frontmcp-channels/examples/channel-sources/file-watcher.md-61-66 (1)
61-66:⚠️ Potential issue | 🟡 MinorClear
lastSizeon disconnect to prevent stale replay offsets.
onDisconnect()closes watchers but keepslastSize. On reconnect, this can skip appended content until file length exceeds prior session length.💡 Suggested fix
async onDisconnect(): Promise<void> { for (const watcher of this.watchers) { watcher.close(); } this.watchers = []; + this.lastSize.clear(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/skills/catalog/frontmcp-channels/examples/channel-sources/file-watcher.md` around lines 61 - 66, onDisconnect currently closes each watcher (this.watchers) but does not clear the per-file replay offset (lastSize), which can cause skipped appended data on reconnect; update onDisconnect to both close watchers and reset the associated lastSize state (e.g., clear this.lastSize map or set each watcher.lastSize = 0) so that onReconnect starts tailing from the file end correctly; ensure you reference and clear the same lastSize storage used by the file watcher logic in this class (the lastSize field) after closing watchers and before emptying this.watchers.apps/e2e/demo-e2e-channels/src/main.ts-4-5 (1)
4-5:⚠️ Potential issue | 🟡 MinorAdd upper bound validation for the
PORTenvironment variable.
parseInt()accepts strings with junk suffixes ("3120abc" → 3120), and the current guard only checksNumber.isFinite(rawPort) && rawPort > 0, which allows values above65535through. These invalid ports fail later at HTTP server bind time instead of being caught early.Suggested fix
-const rawPort = parseInt(process.env['PORT'] ?? '3120', 10); -const port = Number.isFinite(rawPort) && rawPort > 0 ? rawPort : 3120; +const rawPort = Number(process.env['PORT'] ?? 3120); +const port = + Number.isInteger(rawPort) && rawPort > 0 && rawPort <= 65_535 ? rawPort : 3120;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-channels/src/main.ts` around lines 4 - 5, The current PORT parsing using parseInt(raw) allows junk suffixes and doesn't enforce the valid TCP port range; update the logic that computes rawPort/port (the parseInt(process.env['PORT'] ?? '3120') usage) to first validate the raw env string contains only digits (e.g., /^\d+$/) before parsing, then parse to an integer and ensure Number.isInteger(rawPort) and rawPort >= 1 && rawPort <= 65535; if validation fails, fall back to the default 3120 or otherwise handle the error so invalid or out‑of‑range ports are rejected early.libs/sdk/src/transport/adapters/transport.local.adapter.ts-153-164 (1)
153-164:⚠️ Potential issue | 🟡 MinorRemove the
as unknown ascast and accessthis.scope.channelsdirectly.
Scopealready exposes a properly typedchannelsgetter returningChannelRegistry | undefined. The structural cast disables compile-time checking for no reason; simply usethis.scope.channels?.hasAny()andthis.scope.channels?.getCapabilities()instead.As per coding guidelines
**/*.{ts,tsx}: Use strict type checking in TypeScript; avoidanytypes without strong justification.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/transport/adapters/transport.local.adapter.ts` around lines 153 - 164, Remove the structural cast and access the typed channels API directly: replace the temporary "scope" variable that uses "as unknown as { channels?: ... }" with direct references to this.scope.channels (the ChannelRegistry getter), e.g. compute channelCapabilities from this.scope.channels?.getCapabilities() and compute channelInstructions using this.scope.channels?.hasAny() and this.scope.channels?.getChannelInstances(); ensure you remove the cast so TypeScript uses the real ChannelRegistry types and adjust null/undefined guards (?.) where needed to preserve the existing behavior for channelCapabilities and channelInstructions.apps/e2e/demo-e2e-channels/src/apps/channels/channels/replay-alert.channel.ts-14-19 (1)
14-19:⚠️ Potential issue | 🟡 MinorValidate the replay event payload before formatting a notification.
The cast on line 15 means bad
app-eventdata turns intoEvent#undefined: undefinedinstead of failing fast. Parse the payload with Zod before readingindexandmessagefields.Suggested fix
import { Channel, ChannelContext } from '@frontmcp/sdk'; import type { ChannelNotification } from '@frontmcp/sdk'; +import { z } from 'zod'; + +const replayAlertPayloadSchema = z.object({ + index: z.number(), + message: z.string(), +}); `@Channel`({ name: 'replay-alerts', @@ export class ReplayAlertChannel extends ChannelContext { async onEvent(payload: unknown): Promise<ChannelNotification> { - const data = payload as { index: number; message: string }; + const data = replayAlertPayloadSchema.parse(payload); return { content: `Event #${data.index}: ${data.message}`, meta: { index: String(data.index) }, }; } }Per coding guidelines
**/*.{ts,tsx}: Follow existing TypeScript patterns and keep strict typing. Avoid unsafe type assertions without validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-channels/src/apps/channels/channels/replay-alert.channel.ts` around lines 14 - 19, onEvent currently unsafely casts payload to {index:number; message:string} and formats Event `#undefined` when input is wrong; validate the payload with a Zod schema before reading fields. In the onEvent method create a Zod object schema for { index: z.number(), message: z.string() }, parse or safeParse the incoming payload, throw or return an appropriate error/notification on failure, and only format and return the ChannelNotification after successful validation (refer to onEvent and ChannelNotification to locate the code).apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts-15-15 (1)
15-15:⚠️ Potential issue | 🟡 Minor
metais modeled as required even though channel notifications allow it to be omitted.This helper stores
msg.params.metabehind a non-optionalRecord<string, string>. Notifications without metadata will be recorded asundefinedwith the wrong type, which makes downstream assertions onnotification.meta.foounsafe. Either make the field optional insessionNotificationsor normalize missing metadata to{}.As per coding guidelines,
Implement onEvent(payload) to transform source payloads into { content, meta? }.Also applies to: 32-35
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts` at line 15, sessionNotifications currently types meta as a required Record<string,string> which breaks when channel notifications omit meta; change the stored shape to allow meta?: Record<string,string> or normalize missing meta to an empty object when recording. Update the Map declaration for sessionNotifications to Map<string, Array<{ content: string; meta?: Record<string,string> }>> (or keep required but always set meta = {}), and implement onEvent(payload) to transform incoming payloads into { content, meta? } — i.e., extract payload.params.content and set payload.params.meta ?? {} (or omit meta) before pushing into sessionNotifications; apply the same change to the other occurrences referenced around the block (lines ~32-35).libs/skills/catalog/frontmcp-channels/references/channel-sources.md-2-3 (1)
2-3:⚠️ Potential issue | 🟡 MinorThe page metadata still says “five” source types.
Both the frontmatter description and the intro say five, but this page now documents seven: webhook, app-event, agent-completion, job-completion, service, file-watcher, and manual.
Also applies to: 8-8
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/skills/catalog/frontmcp-channels/references/channel-sources.md` around lines 2 - 3, The frontmatter description key and the intro paragraph still say "five" source types but the page documents seven; update the frontmatter 'description' value and the intro sentence that mentions "five" to say "seven" and optionally enumerate the seven types (webhook, app-event, agent-completion, job-completion, service, file-watcher, manual) so the text matches the documented list; make the same update wherever the phrase "five source types" appears (including the duplicate in 8-8).libs/sdk/src/channel/sources/webhook.source.ts-55-56 (1)
55-56:⚠️ Potential issue | 🟡 MinorQuery parameters may contain arrays, not just strings.
Express and Fastify parse repeated query parameters (e.g.,
?id=1&id=2) as arrays. The cast toRecord<string, string>may lose data or cause downstream issues if array values are expected.Consider adjusting the type to handle both cases:
- query: req.query as Record<string, string> | undefined, + query: req.query as Record<string, string | string[]> | undefined,And update the
WebhookPayloadinterface accordingly at line 18.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/sources/webhook.source.ts` around lines 55 - 56, The current cast "query: req.query as Record<string, string> | undefined" loses array query params; change the query typing to accept string or string[] (e.g., Record<string, string | string[]> | undefined) and update the WebhookPayload interface (the type defined around "WebhookPayload") so its query property uses string | string[] for values; ensure any code consuming payload.query handles both single values and arrays (e.g., normalize to array or pick first element where appropriate).apps/e2e/demo-e2e-channels/src/apps/channels/channels/messaging-service.channel.ts-105-113 (1)
105-113:⚠️ Potential issue | 🟡 MinorAdd validation schema for the incoming event payload.
onEvent()receives anunknownpayload but uses an unsafe type assertion on line 106. This bypasses validation and could produce malformed notification content/metadata if the payload shape is unexpected. Since zod is already imported in this file, use a schema with.parse()to validate the payload at the channel boundary:Suggested fix
+const incomingMessageSchema = z.object({ + from: z.string(), + text: z.string(), + chatId: z.string(), +}); + export class MessagingServiceChannel extends ChannelContext { @@ async onEvent(payload: unknown): Promise<ChannelNotification> { - const msg = payload as { from: string; text: string; chatId: string }; + const msg = incomingMessageSchema.parse(payload); return { content: `${msg.from}: ${msg.text}`, meta: { chat_id: msg.chatId, sender: msg.from,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-channels/src/apps/channels/channels/messaging-service.channel.ts` around lines 105 - 113, onEvent currently unsafely asserts payload to { from, text, chatId }; add a zod schema (e.g., const EventSchema = z.object({ from: z.string(), text: z.string(), chatId: z.string() })) and use EventSchema.parse(payload) inside onEvent to validate and obtain a typed msg before building the ChannelNotification; if parse throws, either let the exception bubble or catch and return/throw a clear error so malformed payloads are rejected at the channel boundary. Ensure you reference the onEvent method and the returned ChannelNotification when implementing the schema-based parse.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fc0acd66-0db5-46d1-b44b-ee969376b092
📒 Files selected for processing (90)
apps/e2e/demo-e2e-channels/e2e/channels-errors.e2e.spec.tsapps/e2e/demo-e2e-channels/e2e/channels-file-watcher.e2e.spec.tsapps/e2e/demo-e2e-channels/e2e/channels-replay.e2e.spec.tsapps/e2e/demo-e2e-channels/e2e/channels-security.e2e.spec.tsapps/e2e/demo-e2e-channels/e2e/channels.e2e.spec.tsapps/e2e/demo-e2e-channels/jest.e2e.config.tsapps/e2e/demo-e2e-channels/project.jsonapps/e2e/demo-e2e-channels/src/apps/channels/channels/chat-bridge.channel.tsapps/e2e/demo-e2e-channels/src/apps/channels/channels/deploy-alert.channel.tsapps/e2e/demo-e2e-channels/src/apps/channels/channels/error-alert.channel.tsapps/e2e/demo-e2e-channels/src/apps/channels/channels/file-watcher.channel.tsapps/e2e/demo-e2e-channels/src/apps/channels/channels/manual-status.channel.tsapps/e2e/demo-e2e-channels/src/apps/channels/channels/messaging-service.channel.tsapps/e2e/demo-e2e-channels/src/apps/channels/channels/replay-alert.channel.tsapps/e2e/demo-e2e-channels/src/apps/channels/index.tsapps/e2e/demo-e2e-channels/src/apps/channels/tools/emit-app-event.tool.tsapps/e2e/demo-e2e-channels/src/apps/channels/tools/file-watcher-tools.tool.tsapps/e2e/demo-e2e-channels/src/apps/channels/tools/list-reply-log.tool.tsapps/e2e/demo-e2e-channels/src/apps/channels/tools/replay-tools.tool.tsapps/e2e/demo-e2e-channels/src/apps/channels/tools/send-channel-notification.tool.tsapps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.tsapps/e2e/demo-e2e-channels/src/apps/channels/tools/simulate-incoming.tool.tsapps/e2e/demo-e2e-channels/src/config.tsapps/e2e/demo-e2e-channels/src/main.tsapps/e2e/demo-e2e-channels/tsconfig.app.jsonapps/e2e/demo-e2e-channels/tsconfig.jsonapps/e2e/demo-e2e-channels/webpack.config.jsdocs/docs.jsondocs/frontmcp/features/channels.mdxdocs/frontmcp/features/decorator-driven-development.mdxdocs/frontmcp/guides/your-first-channel.mdxdocs/frontmcp/sdk-reference/contexts/channel-context.mdxdocs/frontmcp/sdk-reference/decorators/channel.mdxdocs/frontmcp/sdk-reference/registries/channel-registry.mdxdocs/frontmcp/servers/channels.mdxlibs/sdk/src/channel/README.mdlibs/sdk/src/channel/channel-notification.service.tslibs/sdk/src/channel/channel-scope.helper.tslibs/sdk/src/channel/channel.events.tslibs/sdk/src/channel/channel.instance.tslibs/sdk/src/channel/channel.registry.tslibs/sdk/src/channel/channel.types.tslibs/sdk/src/channel/channel.utils.tslibs/sdk/src/channel/flows/list-channels.flow.tslibs/sdk/src/channel/flows/send-channel-notification.flow.tslibs/sdk/src/channel/index.tslibs/sdk/src/channel/reply/channel-reply.tool.tslibs/sdk/src/channel/reply/reply.types.tslibs/sdk/src/channel/sources/agent-completion.source.tslibs/sdk/src/channel/sources/app-event.source.tslibs/sdk/src/channel/sources/index.tslibs/sdk/src/channel/sources/job-completion.source.tslibs/sdk/src/channel/sources/webhook.source.tslibs/sdk/src/common/decorators/channel.decorator.tslibs/sdk/src/common/decorators/index.tslibs/sdk/src/common/entries/channel.entry.tslibs/sdk/src/common/entries/index.tslibs/sdk/src/common/interfaces/channel.interface.tslibs/sdk/src/common/interfaces/index.tslibs/sdk/src/common/interfaces/internal/registry.interface.tslibs/sdk/src/common/metadata/app.metadata.tslibs/sdk/src/common/metadata/channel.metadata.tslibs/sdk/src/common/metadata/front-mcp.metadata.tslibs/sdk/src/common/metadata/index.tslibs/sdk/src/common/records/channel.record.tslibs/sdk/src/common/records/index.tslibs/sdk/src/common/schemas/annotated-class.schema.tslibs/sdk/src/common/tokens/app.tokens.tslibs/sdk/src/common/tokens/channel.tokens.tslibs/sdk/src/common/tokens/front-mcp.tokens.tslibs/sdk/src/common/tokens/index.tslibs/sdk/src/front-mcp/front-mcp.tslibs/sdk/src/index.tslibs/sdk/src/notification/index.tslibs/sdk/src/notification/notification.service.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/transport/adapters/transport.local.adapter.tslibs/sdk/src/transport/mcp-handlers/initialize-request.handler.tslibs/skills/catalog/frontmcp-channels/SKILL.mdlibs/skills/catalog/frontmcp-channels/examples/channel-sources/agent-notify.mdlibs/skills/catalog/frontmcp-channels/examples/channel-sources/app-errors.mdlibs/skills/catalog/frontmcp-channels/examples/channel-sources/file-watcher.mdlibs/skills/catalog/frontmcp-channels/examples/channel-sources/job-completion.mdlibs/skills/catalog/frontmcp-channels/examples/channel-sources/replay-buffer.mdlibs/skills/catalog/frontmcp-channels/examples/channel-sources/service-connector.mdlibs/skills/catalog/frontmcp-channels/examples/channel-sources/webhook-github.mdlibs/skills/catalog/frontmcp-channels/examples/channel-two-way/whatsapp-bridge.mdlibs/skills/catalog/frontmcp-channels/references/channel-sources.mdlibs/skills/catalog/frontmcp-channels/references/channel-two-way.mdlibs/skills/catalog/skills-manifest.json
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
libs/sdk/src/channel/channel.instance.ts (2)
28-38: Consider removing underscore prefix from private fields.The coding guidelines recommend using private fields without underscore prefix and exposing via getters. This file uses
_providers,_channelNotificationService,_serviceContext,_replayBuffer, and_maxReplayEventswith underscores.This is a minor style inconsistency that could be addressed for guideline compliance if the team prefers. As per coding guidelines: "Use private fields without underscore prefix and expose via getters following idiomatic TypeScript patterns."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel.instance.ts` around lines 28 - 38, The private field names in ChannelInstance (e.g., _providers, _channelNotificationService, _serviceContext, _replayBuffer, _maxReplayEvents) use an underscore prefix which conflicts with the style guide; rename these private fields to providers, channelNotificationService, serviceContext, replayBuffer, and maxReplayEvents respectively, update all assignments (constructor, initializers) and internal references within the class to use the new names, and keep or add public getters with the intended exposed names if needed so external API remains unchanged; ensure imports/exports and any tests referencing the old private names are updated accordingly.
200-202: Consider logging a warning when notification service is not set.Per the channel context documentation, calling
pushIncoming()early should warn. While the initialization order guarantees the service is wired beforeconnectService(), a warning here would help diagnose misconfigurations or unexpected call sequences.📝 Proposed fix to add warning
if (!this._channelNotificationService) { + const logger = this._providers.get(FrontMcpLogger); + logger.warn(`Channel "${this.name}": notification service not set, notification dropped`); return; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel.instance.ts` around lines 200 - 202, When pushIncoming finds this._channelNotificationService unset (the current check if (!this._channelNotificationService) { return; }) add a warning log before returning; use the channel instance's logger (e.g., this.logger or this._logger) if available, otherwise fallback to console.warn, and include identifying context (channel id/name or other instance properties) and a clear message such as "pushIncoming called before channelNotificationService initialized" to aid debugging.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts`:
- Around line 16-24: The FakeServer only implements notification() but is later
cast to McpServer; add a request() stub to the fake so it safely implements the
expected server API: update the FakeServer interface to include request(method:
string, params?: unknown): Promise<unknown> (or void) and implement that stub in
createFakeServer to return a resolved Promise or appropriate default;
alternatively replace the cast with a properly typed test double that implements
both notification() and request(), so NotificationService.getRoots() calls to
server.request() won't break at runtime.
In `@libs/sdk/src/channel/channel-notification.service.ts`:
- Around line 87-111: sendToSession currently assumes
NotificationService.sendCustomNotification always succeeds, so it can return
true even when per-session transport writes fail; update the API and call path
so sendCustomNotification (or a new sendCustomNotificationWithResult) returns a
boolean/Result or throws on send failures, then have sendToSession call that and
return false when a send error occurs (or rethrow). Specifically, modify
NotificationService.sendCustomNotification and its internal per-session send
logic to surface per-session errors (instead of swallowing them) and change
channel-notification.service.ts sendToSession to await/inspect that result (or
catch the thrown error) before returning true; keep prechecks
(getRegisteredServer, supportsChannels, isChannelSubscribed) as-is but ensure
send failures cause sendToSession to report failure.
In `@libs/sdk/src/channel/channel-scope.helper.ts`:
- Around line 194-200: The teardown function currently aborts on the first
failing disconnectService() call; change it to be best-effort by handling errors
per-channel or using Promise.allSettled on serviceChannels.map(ci =>
ci.disconnectService()), collect/log any errors but do not throw, then always
run unsubscribers (for(const unsub of unsubscribers) unsub()) and
channelEventBus.clear() in a finally/after path so teardown completes even if
some disconnects fail; reference the teardown function, serviceChannels,
disconnectService(), unsubscribers, and channelEventBus.clear() when making the
change.
- Around line 177-180: The loop that calls instance.connectService() on
serviceChannels must rollback already-connected instances if a later connect
fails: track successful instances (e.g., connectedInstances array) as you await
instance.connectService(); if a connectService() throws, iterate over
connectedInstances and call/await instance.disconnectService() (handling/logging
any errors) before rethrowing the original error, and ensure the helper’s
teardown callback remains consistent with this rollback behavior so no
subscriptions/event bus remain open on initialization failure.
In `@libs/sdk/src/channel/channel.instance.ts`:
- Around line 140-151: The switch over ChannelKind in the method that returns a
ChannelContext (handling ChannelKind.FUNCTION and ChannelKind.CLASS_TOKEN) is
non-exhaustive and can return undefined; update the switch to handle unknown/new
enum values by adding a default branch that throws a clear error (or an
exhaustive-check/assert) so the method always returns a ChannelContext or fails
fast; reference the switch on ChannelKind, the ChannelFunctionTokenRecord ->
provide() path that constructs a FunctionChannelContext, and the class-based
path using this.record.provide to guide where to add the default/exhaustive
check.
In `@libs/sdk/src/common/interfaces/channel.interface.ts`:
- Around line 117-121: Replace the generic Error thrown in the default
onReply(reply, meta?) implementation with one of the SDK's specific
protocol-aware error classes (so the channel-reply tool receives an MCP/JSON-RPC
compliant error). Locate the onReply method (referencing this.channelName) and
throw the appropriate SDK error type that includes an MCP error code and
implements toJsonRpcError(); ensure the thrown error message retains the
existing context about twoWay: true and advises overriding onReply(), and
confirm the error class' toJsonRpcError() produces the expected JSON-RPC shape.
In `@libs/sdk/src/scope/scope.instance.ts`:
- Around line 112-116: The stored _channelTeardown currently has type () => void
and is invoked fire-and-forget in Scope.shutdown(), but the helper returns a
Promise-based teardown (() => Promise<void>) so shutdown may return before
teardown completes and rejections are unhandled; change _channelTeardown's type
to () => Promise<void>, assign the helper's teardown directly, and await/return
_channelTeardown() inside Scope.shutdown() (handle errors with try/catch or
propagate the rejection) so connectors disconnect and subscriptions are released
before shutdown resolves; update any related uses of _channelTeardown and the
fields _scopeChannels, _channelNotificationService, _channelEventBus to preserve
async teardown contract.
- Around line 577-586: registerChannelCapabilities is being called without the
agent/job emitters so channel types like "agent-completion" and "job-completion"
will never emit notifications; update the call site to pass the emitter
callbacks (the subscribe/emit handlers used elsewhere) into
registerChannelCapabilities or, if those emitters are not available, throw/fail
fast when those source types are present. Specifically, modify the call in
scope.instance.ts where registerChannelCapabilities({ providers:
this.scopeProviders, owner: scopeRef, channelsList: appChannels, channelsConfig,
notificationService: this.notificationService, flowRegistry: this.scopeFlows,
toolRegistry: this.scopeTools, logger: this.logger }) is invoked to include the
agent/job emitter subscriptions (or a reference to the agent/job emitter
functions) so that "agent-completion" and "job-completion" channels are wired,
or add a guard that checks appChannels for those source types and errors if
emitters are not provided.
---
Nitpick comments:
In `@libs/sdk/src/channel/channel.instance.ts`:
- Around line 28-38: The private field names in ChannelInstance (e.g.,
_providers, _channelNotificationService, _serviceContext, _replayBuffer,
_maxReplayEvents) use an underscore prefix which conflicts with the style guide;
rename these private fields to providers, channelNotificationService,
serviceContext, replayBuffer, and maxReplayEvents respectively, update all
assignments (constructor, initializers) and internal references within the class
to use the new names, and keep or add public getters with the intended exposed
names if needed so external API remains unchanged; ensure imports/exports and
any tests referencing the old private names are updated accordingly.
- Around line 200-202: When pushIncoming finds this._channelNotificationService
unset (the current check if (!this._channelNotificationService) { return; }) add
a warning log before returning; use the channel instance's logger (e.g.,
this.logger or this._logger) if available, otherwise fallback to console.warn,
and include identifying context (channel id/name or other instance properties)
and a clear message such as "pushIncoming called before
channelNotificationService initialized" to aid debugging.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8974f865-2066-42f9-ba28-3dcddf04fc9b
📒 Files selected for processing (19)
apps/e2e/demo-e2e-channels/src/apps/channels/channels/messaging-service.channel.tsapps/e2e/demo-e2e-channels/src/apps/channels/channels/replay-alert.channel.tsapps/e2e/demo-e2e-channels/src/apps/channels/tools/list-reply-log.tool.tsapps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.tsapps/e2e/demo-e2e-channels/src/main.tslibs/sdk/src/channel/channel-notification.service.tslibs/sdk/src/channel/channel-scope.helper.tslibs/sdk/src/channel/channel.instance.tslibs/sdk/src/channel/channel.registry.tslibs/sdk/src/channel/reply/channel-reply.tool.tslibs/sdk/src/channel/sources/agent-completion.source.tslibs/sdk/src/channel/sources/app-event.source.tslibs/sdk/src/common/decorators/channel.decorator.tslibs/sdk/src/common/interfaces/channel.interface.tslibs/sdk/src/front-mcp/front-mcp.tslibs/sdk/src/scope/scope.instance.tslibs/sdk/src/transport/adapters/transport.local.adapter.tslibs/skills/catalog/frontmcp-channels/examples/channel-sources/file-watcher.mdlibs/skills/catalog/frontmcp-channels/references/channel-sources.md
✅ Files skipped from review due to trivial changes (2)
- apps/e2e/demo-e2e-channels/src/apps/channels/tools/list-reply-log.tool.ts
- libs/skills/catalog/frontmcp-channels/references/channel-sources.md
🚧 Files skipped from review as they are similar to previous changes (8)
- libs/sdk/src/front-mcp/front-mcp.ts
- apps/e2e/demo-e2e-channels/src/main.ts
- apps/e2e/demo-e2e-channels/src/apps/channels/channels/replay-alert.channel.ts
- libs/sdk/src/channel/sources/agent-completion.source.ts
- libs/sdk/src/channel/sources/app-event.source.ts
- libs/sdk/src/channel/reply/channel-reply.tool.ts
- libs/skills/catalog/frontmcp-channels/examples/channel-sources/file-watcher.md
- apps/e2e/demo-e2e-channels/src/apps/channels/channels/messaging-service.channel.ts
Contributor
Performance Test ResultsStatus: ✅ All tests passed Summary
Total: 101 tests across 21 projects 📊 View full report in workflow run Generated at: 2026-04-06T22:29:26.334Z |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts (1)
11-18: Replaceunknownwith concreteMcpServerLikeinterface and remove castThe structural interface at line 12 and the cast at lines 78-81 use
unknownto bypass strict typing, which obscures the actual contract and allows test doubles to drift from the expected shape. Instead, define a concreteMcpServerLikeinterface matching what the code needs and use it directly.Suggested refactor
+interface McpServerLike { + notification(msg: { method: string; params: Record<string, unknown> }): void; +} + interface NotificationServiceLike { - registerServer(sessionId: string, server: unknown): void; + registerServer(sessionId: string, server: McpServerLike): void; unregisterServer(sessionId: string): void; setClientCapabilities(sessionId: string, capabilities: Record<string, unknown>): void; subscribeChannel(sessionId: string, channelName: string): boolean; unsubscribeChannel(sessionId: string, channelName: string): boolean; subscribeAllChannels(sessionId: string, channelNames: string[]): void; } @@ -interface FakeServer { - notification(msg: { method: string; params: Record<string, unknown> }): void; -} +type FakeServer = McpServerLike; @@ - notifications.registerServer( - input.sessionId, - fakeServer as unknown as Parameters<typeof notifications.registerServer>[1], - ); + notifications.registerServer(input.sessionId, fakeServer);Per coding guidelines, "Follow existing TypeScript patterns and keep strict typing" and "maintain strict TypeScript correctness."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts` around lines 11 - 18, The NotificationServiceLike interface currently uses unknown for the server parameter and tests cast to bypass typing; define a concrete McpServerLike interface that declares the exact members the code uses (e.g., methods/properties consumed by registerServer/unregisterServer flows) and replace the unknown types in NotificationServiceLike.registerServer and any other places using unknown with McpServerLike; remove the runtime casts (the test double casts around registerServer usage) so callers and tests use McpServerLike directly, ensuring all references (NotificationServiceLike, registerServer, any test helpers) are updated to the new type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts`:
- Around line 203-207: Replace the generic Error and the non-null assertion:
when channel is missing, call this.fail with a specific MCP-style error instance
(e.g., a NotFound/ChannelNotFound error class that implements toJsonRpcError())
constructed with the channel name, instead of new Error(...); remove the
channel! non-null assertion and call channel.pushNotification only after the
channel existence guard (the this.fail branch never returns), using the existing
symbols channel, this.fail, pushNotification, and
input.channelName/input.content/input.targetSessionId to locate and update the
code.
In `@libs/sdk/package.json`:
- Line 83: The SDK currently statically imports vectoriadb in
memory-skill.provider.ts and instantiates TFIDFVectoria in the
MemorySkillProvider constructor, causing module load failures when vectoriadb is
not installed; either revert vectoriadb to a regular dependency in
libs/sdk/package.json or change MemorySkillProvider to lazy-load the module (use
dynamic import like const { TFIDFVectoria } = await import('vectoriadb') inside
the constructor/initialization path) so the export of MemorySkillProvider from
the SDK entry point no longer triggers a hard require at module load time;
alternatively move skill providers to a separate optional export path if you
want to keep it optional.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts`:
- Around line 11-18: The NotificationServiceLike interface currently uses
unknown for the server parameter and tests cast to bypass typing; define a
concrete McpServerLike interface that declares the exact members the code uses
(e.g., methods/properties consumed by registerServer/unregisterServer flows) and
replace the unknown types in NotificationServiceLike.registerServer and any
other places using unknown with McpServerLike; remove the runtime casts (the
test double casts around registerServer usage) so callers and tests use
McpServerLike directly, ensuring all references (NotificationServiceLike,
registerServer, any test helpers) are updated to the new type.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 756254be-e5c9-4fca-a3e1-c8cd4089549a
📒 Files selected for processing (5)
apps/e2e/demo-e2e-channels/src/apps/channels/tools/list-reply-log.tool.tsapps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.tsapps/e2e/demo-e2e-channels/src/apps/channels/tools/simulate-incoming.tool.tslibs/sdk/package.jsonplugins/plugin-cache/package.json
✅ Files skipped from review due to trivial changes (1)
- plugins/plugin-cache/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/e2e/demo-e2e-channels/src/apps/channels/tools/list-reply-log.tool.ts
- apps/e2e/demo-e2e-channels/src/apps/channels/tools/simulate-incoming.tool.ts
…ing in channel management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
libs/sdk/src/channel/channel-scope.helper.ts (2)
89-102: Avoidas anycasts; use proper type narrowing or generics.Lines 94-95 use
as anyto bypass type checking forinstance.metadata.sourceandagentEmitterSubscribe. This pattern is repeated forjob-completion(lines 108-109). Consider defining discriminated union types for channel sources or using type guards to narrow the source type safely.♻️ Example type-safe approach
// Define source type with discriminated union type AgentCompletionSource = { type: 'agent-completion'; agentName?: string }; // Use type guard or assert if (sourceType === 'agent-completion') { const agentSource = instance.metadata.source as AgentCompletionSource; const unsub = wireAgentCompletionSource( instance, agentSource, agentEmitterSubscribe, logger, ); unsubscribers.push(unsub); }As per coding guidelines, "Use strict TypeScript with no
anytypes without strong justification."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel-scope.helper.ts` around lines 89 - 102, The code uses `as any` for `instance.metadata.source` and `agentEmitterSubscribe` when calling `wireAgentCompletionSource` (and similarly for the `job-completion` branch), so add proper types and narrow the union instead of casting: define discriminated union types for channel sources (e.g., an `AgentCompletionSource` with `type: 'agent-completion'`) or implement a type guard that checks `sourceType === 'agent-completion'` and then cast `instance.metadata.source` to the specific source type, and ensure `agentEmitterSubscribe` is the correct emitter type before passing to `wireAgentCompletionSource`; do the analogous change for the `job-completion` branch (e.g., `wireJobCompletionSource`) so both calls are fully typed without `any`.
154-170: Avoidas anyforowner.ref; define a proper owner reference type.Line 160 uses
ref: toolDef as anywhen constructing theEntryOwnerRef. If the owner reference can accept various tool definition types, consider widening thereffield's type inEntryOwnerRefor using a type assertion to a more specific type.♻️ Suggested improvement
const toolInstance = new ToolInstance(toolRecord, providers, { kind: 'scope', id: `_channel:${instance.name}`, - ref: toolDef as any, + ref: toolDef, });If
EntryOwnerRef.refdoesn't acceptToolType, consider updating the type definition toref: unknownor a union type that includes tool definitions.As per coding guidelines, "Use strict TypeScript with no
anytypes without strong justification."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel-scope.helper.ts` around lines 154 - 170, The code currently uses a loose cast `ref: toolDef as any` when constructing the owner ref for ToolInstance; change the type instead of using any by widening EntryOwnerRef.ref to accept the tool definition type (e.g., make EntryOwnerRef.ref: unknown | ToolType | UnionOfToolDefs) or add a specific ToolDef type to the union, then remove the `as any` cast in the tool creation loop (the one that builds new ToolInstance with ref) and adjust any other callers of EntryOwnerRef to comply with the new type; ensure normalizeTool, ToolInstance constructor signature and providers usage remain type-compatible after the EntryOwnerRef.ref type update.libs/di/src/tokens/token.factory.ts (1)
10-14: JSDoc examples are now inaccurate after Symbol.for() change.The examples show
// => Symbol('MyApp:type:UserService')and// => Symbol('MyApp:meta:config'), but the implementation now returnsSymbol.for(...)which has different semantics (globally shared vs. unique).📝 Proposed documentation fix
* // Create typed tokens * const serviceToken = tokens.type('UserService'); - * // => Symbol('MyApp:type:UserService') + * // => Symbol.for('MyApp:type:UserService') * * const metaToken = tokens.meta('config'); - * // => Symbol('MyApp:meta:config') + * // => Symbol.for('MyApp:meta:config')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/di/src/tokens/token.factory.ts` around lines 10 - 14, The JSDoc examples are incorrect because the factory now uses Symbol.for() (global symbols) instead of Symbol(), so update the examples in token.factory.ts to show the actual return value syntax (e.g. Symbol.for('MyApp:type:UserService') and Symbol.for('MyApp:meta:config')) and/or add a short note that tokens.type and tokens.meta produce globally registered symbols via Symbol.for rather than unique symbols; locate usages of tokens.type and tokens.meta in the file to update the example strings and explanatory text accordingly.libs/sdk/src/common/schemas/annotated-class.schema.ts (2)
4-17: Function definition placed before imports is unconventional.The
hasMetadataCompathelper function is defined at lines 10-17, but the import statements from../tokensfollow at lines 18-34. Standard TypeScript/JavaScript convention places all imports at the top of the file before any code definitions. This could cause confusion during maintenance.♻️ Suggested restructure
Move the
hasMetadataCompatfunction definition after all import statements (after line 42) to follow conventional module organization.Also applies to: 18-34
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/schemas/annotated-class.schema.ts` around lines 4 - 17, Move the helper function hasMetadataCompat so all import statements appear before any code: relocate the entire hasMetadataCompat function definition to after the existing imports (including the imports from ../tokens) to follow conventional TS/JS module ordering and improve maintainability; ensure references to Reflect and token usage remain intact after the move.
316-316: Inconsistent cast pattern for symbol-indexed property access.Line 316 uses
(v as unknown as Record<symbol, unknown>)[FrontMcpChannelTokens.type]while similar checks elsewhere (e.g., line 164, 179-181, 197, 220, 253, 269, 286) access the property directly withv[Token.type]without the intermediate cast.For consistency, use the same pattern across all schemas.
♻️ Proposed fix for consistency
// Function-style channel() builder - if ((v as unknown as Record<symbol, unknown>)[FrontMcpChannelTokens.type] !== undefined) { + if (v[FrontMcpChannelTokens.type] !== undefined) { return true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/common/schemas/annotated-class.schema.ts` at line 316, The check on line using (v as unknown as Record<symbol, unknown>)[FrontMcpChannelTokens.type] is inconsistent with other symbol-indexed accesses (which use v[Token.type]); replace this expression with the same pattern used elsewhere—access the symbol directly as v[FrontMcpChannelTokens.type] (or if needed for TS indexing safety use (v as any)[FrontMcpChannelTokens.type]) so the code matches the other checks for symbol properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@libs/sdk/src/channel/channel.instance.ts`:
- Around line 91-102: The catch block in the connectService flow (inside the try
around ctx.onConnect() in channel.instance.ts) only logs the error and clears
this._serviceContext but does not re-throw, so callers like
registerChannelCapabilities get a false-success; update the catch to re-throw
the error after logging (or throw a wrapped Error including channel name and
original err) so the promise rejects and upstream rollback can occur; keep the
existing this._serviceContext = undefined and logger.error call but ensure the
error is propagated by re-throwing `err` (or a new Error that includes `err`)
from the catch.
---
Nitpick comments:
In `@libs/di/src/tokens/token.factory.ts`:
- Around line 10-14: The JSDoc examples are incorrect because the factory now
uses Symbol.for() (global symbols) instead of Symbol(), so update the examples
in token.factory.ts to show the actual return value syntax (e.g.
Symbol.for('MyApp:type:UserService') and Symbol.for('MyApp:meta:config')) and/or
add a short note that tokens.type and tokens.meta produce globally registered
symbols via Symbol.for rather than unique symbols; locate usages of tokens.type
and tokens.meta in the file to update the example strings and explanatory text
accordingly.
In `@libs/sdk/src/channel/channel-scope.helper.ts`:
- Around line 89-102: The code uses `as any` for `instance.metadata.source` and
`agentEmitterSubscribe` when calling `wireAgentCompletionSource` (and similarly
for the `job-completion` branch), so add proper types and narrow the union
instead of casting: define discriminated union types for channel sources (e.g.,
an `AgentCompletionSource` with `type: 'agent-completion'`) or implement a type
guard that checks `sourceType === 'agent-completion'` and then cast
`instance.metadata.source` to the specific source type, and ensure
`agentEmitterSubscribe` is the correct emitter type before passing to
`wireAgentCompletionSource`; do the analogous change for the `job-completion`
branch (e.g., `wireJobCompletionSource`) so both calls are fully typed without
`any`.
- Around line 154-170: The code currently uses a loose cast `ref: toolDef as
any` when constructing the owner ref for ToolInstance; change the type instead
of using any by widening EntryOwnerRef.ref to accept the tool definition type
(e.g., make EntryOwnerRef.ref: unknown | ToolType | UnionOfToolDefs) or add a
specific ToolDef type to the union, then remove the `as any` cast in the tool
creation loop (the one that builds new ToolInstance with ref) and adjust any
other callers of EntryOwnerRef to comply with the new type; ensure
normalizeTool, ToolInstance constructor signature and providers usage remain
type-compatible after the EntryOwnerRef.ref type update.
In `@libs/sdk/src/common/schemas/annotated-class.schema.ts`:
- Around line 4-17: Move the helper function hasMetadataCompat so all import
statements appear before any code: relocate the entire hasMetadataCompat
function definition to after the existing imports (including the imports from
../tokens) to follow conventional TS/JS module ordering and improve
maintainability; ensure references to Reflect and token usage remain intact
after the move.
- Line 316: The check on line using (v as unknown as Record<symbol,
unknown>)[FrontMcpChannelTokens.type] is inconsistent with other symbol-indexed
accesses (which use v[Token.type]); replace this expression with the same
pattern used elsewhere—access the symbol directly as
v[FrontMcpChannelTokens.type] (or if needed for TS indexing safety use (v as
any)[FrontMcpChannelTokens.type]) so the code matches the other checks for
symbol properties.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 386c6a20-2c8d-46dd-9499-e7783ede7eb0
📒 Files selected for processing (9)
apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.tslibs/di/src/tokens/token.factory.tslibs/sdk/src/channel/channel-scope.helper.tslibs/sdk/src/channel/channel.instance.tslibs/sdk/src/common/interfaces/channel.interface.tslibs/sdk/src/common/schemas/annotated-class.schema.tslibs/sdk/src/scope/scope.instance.tsplugins/plugin-dashboard/package.jsonplugins/plugin-remember/package.json
✅ Files skipped from review due to trivial changes (2)
- plugins/plugin-dashboard/package.json
- plugins/plugin-remember/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/sdk/src/scope/scope.instance.ts
- apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts
…rror handling in channel instance
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (3)
apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts (1)
207-209:⚠️ Potential issue | 🟠 MajorUse a specific MCP error class instead of generic
Error.Line 208 throws
new Error(...), which loses structured MCP/JSON-RPC error semantics expected by this codebase.Suggested fix
+class ChannelNotFoundError extends Error { + readonly code = -32602; + + constructor(channelName: string) { + super(`Channel "${channelName}" not found`); + this.name = 'ChannelNotFoundError'; + } + + toJsonRpcError() { + return { code: this.code, message: this.message }; + } +} @@ const channel = scope.channels?.findByName(input.channelName); if (!channel) { - throw new Error(`Channel "${input.channelName}" not found`); + this.fail(new ChannelNotFoundError(input.channelName)); }#!/bin/bash # Verify existing error-handling conventions in adjacent tools and SDK context. rg -n 'toJsonRpcError\(|class .*Error' apps/e2e/demo-e2e-channels/src/apps/channels/tools --type ts rg -nC3 'throw new Error|this\.fail\(' apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts rg -nC3 'fail\(' libs/sdk/src --type tsBased on learnings: Applies to **/*.ts — "Use specific error classes with MCP error codes instead of generic errors; implement toJsonRpcError() method for JSON-RPC compliance."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts` around lines 207 - 209, Replace the generic throw new Error(...) with the project-specific MCP/JSON-RPC error type: construct and throw the MCP error class used across the repo (e.g., MCPError/JsonRpcError) with an appropriate MCP error code and message that includes input.channelName, and ensure the error implements toJsonRpcError() if required by callers; locate the throw in session-tools.tool.ts where channel and input.channelName are checked and swap the generic Error for the MCP error instance so upstream JSON-RPC semantics are preserved.libs/sdk/src/scope/scope.instance.ts (1)
577-589:⚠️ Potential issue | 🟠 MajorDon't leave
agent-completion/job-completionchannels inert.This scope path never passes either subscribe callback, so any declared channel with those source types initializes successfully but can never emit. Either plumb the emitter subscriptions through here or fail fast when one of those source types is present.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/scope/scope.instance.ts` around lines 577 - 589, The channel registration call to registerChannelCapabilities in scope.instance.ts currently never provides agentEmitterSubscribe or jobEmitterSubscribe, so channels with source types "agent-completion" or "job-completion" will be inert; update the registerChannelCapabilities invocation to pass the appropriate subscribe callbacks (e.g., agentEmitterSubscribe and jobEmitterSubscribe from the agent/job registries or scope-level equivalents) alongside providers/flowRegistry/toolRegistry/notificationService, or alternatively detect those source types in this scope path and throw a clear error to fail fast when either "agent-completion" or "job-completion" appears in appChannels; ensure you reference registerChannelCapabilities, appChannels, and the emitter subscribe function names so the subscriptions are wired or the initialization aborts.libs/sdk/src/channel/channel-scope.helper.ts (1)
91-121:⚠️ Potential issue | 🟠 MajorRun one shared best-effort cleanup path for all init/teardown failures.
Step 5 already installs live listeners, but rollback only happens when
connectService()fails. If a later wire, flow registration, or reply-tool registration throws, initialization rejects with listeners still attached and no teardown handle to release them. Also, one throwingunsub()currently aborts the rest of cleanup. Please factor disconnect/unsubscribe/channelEventBus.clear()into a single best-effort helper and invoke it from an outertry/catch, the service rollback, andteardown().Also applies to: 137-147, 177-220
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel-scope.helper.ts` around lines 91 - 121, Create a single best-effort cleanup helper (e.g., runBestEffortCleanup) that iterates the unsubscribers array and calls each unsub() inside its own try/catch, then calls channelEventBus.clear(), and use that helper from the outer initialization try/catch, the connectService() failure rollback path, and teardown(); update places that push results of wireAgentCompletionSource, wireJobCompletionSource, wireAppEventSource (and other wiring like reply-tool/flow registration) to always push their unsub function into the same unsubscribers array so the helper can clean them up, and ensure any thrown unsub() does not abort remaining cleanup.
🧹 Nitpick comments (2)
apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts (1)
11-18: Use concreteFakeServertype instead ofunknownfor server registration.Lines 12, 32, and 84 use
unknownfor MCP server protocol types, forcing a double-cast workaround. Replace the structuralNotificationServiceLike.registerServerparameter with a concreteFakeServertype to eliminate the cast chain and maintain strict typing.Suggested refactor
interface NotificationServiceLike { - registerServer(sessionId: string, server: unknown): void; + registerServer(sessionId: string, server: FakeServer): void; unregisterServer(sessionId: string): void; setClientCapabilities(sessionId: string, capabilities: Record<string, unknown>): void; subscribeChannel(sessionId: string, channelName: string): boolean; unsubscribeChannel(sessionId: string, channelName: string): boolean; subscribeAllChannels(sessionId: string, channelNames: string[]): void; } interface FakeServer { notification(msg: { method: string; params: Record<string, unknown> }): void; - request(msg: { method: string; params?: unknown }): Promise<unknown>; + request(msg: { method: string; params?: unknown }): Promise<Record<string, unknown>>; } - notifications.registerServer( - input.sessionId, - fakeServer as unknown as Parameters<typeof notifications.registerServer>[1], - ); + notifications.registerServer(input.sessionId, fakeServer);Per coding guidelines: "For MCP protocol outputs, prefer concrete MCP result types rather than
unknown" and "Follow existing TypeScript patterns and keep strict typing."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts` around lines 11 - 18, Change the NotificationServiceLike.registerServer parameter from unknown to the concrete FakeServer type and update all implementations and callers to use FakeServer (remove the double-cast workaround). Specifically, replace the server parameter type in NotificationServiceLike.registerServer with FakeServer, import/declare the FakeServer type in this module, and update any functions or classes that implement NotificationServiceLike or call registerServer (and other spots that previously used unknown for MCP server protocol types on lines referenced) to accept/return FakeServer so the casts are no longer required.libs/sdk/src/channel/channel-scope.helper.ts (1)
92-95: Remove theanyescapes from source and tool wiring.These casts sidestep the discriminated source/tool types on a protocol-facing path, so contract drift will only show up at runtime. Please thread the concrete channel source and tool entry types through this helper instead.
As per coding guidelines, "
Do not introduceanywithout clear justification" and "Use strict TypeScript with noanytypes without strong justification; useunknowninstead ofanyfor generic type defaults".Also applies to: 106-109, 157-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/sdk/src/channel/channel-scope.helper.ts` around lines 92 - 95, The helper currently bypasses TypeScript's discriminated source/tool types by casting instance.metadata.source and agentEmitterSubscribe to any when calling wireAgentCompletionSource (and the similar casts at the other occurrences around lines 106-109 and 157-160); update the helper signature to accept properly generic/concrete channel source and tool entry types and propagate those generics into the call to wireAgentCompletionSource and agentEmitterSubscribe so you can pass instance.metadata.source and the emitter without any casts; remove the any casts and adjust the helper's type parameters (or overloads) so instance: ChannelInstance<SourceType, ToolType> (or equivalent existing types) is used, or default to unknown rather than any if a generic default is needed, ensuring all three call sites use the concrete typed parameters instead of any.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts`:
- Around line 207-209: Replace the generic throw new Error(...) with the
project-specific MCP/JSON-RPC error type: construct and throw the MCP error
class used across the repo (e.g., MCPError/JsonRpcError) with an appropriate MCP
error code and message that includes input.channelName, and ensure the error
implements toJsonRpcError() if required by callers; locate the throw in
session-tools.tool.ts where channel and input.channelName are checked and swap
the generic Error for the MCP error instance so upstream JSON-RPC semantics are
preserved.
In `@libs/sdk/src/channel/channel-scope.helper.ts`:
- Around line 91-121: Create a single best-effort cleanup helper (e.g.,
runBestEffortCleanup) that iterates the unsubscribers array and calls each
unsub() inside its own try/catch, then calls channelEventBus.clear(), and use
that helper from the outer initialization try/catch, the connectService()
failure rollback path, and teardown(); update places that push results of
wireAgentCompletionSource, wireJobCompletionSource, wireAppEventSource (and
other wiring like reply-tool/flow registration) to always push their unsub
function into the same unsubscribers array so the helper can clean them up, and
ensure any thrown unsub() does not abort remaining cleanup.
In `@libs/sdk/src/scope/scope.instance.ts`:
- Around line 577-589: The channel registration call to
registerChannelCapabilities in scope.instance.ts currently never provides
agentEmitterSubscribe or jobEmitterSubscribe, so channels with source types
"agent-completion" or "job-completion" will be inert; update the
registerChannelCapabilities invocation to pass the appropriate subscribe
callbacks (e.g., agentEmitterSubscribe and jobEmitterSubscribe from the
agent/job registries or scope-level equivalents) alongside
providers/flowRegistry/toolRegistry/notificationService, or alternatively detect
those source types in this scope path and throw a clear error to fail fast when
either "agent-completion" or "job-completion" appears in appChannels; ensure you
reference registerChannelCapabilities, appChannels, and the emitter subscribe
function names so the subscriptions are wired or the initialization aborts.
---
Nitpick comments:
In `@apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.ts`:
- Around line 11-18: Change the NotificationServiceLike.registerServer parameter
from unknown to the concrete FakeServer type and update all implementations and
callers to use FakeServer (remove the double-cast workaround). Specifically,
replace the server parameter type in NotificationServiceLike.registerServer with
FakeServer, import/declare the FakeServer type in this module, and update any
functions or classes that implement NotificationServiceLike or call
registerServer (and other spots that previously used unknown for MCP server
protocol types on lines referenced) to accept/return FakeServer so the casts are
no longer required.
In `@libs/sdk/src/channel/channel-scope.helper.ts`:
- Around line 92-95: The helper currently bypasses TypeScript's discriminated
source/tool types by casting instance.metadata.source and agentEmitterSubscribe
to any when calling wireAgentCompletionSource (and the similar casts at the
other occurrences around lines 106-109 and 157-160); update the helper signature
to accept properly generic/concrete channel source and tool entry types and
propagate those generics into the call to wireAgentCompletionSource and
agentEmitterSubscribe so you can pass instance.metadata.source and the emitter
without any casts; remove the any casts and adjust the helper's type parameters
(or overloads) so instance: ChannelInstance<SourceType, ToolType> (or equivalent
existing types) is used, or default to unknown rather than any if a generic
default is needed, ensuring all three call sites use the concrete typed
parameters instead of any.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c4c19ec-22ba-45de-aca3-1466849b806f
📒 Files selected for processing (15)
apps/e2e/demo-e2e-channels/src/apps/channels/tools/session-tools.tool.tslibs/di/src/__tests__/token.factory.spec.tslibs/di/src/tokens/token.factory.tslibs/observability/package.jsonlibs/sdk/src/channel/channel-scope.helper.tslibs/sdk/src/channel/channel.instance.tslibs/sdk/src/common/interfaces/channel.interface.tslibs/sdk/src/common/schemas/annotated-class.schema.tslibs/sdk/src/scope/scope.instance.tsnx.jsonplugins/plugin-approval/package.jsonplugins/plugin-cache/package.jsonplugins/plugin-dashboard/package.jsonplugins/plugin-feature-flags/package.jsonplugins/plugin-remember/package.json
✅ Files skipped from review due to trivial changes (7)
- plugins/plugin-feature-flags/package.json
- libs/di/src/tokens/token.factory.ts
- plugins/plugin-cache/package.json
- plugins/plugin-approval/package.json
- plugins/plugin-remember/package.json
- libs/observability/package.json
- plugins/plugin-dashboard/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- libs/sdk/src/common/schemas/annotated-class.schema.ts
- libs/sdk/src/channel/channel.instance.ts
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
1 participant
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.
…or notifications
Summary by CodeRabbit