fix(gateway): honor group/thread session isolation precedence in is_shared_multi_user_session

[briandevans]

What does this PR do?

is_shared_multi_user_session documents that it "mirrors the isolation rules in build_session_key", but its thread branch drops a factor that build_session_key keeps:

# is_shared_multi_user_session (before)
if source.thread_id:
    return not thread_sessions_per_user   # ignores group_sessions_per_user

build_session_key decides per-user isolation as:

isolate_user = group_sessions_per_user
if source.thread_id and not thread_sessions_per_user:
    isolate_user = False
if isolate_user and participant_id:
    key_parts.append(str(participant_id))

So for a thread, the key is per-user iff group_sessions_per_user and thread_sessions_per_user. The helper's thread branch ignores group_sessions_per_user, diverging in exactly one combo: thread present + group_sessions_per_user=False + thread_sessions_per_user=True. There build_session_key emits a shared key (no participant_id), but the helper returns False (claims isolated).

Impact for admins running "groups shared, threads per-user": a co-member of an actually-shared thread session is reported isolated, so the resume IDOR gate (_origin_matches_caller) fail-closes and denies a legitimate member, and that thread's multi-user context/attribution handling is silently skipped.

Fix computes isolate_user identically to build_session_key. This preserves all currently-correct combos and flips only the divergent one. The docstring bullet is reconciled to state threads follow the same group/thread isolation as build_session_key.

Mirrors build_session_key's precedence: isolate_user = group_sessions_per_user, then forced to False when thread_id and not thread_sessions_per_user.

Related Issue

Fixes #

Type of Change

• 🐛 Bug fix (non-breaking change that fixes an issue)

Changes Made

• gateway/session.py: rewrite the thread branch of is_shared_multi_user_session to compute isolate_user the same way build_session_key does, and reconcile the docstring.
• tests/gateway/test_session.py: add test_shared_helper_matches_build_session_key_group_thread, which ties the helper's result to the actual key (shared == (participant_id not in key)) for the divergent combo.

How to Test

1. Before the fix, for a group thread source with group_sessions_per_user=False, thread_sessions_per_user=True: build_session_key(...) produces a shared key (no participant) but is_shared_multi_user_session(...) returns False.
2. After the fix, the helper returns True, matching the key.
3. uv run --with pytest --with pytest-asyncio python3 -m pytest tests/gateway/test_session.py -q → 109 passed. The new test fails-before / passes-after; no other session test regresses.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: macOS 15

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — or N/A
• I've updated cli-config.yaml.example if I added/changed config keys — or N/A
• I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
• I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
• I've updated tool descriptions/schemas if I changed tool behavior — or N/A

[Copilot]

Pull request overview

This PR fixes a behavioral mismatch in the gateway’s session-sharing helper by making is_shared_multi_user_session() follow the same group/thread isolation precedence as build_session_key(), preventing false “isolated” classifications for sessions that are actually shared (notably for threaded group chats under specific config combinations).

Changes:

• Updates gateway/session.py so is_shared_multi_user_session() computes isolation consistently with build_session_key() for threads vs. non-thread group/channel sessions.
• Adjusts the helper docstring to reflect the corrected thread isolation rules.
• Adds a regression test in tests/gateway/test_session.py covering the previously divergent config combination (group shared + threads per-user).

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
gateway/session.py	Aligns is_shared_multi_user_session() logic/docstring with build_session_key() precedence for thread/group isolation.
tests/gateway/test_session.py	Adds a regression test to ensure the helper’s shared/isolated result matches the actual key shape for the divergent thread combo.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[briandevans]

@copilot Thanks — I looked at this closely and am intentionally keeping the helper scoped to the isolation config decision rather than mirroring per-source participant presence, because all three consumers depend on that scoping and widening it would regress two of them:

1. Prompt contract (build_session_context → session.py): test_non_thread_group_shows_user deliberately pins that a default-isolation group carrying a user_name but no user_id renders **User:** "<name>" (single-user), and its sibling test_shared_non_thread_group_prompt_hides_single_user flips to the multi-user note only when group_sessions_per_user=False. Keying the helper on missing participant-id would silently flip every no-user_id group prompt to multi-user, breaking that intended display (and the sender-prefix branch in run.py that follows it).

2. Resume IDOR gate (_resume_allowed, slash_commands.py): this is the security-critical one. The gate treats config-isolated sessions as not shared, then applies an explicit fail-closed participant check ("participant id is missing on one side: cannot prove the same owner — fail closed"). If the helper returned shared=True whenever the current source lacks a participant id, a caller with a missing user_id in a config-isolated group would be classified shared and allowed to resume another identified member's per-user session, short-circuiting that fail-closed check. That widens the IDOR surface rather than closing it.

