From b120bb44f573acc87035fea3c8424584f4499bab Mon Sep 17 00:00:00 2001 From: Jacek Tomaszewski Date: Wed, 26 Aug 2026 09:45:33 +0200 Subject: [PATCH] feat(inbox): a settled review comes back when someone asks for you in words MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reopen rule listened for GitHub's re-request button and nothing else, so a settled row stayed invisible while its author waited — having said so on the PR in plain words. "@you this is ready for another look" is the same ask, made the way people actually make it, and GitHub emits no event for it at all. The poll already reads the conversation for whose-move; it now also reads it for the last comment naming you, and reopens under the same after-settling test the button uses. Your own comments never count, bots never do, `@org/team` never does, and a longer login that starts with yours is not you. Two places, because a fix that only worked while a request was open would half-work: rows GitHub is still asking about read the button first and the words only if the button did not already answer, and rows it has stopped asking about — you reviewed on github.com, which clears the request — read the words alone, off the state-refresh leash that was already spending a call on them. An unsettled row still costs no GitHub call. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 18 +++-- SPEC.md | 126 ++++++++++++++++++++++--------- docs/lifecycle.md | 64 ++++++++++------ src/core/gh.test.ts | 99 +++++++++++++++++++++++- src/core/gh.ts | 40 +++++++++- src/server/daemon.test.ts | 98 ++++++++++++++++++++++++ src/server/daemon.ts | 153 ++++++++++++++++++++++++++++++-------- 7 files changed, 503 insertions(+), 95 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f9c7c54..67c70d7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -109,12 +109,18 @@ left unwritten. Code and tests win when they disagree. withdrawal reason is stricter still (poll-written drafts only, and it confirms against the PR with `fetchReviewRequests` before acting, because there its only evidence is a search index's silence). The poll also does the - reverse (`reopenIfAskedAgain`): a settled row comes back when someone - requested your review *again* after you settled it — the one thing that - reopens a `reviewed`/`skipped` row, since your skip answered the request that - was open at the time and cannot have answered a later one. It turns on - `settledAt` dating the decision and the last `REVIEW_REQUESTED_EVENT` naming - you; team requests never count, because this undoes a decision of yours. Config's `daemon` + reverse (`reopenIfAskedAgain`, `reopenIfAskedInWords`): a settled row comes + back when someone asked for you *again* after you settled it — the one thing + that reopens a `reviewed`/`skipped` row, since your skip answered whatever + was open at the time and cannot have answered a later ask. It turns on + `settledAt` dating the decision and the newer of two asks: the last + `REVIEW_REQUESTED_EVENT` naming you, and the last comment naming you + (`lastMentionOfYou`) — because "@you this is ready for another look" is the + same ask and GitHub has no event for it. The button is read first and spares + the second call; a row GitHub has stopped asking about (you reviewed on + github.com) gets the comment check alone, off the state-refresh leash. Team + requests and team mentions never count, because this undoes a decision of + yours, and neither do bots or your own comments. Config's `daemon` block is re-read every poll, so cockpit toggles apply without a restart. Each poll also publishes what it found (`status.awaiting`): the queue filters on what you settled locally, which stops matching what GitHub asks you for the diff --git a/SPEC.md b/SPEC.md index f46cc83..8ba6997 100644 --- a/SPEC.md +++ b/SPEC.md @@ -539,8 +539,8 @@ Derived sets used throughout: - `SETTLED = {sent, reviewed, skipped}` — out of the live queue. - `SETTLED_BY_YOU = {reviewed, skipped}` — a decision no push may undo; the - single thing that reopens one is a review request made *after* the settle - (§9.6). + single thing that reopens one is somebody asking for the user again *after* + the settle (§9.6). - `HEAD_SENSITIVE = {ready, sent}` — the two statuses that track the PR head. - **Archived is not a status**: it is `pr.state !== "OPEN"`, an orthogonal fact that moves a row to the archive tab whatever its status. @@ -576,8 +576,9 @@ themselves), and filing stamps it with the filed time. Reopening — the one transition out of `SETTLED_BY_YOU` without a click: ``` -reviewed | skipped ── the poll, a review request naming you - made after settledAt (§9.6) ──▶ ready | awaiting +reviewed | skipped ── the poll, someone asking for you again + after settledAt — the re-request button + or a comment naming you (§9.6) ──▶ ready | awaiting ``` `ready` if the row holds a finished, error-free draft; `awaiting` if not (the @@ -602,8 +603,8 @@ under `force`): marked it skipped, so a new push or review request does not reopen it" — the poll's most confusing silence, §5.4). A row dragged back into the inbox every time the author pushes would be cerber overruling the user; - the ways back in are the re-review button, which forces, and a newer - review request (§9.6). + the ways back in are the re-review button, which forces, and somebody + asking for the user again (§9.6). 2. Status in `HEAD_SENSITIVE` and the head the last run *read* (`run.reviewedSha`, falling back to `pr.headSha` for legacy artifacts, with an empty string never matching) equals the PR's current head → @@ -747,19 +748,46 @@ one back when GitHub has moved *to* it — both exist because the queue's picture of who is waiting on what is local, while the thing it pictures lives on github.com. -The gap it closes: a skip answers the review request that was open at the -moment it was made. An author who withdraws the request mid-look, pushes, and -re-requests leaves the row sitting in settled — invisible — while someone -actively waits. Every ordinary read GitHub offers says only *whether* a -request is open, never *when* it was made, so the poll reads the PR's -timeline for the last review-requested event naming the user. - -Rules, all normative: - -- Only rows the awaiting search returned are checked at all — GitHub is - asking right now, so the extra read answers exactly one question: was that - request made after the settle? An unsettled row MUST cost nothing (no - GitHub call). +The gap it closes: a skip answers whatever was open at the moment it was +made. An author who asks again afterwards leaves the row sitting in settled — +invisible — while someone actively waits. + +There are two ways to ask, and an implementation MUST honor both: + +1. **The re-request button.** Withdrawn-then-re-added looks identical to the + original in every ordinary read GitHub offers — they say only *whether* a + request is open, never *when* it was made — so the poll reads the PR's + timeline for the last review-requested event naming the user. +2. **A comment naming the user.** "@you this is ready for another look" is the + same ask made the way people actually make it, and GitHub emits no event + for it. The poll reads the PR conversation (the same issue-comment listing + §9.7 uses) for the newest comment that mentions the user. + +Rules for the mention, all normative: + +- Only `@login` counts, matched case-insensitively and bounded so a longer + login is not a match (`@me` MUST NOT match `@me-bot`, and an email address + MUST NOT match at all). `@org/team` MUST NOT count: a room being addressed + is not somebody asking for *this* user — the mirror of the team rule below. +- The user's own comments never count, and bots never count (`isBot`, §9.7): + a bot that @-mentions the reviewer on every push would otherwise be an ask + on every push. +- The newest qualifying comment by date wins, not by position in the response. + +Rules for both, all normative: + +- Where each check runs, and what it may cost: + - Rows the awaiting search returned get the timeline read. The conversation + is read only if the timeline did not already answer — a live re-request + says everything a comment could, so the second call is spared. + - Rows the search did *not* return get the conversation read alone, hung + off the state-refresh leash of §9.4 that was already spending a call on + them. This case is not optional: the user reviewing on github.com clears + the request and drops the PR out of the awaiting search, which is exactly + the state a "ready for another look" comment tends to arrive in. A reopen + rule that only worked while a request was open would be a fix that + half-works. + - An unsettled row MUST cost nothing (no GitHub call) on either path. - The settle date is `settledAt` when present. Rows settled before the field existed fall back to the latest moment the decision provably came after: `filed.at`, else `run.finishedAt` (a draft cannot be settled before it @@ -771,7 +799,7 @@ Rules, all normative: [REVIEW_REQUESTED_EVENT])` — the page maximum, because a PR that cycled through a dozen reviewers could push the request naming *you* out of a smaller window, silently restoring the bug). -- **Only requests naming the user count.** Team requests are deliberately +- **Only asks naming the user count.** Team requests are deliberately ignored here even though the withdrawal confirmation (§9.5) honors them: refusing to *file work away* is the safe side of that question, while this one *undoes a decision of the user's*, where the safe side is doing nothing @@ -791,9 +819,15 @@ Rules, all normative: there the ordinary rules take over: the freshness guard re-drafts if the head moved since the run read the code, and leaves a current draft alone. +- The log line and the history note MUST come from one place, so the line in + the terminal and the line on the review months later cannot tell different + stories about why a decision of the user's was undone. They differ only in + the fact that differs: the request's date, or the commenter's name and the + comment's date. + This is the **only** thing that reopens a `reviewed`/`skipped` row. A push still does not. No dedicated UI is required: the row simply reappears in the -inbox, and the daemon logs the reopen with the request's date. +inbox, and the daemon logs the reopen with the ask's date. ### 9.7 Whose Move Is It @@ -1393,7 +1427,7 @@ on every queue fetch so no screen can leave it stale. | GitHub read fails (poll) | poll errs, last good awaiting list stands, error published (§9.1) | | GitHub read fails (membership) | "not a member" — fail-closed for trust (§14.1) | | GitHub read fails (whose-move / filing evidence) | `unknown` / no filing — claim nothing (§9.7, §9.5) | -| GitHub read fails (reopen timeline) | row untouched; retried after the leash expires (§9.6) | +| GitHub read fails (reopen timeline or conversation) | row untouched; retried after the leash expires (§9.6) | | Checkout fails | diff-only review, logged, never fatal (§10.4) | | Agent output invalid | one retry with the error echoed; then the run fails (§11.2) | | Agent hangs | 30-min timeout, TERM→KILL, run fails (§11.5) | @@ -1427,8 +1461,8 @@ rate-limit bookkeeping and the in-flight registry are deliberately lost. pushes; sends and settles survive concurrent runs; user comments survive chat turns; `sent` is immutable. §8.5, §12.4–12.5. The one reopening rule (§9.6) respects the decision's scope rather than undoing it: a settle - answered the request open at the time, and only a *newer* request naming - the user brings the row back. + answered whatever was open at the time, and only a *newer* ask naming the + user brings the row back. 8. **Secure defaults, no insecure offering.** Non-loopback without auth is refused, not warned about. §16.1. @@ -1500,20 +1534,41 @@ file(a, filed): # atomic, predicate re-checked against disk settledAt: filed.at} : cur) ``` -### 21.4 Reopen Check (per settled row in the awaiting search, on its own leash) +### 21.4 Reopen Check (per settled row, on a leash) ``` -reopen_if_asked_again(a): +# GitHub is still asking: button first, words only if it did not answer. +reopen_if_asked_again(a): # rows in the awaiting search settled = a.settledAt or a.filed.at or a.run.finishedAt or return if a.sent or cap_spent or checked_recently(a.id): return stamp_checked(a.id) # before the call — outage ≠ retry storm me = current_login() or return - asked = try last_review_request_naming(a.pr, me) else return # no evidence - if asked is null or parse(asked) <= parse(settled): return # team asks never count - update(a.key, cur => asked_again(cur, asked) # re-check on disk + try: + at = last_review_request_naming(a.pr, me) # team asks never count + ask = {at, requested} if at else null + reopen(a, ask if newer_than_settle(a, ask) else asked_in_words(a, me)) + except: return # a failed read is evidence of nothing + +# GitHub stopped asking (own review on github.com): the words are all there is. +reopen_if_asked_in_words(a): # rows the state refresh already touched + if not settled_at_of(a): return + me = current_login() or return + try: reopen(a, asked_in_words(a, me)) + except: return + +asked_in_words(a, me): + c = newest(comment in conversation(a.pr) + where not bot(comment) and comment.author != me + and mentions(comment.body, me)) + return {c.at, mentioned, by: c.author} if c else null + +reopen(a, ask): + if not ask or parse(ask.at) <= parse(settled_at_of(a)): return + saved = update(a.key, cur => newer_than_settle(cur, ask) # re-check on disk ? {...cur, status: cur.has_clean_draft ? ready : awaiting, settledAt: null, filed: null} : cur) + if saved.status not in SETTLED_BY_YOU: log + note_history(wording(ask)) ``` ### 21.5 Chat Turn @@ -1564,11 +1619,14 @@ An implementation conforms when all of the following hold: order, guards, and withdrawal confirmation; bots never count as replies. - [ ] Every settling path stamps `settledAt` (the status route also clears - `filed`); reopening follows §9.6 — only user-named requests newer than - the settle (parsed timestamps), legacy fallback to `filed.at` then - `run.finishedAt`, unsettled rows cost no GitHub call, failed reads - change nothing, the leash stamp is spent before the call, and the - reopen clears `settledAt` and `filed`; a push alone never reopens. + `filed`); reopening follows §9.6 — only user-named asks newer than the + settle (parsed timestamps), by the re-request button *or* a comment + naming the user (never a team, never the user's own, never a bot's, and + never a longer login that starts with theirs); rows outside the awaiting + search are checked too; legacy fallback to `filed.at` then + `run.finishedAt`; unsettled rows cost no GitHub call; failed reads + change nothing; the leash stamp is spent before the call; and the + reopen clears `settledAt` and `filed`. A push alone never reopens. **Runner** - [ ] The tool matrix of §11.3 is exact; the run environment matches §14.3; diff --git a/docs/lifecycle.md b/docs/lifecycle.md index b87d0f3..beb22ac 100644 --- a/docs/lifecycle.md +++ b/docs/lifecycle.md @@ -274,9 +274,10 @@ walk `walkable()`: open, unsettled rows only. | GitHub moved past a finished draft | status → `reviewed`, `filed` set | `fileIfSettledElsewhere` | | Auto-send is on and the draft qualified | status → `sent` (the **sent** tab) | `handleAutoSend` | -And the one that *adds* a row back: your review requested again after you -settled → status → `ready`/`awaiting`, `settledAt` and `filed` cleared -(`reopenIfAskedAgain`; see "Asked again" below). +And the one that *adds* a row back: somebody asking for you again after you +settled — the re-request button, or your name in a comment → status → +`ready`/`awaiting`, `settledAt` and `filed` cleared (`reopenIfAskedAgain`, +`reopenIfAskedInWords`; see "Asked again" below). A **pure stub** is `awaiting`, with no comments and **`run === null`** — it does not look at chapters. The `run` block is what makes this safe: the create @@ -314,27 +315,46 @@ minute, and that the PR itself disagreed with it. ### Asked again: the way back out of settled Filing's mirror image, and the only thing that reopens a settled row on its own -(`reopenIfAskedAgain`, `src/server/daemon.ts`). A skip says "I am done with -this PR", and a push does not undo that. Somebody asking you *again* is a -different event: your skip answered the request that was open when you made it -and cannot have answered one that came later — which is what a request -withdrawn and then re-added is. Nothing else in GitHub's API distinguishes the -two, so this reads the one thing that does: the timestamp of the last -`REVIEW_REQUESTED_EVENT` naming you (`fetchLastReviewRequest`, one GraphQL call). +(`reopenIfAskedAgain` / `reopenIfAskedInWords`, `src/server/daemon.ts`). A skip +says "I am done with this PR", and a push does not undo that. Somebody asking +you *again* is a different event: your skip answered whatever was open when you +made it and cannot have answered something that came later. + +People ask in two ways, and both count: + +1. **The re-request button.** A request withdrawn and then re-added looks + identical to the original in every ordinary read GitHub offers, so this + reads the one thing that tells them apart: the timestamp of the last + `REVIEW_REQUESTED_EVENT` naming you (`fetchLastReviewRequest`, one GraphQL + call). +2. **Your name in a comment** — "@you this is ready for another look". The same + ask, made the way people actually make it, and one GitHub has no event for + at all. The conversation is already read for whose-move (below); this reads + it for the last comment naming you (`lastMentionOfYou`). Your own comments + never count and bots never do, or a bot that @-mentions the reviewer on + every push would be an ask on every push. `@org/team` doesn't count either: + a room being addressed is not somebody asking for *you*. Conditions, all of them: the row is `reviewed` or `skipped`, it was never sent, -GitHub is asking about it *now* (only rows in the awaiting search are checked -at all), and the request is newer than `settledAt`. Then the row goes back to -`ready` — or `awaiting` if it has no finished draft to show — `settledAt` and -`filed` are cleared, and the ordinary rules take it from there: the freshness -guard in §4 re-drafts it if the head has moved since the run read it, and -leaves the existing draft alone if it has not. Same leash as the state checks -above: one call per artifact per 30 minutes, capped per poll. - -Team requests are ignored here on purpose. `stillRequested` counts them because -refusing to *file work away* is the safe side of that question; this decides to -undo a decision of yours, where the safe side is doing nothing unless somebody -named you. +and the ask is newer than `settledAt`. Then the row goes back to `ready` — or +`awaiting` if it has no finished draft to show — `settledAt` and `filed` are +cleared, and the ordinary rules take it from there: the freshness guard in §4 +re-drafts it if the head has moved since the run read it, and leaves the +existing draft alone if it has not. + +Where each runs, and what it costs: rows GitHub is asking about *now* (the +awaiting search) get the button check, and the conversation only if the button +did not already answer — a live re-request says everything a comment could. +Rows GitHub has stopped asking about — you reviewed on github.com, which clears +the request and drops the PR out of that search — get the comment check alone, +hung off the same leashed state refresh that was already spending a call on +them. Either way: one artifact per 30 minutes, capped per poll, and an +unsettled row costs nothing. + +Team *requests* are ignored here on purpose. `stillRequested` counts them +because refusing to *file work away* is the safe side of that question; this +decides to undo a decision of yours, where the safe side is doing nothing +unless somebody named you. Rows settled before `settledAt` existed are dated by the latest moment cerber can prove the decision came after — cerber's own `filed.at`, or `run.finishedAt` diff --git a/src/core/gh.test.ts b/src/core/gh.test.ts index b344dbe..53478ab 100644 --- a/src/core/gh.test.ts +++ b/src/core/gh.test.ts @@ -1,6 +1,14 @@ import { execFile } from "node:child_process"; import { Mock, beforeEach, describe, expect, it, vi } from "vitest"; -import { classifyReply, currentLogin, lastRequestOf, latestOwnReview, resetLoginCache } from "./gh.js"; +import { + classifyReply, + currentLogin, + lastMentionOfYou, + lastRequestOf, + latestOwnReview, + mentionsYou, + resetLoginCache, +} from "./gh.js"; // gh.ts calls `promisify(execFile)`, which honours this symbol — so the mock // resolves to the `{ stdout }` shape the real one does, while still recording @@ -41,8 +49,14 @@ describe("currentLogin", () => { }); describe("classifyReply", () => { - const c = (author: string, at: string) => ({ author, at, bot: false, url: `#${author}-${at}` }); - const bot = (author: string, at: string) => ({ author, at, bot: true, url: null }); + const c = (author: string, at: string, body = "") => ({ + author, + at, + body, + bot: false, + url: `#${author}-${at}`, + }); + const bot = (author: string, at: string, body = "") => ({ author, at, body, bot: true, url: null }); it("says nobody has heard from you when you never spoke", () => { expect(classifyReply([], "me")).toBe("none"); @@ -107,6 +121,85 @@ describe("classifyReply", () => { expect(classifyReply([bot("me", "2026-08-19T10:00:00Z")], "me")).toBe("none"); }); }); + +describe("mentionsYou", () => { + it("finds your name however the sentence puts it", () => { + expect(mentionsYou("@me this is ready for another look", "me")).toBe(true); + expect(mentionsYou("ready now, @me", "me")).toBe(true); + expect(mentionsYou("cc (@me) when you get a sec", "me")).toBe(true); + expect(mentionsYou("done — @me?", "me")).toBe(true); + }); + + it("is not fooled by a longer name that starts with yours", () => { + // The bot that shares your prefix asks for you on every push otherwise. + expect(mentionsYou("@me-bot rebuilt the preview", "me")).toBe(false); + expect(mentionsYou("@median said the same thing", "me")).toBe(false); + }); + + it("does not read an email address as an ask", () => { + expect(mentionsYou("mail jacek@me.dev if it breaks", "me")).toBe(false); + }); + + it("ignores the case GitHub itself ignores", () => { + expect(mentionsYou("@ME ready", "me")).toBe(true); + }); + + it("says nothing about a comment that names nobody", () => { + expect(mentionsYou("rebased onto main", "me")).toBe(false); + expect(mentionsYou("", "me")).toBe(false); + }); + + it("does not take a team's name for yours", () => { + // A room being addressed is not somebody asking for you, and this undoes + // a decision of yours — see `trust.ts` for where teams do count. + expect(mentionsYou("@acme/reviewers could someone look", "me")).toBe(false); + }); +}); + +describe("lastMentionOfYou", () => { + const said = (author: string, at: string, body: string, bot = false) => ({ + author, + at, + body, + bot, + url: `#${author}-${at}`, + }); + + it("returns the newest comment that named you", () => { + const found = lastMentionOfYou( + [ + said("them", "2026-08-19T10:00:00Z", "@me first look?"), + said("them", "2026-08-21T10:00:00Z", "@me ready again"), + said("them", "2026-08-22T10:00:00Z", "rebased"), + ], + "me", + ); + expect(found?.at).toBe("2026-08-21T10:00:00Z"); + }); + + it("reads dates rather than trusting the order they arrived in", () => { + const found = lastMentionOfYou( + [said("them", "2026-08-21T10:00:00Z", "@me later"), said("them", "2026-08-19T10:00:00Z", "@me earlier")], + "me", + ); + expect(found?.at).toBe("2026-08-21T10:00:00Z"); + }); + + it("never counts you asking for yourself", () => { + expect(lastMentionOfYou([said("me", "2026-08-19T10:00:00Z", "cc @me")], "me")).toBeNull(); + }); + + it("never counts a bot that @-mentions the reviewer", () => { + expect( + lastMentionOfYou([said("ci[bot]", "2026-08-19T10:00:00Z", "@me the build is green")], "me"), + ).toBeNull(); + }); + + it("says nobody asked when nobody used your name", () => { + expect(lastMentionOfYou([said("them", "2026-08-19T10:00:00Z", "ready for review")], "me")).toBeNull(); + expect(lastMentionOfYou([], "me")).toBeNull(); + }); +}); import { ghErrorDetail, parsePrRef, searchAwaitingArgs } from "./gh.js"; describe("latestOwnReview", () => { diff --git a/src/core/gh.ts b/src/core/gh.ts index 7a087d5..3f50a36 100644 --- a/src/core/gh.ts +++ b/src/core/gh.ts @@ -170,6 +170,8 @@ export type Reply = "none" | "you" | "them" | "unknown"; export interface Comment { author: string; at: string; + /** What was said. Read for one thing only: whether it names you (`mentionsYou`). */ + body: string; /** CI, changelog and integration bots talk on PRs; none of it is an answer. */ bot: boolean; url: string | null; @@ -187,7 +189,7 @@ export async function fetchConversation(ref: PrRef): Promise { "--paginate", `repos/${ref.owner}/${ref.repo}/issues/${ref.number}/comments`, "--jq", - '.[] | {author: .user.login, at: .created_at, bot: (.user.type == "Bot"), url: .html_url}', + '.[] | {author: .user.login, at: .created_at, body: (.body // ""), bot: (.user.type == "Bot"), url: .html_url}', ]); // --jq streams one object per line rather than a JSON array. return out @@ -229,6 +231,42 @@ export function lastWordOfYours(comments: Comment[], login: string): Comment | n return mine[mine.length - 1] ?? null; } +/** + * Whether a comment speaks to you by name. + * + * GitHub's own rule, near enough: an `@login` that isn't part of a longer word. + * The boundary is "not a login character" rather than `\b`, because logins may + * contain hyphens — `@me-bot` is a different account from `@me`, and + * `you@me.dev` is an email address, not an ask. + * + * Deliberately not team mentions. `@org/team` is a room being addressed, and + * this is used to undo a decision of yours, which wants somebody naming *you*. + */ +export function mentionsYou(body: string, login: string): boolean { + const name = login.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + return new RegExp(`(^|[^\\w-])@${name}(?![\\w-])`, "i").test(body); +} + +/** + * The last comment somebody addressed to you by name, or null if nobody has. + * + * The other half of what the conversation can say. `classifyReply` reads it for + * whose move it is; this reads the same comments for the ask GitHub's own API + * has no event for — "@you this is ready for another look", typed instead of + * clicking the re-request button. Your own comments never count, and bots never + * do: a bot that @-mentions the assignee on every push would otherwise be an + * ask on every push. + */ +export function lastMentionOfYou(comments: Comment[], login: string): Comment | null { + const theirs = comments.filter( + (c) => !isBot(c) && c.author !== login && mentionsYou(c.body, login), + ); + if (theirs.length === 0) return null; + // By date, not by position: this undoes a settle, so it does not lean on the + // API happening to return the conversation in order. + return theirs.reduce((a, b) => (b.at > a.at ? b : a)); +} + /** * A review you submitted on the PR yourself, as GitHub has it. * diff --git a/src/server/daemon.test.ts b/src/server/daemon.test.ts index d915347..9f149e8 100644 --- a/src/server/daemon.test.ts +++ b/src/server/daemon.test.ts @@ -983,6 +983,104 @@ describe("a review you settled, and were asked for again", () => { expect((await pollOnce())?.status).toBe("skipped"); }); + /** A PR comment, the way the conversation endpoint hands them over. */ + const said = (author: string, at: string, body: string, bot = false) => ({ + author, + at, + body, + bot, + url: `https://github.com/acme/widgets/pull/7#issuecomment-${at}`, + }); + + // The ask people actually make. GitHub has no event for it, so reading only + // the re-request button left these rows invisible while their authors waited. + it("puts the row back when the ask was typed instead of clicked", async () => { + await saveArtifact(settled()); + conversation.mockResolvedValue([said("them", "2026-08-24T12:41:22Z", "@me this is ready for re-review")]); + + const after = await pollOnce(); + expect(after?.status).toBe("ready"); + expect(after?.settledAt).toBeNull(); + }); + + it("names who asked, since here somebody did", async () => { + await saveArtifact(settled()); + conversation.mockResolvedValue([said("them", "2026-08-24T12:41:22Z", "@me ready for another look")]); + + expect((await pollOnce())?.history?.map((e) => e.what)).toContain( + "back in the inbox: them asked for you by name on 2026-08-24, after you settled it", + ); + }); + + it("leaves your skip standing when the mention is older than it", async () => { + await saveArtifact(settled()); + conversation.mockResolvedValue([said("them", "2026-08-23T09:00:00Z", "@me could you take this one")]); + + expect((await pollOnce())?.status).toBe("skipped"); + }); + + it("does not take your own comment, or a bot's, for somebody asking", async () => { + await saveArtifact(settled()); + conversation.mockResolvedValue([ + said("me", "2026-08-24T12:00:00Z", "cc @me so I remember"), + said("ci[bot]", "2026-08-24T13:00:00Z", "@me preview is up", true), + ]); + + expect((await pollOnce())?.status).toBe("skipped"); + }); + + it("does not read the conversation when the button already answered", async () => { + // Two calls per settled row per poll is the ceiling, not the price: a live + // re-request says everything a comment could, so the second one is spared. + // The later mention here would win if it were read — it isn't. + await saveArtifact(settled()); + lastRequest.mockResolvedValue("2026-08-24T12:41:22Z"); + conversation.mockResolvedValue([said("them", "2026-08-25T08:00:00Z", "@me ready")]); + + const said_ = (await pollOnce())?.history?.map((e) => e.what) ?? []; + expect(said_).toContain( + "back in the inbox: your review was requested again on 2026-08-24, after you settled it", + ); + expect(said_.some((w) => w.includes("asked for you by name"))).toBe(false); + }); + + // The other half of the same fix: you reviewed on github.com, which clears + // the request and drops the PR out of the awaiting search entirely — and the + // author then asks for you in words on a row nothing else would look at. + it("reopens a row GitHub is no longer asking about", async () => { + await saveArtifact( + settled({ + status: "reviewed" as const, + filed: { + at: "2026-08-24T10:00:00Z", + reason: "own-review" as const, + review: { at: "2026-08-24T10:00:00Z", state: "CHANGES_REQUESTED" as const, url: null }, + reply: null, + }, + }), + ); + search.mockResolvedValue([]); + prInfo.mockResolvedValue({ ...stubArtifact(DISCOVERED).pr, state: "OPEN", isDraft: false }); + conversation.mockResolvedValue([said("them", "2026-08-24T12:41:22Z", "@me fixed, ready for another look")]); + + const after = await pollOnce(); + expect(after?.status).toBe("ready"); + expect(after?.filed).toBeNull(); + }); + + it("leaves a finished draft to the filing rules, not the reopen ones", async () => { + // The two checks share the same leashed slot, and each guards itself: an + // unsettled row is the filing check's business, and a mention on it must + // not divert the row into a reopen it was never eligible for. + await saveArtifact({ ...stubArtifact(DISCOVERED), status: "ready" as const, run: RUN }); + search.mockResolvedValue([]); + prInfo.mockResolvedValue({ ...stubArtifact(DISCOVERED).pr, state: "OPEN", isDraft: false }); + ownReview.mockResolvedValue(null); + requests.mockResolvedValue({ users: [], teams: [] }); + conversation.mockResolvedValue([said("them", "2026-08-24T12:41:22Z", "@me ready")]); + + expect((await pollOnce())?.filed?.reason).toBe("request-withdrawn"); + }); }); describe("askedAgainAfterSettling", () => { diff --git a/src/server/daemon.ts b/src/server/daemon.ts index bad2e6f..5d8128f 100644 --- a/src/server/daemon.ts +++ b/src/server/daemon.ts @@ -11,6 +11,7 @@ import { fetchOwnReview, fetchPrInfo, fetchReviewRequests, + lastMentionOfYou, lastWordOfYours, searchAwaitingMe, stillRequested, @@ -255,24 +256,55 @@ export function settledAtOf(a: Artifact): string | null { } /** - * Whether a review request is a *new* ask rather than the one you settled. + * Whether an ask is a *new* one rather than the one you settled. * * A skip says "I am done with this PR", and a push does not undo that — the * whole point of `SETTLED_BY_YOU`. Somebody asking you again is a different - * event: your skip answered the request that was open when you made it, and it - * cannot have answered one that came after. Withdrawn-then-re-added is the - * shape this catches, and it is not exotic — an author who pulls the request - * while you are mid-look and puts it back with the next push leaves the inbox - * silent about a review someone is now actively waiting on. + * event: your skip answered whatever was open when you made it, and it cannot + * have answered something that came after. Withdrawn-then-re-added is one shape + * this catches, and it is not exotic — an author who pulls the request while + * you are mid-look and puts it back with the next push leaves the inbox silent + * about a review someone is now actively waiting on. The other shape is the + * same author typing it instead (`Ask`). * * Parsed, not compared as text, for the reason `filedByYourAct` gives: our * timestamps carry milliseconds and GitHub's do not, and "…:00.500Z" sorts * before "…:00Z". */ -export function askedAgainAfterSettling(a: Artifact, requestedAt: string | null): boolean { - if (requestedAt === null) return false; +export function askedAgainAfterSettling(a: Artifact, askedAt: string | null): boolean { + if (askedAt === null) return false; const settled = settledAtOf(a); - return settled !== null && Date.parse(requestedAt) > Date.parse(settled); + return settled !== null && Date.parse(askedAt) > Date.parse(settled); +} + +/** + * Somebody asking for you again, and which of the two ways they did it. + * + * `requested` is GitHub's re-request button, which leaves a dated event. + * `mentioned` is a person typing "@you this is ready for another look" — the + * same ask, made the way people actually make it, and one GitHub's API gives + * no event for at all. Both reduce to a date, which is all the settle test + * needs; `by` only exists because the second one can say who. + */ +export type Ask = + | { at: string; how: "requested" } + /** `by` is the commenter. A request has no such field: the event names nobody useful. */ + | { at: string; how: "mentioned"; by: string }; + +/** + * What the daemon says about a reopen, in the log and in the row's history. + * + * One place, so the line the user reads in the terminal and the line they read + * on the review months later cannot tell different stories about why a decision + * of theirs was undone — which is the only thing that makes undoing it fair. + */ +export function askWording(ask: Ask): { log: string; history: string } { + const day = ask.at.slice(0, 10); + const what = + ask.how === "requested" + ? `your review was requested again on ${day}` + : `${ask.by} asked for you by name on ${day}`; + return { log: `${what} — back in the inbox`, history: `back in the inbox: ${what}, after you settled it` }; } /** @@ -360,6 +392,10 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { // needs it. Undefined is "not asked yet"; null is "gh could not say", which // attributes nothing to you rather than guessing. let login: string | null | undefined; + async function who(): Promise { + if (login === undefined) login = await currentLogin().catch(() => null); + return login; + } /** * Put a settled row back in the inbox when somebody has asked you again. @@ -369,9 +405,17 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { * moved *to* it. Both exist because the queue's picture of who is waiting * on what is local, and the thing it is a picture of lives on github.com. * - * Only rows GitHub is asking about right now get here at all — the caller - * is looping over the awaiting search — so the extra read is one call for - * one question: was that request made after you settled? + * Two ways to be asked, because people use both. GitHub's re-request button + * leaves a dated event and is read first: it is the strongest evidence and + * the cheapest call. Failing that, the conversation is read for the last + * comment naming you — "@you this is ready for another look", which is the + * same ask with no event behind it. Reading only the button was the bug: + * a settled row sat invisible while its author waited, having said so in + * plain words on the PR. + * + * Only rows GitHub is still asking about get here — the caller is looping + * over the awaiting search. A row whose request is gone (you reviewed on + * github.com) is caught by `reopenIfAskedInWords` on the other loop instead. */ async function reopenIfAskedAgain(artifact: Artifact): Promise { if (settledAtOf(artifact) === null) return; @@ -379,24 +423,71 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { if (reopenChecks >= STATE_REFRESH_CAP || Date.now() - last < STATE_REFRESH_MIN_MS) return; reopenChecks++; requestCheckedAt.set(artifact.id, Date.now()); - if (login === undefined) login = await currentLogin().catch(() => null); - if (login === null) return; + const me = await who(); + if (me === null) return; - let requestedAt: string | null; + let ask: Ask | null; try { - requestedAt = await fetchLastReviewRequest(artifact.pr, login); + const requestedAt = await fetchLastReviewRequest(artifact.pr, me); + const request: Ask | null = requestedAt === null ? null : { at: requestedAt, how: "requested" }; + // The words are only worth a second call when the button did not + // already answer: a live re-request says everything a comment could. + ask = askedAgainAfterSettling(artifact, request?.at ?? null) + ? request + : await askedInWords(artifact, me); } catch { // A read that failed is not evidence of anything, so nothing happens to // the row. It is not re-asked about until the leash above expires: the // stamp is spent before the call on purpose, so gh being down cannot - // turn every settled row into a retry on every poll. + // turn every settled row into a retry on every poll. Only the reads are + // guarded: a write that fails is a real fault and belongs to the caller. + return; + } + await reopen(artifact, ask); + } + + /** + * The same reopen for a settled row GitHub is *not* asking about. + * + * The case is ordinary: you reviewed on github.com, which clears the + * request and drops the PR out of the awaiting search, and the author then + * pushes and writes "@you ready for another look". No request exists to + * re-read, so the words are the whole of the evidence — and without this + * the mention check would only work for the rows that happen to still carry + * an open request, which is a fix that half-works. + * + * Rides the state-refresh leash rather than one of its own: the caller has + * already spent that artifact's slot for this half-hour, and a settled row + * never reaches `fileIfSettledElsewhere`, so this costs the poll one call + * on rows that were costing it one already. + */ + async function reopenIfAskedInWords(artifact: Artifact): Promise { + if (settledAtOf(artifact) === null) return; + const me = await who(); + if (me === null) return; + let ask: Ask | null; + try { + ask = await askedInWords(artifact, me); + } catch { + // Same as above: a failed read changes nothing and waits for the leash. return; } - if (requestedAt === null || !askedAgainAfterSettling(artifact, requestedAt)) return; - const at = requestedAt; + await reopen(artifact, ask); + } + + /** The conversation's answer to "has anybody asked for me again", or null. */ + async function askedInWords(artifact: Artifact, me: string): Promise { + const mention = lastMentionOfYou(await fetchConversation(artifact.pr), me); + return mention === null ? null : { at: mention.at, how: "mentioned", by: mention.author }; + } + + /** Undo the settle, if the ask really is newer than it. */ + async function reopen(artifact: Artifact, ask: Ask | null): Promise { + if (ask === null || !askedAgainAfterSettling(artifact, ask.at)) return; + const at = ask.at; // Re-read from disk: a re-review or a send may have started in the time - // that call took, and neither wants its status rewritten underneath it. + // those calls took, and neither wants its status rewritten underneath it. const saved = await updateArtifactByKey(artifactKey(artifact.id), (a) => askedAgainAfterSettling(a, at) ? // `filed` goes with the status it explained: cerber's account of why @@ -407,13 +498,11 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { // The status is what says the write landed. `settledAt` cannot: a legacy // row never had one, so a mutation that declined would read as a reopen. if (saved && !SETTLED_BY_YOU.has(saved.status)) { - log(`[${artifact.id}] your review was requested again on ${at.slice(0, 10)} — back in the inbox`); + const said = askWording(ask); + log(`[${artifact.id}] ${said.log}`); // The status change alone says a row moved; this says why it moved, // which is the whole reason a settle is allowed to be undone at all. - await noteHistory( - artifact.id, - `back in the inbox: your review was requested again on ${at.slice(0, 10)}, after you settled it`, - ); + await noteHistory(artifact.id, said.history); } } @@ -463,9 +552,8 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { // Only a finished, unsent draft can be filed — checked here too, so a row // that could never qualify costs no GitHub call at all. if (artifact.status !== "ready" || artifact.sent) return; - if (login === undefined) login = await currentLogin().catch(() => null); - if (login === null) return; - const me = login; + const me = await who(); + if (me === null) return; /** Re-reads from disk: a run or a send may have started in the seconds * these reads took, and neither wants filing out from under it. */ @@ -577,7 +665,14 @@ export function startDaemon(opts: DaemonOptions): DaemonHandle { })); } if (pr.state !== "OPEN") log(`[${artifact.id}] ${pr.state.toLowerCase()} — archived`); - else await fileIfSettledElsewhere(artifact); + else { + // Exactly one of these two spends a call: the first only looks at + // settled rows, the second only at finished drafts. They are the same + // question from opposite sides — has GitHub moved past this, or back + // to it — and a row can only be on one side of it. + await reopenIfAskedInWords(artifact); + await fileIfSettledElsewhere(artifact); + } } catch { // Same: book-keeping can wait for the next poll. }