Skip to content

Commit 34754a8

Browse files
authored
Merge pull request #308321 from abadawi591/abadawi/send-has-image-to-router
Abadawi/send has image to router
2 parents a4ba368 + b2eee58 commit 34754a8

File tree

3 files changed

+187
-57
lines changed

3 files changed

+187
-57
lines changed

extensions/copilot/src/platform/endpoint/node/automodeService.ts

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,7 @@ import { IExperimentationService } from '../../telemetry/common/nullExperimentat
2222
import { ITelemetryService } from '../../telemetry/common/telemetry';
2323
import { ICAPIClientService } from '../common/capiClient';
2424
import { AutoChatEndpoint } from './autoChatEndpoint';
25-
import { RouterDecisionFetcher, RoutingContextSignals } from './routerDecisionFetcher';
25+
import { RouterDecisionError, RouterDecisionFetcher, RoutingContextSignals } from './routerDecisionFetcher';
2626

2727
interface AutoModeAPIResponse {
2828
available_models: string[];
@@ -201,11 +201,13 @@ export class AutomodeService extends Disposable implements IAutomodeService {
201201
"automode.routerFallback" : {
202202
"owner": "lramos15",
203203
"comment": "Reports when the auto mode router is skipped or fails and falls back to default model selection",
204-
"reason": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "comment": "The reason the router was skipped or failed (hasImage, noMatchingEndpoint, routerError, routerTimeout)" }
204+
"reason": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "comment": "The reason the router was skipped or failed, e.g. emptyPrompt, emptyCandidateList, noMatchingEndpoint, routerError, routerTimeout, or a server error code" },
205+
"hasImage": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "comment": "Whether the request contained an attached image" }
205206
}
206207
*/
207208
this._telemetryService.sendMSFTTelemetryEvent('automode.routerFallback', {
208209
reason: routerFallbackReason,
210+
hasImage: String(hasImage(chatRequest)),
209211
});
210212
}
211213
selectedModel = this._selectDefaultModel(entry?.endpoint?.modelProvider, token.available_models, knownEndpoints);
@@ -252,10 +254,6 @@ export class AutomodeService extends Disposable implements IAutomodeService {
252254
const prompt = chatRequest?.prompt?.trim();
253255
const lastRoutedPrompt = entry?.lastRoutedPrompt ?? prompt;
254256

255-
if (hasImage(chatRequest)) {
256-
return { lastRoutedPrompt, fallbackReason: 'hasImage' };
257-
}
258-
259257
if (!this._isRouterEnabled(chatRequest) || conversationId === 'unknown') {
260258
return { lastRoutedPrompt };
261259
}
@@ -278,7 +276,7 @@ export class AutomodeService extends Disposable implements IAutomodeService {
278276
turn_number: (entry?.turnCount ?? 0) + 1,
279277
};
280278
const routingMethod = this._configurationService.getExperimentBasedConfig(ConfigKey.TeamInternal.AutoModeRoutingMethod, this._expService) || undefined;
281-
const result = await this._routerDecisionFetcher.getRouterDecision(prompt, token.session_token, token.available_models, undefined, contextSignals, chatRequest?.sessionId, chatRequest?.id, routingMethod);
279+
const result = await this._routerDecisionFetcher.getRouterDecision(prompt, token.session_token, token.available_models, undefined, contextSignals, conversationId, chatRequest?.id, routingMethod, hasImage(chatRequest));
282280

283281
if (result.fallback) {
284282
this._logService.info(`[AutomodeService] Router signaled fallback: ${result.fallback_reason ?? 'unknown'}, routing_method=${result.routing_method ?? 'n/a'}`);
@@ -303,7 +301,14 @@ export class AutomodeService extends Disposable implements IAutomodeService {
303301
return { selectedModel, lastRoutedPrompt: prompt };
304302
} catch (e) {
305303
const isTimeout = isAbortError(e);
306-
const fallbackReason = isTimeout ? 'routerTimeout' : 'routerError';
304+
let fallbackReason: string;
305+
if (isTimeout) {
306+
fallbackReason = 'routerTimeout';
307+
} else if (e instanceof RouterDecisionError && e.errorCode) {
308+
fallbackReason = e.errorCode;
309+
} else {
310+
fallbackReason = 'routerError';
311+
}
307312
this._logService.error(`Failed to get routed model for conversation ${conversationId} (${fallbackReason}):`, (e as Error).message);
308313
return { lastRoutedPrompt: prompt, fallbackReason };
309314
}

extensions/copilot/src/platform/endpoint/node/routerDecisionFetcher.ts

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,18 @@ export interface RoutingContextSignals {
3838
prompt_char_count?: number;
3939
}
4040

41+
/**
42+
* Thrown when the router API returns a non-OK HTTP response.
43+
* Carries the parsed `errorCode` from the response body (e.g. `no_vision_models`)
44+
* so callers can classify the failure without string-matching the message.
45+
*/
46+
export class RouterDecisionError extends Error {
47+
override readonly name = 'RouterDecisionError';
48+
constructor(message: string, public readonly errorCode?: string) {
49+
super(message);
50+
}
51+
}
52+
4153
/**
4254
* Fetches routing decisions from a classification API to determine which model should handle a query.
4355
*
@@ -54,7 +66,7 @@ export class RouterDecisionFetcher {
5466
) {
5567
}
5668

57-
async getRouterDecision(query: string, autoModeToken: string, availableModels: string[], stickyThreshold?: number, contextSignals?: RoutingContextSignals, conversationId?: string, vscodeRequestId?: string, routingMethod?: string): Promise<RouterDecisionResponse> {
69+
async getRouterDecision(query: string, autoModeToken: string, availableModels: string[], stickyThreshold?: number, contextSignals?: RoutingContextSignals, conversationId?: string, vscodeRequestId?: string, routingMethod?: string, hasImage?: boolean): Promise<RouterDecisionResponse> {
5870
const startTime = Date.now();
5971
const requestBody: Record<string, unknown> = { prompt: query, available_models: availableModels, ...contextSignals };
6072
if (stickyThreshold !== undefined) {
@@ -63,6 +75,9 @@ export class RouterDecisionFetcher {
6375
if (routingMethod) {
6476
requestBody.routing_method = routingMethod;
6577
}
78+
if (hasImage) {
79+
requestBody.has_image = true;
80+
}
6681
const copilotToken = (await this._authService.getCopilotToken()).token;
6782
const abortController = new AbortController();
6883
const timeout = setTimeout(() => abortController.abort(), 1000);
@@ -82,7 +97,15 @@ export class RouterDecisionFetcher {
8297
}
8398

8499
if (!response.ok) {
85-
throw new Error(`Router decision request failed with status ${response.status}: ${response.statusText}`);
100+
const errorText = await response.text().catch(() => '');
101+
let errorCode: string | undefined;
102+
try {
103+
const parsed = JSON.parse(errorText);
104+
if (typeof parsed === 'object' && parsed !== null && 'error' in parsed && typeof parsed.error === 'string') {
105+
errorCode = parsed.error;
106+
}
107+
} catch { /* not JSON */ }
108+
throw new RouterDecisionError(`Router decision request failed with status ${response.status}: ${response.statusText}`, errorCode);
86109
}
87110

