Skip to content

Commit fda1bfc

Browse files
danny-aviladanny-avila
authored
🔬 ci: Add TypeScript Type Checks to Backend Workflow and Fix All Type Errors (#12451)
* fix(data-schemas): resolve TypeScript strict type check errors in source files - Constrain ConfigSection to string keys via `string & keyof TCustomConfig` - Replace broken `z` import from data-provider with TCustomConfig derivation - Add `_id: Types.ObjectId` to IUser matching other Document interfaces - Add `federatedTokens` and `openidTokens` optional fields to IUser - Type mongoose model accessors as `Model<IRole>` and `Model<IUser>` - Widen `getPremiumRate` param to accept `number | null` - Widen `bulkWriteAclEntries` ops to untyped `AnyBulkWriteOperation[]` - Fix `getUserPrincipals` return type to use `PrincipalType` enum - Add non-null assertions for `connection.db` in migration files - Import DailyRotateFile constructor directly instead of relying on broken module augmentation across mismatched node_modules trees - Add winston-daily-rotate-file as devDependency for type resolution * fix(data-schemas): resolve TypeScript type errors in test files - Replace arbitrary test keys with valid TCustomConfig properties in config.spec - Use non-null assertions for permission objects in role.methods.spec - Replace `.SHARED_GLOBAL` access with `.not.toHaveProperty()` for legacy field - Add non-null assertions for balance, writeRate, readRate in spendTokens.spec - Update mock user _id to use ObjectId in user.test - Remove unused Schema import in tenantIndexes.spec * fix(api): resolve TypeScript strict type check errors across source and test files - Widen getUserPrincipals dep type in capabilities middleware - Fix federatedTokens type in createSafeUser return - Use proper mock req type for read-only properties in preAuthTenant.spec - Replace `as IUser` casts with ObjectId-typed mocks in openid/oidc specs - Use TokenExchangeMethodEnum values instead of string literals in MCP specs - Fix SessionStore type compatibility in sessionCache specs - Replace `catch (error: any)` with `(error as Error)` in redis specs - Remove invalid properties from test data in initialize and MCP specs - Add String.prototype.isWellFormed declaration for sanitizeTitle spec * fix(client): resolve TypeScript type errors in shared client components - Add default values for destructured bindings in OGDialogTemplate - Replace broken ExtendedFile import with inline type in FileIcon * ci: add TypeScript type-check job to backend review workflow Add a `typecheck` job that runs `tsc --noEmit` on all four TypeScript workspaces (data-provider, data-schemas, @librechat/api, @librechat/client) after the build step. Catches type errors that rollup builds may miss. * fix(data-schemas): add local type declaration for DailyRotateFile transport The `winston-daily-rotate-file` package ships a module augmentation for `winston/lib/winston/transports`, but it fails when winston and winston-daily-rotate-file resolve from different node_modules trees (which happens in this monorepo due to npm hoisting). Add a local `.d.ts` declaration that augments the same module path from within data-schemas' compilation unit, so `tsc --noEmit` passes while keeping the original runtime pattern (`new winston.transports.DailyRotateFile`). * fix: address code review findings from PR #12451 - Restore typed `AnyBulkWriteOperation<AclEntry>[]` on bulkWriteAclEntries, cast to untyped only at the tenantSafeBulkWrite call site (Finding 1) - Type `findUser` model accessor consistently with `findUsers` (Finding 2) - Replace inline `import('mongoose').ClientSession` with top-level import type - Use `toHaveLength` for spy assertions in playwright-expect spec file - Replace numbered Record casts with `.not.toHaveProperty()` in role.methods.spec for SHARED_GLOBAL assertions - Use per-test ObjectIds instead of shared testUserId in openid.spec - Replace inline `import()` type annotations with top-level SessionData import in sessionCache spec - Remove extraneous blank line in user.ts searchUsers * refactor: address remaining review findings (4–7) - Extract OIDCTokens interface in user.ts; deduplicate across IUser fields and oidc.ts FederatedTokens (Finding 4) - Move String.isWellFormed declaration from spec file to project-level src/types/es2024-string.d.ts (Finding 5) - Replace verbose `= undefined` defaults in OGDialogTemplate with null coalescing pattern (Finding 6) - Replace `Record<string, unknown>` TestConfig with named interface containing explicit test fields (Finding 7)
1 parent d5c7d9f commit fda1bfc

38 files changed

+406
-233
lines changed

‎.github/workflows/backend-review.yml‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,65 @@ jobs:
9797
path: packages/api/dist
9898
retention-days: 2
9999

100+
typecheck:
101+
name: TypeScript type checks
102+
needs: build
103+
runs-on: ubuntu-latest
104+
timeout-minutes: 10
105+
steps:
106+
- uses: actions/checkout@v4
107+
108+
- name: Use Node.js 20.19
109+
uses: actions/setup-node@v4
110+
with:
111+
node-version: '20.19'
112+
113+
- name: Restore node_modules cache
114+
id: cache-node-modules
115+
uses: actions/cache@v4
116+
with:
117+
path: |
118+
node_modules
119+
api/node_modules
120+
packages/api/node_modules
121+
packages/data-provider/node_modules
122+
packages/data-schemas/node_modules
123+
key: node-modules-backend-${{ runner.os }}-20.19-${{ hashFiles('package-lock.json') }}
124+
125+
- name: Install dependencies
126+
if: steps.cache-node-modules.outputs.cache-hit != 'true'
127+
run: npm ci
128+
129+
- name: Download data-provider build
130+
uses: actions/download-artifact@v4
131+
with:
132+
name: build-data-provider
133+
path: packages/data-provider/dist
134+
135+
- name: Download data-schemas build
136+
uses: actions/download-artifact@v4
137+
with:
138+
name: build-data-schemas
139+
path: packages/data-schemas/dist
140+
141+
- name: Download api build
142+
uses: actions/download-artifact@v4
143+
with:
144+
name: build-api
145+
path: packages/api/dist
146+
147+
- name: Type check data-provider
148+
run: npx tsc --noEmit -p packages/data-provider/tsconfig.json
149+
150+
- name: Type check data-schemas
151+
run: npx tsc --noEmit -p packages/data-schemas/tsconfig.json
152+
153+
- name: Type check @librechat/api
154+
run: npx tsc --noEmit -p packages/api/tsconfig.json
155+
156+
- name: Type check @librechat/client
157+
run: npx tsc --noEmit -p packages/client/tsconfig.json
158+
100159
circular-deps:
101160
name: Circular dependency checks
102161
needs: build

‎packages/api/src/admin/config.handler.spec.ts‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1+
import type { Response } from 'express';
2+
import type { ServerRequest } from '~/types/http';
13
import { createAdminConfigHandlers } from './config';
24

35
function mockReq(overrides = {}) {
@@ -7,23 +9,30 @@ function mockReq(overrides = {}) {
79
body: {},
810
query: {},
911
...overrides,
10-
};
12+
} as Partial<ServerRequest> as ServerRequest;
13+
}
14+
15+
interface MockRes {
16+
statusCode: number;
17+
body: undefined | { config?: unknown; error?: string; [key: string]: unknown };
18+
status: jest.Mock;
19+
json: jest.Mock;
1120
}
1221

1322
function mockRes() {
14-
const res = {
23+
const res: MockRes = {
1524
statusCode: 200,
1625
body: undefined,
17-
status: jest.fn((code) => {
26+
status: jest.fn((code: number) => {
1827
res.statusCode = code;
1928
return res;
2029
}),
21-
json: jest.fn((data) => {
30+
json: jest.fn((data: MockRes['body']) => {
2231
res.body = data;
2332
return res;
2433
}),
2534
};
26-
return res;
35+
return res as Partial<Response> as Response & MockRes;
2736
}
2837

2938
function createHandlers(overrides = {}) {
@@ -93,7 +102,7 @@ describe('createAdminConfigHandlers', () => {
93102
await handlers.getConfig(req, res);
94103

95104
expect(res.statusCode).toBe(200);
96-
expect(res.body.config).toEqual(config);
105+
expect(res.body!.config).toEqual(config);
97106
});
98107

99108
it('returns 400 for invalid principalType', async () => {
@@ -191,7 +200,7 @@ describe('createAdminConfigHandlers', () => {
191200
await handlers.deleteConfigField(req, res);
192201

193202
expect(res.statusCode).toBe(400);
194-
expect(res.body.error).toContain('query parameter');
203+
expect(res.body!.error).toContain('query parameter');
195204
});
196205

197206
it('rejects unsafe field paths', async () => {
@@ -408,7 +417,7 @@ describe('createAdminConfigHandlers', () => {
408417
await handlers.getBaseConfig(req, res);
409418

410419
expect(res.statusCode).toBe(200);
411-
expect(res.body.config).toEqual({ interface: { endpointsMenu: true } });
420+
expect(res.body!.config).toEqual({ interface: { endpointsMenu: true } });
412421
});
413422
});
414423
});

‎packages/api/src/app/service.spec.ts‎

Lines changed: 17 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,13 @@
1+
import type { AppConfig } from '@librechat/data-schemas';
12
import { createAppConfigService } from './service';
23

4+
/** Extends AppConfig with mock fields used by merge behavior tests. */
5+
interface TestConfig extends AppConfig {
6+
restricted?: boolean;
7+
x?: string;
8+
interface?: { endpointsMenu?: boolean; [key: string]: boolean | undefined };
9+
}
10+
311
/**
412
* Creates a mock cache that simulates Keyv's namespace behavior.
513
* Keyv stores keys internally as `namespace:key` but its API (get/set/delete)
@@ -18,7 +26,9 @@ function createMockCache(namespace = 'app_config') {
1826
return Promise.resolve(true);
1927
}),
2028
/** Mimic Keyv's opts.store structure for key enumeration in clearOverrideCache */
21-
opts: { store: { keys: () => store.keys() } },
29+
opts: { store: { keys: () => store.keys() } } as {
30+
store?: { keys: () => IterableIterator<string> };
31+
},
2232
_store: store,
2333
};
2434
}
@@ -123,8 +133,10 @@ describe('createAppConfigService', () => {
123133

124134
const config = await getAppConfig({ role: 'ADMIN' });
125135

126-
expect(config.interface.endpointsMenu).toBe(false);
127-
expect(config.endpoints).toEqual(['openAI']);
136+
// Test data uses mock fields that don't exist on AppConfig to verify merge behavior
137+
const merged = config as TestConfig;
138+
expect(merged.interface?.endpointsMenu).toBe(false);
139+
expect(merged.endpoints).toEqual(['openAI']);
128140
});
129141

130142
it('caches merged result with TTL', async () => {
@@ -199,7 +211,7 @@ describe('createAppConfigService', () => {
199211
const config = await getAppConfig({ role: 'ADMIN' });
200212

201213
expect(mockGetConfigs).toHaveBeenCalledTimes(2);
202-
expect((config as Record<string, unknown>).restricted).toBe(true);
214+
expect((config as TestConfig).restricted).toBe(true);
203215
});
204216

205217
it('does not short-circuit other users when one user has no overrides', async () => {
@@ -216,7 +228,7 @@ describe('createAppConfigService', () => {
216228
const config = await getAppConfig({ role: 'ADMIN' });
217229

218230
expect(mockGetConfigs).toHaveBeenCalledTimes(2);
219-
expect((config as Record<string, unknown>).x).toBe('admin-only');
231+
expect((config as TestConfig).x).toBe('admin-only');
220232
});
221233

222234
it('falls back to base config on getApplicableConfigs error', async () => {

‎packages/api/src/auth/openid.spec.ts‎

Lines changed: 23 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
1+
import { Types } from 'mongoose';
12
import { ErrorTypes } from 'librechat-data-provider';
23
import { logger } from '@librechat/data-schemas';
34
import type { IUser, UserMethods } from '@librechat/data-schemas';
45
import { findOpenIDUser } from './openid';
56

7+
function newId() {
8+
return new Types.ObjectId();
9+
}
10+
611
jest.mock('@librechat/data-schemas', () => ({
712
...jest.requireActual('@librechat/data-schemas'),
813
logger: {
@@ -24,7 +29,7 @@ describe('findOpenIDUser', () => {
2429
describe('Primary condition searches', () => {
2530
it('should find user by openidId', async () => {
2631
const mockUser: IUser = {
27-
_id: 'user123',
32+
_id: newId(),
2833
provider: 'openid',
2934
openidId: 'openid_123',
3035
email: 'user@example.com',
@@ -51,7 +56,7 @@ describe('findOpenIDUser', () => {
5156

5257
it('should find user by idOnTheSource', async () => {
5358
const mockUser: IUser = {
54-
_id: 'user123',
59+
_id: newId(),
5560
provider: 'openid',
5661
idOnTheSource: 'source_123',
5762
email: 'user@example.com',
@@ -78,7 +83,7 @@ describe('findOpenIDUser', () => {
7883

7984
it('should find user by both openidId and idOnTheSource', async () => {
8085
const mockUser: IUser = {
81-
_id: 'user123',
86+
_id: newId(),
8287
provider: 'openid',
8388
openidId: 'openid_123',
8489
idOnTheSource: 'source_123',
@@ -109,16 +114,14 @@ describe('findOpenIDUser', () => {
109114
describe('Email-based searches', () => {
110115
it('should find user by email when primary conditions fail and openidId matches', async () => {
111116
const mockUser: IUser = {
112-
_id: 'user123',
117+
_id: newId(),
113118
provider: 'openid',
114119
openidId: 'openid_123',
115120
email: 'user@example.com',
116121
username: 'testuser',
117122
} as IUser;
118123

119-
mockFindUser
120-
.mockResolvedValueOnce(null)
121-
.mockResolvedValueOnce(mockUser);
124+
mockFindUser.mockResolvedValueOnce(null).mockResolvedValueOnce(mockUser);
122125

123126
const result = await findOpenIDUser({
124127
openidId: 'openid_123',
@@ -179,7 +182,7 @@ describe('findOpenIDUser', () => {
179182
describe('Provider conflict handling', () => {
180183
it('should return error when user has different provider', async () => {
181184
const mockUser: IUser = {
182-
_id: 'user123',
185+
_id: newId(),
183186
provider: 'google',
184187
email: 'user@example.com',
185188
username: 'testuser',
@@ -204,16 +207,14 @@ describe('findOpenIDUser', () => {
204207

205208
it('should reject email fallback when existing openidId does not match token sub', async () => {
206209
const mockUser: IUser = {
207-
_id: 'user123',
210+
_id: newId(),
208211
provider: 'openid',
209212
openidId: 'openid_456',
210213
email: 'user@example.com',
211214
username: 'testuser',
212215
} as IUser;
213216

214-
mockFindUser
215-
.mockResolvedValueOnce(null)
216-
.mockResolvedValueOnce(mockUser);
217+
mockFindUser.mockResolvedValueOnce(null).mockResolvedValueOnce(mockUser);
217218

218219
const result = await findOpenIDUser({
219220
openidId: 'openid_123',
@@ -230,16 +231,14 @@ describe('findOpenIDUser', () => {
230231

231232
it('should allow email fallback when existing openidId matches token sub', async () => {
232233
const mockUser: IUser = {
233-
_id: 'user123',
234+
_id: newId(),
234235
provider: 'openid',
235236
openidId: 'openid_123',
236237
email: 'user@example.com',
237238
username: 'testuser',
238239
} as IUser;
239240

240-
mockFindUser
241-
.mockResolvedValueOnce(null)
242-
.mockResolvedValueOnce(mockUser);
241+
mockFindUser.mockResolvedValueOnce(null).mockResolvedValueOnce(mockUser);
243242

244243
const result = await findOpenIDUser({
245244
openidId: 'openid_123',
@@ -258,7 +257,7 @@ describe('findOpenIDUser', () => {
258257
describe('User migration scenarios', () => {
259258
it('should prepare user for migration when email exists without openidId', async () => {
260259
const mockUser: IUser = {
261-
_id: 'user123',
260+
_id: newId(),
262261
email: 'user@example.com',
263262
username: 'testuser',
264263
// No provider and no openidId - needs migration
@@ -287,16 +286,14 @@ describe('findOpenIDUser', () => {
287286

288287
it('should reject when user already has a different openidId', async () => {
289288
const mockUser: IUser = {
290-
_id: 'user123',
289+
_id: newId(),
291290
provider: 'openid',
292291
openidId: 'existing_openid',
293292
email: 'user@example.com',
294293
username: 'testuser',
295294
} as IUser;
296295

297-
mockFindUser
298-
.mockResolvedValueOnce(null)
299-
.mockResolvedValueOnce(mockUser);
296+
mockFindUser.mockResolvedValueOnce(null).mockResolvedValueOnce(mockUser);
300297

301298
const result = await findOpenIDUser({
302299
openidId: 'openid_123',
@@ -313,16 +310,14 @@ describe('findOpenIDUser', () => {
313310

314311
it('should reject when user has no provider but a different openidId', async () => {
315312
const mockUser: IUser = {
316-
_id: 'user123',
313+
_id: newId(),
317314
openidId: 'existing_openid',
318315
email: 'user@example.com',
319316
username: 'testuser',
320317
// No provider field — tests a different branch than openid-provider mismatch
321318
} as IUser;
322319

323-
mockFindUser
324-
.mockResolvedValueOnce(null)
325-
.mockResolvedValueOnce(mockUser);
320+
mockFindUser.mockResolvedValueOnce(null).mockResolvedValueOnce(mockUser);
326321

327322
const result = await findOpenIDUser({
328323
openidId: 'openid_123',
@@ -422,16 +417,14 @@ describe('findOpenIDUser', () => {
422417

423418
it('should pass email to findUser for case-insensitive lookup (findUser handles normalization)', async () => {
424419
const mockUser: IUser = {
425-
_id: 'user123',
420+
_id: newId(),
426421
provider: 'openid',
427422
openidId: 'openid_123',
428423
email: 'user@example.com',
429424
username: 'testuser',
430425
} as IUser;
431426

432-
mockFindUser
433-
.mockResolvedValueOnce(null)
434-
.mockResolvedValueOnce(mockUser);
427+
mockFindUser.mockResolvedValueOnce(null).mockResolvedValueOnce(mockUser);
435428

436429
const result = await findOpenIDUser({
437430
openidId: 'openid_123',
@@ -460,7 +453,7 @@ describe('findOpenIDUser', () => {
460453

461454
it('should reject email fallback when openidId is empty and user has a stored openidId', async () => {
462455
const mockUser: IUser = {
463-
_id: 'user123',
456+
_id: newId(),
464457
provider: 'openid',
465458
openidId: 'existing-real-id',
466459
email: 'user@example.com',

0 commit comments

Comments
 (0)