From f089ebb7e6389caf341790e090a800846cb97f34 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 22 Jun 2026 17:26:01 -0700 Subject: [PATCH 1/2] =?UTF-8?q?fix(review):=20CI=20that=20hasn't=20passed?= =?UTF-8?q?=20is=20never=20'safe=20to=20merge'=20(pending/unverified=20?= =?UTF-8?q?=E2=86=92=20held)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The earlier fix downgraded only FAILED CI to Blocked; a PENDING/unverified CI still fell through to the merge verdict and rendered 'Approved — safe to merge' while checks were in progress (chip already said 'CI pending'). deriveUnifiedStatus now downgrades ANY non-passed ciState before the verdict switch: failed→blocked, pending/unverified→held. Only ciState='passed' can render ready/safe-to-merge. --- src/review/unified-comment.ts | 14 +++++++++----- test/unit/unified-comment.test.ts | 6 ++++-- 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/src/review/unified-comment.ts b/src/review/unified-comment.ts index 979a832afa..f258824f0a 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -217,11 +217,15 @@ const SIGNAL_ICON: Record = { ok: "✅", warn /** Derive the single unified status from reviewbot's decision/recs/CI + the host override. */ export function deriveUnifiedStatus(input: UnifiedReviewInput, ctx: UnifiedCommentContext = {}): UnifiedCommentStatus { if (ctx.statusOverride) return ctx.statusOverride; - // A failing CI is NEVER "safe to merge": a red CI downgrades any otherwise-ready/merge verdict to blocked (the - // disposition layer then closes it for a non-owner author / holds it open for the owner). This runs BEFORE the - // explicit-verdict switch so an optimistic gate "merge" can't render a green "safe to merge" headline over a - // red CI — the exact bug where a PR with a failing codecov/patch showed "Approved — safe to merge". - if (input.readiness?.ciState === "failed") return "blocked"; + // CI gate — a PR is "safe to merge" ONLY when CI is GREEN. This runs BEFORE the explicit-verdict switch so an + // optimistic gate "merge" can never render a "safe to merge" headline over a CI that hasn't passed: + // • failed → BLOCKED (red CI; the disposition layer closes non-owner / holds owner) + // • unverified / pending (chip "CI pending") → HELD (still running / not yet reported — NOT safe to merge) + // Only ciState === "passed" falls through to honor the gate verdict. (Bug this fixes: a PR with a failing + // codecov OR with CI still in progress showed "Approved — safe to merge".) + if (input.readiness && input.readiness.ciState !== "passed") { + return input.readiness.ciState === "failed" ? "blocked" : "held"; + } // An explicit gate verdict is authoritative — it already weighed the reviewers + guardrails. switch (input.decision) { case "merge": diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index c87bdf1b9d..ee08c00dd3 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -36,11 +36,13 @@ describe("deriveUnifiedStatus", () => { expect(deriveUnifiedStatus({ ...base, recommendations: ["request_changes"] })).toBe("held"); }); - it("a failing CI is BLOCKED (never safe-to-merge) and overrides an optimistic merge verdict", () => { + it("CI that hasn't passed is NEVER safe-to-merge — failed→blocked, pending/unverified→held, even over a merge verdict", () => { // A red CI must never render "safe to merge". It downgrades even an explicit `merge` verdict to blocked. expect(deriveUnifiedStatus({ ...base, readiness: { ciState: "failed" } })).toBe("blocked"); expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "failed" } })).toBe("blocked"); - // green CI + merge verdict still renders ready. + // CI still running / not yet reported (chip "CI pending") → HELD, never "safe to merge". + expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "unverified" } })).toBe("held"); + // ONLY green CI + a merge verdict renders ready. expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed" } })).toBe("ready"); }); From 863ad12f62ff4e66eb9263b91e379534a82e14d0 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 22 Jun 2026 17:56:22 -0700 Subject: [PATCH 2/2] fix(review): port reviewbot type-labels + visual after-shot self-poll MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit LABELS (C): port reviewbot's auto-label — a new src/settings/pr-type-label.ts derives exactly ONE per-PR TYPE label (gittensor:bug/feature/priority) from the conventional-commit title + changed paths, applied at review time (best-effort, independent of the gate/autonomy/dry-run/CI) next to the context label. gittensory only ever applied the static 'gittensor' context label + the autonomy status labels; the bug/feature/priority system was never ported. Adds removePullRequestLabel to keep the three mutually exclusive. VISUAL (D): the AFTER (preview) cell was a 'loading' placeholder forever when the deploy wasn't live at review time — the only refill was a fragile deployment_status webhook. Add reviewbot's self-poll: capture.previewPending now schedules a delayed 'recapture-preview' job (90s, bounded to MAX_PREVIEW_POLLS) that re-reviews the PR to re-capture the now-ready shot. Bundles the staged pending-CI fix (CI that hasn't passed is never 'safe to merge'). AI review mode set to 'block' (full dual review) in D1 — separate live change. --- src/github/labels.ts | 12 +++++++++ src/queue/processors.ts | 47 ++++++++++++++++++++++++++++++--- src/settings/pr-type-label.ts | 44 ++++++++++++++++++++++++++++++ src/types.ts | 11 ++++++++ test/unit/pr-type-label.test.ts | 29 ++++++++++++++++++++ test/unit/queue.test.ts | 10 ++++--- 6 files changed, 146 insertions(+), 7 deletions(-) create mode 100644 src/settings/pr-type-label.ts create mode 100644 test/unit/pr-type-label.test.ts diff --git a/src/github/labels.ts b/src/github/labels.ts index 9c1cd4ed03..1c5e026981 100644 --- a/src/github/labels.ts +++ b/src/github/labels.ts @@ -55,3 +55,15 @@ export async function ensurePullRequestLabel( }); return { applied: true, created }; } + +/** Remove a single label from a PR if present. Best-effort — a 404 (label not on the PR) is ignored. Used to + * keep the mutually-exclusive managed TYPE labels (gittensor:bug/feature/priority) down to exactly one. */ +export async function removePullRequestLabel(env: Env, installationId: number, repoFullName: string, pullNumber: number, labelName: string): Promise { + const [owner, repo] = repoFullName.split("/"); + if (!owner || !repo) return; + const token = await createInstallationToken(env, installationId); + const octokit = new Octokit({ auth: token }); + await octokit + .request("DELETE /repos/{owner}/{repo}/issues/{issue_number}/labels/{name}", { owner, repo, issue_number: pullNumber, name: labelName }) + .catch(() => undefined); +} diff --git a/src/queue/processors.ts b/src/queue/processors.ts index f5b9374896..a8638c1873 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -85,7 +85,8 @@ import { parseGittensoryMentionCommand, sanitizePublicComment, } from "../github/commands"; -import { ensurePullRequestLabel } from "../github/labels"; +import { ensurePullRequestLabel, removePullRequestLabel } from "../github/labels"; +import { ALL_TYPE_LABELS, resolvePrTypeLabel } from "../settings/pr-type-label"; import { fetchPublicContributorProfile } from "../github/public"; import { refreshRegistry } from "../registry/sync"; import { buildIssueAdvisory, buildPullRequestAdvisory, evaluateGateCheck, isTestPath } from "../rules/advisory"; @@ -348,6 +349,10 @@ export async function processJob(env: Env, message: JobMessage): Promise { // so flag-OFF does zero work here too. indexRepo / reindexChangedPaths are fully fail-safe (never throw). if (isRagEnabled(env)) await runRagIndexJob(env, message.requestedBy, message.repoFullName, message.paths); return; + case "recapture-preview": + // Delayed visual self-poll: re-review the PR to re-capture the AFTER preview shot once its deploy is live. + await reReviewStoredPullRequest(env, message.deliveryId, message.installationId, message.repoFullName, message.prNumber, message.attempt); + break; case "github-webhook": await processGitHubWebhook(env, message.deliveryId, message.eventName, message.payload); return; @@ -667,7 +672,7 @@ async function maybeRunAgentMaintenance( * "the CI event WAKES the existing row and re-runs the full review". The PR's persisted head SHA is used as-is * (never overwritten from the CI payload — reviewbot scope parity). Best-effort throughout. */ -async function reReviewStoredPullRequest(env: Env, deliveryId: string, installationId: number, repoFullName: string, prNumber: number): Promise { +async function reReviewStoredPullRequest(env: Env, deliveryId: string, installationId: number, repoFullName: string, prNumber: number, previewPollAttempt?: number): Promise { 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; @@ -677,7 +682,7 @@ async function reReviewStoredPullRequest(env: Env, deliveryId: string, installat if (shouldCollectSlopEvidence(settings) || settings.manifestPolicyGateMode !== "off") { await refreshPullRequestDetails(env, repoFullName, prNumber).catch(() => undefined); } - const gate = await maybePublishPrPublicSurface(env, installationId, repoFullName, pr, repo, settings, advisory, { deliveryId }).catch((error) => { + const gate = await maybePublishPrPublicSurface(env, installationId, repoFullName, pr, repo, settings, advisory, { deliveryId, ...(previewPollAttempt !== undefined ? { previewPollAttempt } : {}) }).catch((error) => { console.error(JSON.stringify({ level: "warn", event: "pr_public_surface_failed", deliveryId, repository: repoFullName, pullNumber: prNumber, error: errorMessage(error) })); return undefined; }); @@ -693,6 +698,12 @@ async function reReviewStoredPullRequest(env: Env, deliveryId: string, installat // re-check still catch the settled state. const CI_COALESCE_WINDOW_SECONDS = 60; +// Visual preview self-poll (reviewbot PREVIEW_POLL_SECONDS parity): when a PR's preview deploy isn't live at +// review time, re-review after this delay to re-capture the AFTER shot, up to MAX_PREVIEW_POLLS times (so a +// never-resolving preview can't poll forever ~ 5×90s = 7.5min). +const PREVIEW_POLL_SECONDS = 90; +const MAX_PREVIEW_POLLS = 5; + /** * Coalesce CI-completion re-reviews: claims a per-PR window and returns true if this PR was already re-reviewed * within CI_COALESCE_WINDOW_SECONDS (caller skips). KV-backed (REVIEW_CONFIG); a missing KV or a KV hiccup @@ -1730,7 +1741,7 @@ async function maybePublishPrPublicSurface( repo: Awaited>, settings: Awaited>, advisory: Awaited>, - webhook: { deliveryId: string; authorType?: string | undefined; action?: string | undefined }, + webhook: { deliveryId: string; authorType?: string | undefined; action?: string | undefined; previewPollAttempt?: number | undefined }, ): Promise | undefined> { const author = pr.authorLogin ?? null; // Per-repo cutover gate (GITTENSORY_REVIEW_REPOS): the unified converged comment renders for THIS repo @@ -2164,6 +2175,17 @@ async function maybePublishPrPublicSurface( previewFromChecks: true, }, visualFiles); beforeAfter = capture.routes; + // Visual self-poll: the FIRST capture returns a "loading" placeholder for the AFTER shot when the + // preview deploy isn't live yet (capture.previewPending). Schedule a delayed re-review to re-capture + // the now-ready shot — bounded by `attempt` so a never-resolving preview can't loop (the deployment_status + // webhook also refills it; this is the backstop when that event is missed/late). + const previewPollAttempt = webhook.previewPollAttempt ?? 0; + if (capture.previewPending && previewPollAttempt < MAX_PREVIEW_POLLS) { + await env.JOBS.send( + { type: "recapture-preview", deliveryId: webhook.deliveryId, repoFullName, prNumber: pr.number, installationId, attempt: previewPollAttempt + 1 }, + { delaySeconds: PREVIEW_POLL_SECONDS }, + ).catch((error) => console.log(JSON.stringify({ ev: "recapture_enqueue_failed", repoFullName, pull: pr.number, message: errorMessage(error).slice(0, 120) }))); + } } catch (error) { console.log(JSON.stringify({ ev: "visual_capture_error", repoFullName, pull: pr.number, message: errorMessage(error).slice(0, 200) })); } @@ -2220,6 +2242,23 @@ async function maybePublishPrPublicSurface( failedOutputs.push({ output: "label", error: message }); await recordPublicSurfaceOutputFailure(env, "label", author, repoFullName, pr.number, webhook.deliveryId, message); } + // Per-PR TYPE label (reviewbot auto-label parity): exactly ONE of gittensor:bug/feature/priority by the PR + // title + changed paths. Review-time + neutral, BEST-EFFORT + independent of the context label above so a + // type-label hiccup never drops the "label" output. Files are only fetched when content globs are configured + // (otherwise the label is title-derived). The status labels (ready-to-merge etc.) remain the autonomy layer's. + if (settings.autoLabelEnabled) { + try { + const contentGlobs = (settings as { contentGlobs?: string[] }).contentGlobs ?? []; + const typeFiles = contentGlobs.length > 0 ? await getReviewFiles().catch(() => [] as Awaited>) : []; + const chosenType = resolvePrTypeLabel({ title: pr.title, changedPaths: typeFiles.map((file) => file.path), contentGlobs }); + await ensurePullRequestLabel(env, installationId, repoFullName, pr.number, chosenType, { createMissingLabel: true }); + for (const other of ALL_TYPE_LABELS.filter((label) => label !== chosenType)) { + await removePullRequestLabel(env, installationId, repoFullName, pr.number, other); + } + } catch (error) { + console.log(JSON.stringify({ ev: "type_label_error", repoFullName, pull: pr.number, message: errorMessage(error).slice(0, 150) })); + } + } } if (publishedOutputs.length === 0) { if (failedOutputs.length > 0) { diff --git a/src/settings/pr-type-label.ts b/src/settings/pr-type-label.ts new file mode 100644 index 0000000000..e479b60644 --- /dev/null +++ b/src/settings/pr-type-label.ts @@ -0,0 +1,44 @@ +// Neutral per-PR TYPE label (reviewbot src/core/auto-label.ts parity). Applies EXACTLY ONE of: +// gittensor:priority — a content submission (changed paths match contentGlobs) — truly valuable. +// gittensor:feature — genuine NEW functionality only (conventional-commit `feat`/`feature`). +// gittensor:bug — EVERYTHING ELSE: fix, test, docs, chore, refactor, perf, ci, build, style, revert. +// Public + neutral categorization (NOT the reputation signal). Review-time + independent of the gate / +// autonomy / dry-run (matches reviewbot, where auto-label runs at review start). Fail-safe. +import { matchesAny } from "../signals/change-guardrail"; + +export interface PrTypeLabelSet { + bug: string; + feature: string; + priority: string; +} + +/** The gittensor: namespace the maintainer uses. The three are mutually exclusive (the other two are dropped). */ +export const DEFAULT_TYPE_LABELS: PrTypeLabelSet = { + bug: "gittensor:bug", + feature: "gittensor:feature", + priority: "gittensor:priority", +}; + +export const ALL_TYPE_LABELS: readonly string[] = [DEFAULT_TYPE_LABELS.bug, DEFAULT_TYPE_LABELS.feature, DEFAULT_TYPE_LABELS.priority]; + +/** feature ONLY for genuine new functionality (feat); EVERYTHING else — fix, test, docs, chore, refactor, + * perf, ci, build, style, revert — is bug (a test PR is a test, not a feature). (reviewbot auto-label.ts:27) */ +export function deriveKindFromTitle(title: string | undefined): "bug" | "feature" { + const match = /^([a-zA-Z]+)/.exec((title ?? "").trim()); + const type = match?.[1]?.toLowerCase(); + return type === "feat" || type === "feature" ? "feature" : "bug"; +} + +/** + * Resolve the single TYPE label for a PR (priority order): + * 1. CONTENT submission — any changed path matches a contentGlob → priority. + * 2. else feature (feat) / bug (everything else) by the conventional-commit title prefix. + * Pure + total. Returns the chosen label name. + */ +export function resolvePrTypeLabel(input: { title: string | undefined; changedPaths: string[]; contentGlobs: string[]; labels?: PrTypeLabelSet }): string { + const labels = input.labels ?? DEFAULT_TYPE_LABELS; + if (input.contentGlobs.length > 0 && input.changedPaths.some((path) => matchesAny(path, input.contentGlobs))) { + return labels.priority; + } + return labels[deriveKindFromTitle(input.title)]; +} diff --git a/src/types.ts b/src/types.ts index 55afbd0f08..fa225cf001 100644 --- a/src/types.ts +++ b/src/types.ts @@ -8,6 +8,17 @@ export type JobMessage = eventName: string; payload: GitHubWebhookPayload; } + | { + // Delayed self-poll to re-capture a PR's before/after preview once its preview deploy is live — the first + // review captures a "loading" placeholder when the deploy isn't ready yet (capture.previewPending). Each + // recapture re-reviews the PR; bounded by `attempt` so a never-resolving preview can't loop forever. + type: "recapture-preview"; + deliveryId: string; + repoFullName: string; + prNumber: number; + installationId: number; + attempt: number; + } | { type: "refresh-registry"; requestedBy: "schedule" | "api" | "test"; diff --git a/test/unit/pr-type-label.test.ts b/test/unit/pr-type-label.test.ts new file mode 100644 index 0000000000..715f405422 --- /dev/null +++ b/test/unit/pr-type-label.test.ts @@ -0,0 +1,29 @@ +import { describe, expect, it } from "vitest"; +import { DEFAULT_TYPE_LABELS, deriveKindFromTitle, resolvePrTypeLabel } from "../../src/settings/pr-type-label"; + +describe("deriveKindFromTitle", () => { + it("maps feat/feature → feature; everything else → bug", () => { + expect(deriveKindFromTitle("feat: add X")).toBe("feature"); + expect(deriveKindFromTitle("feature(api): boards")).toBe("feature"); + expect(deriveKindFromTitle("fix: bug")).toBe("bug"); + expect(deriveKindFromTitle("test: add coverage")).toBe("bug"); + expect(deriveKindFromTitle("docs: readme")).toBe("bug"); + expect(deriveKindFromTitle("chore: deps")).toBe("bug"); + expect(deriveKindFromTitle("refactor: cleanup")).toBe("bug"); + expect(deriveKindFromTitle(undefined)).toBe("bug"); + expect(deriveKindFromTitle("")).toBe("bug"); + }); +}); + +describe("resolvePrTypeLabel", () => { + it("returns the feature/bug label by title when no content globs match", () => { + expect(resolvePrTypeLabel({ title: "feat: x", changedPaths: ["src/a.ts"], contentGlobs: [] })).toBe(DEFAULT_TYPE_LABELS.feature); + expect(resolvePrTypeLabel({ title: "fix: y", changedPaths: ["src/a.ts"], contentGlobs: [] })).toBe(DEFAULT_TYPE_LABELS.bug); + }); + + it("returns priority when a changed path matches a content glob (content submission)", () => { + expect(resolvePrTypeLabel({ title: "feat: add entry", changedPaths: ["content/posts/x.md"], contentGlobs: ["content/**"] })).toBe(DEFAULT_TYPE_LABELS.priority); + // a non-content feat with content globs configured but no match → feature + expect(resolvePrTypeLabel({ title: "feat: code", changedPaths: ["src/a.ts"], contentGlobs: ["content/**"] })).toBe(DEFAULT_TYPE_LABELS.feature); + }); +}); diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index f836697952..9fc3972048 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -3487,7 +3487,8 @@ describe("queue processors", () => { }, }); - expect(calls).toEqual({ comments: 0, labels: 2, minerList: 1 }); + // 2 PRs × 3 label POSTs each: the gittensor context label (apply) + the per-PR TYPE label (create + apply). + expect(calls).toEqual({ comments: 0, labels: 6, minerList: 1 }); const cacheAudit = await env.DB.prepare("select event_type, detail from audit_events where actor = ? order by created_at") .bind("oktofeesh1") .all<{ event_type: string; detail: string | null }>(); @@ -3558,7 +3559,9 @@ describe("queue processors", () => { }), ).resolves.toBeUndefined(); - expect(calls).toEqual({ comments: 0, labels: 1 }); + // gittensor context-label apply (fails 503, recorded) + the best-effort type-label create attempt (also 503, + // swallowed). The context-label failure is still recorded below; the type label never drops the recording. + expect(calls).toEqual({ comments: 0, labels: 2 }); const outputFailure = await env.DB.prepare("select event_type, detail from audit_events where event_type = ?") .bind("github_app.pr_label_publish_failed") .first<{ event_type: string; detail: string }>(); @@ -3851,7 +3854,8 @@ describe("queue processors", () => { }, }); - expect(calls).toEqual({ minerList: 2, labels: 1 }); + // 1 labeled PR × 3 label POSTs: the gittensor context label (apply) + the per-PR TYPE label (create + apply). + expect(calls).toEqual({ minerList: 2, labels: 3 }); const cached = await env.DB.prepare("select status, snapshot_json from official_miner_detections where login = ?") .bind("oktofeesh1") .first<{ status: string; snapshot_json: string }>();