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/docs/design-decisions/054-assignment-rate-limit.md b/docs/design-decisions/054-assignment-rate-limit.md new file mode 100644 index 00000000..a8b018e9 --- /dev/null +++ b/docs/design-decisions/054-assignment-rate-limit.md @@ -0,0 +1,795 @@ +# Reviewer Assignment Rate Limit (Rolling Weekly Intake Cap) + +> 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. + +## 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; **§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 + +**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: 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. 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 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 + +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) + + `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) + +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 §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 + +- **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. +- **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. +- **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. 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` 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", +] 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_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 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 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..49b7d34f 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 "" @@ -94,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 ( @@ -101,6 +110,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", @@ -110,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) @@ -197,6 +229,68 @@ 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 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( + 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, "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() i = index_by_id[self.pref1.id] 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/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/forms.py b/qb_site/core/forms.py index aa9d7360..69427d5f 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,40 @@ 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). + + 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"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." + 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 +162,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 +172,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,14 +216,37 @@ 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). + 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 ) - 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 @@ -214,6 +274,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/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_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/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() diff --git a/qb_site/templates/shared/_reviewer_prefs_fields.html b/qb_site/templates/shared/_reviewer_prefs_fields.html index e74fbeb9..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 %} -
-
@@ -63,6 +56,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 %}
@@ -81,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 %} -
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;