Act on four reviews: state machine, resource bounds, teardown
Four agents reviewed this in parallel -- correctness, GNOME integration, edge cases, security. Everything below was reproduced before being fixed; several findings that survived the first reading did not survive a probe and are not here. State machine, the two that mattered most. A pending permission prompt was erased by any subagent bookkeeping event: SubagentStop or the next PreToolUse recomputed the state from scratch, so a session sat at "working" with a dialog open and nothing ever raised it again. Blocked now outlives everything except evidence the question was answered. Separately, the stale-event guard refused whole events, including the subagent counter's increments and decrements -- but those are deltas and deltas commute, so a "+1" that lost a timestamp race left the count short and the batch freed the session while a subagent was still running. The guard now gates the state decision only. Corrupt or hostile state files could wedge the panel or take the hook down for every session: a non-numeric pid raised inside sweep_dead before the hook wrote its own file, so one bad byte stopped new sessions appearing at all. Numbers read back from disk are coerced, one unreadable file no longer aborts the sweep, and a stored timestamp far in the future -- corruption, or a clock stepped backwards by NTP -- no longer refuses every later event forever. Resource bounds, all in the compositor process. A state file was read whole with no size check: a symlink to /dev/zero took a test process past 4 GB in three seconds, which in gnome-shell ends the session. Sizes are checked before the read, sessions and zellij subprocesses are capped, labels ellipsize, and cwd and messages are truncated at the hook. Teardown hung off an overridden destroy(), which only runs when JS calls it. An actor destroyed any other way -- another extension rebuilding the panel boxes -- left the timer and the file monitor running against a disposed actor. It is a destroy signal now. The zellij child is killed rather than merely abandoned. The glyph was pinned to physical pixels and rendered half-size on HiDPI; size comes from the stylesheet, and the foreground colour is normalised by inspection rather than assuming which colour struct the shell hands back. Chip labels: non-Latin names all collapsed to "?", because the split treated every Cyrillic letter as a separator -- notable for a tool whose own README is Russian. Seniority also ranked by time-in-state rather than session age, so after a shell restart the older session could take the digit; the hook now records when the session began. zellij: a dump ends with new_tab_template and swap_tiled_layout blocks whose tab lines carry no name, and their panes were being attached to the last real tab -- which then answered for every unmatched directory, confidently and wrongly. install.py no longer widens the mode of a settings.json someone narrowed to 0600, no longer overwrites the pristine .bak on a second run, no longer replaces a symlink out of a dotfiles repository with a regular file, and quotes the hook path. The debug log is capped and README now says plainly that it records prompts verbatim. Not fixed, deliberately: the panel does push the clock about 70 px left with three labelled chips, which is inherent to putting them in the centre box; two different projects abbreviating alike still read as one project with a digit; GNOME 48 remains unverified for the colour struct and for St.BoxLayout's vertical property, both flagged rather than guessed at.
This commit is contained in:
+135
-34
@@ -21,6 +21,7 @@ Design notes that are easy to get wrong:
|
||||
* No stdlib import beyond what is needed: this runs once per tool call.
|
||||
"""
|
||||
|
||||
import errno
|
||||
import fcntl
|
||||
import json
|
||||
import os
|
||||
@@ -52,6 +53,36 @@ NOTIFICATION_STATES = {
|
||||
# inflate the count with TaskCreate, TaskUpdate and the like.
|
||||
AGENT_TOOLS = {"Agent", "Task"}
|
||||
|
||||
# Events that mean the question has been dealt with. Anything else leaves a
|
||||
# "blocked" session blocked: a subagent finishing, or the next tool starting,
|
||||
# says nothing about the prompt still sitting on your screen, and clearing it
|
||||
# would hide the one state this indicator exists to surface.
|
||||
BLOCK_CLEARING = {"PostToolUse", "UserPromptSubmit", "Stop", "SessionStart"}
|
||||
|
||||
# Nothing here is displayed at full length, and both ends of the pipe have to
|
||||
# survive a hostile or merely absurd value: a multi-megabyte cwd would be copied
|
||||
# into the state file, read back by the compositor and handed to Pango.
|
||||
MAX_TEXT = 512
|
||||
# A stored timestamp further ahead than this is not a concurrent write, it is
|
||||
# corruption -- and left alone it would refuse every later event forever,
|
||||
# freezing the session's displayed state for good.
|
||||
FUTURE_SLACK = 60 # seconds
|
||||
|
||||
|
||||
def number(value, default=0):
|
||||
"""Coerce a value read back from disk. Files are ordinary user-writable
|
||||
JSON: one corrupt field must not take the hook down for every session."""
|
||||
try:
|
||||
n = float(value)
|
||||
except (TypeError, ValueError):
|
||||
return default
|
||||
return default if n != n or n in (float("inf"), float("-inf")) else n
|
||||
|
||||
|
||||
def clip(value):
|
||||
text = value if isinstance(value, str) else ""
|
||||
return text[:MAX_TEXT]
|
||||
|
||||
EVENT_STATES = {
|
||||
# A session that has just opened is waiting for your first prompt, which is
|
||||
# the same thing as one that has finished a turn: the input line is free
|
||||
@@ -74,10 +105,16 @@ def debug_log(event):
|
||||
environment, which cannot be changed without restarting the session.
|
||||
"""
|
||||
marker = os.path.join(STATE_DIR, "debug")
|
||||
if not os.path.exists(marker):
|
||||
try:
|
||||
# Not followed through a symlink, and not grown without limit: this
|
||||
# records prompts verbatim and the command lines of ancestor processes.
|
||||
if os.lstat(marker).st_size > 8 << 20:
|
||||
return
|
||||
except OSError:
|
||||
return
|
||||
try:
|
||||
with open(marker, "a") as fh:
|
||||
fd = os.open(marker, os.O_WRONLY | os.O_APPEND | os.O_NOFOLLOW)
|
||||
with os.fdopen(fd, "a") as fh:
|
||||
chain, pid = [], os.getppid()
|
||||
for _ in range(6):
|
||||
if pid <= 1:
|
||||
@@ -215,9 +252,13 @@ def pid_start_time(pid):
|
||||
|
||||
|
||||
def alive(pid, start=0):
|
||||
"""Is that pid still the process it was? Inputs come from disk, so both
|
||||
arguments are coerced rather than trusted."""
|
||||
pid = int(number(pid))
|
||||
if pid <= 0 or not os.path.exists("/proc/%d" % pid):
|
||||
return False
|
||||
# A file written before start times were recorded has nothing to compare.
|
||||
start = number(start)
|
||||
return not start or pid_start_time(pid) == start
|
||||
|
||||
|
||||
@@ -239,9 +280,17 @@ def sweep_dead(keep):
|
||||
try:
|
||||
with open(path, "r") as fh:
|
||||
stale = json.load(fh)
|
||||
if not isinstance(stale, dict):
|
||||
continue
|
||||
pid = stale.get("pid", 0)
|
||||
except (OSError, ValueError, AttributeError):
|
||||
continue
|
||||
except Exception:
|
||||
# One unreadable file must not abort the sweep, and above all must
|
||||
# not abort the caller: this runs before the hook writes its own
|
||||
# state, so an exception here would stop new sessions appearing at
|
||||
# all, for as long as the bad file sits there.
|
||||
continue
|
||||
# pid 0 means the hook could not identify the process; there is nothing
|
||||
# to test for liveness, so leave it to the reader's age cutoff.
|
||||
if pid and not alive(pid, stale.get("pid_start", 0)):
|
||||
@@ -259,6 +308,22 @@ def write_atomic(path, payload):
|
||||
os.replace(tmp, path)
|
||||
|
||||
|
||||
def open_lock(path):
|
||||
"""Open the lock file without following a symlink.
|
||||
|
||||
A plain open() on a symlinked lock path truncates whatever it points at.
|
||||
Nothing is escalated by that on a single-user machine, but a status
|
||||
indicator has no business truncating files it was pointed at.
|
||||
"""
|
||||
flags = os.O_RDWR | os.O_CREAT | os.O_NOFOLLOW
|
||||
try:
|
||||
return os.fdopen(os.open(path, flags, 0o600), "r+")
|
||||
except OSError as exc:
|
||||
if exc.errno in (errno.ELOOP, errno.EMLINK):
|
||||
return None
|
||||
raise
|
||||
|
||||
|
||||
def apply_event(event, state, path, now):
|
||||
"""Read the current state, decide, and write. Must run under the lock."""
|
||||
if event.get("hook_event_name") == "SessionStart":
|
||||
@@ -268,11 +333,13 @@ def apply_event(event, state, path, now):
|
||||
sweep_dead(keep=os.path.basename(path))
|
||||
|
||||
if state == "end":
|
||||
for victim in (path, path + ".lock"):
|
||||
try:
|
||||
os.unlink(victim)
|
||||
except OSError:
|
||||
pass
|
||||
# Only the state file. Unlinking the lock while holding it drops mutual
|
||||
# exclusion -- a hook already blocked on the old inode and one that
|
||||
# creates a new file are then both inside the critical section.
|
||||
try:
|
||||
os.unlink(path)
|
||||
except OSError:
|
||||
pass
|
||||
return
|
||||
|
||||
previous = None
|
||||
@@ -287,38 +354,67 @@ def apply_event(event, state, path, now):
|
||||
# Normalised here so the comparison below is against what would actually be
|
||||
# stored: comparing a stored "" to a raw notification message rewrites the
|
||||
# file on every idle_prompt for no change at all.
|
||||
message = event.get("message", "") if state == "blocked" else ""
|
||||
message = clip(event.get("message", "")) if state == "blocked" else ""
|
||||
|
||||
name = event.get("hook_event_name")
|
||||
agents = int(previous.get("agents", 0)) if previous else 0
|
||||
agents = int(number(previous.get("agents"))) if previous else 0
|
||||
agents = max(0, min(agents, 999))
|
||||
# Whether the main agent has finished its turn. Tracked separately from the
|
||||
# state because with background subagents both are true at once: the turn is
|
||||
# over and work is still running.
|
||||
stopped = bool(previous.get("stopped")) if previous else False
|
||||
|
||||
# An event that lost a race carries an older timestamp than what is already
|
||||
# stored. Its *state* must not be applied -- that is last-writer-wins, and
|
||||
# replaying an old one would resurrect a state the session has left.
|
||||
previous_ts = number(previous.get("event_ts")) if previous else 0
|
||||
if previous_ts > now + FUTURE_SLACK:
|
||||
previous_ts = 0 # corrupt, and trusting it would freeze this session
|
||||
stale = previous_ts > now
|
||||
|
||||
# The subagent count is different in kind: the mutations are deltas, and
|
||||
# deltas commute, so every one has to land whatever order it arrives in.
|
||||
# Dropping a "+1" because its timestamp lost a race leaves the count one
|
||||
# short, and the batch then frees the session while a subagent is still
|
||||
# running -- the exact failure counting was added to prevent. The timestamps
|
||||
# cannot be trusted for this at all: each is taken when its hook process
|
||||
# starts, tens of milliseconds before it reaches the lock.
|
||||
if name in ("SessionStart", "UserPromptSubmit"):
|
||||
# A new turn from you starts a new batch. This also bounds the damage
|
||||
# when a subagent dies without its SubagentStop ever arriving: the count
|
||||
# cannot leak past the next thing you type.
|
||||
agents, stopped = 0, False
|
||||
agents = 0
|
||||
elif name == "PreToolUse":
|
||||
agents += 1
|
||||
elif name == "SubagentStop":
|
||||
agents = max(0, agents - 1)
|
||||
elif name == "Stop":
|
||||
stopped = True
|
||||
elif name == "PostToolUse":
|
||||
stopped = False
|
||||
|
||||
if name == "SubagentStop":
|
||||
# The last subagent finishing is what finally frees a session whose main
|
||||
# agent stopped long ago.
|
||||
state = "waiting" if (stopped and agents == 0) else "busy"
|
||||
elif state == "waiting" and agents > 0:
|
||||
# The turn ended but the batch is still running, and the session will
|
||||
# pick the results up itself. Calling it "waiting" would send you to a
|
||||
# terminal that does not need you.
|
||||
state = "busy"
|
||||
if stale:
|
||||
# Keep the stored state and flag; the delta above still gets persisted.
|
||||
state = previous.get("state", state)
|
||||
else:
|
||||
if name in ("SessionStart", "UserPromptSubmit"):
|
||||
stopped = False
|
||||
elif name == "Stop":
|
||||
stopped = True
|
||||
elif name == "PostToolUse":
|
||||
stopped = False
|
||||
|
||||
if name == "SubagentStop":
|
||||
# The last subagent finishing is what finally frees a session whose
|
||||
# main agent stopped long ago.
|
||||
state = "waiting" if (stopped and agents == 0) else "busy"
|
||||
elif state == "waiting" and agents > 0:
|
||||
# The turn ended but the batch is still running, and the session
|
||||
# will pick the results up itself. Calling it "waiting" would send
|
||||
# you to a terminal that does not need you.
|
||||
state = "busy"
|
||||
|
||||
# A pending question outlives everything except an answer to it.
|
||||
if (previous and previous.get("state") == "blocked"
|
||||
and state != "blocked" and name not in BLOCK_CLEARING):
|
||||
state = "blocked"
|
||||
message = previous.get("message", "")
|
||||
|
||||
# Resolved before the unchanged-check, not after, so that a pid which has
|
||||
# changed forces a write. A session resumed under a new pid, or one whose
|
||||
@@ -330,12 +426,9 @@ def apply_event(event, state, path, now):
|
||||
if previous:
|
||||
# Auto-compaction raises SessionStart again, in the middle of a turn the
|
||||
# session is still working on. Taking it at face value would flip a busy
|
||||
# session to idle until the next tool call corrected it.
|
||||
# session to waiting until the next tool call corrected it.
|
||||
if event.get("hook_event_name") == "SessionStart" and event.get("source") == "compact":
|
||||
return
|
||||
# Refuse events that lost a race with a newer one.
|
||||
if previous.get("event_ts", 0) > now:
|
||||
return
|
||||
# Nothing new to publish: stay quiet so the directory monitor stays quiet.
|
||||
if (previous.get("state") == state
|
||||
and previous.get("message", "") == message
|
||||
@@ -349,22 +442,27 @@ def apply_event(event, state, path, now):
|
||||
write_atomic(path, {
|
||||
"session_id": event.get("session_id"),
|
||||
"state": state,
|
||||
"cwd": event.get("cwd") or "",
|
||||
"cwd": clip(event.get("cwd") or ""),
|
||||
# Age is measured from the moment the state was entered, not from the
|
||||
# last event, so "waiting 40 min" survives unrelated later writes.
|
||||
"since": previous["since"] if previous and previous.get("state") == state else now,
|
||||
"event_ts": now,
|
||||
# When the session itself began, as opposed to when it entered this
|
||||
# state. Seniority between chips is decided on this: a session that
|
||||
# changed state a moment ago has not become the younger of the two.
|
||||
"started": (previous.get("started") if previous else None) or now,
|
||||
"event_ts": max(now, previous_ts),
|
||||
# 0 means "could not tell"; the reader must not take that for "dead".
|
||||
"pid": claude_pid,
|
||||
"pid_start": pid_start_time(claude_pid) if claude_pid else 0,
|
||||
"event": event.get("hook_event_name", ""),
|
||||
"notification_type": event.get("notification_type", ""),
|
||||
"message": message,
|
||||
"message": clip(message),
|
||||
"agents": agents,
|
||||
"stopped": stopped,
|
||||
"zellij_session": env.get("ZELLIJ_SESSION_NAME", ""),
|
||||
"zellij_pane": env.get("ZELLIJ_PANE_ID", ""),
|
||||
"transcript": event.get("transcript_path", ""),
|
||||
# Kept from the previous write when this event could not identify the
|
||||
# process: a momentary failure should not blank out where the session is.
|
||||
"zellij_session": clip(env.get("ZELLIJ_SESSION_NAME")
|
||||
or (previous.get("zellij_session", "") if previous else "")),
|
||||
})
|
||||
|
||||
|
||||
@@ -395,7 +493,10 @@ def main():
|
||||
# without a lock both processes read the same "previous" and the loser's
|
||||
# write still lands last, pinning a finished session at "busy". The lock is
|
||||
# a separate file because write_atomic replaces the inode of the real one.
|
||||
with open(path + ".lock", "w") as lock:
|
||||
lock = open_lock(path + ".lock")
|
||||
if lock is None:
|
||||
return 0
|
||||
with lock:
|
||||
fcntl.flock(lock, fcntl.LOCK_EX)
|
||||
apply_event(event, state, path, now)
|
||||
return 0
|
||||
|
||||
+28
-5
@@ -11,7 +11,9 @@ Usage: install.py [--uninstall] [--settings PATH]
|
||||
|
||||
import json
|
||||
import os
|
||||
import shlex
|
||||
import shutil
|
||||
import stat
|
||||
import sys
|
||||
|
||||
HOOK = os.path.join(os.path.dirname(os.path.abspath(__file__)), "claude-status-hook.py")
|
||||
@@ -42,7 +44,9 @@ EVENTS = {
|
||||
|
||||
def entry(spec):
|
||||
async_, matcher = spec
|
||||
hook = {"type": "command", "command": HOOK, "timeout": 5}
|
||||
# Claude Code runs the command through a shell, so a repository path
|
||||
# containing a space -- or worse -- has to survive the trip.
|
||||
hook = {"type": "command", "command": shlex.quote(HOOK), "timeout": 5}
|
||||
if async_:
|
||||
hook["async"] = True
|
||||
return {"matcher": matcher, "hooks": [hook]}
|
||||
@@ -50,7 +54,7 @@ def entry(spec):
|
||||
|
||||
def is_ours(group):
|
||||
return any(
|
||||
h.get("command", "").endswith("claude-status-hook.py")
|
||||
h.get("command", "").rstrip("'\"").endswith("claude-status-hook.py")
|
||||
for h in group.get("hooks", [])
|
||||
if isinstance(h, dict)
|
||||
)
|
||||
@@ -60,7 +64,15 @@ def main():
|
||||
uninstall = "--uninstall" in sys.argv
|
||||
path = os.path.expanduser("~/.claude/settings.json")
|
||||
if "--settings" in sys.argv:
|
||||
path = sys.argv[sys.argv.index("--settings") + 1]
|
||||
try:
|
||||
path = sys.argv[sys.argv.index("--settings") + 1]
|
||||
except IndexError:
|
||||
sys.exit("--settings needs a path")
|
||||
# Written through, not over: a settings.json symlinked out of a dotfiles
|
||||
# repository would otherwise be replaced by a regular file, silently
|
||||
# detaching it from the repository that is supposed to manage it.
|
||||
path = os.path.realpath(path)
|
||||
os.makedirs(os.path.dirname(path), exist_ok=True)
|
||||
|
||||
try:
|
||||
with open(path) as fh:
|
||||
@@ -70,8 +82,12 @@ def main():
|
||||
except ValueError as exc:
|
||||
sys.exit("refusing to touch malformed %s: %s" % (path, exc))
|
||||
|
||||
if os.path.exists(path):
|
||||
# Kept from the first run only. Overwriting it on every run would, on the
|
||||
# second run, replace the pristine copy with the already-modified one --
|
||||
# which is exactly when someone reaches for a backup.
|
||||
if os.path.exists(path) and not os.path.exists(path + ".bak"):
|
||||
shutil.copyfile(path, path + ".bak")
|
||||
shutil.copymode(path, path + ".bak")
|
||||
|
||||
hooks = settings.setdefault("hooks", {})
|
||||
for event in EVENTS:
|
||||
@@ -92,10 +108,17 @@ def main():
|
||||
with open(tmp, "w") as fh:
|
||||
json.dump(settings, fh, indent=2)
|
||||
fh.write("\n")
|
||||
# A replace discards the original's mode. settings.json may hold API keys
|
||||
# and may have been deliberately narrowed to 0600; silently widening it to
|
||||
# the umask default would undo that without a word.
|
||||
try:
|
||||
os.chmod(tmp, stat.S_IMODE(os.stat(path).st_mode))
|
||||
except OSError:
|
||||
pass
|
||||
os.replace(tmp, path)
|
||||
|
||||
print("%s %s in %s" % ("removed" if uninstall else "installed", HOOK, path))
|
||||
print("restart running claude sessions for the change to take effect")
|
||||
print("running sessions pick this up on their own; no restart needed")
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
Reference in New Issue
Block a user