fix(gateway): restore missing get_terminal_cwd in session_context#682
fix(gateway): restore missing get_terminal_cwd in session_context#682badMade wants to merge 7 commits into
Conversation
run_agent.py imports get_terminal_cwd from gateway.session_context at module load time (lines 2339, 5892, 10645, 11105), but the symbol was absent — so test collection failed with ImportError and cascaded into ~275 test errors across the suite. Restored as a contextvar-backed compatibility shim: - New _TERMINAL_CWD ContextVar plus _VAR_MAP entry for concurrency-safe per-task working directories in the gateway. - set_session_vars/clear_session_vars now manage _TERMINAL_CWD too. - get_terminal_cwd(default) prefers the contextvar when set to a non-empty path, then falls back to the legacy TERMINAL_CWD env var (still set by cron/scheduler.py, the CLI, and gateway/run.py), then to the caller's default (defaulting to os.getcwd()). - Empty-string contextvar is treated as "unset" so the generic set_session_vars(platform="", chat_id="", ...) call in cron/scheduler.py:1253 doesn't shadow the workdir env var that the scheduler sets right after.
🔎 Lint report:
|
| Rule | Count |
|---|---|
invalid-argument-type |
3 |
First entries
run_agent.py:13574: [invalid-argument-type] invalid-argument-type: Argument to function `len` is incorrect: Expected `Sized`, found `(str & ~AlwaysFalsy) | (dict[Unknown | str, Unknown | str | dict[str, str]] & ~AlwaysFalsy) | (Any & ~AlwaysFalsy) | ... omitted 3 union elements`
run_agent.py:7317: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `str | dict[Unknown | str, Unknown | str | dict[str, str]] | Any | ... omitted 3 union elements`
run_agent.py:13571: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `str | dict[Unknown | str, Unknown | str | dict[str, str]] | Any | ... omitted 3 union elements`
✅ Fixed issues (7):
| Rule | Count |
|---|---|
unresolved-import |
3 |
invalid-argument-type |
3 |
unresolved-reference |
1 |
First entries
gateway/run.py:5544: [unresolved-reference] unresolved-reference: Name `team_id` used when not defined
gateway/platforms/api_server.py:3038: [unresolved-import] unresolved-import: Module `tools.approval` has no member `set_current_run_id`
run_agent.py:7317: [invalid-argument-type] invalid-argument-type: Argument to function `build_anthropic_client` is incorrect: Expected `str`, found `str | dict[Unknown, Unknown] | Any | ... omitted 3 union elements`
run_agent.py:13574: [invalid-argument-type] invalid-argument-type: Argument to function `len` is incorrect: Expected `Sized`, found `(str & ~AlwaysFalsy) | (dict[Unknown, Unknown] & ~AlwaysFalsy) | (Any & ~AlwaysFalsy) | ... omitted 3 union elements`
run_agent.py:11105: [unresolved-import] unresolved-import: Module `gateway.session_context` has no member `get_terminal_cwd`
run_agent.py:13571: [invalid-argument-type] invalid-argument-type: Argument to function `_is_oauth_token` is incorrect: Expected `str`, found `str | dict[Unknown, Unknown] | Any | ... omitted 3 union elements`
gateway/platforms/api_server.py:3036: [unresolved-import] unresolved-import: Module `tools.approval` has no member `reset_current_run_id`
Unchanged: 4355 pre-existing issues carried over.
Diagnostics are surfaced as warnings — this check never fails the build.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a task-local context variable _TERMINAL_CWD (mapped to HERMES_TERMINAL_CWD) to manage the terminal working directory for concurrent gateway tasks. It updates set_session_vars and clear_session_vars to support this new context variable and adds a backward-compatible accessor function get_terminal_cwd that falls back to the legacy TERMINAL_CWD environment variable or the current working directory. There are no review comments, and the implementation looks solid with no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
Restores the missing get_terminal_cwd accessor in gateway/session_context.py by introducing a ContextVar-backed terminal working directory value with legacy TERMINAL_CWD env-var fallback, aiming to fix repo-wide import failures and improve concurrency safety in the gateway.
Changes:
- Add
_TERMINAL_CWDContextVarand register it in_VAR_MAP. - Extend
set_session_vars/clear_session_varsto manage the terminal cwd context state. - Reintroduce
get_terminal_cwd(default=...)as a backward-compatible accessor with defined precedence.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review findings on PR #682: 1. set_session_vars: default terminal_cwd to None and translate to _UNSET internally. Previous default of "" marked _TERMINAL_CWD as "set" on every gateway/cron call that didn't pass it, destroying the never-set-vs-explicitly-cleared distinction. 2. get_terminal_cwd: restore the module invariant that an explicitly cleared contextvar suppresses os.environ fallback (matches the documented behavior of get_session_env). Empty-string in the contextvar now means "explicitly cleared" and returns the caller default; env-var fallback only happens when the contextvar is at the _UNSET sentinel. 3. Add regression tests in tests/gateway/test_session_env.py for: - _UNSET → env-var fallback (cron scheduler flow) - _UNSET → default when no env var - Explicit terminal_cwd via set_session_vars wins over env var - set_session_vars() without terminal_cwd leaves contextvar _UNSET (so the cron scheduler's later os.environ mutation still wins) - clear_session_vars() suppresses env-var fallback (no stale leak) - Concurrent asyncio tasks see isolated terminal_cwd values.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@jules fix: Most failures point to this directly:
The best fix is to restore backward-compatible session/context APIs in the session context module used by:
Primary fixYour tests import from
They also expect matching contextvars/env lookups for:
So update Suggested shape: import contextvars
import os
_UNSET = object()
_PLATFORM = contextvars.ContextVar("HERMES_SESSION_PLATFORM", default=_UNSET)
_CHAT_ID = contextvars.ContextVar("HERMES_SESSION_CHAT_ID", default=_UNSET)
_CHAT_NAME = contextvars.ContextVar("HERMES_SESSION_CHAT_NAME", default=_UNSET)
_USER_ID = contextvars.ContextVar("HERMES_SESSION_USER_ID", default=_UNSET)
_USER_NAME = contextvars.ContextVar("HERMES_SESSION_USER_NAME", default=_UNSET)
_THREAD_ID = contextvars.ContextVar("HERMES_SESSION_THREAD_ID", default=_UNSET)
_SESSION_KEY = contextvars.ContextVar("HERMES_SESSION_KEY", default=_UNSET)
_TERMINAL_CWD = contextvars.ContextVar("TERMINAL_CWD", default=_UNSET)
_VAR_MAP = {
"HERMES_SESSION_PLATFORM": _PLATFORM,
"HERMES_SESSION_CHAT_ID": _CHAT_ID,
"HERMES_SESSION_CHAT_NAME": _CHAT_NAME,
"HERMES_SESSION_USER_ID": _USER_ID,
"HERMES_SESSION_USER_NAME": _USER_NAME,
"HERMES_SESSION_THREAD_ID": _THREAD_ID,
"HERMES_SESSION_KEY": _SESSION_KEY,
}
def set_session_vars(
*,
platform=None,
chat_id=None,
chat_name=None,
user_id=None,
user_name=None,
thread_id=None,
session_key=None,
terminal_cwd=None,
):
tokens = {}
def _set_if_provided(var, value):
if value is not None:
tokens[var] = var.set("" if value is None else str(value))
_set_if_provided(_PLATFORM, platform)
_set_if_provided(_CHAT_ID, chat_id)
_set_if_provided(_CHAT_NAME, chat_name)
_set_if_provided(_USER_ID, user_id)
_set_if_provided(_USER_NAME, user_name)
_set_if_provided(_THREAD_ID, thread_id)
_set_if_provided(_SESSION_KEY, session_key)
if terminal_cwd is not None:
tokens[_TERMINAL_CWD] = _TERMINAL_CWD.set(str(terminal_cwd))
return tokens
def clear_session_vars(tokens):
for var in _VAR_MAP.values():
if var in tokens:
var.reset(tokens[var])
else:
var.set("")
if _TERMINAL_CWD in tokens:
_TERMINAL_CWD.reset(tokens[_TERMINAL_CWD])
else:
_TERMINAL_CWD.set("")
def get_session_env(name, default=""):
var = _VAR_MAP.get(name)
if var is None:
return os.getenv(name, default)
value = var.get()
if value is _UNSET:
return os.getenv(name, default)
return value
def get_terminal_cwd(default=None):
value = _TERMINAL_CWD.get()
if value is _UNSET:
return os.getenv("TERMINAL_CWD", default if default is not None else os.getcwd())
if value == "":
return default if default is not None else os.getcwd()
return valueWhy this is the right fix
Those expectations are visible in the test file:
This same missing API likely causes the large cluster of Secondary fix for
|
|
@copilot fix: Most failures point to this directly:
The best fix is to restore backward-compatible session/context APIs in the session context module used by:
Primary fixYour tests import from
They also expect matching contextvars/env lookups for:
So update Suggested shape: import contextvars
import os
_UNSET = object()
_PLATFORM = contextvars.ContextVar("HERMES_SESSION_PLATFORM", default=_UNSET)
_CHAT_ID = contextvars.ContextVar("HERMES_SESSION_CHAT_ID", default=_UNSET)
_CHAT_NAME = contextvars.ContextVar("HERMES_SESSION_CHAT_NAME", default=_UNSET)
_USER_ID = contextvars.ContextVar("HERMES_SESSION_USER_ID", default=_UNSET)
_USER_NAME = contextvars.ContextVar("HERMES_SESSION_USER_NAME", default=_UNSET)
_THREAD_ID = contextvars.ContextVar("HERMES_SESSION_THREAD_ID", default=_UNSET)
_SESSION_KEY = contextvars.ContextVar("HERMES_SESSION_KEY", default=_UNSET)
_TERMINAL_CWD = contextvars.ContextVar("TERMINAL_CWD", default=_UNSET)
_VAR_MAP = {
"HERMES_SESSION_PLATFORM": _PLATFORM,
"HERMES_SESSION_CHAT_ID": _CHAT_ID,
"HERMES_SESSION_CHAT_NAME": _CHAT_NAME,
"HERMES_SESSION_USER_ID": _USER_ID,
"HERMES_SESSION_USER_NAME": _USER_NAME,
"HERMES_SESSION_THREAD_ID": _THREAD_ID,
"HERMES_SESSION_KEY": _SESSION_KEY,
}
def set_session_vars(
*,
platform=None,
chat_id=None,
chat_name=None,
user_id=None,
user_name=None,
thread_id=None,
session_key=None,
terminal_cwd=None,
):
tokens = {}
def _set_if_provided(var, value):
if value is not None:
tokens[var] = var.set("" if value is None else str(value))
_set_if_provided(_PLATFORM, platform)
_set_if_provided(_CHAT_ID, chat_id)
_set_if_provided(_CHAT_NAME, chat_name)
_set_if_provided(_USER_ID, user_id)
_set_if_provided(_USER_NAME, user_name)
_set_if_provided(_THREAD_ID, thread_id)
_set_if_provided(_SESSION_KEY, session_key)
if terminal_cwd is not None:
tokens[_TERMINAL_CWD] = _TERMINAL_CWD.set(str(terminal_cwd))
return tokens
def clear_session_vars(tokens):
for var in _VAR_MAP.values():
if var in tokens:
var.reset(tokens[var])
else:
var.set("")
if _TERMINAL_CWD in tokens:
_TERMINAL_CWD.reset(tokens[_TERMINAL_CWD])
else:
_TERMINAL_CWD.set("")
def get_session_env(name, default=""):
var = _VAR_MAP.get(name)
if var is None:
return os.getenv(name, default)
value = var.get()
if value is _UNSET:
return os.getenv(name, default)
return value
def get_terminal_cwd(default=None):
value = _TERMINAL_CWD.get()
if value is _UNSET:
return os.getenv("TERMINAL_CWD", default if default is not None else os.getcwd())
if value == "":
return default if default is not None else os.getcwd()
return valueWhy this is the right fix
Those expectations are visible in the test file:
This same missing API likely causes the large cluster of Secondary fix for
|
Auto-merge: checks failingThe following checks did not pass:
Please fix the failing checks before this PR can be merged. |
Five pre-existing bugs were blocking the test job on PR #682. They predate the get_terminal_cwd restoration but the failing job mixes them with the import fix, so resolving here unblocks merge. 1. gateway/run.py: NameError in _is_user_authorized. `team_id` was referenced at lines 5467 and 5543 but never defined in scope, raising NameError on every authorization check. Define it alongside `user_id` near the top of the method as a getattr() with "" default so non-Slack platforms (which don't carry team_id on SessionSource) skip the team-scoped pairing/allowlist branches. Unblocks 22 tests in tests/gateway/test_unauthorized_dm_behavior.py. 2. gateway/platforms/webhook.py: reject unresolved ${VAR} secrets. The HMAC validator previously called hmac.new(secret.encode(), ...) directly even when `secret` was a literal "${WEBHOOK_SECRET}" left over from an unrendered config template — silently authorising any attacker who could compute the HMAC against that literal text. Add a small regex guard that rejects unresolved env-var placeholders before hashing. Fixes the test_validate_placeholder_secret_rejects _literal_hmac regression. 3. tools/web_tools.py: restore _ddgs_package_available. Tests monkeypatch tools.web_tools._ddgs_package_available, but the implementation had been renamed to _ddgs_package_importable, so the patches no-op'd. Rename the canonical helper to _available and keep _importable as a backward-compat alias so external callers don't regress. 4. tools/web_tools.py: stop auto-detecting ddgs as last-resort backend. _get_backend's fallback loop ended with the ddgs package presence check, making ddgs silently win whenever no env-driven backend was configured — surprising users and breaking a test that asserts ddgs must be opted into explicitly via web.backend. 5. tools/browser_tool.py + tests: normalise SSRF wording. The redirect branch said "private/internal address" while the pre- navigation branch said "private or internal address", and tests asserted on both substrings. Canonicalise on "private or internal address" everywhere and update the two redirect tests that explicitly looked for the slash form. Leaves gateway/platforms/* and vision_tools.py wording alone because their existing tests (test_wecom, test_slack, test_media_download_retry) lock in "private/internal" and aren't listed as failing here.
Commit a666275 ("Potential fix for pull request finding" — Copilot Autofix) removed the user_id parameter from set_session_vars, presumably acting on a spurious ty diagnostic. But user_id is still referenced in the body at line 116 (_SESSION_USER_ID.set(user_id)), so every caller of set_session_vars now raised NameError: name 'user_id' is not defined. This was the dominant cause of the test job failure on this PR — ~50 of the 87 failures in tests/gateway/test_session_env.py, tests/cron/test_scheduler.py, tests/cron/test_cron_workdir.py, tests/agent/test_skill_commands.py, tests/acp/test_approval_isolation.py, and tests/tools/test_cron_approval_mode.py all chained off this NameError. The "ty thinks user_id isn't defined" diagnostic at session_context.py:116 was a false positive (the parameter was clearly there in the signature before the autofix); restoring the parameter both removes the spurious warning and unbreaks the call sites.
gateway/platforms/api_server.py:3034 imports ``set_current_run_id`` and ``reset_current_run_id`` from ``tools.approval``, but those functions don't exist there. The ImportError fires inside every API server run, surfacing as the cluster of ``run.failed`` events in tests/gateway/test_api_server_runs.py (test_start_returns_202, test_status_completed_run_includes_output_and_usage, test_status_reflects_explicit_session_id, test_events_stream_returns_completed, test_stop_running_agent, test_stop_interrupt_exception_does_not_crash, test_stop_sends_sentinel_to_events_stream) — all gone with this fix. Mirrors the existing ``set_current_session_key`` / ``reset_current_session_key`` pair: a per-task ContextVar bound at the start of an API run and reset in the finally block so concurrent runs don't share process-global state. Also exposes ``get_current_run_id`` for symmetry.
Summary
get_terminal_cwdsymbol ingateway/session_context.pyto fix a repo-wide import failure that was cascading into ~275 test errors (e.g.tests/run_agent/test_run_agent.pyfailing collection withImportError: cannot import name 'get_terminal_cwd' from 'gateway.session_context')._TERMINAL_CWDContextVar (registered in_VAR_MAP), managed byset_session_vars/clear_session_vars, withget_terminal_cwd(default)preferring the contextvar then falling back to the legacyTERMINAL_CWDenv var (still set bycron/scheduler.py, the CLI, andgateway/run.py), then to the caller's default.set_session_vars(platform="", chat_id="", ...)call incron/scheduler.py:1253does not shadow the workdir env var that the scheduler sets right after — preserving the cron → agent workdir bridge.Why this env var name and not
HERMES_TERMINAL_CWD?The codebase consistently uses
TERMINAL_CWD(noHERMES_prefix) — set bycron/scheduler.py,cli.py,gateway/run.py,hermes_cli/main.py,rl_cli.py, and read bytools/file_tools._resolve_path,agent/prompt_builder, etc. The ContextVar's internal debug name staysHERMES_TERMINAL_CWDto match the file's naming convention.Test plan
from gateway.session_context import get_terminal_cwdsucceeds (the import that was failing).get_terminal_cwd()returnsos.getcwd()when no env var or contextvar is set.get_terminal_cwd('/foo')returns the provided default when nothing is set.get_terminal_cwd()returnsos.environ['TERMINAL_CWD']when set (cron → agent workdir bridge).get_terminal_cwd()returns the contextvar value when set viaset_session_vars(terminal_cwd=...), taking precedence over the env var (gateway concurrency story).set_session_vars(platform="", chat_id="", ...)followed byos.environ['TERMINAL_CWD'] = ...(the cron scheduler's flow) does NOT mask the env var.python -m py_compile gateway/session_context.pypasses.tests/run_agent/test_run_agent.pyand the dependent tests collect successfully.tests/gateway/test_session_env.pystill passes (no regression to existing session contextvar behavior).tests/cron/test_cron_workdir.pystill passes (cron workdir bridge intact).Generated by Claude Code