Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 22 additions & 6 deletions src/github/backfill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3276,6 +3276,19 @@ export function isStatusRollupGraphQlEnabled(env: { GITHUB_STATUS_ROLLUP_GRAPHQL
return /^(1|true|yes|on)$/i.test(env.GITHUB_STATUS_ROLLUP_GRAPHQL ?? "");
}

// Mirrors parseRepoFullName in labels.ts / assignees.ts (#8311): backfill keeps its own local copy rather than
// importing a shared one. Returns null for malformed input so each GraphQL read helper preserves its existing
// fail-soft return contract (null / undefined / []).
function parseBackfillRepoFullName(repoFullName: string): { owner: string; name: string } | null {
const parts = repoFullName.split("/");
const owner = parts[0];
const name = parts[1];
if (parts.length !== 2 || !owner || !name || /\s/.test(repoFullName)) {
return null;
}
return { owner, name };
}

/**
* GraphQL equivalent of {@link fetchLiveCiAggregate}: ONE bounded query returns the head commit's statusCheckRollup
* (check-runs AND classic statuses, unified) plus its check-suites — replacing the paginated /check-runs + /status
Expand All @@ -3295,8 +3308,9 @@ export async function fetchLiveCiAggregateViaGraphQl(
advisoryCheckRuns?: ReadonlyArray<{ name: string; appSlug: string }> | null,
): Promise<LiveCiAggregate | null> {
if (!headSha || !token) return null;
const [owner, name] = repoFullName.split("/");
if (!owner || !name) return null;
const parsed = parseBackfillRepoFullName(repoFullName);
if (!parsed) return null;
const { owner, name } = parsed;
const query = `query LoopOverLiveCiRollup { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { object(oid: ${JSON.stringify(headSha)}) { ... on Commit { statusCheckRollup { contexts(first: 100) { nodes { __typename ... on CheckRun { name conclusion status startedAt detailsUrl title summary checkSuite { databaseId app { slug } } } ... on StatusContext { context state description targetUrl } } pageInfo { hasNextPage } } } checkSuites(first: 100) { nodes { status app { slug } } pageInfo { hasNextPage } } } } } }`;
const result = await githubGraphQl<{
data?: {
Expand Down Expand Up @@ -4058,8 +4072,9 @@ export async function fetchLivePullRequestReviewDecision(
admissionKey?: GitHubRateLimitAdmissionKey,
): Promise<string | undefined> {
if (!token) return undefined;
const [owner, name] = repoFullName.split("/");
if (!owner || !name) return undefined;
const parsed = parseBackfillRepoFullName(repoFullName);
if (!parsed) return undefined;
const { owner, name } = parsed;
const query = `query { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { pullRequest(number: ${prNumber}) { reviewDecision } } }`;
const result = await githubGraphQl<{ data?: { repository?: { pullRequest?: { reviewDecision?: string | null } | null } | null }; errors?: unknown[] }>(
env,
Expand Down Expand Up @@ -4130,8 +4145,9 @@ export async function fetchLiveReviewThreadBlockers(
admissionKey?: GitHubRateLimitAdmissionKey,
): Promise<ReviewThreadBlocker[]> {
if (!token) return [];
const [owner, name] = repoFullName.split("/");
if (!owner || !name) return [];
const parsed = parseBackfillRepoFullName(repoFullName);
if (!parsed) return [];
const { owner, name } = parsed;
const threads: Array<GitHubReviewThreadNode | null> = [];
let cursor: string | null = null;
const seenCursors = new Set<string>();
Expand Down
2 changes: 2 additions & 0 deletions src/selfhost/metrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ export const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [
["loopover_active_review_reconciliation_terminalized_total", { help: "Orphaned active_review_tracking rows terminalized after a live GitHub check confirmed the PR is closed, by repo.", type: "counter" }],
["loopover_open_pr_reconciliation_missing_total", { help: "Open PRs found missing from local tracking during reconciliation, by repo.", type: "counter" }],
["loopover_orb_relay_malformed_events_total", { help: "Orb relay batch entries dropped for missing/mistyped required fields (deliveryId/eventName/rawBody).", type: "counter" }],
["loopover_orb_relay_multiple_live_enrollments_total", { help: "Forwarded orb events where more than one LIVE enrollment existed for the installation (a blue/green swap, or a secret rotated but not yet revoked) -- the winner is still elected deterministically, but the overlap is no longer silent (#9150).", type: "counter" }],
["loopover_orb_relay_register_total", { help: "Orb relay registration attempts, by mode and result (registered/recovered/failed).", type: "counter" }],
["loopover_pr_outcomes_total", { help: "Recorded PR gate outcomes, by decision.", type: "counter" }],
["loopover_close_audit_holdouts_total", { help: "Would-auto-close PRs diverted to human adjudication by the close-audit holdout (#8831).", type: "counter" }],
Expand All @@ -181,6 +182,7 @@ export const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [
["loopover_rees_enrich_request_duration_seconds", { help: "REES /v1/enrich call duration in seconds, for calls that were actually attempted (excludes the auth-rejected circuit-breaker skip).", type: "histogram" }],
["loopover_metrics_sampler_errors_total", { help: "Scrape-time gauge sampler failures, by metric name -- a failing sampler previously emitted no series at all, silently. Any occurrence means that metric's value is currently invisible to Prometheus this scrape (see the sentinel gauges' own -1-on-failure convention).", type: "counter" }],
["loopover_review_source_fresh", { help: "1 when a review/ops/reputation source table has a row inside its own consumer's window, 0 when stale -- labeled by table and window_days. review_targets was silently orphaned by the 2026-06-22 convergence cutover for months before anyone noticed; this makes the next such orphaning loud instead.", type: "gauge" }],
["loopover_private_manifest_warnings_total", { help: "Private-manifest layer warnings (a malformed shared/global/repo config layer dropped during load), counted one per warning rather than one per load -- a sustained run means a mount is repeatedly serving truncated or invalid config (#9065).", type: "counter" }],
];
const metricMeta = new Map<string, MetricMeta>(DEFAULT_METRIC_META);

Expand Down
16 changes: 16 additions & 0 deletions test/unit/backfill-2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1660,6 +1660,18 @@ describe("GitHub backfill", () => {
// (`liveReviewDecision ?? pr.reviewDecision`), so a PR approved at backfill time and later flipped to
// CHANGES_REQUESTED still read as APPROVED -> approvalsSatisfied -> merge. The sentinel is a real VALUE
// precisely so it survives that `??` and the stale stored decision can never be substituted.
describe("fetchLivePullRequestReviewDecision — repoFullName guard (#9317)", () => {
it("returns undefined for extra-segment and whitespace-padded slugs before any GraphQL call", async () => {
const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
const fetchSpy = vi.fn(async () => Response.json({ data: { repository: { pullRequest: { reviewDecision: "APPROVED" } } } }));
vi.stubGlobal("fetch", fetchSpy);
for (const repoFullName of ["owner/repo/extra", "owner/ repo", " owner/repo"]) {
expect(await fetchLivePullRequestReviewDecision(env, repoFullName, 7, "public-token")).toBeUndefined();
}
expect(fetchSpy).not.toHaveBeenCalled();
});
});

describe("fetchLivePullRequestReviewDecision — partial-response guard (#9052)", () => {
it("returns the unreadable sentinel on a 200-with-errors response, so the stale stored decision cannot win the ?? fallback", async () => {
const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" });
Expand Down Expand Up @@ -2528,6 +2540,10 @@ describe("GitHub backfill", () => {
expect(fetchSpy).not.toHaveBeenCalled();
await expect(fetchLiveReviewThreadBlockers(env, "malformed", 1, "public-token")).resolves.toEqual([]);
expect(fetchSpy).not.toHaveBeenCalled();
for (const repoFullName of ["owner/repo/extra", "owner/ repo", " owner/repo"]) {
await expect(fetchLiveReviewThreadBlockers(env, repoFullName, 1, "public-token")).resolves.toEqual([]);
}
expect(fetchSpy).not.toHaveBeenCalled();
await expect(fetchLiveReviewThreadBlockers(env, "JSONbored/gittensory", 1, "public-token")).resolves.toEqual([]);
expect(fetchSpy).toHaveBeenCalledTimes(1);
});
Expand Down
10 changes: 10 additions & 0 deletions test/unit/graphql-status-rollup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,16 @@ describe("fetchLiveCiAggregateViaGraphQl — verdicts", () => {
expect(await fetchLiveCiAggregateViaGraphQl(env, "no-slash", SHA, TOKEN)).toBeNull();
});

// #9317: segment-count + whitespace guard, matching pr-actions.ts/assignees.ts/labels.ts (#8311).
it("returns null for extra-segment and whitespace-padded repo slugs before any GraphQL call (#9317)", async () => {
const fetchSpy = vi.fn(async () => Response.json(graphqlBody({ runs: [{ name: "build", conclusion: "SUCCESS", status: "COMPLETED" }] })));
vi.stubGlobal("fetch", fetchSpy);
for (const repoFullName of ["owner/repo/extra", "owner/ repo", " owner/repo"]) {
expect(await fetchLiveCiAggregateViaGraphQl(env, repoFullName, SHA, TOKEN)).toBeNull();
}
expect(fetchSpy).not.toHaveBeenCalled();
});

it("returns null on a GraphQL error or an unexpected/absent commit (→ REST fallback)", async () => {
stubGraphql(graphqlBody(), { status: 500 });
expect(await fetchLiveCiAggregateViaGraphQl(env, REPO, SHA, TOKEN)).toBeNull();
Expand Down
16 changes: 13 additions & 3 deletions test/unit/salvageability.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,21 @@ describe("resolveAiReviewSalvageableHold", () => {
expect(hold!.comment).toContain("HELD with guidance");
});

it("defaults: no configured floor uses the gate default, and a confidence-less blocker counts as certainty (at/above floor)", () => {
// #9085 (landed via #9237): an absent confidence used to degrade to 1.0 -- maximum certainty -- so "the
// model said nothing" silently cleared even the default floor here. It now degrades to
// CONFIDENCE_WHEN_UNSTATED (0.5), which is sub-floor against the 0.93 gate default, so the low-confidence
// hold owns that case and this resolver stands down. #9085 renamed both sibling assertions in
// rules.test.ts but missed this third consumption site, leaving it asserting the pre-change semantics.
// Both calls are load-bearing for coverage: the first is the ONLY case in this file that exercises the
// `aiReviewCloseConfidence ?? DEFAULT_AI_REVIEW_CLOSE_CONFIDENCE` default arm (every other case passes an
// explicit floor), and the second is the only one exercising the `confidence ?? CONFIDENCE_WHEN_UNSTATED`
// nullish arm.
it("defaults: no configured floor uses the gate default (0.93), and a confidence-less blocker is sub-floor", () => {
expect(resolveAiReviewSalvageableHold(aiEval(0.99), { aiReviewSalvageabilityMinScore: 60 }, salv)).toBeDefined();

const noConfidence = aiEval(0.99);
delete (noConfidence.blockers[0] as { confidence?: number }).confidence;
const hold = resolveAiReviewSalvageableHold(noConfidence, { aiReviewSalvageabilityMinScore: 60 }, salv);
expect(hold).toBeDefined();
expect(resolveAiReviewSalvageableHold(noConfidence, { aiReviewSalvageabilityMinScore: 60 }, salv)).toBeUndefined();
});

it("knob unset (the default) or no score: never changes a disposition", () => {
Expand Down
9 changes: 7 additions & 2 deletions test/unit/selfhost-pg-retention.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,15 @@ function makeRetentionPgPool(remaining: Record<string, number> = {}): MockPgPool
return { rows: [{ n: remaining[table] ?? 0 }], rowCount: 1 };
}

const deleteMatch = /^DELETE FROM (\w+) WHERE ctid IN \(SELECT ctid FROM \1 WHERE .*? LIMIT (\d+)\)$/i.exec(q);
// #9083 (via #9237) changed the emitted shape: retention now deletes by PRIMARY KEY (`id`, `delivery_id`,
// ...) with an explicit `ORDER BY <retention column>`, falling back to the physical row key only for the
// composite-key tables that have no single-column PK -- an index-backed range delete replacing a
// full-scan-per-batch semi-join. Group 2 captures whichever key column is in play, so BOTH paths (mapped
// PK and the `ctid` fallback) stay exercised by this mock rather than one silently ceasing to match.
const deleteMatch = /^DELETE FROM (\w+) WHERE (\w+) IN \(SELECT \2 FROM \1 WHERE .*? ORDER BY \w+ LIMIT (\d+)\)$/i.exec(q);
if (deleteMatch) {
const table = deleteMatch[1] as string;
const limit = Number(deleteMatch[2]);
const limit = Number(deleteMatch[3]);
const have = remaining[table] ?? 0;
const changes = Math.min(have, limit);
remaining[table] = have - changes;
Expand Down
28 changes: 24 additions & 4 deletions test/unit/worker-entry-boundary.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,34 @@ describe("worker entry boundary", () => {
expect(forbidden, `worker entry must not reach agent-only modules: ${forbidden.join(", ")}`).toEqual([]);
});

it("does not reference pixelmatch, pngjs, visual-diff, gifenc, or sharp in worker-reachable source", () => {
// Scans MODULE SPECIFIERS, not raw file text. What this guards is the Worker BUNDLE: a Node-only dep can
// only get bundled by being imported (statically or dynamically), so parsing the same specifiers
// collectReachableSources already walks catches every real inclusion path. Grepping whole-file content
// instead produced a false positive the moment ordinary English prose contained one of these words --
// #9230 added the user-facing string "route(s) crossed the visual-diff threshold" to
// src/review/visual/visual-findings.ts (a file worker-reachable since #4120, importing none of these deps),
// and the raw-content regex failed a green tree over a sentence. Bending correct user-facing copy to dodge
// a test regex would have been the wrong repair; narrowing the check to what it actually means is the right
// one.
it("does not import pixelmatch, pngjs, visual-diff, gifenc, or sharp from worker-reachable source", () => {
const hits = collectReachableSources(WORKER_ENTRY)
.map((file) => {
const content = readFileSync(file, "utf8");
return FORBIDDEN_IDENTIFIERS.test(content) ? relativeToRoot(file) : null;
const offending = parseImportSpecifiers(file).filter((specifier) => FORBIDDEN_IDENTIFIERS.test(specifier));
return offending.length > 0 ? `${relativeToRoot(file)} (${offending.join(", ")})` : null;
})
.filter((entry): entry is string => entry !== null);
expect(hits, `worker-reachable files must not mention Node-only visual diff/GIF/image deps: ${hits.join(", ")}`).toEqual([]);
expect(hits, `worker-reachable files must not import Node-only visual diff/GIF/image deps: ${hits.join(", ")}`).toEqual([]);
});

// Proves the specifier-scoped check above is still DISCRIMINATING, not vacuously passing: the same regex
// must still flag a real dependency import, and must still ignore the same word in prose. Without this, a
// future edit that broke the matching entirely would look identical to a clean tree.
it("the forbidden-identifier check still flags a real import specifier and still ignores prose", () => {
expect(["sharp", "pixelmatch", "gifenc", "pngjs", "@foo/visual-diff"].every((specifier) => FORBIDDEN_IDENTIFIERS.test(specifier))).toBe(true);
expect(["./capture", "../../types", "node:fs", "hono"].some((specifier) => FORBIDDEN_IDENTIFIERS.test(specifier))).toBe(false);
// The exact #9230 prose that broke the old whole-file scan is not a module specifier, so it is correctly
// invisible to a specifier-scoped check -- while the bare dep name it contains still is not.
expect(parseImportSpecifiers(join(srcRoot, "review/visual/visual-findings.ts")).some((s) => FORBIDDEN_IDENTIFIERS.test(s))).toBe(false);
});

it("does not reference visual diff or GIF modules in the published MCP bin bundle", () => {
Expand Down