Skip to content

D8: scope connector relays by per-pod gates - #1550

Merged
lilyshen0722 merged 11 commits into
mainfrom
kai/d8-phase2-schema-gates
Sep 5, 2026
Merged

lilyshen0722 merged 11 commits into
mainfrom
kai/d8-phase2-schema-gates

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Sep 5, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • migrate installable connector projections to user scope with per-pod gates and membership-first outbound dispatch
  • restrict user-scoped connector PATCHes to the linked owner, with canonical/bounded gate keys and membership-validated writes
  • add admin pause/resume, typed paused refusals, inbound refusal, reconciliation, and the one-shot gate migration

Verification

  • npx jest --runInBand focused D8 suites: 131 passing
  • npm run tsc:check
  • npm run lint:ts (0 errors; pre-existing repository warnings)

Full backend suite remains blocked by the existing Node 26 / jsonwebtoken SlowBuffer compatibility failure outside this change.

Comment thread backend/routes/admin/installables.ts Fixed
Comment thread backend/routes/admin/installables.ts Fixed
Comment thread backend/routes/admin/installables.ts Fixed
Comment thread backend/routes/admin/installables.ts Fixed
Comment thread backend/routes/admin/installables.ts Fixed
Comment thread backend/routes/admin/installables.ts Fixed
@samxu01
samxu01 force-pushed the kai/d8-phase2-schema-gates branch from eb0770b to 6c99add Compare September 5, 2026 04:57
@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Gate read at 6c99add3 against main c6dde6bd.

The plan's §2 items are all genuinely implemented

Checked each against the code rather than the summary:

  • Gate keys — isObjectIdKey (/^[a-f\d]{24}$/i), MAX_GATES_PER_WRITE = 100, and every key validated and membership-checked before any update is constructed, so a mixed valid/invalid body writes nothing. The read side interpolates config.gates.${podId}.enabled only after Types.ObjectId.isValid(podId).
  • scope — stamped 'user' by webhookProjector's $setOnInsert, not only by the migration. This was the one that mattered: with default: 'pod' on the schema, a migration-only writer would have left every post-migration install on the permissive canDeleteIntegration branch.
  • Optional podId — guarded at both inbound bridges and at /summary, /podsummary, /status, so nothing stringifies undefined into a query.
  • Resume — $unset on both the child projection and the parent, and the Slack and Telegram webhook tests assert relay*MessageToPod is called once, after resume, rather than asserting the parent status changed. That was the specific way this could have passed its own test while being broken.
  • Fan-out — per-callback try/catch inside Promise.all, so one member's failed relay cannot abort the others.
  • paused — in the partial unique index, liveClaimStatuses, statusForClaim, resultForExisting, uninstall's return path, and sendInstallError.
  • /unmute cannot lift a pause — asserted directly (update.$unset['config.adminPause'] is undefined).

One finding: the gate now governs inbound, which D3 says it does not

isRelayableIntegration (both bridges) replaces the pod comparison with the gate for user-scoped rows:

(integration.scope === 'user'
  ? integration.config?.gates?.[String(podId)]?.enabled === true
  : String(integration.podId) === String(podId))

Inbound calls it as isRelayableIntegration(integration, String(integration.podId)), so a user-scoped row now needs an enabled gate on its own active pod to accept inbound. §2 D3 draws the line the other way: gates are what outbound reads, and podId "stays as the active pod (ADR-025 D12) for bare-message routing".

The divergence is reachable without anyone doing anything unusual:

  1. Owner is in pod A, podId = A, gates[A].enabled = true.
  2. Owner leaves pod A.
  3. sweepOrphanedGates unsets gates[A] within one sweep. podId is untouched.
  4. Every message the owner types in their own DM is now dropped — { relayed: false }, no reply to the chat, no log line on that branch — while the connector still reads connected and "linked to A" on the page.

There is no /pod command in routes/webhooks/telegram.ts today, so the owner has no verb to re-point podId; reinstalling is the only way back.

It fails closed, so this is not a leak and I am not blocking on security. It is REVIEW rule 19: it fails silently and nobody hears it. Two ways out, and the choice belongs to the design note rather than to a patch here:

  • Gate-off means both directions — then D3's sentence is wrong and should be corrected, and the inbound drop needs a line back to the chat saying the pod is no longer connected.
  • Inbound keeps reading podId — then the inbound call site needs the pod-scoped comparison, and the gate stays an outbound-only concept as written.

Non-blocking

sweepOrphanedGates issues one Pod.findById per gate per row on every 5-minute sweep, unbatched, and sweepActiveInstallations now attempts an updateMany per active installation per sweep. Fine at current volume; worth a batched lookup before this is a few thousand connectors.

@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Re-gate at f44a245d. Two things still open; everything else from the ruling landed.

What's correct now. isInboundRelayableIntegration compares podId and never gates, in both bridges. sweepOrphanedGates unsets podId alongside the orphaned gate when they are the same pod. Membership is checked at receive (isPodMember(pod, linkedUserId)), matching the way outbound checks it at send, and the Promise.all was correctly split so the membership decision happens before the user lookup.

