Files
Keysat 6c67245fb5 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.
2026-08-21 12:35:26 -05:00

7.6 KiB
Raw Permalink Blame History

paths
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.