From e3246123f638bb271e6fcdf880b77f57a36218a1 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 3 Aug 2026 03:34:38 +0530 Subject: [PATCH 1/5] feat(review): telemetry for review feature usage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review feature emitted nothing: neither cli/cmd/review.ts nor the engine had any Telemetry reference, and the CLI path creates no session so it never even picked up session_start. We could not answer how often review runs, from where, what it finds, or whether results get published. Two events, emitted from one helper shared by both engine callers — the `review` command and the `dbt_pr_review` tool, distinguished by `invocation`. Instrumenting only the command would have missed every review the agent runs through the tool. review_run one per engine invocation, status completed | failed review_post_outcome publication, CLI path only Caller attribution needs no code at all. The envelope already seeds `source` from Flag.ALTIMATE_CLI_CLIENT, and an event only overrides it by declaring its own `source` field — so these events deliberately declare none, and a caller that exports ALTIMATE_CLI_CLIENT is attributed automatically. That is what let this drop an earlier design carrying a global envelope property, a CLI-wide entrypoint registry, a reviewPullRequest() signature change and typed errors through the engine. None of it was needed. Details that are easy to get wrong, and why they are the way they are: - Publication is a separate event. It happens after the review is computed and can partially succeed, so it cannot honestly be a field on review_run, and a publish failure must not mark a computed review as failed. - `partial` covers every degraded post state. PostResult cannot distinguish them: postError is not cleared when the retry succeeds, and an inline fallback coexists with a real reviewId. - Only two failure reasons plus a fallback. The engine degrades rather than throwing for missing manifests, dispatcher failures and the AI lane, so buckets for those would never fire. Config is matched on its fixed throw prefix and git on the child-process spawn identity, not loose message matching. - `by_category` is zero-filled from ReviewCategory.options and drops unrecognised keys, so a rule that never fired is distinguishable from one that was not possible, and a malformed category cannot mint a new dimension. - `tier_forced` normalises absent to false; the schema treats explicit false as invalid. - `degraded` is the envelope's fidelity flag — no reviewable files, no usable manifest for the changed models, or a surfaced finding whose analysis was undecidable. It does not mean "no warehouse". - The engine call is timed alone; output writing and posting are excluded. Privacy: enums, booleans and counts only. Review findings are about customer schema, and a test asserts that file paths, model and column names, titles and bodies do not appear in the serialized event. The attribution test asserts the serialized customDimensions.source rather than the tracked object, because the envelope seed is invisible to a track() spy — and that seed is the premise the whole design rests on. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb --- .../opencode/src/altimate/review/telemetry.ts | 134 +++++++++++ .../opencode/src/altimate/telemetry/index.ts | 49 ++++ .../src/altimate/tools/dbt-pr-review.ts | 38 ++- packages/opencode/src/cli/cmd/review.ts | 68 ++++-- .../test/altimate/review/telemetry.test.ts | 220 ++++++++++++++++++ 5 files changed, 484 insertions(+), 25 deletions(-) create mode 100644 packages/opencode/src/altimate/review/telemetry.ts create mode 100644 packages/opencode/test/altimate/review/telemetry.test.ts diff --git a/packages/opencode/src/altimate/review/telemetry.ts b/packages/opencode/src/altimate/review/telemetry.ts new file mode 100644 index 0000000000..100f64d841 --- /dev/null +++ b/packages/opencode/src/altimate/review/telemetry.ts @@ -0,0 +1,134 @@ +// altimate_change start — review feature telemetry. +// +// The review engine has two callers: the `review` CLI command and the `dbt_pr_review` tool. They +// share this helper so there is one telemetry contract rather than two that drift — the zero-fill, +// the privacy filtering and the failure classification all live here. +// +// Caller attribution needs no code: neither event declares a `source` field, so the envelope's +// process-level `source` (from Flag.ALTIMATE_CLI_CLIENT) passes through untouched. A caller that +// exports that variable is attributed automatically; one that does not reports `cli`. +import { Telemetry } from "../telemetry" +import { ReviewCategory, type Finding } from "./finding" +import type { VerdictEnvelope } from "./verdict" +import type { PostResult } from "./post-github" + +export type ReviewInvocation = "cli" | "tool" + +/** + * Count surfaced findings by category, zero-filled across the whole enum. + * + * Zero-filled so a category that never fires is distinguishable from one that was never possible + * in this run — an absent key and a zero mean different things to whoever reads the dashboard. + * Keys come from `ReviewCategory.options`, never from the finding values themselves: + * `Telemetry.aggregateFindings` accepts arbitrary strings and returns only observed keys, so a + * malformed category would otherwise become a new dimension. + */ +function countByCategory(findings: Finding[]): Record { + const counts: Record = {} + for (const category of ReviewCategory.options) counts[category] = 0 + for (const finding of findings) { + if (finding.category in counts) counts[finding.category] += 1 + } + return counts +} + +/** + * Classify a thrown review failure without threading typed errors through the engine. + * + * Only two failure modes actually propagate — everything else in the engine degrades rather than + * throwing (missing manifests, dispatcher failures and the AI lane are all caught and turned into + * empty or degraded results). So this deliberately recognises two and calls the rest `error` + * rather than inventing buckets that can never occur. + * + * Matching is on the fixed prefix the config loader throws with, and on the spawn identity of the + * git child process — not broad substring matching over the message, which would drift the moment + * anything is reworded. + */ +export function classifyReviewFailure(err: unknown): "config_error" | "git_error" | "error" { + const message = err instanceof Error ? err.message : String(err) + if (message.startsWith("Failed to load")) return "config_error" + const cmd = (err as { cmd?: unknown } | undefined)?.cmd + if (typeof cmd === "string" && /(^|[\\/\s])git(\s|$)/.test(cmd)) return "git_error" + if (message.startsWith("git ") || message.includes("git diff")) return "git_error" + return "error" +} + +/** + * Map a PostResult onto the outcome enum. + * + * `PostResult` cannot express finer states than this: an inline fallback and a recorded post error + * can coexist with a real review id, and `postError` is not cleared when the retry succeeds. So + * everything short of a clean full post collapses to `partial` rather than pretending to a + * precision the shape does not have. A throw before the summary is posted never reaches here — the + * caller reports `summary_failed` for that. + */ +export function classifyPostOutcome(result: PostResult): "full" | "partial" { + if (result.inlineFellBack || result.postError || result.reviewId === undefined) return "partial" + return "full" +} + +/** Emitted once per engine invocation, whichever caller reached it. */ +export function emitReviewRun(input: { + invocation: ReviewInvocation + durationMs: number + /** Empty on the CLI path, which has no chat session. */ + sessionID: string + envelope?: VerdictEnvelope + error?: unknown +}): void { + try { + const base = { + type: "review_run" as const, + timestamp: Date.now(), + session_id: input.sessionID, + invocation: input.invocation, + duration_ms: input.durationMs, + } + + if (!input.envelope) { + Telemetry.track({ ...base, status: "failed", reason: classifyReviewFailure(input.error) }) + return + } + + const env = input.envelope + Telemetry.track({ + ...base, + status: "completed", + verdict: env.verdict, + ideal_verdict: env.idealVerdict, + // The effective mode, which config can set — not whatever the caller passed as a flag. + mode: env.mode, + tier: env.tier, + // Optional in the schema and explicitly invalid as `false`, so normalise rather than copy. + tier_forced: env.tierForced === true, + degraded: env.summary.degraded, + stale_manifest: env.staleManifest === true, + critical: env.summary.critical, + warning: env.summary.warning, + suggestion: env.summary.suggestion, + by_category: countByCategory(env.findings), + }) + } catch { + // Telemetry must never fail a review. + } +} + +/** Emitted on the CLI path only — the tool does not publish. */ +export function emitReviewPostOutcome(input: { + outcome: "not_requested" | "target_unresolved" | "full" | "partial" | "summary_failed" + durationMs: number + sessionID: string +}): void { + try { + Telemetry.track({ + type: "review_post_outcome", + timestamp: Date.now(), + session_id: input.sessionID, + outcome: input.outcome, + duration_ms: input.durationMs, + }) + } catch { + // Telemetry must never fail a review. + } +} +// altimate_change end diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts index 971ee11342..1c318aa627 100644 --- a/packages/opencode/src/altimate/telemetry/index.ts +++ b/packages/opencode/src/altimate/telemetry/index.ts @@ -906,6 +906,55 @@ export namespace Telemetry { } // altimate_change end + // altimate_change start — review feature usage. + // + // Deliberately NO `source` field on either event: the envelope seeds `source` from + // Flag.ALTIMATE_CLI_CLIENT and an event-declared `source` would override it. Leaving it off is + // what makes caller attribution work with no other code — a plugin setting + // ALTIMATE_CLI_CLIENT is already attributed. + | { + type: "review_run" + timestamp: number + /** Real session on the tool path; empty for the CLI command, which has no chat session. */ + session_id: string + /** Which caller reached the engine. `source` says who launched the process; this says how + * review was invoked within it. */ + invocation: "cli" | "tool" + status: "completed" | "failed" + duration_ms: number + /** Present when status is `completed`. */ + verdict?: string + ideal_verdict?: string + mode?: string + tier?: string + tier_forced?: boolean + /** The envelope's fidelity flag: no reviewable files, no usable manifest for the changed + * models, OR a surfaced finding whose engine analysis was undecidable. It does NOT mean + * merely "no warehouse". */ + degraded?: boolean + stale_manifest?: boolean + critical?: number + warning?: number + suggestion?: number + /** JSON object of the 14-value ReviewCategory enum, zero-filled. Counts surfaced findings + * after dedupe, rubric exclusion and severity threshold — not raw rule detections, and + * not rule-level: `Finding` does not retain a rule key. */ + by_category?: Record + /** Present when status is `failed`. */ + reason?: "config_error" | "git_error" | "error" + } + | { + type: "review_post_outcome" + timestamp: number + session_id: string + /** `partial` covers every "not fully posted as attempted" state PostResult can express — + * inline comments fell back, a post error was recorded, or no review id came back. The + * shape cannot distinguish finer outcomes than that. */ + outcome: "not_requested" | "target_unresolved" | "full" | "partial" | "summary_failed" + duration_ms: number + } + // altimate_change end + /** SHA256 hash a masked error message for anonymous grouping. */ // altimate_change start — provider identity for the onboarding funnel. // diff --git a/packages/opencode/src/altimate/tools/dbt-pr-review.ts b/packages/opencode/src/altimate/tools/dbt-pr-review.ts index 82df71adcd..16ce290242 100644 --- a/packages/opencode/src/altimate/tools/dbt-pr-review.ts +++ b/packages/opencode/src/altimate/tools/dbt-pr-review.ts @@ -2,6 +2,8 @@ import z from "zod" import { Tool } from "../../tool/tool" import { Instance } from "../../project/instance" import { reviewPullRequest } from "../review/run" +// altimate_change — review feature telemetry +import { emitReviewRun } from "../review/telemetry" import { renderSummary, verdictHeadline } from "../review/format" import { ReviewMode } from "../review/verdict" @@ -35,14 +37,36 @@ export const DbtPrReviewTool = Tool.define("dbt_pr_review", { }), async execute(args, ctx) { const cwd = Instance.directory - const env = await reviewPullRequest({ - cwd, - base: args.base, - head: args.head, - manifestPath: args.manifest_path, - mode: args.mode, - modelVersion: ctx.agent, + // altimate_change start — same review_run event as the CLI path, distinguished by + // `invocation`. Instrumenting only cli/cmd/review.ts would miss every review the agent runs + // through this tool, which is a real share of usage. Unlike the CLI, this path has a session. + const startedAt = Date.now() + let env + try { + env = await reviewPullRequest({ + cwd, + base: args.base, + head: args.head, + manifestPath: args.manifest_path, + mode: args.mode, + modelVersion: ctx.agent, + }) + } catch (err) { + emitReviewRun({ + invocation: "tool", + durationMs: Date.now() - startedAt, + sessionID: ctx.sessionID, + error: err, + }) + throw err + } + emitReviewRun({ + invocation: "tool", + durationMs: Date.now() - startedAt, + sessionID: ctx.sessionID, + envelope: env, }) + // altimate_change end return { title: verdictHeadline(env), metadata: { diff --git a/packages/opencode/src/cli/cmd/review.ts b/packages/opencode/src/cli/cmd/review.ts index f8a295b861..4ded5e6bae 100644 --- a/packages/opencode/src/cli/cmd/review.ts +++ b/packages/opencode/src/cli/cmd/review.ts @@ -6,6 +6,8 @@ import { Installation } from "../../installation" import { reviewPullRequest } from "../../altimate/review/run" import { renderSummary } from "../../altimate/review/format" import { postGitHubReview, resolveGitHubTarget } from "../../altimate/review/post-github" +// altimate_change — review feature telemetry +import { classifyPostOutcome, emitReviewPostOutcome, emitReviewRun } from "../../altimate/review/telemetry" import type { ReviewMode } from "../../altimate/review/verdict" import type { Severity } from "../../altimate/review/finding" @@ -74,23 +76,33 @@ export const ReviewCommand = cmd({ ) } await bootstrap(cwd, async () => { - const env = await reviewPullRequest({ - cwd, - base: args.base as string | undefined, - head: args.head as string | undefined, - manifestPath: args.manifest as string | undefined, - mode: args.mode as ReviewMode | undefined, - severityThreshold: args.severity as Severity | undefined, - // With `boolean-negation: false` above, `--no-ai` binds to `noAi` and - // the historical `--ai=false` programmatic path stays supported. - noAi: args.noAi === true || args.ai === false, - explainTier: args.explainTier === true, - forceTier: args.forceTier as "trivial" | "lite" | "full" | undefined, - // Stamp the CLI version into engine.cliVersion so an auditor can - // reconstruct which policy version generated a stored verdict long - // after the binary that ran it is gone. - cliVersion: Installation.VERSION, - }) + // altimate_change — time the engine only. Output writing and posting happen after this and + // must not be counted as review latency, nor turn a computed review into a failed one. + const startedAt = Date.now() + let env + try { + env = await reviewPullRequest({ + cwd, + base: args.base as string | undefined, + head: args.head as string | undefined, + manifestPath: args.manifest as string | undefined, + mode: args.mode as ReviewMode | undefined, + severityThreshold: args.severity as Severity | undefined, + // With `boolean-negation: false` above, `--no-ai` binds to `noAi` and + // the historical `--ai=false` programmatic path stays supported. + noAi: args.noAi === true || args.ai === false, + explainTier: args.explainTier === true, + forceTier: args.forceTier as "trivial" | "lite" | "full" | undefined, + // Stamp the CLI version into engine.cliVersion so an auditor can + // reconstruct which policy version generated a stored verdict long + // after the binary that ran it is gone. + cliVersion: Installation.VERSION, + }) + } catch (err) { + emitReviewRun({ invocation: "cli", durationMs: Date.now() - startedAt, sessionID: "", error: err }) + throw err + } + emitReviewRun({ invocation: "cli", durationMs: Date.now() - startedAt, sessionID: "", envelope: env }) if (args.output) await fs.writeFile(args.output as string, JSON.stringify(env, null, 2)) @@ -101,14 +113,34 @@ export const ReviewCommand = cmd({ process.stdout.write(renderSummary(env) + "\n") } + // altimate_change — publication is its own event: it happens after the review is computed + // and can partially succeed, so it must not fold into review_run. + const postStartedAt = Date.now() + const postDuration = () => Date.now() - postStartedAt + if (!args.post) { + emitReviewPostOutcome({ outcome: "not_requested", durationMs: 0, sessionID: "" }) + } if (args.post) { const target = await resolveGitHubTarget() if (!target) { + emitReviewPostOutcome({ outcome: "target_unresolved", durationMs: postDuration(), sessionID: "" }) UI.println( "⚠️ --post requested but GITHUB_TOKEN / GITHUB_REPOSITORY / PR number could not be resolved; skipping post.", ) } else { - const r = await postGitHubReview(env, target) + let r + try { + r = await postGitHubReview(env, target) + } catch (err) { + // A throw here means the summary comment itself failed; nothing was published. + emitReviewPostOutcome({ outcome: "summary_failed", durationMs: postDuration(), sessionID: "" }) + throw err + } + emitReviewPostOutcome({ + outcome: classifyPostOutcome(r), + durationMs: postDuration(), + sessionID: "", + }) const where = `${target.owner}/${target.repo}#${target.prNumber}` if (r.postError) { UI.println(`⚠️ Posted the summary comment to ${where}, but the review event failed: ${r.postError}`) diff --git a/packages/opencode/test/altimate/review/telemetry.test.ts b/packages/opencode/test/altimate/review/telemetry.test.ts new file mode 100644 index 0000000000..b14bd86493 --- /dev/null +++ b/packages/opencode/test/altimate/review/telemetry.test.ts @@ -0,0 +1,220 @@ +// altimate_change — review feature telemetry. +// +// The load-bearing test here is the last one: caller attribution works only because these events +// do NOT declare a `source` field, so the envelope's process-level value survives. That is +// invisible to a Telemetry.track spy — it only appears after serialization — so it is asserted at +// the transport level. +import { describe, expect, test, afterEach, spyOn, mock } from "bun:test" +import { Telemetry } from "@/altimate/telemetry" +import { + classifyPostOutcome, + classifyReviewFailure, + emitReviewPostOutcome, + emitReviewRun, +} from "@/altimate/review/telemetry" +import { ReviewCategory } from "@/altimate/review/finding" + +function captureEvents() { + const events: Telemetry.Event[] = [] + spyOn(Telemetry, "track").mockImplementation((e: Telemetry.Event) => { + events.push(e) + }) + return events +} + +/** Minimal envelope with only what the emitter reads. */ +function envelope(over: Record = {}) { + return { + verdict: "COMMENT", + idealVerdict: "REQUEST_CHANGES", + mode: "comment", + tier: "full", + summary: { critical: 1, warning: 2, suggestion: 0, degraded: false }, + findings: [ + { category: "join_risk", severity: "critical" }, + { category: "join_risk", severity: "warning" }, + { category: "sql_quality", severity: "warning" }, + ], + ...over, + } as any +} + +afterEach(() => mock.restore()) + +describe("review_run", () => { + test("a completed run reports the envelope's own values", () => { + const events = captureEvents() + emitReviewRun({ invocation: "cli", durationMs: 1234, sessionID: "", envelope: envelope() }) + + const e = events[0] as any + expect(e.type).toBe("review_run") + expect(e.status).toBe("completed") + expect(e.invocation).toBe("cli") + expect(e.verdict).toBe("COMMENT") + // The pre-gating verdict is what shows whether `comment` mode softened a block. + expect(e.ideal_verdict).toBe("REQUEST_CHANGES") + expect(e.critical).toBe(1) + expect(e.duration_ms).toBe(1234) + }) + + test("tier_forced normalises absent to false", () => { + // The schema allows only `true` or absent — `false` is explicitly invalid — so copying the + // raw field would put `undefined` in the event for the common case. + const events = captureEvents() + emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", envelope: envelope() }) + expect((events[0] as any).tier_forced).toBe(false) + + events.length = 0 + emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", envelope: envelope({ tierForced: true }) }) + expect((events[0] as any).tier_forced).toBe(true) + }) + + test("by_category is zero-filled across the whole enum", () => { + const events = captureEvents() + emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", envelope: envelope() }) + + const byCategory = (events[0] as any).by_category + // Zero-filled so "this rule never fired" is distinguishable from "this rule was not possible". + expect(Object.keys(byCategory).sort()).toEqual([...ReviewCategory.options].sort()) + expect(byCategory.join_risk).toBe(2) + expect(byCategory.sql_quality).toBe(1) + expect(byCategory.pii_exposure).toBe(0) + }) + + test("an unrecognised category cannot create a new dimension", () => { + const events = captureEvents() + emitReviewRun({ + invocation: "cli", + durationMs: 1, + sessionID: "", + envelope: envelope({ findings: [{ category: "not_a_real_category", severity: "warning" }] }), + }) + + const byCategory = (events[0] as any).by_category + expect(byCategory.not_a_real_category).toBeUndefined() + expect(Object.keys(byCategory)).toHaveLength(ReviewCategory.options.length) + }) + + test("the tool path carries its session, the CLI path does not", () => { + const events = captureEvents() + emitReviewRun({ invocation: "tool", durationMs: 1, sessionID: "ses_abc", envelope: envelope() }) + expect((events[0] as any).session_id).toBe("ses_abc") + expect((events[0] as any).invocation).toBe("tool") + }) + + test("a failed run reports a reason and no envelope fields", () => { + const events = captureEvents() + emitReviewRun({ invocation: "cli", durationMs: 5, sessionID: "", error: new Error("boom") }) + + const e = events[0] as any + expect(e.status).toBe("failed") + expect(e.reason).toBe("error") + expect(e.verdict).toBeUndefined() + expect(e.by_category).toBeUndefined() + }) + + test("no schema identifier reaches the event", () => { + const events = captureEvents() + emitReviewRun({ + invocation: "cli", + durationMs: 1, + sessionID: "", + envelope: envelope({ + findings: [ + { + category: "pii_exposure", + severity: "critical", + file: "models/marts/customers.sql", + model: "customers", + column: "email", + title: "PII exposed", + body: "column email is now selected", + }, + ], + }), + }) + + // Review findings are about customer schema; the serialized event must contain none of it. + const serialized = JSON.stringify(events[0]) + for (const leak of ["models/marts", "customers", "email", "PII exposed", "now selected"]) { + expect(serialized).not.toContain(leak) + } + }) +}) + +describe("failure classification", () => { + test("the config loader's fixed prefix", () => { + expect(classifyReviewFailure(new Error("Failed to load .altimate/review.yml: bad yaml"))).toBe("config_error") + }) + + test("a git child-process failure by spawn identity, not message text", () => { + const err = Object.assign(new Error("Command failed"), { cmd: "git diff --name-status" }) + expect(classifyReviewFailure(err)).toBe("git_error") + }) + + test("anything else is `error` rather than an invented bucket", () => { + // The engine degrades rather than throwing for missing manifests, dispatcher failures and the + // AI lane, so there are no buckets for those — they never arrive here. + expect(classifyReviewFailure(new Error("something unexpected"))).toBe("error") + expect(classifyReviewFailure("not even an error")).toBe("error") + }) +}) + +describe("post outcome", () => { + test("a clean post is full", () => { + expect(classifyPostOutcome({ reviewId: 1, inlineFellBack: false })).toBe("full") + }) + + test("every degraded state collapses to partial", () => { + // PostResult cannot distinguish these: postError is not cleared when the retry succeeds, and + // an inline fallback coexists with a real reviewId. Claiming finer resolution would be a lie. + expect(classifyPostOutcome({ reviewId: 1, inlineFellBack: true })).toBe("partial") + expect(classifyPostOutcome({ reviewId: 1, inlineFellBack: false, postError: "429" })).toBe("partial") + expect(classifyPostOutcome({ inlineFellBack: false })).toBe("partial") + }) +}) + +describe("caller attribution", () => { + afterEach(async () => { + await Telemetry.shutdown() + mock.restore() + }) + + test("the process client source reaches the serialized event", async () => { + // This is what makes attribution free: the events declare no `source` field, so the envelope's + // seed survives. Asserted after serialization because a Telemetry.track spy cannot see it. + const origDisabled = process.env.ALTIMATE_TELEMETRY_DISABLED + const origCs = process.env.APPLICATIONINSIGHTS_CONNECTION_STRING + const origClient = process.env.ALTIMATE_CLI_CLIENT + const bodies: string[] = [] + const fetchMock = spyOn(global, "fetch").mockImplementation((async (_i: any, init: any) => { + bodies.push(String(init?.body ?? "")) + return new Response("", { status: 200 }) + }) as unknown as typeof fetch) + + try { + delete process.env.ALTIMATE_TELEMETRY_DISABLED + process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = + "InstrumentationKey=k;IngestionEndpoint=https://example.invalid" + process.env.ALTIMATE_CLI_CLIENT = "plugin:claude-code" + await Telemetry.init() + + emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", envelope: envelope() }) + emitReviewPostOutcome({ outcome: "not_requested", durationMs: 0, sessionID: "" }) + await Telemetry.flush() + + const envelopes = JSON.parse(bodies[0]) as any[] + const run = envelopes.find((e) => e.data.baseData.name === "review_run") + const post = envelopes.find((e) => e.data.baseData.name === "review_post_outcome") + expect(run.data.baseData.properties.source).toBe("plugin:claude-code") + expect(post.data.baseData.properties.source).toBe("plugin:claude-code") + } finally { + process.env.ALTIMATE_TELEMETRY_DISABLED = origDisabled + if (origCs !== undefined) process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = origCs + else delete process.env.APPLICATIONINSIGHTS_CONNECTION_STRING + if (origClient !== undefined) process.env.ALTIMATE_CLI_CLIENT = origClient + else delete process.env.ALTIMATE_CLI_CLIENT + fetchMock.mockRestore() + } + }) +}) From 2bee2118872b2334944b1cbcbd8702a0082330e6 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 3 Aug 2026 03:36:46 +0530 Subject: [PATCH 2/5] test(review): end-to-end telemetry against a real review process MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Opt-in via ALTIMATE_E2E=1; skipped otherwise. ~13s. Runs a real `altimate-code review` against a real git repo with its telemetry endpoint pointed at a local sink, and asserts the envelopes that arrive over HTTP. No PTY needed, unlike the onboarding funnel tests — review is a one-shot command. This is the only test that can prove the design's central claim. Caller attribution works because the events declare no `source` field, so the envelope's process-level value survives serialization. A Telemetry.track spy cannot see that, and no in-process test can show that a variable set in a caller's environment reaches a separate process at all. Here the run is spawned with nothing but ALTIMATE_CLI_CLIENT set, and the arriving event carries source=plugin:claude-code. Also asserts the shape as actually serialized rather than as intended: invocation, status, a numeric duration measurement, by_category as a 14-key JSON string per house convention, and a post-outcome event honestly reporting not_requested. Finally it greps the whole payload for the fixture's file name, changed SQL and repo path, since review findings are about customer schema. Confirmed while building it: a two-line diff takes ~9s of engine time, and a repo with a dbt_project.yml but no compiled manifest reports degraded=true — which is why that field is documented as a fidelity flag rather than "no warehouse". Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb --- .../test/e2e/review-telemetry.e2e.test.ts | 124 ++++++++++++++++++ 1 file changed, 124 insertions(+) create mode 100644 packages/opencode/test/e2e/review-telemetry.e2e.test.ts diff --git a/packages/opencode/test/e2e/review-telemetry.e2e.test.ts b/packages/opencode/test/e2e/review-telemetry.e2e.test.ts new file mode 100644 index 0000000000..45ea290d21 --- /dev/null +++ b/packages/opencode/test/e2e/review-telemetry.e2e.test.ts @@ -0,0 +1,124 @@ +// altimate_change — end-to-end review telemetry. +// +// Opt-in via ALTIMATE_E2E=1. Runs a real `altimate-code review` process against a real git repo +// with its telemetry endpoint pointed at a local sink, and asserts the envelopes that actually +// arrive over HTTP. +// +// This is the only test that can prove the central design claim: caller attribution works because +// these events declare no `source` field, so the envelope's process-level value survives +// serialization. A Telemetry.track spy cannot see that, and a unit test cannot prove the flag set +// by a caller's environment reaches a separate process at all. +// +// No PTY needed, unlike the onboarding funnel tests — review is a one-shot command. +import { describe, expect, test } from "bun:test" +import { mkdtemp, writeFile, mkdir, rm } from "node:fs/promises" +import { tmpdir } from "node:os" +import path from "node:path" + +const enabled = process.env.ALTIMATE_E2E === "1" + +type Captured = { name: string; properties: Record; measurements: Record } + +function startSink() { + const envelopes: Captured[] = [] + const server = Bun.serve({ + port: 0, + async fetch(req) { + if (!req.url.endsWith("/v2/track")) return new Response("", { status: 404 }) + for (const item of (await req.json()) as any[]) { + const base = item?.data?.baseData ?? {} + envelopes.push({ + name: String(base.name ?? ""), + properties: base.properties ?? {}, + measurements: base.measurements ?? {}, + }) + } + return new Response("", { status: 200 }) + }, + }) + return { envelopes, url: `http://127.0.0.1:${server.port}`, stop: () => server.stop(true) } +} + +/** A git repo with one committed dbt model and an uncommitted change to review. */ +async function fixtureRepo(home: string) { + const repo = await mkdtemp(path.join(tmpdir(), "review-e2e-repo-")) + const git = (...args: string[]) => Bun.spawnSync(["git", ...args], { cwd: repo, env: { ...process.env, HOME: home } }) + git("init", "-q") + git("config", "user.email", "e2e@example.invalid") + git("config", "user.name", "e2e") + await mkdir(path.join(repo, "models"), { recursive: true }) + await writeFile(path.join(repo, "dbt_project.yml"), "name: e2e\nversion: '1.0'\nprofile: e2e\n") + await writeFile(path.join(repo, "models/orders.sql"), "select 1 as id\n") + git("add", "-A") + git("commit", "-qm", "init") + await writeFile(path.join(repo, "models/orders.sql"), "select 1 as id, 2 as amount\n") + return repo +} + +describe.skipIf(!enabled)("review telemetry (e2e)", () => { + test( + "a real review run reports its caller, and the caller set no CLI flag to do it", + async () => { + const sink = startSink() + // Throwaway HOME so the run cannot read or write the developer's real credentials or + // machine-id. + const home = await mkdtemp(path.join(tmpdir(), "review-e2e-home-")) + const repo = await fixtureRepo(home) + + try { + const proc = Bun.spawn( + [process.execPath, "run", "--conditions=browser", "src/index.ts", "review", "--cwd", repo], + { + // Run from packages/opencode so bun picks up the workspace bunfig.toml for the JSX + // runtime, exactly as the `dev` script does. + cwd: path.resolve(import.meta.dir, "../.."), + stdout: "pipe", + stderr: "pipe", + env: { + ...process.env, + HOME: home, + APPLICATIONINSIGHTS_CONNECTION_STRING: `InstrumentationKey=e2e;IngestionEndpoint=${sink.url}`, + ALTIMATE_TELEMETRY_DISABLED: "false", + // The whole point: only the caller's environment is set. Nothing in the CLI knows + // about this value. + ALTIMATE_CLI_CLIENT: "plugin:claude-code", + }, + }, + ) + expect(await proc.exited).toBe(0) + await Bun.sleep(500) + + const run = sink.envelopes.find((e) => e.name === "review_run") + expect(run).toBeDefined() + + // Attribution with no attribution code — this is the claim the design rests on. + expect(run!.properties.source).toBe("plugin:claude-code") + + expect(run!.properties.invocation).toBe("cli") + expect(run!.properties.status).toBe("completed") + expect(run!.properties.verdict).toBeTruthy() + expect(typeof run!.measurements.duration_ms).toBe("number") + + // Zero-filled across the enum, and serialized as a JSON string per house convention. + const byCategory = JSON.parse(run!.properties.by_category) + expect(Object.keys(byCategory).length).toBe(14) + + // Publication is its own event and reports honestly that none was requested. + const post = sink.envelopes.find((e) => e.name === "review_post_outcome") + expect(post).toBeDefined() + expect(post!.properties.outcome).toBe("not_requested") + + // Findings are about customer schema; none of it may reach telemetry. + const serialized = JSON.stringify(sink.envelopes) + for (const leak of ["orders.sql", "as amount", repo]) { + expect(serialized).not.toContain(leak) + } + } finally { + sink.stop() + await rm(home, { recursive: true, force: true }).catch(() => {}) + await rm(repo, { recursive: true, force: true }).catch(() => {}) + } + }, + 180_000, + ) +}) From 9411d8b8ab8833f028fdb38d5ac1ea6191b66698 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 3 Aug 2026 03:48:55 +0530 Subject: [PATCH 3/5] docs(review): document the review telemetry events MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Required by the contributor checklist on this page, and the page is a user-facing contract: it publishes what the CLI collects and what it never collects, so adding events without listing them makes it untrue. Also records three things that are easy to misread from the field names alone: - `degraded` is a fidelity flag, not a warehouse flag. Confirmed end to end — a repo with a dbt_project.yml but no compiled manifest reports degraded=true. - The category breakdown counts surfaced findings, after de-duplication, rubric exclusion and the severity threshold, grouped by category rather than by rule. `Finding` does not retain a rule key, so rule-level effectiveness is not measurable from this. - The tool path also emits the standard `tool_call` event for the same review, so dashboards should count `review_run`, not both. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb --- docs/docs/reference/telemetry.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/docs/docs/reference/telemetry.md b/docs/docs/reference/telemetry.md index b2688fe24c..34f6d5a827 100644 --- a/docs/docs/reference/telemetry.md +++ b/docs/docs/reference/telemetry.md @@ -62,6 +62,19 @@ We collect the following categories of events: | `activation_job_selected` / `first_job_completed` | Which activation job the user started and, where observable, finished. Completion is reported only for the job that was actually selected, so the two form a coherent pair. **Derived** — see the note below. | | `first_prompt_sent` | The user's first typed message in an onboarding session. Slash commands are excluded, so the hidden `/onboard-connect` submission does not count. | | `onboarding_abandoned` | The CLI exited during a first run without connecting. `last_stage` is the furthest point reached: `started`, `model_picker`, `provider_setup`, `big_pickle_confirm`, or `gateway_auth`. (`connected` is a funnel position but never a `last_stage` — reaching it means the run completed, which is not an abandonment.) Only emitted for a genuine first run — opening `/connect` as an existing user does not enter the funnel, and abandonment after setup completes is out of scope by definition. Emitted on the exit path under a bounded flush, so the measured rate is a lower bound — see [Delivery & Reliability](#delivery--reliability). | +| `review_run` | A dbt/SQL review completed or failed — `invocation` (`cli` for `altimate-code review`, `tool` for the `dbt_pr_review` tool), status, duration, and on success the verdict, the pre-gating verdict, mode, risk tier, and finding counts by severity and by category. No file paths, model or column names, finding titles or bodies, SQL, diff content, or repository/branch/PR names. | +| `review_post_outcome` | Whether a review was published to GitHub — `not_requested`, `target_unresolved`, `full`, `partial`, or `summary_failed`, plus duration. No repository, PR, or comment content. | + +### Notes on the review events + +- `degraded` is a fidelity flag, not a warehouse flag. It is set when a review found no reviewable + files, had no usable manifest for the changed models, or surfaced a finding whose analysis was + undecidable. It does not mean "no warehouse was connected". +- The category breakdown counts findings that were actually surfaced — after de-duplication, rubric + exclusion, and the severity threshold. It is not a count of raw rule detections, and it is grouped + by category rather than by individual rule. +- Reviews run through the `dbt_pr_review` tool also emit the standard `tool_call` event. They are + the same review; count `review_run` rather than both. ### A note on the derived activation events From e21c0ff826a6e9bd4e2632c86e12e6aa27f81934 Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 3 Aug 2026 05:30:07 +0530 Subject: [PATCH 4/5] fix(review): address consensus review findings on #1064 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Major: - `countByCategory` used an object literal plus `in`, so every `Object.prototype` member passed the allowlist: a finding categorised `toString` both minted a dimension and evaluated ` + 1` into a `Record`. Now `Object.create(null)` + `Object.hasOwn`. Zod makes this unreachable today; the guard exists for when validation is bypassed. - `review_post_outcome` had no stated cardinality and three paths that skipped it. The contract is now written down — exactly one per **completed** review, so absence means the review failed rather than that an event was lost — and enforced with a latch plus a `finally` rather than by control flow that only looked exhaustive. `not_requested` moved ahead of the `--output` write and the stdout render; a new `not_attempted` bucket covers a run that dies between the completed review and the post attempt; a throwing `resolveGitHubTarget()` reports `target_unresolved`. Minor: - `classifyReviewFailure` dropped the `message.includes("git diff")` fallback its own docstring disclaimed. It was unreachable for the real git path — `execFile` always sets `cmd`, and its message begins `Command failed: `, so the `cmd` check returns first. - Added the adversarial prototype-key case to the guard test; the existing ordinary-string case cannot reach it. - Added coverage that a throwing `Telemetry.track` cannot propagate out of either emitter, which is what the two empty `catch` blocks promise. Nits: - Deleted the e2e's 500 ms sleep. `proc.exited` already implies the flush landed: the CLI awaits `shutdown()` → `flush()` → the sink's HTTP response. - `ALTIMATE_TELEMETRY_DISABLED` restored conditionally; unconditional assignment wrote the literal string `"undefined"` when the variable was originally absent. - `postStartedAt` moved inside `if (args.post)`, and the `if`/`if` pair is now a single branch. - Corrected the e2e comment claiming the object-valued `by_category` follows the house convention — the sibling map-shaped fields stringify at the call site. Also covers three of the review's flagged test gaps: `stale_manifest` / `degraded` field mapping, and CLI-level control flow through a real process — a new e2e drives `--post` with an unwritable `--output` and asserts exactly one post outcome. Both new unit tests and the new e2e were mutation-checked against their fixes. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb --- docs/docs/reference/telemetry.md | 2 +- .../opencode/src/altimate/review/telemetry.ts | 29 +++-- .../opencode/src/altimate/telemetry/index.ts | 6 +- packages/opencode/src/cli/cmd/review.ts | 101 +++++++++++------- .../test/altimate/review/telemetry.test.ts | 77 ++++++++++++- .../test/e2e/review-telemetry.e2e.test.ts | 71 +++++++++++- 6 files changed, 238 insertions(+), 48 deletions(-) diff --git a/docs/docs/reference/telemetry.md b/docs/docs/reference/telemetry.md index 34f6d5a827..2ff9138de5 100644 --- a/docs/docs/reference/telemetry.md +++ b/docs/docs/reference/telemetry.md @@ -63,7 +63,7 @@ We collect the following categories of events: | `first_prompt_sent` | The user's first typed message in an onboarding session. Slash commands are excluded, so the hidden `/onboard-connect` submission does not count. | | `onboarding_abandoned` | The CLI exited during a first run without connecting. `last_stage` is the furthest point reached: `started`, `model_picker`, `provider_setup`, `big_pickle_confirm`, or `gateway_auth`. (`connected` is a funnel position but never a `last_stage` — reaching it means the run completed, which is not an abandonment.) Only emitted for a genuine first run — opening `/connect` as an existing user does not enter the funnel, and abandonment after setup completes is out of scope by definition. Emitted on the exit path under a bounded flush, so the measured rate is a lower bound — see [Delivery & Reliability](#delivery--reliability). | | `review_run` | A dbt/SQL review completed or failed — `invocation` (`cli` for `altimate-code review`, `tool` for the `dbt_pr_review` tool), status, duration, and on success the verdict, the pre-gating verdict, mode, risk tier, and finding counts by severity and by category. No file paths, model or column names, finding titles or bodies, SQL, diff content, or repository/branch/PR names. | -| `review_post_outcome` | Whether a review was published to GitHub — `not_requested`, `target_unresolved`, `full`, `partial`, or `summary_failed`, plus duration. No repository, PR, or comment content. | +| `review_post_outcome` | Whether a review was published to GitHub — `not_requested`, `not_attempted`, `target_unresolved`, `full`, `partial`, or `summary_failed`, plus duration. Exactly one per **completed** review: a review that failed emits `review_run: failed` and no post event, so absence means the review failed rather than that an event was lost. `not_attempted` is publication requested but never reached (a bad `--output` path, a stdout write error). No repository, PR, or comment content. | ### Notes on the review events diff --git a/packages/opencode/src/altimate/review/telemetry.ts b/packages/opencode/src/altimate/review/telemetry.ts index 100f64d841..fc20cc3b17 100644 --- a/packages/opencode/src/altimate/review/telemetry.ts +++ b/packages/opencode/src/altimate/review/telemetry.ts @@ -24,10 +24,14 @@ export type ReviewInvocation = "cli" | "tool" * malformed category would otherwise become a new dimension. */ function countByCategory(findings: Finding[]): Record { - const counts: Record = {} + // Prototype-less, and membership tested with Object.hasOwn: `{}` plus `in` accepted every + // Object.prototype member, so a finding categorised `toString` both minted a dimension and + // evaluated ` + 1` into a Record. Zod makes that unreachable + // today, but this guard exists precisely for the case where validation was bypassed. + const counts: Record = Object.create(null) for (const category of ReviewCategory.options) counts[category] = 0 for (const finding of findings) { - if (finding.category in counts) counts[finding.category] += 1 + if (Object.hasOwn(counts, finding.category)) counts[finding.category] += 1 } return counts } @@ -41,15 +45,19 @@ function countByCategory(findings: Finding[]): Record { * rather than inventing buckets that can never occur. * * Matching is on the fixed prefix the config loader throws with, and on the spawn identity of the - * git child process — not broad substring matching over the message, which would drift the moment - * anything is reworded. + * git child process (`err.cmd`, set by `execFile`) — not broad substring matching over the + * message, which would drift the moment anything is reworded. A `message.includes("git diff")` + * fallback used to sit below the `cmd` check; it was unreachable for the real git path (execFile + * always sets `cmd`, and its message begins "Command failed: ") and contradicted this paragraph. + * + * The `Failed to load` prefix is itself string matching. It is accurate against the config loader + * today; a typed error at the throw site is what would make it robust. */ export function classifyReviewFailure(err: unknown): "config_error" | "git_error" | "error" { const message = err instanceof Error ? err.message : String(err) if (message.startsWith("Failed to load")) return "config_error" const cmd = (err as { cmd?: unknown } | undefined)?.cmd if (typeof cmd === "string" && /(^|[\\/\s])git(\s|$)/.test(cmd)) return "git_error" - if (message.startsWith("git ") || message.includes("git diff")) return "git_error" return "error" } @@ -113,9 +121,16 @@ export function emitReviewRun(input: { } } -/** Emitted on the CLI path only — the tool does not publish. */ +/** + * Emitted on the CLI path only — the tool does not publish. + * + * CONTRACT: exactly one of these per *completed* review, never more and never fewer. A review that + * threw never reached a publication phase, so it gets `review_run: failed` and no post event — + * absence therefore means "the review failed", not "telemetry was lost". The caller enforces the + * once-ness with a latch plus a `finally`; see cli/cmd/review.ts. + */ export function emitReviewPostOutcome(input: { - outcome: "not_requested" | "target_unresolved" | "full" | "partial" | "summary_failed" + outcome: "not_requested" | "not_attempted" | "target_unresolved" | "full" | "partial" | "summary_failed" durationMs: number sessionID: string }): void { diff --git a/packages/opencode/src/altimate/telemetry/index.ts b/packages/opencode/src/altimate/telemetry/index.ts index 1c318aa627..bb4c9ee467 100644 --- a/packages/opencode/src/altimate/telemetry/index.ts +++ b/packages/opencode/src/altimate/telemetry/index.ts @@ -950,7 +950,11 @@ export namespace Telemetry { /** `partial` covers every "not fully posted as attempted" state PostResult can express — * inline comments fell back, a post error was recorded, or no review id came back. The * shape cannot distinguish finer outcomes than that. */ - outcome: "not_requested" | "target_unresolved" | "full" | "partial" | "summary_failed" + /** `not_attempted`: publication was requested, but the invocation died between the + * completed review and the post attempt (a bad `--output` path, a stdout write error). + * Emitted from the caller's `finally` so a completed review always carries exactly one + * post outcome. */ + outcome: "not_requested" | "not_attempted" | "target_unresolved" | "full" | "partial" | "summary_failed" duration_ms: number } // altimate_change end diff --git a/packages/opencode/src/cli/cmd/review.ts b/packages/opencode/src/cli/cmd/review.ts index 4ded5e6bae..717588c3dd 100644 --- a/packages/opencode/src/cli/cmd/review.ts +++ b/packages/opencode/src/cli/cmd/review.ts @@ -104,54 +104,83 @@ export const ReviewCommand = cmd({ } emitReviewRun({ invocation: "cli", durationMs: Date.now() - startedAt, sessionID: "", envelope: env }) - if (args.output) await fs.writeFile(args.output as string, JSON.stringify(env, null, 2)) - - // Primary output → stdout (pipeable). Diagnostics below → stderr via UI. - if (args.json) { - process.stdout.write(JSON.stringify(env, null, 2) + "\n") - } else { - process.stdout.write(renderSummary(env) + "\n") + // altimate_change start — publication is its own event: it happens after the review is + // computed and can partially succeed, so it must not fold into review_run. + // + // CONTRACT: exactly one review_post_outcome per COMPLETED review. A review that threw + // returned above with `review_run: failed` and never reached a publication phase, so the + // absence of a post event means "the review failed" and never "telemetry was lost". + // + // Enforced by a latch plus the finally below rather than by the control flow being + // obviously exhaustive — it was not. The `not_requested` emit used to sit AFTER the + // `--output` write and the stdout render, so a bad `--output` path produced a completed + // review with no post event at all, indistinguishable from a dropped event. + let postOutcomeEmitted = false + function emitPostOnce(outcome: Parameters[0]["outcome"], durationMs: number) { + if (postOutcomeEmitted) return + postOutcomeEmitted = true + emitReviewPostOutcome({ outcome, durationMs, sessionID: "" }) } - // altimate_change — publication is its own event: it happens after the review is computed - // and can partially succeed, so it must not fold into review_run. - const postStartedAt = Date.now() - const postDuration = () => Date.now() - postStartedAt - if (!args.post) { - emitReviewPostOutcome({ outcome: "not_requested", durationMs: 0, sessionID: "" }) - } - if (args.post) { - const target = await resolveGitHubTarget() - if (!target) { - emitReviewPostOutcome({ outcome: "target_unresolved", durationMs: postDuration(), sessionID: "" }) - UI.println( - "⚠️ --post requested but GITHUB_TOKEN / GITHUB_REPOSITORY / PR number could not be resolved; skipping post.", - ) + try { + // Emitted before anything that can throw, so the no-publication case cannot be lost. + if (!args.post) emitPostOnce("not_requested", 0) + + if (args.output) await fs.writeFile(args.output as string, JSON.stringify(env, null, 2)) + + // Primary output → stdout (pipeable). Diagnostics below → stderr via UI. + if (args.json) { + process.stdout.write(JSON.stringify(env, null, 2) + "\n") } else { - let r + process.stdout.write(renderSummary(env) + "\n") + } + + if (args.post) { + // Started here, not above: the `not_requested` path reports 0 and never reads these, and + // capturing them earlier made that hardcoded 0 look like an oversight. + const postStartedAt = Date.now() + const postDuration = () => Date.now() - postStartedAt + let target try { - r = await postGitHubReview(env, target) + target = await resolveGitHubTarget() } catch (err) { - // A throw here means the summary comment itself failed; nothing was published. - emitReviewPostOutcome({ outcome: "summary_failed", durationMs: postDuration(), sessionID: "" }) + // Defensive today — the resolver returns undefined rather than throwing — but the + // contract should not rest on that staying true. No summary was attempted. + emitPostOnce("target_unresolved", postDuration()) throw err } - emitReviewPostOutcome({ - outcome: classifyPostOutcome(r), - durationMs: postDuration(), - sessionID: "", - }) - const where = `${target.owner}/${target.repo}#${target.prNumber}` - if (r.postError) { - UI.println(`⚠️ Posted the summary comment to ${where}, but the review event failed: ${r.postError}`) - } else { + if (!target) { + emitPostOnce("target_unresolved", postDuration()) UI.println( - `Posted review to ${where}` + - (r.inlineFellBack ? " (inline comments fell back to summary-only)" : ""), + "⚠️ --post requested but GITHUB_TOKEN / GITHUB_REPOSITORY / PR number could not be resolved; skipping post.", ) + } else { + let r + try { + r = await postGitHubReview(env, target) + } catch (err) { + // A throw here means the summary comment itself failed; nothing was published. + emitPostOnce("summary_failed", postDuration()) + throw err + } + emitPostOnce(classifyPostOutcome(r), postDuration()) + const where = `${target.owner}/${target.repo}#${target.prNumber}` + if (r.postError) { + UI.println(`⚠️ Posted the summary comment to ${where}, but the review event failed: ${r.postError}`) + } else { + UI.println( + `Posted review to ${where}` + + (r.inlineFellBack ? " (inline comments fell back to summary-only)" : ""), + ) + } } } + } finally { + // Anything that threw between the completed review and the post attempt — a bad + // `--output` path, a stdout write error. Latched, so a real outcome always wins. + emitPostOnce("not_attempted", 0) } + // altimate_change end // Gate: exit non-zero when blocking, so CI fails the check. if (env.mode === "gate" && env.verdict === "REQUEST_CHANGES") { diff --git a/packages/opencode/test/altimate/review/telemetry.test.ts b/packages/opencode/test/altimate/review/telemetry.test.ts index b14bd86493..d1560e4144 100644 --- a/packages/opencode/test/altimate/review/telemetry.test.ts +++ b/packages/opencode/test/altimate/review/telemetry.test.ts @@ -95,6 +95,54 @@ describe("review_run", () => { expect(Object.keys(byCategory)).toHaveLength(ReviewCategory.options.length) }) + test("a category naming an Object.prototype member cannot slip past the guard", () => { + // The ordinary-string case above cannot catch this: `{}` plus `in` returns true for every + // prototype member, so `toString` both minted a dimension AND made `counts[k] += 1` evaluate + // ` + 1` — a string inside a Record. Fails before the + // Object.create(null) / Object.hasOwn fix. + const events = captureEvents() + emitReviewRun({ + invocation: "cli", + durationMs: 1, + sessionID: "", + envelope: envelope({ + findings: [ + { category: "toString", severity: "warning" }, + { category: "constructor", severity: "warning" }, + { category: "valueOf", severity: "warning" }, + { category: "__proto__", severity: "warning" }, + ], + }), + }) + + const byCategory = (events[0] as any).by_category + expect(Object.keys(byCategory)).toHaveLength(ReviewCategory.options.length) + for (const v of Object.values(byCategory)) expect(typeof v).toBe("number") + }) + + test("stale_manifest and degraded are carried from the envelope", () => { + // Same `=== true` normalisation as tier_forced, which has its own test; these two had none, + // and the shared envelope() helper omits staleManifest so every other test covers only the + // undefined case. + const events = captureEvents() + emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", envelope: envelope() }) + expect((events[0] as any).stale_manifest).toBe(false) + expect((events[0] as any).degraded).toBe(false) + + events.length = 0 + emitReviewRun({ + invocation: "cli", + durationMs: 1, + sessionID: "", + envelope: envelope({ + staleManifest: true, + summary: { critical: 0, warning: 0, suggestion: 0, degraded: true }, + }), + }) + expect((events[0] as any).stale_manifest).toBe(true) + expect((events[0] as any).degraded).toBe(true) + }) + test("the tool path carries its session, the CLI path does not", () => { const events = captureEvents() emitReviewRun({ invocation: "tool", durationMs: 1, sessionID: "ses_abc", envelope: envelope() }) @@ -142,6 +190,22 @@ describe("review_run", () => { }) }) +describe("telemetry failure isolation", () => { + // The two empty catch blocks in the emitters are the "observability must never break + // functionality" guarantee. Removing either one fails these and nothing else. + test("a throwing Telemetry.track cannot propagate out of either emitter", () => { + spyOn(Telemetry, "track").mockImplementation(() => { + throw new Error("buffer full") + }) + + expect(() => + emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", envelope: envelope() }), + ).not.toThrow() + expect(() => emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", error: new Error("x") })).not.toThrow() + expect(() => emitReviewPostOutcome({ outcome: "not_requested", durationMs: 0, sessionID: "" })).not.toThrow() + }) +}) + describe("failure classification", () => { test("the config loader's fixed prefix", () => { expect(classifyReviewFailure(new Error("Failed to load .altimate/review.yml: bad yaml"))).toBe("config_error") @@ -152,6 +216,13 @@ describe("failure classification", () => { expect(classifyReviewFailure(err)).toBe("git_error") }) + test("a git-shaped message without a cmd is not a git error", () => { + // The message fallback that used to classify this was unreachable for the real git path + // (execFile always sets `cmd`, and its message starts "Command failed: ") and contradicted + // the docstring's promise not to substring-match. Removed. + expect(classifyReviewFailure(new Error("git diff exploded"))).toBe("error") + }) + test("anything else is `error` rather than an invented bucket", () => { // The engine degrades rather than throwing for missing manifests, dispatcher failures and the // AI lane, so there are no buckets for those — they never arrive here. @@ -209,7 +280,11 @@ describe("caller attribution", () => { expect(run.data.baseData.properties.source).toBe("plugin:claude-code") expect(post.data.baseData.properties.source).toBe("plugin:claude-code") } finally { - process.env.ALTIMATE_TELEMETRY_DISABLED = origDisabled + // Unlike the two restores below, this was unconditional: an originally-absent variable + // came back as the string "undefined", leaking a disabled-telemetry flag into later tests + // and any child process they spawn. + if (origDisabled !== undefined) process.env.ALTIMATE_TELEMETRY_DISABLED = origDisabled + else delete process.env.ALTIMATE_TELEMETRY_DISABLED if (origCs !== undefined) process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = origCs else delete process.env.APPLICATIONINSIGHTS_CONNECTION_STRING if (origClient !== undefined) process.env.ALTIMATE_CLI_CLIENT = origClient diff --git a/packages/opencode/test/e2e/review-telemetry.e2e.test.ts b/packages/opencode/test/e2e/review-telemetry.e2e.test.ts index 45ea290d21..ddcd940f98 100644 --- a/packages/opencode/test/e2e/review-telemetry.e2e.test.ts +++ b/packages/opencode/test/e2e/review-telemetry.e2e.test.ts @@ -85,8 +85,10 @@ describe.skipIf(!enabled)("review telemetry (e2e)", () => { }, }, ) + // No sleep needed: the CLI awaits Telemetry.shutdown() in its top-level finally, + // shutdown() awaits flush(), flush() awaits the sink's HTTP response, and the sink records + // the envelopes before responding. By the time exit resolves, the request has completed. expect(await proc.exited).toBe(0) - await Bun.sleep(500) const run = sink.envelopes.find((e) => e.name === "review_run") expect(run).toBeDefined() @@ -99,7 +101,10 @@ describe.skipIf(!enabled)("review telemetry (e2e)", () => { expect(run!.properties.verdict).toBeTruthy() expect(typeof run!.measurements.duration_ms).toBe("number") - // Zero-filled across the enum, and serialized as a JSON string per house convention. + // Zero-filled across the enum. The field is declared Record and the + // envelope's object branch stringifies it on the way out — the sibling map-shaped fields + // (sql_quality.by_category, dbt_materialization_dist) declare `string` and stringify at + // the call site instead. Wire bytes are identical; the two shapes are not. const byCategory = JSON.parse(run!.properties.by_category) expect(Object.keys(byCategory).length).toBe(14) @@ -121,4 +126,66 @@ describe.skipIf(!enabled)("review telemetry (e2e)", () => { }, 180_000, ) + + test( + "a completed review always carries exactly one post outcome, even when the run dies after it", + async () => { + const sink = startSink() + const home = await mkdtemp(path.join(tmpdir(), "review-e2e-home-")) + const repo = await fixtureRepo(home) + + try { + // `--output` into a directory that does not exist. The write sits between the completed + // review and the post attempt, and before the fix it threw there with `review_run: + // completed` already emitted and no post event at all — indistinguishable, downstream, + // from a dropped event or an older client. + const proc = Bun.spawn( + [ + process.execPath, + "run", + "--conditions=browser", + "src/index.ts", + "review", + "--cwd", + repo, + "--post", + "--output", + path.join(repo, "no-such-dir", "verdict.json"), + ], + { + cwd: path.resolve(import.meta.dir, "../.."), + stdout: "pipe", + stderr: "pipe", + env: { + ...process.env, + HOME: home, + APPLICATIONINSIGHTS_CONNECTION_STRING: `InstrumentationKey=e2e;IngestionEndpoint=${sink.url}`, + ALTIMATE_TELEMETRY_DISABLED: "false", + ALTIMATE_CLI_CLIENT: "plugin:claude-code", + }, + }, + ) + // Non-zero: the write error propagates. Telemetry still flushes from the top-level finally. + expect(await proc.exited).not.toBe(0) + + const run = sink.envelopes.find((e) => e.name === "review_run") + expect(run).toBeDefined() + expect(run!.properties.status).toBe("completed") + + const posts = sink.envelopes.filter((e) => e.name === "review_post_outcome") + expect(posts).toHaveLength(1) + expect(posts[0]!.properties.outcome).toBe("not_attempted") + + const serialized = JSON.stringify(sink.envelopes) + for (const leak of ["orders.sql", "as amount", repo]) { + expect(serialized).not.toContain(leak) + } + } finally { + sink.stop() + await rm(home, { recursive: true, force: true }).catch(() => {}) + await rm(repo, { recursive: true, force: true }).catch(() => {}) + } + }, + 180_000, + ) }) From f08d139831a63c9e258eb87ab6d2dd7ac7693bbf Mon Sep 17 00:00:00 2001 From: Haider Date: Mon, 3 Aug 2026 13:30:53 +0530 Subject: [PATCH 5/5] fix(review): scope the post-outcome invariant to the CLI, de-order the attribution test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bot review on #1064. Four findings, all valid; two are the same class already fixed on the sibling onboarding PR. - `telemetry.md`: the `review_post_outcome` row claimed "exactly one per **completed** review, so absence means the review failed". That invariant holds only on the CLI path — the `dbt_pr_review` tool completes reviews and never publishes by design, so an analyst correlating events would have read every tool-invoked review as a failure. Scoped to the CLI and the tool path called out explicitly. This was a real error in the wording I added in `4ba07a7`. - `telemetry.md`: the general "each event includes a timestamp, session ID, CLI version, machine ID" sentence had ended up *inside* the review-specific subsection, reading as review-only. Hoisted above it. - `telemetry.test.ts`: the caller-attribution test called the real `Telemetry.init()` without clearing the memoized `initPromise` first, so any earlier init in the same process — including one under `ALTIMATE_TELEMETRY_DISABLED` — silently won and the connection string set by the test was ignored. It also relied on a sibling describe's `afterEach` having undone the `Telemetry.track` spy. Now restores mocks (before installing its own fetch spy, not after), shuts down to clear `initPromise`, asserts `isEnabled()` so a future regression fails with a cause, and reads across all request bodies rather than `bodies[0]`. - Same test: real `init()` writes `~/.altimate/machine-id`, so running the unit suite minted an identity the developer's own CLI would then reuse. `HOME` now points at a temp dir, restored and removed in `finally` — the pattern the e2e in this PR already used. Proven rather than assumed: adding a prior `init()` to this same file makes the attribution test fail without the `shutdown()` and pass with it. Verified: turbo typecheck clean; 921 tests across review/telemetry/upstream/ branding pass; all three `analyze.ts` gates (`--markers --strict`, `--branding`, `--require-markers --strict`) exit 0. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb --- docs/docs/reference/telemetry.md | 6 ++-- .../test/altimate/review/telemetry.test.ts | 32 ++++++++++++++++++- 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/docs/docs/reference/telemetry.md b/docs/docs/reference/telemetry.md index 2ff9138de5..0ef5926aba 100644 --- a/docs/docs/reference/telemetry.md +++ b/docs/docs/reference/telemetry.md @@ -63,7 +63,9 @@ We collect the following categories of events: | `first_prompt_sent` | The user's first typed message in an onboarding session. Slash commands are excluded, so the hidden `/onboard-connect` submission does not count. | | `onboarding_abandoned` | The CLI exited during a first run without connecting. `last_stage` is the furthest point reached: `started`, `model_picker`, `provider_setup`, `big_pickle_confirm`, or `gateway_auth`. (`connected` is a funnel position but never a `last_stage` — reaching it means the run completed, which is not an abandonment.) Only emitted for a genuine first run — opening `/connect` as an existing user does not enter the funnel, and abandonment after setup completes is out of scope by definition. Emitted on the exit path under a bounded flush, so the measured rate is a lower bound — see [Delivery & Reliability](#delivery--reliability). | | `review_run` | A dbt/SQL review completed or failed — `invocation` (`cli` for `altimate-code review`, `tool` for the `dbt_pr_review` tool), status, duration, and on success the verdict, the pre-gating verdict, mode, risk tier, and finding counts by severity and by category. No file paths, model or column names, finding titles or bodies, SQL, diff content, or repository/branch/PR names. | -| `review_post_outcome` | Whether a review was published to GitHub — `not_requested`, `not_attempted`, `target_unresolved`, `full`, `partial`, or `summary_failed`, plus duration. Exactly one per **completed** review: a review that failed emits `review_run: failed` and no post event, so absence means the review failed rather than that an event was lost. `not_attempted` is publication requested but never reached (a bad `--output` path, a stdout write error). No repository, PR, or comment content. | +| `review_post_outcome` | Whether a review was published to GitHub — `not_requested`, `not_attempted`, `target_unresolved`, `full`, `partial`, or `summary_failed`, plus duration. Emitted on the **CLI path only** — the `dbt_pr_review` tool completes reviews but never publishes, so a `review_run` with `invocation: tool` has no post event and that is not a failure. Within the CLI path there is exactly one per **completed** review: a review that failed emits `review_run: failed` and no post event, so absence there means the review failed rather than that an event was lost. `not_attempted` is publication requested but never reached (a bad `--output` path, a stdout write error). No repository, PR, or comment content. | + +Each event includes a timestamp, anonymous session ID, a per-launch correlation ID (`launch_id` — a random value regenerated every process start, not persisted and not derived from your machine or identity; it exists only to group events from the same run), CLI version, and an anonymous machine ID (a random UUID stored in `~/.altimate/machine-id`, generated once and never tied to any personal information). ### Notes on the review events @@ -85,8 +87,6 @@ They are therefore inferred from the closest deterministic signals — the menu - The "something else" branch has no tool signature at all and is never counted. - `first_job_completed` only fires for jobs with a real completion signal. Skill-driven jobs (downstream impact, SQL review, cost) load an instruction bundle and then do their work through other tools, so their completion is not observable and they are absent from this event rather than wrongly counted in it. -Each event includes a timestamp, anonymous session ID, a per-launch correlation ID (`launch_id` — a random value regenerated every process start, not persisted and not derived from your machine or identity; it exists only to group events from the same run), CLI version, and an anonymous machine ID (a random UUID stored in `~/.altimate/machine-id`, generated once and never tied to any personal information). - ## Delivery & Reliability Telemetry events are buffered in memory and flushed periodically. If a flush fails (e.g., due to a transient network error), events are re-added to the buffer for one retry. On process exit, the CLI performs a final flush to avoid losing events from the current session. diff --git a/packages/opencode/test/altimate/review/telemetry.test.ts b/packages/opencode/test/altimate/review/telemetry.test.ts index d1560e4144..a9ed6ea557 100644 --- a/packages/opencode/test/altimate/review/telemetry.test.ts +++ b/packages/opencode/test/altimate/review/telemetry.test.ts @@ -4,6 +4,9 @@ // do NOT declare a `source` field, so the envelope's process-level value survives. That is // invisible to a Telemetry.track spy — it only appears after serialization — so it is asserted at // the transport level. +import fs from "fs" +import os from "os" +import path from "path" import { describe, expect, test, afterEach, spyOn, mock } from "bun:test" import { Telemetry } from "@/altimate/telemetry" import { @@ -257,6 +260,18 @@ describe("caller attribution", () => { const origDisabled = process.env.ALTIMATE_TELEMETRY_DISABLED const origCs = process.env.APPLICATIONINSIGHTS_CONNECTION_STRING const origClient = process.env.ALTIMATE_CLI_CLIENT + // Before the fetch spy below, not after — restoring afterwards would remove that spy and leave + // `bodies` empty. Every other describe in this file spies Telemetry.track, and this is the only + // test that needs the real one plus a real init; relying on a sibling's afterEach to have + // undone that spy makes the result depend on suite ordering. + mock.restore() + // Real init writes ~/.altimate/machine-id. Point HOME at a temp dir so running the unit suite + // cannot mint an identity the developer's own CLI would then reuse. + const origHome = process.env.HOME + const origUserProfile = process.env.USERPROFILE + const tmpHome = fs.mkdtempSync(path.join(os.tmpdir(), "altimate-review-telemetry-")) + process.env.HOME = tmpHome + process.env.USERPROFILE = tmpHome const bodies: string[] = [] const fetchMock = spyOn(global, "fetch").mockImplementation((async (_i: any, init: any) => { bodies.push(String(init?.body ?? "")) @@ -268,13 +283,23 @@ describe("caller attribution", () => { process.env.APPLICATIONINSIGHTS_CONNECTION_STRING = "InstrumentationKey=k;IngestionEndpoint=https://example.invalid" process.env.ALTIMATE_CLI_CLIENT = "plugin:claude-code" + // init() is `initPromise ??= doInit()`, so a resolved initPromise left by any earlier init in + // this process — including one that ran while telemetry was disabled — is handed back as-is + // and the connection string set above is ignored. shutdown() clearing initPromise is the + // only reset seam the module exposes. + await Telemetry.shutdown() await Telemetry.init() + // Fail here with a cause rather than below on an empty batch: a surviving spy, a + // disabled-telemetry env var and an unparseable connection string all show up as `false`. + expect(Telemetry.isEnabled()).toBe(true) emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", envelope: envelope() }) emitReviewPostOutcome({ outcome: "not_requested", durationMs: 0, sessionID: "" }) await Telemetry.flush() - const envelopes = JSON.parse(bodies[0]) as any[] + // Across all bodies, not bodies[0]: the buffer is module-global and the periodic flush can + // fire before this one, splitting these two events across batches. + const envelopes = bodies.flatMap((body) => JSON.parse(body) as any[]) const run = envelopes.find((e) => e.data.baseData.name === "review_run") const post = envelopes.find((e) => e.data.baseData.name === "review_post_outcome") expect(run.data.baseData.properties.source).toBe("plugin:claude-code") @@ -289,6 +314,11 @@ describe("caller attribution", () => { else delete process.env.APPLICATIONINSIGHTS_CONNECTION_STRING if (origClient !== undefined) process.env.ALTIMATE_CLI_CLIENT = origClient else delete process.env.ALTIMATE_CLI_CLIENT + if (origHome !== undefined) process.env.HOME = origHome + else delete process.env.HOME + if (origUserProfile !== undefined) process.env.USERPROFILE = origUserProfile + else delete process.env.USERPROFILE + fs.rmSync(tmpHome, { recursive: true, force: true }) fetchMock.mockRestore() } })