feat(AF-622): idle-reviewer escalation and nudge reminders - #724
Merged
Conversation
review_plans gains escalation_after_hours and nudge_interval_hours (V144), both nullable with null meaning off, so every existing plan keeps exactly today's behaviour on upgrade. Plain INT hours to match approval_timeout_hours — the codebase confines Duration to @ConfigurationProperties and never puts it on an entity. Escalation state (escalated_at, last_nudged_at) lands on the three request tables rather than the plan: it is per-request progress, and stamping it is what will make the scan job idempotent across replicas and restarts. Partial indexes cover only rows still awaiting review. Neither column widens who may approve. Escalation is notify-only by construction — the eligibility path does not read them — because idleness must never become a way around the configured approver set. The DTO ripple uses convenience constructors throughout, so every existing caller compiles unchanged. Both bootstrap fingerprint maps gained the new keys, with a test proving a plan whose escalation delay alone changed is actually reconciled — miss that and an operator's config change would silently never apply.
Two new NotificationEventType values with the full fan-out from .claude/patterns/notification-fanout.md: all ten Java switch sites, two Thymeleaf templates, subject and body keys across seven messages files, and the frontend union, bell case, route and seven locale JSONs. Three of those sites carry a default branch and would have compiled silently wrong: PagerDutyPayloadFactory, EmailNotificationStrategy's subject-args switch, and TicketDescriptionBuilder. The subject-args case is spelled out even though it matches the default, so the choice reads as deliberate rather than as an accident of the switch having one. The bell case matters more than it looks: the backend records an in-app row for every event except TEST, so without it every recipient would see a contentless 'New notification' — the exact miss #625 shipped with. Recipients differ by intent. An escalation goes to the plan's reviewers PLUS every org admin, because the whole point is that the original reviewers did not act; a nudge goes only to the reviewers already on the hook. Both keep the plan's channels rather than fanning out org-wide, matching the other review events. PagerDuty pages on escalation but not on a nudge — a reminder is not an incident.
ReviewEscalationJob closes the gap the issue names: today an approval chain stalls in silence until QueryTimeoutJob auto-rejects the request at approval_timeout_hours, and the submitter finds out by being rejected. This fires the warning shots first — escalate once past escalation_after_hours, re-nudge undecided reviewers on nudge_interval_hours. Notify-only by construction. Neither path touches the decision or eligibility code, and a test asserts the job never calls markTimedOut, recordApprovalAndAdvance or recordRejection — idleness must not become a way around the configured approver set. Both marks are stamped under the row lock in the state service, which re-checks status and prior stamp there rather than trusting the scan. That is what makes the job safe on every replica and across restarts: the stamp decides who wins, so a decision racing the scan yields no stray notification and a sibling replica cannot double-fire. Events publish inside that same transaction, so the AFTER_COMMIT listener only ever sees an escalation that actually took. Injects Clock rather than calling Instant.now(), per the scheduled-job pattern — QueryTimeoutJob and ScheduledQueryRunJob both deviate and are correspondingly untestable on 'is it due yet'. workflow/internal/scheduled had no tests at all; this adds the first.
The API-request twin of ReviewEscalationJob, scanning api_requests against their connector's review plan. It lives in apigov rather than workflow because api_requests is owned there — the same reason ApiRequestTimeoutJob sits beside QueryTimeoutJob instead of inside it. Recipients follow intent, not symmetry: a nudge reminds exactly the people API_REQUEST_SUBMITTED alerted, while an escalation adds admins, because the point of escalating is that those reviewers did not act. Same idempotency shape as the query job — status and prior stamp re-checked before the write, events published in that transaction — so a decision racing the scan yields no stray notification and replicas cannot double-fire.
A bundle has no review plan of its own, so its window is the MINIMUM non-null escalation_after_hours across its members' plans — the strictest member decides, matching the weakest-link union GroupReviewPlanResolver already applies to approvers. A member whose plan has escalation off contributes nothing to that minimum rather than switching it off for the whole bundle, which is why the query carries an EXISTS guard alongside the MIN: COALESCE(MIN(...), 0) alone would make every bundle instantly due. Stamps only, and publishes no event. The requestgroups module has no notification path whatsoever — no listener, no context builder — so grouped requests have never emitted a submitted or approved message either. Publishing events nothing consumes would be dead code, so escalation raises the flag the review queue reads and channel fan-out arrives when grouped-request notifications do. The javadoc says so rather than implying a delivery that does not happen.
Adds the escalation banner on the query detail page, and the two optional plan settings that drive it. Both settings default to empty on create and are never prefilled from a template: a plan that starts nagging reviewers because someone picked a template would be a surprise, and null is the documented 'off' value. A cleared field submits null rather than undefined, so clearing one on an existing plan actually turns it off instead of leaving the old value. The banner shows only while the request is still awaiting a decision, and its copy says plainly that escalating does not change who may decide — tested, because a banner that reads like an approval would be worse than none. admin-review-plans.spec.ts reaches the modal's numeric inputs positionally via .ant-input-number-input, and its index map is now two entries longer. The assertion itself uses .last() so it still resolves the stage row, but the map was documentation someone would trust — corrected, pinned with a count assertion, and the spec now fills and round-trips the new fields.
Covers the two per-plan knobs and why null means off, the three jobs and their registry rows, the notification events with their differing recipient sets, and the env vars. Two things are stated plainly rather than left for a reader to discover: escalation is notify-only and no eligibility path reads either column, and grouped requests stamp but do not notify because the requestgroups module has no notification path at all. The website gains an 'Escalation & reminders' section with its cfg-review-escalation anchor registered in both frontend/src/config/docs.ts and website/app.js so the in-app 'View docs' buttons resolve.
…ructor QueryDetailResponseTest builds a QueryDetailView through the canonical constructor, so the two fields added for escalation shifted its argument list.
Four defects found reviewing this branch, each of which would have shipped silently. QueryRequestEntity had the new escalated_at field inserted between @Version and updatedAt, so Hibernate took escalated_at as the version column. That disables escalation outright (the scan's escalated_at IS NULL is never true once Hibernate stamps it), shows the banner on every request, and — worse — takes optimistic locking off query_requests entirely. The grouped-request scans joined request_group_items on request_group_id; the column is group_id (V106:63). Every tick would have thrown inside the per-row catch, escalating nothing and logging nothing above DEBUG. PUT /review-plans/{id} could set the two knobs but never clear them: absent and null are indistinguishable in a partial-update record, and null is the only way to turn escalation back off. UpdateReviewPlanCommand now carries explicit clear flags — the reconciler sets them when the bootstrap spec omits the field (declarative config), the controller when the caller sends null. ReviewPlanSpec had picked up a backward-compatible convenience constructor. On a @ConfigurationProperties record that breaks value-object binding completely: Spring resolves one constructor by arity, so every bootstrap review plan stopped binding, and the exception named pre-existing fields like name rather than anything new. Removed, with the properties test now asserting the escalation fields bind so a re-added overload fails loudly.
Three docs and the public website already said REVIEW_ESCALATED pages PagerDuty. It could not: PagerDutyTrigger had no mapping for it, so the trigger filter dropped the event before any HTTP call and the case arm in PagerDutyPayloadFactory was unreachable. The docs described the intent correctly; the code was what was missing. REVIEW_STALLED is deliberately its own name rather than reusing ESCALATION, which is already taken by QUERY_ESCALATED — a routing policy raising the approval bar at submission. The two mean opposite things: one is a policy deciding up front that a query needs more scrutiny, the other is nobody having decided at all. REVIEW_NUDGE gets no trigger. A reminder is not an incident and must never wake an on-call responder, so its payload arm is removed rather than left looking reachable. Full surface: enum, fromConfig error text, frontend union and checkbox, seven locales, the notifications doc's trigger list and sample config, the website copy, and an e2e assertion that the value round-trips through the backend config codec.
Four places claimed the grouped-request stamp "surfaces the bundle as escalated in the review queue". Nothing reads request_groups.escalated_at — there is no queue flag and no notification, because requestgroups has no notification path at all. The column is write-only for now, and it earns its place by recording which bundles went idle so the history is already there when either surface arrives. Said that way instead. The api-spec escalation prose had been inserted into the middle of the review-plan endpoint table, orphaning the DELETE row below it. Moved after the table, and documented that an explicit null clears the setting — the one place these two fields differ from every other field on the resource. QueryReviewEscalatedEvent carried an escalationAfterHours nobody read: the notification layer loads the request and its plan anyway to resolve recipients. Dropped, matching ApiReviewEscalatedEvent. The V144 comment claimed created_at means "since it entered review". It does not — it is submission time. That is the right clock, because approval_timeout_hours already uses it and escalate-after must be read against the same baseline as the timeout it precedes; the comment now says so rather than implying a precision the column does not have.
The six new scans are hand-written native SQL — enum casts, optional joins, interval arithmetic, a correlated MIN subquery. The unit tests mock the repository, and the jobs swallow per-row RuntimeExceptions, so a mistyped column produces a job that quietly never escalates anything. That is exactly what happened on this branch (request_group_id for a column named group_id), and nothing in the suite would have caught it. Seeding is raw JDBC rather than JPA on purpose: going through the entities would hide any column the entity does not map, which is the whole class of bug these tests exist to catch. Cleanup is scoped to a marker organization rather than truncating, because the Testcontainers instance is shared across the suite and a blanket delete would either destroy another test's rows or trip a foreign key from a table this helper knows nothing about. The grouped-request cases carry the most weight — they pin the MIN across member plans over both datasource and connector members, and assert that a bundle whose members all have escalation off is never due. Without the EXISTS guard the COALESCE(..., 0) makes it instantly due, and no Java-side test can see that. Also covers markEscalated / markNudged, which had no test at all: escalation fires once and survives a racing decision, nudges repeat and advance the cursor.
Escalations and nudges resolved recipients as the plan's LOWEST stage. That is right for QUERY_SUBMITTED, which fires at submission when lowest and current are the same stage — but these fire hours or days later, exactly when a multi-stage request may have moved on. On a two-stage plan whose stage 1 has approved, the nudge told the stage-1 reviewers who had already decided that the query was still awaiting their review, and never reached the stage-2 approver actually holding it up. Neither did the escalation. That is the precise failure the feature exists to prevent, and it would have looked like a mail-routing complaint rather than a logic bug. The fix is to share one definition rather than write a second: currentStage moves out of DefaultReviewService into core.api.ReviewStages, and both the module that decides who MAY act and the module that decides who gets TOLD now call it. A copy that drifted would authorize one set of reviewers while notifying another. Everything downstream was copy promising "the next stage" — twelve sites, including seven locale pairs, the website, the README, and V144's COMMENT ON COLUMN. All corrected to say what actually happens: the reviewers the request is waiting on, plus every org admin for an escalation. V144 is unapplied, so its operator-visible comment can still be fixed in place rather than needing a follow-up migration to correct. The locale files also pick up the two PagerDuty trigger labels added in the next commit — they are the same JSON objects and splitting them would make both commits unreadable.
REVIEW_ESCALATED and REVIEW_NUDGE are the first event types raised for both queries and API requests, which quietly broke three assumptions that held while every event belonged to exactly one kind. The notification bell routed on the event name alone, so a stalled API request sent the reviewer to the SQL review queue, where it does not appear. It now branches on api_request_id, which is the only thing that actually distinguishes the two. Email delivery keyed off the event type, so an API escalation rendered the query template: a bolded "SQL preview:" over an empty block and "Query type: —", because buildApiRequest has no SQL to supply. API-request events deliver in-app and over chat by the AF-500 convention, so the gate now takes the context rather than just the type. (Documented alongside it: because plan channels resolve by datasource, an API-request context resolves none, so those escalations reach the bell and the SMTP fallback but not a configured chat channel. Pre-existing, but this is the first feature that depends on it.) The PagerDuty dedup key was org + subject, so a CRITICAL-risk query that then stalled folded into the AI_HIGH_RISK incident its risk score had already opened — and paged nobody. A stalled critical query is exactly what REVIEW_STALLED is for. The event type is now part of the key; repeats of the same event on the same subject still dedupe, which is what dedup is for. Also adds the ANOMALY and BREAK_GLASS trigger options: the form offered four of the six values PagerDutyTrigger accepts, so a channel provisioned with either through bootstrap or the API could not be edited in the UI. Tests cover the four notification factories whose switches carry a default — Discord, Teams, Telegram and the ticket-description builder — where a missing arm compiles and ships a bare enum name into a real ServiceNow ticket.
Both stamping services documented a row lock neither took: markEscalated and markNudged used a plain findById while the javadoc promised "under a row lock, so a decision racing the scan produces no stray notification and a sibling replica cannot double-fire". Only the query path did it properly. That was worse than an inaccurate comment. With no pessimistic lock, once-only rested entirely on @Version, and the loser of a race can be the human: DefaultApiReviewService loads the request unlocked, so a reviewer approving inside the 5-minute escalation window hits an optimistic-lock failure. There is no ControllerAdvice branch for it, so that surfaces as a raw 500 on a reviewer's approve click. Both repositories now expose findByIdForUpdate and both stamps use it — the loser waits instead of failing. The grouped-request nudge path is removed rather than fixed. A nudge is a reminder sent to somebody, and requestgroups has no notification path at all, so a group nudge could only advance a cursor nobody reads — an UPDATE and a @Version bump on every pending bundle, every interval, forever, with no observable effect. request_groups therefore keeps escalated_at, which is once-only and records which bundles went idle, and loses last_nudged_at entirely. The two non-sargable nudge indexes go with it: the predicate is COALESCE(last_nudged_at, created_at) + interval, so leading an index with last_nudged_at bought nothing and cost a write on every stamp. ApiRequestStateService had no test at all — its six branches never executed, since the job tests mock it. It has one now, including an assertion that the stamp takes the lock rather than a plain read. Also adds the requestgroups poll-interval entry to application.yml, which was the only one of the three jobs missing from the file.
escalation_after_hours and approval_timeout_hours were validated independently (1–8760 each), so a 48-hour escalation window on a plan with the default 24-hour timeout saved cleanly and then did nothing: QueryTimeoutJob auto-rejects the request before the escalation scan ever sees it. Nothing surfaced that. No error, no warning, and no escalation — an admin would reasonably conclude the feature was broken. Every doc frames escalation as the warning shot BEFORE the timeout, so the setting is now validated to match that, on create and on update, against the post-merge values rather than the request body (a partial update can move either side). Tests cover the clear-flag branches too, which had none — they are what a prior review pass caught as "PUT could never disable escalation", and they would otherwise have shipped back unguarded.
Contributor
Contributor
Coverage Report for Frontend Coverage (frontend)
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||
Contributor
Contributor
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.
Closes #622.
Second and final PR for AF-622. PR 1 (#723) shipped out-of-office delegation; this one adds
idle escalation and nudge reminders, so a review that nobody picks up gets escalated and
re-announced before
QueryTimeoutJobauto-rejects it — instead of the submitter discovering thetimeout after the fact.
Escalation is notify-only. Nothing here changes who may approve: no eligibility path reads
escalated_at,escalation_after_hours, ornudge_interval_hours. An escalated request isdecidable by exactly the same people it was before.
What's in it
Per-plan configuration.
review_plansgainsescalation_after_hoursandnudge_interval_hours(V144), both nullable with null meaning off, so every existing plan isunaffected until an admin opts in. Both are editable from the review-plan modal and via the API.
On
PUT /review-plans/{id}an explicitnullclears the setting. These are the only tworeview-plan fields where null is meaningful rather than "leave unchanged", so
UpdateReviewPlanCommandcarries explicitclearEscalationAfterHours/clearNudgeIntervalHoursflags — a partial-update record cannot tell absent from null on its own, and without them
escalation would have been a one-way switch.
Three jobs, one per request type.
ReviewEscalationJob(queries),ApiReviewEscalationJob(API requests) and
GroupReviewEscalationJob(bundles), each on its ownACCESSFLOW_*_ESCALATION_POLL_INTERVAL(defaultPT5M), each@SchedulerLocked, each taking aninjected
Clockand swallowing per-rowRuntimeExceptions per.claude/patterns/scheduled-job.md.Escalation fires once per request:
escalated_atis stamped in the same transaction as theevent publish, and the stamp is re-checked under the row lock, so a restart, a retry, a second
replica, or a decision racing the scan cannot produce a duplicate. Nudges repeat on the cadence,
with
last_nudged_atas the cursor — measured from submission until the first reminder, from thelast reminder after that.
A grouped request has no plan of its own, so its window is the minimum non-null value across
its members' plans — the strictest member decides, matching the weakest-link union
GroupReviewPlanResolveralready uses. TheEXISTSguard beside theCOALESCE(MIN(...), 0)isload-bearing: without it a bundle whose members all have escalation switched off would read as
instantly due.
Notifications. Two new
NotificationEventTypevalues with the full fan-out from.claude/patterns/notification-fanout.md— all ten Java switch sites (three of which have adefaultand would have compiled silently wrong), two Thymeleaf templates, subject and body keysacross seven
messages*.properties, and the frontend union, bell case and seven locale JSONs.Recipients are the stage the request is blocked on, resolved through the new
core.api.ReviewStages.current— the same definitionDefaultReviewServiceuses to decide whomay act. Sharing it is the point: a second copy that drifted would notify one set of reviewers
while authorizing another.
REVIEW_ESCALATEDadds every active org admin on top, because the wholepoint is that the assigned reviewers did not act;
REVIEW_NUDGEgoes to those reviewers alone.PagerDuty pages on escalation through a new
REVIEW_STALLEDtrigger, and has no trigger at allfor
REVIEW_NUDGE: a reminder is not an incident.An escalation window must be shorter than the approval timeout. A longer one can never fire —
QueryTimeoutJobauto-rejects first — so it is now rejected rather than saved as a setting thatsilently does nothing.
UI. An escalation banner on the query detail page, and the two new numeric fields in the
review-plan modal.
Grouped requests are stamped but silent, and have no nudge at all
GroupReviewEscalationJobwritesrequest_groups.escalated_atand does nothing else — nonotification and no queue badge. That is not an oversight left half-done: the
requestgroupsmodule has no notification path whatsoever (no listener, no context builder), so grouped
requests have never emitted a submitted or approved message either. Publishing an event nothing
consumes would have been worse than not publishing one. The column records which bundles went idle
so the history is already there when a grouped-request notification path arrives.
For the same reason there is no group nudge half: a reminder needs somebody to remind, so a
group nudge could only advance a cursor nobody reads — rewriting every pending bundle on every
interval for no observable effect.
request_groupstherefore hasescalated_atand nolast_nudged_at. Called out indocs/03-data-model.md,docs/09-deployment.mdand on the website.Two shared event types, and what they broke
REVIEW_ESCALATED/REVIEW_NUDGEare the first events raised for both queries and APIrequests, which quietly invalidated three assumptions that held while every event belonged to one
kind:
queue where it does not appear — it now branches on
api_request_id;SQL-preview block — the gate now takes the context, per the AF-500 convention that API-request
events deliver in-app and over chat only;
the incident its risk score had already opened and paged nobody — the event type is now part of
the key.
Also fixed while here: both stamping services documented a row lock neither took. That mattered
beyond the comment — once-only rested on
@Version, and the loser of a race could be the humanreviewer, surfacing as a raw 500 on their approve click. Both now take
findByIdForUpdate.Testing
Unit tests for all three jobs and the state-service stamping, plus repository integration tests
against real PostgreSQL for the six new native queries. Those are not optional here: the queries
are hand-written SQL with enum casts, optional joins and interval arithmetic, the unit tests mock
the repository, and the jobs swallow per-row exceptions — so a mistyped column would have surfaced
only as a job that quietly never escalates anything. Review pass on this branch caught exactly that
(
i.request_group_idfor a column actually namedgroup_id); these tests are the gate that closesit.
Unit tests were added for everything two independent review passes found untested: the clear
flags,
ApiRequestStateService(which had none at all — the job tests mock it, so its six branchesnever executed), the group state service, current-stage recipient resolution, the bell routing, and
the four notification factories whose switches carry a
defaultand would ship a bare enum nameinto a real ServiceNow ticket.
mvn verify -Pcoverage— green, 6341 testsnpm run lint,npm run build,npm run test:coverage— green (92.3% statements, 84.4% branches)One caveat worth recording: an earlier run of the full suite failed once in
AttestationLifecycleIntegrationTest, which this branch does not touch. Its@AfterEachblanket-wipes orgs, users, permissions and
user_notificationsin the container the whole suiteshares, and the log shows an async notification thread from an earlier test hitting a FK violation
on a user that cleanup had just deleted. It passes alone and passes run directly alongside the new
escalation integration tests, and the re-run of the full suite was clean — a latent ordering race
that adding test classes exposed, not a defect here. Filed separately rather than folding an
unrelated test-isolation fix into this PR.
Note for the reviewer
Commit 1352505 carries an unrelated one-line fix to
backend/docker-compose-dev.yml(the PG18volume-mount path) that was in the working tree and got swept in. It is correct and independent of
this feature; say the word and I'll pull it out.