From a8bbd76f33bbacb9b184d2bea49749873d49ca3e Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 24 Jul 2026 14:01:13 -0700 Subject: [PATCH 1/3] fix(review): scope the bare-score public-safety check per repo (metagraphed#8038) sanitizePublicComment throws (never redacts) whenever public-facing AI text matches a bare \bscore\w*\b, to stop loopover's own private gittensor miner trust/reward VALUES from leaking. toPublicSafe catches the throw and returns null, and composeAdvisoryNotes silently discards the whole narrative assessment in favor of the generic "did not include a separate narrative summary" fallback. This filter applied identically across every repo the shared engine serves, but metagraphed's own public schema legitimately has fields named totalScore/credibility/baseTotalScore -- any AI review that naturally describes this code in prose is near-certain to trip the filter and lose its whole write-up. Observed live, recurring. sanitizePublicComment gains an optional allowBareScoreTerm flag, default false (unchanged behavior for every existing caller). Every OTHER forbidden phrase (wallet, hotkey, coldkey, trust score, reward, scoreability, ...) stays enforced unconditionally regardless of this flag -- only the over-broad bare-word check is ever relaxable. New isPublicScoreTermSafeForRepo resolves a fail-closed, config-as-code repo allowlist (LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS) with no '*'/'all' wildcard, since a blanket opt-in would silently cover a future repo that DOES have private trust/reward data. composeAdvisoryNotes threads the flag through to its 3 relevant call sites only; the other 11 toPublicSafe call sites in ai-review.ts are untouched. Additive and inert until an operator sets the env var for a specific repo. --- src/env.d.ts | 7 ++++ src/queue-intelligence.ts | 38 +++++++++++++++++++--- src/services/ai-review.ts | 28 ++++++++++------ test/unit/ai-review.test.ts | 48 ++++++++++++++++++++++++++++ test/unit/queue-intelligence.test.ts | 35 ++++++++++++++++++++ 5 files changed, 142 insertions(+), 14 deletions(-) diff --git a/src/env.d.ts b/src/env.d.ts index fc26b0ffc5..7ab1919feb 100644 --- a/src/env.d.ts +++ b/src/env.d.ts @@ -316,6 +316,13 @@ declare global { * non-repo-scoped contributor/operator tools (another contributor's private data, fleet analytics). Unset * ⇒ none. A separate allowlist from MCP_ACTUATION_REPO_ALLOWLIST so read and write trust can differ (#2455). */ MCP_READ_REPO_ALLOWLIST?: string; + /** Repos where a bare "score"/"scores"/"scored" mention in AI-review-composed public text is safe to + * publish (comma/whitespace `owner/repo` list; no `*`/`all` wildcard -- see + * isPublicScoreTermSafeForRepo's own doc comment for why). Unset ⇒ none: the check stays enforced + * everywhere by default. Set this for a repo whose OWN public schema legitimately names fields like + * `totalScore`/`credibility` (e.g. metagraphed) so a real AI review of that code doesn't have its whole + * narrative assessment silently discarded (#public-score-terms-scoping). */ + LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS?: string; /** Shared bearer secret required by the hosted Orb ingest collector. */ ORB_INGEST_TOKEN?: string; /** Shared bearer secret required by the hosted AMS ingest collector (#5681) — same optional, fail-open diff --git a/src/queue-intelligence.ts b/src/queue-intelligence.ts index a5123b247f..0f3195b55a 100644 --- a/src/queue-intelligence.ts +++ b/src/queue-intelligence.ts @@ -161,15 +161,45 @@ export async function analyzePRQueue( return { rankedPRs: scored.map(({ pr }) => pr), recommendations }; } -export function sanitizePublicComment(comment: string): string { +/** CSV/whitespace repo allowlist for {@link isPublicScoreTermSafeForRepo} -- same fail-closed/wildcard parse + * shape as auth/security.ts's MCP repo allowlists (a distinct, smaller local copy rather than a shared + * import: this one governs a content-safety exemption, not an actuation/read trust boundary, and the two + * concerns should stay independently editable). Unset/empty ⇒ deny (fail closed: the bare-score check stays + * ENFORCED for every repo unless an operator explicitly opts one in). `*`/`all` is deliberately NOT treated + * as a wildcard here, unlike the MCP allowlists -- this exemption is safety-relevant enough that a blanket + * opt-in-everything value would silently cover a FUTURE repo that DOES have private trust/reward data (e.g. + * loopover itself), which the MCP read/actuation allowlists' analogous risk doesn't share. + * LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS is the env var; config-as-code, never hardcoded per #config-as-code. */ +export function isPublicScoreTermSafeForRepo(env: { LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS?: string }, repoFullName: string): boolean { + const entries = (env.LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS ?? "") + .split(/[\s,]+/) + .map((entry) => entry.trim().toLowerCase()) + .filter(Boolean); + return entries.includes(repoFullName.toLowerCase()); +} + +/** `allowBareScoreTerm` (#public-score-terms-scoping, default false = today's unchanged behavior): the bare + * "\bscore\w*\b" check exists to catch a LEAKED gittensor trust/reward VALUE, but it also matches the word + * "score" used as ordinary, already-public API vocabulary -- a real problem for a repo like metagraphed, + * whose own public schema legitimately has fields named `totalScore`/`credibility`/`baseTotalScore`. Any AI + * review discussing that code naturally uses the word "score" and, before this option existed, had its + * WHOLE narrative assessment silently discarded (observed live: metagraphed#8038 and recurring). The other, + * EXPLICIT-PHRASE entries in FORBIDDEN_PUBLIC_COMMENT_WORDS ("trust score", "reward", "scoreability", ...) + * are NEVER skippable by this flag -- those name the actual private concept regardless of repo, so they stay + * enforced everywhere; only the over-broad bare-word check is ever relaxed, and only where a caller has + * positively confirmed (via a repo allowlist, never a blanket default) that "score" is safe public + * vocabulary for that repo. */ +export function sanitizePublicComment(comment: string, options?: { allowBareScoreTerm?: boolean }): string { for (const forbiddenWord of FORBIDDEN_PUBLIC_COMMENT_WORDS) { if (comment.toLowerCase().includes(forbiddenWord.toLowerCase())) { throw new Error(`Public comment contains forbidden word: "${forbiddenWord}"`); } } - const bareScoreMatch = comment.match(BARE_SCORE_TERM_PATTERN); - if (bareScoreMatch) { - throw new Error(`Public comment contains forbidden word: "${bareScoreMatch[0]}"`); + if (!options?.allowBareScoreTerm) { + const bareScoreMatch = comment.match(BARE_SCORE_TERM_PATTERN); + if (bareScoreMatch) { + throw new Error(`Public comment contains forbidden word: "${bareScoreMatch[0]}"`); + } } return comment; } diff --git a/src/services/ai-review.ts b/src/services/ai-review.ts index ce395c5aa0..25bed55d97 100644 --- a/src/services/ai-review.ts +++ b/src/services/ai-review.ts @@ -24,7 +24,7 @@ import { recordAiUsageEvent, sumAiEstimatedNeuronsSince, } from "../db/repositories"; -import { sanitizePublicComment } from "../queue-intelligence"; +import { isPublicScoreTermSafeForRepo, sanitizePublicComment } from "../queue-intelligence"; import { defangReviewInput } from "../review/safety"; import { convergedFeatureActive } from "../review/feature-activation"; import { labelSelfHostReviewerModels, labelSelfHostReviewerNames, resolveConfiguredProviderNames } from "../selfhost/ai-config"; @@ -599,12 +599,14 @@ function neutralizePublicMarkdown(text: string): string { .replace(/([\\`*_{}\[\]()#+!|])/g, "\\$1"); } -/** Returns neutralized text if it is public-safe, otherwise null (drop — never publish). */ -export function toPublicSafe(text: string | null | undefined): string | null { +/** Returns neutralized text if it is public-safe, otherwise null (drop — never publish). `allowBareScoreTerm` + * defaults false (unchanged behavior) -- only composeAdvisoryNotes' repo-scoped call sites ever pass true; + * see sanitizePublicComment's own doc comment for why this is the ONLY relaxable check. */ +export function toPublicSafe(text: string | null | undefined, options?: { allowBareScoreTerm?: boolean }): string | null { const trimmed = (text ?? "").trim(); if (!trimmed) return null; try { - return neutralizePublicMarkdown(sanitizePublicComment(trimmed)); + return neutralizePublicMarkdown(sanitizePublicComment(trimmed, options)); } catch { return null; } @@ -1619,8 +1621,14 @@ function composeFallbackAdvisoryNotes(notes: readonly string[]): string | null { return safeNotes.join("\n\n"); } -/** Compose a public-safe markdown advisory blurb from one or two model reviews. Null if no assessment is safe. */ -export function composeAdvisoryNotes(reviews: ModelReview[]): string | null { +/** Compose a public-safe markdown advisory blurb from one or two model reviews. Null if no assessment is safe. + * `allowBareScoreTerm` (#public-score-terms-scoping, default false): set true only for a repo the caller has + * confirmed via isPublicScoreTermSafeForRepo has no private trust/reward data of its own -- see + * sanitizePublicComment's doc comment. Metagraphed#8038-class bug: without this, ANY review of code with a + * legitimately-public `score`-named field (metagraphed's own `totalScore`/`credibility`) had its entire + * narrative assessment silently discarded in favor of the generic "did not include a separate narrative + * summary" fallback -- observed live, recurring. */ +export function composeAdvisoryNotes(reviews: ModelReview[], options?: { allowBareScoreTerm?: boolean }): string | null { const assessments = reviews.map((r) => r.assessment).filter(Boolean); // High-signal caps: a focused review shows only the few findings that matter (the prompt also asks the // model to be selective + deduplicate). Keep the core blockers and a handful of nits. (#focused-reviews) @@ -1629,12 +1637,12 @@ export function composeAdvisoryNotes(reviews: ModelReview[]): string | null { const nits = [ ...new Set(reviews.flatMap((r) => [...r.nits, ...r.suggestions])), ].slice(0, 5); - const assessment = toPublicSafe(assessments[0] ?? ""); + const assessment = toPublicSafe(assessments[0] ?? "", options); const safeBlockers = blockers - .map((s) => toPublicSafe(s)) + .map((s) => toPublicSafe(s, options)) .filter((s): s is string => Boolean(s)); const safeNits = nits - .map((s) => toPublicSafe(s)) + .map((s) => toPublicSafe(s, options)) .filter((s): s is string => Boolean(s)); const publicAssessment = assessment || fallbackPublicAssessment(safeBlockers, safeNits); @@ -2526,7 +2534,7 @@ export async function runLoopOverAiReview( if (inconclusive) incr("loopover_ai_review_inconclusive_total", { mode: input.mode }); const advisoryNotes = reviewsForNotes.length > 0 - ? (composeAdvisoryNotes(reviewsForNotes) ?? composeFallbackAdvisoryNotes(fallbackNotes)) + ? (composeAdvisoryNotes(reviewsForNotes, { allowBareScoreTerm: isPublicScoreTermSafeForRepo(env, input.repoFullName) }) ?? composeFallbackAdvisoryNotes(fallbackNotes)) : composeFallbackAdvisoryNotes(fallbackNotes); // Line-anchored inline findings (#inline-comments): only propagate model output when the resolved feature gate // asked for it. AI output is PR-author-influenced, so the prompt suffix is not an authorization boundary. diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index 239827c6ac..ca19f67d81 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -3530,6 +3530,54 @@ describe("pure helpers", () => { expect(withNits).toContain("Add coverage for the edge case."); }); + it("REGRESSION (#public-score-terms-scoping, metagraphed#8038): a bare 'score' mention drops the whole assessment by default, but survives when the repo is explicitly allowlisted", () => { + const reviewMentioningScore = [ + { + // A standalone "score" in natural review prose (\bscore\w*\b -- note this does NOT match a camelCase + // identifier like "totalScore", only a real word-boundary-delimited mention; this is exactly the shape + // an AI reviewer's own natural-language description of a scoring field takes). + assessment: "The resolver correctly filters results by their score before returning them.", + suggestions: [], + // Deliberately score-free, unlike the assessment above: proves the fallback text below comes from + // this SURVIVING nit (dropping the assessment must not also silently empty out unrelated findings). + nits: ["Consider extracting the filter helper for reuse."], + blockers: [], + inlineFindings: [], + confidence: 1, + }, + ]; + // Default (no options / allowBareScoreTerm false): unchanged, current behavior -- the whole assessment is + // dropped and the generic no-narrative-summary fallback takes over, same as before this fix existed. + const defaultResult = composeAdvisoryNotes(reviewMentioningScore); + expect(defaultResult).toContain("did not include a separate narrative summary"); + expect(defaultResult).not.toContain("filters results by their score"); + // Allowlisted (what runLoopOverAiReview now passes for a repo in LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS): + // the real narrative assessment survives. + const allowedResult = composeAdvisoryNotes(reviewMentioningScore, { allowBareScoreTerm: true }); + expect(allowedResult).toContain("The resolver correctly filters results by their score"); + expect(allowedResult).not.toContain("did not include a separate narrative summary"); + }); + + it("REGRESSION (#public-score-terms-scoping): the EXPLICIT-PHRASE bans (trust score, reward, scoreability, ...) stay enforced even when allowBareScoreTerm is true", () => { + const reviewLeakingTrustScore = [ + { + assessment: "This PR exposes the miner's raw trust score in the response payload.", + suggestions: [], + // A surviving, unrelated nit -- proves the explicit-phrase throw takes down ONLY the assessment + // (the same "dropping one field must not silently empty everything else" property as the sibling + // test above), not that the whole review vanishes. + nits: ["Add a test for the pagination edge case."], + blockers: [], + inlineFindings: [], + confidence: 1, + }, + ]; + const result = composeAdvisoryNotes(reviewLeakingTrustScore, { allowBareScoreTerm: true }); + expect(result).not.toContain("raw trust score"); + expect(result).toContain("did not include a separate narrative summary"); + expect(result).toContain("Add a test for the pagination edge case."); + }); + it("parseModelReview parses well-formed inline findings, including a trimmed optional suggestion; severity defaults to nit unless exactly 'blocker' (#inline-comments)", () => { const json = JSON.stringify({ assessment: "ok", diff --git a/test/unit/queue-intelligence.test.ts b/test/unit/queue-intelligence.test.ts index 709c97c589..f58f4656d3 100644 --- a/test/unit/queue-intelligence.test.ts +++ b/test/unit/queue-intelligence.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { analyzePRQueue, generatePublicComment, + isPublicScoreTermSafeForRepo, sanitizePublicComment, FORBIDDEN_PUBLIC_COMMENT_WORDS, } from "../../src/queue-intelligence"; @@ -387,3 +388,37 @@ describe("sanitizePublicComment — cohort/score public-boundary gap regression" ); }); }); + +describe("sanitizePublicComment — allowBareScoreTerm option (#public-score-terms-scoping)", () => { + it("passes a bare 'score' mention through when allowBareScoreTerm is true", () => { + const text = "The resolver correctly filters by totalScore before returning results."; + expect(sanitizePublicComment(text, { allowBareScoreTerm: true })).toBe(text); + }); + + it("still throws on a bare 'score' mention when allowBareScoreTerm is explicitly false (unchanged default)", () => { + expect(() => sanitizePublicComment("The score looks good here.", { allowBareScoreTerm: false })).toThrow(/score/i); + }); + + it("still throws on every OTHER forbidden phrase even when allowBareScoreTerm is true — only the bare-word check is relaxable", () => { + for (const forbiddenWord of FORBIDDEN_PUBLIC_COMMENT_WORDS) { + expect(() => sanitizePublicComment(`This comment contains ${forbiddenWord} information`, { allowBareScoreTerm: true })).toThrow(); + } + }); +}); + +describe("isPublicScoreTermSafeForRepo (#public-score-terms-scoping)", () => { + it("denies (fail-closed) when the env var is unset", () => { + expect(isPublicScoreTermSafeForRepo({}, "JSONbored/metagraphed")).toBe(false); + }); + + it("allows a repo present in the comma/whitespace list, case-insensitively", () => { + const env = { LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS: "JSONbored/metagraphed, other/repo" }; + expect(isPublicScoreTermSafeForRepo(env, "jsonbored/METAGRAPHED")).toBe(true); + expect(isPublicScoreTermSafeForRepo(env, "other/repo")).toBe(true); + }); + + it("denies a repo NOT present in the list — no '*'/'all' wildcard escape hatch, unlike the MCP allowlists", () => { + const env = { LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS: "*" }; + expect(isPublicScoreTermSafeForRepo(env, "JSONbored/loopover")).toBe(false); + }); +}); From f60a14c002836ff1bb2786f73ef05221825820f7 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 24 Jul 2026 14:23:04 -0700 Subject: [PATCH 2/3] fix(queue): schedule a guaranteed trailing re-check when mergeable_state stays unknown refreshLiveMergeState's own short inline retry (previous commit, ~4s total) resolves most GitHub mergeable_state "still computing" reads within the same pass, but not all -- on a large PR or under load, GitHub can take longer. Without this, the only remaining catch-up was the periodic agent-regate-sweep, which is NOT a reliable fixed interval: it throttles under GitHub REST-budget backpressure and skips re-arming while a previous sweep trigger is still in flight (index.ts's own backpressure comment) -- under sustained cross-repo load this can stretch well past minutes, during which an overlapping sibling PR can land first and base-conflict the original PR out from under it (observed live, metagraphed#8037). Schedules ONE targeted, single-PR follow-up (60s delay) specifically when a PR holds because mergeableState is "unknown" -- the one genuinely transient value; "dirty"/"blocked"/"behind" are real, stable holds a re-check moments later would only reproduce, so they're deliberately excluded. Reuses the exact delayed-JOBS.send + check-then-claim-after-success dedup pattern scheduleTrailingIssueLinkedReReview already established in this file for an analogous problem, not a new mechanism. Also closes a Codecov branch-coverage gap on the prior score-terms commit: composeAdvisoryNotes' isPublicScoreTermSafeForRepo call site had only its false arm exercised end-to-end through runLoopOverAiReview; added the companion allowed-repo case. --- src/queue/processors.ts | 50 +++++++++++ test/unit/ai-review.test.ts | 27 ++++++ test/unit/queue-lifecycle-guards.test.ts | 109 +++++++++++++++++++++++ 3 files changed, 186 insertions(+) diff --git a/src/queue/processors.ts b/src/queue/processors.ts index 094db35f49..a666b9304b 100644 --- a/src/queue/processors.ts +++ b/src/queue/processors.ts @@ -3330,6 +3330,13 @@ async function runAgentMaintenancePlanAndExecute( finalActionClasses: breakerOnPlan.map((action) => action.actionClass), }, }).catch(() => undefined); + // #merge-race guarantee: "unknown" is the ONE genuinely transient mergeableState value (GitHub still + // computing) -- "dirty"/"blocked"/"behind" are real, stable holds that a re-check moments later would only + // reproduce, so they're deliberately NOT retried here. Best-effort: an enqueue failure here must never + // fail this webhook pass, matching every other trailing-schedule call site in this file. + if ((liveMergeState ?? pr.mergeableState) === "unknown") { + await scheduleTrailingMergeableStateReReview(env, deliveryId, installationId, repoFullName, pr.number).catch(() => undefined); + } } if (breakerOnPlan.length === 0) { return; @@ -4344,6 +4351,49 @@ async function scheduleTrailingIssueLinkedReReview( await putTransientKey(env, key, "1", CI_COALESCE_WINDOW_SECONDS); } +// #merge-race guarantee (metagraphed#8037): refreshLiveMergeState's own short inline retry (ci-resolution.ts, +// ~4s total) resolves most GitHub mergeable_state "still computing" reads within the SAME pass, but not all -- +// on a large PR or under load, GitHub can take longer. Without this, the ONLY remaining catch-up mechanism was +// the periodic agent-regate-sweep, which is NOT a reliable fixed interval: it throttles under GitHub REST-budget +// backpressure and skips re-arming while a previous sweep trigger is still in flight (see the backpressure +// comment in index.ts) -- under sustained cross-repo load this can stretch well past minutes, during which an +// overlapping sibling PR can land first and base-conflict the original PR out from under it (observed live). +// This schedules ONE targeted, single-PR follow-up -- reusing the same agent-regate-pr job every other trailing +// re-check in this file uses -- completely independent of the sweep's own budget/backlog throttling, so a hold +// caused SPECIFICALLY by an unresolved mergeable_state is guaranteed a fresh look within a short, bounded, +// sweep-independent window instead of an unbounded one. +const MERGE_STATE_UNKNOWN_TRAILING_RECHECK_DELAY_SECONDS = 60; +async function scheduleTrailingMergeableStateReReview( + env: Env, + deliveryId: string, + installationId: number, + repoFullName: string, + prNumber: number, +): Promise { + const key = `merge-state-unknown-trailing:${repoFullName.toLowerCase()}#${prNumber}`; + // Same check-then-claim-only-after-send shape as scheduleTrailingIssueLinkedReReview above (#2371 follow-up + // reasoning applies identically here): claiming before the enqueue succeeds would permanently swallow the + // guarantee this function exists to provide for the rest of the window. + if (await getTransientKey(env, key)) return; + try { + await env.JOBS.send( + { type: "agent-regate-pr", deliveryId, repoFullName, prNumber, installationId }, + { delaySeconds: MERGE_STATE_UNKNOWN_TRAILING_RECHECK_DELAY_SECONDS }, + ); + } catch (error) { + console.log( + JSON.stringify({ + event: "merge_state_unknown_trailing_enqueue_failed", + repoFullName, + pull: prNumber, + message: errorMessage(error).slice(0, 120), + }), + ); + return; // do NOT claim -- a later pass hitting the same "unknown" read should retry the enqueue + } + await putTransientKey(env, key, "1", MERGE_STATE_UNKNOWN_TRAILING_RECHECK_DELAY_SECONDS); +} + /** Best-effort wake for sibling PRs discovered to be over the per-contributor cap by a LATER delivery (#2270, * #2479 gate finding): webhook delivery order isn't guaranteed to match PR creation order, so a sibling's own * webhook can fire before this one exists in the DB and wrongly conclude the author is within the cap — with diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index ca19f67d81..b6843ca77b 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -3578,6 +3578,33 @@ describe("pure helpers", () => { expect(result).toContain("Add a test for the pagination edge case."); }); + it("REGRESSION (#public-score-terms-scoping): runLoopOverAiReview resolves isPublicScoreTermSafeForRepo from LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS and threads it into composeAdvisoryNotes end-to-end", async () => { + const run = vi.fn(async () => ({ + response: reviewJson({ assessment: "The resolver correctly filters results by their score before returning them." }), + })); + const allowedEnv = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS: "acme/widgets", + }); + const allowedResult = await runLoopOverAiReview(allowedEnv, baseInput); // baseInput.repoFullName === "acme/widgets" + expect(allowedResult.status).toBe("ok"); + expect(allowedResult.status === "ok" ? allowedResult.advisoryNotes : undefined).toContain( + "The resolver correctly filters results by their score", + ); + + const deniedEnv = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + // Unset LOOPOVER_PUBLIC_SCORE_TERMS_ALLOWED_REPOS: fail-closed default, same repo as above. + }); + const deniedResult = await runLoopOverAiReview(deniedEnv, baseInput); + expect(deniedResult.status).toBe("ok"); + expect(deniedResult.status === "ok" ? deniedResult.advisoryNotes : undefined).not.toContain("filters results by their score"); + }); + it("parseModelReview parses well-formed inline findings, including a trimmed optional suggestion; severity defaults to nit unless exactly 'blocker' (#inline-comments)", () => { const json = JSON.stringify({ assessment: "ok", diff --git a/test/unit/queue-lifecycle-guards.test.ts b/test/unit/queue-lifecycle-guards.test.ts index 21d316081f..c37d6f609d 100644 --- a/test/unit/queue-lifecycle-guards.test.ts +++ b/test/unit/queue-lifecycle-guards.test.ts @@ -5180,6 +5180,115 @@ describe("auto-action convergence: end-to-end plan+execute for the general heuri expect(await renderMetrics()).toContain('loopover_agent_disposition_total{action_class="merge",autonomy_level="auto",blocker_class="none",repo="redacted-1"} 1'); }); + it("REGRESSION (#merge-race, metagraphed#8037): an otherwise-mergeable PR whose live mergeable_state comes back \"unknown\" schedules a trailing single-PR re-check instead of relying solely on the periodic sweep", async () => { + // Same fully-qualifying-to-merge fixture as the convergence-chain test above (real file, CI green, approved + // review, linked-issue gate off) -- the ONLY difference is /pulls/63 reporting mergeable_state "unknown" + // (GitHub still computing) instead of "clean", isolating this as the SOLE hold reason. + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await setupAutoActionRepo(env, { autonomy: { merge: "auto", approve: "auto" }, linkedIssueGateMode: "off" }); + await upsertOfficialMinerDetection(env, "contributor", { status: "confirmed", snapshot: queueMinerSnapshot("contributor") }, 60_000); + const scheduled: Array<{ message: import("../../src/types").JobMessage; options?: { delaySeconds?: number } }> = []; + const originalSend = env.JOBS.send.bind(env.JOBS); + env.JOBS.send = (async (message: import("../../src/types").JobMessage, options?: { delaySeconds?: number }) => { + scheduled.push(options ? { message, options } : { message }); + return originalSend(message, options); + }) as typeof env.JOBS.send; + const seen = { closed: false, merged: false }; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url === "https://api.github.com/graphql") { + return Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }); + } + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/63/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.includes("/pulls/63/reviews")) return Response.json([]); + if (url.includes("/pulls/63/commits")) return Response.json([]); + if (url.endsWith("/pulls/63/merge") && method === "PUT") { + seen.merged = true; + return Response.json({ merged: true }); + } + if (url.endsWith("/pulls/63")) { + return Response.json({ number: 63, state: "open", user: { login: "contributor" }, head: { sha: "conv63" }, mergeable_state: "unknown" }); + } + if (url.includes("/commits/conv63/check-runs")) { + return Response.json({ total_count: 1, check_runs: [{ name: "CI", status: "completed", conclusion: "success", app: { slug: "github-actions" } }] }); + } + if (url.includes("/commits/conv63/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/63/labels")) return Response.json([]); + if (url.includes("/issues/63/comments")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "merge-race-1", + eventName: "pull_request", + payload: prPayload({ number: 63, head: { sha: "conv63" }, body: "Closes #1", action: "synchronize" }), + }); + + // Not merged THIS pass -- mergeableState never resolved to "clean" even after refreshLiveMergeState's own + // inline retry (this fixture returns "unknown" on every /pulls/63 call, simulating GitHub still computing + // well past that short window). + expect(seen.merged).toBe(false); + const holdAudit = await env.DB.prepare("select detail from audit_events where event_type = 'agent.action.hold' order by created_at desc limit 1").first<{ detail: string }>(); + expect(holdAudit?.detail).toContain("mergeable_state is unknown"); + // The guarantee this test exists for: a targeted, single-PR trailing re-check is scheduled, decoupled + // from the periodic agent-regate-sweep's own GitHub-REST-budget throttling. + const trailing = scheduled.find((s) => s.message.type === "agent-regate-pr" && "prNumber" in s.message && s.message.prNumber === 63); + expect(trailing).toBeDefined(); + expect(trailing?.options).toEqual({ delaySeconds: 60 }); + }); + + it("REGRESSION (#merge-race): a real, stable non-clean mergeable_state (dirty) does NOT schedule a trailing re-check — only the transient \"unknown\" value does", async () => { + const env = createTestEnv({ GITHUB_APP_PRIVATE_KEY: await generatePrivateKeyPem() }); + await setupAutoActionRepo(env, { autonomy: { merge: "auto", approve: "auto" }, linkedIssueGateMode: "off" }); + await upsertOfficialMinerDetection(env, "contributor", { status: "confirmed", snapshot: queueMinerSnapshot("contributor") }, 60_000); + const scheduled: Array<{ message: import("../../src/types").JobMessage; options?: { delaySeconds?: number } }> = []; + const originalSend = env.JOBS.send.bind(env.JOBS); + env.JOBS.send = (async (message: import("../../src/types").JobMessage, options?: { delaySeconds?: number }) => { + scheduled.push(options ? { message, options } : { message }); + return originalSend(message, options); + }) as typeof env.JOBS.send; + vi.stubGlobal("fetch", async (input: RequestInfo | URL, init?: RequestInit) => { + const url = input.toString(); + const method = init?.method ?? "GET"; + if (url === "https://api.gittensor.io/miners") return Response.json([]); + if (url === "https://api.github.com/graphql") { + return Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }); + } + if (url.includes("/access_tokens")) return Response.json({ token: "installation-token" }); + if (url.includes("/pulls/64/files")) return Response.json([{ filename: "src/a.ts", status: "modified", additions: 1, deletions: 0, changes: 1, patch: "@@\n+export const ok = true;" }]); + if (url.includes("/pulls/64/reviews")) return Response.json([]); + if (url.includes("/pulls/64/commits")) return Response.json([]); + if (url.endsWith("/pulls/64")) { + return Response.json({ number: 64, state: "open", user: { login: "contributor" }, head: { sha: "conv64" }, mergeable_state: "dirty" }); + } + if (url.includes("/commits/conv64/check-runs")) { + return Response.json({ total_count: 1, check_runs: [{ name: "CI", status: "completed", conclusion: "success", app: { slug: "github-actions" } }] }); + } + if (url.includes("/commits/conv64/status")) return Response.json({ state: "success", statuses: [] }); + if (url.includes("/issues/64/labels")) return Response.json([]); + if (url.includes("/issues/64/comments")) return Response.json([]); + return Response.json({}); + }); + + await processJob(env, { + type: "github-webhook", + deliveryId: "merge-race-dirty-1", + eventName: "pull_request", + payload: prPayload({ number: 64, head: { sha: "conv64" }, body: "Closes #1", action: "synchronize" }), + }); + + const holdAudit = await env.DB.prepare("select detail from audit_events where event_type = 'agent.action.hold' order by created_at desc limit 1").first<{ detail: string }>(); + // "dirty" gets its own dedicated message (agentHoldAuditDetail), distinct from the generic + // "mergeable_state is X" phrasing every OTHER non-clean value shares -- confirms this test fixture actually + // reached the dirty branch, not some other unrelated hold reason. + expect(holdAudit?.detail).toContain("conflicts with the base branch"); + expect(scheduled.some((s) => s.message.type === "agent-regate-pr" && "prNumber" in s.message && s.message.prNumber === 64)).toBe(false); + }); + // #terminal-outcome-audit: end-to-end proof that the LIVE runAgentMaintenancePlanAndExecute call site (not just // the extracted pure precisionBreakerDowngradeDirections/applyPrecisionBreakers unit tests) actually increments // loopover_precision_breaker_downgrades_total when an engaged accuracy circuit-breaker rewrites a real plan. From 0a076dfa056cf522d0954ebc0f570e888ad95f75 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Fri, 24 Jul 2026 14:29:12 -0700 Subject: [PATCH 3/3] test(review): cover composeAdvisoryNotes' null fallthrough end-to-end (Codecov branch gap) The composeAdvisoryNotes(...) ?? composeFallbackAdvisoryNotes(fallbackNotes) line's right-hand branch was never exercised through runLoopOverAiReview -- the prior score-terms tests only ever hit composeAdvisoryNotes' own internal non-null fallback text, never this outer nullish-coalescing fallthrough. Mirrors the existing "composeAdvisoryNotes returns null when no assessment or finding is public-safe" unit fixture, driven end-to-end instead. --- test/unit/ai-review.test.ts | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/test/unit/ai-review.test.ts b/test/unit/ai-review.test.ts index b6843ca77b..31cfbbb6dc 100644 --- a/test/unit/ai-review.test.ts +++ b/test/unit/ai-review.test.ts @@ -3605,6 +3605,24 @@ describe("pure helpers", () => { expect(deniedResult.status === "ok" ? deniedResult.advisoryNotes : undefined).not.toContain("filters results by their score"); }); + it("REGRESSION (#public-score-terms-scoping, branch coverage): runLoopOverAiReview falls through to composeFallbackAdvisoryNotes when composeAdvisoryNotes itself returns null (every field unsafe)", async () => { + // Mirrors the "composeAdvisoryNotes returns null when no assessment or finding is public-safe" unit + // fixture, but driven end-to-end through runLoopOverAiReview so the `composeAdvisoryNotes(...) ?? + // composeFallbackAdvisoryNotes(fallbackNotes)` line's right-hand branch is actually exercised (the + // score-terms tests above only ever hit composeAdvisoryNotes' own internal non-null fallback text, never + // this outer `??`). + const run = vi.fn(async () => ({ + response: reviewJson({ assessment: "reward payout farming", suggestions: ["payout"], nits: ["reward"], blockers: [] }), + })); + const env = createTestEnv({ + AI: { run } as unknown as Ai, + AI_SUMMARIES_ENABLED: "true", + AI_PUBLIC_COMMENTS_ENABLED: "true", + }); + const result = await runLoopOverAiReview(env, baseInput); + expect(result.status).toBe("ok"); + }); + it("parseModelReview parses well-formed inline findings, including a trimmed optional suggestion; severity defaults to nit unless exactly 'blocker' (#inline-comments)", () => { const json = JSON.stringify({ assessment: "ok",