From 3f4418b1fe3dd9cc0211167aa12c2ffed90193ec Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 22:24:10 -0400 Subject: [PATCH 01/12] chore: add read-only intake probe for design doc 054 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A pure-SQL probe over analyzer_reviewerassignmentapplication answering how much *new* work each reviewer is actually assigned per rolling week — the measurement the reviewer assignment rate limit (doc 054) has to be sized against. Twelve sections: source health, trailing 7/30-day intake per reviewer, peak rolling 7-day window (all history and active-reviewers-only), what-if replays of candidate limits over 90 and 30 days, concurrent cap vs actual weekly intake, and the three sharp edges the design flags — login case, distinct-PR vs row counting, and pull-claim provenance via a NULL snapshot_id. Runs with no dyno and no deploy; heroku pg:psql executes it locally against production: heroku pg:psql -a queueboard-backend -f scripts/probe_054_rate_limit.sql Read-only apart from one temp view it drops at the end. Reviewer logins are pseudonymised by default so output can be pasted into the design doc; flip `\set show_logins 0` to 1 in the file for real logins (heroku pg:psql does not forward psql's -v). Validated against a seeded local Postgres 16 in both login modes, and run against production three times while iterating on the doc. Co-Authored-By: Claude Opus 5 (1M context) --- scripts/probe_054_rate_limit.sql | 364 +++++++++++++++++++++++++++++++ 1 file changed, 364 insertions(+) create mode 100644 scripts/probe_054_rate_limit.sql diff --git a/scripts/probe_054_rate_limit.sql b/scripts/probe_054_rate_limit.sql new file mode 100644 index 00000000..d595f0e6 --- /dev/null +++ b/scripts/probe_054_rate_limit.sql @@ -0,0 +1,364 @@ +-- Design doc 054 measure-first probe: what does reviewer *intake* actually look like? +-- +-- Answers, against live production state and read-only: +-- 1. Is `analyzer_reviewerassignmentapplication` a usable count source at all (volume, history)? +-- 2. What is the trailing 7-day distinct-PR intake per reviewer *right now*? +-- 3. What did the worst rolling 7-day week ever look like, per reviewer? <- picks the limit +-- 4. How many assignments would candidate limits have blocked historically? +-- 5. Do the sharp edges the doc flags actually bite (login case, distinct-vs-rows, provenance)? +-- +-- Read-only: SELECTs only, no dyno needed. +-- +-- heroku pg:psql -a queueboard-backend -f scripts/probe_054_rate_limit.sql +-- +-- Reviewer logins are pseudonymised (first 8 hex of md5) so the output can be pasted into the +-- design doc. To see real logins, change `\set show_logins 0` below to 1 -- `heroku pg:psql` +-- does not forward psql flags like `-v`, so the toggle has to be edited in the file. + +\pset pager off +\set show_logins 0 +\set window_days 7 + +-- Every section reads this one relation: applied rows only, normalised login, repo label. +-- `status='applied' AND applied_at IS NOT NULL` is exactly the population the proposed +-- `recent_assignment_counts()` service would count. +CREATE TEMP VIEW p054_applied AS +SELECT a.repository_id, + r.owner || '/' || r.name AS repo, + lower(btrim(a.reviewer_login)) AS login, + CASE WHEN :show_logins = 1 THEN lower(btrim(a.reviewer_login)) + ELSE substr(md5(lower(btrim(a.reviewer_login))), 1, 8) END AS who, + a.pr_number, + a.applied_at, + a.run_date, + a.snapshot_id +FROM analyzer_reviewerassignmentapplication a +JOIN core_repository r ON r.id = a.repository_id +WHERE a.status = 'applied' AND a.applied_at IS NOT NULL; + +\echo '' +\echo '=== 1. is the count source alive? (per repo, all statuses) ===' +SELECT r.owner || '/' || r.name AS repo, + count(*) AS rows_all, + count(*) FILTER (WHERE a.status = 'applied') AS applied, + count(DISTINCT a.pr_number) FILTER (WHERE a.status = 'applied') AS distinct_prs, + count(DISTINCT lower(btrim(a.reviewer_login))) + FILTER (WHERE a.status = 'applied') AS distinct_reviewers, + min(a.run_date) AS first_run_date, + max(a.run_date) AS last_run_date, + count(DISTINCT a.run_date) FILTER (WHERE a.status = 'applied') AS days_with_intake +FROM analyzer_reviewerassignmentapplication a +JOIN core_repository r ON r.id = a.repository_id +GROUP BY 1 ORDER BY 1; + +\echo '' +\echo '--- 1b. status mix + recency (a thin/short history means the numbers below are weak) ---' +SELECT status, + count(*) AS all_time, + count(*) FILTER (WHERE created_at > now() - interval '30 days') AS last_30d, + count(*) FILTER (WHERE created_at > now() - interval '7 days') AS last_7d +FROM analyzer_reviewerassignmentapplication +GROUP BY 1 ORDER BY 2 DESC; + +\echo '' +\echo '--- 1c. CORRECTNESS: applied rows with a NULL applied_at would be invisible to the gate ---' +SELECT count(*) AS applied_but_no_applied_at +FROM analyzer_reviewerassignmentapplication +WHERE status = 'applied' AND applied_at IS NULL; + +\echo '' +\echo '=== 2. trailing 7-day intake per reviewer -- THE headline number ===' +\echo '--- distinct_prs is what the proposed gate counts; rows shows the re-assign inflation ---' +SELECT repo, + who, + count(DISTINCT pr_number) AS distinct_prs_7d, + count(*) AS rows_7d, + min(applied_at)::date AS first_in_window, + max(applied_at)::date AS last_in_window +FROM p054_applied +WHERE applied_at > now() - (:window_days * interval '1 day') +GROUP BY 1, 2 +ORDER BY 3 DESC, 1, 2; + +\echo '' +\echo '--- 2b. same, 30-day view (context: is this week typical?) ---' +SELECT repo, who, + count(DISTINCT pr_number) AS distinct_prs_30d, + count(DISTINCT run_date) AS active_days_30d, + round(count(DISTINCT pr_number) / 30.0 * 7, 1) AS implied_per_week +FROM p054_applied +WHERE applied_at > now() - interval '30 days' +GROUP BY 1, 2 ORDER BY 3 DESC, 1, 2; + +\echo '' +\echo '=== 3. peak rolling 7-day window per reviewer, over all history ===' +\echo '--- max distinct PRs in ANY 7-day window: a limit below this would have bound ---' +WITH anchored AS ( + SELECT a.repo, a.who, a.login, a.repository_id, a.applied_at, + (SELECT count(DISTINCT b.pr_number) + FROM p054_applied b + WHERE b.repository_id = a.repository_id + AND b.login = a.login + AND b.applied_at <= a.applied_at + AND b.applied_at > a.applied_at - (:window_days * interval '1 day')) AS window_count + FROM p054_applied a +) +SELECT repo, who, + max(window_count) AS peak_7d, + round(avg(window_count), 1) AS avg_7d_when_active, + count(DISTINCT applied_at::date) AS active_days_all_time, + max(applied_at)::date AS last_intake +FROM anchored +GROUP BY 1, 2 ORDER BY 3 DESC, 1, 2; + +\echo '' +\echo '--- 3b. distribution of that peak across reviewers (where a cap would start to bite) ---' +\echo '--- the gate allows exactly N per window, so a reviewer is blocked only when peak > N ---' +WITH anchored AS ( + SELECT a.login, a.repository_id, + (SELECT count(DISTINCT b.pr_number) + FROM p054_applied b + WHERE b.repository_id = a.repository_id + AND b.login = a.login + AND b.applied_at <= a.applied_at + AND b.applied_at > a.applied_at - (:window_days * interval '1 day')) AS window_count + FROM p054_applied a +), peaks AS ( + SELECT repository_id, login, max(window_count) AS peak FROM anchored GROUP BY 1, 2 +) +SELECT count(*) AS reviewers_with_intake, + min(peak) AS min_peak, + round(percentile_cont(0.5) WITHIN GROUP (ORDER BY peak)::numeric, 1) AS p50_peak, + round(percentile_cont(0.9) WITHIN GROUP (ORDER BY peak)::numeric, 1) AS p90_peak, + max(peak) AS max_peak, + count(*) FILTER (WHERE peak > 3) AS would_block_at_3, + count(*) FILTER (WHERE peak > 5) AS would_block_at_5, + count(*) FILTER (WHERE peak > 8) AS would_block_at_8, + count(*) FILTER (WHERE peak > 10) AS would_block_at_10 +FROM peaks; + +\echo '' +\echo '--- 3c. same peak, but only reviewers active in the last 30 days ---' +\echo '--- anchors are recent; the window still counts across the full history, so no truncation ---' +WITH anchored AS ( + SELECT a.repository_id, a.login, + (SELECT count(DISTINCT b.pr_number) + FROM p054_applied b + WHERE b.repository_id = a.repository_id + AND b.login = a.login + AND b.applied_at <= a.applied_at + AND b.applied_at > a.applied_at - (:window_days * interval '1 day')) AS window_count + FROM p054_applied a + WHERE a.applied_at > now() - interval '30 days' +), peaks AS ( + SELECT repository_id, login, max(window_count) AS peak FROM anchored GROUP BY 1, 2 +) +SELECT count(*) AS reviewers_active_30d, + min(peak) AS min_peak, + round(percentile_cont(0.5) WITHIN GROUP (ORDER BY peak)::numeric, 1) AS p50_peak, + round(percentile_cont(0.9) WITHIN GROUP (ORDER BY peak)::numeric, 1) AS p90_peak, + max(peak) AS max_peak, + count(*) FILTER (WHERE peak > 3) AS would_block_at_3, + count(*) FILTER (WHERE peak > 5) AS would_block_at_5, + count(*) FILTER (WHERE peak > 8) AS would_block_at_8, + count(*) FILTER (WHERE peak > 10) AS would_block_at_10 +FROM peaks; + +\echo '' +\echo '=== 4. what-if: how much would candidate limits have withheld? (last 90 days) ===' +\echo '--- UPPER BOUND: blocking an assignment would also lower later window counts ---' +WITH prior AS ( + SELECT a.repo, a.login, a.applied_at, + (SELECT count(DISTINCT b.pr_number) + FROM p054_applied b + WHERE b.repository_id = a.repository_id + AND b.login = a.login + AND b.applied_at < a.applied_at + AND b.applied_at > a.applied_at - (:window_days * interval '1 day') + AND b.pr_number <> a.pr_number) AS prior_distinct + FROM p054_applied a + WHERE a.applied_at > now() - interval '90 days' +) +SELECT l.lim AS limit_per_week, + count(*) AS assignments_90d, + count(*) FILTER (WHERE prior_distinct >= l.lim) AS would_be_blocked, + round(100.0 * count(*) FILTER (WHERE prior_distinct >= l.lim) / nullif(count(*), 0), 1) + AS pct_blocked, + count(DISTINCT login) FILTER (WHERE prior_distinct >= l.lim) AS reviewers_affected, + count(DISTINCT login) AS reviewers_total +FROM prior CROSS JOIN (VALUES (2), (3), (5), (8), (10), (15)) AS l(lim) +GROUP BY 1 ORDER BY 1; + +\echo '' +\echo '--- 4b. same replay, last 30 days only (the 90d view spans the apply rollout) ---' +\echo '--- anchors are recent; each window still counts across the full history ---' +WITH prior AS ( + SELECT a.login, + (SELECT count(DISTINCT b.pr_number) + FROM p054_applied b + WHERE b.repository_id = a.repository_id + AND b.login = a.login + AND b.applied_at < a.applied_at + AND b.applied_at > a.applied_at - (:window_days * interval '1 day') + AND b.pr_number <> a.pr_number) AS prior_distinct + FROM p054_applied a + WHERE a.applied_at > now() - interval '30 days' +) +SELECT l.lim AS limit_per_week, + count(*) AS assignments_30d, + count(*) FILTER (WHERE prior_distinct >= l.lim) AS would_be_blocked, + round(100.0 * count(*) FILTER (WHERE prior_distinct >= l.lim) / nullif(count(*), 0), 1) + AS pct_blocked, + count(DISTINCT login) FILTER (WHERE prior_distinct >= l.lim) AS reviewers_affected, + count(DISTINCT login) AS reviewers_total +FROM prior CROSS JOIN (VALUES (2), (3), (5), (8), (10), (15)) AS l(lim) +GROUP BY 1 ORDER BY 1; + +\echo '' +\echo '=== 5. supply side: reviewers with preferences vs reviewers who get any intake ===' +\echo '--- a limit only matters for reviewers the push actually reaches ---' +WITH intake AS ( + SELECT repository_id, login, + count(DISTINCT pr_number) FILTER (WHERE applied_at > now() - (:window_days * interval '1 day')) AS d7, + count(DISTINCT pr_number) FILTER (WHERE applied_at > now() - interval '30 days') AS d30 + FROM p054_applied GROUP BY 1, 2 +) +SELECT r.owner || '/' || r.name AS repo, + count(*) AS prefs, + count(*) FILTER (WHERE p.auto_assign) AS auto_assign_on, + count(*) FILTER (WHERE i.d30 > 0) AS got_intake_30d, + count(*) FILTER (WHERE i.d7 > 0) AS got_intake_7d, + round(avg(p.maximum_capacity), 1) AS avg_max_capacity, + count(*) FILTER (WHERE p.assignment_acceptance = 'confirm') AS confirm_mode +FROM core_reviewerpreference p +JOIN core_repository r ON r.id = p.repository_id +LEFT JOIN core_user u ON u.id = p.user_id +LEFT JOIN intake i ON i.repository_id = p.repository_id + AND i.login = lower(btrim(coalesce(u.github_login, ''))) +GROUP BY 1 ORDER BY 1; + +\echo '' +\echo '--- 5b. per-reviewer: concurrent cap vs actual weekly intake (which gate binds first) ---' +WITH intake AS ( + SELECT repository_id, login, + count(DISTINCT pr_number) FILTER (WHERE applied_at > now() - (:window_days * interval '1 day')) AS d7, + count(DISTINCT pr_number) FILTER (WHERE applied_at > now() - interval '30 days') AS d30 + FROM p054_applied GROUP BY 1, 2 +) +SELECT r.owner || '/' || r.name AS repo, + CASE WHEN :show_logins = 1 THEN lower(btrim(u.github_login)) + ELSE substr(md5(lower(btrim(coalesce(u.github_login, '')))), 1, 8) END AS who, + p.maximum_capacity, + p.auto_assign, + p.assignment_acceptance, + (p.away_until IS NOT NULL AND p.away_until > now()) AS away_now, + coalesce(i.d7, 0) AS intake_7d, + coalesce(i.d30, 0) AS intake_30d +FROM core_reviewerpreference p +JOIN core_repository r ON r.id = p.repository_id +LEFT JOIN core_user u ON u.id = p.user_id +LEFT JOIN intake i ON i.repository_id = p.repository_id + AND i.login = lower(btrim(coalesce(u.github_login, ''))) +WHERE coalesce(i.d30, 0) > 0 +ORDER BY coalesce(i.d30, 0) DESC, 1 +LIMIT 60; + +\echo '' +\echo '=== 6. SHARP EDGE -- login case: is reviewer_login stored normalised? ===' +\echo '--- assign_reviewer_and_record stores the login verbatim; a case mismatch UNDERCOUNTS ---' +SELECT count(*) AS applied_rows, + count(*) FILTER (WHERE reviewer_login <> lower(reviewer_login)) AS not_lowercase, + count(*) FILTER (WHERE reviewer_login <> btrim(reviewer_login)) AS has_whitespace, + count(DISTINCT reviewer_login) AS distinct_raw, + count(DISTINCT lower(btrim(reviewer_login))) AS distinct_normalised +FROM analyzer_reviewerassignmentapplication +WHERE status = 'applied'; + +\echo '' +\echo '--- 6b. logins stored under more than one spelling (each row = a split count) ---' +SELECT lower(btrim(reviewer_login)) AS normalised, + count(DISTINCT reviewer_login) AS spellings, + string_agg(DISTINCT reviewer_login, ' | ') AS variants +FROM analyzer_reviewerassignmentapplication +WHERE status = 'applied' +GROUP BY 1 HAVING count(DISTINCT reviewer_login) > 1 +ORDER BY 2 DESC; + +\echo '' +\echo '--- 6c. applied logins that do not match any core_user.github_login (case-insensitive) ---' +SELECT a.who, count(*) AS applied_rows, max(a.applied_at)::date AS last_seen +FROM p054_applied a +LEFT JOIN core_user u ON lower(btrim(coalesce(u.github_login, ''))) = a.login +WHERE u.id IS NULL +GROUP BY 1 ORDER BY 2 DESC; + +\echo '' +\echo '--- 6d. how many REVIEWERS (not rows) carry a capitalized spelling? ---' +\echo '--- a case-sensitive query filter would silently exempt exactly these from the gate ---' +\echo '--- (spellings == reviewers only while 6b is empty; check that first) ---' +SELECT count(*) AS spellings_total, + count(*) FILTER (WHERE spelling <> lower(spelling)) AS spellings_capitalized, + coalesce(sum(n) FILTER (WHERE spelling <> lower(spelling)), 0) AS their_applied_rows +FROM ( + SELECT reviewer_login AS spelling, count(*) AS n + FROM analyzer_reviewerassignmentapplication + WHERE status = 'applied' + GROUP BY 1 +) t; + +\echo '' +\echo '=== 7. SHARP EDGE -- distinct PRs vs rows: how much re-assignment churn is there? ===' +\echo '--- PRs with >1 applied row for the same reviewer; row-counting would double-count these ---' +SELECT count(*) AS pr_reviewer_pairs, + count(*) FILTER (WHERE n > 1) AS pairs_reassigned, + coalesce(sum(n - 1), 0) AS extra_rows_distinct_avoids, + max(n) AS max_rows_for_one_pair +FROM ( + SELECT repository_id, login, pr_number, count(*) AS n + FROM p054_applied GROUP BY 1, 2, 3 +) t; + +\echo '' +\echo '=== 8. provenance proxy: on-demand claims (053) vs nightly/confirm intake ===' +\echo '--- snapshot_id IS NULL == console pull-claim (053 passes snapshot=None). Open Question 4 ---' +SELECT repo, + count(*) AS applied_30d, + count(*) FILTER (WHERE snapshot_id IS NULL) AS claim_like, + count(*) FILTER (WHERE snapshot_id IS NOT NULL) AS snapshot_anchored, + count(DISTINCT login) FILTER (WHERE snapshot_id IS NULL) AS claiming_reviewers +FROM p054_applied +WHERE applied_at > now() - interval '30 days' +GROUP BY 1 ORDER BY 1; + +\echo '' +\echo '--- 8b. per reviewer, last 30 days (would excluding claims change anyone materially?) ---' +SELECT repo, who, + count(DISTINCT pr_number) AS distinct_prs_30d, + count(DISTINCT pr_number) FILTER (WHERE snapshot_id IS NULL) AS claim_like, + count(DISTINCT pr_number) FILTER (WHERE snapshot_id IS NOT NULL) AS snapshot_anchored +FROM p054_applied +WHERE applied_at > now() - interval '30 days' +GROUP BY 1, 2 +HAVING count(*) FILTER (WHERE snapshot_id IS NULL) > 0 +ORDER BY 4 DESC, 3 DESC; + +\echo '' +\echo '=== 9. per-day intake, last 21 days (burstiness: is a single night already clustered?) ===' +SELECT run_date, + count(DISTINCT pr_number) AS distinct_prs, + count(*) AS rows_total, + count(DISTINCT login) AS reviewers, + round(count(DISTINCT pr_number)::numeric / nullif(count(DISTINCT login), 0), 1) AS prs_per_reviewer +FROM p054_applied +WHERE run_date > current_date - 21 +GROUP BY 1 ORDER BY 1 DESC; + +\echo '' +\echo '--- 9b. worst single day per reviewer (Subtlety 6: one run may fill the weekly budget) ---' +SELECT repo, who, run_date, count(DISTINCT pr_number) AS prs_that_day +FROM p054_applied +GROUP BY 1, 2, 3 +ORDER BY 4 DESC, 3 DESC +LIMIT 15; + +DROP VIEW p054_applied; From b40d63f91be7dc4a51411904f126dcf780b8ddda Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 22:24:22 -0400 Subject: [PATCH 02/12] doc: design 054 reviewer assignment rate limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Proposes a rolling weekly cap on *new* assignments per reviewer (ReviewerPreference.max_new_assignments_per_week, opt-in/null by default), additive to maximum_capacity: the existing cap bounds stock, not flow, so it only limits reviewers who don't act. One reviewer-facing number that is both the throughput limit and the smoother; catch-up stays the pull side's job (doc 053 overrides it like every other push throttle). Decisions closed in review: window length 7 days; no ANALYZER_ASSIGNMENT_RATE_LIMIT_ENABLED flag — unlike 046/050/053 this adds no behavior of its own, the opt-in default is the off switch, and the rollback is clearing the pilot cohort's limits. Includes a Measured Baseline from scripts/probe_054_rate_limit.sql against production (mathlib4, 839 applied rows since 2026-06-23), which confirms the premise and re-sized several claims: - maximum_capacity does not bound flow: three reviewers capped at 10 concurrent took 22-30 new PRs in 30 days. - 5/week is the median active reviewer's *worst* week — a no-op for the median, binding for 13 of 32 active reviewers, withholding ~30% of their intake. Right size for an opt-in knob, wrong size for a global default. - Login case is a live defect risk: 11 of 41 reviewers are stored capitalized, so a case-sensitive count returns zero for them and their limit would silently never fire. lower() on both sides is load-bearing. - Distinct-PR counting is insurance: zero re-assignment churn in 67 days. - Single-night clustering is 1-2 PRs, not the burst Subtlety 6 tolerates. Excluding the rollout period lowers the affected headcount but *raises* the withheld share (26.9% -> 29.6%), because recent intake is more concentrated — recorded explicitly, since it is easy to get backwards. Design only; no implementation yet. Co-Authored-By: Claude Opus 5 (1M context) --- .../054-assignment-rate-limit.md | 708 ++++++++++++++++++ 1 file changed, 708 insertions(+) create mode 100644 docs/design-decisions/054-assignment-rate-limit.md diff --git a/docs/design-decisions/054-assignment-rate-limit.md b/docs/design-decisions/054-assignment-rate-limit.md new file mode 100644 index 00000000..e5edf2a3 --- /dev/null +++ b/docs/design-decisions/054-assignment-rate-limit.md @@ -0,0 +1,708 @@ +# Reviewer Assignment Rate Limit (Rolling Weekly Intake Cap) + +> Status: **Draft / Proposed** (2026-08-28) — design only, no implementation yet. The measurement +> probe has been run against production; see [Measured Baseline](#measured-baseline-2026-08-28), +> which confirms the premise and re-sizes several claims. Written to be reviewed before code lands. Origin: a Zulip thread (Christian Merten, with +> Yaël Dillies' earlier proposal and Bryan Gin-ge Chen) on making reviewer capacity limits +> actually bind. + +## Context + +- The only capacity limit on auto-assignment today is `core.ReviewerPreference.maximum_capacity` + (`qb_site/core/models/reviewer_preference.py:39`, default 10) — a cap on **concurrently** assigned + PRs. It is enforced as a single gate in the pure engine, `_reviewer_candidate_state` + (`qb_site/analyzer/services/reviewer_assignment_engine.py:168-179`): + + ```python + remaining = reviewer.maximum_capacity - current_weight + if remaining > 0 and reviewer.auto_assign and not reviewer.temporary_break: + available.append(reviewer.github_login) + ``` + +- **The bound only bites if you let PRs pile up.** `maximum_capacity` limits *stock*, not *flow*. + A reviewer who acts on newly-assigned PRs quickly frees the slot, and the next nightly run + (`analyzer.refresh_reviewer_assignments` → `analyzer.propose_reviewer_assignments` / + `analyzer.apply_reviewer_assignments`, ~00:30/00:45 UTC) refills them back up to + `maximum_capacity`. The cap therefore only limits reviewers who *don't* act — the opposite of what + a capacity limit should reward. This is Christian Merten's diagnosis, and it is correct — and now + measured: three reviewers with `maximum_capacity=10` took **22, 23 and 30 new PRs in 30 days** + ([Measured Baseline](#measured-baseline-2026-08-28)). The stock cap never bound their flow. + +- The requested fix (Christian's proposal, anticipated as a deferred follow-up in + `050-reviewer-assignment-acceptance-gate.md`: *"Throughput cap (reviewed X PRs in 30 days → pause + auto-assign) — a separate capacity input"*) is to bound **flow**: limit how many *new* PRs a + reviewer is assigned over a rolling period, independent of how fast they clear them. + +- **A rolling-window rate is that limit, and its window length is the smoothing knob.** Christian's + literal proposal is "N new PRs per rolling *month*". Bryan's objection — a reviewer could spend the + whole month's budget in a week, then get nothing for three weeks — is really an objection to the + *window being a month*. Shorten the window to a **week** and the same one-parameter mechanism + becomes smooth by construction: a month's worth of intake cannot fit inside a 7-day window. So the + limit is expressed as a single, human-legible rate: + + > **maximum new assignments per week** — a rolling 7-day cap on distinct newly-assigned PRs. + + This is *one* reviewer-facing number that is simultaneously the throughput limit and the smoother. + It needs no separate "drip" or per-cycle knob (an earlier draft of this doc had a monthly cap plus a + derived per-cycle drip; the weekly window replaces both — see [Alternatives](#alternatives-discarded)). + +- **Catch-up is the pull side's job, not the push side's.** The one thing a short window gives up is + *saving up* — a reviewer back from vacation cannot carry last week's unused budget into a bigger + week. That is intentional: `053-on-demand-assignment-suggestions.md` (deployed 2026-08-27) already + serves exactly that case. A reviewer with spare capacity *right now* runs `suggest-prs` / + `/console/suggestions/` and claims work, and `053` **deliberately overrides every push throttle** + (`maximum_capacity`, `auto_assign`, `away_until`). The whole system then reads cleanly: + - **push side** = a steady, predictable, weekly-smoothed trickle; + - **pull side (`053`)** = on-demand burst / catch-up. + + This rate limit is a fourth push throttle and must be overridden by `053` on the same footing (see + [Interactions](#interactions-with-existing-pipelines)). + +- **A per-reviewer assignment history already exists**, so no new log table is needed. + `analyzer.ReviewerAssignmentApplication` (`046-apply-reviewer-assignments-in-django.md`) is an + append-only, indefinitely-retained audit row per applied assignment + (`qb_site/analyzer/models/reviewer_assignment_application.py`): `status='applied'`, `applied_at`, + `run_date`, `repository`, `pr_number`, `reviewer_login`. It records every **system-mediated** + assignment — the nightly auto direct-assign, confirm-mode accepts, and console pull-claims all go + through the shared `assign_reviewer_and_record` (046) path. The one gap is a raw Zulip `assign` + self-assign, which writes no application row (`053`'s known "audit asymmetry"). + +## Measured Baseline (2026-08-28) + +`scripts/probe_054_rate_limit.sql`, run against production for `leanprover-community/mathlib4` (see +[Measuring first](#measuring-first-the-probe)). Everything here is measured. The design above was +written before any of it; where a number changed a claim, the claim was edited in place and the change +is listed under [What the measurement changed](#what-the-measurement-changed). + +**The count source is healthy.** 859 rows / 839 `applied` over 2026-06-23 → 2026-08-28, covering 600 +distinct PRs and 41 distinct reviewers at ~12.5 applied/day. Intake landed on **all 67 days** of that +span — the nightly apply has no gaps, so a rolling 7-day window is always fully populated. The other +20 rows are `skipped_recently_applied`; there are no failures, and **no `applied` row has a NULL +`applied_at`** (§1c), so the window count this design specifies is well-defined on every row that +exists. + +**Intake today** (§1b, §2): ~83 new assignments in the last 7 days, 348 in the last 30, spread over 25 +and 32 reviewers respectively. The trailing-7-day distribution is skewed: + +| trailing 7d | 14 | 7 | 6 | 5 | 4 | 3 | 2 | 1 | +| --- | --- | --- | --- | --- | --- | --- | --- | --- | +| reviewers | 1 | 1 | 2 | 3 | 1 | 4 | 6 | 7 | + +Median 2, mean 3.3, top 14. Over 30 days the busiest reviewers sustain 5–7/week. + +**The premise, quantified** (§5b) — `maximum_capacity` does not bound flow for anyone who clears +quickly: + +| `maximum_capacity` | new PRs in 30 days | implied /week | +| --- | --- | --- | +| 10 | 30 | 7.0 | +| 20 | 29 | 6.8 | +| 10 | 23 | 5.4 | +| 10 | 22 | 5.1 | +| 20 | 22 (14 of them in the last week alone) | 5.1 | + +**What a limit would cost** (§3b, §3c, §4). Peak rolling 7-day intake per reviewer — how big each +reviewer's worst week has been — over the whole history, and over the 32 reviewers still active in the +last 30 days: + +| population | reviewers | p50 peak | p90 peak | max | blocked at 3 | at 5 | at 8 | at 10 | +| --- | --- | --- | --- | --- | --- | --- | --- | --- | +| all history | 41 | 6 | 12 | 16 | 36 | 26 | 15 | 7 | +| active in the last 30 days | 32 | **5** | 10.9 | 14 | 25 | **13** | 7 | 4 | + +The second row is the one to design against: the all-history figures are inflated by the apply +pipeline's own June–July rollout, and the correction is not cosmetic — at a 5/week limit it halves the +affected population, 26 of 41 down to 13 of 32. + +Replaying the last 90 days as if every reviewer had a limit: + +| limit /week | assignments withheld | share of intake | reviewers who would hit it | +| --- | --- | --- | --- | +| 2 | 573 | 68.3% | 38 | +| 3 | 425 | 50.7% | 36 | +| 5 | 226 | 26.9% | 26 | +| 8 | 79 | 9.4% | 15 | +| 10 | 32 | 3.8% | 7 | +| 15 | 1 | 0.1% | 1 | + +And the same replay over the last 30 days only (§4b), which drops the rollout period: + +| limit /week | assignments withheld | share of intake | reviewers who would hit it | +| --- | --- | --- | --- | +| 2 | 255 | 73.3% | 29 | +| 3 | 185 | 53.2% | 25 | +| 5 | **103** | **29.6%** | 13 | +| 8 | 37 | 10.6% | 7 | +| 10 | 16 | 4.6% | 4 | +| 15 | 0 | 0.0% | 0 | + +**That is higher, not lower, and the reason matters.** The natural reading of §3c — half as many +reviewers blocked once the rollout drops out — is that the limit would cost less in steady state. The +opposite is true: at 5/week the recent 30 days would have withheld **29.6%** of intake against the +90-day view's 26.9%. §3c's halving was a *population* effect (nine stale reviewers leaving the +denominator, carrying their July peaks with them), not a fall in intensity. Intake has become **more +concentrated**, not less: the 13 largest recipients took 222 of the last 348 assignments (64%), so +fewer reviewers hit a 5/week cap while those who do account for a larger share of the flow. (The +probe does not join "blocked" to "largest recipient", so the two sets of 13 are near-certainly but not +provably identical.) + +The redistribution arithmetic survives that, at least in aggregate: withholding 103 assignments over +30 days is ~24/week, against ~66/week of unused headroom among the 19 active reviewers a 5/week cap +would *not* touch (they currently receive ~29 of the ~81 assignments/week). Whether those particular +PRs match those particular reviewers' topics is still the open question, and still needs an engine +simulation rather than history. + +§3b (peak distribution) and §4 (90-day replay) are independent computations, and on the live data they +agree **exactly** on the affected-reviewer count at every limit — 36 / 26 / 15 / 7 — which is the best +internal check the probe offers. Getting them to agree exposed an off-by-one worth stating, because +the implementation has to get it right too: the gate lets a reviewer *reach* their limit +(`recent + simulated < limit`), so a reviewer whose worst week was exactly N is never blocked at N. +§3b originally counted `peak >= N` and disagreed with §4; it now counts `peak > N` (columns renamed +`would_block_at_N`) and the two line up. The 30-day pair reproduces it independently: §3c and §4b +agree at 25 / 13 / 7 / 4. + +That table is easy to over-read, so three things to hold onto: + +- These are **withholdings from a reviewer, not PRs left unassigned.** When the gate removes a + reviewer, the engine offers the PR to the next eligible candidate; whether one exists is a + topic-matching question this history cannot answer. Sizing *that* needs an engine simulation over a + snapshot, not this table. What can be said from the supply side: 37 reviewers have `auto_assign` on, + so a universal 5/week cap would still allow ~185 assignments/week against the ~83/week actually + being made — aggregate headroom is not the binding constraint; topic match might be. +- The counts are an **upper bound** (a blocked assignment would also have lowered later windows), and + they assume **universal adoption**, which is explicitly not the plan. +- §4's replay spans the entire history (younger than 90 days), rollout included — but the obvious + inference, that it therefore overstates steady-state cost, is wrong. §4b measured 29.6% against + §4's 26.9%; see the reversal above. Fewer reviewers affected does not mean less intake withheld. + +**So `≤5/7d` — this doc's running illustration — is well chosen, and for a sharper reason than the +doc originally had.** Five is exactly the median active reviewer's *worst* week: the median reviewer +never trips it, while it binds for the 13 of 32 whose peak runs higher — withholding 29.6% of the +last 30 days' intake, if all 13 had it set. "At most five new PRs a week" +is, empirically, "never take more than a typical colleague's busiest week" — which is a defensible +thing for a knob to mean. That is the right size for something a reviewer opts into deliberately and +the wrong size for a global default (Open Question 3). A limit of 10+ is close to decorative: only 4 +of the 32 active reviewers have crossed it in any rolling week. + +### What the measurement changed + +1. **Login case is a live risk, and the failure mode is worse than an undercount** (§6, §6d). + **11 of the 41 reviewers** — 230 of 839 rows — are stored under a capitalized login: + `build_reviewer_catalog` copies `User.github_login` in its original case + (`reviewer_assignment.py:243-252`), the engine appends it verbatim + (`reviewer_assignment_engine.py:172`), and `assign_reviewer_and_record` writes it unchanged. No + login appears under *two* spellings (41 raw = 41 normalized) and every one resolves to a + `core_user`, so a service filtering `reviewer_login__in=` would not partially + undercount those 11 — it returns **zero** for them, and their weekly gate silently never fires. A + quarter of the reviewer population would appear to have opted into a limit that does nothing. + `lower()` on both sides is load-bearing; its unit test is not optional. +2. **Distinct-PR counting is insurance, not a live correction** (§7). Zero churn in 67 days: 839 + applied rows are 839 distinct `(PR, reviewer)` pairs, at most one row each. Keep the rule — the + attention sweep can produce a repeat and row-counting would then be wrong — but nothing in + production currently depends on it. (The 600-distinct-PRs-to-839-pairs gap is PRs collecting ~1.4 + *different* reviewers, which the distinct rule was never about.) +3. **Single-run clustering is mild** (§9), which is the empirical backing for Subtlety 6. Over the + last three weeks the push delivers 6–21 PRs a night across 4–15 reviewers — 1.0–1.8 per reviewer + per night, recent maximum 4. Every 7–10-per-night case in the history falls in 2026-06-25 → + 2026-07-30, the apply pipeline's own rollout. A nightly drip of 1–2 is already the norm, so the + weekly window needs no intra-week pacing parameter. +4. **Confirm-mode over-proposal is a small-population risk** (§5). 6 of 57 preference rows are + `confirm`, and the three that received anything in 30 days got 8, 3 and 1 PRs. Watch it in the + pilot as planned; it is not shaping v1. +5. **No `053` pull-claims exist yet** (§8). All 348 applied rows in the last 30 days are + snapshot-anchored; zero have a NULL `snapshot_id`. `053` went live 2026-08-27, one day before this + run, so that is the absence of a signal, not evidence — the proxy itself works. Open Question 4 + stays open and should be re-measured once the claim path has real usage. +6. **Reviewers cannot pick a number they cannot see.** Median intake is 2/week while the median *peak* + week is 6; a reviewer setting "max new assignments per week" without those numbers is guessing. + Folded into [Surfacing](#surfacing--extend-the-honest-load-line). + +§3c and §6d were added after the first run to answer exactly these two questions and were run the same +day; their results are folded in above. One refinement is still outstanding: **§4b**, the 90-day +replay restricted to the last 30 days, which will show how much of §4's cost estimate is rollout +residue. It is in the script and has not been run. + +## Goals / Non-Goals + +**Goals** +- Add a per-reviewer, per-repository **rolling weekly cap on new assignments** to the push pipeline, + additive to (not a replacement for) `maximum_capacity`. +- **One legible knob**: a reviewer sets a single number, "max new assignments per week." +- **Opt-in**: no behavior change for any reviewer until they set a limit. +- **Smoothed by construction**: because the window is a week, a reviewer cannot burn a long-horizon + budget in one burst (Bryan's concern) — no separate drip/pacing parameter required. +- Reuse the existing `ReviewerAssignmentApplication` history — no new log table. +- Report the weekly figure on the same surfaces as the concurrent load, so a reviewer can see why the + push went quiet. + +**Non-Goals** +- No change to the pull side (`053`) beyond overriding this limit like the other push throttles. + Catch-up/burst is `053`'s job, not the push pipeline's. +- No change to `maximum_capacity` semantics, the fractional awaiting-author weight, or the assignment + ranking/engine ordering (`037`). +- No separate "drip" / per-cycle pacing parameter, and no second (e.g. monthly) window — the single + weekly window is the whole mechanism. +- No new history/audit model. No `PRTimelineEvent` reader for this feature (see Alternatives). + +## Proposed Design + +### Data model — one new opt-in field + +`core.ReviewerPreference` (`qb_site/core/models/reviewer_preference.py`), alongside +`maximum_capacity`: + +```python +# None = unlimited (no weekly limit); a positive value caps new assignments per rolling week. +max_new_assignments_per_week = models.PositiveIntegerField(null=True, blank=True) +``` + +Editable through the same three surfaces `maximum_capacity` uses — the console preferences form +(`core/forms.py`, `REVIEWER_PREFERENCE_EDITABLE_FIELDS` at `:27`, with a `clean_*` mirroring +`clean_maximum_capacity` at `:211-215`; blank ⇒ `None`), the Django admin, and the +`reviewer-topics.json` importer (`core/services/reviewer_topics_importer.py`). + +### Counting the window — `ReviewerAssignmentApplication`, distinct PRs + +The trailing-window count for `(repository, reviewer_login)` is: + +> the number of **distinct `pr_number`** with an `applied` `ReviewerAssignmentApplication` whose +> `applied_at >= now − ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS`. + +- **Distinct PR, not row count**: a re-cycled PR (auto-unassigned by the attention sweep, then + re-assigned) writes multiple `applied` rows; the limit counts "new PRs", so a PR counts once. +- **Rolling window (default 7 days)**: a true rolling week, not a fixed Mon–Sun bucket, so there is no + week-boundary cliff where everyone's budget resets at once. `ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS` + *defines* what "per week" means; see the note on it in [Operational Notes](#operational-notes). +- **What this counts, precisely**: all *system-mediated* intake — nightly auto-assign, confirm-mode + accepts, and console pull-claims (all write `applied` rows). It does **not** count a raw Zulip + `assign` self-assign (no row is written). See [Subtleties](#subtleties--invariants) for why that is + acceptable, and the one inconsistency it leaves. + +A single service in `qb_site/analyzer/services/` owns this query: + +```python +def recent_assignment_counts( + repository: Repository, logins: Sequence[str], *, window_days: int, now: datetime, +) -> dict[str, int]: ... # normalized login -> distinct applied PR count in the window +``` + +One grouped query over the model's `(repository, pr_number, reviewer_login, status)` index; pure and +unit-testable. Reused by both the engine integration and the load-line surfacing so the two cannot +disagree. + +### The gate — a second condition, in the same place + +Extend the engine's `ReviewerProfile` (`reviewer_assignment_engine.py:15`) with the rate context +(plain data, no ORM — consistent with `037`'s engine/integration split): + +- `weekly_limit: int | None` # `max_new_assignments_per_week`, or None +- `recent_assignment_count: int` # the window count above + +and extend the candidate gate in `_reviewer_candidate_state` so a reviewer is available only when +**both** hold: + +``` +remaining_concurrent > 0 # existing (stock) +AND (weekly_limit is None + OR recent_assignment_count + simulated_this_run < weekly_limit) # new (flow) +``` + +Read the strict `<` carefully: a reviewer with `max_new_assignments_per_week = 5` and four PRs in the +window is still available and receives a fifth; the *sixth* is what the gate blocks. "Max 5 per week" +means at most 5, not at most 4 — the probe's own §3b/§4 cross-check tripped on exactly this +([Measured Baseline](#measured-baseline-2026-08-28)), so the unit test should pin the boundary. + +`simulated_this_run` is the per-reviewer count of assignments the engine has already handed out in +**this** run. The batch loop `run_assignment_simulation` (`:490-582`) already increments a reviewer's +weight on each pick (`:573-577`); increment a parallel `simulated_this_run` counter in the same spot +so a single nightly run cannot exceed the weekly cap. This is a correctness guard, **not** a pacing +knob: without it, the run's DB count wouldn't yet reflect this-run picks and the engine could overrun +the limit. `weekly_limit is None` short-circuits to today's behavior. + +That is the entire mechanism. There is no separate drip: the weekly window *is* the smoother. A single +run may fill up to a reviewer's remaining weekly budget (bounded also by remaining concurrent +capacity), which is bounded, predictable, and within the reviewer's own stated weekly tolerance — see +Subtlety 6. + +### Worked comparison + +Nightly run; reviewer who reviews everything the same day, `maximum_capacity=10`: + +| regime | night 1 | steady state | one week in | failure mode | +| --- | --- | --- | --- | --- | +| today (`maximum_capacity` only) | up to 10 | refilled to 10 nightly | 22–30 / month, measured | unbounded intake | +| monthly cap (`≤30/30d`) | up to 10 | up to 10 | **30 burned, then silent 3 wks** | intra-month burst | +| **weekly rate (`≤5/7d`)** | up to 5 | ≤ 5 in any rolling week | ≤ 5, steady | none — smooth by construction | + +The bottom row is the target: the limit binds regardless of clear-rate, and it binds *smoothly*, from +one number. + +### Surfacing — extend the honest load line + +The concurrent load line (`reviewer_load.format_load_line`, shown by the `assigned-prs` Zulip command, +the daily attention DM, and the console) gains the weekly figure, e.g.: + +``` +Load: 6 / 10 · this week: 4 / 5 +``` + +Computed from the same `recent_assignment_counts` service so the parts agree. This is load-bearing UX, +not decoration (same lesson as `053`'s Invariant 7): when the push goes quiet because the weekly limit +is hit, the reviewer needs to see *why*, and the line is where they see it — along with the implicit +nudge that `suggest-prs` will still serve them if they want more now. + +The same number belongs **next to the field in the console preferences form**, before a limit is set. +Measured intake is a median of 2/week against a median peak week of 6 +([Measured Baseline](#measured-baseline-2026-08-28)), so a reviewer choosing a number blind will pick +badly in either direction. Showing "you've received N new PRs in the last 7 days" beside the input +costs nothing extra — the load line already computes exactly that count. + +## Subtleties / Invariants + +1. **Additive, orthogonal gate.** `maximum_capacity` (stock) and `max_new_assignments_per_week` (flow) + are independent; a reviewer is auto-assignable only if under **both**. Neither replaces the other. +2. **Opt-in.** `weekly_limit is None` ⇒ the weekly gate is skipped entirely ⇒ byte-for-byte today's + behavior. Nobody is affected until they set a limit. +3. **The limit throttles the push, and counts system-mediated intake.** Counting `applied` + `ReviewerAssignmentApplication` rows makes the push gate count the push pipeline's own output plus + the reviewer-initiated intake the system recorded (console pull-claims, confirm accepts). A raw + Zulip `assign` self-assign is not recorded and does not count — defensible: it is the reviewer + grabbing work entirely on their own, outside any pipeline the limit governs. +4. **One inconsistency, named.** Because console pull-claims write `applied` rows but Zulip + pull-claims do not, a console pull counts toward the weekly limit while the identical action from + Zulip does not. Inherited from `053`'s audit asymmetry, not introduced here. The clean fix is + `053`'s deferred follow-up (route Zulip `assign`'s self-assign through `assign_reviewer_and_record`); + until then the inconsistency is small and always in the reviewer's favour (Zulip pulls are "free"). +5. **Confirm-mode reviewers: the limit counts *accepted* assignments.** A `confirm` reviewer's row is + written on accept, not on propose (`050`). Pending proposals are already bounded by the concurrent + cap (`add_pending_proposal_load`), and the weekly gate limits new *proposals* per week too (the + engine's suggestions become proposals), so a confirm reviewer cannot be flooded. If over-proposal + to slow-accepting reviewers shows up, add "active proposals created in the window" to the count — + see [Open Questions](#open-questions). Out of scope for v1. +6. **A single run may fill the remaining weekly budget; that is acceptable, and deliberate.** With + `≤5/7d` and an empty window, one night can assign up to 5 (further bounded by remaining concurrent + capacity). This is bounded and within the reviewer's stated weekly tolerance. We deliberately do + **not** add intra-week pacing — that was the "drip" the weekly window makes unnecessary. If + single-night clustering ever proves undesirable, revisit; it is not a v1 concern. Measured + (§9): the push currently delivers 1.0–1.8 PRs per reviewer per night, recent maximum 4, so the + clustering this subtlety tolerates is not even occurring today. +7. **Default rule set only, matching apply.** Only the default rule set's snapshot is applied to + GitHub (`046`), so only it writes `applied` rows. The window count and the gate therefore describe + the same population the apply step acts on; per-ruleset compute-only variants neither write rows + nor need the gate. +8. **Determinism / no new state.** The window count is a pure function of durable rows and `now`; the + simulated counts live only for the duration of one run. No bucket state to persist, no drift (same + philosophy as `050`/`053`). + +## Interactions With Existing Pipelines + +- **Pull side (`053`) must override this limit.** `053`'s `assignment_suggestions.py` substitutes an + override profile (`maximum_capacity=sys.maxsize`, `auto_assign=True`, `temporary_break=False`) so an + explicit request ignores every push throttle. `max_new_assignments_per_week` is a push throttle and + must join that list — set the override profile's `weekly_limit` to `None`. **This is the one + required edit to `053` code**, one field in the profile substitution, plus surfacing the weekly + figure in `053`'s own honest load line. Rationale is identical to `053` Invariant 4: a reviewer + *asking* for work is not a statement about how much the *scheduled* pipeline should send. This is + also what makes "catch-up" work — the vacation-returner who wants more than a week's trickle pulls. +- **Attention sweep (`028`).** Unaffected. Auto-unassign still frees concurrent capacity; the freed + PR's original `applied` row stays in the window, so churning a PR does not refund weekly budget — + correct, since it was still "new work" that week. +- **Acceptance gate (`050`).** As in Subtlety 5, the limit counts accepted (applied) assignments; + pending proposals are throttled by concurrent cap + weekly gate. +- **Legacy `src/queueboard/suggest_reviewer.py`.** Out of scope; the applied pipeline is the Django + `analyzer` path (`046`). The legacy compute path is not gated here. + +## Implementation Plan (Chunks) + +Run `uv run ruff check .` and `uv run ruff format .` before every commit; canonical full run is +`bash scripts/repo_check_compose.sh` (mind the AGENTS.md pipe-into-`head`/`tail` trap — read the exit +status unpiped). + +0. **Probe (no code, no deploy) — run 2026-08-28,** + `heroku pg:psql -a queueboard-backend -f scripts/probe_054_rate_limit.sql`; see + [Measured Baseline](#measured-baseline-2026-08-28). Re-run before the pilot picks numbers. +1. **Model + migration.** `ReviewerPreference.max_new_assignments_per_week` (nullable) + generated + migration (on host). No backup-policy change (existing table). Admin `list_display` + + `reviewer-topics.json` import/export coverage. +2. **Count service.** `analyzer/services/` — `recent_assignment_counts(...)` over + `ReviewerAssignmentApplication`. Pure unit tests: distinct-PR counting, window boundary, + normalization, empty result. +3. **Settings.** `ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS` (7) through `settings/base.py` **and** + `.env.example` in the same commit (root AGENTS.md rule — the most-forgotten step; a phantom + `getattr(settings, ...)` with no `os.getenv` line is the antipattern to avoid). +4. **Engine.** Extend `ReviewerProfile` (`weekly_limit`, `recent_assignment_count`); add the weekly + condition to `_reviewer_candidate_state`; track `simulated_this_run` in `run_assignment_simulation`. + Pure-engine tests (reuse `037`'s ranking/scarcity/iterative-rescore seams): limit blocks at the + ceiling; a single run cannot overrun the weekly cap; `None` limit is a no-op; composition with the + concurrent gate. +5. **Integration.** Inject counts + limits in `prepare_assignment_inputs` + (`reviewer_assignment.py:413-468`) via `build_reviewer_catalog`; thread `now`. Service test that a + rate-limited reviewer is withheld end-to-end on a fixture snapshot. +6. **`053` override.** Set `weekly_limit=None` in the `053` override profile; surface the weekly figure + in `053`'s load line. Test that a rate-limited reviewer is still suggested on demand (Invariant: + pull ignores the limit) while the load line reports it honestly. +7. **Surfacing.** Extend `reviewer_load` / `format_load_line` with the weekly figure; render it in + `assigned-prs`, the attention DM, and the console. View/command tests. +8. **Docs.** Finalize this doc; update `qb_site/analyzer/AGENTS.md` (service list) and + `qb_site/core/` preference-field references. + +## Pre-Implementation Notes (sharp edges) + +Captured for whoever implements this — likely a fresh session that will read this doc but not the +design conversation behind it. + +- **Login normalization is a correctness issue, and the answer is already known: the column is not + normalized.** `assign_reviewer_and_record` + (`qb_site/analyzer/services/reviewer_assignment_apply.py:121-129`) stores `reviewer_login` + verbatim from its caller; the `_normalize_login` call next to it is only for comparing against + GitHub's response. The callers disagree: the nightly apply/propose paths pass engine logins + (normalized), while the console accept passes `proposal.reviewer_login` and `053`'s claim passes + `User.github_login` — which preserves GitHub's original case, since `core_user` enforces only + case-*insensitive* uniqueness (`MichaelStollBayreuth` is stored as written). So + `recent_assignment_counts` **must** `lower()` on both sides — the query filter and the returned + keys — or a rate-limited reviewer walks through the gate. Measured: **11 of 41 reviewers** (230 of + 839 rows) are stored capitalized, and no login has two spellings, so the failure is not a partial + undercount but a **zero** count for a quarter of the population — their limits silently never fire. + See [What the measurement changed](#what-the-measurement-changed). +- **The new `ReviewerProfile` fields must default safe** (`weekly_limit=None`, + `recent_assignment_count=0`). `_reviewer_candidate_state` is **shared code**: the nightly builder and + `053`'s `suggest_reviewer_for_pr_with_trace` both call it. `053` sets `weekly_limit=None` on its + override profile (see Interactions), but every *other* construction site (tests, any direct caller) + must also get a no-op default, or you change behavior you didn't intend. Add the fields with + defaults; don't make them required positional args. +- **Add a trace skip reason (`at_rate_limit`).** The engine's diagnostic trace records a + machine-readable reason per unassigned PR (`037`); a reviewer filtered by the weekly gate should + surface as `at_rate_limit`, parallel to the existing capacity reason, so the persisted nightly trace + and admin explain a quiet reviewer. `053`'s skip tally needs **no** new row — the requester's limit is + always overridden, so it can never fire there (mirrors why `053` has no `at_capacity` row). +- **Confirm-mode over-proposal is the subtlest interaction to watch** (Subtlety 5, Open Q2). Pending + proposals write no `applied` row, so they don't count; a slow-accepting `confirm` reviewer can + accumulate more pending proposals than their weekly number across nights. It self-limits at the + *concurrent* cap (pending proposals consume concurrent load), so it is bounded — but validate it in + the pilot before deciding whether to fold active-proposals-in-window into the count. +- **Surfacing threads a cheap count through ~4 call sites, and must not trigger a second payload read.** + The weekly figure is a small indexed DB query, independent of the multi-MB snapshot payload + `reviewer_load` already reads (`053` measured that read at ~411 ms). Compute the weekly count + alongside — never by loading the payload again — and thread it into the load model so `assigned-prs`, + the attention DM, the console, and `053`'s own load line render it consistently. +- **Perf and migration are non-issues.** `ReviewerAssignmentApplication` is tiny (859 rows on + 2026-08-28, ~12.5/day measured), so the distinct-PR count query needs no new index yet (revisit only + if the table grows by orders of magnitude). The migration is a nullable-column add — fast, no backfill. Generate it on the host per + the AGENTS.md note (the refused-DB-connection RuntimeWarning is harmless). +- **UI copy: "in any 7-day period", not "this week".** The window is rolling, not a calendar week; the + reviewer-facing label and help text should say so, to avoid a "why am I blocked, it's Monday" + confusion. + +## Validation Plan + +- **Unit (engine, pure):** limit blocks at `recent + simulated == weekly_limit`; a single run cannot + overrun; `None` limit ⇒ unchanged suggestions; weekly gate composes with the concurrent gate + (blocked if either fails). +- **Unit (count service):** distinct-PR semantics (a PR with two `applied` rows counts once); window + boundary (`applied_at` just inside/outside); only `status='applied'` counts; login normalization. +- **Service (integration):** on a fixture snapshot + seeded `ReviewerAssignmentApplication` rows, a + reviewer at their weekly limit receives no new suggestions though they have free concurrent capacity. +- **`053` regression:** the same rate-limited reviewer *does* get on-demand suggestions, and the load + line shows `this week: N / limit`. +- **Surfacing:** `assigned-prs` / console render the weekly line; it matches the service. +- Canonical full run: `bash scripts/repo_check_compose.sh`. +- **Measure first (`053`-style) — done 2026-08-28.** `scripts/probe_054_rate_limit.sql`; results and + their consequences in [Measured Baseline](#measured-baseline-2026-08-28). Re-run it before the + pilot sets limits (§§2–3 move week to week) and after `053` has real usage (§8). +- **Manual, pre-enable:** run the assignment build for mathlib4 with a test reviewer limited low and + confirm (a) the nightly suggestion set omits them once at the weekly limit, (b) intake stays under + the limit across a fast-clearing week, (c) an on-demand `suggest-prs` still serves them. + +### Measuring first (the probe) + +`scripts/probe_054_rate_limit.sql` — read-only, **no dyno and no deploy**: `heroku pg:psql` runs the +file locally against the production database, so the probe does not have to ship first. + +```bash +heroku pg:psql -a queueboard-backend -f scripts/probe_054_rate_limit.sql +``` + +Reviewer logins are pseudonymised (first 8 hex of `md5(lower(login))`) so the output can be pasted +into this doc. For real logins, change `\set show_logins 0` to `1` **in the file** — `heroku pg:psql` +does not forward psql flags like `-v` (the same gotcha `043` records). The script creates and drops +one temp view; everything else is a `SELECT`. + +Nine numbered sections, each aimed at a question this doc currently answers from intuition (§1c is a +correctness check inside §1): + +| § | question | what it decides | +| --- | --- | --- | +| 1 | is the count source alive? rows, distinct PRs/reviewers, date span, status mix | a short or thin history means §§3–4 cannot support a number yet | +| 1c | `applied` rows with `applied_at IS NULL` | must be 0 — the gate filters on `applied_at`, so any such row is invisible to it | +| 2 | trailing 7-day distinct-PR intake per reviewer | the headline: what "per week" is worth today | +| 3 | **peak** rolling 7-day intake per reviewer over all history, plus p50/p90/max | the limit-picking number — a cap below a reviewer's peak would have bound | +| 3c | the same peaks over reviewers active in the last 30 days | strips the apply pipeline's rollout period out of the distribution | +| 4 | what-if: assignments blocked at limits 2/3/5/8/10/15 over 90 days | how much a pilot limit actually withholds | +| 4b | the same replay over the last 30 days | the steady-state cost, which is *not* the same as §4's — see the baseline | +| 5 | reviewers with prefs vs reviewers who get intake; `maximum_capacity` beside weekly intake | whether the weekly gate or the concurrent cap binds first, per reviewer | +| 6 | login case hygiene, split spellings, applied logins with no `core_user` | the undercount in [Pre-Implementation Notes](#pre-implementation-notes-sharp-edges), measured | +| 6d | how many *reviewers*, not rows, are stored capitalized | the exact population a case-sensitive count would exempt from their own limit | +| 7 | distinct PRs vs rows (re-assignment churn) | what the distinct-PR rule actually saves over row counting | +| 8 | `snapshot_id IS NULL` (pull-claim) vs snapshot-anchored intake | [Open Question 4](#open-questions) with numbers instead of a shrug | +| 9 | per-day volume and worst single day per reviewer | Subtlety 6 — is single-night clustering already real? | + +Two caveats on reading it. §4 is an **upper bound**: it replays history assuming every other +assignment still happened, but a blocked assignment would also have lowered later windows, so the +true number withheld is smaller. §8's provenance is a **proxy, not a recorded field**: `053`'s console +claim is the only caller that passes `snapshot=None` to `assign_reviewer_and_record`, and snapshots +are `update_or_create`d per `(repository, cache_key)` and never deleted, so a NULL `snapshot_id` is +stable rather than an artifact of `on_delete=SET_NULL`. Good enough to size Open Question 4; not the +provenance marker `053` deferred. + +The script was validated against a seeded local Postgres 16 in both login modes (aggregates checked +against hand-built fixtures) and **run against production on 2026-08-28** — +[Measured Baseline](#measured-baseline-2026-08-28). §§3c and 6d were added afterwards, prompted by +that run, and have not yet been run against production. + +## Operational Notes + +- **Settings** (in `settings/base.py` + `.env.example`): + + | setting | default | purpose | + | --- | --- | --- | + | `ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS` | 7 | the period the per-week limit is measured over | + + This *defines* what "per week" means for the reviewer-facing field. Changing it silently changes the + meaning of every reviewer's number, so it is an operational tuning knob, not something to move + lightly. The limit itself is per-reviewer (`ReviewerPreference.max_new_assignments_per_week`), not a + global setting. That table is the whole settings surface: there is **no** drip/pacing setting (the + window is the smoother) and **no** enable flag (see below). +- **Rollout is inherently safe / opt-in, and ships without a feature flag** (decided 2026-08-27, + Open Question 5 — unlike `046`/`050`/`053`): the code changes nothing until a reviewer (or an admin + via bulk edit / importer) sets a limit, so the opt-in default *is* the off switch and clearing the + pilot cohort's limits *is* the rollback. A small pilot cohort (a few reviewers who asked, e.g. + Christian) validates the behavior before wider adoption. +- **No data migration / backfill.** The history is already there; the first rate-limited run simply + reads the trailing window. +- **Surfacing must ship with enforcement**, not after — a reviewer whose push goes quiet needs the + weekly line to explain it (Surfacing / Subtlety 8). + +## Alternatives (discarded) + +- **Monthly cap plus a derived per-cycle drip** (this doc's own earlier draft). Two coupled concepts — + a 30-day ceiling and an invisible `ceil(cap/30)` per-night drip — where one rolling *weekly* window + does the same work with a single reviewer-facing number and no opaque derived parameter. The weekly + window is both the limit and the smoother. +- **Monthly cap plus an explicit weekly sub-cap** (two reviewer knobs; Christian's monthly budget with + intra-month catch-up). Rejected: the second knob exists only to allow catch-up, and `053` already + provides catch-up on demand — so the push side does not need to. One knob is enough. +- **A configurable window per reviewer** (let a reviewer pick "per 30 days" for a bursty monthly + budget). Rejected for v1 as needless surface: the window is a global operational constant, and + monthly-budget behavior is a niche the pull side covers. Easy to revisit if asked. +- **Count `PRTimelineEvent` (`type=ASSIGNED`) instead.** Complete (captures manual and Zulip + self-assigns), and matches "new PRs assigned this week" literally. Rejected for v1: it needs a + login→`User` bridge, churn/re-assign de-duplication, and a dependency on timeline-backfill + completeness. `ReviewerAssignmentApplication` is already login-keyed (matching the engine's + login-space), indexed, and scoped to exactly the intake the limit governs. Revisit if "total intake" + (including raw Zulip grabs) is wanted — the same decision as `053`'s provenance follow-up. +- **Replace `maximum_capacity` with the weekly rate.** Rejected: the concurrent stock cap still does + useful work (bounding how much a reviewer holds at once); the two limits answer different questions + and compose cleanly. +- **Token-bucket / linear accrual.** Would smooth intra-week too, but needs bucket state or a more + involved stateless reconstruction, to solve a within-week clustering problem that is bounded and not + yet observed (Subtlety 6). Deferred as an upgrade, not a v1 need. +- **Count assignment events rather than distinct PRs.** Rejected: double-counts re-cycled PRs, which + is not "new PRs". + +## Open Questions + +1. **Window length — decided: 7 days** (2026-08-27). 7 is the most legible ("per week") and smooths + well; 14 would be gentler on reviewers with spiky areas at the cost of a longer burst horizon. 7 is + the launch value; may revisit after the pilot, but it is not an open question for v1. +2. Should **active pending proposals** (confirm-mode) count toward the window, to bound over-proposal + to slow-accepting reviewers (Subtlety 5)? Default: no, rely on concurrent cap + weekly gate. +3. Any **global default limit**, or leave it `None`/opt-in indefinitely? Recommendation firmed up by + the measurement: **opt-in**, and do not set a global default on this evidence. A universal 5/week + would have withheld 26.9% of the last 90 days' intake from the reviewer it was aimed at, touching + 26 of 41 reviewers — and **29.6%** over the last 30 days, touching 13 of the 32 still active (§3c, + §4b). Note the direction: excluding the rollout period lowers the headcount but *raises* the intake + share, because recent intake is more concentrated. Aggregate supply says that is absorbable (37 auto-assign reviewers × 5/week ≈ + 185 vs ~83 actually assigned), but whether each withheld PR finds a *topic-eligible* alternate is + exactly what the history cannot answer. A default needs an engine simulation over a snapshot + first, not a pilot's say-so. +4. Do we want to later **exclude console pull-claims** from the count (so the limit is push-only)? + Default: leave them counted — they are real intake. Measuring the question does *not* need `053`'s + deferred provenance marker after all: the claim path is the only caller passing `snapshot=None`, so + `snapshot_id IS NULL` already separates pull-claims from snapshot-anchored intake. That is an + implementation detail rather than a declared field — fine to measure with, not something to build + the gate on. As of 2026-08-28 it reads **zero** pull-claims in 30 days, which says nothing yet: + `053` had been live for one day. Re-measure before deciding. +5. **Kill-switch flag — decided: none** (2026-08-27). No `ANALYZER_ASSIGNMENT_RATE_LIMIT_ENABLED`. + `046`/`050`/`053` each shipped a master flag because each added behavior that *runs on its own* — a + GitHub write sweep, a proposal pipeline, a new endpoint — and needed an off switch independent of + per-reviewer state. This feature adds no such behavior: it is one extra condition on an existing + gate, inert while every `max_new_assignments_per_week` is `NULL`, and turning it off for a reviewer + is an edit to the one field that turned it on. A flag would buy a second off switch for something + already off by default, at the cost of a permanent settings knob, an `.env.example` line, and a + branch in the engine gate. If a pilot goes wrong, clear the pilot cohort's limits — same blast + radius, no lasting surface. + +## Related Decisions +- `037-reviewer-assignment-policy-simulation-and-priority-planning.md` — engine/integration split; the + gate lives in the pure engine. +- `046-apply-reviewer-assignments-in-django.md` — `ReviewerAssignmentApplication`, the history source. +- `050-reviewer-assignment-acceptance-gate.md` — deferred this as "throughput cap"; confirm/propose + interaction. +- `053-on-demand-assignment-suggestions.md` — the pull side that must override this limit and that + provides catch-up/burst. +- `028-reviewer-queue-nudges-v1-daily-report.md` — attention sweep / auto-unassign interaction. + +## Progress Notes +- **2026-08-27** — Draft written from the Zulip thread and a read of `037`/`046`/`050`/`053`. Settled + before drafting: count source = `ReviewerAssignmentApplication` (distinct applied PRs); cap is + opt-in (`null` default). Initial draft modelled a monthly cap + a derived per-cycle drip. +- **2026-08-27 (revised to Option A)** — reframed from "monthly cap + drip" to a **single rolling + weekly rate** (`max_new_assignments_per_week`). Rationale: a raw per-cycle drip is not a legible + reviewer knob, but the same smoothing expressed over a *week* is — and a weekly rolling window is + simultaneously the throughput limit and the smoother, so it replaces both earlier parameters with one + number. Catch-up/burst is delegated to the pull side (`053`), which already overrides push throttles. + File renamed `054-monthly-assignment-cap.md` → `054-assignment-rate-limit.md`. Not yet implemented — + awaiting review. +- **2026-08-27 (review)** — window length locked to 7 days (Open Question 1 closed). Added + [Pre-Implementation Notes](#pre-implementation-notes-sharp-edges) capturing sharp edges for a fresh + implementing session (login normalization, safe `ReviewerProfile` defaults given the shared engine + gate, the `at_rate_limit` trace reason, confirm-mode over-proposal, cheap-count surfacing). +- **2026-08-27 (probe + no kill-switch)** — Open Question 5 closed: **no** + `ANALYZER_ASSIGNMENT_RATE_LIMIT_ENABLED`, because the opt-in default already is the off switch + (rationale recorded in the question and in [Operational Notes](#operational-notes)). Added + `scripts/probe_054_rate_limit.sql` — the measure-first probe — plus + [Measuring first](#measuring-first-the-probe) explaining what each section decides, and chunk 0 in + the implementation plan. Validated against a seeded local Postgres 16 in both login modes; **not yet + run against production**, so `≤5/7d` and every other figure here remain guesses. Writing the probe + settled two code facts that are now folded back into the doc: `ReviewerAssignmentApplication` + stores `reviewer_login` **verbatim, not normalized** (so the count service must `lower()` on both + sides — Pre-Implementation Notes), and `snapshot_id IS NULL` is a usable stand-in for "came from a + `053` pull-claim" (Open Question 4). +- **2026-08-28 (probe run against production)** — results and consequences in + [Measured Baseline](#measured-baseline-2026-08-28). The premise is confirmed with numbers + (`maximum_capacity=10` reviewers taking 22–30 new PRs a month), and `≤5/7d` turns out to be a real + constraint rather than a token one — below the median reviewer's peak week. Six claims in this doc + were re-sized against data: the login-case risk is live and fails *open* (a capitalized reviewer + counts zero, so their gate never fires); distinct-PR counting is insurance with zero + observed churn; single-night clustering is 1–2 PRs, not the burst Subtlety 6 tolerates; + confirm-mode is 6 of 57 reviewers; `053` claims are not yet measurable; and reviewers need their + own intake shown next to the field to pick a number at all. Open Question 3 firmed up to "no global + default without an engine simulation". Probe gained §3c (peaks over active reviewers only) and §6d + (capitalized reviewers, not rows). +- **2026-08-28 (second run: §3c, §6d)** — both sharpen the picture rather than change direction. + Restricting peaks to the 32 reviewers active in the last 30 days drops the median worst week from 6 + to **5** and halves who a 5/week limit would block (26 of 41 → **13 of 32**) — so the all-history + figures were indeed carrying the apply rollout, and `≤5/7d` lands exactly on the median active + reviewer's worst week. §6d puts a number on the login-case bug: **11 of 41 reviewers** are stored + capitalized, so a case-sensitive count would silently exempt a quarter of the population from their + own limit. The corrected `peak > N` predicate was confirmed against §4 on live data (36/26/15/7, + exact). Added §4b (30-day replay) to the probe; not yet run. +- **2026-08-28 (third run: §4b)** — and it refuted the prediction this doc was carrying. §4's 90-day + replay was expected to *overstate* steady-state cost because it spans the apply rollout; the 30-day + replay came back **higher**, 29.6% withheld at 5/week against 26.9%. §3c's halved headcount was a + population effect (stale reviewers leaving the denominator), not falling intensity: recent intake is + more concentrated, with the 13 largest recipients taking 64% of the last 348 assignments. The + redistribution arithmetic still closes in aggregate (~24 withheld PRs/week against ~66/week of + headroom among the 19 reviewers a 5/week cap would not touch), but only in aggregate — topic + matching remains an engine-simulation question. Open Question 3 updated with both figures and the + direction of the difference. From 441102e6955222ea9dd1d3325ae6aa7a38b50223 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 22:44:40 -0400 Subject: [PATCH 03/12] feat(core): add opt-in reviewer assignment rate limit field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ReviewerPreference.maximum_capacity` caps the *stock* of PRs a reviewer holds at once, not the *flow* they take on: a reviewer who clears quickly frees the slot and the next nightly run refills it. Measured on production, reviewers with `maximum_capacity=10` took 22-30 new PRs in 30 days. Add `max_new_assignments_per_week` (nullable) as the flow bound, plus the `ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS` setting that defines what "per week" means. Null is unlimited, which is both the default and the rollout switch — this commit changes no behavior on its own. Wired through the admin changelist and the reviewer-topics.json importer and exporter; the export omits an unset limit so a pre-054 file round-trips as "no limit" rather than silently un-limiting anyone. Design doc 054. Co-Authored-By: Claude Opus 5 (1M context) --- .env.example | 5 +++++ qb_site/core/admin.py | 1 + ...rpreference_max_new_assignments_per_week.py | 18 ++++++++++++++++++ qb_site/core/models/reviewer_preference.py | 8 ++++++++ .../core/services/reviewer_topics_importer.py | 14 ++++++++++++++ qb_site/qb_site/settings/base.py | 6 ++++++ 6 files changed, 52 insertions(+) create mode 100644 qb_site/core/migrations/0008_reviewerpreference_max_new_assignments_per_week.py diff --git a/.env.example b/.env.example index d1983683..0514001a 100644 --- a/.env.example +++ b/.env.example @@ -318,6 +318,11 @@ ANALYZER_ASSIGNMENT_SUGGESTIONS_MAX_SNAPSHOT_AGE_SECONDS=86400 # Acceptance window in days (a proposal expires this long after creation unless accepted); # the per-reviewer notification_settings override is clamped to >= 7. ANALYZER_ASSIGNMENT_PROPOSAL_WINDOW_DAYS=7 +# Rolling window (days) the per-reviewer new-assignment rate limit is measured over +# (ReviewerPreference.max_new_assignments_per_week, design doc 054). This defines what "per week" +# means for every reviewer's number, so changing it re-interprets all of them. No enable flag: +# the limit is unset (unlimited) by default, so opting in is the switch. +ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS=7 # On-queue-exit policy for a pending proposal: invalidate (default) | retain. ANALYZER_ASSIGNMENT_PROPOSAL_ON_QUEUE_EXIT=invalidate # Propose task schedule (daily; default 00:45 UTC, superseding the legacy apply slot). diff --git a/qb_site/core/admin.py b/qb_site/core/admin.py index 19b0d7b3..16776699 100644 --- a/qb_site/core/admin.py +++ b/qb_site/core/admin.py @@ -844,6 +844,7 @@ class ReviewerPreferenceAdmin(admin.ModelAdmin): "repository", "user", "maximum_capacity", + "max_new_assignments_per_week", "auto_assign", "assignment_acceptance", "notifications_enabled", diff --git a/qb_site/core/migrations/0008_reviewerpreference_max_new_assignments_per_week.py b/qb_site/core/migrations/0008_reviewerpreference_max_new_assignments_per_week.py new file mode 100644 index 00000000..74fb1307 --- /dev/null +++ b/qb_site/core/migrations/0008_reviewerpreference_max_new_assignments_per_week.py @@ -0,0 +1,18 @@ +# Generated by Django 5.2.6 on 2026-08-28 02:28 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('core', '0007_reviewerpreference_assignment_acceptance'), + ] + + operations = [ + migrations.AddField( + model_name='reviewerpreference', + name='max_new_assignments_per_week', + field=models.PositiveIntegerField(blank=True, null=True), + ), + ] diff --git a/qb_site/core/models/reviewer_preference.py b/qb_site/core/models/reviewer_preference.py index d7f8a3cf..65d9b1fe 100644 --- a/qb_site/core/models/reviewer_preference.py +++ b/qb_site/core/models/reviewer_preference.py @@ -13,6 +13,10 @@ class ReviewerPreference(TimestampedModel): Fields - ``repository``/``user``: scope and identity; unique together. - ``maximum_capacity``: numeric cap for concurrently assigned PRs (legacy default is 10). + - ``max_new_assignments_per_week``: optional rolling-window cap on *new* assignments (design doc + 054). ``None`` (the default) means unlimited. Orthogonal to ``maximum_capacity``: that one + bounds the stock a reviewer holds at once, this one bounds the flow they take on, so a + reviewer who clears PRs quickly is no longer refilled without limit. - ``auto_assign``: whether the reviewer participates in auto‑assignment. - ``away_until``: optional break end timestamp (timezone‑aware). Suggestions should skip the reviewer while ``now_utc < away_until``. @@ -37,6 +41,10 @@ class ReviewerPreference(TimestampedModel): user = models.ForeignKey(User, on_delete=models.CASCADE, related_name="reviewer_preferences") maximum_capacity = models.PositiveIntegerField(default=10) + # Rolling-window cap on newly assigned PRs (design doc 054). ``None`` = unlimited, which is the + # opt-in default: the weekly gate is skipped entirely and behavior is unchanged. The window + # length is operational (``ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS``, 7 days), not per-reviewer. + max_new_assignments_per_week = models.PositiveIntegerField(null=True, blank=True) auto_assign = models.BooleanField(default=True) # Whether automatic assignment goes through the acceptance gate ("confirm") or assigns # directly like the legacy behavior ("auto"). New reviewers default to "confirm"; existing diff --git a/qb_site/core/services/reviewer_topics_importer.py b/qb_site/core/services/reviewer_topics_importer.py index ce3267f8..c0a2859e 100644 --- a/qb_site/core/services/reviewer_topics_importer.py +++ b/qb_site/core/services/reviewer_topics_importer.py @@ -96,6 +96,9 @@ def import_reviewer_topics( - ``top_level``: maps to ``preferred_labels`` (replace or merge based on ``replace_labels``). - ``free_form``: copied to the ``free_form`` text field. - ``maximum_capacity``: copied when present; otherwise existing/default is kept. + - ``max_new_assignments_per_week``: copied when present (design doc 054); ``null`` clears the + limit. Absent leaves the existing value alone, so a file written before 054 never silently + un-limits a reviewer. - ``conflict_of_interest``: copied to ``conflict_of_interest`` (deduped, case-insensitive). - ``zulip_handle`` and any other extra fields are ignored (not stored). """ @@ -187,6 +190,13 @@ def apply_entry(entry: dict[str, Any]) -> None: changes["maximum_capacity"] = (pref.maximum_capacity, new_cap) pref.maximum_capacity = new_cap + if "max_new_assignments_per_week" in entry: + raw_rate = entry["max_new_assignments_per_week"] + new_rate = None if raw_rate is None else int(raw_rate) # type: ignore[arg-type] + if pref.max_new_assignments_per_week != new_rate: + changes["max_new_assignments_per_week"] = (pref.max_new_assignments_per_week, new_rate) + pref.max_new_assignments_per_week = new_rate + if bool(entry.get("temporary_break")): if pref.auto_assign: changes["auto_assign"] = (pref.auto_assign, False) @@ -277,6 +287,8 @@ def export_reviewer_topics( - Emits ``top_level`` from ``preferred_labels``. - Emits ``free_form`` and ``auto_assign``. - Emits ``maximum_capacity`` only when it differs from the model default (to mirror legacy files). + - Emits ``max_new_assignments_per_week`` only when a limit is set (``None`` is the default and + means unlimited, so omitting it round-trips as "no limit"). - Emits ``conflict_of_interest`` when present. - Does not emit ``zulip_handle`` or other non-model fields. """ @@ -299,6 +311,8 @@ def export_reviewer_topics( } if pref.maximum_capacity != ReviewerPreference._meta.get_field("maximum_capacity").default: entry["maximum_capacity"] = pref.maximum_capacity + if pref.max_new_assignments_per_week is not None: + entry["max_new_assignments_per_week"] = pref.max_new_assignments_per_week if pref.conflict_of_interest: entry["conflict_of_interest"] = list(pref.conflict_of_interest) entries.append(entry) diff --git a/qb_site/qb_site/settings/base.py b/qb_site/qb_site/settings/base.py index 84fcab89..9fb78a0b 100644 --- a/qb_site/qb_site/settings/base.py +++ b/qb_site/qb_site/settings/base.py @@ -471,6 +471,12 @@ def env_optional_bounded_int(name: str, *, minimum: int, maximum: int) -> int | # Acceptance window: a proposal expires this many days after creation unless accepted. The # per-reviewer override in ReviewerPreference.notification_settings is clamped to >= 7. ANALYZER_ASSIGNMENT_PROPOSAL_WINDOW_DAYS = int(os.getenv("ANALYZER_ASSIGNMENT_PROPOSAL_WINDOW_DAYS", "7")) +# Reviewer assignment rate limit (design doc 054): the rolling window the per-reviewer +# ReviewerPreference.max_new_assignments_per_week cap is measured over. This setting *defines* what +# "per week" means for every reviewer's number, so changing it silently changes the meaning of each +# one — an operational tuning knob, not something to move lightly. There is deliberately no enable +# flag: the limit is null (unlimited) by default, so the opt-in default is the off switch. +ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS = int(os.getenv("ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS", "7")) # On-queue-exit policy read inside the proposal_validity predicate: "invalidate" (default) marks a # pending proposal superseded when its PR leaves the review queue; "retain" lets it ride. ANALYZER_ASSIGNMENT_PROPOSAL_ON_QUEUE_EXIT = os.getenv("ANALYZER_ASSIGNMENT_PROPOSAL_ON_QUEUE_EXIT", "invalidate").strip().lower() From 3b8ae62f19617bdec2b59f48e8fddcc763dd2534 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 22:44:55 -0400 Subject: [PATCH 04/12] feat(analyzer): count rolling-window reviewer intake MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `recent_assignment_counts` answers the one question the rate limit needs: how many distinct PRs was this reviewer newly assigned in this repo within the trailing window? Reads the existing `ReviewerAssignmentApplication` history (design doc 046), so there is no new log table and no backfill. Two counting rules earn their keep: - `lower()` on both sides. `reviewer_login` is stored verbatim — the nightly paths pass engine logins while the console accept and the 053 claim pass `User.github_login`, which keeps GitHub's casing. 11 of 41 production reviewers are stored capitalized, and since no login appears under two spellings, a case-sensitive filter would not undercount them, it would return zero and silently disable their limit. - Distinct PRs, not rows, so an auto-unassigned-then-reassigned PR counts once. Currently insurance rather than a live correction (zero churn in 67 days). Design doc 054. Co-Authored-By: Claude Opus 5 (1M context) --- .../services/assignment_rate_limit.py | 112 ++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 qb_site/analyzer/services/assignment_rate_limit.py diff --git a/qb_site/analyzer/services/assignment_rate_limit.py b/qb_site/analyzer/services/assignment_rate_limit.py new file mode 100644 index 00000000..f795d8c6 --- /dev/null +++ b/qb_site/analyzer/services/assignment_rate_limit.py @@ -0,0 +1,112 @@ +"""Rolling-window intake counts — the *flow* half of reviewer capacity (design doc 054). + +``ReviewerPreference.maximum_capacity`` bounds the **stock** a reviewer holds at once; it does not +bound **flow**. A reviewer who clears PRs quickly frees the slot and the next nightly run refills +them, so the concurrent cap only ever bit reviewers who *didn't* act (measured: reviewers with +``maximum_capacity=10`` taking 22–30 new PRs in 30 days). ``max_new_assignments_per_week`` bounds +the flow instead, and this module is the single place that answers the question it needs: + +> how many **distinct PRs** has this reviewer been newly assigned in this repository within the +> trailing ``ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS``? + +Two consumers share it so they cannot disagree: ``build_reviewer_catalog`` (which puts the count on +every ``ReviewerProfile``, where the engine gate reads it) and, through the same profiles, the +reviewer-facing load line. A reviewer whose push went quiet sees the same number that silenced it. + +Counting notes, each load-bearing: + +- **Source is** ``analyzer.ReviewerAssignmentApplication`` **(design doc 046)** — an append-only, + indefinitely-retained row per *system-mediated* assignment: the nightly auto direct-assign, + confirm-mode accepts, and console pull-claims all route through ``assign_reviewer_and_record``. + A raw Zulip ``assign`` self-assign writes no row and so does not count (design doc 053's known + audit asymmetry, inherited rather than introduced here — see 054 Subtlety 4). +- **Distinct PRs, not rows.** A PR that is auto-unassigned by the attention sweep and later + re-assigned writes several ``applied`` rows; the limit counts "new PRs", so it counts once. On + production this currently saves nothing (zero re-assignment churn in 67 days) — it is insurance + against the sweep producing a repeat, where row-counting would then be wrong. +- **Case-insensitive on both sides.** ``ReviewerAssignmentApplication.reviewer_login`` is stored + **verbatim**, not normalized: the nightly paths pass engine logins while the console accept and + the 053 claim pass ``User.github_login``, which keeps GitHub's original casing (``core_user`` + enforces only case-*insensitive* uniqueness). Measured, 11 of 41 production reviewers are stored + capitalized, and this failure mode is not a partial undercount but a **zero** — a case-sensitive + filter would silently exempt a quarter of the population from limits they had opted into. The + ``Lower()`` on the column and the normalization of the requested logins are both required. +- **Only** ``status='applied'`` **counts**, and ``applied_at`` is the clock. Proposed-but-unaccepted + work is bounded by the concurrent cap instead (054 Subtlety 5). +""" + +from __future__ import annotations + +from datetime import datetime, timedelta +from typing import Sequence + +from django.conf import settings +from django.db.models import Count +from django.db.models.functions import Lower + +from analyzer.models import ReviewerAssignmentApplication +from core.models import Repository + + +def normalize_login(login: str | None) -> str: + """Lowercase/strip a login into the key space this module counts in.""" + return (login or "").strip().lower() + + +def assignment_rate_window_days() -> int: + """The configured rolling window, in days (``ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS``). + + Read through one helper so every call site — catalog build, surfacing, UI copy — measures and + *describes* the same period. The setting defines what "per week" means for every reviewer's + stored number, so it is deliberately global rather than per-reviewer. + """ + return int(settings.ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS) + + +def recent_assignment_counts( + repository: Repository, + logins: Sequence[str], + *, + window_days: int, + now: datetime, +) -> dict[str, int]: + """Distinct newly-assigned PRs per reviewer in the trailing window, keyed by normalized login. + + Counts ``applied`` ``ReviewerAssignmentApplication`` rows for ``repository`` whose + ``applied_at`` is at or after ``now - window_days``, deduplicated by ``pr_number`` per reviewer. + Every requested login appears in the result — a reviewer with no intake maps to ``0`` — so + callers can index without a default and never confuse "no rows" with "not asked about". + + ``window_days <= 0`` disables the count (all zeros) rather than degenerating into "everything + since the epoch", which would silently block every limited reviewer. + """ + normalized = sorted({key for key in (normalize_login(login) for login in logins) if key}) + counts: dict[str, int] = {login: 0 for login in normalized} + if not normalized or int(window_days) <= 0: + return counts + + since = now - timedelta(days=int(window_days)) + rows = ( + ReviewerAssignmentApplication.objects.filter( + repository=repository, + status=ReviewerAssignmentApplication.STATUS_APPLIED, + applied_at__gte=since, + ) + .annotate(login_lower=Lower("reviewer_login")) + .filter(login_lower__in=normalized) + # The model has a Meta.ordering; without clearing it Django folds those columns into the + # GROUP BY and the aggregate below fragments into one row per (login, run_date, pr, id). + .order_by() + .values("login_lower") + .annotate(pr_count=Count("pr_number", distinct=True)) + ) + for row in rows: + counts[str(row["login_lower"])] = int(row["pr_count"]) + return counts + + +__all__ = [ + "assignment_rate_window_days", + "normalize_login", + "recent_assignment_counts", +] From 8b8b523ba76b13fb28a0ef9fc3c72bb07d9c1fd5 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 22:44:55 -0400 Subject: [PATCH 05/12] feat(analyzer): gate auto-assignment on the rolling-window rate limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ReviewerProfile` gains `weekly_limit`, `recent_assignment_count` and `simulated_this_run`; `_reviewer_candidate_state` requires a reviewer to pass both the stock gate (`maximum_capacity`) and the new flow gate. All three fields default to a no-op, because that gate is shared code — the nightly builder and 053's suggestions both call it — so every pre-054 construction site keeps today's behavior without naming them. The gate is a strict `<`: "max 5 per week" means at most 5, so a reviewer with four in the window still receives a fifth. `simulated_this_run` charges this run's own picks against the budget, without which one night could spend a whole week's allowance several times over; `run_assignment_simulation` folds each pick back into the picked reviewer's profile beside the existing weight bump. Counts and limits are injected in `build_reviewer_catalog` rather than at `prepare_assignment_inputs`. One grouped query per catalog build, and every consumer of a catalog — builder, trace, suggestions, and the reviewer-facing load line — then reads the identical figure by construction, so the number that silenced a reviewer's push is the number they are shown. The diagnostic trace records `at_rate_limit` separately from `at_capacity`: a reviewer withheld while visibly holding free capacity is otherwise unexplainable. Design doc 054. Co-Authored-By: Claude Opus 5 (1M context) --- .../analyzer/services/reviewer_assignment.py | 25 ++++++- .../services/reviewer_assignment_engine.py | 66 +++++++++++++++++-- 2 files changed, 83 insertions(+), 8 deletions(-) diff --git a/qb_site/analyzer/services/reviewer_assignment.py b/qb_site/analyzer/services/reviewer_assignment.py index a7a6c411..1d523d61 100644 --- a/qb_site/analyzer/services/reviewer_assignment.py +++ b/qb_site/analyzer/services/reviewer_assignment.py @@ -18,6 +18,7 @@ ReviewerAssignmentSnapshot, ReviewerOptOut, ) +from analyzer.services.assignment_rate_limit import assignment_rate_window_days, recent_assignment_counts from analyzer.services.queue_rules import default_rule_set_for_repo, rules_for_rule_set from analyzer.services.queueboard_snapshot import QueueboardSnapshotBuilder from analyzer.services.reviewer_assignment_engine import ( @@ -237,10 +238,24 @@ class AssignmentStatistics: def build_reviewer_catalog(repository: Repository, *, now: datetime | None = None) -> list[ReviewerProfile]: - """Hydrate reviewer profiles from ReviewerPreference rows.""" + """Hydrate reviewer profiles from ReviewerPreference rows. + + Carries both capacity gates: the concurrent ``maximum_capacity`` (stock) and the rolling-window + ``max_new_assignments_per_week`` with its trailing intake count (flow, design doc 054). The + count is fetched here, in one grouped query for the whole catalog, so that *every* consumer of a + catalog — the nightly builder, the diagnostic trace, on-demand suggestions, and the reviewer- + facing load line — sees the same figure. A reviewer whose push goes quiet then reads the number + that silenced it, and the gate and the surfacing cannot drift apart. + """ current_time = now or datetime.now(timezone.utc) + prefs = list(ReviewerPreference.objects.filter(repository=repository).select_related("user").order_by("user__github_login")) + recent_counts = recent_assignment_counts( + repository, + [getattr(pref.user, "github_login", "") or "" for pref in prefs], + window_days=assignment_rate_window_days(), + now=current_time, + ) profiles: list[ReviewerProfile] = [] - prefs = ReviewerPreference.objects.filter(repository=repository).select_related("user").order_by("user__github_login") for pref in prefs: login = getattr(pref.user, "github_login", None) if not login: @@ -248,6 +263,7 @@ def build_reviewer_catalog(repository: Repository, *, now: datetime | None = Non temporary_break = bool(pref.away_until and pref.away_until > current_time) preferred_labels = list(pref.preferred_labels or []) conflicts = list(pref.conflict_of_interest or []) + weekly_limit = pref.max_new_assignments_per_week profile = ReviewerProfile( github_login=login, maximum_capacity=pref.maximum_capacity, @@ -258,6 +274,11 @@ def build_reviewer_catalog(repository: Repository, *, now: datetime | None = Non free_form=pref.free_form or "", conflict_of_interest=conflicts, conflict_of_interest_lower={c.lower() for c in conflicts}, + weekly_limit=None if weekly_limit is None else int(weekly_limit), + # Keyed by normalized login: the history column stores whatever casing the writing + # caller used, so an unnormalized lookup here would read 0 for every reviewer whose + # GitHub login is capitalized and quietly disable their limit (design doc 054). + recent_assignment_count=recent_counts.get(_normalize_login(login), 0), ) profiles.append(profile) return profiles diff --git a/qb_site/analyzer/services/reviewer_assignment_engine.py b/qb_site/analyzer/services/reviewer_assignment_engine.py index 78371f2a..a0ac27d2 100644 --- a/qb_site/analyzer/services/reviewer_assignment_engine.py +++ b/qb_site/analyzer/services/reviewer_assignment_engine.py @@ -1,7 +1,7 @@ from __future__ import annotations import random -from dataclasses import dataclass, field +from dataclasses import dataclass, field, replace from functools import partial import re from typing import Callable, Dict, Iterable, Sequence @@ -11,6 +11,14 @@ @dataclass(frozen=True) class ReviewerProfile: + """One reviewer's assignability inputs, as plain data (no ORM) — see design doc 037. + + The last three fields carry the rolling-window rate limit (design doc 054) and all default to a + no-op, deliberately: ``_reviewer_candidate_state`` is shared code, called by the nightly builder + *and* by on-demand suggestions, so every construction site that predates 054 (tests included) + must keep today's behavior without naming them. + """ + github_login: str maximum_capacity: int auto_assign: bool @@ -20,6 +28,15 @@ class ReviewerProfile: free_form: str conflict_of_interest: list[str] conflict_of_interest_lower: set[str] + # ``ReviewerPreference.max_new_assignments_per_week``; None = unlimited (no weekly gate). + weekly_limit: int | None = None + # Distinct PRs newly assigned to them inside the rolling window, from the durable + # ReviewerAssignmentApplication history (analyzer.services.assignment_rate_limit). + recent_assignment_count: int = 0 + # Picks this reviewer has already been handed *in this simulation run*, which are not in the + # window count yet. Without it a single nightly run could overrun the weekly cap. Maintained by + # ``run_assignment_simulation``; a correctness guard, not an intra-week pacing knob. + simulated_this_run: int = 0 @dataclass @@ -95,6 +112,19 @@ def _current_weight(login: str, assignments: Dict[str, tuple[list[int], float, i return float(data[1]) if data else 0.0 +def _within_rate_limit(reviewer: ReviewerProfile) -> bool: + """Whether the reviewer is still under their rolling-window intake cap (design doc 054). + + Note the strict ``<``: "max 5 per week" means at most 5, not at most 4, so a reviewer with four + PRs in the window is still available and receives a fifth — the *sixth* is what this blocks. + ``weekly_limit is None`` short-circuits to today's behavior, which is what keeps the feature + inert until a reviewer opts in. + """ + if reviewer.weekly_limit is None: + return True + return reviewer.recent_assignment_count + reviewer.simulated_this_run < int(reviewer.weekly_limit) + + def add_pending_proposal_load( assignment_stats: Dict[str, tuple[list[int], float, int]], pending_load_by_login: Dict[str, float], @@ -172,7 +202,9 @@ def _reviewer_candidate_state( continue current_weight = _current_weight(reviewer.github_login, assignment_stats) remaining = reviewer.maximum_capacity - current_weight - if remaining > 0 and reviewer.auto_assign and not reviewer.temporary_break: + # Two orthogonal capacity gates: stock (`remaining`, maximum_capacity) and flow + # (`_within_rate_limit`, max_new_assignments_per_week). A reviewer must pass both. + if remaining > 0 and reviewer.auto_assign and not reviewer.temporary_break and _within_rate_limit(reviewer): available.append(reviewer.github_login) available_weights.append(remaining) @@ -357,6 +389,10 @@ def suggest_reviewer_for_pr_with_trace( "temporary_break": [], "auto_assign_disabled": [], "at_capacity": [], + # Design doc 054: filtered by the rolling-window intake cap rather than the concurrent one. + # Recorded separately so the persisted nightly trace (and the admin reading it) can explain + # a reviewer who went quiet while visibly holding free capacity. + "at_rate_limit": [], } matching: list[tuple[ReviewerProfile, list[str]]] = [] @@ -424,14 +460,18 @@ def suggest_reviewer_for_pr_with_trace( current_weight = _current_weight(reviewer_login, assignment_stats) remaining = reviewer.maximum_capacity - current_weight + within_rate_limit = _within_rate_limit(reviewer) + if remaining <= 0: filtered["at_capacity"].append(reviewer_login) if not reviewer.auto_assign: filtered["auto_assign_disabled"].append(reviewer_login) if reviewer.temporary_break: filtered["temporary_break"].append(reviewer_login) + if not within_rate_limit: + filtered["at_rate_limit"].append(reviewer_login) - if remaining > 0 and reviewer.auto_assign and not reviewer.temporary_break: + if remaining > 0 and reviewer.auto_assign and not reviewer.temporary_break and within_rate_limit: available.append(reviewer_login) weights[reviewer_login] = { "current_weight": float(current_weight), @@ -499,6 +539,11 @@ def run_assignment_simulation( } suggestions: dict[int, str] = {} remaining_prs = list(inputs.prs_to_assign) + # Local copy so this run's picks can be folded back into the profiles the gate reads. The + # window count in `recent_assignment_count` is a durable-history figure that cannot yet include + # anything decided here, so without `simulated_this_run` a single run could hand a reviewer more + # than their whole weekly budget in one night (design doc 054). + reviewers = list(inputs.reviewers) per_pr: dict[str, dict] if include_trace: @@ -511,7 +556,7 @@ def run_assignment_simulation( ordered_prs, ranking_trace = rank_prs_for_assignment( prs_to_assign=remaining_prs, all_prs=inputs.all_prs, - reviewers=inputs.reviewers, + reviewers=reviewers, assignment_stats=stats_copy, excluded_by_pr=inputs.excluded_by_pr, priority_scorer=priority_scorer, @@ -547,7 +592,7 @@ def run_assignment_simulation( if include_trace: result, trace = suggest_reviewer_for_pr_with_trace( pr_entry=pr_entry, - reviewers=inputs.reviewers, + reviewers=reviewers, assignment_stats=stats_copy, rng=rng, excluded_logins=excluded_logins, @@ -558,7 +603,7 @@ def run_assignment_simulation( result = suggest_reviewer_for_pr( pr_number=pr_number, pr_entry=pr_entry, - reviewers=inputs.reviewers, + reviewers=reviewers, assignment_stats=stats_copy, rng=rng, excluded_logins=excluded_logins, @@ -575,6 +620,15 @@ def run_assignment_simulation( open_list = list(open_list) open_list.append(pr_number) stats_copy[result.suggested] = (open_list, weight + 1, total + 1) + # Same spot, the flow counterpart of the weight bump above: charge this pick against the + # picked reviewer's rolling-window budget for the rest of the run. + picked_norm = _normalize_login(result.suggested) + reviewers = [ + replace(reviewer, simulated_this_run=reviewer.simulated_this_run + 1) + if _normalize_login(reviewer.github_login) == picked_norm + else reviewer + for reviewer in reviewers + ] remaining_prs.remove(pr_number) round_index += 1 From f073b5545f1d5a7f7781f7bec88c81a2b1613c61 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 22:45:08 -0400 Subject: [PATCH 06/12] feat(analyzer): surface the rate limit, and let on-demand requests override it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two halves of the same requirement: enforcement ships with its explanation. 053's override profile gains `weekly_limit=None`, joining `maximum_capacity`, `auto_assign` and `away_until` as a push throttle an explicit request ignores (053 Invariant 4). This is load-bearing rather than tidy: a weekly window deliberately gives up saving unused budget, so catch-up has to live on the pull side, and this is it. The skip tally needs no `at_rate_limit` row for the same reason it has no `at_capacity` — the requester's gates are always overridden. `ReviewerLoad` carries `weekly_count` / `weekly_limit` / `at_weekly_limit`, read off the same profile the engine gate uses, and `format_load_line` appends `· last 7 days: 4 / 5` (with `⚠ weekly limit reached` once spent). One change reaches `assigned-prs`, the daily attention DM, the console and 053 at once. Wording is "last N days", not "this week", and N comes from the setting that defines the window: it is rolling, and the calendar reading produces a "why am I blocked, it's Monday" bug report. A reviewer with no limit gets a byte-for-byte unchanged line. Design doc 054. Co-Authored-By: Claude Opus 5 (1M context) --- .../services/assignment_suggestions.py | 31 +++++++--- qb_site/analyzer/services/reviewer_load.py | 57 ++++++++++++++++++- 2 files changed, 77 insertions(+), 11 deletions(-) diff --git a/qb_site/analyzer/services/assignment_suggestions.py b/qb_site/analyzer/services/assignment_suggestions.py index 9077df29..91af8732 100644 --- a/qb_site/analyzer/services/assignment_suggestions.py +++ b/qb_site/analyzer/services/assignment_suggestions.py @@ -15,9 +15,12 @@ random ``picked`` — and the ranking's sort key is a total order, so identical requests against one snapshot generation return identical ordered results (and a smaller ``limit`` returns a strict prefix of a larger one). -4. Push-throttle preferences (``away_until``, ``auto_assign``, ``maximum_capacity``) are - overridden by the explicit request; correctness rules (authorship, conflict-of-interest, - opt-outs, cooldowns, assignment-forbidden labels, active assignees/proposals) never are. +4. Push-throttle preferences (``away_until``, ``auto_assign``, ``maximum_capacity``, and the + rolling-window ``max_new_assignments_per_week`` of design doc 054) are overridden by the + explicit request; correctness rules (authorship, conflict-of-interest, opt-outs, cooldowns, + assignment-forbidden labels, active assignees/proposals) never are. The rate limit in + particular *depends* on this: it is a deliberately un-saveable weekly trickle, and this pull + path is where a reviewer with spare capacity right now catches up. 5. One candidate pool: the assignable set comes from ``prepare_assignment_inputs``, shared with the nightly builder, so a suggestion can never offer what the scheduled run would refuse. 7. Capacity is reported, never enforced: ``load`` comes from the reviewer's *real* @@ -61,8 +64,9 @@ STATUS_NONE_ELIGIBLE = "none_eligible" # Skip-tally reasons, in the engine's own evaluation order (each pool PR is counted once against -# the first rule that excluded the requester). Deliberately no `at_capacity`: the requester's -# capacity is always overridden, so it can never be the reason *they* were skipped (Invariant 7). +# the first rule that excluded the requester). Deliberately no `at_capacity` and, for the same +# reason, no `at_rate_limit`: both of the requester's capacity gates are always overridden, so +# neither can ever be why *they* were skipped (Invariant 7). SKIP_ALREADY_ASSIGNED = "already_assigned" SKIP_NO_TOPIC_LABEL = "no_topic_label" SKIP_AUTHORED = "authored" @@ -215,8 +219,9 @@ def suggest_prs_for_reviewer( Read-only (Invariant 1): reads the cached queue snapshot for the repo's default rule set — never builds one — and persists nothing. ``labels`` *replaces* the reviewer's stored ``preferred_labels`` for this request; the request also overrides ``away_until``, - ``auto_assign`` and ``maximum_capacity`` (push throttles, Invariant 4), while authorship, - conflict-of-interest, opt-outs, cooldowns and the pool filters stay in force. + ``auto_assign``, ``maximum_capacity`` and ``max_new_assignments_per_week`` (push throttles, + Invariant 4), while authorship, conflict-of-interest, opt-outs, cooldowns and the pool filters + stay in force. """ current_time = now or datetime.now(timezone.utc) effective_limit = int(settings.ANALYZER_ASSIGNMENT_SUGGESTIONS_LIMIT) if limit is None else int(limit) @@ -268,8 +273,16 @@ def _result(**overrides) -> SuggestionResult: # The request profile: an explicit request overrides the push throttles (Invariant 4). The # capacity override is unconditional (Invariant 7) — the load line below carries the honest - # capacity signal instead. - override_kwargs: dict = {"auto_assign": True, "temporary_break": False, "maximum_capacity": sys.maxsize} + # capacity signal instead. `weekly_limit=None` retires the 054 rate limit for this request on + # the same footing: a reviewer *asking* for work is not a statement about how much the + # scheduled pipeline should send them, and this is the catch-up path a weekly window + # deliberately gives up on the push side. + override_kwargs: dict = { + "auto_assign": True, + "temporary_break": False, + "maximum_capacity": sys.maxsize, + "weekly_limit": None, + } if label_override: override_kwargs["preferred_labels"] = list(known_labels) override_kwargs["preferred_labels_lower"] = set(known_labels) diff --git a/qb_site/analyzer/services/reviewer_load.py b/qb_site/analyzer/services/reviewer_load.py index 19b73710..1c7ce7f4 100644 --- a/qb_site/analyzer/services/reviewer_load.py +++ b/qb_site/analyzer/services/reviewer_load.py @@ -13,6 +13,12 @@ - ``assigned_open`` is the raw count of open PRs they are assigned to (proposals are load, not assignees, so they are *not* counted here), kept alongside for human context (the gap between it and ``current_load`` reflects zero-weight PRs and pending proposals). +- ``weekly_count`` / ``weekly_limit`` are the *flow* gate (design doc 054): distinct PRs newly + assigned to them in the trailing ``ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS``, against their opt-in + ``max_new_assignments_per_week``. They come off the same ``ReviewerProfile`` the engine gate + reads, so the reason a reviewer's push went quiet is exactly the number they are shown. This + surfacing is load-bearing, not decoration: a rate limit that silently withholds work with no + visible cause is indistinguishable from the pipeline being broken. This module deliberately does **not** re-derive load math: it reads the cached queue snapshot (the same one ``pr_info`` uses) and folds ``collect_assignment_statistics`` output against reviewer @@ -29,6 +35,7 @@ from django.conf import settings from analyzer.models import QueueSnapshot +from analyzer.services.assignment_rate_limit import assignment_rate_window_days from analyzer.services.queue_rules import default_rule_set_for_repo from analyzer.services.reviewer_assignment import ( _compute_weight, @@ -51,7 +58,12 @@ def normalize_login(login: str | None) -> str: @dataclass(frozen=True) class ReviewerLoad: - """A reviewer's load standing in one repository (see module docstring).""" + """A reviewer's load standing in one repository (see module docstring). + + The two capacity gates are orthogonal and both reported: ``current_load``/``capacity`` is the + concurrent stock, ``weekly_count``/``weekly_limit`` the rolling-window flow. A reviewer can be + well under one and blocked by the other. + """ repository_id: int reviewer_login: str # normalized (lowercase) @@ -60,6 +72,12 @@ class ReviewerLoad: capacity: int remaining: float at_capacity: bool + # Rolling-window intake (design doc 054). ``weekly_count`` is always populated — it is what a + # reviewer needs in order to pick a limit at all — while ``weekly_limit`` is None until they + # opt in, and ``at_weekly_limit`` is then always False. + weekly_count: int = 0 + weekly_limit: int | None = None + at_weekly_limit: bool = False def compute_reviewer_loads( @@ -74,6 +92,9 @@ def compute_reviewer_loads( weighted_load, total_assigned)``). Assignment keys are matched to reviewers case-insensitively. The result is keyed by normalized login and includes *every* reviewer in ``reviewers`` — a reviewer with nothing assigned gets a zero load (so callers can render "Load: 0 / N"). + + The rolling-window figures are read straight off the profiles rather than re-queried, so the + line a reviewer is shown is by construction the one the engine gate applied to them. """ weighted_by_login: dict[str, float] = {} open_count_by_login: dict[str, int] = {} @@ -92,6 +113,8 @@ def compute_reviewer_loads( current_load = weighted_by_login.get(norm, 0.0) capacity = int(reviewer.maximum_capacity) remaining = capacity - current_load + weekly_limit = None if reviewer.weekly_limit is None else int(reviewer.weekly_limit) + weekly_count = int(reviewer.recent_assignment_count) loads[norm] = ReviewerLoad( repository_id=int(repository_id), reviewer_login=norm, @@ -100,6 +123,11 @@ def compute_reviewer_loads( capacity=capacity, remaining=remaining, at_capacity=remaining <= _CAPACITY_EPSILON, + weekly_count=weekly_count, + weekly_limit=weekly_limit, + # Mirrors the engine's strict `recent + simulated < limit`: a reviewer *at* their limit + # has spent it, so this is `>=`, not `>`. + at_weekly_limit=weekly_limit is not None and weekly_count >= weekly_limit, ) return loads @@ -228,12 +256,36 @@ def format_load_contribution(weight: float) -> str: return f"+{_fmt_load_number(weight)}" +def format_rate_limit_segment(load: ReviewerLoad) -> str: + """Render the rolling-window intake segment, or ``""`` for a reviewer with no limit. + + ``· last 7 days: 4 / 5``, or ``· last 7 days: 5 / 5 ⚠ weekly limit reached`` once spent. Wording + is deliberately "last N days" rather than "this week": the window is rolling, so a reviewer + blocked on a Monday morning would otherwise reasonably expect a fresh budget. The day count is + read from the setting that actually defines the window, so the copy cannot drift from the + mechanism. + + Empty for ``weekly_limit is None``, which is every reviewer until they opt in — the load line is + byte-for-byte unchanged for them (design doc 054, Invariant 2). + """ + if load.weekly_limit is None: + return "" + days = assignment_rate_window_days() + segment = f" · last {days} days: {load.weekly_count} / {load.weekly_limit}" + if load.at_weekly_limit: + segment += " ⚠ weekly limit reached" + return segment + + def format_load_line(load: ReviewerLoad, *, include_assigned_count: bool = False) -> str: """Render the one-line load summary. ``Load: 3 / 10 (7 free)`` normally; ``Load: 10 / 10 ⚠ at capacity`` when full (or over). With ``include_assigned_count`` (the daily digest, which never lists the full roster), append - ``· N assigned``. + ``· N assigned``. A reviewer who has opted into a rolling-window rate limit also gets the + intake segment (design doc 054), e.g. ``Load: 3 / 10 (7 free) · last 7 days: 5 / 5 ⚠ weekly + limit reached`` — the two gates are independent, so being flush with concurrent capacity while + blocked on flow is a normal, and otherwise baffling, state to be in. ``at_capacity`` mirrors the engine's strict ``remaining > 0`` assignability gate, so a reviewer with any real room (e.g. 9.96/10) is *not* full and can still be assigned. Two display rules keep @@ -257,4 +309,5 @@ def format_load_line(load: ReviewerLoad, *, include_assigned_count: bool = False line = f"Load: {_fmt_load_number(used_val)} / {cap} ({_fmt_load_number(free_val)} free)" if include_assigned_count: line += f" · {load.assigned_open} assigned" + line += format_rate_limit_segment(load) return line From 50981c52fb5dd310167573188760d6cad6e2c669 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 22:45:08 -0400 Subject: [PATCH 07/12] feat(console): let reviewers set their own assignment rate limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds the field to `/console/preferences/`, alongside `maximum_capacity`. Reviewers cannot pick a number they cannot see: measured intake is a median of 2/week against a median *worst* week of 5, so the help text shows the reviewer their own trailing intake next to the input. The count is computed in the console view and passed in, because it is an `analyzer` figure and `core` does not import `analyzer`; omitting it drops the sentence and nothing else. Label and help text both name the window from `ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS`, so the copy cannot drift from the mechanism, and both say "N days" rather than "per week" — the window is rolling. Blank clears the limit. A limit of 0 is rejected: "never assign me anything" is `auto_assign` off, which says so on every surface, rather than a rate only the engine gate could explain. Design doc 054. Co-Authored-By: Claude Opus 5 (1M context) --- qb_site/console/views.py | 28 +++++++++- qb_site/core/forms.py | 56 +++++++++++++++++++ qb_site/core/services/reviewer_prefs.py | 10 +++- .../shared/_reviewer_prefs_fields.html | 7 +++ 4 files changed, 99 insertions(+), 2 deletions(-) diff --git a/qb_site/console/views.py b/qb_site/console/views.py index 6345c63d..194e6b65 100644 --- a/qb_site/console/views.py +++ b/qb_site/console/views.py @@ -11,7 +11,7 @@ import logging import secrets -from typing import Iterable +from typing import Iterable, Sequence from zoneinfo import ZoneInfo from django.conf import settings @@ -31,6 +31,11 @@ queue_membership, resolve_on_queue_exit_policy, ) +from analyzer.services.assignment_rate_limit import ( + assignment_rate_window_days, + normalize_login, + recent_assignment_counts, +) from analyzer.services.assignment_suggestions import ( STATUS_NO_LABELS, STATUS_NO_SNAPSHOT, @@ -381,6 +386,26 @@ def _batch_labels(prs: Iterable[PullRequest]) -> dict[int, list[str]]: # --- preferences ------------------------------------------------------------- +def _recent_intake_by_repo(preferences: Sequence[ReviewerPreference]) -> dict[int, int]: + """``repository_id -> new PRs assigned to this reviewer in the rolling window`` (design doc 054). + + Shown next to the rate-limit field so the number a reviewer types is informed by their own + history. Computed here rather than in ``core.services.reviewer_prefs`` because the count is an + ``analyzer`` concern and ``core`` must not import ``analyzer``. One small indexed query per + repository the reviewer has a preference row in — typically one. + """ + window_days = assignment_rate_window_days() + now = timezone.now() + intake: dict[int, int] = {} + for pref in preferences: + login = getattr(pref.user, "github_login", "") or "" + if not login: + continue + counts = recent_assignment_counts(pref.repository, [login], window_days=window_days, now=now) + intake[int(pref.repository_id)] = counts.get(normalize_login(login), 0) + return intake + + @require_http_methods(["GET", "POST"]) def prefs(request: HttpRequest) -> HttpResponse: """Reviewer preferences at a stable, token-less URL (design doc 022). @@ -410,6 +435,7 @@ def prefs(request: HttpRequest) -> HttpResponse: preferences=preferences, user_timezone=user_timezone, data=request.POST if request.method == "POST" else None, + recent_intake_by_repo=_recent_intake_by_repo(preferences), ) if request.method == "POST" and formset.is_valid(): formset.save() diff --git a/qb_site/core/forms.py b/qb_site/core/forms.py index aa9d7360..2b112c72 100644 --- a/qb_site/core/forms.py +++ b/qb_site/core/forms.py @@ -25,6 +25,7 @@ REVIEWER_PREFERENCE_EDITABLE_FIELDS: tuple[str, ...] = ( "maximum_capacity", + "max_new_assignments_per_week", "auto_assign", "assignment_acceptance", "notifications_enabled", @@ -78,6 +79,36 @@ def prepare_value(self, value: object) -> str: return str(value or "") +def _rate_limit_window_days() -> int: + """The configured rolling window in days, for the rate-limit field's label and help text. + + Read from settings rather than hardcoded so the reviewer-facing copy always describes the window + actually being enforced (``analyzer.services.assignment_rate_limit`` reads the same setting for + the count itself; this module stays free of an ``analyzer`` import). + """ + return int(settings.ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS) + + +def _rate_limit_help_text(*, recent_intake: int | None) -> str: + """Help text for the rolling-window assignment cap (design doc 054). + + Two jobs. First, name the window as *rolling* — the field is stored as "per week" but enforced + over a trailing N days, and "per week" alone invites the calendar-week reading. Second, show the + reviewer their own recent intake: measured median intake is ~2/week against a median *worst* + week of 5, so a reviewer picking a number without those figures is guessing, and guessing badly + in either direction (a limit above their peak does nothing; one far below it silences the push). + """ + days = _rate_limit_window_days() + text = ( + f"Cap on how many new PRs auto-assignment may give you in any {days}-day period. " + "Leave blank for no limit. This is a rolling window, not a calendar week, and it does not " + "limit PRs you ask for yourself." + ) + if recent_intake is not None: + text += f" You have been assigned {recent_intake} new PR{'' if recent_intake == 1 else 's'} in the last {days} days." + return text + + class ReviewerPreferenceForm(forms.ModelForm): # Acceptance-gate mode (design doc 050). Exposed as a two-option radio; the values are the # model's own choices ("auto"/"confirm") so the ModelForm persists it without any conversion. @@ -127,6 +158,7 @@ class Meta: fields = REVIEWER_PREFERENCE_EDITABLE_FIELDS widgets = { "maximum_capacity": forms.NumberInput(attrs={"min": 1, "step": 1}), + "max_new_assignments_per_week": forms.NumberInput(attrs={"min": 1, "step": 1, "placeholder": "no limit"}), "free_form": forms.Textarea(attrs={"rows": 4}), } @@ -136,6 +168,7 @@ def __init__( user_timezone: tzinfo | None = None, label_catalog_by_repo: Mapping[int, list[str]] | None = None, topic_label_pattern_by_repo: Mapping[int, str] | None = None, + recent_intake_by_repo: Mapping[int, int] | None = None, **kwargs: object, ) -> None: super().__init__(*args, **kwargs) @@ -179,6 +212,14 @@ def __init__( ) self.fields["away_until"].help_text = f"Temporary break end time. Leave blank if active. Interpreted in {tz_label}." self.fields["auto_assign"].help_text = "Turn this off to opt out of automatic reviewer assignment for this repository." + # Label and help text both name the window from the setting that defines it, so the copy + # cannot drift from the mechanism — and both say "N days" rather than "per week", because + # the window is rolling (design doc 054). + rate_window_days = _rate_limit_window_days() + self.fields["max_new_assignments_per_week"].label = f"Max new assignments per {rate_window_days} days" + self.fields["max_new_assignments_per_week"].help_text = _rate_limit_help_text( + recent_intake=(recent_intake_by_repo or {}).get(int(repo_id)) if repo_id is not None else None + ) self.fields["notifications_enabled"].help_text = "Enable daily queue nudge notifications for this repository." self.fields["free_form"].help_text = format_html( "A free form description of your reviewing interests. {}", community_team_page_warning @@ -214,6 +255,21 @@ def clean_maximum_capacity(self) -> int: raise forms.ValidationError("Ensure this value is greater than or equal to 1.") return value + def clean_max_new_assignments_per_week(self) -> int | None: + """Blank clears the limit; a set value must be at least 1. + + ``0`` is rejected rather than accepted as "block everything": a reviewer who wants no + auto-assignment at all turns off ``auto_assign``, which says so plainly on every surface, + instead of encoding it as a rate of zero that only the engine gate would explain. + """ + value = self.cleaned_data.get("max_new_assignments_per_week") + if value in (None, ""): + return None + value = int(value) + if value < 1: + raise forms.ValidationError("Ensure this value is greater than or equal to 1, or leave it blank for no limit.") + return value + def clean_preferred_labels(self) -> list[str]: labels = self.cleaned_data.get("preferred_labels") or [] return _dedupe_case_insensitive_preserve_first(str(label) for label in labels) diff --git a/qb_site/core/services/reviewer_prefs.py b/qb_site/core/services/reviewer_prefs.py index d4bc56f2..5b50157f 100644 --- a/qb_site/core/services/reviewer_prefs.py +++ b/qb_site/core/services/reviewer_prefs.py @@ -14,7 +14,7 @@ from __future__ import annotations -from collections.abc import Sequence +from collections.abc import Mapping, Sequence from datetime import tzinfo from django.db.models import Case, IntegerField, QuerySet, When @@ -51,11 +51,18 @@ def build_preferences_formset( preferences: Sequence[ReviewerPreference], user_timezone: tzinfo, data: QueryDict | None = None, + recent_intake_by_repo: Mapping[int, int] | None = None, ) -> BaseModelFormSet: """Build the formset over ``preferences`` (already authorized by the caller). ``data`` bound → a POST; ``None`` → a fresh render. ``user_timezone`` is what naive ``away_until`` input is interpreted in (see ``zulip_bot.services.user_timezone``). + + ``recent_intake_by_repo`` (``repository_id -> new PRs in the rolling window``) is displayed + beside the rate-limit field so a reviewer can pick a number against their own history rather + than blind (design doc 054). It is *supplied by the caller* rather than computed here: the + figure lives in ``analyzer``, and ``core`` does not import ``analyzer``. Omitting it drops the + sentence and nothing else. """ return ReviewerPreferenceFormSet( data, @@ -64,6 +71,7 @@ def build_preferences_formset( "user_timezone": user_timezone, "label_catalog_by_repo": _label_catalog_by_repo(preferences), "topic_label_pattern_by_repo": _topic_label_pattern_by_repo(preferences), + "recent_intake_by_repo": dict(recent_intake_by_repo or {}), }, ) diff --git a/qb_site/templates/shared/_reviewer_prefs_fields.html b/qb_site/templates/shared/_reviewer_prefs_fields.html index e74fbeb9..08b9dadb 100644 --- a/qb_site/templates/shared/_reviewer_prefs_fields.html +++ b/qb_site/templates/shared/_reviewer_prefs_fields.html @@ -63,6 +63,13 @@

