mirror of
https://github.com/headroomlabs-ai/headroom.git
synced 2026-08-27 14:17:10 -04:00
fix(rtk): link managed rtk onto PATH instead of mutating the hook (#1698)
## 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.
This commit is contained in:
Manmit Singh
GitHub
parent
681b9a8c1a
commit
140cb05fbc
3 changed files with 191 additions and 134 deletions
|
|
@ -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"(?<![\w/-])rtk(?=\s|$)",
|
||||
lambda _match: quoted_path,
|
||||
original,
|
||||
)
|
||||
# Prefer ~/.local/bin (conventionally on PATH), then any other PATH dir.
|
||||
ordered: list[Path] = []
|
||||
if str(preferred) in path_dirs:
|
||||
ordered.append(preferred)
|
||||
for entry in path_dirs:
|
||||
if not entry:
|
||||
continue
|
||||
candidate = Path(entry)
|
||||
if candidate not in ordered:
|
||||
ordered.append(candidate)
|
||||
|
||||
if count and patched != original:
|
||||
_write_text(hook_script_path, patched)
|
||||
return True
|
||||
target = rtk_path.resolve()
|
||||
|
||||
return False
|
||||
for target_dir in ordered:
|
||||
link = target_dir / "rtk"
|
||||
try:
|
||||
# Existing correct link — done.
|
||||
if link.is_symlink() and link.resolve() == target:
|
||||
return link
|
||||
# Never clobber a real file or a link pointing elsewhere.
|
||||
if link.exists() or link.is_symlink():
|
||||
continue
|
||||
# Create ~/.local/bin on demand; other PATH dirs must already exist.
|
||||
if target_dir == preferred:
|
||||
target_dir.mkdir(parents=True, exist_ok=True)
|
||||
if not target_dir.is_dir() or not os.access(target_dir, os.W_OK):
|
||||
continue
|
||||
link.symlink_to(target)
|
||||
return link
|
||||
except OSError:
|
||||
continue
|
||||
|
||||
return None
|
||||
|
||||
|
||||
def _setup_lean_ctx_agent(agent: str, verbose: bool = False) -> Path | None:
|
||||
|
|
|
|||
|
|
@ -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
|
||||
127
tests/test_cli/test_wrap_rtk_on_path.py
Normal file
127
tests/test_cli/test_wrap_rtk_on_path.py
Normal file
|
|
@ -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
|
||||
Loading…
Add table
Add a link
Reference in a new issue