From c613cfa9a7c67b8caf06d6e2466d949c6ac17e75 Mon Sep 17 00:00:00 2001 From: Samuel Gudi Date: Fri, 8 May 2026 18:11:16 +0200 Subject: [PATCH] refactor(profiles): relocate _profiles_match to api/profiles.py (#1895 review) Maintainer review on PR #1895 flagged that mcp_server.py duplicated the visibility model from api/routes.py:75. Move the canonical helper into api/profiles.py (next to _is_root_profile, on which it depends) so both api/routes.py and mcp_server.py import the same function instead of carrying parallel definitions that could drift as the model evolves. - api/profiles.py: + _profiles_match (verbatim from former routes.py:75-97) - api/routes.py: replace local definition with re-export to keep all existing _profiles_match(...) call sites resolving without per-call-site refactors - mcp_server.py: drop local copy, import _profiles_match alongside the existing api.profiles imports (line 59) - tests: + test_profiles_match_single_source_of_truth asserts identity (mcp.module._profiles_match is api.profiles._profiles_match is api.routes._profiles_match) so any re-introduction of a local copy trips the test + test_profiles_match_input_matrix parametrize across the (None|''|'default'|'foo') x (None|''|'default'|'foo'|'bar') visibility matrix per maintainer suggestion Behaviour unchanged. Zero call-site changes anywhere in api/routes.py. Co-Authored-By: Claude (Opus 4.7) --- api/profiles.py | 27 ++++++++++++++++++ api/routes.py | 28 ++++--------------- mcp_server.py | 20 +------------- tests/test_mcp_server.py | 60 ++++++++++++++++++++++++++++++++++++++++ 4 files changed, 93 insertions(+), 42 deletions(-) diff --git a/api/profiles.py b/api/profiles.py index f9744c27..9af9bcba 100644 --- a/api/profiles.py +++ b/api/profiles.py @@ -170,6 +170,33 @@ def _is_root_profile(name: str) -> bool: return name in _root_profile_name_cache +def _profiles_match(row_profile, active_profile) -> bool: + """Return True if a session/project row's profile matches the active profile. + + Treats both the literal alias 'default' and any renamed-root display name + (per _is_root_profile) as equivalent, so legacy rows tagged 'default' + still surface when the user has renamed the root profile to e.g. 'kinni', + and vice versa. + + A row with no profile (`None` or empty string) is treated as belonging to + the root profile — that's the convention used by the legacy backfill at + api/models.py::all_sessions, and matches the default seen in + `static/sessions.js` (`S.activeProfile||'default'`). + + Originally lived in api/routes.py; relocated here so both routes.py and + out-of-process consumers (mcp_server.py) can import the canonical helper + instead of duplicating the body. See #1614 for the visibility model. + """ + row = row_profile or 'default' + active = active_profile or 'default' + if row == active: + return True + # Cross-alias the renamed root. + if _is_root_profile(row) and _is_root_profile(active): + return True + return False + + def get_active_profile_name() -> str: """Return the currently active profile name. diff --git a/api/routes.py b/api/routes.py index 1fc35ad0..151b03a8 100644 --- a/api/routes.py +++ b/api/routes.py @@ -72,29 +72,11 @@ _STALE_MESSAGING_END_REASONS = {"session_reset", "session_switch"} # when the active profile is `'default'`. _is_root_profile() is the # canonical check. -def _profiles_match(row_profile, active_profile) -> bool: - """Return True if a session/project row's profile matches the active profile. - - Treats both the literal alias 'default' and any renamed-root display name - (per _is_root_profile) as equivalent, so legacy rows tagged 'default' - still surface when the user has renamed the root profile to e.g. 'kinni', - and vice versa. - - A row with no profile (`None` or empty string) is treated as belonging to - the root profile — that's the convention used by the legacy backfill at - api/models.py::all_sessions, and matches the default seen in - `static/sessions.js` (`S.activeProfile||'default'`). - """ - from api.profiles import _is_root_profile - - row = row_profile or 'default' - active = active_profile or 'default' - if row == active: - return True - # Cross-alias the renamed root. - if _is_root_profile(row) and _is_root_profile(active): - return True - return False +# Canonical helper now lives in api.profiles so out-of-process consumers +# (mcp_server.py) can import it without duplicating the visibility model. +# Re-exported here so existing `_profiles_match(...)` call sites in this +# module keep resolving without per-call-site refactors. +from api.profiles import _profiles_match # noqa: F401, E402 (re-export) def _all_profiles_query_flag(parsed_url) -> bool: diff --git a/mcp_server.py b/mcp_server.py index 824c240d..53ff2ef4 100644 --- a/mcp_server.py +++ b/mcp_server.py @@ -56,7 +56,7 @@ from api.config import ( STATE_DIR, SESSION_DIR, SESSION_INDEX_FILE, PROJECTS_FILE, HOME, ) from api.models import load_projects, save_projects -from api.profiles import get_active_profile_name, _is_root_profile +from api.profiles import get_active_profile_name, _is_root_profile, _profiles_match # ── Apply --profile override before any module uses get_active_profile_name if _profile_arg is not None: @@ -92,24 +92,6 @@ def _validate_color(color: str | None) -> str | None: return None -def _profiles_match(row_profile: str | None, active: str | None) -> bool: - """Cross-profile ownership check — mirrors api/routes.py:_profiles_match (#1614). - - A row with no profile (None or empty) is treated as belonging to the root - profile ('default'), per the canonical webapp convention. This keeps the - MCP visibility model identical to the HTTP API: a non-root profile cannot - see legacy untagged projects, only the root profile (or a renamed-root - alias) can. - """ - row = row_profile or 'default' - act = active or 'default' - if row == act: - return True - if _is_root_profile(row) and _is_root_profile(act): - return True - return False - - def _load_index() -> list: """Read the session index. Falls back to empty list on failure.""" if not SESSION_INDEX_FILE.exists(): diff --git a/tests/test_mcp_server.py b/tests/test_mcp_server.py index 9eb0ddf5..8bf9d381 100644 --- a/tests/test_mcp_server.py +++ b/tests/test_mcp_server.py @@ -426,3 +426,63 @@ class TestApiPassword: assert self.mod._api_password() == "secret123" finally: os.environ.pop("HERMES_WEBUI_PASSWORD", None) + + +# ═══════════════════════════════════════════════════════════════════════════ +# _profiles_match parity (mcp_server vs api.routes vs api.profiles) +# ═══════════════════════════════════════════════════════════════════════════ +# +# Locks the canonical-helper relocation: mcp_server.py and api/routes.py both +# now import _profiles_match from api/profiles.py. If anyone re-introduces a +# local copy in either module, both the identity check and the input-matrix +# parametrize trip immediately. + +async def test_profiles_match_single_source_of_truth(): + """All three module names resolve to the same canonical object. + + This locks the relocation: mcp_server.py and api/routes.py both import + _profiles_match from api/profiles.py rather than carrying a local copy. + Re-introducing a local definition in either module trips this test + immediately. + + Imported here in a clean module-import context (not via _reimport_mcp, + which would re-execute api/profiles.py and produce a distinct function + object that's behaviorally identical but fails the `is` check). + """ + # Make sure no test fixture left a re-import side-effect on these modules. + for k in ('mcp_server', 'api.routes', 'api.profiles'): + sys.modules.pop(k, None) + import api.profiles as _profiles_mod + import api.routes as _routes_mod + import mcp_server as _mcp_mod + canonical = _profiles_mod._profiles_match + assert _routes_mod._profiles_match is canonical + assert _mcp_mod._profiles_match is canonical + + +@pytest.mark.parametrize("a, b", [ + (None, None), + (None, ''), + ('', None), + ('', ''), + (None, 'default'), + ('default', None), + ('default', 'default'), + ('foo', 'foo'), + ('foo', 'bar'), + ('foo', None), + (None, 'foo'), + ('default', 'foo'), + ('foo', 'default'), +]) +async def test_profiles_match_input_matrix(a, b): + """mcp_server._profiles_match agrees with api.routes._profiles_match + on every (row, active) pair across the visibility matrix. + + Note: function-object identity is checked separately in + test_profiles_match_single_source_of_truth — here we only assert + behavioral parity, which is robust to test-fixture re-imports that + clear and re-execute api.profiles.""" + from mcp_server import _profiles_match as mcp_match + from api.routes import _profiles_match as routes_match + assert mcp_match(a, b) == routes_match(a, b)