Apply Opus pre-release SHOULD-FIX + NITs (in-PR per release policy)

SHOULD-FIX #1 (renamed-root client cross-alias): drop strict-equality client filter at static/sessions.js:1853. Server-side _profiles_match cross-aliases 'default'-tagged rows to a renamed root 'kinni'; the strict-equality client would reject them, dropping every legacy session for renamed-root users. The server is now solely authoritative for profile scoping. SHOULD-FIX #2 (messaging-source dedupe ordering): _keep_latest_messaging_session_per_source now runs AFTER the profile filter at api/routes.py:2078. Before, it ran on the merged-cross-profile list with profile-blind keys, discarding the older profile's row across profiles before the scope filter — leaving zero rows for any messaging identity the active profile shared with another profile. NIT #3: _projects_migrated flag now set only AFTER successful save_projects. NIT #4: cleaned dead test code in test_is_root_profile_invalidation_drops_stale. NIT #5: _create_profile_fallback's clone_from=='default' literal now routes through _is_root_profile() for parity with the 5 other callsites. +2 regression tests pin the SHOULD-FIX shapes: - test_keep_latest_messaging_runs_after_profile_filter (source-string ordering) - test_static_sessions_js_trusts_server_profile_scoping (no client re-filter) 4173 -> 4175 tests pass. 0 regressions.
2026-05-04 16:17:26 +00:00
parent e8862632ed
commit 6bc0f9c4d5
7 changed files with 110 additions and 35 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -10,17 +10,25 @@

 ### Tests

-4142 → **4173 passing** (+31 regression tests across `tests/test_issue1611_session_profile_filtering.py` (9), `tests/test_issue1612_renamed_root_profile.py` (11), `tests/test_issue1614_project_profile_filtering.py` (11)). 0 regressions. Full suite in ~120s.
+4142 → **4175 passing** (+33 regression tests across `tests/test_issue1611_session_profile_filtering.py` (11), `tests/test_issue1612_renamed_root_profile.py` (11), `tests/test_issue1614_project_profile_filtering.py` (11)). 0 regressions. Full suite in ~120s.

 ### Pre-release verification

- Self-built fix (nesquena-hermes), pending independent review APPROVED by nesquena and Opus advisor pre-merge pass.
+- Self-built fix (nesquena-hermes), Opus advisor pre-merge pass with 2 SHOULD-FIX absorbed in-PR (see Opus-applied fixes below); independent review APPROVED by nesquena pending.
 - `_is_root_profile` invalidation cycle exercised via test_is_root_profile_invalidation_drops_stale (cache populated, then dropped after simulated profile rename).
 - `ensure_cron_project` per-profile isolation exercised via test_ensure_cron_project_creates_per_profile (two profiles → two distinct project_ids).
 - Legacy migration covered: untagged projects with sessions inherit session profile; orphan projects fall back to 'default'; idempotent (no-op on second call).
 - Cross-alias matching pinned: `_profiles_match('default', 'kinni')` returns True only when `kinni` is `is_default`.
 - Source-string assertions pin the active-profile guards on `/api/projects/{rename,delete}` and `/api/session/move`.

+### Opus-applied fixes (absorbed in-PR per release policy)
+
+- **SHOULD-FIX #1 (renamed-root client cross-alias)**: removed the strict-equality client filter at `static/sessions.js:1853`. Server-side `_profiles_match` cross-aliases `'default'`-tagged rows to a renamed root `'kinni'`; a strict-equality client filter would have rejected them, dropping every legacy session for renamed-root users. Server is now solely authoritative for profile scoping. Same fix applied to the `otherProfileCount` client fallback.
+- **SHOULD-FIX #2 (messaging-source dedupe ordering)**: moved `_keep_latest_messaging_session_per_source(merged)` to AFTER the profile filter at `api/routes.py:2078`. Before: the dedupe ran on the merged-cross-profile list with profile-blind keys, discarding the older profile's row across profiles, then the profile filter scoped to the active profile — leaving zero rows for any messaging identity the active profile shared with another profile. After: filter first, then dedupe within scope.
+- **NIT #3 (migration save-failure)**: `_projects_migrated = True` flag now set only AFTER successful `save_projects()`. A failed save no longer poisons the in-memory state for the rest of process lifetime.
+- **NIT #4 (dead test code)**: cleaned up the dead double-assignment in `test_is_root_profile_invalidation_drops_stale`.
+- **NIT #5 (`_create_profile_fallback` literal-default)**: routed the `clone_from == 'default'` literal in the no-hermes-cli fallback path through `_is_root_profile()` for parity with the other 5 callsites.
+

 ## [v0.50.292] — 2026-05-04

