Skip to content

Add the scheduled ReviewSLA job: comments, reminders, digest, dashboard - #44

Merged
willson556 merged 5 commits into
mainfrom
review-sla-remind
Oct 2, 2026
Merged

willson556 merged 5 commits into
mainfrom
review-sla-remind

Conversation

@willson556

@willson556 willson556 commented Oct 1, 2026 •

Copy link
Copy Markdown
Member

Step 2 of Proposal 2's rollout (rollout plan): the ReviewSLA action. Merging it changes nothing by itself.

The scheduled workflow that runs it lives in a private repo, not this one, because this repo's run logs are public and the job's would show who is out of office and activity in the private repos. The README has the workflow. It's skipped until that repo's REVIEW_SLA_MODE variable is set, and setting it back to off is the rollback.

Every 30 minutes during business hours (10:00 to 17:00 Pacific, business days), it reads the open PRs in the five pilot repos and this one. From their timelines it rebuilds each assignee's clock (ReviewSLA/owed.js):

  • First response: 4 business hours under 250 added lines, 7 otherwise. It starts at the latest of: the PR marked ready, the person assigned, and, mid-stack, their approval of the PR below or that PR merging.
  • Stacks: an assignee owes only on the lowest ready PR they're assigned to and haven't approved. Requesting changes doesn't move them up.
  • Re-review: 4 business hours from a re-request, but only if the author has pushed since that reviewer's last review. Graphite's re-requests on gt submit start nothing.
  • Drafts stop every clock on the PR, and marking it ready again starts them over.

What each mode does:

Mode Effect
shadow A Review SLA comment on each PR where someone owes a review (who owes what, due when), the Confluence dashboard and the 10:00 Slack digest
remind Also a Slack DM when a review is due, and reassignment of anyone the PTO calendar has out before their review is due
enforce Reassignment at twice the target is step 3; for now it warns and runs as remind

Details:

  • Reminders are sent once. The comment's hidden marker records each reminder. The comment is written before the DM goes out, so a failure means a missed reminder, not a repeated one. Runs don't overlap.
  • Out-of-office reassignment replaces only the person who's out, on every PR in the stack they're assigned to. A domain approver is replaced from the domain team, a rotation reviewer from the rotation team. A comment on the lowest of those PRs says who took over. The least-loaded pick moves from AssignReviewers to ReviewConfig/pick.js, so both pick the same way.
  • The digest comes from the 10:00 Pacific cron, one for daylight time and one for standard time, so a late scheduled run still posts it once.
  • The dashboard stores a hash of its content in the page version's message. It saves only when that changes, as a minor edit, because Confluence rewrites the page body on save.
  • API errors are warnings, and that PR, repo, channel or page is skipped for the run.
  • Re-review compares the PR's head commit with the commit each review was left on, not commit dates. Commit dates need the app's Contents permission in private repos, and a rebase rewrites them.

Setup: the org secrets and variables in the README are all set, including CONFLUENCE_URL and CONFLUENCE_USER. Still to do: the private repo with the workflow and its REVIEW_SLA_MODE variable.

Testing:

  • node --test AssignReviewers/ ReviewConfig/ ReviewSLA/: 81 cases, 26 of them new.
    • The clock rules: the 16:30 Friday small PR, the three-PR stack, approving the bottom, requesting changes, a draft below, the PR below merging, re-review with and without a push, draft and ready again, late assignment, and bots and merge queue.
    • Whole runs against a fake org: off, outside business hours, shadow vs remind, one DM per clock across runs, reassignment across a two-PR stack, someone out only after their due time, the digest cron in daylight and standard time, the dashboard saving only on change, an unreadable repo, a failed comment write sending no DM, a missing Slack ID, and enforce.
  • Dry run of the GitHub query and clocks, read-only, against all six repos today: 197 open PRs read. 2 clocks are running, both on this repo's own stack. StaflLib and coit-tower have 6 ready PRs with assignees, all already reviewed.
  • A shadow run in Actions from a throwaway branch (since deleted) read the open PRs, wrote the Review SLA comments on AssignReviewers: skip people who are out, from the Rippling PTO calendar #41 and Add ReviewSLA step 1: first-response clocks and report mode #43, and saved the Confluence dashboard. Before the commit-ID change, the private repos failed with "Resource not accessible by integration". Slack hasn't been called for real yet.

🤖 Generated with Claude Code

@staflsystemsci

staflsystemsci Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Review SLA

Nobody owes a review on this PR right now.

Due times are in business hours, 10:00 to 17:00 Pacific: 4 h for a PR under 250 added lines, 7 h otherwise, and 4 h for a re-review. Mid-stack, an assignee owes a response on the lowest PR they haven't approved. Dashboard

@staflsystemsci staflsystemsci Bot assigned thebhef and unassigned nehalkpatel Oct 2, 2026
Comment thread ReviewSLA/owed.js
@willson556
willson556 requested a review from nehalkpatel October 2, 2026 16:45

willson556 commented Oct 2, 2026 •

Copy link
Copy Markdown
Member Author

Merge activity

  • Oct 2, 10:56 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Oct 2, 11:00 PM UTC: Graphite rebased this pull request as part of a merge.
  • Oct 2, 11:00 PM UTC: @willson556 merged this pull request with Graphite.

@willson556
willson556 changed the base branch from review-sla to graphite-base/44 October 2, 2026 22:57
@willson556
willson556 changed the base branch from graphite-base/44 to main October 2, 2026 22:58
willson556 and others added 4 commits October 2, 2026 22:59
Step 2 of Proposal 2's rollout. Every 30 minutes during business hours the
job rebuilds each assignee's clock from the open PRs' timelines (first
response with the stack rule, and re-review after a push and re-request),
then, by REVIEW_SLA_MODE:

- shadow: a Review SLA comment on each PR, the Confluence dashboard (saved
  only when it changes, as a minor edit) and the 10:00 Slack digest
- remind: also a Slack DM when a review is due, recorded in the comment's
  hidden marker so it's sent once, and reassignment across the stack of an
  assignee the PTO calendar has out before their review is due

The least-loaded pick moves from AssignReviewers to ReviewConfig/pick.js so
reassignment picks the way assignment does, and pto.js gains readOutDates
for each person's out dates.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Commit dates need the app's Contents permission in private repos, and a
rebase rewrites them. The PR's head commit against the commit each review
was left on needs only Pull requests read, and can't be fooled by a rebase.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This repo's run logs are public, and the job's would show who is out of
office and activity in the private repos it reads. The scheduled workflow
moves to a private repo; the README has it.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ermission

GraphQL's PullRequestReview.commit is refused in private repos without the
app's Contents permission. REST's commit_id isn't, so it's read for the PRs
where a re-review could be running: a re-request after that person's review.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…c PRs

An assignee who is out was replaced on every PR in the stack, including ones
they'd already approved, so their replacement owed reviews that were done.
Now only the PRs they haven't approved change hands.

The reassignment notice said the person was out of office, which on a public
repo's PR anyone can read. There it now says only that they aren't available.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@willson556
willson556 merged commit a97c52b into main Oct 2, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants