Skip to content

Commit a10b7d1

Browse files
author
David Ruzicka
committed
fix(03.1-recheck): address post-review gaps from second recheck pass
- Add mixed-failure ZodError test (upstream_mcp array + missing profile_name must re-throw raw ZodError, not wrap as ValidationError) - Add validation_timeout_ms float rejection test (1.5 → rejects as non-integer) - Strengthen weak http-transport skip-validation assertion (not.toBe(500) → toBe(200)) - Simplify generic-profile.test.ts upstream proxy detection to !!profile.upstream_mcp (loaded profile can never carry legacy array; remove hasUpstreamMcpFlag dependency) - Extend MIGRATION-CLEANUP comment to include UPSTREAM_MCP_ARRAY_REJECTION_MESSAGE export as third required cleanup site Agent authored
1 parent 391664b commit a10b7d1

5 files changed

Lines changed: 40 additions & 7 deletions

File tree

src/profile/profile-loader.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3357,6 +3357,29 @@ paths:
33573357
expect(err.message).toContain('upstream_mcp schema validation failed');
33583358
});
33593359

3360+
it('re-throws raw ZodError when upstream_mcp array fails alongside other field errors', async () => {
3361+
const loader = new ProfileLoader();
3362+
const fs = await import('fs/promises');
3363+
const tmpPath = `/tmp/profile-upstream-mixed-${Date.now()}-${Math.random()}.json`;
3364+
await fs.writeFile(
3365+
tmpPath,
3366+
JSON.stringify({
3367+
// profile_name intentionally omitted — triggers a separate Zod issue
3368+
tools: [],
3369+
upstream_mcp: [{ name: 'p', transport: { type: 'http-streamable', url: 'https://u.example.com/mcp' } }],
3370+
}),
3371+
'utf-8',
3372+
);
3373+
expect.assertions(2);
3374+
const err = await loader.load(tmpPath).catch((e) => e);
3375+
// Mixed failure: both upstream_mcp (array) and profile_name (missing) fail Zod.
3376+
// The wrapping condition requires ALL issues to be upstream_mcp-rooted, so the
3377+
// raw ZodError must be re-thrown to preserve the profile_name error.
3378+
const { ZodError } = await import('zod');
3379+
expect(err).toBeInstanceOf(ZodError);
3380+
expect((err as import('zod').ZodError).issues.some((i) => i.path[0] !== 'upstream_mcp')).toBe(true);
3381+
});
3382+
33603383
it('loads profile with single-object upstream_mcp', async () => {
33613384
const loader = new ProfileLoader();
33623385
const fs = await import('fs/promises');

src/profile/upstream-mcp-config.test.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,15 @@ describe('resolveUpstreamMcpConfig – validator error branches', () => {
224224
expect(() => resolveUpstreamMcpConfig(profile)).toThrow(/validation_timeout_ms must be a positive integer/);
225225
});
226226

227+
it('rejects non-integer validation_timeout_ms', () => {
228+
const profile = makeProfile({
229+
name: 'p1',
230+
transport: { type: 'http-streamable', url: 'https://example.com/mcp' },
231+
validation_timeout_ms: 1.5,
232+
});
233+
expect(() => resolveUpstreamMcpConfig(profile)).toThrow(/validation_timeout_ms must be a positive integer/);
234+
});
235+
227236
it('rejects empty tool_prefix', () => {
228237
const profile = makeProfile({
229238
name: 'p1',

src/profile/upstream-mcp-config.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -228,10 +228,11 @@ export function looksLikeUpstreamMcpProxy(value: unknown): boolean {
228228

229229
// MIGRATION-CLEANUP(phase-03.1): remove this function and all its callers once
230230
// all deployed profiles have been migrated to singular upstream_mcp object.
231-
// Both cleanup sites (this function + the Array.isArray branch in profile-resolver.ts
232-
// extractEnvVars) MUST be removed together — they serve the same migration window.
233-
// Removal: grep -rn "MIGRATION-CLEANUP(phase-03.1)" src/ — 2 code sites (grep returns 3 lines
234-
// because this comment block spans 2 lines; the third hit is in profile-resolver.ts).
231+
// Cleanup sites (must all be removed together):
232+
// 1. This function (hasUpstreamMcpFlag)
233+
// 2. Array.isArray branch in profile-resolver.ts extractEnvVars
234+
// 3. UPSTREAM_MCP_ARRAY_REJECTION_MESSAGE export + its import in profile-loader.ts
235+
// Removal: grep -rn "MIGRATION-CLEANUP(phase-03.1)" src/ — 3 code sites.
235236
/**
236237
* Returns true if the raw profile.upstream_mcp value indicates an upstream MCP
237238
* provider is configured. Tolerates BOTH the legacy array shape and the post

src/testing/generic-profile.test.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import { loadTestDefinitionSync, validateTestAgainstProfile } from './test-loade
88
import { processTemplate } from './template-utils.js';
99
import { Profile } from '../types/profile.js';
1010
import { ProfileLoader } from '../profile/profile-loader.js';
11-
import { hasUpstreamMcpFlag } from '../profile/upstream-mcp-config.js';
1211
import { assertRequestMatches, assertRequestsSequence } from './request-assertions.js';
1312

1413
type ToolCallResponse = {
@@ -243,7 +242,7 @@ testFiles.forEach(testFile => {
243242
validateTestAgainstProfile(testDef, profile);
244243

245244
const isUpstreamProxy =
246-
hasUpstreamMcpFlag(profile.upstream_mcp) &&
245+
!!profile.upstream_mcp &&
247246
!profile.openapi_spec_path;
248247

249248
server = new MCPServer();

src/transport/http-transport.upstream-validation.test.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,7 @@ describeIfListen('upstream credential validation at session init', () => {
342342
.send(INIT_REQUEST);
343343

344344
// Should succeed (session created) since no validation manager
345-
expect(res.status).not.toBe(500);
345+
expect(res.status).toBe(200);
346+
expect(res.body).not.toHaveProperty('error');
346347
});
347348
});

0 commit comments

Comments
 (0)