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

Address PR #222 re-review: fix source-uniqueness detection + coverage/cleanups

Browse Source 
MUST-FIX
- isSourceUniqueViolation read the wrong error field: kysely-postgres-js
  (postgres@3.4.8) puts the violated constraint on `constraint_name`, not
  node-postgres' `.constraint`, so a concurrent same-slug+language import's
  23505 was never recognized as a source-collision and surfaced a false
  "name already exists" error. Now read `constraint_name` (with `.constraint`
  as a fallback for other drivers). Fix the faked test fixture (it built the
  error with the same wrong `.constraint` field, masking the bug): it now
  uses `constraint_name`, so the test genuinely exercises the skip path and
  FAILS against the unfixed code.
- Extract the catalog modal's role-state computation into a pure
  `catalogRoleInstallState(role, workspaceRoles, language)` helper (mirrors
  role-launch.ts) and cover it with vitest: import / installed / update /
  same-slug-different-language.

SUGGESTIONS
- Restore IAiRoleUpdateFromCatalogResult as a discriminated union mirroring
  the server; narrow the consumer via `"reason" in result` (the boolean
  discriminant does not narrow under strictNullChecks:false).
- README: add a "How it's served" section documenting AI_AGENT_ROLES_CATALOG_URL
  (remote http(s) base / local path / empty => in-repo folder).
- check.mjs: drop the redundant `const key = slug` alias.
- Cover the reason->message mapping in useUpdateAiRoleFromCatalogMutation
  (4 branches) via renderHook with a mocked service.
- Cover importFromCatalog "bundle not in index" => BadGateway.
- Cover updateFromCatalog "slug in index but missing in bundle file" =>
  not-in-catalog.

ARCHITECTURE
- Extract the shared catalog read prefix: a private `loadBundleById`
  (fetchIndex -> meta -> fetchBundle -> versionMap) reused by getCatalogBundle
  and importFromCatalog, and a `catalogRoleContentFields` mapper shared by the
  import insert and update patch. The three orchestrations and their distinct
  write paths stay separate.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-06-27 01:01:29 +03:00
