Resolves #17163
Refs #17141
The managed review budget stops counting reviews and starts counting families. One ordinary Request Changes per canonical reviewer family, charged to the authenticated submitter, refused outright when that submitter cannot be placed — and the escape hatch now has to name four facts, one of which is checked against the PR's own history rather than against itself.
Evidence: L2 (unit-level: pure resolver + service admission driven through managePrReview with stubbed GraphQL) → L2 required (every AC is a decision made inside the admission path; no host, UI, or deployment effect is involved). Residual: AC8 cohort receipt, Residual-Owner: #17141.
Deltas from ticket
The service could not name its own reviewer, and nothing in the ticket said so. fetchAndCacheViewerPermission asked GitHub who the viewer was, read the permission off the answer, and discarded the identity. A per-family budget cannot charge a round without it, so the login is now cached beside the permission it was fetched with — same authenticated call, because asking separately invites the two to disagree about who the viewer is.
The identity is the authenticated login, never parsed from review prose. Deliberately the mirror of resolveAuthorFamily, which reads the PR body self-id precisely because an opener's login can mis-resolve. On the reviewer side the submitting login is the act and the signature is a claim about it.
Family resolution consumes agentFamilyResolution.mjs rather than adding a second roster. The ticket's own architectural note says no second review tracker is needed; the same applies to the family facts it counts by. resolveReviewerFamily returns a {classified, family, login} pair rather than a bare family, because a resolver answering undefined for both "not a rostered agent" and "no login at all" hands the caller one value for two situations demanding opposite handling — and the caller then takes whichever branch reads more naturally.
The override became a receipt with a falsifiable clause. old-head must match a head some prior review was actually submitted against. Every other clause validates the receipt against itself and can be satisfied by writing it better; this one is verified against the PR's review population, so an invented history fails however well phrased.
Scope split. #17141 spans runtime admission and .agents/skills/pr-review/** payload work. This PR is the runtime half and #17141 stays open for the rest; #17163 is the honest leaf it delivers.
Test Evidence
ai/services/github-workflow/PullRequestService.mjs + ai/services/graph/agentFamilyResolution.mjs:
UNIT_TEST_MODE=true npx playwright test -c test/playwright/playwright.config.unit.mjs \
test/playwright/unit/ai/services/github-workflow/ test/playwright/unit/ai/services/graph/
→ EXIT=0, 1053 passed
Six #15257 tests encoded the superseded global-ceiling contract and are rewritten rather than deleted; four were failing only for want of a submitting identity, which the suite now seeds and restores around the singleton so a leaked viewer cannot decide another suite's budget.
New cases, each covering something no prior test could express:
| case |
what it pins |
| second family keeps its round |
identical fixture, only the seat changes — a passing result cannot come from any clause but family keying |
| unclassified submitter refused |
driven on a PR with zero prior reviews, the freest case under the old count, so the refusal can only be the unplaceable submitter |
| receipt refusal matrix |
one clause removed at a time from an otherwise-valid receipt, plus a control admitting the complete one — seven refusals with no control would pass against a validator that refused everything |
| second re-entry refused |
the exception is terminal, or it becomes the budget |
Mutation-verified individually: with the history-checked clause disabled an invented old-head is admitted; with the per-service guard removed the diagnostic returns the pre-fix shape.
Post-Merge Validation
Residual-Owner: #17141
That obligation is AC8's cohort receipt, which already owns the post-merge measurement for the terminal-review decision — an existing open ticket, not one minted to satisfy this gate.
A second item I had drafted here — confirm no rostered maintainer is refused as unclassifiable — is dropped rather than owned. It restated what the unit matrix already proves (every rostered family resolves against the LIVE roster, not a hand-built map) and named no fact only production could establish. An unfalsifiable checkbox survives forever because nobody can ever tick it.
Commits
4f23d777bc — reviewer-family resolution as a {classified, family, login} pair, consuming the identity graph38ebf2ee4a — cache the viewer login beside its permission73ad630f90 — charge the budget to the submitting family; fail closed when unnameabled41daea56f — repair-minted receipt with four checkable facts; terminal per family
Evolution
The first wiring awaited the viewer identity inside the validator. The suite rejected it immediately and removing that call fixed a failing test on its own: a validator that makes its own round trip can fail for reasons unrelated to the review it is judging. The identity is now read from the startup cache, and a cold cache resolves to null — which the budget already treats as unclassifiable and refuses, which is the safe direction.
Authored by Grace (Claude Opus 5, Claude Code). Session b17338dd-b474-494f-b08c-683044de2ddb.
Resolves #17163 Refs #17141
The managed review budget stops counting reviews and starts counting families. One ordinary Request Changes per canonical reviewer family, charged to the authenticated submitter, refused outright when that submitter cannot be placed — and the escape hatch now has to name four facts, one of which is checked against the PR's own history rather than against itself.
Evidence: L2 (unit-level: pure resolver + service admission driven through
managePrReviewwith stubbed GraphQL) → L2 required (every AC is a decision made inside the admission path; no host, UI, or deployment effect is involved). Residual: AC8 cohort receipt, Residual-Owner: #17141.Deltas from ticket
The service could not name its own reviewer, and nothing in the ticket said so.
fetchAndCacheViewerPermissionasked GitHub who the viewer was, read the permission off the answer, and discarded the identity. A per-family budget cannot charge a round without it, so the login is now cached beside the permission it was fetched with — same authenticated call, because asking separately invites the two to disagree about who the viewer is.The identity is the authenticated login, never parsed from review prose. Deliberately the mirror of
resolveAuthorFamily, which reads the PR body self-id precisely because an opener's login can mis-resolve. On the reviewer side the submitting login is the act and the signature is a claim about it.Family resolution consumes
agentFamilyResolution.mjsrather than adding a second roster. The ticket's own architectural note says no second review tracker is needed; the same applies to the family facts it counts by.resolveReviewerFamilyreturns a{classified, family, login}pair rather than a bare family, because a resolver answeringundefinedfor both "not a rostered agent" and "no login at all" hands the caller one value for two situations demanding opposite handling — and the caller then takes whichever branch reads more naturally.The override became a receipt with a falsifiable clause.
old-headmust match a head some prior review was actually submitted against. Every other clause validates the receipt against itself and can be satisfied by writing it better; this one is verified against the PR's review population, so an invented history fails however well phrased.Scope split. #17141 spans runtime admission and
.agents/skills/pr-review/**payload work. This PR is the runtime half and #17141 stays open for the rest; #17163 is the honest leaf it delivers.Test Evidence
ai/services/github-workflow/PullRequestService.mjs+ai/services/graph/agentFamilyResolution.mjs:Six
#15257tests encoded the superseded global-ceiling contract and are rewritten rather than deleted; four were failing only for want of a submitting identity, which the suite now seeds and restores around the singleton so a leaked viewer cannot decide another suite's budget.New cases, each covering something no prior test could express:
Mutation-verified individually: with the history-checked clause disabled an invented
old-headis admitted; with the per-service guard removed the diagnostic returns the pre-fix shape.Post-Merge Validation
REQUEST_CHANGESfrom each active family confirms the audit block recordsreviewerFamilyandfamilySubmittedRequestChangesas observed on a real PR.Residual-Owner: #17141
That obligation is AC8's cohort receipt, which already owns the post-merge measurement for the terminal-review decision — an existing open ticket, not one minted to satisfy this gate.
A second item I had drafted here — confirm no rostered maintainer is refused as unclassifiable — is dropped rather than owned. It restated what the unit matrix already proves (every rostered family resolves against the LIVE roster, not a hand-built map) and named no fact only production could establish. An unfalsifiable checkbox survives forever because nobody can ever tick it.
Commits
4f23d777bc— reviewer-family resolution as a{classified, family, login}pair, consuming the identity graph38ebf2ee4a— cache the viewer login beside its permission73ad630f90— charge the budget to the submitting family; fail closed when unnameabled41daea56f— repair-minted receipt with four checkable facts; terminal per familyEvolution
The first wiring awaited the viewer identity inside the validator. The suite rejected it immediately and removing that call fixed a failing test on its own: a validator that makes its own round trip can fail for reasons unrelated to the review it is judging. The identity is now read from the startup cache, and a cold cache resolves to
null— which the budget already treats as unclassifiable and refuses, which is the safe direction.Authored by Grace (Claude Opus 5, Claude Code). Session b17338dd-b474-494f-b08c-683044de2ddb.