Route telemetry context through CLI, SDK, and cloud#1033
Conversation
📝 WalkthroughWalkthroughThis PR migrates telemetry implementation from a public package into the CLI, replaces anonymous identity with distinct ID tracking, adds orchestrator harness detection and propagation across the system, enriches cloud requests and broker events with telemetry context, converts SDK facade methods to async, and deprecates the public telemetry package. ChangesTelemetry architecture refactor and context propagation
🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
CodeAnt AI is reviewing your PR. |
codeant-ai
Bot
added
the
size:XXL
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements comprehensive telemetry routing across the CLI, SDK, cloud client, Rust broker, and hosted relaycast engine, introducing common properties like app, surface, and orchestrator harness. The review feedback highlights critical performance and reliability concerns: executing expensive process-tree traversals unconditionally on CLI and broker startup even when telemetry is disabled, the potential loss of telemetry events in short-lived SDK scripts due to the lack of an automatic shutdown flush, and the risk of telemetry failures disrupting the main application flow because tracking calls are not wrapped in try-catch blocks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function propagateTelemetryContextToChildren(): string { | ||
| const orchestratorHarness = detectOrchestratorHarness(); |
There was a problem hiding this comment.
The detectOrchestratorHarness() function is called unconditionally on every CLI invocation, even when telemetry is disabled. Since this function synchronously spawns up to 8 ps processes on Unix systems to traverse the process tree, it introduces a significant performance penalty (often 50-100ms or more) to fast commands like relay --help or for users who have opted out of telemetry.
We should check if telemetry is enabled first, and only perform the expensive process traversal if necessary.
| function propagateTelemetryContextToChildren(): string { | |
| const orchestratorHarness = detectOrchestratorHarness(); | |
| function propagateTelemetryContextToChildren(): string { | |
| const telemetryEnabled = isTelemetryEnabled(); | |
| const orchestratorHarness = telemetryEnabled ? detectOrchestratorHarness() : 'unknown'; |
| fn detect_orchestrator_harness() -> String { | ||
| for key in [ |
There was a problem hiding this comment.
Similar to the CLI, detect_orchestrator_harness() is executed unconditionally on broker startup, which synchronously spawns ps up to 8 times on Unix platforms. This introduces unnecessary latency on startup when telemetry is disabled.
We should check if telemetry is disabled via environment variables (AGENT_RELAY_TELEMETRY_DISABLED or DO_NOT_TRACK) at the very beginning of the function to return early and avoid the expensive process traversal.
fn detect_orchestrator_harness() -> String {
if let Ok(val) = std::env::var("AGENT_RELAY_TELEMETRY_DISABLED").or_else(|_| std::env::var("DO_NOT_TRACK")) {
if val == "1" || val.to_lowercase() == "true" {
return UNKNOWN_ORCHESTRATOR_HARNESS.to_string();
}
}
for key in [| import { | ||
| detectOrchestratorHarness, | ||
| initTelemetry, | ||
| track, | ||
| type CommonProperties, | ||
| } from '@agent-relay/telemetry'; |
There was a problem hiding this comment.
Import the shutdown function from @agent-relay/telemetry so that we can register an exit hook to flush pending telemetry events before the process exits.
| import { | |
| detectOrchestratorHarness, | |
| initTelemetry, | |
| track, | |
| type CommonProperties, | |
| } from '@agent-relay/telemetry'; | |
| import { | |
| detectOrchestratorHarness, | |
| initTelemetry, | |
| track, | |
| shutdown, | |
| type CommonProperties, | |
| } from '@agent-relay/telemetry'; |
| export function initSdkTelemetry(): void { | ||
| if (initialized) return; | ||
| initialized = true; | ||
| initTelemetry({ | ||
| showNotice: false, | ||
| sdkVersion: resolveSdkVersion(), | ||
| app: 'sdk', | ||
| surface: 'sdk', | ||
| orchestratorHarness: orchestratorHarness(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Short-lived SDK scripts will likely exit before PostHog has a chance to flush telemetry events (which has a default flushAt: 10 or flushes periodically). Since the SDK does not expose or call shutdown on process exit, telemetry events for short-lived scripts will be lost.
We should register a process.once('beforeExit', ...) listener in initSdkTelemetry to automatically call shutdown() and flush pending events before the process exits.
export function initSdkTelemetry(): void {
if (initialized) return;
initialized = true;
initTelemetry({
showNotice: false,
sdkVersion: resolveSdkVersion(),
app: 'sdk',
surface: 'sdk',
orchestratorHarness: orchestratorHarness(),
});
if (typeof process !== 'undefined' && typeof process.on === 'function') {
process.once('beforeExit', () => {
shutdown().catch(() => {});
});
}
}| export async function withSdkMethodTelemetry<T>(method: string, fn: () => Promise<T>): Promise<T> { | ||
| const started = Date.now(); | ||
| try { | ||
| const result = await fn(); | ||
| trackSdkMethodCall({ method, success: true, durationMs: Date.now() - started }); | ||
| return result; | ||
| } catch (error) { | ||
| trackSdkMethodCall({ method, success: false, durationMs: Date.now() - started, error }); | ||
| throw error; | ||
| } | ||
| } | ||
|
|
||
| export async function withSdkWorkflowTelemetry<T>(operation: string, fn: () => Promise<T>): Promise<T> { | ||
| const started = Date.now(); | ||
| try { | ||
| const result = await fn(); | ||
| trackSdkWorkflowRun({ operation, success: true, durationMs: Date.now() - started }); | ||
| return result; | ||
| } catch (error) { | ||
| trackSdkWorkflowRun({ operation, success: false, durationMs: Date.now() - started, error }); | ||
| throw error; | ||
| } | ||
| } |
There was a problem hiding this comment.
Telemetry tracking calls (trackSdkMethodCall and trackSdkWorkflowRun) are not wrapped in try/catch blocks inside withSdkMethodTelemetry and withSdkWorkflowTelemetry. If any telemetry initialization or tracking call throws an error (e.g., due to environment issues or PostHog client errors), it will propagate and fail the user's primary SDK method/workflow call.
Telemetry should always fail silently and never disrupt the main application flow. We should wrap the tracking calls in try/catch blocks.
export async function withSdkMethodTelemetry<T>(method: string, fn: () => Promise<T>): Promise<T> {
const started = Date.now();
try {
const result = await fn();
try {
trackSdkMethodCall({ method, success: true, durationMs: Date.now() - started });
} catch {
// Telemetry failures should never disrupt the main application flow
}
return result;
} catch (error) {
try {
trackSdkMethodCall({ method, success: false, durationMs: Date.now() - started, error });
} catch {
// Telemetry failures should never disrupt the main application flow
}
throw error;
}
}
export async function withSdkWorkflowTelemetry<T>(operation: string, fn: () => Promise<T>): Promise<T> {
const started = Date.now();
try {
const result = await fn();
try {
trackSdkWorkflowRun({ operation, success: true, durationMs: Date.now() - started });
} catch {
// Telemetry failures should never disrupt the main application flow
}
return result;
} catch (error) {
try {
trackSdkWorkflowRun({ operation, success: false, durationMs: Date.now() - started, error });
} catch {
// Telemetry failures should never disrupt the main application flow
}
throw error;
}
}![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9f18f6819
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { | ||
| detectOrchestratorHarness, | ||
| initTelemetry, | ||
| track, | ||
| type CommonProperties, | ||
| } from '@agent-relay/telemetry'; |
There was a problem hiding this comment.
When consumers bundle @agent-relay/sdk for a browser or edge worker, this new static import makes the public SDK entrypoint pull in @agent-relay/telemetry; that package imports posthog-node plus Node builtins such as node:child_process, node:fs, and node:os. Because src/index.ts exports facade, the bundle now fails to resolve those Node-only modules even if telemetry is disabled. Make the telemetry path lazy/optional behind a Node-runtime check or keep it out of the browser-compatible SDK surface.
Useful? React with 👍 / 👎.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| if ('channel' in input && input.channel) { | ||
| return base.send(input); |
There was a problem hiding this comment.
Suggestion: The channel-branch guard incorrectly requires input.channel to be truthy. A valid RelaySendChannelMessageInput with an empty channel string will skip this branch, get cast to the direct-message shape, and then crash when channel-target detection calls string methods on an undefined to field. Route by structural type ('channel' in input) and let downstream validation handle empty channel values. [incorrect condition logic]
Severity Level: Major ⚠️
- ❌ AgentRelay.messages.send crashes on empty channel strings.
- ⚠️ SDK consumers see TypeError before any network request.
- ⚠️ Messaging workflows can fail due to minor input bugs.Steps of Reproduction ✅
1. In a consumer of the SDK, construct an `AgentRelay` using `new AgentRelay({
workspaceKey: 'test-workspace-key' })` from `packages/sdk/src/agent-relay.ts` (constructor
and properties defined at lines 71–93, `messages` getter at lines 124–129), so that
`this.messaging` is a `RelaycastMessagingClient` and `this.messages` returns
`createEnrichedMessages(this.messaging.messages, ...)` from
`packages/sdk/src/facade.ts:216–221`.
2. Call the high-level send API with a structurally valid channel input but an empty
channel string: `await relay.messages.send({ channel: '', text: 'hello' })`. This
satisfies the `RelaySendChannelMessageInput` shape defined in
`packages/sdk/src/messaging/types.ts:16–22` (`channel: string; text: string; ...`) and
matches the `EnrichedMessages.send` overload in `facade.ts:135–138`.
3. Inside `createEnrichedMessages` in `facade.ts:216–256`, the `enriched.send`
implementation at lines 222–256 executes. The guard `if ('channel' in input &&
input.channel)` at lines 224–225 evaluates to `false` because `input.channel` is the empty
string, so the channel-shaped input is misclassified and execution falls through to the
`RelaySendMessageInput` branch, casting `input` to `sendInput: RelaySendMessageInput` at
line 227 even though it has no `to` field.
4. The next logic assumes a valid `sendInput.to` from `RelaySendMessageInput` (defined at
`facade.ts:96–108`) and eventually calls `isChannelTarget(sendInput.to)` at
`facade.ts:239`. Since `sendInput.to` is `undefined` on the misclassified input,
`isChannelTarget` (defined at `facade.ts:61–63` as `to.startsWith('#')`) throws a
`TypeError: Cannot read properties of undefined (reading 'startsWith')`, causing
`withSdkMethodTelemetry('messages.send', ...)` at `facade.ts:223–256` to record a failed
SDK method and propagate the runtime crash back to the caller instead of routing the
request through `base.messages.send` in `packages/sdk/src/messaging/relaycast.ts:489–501`.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/sdk/src/facade.ts
**Line:** 224:225
**Comment:**
*Incorrect Condition Logic: The channel-branch guard incorrectly requires `input.channel` to be truthy. A valid `RelaySendChannelMessageInput` with an empty channel string will skip this branch, get cast to the direct-message shape, and then crash when channel-target detection calls string methods on an undefined `to` field. Route by structural type (`'channel' in input`) and let downstream validation handle empty channel values.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
| if base == "zed" || lower.contains("zed") { | ||
| return Some("zed"); |
There was a problem hiding this comment.
Suggestion: The broker's harness inference also classifies any command containing zed as Zed, which creates false positives for unrelated process names and pollutes telemetry dimensions. Match only normalized executable names (or path segments) instead of generic substring checks. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Broker telemetry mislabels arbitrary parents as Zed harness.
- ⚠️ Telemetry dashboards overreport Zed-driven broker usage.Steps of Reproduction ✅
1. Observe that the broker CLI entrypoint `run()` in `crates/broker/src/cli/mod.rs:11-20`
constructs a telemetry client via `let telemetry = TelemetryClient::new();` (line ~114 in
the file, as shown in the repo code graph).
2. In `TelemetryClient::new` ( `crates/broker/src/telemetry.rs:540-599` ), the struct
field `orchestrator_harness` is initialized by calling `detect_orchestrator_harness()`
(line 26 of that snippet).
3. `detect_orchestrator_harness()` ( `crates/broker/src/telemetry.rs:164-179` ) falls back
to `detect_process_orchestrator_harness()` when no explicit harness env var is set; on
Unix, `detect_process_orchestrator_harness()` walks parent PIDs and calls
`lookup_process_info()` / `infer_harness_from_command()` for each parent (lines 134-147).
4. Inside `infer_harness_from_command()` ( `crates/broker/src/telemetry.rs:60-107` ), the
`zed` branch is implemented as `if base == "zed" || lower.contains("zed") { return
Some("zed"); }` (lines 362-363 in the current file), so a parent process command like
`/usr/bin/analyzed` (base `analyzed`, but `lower` contains the substring `zed`) is
misclassified as the Zed harness; this value is then emitted on every broker telemetry
event via the `orchestrator_harness` property merged in `TelemetryClient::track` (lines
44-57 in the `540-599` snippet).Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/broker/src/telemetry.rs
**Line:** 362:363
**Comment:**
*Incorrect Condition Logic: The broker's harness inference also classifies any command containing `zed` as Zed, which creates false positives for unrelated process names and pollutes telemetry dimensions. Match only normalized executable names (or path segments) instead of generic substring checks.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| function resolveSdkVersion(): string | undefined { | ||
| const version = process.env.AGENT_RELAY_SDK_VERSION?.trim(); | ||
| return version || undefined; |
There was a problem hiding this comment.
Suggestion: resolveSdkVersion only reads AGENT_RELAY_SDK_VERSION, so SDK telemetry emitted outside the CLI path usually has no sdk_version; then initTelemetry falls back to the telemetry package version for agent_relay_version, producing incorrect version attribution in dashboards. Resolve the SDK package version directly (or inject it at build time) instead of relying only on an env var. [logic error]
Severity Level: Major ⚠️
- ⚠️ SDK telemetry events tagged with telemetry package version.
- ⚠️ Dashboards cannot accurately segment by SDK version.Steps of Reproduction ✅
1. In a Node application using the SDK, call any facade method that is wrapped with
telemetry, e.g. `createEnrichedMessages(...).send(...)` in
`packages/sdk/src/facade.ts:222-247`, which internally wraps the send logic in
`withSdkMethodTelemetry('messages.send', ...)` (line 223).
2. `withSdkMethodTelemetry` in `packages/sdk/src/telemetry.ts:84-93` calls
`trackSdkMethodCall`, which first calls `initSdkTelemetry()` (lines 30-40) before emitting
the event.
3. `initSdkTelemetry()` calls `initTelemetry({ sdkVersion: resolveSdkVersion(), app:
'sdk', surface: 'sdk', orchestratorHarness: orchestratorHarness() })` (lines 33-39). In a
typical non-CLI SDK integration, `process.env.AGENT_RELAY_SDK_VERSION` is not set, so
`resolveSdkVersion()` (lines 11-13) trims the env var, gets `undefined`, and returns
`undefined`.
4. In `packages/telemetry/src/client.ts:60-75`, `buildCommonProperties` computes the
primary `agent_relay_version` as `versions.cliVersion ?? versions.brokerVersion ??
versions.sdkVersion ?? getFallbackVersion()`. With `cliVersion`, `brokerVersion`, and
`sdkVersion` all undefined (because only the SDK is in use and `AGENT_RELAY_SDK_VERSION`
was unset), `getFallbackVersion()` is used instead (lines 45-57), which the comments note
resolves the *telemetry package* `package.json`. As a result, SDK events are tagged with
the telemetry package version in `agent_relay_version`, and `sdk_version` is absent,
producing incorrect version attribution in dashboards for any SDK-only usage that is not
launched via the CLI.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/sdk/src/telemetry.ts
**Line:** 11:13
**Comment:**
*Logic Error: `resolveSdkVersion` only reads `AGENT_RELAY_SDK_VERSION`, so SDK telemetry emitted outside the CLI path usually has no `sdk_version`; then `initTelemetry` falls back to the telemetry package version for `agent_relay_version`, producing incorrect version attribution in dashboards. Resolve the SDK package version directly (or inject it at build time) instead of relying only on an env var.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| if (base === 'droid' || lower.includes('droid')) return 'droid'; | ||
| if (base === 'amp' || normalized.includes('/amp')) return 'amp'; | ||
| if (lower.includes('copilot')) return 'github-copilot'; | ||
| if (base === 'zed' || lower.includes('zed')) return 'zed'; |
There was a problem hiding this comment.
Suggestion: The zed detection uses a raw substring match, so unrelated process names containing zed (for example analyzed or dozed) will be misclassified as the Zed harness. Restrict matching to executable/base-name boundaries instead of generic substring matching. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Node telemetry mislabels arbitrary parents as Zed harness.
- ⚠️ Harness usage metrics overreport Zed in dashboards.Steps of Reproduction ✅
1. `detectOrchestratorHarness()` in
`packages/telemetry/src/orchestrator-harness.ts:82-107` is used by the telemetry client
via `buildCommonProperties` in `packages/telemetry/src/client.ts:60-75`, where
`orchestrator_harness` is set to `versions.orchestratorHarness ??
detectOrchestratorHarness() ?? UNKNOWN_ORCHESTRATOR_HARNESS` (lines 94-96) whenever the
caller did not explicitly supply a harness.
2. In a Node process (e.g. the CLI or any other consumer using `@agent-relay/telemetry`
directly), `detectOrchestratorHarness()` first checks for explicit env overrides, then
walks parent PIDs using a `processLookup` function (defaulting to `lookupProcessInfo`) and
passes each parent's command string to `inferHarnessFromCommand()` (lines 89-100 in
`orchestrator-harness.ts`).
3. `inferHarnessFromCommand()` (lines 36-54) normalizes the command and computes `base`,
but the `zed` detection branch is implemented as `if (base === 'zed' ||
lower.includes('zed')) return 'zed';` (line 52), so a command like `"/usr/bin/analyzed"`
or `"my-zedless-tool"`—where `base` is not `'zed'` but `lower` contains the substring
`'zed'`—will be incorrectly classified as the Zed harness.
4. When such a parent process exists, `detectOrchestratorHarness()` returns `'zed'`, and
`buildCommonProperties` propagates this into the `orchestrator_harness` property for all
telemetry events emitted by that process, leading to misclassified harness telemetry
despite no actual Zed harness being involved.Fix in Cursor | Fix in VSCode Claude
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** packages/telemetry/src/orchestrator-harness.ts
**Line:** 52:52
**Comment:**
*Incorrect Condition Logic: The `zed` detection uses a raw substring match, so unrelated process names containing `zed` (for example `analyzed` or `dozed`) will be misclassified as the Zed harness. Restrict matching to executable/base-name boundaries instead of generic substring matching.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/telemetry/src/client.ts (1)
76-93: 💤 Low valueConsider extracting the app/surface inference logic to reduce duplication.
The app and surface fields use identical inference logic (lines 76-93). While the duplication is manageable, extracting a helper could improve maintainability.
♻️ Optional refactor to reduce duplication
+function inferComponent( + explicit: string | undefined, + cliVersion: string | undefined, + brokerVersion: string | undefined, + sdkVersion: string | undefined +): string { + return ( + explicit ?? + (cliVersion ? 'cli' : brokerVersion ? 'broker' : sdkVersion ? 'sdk' : 'unknown') + ); +} + function buildCommonProperties(versions: { cliVersion?: string; sdkVersion?: string; brokerVersion?: string; app?: string; surface?: string; orchestratorHarness?: string; }): CommonProperties { const primary = versions.cliVersion ?? versions.brokerVersion ?? versions.sdkVersion ?? getFallbackVersion(); return { - app: - versions.app ?? - (versions.cliVersion - ? 'cli' - : versions.brokerVersion - ? 'broker' - : versions.sdkVersion - ? 'sdk' - : 'unknown'), - surface: - versions.surface ?? - (versions.cliVersion - ? 'cli' - : versions.brokerVersion - ? 'broker' - : versions.sdkVersion - ? 'sdk' - : 'unknown'), + app: inferComponent(versions.app, versions.cliVersion, versions.brokerVersion, versions.sdkVersion), + surface: inferComponent(versions.surface, versions.cliVersion, versions.brokerVersion, versions.sdkVersion),🤖 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/telemetry/src/client.ts` around lines 76 - 93, Duplicate inference logic for app and surface should be extracted into a small helper to improve maintainability: add a helper function (e.g., inferAppOrSurface or inferVariant) that takes the versions object and returns versions.app ?? (versions.cliVersion ? 'cli' : versions.brokerVersion ? 'broker' : versions.sdkVersion ? 'sdk' : 'unknown'), then replace the inline expressions used for both app and surface with calls to this helper; ensure the helper is colocated with the telemetry client code and preserves the exact precedence/values and TypeScript types used by the existing code.packages/cloud/src/telemetry-headers.ts (1)
7-13: ⚡ Quick winCentralize telemetry env-key constants to avoid CLI/cloud drift
AGENT_RELAY_CLIENT_ID_ENV,TELEMETRY_SURFACE_ENV, andTELEMETRY_CLIENT_ENVare duplicated string literals in bothpackages/cloud/src/telemetry-headers.tsandpackages/cli/src/cli/bootstrap.ts. Centralize these env-key constants (e.g., export from@agent-relay/telemetry) and import them in both places to prevent silent mismatches if one side changes.🤖 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/cloud/src/telemetry-headers.ts` around lines 7 - 13, Centralize the duplicated telemetry env-key constants by exporting AGENT_RELAY_CLIENT_ID_ENV, TELEMETRY_SURFACE_ENV, and TELEMETRY_CLIENT_ENV from a single shared module (e.g., add exports in `@agent-relay/telemetry`), then replace the hard-coded declarations in packages/cloud/src/telemetry-headers.ts (currently declaring AGENT_RELAY_CLIENT_ID_ENV, TELEMETRY_SURFACE_ENV, TELEMETRY_CLIENT_ENV) and the duplicates in packages/cli/src/cli/bootstrap.ts with imports from that shared module; remove the local constant declarations and update any references to use the imported symbols so both cloud and CLI use the same authoritative values.
🤖 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 `@packages/cloud/src/telemetry-headers.ts`:
- Around line 26-29: The function isTelemetryDisabledByEnv currently uses
nullish coalescing between AGENT_RELAY_TELEMETRY_DISABLED and DO_NOT_TRACK which
causes DO_NOT_TRACK to be ignored when AGENT_RELAY_TELEMETRY_DISABLED is set to
a non-truthy value; change isTelemetryDisabledByEnv to use an OR-based truthy
check (e.g., evaluate each env var with the same truthiness rules used in
packages/telemetry/src/config.ts or call its isTruthyEnv helper) so that
telemetry is disabled when either AGENT_RELAY_TELEMETRY_DISABLED or DO_NOT_TRACK
is truthy.
In `@packages/sdk/src/telemetry.ts`:
- Around line 50-82: The telemetry helpers trackSdkMethodCall and
trackSdkWorkflowRun must never throw; wrap the calls to initSdkTelemetry(),
sdkCommonOverrides()/detectOrchestratorHarness(), and track(...) inside a
try/catch that swallows any exceptions (optionally logging them to a
non-throwing debug logger) so telemetry failures cannot change business results;
specifically, update both trackSdkMethodCall and trackSdkWorkflowRun to call
initSdkTelemetry(), build errorClass, call sdkCommonOverrides() and track(...)
all inside a single try block and catch any error to suppress it (do not
rethrow), ensuring initTelemetry/detectOrchestratorHarness/track errors are
contained.
---
Nitpick comments:
In `@packages/cloud/src/telemetry-headers.ts`:
- Around line 7-13: Centralize the duplicated telemetry env-key constants by
exporting AGENT_RELAY_CLIENT_ID_ENV, TELEMETRY_SURFACE_ENV, and
TELEMETRY_CLIENT_ENV from a single shared module (e.g., add exports in
`@agent-relay/telemetry`), then replace the hard-coded declarations in
packages/cloud/src/telemetry-headers.ts (currently declaring
AGENT_RELAY_CLIENT_ID_ENV, TELEMETRY_SURFACE_ENV, TELEMETRY_CLIENT_ENV) and the
duplicates in packages/cli/src/cli/bootstrap.ts with imports from that shared
module; remove the local constant declarations and update any references to use
the imported symbols so both cloud and CLI use the same authoritative values.
In `@packages/telemetry/src/client.ts`:
- Around line 76-93: Duplicate inference logic for app and surface should be
extracted into a small helper to improve maintainability: add a helper function
(e.g., inferAppOrSurface or inferVariant) that takes the versions object and
returns versions.app ?? (versions.cliVersion ? 'cli' : versions.brokerVersion ?
'broker' : versions.sdkVersion ? 'sdk' : 'unknown'), then replace the inline
expressions used for both app and surface with calls to this helper; ensure the
helper is colocated with the telemetry client code and preserves the exact
precedence/values and TypeScript types used by the existing code.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c7d69eda-e051-4dc4-bce9-ffb4b379d18f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.agentworkforce/trajectories/completed/2026-06/traj_6kef2z1redhb/summary.md.agentworkforce/trajectories/completed/2026-06/traj_6kef2z1redhb/trajectory.jsonCHANGELOG.mdcrates/broker/src/telemetry.rspackages/cli/src/cli/bootstrap.tspackages/cli/src/cli/entrypoint.test.tspackages/cloud/src/api-client.tspackages/cloud/src/auth.test.tspackages/cloud/src/auth.tspackages/cloud/src/telemetry-headers.tspackages/sdk/package.jsonpackages/sdk/src/agent-relay.tspackages/sdk/src/facade.tspackages/sdk/src/telemetry.tspackages/sdk/tsconfig.build.jsonpackages/sdk/tsconfig.jsonpackages/telemetry/src/client.tspackages/telemetry/src/events.tspackages/telemetry/src/index.tspackages/telemetry/src/orchestrator-harness.test.tspackages/telemetry/src/orchestrator-harness.ts
| function isTelemetryDisabledByEnv(env: NodeJS.ProcessEnv): boolean { | ||
| const disabled = env.AGENT_RELAY_TELEMETRY_DISABLED ?? env.DO_NOT_TRACK; | ||
| return disabled === '1' || disabled?.toLowerCase() === 'true'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the canonical env opt-out implementation to compare semantics.
rg -nP -C6 'isDisabledByEnv' --type=ts -g 'packages/telemetry/src/config.ts'Repository: AgentWorkforce/relay
Length of output: 1639
Fix env opt-out semantics to match packages/telemetry/src/config.ts.
isTelemetryDisabledByEnv in packages/cloud/src/telemetry-headers.ts uses env.AGENT_RELAY_TELEMETRY_DISABLED ?? env.DO_NOT_TRACK, so if AGENT_RELAY_TELEMETRY_DISABLED is set to a non-truthy value (e.g. '0'/'false'), it will ignore DO_NOT_TRACK='true'. In packages/telemetry/src/config.ts, isDisabledByEnv() disables when either env var is truthy (isTruthyEnv(...) || isTruthyEnv(...)).
Reuse the canonical logic (or align the cloud check to an OR-based evaluation over both env vars).
🤖 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/cloud/src/telemetry-headers.ts` around lines 26 - 29, The function
isTelemetryDisabledByEnv currently uses nullish coalescing between
AGENT_RELAY_TELEMETRY_DISABLED and DO_NOT_TRACK which causes DO_NOT_TRACK to be
ignored when AGENT_RELAY_TELEMETRY_DISABLED is set to a non-truthy value; change
isTelemetryDisabledByEnv to use an OR-based truthy check (e.g., evaluate each
env var with the same truthiness rules used in packages/telemetry/src/config.ts
or call its isTruthyEnv helper) so that telemetry is disabled when either
AGENT_RELAY_TELEMETRY_DISABLED or DO_NOT_TRACK is truthy.
| export function trackSdkMethodCall(input: { | ||
| method: string; | ||
| success: boolean; | ||
| durationMs: number; | ||
| error?: unknown; | ||
| }): void { | ||
| initSdkTelemetry(); | ||
| const errorClass = input.error === undefined ? undefined : errorClassName(input.error); | ||
| track('sdk_method_call', { | ||
| method: input.method, | ||
| success: input.success, | ||
| duration_ms: input.durationMs, | ||
| ...(errorClass ? { error_class: errorClass } : {}), | ||
| ...sdkCommonOverrides(), | ||
| }); | ||
| } | ||
|
|
||
| export function trackSdkWorkflowRun(input: { | ||
| operation: string; | ||
| success: boolean; | ||
| durationMs: number; | ||
| error?: unknown; | ||
| }): void { | ||
| initSdkTelemetry(); | ||
| const errorClass = input.error === undefined ? undefined : errorClassName(input.error); | ||
| track('sdk_workflow_run', { | ||
| operation: input.operation, | ||
| success: input.success, | ||
| duration_ms: input.durationMs, | ||
| ...(errorClass ? { error_class: errorClass } : {}), | ||
| ...sdkCommonOverrides(), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Telemetry emission must not be able to break SDK operations.
trackSdkMethodCall / trackSdkWorkflowRun invoke initSdkTelemetry() → initTelemetry(...), sdkCommonOverrides() → detectOrchestratorHarness(), and track(...) with no error guard. In the wrappers (Lines 84-106) these run after await fn() succeeds, so a throw here discards a successful result; and in the catch branch a throw masks the user's real error before the throw error rethrow. Telemetry is a side-effect and should never alter business-logic outcomes.
🛡️ Proposed fix: swallow telemetry failures at the emit boundary
export function trackSdkMethodCall(input: {
method: string;
success: boolean;
durationMs: number;
error?: unknown;
}): void {
- initSdkTelemetry();
- const errorClass = input.error === undefined ? undefined : errorClassName(input.error);
- track('sdk_method_call', {
- method: input.method,
- success: input.success,
- duration_ms: input.durationMs,
- ...(errorClass ? { error_class: errorClass } : {}),
- ...sdkCommonOverrides(),
- });
+ try {
+ initSdkTelemetry();
+ const errorClass = input.error === undefined ? undefined : errorClassName(input.error);
+ track('sdk_method_call', {
+ method: input.method,
+ success: input.success,
+ duration_ms: input.durationMs,
+ ...(errorClass ? { error_class: errorClass } : {}),
+ ...sdkCommonOverrides(),
+ });
+ } catch {
+ // Telemetry failures must never affect SDK behavior.
+ }
}
export function trackSdkWorkflowRun(input: {
operation: string;
success: boolean;
durationMs: number;
error?: unknown;
}): void {
- initSdkTelemetry();
- const errorClass = input.error === undefined ? undefined : errorClassName(input.error);
- track('sdk_workflow_run', {
- operation: input.operation,
- success: input.success,
- duration_ms: input.durationMs,
- ...(errorClass ? { error_class: errorClass } : {}),
- ...sdkCommonOverrides(),
- });
+ try {
+ initSdkTelemetry();
+ const errorClass = input.error === undefined ? undefined : errorClassName(input.error);
+ track('sdk_workflow_run', {
+ operation: input.operation,
+ success: input.success,
+ duration_ms: input.durationMs,
+ ...(errorClass ? { error_class: errorClass } : {}),
+ ...sdkCommonOverrides(),
+ });
+ } catch {
+ // Telemetry failures must never affect SDK behavior.
+ }
}🤖 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/sdk/src/telemetry.ts` around lines 50 - 82, The telemetry helpers
trackSdkMethodCall and trackSdkWorkflowRun must never throw; wrap the calls to
initSdkTelemetry(), sdkCommonOverrides()/detectOrchestratorHarness(), and
track(...) inside a try/catch that swallows any exceptions (optionally logging
them to a non-throwing debug logger) so telemetry failures cannot change
business results; specifically, update both trackSdkMethodCall and
trackSdkWorkflowRun to call initSdkTelemetry(), build errorClass, call
sdkCommonOverrides() and track(...) all inside a single try block and catch any
error to suppress it (do not rethrow), ensuring
initTelemetry/detectOrchestratorHarness/track errors are contained.
|
Reviewed PR #1033 and fixed one concrete issue. Changed:
Validation:
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 22 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cloud/src/telemetry-headers.ts">
<violation number="1" location="packages/cloud/src/telemetry-headers.ts:27">
P2: Empty `AGENT_RELAY_TELEMETRY_DISABLED` suppresses `DO_NOT_TRACK` fallback, which can bypass telemetry opt-out.</violation>
<violation number="2" location="packages/cloud/src/telemetry-headers.ts:58">
P2: Version fallback is blocked when CLI version env var is empty, causing dropped telemetry version context.</violation>
</file>
<file name="packages/telemetry/src/orchestrator-harness.ts">
<violation number="1" location="packages/telemetry/src/orchestrator-harness.ts:44">
P2: Substring-based harness detection can misclassify unrelated commands as `cursor` and skew telemetry labels.</violation>
</file>
<file name=".agentworkforce/trajectories/completed/2026-06/traj_6kef2z1redhb/summary.md">
<violation number="1" location=".agentworkforce/trajectories/completed/2026-06/traj_6kef2z1redhb/summary.md:1">
P3: Typo in heading: "Rope" should be "Route" to match the PR title and all other references in the document.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| maxLength: 80, | ||
| pattern: HEADER_VALUE_ALLOWED, | ||
| }); | ||
| const version = sanitizeHeaderValue(env.AGENT_RELAY_CLI_VERSION ?? env.AGENT_RELAY_SDK_VERSION, { |
There was a problem hiding this comment.
P2: Version fallback is blocked when CLI version env var is empty, causing dropped telemetry version context.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cloud/src/telemetry-headers.ts, line 58:
<comment>Version fallback is blocked when CLI version env var is empty, causing dropped telemetry version context.</comment>
<file context>
@@ -0,0 +1,81 @@
+ maxLength: 80,
+ pattern: HEADER_VALUE_ALLOWED,
+ });
+ const version = sanitizeHeaderValue(env.AGENT_RELAY_CLI_VERSION ?? env.AGENT_RELAY_SDK_VERSION, {
+ maxLength: 48,
+ pattern: HEADER_VALUE_ALLOWED,
</file context>
| const version = sanitizeHeaderValue(env.AGENT_RELAY_CLI_VERSION ?? env.AGENT_RELAY_SDK_VERSION, { | |
| const version = sanitizeHeaderValue(env.AGENT_RELAY_CLI_VERSION?.trim() || env.AGENT_RELAY_SDK_VERSION?.trim(), { |
| } | ||
|
|
||
| function isTelemetryDisabledByEnv(env: NodeJS.ProcessEnv): boolean { | ||
| const disabled = env.AGENT_RELAY_TELEMETRY_DISABLED ?? env.DO_NOT_TRACK; |
There was a problem hiding this comment.
P2: Empty AGENT_RELAY_TELEMETRY_DISABLED suppresses DO_NOT_TRACK fallback, which can bypass telemetry opt-out.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cloud/src/telemetry-headers.ts, line 27:
<comment>Empty `AGENT_RELAY_TELEMETRY_DISABLED` suppresses `DO_NOT_TRACK` fallback, which can bypass telemetry opt-out.</comment>
<file context>
@@ -0,0 +1,81 @@
+}
+
+function isTelemetryDisabledByEnv(env: NodeJS.ProcessEnv): boolean {
+ const disabled = env.AGENT_RELAY_TELEMETRY_DISABLED ?? env.DO_NOT_TRACK;
+ return disabled === '1' || disabled?.toLowerCase() === 'true';
+}
</file context>
| const disabled = env.AGENT_RELAY_TELEMETRY_DISABLED ?? env.DO_NOT_TRACK; | |
| const disabled = env.AGENT_RELAY_TELEMETRY_DISABLED?.trim() || env.DO_NOT_TRACK?.trim(); |
|
|
||
| if (base === 'claude' || lower.includes('claude-code')) return 'claude-code'; | ||
| if (base === 'codex' || normalized.includes('/codex')) return 'codex'; | ||
| if (base === 'cursor' || base === 'cursor-agent' || lower.includes('cursor')) return 'cursor'; |
There was a problem hiding this comment.
P2: Substring-based harness detection can misclassify unrelated commands as cursor and skew telemetry labels.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/telemetry/src/orchestrator-harness.ts, line 44:
<comment>Substring-based harness detection can misclassify unrelated commands as `cursor` and skew telemetry labels.</comment>
<file context>
@@ -0,0 +1,107 @@
+
+ if (base === 'claude' || lower.includes('claude-code')) return 'claude-code';
+ if (base === 'codex' || normalized.includes('/codex')) return 'codex';
+ if (base === 'cursor' || base === 'cursor-agent' || lower.includes('cursor')) return 'cursor';
+ if (base === 'gemini' || base === 'gemini-cli' || lower.includes('gemini-cli')) return 'gemini-cli';
+ if (base === 'aider' || lower.includes('aider')) return 'aider';
</file context>
| @@ -0,0 +1,45 @@ | |||
| # Trajectory: Rope telemetry through CLI SDK and hosted backend | |||
There was a problem hiding this comment.
P3: Typo in heading: "Rope" should be "Route" to match the PR title and all other references in the document.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .agentworkforce/trajectories/completed/2026-06/traj_6kef2z1redhb/summary.md, line 1:
<comment>Typo in heading: "Rope" should be "Route" to match the PR title and all other references in the document.</comment>
<file context>
@@ -0,0 +1,45 @@
+# Trajectory: Rope telemetry through CLI SDK and hosted backend
+
+> **Status:** ✅ Completed
</file context>
| # Trajectory: Rope telemetry through CLI SDK and hosted backend | |
| # Trajectory: Route telemetry through CLI SDK and hosted backend |
|
Fixed the PR-local breakage I found:
Verification run locally:
Rust broker tests could not be run directly because this container has no Rust toolchain; the repo’s |
|
Fixed the PR-local breakage I found:
Verification run locally:
Rust broker tests could not be run directly because this container has no Rust toolchain; the repo’s |
|
Reviewed and fixed PR #1033 locally. Changes made:
Validated:
Cargo was not available in the container, so I could not run Rust telemetry tests locally. |
|
Reviewed and fixed PR #1033 locally. Changes made:
Validated:
Cargo was not available in the container, so I could not run Rust telemetry tests locally. |
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
size:XL
|
CodeAnt AI Incremental review completed. |
|
Reviewed PR #1033 and fixed the local breakages I found:
Local checks passed:
Rust broker tests were not run because Cargo is not installed in this environment. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/cli/commands/setup.ts (1)
75-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate stale “anonymous telemetry” wording in command help.
Line 75 still says “Manage anonymous telemetry…”, which conflicts with the new “usage telemetry” + distinct ID wording shown in this command.
Suggested patch
- .description('Manage anonymous telemetry (enable/disable/status)') + .description('Manage usage telemetry (enable/disable/status)')🤖 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/cli/src/cli/commands/setup.ts` at line 75, Update the command description string to replace the outdated "anonymous telemetry" phrasing with the new terminology used elsewhere ("usage telemetry" and the distinct ID). Locate the command builder where .description('Manage anonymous telemetry (enable/disable/status)') is set (in the setup command) and change the text to something like "Manage usage telemetry and distinct ID (enable/disable/status)" so the help output matches the updated UI/UX language.
♻️ Duplicate comments (1)
packages/cloud/src/telemetry-headers.ts (1)
26-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHonor
DO_NOT_TRACKindependently ofAGENT_RELAY_TELEMETRY_DISABLED.Line 27 currently uses
??, soAGENT_RELAY_TELEMETRY_DISABLED='0'can suppressDO_NOT_TRACK='true'. That can still emit telemetry despite an explicit DNT signal.Proposed fix
function isTelemetryDisabledByEnv(env: NodeJS.ProcessEnv): boolean { - const disabled = env.AGENT_RELAY_TELEMETRY_DISABLED ?? env.DO_NOT_TRACK; - return disabled === '1' || disabled?.toLowerCase() === 'true'; + const isTruthy = (value: string | undefined): boolean => + value === '1' || value?.toLowerCase() === 'true'; + + return isTruthy(env.AGENT_RELAY_TELEMETRY_DISABLED) || isTruthy(env.DO_NOT_TRACK); }🤖 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/cloud/src/telemetry-headers.ts` around lines 26 - 29, The function isTelemetryDisabledByEnv currently uses the nullish coalescing operator so AGENT_RELAY_TELEMETRY_DISABLED='0' can block DO_NOT_TRACK='true'; update isTelemetryDisabledByEnv to evaluate AGENT_RELAY_TELEMETRY_DISABLED and DO_NOT_TRACK independently (e.g., check both values and return true if either equals '1' or case-insensitive 'true'), referencing the function name isTelemetryDisabledByEnv and the env keys AGENT_RELAY_TELEMETRY_DISABLED and DO_NOT_TRACK.
🧹 Nitpick comments (3)
CHANGELOG.md (3)
35-35: ⚡ Quick winReconsider including internal telemetry infrastructure changes.
This entry describes backend telemetry implementation details rather than user-visible changes. The guidelines specify omitting internal notes and implementation details. Unless there's a direct user-facing benefit (e.g., "Telemetry now persists across CLI restarts"), consider removing this entry or rewriting it to focus on observable user impact.
As per coding guidelines: "Changelog entries should be concise and impact-first, with one short bullet per user-visible change. Omit issue/PR links, internal notes, and implementation details."
🤖 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 `@CHANGELOG.md` at line 35, This changelog entry exposes internal telemetry implementation; remove or rewrite it to be user-facing and concise: either delete the `agent-relay` / Relaycast telemetry bullet entirely or replace it with a one-line, impact-first statement that users care about (e.g., "CLI and SDK requests are now distinguished in backend telemetry so support can better diagnose usage") without mentioning internal systems like `agent-relay`, `Relaycast`, or implementation details about client identity context. Ensure the final bullet follows the project's rule of one short, user-visible change per line.
49-49: ⚡ Quick winRemove implementation details about attribution mechanism.
The second clause "SDK/API attribution uses Relaycast origin metadata instead" describes internal implementation details. Consider removing everything after "telemetry`" to keep the entry focused on the user-visible change (dependency removal).
✂️ Suggested simplification
-- `@agent-relay/sdk` no longer emits client-side analytics or depends on `@agent-relay/telemetry`; SDK/API attribution uses Relaycast origin metadata instead. +- `@agent-relay/sdk` no longer emits client-side analytics or depends on `@agent-relay/telemetry`.As per coding guidelines: "Changelog entries should be concise and impact-first, with one short bullet per user-visible change. Omit issue/PR links, internal notes, and implementation details."
🤖 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 `@CHANGELOG.md` at line 49, Update the CHANGELOG entry by removing the internal implementation clause so the bullet reads only that `@agent-relay/sdk` no longer emits client-side analytics or depends on `@agent-relay/telemetry`; delete the trailing phrase "SDK/API attribution uses Relaycast origin metadata instead" and leave the concise, user-facing statement about dependency removal.
48-48: ⚡ Quick winRemove internal build process details.
This entry describes an internal build validation change that isn't user-visible. The guidelines specify focusing on user-visible changes only. Unless this build change affects what users can install or how the packages behave, this should be removed from the changelog.
As per coding guidelines: "Changelog entries should be concise and impact-first, with one short bullet per user-visible change. Omit issue/PR links, internal notes, and implementation details."
🤖 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 `@CHANGELOG.md` at line 48, The changelog contains an internal build-process detail ("Root builds now validate the simplified core package set: config, utils, SDK, harness-driver, harnesses, and CLI.") that must be removed or replaced with a user-visible impact statement; edit CHANGELOG.md to delete that line (or rewrite it as a concise, impact-first bullet describing any user-facing change such as "Simplified core packages to improve install size/stability" if there is an actual user impact) so the entry only contains user-visible changes and omits implementation/internal notes.
🤖 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.
Outside diff comments:
In `@packages/cli/src/cli/commands/setup.ts`:
- Line 75: Update the command description string to replace the outdated
"anonymous telemetry" phrasing with the new terminology used elsewhere ("usage
telemetry" and the distinct ID). Locate the command builder where
.description('Manage anonymous telemetry (enable/disable/status)') is set (in
the setup command) and change the text to something like "Manage usage telemetry
and distinct ID (enable/disable/status)" so the help output matches the updated
UI/UX language.
---
Duplicate comments:
In `@packages/cloud/src/telemetry-headers.ts`:
- Around line 26-29: The function isTelemetryDisabledByEnv currently uses the
nullish coalescing operator so AGENT_RELAY_TELEMETRY_DISABLED='0' can block
DO_NOT_TRACK='true'; update isTelemetryDisabledByEnv to evaluate
AGENT_RELAY_TELEMETRY_DISABLED and DO_NOT_TRACK independently (e.g., check both
values and return true if either equals '1' or case-insensitive 'true'),
referencing the function name isTelemetryDisabledByEnv and the env keys
AGENT_RELAY_TELEMETRY_DISABLED and DO_NOT_TRACK.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 35: This changelog entry exposes internal telemetry implementation;
remove or rewrite it to be user-facing and concise: either delete the
`agent-relay` / Relaycast telemetry bullet entirely or replace it with a
one-line, impact-first statement that users care about (e.g., "CLI and SDK
requests are now distinguished in backend telemetry so support can better
diagnose usage") without mentioning internal systems like `agent-relay`,
`Relaycast`, or implementation details about client identity context. Ensure the
final bullet follows the project's rule of one short, user-visible change per
line.
- Line 49: Update the CHANGELOG entry by removing the internal implementation
clause so the bullet reads only that `@agent-relay/sdk` no longer emits
client-side analytics or depends on `@agent-relay/telemetry`; delete the
trailing phrase "SDK/API attribution uses Relaycast origin metadata instead" and
leave the concise, user-facing statement about dependency removal.
- Line 48: The changelog contains an internal build-process detail ("Root builds
now validate the simplified core package set: config, utils, SDK,
harness-driver, harnesses, and CLI.") that must be removed or replaced with a
user-visible impact statement; edit CHANGELOG.md to delete that line (or rewrite
it as a concise, impact-first bullet describing any user-facing change such as
"Simplified core packages to improve install size/stability" if there is an
actual user impact) so the entry only contains user-visible changes and omits
implementation/internal notes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 137b81fa-917c-4405-89f6-c1887670ba3b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (26)
CHANGELOG.mdpackage.jsonpackages/cli/package.jsonpackages/cli/src/cli/bootstrap.tspackages/cli/src/cli/commands/cloud.test.tspackages/cli/src/cli/commands/cloud.tspackages/cli/src/cli/commands/setup.tspackages/cli/src/cli/entrypoint.test.tspackages/cli/src/cli/lib/broker-lifecycle.tspackages/cli/src/cli/lib/exit.tspackages/cli/src/cli/telemetry/client.tspackages/cli/src/cli/telemetry/config.tspackages/cli/src/cli/telemetry/events.tspackages/cli/src/cli/telemetry/index.tspackages/cli/src/cli/telemetry/machine-id.tspackages/cli/src/cli/telemetry/orchestrator-harness.test.tspackages/cli/src/cli/telemetry/orchestrator-harness.tspackages/cli/src/cli/telemetry/posthog-config.tspackages/cloud/src/auth.test.tspackages/cloud/src/telemetry-headers.tspackages/sdk/src/__tests__/agent-relay.test.tspackages/sdk/src/facade.tspackages/sdk/tsconfig.build.jsonpackages/sdk/tsconfig.jsonpackages/telemetry/package.jsonpackages/telemetry/tsconfig.json
💤 Files with no reviewable changes (4)
- packages/cli/src/cli/telemetry/orchestrator-harness.test.ts
- packages/telemetry/tsconfig.json
- packages/cli/src/cli/telemetry/events.ts
- packages/cli/src/cli/telemetry/orchestrator-harness.ts
✅ Files skipped from review due to trivial changes (2)
- packages/cli/src/cli/lib/exit.ts
- packages/cli/src/cli/commands/cloud.test.ts
|
Reviewed PR #1033 and fixed a regression in CLI telemetry initialization: the first-run notice is now shown and recorded before telemetry capture no-ops for missing PostHog config. Added coverage in client.test.ts. The code change is in client.ts. Validated:
Rust tests were not runnable locally because no Rust toolchain is installed; |
There was a problem hiding this comment.
1 issue found across 28 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/cli/src/cli/telemetry/config.ts">
<violation number="1" location="packages/cli/src/cli/telemetry/config.ts:33">
P2: Distinct ID migration accepts empty-string IDs, so invalid persisted IDs are no longer regenerated.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
|
||
| return { | ||
| const prefs = JSON.parse(content) as StoredTelemetryPrefs; | ||
| const distinctId = prefs.distinctId ?? prefs.anonymousId ?? createDistinctId(); |
There was a problem hiding this comment.
P2: Distinct ID migration accepts empty-string IDs, so invalid persisted IDs are no longer regenerated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/cli/src/cli/telemetry/config.ts, line 33:
<comment>Distinct ID migration accepts empty-string IDs, so invalid persisted IDs are no longer regenerated.</comment>
<file context>
@@ -27,26 +29,27 @@ export function loadPrefs(): TelemetryPrefs {
-
- return {
+ const prefs = JSON.parse(content) as StoredTelemetryPrefs;
+ const distinctId = prefs.distinctId ?? prefs.anonymousId ?? createDistinctId();
+ const normalized: TelemetryPrefs = {
enabled: prefs.enabled ?? true,
</file context>
| const distinctId = prefs.distinctId ?? prefs.anonymousId ?? createDistinctId(); | |
| const distinctId = prefs.distinctId?.trim() || prefs.anonymousId?.trim() || createDistinctId(); |
|
Reviewed PR #1033 against Verified locally:
Limitations:
|
|
Reviewed PR #1033 against Verified locally:
Limitations:
|
|
Reviewed PR #1033 and fixed a cloud auth regression in Local verification:
|
|
Reviewed PR #1033 and fixed a cloud auth regression in Local verification:
|
|
CodeAnt AI is running Incremental review |
codeant-ai
Bot
added
size:XL
|
CodeAnt AI Incremental review completed. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/cli/src/cli/telemetry/posthog-config.ts (1)
32-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
POSTHOG_HOSTbefore applying overrideLine 32 currently accepts whitespace-only values (e.g.
' ') as a valid override, which can produce an invalid host and break telemetry initialization. Reuse trimmed parsing here, like key parsing.Suggested fix
const HOST = 'https://i.agentrelay.com'; function readKey(name: string): string | null { const raw = process.env[name]; if (!raw) return null; const trimmed = raw.trim(); return trimmed.length > 0 ? trimmed : null; } + +function readHost(name: string): string | null { + const raw = process.env[name]; + if (!raw) return null; + const trimmed = raw.trim(); + return trimmed.length > 0 ? trimmed : null; +} export function getPostHogConfig(): { apiKey: string; host: string } | null { - const host = process.env.POSTHOG_HOST || HOST; + const host = readHost('POSTHOG_HOST') || HOST;🤖 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/cli/src/cli/telemetry/posthog-config.ts` at line 32, The POSTHOG_HOST override is currently taken verbatim, allowing whitespace-only values to become an invalid host; update the logic around the host constant so you trim and validate process.env.POSTHOG_HOST before using it: compute a trimmed value of process.env.POSTHOG_HOST (or null/empty check) and only assign that to the host variable when non-empty, otherwise fall back to the existing HOST constant (refer to POSTHOG_HOST, the host constant declaration, and HOST).
🧹 Nitpick comments (1)
packages/cli/src/cli/telemetry/posthog-config.test.ts (1)
6-10: ⚡ Quick winAdd a regression test for whitespace
POSTHOG_HOSTCurrent cases don’t cover the whitespace override edge case. Add a test asserting that
POSTHOG_HOST=' 'falls back tohttps://i.agentrelay.com.Suggested test case
it('allows POSTHOG_HOST to override the hosted proxy', () => { vi.stubEnv('POSTHOG_API_KEY', 'debug-key'); vi.stubEnv('POSTHOG_HOST', 'https://posthog.example.test'); expect(getPostHogConfig()).toEqual({ apiKey: 'debug-key', host: 'https://posthog.example.test', }); }); + + it('falls back to hosted proxy when POSTHOG_HOST is blank/whitespace', () => { + vi.stubEnv('POSTHOG_API_KEY', 'debug-key'); + vi.stubEnv('POSTHOG_HOST', ' '); + + expect(getPostHogConfig()).toEqual({ + apiKey: 'debug-key', + host: 'https://i.agentrelay.com', + }); + });Also applies to: 25-33
🤖 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/cli/src/cli/telemetry/posthog-config.test.ts` around lines 6 - 10, Add a regression test in packages/cli/src/cli/telemetry/posthog-config.test.ts that stubs POSTHOG_HOST to a whitespace string (e.g., vi.stubEnv('POSTHOG_HOST', ' ')) and asserts that the code under test (the module being tested in this file) falls back to the default host 'https://i.agentrelay.com'; place the new test alongside existing cases (after the beforeEach block) and use the same test helper or function used in other tests in this file to obtain the resolved host and expect it to equal 'https://i.agentrelay.com'.
🤖 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.
Outside diff comments:
In `@packages/cli/src/cli/telemetry/posthog-config.ts`:
- Line 32: The POSTHOG_HOST override is currently taken verbatim, allowing
whitespace-only values to become an invalid host; update the logic around the
host constant so you trim and validate process.env.POSTHOG_HOST before using it:
compute a trimmed value of process.env.POSTHOG_HOST (or null/empty check) and
only assign that to the host variable when non-empty, otherwise fall back to the
existing HOST constant (refer to POSTHOG_HOST, the host constant declaration,
and HOST).
---
Nitpick comments:
In `@packages/cli/src/cli/telemetry/posthog-config.test.ts`:
- Around line 6-10: Add a regression test in
packages/cli/src/cli/telemetry/posthog-config.test.ts that stubs POSTHOG_HOST to
a whitespace string (e.g., vi.stubEnv('POSTHOG_HOST', ' ')) and asserts that
the code under test (the module being tested in this file) falls back to the
default host 'https://i.agentrelay.com'; place the new test alongside existing
cases (after the beforeEach block) and use the same test helper or function used
in other tests in this file to obtain the resolved host and expect it to equal
'https://i.agentrelay.com'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a78cc1ce-ec1a-4c73-a5c8-1c5861f5d356
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
CHANGELOG.mdpackages/cli/src/cli/telemetry/posthog-config.test.tspackages/cli/src/cli/telemetry/posthog-config.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/package-validation.yml (1)
127-129: ⚡ Quick winUpdate comment to reflect non-broker packages in skip list.
The comment explains that broker-* packages are skipped because they ship Rust binaries, but the skip list at line 130 also includes several non-broker packages (
build-plans,brand,personas,telemetry) that don't fit this description. Consider updating the comment to clarify that the skip list covers both broker packages and other non-Node package directories.🤖 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 @.github/workflows/package-validation.yml around lines 127 - 129, Update the explanatory comment above the dist-file check to accurately describe the skip list: mention that it skips broker-* packages (which ship Rust-built binaries) as well as other non-Node package directories such as build-plans, brand, personas, and telemetry; adjust the wording near the existing comment block so it explicitly states "broker packages and other non-Node package directories (e.g., build-plans, brand, personas, telemetry) are skipped" to match the skip list.
🤖 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 `@packages/cli/src/cli/telemetry/machine-id.test.ts`:
- Line 36: The test builds a RegExp from os.hostname() without escaping, so
hostnames containing regex metacharacters can break matching; update the test to
escape the hostname before interpolation (e.g., create an escapeRegex helper or
use a safe escape utility) and use the escaped value in the RegExp call used in
the expect assertion (the line creating new
RegExp(`^${os.hostname()}-[a-f0-9]{16}$`) and the similar line at 43), ensuring
the pattern becomes ^<escaped-hostname>-[a-f0-9]{16}$.
---
Nitpick comments:
In @.github/workflows/package-validation.yml:
- Around line 127-129: Update the explanatory comment above the dist-file check
to accurately describe the skip list: mention that it skips broker-* packages
(which ship Rust-built binaries) as well as other non-Node package directories
such as build-plans, brand, personas, and telemetry; adjust the wording near the
existing comment block so it explicitly states "broker packages and other
non-Node package directories (e.g., build-plans, brand, personas, telemetry) are
skipped" to match the skip list.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f5607a85-9ab4-4fd5-968f-c00adf485c66
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/package-validation.ymlCHANGELOG.mdcrates/broker/src/telemetry.rspackages/cli/package.jsonpackages/cli/src/cli/telemetry/config.test.tspackages/cli/src/cli/telemetry/machine-id.test.ts
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/cli/package.json
- crates/broker/src/telemetry.rs
| it('creates a machine id when none exists', () => { | ||
| const machineId = loadMachineId(); | ||
|
|
||
| expect(machineId).toMatch(new RegExp(`^${os.hostname()}-[a-f0-9]{16}$`)); |
There was a problem hiding this comment.
Escape hostname in regex pattern to prevent matching issues.
When os.hostname() contains regex metacharacters (e.g., dots, brackets), the unescaped interpolation can cause incorrect pattern matching or test failures. Although this is test code and the risk is low, escaping ensures correctness across different environments.
🛡️ Proposed fix to escape hostname in regex patterns
- expect(machineId).toMatch(new RegExp(`^${os.hostname()}-[a-f0-9]{16}$`));
+ expect(machineId).toMatch(new RegExp(`^${os.hostname().replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}-[a-f0-9]{16}$`));And for line 43:
- expect(loadMachineId()).toMatch(new RegExp(`^${os.hostname()}-[a-z0-9]+$`));
+ expect(loadMachineId()).toMatch(new RegExp(`^${os.hostname().replace(/[.*+?^${}()|[\]\\]/g, '\\$&')}-[a-z0-9]+$`));Also applies to: 43-43
🤖 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/cli/src/cli/telemetry/machine-id.test.ts` at line 36, The test
builds a RegExp from os.hostname() without escaping, so hostnames containing
regex metacharacters can break matching; update the test to escape the hostname
before interpolation (e.g., create an escapeRegex helper or use a safe escape
utility) and use the escaped value in the RegExp call used in the expect
assertion (the line creating new RegExp(`^${os.hostname()}-[a-f0-9]{16}$`) and
the similar line at 43), ensuring the pattern becomes
^<escaped-hostname>-[a-f0-9]{16}$.
1 participant
User description
Summary
https://i.agentrelay.comby default, withPOSTHOG_HOSTretained as an override@agent-relay/sdk; SDK/API attribution now relies on Relaycast origin metadataagent-relayCLI and leave@agent-relay/telemetryas a private/deprecated workspace placeholderAGENT_RELAY_DISTINCT_ID->X-Agent-Relay-Distinct-IdVerification
npm --prefix packages/cloud run buildnpm --prefix packages/cli run buildnpm --prefix packages/sdk run buildnpm --prefix packages/sdk run testnpx vitest run packages/cloud/src/auth.test.tsnpx vitest run packages/cli/src/cli/bootstrap.test.ts packages/cli/src/cli/entrypoint.test.ts packages/cli/src/cli/commands/cloud.test.ts packages/cli/src/cli/telemetry/orchestrator-harness.test.tsnpx vitest run packages/cli/src/cli/telemetry/posthog-config.test.ts packages/cli/src/cli/telemetry/orchestrator-harness.test.tsnpm --prefix packages/cli run pack:validateCodeAnt-AI Description
Route telemetry context through the CLI, SDK, cloud requests, and hosted backend
What Changed
Impact
✅ Clearer telemetry attribution✅ Safer cloud request identity✅ Fewer duplicate analytics events💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.