parent 26ca19f89e
commit 2b7c861f78
10 changed files with 422 additions and 73 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -664,9 +664,11 @@ describe('AiAgentRolesService guards', () => {
{ slug: 'b', version: 1 },
],
});
// The kysely-postgres-js driver surfaces the violated constraint on
// `constraint_name` (not node-postgres' `.constraint`), matching prod.
const sourceRace = Object.assign(new Error('duplicate key'), {
code: '23505',
constraint: 'ai_agent_roles_workspace_source_unique',
constraint_name: 'ai_agent_roles_workspace_source_unique',
});
repo.insert
.mockRejectedValueOnce(sourceRace)
@@ -714,6 +716,17 @@ describe('AiAgentRolesService guards', () => {
logSpy.mockRestore();
}
});
it('bundleId absent from the index => BadGateway (no insert)', async () => {
// The requested bundle is not listed in the fetched index (a stale client
// or an index/bundle drift); the import must surface a 502 rather than
// silently doing nothing or dereferencing a missing meta.
const { service, repo } = makeImportService({});
await expect(
service.importFromCatalog('ws-1', 'u1', dto({ bundleId: 'missing' })),
).rejects.toBeInstanceOf(BadGatewayException);
expect(repo.insert).not.toHaveBeenCalled();
});
});
describe('updateFromCatalog', () => {
@@ -847,6 +860,22 @@ describe('AiAgentRolesService guards', () => {
expect('enabled' in patch).toBe(false);
});
it('slug listed in the index but missing from the bundle file => not-in-catalog', async () => {
// Index/bundle drift: the index still advertises a newer `researcher`
// (v5 > installed v1) in an offered language, but the fetched bundle file
// no longer contains that slug. The update must no-op as not-in-catalog,
// not throw or write a half-resolved role.
const { service, repo } = makeUpdateService({
role: imported(1),
bundleRoles: [
{ slug: 'someone-else', name: 'Other', instructions: 'x' },
],
});
const res = await service.updateFromCatalog('ws-1', { id: 'r1' } as never);
expect(res).toEqual({ updated: false, reason: 'not-in-catalog' });
expect(repo.update).not.toHaveBeenCalled();
});
it('new catalog name collides with another live role => keeps current name', async () => {
const role = imported(1);
const other = makeRow({ id: 'r2', name: 'Researcher v5' });

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

@@ -14,7 +14,11 @@ import { CreateAgentRoleDto, UpdateAgentRoleDto } from './dto/agent-role.dto';
import { ImportFromCatalogDto, UpdateFromCatalogDto } from './dto/agent-role-catalog.dto';
import { RoleModelConfig } from './role-model-config';
import { AiAgentRolesCatalogProvider } from './catalog/ai-agent-roles-catalog.provider';
import { CatalogBundleMeta } from './catalog/catalog-types';
import {
CatalogBundleFile,
CatalogBundleMeta,
CatalogRole,
} from './catalog/catalog-types';
/**
* Full (admin) view of an agent role. There are no secret columns on this table
@@ -217,6 +221,30 @@ export class AiAgentRolesService {
return { languages, bundles };
}
/**
* Shared read prefix for the two bundle-by-id catalog paths (getCatalogBundle /
* importFromCatalog): fetch the index, resolve the requested bundle's meta
* (502 if the index does not list it), fetch its per-language file, and build
* the slug->version map from the meta. The callers keep their own response /
* write logic; only this duplicated read is factored out here.
*/
private async loadBundleById(
bundleId: string,
language: string,
): Promise<{
meta: CatalogBundleMeta;
file: CatalogBundleFile;
versions: Map<string, number>;
}> {
const index = await this.catalog.fetchIndex();
const meta = index.bundles.find((b) => b.id === bundleId);
if (!meta) {
throw new BadGatewayException('Catalog bundle not found');
}
const file = await this.catalog.fetchBundle(bundleId, language);
return { meta, file, versions: versionMap(meta) };
}
/**
* Open one bundle in a language: returns each role's content plus the version
* taken from the index (so the client can compare against an imported role's
@@ -239,13 +267,7 @@ export class AiAgentRolesService {
version: number;
}[];
}> {
const index = await this.catalog.fetchIndex();
const meta = index.bundles.find((b) => b.id === bundleId);
if (!meta) {
throw new BadGatewayException('Catalog bundle not found');
}
const file = await this.catalog.fetchBundle(bundleId, language);
const versions = versionMap(meta);
const { file, versions } = await this.loadBundleById(bundleId, language);
return {
bundleId,
language,
@@ -284,13 +306,10 @@ export class AiAgentRolesService {
renamed: number;
errors: { slug: string; message: string }[];
}> {
const index = await this.catalog.fetchIndex();
const meta = index.bundles.find((b) => b.id === dto.bundleId);
if (!meta) {
throw new BadGatewayException('Catalog bundle not found');
}
const file = await this.catalog.fetchBundle(dto.bundleId, dto.language);
const versions = versionMap(meta);
const { file, versions } = await this.loadBundleById(
dto.bundleId,
dto.language,
);
const errors: { slug: string; message: string }[] = [];
@@ -355,15 +374,8 @@ export class AiAgentRolesService {
workspaceId,
creatorId,
name,
emoji: emptyToNull(role.emoji),
description: emptyToNull(role.description),
instructions: role.instructions,
modelConfig: normalizeModelConfig(role.modelConfig) as
| Record<string, unknown>
| null,
...catalogRoleContentFields(role),
enabled: true,
autoStart: role.autoStart ?? true,
launchMessage: emptyToNull(role.launchMessage ?? undefined),
source: { slug: role.slug, language: dto.language, version },
});
created++;
@@ -465,14 +477,7 @@ export class AiAgentRolesService {
await this.repo.update(dto.id, workspaceId, {
name,
emoji: emptyToNull(fresh.emoji),
description: emptyToNull(fresh.description),
instructions: fresh.instructions,
modelConfig: normalizeModelConfig(fresh.modelConfig) as
| Record<string, unknown>
| null,
autoStart: fresh.autoStart ?? true,
launchMessage: emptyToNull(fresh.launchMessage ?? undefined),
...catalogRoleContentFields(fresh),
// enabled is deliberately NOT changed.
source: {
slug: source.slug,
@@ -562,19 +567,49 @@ const SOURCE_UNIQUE_CONSTRAINT = 'ai_agent_roles_workspace_source_unique';
/**
* Whether `err` is the 23505 raised by the SOURCE-uniqueness index specifically
* (vs the name-uniqueness index). Postgres sets `constraint` to the violated
* index name on a unique violation; we key off that so a source race is skipped
* while a name race still surfaces as a friendly per-role error. A 23505 with no
* constraint name (e.g. a wrapped/test error) is NOT treated as a source
* collision, preserving the existing name-race behavior.
* (vs the name-uniqueness index). The active driver (`kysely-postgres-js` over
* `postgres@3.4.8`) exposes the violated constraint name on `constraint_name`,
* so we key off that (accepting the node-postgres-style `.constraint` as a
* fallback for other drivers) — that way a source race is skipped while a name
* race still surfaces as a friendly per-role error. A 23505 with no constraint
* name (e.g. a wrapped/test error) is NOT treated as a source collision,
* preserving the existing name-race behavior.
*/
function isSourceUniqueViolation(err: unknown): boolean {
if (!isUniqueViolation(err)) return false;
const e = err as { constraint_name?: unknown; constraint?: unknown };
return (
isUniqueViolation(err) &&
(err as { constraint?: unknown }).constraint === SOURCE_UNIQUE_CONSTRAINT
e.constraint_name === SOURCE_UNIQUE_CONSTRAINT ||
e.constraint === SOURCE_UNIQUE_CONSTRAINT
);
}
/**
* The role-content fields shared by import (insert) and update (patch) of a
* catalog role: emoji/description/launchMessage normalized to null, model config
* normalized, autoStart defaulted. The caller adds the write-specific fields
* (`name`, `source`, and on insert `workspaceId`/`creatorId`/`enabled`).
*/
function catalogRoleContentFields(role: CatalogRole): {
emoji: string | null;
description: string | null;
instructions: string;
modelConfig: Record<string, unknown> | null;
autoStart: boolean;
launchMessage: string | null;
} {
return {
emoji: emptyToNull(role.emoji),
description: emptyToNull(role.description),
instructions: role.instructions,
modelConfig: normalizeModelConfig(role.modelConfig) as
| Record<string, unknown>
| null,
autoStart: role.autoStart ?? true,
launchMessage: emptyToNull(role.launchMessage ?? undefined),
};
}
/** '' / whitespace-only / undefined / null => null; otherwise the trimmed value. */
function emptyToNull(value: string | null | undefined): string | null {
if (value === undefined || value === null) return null;