[sonichi]

What changed, and why

A new optional skill, pr-watch: event-driven GitHub PR watching for an agent. Watching PRs today means polling or a Monitor that expires every 30 minutes. This skill uses GitHub's gh webhook forward (the cli/gh-webhook extension) inside a Claude Code mod, so each relevant event reaches the session (a new turn when idle, or context on the next tool result mid-turn), with no polling and no expiry.

• skills/pr-watch/scripts/pr_watch.py: CLI (add, remove, list, status, install-mod, cleanup, config, track) and the runner (run) the mod spawns. The runner keeps one gh webhook forward per watched repo, parses its stdout JSON stream, filters by the watch config, and prints one PR_EVENT … line per event. Standard library plus gh.
• skills/pr-watch/mod/: the mod. On session.start it spawns the runner, splits stdout into lines and drops repeats within 60 s. It respawns the runner after 30 s if it exits, except when the extension is missing or another runner holds the lock.
• Mid-turn delivery. $.prompt.submit only starts a turn once the session is idle, so the mod tracks turn.start / turn.complete. Idle: the event is submitted as a prompt built from the template the runner sent (see Configuration). Turn running: the event is held and a tool.call hook attaches it as context to the main loop's next tool result (never a subagent's, never a denied call); whatever is still held at turn.complete is submitted as one prompt. The tool.call hook carries .catch(($, e, next) => next.called ? next(e) : { deny }). The mod never passes $ to a helper and imports no node:* module (either makes the engine refuse it); SKILL.md records both under "Editing the mod".
• Events: pull_request opened/synchronize/reopened/ready_for_review/closed (with merged) on a watched author's PR (reason=author); review_requested when the reviewer is one of the configured "me" logins, on any PR in the repo; pull_request_review submitted on a watched author's PR (reviewer + state), except a review by one of the "me" logins: that is the owner's own review echoing back, so it is dropped (it still marks the PR reviewed for the reviewed rule). Only logins, numbers, states and short shas reach the prompt. Titles and bodies never do.
• PRs the owner already reviewed (add --reviewed, needs a non-* --me): synchronize/reopened/ready_for_review/closed on any PR in the repo where a "me" login has submitted a review, whoever wrote it (reason=reviewed). The payload doesn't list reviewers, so the runner asks repos/{repo}/pulls/{n}/reviews once (10 s timeout) and caches the answer per PR: a yes for the runner's lifetime, a no for 10 minutes; an owner review it sees submitted marks the PR at once. On an API failure the event is forwarded with reviewed=unknown, never dropped. classify() takes the lookup as an argument and stays pure. An event matching both rules yields one reason=author line, with no lookup.
• Dismissed owner reviews: pull_request_review dismissed where the dismissed review is by a "me" login, on any PR in the repo → review_dismissed reviewer=… author=… head=…. The forwarder already subscribes to pull_request,pull_request_review (now pinned by a test).

Configuration (no hardcoded values)

