feat(review): telemetry for review feature usage - #1064
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughAdded shared telemetry for review runs and publication outcomes across CLI and tool entrypoints. The change defines event schemas, preserves caller attribution, redacts review content, isolates telemetry failures, and adds unit, integration, and end-to-end coverage. ChangesReview telemetry
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
Thanks for updating your PR! It now meets our contributing guidelines. 👍 |
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 `<native function> + 1` into a
`Record<string, number>`. 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/opencode/test/e2e/review-telemetry.e2e.test.ts (1)
108-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the expected category count from
ReviewCategory.options.The literal
14duplicates the enum size. If a category is added or removed, this test fails with a message that does not name the cause. ImportReviewCategoryand assert againstReviewCategory.options.length, as the unit test does.♻️ Proposed change
const byCategory = JSON.parse(run!.properties.by_category) - expect(Object.keys(byCategory).length).toBe(14) + expect(Object.keys(byCategory).length).toBe(ReviewCategory.options.length)Add the import at the top of the file:
import { ReviewCategory } from "`@/altimate/review/finding`"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/e2e/review-telemetry.e2e.test.ts` around lines 108 - 109, Update the byCategory assertion in the review telemetry test to compare against ReviewCategory.options.length instead of the hardcoded 14, and add the ReviewCategory import from the indicated review finding module.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/docs/reference/telemetry.md`:
- Around line 52-62: Reorder the telemetry documentation so the general event
metadata sentence about timestamp, session ID, CLI version, and machine ID
appears before the “Notes on the review events” subsection. Keep the existing
review-note bullets unchanged and ensure that subsection follows the general
event description.
In `@packages/opencode/test/altimate/review/telemetry.test.ts`:
- Around line 254-275: Reset the memoized telemetry state before initialization
in the test “the process client source reaches the serialized event” by awaiting
Telemetry.shutdown() after configuring the environment and before calling
Telemetry.init(). Keep the existing test setup and assertions unchanged so each
run initializes with the configured connection string and a clean buffer.
---
Nitpick comments:
In `@packages/opencode/test/e2e/review-telemetry.e2e.test.ts`:
- Around line 108-109: Update the byCategory assertion in the review telemetry
test to compare against ReviewCategory.options.length instead of the hardcoded
14, and add the ReviewCategory import from the indicated review finding module.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 3e91207d-4e7f-48f8-871d-1b5f802ead35
📒 Files selected for processing (7)
docs/docs/reference/telemetry.mdpackages/opencode/src/altimate/review/telemetry.tspackages/opencode/src/altimate/telemetry/index.tspackages/opencode/src/altimate/tools/dbt-pr-review.tspackages/opencode/src/cli/cmd/review.tspackages/opencode/test/altimate/review/telemetry.test.tspackages/opencode/test/e2e/review-telemetry.e2e.test.ts
Code Review SummaryStatus: 1 Suggestion Found | Recommendation: Merge (non-blocking) Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (7 files)
The post-outcome latch ( Fix these issues in Kilo Cloud Previous Review Summaries (2 snapshots, latest commit 67a7729)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 67a7729)Status: No Issues Found | Recommendation: Merge Overview — incremental review (commit
|
There was a problem hiding this comment.
All reported issues were addressed across 7 files
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
…e 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
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/opencode/test/altimate/review/telemetry.test.ts (1)
7-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the
tmpdir()fixture for this test.Replace the
fs.mkdtempSync(path.join(os.tmpdir(), ...))setup withawait using tmp = await tmpdir(). Remove the manual temporary-directory cleanup. This gives the test the required per-test temporary-directory scope.Based on learnings: “For brand-new test files added under
packages/opencode/test/altimate/, importtmpdirfromfixture/fixture.tsand useawait using tmp = await tmpdir().”Also applies to: 270-274
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/altimate/review/telemetry.test.ts` around lines 7 - 9, Update the temporary-directory setup in the affected test to import tmpdir from fixture/fixture.ts and use await using tmp = await tmpdir() instead of fs.mkdtempSync with os.tmpdir and path.join. Remove the manual cleanup and rely on the fixture’s per-test scope, updating references to use the fixture-provided directory.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/opencode/test/altimate/review/telemetry.test.ts`:
- Around line 7-9: Update the temporary-directory setup in the affected test to
import tmpdir from fixture/fixture.ts and use await using tmp = await tmpdir()
instead of fs.mkdtempSync with os.tmpdir and path.join. Remove the manual
cleanup and rely on the fixture’s per-test scope, updating references to use the
fixture-provided directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: be958b3e-ad0c-4bce-9452-26c1808ec66d
📒 Files selected for processing (2)
docs/docs/reference/telemetry.mdpackages/opencode/test/altimate/review/telemetry.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/docs/reference/telemetry.md
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb
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 `<native function> + 1` into a
`Record<string, number>`. 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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb
…e 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
67a7729 to
f08d139
Compare
dev-punia-altimate
left a comment
There was a problem hiding this comment.
approving as requested
| type: "review_post_outcome" | ||
| timestamp: number | ||
| session_id: string | ||
| /** `partial` covers every "not fully posted as attempted" state PostResult can express — |
There was a problem hiding this comment.
SUGGESTION: Two consecutive /** */ blocks sit above outcome; only the second binds to the field in doc tooling.
The first block (documenting the partial outcome) is orphaned — TypeScript attaches only the immediately-preceding /** */ to a declaration, so the partial note never surfaces in hover/TypeDoc for outcome. Merging both blocks into a single /** */ keeps both the partial and not_attempted explanations attached to the field.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Issue for this PR
Closes #1065
Type of change
What does this PR do?
The review feature emitted no telemetry at all — no
Telemetryreference incli/cmd/review.tsor the engine, and no session on the CLI path, so not evensession_start. We couldn't tell how often review runs, who runs it, what it finds, or whether results get published.Adds two events from one helper shared by both callers of the engine — the
reviewcommand and thedbt_pr_reviewtool — separated by aninvocationfield. Instrumenting only the command would have missed every review an agent runs through the tool.Caller attribution needed no code.
toAppInsightsEnvelopesalready seedssourcefromFlag.ALTIMATE_CLI_CLIENT, and an event only overrides that by declaring its ownsourcefield. These events declare none, so anything exportingALTIMATE_CLI_CLIENTis attributed for free. That let me drop an earlier design carrying a global envelope property, a per-command entrypoint registry, areviewPullRequest()signature change and typed errors through the engine.Four choices that aren't obvious from the diff:
review_run, and a failed post must not mark a computed review as failed.partialcovers every degraded post state.PostResultcan't distinguish them —postErrorisn't cleared when the retry succeeds, and an inline fallback coexists with a realreviewId.by_categoryis zero-filled from the 14-value enum and drops unknown keys, so "never fired" differs from "not possible" and a malformed category can't mint a new dimension.Enums, booleans and counts only — review findings are about customer schema.
How did you verify your code works?
sourcefield to the event fails the attribution test, and removing the zero-fill fails two others.ALTIMATE_E2E=1 bun test test/e2e/, ~13s) that runs a realaltimate-code reviewprocess against a real git repo, with telemetry pointed at a local HTTP sink. Spawned with onlyALTIMATE_CLI_CLIENTset, the arriving envelope carriedsource=plugin:claude-code— which is the one claim the whole design rests on and can't be shown in-process.dbt_project.ymlbut no compiled manifest reportsdegraded=true, sodegradedis a fidelity flag, not a "no warehouse" flag. Docs corrected.tsgo --noEmitclean onpackages/opencode; 162 telemetry tests passing.Screenshots / recordings
N/A — no UI change.
Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_018SLUQF3xgZHsGZHSjxe7vb
Summary by CodeRabbit
New Features
Documentation