fix(tui_gateway): tolerate a deleted working directory across session, completion and exec paths (supersedes #40153) - #76131
Conversation
…, completion and exec paths os.getcwd() raises FileNotFoundError once the process's working directory is removed out from under it — the folder Hermes was launched in gets deleted, rebuilt, or `git worktree remove`'d mid-session. tui_gateway called it unguarded on seven fallback paths. This is process death, not degradation. session.create and session.resume are not in server._LONG_HANDLERS, so they run inline on the reader thread, and tui_gateway/entry.py calls dispatch(req) with no try/except — an escaping FileNotFoundError exits the stdio gateway. Only the Desktop/WS path degrades, because ws.py catches around its dispatch call. Add _safe_getcwd() to tui_gateway/server.py, mirroring the already-merged tools/terminal_tool.py helper (NousResearch#39491 / ad69d3e) exactly, including its TERMINAL_CWD -> home fallback chain, so the two surfaces cannot drift. Catch FileNotFoundError only, for parity: a genuine PermissionError still surfaces rather than being masked. Substitute at all seven sites: server.py _SlashWorker spawn, _default_session_cwd, _completion_cwd or-chain, _completion_cwd isdir-failed tail methods_tools.py cli.exec, config.show, shell.exec _completion_cwd's tail is reachable even when the client supplies a cwd: in this scenario that cwd IS the deleted directory, so os.path.isdir returns False and the fallback fires. The `except Exception: pass` above it does not cover that return. methods_tools.py calls the bare name with no import. HandlerRegistry.install() rebuilds every handler with types.FunctionType(fn.__code__, vars(server), ...), so handler bodies resolve against server.py's namespace — the same reason methods_complete.py calls bare _completion_cwd. A test pins that the name is resolvable in each rebound handler's __globals__ so these sites cannot ship a latent NameError. The _SlashWorker subprocess block is otherwise untouched: profile-home env scoping (NousResearch#40677), UTF-8 lossy decode (NousResearch#53137), windows_hide_flags() and start_new_session=True are all preserved, and asserted in the new test. tui_gateway/compute_host.py's two os.getcwd() calls are deliberately excluded: host_supervisor.py spawns that child with an explicit cwd=_repo_root(), so the host process's cwd is a live installation-owned directory by construction and cannot be the deleted launch dir.
There was a problem hiding this comment.
🟢 Ready to approve
The change is narrowly scoped, fixes a concrete crash condition, and includes direct regression coverage for the affected call paths.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Fixes a TUI gateway crash class where os.getcwd() raises FileNotFoundError after the process working directory is deleted mid-session, by centralizing a safe cwd resolver in tui_gateway and routing all relevant fallback paths through it.
Changes:
- Add
tui_gateway.server._safe_getcwd()and use it for_SlashWorker,_default_session_cwd(), and both_completion_cwd()fallback paths. - Replace
os.getcwd()with_safe_getcwd()intui_gateway/methods_tools.pyforcli.exec,config.show, andshell.exec. - Add a focused regression test suite covering helper behavior and the substituted call sites.
File summaries
| File | Description |
|---|---|
| tui_gateway/server.py | Introduces _safe_getcwd() and routes session/completion/slash-worker cwd fallbacks through it to prevent stdio gateway process exits. |
| tui_gateway/methods_tools.py | Uses _safe_getcwd() for tool subprocess cwd and config display to avoid crashes when the launch CWD is deleted. |
| tests/tui_gateway/test_safe_getcwd.py | Adds regression coverage for deleted-CWD behavior across helper, session defaults, completion, slash worker, and handler rebinding. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 2
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
| Every ``os.getcwd()`` in this package is a *fallback* on a path that has | ||
| already exhausted its explicit sources, so a raise here is never recoverable | ||
| locally — ``session.create``/``session.resume`` run inline (they are not in | ||
| ``_LONG_HANDLERS``) and ``tui_gateway/entry.py`` does not guard | ||
| ``dispatch()``, so the stdio gateway process exits. | ||
|
|
||
| Mirrors :func:`tools.terminal_tool._safe_getcwd` (added in #39491) exactly, | ||
| including the TERMINAL_CWD -> home fallback chain, so the two surfaces | ||
| cannot drift. |
| def test_slash_worker_spawns_with_fallback_cwd_and_preserves_contract(monkeypatch, tmp_path): | ||
| """The cwd substitution must not disturb the profile-home env scoping | ||
| (#40677), UTF-8 lossy decode (#53137), windows_hide_flags() or | ||
| start_new_session=True that this block has accumulated.""" | ||
| monkeypatch.setenv("TERMINAL_CWD", str(tmp_path)) | ||
| with patch.dict("sys.modules", { | ||
| "hermes_constants": MagicMock( | ||
| get_hermes_home=MagicMock(return_value=str(tmp_path)) | ||
| ), | ||
| }): | ||
| with patch("subprocess.Popen") as mock_popen: | ||
| mock_popen.return_value.stdout = MagicMock() | ||
| mock_popen.return_value.stderr = MagicMock() | ||
| with patch("os.getcwd", _getcwd_raises()): | ||
| server._SlashWorker(session_key="k", model="m") | ||
|
|
||
| assert mock_popen.called, "Popen was not invoked" | ||
| kwargs = mock_popen.call_args[1] | ||
| assert kwargs["cwd"] == str(tmp_path) | ||
| # preservation guarantee, asserted rather than promised | ||
| assert kwargs["env"] is not None | ||
| assert kwargs["start_new_session"] is True | ||
| assert kwargs["encoding"] == "utf-8" | ||
| assert kwargs["errors"] == "replace" |
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused deleted-CWD recovery coverage. The premise is confirmed on current main: tui_gateway/server.py:2179 and :2187 retain unguarded completion fallbacks, _default_session_cwd() calls os.getcwd() at :1371, and the three methods_tools.py sites are likewise unguarded. The proposed helper matches the merged terminal precedent at tools/terminal_tool.py:1314.
Problems
- The new helper and test-module docstrings say
session.resumeis not in_LONG_HANDLERSand runs inline. Current main listssession.resumeattui_gateway/server.py:276andshell.execat:277; these paths are caught by the pool wrapper at:1793-1798. The safe fallback remains correct, but the claimed process-death mechanism is only accurate for inline paths such assession.create.
Suggested changes
- Narrow those comments to the actual inline path and scope the “all sites” language to the seven selected fallback sites.
tui_gateway/compute_host.py:520and:752remain directos.getcwd()calls, with the normal supervisor construction using an explicit repository-root cwd (tui_gateway/host_supervisor.py:326-329).
Automated hermes-sweeper review.
|
|
||
| Mirrors :func:`tools.terminal_tool._safe_getcwd` (added in #39491) exactly, | ||
| including the TERMINAL_CWD -> home fallback chain, so the two surfaces | ||
| cannot drift. |
There was a problem hiding this comment.
Please scope this claim to the seven selected fallback sites. tui_gateway/compute_host.py:520 and :752 still call os.getcwd() directly (the normal supervisor starts that child with an explicit repository-root cwd), so “every os.getcwd() in this package” and “cannot drift” overstate the guarantee.
There was a problem hiding this comment.
You're right — both claims were too broad. Scoped in c6d5a4a.
The docstring now names the seven sites this helper actually guards, instead of claiming every os.getcwd() in the package: _SlashWorker, _default_session_cwd and both _completion_cwd returns in server.py, plus cli.exec, config.show and shell.exec in methods_tools.py.
It also records the compute_host.py:520 / :752 exclusion and the reason, as you describe it: host_supervisor.py:328 starts that child with cwd=str(self.cwd), which defaults to _repo_root() (host_supervisor.py:150), so the compute host never inherits the launch directory and cannot observe it deleted — Popen would fail before the child ran if it could. Those two stay bare os.getcwd() on that basis rather than widening the diff.
"cannot drift" is gone. The two bodies are byte-identical today, but they are independent copies with nothing enforcing the mirror, so the docstring now says to change both together.
The same commit also tightens the _SlashWorker test, which claimed to cover profile-home env scoping (#40677) but passed no profile_home= and only asserted env is not None. It now passes a profile home distinct from the expected fallback cwd and asserts HERMES_HOME in the spawned env; the assertion fails if the build_subprocess_env extra= override is dropped.
teknium1
added
sweeper:blast-moderate
…ack sites The docstring claimed every os.getcwd() in tui_gateway is a fallback and that this helper "cannot drift" from tools.terminal_tool._safe_getcwd. Both overstate the guarantee. compute_host.py:520 and :752 still call os.getcwd() directly, and that is correct: host_supervisor.py spawns the compute host with cwd=str(self.cwd) (:328), which defaults to _repo_root() (:150), so that child cannot observe a deleted launch directory — Popen would fail first if it could. Name the seven sites this helper actually guards and record the exclusion with its reason. The two helpers are byte-identical today but are independent copies with nothing enforcing the mirror, so say "change both together" instead of claiming they cannot drift. Also make the _SlashWorker test assert the contract its docstring promises: it claimed to cover profile-home env scoping (NousResearch#40677) while passing no profile_home= and only asserting env is not None. It now passes a profile home distinct from the expected fallback cwd and asserts HERMES_HOME in the spawned env, which fails if the build_subprocess_env extra= override is removed.
What does this PR do?
The guard for this exact bug is already enforced in
tools/and entirely absent intui_gateway/.tools/terminal_tool.py::_safe_getcwd(:1314) landed as mergedad69d3edc/ #39491 "fix(terminal): guard os.getcwd() against a deleted CWD"; #19928 "fix(local): recover when persistent_shell cwd is deleted (#17558)" and #19933 merged the same idea on the local-shell surface.git grep _safe_getcwd -- tui_gateway/on main returns nothing — the TUI gateway never got it. This PR closes that asymmetry.In user terms: "I deleted (or
git worktree remove'd, or rebuilt) the folder I launched Hermes in, and now the TUI can't open a new chat, can't resume a session, and slash commands are dead — the gateway just falls over."os.getcwd()raisesFileNotFoundErroronce the process's working directory is removed out from under it.tui_gateway/calls it unguarded on seven fallback paths.This is process death, not degradation.
session.createandsession.resumeare not inserver._LONG_HANDLERS, sodispatch()runs them inline viareturn handle_request(req)with noexcept, andtui_gateway/entry.py:480callsresp = dispatch(req)with notry/exceptand none inmain()— an escapingFileNotFoundErrorexits the stdio gateway process. Only the Desktop/WS path degrades, becausetui_gateway/ws.py:383catches around its own dispatch call and returns-32603.Reachability chains, verified on main:
session.createmethods_session.py:34→_completion_cwdsession.resumemethods_session.py:423/:512→_default_session_cwdshell.execmethods_tools.py:1889-32000/5003, command never runsconfig.showmethods_tools.py:14115030, settings panel fails to rendercli.execmethods_tools.py:394-32000 handler error_completion_cwd's isdir-failed tail is reachable even when the client supplies acwd— in this scenario the client's last-known cwd is the deleted directory, soos.path.isdir()returns False and the fallback fires. Theexcept Exception: passabove it does not cover thatreturn.Related Issue
No issue exists for this surface, so there is no
Fixes:line. The nearest filed report, #62169 ("Terminal sandbox: deleted CWD permanently breaks all subsequent commands (exit 126)"), is the terminal-sandbox surface and is already fixed there — cited as corroborating evidence that users hit this class in the wild, not as the issue this closes.Supersedes #40153 (@Dusk1e). See below.
Supersedes #40153 — deficiency shown mechanically, not argued
#40153 identified a real bug and its helper is sound. It has been
CONFLICTING/DIRTYsince 2026-06-05 (last commit2026-06-05T22:26:10Z; its only force-push predates the 07-14 review by five weeks) and is incomplete against current main. Two mechanical facts, not opinions:grep -c '_default_session_cwd'over its entire diff returns 0. It never touches the site the review explicitly named.208 / 234 / 793 / 801; main's real call sites are381 / 1371 / 2179 / 2187— roughly 1400 lines of drift. That drift is why it readsDIRTY, and the_SlashWorkerblock it patches has since gained profile-home env scoping ([Bug]: Profile-local skills are unavailable in Dashboard/TUI/Desktop GUI because child processes use the root HERMES_HOME #40677), UTF-8 lossy decode (fix: TUI gateway crash on Windows - UnicodeDecodeError in subprocess _readerthread #53137),windows_hide_flags()andstart_new_session=True. Its diff still shows the oldenv=os.environ.copy()form with noencoding=/creationflags=/start_new_session=.The 2026-07-14 automated review on #40153 (
keep_open salvageability=high) states both points:Both are satisfied here:
_default_session_cwdis substituted and covered by a dedicated test, and the_SlashWorkerblock is preserved byte-for-byte apart from thecwd=argument — withenv,creationflags,start_new_session=True,encoding="utf-8"anderrors="replace"asserted in a test rather than merely promised.Credit for identifying the failure path belongs to @Dusk1e; this is the complete, rebased version against current main.
Three-way red-before proof
Run against the new test file, which is identical in all three legs. This is the artifact that distinguishes a genuine improvement from a re-file:
tui_gatewaystateorigin/main, no fix_SlashWorker+ both_completion_cwdsites)_default_session_cwdand all threemethods_toolscases still redLeg 2's four survivors are exactly
test_default_session_cwd_survives_deleted_cwd,test_cli_exec_runs_with_fallback_cwd,test_shell_exec_runs_with_fallback_cwd,test_config_show_reports_fallback_working_dir.Changes Made
tui_gateway/server.py— added_safe_getcwd()immediately above_default_session_cwd, mirroringtools/terminal_tool.py:1314exactly (sameTERMINAL_CWD→expanduser("~")chain) so the two surfaces cannot drift. Substituted at four sites:_SlashWorkerspawn (:381),_default_session_cwd(:1371),_completion_cwdor-chain (:2179),_completion_cwdisdir-failed tail (:2187).tui_gateway/methods_tools.py— substituted at three more sites sharing the identical root cause:cli.exec(:394),config.show(:1411),shell.exec(:1889).tests/tui_gateway/test_safe_getcwd.py— new file, 14 tests.Exception type —
FileNotFoundError, notOSError. Chosen for exact parity with the mergedtools/terminal_tool.pyidiom, so a reader diffing the two helpers sees them as identical. A genuinePermissionErrortherefore still surfaces instead of being silently masked, and a test pins that. #67308 is separately proposing to widen thetools/helper to cover the macOS-TCCPermissionErrorcase; if that merges, widening this copy to match is a one-line follow-up. Diverging pre-emptively would create exactly the drift this PR exists to remove.No import added to
methods_tools.py— this is deliberate.tui_gateway/method_ctx.py::HandlerRegistry.install()rebuilds every handler withtypes.FunctionType(fn.__code__, vars(server), ...), so names in amethods_*.pyhandler body resolve againstserver.py's namespace — which is whymethods_complete.pycalls bare_completion_cwdwhile importing onlyHandlerRegistry. All three sites were individually confirmed to sit inside@methodbodies (cli.exec:371,config.show:1383,shell.exec:1866), and a parametrized test asserts_safe_getcwdis present in each rebound handler's__globals__so these sites cannot ship a latentNameError.Sites deliberately excluded
tui_gateway/compute_host.py:520(str(frame.get("cwd") or os.getcwd())) and:752(thehellohandshake) are not substituted. Both rest on one verifiable fact rather than two separate arguments:tui_gateway/host_supervisor.py:326spawns that child with an explicitcwd=str(self.cwd), andself.cwddefaults to_repo_root()(:150), never the user's launch directory. The compute-host process therefore always runs with a live, installation-owned cwd, soos.getcwd()there cannot observe the deleted launch dir — and if that directory were itself missing,Popenwould fail at spawn time and the child would never reach either line. Substituting there would add unreachable code.How to Test
To reproduce the original failure by hand:
mkdir /tmp/scratch && cd /tmp/scratch, launch the TUI from there.rmdir /tmp/scratch.session.create) or resume one (session.resume). Onmainthe stdio gateway process exits; with this PR the session opens inTERMINAL_CWD, falling back to the user's home directory.Per-file results (CI uses per-file subprocess isolation, so these were verified per file, not via a flat
pytest tests/run):tests/tui_gateway/test_safe_getcwd.pytests/tui_gateway/test_protocol.pytests/tui_gateway/test_session_cwd_follow.pytests/tui_gateway/test_subprocess_encoding.pytests/tui_gateway/test_slash_worker_profile_home.pytests/tui_gateway/test_slash_worker_sys_path.pytests/tui_gateway/test_slash_worker_ansi.pytests/tui_gateway/test_slash_worker_mcp_discovery.pytests/gateway/test_complete_path_at_filter.pytests/gateway/test_completion_delivery.pytests/gateway/test_config_cwd_bridge.pytests/gateway/test_cwd_placeholder.pyChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests pass — ran the affected files individually instead (table above); a flatpytest tests/run is not reproducible in this repo, and CI isolates per fileDocumentation & Housekeeping
docs/, docstrings) — docstrings on the new helper and the_completion_cwdtail; no user-facing docs affectedos.getenv+expanduser); noos.namebranching was introduced, and the Windows-specificcreationflags/windows_hide_flags()on both subprocess blocks are preserved and asserted. Executed on macOS only.