fix(tools): dedup eviction task_id + workdir cwd leak
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).
This commit is contained in:
parent
9467e99ac5
commit
9d08c95464
|
|
@ -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)
|
||||
|
|
|
|||
|
|
@ -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."""
|
||||
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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", "")
|
||||
|
|
|
|||
Loading…
Reference in New Issue