Agent review: spend limits per review, per pull request and per repository (#70)
Security's spend controls for agent review, enforced by Diffinery rather than the agent: - Per review: the model proxy counts each run's requests. A review stops at 200 model requests or 30 minutes, the proxy answers 429, and the check fails. - Per pull request: 10 automatic reviews and 20 in all in 24 hours. Past them, the head gets an action-required check, and asking returns 429. - Per repository: a daily limit the payer sets (default 50, 1 to 1000) on Settings -> Agent review or with daily_limit. Past it, the same check and 429, and the payer gets a review_limit_reached email at most once a day. Claude-Session: https://claude.ai/code/session_014VDQqT8oL18uLJ9fpsNxW2 Co-authored-by: Claude <noreply@anthropic.com>
Canberk Demirci authored · GitHub committed signed1 parent 01bc028commit a84e06b6dd1abc403e1ff14b1cc3a51e120a238cBrowse files
14 files changed+245−23
Files (14)
- apps/diffinery/docs/agents/review.md+21−5
- apps/diffinery/src/agentic/ProjectDO.ts+48−5
- apps/diffinery/src/agentic/api.ts+1−1
- apps/diffinery/src/agentic/proxy.ts+2−1
- apps/diffinery/src/agentic/review-web.tsx+10−3
- apps/diffinery/src/agentic/review.ts+51−0
- apps/diffinery/src/agentic/reviews.ts+9−3
- apps/diffinery/src/docs/api-meta.ts+3−2
- apps/diffinery/src/mail/samples.ts+1−0
- apps/diffinery/src/mail/templates.ts+18−0
- apps/diffinery/src/mail/types.ts+3−0
- apps/diffinery/test/e2e/review.test.mjs+53−1
- apps/diffinery/test/unit/email.test.ts+1−1
- apps/diffinery/test/unit/review.test.ts+24−1
modifiedapps/diffinery/docs/agents/review.md+21−5View file
| @@ -19,7 +19,8 @@ | ||
| 19 | 19 | - **Only when someone asks.** |
| 20 | 20 | 4. Choose **which pull requests** to review: all of them, only ones from people, or only ones from agents. Drafts are skipped unless you tick **Review draft pull requests too**. |
| 21 | 21 | 5. Optionally turn on **Autofix** (see [Fix it and autofix](#fix-it-and-autofix)). |
| 22 | -6. Tick the confirmation and click **Turn on agent review**. | |
| 22 | +6. Set the **daily limit**: the most reviews the repository runs in 24 hours (50 unless you change it; see [Limits](#limits)). | |
| 23 | +7. Tick the confirmation and click **Turn on agent review**. | |
| 23 | 24 | |
| 24 | 25 | **Every review on the repository uses your model key**, automatic reviews and ones other people ask for alike. The settings page and every review's footer name whose key it is. Another admin who saves the settings becomes the one whose key is used. **Turn off** stops new reviews. |
| 25 | 26 | |
| @@ -45,7 +46,8 @@ | ||
| 45 | 46 | | One or more blocking findings | failure | |
| 46 | 47 | | The agent produced no findings block | failure: "The review didn't produce findings" | |
| 47 | 48 | | The review was stopped, or a newer push superseded it | cancelled | |
| 48 | -| Your key or your access is gone, or the author can't be reviewed automatically | action required | | |
| 49 | +| The review ran past its time or model-request limit | failure: "The review didn't finish" | | |
| 50 | +| Your key or your access is gone, the author can't be reviewed automatically, or a daily limit was reached | action required | | |
| 49 | 51 | |
| 50 | 52 | The review and its findings show on the pull request like any review. The run, with its full transcript, is on the repository's **Tasks** tab, labelled **Review**. Reviews don't count toward required approvals, and an agent review never requests changes. Whether findings block a merge is up to the check. |
| 51 | 53 | |
| @@ -94,9 +96,23 @@ | ||
| 94 | 96 | |
| 95 | 97 | A passing Agent review means **the agent found nothing**, not that the change is safe. Pull request content can try to trick the agent ("prompt injection") into reporting nothing. Keep people's approvals required as well. |
| 96 | 98 | |
| 99 | +## Limits | |
| 100 | + | |
| 101 | +Diffinery enforces these itself, so an agent misled by a pull request can't run past them: | |
| 102 | + | |
| 103 | +| Limit | What happens | | |
| 104 | +| --- | --- | | |
| 105 | +| **30 minutes** per review | The review stops, and its check fails. | | |
| 106 | +| **200 model requests** per review | The model proxy refuses the next request and stops the review, and its check fails. | | |
| 107 | +| **10 automatic reviews** per pull request in 24 hours | Later pushes get an action-required "Waiting for a request" check. People with write access can still ask. | | |
| 108 | +| **20 reviews** per pull request in 24 hours, asked for or automatic | The check asks for action, and asking returns `429`. | | |
| 109 | +| The repository's **daily limit** (default 50, from 1 to 1000) | The check asks for action, asking returns `429`, and the admin whose key reviews use gets an email, at most once a day. | | |
| 110 | + | |
| 111 | +Daily limits count the reviews started in the last 24 hours, so reviews run again as older ones fall out of that window. Change the repository's limit under **Settings → Agent review**. | |
| 112 | + | |
| 97 | 113 | ## Who pays, and the risks |
| 98 | 114 | |
| 99 | -- Every review spends the model key of the admin who turned reviews on. Use a **dedicated key with a spending limit**. | |
| 115 | +- Every review spends the model key of the admin who turned reviews on. Use a **dedicated key with a spending limit** with your provider as well: Diffinery's [limits](#limits) count reviews and model requests, not tokens or money. | |
| 100 | 116 | - While a review runs, anything in its sandbox, including code from the pull request, can use that key through the model proxy. The proxy forwards only the model endpoints, and the review's model token stops working when the agent exits (see [How your key is protected](/docs/agents/tasks#how-your-key-is-protected)). |
| 101 | 117 | - The sandbox can't push or write repository content. Its credential acts as the agent with read access. |
| 102 | 118 | |
| @@ -105,9 +121,9 @@ | ||
| 105 | 121 | | Endpoint | Does | Who | |
| 106 | 122 | | --- | --- | --- | |
| 107 | 123 | | `GET /repos/{owner}/{repo}/diffinery/agent-review` | The repository's agent review settings | Write access | |
| 108 | -| `PUT /repos/{owner}/{repo}/diffinery/agent-review` | Turn on, change (`agent`, `runtime`, `command`, `trigger`: `push`, `open` or `request`, `authors`: `all`, `people` or `agents`, `drafts`, `autofix`, `accept_terms: true`) or turn off (`enabled: false`) | Repository admin, browser session only | | |
| 124 | +| `PUT /repos/{owner}/{repo}/diffinery/agent-review` | Turn on, change (`agent`, `runtime`, `command`, `trigger`: `push`, `open` or `request`, `authors`: `all`, `people` or `agents`, `drafts`, `autofix`, `daily_limit`: 1 to 1000, `accept_terms: true`) or turn off (`enabled: false`) | Repository admin, browser session only | | |
| 109 | 125 | | `GET /repos/{owner}/{repo}/pulls/{number}/diffinery/agent-review` | The pull request's review task, the last reviewed head and every finding with its state (`open`, `fixed`, `dismissed`) | Write access | |
| 110 | -| `POST /repos/{owner}/{repo}/pulls/{number}/diffinery/agent-review` | Ask for a review | People with write access (not agents) | | |
| 126 | +| `POST /repos/{owner}/{repo}/pulls/{number}/diffinery/agent-review` | Ask for a review (`429` past a daily limit) | People with write access (not agents) | | |
| 111 | 127 | |
| 112 | 128 | ```json |
| 113 | 129 | { |
modifiedapps/diffinery/src/agentic/ProjectDO.ts+48−5View file
| @@ -5,7 +5,8 @@ | ||
| 5 | 5 | import { atLeast } from "../repo/types.js"; |
| 6 | 6 | import { taskCaller, TaskAccessError } from "./access.js"; |
| 7 | 7 | import type { Provider } from "./keys.js"; |
| 8 | -import { countText, findingComment, rulesFiles, findingId, MAX_AUTOFIXES, placeFindings, REVIEW_APP, REVIEW_CHECK, REVIEW_CREATOR, reviewBody, reviewOutput, reviewPrompt, fixGoal, type Finding, type PriorFinding, type ReviewSettings } from "./review.js"; | |
| 8 | +import { enqueueMail } from "../mail/send.js"; | |
| 9 | +import { countText, dayAgo, findingComment, rulesFiles, findingId, MAX_AUTOFIXES, placeFindings, REPO_DAILY_DEFAULT, REVIEW_APP, REVIEW_CHECK, REVIEW_CREATOR, REVIEW_MAX_MODEL_CALLS, REVIEW_TIMEOUT_MS, reviewBody, reviewLimit, reviewOutput, reviewPrompt, fixGoal, shouldMailLimit, type Finding, type PriorFinding, type ReviewLimit, type ReviewSettings } from "./review.js"; | |
| 9 | 10 | import { agentEnv, finalMessage, INSTALL_TIMEOUT_MS, RUN_TIMEOUT_MS, RUNTIMES, runPrompt, taskBranch, taskTitle, type RuntimeId, type TranscriptLine } from "./runtimes.js"; |
| 10 | 11 | |
| 11 | 12 | /** |
| @@ -74,6 +75,8 @@ | ||
| 74 | 75 | out_off: number; |
| 75 | 76 | err_off: number; |
| 76 | 77 | token_hash: string | null; |
| 78 | + /** Requests the run's agent made through the model proxy. */ | |
| 79 | + model_calls: number; | |
| 77 | 80 | exit_code: number | null; |
| 78 | 81 | error: string | null; |
| 79 | 82 | checked_at: string | null; |
| @@ -169,6 +172,7 @@ | ||
| 169 | 172 | ["tasks", "reviewed_sha", "reviewed_sha TEXT"], |
| 170 | 173 | ["tasks", "autofixes", "autofixes INTEGER NOT NULL DEFAULT 0"], |
| 171 | 174 | ["runs", "head_sha", "head_sha TEXT"], |
| 175 | + ["runs", "model_calls", "model_calls INTEGER NOT NULL DEFAULT 0"], | |
| 172 | 176 | ]; |
| 173 | 177 | |
| 174 | 178 | /** A finding as stored. */ |
| @@ -355,7 +359,7 @@ | ||
| 355 | 359 | * Review a pull request now, or once its current review ends. An automatic |
| 356 | 360 | * request for a head that was already reviewed does nothing. |
| 357 | 361 | */ |
| 358 | - async requestReview(input: ReviewRequest): Promise<{ task: number; status: "started" | "queued" | "skipped" }> { | |
| 362 | + async requestReview(input: ReviewRequest): Promise<{ task: number; status: "started" | "queued" | "skipped" | "limited"; reason?: string }> { | |
| 359 | 363 | const s = input.settings; |
| 360 | 364 | let t = this.reviewTask(input.pr); |
| 361 | 365 | const at = now(); |
| @@ -404,17 +408,50 @@ | ||
| 404 | 408 | return { task: t.number, status: "queued" }; |
| 405 | 409 | } |
| 406 | 410 | if (!input.requestedBy && t.reviewed_sha === input.headSha) return { task: t.number, status: "skipped" }; |
| 411 | + const limited = await this.overLimit(t, input.headSha, !input.requestedBy); | |
| 412 | + if (limited) return { task: t.number, status: "limited", reason: limited.summary }; | |
| 407 | 413 | await this.reviewCheck(t, input.headSha, "queued", null, "Queued", ""); |
| 408 | 414 | this.startRun(t.number, "review", input.requestedBy, input.providers); |
| 409 | 415 | return { task: t.number, status: "started" }; |
| 410 | 416 | } |
| 411 | 417 | |
| 418 | + /** | |
| 419 | + * The daily limits: when one is reached, the head gets an action-required | |
| 420 | + * check instead of a review, and the payer an email (once a day) when it is | |
| 421 | + * the repository's. | |
| 422 | + */ | |
| 423 | + private async overLimit(t: TaskRow, headSha: string, automatic: boolean): Promise<ReviewLimit | null> { | |
| 424 | + const settings = this.reviewSettings(); | |
| 425 | + if (!settings) return null; | |
| 426 | + const since = dayAgo(); | |
| 427 | + const count = (where: string, ...args: unknown[]) => this.sql.exec<{ n: number }>(`SELECT count(*) AS n FROM runs WHERE kind = 'review' AND started_at > ? ${where}`, since, ...args).one().n; | |
| 428 | + const limit = reviewLimit({ repo: count(""), pr: count("AND task = ?", t.number), prAutomatic: count("AND task = ? AND message IS NULL", t.number) }, automatic, settings); | |
| 429 | + if (!limit) return null; | |
| 430 | + await this.reviewCheck(t, headSha, "completed", "action_required", limit.title, limit.summary); | |
| 431 | + if (limit.scope === "repo" && shouldMailLimit(settings)) { | |
| 432 | + this.setReviewSettings({ ...settings, limitMailedAt: now() }); | |
| 433 | + const repo = await this.repoPath(t); | |
| 434 | + await enqueueMail(this.env, [{ kind: "review_limit_reached", data: { repo, limit: settings.dailyLimit ?? REPO_DAILY_DEFAULT, pr: t.pr_number! }, to: { accountId: settings.person.id }, reason: "your_activity", at: now() }]); | |
| 435 | + } | |
| 436 | + return limit; | |
| 437 | + } | |
| 438 | + | |
| 412 | 439 | /** The model proxy's check: which run a proxy token belongs to, while that run's agent is running. */ |
| 413 | - async modelAuth(token: string): Promise<{ task: number; run: number; personId: string; agentId: string; repoId: string; providers: Provider[] } | null> { | |
| 440 | + async modelAuth(token: string): Promise<{ task: number; run: number; personId: string; agentId: string; repoId: string; providers: Provider[] } | { limit: string } | null> { | |
| 414 | 441 | const hash = await sha256(token); |
| 415 | 442 | const r = this.sql.exec<RunRow>("SELECT * FROM runs WHERE token_hash = ? AND step = 'agent'", hash).toArray()[0]; |
| 416 | 443 | if (!r) return null; |
| 417 | 444 | const t = this.task(r.task)!; |
| 445 | + const calls = r.model_calls + 1; | |
| 446 | + this.sql.exec("UPDATE runs SET model_calls = ? WHERE task = ? AND seq = ?", calls, r.task, r.seq); | |
| 447 | + if (t.kind === "review") { | |
| 448 | + // A review's budget is charged here, not trusted to the agent: its sandbox runs the pull request's code. | |
| 449 | + const limit = Date.parse(r.started_at) + REVIEW_TIMEOUT_MS < Date.now() ? `The review took longer than ${REVIEW_TIMEOUT_MS / 60_000} minutes and was stopped.` : calls > REVIEW_MAX_MODEL_CALLS ? `The review used its limit of ${REVIEW_MAX_MODEL_CALLS} model requests and was stopped.` : null; | |
| 450 | + if (limit) { | |
| 451 | + await this.endRun(r, "failed", limit); | |
| 452 | + return { limit }; | |
| 453 | + } | |
| 454 | + } | |
| 418 | 455 | return { task: t.number, run: r.seq, personId: t.person_id, agentId: t.agent_id, repoId: t.repo_id, providers: JSON.parse(r.providers) as Provider[] }; |
| 419 | 456 | } |
| 420 | 457 | |
| @@ -458,7 +495,7 @@ | ||
| 458 | 495 | |
| 459 | 496 | private async advance(r: RunRow): Promise<void> { |
| 460 | 497 | const t = this.mustTask(r.task); |
| 461 | - if (Date.parse(r.started_at) + RUN_TIMEOUT_MS < Date.now()) return this.endRun(r, "failed", "The run took longer than 6 hours and was stopped."); | |
| 498 | + if (Date.parse(r.started_at) + timeoutOf(t) < Date.now()) return this.endRun(r, "failed", t.kind === "review" ? `The review took longer than ${REVIEW_TIMEOUT_MS / 60_000} minutes and was stopped.` : "The run took longer than 6 hours and was stopped."); | |
| 462 | 499 | switch (r.step) { |
| 463 | 500 | case "boot": |
| 464 | 501 | return t.kind === "review" ? this.reviewBoot(t, r) : this.boot(t, r); |
| @@ -478,7 +515,7 @@ | ||
| 478 | 515 | const token = `dmt_${t.repo_id}_${randomToken(32)}`; |
| 479 | 516 | this.sql.exec("UPDATE runs SET token_hash = ?, checked_at = ? WHERE task = ? AND seq = ?", await sha256(token), now(), r.task, r.seq); |
| 480 | 517 | const { env, secret } = agentEnv(this.env.PUBLIC_URL, providers, token); |
| 481 | - return this.startProc(r, "agent", RUNTIMES[t.runtime].command(t.command ?? undefined), { ...env, DIFFINERY_TASK_PROMPT: r.prompt }, secret, RUN_TIMEOUT_MS - (Date.now() - Date.parse(r.started_at)), null); | |
| 518 | + return this.startProc(r, "agent", RUNTIMES[t.runtime].command(t.command ?? undefined), { ...env, DIFFINERY_TASK_PROMPT: r.prompt }, secret, timeoutOf(t) - (Date.now() - Date.parse(r.started_at)), null); | |
| 482 | 519 | } |
| 483 | 520 | if (!r.checked_at || Date.parse(r.checked_at) + RECHECK_MS < Date.now()) { |
| 484 | 521 | await this.checkAccess(t); |
| @@ -805,6 +842,10 @@ | ||
| 805 | 842 | this.sql.exec("UPDATE tasks SET state = 'ready', updated_at = ? WHERE number = ?", now(), t.number); |
| 806 | 843 | return; |
| 807 | 844 | } |
| 845 | + if (await this.overLimit(t, issue.pull.headSha, !requestedBy)) { | |
| 846 | + this.sql.exec("UPDATE tasks SET state = 'ready', updated_at = ? WHERE number = ?", now(), t.number); | |
| 847 | + return; | |
| 848 | + } | |
| 808 | 849 | await this.reviewCheck(t, issue.pull.headSha, "queued", null, "Queued", ""); |
| 809 | 850 | this.startRun(t.number, "review", requestedBy, providers); |
| 810 | 851 | } |
| @@ -858,6 +899,8 @@ | ||
| 858 | 899 | } |
| 859 | 900 | } |
| 860 | 901 | |
| 902 | +const timeoutOf = (t: TaskRow): number => (t.kind === "review" ? REVIEW_TIMEOUT_MS : RUN_TIMEOUT_MS); | |
| 903 | + | |
| 861 | 904 | function splitLines(s: string): string[] { |
| 862 | 905 | if (!s) return []; |
| 863 | 906 | const lines = s.split("\n"); |
modifiedapps/diffinery/src/agentic/api.ts+1−1View file
| @@ -130,7 +130,7 @@ | ||
| 130 | 130 | const b = await jsonBody<SettingsInput & { accept_terms?: unknown }>(c); |
| 131 | 131 | const s = await saveReviewSettings(c, l, { ...b, accept: b.accept_terms }); |
| 132 | 132 | const me = requireAccount(c); |
| 133 | - c.executionCtx.waitUntil(c.get("dir").audit({ actorId: me.id, actorLogin: me.login, action: "review.settings", target: full(l), detail: { enabled: s?.enabled ?? false, agent: s?.agent.login, runtime: s?.runtime, trigger: s?.trigger, autofix: s?.autofix } })); | |
| 133 | + c.executionCtx.waitUntil(c.get("dir").audit({ actorId: me.id, actorLogin: me.login, action: "review.settings", target: full(l), detail: { enabled: s?.enabled ?? false, agent: s?.agent.login, runtime: s?.runtime, trigger: s?.trigger, autofix: s?.autofix, daily_limit: s?.dailyLimit } })); | |
| 134 | 134 | return c.json(reviewSettingsJson(s)); |
| 135 | 135 | }); |
| 136 | 136 |
modifiedapps/diffinery/src/agentic/proxy.ts+2−1View file
| @@ -42,7 +42,7 @@ | ||
| 42 | 42 | const DROP = new Set(["authorization", "x-api-key", "cookie", "host", "content-length", "connection", "cf-connecting-ip", "cf-ipcountry", "cf-ray", "cf-visitor", "x-forwarded-for", "x-forwarded-proto", "x-real-ip"]); |
| 43 | 43 | |
| 44 | 44 | const err = (status: number, message: string) => |
| 45 | - new Response(JSON.stringify({ type: "error", error: { type: status === 401 ? "authentication_error" : status === 404 ? "not_found_error" : "permission_error", message } }), { status, headers: { "content-type": "application/json" } }); | |
| 45 | + new Response(JSON.stringify({ type: "error", error: { type: status === 401 ? "authentication_error" : status === 404 ? "not_found_error" : status === 429 ? "rate_limit_error" : "permission_error", message } }), { status, headers: { "content-type": "application/json" } }); | |
| 46 | 46 | |
| 47 | 47 | export function proxyToken(req: Request): string | null { |
| 48 | 48 | const key = req.headers.get("x-api-key"); |
| @@ -65,6 +65,7 @@ | ||
| 65 | 65 | const project = env.PROJECT.get(env.PROJECT.idFromName(repoId)); |
| 66 | 66 | const run = await project.modelAuth(token); |
| 67 | 67 | if (!run) return err(401, "this model token isn't valid: its task run has ended"); |
| 68 | + if ("limit" in run) return err(429, run.limit); | |
| 68 | 69 | if (!run.providers.includes(provider)) return err(403, `this task doesn't use ${PROVIDERS[provider].name}`); |
| 69 | 70 | // The person must still be allowed to run the task, on every request; otherwise the task stops. |
| 70 | 71 | const dir = new Directory(env); |
modifiedapps/diffinery/src/agentic/review-web.tsx+10−3View file
| @@ -8,7 +8,7 @@ | ||
| 8 | 8 | import { page as settingsPage } from "../web/repo-settings.js"; |
| 9 | 9 | import { Form, Time, act, post, prm, repoUrl, statusOf, type WebApp } from "../web/ui.js"; |
| 10 | 10 | import { ModelKeys, PROVIDERS } from "./keys.js"; |
| 11 | -import { AUTHORS, findingId, MAX_AUTOFIXES, TRIGGERS, type ReviewSettings } from "./review.js"; | |
| 11 | +import { AUTHORS, findingId, MAX_AUTOFIXES, PR_DAILY_AUTOMATIC, PR_DAILY_REVIEWS, REPO_DAILY_DEFAULT, REPO_DAILY_MAX, REVIEW_MAX_MODEL_CALLS, REVIEW_TIMEOUT_MS, TRIGGERS, type ReviewSettings } from "./review.js"; | |
| 12 | 12 | import { askForReview, reviewSettings, runtimeOk, saveReviewSettings } from "./reviews.js"; |
| 13 | 13 | import { RUNTIMES, type RuntimeId } from "./runtimes.js"; |
| 14 | 14 | import { drivableAgents } from "./tasks.js"; |
| @@ -128,6 +128,13 @@ | ||
| 128 | 128 | label={<>Autofix: when a review finds blocking problems, start an agent task that fixes them on the pull request's branch (at most {MAX_AUTOFIXES} times per pull request)</>} |
| 129 | 129 | /> |
| 130 | 130 | </div> |
| 131 | + <div class="field"> | |
| 132 | + <label for="daily_limit">Daily limit</label> | |
| 133 | + <input id="daily_limit" name="daily_limit" type="number" min={1} max={REPO_DAILY_MAX} step={1} style="max-width:10em" value={v("daily_limit", String(s.dailyLimit ?? REPO_DAILY_DEFAULT))} /> | |
| 134 | + <div class="hint"> | |
| 135 | + The most reviews this repository runs in 24 hours. When it is reached, pull requests get an action-required check instead of a review, and you get an email. Each pull request also gets at most {PR_DAILY_AUTOMATIC} automatic reviews and {PR_DAILY_REVIEWS} in all a day, and each review stops after {REVIEW_TIMEOUT_MS / 60_000} minutes or {REVIEW_MAX_MODEL_CALLS} model requests. | |
| 136 | + </div> | |
| 137 | + </div> | |
| 131 | 138 | <div class="flash info"> |
| 132 | 139 | Every review on this repository, automatic or asked for by anyone with write access, runs with <b>your</b> model key{youPay ? "" : `, instead of ${current!.person.login}'s`}. Anything the agent reads, including code in pull requests, can try to trick it ("prompt injection"); its sandbox is read-only, but it can use your model access while a review runs. Use a dedicated key with a spending limit. |
| 133 | 140 | </div> |
| @@ -159,11 +166,11 @@ | ||
| 159 | 166 | |
| 160 | 167 | post(web, "/:owner/:repo/settings/agent-review", async (c, f) => { |
| 161 | 168 | const l = await admin(c); |
| 162 | - const values = { agent: f.s("agent"), runtime: f.s("runtime") || "claude-code", command: f.s("command"), trigger: f.s("trigger") || "push", authors: f.s("authors") || "all", drafts: f.all("drafts").length ? "1" : "", autofix: f.all("autofix").length ? "1" : "" }; | |
| 169 | + const values = { agent: f.s("agent"), runtime: f.s("runtime") || "claude-code", command: f.s("command"), trigger: f.s("trigger") || "push", authors: f.s("authors") || "all", drafts: f.all("drafts").length ? "1" : "", autofix: f.all("autofix").length ? "1" : "", daily_limit: f.s("daily_limit") }; | |
| 163 | 170 | try { |
| 164 | 171 | const s = await saveReviewSettings(c, l, { ...values, drafts: !!values.drafts, autofix: !!values.autofix, accept: f.all("accept").length > 0 }); |
| 165 | 172 | const me = requireAccount(c); |
| 166 | - c.executionCtx.waitUntil(c.get("dir").audit({ actorId: me.id, actorLogin: me.login, action: "review.settings", target: `${l.repo.owner_login}/${l.repo.name}`, detail: { enabled: true, agent: s?.agent.login, runtime: s?.runtime, trigger: s?.trigger, autofix: s?.autofix } })); | |
| 173 | + c.executionCtx.waitUntil(c.get("dir").audit({ actorId: me.id, actorLogin: me.login, action: "review.settings", target: `${l.repo.owner_login}/${l.repo.name}`, detail: { enabled: true, agent: s?.agent.login, runtime: s?.runtime, trigger: s?.trigger, autofix: s?.autofix, daily_limit: s?.dailyLimit } })); | |
| 167 | 174 | } catch (e) { |
| 168 | 175 | const st = statusOf(e); |
| 169 | 176 | if (st.status >= 500) throw e; |
modifiedapps/diffinery/src/agentic/review.ts+51−0View file
| @@ -26,6 +26,10 @@ | ||
| 26 | 26 | drafts: boolean; |
| 27 | 27 | authors: ReviewAuthors; |
| 28 | 28 | autofix: boolean; |
| 29 | + /** Most reviews the repository runs in 24 hours, set by the payer (REPO_DAILY_DEFAULT when unset). */ | |
| 30 | + dailyLimit?: number; | |
| 31 | + /** When the payer was last emailed that the daily limit was reached. */ | |
| 32 | + limitMailedAt?: string; | |
| 29 | 33 | optedInAt: string; |
| 30 | 34 | updatedAt: string; |
| 31 | 35 | } |
| @@ -37,6 +41,53 @@ | ||
| 37 | 41 | export const MAX_INLINE_FINDINGS = 25; |
| 38 | 42 | export const MAX_AUTOFIXES = 3; |
| 39 | 43 | |
| 44 | +/** | |
| 45 | + * Spend limits, enforced by Diffinery rather than the agent: pull request | |
| 46 | + * content can talk an agent into anything, so these bound what one review, | |
| 47 | + * one pull request and one repository can cost the payer. | |
| 48 | + */ | |
| 49 | +/** A review's wall-clock limit, from its start; the model proxy refuses it after this too. */ | |
| 50 | +export const REVIEW_TIMEOUT_MS = 30 * 60 * 1000; | |
| 51 | +/** Model requests (turns) one review may make through the proxy. */ | |
| 52 | +export const REVIEW_MAX_MODEL_CALLS = 200; | |
| 53 | +/** Automatic reviews of one pull request in 24 hours; more wait for someone to ask. */ | |
| 54 | +export const PR_DAILY_AUTOMATIC = 10; | |
| 55 | +/** Reviews of one pull request in 24 hours, asked for or automatic. */ | |
| 56 | +export const PR_DAILY_REVIEWS = 20; | |
| 57 | +export const REPO_DAILY_DEFAULT = 50; | |
| 58 | +export const REPO_DAILY_MAX = 1000; | |
| 59 | +const DAY_MS = 24 * 60 * 60 * 1000; | |
| 60 | + | |
| 61 | +export interface ReviewCounts { | |
| 62 | + /** Review runs started in the last 24 hours: in the repository, on this pull request, and automatic ones on it. */ | |
| 63 | + repo: number; | |
| 64 | + pr: number; | |
| 65 | + prAutomatic: number; | |
| 66 | +} | |
| 67 | + | |
| 68 | +export type ReviewLimit = { scope: "repo" | "pr"; title: string; summary: string }; | |
| 69 | + | |
| 70 | +/** Which daily limit, if any, keeps a review from starting now. */ | |
| 71 | +export function reviewLimit(counts: ReviewCounts, automatic: boolean, settings: Pick<ReviewSettings, "dailyLimit" | "person">): ReviewLimit | null { | |
| 72 | + const repoLimit = settings.dailyLimit ?? REPO_DAILY_DEFAULT; | |
| 73 | + if (counts.repo >= repoLimit) | |
| 74 | + return { | |
| 75 | + scope: "repo", | |
| 76 | + title: "Daily review limit reached", | |
| 77 | + summary: `This repository ran ${counts.repo} agent reviews in the last 24 hours, its limit of ${repoLimit}. ${settings.person.login}, whose key reviews use, can raise it under Settings → Agent review. Ask for a review again later.`, | |
| 78 | + }; | |
| 79 | + if (counts.pr >= PR_DAILY_REVIEWS) return { scope: "pr", title: "Daily review limit reached for this pull request", summary: `This pull request had ${counts.pr} agent reviews in the last 24 hours, the most one pull request gets. Ask for a review again later.` }; | |
| 80 | + if (automatic && counts.prAutomatic >= PR_DAILY_AUTOMATIC) | |
| 81 | + return { scope: "pr", title: "Waiting for a request", summary: `This pull request had ${counts.prAutomatic} automatic reviews in the last 24 hours, the most it gets. Someone with write access can still ask for a review.` }; | |
| 82 | + return null; | |
| 83 | +} | |
| 84 | + | |
| 85 | +/** The start of the 24 hours the daily limits count, as stored timestamps compare. */ | |
| 86 | +export const dayAgo = (at = Date.now()): string => new Date(at - DAY_MS).toISOString(); | |
| 87 | + | |
| 88 | +/** Whether to email the payer that the daily limit was reached: once per 24 hours. */ | |
| 89 | +export const shouldMailLimit = (settings: Pick<ReviewSettings, "limitMailedAt">, at = Date.now()): boolean => !settings.limitMailedAt || Date.parse(settings.limitMailedAt) <= at - DAY_MS; | |
| 90 | + | |
| 40 | 91 | export const TRIGGERS: Record<ReviewTrigger, string> = { |
| 41 | 92 | push: "Every pull request, when it opens and on every push", |
| 42 | 93 | open: "Once per pull request, when it opens or is marked ready for review", |
modifiedapps/diffinery/src/agentic/reviews.ts+9−3View file
| @@ -8,7 +8,7 @@ | ||
| 8 | 8 | import { taskCaller, TaskAccessError } from "./access.js"; |
| 9 | 9 | import { ModelKeys, PROVIDERS, type Provider } from "./keys.js"; |
| 10 | 10 | import type { FindingRow } from "./ProjectDO.js"; |
| 11 | -import { asksForReview, REVIEW_APP, REVIEW_CHECK, REVIEW_CREATOR, TRIGGERS, AUTHORS, type ReviewAuthors, type ReviewSettings, type ReviewTrigger } from "./review.js"; | |
| 11 | +import { asksForReview, REPO_DAILY_DEFAULT, REPO_DAILY_MAX, REVIEW_APP, REVIEW_CHECK, REVIEW_CREATOR, TRIGGERS, AUTHORS, type ReviewAuthors, type ReviewSettings, type ReviewTrigger } from "./review.js"; | |
| 12 | 12 | import { isRuntime, MAX_COMMAND, RUNTIMES, type RuntimeId } from "./runtimes.js"; |
| 13 | 13 | import { projectStub, requirePersonSession } from "./tasks.js"; |
| 14 | 14 | |
| @@ -39,6 +39,7 @@ | ||
| 39 | 39 | drafts?: unknown; |
| 40 | 40 | authors?: unknown; |
| 41 | 41 | autofix?: unknown; |
| 42 | + daily_limit?: unknown; | |
| 42 | 43 | accept?: unknown; |
| 43 | 44 | } |
| 44 | 45 | |
| @@ -77,6 +78,8 @@ | ||
| 77 | 78 | for (const p of RUNTIMES[runtime].needs) { |
| 78 | 79 | if (!have.has(p)) throw new HttpError(422, `${RUNTIMES[runtime].name} needs your ${PROVIDERS[p].name} API key: add it in Settings → Model access`); |
| 79 | 80 | } |
| 81 | + const dailyLimit = input.daily_limit === undefined || input.daily_limit === "" ? REPO_DAILY_DEFAULT : Number(input.daily_limit); | |
| 82 | + if (!Number.isInteger(dailyLimit) || dailyLimit < 1 || dailyLimit > REPO_DAILY_MAX) throw new HttpError(422, `\`daily_limit\` must be a whole number from 1 to ${REPO_DAILY_MAX}`); | |
| 80 | 83 | if (input.accept !== true) throw new HttpError(422, "confirm that every agent review on this repository, including ones other people ask for, uses your model key"); |
| 81 | 84 | const at = now(); |
| 82 | 85 | const settings: ReviewSettings = { |
| @@ -89,6 +92,7 @@ | ||
| 89 | 92 | drafts: input.drafts === true, |
| 90 | 93 | authors, |
| 91 | 94 | autofix: input.autofix === true, |
| 95 | + dailyLimit, | |
| 92 | 96 | optedInAt: at, |
| 93 | 97 | updatedAt: at, |
| 94 | 98 | }; |
| @@ -96,7 +100,7 @@ | ||
| 96 | 100 | return settings; |
| 97 | 101 | } |
| 98 | 102 | |
| 99 | -export type ReviewOutcome = { status: "started" | "queued" | "skipped"; task?: number; reason?: string }; | |
| 103 | +export type ReviewOutcome = { status: "started" | "queued" | "skipped" | "limited"; task?: number; reason?: string }; | |
| 100 | 104 | |
| 101 | 105 | /** Review `pr` now (or after its current review), as the repository's settings say. */ |
| 102 | 106 | export async function startReview(env: Env, dir: Directory, repo: Repo, pr: number, requestedBy: Account | null): Promise<ReviewOutcome> { |
| @@ -127,7 +131,7 @@ | ||
| 127 | 131 | if (missing.length) return blocked(`${settings.person.login}'s ${missing.map((p) => PROVIDERS[p].name).join(" and ")} API key is gone; they can add it in Settings → Model access`); |
| 128 | 132 | const providers: Provider[] = settings.runtime === "custom" ? [...have] : RUNTIMES[settings.runtime].needs; |
| 129 | 133 | const r = await projectStub(env, repo.id).requestReview({ repoId: repo.id, pr, head: issue.pull.head, base: issue.pull.base, headSha: head, settings, providers, requestedBy: requestedBy?.login ?? null }); |
| 130 | - return { status: r.status, task: r.task, ...(r.status === "skipped" ? { reason: "this head was already reviewed" } : {}) }; | |
| 134 | + return { status: r.status, task: r.task, ...(r.status === "skipped" ? { reason: "this head was already reviewed" } : r.reason ? { reason: r.reason } : {}) }; | |
| 131 | 135 | } |
| 132 | 136 | |
| 133 | 137 | /** A person with write access asks for a review (button, REST). */ |
| @@ -140,6 +144,7 @@ | ||
| 140 | 144 | const settings = await reviewSettings(c.env, l.repo.id); |
| 141 | 145 | if (!settings?.enabled) throw new HttpError(422, "agent review isn't turned on for this repository: an admin can turn it on under Settings → Agent review"); |
| 142 | 146 | const out = await startReview(c.env, c.get("dir"), l.repo, pr, me); |
| 147 | + if (out.status === "limited") throw new HttpError(429, out.reason ?? "a daily review limit was reached"); | |
| 143 | 148 | if (out.status === "skipped") throw new HttpError(422, out.reason ?? "the review couldn't start"); |
| 144 | 149 | c.executionCtx.waitUntil(c.get("dir").audit({ actorId: me.id, actorLogin: me.login, action: "review.request", target: `${l.repo.owner_login}/${l.repo.name}#${pr}` })); |
| 145 | 150 | return out; |
| @@ -210,6 +215,7 @@ | ||
| 210 | 215 | drafts: s.drafts, |
| 211 | 216 | authors: s.authors, |
| 212 | 217 | autofix: s.autofix, |
| 218 | + daily_limit: s.dailyLimit ?? REPO_DAILY_DEFAULT, | |
| 213 | 219 | opted_in_at: s.optedInAt, |
| 214 | 220 | updated_at: s.updatedAt, |
| 215 | 221 | } |
modifiedapps/diffinery/src/docs/api-meta.ts+3−2View file
| @@ -1400,7 +1400,7 @@ | ||
| 1400 | 1400 | }, |
| 1401 | 1401 | "GET /repos/{owner}/{repo}/diffinery/agent-review": { |
| 1402 | 1402 | summary: "Get agent review settings", |
| 1403 | - description: "Whether agent review is on, which agent reviews with which runtime, whose model key every review uses (`uses_key_of`), when it reviews (`trigger`: `push`, `open` or `request`), which pull requests (`authors`: `all`, `people` or `agents`), `drafts` and `autofix`. `{\"enabled\": false}` when it was never turned on. Needs write access. See /docs/agents/review.", | |
| 1403 | + description: "Whether agent review is on, which agent reviews with which runtime, whose model key every review uses (`uses_key_of`), when it reviews (`trigger`: `push`, `open` or `request`), which pull requests (`authors`: `all`, `people` or `agents`), `drafts`, `autofix` and `daily_limit`. `{\"enabled\": false}` when it was never turned on. Needs write access. See /docs/agents/review.", | |
| 1404 | 1404 | access: "write", |
| 1405 | 1405 | }, |
| 1406 | 1406 | "PUT /repos/{owner}/{repo}/diffinery/agent-review": { |
| @@ -1415,6 +1415,7 @@ | ||
| 1415 | 1415 | authors: ["string", "`all` (default), `people` or `agents`."], |
| 1416 | 1416 | drafts: ["boolean", "Review drafts too (default false)."], |
| 1417 | 1417 | autofix: ["boolean", "Start a fix task on the pull request's branch for blocking findings (default false; at most 3 times per pull request)."], |
| 1418 | + daily_limit: ["integer", "Most reviews the repository runs in 24 hours, 1 to 1000 (default 50). Past it, pull requests get an action-required check and you get an email."], | |
| 1418 | 1419 | accept_terms: ["boolean", "Must be true: reviews use your model key."], |
| 1419 | 1420 | enabled: ["boolean", "`false` turns agent review off."], |
| 1420 | 1421 | }, |
| @@ -1426,7 +1427,7 @@ | ||
| 1426 | 1427 | }, |
| 1427 | 1428 | "POST /repos/{owner}/{repo}/pulls/{number}/diffinery/agent-review": { |
| 1428 | 1429 | summary: "Ask for an agent review", |
| 1429 | - description: "Reviews the pull request now, or once its current review ends, whatever the trigger setting. People with write access only (any of their tokens); agents can't ask. 422 when agent review is off. Audited as `review.request`.", | |
| 1430 | + description: "Reviews the pull request now, or once its current review ends, whatever the trigger setting. People with write access only (any of their tokens); agents can't ask. 422 when agent review is off; 429 past a daily limit (the repository's, or 20 reviews of one pull request in 24 hours). Audited as `review.request`.", | |
| 1430 | 1431 | access: "write", |
| 1431 | 1432 | status: 202, |
| 1432 | 1433 | }, |
modifiedapps/diffinery/src/mail/samples.ts+1−0View file
| @@ -65,6 +65,7 @@ | ||
| 65 | 65 | item("token-expiring", "Access token expiring", { kind: "token_expiring", reason: "security_alert", data: { name: "ci-deploy", prefix: "dfy_Ab12Cd", expiresAt: exp } }), |
| 66 | 66 | item("agent-created", "Agent added", { kind: "agent_created", reason: "security_alert", data: { agent: "review-bot" } }), |
| 67 | 67 | item("model-key-added", "Model key added", { kind: "model_key_added", reason: "security_alert", data: { provider: "Anthropic", hint: "x7Qa" } }), |
| 68 | + item("review-limit", "Agent review limit reached", { kind: "review_limit_reached", reason: "your_activity", data: { repo, limit: 50, pr: 42 } }), | |
| 68 | 69 | item("push-bypassed", "Push protection bypassed", { kind: "push_bypassed", reason: "security_alert", data: { repo, pusher: me, kinds: ["AWS access key"], reason: "Fake key used by a test fixture", refs: ["refs/heads/main"] } }), |
| 69 | 70 | item("push-blocked", "Push blocked", { kind: "push_blocked", reason: "security_alert", data: { repo, kinds: ["GitHub token"], refs: ["refs/heads/feature"] } }), |
| 70 | 71 | item("commit-blocked", "Commit blocked", { kind: "push_blocked", reason: "security_alert", data: { repo, kinds: ["AWS access key"], paths: ["config.env"] } }), |
modifiedapps/diffinery/src/mail/templates.ts+18−0View file
| @@ -453,6 +453,24 @@ | ||
| 453 | 453 | url: page, |
| 454 | 454 | }; |
| 455 | 455 | } |
| 456 | + case "review_limit_reached": { | |
| 457 | + const d = job.data; | |
| 458 | + const page = `${pub}/${d.repo}/settings/agent-review`; | |
| 459 | + return { | |
| 460 | + subject: `[${d.repo}] Agent review reached its daily limit`, | |
| 461 | + preheader: `${d.limit} reviews ran with your model key in the last 24 hours, so new ones wait.`, | |
| 462 | + eyebrow: { text: "Agent review", tone: "amber" }, | |
| 463 | + title: ["Agent review on ", { a: d.repo, href: `${pub}/${d.repo}` }, " reached its daily limit"], | |
| 464 | + blocks: [ | |
| 465 | + { p: [`Agent reviews on this repository use your model key. ${d.limit} ran in the last 24 hours, the daily limit you set, so pull requests get an action-required check instead of a review until older reviews fall out of the 24 hours. Then reviews run again when someone asks or pushes.`] }, | |
| 466 | + { kv: [["Repository", d.repo], ["Daily limit", String(d.limit)], ["Last pull request", [{ a: `#${d.pr}`, href: `${pub}/${d.repo}/pull/${d.pr}` }]], ["When", at(job.at)]] }, | |
| 467 | + { callout: ["If this is more reviews than you expected, look at which pull requests ran them on the Tasks tab. Something may be pushing often, or asking for reviews."], tone: "warn" }, | |
| 468 | + { button: { label: "Agent review settings", url: page } }, | |
| 469 | + ], | |
| 470 | + context: repoContext(pub, d.repo), | |
| 471 | + url: page, | |
| 472 | + }; | |
| 473 | + } | |
| 456 | 474 | case "push_bypassed": { |
| 457 | 475 | const d = job.data; |
| 458 | 476 | return { |
modifiedapps/diffinery/src/mail/types.ts+3−0View file
| @@ -77,6 +77,7 @@ | ||
| 77 | 77 | | { kind: "token_expiring"; data: { name: string; prefix: string; expiresAt: string; agent?: string } } |
| 78 | 78 | | { kind: "agent_created"; data: { agent: string } } |
| 79 | 79 | | { kind: "model_key_added"; data: { provider: string; hint: string } } |
| 80 | + | { kind: "review_limit_reached"; data: { repo: string; limit: number; pr: number } } | |
| 80 | 81 | | { kind: "push_bypassed"; data: { repo: string; pusher: string; kinds: string[]; reason: string; refs?: string[] } } |
| 81 | 82 | | { kind: "push_blocked"; data: { repo: string; kinds: string[]; refs?: string[]; paths?: string[] } } |
| 82 | 83 | | { kind: "account_status"; data: { status: "disabled" | "active" } } |
| @@ -122,6 +123,8 @@ | ||
| 122 | 123 | case "invite": |
| 123 | 124 | case "welcome": |
| 124 | 125 | case "signin_link": |
| 126 | + // It is about the payer's own model spend, so it always sends. | |
| 127 | + case "review_limit_reached": | |
| 125 | 128 | case "test": |
| 126 | 129 | return "account"; |
| 127 | 130 | case "invite_accepted": |
modifiedapps/diffinery/test/e2e/review.test.mjs+53−1View file
| @@ -7,7 +7,8 @@ | ||
| 7 | 7 | // Covers turning reviews on (admin, browser session, opt-in), an automatic |
| 8 | 8 | // review on open with inline findings and the Agent review check, the review's |
| 9 | 9 | // read-only sandbox, autofix pushing to the pull request's branch, the |
| 10 | -// re-review marking the finding fixed, Fix it, and asking for a review. | |
| 10 | +// re-review marking the finding fixed, Fix it, asking for a review, and the | |
| 11 | +// spend limits (model requests per review, the repository's daily limit). | |
| 11 | 12 | import { spawn, execFileSync } from "node:child_process"; |
| 12 | 13 | import { mkdtempSync, writeFileSync, mkdirSync, rmSync, readdirSync, statSync, readFileSync, existsSync } from "node:fs"; |
| 13 | 14 | import { tmpdir } from "node:os"; |
| @@ -383,3 +384,54 @@ | ||
| 383 | 384 | assert.equal(off.status, 422); |
| 384 | 385 | assert.match(off.json.message, /isn't turned on/); |
| 385 | 386 | }); |
| 387 | + | |
| 388 | +test("agent review: spend limits per review and per repository", async () => { | |
| 389 | + const owner = (await ok("GET", "/user")).login; | |
| 390 | + const S = `/repos/${owner}/review-demo/diffinery/agent-review`; | |
| 391 | + const P = `/repos/${owner}/review-demo/pulls/1/diffinery/agent-review`; | |
| 392 | + // A reviewer that keeps calling the model, as a prompt-injected agent might. | |
| 393 | + const script = join(work, "greedy-agent.sh"); | |
| 394 | + writeFileSync(script, `#!/bin/bash\nfor i in $(seq 1 260); do\n curl -s -o /dev/null -w "%{http_code}\\n" -X POST "$ANTHROPIC_BASE_URL/v1/messages" -H "x-api-key: $ANTHROPIC_API_KEY" -H "content-type: application/json" -d '{"model":"m","messages":[]}'\ndone\n`); | |
| 395 | + const settings = { agent: "review-bot", runtime: "custom", command: `bash ${script}`, trigger: "request", accept_terms: true }; | |
| 396 | + | |
| 397 | + // The payer's daily limit is a whole number from 1 to 1000. | |
| 398 | + for (const daily_limit of [0, 1001, 2.5, "x"]) assert.equal((await asPerson("PUT", S, { ...settings, daily_limit })).status, 422, String(daily_limit)); | |
| 399 | + const on = await asPerson("PUT", S, { ...settings, daily_limit: 1000 }); | |
| 400 | + assert.equal(on.status, 200, JSON.stringify(on.json)); | |
| 401 | + assert.equal(on.json.daily_limit, 1000); | |
| 402 | + const page = await (await fetch(`${base}/${owner}/review-demo/settings/agent-review`)).text(); | |
| 403 | + assert.match(page, /name="daily_limit"[^>]*value="1000"/); | |
| 404 | + | |
| 405 | + // Per review: the model proxy stops a review at its limit of model requests, and the check fails. | |
| 406 | + const task = (await reviewOf(owner, 1)).task.number; | |
| 407 | + const runs0 = (await ok("GET", `/repos/${owner}/review-demo/diffinery/tasks/${task}`)).runs.length; | |
| 408 | + const before = modelRequests.length; | |
| 409 | + assert.equal((await api("POST", P, {})).status, 202); | |
| 410 | + const t = await until("the greedy review to stop", async () => { | |
| 411 | + const t = await ok("GET", `/repos/${owner}/review-demo/diffinery/tasks/${task}`); | |
| 412 | + return t.runs.length > runs0 && t.state !== "running" ? t : null; | |
| 413 | + }, 180_000); | |
| 414 | + const run = t.runs.at(-1); | |
| 415 | + assert.equal(run.state, "failed"); | |
| 416 | + assert.match(run.error, /used its limit of 200 model requests/); | |
| 417 | + assert.equal(modelRequests.length - before, 200, "no request past the limit reached the provider"); | |
| 418 | + const head = (await ok("GET", `/repos/${owner}/review-demo/pulls/1`)).head.sha; | |
| 419 | + const check = () => ok("GET", `/repos/${owner}/review-demo/commits/${head}/check-runs`).then((r) => r.check_runs.find((c) => c.name === "Agent review")); | |
| 420 | + assert.equal((await check()).conclusion, "failure"); | |
| 421 | + | |
| 422 | + // Per repository: past the payer's daily limit, a request is refused, the check asks for action, and the payer gets one email. | |
| 423 | + assert.equal((await asPerson("PUT", S, { ...settings, daily_limit: 1 })).status, 200); | |
| 424 | + const limited = await api("POST", P, {}); | |
| 425 | + assert.equal(limited.status, 429, JSON.stringify(limited.json)); | |
| 426 | + assert.match(limited.json.message, /its limit of 1\. .* can raise it/); | |
| 427 | + const c = await check(); | |
| 428 | + assert.equal(c.conclusion, "action_required"); | |
| 429 | + assert.equal(c.output.title, "Daily review limit reached"); | |
| 430 | + const mails = async () => (await ok("GET", "/diffinery/emails?per_page=200")).filter((e) => e.kind === "review_limit_reached"); | |
| 431 | + const mail = await until("the limit email", async () => (await mails())[0]); | |
| 432 | + assert.equal(mail.subject, `[${owner}/review-demo] Agent review reached its daily limit`); | |
| 433 | + assert.equal((await api("POST", P, {})).status, 429); | |
| 434 | + await sleep(3000); | |
| 435 | + assert.equal((await mails()).length, 1, "one email a day"); | |
| 436 | + assert.equal((await ok("GET", `/repos/${owner}/review-demo/diffinery/tasks/${task}`)).runs.length, runs0 + 1, "nothing ran past the limit"); | |
| 437 | +}); |
modifiedapps/diffinery/test/unit/email.test.ts+1−1View file
| @@ -15,7 +15,7 @@ | ||
| 15 | 15 | describe("email templates", () => { |
| 16 | 16 | it("has a sample for every kind of email", () => { |
| 17 | 17 | const kinds = new Set(samples.map((s) => s.job.kind)); |
| 18 | - for (const k of ["signin_new", "passkey_added", "passkey_removed", "token_created", "token_expiring", "agent_created", "model_key_added", "push_bypassed", "push_blocked", "account_status", "invite", "invite_accepted", "welcome", "signin_link", "access_request", "org_membership", "repo_access", "repo_created", "repo_deleted", "repo_transferred", "thread", "run", "test"]) { | |
| 18 | + for (const k of ["signin_new", "passkey_added", "passkey_removed", "token_created", "token_expiring", "agent_created", "model_key_added", "review_limit_reached", "push_bypassed", "push_blocked", "account_status", "invite", "invite_accepted", "welcome", "signin_link", "access_request", "org_membership", "repo_access", "repo_created", "repo_deleted", "repo_transferred", "thread", "run", "test"]) { | |
| 19 | 19 | expect(kinds.has(k as MailJob["kind"]), k).toBe(true); |
| 20 | 20 | } |
| 21 | 21 | }); |
modifiedapps/diffinery/test/unit/review.test.ts+24−1View file
| @@ -1,5 +1,5 @@ | ||
| 1 | 1 | import { describe, expect, it } from "vitest"; |
| 2 | -import { asksForReview, countText, findingComment, findingId, fixGoal, parseReview, placeFindings, reviewBody, reviewOutput, reviewPrompt, rulesFiles, type Finding } from "../../src/agentic/review.js"; | |
| 2 | +import { asksForReview, countText, dayAgo, PR_DAILY_AUTOMATIC, PR_DAILY_REVIEWS, REPO_DAILY_DEFAULT, reviewLimit, shouldMailLimit, findingComment, findingId, fixGoal, parseReview, placeFindings, reviewBody, reviewOutput, reviewPrompt, rulesFiles, type Finding } from "../../src/agentic/review.js"; | |
| 3 | 3 | import type { TranscriptLine } from "../../src/agentic/runtimes.js"; |
| 4 | 4 | |
| 5 | 5 | const finding = (o: Partial<Finding> = {}): Finding => ({ path: "src/a.ts", line: 3, side: "RIGHT", severity: "blocking", title: "Bug", body: "It breaks.", suggestion: null, ...o }); |
| @@ -146,3 +146,26 @@ | ||
| 146 | 146 | expect(g).toMatch(/```\nreturn 0;\n```/); |
| 147 | 147 | }); |
| 148 | 148 | }); |
| 149 | + | |
| 150 | +describe("daily limits", () => { | |
| 151 | + const person = { id: "u1", login: "ayse" }; | |
| 152 | + it("stops at the repository's limit, the payer's choice or the default", () => { | |
| 153 | + expect(reviewLimit({ repo: REPO_DAILY_DEFAULT - 1, pr: 0, prAutomatic: 0 }, true, { person })).toBeNull(); | |
| 154 | + expect(reviewLimit({ repo: REPO_DAILY_DEFAULT, pr: 0, prAutomatic: 0 }, false, { person })).toMatchObject({ scope: "repo", title: "Daily review limit reached" }); | |
| 155 | + expect(reviewLimit({ repo: 3, pr: 0, prAutomatic: 0 }, false, { person, dailyLimit: 3 })?.summary).toMatch(/limit of 3\. ayse, whose key reviews use, can raise it/); | |
| 156 | + expect(reviewLimit({ repo: 2, pr: 0, prAutomatic: 0 }, false, { person, dailyLimit: 3 })).toBeNull(); | |
| 157 | + }); | |
| 158 | + it("limits automatic reviews of one pull request before ones people ask for", () => { | |
| 159 | + const auto = { repo: 0, pr: PR_DAILY_AUTOMATIC, prAutomatic: PR_DAILY_AUTOMATIC }; | |
| 160 | + expect(reviewLimit(auto, true, { person })).toMatchObject({ scope: "pr", title: "Waiting for a request" }); | |
| 161 | + expect(reviewLimit(auto, false, { person })).toBeNull(); | |
| 162 | + expect(reviewLimit({ repo: 0, pr: PR_DAILY_REVIEWS, prAutomatic: 0 }, false, { person })).toMatchObject({ scope: "pr", title: "Daily review limit reached for this pull request" }); | |
| 163 | + }); | |
| 164 | + it("emails the payer at most once a day", () => { | |
| 165 | + const at = Date.parse("2026-10-13T12:00:00Z"); | |
| 166 | + expect(shouldMailLimit({}, at)).toBe(true); | |
| 167 | + expect(shouldMailLimit({ limitMailedAt: "2026-10-13T01:00:00.000Z" }, at)).toBe(false); | |
| 168 | + expect(shouldMailLimit({ limitMailedAt: "2026-10-12T11:59:00.000Z" }, at)).toBe(true); | |
| 169 | + expect(dayAgo(at)).toBe("2026-10-12T12:00:00.000Z"); | |
| 170 | + }); | |
| 171 | +}); |