From 04090ada0f6aaaf2ee1cd0886064989ace4e1029 Mon Sep 17 00:00:00 2001 From: buua436 Date: Wed, 23 Sep 2026 10:07:55 +0800 Subject: [PATCH] refa: remove chat model target resolver (#20055) --- internal/agent/retrievalbridge/harness.go | 22 ++++++++---- internal/service/chat_pipeline.go | 43 ++++++++++++++++------- internal/service/chat_pipeline_test.go | 27 +++++--------- internal/service/model_service.go | 19 ---------- internal/service/model_solver.go | 28 +++++++-------- 5 files changed, 66 insertions(+), 73 deletions(-) diff --git a/internal/agent/retrievalbridge/harness.go b/internal/agent/retrievalbridge/harness.go index 0bf8d6f777..63dff5c7ac 100644 --- a/internal/agent/retrievalbridge/harness.go +++ b/internal/agent/retrievalbridge/harness.go @@ -57,12 +57,22 @@ func NewHarnessRetriever(modelProviderService *service.ModelProviderService, met // "gpt-4o"), i.e. Python chat_mdl.llm_name. It is the gen_json reply-cache // key's model component; empty disables that cache. resolvedModelName := "" - // The resolution goes through the same entry point the pipeline's own - // capability probe uses, so the model decided there is the model wired - // here: req.ModelID may be empty (a dialog without an llm_id), and both - // layers must then fall back to the tenant default rather than resolving - // nothing here and judging capability there. - if target, mErr := modelProviderService.ResolveChatModelTarget(ctx, req.TenantID, req.ModelID); mErr == nil { + // Resolve the same Chat model that the pipeline uses. An empty model ID + // selects the tenant default; an explicit reference must be enrolled as + // Chat, so an image2text-only model is rejected here. + var target *service.ModelTarget + var mErr error + if modelProviderService == nil { + mErr = errors.New("model provider service is not initialized") + } else { + solver := service.NewModelSolver() + if strings.TrimSpace(req.ModelID) == "" { + target, mErr = solver.ResolveDefaultModelConfig(ctx, req.TenantID, entity.ModelTypeChat) + } else { + target, mErr = solver.ResolveModelConfig(ctx, req.TenantID, entity.ModelTypeChat, req.ModelID) + } + } + if mErr == nil { if inv := component.NewResolvedInvoker(target.Driver, target.ModelName, target.APIConfig); inv != nil { // MaxLength mirrors Python LLMBundle.max_length (the model's // context window in tokens); message-fitting nodes (calculate, diff --git a/internal/service/chat_pipeline.go b/internal/service/chat_pipeline.go index bfeda53f8c..d3bb211073 100644 --- a/internal/service/chat_pipeline.go +++ b/internal/service/chat_pipeline.go @@ -1638,7 +1638,12 @@ func (s *ChatPipelineService) AsyncChatSolo( } // 4. Build the chat model wrapper. - target, err := s.ModelProviderSvc.ResolveChatModelTarget(ctx, chat.TenantID, chat.LLMID) + var target *ModelTarget + if strings.TrimSpace(chat.LLMID) == "" { + target, err = s.ModelProviderSvc.modelSolver().ResolveDefaultModelConfig(ctx, chat.TenantID, entity.ModelTypeChat) + } else { + target, err = s.ModelProviderSvc.modelSolver().ResolveModelConfig(ctx, chat.TenantID, entity.ModelTypeChat, chat.LLMID) + } if err != nil { out <- AsyncChatResult{ Answer: fmt.Sprintf("**ERROR**: %s", err.Error()), @@ -2130,12 +2135,13 @@ func tokenizeText(text string) string { } // getLLMModelConfig resolves the LLM model configuration for the chat. -// Mirrors Python's three-branch resolver at dialog_service.py:552-561, -// extended so the tenant-default branch also probes vision capability: +// Mirrors Python's three-branch resolver at dialog_service.py:552-561. Chat +// model resolution always requires the enrolled Chat type; vision capability +// is determined separately for attachment dispatch: // // if chat.llm_id: -// if "image2text" in get_model_type_by_name(...): → IMAGE2TEXT -// else: → CHAT +// if "chat" and "image2text" in get_model_type_by_name(...): → IMAGE2TEXT +// else: → CHAT // else: → tenant default // (IMAGE2TEXT when the default model is vision-capable, else CHAT) // @@ -2166,15 +2172,14 @@ func (s *ChatPipelineService) getLLMModelConfig(ctx context.Context, chat *entit return cfg, modelName, factoryName, baseURL, nil } - // Branches 1/2: explicit LLM. Resolve the enrolled type first — IMAGE2TEXT - // when the LLM is registered as vision-capable, CHAT otherwise — and let the - // same resolution report the model's tool capability. + // Branches 1/2: explicit LLM. Resolve it as a Chat model first so an + // image2text-only enrollment is rejected. The enrolled type is resolved + // separately below only to decide whether image attachments are allowed. // // This mirrors Python, which resolves chat_mdl once in get_models() and then // reads chat_mdl.is_tools off it (dialog_service.py rag_agent): one lookup, and // the model that runs is by construction the model that was judged. - modelType := s.ModelProviderSvc.modelSolver().ResolveChatModelType(ctx, chat.TenantID, chat.LLMID) - target, err := s.ModelProviderSvc.modelSolver().ResolveModelConfig(ctx, chat.TenantID, modelType, chat.LLMID) + target, err := s.ModelProviderSvc.modelSolver().ResolveModelConfig(ctx, chat.TenantID, entity.ModelTypeChat, chat.LLMID) if err != nil { return nil, "", "", "", err } @@ -2184,7 +2189,7 @@ func (s *ChatPipelineService) getLLMModelConfig(ctx context.Context, chat *entit if err != nil { return nil, "", "", "", err } - cfg["model_type"] = chatModelTypeName(modelType) + cfg["model_type"] = s.resolveChatModelType(ctx, chat.TenantID, chat.LLMID) cfg["is_tools"] = target.SupportsTools return cfg, modelName, factoryName, baseURL, nil } @@ -2331,7 +2336,13 @@ func (s *ChatPipelineService) getModels(ctx context.Context, chat *entity.Chat) } // Chat model. - target, err := s.ModelProviderSvc.ResolveChatModelTarget(ctx, chat.TenantID, chat.LLMID) + var target *ModelTarget + var err error + if strings.TrimSpace(chat.LLMID) == "" { + target, err = s.ModelProviderSvc.modelSolver().ResolveDefaultModelConfig(ctx, chat.TenantID, entity.ModelTypeChat) + } else { + target, err = s.ModelProviderSvc.modelSolver().ResolveModelConfig(ctx, chat.TenantID, entity.ModelTypeChat, chat.LLMID) + } var chatModel *modelModule.ChatModel if err == nil { chatModel = modelModule.NewChatModel(target.Driver, &target.ModelName, target.APIConfig) @@ -2905,7 +2916,13 @@ func (s *ChatPipelineService) buildChatDriver(ctx context.Context, chat *entity. if chatModel != nil { return chatModel } - target, err := s.ModelProviderSvc.ResolveChatModelTarget(ctx, chat.TenantID, chat.LLMID) + var target *ModelTarget + var err error + if strings.TrimSpace(chat.LLMID) == "" { + target, err = s.ModelProviderSvc.modelSolver().ResolveDefaultModelConfig(ctx, chat.TenantID, entity.ModelTypeChat) + } else { + target, err = s.ModelProviderSvc.modelSolver().ResolveModelConfig(ctx, chat.TenantID, entity.ModelTypeChat, chat.LLMID) + } if err != nil { return nil } diff --git a/internal/service/chat_pipeline_test.go b/internal/service/chat_pipeline_test.go index 8e44a8f9bd..338cdaac0e 100644 --- a/internal/service/chat_pipeline_test.go +++ b/internal/service/chat_pipeline_test.go @@ -2442,10 +2442,6 @@ func TestGetLLMModelConfigCarriesToolSupport(t *testing.T) { }{ {"tool-capable model", "model-tools-on", true, "chat"}, {"model without tool support", "model-tools-off", false, "chat"}, - // Enrolled only as image-to-text. The type has to be resolved before the - // row is loaded, otherwise the load is rejected as a chat model and the - // capability reads as absent while the pipeline runs on this same model. - {"image2text-only model", "model-image2text-tools", true, "image2text"}, } { t.Run(tc.name, func(t *testing.T) { cfg, _, _, _, err := svc.getLLMModelConfig(t.Context(), &entity.Chat{TenantID: "tenant-1", LLMID: tc.llm}) @@ -2462,24 +2458,17 @@ func TestGetLLMModelConfigCarriesToolSupport(t *testing.T) { } } -// TestResolveChatModelTargetAcceptsImage2TextOnlyModel pins the shared entry point -// the agentic wiring and the generation path both use: it must load a model -// enrolled only as image-to-text — and report its capability — instead of -// rejecting it as a chat model, which is what made such a request silently fall -// back to the regular RAG path. -func TestResolveChatModelTargetAcceptsImage2TextOnlyModel(t *testing.T) { +// TestResolveModelConfigRejectsImage2TextOnlyModel ensures that a model +// enrolled only as image-to-text cannot be used as a chat model. +func TestResolveModelConfigRejectsImage2TextOnlyModel(t *testing.T) { setupChatPipelineToolSupportTestDB(t) svc := NewChatPipelineService() - target, err := svc.ModelProviderSvc.ResolveChatModelTarget(t.Context(), "tenant-1", "model-image2text-tools") - if err != nil { - t.Fatalf("ResolveChatModelTarget(image2text-only model) failed: %v", err) - } - if !target.ModelType.Has(entity.ModelTypeImage2Text) { - t.Errorf("resolved type = %v, want image2text", target.ModelType) - } - if !target.SupportsTools { - t.Error("resolved target reported no tool support, want true") + _, err := svc.ModelProviderSvc.modelSolver().ResolveModelConfig( + t.Context(), "tenant-1", entity.ModelTypeChat, "model-image2text-tools", + ) + if err == nil { + t.Fatal("ResolveModelConfig accepted an image2text-only model as chat") } } diff --git a/internal/service/model_service.go b/internal/service/model_service.go index 882510a77b..dd9bee3efd 100644 --- a/internal/service/model_service.go +++ b/internal/service/model_service.go @@ -3496,25 +3496,6 @@ type AddModelRequest struct { Extra map[string]interface{} `json:"extra"` } -// ResolveChatModelTarget resolves the chat model a request will run on: the -// caller's reference when it has one, the tenant default otherwise — a dialog -// without an llm_id still runs on the default (dialog_service get_models). -// Shared by the capability probe and the agentic wiring so the two cannot resolve -// different models for one request. -func (m *ModelProviderService) ResolveChatModelTarget(ctx context.Context, tenantID, modelRef string) (*ModelTarget, error) { - if m == nil { - return nil, fmt.Errorf("%w: model provider service is not initialized", errModelConfigUnavailable) - } - solver := m.modelSolver() - if strings.TrimSpace(modelRef) == "" { - return solver.ResolveDefaultModelConfig(ctx, tenantID, entity.ModelTypeChat) - } - // Resolve the enrolled type first: a reference enrolled only as image-to-text - // is a valid chat-pipeline input (its driver answers chat requests), and - // resolving it as chat would fail the type check outright. - return solver.ResolveModelConfig(ctx, tenantID, solver.ResolveChatModelType(ctx, tenantID, modelRef), modelRef) -} - // modelTargetRef renders a resolved model as the lookups' reference: its // tenant_model id, or the composite "model@instance@provider" form. func modelTargetRef(target *ModelTarget) string { diff --git a/internal/service/model_solver.go b/internal/service/model_solver.go index 29958f2479..ad06de06f9 100644 --- a/internal/service/model_solver.go +++ b/internal/service/model_solver.go @@ -116,17 +116,11 @@ func (s *ModelSolver) ResolveModelConfig(ctx context.Context, tenantID string, m }, nil } -// ResolveChatModelType returns the type a chat reference must be resolved as: -// ModelTypeImage2Text when the reference is enrolled vision-capable, chat -// otherwise. -// -// A dialog's llm_id may name a model that is enrolled ONLY as image-to-text, and -// that model is still a valid chat-pipeline input. Resolving such a reference as -// chat fails outright ("cannot be used as chat model"), so every caller that -// needs a capability of the request's chat model must agree on which row to load -// — otherwise the model the request runs on and the model it is judged by are two -// different rows (or, for an image2text-only enrollment, the second lookup fails -// and the capability reads as absent). +// ResolveChatModelType returns the output type used by the chat pipeline for +// attachment dispatch. A model enrolled as both chat and image2text is rendered +// as image2text so image content can be passed to it; a chat-only model remains +// chat. An image2text-only enrollment remains chat here so ResolveModelConfig +// can reject it as an invalid chat model before this display type is used. // // Probe failures are conservative and yield chat: that is the type a plain chat // model is enrolled as, and it keeps image attachments out of a model whose @@ -139,12 +133,14 @@ func (s *ModelSolver) ResolveChatModelType(ctx context.Context, tenantID, modelR if err != nil { return entity.ModelTypeChat } + hasChat := false + hasImage2Text := false for _, mt := range modelTypes { - // ModelType is a bitmask: a model enrolled as chat+image2text reports a - // combined value, so test membership, not equality. - if mt.Has(entity.ModelTypeImage2Text) { - return entity.ModelTypeImage2Text - } + hasChat = hasChat || mt.Has(entity.ModelTypeChat) + hasImage2Text = hasImage2Text || mt.Has(entity.ModelTypeImage2Text) + } + if hasChat && hasImage2Text { + return entity.ModelTypeImage2Text } return entity.ModelTypeChat }