Repository navigation
Conversation
Adds a `HYPERDRIVE` binding and read-only Drizzle queries for the data a directory-sync consumer reconciles: organizations, sub-organizations, users, memberships, removal tombstones, and group assignments. No migration. The WorkOS columns this service needs already exist — `organizations.directory_id`, `directory_group_id`, `directory_idp_id`, `external_org_id` and `kilocode_users.external_id` — so this is wiring only. Read-only on purpose: nothing writes, and the poll loop does not use these queries yet, so `/status` is their only caller. It gains a `database` block reporting reachability, the live organization count, and latency. `getPgDb` wraps `getWorkerDb` per call rather than caching a client, per the repo rule: a cached client outlives the context that created it and its I/O fails in whatever context picks it up next. Verified against a live database — stopping PostgreSQL turns the probe to `reachable: false` with the reason and no 500, and restarting it recovers with no Worker restart, because each call opens its own pool. Four schema facts the queries encode: - Every organization query filters `deleted_at IS NULL`. The unique indexes on `directory_group_id` and `external_org_id` are partial, scoped to live rows, so a soft-deleted organization can hold the same binding as a live one. The test seeds exactly that collision. - `organization_memberships` has no foreign keys on either column, so a row does not prove the organization or user exists. - `kilocode_users.external_id` is not unique, so the lookup returns an array instead of pretending a single row is guaranteed. - User IDs are `text`, not `uuid`. The test uses an `oauth/`-shaped ID so a stray UUID cast would fail. The bundle grows from 509 KiB to 2237 KiB (384 KiB gzipped, 75% of the 3 MiB limit) and startup from 8.9 ms to 55 ms. That is the shared schema plus `pg`, the same cost every database-backed Worker here pays: `ai-attribution` is 2152 KiB on the same dependency. `services/scim-sync/**` joins the `kilocode_backend` path filter — both copies, since `scripts/changes-filter-retry.test.mjs` pins them identical — so the new `test:postgres` suite actually runs on a PR that only touches this service.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change fixes the prior docblock finding by splitting Assumptions: Files Reviewed (3 files)
Previous Review Summaries (4 snapshots, latest commit 7f06c3c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 7f06c3c)Status: 1 Issue Found | Recommendation: Address before merge Executive SummaryThe incremental change only documents the WorkOS-directory-to-organization mapping and realigns the Postgres fixture with it; the one new finding is that the documentation presents Assumptions: Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Fix these issues in Kilo Cloud Previous review (commit 4f8eb7d)Status: No Issues Found | Recommendation: Merge Executive SummaryThe incremental change deletes the unused Assumptions: Files Reviewed (3 files)
Previous review (commit e832af0)Status: No Issues Found | Recommendation: Merge Executive SummaryBoth prior warnings are fixed at Assumptions: Files Reviewed (6 files)
Previous review (commit 9d1d175)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (15 files)
Reviewed by deepseek-flash-latest · Input: 46K · Output: 8.5K · Cached: 528.6K Review guidance: REVIEW.md from base branch |
…s users Review feedback, two defects: `getPgDb` set only `statement_timeout`, which PostgreSQL applies from the startup packet and so cannot fire before a connection exists. A database or Hyperdrive that refuses or blackholes new connections left the probe waiting on `pg.Pool`'s default, which has no client-side connect timeout — the exact outage `/status` exists to report. The earlier smoke test passed only because `docker stop` refuses connections immediately. Now sets `connectionTimeoutMillis` alongside it, as `ai-attribution` and `container-usage-meter` both do. `findUserByNormalizedEmail` took the first of possibly several rows. `IDX_kilocode_users_normalized_email` is a plain non-unique index, so that row was non-deterministic, and linking a directory user to the wrong Kilo account is the consequence. Renamed to `findUsersByNormalizedEmail` and returns every match, consistent with `findUsersByExternalId` and with the app-side resolver that reads two candidates specifically to detect ambiguity. `DatabaseStatus.error` is gone. A driver error names the failing SQL and its parameters; that belongs in Workers Logs, not in a response body that may one day be exposed. The probe now reports only whether the database answered and how slowly. Verified: the body carries no SQL while the log still does. Also documents three things the review asked about, at the code rather than in a thread: that `organization_groups` are Kilo capability policies and not WorkOS directory groups despite the name; that `external_id` is the only directory identifier on `kilocode_users`, the `directory_*` columns being on `organizations` alone; and that `/status` is unreachable in production because the Worker has no `workers_dev`, `routes`, or inbound service binding.
Not needed. `organization_groups` are Kilo capability policy sets, not WorkOS directory groups, and nothing populates them from directory sync — a WorkOS directory group binds to an organization through `organizations.directory_group_id`, not to a row in that table. Removes `src/db/groups.ts` and its fixtures and assertions. The README keeps one sentence about them, because deleting a membership still cascades a user's group assignments away through a composite foreign key: a membership write has authorization consequences that no directory event names, and a future reconciler needs to know that even though it never reads the table. `findOrganizationByDirectoryGroupId` stays. Despite the name it reads `organizations.directory_group_id`, which is the primary WorkOS binding for an organization and has nothing to do with `organization_groups`.
…tion The hierarchy is the mapping: | WorkOS | Kilo | Binding | |---|---|---| | directory | parent organization | `directory_id` | | directory group | sub-organization | `directory_group_id`, `directory_idp_id` | | directory user | user | `kilocode_users.external_id` | The binding columns live on every `organizations` row, parent or child, which is what makes a sub-organization the thing a directory group resolves to. I had assumed `organization_groups` was the analogue, which is why the previous commit removed those queries for a weaker reason than the real one: that table holds capability policy sets and has nothing to do with directory sync. So `findOrganizationByDirectoryGroupId` is not an awkwardly named survivor of that removal — it is the central lookup for a `dsync.group.*` event, and `findChildOrganizations` lists a directory's groups. The test fixtures move the group binding off the parent and onto the sub-organization, which is how real data looks, and a new case asserts the group lookup returns the child rather than its parent. The soft-deleted row now shares the binding with the live sub-organization, so the partial-unique-index regression still has something to catch. The non-unique `directory_idp_id` reason sharpens too: a disconnect/reconnect leaves two live sub-organizations briefly sharing an IdP group. No behavior change; comments, README, and fixtures only.
Review feedback, and a correct catch. My previous commit claimed the children of a parent are "also the set of directory groups bound to that directory". That only holds if every sub-organization carries a binding, and `createChildOrganization` (`apps/web/src/lib/organizations/organization-hierarchy.ts:102`) sets no directory columns at all — both the organization and admin routers call it. So a parent can hold a mix, and enumerating a directory's groups with `findChildOrganizations` would treat a hand-made sub-organization as a directory group. Adds `findDirectoryBoundChildOrganizations`, which filters on `directory_group_id IS NOT NULL`, and documents `findChildOrganizations` as every child rather than a group set. The README points at the filtered query. The fixtures now seed a hand-made sibling with no binding, and one test asserts each query returns the right set. That also exposed a teardown bug: the self-referential foreign key is ON DELETE RESTRICT, and the teardown deleted children from a hand-written list that the new fixture was not in, so the parent delete failed. It now derives children from `parent_organization_id`, so adding a fixture cannot silently break teardown. Verified by running the suite twice and confirming no rows remain.
Summary
An incremental step. The goal of this PR is narrow: connect
scim-syncto the shared PostgreSQL and prove it can read the organization and user data a directory-sync consumer reconciles. Everything beyond reading is out of scope.No migration. The WorkOS columns this service needs already exist in
packages/db/src/schema.ts—organizations.directory_id,directory_group_id,directory_idp_id,external_org_id(lines 3370-3374) andkilocode_users.external_id(line 550), whose own comment names this use case. So this is wiring, not schema work.Same instance, same data layer
HYPERDRIVEbinding id624ec80650dd414199349f4e217ddb10— the same Hyperdrive config and same PostgreSQL every other service in this repo uses. Workers cannot hold long-lived connections, so Hyperdrive owns the regional pool andgetPgDbopens amax: 1pool per call against the string it hands out, viagetWorkerDbfrom@kilocode/db/client. Drizzle, bound to the shared schema.No secret to provision: the Hyperdrive config stores the credentials and CA itself (
sslmode: verify-full), and hands the Worker a local connection string. None of the five existing Hyperdrive services define aPOSTGRES_URL.Read-only
The modules under
src/db/only select. Nothing writes, and the poll loop does not use them yet, so/statusis their only caller:organizations.tsdirectory_group_id, bydirectory_idp_id, byexternal_org_id, children by parent, live countusers.tsexternal_id, bynormalized_emailmemberships.tsEvery function takes
db: WorkerDbas its first argument, so they are exercised directly in tests without a Worker, and the Durable Object can call them later with a per-call client.Scope is organizations, users, and the membership that joins them. Kilo's
organization_groupsare deliberately not read: they are capability policy sets (read,edit,browser,command,mcp), not WorkOS directory groups, and nothing populates them from directory sync. Note thatfindOrganizationByDirectoryGroupIdreadsorganizations.directory_group_id— the primary WorkOS binding for an organization — and is unrelated to that table despite the name.Client lifecycle
getPgDbis called per operation and never cached in module scope, per the repo rule inworkers-best-practices. A cached client outlives the request or Durable Object context that created it, and its I/O then fails in whatever context picks it up next.That is not theoretical here — it is why the probe self-heals after an outage. Verified below.
Four schema facts the queries encode
Each is easy to get wrong and none is visible from a column name:
deleted_at IS NULL. The unique indexes ondirectory_group_idandexternal_org_idare partial, scoped to live rows, so a soft-deleted organization can hold the same binding as a live one. Omit the filter and those lookups stop being deterministic. The test seeds exactly that collision.organization_membershipshas no foreign keys on eitherorganization_idorkilo_user_id, so a row does not prove the organization or user exists, and neither side is cleaned up when the other goes away.external_idnornormalized_emailis unique onkilocode_users, so both lookups return arrays rather than pretending one row is guaranteed. Picking the first row would link a directory user to a non-deterministic Kilo account.text, notuuid— OAuth IDs look likeoauth/…. No UUID validation, parsing, or casts.Verification
Postgres suite — 12/12 against a real database
pnpm --filter cloudflare-scim-sync test:postgres, run against local Docker PostgreSQL with migrations applied, and in CI against apgvector/pgvector:pg16container. Fixtures are UUID-suffixed and torn down inafterAll.The two that matter most:
directory_group_id,directory_idp_id, andexternal_org_idas the live parent — which the partial unique indexes permit. The test asserts every lookup returns the live row.Plus an
oauth/-shaped user ID throughout, so a stray::uuidcast would fail rather than pass silently.Live Worker against live PostgreSQL
wrangler devwith Hyperdrive'slocalConnectionString:GET /statusreachable: true,latencyMs: 23organizationCount0 → 1docker stopPostgreSQL, re-probereachable: false, HTTP 200 — reported, not a 500, and no SQL in the bodydocker start, re-probereachable: trueagain, with no Worker restartThat last row is the client-lifecycle rule paying off concretely: because
getPgDbis called per operation, recovery needs nothing. A module-scope client would have stayed broken.Other gates
tsgo --noEmit, oxlint, oxfmt, markdown-table-padding all clean.pnpm-lock.yamladditive only — 6 insertions, 0 deletions.scripts/changes-filter-retry.test.mjs7/7, confirming the twoci.ymlfilter copies stayed byte-identical.Visual Changes
N/A
Reviewer Notes
pg, the cost every database-backed Worker here pays —ai-attributionis 2152 KiB on the same dependency,container-usage-meter1730 KiB. Worth knowing the headroom is now finite.test-scriptsis gated on thekilocode_backendpath filter, which did not listservices/scim-sync/**— so the new suite would have silently skipped on a PR touching only this service. Added to both filter copies, sincechanges-filter-retry.test.mjspins them identical./statusis not reachable in production.workers_dev: false, noroutes, and no inbound service binding, so nothing can issue an HTTP request to this Worker once deployed; it answers underwrangler dev. Anything exposing it later must add authentication first — it reports internal state, and reaching it drives a billable Durable Object request and a database query.test:postgresfails locally on a missing column, the local database is behind the schema.POSTGRES_URL=… NODE_ENV=development pnpm drizzle migratefixes it; that is what CI's step does before running the suite. I hit this during development.2026-04-29 01:16:12.945+00, whichz.string().datetime()rejects. Nothing here crosses a strict boundary yet; the README records it for whoever adds one.countOrganizationscounts in SQL. My first draft selected every id and read.length, which would have materialized the whole table for a probe.