1. Seed 6b's first half is not satisfied — both bridges

The new membership branch returns silently:

const pod = await PodModel.findById(podId).select('type createdBy members').lean();
if (!pod || !isPodMember(pod, linkedUserId)) {
  console.warn('[slack-bridge] inbound dropped — linked user is no longer a pod member');
  return { relayed: false };
}

replyNoActivePod is only reached from the earlier !integration.podId branch. But podId is unset by the sweep, which runs every five minutes — so in the window between the owner leaving pod A and the next sweep, podId is still A, gates[A] is still present, and this branch is the one that fires. Every message the owner types in that window disappears with nothing but a server log.

Seed 6b is explicit that this is the case to answer:

A bare message typed before the sweep → no pod post (membership fails at receive) and one reply in the chat naming the Connectors page

Today only the post-sweep half replies. Both branches need replyNoActivePod.

2. The copy is not the agreed copy

#1551 D3 gives the sentence literally:

This chat isn't linked to a pod any more. Pick one on the Connectors page.

Shipped in both bridges:

const NO_ACTIVE_POD_REPLY = 'This connector has no active pod. Choose one in Commonly first.';

That is the string from the /summary, /podsummary and /status handlers, not the one the note specifies. The note's version is also the more accurate of the two once §1 is fixed: in the membership branch podId is still set, so "has no active pod" is false there, while "isn't linked to a pod any more" is true in both branches. And "the Connectors page" names the destination; "in Commonly" does not.

One constant, both bridges, both branches.

Nothing else in the delta since 6c99add3 changes my earlier read.

Comment thread backend/routes/integrations.ts Fixed
@lilyshen0722

Copy link
Copy Markdown
Contributor Author

Final gate at 399e0028, against main c6dde6bd. Clear from me on the code; the press waits on Analyze (javascript-typescript), which is still pending.

The last change, described accurately

findByIdAndUpdate(integration._id, update) and findByIdAndUpdate(id, update) address the same document — id is ObjectId-validated a few lines above and is what loaded integration. So this silences the dataflow alert without changing behaviour. It is still the shape I want, because the authorised document is the authority rather than the parameter it was re-derived from, but it did not close an open hole and I would rather say so than let a green tick imply otherwise.

The two changes before it were substantive: the limiters sit at the route sinks ahead of auth, and the :id validation runs before the findById.

Closing a gap in my own read

I had reviewed every source file in this PR except backend/scripts/migrate-connector-gates.ts. Read now:

  • The update CAS carries installationId, podId and 'config.gates': { $exists: false }, so a second run modifies nothing even if a live writer creates a gate between the cursor read and the write. Seed 8 holds structurally, not just by observation.
  • Legacy rows are excluded by installationId: { $exists: true, $type: 'string' }, so they keep scope: 'pod' and today's authorisation.
  • --dry takes the continue before any write.

One ordering property worth stating because the whole of D3 rests on it: paused only enters the partial unique index once syncIndexes() runs, and seed-builtin-connectors.ts:101 calls it during the boot path that server.ts already invokes. The protection therefore arrives with the image rather than depending on an operator remembering to run the migration script.

Cumulative verdict

All seven items from the TASK-010 plan's §2 are implemented and tested at this head, the D3 ruling from #1551 is carried in both directions (inbound compares podId, the prune unsets podId with the gate, both bridges answer the chat on both drop branches), and PATCH { podId } is correctly confined to user-scoped rows, the linked owner, and a pod that owner is still in.

Nothing outstanding from this seat.

Comment thread backend/routes/integrations.ts Dismissed
lilyshen0722 added a commit that referenced this pull request Sep 5, 2026
…n switch, leaving it is heard, the page writes podId (#1551)

* docs(d8-phase-2): inbound never reads the gate — the active pod is its own switch, leaving it is heard in the chat, and the page writes podId

Vera's #1550 review (63797): isRelayableIntegration decided inbound by the
gate, so a pruned gate silently dropped every message the owner typed while
the page still read "linked to A". D3 now says inbound compares podId with
membership checked at receive, the prune unsets podId with the gate, and an
unrelayable message is answered in the chat. /pod was never built, so D4's
gate list gains the active tag and Make active; D2 gains the not-linked row;
seed 6b covers the walk.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* docs(d8-phase-2): the no-active-pod reply reuses the command string #1550 already sends (Vera)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* docs(d8-phase-2): the gate list's only rule is membership, and it is the server's — the picker's community/showcase filter matches no pod type (Vera)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

* docs(d8-phase-2): publicIntegration projects adminPause to { reason, at } — the moderated never learns who (Vera)

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
@samxu01
samxu01 force-pushed the kai/d8-phase2-schema-gates branch from 03489f5 to f5b582d Compare September 5, 2026 12:17
@lilyshen0722
lilyshen0722 merged commit 87e897c into main Sep 5, 2026
14 checks passed
@lilyshen0722
lilyshen0722 deleted the kai/d8-phase2-schema-gates branch September 5, 2026 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants