Skip to content

Commit ca29452

Browse files
ersinkocclaude
andcommitted
πŸ”§ fix(config-center): harden validation and fix multi-entry availability
- Add required field validation for config entries (POST + PUT routes) with merged-data check for partial updates - Fix isAvailable() to check ANY entry, not just default β€” multi-entry services now correctly report availability - Add proper configSchema structure validation (Zod schema for ConfigFieldDefinition with type enum, max lengths, options) - Remove getApiKey() async auto-register race condition β€” tools register config requirements via registerToolConfigRequirements instead - Add 4 new route tests for required field validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 89e32b7 commit ca29452

7 files changed

Lines changed: 164 additions & 41 deletions

File tree

β€Žpackages/gateway/src/db/repositories/config-services.test.tsβ€Ž

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1268,13 +1268,14 @@ describe('ConfigServicesRepository', () => {
12681268
expect(repo.isAvailable('openai')).toBe(false);
12691269
});
12701270

1271-
it('should return false when no default entry', async () => {
1271+
it('should return true when non-default entry has data', async () => {
12721272
mockAdapter.query
12731273
.mockResolvedValueOnce([makeServiceRow()])
12741274
.mockResolvedValueOnce([makeEntryRow({ is_default: false })]);
12751275
await repo.initialize();
12761276

1277-
expect(repo.isAvailable('openai')).toBe(false);
1277+
// isAvailable checks ANY entry with data, not just default
1278+
expect(repo.isAvailable('openai')).toBe(true);
12781279
});
12791280

12801281
it('should return false when service not found', () => {

β€Žpackages/gateway/src/db/repositories/config-services.tsβ€Ž

Lines changed: 10 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -685,21 +685,22 @@ export class ConfigServicesRepository extends BaseRepository {
685685
/**
686686
* Check whether a service is configured and available.
687687
*
688-
* A service is available when it has a default entry whose data
689-
* contains at least one non-empty value.
688+
* A service is available when it is active and has ANY entry (default
689+
* or otherwise) whose data contains at least one non-empty value.
690690
*/
691691
isAvailable(serviceName: string): boolean {
692692
const svc = cache.services.get(serviceName);
693693
if (!svc || !svc.isActive) return false;
694694

695-
const defaultEntry = this.getDefaultEntry(serviceName);
696-
if (!defaultEntry) return false;
695+
const entries = cache.entries.get(serviceName);
696+
if (!entries || entries.length === 0) return false;
697697

698-
// Check if any data field has a non-empty value
699-
const data = defaultEntry.data;
700-
for (const key of Object.keys(data)) {
701-
const val = data[key];
702-
if (val !== null && val !== undefined && val !== '') return true;
698+
// Check if any entry has at least one non-empty data field
699+
for (const entry of entries) {
700+
for (const key of Object.keys(entry.data)) {
701+
const val = entry.data[key];
702+
if (val !== null && val !== undefined && val !== '') return true;
703+
}
703704
}
704705

705706
return false;

β€Žpackages/gateway/src/middleware/validation.tsβ€Ž

Lines changed: 17 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -761,7 +761,23 @@ export const createConfigServiceSchema = z.object({
761761
displayName: z.string().min(1).max(200),
762762
category: z.string().min(1).max(100),
763763
description: z.string().max(2000).optional(),
764-
configSchema: z.array(z.record(z.string(), z.unknown())).optional(),
764+
configSchema: z
765+
.array(
766+
z.object({
767+
name: z.string().min(1).max(100),
768+
label: z.string().max(200).optional(),
769+
type: z.enum(['string', 'secret', 'url', 'number', 'boolean', 'select', 'json']),
770+
description: z.string().max(1000).optional(),
771+
required: z.boolean().optional(),
772+
defaultValue: z.union([z.string(), z.number(), z.boolean(), z.null()]).optional(),
773+
envVar: z.string().max(200).optional(),
774+
placeholder: z.string().max(500).optional(),
775+
options: z.array(z.object({ label: z.string(), value: z.string() })).optional(),
776+
order: z.number().int().min(0).max(1000).optional(),
777+
})
778+
)
779+
.max(50)
780+
.optional(),
765781
requiredBy: z.array(z.string().max(200)).optional(),
766782
docsUrl: z.string().max(2000).optional(),
767783
});

β€Žpackages/gateway/src/routes/config-services.test.tsβ€Ž

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -519,4 +519,78 @@ describe('Config Services Routes', () => {
519519
expect(res.status).toBe(404);
520520
});
521521
});
522+
523+
// ========================================================================
524+
// Required field validation
525+
// ========================================================================
526+
527+
describe('required field validation', () => {
528+
const serviceWithRequired = {
529+
...sampleService,
530+
configSchema: [
531+
{ name: 'api_key', type: 'secret', label: 'API Key', required: true },
532+
{ name: 'region', type: 'text', label: 'Region', required: false },
533+
],
534+
};
535+
536+
it('POST entry returns 400 when required field missing', async () => {
537+
mockConfigServicesRepo.getByName.mockReturnValue(serviceWithRequired);
538+
539+
const res = await app.request('/config-services/gmail/entries', {
540+
method: 'POST',
541+
headers: { 'Content-Type': 'application/json' },
542+
body: JSON.stringify({ data: { region: 'us-east-1' } }),
543+
});
544+
545+
expect(res.status).toBe(400);
546+
const json = await res.json();
547+
expect(json.error.message).toContain('API Key');
548+
});
549+
550+
it('POST entry allows when required field present', async () => {
551+
mockConfigServicesRepo.getByName.mockReturnValue(serviceWithRequired);
552+
mockConfigServicesRepo.createEntry.mockResolvedValue(sampleEntry);
553+
554+
const res = await app.request('/config-services/gmail/entries', {
555+
method: 'POST',
556+
headers: { 'Content-Type': 'application/json' },
557+
body: JSON.stringify({ data: { api_key: 'sk-test' } }),
558+
});
559+
560+
expect(res.status).toBe(201);
561+
});
562+
563+
it('PUT entry returns 400 when required field cleared', async () => {
564+
mockConfigServicesRepo.getByName.mockReturnValue(serviceWithRequired);
565+
mockConfigServicesRepo.getEntries.mockReturnValue([
566+
{ ...sampleEntry, data: { api_key: 'sk-old', region: 'us-east-1' } },
567+
]);
568+
569+
const res = await app.request('/config-services/gmail/entries/entry-1', {
570+
method: 'PUT',
571+
headers: { 'Content-Type': 'application/json' },
572+
body: JSON.stringify({ data: { api_key: '' } }),
573+
});
574+
575+
expect(res.status).toBe(400);
576+
const json = await res.json();
577+
expect(json.error.message).toContain('API Key');
578+
});
579+
580+
it('PUT entry allows partial update with required field intact', async () => {
581+
mockConfigServicesRepo.getByName.mockReturnValue(serviceWithRequired);
582+
mockConfigServicesRepo.getEntries.mockReturnValue([
583+
{ ...sampleEntry, data: { api_key: 'sk-old', region: 'us-east-1' } },
584+
]);
585+
mockConfigServicesRepo.updateEntry.mockResolvedValue(sampleEntry);
586+
587+
const res = await app.request('/config-services/gmail/entries/entry-1', {
588+
method: 'PUT',
589+
headers: { 'Content-Type': 'application/json' },
590+
body: JSON.stringify({ data: { region: 'eu-west-1' } }),
591+
});
592+
593+
expect(res.status).toBe(200);
594+
});
595+
});
522596
});

β€Žpackages/gateway/src/routes/config-services.tsβ€Ž

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,25 @@ export const configServicesRoutes = new Hono();
3232
// HELPERS
3333
// =============================================================================
3434

35+
/**
36+
* Validate entry data against service schema's required fields.
37+
* Returns array of missing required field names (empty = valid).
38+
*/
39+
function validateRequiredFields(
40+
data: Record<string, unknown>,
41+
schema: ConfigFieldDefinition[]
42+
): string[] {
43+
const missing: string[] = [];
44+
for (const field of schema) {
45+
if (!field.required) continue;
46+
const val = data[field.name];
47+
if (val === undefined || val === null || val === '') {
48+
missing.push(field.label || field.name);
49+
}
50+
}
51+
return missing;
52+
}
53+
3554
/**
3655
* Detect if a value looks like it was masked by maskSecret().
3756
* Matches patterns like "abcd...wxyz" or "****".
@@ -256,6 +275,22 @@ configServicesRoutes.post('/:name/entries', async (c) => {
256275
}
257276

258277
const body = await c.req.json<CreateConfigEntryInput>();
278+
279+
// Validate required fields
280+
if (body.data && service.configSchema.length > 0) {
281+
const missing = validateRequiredFields(body.data, service.configSchema);
282+
if (missing.length > 0) {
283+
return apiError(
284+
c,
285+
{
286+
code: ERROR_CODES.VALIDATION_ERROR,
287+
message: `Missing required fields: ${missing.join(', ')}`,
288+
},
289+
400
290+
);
291+
}
292+
}
293+
259294
const entry = await configServicesRepo.createEntry(name, body);
260295

261296
wsGateway.broadcast('data:changed', { entity: 'config_service', action: 'updated', id: name });
@@ -296,6 +331,23 @@ configServicesRoutes.put('/:name/entries/:entryId', async (c) => {
296331
}
297332
}
298333

334+
// Validate required fields (merge with existing data to allow partial updates)
335+
if (body.data && service.configSchema.length > 0) {
336+
const existingData = configServicesRepo.getEntries(name).find((e) => e.id === entryId)?.data ?? {};
337+
const mergedData = { ...existingData, ...body.data };
338+
const missing = validateRequiredFields(mergedData, service.configSchema);
339+
if (missing.length > 0) {
340+
return apiError(
341+
c,
342+
{
343+
code: ERROR_CODES.VALIDATION_ERROR,
344+
message: `Missing required fields: ${missing.join(', ')}`,
345+
},
346+
400
347+
);
348+
}
349+
}
350+
299351
const updated = await configServicesRepo.updateEntry(entryId, body);
300352
if (!updated) {
301353
return notFoundError(c, 'Config entry', entryId);

β€Žpackages/gateway/src/services/config-center-impl.test.tsβ€Ž

Lines changed: 5 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -81,27 +81,15 @@ describe('GatewayConfigCenter', () => {
8181
expect(center.getApiKey('openai')).toBeUndefined();
8282
});
8383

84-
it('auto-registers unknown service via upsert', () => {
84+
it('returns undefined for unregistered service without throwing', () => {
8585
mockConfigServicesRepo.getApiKey.mockReturnValue(undefined);
8686
mockConfigServicesRepo.getByName.mockReturnValue(null);
87-
mockConfigServicesRepo.upsert.mockResolvedValue({});
8887

89-
center.getApiKey('new-service');
88+
const key = center.getApiKey('unknown-service');
9089

91-
expect(mockConfigServicesRepo.upsert).toHaveBeenCalledWith({
92-
name: 'new-service',
93-
displayName: 'new-service',
94-
category: 'general',
95-
description: 'Auto-discovered service',
96-
});
97-
});
98-
99-
it('does not throw when upsert fails', () => {
100-
mockConfigServicesRepo.getApiKey.mockReturnValue(undefined);
101-
mockConfigServicesRepo.getByName.mockReturnValue(null);
102-
mockConfigServicesRepo.upsert.mockRejectedValue(new Error('DB error'));
103-
104-
expect(() => center.getApiKey('failing-service')).not.toThrow();
90+
expect(key).toBeUndefined();
91+
// Should NOT auto-register β€” tools register via registerToolConfigRequirements
92+
expect(mockConfigServicesRepo.upsert).not.toHaveBeenCalled();
10593
});
10694
});
10795

β€Žpackages/gateway/src/services/config-center-impl.tsβ€Ž

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -56,18 +56,9 @@ export class GatewayConfigCenter implements ConfigCenter {
5656
getApiKey(serviceName: string): string | undefined {
5757
const key = configServicesRepo.getApiKey(serviceName);
5858

59-
// Runtime auto-detection: if service doesn't exist at all, register it (fire-and-forget)
60-
if (!configServicesRepo.getByName(serviceName)) {
61-
configServicesRepo
62-
.upsert({
63-
name: serviceName,
64-
displayName: serviceName,
65-
category: 'general',
66-
description: 'Auto-discovered service',
67-
})
68-
.catch((err) => {
69-
log.warn(`Auto-register service "${serviceName}" failed`, { error: String(err) });
70-
});
59+
// Log when unknown service is accessed β€” tools should register via registerToolConfigRequirements
60+
if (!key && !configServicesRepo.getByName(serviceName)) {
61+
log.debug(`getApiKey called for unregistered service "${serviceName}"`);
7162
}
7263

7364
return key;

0 commit comments

Comments
Β (0)