diff --git a/src/db/repositories.ts b/src/db/repositories.ts index edfc08f76..bff5bd177 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -3564,6 +3564,28 @@ export async function hasAuditEventForDelivery(env: Env, actor: string, eventTyp return (row?.count ?? 0) > 0; } +/** #9000: how many of ANY of `eventTypes` exist for `targetKey` since `sinceIso`, actor-agnostic. The + * lost-click recovery needs "was this retrigger processed recently, by anyone or by the recovery itself" -- + * hasAuditEventForDelivery above is deliberately actor+delivery-keyed for redelivery guarding, which is the + * wrong shape here: a lost delivery has no deliveryId to match, and the processing pass may have run under a + * different actor than whoever ticked the box. */ +export async function countAuditEventsForTargetSince(env: Env, eventTypes: readonly string[], targetKey: string, sinceIso: string): Promise { + if (eventTypes.length === 0) return 0; + const db = getDb(env.DB); + const [row] = await db + .select({ count: sql`count(*)` }) + .from(auditEvents) + .where( + and( + inArray(auditEvents.eventType, [...eventTypes]), + eq(auditEvents.targetKey, targetKey), + gte(auditEvents.createdAt, sinceIso), + ), + ); + /* v8 ignore next -- count(*) always returns exactly one row; the empty-array guard only satisfies the destructure type. */ + return row?.count ?? 0; +} + /** Whether `eventType` has ALREADY been recorded for this `targetKey` at this EXACT `headSha` -- unlike * `hasAuditEventForDelivery` above (which guards a single redelivered webhook within a short window), this has * no time bound: a head SHA is a stable, permanent identity, so a match at any point in the past is still a diff --git a/src/github/comments.ts b/src/github/comments.ts index 885898f6c..77436322b 100644 --- a/src/github/comments.ts +++ b/src/github/comments.ts @@ -67,7 +67,7 @@ export async function createOrUpdatePrIntelligenceComment( pullNumber: number, body: string, options: { createIfMissing?: boolean | undefined; mode?: AgentActionMode } = {}, -): Promise<{ id: number; html_url?: string; changed: boolean } | null> { +): Promise<{ id: number; html_url?: string; changed: boolean; previousBody?: string } | null> { return createOrUpdateIssueCommentWithMarker(env, installationId, repoFullName, pullNumber, body, PR_INTELLIGENCE_COMMENT_MARKER, options); } @@ -130,7 +130,7 @@ async function createOrUpdateIssueCommentWithMarker( body: string, marker: string, options: { createIfMissing?: boolean | undefined; mode?: AgentActionMode } = {}, -): Promise<{ id: number; html_url?: string; changed: boolean } | null> { +): Promise<{ id: number; html_url?: string; changed: boolean; previousBody?: string } | null> { const parts = repoFullName.split("/"); const owner = parts[0]; const repo = parts[1]; @@ -168,12 +168,19 @@ async function createOrUpdateIssueCommentWithMarker( // marker — also collapses a duplicate webhook delivery for the same commit. // #9069: compared through comparableCommentBody so the panel's per-pass "Review updated" timestamp — which // changes on every render and made this check unreachable for the panel — no longer counts as a change. + // #9000: the body we are ABOUT to overwrite, surfaced to the caller. The panel renderer always emits + // its interactive checkboxes unticked, so a tick lives only in the live comment -- and this PATCH is + // the exact moment a lost-delivery click (a box the user ticked whose webhook never became a job) would + // otherwise be silently erased, leaving the maintainer certain ORB ignored them. Returning the previous + // body costs nothing (it was already fetched for the marker search) and lets the publish path notice + // and honor the intent instead of destroying the only evidence it existed. /* v8 ignore next -- `?? ""` is a type-level guard only: the marker filter above requires * `comment.body?.includes(candidate)`, so any comment that becomes `canonical` provably has a non-empty * string body. Kept because IssueComment types `body` as `string | null | undefined`. */ - if (comparableCommentBody(canonical.body ?? "") === comparableCommentBody(body)) { + const previousBody = canonical.body ?? ""; + if (comparableCommentBody(previousBody) === comparableCommentBody(body)) { await deleteDuplicateMarkerComments(octokit, owner, repo, existing, canonical.id); - return { id: canonical.id, ...(canonical.html_url !== undefined ? { html_url: canonical.html_url } : {}), changed: false }; + return { id: canonical.id, ...(canonical.html_url !== undefined ? { html_url: canonical.html_url } : {}), changed: false, previousBody }; } const response = await octokit.request("PATCH /repos/{owner}/{repo}/issues/comments/{comment_id}", { owner, @@ -182,7 +189,7 @@ async function createOrUpdateIssueCommentWithMarker( body, }); await deleteDuplicateMarkerComments(octokit, owner, repo, existing, canonical.id); - return { ...(response.data as { id: number; html_url?: string }), changed: true }; + return { ...(response.data as { id: number; html_url?: string }), changed: true, previousBody }; } if (options.createIfMissing === false) return null; const response = await octokit.request("POST /repos/{owner}/{repo}/issues/{issue_number}/comments", { diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 91998e444..cf3751108 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -69,6 +69,7 @@ import { countRecentAuditEventsForActorInRepo, countRecentAuditEventsForActorInRepoWithTargetSuffix, recentStaleRecheckDeniedPullNumbers, + countAuditEventsForTargetSince, hasAuditEventForDelivery, hasAuditEventForHeadSha, recordGateBlockOutcome, @@ -4536,7 +4537,29 @@ async function prReadyForReview( repoFullName, pr.number, `${PR_PANEL_COMMENT_MARKER}\n\n${renderWaitingForCiPlaceholder({ reason: waitingReason })}`, - ).catch(() => undefined); + ) + .then((placeholderResult) => + // #9000: this placeholder is the FIRST panel overwrite on a deferring pass, so it is where a + // lost click would otherwise be erased with the pass never reaching the final publish at all. + // A legitimately-deferring retrigger pass is protected by ordering, not luck: its handler now + // marks the pending marker BEFORE calling prReadyForReview, so the marker guard inside skips it. + maybeRecoverLostPanelRetrigger(env, { + previousBody: placeholderResult?.previousBody, + forceAiReview: undefined, + installationId, + repoFullName, + prNumber: pr.number, + /* v8 ignore next -- the null arm is structurally unreachable here: a falsy headSha makes + * prReadyForReview return true unconditionally (its own documented design), so this CI-wait + * placeholder never renders for a no-head PR. Type-level guard only. */ + headSha: pr.headSha ?? null, + prCreatedAt: pr.createdAt ?? null, + deliveryId, + }), + ).catch( + /* v8 ignore next -- fail-safe: recovery is additive; its failure must never turn a CI-wait deferral into a thrown error. */ + () => undefined, + ); } return false; } @@ -4759,6 +4782,87 @@ function pendingPrPanelRetriggerKey(repoFullName: string, prNumber: number, head return `pr-panel-retrigger-pending:${repoFullName.toLowerCase()}#${prNumber}:${headSha}`; } +// #9000, the lost-click half. Three checkbox states used to look identical to a maintainer: processed +// (panel republished, box reset), deferred for CI (box stays ticked, intent persisted by the pending marker +// above), and DELIVERY LOST (box stays ticked, nothing recorded, nothing will ever happen). Only the third is +// broken, and it is unrecoverable from our own state alone -- the tick exists only in the live comment body, +// because the webhook that would have told us about it never became a job (three such losses observed live on +// #8972, 2026-07-26). +// +// The interception point is the panel republish: createOrUpdatePrIntelligenceComment already fetches the +// existing comment for its marker search, and the publish was about to OVERWRITE the ticked box with the +// renderer's unconditional `- [ ]` -- erasing the only evidence of the click, on the exact pass best placed +// to honor it. So recovery costs zero extra API calls and needs no new sweep: any pass that republishes the +// panel (webhook, re-gate sweep, backlog convergence) doubles as the detector, which is what bounds the +// issue's "within one sweep interval" acceptance. +const PR_PANEL_RETRIGGER_RECOVERED_EVENT_TYPE = "github_app.pr_panel_retrigger_recovered"; + +async function maybeRecoverLostPanelRetrigger( + env: Env, + args: { + previousBody: string | undefined; + forceAiReview: boolean | undefined; + installationId: number; + repoFullName: string; + prNumber: number; + headSha: string | null; + prCreatedAt: string | null; + deliveryId: string; + }, +): Promise { + // The box in the body we just replaced was not ticked -- nothing to recover. This is the overwhelmingly + // common case and the only read it costs is a string scan of a body we already held. + if (!isCheckedPrPanelRetrigger(args.previousBody)) return; + // This pass IS the retrigger being processed (or a recovery pass consuming the marker below): overwriting + // its own tick is the normal receipt acknowledgement, not a loss. + if (args.forceAiReview === true) return; + const targetKey = `${args.repoFullName}#${args.prNumber}`; + const sinceIso = new Date(Date.now() - COMMAND_RATE_LIMIT_REDELIVERY_WINDOW_MS).toISOString(); + // A recent processed retrigger means the tick we replaced was ALREADY honored -- the delivery raced this + // pass rather than being lost. Actor-agnostic on purpose: any recent processing settles the question, and + // the audit query helpers are actor-keyed, so this reads the raw ledger. + const recent = await countAuditEventsForTargetSince( + env, + [ "github_app.pr_panel_retriggered", PR_PANEL_RETRIGGER_RECOVERED_EVENT_TYPE ], + targetKey, + sinceIso, + ); + if (recent > 0) return; + // The deferred case (#7626): the click WAS processed and is waiting for CI behind the pending marker. Peek, + // never consume -- consumption belongs to the review pass that will honor it. + if (args.headSha) { + const pending = await getTransientKey(env, pendingPrPanelRetriggerKey(args.repoFullName, args.prNumber, args.headSha)); + if (pending === PENDING_PR_PANEL_RETRIGGER_MARKER) return; + } + + // A genuinely lost click. Honor the intent exactly the way the deferred path does -- persist the pending + // marker so the next review pass consumes it as forceAiReview -- and enqueue that pass NOW rather than + // waiting for the next natural touch, since "the next natural touch" is precisely what a PR with a lost + // click cannot count on. The audit event doubles as the loop guard above and as the named reason #9003's + // invariant demands. + await recordAuditEvent(env, { + eventType: PR_PANEL_RETRIGGER_RECOVERED_EVENT_TYPE, + actor: null, + targetKey, + outcome: "queued", + detail: "panel rerun checkbox found ticked with no matching processing -- the delivery was lost; recovering the click as a forced re-review", + metadata: { deliveryId: args.deliveryId, repoFullName: args.repoFullName, headSha: args.headSha }, + }).catch( + /* v8 ignore next -- fail-safe: an audit write failure must not abort the recovery it is narrating. */ + () => undefined, + ); + await markPendingPrPanelRetrigger(env, args.repoFullName, args.prNumber, args.headSha); + const job: JobMessage = { + type: "agent-regate-pr", + deliveryId: `panel-retrigger-recovery:${args.repoFullName}#${args.prNumber}`, + repoFullName: args.repoFullName, + prNumber: args.prNumber, + installationId: args.installationId, + ...(args.prCreatedAt ? { prCreatedAt: args.prCreatedAt } : {}), + }; + await env.JOBS.send(job); +} + /** Persists the pending forceAiReview intent for this exact (repo, PR, headSha) -- best-effort: a storage * hiccup here just means the eventual natural re-evaluation reviews without the forced fresh call, the SAME * degraded-but-safe outcome as before this marker existed, never a thrown error or a blocked PR. */ @@ -11166,7 +11270,7 @@ async function maybePublishPrPublicSurface( if (shouldShowPlaceholderNow) { const placeholderBody = `${PR_PANEL_COMMENT_MARKER}\n\n${renderReviewingPlaceholder()}`; try { - await createOrUpdatePrIntelligenceComment( + const placeholderResult = await createOrUpdatePrIntelligenceComment( env, installationId, repoFullName, @@ -11174,6 +11278,21 @@ async function maybePublishPrPublicSurface( placeholderBody, { mode }, ); + // #9000: on a pass that runs the AI, THIS overwrite (not the final publish) is the one that + // replaces whatever the maintainer last saw -- including a ticked box whose delivery was lost. + await maybeRecoverLostPanelRetrigger(env, { + previousBody: placeholderResult?.previousBody, + forceAiReview: webhook.forceAiReview, + installationId, + repoFullName, + prNumber: pr.number, + headSha: pr.headSha ?? null, + prCreatedAt: pr.createdAt ?? null, + deliveryId: webhook.deliveryId, + }).catch( + /* v8 ignore next -- fail-safe: same additive-surface rule as the CI-wait hook; a recovery failure must not fail the placeholder publish. */ + () => undefined, + ); } catch (error) { /* v8 ignore next -- placeholder rate-limit propagation is covered by final-comment rate-limit tests. */ if (isGitHubRateLimitedError(error)) throw error; @@ -12855,6 +12974,23 @@ async function maybePublishPrPublicSurface( // idempotency check) sets this false -- a real create/update, or the createIfMissing:false null case // (not reachable on this call site, which never sets that option), both default true. commentContentChanged = commentResult?.changed ?? true; + // #9000: the body this publish just replaced may carry a ticked rerun checkbox whose webhook delivery + // was lost -- the overwrite above would otherwise be the moment that intent is silently erased. Fire-and- + // forget semantics are wrong here (the recovery enqueues a job), but a recovery failure must not fail + // the publish that hosted it, hence the terminal catch. + await maybeRecoverLostPanelRetrigger(env, { + previousBody: commentResult?.previousBody, + forceAiReview: webhook.forceAiReview, + installationId, + repoFullName, + prNumber: pr.number, + headSha: pr.headSha ?? null, + prCreatedAt: pr.createdAt ?? null, + deliveryId: webhook.deliveryId, + }).catch( + /* v8 ignore next -- fail-safe: a recovery failure must not fail the publish that hosted it. */ + () => undefined, + ); publishedOutputs.push("comment"); incr("loopover_reviews_published_total", { repo: repoFullName }); // Real end-to-end review latency (#review-latency-metric): pr.headShaObservedAt is the moment THIS @@ -14304,6 +14440,13 @@ async function maybeProcessPrPanelRetrigger( await refreshPullRequestDetails(env, repoFullName, pr.number, { force: true }); } const liveFacts = createLiveGithubFacts(); + // #7626 intent persistence, moved BEFORE the readiness check (#9000): prReadyForReview's own CI-wait + // placeholder republishes the panel, and the lost-click recovery hooked into every panel republish treats + // "ticked box, no recent processing, no pending marker" as a lost delivery. During THIS pass the click is + // very much being processed -- marking first is what tells the recovery so. The immediate-readiness path + // consumes the marker right back (it threads forceAiReview itself); a crash between mark and consume + // degrades to one extra forced review on the next pass, the safe direction for an explicit user click. + await markPendingPrPanelRetrigger(env, repoFullName, pr.number, pr.headSha); if ( !(await prReadyForReview( env, @@ -14315,10 +14458,9 @@ async function maybeProcessPrPanelRetrigger( liveFacts, )) ) { - // #7626: the explicit forceAiReview intent below is otherwise lost the instant this defers -- persist it - // so the next natural re-evaluation of this exact (repo, PR, headSha), whichever entry point reaches it - // first once CI settles, can still honor the user's click instead of silently replaying stale content. - await markPendingPrPanelRetrigger(env, repoFullName, pr.number, pr.headSha); + // The marker set above persists the forceAiReview intent across the deferral -- the next natural + // re-evaluation of this exact (repo, PR, headSha), whichever entry point reaches it first once CI + // settles, consumes it and forces the fresh review the user asked for. await recordAuditEvent(env, { eventType: "github_app.pr_panel_retrigger_deferred", actor, @@ -14329,6 +14471,9 @@ async function maybeProcessPrPanelRetrigger( }).catch(() => undefined); return true; } + // Immediate-readiness path: this pass threads forceAiReview directly, so the marker set above must not + // survive to force a SECOND review on some later, unrelated pass. One-shot consume; result discarded. + await consumePendingPrPanelRetrigger(env, repoFullName, pr.number, pr.headSha); await maybePublishPrPublicSurface( env, installationId, diff --git a/test/unit/github-comments.test.ts b/test/unit/github-comments.test.ts index b9ca33dd2..1022a72ac 100644 --- a/test/unit/github-comments.test.ts +++ b/test/unit/github-comments.test.ts @@ -396,7 +396,9 @@ describe("GitHub PR intelligence comments", () => { const result = await createOrUpdatePrIntelligenceComment(createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), 123, "JSONbored/gittensory", 12, body); - expect(result).toEqual({ id: 101, html_url: "https://github.com/comment/101", changed: false }); // html_url-present branch of the early return; changed:false is the #6724 no-op signal + // #9000: previousBody rides every canonical-comment return (identical or PATCHed) — it is how the panel + // publish detects a ticked rerun checkbox it is about to overwrite (the lost-click recovery). + expect(result).toEqual({ id: 101, html_url: "https://github.com/comment/101", changed: false, previousBody: body }); // html_url-present branch of the early return; changed:false is the #6724 no-op signal expect(calls.some((call) => call.startsWith("PATCH "))).toBe(false); // identical body → NO GitHub write }); @@ -416,7 +418,7 @@ describe("GitHub PR intelligence comments", () => { const result = await createOrUpdatePrIntelligenceComment(createTestEnv({ GITHUB_APP_PRIVATE_KEY: privateKey }), 123, "JSONbored/gittensory", 12, body); - expect(result).toEqual({ id: 202, changed: false }); // html_url-absent branch → no html_url key; changed:false is the #6724 no-op signal + expect(result).toEqual({ id: 202, changed: false, previousBody: body }); // html_url-absent branch → no html_url key; changed:false is the #6724 no-op signal expect(calls.some((call) => call.startsWith("PATCH "))).toBe(false); }); @@ -439,7 +441,9 @@ describe("GitHub PR intelligence comments", () => { // The whole point of #9069: a clock-only delta is NOT a content change, so no GitHub write and no // self-inflicted issue_comment.edited delivery. changed:false also keeps the #6724 no-op accounting honest. - expect(result).toEqual({ id: 303, html_url: "https://github.com/comment/303", changed: false }); + // previousBody is the POSTED body (what a reader saw), not the re-render — the distinction the + // lost-click recovery depends on, since a ticked checkbox only ever exists in the posted copy. + expect(result).toEqual({ id: 303, html_url: "https://github.com/comment/303", changed: false, previousBody: posted }); expect(calls.some((call) => call.startsWith("PATCH "))).toBe(false); }); diff --git a/test/unit/pr-panel-retrigger-lost-click-recovery.test.ts b/test/unit/pr-panel-retrigger-lost-click-recovery.test.ts new file mode 100644 index 000000000..7cc8f65fc --- /dev/null +++ b/test/unit/pr-panel-retrigger-lost-click-recovery.test.ts @@ -0,0 +1,322 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { + upsertInstallation, + upsertOfficialMinerDetection, + upsertPullRequestFromGitHub, + upsertRepositoryFromGitHub, + upsertRepositorySettings, +} from "../../src/db/repositories"; +import { processJob, reReviewStoredPullRequest } from "../../src/queue/processors"; +import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; +import { normalizeRegistryPayload } from "../../src/registry/normalize"; +import { persistRegistrySnapshot } from "../../src/registry/sync"; +import { asCloudEnv, createTestEnv } from "../helpers/d1"; +import { generatePrivateKeyPem } from "../helpers/github-app-key"; + +// #9000, the lost-click half. Three checkbox states looked identical to a maintainer: processed (panel +// republished, box reset), deferred for CI (box stays ticked, honored later via the #7626 pending marker), +// and DELIVERY LOST (box stays ticked, nothing recorded, nothing will ever happen -- three such losses +// observed live on #8972). Recovery intercepts the panel republish, which already fetches the existing +// comment for its marker search: a ticked box in the body being overwritten, with no recent processing and +// no pending marker, IS the lost intent -- and the overwrite was previously the moment it got erased. + +function queueMinerSnapshot(login: string) { + return { + source: "gittensor_api" as const, + githubId: "123", + githubUsername: login, + isEligible: true, + credibility: 1, + eligibleRepoCount: 1, + issueDiscoveryScore: 0, + issueTokenScore: 0, + issueCredibility: 1, + isIssueEligible: false, + issueEligibleRepoCount: 0, + alphaPerDay: 0, + taoPerDay: 0, + usdPerDay: 0, + totals: { + pullRequests: 3, + mergedPullRequests: 2, + openPullRequests: 1, + closedPullRequests: 0, + openIssues: 0, + closedIssues: 0, + solvedIssues: 0, + validSolvedIssues: 0, + }, + repositories: [], + pullRequests: [], + issueLabels: [], + }; +} + +// Same fixture shape as pr-panel-retrigger-pending-force-review.test.ts (#7626), this suite's sibling: it is +// the proven minimal seed that drives a real publish (comment surface on, AI in block mode) without needing +// merge/approve endpoint mocks. +async function seedRecoveryRepo(env: Env) { + await persistRegistrySnapshot( + asCloudEnv(env), + normalizeRegistryPayload( + { "JSONbored/gittensory": { emission_share: 0.01, issue_discovery_share: 0 } }, + { kind: "raw-github", url: "https://example.test" }, + "2026-05-23T00:00:00.000Z", + ), + ); + await upsertInstallation(env, { action: "created", installation: { id: 123, account: { login: "JSONbored", id: 1, type: "User" }, target_type: "User", repository_selection: "selected", permissions: {}, events: [] } }); + await upsertRepositoryFromGitHub(env, { name: "gittensory", full_name: "JSONbored/gittensory", private: false, owner: { login: "JSONbored" } }, 123); + await upsertRepositorySettings(env, { + repoFullName: "JSONbored/gittensory", + autoLabelEnabled: false, + gatePack: "oss-anti-slop", + autonomy: { label: "auto" }, + }); + await upsertRepoFocusManifest(env, "JSONbored/gittensory", { + settings: { + commentMode: "all_prs", + publicSurface: "comment_only", + checkRunMode: "off", + reviewCheckMode: "required", + aiReviewMode: "block", + }, + }); + await upsertOfficialMinerDetection(env, "contributor", { status: "confirmed", snapshot: queueMinerSnapshot("contributor") }, 60_000); +} + +/** The live panel body as GitHub would return it after a click whose webhook was lost: ticked box, our bot. */ +const TICKED_LIVE_PANEL = [ + "", + "", + "Stale review content from an earlier pass.", + "", + "- [x] Re-run LoopOver review", +].join("\n"); + +const UNTICKED_LIVE_PANEL = TICKED_LIVE_PANEL.replace("- [x]", "- [ ]"); + +function stubGitHub(prNumber: number, sha: string, livePanelBody: string | null, opts: { ciPending?: boolean } = {}) { + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url.includes("/access_tokens")) return Response.json({ token: "fake-installation-token" }); + if (url.includes(`/pulls/${prNumber}/files`)) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + // sha "" models the no-head ghost PR: the resync would otherwise self-heal the fixture from this + // live read and the recovery would never see a SHA-less record. + if (url.endsWith(`/pulls/${prNumber}`)) return Response.json({ number: prNumber, title: "Recovery PR", state: "open", user: { login: "contributor" }, head: sha === "" ? {} : { sha }, labels: [], body: "Closes #1", mergeable_state: "clean" }); + if (url.includes(`/commits/${sha}/check-runs`)) { + return opts.ciPending + ? Response.json({ total_count: 1, check_runs: [{ name: "test", status: "in_progress", conclusion: null, app: { slug: "github-actions" } }] }) + : Response.json({ total_count: 1, check_runs: [{ name: "test", status: "completed", conclusion: "success", app: { slug: "github-actions" } }] }); + } + if (url.includes(`/commits/${sha}/status`)) return Response.json({ state: opts.ciPending ? "pending" : "success", statuses: [] }); + if (url.includes("/issues/1")) return Response.json({ number: 1, title: "Issue", state: "open", labels: [], user: { login: "reporter" } }); + if (url.includes(`/issues/${prNumber}/comments`) && method === "GET") { + return Response.json( + livePanelBody === null ? [] : [{ id: 900, body: livePanelBody, user: { login: "loopover-orb[bot]", type: "Bot" } }], + ); + } + if (url.includes("/comments") && (method === "POST" || method === "PATCH")) return Response.json({ id: 900 }, { status: 201 }); + if (url.includes("/branches/")) return Response.json({ protected: false, protection: { required_status_checks: { contexts: [] } } }); + return Response.json({}); + }); +} + +async function recoveryEnv(onAi?: () => void) { + return createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => { onAi?.(); return { response: JSON.stringify({ assessment: "Fresh opinion.", blockers: [], nits: [], suggestions: [] }) }; } } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + }); +} + +async function seedPr(env: Env, prNumber: number, sha: string) { + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: prNumber, + title: "Recovery PR", + state: "open", + user: { login: "contributor" }, + author_association: "CONTRIBUTOR", + head: { sha }, + base: { ref: "main" }, + labels: [], + body: "Closes #1", + created_at: "2026-07-20T00:00:00Z", + }); +} + +async function recoveredEventCount(env: Env): Promise { + const row = await env.DB.prepare("select count(*) as n from audit_events where event_type = ?") + .bind("github_app.pr_panel_retrigger_recovered") + .first<{ n: number }>(); + return row?.n ?? 0; +} + +describe("PR-panel retrigger lost-click recovery (#9000)", () => { + beforeEach(async () => { + vi.useFakeTimers({ toFake: ["Date"] }); + vi.setSystemTime(new Date("2026-07-28T00:00:00.000Z")); + }); + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + it("REGRESSION (#8972 shape): a ticked box with no matching processing is recovered, not erased — event + marker + prompt re-review job", async () => { + const env = await recoveryEnv(); + const sent: unknown[] = []; + (env as { JOBS: Queue }).JOBS = { send: async (job: unknown) => { sent.push(job); } } as unknown as Queue; + await seedRecoveryRepo(env); + await seedPr(env, 801, "shaLost"); + stubGitHub(801, "shaLost", TICKED_LIVE_PANEL); + + // A NATURAL pass — a sweep re-review, not a retrigger webhook (the webhook is the thing that was lost). + await reReviewStoredPullRequest(env, "sweep-natural-801", 123, "JSONbored/gittensory", 801); + + // The recovery named itself (#9003's invariant) rather than silently erasing the tick with `- [ ]`. + expect(await recoveredEventCount(env)).toBe(1); + // ...and enqueued a prompt follow-up pass, since a PR with a lost click cannot count on a natural one. + const recovery = sent.find((job) => (job as { deliveryId?: string }).deliveryId?.startsWith("panel-retrigger-recovery:")); + expect(recovery).toMatchObject({ + type: "agent-regate-pr", + repoFullName: "JSONbored/gittensory", + prNumber: 801, + installationId: 123, + prCreatedAt: expect.any(String), // the #9499 oldest-first sort key — omitting it INVERTS queue order + }); + }); + + it("end-to-end: the recovery job's pass consumes the marker, forces a FRESH AI review, and does not re-fire", async () => { + let aiCalls = 0; + const env = await recoveryEnv(() => { aiCalls += 1; }); + const sent: Array<{ deliveryId?: string }> = []; + (env as { JOBS: Queue }).JOBS = { send: async (job: unknown) => { sent.push(job as { deliveryId?: string }); } } as unknown as Queue; + await seedRecoveryRepo(env); + await seedPr(env, 802, "shaLost2"); + stubGitHub(802, "shaLost2", TICKED_LIVE_PANEL); + + await reReviewStoredPullRequest(env, "sweep-natural-802", 123, "JSONbored/gittensory", 802); + const aiCallsAfterDetection = aiCalls; + const recovery = sent.find((job) => job.deliveryId?.startsWith("panel-retrigger-recovery:")); + expect(recovery).toBeDefined(); + + // GitHub now shows the box unticked (the detection pass republished the panel) — so the recovery pass + // must take its intent from the CONSUMED MARKER, not from re-reading the comment. + stubGitHub(802, "shaLost2", UNTICKED_LIVE_PANEL); + await processJob(env, recovery as Parameters[1]); + + // The lost click bought exactly one fresh review (the #7626 marker is one-shot)... + expect(aiCalls).toBeGreaterThan(aiCallsAfterDetection); + // ...and the recovery pass did not diagnose ITS own republish as a second lost click. + expect(await recoveredEventCount(env)).toBe(1); + }); + + it("INVARIANT: a recently-PROCESSED retrigger is not re-recovered — the delivery raced the pass, it was not lost", async () => { + const env = await recoveryEnv(); + await seedRecoveryRepo(env); + await seedPr(env, 803, "shaRaced"); + // The processed event a real retrigger records, moments ago. + await env.DB.prepare( + `INSERT INTO audit_events (id, event_type, actor, target_key, outcome, detail, metadata_json, created_at) + VALUES ('race1', 'github_app.pr_panel_retriggered', 'maintainer', 'JSONbored/gittensory#803', 'completed', 'x', '{}', ?)`, + ).bind(new Date(Date.now() - 60_000).toISOString()).run(); + stubGitHub(803, "shaRaced", TICKED_LIVE_PANEL); + + await reReviewStoredPullRequest(env, "sweep-natural-803", 123, "JSONbored/gittensory", 803); + + expect(await recoveredEventCount(env)).toBe(0); + }); + + it("INVARIANT: a DEFERRED click (pending marker present) is not double-recovered — it is already being honored", async () => { + // The #7626 flow: the retrigger handler marks the pending marker BEFORE its readiness check, so any panel + // republish that happens while the click is deferred (including the CI-wait placeholder inside that very + // pass) sees the marker and stands down. Without this guard, every deferral would also spawn a recovery. + // Same injected-cache seam the sibling #7626 suite uses -- putTransientKey is module-private, so the + // marker is seeded exactly as markPendingPrPanelRetrigger writes it (lowercased repo, same value). + const cache = { + values: new Map([["pr-panel-retrigger-pending:jsonbored/gittensory#806:shaDeferred", "1"]]), + async get(key: string): Promise { return this.values.get(key) ?? null; }, + async set(key: string, value: string): Promise { this.values.set(key, value); }, + async del(key: string): Promise { this.values.delete(key); }, + }; + const env = createTestEnv({ + GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem(), + AI: { run: async () => ({ response: JSON.stringify({ assessment: "Fresh opinion.", blockers: [], nits: [], suggestions: [] }) }) } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + AI_DAILY_NEURON_BUDGET: "100000", + SELFHOST_TRANSIENT_CACHE: cache, + }); + await seedRecoveryRepo(env); + await seedPr(env, 806, "shaDeferred"); + // CI PENDING is the load-bearing part: the pass defers inside prReadyForReview and its CI-wait + // placeholder republishes the ticked panel BEFORE the #7626 marker would be consumed -- exactly the + // window in which a deferring retrigger's own click must not be misread as lost. + stubGitHub(806, "shaDeferred", TICKED_LIVE_PANEL, { ciPending: true }); + + await reReviewStoredPullRequest(env, "sweep-natural-806", 123, "JSONbored/gittensory", 806); + + expect(await recoveredEventCount(env)).toBe(0); + // ...and the marker survives untouched for the pass that WILL honor it once CI settles. + expect(cache.values.get("pr-panel-retrigger-pending:jsonbored/gittensory#806:shaDeferred")).toBe("1"); + }); + + it("a ghost PR (no head SHA, no created_at) still recovers — the marker guard is skipped, never crashed", async () => { + // The no-head-ghost-PR shape the placeholder logic already special-cases: recovery cannot key a pending + // marker without a SHA, so it proceeds straight to recovering (the safe direction for an explicit click), + // and the enqueued job simply omits the optional prCreatedAt sort key. + const env = await recoveryEnv(); + const sent: Array<{ deliveryId?: string; prCreatedAt?: string }> = []; + (env as { JOBS: Queue }).JOBS = { send: async (job: unknown) => { sent.push(job as never); } } as unknown as Queue; + await seedRecoveryRepo(env); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { + number: 807, + title: "Ghost PR", + state: "open", + user: { login: "contributor" }, + author_association: "CONTRIBUTOR", + head: { sha: "" }, + base: { ref: "main" }, + labels: [], + body: "Closes #1", + } as never); + stubGitHub(807, "", TICKED_LIVE_PANEL); + + await reReviewStoredPullRequest(env, "sweep-ghost-807", 123, "JSONbored/gittensory", 807); + + expect(await recoveredEventCount(env)).toBe(1); + const recovery = sent.find((job) => job.deliveryId?.startsWith("panel-retrigger-recovery:")); + expect(recovery).toBeDefined(); + expect(recovery?.prCreatedAt).toBeUndefined(); // the conditional-spread arm: no createdAt, no sort key + }); + + it("countAuditEventsForTargetSince returns 0 for an empty event-type list without touching the DB", async () => { + const { countAuditEventsForTargetSince } = await import("../../src/db/repositories"); + const env = await recoveryEnv(); + expect(await countAuditEventsForTargetSince(env, [], "o/r#1", new Date(0).toISOString())).toBe(0); + }); + + it("INVARIANT: a box the panel shows UNTICKED recovers nothing — the common case costs one string scan", async () => { + const env = await recoveryEnv(); + await seedRecoveryRepo(env); + await seedPr(env, 804, "shaClean"); + stubGitHub(804, "shaClean", UNTICKED_LIVE_PANEL); + + await reReviewStoredPullRequest(env, "sweep-natural-804", 123, "JSONbored/gittensory", 804); + + expect(await recoveredEventCount(env)).toBe(0); + }); + + it("INVARIANT: no prior panel comment at all (first publish) recovers nothing", async () => { + const env = await recoveryEnv(); + await seedRecoveryRepo(env); + await seedPr(env, 805, "shaFirst"); + stubGitHub(805, "shaFirst", null); + + await reReviewStoredPullRequest(env, "sweep-first-805", 123, "JSONbored/gittensory", 805); + + expect(await recoveredEventCount(env)).toBe(0); + }); +}); diff --git a/test/unit/pr-panel-retrigger-pending-force-review.test.ts b/test/unit/pr-panel-retrigger-pending-force-review.test.ts index 5e3a61be0..8b473bc57 100644 --- a/test/unit/pr-panel-retrigger-pending-force-review.test.ts +++ b/test/unit/pr-panel-retrigger-pending-force-review.test.ts @@ -276,9 +276,12 @@ describe("PR-panel retrigger pending force-review marker (#7626)", () => { .bind("github_app.pr_panel_retriggered", "JSONbored/gittensory#702") .first<{ outcome: string }>(); expect(retriggeredAudit?.outcome).toBe("completed"); // the immediate path really did run (sanity) - // No pending-retrigger marker was ever written -- the immediate-success path sets forceAiReview directly - // and never touches markPendingPrPanelRetrigger (that only runs on the DEFER branch). - expect([...cache.values.keys()].some((key) => key.startsWith("pr-panel-retrigger-pending:"))).toBe(false); + // No pending-retrigger INTENT survives the immediate path. (#9000 changed the mechanism: the handler now + // marks the pending intent BEFORE the readiness check -- so the lost-click recovery hooked into the + // CI-wait placeholder can tell a mid-processing click from a lost one -- and consumes it right back on + // this immediate branch. Consumption may leave the one-shot "consumed" sentinel behind on adapters + // without a real delete, so the invariant is "no key still holds the PENDING value", not key absence.) + expect([...cache.values.entries()].some(([key, value]) => key.startsWith("pr-panel-retrigger-pending:") && value === "1")).toBe(false); }); it("a new commit (different head SHA) after a pending-but-unhonored marker does not inherit the old marker's forceAiReview intent", async () => {