diff --git a/README.md b/README.md index d0bd33a..d36f9de 100644 --- a/README.md +++ b/README.md @@ -145,6 +145,14 @@ skill on arrival if it doesn't have it. bump `updated_at` and move nothing in the snapshot, so no hint is sent at all. With webhooks the filter is sharper still, by event action. +**Hints are rate-limited per PR and role.** Every hint costs the receiving +session a full model turn, so after one goes out the next waits +`hintCooldownSeconds` (default 300) and arrives carrying every reason that +accumulated meanwhile. A hint identical to the last one sent is dropped, and so +is a `ci` hint to a `land` session whose own worktree already holds that head +commit — it pushed it. Reasons are banked until they are actually delivered, so +a busy pane or a cooldown delays a hint but never loses one. + **An epoch guards the first run.** `~/.local/state/reviewer/epoch` is written once; PRs created before it never spawn a session, so switching the daemon on doesn't wake every open PR you have. It gates creation only — start a session diff --git a/bin/agents-config.example.json b/bin/agents-config.example.json index 4efa8a5..6c8a43c 100644 --- a/bin/agents-config.example.json +++ b/bin/agents-config.example.json @@ -2,6 +2,7 @@ "pollSeconds": 60, "reconcileSeconds": 120, "maxSessionsPerTick": 2, + "hintCooldownSeconds": 300, "reviewProfile": "review", "group": "pr", "webhookPort": 7474, diff --git a/bin/reviewer-poll.ts b/bin/reviewer-poll.ts index 00c84c2..52340c4 100644 --- a/bin/reviewer-poll.ts +++ b/bin/reviewer-poll.ts @@ -42,6 +42,7 @@ const ROLES_DEFAULT = ["review", "blitz"]; type Config = { pollSeconds?: number; reconcileSeconds?: number; + hintCooldownSeconds?: number; // quiet period per PR and role between hints maxSessionsPerTick?: number; reviewProfile?: string; reviewTool?: string; @@ -97,6 +98,17 @@ const dirty = new Set(); const noPulls = new Set(); let firstRun = false; +// The snapshot advances on the tick that diffed it, so a reason not sent +// immediately can never be recomputed. Held per PR and role until it goes out. +const pending = new Map>(); +const hintedAt = new Map(); +const lastHint = new Map(); +const HINT_COOLDOWN_MS = (config.hintCooldownSeconds ?? 300) * 1000; + +// Canonical order, so a coalesced hint reads the same however it accumulated. +// That is what makes the duplicate check below meaningful. +const REASON_ORDER = ["ci", "state", "conflicts", "comments"]; + const log = (...args: unknown[]) => console.log(new Date().toISOString(), ...args); // A webhook has to cut the wait short, or its only effect would be to mark a @@ -321,6 +333,14 @@ async function seenIds(worktree: string, n: number): Promise | null> } } +// True when the session's own worktree already holds the PR head, which means +// the session pushed it and does not need waking to hear about its own commit. +function pushedLocally(worktree: string, sha: string): boolean { + if (!sha) return false; + const proc = Bun.spawnSync(["git", "-C", worktree, "rev-parse", "HEAD"]); + return proc.exitCode === 0 && proc.stdout.toString().trim() === sha; +} + // Ids of everything commented after `since`. null means the fetch failed. async function newCommentIds(pr: Pr, since: string): Promise { try { @@ -838,6 +858,16 @@ async function evaluate(prs: Pr[], mentioned: Set, budget: { sessions: n log(`${session.title} is waiting on input (${full.key})`); } + // Conflicts are the author's to resolve on their own branch, so the + // reviewer never hears about them. Comments it does hear: a reply to a + // finding is addressed to the reviewer, and an addressed thread is the + // reviewer's to resolve (review-pr §3.1). + // Banked before anything can skip out of this iteration. + const pkey = `${full.key}:${role}`; + const banked = pending.get(pkey) ?? new Set(); + for (const r of role === "land" ? why : why.filter((w) => w !== "conflicts")) banked.add(r); + if (banked.size) pending.set(pkey, banked); + // A send into a busy pane can be swallowed. Since hints are idempotent, // holding it costs one cycle and nothing else. if (st !== "idle") { @@ -849,25 +879,53 @@ async function evaluate(prs: Pr[], mentioned: Set, budget: { sessions: n if (known && !known.prompted) { await send(known.profile, session.id, opening(full, skill)); known.prompted = true; + pending.delete(pkey); // the opening sends it to read the PR whole + hintedAt.set(pkey, Date.now()); continue; } - // Conflicts are the author's to resolve on their own branch, so the - // reviewer never hears about them. Comments it does hear: a reply to a - // finding is addressed to the reviewer, and an addressed thread is the - // reviewer's to resolve (review-pr §3.1). - let mine = role === "land" ? why : why.filter((w) => w !== "conflicts"); - if (mine.includes("comments") && prev) { + const acc = pending.get(pkey); + if (!acc?.size) continue; // label, assignee, edited title: nothing to act on + + if (acc.has("comments") && prev) { const ids = await newCommentIds(full, prev.updatedAt); const seen = session.path ? await seenIds(session.path, full.number) : null; if (ids?.length && seen && ids.every((id) => seen.has(id))) { - mine = mine.filter((w) => w !== "comments"); + acc.delete("comments"); log(`comments on ${full.key} already in ${session.title}'s seen file, hint dropped`); } } - if (!mine.length) continue; // label, assignee, edited title: nothing to act on + // The land session pushed the commit CI is running on, so the run is no + // news to it. A reviewer still hears about it: the head moved under them. + if (acc.has("ci") && role === "land" && session.path && pushedLocally(session.path, full.headSha)) { + acc.delete("ci"); + log(`ci on ${full.key} is ${session.title}'s own push, hint dropped`); + } + if (!acc.size) { + pending.delete(pkey); + continue; + } + + const wait = HINT_COOLDOWN_MS - (Date.now() - (hintedAt.get(pkey) ?? 0)); + if (wait > 0) { + dirty.add(full.key); + setTimeout(wake, wait + 1000); + log(`cooling ${full.key} (${[...acc].join(",")}): ${Math.round(wait / 1000)}s left`); + continue; + } + + const mine = REASON_ORDER.filter((r) => acc.has(r)); + const message = hint(full, mine, skill); + if (lastHint.get(pkey) === message) { + pending.delete(pkey); + log(`hint ${full.key} repeats the last one verbatim, dropped`); + continue; + } try { - await send(session.profile, session.id, hint(full, mine, skill)); + await send(session.profile, session.id, message); + pending.delete(pkey); + hintedAt.set(pkey, Date.now()); + lastHint.set(pkey, message); log(`hint ${full.key} reason=${mine.join(",")} -> ${session.title}`); } catch (e) { dirty.add(full.key);