Repository navigation
Generic remote connectors (multi-session, routing, secrets) - #155
Merged
Merged
Conversation
- Add /connectors/:kind/:instanceId alongside /home/connectors/:id; stable DO keys via connectorSessionKey (home ids unchanged). - Pass X-Kody-Connector-Session-Key into the session DO; validate hello kind and id against ingress; optional connectorKind on hello from home-connector. - Extend McpCallerContext with remoteConnectors; normalize refs for registry, Home MCP bridge, meta, and search. - Merge multiple synthesized remote-tool domains; keep legacy home_* names for single home:default; use remote:<kind>:<id> domains otherwise. - Add REMOTE_CONNECTOR_SECRETS JSON map with HOME_CONNECTOR_SHARED_SECRET fallback for home; meta_list_remote_connector_status capability. - Search structured content gains remoteConnectorStatuses; homeConnectorStatus includes connectorKind when present. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
|
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:
📝 WalkthroughWalkthroughGeneralizes the home connector into a remote-connector model: adds normalization, routing, secret resolution, session keys, schemas, capability synthesis, status tooling, tests, and docs; updates routing/session handling and propagates connector kind+instanceId across worker/home/tooling codepaths. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Worker as Worker/Router
participant Session as HOME_CONNECTOR_SESSION<br/>DurableObject
participant Connector as Remote Connector
Client->>Worker: HTTP/WS request /connectors/:kind/:instanceId/...
Worker->>Worker: parseConnectorRoutePath(pathname)
Worker->>Worker: sessionKey = connectorSessionKey(kind, instanceId)
Worker->>Session: resolve by name sessionKey
Worker->>Session: forward request (add header X-Kody-Connector-Session-Key)
Session->>Connector: deliver request (upgrade / RPC)
Connector->>Connector: validate ingressSessionKey matches header
alt keys match
Connector->>Worker: respond with RPC/snapshot result
Worker-->>Client: proxied response
else mismatch
Connector->>Worker: send server.error and close (4003)
Worker-->>Client: error response
end
sequenceDiagram
participant App as MCP App
participant Normalizer as Remote Connector Ref Normalizer
participant Registry as Capability Registry
participant Synth as Remote Tool Domain Synthesizer
participant Client as Remote MCP Client
participant Connector as Remote Connector
App->>Normalizer: normalizeRemoteConnectorRefs(callerContext)
Normalizer-->>App: [{kind,instanceId}, ...]
App->>Registry: getCapabilityRegistryForContext(...)
Registry->>Synth: synthesizeRemoteToolDomain(env, ref, allRefs) (concurrent)
Synth->>Client: createRemoteConnectorMcpClient(env, ref.kind, ref.instanceId)
Client->>Connector: fetch snapshot (MCP)
Connector-->>Client: snapshot
Synth->>Registry: {domain, bindings}
Registry-->>App: combined registry with synthesized domains
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kentcdodds
marked this pull request as ready for review
April 11, 2026 23:40
Contributor
|
🔎 Preview deployed: https://kody-pr-155.kentcdodds.workers.dev Worker: Mocks:
|
- Add architecture/remote-connectors.md (URLs, hello, JSON-RPC, MCP context) - Link from home-connector, request-lifecycle, architecture index, env vars, adding-capabilities; refresh troubleshooting meta tool hints Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/mcp/tools/search.ts (1)
819-834:⚠️ Potential issue | 🟠 Major
remoteConnectorStatusesnever makes it into the final structured response.Line 819 rebuilds
SearchResultStructuredContent, but it only copieshomeConnectorStatusand omitstrimmedPayload.remoteConnectorStatuses. As written, callers will never see the new remote status payload even when Lines 712-776 populate it.Proposed fix
const result: SearchResultStructuredContent = { offline: trimmedPayload.offline, warnings: trimmedPayload.warnings, ...(trimmedPayload.memories ? { memories: trimmedPayload.memories, } : {}), ...(trimmedPayload.homeConnectorStatus ? { homeConnectorStatus: trimmedPayload.homeConnectorStatus } : {}), + ...(trimmedPayload.remoteConnectorStatuses + ? { + remoteConnectorStatuses: + trimmedPayload.remoteConnectorStatuses, + } + : {}), matches: toSlimStructuredMatches({ matches: trimmedPayload.matches, baseUrl, }), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/search.ts` around lines 819 - 834, The constructed SearchResultStructuredContent object omits trimmedPayload.remoteConnectorStatuses so callers never receive remoteConnectorStatuses; update the object construction in the block building SearchResultStructuredContent to include remoteConnectorStatuses when present (similar to the existing conditional for homeConnectorStatus), i.e. check trimmedPayload.remoteConnectorStatuses and add { remoteConnectorStatuses: trimmedPayload.remoteConnectorStatuses } into the spread so the final response contains the remoteConnectorStatuses alongside matches produced by toSlimStructuredMatches.
🧹 Nitpick comments (4)
packages/worker/src/mcp/context.node.test.ts (1)
32-43: Re-add explicit assertions for connector defaults in parse test.
toMatchObjectis reasonable, but this test now misses regressions where parsedhomeConnectorId/remoteConnectorsdefaults drift.✅ Minimal test-hardening diff
test('parseMcpCallerContext validates caller context shape', () => { - expect( - parseMcpCallerContext({ + const parsed = parseMcpCallerContext({ baseUrl: 'https://example.com', user: { userId: '123', email: 'user@example.com', displayName: 'user', }, storageContext: { sessionId: 'session-123', appId: 'app-123', }, - }), - ).toMatchObject({ + }) + expect(parsed).toMatchObject({ baseUrl: 'https://example.com', user: { userId: '123', email: 'user@example.com', displayName: 'user', }, storageContext: { sessionId: 'session-123', appId: 'app-123', }, }) + expect(parsed).toMatchObject({ + homeConnectorId: null, + remoteConnectors: null, + }) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/context.node.test.ts` around lines 32 - 43, The test uses toMatchObject and therefore doesn't assert connector default values; update the failing test in context.node.test.ts to re-add explicit assertions that the parsed context's homeConnectorId and remoteConnectors have the expected default values (e.g., expect(parsed.homeConnectorId).toBe(...) and expect(parsed.remoteConnectors).toEqual(...)) after calling the parser (the function under test, e.g., parse or parseContext), so regressions in defaults for homeConnectorId and remoteConnectors are caught.packages/worker/src/remote-connector/remote-connectors-shared.node.test.ts (1)
4-26: Add an explicitremoteConnectors: []regression case.The compatibility rule here hinges on treating
remoteConnectors: undefineddifferently from an explicitly empty array. Right now the suite only coversundefinedand a populated array, so an implementation that accidentally falls back tohomeConnectorIdfor[]would still pass.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/remote-connector/remote-connectors-shared.node.test.ts` around lines 4 - 26, Add a regression test for normalizeRemoteConnectorRefs to assert that when remoteConnectors is explicitly an empty array it does NOT fall back to homeConnectorId; call normalizeRemoteConnectorRefs with homeConnectorId set (e.g., 'living-room') and remoteConnectors: [] and expect an empty array result ([]), similarly to the existing tests for undefined and populated arrays so the function's handling of an explicit empty array is validated.packages/worker/src/mcp/capabilities/meta/meta-list-remote-connector-status.ts (1)
43-60: Fetch connector statuses concurrently.Each
getRemoteConnectorStatus()call is independent, so this loop makes latency grow linearly with connector count.Promise.all()keeps this troubleshooting capability responsive when several remotes are attached.♻️ Suggested change
async handler(_args, ctx) { const refs = normalizeRemoteConnectorRefs(ctx.callerContext) - const connectors = [] - for (const ref of refs) { - const s = await getRemoteConnectorStatus(ctx.env, ref) - connectors.push({ + const connectors = await Promise.all( + refs.map(async (ref) => { + const s = await getRemoteConnectorStatus(ctx.env, ref) + return { connector_kind: s.connectorKind, connector_instance_id: s.connectorId ?? ref.instanceId, status: s.state, connected: s.connected, connected_at: s.connectedAt, last_seen_at: s.lastSeenAt, tool_count: s.toolCount, message: s.message, error: s.error, - }) - } + } + }), + ) return { connectors } },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/meta/meta-list-remote-connector-status.ts` around lines 43 - 60, The handler currently awaits getRemoteConnectorStatus sequentially inside the for loop causing linear latency; change it to fetch all statuses concurrently using Promise.all over refs (from normalizeRemoteConnectorRefs(ctx.callerContext)) and then map results into the connectors array (preserve keys connector_kind, connector_instance_id, status, connected, connected_at, last_seen_at, tool_count, message, error). Ensure connector_instance_id falls back to ref.instanceId when s.connectorId is nullish, and keep the same return shape { connectors }.packages/worker/src/remote-connector/connector-session-key.node.test.ts (1)
17-31: Add coverage for/connectors/home/:instanceId/....The suite locks down custom generic routes and legacy home routes, but this PR also accepts the generic
homeform. A regression there would bypass the new compatibility path without failing these tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/remote-connector/connector-session-key.node.test.ts` around lines 17 - 31, Add a test asserting that parseConnectorRoutePath correctly handles the generic home form '/connectors/home/:instanceId/...': call parseConnectorRoutePath('/connectors/home/default/rpc/tools-list') (or similar) and expect an object with kind: 'home', instanceId: 'default', and rest: '/rpc/tools-list'; this mirrors the existing expectations for custom and legacy home routes and ensures the generic home route is covered in connector-session-key.node.test.ts so regressions are detected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/shared/src/chat.ts`:
- Around line 26-36: The remoteConnectorRefSchema currently allows
whitespace-only strings because minLength(1) checks raw length; update the
schema for remoteConnectorRefSchema so both fields (kind and instanceId) are
validated on their trimmed value: change the validators on kind and instanceId
in remoteConnectorRefSchema to first trim the input (or apply a
transform/refinement that calls .trim()) then enforce non-empty (e.g.,
trimmed.length >= 1 or a trimmed minLength/regex). Ensure the change is applied
to the string validators referenced as kind and instanceId so whitespace-only
values are rejected at validation time.
In `@packages/shared/src/remote-connectors.ts`:
- Around line 11-17: normalizeKind and normalizeInstanceId currently trim (and
lowercase for kind) but allow whitespace-only inputs to become empty strings;
change both functions to trim (and lower) then validate that the result is
non-empty and throw a clear Error (e.g., "invalid kind" / "invalid instanceId")
so callers fail fast instead of producing missing path segments. Apply the same
non-empty-after-trim validation to the other normalizer helpers in this file
(the functions in the 24-40 range) so all refs are rejected if they normalize to
blank.
In `@packages/worker/src/env-schema.ts`:
- Line 128: REMOTE_CONNECTOR_SECRETS is currently only validated as a non-empty
string but must be JSON that maps keys like "kind:instanceId" to credential
values; update the schema in env-schema.ts to parse and validate JSON at boot
instead of optionalNonEmptyStringSchema by replacing REMOTE_CONNECTOR_SECRETS
with a custom parser schema that: accepts an absent value, parses the string
into JSON, ensures the result is an object/dictionary, validates each key
matches the expected "^[^:]+:[^:]+$" pattern (or similar) and each value is a
non-empty string, and surfaces a descriptive validation error on parse/failure
so startup fails fast. Reference the REMOTE_CONNECTOR_SECRETS symbol when
implementing the new parse-and-validate schema.
In `@packages/worker/src/home/utils.ts`:
- Around line 45-49: The parsing currently treats a non-string connectorKind as
absent; instead, detect if 'connectorKind' exists on the input (use the presence
of the key via (value as Record<string, unknown>)['connectorKind'] or the
in-operator) and if it exists but typeof connectorKindRaw !== 'string' throw a
validation error (or return a protocol-level error) so malformed payloads are
rejected; otherwise continue to trim and lowercase connectorKind as before (keep
the connectorKindRaw -> connectorKind logic but only after confirming it's a
string).
In `@packages/worker/src/mcp/capabilities/registry.ts`:
- Around line 35-45: The loop calling synthesizeRemoteToolDomain over refs
currently lets a single rejected snapshot abort getCapabilityRegistryForContext
and drop healthy domains; change this to perform per-ref resolution (e.g., map
refs -> synthesizeRemoteToolDomain promises and use Promise.allSettled) so
failures for individual refs are caught and logged/ignored while successful
syntheses still push synthesized.domain into synthesizedDomains, and ensure the
fallback/return to staticRegistry still happens when synthesizedDomains remains
empty; look for normalizeRemoteConnectorRefs, synthesizeRemoteToolDomain, and
the surrounding getCapabilityRegistryForContext logic to implement the per-ref
error isolation.
In `@packages/worker/src/remote-connector/connector-session-key.ts`:
- Around line 20-39: parseConnectorRoutePath is using raw path segments to build
the session identity while connectorIngressPath percent-encodes kind and
instanceId, causing mismatched keys for values like "foo bar" or "team/a";
modify parseConnectorRoutePath to decodeURIComponent() the extracted kind and
instanceId (both in the '/connectors/:kind/:instanceId/...' branch and the
'/home/connectors/:instanceId/...' branch), validate that decoding succeeds and
that the decoded values are non-empty, and treat any URIError from malformed
encodings as invalid input (return null) so the session key generation matches
connectorIngressPath.
In `@packages/worker/src/remote-connector/remote-domain-id.ts`:
- Around line 3-12: The slugging logic in remoteConnectorDomainId and
remoteConnectorCapabilityPrefix collapses distinct instanceIds (e.g. "a__b" vs
"a_b") causing collisions; replace the collapsing sanitization with a reversible
or stable disambiguator: encode the raw instanceId using a safe,
filesystem/URL-friendly encoding (e.g. base64url or encodeURIComponent) or
append a short stable hash (e.g. first 8 chars of SHA-256) to the sanitized slug
so uniqueness is preserved, then use that encoded/hashed token wherever
remoteConnectorDomainId and remoteConnectorCapabilityPrefix compute ids so both
functions stay consistent and reversible.
In `@packages/worker/src/remote-connector/resolve-remote-connector-secret.ts`:
- Around line 13-29: The JSON.parse failure for REMOTE_CONNECTOR_SECRETS is
currently swallowed (catch { /* fall through to legacy */ }), causing silent
misconfiguration; update the catch to surface the error instead: in
resolve-remote-connector-secret, when parsing mapRaw fails, throw or log a clear
error that includes the original exception and the REMOTE_CONNECTOR_SECRETS
contents (and optionally the connector key `${k}:${id}`) so the caller can fail
fast rather than falling back to HOME_CONNECTOR_SHARED_SECRET; keep existing
behavior for valid parsed objects and the existing checks for fromMap but do not
silently continue on parse errors.
---
Outside diff comments:
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 819-834: The constructed SearchResultStructuredContent object
omits trimmedPayload.remoteConnectorStatuses so callers never receive
remoteConnectorStatuses; update the object construction in the block building
SearchResultStructuredContent to include remoteConnectorStatuses when present
(similar to the existing conditional for homeConnectorStatus), i.e. check
trimmedPayload.remoteConnectorStatuses and add { remoteConnectorStatuses:
trimmedPayload.remoteConnectorStatuses } into the spread so the final response
contains the remoteConnectorStatuses alongside matches produced by
toSlimStructuredMatches.
---
Nitpick comments:
In
`@packages/worker/src/mcp/capabilities/meta/meta-list-remote-connector-status.ts`:
- Around line 43-60: The handler currently awaits getRemoteConnectorStatus
sequentially inside the for loop causing linear latency; change it to fetch all
statuses concurrently using Promise.all over refs (from
normalizeRemoteConnectorRefs(ctx.callerContext)) and then map results into the
connectors array (preserve keys connector_kind, connector_instance_id, status,
connected, connected_at, last_seen_at, tool_count, message, error). Ensure
connector_instance_id falls back to ref.instanceId when s.connectorId is
nullish, and keep the same return shape { connectors }.
In `@packages/worker/src/mcp/context.node.test.ts`:
- Around line 32-43: The test uses toMatchObject and therefore doesn't assert
connector default values; update the failing test in context.node.test.ts to
re-add explicit assertions that the parsed context's homeConnectorId and
remoteConnectors have the expected default values (e.g.,
expect(parsed.homeConnectorId).toBe(...) and
expect(parsed.remoteConnectors).toEqual(...)) after calling the parser (the
function under test, e.g., parse or parseContext), so regressions in defaults
for homeConnectorId and remoteConnectors are caught.
In `@packages/worker/src/remote-connector/connector-session-key.node.test.ts`:
- Around line 17-31: Add a test asserting that parseConnectorRoutePath correctly
handles the generic home form '/connectors/home/:instanceId/...': call
parseConnectorRoutePath('/connectors/home/default/rpc/tools-list') (or similar)
and expect an object with kind: 'home', instanceId: 'default', and rest:
'/rpc/tools-list'; this mirrors the existing expectations for custom and legacy
home routes and ensures the generic home route is covered in
connector-session-key.node.test.ts so regressions are detected.
In `@packages/worker/src/remote-connector/remote-connectors-shared.node.test.ts`:
- Around line 4-26: Add a regression test for normalizeRemoteConnectorRefs to
assert that when remoteConnectors is explicitly an empty array it does NOT fall
back to homeConnectorId; call normalizeRemoteConnectorRefs with homeConnectorId
set (e.g., 'living-room') and remoteConnectors: [] and expect an empty array
result ([]), similarly to the existing tests for undefined and populated arrays
so the function's handling of an explicit empty array is validated.
🪄 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
Run ID: e850bfc1-9eca-4552-92a4-60b529c437b0
📒 Files selected for processing (31)
.github/workflows/preview.ymlpackages/home-connector/src/transport/worker-connector.tspackages/shared/src/chat.tspackages/shared/src/remote-connectors.tspackages/worker/client/mcp-apps/kody-ui-utils.tspackages/worker/src/env-schema.tspackages/worker/src/home/client-transport.tspackages/worker/src/home/client.tspackages/worker/src/home/mcp.tspackages/worker/src/home/session.tspackages/worker/src/home/status.tspackages/worker/src/home/types.tspackages/worker/src/home/utils.tspackages/worker/src/index.tspackages/worker/src/mcp-auth.workers.test.tspackages/worker/src/mcp/capabilities/domain-metadata.tspackages/worker/src/mcp/capabilities/home/index.tspackages/worker/src/mcp/capabilities/meta/domain.tspackages/worker/src/mcp/capabilities/meta/meta-get-home-connector-status.tspackages/worker/src/mcp/capabilities/meta/meta-list-remote-connector-status.tspackages/worker/src/mcp/capabilities/registry.tspackages/worker/src/mcp/context.node.test.tspackages/worker/src/mcp/context.tspackages/worker/src/mcp/tools/search-format.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/remote-connector/connector-session-key.node.test.tspackages/worker/src/remote-connector/connector-session-key.tspackages/worker/src/remote-connector/remote-connectors-shared.node.test.tspackages/worker/src/remote-connector/remote-domain-id.tspackages/worker/src/remote-connector/resolve-remote-connector-secret.node.test.tspackages/worker/src/remote-connector/resolve-remote-connector-secret.ts
Comment on lines
+3
to
+12
| export function remoteConnectorDomainId(ref: RemoteConnectorRef): string { | ||
| const k = ref.kind.trim().toLowerCase() | ||
| const id = | ||
| ref.instanceId | ||
| .trim() | ||
| .replaceAll(/[^\w-]+/g, '_') | ||
| .replaceAll(/_+/g, '_') | ||
| .replace(/^_|_$/g, '') || 'instance' | ||
| return `remote:${k}:${id}` | ||
| } |
There was a problem hiding this comment.
Avoid many-to-one slugging for connector identity.
remoteConnectorDomainId() and remoteConnectorCapabilityPrefix() both collapse distinct instance ids into the same synthesized names (a__b vs a_b, and a-b vs a_b). Once both connectors are attached, their domains/capabilities can collide even though they are different remotes. Use a reversible encoding or add a stable disambiguator.
Also applies to: 18-38
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/remote-connector/remote-domain-id.ts` around lines 3 -
12, The slugging logic in remoteConnectorDomainId and
remoteConnectorCapabilityPrefix collapses distinct instanceIds (e.g. "a__b" vs
"a_b") causing collisions; replace the collapsing sanitization with a reversible
or stable disambiguator: encode the raw instanceId using a safe,
filesystem/URL-friendly encoding (e.g. base64url or encodeURIComponent) or
append a short stable hash (e.g. first 8 chars of SHA-256) to the sanitized slug
so uniqueness is preserved, then use that encoded/hashed token wherever
remoteConnectorDomainId and remoteConnectorCapabilityPrefix compute ids so both
functions stay consistent and reversible.
Comment on lines
+13
to
+29
| const mapRaw = env.REMOTE_CONNECTOR_SECRETS?.trim() | ||
| if (mapRaw) { | ||
| try { | ||
| const parsed = JSON.parse(mapRaw) as unknown | ||
| if (parsed && typeof parsed === 'object' && !Array.isArray(parsed)) { | ||
| const key = `${k}:${id}` | ||
| const fromMap = (parsed as Record<string, unknown>)[key] | ||
| if (typeof fromMap === 'string' && fromMap.trim()) { | ||
| return fromMap.trim() | ||
| } | ||
| } | ||
| } catch { | ||
| // fall through to legacy | ||
| } | ||
| } | ||
| if (k === 'home') { | ||
| return env.HOME_CONNECTOR_SHARED_SECRET?.trim() |
There was a problem hiding this comment.
Don't silently ignore invalid REMOTE_CONNECTOR_SECRETS JSON.
A parse failure here is treated the same as “no map configured”. That takes every non-home connector down, and home can quietly fall back to HOME_CONNECTOR_SHARED_SECRET instead of the intended per-instance override. Please surface this as an explicit misconfiguration instead of silently continuing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/remote-connector/resolve-remote-connector-secret.ts`
around lines 13 - 29, The JSON.parse failure for REMOTE_CONNECTOR_SECRETS is
currently swallowed (catch { /* fall through to legacy */ }), causing silent
misconfiguration; update the catch to surface the error instead: in
resolve-remote-connector-secret, when parsing mapRaw fails, throw or log a clear
error that includes the original exception and the REMOTE_CONNECTOR_SECRETS
contents (and optionally the connector key `${k}:${id}`) so the caller can fail
fast rather than falling back to HOME_CONNECTOR_SHARED_SECRET; keep existing
behavior for valid parsed objects and the existing checks for fromMap but do not
silently continue on parse errors.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
docs/contributing/architecture/remote-connectors.md (1)
75-79: Clarify the bidirectional notification behavior.The current wording conflates two distinct behaviors:
- How a connector should handle incoming
notifications/tools/list_changedfrom the Worker (by re-listing tools)- How the reference implementation proactively sends
notifications/tools/list_changedto the Worker afterserver.ackThe word "responds" in "responds by sending" is confusing because it suggests responding to the incoming notification, but the sentence describes sending after
server.ackinstead.📝 Suggested rewording for clarity
-The connector should handle **`notifications/tools/list_changed`** from the -Worker by re-listing tools if it implements dynamic registration; the reference -implementation in `packages/home-connector` responds by sending a -`notifications/tools/list_changed` **to** the Worker after `server.ack` so the -session refreshes its tool snapshot. +The connector should handle **`notifications/tools/list_changed`** from the +Worker by re-listing tools if it implements dynamic registration. The reference +implementation in `packages/home-connector` also proactively sends a +`notifications/tools/list_changed` **to** the Worker immediately after receiving +`server.ack` to trigger an initial tool snapshot refresh.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/contributing/architecture/remote-connectors.md` around lines 75 - 79, Rewrite the paragraph to separate the two behaviors: state that a connector should handle incoming notifications/tools/list_changed from the Worker by re-listing tools if it supports dynamic registration, and then note independently that the reference implementation in packages/home-connector proactively sends notifications/tools/list_changed to the Worker after server.ack to refresh the session’s tool snapshot; remove the word “responds” and make both actions explicitly distinct (handling incoming vs proactively sending) and reference notifications/tools/list_changed and server.ack so readers can locate the related logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/contributing/architecture/remote-connectors.md`:
- Around line 75-79: Rewrite the paragraph to separate the two behaviors: state
that a connector should handle incoming notifications/tools/list_changed from
the Worker by re-listing tools if it supports dynamic registration, and then
note independently that the reference implementation in packages/home-connector
proactively sends notifications/tools/list_changed to the Worker after
server.ack to refresh the session’s tool snapshot; remove the word “responds”
and make both actions explicitly distinct (handling incoming vs proactively
sending) and reference notifications/tools/list_changed and server.ack so
readers can locate the related logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af883bcd-04d5-4f53-a903-baa248aeb1e1
📒 Files selected for processing (7)
docs/contributing/adding-capabilities.mddocs/contributing/architecture/home-connector.mddocs/contributing/architecture/index.mddocs/contributing/architecture/remote-connectors.mddocs/contributing/architecture/request-lifecycle.mddocs/contributing/environment-variables.mddocs/use/troubleshooting.md
✅ Files skipped from review due to trivial changes (5)
- docs/contributing/architecture/index.md
- docs/contributing/architecture/home-connector.md
- docs/contributing/adding-capabilities.md
- docs/use/troubleshooting.md
- docs/contributing/environment-variables.md
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/home/session.ts`:
- Around line 228-230: Canonicalize connectorKind and connectorId before any
auth, persistence, or ACKs: compute declaredKind by trimming
message.connectorKind (defaulting to 'home' if null/empty after trim) then
lowercasing it, and compute canonicalInstanceId by trimming message.connectorId;
use these (not raw message fields) when calling connectorSessionKey, when
persisting/echoing identities, and in the other blocks that reference
connectorKind/connectorId (the same logic applied around the later uses in this
file). Ensure whitespace-only connectorKind falls back to 'home' and replace raw
message.connectorId with canonicalInstanceId everywhere it's stored or echoed.
🪄 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
Run ID: d61c5b6a-a844-4a6b-ba90-abcabff5c801
📒 Files selected for processing (3)
packages/worker/src/home/session.tspackages/worker/src/remote-connector/connector-session-key.tspackages/worker/src/remote-connector/remote-domain-id.ts
✅ Files skipped from review due to trivial changes (1)
- packages/worker/src/remote-connector/connector-session-key.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/remote-connector/remote-domain-id.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
| return id | ||
| } | ||
| return `${k}:${id}` | ||
| } |
Contributor
There was a problem hiding this comment.
Session key collision between home-colon instanceIds and non-home kinds
Low Severity
connectorSessionKey('home', 'foo:bar') returns 'home:foo:bar', and connectorSessionKey('home:foo', 'bar') also returns 'home:foo:bar'. Two logically distinct connector references — one with kind='home' and a colon-containing instanceId, the other with a colon-containing kind — resolve to the same Durable Object session key, causing them to share state, WebSocket connections, and tool snapshots.
Reviewed by Cursor Bugbot for commit 141ee10. Configure here.
…low-ups - Stop writing CLOUDFLARE_ACCOUNT_ID into preview secret overrides; preview wrangler already injects it as a var, and bulk secret upload rejected duplicate binding names (Cloudflare API 10053). - Include remoteConnectorStatuses in search structuredContent after trimming. - Validate remoteConnectorRef kind/instanceId with trimmed non-empty schemas; filter blank refs after normalization. - Use Promise.allSettled for per-ref domain synthesis; parallelize meta list status; canonicalize connector id in session hello persistence and ack. - Reject non-string connectorKind in hello parse when the key is present. - Parse REMOTE_CONNECTOR_SECRETS at Worker env validation; runtime resolver accepts parsed object or JSON string with parse error logging. - Tests: generic /connectors/home/... route, empty remoteConnectors regression; doc clarifications for list_changed and env vars. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/worker/src/home/session.ts (1)
228-229:⚠️ Potential issue | 🟡 MinorWhitespace-only
connectorKinddoesn't fall back to'home'.If
message.connectorKindis" "(whitespace only), the current code producesdeclaredKind = ""instead of falling back to'home'. This could cause session key mismatches downstream.Suggested fix
- const declaredKind = (message.connectorKind ?? 'home').trim().toLowerCase() + const declaredKind = message.connectorKind?.trim().toLowerCase() || 'home'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/home/session.ts` around lines 228 - 229, declaredKind currently uses (message.connectorKind ?? 'home').trim().toLowerCase() which turns a whitespace-only connectorKind into an empty string instead of falling back; update the logic around declaredKind (and read message.connectorKind) to trim first, then treat an empty result as 'home' before lowercasing — e.g., compute a trimmedKind = (message.connectorKind ?? '').trim(), then set declaredKind = (trimmedKind === '' ? 'home' : trimmedKind).toLowerCase() so whitespace-only values correctly fall back to 'home'.
🧹 Nitpick comments (3)
packages/worker/src/mcp/capabilities/home/index.ts (1)
29-32: Edge case: capability name could start/end with underscore.The sanitization correctly replaces non-word characters, but if
toolNamestarts or ends with special characters, the result could beprefix__toolnameorprefix_toolname_. This is unlikely to cause issues but worth noting.Optional hardening
function createCapabilityNameFromPrefix(prefix: string, toolName: string) { - const safeTool = toolName.replaceAll(/[^\w]+/g, '_').replaceAll(/_+/g, '_') + const safeTool = toolName + .replaceAll(/[^\w]+/g, '_') + .replaceAll(/_+/g, '_') + .replace(/^_|_$/g, '') return `${prefix}_${safeTool}` }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/capabilities/home/index.ts` around lines 29 - 32, The sanitization in createCapabilityNameFromPrefix can produce leading/trailing underscores when toolName starts/ends with non-word characters; update createCapabilityNameFromPrefix to trim any leading/trailing underscores after the current replaceAll calls (and keep the existing collapse of multiple underscores) so the returned `${prefix}_${safeTool}` never contains accidental double or trailing underscores; ensure you still replace internal non-word runs with single underscores and then remove underscores at the start/end of safeTool before concatenating.packages/worker/src/mcp/tools/search.ts (1)
206-219: Consider parallelizing remote connector status checks.The status checks run sequentially via
for...of, which could add latency when multiple remote connectors are configured. UsingPromise.allwould fetch all statuses concurrently.Suggested refactor
export async function loadDownRemoteConnectorStatuses(input: { env: Env callerContext: Pick<McpCallerContext, 'homeConnectorId' | 'remoteConnectors'> }): Promise<Array<HomeConnectorStatus>> { const refs = normalizeRemoteConnectorRefs(input.callerContext) - const down: Array<HomeConnectorStatus> = [] - for (const ref of refs) { - const status = await getRemoteConnectorStatus(input.env, ref) - if (shouldIncludeRemoteConnectorStatus(status)) { - down.push(status) - } - } - return down + const statuses = await Promise.all( + refs.map((ref) => getRemoteConnectorStatus(input.env, ref)), + ) + return statuses.filter(shouldIncludeRemoteConnectorStatus) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/mcp/tools/search.ts` around lines 206 - 219, The function loadDownRemoteConnectorStatuses currently iterates refs sequentially calling getRemoteConnectorStatus which adds latency; instead map normalizeRemoteConnectorRefs(input.callerContext) to an array of getRemoteConnectorStatus promises, await Promise.all to fetch statuses in parallel, then filter the resolved statuses with shouldIncludeRemoteConnectorStatus and return the filtered array; ensure you still call normalizeRemoteConnectorRefs and preserve the same HomeConnectorStatus type and error propagation behavior.packages/worker/src/remote-connector/connector-session-key.ts (1)
33-42: Minor: redundant trim operations.Lines 34-35 trim
parts[n]before decoding, then lines 37-38 trim the decoded values again. The second trim is sufficient sincedecodeURIComponentdoesn't add whitespace. This is harmless but slightly redundant.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/remote-connector/connector-session-key.ts` around lines 33 - 42, The code in connector-session-key.ts redundantly trims the raw segments before calling decodeSegment and then trims the decoded values again; update the extraction to stop trimming before decodeSegment (i.e., call decodeSegment(parts[1]) and decodeSegment(parts[2]) or otherwise remove the earlier .trim()), keep the existing checks that ensure decodedKind/decodedInstanceId are truthy, then continue trimming only the decodedKind/decodedInstanceId into kind and instanceId and return { kind, instanceId, rest } as before to eliminate the duplicate trimming while preserving behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/worker/src/home/session.ts`:
- Around line 228-229: declaredKind currently uses (message.connectorKind ??
'home').trim().toLowerCase() which turns a whitespace-only connectorKind into an
empty string instead of falling back; update the logic around declaredKind (and
read message.connectorKind) to trim first, then treat an empty result as 'home'
before lowercasing — e.g., compute a trimmedKind = (message.connectorKind ??
'').trim(), then set declaredKind = (trimmedKind === '' ? 'home' :
trimmedKind).toLowerCase() so whitespace-only values correctly fall back to
'home'.
---
Nitpick comments:
In `@packages/worker/src/mcp/capabilities/home/index.ts`:
- Around line 29-32: The sanitization in createCapabilityNameFromPrefix can
produce leading/trailing underscores when toolName starts/ends with non-word
characters; update createCapabilityNameFromPrefix to trim any leading/trailing
underscores after the current replaceAll calls (and keep the existing collapse
of multiple underscores) so the returned `${prefix}_${safeTool}` never contains
accidental double or trailing underscores; ensure you still replace internal
non-word runs with single underscores and then remove underscores at the
start/end of safeTool before concatenating.
In `@packages/worker/src/mcp/tools/search.ts`:
- Around line 206-219: The function loadDownRemoteConnectorStatuses currently
iterates refs sequentially calling getRemoteConnectorStatus which adds latency;
instead map normalizeRemoteConnectorRefs(input.callerContext) to an array of
getRemoteConnectorStatus promises, await Promise.all to fetch statuses in
parallel, then filter the resolved statuses with
shouldIncludeRemoteConnectorStatus and return the filtered array; ensure you
still call normalizeRemoteConnectorRefs and preserve the same
HomeConnectorStatus type and error propagation behavior.
In `@packages/worker/src/remote-connector/connector-session-key.ts`:
- Around line 33-42: The code in connector-session-key.ts redundantly trims the
raw segments before calling decodeSegment and then trims the decoded values
again; update the extraction to stop trimming before decodeSegment (i.e., call
decodeSegment(parts[1]) and decodeSegment(parts[2]) or otherwise remove the
earlier .trim()), keep the existing checks that ensure
decodedKind/decodedInstanceId are truthy, then continue trimming only the
decodedKind/decodedInstanceId into kind and instanceId and return { kind,
instanceId, rest } as before to eliminate the duplicate trimming while
preserving behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04933794-b007-4ccd-8607-97636044fb13
📒 Files selected for processing (21)
.github/workflows/preview.ymldocs/contributing/adding-capabilities.mddocs/contributing/architecture/remote-connectors.mddocs/contributing/architecture/request-lifecycle.mddocs/contributing/environment-variables.mdpackages/shared/src/chat.tspackages/shared/src/remote-connectors.tspackages/worker/src/env-schema.tspackages/worker/src/home/client-transport.tspackages/worker/src/home/session.tspackages/worker/src/home/utils.tspackages/worker/src/mcp/capabilities/home/index.tspackages/worker/src/mcp/capabilities/meta/meta-list-remote-connector-status.tspackages/worker/src/mcp/capabilities/registry.tspackages/worker/src/mcp/context.node.test.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/remote-connector/connector-session-key.node.test.tspackages/worker/src/remote-connector/connector-session-key.tspackages/worker/src/remote-connector/remote-connectors-shared.node.test.tspackages/worker/src/remote-connector/resolve-remote-connector-secret.node.test.tspackages/worker/src/remote-connector/resolve-remote-connector-secret.ts
✅ Files skipped from review due to trivial changes (5)
- docs/contributing/adding-capabilities.md
- packages/worker/src/remote-connector/remote-connectors-shared.node.test.ts
- packages/worker/src/mcp/context.node.test.ts
- packages/worker/src/remote-connector/resolve-remote-connector-secret.node.test.ts
- packages/worker/src/remote-connector/connector-session-key.node.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- packages/shared/src/chat.ts
- .github/workflows/preview.yml
- packages/worker/src/mcp/capabilities/meta/meta-list-remote-connector-status.ts
- packages/worker/src/home/client-transport.ts
- packages/worker/src/env-schema.ts
- packages/worker/src/remote-connector/resolve-remote-connector-secret.ts
- docs/contributing/environment-variables.md
- packages/shared/src/remote-connectors.ts
- packages/worker/src/home/utils.ts
Treat whitespace-only connectorKind as home in session hello; parallelize search remote connector status fetches; trim synthesized capability name edges; decode route segments without redundant pre-trim. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/worker/src/home/session.ts (1)
227-308:⚠️ Potential issue | 🟠 MajorReject blank
connectorIdin the hello handshake.At Line 232, a whitespace-only
message.connectorIdbecomes'', but the code still derives a session key, resolves secrets, persists the session, and ACKs it. That leaves an authenticated session with no usable identity, andgetSnapshot()later hides it because!connectorId. Fail fast if the trimmed id is empty.Proposed fix
const canonicalInstanceId = message.connectorId.trim() + if (!canonicalInstanceId) { + ws.send( + stringifyHomeConnectorMessage({ + type: 'server.error', + message: 'Connector id is required.', + }), + ) + ws.close(4000, 'invalid-connector-id') + return + } const expectedSessionKey = connectorSessionKey( declaredKind, canonicalInstanceId, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/home/session.ts` around lines 227 - 308, The handleHello flow currently accepts a blank/whitespace connectorId (message.connectorId) after trimming (canonicalInstanceId), creating an anonymous session; add an early guard in handleHello to reject when canonicalInstanceId === '': log/Sentry capture a clear error (e.g., "Missing connectorId in hello"), send a server.error message to the websocket indicating the connectorId is required, and close the ws with an appropriate close code (e.g., 4002 or similar) without persisting state or proceeding to connectorSessionKey or resolveRemoteConnectorSharedSecret; ensure the check runs before computing expectedSessionKey/expectedSecret and before persisting/session ack.
♻️ Duplicate comments (1)
packages/worker/src/remote-connector/connector-session-key.ts (1)
24-55:⚠️ Potential issue | 🟠 MajorDon't collapse empty path segments when parsing connector routes.
At Line 24,
split('/').filter(Boolean)turns malformed paths into valid identities. For example,/connectors/custom//alpha/snapshotgets parsed askind=custom,instanceId=alphainstead of being rejected. That can route a bad URL to the wrong connector session and also mutatesrestby removing empty segments. Parse fixed indexes from the raw split array and reject emptykind/instanceIdsegments instead.Proposed fix
- const parts = pathname.split('/').filter(Boolean) + const parts = pathname.split('/') const decodeSegment = (value: string) => { try { return decodeURIComponent(value) } catch { return null } } // /connectors/:kind/:instanceId/... - if (parts.length >= 3 && parts[0] === 'connectors' && parts[1] && parts[2]) { - const decodedKind = decodeSegment(parts[1]!) - const decodedInstanceId = decodeSegment(parts[2]!) + if (parts[0] === '' && parts[1] === 'connectors' && parts[2] && parts[3]) { + const decodedKind = decodeSegment(parts[2]!) + const decodedInstanceId = decodeSegment(parts[3]!) if (!decodedKind || !decodedInstanceId) return null const kind = decodedKind.trim() const instanceId = decodedInstanceId.trim() if (!kind || !instanceId) return null - const rest = parts.length > 3 ? `/${parts.slice(3).join('/')}` : '' + const rest = parts.length > 4 ? `/${parts.slice(4).join('/')}` : '' return { kind, instanceId, rest } } // /home/connectors/:instanceId/... if ( - parts.length >= 3 && - parts[0] === 'home' && - parts[1] === 'connectors' && - parts[2] + parts[0] === '' && + parts[1] === 'home' && + parts[2] === 'connectors' && + parts[3] ) { - const decodedInstanceId = decodeSegment(parts[2]!) + const decodedInstanceId = decodeSegment(parts[3]!) if (!decodedInstanceId) return null const instanceId = decodedInstanceId.trim() if (!instanceId) return null - const rest = parts.length > 3 ? `/${parts.slice(3).join('/')}` : '' + const rest = parts.length > 4 ? `/${parts.slice(4).join('/')}` : '' return { kind: 'home', instanceId, rest } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/worker/src/remote-connector/connector-session-key.ts` around lines 24 - 55, The parsing currently collapses empty path segments because it uses pathname.split('/').filter(Boolean); change this to use the raw split array (const parts = pathname.split('/')) and then validate by indexing into parts (e.g., parts[0], parts[1], parts[2], etc.) without removing empty strings; in the connector branch that checks parts[0] === 'connectors' and the home branch that checks parts[0] === 'home' && parts[1] === 'connectors', explicitly reject when parts[1] or parts[2] are empty strings (after decode via decodeSegment) so malformed paths like /connectors/custom//alpha... are returned null, and construct rest by joining parts.slice(3).join('/') so empty segments in the tail are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/worker/src/mcp/capabilities/home/index.ts`:
- Around line 29-35: The sanitized capability name created by
createCapabilityNameFromPrefix can collapse different tool names into the same
key, so modify the code that registers/assigns capability names (the place that
currently writes the binding and may overwrite an existing key) to detect
collisions and fail-fast: after generating capabilityName with
createCapabilityNameFromPrefix, check the target registry/map/object for an
existing entry for that capabilityName; if one exists, either throw an error
that includes both original tool names and the conflicting capabilityName, or
disambiguate deterministically (e.g., append a short counter or hash suffix) and
ensure the chosen strategy is consistently applied wherever
createCapabilityNameFromPrefix is used (including the other call sites noted).
Ensure the error includes the conflicting tool names and the generated
capabilityName so the problem is visible in CI/runtime.
- Around line 86-90: The binding currently sets kind from snapshot.connectorKind
which can differ from the synthesized ref and cause routing to the wrong
MCP/secret; update the RemoteToolCapabilityBinding construction (the binding
object that includes capabilityName, instanceId, mcpToolName) to derive kind
from ref.kind (e.g., use (ref.kind).trim().toLowerCase()) instead of
snapshot.connectorKind, and keep snapshot.connectorKind only for display/search
metadata; also ensure any downstream routing/selection logic that reads
binding.kind (the code that picks the MCP client and status target) will now use
the ref-derived kind so runtime routing remains anchored to ref.kind.
---
Outside diff comments:
In `@packages/worker/src/home/session.ts`:
- Around line 227-308: The handleHello flow currently accepts a blank/whitespace
connectorId (message.connectorId) after trimming (canonicalInstanceId), creating
an anonymous session; add an early guard in handleHello to reject when
canonicalInstanceId === '': log/Sentry capture a clear error (e.g., "Missing
connectorId in hello"), send a server.error message to the websocket indicating
the connectorId is required, and close the ws with an appropriate close code
(e.g., 4002 or similar) without persisting state or proceeding to
connectorSessionKey or resolveRemoteConnectorSharedSecret; ensure the check runs
before computing expectedSessionKey/expectedSecret and before persisting/session
ack.
---
Duplicate comments:
In `@packages/worker/src/remote-connector/connector-session-key.ts`:
- Around line 24-55: The parsing currently collapses empty path segments because
it uses pathname.split('/').filter(Boolean); change this to use the raw split
array (const parts = pathname.split('/')) and then validate by indexing into
parts (e.g., parts[0], parts[1], parts[2], etc.) without removing empty strings;
in the connector branch that checks parts[0] === 'connectors' and the home
branch that checks parts[0] === 'home' && parts[1] === 'connectors', explicitly
reject when parts[1] or parts[2] are empty strings (after decode via
decodeSegment) so malformed paths like /connectors/custom//alpha... are returned
null, and construct rest by joining parts.slice(3).join('/') so empty segments
in the tail are preserved.
🪄 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
Run ID: bed0f879-76a3-45f0-b3ff-e7a51b4ab3d7
📒 Files selected for processing (4)
packages/worker/src/home/session.tspackages/worker/src/mcp/capabilities/home/index.tspackages/worker/src/mcp/tools/search.tspackages/worker/src/remote-connector/connector-session-key.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/worker/src/mcp/tools/search.ts
Comment on lines
+29
to
35
| function createCapabilityNameFromPrefix(prefix: string, toolName: string) { | ||
| const safeTool = toolName | ||
| .replaceAll(/[^\w]+/g, '_') | ||
| .replaceAll(/_+/g, '_') | ||
| .replace(/^_|_$/g, '') | ||
| return `${prefix}_${safeTool}` | ||
| } |
There was a problem hiding this comment.
Fail fast on sanitized capability-name collisions.
The sanitization here can collapse distinct MCP tool names into the same capability key (foo-bar, foo_bar, foo__bar, etc.). When that happens, Line 202 silently overwrites the earlier binding and one tool becomes unreachable. Please disambiguate or throw when a generated capabilityName already exists.
Suggested guard
for (const tool of snapshot.tools) {
const { capability, binding } = createCapabilityFromTool({
snapshot,
tool,
ref,
domainId: domainIdForCapabilities,
capabilityPrefix,
domainKeywordRoots,
})
+ if (bindings[binding.capabilityName]) {
+ throw new Error(
+ `Duplicate synthesized capability name "${binding.capabilityName}" for ${ref.kind}:${ref.instanceId}.`,
+ )
+ }
capabilities.push(capability)
bindings[binding.capabilityName] = binding
}Also applies to: 82-85, 193-202
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/home/index.ts` around lines 29 - 35, The
sanitized capability name created by createCapabilityNameFromPrefix can collapse
different tool names into the same key, so modify the code that
registers/assigns capability names (the place that currently writes the binding
and may overwrite an existing key) to detect collisions and fail-fast: after
generating capabilityName with createCapabilityNameFromPrefix, check the target
registry/map/object for an existing entry for that capabilityName; if one
exists, either throw an error that includes both original tool names and the
conflicting capabilityName, or disambiguate deterministically (e.g., append a
short counter or hash suffix) and ensure the chosen strategy is consistently
applied wherever createCapabilityNameFromPrefix is used (including the other
call sites noted). Ensure the error includes the conflicting tool names and the
generated capabilityName so the problem is visible in CI/runtime.
Comment on lines
+86
to
90
| const binding: RemoteToolCapabilityBinding = { | ||
| capabilityName, | ||
| connectorId: snapshot.connectorId, | ||
| kind: (snapshot.connectorKind ?? ref.kind).trim().toLowerCase(), | ||
| instanceId: ref.instanceId, | ||
| mcpToolName: tool.name, |
There was a problem hiding this comment.
Keep runtime routing anchored to ref.kind.
binding.kind is taken from snapshot.connectorKind, but Lines 119-123 and 128-131 later use that value to select the MCP client and status target. If a connector reports a different kind than the normalized ref it was synthesized from, this capability can call/check the wrong connector/secret. Please keep binding.kind tied to ref.kind and use snapshot metadata only for display/search text. packages/worker/src/home/status.ts:140-145 routes by the supplied kind, so this changes the actual target connector.
Suggested fix
const binding: RemoteToolCapabilityBinding = {
capabilityName,
- kind: (snapshot.connectorKind ?? ref.kind).trim().toLowerCase(),
+ kind: ref.kind.trim().toLowerCase(),
instanceId: ref.instanceId,
mcpToolName: tool.name,
}Also applies to: 119-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/worker/src/mcp/capabilities/home/index.ts` around lines 86 - 90, The
binding currently sets kind from snapshot.connectorKind which can differ from
the synthesized ref and cause routing to the wrong MCP/secret; update the
RemoteToolCapabilityBinding construction (the binding object that includes
capabilityName, instanceId, mcpToolName) to derive kind from ref.kind (e.g., use
(ref.kind).trim().toLowerCase()) instead of snapshot.connectorKind, and keep
snapshot.connectorKind only for display/search metadata; also ensure any
downstream routing/selection logic that reads binding.kind (the code that picks
the MCP client and status target) will now use the ref-derived kind so runtime
routing remains anchored to ref.kind.
Contributor
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
Reviewed by Cursor Bugbot for commit e017dd3. Configure here.
| k === 'home' && | ||
| allRefs.length === 1 && | ||
| allRefs[0]?.kind === 'home' && | ||
| allRefs[0]?.instanceId.trim() === 'default' |
Contributor
There was a problem hiding this comment.
Inconsistent normalization may cause domain/prefix mismatch
Low Severity
The isOnlyBuiltinHomeDomain check in synthesizeRemoteToolDomain uses allRefs[0]?.kind === 'home' without normalization, while the equivalent isOnlyBuiltinHome check in remoteConnectorCapabilityPrefix uses allRefs[0]?.kind.trim().toLowerCase() === 'home' with normalization. These two checks govern the same domain-id vs. capability-prefix decision for the same allRefs. If they ever disagree, capabilities would be registered with prefix home but domain remote:home:default (or vice versa), causing a silent mismatch. Currently safe because normalizeRemoteConnectorRefs always pre-normalizes, but the inconsistency is fragile.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit e017dd3. Configure here.
This was referenced Apr 12, 2026
This was referenced Jul 4, 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.
Summary
Generalizes the home connector into remote connectors: stable session keys,
/connectors/:kind/:instanceIdplus legacy/home/connectors/:id,McpCallerContext.remoteConnectors, per-connector secrets (REMOTE_CONNECTOR_SECRETS), merged synthesized tool domains, meta + search status hints, and architecture docs.Follow-ups in this branch
CLOUDFLARE_ACCOUNT_IDfrom secret overrides (Wrangler binding conflict / API 10053).remoteConnectorStatusesin search structured content; stricter ref/env validation; parallel domain synthesis and meta status; session hello canonicalization; tests for generic/connectors/home/...andremoteConnectors: [].connectorKindfalls back tohomein session hello;loadDownRemoteConnectorStatusesusesPromise.all; capability name sanitization trims leading/trailing underscores; route parsing decodes without redundant pre-trim.Test plan
npm run typechecknpm run test(worker)npm run lintnpm run formatSummary by CodeRabbit
New Features
Refactor
Documentation
Tests
Bug Fixes