feat: on-demand assignment suggestions - #196
Merged
Merged
Conversation
Living implementation plan for a reviewer-initiated "what should I review?" surface in Zulip and the console, plus the read-only probes used to calibrate it against production. The design is measured rather than assumed. Against mathlib4 the assignable pool is 516 PRs while the nightly engine places 11, because 29 of 57 reviewers sit at capacity and 21 are unavailable by preference. Every reviewer is reachable by exactly one override, and the groups are disjoint: 20 need the availability override, 29 the capacity override, 3 a label override. Decisions that followed from the measurement: - Capacity is overridden unconditionally. It is a push-throttle like auto_assign and away_until, not a correctness rule; gating it behind a second signal would have been a rubber stamp for a majority of reviewers and a dead end on Zulip, which has nowhere to put a toggle. The load line becomes the honest capacity signal instead. - Limits are 10 on the console and 5 in Zulip, with a console link for the rest. Results are deterministic within a snapshot generation, so the Zulip list is a strict prefix of the console list. - The engine is not modified. Inverting it is a profile substitution, verified to agree with the live engine on all 29,412 (reviewer, PR) pairs. - A request is ~476 ms, of which ~411 ms is the snapshot payload read and ~85 ms is all engine compute. The cache worth building is therefore the payload keyed on (repo_id, cache_key, generated_at), not a per-reviewer result cache; it is deferred and belongs to reviewer_load. No runtime code yet. All flags will default off, following the 046/050 staged-rollout discipline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Chunks 1+2 of design doc 053. New read-only service analyzer.services.assignment_suggestions.suggest_prs_for_reviewer: ranks the shared assignable pool and keeps the PRs where the requesting reviewer lands in the engine's own available list, with the push throttles (away_until, auto_assign, maximum_capacity) overridden via a dataclasses.replace profile substitution and correctness rules (authorship, conflicts, opt-outs, cooldowns) untouched. The load line reports the real preference capacity, never the override. Includes the skip tally (no_topic_label/authored/conflict_of_interest/no_area_match/ outranked/excluded) plus one reason beyond the doc's table: already_assigned, because the pool's active-assignee filter reads real availability, so an away/auto-assign-off requester's own assigned PRs survive into the pool and must not be suggested back to them. Promotes _prepare_assignment_inputs to public prepare_assignment_inputs (shared-pool invariant 5) and updates the 053 probes for the rename. Settings are wired in the same commit (service reads LIMIT/MAX_LABELS, so splitting would create phantom settings): all five ANALYZER_ASSIGNMENT_SUGGESTIONS_* knobs in base.py + .env.example, flags default off per the 046/050 rollout discipline. Tested: analyzer suggestion/assignment/load suites (125 tests) against dockerized Postgres; ruff check/format clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Chunk 4 of design doc 053 (built before the Zulip chunk so the footer's console:suggestions reverse resolves). /console/suggestions/ renders suggest_prs_for_reviewer per repo — load line from the reviewer's real capacity, label-override search form, matched-label highlighting, scarcity/queue-age/load-contribution per row, and the shared skip-tally line (format_skip_summary, added to the service so both surfaces render one phrasing). ?repo=/?labels= pre-fill from the Zulip footer link and are validated, never trusted. POST /console/suggestions/claim/ re-verifies every posted PR number against a fresh suggestion run under the same label override before any GitHub write (Invariant 6), assigns only the session reviewer's own login via the 046 path (assign_reviewer_and_record, snapshot=None), renders an assigned/failed split, and surfaces co-assignees (no hold, Invariant 8). Read path gated by ANALYZER_ASSIGNMENT_SUGGESTIONS_ENABLED, the write additionally by ..._CONSOLE_CLAIM_ENABLED. Home dashboard gains the 'Find PRs to review' entry point, including in the previously dead-end empty state. Console AGENTS.md updated. Tested: console suite (78 tests) against dockerized Postgres — the claim re-check runs the real service over seeded snapshots; GitHub mocked. ruff check/format clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Chunk 3 of design doc 053. New in-place command `suggest-prs [<owner/repo>] [<label> ...]` (aliases next-pr, suggest-pr): renders the shared assignment_suggestions service — load line, up to ANALYZER_ASSIGNMENT_SUGGESTIONS_ZULIP_LIMIT one-line PR entries, and a footer with the assign #NNN hint, the snapshot timestamp (a channel message is permanent; the console page is live) and a token-less console link carrying ?repo=&labels= so 'more suggestions' continues the same question. Label tokens replace the sender's stored preferred_labels for the request; unknown/non-topic labels are reported back; an empty result renders the skip tally. In-place reply by design (the follow-up assign is in-place too); the reply still passes through split_message_chunks as belt-and-braces, falling back to proactive sends into the same conversation if it ever exceeds one message. Gated by ANALYZER_ASSIGNMENT_SUGGESTIONS_ENABLED; claiming reuses the existing assign command and its flags. zulip_bot AGENTS.md updated. Tested: zulip_bot suite against dockerized Postgres (incl. the live registry dispatchability test); ruff check/format clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Chunk 5 of design doc 053. Marks the doc Implemented (flags dark), documents the already_assigned skip reason the implementation added (the pool's active-assignee filter reads real availability, so an away/auto-assign-off requester's own PRs survive into the pool), and records the other implementation deviations: chunks 1+2 merged to avoid a phantom-settings window, console built before Zulip for the reverse() dependency, format_skip_summary shared via the service, LabelDef-backed unknown-label validation, and the unbounded claim re-check limit. Adds assignment_suggestions.py to the analyzer AGENTS.md service list. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s (053) Three ways the suggestion service could quietly report something untrue. Scarcity: `available_reviewer_count` was read from the ranking's `details`, which is computed over the *override* catalog where the requester is unconditionally available. That counted them as supply even when really at capacity or away — overstating by one to exactly the reviewer reading it, and most where it matters. It could never report 0. Production has 22 of 50 labels with a single interested reviewer, so the sole candidate often *is* the requester: "1 available reviewer" where the truth was 0 is the common case, not an edge case. Now recomputed against the real catalog, bounded by `limit`. The ranking deliberately stays on the override catalog. Trace contract: `_classify_skip` reads `trace["potential"]`, but three early-return paths never set it. Correct today only because the classifier checks topic-label/author/COI first and never reaches the read. `_pr_trace_base` now seeds the key so the invariant holds structurally, not by luck of ordering; reorder those checks and skips would have silently misattributed to `outranked`. Stale snapshots: with no *active* rule set the cache key falls back to the literal "default", where a long-dead row survives (production carries one from 2026-03). The no-snapshot guard was satisfied by it, serving months-old PRs as live. New ANALYZER_ASSIGNMENT_SUGGESTIONS_MAX_SNAPSHOT_AGE_SECONDS (24h, 0 disables) refuses it, still reporting generated_at so operators see how stale. Label caps: the MAX_LABELS slice ran before the known/unknown split, so labels past the cap landed in neither list and the request was silently narrowed. Reviewers hold up to 11 stored labels against a cap of 5, and the broad label override is the biggest unlock in the measured baseline. Now reported in `dropped_labels`, kept distinct from `unknown_labels`: a typo inside the cap and a good label past it are different problems. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`Repository.objects.filter(id=request.POST.get("repo_id") or 0)` raises
ValueError inside the ORM on a non-numeric value, so a hand-rolled POST returned
a 500 instead of reaching the not-found branch. Verified against the old code:
`ValueError: Field 'id' expected a number but got 'not-a-number'`.
Extracted `_repo_from_post` with an isdigit() guard. Applied to `claim` and also
to `unassign`, which carried the identical pattern and is already live behind its
own flag — the GET view guarded this, these two did not.
Also renders the new `dropped_labels` so a reviewer who pastes more labels than
the cap allows is told, rather than silently getting a narrower answer.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
suggest-prs reported `unknown_labels` but said nothing about labels dropped by ANALYZER_ASSIGNMENT_SUGGESTIONS_MAX_LABELS, so asking for eight labels silently answered for five. Kept as its own line, distinct from the not-a-topic-label line: these are good labels the cap refused, and folding them together would suggest the reviewer's own labels were wrong. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both probes imported `prepare_assignment_inputs`, which exists only on this branch, so they failed on the deployed revision with: ImportError: cannot import name 'prepare_assignment_inputs' ... Did you mean: '_prepare_assignment_inputs'? These measure the *pre-deploy* baseline, so they have to run against a revision that predates their own rename — a baseline probe that only runs post-deploy cannot establish a baseline. Both now try the public name and fall back to the private one. Verified in both directions against a simulated pre-053 revision. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Re-ran both probes before enabling the flags, as the Measured Baseline section instructs. The shape holds and the cost improved: end-to-end 476ms -> 357ms, transient peak 81.6MB -> 79.8MB, assignable pool 516 -> 519 (463 with a topic label both times). engine_mismatches: 0. The 08-25 figures are left as written — they are what the design was calibrated against, and the audit trail is worth more than a tidy document. Added a delta table, the two integrity checks (engine_mismatches 0; the skip tally summing to exactly 29,583 = 57 reviewers x 519 PRs in both passes, so every pair is attributed once), and the counterfactual: with no overrides, 44 of 57 reviewers have zero eligible PRs. Replaced the stale "rank ~78 of 516" with the measured distribution under this pass's own semantics (median 8, p75 60, p90 162, max 302 of 519) and added an explicit do-not-truncate warning. The 16ms walk invites a top-N cap; that would return empty for the ~10% of reviewers whose first eligible PR ranks past 162 — precisely the narrow-area reviewers this feature exists to reach, and the failure would be indistinguishable from "nothing to review". Also documents `dropped_labels`, the new MAX_SNAPSHOT_AGE_SECONDS setting, and design notes 5b/5c for the two guards added this session. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Running the syncer suite a few times in a row started failing 35 tests. Nothing in the diff was responsible: a clean checkout reproduced it identically. `syncer.services.task_dedupe` and `syncer.services.rate_budget` keep short-TTL keys in the broker's Redis, and the dedupe keys are `(repo_id, pr_number)` -scoped. Django recreates the test database every run, so ids restart from 1 and collide with the previous run's leftovers; `sync_pr` then returns `runtime_deduped` / `recently_processed` and every test expecting real work fails. Tests were also writing those keys into whatever Redis a local `docker compose up` stack was using. The failure mode is what makes this worth fixing properly rather than noting in a README. It misleads at every step: failures land in suites unrelated to dedupe (backfill), the first run of a session is green so it looks like something you just did, each failing test passes in isolation so it reads as flakiness, and `git stash` is the only thing that disproves the obvious suspect. Two guards, because either alone leaves a gap: - TEST_RUNNER (base.py, so a host run gets it too) clears the app's namespaces before the suite starts. This is what breaks the run-to-run chain. - ci.py repoints the broker at its own Redis database index (default 15), so a run cannot touch a dev stack's keyspace. Scan-and-delete over the declared prefixes rather than FLUSHDB: it stays correct if someone runs against a shared index anyway, and can never destroy data this project did not write. Verified by poisoning the index with 2,400 keys shaped like a previous run's leftovers — the runner cleared them and the suite passed — and by four consecutive green runs where the third would previously have failed. `syncer/tests/test_redis_isolation.py` scans both key-writing modules for key literals and fails if one is not covered by SHARED_REDIS_KEY_PATTERNS, so a new prefix cannot silently reintroduce the leak. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three lessons, each of which cost real time or produced a wrong answer. Pipeline exit codes. `bash scripts/repo_check_compose.sh | tail -60` reports `tail`'s status, so a build that died before any test ran came back as exit 0 and was reported as passing. That led to a confident, repeated, wrong claim that the script swallows failures — it does not; it has `set -euo pipefail` and an explicit `exit 1` on migrate failure. The invocation was at fault. Also notes the corollary: confirm a check actually ran, rather than reading the absence of a traceback as success. Fallback coverage. "Compose is unavailable, so run focused tests" was already documented, but not *which* checks that skips. Steps 2 and 4-6 are cheap and catch what the test suites cannot — an unmigrated model change, a table missing from the backup policy. Enumerated so the fallback is a checklist rather than a judgement call, and so "which steps ran" is answerable by number. Docker credential helper. In a sandboxed environment `docker-credential-desktop` cannot write under ~/Library/Containers and takes the build down with `error getting credentials`, though every image this stack pulls is public and needs no credentials at all. Records the workaround, including the two non-obvious parts: overriding DOCKER_CONFIG also moves where the CLI finds its plugins and its contexts, so `docker compose` disappears and the desktop socket must be named explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`_get_redis_client()` connects lazily, so it returns a client object even when nothing is listening; the `is None` guard never fired and the test errored with a socket failure instead of skipping. Ping before using it. Caught by running the canonical script rather than the per-app tests: it starts `web` against `db` only, so `redis:6379` does not resolve there, while the focused loop this was developed against had Redis up. The runner itself already degraded correctly (logs a warning, continues) — only the test was brittle. Worth recording why the leak this guards against never broke the canonical script: with no Redis reachable, dedupe always fails open. It bites the workflows that do have Redis up — a full `docker compose up`, or the focused host-test loop in qb_site/AGENTS.md. Also sharpens that AGENTS.md note now that it is verified end to end: with a credsStore-free DOCKER_CONFIG the canonical script completes all 12 steps in the sandbox, image build included, so there is no need to assemble the steps by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Docker Desktop's credential helper cannot start when a sandbox blocks writes to ~/Library/Containers, and it takes every image pull down with it — reported only as `failed to solve: error getting credentials`. This project pulls nothing but public images, so the helper is pure overhead here: a DOCKER_CONFIG without "credsStore" skips it and the canonical script runs green in the sandbox, all 12 steps including the image build. Adds `docs/sandboxed_docker_setup.md` covering symptom, one-command diagnosis, root cause, setup, verification, undo, and the condition that ends its usefulness (a private-registry image would need the real helper, and therefore write access to ~/Library/Containers/com.docker.docker/). Linked from the root AGENTS.md under Configuration & Environment, where a human looks for environment setup, and from qb_site/AGENTS.md by symptom, where an agent hits the error. The config lives in the repo as a gitignored `.docker-nocreds/`, with the two variables in a gitignored `.docker-env` (`.docker-env.example` committed). Under $HOME it does not work: the sandbox cannot read a new home directory, and the failure is easy to misread, because the sandbox returns ENOENT for home paths that do not exist but EPERM for ones that exist outside its allowlist — so a freshly created ~/.docker-nocreds is indistinguishable from one never created, and both look like a permissions bug rather than a wrong location. Keeping it in the repo also means the setup can be created and verified without leaving the sandbox. Every command in the runbook was run rather than assumed, which is what surfaced the $HOME problem. Two other results worth recording: - The cli-plugins symlink points at Docker Desktop's own plugin bundle, so none of this references ~/.docker. `rm -rf` on the containing directory removes the symlink, not its target — verified explicitly, since being wrong there would delete the user's Docker CLI plugins. - A read-only config directory is sufficient; Docker only reads it. Worth stating because it contrasts with ~/Library/Containers, where read-only is *not* enough — there the helper is trying to create directories, which is why the obvious "just grant read access" instinct fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add a page to the console and a zulip command (
suggest-prs) which return up to 10 suggestions of PRs to review. A set of labels can be specified to scope the suggestions differently from the default. Reviewers then can assign themselves to those PRs manually.