diff --git a/workspace-server/internal/handlers/platform_agent_integration_test.go b/workspace-server/internal/handlers/platform_agent_integration_test.go index c73f2777a..c83bf0ed8 100644 --- a/workspace-server/internal/handlers/platform_agent_integration_test.go +++ b/workspace-server/internal/handlers/platform_agent_integration_test.go @@ -290,3 +290,152 @@ func TestIntegration_PlatformAgentInstall_StatusNotOnline(t *testing.T) { t.Logf("fresh platform-agent status=%q (expected 'offline', but accepting non-online)", status) } } + +// TestIntegration_PlatformAgentInstall_OnConflictUpdatesRuntime locks in +// the core#2496 root-fix. Pre-#2496, the ON CONFLICT clause on the +// platform-agent upsert only set kind and parent_id; the runtime column +// was left at its INSERT-time value (the schema default). So if a +// platform-agent row had been self-registered first (creating a row with +// the schema default runtime) and the install endpoint was then called, +// the runtime would be left STALE — not updated to 'claude-code'. +// +// #2496 added `runtime = 'claude-code'` to the ON CONFLICT DO UPDATE +// SET clause, so the runtime is now corrected on every install, not just +// on the initial insert. +// +// This test seeds a platform-agent row with a NON-canonical runtime +// ('codex' — anything other than 'claude-code'), then calls +// installPlatformAgent. The post-install assertion is: the runtime +// MUST be 'claude-code'. Pre-#2496, this assertion would fail; post-#2496, +// the ON CONFLICT clause resets it. +// +// We also test the "fresh insert" path (no prior row) to confirm the +// regression is specifically about the conflict-path; if a future refactor +// accidentally drops the runtime from the INSERT VALUES too, that case +// catches it. +func TestIntegration_PlatformAgentInstall_OnConflictUpdatesRuntime(t *testing.T) { + conn := integrationDB_PlatformAgentInstall(t) + ctx := context.Background() + + tag := uuid.New().String()[:8] + platformID := uuid.New().String() + paName := "Org Concierge " + tag + staleRuntime := "codex" // deliberately non-canonical + + // The schema has `uniq_workspaces_one_platform_root` (UNIQUE + // partial index allowing at most one row with kind='platform' AND + // parent_id IS NULL per database). A prior sibling test in this + // package (e.g. TestIntegration_PlatformAgentInstall_ReparentsRoot + // AndMovesAnchors) leaves a platform-agent row behind; its cleanup + // deletes by its OWN uuid+name, so by the time this test runs, + // the slot may be occupied by a prior test's row (depending on + // test scheduling). + // + // To make this test robust to test ordering, the pre-seed + // cleanup DOWNGRADES any existing platform root to 'workspace' + // (matching what installPlatformAgent itself does for prior + // roots — see its step 0). We use UPDATE not DELETE because + // child workspaces may have `parent_id` pointing at the platform + // root, and a DELETE would trip `workspaces_parent_id_fkey`. + // Downgrading (UPDATE kind='workspace') is FK-safe and clears + // the uniq-platform-root slot. + cleanup := func() { + _, _ = conn.ExecContext(ctx, `DELETE FROM workspaces WHERE name = $1`, paName) + _, _ = conn.ExecContext(ctx, `DELETE FROM workspaces WHERE id = $1`, platformID) + // Belt-and-suspenders: also downgrade any other platform-agent + // row left by a sibling test, so the next test's seed slot + // is free. + _, _ = conn.ExecContext(ctx, `UPDATE workspaces SET kind = 'workspace' WHERE kind = 'platform' AND parent_id IS NULL AND id <> $1`, platformID) + } + t.Cleanup(cleanup) + // Pre-seed: clear the uniq-platform-root slot by downgrading + // any existing platform root to 'workspace'. This is FK-safe + // and matches installPlatformAgent's own step 0 (so we're + // exercising the same shape the production code uses). + if _, err := conn.ExecContext(ctx, + `UPDATE workspaces SET kind = 'workspace' WHERE kind = 'platform' AND parent_id IS NULL`); err != nil { + t.Fatalf("pre-seed: downgrade existing platform-agent rows: %v", err) + } + cleanup() + + // Case 1: ON CONFLICT path. Seed the platform-agent row with a STALE + // runtime (simulates a self-registered row with the schema default). + // The seed is INSERT (not UPSERT) to deliberately NOT use the install + // endpoint's path — we're pre-arranging the conflict scenario. + if _, err := conn.ExecContext(ctx, ` + INSERT INTO workspaces (id, name, kind, tier, status, runtime, parent_id) + VALUES ($1, $2, 'platform', 0, 'offline', $3, NULL)`, + platformID, paName, staleRuntime); err != nil { + t.Fatalf("seed stale platform agent: %v", err) + } + + // Sanity: the seeded row has the stale runtime (not the canonical + // 'claude-code'). If this assertion fails, the seed is wrong; fix + // the seed, not the test assertion. + var seededRuntime string + if err := conn.QueryRowContext(ctx, + `SELECT runtime FROM workspaces WHERE id = $1`, platformID).Scan(&seededRuntime); err != nil { + t.Fatalf("read seeded runtime: %v", err) + } + if seededRuntime != staleRuntime { + t.Fatalf("seed precondition: runtime=%q, want %q (the test would be meaningless if the seed already had 'claude-code')", + seededRuntime, staleRuntime) + } + + // Now call installPlatformAgent. Per the pre-#2496 bug, the + // ON CONFLICT DO UPDATE SET clause would have left runtime stuck + // at 'codex'. Per the #2496 fix, it must now be 'claude-code'. + if err := installPlatformAgent(ctx, conn, platformID, paName); err != nil { + t.Fatalf("install: %v", err) + } + + // PRIMARY ASSERTION: the runtime is 'claude-code' after install. + // Pre-#2496, the runtime would be 'codex' (stale). Post-#2496, the + // ON CONFLICT clause sets it to 'claude-code'. + var postInstallRuntime string + if err := conn.QueryRowContext(ctx, + `SELECT runtime FROM workspaces WHERE id = $1`, platformID).Scan(&postInstallRuntime); err != nil { + t.Fatalf("read post-install runtime: %v", err) + } + if postInstallRuntime != "claude-code" { + t.Fatalf("core#2496 regression: post-install runtime=%q, want 'claude-code'. "+ + "This means the ON CONFLICT DO UPDATE SET clause is NOT setting runtime on the conflict path. "+ + "Pre-#2496, the clause was: `ON CONFLICT (id) DO UPDATE SET kind = 'platform', parent_id = NULL` "+ + "(no runtime update). Post-#2496, it MUST be: `... SET kind = 'platform', runtime = 'claude-code', parent_id = NULL`.", + postInstallRuntime) + } + + // Sanity: a second install (still the conflict path) keeps runtime + // at 'claude-code' (idempotency assertion). + if err := installPlatformAgent(ctx, conn, platformID, paName); err != nil { + t.Fatalf("second install: %v", err) + } + var post2Runtime string + if err := conn.QueryRowContext(ctx, + `SELECT runtime FROM workspaces WHERE id = $1`, platformID).Scan(&post2Runtime); err != nil { + t.Fatalf("read post-second-install runtime: %v", err) + } + if post2Runtime != "claude-code" { + t.Fatalf("idempotent: post-second-install runtime=%q, want 'claude-code'", post2Runtime) + } + + // Case 2: Fresh insert path. Verify the runtime='claude-code' is + // also set on the INSERT VALUES, not just on conflict. This guards + // against a future refactor that moves the runtime to the conflict + // clause only and removes it from the INSERT — a regression that + // would let a self-registering row skip the runtime entirely. + freshID := uuid.New().String() + freshName := "Org Concierge fresh " + tag + if err := installPlatformAgent(ctx, conn, freshID, freshName); err != nil { + t.Fatalf("fresh install: %v", err) + } + var freshRuntime string + if err := conn.QueryRowContext(ctx, + `SELECT runtime FROM workspaces WHERE id = $1`, freshID).Scan(&freshRuntime); err != nil { + t.Fatalf("read fresh runtime: %v", err) + } + if freshRuntime != "claude-code" { + t.Fatalf("fresh insert path: runtime=%q, want 'claude-code'", freshRuntime) + } + _, _ = conn.ExecContext(ctx, `DELETE FROM workspaces WHERE id = $1`, freshID) +}