feat(db): consolidate host/client identity on machineId#3784
Merged
saddlepaddle merged 5 commits intomainfrom Apr 27, 2026
Merged
feat(db): consolidate host/client identity on machineId#3784saddlepaddle merged 5 commits intomainfrom
saddlepaddle merged 5 commits intomainfrom
Conversation
Drop UUID surrogates on v2_hosts/v2_clients/v2_users_hosts in favor of composite primary keys keyed on the existing machineId. Same value the host service already uses to identify itself (getHostId, formerly getHashedDeviceId), now canonical end-to-end across DB, cloud API, relay, and renderer. Drops the dead session_hosts table. Renames the device-info shared lib + cloud trpc device router to host (v1 device.heartbeat / device.registerDevice retained against the untouched device_presence table). Bumps host-service version to 0.3.0 so the desktop coordinator force-respawns older binaries that still call device.ensureV2Host.
Contributor
|
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:
📝 WalkthroughWalkthroughConsolidates host identity around organization-scoped machine IDs: replaces UUID host/client keys with composite (organizationId, machineId), removes Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Greptile SummaryThis PR consolidates the v2 host identity model by replacing UUID surrogate PKs on Confidence Score: 5/5Safe to merge; the one-way migration has been validated on a prod fork with zero orphans, and all code changes are internally consistent. All findings are P2. The null-out query scope concern in the migration is theoretical — it would only trigger on pre-existing cross-org data integrity anomalies that the application logic already prevents, and the PR author explicitly validated against a prod fork confirming zero FK orphans. Every call-site rename (device→host, id→machineId) is complete and consistent across relay, cloud, host-service, and desktop. packages/db/drizzle/0037_consolidate_host_client_machine_id.sql — the null-out subqueries at lines 47–48 are theoretically non-org-scoped; low practical risk but worth hardening if the migration is ever re-used as a template.
|
| Filename | Overview |
|---|---|
| packages/db/drizzle/0037_consolidate_host_client_machine_id.sql | Hand-crafted migration drops session_hosts, translates UUID FKs to composite (org, machineId) keys via intermediate text cast; null-out queries lack org-scope and rely on an implicit cross-org invariant, though validated on a prod fork with zero orphans. |
| packages/trpc/src/router/host/host.ts | New cloud-side host router migrated from device.*; adds org-scoped inArray checks in checkAccess and setOnline — strictly more correct than the old UUID-keyed code. |
| packages/db/src/schema/schema.ts | Drops UUID surrogate PKs from v2_hosts, v2_clients, v2_users_hosts; replaces inline FK references with composite foreignKey() builders; removes sessionHosts table; changes automations/automation_runs host FK columns to text with composite FKs. |
| packages/trpc/src/router/automation/dispatch.ts | All host.id references replaced with host.machineId; resolveTargetHost now uses composite (organizationId, machineId) WHERE clauses and JOIN conditions — logic is correct. |
| packages/trpc/src/router/device/device.ts | v2 host procedures removed (ensureV2Host, ensureV2Client, checkHostAccess, setHostOnline); only v1 heartbeat/registerDevice remain — clean retirement boundary. |
| packages/shared/src/host-info.ts | Rename of device-info.ts; salt preserved verbatim so existing host IDs stay stable; getHostId(), getMachineId(), getHostName() semantics unchanged from prior functions. |
| packages/host-service/src/tunnel/connect.ts | Switches from device.ensureV2Host to host.ensure and logs host.machineId instead of host.id — straightforward and consistent. |
| apps/relay/src/tunnel.ts | Updates setHostOnline calls to host.setOnline — relay hostId is already a machineId text so the API surface change is transparent. |
| apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts | Removes sessionHosts collection; updates getKey callbacks for v2Hosts (machineId), v2Clients (userId:machineId), and v2UsersHosts (userId:hostId) to reflect composite PKs — all unique within org scope. |
Sequence Diagram
sequenceDiagram
participant Desktop as Desktop (coordinator)
participant HostSvc as Host Service
participant Cloud as Cloud API (tRPC)
participant DB as Postgres (v2_hosts)
participant Relay as Relay
Desktop->>HostSvc: spawn with HOST_CLIENT_ID, HOST_NAME env vars
HostSvc->>Cloud: host.ensure({ organizationId, machineId, name })
Cloud->>DB: UPSERT v2_hosts ON (org_id, machine_id)
DB-->>Cloud: { machineId, ... }
Cloud-->>HostSvc: host (machineId is PK)
HostSvc->>Relay: WebSocket connect with hostId=machineId + JWT
Relay->>Cloud: host.setOnline({ hostId: machineId, isOnline: true })
Cloud->>DB: UPDATE v2_hosts SET is_online=true WHERE (org_id, machine_id) IN memberships
Note over Relay,Cloud: Renderer connects to relay
Relay->>Cloud: host.checkAccess({ hostId: machineId })
Cloud->>DB: SELECT v2_users_hosts WHERE (user_id, org_ids, host_id)
DB-->>Cloud: { allowed: true }
Cloud-->>Relay: allowed
Note over HostSvc,Relay: Disconnect
Relay->>Cloud: host.setOnline({ hostId: machineId, isOnline: false })
Cloud->>DB: UPDATE v2_hosts SET is_online=false
Prompt To Fix All With AI
This is a comment left during a code review.
Path: packages/db/drizzle/0037_consolidate_host_client_machine_id.sql
Line: 47-48
Comment:
**Null-out queries lack `organization_id` scope**
After the UUID→machineId translation in step 5, rows whose original UUID no longer exists in `v2_hosts` retain their stringified-UUID value. The null-out queries check only whether the value appears in the global `machine_id` set, not whether the `(organization_id, machine_id)` composite pair exists:
```sql
UPDATE "automation_runs" SET "host_id" = NULL
WHERE "host_id" IS NOT NULL
AND "host_id" NOT IN (SELECT "machine_id" FROM "v2_hosts");
```
If — due to a pre-existing data-integrity anomaly — an `automation_run` row ended up pointing (via UUID) to a `v2_hosts` row in a *different* organization, the translation step would write that org's `machine_id` into `host_id`. The null-out would then skip it (because the machineId exists in *some* org), and the subsequent `ADD CONSTRAINT … FOREIGN KEY (organization_id, host_id)` would fail with an FK violation.
A more defensive form avoids this:
```sql
UPDATE "automation_runs" SET "host_id" = NULL
WHERE "host_id" IS NOT NULL
AND NOT EXISTS (
SELECT 1 FROM "v2_hosts"
WHERE "machine_id" = "automation_runs"."host_id"
AND "organization_id" = "automation_runs"."organization_id"
);
```
Same pattern applies to the `automations` null-out on `target_host_id`. In practice this is safe given the prod validation confirms zero FK orphans, but the current form relies on an implicit cross-org invariant that the migration itself doesn't enforce.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(db): consolidate host/client identi..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+47
to
+48
| ALTER TABLE "v2_hosts" DROP COLUMN "id";--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" DROP COLUMN "id";--> statement-breakpoint |
There was a problem hiding this comment.
organization_id scope
After the UUID→machineId translation in step 5, rows whose original UUID no longer exists in v2_hosts retain their stringified-UUID value. The null-out queries check only whether the value appears in the global machine_id set, not whether the (organization_id, machine_id) composite pair exists:
UPDATE "automation_runs" SET "host_id" = NULL
WHERE "host_id" IS NOT NULL
AND "host_id" NOT IN (SELECT "machine_id" FROM "v2_hosts");If — due to a pre-existing data-integrity anomaly — an automation_run row ended up pointing (via UUID) to a v2_hosts row in a different organization, the translation step would write that org's machine_id into host_id. The null-out would then skip it (because the machineId exists in some org), and the subsequent ADD CONSTRAINT … FOREIGN KEY (organization_id, host_id) would fail with an FK violation.
A more defensive form avoids this:
UPDATE "automation_runs" SET "host_id" = NULL
WHERE "host_id" IS NOT NULL
AND NOT EXISTS (
SELECT 1 FROM "v2_hosts"
WHERE "machine_id" = "automation_runs"."host_id"
AND "organization_id" = "automation_runs"."organization_id"
);Same pattern applies to the automations null-out on target_host_id. In practice this is safe given the prod validation confirms zero FK orphans, but the current form relies on an implicit cross-org invariant that the migration itself doesn't enforce.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/db/drizzle/0037_consolidate_host_client_machine_id.sql
Line: 47-48
Comment:
**Null-out queries lack `organization_id` scope**
After the UUID→machineId translation in step 5, rows whose original UUID no longer exists in `v2_hosts` retain their stringified-UUID value. The null-out queries check only whether the value appears in the global `machine_id` set, not whether the `(organization_id, machine_id)` composite pair exists:
```sql
UPDATE "automation_runs" SET "host_id" = NULL
WHERE "host_id" IS NOT NULL
AND "host_id" NOT IN (SELECT "machine_id" FROM "v2_hosts");
```
If — due to a pre-existing data-integrity anomaly — an `automation_run` row ended up pointing (via UUID) to a `v2_hosts` row in a *different* organization, the translation step would write that org's `machine_id` into `host_id`. The null-out would then skip it (because the machineId exists in *some* org), and the subsequent `ADD CONSTRAINT … FOREIGN KEY (organization_id, host_id)` would fail with an FK violation.
A more defensive form avoids this:
```sql
UPDATE "automation_runs" SET "host_id" = NULL
WHERE "host_id" IS NOT NULL
AND NOT EXISTS (
SELECT 1 FROM "v2_hosts"
WHERE "machine_id" = "automation_runs"."host_id"
AND "organization_id" = "automation_runs"."organization_id"
);
```
Same pattern applies to the `automations` null-out on `target_host_id`. In practice this is safe given the prod validation confirms zero FK orphans, but the current form relies on an implicit cross-org invariant that the migration itself doesn't enforce.
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts (1)
9-17:⚠️ Potential issue | 🟡 MinorUpdate stale docstring:
ensureV2Host→host.ensure.The function-level comment still references
ensureV2Host, but the implementation now callsctx.api.host.ensure.mutate(line 69).📝 Proposed doc update
* - Writes `branch.<name>.base` from `composer.baseBranch` for the Changes tab. - * - `ensureV2Host` + `v2Workspace.create` with rollback on failure. + * - `host.ensure` + `v2Workspace.create` with rollback on failure. * - Inserts the local `workspaces` row.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts` around lines 9 - 17, The function docstring is stale: replace the reference to ensureV2Host with the current host.ensure call and reflect that implementation uses ctx.api.host.ensure.mutate; update the bullet that currently reads "ensureV2Host + v2Workspace.create with rollback on failure" to mention "host.ensure (via ctx.api.host.ensure.mutate) + v2Workspace.create with rollback on failure" and adjust any surrounding wording to match the actions performed (inserting local workspaces row, optional setup terminal spawn, clearing progress).
🧹 Nitpick comments (9)
packages/db/drizzle/0037_consolidate_host_client_machine_id.sql (2)
30-42: Translation UPDATEs don't constrain byorganization_id— verified safe but worth a guard for defense-in-depth.Lines 33–36 match
host_id = h.id::textwithout also requiring<table>.organization_id = h.organization_id. The oldv2_hosts.idwas a global UUID PK, so each match is 1:1, but if any pre-existing row had a cross-org reference (organization_id mismatch with the host it pointed to) the translation would silently rewrite it to the other org'smachine_idand the new composite FK would then reject it as an orphan.PR objectives state the prod-fork validation showed 0 FK orphans post-migration, which confirms there are no such cross-org rows in practice — so this is fine as written. For future readers/safety, an
AND <table>.organization_id = h.organization_idclause would make the intent explicit and turn any latent cross-org row into a leftover that gets nulled (for nullable FKs) or surfaced by the FK ADD (for NOT NULL FKs after the orphan deletes).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/drizzle/0037_consolidate_host_client_machine_id.sql` around lines 30 - 42, The UPDATEs that translate stringified v2_hosts.id to machine_id (the UPDATE statements updating "automation_runs"."host_id", "automations"."target_host_id", "v2_users_hosts"."host_id", and "v2_workspaces"."host_id") should include an explicit organization guard to avoid cross-org matches; add AND "<table>".organization_id = h.organization_id to each corresponding FROM ... WHERE clause so the join only matches hosts within the same organization, and apply the same organization_id check to the subsequent NULL-ing UPDATEs (the WHERE ... NOT IN (SELECT "machine_id" FROM "v2_hosts")) as appropriate to ensure orphan detection remains organization-scoped.
1-3:DISABLE ROW LEVEL SECURITYbeforeDROP TABLE CASCADEis a no-op.
DROP TABLEremoves the table along with its RLS policies and settings, so disabling RLS first is unnecessary. Harmless, just noise.♻️ Optional cleanup
-- session_hosts is dead (no writes anywhere); drop with CASCADE. -ALTER TABLE "session_hosts" DISABLE ROW LEVEL SECURITY;--> statement-breakpoint DROP TABLE "session_hosts" CASCADE;--> statement-breakpoint🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/drizzle/0037_consolidate_host_client_machine_id.sql` around lines 1 - 3, The ALTER TABLE "session_hosts" DISABLE ROW LEVEL SECURITY; line is redundant because DROP TABLE "session_hosts" CASCADE; will remove the table and any RLS settings/policies; remove the ALTER TABLE ... DISABLE ROW LEVEL SECURITY; statement and keep only DROP TABLE "session_hosts" CASCADE; so the migration is cleaner and avoids no-op noise.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.ts (1)
87-127: Minor: redundanthostId/hostMachineIdfields and an unreachablecloudbranch.After this change,
hostIdandhostMachineIdin the select are populated from the same column (hosts.machineId), so they always have identical values. Combined with theinnerJoinonhosts,row.hostMachineIdcannot benullhere, so thehostType === "cloud"branch (Line 137) is unreachable in this hook. If "cloud" workspaces should be surfaced, the inner join needs to become a left join; otherwise consider dropping thecloudbranch and one of the duplicated fields to reduce confusion.Also applies to: 135-140
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.ts` around lines 87 - 127, The select projects both hostId and hostMachineId from hosts.machineId and the innerJoin on collections.v2Hosts makes hosts.machineId non-nullable, which renders the hostType === "cloud" branch unreachable; in useAccessibleV2Workspaces update either (A) change the innerJoin that joins { hosts: collections.v2Hosts } to a leftJoin so cloud workspaces (nullable hosts.machineId) can be returned and keep both fields if needed, or (B) if cloud workspaces are not required, remove the duplicated field (drop either hostId or hostMachineId from the select) and remove the hostType === "cloud" branch to avoid dead code—adjust downstream consumers of the selected row shape accordingly (select block and any logic referencing hostId/hostMachineId and the cloud branch).packages/trpc/src/router/v2-workspace/v2-workspace.ts (1)
108-108: TightenhostIdvalidation to match the 32-character hex format produced bygetHostId().
getHostId()returns a deterministic 32-character lowercase hex string (SHA256 HMAC sliced to 32 chars). Replacing.uuid()with.min(1)accepts arbitrary strings that fail only at the DB lookup stage. A stricter schema (length 32, lowercase hex) would reject malformed input earlier with a clearer 400 error and reduce unnecessary DB round-trips.♻️ Proposed refactor
- hostId: z.string().min(1), + hostId: z + .string() + .length(32) + .regex(/^[0-9a-f]{32}$/, "Invalid host id"),- hostId: z.string().min(1).optional(), + hostId: z + .string() + .length(32) + .regex(/^[0-9a-f]{32}$/, "Invalid host id") + .optional(),Extract a shared
hostIdSchemato keep this consistent across v2-workspace.ts and host.ts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts` at line 108, Tighten the hostId schema by replacing the loose z.string().min(1) with a shared hostIdSchema that enforces exactly 32 lowercase hex characters (e.g., regex /^[0-9a-f]{32}$/ and length 32) and use that schema in the v2-workspace validation where hostId appears; extract and export/import hostIdSchema so the same schema is used in host.ts (and any other places currently using .uuid() or .min(1)) to ensure inputs match the deterministic getHostId() format and fail fast with a 400.apps/desktop/src/lib/trpc/routers/auth/index.ts (1)
25-28: Optional: renamegetDeviceInfo/deviceId/deviceNamefor consistency.Now that the underlying source is
host-info(getHostId/getHostName) and the broader PR renamesDEVICE_*env vars anddevice.*cloud procedures tohost.*, this Electron IPC procedure is the last remainingdevice-named surface for v2 host identity. Renaming togetHostInfo/hostId/hostNamewould keep the public surface uniform; deferring is fine if you'd rather keep the renderer auth-client diff small.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/lib/trpc/routers/auth/index.ts` around lines 25 - 28, Rename the procedure and its returned fields to match the host-* naming: change the publicProcedure named getDeviceInfo to getHostInfo and update the returned keys deviceId/deviceName to hostId/hostName while still calling getHostId() and getHostName(); ensure any references to getDeviceInfo or the deviceId/deviceName keys in the renderer/auth-client are updated accordingly to avoid breaking callers.packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts (1)
65-71: Type narrowing: preferAwaited<ReturnType<...>>for consistency.Hand-writing
{ machineId: string }decouples this declaration from the actualhost.ensureresponse shape; if the procedure later adds fields they'll be invisible here, and ifmachineIdis renamed or removed this site won't fail to typecheck against the source of truth. Line 81 already uses theAwaited<ReturnType<…>>pattern forv2Workspace.create.mutate.♻️ Proposed refactor
- let host: { machineId: string }; + let host: Awaited<ReturnType<typeof ctx.api.host.ensure.mutate>>; try { host = await ctx.api.host.ensure.mutate({ organizationId: ctx.organizationId, machineId, name: hostName, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts` around lines 65 - 71, The local declaration for host is manually typed as `{ machineId: string }`; change it to use the actual return type of the API call to keep types in sync by declaring host as Awaited<ReturnType<typeof ctx.api.host.ensure.mutate>> (or the equivalent type alias) and keep the existing await call to ctx.api.host.ensure.mutate({ organizationId: ctx.organizationId, machineId, name: hostName }) unchanged so future shape changes are reflected here automatically.packages/trpc/src/router/automation/schema.ts (1)
28-42: Consider validating the machineId shape rather thanmin(1).
z.string().min(1).nullish()is correct now thattargetHostIdis a machineId, but it accepts any non-empty string. Per PR notes, all stored host IDs are 32-char hex (fromgetHashedDeviceId()); a regex (e.g.z.string().regex(/^[a-f0-9]{32}$/)) would catch malformed input at the edge instead of bouncing it viaverifyHostAccess→NOT_FOUND. Worth doing in coordination with the matchingv2Workspace.hostIdschema if you want consistent validation across the API surface. Safe to defer otherwise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/trpc/src/router/automation/schema.ts` around lines 28 - 42, The targetHostId field in both the automation schema and updateAutomationSchema currently uses z.string().min(1).nullish(), which allows any non-empty string; change validation to require the 32-char hex machineId shape used by getHashedDeviceId by replacing the current validator with a regex-based string validator (e.g. /^[a-f0-9]{32}$/) while preserving nullish/optional behavior; update the targetHostId declarations in the schemas (and consider mirroring the same change for the corresponding v2Workspace.hostId schema for consistency).packages/host-service/src/terminal/env-strip.ts (1)
16-36:HOST_CLIENT_ID/HOST_NAMEare now redundant with theHOST_prefix.Since
STRIP_PREFIXESalready contains"HOST_"(line 35), every env var beginning withHOST_is stripped by the prefix check at line 52. The two explicit entries (lines 20-21) are no-ops compared to before, whenDEVICE_*keys did not match any prefix. This is harmless — keeping them documents desktop's runtime additions — but you could either drop them or, conversely, drop the broadHOST_prefix if the intent is precise denylisting (analogous toDESKTOP_*being exact keys per the new comment).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/host-service/src/terminal/env-strip.ts` around lines 16 - 36, HOST_CLIENT_ID and HOST_NAME in HOST_SERVICE_RUNTIME_KEYS are redundant because STRIP_PREFIXES already contains "HOST_" which will strip any HOST_* env vars; either remove the two explicit entries (HOST_CLIENT_ID, HOST_NAME) from HOST_SERVICE_RUNTIME_KEYS to avoid duplication, or remove "HOST_" from STRIP_PREFIXES if you intend to only denylist exact keys; update the HOST_SERVICE_RUNTIME_KEYS set (or STRIP_PREFIXES array) accordingly and run tests to ensure behavior matches the intended precise denylist policy.apps/desktop/src/main/lib/host-service-coordinator.ts (1)
81-81: Optional: reusethis.machineIdinstead of re-invokinggetHostId().
this.machineIdis initialized at coordinator construction viagetHostId()(Line 81) and is the source of truth used everywhere else (e.g.,Connection.machineIdreturned fromstart/spawn/tryAdopt). Reusing it forHOST_CLIENT_IDkeeps the spawned child guaranteed-consistent with what the renderer sees, even ifgetHostId()is ever changed to be non-deterministic across calls.♻️ Suggested tweak
ELECTRON_RUN_AS_NODE: "1", ORGANIZATION_ID: organizationId, - HOST_CLIENT_ID: getHostId(), + HOST_CLIENT_ID: this.machineId, HOST_NAME: getHostName(),Also applies to: 467-468
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/main/lib/host-service-coordinator.ts` at line 81, Replace direct calls to getHostId() with the coordinator instance's machineId to ensure a single source of truth: use this.machineId when constructing HOST_CLIENT_ID and wherever getHostId() is invoked in methods like start, spawn, and tryAdopt (and the occurrences around the previous lines 467-468) so the spawned child sees the exact same Connection.machineId the renderer uses; update those assignments/usages to reference this.machineId instead of re-invoking getHostId().
🤖 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/db/drizzle/0037_consolidate_host_client_machine_id.sql`:
- Around line 1-59: The migration 0037_consolidate_host_client_machine_id.sql
was hand-edited to perform a stringified-UUID → machine_id translation (e.g.,
the UPDATEs that set "host_id" = h."machine_id") rather than being generated by
bunx drizzle-kit; add a clear header comment at the top of this SQL (including
migration id 0037, author/date, and one-line rationale that this is a deliberate
one-off because drizzle-kit cannot express the intermediate string→machine
translation) so future contributors won’t regenerate or modify it by accident,
and ensure the PR description references this header and the journal/snapshot
synchronization already noted.
In `@packages/trpc/src/router/host/host.ts`:
- Line 1: The router imports and uses the non-WebSocket client `db` for
checkAccess and setOnline while other mutations use `dbWs`; update the import to
rely on `dbWs` and replace all usages of `db` inside the functions checkAccess
and setOnline (both occurrences around the database calls in those functions) to
use `dbWs` so that these operations consistently use the WebSocket client like
ensure and ensureClient.
- Around line 53-67: The membership insert in host.ensure currently grants
"owner" via dbWs.insert(v2UsersHosts) regardless of whether the host was newly
created; to fix, detect whether the v2Hosts upsert actually created the host
(e.g., use the v2Hosts upsert/insert call's returning/rowCount or perform an
existence check before/after) and only run the v2UsersHosts insert when that
operation indicates creation; perform both steps inside a transaction to avoid
races and keep the existing .onConflictDoNothing on v2UsersHosts to preserve
idempotency.
---
Outside diff comments:
In
`@packages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.ts`:
- Around line 9-17: The function docstring is stale: replace the reference to
ensureV2Host with the current host.ensure call and reflect that implementation
uses ctx.api.host.ensure.mutate; update the bullet that currently reads
"ensureV2Host + v2Workspace.create with rollback on failure" to mention
"host.ensure (via ctx.api.host.ensure.mutate) + v2Workspace.create with rollback
on failure" and adjust any surrounding wording to match the actions performed
(inserting local workspaces row, optional setup terminal spawn, clearing
progress).
---
Nitpick comments:
In `@apps/desktop/src/lib/trpc/routers/auth/index.ts`:
- Around line 25-28: Rename the procedure and its returned fields to match the
host-* naming: change the publicProcedure named getDeviceInfo to getHostInfo and
update the returned keys deviceId/deviceName to hostId/hostName while still
calling getHostId() and getHostName(); ensure any references to getDeviceInfo or
the deviceId/deviceName keys in the renderer/auth-client are updated accordingly
to avoid breaking callers.
In `@apps/desktop/src/main/lib/host-service-coordinator.ts`:
- Line 81: Replace direct calls to getHostId() with the coordinator instance's
machineId to ensure a single source of truth: use this.machineId when
constructing HOST_CLIENT_ID and wherever getHostId() is invoked in methods like
start, spawn, and tryAdopt (and the occurrences around the previous lines
467-468) so the spawned child sees the exact same Connection.machineId the
renderer uses; update those assignments/usages to reference this.machineId
instead of re-invoking getHostId().
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.ts`:
- Around line 87-127: The select projects both hostId and hostMachineId from
hosts.machineId and the innerJoin on collections.v2Hosts makes hosts.machineId
non-nullable, which renders the hostType === "cloud" branch unreachable; in
useAccessibleV2Workspaces update either (A) change the innerJoin that joins {
hosts: collections.v2Hosts } to a leftJoin so cloud workspaces (nullable
hosts.machineId) can be returned and keep both fields if needed, or (B) if cloud
workspaces are not required, remove the duplicated field (drop either hostId or
hostMachineId from the select) and remove the hostType === "cloud" branch to
avoid dead code—adjust downstream consumers of the selected row shape
accordingly (select block and any logic referencing hostId/hostMachineId and the
cloud branch).
In `@packages/db/drizzle/0037_consolidate_host_client_machine_id.sql`:
- Around line 30-42: The UPDATEs that translate stringified v2_hosts.id to
machine_id (the UPDATE statements updating "automation_runs"."host_id",
"automations"."target_host_id", "v2_users_hosts"."host_id", and
"v2_workspaces"."host_id") should include an explicit organization guard to
avoid cross-org matches; add AND "<table>".organization_id = h.organization_id
to each corresponding FROM ... WHERE clause so the join only matches hosts
within the same organization, and apply the same organization_id check to the
subsequent NULL-ing UPDATEs (the WHERE ... NOT IN (SELECT "machine_id" FROM
"v2_hosts")) as appropriate to ensure orphan detection remains
organization-scoped.
- Around line 1-3: The ALTER TABLE "session_hosts" DISABLE ROW LEVEL SECURITY;
line is redundant because DROP TABLE "session_hosts" CASCADE; will remove the
table and any RLS settings/policies; remove the ALTER TABLE ... DISABLE ROW
LEVEL SECURITY; statement and keep only DROP TABLE "session_hosts" CASCADE; so
the migration is cleaner and avoids no-op noise.
In `@packages/host-service/src/terminal/env-strip.ts`:
- Around line 16-36: HOST_CLIENT_ID and HOST_NAME in HOST_SERVICE_RUNTIME_KEYS
are redundant because STRIP_PREFIXES already contains "HOST_" which will strip
any HOST_* env vars; either remove the two explicit entries (HOST_CLIENT_ID,
HOST_NAME) from HOST_SERVICE_RUNTIME_KEYS to avoid duplication, or remove
"HOST_" from STRIP_PREFIXES if you intend to only denylist exact keys; update
the HOST_SERVICE_RUNTIME_KEYS set (or STRIP_PREFIXES array) accordingly and run
tests to ensure behavior matches the intended precise denylist policy.
In
`@packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts`:
- Around line 65-71: The local declaration for host is manually typed as `{
machineId: string }`; change it to use the actual return type of the API call to
keep types in sync by declaring host as Awaited<ReturnType<typeof
ctx.api.host.ensure.mutate>> (or the equivalent type alias) and keep the
existing await call to ctx.api.host.ensure.mutate({ organizationId:
ctx.organizationId, machineId, name: hostName }) unchanged so future shape
changes are reflected here automatically.
In `@packages/trpc/src/router/automation/schema.ts`:
- Around line 28-42: The targetHostId field in both the automation schema and
updateAutomationSchema currently uses z.string().min(1).nullish(), which allows
any non-empty string; change validation to require the 32-char hex machineId
shape used by getHashedDeviceId by replacing the current validator with a
regex-based string validator (e.g. /^[a-f0-9]{32}$/) while preserving
nullish/optional behavior; update the targetHostId declarations in the schemas
(and consider mirroring the same change for the corresponding v2Workspace.hostId
schema for consistency).
In `@packages/trpc/src/router/v2-workspace/v2-workspace.ts`:
- Line 108: Tighten the hostId schema by replacing the loose z.string().min(1)
with a shared hostIdSchema that enforces exactly 32 lowercase hex characters
(e.g., regex /^[0-9a-f]{32}$/ and length 32) and use that schema in the
v2-workspace validation where hostId appears; extract and export/import
hostIdSchema so the same schema is used in host.ts (and any other places
currently using .uuid() or .min(1)) to ensure inputs match the deterministic
getHostId() format and fail fast with a 400.
🪄 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: def579c9-bb3a-4136-a0f3-93e8e2b56ac7
📒 Files selected for processing (44)
apps/desktop/src/lib/trpc/routers/auth/index.tsapps/desktop/src/lib/trpc/routers/auth/utils/crypto-storage.tsapps/desktop/src/main/lib/host-service-coordinator.tsapps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/useWorkspaceHostUrl.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/V2WorkspaceOpenInButton/V2WorkspaceOpenInButton.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/hooks/useOpenInExternalEditor/useOpenInExternalEditor.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/hooks/useBranchPickerController/useBranchPickerController.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/components/DevicePicker/hooks/useWorkspaceHostOptions/useWorkspaceHostOptions.tsapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsxapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/electric-proxy/src/where.tsapps/relay/src/access.tsapps/relay/src/tunnel.tspackages/db/drizzle/0037_consolidate_host_client_machine_id.sqlpackages/db/drizzle/meta/0037_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/relations.tspackages/db/src/schema/schema.tspackages/host-service/src/terminal/env-strip.tspackages/host-service/src/terminal/env.test.tspackages/host-service/src/trpc/router/host/host.tspackages/host-service/src/trpc/router/workspace-creation/procedures/adopt.tspackages/host-service/src/trpc/router/workspace-creation/procedures/create.tspackages/host-service/src/trpc/router/workspace-creation/shared/finish-checkout.tspackages/host-service/src/trpc/router/workspace/workspace.tspackages/host-service/src/tunnel/connect.tspackages/shared/package.jsonpackages/shared/src/host-info.tspackages/trpc/src/root.tspackages/trpc/src/router/automation/automation.tspackages/trpc/src/router/automation/dispatch.tspackages/trpc/src/router/automation/schema.tspackages/trpc/src/router/device/device.tspackages/trpc/src/router/host/host.tspackages/trpc/src/router/host/index.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
💤 Files with no reviewable changes (1)
- apps/electric-proxy/src/where.ts
Comment on lines
+1
to
+59
| -- session_hosts is dead (no writes anywhere); drop with CASCADE. | ||
| ALTER TABLE "session_hosts" DISABLE ROW LEVEL SECURITY;--> statement-breakpoint | ||
| DROP TABLE "session_hosts" CASCADE;--> statement-breakpoint | ||
|
|
||
| -- Delete orphan FK rows in NOT NULL columns BEFORE the type change. | ||
| -- Once host_id is text, we can't easily distinguish orphans from valid uuids, | ||
| -- and the post-translation rows would still have stringified-UUID values that | ||
| -- can't satisfy the new composite FK to v2_hosts(organization_id, machine_id). | ||
| DELETE FROM "v2_users_hosts" WHERE "host_id" NOT IN (SELECT "id" FROM "v2_hosts");--> statement-breakpoint | ||
| DELETE FROM "v2_workspaces" WHERE "host_id" NOT IN (SELECT "id" FROM "v2_hosts");--> statement-breakpoint | ||
|
|
||
| -- Drop old unique constraints (replaced by composite PKs below). | ||
| ALTER TABLE "v2_clients" DROP CONSTRAINT "v2_clients_org_user_machine_unique";--> statement-breakpoint | ||
| ALTER TABLE "v2_hosts" DROP CONSTRAINT "v2_hosts_org_machine_id_unique";--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" DROP CONSTRAINT "v2_users_hosts_org_user_host_unique";--> statement-breakpoint | ||
|
|
||
| -- Drop old FK constraints (column type change requires no incoming FK). | ||
| ALTER TABLE "automation_runs" DROP CONSTRAINT "automation_runs_host_id_v2_hosts_id_fk";--> statement-breakpoint | ||
| ALTER TABLE "automations" DROP CONSTRAINT "automations_target_host_id_v2_hosts_id_fk";--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" DROP CONSTRAINT "v2_users_hosts_host_id_v2_hosts_id_fk";--> statement-breakpoint | ||
| ALTER TABLE "v2_workspaces" DROP CONSTRAINT "v2_workspaces_host_id_v2_hosts_id_fk";--> statement-breakpoint | ||
|
|
||
| -- Cast FK columns uuid -> text. USING does not support subqueries, so we | ||
| -- stringify the uuid here and translate to machine_id in the UPDATEs below. | ||
| ALTER TABLE "automation_runs" ALTER COLUMN "host_id" SET DATA TYPE text USING "host_id"::text;--> statement-breakpoint | ||
| ALTER TABLE "automations" ALTER COLUMN "target_host_id" SET DATA TYPE text USING "target_host_id"::text;--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" ALTER COLUMN "host_id" SET DATA TYPE text USING "host_id"::text;--> statement-breakpoint | ||
| ALTER TABLE "v2_workspaces" ALTER COLUMN "host_id" SET DATA TYPE text USING "host_id"::text;--> statement-breakpoint | ||
|
|
||
| -- Translate stringified UUIDs to machine_ids via UPDATE FROM v2_hosts. | ||
| -- v2_hosts.id still exists at this point (dropped further down); cast to text | ||
| -- to compare against the now-text host_id columns. | ||
| UPDATE "automation_runs" SET "host_id" = h."machine_id" FROM "v2_hosts" h WHERE "automation_runs"."host_id" = h."id"::text;--> statement-breakpoint | ||
| UPDATE "automations" SET "target_host_id" = h."machine_id" FROM "v2_hosts" h WHERE "automations"."target_host_id" = h."id"::text;--> statement-breakpoint | ||
| UPDATE "v2_users_hosts" SET "host_id" = h."machine_id" FROM "v2_hosts" h WHERE "v2_users_hosts"."host_id" = h."id"::text;--> statement-breakpoint | ||
| UPDATE "v2_workspaces" SET "host_id" = h."machine_id" FROM "v2_hosts" h WHERE "v2_workspaces"."host_id" = h."id"::text;--> statement-breakpoint | ||
|
|
||
| -- For nullable FK columns, NULL out any orphans whose stringified UUIDs did not | ||
| -- translate. Necessary so the new composite FK validates. NOT NULL columns | ||
| -- already had orphans deleted above. | ||
| UPDATE "automation_runs" SET "host_id" = NULL WHERE "host_id" IS NOT NULL AND "host_id" NOT IN (SELECT "machine_id" FROM "v2_hosts");--> statement-breakpoint | ||
| UPDATE "automations" SET "target_host_id" = NULL WHERE "target_host_id" IS NOT NULL AND "target_host_id" NOT IN (SELECT "machine_id" FROM "v2_hosts");--> statement-breakpoint | ||
|
|
||
| -- Drop old uuid `id` columns. This implicitly drops the old PRIMARY KEY | ||
| -- constraints, freeing the tables to receive composite PKs below. | ||
| ALTER TABLE "v2_clients" DROP COLUMN "id";--> statement-breakpoint | ||
| ALTER TABLE "v2_hosts" DROP COLUMN "id";--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" DROP COLUMN "id";--> statement-breakpoint | ||
|
|
||
| -- Add new composite PRIMARY KEYs. | ||
| ALTER TABLE "v2_clients" ADD CONSTRAINT "v2_clients_organization_id_user_id_machine_id_pk" PRIMARY KEY("organization_id","user_id","machine_id");--> statement-breakpoint | ||
| ALTER TABLE "v2_hosts" ADD CONSTRAINT "v2_hosts_organization_id_machine_id_pk" PRIMARY KEY("organization_id","machine_id");--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" ADD CONSTRAINT "v2_users_hosts_organization_id_user_id_host_id_pk" PRIMARY KEY("organization_id","user_id","host_id");--> statement-breakpoint | ||
|
|
||
| -- Add new composite FOREIGN KEYs (now that v2_hosts has its composite PK). | ||
| ALTER TABLE "automation_runs" ADD CONSTRAINT "automation_runs_host_fk" FOREIGN KEY ("organization_id","host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE set null ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "automations" ADD CONSTRAINT "automations_target_host_fk" FOREIGN KEY ("organization_id","target_host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE set null ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" ADD CONSTRAINT "v2_users_hosts_host_fk" FOREIGN KEY ("organization_id","host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "v2_workspaces" ADD CONSTRAINT "v2_workspaces_host_fk" FOREIGN KEY ("organization_id","host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE no action ON UPDATE no action; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm 0037 is registered in the journal and has a corresponding snapshot.
fd -t f '_journal.json' packages/db/drizzle | head -5
fd -t f '0037' packages/db/drizzleRepository: superset-sh/superset
Length of output: 211
🏁 Script executed:
#!/bin/bash
# Check if 0037 is registered in the journal and inspect the snapshot
cat packages/db/drizzle/meta/_journal.json | jq '.[] | select(.tag | contains("0037"))'
echo "---"
cat packages/db/drizzle/meta/0037_snapshot.json | head -20Repository: superset-sh/superset
Length of output: 662
🏁 Script executed:
#!/bin/bash
# Check the raw journal structure
echo "=== Journal file (first 50 lines) ==="
head -50 packages/db/drizzle/meta/_journal.json
echo ""
echo "=== Check for 0037 in journal ==="
grep -i "0037" packages/db/drizzle/meta/_journal.jsonRepository: superset-sh/superset
Length of output: 1217
🏁 Script executed:
#!/bin/bash
# Extract the complete 0037 entry from the journal
echo "=== Full 0037 entry in journal ==="
cat packages/db/drizzle/meta/_journal.json | grep -A 5 '"0037_consolidate'
echo ""
echo "=== Snapshot version and dialect ==="
head -10 packages/db/drizzle/meta/0037_snapshot.json | grep -E '"version"|"dialect"'
echo ""
echo "=== Check snapshot file size ==="
wc -l packages/db/drizzle/meta/0037_snapshot.jsonRepository: superset-sh/superset
Length of output: 359
Hand-edited migration deviates from the packages/db/** guideline, but metadata is properly maintained.
This file is authored manually rather than generated by bunx drizzle-kit generate. The PR objectives explicitly justify this (drizzle-kit cannot synthesize the stringified-UUID → machine_id data translation between the column type change and the new composite FK), which is reasonable — but it's worth being explicit in the migration header or PR that this is a deliberate one-off so future contributors don't pattern-match on it.
The journal entry for 0037 and snapshot file are both present and properly synchronized, so no metadata inconsistency exists.
Per coding guidelines: "Never manually edit migration files in packages/db/drizzle/, including .sql files, meta/_journal.json, and snapshot files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/drizzle/0037_consolidate_host_client_machine_id.sql` around lines
1 - 59, The migration 0037_consolidate_host_client_machine_id.sql was
hand-edited to perform a stringified-UUID → machine_id translation (e.g., the
UPDATEs that set "host_id" = h."machine_id") rather than being generated by bunx
drizzle-kit; add a clear header comment at the top of this SQL (including
migration id 0037, author/date, and one-line rationale that this is a deliberate
one-off because drizzle-kit cannot express the intermediate string→machine
translation) so future contributors won’t regenerate or modify it by accident,
and ensure the PR description references this header and the journal/snapshot
synchronization already noted.
| @@ -0,0 +1,169 @@ | |||
| import { db, dbWs } from "@superset/db/client"; | |||
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the db client to confirm semantics of db vs dbWs.
fd -e ts | xargs rg -nP -C5 'export\s+(const|let|var)\s+(db|dbWs)\b' -g '**/db/client*'
echo "---"
# Scan other routers for prevailing convention.
fd -e ts -p 'packages/trpc/src/router' | xargs rg -nP -C1 '\bdbWs\b|\bdb\b\.(insert|update|delete)' | head -120Repository: superset-sh/superset
Length of output: 8688
🏁 Script executed:
cat -n packages/trpc/src/router/host/host.ts | head -200Repository: superset-sh/superset
Length of output: 5100
Use dbWs consistently for all database operations.
Both checkAccess (line 123) and setOnline (lines 137, 155) use db, while all other mutations (ensure, ensureClient) use dbWs. Every other router in the codebase (workspace, v2-workspace, v2-project) uses dbWs exclusively for all operations. For consistency and to match the established pattern, change these two operations to dbWs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/trpc/src/router/host/host.ts` at line 1, The router imports and uses
the non-WebSocket client `db` for checkAccess and setOnline while other
mutations use `dbWs`; update the import to rely on `dbWs` and replace all usages
of `db` inside the functions checkAccess and setOnline (both occurrences around
the database calls in those functions) to use `dbWs` so that these operations
consistently use the WebSocket client like ensure and ensureClient.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
3 issues found across 44 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/db/drizzle/0037_consolidate_host_client_machine_id.sql">
<violation number="1" location="packages/db/drizzle/0037_consolidate_host_client_machine_id.sql:56">
P1: `ON DELETE SET NULL` on this composite FK will try to NULL **both** `organization_id` and `host_id`. Since `organization_id` is `NOT NULL`, deleting a referenced `v2_host` will raise a constraint violation instead of gracefully clearing just `host_id`. Use the PostgreSQL 15+ column-list syntax to target only `host_id`.</violation>
</file>
<file name="packages/db/src/schema/schema.ts">
<violation number="1" location="packages/db/src/schema/schema.ts:742">
P1: Composite FK with `onDelete("set null")` will fail at runtime. PostgreSQL's `ON DELETE SET NULL` nulls **all** FK columns, but `organizationId` is NOT NULL, so deleting a referenced host will raise a constraint violation instead of gracefully nulling just `targetHostId`. PostgreSQL 15+ supports `SET NULL (column_name)` but Drizzle doesn't expose that syntax. Consider removing `.onDelete("set null")` and handling nulling `targetHostId` in application code, or splitting into two separate constraints.</violation>
</file>
<file name="packages/trpc/src/router/host/host.ts">
<violation number="1" location="packages/trpc/src/router/host/host.ts:53">
P1: The `v2UsersHosts` insert unconditionally grants `owner` role to whoever calls `ensure`, regardless of whether they actually own the host. Since `machineId` is exposed to all org members via synced collections, any org member can call `host.ensure` with another member's `machineId` and gain `owner` access to that host.
Guard the membership insert so it only grants `owner` when the caller is the host's creator (i.e., `host.createdByUserId === ctx.userId`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| ALTER TABLE "v2_users_hosts" ADD CONSTRAINT "v2_users_hosts_organization_id_user_id_host_id_pk" PRIMARY KEY("organization_id","user_id","host_id");--> statement-breakpoint | ||
|
|
||
| -- Add new composite FOREIGN KEYs (now that v2_hosts has its composite PK). | ||
| ALTER TABLE "automation_runs" ADD CONSTRAINT "automation_runs_host_fk" FOREIGN KEY ("organization_id","host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE set null ON UPDATE no action;--> statement-breakpoint |
Contributor
There was a problem hiding this comment.
P1: ON DELETE SET NULL on this composite FK will try to NULL both organization_id and host_id. Since organization_id is NOT NULL, deleting a referenced v2_host will raise a constraint violation instead of gracefully clearing just host_id. Use the PostgreSQL 15+ column-list syntax to target only host_id.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/drizzle/0037_consolidate_host_client_machine_id.sql, line 56:
<comment>`ON DELETE SET NULL` on this composite FK will try to NULL **both** `organization_id` and `host_id`. Since `organization_id` is `NOT NULL`, deleting a referenced `v2_host` will raise a constraint violation instead of gracefully clearing just `host_id`. Use the PostgreSQL 15+ column-list syntax to target only `host_id`.</comment>
<file context>
@@ -0,0 +1,59 @@
+ALTER TABLE "v2_users_hosts" ADD CONSTRAINT "v2_users_hosts_organization_id_user_id_host_id_pk" PRIMARY KEY("organization_id","user_id","host_id");--> statement-breakpoint
+
+-- Add new composite FOREIGN KEYs (now that v2_hosts has its composite PK).
+ALTER TABLE "automation_runs" ADD CONSTRAINT "automation_runs_host_fk" FOREIGN KEY ("organization_id","host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE set null ON UPDATE no action;--> statement-breakpoint
+ALTER TABLE "automations" ADD CONSTRAINT "automations_target_host_fk" FOREIGN KEY ("organization_id","target_host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE set null ON UPDATE no action;--> statement-breakpoint
+ALTER TABLE "v2_users_hosts" ADD CONSTRAINT "v2_users_hosts_host_fk" FOREIGN KEY ("organization_id","host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint
</file context>
Suggested change
| ALTER TABLE "automation_runs" ADD CONSTRAINT "automation_runs_host_fk" FOREIGN KEY ("organization_id","host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE set null ON UPDATE no action;--> statement-breakpoint | |
| ALTER TABLE "automation_runs" ADD CONSTRAINT "automation_runs_host_fk" FOREIGN KEY ("organization_id","host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE set null ("host_id") ON UPDATE no action;--> statement-breakpoint |
machineId alone collides on the relay tunnel map when the same physical
machine is registered as a host in multiple orgs. Switch the relay
routing key to "${organizationId}:${machineId}" everywhere the desktop,
host service, and cloud trpc construct it.
Code-splits buildHostRoutingKey / parseHostRoutingKey out of host-info
into a browser-safe @superset/shared/host-routing entry so the renderer
no longer transitively imports node:child_process via host-info.
Adds a useHostTargetUrl hook that wraps the (active host vs relay)
URL resolution with the active organization's id, replacing the inline
${env.RELAY_URL}/hosts/${hostId} pattern at every renderer call site.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/dispatchForkLaunch.ts (1)
166-178: LGTM, with a small UX nit on the null-org fallback.Threading
activeOrganizationIdthrough is right, and returningnullwhen it's missing for non-local targets cleanly reuses the existing "couldn't reach host service" path. Minor: if a terminal launch hits this branch (hostTarget.kind !== "local" && !activeOrganizationId), the user sees "Couldn't reach host service" on line 137, which misattributes a "no active organization" state to a connectivity problem. Consider a distinct toast (or a dedicated branch in the caller) for the missing-org case to make it actionable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/dispatchForkLaunch.ts around lines 166 - 178, resolveHostUrl currently returns null for non-local hostTarget when activeOrganizationId is missing, which causes callers (like dispatchForkLaunch) to show a misleading "Couldn't reach host service" toast; change resolveHostUrl to return a distinct sentinel (e.g., the string "MISSING_ORGANIZATION") instead of null when hostTarget.kind !== "local" and !activeOrganizationId (referencing resolveHostUrl, hostTarget, activeOrganizationId, buildHostRoutingKey, env.RELAY_URL), and update the caller (dispatchForkLaunch) to check for that sentinel and show a specific "no active organization" toast/handling path rather than the connectivity error.apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useAdoptWorktree/useAdoptWorktree.ts (1)
25-42: Consider extracting the local-vs-remote URL resolution into a shared helper.The same
kind === "local" ? activeHostUrl : activeOrganizationId ? buildHostRoutingKey(...) : nullpattern is duplicated acrossuseAdoptWorktree,useCheckoutDashboardWorkspace, anduseCreateDashboardWorkspace. The newuseHostTargetUrlhook centralizes this logic for components that resolve a URL at render time, but these mutation callbacks need resolution at call-time (insideuseCallback), so they can't reuse the hook directly.A small non-hook utility (e.g.,
resolveHostTargetUrl(hostTarget, { activeHostUrl, activeOrganizationId, relayUrl })) would let all three call sites share one implementation and keepuseHostTargetUrlas a thin wrapper around it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useAdoptWorktree/useAdoptWorktree.ts` around lines 25 - 42, The URL resolution logic in useAdoptWorktree is duplicated across useCheckoutDashboardWorkspace and useCreateDashboardWorkspace; extract it into a small non-hook utility (e.g., resolveHostTargetUrl(hostTarget, { activeHostUrl, activeOrganizationId, relayUrl })) and update useAdoptWorktree to call resolveHostTargetUrl inside its async callback to compute hostUrl instead of inlining the ternary, keeping useHostTargetUrl as a thin wrapper (if present) that calls the new utility and reusing buildHostRoutingKey for remote routing key construction.apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsx (1)
62-64: Consider passing the resolvedhostUrltodispatchForkLaunchdirectly.
useFireIntentnow resolveshostUrlonce viauseHostTargetUrl, butdispatchForkLaunchis still threaded bothactiveHostUrlandactiveOrganizationIdso it can re-derive the same URL. Passing the already-resolvedhostUrl(orpending.hostTargetplus the resolved URL) would avoid the duplicate resolution and remove one place where the local-vs-remote routing logic has to stay in sync.Minor — fine to defer until the helper extraction noted in
useAdoptWorktreelands.Also applies to: 167-167
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx around lines 62 - 64, Call sites should pass the already-resolved hostUrl (from useHostTargetUrl) into dispatchForkLaunch instead of letting dispatchForkLaunch re-derive the URL from activeHostUrl/activeOrganizationId; update dispatchForkLaunch's signature to accept an optional resolvedHostUrl (or both pending.hostTarget and resolved hostUrl) and prefer that value for routing logic, falling back to the existing activeHostUrl/activeOrganizationId behavior only if resolvedHostUrl is not provided; update the callers in page.tsx (where hostUrl is defined) to forward hostUrl when invoking dispatchForkLaunch so the duplicate resolution in useFireIntent/dispatchForkLaunch is eliminated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/dispatchForkLaunch.ts:
- Around line 166-178: resolveHostUrl currently returns null for non-local
hostTarget when activeOrganizationId is missing, which causes callers (like
dispatchForkLaunch) to show a misleading "Couldn't reach host service" toast;
change resolveHostUrl to return a distinct sentinel (e.g., the string
"MISSING_ORGANIZATION") instead of null when hostTarget.kind !== "local" and
!activeOrganizationId (referencing resolveHostUrl, hostTarget,
activeOrganizationId, buildHostRoutingKey, env.RELAY_URL), and update the caller
(dispatchForkLaunch) to check for that sentinel and show a specific "no active
organization" toast/handling path rather than the connectivity error.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/`$pendingId/page.tsx:
- Around line 62-64: Call sites should pass the already-resolved hostUrl (from
useHostTargetUrl) into dispatchForkLaunch instead of letting dispatchForkLaunch
re-derive the URL from activeHostUrl/activeOrganizationId; update
dispatchForkLaunch's signature to accept an optional resolvedHostUrl (or both
pending.hostTarget and resolved hostUrl) and prefer that value for routing
logic, falling back to the existing activeHostUrl/activeOrganizationId behavior
only if resolvedHostUrl is not provided; update the callers in page.tsx (where
hostUrl is defined) to forward hostUrl when invoking dispatchForkLaunch so the
duplicate resolution in useFireIntent/dispatchForkLaunch is eliminated.
In
`@apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useAdoptWorktree/useAdoptWorktree.ts`:
- Around line 25-42: The URL resolution logic in useAdoptWorktree is duplicated
across useCheckoutDashboardWorkspace and useCreateDashboardWorkspace; extract it
into a small non-hook utility (e.g., resolveHostTargetUrl(hostTarget, {
activeHostUrl, activeOrganizationId, relayUrl })) and update useAdoptWorktree to
call resolveHostTargetUrl inside its async callback to compute hostUrl instead
of inlining the ternary, keeping useHostTargetUrl as a thin wrapper (if present)
that calls the new utility and reusing buildHostRoutingKey for remote routing
key construction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 66b39d87-a399-4b50-9874-6c8d34dd9996
📒 Files selected for processing (24)
apps/desktop/src/renderer/hooks/host-service/useHostTargetUrl/index.tsapps/desktop/src/renderer/hooks/host-service/useHostTargetUrl/useHostTargetUrl.tsapps/desktop/src/renderer/hooks/host-service/useWorkspaceHostUrl/useWorkspaceHostUrl.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/hooks/useProjectFileSearch/useProjectFileSearch.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.utils.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/dispatchForkLaunch.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/pending/$pendingId/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/layout.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/GitHubIssueLinkCommand/GitHubIssueLinkCommand.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/PromptGroup/components/PRLinkCommand/PRLinkCommand.tsxapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceForm/hooks/useBranchContext/useBranchContext.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceModalContent/hooks/useSelectedHostProjectIds/useSelectedHostProjectIds.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useAdoptWorktree/useAdoptWorktree.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCheckoutDashboardWorkspace/useCheckoutDashboardWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/hooks/useCreateDashboardWorkspace/useCreateDashboardWorkspace.tsapps/desktop/src/renderer/routes/_authenticated/components/GlobalTerminalLifecycle/hooks/useGlobalTerminalLifecycle/useGlobalTerminalLifecycle.tsapps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsxpackages/host-service/src/tunnel/connect.tspackages/shared/package.jsonpackages/shared/src/host-routing.tspackages/trpc/src/router/automation/dispatch.tspackages/trpc/src/router/host/host.ts
✅ Files skipped from review due to trivial changes (2)
- apps/desktop/src/renderer/hooks/host-service/useHostTargetUrl/index.ts
- apps/desktop/src/renderer/routes/_authenticated/components/DashboardNewWorkspaceModal/components/DashboardNewWorkspaceModalContent/hooks/useSelectedHostProjectIds/useSelectedHostProjectIds.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/shared/package.json
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.ts
- apps/desktop/src/renderer/routes/_authenticated/components/V2NotificationController/V2NotificationController.tsx
- packages/host-service/src/tunnel/connect.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/components/DashboardSidebarPortsList/hooks/useDashboardSidebarPortsData/useDashboardSidebarPortsData.test.ts
- host.ensure: gate v2_users_hosts owner-grant on host.createdByUserId === ctx.userId. Prevents an org member who knows another member's machineId (it's in the synced collection) from calling host.ensure and being granted owner role on someone else's host. - Drop composite FKs on automations.target_host_id and automation_runs.host_id (migration 0038). These nullable refs originally used ON DELETE SET NULL with a single-column FK; the consolidation made them composite, which breaks SET NULL semantics (PG would try to NULL the NOT NULL organization_id too). Both columns are best-effort pointers — dispatcher already handles the host-not-found path — so dropping the FK is the cleaner fix than the PG15+ column-list workaround. v2_users_hosts (CASCADE) and v2_workspaces (NOT NULL, deletion-blocks) keep their composite FKs intentionally.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/db/src/schema/schema.ts (1)
444-447: Remove redundantorganizationIdindexes in v2_hosts, v2_clients, and v2_users_hosts tables.With
organizationIdas the leading column in the composite primary keys for all three tables, PostgreSQL implicitly creates and maintains a B-tree index that satisfies any queries filtering onorganizationIdalone. The explicit single-column indexes (v2_hosts_organization_id_idx,v2_clients_organization_id_idx,v2_users_hosts_organization_id_idx) are redundant and only add write overhead and disk usage with no read benefit. Query audit confirms all accesses are composite filters (e.g.,organizationId + machineId), never single-column lookups.The
userIdandhostIdindexes remain necessary since they aren't leading columns.♻️ Proposed cleanup for all three tables
(table) => [ primaryKey({ columns: [table.organizationId, table.machineId] }), - index("v2_hosts_organization_id_idx").on(table.organizationId), ],(table) => [ primaryKey({ columns: [table.organizationId, table.userId, table.machineId], }), - index("v2_clients_organization_id_idx").on(table.organizationId), index("v2_clients_user_id_idx").on(table.userId), ],(table) => [ primaryKey({ columns: [table.organizationId, table.userId, table.hostId], }), foreignKey({ columns: [table.organizationId, table.hostId], foreignColumns: [v2Hosts.organizationId, v2Hosts.machineId], name: "v2_users_hosts_host_fk", }).onDelete("cascade"), - index("v2_users_hosts_organization_id_idx").on(table.organizationId), index("v2_users_hosts_user_id_idx").on(table.userId), index("v2_users_hosts_host_id_idx").on(table.hostId), ],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/db/src/schema/schema.ts` around lines 444 - 447, Remove the redundant single-column organizationId indexes that duplicate the composite PK for each table: delete the index("v2_hosts_organization_id_idx").on(table.organizationId) in the v2_hosts table definition (which already has primaryKey({ columns: [table.organizationId, table.machineId] })), and likewise remove index("v2_clients_organization_id_idx") and index("v2_users_hosts_organization_id_idx") from the v2_clients and v2_users_hosts table definitions; keep the composite primaryKey declarations and retain the remaining indexes (e.g., userId and hostId indexes) that are not covered by the PK.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/db/src/schema/schema.ts`:
- Around line 444-447: Remove the redundant single-column organizationId indexes
that duplicate the composite PK for each table: delete the
index("v2_hosts_organization_id_idx").on(table.organizationId) in the v2_hosts
table definition (which already has primaryKey({ columns: [table.organizationId,
table.machineId] })), and likewise remove
index("v2_clients_organization_id_idx") and
index("v2_users_hosts_organization_id_idx") from the v2_clients and
v2_users_hosts table definitions; keep the composite primaryKey declarations and
retain the remaining indexes (e.g., userId and hostId indexes) that are not
covered by the PK.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 15871f40-c7f3-409d-a287-4244a6d1f755
📒 Files selected for processing (5)
packages/db/drizzle/0038_drop_optional_host_fks.sqlpackages/db/drizzle/meta/0038_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/schema.tspackages/trpc/src/router/host/host.ts
✅ Files skipped from review due to trivial changes (1)
- packages/db/drizzle/0038_drop_optional_host_fks.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/db/drizzle/meta/_journal.json
…nt-identity # Conflicts: # apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.ts # apps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.ts # packages/db/drizzle/meta/0037_snapshot.json # packages/db/drizzle/meta/0038_snapshot.json # packages/db/drizzle/meta/_journal.json # packages/host-service/src/trpc/router/workspace-creation/procedures/adopt.ts # packages/trpc/src/router/v2-workspace/v2-workspace.ts
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Pre-PR FK on automation_runs.host_id and automations.target_host_id was single-column uuid → v2_hosts.id; it didn't enforce that the row's organization_id matched the host's. If any cross-org row existed (past bug, manual SQL, etc.), the uuid→machineId translation step would write the wrong-org's machine_id, and the new composite FK on (organization_id, host_id) would reject it. Switch the null-out predicate from `host_id NOT IN (SELECT machine_id …)` to `NOT EXISTS (… WHERE machine_id = X AND organization_id = X)` so the migration self-defends instead of trusting the cross-org invariant. Identical behavior on data that satisfies the invariant (prod confirmed zero orphans), defensive against environments that don't.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts (1)
235-258: Join updates look correct; consider dropping the now-redundanthostMachineIdfield.The join now keys on
hosts.machineId, which matches the new identity model whereworkspaces.hostIdstores the machineId text. As a result, the selectedhostMachineIdfield is effectively the same value ashostId(modulo the left-joinnullcase when no host row exists). DownstreamhostTypederivation could be simplified to usehostIddirectly and a "host row exists" boolean, eliminating the duplicate field and thenullfallback gymnastics.Not blocking — happy to leave as-is for a smaller diff.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts` around lines 235 - 258, Remove the redundant hostMachineId selection in the query inside useDashboardSidebarData: stop selecting hostMachineId from hosts?.machineId and instead rely on hostId (workspaces.hostId) plus the presence of hosts (e.g., hosts !== null) wherever hostMachineId was previously used; update any downstream logic that derived hostType or checked host existence to use hostId and a boolean like hostExists (hosts != null) so null-fallback gymnastics are eliminated and the select projection only returns id, projectId, hostId, type, hostIsOnline, name, branch, createdAt, updatedAt, tabOrder, sectionId, isHidden.apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.ts (1)
25-31:hostIdnow duplicateshostMachineId; thestring | nullannotation and "cloud" branch are stale.After the join switch (
workspaces.hostId ↔ hosts.machineId),row.hostIdandrow.hostMachineIdare guaranteed-equal projections of the samev2Hosts.machineIdcolumn. Two follow-on issues:
AccessibleV2Workspace.hostMachineId: string | nullno longer reflects reality — the inner join on a NOT NULLmachineIdmakes it always a string.- The
row.hostMachineId == null ? "cloud" : ...branch (and the correspondingrow.hostMachineId != null && ...guard) is unreachable.Consider dropping the duplicate field (or the
| nullfrom its type) and simplifying thehostType/isAutoVisibleMainlogic to comparerow.hostId === machineId. If a "cloud workspace with no host" case still exists somewhere, it'd require switching to aleftJoin, which is a separate decision.♻️ Sketch
export interface AccessibleV2Workspace { ... hostId: string; hostName: string; - hostMachineId: string | null; hostIsOnline: boolean; hostType: V2WorkspaceHostType; ... }- const hostType: V2WorkspaceHostType = - row.hostMachineId == null - ? "cloud" - : row.hostMachineId === machineId - ? "local-device" - : "remote-device"; - const isAutoVisibleMain = - row.type === "main" && - row.hostMachineId != null && - row.hostMachineId === machineId && - row.sidebarProjectId != null; + const hostType: V2WorkspaceHostType = + row.hostId === machineId ? "local-device" : "remote-device"; + const isAutoVisibleMain = + row.type === "main" && + row.hostId === machineId && + row.sidebarProjectId != null;Also applies to: 138-145, 154-164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.ts` around lines 25 - 31, AccessibleV2Workspace currently declares hostMachineId as string | null but hostId and hostMachineId are identical non-null projections after the join; update the hook useAccessibleV2Workspaces to remove the unreachable null branch and duplicate field by either (a) deleting hostMachineId and using hostId everywhere, or (b) change AccessibleV2Workspace.hostMachineId to string (remove | null) and remove the row.hostMachineId == null ? "cloud" : ... and the guards; simplify hostType and isAutoVisibleMain logic to compare row.hostId === machineId (and remove any cloud/leftJoin handling unless you intentionally convert the SQL to a leftJoin).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.ts`:
- Around line 235-258: Remove the redundant hostMachineId selection in the query
inside useDashboardSidebarData: stop selecting hostMachineId from
hosts?.machineId and instead rely on hostId (workspaces.hostId) plus the
presence of hosts (e.g., hosts !== null) wherever hostMachineId was previously
used; update any downstream logic that derived hostType or checked host
existence to use hostId and a boolean like hostExists (hosts != null) so
null-fallback gymnastics are eliminated and the select projection only returns
id, projectId, hostId, type, hostIsOnline, name, branch, createdAt, updatedAt,
tabOrder, sectionId, isHidden.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.ts`:
- Around line 25-31: AccessibleV2Workspace currently declares hostMachineId as
string | null but hostId and hostMachineId are identical non-null projections
after the join; update the hook useAccessibleV2Workspaces to remove the
unreachable null branch and duplicate field by either (a) deleting hostMachineId
and using hostId everywhere, or (b) change AccessibleV2Workspace.hostMachineId
to string (remove | null) and remove the row.hostMachineId == null ? "cloud" :
... and the guards; simplify hostType and isAutoVisibleMain logic to compare
row.hostId === machineId (and remove any cloud/leftJoin handling unless you
intentionally convert the SQL to a leftJoin).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f31086d9-49b8-4d18-b36d-8b6c662a575e
📒 Files selected for processing (21)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/automations/page.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/DashboardSidebar/hooks/useDashboardSidebarData/useDashboardSidebarData.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspaces/hooks/useAccessibleV2Workspaces/useAccessibleV2Workspaces.tsapps/desktop/src/renderer/routes/_authenticated/hooks/useOptimisticCollectionActions/useOptimisticCollectionActions.tsapps/desktop/src/renderer/routes/_authenticated/providers/CollectionsProvider/collections.tsapps/desktop/src/renderer/routes/_authenticated/settings/hosts/$hostId/components/HostSettings/HostSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/hosts/components/HostsSettingsSidebar/HostsSettingsSidebar.tsxapps/desktop/src/renderer/routes/_authenticated/settings/hosts/page.tsxpackages/db/drizzle/0039_consolidate_host_client_machine_id.sqlpackages/db/drizzle/meta/0039_snapshot.jsonpackages/db/drizzle/meta/_journal.jsonpackages/db/src/schema/schema.tspackages/host-service/src/trpc/router/project/utils/ensure-main-workspace.tspackages/host-service/src/trpc/router/workspace-creation/procedures/adopt.tspackages/host-service/src/trpc/router/workspace-creation/procedures/create.tspackages/host-service/src/trpc/router/workspace/workspace.tspackages/shared/package.jsonpackages/trpc/src/root.tspackages/trpc/src/router/automation/schema.tspackages/trpc/src/router/v2-host/v2-host.tspackages/trpc/src/router/v2-workspace/v2-workspace.ts
✅ Files skipped from review due to trivial changes (2)
- packages/db/drizzle/meta/_journal.json
- packages/trpc/src/router/automation/schema.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/shared/package.json
- packages/host-service/src/trpc/router/workspace/workspace.ts
- packages/db/src/schema/schema.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/db/drizzle/0039_consolidate_host_client_machine_id.sql`:
- Around line 1-69: The migration file
0039_consolidate_host_client_machine_id.sql is hand-authored and violates the
repo workflow; regenerate the migration from the schema in
packages/db/src/schema/schema.ts with bunx drizzle-kit generate
--name=consolidate_host_client_machine_id and remove the manually written full
SQL, keeping only the non-expressible DB transformations as a small companion
custom SQL (use drizzle-kit --custom): specifically preserve the ALTER TABLE ...
SET DATA TYPE ... USING "host_id"::text casts and the UPDATE translations that
set machine_id (the UPDATE statements targeting "automation_runs",
"automations", "v2_users_hosts", "v2_workspaces" that replace id::text with
h."machine_id") and the cross-org NULL-out UPDATEs that set host_id=NULL for
"automation_runs" and "automations"; ensure all other DDL (DROP TABLE
session_hosts, dropping/adding constraints, composite PK/FK changes, index
recreation) is produced by drizzle from the schema rather than handwritten in
0039.
- Around line 5-10: The DELETEs only remove rows whose host_id is missing
entirely; update them to also remove rows where the host exists but belongs to a
different organization. For v2_users_hosts and v2_workspaces, change the WHERE
clause to delete rows matching either no host (no row in v2_hosts with id =
host_id) OR a host whose organization_id does not equal the row's
organization_id (use NOT EXISTS (SELECT 1 FROM v2_hosts h WHERE h.id = host_id
AND h.organization_id = <table>.organization_id)). Reference tables/columns:
v2_users_hosts(host_id, organization_id), v2_workspaces(host_id,
organization_id), v2_hosts(id, organization_id).
🪄 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: ff848512-6e90-4889-8587-a6c9aba51846
📒 Files selected for processing (1)
packages/db/drizzle/0039_consolidate_host_client_machine_id.sql
Comment on lines
+1
to
+69
| -- session_hosts is dead (no writes anywhere); drop with CASCADE. | ||
| ALTER TABLE "session_hosts" DISABLE ROW LEVEL SECURITY;--> statement-breakpoint | ||
| DROP TABLE "session_hosts" CASCADE;--> statement-breakpoint | ||
|
|
||
| -- Delete orphan FK rows in NOT NULL columns BEFORE the type change. | ||
| -- Once host_id is text, we can't easily distinguish orphans from valid uuids, | ||
| -- and the post-translation rows would still have stringified-UUID values that | ||
| -- can't satisfy the new composite FK to v2_hosts(organization_id, machine_id). | ||
| DELETE FROM "v2_users_hosts" WHERE "host_id" NOT IN (SELECT "id" FROM "v2_hosts");--> statement-breakpoint | ||
| DELETE FROM "v2_workspaces" WHERE "host_id" NOT IN (SELECT "id" FROM "v2_hosts");--> statement-breakpoint | ||
|
|
||
| -- Drop old unique constraints (replaced by composite PKs below). | ||
| ALTER TABLE "v2_clients" DROP CONSTRAINT "v2_clients_org_user_machine_unique";--> statement-breakpoint | ||
| ALTER TABLE "v2_hosts" DROP CONSTRAINT "v2_hosts_org_machine_id_unique";--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" DROP CONSTRAINT "v2_users_hosts_org_user_host_unique";--> statement-breakpoint | ||
|
|
||
| -- Drop old FK constraints (column type change requires no incoming FK). | ||
| ALTER TABLE "automation_runs" DROP CONSTRAINT "automation_runs_host_id_v2_hosts_id_fk";--> statement-breakpoint | ||
| ALTER TABLE "automations" DROP CONSTRAINT "automations_target_host_id_v2_hosts_id_fk";--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" DROP CONSTRAINT "v2_users_hosts_host_id_v2_hosts_id_fk";--> statement-breakpoint | ||
| ALTER TABLE "v2_workspaces" DROP CONSTRAINT "v2_workspaces_host_id_v2_hosts_id_fk";--> statement-breakpoint | ||
|
|
||
| -- Drop the partial unique index on v2_workspaces (project_id, host_id) WHERE | ||
| -- type='main' added by 0038. Recreated below after host_id becomes text. | ||
| DROP INDEX "v2_workspaces_one_main_per_host";--> statement-breakpoint | ||
|
|
||
| -- Cast FK columns uuid -> text. PostgreSQL has no implicit uuid->text cast, | ||
| -- so USING is required. We stringify the uuid here and translate to | ||
| -- machine_id in the UPDATEs below. | ||
| ALTER TABLE "automation_runs" ALTER COLUMN "host_id" SET DATA TYPE text USING "host_id"::text;--> statement-breakpoint | ||
| ALTER TABLE "automations" ALTER COLUMN "target_host_id" SET DATA TYPE text USING "target_host_id"::text;--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" ALTER COLUMN "host_id" SET DATA TYPE text USING "host_id"::text;--> statement-breakpoint | ||
| ALTER TABLE "v2_workspaces" ALTER COLUMN "host_id" SET DATA TYPE text USING "host_id"::text;--> statement-breakpoint | ||
|
|
||
| -- Translate stringified UUIDs to machine_ids via UPDATE FROM v2_hosts. | ||
| -- v2_hosts.id still exists at this point (dropped further down); cast to text | ||
| -- to compare against the now-text host_id columns. | ||
| UPDATE "automation_runs" SET "host_id" = h."machine_id" FROM "v2_hosts" h WHERE "automation_runs"."host_id" = h."id"::text;--> statement-breakpoint | ||
| UPDATE "automations" SET "target_host_id" = h."machine_id" FROM "v2_hosts" h WHERE "automations"."target_host_id" = h."id"::text;--> statement-breakpoint | ||
| UPDATE "v2_users_hosts" SET "host_id" = h."machine_id" FROM "v2_hosts" h WHERE "v2_users_hosts"."host_id" = h."id"::text;--> statement-breakpoint | ||
| UPDATE "v2_workspaces" SET "host_id" = h."machine_id" FROM "v2_hosts" h WHERE "v2_workspaces"."host_id" = h."id"::text;--> statement-breakpoint | ||
|
|
||
| -- For nullable columns, NULL out any rows that don't have a matching | ||
| -- (organization_id, machine_id) pair in v2_hosts. NOT EXISTS with the | ||
| -- composite check rather than a global machine_id IN (...) so the migration | ||
| -- doesn't rely on an implicit "automation_runs.organization_id always matches | ||
| -- the host's organization_id" invariant — if any cross-org row exists, the | ||
| -- translation step above would have written the wrong-org's machine_id and | ||
| -- the new composite FK would reject it. | ||
| UPDATE "automation_runs" SET "host_id" = NULL WHERE "host_id" IS NOT NULL AND NOT EXISTS (SELECT 1 FROM "v2_hosts" h WHERE h."machine_id" = "automation_runs"."host_id" AND h."organization_id" = "automation_runs"."organization_id");--> statement-breakpoint | ||
| UPDATE "automations" SET "target_host_id" = NULL WHERE "target_host_id" IS NOT NULL AND NOT EXISTS (SELECT 1 FROM "v2_hosts" h WHERE h."machine_id" = "automations"."target_host_id" AND h."organization_id" = "automations"."organization_id");--> statement-breakpoint | ||
|
|
||
| -- Drop old uuid `id` columns. Implicitly drops the old PRIMARY KEY constraints, | ||
| -- freeing the tables to receive composite PKs below. | ||
| ALTER TABLE "v2_clients" DROP COLUMN "id";--> statement-breakpoint | ||
| ALTER TABLE "v2_hosts" DROP COLUMN "id";--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" DROP COLUMN "id";--> statement-breakpoint | ||
|
|
||
| -- Add new composite PRIMARY KEYs. | ||
| ALTER TABLE "v2_clients" ADD CONSTRAINT "v2_clients_organization_id_user_id_machine_id_pk" PRIMARY KEY("organization_id","user_id","machine_id");--> statement-breakpoint | ||
| ALTER TABLE "v2_hosts" ADD CONSTRAINT "v2_hosts_organization_id_machine_id_pk" PRIMARY KEY("organization_id","machine_id");--> statement-breakpoint | ||
| ALTER TABLE "v2_users_hosts" ADD CONSTRAINT "v2_users_hosts_organization_id_user_id_host_id_pk" PRIMARY KEY("organization_id","user_id","host_id");--> statement-breakpoint | ||
|
|
||
| -- Add new composite FOREIGN KEYs (now that v2_hosts has its composite PK). | ||
| ALTER TABLE "v2_users_hosts" ADD CONSTRAINT "v2_users_hosts_host_fk" FOREIGN KEY ("organization_id","host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE cascade ON UPDATE no action;--> statement-breakpoint | ||
| ALTER TABLE "v2_workspaces" ADD CONSTRAINT "v2_workspaces_host_fk" FOREIGN KEY ("organization_id","host_id") REFERENCES "public"."v2_hosts"("organization_id","machine_id") ON DELETE no action ON UPDATE no action;--> statement-breakpoint | ||
|
|
||
| -- Recreate the partial unique index on v2_workspaces against the now-text host_id. | ||
| CREATE UNIQUE INDEX "v2_workspaces_one_main_per_host" ON "v2_workspaces" USING btree ("project_id","host_id") WHERE "v2_workspaces"."type" = 'main'; |
Contributor
There was a problem hiding this comment.
Hand-authored migration file violates repository workflow.
This entire file is a manually authored SQL migration under packages/db/drizzle/, but the project mandates that migrations be generated from schema changes. The PR objectives even call this out explicitly ("provides hand-authored SQL"). The schema definitions in packages/db/src/schema/schema.ts already declare the target shape (composite PKs, text machine_id/host_id, removal of session_hosts), so the migration should be produced from those.
Please regenerate this migration via bunx drizzle-kit generate --name=consolidate_host_client_machine_id and only retain hand-authored SQL fragments where Drizzle cannot express the data transformation (UUID→text cast USING clauses, the stringified-UUID → machine_id translation in lines 38-41, and the cross-org NULL-out in lines 50-51). Those bits typically go into a small companion .sql produced by drizzle-kit generate --custom, not a fully hand-written 0039_*.sql.
As per coding guidelines: "Create database migrations by modifying schema files in packages/db/src/schema/ and running bunx drizzle-kit generate --name=\"<sample_name_snake_case>\". Never manually edit migration files in packages/db/drizzle/, including .sql files, meta/_journal.json, and snapshot files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/drizzle/0039_consolidate_host_client_machine_id.sql` around lines
1 - 69, The migration file 0039_consolidate_host_client_machine_id.sql is
hand-authored and violates the repo workflow; regenerate the migration from the
schema in packages/db/src/schema/schema.ts with bunx drizzle-kit generate
--name=consolidate_host_client_machine_id and remove the manually written full
SQL, keeping only the non-expressible DB transformations as a small companion
custom SQL (use drizzle-kit --custom): specifically preserve the ALTER TABLE ...
SET DATA TYPE ... USING "host_id"::text casts and the UPDATE translations that
set machine_id (the UPDATE statements targeting "automation_runs",
"automations", "v2_users_hosts", "v2_workspaces" that replace id::text with
h."machine_id") and the cross-org NULL-out UPDATEs that set host_id=NULL for
"automation_runs" and "automations"; ensure all other DDL (DROP TABLE
session_hosts, dropping/adding constraints, composite PK/FK changes, index
recreation) is produced by drizzle from the schema rather than handwritten in
0039.
Comment on lines
+5
to
+10
| -- Delete orphan FK rows in NOT NULL columns BEFORE the type change. | ||
| -- Once host_id is text, we can't easily distinguish orphans from valid uuids, | ||
| -- and the post-translation rows would still have stringified-UUID values that | ||
| -- can't satisfy the new composite FK to v2_hosts(organization_id, machine_id). | ||
| DELETE FROM "v2_users_hosts" WHERE "host_id" NOT IN (SELECT "id" FROM "v2_hosts");--> statement-breakpoint | ||
| DELETE FROM "v2_workspaces" WHERE "host_id" NOT IN (SELECT "id" FROM "v2_hosts");--> statement-breakpoint |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the migration file
fd -a "0039_consolidate_host_client_machine_id.sql" packages/db/Repository: superset-sh/superset
Length of output: 147
🏁 Script executed:
# Read the migration file to verify the content and line numbers
wc -l /home/jailuser/git/packages/db/drizzle/0039_consolidate_host_client_machine_id.sqlRepository: superset-sh/superset
Length of output: 150
🏁 Script executed:
# Read the entire migration file with line numbers
cat -n /home/jailuser/git/packages/db/drizzle/0039_consolidate_host_client_machine_id.sqlRepository: superset-sh/superset
Length of output: 6511
🏁 Script executed:
# Find the schema files
fd -a "schema" packages/db/src/Repository: superset-sh/superset
Length of output: 159
🏁 Script executed:
# Check schema file size first
wc -l /home/jailuser/git/packages/db/src/schema/schema.tsRepository: superset-sh/superset
Length of output: 120
🏁 Script executed:
# Search for v2_users_hosts and v2_workspaces table definitions and their host_id column
rg -A 30 "export const v2_users_hosts" /home/jailuser/git/packages/db/src/schema/schema.tsRepository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for table definitions differently
rg "v2_users_hosts|v2_workspaces" /home/jailuser/git/packages/db/src/schema/schema.ts | head -20Repository: superset-sh/superset
Length of output: 590
🏁 Script executed:
# Search for the table definitions more carefully
rg -B 5 "v2_users_hosts_host_fk" /home/jailuser/git/packages/db/src/schema/schema.ts | head -40Repository: superset-sh/superset
Length of output: 298
🏁 Script executed:
# Find the v2_users_hosts table definition with column definitions
rg -B 50 "v2_users_hosts_host_fk" /home/jailuser/git/packages/db/src/schema/schema.ts | grep -A 50 "export const v2_users_hosts"Repository: superset-sh/superset
Length of output: 46
🏁 Script executed:
# Search for the table definition with a different pattern
rg "hostId.*text" /home/jailuser/git/packages/db/src/schema/schema.ts | head -20Repository: superset-sh/superset
Length of output: 165
🏁 Script executed:
# Get context around hostId definitions to identify which tables they belong to
rg -B 5 "hostId.*text" /home/jailuser/git/packages/db/src/schema/schema.tsRepository: superset-sh/superset
Length of output: 642
🏁 Script executed:
# Get more context to identify which tables own these columns
rg -B 15 'hostId.*text.*notNull' /home/jailuser/git/packages/db/src/schema/schema.ts | grep -E "export const|hostId: text"Repository: superset-sh/superset
Length of output: 212
Cross-org references in NOT NULL host columns will break the new composite FK.
The orphan-purge at lines 9–10 only deletes rows whose host_id doesn't exist in v2_hosts.id at all. The pre-existing FK was host_id → v2_hosts.id (single-column UUID), which did not constrain organization_id to match. So a v2_users_hosts or v2_workspaces row in org_A referencing a v2_hosts row in org_B is not an orphan today — but after the UPDATEs at lines 40–41 it will get rewritten to org_B's machine_id, and the new composite FK at lines 65–66 will then fail.
The nullable columns (automation_runs, automations) are handled correctly at lines 50–51 with a composite NOT EXISTS check; the NOT NULL columns need the same composite check (delete, since they can't be NULLed). The dev-fork validation reported 0 orphans, but production may differ and the migration is one-way.
🛡️ Proposed fix to also purge cross-org references
-DELETE FROM "v2_users_hosts" WHERE "host_id" NOT IN (SELECT "id" FROM "v2_hosts");--> statement-breakpoint
-DELETE FROM "v2_workspaces" WHERE "host_id" NOT IN (SELECT "id" FROM "v2_hosts");--> statement-breakpoint
+DELETE FROM "v2_users_hosts" WHERE NOT EXISTS (
+ SELECT 1 FROM "v2_hosts" h
+ WHERE h."id" = "v2_users_hosts"."host_id"
+ AND h."organization_id" = "v2_users_hosts"."organization_id"
+);--> statement-breakpoint
+DELETE FROM "v2_workspaces" WHERE NOT EXISTS (
+ SELECT 1 FROM "v2_hosts" h
+ WHERE h."id" = "v2_workspaces"."host_id"
+ AND h."organization_id" = "v2_workspaces"."organization_id"
+);--> statement-breakpointWorth confirming on the production snapshot before deploy — count any cross-org rows that would be silently dropped/rejected:
-- Run against a prod read replica / snapshot before applying the migration.
SELECT COUNT(*) AS cross_org_users_hosts
FROM v2_users_hosts uh
JOIN v2_hosts h ON h.id = uh.host_id
WHERE h.organization_id <> uh.organization_id;
SELECT COUNT(*) AS cross_org_workspaces
FROM v2_workspaces w
JOIN v2_hosts h ON h.id = w.host_id
WHERE h.organization_id <> w.organization_id;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/db/drizzle/0039_consolidate_host_client_machine_id.sql` around lines
5 - 10, The DELETEs only remove rows whose host_id is missing entirely; update
them to also remove rows where the host exists but belongs to a different
organization. For v2_users_hosts and v2_workspaces, change the WHERE clause to
delete rows matching either no host (no row in v2_hosts with id = host_id) OR a
host whose organization_id does not equal the row's organization_id (use NOT
EXISTS (SELECT 1 FROM v2_hosts h WHERE h.id = host_id AND h.organization_id =
<table>.organization_id)). Reference tables/columns: v2_users_hosts(host_id,
organization_id), v2_workspaces(host_id, organization_id), v2_hosts(id,
organization_id).
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
v2_hosts,v2_clients,v2_users_hostsin favor of composite PKs keyed on the existingmachineId(output ofgetHashedDeviceId(), nowgetHostId()). One canonical identifier end-to-end — DB, cloud API, relay, host service, renderer — instead of translating between a server-issued UUID and a hashed-machine-id text in different layers.devicetrpc router tohostfor the v2-host procedures (host.ensure,host.ensureClient,host.checkAccess,host.setOnline). Keeps a v1devicerouter with justheartbeatandregisterDeviceso the existingdevice_presencetable stays untouched and gets retired with the rest of v1.packages/shared/src/device-info.ts→host-info.ts) and the desktop env vars (DEVICE_CLIENT_ID/DEVICE_NAME→HOST_CLIENT_ID/HOST_NAME). Function/value semantics preserved (salt unchanged, existing host IDs stay stable).session_hoststable — confirmed dead (no writes anywhere, no renderer consumers).HOST_SERVICE_VERSIONandMIN_HOST_SERVICE_VERSIONto0.3.0so the desktop coordinator force-respawns any adopted host service still callingdevice.ensureV2Host.Migration
packages/db/drizzle/0037_consolidate_host_client_machine_id.sqlis hand-edited (drizzle-kit's auto-generated SQL doesn't translate FK values nor handle the order in which old PKs / new PKs swap). The order is:session_hosts.v2_users_hosts/v2_workspaces(NOT NULL FK columns).USING host_id::text(Postgres doesn't support subqueries inUSING, so we stringify here and translate below).UPDATE FROM v2_hoststo translate stringified UUIDs tomachine_idvalues.automations.target_host_id,automation_runs.host_id).idcolumns (implicitly drops old PRIMARY KEY constraints).Validated against a Neon dev branch forked from prod:
device_presencerows preservedhost_id/target_host_idvalues are 32-hexmachineIds, no UUID-string remnantsON DELETEbehaviorsdevice_presenceanddevice_typeenum untouchedsession_hostsgone, removed from Electric SQL publicationDeploy
Cloud API + relay + desktop must ship together:
device.ensureV2Host,device.checkHostAccess,device.setHostOnlineare gone — relay calls fail until cloud is on new code.MIN_HOST_SERVICE_VERSION = 0.3.0mitigates by forcing kill+respawn of adopted host services that report a lower version.The migration is one-way: dropping
v2_hosts.idcan't be reverted in place. Confirmed safe on a fresh fork of prod.Out of Scope
device_presence(v1 concept) and thedevice.heartbeat/device.registerDeviceprocedures it backs — left fully intact, will be retired with v1.getDeviceInfoelectron tRPC procedure still returns{ deviceId, deviceName }so the v1useDevicePresencehook keeps working unchanged.v2_workspaces.id,automations.id, etc.) — those are user-visible entity IDs exposed via the API; intentionally kept.Test plan
host_idvalues are machineId-formatted.host.ensureand tunnel setsv2_hosts.is_online = true.v2_workspaces.host_idstores machineId text.bun run typecheckclean across all 26 packages.bun run lint:fixclean.env.test.ts,useDashboardSidebarPortsData.test.ts) green.Summary by cubic
Unifies host/client identity on machineId and scopes relay routing to
${organizationId}:${machineId}to prevent collisions. DB, APIs, relay, desktop, and host-service now use onegetHostId(); v2 APIs move fromdevicetohost.Refactors
v2_hosts,v2_clients,v2_users_hosts; added composite PKs on(organization_id, machine_id)and updated joins tov2_hosts.machine_id.v2_workspaces.host_idandautomations.target_host_idare now machineId text;v2_workspaceskeeps a composite FK; nullable host refs inautomations/automation_runsare no longer FK‑constrained.devicetohost(host.ensure,host.ensureClient,host.checkAccess,host.setOnline); v1device.heartbeat/device.registerDeviceretained fordevice_presence.@superset/shared/host-routing(buildHostRoutingKey); newuseHostTargetUrlresolves local vs relay URLs with org context.@superset/shared/device-info→@superset/shared/host-info(getHashedDeviceId→getHostId,getDeviceName→getHostName); desktop env varsDEVICE_*→HOST_CLIENT_ID/HOST_NAME.0.3.0; desktopMIN_HOST_SERVICE_VERSIONset to0.3.0to restart older binaries that call removeddevice.*procedures.session_hoststable and related code; tightened owner grants sohost.ensureonly assigns owner whencreatedByUserId === caller.Migration
packages/db/drizzle/0039_consolidate_host_client_machine_id.sql: casts FK columns to text, translates UUIDs to machineIds, drops oldidcolumns, installs composite PKs/FKs, and recreates the partialv2_workspaces(project_id, host_id) WHERE type='main'index. Defensively NULLs unmatched nullable refs using a(organization_id, machine_id)NOT EXISTS check.packages/db/drizzle/0038_drop_optional_host_fks.sqldrops FKs onautomations.target_host_idandautomation_runs.host_idto avoid broken SET NULL semantics with composite keys.host.ensure.Written for commit fa086ab. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Chores