feat(telemetry): add privacy-constrained command usage telemetry#151
Merged
Conversation
Introduce an opt-out telemetry pipeline so we can understand command adoption, error rates, update health, and package or skill usage distribution without harvesting user content. Events are emitted by the generic executeCli path and persisted to a local outbox before being flushed to PostHog Cloud (EU region) with person profile processing disabled and a random local device id. A telemetry-decisions architecture test forces every registered command to make an explicit telemetry decision and rejects forbidden, private, or sensitive property names at build time. Add `oo telemetry status|enable|disable`, the `telemetry.enabled` config key, and `OO_TELEMETRY_DISABLED` / `DO_NOT_TRACK` env overrides. Disabling telemetry attempts to purge pending local events immediately and prevents future sends. Document the boundary in PRIVACY.md, PRIVACY-ZH_CN.md, README files, and docs/commands*.md so users can audit exactly what is and is not collected. Signed-off-by: Kevin Cui <bh@bugs.cc>
|
Caution Review failedPull request was closed or merged during review Summary by CodeRabbit
WalkthroughAdds a complete telemetry system: schemas, payload builder/validator, SQLite outbox, and flusher with retry/backoff/splitting. Extends CLI contracts, settings, and store paths. Wires an observer/recorder into the commander adapter and run-cli, emits telemetry post-execution, and can spawn a flusher. Introduces a telemetry CLI (status/enable/disable) and a decisions guard test. Instruments many commands to record low-cardinality properties. Includes extensive tests and documentation/i18n updates. Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Recorder
participant Emitter
participant OutboxDB
participant Flusher
participant PostHog
User->>CLI: Run command
CLI->>Recorder: Resolved/Failed/Completed
CLI->>Emitter: Emit if enabled
Emitter->>OutboxDB: Enqueue payload
Emitter->>Flusher: Maybe spawn
Flusher->>OutboxDB: Lease rows
Flusher->>PostHog: POST batch
PostHog-->>Flusher: 2xx/4xx/5xx/timeout
Flusher->>OutboxDB: Delete/Split/Backoff/Release
Possibly related PRs
✨ Finishing Touches✨ Simplify code
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (9)
src/application/commands/file/download.cli.test.ts (1)
53-57: ⚡ Quick winPrefer
resolveStorePathsover manually constructing the telemetry directory path.
join(sandbox.env.XDG_CONFIG_HOME!, APP_NAME, "telemetry")hardcodes the"telemetry"directory name (duplicating the privatedefaultTelemetryDirectoryNameconstant instore-path.ts) and assumesXDG_CONFIG_HOMEis always populated.update.test.tsalready usesresolveStorePathswhich handles all three platform layouts and stays in sync automatically.♻️ Proposed refactor
+import { resolveStorePaths } from "../../../adapters/store/store-path.ts"; import { APP_NAME } from "../../config/app-config.ts"; import { parseTelemetryRowPayload, readTelemetryRowsForTest, } from "../../telemetry/outbox.ts"; // inside the test, after creating sandbox: + const storePaths = resolveStorePaths({ + appName: APP_NAME, + env: sandbox.env, + platform: process.platform, + }); const telemetryPayload = parseTelemetryRowPayload( - readTelemetryRowsForTest( - join(sandbox.env.XDG_CONFIG_HOME!, APP_NAME, "telemetry"), - )[0]!, + readTelemetryRowsForTest(storePaths.telemetryDirectory)[0]!, );🤖 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 `@src/application/commands/file/download.cli.test.ts` around lines 53 - 57, The test constructs the telemetry directory manually using join(sandbox.env.XDG_CONFIG_HOME!, APP_NAME, "telemetry"), duplicating the private defaultTelemetryDirectoryName and assuming XDG_CONFIG_HOME exists; replace that manual path with the platform-aware resolver by calling resolveStorePaths(sandbox.env) and using its telemetry path (the property returned by resolveStorePaths used elsewhere in update.test.ts) as the argument to readTelemetryRowsForTest so parseTelemetryRowPayload(readTelemetryRowsForTest(resolveStorePaths(sandbox.env).telemetry)[0]!) is used instead of the hardcoded join.src/application/commands/cloud-task/wait.test.ts (1)
324-332: ⚡ Quick winExtract the telemetry mock factory to
__tests__/helpers.ts.The inline telemetry mock construction here (Lines 324–332) is byte-for-byte identical to the one in
src/application/commands/package/search.test.ts(Lines 240–248). Apply thecreateMockTelemetryhelper from__tests__/helpers.tssuggested there:♻️ Proposed change in createWaitContext
- telemetry: options.telemetryProperties === undefined - ? undefined - : { - directoryPath: "", - recordProperties(properties) { - Object.assign(options.telemetryProperties!, properties); - }, - suppressCurrentInvocation() {}, - }, + telemetry: options.telemetryProperties === undefined + ? undefined + : createMockTelemetry(options.telemetryProperties),Based on learnings: "When identical logic appears in 2+ files, extract it to a shared module" and "If a helper function might be called by other test files, it must be placed in the
__tests__/helpers.tsfile."🤖 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 `@src/application/commands/cloud-task/wait.test.ts` around lines 324 - 332, Replace the inline telemetry mock in createWaitContext with the shared helper: import and call createMockTelemetry from __tests__/helpers.ts instead of constructing the object inline; pass options.telemetryProperties (or undefined) into createMockTelemetry and use its return value for the telemetry property in createWaitContext so the byte-for-byte duplicate block (the directoryPath/recordProperties/suppressCurrentInvocation mock) is centralized in the helper.src/application/commands/package/search.test.ts (1)
240-248: ⚡ Quick winExtract the telemetry mock factory to
__tests__/helpers.ts.The inline construction of the telemetry mock (the ternary that produces
{ directoryPath, recordProperties, suppressCurrentInvocation }) is duplicated insrc/application/commands/cloud-task/wait.test.ts(Lines 324–332). A small shared helper in__tests__/helpers.tswould eliminate the duplication.♻️ Proposed extraction to `__tests__/helpers.ts`
+ // In __tests__/helpers.ts + export function createMockTelemetry( + properties: Record<string, CliTelemetryPropertyValue>, + ): CliTelemetryContext { + return { + directoryPath: "", + recordProperties(props) { + Object.assign(properties, props); + }, + suppressCurrentInvocation() {}, + }; + }Then replace the inline block in
createSearchContext:- telemetry: options.telemetryProperties === undefined - ? undefined - : { - directoryPath: "", - recordProperties(properties) { - Object.assign(options.telemetryProperties!, properties); - }, - suppressCurrentInvocation() {}, - }, + telemetry: options.telemetryProperties === undefined + ? undefined + : createMockTelemetry(options.telemetryProperties),Apply the same substitution in
cloud-task/wait.test.ts.Based on learnings: "When identical logic appears in 2+ files, extract it to a shared module" and "If a helper function might be called by other test files, it must be placed in the
__tests__/helpers.tsfile."🤖 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 `@src/application/commands/package/search.test.ts` around lines 240 - 248, Extract the inline telemetry mock into a shared test helper named makeTelemetryMock that returns { directoryPath: "", recordProperties(properties){ Object.assign(telemetryPropertiesRef, properties); }, suppressCurrentInvocation(){} } and accept a reference to the options.telemetryProperties object; then replace the ternary in createSearchContext (the block that checks options.telemetryProperties and constructs the mock) with a call to makeTelemetryMock(options.telemetryProperties) and apply the same replacement in the cloud-task/wait test so both tests reuse the helper.src/application/commands/cloud-task/index.cli.test.ts (1)
12-15: ⚡ Quick winMove
readTelemetryRowsForTestto__tests__/helpers.ts.
readTelemetryRowsForTestis exported from the production moduleoutbox.tsbut is only used in test files. Per the coding guideline, test helpers called by multiple test files must be placed in__tests__/helpers.tsrather than in production modules.🤖 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 `@src/application/commands/cloud-task/index.cli.test.ts` around lines 12 - 15, The helper readTelemetryRowsForTest is a test-only utility currently exported from the production module (outbox.ts); move its implementation into the test helpers file __tests__/helpers.ts, stop exporting it from outbox.ts, and update all test imports (including this test that currently imports readTelemetryRowsForTest alongside parseTelemetryRowPayload) to import readTelemetryRowsForTest from __tests__/helpers.ts while keeping parseTelemetryRowPayload imported from the production module; ensure the moved function name and signature remain identical so tests continue to work.src/application/bootstrap/run-cli.test.ts (1)
788-792: ⚡ Quick winPromote this telemetry-directory helper to shared test utilities.
This path logic has already started to duplicate across telemetry tests. Reusing
resolveStorePaths(...).telemetryDirectoryor moving the helper into__tests__/helpers.tswill keep the assertions aligned when the store layout changes again.As per coding guidelines: If a helper function might be called by other test files, it must be placed in the
__tests__/helpers.tsfile.🤖 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 `@src/application/bootstrap/run-cli.test.ts` around lines 788 - 792, The helper resolveSandboxTelemetryDirectory duplicates path logic; move it to the shared test helpers so other tests reuse the same logic: replace usages of resolveSandboxTelemetryDirectory in this test with a call to resolveStorePaths(...).telemetryDirectory (or export a helper from __tests__/helpers.ts that returns join(APP_NAME, "telemetry") based on the supplied sandbox.env/XDG_CONFIG_HOME) and remove the local function; ensure you reference the existing resolveStorePaths/telemetryDirectory API or export the new helper from __tests__/helpers.ts so all tests import the same implementation.src/application/telemetry/control.ts (1)
119-123: 💤 Low valueConsider extracting duplicated
isPlainObjectRecordutility.This type guard is identical to the one in
settings.ts(lines 257-261). Based on learnings, when identical logic appears in 2+ files, it should be extracted to a shared module.🤖 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 `@src/application/telemetry/control.ts` around lines 119 - 123, Duplicate type guard isPlainObjectRecord should be extracted to a shared utility module: create a new export (e.g., export function isPlainObjectRecord(value: unknown): value is Record<string, unknown>) in a common utils file, replace the local implementations in control.ts and settings.ts with imports from that shared module, update any import paths/usages to reference the new shared function, and remove the duplicated definitions to avoid drift.src/application/telemetry/emitter.ts (1)
252-257: 💤 Low valueConsider extracting duplicated
isNodeNotFoundErrorutility.This error-checking pattern is common across the codebase for ENOENT handling. If this pattern exists elsewhere, consider extracting it to a shared utility.
🤖 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 `@src/application/telemetry/emitter.ts` around lines 252 - 257, The isNodeNotFoundError function duplicates an ENOENT check and should be extracted to a shared utility for reuse: create a small exported helper (e.g., export function isNodeNotFoundError(error: unknown): boolean) in a common utility module (e.g., telemetry/utils or shared/errors), replace the local isNodeNotFoundError implementation in emitter.ts with an import from that module, and update any other files that implement the same pattern to import and use this shared helper (ensure the exported name matches usages and update imports accordingly).src/application/commands/config/set.ts (1)
49-55: ⚡ Quick winRemove redundant validation.
The
isConfigKey(input.key)check is unnecessary. The input schema (configSetInputSchema) already validates the key usingconfigKeySchema, soinput.keyis guaranteed to be a validConfigKeyby the time the handler executes.As per coding guidelines: "Do not re-parse or re-validate data whose type is already guaranteed by the function signature."
Proposed fix
handler: async (input, context) => { context.telemetry?.recordProperties({ config_key: input.key }); - const definition = isConfigKey(input.key) - ? configDefinitions[input.key] - : undefined; - - if (!definition) { - throw createInvalidConfigKeyError({ key: input.key }); - } + const definition = configDefinitions[input.key]; const parsedValue = definition.parseRawValue(input.value);🤖 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 `@src/application/commands/config/set.ts` around lines 49 - 55, Remove the redundant runtime validation: the input.key is already validated by configSetInputSchema/configKeySchema, so delete the isConfigKey(...) check and the createInvalidConfigKeyError branch; instead directly read the definition from configDefinitions using definition = configDefinitions[input.key] (or use a non-null assertion if needed) and proceed—remove references to isConfigKey and the throw of createInvalidConfigKeyError from the handler in set.ts.src/application/telemetry/flusher.ts (1)
161-175: 💤 Low valueMinor inefficiency: redundant serialization.
The
nextBodycomputed at line 159 is discarded when finalizing a chunk, and thencreateTelemetryBatchRequestBody(currentItems)is called again at line 169 to produce the same result. However, this pattern ensures correctness whencurrentRows.length === 0, so the current approach is acceptable.🤖 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 `@src/application/telemetry/flusher.ts` around lines 161 - 175, The code redundantly calls createTelemetryBatchRequestBody(currentItems) when closing a chunk even though nextBody already contains that serialization; to fix, when the condition in the chunking logic in flusher.ts is met use nextBody as the chunk body instead of recomputing: push { body: nextBody, rows: currentRows } into chunks, then reset currentRows = [row] and currentItems = [item] as you already do; ensure this preserves the existing correctness when currentRows.length === 0 (i.e., nextBody was computed from currentItems) and keeps all other variables (chunks, row, item, telemetryChunkMaxEvents, telemetryChunkMaxBytes) unchanged.
🤖 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 `@src/application/bootstrap/run-cli.ts`:
- Around line 333-349: The call to emitCliCommandTelemetry(...) in executeCli
can throw and, because it's in the finally path after the command has finished,
could override the settled exitCode; wrap the emitCliCommandTelemetry invocation
in a try/catch so any errors are caught and do not bubble up (do not rethrow),
and log the failure (using logger or telemetryRecorder) so telemetry failures
cannot change the process result; ensure you still await emitCliCommandTelemetry
but swallow/log exceptions to preserve the original exitCode and behavior of
executeCli.
In `@src/application/commands/cloud-task/index.cli.test.ts`:
- Around line 201-203: Add a negative assertion for the snake_case key to mirror
the log test: update the test that currently asserts
expect(telemetryPayload?.properties).not.toHaveProperty("data"),
expect(...).not.toHaveProperty("input"), and
expect(...).not.toHaveProperty("taskID") by also asserting
expect(telemetryPayload?.properties).not.toHaveProperty("task_id"); this keeps
coverage consistent with the cloud-task.log test and ensures neither "taskID"
nor "task_id" is accidentally recorded.
In `@src/application/commands/cloud-task/list.ts`:
- Around line 112-122: The current code records raw CLI option values packageId
and blockId into telemetryProperties and calls
context.telemetry?.recordProperties, which leaks user-supplied identifiers;
change this to only record whether each filter was provided (e.g., set telemetry
keys like package_filter_provided and block_filter_provided to "true" when
packageId/blockId are defined) and remove the telemetryProperties intermediate
variable; update the call to context.telemetry?.recordProperties to only submit
those presence flags instead of the raw packageId/blockId values.
In `@src/application/commands/cloud-task/run.ts`:
- Around line 113-118: Remove high-cardinality identifier fields from the
telemetry call in context.telemetry?.recordProperties: stop sending
block.blockName, packageInfo.packageName, and packageInfo.packageVersion; keep
only privacy-safe low-cardinality properties such as input.dryRun. If you need
to capture those concepts, replace them with enumerated/bucketed dimensions
(e.g., block_type_enum, package_version_major_bucket) produced by helper
functions, and call recordProperties with those low-cardinality keys instead of
the raw strings.
In `@src/application/commands/connector/run.ts`:
- Around line 94-101: Replace free-form telemetry fields with low-cardinality
values: instead of passing actionName and input.serviceName directly into
context.telemetry.recordProperties, map them to bounded enums/buckets (e.g., via
new helpers mapActionToEnum(actionName) and
mapServiceToEnum(input.serviceName)); keep data_size_bucket and dry_run as-is;
likewise for the error_code emitted around the error handling later (replace any
raw error string with mapErrorToEnum(error) or a small set of predefined error
codes). Update the call site of context.telemetry.recordProperties to use these
mapping helpers so only enumerated/bucketed/boolean values are recorded.
In `@src/application/commands/package/info.test.ts`:
- Around line 165-168: The test currently asserts raw package identity fields
via telemetryProperties (package_name and package_version); replace these with
low-cardinality, privacy-safe dimensions by asserting on bucketed or enum-based
keys (e.g., package_name_bucket or package_family and package_version_bucket or
version_major_bucket) or presence of an allowed enum value/flag instead of the
raw strings; update the assertions in the test that reference
telemetryProperties so they check for the bucket/enum keys and expected allowed
values (or that the values match a regex/bucketing function) and remove checks
for the raw package_name and package_version fields.
In `@src/application/commands/skills/publish.test.ts`:
- Around line 115-122: Tests are asserting raw telemetry properties package_name
and skill_id (in the properties object in publish.test.ts and the other
assertions around lines 453–459); change the tests to expect privacy-safe,
low-cardinality fields instead (for example assert a package_name_bucket or
package_origin enum, and a skill_id_present boolean or skill_id_bucket) and
remove any expectation of the raw package_name/skill_id values; update both the
properties assertion block and the other related assertions to use these
buckets/enums so tests match the telemetry contract.
In `@src/application/commands/skills/publish.ts`:
- Around line 227-233: Telemetry currently records sensitive identifiers:
package_name (from resolveCanonicalSkillPackageName) and skill_id (raw CLI
input); remove these fields from the telemetry.recordProperties calls in both
publishLocalSkillPackage and publishSkillPackage (the blocks around the existing
context.telemetry?.recordProperties calls) so only non-identifying properties
(adopted, source_kind, visibility) are sent. If you need aggregated skill-name
analytics, route that through the existing telemetry helpers in
src/application/commands/skills/telemetry.ts which apply sampling/truncation
rather than emitting raw account or user-supplied identifiers.
In `@src/application/commands/skills/registry-skill-install.ts`:
- Around line 97-101: The telemetry call is emitting a raw high-cardinality
package_name (packageInfo.packageName); replace that field with the
low-cardinality package telemetry properties provided by the skills telemetry
helper: import and call the package telemetry helper (e.g.,
createPackageTelemetryProperties or the equivalent export from the skills
telemetry module) passing packageInfo, and spread its returned properties into
context.telemetry?.recordProperties alongside the existing
createSkillIdsTelemetryProperties([]), removing the raw package_name entry.
In `@src/application/commands/telemetry/status.ts`:
- Around line 56-66: formatTelemetryEnabledStatus currently returns hard-coded
English strings ("true", "false (env)", "false (config)"); change it to produce
localized strings by using the app's translator instead of raw text. Update the
function signature (formatTelemetryEnabledStatus) to accept the translation
function (e.g. t: (key: string, opts?: any) => string) or import the existing
translator used elsewhere in this file, then replace the three literal returns
with calls to t() using appropriate i18n keys (e.g. telemetry.enabled.true,
telemetry.enabled.false_env, telemetry.enabled.false_config) so the output is
fully localized.
In `@src/application/telemetry/emitter.ts`:
- Around line 176-178: The if (!shouldSpawn) { return; } check inside the
finally block of the emitter function is unreachable because the same condition
already returns earlier (lines around the prior return) — remove that redundant
check and its return so the finally block only contains necessary cleanup/logic;
locate the unreachable check by searching for shouldSpawn within the function in
src/application/telemetry/emitter.ts (the function containing the earlier return
and the finally block) and delete the redundant if-statement, ensuring no other
logic depends on it.
In `@src/application/telemetry/outbox.test.ts`:
- Around line 103-114: Replace the local weak helper isUuidV7ForTest with the
shared, strict validator isUuidV7 from src/application/telemetry/uuid.ts: remove
the isUuidV7ForTest function, add an import for isUuidV7, and update any test
assertions that call isUuidV7ForTest to call isUuidV7 instead so tests use the
canonical hex-checking UUIDv7 validator.
---
Nitpick comments:
In `@src/application/bootstrap/run-cli.test.ts`:
- Around line 788-792: The helper resolveSandboxTelemetryDirectory duplicates
path logic; move it to the shared test helpers so other tests reuse the same
logic: replace usages of resolveSandboxTelemetryDirectory in this test with a
call to resolveStorePaths(...).telemetryDirectory (or export a helper from
__tests__/helpers.ts that returns join(APP_NAME, "telemetry") based on the
supplied sandbox.env/XDG_CONFIG_HOME) and remove the local function; ensure you
reference the existing resolveStorePaths/telemetryDirectory API or export the
new helper from __tests__/helpers.ts so all tests import the same
implementation.
In `@src/application/commands/cloud-task/index.cli.test.ts`:
- Around line 12-15: The helper readTelemetryRowsForTest is a test-only utility
currently exported from the production module (outbox.ts); move its
implementation into the test helpers file __tests__/helpers.ts, stop exporting
it from outbox.ts, and update all test imports (including this test that
currently imports readTelemetryRowsForTest alongside parseTelemetryRowPayload)
to import readTelemetryRowsForTest from __tests__/helpers.ts while keeping
parseTelemetryRowPayload imported from the production module; ensure the moved
function name and signature remain identical so tests continue to work.
In `@src/application/commands/cloud-task/wait.test.ts`:
- Around line 324-332: Replace the inline telemetry mock in createWaitContext
with the shared helper: import and call createMockTelemetry from
__tests__/helpers.ts instead of constructing the object inline; pass
options.telemetryProperties (or undefined) into createMockTelemetry and use its
return value for the telemetry property in createWaitContext so the
byte-for-byte duplicate block (the
directoryPath/recordProperties/suppressCurrentInvocation mock) is centralized in
the helper.
In `@src/application/commands/config/set.ts`:
- Around line 49-55: Remove the redundant runtime validation: the input.key is
already validated by configSetInputSchema/configKeySchema, so delete the
isConfigKey(...) check and the createInvalidConfigKeyError branch; instead
directly read the definition from configDefinitions using definition =
configDefinitions[input.key] (or use a non-null assertion if needed) and
proceed—remove references to isConfigKey and the throw of
createInvalidConfigKeyError from the handler in set.ts.
In `@src/application/commands/file/download.cli.test.ts`:
- Around line 53-57: The test constructs the telemetry directory manually using
join(sandbox.env.XDG_CONFIG_HOME!, APP_NAME, "telemetry"), duplicating the
private defaultTelemetryDirectoryName and assuming XDG_CONFIG_HOME exists;
replace that manual path with the platform-aware resolver by calling
resolveStorePaths(sandbox.env) and using its telemetry path (the property
returned by resolveStorePaths used elsewhere in update.test.ts) as the argument
to readTelemetryRowsForTest so
parseTelemetryRowPayload(readTelemetryRowsForTest(resolveStorePaths(sandbox.env).telemetry)[0]!)
is used instead of the hardcoded join.
In `@src/application/commands/package/search.test.ts`:
- Around line 240-248: Extract the inline telemetry mock into a shared test
helper named makeTelemetryMock that returns { directoryPath: "",
recordProperties(properties){ Object.assign(telemetryPropertiesRef, properties);
}, suppressCurrentInvocation(){} } and accept a reference to the
options.telemetryProperties object; then replace the ternary in
createSearchContext (the block that checks options.telemetryProperties and
constructs the mock) with a call to
makeTelemetryMock(options.telemetryProperties) and apply the same replacement in
the cloud-task/wait test so both tests reuse the helper.
In `@src/application/telemetry/control.ts`:
- Around line 119-123: Duplicate type guard isPlainObjectRecord should be
extracted to a shared utility module: create a new export (e.g., export function
isPlainObjectRecord(value: unknown): value is Record<string, unknown>) in a
common utils file, replace the local implementations in control.ts and
settings.ts with imports from that shared module, update any import paths/usages
to reference the new shared function, and remove the duplicated definitions to
avoid drift.
In `@src/application/telemetry/emitter.ts`:
- Around line 252-257: The isNodeNotFoundError function duplicates an ENOENT
check and should be extracted to a shared utility for reuse: create a small
exported helper (e.g., export function isNodeNotFoundError(error: unknown):
boolean) in a common utility module (e.g., telemetry/utils or shared/errors),
replace the local isNodeNotFoundError implementation in emitter.ts with an
import from that module, and update any other files that implement the same
pattern to import and use this shared helper (ensure the exported name matches
usages and update imports accordingly).
In `@src/application/telemetry/flusher.ts`:
- Around line 161-175: The code redundantly calls
createTelemetryBatchRequestBody(currentItems) when closing a chunk even though
nextBody already contains that serialization; to fix, when the condition in the
chunking logic in flusher.ts is met use nextBody as the chunk body instead of
recomputing: push { body: nextBody, rows: currentRows } into chunks, then reset
currentRows = [row] and currentItems = [item] as you already do; ensure this
preserves the existing correctness when currentRows.length === 0 (i.e., nextBody
was computed from currentItems) and keeps all other variables (chunks, row,
item, telemetryChunkMaxEvents, telemetryChunkMaxBytes) unchanged.
🪄 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
Run ID: 9f31334a-bb46-4973-a5b1-9e3a64125c61
⛔ Files ignored due to path filters (3)
src/application/bootstrap/__snapshots__/run-cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/config/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snapsrc/application/commands/telemetry/__snapshots__/index.cli.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (85)
AGENTS.mdCLAUDE.mdPRIVACY-ZH_CN.mdPRIVACY.mdREADME-ZH_CN.mdREADME.mddocs/commands.mddocs/commands.zh-CN.mdsrc/adapters/commander/commander-cli-adapter.tssrc/adapters/store/file-settings-store.test.tssrc/adapters/store/store-path.test.tssrc/adapters/store/store-path.tssrc/application/bootstrap/run-cli.test.tssrc/application/bootstrap/run-cli.tssrc/application/commands/auth/login.tssrc/application/commands/auth/logout.tssrc/application/commands/auth/status.tssrc/application/commands/auth/switch.tssrc/application/commands/catalog.tssrc/application/commands/check-update.cli.test.tssrc/application/commands/check-update.tssrc/application/commands/cloud-task/index.cli.test.tssrc/application/commands/cloud-task/list.tssrc/application/commands/cloud-task/log.tssrc/application/commands/cloud-task/result.tssrc/application/commands/cloud-task/run.tssrc/application/commands/cloud-task/wait.test.tssrc/application/commands/cloud-task/wait.tssrc/application/commands/completion.tssrc/application/commands/config/get.tssrc/application/commands/config/set.tssrc/application/commands/config/shared.tssrc/application/commands/config/unset.tssrc/application/commands/connector/index.cli.test.tssrc/application/commands/connector/run.tssrc/application/commands/connector/search.tssrc/application/commands/file/download.cli.test.tssrc/application/commands/file/download.tssrc/application/commands/file/upload.cli.test.tssrc/application/commands/file/upload.tssrc/application/commands/install.tssrc/application/commands/package/info.test.tssrc/application/commands/package/info.tssrc/application/commands/package/search.test.tssrc/application/commands/package/search.tssrc/application/commands/search.cli.test.tssrc/application/commands/search.tssrc/application/commands/self-update-telemetry.tssrc/application/commands/self-update.cli.test.tssrc/application/commands/skills/index.test.tssrc/application/commands/skills/install.tssrc/application/commands/skills/publish.test.tssrc/application/commands/skills/publish.tssrc/application/commands/skills/registry-skill-install.tssrc/application/commands/skills/search.test.tssrc/application/commands/skills/search.tssrc/application/commands/skills/telemetry.tssrc/application/commands/skills/update.test.tssrc/application/commands/skills/update.tssrc/application/commands/telemetry-decisions.test.tssrc/application/commands/telemetry/disable.tssrc/application/commands/telemetry/enable.tssrc/application/commands/telemetry/index.cli.test.tssrc/application/commands/telemetry/index.tssrc/application/commands/telemetry/status.tssrc/application/commands/update.tssrc/application/contracts/cli.tssrc/application/schemas/settings.tssrc/application/telemetry/__tests__/helpers.tssrc/application/telemetry/buckets.test.tssrc/application/telemetry/buckets.tssrc/application/telemetry/constants.tssrc/application/telemetry/control.test.tssrc/application/telemetry/control.tssrc/application/telemetry/emitter.tssrc/application/telemetry/flusher.test.tssrc/application/telemetry/flusher.tssrc/application/telemetry/invocation.tssrc/application/telemetry/outbox.test.tssrc/application/telemetry/outbox.tssrc/application/telemetry/payload.test.tssrc/application/telemetry/payload.tssrc/application/telemetry/status.tssrc/application/telemetry/uuid.tssrc/i18n/catalog.ts
Ensure database handles are closed when SQLite pragma setup or telemetry schema initialization fails. This prevents leaked handles and keeps recovery paths from leaving partially initialized databases open. Share symlink test setup so publish packaging coverage behaves consistently on Windows and Unix-like systems. Signed-off-by: Kevin Cui <bh@bugs.cc>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/application/commands/skills/publish.test.ts (1)
111-124:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRaw
package_nameandskill_idin telemetry assertions are still unresolved.Both telemetry assertion blocks assert raw user-supplied identifiers:
package_name: "@alice/demo-skill", // line 119 skill_id: "demo-skill", // line 120 package_name: "@alice/agent-skill", // line 456 skill_id: "agent-skill", // line 457Per coding guidelines, telemetry must never record raw user input, package names, or free-form identifiers. These values must be replaced with privacy-safe alternatives (e.g.,
package_scope: "scoped" | "unscoped",skill_id_present: true). The fix must also be applied upstream inpublish.tswhererecordProperties(...)is called.As per coding guidelines: "Never record raw user input, paths, cwd, filenames, URL hosts, account IDs/names/emails, usernames, hostnames, error messages, stack traces, tokens, secrets, or free-form option values. Record bucketed values or stable enums instead."
Also applies to: 448-461
🤖 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 `@src/application/commands/skills/publish.test.ts` around lines 111 - 124, The tests and upstream telemetry call are currently asserting and recording raw identifiers (package_name and skill_id); change telemetry to avoid recording raw user input by replacing those fields with privacy-safe buckets and booleans (e.g., package_scope: "scoped" | "unscoped" and skill_id_present: true/false) and update the tests accordingly. In publish.ts where recordProperties(...) is invoked, stop passing the raw package_name and skill_id and instead compute and pass package_scope (detect whether the package name is scoped) and skill_id_present (boolean indicating presence/format) along with any other allowed enums (e.g., visibility/source_kind unchanged); then update the test assertions in publish.test.ts (the parseTelemetryRowPayload assertions that currently reference package_name and skill_id) to expect the new privacy-safe keys and values. Ensure any other occurrences (e.g., the second block mentioned) are updated the same way.
🧹 Nitpick comments (2)
src/application/telemetry/outbox.ts (2)
489-491: ⚡ Quick win
closeTelemetryDatabaseis a single-line delegation with no added logic — inlinedatabase.close()directly.export function closeTelemetryDatabase(database: Database): void { database.close(); // zero logic, validation, or semantic value added }All call sites (five within this file, plus callers in other modules) should call
database.close()directly.As per coding guidelines: "Never create single-line functions that merely delegate to another function without adding logic, validation, or semantic value."
🤖 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 `@src/application/telemetry/outbox.ts` around lines 489 - 491, Remove the trivial helper closeTelemetryDatabase and replace all its usages with direct database.close() calls: delete the exported function closeTelemetryDatabase(database: Database): void { database.close(); }, update every call site in this file and other modules to invoke .close() on the Database instance directly, remove any import/exports referencing closeTelemetryDatabase, and run the TypeScript build/tsserver to ensure no remaining references.
317-375: ⚡ Quick win
leaseReadyTelemetryRowsusestry/catch/rethrowfor cleanup; usetry/finallyinstead.The
catchblock exclusively performs cleanup (ROLLBACK) and rethrows — the pattern the guideline explicitly targets.♻️ Proposed fix
- database.run("BEGIN IMMEDIATE"); + database.run("BEGIN IMMEDIATE"); + let committed = false; try { // ... SELECT + UPDATE rows ... database.run("COMMIT"); + committed = true; } - catch (error) { - database.run("ROLLBACK"); - throw error; + finally { + if (!committed) { + database.run("ROLLBACK"); + } }As per coding guidelines: "Use
try/finallyovertry/catch/rethrowfor cleanup. When the only purpose of acatchblock is to clean up and rethrow, usefinallyinstead."🤖 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 `@src/application/telemetry/outbox.ts` around lines 317 - 375, The try/catch currently only does cleanup (ROLLBACK) and rethrows; change it to try/finally: after database.run("BEGIN IMMEDIATE") use a boolean flag (e.g., committed = false), in the try block perform the query loop and call database.run("COMMIT") then set committed = true, and in a finally block call database.run("ROLLBACK") only if committed is still false; keep uses of leasedRows, leaseStatement, and the same parameters for leaseUntilMs/nowMs unchanged.
🤖 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.
Duplicate comments:
In `@src/application/commands/skills/publish.test.ts`:
- Around line 111-124: The tests and upstream telemetry call are currently
asserting and recording raw identifiers (package_name and skill_id); change
telemetry to avoid recording raw user input by replacing those fields with
privacy-safe buckets and booleans (e.g., package_scope: "scoped" | "unscoped"
and skill_id_present: true/false) and update the tests accordingly. In
publish.ts where recordProperties(...) is invoked, stop passing the raw
package_name and skill_id and instead compute and pass package_scope (detect
whether the package name is scoped) and skill_id_present (boolean indicating
presence/format) along with any other allowed enums (e.g.,
visibility/source_kind unchanged); then update the test assertions in
publish.test.ts (the parseTelemetryRowPayload assertions that currently
reference package_name and skill_id) to expect the new privacy-safe keys and
values. Ensure any other occurrences (e.g., the second block mentioned) are
updated the same way.
---
Nitpick comments:
In `@src/application/telemetry/outbox.ts`:
- Around line 489-491: Remove the trivial helper closeTelemetryDatabase and
replace all its usages with direct database.close() calls: delete the exported
function closeTelemetryDatabase(database: Database): void { database.close(); },
update every call site in this file and other modules to invoke .close() on the
Database instance directly, remove any import/exports referencing
closeTelemetryDatabase, and run the TypeScript build/tsserver to ensure no
remaining references.
- Around line 317-375: The try/catch currently only does cleanup (ROLLBACK) and
rethrows; change it to try/finally: after database.run("BEGIN IMMEDIATE") use a
boolean flag (e.g., committed = false), in the try block perform the query loop
and call database.run("COMMIT") then set committed = true, and in a finally
block call database.run("ROLLBACK") only if committed is still false; keep uses
of leasedRows, leaseStatement, and the same parameters for leaseUntilMs/nowMs
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d00284d0-9c40-415d-9da4-ac15a632a124
📒 Files selected for processing (5)
src/adapters/store/sqlite-utils.tssrc/application/commands/skills/__tests__/helpers.tssrc/application/commands/skills/package-conversion.test.tssrc/application/commands/skills/publish.test.tssrc/application/telemetry/outbox.ts
Wrap emitCliCommandTelemetry in try/catch so a failure in the telemetry pipeline only logs a warning and does not affect the CLI exit code or break user-facing flows. Remove the closeTelemetryDatabase wrapper and call database.close() directly at every call site. Reshape leaseReadyTelemetryRows to use try/finally with a committed flag so rollback runs on any failure path. Replace the local UUID v7 helper in outbox.test.ts with the shared isUuidV7 utility. Update telemetry decision reasons to describe the product-domain dimensions each command records, and tighten related tests to assert that raw task_id is never emitted. Signed-off-by: Kevin Cui <bh@bugs.cc>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introduce an opt-out telemetry pipeline so we can understand command adoption, error rates, update health, and package or skill usage distribution without harvesting user content.
Events are emitted by the generic executeCli path and persisted to a local outbox before being flushed to PostHog Cloud (EU region) with person profile processing disabled and a random local device id. A telemetry-decisions architecture test forces every registered command to make an explicit telemetry decision and rejects forbidden, private, or sensitive property names at build time.
Add
oo telemetry status|enable|disable, thetelemetry.enabledconfig key, andOO_TELEMETRY_DISABLED/DO_NOT_TRACKenv overrides. Disabling telemetry attempts to purge pending local events immediately and prevents future sends. Document the boundary in PRIVACY.md, PRIVACY-ZH_CN.md, README files, and docs/commands*.md so users can audit exactly what is and is not collected.