From e1b3cb6fc3aa6b154445fd79155404c4b7d29474 Mon Sep 17 00:00:00 2001 From: Nick M <274344962+nickmopen@users.noreply.github.com> Date: Thu, 9 Jul 2026 16:22:27 -0500 Subject: [PATCH] feat(miner-manage): add a proactive decision-pack rebuild trigger on pr_outcome write (#4283) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Today a contributor's decision pack only rebuilds when next REQUESTED and found older than DECISION_PACK_MAX_AGE_MS (~6h passive check). This wires recordPrOutcome (the pull_request.closed handler) to proactively enqueue a rebuild for the PR author the moment a real outcome is recorded, so the pack refreshes within seconds instead of up to 6h later. - outcomes-wire.ts: after the pr_outcome row is written, call tryEnqueueDecisionPackRebuild (src/services/decision-pack.ts) for the already-extracted authorLogin. Best-effort + non-blocking (.catch) so an enqueue failure never affects outcome recording; the non-authoritative self-close already returned above; empty author login is skipped. The ~6h passive check stays as the fallback ceiling — this only ADDS a proactive trigger. - Tests: merged close enqueues for the author (lowercased); self-close does NOT; a ghost/no-author-login PR does NOT; an enqueue failure never throws. Verified locally: preflight (secret-scan+typecheck) pass; codecov lcov new block fully covered (lines+branches); full suite 12815 passed, 0 failed. --- src/review/outcomes-wire.ts | 17 ++++++ ...iner-decision-pack-rebuild-trigger.test.ts | 54 +++++++++++++++++++ 2 files changed, 71 insertions(+) create mode 100644 test/unit/miner-decision-pack-rebuild-trigger.test.ts diff --git a/src/review/outcomes-wire.ts b/src/review/outcomes-wire.ts index 2f7fcc76ba..ba22c683b2 100644 --- a/src/review/outcomes-wire.ts +++ b/src/review/outcomes-wire.ts @@ -24,6 +24,7 @@ // once a repo's merge precision actually drops below the floor over a real sample. import { recordAuditEvent } from "../db/repositories"; +import { tryEnqueueDecisionPackRebuild } from "../services/decision-pack"; import { incr } from "../selfhost/metrics"; import type { GitHubWebhookPayload } from "../types"; import { errorMessage, nowIso } from "../utils/json"; @@ -337,6 +338,22 @@ export async function recordPrOutcome( ), ); + // #4283: proactively refresh THIS PR author's decision pack now (within seconds) instead of waiting up to + // DECISION_PACK_MAX_AGE_MS (~6h) for the next passive staleness read at serving time. Best-effort + non-blocking — + // an enqueue failure must never affect pr_outcome recording (mirrors the caller's own `.catch` at processors.ts). + // A non-authoritative self-close already returned above, so this only fires on real outcomes; skip an empty login. + // The 6h passive check stays as the fallback ceiling for authors this proactive path misses. + if (authorLogin) { + await tryEnqueueDecisionPackRebuild(env, authorLogin).catch((error) => + console.warn( + JSON.stringify({ + event: "pr_outcome_decision_pack_rebuild_error", + message: errorMessage(error).slice(0, 160), + }), + ), + ); + } + // Discord/Slack action notifications are emitted by the action executor, which knows the exact bot action that // was attempted and can audit the delivery. This outcome recorder only stores realized ground truth. Emitting // another webhook from the GitHub `pull_request.closed` event duplicated bot-action notifications and could diff --git a/test/unit/miner-decision-pack-rebuild-trigger.test.ts b/test/unit/miner-decision-pack-rebuild-trigger.test.ts new file mode 100644 index 0000000000..6cc8313895 --- /dev/null +++ b/test/unit/miner-decision-pack-rebuild-trigger.test.ts @@ -0,0 +1,54 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import { createTestEnv } from "../helpers/d1"; +import type { GitHubWebhookPayload } from "../../src/types"; + +// Spy on the one function this issue wires up; keep every other decision-pack export real so outcomes-wire's +// transitive graph is unaffected. +const { enqueueSpy } = vi.hoisted(() => ({ enqueueSpy: vi.fn() })); +vi.mock("../../src/services/decision-pack", async (importActual) => ({ + ...(await importActual()), + tryEnqueueDecisionPackRebuild: enqueueSpy, +})); + +import { recordPrOutcome } from "../../src/review/outcomes-wire"; + +const closedPr = (opts: { author: string; sender: string; merged: boolean; senderType?: string }) => + ({ + action: "closed", + pull_request: { number: 7, merged_at: opts.merged ? "2026-01-01T00:00:00Z" : null, user: { login: opts.author } }, + repository: { full_name: "acme/widgets" }, + sender: { login: opts.sender, type: opts.senderType ?? "User" }, + }) as unknown as GitHubWebhookPayload; + +describe("recordPrOutcome → proactive decision-pack rebuild (#4283)", () => { + beforeEach(() => enqueueSpy.mockReset().mockResolvedValue(true)); + + it("enqueues a rebuild for the PR author on a merged (maintainer-authoritative) close", async () => { + const env = createTestEnv(); + await recordPrOutcome(env, "pull_request", closedPr({ author: "Miner1", sender: "maintainer", merged: true })); + expect(enqueueSpy).toHaveBeenCalledWith(env, "miner1"); // authorLogin, lowercased + }); + + it("does NOT enqueue on a self-close (author closes their own unmerged PR — anti-poisoning suppression)", async () => { + const env = createTestEnv(); + await recordPrOutcome(env, "pull_request", closedPr({ author: "miner1", sender: "miner1", merged: false })); + expect(enqueueSpy).not.toHaveBeenCalled(); + }); + + it("does NOT enqueue when the PR has no author login (ghost/deleted account)", async () => { + const env = createTestEnv(); + // merged (so the self-close guard doesn't apply), but no user login ⇒ authorLogin is empty ⇒ skip the enqueue + const payload = { action: "closed", pull_request: { number: 7, merged_at: "2026-01-01T00:00:00Z", user: null }, repository: { full_name: "acme/widgets" }, sender: { login: "maintainer", type: "User" } } as unknown as GitHubWebhookPayload; + await recordPrOutcome(env, "pull_request", payload); + expect(enqueueSpy).not.toHaveBeenCalled(); + }); + + it("a rebuild-enqueue failure never throws out of recordPrOutcome", async () => { + const env = createTestEnv(); + enqueueSpy.mockRejectedValueOnce(new Error("boom")); + await expect( + recordPrOutcome(env, "pull_request", closedPr({ author: "miner1", sender: "bot", senderType: "Bot", merged: true })), + ).resolves.toBeUndefined(); + expect(enqueueSpy).toHaveBeenCalledWith(env, "miner1"); + }); +});