From 6edbbab43b509fc982d67ffbeae0f85169800fed Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Wed, 24 Jun 2026 23:18:31 +0300 Subject: [PATCH] refactor(ai): unify provider-settings allowlist + stronger chatApiStyle tests (#177 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the second #177 review: - Architecture (the silent allowlist drift): the writable provider-setting keys were maintained by hand in two TS-uncheckable places — the key-loop in ai-settings.service and the SQL ALLOWED list in the generic workspace repo (a miss there silently dropped a field on persist, exactly what bit chatApiStyle). Introduce one typed source of truth PROVIDER_SETTINGS_KEYS in ai.types (`satisfies readonly (keyof AiProviderSettings)[]`), have the service consume it, and keep the repo's own copy (it can't import AI types) guarded by a parity test so any future drift fails in CI. - Tests: - ai.service.include-usage.spec: mocks @ai-sdk/openai-compatible and asserts the factory is called with { includeUsage: true, baseURL, apiKey, fetch, name } — `.provider` alone could not catch a dropped includeUsage (the token-usage zeroing regression); also asserts the 'openai' style does NOT use it. - ai-provider-settings-keys.spec: the allowlist parity check + DTO validation for chatApiStyle (@IsIn accepts both values, rejects garbage, optional). - CHANGELOG: [Unreleased] entries for the new "Protocol" / chatApiStyle setting and the default provider change (openai -> openai-compatible). (#175, #177) server + client tsc clean; 42 ai/settings specs green. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 13 +++++ .../repos/workspace/workspace.repo.ts | 26 ++++++++- .../ai/ai-provider-settings-keys.spec.ts | 43 ++++++++++++++ .../integrations/ai/ai-settings.service.ts | 18 +----- .../ai/ai.service.include-usage.spec.ts | 58 +++++++++++++++++++ apps/server/src/integrations/ai/ai.types.ts | 28 +++++++++ 6 files changed, 169 insertions(+), 17 deletions(-) create mode 100644 apps/server/src/integrations/ai/ai-provider-settings-keys.spec.ts create mode 100644 apps/server/src/integrations/ai/ai.service.include-usage.spec.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ab0ca99..26adb3f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,9 +25,22 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 flagging dangling references, empty or duplicate definitions, and `[^id]` markers inside table rows, so an agent can fix its own markup. The page is still created; the field is omitted when there are no problems. (#166) +- **AI chat "Protocol" setting (`chatApiStyle`).** A new admin choice in AI + settings for the `openai` driver: `openai-compatible` (default) routes chat + through `@ai-sdk/openai-compatible`, which surfaces a provider's streamed + reasoning (`reasoning_content` → reasoning parts) for z.ai/GLM, DeepSeek, + OpenRouter, etc.; `openai` uses the official provider (real-OpenAI + reasoning-model request shaping). Chosen explicitly rather than inferred from + the base URL, since a custom URL can front real OpenAI too. (#175, #177) ### Changed +- **AI chat default provider is now `openai-compatible` (reasoning surfaced).** + For the `openai` driver the chat provider defaults to the openai-compatible + implementation, so a workspace pointing at z.ai/GLM/DeepSeek now streams the + model's reasoning out of the box. An endpoint that is real OpenAI behind a + custom base URL should set the new `chatApiStyle` "Protocol" to `openai`. (#177) + - **Footnotes now reuse (Pandoc semantics).** Multiple `[^a]` references to the same id are ONE footnote — one number, one definition, several back-references — instead of being renamed to `a__2`, `a__3`. Duplicate `[^a]:` definitions are diff --git a/apps/server/src/database/repos/workspace/workspace.repo.ts b/apps/server/src/database/repos/workspace/workspace.repo.ts index 95e33aa9..60e0a66e 100644 --- a/apps/server/src/database/repos/workspace/workspace.repo.ts +++ b/apps/server/src/database/repos/workspace/workspace.repo.ts @@ -10,6 +10,29 @@ import { import { ExpressionBuilder, sql } from 'kysely'; import { DB, Workspaces } from '@docmost/db/types/db'; +/** + * Writable `settings.ai.provider` keys, enforced at this generic SQL layer. This + * repo cannot import AI-feature types, so this list is its own copy; a parity + * test (ai-provider-settings-keys.spec.ts) asserts it equals + * PROVIDER_SETTINGS_KEYS in ai.types so a future drift fails in CI rather than + * silently dropping a field at this boundary. + */ +export const AI_PROVIDER_SETTINGS_ALLOWED: readonly string[] = [ + 'driver', + 'chatModel', + 'chatApiStyle', + 'embeddingModel', + 'baseUrl', + 'embeddingBaseUrl', + 'sttModel', + 'sttBaseUrl', + 'sttApiStyle', + 'sttLanguage', + 'systemPrompt', + 'publicShareChatModel', + 'publicShareAssistantRoleId', +]; + @Injectable() export class WorkspaceRepo { public baseFields: Array = [ @@ -239,9 +262,8 @@ export class WorkspaceRepo { // is a real jsonb object, never a double-encoded string. The CASE self-heals // workspaces whose settings.ai.provider was previously corrupted into an // array/string. - const ALLOWED = ['driver', 'chatModel', 'chatApiStyle', 'embeddingModel', 'baseUrl', 'embeddingBaseUrl', 'sttModel', 'sttBaseUrl', 'sttApiStyle', 'sttLanguage', 'systemPrompt', 'publicShareChatModel', 'publicShareAssistantRoleId']; const entries = Object.entries(provider).filter( - ([k, v]) => v !== undefined && ALLOWED.includes(k), + ([k, v]) => v !== undefined && AI_PROVIDER_SETTINGS_ALLOWED.includes(k), ); const patch = entries.length ? sql`jsonb_build_object(${sql.join( diff --git a/apps/server/src/integrations/ai/ai-provider-settings-keys.spec.ts b/apps/server/src/integrations/ai/ai-provider-settings-keys.spec.ts new file mode 100644 index 00000000..64a4dbea --- /dev/null +++ b/apps/server/src/integrations/ai/ai-provider-settings-keys.spec.ts @@ -0,0 +1,43 @@ +import { validate } from 'class-validator'; +import { plainToInstance } from 'class-transformer'; +import { PROVIDER_SETTINGS_KEYS } from './ai.types'; +import { AI_PROVIDER_SETTINGS_ALLOWED } from '@docmost/db/repos/workspace/workspace.repo'; +import { UpdateAiSettingsDto } from './dto/update-ai-settings.dto'; + +/** + * Drift guard: the writable provider-settings keys are maintained in two layers + * that TypeScript cannot cross-check — PROVIDER_SETTINGS_KEYS (ai.types, used by + * the settings service) and AI_PROVIDER_SETTINGS_ALLOWED (the generic workspace + * repo's SQL boundary). A key missing from the repo copy silently drops the field + * on persist (exactly what happened to chatApiStyle), so this asserts they match. + */ +describe('provider-settings key allowlist parity', () => { + it('the repo SQL allowlist equals PROVIDER_SETTINGS_KEYS', () => { + expect([...AI_PROVIDER_SETTINGS_ALLOWED].sort()).toEqual( + [...PROVIDER_SETTINGS_KEYS].sort(), + ); + }); +}); + +/** DTO validation for the new chatApiStyle field (@IsIn(CHAT_API_STYLES)). */ +describe('UpdateAiSettingsDto.chatApiStyle', () => { + const errorsFor = async (chatApiStyle: unknown) => + validate(plainToInstance(UpdateAiSettingsDto, { chatApiStyle })); + + it('accepts both valid values', async () => { + for (const v of ['openai-compatible', 'openai']) { + const errs = await errorsFor(v); + expect(errs.find((e) => e.property === 'chatApiStyle')).toBeUndefined(); + } + }); + + it('rejects an unknown value', async () => { + const errs = await errorsFor('definitely-not-a-style'); + expect(errs.find((e) => e.property === 'chatApiStyle')).toBeDefined(); + }); + + it('accepts the field being omitted (optional)', async () => { + const errs = await validate(plainToInstance(UpdateAiSettingsDto, {})); + expect(errs.find((e) => e.property === 'chatApiStyle')).toBeUndefined(); + }); +}); diff --git a/apps/server/src/integrations/ai/ai-settings.service.ts b/apps/server/src/integrations/ai/ai-settings.service.ts index 0717c3c4..05020fa9 100644 --- a/apps/server/src/integrations/ai/ai-settings.service.ts +++ b/apps/server/src/integrations/ai/ai-settings.service.ts @@ -15,6 +15,7 @@ import { ResolvedAiConfig, SttApiStyle, ChatApiStyle, + PROVIDER_SETTINGS_KEYS, } from './ai.types'; /** @@ -280,21 +281,8 @@ export class AiSettingsService { // Persist non-secret provider fields (only those present in the partial). const providerPatch: Partial = {}; - for (const key of [ - 'driver', - 'chatModel', - 'chatApiStyle', - 'embeddingModel', - 'baseUrl', - 'embeddingBaseUrl', - 'sttModel', - 'sttBaseUrl', - 'sttApiStyle', - 'sttLanguage', - 'systemPrompt', - 'publicShareChatModel', - 'publicShareAssistantRoleId', - ] as const) { + // Single source of truth for the writable provider keys (see ai.types). + for (const key of PROVIDER_SETTINGS_KEYS) { if (nonSecret[key] !== undefined) { (providerPatch as Record)[key] = nonSecret[key]; } diff --git a/apps/server/src/integrations/ai/ai.service.include-usage.spec.ts b/apps/server/src/integrations/ai/ai.service.include-usage.spec.ts new file mode 100644 index 00000000..7eb86749 --- /dev/null +++ b/apps/server/src/integrations/ai/ai.service.include-usage.spec.ts @@ -0,0 +1,58 @@ +// `.provider` alone cannot prove the openai-compatible factory was called with +// `includeUsage: true` — a regression dropping it (which zeroes streamed token +// usage / reasoning-token metadata) would still pass. So mock the factory and +// assert the exact args. jest.mock is module-scoped, hence a dedicated file. + +const mockCompatibleModel = { provider: 'openai-compatible.chat', modelId: 'm' }; +// jest allows `mock`-prefixed vars inside a jest.mock factory. +const mockCreateOpenAICompatible = jest.fn( + (_settings: unknown) => () => mockCompatibleModel, +); + +jest.mock('@ai-sdk/openai-compatible', () => ({ + createOpenAICompatible: (settings: unknown) => + mockCreateOpenAICompatible(settings), +})); + +import { AiService } from './ai.service'; + +describe('AiService.getChatModel openai-compatible factory args', () => { + function serviceWith(chatApiStyle?: 'openai-compatible' | 'openai') { + const aiSettings = { + resolve: jest.fn().mockResolvedValue({ + driver: 'openai', + chatModel: 'glm-5.2', + apiKey: 'the-key', + baseUrl: 'https://api.z.ai/v4', + chatApiStyle, + }), + }; + return new AiService( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + aiSettings as any, + { find: jest.fn() } as never, + { decryptSecret: jest.fn() } as never, + ); + } + + beforeEach(() => mockCreateOpenAICompatible.mockClear()); + + it('passes includeUsage:true plus baseURL/apiKey/fetch (default style)', async () => { + await serviceWith().getChatModel('ws-1'); // unset -> openai-compatible + expect(mockCreateOpenAICompatible).toHaveBeenCalledTimes(1); + expect(mockCreateOpenAICompatible).toHaveBeenCalledWith( + expect.objectContaining({ + name: 'openai-compatible', + baseURL: 'https://api.z.ai/v4', + apiKey: 'the-key', + includeUsage: true, + fetch: expect.any(Function), + }), + ); + }); + + it("does NOT use the openai-compatible factory for chatApiStyle 'openai'", async () => { + await serviceWith('openai').getChatModel('ws-1'); + expect(mockCreateOpenAICompatible).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/server/src/integrations/ai/ai.types.ts b/apps/server/src/integrations/ai/ai.types.ts index 5cdb6d1d..29c8d6f2 100644 --- a/apps/server/src/integrations/ai/ai.types.ts +++ b/apps/server/src/integrations/ai/ai.types.ts @@ -57,6 +57,34 @@ export interface AiProviderSettings { publicShareAssistantRoleId?: string; } +/** + * The persisted, non-secret provider setting keys — the SINGLE source of truth + * for which fields a settings update may write through to `settings.ai.provider`. + * `satisfies readonly (keyof AiProviderSettings)[]` makes the compiler reject a + * typo or a key that is not a real provider setting. + * + * The settings service consumes this directly. The generic workspace repo cannot + * import AI types, so it keeps its own copy of the same keys, guarded by a parity + * test against this constant (so any future drift fails in CI, not silently in + * prod — a missing key there validates fine, passes the service, and is then + * dropped at the SQL boundary with no error). + */ +export const PROVIDER_SETTINGS_KEYS = [ + 'driver', + 'chatModel', + 'chatApiStyle', + 'embeddingModel', + 'baseUrl', + 'embeddingBaseUrl', + 'sttModel', + 'sttBaseUrl', + 'sttApiStyle', + 'sttLanguage', + 'systemPrompt', + 'publicShareChatModel', + 'publicShareAssistantRoleId', +] as const satisfies readonly (keyof AiProviderSettings)[]; + /** * Fully resolved provider config, including the decrypted API key for the * stored driver. Returned by `AiSettingsService.resolve`. The keys are held in