feat(db): CMP-1 persist durable custom agents - #67
Conversation
6766ef7 to
a6a2b45
Compare
a6a2b45 to
2a3b167
Compare
There was a problem hiding this comment.
14 issues found across 48 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/prisma/migrations/20260803210000_agent_builder_foundation/migration.sql">
<violation number="1" location="packages/db/prisma/migrations/20260803210000_agent_builder_foundation/migration.sql:256">
P1: A definition can be linked to a version belonging to a different agent because this foreign key validates only the version ID, not the `(definition.id, version.agentId)` relationship. That lets `AgentRunsService.runNow` create a run for one agent using another agent's instructions; composite ownership foreign keys (and equivalent checks for trigger/run/action/audit links) would keep these durable records internally consistent.</violation>
</file>
<file name="packages/db/prisma/migrations/20260805220000_conversation_attachments/migration.sql">
<violation number="1" location="packages/db/prisma/migrations/20260805220000_conversation_attachments/migration.sql:66">
P2: Multi-attachment submissions can come back in a different order after this migration: every backfilled attachment gets the same `createdAt`, so the `id` tie-breaker orders by a hash instead of the original attachment order. Preserving `WITH ORDINALITY` through the backfill and ordering by it would keep prompt and transcript attachment order stable.</violation>
<violation number="2" location="packages/db/prisma/migrations/20260805220000_conversation_attachments/migration.sql:72">
P2: A mixed legacy attachment array loses entries that do not have valid base64: the metadata update replaces the whole array with only rows selected by `legacy_attachments`. Merging migrated metadata back into the original array would preserve other attachment metadata while still removing stored base64.</violation>
</file>
<file name="packages/auth/src/signed-in.ts">
<violation number="1" location="packages/auth/src/signed-in.ts:16">
P2: Async sign-in handlers no longer run in registration order, so handlers that depend on earlier initialization can race and produce incorrect sign-in side effects. Retain the previous sequential await semantics unless this API explicitly guarantees that subscribers are independent.</violation>
</file>
<file name="apps/api/src/agent/agent-access.service.ts">
<violation number="1" location="apps/api/src/agent/agent-access.service.ts:36">
P2: A role revocation or demotion can be bypassed for an in-flight mutation: if membership changes after this check returns but before the caller's transaction starts, the old authorized role is still used to update, deploy, pause, archive, restore, or delete the agent. Revalidate the membership and management permission inside the same transaction as the mutation, or otherwise make the authorization check and write atomic.</violation>
</file>
<file name="packages/db/prisma/migrations/20260805140000_single_active_conversation_share/migration.sql">
<violation number="1" location="packages/db/prisma/migrations/20260805140000_single_active_conversation_share/migration.sql:17">
P2: Development startup can fail with schema drift immediately after this migration because the raw partial index is not represented in `schema.prisma`, while `prepare-dev.ts` compares the live database to that schema. The index needs an explicit drift-check/Prisma migration handling strategy so this intentional database-only object is preserved without making every prepared database appear out of sync.</violation>
</file>
<file name="apps/api/src/conversations/conversations.router.ts">
<violation number="1" location="apps/api/src/conversations/conversations.router.ts:47">
P2: A user with an authenticated session can continue listing and mutating builder conversations without a workspace-membership check. The new builder handlers should enforce `assertWorkspaceMember` for the full builder lifecycle, not only resource search and sharing, so stale sessions cannot bypass the workspace access boundary.</violation>
</file>
<file name="packages/db/turbo.json">
<violation number="1" location="packages/db/turbo.json:10">
P2: `dev:prepare` is defined but never wired into the dev flow: no task's `dev` depends on `^dev:prepare` (root `turbo.json` `dev` has no `dependsOn`, and apps' `dev` depend only on `$TURBO_EXTENDS$`), and root `package.json` has no `dev:prepare` script, so `bun run dev` will not run migrations or regenerate the Prisma client before services boot, contradicting the README's "root `bun run dev` runs `dev:prepare` before any service starts." Add `"dev": { "dependsOn": ["^dev:prepare"] ... }` to the root config or otherwise invoke `dev:prepare` from `turbo run dev`.</violation>
</file>
<file name="packages/db/prisma/migrations/20260804210000_agent_builder_artifacts/migration.sql">
<violation number="1" location="packages/db/prisma/migrations/20260804210000_agent_builder_artifacts/migration.sql:18">
P2: Version-scoped artifacts are not protected against duplicate revisions: when `conversationId` is null, repeated writes for the same `versionId`/`path`/`revision` are accepted because `versionId` is absent from the unique key. Separate uniqueness constraints for conversation-scoped and version-scoped artifacts would preserve revision idempotency in both workflows.</violation>
<violation number="2" location="packages/db/prisma/migrations/20260804210000_agent_builder_artifacts/migration.sql:22">
P2: Deleting a builder conversation leaves conversation-only artifacts orphaned in the database, retaining their code content while making it unreachable from the builder API. Conversation removal should explicitly delete these rows (while preserving version-linked artifacts if that is required) instead of relying on `SET NULL`.</violation>
</file>
<file name="apps/api/package.json">
<violation number="1" location="apps/api/package.json:19">
P2: Adding `--preload ./test/setup.ts` makes every test invocation import the `@crm/db` `db` client (setup.ts does `import { db } from "@crm/db"`), and `packages/db/src/client.ts` throws at module load when `DATABASE_URL` is unset. Before this change the pure unit suites (logging, error-formatter, domain, google-*) never loaded the db client and ran without a database; now `bun test`/`test:watch` hard-fail for all suites when `DATABASE_URL` is absent, contradicting the "Database-backed suites require DATABASE_URL" note in the verification. Consider guarding the preload so DB-free unit runs still work (e.g. only disconnect when a client exists, or make setup.ts tolerant of a missing DATABASE_URL).</violation>
</file>
<file name="apps/api/src/conversations/conversation-attachments.controller.ts">
<violation number="1" location="apps/api/src/conversations/conversation-attachments.controller.ts:38">
P2: A revoked share or removed workspace membership can still serve the attachment from the browser cache for up to a year because `private, max-age=31536000, immutable` allows reuse without revalidation. Since attachment authorization is intended to be checked on each fetch, use a non-persistent policy such as `private, no-store` here.</violation>
</file>
<file name="apps/api/src/agent/agent-trigger.service.ts">
<violation number="1" location="apps/api/src/agent/agent-trigger.service.ts:134">
P2: These two new pokes target `/internal/crm/builder-dispatch` and `/internal/crm/agent-dispatch`, but no route with either path exists anywhere in the repo — `apps/agent/agent/channels/crm.ts` only registers `/internal/crm/dispatch` and `/internal/crm/verify-key`. Because `pokeRoute` fire-and-forgets the fetch and only `.catch`es thrown errors, a missing route resolves with a 404 Response and the poke silently no-ops with no log, so queued builder conversations / deployed runs get no dispatch nudge until whatever PR supplies the agent-side route lands. Confirm those endpoints are added in the matching runtime PR (#60) before this ships, or these pokes are dead until then.</violation>
</file>
<file name="packages/db/scripts/prepare-dev.ts">
<violation number="1" location="packages/db/scripts/prepare-dev.ts:26">
P3: The drift===2 message always asserts the schema is ahead of the database, but `prisma migrate diff` returns 2 for a difference in either direction. If the local DB ever ends up ahead (a manually applied change, or a migration deleted from the migrations/ folder after being applied), the message and the suggested fix (create a new migration) are wrong and would lead a dev to generate a spurious migration. Consider detecting the direction, e.g. by also diffing to the migrations state, or wording the message to cover both directions.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| CREATE INDEX "agentConversation_agentId_lastMessageAt_idx" ON "agentConversation"("agentId", "lastMessageAt"); | ||
|
|
||
| ALTER TABLE "agentDefinition" ADD CONSTRAINT "agentDefinition_createdById_fkey" FOREIGN KEY ("createdById") REFERENCES "user"("id") ON DELETE RESTRICT ON UPDATE CASCADE; | ||
| ALTER TABLE "agentDefinition" ADD CONSTRAINT "agentDefinition_currentVersionId_fkey" FOREIGN KEY ("currentVersionId") REFERENCES "agentVersion"("id") ON DELETE SET NULL ON UPDATE CASCADE; |
There was a problem hiding this comment.
P1: A definition can be linked to a version belonging to a different agent because this foreign key validates only the version ID, not the (definition.id, version.agentId) relationship. That lets AgentRunsService.runNow create a run for one agent using another agent's instructions; composite ownership foreign keys (and equivalent checks for trigger/run/action/audit links) would keep these durable records internally consistent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/migrations/20260803210000_agent_builder_foundation/migration.sql, line 256:
<comment>A definition can be linked to a version belonging to a different agent because this foreign key validates only the version ID, not the `(definition.id, version.agentId)` relationship. That lets `AgentRunsService.runNow` create a run for one agent using another agent's instructions; composite ownership foreign keys (and equivalent checks for trigger/run/action/audit links) would keep these durable records internally consistent.</comment>
<file context>
@@ -0,0 +1,290 @@
+CREATE INDEX "agentConversation_agentId_lastMessageAt_idx" ON "agentConversation"("agentId", "lastMessageAt");
+
+ALTER TABLE "agentDefinition" ADD CONSTRAINT "agentDefinition_createdById_fkey" FOREIGN KEY ("createdById") REFERENCES "user"("id") ON DELETE RESTRICT ON UPDATE CASCADE;
+ALTER TABLE "agentDefinition" ADD CONSTRAINT "agentDefinition_currentVersionId_fkey" FOREIGN KEY ("currentVersionId") REFERENCES "agentVersion"("id") ON DELETE SET NULL ON UPDATE CASCADE;
+
+ALTER TABLE "agentVersion" ADD CONSTRAINT "agentVersion_agentId_fkey" FOREIGN KEY ("agentId") REFERENCES "agentDefinition"("id") ON DELETE RESTRICT ON UPDATE CASCADE;
</file context>
| 'type', attachment."mediaType", | ||
| 'size', attachment.size | ||
| ) | ||
| ORDER BY attachment."createdAt", attachment.id |
There was a problem hiding this comment.
P2: Multi-attachment submissions can come back in a different order after this migration: every backfilled attachment gets the same createdAt, so the id tie-breaker orders by a hash instead of the original attachment order. Preserving WITH ORDINALITY through the backfill and ordering by it would keep prompt and transcript attachment order stable.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/migrations/20260805220000_conversation_attachments/migration.sql, line 66:
<comment>Multi-attachment submissions can come back in a different order after this migration: every backfilled attachment gets the same `createdAt`, so the `id` tie-breaker orders by a hash instead of the original attachment order. Preserving `WITH ORDINALITY` through the backfill and ordering by it would keep prompt and transcript attachment order stable.</comment>
<file context>
@@ -0,0 +1,86 @@
+ 'type', attachment."mediaType",
+ 'size', attachment.size
+ )
+ ORDER BY attachment."createdAt", attachment.id
+ ) AS attachments
+ FROM "agentConversationAttachment" AS attachment
</file context>
| GROUP BY attachment."submissionId" | ||
| ) | ||
| UPDATE "agentConversationSubmission" AS submission | ||
| SET message = jsonb_set(submission.message, '{attachments}', metadata.attachments) |
There was a problem hiding this comment.
P2: A mixed legacy attachment array loses entries that do not have valid base64: the metadata update replaces the whole array with only rows selected by legacy_attachments. Merging migrated metadata back into the original array would preserve other attachment metadata while still removing stored base64.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/migrations/20260805220000_conversation_attachments/migration.sql, line 72:
<comment>A mixed legacy attachment array loses entries that do not have valid base64: the metadata update replaces the whole array with only rows selected by `legacy_attachments`. Merging migrated metadata back into the original array would preserve other attachment metadata while still removing stored base64.</comment>
<file context>
@@ -0,0 +1,86 @@
+ GROUP BY attachment."submissionId"
+)
+UPDATE "agentConversationSubmission" AS submission
+SET message = jsonb_set(submission.message, '{attachments}', metadata.attachments)
+FROM attachment_metadata AS metadata
+WHERE submission.id = metadata."submissionId";
</file context>
| await Promise.all( | ||
| handlers.map(async (handler) => { | ||
| try { | ||
| await handler(user); | ||
| } catch (error) { | ||
| console.error("[auth] a sign-in handler failed", error); | ||
| } | ||
| }), | ||
| ); |
There was a problem hiding this comment.
P2: Async sign-in handlers no longer run in registration order, so handlers that depend on earlier initialization can race and produce incorrect sign-in side effects. Retain the previous sequential await semantics unless this API explicitly guarantees that subscribers are independent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/auth/src/signed-in.ts, line 16:
<comment>Async sign-in handlers no longer run in registration order, so handlers that depend on earlier initialization can race and produce incorrect sign-in side effects. Retain the previous sequential await semantics unless this API explicitly guarantees that subscribers are independent.</comment>
<file context>
@@ -13,11 +13,13 @@ export async function notifySignedIn(user: {
- console.error("[auth] a sign-in handler failed", error);
- }
- }
+ await Promise.all(
+ handlers.map(async (handler) => {
+ try {
</file context>
| await Promise.all( | |
| handlers.map(async (handler) => { | |
| try { | |
| await handler(user); | |
| } catch (error) { | |
| console.error("[auth] a sign-in handler failed", error); | |
| } | |
| }), | |
| ); | |
| for (const handler of handlers) { | |
| try { | |
| await handler(user); | |
| } catch (error) { | |
| console.error("[auth] a sign-in handler failed", error); | |
| } | |
| } |
| } | ||
|
|
||
| async assertCanManage(agentId: string, userId: string) { | ||
| const [role, agent] = await Promise.all([ |
There was a problem hiding this comment.
P2: A role revocation or demotion can be bypassed for an in-flight mutation: if membership changes after this check returns but before the caller's transaction starts, the old authorized role is still used to update, deploy, pause, archive, restore, or delete the agent. Revalidate the membership and management permission inside the same transaction as the mutation, or otherwise make the authorization check and write atomic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/agent/agent-access.service.ts, line 36:
<comment>A role revocation or demotion can be bypassed for an in-flight mutation: if membership changes after this check returns but before the caller's transaction starts, the old authorized role is still used to update, deploy, pause, archive, restore, or delete the agent. Revalidate the membership and management permission inside the same transaction as the mutation, or otherwise make the authorization check and write atomic.</comment>
<file context>
@@ -0,0 +1,85 @@
+ }
+
+ async assertCanManage(agentId: string, userId: string) {
+ const [role, agent] = await Promise.all([
+ this.assertMember(userId),
+ this.db.agentDefinition.findFirst({
</file context>
| CONSTRAINT "agentBuilderArtifact_pkey" PRIMARY KEY ("id") | ||
| ); | ||
|
|
||
| CREATE UNIQUE INDEX "agentBuilderArtifact_conversationId_path_revision_key" ON "agentBuilderArtifact"("conversationId", "path", "revision"); |
There was a problem hiding this comment.
P2: Version-scoped artifacts are not protected against duplicate revisions: when conversationId is null, repeated writes for the same versionId/path/revision are accepted because versionId is absent from the unique key. Separate uniqueness constraints for conversation-scoped and version-scoped artifacts would preserve revision idempotency in both workflows.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/prisma/migrations/20260804210000_agent_builder_artifacts/migration.sql, line 18:
<comment>Version-scoped artifacts are not protected against duplicate revisions: when `conversationId` is null, repeated writes for the same `versionId`/`path`/`revision` are accepted because `versionId` is absent from the unique key. Separate uniqueness constraints for conversation-scoped and version-scoped artifacts would preserve revision idempotency in both workflows.</comment>
<file context>
@@ -0,0 +1,23 @@
+ CONSTRAINT "agentBuilderArtifact_pkey" PRIMARY KEY ("id")
+);
+
+CREATE UNIQUE INDEX "agentBuilderArtifact_conversationId_path_revision_key" ON "agentBuilderArtifact"("conversationId", "path", "revision");
+CREATE INDEX "agentBuilderArtifact_conversationId_createdAt_idx" ON "agentBuilderArtifact"("conversationId", "createdAt");
+CREATE INDEX "agentBuilderArtifact_versionId_path_idx" ON "agentBuilderArtifact"("versionId", "path");
</file context>
| "start:prod": "bun dist/main.js", | ||
| "test": "CRM_TELEMETRY_DISABLED=1 bun test", | ||
| "test:watch": "bun test --watch", | ||
| "test": "CRM_TELEMETRY_DISABLED=1 bun test --preload ./test/setup.ts", |
There was a problem hiding this comment.
P2: Adding --preload ./test/setup.ts makes every test invocation import the @crm/db db client (setup.ts does import { db } from "@crm/db"), and packages/db/src/client.ts throws at module load when DATABASE_URL is unset. Before this change the pure unit suites (logging, error-formatter, domain, google-*) never loaded the db client and ran without a database; now bun test/test:watch hard-fail for all suites when DATABASE_URL is absent, contradicting the "Database-backed suites require DATABASE_URL" note in the verification. Consider guarding the preload so DB-free unit runs still work (e.g. only disconnect when a client exists, or make setup.ts tolerant of a missing DATABASE_URL).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/package.json, line 19:
<comment>Adding `--preload ./test/setup.ts` makes every test invocation import the `@crm/db` `db` client (setup.ts does `import { db } from "@crm/db"`), and `packages/db/src/client.ts` throws at module load when `DATABASE_URL` is unset. Before this change the pure unit suites (logging, error-formatter, domain, google-*) never loaded the db client and ran without a database; now `bun test`/`test:watch` hard-fail for all suites when `DATABASE_URL` is absent, contradicting the "Database-backed suites require DATABASE_URL" note in the verification. Consider guarding the preload so DB-free unit runs still work (e.g. only disconnect when a client exists, or make setup.ts tolerant of a missing DATABASE_URL).</comment>
<file context>
@@ -16,8 +16,8 @@
"start:prod": "bun dist/main.js",
- "test": "CRM_TELEMETRY_DISABLED=1 bun test",
- "test:watch": "bun test --watch",
+ "test": "CRM_TELEMETRY_DISABLED=1 bun test --preload ./test/setup.ts",
+ "test:watch": "CRM_TELEMETRY_DISABLED=1 bun test --watch --preload ./test/setup.ts",
"trpc:generate": "nestjs-trpc generate -e src/app.module.ts -r \"**/*.router.ts\" -o src/generated",
</file context>
| ? attachment.mediaType | ||
| : "application/octet-stream"; | ||
|
|
||
| response.setHeader("Cache-Control", "private, max-age=31536000, immutable"); |
There was a problem hiding this comment.
P2: A revoked share or removed workspace membership can still serve the attachment from the browser cache for up to a year because private, max-age=31536000, immutable allows reuse without revalidation. Since attachment authorization is intended to be checked on each fetch, use a non-persistent policy such as private, no-store here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/conversations/conversation-attachments.controller.ts, line 38:
<comment>A revoked share or removed workspace membership can still serve the attachment from the browser cache for up to a year because `private, max-age=31536000, immutable` allows reuse without revalidation. Since attachment authorization is intended to be checked on each fetch, use a non-persistent policy such as `private, no-store` here.</comment>
<file context>
@@ -0,0 +1,56 @@
+ ? attachment.mediaType
+ : "application/octet-stream";
+
+ response.setHeader("Cache-Control", "private, max-age=31536000, immutable");
+ response.setHeader("Content-Length", content.byteLength.toString());
+ response.setHeader("Content-Type", mediaType);
</file context>
| } | ||
|
|
||
| builderConversationQueued(): void { | ||
| this.pokeRoute("/internal/crm/builder-dispatch"); |
There was a problem hiding this comment.
P2: These two new pokes target /internal/crm/builder-dispatch and /internal/crm/agent-dispatch, but no route with either path exists anywhere in the repo — apps/agent/agent/channels/crm.ts only registers /internal/crm/dispatch and /internal/crm/verify-key. Because pokeRoute fire-and-forgets the fetch and only .catches thrown errors, a missing route resolves with a 404 Response and the poke silently no-ops with no log, so queued builder conversations / deployed runs get no dispatch nudge until whatever PR supplies the agent-side route lands. Confirm those endpoints are added in the matching runtime PR (#60) before this ships, or these pokes are dead until then.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/api/src/agent/agent-trigger.service.ts, line 134:
<comment>These two new pokes target `/internal/crm/builder-dispatch` and `/internal/crm/agent-dispatch`, but no route with either path exists anywhere in the repo — `apps/agent/agent/channels/crm.ts` only registers `/internal/crm/dispatch` and `/internal/crm/verify-key`. Because `pokeRoute` fire-and-forgets the fetch and only `.catch`es thrown errors, a missing route resolves with a 404 Response and the poke silently no-ops with no log, so queued builder conversations / deployed runs get no dispatch nudge until whatever PR supplies the agent-side route lands. Confirm those endpoints are added in the matching runtime PR (#60) before this ships, or these pokes are dead until then.</comment>
<file context>
@@ -130,6 +130,14 @@ export class AgentTriggerService {
}
+ builderConversationQueued(): void {
+ this.pokeRoute("/internal/crm/builder-dispatch");
+ }
+
</file context>
| "--exit-code", | ||
| ]); | ||
|
|
||
| if (drift === 2) { |
There was a problem hiding this comment.
P3: The drift===2 message always asserts the schema is ahead of the database, but prisma migrate diff returns 2 for a difference in either direction. If the local DB ever ends up ahead (a manually applied change, or a migration deleted from the migrations/ folder after being applied), the message and the suggested fix (create a new migration) are wrong and would lead a dev to generate a spurious migration. Consider detecting the direction, e.g. by also diffing to the migrations state, or wording the message to cover both directions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/db/scripts/prepare-dev.ts, line 26:
<comment>The drift===2 message always asserts the schema is ahead of the database, but `prisma migrate diff` returns 2 for a difference in either direction. If the local DB ever ends up ahead (a manually applied change, or a migration deleted from the migrations/ folder after being applied), the message and the suggested fix (create a new migration) are wrong and would lead a dev to generate a spurious migration. Consider detecting the direction, e.g. by also diffing to the migrations state, or wording the message to cover both directions.</comment>
<file context>
@@ -0,0 +1,52 @@
+ "--exit-code",
+]);
+
+if (drift === 2) {
+ console.error(
+ "The Prisma schema is ahead of the database. Create a migration with `bun run db:migrate` before starting dev.",
</file context>
Summary
Adds the durable persistence and API domain for custom agents: definitions, immutable versions, triggers, runs, actions, audit records, conversation submissions, attachments, sharing, feedback, and access control.
Why
Custom agents need a durable, permission-aware lifecycle before the builder UI or runtime can rely on them.
Stack
Bottom of native stack #68; targets main through this pull request only.
Verification