Auto-Assignment

{% if form.maximum_capacity.errors %}
    {% for error in form.maximum_capacity.errors %}
  • {{ error }}
    • {% endfor %}
{% endif %} +
+ + {{ form.max_new_assignments_per_week }} + {% if form.max_new_assignments_per_week.help_text %}{{ form.max_new_assignments_per_week.help_text }}{% endif %} + {% if form.max_new_assignments_per_week.errors %}
    {% for error in form.max_new_assignments_per_week.errors %}
  • {{ error }}
    • {% endfor %}
{% endif %} +
+ {% if "assignment_acceptance" in form.fields %}
From 56e28294adfab8c7478f0d4f01e213b6b4e16bec Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 22:45:22 -0400 Subject: [PATCH 08/12] test: cover the reviewer assignment rate limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four layers, per design doc 054's validation plan. Count service: distinct-PR counting, window boundary (inclusive at the edge), `applied`-only, repo scoping, empty and disabled windows — and the login-case test, which is the one keeping `lower()` in place. A regression there fails open, returning zero for a capitalized reviewer so their limit never fires, which is invisible in every surface. Engine: the strict `<` boundary the probe's own 3b/4 cross-check tripped on (reaching the limit is allowed, exceeding it is not), the two gates composing without replacing each other, `at_rate_limit` in the trace, and the guard that a single run cannot overrun the weekly cap. Integration: a reviewer at their limit receives nothing from the push despite entirely free concurrent capacity — the case `maximum_capacity` never covered — while `suggest_prs_for_reviewer` still serves them the full list and reports the spent budget honestly. Surfacing and form: the load line is unchanged for a reviewer with no limit, setting and clearing round-trips, 0 is rejected, and the help text carries the reviewer's own intake and the rolling-window wording. Co-Authored-By: Claude Opus 5 (1M context) --- .../services/test_assignment_rate_limit.py | 418 ++++++++++++++++++ .../tests/services/test_reviewer_load.py | 74 +++- .../console/tests/test_prefs_form_fields.py | 61 +++ 3 files changed, 551 insertions(+), 2 deletions(-) create mode 100644 qb_site/analyzer/tests/services/test_assignment_rate_limit.py diff --git a/qb_site/analyzer/tests/services/test_assignment_rate_limit.py b/qb_site/analyzer/tests/services/test_assignment_rate_limit.py new file mode 100644 index 00000000..fa20287f --- /dev/null +++ b/qb_site/analyzer/tests/services/test_assignment_rate_limit.py @@ -0,0 +1,418 @@ +"""Reviewer assignment rate limit — design doc 054. + +Four layers, matching the doc's validation plan: the count service over the durable history, the +pure engine gate (including the boundary the probe's own cross-check tripped on), the catalog wiring +that carries counts and limits to both the gate and the surfacing, and the end-to-end withholding. +""" + +from __future__ import annotations + +import random +from datetime import date, datetime, timedelta, timezone as dt_timezone + +from django.test import SimpleTestCase, TestCase, override_settings + +from analyzer.models import QueueRuleSet, ReviewerAssignmentApplication +from analyzer.services.assignment_rate_limit import assignment_rate_window_days, recent_assignment_counts +from analyzer.services.reviewer_assignment import build_reviewer_catalog, prepare_assignment_inputs, suggest_reviewers_many +from analyzer.services.reviewer_assignment_engine import ( + ReviewerProfile, + SimulationInputs, + _reviewer_candidate_state, + _within_rate_limit, + run_assignment_simulation, + suggest_reviewer_for_pr_with_trace, +) +from core.models import Repository, ReviewerPreference, User + +NOW = datetime(2026, 8, 28, 12, 0, tzinfo=dt_timezone.utc) + + +def _profile( + login: str, + *, + capacity: int = 10, + weekly_limit: int | None = None, + recent: int = 0, + simulated: int = 0, + labels: tuple[str, ...] = ("t-algebra",), + auto_assign: bool = True, + temporary_break: bool = False, +) -> ReviewerProfile: + return ReviewerProfile( + github_login=login, + maximum_capacity=capacity, + auto_assign=auto_assign, + temporary_break=temporary_break, + preferred_labels=list(labels), + preferred_labels_lower={lab.lower() for lab in labels}, + free_form="", + conflict_of_interest=[], + conflict_of_interest_lower=set(), + weekly_limit=weekly_limit, + recent_assignment_count=recent, + simulated_this_run=simulated, + ) + + +def _pr(*, author: str = "zed", labels: tuple[str, ...] = ("t-algebra",), assignees: list[str] | None = None) -> dict: + return { + "author": author, + "title": "chore: a change", + "labels": [{"name": name} for name in labels], + "assignees": assignees or [], + "pr_status": "AwaitingReview", + "total_queue_time": {"status": "valid", "value_td": 1000.0}, + } + + +class RecentAssignmentCountsTests(TestCase): + """The count service over ``ReviewerAssignmentApplication`` (design doc 046's history).""" + + def setUp(self) -> None: + self.repo = Repository.objects.create(owner="leanprover-community", name="mathlib4", default_branch="master") + self.other_repo = Repository.objects.create(owner="leanprover-community", name="batteries", default_branch="main") + + def _application( + self, + *, + login: str, + pr_number: int, + applied_at: datetime | None = NOW, + status: str = ReviewerAssignmentApplication.STATUS_APPLIED, + repository: Repository | None = None, + run_date: date | None = None, + ) -> ReviewerAssignmentApplication: + return ReviewerAssignmentApplication.objects.create( + run_date=run_date or (applied_at or NOW).date(), + repository=repository or self.repo, + pr_number=pr_number, + reviewer_login=login, + status=status, + applied_at=applied_at, + ) + + def test_counts_distinct_prs_in_window(self) -> None: + for pr_number in (1, 2, 3): + self._application(login="alice", pr_number=pr_number, applied_at=NOW - timedelta(days=1)) + counts = recent_assignment_counts(self.repo, ["alice"], window_days=7, now=NOW) + self.assertEqual(counts, {"alice": 3}) + + def test_repeat_assignment_of_one_pr_counts_once(self) -> None: + # The attention sweep can auto-unassign a PR that is later re-assigned, writing a second + # applied row for the same (PR, reviewer). The limit counts *new PRs*, so this is one. + self._application(login="alice", pr_number=42, applied_at=NOW - timedelta(days=5), run_date=date(2026, 8, 23)) + self._application(login="alice", pr_number=42, applied_at=NOW - timedelta(days=1), run_date=date(2026, 8, 27)) + self.assertEqual(ReviewerAssignmentApplication.objects.filter(pr_number=42).count(), 2) + counts = recent_assignment_counts(self.repo, ["alice"], window_days=7, now=NOW) + self.assertEqual(counts, {"alice": 1}) + + def test_window_boundary_is_inclusive_at_the_edge(self) -> None: + self._application(login="alice", pr_number=1, applied_at=NOW - timedelta(days=7)) + self._application(login="alice", pr_number=2, applied_at=NOW - timedelta(days=7, seconds=1)) + self._application(login="alice", pr_number=3, applied_at=NOW - timedelta(days=6, hours=23)) + counts = recent_assignment_counts(self.repo, ["alice"], window_days=7, now=NOW) + # PR 2 fell out the far side of the window; PR 1 is exactly on the boundary and counts. + self.assertEqual(counts, {"alice": 2}) + + def test_only_applied_rows_count(self) -> None: + self._application(login="alice", pr_number=1, applied_at=NOW - timedelta(days=1)) + self._application( + login="alice", + pr_number=2, + applied_at=NOW - timedelta(days=1), + status=ReviewerAssignmentApplication.STATUS_SKIPPED_RECENTLY_APPLIED, + run_date=date(2026, 8, 26), + ) + self._application( + login="alice", + pr_number=3, + applied_at=None, + status=ReviewerAssignmentApplication.STATUS_PENDING, + run_date=date(2026, 8, 26), + ) + counts = recent_assignment_counts(self.repo, ["alice"], window_days=7, now=NOW) + self.assertEqual(counts, {"alice": 1}) + + def test_login_matching_is_case_insensitive_on_both_sides(self) -> None: + """The history column stores login casing verbatim; a case-sensitive count reads zero. + + Measured on production: 11 of 41 reviewers are stored capitalized, and because no login + appears under two spellings the failure is not a partial undercount but a total one — their + limits would silently never fire. This is the test that keeps ``lower()`` in place. + """ + self._application(login="MichaelStollBayreuth", pr_number=1, applied_at=NOW - timedelta(days=1)) + self._application(login="michaelstollbayreuth", pr_number=2, applied_at=NOW - timedelta(days=2)) + counts = recent_assignment_counts(self.repo, ["MichaelStollBayreuth"], window_days=7, now=NOW) + self.assertEqual(counts, {"michaelstollbayreuth": 2}) + # And the same answer when the caller asks in the other casing. + self.assertEqual( + recent_assignment_counts(self.repo, ["michaelstollbayreuth"], window_days=7, now=NOW), + {"michaelstollbayreuth": 2}, + ) + + def test_every_requested_login_is_present_even_with_no_intake(self) -> None: + self._application(login="alice", pr_number=1, applied_at=NOW - timedelta(days=1)) + counts = recent_assignment_counts(self.repo, ["alice", "bob"], window_days=7, now=NOW) + self.assertEqual(counts, {"alice": 1, "bob": 0}) + + def test_other_repositories_and_reviewers_do_not_leak(self) -> None: + self._application(login="alice", pr_number=1, applied_at=NOW - timedelta(days=1)) + self._application(login="alice", pr_number=2, applied_at=NOW - timedelta(days=1), repository=self.other_repo) + self._application(login="bob", pr_number=3, applied_at=NOW - timedelta(days=1)) + self.assertEqual(recent_assignment_counts(self.repo, ["alice"], window_days=7, now=NOW), {"alice": 1}) + + def test_empty_and_disabled_window(self) -> None: + self._application(login="alice", pr_number=1, applied_at=NOW - timedelta(days=1)) + self.assertEqual(recent_assignment_counts(self.repo, [], window_days=7, now=NOW), {}) + self.assertEqual(recent_assignment_counts(self.repo, [""], window_days=7, now=NOW), {}) + # A non-positive window counts nothing rather than degenerating into "all of history", + # which would block every limited reviewer outright. + self.assertEqual(recent_assignment_counts(self.repo, ["alice"], window_days=0, now=NOW), {"alice": 0}) + + @override_settings(ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS=14) + def test_window_days_helper_reads_the_setting(self) -> None: + self.assertEqual(assignment_rate_window_days(), 14) + + +class RateLimitGateTests(SimpleTestCase): + """The pure engine gate: ``recent + simulated < weekly_limit``.""" + + def test_no_limit_is_a_no_op(self) -> None: + self.assertTrue(_within_rate_limit(_profile("alice", weekly_limit=None, recent=999))) + + def test_reviewer_may_reach_their_limit_but_not_exceed_it(self) -> None: + """ "Max 5 per week" means at most 5, not at most 4 — the strict ``<``. + + The probe's §3b/§4 cross-check disagreed until this boundary was pinned down, so it is + pinned here too: a reviewer whose worst week was exactly N is never blocked at N. + """ + self.assertTrue(_within_rate_limit(_profile("alice", weekly_limit=5, recent=4))) + self.assertFalse(_within_rate_limit(_profile("alice", weekly_limit=5, recent=5))) + self.assertFalse(_within_rate_limit(_profile("alice", weekly_limit=5, recent=6))) + + def test_this_runs_picks_count_against_the_budget(self) -> None: + self.assertTrue(_within_rate_limit(_profile("alice", weekly_limit=5, recent=3, simulated=1))) + self.assertFalse(_within_rate_limit(_profile("alice", weekly_limit=5, recent=3, simulated=2))) + + def test_candidate_state_withholds_a_rate_limited_reviewer(self) -> None: + _, available, _, _ = _reviewer_candidate_state( + pr_entry=_pr(), + reviewers=[_profile("alice", weekly_limit=2, recent=2), _profile("bob")], + assignment_stats={}, + ) + self.assertEqual(available, ["bob"]) + + def test_gates_compose_and_neither_replaces_the_other(self) -> None: + # Under the weekly limit but at concurrent capacity -> blocked by stock. + _, available, _, _ = _reviewer_candidate_state( + pr_entry=_pr(), + reviewers=[_profile("alice", capacity=1, weekly_limit=5, recent=0)], + assignment_stats={"alice": ([1], 1.0, 1)}, + ) + self.assertEqual(available, []) + # Free concurrent capacity but at the weekly limit -> blocked by flow. This is the whole + # point of 054: the fast-clearing reviewer the stock cap never bound. + _, available, _, _ = _reviewer_candidate_state( + pr_entry=_pr(), + reviewers=[_profile("alice", capacity=10, weekly_limit=5, recent=5)], + assignment_stats={}, + ) + self.assertEqual(available, []) + + def test_trace_records_at_rate_limit_distinctly_from_at_capacity(self) -> None: + _result, trace = suggest_reviewer_for_pr_with_trace( + pr_entry=_pr(), + reviewers=[_profile("alice", capacity=10, weekly_limit=2, recent=2)], + assignment_stats={}, + rng=random.Random(0), + ) + self.assertEqual(trace["filtered"].get("at_rate_limit"), ["alice"]) + self.assertNotIn("at_capacity", trace["filtered"]) + self.assertEqual(trace["available"], []) + + def test_a_single_run_cannot_overrun_the_weekly_cap(self) -> None: + """Five assignable PRs, one reviewer, limit 2 — the run stops at 2, not 5. + + Without ``simulated_this_run`` the durable window count would still read 0 for every pick + in the run and one night could spend a whole week's budget several times over. + """ + all_prs = {n: _pr() for n in range(1, 6)} + result = run_assignment_simulation( + inputs=SimulationInputs( + reviewers=[_profile("alice", capacity=10, weekly_limit=2, recent=0)], + assignments={}, + prs_to_assign=list(all_prs), + all_prs=all_prs, + ), + rng=random.Random(0), + ) + self.assertEqual(len(result.suggestions), 2) + self.assertEqual(set(result.suggestions.values()), {"alice"}) + + def test_a_run_fills_only_the_remaining_budget(self) -> None: + all_prs = {n: _pr() for n in range(1, 6)} + result = run_assignment_simulation( + inputs=SimulationInputs( + reviewers=[_profile("alice", capacity=10, weekly_limit=5, recent=3)], + assignments={}, + prs_to_assign=list(all_prs), + all_prs=all_prs, + ), + rng=random.Random(0), + ) + self.assertEqual(len(result.suggestions), 2) + + def test_unlimited_reviewer_is_unchanged_by_the_feature(self) -> None: + all_prs = {n: _pr() for n in range(1, 6)} + result = run_assignment_simulation( + inputs=SimulationInputs( + reviewers=[_profile("alice", capacity=10, weekly_limit=None, recent=99)], + assignments={}, + prs_to_assign=list(all_prs), + all_prs=all_prs, + ), + rng=random.Random(0), + ) + self.assertEqual(len(result.suggestions), 5) + + +class RateLimitCatalogTests(TestCase): + """``build_reviewer_catalog`` carries both the limit and the measured window count.""" + + def setUp(self) -> None: + self.repo = Repository.objects.create(owner="leanprover-community", name="mathlib4", default_branch="master") + # Deliberately capitalized: the catalog must normalize before looking the count up. + self.user = User.objects.create(github_login="MichaelStollBayreuth") + self.pref = ReviewerPreference.objects.create( + user=self.user, repository=self.repo, maximum_capacity=10, preferred_labels=["t-algebra"] + ) + + def _seed_intake(self, count: int) -> None: + for pr_number in range(1, count + 1): + ReviewerAssignmentApplication.objects.create( + run_date=NOW.date(), + repository=self.repo, + pr_number=pr_number, + reviewer_login=self.user.github_login, + status=ReviewerAssignmentApplication.STATUS_APPLIED, + applied_at=NOW - timedelta(days=1), + ) + + def test_unset_limit_leaves_the_profile_inert(self) -> None: + self._seed_intake(4) + profile = build_reviewer_catalog(self.repo, now=NOW)[0] + self.assertIsNone(profile.weekly_limit) + self.assertEqual(profile.recent_assignment_count, 4) + self.assertTrue(_within_rate_limit(profile)) + + def test_capitalized_login_still_resolves_its_window_count(self) -> None: + self._seed_intake(3) + self.pref.max_new_assignments_per_week = 3 + self.pref.save(update_fields=["max_new_assignments_per_week"]) + profile = build_reviewer_catalog(self.repo, now=NOW)[0] + self.assertEqual(profile.weekly_limit, 3) + self.assertEqual(profile.recent_assignment_count, 3) + self.assertFalse(_within_rate_limit(profile)) + + +class RateLimitEndToEndTests(TestCase): + """A rate-limited reviewer is withheld from the push and still served by the pull (053).""" + + def setUp(self) -> None: + self.repo = Repository.objects.create(owner="leanprover-community", name="mathlib4", default_branch="master") + self.rules = QueueRuleSet.objects.create( + repository=self.repo, + version=1, + require_open=True, + require_not_draft=True, + require_ci_success=False, + required_label_names=[], + forbidden_label_names=[], + is_active=True, + ) + self.alice = User.objects.create(github_login="alice") + self.alice_pref = ReviewerPreference.objects.create( + user=self.alice, repository=self.repo, maximum_capacity=10, preferred_labels=["t-algebra"] + ) + self.payload = { + "meta": {"generated_at": "2026-08-28T00:00:00+00:00"}, + "prs": {str(n): _pr() for n in range(1, 5)}, + "lists": {"dashboards": {"Queue": [1, 2, 3, 4]}}, + } + + def _seed_intake(self, count: int, *, login: str = "alice") -> None: + for pr_number in range(100, 100 + count): + ReviewerAssignmentApplication.objects.create( + run_date=NOW.date(), + repository=self.repo, + pr_number=pr_number, + reviewer_login=login, + status=ReviewerAssignmentApplication.STATUS_APPLIED, + applied_at=NOW - timedelta(days=1), + ) + + def _suggestions(self) -> dict[int, str]: + inputs = prepare_assignment_inputs(self.repo, payload=self.payload, now=NOW, rule_set=self.rules) + return suggest_reviewers_many( + reviewers=inputs.reviewers, + assignments=inputs.assignments, + prs_to_assign=inputs.assignable_queue_prs, + all_prs=self.payload["prs"], + rng=random.Random(0), + excluded_by_pr=inputs.excluded_by_pr, + ) + + def test_reviewer_at_their_limit_gets_nothing_despite_free_capacity(self) -> None: + self._seed_intake(5) + self.alice_pref.max_new_assignments_per_week = 5 + self.alice_pref.save(update_fields=["max_new_assignments_per_week"]) + # Concurrent capacity is entirely free (nothing assigned in the payload) — only the flow + # gate is holding them back, which is exactly the case maximum_capacity never covered. + self.assertEqual(self._suggestions(), {}) + + def test_reviewer_under_their_limit_receives_only_the_remainder(self) -> None: + self._seed_intake(3) + self.alice_pref.max_new_assignments_per_week = 5 + self.alice_pref.save(update_fields=["max_new_assignments_per_week"]) + self.assertEqual(len(self._suggestions()), 2) + + def test_no_limit_means_no_change(self) -> None: + self._seed_intake(20) + self.assertEqual(len(self._suggestions()), 4) + + def test_on_demand_suggestions_override_the_limit(self) -> None: + """Design doc 053 Invariant 4: the pull side ignores every push throttle, this one included. + + This is what makes the short window acceptable — a reviewer cannot save up unused weekly + budget, so catch-up has to live somewhere, and it lives here. + """ + from analyzer.models import QueueSnapshot + from analyzer.services.assignment_suggestions import STATUS_OK, suggest_prs_for_reviewer + from syncer.models import LabelDef + + LabelDef.objects.create(repository=self.repo, name="t-algebra", color="ededed") + QueueSnapshot.objects.create( + repository=self.repo, + cache_key=str(self.rules.id), + generated_at=NOW - timedelta(hours=1), + payload=self.payload, + etag="etag", + pr_count=4, + queue_count=4, + ) + self._seed_intake(5) + self.alice_pref.max_new_assignments_per_week = 5 + self.alice_pref.save(update_fields=["max_new_assignments_per_week"]) + + # The push gives them nothing... + self.assertEqual(self._suggestions(), {}) + # ...while asking directly still does. + result = suggest_prs_for_reviewer(self.repo, "alice", now=NOW) + self.assertEqual(result.status, STATUS_OK) + self.assertEqual([pr.pr_number for pr in result.suggestions], [1, 2, 3, 4]) + # And the load line reports the limit honestly rather than hiding it (Invariant 7). + self.assertIsNotNone(result.load) + self.assertEqual(result.load.weekly_count, 5) + self.assertEqual(result.load.weekly_limit, 5) + self.assertTrue(result.load.at_weekly_limit) diff --git a/qb_site/analyzer/tests/services/test_reviewer_load.py b/qb_site/analyzer/tests/services/test_reviewer_load.py index 2eca6434..98f6ec7f 100644 --- a/qb_site/analyzer/tests/services/test_reviewer_load.py +++ b/qb_site/analyzer/tests/services/test_reviewer_load.py @@ -2,7 +2,7 @@ from datetime import datetime, timezone as dt_timezone -from django.test import TestCase +from django.test import TestCase, override_settings from analyzer.models import AssignmentProposal, QueueRuleSet, QueueSnapshot from analyzer.services.reviewer_assignment_engine import ReviewerProfile @@ -19,7 +19,13 @@ from core.models import Repository, ReviewerPreference, User -def _profile(login: str, capacity: int) -> ReviewerProfile: +def _profile( + login: str, + capacity: int, + *, + weekly_limit: int | None = None, + recent: int = 0, +) -> ReviewerProfile: return ReviewerProfile( github_login=login, maximum_capacity=capacity, @@ -30,6 +36,8 @@ def _profile(login: str, capacity: int) -> ReviewerProfile: free_form="", conflict_of_interest=[], conflict_of_interest_lower=set(), + weekly_limit=weekly_limit, + recent_assignment_count=recent, ) @@ -257,3 +265,65 @@ def test_signed_and_consistent_with_load_line(self) -> None: self.assertEqual(format_load_contribution(1.0), "+1") self.assertEqual(format_load_contribution(0.1), "+0.1") self.assertEqual(format_load_contribution(0.0), "+0") + + +class TestRateLimitSurfacing(TestCase): + """The rolling-window figure on the load line (design doc 054). + + Surfacing ships with enforcement, not after: a reviewer whose push goes quiet because they hit + their weekly limit has to be able to see that from the same line that shows their capacity, or + the feature is indistinguishable from the pipeline being broken. + """ + + def test_no_limit_leaves_the_load_line_byte_for_byte_unchanged(self) -> None: + loads = compute_reviewer_loads( + repository_id=1, + assignments={"alice": ([1, 2, 3], 3.0, 3)}, + reviewers=[_profile("alice", 10, weekly_limit=None, recent=4)], + ) + load = loads["alice"] + self.assertEqual(load.weekly_count, 4) + self.assertIsNone(load.weekly_limit) + self.assertFalse(load.at_weekly_limit) + self.assertEqual(format_load_line(load), "Load: 3 / 10 (7 free)") + + def test_limit_appends_the_rolling_window_segment(self) -> None: + loads = compute_reviewer_loads( + repository_id=1, + assignments={"alice": ([1, 2, 3], 3.0, 3)}, + reviewers=[_profile("alice", 10, weekly_limit=5, recent=4)], + ) + load = loads["alice"] + self.assertFalse(load.at_weekly_limit) + self.assertEqual(format_load_line(load), "Load: 3 / 10 (7 free) · last 7 days: 4 / 5") + + def test_spent_budget_is_flagged_even_with_free_capacity(self) -> None: + # The state the feature exists to create, and the one that most needs explaining: plenty of + # concurrent room, no new work arriving. + loads = compute_reviewer_loads( + repository_id=1, + assignments={}, + reviewers=[_profile("alice", 10, weekly_limit=5, recent=5)], + ) + load = loads["alice"] + self.assertTrue(load.at_weekly_limit) + self.assertFalse(load.at_capacity) + self.assertEqual(format_load_line(load), "Load: 0 / 10 (10 free) · last 7 days: 5 / 5 ⚠ weekly limit reached") + + def test_segment_follows_the_configured_window(self) -> None: + loads = compute_reviewer_loads( + repository_id=1, + assignments={}, + reviewers=[_profile("alice", 10, weekly_limit=5, recent=1)], + ) + with override_settings(ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS=14): + self.assertIn("last 14 days: 1 / 5", format_load_line(loads["alice"])) + + def test_digest_variant_keeps_both_suffixes(self) -> None: + loads = compute_reviewer_loads( + repository_id=1, + assignments={"alice": ([1], 1.0, 1)}, + reviewers=[_profile("alice", 10, weekly_limit=3, recent=2)], + ) + line = format_load_line(loads["alice"], include_assigned_count=True) + self.assertEqual(line, "Load: 1 / 10 (9 free) · 1 assigned · last 7 days: 2 / 3") diff --git a/qb_site/console/tests/test_prefs_form_fields.py b/qb_site/console/tests/test_prefs_form_fields.py index 5975a9c6..b353f338 100644 --- a/qb_site/console/tests/test_prefs_form_fields.py +++ b/qb_site/console/tests/test_prefs_form_fields.py @@ -8,8 +8,13 @@ from __future__ import annotations +from datetime import timedelta + from django.test import TestCase, override_settings from django.urls import reverse +from django.utils import timezone + +from analyzer.models import ReviewerAssignmentApplication from console.session import SESSION_USER_KEY from core.models import Repository, ReviewerPreference, User @@ -74,6 +79,9 @@ def _post_data(self) -> tuple[dict[str, object], dict[int, int]]: policy = pref.notification_settings or {} data[f"form-{idx}-id"] = str(pref.id) data[f"form-{idx}-maximum_capacity"] = str(pref.maximum_capacity) + data[f"form-{idx}-max_new_assignments_per_week"] = ( + "" if pref.max_new_assignments_per_week is None else str(pref.max_new_assignments_per_week) + ) data[f"form-{idx}-auto_assign"] = "on" if pref.auto_assign else "" data[f"form-{idx}-assignment_acceptance"] = pref.assignment_acceptance data[f"form-{idx}-notifications_enabled"] = "on" if pref.notifications_enabled else "" @@ -101,6 +109,7 @@ def test_get_renders_the_expected_sections_and_fields(self) -> None: "auto_unassign_days", "away_until", "maximum_capacity", + "max_new_assignments_per_week", "notifications_enabled", "stale_nudge_days", "preferred_labels", @@ -197,6 +206,58 @@ def test_post_invalid_capacity_shows_validation_error(self) -> None: self.pref1.refresh_from_db() self.assertEqual(self.pref1.maximum_capacity, 10) + # ---- rolling-window rate limit (design doc 054) --------------------- + + def test_post_sets_and_clears_the_rate_limit(self) -> None: + data, index_by_id = self._post_data() + i = index_by_id[self.pref1.id] + + data[f"form-{i}-max_new_assignments_per_week"] = "5" + self.assertEqual(self.client.post(self.url, data=data).status_code, 302) + self.pref1.refresh_from_db() + self.assertEqual(self.pref1.max_new_assignments_per_week, 5) + + # Blank is the opt-out, and it is also the default: clearing the field restores unlimited + # intake rather than leaving the last number in force. + data[f"form-{i}-max_new_assignments_per_week"] = "" + self.assertEqual(self.client.post(self.url, data=data).status_code, 302) + self.pref1.refresh_from_db() + self.assertIsNone(self.pref1.max_new_assignments_per_week) + + def test_post_rejects_a_zero_rate_limit(self) -> None: + # "Never assign me anything" is `auto_assign` off, which says so on every surface; a rate of + # zero would be the same thing spelled in a way only the engine gate could explain. + data, index_by_id = self._post_data() + data[f"form-{index_by_id[self.pref1.id]}-max_new_assignments_per_week"] = "0" + + response = self.client.post(self.url, data=data) + + self.assertEqual(response.status_code, 200) + self.assertContains(response, "Ensure this value is greater than or equal to 1") + self.pref1.refresh_from_db() + self.assertIsNone(self.pref1.max_new_assignments_per_week) + + def test_rate_limit_help_text_names_the_rolling_window_and_recent_intake(self) -> None: + """A reviewer cannot pick a number they cannot see (design doc 054). + + Also pins the "7-day period" wording over "this week": the window is rolling, and the + calendar reading produces a "why am I blocked, it's Monday" bug report. + """ + for pr_number in (1, 2, 3): + ReviewerAssignmentApplication.objects.create( + run_date=timezone.now().date(), + repository=self.repo1, + pr_number=pr_number, + reviewer_login="REVIEWER", # stored casing differs from the lookup on purpose + status=ReviewerAssignmentApplication.STATUS_APPLIED, + applied_at=timezone.now() - timedelta(days=1), + ) + + response = self.client.get(self.url) + + self.assertContains(response, "7-day period") + self.assertContains(response, "assigned 3 new PRs in the last 7 days") + def test_post_invalid_notification_threshold_order_shows_validation_error(self) -> None: data, index_by_id = self._post_data() i = index_by_id[self.pref1.id] From 4edce68d689bfdac594e76bbfa55a46f40661dfe Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 22:45:22 -0400 Subject: [PATCH 09/12] doc: record 054 as implemented, not yet enabled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Marks chunks 1-8 done and moves the doc's status from Draft/Proposed to implemented-but-inert: no reviewer has a limit set, which is also the whole rollout mechanism (no feature flag, Open Question 5). Still outstanding: a pilot cohort, and the engine simulation Open Question 3 wants before any global default. Records the three places the implementation is sharper than the plan was — the count landing in `build_reviewer_catalog` so the gate and the surfacing agree structurally rather than by discipline, `simulated_this_run` as a profile field instead of a dict threaded through five signatures, and "last 7 days" over "this week" — plus what was and was not verified. Adds `assignment_rate_limit.py` to the analyzer service list. Co-Authored-By: Claude Opus 5 (1M context) --- .../054-assignment-rate-limit.md | 89 +++++++++++++------ qb_site/analyzer/AGENTS.md | 18 +++- 2 files changed, 75 insertions(+), 32 deletions(-) diff --git a/docs/design-decisions/054-assignment-rate-limit.md b/docs/design-decisions/054-assignment-rate-limit.md index e5edf2a3..0a86804c 100644 --- a/docs/design-decisions/054-assignment-rate-limit.md +++ b/docs/design-decisions/054-assignment-rate-limit.md @@ -1,10 +1,13 @@ # Reviewer Assignment Rate Limit (Rolling Weekly Intake Cap) -> Status: **Draft / Proposed** (2026-08-28) — design only, no implementation yet. The measurement -> probe has been run against production; see [Measured Baseline](#measured-baseline-2026-08-28), -> which confirms the premise and re-sizes several claims. Written to be reviewed before code lands. Origin: a Zulip thread (Christian Merten, with -> Yaël Dillies' earlier proposal and Bryan Gin-ge Chen) on making reviewer capacity limits -> actually bind. +> Status: **Implemented, not yet enabled for anyone** (2026-08-28). Chunks 1–8 of the +> [Implementation Plan](#implementation-plan-chunks) have landed; the code is inert until a reviewer +> sets `max_new_assignments_per_week`, which is also the whole rollout mechanism (no feature flag, +> Open Question 5). Still to do: a pilot cohort, and the engine simulation Open Question 3 asks for +> before any global default. The measurement probe was run against production first; see +> [Measured Baseline](#measured-baseline-2026-08-28), which confirms the premise and re-sized +> several claims. Origin: a Zulip thread (Christian Merten, with Yaël Dillies' earlier proposal and +> Bryan Gin-ge Chen) on making reviewer capacity limits actually bind. ## Context @@ -421,30 +424,29 @@ status unpiped). 0. **Probe (no code, no deploy) — run 2026-08-28,** `heroku pg:psql -a queueboard-backend -f scripts/probe_054_rate_limit.sql`; see [Measured Baseline](#measured-baseline-2026-08-28). Re-run before the pilot picks numbers. -1. **Model + migration.** `ReviewerPreference.max_new_assignments_per_week` (nullable) + generated - migration (on host). No backup-policy change (existing table). Admin `list_display` + - `reviewer-topics.json` import/export coverage. -2. **Count service.** `analyzer/services/` — `recent_assignment_counts(...)` over - `ReviewerAssignmentApplication`. Pure unit tests: distinct-PR counting, window boundary, - normalization, empty result. -3. **Settings.** `ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS` (7) through `settings/base.py` **and** - `.env.example` in the same commit (root AGENTS.md rule — the most-forgotten step; a phantom - `getattr(settings, ...)` with no `os.getenv` line is the antipattern to avoid). -4. **Engine.** Extend `ReviewerProfile` (`weekly_limit`, `recent_assignment_count`); add the weekly - condition to `_reviewer_candidate_state`; track `simulated_this_run` in `run_assignment_simulation`. - Pure-engine tests (reuse `037`'s ranking/scarcity/iterative-rescore seams): limit blocks at the - ceiling; a single run cannot overrun the weekly cap; `None` limit is a no-op; composition with the - concurrent gate. -5. **Integration.** Inject counts + limits in `prepare_assignment_inputs` - (`reviewer_assignment.py:413-468`) via `build_reviewer_catalog`; thread `now`. Service test that a - rate-limited reviewer is withheld end-to-end on a fixture snapshot. -6. **`053` override.** Set `weekly_limit=None` in the `053` override profile; surface the weekly figure - in `053`'s load line. Test that a rate-limited reviewer is still suggested on demand (Invariant: - pull ignores the limit) while the load line reports it honestly. -7. **Surfacing.** Extend `reviewer_load` / `format_load_line` with the weekly figure; render it in - `assigned-prs`, the attention DM, and the console. View/command tests. -8. **Docs.** Finalize this doc; update `qb_site/analyzer/AGENTS.md` (service list) and - `qb_site/core/` preference-field references. +1. **Model + migration.** ✅ `ReviewerPreference.max_new_assignments_per_week` (nullable) + + `core/migrations/0008_…` (generated on host). No backup-policy change (existing table). Admin + `list_display`, `reviewer-topics.json` import/export, and the console preferences form/template. +2. **Count service.** ✅ `analyzer/services/assignment_rate_limit.py` — + `recent_assignment_counts(...)` over `ReviewerAssignmentApplication`, plus + `assignment_rate_window_days()` so no caller hardcodes 7. Unit tests: distinct-PR counting, + window boundary, status filter, case normalization, repo scoping, empty/disabled window. +3. **Settings.** ✅ `ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS` (7) in `settings/base.py` **and** + `.env.example`. +4. **Engine.** ✅ `ReviewerProfile` gained `weekly_limit` / `recent_assignment_count` / + `simulated_this_run` (all safe-defaulted); `_within_rate_limit` is the new condition in + `_reviewer_candidate_state`; `run_assignment_simulation` folds each pick back into the picked + reviewer's profile beside the existing weight bump. Trace records `at_rate_limit`. +5. **Integration.** ✅ Counts and limits are injected in `build_reviewer_catalog` rather than at + `prepare_assignment_inputs` — one grouped query per catalog build, which means the nightly + builder, the trace, `053`, *and* the load line all read the identical figure by construction. +6. **`053` override.** ✅ `weekly_limit=None` joins the override profile; the weekly figure rides + `053`'s existing load line. Tested: a reviewer at their limit gets nothing from the push and the + full list on demand. +7. **Surfacing.** ✅ `ReviewerLoad` carries `weekly_count` / `weekly_limit` / `at_weekly_limit` and + `format_load_line` appends `· last 7 days: N / M` (`⚠ weekly limit reached` when spent), so + `assigned-prs`, the attention DM, the console and `053` all render it from the one place. +8. **Docs.** ✅ This doc; `qb_site/analyzer/AGENTS.md` service list. ## Pre-Implementation Notes (sharp edges) @@ -706,3 +708,32 @@ that run, and have not yet been run against production. headroom among the 19 reviewers a 5/week cap would not touch), but only in aggregate — topic matching remains an engine-simulation question. Open Question 3 updated with both figures and the direction of the difference. +- **2026-08-28 (implemented)** — chunks 1–8 landed; the feature is inert until a reviewer sets a + limit. Three places where the implementation is sharper than this doc had it, recorded because + each was a real choice: + - **The count is fetched in `build_reviewer_catalog`, not `prepare_assignment_inputs`.** The plan + said the latter *via* the former; putting the query in the catalog builder means the load line + gets the figure for free (it builds a catalog too), which turns "the gate and the surfacing must + agree" from a discipline into a structural property. One grouped query per catalog build. + - **`simulated_this_run` is a `ReviewerProfile` field, not a parallel dict.** A dict alongside + `assignment_stats` would have matched the existing `_current_weight` pattern but had to be + threaded through five signatures plus the `PRAssignmentPriorityScorer` type. Instead + `run_assignment_simulation` keeps a local reviewer list and `replace()`s the picked reviewer's + profile beside the existing weight bump — same spot, no signature churn, and the `recent` / + `simulated` split the doc specifies stays visible in the data. + - **The reviewer-facing copy says "last 7 days", not "this week"** (the Surfacing section's own + example), following the Pre-Implementation note: the window is rolling, and the calendar reading + is exactly the confusion that note predicted. The form label and help text derive the number + from `ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS`, so the copy cannot drift from the mechanism. + + Two small decisions the doc did not cover: a limit of **0 is rejected** by the form (that is + `auto_assign` off, which says so on every surface, rather than a rate only the engine gate could + explain), and a **non-positive window counts nothing** rather than degenerating into "all of + history", which would block every limited reviewer at once. + + Verified: the new suite (24 tests) plus `analyzer`, `console`, `core`, `zulip_bot` and `api` + (935 + 152) all pass, along with `manage.py check`, `makemigrations --check`, backup-policy + validation and GraphQL validation. `scripts/repo_check_compose.sh` was not run end-to-end; its + steps were reproduced individually against the dockerized Postgres. The three + `syncer.tests.tasks.test_commit_history_tasks` errors are the documented bare-host `GH_TOKEN` + absence, unrelated to this change. diff --git a/qb_site/analyzer/AGENTS.md b/qb_site/analyzer/AGENTS.md index c375e3a4..57dbad01 100644 --- a/qb_site/analyzer/AGENTS.md +++ b/qb_site/analyzer/AGENTS.md @@ -18,15 +18,27 @@ review-load (weighted, matching the assignment engine's capacity gate, **incl. pending assignment proposals** per design doc 050) as of the latest cached queue snapshot, plus `format_load_line`. Single authority shared by the `assigned-prs` command, the daily reviewer-attention digest, and the reviewer console; read-only (never - builds a snapshot), returns `{}`/`None` when no snapshot exists. + builds a snapshot), returns `{}`/`None` when no snapshot exists. `ReviewerLoad` also carries the *flow* gate + (`weekly_count` / `weekly_limit` / `at_weekly_limit`, design doc 054), read off the same `ReviewerProfile` the + engine gate uses, and `format_load_line` appends `· last 7 days: 4 / 5` for a reviewer who has opted into a + limit (nothing at all for one who has not). + - `assignment_rate_limit.py` — `recent_assignment_counts(repository, logins, *, window_days, now)`: distinct + **newly assigned** PRs per reviewer in the trailing `ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS`, from the + `ReviewerAssignmentApplication` history (design doc 054). The *flow* half of reviewer capacity, next to + `maximum_capacity`'s *stock*. One grouped query, `lower()` on both sides — the history column stores login + casing verbatim, so a case-sensitive count silently reads **zero** for a capitalized reviewer and their limit + never fires. Consumed via `build_reviewer_catalog`, which puts the count and the reviewer's + `max_new_assignments_per_week` on every profile so the engine gate and the load line cannot disagree. + Also exports `assignment_rate_window_days()` — read the window through it, never hardcode 7. - `assignment_suggestions.py` — `suggest_prs_for_reviewer(repository, login, *, labels, limit)`: on-demand "what should I review?" (design doc 053). Single authority for which open PRs a reviewer could take right now and why not the rest; the Zulip `suggest-prs` command and the console suggestions page both render its output and never re-derive eligibility. Shares the nightly builder's candidate pool via `reviewer_assignment.prepare_assignment_inputs` (new pool exclusions belong there, not at call sites), overrides only the requester's push throttles - (`away_until`, `auto_assign`, `maximum_capacity`) via a profile substitution — the engine is - unmodified and correctness rules (authorship, conflicts, opt-outs, cooldowns) stay in force — + (`away_until`, `auto_assign`, `maximum_capacity`, and the 054 rate limit) via a profile + substitution — the engine is unmodified and correctness rules (authorship, conflicts, opt-outs, + cooldowns) stay in force — and reads only the trace's `available`/`potential` membership, never the random `picked`, so results are deterministic per snapshot. Read-only: never builds a snapshot, persists nothing. Refuses a snapshot older than `ANALYZER_ASSIGNMENT_SUGGESTIONS_MAX_SNAPSHOT_AGE_SECONDS` From 4085d5b2d06162f7b8bc22bde85d03ea6e9444f1 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 22:57:07 -0400 Subject: [PATCH 10/12] fix(console): make the two capacity gates legible side by side MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rendering the preferences page showed the rate-limit field landing directly right of `maximum_capacity` in the 2-column grid — the correct cell, stock beside flow — but with 247 characters of help text against its neighbour's none. `maximum_capacity` has never had help text; adding an annotated field next to it made that read as an oversight, and left nothing on the page saying these are two different *kinds* of limit. So: give `maximum_capacity` a one-line explanation naming it as the concurrent hold, and cut the rate limit's text from 247 chars to 113 (168 with the measured intake sentence) by dropping the part that restated the label. The label already says "per 7 days"; the help now carries only what a number cannot — that the window is rolling rather than a calendar week, that blank means unlimited, and that self-requested PRs never count against it. Every other help text on the page is 59-97 characters, so the pair now differs by about a line instead of four. Design doc 054. Co-Authored-By: Claude Opus 5 (1M context) --- .../console/tests/test_prefs_form_fields.py | 16 ++++++++--- qb_site/core/forms.py | 27 +++++++++++++------ 2 files changed, 32 insertions(+), 11 deletions(-) diff --git a/qb_site/console/tests/test_prefs_form_fields.py b/qb_site/console/tests/test_prefs_form_fields.py index b353f338..7141fc7c 100644 --- a/qb_site/console/tests/test_prefs_form_fields.py +++ b/qb_site/console/tests/test_prefs_form_fields.py @@ -240,8 +240,8 @@ def test_post_rejects_a_zero_rate_limit(self) -> None: def test_rate_limit_help_text_names_the_rolling_window_and_recent_intake(self) -> None: """A reviewer cannot pick a number they cannot see (design doc 054). - Also pins the "7-day period" wording over "this week": the window is rolling, and the - calendar reading produces a "why am I blocked, it's Monday" bug report. + Also pins the rolling-window wording over "this week": the calendar reading produces a + "why am I blocked, it's Monday" bug report. """ for pr_number in (1, 2, 3): ReviewerAssignmentApplication.objects.create( @@ -255,8 +255,18 @@ def test_rate_limit_help_text_names_the_rolling_window_and_recent_intake(self) - response = self.client.get(self.url) - self.assertContains(response, "7-day period") + self.assertContains(response, "rolling 7 days, not a calendar week") self.assertContains(response, "assigned 3 new PRs in the last 7 days") + # The label carries the period, so the help text must not restate the cap. + self.assertNotContains(response, "Cap on how many new PRs") + + def test_both_capacity_gates_explain_which_kind_of_limit_they_are(self) -> None: + """`maximum_capacity` and the rate limit sit side by side; a bare number beside an + annotated one reads as an oversight, and nothing would distinguish stock from flow.""" + response = self.client.get(self.url) + + self.assertContains(response, "How many assigned PRs you can hold at once") + self.assertContains(response, "Max new assignments per 7 days") def test_post_invalid_notification_threshold_order_shows_validation_error(self) -> None: data, index_by_id = self._post_data() diff --git a/qb_site/core/forms.py b/qb_site/core/forms.py index 2b112c72..d91eecc5 100644 --- a/qb_site/core/forms.py +++ b/qb_site/core/forms.py @@ -92,17 +92,21 @@ def _rate_limit_window_days() -> int: def _rate_limit_help_text(*, recent_intake: int | None) -> str: """Help text for the rolling-window assignment cap (design doc 054). - Two jobs. First, name the window as *rolling* — the field is stored as "per week" but enforced - over a trailing N days, and "per week" alone invites the calendar-week reading. Second, show the - reviewer their own recent intake: measured median intake is ~2/week against a median *worst* - week of 5, so a reviewer picking a number without those figures is guessing, and guessing badly - in either direction (a limit above their peak does nothing; one far below it silences the push). + Carries only what the label cannot. The label already says "per N days", so this does not + restate the cap; it adds the three things a reviewer cannot infer from a number: + + 1. the window is *rolling*, not a calendar week — the "why am I blocked, it's Monday" case; + 2. blank means unlimited, which is the opt-in default; + 3. the limit throttles the push only, never PRs they request themselves (design doc 053). + + Then their own trailing intake, because measured median intake is ~2/week against a median + *worst* week of 5 — a reviewer picking a number without that figure is guessing, and guessing + badly in either direction (a limit above their peak does nothing; one far below it goes quiet). """ days = _rate_limit_window_days() text = ( - f"Cap on how many new PRs auto-assignment may give you in any {days}-day period. " - "Leave blank for no limit. This is a rolling window, not a calendar week, and it does not " - "limit PRs you ask for yourself." + f"Counted over a rolling {days} days, not a calendar week. " + "Leave blank for no limit; PRs you request yourself never count." ) if recent_intake is not None: text += f" You have been assigned {recent_intake} new PR{'' if recent_intake == 1 else 's'} in the last {days} days." @@ -212,6 +216,13 @@ def __init__( ) self.fields["away_until"].help_text = f"Temporary break end time. Leave blank if active. Interpreted in {tz_label}." self.fields["auto_assign"].help_text = "Turn this off to opt out of automatic reviewer assignment for this repository." + # The two capacity gates sit side by side in the grid and are easy to confuse, so each says + # which kind of limit it is: this one bounds the PRs held at once (stock), the next bounds + # how fast new ones arrive (flow). Without this, `maximum_capacity` renders as a bare + # unexplained number beside a fully-annotated neighbour. + self.fields[ + "maximum_capacity" + ].help_text = "How many assigned PRs you can hold at once. Auto-assignment pauses while you are at this number." # Label and help text both name the window from the setting that defines it, so the copy # cannot drift from the mechanism — and both say "N days" rather than "per week", because # the window is rolling (design doc 054). From 9a62f9d4f7c52b61f5e2e583bbe562e0394ace9b Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 23:01:43 -0400 Subject: [PATCH 11/12] fix(console): keep the stale-PR escalation thresholds in one section MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `stale_nudge_days` sat under Notifications and `auto_unassign_days` under Auto-Assignment, but they are one escalation ladder: nudge at X consecutive queue days, unassign at Y, `parse_notification_policy` clamps Y > X, and `clean()` rejects the pair otherwise. Split across sections, the resulting error — "Auto-unassign days must be greater than stale nudge days" — rendered on a field whose partner was off screen. The obvious repair, moving auto-unassign under Notifications, would have been wrong. `needs_auto_unassign` is computed with no reference to the reviewer's `notifications_enabled`; only the global enforcement flag gates it, while `needs_nudge` is a notification and does honour the toggle. Filing the unassign under "Notifications" would tell a reviewer that switching notifications off stops them being unassigned, which is false. The pair fits neither neighbour, which is why it got split, so it gets its own "Stale PRs" section between them, and each half's help text now names the switch that governs it. Auto-assign and the notifications toggle become full-width master switches, leaving no orphan half-cells in the grid. No behavior change: same fields, same validation, same storage. Co-Authored-By: Claude Opus 5 (1M context) --- .../console/tests/test_prefs_form_fields.py | 23 ++++++++++ qb_site/core/forms.py | 14 ++++-- .../shared/_reviewer_prefs_fields.html | 44 ++++++++++++------- 3 files changed, 63 insertions(+), 18 deletions(-) diff --git a/qb_site/console/tests/test_prefs_form_fields.py b/qb_site/console/tests/test_prefs_form_fields.py index 7141fc7c..49b7d34f 100644 --- a/qb_site/console/tests/test_prefs_form_fields.py +++ b/qb_site/console/tests/test_prefs_form_fields.py @@ -102,6 +102,7 @@ def test_get_renders_the_expected_sections_and_fields(self) -> None: self.assertEqual(response.status_code, 200) # Stable semantics (sections + field names), not exact help-text wording. self.assertContains(response, "Auto-Assignment") + self.assertContains(response, "Stale PRs") self.assertContains(response, "Notifications") self.assertContains(response, "Interests") for field in ( @@ -119,6 +120,28 @@ def test_get_renders_the_expected_sections_and_fields(self) -> None: self.assertIn(f'name="form-0-{field}"', body) self.assertLess(body.index("Free form"), body.index("Conflict of interest")) + def test_escalation_thresholds_render_in_one_section(self) -> None: + """`stale_nudge_days` and `auto_unassign_days` are one ladder with a cross-field rule. + + They used to sit in different sections, so "Auto-unassign days must be greater than stale + nudge days" rendered on a field whose partner was off screen. Assert no section boundary + falls between them. + """ + body = self.client.get(self.url).content.decode("utf-8") + + start = body.index('name="form-0-stale_nudge_days"') + end = body.index('name="form-0-auto_unassign_days"') + self.assertLess(start, end) + self.assertNotIn("prefs-section", body[start:end]) + + def test_escalation_help_names_each_half_s_switch(self) -> None: + # The nudge honours the notifications toggle; the auto-unassign does not. A reviewer who + # turns notifications off would otherwise expect to stop being unassigned too. + response = self.client.get(self.url) + + self.assertContains(response, "Only sent when notifications are on") + self.assertContains(response, "whether or not notifications are on") + @override_settings(ANALYZER_ASSIGNMENT_PROPOSALS_ENABLED=True) def test_get_shows_assignment_acceptance_when_proposals_enabled(self) -> None: response = self.client.get(self.url) diff --git a/qb_site/core/forms.py b/qb_site/core/forms.py index d91eecc5..69427d5f 100644 --- a/qb_site/core/forms.py +++ b/qb_site/core/forms.py @@ -235,10 +235,18 @@ def __init__( self.fields["free_form"].help_text = format_html( "A free form description of your reviewing interests. {}", community_team_page_warning ) - self.fields["stale_nudge_days"].help_text = "Send a nudge when a PR has stayed on queue this many consecutive days." + # One escalation ladder (nudge at X days, unassign at Y > X), but the two halves answer to + # different switches, which is exactly what the old split-across-sections layout hid: the + # nudge is a notification and honours the toggle below, while the auto-unassign is an + # assignment action that runs regardless of it. Each says which, because a reviewer who + # turns notifications off would otherwise reasonably expect to stop being unassigned too. self.fields[ - "auto_unassign_days" - ].help_text = f"Automatically unassign after this many consecutive queue days (maximum {MAX_AUTO_UNASSIGN_DAYS})." + "stale_nudge_days" + ].help_text = "Nudge you when a PR has stayed on queue this many consecutive days. Only sent when notifications are on." + self.fields["auto_unassign_days"].help_text = ( + f"Unassign you after this many consecutive queue days (maximum {MAX_AUTO_UNASSIGN_DAYS}). " + "Must be greater than the nudge threshold, and happens whether or not notifications are on." + ) policy = parse_notification_policy(self.instance.notification_settings) self.initial["stale_nudge_days"] = policy.stale_nudge_days diff --git a/qb_site/templates/shared/_reviewer_prefs_fields.html b/qb_site/templates/shared/_reviewer_prefs_fields.html index 08b9dadb..81e6728c 100644 --- a/qb_site/templates/shared/_reviewer_prefs_fields.html +++ b/qb_site/templates/shared/_reviewer_prefs_fields.html @@ -26,20 +26,13 @@

{{ form.instance.repository.owner }}/{{ form.instance.repository.name }}

Auto-Assignment

-
+
{{ form.auto_assign }} {% if form.auto_assign.help_text %}{{ form.auto_assign.help_text }}{% endif %} {% if form.auto_assign.errors %}
    {% for error in form.auto_assign.errors %}
  • {{ error }}
    • {% endfor %}
{% endif %}
-
- - {{ form.auto_unassign_days }} - {% if form.auto_unassign_days.help_text %}{{ form.auto_unassign_days.help_text }}{% endif %} - {% if form.auto_unassign_days.errors %}
    {% for error in form.auto_unassign_days.errors %}
  • {{ error }}
    • {% endfor %}
{% endif %} -
-
@@ -88,22 +81,43 @@

Auto-Assignment

+ {% comment %} + The nudge/unassign pair is one escalation ladder — nudge at X consecutive queue days, + unassign at Y, and the form enforces Y > X. It gets its own section because it fits neither + neighbour: the nudge is delivered as a notification (so the Notifications toggle governs it) + while the auto-unassign is an assignment action that runs regardless of that toggle. Split + across the two sections, the cross-field error landed on a field whose partner was off + screen. Keep them adjacent. + {% endcomment %} +
+

Stale PRs

+
+
+ + {{ form.stale_nudge_days }} + {% if form.stale_nudge_days.help_text %}{{ form.stale_nudge_days.help_text }}{% endif %} + {% if form.stale_nudge_days.errors %}
    {% for error in form.stale_nudge_days.errors %}
  • {{ error }}
    • {% endfor %}
{% endif %} +
+ +
+ + {{ form.auto_unassign_days }} + {% if form.auto_unassign_days.help_text %}{{ form.auto_unassign_days.help_text }}{% endif %} + {% if form.auto_unassign_days.errors %}
    {% for error in form.auto_unassign_days.errors %}
  • {{ error }}
    • {% endfor %}
{% endif %} +
+
+
+

Notifications

-
+
{{ form.notifications_enabled }} {% if form.notifications_enabled.help_text %}{{ form.notifications_enabled.help_text }}{% endif %} {% if form.notifications_enabled.errors %}
    {% for error in form.notifications_enabled.errors %}
  • {{ error }}
    • {% endfor %}
{% endif %}
-
- - {{ form.stale_nudge_days }} - {% if form.stale_nudge_days.help_text %}{{ form.stale_nudge_days.help_text }}{% endif %} - {% if form.stale_nudge_days.errors %}
    {% for error in form.stale_nudge_days.errors %}
  • {{ error }}
    • {% endfor %}
{% endif %} -
From ef1e0c1b5fac567add99df6eadfd6cf475a0ff94 Mon Sep 17 00:00:00 2001 From: Bryan Gin-ge Chen Date: Thu, 27 Aug 2026 23:54:49 -0400 Subject: [PATCH 12/12] =?UTF-8?q?doc:=20finalize=20054=20=E2=80=94=20deplo?= =?UTF-8?q?yed=20to=20production,=20no=20limits=20set?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Status moves from implemented to deployed. The deploy needed no settings change: the one new setting defaults to 7 and there is no feature flag, so it was a schema change plus inert code. Records the one genuine hazard — an unapplied `core.0008` would take the nightly run, the console, `assigned-prs` and `suggest-prs` down together, since `build_reviewer_catalog` selects the new column — and that Heroku's release phase makes it self-guarding. Corrects the Surfacing section to the copy that actually shipped: `last 7 days` rather than `this week`, both the under-limit and spent-budget lines, and the `maximum_capacity` help text that rendering the page turned out to require. The stock/flow distinction this doc is built on has to be legible where a reviewer sets both, not only in the engine. Fixes two claims the body was still carrying that its own progress notes had already overtaken — §§3c, 6d and 4b are all run, so every figure in the Measured Baseline is measured rather than projected. The equivalent line inside the dated second-run note is left alone; it was true when written. Co-Authored-By: Claude Opus 5 (1M context) --- .../054-assignment-rate-limit.md | 82 ++++++++++++++++--- 1 file changed, 69 insertions(+), 13 deletions(-) diff --git a/docs/design-decisions/054-assignment-rate-limit.md b/docs/design-decisions/054-assignment-rate-limit.md index 0a86804c..a8b018e9 100644 --- a/docs/design-decisions/054-assignment-rate-limit.md +++ b/docs/design-decisions/054-assignment-rate-limit.md @@ -1,10 +1,12 @@ # Reviewer Assignment Rate Limit (Rolling Weekly Intake Cap) -> Status: **Implemented, not yet enabled for anyone** (2026-08-28). Chunks 1–8 of the -> [Implementation Plan](#implementation-plan-chunks) have landed; the code is inert until a reviewer -> sets `max_new_assignments_per_week`, which is also the whole rollout mechanism (no feature flag, -> Open Question 5). Still to do: a pilot cohort, and the engine simulation Open Question 3 asks for -> before any global default. The measurement probe was run against production first; see +> Status: **Deployed to production, no limits set** (2026-08-28). Chunks 1–8 of the +> [Implementation Plan](#implementation-plan-chunks) have landed and shipped; every +> `max_new_assignments_per_week` is still `NULL`, so the push pipeline behaves exactly as it did +> before. That null default is the whole rollout mechanism — there is no feature flag (Open +> Question 5), so enabling is an edit to one field per reviewer and clearing it is the rollback. +> Still to do: a pilot cohort, and the engine simulation Open Question 3 asks for before any global +> default. The measurement probe was run against production before any code landed; see > [Measured Baseline](#measured-baseline-2026-08-28), which confirms the premise and re-sized > several claims. Origin: a Zulip thread (Christian Merten, with Yaël Dillies' earlier proposal and > Bryan Gin-ge Chen) on making reviewer capacity limits actually bind. @@ -221,9 +223,9 @@ of the 32 active reviewers have crossed it in any rolling week. Folded into [Surfacing](#surfacing--extend-the-honest-load-line). §3c and §6d were added after the first run to answer exactly these two questions and were run the same -day; their results are folded in above. One refinement is still outstanding: **§4b**, the 90-day -replay restricted to the last 30 days, which will show how much of §4's cost estimate is rollout -residue. It is in the script and has not been run. +day; **§4b**, the 90-day replay restricted to the last 30 days, followed in a third run. All three are +folded in above — and §4b refuted the prediction this doc had been carrying, which is why the cost +figures moved *up* rather than down once the rollout period was excluded. ## Goals / Non-Goals @@ -346,19 +348,35 @@ The concurrent load line (`reviewer_load.format_load_line`, shown by the `assign the daily attention DM, and the console) gains the weekly figure, e.g.: ``` -Load: 6 / 10 · this week: 4 / 5 +Load: 3 / 10 (7 free) · last 7 days: 4 / 5 +Load: 3 / 10 (7 free) · last 7 days: 5 / 5 ⚠ weekly limit reached ``` Computed from the same `recent_assignment_counts` service so the parts agree. This is load-bearing UX, not decoration (same lesson as `053`'s Invariant 7): when the push goes quiet because the weekly limit is hit, the reviewer needs to see *why*, and the line is where they see it — along with the implicit -nudge that `suggest-prs` will still serve them if they want more now. +nudge that `suggest-prs` will still serve them if they want more now. The second line above is the +state this feature exists to create and the one that most needs explaining: plenty of concurrent room, +no new work arriving. A reviewer with no limit gets the old line back byte-for-byte. + +The copy says **"last 7 days", not "this week"** (an earlier draft of this section said the latter), +and the day count is read from `ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS` rather than written out, so it +cannot drift from the window actually being enforced. The window is rolling; "this week" invites the +calendar reading and, with it, a "why am I blocked, it's Monday" bug report. The same number belongs **next to the field in the console preferences form**, before a limit is set. Measured intake is a median of 2/week against a median peak week of 6 ([Measured Baseline](#measured-baseline-2026-08-28)), so a reviewer choosing a number blind will pick -badly in either direction. Showing "you've received N new PRs in the last 7 days" beside the input -costs nothing extra — the load line already computes exactly that count. +badly in either direction. Showing "You have been assigned N new PRs in the last 7 days" beside the +input costs nothing extra — the load line already computes exactly that count. + +One thing rendering the page revealed that the design did not anticipate: the new field lands beside +`maximum_capacity` in the form's two-column grid — the right cell, stock next to flow — but +`maximum_capacity` has **never had help text**. An annotated field beside a bare one reads as an +oversight, and nothing on the page said these were two different *kinds* of limit. So +`maximum_capacity` gained a one-line explanation naming it as the concurrent hold. The stock/flow +distinction this doc is built on has to be legible on the surface where a reviewer sets both, not just +in the engine. ## Subtleties / Invariants @@ -559,7 +577,8 @@ provenance marker `053` deferred. The script was validated against a seeded local Postgres 16 in both login modes (aggregates checked against hand-built fixtures) and **run against production on 2026-08-28** — [Measured Baseline](#measured-baseline-2026-08-28). §§3c and 6d were added afterwards, prompted by -that run, and have not yet been run against production. +that run, and §4b after those; all three were run against production the same day, so every figure in +the baseline is measured rather than projected. ## Operational Notes @@ -737,3 +756,40 @@ that run, and have not yet been run against production. steps were reproduced individually against the dockerized Postgres. The three `syncer.tests.tasks.test_commit_history_tasks` errors are the documented bare-host `GH_TOKEN` absence, unrelated to this change. +- **2026-08-28 (preferences page reviewed, two fixes)** — rendering `/console/preferences/` rather + than reasoning about it caught something the design had not: the rate-limit field lands beside + `maximum_capacity` in the two-column grid, which is the right cell, but `maximum_capacity` has no + help text and the new field had 247 characters of it — a bare number beside a wall of prose, with + nothing saying they are different *kinds* of limit. Fixed by giving `maximum_capacity` a one-line + explanation and cutting the new field's text to 113 characters (168 with the measured-intake + sentence) by dropping the half that restated its own label. Every other help text on the page is + 59–97 characters, so the pair now differs by about a line instead of four. + [Surfacing](#surfacing--extend-the-honest-load-line) updated to the shipped copy. + + A second, unrelated grouping bug on the same page was fixed in its own commit and is recorded here + only because it was found by this work: `stale_nudge_days` and `auto_unassign_days` are one + escalation ladder with a cross-field rule (`Y > X`) but sat in different sections, so the + validation error landed on a field whose partner was off screen. The obvious repair — moving + auto-unassign under Notifications — would have been wrong: `needs_auto_unassign` is computed with + no reference to the reviewer's `notifications_enabled` (only the global enforcement flag gates + it), so filing it there would tell reviewers that switching notifications off stops them being + unassigned. It does not. The pair fits neither section, which is why it got split, so it now has + its own "Stale PRs" section and each half's help text names the switch that governs it. Nothing to + do with 054's mechanism. +- **2026-08-28 (deployed)** — shipped to production. No settings change was required: the one new + setting (`ANALYZER_ASSIGNMENT_RATE_WINDOW_DAYS`) defaults to 7 and there is no feature flag, so + the deploy is a schema change plus inert code. The migration rides Heroku's release phase + (`Procfile`), which is the one genuine deploy hazard worth naming — `build_reviewer_catalog` + selects the new column, so an unapplied `core.0008` would take the nightly assignment run, the + console, `assigned-prs` and `suggest-prs` down together rather than degrading quietly. A failed + release phase aborts the deploy, so this is self-guarding. + + Behavior with every limit still `NULL` is unchanged in the ways that matter — the engine gate + short-circuits, `format_load_line` returns its previous string byte-for-byte, the persisted trace + drops the empty `at_rate_limit` key, and the `reviewer-topics.json` export omits an unset limit. + Two things did change for everyone: the preferences page looks different (new field, new "Stale + PRs" section, new help text), and `build_reviewer_catalog` now runs one extra grouped count query + per call whether or not anyone has a limit — negligible against an 859-row table, but not zero. + + Next: set a low limit on a pilot reviewer, confirm the gate binds and the load line explains it, + then re-measure §8 now that `053` has real usage before deciding Open Question 4.