From b5a6f0e1d6ffefe502a45bf4cc9ff4f18d957428 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Tue, 28 Jul 2026 12:17:59 -0700 Subject: [PATCH] fix(review): recover lost panel-retrigger clicks instead of silently erasing them (#9000) The lost-click half of #9000, completing the issue (the skip-auditing half landed earlier; its enforcement went in as #9583). THE DEFECT. 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, 2026-07-26). 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. Worse, the next panel republish -- whose renderer emits its checkboxes unticked unconditionally -- would OVERWRITE the ticked box, erasing the only evidence of the click on the exact pass best placed to honor it. THE FIX, at zero additional API cost. createOrUpdateIssueCommentWithMarker already fetches the existing comment for its marker search; it now returns that pre-overwrite body. Every panel republish (webhook pass, re-gate sweep, backlog convergence) doubles as the detector: a ticked rerun box in the body being replaced, with no recent processing and no pending marker, IS a lost click. Recovery then: - records github_app.pr_panel_retrigger_recovered (the named reason #9003 demands, and the loop guard against re-detection), - persists the #7626 pending marker, so the next review pass consumes it as forceAiReview exactly like a deferred click, and - enqueues an agent-regate-pr job immediately (with the #9499 prCreatedAt sort key) -- a PR with a lost click is precisely the PR that cannot count on a natural pass. No new sweep, no new persistence: the issue's "within one sweep interval" acceptance is bounded by the republish cadence that already exists. FALSE-POSITIVE PROOFING, the part that took the design care: - A pass that IS the retrigger (forceAiReview) skips detection -- overwriting its own tick is the receipt acknowledgement, not a loss. - A recently-processed retrigger (actor-agnostic audit lookup, new countAuditEventsForTargetSince -- hasAuditEventForDelivery is deliberately actor+delivery-keyed, the wrong shape when the delivery never existed) means the delivery raced the pass rather than being lost. - A DEFERRED click is guarded by ordering, not luck: the retrigger handler now marks the pending marker BEFORE its readiness check (consuming it right back on the immediate path), so the CI-wait placeholder inside that very pass sees the marker and stands down. A crash between mark and consume degrades to one extra forced review -- the safe direction for an explicit user click. The #7626 sibling test's "never creates a pending marker" assertion is updated to the invariant it documents ("no pending INTENT survives the immediate path") rather than its old mechanism (key absence), since consumption can leave the one-shot consumed sentinel behind on adapters without a real delete. TESTS. Eight in the new suite: the #8972 regression (natural pass recovers the tick: event + marker + job with the sort key), full end-to-end (the recovery job's pass consumes the marker, spends exactly one fresh AI review, and does not re-diagnose its own republish), the raced-delivery guard, the deferred-click guard (CI genuinely pending, marker untouched afterward), the no-head ghost PR (marker guard skipped, job omits the optional sort key), unticked and first-publish no-ops, and the empty event-type-list guard. Plus the three github-comments assertions extended to pin previousBody, including that it is the POSTED body, not the re-render, on the byte-identical skip path. Changed lines: 0 uncovered statements, 0 uncovered branches. --- src/db/repositories.ts | 22 ++ src/github/comments.ts | 17 +- src/queue/processors.ts | 157 ++++++++- test/unit/github-comments.test.ts | 10 +- ...anel-retrigger-lost-click-recovery.test.ts | 322 ++++++++++++++++++ ...nel-retrigger-pending-force-review.test.ts | 9 +- 6 files changed, 520 insertions(+), 17 deletions(-) create mode 100644 test/unit/pr-panel-retrigger-lost-click-recovery.test.ts diff --git a/src/db/repositories.ts b/src/db/repositories.ts index edfc08f76a..bff5bd1775 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 885898f6c1..77436322bd 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 91998e4445..cf37511083 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 b9ca33dd21..1022a72ac1 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 0000000000..7cc8f65fc8 --- /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 5e3a61be0a..8b473bc57f 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 () => {