* rebase #7351 onto master (bind turn workspace as agent session cwd) Co-authored-by: samfoy <samfoy@users.noreply.github.com> * fix test: use deployed agent.runtime_cwd API (_SESSION_CWD.get) not nonexistent _session_cwd_override The two failing assertions referenced agent.runtime_cwd._session_cwd_override(), a helper not present in the installed Agent (which exposes _SESSION_CWD, resolve_agent_cwd, set/clear_session_cwd). The production binder is correct and verified: binding workspace=/tmp sets _SESSION_CWD.get()=='/tmp' and resolve_agent_cwd()=='/tmp', reset restores _UNSET. Line 343 was redundant with the surrounding assertions; line 386 now reads _SESSION_CWD.get(). Co-authored-by: samfoy <samfoy@users.noreply.github.com> * docs(changelog): stamp #7351 turn workspace cwd binding --------- Co-authored-by: n <a@n> Co-authored-by: samfoy <samfoy@users.noreply.github.com>
This commit is contained in:
3 files changed
+135
-3
No files matched your search
@@ -5,6 +5,8 @@
|
||||
|
||||
### Fixed
|
||||
|
||||
- **Agent turns now run in the workspace you selected instead of the server's install directory.** Because the WebUI runs the agent in-process, anything that resolved a default working directory from the process (`os.getcwd()`) landed in the Hermes install tree rather than the chosen workspace — so conductor children could record the wrong working directory and even pick up the install tree's own `AGENTS.md` as workspace doctrine. Each turn now binds its workspace to a task-local runtime cwd, so concurrent turns keep their own workspace and the binding is restored cleanly when the turn ends (a malformed workspace path degrades gracefully instead of failing the turn). Thanks @samfoy. (#7351)
|
||||
|
||||
- **A stale WebUI runtime after an Agent update is now reported and rejected without an unsafe automatic restart, and its update-marker read can no longer hang or exhaust memory.** When the running Agent revision no longer matches the on-disk checkout, WebUI keeps returning the manual-restart `409` and now surfaces richer update diagnostics while leaving the actual restart to the operator (it never restarts itself into a possibly half-applied checkout). The shared update marker — attacker-adjacent state any process with write access to the Agent home can create — is read defensively: the read never follows a symlink, never blocks on a FIFO/device, and never reads an unbounded file, classifying anything that is not a small regular file as unknown (fail-closed), with a portable fallback on platforms lacking the atomic open flags. Thanks @hrdwdmrbl. (#7160)
|
||||
|
||||
- **Sidebar session polling no longer retains full transcripts and run-journal detail in the response cache.** Cached rows are now projected to the canonical sidebar field allowlist before storage, heavy metadata is stripped across index and full-scan paths, nested request values are isolated, and invalidated rebuilds are rejected atomically instead of being reinserted after a concurrent clear. Thanks @martindell. (#7489)
|
||||
|
||||
+42
-3
@@ -2772,11 +2772,11 @@ def _reset_streaming_hermes_home_override(override_mod, override_token, override
|
||||
# the _run_agent_streaming thread (concurrent tool batches use
|
||||
# contextvars.copy_context() so children inherit this binding); binding the
|
||||
# context-local here makes the capture task/thread-local and race-immune.
|
||||
def _set_turn_session_identity(session_id: str):
|
||||
def _set_turn_session_identity(session_id: str, workspace: str = ""):
|
||||
"""Bind THIS turn's session identity to the current (task/thread-local)
|
||||
context and return an opaque token for _reset_turn_session_identity.
|
||||
|
||||
Binds three context-locals so every session-key / UI-owner consumer is
|
||||
Binds four context-locals so every session-key / UI-owner consumer is
|
||||
covered without a race:
|
||||
* ``tools.approval._approval_session_key`` — checked FIRST by
|
||||
``get_current_session_key`` (the exact call terminal_tool.py makes for
|
||||
@@ -2787,6 +2787,18 @@ def _set_turn_session_identity(session_id: str):
|
||||
return address stamped onto ProcessSession.origin_ui_session_id and
|
||||
completion events by modern hermes-agent builds. Authoritative for
|
||||
wakeup routing when present (see ``_resolve_completion_target``).
|
||||
* ``agent.runtime_cwd._SESSION_CWD`` — this turn's workspace, when
|
||||
*workspace* is given. The WebUI runs the agent IN-PROCESS, so
|
||||
``os.getcwd()`` is the server's launch directory, not the workspace the
|
||||
user selected. Anything resolving a default working directory from the
|
||||
process therefore lands in the Hermes install tree: measured, every
|
||||
conductor child spawned from a WebUI session recorded
|
||||
``workdir=~/.hermes/hermes-agent`` while the selected workspace was
|
||||
``~/workspace``, which also fed those children the install tree's own
|
||||
contributor ``AGENTS.md`` as workspace doctrine. ``TERMINAL_CWD`` is
|
||||
already set per turn for the same purpose but is a process-global that
|
||||
concurrent turns overwrite; this contextvar is task-local, so it is
|
||||
race-immune for exactly the reason the three bindings above are.
|
||||
|
||||
It deliberately does NOT call ``gateway.session_context.set_session_vars``:
|
||||
that blanket setter also zeroes the platform/chat_id/user contextvars,
|
||||
@@ -2811,6 +2823,16 @@ def _set_turn_session_identity(session_id: str):
|
||||
tokens["ui_session_id"] = _UI_SID.set(sid)
|
||||
except Exception:
|
||||
logger.debug("per-turn _SESSION_UI_SESSION_ID bind failed", exc_info=True)
|
||||
if workspace:
|
||||
# Bound via the ContextVar directly, not ``set_session_cwd``, so the
|
||||
# reset below can use reset-token semantics like every sibling above.
|
||||
# ``clear_session_cwd()`` would instead pin "" and mask the CLI/cron
|
||||
# env fallback for a reused thread-pool worker.
|
||||
try:
|
||||
from agent.runtime_cwd import _SESSION_CWD as _SCWD
|
||||
tokens["session_cwd"] = _SCWD.set(str(workspace))
|
||||
except Exception:
|
||||
logger.debug("per-turn _SESSION_CWD bind failed", exc_info=True)
|
||||
return tokens
|
||||
|
||||
|
||||
@@ -2825,6 +2847,13 @@ def _reset_turn_session_identity(tokens) -> None:
|
||||
"""
|
||||
if not tokens:
|
||||
return
|
||||
tok = tokens.get("session_cwd")
|
||||
if tok is not None:
|
||||
try:
|
||||
from agent.runtime_cwd import _SESSION_CWD as _SCWD
|
||||
_SCWD.reset(tok)
|
||||
except Exception:
|
||||
logger.debug("per-turn _SESSION_CWD reset failed", exc_info=True)
|
||||
tok = tokens.get("ui_session_id")
|
||||
if tok is not None:
|
||||
try:
|
||||
@@ -9300,7 +9329,17 @@ def _run_agent_streaming(
|
||||
# captures THIS session, not a concurrent turn's process-global env).
|
||||
# Co-located with the existing env-restore lifecycle: set here, reset
|
||||
# in the outer finally next to _clear_thread_env().
|
||||
_turn_session_identity_tokens = _set_turn_session_identity(session_id)
|
||||
# Resolved the same way as `s.workspace` below so the bound cwd and the
|
||||
# session's own record cannot disagree. Guarded: a turn must not die
|
||||
# here because a workspace path is malformed.
|
||||
try:
|
||||
_turn_workspace_cwd = str(Path(workspace).expanduser().resolve())
|
||||
except Exception:
|
||||
_turn_workspace_cwd = ""
|
||||
logger.debug("per-turn workspace cwd resolve failed", exc_info=True)
|
||||
_turn_session_identity_tokens = _set_turn_session_identity(
|
||||
session_id, workspace=_turn_workspace_cwd
|
||||
)
|
||||
s = get_session(session_id)
|
||||
_turn_pending_source = getattr(s, 'pending_user_source', None) or 'webui'
|
||||
_active_turn_identity = _active_turn_authority(s, stream_id, msg_text)
|
||||
|
||||
@@ -307,3 +307,94 @@ def test_turn_identity_binder_sets_ui_session_id():
|
||||
assert sc._SESSION_UI_SESSION_ID.get() == "webui-sid-42"
|
||||
assert sc.get_session_env("HERMES_UI_SESSION_ID", "") == "webui-sid-42"
|
||||
assert sc._SESSION_UI_SESSION_ID.get() is sc._UNSET
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Per-turn workspace cwd — the agent runs IN-PROCESS, so os.getcwd() is the
|
||||
# server's launch dir, not the user's selected workspace
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
def test_turn_identity_binder_sets_session_cwd():
|
||||
"""The binder must pin ``agent.runtime_cwd._SESSION_CWD`` to the turn's
|
||||
workspace, and restore ``_UNSET`` on exit.
|
||||
|
||||
The WebUI runs the agent in-process, so any consumer resolving a default
|
||||
working directory from the process sees the server's launch directory. That
|
||||
is the Hermes install tree in a normal deployment, which is never a
|
||||
workspace. ``TERMINAL_CWD`` is already stamped per turn for this purpose but
|
||||
is a process-global that concurrent turns overwrite, so routing must use the
|
||||
task-local contextvar for the same reason session-key routing does.
|
||||
|
||||
Pre-fix: the binder ignored the workspace entirely and ``_SESSION_CWD``
|
||||
stayed ``_UNSET`` for the whole turn (RED).
|
||||
"""
|
||||
streaming = importlib.import_module("api.streaming")
|
||||
pytest.importorskip("agent.runtime_cwd", reason="hermes-agent not installed")
|
||||
from agent import runtime_cwd as rc
|
||||
|
||||
assert rc._SESSION_CWD.get() is rc._UNSET
|
||||
tokens = streaming._set_turn_session_identity("sid-cwd-1", workspace="/tmp")
|
||||
try:
|
||||
assert "session_cwd" in tokens, (
|
||||
"binder ignored the workspace: _SESSION_CWD was never bound, so an "
|
||||
"in-process agent still resolves the server's launch directory"
|
||||
)
|
||||
assert rc._SESSION_CWD.get() == "/tmp"
|
||||
assert str(rc.resolve_agent_cwd()) == "/tmp"
|
||||
finally:
|
||||
streaming._reset_turn_session_identity(tokens)
|
||||
# Reset-token semantics, not a blanket clear: _UNSET (never "") so a reused
|
||||
# thread-pool worker leaks no cwd and CLI/cron env fallback resumes.
|
||||
assert rc._SESSION_CWD.get() is rc._UNSET
|
||||
|
||||
|
||||
def test_turn_identity_binder_omits_session_cwd_without_a_workspace():
|
||||
"""No workspace means no cwd binding, so non-workspace callers are unchanged."""
|
||||
streaming = importlib.import_module("api.streaming")
|
||||
pytest.importorskip("agent.runtime_cwd", reason="hermes-agent not installed")
|
||||
from agent import runtime_cwd as rc
|
||||
|
||||
tokens = streaming._set_turn_session_identity("sid-cwd-2")
|
||||
try:
|
||||
assert "session_cwd" not in tokens
|
||||
assert rc._SESSION_CWD.get() is rc._UNSET
|
||||
finally:
|
||||
streaming._reset_turn_session_identity(tokens)
|
||||
|
||||
|
||||
def test_concurrent_turns_keep_their_own_workspace_cwd():
|
||||
"""Two concurrent turns must not see each other's workspace.
|
||||
|
||||
This is the same invariant as the session-key race above, one variable over:
|
||||
``TERMINAL_CWD`` is a process-global the WebUI mutates per turn and restores
|
||||
afterwards, so an overlapping turn leaves another session's workspace in it.
|
||||
The contextvar binding is task-local and must therefore win.
|
||||
"""
|
||||
streaming = importlib.import_module("api.streaming")
|
||||
pytest.importorskip("agent.runtime_cwd", reason="hermes-agent not installed")
|
||||
from agent import runtime_cwd as rc
|
||||
|
||||
seen: dict[str, str] = {}
|
||||
barrier = threading.Barrier(2)
|
||||
|
||||
def turn(label: str, ws: str) -> None:
|
||||
tokens = streaming._set_turn_session_identity(f"sid-{label}", workspace=ws)
|
||||
try:
|
||||
# Both turns are now bound; each must still read its OWN workspace.
|
||||
barrier.wait(timeout=5)
|
||||
seen[label] = rc._SESSION_CWD.get()
|
||||
finally:
|
||||
streaming._reset_turn_session_identity(tokens)
|
||||
|
||||
threads = [
|
||||
threading.Thread(target=turn, args=("A", "/tmp")),
|
||||
threading.Thread(target=turn, args=("B", "/var/tmp")),
|
||||
]
|
||||
for t in threads:
|
||||
t.start()
|
||||
for t in threads:
|
||||
t.join(timeout=10)
|
||||
|
||||
assert seen.get("A") == "/tmp", f"turn A saw {seen.get('A')!r}"
|
||||
assert seen.get("B") == "/var/tmp", f"turn B saw {seen.get('B')!r}"
|
||||
assert rc._SESSION_CWD.get() is rc._UNSET
|
||||
Reference in new issue
Block a user