From 8fac717cd1fb0f50a18c07dc1bd82e30fa932c99 Mon Sep 17 00:00:00 2001 From: buua436 Date: Tue, 22 Sep 2026 19:16:36 +0800 Subject: [PATCH] refa: remove standalone tool support resolver (#20047) --- internal/service/chat_pipeline.go | 5 +- internal/service/model_service.go | 160 ------------------------- internal/service/model_service_test.go | 66 +++++----- internal/service/model_solver.go | 56 +++++++-- 4 files changed, 83 insertions(+), 204 deletions(-) diff --git a/internal/service/chat_pipeline.go b/internal/service/chat_pipeline.go index 3968d4b66f..bfeda53f8c 100644 --- a/internal/service/chat_pipeline.go +++ b/internal/service/chat_pipeline.go @@ -2216,8 +2216,9 @@ func chatConfigSupportsTools(cfg map[string]interface{}) bool { if cfg == nil { return false } - // Read the value the way the persisted flag is read (extraToolSupport): it is - // written as a JSON boolean but has historically also been spelled as a string. + // Read the resolved capability as a boolean. ModelSolver accepts the + // persisted JSON boolean and the historical string representation before it + // stores the result on ModelTarget. switch v := cfg["is_tools"].(type) { case bool: return v diff --git a/internal/service/model_service.go b/internal/service/model_service.go index 003d758918..882510a77b 100644 --- a/internal/service/model_service.go +++ b/internal/service/model_service.go @@ -3496,99 +3496,6 @@ type AddModelRequest struct { Extra map[string]interface{} `json:"extra"` } -// ResolveModelToolSupport reports whether the resolved chat model supports -// function calling (tool calls). It mirrors Python dialog_service.rag_agent's -// `if not getattr(chat_mdl, "is_tools", False)` gate: a model without tool -// support must skip the outer rag_agent react loop and fall back to the direct -// graph (Python falls back to async_chat). -// -// Precedence mirrors Python's tenant_model_service -// `"is_tools": model_extra.get("is_tools", is_tool)` (:363): the flag persisted -// on the tenant model wins, and the provider catalog is only a default for -// models enrolled without one. That is why the tenant_model row is resolved -// first — by UUID, or for a composite "model@instance@provider" reference -// through the tenant's own provider/instance rows. Reading the catalog first -// instead would send a tenant-disabled model through the outer react loop — -// where a model that does not actually call tools answers from its own -// knowledge and the retrieval never runs. -// -// The reference is validated before model_extra is read, mirroring Python's -// get_model_config_by_id (:324-347): a missing or disabled model, a model not -// enrolled as this type, a missing provider, or a provider the tenant cannot -// reach returns an error rather than a definitive is_tools answer. Callers treat -// an error as "no tool support", the same outcome as an unset flag. -func (m *ModelProviderService) ResolveModelToolSupport(ctx context.Context, tenantID string, modelType entity.ModelType, modelRef string) (bool, error) { - if strings.TrimSpace(modelRef) == "" { - return false, fmt.Errorf("model ref is required") - } - - // Tenant-model UUID path. IDs are globally unique and may belong to a - // provider shared with a joined tenant, so the lookup itself is unscoped — - // tenantCanReachProviderTenant below decides access. - if modelObj, err := m.modelDAO.GetByID(ctx, dao.DB, modelRef); err == nil { - if modelObj.Status != "active" { - return false, fmt.Errorf("tenant model id=%s is disabled", modelRef) - } - if !entity.ModelType(modelObj.ModelType).Has(modelType) { - return false, fmt.Errorf("tenant model id=%s cannot be used as %s model", modelRef, modelType.String()) - } - provider, provErr := m.modelProviderDAO.GetByID(ctx, dao.DB, modelObj.ProviderID) - if provErr != nil && !errors.Is(provErr, gorm.ErrRecordNotFound) { - return false, provErr - } - if provider == nil { - return false, fmt.Errorf("provider id=%s not found for model id=%s", modelObj.ProviderID, modelRef) - } - allowed, accessErr := m.tenantCanReachProviderTenant(ctx, tenantID, provider.TenantID) - if accessErr != nil { - return false, accessErr - } - if !allowed { - return false, fmt.Errorf("tenant %s has no access to provider owned by tenant %s", tenantID, provider.TenantID) - } - return toolSupportFromEnrollment(modelObj.Extra, "", provider.ProviderName, modelObj.ModelName), nil - } else if !errors.Is(err, gorm.ErrRecordNotFound) { - return false, err - } - - // Composite "model@instance@provider" path. The tenant's own rows are - // resolved so a flag persisted on the enrolled model still beats the - // catalog, exactly as it does on the UUID path. - pureModelName, instanceName, providerName, err := parseModelName(modelRef) - if err != nil { - return false, err - } - provider, err := m.modelProviderDAO.GetByTenantIDAndProviderName(ctx, dao.DB, tenantID, providerName) - if err != nil { - return false, fmt.Errorf("provider %q lookup failed: %w", providerName, err) - } - if provider == nil { - return false, fmt.Errorf("provider %q not found for model %q", providerName, modelRef) - } - instance, err := m.modelInstanceDAO.GetByProviderIDAndInstanceName(ctx, dao.DB, provider.ID, instanceName) - if err != nil { - return false, fmt.Errorf("instance %q lookup failed: %w", instanceName, err) - } - if instance == nil { - return false, fmt.Errorf("instance %q not found for model %q", instanceName, modelRef) - } - modelObj, err := m.modelDAO.GetByProviderIDAndInstanceIDAndModelTypeAndModelName(ctx, dao.DB, provider.ID, instance.ID, int(modelType), pureModelName) - if err == nil { - if modelObj.Status != "active" { - return false, fmt.Errorf("model %q is disabled", modelRef) - } - return toolSupportFromEnrollment(modelObj.Extra, "", provider.ProviderName, modelObj.ModelName), nil - } - if !errors.Is(err, gorm.ErrRecordNotFound) { - return false, fmt.Errorf("model %q lookup failed: %w", modelRef, err) - } - - // The tenant never enrolled this model as this type: the catalog is the only - // source left (the instance api_key payload Python reads here is seeded from - // the same catalog at enrolment time). - return catalogToolSupport(providerName, pureModelName), nil -} - // 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). @@ -3620,28 +3527,6 @@ func modelTargetRef(target *ModelTarget) string { return fmt.Sprintf("%s@%s@%s", target.ModelName, target.InstanceName, target.ProviderName) } -// toolSupportFromEnrollment applies the capability precedence the rest of the -// service uses: the is_tools flag persisted on the enrolled model, then the flag -// carried by the instance credential payload, then the provider catalog's -// declaration for the model. -// -// Python's precedence has the same shape — model_extra.get("is_tools", is_tool), -// where is_tool comes out of the instance api_key payload — with the catalog as -// our last resort, because a tenant may enrol a model the catalog does not -// describe at all. -func toolSupportFromEnrollment(extra, instanceAPIKey, providerName, modelName string) bool { - if ts, ok := extraToolSupport(extra); ok { - return ts - } - // The instance credential can be a JSON object carrying the key together with - // capability flags; extraToolSupport reads is_tools out of either blob and - // reports "absent" for a plain credential string. - if ts, ok := extraToolSupport(instanceAPIKey); ok { - return ts - } - return catalogToolSupport(providerName, modelName) -} - // The tool-calling verdict is no longer memoized: it is computed while the model // is resolved (see resolvedModel.supportsTools) and travels with the resolution, // so there is no second lookup to amortize. @@ -3665,51 +3550,6 @@ func (m *ModelProviderService) tenantCanReachProviderTenant(ctx context.Context, return false, nil } -// extraToolSupport reads the is_tools flag persisted on a tenant model's extra -// JSON. The value is written as a JSON boolean (addModelToInstance stores -// llm.Tools.Support verbatim) but has historically also been spelled as a -// string, so both shapes are accepted. ok is false when the key is absent or -// the extra blob is unreadable, letting the caller fall back to the catalog. -func extraToolSupport(extra string) (bool, bool) { - if strings.TrimSpace(extra) == "" { - return false, false - } - var fields map[string]interface{} - if err := json.Unmarshal([]byte(extra), &fields); err != nil { - return false, false - } - v, ok := fields["is_tools"] - if !ok { - return false, false - } - switch t := v.(type) { - case bool: - return t, true - case string: - return strings.EqualFold(strings.TrimSpace(t), "true"), true - case float64: - return t != 0, true - } - return false, false -} - -// catalogToolSupport reports whether a provider's catalog declares the named -// model as supporting tool (function) calling. Returns false when the provider -// or model is unknown. Mirrors RAGFlow's model_meta "is_tools" feature derived -// from conf/models/*.json. -func catalogToolSupport(providerName, modelName string) bool { - pm := dao.GetModelProviderManager() - provider := pm.FindProvider(providerName) - if provider == nil { - return false - } - mi := pm.FindModel(provider, modelName) - if mi == nil || mi.Tools == nil { - return false - } - return mi.Tools.Support -} - type AddCustomModelRequest struct { ProviderName string `json:"provider_name"` InstanceName string `json:"instance_name"` diff --git a/internal/service/model_service_test.go b/internal/service/model_service_test.go index 9b5e754973..193b3c2278 100644 --- a/internal/service/model_service_test.go +++ b/internal/service/model_service_test.go @@ -936,12 +936,10 @@ func TestSplitRightAnchoredModelName(t *testing.T) { } } -// TestModelProviderServiceResolveModelToolSupportValidatesTenantModelRefs pins -// the validation Python's get_model_config_by_id performs before model_extra is -// read (tenant_model_service.py:324-347): a disabled model, a model not enrolled -// as the requested type, and a model whose provider the caller's tenant cannot -// reach must not produce a definitive tool-support answer. -func TestModelProviderServiceResolveModelToolSupportValidatesTenantModelRefs(t *testing.T) { +// TestModelSolverResolveModelConfigReportsToolSupport verifies that the +// resolved target carries tool support and invalid references fail during the +// same resolution. +func TestModelSolverResolveModelConfigReportsToolSupport(t *testing.T) { db := setupModelProviderServiceTestDB(t) useModelProviderServiceTestDB(t, db) activeStatus := "1" @@ -963,12 +961,13 @@ func TestModelProviderServiceResolveModelToolSupportValidatesTenantModelRefs(t * } svc := NewModelProviderService() + solver := svc.modelSolver() ctx := t.Context() // A reference that passes validation still reads the persisted flag. - got, err := svc.ResolveModelToolSupport(ctx, "tenant-1", entity.ModelTypeChat, "model-active") - if err != nil || !got { - t.Fatalf("active chat model = (%v, %v), want (true, nil)", got, err) + target, err := solver.ResolveModelConfig(ctx, "tenant-1", entity.ModelTypeChat, "model-active") + if err != nil || target == nil || !target.SupportsTools { + t.Fatalf("active chat model = (%v, %v), want tool support", target, err) } for _, tc := range []struct { @@ -979,23 +978,23 @@ func TestModelProviderServiceResolveModelToolSupportValidatesTenantModelRefs(t * {"not enrolled as chat", "model-embedding"}, {"provider owned by another tenant", "model-foreign"}, } { - got, err := svc.ResolveModelToolSupport(ctx, "tenant-1", entity.ModelTypeChat, tc.modelID) + target, err := solver.ResolveModelConfig(ctx, "tenant-1", entity.ModelTypeChat, tc.modelID) if err == nil { t.Errorf("%s: err = nil, want a validation error", tc.name) } - if got { + if target != nil && target.SupportsTools { t.Errorf("%s: tool support = true, want false", tc.name) } } } -// TestModelProviderServiceResolveModelToolSupportPrefersTenantFlag covers the +// TestModelSolverResolveModelConfigPrefersTenantToolFlag covers the // composite "@@" reference shape (chat.llm_id and the // harness ModelID accept both a UUID and a composite ref): the flag persisted on // the tenant_model row must beat the provider catalog in both directions, // mirroring Python's model_extra.get("is_tools", is_tool), and a disabled row // must be rejected instead of falling back to the catalog. -func TestModelProviderServiceResolveModelToolSupportPrefersTenantFlag(t *testing.T) { +func TestModelSolverResolveModelConfigPrefersTenantToolFlag(t *testing.T) { catalog := dao.GetModelProviderManager().FindProvider("OpenAI") if catalog == nil { t.Skip("OpenAI catalog is unavailable") @@ -1038,36 +1037,34 @@ func TestModelProviderServiceResolveModelToolSupportPrefersTenantFlag(t *testing } svc := NewModelProviderService() + solver := svc.modelSolver() ctx := t.Context() - got, err := svc.ResolveModelToolSupport(ctx, "tenant-1", entity.ModelTypeChat, toolsOn+"@default@OpenAI") + target, err := solver.ResolveModelConfig(ctx, "tenant-1", entity.ModelTypeChat, toolsOn+"@default@OpenAI") if err != nil { t.Fatalf("tool-capable model: err = %v", err) } - if got { + if target.SupportsTools { t.Errorf("tool support = true, want false: the tenant_model flag must win over the catalog") } - got, err = svc.ResolveModelToolSupport(ctx, "tenant-1", entity.ModelTypeChat, toolsOff+"@default@OpenAI") + target, err = solver.ResolveModelConfig(ctx, "tenant-1", entity.ModelTypeChat, toolsOff+"@default@OpenAI") if err != nil { t.Fatalf("tool-incapable model: err = %v", err) } - if !got { + if !target.SupportsTools { t.Errorf("tool support = false, want true: the tenant_model flag must win over the catalog") } - got, err = svc.ResolveModelToolSupport(ctx, "tenant-1", entity.ModelTypeChat, toolsOn+"-disabled@default@OpenAI") - if err == nil || got { - t.Errorf("disabled model = (%v, %v), want (false, error)", got, err) + target, err = solver.ResolveModelConfig(ctx, "tenant-1", entity.ModelTypeChat, toolsOn+"-disabled@default@OpenAI") + if err == nil || (target != nil && target.SupportsTools) { + t.Errorf("disabled model = (%v, %v), want (false, error)", target, err) } } -// TestModelProviderServiceResolveModelToolSupportPropagatesLookupFailure pins the -// distinction between an intentional not-found fallback and a real lookup -// failure: a database error must surface as an error instead of falling through -// to a successful (false, nil) answer, which callers read as "this model has no -// tool support". -func TestModelProviderServiceResolveModelToolSupportPropagatesLookupFailure(t *testing.T) { +// TestModelSolverResolveModelConfigPropagatesLookupFailure verifies that model +// resolution surfaces database failures instead of returning a partial target. +func TestModelSolverResolveModelConfigPropagatesLookupFailure(t *testing.T) { db := setupModelProviderServiceTestDB(t) useModelProviderServiceTestDB(t, db) activeStatus := "1" @@ -1084,9 +1081,10 @@ func TestModelProviderServiceResolveModelToolSupportPropagatesLookupFailure(t *t } svc := NewModelProviderService() + solver := svc.modelSolver() ctx := t.Context() - if got, err := svc.ResolveModelToolSupport(ctx, "tenant-1", entity.ModelTypeChat, "model-1"); err != nil || !got { - t.Fatalf("baseline = (%v, %v), want (true, nil)", got, err) + if target, err := solver.ResolveModelConfig(ctx, "tenant-1", entity.ModelTypeChat, "model-1"); err != nil || target == nil || !target.SupportsTools { + t.Fatalf("baseline = (%v, %v), want tool support", target, err) } // Drop the provider table: the lookup now fails with a database error @@ -1096,20 +1094,20 @@ func TestModelProviderServiceResolveModelToolSupportPropagatesLookupFailure(t *t t.Fatalf("failed to drop provider table: %v", err) } // Composite refs resolve the provider row directly. - if got, err := svc.ResolveModelToolSupport(ctx, "tenant-1", entity.ModelTypeChat, "gpt-test@default@OpenAI"); err == nil { - t.Errorf("composite provider lookup failure = (%v, nil), want a propagated error", got) + if target, err := solver.ResolveModelConfig(ctx, "tenant-1", entity.ModelTypeChat, "gpt-test@default@OpenAI"); err == nil { + t.Errorf("composite provider lookup failure = (%v, nil), want a propagated error", target) } // The UUID path too. - if got, err := svc.ResolveModelToolSupport(ctx, "tenant-1", entity.ModelTypeChat, "model-1"); err == nil { - t.Errorf("uuid provider lookup failure = (%v, nil), want a propagated error", got) + if target, err := solver.ResolveModelConfig(ctx, "tenant-1", entity.ModelTypeChat, "model-1"); err == nil { + t.Errorf("uuid provider lookup failure = (%v, nil), want a propagated error", target) } // A failing tenant_model lookup must propagate as well. if err := db.Migrator().DropTable(&entity.TenantModel{}); err != nil { t.Fatalf("failed to drop model table: %v", err) } - if got, err := svc.ResolveModelToolSupport(ctx, "tenant-1", entity.ModelTypeChat, "model-1"); err == nil { - t.Errorf("model lookup failure = (%v, nil), want a propagated error", got) + if target, err := solver.ResolveModelConfig(ctx, "tenant-1", entity.ModelTypeChat, "model-1"); err == nil { + t.Errorf("model lookup failure = (%v, nil), want a propagated error", target) } } diff --git a/internal/service/model_solver.go b/internal/service/model_solver.go index e95534dd80..29958f2479 100644 --- a/internal/service/model_solver.go +++ b/internal/service/model_solver.go @@ -18,6 +18,7 @@ package service import ( "context" + "encoding/json" "errors" "fmt" "strings" @@ -45,11 +46,6 @@ type ModelTarget struct { ModelInfo *modelModule.Model ContextLength int MaxTokens int - // SupportsTools is the resolved model's tool-calling capability, computed - // while the model is being resolved (see resolvedModel.supportsTools) so - // callers never have to look the model up a second time to read it. Python - // carries the same value as the `is_tools` field of the model config it - // resolved once, and every consumer reads it from there. SupportsTools bool } @@ -260,9 +256,9 @@ type resolvedModel struct { maxTokens int } -// supportsTools reports whether the resolved model can emit tool calls, using the -// same precedence the standalone probe uses: the flag persisted on the enrolled -// model, then the instance credential payload, then the provider catalog. +// supportsTools reports whether the resolved model can emit tool calls. The +// enrollment flag takes precedence over the instance credential and provider +// catalog defaults. // // It is a method on the resolution rather than a separate lookup so that the // capability is answered by the row that was just loaded, instead of by a second @@ -289,6 +285,50 @@ func (m *resolvedModel) supportsTools() bool { return toolSupportFromEnrollment(extra, instanceAPIKey, providerName, modelName) } +func toolSupportFromEnrollment(extra, instanceAPIKey, providerName, modelName string) bool { + if supported, ok := extraToolSupport(extra); ok { + return supported + } + if supported, ok := extraToolSupport(instanceAPIKey); ok { + return supported + } + return catalogToolSupport(providerName, modelName) +} + +func extraToolSupport(extra string) (bool, bool) { + if strings.TrimSpace(extra) == "" { + return false, false + } + var fields map[string]any + if err := json.Unmarshal([]byte(extra), &fields); err != nil { + return false, false + } + value, ok := fields["is_tools"] + if !ok { + return false, false + } + switch value := value.(type) { + case bool: + return value, true + case string: + return strings.EqualFold(strings.TrimSpace(value), "true"), true + case float64: + return value != 0, true + default: + return false, false + } +} + +func catalogToolSupport(providerName, modelName string) bool { + providerManager := dao.GetModelProviderManager() + provider := providerManager.FindProvider(providerName) + if provider == nil { + return false + } + model := providerManager.FindModel(provider, modelName) + return model != nil && model.Tools != nil && model.Tools.Support +} + func (s *ModelSolver) lookupTenantModel(ctx context.Context, tenantID, modelRef string) (*entity.TenantModel, error) { if s == nil || s.service == nil { return nil, fmt.Errorf("%w: model solver is not initialized", errModelConfigUnavailable)