diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 4016764ec9..ec912735d6 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -2830,7 +2830,13 @@ export async function findHottestInconclusiveReviewTargetForRepo( eq(aiUsageEvents.feature, "ai_review_pr"), gte(aiUsageEvents.createdAt, sinceIso), sql`json_extract(${aiUsageEvents.metadataJson}, '$.repoFullName') = ${repoFullName}`, - sql`json_extract(${aiUsageEvents.metadataJson}, '$.inconclusive') = 1`, + // #4997: `inconclusive` is stored as a JSON boolean. SQLite's json_extract surfaces a JSON boolean as the + // SQL integer 1/0, but the self-host Postgres translation of json_extract (pg-dialect.ts) rewrites this to + // `->>'inconclusive'`, which ALWAYS returns text -- comparing that text to a bare integer literal throws a + // Postgres type-mismatch error on every call. CAST to TEXT first so both backends compare text to text: + // SQLite's CAST(1 AS TEXT) = '1' (identical to the old `= 1` semantics), Postgres's `->>'inconclusive'` + // already yields 'true'/'false'. + sql`CAST(json_extract(${aiUsageEvents.metadataJson}, '$.inconclusive') AS TEXT) IN ('1', 'true')`, ), ) .groupBy(pullNumberExpr) diff --git a/test/unit/db-parsers.test.ts b/test/unit/db-parsers.test.ts index 694f4336b5..052ea0419c 100644 --- a/test/unit/db-parsers.test.ts +++ b/test/unit/db-parsers.test.ts @@ -8,6 +8,7 @@ import { countRecentAuditEventsForActorAndTarget, countRecentAuditEventsForActorInRepo, countRecentAuditEventsForActorInRepoWithTargetSuffix, + findHottestInconclusiveReviewTargetForRepo, findHottestReviewTargetForRepo, hasAuditEventForDelivery, hasAuditEventForHeadSha, @@ -24,6 +25,7 @@ import { markPullRequestsRegated, markPullRequestsBacklogConvergenceRegated, markPullRequestSurfacePublished, + recordAiUsageEvent, recordAuditEvent, recordWebhookEvent, upsertOfficialMinerDetection, @@ -748,6 +750,39 @@ describe("database row parser hardening", () => { }); }); + it("findHottestInconclusiveReviewTargetForRepo counts only inconclusive:true calls, scoped to ONE repo (#4997 regression: the JSON-boolean comparison must exclude inconclusive:false, not just miss on total volume)", async () => { + const env = createTestEnv(); + const record = (repoFullName: string, pullNumber: number, inconclusive: boolean) => + recordAiUsageEvent(env, { + feature: "ai_review_pr", + model: "self-host:claude-code", + status: "ok", + estimatedNeurons: 100, + metadata: { repoFullName, pullNumber, inconclusive }, + }); + // owner/repo#1: 3 inconclusive calls -- the hottest inconclusive target for this repo. + await record("owner/repo", 1, true); + await record("owner/repo", 1, true); + await record("owner/repo", 1, true); + // owner/repo#1 ALSO has 2 successful (non-inconclusive) calls on the SAME PR -- must not inflate the count. + await record("owner/repo", 1, false); + await record("owner/repo", 1, false); + // owner/repo#2: only 1 inconclusive call -- must not win over #1. + await record("owner/repo", 2, true); + // A different repo's inconclusive calls must not leak into this repo's count. + await record("owner/other", 1, true); + await record("owner/other", 1, true); + + const hottest = await findHottestInconclusiveReviewTargetForRepo(env, "owner/repo", "2020-01-01T00:00:00.000Z"); + expect(hottest).toEqual({ targetKey: "owner/repo#1", count: 3 }); + + // A cutoff after all the recorded events must find nothing. + expect(await findHottestInconclusiveReviewTargetForRepo(env, "owner/repo", "2099-01-01T00:00:00.000Z")).toBeNull(); + // A repo with only non-inconclusive calls must find nothing. + await record("owner/all-ok", 1, false); + expect(await findHottestInconclusiveReviewTargetForRepo(env, "owner/all-ok", "2020-01-01T00:00:00.000Z")).toBeNull(); + }); + it("hasAuditEventForDelivery finds a matching deliveryId inside metadata_json, scoped to actor+eventType+targetKey (#2560)", async () => { const env = createTestEnv(); await recordAuditEvent(env, { diff --git a/test/unit/selfhost-pg-dialect.test.ts b/test/unit/selfhost-pg-dialect.test.ts index b5dcd1578c..b0a00c6e60 100644 --- a/test/unit/selfhost-pg-dialect.test.ts +++ b/test/unit/selfhost-pg-dialect.test.ts @@ -31,6 +31,19 @@ describe("pg-dialect (#977 SQLite → Postgres)", () => { expect(translateFunctions("json_extract(meta, '$.mode')")).toBe("((meta)::jsonb ->> 'mode')"); }); + it("REGRESSION (#4997): a JSON-boolean json_extract comparison survives translation as text-to-text, not text-to-integer", () => { + // findHottestInconclusiveReviewTargetForRepo (repositories.ts) compares a stored JSON boolean. SQLite's + // json_extract surfaces a JSON boolean as the SQL integer 1/0, but Postgres's `->>` ALWAYS returns text -- + // comparing that text against a bare integer literal (the original `= 1`) throws a Postgres type-mismatch + // error on every call. CAST to TEXT first so the comparison is valid on both backends. + const translated = translateFunctions("CAST(json_extract(metadata_json, '$.inconclusive') AS TEXT) IN ('1', 'true')"); + expect(translated).toBe("CAST(((metadata_json)::jsonb ->> 'inconclusive') AS TEXT) IN ('1', 'true')"); + // No bare-integer comparison against a json_extract/->> expression should remain anywhere in the codebase -- + // this is the ONE call site, and it's fixed. (Documents the invariant the fix restores; not itself testing + // translateFunctions with anything new.) + expect(translated).not.toMatch(/->>\s*'[a-z]+'\s*\)?\s*=\s*\d/); + }); + it("translates INSERT OR IGNORE / REPLACE to ON CONFLICT", () => { expect(translateInsertOr("INSERT OR IGNORE INTO t (a) VALUES (?)")).toBe("INSERT INTO t (a) VALUES (?) ON CONFLICT DO NOTHING"); const replace = translateInsertOr("INSERT OR REPLACE INTO system_flags (key, value, updated_at) VALUES (?, '1', CURRENT_TIMESTAMP)");