fix: skip inactive shared defaults for music and sound-effect models

[OliverBryant]

Split out of the #997 media-usage series as its own change, since it is a
behaviour change unrelated to metering — it changes which model gets
resolved, not how usage is recorded. Reviewers asked for it to be called
out separately rather than riding along.

Independent of the rest of the series — this branches straight off
main and can merge in any order relative to #1422, #1424, #1425 and
#1457.

The bug

The shared-default branches of get_default_sound_effect_model and
get_default_music_model do not filter on is_active, unlike the
user-default branches directly above them. So an inactive shared default
still resolves to a model instance — one that is absent from the audio
tool's own registry, which means callers fall back to a placeholder model
name instead of the real one.

Review feedback addressed

The comment on #997 noted this predicate was new and unasserted by any
test, so a future revert would go unnoticed, and asked for a case
covering an inactive shared default being skipped. Added:
test_audio_generation_shared_default_requires_active_model, which
asserts the is_active column is among the filter conditions for both
getters. It follows the existing
test_audio_generation_default_closes_owned_session pattern in the same
class, which already inspects filter_query.call_args.args the same way.

Testing

CI. Local runs are not usable in my environment — importing
xagent.core.model.chat hangs on this machine, on main as well as on
this branch — so I am relying on CI rather than claiming a local pass I
did not get.

[gemini-code-assist]

Code Review

This pull request ensures that inactive shared defaults are filtered out by adding a DBModel.is_active check to both get_default_sound_effect_model and get_default_music_model in model_service.py, and introduces a corresponding unit test. The review feedback suggests making the new unit test more robust by explicitly asserting that the query filter was called before checking its arguments, preventing confusing attribute errors if the query structure changes in the future.

[OliverBryant]

Rebased onto latest main (1fdc2f25). This was purely a stale-base refresh: the branch was still based on 7ce7a8ff, which made the GitHub diff show the whole intervening history rather than this PR's own change.

The rebase applied cleanly with no conflicts, and the resulting patch is byte-identical to the pre-rebase one — still the same 2 files, +42/-0 (model_service.py +9, test_model_service.py +33). Neither touched file had drifted on main since the old merge-base.

No API migration was required here. Unlike the other PRs split out of #997, I verified by grep that this branch contains zero references to MediaUnit, MediaCallType, media_usage, or add_media_usage — in the diff and in both touched files. The change is confined to adding an is_active predicate to the shared-default branches of the two audio-model getters, so it is unaffected by the metering primitive rename that landed in #1527.

Checks run locally on the rebased tree: ruff check clean, isort --check-only clean, and mypy reports exactly the same 63 pre-existing errors as main does for this file — no new ones. I also re-confirmed the added test is not vacuous: its assertion accepts the filter list built with DBModel.is_active and rejects the one without it, so reverting the fix does fail the test. Full suites are left to CI.

[rogercloud]

PR summary

This PR adds DBModel.is_active to the shared-default queries for sound-effect and music models so they follow the active-only eligibility already enforced by user-default lookups and model registries. This prevents a deactivated shared model from being instantiated outside the active registry, where audio-tool selection can lose the configured identity and produce phantom/placeholder usage attribution.
Blocking: no — recommended event: APPROVE

Update summary

Since the previous review, the branch was rebased onto latest main, and current head c363ff2 is the single fix commit. The actual semantic patch remains the two symmetric active predicates plus the new parametrized test (+42/-0); there is no API or metering migration.

Independent approach verdict: sound

The approach is sound: model eligibility is corrected in model_service before runtime tool instantiation, at the boundary where persisted defaults become provider instances. It adds no persistence or API boundary change (and no metering migration), while preserving visibility, category/ability filtering, fallback behavior, and session ownership; no design-level concern survived.

Prior findings checklist

These are separate canonical roots, not duplicates: the first is a test-diagnostic robustness issue (filter_query.call_args may be absent), while the second is a behavioral/polarity coverage issue (the test never proves that an inactive shared default is skipped). Their causes, impacts, and required remediations differ.

