Route requests to the nearest eligible reviewers

.claude/CLAUDE.md

@@ -498,15 +498,34 @@ role membership and the direct grant are not collapsed into one record.
 
 ## Approver routing
 
-For all request types, approval routing follows this chain — this affects notification plugin
-code and any logic that determines who can approve:
-
-1. **Group/role owner** — designated owner(s) of the target group or role; groups may be unowned
-2. **App owner** — if the target is an `AppGroup` with no group owners, falls back to app owner; apps may also be unowned
-3. **Access admin** — final fallback if no owners exist at any prior tier
-
-Implemented in `api/models/okta_group.py` (`get_group_managers`) and
-`api/models/app_group.py` (`get_app_managers`, `get_access_owners`).
+Who reviews a request is defined once, in `api/models/request_reviewers.py`. Notifications
+(open and close), the "Assigned to Me" views, and the request detail pages all read it; never
+rebuild the chain elsewhere.
+
+Each request has owner levels, nearest first:
+
+| Request | Owner levels |
+|---|---|
+| Access request, individual or role-based, for group G | owners of G → owners of G's app (app groups only) → Access admins |
+| Group request to create G | owners of the requested app (app groups only) → Access admins |
+
+An **eligible reviewer** owns at a level and is not the requester, not deleted, and (unless an
+Access admin) not blocked by a `disallow_self_add_*` tag: approving a role-based request would
+add a reviewer who is a member of the requesting role to G. This mirrors
+`CheckForSelfAdd.execute_for_role`. A request's **assigned reviewers** are the eligible reviewers
+at the nearest level that has any. The detail pages list the **eligible reviewers by level**
+(`get_eligible_reviewers_by_level`): nearest first, each user once at their nearest level, levels
+with no eligible reviewer left out. The assigned reviewers are therefore listed first, and the
+rest show a requester who to contact to escalate.
+
+Close notifications go to the reviewers assigned while the request was open, so an operation that
+changes ownership or deletes a group takes that snapshot (`snapshot_assigned_reviewers`, or
+`snapshot_obsolete_request_reviewers` for a group's pending requests) before it makes the change.
+
+Assignment is not authorization. Who may approve or reject is checked by the resolve routes
+(`can_manage_group`, the self-approval guard, `CheckForSelfAdd`, and the group-request
+admin/app-owner checks); an Access admin can resolve any request. Every assigned reviewer can
+approve; `tests/test_request_reviewers.py` checks that.
 
 ## Authorization
 
@@ -586,8 +605,7 @@ owner, app owner (if app group), or Access admin.
 
 **`GroupRequest`** — a user requests creation of a new group, app group, or role. A new `App`
 cannot be requested via this flow. On approval, the requester is added as an owner. App group
-requests require selecting a parent app and route to the app's owners if they exist, otherwise
-to Access admins; vanilla group and role requests route to Access admins.
+requests require selecting a parent app. For who reviews it, see Approver routing.
 
 ## Plugin system
 
@@ -700,3 +718,6 @@ infrastructure details live there too.
   and role adds too
 - **Querying tags without filtering for enabled** — disabled tags still exist in the DB
 - **Putting operator-specific logic in the Access repo** — it belongs in an operator's own plugin
+- **Rebuilding the reviewer chain** — use `api/models/request_reviewers.py`
+  (`get_assigned_reviewers`, `is_assigned_reviewer`) instead of chaining
+  `get_group_managers` / `get_app_managers` / `get_access_owners`

README.md

@@ -309,11 +309,10 @@ A value of `0` or less is rejected at startup: it would close every pending
 request on the next sync, and is almost always a mistaken attempt to disable
 expiration. Use `never` for that.
 
-Closing a request notifies its requester and every approver eligible to review
-it; for a role request that includes Access admins. A sync run that closes many
-requests at once sends a correspondingly large number of notifications, so on a
-busy instance it is worth knowing how many requests are pending past a cutoff or
-past their requested end date.
+Closing a request notifies its requester and its assigned reviewers. A sync run
+that closes many requests at once sends a correspondingly large number of
+notifications, so on a busy instance it is worth knowing how many requests are
+pending past a cutoff or past their requested end date.
 
 ##### Requiring request reasons
 
@@ -619,7 +618,7 @@ As of Access 2.0 the plugin hooks are **async** — Access is an async applicati
 - **Delivery is not guaranteed, and `sync_group` is only as much of a safety net as you make it.** A worker killed between the response and the drain drops the fire, as does a request that fails after queuing one. Access does not police what `sync_group` does, so a plugin that implements it as a full idempotent reconciliation converges afterwards and one that does not, does not. One gap it cannot close either way: it sees only current membership, so a delta-based revoke cannot recover a lost `group_members_removed`. If your plugin records events rather than converging state — an audit or notification plugin — expect gaps.
 - **`group_deleted` is the exception and runs inline, on the request.** The sweep skips soft-deleted groups, so nothing would revisit a delete that went missing, and the external group would outlive the Access one. Your `group_deleted` therefore blocks the API response — keep it as quick as the external system allows — and the only way to lose it is to raise.
 - **Whatever a hook writes is absent from the create/update response the user just got.** Declare `pending_values` on the status property that carries your reconcile state (e.g. `pending_values=("pending",)` on a `sync_status`) and the UI keeps refreshing the group until it moves off them. If you declare them, write a status on *every* reconcile outcome, including the ones where you decide there is nothing to do — an empty status reads as "the hook has not run yet" and the UI will poll for it.
-- **Request-path notification hooks receive read-only, detached ORM snapshots.** Every notification raised from an HTTP request — the `*_created` and `*_completed` hooks for access, role, and group requests — runs in a post-response background task, after the request's DB session has been torn down, so Access detaches (`expunge`s) the objects it passes. Read only the attributes Access already loaded — the request, its group/role, a group request's `requested_app`, the requester, and the approvers, and their columns — and don't trigger a lazy load / re-query (there's no live session behind them) or expect mutations to persist. If your notifier needs a wider object graph than what's passed, open an issue so the operation can eager-load it. (The `access_expiring_*` hooks are the exception — they run inline in the syncer CronJob.)
+- **Request-path notification hooks receive read-only, detached ORM snapshots.** Every notification raised from an HTTP request — the `*_created` and `*_completed` hooks for access, role, and group requests — runs in a post-response background task, after the request's DB session has been torn down, so Access detaches (`expunge`s) the objects it passes. Read only the attributes Access already loaded — the request, its group/role, a group request's `requested_app`, the requester, and the approvers, and their columns — and don't trigger a lazy load / re-query (there's no live session behind them) or expect mutations to persist. `approvers` is the request's assigned reviewers in every request hook; see `NotificationPluginSpec`. If your notifier needs a wider object graph than what's passed, open an issue so the operation can eager-load it. (The `access_expiring_*` hooks are the exception — they run inline in the syncer CronJob.)
 - **Log through `logging.getLogger(__name__)`.** Access attributes a log line to your plugin by matching the logger name against your plugin's top-level package, so lines from the root logger (`logging.warning(...)`) or a logger named outside your package show up as `lib:<name>` instead of `plugin:<package>`. See `LOG_FORMAT` above.
 - **Metrics `batch_metrics` is an async context manager** — use it as `async with metrics.batch_metrics(): await metrics.record_counter(...)`.
 - **These hooks stay synchronous:** the app-group-lifecycle metadata/config-schema/status-schema/validation hooks (`get_plugin_metadata`, `get_plugin_*_config_properties`, `validate_plugin_*_config`, `get_plugin_*_status_properties`). They are pure schema accessors with no I/O.

api/models/core_models.py

@@ -657,7 +657,9 @@ class AppGroup(OktaGroup):
     __tablename__ = "app_group"
     id: Mapped[str] = mapped_column(Unicode(50), ForeignKey("okta_group.id"), primary_key=True)
 
-    app_id: Mapped[str] = mapped_column(Unicode(50), ForeignKey("app.id"), nullable=False)
+    # Indexed for the lookups of an app's groups and its owners group, which
+    # Postgres would otherwise seq-scan (it does not index FK columns).
+    app_id: Mapped[str] = mapped_column(Unicode(50), ForeignKey("app.id"), nullable=False, index=True)
     # Is this group the app owner group and can administer the app and other app groups?
     # Membership to an app onwer group implicitly grants group owner permissions on the
     # group to administer and manage membership of the app owner group