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 09adbbe438..6af5023af7 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); @@ -3897,6 +3927,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 f6a6e567b9..a7fb0dee67 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"; @@ -614,14 +617,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 = changedPathsForGuardrail(changedFiles); const repoOwner = repoFullName.includes("/") ? repoFullName.slice(0, repoFullName.indexOf("/")) : ""; const authorLogin = pr.authorLogin ?? ""; @@ -642,10 +653,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; @@ -664,6 +677,7 @@ async function maybeRunAgentMaintenance( agentPaused: settings.agentPaused, agentDryRun: settings.agentDryRun, installationPermissions, + authorLogin: pr.authorLogin, }, planned, ); @@ -679,6 +693,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); @@ -2017,28 +2051,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) { @@ -2144,7 +2198,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/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/review/unified-comment.ts b/src/review/unified-comment.ts index 88879fcdea..cca3f8c279 100644 --- a/src/review/unified-comment.ts +++ b/src/review/unified-comment.ts @@ -431,7 +431,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( @@ -439,7 +438,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/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/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 1fdb8c7db3..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, 1_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/queue.test.ts b/test/unit/queue.test.ts index 68c6b49690..fb226441f0 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -1623,15 +1623,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 3f9e7b0721..eaebce1333 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -101,9 +101,17 @@ 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 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(checkboxLine!.startsWith(">")).toBe(false); + // The colored sidebar — every non-empty line above the checkbox — IS blockquote-wrapped. + 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", () => { diff --git a/wrangler.jsonc b/wrangler.jsonc index 0ca0bbf5a5..8c5cb9e42b 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 — @@ -79,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