vvzvlad/gitmost
1
0
Fork 0
Code Issues 14 Pull Requests 12 Packages Projects Releases Activity

Address PR #222 review: migration order, provider logging, catalog tests

Browse Source 
- Rename catalog-source migration 20260626T120000 -> T150000 so it sorts
  after develop's latest migration (T140000-page-temporary-notes); the old
  timestamp predated ai-chat-message-status/share-aliases and tripped
  Kysely's #ensureMigrationsInOrder, aborting server boot.
- Provider: inject a Nest Logger and log the real cause (incl. response
  status) in the parseJson / readLocal / fetchRemote catch blocks, and
  propagate a useful cause into the BadGatewayException message; add a
  shortError helper (robust to jest's realm-shifted Error-likes).
- Provider: replace the manual Uint8Array assembly with
  Buffer.concat(chunks).toString('utf8'); keep the streaming size cap.
- Controller spec: add admin-gate coverage for the 4 catalog routes
  (catalog/catalogBundle/import/updateFromCatalog) - non-admin Forbidden +
  service untouched, admin delegates with the right args.
- Service spec: add getCatalog/getCatalogBundle tests covering the
  localized() three-tier fallback, the sorted language union, the
  missing-bundle BadGateway, and the role-version default.
- Provider spec: add remote fetch-rejects and non-ok (503) error branches.
- Service: drop the dead Date.now() tail in freeName (now an explicit
  unreachable throw) and extract a shared isUniqueViolation() predicate.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-06-26 22:32:24 +03:00
parent 20ad272c23
commit ef678e34e1
6 changed files with 391 additions and 28 deletions
Download Patch File Download Diff File Expand all files Collapse all files

88
apps/server/src/core/ai-chat/roles/ai-agent-roles.controller.spec.ts
View File

@@ -39,6 +39,10 @@ describe('AiAgentRolesController admin gate', () => {
create: jest.fn().mockResolvedValue({ id: 'r1' }),
update: jest.fn().mockResolvedValue({ id: 'r1' }),
remove: jest.fn().mockResolvedValue({ success: true }),
getCatalog: jest.fn().mockResolvedValue({ languages: [], bundles: [] }),
getCatalogBundle: jest.fn().mockResolvedValue({ roles: [] }),
importFromCatalog: jest.fn().mockResolvedValue({ created: 0 }),
updateFromCatalog: jest.fn().mockResolvedValue({ updated: false }),
};
const controller = new AiAgentRolesController(
rolesService as never,
@@ -109,6 +113,90 @@ describe('AiAgentRolesController admin gate', () => {
});
});
// Catalog routes (browse + import) are ALL admin-only: a non-admin caller must
// get ForbiddenException with the service untouched; an admin delegates with
// the right arguments (import/update-from-catalog carry workspace.id).
describe('catalog routes admin gate', () => {
const catalogDto = { language: 'en' } as never;
const bundleDto = { bundleId: 'general', language: 'en' } as never;
const importDto = {
bundleId: 'general',
language: 'en',
conflict: 'skip',
} as never;
const updateDto = { id: 'r1' } as never;
describe('non-admin is rejected and the service is NOT called', () => {
it('catalog', async () => {
const { controller, rolesService } = makeController(false);
await expect(
controller.catalog(catalogDto, user, workspace),
).rejects.toBeInstanceOf(ForbiddenException);
expect(rolesService.getCatalog).not.toHaveBeenCalled();
});
it('catalog/bundle', async () => {
const { controller, rolesService } = makeController(false);
await expect(
controller.catalogBundle(bundleDto, user, workspace),
).rejects.toBeInstanceOf(ForbiddenException);
expect(rolesService.getCatalogBundle).not.toHaveBeenCalled();
});
it('import', async () => {
const { controller, rolesService } = makeController(false);
await expect(
controller.import(importDto, user, workspace),
).rejects.toBeInstanceOf(ForbiddenException);
expect(rolesService.importFromCatalog).not.toHaveBeenCalled();
});
it('update-from-catalog', async () => {
const { controller, rolesService } = makeController(false);
await expect(
controller.updateFromCatalog(updateDto, user, workspace),
).rejects.toBeInstanceOf(ForbiddenException);
expect(rolesService.updateFromCatalog).not.toHaveBeenCalled();
});
});
describe('admin delegates to the service', () => {
it('catalog passes the requested language', async () => {
const { controller, rolesService } = makeController(true);
await controller.catalog(catalogDto, user, workspace);
expect(rolesService.getCatalog).toHaveBeenCalledWith('en');
});
it('catalog/bundle passes bundleId + language', async () => {
const { controller, rolesService } = makeController(true);
await controller.catalogBundle(bundleDto, user, workspace);
expect(rolesService.getCatalogBundle).toHaveBeenCalledWith(
'general',
'en',
);
});
it('import passes workspace.id + user.id + dto', async () => {
const { controller, rolesService } = makeController(true);
await controller.import(importDto, user, workspace);
expect(rolesService.importFromCatalog).toHaveBeenCalledWith(
'ws-1',
'u1',
importDto,
);
});
it('update-from-catalog passes workspace.id + dto', async () => {
const { controller, rolesService } = makeController(true);
await controller.updateFromCatalog(updateDto, user, workspace);
expect(rolesService.updateFromCatalog).toHaveBeenCalledWith(
'ws-1',
updateDto,
);
});
});
});
describe('list (member-reachable)', () => {
it('non-admin reaches list and the service is asked for the picker view (isAdmin=false)', async () => {
const { controller, rolesService } = makeController(false);

222
apps/server/src/core/ai-chat/roles/ai-agent-roles.service.spec.ts
View File

@@ -1,4 +1,8 @@
import { BadRequestException, ConflictException } from '@nestjs/common';
import {
BadGatewayException,
BadRequestException,
ConflictException,
} from '@nestjs/common';
import { AiAgentRolesService } from './ai-agent-roles.service';
import type { AiAgentRole } from '@docmost/db/types/entity.types';
import type {
@@ -786,4 +790,220 @@ describe('AiAgentRolesService guards', () => {
expect(repo.update.mock.calls[0][2].name).toBe('Researcher');
});
});
// ---------------------------------------------------------------------------
// Catalog browse (getCatalog / getCatalogBundle) against a MOCKED provider.
// Covers the localized() three-tier fallback (requested lang -> en -> first ->
// null), the sorted union of bundle languages, the missing-bundle BadGateway,
// and the role-version default.
// ---------------------------------------------------------------------------
describe('getCatalog', () => {
function makeBrowseService(index: unknown) {
const repo = {
findById: jest.fn(),
insert: jest.fn(),
update: jest.fn(),
softDelete: jest.fn(),
listByWorkspace: jest.fn(),
};
const catalog = {
fetchIndex: jest.fn().mockResolvedValue(index),
fetchBundle: jest.fn(),
};
const service = new AiAgentRolesService(repo as never, catalog as never);
return { service, catalog };
}
it('returns the sorted union of every bundle language', async () => {
const { service } = makeBrowseService({
schemaVersion: 1,
bundles: [
{
id: 'a',
name: { en: 'A' },
languages: ['ru', 'en'],
roles: [],
},
{
id: 'b',
name: { en: 'B' },
languages: ['en', 'de'],
roles: [],
},
],
});
const res = await service.getCatalog('en');
expect(res.languages).toEqual(['de', 'en', 'ru']);
});
it('localized name uses the requested language when present', async () => {
const { service } = makeBrowseService({
schemaVersion: 1,
bundles: [
{
id: 'a',
name: { en: 'General', ru: 'Общие' },
description: { en: 'desc-en', ru: 'desc-ru' },
languages: ['en', 'ru'],
roles: [{ slug: 'researcher', version: 2 }],
},
],
});
const res = await service.getCatalog('ru');
expect(res.bundles[0]).toMatchObject({
id: 'a',
name: 'Общие',
description: 'desc-ru',
languages: ['en', 'ru'],
roles: [{ slug: 'researcher', version: 2 }],
});
});
it('localized name falls back to en when the requested language is missing', async () => {
const { service } = makeBrowseService({
schemaVersion: 1,
bundles: [
{
id: 'a',
name: { en: 'General', ru: 'Общие' },
languages: ['en', 'ru'],
roles: [],
},
],
});
const res = await service.getCatalog('fr');
expect(res.bundles[0].name).toBe('General');
});
it('localized name falls back to the first available locale when en is absent', async () => {
const { service } = makeBrowseService({
schemaVersion: 1,
bundles: [
{
id: 'a',
name: { ru: 'Общие', de: 'Allgemein' },
languages: ['ru', 'de'],
roles: [],
},
],
});
const res = await service.getCatalog('fr');
// Neither 'fr' nor 'en' is present -> first available value.
expect(res.bundles[0].name).toBe('Общие');
});
it('empty name map => falls back to the bundle id; absent description => null', async () => {
const { service } = makeBrowseService({
schemaVersion: 1,
bundles: [
{
id: 'a',
name: {},
languages: ['en'],
roles: [],
},
],
});
const res = await service.getCatalog('en');
expect(res.bundles[0].name).toBe('a');
expect(res.bundles[0].description).toBeNull();
});
});
describe('getCatalogBundle', () => {
function makeBundleService(opts: {
index: unknown;
bundle: unknown;
}) {
const repo = {
findById: jest.fn(),
insert: jest.fn(),
update: jest.fn(),
softDelete: jest.fn(),
listByWorkspace: jest.fn(),
};
const catalog = {
fetchIndex: jest.fn().mockResolvedValue(opts.index),
fetchBundle: jest.fn().mockResolvedValue(opts.bundle),
};
const service = new AiAgentRolesService(repo as never, catalog as never);
return { service, catalog };
}
const index = {
schemaVersion: 1,
bundles: [
{
id: 'general',
name: { en: 'General' },
languages: ['en'],
roles: [{ slug: 'researcher', version: 4 }],
},
],
};
it('missing bundle in the index => BadGateway', async () => {
const { service, catalog } = makeBundleService({
index,
bundle: { schemaVersion: 1, language: 'en', roles: [] },
});
await expect(
service.getCatalogBundle('ghost', 'en'),
).rejects.toBeInstanceOf(BadGatewayException);
expect(catalog.fetchBundle).not.toHaveBeenCalled();
});
it('maps role content with the version taken from the index', async () => {
const { service } = makeBundleService({
index,
bundle: {
schemaVersion: 1,
language: 'en',
roles: [
{
slug: 'researcher',
name: 'Researcher',
instructions: 'be a researcher',
emoji: '🔬',
autoStart: false,
launchMessage: 'go',
},
],
},
});
const res = await service.getCatalogBundle('general', 'en');
expect(res).toMatchObject({ bundleId: 'general', language: 'en' });
expect(res.roles[0]).toEqual({
slug: 'researcher',
emoji: '🔬',
name: 'Researcher',
description: null,
instructions: 'be a researcher',
autoStart: false,
launchMessage: 'go',
version: 4,
});
});
it('role absent from the index meta => version defaults to 1; autoStart defaults to true', async () => {
const { service } = makeBundleService({
index,
bundle: {
schemaVersion: 1,
language: 'en',
roles: [
{ slug: 'newcomer', name: 'Newcomer', instructions: 'hi' },
],
},
});
const res = await service.getCatalogBundle('general', 'en');
expect(res.roles[0]).toMatchObject({
slug: 'newcomer',
version: 1,
autoStart: true,
emoji: null,
launchMessage: null,
});
});
});
});

32
apps/server/src/core/ai-chat/roles/ai-agent-roles.service.ts
View File

@@ -509,11 +509,7 @@ export class AiAgentRolesService {
* failures keep surfacing as 500s.
*/
function rethrowDuplicateName(err: unknown, name: string): never {
if (
err &&
typeof err === 'object' &&
(err as { code?: unknown }).code === '23505'
) {
if (isUniqueViolation(err)) {
throw new ConflictException(
`A role named "${name}" already exists in this workspace.`,
);
@@ -521,6 +517,15 @@ function rethrowDuplicateName(err: unknown, name: string): never {
throw err;
}
/** Whether `err` is a Postgres unique-violation (SQLSTATE 23505). */
function isUniqueViolation(err: unknown): boolean {
return (
!!err &&
typeof err === 'object' &&
(err as { code?: unknown }).code === '23505'
);
}
/** '' / whitespace-only / undefined / null => null; otherwise the trimmed value. */
function emptyToNull(value: string | null | undefined): string | null {
if (value === undefined || value === null) return null;
@@ -564,23 +569,20 @@ function localized(
* `taken` after a successful insert.
*/
function freeName(base: string, taken: Set<string>): string {
let n = 2;
// Cap the search defensively; the loop always terminates well before this.
while (n < 1000) {
// `taken` is finite, so within `taken.size + 2` iterations a candidate index
// is guaranteed free; the 1000 cap is a defensive upper bound far above any
// realistic per-name collision count. The throw below is therefore
// unreachable in practice and only satisfies the return-type checker.
for (let n = 2; n < 1000; n++) {
const candidate = `${base} (${n})`;
if (!taken.has(candidate.toLowerCase())) return candidate;
n++;
}
return `${base} (${Date.now()})`;
throw new BadRequestException(`Too many roles named "${base}"`);
}
/** A short, safe message for an import insert failure (409 vs other). */
function importErrorMessage(err: unknown): string {
if (
err &&
typeof err === 'object' &&
(err as { code?: unknown }).code === '23505'
) {
if (isUniqueViolation(err)) {
return 'A role with this name already exists';
}
return 'Failed to import role';

18
apps/server/src/core/ai-chat/roles/catalog/ai-agent-roles-catalog.provider.spec.ts
View File

@@ -174,6 +174,24 @@ describe('AiAgentRolesCatalogProvider (local fixtures)', () => {
);
});
it('fetch rejects (network failure) => BadGateway (unavailable)', async () => {
global.fetch = jest
.fn()
.mockRejectedValue(new Error('ECONNREFUSED')) as never;
const provider = makeProvider('https://catalog.example.com');
await expect(provider.fetchIndex()).rejects.toBeInstanceOf(
BadGatewayException,
);
});
it('non-ok response (503) => BadGateway carrying the status', async () => {
global.fetch = jest.fn().mockResolvedValue(
mockResponse({ ok: false, status: 503, body: null }),
) as never;
const provider = makeProvider('https://catalog.example.com');
await expect(provider.fetchIndex()).rejects.toThrow(/503/);
});
it('small streamed body parses normally (cap not hit)', async () => {
const json = JSON.stringify({
schemaVersion: 1,

59
apps/server/src/core/ai-chat/roles/catalog/ai-agent-roles-catalog.provider.ts
View File

@@ -4,6 +4,7 @@ import {
BadGatewayException,
BadRequestException,
Injectable,
Logger,
} from '@nestjs/common';
import { EnvironmentService } from '../../../../integrations/environment/environment.service';
import {
@@ -35,6 +36,8 @@ const MAX_BYTES = 1_000_000;
*/
@Injectable()
export class AiAgentRolesCatalogProvider {
private readonly logger = new Logger(AiAgentRolesCatalogProvider.name);
constructor(private readonly environmentService: EnvironmentService) {}
/** Read + validate the top-level index (`index.json`). */
@@ -79,9 +82,11 @@ export class AiAgentRolesCatalogProvider {
private parseJson(raw: string, rel: string): unknown {
try {
return JSON.parse(raw);
} catch {
} catch (err) {
const reason = shortError(err);
this.logger.error(`Agent roles catalog JSON parse failed (${rel}): ${reason}`);
throw new BadGatewayException(
`Agent roles catalog file is not valid JSON (${rel})`,
`Agent roles catalog file is not valid JSON (${rel}): ${reason}`,
);
}
}
@@ -102,8 +107,14 @@ export class AiAgentRolesCatalogProvider {
private async readLocal(dir: string, rel: string): Promise<string> {
try {
return await fs.readFile(path.join(dir, rel), 'utf8');
} catch {
throw new BadGatewayException('Agent roles catalog is unavailable');
} catch (err) {
const reason = shortError(err);
this.logger.error(
`Agent roles catalog local read failed (${path.join(dir, rel)}): ${reason}`,
);
throw new BadGatewayException(
`Agent roles catalog is unavailable: ${reason}`,
);
}
}
@@ -122,10 +133,19 @@ export class AiAgentRolesCatalogProvider {
let response: Response;
try {
response = await fetch(url, { signal: controller.signal });
} catch {
throw new BadGatewayException('Agent roles catalog is unavailable');
} catch (err) {
const reason = shortError(err);
this.logger.error(
`Agent roles catalog remote fetch failed (${rel}): ${reason}`,
);
throw new BadGatewayException(
`Agent roles catalog is unavailable: ${reason}`,
);
}
if (!response.ok) {
this.logger.error(
`Agent roles catalog remote returned ${response.status} (${rel})`,
);
throw new BadGatewayException(
`Agent roles catalog returned ${response.status}`,
);
@@ -179,13 +199,28 @@ async function readStreamCapped(
// Release the stream on both the normal and the too-large/abort paths.
await reader.cancel().catch(() => undefined);
}
const merged = new Uint8Array(total);
let offset = 0;
for (const chunk of chunks) {
merged.set(chunk, offset);
offset += chunk.length;
return Buffer.concat(chunks).toString('utf8');
}
/**
* A short, non-sensitive error string for logging/propagation: only the first
* line of the message head is kept (upstream bodies / URLs are discarded).
*/
function shortError(err: unknown): string {
let message = '';
if (typeof err === 'string') {
message = err;
} else if (
err &&
typeof err === 'object' &&
typeof (err as { message?: unknown }).message === 'string'
) {
// Read `.message` directly (works for Error instances and the realm-shifted
// Error-likes jest can hand back, where `instanceof Error` is false).
message = (err as { message: string }).message;
}
return new TextDecoder('utf-8').decode(merged);
const head = (message || 'unknown error').split('\n')[0];
return head.length > 200 ? `${head.slice(0, 200)}…` : head;
}
// ---------------------------------------------------------------------------

0
apps/server/src/database/migrations/20260626T120000-ai-agent-roles-catalog-source.ts → apps/server/src/database/migrations/20260626T150000-ai-agent-roles-catalog-source.ts
View File