Skip to content

Abadawi/send has image to router #308321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abadawi591 wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Abadawi/send has image to router #308321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
155c2ad
Send has_image to router instead of skipping it for vision requests
abadawi591 Apr 7, 2026
a55ff3a
Fix no-explicit-any lint warnings
abadawi591 Apr 7, 2026
9b084c3
List all possible routerFallback reason values in GDPR annotation
abadawi591 Apr 7, 2026
75af0f6
Assert noVisionModels error classification in test
abadawi591 Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions extensions/copilot/src/platform/endpoint/node/automodeService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import { IExperimentationService } from '../../telemetry/common/nullExperimentat
import { ITelemetryService } from '../../telemetry/common/telemetry';
import { ICAPIClientService } from '../common/capiClient';
import { AutoChatEndpoint } from './autoChatEndpoint';
import { RouterDecisionFetcher, RoutingContextSignals } from './routerDecisionFetcher';
import { RouterDecisionError, RouterDecisionFetcher, RoutingContextSignals } from './routerDecisionFetcher';

interface AutoModeAPIResponse {
available_models: string[];
Expand Down Expand Up @@ -201,7 +201,7 @@ export class AutomodeService extends Disposable implements IAutomodeService {
"automode.routerFallback" : {
"owner": "lramos15",
"comment": "Reports when the auto mode router is skipped or fails and falls back to default model selection",
"reason": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "comment": "The reason the router was skipped or failed (hasImage, noMatchingEndpoint, routerError, routerTimeout)" }
"reason": { "classification": "SystemMetaData", "purpose": "PerformanceAndHealth", "comment": "The reason the router was skipped or failed (emptyPrompt, emptyCandidateList, noMatchingEndpoint, noVisionModels, routerError, routerTimeout)" }
}
*/
this._telemetryService.sendMSFTTelemetryEvent('automode.routerFallback', {
Expand Down Expand Up @@ -252,10 +252,6 @@ export class AutomodeService extends Disposable implements IAutomodeService {
const prompt = chatRequest?.prompt?.trim();
const lastRoutedPrompt = entry?.lastRoutedPrompt ?? prompt;

if (hasImage(chatRequest)) {
return { lastRoutedPrompt, fallbackReason: 'hasImage' };
}

if (!this._isRouterEnabled(chatRequest) || conversationId === 'unknown') {
return { lastRoutedPrompt };
}
Expand All @@ -277,7 +273,7 @@ export class AutomodeService extends Disposable implements IAutomodeService {
previous_model: entry?.endpoint?.model,
turn_number: (entry?.turnCount ?? 0) + 1,
};
const result = await this._routerDecisionFetcher.getRouterDecision(prompt, token.session_token, token.available_models, undefined, contextSignals, chatRequest?.sessionId, chatRequest?.id);
const result = await this._routerDecisionFetcher.getRouterDecision(prompt, token.session_token, token.available_models, undefined, contextSignals, chatRequest?.sessionId, chatRequest?.id, hasImage(chatRequest));
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the router call, conversationId is computed earlier as sessionResource?.toString() ?? sessionId, but here chatRequest?.sessionId is passed to getRouterDecision() for the conversationId argument. This can cause router telemetry (and any downstream logging keyed by that argument) to miss the sessionResource-based id. Pass the already-computed conversationId variable instead to keep IDs consistent with contextSignals.session_id and cache keys.

Suggested change
const result = await this._routerDecisionFetcher.getRouterDecision(prompt, token.session_token, token.available_models, undefined, contextSignals, chatRequest?.sessionId, chatRequest?.id, hasImage(chatRequest));
const result = await this._routerDecisionFetcher.getRouterDecision(prompt, token.session_token, token.available_models, undefined, contextSignals, conversationId, chatRequest?.id, hasImage(chatRequest));

Copilot uses AI. Check for mistakes.

if (!result.candidate_models.length) {
return { lastRoutedPrompt: prompt, fallbackReason: 'emptyCandidateList' };
Expand All @@ -297,7 +293,8 @@ export class AutomodeService extends Disposable implements IAutomodeService {
return { selectedModel, lastRoutedPrompt: prompt };
} catch (e) {
const isTimeout = isAbortError(e);
const fallbackReason = isTimeout ? 'routerTimeout' : 'routerError';
const errorCode = e instanceof RouterDecisionError ? e.errorCode : undefined;
const fallbackReason = isTimeout ? 'routerTimeout' : errorCode === 'no_vision_models' ? 'noVisionModels' : 'routerError';
this._logService.error(`Failed to get routed model for conversation ${conversationId} (${fallbackReason}):`, (e as Error).message);
return { lastRoutedPrompt: prompt, fallbackReason };
}
Expand Down
18 changes: 16 additions & 2 deletions extensions/copilot/src/platform/endpoint/node/routerDecisionFetcher.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,12 @@ export interface RoutingContextSignals {
prompt_char_count?: number;
}

export class RouterDecisionError extends Error {
constructor(message: string, public readonly errorCode?: string) {
super(message);
}
}

/**
* Fetches routing decisions from a classification API to determine which model should handle a query.
*
Expand All @@ -48,12 +54,15 @@ export class RouterDecisionFetcher {
) {
}

async getRouterDecision(query: string, autoModeToken: string, availableModels: string[], stickyThreshold?: number, contextSignals?: RoutingContextSignals, conversationId?: string, vscodeRequestId?: string): Promise<RouterDecisionResponse> {
async getRouterDecision(query: string, autoModeToken: string, availableModels: string[], stickyThreshold?: number, contextSignals?: RoutingContextSignals, conversationId?: string, vscodeRequestId?: string, hasImage?: boolean): Promise<RouterDecisionResponse> {
const startTime = Date.now();
const requestBody: Record<string, unknown> = { prompt: query, available_models: availableModels, ...contextSignals };
if (stickyThreshold !== undefined) {
requestBody.sticky_threshold = stickyThreshold;
}
if (hasImage) {
requestBody.has_image = true;
}
const copilotToken = (await this._authService.getCopilotToken()).token;
const abortController = new AbortController();
const timeout = setTimeout(() => abortController.abort(), 1000);
Expand All @@ -73,7 +82,12 @@ export class RouterDecisionFetcher {
}

if (!response.ok) {
throw new Error(`Router decision request failed with status ${response.status}: ${response.statusText}`);
const errorText = await response.text().catch(() => '');
let errorCode: string | undefined;
try {
errorCode = JSON.parse(errorText).error;
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

errorCode = JSON.parse(errorText).error; assigns an untyped value into a string | undefined. If the server ever returns a non-string error, the comparison in AutomodeService will silently fail and misclassify the fallback reason. Consider validating typeof parsed.error === 'string' before assigning.

Suggested change
errorCode = JSON.parse(errorText).error;
const parsed = JSON.parse(errorText);
if (typeof parsed === 'object' && parsed !== null && 'error' in parsed && typeof parsed.error === 'string') {
errorCode = parsed.error;
}

Copilot uses AI. Check for mistakes.
} catch { /* not JSON */ }
throw new RouterDecisionError(`Router decision request failed with status ${response.status}: ${response.statusText}`, errorCode);
}

const text = await response.text();
Expand Down
116 changes: 67 additions & 49 deletions extensions/copilot/src/platform/endpoint/node/test/automodeService.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -688,40 +688,19 @@ describe('AutomodeService', () => {
expect(routerCallCount2).toBe(1);
});

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

(mockCAPIClientService.makeRequest as ReturnType<typeof vi.fn>).mockImplementation((_body: any, opts: any) => {
if (opts?.type === RequestType.ModelRouter) {
return Promise.resolve({
ok: true,
status: 200,
headers: createMockHeaders(),
text: vi.fn().mockResolvedValue(JSON.stringify({
predicted_label: 'needs_reasoning',
confidence: 0.9,
latency_ms: 30,
chosen_model: 'claude-sonnet',
candidate_models: ['claude-sonnet'],
scores: { needs_reasoning: 0.9, no_reasoning: 0.1 },
sticky_override: false
}))
});
}
return Promise.resolve(
makeMockTokenResponse({
available_models: ['claude-sonnet', 'gpt-4o'],
expires_at: Math.floor(Date.now() / 1000) + 3600,
session_token: 'test-token',
})
);
});
mockRouterResponse(
['gpt-4o', 'claude-sonnet'],
{ chosen_model: 'gpt-4o', candidate_models: ['gpt-4o'] }
);

automodeService = createService();

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

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

Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test resets the router mock call history right after turn 1, but never asserts that the router was actually called for the image request (the behavior the test name/comment claims). Add an assertion on turn 1 (before mockClear()) to ensure the test would fail if the router were skipped again.

Suggested change
expect(mockCAPIClientService.makeRequest).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({ type: RequestType.ModelRouter })
);

Copilot uses AI. Check for mistakes.
// Turn 2: same prompt (tool-calling iteration) — router should NOT be called
const samePromptRequest: Partial<ChatRequest> = {
location: ChatLocation.Panel,
prompt: 'describe this image',
sessionId: 'session-transient-fallback',
};

await automodeService.resolveAutoModeEndpoint(samePromptRequest as ChatRequest, [gpt4oEndpoint, claudeEndpoint]);

// Router should not have been called for either turn so far
expect(mockCAPIClientService.makeRequest).not.toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({ type: RequestType.ModelRouter })
// Reset mock call tracking
(mockCAPIClientService.makeRequest as ReturnType<typeof vi.fn>).mockClear();
mockRouterResponse(
['gpt-4o', 'claude-sonnet'],
{ chosen_model: 'gpt-4o', candidate_models: ['gpt-4o'] }
);

// Turn 3: new prompt — router should still NOT be called (skipped after first turn)
// Turn 2: new prompt — router should NOT be called (skipRouter after first turn)
const textRequest: Partial<ChatRequest> = {
location: ChatLocation.Panel,
prompt: 'write a function',
Expand All @@ -755,39 +726,86 @@ describe('AutomodeService', () => {

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

// Router should not have been called at all
// Router should not have been called on turn 2
expect(mockCAPIClientService.makeRequest).not.toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({ type: RequestType.ModelRouter })
);
});

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

mockRouterResponse(
['claude-sonnet', 'gpt-4o'],
{ chosen_model: 'claude-sonnet', candidate_models: ['claude-sonnet'] }
['gpt-4o', 'claude-sonnet'],
{ chosen_model: 'gpt-4o', candidate_models: ['gpt-4o'] }
);

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

const result = await automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [gpt4oEndpoint, claudeEndpoint]);
// Router should be skipped; vision fallback should pick the vision-capable model
expect(result.model).toBe('gpt-4o');
// Verify router was NOT called
expect(mockCAPIClientService.makeRequest).not.toHaveBeenCalledWith(
// Verify router WAS called (not skipped)
expect(mockCAPIClientService.makeRequest).toHaveBeenCalledWith(
expect.objectContaining({
body: expect.stringContaining('"has_image":true')
}),
expect.objectContaining({ type: RequestType.ModelRouter })
);
Comment on lines +757 to +762
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertion checks for a raw substring '"has_image":true' in the request body, which is brittle (JSON spacing / serialization order changes can break it). Prefer parsing the JSON body (or matching via a predicate) and asserting has_image === true.

Suggested change
expect(mockCAPIClientService.makeRequest).toHaveBeenCalledWith(
expect.objectContaining({
body: expect.stringContaining('"has_image":true')
}),
expect.objectContaining({ type: RequestType.ModelRouter })
);
const routerCall = (mockCAPIClientService.makeRequest as ReturnType<typeof vi.fn>).mock.calls.find(([, opts]) => opts?.type === RequestType.ModelRouter);
expect(routerCall).toBeDefined();
const [routerRequestBody] = routerCall!;
expect(JSON.parse(routerRequestBody.body).has_image).toBe(true);

Copilot uses AI. Check for mistakes.
});