88111
const text = await response.text();

extensions/copilot/src/platform/endpoint/node/test/automodeService.spec.ts

Lines changed: 149 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -688,40 +688,19 @@ describe('AutomodeService', () => {
688688
expect(routerCallCount2).toBe(1);
689689
});
690690

691-
it('should skip router on new turn after a transient fallback reason without invalidation', async () => {
691+
it('should skip router on subsequent turns after image request routed on first turn', async () => {
692692
enableRouter();
693693
const gpt4oEndpoint = createEndpoint('gpt-4o', 'OpenAI', { supportsVision: true });
694694
const claudeEndpoint = createEndpoint('claude-sonnet', 'Anthropic');
695695

696-
(mockCAPIClientService.makeRequest as ReturnType<typeof vi.fn>).mockImplementation((_body: any, opts: any) => {
697-
if (opts?.type === RequestType.ModelRouter) {
698-
return Promise.resolve({
699-
ok: true,
700-
status: 200,
701-
headers: createMockHeaders(),
702-
text: vi.fn().mockResolvedValue(JSON.stringify({
703-
predicted_label: 'needs_reasoning',
704-
confidence: 0.9,
705-
latency_ms: 30,
706-
chosen_model: 'claude-sonnet',
707-
candidate_models: ['claude-sonnet'],
708-
scores: { needs_reasoning: 0.9, no_reasoning: 0.1 },
709-
sticky_override: false
710-
}))
711-
});
712-
}
713-
return Promise.resolve(
714-
makeMockTokenResponse({
715-
available_models: ['claude-sonnet', 'gpt-4o'],
716-
expires_at: Math.floor(Date.now() / 1000) + 3600,
717-
session_token: 'test-token',
718-
})
719-
);
720-
});
696+
mockRouterResponse(
697+
['gpt-4o', 'claude-sonnet'],
698+
{ chosen_model: 'gpt-4o', candidate_models: ['gpt-4o'] }
699+
);
721700

722701
automodeService = createService();
723702

724-
// Turn 1: image request — router is skipped (transient fallback)
703+
// Turn 1: image request — router IS called now
725704
const imageRequest: Partial<ChatRequest> = {
726705
location: ChatLocation.Panel,
727706
prompt: 'describe this image',
@@ -731,22 +710,18 @@ describe('AutomodeService', () => {
731710

732711
await automodeService.resolveAutoModeEndpoint(imageRequest as ChatRequest, [gpt4oEndpoint, claudeEndpoint]);
733712

734-
// Turn 2: same prompt (tool-calling iteration) — router should NOT be called
735-
const samePromptRequest: Partial<ChatRequest> = {
736-
location: ChatLocation.Panel,
737-
prompt: 'describe this image',
738-
sessionId: 'session-transient-fallback',
739-
};
740-
741-
await automodeService.resolveAutoModeEndpoint(samePromptRequest as ChatRequest, [gpt4oEndpoint, claudeEndpoint]);
742-
743-
// Router should not have been called for either turn so far
744-
expect(mockCAPIClientService.makeRequest).not.toHaveBeenCalledWith(
713+
expect(mockCAPIClientService.makeRequest).toHaveBeenCalledWith(
745714
expect.anything(),
746715
expect.objectContaining({ type: RequestType.ModelRouter })
747716
);
717+
// Reset mock call tracking
718+
(mockCAPIClientService.makeRequest as ReturnType<typeof vi.fn>).mockClear();
719+
mockRouterResponse(
720+
['gpt-4o', 'claude-sonnet'],
721+
{ chosen_model: 'gpt-4o', candidate_models: ['gpt-4o'] }
722+
);
748723

749-
// Turn 3: new prompt — router should still NOT be called (skipped after first turn)
724+
// Turn 2: new prompt — router should NOT be called (skipRouter after first turn)
750725
const textRequest: Partial<ChatRequest> = {
751726
location: ChatLocation.Panel,
752727
prompt: 'write a function',
@@ -755,39 +730,166 @@ describe('AutomodeService', () => {
755730

756731
await automodeService.resolveAutoModeEndpoint(textRequest as ChatRequest, [gpt4oEndpoint, claudeEndpoint]);
757732

758-
// Router should not have been called at all
733+
// Router should not have been called on turn 2
759734
expect(mockCAPIClientService.makeRequest).not.toHaveBeenCalledWith(
760735
expect.anything(),
761736
expect.objectContaining({ type: RequestType.ModelRouter })
762737
);
763738
});
764739

765-
it('should skip router for image requests and use default selection', async () => {
740+
it('should send has_image to router for image requests', async () => {
766741
enableRouter();
767742
const gpt4oEndpoint = createEndpoint('gpt-4o', 'OpenAI', { supportsVision: true });
768743
const claudeEndpoint = createEndpoint('claude-sonnet', 'Anthropic');
769744

770745
mockRouterResponse(
771-
['claude-sonnet', 'gpt-4o'],
772-
{ chosen_model: 'claude-sonnet', candidate_models: ['claude-sonnet'] }
746+
['gpt-4o', 'claude-sonnet'],
747+
{ chosen_model: 'gpt-4o', candidate_models: ['gpt-4o'] }
773748
);
774749

775750
automodeService = createService();
776751
const chatRequest: Partial<ChatRequest> = {
777752
location: ChatLocation.Panel,
778753
prompt: 'describe this image',
779-
sessionId: 'session-vision-skip-router',
754+
sessionId: 'session-vision-router',
780755
references: [{ id: 'img', value: { mimeType: 'image/png', data: new Uint8Array() } }] as any
781756
};
782757

783758
const result = await automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [gpt4oEndpoint, claudeEndpoint]);
784-
// Router should be skipped; vision fallback should pick the vision-capable model
785759
expect(result.model).toBe('gpt-4o');
786-
// Verify router was NOT called
787-
expect(mockCAPIClientService.makeRequest).not.toHaveBeenCalledWith(
760+
// Verify router WAS called (not skipped)
761+
const routerCall = (mockCAPIClientService.makeRequest as ReturnType<typeof vi.fn>).mock.calls.find(([, opts]) => opts?.type === RequestType.ModelRouter);
762+
expect(routerCall).toBeDefined();
763+
const [routerRequestBody] = routerCall!;
764+
expect(JSON.parse(routerRequestBody.body).has_image).toBe(true);
765+
});
766+
767+
it('should fall back to vision model when router returns no_vision_models error', async () => {
768+
enableRouter();
769+
const gpt4oEndpoint = createEndpoint('gpt-4o', 'OpenAI', { supportsVision: true });
770+
const claudeEndpoint = createEndpoint('claude-sonnet', 'Anthropic');
771+
772+
(mockCAPIClientService.makeRequest as ReturnType<typeof vi.fn>).mockImplementation((_body: any, opts: any) => {
773+
if (opts?.type === RequestType.ModelRouter) {
774+
return Promise.resolve({
775+
ok: false,
776+
status: 400,
777+
statusText: 'Bad Request',
778+
headers: createMockHeaders(),
779+
text: vi.fn().mockResolvedValue(JSON.stringify({ error: 'no_vision_models' }))
780+
});
781+
}
782+
return Promise.resolve(
783+
makeMockTokenResponse({
784+
available_models: ['gpt-4o', 'claude-sonnet'],
785+
expires_at: Math.floor(Date.now() / 1000) + 3600,
786+
session_token: 'test-token',
787+
})
788+
);
789+
});
790+
791+
automodeService = createService();
792+
const chatRequest: Partial<ChatRequest> = {
793+
location: ChatLocation.Panel,
794+
prompt: 'describe this image',
795+
sessionId: 'session-no-vision',
796+
references: [{ id: 'img', value: { mimeType: 'image/png', data: new Uint8Array() } }] as any
797+
};
798+
799+
const result = await automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [gpt4oEndpoint, claudeEndpoint]);
800+
// Should fall back to default selection, then vision fallback picks gpt-4o
801+
expect(result.model).toBe('gpt-4o');
802+
// Verify the router was called and the error code was passed through from the server
803+
expect(mockCAPIClientService.makeRequest).toHaveBeenCalledWith(
788804
expect.anything(),
789805
expect.objectContaining({ type: RequestType.ModelRouter })
790806
);
807+
expect(mockLogService.error).toHaveBeenCalledWith(
808+
expect.stringContaining('(no_vision_models)'),
809+
expect.anything()
810+
);
811+
});
812+
813+
it('should fall back to routerError when router returns non-JSON error body', async () => {
814+
// When the router returns an HTML error page or other non-JSON body,
815+
// errorCode should be undefined and fallbackReason should be 'routerError'
816+
// — NOT the raw response body leaked into telemetry.
817+
enableRouter();
818+
const gpt4oEndpoint = createEndpoint('gpt-4o', 'OpenAI');
819+
820+
(mockCAPIClientService.makeRequest as ReturnType<typeof vi.fn>).mockImplementation((_body: any, opts: any) => {
821+
if (opts?.type === RequestType.ModelRouter) {
822+
return Promise.resolve({
823+
ok: false,
824+
status: 502,
825+
statusText: 'Bad Gateway',
826+
headers: createMockHeaders(),
827+
text: vi.fn().mockResolvedValue('<html><body>Bad Gateway</body></html>')
828+
});
829+
}
830+
return Promise.resolve(
831+
makeMockTokenResponse({
832+
available_models: ['gpt-4o'],
833+
expires_at: Math.floor(Date.now() / 1000) + 3600,
834+
session_token: 'test-token',
835+
})
836+
);
837+
});
838+
839+
automodeService = createService();
840+
const chatRequest: Partial<ChatRequest> = {
841+
location: ChatLocation.Panel,
842+
prompt: 'test prompt',
843+
sessionId: 'session-html-error',
844+
};
845+
846+
const result = await automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [gpt4oEndpoint]);
847+
expect(result.model).toBe('gpt-4o');
848+
// Should log generic 'routerError', NOT the HTML body
849+
expect(mockLogService.error).toHaveBeenCalledWith(
850+
expect.stringContaining('(routerError)'),
851+
expect.anything()
852+
);
853+
});
854+
855+
it('should fall back to routerError when router returns JSON without error field', async () => {
856+
// When the server returns valid JSON but without an 'error' field,
857+
// errorCode should be undefined and fallbackReason should be 'routerError'.
858+
enableRouter();
859+
const gpt4oEndpoint = createEndpoint('gpt-4o', 'OpenAI');
860+
861+
(mockCAPIClientService.makeRequest as ReturnType<typeof vi.fn>).mockImplementation((_body: any, opts: any) => {
862+
if (opts?.type === RequestType.ModelRouter) {
863+
return Promise.resolve({
864+
ok: false,
865+
status: 400,
866+
statusText: 'Bad Request',
867+
headers: createMockHeaders(),
868+
text: vi.fn().mockResolvedValue(JSON.stringify({ message: 'something went wrong' }))
869+
});
870+
}
871+
return Promise.resolve(
872+
makeMockTokenResponse({
873+
available_models: ['gpt-4o'],
874+
expires_at: Math.floor(Date.now() / 1000) + 3600,
875+
session_token: 'test-token',
876+
})
877+
);
878+
});
879+
880+
automodeService = createService();
881+
const chatRequest: Partial<ChatRequest> = {
882+
location: ChatLocation.Panel,
883+
prompt: 'test prompt',
884+
sessionId: 'session-json-no-error',
885+
};
886+
887+
const result = await automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [gpt4oEndpoint]);
888+
expect(result.model).toBe('gpt-4o');
889+
expect(mockLogService.error).toHaveBeenCalledWith(
890+
expect.stringContaining('(routerError)'),
891+
expect.anything()
892+
);
791893
});
792894

793895
it('should be a no-op when invalidateRouterCache is called with unknown conversationId', async () => {

0 commit comments

Comments
 (0)