Skip to content

Commit 67a7729

Browse files
sahrizviclaude
andcommitted
fix(review): scope the post-outcome invariant to the CLI, de-order the attribution test
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb
1 parent 4ba07a7 commit 67a7729

2 files changed

Lines changed: 34 additions & 3 deletions

File tree

‎docs/docs/reference/telemetry.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,9 @@ We collect the following categories of events:
4848
| `validator_check` | A completion-gate validator ran on session end — validator name, `ok` boolean, step, retry count, `enforced` flag (false in shadow mode), and structured `details` (model counts, elapsed time, concurrency limit — no SQL or model content). Only emitted when `ALTIMATE_VALIDATORS_ENABLED=1` or `ALTIMATE_VALIDATORS_SHADOW=1`. See [Validators](../data-engineering/validators.md). |
4949
| `validator_retries_exhausted` | A session terminated with unresolved validator failures after exhausting the synthetic-retry budget — names of the failing validators (no failure body content). |
5050
| `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. |
51-
| `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. |
51+
| `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. |
52+
53+
Each event includes a timestamp, anonymous session ID, CLI version, and an anonymous machine ID (a random UUID stored in `~/.altimate/machine-id`, generated once and never tied to any personal information).
5254

5355
### Notes on the review events
5456

@@ -61,7 +63,6 @@ We collect the following categories of events:
6163
- Reviews run through the `dbt_pr_review` tool also emit the standard `tool_call` event. They are
6264
the same review; count `review_run` rather than both.
6365

64-
Each event includes a timestamp, anonymous session ID, CLI version, and an anonymous machine ID (a random UUID stored in `~/.altimate/machine-id`, generated once and never tied to any personal information).
6566

6667
## Delivery & Reliability
6768

‎packages/opencode/test/altimate/review/telemetry.test.ts‎

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@
44
// do NOT declare a `source` field, so the envelope's process-level value survives. That is
55
// invisible to a Telemetry.track spy — it only appears after serialization — so it is asserted at
66
// the transport level.
7+
import fs from "fs"
8+
import os from "os"
9+
import path from "path"
710
import { describe, expect, test, afterEach, spyOn, mock } from "bun:test"
811
import { Telemetry } from "@/altimate/telemetry"
912
import {
@@ -257,6 +260,18 @@ describe("caller attribution", () => {
257260
const origDisabled = process.env.ALTIMATE_TELEMETRY_DISABLED
258261
const origCs = process.env.APPLICATIONINSIGHTS_CONNECTION_STRING
259262
const origClient = process.env.ALTIMATE_CLI_CLIENT
263+
// Before the fetch spy below, not after — restoring afterwards would remove that spy and leave
264+
// `bodies` empty. Every other describe in this file spies Telemetry.track, and this is the only
265+
// test that needs the real one plus a real init; relying on a sibling's afterEach to have
266+
// undone that spy makes the result depend on suite ordering.
267+
mock.restore()
268+
// Real init writes ~/.altimate/machine-id. Point HOME at a temp dir so running the unit suite
269+
// cannot mint an identity the developer's own CLI would then reuse.
270+
const origHome = process.env.HOME
271+
const origUserProfile = process.env.USERPROFILE
272+
const tmpHome = fs.mkdtempSync(path.join(os.tmpdir(), "altimate-review-telemetry-"))
273+
process.env.HOME = tmpHome
274+
process.env.USERPROFILE = tmpHome
260275
const bodies: string[] = []
261276
const fetchMock = spyOn(global, "fetch").mockImplementation((async (_i: any, init: any) => {
262277
bodies.push(String(init?.body ?? ""))
@@ -268,13 +283,23 @@ describe("caller attribution", () => {
268283
process.env.APPLICATIONINSIGHTS_CONNECTION_STRING =
269284
"InstrumentationKey=k;IngestionEndpoint=https://example.invalid"
270285
process.env.ALTIMATE_CLI_CLIENT = "plugin:claude-code"
286+
// init() is `initPromise ??= doInit()`, so a resolved initPromise left by any earlier init in
287+
// this process — including one that ran while telemetry was disabled — is handed back as-is
288+
// and the connection string set above is ignored. shutdown() clearing initPromise is the
289+
// only reset seam the module exposes.
290+
await Telemetry.shutdown()
271291
await Telemetry.init()
292+
// Fail here with a cause rather than below on an empty batch: a surviving spy, a
293+
// disabled-telemetry env var and an unparseable connection string all show up as `false`.
294+
expect(Telemetry.isEnabled()).toBe(true)
272295

273296
emitReviewRun({ invocation: "cli", durationMs: 1, sessionID: "", envelope: envelope() })
274297
emitReviewPostOutcome({ outcome: "not_requested", durationMs: 0, sessionID: "" })
275298
await Telemetry.flush()
276299

277-
const envelopes = JSON.parse(bodies[0]) as any[]
300+
// Across all bodies, not bodies[0]: the buffer is module-global and the periodic flush can
301+
// fire before this one, splitting these two events across batches.
302+
const envelopes = bodies.flatMap((body) => JSON.parse(body) as any[])
278303
const run = envelopes.find((e) => e.data.baseData.name === "review_run")
279304
const post = envelopes.find((e) => e.data.baseData.name === "review_post_outcome")
280305
expect(run.data.baseData.properties.source).toBe("plugin:claude-code")
@@ -289,6 +314,11 @@ describe("caller attribution", () => {
289314
else delete process.env.APPLICATIONINSIGHTS_CONNECTION_STRING
290315
if (origClient !== undefined) process.env.ALTIMATE_CLI_CLIENT = origClient
291316
else delete process.env.ALTIMATE_CLI_CLIENT
317+
if (origHome !== undefined) process.env.HOME = origHome
318+
else delete process.env.HOME
319+
if (origUserProfile !== undefined) process.env.USERPROFILE = origUserProfile
320+
else delete process.env.USERPROFILE
321+
fs.rmSync(tmpHome, { recursive: true, force: true })
292322
fetchMock.mockRestore()
293323
}
294324
})

0 commit comments

Comments
 (0)