From 7f62af7fd592d55f3017120d14906634be9b92f4 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 22 Jun 2026 18:25:46 -0700 Subject: [PATCH 1/5] =?UTF-8?q?fix(github):=20cache=20installation=20token?= =?UTF-8?q?s=20=E2=80=94=20stop=20the=20rate-limit=20storm=20that=20dead-l?= =?UTF-8?q?ettered=20reviews?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ROOT CAUSE of the dead-lettered reviews + stale head SHAs: createInstallationToken minted a FRESH GitHub token on EVERY call (no cache). The converged review path mints several tokens per review (live CI aggregate, mergeable fetch, file resolve, visual capture, permission check) and across the sweep + re-reviews this exhausted the hourly GitHub rate limit (observed min_remaining=0 at 6753 calls/hr) → reviews errored → dead-lettered → missed synchronize webhooks → stale stored head SHAs → reviews ran against the old (green) commit → wrong 'CI is green' approvals after a rebase. FIX: in-isolate token cache (valid ~1h, 2-min safety margin) → ~1 mint/hour/installation instead of per-call. Tests: clear the cache per-test; token-count assertions drop to the cached count. --- src/github/app.ts | 21 ++++++++++++++++++++- test/unit/github-app.test.ts | 5 ++++- test/unit/queue.test.ts | 18 +++++++++++------- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/src/github/app.ts b/src/github/app.ts index 40cc104702..15265cdc27 100644 --- a/src/github/app.ts +++ b/src/github/app.ts @@ -37,7 +37,18 @@ function timeoutFetch(input: RequestInfo | URL, init?: RequestInit): Promise(); +const TOKEN_SAFETY_MARGIN_MS = 120_000; + export async function createInstallationToken(env: Env, installationId: number): Promise { + const cached = installationTokenCache.get(installationId); + if (cached && cached.expiresAtMs - TOKEN_SAFETY_MARGIN_MS > Date.now()) return cached.token; const jwt = await createAppJwt(env); const response = await timeoutFetch(`https://api.github.com/app/installations/${installationId}/access_tokens`, { method: "POST", @@ -47,11 +58,19 @@ export async function createInstallationToken(env: Env, installationId: number): const body = await response.text(); throw new Error(`Failed to create GitHub installation token (${response.status}): ${body.slice(0, 200)}`); } - const payload = (await response.json()) as { token?: string }; + const payload = (await response.json()) as { token?: string; expires_at?: string }; if (!payload.token) throw new Error("GitHub installation token response did not include a token."); + const expiresAtMs = payload.expires_at ? Date.parse(payload.expires_at) : Date.now() + 50 * 60_000; + installationTokenCache.set(installationId, { token: payload.token, expiresAtMs }); return payload.token; } +/** Test-only: clear the in-isolate installation-token cache so each test starts fresh (the module-level Map + * otherwise leaks a cached token across test cases that share an installation id). */ +export function clearInstallationTokenCacheForTest(): void { + installationTokenCache.clear(); +} + export async function getAppInstallation(env: Env, installationId: number): Promise> { const jwt = await createAppJwt(env); const response = await timeoutFetch(`https://api.github.com/app/installations/${installationId}`, { diff --git a/test/unit/github-app.test.ts b/test/unit/github-app.test.ts index 39327ff339..185e96e956 100644 --- a/test/unit/github-app.test.ts +++ b/test/unit/github-app.test.ts @@ -1,6 +1,7 @@ -import { afterEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { generateKeyPairSync } from "node:crypto"; import { + clearInstallationTokenCacheForTest, createInstallationToken, createOrUpdateCheckRun, createOrUpdateGateCheckRun, @@ -13,6 +14,8 @@ import { import type { Advisory } from "../../src/types"; import { createTestEnv } from "../helpers/d1"; +beforeEach(() => clearInstallationTokenCacheForTest()); + describe("GitHub check runs", () => { afterEach(() => { vi.unstubAllGlobals(); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 9fc3972048..efd85b854c 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { clearInstallationTokenCacheForTest } from "../../src/github/app"; import { listCollisionEdges, createAgentRun, @@ -44,6 +45,7 @@ describe("queue processors", () => { // Freshness-SLO fixtures are dated relative to late May 2026; pin the clock so staleness windows // stay deterministic regardless of when CI runs. beforeEach(() => { + clearInstallationTokenCacheForTest(); vi.useFakeTimers({ toFake: ["Date"] }); vi.setSystemTime(new Date("2026-05-28T00:00:00.000Z")); }); @@ -1959,7 +1961,8 @@ describe("queue processors", () => { }, }); - expect(calls).toEqual({ token: 2, permission: 1, minerList: 1, commentGets: 1, commentPatches: 1 }); + // token: 1 — the installation token is now cached + reused within the request (was 2: main + permission check). + expect(calls).toEqual({ token: 1, permission: 1, minerList: 1, commentGets: 1, commentPatches: 1 }); expect(patchedBody).toContain(""); expect(patchedBody).toContain("Readiness score:"); expect(patchedBody).toContain("- [ ] Re-run Gittensory review"); @@ -2268,7 +2271,8 @@ describe("queue processors", () => { }, }); - expect(calls).toEqual({ token: 2, permission: 1, minerList: 1, commentGets: 1, commentPatches: 1 }); + // token: 1 — the installation token is now cached + reused within the request (was 2: main + permission check). + expect(calls).toEqual({ token: 1, permission: 1, minerList: 1, commentGets: 1, commentPatches: 1 }); }); it("skips PR panel reruns when the editing actor and PR author are unavailable", async () => { @@ -4045,9 +4049,9 @@ describe("queue processors", () => { }); expect(calls.commentsCreated).toBe(8); - // Each command now also resolves the commenter's real repo permission (#788), which fetches its own - // installation token — so the per-command token count is 2 (permission check + comment). - expect(calls.token).toBe(16); + // The installation token is cached + reused across all 8 commands (each previously minted 2 — permission + // check + comment — for 16 total). Caching collapses them to a single mint, which is the rate-limit fix. + expect(calls.token).toBe(1); expect(calls.minerList).toBeGreaterThanOrEqual(1); const audit = await env.DB.prepare("select event_type, detail from audit_events where target_key = ? order by created_at") .bind("JSONbored/gittensory#77") @@ -4154,7 +4158,7 @@ describe("queue processors", () => { }, }); - expect(calls).toEqual({ commentsCreated: 1, token: 2 }); // +1 token for the #788 permission check + expect(calls).toEqual({ commentsCreated: 1, token: 1 }); // token cached + reused across the #788 permission check const audit = await env.DB.prepare("select event_type, detail, metadata_json from audit_events where target_key = ? order by created_at") .bind("JSONbored/gittensory#90") .all<{ event_type: string; detail: string | null; metadata_json: string }>(); @@ -4287,7 +4291,7 @@ describe("queue processors", () => { }, }); - expect(calls).toEqual({ commentsCreated: 1, token: 2, minerList: 0 }); // +1 token for the #788 permission check + expect(calls).toEqual({ commentsCreated: 1, token: 1, minerList: 0 }); // token cached + reused across the #788 permission check const audit = await env.DB.prepare("select event_type, detail from audit_events where target_key = ? order by created_at") .bind("JSONbored/gittensory#91") .all<{ event_type: string; detail: string | null }>(); From f43d3c71474a1f8f04a8ef7e11b50bf62e441671 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 22 Jun 2026 18:49:06 -0700 Subject: [PATCH 2/5] fix(review): re-gate sweep RE-PUBLISHES (refresh stale comments/labels on idle PRs) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The sweep re-gated + ran auto-maintain but never re-published the unified comment, so an idle PR kept a stale comment ('safe to merge' from before a fix) + stale status label forever even after the head was re-synced. Switch to reReviewStoredPullRequest (rebuild advisory → re-publish comment with current head/CI → re-run auto-maintain) so comment + label + action reflect reality. Paced at SWEEP_MAX_PRS/sweep; cheap with token caching. --- src/queue/processors.ts | 22 ++++++++-------------- 1 file changed, 8 insertions(+), 14 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index a8638c1873..db7b6e5973 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -543,21 +543,15 @@ async function sweepRepoRegate(env: Env, repoFullName: string | undefined): Prom const gate = evaluateGateCheck(advisory, gateCheckPolicy(settings, null, undefined, pr.slopRisk ?? null)); verdicts[String(pr.number)] = gate.conclusion; if (gate.conclusion === "failure" || gate.conclusion === "action_required") flaggedPulls.push(pr.number); - // Backstop the CI-completion trigger: re-run auto-maintain so a clean+green+approved PR is merged and a - // red-CI non-owner PR is closed (owner held) even if its check_run/check_suite webhook was missed or - // coalesced. maybeRunAgentMaintenance self-guards on autonomy + fetches the live CI aggregate itself. + // Backstop the CI-completion trigger AND refresh the stale public comment: fully RE-REVIEW each stale open + // PR (rebuild advisory → re-publish the unified comment with the current head/CI → re-run auto-maintain). The + // re-gate above only recomputes the audit verdict; without this re-publish, an idle PR keeps a stale comment + // (e.g. "safe to merge" from before a fix) and a stale status label forever. reReviewStoredPullRequest reads + // the freshly-synced head + the live CI, so the comment + label + action all reflect reality. Paced at + // SWEEP_MAX_PRS per sweep; cheap now that installation tokens are cached. if (sweepInstallationId != null) { - await maybeRunAgentMaintenance(env, { - installationId: sweepInstallationId, - repoFullName, - repo, - pr, - settings, - otherOpenPullRequests: others, - deliveryId: `regate-sweep:${repoFullName}#${pr.number}`, - gate, - }).catch((error) => { - console.error(JSON.stringify({ level: "warn", event: "agent_maintenance_failed", deliveryId: `regate-sweep:${repoFullName}#${pr.number}`, repository: repoFullName, pullNumber: pr.number, error: errorMessage(error) })); + await reReviewStoredPullRequest(env, `regate-sweep:${repoFullName}#${pr.number}`, sweepInstallationId, repoFullName, pr.number).catch((error) => { + console.error(JSON.stringify({ level: "warn", event: "sweep_rereview_failed", deliveryId: `regate-sweep:${repoFullName}#${pr.number}`, repository: repoFullName, pullNumber: pr.number, error: errorMessage(error) })); }); } } From 34b32b01e6a6aafc45abf7e0ff080dde7177ba57 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 22 Jun 2026 19:16:52 -0700 Subject: [PATCH 3/5] fix(ai-review): raise neuron budget off the free-tier 10k cap (enterprise) + interactive re-run checkbox MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The 10k AI_DAILY_NEURON_BUDGET default starved EVERY dual-AI review (~573 neurons each) into quota_exceeded on the enterprise Workers AI plan (account does hundreds of k/day), so no review ever produced its synthesis. Models were correct all along (gpt-oss-120b + nemotron-3-120b-a12b); the self-imposed cap was the sole blocker. - AI_DAILY_NEURON_BUDGET 10k -> 2M; code clamp ceiling 1M -> 10M (runaway backstop, not a cap) - AI_MAX_OUTPUT_TOKENS 256 -> 1024 (room for a real assessment + suggestions + risks) - unified-comment: render the re-run checkbox OUTSIDE the alert blockquote — GitHub disables task-list checkboxes inside a blockquote, so clicking it never fired issue_comment.edited - sweep: keep the full re-review re-publish (affordable under the raised budget) so idle PRs refresh their comment/label/synthesis and dead-lettered reviews get a fresh verdict --- src/queue/processors.ts | 13 +++++++------ src/review/unified-comment.ts | 9 +++++++-- src/services/ai-review.ts | 2 +- test/unit/unified-comment.test.ts | 11 +++++++++-- wrangler.jsonc | 8 ++++++-- 5 files changed, 30 insertions(+), 13 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index db7b6e5973..d7eb957452 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -543,12 +543,13 @@ async function sweepRepoRegate(env: Env, repoFullName: string | undefined): Prom const gate = evaluateGateCheck(advisory, gateCheckPolicy(settings, null, undefined, pr.slopRisk ?? null)); verdicts[String(pr.number)] = gate.conclusion; if (gate.conclusion === "failure" || gate.conclusion === "action_required") flaggedPulls.push(pr.number); - // Backstop the CI-completion trigger AND refresh the stale public comment: fully RE-REVIEW each stale open - // PR (rebuild advisory → re-publish the unified comment with the current head/CI → re-run auto-maintain). The - // re-gate above only recomputes the audit verdict; without this re-publish, an idle PR keeps a stale comment - // (e.g. "safe to merge" from before a fix) and a stale status label forever. reReviewStoredPullRequest reads - // the freshly-synced head + the live CI, so the comment + label + action all reflect reality. Paced at - // SWEEP_MAX_PRS per sweep; cheap now that installation tokens are cached. + // Backstop the CI-completion trigger AND refresh the stale public comment: fully RE-REVIEW each stale open PR + // (rebuild dual-AI advisory → re-publish the unified comment with the current head/CI/synthesis → re-run + // auto-maintain). The re-gate above only recomputes the deterministic audit verdict; without this re-publish + // an idle PR keeps a stale comment ("safe to merge" from before a fix) and a stale label forever, and a + // previously dead-lettered review never gets a fresh verdict. This re-runs the dual-AI review every sweep — + // affordable on the enterprise Workers AI plan (the daily neuron budget is a runaway backstop, not a free-tier + // cap). Paced at SWEEP_MAX_PRS per sweep; installation tokens are cached so the GitHub cost stays bounded. if (sweepInstallationId != null) { await reReviewStoredPullRequest(env, `regate-sweep:${repoFullName}#${pr.number}`, sweepInstallationId, repoFullName, pr.number).catch((error) => { console.error(JSON.stringify({ level: "warn", event: "sweep_rereview_failed", deliveryId: `regate-sweep:${repoFullName}#${pr.number}`, repository: repoFullName, pullNumber: pr.number, error: errorMessage(error) })); diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index f258824f0a..152ec4385c 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -427,7 +427,6 @@ export function renderUnifiedReviewComment(input: UnifiedReviewInput, ctx: Unifi if (c.body.trim()) blocks.push(c.rawHtml ? detailsRaw(c.title, c.body.trim()) : details(c.title, c.body.trim())); } - if (ctx.reRunLabel) blocks.push(`- [ ] ${ctx.reRunLabel}`); // Color-coded status legend (key) — a quiet footer mapping each headline color/icon to its meaning, so a // reader can tell at a glance what "this PR's status" means. Squares are the SAME ones used in the headline. blocks.push( @@ -435,7 +434,13 @@ export function renderUnifiedReviewComment(input: UnifiedReviewInput, ctx: Unifi ); if (ctx.footerMarkdown?.trim()) blocks.push(`---\n${ctx.footerMarkdown.trim()}`); - return asAlert(meta.alert, blocks.join("\n\n")); + // The re-run checkbox MUST render at top level, OUTSIDE the alert blockquote. GitHub disables interactive + // task-list checkboxes inside a blockquote (every line `> `-prefixed by asAlert), so a checkbox emitted via + // asAlert can never be ticked — no issue_comment.edited fires and maybeProcessPrPanelRetrigger never runs. + // Appending it after the alert keeps the box clickable AND keeps the checked-marker regex matching a non- + // quoted `- [x] …` line. The PR_PANEL_COMMENT_MARKER prepended by the bridge still leads the body. + const alerted = asAlert(meta.alert, blocks.join("\n\n")); + return ctx.reRunLabel ? `${alerted}\n\n- [ ] ${ctx.reRunLabel}` : alerted; } /** diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index 1fdb8c7db3..f466fbbe77 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -400,7 +400,7 @@ export async function runGittensoryAiReview(env: Env, input: GittensoryAiReviewI // Estimate against the EFFECTIVE system prompt (`system`) so grounding's extra context is billed against the // budget. Flag-OFF, `system === REVIEW_SYSTEM_PROMPT`, so the estimate is byte-identical to today. const estimatedNeurons = freeAiCalls === 0 ? 0 : estimateNeurons(system.length + user.length, maxTokens, freeAiCalls); - const budget = clampNumber(Number(env.AI_DAILY_NEURON_BUDGET || 10000), 0, 1_000_000); + const budget = clampNumber(Number(env.AI_DAILY_NEURON_BUDGET || 10000), 0, 10_000_000); const used = await sumAiEstimatedNeuronsSince(env, utcDayStartIso()); const remainingBudget = Math.max(0, budget - used); if (estimatedNeurons > remainingBudget) { diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index ee08c00dd3..3839edb5f2 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -100,9 +100,16 @@ describe("renderUnifiedReviewComment", () => { expect(md).toContain("Checked by Gittensory."); }); - it("the entire comment is blockquote-wrapped (the full colored sidebar)", () => { + it("wraps the review body in the colored blockquote but renders the re-run checkbox OUTSIDE it (interactive)", () => { const md = renderUnifiedReviewComment({ ...base, decision: "merge" }, ctx); - expect(md.split("\n").every((l) => l.startsWith(">"))).toBe(true); + const lines = md.split("\n"); + const checkboxIdx = lines.findIndex((l) => l.includes("Re-run Gittensory review")); + expect(checkboxIdx).toBeGreaterThan(-1); + // GitHub disables task-list checkboxes inside a blockquote, so the re-run box must be at top level + // (otherwise it can never be ticked → no issue_comment.edited → the on-demand re-run never fires). + expect(lines[checkboxIdx].startsWith(">")).toBe(false); + // The colored sidebar — every non-empty line above the checkbox — IS blockquote-wrapped. + expect(lines.slice(0, checkboxIdx).filter((l) => l.length > 0).every((l) => l.startsWith(">"))).toBe(true); }); it("blocked state uses the caution alert, red bar, and an expanded blockers section", () => { diff --git a/wrangler.jsonc b/wrangler.jsonc index 0ca0bbf5a5..e756b139e8 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -41,9 +41,13 @@ "AI_SUMMARIES_ENABLED": "true", "AI_PUBLIC_COMMENTS_ENABLED": "true", "WORKERS_AI_SUMMARY_MODEL": "@cf/meta/llama-3.1-8b-instruct-fp8-fast", - "AI_DAILY_NEURON_BUDGET": "10000", + // Enterprise Workers AI plan: the daily neuron budget is a runaway-loop BACKSTOP, not a free-tier cap. + // The old "10000" starved EVERY dual-AI review (~573 neurons each) into quota_exceeded. Account headroom is + // ~hundreds of k/day; 2,000,000 never blocks normal review volume while still capping an infinite-loop bug. + "AI_DAILY_NEURON_BUDGET": "2000000", "AI_BYOK_DAILY_REPO_LIMIT": "25", - "AI_MAX_OUTPUT_TOKENS": "256", + // Enterprise: give the reviewer room for a real assessment + suggestions + risks (256 floored the synthesis). + "AI_MAX_OUTPUT_TOKENS": "1024", "AI_GATEWAY_ID": "", "ADMIN_GITHUB_LOGINS": "JSONbored", // Convergence (Stage D): render the public PR comment via the unified-comment bridge. Default OFF — From 452bac4aa78432f5218c822bc7ce13d05c970bbc Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 22 Jun 2026 20:16:05 -0700 Subject: [PATCH 4/5] =?UTF-8?q?fix(review):=20unbounded=20gate=20check-run?= =?UTF-8?q?=20title=20422'd=20=E2=86=92=20aborted=20the=20WHOLE=20review?= =?UTF-8?q?=20(no=20comment/audit/close)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit THE root cause of 'nothing works': formatGateCheckOutput returned output.title unbounded. GitHub 422s an over-long check-run title; the throw escaped createOrUpdateGateCheckRun (unwrapped) and the outer catch RE-THREW, so the review aborted before publishing the comment, writing the audit row, and running auto-maintain (merge/close). #1075 appended failing-check names to the title, so RED-CI PRs got long titles → 422 → they were never reviewed or closed — exactly the PRs that should auto-close. - advisory.ts: cap gate output.title at 255 (matches the annotation cap) — prevents the 422 - processors.ts: wrap createOrUpdateGateCheckRun in a local try/catch — a check-run API failure now finalizes the pending check to neutral and CONTINUES (records github_app.gate_check_failed_nonfatal) instead of dead-lettering the whole review - tests updated for the non-fatal-continue behavior --- src/queue/processors.ts | 56 +++++++++++++++++++++---------- src/rules/advisory.ts | 7 ++-- test/unit/queue.test.ts | 7 ++-- test/unit/unified-comment.test.ts | 9 ++--- 4 files changed, 52 insertions(+), 27 deletions(-) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index d7eb957452..5b18497530 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -2009,28 +2009,48 @@ async function maybePublishPrPublicSurface( await recordNativeGateDecision(env, { project: repoFullName, pullNumber: pr.number, headSha: pr.headSha, conclusion: gateEvaluation.conclusion, reasonCode }); } if (gateEnabled) { - const gateCheckResult = await createOrUpdateGateCheckRun( - env, - installationId, - repoFullName, - advisory, - gatePolicy, - { - checkRunId: pendingGateCheckRunId, - }, - ); - if (gateCheckResult?.kind === "published") gateFinalized = true; - if (gateCheckResult?.kind === "permission_missing") { - await auditGateCheckPermissionMissing(env, author, repoFullName, pr.number, webhook.deliveryId, gateCheckResult.warning); - // A 403 on the COMPLETION call is classified as permission_missing and does NOT throw, so the catch - // below never runs and the pending in_progress check would be orphaned. But the pending check already - // posted (pendingGateCheckRunId is set), proving the App had Checks:write — so a 403 here is almost - // always a transient secondary-rate-limit, not a real revocation. Finalize the pending check to - // neutral (mirrors the catch); if it were a genuine revocation this PATCH also 403s and is swallowed. + try { + const gateCheckResult = await createOrUpdateGateCheckRun( + env, + installationId, + repoFullName, + advisory, + gatePolicy, + { + checkRunId: pendingGateCheckRunId, + }, + ); + if (gateCheckResult?.kind === "published") gateFinalized = true; + if (gateCheckResult?.kind === "permission_missing") { + await auditGateCheckPermissionMissing(env, author, repoFullName, pr.number, webhook.deliveryId, gateCheckResult.warning); + // A 403 on the COMPLETION call is classified as permission_missing and does NOT throw, so the catch + // below never runs and the pending in_progress check would be orphaned. But the pending check already + // posted (pendingGateCheckRunId is set), proving the App had Checks:write — so a 403 here is almost + // always a transient secondary-rate-limit, not a real revocation. Finalize the pending check to + // neutral (mirrors the catch); if it were a genuine revocation this PATCH also 403s and is swallowed. + if (pendingGateCheckRunId !== undefined && !gateFinalized) { + await createOrUpdateErroredGateCheckRun(env, installationId, repoFullName, advisory, { checkRunId: pendingGateCheckRunId }).catch(() => undefined); + gateFinalized = true; + } + } + } catch (checkError) { + // CRITICAL: a check-run API failure (e.g. a 422 from an over-long output.title) must NEVER abort the + // review. The outer catch re-throws → the comment, the audit row, and the auto-action (merge/close) + // would all be skipped and the review dead-lettered. That is exactly why red-CI PRs (whose gate title + // grew long with failing-check names) were silently never reviewed or closed. Finalize the pending + // check to a neutral terminal state so it doesn't hang, log, and CONTINUE — do not re-throw. if (pendingGateCheckRunId !== undefined && !gateFinalized) { await createOrUpdateErroredGateCheckRun(env, installationId, repoFullName, advisory, { checkRunId: pendingGateCheckRunId }).catch(() => undefined); gateFinalized = true; } + await recordAuditEvent(env, { + eventType: "github_app.gate_check_failed_nonfatal", + actor: author, + targetKey: `${repoFullName}#${pr.number}`, + outcome: "error", + detail: errorMessage(checkError), + metadata: { deliveryId: webhook.deliveryId, repoFullName }, + }).catch(() => undefined); } } } catch (error) { diff --git a/src/rules/advisory.ts b/src/rules/advisory.ts index f9f73d91f4..86023f830c 100644 --- a/src/rules/advisory.ts +++ b/src/rules/advisory.ts @@ -414,7 +414,7 @@ export function formatGateCheckOutput(gate: GateCheckEvaluation): { title: strin } if (gate.conclusion === "neutral" || gate.conclusion === "skipped") { return { - title: gate.title, + title: gate.title.slice(0, 255), summary: gate.summary, text: "Gittensory did not create a contributor-facing failure for this event.", }; @@ -424,7 +424,10 @@ export function formatGateCheckOutput(gate: GateCheckEvaluation): { title: strin return `- ${sanitizeForCheckRun(finding.title)}.${action}`; }); return { - title: gate.title, + // GitHub's check-run output.title 422s when too long; cap it (matches the 255 cap used for annotations). + // An unbounded title (e.g. when failing-check names are appended) threw a 422 that aborted the ENTIRE + // review before the comment, audit, and auto-action — so red-CI PRs were never reviewed or closed. + title: gate.title.slice(0, 255), summary: "Gittensory Gate found a repo-configured hard blocker.", text: blockerLines.length > 0 ? blockerLines.join("\n") : "A configured hard blocker was found.", }; diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index efd85b854c..98b665f5f9 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1592,15 +1592,16 @@ describe("queue processors", () => { }, }); - // The completion PATCH failed (500), so the catch finalized the SAME check run (id 970) to a neutral, - // non-blocking terminal state — never left hanging in_progress. + // The completion PATCH failed (500), so the LOCAL check-run catch finalized the SAME check run (id 970) to + // a neutral, non-blocking terminal state — never left hanging in_progress — and CONTINUED the review + // (no re-throw), so the comment/audit/auto-action still run instead of the whole review dead-lettering. expect(patchBodies.length).toBe(2); const finalize = patchBodies[1]; expect(finalize?.status).toBe("completed"); expect(finalize?.conclusion).toBe("neutral"); expect(finalize?.output?.title).toBe("Gittensory Gate — could not finish evaluating"); const audit = await env.DB.prepare("select outcome from audit_events where event_type = ? and target_key = ?") - .bind("github_app.gate_finalized_on_error", "JSONbored/gittensory#80") + .bind("github_app.gate_check_failed_nonfatal", "JSONbored/gittensory#80") .first<{ outcome: string }>(); expect(audit?.outcome).toBe("error"); }); diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index 3839edb5f2..398eed93c2 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -103,13 +103,14 @@ describe("renderUnifiedReviewComment", () => { it("wraps the review body in the colored blockquote but renders the re-run checkbox OUTSIDE it (interactive)", () => { const md = renderUnifiedReviewComment({ ...base, decision: "merge" }, ctx); const lines = md.split("\n"); - const checkboxIdx = lines.findIndex((l) => l.includes("Re-run Gittensory review")); - expect(checkboxIdx).toBeGreaterThan(-1); + const checkboxLine = lines.find((l) => l.includes("Re-run Gittensory review")); + expect(checkboxLine).toBeDefined(); // GitHub disables task-list checkboxes inside a blockquote, so the re-run box must be at top level // (otherwise it can never be ticked → no issue_comment.edited → the on-demand re-run never fires). - expect(lines[checkboxIdx].startsWith(">")).toBe(false); + expect(checkboxLine!.startsWith(">")).toBe(false); // The colored sidebar — every non-empty line above the checkbox — IS blockquote-wrapped. - expect(lines.slice(0, checkboxIdx).filter((l) => l.length > 0).every((l) => l.startsWith(">"))).toBe(true); + const bodyAbove = lines.slice(0, lines.indexOf(checkboxLine!)).filter((l) => l.length > 0); + expect(bodyAbove.every((l) => l.startsWith(">"))).toBe(true); }); it("blocked state uses the caution alert, red bar, and an expanded blockers section", () => { From ca4b7c04ea5a4752a2c98988ef886e9e2f04df5d Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 22 Jun 2026 23:52:10 -0700 Subject: [PATCH 5/5] fix(review): production reliability pass (budget/loop/merge/notify) - ai-review: neuron budget defaults to the safe max when unset/empty/NaN. The deployed worker read the 10k free-tier default off main and starved every dual-AI review into quota_exceeded; an explicit value (incl. "0" to disable) still wins. - review loop: refresh reviewDecision live so the action planner does not re-post a review every cycle. - CI gate: only branch-protection REQUIRED red checks block; non-required reds are surfaced, not blocking. - merges: classify terminal merge failures (perm/conflict/required-absent) and stop retrying them every sweep; transient failures retry to a cap (+ migration 0052). - safety: dial back the secret_leak heuristic so config/workflow PRs are not false-blocked. - stats: read the live audit_events overlay for homepage counters. - notify: per-repo Discord embeds on merge/close/manual. - rebase: update-branch before review when behind base. --- migrations/0052_pr_merge_attempt_terminal.sql | 21 +++++ src/db/repositories.ts | 33 +++++++ src/db/schema.ts | 6 ++ src/github/backfill.ts | 83 +++++++++++++++-- src/github/pr-actions.ts | 23 +++++ src/index.ts | 19 ++-- src/queue/processors.ts | 57 +++++++++--- src/review/public-stats.ts | 88 +++++++++++++++++-- src/review/safety.ts | 28 ++++-- src/services/agent-action-executor.ts | 53 ++++++++++- src/services/ai-review.ts | 8 +- src/services/merge-failure.ts | 43 +++++++++ src/services/notify-discord.ts | 73 +++++++++++++++ src/settings/agent-actions.ts | 10 ++- src/settings/agent-sweep.ts | 7 +- src/types.ts | 6 ++ test/unit/agent-sweep.test.ts | 8 +- test/unit/index.test.ts | 16 ++-- test/unit/public-stats.test.ts | 9 ++ wrangler.jsonc | 2 +- 20 files changed, 532 insertions(+), 61 deletions(-) create mode 100644 migrations/0052_pr_merge_attempt_terminal.sql create mode 100644 src/services/merge-failure.ts create mode 100644 src/services/notify-discord.ts diff --git a/migrations/0052_pr_merge_attempt_terminal.sql b/migrations/0052_pr_merge_attempt_terminal.sql new file mode 100644 index 0000000000..86dd7db581 --- /dev/null +++ b/migrations/0052_pr_merge_attempt_terminal.sql @@ -0,0 +1,21 @@ +-- RC3 (terminal-fail merges): stop the auto-maintain merge retry-forever loop. +-- +-- BEFORE: executeAgentMaintenanceActions() calls mergePullRequest(); a 403 (Resource not accessible) / 405 +-- (method not allowed) / 409 (required check absent) / conflict throws, the action is recorded as an `error` +-- audit row, but the pull_requests row stays plannable — so EVERY webhook + every scheduled re-gate sweep +-- re-plans the same merge and it fails again, with no cap and no backoff. (reviewbot parity: review_targets' +-- attempt_count + terminal_at, which gittensory's normalized planner path never had.) +-- +-- AFTER: a non-transient merge failure marks the PR terminally merge-blocked FOR THE CURRENT HEAD SHA. The +-- planner skips planning a merge while merge_blocked_sha == headSha, and the executor caps retries via +-- merge_attempt_count so even a misclassified transient failure escalates to a human instead of looping. +-- +-- merge_blocked_sha is keyed to the head SHA so a NEW commit (which upsertPullRequestFromGitHub writes) clears +-- the block automatically — a pushed fix gets a fresh merge attempt without any manual reset. +-- +-- pull_requests IS a Drizzle table (src/db/schema.ts), so these columns are added to the Drizzle schema too; +-- this raw migration is the production DDL applied by `wrangler d1 migrations apply` (drizzle-kit is not the +-- runtime migrator here). +ALTER TABLE pull_requests ADD COLUMN merge_attempt_count INTEGER NOT NULL DEFAULT 0; +ALTER TABLE pull_requests ADD COLUMN merge_blocked_sha TEXT; +ALTER TABLE pull_requests ADD COLUMN merge_blocked_reason TEXT; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index bde87726c3..b92ed9c063 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -2495,6 +2495,36 @@ export async function getPullRequest(env: Env, fullName: string, number: number) return row ? toPullRequestRecordFromRow(row) : null; } +// RC3 terminal-fail merges. The auto-maintain executor calls these when a merge mutation fails so the planner +// stops planning a merge it can never complete (403/405/409/conflict), instead of retrying every sweep forever. + +/** Increment the failed-merge attempt counter for a PR, scoped to the head SHA that failed. Returns the new + * count. Scoping to headSha means a new commit's attempts start fresh once the row's head advances. */ +export async function bumpPullRequestMergeAttempt(env: Env, fullName: string, number: number, headSha: string): Promise { + const db = getDb(env.DB); + await db + .update(pullRequests) + .set({ mergeAttemptCount: sql`${pullRequests.mergeAttemptCount} + 1`, updatedAt: nowIso() }) + .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha))); + const [row] = await db + .select({ count: pullRequests.mergeAttemptCount }) + .from(pullRequests) + .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number))) + .limit(1); + return Number(row?.count ?? 0); +} + +/** Mark a PR terminally merge-blocked for its current head SHA: the planner skips the `merge` disposition while + * merge_blocked_sha == headSha. Scoped to headSha so a later commit (a pushed fix) auto-clears the block (the + * guard compares it to the live head). Records the human-readable terminal reason. */ +export async function markPullRequestMergeBlocked(env: Env, fullName: string, number: number, headSha: string, reason: string): Promise { + const db = getDb(env.DB); + await db + .update(pullRequests) + .set({ mergeBlockedSha: headSha, mergeBlockedReason: reason.slice(0, 280), updatedAt: nowIso() }) + .where(and(eq(pullRequests.repoFullName, fullName), eq(pullRequests.number, number), eq(pullRequests.headSha, headSha))); +} + export async function getIssue(env: Env, fullName: string, number: number): Promise { const db = getDb(env.DB); const [row] = await db.select().from(issues).where(and(eq(issues.repoFullName, fullName), eq(issues.number, number))).limit(1); @@ -3898,6 +3928,9 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull linkedIssues: parseJson(row.linkedIssuesJson, []), slopRisk: row.slopRisk, slopBand: row.slopBand, + mergeAttemptCount: row.mergeAttemptCount, + mergeBlockedSha: row.mergeBlockedSha, + mergeBlockedReason: row.mergeBlockedReason, }; } diff --git a/src/db/schema.ts b/src/db/schema.ts index 7296a6c595..8630cd327f 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -276,6 +276,12 @@ export const pullRequests = sqliteTable( // Latest deterministic slop assessment (gittensory-computed; written separately from the GitHub sync). slopRisk: integer("slop_risk"), slopBand: text("slop_band"), + // RC3 terminal-fail merges: failed-merge attempt count + the head SHA at which the merge is terminally + // blocked (perms/required-check/conflict) so the planner stops planning a merge. Keyed to head SHA → a new + // commit auto-clears it. gittensory-computed (executor-written), omitted from the GitHub-sync SET clause. + mergeAttemptCount: integer("merge_attempt_count").notNull().default(0), + mergeBlockedSha: text("merge_blocked_sha"), + mergeBlockedReason: text("merge_blocked_reason"), createdAt: text("created_at").notNull().$defaultFn(() => nowIso()), updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }, diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 24d29a128e..2935973466 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -1910,9 +1910,42 @@ const CI_PASSING_CONCLUSIONS = new Set(["success", "neutral", "skipped"]); export type LiveCiAggregate = { ciState: "passed" | "failed" | "pending" | "unverified"; + // Checks that FAIL the gate: every failing check when required contexts are unknown, else only the failing + // REQUIRED contexts. These drive ciState === "failed" and the disposition (no-merge / close / request-changes). failingDetails: Array<{ name: string; summary?: string; detailsUrl?: string }>; + // RC2: checks that are RED but NOT in branch-protection's required set (e.g. codecov/patch, codecov/project). + // Surfaced to the contributor but they do NOT fail the gate, block merge/approve, or force request_changes. + // Empty when required contexts are unknown (best-effort fetch failed / no protection) — then every red check + // stays in failingDetails (byte-identical to pre-RC2). + nonRequiredFailingDetails: Array<{ name: string; summary?: string; detailsUrl?: string }>; }; +/** + * RC2 best-effort fetch of the base branch's branch-protection REQUIRED status-check contexts. Returns the set + * of required context names (covering both the legacy `contexts` array and the newer `checks[].context` shape), + * or `null` when none can be determined — a 404 (no protection / no required checks), a 403 (token lacks + * admin:repo, common for installations/forks), or any other error. `null`/empty makes fetchLiveCiAggregate fall + * back to folding ALL red checks into the gate, so a fetch failure can never silently pass a required red check. + */ +export async function fetchRequiredStatusContexts(env: Env, repoFullName: string, baseRef: string | null | undefined, token: string | undefined): Promise | null> { + if (!baseRef) return null; + const result = await githubJsonWithHeaders<{ contexts?: Array | null; checks?: Array<{ context?: string | null }> | null }>( + env, + repoFullName, + `/branches/${encodeURIComponent(baseRef)}/protection/required_status_checks`, + token, + ).catch(() => undefined); + if (!result) return null; // 404 (no protection) / 403 (no admin) — treat as "unknown". + const names = new Set(); + for (const ctx of result.data.contexts ?? []) { + if (typeof ctx === "string" && ctx.trim().length > 0) names.add(ctx); + } + for (const check of result.data.checks ?? []) { + if (typeof check?.context === "string" && check.context.trim().length > 0) names.add(check.context); + } + return names; +} + /** * Fetch the head SHA's LIVE CI aggregate over BOTH GitHub Check-runs AND classic commit-statuses. This is the * reviewbot `getAllChecksState` parity that the converged auto-maintain path needs: codecov (codecov/patch, @@ -1922,9 +1955,22 @@ export type LiveCiAggregate = { * present → "passed"; none at all → "unverified". The disposition layer NEVER approves/merges unless "passed", * and closes (non-owner) / holds (owner) on "failed". Best-effort: a fetch error degrades that source to empty. */ -export async function fetchLiveCiAggregate(env: Env, repoFullName: string, headSha: string | null | undefined, token: string | undefined): Promise { - if (!headSha) return { ciState: "unverified", failingDetails: [] }; +export async function fetchLiveCiAggregate( + env: Env, + repoFullName: string, + headSha: string | null | undefined, + token: string | undefined, + // RC2: when a NON-EMPTY set, only these branch-protection-required contexts gate the PR — a red check outside + // the set is surfaced (nonRequiredFailingDetails) but never fails the gate. null/empty ⇒ fold ALL red checks + // (pre-RC2 behavior), the safe fallback when protection can't be read. + requiredContexts?: ReadonlySet | null, +): Promise { + if (!headSha) return { ciState: "unverified", failingDetails: [], nonRequiredFailingDetails: [] }; + // Only enforce a required subset when we actually resolved one; otherwise every red check is gate-failing. + const enforceRequiredOnly = requiredContexts != null && requiredContexts.size > 0; + const isRequired = (name: string): boolean => !enforceRequiredOnly || requiredContexts!.has(name); const failingDetails: LiveCiAggregate["failingDetails"] = []; + const nonRequiredFailingDetails: LiveCiAggregate["nonRequiredFailingDetails"] = []; let total = 0; let anyPending = false; @@ -1943,11 +1989,12 @@ export async function fetchLiveCiAggregate(env: Env, repoFullName: string, headS const status = (run.status ?? "").toLowerCase(); if (conclusion ? CI_FAILING_CONCLUSIONS.has(conclusion) : false) { const summary = [run.output?.title, run.output?.summary].find((value): value is string => typeof value === "string" && value.trim().length > 0)?.trim().slice(0, 200); - failingDetails.push({ name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) }); + const detail = { name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) }; + (isRequired(run.name) ? failingDetails : nonRequiredFailingDetails).push(detail); } else if (conclusion ? CI_PASSING_CONCLUSIONS.has(conclusion) : status === "completed") { // concluded and not failing → passing - } else { - anyPending = true; // queued / in_progress / not yet concluded + } else if (isRequired(run.name)) { + anyPending = true; // queued / in_progress / not yet concluded — only a REQUIRED check holds the gate } } if (!hasNextPage(result.link)) break; @@ -1968,16 +2015,20 @@ export async function fetchLiveCiAggregate(env: Env, repoFullName: string, headS const name = ctx.context ?? "status"; if (state === "failure" || state === "error") { const summary = typeof ctx.description === "string" ? ctx.description.trim().slice(0, 200) : ""; - failingDetails.push({ name, ...(summary ? { summary } : {}), ...(ctx.target_url ? { detailsUrl: ctx.target_url } : {}) }); + const detail = { name, ...(summary ? { summary } : {}), ...(ctx.target_url ? { detailsUrl: ctx.target_url } : {}) }; + (isRequired(name) ? failingDetails : nonRequiredFailingDetails).push(detail); } else if (state === "success") { // passing - } else { - anyPending = true; // pending + } else if (isRequired(name)) { + anyPending = true; // pending — only a REQUIRED context holds the gate } } + // ciState reflects ONLY gate-failing (required, or all-when-unknown) checks. A repo whose only red check is a + // non-required codecov/* therefore reports "passed" and is eligible to merge/approve, with the codecov + // failure riding along in nonRequiredFailingDetails for the contributor to see. const ciState: LiveCiAggregate["ciState"] = failingDetails.length > 0 ? "failed" : anyPending ? "pending" : total > 0 ? "passed" : "unverified"; - return { ciState, failingDetails }; + return { ciState, failingDetails, nonRequiredFailingDetails }; } /** @@ -1993,6 +2044,20 @@ export async function fetchLivePullRequestMergeState(env: Env, repoFullName: str return result?.data.mergeable_state ?? undefined; } +/** RC1 (idempotent reviews): the PR's LIVE reviewDecision (APPROVED / CHANGES_REQUESTED / REVIEW_REQUIRED) via + * GraphQL. The STORED reviewDecision is only written by the open-PR backfill and goes stale, so the action + * planner's approve/request-changes dedup was blind and re-posted a review every cycle — the re-review loop. + * Refreshing it live makes the dedup accurate. Best-effort: returns undefined on any error (caller falls back + * to the stored value). */ +export async function fetchLivePullRequestReviewDecision(env: Env, repoFullName: string, prNumber: number, token: string | undefined): Promise { + if (!token) return undefined; + const [owner, name] = repoFullName.split("/"); + if (!owner || !name) return undefined; + const query = `query { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { pullRequest(number: ${prNumber}) { reviewDecision } } }`; + const result = await githubGraphQl<{ data?: { repository?: { pullRequest?: { reviewDecision?: string | null } | null } | null } }>(env, query, token).catch(() => undefined); + return result?.data?.repository?.pullRequest?.reviewDecision ?? undefined; +} + async function fetchPullRequestDetailsFromGraphQl( env: Env, repoFullName: string, diff --git a/src/github/pr-actions.ts b/src/github/pr-actions.ts index 2fe64a8031..cea01a86ff 100644 --- a/src/github/pr-actions.ts +++ b/src/github/pr-actions.ts @@ -60,6 +60,29 @@ export async function mergePullRequest( return { merged: data.merged ?? true, sha: data.sha ?? null }; } +/** Rebase a PR onto its base via GitHub's update-branch (merges the current base into the PR head). Keeps a + * BEHIND PR current before reviewing/merging so the review + required CI run against the merged result — + * reviewbot parity. `expectedHeadSha` guards against racing a head that moved since we read it. The PUT + * returns 202 (update queued) on success; a caller treats any throw as best-effort (e.g. 422 when already + * up to date or the branch is dirty/conflicting — those are handled by the gate, not retried here). */ +export async function updatePullRequestBranch( + env: Env, + installationId: number, + repoFullName: string, + pullNumber: number, + expectedHeadSha?: string | undefined, +): Promise { + const { owner, repo } = splitRepo(repoFullName); + const token = await createInstallationToken(env, installationId); + const octokit = new Octokit({ auth: token }); + await octokit.request("PUT /repos/{owner}/{repo}/pulls/{pull_number}/update-branch", { + owner, + repo, + pull_number: pullNumber, + ...(expectedHeadSha ? { expected_head_sha: expectedHeadSha } : {}), + }); +} + /** Post a plain issue/PR comment (used for the templated close message before closing). */ export async function createIssueComment(env: Env, installationId: number, repoFullName: string, issueNumber: number, body: string): Promise<{ id: number }> { const { owner, repo } = splitRepo(repoFullName); diff --git a/src/index.ts b/src/index.ts index 5264dfeb09..a213a9a083 100644 --- a/src/index.ts +++ b/src/index.ts @@ -42,19 +42,22 @@ async function enqueueScheduledJobs(env: Env, controller: ScheduledController): const hour = scheduledAt.getUTCHours(); const isHourly = minute === 0; const isFullSyncWindow = isHourly && hour % 6 === 0; - const jobs: JobMessage[] = [ - { type: "backfill-registered-repos", requestedBy: "schedule", mode: isFullSyncWindow ? "full" : "light" }, - { type: "repair-data-fidelity", requestedBy: "schedule" }, - { type: "refresh-installation-health", requestedBy: "schedule" }, - ]; + // The light auto-maintain sweep runs EVERY cron tick (~every 2 min) so an approved+clean PR MERGES and a + // red-CI non-owner PR CLOSES promptly — reviewbot parity (its cron fired every minute). It re-fetches LIVE CI + + // mergeable and only ACTS (merge/close/hold); it never re-runs the AI, so it is cheap enough for this cadence. + // Previously this was gated by `isHourly`, so an approved PR could wait ~an hour for its merge pass. + const jobs: JobMessage[] = [{ type: "agent-regate-sweep", requestedBy: "schedule" }]; + // The heavier sync/health jobs keep their ~30-minute cadence even though the cron now ticks every ~2 minutes. + if (minute % 30 === 0) { + jobs.push({ type: "backfill-registered-repos", requestedBy: "schedule", mode: isFullSyncWindow ? "full" : "light" }); + jobs.push({ type: "repair-data-fidelity", requestedBy: "schedule" }); + jobs.push({ type: "refresh-installation-health", requestedBy: "schedule" }); + } if (isHourly) { jobs.push({ type: "refresh-registry", requestedBy: "schedule" }); jobs.push({ type: "refresh-scoring-model", requestedBy: "schedule" }); jobs.push({ type: "refresh-upstream-drift", requestedBy: "schedule" }); jobs.push({ type: "rollup-product-usage", requestedBy: "schedule", days: 7 }); - // Agent layer (#777): re-gate stale open PRs hourly. Fans out to one job per agent-configured repo; - // webhooks don't fire when a PR's base advances, so this is what keeps those verdicts fresh. - jobs.push({ type: "agent-regate-sweep", requestedBy: "schedule" }); // Convergence (ops / observability, flag GITTENSORY_REVIEW_OPS). Hourly anomaly scan over gittensory's own // review-outcome data. Enqueued ONLY when the flag is ON — flag-OFF (default) this job is never created, // so the cron tick does ZERO new work and the enqueued set is byte-identical to today. diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 5b18497530..e89d21bb00 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -66,6 +66,8 @@ import { fetchAndStorePullRequestFilesForReview, fetchLiveCiAggregate, fetchLivePullRequestMergeState, + fetchLivePullRequestReviewDecision, + fetchRequiredStatusContexts, refreshContributorActivity, refreshInstallationHealth, refreshPullRequestDetails, @@ -86,6 +88,7 @@ import { sanitizePublicComment, } from "../github/commands"; import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels"; +import { updatePullRequestBranch } from "../github/pr-actions"; import { ALL_TYPE_LABELS, resolvePrTypeLabel } from "../settings/pr-type-label"; import { fetchPublicContributorProfile } from "../github/public"; import { refreshRegistry } from "../registry/sync"; @@ -543,13 +546,12 @@ async function sweepRepoRegate(env: Env, repoFullName: string | undefined): Prom const gate = evaluateGateCheck(advisory, gateCheckPolicy(settings, null, undefined, pr.slopRisk ?? null)); verdicts[String(pr.number)] = gate.conclusion; if (gate.conclusion === "failure" || gate.conclusion === "action_required") flaggedPulls.push(pr.number); - // Backstop the CI-completion trigger AND refresh the stale public comment: fully RE-REVIEW each stale open PR - // (rebuild dual-AI advisory → re-publish the unified comment with the current head/CI/synthesis → re-run - // auto-maintain). The re-gate above only recomputes the deterministic audit verdict; without this re-publish - // an idle PR keeps a stale comment ("safe to merge" from before a fix) and a stale label forever, and a - // previously dead-lettered review never gets a fresh verdict. This re-runs the dual-AI review every sweep — - // affordable on the enterprise Workers AI plan (the daily neuron budget is a runaway backstop, not a free-tier - // cap). Paced at SWEEP_MAX_PRS per sweep; installation tokens are cached so the GitHub cost stays bounded. + // FULL fresh review of every open PR (the operator's requirement: gittensory must post its OWN review before + // acting — the old comments are reviewbot's, pre-consolidation). reReviewStoredPullRequest re-runs the dual-AI, + // re-publishes gittensory's unified comment with the current head/CI/synthesis, then auto-maintains (merge a + // clean+approved PR, close a red-CI non-owner PR, hold the owner's). The per-commit decision cache + // (cacheReviewDecision / replay in maybePublishPrPublicSurface) makes the repeat passes cheap — the dual-AI + // only re-runs when the head SHA changed, so this is affordable on a tight cron without re-AIing every sweep. if (sweepInstallationId != null) { await reReviewStoredPullRequest(env, `regate-sweep:${repoFullName}#${pr.number}`, sweepInstallationId, repoFullName, pr.number).catch((error) => { console.error(JSON.stringify({ level: "warn", event: "sweep_rereview_failed", deliveryId: `regate-sweep:${repoFullName}#${pr.number}`, repository: repoFullName, pullNumber: pr.number, error: errorMessage(error) })); @@ -606,14 +608,22 @@ async function maybeRunAgentMaintenance( // planner uses this to NEVER approve/merge a PR whose CI isn't green, to CLOSE a red-CI non-owner PR (citing // the failing checks) / HOLD the owner's, and to DEFER entirely while CI is still pending. const ciToken = await createInstallationToken(env, installationId).catch(() => undefined); - const [changedFiles, hardGuardrailGlobs, ciAggregate, liveMergeState] = await Promise.all([ + const [changedFiles, hardGuardrailGlobs, requiredContexts, liveMergeState, liveReviewDecision] = await Promise.all([ resolvePullRequestFilesForReview(env, { installationId, repoFullName, pullNumber: pr.number }), loadHardGuardrailGlobs(env, repoFullName), - fetchLiveCiAggregate(env, repoFullName, pr.headSha, ciToken ?? env.GITHUB_PUBLIC_TOKEN), + // RC2: branch-protection REQUIRED status contexts, so only a required red check gates the PR (a red + // codecov/* is surfaced but never blocks merge/approve or forces request_changes). null ⇒ fold all red. + fetchRequiredStatusContexts(env, repoFullName, pr.baseRef ?? args.repo?.defaultBranch, ciToken ?? env.GITHUB_PUBLIC_TOKEN), // Live mergeable_state — the stored one lags GitHub's async recompute after the bot's own approve, which // otherwise leaves a green+approved PR stuck OPEN at mergeState=CLEAN (never auto-merged). fetchLivePullRequestMergeState(env, repoFullName, pr.number, ciToken ?? env.GITHUB_PUBLIC_TOKEN), + // RC1: live reviewDecision so the approve/request-changes dedup is accurate. The STORED reviewDecision is + // only written by the open-PR backfill and goes stale → the planner re-posted a review every cycle (the + // re-review loop with 14-23 stacked reviews). With the live value, an already-approved/changes-requested PR + // is not re-reviewed for the same state. + fetchLivePullRequestReviewDecision(env, repoFullName, pr.number, ciToken ?? env.GITHUB_PUBLIC_TOKEN), ]); + const ciAggregate = await fetchLiveCiAggregate(env, repoFullName, pr.headSha, ciToken ?? env.GITHUB_PUBLIC_TOKEN, requiredContexts); const changedPaths = changedFiles.map((file) => file.path).filter((path) => path.length > 0); const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")) : ""; const authorLogin = pr.authorLogin ?? ""; @@ -634,10 +644,12 @@ async function maybeRunAgentMaintenance( failingCheckNames: ciAggregate.failingDetails.map((detail) => detail.name), pr: { mergeableState: liveMergeState ?? pr.mergeableState, - reviewDecision: pr.reviewDecision, + reviewDecision: liveReviewDecision ?? pr.reviewDecision, slopRisk: pr.slopRisk, labels: pr.labels, linkedDuplicateCount: linkedIssueDuplicatePullRequestsForGate(pr, otherOpenPullRequests).length, + headSha: pr.headSha, + mergeBlockedSha: pr.mergeBlockedSha, }, }); if (planned.length === 0) return; @@ -656,6 +668,7 @@ async function maybeRunAgentMaintenance( agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, installationPermissions, + authorLogin: pr.authorLogin, }, planned, ); @@ -671,6 +684,26 @@ async function reReviewStoredPullRequest(env: Env, deliveryId: string, installat const [repo, settings] = await Promise.all([getRepository(env, repoFullName), resolveRepositorySettings(env, repoFullName)]); const pr = await getPullRequest(env, repoFullName, prNumber); if (!pr || pr.state !== "open") return; + // Rebase-if-behind BEFORE reviewing (reviewbot parity): if the branch is BEHIND base, issue update-branch so + // the fresh review + required CI run against the merged result. The rebase creates a new head → a synchronize + // webhook (or the next sweep) re-reviews it; skip the rest of THIS pass so we never review/act on the stale + // head. Best-effort: a dirty/conflicting branch (422) is left for the gate to close, not rebased here. + if (isAgentConfigured(settings.autonomy) && !pr.isDraft && pr.headSha) { + const rebaseToken = await createInstallationToken(env, installationId).catch(() => undefined); + const liveMergeState = rebaseToken ? await fetchLivePullRequestMergeState(env, repoFullName, prNumber, rebaseToken) : undefined; + if (liveMergeState === "behind") { + const rebased = await updatePullRequestBranch(env, installationId, repoFullName, prNumber, pr.headSha) + .then(() => true) + .catch((error) => { + console.log(JSON.stringify({ ev: "rebase_failed", repoFullName, pull: prNumber, message: errorMessage(error).slice(0, 120) })); + return false; + }); + if (rebased) { + await recordAuditEvent(env, { eventType: "github_app.pr_branch_updated", actor: "gittensory", targetKey: `${repoFullName}#${prNumber}`, outcome: "completed", detail: "behind base; update-branch issued before review", metadata: { deliveryId, repoFullName } }).catch(() => undefined); + return; // the rebase fires a synchronize → fresh review runs on the new head + } + } + } const otherOpenPullRequests = await listOtherOpenPullRequests(env, repoFullName, prNumber); const advisory = buildPullRequestAdvisory(repo, pr, { otherOpenPullRequests, requireLinkedIssue: shouldCollectLinkedIssueEvidence(settings) }); await persistAdvisory(env, advisory); @@ -2156,7 +2189,9 @@ async function maybePublishPrPublicSurface( // auto-maintain planner uses so the public chip and the disposition can never disagree. "pending" folds to // the "unverified" bucket for the 3-state comment chip (renders "CI pending"). const ciToken = await createInstallationToken(env, installationId).catch(() => undefined); - const liveCi = await fetchLiveCiAggregate(env, repoFullName, pr.headSha, ciToken ?? env.GITHUB_PUBLIC_TOKEN); + // RC2: only branch-protection-required checks gate the PR; a red codecov/* is surfaced but never blocks. + const requiredContexts = await fetchRequiredStatusContexts(env, repoFullName, pr.baseRef ?? repo?.defaultBranch, ciToken ?? env.GITHUB_PUBLIC_TOKEN); + const liveCi = await fetchLiveCiAggregate(env, repoFullName, pr.headSha, ciToken ?? env.GITHUB_PUBLIC_TOKEN, requiredContexts); const ciState: MergeReadiness["ciState"] = liveCi.ciState === "passed" ? "passed" : liveCi.ciState === "failed" ? "failed" : "unverified"; // Per-failed-check WHY (codecov %/test/lint reason) from each check-run output or commit-status // description — capped + public-safe (name + short reason only). The renderer lists these under the CI chip. diff --git a/src/review/public-stats.ts b/src/review/public-stats.ts index d1df0b65bb..b2435a65cd 100644 --- a/src/review/public-stats.ts +++ b/src/review/public-stats.ts @@ -3,9 +3,13 @@ // off the public endpoint 404s, so the deploy is byte-identical to today until the flag is deliberately set. // // REALTIME: queries the live tables directly (no rollup/cron) so a new review shows up within the 60s HTTP cache -// window — single source of truth, always current. The source is review_targets (one row per PR, scoped to the -// repos the review system handles: gittensory, awesome-claude, metagraphed) with the terminal review DISPOSITION -// in `status`. This is NOT the broader pull_requests / recent_merged_pull_requests mining caches. +// window. review_targets is the HISTORICAL base (one row per PR, terminal DISPOSITION in `status`), but it was +// FROZEN at the agent cutover (gittensory[bot] became the sole reviewer/actor and stopped writing it) — so on +// its own it reads a dead table (RC8). The post-cutover live ledger is audit_events: a review lands as +// github_app.pr_public_surface_published, a merge as agent.action.merge, a close as agent.action.close. We +// therefore UNION the two: the frozen review_targets snapshot PLUS the audit_events that occurred AFTER the +// snapshot freeze point, de-duped by PR so nothing already terminal in the snapshot is double-counted. This is +// NOT the broader pull_requests / recent_merged_pull_requests mining caches. // // DISPOSITIONS (terminal): merged / closed = gittensory auto-actioned; commented = reviewed + advised, deferred // to a maintainer; manual = escalated to a maintainer; ignored = skipped (drafts/bots/excluded); error = failed. @@ -71,6 +75,47 @@ interface DispositionRow { error: number; } +/** Post-freeze live delta per project, derived from audit_events (the live ledger). One distinct PR per row + * per kind; reviewed/commented = pr_public_surface_published, merged = live agent.action.merge, closed = + * live agent.action.close. project == repoFullName == the target_key prefix before '#'. */ +interface LiveDeltaRow { + project: string; + reviewed: number; + merged: number; + closed: number; +} + +/** Add the live audit_events overlay onto the frozen review_targets base, de-duped by PR. The overlay only + * counts events strictly AFTER `frozenIso` (the snapshot freeze point) and only PRs whose target_key is NOT + * already present in review_targets, so historical PRs are never double-counted. A merge/close is counted + * only for the live agent path (metadata_json.mode = 'live'); dry-runs (which also record outcome + * 'completed') are excluded. SQLite: instr(target_key,'#') splits `${repoFullName}#${number}` back to the + * project (== repoFullName). */ +const LIVE_DELTA_SQL = ` + WITH frozen AS ( + SELECT DISTINCT project || '#' || number AS tkey FROM review_targets + ), + live AS ( + SELECT + substr(target_key, 1, instr(target_key, '#') - 1) AS project, + target_key AS tkey, + MAX(CASE WHEN event_type = 'github_app.pr_public_surface_published' AND outcome = 'completed' THEN 1 ELSE 0 END) AS reviewed, + MAX(CASE WHEN event_type = 'agent.action.merge' AND outcome = 'completed' AND json_extract(metadata_json, '$.mode') = 'live' THEN 1 ELSE 0 END) AS merged, + MAX(CASE WHEN event_type = 'agent.action.close' AND outcome = 'completed' AND json_extract(metadata_json, '$.mode') = 'live' THEN 1 ELSE 0 END) AS closed + FROM audit_events + WHERE created_at > ? + AND instr(target_key, '#') > 0 + AND event_type IN ('github_app.pr_public_surface_published', 'agent.action.merge', 'agent.action.close') + GROUP BY target_key + ) + SELECT project, + SUM(reviewed) AS reviewed, + SUM(merged) AS merged, + SUM(closed) AS closed + FROM live + WHERE tkey NOT IN (SELECT tkey FROM frozen) + GROUP BY project`; + export interface PublicStatsPayload { generatedAt: string; updatedAt: string; @@ -102,10 +147,14 @@ const DISPOSITION_SELECT = ` SUM(CASE WHEN status = 'manual' THEN 1 ELSE 0 END) AS manual, SUM(CASE WHEN status = 'error' THEN 1 ELSE 0 END) AS error`; -/** Assemble the public-safe payload from the LIVE review ledger (cheap: review_targets is one row per PR). */ +/** Assemble the public-safe payload from the frozen review_targets snapshot UNION the live audit_events overlay. */ export async function getPublicStats(env: Env, nowMs: number = Date.now()): Promise { const sinceIso = new Date(nowMs - 7 * 86_400_000).toISOString().slice(0, 19).replace("T", " "); - const [dispositions, reversalRows, weekly] = await Promise.all([ + // The snapshot freeze point: review_targets stopped advancing at the agent cutover. We overlay only the + // audit_events strictly newer than this so the homepage counters keep moving without double-counting history. + const frozenRows = await safeAll<{ frozenAt: string | null }>(env, `SELECT MAX(created_at) AS frozenAt FROM review_targets`); + const frozenIso = frozenRows[0]?.frozenAt ?? "0"; + const [dispositions, reversalRows, weekly, liveDeltas, weeklyLive] = await Promise.all([ safeAll(env, `SELECT project, COUNT(*) AS handled,${DISPOSITION_SELECT} FROM review_targets GROUP BY project`), safeAll<{ project: string; reversed: number }>( env, @@ -117,11 +166,30 @@ export async function getPublicStats(env: Env, nowMs: number = Date.now()): Prom `SELECT${DISPOSITION_SELECT.replace(/, $/, "")} FROM review_targets WHERE created_at >= ?`, sinceIso, ), + safeAll(env, LIVE_DELTA_SQL, frozenIso), + safeAll(env, LIVE_DELTA_SQL.replace("created_at > ?", "created_at > ? AND created_at >= ?"), frozenIso, new Date(nowMs - 7 * 86_400_000).toISOString()), ]); + // Fold the live overlay into the frozen disposition base, keyed by project (== repoFullName). A live review + // adds to `commented` (reviewed-but-not-yet-auto-actioned); a live merge/close adds to `merged`/`closed`. + const dispoByProject = new Map(dispositions.map((d) => [d.project, { ...d }])); + for (const delta of liveDeltas) { + const base = dispoByProject.get(delta.project) ?? { project: delta.project, handled: 0, merged: 0, closed: 0, commented: 0, ignored: 0, manual: 0, error: 0 }; + const dMerged = delta.merged ?? 0; + const dClosed = delta.closed ?? 0; + // A reviewed PR that was neither merged nor closed by the agent counts as commented (reviewed + advised). + const dCommented = Math.max((delta.reviewed ?? 0) - dMerged - dClosed, 0); + base.handled = (base.handled ?? 0) + dMerged + dClosed + dCommented; + base.merged = (base.merged ?? 0) + dMerged; + base.closed = (base.closed ?? 0) + dClosed; + base.commented = (base.commented ?? 0) + dCommented; + dispoByProject.set(delta.project, base); + } + const mergedDispositions = [...dispoByProject.values()]; + const reversedByProject = new Map(reversalRows.map((r) => [r.project, r.reversed ?? 0])); const totals = { handled: 0, merged: 0, closed: 0, commented: 0, ignored: 0, manual: 0, error: 0, reversed: 0 }; - const byProject = dispositions + const byProject = mergedDispositions .map((d) => { const merged = d.merged ?? 0; const closed = d.closed ?? 0; @@ -145,7 +213,11 @@ export async function getPublicStats(env: Env, nowMs: number = Date.now()): Prom .sort((a, b) => b.reviewed - a.reviewed); const reviewed = reviewedOf(totals); - const w = weekly[0] ?? { merged: 0, closed: 0, commented: 0, manual: 0 }; + // Weekly hero delta: frozen trailing-7d rows (if any remain in-window post-freeze) PLUS the live trailing-7d + // overlay, so '+N this week' reflects what the agent has actually done since the cutover. + const wBase = weekly[0] ?? { merged: 0, closed: 0, commented: 0, manual: 0 }; + const wLive = weeklyLive.reduce((acc, r) => ({ reviewed: acc.reviewed + (r.reviewed ?? 0), merged: acc.merged + (r.merged ?? 0) }), { reviewed: 0, merged: 0 }); + const w = { merged: (wBase.merged ?? 0) + wLive.merged, closed: wBase.closed ?? 0, commented: wBase.commented ?? 0, manual: wBase.manual ?? 0 }; const generatedAt = new Date(nowMs).toISOString(); return { generatedAt, @@ -157,7 +229,7 @@ export async function getPublicStats(env: Env, nowMs: number = Date.now()): Prom accuracyPct: accuracyPct(totals.merged, totals.closed, totals.reversed), minutesSaved: reviewed * MINUTES_SAVED_PER_PR, }, - weekly: { reviewed: reviewedOf(w), merged: w.merged ?? 0 }, + weekly: { reviewed: reviewedOf(w) + wLive.reviewed, merged: w.merged ?? 0 }, byProject, }; } diff --git a/src/review/safety.ts b/src/review/safety.ts index 59b6cc73df..e1633b69cf 100644 --- a/src/review/safety.ts +++ b/src/review/safety.ts @@ -9,6 +9,15 @@ import type { AdvisoryFinding } from "../types"; import { neutralizePromptInjection, safeReviewTitle } from "./prompt-injection"; import { scanForSecrets } from "./secrets-scan"; +// Concrete credential formats only — NOT the weak heuristics (`seed_or_mnemonic` / `bittensor_key`) that +// false-positive on legitimate config/workflow content. A `coldkey:` / `hotkey =` line or the word +// "mnemonic" in a .toml, .github/workflows/**, or wrangler/workers config is NOT a leaked credential, but it +// matches those two patterns — on these Bittensor repos that wrongly hard-blocked owner config/workflow PRs +// (RC6: #1505/#1495/#1485). A real-format token IS a leak regardless of the file it lives in, so we keep the +// concrete formats as hard blockers and ignore only the ambiguous heuristics. This mirrors the same gate the +// content lane already uses (src/review/content-lane/security-scan.ts). +const HARD_SECRET_KINDS = new Set(["github_token", "github_pat", "private_key_block", "aws_access_key", "slack_token"]); + /** True when the safety scan is enabled. Flag-OFF (default) → every helper below is a no-op pass-through. */ export function isSafetyEnabled(env: { GITTENSORY_REVIEW_SAFETY?: string | undefined }): boolean { return /^(1|true|yes|on)$/i.test(env.GITTENSORY_REVIEW_SAFETY ?? ""); @@ -34,17 +43,24 @@ export function defangReviewInput(input: SafetyReviewInput): { title: string; bo /** * Scan the PR diff for leaked secrets and, on a hit, return ONE critical `secret_leak` advisory finding (else * null). Mapped to gittensory's {@link AdvisoryFinding} shape. The gate treats this code as a hard blocker - * (see rules/advisory.ts) so a leaked secret holds the PR. Callers MUST gate this on {@link isSafetyEnabled} — - * when OFF, no finding is produced so the advisory/gate is unchanged. + * (see rules/advisory.ts) so a leaked secret holds the PR. Only CONCRETE credential formats + * ({@link HARD_SECRET_KINDS}) qualify — the weak `seed_or_mnemonic` / `bittensor_key` heuristics are ignored + * here because they false-positive on legitimate config/workflow content (e.g. `coldkey:` / `hotkey =` lines + * in *.toml, .github/workflows/**, or wrangler/workers config). Callers MUST gate this on + * {@link isSafetyEnabled} — when OFF, no finding is produced so the advisory/gate is unchanged. */ export function secretLeakFinding(diff: string): AdvisoryFinding | null { - const scan = scanForSecrets(diff); - if (!scan.found) return null; + // Only CONCRETE credential formats hard-block. The raw scanner also returns the weak `seed_or_mnemonic` / + // `bittensor_key` heuristics, which false-positive on `coldkey:` / `hotkey =` / "mnemonic" lines in + // legitimate config/workflow files (RC6); those are filtered out here so they never produce a `secret_leak` + // blocker. A real token (github_token, aws_access_key, …) still blocks regardless of which file it is in. + const kinds = scanForSecrets(diff).kinds.filter((kind) => HARD_SECRET_KINDS.has(kind)); + if (kinds.length === 0) return null; return { code: "secret_leak", severity: "critical", - title: `Possible leaked secret in the diff (${scan.kinds.join(", ")})`, - detail: `The PR diff matches secret pattern(s): ${scan.kinds.join(", ")}. A committed credential must be rotated and removed from the change before merge.`, + title: `Possible leaked secret in the diff (${kinds.join(", ")})`, + detail: `The PR diff matches secret pattern(s): ${kinds.join(", ")}. A committed credential must be rotated and removed from the change before merge.`, action: "Remove the secret from the diff, rotate the exposed credential, then re-run the gate.", }; } diff --git a/src/services/agent-action-executor.ts b/src/services/agent-action-executor.ts index 0fe61ab793..3b928eff92 100644 --- a/src/services/agent-action-executor.ts +++ b/src/services/agent-action-executor.ts @@ -1,4 +1,6 @@ -import { createPendingAgentActionIfAbsent, insertNotificationDeliveryIfAbsent, recordAuditEvent } from "../db/repositories"; +import { bumpPullRequestMergeAttempt, createPendingAgentActionIfAbsent, insertNotificationDeliveryIfAbsent, markPullRequestMergeBlocked, recordAuditEvent } from "../db/repositories"; +import { classifyMergeFailure, MERGE_RETRY_CAP } from "./merge-failure"; +import { notifyActionToDiscord, type NotifyOutcome } from "./notify-discord"; import { ensurePullRequestLabel } from "../github/labels"; import { closePullRequest, createIssueComment, createPullRequestReview, mergePullRequest } from "../github/pr-actions"; import { isActingAutonomyLevel, resolveAutonomy } from "../settings/autonomy"; @@ -24,6 +26,8 @@ export type AgentActionExecutionContext = { agentPaused?: boolean | undefined; agentDryRun?: boolean | undefined; installationPermissions: Record | null | undefined; + // PR author login — surfaced as the "Submitter" in the per-repo Discord action notification. + authorLogin?: string | null | undefined; }; export type AgentActionOutcome = { @@ -87,14 +91,61 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE try { await performAction(env, ctx, action); await audit("completed", action.reason); + // Per-repo Discord notification on a terminal/visible action (reviewbot parity): merge→merged, + // close→closed, request_changes→manual review. Best-effort; never affects the action. RC1 dedups at the + // action level, so this fires once per outcome per PR (no spam). + const notifyOutcome: NotifyOutcome | null = + action.actionClass === "merge" ? "merged" : action.actionClass === "close" ? "closed" : action.actionClass === "request_changes" ? "manual" : null; + if (notifyOutcome) { + await notifyActionToDiscord(env, { repoFullName: ctx.repoFullName, pullNumber: ctx.pullNumber, outcome: notifyOutcome, summary: action.reason, submitter: ctx.authorLogin }).catch(() => undefined); + } } catch (error) { await audit("error", errorMessage(error)); + // RC3 terminal-fail merges: a merge that fails on perms (403/405) / required-check-absent (409) / a real + // conflict can NEVER complete for this commit — mark it terminally merge-blocked so the planner stops + // re-planning it every sweep. A possibly-transient failure is retried up to MERGE_RETRY_CAP then held. + if (action.actionClass === "merge" && ctx.headSha) { + await handleMergeFailure(env, ctx, error); + } } } return outcomes; } +// RC3: persist the outcome of a FAILED merge so it is never retried blindly forever. A non-transient failure +// (403/405 perms, 409 required-check-absent, merge conflict) is terminal immediately; an otherwise-unclassified +// failure (e.g. base moved during the merge — a benign TOCTOU race) is retried up to MERGE_RETRY_CAP and then +// escalated to the same terminal hold. Either way the planner suppresses the merge for this head SHA and the PR +// is held for a human (never auto-closed). +async function handleMergeFailure(env: Env, ctx: AgentActionExecutionContext, error: unknown): Promise { + const headSha = ctx.headSha; + /* v8 ignore next -- guarded at the call site; defensive. */ + if (!headSha) return; + const message = errorMessage(error); + const { terminal: classifiedTerminal, reason: classifiedReason } = classifyMergeFailure(error); + let terminal = classifiedTerminal; + let reason = classifiedReason; + if (!terminal) { + // Possibly transient: bound the retries so a persistently-failing "clean" merge still escalates. + const attempts = await bumpPullRequestMergeAttempt(env, ctx.repoFullName, ctx.pullNumber, headSha); + if (attempts >= MERGE_RETRY_CAP) { + terminal = true; + reason = `merge could not complete after ${attempts} attempt(s): ${message}`; + } + } + if (!terminal) return; + await markPullRequestMergeBlocked(env, ctx.repoFullName, ctx.pullNumber, headSha, reason); + await recordAuditEvent(env, { + eventType: "agent.action.merge_blocked", + actor: AGENT_ACTOR, + targetKey: `${ctx.repoFullName}#${ctx.pullNumber}`, + outcome: "denied", + detail: `merge held for human — ${reason}`, + metadata: { repoFullName: ctx.repoFullName, pullNumber: ctx.pullNumber, headSha, reason: reason.slice(0, 280) }, + }).catch(() => undefined); +} + async function performAction(env: Env, ctx: AgentActionExecutionContext, action: PlannedAgentAction): Promise { switch (action.actionClass) { case "label": diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index f466fbbe77..23e002648f 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -400,7 +400,13 @@ export async function runGittensoryAiReview(env: Env, input: GittensoryAiReviewI // Estimate against the EFFECTIVE system prompt (`system`) so grounding's extra context is billed against the // budget. Flag-OFF, `system === REVIEW_SYSTEM_PROMPT`, so the estimate is byte-identical to today. const estimatedNeurons = freeAiCalls === 0 ? 0 : estimateNeurons(system.length + user.length, maxTokens, freeAiCalls); - const budget = clampNumber(Number(env.AI_DAILY_NEURON_BUDGET || 10000), 0, 10_000_000); + // FAIL-SAFE default (#budget-no-starve): the daily neuron budget is a runaway-LOOP backstop, not a normal- + // operation gate. An absent/empty/non-numeric env var must default HIGH (the clamp max), never to a tiny value + // that silently starves every dual-AI review into quota_exceeded — that exact misconfig (the deployed worker + // read the 10k free-tier default off `main` while this branch said 2M) blocked all reviews. An EXPLICIT value + // (including "0" to deliberately disable) still wins; only unset/empty/NaN falls back to the safe maximum. + const rawNeuronBudget = Number(env.AI_DAILY_NEURON_BUDGET); + const budget = clampNumber(env.AI_DAILY_NEURON_BUDGET && Number.isFinite(rawNeuronBudget) ? rawNeuronBudget : 10_000_000, 0, 10_000_000); const used = await sumAiEstimatedNeuronsSince(env, utcDayStartIso()); const remainingBudget = Math.max(0, budget - used); if (estimatedNeurons > remainingBudget) { diff --git a/src/services/merge-failure.ts b/src/services/merge-failure.ts new file mode 100644 index 0000000000..bc30878e2d --- /dev/null +++ b/src/services/merge-failure.ts @@ -0,0 +1,43 @@ +import { errorMessage } from "../utils/json"; + +// RC3 terminal-fail merges. A merge mutation that fails for one of these reasons can NEVER complete for the +// current commit, so retrying it every sweep is pointless and noisy — classify it once and let the executor +// mark the PR terminally merge-blocked (held for a human) instead of looping forever. +// +// • 403 Resource not accessible by integration → the App lacks pull_requests:write / the branch is +// protected against the App. A human must re-consent or merge. +// • 405 Method Not Allowed → merge not allowed (e.g. required reviews/checks policy forbids an App merge). +// • 409 Conflict → a required status check is absent / head moved into a non-mergeable state. +// • merge-conflict text → the branch genuinely conflicts with base; only the contributor can resolve it. +// +// A failure that matches none of these is treated as POSSIBLY transient (e.g. "Base branch was modified" — a +// benign TOCTOU race that a re-attempt against the new base resolves), so the executor retries it up to +// MERGE_RETRY_CAP before escalating to the same terminal hold. +export const MERGE_RETRY_CAP = 5; + +const TERMINAL_MERGE_STATUSES = new Set([403, 405, 409]); + +/** True when the merge error TEXT describes a real content conflict (vs a behind-but-clean branch). */ +function isMergeConflictMessage(message: string): boolean { + return /merge conflict|not mergeable|cannot be merged|has conflicts|conflicts? with the base/i.test(message); +} + +/** Read the HTTP status off an Octokit RequestError (it sets `.status`); undefined for non-HTTP errors. */ +function httpStatus(error: unknown): number | undefined { + const status = (error as { status?: unknown } | null | undefined)?.status; + return typeof status === "number" ? status : undefined; +} + +/** Classify a failed merge. `terminal: true` → never re-plan this merge for the current commit (hold for a + * human). `terminal: false` → possibly transient; the caller retries up to MERGE_RETRY_CAP. `reason` is a + * short human-readable summary persisted on the PR + audit record. */ +export function classifyMergeFailure(error: unknown): { terminal: boolean; reason: string } { + const message = errorMessage(error); + const status = httpStatus(error); + if (status === 403) return { terminal: true, reason: `merge forbidden (403 — pull_requests:write or branch protection): ${message}` }; + if (status === 405) return { terminal: true, reason: `merge not allowed (405 — repo merge policy forbids an automated merge): ${message}` }; + if (status === 409) return { terminal: true, reason: `merge conflict / required check absent (409): ${message}` }; + if (status !== undefined && TERMINAL_MERGE_STATUSES.has(status)) return { terminal: true, reason: `merge rejected (${status}): ${message}` }; + if (isMergeConflictMessage(message)) return { terminal: true, reason: `branch conflicts with base — contributor must rebase: ${message}` }; + return { terminal: false, reason: message }; +} diff --git a/src/services/notify-discord.ts b/src/services/notify-discord.ts new file mode 100644 index 0000000000..bf96c72af8 --- /dev/null +++ b/src/services/notify-discord.ts @@ -0,0 +1,73 @@ +import { errorMessage } from "../utils/json"; + +// Per-repo Discord notifications (reviewbot parity). Each repo notifies its OWN channel on a terminal action — +// merged / closed / changes-requested(manual) — so the operator sees what the bot did, like the old Reviewbott +// embeds. Best-effort: a notify failure NEVER affects the gate/action (wrapped + swallowed by the caller). +// RC1 already dedups at the action level (the planner won't re-post an unchanged verdict), so this fires once +// per outcome per PR without a separate notification ledger. + +const ALLOWED_DISCORD_HOSTS = new Set(["discord.com", "discordapp.com", "canary.discord.com", "ptb.discord.com"]); + +function isValidDiscordWebhook(url: string): boolean { + try { + const parsed = new URL(url); + return parsed.protocol === "https:" && ALLOWED_DISCORD_HOSTS.has(parsed.hostname.toLowerCase()) && parsed.pathname.startsWith("/api/webhooks/"); + } catch { + return false; + } +} + +// Map each repo to its operator-set webhook SECRET name (set via `wrangler secret put`). A repo with no mapping +// (or an unset secret) simply does not notify — byte-identical to today for any other repo. +const WEBHOOK_SECRET_BY_REPO: Record = { + "jsonbored/gittensory": "GITTENSORY_DISCORD_WEBHOOK", + "jsonbored/metagraphed": "METAGRAPHED_DISCORD_WEBHOOK", + "jsonbored/awesome-claude": "AWESOME_DISCORD_WEBHOOK", +}; + +function resolveWebhook(env: Env, repoFullName: string): string | undefined { + const name = WEBHOOK_SECRET_BY_REPO[repoFullName.toLowerCase()]; + if (!name) return undefined; + const value = (env as unknown as Record)[name]; + return typeof value === "string" && value.length > 0 ? value : undefined; +} + +export type NotifyOutcome = "merged" | "closed" | "manual"; + +const OUTCOME_META: Record = { + merged: { word: "merged", color: 0x2ea043 }, + closed: { word: "closed", color: 0xcf222e }, + manual: { word: "manual review", color: 0xbf8700 }, +}; + +/** Post a per-action Discord embed (merged/closed/manual) to the repo's channel. Best-effort: never throws. */ +export async function notifyActionToDiscord( + env: Env, + params: { repoFullName: string; pullNumber: number; outcome: NotifyOutcome; summary: string; submitter?: string | null | undefined }, +): Promise { + const webhookUrl = resolveWebhook(env, params.repoFullName); + if (!webhookUrl || !isValidDiscordWebhook(webhookUrl)) return; + const meta = OUTCOME_META[params.outcome]; + const body = { + username: "Gittensory", + embeds: [ + { + title: `${params.repoFullName}#${params.pullNumber} · ${meta.word}`, + url: `https://github.com/${params.repoFullName}/pull/${params.pullNumber}`, + description: (params.summary || meta.word).slice(0, 1800), + color: meta.color, + fields: [ + { name: "Outcome", value: `\`${params.outcome}\``, inline: true }, + { name: "PR", value: `#${params.pullNumber}`, inline: true }, + ...(params.submitter ? [{ name: "Submitter", value: `@${params.submitter}`, inline: true }] : []), + ], + footer: { text: `Gittensory · ${params.repoFullName}` }, + }, + ], + }; + try { + await fetch(webhookUrl, { method: "POST", headers: { "content-type": "application/json" }, body: JSON.stringify(body), signal: AbortSignal.timeout(10_000) }); + } catch (error) { + console.warn(JSON.stringify({ ev: "discord_notify_failed", repo: params.repoFullName, pull: params.pullNumber, message: errorMessage(error).slice(0, 120) })); + } +} diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index 888cb6a671..d574e6de8d 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -79,6 +79,10 @@ export type AgentActionPlanInput = { slopRisk?: number | null | undefined; labels: string[]; linkedDuplicateCount?: number | undefined; + // RC3 terminal-fail merges: the live head SHA + the SHA at which a prior merge was terminally blocked + // (perms/required-check/conflict). When they match, the merge can't complete for this commit → suppress it. + headSha?: string | null | undefined; + mergeBlockedSha?: string | null | undefined; }; }; @@ -174,7 +178,11 @@ export function planAgentMaintenanceActions(input: AgentActionPlanInput): Planne // the failing checks). Owner + maintainer-automation PRs are NEVER closed (a red-CI owner PR is held via the // request_changes above, left open for the maintainer). Mutually exclusive with merge. const mergeableClean = input.pr.mergeableState === "clean"; - const canMerge = readyToMerge && acting("merge") && mergeableClean && approvalsSatisfied; + // RC3: a prior merge attempt failed terminally for THIS exact head SHA (403/405/409/conflict) → never re-plan + // the merge; it can't complete for this commit. A new commit makes the live head differ from mergeBlockedSha, + // so this only suppresses the genuinely-stuck merge — the PR falls through to needs-human-review. + const mergeTerminallyBlocked = input.pr.mergeBlockedSha != null && input.pr.headSha != null && input.pr.mergeBlockedSha === input.pr.headSha; + const canMerge = readyToMerge && acting("merge") && mergeableClean && approvalsSatisfied && !mergeTerminallyBlocked; if (canMerge) { actions.push({ actionClass: "merge", diff --git a/src/settings/agent-sweep.ts b/src/settings/agent-sweep.ts index df3f9ec6ae..f49a7d75d8 100644 --- a/src/settings/agent-sweep.ts +++ b/src/settings/agent-sweep.ts @@ -10,8 +10,11 @@ import type { PullRequestRecord } from "../types"; export const SWEEP_MAX_PRS = 25; // Skip-if-fresh window: a PR touched within this span was almost certainly just gated by its webhook, so the -// sweep leaves it alone and spends its budget on genuinely stale PRs. One hour mirrors the sweep cadence. -export const SWEEP_FRESHNESS_MS = 60 * 60 * 1000; +// sweep leaves it alone for that brief moment to avoid racing the in-flight webhook review. Kept SHORT (2 min) +// because the sweep is now LIGHT (re-gate + act, no AI) and runs every ~2 min — a just-approved PR must be +// re-evaluated within minutes so it MERGES once its approval registers (BLOCKED→CLEAN). One hour stranded +// approved PRs unmerged for up to an hour. +export const SWEEP_FRESHNESS_MS = 2 * 60 * 1000; /** * Select the open PRs a single repo sweep should recompute: drop drafts and anything updated within diff --git a/src/types.ts b/src/types.ts index fa225cf001..71b10d4edb 100644 --- a/src/types.ts +++ b/src/types.ts @@ -412,6 +412,12 @@ export type PullRequestRecord = { * the repo opted into slop. `null`/absent = not assessed (slop off, or PR not yet processed). */ slopRisk?: number | null | undefined; slopBand?: string | null | undefined; + /** RC3 terminal-fail merges: failed auto-merge attempt count, and the head SHA at which the merge is + * terminally blocked (with a human-readable reason). When mergeBlockedSha === headSha the planner suppresses + * the `merge` disposition (held for a human); a new commit clears the block. */ + mergeAttemptCount?: number | null | undefined; + mergeBlockedSha?: string | null | undefined; + mergeBlockedReason?: string | null | undefined; }; export type IssueRecord = { diff --git a/test/unit/agent-sweep.test.ts b/test/unit/agent-sweep.test.ts index 874a7694f2..f01b32382d 100644 --- a/test/unit/agent-sweep.test.ts +++ b/test/unit/agent-sweep.test.ts @@ -19,9 +19,9 @@ function pr(overrides: Partial & { number: number }): PullReq describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { it("drops PRs updated within the freshness window (recently gated by their webhook)", () => { - const pulls = [pr({ number: 1, updatedAt: minutesAgo(5) }), pr({ number: 2, updatedAt: minutesAgo(120) })]; + const pulls = [pr({ number: 1, updatedAt: minutesAgo(1) }), pr({ number: 2, updatedAt: minutesAgo(120) })]; const picked = selectRegateCandidates({ pulls, now: NOW }); - expect(picked.map((p) => p.number)).toEqual([2]); + expect(picked.map((p) => p.number)).toEqual([2]); // #1 updated 1m ago is inside the 2-min freshness window }); it("orders the stalest first and bounds to max (rate-aware)", () => { @@ -63,8 +63,8 @@ describe("selectRegateCandidates (#777 re-gate sweep selection)", () => { expect(picked.map((p) => p.number)).toEqual([2, 1]); // drafts still excluded; both non-draft kept, stalest first }); - it("defaults: freshness window is one hour and the cap is 25", () => { - expect(SWEEP_FRESHNESS_MS).toBe(60 * 60 * 1000); + it("defaults: freshness window is two minutes and the cap is 25", () => { + expect(SWEEP_FRESHNESS_MS).toBe(2 * 60 * 1000); expect(SWEEP_MAX_PRS).toBe(25); const pulls = Array.from({ length: 40 }, (_, i) => pr({ number: i + 1, updatedAt: minutesAgo(120 + i) })); expect(selectRegateCandidates({ pulls, now: NOW })).toHaveLength(25); diff --git a/test/unit/index.test.ts b/test/unit/index.test.ts index 00dbab5691..6401268cbe 100644 --- a/test/unit/index.test.ts +++ b/test/unit/index.test.ts @@ -65,7 +65,7 @@ describe("worker entrypoint", () => { expect(waitUntil).toHaveLength(1); }); - it("enqueues light scheduled work outside hourly and full-sync windows", async () => { + it("enqueues only the light auto-maintain sweep on a regular tick (not :00 or :30)", async () => { const sent: Array = []; const env = createTestEnv({ JOBS: { @@ -76,14 +76,12 @@ describe("worker entrypoint", () => { }); const waitUntil: Promise[] = []; - await worker.scheduled(controllerFor("2026-05-25T05:15:00.000Z"), env, executionContext(waitUntil)); + await worker.scheduled(controllerFor("2026-05-25T05:14:00.000Z"), env, executionContext(waitUntil)); await Promise.all(waitUntil); - expect(sent).toEqual([ - { type: "backfill-registered-repos", requestedBy: "schedule", mode: "light" }, - { type: "repair-data-fidelity", requestedBy: "schedule" }, - { type: "refresh-installation-health", requestedBy: "schedule" }, - ]); + // A regular */2 tick (not :00, not :30) enqueues ONLY the light auto-maintain sweep — the heavier sync/health + // jobs are gated to :00/:30, so the tight cadence stays cheap while merges/closes fire promptly. + expect(sent).toEqual([{ type: "agent-regate-sweep", requestedBy: "schedule" }]); }); it("enqueues hourly refreshes without full detail work outside the six-hour window", async () => { @@ -101,6 +99,7 @@ describe("worker entrypoint", () => { await Promise.all(waitUntil); expect(sent).toEqual([ + { type: "agent-regate-sweep", requestedBy: "schedule" }, { type: "backfill-registered-repos", requestedBy: "schedule", mode: "light" }, { type: "repair-data-fidelity", requestedBy: "schedule" }, { type: "refresh-installation-health", requestedBy: "schedule" }, @@ -108,7 +107,6 @@ describe("worker entrypoint", () => { { type: "refresh-scoring-model", requestedBy: "schedule" }, { type: "refresh-upstream-drift", requestedBy: "schedule" }, { type: "rollup-product-usage", requestedBy: "schedule", days: 7 }, - { type: "agent-regate-sweep", requestedBy: "schedule" }, ]); }); @@ -127,6 +125,7 @@ describe("worker entrypoint", () => { await Promise.all(waitUntil); expect(sent).toEqual([ + { type: "agent-regate-sweep", requestedBy: "schedule" }, { type: "backfill-registered-repos", requestedBy: "schedule", mode: "full" }, { type: "repair-data-fidelity", requestedBy: "schedule" }, { type: "refresh-installation-health", requestedBy: "schedule" }, @@ -134,7 +133,6 @@ describe("worker entrypoint", () => { { type: "refresh-scoring-model", requestedBy: "schedule" }, { type: "refresh-upstream-drift", requestedBy: "schedule" }, { type: "rollup-product-usage", requestedBy: "schedule", days: 7 }, - { type: "agent-regate-sweep", requestedBy: "schedule" }, { type: "generate-signal-snapshots", requestedBy: "schedule" }, { type: "build-burden-forecasts", requestedBy: "schedule" }, { type: "build-contributor-evidence", requestedBy: "schedule" }, diff --git a/test/unit/public-stats.test.ts b/test/unit/public-stats.test.ts index 8f3f53d9b1..af49df6d01 100644 --- a/test/unit/public-stats.test.ts +++ b/test/unit/public-stats.test.ts @@ -31,6 +31,15 @@ describe("getPublicStats — live aggregate over the review ledger", () => { // Real prod proportions: merged 1392 + closed 724 + commented 514 + ignored 491 + manual 78 + error 34 = 3233; // reviewed = 1392+724+514+78 = 2708; reversed 33 over 2116 auto-actions. function ledger(sql: string): Row[] { + // RC8: the live audit_events overlay reads return [] here so the existing assertions stay byte-identical to + // the frozen base (the overlay adds nothing). Matched FIRST so the weekly-overlay variant — which contains + // both `FROM audit_events` and `created_at >= ?` — never falls through to the frozen-weekly branch. + if (sql.includes("FROM audit_events")) { + return []; + } + if (sql.includes("MAX(created_at) AS frozenAt")) { + return [{ frozenAt: "2026-06-22 17:28:00" }]; + } if ( sql.includes("FROM review_targets") && sql.includes("GROUP BY project") diff --git a/wrangler.jsonc b/wrangler.jsonc index e756b139e8..8c5cb9e42b 100644 --- a/wrangler.jsonc +++ b/wrangler.jsonc @@ -83,7 +83,7 @@ // Default OFF — flag-OFF performs no retrieval, uses no adapter, makes no vector query, and keeps the // reviewer prompt byte-identical. Even when ON it is inert until a repo's vector index is populated (the // index-population job + cron is a deploy-time follow-up; a cold/missing index degrades to no context). - "GITTENSORY_REVIEW_RAG": "true", + "GITTENSORY_REVIEW_RAG": "false", // Convergence (self-improve / auto-tune): run the ported self-improvement loop on the cron tick over // gittensory's own review-outcome data — compute tuning recommendations, SHADOW-SOAK any strictly- // tightening one, and AUTO-PROMOTE it to live ONLY after the soak window passes the gate; every action is