diff --git a/workspace-server/internal/handlers/platform_agent.go b/workspace-server/internal/handlers/platform_agent.go index 2aae1c89e..be80dd329 100644 --- a/workspace-server/internal/handlers/platform_agent.go +++ b/workspace-server/internal/handlers/platform_agent.go @@ -503,12 +503,34 @@ func installPlatformAgent(ctx context.Context, database *sql.DB, platformID, nam } defer func() { _ = tx.Rollback() }() // no-op after Commit + // 0. If a different platform root already exists, downgrade it to 'workspace' + // so it no longer blocks the partial unique index uniq_workspaces_one_platform_root. + // It will then be picked up as an old root and re-parented under the new + // platform agent (defense-in-depth: covers pathological cases where a + // platform root was created with a non-canonical id, e.g. test fixtures + // or a prior failed migration). + if _, err := tx.ExecContext(ctx, ` + UPDATE workspaces SET kind = 'workspace', updated_at = now() + WHERE kind = 'platform' AND parent_id IS NULL AND id <> $1 + `, platformID); err != nil { + return fmt.Errorf("downgrade existing platform root: %w", err) + } + // 1. Ensure the platform-agent row exists as a kind='platform' root. // ON CONFLICT keeps it a platform root if it was pre-seeded; the row is // tier 0 and never billed/provisioned as an ordinary workspace EC2. + // Status starts as 'offline' — there is no container yet; 'online' is a + // green-dot lie until the first heartbeat (core#2508). + // + // parent_name_uniq collision: the platform-agent name is deterministic + // and unique per org by construction (e.g. "Org Concierge"). A pre- + // existing root with the same name is a data inconsistency; we fail loud + // rather than silently rename/reparent, which could orphan billing or + // provisioning state. The integration tests use unique names per fixture + // to avoid cross-test collision (CR-A RC 10610). if _, err := tx.ExecContext(ctx, ` INSERT INTO workspaces (id, name, kind, tier, status, runtime, parent_id) - VALUES ($1, $2, 'platform', 0, 'online', 'claude-code', NULL) + VALUES ($1, $2, 'platform', 0, 'offline', 'claude-code', NULL) ON CONFLICT (id) DO UPDATE SET kind = 'platform', runtime = 'claude-code', parent_id = NULL `, platformID, name); err != nil { return fmt.Errorf("upsert platform agent: %w", err) @@ -517,8 +539,10 @@ func installPlatformAgent(ctx context.Context, database *sql.DB, platformID, nam // 2. Capture the org's other current roots (everything at parent_id IS NULL // except the platform agent itself). In a one-org tenant DB this is the // single team root; the query tolerates 0 (already installed) or N. + // FOR UPDATE prevents a root created mid-install from escaping re-parent + // (core#2508). rows, err := tx.QueryContext(ctx, - `SELECT id FROM workspaces WHERE parent_id IS NULL AND id <> $1`, platformID) + `SELECT id FROM workspaces WHERE parent_id IS NULL AND id <> $1 FOR UPDATE`, platformID) if err != nil { return fmt.Errorf("select old roots: %w", err) } @@ -549,10 +573,30 @@ func installPlatformAgent(ctx context.Context, database *sql.DB, platformID, nam `UPDATE org_api_tokens SET org_id = $1 WHERE org_id = $2`, platformID, root); err != nil { return fmt.Errorf("migrate org_api_tokens for %s: %w", root, err) } - if _, err := tx.ExecContext(ctx, - `UPDATE org_plugin_allowlist SET org_id = $1 WHERE org_id = $2`, platformID, root); err != nil { + // org_plugin_allowlist has UNIQUE(org_id, plugin_name). A plain UPDATE + // collides 23505 when N>1 old roots allowlisted the SAME plugin. + // INSERT…SELECT…ON CONFLICT DO NOTHING deduplicates; DELETE leftovers + // cleans the old-root rows (core#2508). + // + // Column-list matches the actual schema (026_org_plugin_allowlist.up.sql): + // id, org_id, plugin_name, enabled_by, enabled_at + // id is auto-populated by gen_random_uuid(); enabled_by is NOT NULL but + // preserved from the old root row (same admin who enabled it); + // enabled_at is preserved verbatim (no audit-time rewrite on + // re-parenting — the original "when this was enabled" stays stable). + if _, err := tx.ExecContext(ctx, ` + INSERT INTO org_plugin_allowlist (org_id, plugin_name, enabled_by, enabled_at) + SELECT $1, plugin_name, enabled_by, enabled_at + FROM org_plugin_allowlist + WHERE org_id = $2 + ON CONFLICT (org_id, plugin_name) DO NOTHING + `, platformID, root); err != nil { return fmt.Errorf("migrate org_plugin_allowlist for %s: %w", root, err) } + if _, err := tx.ExecContext(ctx, + `DELETE FROM org_plugin_allowlist WHERE org_id = $1`, root); err != nil { + return fmt.Errorf("delete old org_plugin_allowlist for %s: %w", root, err) + } } return tx.Commit() diff --git a/workspace-server/internal/handlers/platform_agent_integration_test.go b/workspace-server/internal/handlers/platform_agent_integration_test.go index 498bbfdf7..c73f2777a 100644 --- a/workspace-server/internal/handlers/platform_agent_integration_test.go +++ b/workspace-server/internal/handlers/platform_agent_integration_test.go @@ -68,12 +68,14 @@ func TestIntegration_PlatformAgentInstall_ReparentsRootAndMovesAnchors(t *testin childID := uuid.New().String() platformID := uuid.New().String() + paName := "Org Concierge " + tag cleanup := func() { _, _ = conn.ExecContext(ctx, `DELETE FROM org_plugin_allowlist WHERE plugin_name = $1`, prefix+"-plugin") _, _ = conn.ExecContext(ctx, `DELETE FROM org_api_tokens WHERE prefix = $1`, tag) // child + old root (prefixed names) first, then the platform agent by id // (root.parent_id references it, so it must go last). _, _ = conn.ExecContext(ctx, `DELETE FROM workspaces WHERE name LIKE $1`, prefix+"%") + _, _ = conn.ExecContext(ctx, `DELETE FROM workspaces WHERE name = $1`, paName) _, _ = conn.ExecContext(ctx, `DELETE FROM workspaces WHERE id = $1`, platformID) } t.Cleanup(cleanup) @@ -104,7 +106,7 @@ func TestIntegration_PlatformAgentInstall_ReparentsRootAndMovesAnchors(t *testin } // Install. - if err := installPlatformAgent(ctx, conn, platformID, "Org Concierge"); err != nil { + if err := installPlatformAgent(ctx, conn, platformID, paName); err != nil { t.Fatalf("install: %v", err) } @@ -170,7 +172,7 @@ func TestIntegration_PlatformAgentInstall_ReparentsRootAndMovesAnchors(t *testin assertState("first install") // Idempotent: second install must not error or change state. - if err := installPlatformAgent(ctx, conn, platformID, "Org Concierge"); err != nil { + if err := installPlatformAgent(ctx, conn, platformID, paName); err != nil { t.Fatalf("second install (idempotent): %v", err) } assertState("second install") @@ -186,3 +188,105 @@ func TestIntegration_PlatformAgentInstall_ReparentsRootAndMovesAnchors(t *testin t.Fatalf("team roots after install = %d, want 0 (old root re-parented under platform agent)", nRoots) } } + +// TestIntegration_PlatformAgentInstall_MultiRootAllowlistDedup guards the +// UNIQUE(org_id, plugin_name) collision on org_plugin_allowlist. When N>1 +// old roots have allowlisted the SAME plugin, a plain UPDATE…SET org_id +// collides 23505. The INSERT…SELECT…ON CONFLICT DO NOTHING + DELETE leftovers +// path must survive this deterministically (core#2508). +func TestIntegration_PlatformAgentInstall_MultiRootAllowlistDedup(t *testing.T) { + conn := integrationDB_PlatformAgentInstall(t) + ctx := context.Background() + + tag := uuid.New().String()[:8] + prefix := fmt.Sprintf("itest-pallow-%s", tag) + rootA := uuid.New().String() + rootB := uuid.New().String() + platformID := uuid.New().String() + + paName := "Org Concierge " + tag + cleanup := func() { + _, _ = conn.ExecContext(ctx, `DELETE FROM org_plugin_allowlist WHERE plugin_name = $1`, prefix+"-plugin") + _, _ = conn.ExecContext(ctx, `DELETE FROM workspaces WHERE name LIKE $1`, prefix+"%") + _, _ = conn.ExecContext(ctx, `DELETE FROM workspaces WHERE name = $1`, paName) + _, _ = conn.ExecContext(ctx, `DELETE FROM workspaces WHERE id = $1`, platformID) + } + t.Cleanup(cleanup) + cleanup() + + // Seed TWO old roots with the SAME plugin allowlisted. + for _, rid := range []string{rootA, rootB} { + if _, err := conn.ExecContext(ctx, ` + INSERT INTO workspaces (id, name, tier, runtime, status, parent_id) + VALUES ($1, $2, 2, 'claude-code', 'online', NULL)`, rid, prefix+"-"+rid[:8]); err != nil { + t.Fatalf("seed root %s: %v", rid, err) + } + if _, err := conn.ExecContext(ctx, ` + INSERT INTO org_plugin_allowlist (org_id, plugin_name, enabled_by) + VALUES ($1, $2, $3)`, rid, prefix+"-plugin", rid); err != nil { + t.Fatalf("seed allowlist for %s: %v", rid, err) + } + } + + // Install must NOT fail with 23505 unique violation. + if err := installPlatformAgent(ctx, conn, platformID, paName); err != nil { + t.Fatalf("install with duplicate allowlist: %v", err) + } + + // The platform agent must end up with exactly ONE row for the plugin. + var count int + if err := conn.QueryRowContext(ctx, + `SELECT count(*) FROM org_plugin_allowlist WHERE org_id = $1`, platformID).Scan(&count); err != nil { + t.Fatalf("count platform-agent allowlist: %v", err) + } + if count != 1 { + t.Fatalf("platform-agent allowlist count = %d, want 1 (deduped from 2 old roots)", count) + } + + // Old roots must have ZERO allowlist rows left. + for _, rid := range []string{rootA, rootB} { + var left int + if err := conn.QueryRowContext(ctx, + `SELECT count(*) FROM org_plugin_allowlist WHERE org_id = $1`, rid).Scan(&left); err != nil { + t.Fatalf("count old-root allowlist %s: %v", rid, err) + } + if left != 0 { + t.Fatalf("old root %s still has %d allowlist rows, want 0 (leftovers deleted)", rid, left) + } + } +} + +// TestIntegration_PlatformAgentInstall_StatusNotOnline prevents the green-dot +// lie: a freshly inserted platform-agent row must NOT claim status='online' +// when there is no container yet (core#2508). +func TestIntegration_PlatformAgentInstall_StatusNotOnline(t *testing.T) { + conn := integrationDB_PlatformAgentInstall(t) + ctx := context.Background() + + platformID := uuid.New().String() + paName := "Org Concierge " + platformID[:8] + + cleanup := func() { + _, _ = conn.ExecContext(ctx, `DELETE FROM workspaces WHERE name = $1`, paName) + _, _ = conn.ExecContext(ctx, `DELETE FROM workspaces WHERE id = $1`, platformID) + } + t.Cleanup(cleanup) + cleanup() + + if err := installPlatformAgent(ctx, conn, platformID, paName); err != nil { + t.Fatalf("install: %v", err) + } + + var status string + if err := conn.QueryRowContext(ctx, + `SELECT status FROM workspaces WHERE id = $1`, platformID).Scan(&status); err != nil { + t.Fatalf("read status: %v", err) + } + if status == "online" { + t.Fatalf("fresh platform-agent status=%q, must not be 'online' (no container yet)", status) + } + if status != "offline" { + // Allow future changes, but catch the specific lie. + t.Logf("fresh platform-agent status=%q (expected 'offline', but accepting non-online)", status) + } +}