From 140cb05fbc76e0cd1a54d2a8f98cbbd634a227cd Mon Sep 17 00:00:00 2001 From: Manmit Singh Date: Tue, 7 Jul 2026 22:53:26 +0530 Subject: [PATCH] fix(rtk): link managed rtk onto PATH instead of mutating the hook (#1698) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description `headroom wrap claude` / `headroom update` patched `~/.claude/hooks/rtk-rewrite.sh` after `rtk init --global --auto-patch` wrote it. `rtk` bakes the expected SHA-256 of the canonical hook into itself, so the post-write mutation trips its integrity guard — `rtk verify` reports `hook integrity check FAILED … RTK will not execute` and rtk hard-refuses to run. The patch also only absolutized the `rtk` inside the hook, but `rtk rewrite` emits a bare `rtk` on stdout at runtime that still needs PATH resolution, so the original silent-no-op (#487) was never actually fixed. This leaves the hook untouched and instead links the managed binary onto PATH. Closes #1631 ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) - [ ] New feature (non-breaking change that adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Documentation update - [ ] Performance improvement - [ ] Code refactoring (no functional changes) ## Changes Made - Removed `_patch_rtk_hook_absolute_path` (mutated the canonical hook → broke rtk's SHA-256 integrity guard). - Added `_ensure_rtk_on_path`: symlinks the Headroom-managed `rtk` into a PATH dir (prefers `~/.local/bin`) so the bare `rtk` that `rtk rewrite` emits resolves, leaving the hook byte-for-byte as `rtk init` wrote it. - No-op when a `rtk` already resolves on PATH, on Windows, or when no writable PATH dir exists; never clobbers an existing real file or foreign binary. - Rewrote the test module (`test_wrap_rtk_on_path.py`) for the new behavior. ## Testing - [x] Unit tests pass (`pytest`) - [x] Linting passes (`ruff check .`) - [ ] Type checking passes (`mypy headroom`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output ```text $ .venv/bin/python -m pytest tests/test_cli/test_wrap_rtk_on_path.py -q collected 7 items tests/test_cli/test_wrap_rtk_on_path.py ....... [100%] ============================== 7 passed in 0.25s =============================== $ .venv/bin/ruff check headroom/cli/wrap.py tests/test_cli/test_wrap_rtk_on_path.py All checks passed! ``` ## Real Behavior Proof - Environment: macOS arm64, Python 3.14, repo `.venv`, rtk hook-version 2 (matches reporter's rtk 0.28.2 setup). - Exact command / steps: `.venv/bin/python -m pytest tests/test_cli/test_wrap_rtk_on_path.py -q` — covers: no-op when rtk already on PATH, symlink created into a PATH dir when missing, `~/.local/bin` preferred + created on demand, idempotent second run, existing-file not clobbered (falls through to next dir), no-op on Windows and when no writable PATH dir exists. - Observed result: 7 passed; the canonical hook file is never written, so rtk's baked-in SHA-256 stays valid and `rtk verify` no longer fails. - Not tested: live end-to-end `rtk verify` PASS on a machine with rtk installed (no rtk binary in CI sandbox); logic mirrors the reporter's verified manual fix (symlink managed rtk into a PATH dir + untouched canonical hook). ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] I have updated the CHANGELOG.md if applicable ## Additional Notes Type checking / docs / CHANGELOG left unchecked: no public API or docs change, and CHANGELOG is release-managed. The fix is confined to `wrap.py`'s rtk setup path. --- headroom/cli/wrap.py | 105 ++++++++++------- tests/test_cli/test_wrap_rtk_hook_patch.py | 93 --------------- tests/test_cli/test_wrap_rtk_on_path.py | 127 +++++++++++++++++++++ 3 files changed, 191 insertions(+), 134 deletions(-) delete mode 100644 tests/test_cli/test_wrap_rtk_hook_patch.py create mode 100644 tests/test_cli/test_wrap_rtk_on_path.py diff --git a/headroom/cli/wrap.py b/headroom/cli/wrap.py index 7cafc164e..d73e03c72 100644 --- a/headroom/cli/wrap.py +++ b/headroom/cli/wrap.py @@ -21,7 +21,6 @@ import io import json import os import re -import shlex import shutil import signal import socket @@ -593,65 +592,89 @@ def _setup_rtk(verbose: bool = False) -> Path | None: if verbose: click.echo(" rtk hooks registered in Claude Code") try: - patched = _patch_rtk_hook_absolute_path(rtk_path) - if patched and verbose: - click.echo(" rtk hook script patched to use absolute path") + linked = _ensure_rtk_on_path(rtk_path) + if linked and verbose: + click.echo(f" rtk linked onto PATH at {linked}") except Exception as e: if verbose: - click.echo(f" rtk hook absolute-path patch skipped: {e}") + click.echo(f" rtk PATH link skipped: {e}") else: click.echo(" rtk hook registration failed — continuing without it") return rtk_path -def _patch_rtk_hook_absolute_path(rtk_path: Path, hook_script_path: Path | None = None) -> bool: - """Rewrite bare ``rtk`` invocations in the generated Claude hook script - to use the absolute path to the RTK binary Headroom manages. +def _ensure_rtk_on_path(rtk_path: Path, path_dirs: list[str] | None = None) -> Path | None: + """Make the Headroom-managed rtk resolvable as a bare ``rtk`` on PATH. - ``rtk init --global --auto-patch`` writes ``~/.claude/hooks/rtk-rewrite.sh`` - with a bare ``rtk`` command that depends on PATH lookup. Since - ``~/.headroom/bin`` (where Headroom installs rtk) is not automatically - added to PATH, that lookup fails and the hook silently does nothing. + ``rtk init --global --auto-patch`` writes ``~/.claude/hooks/rtk-rewrite.sh``, + and ``rtk rewrite`` emits a bare ``rtk`` token at runtime that the hook feeds + back to the shell — so bare ``rtk`` has to resolve on PATH regardless of the + hook's contents. Since ``~/.headroom/bin`` (where Headroom installs rtk) is + not on PATH by default, that lookup fails and compression silently never + runs (issue #487). - This rewrites bare ``rtk`` command tokens to the absolute, shell-quoted - path of the rtk binary so the hook works regardless of PATH. + An earlier fix rewrote the generated hook to hard-code rtk's absolute path. + That mutates the hook *after* ``rtk init`` bakes in its expected SHA-256, so + rtk's integrity guard rejects it (``hook integrity check FAILED … RTK will + not execute``) and only absolutizes the hook's own ``rtk`` call — not the + bare ``rtk`` that ``rtk rewrite`` emits at runtime (issue #1631). Instead, + leave the canonical hook untouched and link the managed binary into a PATH + directory so bare ``rtk`` resolves. - Idempotent: only rewrites bare ``rtk`` tokens (not paths that already - point elsewhere), and only writes the file back if content changed. + Idempotent and conservative: + * no-op if a ``rtk`` already resolves on PATH (managed or system); + * no-op on Windows (symlinks need privilege; hooks resolve differently); + * only creates/refreshes a symlink Headroom owns — never clobbers an + existing real file or foreign binary. - Returns True if the hook script was modified. + Returns the link path that was created or already correct, else ``None``. """ - if hook_script_path is None: - hook_script_path = Path.home() / ".claude" / "hooks" / "rtk-rewrite.sh" + if sys.platform == "win32": + return None - if not hook_script_path.exists(): - return False + # A bare `rtk` already resolves — the hook will find it, nothing to do. + if shutil.which("rtk"): + return None - original = _read_text(hook_script_path) + if path_dirs is None: + path_dirs = os.environ.get("PATH", "").split(os.pathsep) - # Quote the absolute path safely for POSIX shells. This matters because - # paths containing spaces or other shell-special characters (e.g. - # "/Users/Alice Smith/.headroom/bin/rtk") must be quoted, or the - # generated script will break when the shell splits on whitespace. - quoted_path = shlex.quote(str(rtk_path)) + preferred = Path.home() / ".local" / "bin" - # Replace bare `rtk` command tokens with the quoted absolute path. - # Matches `rtk` as a standalone word (preceded by start-of-line or - # whitespace/operators, followed by whitespace or end-of-line), so it - # won't touch things like "rtkfoo" or "/some/path/rtk" that are already - # absolute. - patched, count = re.subn( - r"(? Path | None: diff --git a/tests/test_cli/test_wrap_rtk_hook_patch.py b/tests/test_cli/test_wrap_rtk_hook_patch.py deleted file mode 100644 index 96a7d6845..000000000 --- a/tests/test_cli/test_wrap_rtk_hook_patch.py +++ /dev/null @@ -1,93 +0,0 @@ -"""Tests for ``_patch_rtk_hook_absolute_path``. - -``rtk init --global --auto-patch`` writes ``~/.claude/hooks/rtk-rewrite.sh`` -with a bare ``rtk`` command that depends on PATH lookup. Since -``~/.headroom/bin`` is not automatically added to PATH, that lookup fails -silently and token compression never occurs (see issue #487). - -``_patch_rtk_hook_absolute_path`` rewrites bare ``rtk`` tokens in the -generated hook script to the absolute, shell-quoted path of the rtk binary -that Headroom manages. -""" - -from __future__ import annotations - -import shlex -from pathlib import Path - -from headroom.cli.wrap import _patch_rtk_hook_absolute_path - - -def test_patches_bare_rtk_to_absolute_path(tmp_path: Path) -> None: - hook_script = tmp_path / "rtk-rewrite.sh" - hook_script.write_text( - '#!/bin/sh\nif command -v rtk >/dev/null 2>&1; then\n exec rtk rewrite "$@"\nfi\n' - ) - - rtk_path = Path("/home/user/.headroom/bin/rtk") - changed = _patch_rtk_hook_absolute_path(rtk_path, hook_script) - - content = hook_script.read_text() - quoted = shlex.quote(str(rtk_path)) - - assert changed is True - assert f"exec {quoted} rewrite" in content - - -def test_quotes_path_containing_spaces(tmp_path: Path) -> None: - """Paths with spaces (e.g. /Users/Alice Smith/...) must be shell-quoted.""" - hook_script = tmp_path / "rtk-rewrite.sh" - hook_script.write_text( - '#!/bin/sh\nif command -v rtk >/dev/null 2>&1; then\n exec rtk rewrite "$@"\nfi\n' - ) - - rtk_path = Path("/Users/Alice Smith/.headroom/bin/rtk") - changed = _patch_rtk_hook_absolute_path(rtk_path, hook_script) - - content = hook_script.read_text() - quoted = shlex.quote(str(rtk_path)) - - assert changed is True - assert f"exec {quoted} rewrite" in content - # The raw, unquoted path must never appear unescaped in the script. - assert "exec /Users/Alice Smith/.headroom/bin/rtk rewrite" not in content - - -def test_idempotent_second_run_is_noop(tmp_path: Path) -> None: - hook_script = tmp_path / "rtk-rewrite.sh" - hook_script.write_text('exec rtk rewrite "$@"\n') - - rtk_path = Path("/home/user/.headroom/bin/rtk") - - first = _patch_rtk_hook_absolute_path(rtk_path, hook_script) - content_after_first = hook_script.read_text() - - second = _patch_rtk_hook_absolute_path(rtk_path, hook_script) - content_after_second = hook_script.read_text() - - assert first is True - assert second is False - assert content_after_first == content_after_second - - -def test_missing_hook_script_is_noop(tmp_path: Path) -> None: - missing = tmp_path / "does-not-exist.sh" - rtk_path = Path("/home/user/.headroom/bin/rtk") - - changed = _patch_rtk_hook_absolute_path(rtk_path, missing) - - assert changed is False - assert not missing.exists() - - -def test_does_not_touch_words_containing_rtk(tmp_path: Path) -> None: - """Tokens like 'rtkfoo' or an already-absolute '/some/path/rtk' are left alone.""" - hook_script = tmp_path / "rtk-rewrite.sh" - original = '#!/bin/sh\necho rtkfoo\nexec /already/absolute/rtk rewrite "$@"\n' - hook_script.write_text(original) - - rtk_path = Path("/home/user/.headroom/bin/rtk") - changed = _patch_rtk_hook_absolute_path(rtk_path, hook_script) - - assert changed is False - assert hook_script.read_text() == original diff --git a/tests/test_cli/test_wrap_rtk_on_path.py b/tests/test_cli/test_wrap_rtk_on_path.py new file mode 100644 index 000000000..0782e5f21 --- /dev/null +++ b/tests/test_cli/test_wrap_rtk_on_path.py @@ -0,0 +1,127 @@ +"""Tests for ``_ensure_rtk_on_path``. + +``rtk init --global --auto-patch`` writes ``~/.claude/hooks/rtk-rewrite.sh``, +and ``rtk rewrite`` emits a bare ``rtk`` token at runtime that the hook feeds +back to the shell — so bare ``rtk`` must resolve on PATH. Since +``~/.headroom/bin`` is not on PATH by default, that lookup fails and token +compression never runs (issue #487). + +The earlier fix rewrote the generated hook to hard-code rtk's absolute path, +but that mutates the hook after ``rtk init`` bakes in its expected SHA-256, so +rtk's integrity guard rejects it (issue #1631). ``_ensure_rtk_on_path`` instead +leaves the canonical hook untouched and links the managed binary into a PATH +directory so bare ``rtk`` resolves. +""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from headroom.cli import wrap +from headroom.cli.wrap import _ensure_rtk_on_path + + +@pytest.fixture +def rtk_binary(tmp_path: Path) -> Path: + managed = tmp_path / ".headroom" / "bin" / "rtk" + managed.parent.mkdir(parents=True) + managed.write_text("#!/bin/sh\n") + managed.chmod(0o755) + return managed + + +def test_noop_when_rtk_already_on_path(rtk_binary: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(wrap.sys, "platform", "linux") + monkeypatch.setattr(wrap.shutil, "which", lambda _cmd: "/usr/bin/rtk") + + assert _ensure_rtk_on_path(rtk_binary, path_dirs=["/usr/bin"]) is None + + +def test_noop_on_windows(rtk_binary: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(wrap.sys, "platform", "win32") + + assert _ensure_rtk_on_path(rtk_binary, path_dirs=["C:\\bin"]) is None + + +def test_links_into_path_dir_when_missing( + rtk_binary: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(wrap.sys, "platform", "linux") + monkeypatch.setattr(wrap.shutil, "which", lambda _cmd: None) + bindir = tmp_path / "path-bin" + bindir.mkdir() + + link = _ensure_rtk_on_path(rtk_binary, path_dirs=[str(bindir)]) + + assert link == bindir / "rtk" + assert link.is_symlink() + assert link.resolve() == rtk_binary.resolve() + + +def test_prefers_local_bin( + rtk_binary: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(wrap.sys, "platform", "linux") + monkeypatch.setattr(wrap.shutil, "which", lambda _cmd: None) + home = tmp_path / "home" + monkeypatch.setattr(wrap.Path, "home", classmethod(lambda _cls: home)) + other = tmp_path / "other-bin" + other.mkdir() + local_bin = home / ".local" / "bin" + + # ~/.local/bin does not exist yet but is on PATH — it is created on demand + # and preferred over the other writable dir. + link = _ensure_rtk_on_path(rtk_binary, path_dirs=[str(other), str(local_bin)]) + + assert link == local_bin / "rtk" + assert link.is_symlink() + + +def test_idempotent_second_run( + rtk_binary: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(wrap.sys, "platform", "linux") + monkeypatch.setattr(wrap.shutil, "which", lambda _cmd: None) + bindir = tmp_path / "path-bin" + bindir.mkdir() + + first = _ensure_rtk_on_path(rtk_binary, path_dirs=[str(bindir)]) + second = _ensure_rtk_on_path(rtk_binary, path_dirs=[str(bindir)]) + + assert first == second == bindir / "rtk" + assert second.resolve() == rtk_binary.resolve() + + +def test_does_not_clobber_existing_file( + rtk_binary: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(wrap.sys, "platform", "linux") + monkeypatch.setattr(wrap.shutil, "which", lambda _cmd: None) + occupied = tmp_path / "occupied-bin" + occupied.mkdir() + foreign = occupied / "rtk" + foreign.write_text("#!/bin/sh\n# a different rtk\n") + fallback = tmp_path / "fallback-bin" + fallback.mkdir() + + link = _ensure_rtk_on_path(rtk_binary, path_dirs=[str(occupied), str(fallback)]) + + # The real file is left untouched; the link lands in the next writable dir. + assert foreign.read_text() == "#!/bin/sh\n# a different rtk\n" + assert not foreign.is_symlink() + assert link == fallback / "rtk" + assert link.is_symlink() + + +def test_noop_when_no_writable_path_dir( + rtk_binary: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setattr(wrap.sys, "platform", "linux") + monkeypatch.setattr(wrap.shutil, "which", lambda _cmd: None) + home = tmp_path / "home" + monkeypatch.setattr(wrap.Path, "home", classmethod(lambda _cls: home)) + + # Only a non-existent, non-preferred dir on PATH — nothing to link into. + assert _ensure_rtk_on_path(rtk_binary, path_dirs=[str(tmp_path / "ghost")]) is None