From 9d08c95464c90bd47e2159f97d63bd3f3d4cf65e Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Sat, 1 Aug 2026 15:09:02 -0700 Subject: [PATCH] fix(tools): dedup eviction task_id + workdir cwd leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent HIGH-severity correctness bugs found in a core-tools audit, each reproduced live against current main: 1. Read-dedup was never evicted after a write on non-default tasks. _invalidate_dedup_for_path looked up the read-tracker under the correct task_id but resolved the path with _resolve_path(filepath) — which DEFAULTS task_id='default'. The dedup cache is keyed by the task-resolved absolute path, so for any task whose workspace cwd differs from the process cwd (every -w worktree / Desktop / ACP session using relative paths) the computed key never matched and the stale entry was never removed. A read_file after a write_file/patch could then return the OLD content stub when mtime coincided. Fix: pass task_id through. 2. A per-command workdir override permanently hijacked the session cwd. The post-command dual-write unconditionally recorded env.cwd (stamped to the transient workdir) into the durable session-cwd store, so every later command that omitted workdir inherited the one-off directory — contradicting the documented 'Working directory for this command' contract. Fix: skip the session-cwd record when workdir was explicitly supplied. Both verified with sabotage-checked regression tests (fail without the fix). --- tests/tools/test_file_tools.py | 45 +++++++++++++++++++++++++++ tests/tools/test_terminal_task_cwd.py | 40 ++++++++++++++++++++++++ tools/file_tools.py | 2 +- tools/terminal_tool.py | 8 ++++- 4 files changed, 93 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_file_tools.py b/tests/tools/test_file_tools.py index 2fbe01f18d7a2..b009c009f25dc 100644 --- a/tests/tools/test_file_tools.py +++ b/tests/tools/test_file_tools.py @@ -657,3 +657,48 @@ class TestSilentFileMisplacementE2E: "file silently misplaced into config default (the #26211 bug)" tt.clear_session_cwd(task_id) + + +class TestDedupInvalidationTaskResolution: + """Real-IO regression: dedup eviction must resolve paths per-task. + + ``_invalidate_dedup_for_path`` looked up the read-tracker under the correct + task_id but resolved the path with the DEFAULT task, so for any task whose + workspace cwd differs from the process cwd (every ``-w``/Desktop/ACP + session using relative paths) the computed key never matched the cached + key and the stale-read entry was never evicted. A read after a write could + then be served the OLD content stub. + """ + + def test_invalidate_evicts_the_task_resolved_key(self, tmp_path, monkeypatch): + import tools.terminal_tool as tt + import tools.file_tools as ft + + workspace = tmp_path / "workspace" + proc = tmp_path / "proc" + workspace.mkdir() + proc.mkdir() + monkeypatch.delenv("TERMINAL_CWD", raising=False) + monkeypatch.chdir(proc) # process cwd != task workspace + + task_id = "acp-dedup" + monkeypatch.setattr(tt, "_task_env_overrides", {task_id: {"cwd": str(workspace)}}) + (workspace / "data.txt").write_text("v1\n") + + # The task resolves the relative path into the workspace; the default + # task (the old buggy resolution) would resolve into proc. + correct = str(ft._resolve_path("data.txt", task_id)) + buggy = str(ft._resolve_path("data.txt")) + assert correct != buggy, "test precondition: cwds must diverge" + + # Populate the dedup cache via a real read. + ft.read_file_tool("data.txt", task_id=task_id) + keys = [k[0] for k in ft._read_tracker.get(task_id, {}).get("dedup", {})] + assert correct in keys, keys + + # Invalidate as write_file_tool does; the entry must be gone. + ft._invalidate_dedup_for_path("data.txt", task_id) + remaining = [k[0] for k in ft._read_tracker.get(task_id, {}).get("dedup", {})] + assert correct not in remaining, remaining + + ft._read_tracker.pop(task_id, None) diff --git a/tests/tools/test_terminal_task_cwd.py b/tests/tools/test_terminal_task_cwd.py index 01039006bc9a7..447dfc332c8aa 100644 --- a/tests/tools/test_terminal_task_cwd.py +++ b/tests/tools/test_terminal_task_cwd.py @@ -76,6 +76,46 @@ def test_explicit_workdir_still_wins_over_registered_task_cwd(monkeypatch): assert calls == [{"timeout": 60, "cwd": "/explicit/workdir", "bounded_capture": True}] +def test_explicit_workdir_does_not_persist_into_session_cwd(monkeypatch): + """A per-command ``workdir`` must not hijack the durable session cwd. + + Regression: the post-command dual-write recorded ``env.cwd`` (stamped to + the transient ``workdir``) into the session-cwd store, so every later + command that omitted ``workdir`` inherited the one-off directory. + """ + recorded = [] + + class FakeEnv: + env = {} + cwd = "/workspace/acp" + + def execute(self, command, **kwargs): + # Marker parse stamps env.cwd to where the command ran. + self.cwd = kwargs.get("cwd", self.cwd) + return {"output": "ok", "returncode": 0} + + task_id = "acp-session-2" + monkeypatch.setattr(terminal_tool, "_active_environments", {task_id: FakeEnv()}) + monkeypatch.setattr(terminal_tool, "_last_activity", {}) + monkeypatch.setattr(terminal_tool, "_task_env_overrides", {task_id: {"cwd": "/workspace/acp"}}) + monkeypatch.setattr(terminal_tool, "_get_env_config", lambda: _minimal_terminal_config()) + monkeypatch.setattr( + terminal_tool, + "_check_all_guards", + lambda command, env_type, **kwargs: {"approved": True}, + ) + monkeypatch.setattr( + terminal_tool, + "record_session_cwd", + lambda session_key, cwd: recorded.append((session_key, cwd)), + ) + + terminal_tool.terminal_tool(command="pwd", task_id=task_id, workdir="/one/off/dir") + + # The transient workdir must NOT have been recorded as the session cwd. + assert all(cwd != "/one/off/dir" for _, cwd in recorded), recorded + + def test_background_command_prefers_recorded_session_cwd_over_init_time_cwd(monkeypatch): """Background process launches must also use the recorded session cwd.""" diff --git a/tools/file_tools.py b/tools/file_tools.py index 737d728c41393..a31ec7432e0a9 100644 --- a/tools/file_tools.py +++ b/tools/file_tools.py @@ -1530,7 +1530,7 @@ def _invalidate_dedup_for_path(filepath: str, task_id: str) -> None: internally. """ try: - resolved = str(_resolve_path(filepath)) + resolved = str(_resolve_path(filepath, task_id)) except (OSError, ValueError): return with _read_tracker_lock: diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index 6fa0cfe7e3dfa..5ebf349ae8a19 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -2924,7 +2924,13 @@ def terminal_tool( # session — record it under the session key so the durable record # never depends on the shared env surviving or on who drives the # env next. - record_session_cwd(session_key, getattr(env, "cwd", None)) + # + # BUT: a per-command ``workdir`` override is transient by contract + # (docstring: "Working directory for this command"). Recording it + # would hijack the session's durable cwd for every later command + # that doesn't pass ``workdir``. Skip the dual-write in that case. + if not workdir: + record_session_cwd(session_key, getattr(env, "cwd", None)) # Extract output output = result.get("output", "")