Send has_image to router instead of skipping it for vision requests#5040
Send has_image to router instead of skipping it for vision requests#5040abadawi591 wants to merge 4 commits into
Conversation
The server-side auto-intent-service now handles has_image via a vision-aware pre-filter instead of returning HTTP 400. Remove the client-side hasImage early return in _tryRouterSelection() and send has_image: true in the router request body when images are present. Changes: - automodeService.ts: remove hasImage bail-out, pass hasImage to router - routerDecisionFetcher.ts: accept hasImage param, include has_image in request body, parse error code from 400 responses - automodeService.ts: handle no_vision_models error as a distinct fallback reason; _applyVisionFallback remains as safety net - automodeService.spec.ts: replace skip-router test with send-has_image test, add no_vision_models error handling test, update transient fallback test
There was a problem hiding this comment.
Pull request overview
This PR updates auto mode routing for vision requests so the client no longer bypasses the router when images are present, and instead sends a has_image signal to the router API while adding a specific fallback path for no_vision_models router errors.
Changes:
- Remove the client-side “has image => skip router” early return and pass
hasImageinto router selection. - Include
has_image: truein the router request body when images are present and surface router error codes from 400 responses. - Update AutomodeService unit tests to validate routing with images and add coverage for the
no_vision_modelsfallback case.
Show a summary per file
| File | Description |
|---|---|
| src/platform/endpoint/node/automodeService.ts | Stops bypassing router for image turns, forwards hasImage to the router, and classifies no_vision_models distinctly. |
| src/platform/endpoint/node/routerDecisionFetcher.ts | Adds optional hasImage parameter, includes has_image in request body, and parses router error code from error responses. |
| src/platform/endpoint/node/test/automodeService.spec.ts | Updates/extends tests for image routing behavior and router error fallback behavior. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 4
| const errorText = await response.text().catch(() => ''); | ||
| let errorCode: string | undefined; | ||
| try { | ||
| errorCode = JSON.parse(errorText).error; | ||
| } catch { /* not JSON */ } | ||
| const err = new Error(`Router decision request failed with status ${response.status}: ${response.statusText}`); | ||
| (err as any).errorCode = errorCode; | ||
| throw err; |
There was a problem hiding this comment.
Avoid attaching errorCode via (err as any). Define a small typed error (e.g., class RouterDecisionError extends Error { errorCode?: string }) or use a type assertion to a dedicated interface so callers can safely narrow without any casts.
Replace (err as any).errorCode pattern with a typed RouterDecisionError class that carries the errorCode as a proper property.
Add emptyPrompt and emptyCandidateList to the comment so telemetry documentation matches runtime behavior.
Verify the router was actually called and the error was logged with the noVisionModels fallback reason, not just that the model is gpt-4o.
| 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'; |
There was a problem hiding this comment.
Nested ternaries are just a no-go for me. Just change this into some if-statements.
| } catch (e) { | ||
| const isTimeout = isAbortError(e); | ||
| const fallbackReason = isTimeout ? 'routerTimeout' : 'routerError'; | ||
| const errorCode = e instanceof RouterDecisionError ? e.errorCode : undefined; |
There was a problem hiding this comment.
Looking at the usage of RouterDecisionError, I'm not sure this class is required. The entire class is declared just to carry e.errorCode, a string value read once.
| prompt_char_count?: number; | ||
| } | ||
|
|
||
| export class RouterDecisionError extends Error { |
There was a problem hiding this comment.
Error classes should have a this.name declared.
| } catch { /* not JSON */ } | ||
| throw new RouterDecisionError(`Router decision request failed with status ${response.status}: ${response.statusText}`, errorCode); |
There was a problem hiding this comment.
I'm not sure why it is fine to completely ditch errorText when we couldn't parse errorText.
| } catch { /* not JSON */ } | |
| throw new RouterDecisionError(`Router decision request failed with status ${response.status}: ${response.statusText}`, errorCode); | |
| } catch { /* not JSON */ } | |
| throw new RouterDecisionError(`Router decision request failed with status ${response.status}: ${response.statusText}`, errorCode || errorText); |
Maybe?
aashna
left a comment
aashna
left a comment
There was a problem hiding this comment.
Nice work wiring the image signal through to the router. A couple things beyond jinu's comments.
| "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', { |
There was a problem hiding this comment.
Now that image requests go through the router successfully, there's no way to tell "router picked gpt-4o for a text query" apart from "router picked gpt-4o for an image query" in telemetry. Consider adding hasImage: String(hasImage(chatRequest)) to automode.routerFallback properties here (and to routerModelSelected if it still exists) so we can measure vision routing quality separately.
| 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'; |
There was a problem hiding this comment.
Separate from the nested ternary issue jinu flagged — this silently swallows the server error code for non-vision 400s. If the server returns 400 with empty_prompt or some future error code, this maps it to generic routerError and we lose the real reason. Consider passing errorCode through directly:
let fallbackReason: string;
if (isTimeout) {
fallbackReason = 'routerTimeout';
} else if (errorCode) {
fallbackReason = errorCode;
} else {
fallbackReason = 'routerError';
}New server error codes surface in telemetry automatically without needing client-side mapping.
|
Thanks for the contribution! This repository has been archived because the project has moved into the main VS Code repository. Could you please reopen/recreate this PR against: We’ll continue reviewing contributions there. Thanks! |
The server-side auto-intent-service now handles has_image via a vision-aware pre-filter instead of returning HTTP 400. Remove the client-side hasImage early return in _tryRouterSelection() and send has_image: true in the router request body when images are present.
Changes: