From 95a566b1e769cca26dfa441c1ff963ec03bdc7d6 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Fri, 24 Jul 2026 22:44:30 -0700 Subject: [PATCH] fix: brace-group the filtered export dump so $BASHPID expands in the parent shell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up for salvaged PR #69380. The snippet 'export -p | grep -vE ... > $tmp.$BASHPID || true' attaches the redirect to the grep pipeline segment, so $BASHPID expands inside grep's pipeline subshell — a DIFFERENT pid than the parent shell that expands the follow-up 'mv $tmp' operand. The dump landed in an orphaned temp file, mv failed silently (2>/dev/null), and the shared snapshot never updated again: exported env / venv activation stopped persisting between commands (tests/tools/test_local_shell_init.py TestSnapshotEndToEnd caught it). Wrap the pipeline in a brace group and attach the redirect to the group so the expansion happens in the current shell, matching mv's expansion. Also rename the shell-init probe var HERMES_SESSION_ENV_PROBE -> HERMES_STICKY_ENV_PROBE: it matched the HERMES_SESSION_ prefix the salvaged fix now intentionally strips from snapshots, and the snippet-shape test is updated to pin the brace-group contract. --- tests/tools/test_local_shell_init.py | 6 +++--- tests/tools/test_snapshot_session_id_leak.py | 9 ++++++++- tools/environments/base.py | 20 ++++++++++++++++++-- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/tests/tools/test_local_shell_init.py b/tests/tools/test_local_shell_init.py index f1e4f5b0451..178c02e6a95 100644 --- a/tests/tools/test_local_shell_init.py +++ b/tests/tools/test_local_shell_init.py @@ -194,12 +194,12 @@ class TestSnapshotEndToEnd: env = LocalEnvironment(cwd=str(tmp_path), timeout=15) try: first = env.execute( - 'export HERMES_SESSION_ENV_PROBE="sticky"; ' + 'export HERMES_STICKY_ENV_PROBE="sticky"; ' 'export PATH="/tmp/hermes-session-bin:$PATH"; ' - 'echo "first=$HERMES_SESSION_ENV_PROBE"' + 'echo "first=$HERMES_STICKY_ENV_PROBE"' ) second = env.execute( - 'echo "second=$HERMES_SESSION_ENV_PROBE"; echo "PATH=$PATH"' + 'echo "second=$HERMES_STICKY_ENV_PROBE"; echo "PATH=$PATH"' ) finally: env.cleanup() diff --git a/tests/tools/test_snapshot_session_id_leak.py b/tests/tools/test_snapshot_session_id_leak.py index d48f4f44968..c48555988ae 100644 --- a/tests/tools/test_snapshot_session_id_leak.py +++ b/tests/tools/test_snapshot_session_id_leak.py @@ -58,7 +58,14 @@ def test_export_snippet_shape(): assert "export -p" in snippet assert "grep -vE" in snippet assert "/tmp/snap.tmp.$BASHPID" in snippet - assert snippet.rstrip().endswith("|| true") + # The redirection must be attached to a brace group wrapping the pipeline, + # NOT to the grep segment: a redirect on grep expands $BASHPID inside + # grep's pipeline subshell (a different PID than the parent shell that + # expands the follow-up ``mv`` operand), silently orphaning the dump and + # breaking snapshot env persistence entirely. + assert snippet.lstrip().startswith("{ ") + assert "|| true; }" in snippet + assert snippet.rstrip().endswith("> /tmp/snap.tmp.$BASHPID") # --------------------------------------------------------------------------- diff --git a/tools/environments/base.py b/tools/environments/base.py index 7199079fb24..74c33aff753 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -412,9 +412,20 @@ def _export_dump_excluding_session_vars(tmp_path: str) -> str: persist across sessions in the shared snapshot. ``grep -vE`` returns exit 1 when it filters everything, so ``|| true`` keeps the pipeline's success contract intact for the callers that chain on it. + + The pipeline MUST be wrapped in a brace group with the redirection applied + to the group, not to the last pipeline segment. *tmp_path* typically embeds + ``$BASHPID`` for concurrency-safe temp names; a redirection attached + directly to ``grep`` is expanded inside grep's own pipeline subshell, where + ``$BASHPID`` resolves to the grep subshell's PID — while the caller's + follow-up ``mv $tmp`` expands in the parent shell to a DIFFERENT PID. The + dump then lands in an orphaned temp file and the snapshot silently never + updates (all exported-env persistence breaks). The brace-group redirect is + expanded in the current shell, keeping both expansions consistent. """ return ( - f"export -p | grep -vE '{_SNAPSHOT_EXCLUDED_ENV_REGEX}' > {tmp_path} || true" + f"{{ export -p | grep -vE '{_SNAPSHOT_EXCLUDED_ENV_REGEX}' || true; }} " + f"> {tmp_path}" ) @@ -678,9 +689,14 @@ class BaseEnvironment(ABC): # Chain mv on the export succeeding so a failed/partial dump never # replaces a good snapshot; drop the temp on failure so it isn't # orphaned (cleaned up wholesale in LocalEnvironment.cleanup too). + # NOTE: the redirection must be attached to a brace group, not to the + # grep pipeline segment — ``_snap_tmp`` embeds ``$BASHPID``, and a + # redirect on grep is expanded inside grep's pipeline subshell (a + # different PID than the parent shell that expands the ``mv`` operand), + # silently orphaning the dump. See _export_dump_excluding_session_vars. if self._snapshot_ready: parts.append( - f"{{ export -p | grep -vE '{_SNAPSHOT_EXCLUDED_ENV_REGEX}' > {_snap_tmp} " + f"{{ {_export_dump_excluding_session_vars(_snap_tmp)} " f"&& mv -f {_snap_tmp} {_quoted_snap}; }} " f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true" )