Skip to content

fix(gateway): honor per-platform session isolation overrides - #84925

Open
Diaspar4u wants to merge 7 commits into
NousResearch:mainfrom
Diaspar4u:fix/platform-session-isolation-overrides
Open

fix(gateway): honor per-platform session isolation overrides#84925
Diaspar4u wants to merge 7 commits into
NousResearch:mainfrom
Diaspar4u:fix/platform-session-isolation-overrides

Conversation

@Diaspar4u

@Diaspar4u Diaspar4u commented Aug 13, 2026

Copy link
Copy Markdown

Summary

  • honor per-platform group_sessions_per_user and thread_sessions_per_user session-key overrides
  • preserve nested <platform>.extra values while loading config
  • keep authorization, persisted /resume scoping, Discord prospective-thread attribution, and sender attribution aligned with the same effective per-platform isolation policy

Test plan

  • scripts/run_tests.sh tests/gateway/test_session.py tests/gateway/test_resume_command.py tests/gateway/test_config.py tests/gateway/test_whatsapp_text_batching.py tests/gateway/test_whatsapp_group_gating.py -q
  • ruff check gateway/config.py gateway/run.py gateway/session.py gateway/slash_commands.py tests/gateway/test_config.py tests/gateway/test_session.py tests/gateway/test_resume_command.py
  • git diff --check

Supersedes #81207, including its reported prompt-context gap. The first two session-isolation commits were carried from the stack in #81208 with original authorship preserved; #81208 retains separate WhatsApp mention-tagging scope.

@alt-glitch alt-glitch added type/bug Something isn't working comp/gateway Gateway runner, session dispatch, delivery area/config Config system, migrations, profiles P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 13, 2026
@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from 0c4a482 to 6af89e8 Compare August 13, 2026 12:58
@Diaspar4u

Copy link
Copy Markdown
Author

Re-review requested. Closed the remaining per-platform isolation gaps in authorization, persisted /resume scoping, Discord prospective-thread attribution, and config loading while keeping the PR focused on session isolation overrides.

Validation: 152 focused gateway tests passed; focused Ruff, diff checks, and SSH signature verification passed.

dhruvkej9 and others added 4 commits August 13, 2026 13:00
…ssion keys

SessionStore._generate_session_key and the run.py fallback/write paths
read group_sessions_per_user / thread_sessions_per_user only from the
global gateway config, while the adapters' text-batching keys resolve
per-platform extra.* overrides. This divergence means a WhatsApp group
with extra.group_sessions_per_user: false still gets per-user session
keys on the main dispatch path, so each member carries a separate
context even though batching keys say they share one.

Resolve the per-platform extra overrides in the same three places the
session key is built so dispatch, fallback, and shared-session
attribution never diverge from the adapter's batching key.
The shared-key loop in load_gateway_config() only bridged known top-level
keys (dm_policy, group_policy, require_mention, ...) into the platform
extra and silently dropped the platform's own nested ``extra:`` dict. So
``whatsapp.extra.group_sessions_per_user: false`` never reached
PlatformConfig.extra, and the session-key paths (which honor per-platform
extra) fell back to the global default - every group member kept a
separate context even though the config asked for a shared group session.

Preserve the nested ``extra:`` dict before applying bridged keys so
top-level bridged keys keep precedence. Adds a regression test that fails
on main (nested extra dropped) and passes with this fix.
@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from 6af89e8 to d8b6490 Compare August 13, 2026 17:00
@Diaspar4u

Diaspar4u commented Aug 13, 2026

Copy link
Copy Markdown
Author

Rebased the completed isolation fix onto current main at head d8b6490fd6; the only overlapping upstream change in gateway/run.py was preserved. All four upstream-range commits have verified SSH signatures.

Validation: 164 focused gateway tests passed; focused Ruff, git diff --check, and upstream-range diff checks passed.

GitHub Actions for exact head d8b6490fd67e22af9265aaabad5274c24dffc388 are awaiting write-access approval; please approve the CI and Docker workflow runs.

@coderabbitai review

Maintainers: please re-review the current head.

@Diaspar4u

Copy link
Copy Markdown
Author

@teknium1 @alt-glitch please re-review the current head.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference, author can ignore or act on any point.

fix(gateway): honor per-platform session isolation overrides

