Redundant temp-file read in LocalEnvironment._update_cwd

Fixes on record

Merged to main. A stamp counts the issues a fix is linked to.

• #65483 fix(local): parse cwd from the stdout marker, drop the temp-file write

  merged to main

  files: tests/tools/test_base_environment.py, tests/tools/test_local_env_cwd_recovery.py, tests/tools/test_local_env_windows_msys.py, tools/environments/base.py, tools/environments/local.py

  Show the diff

  • tests/tools/test_base_environment.py

    @@ -68,7 +68,8 @@ def test_basic_shape(self):
             assert "eval 'echo hello'" in wrapped
             assert "__hermes_ec=$?" in wrapped
             assert "export -p >" in wrapped
    -        assert "pwd -P >" in wrapped
    +        # cwd travels via the stdout marker only — no temp-file write.
    +        assert "pwd -P >" not in wrapped
             assert env._cwd_marker in wrapped
             assert "exit $__hermes_ec" in wrapped
     
    @@ -357,7 +358,9 @@ def cleanup(self):
     
                 assert stat.S_IMODE(user_file.stat().st_mode) == 0o644
                 assert stat.S_IMODE(Path(env._snapshot_path).stat().st_mode) == 0o600
    -            assert stat.S_IMODE(Path(env._cwd_file).stat().st_mode) == 0o600
    +            # The cwd temp file is no longer written (cwd travels via the
    +            # stdout marker for every backend) — nothing to leak on disk.
    +            assert not Path(env._cwd_file).exists()
             finally:
                 os.umask(old_umask)
     

  • tests/tools/test_local_env_cwd_recovery.py

    @@ -160,13 +160,14 @@ def test_skips_assignment_when_marker_path_missing(self, tmp_path):
             with patch.object(LocalEnvironment, "init_session", autospec=True, return_value=None):
                 env = LocalEnvironment(cwd=str(original), timeout=10)
     
    -        # Simulate the stale-marker case: the prior command's ``pwd -P`` left
    -        # a path in the cwd file, but that path has since been deleted.
    +        # Simulate the stale-marker case: the prior command emitted a cwd
    +        # marker for a directory that has since been deleted.
             deleted = tmp_path / "wedge-repro"
    -        with open(env._cwd_file, "w") as f:
    -            f.write(str(deleted))
    +        marker = env._cwd_marker
     
    -        env._update_cwd({"output": "", "returncode": 0})
    +        env._update_cwd(
    +            {"output": f"x\n{marker}{deleted}{marker}\n", "returncode": 0}
    +        )
     
             assert env.cwd == str(original)
     
    @@ -178,10 +179,10 @@ def test_accepts_assignment_when_marker_path_exists(self, tmp_path):
     
             with patch.object(LocalEnvironment, "init_session", autospec=True, return_value=None):
                 env = LocalEnvironment(cwd=str(original), timeout=10)
    +        marker = env._cwd_marker
     
    -        with open(env._cwd_file, "w") as f:
    -            f.write(str(new_dir))
    -
    -        env._update_cwd({"output": "", "returncode": 0})
    +        env._update_cwd(
    +            {"output": f"x\n{marker}{new_dir}{marker}\n", "returncode": 0}
    +        )
     
             assert env.cwd == str(new_dir)

  • tests/tools/test_local_env_windows_msys.py

    @@ -180,15 +180,15 @@ def test_msys_path_resolves_to_native_when_native_exists(
     
     
     # ---------------------------------------------------------------------------
    -# End-to-end: _update_cwd via marker file (Windows simulation)
    +# End-to-end: _update_cwd via stdout marker (Windows simulation)
     # ---------------------------------------------------------------------------
     
     class TestUpdateCwdWindowsMsys:
    -    def test_marker_file_msys_path_stored_in_native_form(
    +    def test_marker_output_msys_path_stored_in_native_form(
             self, monkeypatch, tmp_path,
         ):
    -        """When Git Bash writes ``/c/Users/x`` to the cwd marker file on
    -        Windows, ``_update_cwd`` must translate to native form before
    +        """When Git Bash emits ``/c/Users/x`` in the cwd marker on Windows,
    +        ``_update_cwd`` must translate to native form before
             validating and storing — otherwise ``os.path.isdir`` rejects a
             perfectly real directory."""
             original = tmp_path / "starting"
    @@ -205,18 +205,21 @@ def test_marker_file_msys_path_stored_in_native_form(
             # Pretend Git Bash wrote an MSYS path that maps to tmp_path/"next"
             new_dir = tmp_path / "next"
             new_dir.mkdir()
    +        marker = env._cwd_marker
     
    -        with open(env._cwd_file, "w") as f:
    -            f.write("/c/whatever/from/bash")
    -
    -        # Translate the synthetic MSYS string to the real native dir.
    +        # Translate the synthetic MSYS marker path to the real native dir.
             def fake_translate(p):
                 if p == "/c/whatever/from/bash":
                     return str(new_dir)
                 return p
     
             with patch.object(local_mod, "_msys_to_windows_path", side_effect=fake_translate):
    -            env._update_cwd({"output": "", "returncode": 0})
    +            env._update_cwd(
    +                {
    +                    "output": f"x\n{marker}/c/whatever/from/bash{marker}\n",
    +                    "returncode": 0,
    +                }
    +            )
     
             assert env.cwd == str(new_dir)
     

  • tools/environments/base.py

    @@ -478,7 +478,6 @@ def init_session(self):
             # ``Directory \\drivers\\etc does not exist`` failure class.
             # On POSIX this is plain ``shlex.quote``.
             _quoted_snap = self._quote_shell_path(self._snapshot_path)
    -        _quoted_cwd_file = self._quote_shell_path(self._cwd_file)
             # Use atomic file replacement: assemble the snapshot in a temp file,
             # then mv it over the final path.  This prevents concurrent source()
             # calls from reading a half-written snapshot when another terminal
    @@ -523,7 +522,6 @@ def init_session(self):
                 # partial temp rather than leave it to be sourced or orphaned.
                 f"mv -f {_snap_tmp} {_quoted_snap} || rm -f {_snap_tmp}\n"
                 f"builtin cd -- {_quoted_cwd} 2>/dev/null || true\n"
    -            f"pwd -P > {_quoted_cwd_file} 2>/dev/null || true\n"
                 f"printf '\\n{self._cwd_marker}%s{self._cwd_marker}\\n' \"$(pwd -P)\"\n"
             )
             try:
    @@ -602,10 +600,9 @@ def _wrap_command(self, command: str, cwd: str) -> str:
             re-dumps env vars, and emits CWD markers."""
             escaped = command.replace("'", "'\\''")
     
    -        # Quote snapshot/cwd-file paths (see init_session — LocalEnvironment
    -        # rewrites ``C:/...`` to ``/c/...`` so MSYS doesn't mangle them).
    +        # Quote the snapshot path (see init_session — LocalEnvironment
    +        # rewrites ``C:/...`` to ``/c/...`` so MSYS doesn't mangle it).
             _quoted_snap = self._quote_shell_path(self._snapshot_path)
    -        _quoted_cwd_file = self._quote_shell_path(self._cwd_file)
             # Use atomic file replacement for env snapshot updates (issue #38249).
             # Assemble into a per-writer-unique temp file, then mv to atomically
             # replace the snapshot so concurrent source() calls never read a
    @@ -651,8 +648,8 @@ def _wrap_command(self, command: str, cwd: str) -> str:
                     f"2>/dev/null || rm -f {_snap_tmp} 2>/dev/null || true"
                 )
     
    -        # Write CWD to file (local reads this) and stdout marker (remote parses this)
    -        parts.append(f"pwd -P > {_quoted_cwd_file} 2>/dev/null || true")
    +        # Emit the CWD stdout marker; all backends (including local, since
    +        # PR #63255) parse it from output — no temp-file write needed.
             # Use a distinct line for the marker. The leading \n ensures
             # the marker starts on its own line even if the command doesn't
             # end with a newline (e.g. printf 'exact'). We'll strip this

  • tools/environments/local.py

    @@ -1314,31 +1314,14 @@ def _wait_for_group_exit(pgid: int, timeout: float) -> bool:
                     pass
     
         def _update_cwd(self, result: dict):
    -        """Read CWD from temp file (local-only, no round-trip needed).
    -
    -        Skip the assignment when the path no longer exists as a directory —
    -        ``pwd -P`` on a deleted cwd can leave a stale value in the marker
    -        file, and propagating it would re-wedge the next ``Popen``.  The
    -        ``_run_bash`` recovery path will resolve a safe fallback if needed.
    -
    -        On Windows, the value written by Git Bash's ``pwd -P`` is in
    -        MSYS form (``/c/Users/x``). Translate it to native Windows form
    -        before validating with ``os.path.isdir`` and before storing on
    -        ``self.cwd``; otherwise the isdir check rejects every valid
    -        result and ``_run_bash`` later prints a misleading "cwd is
    -        missing" warning on every command.
    -        """
    -        try:
    -            with open(self._cwd_file, encoding="utf-8") as f:
    -                cwd_path = f.read().strip()
    -            if _IS_WINDOWS:
    -                cwd_path = _msys_to_windows_path(cwd_path)
    -            if cwd_path and os.path.isdir(cwd_path):
    -                self.cwd = cwd_path
    -        except (OSError, FileNotFoundError):
    -            pass
    +        """Update cwd from the stdout marker emitted by the wrapped command.
     
    -        # Still strip the marker from output so it's not visible
    +        The base command wrapper already appends ``pwd -P`` to stdout inside a
    +        session-specific marker, so the local backend can share the same parser
    +        as remote backends instead of re-reading the temp file it just wrote.
    +        ``_extract_cwd_from_output`` keeps the local Windows normalization and
    +        stale-path rollback semantics intact.
    +        """
             self._extract_cwd_from_output(result)
     
         def _extract_cwd_from_output(self, result: dict):