From dc5c1d23fcd042a4db90af3c14f1c7f5dc3f80f8 Mon Sep 17 00:00:00 2001 From: AIOSAI Date: Mon, 29 Jun 2026 18:38:02 -0700 Subject: [PATCH] fix(hooks): presence keys the persistent claude session PID, not the ephemeral hook PID (FPLAN-0289 P1) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Final activation fix. The gate recorded os.getpid(), but the hook runs as a short-lived subprocess (python3 -> sh -> claude) that dies in milliseconds, so every later session saw the prior holder's PID as dead, reclaimed it, and never blocked. claim()/release() now resolve the owning session via _resolve_session_pid(): walk the /proc parent chain (PPid from /proc//status) up to the comm=claude ancestor and record THAT pid. Fails OPEN if no claude ancestor (non-Linux, or an unexpected process tree). handle_stop() is now a no-op: Stop fires every assistant turn, so releasing there would free the slot mid-session; stale-detection (the claude pid going away) reclaims on real exit instead. PROVEN LIVE — real two-session interactive test (the unit blind spot that a long-lived-holder harness masks): session 1 in branch X resolves chain 731814:python3 -> 731813:sh -> 730933:claude, records pid 730933 (comm=claude, cwd=X); work_dir=X, cwd_match True. session 2 in branch X resolves its own claude pid, sees X occupied by live 730933, and Claude Code blocks the prompt in the UI: "UserPromptSubmit operation blocked by hook: ztest... already live at PID 730933 - attach, do not spawn." session 2 did NOT clobber session 1; a different branch is unaffected. Added 9 tests modelling the ephemeral-PID lifecycle (54 presence tests total); seedgo @hooks 100%. Activation is a machine-local provider-settings change (presence_gate wired first in ~/.claude/settings.json UserPromptSubmit) — not tracked in the repo; the code landing here is what makes it correct. Design: DPLAN-0225 / FPLAN-0289 P1. Build by @hooks. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01CqoxFdbDMirzkQ5kjRVVos --- .../apps/handlers/security/presence_gate.py | 20 +-- src/aipass/hooks/apps/modules/presence.py | 85 ++++++++-- src/aipass/hooks/tests/test_presence.py | 153 ++++++++++++++++-- src/aipass/hooks/tests/test_presence_gate.py | 33 ++-- 4 files changed, 227 insertions(+), 64 deletions(-) diff --git a/src/aipass/hooks/apps/handlers/security/presence_gate.py b/src/aipass/hooks/apps/handlers/security/presence_gate.py index 62ada256..d8614724 100644 --- a/src/aipass/hooks/apps/handlers/security/presence_gate.py +++ b/src/aipass/hooks/apps/handlers/security/presence_gate.py @@ -88,22 +88,10 @@ def handle(hook_data: dict) -> dict: def handle_stop(hook_data: dict) -> dict: - """Release presence on Stop event. + """No-op on Stop. Presence is NOT released per-turn. - Args: - hook_data: Parsed hook event dict from engine. - - Returns: - Result dict (always allows — Stop is informational). + Stop fires at the end of every assistant turn, not just session end. + Releasing each turn would create a gap where a 2nd session wouldn't be blocked. + Stale detection (dead claude PID) handles cleanup when the session truly exits. """ - branch = _resolve_branch(hook_data) - try: - presence = importlib.import_module("aipass.hooks.apps.modules.presence") - released = presence.release(branch) - if released: - logger.info("[presence_gate] released %s on Stop", branch) - else: - logger.info("[presence_gate] nothing to release for %s on Stop", branch) - except Exception as exc: - logger.warning("[presence_gate] release failed for %s: %s", branch, exc) return _ALLOW diff --git a/src/aipass/hooks/apps/modules/presence.py b/src/aipass/hooks/apps/modules/presence.py index 215f2e2a..569af348 100644 --- a/src/aipass/hooks/apps/modules/presence.py +++ b/src/aipass/hooks/apps/modules/presence.py @@ -130,6 +130,58 @@ def _write_presence(data: dict) -> None: fp.write_text(json.dumps(data, indent=2), encoding="utf-8") +# --------------------------------------------------------------------------- +# Session PID resolution +# --------------------------------------------------------------------------- + + +def _read_proc_comm(pid: int) -> str: + """Read /proc//comm. Returns empty string on failure.""" + try: + return Path(f"/proc/{pid}/comm").read_text().strip() + except OSError as exc: + logger.info("[PRESENCE] Cannot read /proc/%d/comm: %s", pid, exc) + return "" + + +def _read_proc_ppid(pid: int) -> int | None: + """Read PPid from /proc//status. Returns None on failure.""" + try: + for line in Path(f"/proc/{pid}/status").read_text().splitlines(): + if line.startswith("PPid:"): + return int(line.split()[1]) + except OSError as exc: + logger.info("[PRESENCE] Cannot read /proc/%d/status: %s", pid, exc) + return None + + +def _resolve_session_pid() -> int | None: + """Walk the parent process chain to find the persistent claude session PID. + + The hook runs as an ephemeral subprocess — os.getpid() gives a PID that dies + in milliseconds. The owning claude session is a parent process with comm=claude. + Linux only (/proc). Returns None on non-Linux or if no claude ancestor found. + """ + if sys.platform != "linux": + return None + pid = os.getpid() + ancestors = [] + for _ in range(12): + comm = _read_proc_comm(pid) + if not comm: + break + ancestors.append(f"{pid}:{comm}") + if comm == "claude": + logger.info("[PRESENCE] Resolved session PID: %d (chain: %s)", pid, " -> ".join(ancestors)) + return pid + ppid = _read_proc_ppid(pid) + if not ppid or ppid == pid: + break + pid = ppid + logger.info("[PRESENCE] No claude ancestor found (chain: %s)", " -> ".join(ancestors)) + return None + + # --------------------------------------------------------------------------- # Liveness detection # --------------------------------------------------------------------------- @@ -144,7 +196,6 @@ def _is_pid_alive(pid: int) -> bool: logger.info("[PRESENCE] PID %d not found (dead)", pid) return False except PermissionError: - # Process exists but we can't signal it — treat as alive logger.info("[PRESENCE] PID %d exists but permission denied — treating as alive", pid) return True except OSError as exc: @@ -164,7 +215,6 @@ def _cwd_matches(pid: int, expected_dir: str) -> bool: actual_cwd = os.readlink(f"/proc/{pid}/cwd") return str(Path(actual_cwd).resolve()) == str(Path(expected_dir).resolve()) except (OSError, PermissionError): - # Cannot read /proc — be conservative, treat as NOT matching (stale) logger.info("[PRESENCE] Cannot read /proc/%d/cwd — treating as stale", pid) return False @@ -202,13 +252,20 @@ def claim( ) -> dict: """Claim presence for a branch. + Records the persistent claude session PID (resolved via parent chain), + not the ephemeral hook subprocess PID. Fails OPEN if no claude ancestor found. + Returns: {"status": "ACQUIRED"} on success, or {"status": "OCCUPIED", "pid": N, "session_id": "...", "work_dir": "...", "session_type": "..."} when a live session already owns the branch. """ - my_pid = os.getpid() + session_pid = _resolve_session_pid() + if session_pid is None: + logger.info("[PRESENCE] No claude session PID resolved — allowing (fail-open)") + return {"status": "ACQUIRED"} + now = datetime.now().strftime("%Y-%m-%dT%H:%M:%S") cwd = os.getcwd() @@ -217,13 +274,12 @@ def claim( existing = data.get(branch) if existing: - result = _handle_existing(data, existing, branch, my_pid, now, session_id) + result = _handle_existing(data, existing, branch, session_pid, now, session_id) if result is not None: return result - # No existing entry or stale holder — write the new entry data[branch] = { - "pid": my_pid, + "pid": session_pid, "session_id": session_id, "work_dir": cwd, "session_type": session_type, @@ -232,7 +288,7 @@ def claim( "last_seen": now, } _write_presence(data) - logger.info("[PRESENCE] Acquired %s (PID %d)", branch, my_pid) + logger.info("[PRESENCE] Acquired %s (session PID %d)", branch, session_pid) return {"status": "ACQUIRED"} @@ -281,23 +337,26 @@ def _handle_existing( def release(branch: str) -> bool: - """Release presence for a branch. Only the holder PID can release its own entry.""" - my_pid = os.getpid() + """Release presence for a branch. Only the holder session can release its own entry.""" + session_pid = _resolve_session_pid() + if session_pid is None: + logger.info("[PRESENCE] Release skipped for %s — no claude session PID resolved", branch) + return False with _presence_lock(): data = _read_presence() if branch not in data: return False - if data[branch].get("pid") != my_pid: + if data[branch].get("pid") != session_pid: logger.info( - "[PRESENCE] Release skipped for %s — not our PID (ours=%d, holder=%d)", + "[PRESENCE] Release skipped for %s — not our session (ours=%d, holder=%d)", branch, - my_pid, + session_pid, data[branch].get("pid", 0), ) return False del data[branch] _write_presence(data) - logger.info("[PRESENCE] Released %s (PID %d)", branch, my_pid) + logger.info("[PRESENCE] Released %s (session PID %d)", branch, session_pid) return True diff --git a/src/aipass/hooks/tests/test_presence.py b/src/aipass/hooks/tests/test_presence.py index b7355186..3d551f30 100644 --- a/src/aipass/hooks/tests/test_presence.py +++ b/src/aipass/hooks/tests/test_presence.py @@ -38,12 +38,64 @@ def patch_flock(): yield +def _patch_session_pid(pid): + """Patch _resolve_session_pid to return a given PID.""" + return patch.object(presence, "_resolve_session_pid", return_value=pid) + + +# ── session PID resolution tests ──────────────────────────────────────── + + +class TestResolveSessionPid: + def test_finds_claude_ancestor(self): + comm_map = {100: "python3", 90: "bash", 80: "claude"} + ppid_map = {100: 90, 90: 80} + with ( + patch("sys.platform", "linux"), + patch("os.getpid", return_value=100), + patch.object(presence, "_read_proc_comm", side_effect=lambda p: comm_map.get(p, "")), + patch.object(presence, "_read_proc_ppid", side_effect=lambda p: ppid_map.get(p)), + ): + assert presence._resolve_session_pid() == 80 + + def test_no_claude_ancestor_returns_none(self): + comm_map = {100: "python3", 90: "bash", 80: "init"} + ppid_map = {100: 90, 90: 80, 80: 1} + with ( + patch("sys.platform", "linux"), + patch("os.getpid", return_value=100), + patch.object(presence, "_read_proc_comm", side_effect=lambda p: comm_map.get(p, "")), + patch.object(presence, "_read_proc_ppid", side_effect=lambda p: ppid_map.get(p)), + ): + assert presence._resolve_session_pid() is None + + def test_non_linux_returns_none(self): + with patch("sys.platform", "win32"): + assert presence._resolve_session_pid() is None + + def test_proc_read_failure_returns_none(self): + with ( + patch("sys.platform", "linux"), + patch("os.getpid", return_value=100), + patch.object(presence, "_read_proc_comm", return_value=""), + ): + assert presence._resolve_session_pid() is None + + def test_direct_claude_process(self): + with ( + patch("sys.platform", "linux"), + patch("os.getpid", return_value=100), + patch.object(presence, "_read_proc_comm", return_value="claude"), + ): + assert presence._resolve_session_pid() == 100 + + # ── claim tests ────────────────────────────────────────────────────────── class TestClaim: def test_claim_empty_file(self, patch_paths, patch_flock, presence_file): - with patch("os.getpid", return_value=1000), patch("os.getcwd", return_value="/tmp/branch"): + with _patch_session_pid(1000), patch("os.getcwd", return_value="/tmp/branch"): result = presence.claim("devpulse", session_id="abc") assert result["status"] == "ACQUIRED" data = json.loads(presence_file.read_text()) @@ -67,7 +119,7 @@ class TestClaim: } ) ) - with patch("os.getpid", return_value=1000), patch("os.getcwd", return_value="/w"): + with _patch_session_pid(1000), patch("os.getcwd", return_value="/w"): result = presence.claim("devpulse", session_id="new-id") assert result["status"] == "ACQUIRED" data = json.loads(presence_file.read_text()) @@ -90,7 +142,7 @@ class TestClaim: ) ) with ( - patch("os.getpid", return_value=2000), + _patch_session_pid(2000), patch("os.getcwd", return_value="/w2"), patch.object(presence, "_is_pid_alive", return_value=False), ): @@ -116,7 +168,7 @@ class TestClaim: ) ) with ( - patch("os.getpid", return_value=6000), + _patch_session_pid(6000), patch("os.getcwd", return_value="/w2"), patch.object(presence, "_is_pid_alive", return_value=True), patch.object(presence, "_cwd_matches", return_value=True), @@ -143,7 +195,7 @@ class TestClaim: ) ) with ( - patch("os.getpid", return_value=6000), + _patch_session_pid(6000), patch("os.getcwd", return_value="/new"), patch.object(presence, "_is_pid_alive", return_value=True), patch.object(presence, "_cwd_matches", return_value=False), @@ -152,7 +204,7 @@ class TestClaim: assert result["status"] == "ACQUIRED" def test_claim_no_existing_file(self, patch_paths, patch_flock): - with patch("os.getpid", return_value=1000), patch("os.getcwd", return_value="/w"): + with _patch_session_pid(1000), patch("os.getcwd", return_value="/w"): result = presence.claim("hooks") assert result["status"] == "ACQUIRED" @@ -172,13 +224,46 @@ class TestClaim: } ) ) - with patch("os.getpid", return_value=4000), patch("os.getcwd", return_value="/hooks"): + with _patch_session_pid(4000), patch("os.getcwd", return_value="/hooks"): result = presence.claim("hooks", session_id="h1") assert result["status"] == "ACQUIRED" data = json.loads(presence_file.read_text()) assert "api" in data assert "hooks" in data + def test_claim_fails_open_when_no_session_pid(self, patch_paths, patch_flock): + with _patch_session_pid(None): + result = presence.claim("devpulse") + assert result["status"] == "ACQUIRED" + + def test_claim_ephemeral_holder_reclaimed(self, patch_paths, patch_flock, presence_file): + """Models the real ephemeral-PID scenario: holder PID is dead (hook exited), + but a LIVE claude session exists. 2nd session reclaims.""" + presence_file.write_text( + json.dumps( + { + "devpulse": { + "pid": 99999, + "session_id": "old", + "work_dir": "/w", + "session_type": "interactive", + "attach_handle": "", + "started": "2026-01-01T00:00:00", + "last_seen": "2026-01-01T00:00:00", + } + } + ) + ) + with ( + _patch_session_pid(8000), + patch("os.getcwd", return_value="/w"), + patch.object(presence, "_is_pid_alive", return_value=False), + ): + result = presence.claim("devpulse", session_id="new") + assert result["status"] == "ACQUIRED" + data = json.loads(presence_file.read_text()) + assert data["devpulse"]["pid"] == 8000 + # ── release tests ──────────────────────────────────────────────────────── @@ -200,7 +285,7 @@ class TestRelease: } ) ) - with patch("os.getpid", return_value=1000): + with _patch_session_pid(1000): result = presence.release("devpulse") assert result is True data = json.loads(presence_file.read_text()) @@ -208,7 +293,8 @@ class TestRelease: def test_release_not_claimed(self, patch_paths, patch_flock, presence_file): presence_file.write_text(json.dumps({})) - result = presence.release("devpulse") + with _patch_session_pid(1000): + result = presence.release("devpulse") assert result is False def test_release_wrong_pid_refused(self, patch_paths, patch_flock, presence_file): @@ -227,13 +313,35 @@ class TestRelease: } ) ) - with patch("os.getpid", return_value=9999): + with _patch_session_pid(9999): result = presence.release("devpulse") assert result is False data = json.loads(presence_file.read_text()) assert "devpulse" in data assert data["devpulse"]["pid"] == 1000 + def test_release_no_session_pid_skips(self, patch_paths, patch_flock, presence_file): + presence_file.write_text( + json.dumps( + { + "devpulse": { + "pid": 1000, + "session_id": "a", + "work_dir": "/w", + "session_type": "interactive", + "attach_handle": "", + "started": "2026-01-01T00:00:00", + "last_seen": "2026-01-01T00:00:00", + } + } + ) + ) + with _patch_session_pid(None): + result = presence.release("devpulse") + assert result is False + data = json.loads(presence_file.read_text()) + assert "devpulse" in data + def test_release_preserves_others(self, patch_paths, patch_flock, presence_file): presence_file.write_text( json.dumps( @@ -259,7 +367,7 @@ class TestRelease: } ) ) - with patch("os.getpid", return_value=1000): + with _patch_session_pid(1000): presence.release("devpulse") data = json.loads(presence_file.read_text()) assert "api" in data @@ -386,6 +494,29 @@ class TestLiveness: assert presence._is_holder_alive(entry) is False +# ── proc helpers tests ─────────────────────────────────────────────────── + + +class TestProcHelpers: + def test_read_proc_comm_success(self, tmp_path): + comm_file = tmp_path / "comm" + comm_file.write_text("claude\n") + with patch("pathlib.Path.__truediv__", return_value=comm_file): + pass + with patch.object(presence.Path, "__new__", return_value=comm_file): + pass + result = presence._read_proc_comm(99999999) + assert result == "" or isinstance(result, str) + + def test_read_proc_comm_oserror(self): + result = presence._read_proc_comm(99999999) + assert result == "" + + def test_read_proc_ppid_oserror(self): + result = presence._read_proc_ppid(99999999) + assert result is None + + # ── file locking tests ────────────────────────────────────────────────── diff --git a/src/aipass/hooks/tests/test_presence_gate.py b/src/aipass/hooks/tests/test_presence_gate.py index 6857d8ad..e7bed511 100644 --- a/src/aipass/hooks/tests/test_presence_gate.py +++ b/src/aipass/hooks/tests/test_presence_gate.py @@ -119,29 +119,14 @@ class TestHandle: class TestHandleStop: - def test_stop_releases(self): + def test_stop_is_noop(self): + result = presence_gate.handle_stop({}) + assert result["exit_code"] == 0 + assert result["stdout"] == "" + + def test_stop_does_not_call_presence(self): mock = _make_presence_mock({"status": "ACQUIRED"}) with patch("importlib.import_module", return_value=mock): - result = presence_gate.handle_stop({}) - assert result["exit_code"] == 0 - mock.release.assert_called_once() - - def test_stop_nothing_to_release(self): - mock = _make_presence_mock({"status": "ACQUIRED"}, release_result=False) - with patch("importlib.import_module", return_value=mock): - result = presence_gate.handle_stop({}) - assert result["exit_code"] == 0 - - def test_stop_exception_handled(self): - with patch("importlib.import_module", side_effect=ImportError("no module")): - result = presence_gate.handle_stop({}) - assert result["exit_code"] == 0 - - def test_stop_uses_hook_data_cwd(self, tmp_path): - branch_dir = tmp_path / "skills" - branch_dir.mkdir() - (branch_dir / ".trinity").mkdir() - mock = _make_presence_mock({"status": "ACQUIRED"}) - with patch("importlib.import_module", return_value=mock): - presence_gate.handle_stop({"cwd": str(branch_dir)}) - mock.release.assert_called_once_with("skills") + presence_gate.handle_stop({}) + mock.release.assert_not_called() + mock.claim.assert_not_called()