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
18 changes: 16 additions & 2 deletions src/queue/ai-review-orchestration.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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";
Expand Down Expand Up @@ -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
Expand Down
23 changes: 23 additions & 0 deletions src/review/judgment-agreement.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
39 changes: 39 additions & 0 deletions src/selfhost/posthog.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, unknown> | 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 = {
Expand Down
27 changes: 26 additions & 1 deletion test/unit/judgment-agreement.test.ts
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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([]);
});
});
64 changes: 64 additions & 0 deletions test/unit/selfhost-posthog.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ import {
forwardStructuredLogToPostHog,
initPostHog,
installPostHogStructuredLogForwarding,
capturePostHogAiMetric,
POSTHOG_AI_DEGRADED_EVENT,
POSTHOG_AI_METRIC_EVENT,
POSTHOG_MONITOR_HEARTBEAT_EVENT,
resetPostHogForTest,
resolvePostHogRelease,
Expand Down Expand Up @@ -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();
Expand Down