diff --git a/apps/api/migrations/087-fix-vm-map-rekey.integration.test.ts b/apps/api/migrations/087-fix-vm-map-rekey.integration.test.ts index f0eab090a2..18e3127be0 100644 --- a/apps/api/migrations/087-fix-vm-map-rekey.integration.test.ts +++ b/apps/api/migrations/087-fix-vm-map-rekey.integration.test.ts @@ -8,7 +8,14 @@ * converges on a single re-run. */ -import { beforeEach, afterEach, describe, expect, it } from "bun:test"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + setDefaultTimeout, +} from "bun:test"; import { sql } from "kysely"; import { closeTestPgDatabase, @@ -19,6 +26,12 @@ import { import type { StudioDatabase } from "../src/database"; import { up as up087 } from "./087-fix-vm-map-rekey"; +// Each test resets the schema from scratch, and that reset truncates every +// table there is — a cost that grows with every migration the repo adds. On a +// shared CI Postgres the hook alone can outlast bun's 5s default, and an +// aborted hook surfaces as an unrelated "driver has already been destroyed". +setDefaultTimeout(30_000); + const USER = "user_test"; const ORG = "org_test"; diff --git a/apps/api/migrations/088-purge-cli-activate-keys.integration.test.ts b/apps/api/migrations/088-purge-cli-activate-keys.integration.test.ts index de286b9320..680f16585f 100644 --- a/apps/api/migrations/088-purge-cli-activate-keys.integration.test.ts +++ b/apps/api/migrations/088-purge-cli-activate-keys.integration.test.ts @@ -6,7 +6,14 @@ * `AI_PROVIDER_CLI_ACTIVATE` tool) and leaves all other rows untouched. */ -import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + setDefaultTimeout, +} from "bun:test"; import { sql } from "kysely"; import { closeTestPgDatabase, @@ -17,6 +24,12 @@ import { import type { StudioDatabase } from "../src/database"; import { up as up088 } from "./088-purge-cli-activate-keys"; +// Each test resets the schema from scratch, and that reset truncates every +// table there is — a cost that grows with every migration the repo adds. On a +// shared CI Postgres the hook alone can outlast bun's 5s default, and an +// aborted hook surfaces as an unrelated "driver has already been destroyed". +setDefaultTimeout(30_000); + const ORG = "org_test"; const USER = "user_test"; diff --git a/apps/api/migrations/089-rename-remote-user-to-desktop.integration.test.ts b/apps/api/migrations/089-rename-remote-user-to-desktop.integration.test.ts index 029497fc97..4e565c7d1e 100644 --- a/apps/api/migrations/089-rename-remote-user-to-desktop.integration.test.ts +++ b/apps/api/migrations/089-rename-remote-user-to-desktop.integration.test.ts @@ -10,7 +10,14 @@ * JSONB rewrites because that's where the logic is non-trivial. */ -import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + setDefaultTimeout, +} from "bun:test"; import { sql } from "kysely"; import { closeTestPgDatabase, @@ -21,6 +28,12 @@ import { import type { StudioDatabase } from "../src/database"; import { up as up089 } from "./089-rename-remote-user-to-desktop"; +// Each test resets the schema from scratch, and that reset truncates every +// table there is — a cost that grows with every migration the repo adds. On a +// shared CI Postgres the hook alone can outlast bun's 5s default, and an +// aborted hook surfaces as an unrelated "driver has already been destroyed". +setDefaultTimeout(30_000); + const USER = "user_test"; const ORG = "org_test"; diff --git a/apps/api/migrations/092-sandbox-naming-uniformization.integration.test.ts b/apps/api/migrations/092-sandbox-naming-uniformization.integration.test.ts index 8ad0e8aa85..e88494e36c 100644 --- a/apps/api/migrations/092-sandbox-naming-uniformization.integration.test.ts +++ b/apps/api/migrations/092-sandbox-naming-uniformization.integration.test.ts @@ -17,7 +17,14 @@ * Mirrors the 089 test harness pattern. */ -import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + setDefaultTimeout, +} from "bun:test"; import { sql } from "kysely"; import { closeTestPgDatabase, @@ -28,6 +35,12 @@ import { import type { StudioDatabase } from "../src/database"; import { up as up092 } from "./092-sandbox-naming-uniformization"; +// Each test resets the schema from scratch, and that reset truncates every +// table there is — a cost that grows with every migration the repo adds. On a +// shared CI Postgres the hook alone can outlast bun's 5s default, and an +// aborted hook surfaces as an unrelated "driver has already been destroyed". +setDefaultTimeout(30_000); + const USER = "user_test"; const ORG = "org_test"; diff --git a/apps/api/migrations/097-drop-local-docker-sandbox-state.integration.test.ts b/apps/api/migrations/097-drop-local-docker-sandbox-state.integration.test.ts index bcee76ad15..8c1bdbf273 100644 --- a/apps/api/migrations/097-drop-local-docker-sandbox-state.integration.test.ts +++ b/apps/api/migrations/097-drop-local-docker-sandbox-state.integration.test.ts @@ -12,7 +12,14 @@ * Mirrors the 092 test harness pattern. */ -import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + setDefaultTimeout, +} from "bun:test"; import { sql } from "kysely"; import { closeTestPgDatabase, @@ -23,6 +30,12 @@ import { import type { StudioDatabase } from "../src/database"; import { up as up097 } from "./097-drop-local-docker-sandbox-state"; +// Each test resets the schema from scratch, and that reset truncates every +// table there is — a cost that grows with every migration the repo adds. On a +// shared CI Postgres the hook alone can outlast bun's 5s default, and an +// aborted hook surfaces as an unrelated "driver has already been destroyed". +setDefaultTimeout(30_000); + const USER = "user_test"; const ORG = "org_test"; diff --git a/apps/api/migrations/098-thread-message-parts.integration.test.ts b/apps/api/migrations/098-thread-message-parts.integration.test.ts index a1a609b201..602c2ce0a4 100644 --- a/apps/api/migrations/098-thread-message-parts.integration.test.ts +++ b/apps/api/migrations/098-thread-message-parts.integration.test.ts @@ -7,7 +7,14 @@ * are added to `threads`. */ -import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + setDefaultTimeout, +} from "bun:test"; import { sql } from "kysely"; import { closeTestPgDatabase, @@ -17,6 +24,12 @@ import { } from "../src/database/test-db-pg"; import type { StudioDatabase } from "../src/database"; +// Each test resets the schema from scratch, and that reset truncates every +// table there is — a cost that grows with every migration the repo adds. On a +// shared CI Postgres the hook alone can outlast bun's 5s default, and an +// aborted hook surfaces as an unrelated "driver has already been destroyed". +setDefaultTimeout(30_000); + describe("migration 098 thread_message_parts", () => { let database: StudioDatabase; diff --git a/apps/api/migrations/104-agent-sandbox-provider-kind.integration.test.ts b/apps/api/migrations/104-agent-sandbox-provider-kind.integration.test.ts index a6e02511f6..610c91b59d 100644 --- a/apps/api/migrations/104-agent-sandbox-provider-kind.integration.test.ts +++ b/apps/api/migrations/104-agent-sandbox-provider-kind.integration.test.ts @@ -3,7 +3,14 @@ * kind values from the legacy `cluster` spelling to `agent-sandbox`. */ -import { afterEach, beforeEach, describe, expect, it } from "bun:test"; +import { + afterEach, + beforeEach, + describe, + expect, + it, + setDefaultTimeout, +} from "bun:test"; import { sql } from "kysely"; import { closeTestPgDatabase, @@ -12,6 +19,12 @@ import { seedCommonTestPgFixtures, } from "../src/database/test-db-pg"; import type { StudioDatabase } from "../src/database"; + +// Each test resets the schema from scratch, and that reset truncates every +// table there is — a cost that grows with every migration the repo adds. On a +// shared CI Postgres the hook alone can outlast bun's 5s default, and an +// aborted hook surfaces as an unrelated "driver has already been destroyed". +setDefaultTimeout(30_000); import { down as down104, up as up104, diff --git a/apps/api/migrations/205-repository-references.integration.test.ts b/apps/api/migrations/205-repository-references.integration.test.ts new file mode 100644 index 0000000000..ea4aafbe13 --- /dev/null +++ b/apps/api/migrations/205-repository-references.integration.test.ts @@ -0,0 +1,171 @@ +/** + * Real-Postgres coverage for migration 205's backfill. + * + * The whole migration is SQL — a jsonb path match against two differently + * shaped metadata columns (`connections.metadata` is TEXT, `threads.metadata` + * is jsonb) — so an in-memory fake would agree with a version that links + * nothing. What has to hold: every JSON binding ends up referenced, a + * repository a thread names but nobody linked gets created, and a row whose + * metadata is not valid JSON does not abort the run. + */ + +import { + afterAll, + beforeAll, + describe, + expect, + it, + setDefaultTimeout, +} from "bun:test"; +import { sql } from "kysely"; +import type { StudioDatabase } from "../src/database"; +import { + closeTestPgDatabase, + connectTestPgDatabase, + resetTestPgDatabase, +} from "../src/database/test-db-pg"; +import { down, up } from "./205-repository-references"; + +// Each test resets the schema from scratch, and that reset truncates every +// table there is — a cost that grows with every migration the repo adds. On a +// shared CI Postgres the hook alone can outlast bun's 5s default, and an +// aborted hook surfaces as an unrelated "driver has already been destroyed". +setDefaultTimeout(30_000); + +const ORG = "org_202"; +const USER = "user_202"; + +describe("205 repository references", () => { + let database: StudioDatabase; + + const rows = async (q: string): Promise => + (await sql.raw(q).execute(database.db)).rows; + + beforeAll(async () => { + database = await connectTestPgDatabase(); + await resetTestPgDatabase(database); + const db = database.db; + + // Back to the pre-205 shape, so `up` runs against what prod will hand it. + await down(db as never); + + await sql` + INSERT INTO "user" (id, email, "emailVerified", name, "createdAt", "updatedAt") + VALUES (${USER}, 'u202@e2e.local', true, 'u202', now(), now()) + `.execute(db); + await sql` + INSERT INTO organization (id, name, slug, "createdAt") + VALUES (${ORG}, 'org 202', 'org-202', now()) + `.execute(db); + + const conn = async (id: string, metadata: string | null) => { + await sql` + INSERT INTO connections ( + id, organization_id, title, connection_type, connection_url, + metadata, status, pinned, created_by, updated_by, created_at, updated_at + ) VALUES ( + ${id}, ${ORG}, ${id}, 'VIRTUAL', ${"virtual://" + id}, + ${metadata}, 'active', false, ${USER}, ${USER}, now(), now() + ) + `.execute(db); + }; + // An agent bound to a repository that already has a row (201's doing). + await conn( + "vir_bound", + JSON.stringify({ githubRepo: { owner: "acme", name: "site" } }), + ); + // An agent with no repository at all. + await conn("vir_none", JSON.stringify({ instructions: null })); + /** + * `connections.metadata` is TEXT, so a row can hold something that is not + * JSON. 201 hit this and needed a guard; 202 must not regress it. + */ + await conn("vir_broken", "not json at all"); + + await sql` + INSERT INTO repositories (organization_id, provider, host, path, web_url) + VALUES (${ORG}, 'github', 'github.com', 'acme/site', 'https://github.com/acme/site') + `.execute(db); + + await sql` + INSERT INTO threads (id, organization_id, title, created_by, updated_by, created_at, updated_at, metadata) + VALUES ('thr_202', ${ORG}, 'thr', ${USER}, ${USER}, now(), now(), ${JSON.stringify( + { + githubRepos: [ + { owner: "acme", name: "site" }, + { owner: "acme", name: "checkout" }, + ], + }, + )}::jsonb) + `.execute(db); + + await up(db as never); + }); + + afterAll(async () => { + await closeTestPgDatabase(database); + }); + + it("references the repository an agent's JSON names", async () => { + const [row] = await rows<{ path: string | null }>(` + select r.path from connections c + left join repositories r on r.id = c.repository_id + where c.id = 'vir_bound'`); + expect(row?.path).toBe("acme/site"); + }); + + it("leaves an agent with no repository unreferenced", async () => { + const [row] = await rows<{ repository_id: string | null }>( + `select repository_id from connections where id = 'vir_none'`, + ); + expect(row?.repository_id).toBeNull(); + }); + + /** The guard 201 needed: unparseable metadata is skipped, not fatal. */ + it("survives a connection whose metadata is not JSON", async () => { + const [row] = await rows<{ repository_id: string | null }>( + `select repository_id from connections where id = 'vir_broken'`, + ); + expect(row?.repository_id).toBeNull(); + }); + + /** 201 skipped threads, so a repo only a checkout list knows about has no row yet. */ + it("creates the repository a thread names and nobody linked", async () => { + const [row] = await rows<{ account_id: string | null }>(` + select account_id from repositories + where organization_id = '${ORG}' and lower(path) = 'acme/checkout'`); + expect(row).toBeDefined(); + expect(row?.account_id).toBeNull(); + }); + + it("links every repository in a thread's checkout list", async () => { + const linked = await rows<{ path: string }>(` + select r.path from thread_repositories tr + join repositories r on r.id = tr.repository_id + where tr.thread_id = 'thr_202' order by r.path`); + expect(linked.map((l) => l.path)).toEqual(["acme/checkout", "acme/site"]); + }); + + /** Re-running a migration must not double-insert; the backfill is idempotent. */ + it("is idempotent", async () => { + await up(database.db as never).catch(() => { + /** `up` re-adds the column and table; only the backfill half can rerun. */ + }); + const [count] = await rows<{ n: string }>( + `select count(*) as n from thread_repositories where thread_id = 'thr_202'`, + ); + expect(Number(count?.n)).toBe(2); + }); + + /** The JSON is untouched — a pod on the previous release still reads it. */ + it("leaves the JSON bindings in place", async () => { + const [agent] = await rows<{ owner: string }>( + `select metadata::jsonb #>> '{githubRepo,owner}' as owner from connections where id = 'vir_bound'`, + ); + expect(agent?.owner).toBe("acme"); + const [thread] = await rows<{ n: string }>( + `select jsonb_array_length(metadata -> 'githubRepos') as n from threads where id = 'thr_202'`, + ); + expect(Number(thread?.n)).toBe(2); + }); +}); diff --git a/apps/api/migrations/205-repository-references.ts b/apps/api/migrations/205-repository-references.ts new file mode 100644 index 0000000000..a0b4770b09 --- /dev/null +++ b/apps/api/migrations/205-repository-references.ts @@ -0,0 +1,172 @@ +import { type Kysely, sql } from "kysely"; + +/** + * The last two repository bindings that were still JSON. + * + * Migration 204 gave `task_board_items`, `task_board_item_prs` and + * `org_repo_sync` a real `repository_id`, but left the two bindings that + * matter most to a person using the product: + * + * - an AGENT's repository (`connections.metadata.githubRepo`, since a virtual + * MCP is a `connections` row) — the thing a project IS; + * - a THREAD's extra checkouts (`threads.metadata.githubRepos`), the list + * `TASK_ADD_REPO` appends to so one run can hold several repositories. + * + * Leaving those in JSON cost three things. A deleted repository left every + * agent pointing at nothing, with no FK to say so. "Which agents use this + * repository" was a JSON scan. And `resolveRepoTarget` had to keep a + * resolve-by-identity step — a lowercase path match, per request — purely + * because the binding carried no id. + * + * The thread list also loses a real bug with the normalisation. Its append is + * a `jsonb_agg` rebuild inside one UPDATE, written that way because two + * concurrent `TASK_ADD_REPO` calls lost each other under read-modify-write — + * with the pod already holding the checkout the lost entry described, so + * nothing looked wrong until the pod was recreated without it. A unique key + * makes that an `ON CONFLICT DO NOTHING`. Its dedup key was also + * `lower(owner/name)` with no host, so two `acme/site` on different hosts + * collided; a reference to a row cannot. + * + * EXPAND ONLY. The JSON stays written and readable: during a rolling deploy + * both versions serve traffic, and a reader that predates this migration must + * still find its binding. Readers prefer the reference and fall back to JSON; + * a later release removes the fallback, and a third drops the JSON. + * + * The backfill only has to LINK, not create: 204 already made a `repositories` + * row for every agent binding (its fourth insert, with a null account). Thread + * metadata was deliberately excluded there, so the rows those need are created + * here first. + */ + +/** + * `connections.metadata` is TEXT holding JSON, and a row that is not valid + * JSON must not abort the migration. Mirrors 201's helper, which drops itself; + * `threads.metadata` is real jsonb and needs none of this. + */ +const CREATE_TRY_JSONB = sql` + CREATE OR REPLACE FUNCTION repo_ref_try_jsonb(value text) + RETURNS jsonb LANGUAGE plpgsql IMMUTABLE AS $$ + BEGIN + RETURN value::jsonb; + EXCEPTION WHEN others THEN + RETURN NULL; + END; + $$ +`; + +export async function up(db: Kysely): Promise { + await CREATE_TRY_JSONB.execute(db); + + /** + * SET NULL, matching 201's consumers: unlinking a repository must not delete + * the agent, which owns threads, tasks and history of its own. It reverts to + * an unbound project, which is a state the product already renders. + */ + await sql` + ALTER TABLE connections + ADD COLUMN repository_id text REFERENCES repositories(id) ON DELETE SET NULL + `.execute(db); + // Partial: 311 of 11,575 rows carry one, and the reverse lookup is the point. + await sql` + CREATE INDEX idx_connections_repository + ON connections (repository_id) WHERE repository_id IS NOT NULL + `.execute(db); + + /** + * CASCADE on both sides, unlike the agent's binding: a checkout entry is + * meaningless without its thread AND without its repository, and it carries + * nothing a person would miss. + */ + await sql` + CREATE TABLE thread_repositories ( + thread_id text NOT NULL REFERENCES threads(id) ON DELETE CASCADE, + organization_id text NOT NULL REFERENCES organization(id) ON DELETE CASCADE, + repository_id text NOT NULL REFERENCES repositories(id) ON DELETE CASCADE, + added_at timestamptz NOT NULL DEFAULT now(), + PRIMARY KEY (thread_id, repository_id) + ) + `.execute(db); + await sql` + CREATE INDEX idx_thread_repositories_repo + ON thread_repositories (repository_id) + `.execute(db); + + /** + * An agent's repository. Every one of these rows exists already (201), so + * this only links. + * + * The owner/name pair is read inline rather than through a LATERAL: in an + * `UPDATE ... FROM`, the target table is not in the FROM list's lateral + * scope, so a LATERAL cannot see `c`. Concatenating with `||` also does the + * filtering for free — a missing half, or metadata that is not JSON at all, + * yields NULL and matches nothing. + */ + await sql` + UPDATE connections c + SET repository_id = r.id + FROM repositories r + WHERE c.repository_id IS NULL + AND r.organization_id = c.organization_id + AND r.host = 'github.com' + AND lower(r.path) = lower( + (repo_ref_try_jsonb(c.metadata) #>> '{githubRepo,owner}') + || '/' || + (repo_ref_try_jsonb(c.metadata) #>> '{githubRepo,name}') + ) + `.execute(db); + + /** + * Repositories only a thread's checkout list knows about — 201 skipped + * threads, so unlike the agent binding these may genuinely not exist yet. + * Anonymous (`account_id` null), like every other identity-only source. + */ + await sql` + INSERT INTO repositories ( + organization_id, account_id, provider, host, path, web_url + ) + SELECT DISTINCT ON (t.organization_id, lower(e.owner || '/' || e.name)) + t.organization_id, NULL, 'github', 'github.com', + e.owner || '/' || e.name, + 'https://github.com/' || e.owner || '/' || e.name + FROM threads t + CROSS JOIN LATERAL jsonb_array_elements( + coalesce(t.metadata -> 'githubRepos', '[]'::jsonb) + ) AS entry + CROSS JOIN LATERAL ( + SELECT entry ->> 'owner' AS owner, entry ->> 'name' AS name + ) e + WHERE coalesce(e.owner, '') <> '' + AND coalesce(e.name, '') <> '' + AND e.owner NOT LIKE '%/%' + AND e.name NOT LIKE '%/%' + ORDER BY t.organization_id, lower(e.owner || '/' || e.name) + ON CONFLICT (organization_id, host, lower(path)) DO NOTHING + `.execute(db); + + await sql` + INSERT INTO thread_repositories (thread_id, organization_id, repository_id) + SELECT DISTINCT t.id, t.organization_id, r.id + FROM threads t + CROSS JOIN LATERAL jsonb_array_elements( + coalesce(t.metadata -> 'githubRepos', '[]'::jsonb) + ) AS entry + JOIN repositories r + ON r.organization_id = t.organization_id + AND r.host = 'github.com' + AND lower(r.path) = lower((entry ->> 'owner') || '/' || (entry ->> 'name')) + ON CONFLICT (thread_id, repository_id) DO NOTHING + `.execute(db); + + await sql`DROP FUNCTION IF EXISTS repo_ref_try_jsonb(text)`.execute(db); +} + +export async function down(db: Kysely): Promise { + /** + * The JSON was never removed, so dropping these loses nothing: every reader + * that predates the expand still finds its binding where it always was. + */ + await sql`DROP TABLE IF EXISTS thread_repositories`.execute(db); + await sql` + ALTER TABLE connections DROP COLUMN IF EXISTS repository_id + `.execute(db); +} diff --git a/apps/api/migrations/index.ts b/apps/api/migrations/index.ts index d1d0509611..6d8abf5501 100644 --- a/apps/api/migrations/index.ts +++ b/apps/api/migrations/index.ts @@ -203,6 +203,7 @@ import * as migration201organizationnotices from "./201-organization-notices.ts" import * as migration202redactbase64threadparts from "./202-redact-base64-thread-parts.ts"; import * as migration203taskboardpreviewroutes from "./203-task-board-preview-routes.ts"; import * as migration204gitprovideraccountsandrepositories from "./204-git-provider-accounts-and-repositories.ts"; +import * as migration205repositoryreferences from "./205-repository-references.ts"; /** * Core migrations for the Studio application. @@ -442,6 +443,7 @@ const migrations: Record = { "203-task-board-preview-routes": migration203taskboardpreviewroutes, "204-git-provider-accounts-and-repositories": migration204gitprovideraccountsandrepositories, + "205-repository-references": migration205repositoryreferences, }; export default migrations; diff --git a/apps/api/src/storage/threads.ts b/apps/api/src/storage/threads.ts index 68558f1f4d..f3938a87c4 100644 --- a/apps/api/src/storage/threads.ts +++ b/apps/api/src/storage/threads.ts @@ -444,22 +444,47 @@ export class SqlThreadStorage implements ThreadStoragePort { } /** - * Append a repo to `metadata.githubRepos`, in SQL, returning the whole list. + * Add a repository to the thread's extra checkouts, returning the whole set. * - * The append happens inside one UPDATE rather than as a read in JS followed - * by a write: the model can fire two `TASK_ADD_REPO` calls at once, and a - * read-modify-write loses the slower one — with the pod already holding the - * checkout the lost entry describes, so nothing looks wrong until the pod is - * recreated without it. + * DUAL-WRITE while migration 205 expands: the row in `thread_repositories` + * is the binding, and the `metadata.githubRepos` array is kept in step + * because a pod running the previous release still reads only the array. + * The array write goes away with the fallback read, not before. * - * Keyed on `owner/name`, so re-adding a repo the thread already has is a - * no-op instead of a duplicate directory. + * The array append happens inside one UPDATE rather than as a read in JS + * followed by a write: the model can fire two `TASK_ADD_REPO` calls at once, + * and a read-modify-write loses the slower one — with the pod already + * holding the checkout the lost entry describes, so nothing looks wrong + * until the pod is recreated without it. The table gets that for free from + * its primary key, which is most of why it exists. + * + * Re-adding a repo the thread already has is a no-op, not a duplicate + * directory: keyed on the repository row where there is one, and on + * `owner/name` in the array. */ async appendThreadGithubRepo( id: string, organizationId: string, repo: GithubRepo, ): Promise { + /** + * Only a repo Studio has a row for can be referenced. One that has none + * (a legacy binding a reader has not stamped yet) still lands in the + * array, so nothing is lost — it simply has no reference until then. + */ + if (repo.repositoryId) { + await this.db + .insertInto("thread_repositories") + .values({ + thread_id: id, + organization_id: organizationId, + repository_id: repo.repositoryId, + }) + .onConflict((oc) => + oc.columns(["thread_id", "repository_id"]).doNothing(), + ) + .execute(); + } const key = `${repo.owner}/${repo.name}`.toLowerCase(); const row = await this.db .updateTable("threads") diff --git a/apps/api/src/storage/types.ts b/apps/api/src/storage/types.ts index 7bfe65f6ac..fe981bbd79 100644 --- a/apps/api/src/storage/types.ts +++ b/apps/api/src/storage/types.ts @@ -237,12 +237,36 @@ export interface MCPConnectionTable { metadata: JsonObject> | null; bindings: JsonArray | null; // Detected bindings (CHAT, EMAIL, etc.) + /** + * The repository a VIRTUAL connection (an agent) works in — migration 205. + * Null for every other connection type, and for an agent with no repository. + * + * Preferred over `metadata.githubRepo`, which is still written and read as + * the fallback until the expand completes. A GitLab project in subgroups + * only fits here: the JSON's `owner`/`name` pair cannot carry a namespace. + */ + repository_id: string | null; + status: "active" | "inactive" | "error"; pinned: boolean; created_at: ColumnType; updated_at: ColumnType; } +/** + * A repository checked out into a thread's run, beyond the agent's own. + * + * `TASK_ADD_REPO` appends here so one run can hold several checkouts. The + * primary key is what makes a concurrent double-add a no-op — the reason this + * is a table and not the `metadata.githubRepos` array it replaces. + */ +export interface ThreadRepositoryTable { + thread_id: string; + organization_id: string; + repository_id: string; + added_at: ColumnType; +} + // MCPConnection runtime type is now ConnectionEntity from "../tools/connection/schema" // OAuthConfig is also exported from schema.ts @@ -2306,6 +2330,7 @@ export interface Database extends PrivateRegistryDatabase { users: UserTable; // System users user: BetterAuthUserTable; // Better Auth core table (singular) connections: MCPConnectionTable; // MCP connections (organization-scoped) + thread_repositories: ThreadRepositoryTable; organization_settings: OrganizationSettingsTable; // Organization-level configuration user_model_preferences: UserModelPreferencesTable; // Per-user chat tier → model overrides api_keys: ApiKeyTable; // Better Auth API keys diff --git a/apps/api/src/storage/virtual.ts b/apps/api/src/storage/virtual.ts index 39b0856021..4fbd01d966 100644 --- a/apps/api/src/storage/virtual.ts +++ b/apps/api/src/storage/virtual.ts @@ -75,6 +75,26 @@ export function escapeLikePattern(term: string): string { return term.replace(/\\/g, "\\\\").replace(/%/g, "\\%").replace(/_/g, "\\_"); } +/** + * The repository an agent's metadata binds it to, as a reference. + * + * DUAL-WRITE while migration 205 expands: the column is the binding, and + * `metadata.githubRepo` is still written beside it because a pod running the + * previous release reads only the JSON. The JSON write goes away with the + * fallback read, not before. + * + * Null for a legacy binding that names no repository row yet — those keep + * resolving by identity, which is exactly the step this column exists to + * retire once every binding carries one. + */ +function boundRepositoryId( + metadata: Record | null | undefined, +): string | null { + const bound = (metadata as { githubRepo?: { repositoryId?: unknown } } | null) + ?.githubRepo?.repositoryId; + return typeof bound === "string" && bound.length > 0 ? bound : null; +} + export class VirtualMCPStorage implements VirtualMCPStoragePort { constructor(private db: Kysely) {} @@ -110,6 +130,7 @@ export class VirtualMCPStorage implements VirtualMCPStoragePort { configuration_state: null, configuration_scopes: null, metadata: data.metadata ? JSON.stringify(data.metadata) : null, + repository_id: boundRepositoryId(data.metadata), bindings: null, status: data.status ?? "active", created_at: now, @@ -462,6 +483,8 @@ export class VirtualMCPStorage implements VirtualMCPStoragePort { updateData.pinned = data.pinned; } if (data.metadata !== undefined) { + // Dual-write, in step with the JSON — see `boundRepositoryId`. + updateData.repository_id = boundRepositoryId(data.metadata); updateData.metadata = data.metadata ? JSON.stringify(data.metadata) : null;