From 6c67245fb5d1c017bc02d57d7b86bf2a2dfae515 Mon Sep 17 00:00:00 2001 From: Keysat Date: Fri, 21 Aug 2026 12:35:26 -0500 Subject: [PATCH] Handoff: move the watch internals to a scoped guide, prune Current state AGENTS.md had grown 72% in one session, most of it push-watch mechanics that only matter when editing src/bot.py. Those move to docs/guides/push-watch.md (paths-scoped, symlinked from .claude/rules, matching the ten31-database and spark-control convention); AGENTS.md keeps the operational half and a pointer. Current state goes back to being a snapshot rather than a session log. --- .claude/rules/push-watch.md | 1 + AGENTS.md | 137 ++++++++++++------------------------ docs/guides/push-watch.md | 123 ++++++++++++++++++++++++++++++++ 3 files changed, 170 insertions(+), 91 deletions(-) create mode 120000 .claude/rules/push-watch.md create mode 100644 docs/guides/push-watch.md diff --git a/.claude/rules/push-watch.md b/.claude/rules/push-watch.md new file mode 120000 index 0000000..618bbfc --- /dev/null +++ b/.claude/rules/push-watch.md @@ -0,0 +1 @@ +../../docs/guides/push-watch.md \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index aae9d36..eafd9c3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,45 +68,23 @@ flat JSON in a writable `state/` mount. because the bot merges with `force_merge: true` (proven on ten31-site PR #4 against this exact rule shape). Tell a collaborator that "1 approval required" is expected, not stuck. Post-merge publishing is opt-in per repo (`[repo.].deploy_on_merge`); without it a merge merges and redacts, nothing more. -- **Push watch (opt-in per repo):** notifies whenever someone who isn't me pushes commits, and posts a - `claude -p` assessment of what they did. Turn on with `[repo.].watch_pushes = true` plus - `[gitea] my_emails` (my commit emails — anyone *not* on that list notifies, so an unrecognised - address surfaces rather than slips through; these repos' commits aren't linked to Gitea accounts, - the API returns `author: null`, so the **git email is the only identity that works**). Optional +- **Push watch (opt-in per repo):** notifies whenever someone who isn't me pushes commits, and posts + a `claude -p` assessment of what they did as a threaded reply. Turn on with + `[repo.].watch_pushes = true` plus `[gitea] my_emails` (my commit emails — anyone *not* on + that list notifies, so an unrecognised address surfaces rather than slips through). Optional `[repo.].expected_paths` globs mark the collaborator's expected territory; files outside it - are flagged in the notice (deterministically, not by the model) and called out to the assessor. - Shape: **notify first, assess second** — the notice posts immediately, the commits are recorded in - `state/` *before* the assessment starts, and the assessment lands as a threaded reply. Scope is - every branch *except* branches with an open PR; the default branch is walked first, so a commit - reachable from several branches is reported as being on `master` rather than on some stale branch. - On first run the watch arms at "now" (it never replays history into the room); to force a test, - backdate `watch.since` in `state/rooms.json`. - - **Why the ordering:** recording before the assessment is what stops a crash, a restart or a send - error in those minutes from reposting the whole notice and re-running the assessment. The cost is - the opposite failure — a crash mid-assessment leaves a bare "_Assessing…_" in the room with no - retry. Preferred deliberately: a missing assessment is visible, a duplicate one is noise. - - **What it says out loud rather than logging:** more than `WATCH_MAX_PAGES × WATCH_COMMIT_LIMIT` - (150) commits arriving at once, and a commit whose file list Gitea didn't return (the path check - is then incomplete, and it must never print the all-clear). - - **Bounds:** ≤`WATCH_MAX_BATCHES` (2) assessments per repo per cycle, each ≤`[defaults] - watch_timeout_seconds` (480s; a real two-commit run with one panel subagent measured 173s), so a - busy push can hold the other rooms' polling for ~16 min worst case — the head-of-line cost ROADMAP - Phase 3 tracks, now bounded rather than open-ended. Held-over batches keep their commits out of - `seen` and pin the `since` cursor, so they go out next cycle; after `WATCH_GIVEUP_HOURS` (48) the - bot stops pinning and logs what it gave up on, so an unsendable batch can't freeze the watch. - - **Merging a PR banks its commits** (`remember_pr_commits`, including the merge commit Gitea - creates), whether it was merged from the thread or in Gitea's web UI — otherwise those commits - land on `master` and read as a fresh push of work the review already covered. - - **What it is not:** the author check reads the git email, which anyone can set. It is an - accountability aid for a cooperative collaborator, not an authentication boundary. And push - notices are *not* redactable — `clear` only reaches PR threads, so they accumulate as a log. + are flagged in the notice, computed from the file list rather than asked of the model. Scope is + every branch except ones with an open PR. On first run it arms at "now" and never replays history. + Push notices are **not** redactable (`clear` only reaches PR threads), so they accumulate as a log, + and the author check reads the git email — an accountability aid for a cooperative collaborator, + not an authentication boundary. **Before editing the watch, read `docs/guides/push-watch.md`:** the + record-before-assess ordering, the `since` cursor's four rules, the per-cycle bounds and the prompt + fence are all load-bearing and none are obvious from the code. - **Tests:** `python3 tests/test_watch.py` — no pytest, no deps, stubs matrix-nio. Covers the push - watch's *policy*: who gets reported, cross-branch dedupe, the `since` cursor, the path flag, and - that every attacker-authored field lands inside the prompt's fence. 17 mutations of `src/bot.py` - were each confirmed to flip it to FAIL (2026-08-21). What it does **not** cover is the orchestration - inside `watch_repo` — the record-before-assess ordering, the per-cycle cap, held batches — because - that lives in closures over the live clients; those are verified by exercising the bot in a real - room, which is still how everything else here is verified. + watch's *policy* (who gets reported, dedupe, the cursor, the path flag, prompt fencing); 17 + mutations of `src/bot.py` were each confirmed to flip it to FAIL (2026-08-21). The orchestration + inside `watch_repo`, and everything else in this bot, is still verified by exercising it in a real + review room. - **Prove the poll loop is alive** (silence is normal — the loop logs only on a review or an error, so a healthy bot can go days without a line). Sample the Spark's host socket table fast enough to catch a poll: `end=$((SECONDS+130)); while [ $SECONDS -lt $end ]; do case "$(cat /proc/net/tcp)" in *EE73*) @@ -138,11 +116,11 @@ flat JSON in a writable `state/` mount. secrets/config via read-only mounts; entrypoint writes `~/.ssh/config` for the Mac alias). - `tests/` — `test_watch.py` + a sanitized five-branch Gitea fixture (identities replaced, history overlap preserved because that overlap is what the dedupe is for). -- `state/` — gitignored runtime JSON (`rooms.json`: room map + agents + heads + thread roots, plus - `watch: {since, seen[]}` per push-watched room — `since` is the Gitea `since` cursor, trailing the - newest commit by 24h so a committer's clock skew can't hide a push, and clamped to now so a - future-dated commit can't push the cursor into the future and silence the watch; `seen` is the last - 500 SHAs accounted for). +- `state/` — gitignored runtime JSON (`rooms.json`: room map + agents + heads + thread roots + + default branch, plus `watch: {since, seen[]}` per push-watched room; the cursor's rules are in the + push-watch guide). +- `docs/guides/push-watch.md` — the watch's internals (symlinked from `.claude/rules/`, scoped to + `src/bot.py`, so it loads when you edit the bot). ## Decisions @@ -158,12 +136,10 @@ flat JSON in a writable `state/` mount. - **Panel composition is per-room, set in chat** (onboarding message + `agents +/-`), not config. `design-checker` joined the panel with the push watch — it audits against a repo's own `design/DESIGN.md` + tokens, which is exactly the brand question ten31-brand's room asks. -- **Push watch: notify first, assess second** (2026-08-21). The notice is a plain Gitea-derived - message posted before any `claude -p` runs, and the path flag ("outside the expected area") is - computed from globs, not asked of the model — so the thing you actually need (that someone pushed, - and where) never depends on a model call that can time out, fail, or be wrong. The assessment is a - threaded reply under it. Beat: one composed message after the assessment (prettier, but a 10-minute - `claude -p` budget sits between a push and knowing about it). +- **Push watch: notify first, assess second** (2026-08-21). What you need (someone pushed, and where) + is plain Gitea data and a glob check, so it never depends on a model call that can be slow, fail or + be wrong; the assessment threads under it. Beat: one composed message after the assessment + (prettier, but a 480s budget then sits between a push and knowing about it). - **Watch scope: every branch except ones with an open PR.** A PR already gets the full review treatment, so re-notifying its commits is noise. Beat: master-only (misses a side-branch push that never opens a PR). @@ -181,11 +157,9 @@ flat JSON in a writable `state/` mount. ## Sovereignty **Untrusted input.** A PR's title/description and a pushed commit's subjects, filenames and diff are -written by someone else and go to `claude -p` running on the Mac, in the owner's checkout, under the -owner's OAuth token. Both prompts put *every* such field inside one `UNTRUSTED DATA ` fence -with a per-run random tag, so a forged terminator can't close it, and instruct the model to report -rather than obey anything inside. That is the cheap half; the tool-policy half (an explicit read-only -`--allowedTools` on that run) needs a matrix-bridge change and is the top ROADMAP item. +written by someone else and reach `claude -p` on the Mac, in my checkout, under my OAuth token. Both +prompts fence every such field (see the push-watch guide); the remaining half is a read-only tool +policy on that run, which needs a matrix-bridge change and is the top ROADMAP item. Reviews send PR diffs to `claude -p` (the subscription), not a frontier API on payload data; that's @@ -194,45 +168,26 @@ revisit this — local inference via Spark Control would be the path. ## Current state -**LIVE and in steady use**, serving **two** review rooms. Verified 2026-08-18: the container had been -up since 08-13 with zero restarts, and the poll loop is genuinely alive (two Gitea connections exactly -60s apart — see the liveness recipe in Commands). +**LIVE on the Spark** (`e5a187b` + docs, no redeploy pending), two review rooms, container healthy, +poll loop alive. `python3 tests/test_watch.py` green. -**2026-08-21 — the collaborator push watch shipped and is LIVE on the Spark** (`e5a187b`, no redeploy -pending). Verified end to end in the real room, not just in tests: -- a real notice + assessment for jonathan's `75cc63d` (VERDICT: OK, panel off), and -- the owner's own `4230f30`/`d307bdd`, pushed while the watch was already running, recorded silently - with no notice — the identity filter proven on live traffic rather than a fixture, and -- the record-before-assess ordering proven at runtime: the commit was already in `state/rooms.json` - while the assessment's `ssh mac-bridge` process was still alive. +**The push watch is live and verified in the real room** (2026-08-21): a notice + assessment landed +for jonathan's `75cc63d`, and my own commits pushed minutes later were recorded silently. On for +ten31-brand only. The Spark's `config.toml` holds the new `my_emails` + `[repo.ten31-brand]` keys +(gitignored, so they exist only there); pre-change copies of it and `rooms.json` are backed up +alongside them, dated `20260821`. -Two independent reviewer passes ran over it. The first found the batch being recorded only *after* -its assessment (a crash would have replayed the whole notice) and an all-clear line printable from a -missing file list; the second found untrusted PR/commit metadata sitting above the prompt's fence. -All fixed, each with a mutation that reproduces it. The Spark's `config.toml` carries the new -`[gitea] my_emails` + `[repo.ten31-brand]` keys (gitignored, so they live only there); the pre-change -copies are at `~/gitea-review-bot/config.toml.bak-20260821` and `~/rooms.json.bak-20260821`. - -- **ten31-site** — proven end-to-end on PR #4 (2026-06-28: review → `merge` + `yes` → force_merge → - auto-publish → redacted). **PR #5 "Update index.html" is still open**, reviewed 2026-06-30, its thread - live in the room and waiting on a `merge`/`reject`. Push watch is **off** here. -- **ten31-brand** — onboarded 2026-08-18 to watch the interactive deck - (`design/deck/interactive/index.html`). Collaborator `jonathan` uses **both** routes, so both are - now covered: three PRs (#1–#3, all his, all merged 08-19/08-20, all reviewed by the bot) **and** - direct pushes to `master` — 18 of the last 50 commits there are his - (`Jonathan Kirkwood `, most recently 2026-08-21). Two earlier notes here were - wrong and are corrected: he is *not* blocked from pushing to `master`, and the repo has *not* had - zero PRs. Every commit he has made touches exactly one file, - `design/deck/interactive/index.html`, which is why `expected_paths` is that subtree. - The review threads for #2 and #3 are **orphaned in the room right now** — merged outside the bot, - so the stale prune dropped their roots without redacting. That is Next step 2 below, no longer - hypothetical. +- **ten31-site** — PR #5 "Update index.html" still open, reviewed 2026-06-30, waiting on a + `merge`/`reject` in its thread. Push watch off here. +- **ten31-brand** — `jonathan` uses **both** routes: three PRs (#1–#3, all merged) *and* direct + pushes to `master` (18 of the last 50 commits). Both are covered now. Every commit he has made + touches only `design/deck/interactive/index.html`, hence that `expected_paths`. **Next steps, in priority order:** -1. **Fix the orphaned-thread prune** (`src/bot.py` ~L594): the poll loop drops a PR's thread root from - state when the PR leaves the open list *without redacting*, so a PR merged or closed in the Gitea web - UI strands its thread and `clear` can no longer reach it. More likely to bite now with two rooms. -2. **`/triage` the 3 untriaged inbox items** tagged for this repo — adjudication never reads the inbox, so - their content is still unknown here. -3. With a 2nd repo mapped, ROADMAP Phase 3's verify is unblocked: next time both rooms have open PRs at - once, watch a cycle (`docker compose logs -f`) for a long review head-of-line-blocking the other room. +1. **Fix the orphaned-thread prune** (`src/bot.py`): a PR resolved in Gitea's web UI loses its thread + root from state without being redacted, so `clear` can't reach it. No longer hypothetical — the + threads for ten31-brand #2 and #3 are stranded in that room right now. +2. **`/triage` the 3 untriaged inbox items** tagged for this repo. +3. From ROADMAP: a read-only tool policy for the `claude -p` run (needs a matrix-bridge change), and + the Phase 3 verify — next time both rooms have open PRs, watch a cycle for one review + head-of-line-blocking the other. diff --git a/docs/guides/push-watch.md b/docs/guides/push-watch.md new file mode 100644 index 0000000..93d186d --- /dev/null +++ b/docs/guides/push-watch.md @@ -0,0 +1,123 @@ +--- +paths: + - src/bot.py +--- + +# The collaborator push watch + +Read this before editing the watch in `src/bot.py`. It answers "why is it written this way", +which is not obvious from the code and is where the bugs were. + +**What it is:** opt-in per repo (`[repo.].watch_pushes`), it notices commits pushed by +anyone who isn't me and posts an assessment of them. Shipped 2026-08-21 for ten31-brand, where a +collaborator with write access pushes straight to `master`. + +## Notify first, assess second + +The notice is plain Gitea data — author, branch, commit subjects, files, links — and the +"outside the expected area" flag is computed from `expected_paths` globs, never asked of the +model. So the thing you actually need (someone pushed, and where) never depends on a `claude -p` +call that can be slow, fail, or be wrong. The assessment is a threaded reply under it. + +**The batch is recorded in `state/` before the assessment starts.** This is the ordering, and it +is load-bearing: a send error, a container update or the poll-task guard firing in the minutes an +assessment takes would otherwise leave the commits unrecorded, and the next cycle would repost the +whole notice and re-run the assessment. The accepted cost is the opposite failure — a crash +mid-assessment leaves a bare "_Assessing…_" in the room with no retry. A missing assessment is +visible; a duplicate notice is noise. (Found by a reviewer pass, not in production.) + +## The `since` cursor (`state/rooms.json` → `watch: {since, seen[]}`) + +`since` is Gitea's `since` query param; `seen` is the last `WATCH_SEEN_MAX` (500) SHAs accounted +for. Four rules, each protecting against a way the watch goes quiet or repeats itself: + +- **Trails the newest commit by `WATCH_LOOKBACK_HOURS` (24).** A commit date comes from the + committer's machine, so a clock a few hours behind would otherwise land a commit *before* the + cursor and hide it forever. +- **Clamped to now.** Without it a single future-dated commit parks the cursor ahead of every real + push and silences the watch permanently. +- **Never moves backwards**, and is **floored** at the oldest commit that was listed but not + notified (a notice that failed to post, or a batch deferred by the per-cycle cap), so a held-over + push stays listable instead of ageing out of the window unseen. +- **Gives up after `WATCH_GIVEUP_HOURS` (48)** and logs what it dropped, so a permanently + unsendable batch (lost send power, a tombstoned room) can't freeze the cursor for good. + +On first run the watch arms at "now" — it never replays history into a room. To force a test, +stop the container, backdate `watch.since`, start it. `state/` is root-owned on the Spark and +`sudo` there wants a password, so edit it with a throwaway root container: +`docker run --rm -v "$PWD/state:/s" python:3.12-slim python -c '...'`. + +## Scope, dedupe, and not repeating the PR review + +- Every branch **except** ones with an open PR — that PR's review already covers those commits. +- The **default branch is walked first** (looked up once and cached in `room["default_branch"]`), + because a commit is usually reachable from several branches and gets reported under whichever is + walked first. "3 new commits on master" is the true answer for anything that has landed. +- **Merging or closing a PR banks its commits** (`remember_pr_commits`, including the merge commit + Gitea creates, which is not in the PR's own commit list). This runs on the in-thread merge/close + path *before* the deploy — a deploy can take `DEPLOY_TIMEOUT` and a poll cycle landing inside that + window would see the merged commits as a fresh push — and on the poll loop's stale-PR path, which + is what catches a merge done in Gitea's web UI. + +## Bounds (one push must not stall every room) + +The poll loop is one serial async task, so an assessment blocks every other room. Hence +`WATCH_MAX_BATCHES` (2) assessments per repo per cycle, each bounded by `[defaults] +watch_timeout_seconds` (480s). **Basis for 480:** a real two-commit assessment with one panel +subagent measured 173s wall clock on 2026-08-21. Worst case is therefore ~16 min of held polling — +the head-of-line cost ROADMAP Phase 3 tracks, now bounded rather than open-ended. Deferred batches +keep their commits out of `seen` and pin the cursor, so they go out next cycle. + +`list_branch_commits` pages to `WATCH_MAX_PAGES × WATCH_COMMIT_LIMIT` (150) commits per branch per +cycle. Past that it says so **in the room**, not just the log — a silent cap reads as "nothing to +report", which is the one thing this feature must never do. Same rule for a commit whose file list +Gitea didn't return: the path check is then incomplete and must never print the all-clear. + +## Identity + +`commit_is_mine` reads the **git email** against `[gitea] my_emails`, because these repos' commits +aren't linked to Gitea accounts — the API returns `author: null`. Anything unrecognised counts as +someone else's, so a new collaborator notifies rather than slips through. A linked Gitea account +matching the repo owner also counts as mine, for repos where linking does work. + +**This is not an authentication boundary.** Anyone can set `user.email`. It is an accountability +aid for a cooperative collaborator. + +## Untrusted input + +A PR's title and description, and a pushed commit's subjects, filenames and diff, are written by +someone else and reach `claude -p` on the Mac, in the owner's checkout, under the owner's OAuth +token. Both prompt builders put **every** such field inside one `UNTRUSTED DATA ` fence +(`untrusted_fence`), with a per-run random tag so a forged terminator can't close it, and instruct +the model to report rather than obey anything inside. The instruction region carries only values +the attacker doesn't control: the repo label and globs from our own config, the PR number, branch +refs (git forbids a newline in one), and counts. + +A first version fenced only the diff while the commit listing and PR body sat above it — escapable +with a forged terminator, caught by a reviewer pass. `tests/test_watch.py` now asserts *placement* +(hostile text lands below the BEGIN marker), not the presence of a marker. + +The remaining half is an explicit read-only tool policy on that run; it needs a matrix-bridge +change and is the top ROADMAP item. + +## Gitea API facts (verified against 1.27.1, 2026-08-21) + +- `GET /repos/{o}/{r}/commits?sha=&since=&page=&limit=&files=true&stat=false&verification=false` + — `since` wants RFC3339; emit it as `...Z` (`iso_z`) so a `+00:00` offset never has to survive URL + encoding, and `urllib.parse.quote(branch, safe="")` because a branch name can contain `+`. +- `GET /repos/{o}/{r}/git/commits/{sha}.diff` with `Accept: text/plain` for a single commit's diff. +- `GET /repos/{o}/{r}/pulls/{n}/commits` and `GET /repos/{o}/{r}/pulls/{n}` (`merged_commit_id`). +- The live token is scoped `write:repository`, which covers all of the above. + +## What the tests cover, and what they don't + +`python3 tests/test_watch.py` (no pytest, no deps; stubs matrix-nio, which the policy code doesn't +need) covers the pure policy: who gets reported, cross-branch dedupe, replay suppression, the +cursor's four rules, the path flag, and fence placement — against a sanitized five-branch fixture +whose branches genuinely overlap, since that overlap is what dedupe is for. 17 mutations of +`src/bot.py` were each confirmed to flip it to FAIL (2026-08-21). + +It does **not** cover the orchestration in `watch_repo` — record-before-assess ordering, the +per-cycle cap, held batches — because that lives in closures over the live Matrix and SSH clients. +Those were verified by exercising the bot in the real room: the commit was already in +`state/rooms.json` while the assessment's `ssh mac-bridge` process was still alive.