Centralizing resolution in resolve_session_isolation() and threading it through the key/attribution/auth paths is the right approach, and the override tests are good. Observations:

  1. Duplicated resolution in build_session_context. gateway/session.py build_session_context re-implements the platform-extra lookup inline (platform_extra.get(...)) instead of calling resolve_session_isolation(config, source) — two implementations that can drift. Consider having it call the helper.
  2. is_shared_multi_user_session semantic change via effective_session_thread_id. Switching from source.thread_id to thread_id or prospective_thread_id changes shared/isolated classification for all platforms with prospective threads, not just the ones under test — a prospective thread now keys off thread_sessions_per_user instead of group_sessions_per_user. The tests document the intent, but confirm this flip is wanted platform-wide, since it also drives /resume scoping and the session-context prompt, not just keying.
  3. Overlap with feat(whatsapp): observe authorized group context before response #84926. The session-isolation hunks here (config extra preservation, resolve_session_isolation, effective_session_*) also appear in feat(whatsapp): observe authorized group context before response #84926 — whichever merges second will conflict/duplicate. Coordinate so only one copy lands.

@Diaspar4u

Copy link
Copy Markdown
Author

Addressed all three points on current head c141281d68da:

  1. Fixed the duplicated resolution path. build_session_context() now calls the same resolve_session_isolation() helper used by keying, authorization, and attribution.
  2. Bound the prospective-thread behavior to its actual contract. Only Discord can promote prospective_thread_id into thread routing/classification; a new regression proves stray prospective metadata on another platform preserves its existing group key and per-user isolation. The existing Discord initiate-in-channel/continue-in-thread behavior and sender attribution remain covered and passing.
  3. Confirmed the stack coordination. feat(whatsapp): observe authorized group context before response #84926 is explicitly documented as stacked on this PR and will be refreshed onto main after fix(gateway): honor per-platform session isolation overrides #84925 merges, so the shared isolation commits land only once.

Validation: 165 existing and new session/resume/config/WhatsApp isolation tests passed; focused Ruff, attribution audit, both diff checks, and all six upstream-range SSH signature checks passed.

@teknium1 @alt-glitch please re-review the materially changed current head.

@Diaspar4u

Diaspar4u commented Aug 17, 2026

Copy link
Copy Markdown
Author

Fixed profile-safe recovery when a per-user session key becomes shared: durable session ownership now wins over the legacy key namespace, while rows owned by another profile remain rejected. Added continuity and cross-profile rejection regressions.

Current head e9dde73 preserves the validated patch exactly. Validation: 165 focused tests passed; focused Ruff, diff checks, attribution audit, and all upstream-range commit signatures passed.

@teknium1 @alt-glitch please re-review the current head. Actions on this exact SHA are awaiting write-access approval; please approve the workflow runs.

@Diaspar4u
Diaspar4u force-pushed the fix/platform-session-isolation-overrides branch from 9e363b6 to e9dde73 Compare August 18, 2026 07:15
@Diaspar4u

Copy link
Copy Markdown
Author

Final disposition on #81207: #84925 is the preferred replacement. It carries #81207’s two session-isolation commits with original authorship preserved, then applies the same effective per-platform policy consistently to authorization, durable /resume scoping, Discord prospective-thread attribution, shared-session sender attribution, canonical context construction, and nested platform configuration loading. Those additional boundaries prevent session keying from disagreeing with access, replay, or attribution behavior. No unique isolation-fix scope remains in #81207. #81208 remains separate only for its WhatsApp mention-tagging behavior.

@Diaspar4u

Copy link
Copy Markdown
Author

Significant-overlap disposition: #62370 fixes one remaining global group/thread-precedence case inside is_shared_multi_user_session; #84925 instead resolves effective per-platform overrides and applies that policy consistently across keying, authorization, /resume, context construction, and Discord attribution. #62370 is a complementary narrow helper correction, not a replacement for #84925’s broader consistency contract. #47794 is also complementary: it preserves each appended author inside WhatsApp’s debounce batch, while #84925 governs isolation and attribution after the event reaches gateway session handling. Consolidation must retain those two narrow fixes, but neither makes #84925 unnecessary.

@Diaspar4u

Copy link
Copy Markdown
Author

Further relationship disposition: #13939 deliberately decouples sender attribution from session isolation by adding a default-on stable [from … uid:…] prefix in every chat context. #84925 keeps the existing attribution model but makes isolation, authorization, resume visibility, and shared-session attribution agree on the same effective per-platform policy. #13939 is a broader competing attribution contract, not a replacement for #84925’s isolation fix. #75770 is a behavior-preserving extraction of resume authorization from slash_commands.py; if it lands first, #84925’s changed authorization logic must be ported into the new mixin. That structural refactor does not remove #84925’s semantic requirement.

@Enough1122

Copy link
Copy Markdown
Contributor

AI code review — automated review for reference — not a maintainer.

Thanks for the disposition — noted that #13939 is a broader competing attribution contract rather than a replacement for #84925's isolation fix, and that #75770 only requires porting the authorization logic if it lands first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/config Config system, migrations, profiles comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants