Give connector identity one owner: the instance id - #126
Open
hristo2612 wants to merge 1 commit into
Open
Conversation
The registry is keyed by connector instance id, but every connector except
Discord stamped IncomingMessage.connector and its session-key prefix with the
type name. For a named instance ({id: "slack-support", type: "slack"}) the
lookup in deliverConnectorReply missed and returned silently, so every reply
from a runWebSession turn was dropped, and two instances of one type collided
onto a single session.
Connector gains `id`; every connector class sets it from config.id with its
type literal as the fallback, and stamps and key prefixes derive from it.
DiscordConnector's bespoke instanceId field is deleted (its name is now the
type constant), and its proxyToRemote path now derives the same key as the
inbound path instead of a default-prefixed one. /api/status keys health by
registry key so same-type instances stop overwriting each other, and the
deliverConnectorReply miss now logs the drop instead of hiding it.
For legacy top-level config id === type, so stamps, session keys and status
keys stay byte-identical; the unchanged threads-test assertions pin that.
Also deletes the parallel connectors[] array in server.ts, which only ever
mirrored connectorMap, and fixes the prose this falsifies.
hristo2612
marked this pull request as ready for review
August 3, 2026 17:10
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
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.
Selected area
Connector identity across the gateway: the connector registry, the five
IncomingMessage.connectorstamp sites, everyderiveSessionKeyprefix,/api/statushealth keying, and thedeliverConnectorReplylookup.Evidence of blended concerns
Connector.nameandsession.connectorcarried two different identities — the connector instance id and the connector type name — and onlyDiscordConnectorhad ever been moved to the id. The two agree only while an instance is named exactly after its type.gateway/server.tsdoesconnectorMap.set(instance.id, connector), and normalization injects the id into every constructor config ({ ...raw, id }). The shipped docs already state the rule: "Connector ids are what the rest of the gateway addresses".this.nameat three sites, Telegram at one, WhatsApp used a hardcoded"whatsapp"literal, and remote-Discord hardcoded"discord". Discord itself carriednameandinstanceIdas two fields holding the identical expression.deliverConnectorReplydidconst connector = connectors.get(session.connector); if (!connector) return;. For an instance configured{id: "slack-support", type: "slack"}the registry key isslack-supportbut the session recordedslack— the lookup missed and the function returned with no log, no event, no error. This is the only delivery path forrunWebSessionturns (four call sites: parent callbacks, cron follow-ups, fallback and retry results), so every reply to a named instance was dropped invisibly.proxyToRemote./api/statuskeyed health byconnector.name, so same-type instances overwrote each other, whileGET /api/connectorsalready read per instance.server.tskept a parallelconnectors: Connector[]array whose only uses were a push, a splice, and the shutdown loop — all served byconnectorMap.values().Load-bearing fact: legacy top-level config blocks get
id === type, pinned by a pre-existing test. For every unnamed install this change is a behavior-preserving rename — stamps, session keys and status keys stay byte-identical. Only namedinstances[]users change, and their replies are dropped today, so there is no working behavior to lose.Fixed constraint budget
netLineDelta*.test.tsfilesTouchednewFilesmaxFileLinesidfield on the existingConnectorinterfaceThe growth allowance above zero is test-only: the contract's rubric requires a regression test for the shut hole, and the Todo itself framed the win as ownership rather than raw line count.
Measured budget (reconciled)
netLineDeltaproductNetLineDeltafilesTouchednewFilesmaxFileLinesVerified contract defect:
filesTouched17 is unsatisfiableThe 17-file count was derived before the interface change was costed. Adding the required
id: stringto theConnectorinterface makes four further files mandatory, all one-line changes:sessions/manager.tsgateway/__tests__/session-attempt-race.test.tssessions/__tests__/platform-context-dispatch.test.tssessions/__tests__/context.test.tsIndependently reproduced by the verifier: reverting exactly those four files to base makes
pnpm typecheckfail with threeTS2741errors — each a stubConnectorobject now missing the new required field — and makescontext.test.tsfail on the old<name>placeholder that the contract separately mandates changing to<id>. No 17-file implementation exists that also satisfies the interface-change and prose criteria. The three per-file growth breaches (slack +2, telegram +2, whatsapp +4) come from the same mechanism and are fully offset —productNetLineDeltalands at exactly 0.What was deleted or clarified
Deleted
DiscordConnector.instanceId— redundant with the newid; both held the identical expression.connectors: Connector[]array inserver.ts, along with its push, splice and shutdown-loop consumers, all moved ontoconnectorMap."whatsapp"connector literal and the hardcoded type prefixes in the Slack, Telegram and WhatsApp session-key derivations.Clarified
Connectorgains exactly one field,id: string, documented as the instance id and the registry key, equal to the type for legacy top-level config. This is the single owner of connector identity.idfromconfig.idwith its type literal as the defensive fallback — the pattern Discord already used.deriveSessionKeyin the Slack and Telegram helpers gained a prefix parameter defaulting to the type literal, matching the shape the Discord helper already had, so existing assertions keep passing and pin the legacy byte-identity.discord/index.tsproxyToRemotenow passes the instance id, so the proxy path derives the same key as the inbound path instead of contradicting it./api/statuskeys connectors by the registry key rather thanconnector.name; output is identical for legacy configs.returnindeliverConnectorReplyis now alogger.warnnaming the session id and the unresolved connector value. This failure class can no longer be invisible./api/connectors/<name>/sendreferences, and two stale "names" code comments. No route or parameter renames.Tests
Five additions, zero deletions. The one rewrite is the former "missing from map" case, which pinned the silent drop as correct behavior; it now asserts the drop is logged and still does not throw.
run-web-session-connector-reply.test.ts— named-instance delivery (map keyedslack-support, session stampedslack-support→ reply delivered) plus the logged-drop assertion.connectors.test.ts— a constructed named instance carriesidfrom config and stamps it.slack/threads.test.tsandtelegram/__tests__/threads.test.ts— one custom-prefix case each; the existing literal-prefix assertions are untouched and now pin legacy byte-identity.Gate results, run from the worktree after the final commit:
pnpm typecheck— exit 0, 2/2 tasks successful.pnpm test— exit 0.jinn-cli: 314 files, 3891 passed, 7 skipped.@jinn/web: 123 files, 1294 passed.pnpm build— exit 0, compiled todist, web assets synced.Independent verification
Verdict ship at this head: all 13 acceptance criteria hold, zero blockers, zero majors, four minors. Privacy scan of the full diff came back clean — only
xapp-test/xoxb-test-style fixtures, no real identifiers, no co-author trailers.Minors, all non-blocking and recorded as follow-up candidates rather than fixed here, since each sits on the contract's out-of-scope list:
/doctor, the stop log, and thenamefield ofGET /api/connectors.connector: "discord"while forwarding an instance-id-prefixed session key; the proxy URLs are explicitly out of scope.connectors.test.tscase pins the id through the test helper rather than end-to-end, though normalization'sconfig.idinjection is already pinned by a pre-existing assertion.Not verified: live delivery through a real named Slack or Discord instance, the remote-proxy topology live, and web-UI rendering for named Discord instances — all need platform credentials.