fix(pr-daemon): rate-limit and coalesce hints
Every hint costs the receiving session a full model turn. The daemon sent one per forge event with no quiet period, so a busy PR produced 20+ in a day and sometimes repeated a payload verbatim. Reasons are now banked per PR and role until a hint actually goes out, so a busy pane or a cooldown delays one but never loses it. After a hint, the next waits hintCooldownSeconds (default 300) and carries everything that accumulated. A payload identical to the last is dropped, as is a ci hint to a land session whose worktree already holds that head commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
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.
|
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
|
**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
|
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
|
doesn't wake every open PR you have. It gates creation only — start a session
|
||||||
|
|||||||
@@ -2,6 +2,7 @@
|
|||||||
"pollSeconds": 60,
|
"pollSeconds": 60,
|
||||||
"reconcileSeconds": 120,
|
"reconcileSeconds": 120,
|
||||||
"maxSessionsPerTick": 2,
|
"maxSessionsPerTick": 2,
|
||||||
|
"hintCooldownSeconds": 300,
|
||||||
"reviewProfile": "review",
|
"reviewProfile": "review",
|
||||||
"group": "pr",
|
"group": "pr",
|
||||||
"webhookPort": 7474,
|
"webhookPort": 7474,
|
||||||
|
|||||||
+67
-9
@@ -42,6 +42,7 @@ const ROLES_DEFAULT = ["review", "blitz"];
|
|||||||
type Config = {
|
type Config = {
|
||||||
pollSeconds?: number;
|
pollSeconds?: number;
|
||||||
reconcileSeconds?: number;
|
reconcileSeconds?: number;
|
||||||
|
hintCooldownSeconds?: number; // quiet period per PR and role between hints
|
||||||
maxSessionsPerTick?: number;
|
maxSessionsPerTick?: number;
|
||||||
reviewProfile?: string;
|
reviewProfile?: string;
|
||||||
reviewTool?: string;
|
reviewTool?: string;
|
||||||
@@ -97,6 +98,17 @@ const dirty = new Set<string>();
|
|||||||
const noPulls = new Set<string>();
|
const noPulls = new Set<string>();
|
||||||
let firstRun = false;
|
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<string, Set<string>>();
|
||||||
|
const hintedAt = new Map<string, number>();
|
||||||
|
const lastHint = new Map<string, string>();
|
||||||
|
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);
|
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
|
// 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<Set<string> | 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.
|
// Ids of everything commented after `since`. null means the fetch failed.
|
||||||
async function newCommentIds(pr: Pr, since: string): Promise<string[] | null> {
|
async function newCommentIds(pr: Pr, since: string): Promise<string[] | null> {
|
||||||
try {
|
try {
|
||||||
@@ -838,6 +858,16 @@ async function evaluate(prs: Pr[], mentioned: Set<string>, budget: { sessions: n
|
|||||||
log(`${session.title} is waiting on input (${full.key})`);
|
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<string>();
|
||||||
|
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,
|
// A send into a busy pane can be swallowed. Since hints are idempotent,
|
||||||
// holding it costs one cycle and nothing else.
|
// holding it costs one cycle and nothing else.
|
||||||
if (st !== "idle") {
|
if (st !== "idle") {
|
||||||
@@ -849,25 +879,53 @@ async function evaluate(prs: Pr[], mentioned: Set<string>, budget: { sessions: n
|
|||||||
if (known && !known.prompted) {
|
if (known && !known.prompted) {
|
||||||
await send(known.profile, session.id, opening(full, skill));
|
await send(known.profile, session.id, opening(full, skill));
|
||||||
known.prompted = true;
|
known.prompted = true;
|
||||||
|
pending.delete(pkey); // the opening sends it to read the PR whole
|
||||||
|
hintedAt.set(pkey, Date.now());
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Conflicts are the author's to resolve on their own branch, so the
|
const acc = pending.get(pkey);
|
||||||
// reviewer never hears about them. Comments it does hear: a reply to a
|
if (!acc?.size) continue; // label, assignee, edited title: nothing to act on
|
||||||
// finding is addressed to the reviewer, and an addressed thread is the
|
|
||||||
// reviewer's to resolve (review-pr §3.1).
|
if (acc.has("comments") && prev) {
|
||||||
let mine = role === "land" ? why : why.filter((w) => w !== "conflicts");
|
|
||||||
if (mine.includes("comments") && prev) {
|
|
||||||
const ids = await newCommentIds(full, prev.updatedAt);
|
const ids = await newCommentIds(full, prev.updatedAt);
|
||||||
const seen = session.path ? await seenIds(session.path, full.number) : null;
|
const seen = session.path ? await seenIds(session.path, full.number) : null;
|
||||||
if (ids?.length && seen && ids.every((id) => seen.has(id))) {
|
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`);
|
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 {
|
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}`);
|
log(`hint ${full.key} reason=${mine.join(",")} -> ${session.title}`);
|
||||||
} catch (e) {
|
} catch (e) {
|
||||||
dirty.add(full.key);
|
dirty.add(full.key);
|
||||||
|
|||||||
Reference in New Issue
Block a user