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. }