fix(core): support cross-auth fast side queries#4117
Merged
Conversation
Collaborator
Author
|
No description provided. |
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
wenshao
reviewed
May 14, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Test coverage note: The new getFastModelForSideQuery() path is well-tested in baseLlmClient.test.ts, but the side-query callers (runSideQuery, chatRecordingService, sessionTitle, toolUseSummary) don't mock this new method in their test suites — cross-auth fast model scenarios aren't covered at the integration boundary. Consider adding getFastModelForSideQuery mocks to the tests in sideQuery.test.ts, chatRecordingService.autoTitle.test.ts, sessionTitle.test.ts, and toolUseSummary.test.ts.
Documentation: The JSDoc for the model parameter in SideQueryJsonOptions and SideQueryTextOptions (sideQuery.ts:22-26) still documents the old fallback chain (config.getFastModel?.() ?? config.getModel()). Please update it to reflect the new chain: config.getFastModelForSideQuery?.() ?? config.getFastModel?.() ?? config.getModel() ?? DEFAULT_QWEN_MODEL.
Design note: The pattern config.getFastModelForSideQuery?.() ?? config.getFastModel() is duplicated across 7 call sites. Since getFastModelForSideQuery() is a strict superset of getFastModel() (it checks all auth types vs. only the current one), the ?.() and ?? getFastModel() fallback will eventually become unnecessary once the method is fully promoted. Consider consolidating callers after stabilization.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Collaborator
Author
E2E Test Report — Side-Query Fast-Model VerificationVerified that all reachable side-query call sites correctly route through the configured Cross-auth verification (PR's specific focus)Configured a deliberate cross-auth scenario:
A headless prompt that ends without tool calls triggered the next-speaker side query. The fast-model call returned Coverage of all reachable side-query sites
Tier 3 — Skipped
What each run verifiedFor every call site the OpenAI log file confirmed Plan corrections surfaced during runs
Test isolationAll runs used |
… docs Compute the model selector once at the top of `resolveForModel` and pass it through to `createContentGeneratorForModel` and `resolveModelAcrossAuthTypes`. This eliminates the redundant selector resolution that happened up to five times per cross-auth side query (once per call, plus once inside each downstream helper). Also update the JSDoc for `SideQueryJsonOptions.model` and `SideQueryTextOptions.model` to reflect the actual fallback chain (`getFastModelForSideQuery` → `getFastModel` → `getModel` → `DEFAULT_QWEN_MODEL`) introduced in this PR.
Collaborator
Author
|
Thanks for the review. Replying inline:
Those test mocks omit
Fixed in 4b198dd. JSDoc for both interfaces now reflects the actual chain (
Agreed. Leaving the |
wenshao
approved these changes
May 14, 2026
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅ — DeepSeek/deepseek-v4-pro via Qwen Code /review
This was referenced May 14, 2026
TaimoorSiddiquiOfficial
pushed a commit
to TaimoorSiddiquiOfficial/HopCode
that referenced
this pull request
May 15, 2026
* fix(core): support cross-auth fast side queries * refactor(core): hoist resolveForModel selector and refresh side-query docs Compute the model selector once at the top of `resolveForModel` and pass it through to `createContentGeneratorForModel` and `resolveModelAcrossAuthTypes`. This eliminates the redundant selector resolution that happened up to five times per cross-auth side query (once per call, plus once inside each downstream helper). Also update the JSDoc for `SideQueryJsonOptions.model` and `SideQueryTextOptions.model` to reflect the actual fallback chain (`getFastModelForSideQuery` → `getFastModel` → `getModel` → `DEFAULT_QWEN_MODEL`) introduced in this PR. (cherry picked from commit cc800d0)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
inherit,fast, bare model IDs, andauthType:modelId; keepsConfig.getFastModel()conservative for current-auth runtime callers; adds a side-query-specific fast-model resolver; and teachesBaseLlmClientto route qualified side-query requests through the target provider while sending the bare model ID to the API./model --fast, the fast model dialog,/rename --auto, auto titles, recaps, memory relevance, tool summaries, and dynamic command localization now use that centralized side-query default instead of each caller passing its own default model.fastModelconfigured under another auth type, for exampleopenai:deepseek-v4-flash, fast-model side queries were treated as unavailable or routed through the wrong provider. That made flows such as/rename --autoskip the side query instead of using the configured fast model.getFastModel()intentionally remains current-auth-only, whilegetFastModelForSideQuery()can return an authType-qualified selector because side queries go through the per-modelBaseLlmClientrouting path.Validation
/model --fast deepseek-v4-flash,/model --fast openai:deepseek-v4-flash, fast-dialog selection persistence,/rename --autofast-model gating,runSideQuery()default model selection,model: fast, and cross-providerBaseLlmClientJSON/text generation routing.getFastModel().runSideQuery()default model path and theBaseLlmClient.resolveForModel()tests foropenai:shared-modelandmodel: fast.Scope / Risk
getFastModel()path current-auth-only to avoid widening runtime provider switching beyond the side-query path.ModelConfigremains declaration metadata or should be removed from agent runtime; design a shared runtime-view resolver for forked-agent and speculation paths; support cross-auth subagent declarations andmodel: fastthere; then revisit whethergetFastModel()should ever become cross-auth globally or whether the side-query split should remain permanent.Testing Matrix
Testing matrix notes:
./locales/es.js, but the suite passes.Linked Issues / Bugs