diff --git a/docs/docs/reference/telemetry.md b/docs/docs/reference/telemetry.md index b2688fe24..0ef5926ab 100644 --- a/docs/docs/reference/telemetry.md +++ b/docs/docs/reference/telemetry.md @@ -62,6 +62,21 @@ 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`, `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 + +- `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 @@ -72,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/src/altimate/review/telemetry.ts b/packages/opencode/src/altimate/review/telemetry.ts new file mode 100644 index 000000000..fc20cc3b1 --- /dev/null +++ b/packages/opencode/src/altimate/review/telemetry.ts @@ -0,0 +1,149 @@ +// 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 { + // 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 (Object.hasOwn(counts, finding.category)) 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 (`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" + 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. + * + * 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" | "not_attempted" | "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 971ee1134..bb4c9ee46 100644 --- a/packages/opencode/src/altimate/telemetry/index.ts +++ b/packages/opencode/src/altimate/telemetry/index.ts @@ -906,6 +906,59 @@ 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. */ + /** `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 + /** 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 82df71adc..16ce29024 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 f8a295b86..717588c3d 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,52 +76,111 @@ 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, - }) - - if (args.output) await fs.writeFile(args.output as string, JSON.stringify(env, null, 2)) + // 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 }) - // 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: "" }) } - if (args.post) { - const target = await resolveGitHubTarget() - if (!target) { - 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 { - const r = await postGitHubReview(env, target) - 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 { + 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 { + target = await resolveGitHubTarget() + } catch (err) { + // 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 + } + 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 new file mode 100644 index 000000000..a9ed6ea55 --- /dev/null +++ b/packages/opencode/test/altimate/review/telemetry.test.ts @@ -0,0 +1,325 @@ +// 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 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 { + 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("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() }) + 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("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") + }) + + 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("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. + 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 + // 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 ?? "")) + 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" + // 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() + + // 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") + expect(post.data.baseData.properties.source).toBe("plugin:claude-code") + } finally { + // 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 + 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() + } + }) +}) 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 000000000..ddcd940f9 --- /dev/null +++ b/packages/opencode/test/e2e/review-telemetry.e2e.test.ts @@ -0,0 +1,191 @@ +// 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", + }, + }, + ) + // 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) + + 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. 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) + + // 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, + ) + + 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, + ) +})