Canonical root and source evidence	Status	Current verification and context
3802029210 (PR inline, NOT FIXED); 4958529477 (duplicate review body, NOT FIXED); 5365058912 (OliverBryant author context)	NOT FIXED — confirmed minor; follow-up required	tests/web/test_model_service.py:731 still dereferences filter_query.call_args.args without first asserting that call_args is non-None. The body-only occurrence is the same root. The author's claim in 5365058912 that removing the predicate makes the test fail addresses predicate-presence coverage only; it does not make this dereference safe.
3683463297 (linked #997 inline, NOT FIXED); 4819750857 (linked #997 review/R13, NOT FIXED); 5101719615 (linked author context); 5365058912 (PR author context)	NOT FIXED — confirmed minor; follow-up required	Linked #997 explicitly requested a behavior case showing an inactive shared default being skipped; review 4819750857 records that same R13 root. The current test at tests/web/test_model_service.py:728-732 only inspects is_active expression metadata on a MagicMock whose .all() is forced to []; it never creates an inactive row or observes the getter/factory result. The linked follow-up says filters and a test were added, but the test still permits an inverted predicate such as DBModel.is_active.is_(False). OliverBryant's 5365058912 claim that removing the predicate fails the test therefore does not answer the polarity or observable-behavior gap.

No confirmed critical or major issues survived verification; both canonical roots above are confirmed minor follow-ups.

Line-level findings (ordered by severity)

Minor

1. [minor] tests/web/test_model_service.py:731 — Guard the mocked call before iterating its arguments.

   Impact: filter_query.call_args is None if the query chain changes, the filter is bypassed, or the getter returns before this mock is called. The test then raises AttributeError: 'NoneType' object has no attribute 'args' instead of reporting a clear assertion failure, making a future regression harder to diagnose.

   I saw OliverBryant's 5365058912 reply that removing the is_active predicate makes this test fail. I re-checked the current test: that claim covers only the predicate-presence assertion; it does not establish that filter_query.call_args exists, so it does not resolve this separate diagnostic root.

   Suggested fix: assert the call before reading its arguments:

   assert filter_query.call_args is not None, "The query filter was not called."
   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
   )

2. [minor] tests/web/test_model_service.py:728 — Assert inactive-default selection behavior, not only expression shape.

   Impact: this MagicMock test hard-codes .all() to return [] regardless of the filter conditions and never creates an inactive shared default, observes the getter result, or checks the model factory. A future polarity regression such as DBModel.is_active.is_(False) would still expose an is_active key and pass, allowing an inactive model to be instantiated outside the active registry and causing the audio path to lose its configured identity/usage attribution.

   OliverBryant's 5365058912 reply correctly shows that removing the predicate is caught, but that is weaker than the required contract and does not test polarity or observable selection. This is the same underlying behavior gap raised in linked #997 discussion 3683463297, not a duplicate of the call_args diagnostic finding.

   Suggested fix: use the existing db_session fixture to create a visible shared UserDefaultModel whose related Model.is_active=False, parametrize the corresponding factory with the getter, and assert the inactive row is neither instantiated nor returned. For example:

   result = get_default(user_id=None, db=db_session)
   assert result is None
   model_factory.assert_not_called()

   Add an eligible active shared/default fallback case where ordering is deterministic and assert that the active model is returned. These checks fail for a removed, inverted, or otherwise ineffective active filter.

Criteria coverage

• Production correctness, security, resource lifecycle, API/compatibility, persistence/migration, error paths, and documentation: no issue found. The predicates align shared-default eligibility with the active registries while visibility, session ownership/closure, provider teardown, and public interfaces remain unchanged.
• Test coverage: the two confirmed minor gaps above are the unchecked call_args dereference and the absence of an inactive-row behavior assertion.
• Testing: no local tests were run per review policy; CI is green.

Blocking status & recommended decision

Blocking: no. No blocking issues were confirmed. Both remaining findings are minor test-quality follow-ups and do not block merge. Recommended event: APPROVE.