diff --git a/src/queue/ai-review-orchestration.ts b/src/queue/ai-review-orchestration.ts index 6137c2009..e3951f898 100644 --- a/src/queue/ai-review-orchestration.ts +++ b/src/queue/ai-review-orchestration.ts @@ -27,7 +27,7 @@ import { buildPullRequestAdvisory } from "../rules/advisory"; import { recordAuditEvent, getDecryptedRepositoryAiKey, getRepository, listCheckSummaries, listPullRequestFiles } from "../db/repositories"; import { registerHeldLock, unregisterHeldLock } from "./held-lock-registry"; import { recordRoutingShadow } from "../services/reviewer-routing"; -import { scoreJudgmentAgreement } from "../review/judgment-agreement"; +import { judgmentAgreementMetrics, scoreJudgmentAgreement } from "../review/judgment-agreement"; import { persistDecisionReplayPrompt } from "../review/decision-replay"; import { createInstallationToken } from "../github/app"; import type { AgentActionMode } from "../settings/agent-execution"; @@ -68,7 +68,7 @@ import { resolveEnrichmentLinkedIssue, resolveEnrichmentLinkedIssueNumbers, } from "../review/enrichment-wire"; -import { capturePostHogReviewFailure } from "../selfhost/posthog"; +import { capturePostHogAiMetric, capturePostHogReviewFailure } from "../selfhost/posthog"; import { isReputationEnabled, shouldSkipAiForReputation } from "../review/reputation-wire"; import { isConvergenceRepoAllowed } from "../review/cutover-gate"; import { resolveConvergedFeature } from "../review/feature-activation"; @@ -973,6 +973,20 @@ export async function runAiReviewForAdvisory( detail: vote.votedFail ? "flagged a blocking defect" : "did not flag a blocking defect", metadata: { repoFullName: args.repoFullName, vote: vote.votedFail ? "fail" : "non_fail" }, }).catch(() => undefined); + // #10226: the same stance, onto the review's own AI trace, so PostHog can read cost and model against + // whether the review actually flagged anything. Best-effort like the audit write above -- the capture is + // a no-op when PostHog is off and never throws. + capturePostHogAiMetric({ + name: "reviewer_vote_fail", + value: vote.votedFail ? 1 : 0, + context: { repo: args.repoFullName, pullNumber: args.pr.number, agent: vote.reviewer }, + }); + } + // #10226: inter-run agreement, emitted ONCE per review rather than per finding. The helper decides what is + // worth reporting -- an uncorroborated review yields nothing, so no fabricated agreement floor lands in an + // average alongside real scores. + for (const metric of judgmentAgreementMetrics([...result.reviewerVotes, ...result.selfConsistencySamples])) { + capturePostHogAiMetric({ ...metric, context: { repo: args.repoFullName, pullNumber: args.pr.number } }); } // #8229 stage 1: the report-only routing shadow — records what evidence-weighted routing WOULD have // preferred for this repo (audit metadata only; the recap aggregates it). Same best-effort discipline diff --git a/src/review/judgment-agreement.ts b/src/review/judgment-agreement.ts index 8b1dbaa7a..801bd9bbd 100644 --- a/src/review/judgment-agreement.ts +++ b/src/review/judgment-agreement.ts @@ -64,6 +64,29 @@ export function scoreJudgmentAgreement(samples: readonly JudgmentSample[], verba return { agreement, confidence: agreement * stated, sampleCount, uncorroborated }; } +/** + * The quality metrics this review's judgment agreement is worth reporting to PostHog (#10226). PURE — the + * caller does the emitting. + * + * Returns an EMPTY list for an uncorroborated review rather than reporting the agreement floor: with fewer + * than two samples there is nothing to agree WITH, and `UNCORROBORATED_AGREEMENT` is a deliberate placeholder, + * not a measurement. Publishing it as one would put a fabricated 0.5 into an average alongside real scores. + * + * Only the confidence-INDEPENDENT half of the score is reported. `agreement` and `sampleCount` are properties + * of the stances themselves; `confidence` folds in a per-finding verbalized confidence, so it belongs to a + * finding rather than to the review. + */ +export function judgmentAgreementMetrics( + samples: readonly JudgmentSample[], +): Array<{ name: string; value: number }> { + const scored = scoreJudgmentAgreement(samples, 1); + if (scored.uncorroborated) return []; + return [ + { name: "judgment_agreement", value: scored.agreement }, + { name: "judgment_sample_count", value: scored.sampleCount }, + ]; +} + // NOT IMPLEMENTED HERE, deliberately (#8834): the issue also describes running N=2-3 evaluations of the SAME // judge with few-shot exemplars rotated out of the golden corpus ("simulated annotators"). That is a strictly // better agreement signal than two different models voting once each — it isolates the judge's own diff --git a/src/selfhost/posthog.ts b/src/selfhost/posthog.ts index 8abc9ceb7..585c9455a 100644 --- a/src/selfhost/posthog.ts +++ b/src/selfhost/posthog.ts @@ -484,6 +484,45 @@ export type PostHogAiDegradationReason = "circuit_open" | "chain_exhausted"; export const POSTHOG_AI_DEGRADED_EVENT = "selfhost_ai_degraded"; +export const POSTHOG_AI_METRIC_EVENT = "$ai_metric"; + +/** One review-quality measurement, joined to the AI trace that produced it (#10226). + * + * Rich quality signal -- reviewer stances, inter-run agreement, precision -- already reaches SQL, Grafana and + * the maintainer recap, but never PostHog, so cost and model could not be read against whether the review was + * any GOOD. `$ai_metric` is PostHog's own event for exactly this, and the property contract below is taken + * verbatim from the SDK's `captureTraceMetric` (@posthog/core): name, value, trace id -- with the value + * STRINGIFIED, which is the SDK's own choice, matched here so a hand-built event and an SDK-built one are + * indistinguishable downstream. + * + * The trace id defaults to the ambient OTel trace, the same one every generation under this review already + * carries, so the metric lands on the trace rather than floating free. No trace, no event -- an orphan + * quality score joins to nothing and would only inflate counts. */ +export function capturePostHogAiMetric(event: { + name: string; + value: number | string | boolean; + /** Extra low-cardinality context (e.g. which reviewer) -- routed through the shared operational allowlist, + * so anything not on it is dropped exactly like every other capture path in this file. */ + context?: Record | undefined; +}): void { + if (!active || !client) return; + const traceId = currentOtelTraceIds()?.trace_id; + if (!traceId) return; + const operational = operationalProperties(event.context); + client.capture({ + distinctId: POSTHOG_DISTINCT_ID, + event: POSTHOG_AI_METRIC_EVENT, + properties: { + ...operational, + $ai_trace_id: traceId, + $ai_metric_name: event.name, + $ai_metric_value: String(event.value), + environment: posthogEnvironment, + }, + ...repoGroup(operational), + }); +} + /** One AI request that produced NO generation. Same metadata-only posture as * {@link PostHogAiGenerationEvent}: provider/model ids, the reason, and an already-redacted error string. */ export type PostHogAiDegradationEvent = { diff --git a/test/unit/judgment-agreement.test.ts b/test/unit/judgment-agreement.test.ts index 984a5d9aa..df42512a7 100644 --- a/test/unit/judgment-agreement.test.ts +++ b/test/unit/judgment-agreement.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { scoreJudgmentAgreement, UNCORROBORATED_AGREEMENT } from "../../src/review/judgment-agreement"; +import { judgmentAgreementMetrics, scoreJudgmentAgreement, UNCORROBORATED_AGREEMENT } from "../../src/review/judgment-agreement"; // #8834: the per-decision confidence signal. The contract worth pinning is that the score can never read as // MORE certain than its weakest input, and that a run with nothing to corroborate it is never scored as @@ -64,3 +64,28 @@ describe("scoreJudgmentAgreement (#8834)", () => { } }); }); + +describe("judgmentAgreementMetrics (#10226 — what is worth reporting to PostHog)", () => { + const sample = (votedFail: boolean): { reviewer: string; votedFail: boolean } => ({ reviewer: "claude-code", votedFail }); + + it("reports agreement and sample count for a corroborated review", () => { + expect(judgmentAgreementMetrics([sample(true), sample(true), sample(false)])).toEqual([ + { name: "judgment_agreement", value: 2 / 3 }, + { name: "judgment_sample_count", value: 3 }, + ]); + }); + + it("reports unanimity as a real 1.0, not a special case", () => { + expect(judgmentAgreementMetrics([sample(true), sample(true)])).toEqual([ + { name: "judgment_agreement", value: 1 }, + { name: "judgment_sample_count", value: 2 }, + ]); + }); + + it("reports NOTHING for an uncorroborated review rather than publishing the agreement floor", () => { + // UNCORROBORATED_AGREEMENT (0.5) is a deliberate placeholder, not a measurement. Emitting it would drop a + // fabricated 0.5 into an average alongside real scores. + expect(judgmentAgreementMetrics([sample(true)])).toEqual([]); + expect(judgmentAgreementMetrics([])).toEqual([]); + }); +}); diff --git a/test/unit/selfhost-posthog.test.ts b/test/unit/selfhost-posthog.test.ts index a9df1e2f6..e91069542 100644 --- a/test/unit/selfhost-posthog.test.ts +++ b/test/unit/selfhost-posthog.test.ts @@ -32,7 +32,9 @@ import { forwardStructuredLogToPostHog, initPostHog, installPostHogStructuredLogForwarding, + capturePostHogAiMetric, POSTHOG_AI_DEGRADED_EVENT, + POSTHOG_AI_METRIC_EVENT, POSTHOG_MONITOR_HEARTBEAT_EVENT, resetPostHogForTest, resolvePostHogRelease, @@ -874,6 +876,68 @@ describe("capturePostHogAiDegradation (#10186 — a request that reached NO mode }); }); +describe("capturePostHogAiMetric (#10226 — review quality, joined to the AI trace)", () => { + it("is a no-op when PostHog is unconfigured", () => { + capturePostHogAiMetric({ name: "reviewer_vote_fail", value: 1 }); + expect(mocks.capture).not.toHaveBeenCalled(); + }); + + it("is a no-op with no ambient trace — an orphan quality score joins to nothing", async () => { + otelMocks.currentOtelTraceIds.mockReturnValue(undefined); + await initPostHog({ POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + capturePostHogAiMetric({ name: "reviewer_vote_fail", value: 1 }); + expect(mocks.capture).not.toHaveBeenCalled(); + }); + + it("emits the SDK's exact property contract, with the value stringified", async () => { + otelMocks.currentOtelTraceIds.mockReturnValue({ trace_id: "review-trace-1", span_id: "span-1" }); + await initPostHog({ POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + capturePostHogAiMetric({ name: "judgment_agreement", value: 0.75, context: { repo: "owner/repo", pullNumber: 7 } }); + const call = mocks.capture.mock.calls[0]?.[0]; + expect(call.event).toBe(POSTHOG_AI_METRIC_EVENT); + // @posthog/core's captureTraceMetric stringifies the value; a hand-built event must be + // indistinguishable from an SDK-built one downstream. + expect(call.properties.$ai_metric_name).toBe("judgment_agreement"); + expect(call.properties.$ai_metric_value).toBe("0.75"); + expect(call.properties.$ai_trace_id).toBe("review-trace-1"); + expect(call.properties.repo).toBe("owner/repo"); + expect(call.groups).toEqual({ repo: "owner/repo" }); + }); + + it("stringifies a numeric zero and a boolean rather than dropping them", async () => { + otelMocks.currentOtelTraceIds.mockReturnValue({ trace_id: "t", span_id: "s" }); + await initPostHog({ POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + capturePostHogAiMetric({ name: "reviewer_vote_fail", value: 0 }); + capturePostHogAiMetric({ name: "flagged", value: false }); + // A "did not flag" vote is real signal, not an absent one. + expect(mocks.capture.mock.calls[0]?.[0].properties.$ai_metric_value).toBe("0"); + expect(mocks.capture.mock.calls[1]?.[0].properties.$ai_metric_value).toBe("false"); + }); + + it("omits the repo group when the metric has no repo context", async () => { + otelMocks.currentOtelTraceIds.mockReturnValue({ trace_id: "t", span_id: "s" }); + await initPostHog({ POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + capturePostHogAiMetric({ name: "reviewer_vote_fail", value: 1 }); + expect("groups" in (mocks.capture.mock.calls[0]?.[0] as object)).toBe(false); + }); + + it("drops a context key that is not on the shared operational allowlist", async () => { + otelMocks.currentOtelTraceIds.mockReturnValue({ trace_id: "t", span_id: "s" }); + await initPostHog({ POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + capturePostHogAiMetric({ name: "reviewer_vote_fail", value: 1, context: { notAllowlisted: "dropped" } }); + expect("notAllowlisted" in mocks.capture.mock.calls[0]?.[0].properties).toBe(false); + }); + + it("never carries prompt/response content", async () => { + otelMocks.currentOtelTraceIds.mockReturnValue({ trace_id: "t", span_id: "s" }); + await initPostHog({ POSTHOG_API_KEY: "phc_test_key" } as unknown as NodeJS.ProcessEnv); + capturePostHogAiMetric({ name: "reviewer_vote_fail", value: 1 }); + const keys = Object.keys(mocks.capture.mock.calls[0]?.[0].properties); + expect(keys).not.toContain("$ai_input"); + expect(keys).not.toContain("$ai_output_choices"); + }); +}); + describe("flushPostHog / shutdownPostHog", () => { it("flushPostHog is a no-op when unconfigured", async () => { await flushPostHog();