From de1f4440d3d5d2f314ab2a6a1e039589f7b90574 Mon Sep 17 00:00:00 2001 From: Omer Faruk Bayrak <114455087+ofcode-dev@users.noreply.github.com> Date: Sun, 18 Oct 2026 08:26:49 +0300 Subject: [PATCH] Fix extraction races, installer reliability, and failing CI --- .github/workflows/ci.yml | 4 +- CHANGELOG.md | 21 +++++ Makefile | 4 +- README.md | 5 +- docs/DESIGN.md | 5 +- hooks/extract.py | 54 +++++++++---- install.sh | 106 +++++++++++++++++++------- tests/test_cli.sh | 4 +- tests/test_extract.py | 76 ++++++++++++++++++ tests/test_install.py | 161 +++++++++++++++++++++++++++++++++++++++ 10 files changed, 389 insertions(+), 51 deletions(-) create mode 100644 tests/test_install.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2fa8161..230e88a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -9,8 +9,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - name: Install shellcheck - run: sudo apt-get install -y shellcheck + - name: Install test tools + run: sudo apt-get install -y shellcheck tmux - run: make check macos-test: diff --git a/CHANGELOG.md b/CHANGELOG.md index ed0b683..65bbda8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,26 @@ # Changelog +## Unreleased + +### Fixed +- Restore Linux CI by explicitly quoting empty `CDPATH` assignments. +- Preserve the order of multiple text blocks within an assistant message and + skip malformed message fields without losing the rest of the answer. +- Retry missing or disappearing transcripts during settling, retaining text + already read and falling back to the Stop payload at the deadline. +- Give overlapping hook writes separate atomic temporary files; store answers + with owner-only permissions. +- Quote hook and tmux commands for repository paths containing shell syntax, + including when upgrading existing installations. +- Keep repeated installs idempotent when `set-clipboard` is off; preserve + settings permissions and unrelated hooks on install and uninstall. +- Remove tmux bindings even if tmux has been removed, avoid creating config + during uninstall, and replace the `kill-server` advice with `unbind-key y`. + +### Added +- Regression tests for extraction races and installer behavior in disposable + homes, including a real tmux configuration/parser check on Linux CI. + ## 0.2.0 — 2026-09-14 ### Changed diff --git a/Makefile b/Makefile index 4b8946b..64ecd9d 100644 --- a/Makefile +++ b/Makefile @@ -17,10 +17,10 @@ lint: echo "lint: shellcheck not installed, skipping shell lint"; \ fi @if python3 -m pyflakes --version >/dev/null 2>&1; then \ - python3 -m pyflakes hooks/extract.py tests/test_extract.py; \ + python3 -m pyflakes hooks/extract.py tests/*.py; \ else \ echo "lint: pyflakes not installed, falling back to py_compile"; \ - python3 -m py_compile hooks/extract.py; \ + python3 -m py_compile hooks/extract.py tests/*.py; \ fi check: lint test diff --git a/README.md b/README.md index 109c348..5cb814d 100644 --- a/README.md +++ b/README.md @@ -168,7 +168,10 @@ make lint # shellcheck, when installed The tests build synthetic transcripts and drive the hook the way Claude Code does (payload on stdin, `TMUX_PANE` in the environment), including the case where the transcript is still behind when the hook fires. CI runs them on -Linux and macOS. Changes are listed in [CHANGELOG.md](CHANGELOG.md). +Linux and macOS. Installer tests use disposable homes and preserve the real +user's configuration. When tmux is installed, they also verify the generated +binding with an isolated tmux server; Linux CI includes this check. +Changes are listed in [CHANGELOG.md](CHANGELOG.md). ## License diff --git a/docs/DESIGN.md b/docs/DESIGN.md index a88045f..d63b2a3 100644 --- a/docs/DESIGN.md +++ b/docs/DESIGN.md @@ -67,8 +67,9 @@ One JSON object per line. Measured shape: Four things this dictates: -1. **One content block per assistant line.** An answer is spread across many - lines, so all of them must be collected and re-joined in order. +1. **One assistant message per line.** An answer can span many lines and a + message can hold several text blocks. Collect messages in chronological + order while preserving the order of the blocks within each message. 2. **Tool results are `user` entries.** Naively stopping at the first `user` entry truncates every answer that used a tool. The boundary test is "a `user` entry containing at least one non-`tool_result` block". diff --git a/hooks/extract.py b/hooks/extract.py index 0982707..0a4b3d2 100755 --- a/hooks/extract.py +++ b/hooks/extract.py @@ -12,6 +12,7 @@ import json import os import sys +import tempfile import time VERSION = "0.2.0" @@ -55,7 +56,8 @@ def iter_lines_reverse(path, chunk_size=CHUNK): def blocks_of(entry): - content = (entry.get("message") or {}).get("content") + message = entry.get("message") + content = message.get("content") if isinstance(message, dict) else None if isinstance(content, list): return [b for b in content if isinstance(b, dict)] if isinstance(content, str): @@ -92,12 +94,17 @@ def extract_answer(lines): kind = entry.get("type") if kind == "assistant": + message_parts = [] for block in blocks_of(entry): # thinking and tool_use are not the answer. - if block.get("type") == "text": - text = (block.get("text") or "").strip() + if block.get("type") == "text" and isinstance(block.get("text"), str): + text = block["text"].strip() if text: - parts.append(text) + message_parts.append(text) + # Only entries run backwards; blocks within an entry are already + # chronological and must stay together when parts is reversed. + if message_parts: + parts.append("\n\n".join(message_parts)) elif kind == "user": if is_human_turn(entry): break @@ -147,9 +154,8 @@ def caught_up(answer, last_message): The hook payload carries that block as `last_assistant_message`; when the extracted answer ends with it, nothing more is coming. """ - if not last_message: - return False - return squash(answer).endswith(squash(last_message)) + final_text = squash(last_message or "") + return bool(final_text) and squash(answer).endswith(final_text) def settle(path, last_message, max_wait=None): @@ -165,6 +171,7 @@ def settle(path, last_message, max_wait=None): """ if max_wait is None: max_wait = SETTLE_MAX + last_message = (last_message or "").strip() deadline = time.monotonic() + max_wait answer = "" size = None @@ -175,9 +182,17 @@ def settle(path, last_message, max_wait=None): except OSError: current = -1 if current != size: - size = current quiet = 0 - answer = extract_answer(iter_lines_reverse(path)) + try: + candidate = extract_answer(iter_lines_reverse(path)) if current >= 0 else answer + except OSError: + # The file may disappear between stat and open. Retry even + # if it comes back with the same size, retaining any text + # already read so complete() can still add the final block. + size = None + else: + size = current + answer = candidate if caught_up(answer, last_message): return answer else: @@ -219,12 +234,19 @@ def log_error(exc): def write_answer(text, dest): - os.makedirs(os.path.dirname(dest), exist_ok=True) - tmp = dest + ".tmp" - with open(tmp, "w", encoding="utf-8") as fh: - fh.write(text) - # Atomic: a keypress landing mid-write must not read a half-written file. - os.replace(tmp, dest) + directory = os.path.dirname(dest) + os.makedirs(directory, mode=0o700, exist_ok=True) + # Overlapping hooks need separate temporary files. mkstemp also keeps + # answers private (0600) regardless of the user's umask. + fd, tmp = tempfile.mkstemp(prefix=os.path.basename(dest) + ".", suffix=".tmp", dir=directory) + try: + with os.fdopen(fd, "w", encoding="utf-8") as fh: + fh.write(text) + # A keypress landing mid-write must not read a half-written file. + os.replace(tmp, dest) + finally: + if os.path.exists(tmp): + os.remove(tmp) def prune(keep_days=KEEP_DAYS, now=None): @@ -278,7 +300,7 @@ def main(): transcript = payload.get("transcript_path") last_message = payload.get("last_assistant_message") - if not transcript or not os.path.exists(transcript): + if not transcript: # The payload still carries the final block; better than nothing. answer = complete("", last_message) else: diff --git a/install.sh b/install.sh index e58642b..05e1198 100755 --- a/install.sh +++ b/install.sh @@ -8,7 +8,7 @@ set -eu -REPO=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd) +REPO=$(CDPATH='' cd -- "$(dirname -- "$0")" && pwd) SETTINGS="$HOME/.claude/settings.json" TMUX_CONF="$HOME/.tmux.conf" MARK_BEGIN="# >>> cc-yank >>>" @@ -24,30 +24,37 @@ command -v python3 >/dev/null 2>&1 || { echo "cc-yank: python3 is required (stdlib only, no packages)." >&2 exit 1 } -chmod +x "$REPO/hooks/extract.py" "$REPO/bin/cc-yank" +if [ "$uninstall" -eq 0 ]; then + chmod +x "$REPO/hooks/extract.py" "$REPO/bin/cc-yank" +fi # ---------------------------------------------------- ~/.claude/settings.json # Merged with python3 rather than sed: this file holds the user's whole # configuration and a botched edit costs them more than this tool is worth. echo "settings.json" -if [ ! -f "$SETTINGS" ]; then - mkdir -p "$(dirname "$SETTINGS")" - printf '{}\n' > "$SETTINGS" - say "created $SETTINGS" -fi - python3 - "$SETTINGS" "$REPO/hooks/extract.py" "$uninstall" <<'PY' -import json, os, shutil, sys, time +import json, os, shlex, shutil, sys, time path, hook, uninstall = sys.argv[1], sys.argv[2], sys.argv[3] == "1" -command = '"%s"' % hook +command = shlex.quote(hook) +# Recognize commands written by older installers as well. +commands = {command, hook, '"%s"' % hook} + +if not os.path.isfile(path): + if uninstall: + print(" no cc-yank hook registered") + sys.exit(0) + os.makedirs(os.path.dirname(path), exist_ok=True) + with open(path, "w") as fh: + fh.write("{}\n") + print(" created %s" % path) with open(path) as fh: settings = json.load(fh) stop = settings.setdefault("hooks", {}).setdefault("Stop", []) present = any( - h.get("command") in (command, hook) + h.get("command") in commands for entry in stop for h in entry.get("hooks", []) ) @@ -57,15 +64,23 @@ if uninstall: sys.exit(0) for entry in stop: entry["hooks"] = [ - h for h in entry.get("hooks", []) if h.get("command") not in (command, hook) + h for h in entry.get("hooks", []) if h.get("command") not in commands ] settings["hooks"]["Stop"] = [e for e in stop if e.get("hooks")] if not settings["hooks"]["Stop"]: del settings["hooks"]["Stop"] action = "removed Stop hook" elif present: - print(" Stop hook already registered") - sys.exit(0) + changed = False + for entry in stop: + for h in entry.get("hooks", []): + if h.get("command") in commands and h["command"] != command: + h["command"] = command + changed = True + if not changed: + print(" Stop hook already registered") + sys.exit(0) + action = "updated Stop hook quoting" else: stop.append({"hooks": [{"type": "command", "command": command}]}) action = "registered Stop hook" @@ -82,16 +97,14 @@ tmp = path + ".tmp" with open(tmp, "w") as fh: json.dump(settings, fh, indent=2, ensure_ascii=False) fh.write("\n") +shutil.copymode(path, tmp) os.replace(tmp, path) print(" %s (backup: %s)" % (action, os.path.basename(backup))) PY # ------------------------------------------------------------- ~/.tmux.conf echo "tmux" -if ! command -v tmux >/dev/null 2>&1; then - say "tmux not installed — the keybinding is unavailable." - say "cc-yank still works as a CLI: $REPO/bin/cc-yank" -elif [ "$uninstall" -eq 1 ]; then +if [ "$uninstall" -eq 1 ]; then if [ -f "$TMUX_CONF" ] && grep -qF "$MARK_BEGIN" "$TMUX_CONF"; then tmp=$(mktemp) sed "/^$MARK_BEGIN\$/,/^$MARK_END\$/d" "$TMUX_CONF" > "$tmp" @@ -100,17 +113,55 @@ elif [ "$uninstall" -eq 1 ]; then else say "no cc-yank binding found" fi -elif [ -f "$TMUX_CONF" ] && grep -qF "$MARK_BEGIN" "$TMUX_CONF"; then - say "binding already present" +elif ! command -v tmux >/dev/null 2>&1; then + say "tmux not installed — the keybinding is unavailable." + say "cc-yank still works as a CLI: $REPO/bin/cc-yank" else - cat >> "$TMUX_CONF" <>> cc-yank >>>": + managed = True + elif line.rstrip("\n") == "# <<< cc-yank <<<": + managed = False + if managed and line.startswith("bind-key y run-shell "): + line = binding + "\n" + lines.append(line) +if lines != original: + tmp = path + ".tmp" + with open(tmp, "w") as fh: + fh.writelines(lines) + shutil.copymode(path, tmp) + os.replace(tmp, path) + print(" updated binding quoting") +else: + print(" binding already present") +PY + else + cat >> "$TMUX_CONF" </dev/null 2>&1 && [ "$uninstall" -eq 0 ]; then clip=$(tmux show-options -gv set-clipboard 2>/dev/null || echo external) if [ "$clip" = "off" ]; then - printf '%s\nset -g set-clipboard external\n%s\n' "$MARK_BEGIN" "$MARK_END" >> "$TMUX_CONF" - say "set-clipboard was off — set to external (required for OSC 52)" + if ! sed -n "/^$MARK_BEGIN\$/,/^$MARK_END\$/p" "$TMUX_CONF" | + grep -qFx 'set -g set-clipboard external'; then + printf '%s\nset -g set-clipboard external\n%s\n' "$MARK_BEGIN" "$MARK_END" >> "$TMUX_CONF" + fi + say "set-clipboard is off — external is configured; reload tmux config to activate" else say "set-clipboard = $clip (ok)" fi @@ -127,7 +181,7 @@ fi echo if [ "$uninstall" -eq 1 ]; then - echo "Uninstalled. Restart Claude Code, and run: tmux kill-server" + echo "Uninstalled. Restart Claude Code. In a running tmux, run: tmux unbind-key y" else cat <>> cc-yank >>>\n" + + "bind-key y run-shell -b '\"%s\" #{pane_id}'\n" % (repo / "bin" / "cc-yank") + + "# <<< cc-yank <<<\n") + self.install(repo=repo) + settings = json.loads(self.settings.read_text()) + command = settings["hooks"]["Stop"][0]["hooks"][0]["command"] + self.assertEqual(shlex.split(command), [str(repo / "hooks" / "extract.py")]) + result = subprocess.run(["sh", "-c", command + " --version"], + env=self.env, capture_output=True, text=True) + self.assertEqual(result.returncode, 0, result.stderr) + binding = next(line for line in self.tmux_conf.read_text().splitlines() + if line.startswith("bind-key ")) + shell_command = shlex.split(binding)[-1] + self.assertEqual(shlex.split(shell_command), [str(repo / "bin" / "cc-yank"), "#{pane_id}"]) + result = subprocess.run(["sh", "-c", shell_command.replace("#{pane_id}", "--version")], + env=self.env, capture_output=True, text=True) + self.assertEqual(result.returncode, 0, result.stderr) + + @unittest.skipUnless(shutil.which("tmux"), "tmux not installed") + def test_generated_binding_runs_through_real_tmux_parser(self): + repo = self.root / 'repo \'quotes" $cash #{pane_id}' + repo.mkdir() + shutil.copy2(REPO / "install.sh", repo) + for directory in ("hooks", "bin"): + shutil.copytree(REPO / directory, repo / directory) + output = self.root / "called.json" + (repo / "bin" / "cc-yank").write_text( + "#!/usr/bin/env python3\nimport json, sys\n" + + "with open(%r, 'w') as fh: json.dump(sys.argv[1:], fh)\n" % str(output) + ) + self.tmux_available() + self.install(repo=repo) + executable = shutil.which("tmux") + socket = str(self.root / "tmux.sock") + prefix = [executable, "-S", socket] + env = dict(self.env, TERM="xterm-256color", TMUX="", TMUX_PANE="") + + def run(*args): + result = subprocess.run([*prefix, *args], env=env, text=True, capture_output=True) + self.assertEqual(result.returncode, 0, result.stdout + result.stderr) + return result.stdout.strip() + + # Start a private server, then exercise both configuration parsing and + # run-shell's second parsing/formatting pass after a pane exists. + try: + run("-f", "/dev/null", "new-session", "-d", "-s", "test", "/bin/sh") + run("source-file", str(self.tmux_conf)) + pane = run("display-message", "-p", "#{pane_id}") + binding = next(line for line in self.tmux_conf.read_text().splitlines() + if line.startswith("bind-key ")) + command_file = self.root / "run.conf" + command_file.write_text(binding.replace("bind-key y run-shell -b ", "run-shell ", 1) + "\n") + run("source-file", str(command_file)) + self.assertEqual(json.loads(output.read_text()), [pane]) + finally: + subprocess.run([*prefix, "kill-server"], env=env, capture_output=True) + + +if __name__ == "__main__": + unittest.main()