So the helper answers "is participant-sharing enabled by config?" — the question its three call sites actually consume — and the key-shape divergence you spotted for the missing-user_id case is handled downstream (the resume gate's own fail-closed pid check; the prompt's single-user display). Mirroring it into the helper would break the prompt contract and loosen the resume gate, so I'm leaving the helper as-is on this PR.

[teknium1]

Thanks for tracing this to the shared classification helper. The premise is confirmed on current main: gateway/session.py:846-848 ignores group_sessions_per_user, while build_session_key() applies it at gateway/session.py:952-957. The new logic in this PR matches that configuration precedence.

Problems

• docs/session-lifecycle.md:269-275 still documents the prior thread-only rule and should be updated with the same group/thread condition.
• tests/gateway/test_session.py:825-856 describes a full key mirror, but the helper intentionally remains configuration-scoped when a participant ID is absent; build_session_key() only appends an ID conditionally at gateway/session.py:936-957. The PR discussion correctly preserves the resume gate's fail-closed behavior for that case.

Suggested changes

• Update the lifecycle documentation and reword the test contract to cover configuration-level isolation precedence, while retaining the reported regression case.

Automated hermes-sweeper review.

[teknium1]

Please describe this as a group/thread configuration-precedence regression rather than an exact build_session_key() mirror. As discussed on this PR, the helper intentionally remains configuration-scoped when user_id_alt/user_id is absent, whereas the key conditionally appends that identifier.

[briandevans]

Addressed in 4a05a0ee (test framing) and bcb811ee (docs), and I have retitled the PR to drop the "exact mirror" framing.

The test docstring now states the narrower claim: the helper tracks build_session_key's group/thread isolation precedence, not its full key string, and deliberately reports the configuration-scoped decision when user_id_alt/user_id is absent instead of reconstructing the conditionally-appended identifier. docs/session-lifecycle.md describes it the same way.

The PR title is now "fix(gateway): honor group/thread session isolation precedence in is_shared_multi_user_session" so the most visible framing matches.

The change itself is unchanged and still needed — gateway/session.py:1004-1005 on current main is still if source.thread_id: return not thread_sessions_per_user, which drops group_sessions_per_user from the precedence entirely.

[briandevans]

Correction to my earlier reply on this thread: it said this was handled in 4a05a0ee (test framing) and bcb811ee (docs). The test half was true — the docstring on test_shared_helper_matches_build_session_key_group_thread does state the narrower precedence claim. The docs half was wrong. bcb811ee is the commit that introduced the "This tracks build_session_key() exactly" wording in docs/session-lifecycle.md; it did not remove it, and that sentence was still live on the head when I claimed otherwise.

Fixed in f2a613995 (docs only, one hunk). The paragraph after the decision-logic fence in docs/session-lifecycle.md now reads:

This tracks build_session_key()'s group/thread isolation precedence — not its full key string (gateway/session.py): the group setting is the base, and a thread only stays per-user isolated when both group_sessions_per_user and thread_sessions_per_user are enabled. The helper deliberately stays configuration-scoped: when no user_id_alt/user_id is present, build_session_key() omits the per-user identifier it would otherwise append and the key is shared by construction, so the helper reports that configuration decision rather than reconstructing the literal key.

The divergence statement is checked against the code: build_session_key() computes participant_id = source.user_id_alt or source.user_id and appends it only under if isolate_user and participant_id:, so an absent identifier means no per-user suffix and a key that is shared by construction.

The code comment inside the fence was already precedence-framed by bcb811ee and is unchanged. gateway/session.py is untouched by this commit.

[briandevans]

Both points addressed in f2a6364bf.

• Doc staleness: docs/session-lifecycle.md §5 still showed the old thread-only rule (return not thread_sessions_per_user), which contradicts the fixed helper — under it a thread with group_sessions_per_user=False, thread_sessions_per_user=True is shared, not isolated. Rewrote the decision-logic snippet and the summary table to match build_session_key()'s precedence (gateway/session.py:956-961): the group setting is the base, and a thread stays per-user isolated only when both flags are enabled.
• Test contract wording: reworded the group/thread mirror test's docstring to say the helper mirrors build_session_key's isolation decision (whether a per-user participant_id would be appended), not the literal key string — so its configuration-scoped, fail-closed behavior when no participant ID is present reads as intentional. Assertions unchanged; the reported regression case is retained.

Docs + test-comment only, no behavior change. tests/gateway/test_session.py green (109 passed).