• The skill's settings are declared in skills/pr-watch/manifest.json config, per skills/MANIFEST.md "Config-only manifests": PR_WATCH_PROMPT (the event prompt template), PR_WATCH_TRACKER_ROOM (default empty: no tracker), PR_WATCH_TRACKER_TITLE (default PR watch), PR_WATCH_COMMONS_SCRIPT (default skills/room-commons/scripts/room_commons.py, resolved against the repo root derived from pr_watch.py's location, never the cwd).
• Read by setting(name, cli=None, st=None) with the documented precedence CLI > env > manifest > <state>/config.json settings; an empty value at any layer counts as unset. pr_watch.py config get [KEY] | config set KEY VALUE writes the config.json layer, signals the runner, and says so when env or a non-empty manifest default still wins.
• The default prompt exists once, in the manifest. The runner resolves it and prints PR_WATCH_PROMPT_TEMPLATE <json string> on start and on every SIGHUP; the mod keeps the last one it received and dropped its own copy (and its $.fs.read of config.json). Before any template arrives, the mod sends the bare event lines. {track} in the template becomes a one-sentence "record the outcome with track" hint only when a tracker room is set.
• install-mod also copies manifest.json beside the runner copy, so the copy reads the same defaults. No room ids, logins or repo names are in the skill's code, docs or tests (examples use acme/widgets, alice, !room:server).
• From a fresh clone without a workspace symlink, resolve_workspace resolves to <clone>/workspace; SKILL.md says to pass --state-dir there (or run the engine checkout's copy).

Tracker: an HTML page in a configurable Commons room

• pr_watch.py track --pr OWNER/REPO#N [--author A] [--state open|merged|closed] [--head SHA8] [--verdict approved|changes_requested|commented|none] [--event TEXT] [--link URL] [--room R] [--title T] upserts a row in <state>/tracker.json keyed repo#n with updated_at. With no room set it only records and says so in one line.
• render_tracker(rows, title, now) is pure: a self-contained HTML page with inline CSS for light and dark (prefers-color-scheme) and phone width (scrolling table), open PRs first then newest, columns PR (link) / author / state / head reviewed / verdict / last event / updated, and the note "Generated by pr-watch; edits here are overwritten." Every value is HTML-escaped; only http(s) URLs become links.
• Publishing goes through the room-commons CLI, run with sys.executable and a 60 s timeout. The CLI has no whole-page write for HTML pages, so the whole-page replace is read then replace -- <room> <current text> <new page>:
  • first publish: --json page-add --kind html <room> <title>, page kind (html-<id>) and room recorded in tracker.json;
  • later: --json pages <room> (is the page still listed?), --json --kind html-<id> read <room>, --kind html-<id> replace -- <room> <old> <new>;
  • a page no longer listed, or a different room, gets a new page.
• Any failure keeps the row, prints one PR_WATCH_NOTE tracker publish failed: … and exits 0; the next track republishes everything. A missing script path is noted and skipped. The runner never publishes and never needs room credentials; only track, run by the agent, does.

Limits handled in code:

1. One forwarder hook per repo (HTTP 422 Hook already exists): the runner lists forwarder hooks (name: cli, url webhook-forwarder.github.com) before it starts. If one exists that it didn't create, it emits a PR_WATCH_NOTE, doesn't start, and retries every 5 minutes, or at once on the next SIGHUP (add, remove, config set): the reload bumps a wake generation that cuts every backed-off loop's wait short. The reason and retry time are kept in backoff.json, and status prints backing off: foreign forwarder hook <id>, retry at HH:MM.
2. A killed forwarder leaves its hook behind: the runner records the hook ids its forwarders create in state/pr-watch/hooks.json. It deletes exactly those ids on forwarder restart, on runner stop and at the next start, and never deletes another hook. A failed delete stays recorded; a 404 counts as gone.
3. Runner killed outright: the gh children's pids are kept in forwarders.json, and the next start stops any that are still running.
4. Mod-written files reload the mod: all state, and the runner copy install-mod places, live under <workspace>/state/pr-watch/ (workspace resolved via scripts/sutando-config.sh workspace). install-mod refuses a state dir inside the mod.
5. Lifetime: the runner is a $.process.spawn child, so a reload or session end kills it and the next session.start respawns it. A file lock keeps it to one runner per state dir.

What to look at first

classify(), ReviewedLookup, drop_recorded() and Runner.forward_once() in pr_watch.py; then mod/hooks/register.ts (the turn.start / turn.complete / tool.call hooks). For the config and tracker: setting_source(), prompt_template(), render_tracker(), publish_tracker() and cmd_track().

Worst-case disruption to users, and mitigation

The skill is opt-in. The manifest declares no claude_plugin, tools or hooks, so nothing loads unless the owner runs install-mod and turns on hot reload for the mods folder (or passes --plugin-dir). Without a config the mod stays idle. The repo copy's paths.ts is empty, so even the in-repo mod stays idle. The destructive action, deleting webhooks, is limited to ids this skill recorded when it created them. Author-rule lines gained a trailing reason=author; a custom prompt template that matched the old line exactly would need a look. The mid-turn path adds text only to the main loop's tool results, and only while events are waiting. The one CI change adds a claude plugin test line to the existing Mod harness job. Installs from an earlier head of this PR have a "prompt" key in config.json; it is no longer read (the template comes from the manifest, overridable by PR_WATCH_PROMPT in the env). The tracker is off until a room is set, and its failures never fail track.

What user behavior this proves / evidence

Feature, so there's no failing "before". Evidence at HEAD:

$ python3 tests/skills/pr-watch/pr_watch.test.py
Ran 90 tests in 32.473s
OK
$ claude plugin validate skills/pr-watch/mod
  ❯ ./register.ts hooks: session.start, turn.start, turn.complete, tool.call
  ❯ ./register.ts gating hook with .catch: tool.call
  ❯ ./register.ts calls: $.clock.now, $.clock.sleep, $.process.spawn, $.prompt.submit, $.ui.log
✔ Validation passed
$ (cd skills/pr-watch && claude plugin test mod)
 9 pass
 0 fail
$ bash scripts/review-checks.sh --diff <(git diff origin/main...HEAD)
review-checks: PASS (hardcoded-paths + root-artifacts + prose-cap clean)

Coverage was not re-run locally for this update (no heavy local runs on the author's machine); PR CI's coverage gate measures it.

The Python suite covers:

• the config CLI;
• the filter, against fixture payloads for every forwarded event type plus ignored ones (labeled, dismissed) and a forged-newline field;
• the JSON stream splitter (pretty or compact objects, partial tail, noise);
• the hook record, against a fake gh on PATH;
• install-mod;
• the reviewed rule (my review → reason=reviewed; none → dropped; lookup failure → reviewed=unknown; only the four actions; author+reviewed → one reason=author line with no lookup), the ReviewedLookup cache (one call per PR, the 10-minute no, the lifetime yes, failures not cached, an observed owner review marking the PR) and the dismissed rule (mine → forwarded; someone else's → not);
• that the forwarder is started with --events=pull_request,pull_request_review, and that forward_once passes the lookup to the filter;
• settings: each layer beats the ones below it (CLI > env > manifest > config.json), an empty value falls through, the shipped manifest declares every key, config set/get, and the "a higher layer still wins" message;
• the runner's PR_WATCH_PROMPT_TEMPLATE line: one JSON line on start and one per SIGHUP (in the real run loop and in-process), the {track} hint only with a room, and an env override;
• the tracker: upsert keeps earlier fields and stamps updated_at; open-first then newest ordering; bad --pr/--state/--author refused with nothing written; render_tracker escaping (a login and an event containing <script>), a javascript: link rendered as text, dark-mode CSS and viewport present; publishing against a fake room-commons script: the first track runs page-add, read, replace and records html-p1, the second runs pages, read, replace on the same page, a removed page and a new room each get a new page, a failing page-add or a timeout keeps the row and exits 0 with one PR_WATCH_NOTE, the next track republishes, a missing script is noted and skipped, and an unset room only records;
• the backoff: a foreign hook is recorded with its reason and retry time and status prints it (and not running without a live runner), the record clears when the forwarder starts; a backed-off loop stays asleep without a wake and retries at once on one; shutdown wakes it; and cmd_run against a fake gh retries a backed-off repo within seconds of a SIGHUP once the foreign hook is gone;
• the owner's own review_submitted is dropped while observe still marks the PR (with no reviews API call);
• the real run loop against a fake gh. That test checks that events become lines and that the hook the runner created is recorded and then deleted on SIGTERM. It also checks that a pre-existing forwarder hook is reported, never deleted, and never forwarded over.

Positive controls: removing the per-repo filter in drop_recorded fails test_only_recorded_ids_are_deleted, and dropping the pre-existing-hook check fails test_a_hook_it_did_not_create_is_reported_and_never_deleted. Replacing _esc/_link with unescaped versions fails test_render_escapes_every_value_and_links_only_http, and making Runner.wake a no-op fails test_cmd_run_retries_a_backed_off_repo_on_sighup.

The mod's pure helpers (takeLines including the template line, parseTemplate, Dedupe, messageFor with a template and with none yet, takeEvents, withContext) are unit-tested under claude plugin test; the hook wiring itself (turn.start/turn.complete/tool.call ordering in a live session) is not exercised by a test here.

This PR has no live round trip against a GitHub repo, and the tracker has not been published to a real room: the room-commons commands are exercised only against a fake script that mimics their argument and JSON shapes. The behavior it's built from (one forwarder per repo, a hook left behind after a kill, the stdout JSON stream) was observed on a working single-author reference setup, which this generalizes.

Feature/documentation consistency

skills/pr-watch/SKILL.md is the skill's canonical doc: when to use it, setup, requirements, costs and limits. No existing docs/ page covers PR watching, so docs/catalog.json is unchanged. N/A otherwise.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 94%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

• skills/pr-watch/scripts/pr_watch.py (98.2%): Missing lines 150-151,206,767,778-779,827-828,839-840,850,857,863,879

Summary

• Total: 765 lines
• Missing: 14 lines
• Coverage: 98%

skills/pr-watch/scripts/pr_watch.py

Lines 146-155

  146 
  147 def manifest_config() -> dict:
  148     try:
  149         cfg = json.loads(MANIFEST.read_text()).get("config") or {}
! 150     except (OSError, ValueError, AttributeError):
! 151         return {}
  152     return cfg if isinstance(cfg, dict) else {}
  153 
  154 
  155 def setting_source(name: str, cli: Optional[str] = None, st: Optional[State] = None) -> Tuple[str, str]:

---

Lines 202-210

  202             return 0
  203         print(json.dumps({k: dict(zip(("value", "from"), setting_source(k, st=st))) for k in SETTINGS}, indent=2))
  204         return 0
  205     if not key or value is None:
! 206         raise SystemExit("pr-watch: config set KEY VALUE")
  207     setting_source(key)
  208     st.ensure()
  209     cfg = load_config(st)
  210     settings = cfg.setdefault("settings", {})

---

Lines 763-771

  763 """
  764 
  765 
  766 def _now() -> str:
! 767     return datetime.datetime.now(datetime.timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ")
  768 
  769 
  770 def _esc(value) -> str:
  771     return html.escape("" if value is None else str(value), quote=True)

---

Lines 774-783

  774 def _link(url, label: str) -> str:
  775     """An anchor for an http(s) url; anything else is the escaped label alone."""
  776     try:
  777         ok = isinstance(url, str) and urlparse(url).scheme in ("http", "https") and bool(urlparse(url).netloc)
! 778     except ValueError:
! 779         ok = False
  780     return f'<a href="{_esc(url)}" rel="noopener noreferrer">{_esc(label)}</a>' if ok else _esc(label)
  781 
  782 
  783 def tracker_order(rows: Iterable[dict]) -> List[dict]:

---

Lines 823-832

  823         r = subprocess.run([sys.executable, str(script)] + args, capture_output=True, text=True,
  824                            timeout=COMMONS_TIMEOUT_S)
  825     except subprocess.TimeoutExpired:
  826         raise PublishError(f"{_verb(args)} timed out after {COMMONS_TIMEOUT_S}s")
! 827     except OSError as exc:
! 828         raise PublishError(f"{_verb(args)}: {exc}")
  829     if r.returncode != 0:
  830         why = (r.stderr or r.stdout or "").strip().splitlines()
  831         raise PublishError(f"{_verb(args)} exited {r.returncode}: {why[-1] if why else 'no output'}")
  832     return r.stdout

---

Lines 835-844

  835 def _commons_json(script: Path, args: List[str]):
  836     out = run_commons(script, args)
  837     try:
  838         return json.loads(out)
! 839     except ValueError:
! 840         raise PublishError(f"{_verb(args)} printed no JSON")
  841 
  842 
  843 def publish_tracker(st: State, data: dict, room: str, title: str, script: Path) -> str:
  844     """Write the whole tracker page into the room, creating it when it is missing."""

---

Lines 846-854

  846     kind = data.get("page_kind") if data.get("page_room") == room else None
  847     if kind:
  848         listed = _commons_json(script, ["--json", "pages", room])
  849         if not isinstance(listed, list):
! 850             raise PublishError("pages printed no list")
  851         if kind not in {p.get("kind") for p in listed if isinstance(p, dict)}:
  852             kind = None
  853     if not kind:
  854         made = _commons_json(script, ["--json", "page-add", "--kind", "html", room, title])

---

Lines 853-861

  853     if not kind:
  854         made = _commons_json(script, ["--json", "page-add", "--kind", "html", room, title])
  855         kind = made.get("kind") if isinstance(made, dict) else None
  856         if not isinstance(kind, str) or not PAGE_KIND_RE.match(kind):
! 857             raise PublishError(f"page-add returned no html page kind: {str(made)[:80]}")
  858         data["page_kind"], data["page_room"] = kind, room
  859         write_json(st.tracker, data)
  860     got = _commons_json(script, ["--json", "--kind", kind, "read", room])
  861     current = got.get("text") if isinstance(got, dict) else None

---

Lines 859-867

  859         write_json(st.tracker, data)
  860     got = _commons_json(script, ["--json", "--kind", kind, "read", room])
  861     current = got.get("text") if isinstance(got, dict) else None
  862     if not isinstance(current, str):
! 863         raise PublishError("read returned no page text")
  864     if current != page:
  865         run_commons(script, ["--kind", kind, "replace", "--", room, current, page])
  866     return kind

---

Lines 875-883

  875         raise SystemExit(f"pr-watch: not a GitHub login: {args.author!r}")
  876     st.ensure()
  877     data = read_json(st.tracker, {})
  878     if not isinstance(data, dict):
! 879         raise SystemExit(f"pr-watch: {st.tracker} is not a JSON object")
  880     rows = data.setdefault("rows", {})
  881     key = f"{repo}#{number}"
  882     row = rows.get(key) or {"repo": repo, "number": number, "link": f"https://github.com/{repo}/pull/{number}"}
  883     for field in ("author", "state", "head", "verdict", "event", "link"):

---

[github-actions]

@cla-assistant check

[qingyun-wu]

Approving at 44c11d7.

Read the skill end to end: pr_watch.py (config/settings, classify, ReviewedLookup, hook record/drop, the Runner loop and backoff, tracker render/publish, CLI), mod/hooks/register.ts, the mod tests, the manifest, SKILL.md and the CI/tsconfig changes.

What I checked and found sound:

• Nothing from GitHub can steer the session. Every value that reaches a PR_EVENT line goes through _safe (charset-restricted, 80 chars); titles and bodies never leave the runner, and test_untrusted_fields_cannot_break_the_line pins the forged-newline case. The prompt template comes only from the manifest or the owner's env/config.
• Webhook deletion is bounded. drop_recorded deletes exactly the ids in hooks.json; a foreign forwarder hook is reported and backed off, never touched (test_a_hook_it_did_not_create_is_reported_and_never_deleted). Stale forwarder pids are killed only when the process command still says webhook … forward.
• The mod follows the engine's rules: $ never leaves a hook, no node:* imports, the gating tool.call hook carries the .catch that denies only when next was not called, subagent calls (e.agentId) and denied calls never carry events, and turn.complete clears the flag before next(e).
• Opt-in by construction: the manifest declares no plugin/hooks, the repo copy's paths.ts is empty, so nothing runs until install-mod plus the owner's own hot-reload step.

Verified locally in a scratch worktree at 44c11d7 (one commit past the head the description's evidence was taken at; it adds sortable tracker columns and the state copy's commons-script resolution, plus one test):

• python3 tests/skills/pr-watch/pr_watch.test.py: 91 tests, OK.
• claude plugin validate skills/pr-watch/mod: passed; claude plugin test mod: 9 pass, 0 fail.
• bash scripts/review-checks.sh --diff: PASS; ruff check on the script and test: clean.
• CI at posting: 16 passing, 18 pending, failing: none.

One non-blocking note for a follow-up: Runner._record records any forwarder hook that appears within 30 s of starting gh webhook forward, and since before is empty by then, a hook another person creates on the same repo in that window would be recorded as ours and deleted on the next restart. Narrow unless two admins forward the same repo at the same moment, so fine to leave.

Agent-assisted review by Mars, Qingyun's Sutando seat, at the owner's request.