From 31648de4f7df50d1579d03baa1804f7552fc7399 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Mon, 29 Jun 2026 15:43:21 -0700 Subject: [PATCH] fix(review): enforce blocker-first PR verdicts --- ...4_pull_request_linked_issue_claimed_at.sql | 7 ++ src/db/repositories.ts | 32 ++++- src/db/schema.ts | 1 + src/github/backfill.ts | 119 ++++++++++++++---- src/queue/processors.ts | 97 +++++++++----- src/review/content-lane-wire.ts | 5 +- src/review/grounding-wire.ts | 8 +- src/review/guardrail-config.ts | 7 +- src/review/review-thread-findings.ts | 97 ++++++++++++++ src/review/unified-comment.ts | 7 +- src/rules/advisory.ts | 115 +++++------------ src/rules/predicted-gate.ts | 4 +- src/settings/agent-actions.ts | 73 ++++------- src/signals/duplicate-winner.ts | 42 ++++++- src/signals/engine.ts | 44 ++++--- src/types.ts | 15 ++- test/unit/agent-actions.test.ts | 66 ++++------ test/unit/backfill.test.ts | 102 ++++++++++++++- test/unit/content-lane-wire.test.ts | 2 +- test/unit/duplicate-winner.test.ts | 64 +++++++++- test/unit/gate-check-policy.test.ts | 62 +++++---- test/unit/grounding-wiring.test.ts | 2 +- test/unit/maintainer-activation.test.ts | 17 ++- test/unit/predicted-gate.test.ts | 20 +-- test/unit/queue.test.ts | 1 + test/unit/rules.test.ts | 30 ++--- test/unit/signals-coverage.test.ts | 16 ++- test/unit/unified-comment.test.ts | 11 +- 28 files changed, 710 insertions(+), 356 deletions(-) create mode 100644 migrations/0084_pull_request_linked_issue_claimed_at.sql create mode 100644 src/review/review-thread-findings.ts diff --git a/migrations/0084_pull_request_linked_issue_claimed_at.sql b/migrations/0084_pull_request_linked_issue_claimed_at.sql new file mode 100644 index 0000000000..3163690505 --- /dev/null +++ b/migrations/0084_pull_request_linked_issue_claimed_at.sql @@ -0,0 +1,7 @@ +ALTER TABLE pull_requests ADD COLUMN linked_issue_claimed_at TEXT; + +UPDATE pull_requests +SET linked_issue_claimed_at = COALESCE(json_extract(payload_json, '$.updated_at'), updated_at, created_at) +WHERE linked_issues_json IS NOT NULL + AND linked_issues_json != '[]' + AND linked_issues_json != ''; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index e62eda7d05..7771512266 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -304,7 +304,21 @@ export async function upsertPullRequestFromGitHub( ): Promise { const record = toPullRequestRecord(repoFullName, pr); const db = getDb(env.DB); - const lastSeenOpenAt = pr.state === "open" ? (options.seenOpenAt ?? nowIso()) : null; + const syncedAt = nowIso(); + const lastSeenOpenAt = pr.state === "open" ? (options.seenOpenAt ?? syncedAt) : null; + const linkedIssuesJson = jsonString(record.linkedIssues); + const observedLinkedIssueClaimedAt = record.linkedIssues.length > 0 ? syncedAt : null; + const existingClaimRows = await db + .select({ linkedIssuesJson: pullRequests.linkedIssuesJson, linkedIssueClaimedAt: pullRequests.linkedIssueClaimedAt }) + .from(pullRequests) + .where(and(eq(pullRequests.repoFullName, repoFullName), eq(pullRequests.number, pr.number))) + .limit(1); + const linkedIssueClaimedAt = + record.linkedIssues.length === 0 + ? null + : existingClaimRows[0]?.linkedIssuesJson === linkedIssuesJson + ? (existingClaimRows[0].linkedIssueClaimedAt ?? observedLinkedIssueClaimedAt) + : observedLinkedIssueClaimedAt; await db .insert(pullRequests) .values({ @@ -321,10 +335,11 @@ export async function upsertPullRequestFromGitHub( mergedAt: pr.merged_at ?? undefined, htmlUrl: pr.html_url, labelsJson: jsonString(record.labels), - linkedIssuesJson: jsonString(record.linkedIssues), + linkedIssuesJson, + linkedIssueClaimedAt, lastSeenOpenAt, payloadJson: jsonString(compactGitHubPayload(pr)), - updatedAt: nowIso(), + updatedAt: syncedAt, }) .onConflictDoUpdate({ target: [pullRequests.repoFullName, pullRequests.number], @@ -339,13 +354,17 @@ export async function upsertPullRequestFromGitHub( mergedAt: pr.merged_at ?? undefined, htmlUrl: pr.html_url, labelsJson: jsonString(record.labels), - linkedIssuesJson: jsonString(record.linkedIssues), + linkedIssuesJson, + linkedIssueClaimedAt: + record.linkedIssues.length === 0 + ? null + : sql`CASE WHEN ${pullRequests.linkedIssuesJson} = ${linkedIssuesJson} THEN COALESCE(${pullRequests.linkedIssueClaimedAt}, ${observedLinkedIssueClaimedAt}) ELSE ${observedLinkedIssueClaimedAt} END`, lastSeenOpenAt, payloadJson: jsonString(compactGitHubPayload(pr)), - updatedAt: nowIso(), + updatedAt: syncedAt, }, }); - return record; + return { ...record, linkedIssueClaimedAt }; } export async function upsertIssueFromGitHub(env: Env, repoFullName: string, issue: GitHubIssuePayload, options: { seenOpenAt?: string } = {}): Promise { @@ -4201,6 +4220,7 @@ function toPullRequestRecordFromRow(row: typeof pullRequests.$inferSelect): Pull createdAt: payload.created_at, updatedAt: payload.updated_at ?? row.updatedAt, closedAt: payload.closed_at, + linkedIssueClaimedAt: row.linkedIssueClaimedAt, labels: parseJson(row.labelsJson, []), linkedIssues: parseJson(row.linkedIssuesJson, []), slopRisk: row.slopRisk, diff --git a/src/db/schema.ts b/src/db/schema.ts index ac475f4d7c..a3b6f4e1c3 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -283,6 +283,7 @@ export const pullRequests = sqliteTable( htmlUrl: text("html_url"), labelsJson: text("labels_json").notNull().default("[]"), linkedIssuesJson: text("linked_issues_json").notNull().default("[]"), + linkedIssueClaimedAt: text("linked_issue_claimed_at"), lastSeenOpenAt: text("last_seen_open_at"), payloadJson: text("payload_json").notNull().default("{}"), // Latest deterministic slop assessment (gittensory-computed; written separately from the GitHub sync). diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 545f8e8d04..00f76b2003 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -65,6 +65,7 @@ import { GITTENSORY_GATE_CHECK_NAME, GITTENSORY_LEGACY_GATE_CHECK_NAME, } from "../review/check-names"; +import { buildReviewThreadBlocker, type ReviewThreadBlocker } from "../review/review-thread-findings"; import { delayUntil, shouldWaitForGitHubRateLimit } from "./rate-limit"; type GitHubLabelPayload = { @@ -1943,13 +1944,10 @@ export type LiveCiAggregate = { // than ciState: a non-required pending check must not fail the gate, but review execution should still wait // until every visible CI signal has settled. hasPending: boolean; - // 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). + // Checks that FAIL the gate. Any completed red check/status is adverse, required or not; required contexts are + // still used for absent/pending detection so missing required CI cannot silently pass. 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). + // Historical compatibility: non-required red checks are now folded into failingDetails so this stays empty. nonRequiredFailingDetails: Array<{ name: string; summary?: string; detailsUrl?: string }>; }; @@ -1985,20 +1983,17 @@ export async function fetchRequiredStatusContexts(env: Env, repoFullName: string * reviewbot `getAllChecksState` parity that the converged auto-maintain path needs: codecov (codecov/patch, * codecov/project) and many other tools post a classic COMMIT-STATUS, not a check-run — fetching only * `/check-runs` (what the backfill sync does) misses them entirely, which is why a red codecov was reported as - * "CI green". When branch-protection required contexts are known, only those trusted contexts gate review or - * automation; non-required failures are reported separately. When required contexts cannot be determined, we - * conservatively fold all checks/statuses into the gate so required red checks are not silently ignored. - * Best-effort: a fetch error degrades that source to empty. + * "CI green". Any completed red check/status is adverse and fails the aggregate, required or not. Branch + * protection contexts are still used for required-context absence/pending detection. 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, - // Branch-protection REQUIRED contexts are the trust boundary for CI gate authority. Non-required checks may be - // influenced by PR authors or third-party actors, so they are surfaced as advisory details but must not defer - // review, fail the merge gate, or drive automated close decisions. If required contexts are unavailable, fall - // back to gating on all contexts to avoid silently passing an unknown required failure. + // Branch-protection REQUIRED contexts are the trust boundary for required-context absence/pending detection. + // Completed red checks/statuses still fail the aggregate even when they are not branch-protection-required. requiredContexts?: ReadonlySet | null, ): Promise { if (!headSha) return { ciState: "unverified", hasPending: false, failingDetails: [], nonRequiredFailingDetails: [] }; @@ -2046,8 +2041,7 @@ export async function fetchLiveCiAggregate( 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); - const detail = { name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) }; - (isRequired(run.name) ? failingDetails : nonRequiredFailingDetails).push(detail); + failingDetails.push({ name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) }); } else if (conclusion ? CI_PASSING_CONCLUSIONS.has(conclusion) : status === "completed") { // concluded and not failing → passing } else { @@ -2085,8 +2079,7 @@ export async function fetchLiveCiAggregate( const state = (ctx.state ?? "").toLowerCase(); if (state === "failure" || state === "error") { const summary = typeof ctx.description === "string" ? ctx.description.trim().slice(0, 200) : ""; - const detail = { name, ...(summary ? { summary } : {}), ...(ctx.target_url ? { detailsUrl: ctx.target_url } : {}) }; - (isRequired(name) ? failingDetails : nonRequiredFailingDetails).push(detail); + failingDetails.push({ name, ...(summary ? { summary } : {}), ...(ctx.target_url ? { detailsUrl: ctx.target_url } : {}) }); } else if (state === "success") { // passing } else { @@ -2136,9 +2129,8 @@ export async function fetchLiveCiAggregate( } } - // 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. + // ciState reflects every completed red check/status. Required contexts additionally prevent absent or pending + // required checks from being treated as passed. let ciState: LiveCiAggregate["ciState"] = failingDetails.length > 0 ? "failed" : anyPending ? "pending" : total > 0 ? "passed" : "unverified"; // Fail CLOSED on incomplete visibility: if either CI source could not be fully read, we cannot certify the // commit as passed/clean — hold (pending) so the gate waits and re-evaluates on the next sweep instead of @@ -2231,6 +2223,91 @@ export async function fetchLivePullRequestReviewDecision(env: Env, repoFullName: return result?.data?.repository?.pullRequest?.reviewDecision ?? undefined; } +type GitHubReviewThreadResponse = { + data?: { + repository?: { + pullRequest?: { + reviewThreads?: { + nodes?: Array<{ + isResolved?: boolean | null; + isOutdated?: boolean | null; + path?: string | null; + line?: number | null; + comments?: { + nodes?: Array<{ + body?: string | null; + url?: string | null; + author?: { login?: string | null } | null; + } | null> | null; + } | null; + } | null> | null; + } | null; + } | null; + } | null; + }; +}; + +/** Fetch unresolved GitHub review threads that should block merge readiness. GraphQL is required because REST + * review comments do not expose thread resolution; if GraphQL is unavailable this fails open to [] rather than + * guessing. Own gittensory-authored inline-comment threads are ignored so the bot never blocks on itself. */ +export async function fetchLiveReviewThreadBlockers(env: Env, repoFullName: string, prNumber: number, token: string | undefined): Promise { + if (!token) return []; + const [owner, name] = repoFullName.split("/"); + if (!owner || !name) return []; + const query = `query GittensoryPullRequestReviewThreads { + repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { + pullRequest(number: ${prNumber}) { + reviewThreads(first: 50) { + nodes { + isResolved + isOutdated + path + line + comments(first: 20) { + nodes { + body + url + author { login } + } + } + } + } + } + } + }`; + const result = await githubGraphQl(env, query, token).catch(() => undefined); + const threads = result?.data?.repository?.pullRequest?.reviewThreads?.nodes; + if (!threads) return []; + const blockers: ReviewThreadBlocker[] = []; + for (const thread of threads) { + if (!thread || thread.isResolved !== false || thread.isOutdated === true) continue; + const comments = (thread.comments?.nodes ?? []) + .flatMap((comment) => + comment + ? [ + { + body: comment.body, + url: comment.url, + authorLogin: comment.author?.login, + }, + ] + : [], + ) + .filter((comment) => !isOwnReviewThreadAuthor(comment.authorLogin)); + const blocker = buildReviewThreadBlocker({ + path: thread.path, + line: thread.line, + comments, + }); + if (blocker) blockers.push(blocker); + } + return blockers; +} + +function isOwnReviewThreadAuthor(login: string | null | undefined): boolean { + return /\bgittensory[-\w]*\[bot\]$/i.test(login ?? "") || /^(gittensory|gittensory-orb)$/i.test(login ?? ""); +} + /** The deterministic linked-issue facts the hard-rule evaluator needs (labels / assignees / open-state). */ export type LinkedIssueFactsResult = { number: number; labels: string[]; assignees: string[]; state: string; authorLogin: string | null }; diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 9b2b97d3d7..07ffcb2af4 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -79,6 +79,7 @@ import { fetchLivePullRequestHeadSha, fetchLivePullRequestMergeState, fetchLivePullRequestReviewDecision, + fetchLiveReviewThreadBlockers, fetchLivePullRequestState, fetchOpenPullRequestNumbersForCommit, fetchRequiredStatusContexts, @@ -262,7 +263,7 @@ import { unionScopedOverlapClusters, type ContributorProfile, } from "../signals/engine"; -import { isDuplicateClusterWinner } from "../signals/duplicate-winner"; +import { isDuplicateClusterWinnerByClaim } from "../signals/duplicate-winner"; import { buildUnifiedReviewDiff } from "../review/review-diff"; import { buildUnifiedCommentBody } from "../review/unified-comment-bridge"; import { isRetryableJobError, RetryableJobError } from "./retryable"; @@ -320,7 +321,6 @@ import { isPlannerEnabled, } from "../review/planner"; import { - aiCiRefutationActive, buildReviewGroundingText, checkSummaryText as checkFailureSummaryText, isGroundingEnabled, @@ -333,6 +333,7 @@ import { } from "../review/enrichment-wire"; import { captureReviewFailure } from "../selfhost/sentry"; import { evaluateWithSurfaceLane } from "../review/content-lane-wire"; +import { reviewThreadBlockerFinding } from "../review/review-thread-findings"; import { indexRepo, reindexChangedPaths } from "../review/rag-index"; import { isReputationEnabled, @@ -1204,10 +1205,9 @@ export function applyPrecisionBreakers( } /** - * #1177: a red CI may bypass the hard-guardrail manual hold (#ci-fail-closes-guarded) ONLY when we proved the - * failing checks are required branch-protection contexts. A null fetch (unreadable branch protection) or an - * EMPTY required set means `fetchLiveCiAggregate` may have folded an optional / third-party red into `failed`, - * which must keep a guarded PR held for a human rather than auto-close it. + * Historical compatibility helper for callers/tests that still need to know whether branch-protection contexts + * were readable. The disposition planner no longer uses this to soften red CI: any visible completed red + * check/status is adverse, while required contexts still matter for missing/pending detection. */ export function hasVerifiedRequiredContexts( requiredContexts: Set | null, @@ -1364,15 +1364,9 @@ async function maybeRunAgentMaintenance( const planned = planAgentMaintenanceActions({ conclusion: gate.conclusion, blockerTitles: gate.blockers.map((blocker) => blocker.title), - // CI-refutation (#ai-ci-refutation): thread the blocker CODES + the active gate so the planner suppresses an - // AI-judgment-only failure (ai_consensus_defect / ai_review_split) on a green-CI PR — the deterministic - // validator overrules the model hallucination, so a clean+green PR MERGES instead of being false-closed. - // `aiCiRefutationEnabled` is the SAME grounding+convergence gate the public-comment reconciliation uses, passed - // as a single boolean so the refutation condition is unit-tested in the planner and this site carries no branch. - // Enabled=false (non-convergence / grounding-off) ⇒ the refutation is a no-op ⇒ byte-identical verdict. The - // codes are public-safe finding identifiers (no rubric/scoring/reward terms). + // Public-safe finding identifiers retained for telemetry/action reasons. They no longer refute a blocker on + // green CI; once the gate says failure, the close/hold decision follows that verdict. gateBlockerCodes: gate.blockers.map((blocker) => blocker.code), - aiCiRefutationEnabled: aiCiRefutationActive(env, repoFullName), autonomy: settings.autonomy, autoMaintain: settings.autoMaintain, slopGateMinScore: settings.slopGateMinScore, @@ -1405,10 +1399,11 @@ async function maybeRunAgentMaintenance( // reason ("duplicate of another open PR" via agent-actions when count > 0). When the flag is ON and this // PR is the cluster winner, force the count to 0 so the winner's close reason OMITS the duplicate cause // (it can still close on its own merits — CI/conflict/blockers). Flag-OFF short-circuits ⇒ the real - // count is used (byte-identical). The sibling numbers are open-only, so the lowest is the open winner. + // count is used (byte-identical). Unknown claim time keeps the duplicate cause. linkedDuplicateCount: dupWinnerLinkedDuplicateCount( - linkedIssueDuplicatePullRequestsForGate(pr, otherOpenPullRequests), + linkedIssueDuplicatePullRequestRecordsForGate(pr, otherOpenPullRequests), pr.number, + pr.linkedIssueClaimedAt, env.GITTENSORY_DUPLICATE_WINNER === "true", ), headSha: pr.headSha, @@ -3050,7 +3045,7 @@ async function processGitHubWebhook( } if ( installationId && - shouldProcessPullRequestPublicSurface(payload.action) + shouldProcessPullRequestPublicSurface(eventName, payload.action) ) { if ( shouldCollectSlopEvidence(settings) || @@ -3373,8 +3368,18 @@ export function shouldRunSlopAiAdvisory( } function shouldProcessPullRequestPublicSurface( + eventName: string, action: string | undefined, ): boolean { + if (eventName === "pull_request_review_comment") { + return action === "created" || action === "edited" || action === "deleted"; + } + if (eventName === "pull_request_review_thread") { + return action === "resolved" || action === "unresolved"; + } + if (eventName === "pull_request_review") { + return action === "submitted" || action === "edited" || action === "dismissed"; + } return ( PR_PUBLIC_SURFACE_ACTIONS.has(action ?? "") || PR_GATE_CLOSED_ACTIONS.has(action ?? "") @@ -3421,7 +3426,7 @@ export function gateCheckPolicy( slopRisk: slopRisk ?? null, confirmedContributor: confirmedContributorForPack, // PR-size + guardrail manual-review HOLD (#gate-size / #gate-guardrail): the MODE comes from config; the - // thresholds default to 10 files / 500 lines (advisory.ts constants); the live counts + guardrail-hit come from + // thresholds default to 10 files / 1000 lines (advisory.ts constants); the live counts + guardrail-hit come from // the per-PR sizeContext threaded by the caller. sizeGateMode: settings.sizeGateMode, changedFileCount: sizeContext?.changedFileCount ?? null, @@ -3699,7 +3704,7 @@ export async function runAiReviewForAdvisory( // block, falling back to the `convergedRepoAllowed` allowlist when unset (byte-identical default). The (cached) // manifest is loaded once and shared, and ONLY when at least one of the two features is globally enabled — so a // deploy with both flags off does no extra read (preserves the no-op default). Grounding deliberately stays on - // `convergedRepoAllowed` here so it remains coherent with the disposition-side CI-refutation gate (#deferred). + // `convergedRepoAllowed` here so prompt grounding remains tied to the converged review allowlist. const featureManifest = isReputationEnabled(env) || isRagEnabled(env) ? await loadRepoFocusManifest(env, args.repoFullName).catch(() => null) @@ -4113,16 +4118,17 @@ export async function runAiSlopForAdvisory( * when count > 0). Flag-OFF (default) returns the real sibling count — byte-identical to today. */ export function dupWinnerLinkedDuplicateCount( - openSiblingNumbers: number[], + openSiblings: Pick[], prNumber: number, + linkedIssueClaimedAt: string | null | undefined, duplicateWinnerEnabled: boolean, ): number { if ( duplicateWinnerEnabled && - isDuplicateClusterWinner(prNumber, openSiblingNumbers) + isDuplicateClusterWinnerByClaim({ number: prNumber, linkedIssueClaimedAt }, openSiblings) ) return 0; - return openSiblingNumbers.length; + return openSiblings.length; } /** @@ -4185,18 +4191,23 @@ export function linkedIssueDuplicatePullRequestsForGate( pr: PullRequestRecord, pullRequests: PullRequestRecord[], ): number[] { + return linkedIssueDuplicatePullRequestRecordsForGate(pr, pullRequests).map((otherPr) => otherPr.number); +} + +export function linkedIssueDuplicatePullRequestRecordsForGate( + pr: PullRequestRecord, + pullRequests: PullRequestRecord[], +): PullRequestRecord[] { const linkedIssues = new Set(pr.linkedIssues); if (linkedIssues.size === 0) return []; return [ - ...new Set( + ...new Map( pullRequests.flatMap((otherPr) => { if (otherPr.number === pr.number || otherPr.state !== "open") return []; - return otherPr.linkedIssues.some((issue) => linkedIssues.has(issue)) - ? [otherPr.number] - : []; + return otherPr.linkedIssues.some((issue) => linkedIssues.has(issue)) ? [[otherPr.number, otherPr] as const] : []; }), - ), - ].sort((left, right) => left - right); + ).values(), + ].sort((left, right) => left.number - right.number); } async function auditGateCheckPermissionMissing( @@ -4514,19 +4525,19 @@ async function maybePublishPrPublicSurface( // penalty (below), and the public panel builders (further down) so they agree by construction. Flag-OFF // (default) ⇒ duplicateWinnerEnabled is false and isDupWinner is false ⇒ every guard short-circuits // (byte-identical). - const linkedDuplicatePrsForGate = linkedIssueDuplicatePullRequestsForGate( + const linkedDuplicatePrsForGate = linkedIssueDuplicatePullRequestRecordsForGate( pr, repoPullRequests, ); const duplicateWinnerEnabled = env.GITTENSORY_DUPLICATE_WINNER === "true"; const isDupWinner = duplicateWinnerEnabled && - isDuplicateClusterWinner(pr.number, linkedDuplicatePrsForGate); + isDuplicateClusterWinnerByClaim(pr, linkedDuplicatePrsForGate); const readiness = buildPublicReadinessScore({ pr, preflight, queueHealth, - linkedDuplicatePrs: isDupWinner ? [] : linkedDuplicatePrsForGate, + linkedDuplicatePrs: isDupWinner ? [] : linkedDuplicatePrsForGate.map((otherPr) => otherPr.number), scopedOverlapCount: unionScopedOverlapClusters( collisions, pr, @@ -4827,6 +4838,24 @@ async function maybePublishPrPublicSurface( files: await getReviewFiles(), }); + // Unresolved GitHub review threads (for example external security scanner inline findings) are blocking + // review facts. Fetch them before gate evaluation so the normal blocker path drives the check-run, comment, + // and disposition consistently. Fail-open on GitHub/GraphQL errors: a transient thread-read failure should not + // invent a blocker, but any thread we can see must be resolved before approval/merge. + if (gateEnabled) { + const reviewThreadToken = + (await createInstallationToken(env, installationId).catch( + () => undefined, + )) ?? env.GITHUB_PUBLIC_TOKEN; + const reviewThreadBlockers = await fetchLiveReviewThreadBlockers( + env, + repoFullName, + pr.number, + reviewThreadToken, + ).catch(() => []); + advisory.findings.push(...reviewThreadBlockers.map(reviewThreadBlockerFinding)); + } + // First-time-contributor grace (#552): compute the author's complete per-repo PR history // (excluding this PR) with an aggregate DB query. Do not derive policy-enforcement history from // the bounded repoPullRequests sample; missing or case-mismatched history could soften a block. @@ -5172,7 +5201,8 @@ async function maybePublishPrPublicSurface( const ciToken = await createInstallationToken(env, installationId).catch( () => undefined, ); - // RC2: only branch-protection-required checks gate the PR; a red codecov/* is surfaced but never blocks. + // Required contexts still detect missing/pending required CI, but every visible completed red check/status is + // adverse and blocks the PR. const requiredContexts = await fetchRequiredStatusContexts( env, repoFullName, @@ -5220,8 +5250,7 @@ async function maybePublishPrPublicSurface( : {}), ...(failingDetails.length > 0 ? { failingDetails } : {}), }; - // The public comment must match the authoritative Gate check-run conclusion. Planner-level CI refutation can - // affect auto-actions, but it must never recolor the review comment green while the posted Gate is red. + // The public comment must match the authoritative Gate check-run conclusion. const commentGate = gateEvaluation; // Observability (#reviews-dashboard): record the would-be gate verdict so the Grafana panel shows the // merge/close/hold mix — the "are we rubber-stamping?" signal — even in advisory/dryRun (this is the rendered verdict). diff --git a/src/review/content-lane-wire.ts b/src/review/content-lane-wire.ts index f990a88d3a..d0828ba5af 100644 --- a/src/review/content-lane-wire.ts +++ b/src/review/content-lane-wire.ts @@ -24,9 +24,8 @@ import { METAGRAPHED_LANE_SPEC } from "./content-lane/registry-logic"; import { isConvergenceRepoAllowed } from "./cutover-gate"; import { makeGithubFileFetcher } from "./grounding-wire"; -// Deterministic surface-lane finding codes. DELIBERATELY NOT in AI_JUDGMENT_BLOCKER_CODES — a deterministic -// surface close is a FACT, so the green-CI refutation (reconcileGateEvaluationForGreenCi) must never flip it to -// a merge. Regression-asserted in the test suite. +// Deterministic surface-lane finding codes. DELIBERATELY NOT in AI_JUDGMENT_BLOCKER_CODES; surface closes are +// facts, and blocker findings must never be flipped to merge by green CI. const SURFACE_REJECT_CODE = "surface_lane_reject"; const SURFACE_MANUAL_CODE = "surface_lane_manual"; const SURFACE_TITLE = "Registry surface review"; diff --git a/src/review/grounding-wire.ts b/src/review/grounding-wire.ts index 94b8c9950c..f8d5daf8dd 100644 --- a/src/review/grounding-wire.ts +++ b/src/review/grounding-wire.ts @@ -30,12 +30,8 @@ export function isGroundingEnabled(env: { GITTENSORY_REVIEW_GROUNDING?: string | return /^(1|true|yes|on)$/i.test(env.GITTENSORY_REVIEW_GROUNDING ?? ""); } -/** True when the AI CI-refutation (#ai-ci-refutation) is ACTIVE for this repo: grounding is ON (the converged AI - * review feeds the finished CI status to the reviewer, so enforcing that ground truth is coherent) AND the repo - * is convergence-allowlisted. Centralized so the disposition refutation (agent-actions) and the public-comment - * reconciliation gate on the SAME condition — the merge/close action and the rendered comment can never disagree. - * A single call (not an inline `&&` at the call sites) so the processor carries no branch and this is the one - * place the condition is unit-tested. */ +/** Historical compatibility helper for the removed AI CI-refutation path. Grounding still feeds CI/full-file truth + * into the reviewer prompt, but green CI no longer rewrites a configured AI blocker into success. */ export function aiCiRefutationActive(env: Env, repoFullName: string): boolean { return isGroundingEnabled(env) && isConvergenceRepoAllowed(env, repoFullName); } diff --git a/src/review/guardrail-config.ts b/src/review/guardrail-config.ts index e58e767813..3236e8d3b5 100644 --- a/src/review/guardrail-config.ts +++ b/src/review/guardrail-config.ts @@ -1,4 +1,5 @@ -// Per-repo hard-guardrail path globs (paths that force MANUAL review — no auto-merge / no auto-close). +// Per-repo hard-guardrail path globs. These force MANUAL review only for otherwise-ready PRs: no auto-merge / +// no auto-approve, but blockers, red CI, and base conflicts still close for close-eligible contributors. // // Self-host note: hosted reviews and hosted policy storage are retired. Review execution uses the container-private // `.gittensory.yml` path for repo policy; these hard guardrails remain built-in invariants so a missing private @@ -7,7 +8,7 @@ export const DEFAULT_CRUCIAL_GUARDRAIL_GLOBS = [".github/workflows/**", "scripts // The gate's OWN policy files, guarded for EVERY repo regardless of private config. A PR that edits the // config-as-code that defines the gate or coverage policy (the `.gittensory.*` focus manifest the loader -// reads, or `codecov.yml`) must always be HELD for the owner — otherwise one auto-merged config-only PR +// reads, or `codecov.yml`) must always be HELD when otherwise-ready — otherwise one auto-merged config-only PR // could weaken the gate repo-wide before any subsequent PR is evaluated against the new policy. The // manifest filenames mirror signals/focus-manifest-loader's candidates; this only ever WIDENS the guard. export const CONFIG_AS_CODE_GUARDRAIL_GLOBS = [ @@ -25,7 +26,7 @@ export const CONFIG_AS_CODE_GUARDRAIL_GLOBS = [ // The review engine's OWN decision + safety code — its crown jewels — guarded for EVERY repo regardless of // private config. A contributor PR that edits how the gate decides a verdict, how a merge or close executes, the // action-mode kill-switch, scoring, auth, the CI aggregate the gate reads, or the guardrail itself must be HELD -// for the owner: the engine must never auto-merge a change to the very code that governs its own autonomy. +// when otherwise-ready: the engine must never auto-merge a change to the very code that governs its own autonomy. // These are gittensory engine-specific paths, so they never match an unrelated reviewed repo's PR (e.g. // metagraphed has no src/rules/** or agent-action-executor.ts); like the config-as-code set above, this only // ever WIDENS the guard. diff --git a/src/review/review-thread-findings.ts b/src/review/review-thread-findings.ts new file mode 100644 index 0000000000..42eb2edd95 --- /dev/null +++ b/src/review/review-thread-findings.ts @@ -0,0 +1,97 @@ +import type { AdvisoryFinding } from "../types"; + +export const REVIEW_THREAD_BLOCKER_CODE = "review_thread_unresolved"; + +const SCANNER_FINDING_MARKER = /\n**P1:** PUBLIC_LOCAL_PATH_INLINE regex fails to match Windows backslash paths", + url: "https://github.example/thread", + author: { login: "superagent-security[bot]" }, + }, + ], + }, + }, + ], + }, + }, + }, + }, + }); + } + return new Response("not found", { status: 404 }); + }); + + const blockers = await fetchLiveReviewThreadBlockers(env, "JSONbored/gittensory", 1748, "public-token"); + + expect(blockers).toEqual([ + expect.objectContaining({ + title: "PUBLIC_LOCAL_PATH_INLINE regex fails to match Windows backslash paths", + priority: "P1", + path: "src/signals/redaction.ts", + line: 30, + authorLogin: "superagent-security[bot]", + url: "https://github.example/thread", + scannerFinding: true, + }), + ]); + }); + + it("ignores resolved, outdated, own-bot, and empty review threads", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + if (input.toString() === "https://api.github.com/graphql") { + return Response.json({ + data: { + repository: { + pullRequest: { + reviewThreads: { + nodes: [ + { isResolved: true, isOutdated: false, path: "a.ts", line: 1, comments: { nodes: [{ body: "resolved", author: { login: "scanner[bot]" } }] } }, + { isResolved: false, isOutdated: true, path: "b.ts", line: 2, comments: { nodes: [{ body: "outdated", author: { login: "scanner[bot]" } }] } }, + { isResolved: false, isOutdated: false, path: "c.ts", line: 3, comments: { nodes: [{ body: "own bot", author: { login: "gittensory-orb[bot]" } }] } }, + { isResolved: false, isOutdated: false, path: "d.ts", line: 4, comments: { nodes: [{ body: " ", author: { login: "scanner[bot]" } }, null] } }, + null, + ], + }, + }, + }, + }, + }); + } + return new Response("not found", { status: 404 }); + }); + + await expect(fetchLiveReviewThreadBlockers(env, "JSONbored/gittensory", 1, "public-token")).resolves.toEqual([]); + }); + + it("fails open without a token, malformed repo name, or GraphQL response", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + const fetchSpy = vi.fn(async () => new Response("boom", { status: 500 })); + vi.stubGlobal("fetch", fetchSpy); + + await expect(fetchLiveReviewThreadBlockers(env, "JSONbored/gittensory", 1, undefined)).resolves.toEqual([]); + expect(fetchSpy).not.toHaveBeenCalled(); + await expect(fetchLiveReviewThreadBlockers(env, "malformed", 1, "public-token")).resolves.toEqual([]); + expect(fetchSpy).not.toHaveBeenCalled(); + await expect(fetchLiveReviewThreadBlockers(env, "JSONbored/gittensory", 1, "public-token")).resolves.toEqual([]); + expect(fetchSpy).toHaveBeenCalledTimes(1); + }); + }); + describe("fetchRequiredStatusContexts", () => { it("returns null without fetching when baseRef is missing", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); diff --git a/test/unit/content-lane-wire.test.ts b/test/unit/content-lane-wire.test.ts index adf7b388a5..635bdd0693 100644 --- a/test/unit/content-lane-wire.test.ts +++ b/test/unit/content-lane-wire.test.ts @@ -41,7 +41,7 @@ describe("surfaceVerdictToGate", () => { expect(evaluation.blockers).toHaveLength(1); expect(evaluation.blockers[0]?.severity).toBe("critical"); expect(finding?.code).toBe("surface_lane_reject"); - // Regression guard: a deterministic surface close must never be refutable by green CI. + // Regression guard: deterministic surface blockers remain outside AI-judgment telemetry. expect(AI_JUDGMENT_BLOCKER_CODES.has(evaluation.blockers[0]!.code)).toBe(false); }); diff --git a/test/unit/duplicate-winner.test.ts b/test/unit/duplicate-winner.test.ts index 4ba49829d8..65c042cb30 100644 --- a/test/unit/duplicate-winner.test.ts +++ b/test/unit/duplicate-winner.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { isDuplicateClusterWinner } from "../../src/signals/duplicate-winner"; +import { isDuplicateClusterWinner, isDuplicateClusterWinnerByClaim } from "../../src/signals/duplicate-winner"; import { dupWinnerLinkedDuplicateCount, linkedIssueDuplicatePullRequestsForGate } from "../../src/queue/processors"; import type { PullRequestRecord } from "../../src/types"; import { listOtherOpenPullRequests, upsertPullRequestFromGitHub } from "../../src/db/repositories"; @@ -35,22 +35,74 @@ describe("isDuplicateClusterWinner (#dup-winner)", () => { }); }); +describe("isDuplicateClusterWinnerByClaim (#dup-winner claim election)", () => { + const claim = (number: number, linkedIssueClaimedAt: string | null) => ({ number, linkedIssueClaimedAt }); + + it("elects the earliest observed linked-issue claimant, not the lowest PR number", () => { + expect(isDuplicateClusterWinnerByClaim(claim(13, "2026-06-29T10:00:00.000Z"), [claim(12, "2026-06-29T10:05:00.000Z")])).toBe(true); + }); + + it("blocks an older PR that edits in the same issue after a newer PR already claimed it", () => { + expect(isDuplicateClusterWinnerByClaim(claim(12, "2026-06-29T10:05:00.000Z"), [claim(13, "2026-06-29T10:00:00.000Z")])).toBe(false); + }); + + it("falls back to PR number only for equal known claim timestamps", () => { + expect(isDuplicateClusterWinnerByClaim(claim(12, "2026-06-29T10:00:00.000Z"), [claim(13, "2026-06-29T10:00:00.000Z")])).toBe(true); + expect(isDuplicateClusterWinnerByClaim(claim(13, "2026-06-29T10:00:00.000Z"), [claim(12, "2026-06-29T10:00:00.000Z")])).toBe(false); + }); + + it("fails closed when any duplicate claimant has an unknown or invalid claim timestamp", () => { + expect(isDuplicateClusterWinnerByClaim(claim(12, null), [claim(13, "2026-06-29T10:00:00.000Z")])).toBe(false); + expect(isDuplicateClusterWinnerByClaim(claim(12, "2026-06-29T10:00:00.000Z"), [claim(13, "not-a-date")])).toBe(false); + }); +}); + describe("dupWinnerLinkedDuplicateCount (#dup-winner close-reason seam)", () => { it("winner + flag ON ⇒ 0 (close reason omits the duplicate cause)", () => { - expect(dupWinnerLinkedDuplicateCount([13, 14], 12, true)).toBe(0); + expect( + dupWinnerLinkedDuplicateCount( + [ + { number: 13, linkedIssueClaimedAt: "2026-06-29T10:01:00.000Z" }, + { number: 14, linkedIssueClaimedAt: "2026-06-29T10:02:00.000Z" }, + ], + 12, + "2026-06-29T10:00:00.000Z", + true, + ), + ).toBe(0); }); it("loser + flag ON ⇒ real sibling count (close reason includes the duplicate cause)", () => { - expect(dupWinnerLinkedDuplicateCount([12, 13], 14, true)).toBe(2); + expect( + dupWinnerLinkedDuplicateCount( + [ + { number: 12, linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z" }, + { number: 13, linkedIssueClaimedAt: "2026-06-29T10:01:00.000Z" }, + ], + 14, + "2026-06-29T10:02:00.000Z", + true, + ), + ).toBe(2); }); it("flag OFF ⇒ real sibling count even for a would-be winner (byte-identical)", () => { - expect(dupWinnerLinkedDuplicateCount([13, 14], 12, false)).toBe(2); + expect( + dupWinnerLinkedDuplicateCount( + [ + { number: 13, linkedIssueClaimedAt: "2026-06-29T10:01:00.000Z" }, + { number: 14, linkedIssueClaimedAt: "2026-06-29T10:02:00.000Z" }, + ], + 12, + "2026-06-29T10:00:00.000Z", + false, + ), + ).toBe(2); }); it("no siblings ⇒ 0 regardless of the flag", () => { - expect(dupWinnerLinkedDuplicateCount([], 12, true)).toBe(0); - expect(dupWinnerLinkedDuplicateCount([], 12, false)).toBe(0); + expect(dupWinnerLinkedDuplicateCount([], 12, "2026-06-29T10:00:00.000Z", true)).toBe(0); + expect(dupWinnerLinkedDuplicateCount([], 12, "2026-06-29T10:00:00.000Z", false)).toBe(0); }); }); diff --git a/test/unit/gate-check-policy.test.ts b/test/unit/gate-check-policy.test.ts index af9a5194d2..54b21fa7a3 100644 --- a/test/unit/gate-check-policy.test.ts +++ b/test/unit/gate-check-policy.test.ts @@ -5,6 +5,7 @@ import { buildAuthorizedPrActionAdvisory, gateCheckPolicy, resolveLinkedIssueAut import { createTestEnv } from "../helpers/d1"; import { upsertIssueFromGitHub, upsertRepositoryFromGitHub } from "../../src/db/repositories"; import { evaluateGateCheck } from "../../src/rules/advisory"; +import { REVIEW_THREAD_BLOCKER_CODE } from "../../src/review/review-thread-findings"; import { parseFocusManifest, resolveEffectiveSettings } from "../../src/signals/focus-manifest"; import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader"; import type { Advisory, PullRequestRecord, RepositorySettings } from "../../src/types"; @@ -87,11 +88,11 @@ describe(".gittensory.yml settings override (resolveEffectiveSettings)", () => { expect(evaluateGateCheck(missingIssueAdvisory(), gateCheckPolicy(eff, null, true)).conclusion).toBe("failure"); }); - it("end-to-end: manifest gate.firstTimeContributorGrace:true softens a newcomer's block to advisory (#822/#552)", () => { + it("end-to-end: manifest gate.firstTimeContributorGrace:true no longer softens a newcomer's blocker (#822/#552)", () => { const eff = resolveEffectiveSettings(settings({ linkedIssueGateMode: "block", firstTimeContributorGrace: false }), parseFocusManifest({ gate: { firstTimeContributorGrace: true } })); expect(eff.firstTimeContributorGrace).toBe(true); const newcomerPolicy = gateCheckPolicy(eff, null, true, null, { mergedPrCount: 0, closedUnmergedPrCount: 0 }); - expect(evaluateGateCheck(missingIssueAdvisory(), newcomerPolicy).conclusion).toBe("neutral"); + expect(evaluateGateCheck(missingIssueAdvisory(), newcomerPolicy).conclusion).toBe("failure"); }); it("blocks a non-confirmed contributor identically to a confirmed one (#gate-nonconfirmed)", () => { @@ -229,33 +230,30 @@ describe("AI close-confidence threshold gate (#7)", () => { expect(out.blockers.map((f) => f.code)).toEqual(["ai_consensus_defect"]); }); - it("holds for human review when mode=block but confidence < the floor (#7 regression)", () => { + it("blocks when mode=block even when confidence is below the configured floor (#7 regression)", () => { const out = evaluateGateCheck(aiDefectWith(0.92), gateCheckPolicy(settings({ aiReviewMode: "block" }), null, true)); - expect(out.conclusion).toBe("neutral"); // below 0.93 → manual hold, not an auto-mergeable pass - expect(out.title).toBe("Gittensory Orb Review Agent — held for human review"); - expect(out.blockers).toEqual([]); - expect(out.warnings.map((f) => f.code)).toContain("ai_consensus_defect"); + expect(out.conclusion).toBe("failure"); + expect(out.blockers.map((f) => f.code)).toEqual(["ai_consensus_defect"]); }); - it("blocks exactly AT the floor (>= boundary) and not just below it", () => { + it("threads a custom floor but does not use it to downgrade below-floor defects", () => { // policy.aiReviewCloseConfidence is threaded onto the policy from settings. const policy = gateCheckPolicy(settings({ aiReviewMode: "block", aiReviewCloseConfidence: 0.7 }), null, true); - expect(evaluateGateCheck(aiDefectWith(0.7), policy).conclusion).toBe("failure"); // == threshold → blocks - expect(evaluateGateCheck(aiDefectWith(0.69), policy).conclusion).toBe("neutral"); // just below → manual hold + expect(policy.aiReviewCloseConfidence).toBe(0.7); + expect(evaluateGateCheck(aiDefectWith(0.7), policy).conclusion).toBe("failure"); + expect(evaluateGateCheck(aiDefectWith(0.69), policy).conclusion).toBe("failure"); }); - it("honors a custom aiReviewCloseConfidence (the `?? 0.93` default is NOT used when set) (#7)", () => { - // A high custom floor of 0.99 holds a 0.95 defect for human review (the 0.93 default would have blocked it). + it("keeps custom aiReviewCloseConfidence as calibration context without softening blockers (#7)", () => { const strict = gateCheckPolicy(settings({ aiReviewMode: "block", aiReviewCloseConfidence: 0.99 }), null, true); - expect(evaluateGateCheck(aiDefectWith(0.95), strict).conclusion).toBe("neutral"); - // A low custom floor of 0.3 blocks a 0.5 defect that the 0.93 default would have left advisory. + expect(evaluateGateCheck(aiDefectWith(0.95), strict).conclusion).toBe("failure"); const lenient = gateCheckPolicy(settings({ aiReviewMode: "block", aiReviewCloseConfidence: 0.3 }), null, true); expect(evaluateGateCheck(aiDefectWith(0.5), lenient).conclusion).toBe("failure"); }); - it("a finding WITHOUT a confidence degrades to 1.0 and blocks under the default floor (graceful fallback) (#7)", () => { + it("a finding WITHOUT a confidence still blocks under aiReview:block (#7)", () => { const out = evaluateGateCheck(aiDefectWith(undefined), gateCheckPolicy(settings({ aiReviewMode: "block" }), null, true)); - expect(out.conclusion).toBe("failure"); // no confidence → treated as 1.0 → always clears 0.93 + expect(out.conclusion).toBe("failure"); }); it("never blocks when mode=advisory, regardless of a high confidence (#7)", () => { @@ -264,15 +262,14 @@ describe("AI close-confidence threshold gate (#7)", () => { it("applies the same confidence floor to an ai_review_split finding (#7)", () => { const policy = gateCheckPolicy(settings({ aiReviewMode: "block" }), null, true); - expect(evaluateGateCheck(splitDefectWith(0.95), policy).conclusion).toBe("failure"); // clears 0.93 → blocks - expect(evaluateGateCheck(splitDefectWith(0.92), policy).conclusion).toBe("neutral"); // below 0.93 → manual hold + expect(evaluateGateCheck(splitDefectWith(0.95), policy).conclusion).toBe("failure"); + expect(evaluateGateCheck(splitDefectWith(0.92), policy).conclusion).toBe("failure"); }); it("resolveEffectiveSettings maps gate.aiReview.closeConfidence (clamped) into the policy floor (#7)", () => { const eff = resolveEffectiveSettings(settings({ aiReviewMode: "off" }), parseFocusManifest({ gate: { aiReview: { mode: "block", closeConfidence: 0.4 } } })); expect(eff.aiReviewCloseConfidence).toBe(0.4); expect(eff.aiReviewMode).toBe("block"); - // a 0.5 defect clears the configured 0.4 floor → blocks (it would NOT under the 0.93 default). expect(evaluateGateCheck(aiDefectWith(0.5), gateCheckPolicy(eff, null, true)).conclusion).toBe("failure"); }); }); @@ -405,20 +402,19 @@ describe("merge-readiness composite gate (#551)", () => { }); }); -describe("first-time-contributor grace (#552)", () => { +describe("first-time-contributor grace compatibility (#552)", () => { // A would-be hard blocker for a confirmed contributor (linked-issue: block trips on the missing-issue PR). const blockingPolicy = { linkedIssueGateMode: "block" as const, confirmedContributor: true }; - it("(a) softens the block to a neutral/advisory gate for a genuine newcomer (0 merged, 0 closed-unmerged)", () => { + it("(a) does not soften blockers for a genuine newcomer (0 merged, 0 closed-unmerged)", () => { const result = evaluateGateCheck(missingIssueAdvisory(), { ...blockingPolicy, firstTimeContributorGrace: true, authorMergedPrCount: 0, authorClosedUnmergedPrCount: 0, }); - expect(result.conclusion).toBe("neutral"); - expect(result.blockers).toEqual([]); - expect(result.title).toContain("first-contribution grace"); + expect(result.conclusion).toBe("failure"); + expect(result.blockers.map((finding) => finding.code)).toEqual(["missing_linked_issue"]); }); it("(b) still blocks a repeat offender (0 merged, >= 3 closed-unmerged) — grace does not apply", () => { @@ -459,7 +455,19 @@ describe("first-time-contributor grace (#552)", () => { expect(policy.firstTimeContributorGrace).toBe(true); expect(policy.authorMergedPrCount).toBe(0); expect(policy.authorClosedUnmergedPrCount).toBe(1); - expect(evaluateGateCheck(missingIssueAdvisory(), { ...policy, linkedIssueGateMode: "block" }).conclusion).toBe("neutral"); + expect(evaluateGateCheck(missingIssueAdvisory(), { ...policy, linkedIssueGateMode: "block" }).conclusion).toBe("failure"); + }); +}); + +describe("review-thread blocker gate", () => { + it("always blocks unresolved review-thread findings", () => { + const advisory: Advisory = { + ...missingIssueAdvisory(), + findings: [{ code: REVIEW_THREAD_BLOCKER_CODE, severity: "critical", title: "review thread unresolved", detail: "Resolve it." }], + }; + const result = evaluateGateCheck(advisory); + expect(result.conclusion).toBe("failure"); + expect(result.blockers.map((finding) => finding.code)).toEqual([REVIEW_THREAD_BLOCKER_CODE]); }); }); @@ -693,8 +701,8 @@ describe("size + guardrail manual-review HOLD (#gate-size / #gate-guardrail)", ( const clean = (): Advisory => ({ ...missingIssueAdvisory(), findings: [] }); it("holds (neutral) an oversized PR; passes under thresholds; off/unset = no hold", () => { expect(evaluateGateCheck(clean(), { sizeGateMode: "advisory", changedFileCount: 12, changedLineCount: 10 }).conclusion).toBe("neutral"); // > 10 files - expect(evaluateGateCheck(clean(), { sizeGateMode: "advisory", changedFileCount: 2, changedLineCount: 600 }).conclusion).toBe("neutral"); // > 500 lines - expect(evaluateGateCheck(clean(), { sizeGateMode: "advisory", changedFileCount: 9, changedLineCount: 499 }).conclusion).toBe("success"); // under both thresholds + expect(evaluateGateCheck(clean(), { sizeGateMode: "advisory", changedFileCount: 2, changedLineCount: 1000 }).conclusion).toBe("neutral"); // >= 1000 lines + expect(evaluateGateCheck(clean(), { sizeGateMode: "advisory", changedFileCount: 9, changedLineCount: 999 }).conclusion).toBe("success"); // under both thresholds expect(evaluateGateCheck(clean(), { sizeGateMode: "off", changedFileCount: 50, changedLineCount: 9000 }).conclusion).toBe("success"); // gate off ⇒ no hold expect(evaluateGateCheck(clean(), { changedFileCount: 50, changedLineCount: 9000 }).conclusion).toBe("success"); // mode unset ⇒ no hold expect(evaluateGateCheck(clean(), { sizeGateMode: "advisory" }).conclusion).toBe("success"); // no counts ⇒ 0 ⇒ no hold diff --git a/test/unit/grounding-wiring.test.ts b/test/unit/grounding-wiring.test.ts index 9ca078bc55..3cb5ba0c09 100644 --- a/test/unit/grounding-wiring.test.ts +++ b/test/unit/grounding-wiring.test.ts @@ -88,7 +88,7 @@ describe("isGroundingEnabled", () => { }); }); -describe("aiCiRefutationActive (#ai-ci-refutation gate)", () => { +describe("aiCiRefutationActive compatibility helper", () => { const env = (grounding: string, repos: string) => ({ GITTENSORY_REVIEW_GROUNDING: grounding, GITTENSORY_REVIEW_REPOS: repos }) as unknown as Env; const REPO = "JSONbored/metagraphed"; diff --git a/test/unit/maintainer-activation.test.ts b/test/unit/maintainer-activation.test.ts index 8384f6199b..5618d8af9f 100644 --- a/test/unit/maintainer-activation.test.ts +++ b/test/unit/maintainer-activation.test.ts @@ -171,8 +171,11 @@ describe("buildMaintainerActivationPreview", () => { repoFullName: repo.fullName, repo, settings: settings(), - // Two OPEN PRs link the same issue (#42) → a duplicate cluster. Winner = lowest open number = #1. - pullRequests: [pr(1, { linkedIssues: [42] }), pr(2, { linkedIssues: [42] })], + // Two OPEN PRs link the same issue (#42) → a duplicate cluster. Winner = earliest observed claim = #1. + pullRequests: [ + pr(1, { linkedIssues: [42], linkedIssueClaimedAt: "2026-06-14T00:00:00.000Z" }), + pr(2, { linkedIssues: [42], linkedIssueClaimedAt: "2026-06-14T00:01:00.000Z" }), + ], generatedAt: "2026-06-14T00:00:00.000Z", duplicateWinnerEnabled: true, }); @@ -187,7 +190,10 @@ describe("buildMaintainerActivationPreview", () => { repoFullName: repo.fullName, repo, settings: settings(), - pullRequests: [pr(1, { linkedIssues: [42] }), pr(2, { linkedIssues: [42] })], + pullRequests: [ + pr(1, { linkedIssues: [42], linkedIssueClaimedAt: "2026-06-14T00:00:00.000Z" }), + pr(2, { linkedIssues: [42], linkedIssueClaimedAt: "2026-06-14T00:01:00.000Z" }), + ], generatedAt: "2026-06-14T00:00:00.000Z", }); expect(preview.findingCodeCounts).toContainEqual({ code: "duplicate_pr_risk", count: 2 }); @@ -199,7 +205,10 @@ describe("buildMaintainerActivationPreview", () => { repo, settings: settings(), // The only other PR linking #42 is CLOSED → not a live duplicate, so the open winner stays clean. - pullRequests: [pr(1, { linkedIssues: [42] }), pr(2, { state: "closed", linkedIssues: [42] })], + pullRequests: [ + pr(1, { linkedIssues: [42], linkedIssueClaimedAt: "2026-06-14T00:00:00.000Z" }), + pr(2, { state: "closed", linkedIssues: [42], linkedIssueClaimedAt: "2026-06-14T00:01:00.000Z" }), + ], generatedAt: "2026-06-14T00:00:00.000Z", duplicateWinnerEnabled: true, }); diff --git a/test/unit/predicted-gate.test.ts b/test/unit/predicted-gate.test.ts index 6e3306a784..d8c447c006 100644 --- a/test/unit/predicted-gate.test.ts +++ b/test/unit/predicted-gate.test.ts @@ -142,13 +142,13 @@ describe("buildPredictedGateVerdict", () => { expect(noGate.blockers.some((b) => b.code === "missing_linked_issue")).toBe(false); }); - it("honors public gate.firstTimeContributorGrace with predicted author history", () => { + it("does not let public gate.firstTimeContributorGrace soften duplicate blockers", () => { const newcomer = verdict({ gate: { duplicates: "block", firstTimeContributorGrace: true }, pullRequests: [openPr(42, "Retry uploads on 5xx responses", [7], "someone-else")], }); - expect(newcomer.conclusion).toBe("neutral"); - expect(newcomer.blockers).toHaveLength(0); + expect(newcomer.conclusion).toBe("failure"); + expect(newcomer.blockers.some((b) => b.code === "duplicate_pr_risk")).toBe(true); const returning = verdict({ gate: { duplicates: "block", firstTimeContributorGrace: true }, @@ -162,8 +162,8 @@ describe("buildPredictedGateVerdict", () => { }); it("matches author history case-insensitively, like the live gate (#audit-§4)", () => { - // The merged PR's author is "MINER1" (different case from the contributor "miner1"). The predictor must - // count it as history — otherwise it would falsely predict newcomer grace (neutral) while the live gate fails. + // The merged PR's author is "MINER1" (different case from the contributor "miner1"). The predictor still + // counts it as history, but blocker disposition no longer depends on first-time grace. const mixedCase = verdict({ gate: { duplicates: "block", firstTimeContributorGrace: true }, pullRequests: [ @@ -174,9 +174,9 @@ describe("buildPredictedGateVerdict", () => { expect(mixedCase.conclusion).toBe("failure"); }); - it("denies first-contribution grace to a repeat offender via the closed-unmerged author-count path", () => { - // The author has 3 prior CLOSED-unmerged PRs (state === "closed" && !mergedAt) in this repo, so - // authorClosedUnmergedPrCount === 3 → isRepeatOffender → grace does NOT apply and the gate blocks. + it("keeps duplicate blockers for repeat offenders via the closed-unmerged author-count path", () => { + // The author has 3 prior CLOSED-unmerged PRs (state === "closed" && !mergedAt) in this repo. Blocker + // disposition no longer depends on first-time grace, so the gate blocks either way. const closedUnmerged = (number: number, title: string): PullRequestRecord => ({ ...openPr(number, title, [], "miner1"), state: "closed", @@ -195,8 +195,8 @@ describe("buildPredictedGateVerdict", () => { }); it("counts a closed-but-merged PR as merge history via the mergedAt fallback (not state === merged)", () => { - // The prior PR has state "closed" yet carries a mergedAt timestamp, so it is only counted as merge - // history through the `|| pr.mergedAt` fallback → authorMergedPrCount >= 1 → not a newcomer → no grace. + // The prior PR has state "closed" yet carries a mergedAt timestamp, so it is still counted as merge history. + // Blocker disposition no longer depends on first-time grace, so the gate blocks either way. const result = verdict({ gate: { duplicates: "block", firstTimeContributorGrace: true }, pullRequests: [ diff --git a/test/unit/queue.test.ts b/test/unit/queue.test.ts index 06c9c7d284..eaa91c567a 100644 --- a/test/unit/queue.test.ts +++ b/test/unit/queue.test.ts @@ -7310,6 +7310,7 @@ describe("queue processors", () => { }); // The shared issue + the HIGHER-numbered open sibling (#92) → forms the same-issue duplicate cluster. await upsertIssueFromGitHub(env, "JSONbored/gittensory", { number: 1, title: "Cache the registry sync", state: "open", user: { login: "maintainer" }, author_association: "OWNER", body: "We should cache the registry fetch." }); + await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 91, title: "Fix the cache", state: "open", user: { login: "contributor" }, author_association: "CONTRIBUTOR", head: { sha: "win91" }, labels: [], body: "Fixes #1\n\nValidation: npm test" }); await upsertPullRequestFromGitHub(env, "JSONbored/gittensory", { number: 92, title: "Also fix the cache", state: "open", user: { login: "other" }, author_association: "CONTRIBUTOR", head: { sha: "sib92" }, labels: [], body: "Fixes #1" }); let gatePatchBody: { conclusion?: string; output?: { title?: string; text?: string } } = {}; diff --git a/test/unit/rules.test.ts b/test/unit/rules.test.ts index ac4389d6dc..4c2fb72f2d 100644 --- a/test/unit/rules.test.ts +++ b/test/unit/rules.test.ts @@ -133,8 +133,9 @@ describe("advisory rules", () => { headSha: "abc123", labels: [], linkedIssues: [4], + linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z", }; - const higherSibling: PullRequestRecord = { ...winner, number: 13, title: "Alternative registry sync" }; + const higherSibling: PullRequestRecord = { ...winner, number: 13, title: "Alternative registry sync", linkedIssueClaimedAt: "2026-06-29T10:01:00.000Z" }; const advisory = buildPullRequestAdvisory(repo, winner, { otherOpenPullRequests: [higherSibling], duplicateWinnerEnabled: true }); @@ -152,8 +153,9 @@ describe("advisory rules", () => { headSha: "abc123", labels: [], linkedIssues: [4], + linkedIssueClaimedAt: "2026-06-29T10:01:00.000Z", }; - const lowerSibling: PullRequestRecord = { ...loser, number: 12, title: "Add registry sync" }; + const lowerSibling: PullRequestRecord = { ...loser, number: 12, title: "Add registry sync", linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z" }; const advisory = buildPullRequestAdvisory(repo, loser, { otherOpenPullRequests: [lowerSibling], duplicateWinnerEnabled: true }); @@ -171,8 +173,9 @@ describe("advisory rules", () => { headSha: "abc123", labels: [], linkedIssues: [4], + linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z", }; - const higherSibling: PullRequestRecord = { ...wouldBeWinner, number: 13, title: "Alternative registry sync" }; + const higherSibling: PullRequestRecord = { ...wouldBeWinner, number: 13, title: "Alternative registry sync", linkedIssueClaimedAt: "2026-06-29T10:01:00.000Z" }; const advisory = buildPullRequestAdvisory(repo, wouldBeWinner, { otherOpenPullRequests: [higherSibling], duplicateWinnerEnabled: false }); @@ -190,8 +193,9 @@ describe("advisory rules", () => { headSha: "abc123", labels: [], linkedIssues: [4], + linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z", }; - const unrelated: PullRequestRecord = { ...lonePr, number: 13, title: "Unrelated change", linkedIssues: [99] }; + const unrelated: PullRequestRecord = { ...lonePr, number: 13, title: "Unrelated change", linkedIssues: [99], linkedIssueClaimedAt: "2026-06-29T10:01:00.000Z" }; const advisory = buildPullRequestAdvisory(repo, lonePr, { otherOpenPullRequests: [unrelated], duplicateWinnerEnabled: true }); @@ -1078,7 +1082,7 @@ describe("firstAddedLineFromPatch", () => { }); }); -describe("CI-refutation of the public comment gate (#ai-ci-refutation)", () => { +describe("green-CI compatibility reconciliation of the public comment gate", () => { const finding = (code: string): import("../../src/types").AdvisoryFinding => ({ code, severity: "critical", title: `t:${code}`, detail: `d:${code}` }); const failure = (codes: string[]): import("../../src/rules/advisory").GateCheckEvaluation => ({ enabled: true, @@ -1096,22 +1100,20 @@ describe("CI-refutation of the public comment gate (#ai-ci-refutation)", () => { expect(isAiJudgmentOnlyFailure(failure(["ai_consensus_defect", "duplicate_open_pr"]))).toBe(false); expect(isAiJudgmentOnlyFailure(failure(["slop_high"]))).toBe(false); expect(isAiJudgmentOnlyFailure(failure(["ai_review_inconclusive"]))).toBe(false); - // An empty blocker list is not a refutable AI-only failure. + // An empty blocker list is not an AI-only failure. expect(isAiJudgmentOnlyFailure({ ...failure([]), conclusion: "failure" })).toBe(false); // A non-failure conclusion is never AI-only-failure. expect(isAiJudgmentOnlyFailure({ ...failure(["ai_consensus_defect"]), conclusion: "success" })).toBe(false); }); - it("enabled + green CI + AI-judgment-only failure → SUCCESS with cleared blockers (matches the merge disposition)", () => { - const out = reconcileGateEvaluationForGreenCi(failure(["ai_consensus_defect"]), "passed", true); - expect(out.conclusion).toBe("success"); - expect(out.blockers).toEqual([]); - expect(out.title).toBe("Gittensory Orb Review Agent passed"); - expect(out.summary).toContain("advisory, not blocking"); + it("enabled + green CI + AI-judgment-only failure stays a failure", () => { + const fail = failure(["ai_consensus_defect"]); + expect(reconcileGateEvaluationForGreenCi(fail, "passed", true)).toBe(fail); }); - it("enabled + green CI + split-only failure → SUCCESS too", () => { - expect(reconcileGateEvaluationForGreenCi(failure(["ai_review_split"]), "passed", true).conclusion).toBe("success"); + it("enabled + green CI + split-only failure stays a failure too", () => { + const fail = failure(["ai_review_split"]); + expect(reconcileGateEvaluationForGreenCi(fail, "passed", true)).toBe(fail); }); it("is GATED by `enabled` — enabled=false returns the failure UNCHANGED even on green CI", () => { diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 094f833847..0be56ee9da 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -773,9 +773,19 @@ describe("signal coverage edge cases", () => { it("#dup-winner: panel hard-duplicate block is suppressed for the winner, kept for the loser, byte-identical when flag OFF", () => { const directRepo = repo("owner/dupwin"); const dupIssue = issue(directRepo.fullName, 42, "Cache invalidation race"); - // Two open PRs on the same issue: 70 is the lowest open number (the winner), 88 is the loser. - const winnerPr = pr(directRepo.fullName, 70, "Fix the cache race", { authorLogin: "miner", linkedIssues: [42], body: "Fixes #42" }); - const loserPr = pr(directRepo.fullName, 88, "Also fixes the cache race", { authorLogin: "other", linkedIssues: [42], body: "Fixes #42" }); + // Two open PRs on the same issue: 70 claimed the issue first (the winner), 88 is the later claimant. + const winnerPr = pr(directRepo.fullName, 70, "Fix the cache race", { + authorLogin: "miner", + linkedIssues: [42], + linkedIssueClaimedAt: "2026-06-29T10:00:00.000Z", + body: "Fixes #42", + }); + const loserPr = pr(directRepo.fullName, 88, "Also fixes the cache race", { + authorLogin: "other", + linkedIssues: [42], + linkedIssueClaimedAt: "2026-06-29T10:01:00.000Z", + body: "Fixes #42", + }); const collisions = buildCollisionReport(directRepo.fullName, [dupIssue], [winnerPr, loserPr]); const queueHealth = buildQueueHealth(directRepo, [dupIssue], [winnerPr, loserPr], collisions); const blockSettings = { ...repoSettings(directRepo.fullName), gateCheckMode: "enabled" as const, duplicatePrGateMode: "block" as const }; diff --git a/test/unit/unified-comment.test.ts b/test/unit/unified-comment.test.ts index da60fdcc9e..8d2d3f7a1e 100644 --- a/test/unit/unified-comment.test.ts +++ b/test/unit/unified-comment.test.ts @@ -74,8 +74,7 @@ describe("deriveUnifiedStatus", () => { it("a guarded-path hold downgrades a would-be-ready PR to held — never 'safe to merge' (#guarded-hold-comment)", () => { // A clean+green PR that touches a hard-guardrail path is HELD for owner review, so the comment says held. expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed" } }, { heldForReview: true })).toBe("held"); - // A guarded close with RED required CI still closes (the red check overrides the guardrail hold), so the - // headline stays "blocked"/Closed. The held-vs-closed nuance for non-red guarded closes is covered below. + // A guarded close still closes; guardrails hold only otherwise-ready PRs. expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "failed" } }, { heldForReview: true })).toBe("blocked"); // Without the hold flag, the same clean+green PR is ready. expect(deriveUnifiedStatus({ ...base, decision: "merge", readiness: { ciState: "passed" } }, { heldForReview: false })).toBe("ready"); @@ -85,10 +84,10 @@ describe("deriveUnifiedStatus", () => { // #9: an owner / automation-bot author is NEVER auto-closed → a gate "close" verdict renders held while CI is green/unknown. expect(deriveUnifiedStatus({ ...base, decision: "close" }, { neverClosed: true })).toBe("held"); expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "failed" } }, { neverClosed: true })).toBe("blocked"); - // #8: a guarded-path close is the disposition's HOLD (owner review) unless a red required check forces it. - expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "passed" } }, { heldForReview: true })).toBe("held"); - expect(deriveUnifiedStatus({ ...base, decision: "close" }, { heldForReview: true })).toBe("held"); // CI not yet reported → held - expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "failed" } }, { heldForReview: true })).toBe("blocked"); // red required CI → real close + // Guardrails do not downgrade a close/blocker verdict to held. + expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "passed" } }, { heldForReview: true })).toBe("blocked"); + expect(deriveUnifiedStatus({ ...base, decision: "close" }, { heldForReview: true })).toBe("blocked"); + expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "failed" } }, { heldForReview: true })).toBe("blocked"); // A genuine contributor close (no guard, not owner/bot) still headlines Closed/blocked. expect(deriveUnifiedStatus({ ...base, decision: "close" })).toBe("blocked"); expect(deriveUnifiedStatus({ ...base, decision: "close", readiness: { ciState: "failed" } })).toBe("blocked");