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.
This commit is contained in:
Symlink
+1
@@ -0,0 +1 @@
|
||||
../../docs/guides/push-watch.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.<name>].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.<name>].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.<name>].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.<name>].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 <nonce>` 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 <jonathan@ten31.vc>`, 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.
|
||||
|
||||
@@ -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.<name>].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 <nonce>` 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=<branch>&since=<RFC3339>&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.
|
||||
Reference in New Issue
Block a user