it('should fall back to vision model when router returns no_vision_models error', async () => {
enableRouter();
const gpt4oEndpoint = createEndpoint('gpt-4o', 'OpenAI', { supportsVision: true });
const claudeEndpoint = createEndpoint('claude-sonnet', 'Anthropic');

(mockCAPIClientService.makeRequest as ReturnType<typeof vi.fn>).mockImplementation((_body: any, opts: any) => {
if (opts?.type === RequestType.ModelRouter) {
return Promise.resolve({
ok: false,
status: 400,
statusText: 'Bad Request',
headers: createMockHeaders(),
text: vi.fn().mockResolvedValue(JSON.stringify({ error: 'no_vision_models' }))
});
}
return Promise.resolve(
makeMockTokenResponse({
available_models: ['gpt-4o', 'claude-sonnet'],
expires_at: Math.floor(Date.now() / 1000) + 3600,
session_token: 'test-token',
})
);
});

automodeService = createService();
const chatRequest: Partial<ChatRequest> = {
location: ChatLocation.Panel,
prompt: 'describe this image',
sessionId: 'session-no-vision',
references: [{ id: 'img', value: { mimeType: 'image/png', data: new Uint8Array() } }] as any
};

const result = await automodeService.resolveAutoModeEndpoint(chatRequest as ChatRequest, [gpt4oEndpoint, claudeEndpoint]);
// Should fall back to default selection, then vision fallback picks gpt-4o
expect(result.model).toBe('gpt-4o');
// Verify the router was called and the error was classified as noVisionModels
expect(mockCAPIClientService.makeRequest).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({ type: RequestType.ModelRouter })
);
expect(mockLogService.error).toHaveBeenCalledWith(
expect.stringContaining('(noVisionModels)'),
expect.anything()
);
});

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