From 4fa8d7bb67dbc7c48332ac042e133ee27c5cdf07 Mon Sep 17 00:00:00 2001 From: kshitij <82637225+kshitijk4poor@users.noreply.github.com> Date: Sun, 2 Aug 2026 22:31:18 +0530 Subject: [PATCH] fix(tools): staleness + tracking-parity fixes for the not-found cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the #25387 salvage: 1. CRITICAL: a cached miss survived out-of-band file creation (terminal command, external process) for the full 60s TTL — breaking the common agent pattern 'check for file -> create it -> read it' (live-repro'd). Serve-side existence guard: one ~free stat before serving a cached miss; if the path now exists the entry is evicted and the real read runs. Also fixes the search-root variant (write under a cached-missing directory). Both mutation-checked. 2. notify_other_tool_call now clears the task's not_found entries too (belt: the dispatcher calls it for every non-read tool). 3. Tracking parity: the record sites no longer early-return. On upstream, error results flow through consecutive-loop detection and dedup bookkeeping; short-circuiting skipped that and broke TestDedupInvalidationTaskResolution when preceded by TestSilentFileMisplacementE2E (bisected: the early return at the read record site was the trigger). Recording is now side-effect-identical to upstream; serving from the cache remains the optimization. Also reuse the already-computed _resolved instead of resolving a second time. --- tests/tools/test_file_tools.py | 74 ++++++++++++++++++++++++++++++++++ tools/file_tools.py | 33 +++++++++++++-- 2 files changed, 103 insertions(+), 4 deletions(-) diff --git a/tests/tools/test_file_tools.py b/tests/tools/test_file_tools.py index 1096194968..1efe656698 100644 --- a/tests/tools/test_file_tools.py +++ b/tests/tools/test_file_tools.py @@ -920,3 +920,77 @@ class TestNotFoundCache: assert _check_not_found_cache("read", "/tmp/ttl-test", tid) is None with ft._read_tracker_lock: assert ("read", "/tmp/ttl-test") not in _read_tracker[tid].get("not_found", {}) + + def test_out_of_band_creation_defeats_cached_miss(self, tmp_path): + """CRITICAL staleness contract: a file created AFTER a cached miss — + by a terminal command or any external process, NOT write_file_tool — + must be served for real on the next read. The agent pattern + 'check for file → create it → read it' breaks otherwise.""" + from tools.file_tools import ( + _check_not_found_cache, + _record_not_found, + _read_tracker, + ) + + tid = "neg-cache-oob-read" + _read_tracker.pop(tid, None) + target = tmp_path / "created-later.txt" + + _record_not_found("read", str(target), tid, '{"error":"File not found: x"}') + assert _check_not_found_cache("read", str(target), tid) is not None + + # Out-of-band creation: plain filesystem write, no tool hook fires. + target.write_text("real content\n") + + # The cached miss must NOT be served once the path exists… + assert _check_not_found_cache("read", str(target), tid) is None, ( + "stale 'File not found' served after the file was created " + "out-of-band — the existence guard regressed" + ) + # …and the entry is evicted, not just skipped. + with __import__("tools.file_tools", fromlist=["x"])._read_tracker_lock: + assert ("read", str(target)) not in _read_tracker[tid].get("not_found", {}) + + def test_out_of_band_creation_defeats_cached_search_miss(self, tmp_path): + """Same contract for search roots: creating a file under a + previously-missing directory must defeat the cached 'Path not found'.""" + from tools.file_tools import ( + _check_not_found_cache, + _record_not_found, + _read_tracker, + ) + + tid = "neg-cache-oob-search" + _read_tracker.pop(tid, None) + missing_dir = tmp_path / "later-dir" + + _record_not_found("search", str(missing_dir), tid, '{"error":"Path not found: x"}') + assert _check_not_found_cache("search", str(missing_dir), tid) is not None + + missing_dir.mkdir() + (missing_dir / "x.txt").write_text("hi\n") + + assert _check_not_found_cache("search", str(missing_dir), tid) is None, ( + "stale 'Path not found' served after the directory was created" + ) + + def test_notify_other_tool_call_clears_not_found(self): + """Belt-and-suspenders: any non-read tool (terminal etc.) invalidates + the task's negative cache via the dispatcher's notify hook.""" + from tools.file_tools import ( + _check_not_found_cache, + _record_not_found, + _read_tracker, + notify_other_tool_call, + ) + + tid = "neg-cache-notify" + _read_tracker.pop(tid, None) + _record_not_found("read", "/tmp/never-exists-notify", tid, '{"error":"x"}') + assert _check_not_found_cache("read", "/tmp/never-exists-notify", tid) is not None + + notify_other_tool_call(tid) + + assert _check_not_found_cache("read", "/tmp/never-exists-notify", tid) is None, ( + "notify_other_tool_call must clear cached misses" + ) diff --git a/tools/file_tools.py b/tools/file_tools.py index 88d79c0c33..1078d736df 100644 --- a/tools/file_tools.py +++ b/tools/file_tools.py @@ -954,6 +954,7 @@ def _check_not_found_cache(op: str, resolved_str: str, task_id: str) -> str | No Eviction: TTL or write_file/patch on the path (see invalidate_for_path). """ + import os as _os import time with _read_tracker_lock: task_data = _read_tracker.get(task_id) @@ -969,6 +970,15 @@ def _check_not_found_cache(op: str, resolved_str: str, task_id: str) -> str | No if time.monotonic() - ts > _NOT_FOUND_TTL_SECONDS: nf.pop((op, resolved_str), None) return None + # Existence guard: the path may have been created since we cached + # the miss — by a terminal command, another agent, or any external + # process (write_file/patch invalidate explicitly, but they're not + # the only writers). The agent pattern "check file → create it → + # read it" is common; serving a stale miss for up to the TTL breaks + # it. One stat is ~free next to the subprocess walk we're skipping. + if _os.path.exists(resolved_str): + nf.pop((op, resolved_str), None) + return None return cached_json @@ -1342,7 +1352,7 @@ def read_file_tool(path: str, offset: int = 1, limit: int = 500, task_id: str = # If we already discovered this path doesn't exist (within TTL), # return the cached error without spawning the subprocess + # similar-files walk. Cleared by write_file/patch on the same path. - resolved_str_for_neg = str(_resolve_path_for_task(path, task_id)) + resolved_str_for_neg = str(_resolved) cached_not_found = _check_not_found_cache("read", resolved_str_for_neg, task_id) if cached_not_found is not None: return cached_not_found @@ -1413,11 +1423,16 @@ def read_file_tool(path: str, offset: int = 1, limit: int = 500, task_id: str = # ── Populate negative-result cache on not-found ─────────────── # _suggest_similar_files returns ReadResult(error="File not found: .."). # Cache the JSON we'd return so a retry skips the parent-dir walk. + # Deliberately NO early return: on upstream, error results flow + # through the tracking block below (consecutive-loop detection, + # dedup bookkeeping via the resolved path) and the normal exit — + # short-circuiting here changes that behavior (and broke a real + # test interaction). Serving from the cache (above) is the + # optimization; recording must stay side-effect-identical. _err = result_dict.get("error") or "" if isinstance(_err, str) and _err.startswith("File not found:"): _not_found_json = json.dumps(result_dict, ensure_ascii=False) _record_not_found("read", resolved_str_for_neg, task_id, _not_found_json) - return _not_found_json # ── Character-count guard ───────────────────────────────────── # We're model-agnostic so we can't count tokens; characters are @@ -1594,6 +1609,15 @@ def notify_other_tool_call(task_id: str = "default"): # progress, so clear per-key dedup hit counters too. if "dedup_hits" in task_data: task_data["dedup_hits"].clear() + # Any other tool (terminal, delegate, ...) may have created a + # previously-missing path — a cached miss is no longer + # trustworthy. The serve-side existence guard in + # _check_not_found_cache already covers this, but clearing + # here keeps the cache honest and covers exotic cases the + # stat can't (e.g. permission flips). + nf = task_data.get("not_found") + if nf: + nf.clear() def _invalidate_dedup_for_path(filepath: str, task_id: str) -> None: @@ -2085,12 +2109,13 @@ def search_tool(pattern: str, target: str = "content", path: str = ".", "token, cache, or secret-bearing environment files." ) - # Populate negative cache when search root was missing. + # Populate negative cache when search root was missing. No early + # return — same rationale as the read path: error results keep + # flowing through the consecutive-search bookkeeping below. _search_err = result_dict.get("error") or "" if isinstance(_search_err, str) and _search_err.startswith("Path not found:"): _search_nf_json = json.dumps(result_dict, ensure_ascii=False) _record_not_found("search", resolved_search_path, task_id, _search_nf_json) - return _search_nf_json if count >= 3: result_dict["_warning"] = (