feat(session_context): add set/reset_current_turn_session_key public wrappers - #32927
feat(session_context): add set/reset_current_turn_session_key public wrappers#32927Isla-Liu wants to merge 4 commits into
Conversation
|
Quick downstream context on why this PR exists, plus a side-effect read so the merge call is easy. Why these wrappers. hermes-webui#2968 (nesquena/hermes-webui#2968) needs to bind a per-turn What ships — additive only.
What does NOT change.
Side effects. None expected — pure indirection over an existing ContextVar, no shared state, no thread-pool implications beyond what the ContextVar already has. If Downstream follow-up. Once this merges, the WebUI side replaces the private Refs: hermes-webui#2968 — nesquena/hermes-webui#2968 |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds public, per-turn session-key ContextVar setter/resetter APIs and validates expected ContextVar behavior via unit tests.
Changes:
- Introduce
set_current_turn_session_key()/reset_current_turn_session_key()wrappers aroundgateway.session_context._SESSION_KEY. - Add tests for round-trip, nesting, and normalization behavior (including
Noneinput).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| gateway/session_context.py | Adds public per-turn session key setter/resetter APIs returning/resetting contextvars.Token. |
| tests/test_session_context_public_turn_setters.py | Adds tests exercising per-turn session key bind/reset behavior, including nesting and None handling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Adds a public wrapper pair for the per-turn session-identity ContextVar in gateway.session_context, mirroring the structural pattern that tools/approval.py uses for the approval-session ContextVar. Naming: deliberately distinct from approval.py's set_current_session_key -- the two ContextVars carry different concerns and downstream callers will eventually need to set both. 'turn_session' disambiguates from the approval-session. Motivation: hermes-webui PR NousResearch#2968 (nesquena/hermes-webui#2968) currently binds per-turn session identity by importing _SESSION_KEY and calling .set() directly, which the maintainer flagged as a private-API touch. This wrapper closes that gap; the webui-side refactor lands as a follow-up commit on that PR once this lands.
Anchors: - Copilot r3308087275/r3308087295: type signatures now accept Optional[str] and use Token[str]; import Optional. Public setter docstring also notes None is normalised to empty string. - Copilot r3308087305: rewrite turn_session vs session paragraph in set_current_turn_session_key docstring to reference the actual API surface (turn_session_key suffix, _SESSION_KEY ContextVar) so wording matches the names callers see. - Copilot r3308087325/r3308087343: harden test_set_and_reset_round_trip by capturing the surrounding ContextVar value before set() and asserting identity equality after reset() instead of relying on a != 'session-abc' check that could flake when an outer context already binds that string.
Anchors: - Copilot r3308922402: widen turn-session token typing for _UNSET sentinel - Copilot r3308922430: widen reset token typing for _UNSET sentinel - Copilot r3308922450: align _SESSION_KEY ContextVar token type with sentinel - Copilot r3308922477: assert session key behavior through public get_session_env
6e8c79a to
f8ffb2c
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying a real downstream API seam. Current WebUI master still imports gateway.session_context._SESSION_KEY directly in api/streaming.py:1825 and resets it at :1846.
Problems
- The PR is
DIRTYand its broad sentinel/ContextVar rewrite (gateway/session_context.pyPR head:43-74,:228) predates current main's session-context expansion and hardening (gateway/session_context.py:73-121,:157-301). Salvaging that rewrite would risk discarding current behavior unrelated to the requested API. turn_session_keyis a third public API at PR headgateway/session_context.py:143-150; the documented downstream contract only uses the explicit setter/resetter pair.
Suggested changes
- Salvage only the additive setter/resetter pair against current main, preserving the current
_UNSET,_VAR_MAP, and fallback behavior. - Omit the context manager unless a concrete caller needs it.
This is an automated hermes-sweeper review.
| from contextvars import ContextVar, Token | ||
| from typing import Iterator, Optional | ||
|
|
||
| class _UnsetType: |
There was a problem hiding this comment.
Please do not carry this sentinel/type migration into the salvage. The requested change is the public session-key wrapper pair, while current main has since expanded and hardened this ContextVar module; preserve its current state representation and add the wrappers only.
| _SESSION_KEY.reset(token) | ||
|
|
||
|
|
||
| @contextmanager |
There was a problem hiding this comment.
The downstream contract described by this PR uses the explicit setter/resetter pair. Please omit this additional public context-manager API unless a concrete consumer needs it.
This adds public wrappers for the per-turn session-identity ContextVar in
gateway.session_context, mirroring the structural pattern thattools/approval.pyuses for the approval-session ContextVar.Motivation
currently binds per-turn session identity by importing
_SESSION_KEYand calling
.set(...)directly insideapi/streaming.py. Thewebui maintainer flagged the private-API touch and asked for a public
setter to mirror
tools/approval.py:set_current_session_key. This PRcloses that gap.
What ships
gateway.session_context.set_current_turn_session_key(key: str) -> Tokengateway.session_context.reset_current_turn_session_key(token: Token) -> NoneNone-normalized-to-empty.
_SESSION_KEYitself or to existing callers; the wrappersare additive.
Naming
tools/approval.py'sset_current_session_key— the two ContextVars carry differentconcerns and downstream callers may need to set both in the same
scope.
turn_session(this module) vssession(approval) keepsimports unambiguous.
Follow-up
private touch in
api/streaming.py:_set_turn_session_identitywithset_current_turn_session_keyonce this lands. No webui change isneeded in this PR.
Refs