--- a/api/models.py
+++ b/api/models.py
@@ -1009,12 +1009,8 @@ def _backfill_project_profiles_if_needed(projects: list) -> bool:
    (cached via the module-level _projects_migrated flag) but the result is
    persisted so it's a one-time write.
    """
-    global _projects_migrated
-    if _projects_migrated:
-        return False
    untagged = [p for p in projects if not p.get('profile')]
    if not untagged:
-        _projects_migrated = True
        return False

    # Build session_id -> profile map for the untagged project_ids.
@@ -1036,7 +1032,6 @@ def _backfill_project_profiles_if_needed(projects: list) -> bool:
        inferred = session_profile_by_project.get(p.get('project_id'), 'default')
        p['profile'] = inferred
        mutated = True
-    _projects_migrated = True
    return mutated


@@ -1047,19 +1042,28 @@ def load_projects(*, _migrate: bool = True) -> list:
    on legacy untagged projects (#1614). Disable via `_migrate=False` for
    callsites that want the raw on-disk shape (test fixtures, e.g.).
    """
+    global _projects_migrated
    if not PROJECTS_FILE.exists():
        return []
    try:
        projects = json.loads(PROJECTS_FILE.read_text(encoding='utf-8'))
    except Exception:
        return []
-    if _migrate:
+    if _migrate and not _projects_migrated:
        with _PROJECTS_MIGRATION_LOCK:
+            # Re-check inside the lock — another thread may have raced.
+            if _projects_migrated:
+                return projects
            if _backfill_project_profiles_if_needed(projects):
                try:
                    save_projects(projects)
+                    _projects_migrated = True
                except Exception:
                    logger.debug("Failed to persist project profile backfill")
+                    # Leave _projects_migrated False so a future call retries.
+            else:
+                # Nothing to migrate — already tagged.
+                _projects_migrated = True
    return projects

 def save_projects(projects) -> None:
--- a/api/profiles.py
+++ b/api/profiles.py
@@ -727,7 +727,7 @@ def _create_profile_fallback(name: str, clone_from: str = None,

    # Clone config files from source profile if requested
    if clone_config and clone_from:
-        if clone_from == 'default':
+        if _is_root_profile(clone_from):
            source_dir = _DEFAULT_HERMES_HOME
        else:
            source_dir = _DEFAULT_HERMES_HOME / 'profiles' / clone_from
@@ -776,7 +776,7 @@ def create_profile_api(name: str, clone_from: str = None,
    _validate_profile_name(name)
    # Defense-in-depth: validate clone_from here too, even though routes.py
    # also validates it. Any caller that bypasses the HTTP layer gets protection.
-    if clone_from is not None and clone_from != 'default':
+    if clone_from is not None and not _is_root_profile(clone_from):
        _validate_profile_name(clone_from)

    try:
--- a/api/routes.py
+++ b/api/routes.py
@@ -2050,13 +2050,20 @@ def handle_get(handler, parsed) -> bool:
            key=lambda s: s.get("last_message_at") or s.get("updated_at", 0) or 0,
            reverse=True,
        )
-        merged = _keep_latest_messaging_session_per_source(merged)
        # ── Profile scoping (#1611) ────────────────────────────────────────
        # Default: filter to the active profile. ?all_profiles=1 opts into
        # the aggregate view used by the "All profiles" sidebar toggle.
        # The other_profile_count is always returned so the UI can render
        # the "Show N from other profiles" affordance without sending the
        # cross-profile rows by default.
+        #
+        # IMPORTANT: scope BEFORE _keep_latest_messaging_session_per_source.
+        # _messaging_source_key is profile-blind (#1614 follow-up): if the
+        # same Slack/Telegram identity has sessions in profiles A and B, a
+        # profile-blind dedupe would discard the older one even when scoped
+        # to its own profile, leaving that profile with zero rows for that
+        # source. Filter first so the dedupe operates only within the active
+        # profile's rows.
        from api.profiles import get_active_profile_name
        active_profile = get_active_profile_name()
        all_profiles = _all_profiles_query_flag(parsed)
@@ -2067,6 +2074,7 @@ def handle_get(handler, parsed) -> bool:
            scoped = [s for s in merged
                      if _profiles_match(s.get("profile"), active_profile)]
            other_profile_count = len(merged) - len(scoped)
+        scoped = _keep_latest_messaging_session_per_source(scoped)
        safe_merged = []
        for s in scoped:
            item = dict(s)
--- a/static/sessions.js
+++ b/static/sessions.js
@@ -1868,14 +1868,14 @@ function renderSessionListFromCache(){
    (activeSidForSidebar&&s.session_id===activeSidForSidebar) ||
    (S.session&&s.session_id===S.session.session_id&&(S.session.message_count||0)>0)
  );
-  // Filter by active profile (unless "All profiles" is toggled on).
-  // Server backfills profile='default' for legacy sessions, so every session has a profile.
-  // The server already scopes /api/sessions by the active profile by default (#1611),
-  // so this is a defense-in-depth client-side mirror — _showAllProfiles requests
-  // ?all_profiles=1 which short-circuits the server filter.
-  const profileFiltered=_showAllProfiles
-    ? withMessages
-    : withMessages.filter(s=>(s.profile||'default')===(S.activeProfile||'default'));
+  // The server is authoritative for profile scoping (#1611): it filters by
+  // active profile when no query param is set, and returns the aggregate when
+  // we send ?all_profiles=1. The renamed-root cross-alias (a row tagged
+  // 'default' matching active 'kinni' when kinni.is_default) lives server-side
+  // in _profiles_match, and a strict-equality client filter would reject those
+  // rows incorrectly. So we trust the wire data and skip the redundant client
+  // filter entirely.
+  const profileFiltered=withMessages;
  // Filter by active project. NO_PROJECT_FILTER sentinel asks for sessions
  // with no project_id; otherwise filter to the matching project_id, or
  // pass through when no filter is active.
@@ -1965,11 +1965,9 @@ function renderSessionListFromCache(){
  // Profile filter toggle (show sessions from other profiles).
  // Cross-profile rows live SERVER-SIDE behind ?all_profiles=1, so the toggle
  // must trigger a refetch — there's no client-cached aggregate to slice through.
-  // Falls back to client-side count if server didn't supply other_profile_count
-  // (e.g. older server build), to preserve UI affordance during partial rollouts.
-  const otherProfileCount = _otherProfileCount > 0
-    ? _otherProfileCount
-    : withMessages.filter(s=>(s.profile||'default')!==(S.activeProfile||'default')).length;
+  // The server is authoritative for the count (renamed-root cross-alias is
+  // server-side). A naive strict-equality client fallback would mis-count.
+  const otherProfileCount = _otherProfileCount;
  if(otherProfileCount>0&&!_showAllProfiles){
    const pfToggle=document.createElement('div');
    pfToggle.style.cssText='font-size:10px;padding:4px 10px;color:var(--muted);cursor:pointer;text-align:center;opacity:.7;';
--- a/tests/test_issue1611_session_profile_filtering.py
+++ b/tests/test_issue1611_session_profile_filtering.py
@@ -102,9 +102,10 @@ def test_static_sessions_js_no_cli_session_bypass():
    """static/sessions.js must NOT filter via `s.is_cli_session || s.profile ===`.

    The original bypass let every CLI-imported session leak into the active-profile
-    sidebar regardless of which profile owned it. After #1611, the filter is
-    solely on `(s.profile||'default') === (S.activeProfile||'default')` — server
-    already scoped the wire data, this is defense-in-depth.
+    sidebar regardless of which profile owned it. After #1611 + the Opus pre-release
+    SHOULD-FIX, the client trusts the server's scoped wire data and does not
+    re-filter by profile at all (a strict-equality client filter would reject
+    the server's renamed-root cross-aliased rows).
    """
    from pathlib import Path

@@ -117,10 +118,6 @@ def test_static_sessions_js_no_cli_session_bypass():
    assert "s.is_cli_session || s.profile === S.activeProfile" not in src, (
        "Old CLI-session bypass must be removed (#1611)"
    )
-    # And the new shape is present
-    assert "(s.profile||'default')===(S.activeProfile||'default')" in src, (
-        "Expected the new active-profile-only filter shape"
-    )


 def test_static_sessions_js_uses_all_profiles_query_when_toggle_on():
@@ -145,6 +142,69 @@ def test_static_sessions_js_uses_all_profiles_query_when_toggle_on():
    )


+# ── SHOULD-FIX #2: profile filter must run BEFORE messaging-source dedupe ──
+# Bug shape (Opus pre-release advisor): _messaging_source_key is profile-blind,
+# so if profiles A and B both have a session for the same Slack identity, a
+# profile-blind dedupe runs first and discards the older profile's row, then
+# the profile filter scopes — leaving the losing profile with zero rows for
+# that source.
+
+
+def test_keep_latest_messaging_runs_after_profile_filter():
+    """Source-string check: api/routes.py /api/sessions handler must call
+    _keep_latest_messaging_session_per_source AFTER the profile filter."""
+    from pathlib import Path
+
+    repo_root = Path(__file__).parent.parent
+    src = (repo_root / 'api' / 'routes.py').read_text(encoding='utf-8')
+
+    handler_idx = src.find('parsed.path == "/api/sessions":')
+    assert handler_idx > 0
+    next_handler = src.find('parsed.path == "/api/projects":', handler_idx)
+    block = src[handler_idx:next_handler]
+
+    filter_idx = block.find('_profiles_match(s.get("profile"), active_profile)')
+    dedupe_idx = block.find('_keep_latest_messaging_session_per_source(scoped)')
+    assert filter_idx > 0, "Profile filter not found in /api/sessions handler"
+    assert dedupe_idx > 0, "Messaging dedupe must run on the scoped list"
+    assert filter_idx < dedupe_idx, (
+        "Profile filter must run BEFORE messaging-source dedupe — running it "
+        "after lets the dedupe discard the active profile's row when both "
+        "profiles share a messaging identity (Opus pre-release SHOULD-FIX #2)"
+    )
+
+
+# ── SHOULD-FIX #1: client filter must NOT strict-equality-reject server cross-aliased rows ──
+
+
+def test_static_sessions_js_trusts_server_profile_scoping():
+    """After SHOULD-FIX #1, the client should NOT re-filter via strict equality.
+
+    Bug shape: server returns rows tagged 'default' to an active 'kinni' user
+    (when kinni is the renamed root) via _profiles_match cross-alias. A
+    naïve `(s.profile||'default')===(S.activeProfile||'default')` client filter
+    rejects them — user loses every legacy 'default'-tagged session.
+
+    Fix: drop the redundant client filter; trust the server."""
+    from pathlib import Path
+
+    repo_root = Path(__file__).parent.parent
+    src = (repo_root / 'static' / 'sessions.js').read_text(encoding='utf-8')
+
+    # The fragile client-side strict-equality filter must be gone.
+    forbidden = "withMessages.filter(s=>(s.profile||'default')===(S.activeProfile||'default'))"
+    assert forbidden not in src, (
+        "Client must not re-filter rows the server already cross-aliased "
+        "(Opus pre-release SHOULD-FIX #1)"
+    )
+
+    # And the count fallback that ran the same broken comparison must be gone too.
+    forbidden_count = "withMessages.filter(s=>(s.profile||'default')!==(S.activeProfile||'default')).length"
+    assert forbidden_count not in src, (
+        "Client otherProfileCount must come from server, not strict-equality fallback"
+    )
+
+
 # ── Cleanup ────────────────────────────────────────────────────────────────


--- a/tests/test_issue1612_renamed_root_profile.py
+++ b/tests/test_issue1612_renamed_root_profile.py
@@ -71,13 +71,10 @@ def test_is_root_profile_invalidation_drops_stale(monkeypatch):
    """Explicit invalidation forces re-query on next call."""
    import api.profiles as p

-    states = [
+    seq = [
        [{'name': 'kinni', 'is_default': True, 'path': '/tmp/.hermes'}],
        [{'name': 'noblepro', 'is_default': True, 'path': '/tmp/.hermes'}],
    ]
-    monkeypatch.setattr(p, 'list_profiles_api', lambda: states[len(states) - len([s for s in states])])
-    # Simpler: pop pattern
-    seq = list(states)
    monkeypatch.setattr(p, 'list_profiles_api', lambda: seq[0] if seq else [])

    p._invalidate_root_profile_cache()