From f0badd98993793028a879218831b207916b433d8 Mon Sep 17 00:00:00 2001 From: Safi Date: Fri, 29 May 2026 11:41:42 +0100 Subject: [PATCH] fix Windows claude-cli WinError 2 and post-commit hook silent drop on rapid commits Co-Authored-By: Claude Sonnet 4.6 --- graphify/llm.py | 21 +++- graphify/watch.py | 119 +++++++++++++++++- tests/test_claude_cli_backend.py | 78 ++++++++++++ tests/test_watch.py | 210 +++++++++++++++++++++++++++++++ uv.lock | 2 +- 5 files changed, 424 insertions(+), 6 deletions(-) diff --git a/graphify/llm.py b/graphify/llm.py index 2a993b0..4807500 100644 --- a/graphify/llm.py +++ b/graphify/llm.py @@ -490,10 +490,27 @@ def _call_claude_cli(user_message: str, max_tokens: int = 8192, *, deep_mode: bo ANTHROPIC_API_KEY. Useful for Pro/Max subscribers who don't want to provision a pay-as-you-go API key just to run graphify's semantic pass. """ + import platform import shutil import subprocess - if shutil.which("claude") is None: + # On Windows, npm installs `claude` as both `claude.ps1` and `claude.cmd` + # alongside each other. When PATHEXT lists `.PS1` before `.CMD`, + # `shutil.which("claude")` returns `claude.ps1`, which `CreateProcess` + # cannot execute directly — it raises `[WinError 2] The system cannot + # find the file specified`. `claude.cmd` IS executable by CreateProcess, + # so prefer it explicitly on Windows. See issue #1072. + claude_cmd = "claude" + if platform.system() == "Windows": + cmd_path = shutil.which("claude.cmd") + if cmd_path: + claude_cmd = cmd_path + elif shutil.which("claude") is None: + raise RuntimeError( + "Claude Code CLI not found on $PATH. Install from " + "https://claude.ai/code and run `claude` once to authenticate." + ) + elif shutil.which("claude") is None: raise RuntimeError( "Claude Code CLI not found on $PATH. Install from " "https://claude.ai/code and run `claude` once to authenticate." @@ -508,7 +525,7 @@ def _call_claude_cli(user_message: str, max_tokens: int = 8192, *, deep_mode: bo # Replacing the default prompt eliminates the conflict at the source. # Side benefit: cache-creation tokens per call drop ~19% in practice. cli_args = [ - "claude", "-p", + claude_cmd, "-p", "--output-format", "json", "--no-session-persistence", "--system-prompt", _extraction_system(deep=deep_mode), diff --git a/graphify/watch.py b/graphify/watch.py index ecc1721..8a2c752 100644 --- a/graphify/watch.py +++ b/graphify/watch.py @@ -9,6 +9,83 @@ import time from pathlib import Path _GRAPHIFY_OUT = os.environ.get("GRAPHIFY_OUT", "graphify-out") +_PENDING_FILENAME = ".pending_changes" +_PENDING_DRAIN_MAX_PASSES = 20 + + +def _queue_pending(out_dir: Path, changed_paths: list[Path]) -> None: + """Append ``changed_paths`` to ``out_dir/.pending_changes`` (one per line). + + Used by a post-commit hook process that cannot acquire ``_rebuild_lock`` + so its change set is not silently dropped (#1059). The lock-holding + process drains this file before and after its rebuild and merges the + contents with its own change set. + + Opened in append mode so concurrent writers do not clobber each other on + POSIX; each ``write()`` of a small payload is effectively atomic. A + trailing newline is always written so partial-line corruption stays + confined to the offending entry and is skipped on drain. + """ + if not changed_paths: + return + out_dir.mkdir(parents=True, exist_ok=True) + pending = out_dir / _PENDING_FILENAME + payload = "".join(f"{os.fspath(p)}\n" for p in changed_paths) + with open(pending, "a", encoding="utf-8") as fh: + fh.write(payload) + + +def _drain_pending(out_dir: Path) -> list[Path]: + """Read + unlink ``out_dir/.pending_changes`` and return deduplicated paths. + + Returns an empty list if the file does not exist. Empty/whitespace lines + are silently skipped so a partial concurrent write that left only a + fragment cannot poison the merge. + """ + pending = out_dir / _PENDING_FILENAME + if not pending.exists(): + return [] + try: + raw = pending.read_text(encoding="utf-8") + except OSError: + return [] + # Unlink BEFORE returning so a crash between read and process retains the + # data in the next caller's view via the lines we are about to return — + # i.e. losing the file after reading is fine, losing it before would be a + # bug. Use missing_ok to tolerate a racing drain on platforms where + # rename/unlink may interleave. + with contextlib.suppress(FileNotFoundError): + pending.unlink() + seen: set[str] = set() + out: list[Path] = [] + for line in raw.splitlines(): + s = line.strip() + if not s or s in seen: + continue + seen.add(s) + out.append(Path(s)) + return out + + +def _merge_changed_paths(*sources: "list[Path] | None") -> list[Path]: + """Concatenate path lists, preserving order and dropping duplicates. + + Used to combine a hook process's own ``changed_paths`` with the drained + contents of ``.pending_changes`` so the lock-holding rebuild covers + every queued commit's worth of files (#1059). + """ + seen: set[str] = set() + out: list[Path] = [] + for src in sources: + if not src: + continue + for p in src: + key = os.fspath(p) + if key in seen: + continue + seen.add(key) + out.append(p) + return out @contextlib.contextmanager @@ -320,19 +397,55 @@ def _rebuild_code( """ out = watch_path / _GRAPHIFY_OUT if acquire_lock: + # #1059: incremental (changed_paths is not None) hooks must not drop + # their change set when another rebuild is already running. Queue + # before attempting the lock so a non-blocking failure still records + # the work; the lock-holder drains the queue and merges it in. Full- + # corpus rebuilds skip the queue entirely — they already cover every + # file, so there is nothing to merge. + if changed_paths is not None and not block_on_lock: + _queue_pending(out, list(changed_paths)) with _rebuild_lock(out, blocking=block_on_lock) as got: if not got: print("[graphify watch] Rebuild already in progress for " - f"{watch_path.resolve()} - skipping.") + f"{watch_path.resolve()} - changes queued.") return False - return _rebuild_code( + # Lock acquired. Drain anything queued by earlier contenders + # (including, importantly, the paths we just queued ourselves) + # and merge with our own change set so a single rebuild covers + # everything outstanding. + if changed_paths is not None: + merged = _merge_changed_paths(changed_paths, _drain_pending(out)) + else: + # Full-corpus rebuild supersedes any queued incremental work. + _drain_pending(out) + merged = None + ok = _rebuild_code( watch_path, - changed_paths=changed_paths, + changed_paths=merged, follow_symlinks=follow_symlinks, force=force, no_cluster=no_cluster, acquire_lock=False, ) + # Late-arrival drain: another hook may have queued work while we + # were rebuilding. Loop up to _PENDING_DRAIN_MAX_PASSES times so a + # storm of commits eventually quiesces without livelocking. A full + # rebuild already saw everything, so skip this for changed_paths is None. + if merged is not None: + for _ in range(_PENDING_DRAIN_MAX_PASSES): + late = _drain_pending(out) + if not late: + break + ok = _rebuild_code( + watch_path, + changed_paths=late, + follow_symlinks=follow_symlinks, + force=force, + no_cluster=no_cluster, + acquire_lock=False, + ) and ok + return ok watch_root = watch_path.resolve() project_root = Path.cwd().resolve() if not watch_path.is_absolute() else watch_root diff --git a/tests/test_claude_cli_backend.py b/tests/test_claude_cli_backend.py index c368290..eeb6fd2 100644 --- a/tests/test_claude_cli_backend.py +++ b/tests/test_claude_cli_backend.py @@ -115,3 +115,81 @@ def test_no_session_persistence_flag_in_subprocess(fake_claude): llm._call_claude_cli("dummy", max_tokens=8192) call_args = fake_claude.call_args[0][0] assert "--no-session-persistence" in call_args + + +# ---------- Windows path resolution (#1072) ---------- + + +def test_windows_prefers_claude_cmd_over_bare_claude(monkeypatch): + """On Windows, npm installs `claude.ps1` alongside `claude.cmd`. + `CreateProcess` cannot execute `.ps1` directly (raises WinError 2), + so we must explicitly resolve `claude.cmd` and pass its full path + to subprocess.run. See issue #1072.""" + completed = MagicMock(returncode=0, stdout=json.dumps(_ENVELOPE), stderr="") + monkeypatch.setattr(llm, "_response_is_hollow", lambda raw, parsed: False) + + def fake_which(name): + # Simulate Windows PATHEXT=.PS1;.CMD ordering: bare "claude" + # resolves to the .ps1 (unexecutable by CreateProcess), while + # "claude.cmd" resolves to the .cmd shim. + return { + "claude": r"C:\Users\u\AppData\Roaming\npm\claude.ps1", + "claude.cmd": r"C:\Users\u\AppData\Roaming\npm\claude.cmd", + }.get(name) + + with patch("platform.system", return_value="Windows"), \ + patch("shutil.which", side_effect=fake_which), \ + patch("subprocess.run", return_value=completed) as run: + llm._call_claude_cli("dummy", max_tokens=8192) + + argv = run.call_args.args[0] + assert argv[0] == r"C:\Users\u\AppData\Roaming\npm\claude.cmd", ( + f"Expected full path to claude.cmd on Windows, got {argv[0]!r}" + ) + + +def test_windows_falls_back_to_bare_claude_when_cmd_missing(monkeypatch): + """If `claude.cmd` is somehow unavailable but `claude` resolves + (e.g. WSL-style install), fall back to the bare name so the + existing behaviour is preserved.""" + completed = MagicMock(returncode=0, stdout=json.dumps(_ENVELOPE), stderr="") + monkeypatch.setattr(llm, "_response_is_hollow", lambda raw, parsed: False) + + def fake_which(name): + if name == "claude.cmd": + return None + if name == "claude": + return "/usr/local/bin/claude" + return None + + with patch("platform.system", return_value="Windows"), \ + patch("shutil.which", side_effect=fake_which), \ + patch("subprocess.run", return_value=completed) as run: + llm._call_claude_cli("dummy", max_tokens=8192) + + argv = run.call_args.args[0] + assert argv[0] == "claude" + + +def test_windows_raises_when_neither_cmd_nor_bare_claude_present(): + """If neither `claude.cmd` nor `claude` are on PATH on Windows, + raise the standard not-found error.""" + with patch("platform.system", return_value="Windows"), \ + patch("shutil.which", return_value=None): + with pytest.raises(RuntimeError, match="Claude Code CLI not found"): + llm._call_claude_cli("dummy", max_tokens=8192) + + +def test_non_windows_uses_bare_claude(monkeypatch): + """On non-Windows platforms, behaviour is unchanged: bare `claude` + is passed to subprocess.run (shell resolves it via PATH).""" + completed = MagicMock(returncode=0, stdout=json.dumps(_ENVELOPE), stderr="") + monkeypatch.setattr(llm, "_response_is_hollow", lambda raw, parsed: False) + + with patch("platform.system", return_value="Linux"), \ + patch("shutil.which", return_value="/usr/local/bin/claude"), \ + patch("subprocess.run", return_value=completed) as run: + llm._call_claude_cli("dummy", max_tokens=8192) + + argv = run.call_args.args[0] + assert argv[0] == "claude" diff --git a/tests/test_watch.py b/tests/test_watch.py index db1e5ce..80dff31 100644 --- a/tests/test_watch.py +++ b/tests/test_watch.py @@ -484,3 +484,213 @@ def test_rebuild_code_prunes_deleted_file_nodes(tmp_path): assert "keep.py" in after_sources, "untouched file's nodes should survive" finally: os.chdir(cwd) + + +# --- #1059: pending-changes queue prevents commit drops under lock contention --- + + +def test_queue_and_drain_pending_round_trip(tmp_path): + """_queue_pending writes one path per line; _drain_pending reads + unlinks + and returns the same set of paths.""" + from graphify.watch import _queue_pending, _drain_pending, _PENDING_FILENAME + + out = tmp_path / "graphify-out" + paths = [Path("a.py"), Path("sub/b.py"), Path("c.md")] + _queue_pending(out, paths) + + pending_file = out / _PENDING_FILENAME + assert pending_file.exists() + # Each path written on its own line. + assert pending_file.read_text(encoding="utf-8").splitlines() == [ + "a.py", "sub/b.py", "c.md", + ] + + drained = _drain_pending(out) + assert drained == paths + # Drain unlinks so subsequent callers see an empty queue. + assert not pending_file.exists() + assert _drain_pending(out) == [] + + +def test_drain_pending_dedupes_and_skips_blank_lines(tmp_path): + """Repeated appends across concurrent contenders must dedupe; partial + writes leaving blank lines must not poison the merge.""" + from graphify.watch import _queue_pending, _drain_pending + + out = tmp_path / "graphify-out" + _queue_pending(out, [Path("a.py"), Path("b.py")]) + _queue_pending(out, [Path("b.py"), Path("c.py")]) + # Simulate a torn write leaving an empty line. + with open(out / ".pending_changes", "a", encoding="utf-8") as fh: + fh.write("\n \n") + + drained = _drain_pending(out) + assert drained == [Path("a.py"), Path("b.py"), Path("c.py")] + + +def test_queue_pending_noop_on_empty_list(tmp_path): + """Empty change set must not create an empty .pending_changes file.""" + from graphify.watch import _queue_pending, _PENDING_FILENAME + + out = tmp_path / "graphify-out" + _queue_pending(out, []) + assert not (out / _PENDING_FILENAME).exists() + + +@pytest.mark.skipif(sys.platform == "win32", reason="fcntl-only (POSIX)") +def test_rebuild_code_queues_on_lock_contention(tmp_path, monkeypatch, capsys): + """#1059: when the rebuild lock is held, an incremental hook must queue + its changed_paths to .pending_changes and print 'queued' instead of + silently dropping the change set.""" + from graphify.watch import _rebuild_code, _rebuild_lock, _PENDING_FILENAME + + out = tmp_path / "graphify-out" + out.mkdir() + + # Hold the lock so the next non-blocking attempt fails. Use a real + # _rebuild_lock context manager in this same process — flock on the same + # file descriptor would otherwise be re-entrant on Linux, so we open + # the file ourselves via the lock helper. + with _rebuild_lock(out, blocking=False) as outer_got: + assert outer_got is True + + ok = _rebuild_code( + tmp_path, + changed_paths=[Path("a.py"), Path("b.py")], + ) + assert ok is False + + # Output should say "queued", not "skipping". + captured = capsys.readouterr().out + assert "queued" in captured.lower() + assert "skipping" not in captured.lower() + + # And the paths must have been written to the pending file so the + # eventual lock-holder can drain them. + pending = out / _PENDING_FILENAME + assert pending.exists() + assert pending.read_text(encoding="utf-8").splitlines() == ["a.py", "b.py"] + + +@pytest.mark.skipif(sys.platform == "win32", reason="fcntl-only (POSIX)") +def test_rebuild_code_merges_pending_on_acquire(tmp_path, monkeypatch): + """#1059: the process that acquires the lock must drain .pending_changes + and pass the merged change set to the inner rebuild call.""" + from graphify import watch as watch_mod + + out = tmp_path / "graphify-out" + out.mkdir() + # Pre-populate the queue as if an earlier contender had dropped its paths. + watch_mod._queue_pending(out, [Path("queued1.py"), Path("queued2.py")]) + + # Snapshot the original BEFORE monkeypatching so we can drive the outer + # dispatch path while the inner recursive call resolves to our spy. + orig_rebuild = watch_mod._rebuild_code + inner_calls: list[list[str]] = [] + + def recording_inner(watch_path, **kwargs): + if kwargs.get("acquire_lock") is False: + paths = kwargs.get("changed_paths") or [] + inner_calls.append([p.as_posix() for p in paths]) + return True + + monkeypatch.setattr(watch_mod, "_rebuild_code", recording_inner) + + ok = orig_rebuild( + tmp_path, + changed_paths=[Path("own.py"), Path("queued1.py")], + ) + assert ok is True + + # The first inner call must have received the merged + deduped set: + # own.py first (caller's order preserved), then drained queued1/queued2, + # with queued1.py deduped against own's prior occurrence. + assert inner_calls, "inner _rebuild_code should have been called" + assert inner_calls[0] == ["own.py", "queued1.py", "queued2.py"] + + # And .pending_changes was drained. + assert not (out / watch_mod._PENDING_FILENAME).exists() + + +@pytest.mark.skipif(sys.platform == "win32", reason="fcntl-only (POSIX)") +def test_rebuild_code_drains_late_arrivals(tmp_path, monkeypatch): + """#1059: after the primary rebuild, the lock-holder must loop and drain + any paths queued by hooks that arrived mid-rebuild.""" + from graphify import watch as watch_mod + from graphify.watch import _rebuild_code as orig_rebuild + + out = tmp_path / "graphify-out" + out.mkdir() + + inner_calls: list[list[str]] = [] + call_state = {"i": 0} + + def fake_inner(watch_path, **kwargs): + if kwargs.get("acquire_lock") is False: + paths = [p.as_posix() for p in (kwargs.get("changed_paths") or [])] + inner_calls.append(paths) + # Simulate a late-arriving hook that queues during the FIRST + # inner rebuild only. The outer drain loop must see it. + call_state["i"] += 1 + if call_state["i"] == 1: + watch_mod._queue_pending(out, [Path("late.py")]) + return True + + monkeypatch.setattr(watch_mod, "_rebuild_code", fake_inner) + + ok = orig_rebuild(tmp_path, changed_paths=[Path("own.py")]) + assert ok is True + + # First inner call covers our own change set; second is the late-drain + # pass that picks up "late.py". + assert len(inner_calls) >= 2 + assert inner_calls[0] == ["own.py"] + assert inner_calls[1] == ["late.py"] + # And the queue is now empty (no further late drains). + assert not (out / watch_mod._PENDING_FILENAME).exists() + + +def test_rebuild_code_full_corpus_skips_pending_queue(tmp_path, monkeypatch): + """#1059: changed_paths=None means a full-corpus rebuild — the queue + must not be touched on the failure path because there is nothing + incremental to preserve.""" + from graphify import watch as watch_mod + from graphify.watch import _rebuild_code as orig_rebuild + + out = tmp_path / "graphify-out" + out.mkdir() + + # Pre-existing queued paths from an earlier incremental hook. + watch_mod._queue_pending(out, [Path("earlier.py")]) + + # Force the inner call to record what it saw. + seen: list = [] + + def fake_inner(watch_path, **kwargs): + if kwargs.get("acquire_lock") is False: + seen.append(kwargs.get("changed_paths")) + return True + + monkeypatch.setattr(watch_mod, "_rebuild_code", fake_inner) + + ok = orig_rebuild(tmp_path, changed_paths=None) + assert ok is True + # Full-corpus rebuild passes None to the inner call (does not merge in + # the queued paths — a full rebuild already covers them). + assert seen == [None] + # The queue still gets drained on entry so stale entries don't leak, + # but no late-arrival loop runs for the full-corpus path. + assert not (out / watch_mod._PENDING_FILENAME).exists() + + +def test_merge_changed_paths_dedupes_in_order(): + """_merge_changed_paths preserves first-seen order and drops dupes.""" + from graphify.watch import _merge_changed_paths + + merged = _merge_changed_paths( + [Path("a.py"), Path("b.py")], + None, + [Path("b.py"), Path("c.py")], + [Path("a.py")], + ) + assert [p.as_posix() for p in merged] == ["a.py", "b.py", "c.py"] diff --git a/uv.lock b/uv.lock index 706979f..1a78eb9 100644 --- a/uv.lock +++ b/uv.lock @@ -1109,7 +1109,7 @@ wheels = [ [[package]] name = "graphifyy" -version = "0.8.23" +version = "0.8.24" source = { editable = "." } dependencies = [ { name = "datasketch" },