diff --git a/apps/server/src/core/ai-chat/ai-chat.role-resolve.spec.ts b/apps/server/src/core/ai-chat/ai-chat.role-resolve.spec.ts index 1b0afeb4..f8348dfb 100644 --- a/apps/server/src/core/ai-chat/ai-chat.role-resolve.spec.ts +++ b/apps/server/src/core/ai-chat/ai-chat.role-resolve.spec.ts @@ -31,13 +31,16 @@ describe('AiChatService.resolveRoleForRequest', () => { function makeService(opts: { chat?: { roleId: string | null } | undefined; + // The role returned by findLiveEnabled (the live + enabled + workspace-scoped + // lookup). undefined models a missing / soft-deleted / disabled / cross- + // workspace role — the repo, not the service, now enforces those filters. role?: AiAgentRole | undefined; }) { const aiChatRepo = { findById: jest.fn().mockResolvedValue(opts.chat), }; const aiAgentRoleRepo = { - findById: jest.fn().mockResolvedValue(opts.role), + findLiveEnabled: jest.fn().mockResolvedValue(opts.role), }; const service = new AiChatService( {} as never, // ai @@ -66,8 +69,11 @@ describe('AiChatService.resolveRoleForRequest', () => { expect(resolved).toBe(role); // The role lookup used the chat's role id, never the body's. - expect(aiAgentRoleRepo.findById).toHaveBeenCalledWith('chat-role', 'ws-1'); - expect(aiAgentRoleRepo.findById).not.toHaveBeenCalledWith( + expect(aiAgentRoleRepo.findLiveEnabled).toHaveBeenCalledWith( + 'chat-role', + 'ws-1', + ); + expect(aiAgentRoleRepo.findLiveEnabled).not.toHaveBeenCalledWith( 'attacker-role', expect.anything(), ); @@ -77,8 +83,8 @@ describe('AiChatService.resolveRoleForRequest', () => { it('scopes the role lookup to the workspace (cross-workspace roleId => null)', async () => { // The repo stub returns undefined to model a roleId that does not exist in - // THIS workspace (findById is workspace-scoped). resolveRoleForRequest must - // still pass workspace.id to the lookup. + // THIS workspace (findLiveEnabled is workspace-scoped). resolveRoleForRequest + // must still pass workspace.id to the lookup. const { service, aiAgentRoleRepo } = makeService({ chat: undefined, role: undefined, @@ -88,17 +94,18 @@ describe('AiChatService.resolveRoleForRequest', () => { const resolved = await service.resolveRoleForRequest(workspace, body); expect(resolved).toBeNull(); - expect(aiAgentRoleRepo.findById).toHaveBeenCalledWith( + expect(aiAgentRoleRepo.findLiveEnabled).toHaveBeenCalledWith( 'role-from-other-ws', 'ws-1', ); }); - it('role found but disabled (enabled=false) => null (disabled role not applied)', async () => { - const role = makeRole({ enabled: false }); + it('disabled role: findLiveEnabled filters it out (undefined) => null (disabled role not applied)', async () => { + // The repo's findLiveEnabled enforces enabled=true, so a disabled role never + // comes back; the service just maps that undefined to null. const { service } = makeService({ chat: { roleId: 'role-1' }, - role, + role: undefined, }); const body: AiChatStreamBody = { chatId: 'chat-1' }; @@ -130,7 +137,10 @@ describe('AiChatService.resolveRoleForRequest', () => { const resolved = await service.resolveRoleForRequest(workspace, body); expect(resolved).toBe(role); - expect(aiAgentRoleRepo.findById).toHaveBeenCalledWith('picked', 'ws-1'); + expect(aiAgentRoleRepo.findLiveEnabled).toHaveBeenCalledWith( + 'picked', + 'ws-1', + ); // No chat lookup happens when there is no chatId. expect(aiChatRepo.findById).not.toHaveBeenCalled(); }); @@ -149,7 +159,10 @@ describe('AiChatService.resolveRoleForRequest', () => { const resolved = await service.resolveRoleForRequest(workspace, body); expect(resolved).toBe(role); - expect(aiAgentRoleRepo.findById).toHaveBeenCalledWith('body-role', 'ws-1'); + expect(aiAgentRoleRepo.findLiveEnabled).toHaveBeenCalledWith( + 'body-role', + 'ws-1', + ); }); it('no role anywhere (universal assistant): returns null without a role lookup', async () => { @@ -163,6 +176,6 @@ describe('AiChatService.resolveRoleForRequest', () => { expect(resolved).toBeNull(); // Short-circuit: no roleId means no lookup at all. - expect(aiAgentRoleRepo.findById).not.toHaveBeenCalled(); + expect(aiAgentRoleRepo.findLiveEnabled).not.toHaveBeenCalled(); }); }); diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index 46cb6e90..4f6f4f47 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -150,14 +150,14 @@ export class AiChatService { roleId = body.roleId; } if (!roleId) return null; - const role = await this.aiAgentRoleRepo.findById(roleId, workspace.id); - // A disabled role falls back to the universal assistant: it must not apply - // its persona/model override even to a chat that was bound to it earlier. - // findById already excludes soft-deleted roles; this also drops disabled - // ones, server-authoritatively, for both the new-chat (body.roleId) and - // existing-chat (chat.role_id) paths. - if (!role || !role.enabled) return null; - return role; + // A disabled or soft-deleted role falls back to the universal assistant: it + // must not apply its persona/model override even to a chat that was bound to + // it earlier. findLiveEnabled enforces this (live + enabled + workspace + // scope), server-authoritatively, for both the new-chat (body.roleId) and + // existing-chat (chat.role_id) paths — the single shared invariant. + return ( + (await this.aiAgentRoleRepo.findLiveEnabled(roleId, workspace.id)) ?? null + ); } /** diff --git a/apps/server/src/core/ai-chat/public-share-chat.service.ts b/apps/server/src/core/ai-chat/public-share-chat.service.ts index 310fd0cf..2844b33c 100644 --- a/apps/server/src/core/ai-chat/public-share-chat.service.ts +++ b/apps/server/src/core/ai-chat/public-share-chat.service.ts @@ -153,9 +153,11 @@ export class PublicShareChatService { const resolved = await this.aiSettings.resolve(workspaceId); const roleId = resolved?.publicShareAssistantRoleId; if (!roleId) return null; - const role = await this.aiAgentRoleRepo.findById(roleId, workspaceId); - if (!role || !role.enabled) return null; - return role; + // Same shared invariant as the authenticated chat: only a live + enabled + + // workspace-scoped role applies; otherwise the built-in locked persona does. + return ( + (await this.aiAgentRoleRepo.findLiveEnabled(roleId, workspaceId)) ?? null + ); } /** diff --git a/apps/server/src/core/ai-chat/public-share-chat.spec.ts b/apps/server/src/core/ai-chat/public-share-chat.spec.ts index 6cc9039a..55e5571b 100644 --- a/apps/server/src/core/ai-chat/public-share-chat.spec.ts +++ b/apps/server/src/core/ai-chat/public-share-chat.spec.ts @@ -251,8 +251,10 @@ describe('buildShareSystemPrompt locking', () => { describe('PublicShareChatService model fallback', () => { // `role` (optional) drives both the resolved settings (its id is returned as - // publicShareAssistantRoleId) and the role repo's findById mock, so the same - // helper exercises the no-role fallback AND the role-override paths. + // publicShareAssistantRoleId) and the role repo's findLiveEnabled mock, so the + // same helper exercises the no-role fallback AND the role-override paths. The + // mock mirrors the real repo: findLiveEnabled only returns a role that is live + // AND enabled, so a disabled `role` resolves to undefined here. function makeService( resolvePublicModel: string | undefined, role?: { @@ -272,7 +274,9 @@ describe('PublicShareChatService model fallback', () => { const getChatModel = jest.fn().mockResolvedValue('MODEL'); const ai = { getChatModel }; const aiAgentRoleRepo = { - findById: jest.fn().mockResolvedValue(role ?? undefined), + findLiveEnabled: jest + .fn() + .mockResolvedValue(role && role.enabled ? role : undefined), }; const redisService = { getOrThrow: () => new FakeRedis() } as never; const service = new PublicShareChatService( @@ -314,14 +318,14 @@ describe('PublicShareChatService model fallback', () => { expect(await service.resolveShareRole('ws-1')).toBeNull(); }); - it('returns null when findById resolves undefined (missing/soft-deleted)', async () => { + it('returns null when findLiveEnabled resolves undefined (missing/soft-deleted/disabled)', async () => { const { service, aiAgentRoleRepo } = makeService('cheap-model', { id: 'r-1', name: 'R', enabled: true, }); - // The settings point at r-1, but the repo can no longer find it. - aiAgentRoleRepo.findById.mockResolvedValue(undefined); + // The settings point at r-1, but the repo can no longer find it live+enabled. + aiAgentRoleRepo.findLiveEnabled.mockResolvedValue(undefined); expect(await service.resolveShareRole('ws-1')).toBeNull(); }); diff --git a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts new file mode 100644 index 00000000..91a3ffad --- /dev/null +++ b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.spec.ts @@ -0,0 +1,51 @@ +import { AiAgentRoleRepo } from './ai-agent-roles.repo'; +import type { KyselyDB } from '../../types/kysely.types'; + +/** + * Unit test for the SECURITY invariant carried by + * AiAgentRoleRepo.findLiveEnabled: it is the single source of truth shared by + * the authenticated chat and the anonymous public-share assistant for "resolve + * a roleId to a LIVE, ENABLED role scoped to the workspace, else undefined". + * + * A live Postgres is out of scope here; instead we record the query the repo + * builds and assert it pins ALL of the security filters: id, workspaceId, + * deletedAt IS NULL, and enabled = true. If any of those `where` clauses is + * dropped, the role scoping silently widens — this test guards exactly that. + */ +describe('AiAgentRoleRepo.findLiveEnabled', () => { + function makeRepoWithSpy(result: unknown) { + const where = jest.fn(); + const builder = { + selectAll: jest.fn(() => builder), + where: jest.fn((...args: unknown[]) => { + where(...args); + return builder; + }), + executeTakeFirst: jest.fn().mockResolvedValue(result), + }; + const db = { + selectFrom: jest.fn(() => builder), + } as unknown as KyselyDB; + return { repo: new AiAgentRoleRepo(db), db, where }; + } + + it('queries scoped to id + workspace, live (deletedAt null) AND enabled', async () => { + const role = { id: 'r-1', workspaceId: 'ws-1', enabled: true }; + const { repo, db, where } = makeRepoWithSpy(role); + + const result = await repo.findLiveEnabled('r-1', 'ws-1'); + + expect(result).toBe(role); + expect(db.selectFrom).toHaveBeenCalledWith('aiAgentRoles'); + // Every security filter must be present. + expect(where).toHaveBeenCalledWith('id', '=', 'r-1'); + expect(where).toHaveBeenCalledWith('workspaceId', '=', 'ws-1'); + expect(where).toHaveBeenCalledWith('deletedAt', 'is', null); + expect(where).toHaveBeenCalledWith('enabled', '=', true); + }); + + it('returns undefined when no live+enabled role matches', async () => { + const { repo } = makeRepoWithSpy(undefined); + expect(await repo.findLiveEnabled('r-1', 'ws-1')).toBeUndefined(); + }); +}); diff --git a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts index 4adfa677..1d44c776 100644 --- a/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts +++ b/apps/server/src/database/repos/ai-agent-roles/ai-agent-roles.repo.ts @@ -32,6 +32,29 @@ export class AiAgentRoleRepo { .executeTakeFirst(); } + /** + * Single live (not soft-deleted) AND enabled role scoped to the workspace, or + * undefined. This is the SECURITY invariant shared by the authenticated chat + * and the anonymous public-share assistant: a role only applies its persona / + * model override when it currently exists, is not soft-deleted, and is enabled + * — a disabled or deleted role server-authoritatively degrades to the built-in + * universal assistant. Single source of truth so the two resolve paths cannot + * drift apart. + */ + async findLiveEnabled( + id: string, + workspaceId: string, + ): Promise { + return this.db + .selectFrom('aiAgentRoles') + .selectAll('aiAgentRoles') + .where('id', '=', id) + .where('workspaceId', '=', workspaceId) + .where('deletedAt', 'is', null) + .where('enabled', '=', true) + .executeTakeFirst(); + } + /** All live roles for the workspace (management list + chat picker). */ async listByWorkspace(workspaceId: string): Promise { return this.db