From c363ff2a05f6322d5e3adfd2e62023508d58ded5 Mon Sep 17 00:00:00 2001 From: OliverBryant <2713999266@qq.com> Date: Tue, 18 Aug 2026 15:43:34 +0800 Subject: [PATCH 1/2] fix: skip inactive shared defaults for music and sound-effect models MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared-default branches of `get_default_sound_effect_model` and `get_default_music_model` did not filter on `is_active`, unlike the user-default branches directly above them. An inactive shared default therefore still resolved to a model instance that is absent from the audio tool's own registry, so callers fell back to a placeholder model name instead of the real one. Split out of the media-usage metering series (#997) because it is a behaviour change unrelated to metering — it changes which model gets resolved, not how usage is recorded — and reviewers asked for it to be called out on its own. Adds the test coverage that review asked for: the predicate was previously unasserted, so a revert would have gone unnoticed. --- src/xagent/web/services/model_service.py | 9 +++++++ tests/web/test_model_service.py | 33 ++++++++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/src/xagent/web/services/model_service.py b/src/xagent/web/services/model_service.py index 02f5688e93..dd4c6fbffa 100644 --- a/src/xagent/web/services/model_service.py +++ b/src/xagent/web/services/model_service.py @@ -1476,6 +1476,11 @@ def get_default_sound_effect_model( .filter( UserDefaultModel.config_type == "sound_effect", DBModel.category == "sound_effect", + # Mirrors the user-default branch above: without this an + # inactive shared default still resolves to a model + # instance that is absent from the tool's own registry, + # so usage records fall back to a phantom model name. + DBModel.is_active, sa_cast(DBModel.abilities, String).contains('"generate"'), UserModel.is_shared.is_(True), UserDefaultModel.user_id.in_( @@ -1539,6 +1544,10 @@ def get_default_music_model( .filter( UserDefaultModel.config_type == "music", DBModel.category == "music", + # Mirrors the user-default branch above; see the + # sound-effect getter for why an inactive shared + # default corrupts usage attribution. + DBModel.is_active, sa_cast(DBModel.abilities, String).contains('"generate"'), UserModel.is_shared.is_(True), UserDefaultModel.user_id.in_( diff --git a/tests/web/test_model_service.py b/tests/web/test_model_service.py index bf2c82c873..76c5a82b17 100644 --- a/tests/web/test_model_service.py +++ b/tests/web/test_model_service.py @@ -698,6 +698,39 @@ def test_audio_generation_default_closes_owned_session(self, get_default): session_factory.assert_called_once_with() mock_db.close.assert_called_once_with() + @pytest.mark.parametrize( + "get_default", + [get_default_sound_effect_model, get_default_music_model], + ) + def test_audio_generation_shared_default_requires_active_model(self, get_default): + """The shared-default branch must filter on is_active. + + Without it an inactive shared default still resolves to a model + instance that is absent from the audio tool's own registry, so its + usage records fall back to a phantom model name instead of the real + one. This mirrors the user-default branch, which already filtered. + """ + mock_db = MagicMock() + filter_query = ( + mock_db.query.return_value.join.return_value.join.return_value.filter + ) + filter_query.return_value.limit.return_value.all.return_value = [] + session_factory = MagicMock(return_value=mock_db) + + with patch( + "xagent.web.models.database.get_session_local", + return_value=session_factory, + ): + get_default(user_id=None) + + # The is_active column itself is passed as a filter condition, so look + # for a condition naming that column rather than a comparison value. + assert any( + getattr(condition, "key", None) == "is_active" + or getattr(getattr(condition, "left", None), "key", None) == "is_active" + for condition in filter_query.call_args.args + ), "shared audio-generation default must filter on DBModel.is_active" + @pytest.mark.parametrize( "get_default", [get_default_embedding_model, get_default_rerank_model], From cbba1acccd325a08008d9ec80c091d614124c7f6 Mon Sep 17 00:00:00 2001 From: OliverBryant <2713999266@qq.com> Date: Fri, 21 Aug 2026 15:36:48 +0800 Subject: [PATCH 2/2] test: assert inactive shared audio defaults are skipped, not just filtered Addresses both review findings on the shared-default active-filter test. Guard `filter_query.call_args` before dereferencing `.args`. If the query chain changes or the filter is never reached, the test now reports a clear assertion failure instead of `AttributeError: 'NoneType' object has no attribute 'args'`. Add a behavioural test that runs the real query against the in-memory SQLite fixture. The existing assertion only checks that some condition names `is_active`, so an inverted predicate such as `DBModel.is_active.is_(False)` satisfied it while still returning a deactivated model. The new test creates a visible shared default whose model is inactive, asserts the getter returns None and never calls the model factory, then flips the row to active and asserts the model is returned -- the active case is what makes an inverted predicate observable rather than vacuously passing. Verified by mutation: inverting the predicate to `is_active.is_(False)` fails the new test for both getters, and removing the predicate entirely fails all four parametrisations. --- tests/web/test_model_service.py | 78 ++++++++++++++++++++++++++++++++- 1 file changed, 77 insertions(+), 1 deletion(-) diff --git a/tests/web/test_model_service.py b/tests/web/test_model_service.py index 76c5a82b17..aa7ca18f9e 100644 --- a/tests/web/test_model_service.py +++ b/tests/web/test_model_service.py @@ -8,7 +8,7 @@ from xagent.web.models.database import Base from xagent.web.models.model import Model -from xagent.web.models.user import User +from xagent.web.models.user import User, UserDefaultModel, UserModel from xagent.web.services.model_service import ( _is_model_visible_to_user, get_asr_models, @@ -725,12 +725,88 @@ def test_audio_generation_shared_default_requires_active_model(self, get_default # The is_active column itself is passed as a filter condition, so look # for a condition naming that column rather than a comparison value. + assert filter_query.call_args is not None, ( + "the shared-default query never reached its filter() call" + ) assert any( getattr(condition, "key", None) == "is_active" or getattr(getattr(condition, "left", None), "key", None) == "is_active" for condition in filter_query.call_args.args ), "shared audio-generation default must filter on DBModel.is_active" + @pytest.mark.parametrize( + ("get_default", "category", "config_type", "factory_module"), + [ + ( + get_default_sound_effect_model, + "sound_effect", + "sound_effect", + "xagent.core.model.sound_effect.get_sound_effect_model_instance", + ), + ( + get_default_music_model, + "music", + "music", + "xagent.core.model.music.get_music_model_instance", + ), + ], + ) + def test_audio_generation_shared_default_skips_inactive_model( + self, + db_session, + admin_user, + get_default, + category, + config_type, + factory_module, + ): + """An inactive shared default must not be selected or instantiated. + + The expression-shape assertion above only proves that *some* condition + naming ``is_active`` is present; an inverted predicate such as + ``DBModel.is_active.is_(False)`` would satisfy it while still handing + back a deactivated model. This exercises the real query against SQLite + so polarity is observable: the inactive row must be skipped, and an + active one must still be returned. + """ + inactive = Model( + model_id=f"inactive-{category}", + category=category, + model_provider="test", + model_name=f"inactive-{category}", + api_key="test-api-key", + abilities=["generate"], + is_active=False, + ) + db_session.add(inactive) + db_session.commit() + db_session.refresh(inactive) + db_session.add( + UserModel(user_id=admin_user.id, model_id=inactive.id, is_shared=True) + ) + db_session.add( + UserDefaultModel( + user_id=admin_user.id, + model_id=inactive.id, + config_type=config_type, + ) + ) + db_session.commit() + + with patch(factory_module) as factory: + assert get_default(user_id=None, db=db_session) is None + factory.assert_not_called() + + # Same wiring, but active: proves the query is not simply matching + # nothing for an unrelated reason. + inactive.is_active = True + db_session.commit() + + with patch(factory_module) as factory: + result = get_default(user_id=None, db=db_session) + factory.assert_called_once() + assert result is factory.return_value + @pytest.mark.parametrize( "get_default", [get_default_embedding_model, get_default_rerank_model],