mirror of
https://github.com/zhayujie/chatgpt-on-wechat.git
synced 2026-07-17 11:07:11 +08:00
fix: address review — no auto-hijack, sync model on activation, cleanup
1. Creating a provider no longer auto-switches bot_type. Only an explicit make_active=true changes the active model — prevents silently hijacking users on Claude/OpenAI/etc. 2. When a provider IS activated, its 'model' is now written into the global 'model' field. This ensures all three paths (regular chat, agent_bridge, vision) use the correct model without per-path patches. 3. Removed unused i18n key 'models_custom_name_exists' (no longer referenced after the id-based rework removed name-collision checks). 4. Updated tests: 39 passing (added model-sync tests, fixed tests that relied on the removed auto-activation behavior).
This commit is contained in:
@@ -46,7 +46,6 @@ const I18N = {
|
||||
models_custom_delete_confirm_msg: '确定删除该自定义厂商吗?此操作无法撤销。',
|
||||
models_custom_name_required: '请填写名称',
|
||||
models_custom_base_required: '请填写 API Base',
|
||||
models_custom_name_exists: '该名称已存在',
|
||||
models_custom_empty: '尚未配置自定义厂商,点击添加',
|
||||
models_custom_edit_title: '编辑自定义厂商',
|
||||
models_custom_add_title: '添加自定义厂商',
|
||||
@@ -268,7 +267,6 @@ const I18N = {
|
||||
models_custom_delete_confirm_msg: 'Delete this custom provider? This cannot be undone.',
|
||||
models_custom_name_required: 'Name is required',
|
||||
models_custom_base_required: 'API Base is required',
|
||||
models_custom_name_exists: 'A provider with this name already exists',
|
||||
models_custom_empty: 'No custom providers yet, click to add',
|
||||
models_custom_edit_title: 'Edit custom provider',
|
||||
models_custom_add_title: 'Add custom provider',
|
||||
|
||||
@@ -2798,7 +2798,12 @@ class ModelsHandler:
|
||||
"""Write the providers list to both in-memory conf and the on-disk
|
||||
config, then reset the bridge so bots rebuild.
|
||||
|
||||
If ``bot_type`` is given, also update ``bot_type``."""
|
||||
If ``bot_type`` is given, also update ``bot_type``. When activating a
|
||||
provider (bot_type is ``custom:<id>``), also write the provider's
|
||||
``model`` into the global ``model`` field so that all paths (chat,
|
||||
agent, vision) automatically use the correct model."""
|
||||
from models.custom_provider import parse_custom_bot_type
|
||||
|
||||
local_config = conf()
|
||||
file_cfg = self._read_file_config()
|
||||
local_config["custom_providers"] = providers
|
||||
@@ -2806,6 +2811,13 @@ class ModelsHandler:
|
||||
if bot_type is not None:
|
||||
local_config["bot_type"] = bot_type
|
||||
file_cfg["bot_type"] = bot_type
|
||||
# Sync the provider's model into the global model field.
|
||||
_, pid = parse_custom_bot_type(bot_type)
|
||||
if pid:
|
||||
provider = next((p for p in providers if p.get("id") == pid), None)
|
||||
if provider and provider.get("model"):
|
||||
local_config["model"] = provider["model"]
|
||||
file_cfg["model"] = provider["model"]
|
||||
self._write_file_config(file_cfg)
|
||||
self._reset_bridge()
|
||||
|
||||
@@ -2865,12 +2877,9 @@ class ModelsHandler:
|
||||
existing.pop("model", None)
|
||||
created = False
|
||||
|
||||
# Decide bot_type.
|
||||
_, current_active_id = parse_custom_bot_type(local_config.get("bot_type") or "")
|
||||
# Decide bot_type — only switch when explicitly requested.
|
||||
new_bot_type = None
|
||||
if make_active or (created and not current_active_id):
|
||||
# Activate on explicit request, or auto-activate the very first
|
||||
# provider so the resolver has a definite target.
|
||||
if make_active:
|
||||
new_bot_type = f"custom:{provider_id}"
|
||||
|
||||
self._persist_custom_providers(providers, new_bot_type)
|
||||
|
||||
@@ -72,22 +72,31 @@ class TestSetCustomProvider(unittest.TestCase):
|
||||
set_conf({"bot_type": "custom", "custom_providers": []})
|
||||
self.h = _HandlerHarness()
|
||||
|
||||
def test_create_first_provider_auto_activates(self):
|
||||
def test_create_provider_does_not_hijack_bot_type(self):
|
||||
"""Creating a provider without make_active must not change bot_type."""
|
||||
res = self.h.call(action="set_custom_provider", name="siliconflow",
|
||||
api_base="https://api.siliconflow.cn/v1", api_key="sf-key")
|
||||
self.assertEqual(res["status"], "success")
|
||||
self.assertTrue(res["created"])
|
||||
self.assertIn("id", res)
|
||||
# bot_type should be updated to "custom:<id>"
|
||||
# bot_type must remain unchanged — no auto-activation.
|
||||
bot_type = config_module.conf().get("bot_type")
|
||||
self.assertTrue(bot_type.startswith("custom:"))
|
||||
self.assertEqual(bot_type, f"custom:{res['id']}")
|
||||
self.assertEqual(bot_type, "custom") # unchanged from setUp
|
||||
providers = config_module.conf().get("custom_providers")
|
||||
self.assertEqual(len(providers), 1)
|
||||
self.assertEqual(providers[0]["id"], res["id"])
|
||||
self.assertEqual(providers[0]["name"], "siliconflow")
|
||||
self.assertEqual(self.h.bridge_resets, 1)
|
||||
|
||||
def test_create_with_make_active_switches_bot_type(self):
|
||||
"""Creating a provider with make_active=true must switch bot_type."""
|
||||
res = self.h.call(action="set_custom_provider", name="siliconflow",
|
||||
api_base="https://api.siliconflow.cn/v1", api_key="sf-key",
|
||||
make_active=True)
|
||||
self.assertEqual(res["status"], "success")
|
||||
bot_type = config_module.conf().get("bot_type")
|
||||
self.assertEqual(bot_type, f"custom:{res['id']}")
|
||||
|
||||
def test_create_requires_api_base(self):
|
||||
res = self.h.call(action="set_custom_provider", name="x", api_key="k")
|
||||
self.assertEqual(res["status"], "error")
|
||||
@@ -98,12 +107,13 @@ class TestSetCustomProvider(unittest.TestCase):
|
||||
self.assertEqual(res["status"], "error")
|
||||
|
||||
def test_second_provider_does_not_steal_active(self):
|
||||
# Explicitly activate the first provider.
|
||||
res1 = self.h.call(action="set_custom_provider", name="a",
|
||||
api_base="https://a/v1", api_key="ak")
|
||||
api_base="https://a/v1", api_key="ak", make_active=True)
|
||||
res2 = self.h.call(action="set_custom_provider", name="b",
|
||||
api_base="https://b/v1", api_key="bk")
|
||||
self.assertTrue(res2["created"])
|
||||
# First provider stays active unless make_active is requested.
|
||||
# First provider stays active — second creation doesn't steal it.
|
||||
bot_type = config_module.conf().get("bot_type")
|
||||
self.assertEqual(bot_type, f"custom:{res1['id']}")
|
||||
|
||||
@@ -155,7 +165,8 @@ class TestDeleteCustomProvider(unittest.TestCase):
|
||||
set_conf({"bot_type": "custom", "custom_providers": []})
|
||||
self.h = _HandlerHarness()
|
||||
self.res_a = self.h.call(action="set_custom_provider", name="a",
|
||||
api_base="https://a/v1", api_key="ak")
|
||||
api_base="https://a/v1", api_key="ak",
|
||||
make_active=True)
|
||||
self.res_b = self.h.call(action="set_custom_provider", name="b",
|
||||
api_base="https://b/v1", api_key="bk")
|
||||
|
||||
@@ -191,7 +202,8 @@ class TestSetActiveCustomProvider(unittest.TestCase):
|
||||
set_conf({"bot_type": "custom", "custom_providers": []})
|
||||
self.h = _HandlerHarness()
|
||||
self.res_a = self.h.call(action="set_custom_provider", name="a",
|
||||
api_base="https://a/v1", api_key="ak")
|
||||
api_base="https://a/v1", api_key="ak",
|
||||
make_active=True)
|
||||
self.res_b = self.h.call(action="set_custom_provider", name="b",
|
||||
api_base="https://b/v1", api_key="bk")
|
||||
|
||||
@@ -206,6 +218,27 @@ class TestSetActiveCustomProvider(unittest.TestCase):
|
||||
# bot_type unchanged
|
||||
self.assertEqual(config_module.conf().get("bot_type"), f"custom:{self.res_a['id']}")
|
||||
|
||||
def test_activation_syncs_model_to_global(self):
|
||||
"""Activating a provider must write its model into global model field."""
|
||||
set_conf({"bot_type": "custom", "custom_providers": [], "model": "gpt-4o"})
|
||||
h = _HandlerHarness()
|
||||
res = h.call(action="set_custom_provider", name="sf",
|
||||
api_base="https://sf/v1", api_key="k", model="deepseek-v3",
|
||||
make_active=True)
|
||||
# Global model field should now be the provider's model.
|
||||
self.assertEqual(config_module.conf().get("model"), "deepseek-v3")
|
||||
self.assertEqual(config_module.conf().get("bot_type"), f"custom:{res['id']}")
|
||||
|
||||
def test_activation_without_model_keeps_global_model(self):
|
||||
"""Activating a provider with no model must not overwrite global model."""
|
||||
set_conf({"bot_type": "custom", "custom_providers": [], "model": "gpt-4o"})
|
||||
h = _HandlerHarness()
|
||||
h.call(action="set_custom_provider", name="local",
|
||||
api_base="http://localhost:11434/v1", api_key="",
|
||||
make_active=True)
|
||||
# Global model field should remain unchanged.
|
||||
self.assertEqual(config_module.conf().get("model"), "gpt-4o")
|
||||
|
||||
|
||||
class TestProviderOverviewExpansion(unittest.TestCase):
|
||||
"""_provider_overview / _custom_provider_cards should expand the list."""
|
||||
|
||||
Reference in New Issue
Block a user