Proj/analytics system tool mode#726
Conversation
Adds a typed analytics: boolean field to the GlobalConfig domain entity with safe migration for legacy configs (defaults to false when absent to preserve the opt-in promise across upgrades). Widens toJson() return to a typed GlobalConfigJson shape. Type guard rejects non-boolean values when the analytics key is structurally present. Tests moved to test/unit/server/core/domain/entities/global-config.test.ts and extended with the seven scenarios required by the ticket.
Adds the read-only brv analytics status oclif command, which prints
whether CLI analytics is enabled or disabled. Output is "Analytics:
enabled" or "Analytics: disabled" by default; --format json emits the
shape {"analytics": "enabled" | "disabled"} via the repo's standard
writeJsonResponse envelope.
Routing follows login / logout / locations: oclif uses withDaemonRetry
to emit a globalConfig:get transport event (auto-spawns the daemon).
The daemon-side GlobalConfigHandler re-reads the on-disk config every
call (no in-memory cache); on a fresh install it seeds a stable
deviceId via GlobalConfig.create(randomUUID()) so device identity is
available from the first read.
Help text describes what telemetry is collected and links to a
placeholder privacy policy URL pending M1.5.
Adds two oclif commands that toggle the analytics flag in GlobalConfig: brv analytics enable and brv analytics disable. Output is "Analytics enabled" / "Analytics disabled" on a state change, or "Analytics already enabled" / "Analytics already disabled" when the flag was already in the target state (idempotent, exits 0). Routing matches M1.2: oclif uses withDaemonRetry to emit the new globalConfig:setAnalytics transport event. The daemon-side GlobalConfigHandler gains a SET_ANALYTICS listener that reads the current config (or seeds a fresh one with a stable deviceId if absent), applies the new value via GlobalConfig.fromJson round-trip (the entity is immutable with a private constructor), and writes back. The idempotent path skips the file write entirely when previous state matches the requested value, so a disable on a fresh install does not create a stub config file. M1.3 ships without the disclosure prompt - the enable command flips the bit immediately. M1.4 (ENG-2618) follows with the disclosure UX wrapper. Reviewers should expect that follow-up. Adds 13 new tests covering all 7 ticket Test plan scenarios: 4 deterministic command-level tests for enable/disable success and idempotent paths, 1 transport-contract assertion per command, plus 3 handler-level integration tests for SET-then-GET state coherence and concurrent-write last-writer-wins semantics.
Wraps M1.3's bit-flip path so brv analytics enable shows a disclosure that the user must accept before the flag flips. CI / non-interactive use is supported via --yes. Flow: the command first reads current state via globalConfig:get. If analytics is already enabled, it prints "Analytics already enabled" and exits 0 with no prompt and no write. Otherwise it loads the disclosure markdown, prints it, and either accepts implicitly (--yes), prompts via @inquirer/prompts confirm (interactive TTY), or refuses with a clear error (non-TTY without --yes). On accept, the existing globalConfig:setAnalytics path flips the bit; on reject, "Analytics not enabled" prints and the command exits 0 without writing. Disclosure copy lives in src/server/templates/sections/analytics-disclosure.md with lorem ipsum bodies per hoang's request in the Linear ticket. Section headers (what / surfaces / where / cross-device alias / how to disable / privacy policy) are load-bearing for tests and remain stable; PM and legal will replace bodies before the M1 release. Privacy policy URL lives in src/shared/constants/privacy.ts as a placeholder pending the M1.5 docs page; reviewers should update it once the canonical URL is finalized. A TODO(M2) marker is preserved in enable.ts so that when IAnalyticsClient lands the first event sent after enable will be analytics_enabled itself (industry practice). Adds 8 tests covering all 7 ticket scenarios: interactive accept, interactive reject, --yes bypass, already-enabled short-circuit, non-TTY refusal with non-zero exit, disclosure section coverage, and privacy URL constant shape. PM/legal sign-off and the milestone disclosure-file link are manual steps to record at PR / milestone-update time.
Establishes the consumer-facing contract for the daemon-side analytics
machinery. Future M2 sub-tickets (queue, resolvers, real client,
transport handler, IPC bridge) build against this interface; the no-op
serves as the daemon's default before M2.5 wires the real one and as a
stub for unit tests.
Adds the domain types in src/server/core/domain/analytics/:
- AnalyticsEvent (internal camelCase: name, properties, timestamp)
- Identity (wire snake_case: user_id?, device_id, email?, name?)
- AnalyticsEventWithIdentity (Identity-stamped event)
- AnalyticsBatchJson (wire shape: schema_version: 1, events)
- AnalyticsBatch class with private constructor + static create +
static fromJson (graceful failure, returns undefined on malformed
input) + toJson, mirroring the GlobalConfig pattern
Adds IAnalyticsClient { track; flush } in core/interfaces/analytics/
and NoOpAnalyticsClient in infra/analytics/. The no-op's track() is a
true no-op (no allocations beyond the call frame, no buffering); its
flush() always resolves to an empty batch.
Tests: 24 cases. AnalyticsBatch round-trips empty + populated batches,
and fromJson rejects 13 distinct malformations (missing schema_version,
schema_version != 1, events not array, missing/non-string event name,
missing identity, missing/empty device_id, non-number timestamp,
non-object properties, etc). NoOpAnalyticsClient verifies track does
not throw under varied input and flush stays empty after many tracks.
Adds the in-memory queue that M2.5's AnalyticsClient will buffer events into. Configurable cap with default 1000, drop-oldest on overflow, and a cumulative droppedCount that survives drain calls for later observability (surfaced in M4 via brv analytics status). The interface IAnalyticsQueue lives next to its sibling IAnalyticsClient under core/interfaces/analytics/. The BoundedQueue implementation in infra/analytics/ uses a plain Array with push + shift; at maxSize=1000 the O(n) shift on overflow is negligible. drain transfers ownership of the events array to the caller and re-initializes the internal queue, avoiding a defensive clone. dropped is a private counter that no method resets. The constructor validates maxSize is a non-negative integer; negative NaN, Infinity, and fractional values throw fast at construction time. Without this check, a negative maxSize would cause an infinite loop in push(): the `while events.length > maxSize` condition stays true at length === 0 when maxSize < 0, with shift() as a no-op. Tests cover the six ticket scenarios plus constructor validation: FIFO drain, empty-queue defaults, drop-oldest with droppedCount tracking, multi-drop FIFO order, cumulative droppedCount across drains, size() invariant, default-cap behavior, drain ownership transfer, and 5 constructor cases (negative, NaN, Infinity, fractional all throw; maxSize === 0 is accepted as a degenerate but valid cap). 17 tests total. Test fixtures use a fresh identity object per makeEvent call rather than a shared module-scope reference, so future tests cannot accidentally mutate the shared identity and bleed across cases. Note: had to introduce a small pushAll(queue, events[]) helper in the test file because the unicorn/no-array-push-push lint rule fires on consecutive .push() calls regardless of receiver type. The helper wraps a for-of loop, which the rule accepts.
Adds the resolver that stamps the five super properties onto every analytics event: device_id, cli_version, os, node_version, environment. M2.5's AnalyticsClient will inject this resolver and merge its output into every track() call. Wire-format snake_case throughout (matches the rest of the analytics domain). cli_version, os, node_version, and environment are static across the daemon's lifetime and lazy-cached on first resolve(). The package.json reader is invoked exactly once per resolver instance, verified by a sinon-stub assertion. device_id is re-read from IGlobalConfigStore on every resolve() call so a swapped GlobalConfig is observable. The tradeoff: resolve() returns Promise<SuperProperties> instead of sync SuperProperties, which cascades async to M2.5's AnalyticsClient.track() (deviation from M2.5's spec to be addressed when M2.5 lands). The cli-version reader was duplicated inline in brv-server.ts; this commit extracts it to src/server/utils/read-cli-version.ts and both sites now import the single source. The walk-up depth changes from 4 to 3 levels (the new utility lives one level closer to the project root) but resolves to the same package.json. environment defaults to 'production' when BRV_ENV is unset or set to any value other than 'development'. Fail-safe in unfamiliar territory. 10 tests cover the seven ticket scenarios plus three additional cases: device_id re-read on every call, environment default when BRV_ENV is unset, and BRV_ENV=production explicitly. Tests save & restore process.env.BRV_ENV per scenario to avoid cross-test pollution.
Adds IdentityResolver — produces the per-event Identity that M2.5 will
stamp onto every analytics event. Anonymous: {device_id} only.
Registered: {user_id, email?, name?, device_id} where empty user fields
are OMITTED (not present as undefined) so the wire envelope stays
clean for downstream serializers.
Per ticket DoD, defines a consumer-side IAuthStateReader interface
(1 method: getToken()) co-located with IIdentityResolver in
core/interfaces/analytics/. The full IAuthStateStore is broader; the
resolver only needs sync access to the cached token. This keeps the
auth module unaware of the analytics consumer, matching CLAUDE.md
"interfaces at the consumer".
resolve() is async because IGlobalConfigStore.read() is async (same
precedent as M2.3 super-properties resolver). Each call re-reads both
sources so auth-state transitions mid-batch are observable to M2.5
(M2.5 Test #3 requires this).
Empty-field omission uses conditional spread:
...(token.userEmail ? {email: token.userEmail} : {})
which OMITS the key entirely. Tests assert via chai's
.to.not.have.property('email') to distinguish absent keys from
explicit-undefined values.
9 tests cover all 6 ticket scenarios plus 2 bonus cases (empty
userName, missing GlobalConfig). Auth-transition tests use a mutable
authReader wrapper to flip identity between resolve() calls,
verifying per-call freshness with no caching.
Composes M2.2 (queue), M2.3 (super-props), M2.4 (identity) into the daemon-scoped AnalyticsClient. GlobalConfigHandler now caches the analytics flag synchronously so AnalyticsClient.isEnabled stays sync; refreshCache() is awaited in setupFeatureHandlers before construction so the very first track() (daemon_start) sees the correct flag. Adds an ESLint no-restricted-imports rule preventing IAnalyticsClient from being imported outside src/server/infra/; non-daemon consumers should reach analytics through the analytics:track transport event (M2.6).
- refreshCache(): catch block now explicitly sets cachedAnalytics=false on read failure. Under the prior fix the field defaulted to undefined, so a silent catch left getCachedAnalytics() throwing — which would crash the daemon on bootstrap if any IGlobalConfigStore implementation ever rejected from read(). Production FileGlobalConfigStore catches its own errors, but defense-in-depth restores the comment's promise. - GlobalConfigHandler class JSDoc: corrected stale "populated at construction" wording. The eager constructor-refresh was removed during the prior fix-pass; consumers now MUST await refreshCache(). - AnalyticsClient class JSDoc: tightened the no-crash invariant. The guarantee covers async resolver/queue errors only — a sync isEnabled() throw (e.g. getCachedAnalytics before refreshCache) propagates by design to surface bootstrap-misconfiguration loudly. - New integration test locks the refreshCache fail-safe: a stub IGlobalConfigStore whose read() rejects must leave the cache at false (NOT undefined), and getCachedAnalytics() must NOT throw afterwards.
Implements M2.6 transport mechanism for analytics: - analytics:track transport event with wire-level Zod validation (src/shared/transport/events/analytics-events.ts, AnalyticsTrackPayloadSchema in core/domain/transport/schemas.ts) - Daemon-side AnalyticsHandler routes valid payloads to AnalyticsClient.track (src/server/infra/transport/handlers/ analytics-handler.ts, wired in feature-handlers.ts) - Shared emitAnalytics helper at src/shared/analytics/emit.ts for in-process consumers (TUI, oclif, MCP, webui) - Round-trip integration test exercises stub-emit -> handler -> queue with full identity + super-properties stamping Deviation from ticket spec (see ENG-2628 Linear comment): - Oclif sample consumer (cli_invocation from lifecycle hook) NOT shipped -- team decided against oclif lifecycle hooks. Follow-up ticket needed. Tests added: 21 (schema 9, handler 5, helper 4, integration 3).
Schema-only milestone. No emitter wiring, no daemon-side validation. Each shipped analytics event now has a TypeScript type plus a Zod .strict() runtime schema, and typed enums eliminate magic strings. Events covered (7): - daemon_start (M2.5): empty schema; super-properties cover cold-start - cli_invocation (M2.6): command_id, flag_names, is_tty, is_ci, terminal_program?, runtime, package_manager - mcp_session_start (M2.6): client_name - mcp_tool_called (M2.6): tool_name, client_name, success, duration_ms - task_created: task_type, task_id, has_files, has_folder - task_completed: task_type, task_id, duration_ms - task_failed: task_type, task_id, duration_ms Typed enums: - AnalyticsEventNames (7 entries): wire-format event names - TaskTypes (5 entries): mirrors daemon TaskInfo.type union - TASK_TYPE_VALUES tuple: single source of truth used by every task_* schema's z.enum(...) call Privacy fixture walks ALL_EVENT_SCHEMAS and rejects any field name on the forbidden PII list (cwd, argv, path, email, result, ...). Adding a banned field name later fails the build. Wire-shape source of truth: AnalyticsTrackPayloadSchema lives in shared/transport/events/analytics-events.ts (alongside the AnalyticsEvents constants). Both the daemon handler and the shared emitAnalytics helper consume the same Zod-derived AnalyticsTrackPayload type, so the wire shape can no longer drift between two declarations. The previous server-side duplicate has been removed. Drift detection: a new compile-time + runtime test asserts the shared TaskTypes enum stays in lock-step with the server-side TaskTypeSchema. If a daemon contributor adds a new TaskInfo.type value and forgets to mirror it, the build fails loudly instead of emitting events that silently fail wire-side validation. Magic-string cleanup: the only existing analytics emit site at brv-server.ts now passes AnalyticsEventNames.DAEMON_START instead of the raw string literal. Deliberate deviations from ticket text: 1. command_id is z.string() not a typed enum: oclif manifest is the source of truth (~80 commands), a hardcoded mirror would rot. 2. Emitter wiring (oclif init, MCP oninitialized, MCP wrapper) and TaskAnalyticsHook are deferred to a follow-up ticket; this milestone ships schema declarations only. 3. task_failed has no error_class/error_code: that would require a breaking ITaskLifecycleHook.onTaskError signature change which belongs in its own ticket. 29 files changed, 0 files modified outside this milestone's scope. Tests: 7500 passing (+74 new under shared/analytics/ + drift + payload schema). Lint, typecheck, build all green.
… depth) Four review-driven hardening fixes on top of the M2.8 catalog. Zero behavior changes for existing wire/runtime callers; all changes are type-tightening or test-quality improvements. NF2 — Typed `emitAnalytics`. The shared emit boundary now takes a generic `<E extends AnalyticsEventName>` and derives the properties type from the M2.8 discriminated union. Magic-string typos (`'daemon_starts'`) and wrong-shape payloads (`tool_name` on `cli_invocation`) become compile errors instead of runtime drops. The properties argument is optional only for events whose schema has no required keys (e.g. `daemon_start`); other events require a fully-shaped payload. NF5 — Replace batch.ts hand-rolled type guards with Zod. The three `as Record<string, unknown>` casts (which violated CLAUDE.md "avoid `as Type` assertions") are gone, replaced by IdentityWireSchema / AnalyticsEventWithIdentityWireSchema / AnalyticsBatchJsonSchema. All 19 existing fromJson rejection tests still pass — Zod preserves the same edge cases (empty/whitespace device_id, missing schema_version, non-object properties, etc.) at ~half the line count. I5 — Privacy fixture walker recurses into nested ZodObject, ZodArray elements, and ZodOptional / ZodNullable wrappers. Three new regression tests verify the walker catches forbidden names (`email`, `password`, `token`, `api_key`) embedded inside nested, arrayed, and optional/nullable shapes. Today's schemas are flat, but the fixture now stays correct as future schemas grow. I6 — Extend FORBIDDEN_FIELD_NAMES from 20 to 41 entries: adds secrets (`access_token`, `auth_token`, `api_key`, `cookie`, `credential`, `password`, `secret`, `session_id`, `session_token`, `token`, `auth_header`), additional PII (`address`, `display_name`, `first_name`, `last_name`, `phone`, `phone_number`, `ssn`, `username`). The list now matches a realistic threat model for the analytics pipeline. NF7 — Already covered by existing test #6 in test/commands/analytics/enable.test.ts (retracted; my round-4 review hallucinated the gap because I read only the first 80 lines of the file). Tests: 7503 passing (+3 walker tests). Lint, typecheck, build all green. No production wiring changed; the only existing emit site (brv-server.ts) still calls `analyticsClient.track(...)` directly and is unaffected.
- New stored-record.ts: StoredAnalyticsRecord extends AnalyticsEventWithIdentity
with {id, status, attempts}; Readonly via z.infer wrapper for consistency
with rest of analytics domain (Identity, AnalyticsEvent, batch types)
- Zod schema for read-from-disk validation; default strip mode (matches
batch.ts precedent, forward-compat with future field additions)
- Export MAX_ATTEMPTS=3 constant for M9.2's updateStatus retry-cap policy
- toWireEvent helper strips local-only fields (id, status, attempts) for
M4 backend ship path
- batch.ts and existing M2 wire types unchanged (boundary preserved)
- 23 unit tests: 1 const + 16 schema + 6 helper; full suite 7526 passing
- New IJsonlAnalyticsStore interface: append/updateStatus/list/loadPending
plus droppedSentCount/droppedFullCount counters for M4.6 observability
- New JsonlAnalyticsStore class: file-backed JSONL at
{baseDir}/analytics-queue.jsonl
- Retry-cap policy lives inside updateStatus(_, 'failed'): increment attempts;
stay 'pending' while attempts < MAX_ATTEMPTS; flip terminal 'failed' at cap;
no overshoot on terminal rows
- Write serialization via private writeChain Promise mutex; eliminates
appendFile vs readFile/rename interleave race for concurrent track + flush
- Atomic rewrite: tmp file + rename (mirrors FileQueryLogStore.writeAtomic)
- Append uses FileHandle.sync() for fsync durability so row survives daemon kill
- File-size cap (default 5000 rows / 10MB): drop oldest sent first; pending and
failed never dropped; if cap full of pending+failed, append silent no-op
plus droppedFullCount++
- list sort: (timestamp DESC, id DESC) for stable same-timestamp ordering
- Read methods (list, loadPending) skip the write chain; consistent snapshot
via atomic rename
- Corruption tolerance: skip JSON.parse-failures and Zod-schema-failures
- 30 unit tests using tmpdir(); full suite 7554 passing
…sync (M9.3)
- Widen IAnalyticsQueue + BoundedQueue from AnalyticsEventWithIdentity to
StoredAnalyticsRecord; id propagates through the queue
- AnalyticsClientDeps adds jsonlStore: IJsonlAnalyticsStore
- trackAsync: resolve identity + super-props, generate randomUUID id,
compose StoredAnalyticsRecord{status:'pending', attempts:0},
await jsonlStore.append() FIRST, then queue.push() on success
- JSONL append failure: queue NOT pushed, silent drop preserves
"JSONL is source of truth" invariant; "MUST NOT crash consumer" honored
- feature-handlers.ts constructs ONE JsonlAnalyticsStore({baseDir:
getGlobalDataDir()}) and injects into AnalyticsClient. Same instance
will be shared with M11.2's analytics-list-handler
- 6 new unit tests covering dual-write happy path, JSONL-fail fallback,
uuid distinctness, queue.size() = JSONL row count, disabled no-op
- All existing M2 AnalyticsClient + BoundedQueue tests pass with widened
type
- Integration tests (daemon-tracking, transport) updated with real
JsonlAnalyticsStore({baseDir: testDir}) per tmpdir convention
- Full suite 7562 passing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…nabled to analytics.share (#746) The key name now reflects what it controls: local analytics tracking is always on (regardless of auth or this setting); the flag only gates whether collected events are shared with the remote telemetry backend. - Rename the user-facing key value analytics.enabled -> analytics.share across settings set/get/list, the onboarding opt-in script, and the disclosure command reference. Hard rename (no alias) — the feature is unreleased. - Behavior unchanged: tracking stays always-on locally; the flag still gates only remote sharing. - Internals kept (value-only rename): GlobalConfig.analytics field, the globalConfig:setAnalytics transport event, in-process method names, and the key-constant identifier names.
|
Claude finished @bao-byterover's task in 3m 29s —— View job Claude PR Review — Analytics System (M1 → M17) — Sixteenth passOverall score: 3 / 5 — Same engineering substance carrying forward (durable JSONL queue, careful failure-mode taxonomy, exceptional test coverage). The new ENG-3035 commit ( Todos
What's new since the fifteenth review (1 commit)
New findings on this pass
Prior findings — status
What's good in this push (ENG-3035)
Items outside the inline scope (carried forward)
VerdictOne ship-blocker, unchanged for the sixteenth review in a row. Replace lorem-ipsum disclosure copy in both |
device_id is now seeded at daemon bootstrap and before each remote flush, independent of the analytics flag. Previously it was created only when analytics was enabled, so fresh or never-authed users recorded events with an empty device_id that the wire schema and sender dropped, and telemetry never shipped anonymously. analytics:track dispatch now derives from a single source of truth (ALL_EVENT_SCHEMAS): AnyAnalyticsEvent is derived from it, the handler validates against it and forwards through a segregated IWireEventTracker, replacing the per-event switch. Also fixes silently dropped swarm_* events. No as-casts.
The status text read 'Analytics: enabled/disabled', but local analytics tracking is unconditional (AnalyticsClient.track always records to JSONL); only remote sharing (analytics.share) toggles. Relabelled to 'Analytics sharing: enabled/disabled' so it no longer implies tracking stopped. Read-only settings rows (e.g. analytics.status) are now expandable in the TUI: Enter opens a detail panel showing the full multi-line value (list rows only render the headline), Esc returns to the list. Adds displayDetail to the SettingsRow view-model, carrying the full block.
isAnalyticsEventName used `value in ALL_EVENT_SCHEMAS`, which matches inherited Object.prototype keys ('toString', 'constructor', '__proto__', ...). Such an event passed the guard, then ALL_EVENT_SCHEMAS[event].safeParse resolved to an inherited member (not a Zod schema) and threw at the wire-side dispatch, escaping the handler. Switch to Object.hasOwn (own-property only) so prototype-key events drop like any other unknown event. Add a regression test that asserts they drop without throwing.
Also refresh the AnalyticsClient class docblock to match the unconditional track() contract: local recording is unconditional, the isEnabled gate lives in flush() (remote send), not track().
Add test/e2e/analytics/mcp-tool-db.e2e.ts (npm run test:e2e:mcp): drives the real 'brv mcp' over JSON-RPC and verifies mcp_session_start / mcp_tool_called / mcp_session_ended land in postgres raw_events across brv-query and brv-curate roundtrips (anonymous), plus device_id rotation on auth:logout.
|
Claude finished @bao-byterover's task in 5m 43s —— View job Claude PR Review — Analytics System (M1 → M17) — Seventeenth passOverall score: 3 / 5 — Same engineering substance carrying forward (durable JSONL queue, careful failure-mode taxonomy, exceptional test coverage). The three new commits in this push are the most productive bundle since ENG-2658: one fixes a real anonymous-shipping bug (fresh users silently shipped nothing), one closes the long-standing 48-case dispatch-switch finding from the very first review (E), one closes a security-relevant prototype-pollution guard bypass, and a UX-quality push furthers the trust-separation framing via the "Analytics sharing" label change. Score held at 3/5 because the single ship-blocker from the prior sixteen reviews — lorem-ipsum disclosure body — is still in the tree. Todos
What's new since the sixteenth review (3 substantive commits)
New findings on this pass
Prior findings — status
What's good in this push
Items outside the inline scope (carried forward)
VerdictOne ship-blocker, unchanged for the seventeenth review in a row. Replace lorem-ipsum disclosure copy in both The three new commits represent the most substantive review-response bundle since ENG-2658: Finding E (standing since the 1st review) is fully closed, the anonymous-shipping bug for fresh users is fixed at root, and a security-relevant prototype-pollution guard bypass is closed with a regression test. The disclosure-body lorem ipsum is now the only substantive item between this branch and merge. |
|
Claude finished @bao-byterover's task in 1m 37s —— View job Claude PR Review — Analytics System (M1 → M17) — Eighteenth passOverall score: 3 / 5 — Same engineering substance carrying forward (durable JSONL queue, careful failure-mode taxonomy, exceptional test coverage). No analytics-substantive changes since the seventeenth review — the only new commit on this branch ( Todos
What's new since the seventeenth reviewOnly Standing ship-blocker — unresolved, eighteenth flag
Prior findings — status
Items outside the inline scope (carried forward)
VerdictOne ship-blocker, eighteenth review. Replace lorem-ipsum disclosure copy in both The engineering substance of this branch is excellent. The merge readiness is one PM/legal copy round away. |
| Lorem ipsum dolor sit amet, consectetur adipiscing elit. Event names and | ||
| super properties (`device_id`, `cli_version`, `os`, `node_version`, | ||
| `environment`) are recorded. No content of your queries, files, or | ||
| memory is collected. |
There was a problem hiding this comment.
issue (ship-blocking — eighteenth flag): Unchanged for the eighteenth review in a row. Every section body still opens with literal Lorem ipsum. The only commit since pass 17 (5dc2457) is a merge from main that brought in unrelated curate-protocol / html-renderer work — no new analytics surface changes, no movement on the disclosure body.
Four formal consent surfaces (oclif text-mode brv settings set analytics.share true, oclif text+--yes, TUI confirm-disclosure overlay, agent-driven --yes from the onboarding skill at onboarding.md:296) plus the WebUI Privacy panel default-render this lorem ipsum text (src/webui/features/analytics/constants.ts:13-39).
This remains the only substantive ship-blocker on the PR. The fix is two files — no external dependency, no submodule bump, no agent-team handoff. With the analytics.share rename now in place, the descriptor honesty at settings.ts:200 ("Local tracking is always on"), the "Analytics sharing" status label, and the device_id rotation/seeding fixes from 219da05 + b2274ea, the PM/legal round has unusually clean foundations to lean on.
…eal text; drop brittle content-assertion test.
|
Claude finished @leehpham's task in 3m 41s —— View job Claude PR Review — Analytics System (M1 → M17) — Nineteenth passOverall score: 4 / 5 — The headline ship-blocker from the prior eighteen reviews is finally closed in the canonical disclosure markdown (
Todos
What's new since the eighteenth review (1 substantive commit)
New findings on this pass
Prior findings — status
What's good in this push
VerdictHalf-closed ship-blocker. The canonical disclosure markdown round is the work this branch has been waiting nineteen reviews for, and it landed well. To close the remaining half before merge: (1) replace the lorem-ipsum bodies in The enumeration gaps ( |
| export const ANALYTICS_DISCLOSURE_SECTIONS: readonly AnalyticsDisclosureSection[] = [ | ||
| { | ||
| body: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod tempor incididunt ut labore.', | ||
| icon: Database, | ||
| label: 'WHAT IS COLLECTED', | ||
| }, | ||
| { | ||
| body: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod tempor incididunt ut labore.', | ||
| icon: Eye, | ||
| label: 'WHICH SURFACES ARE TRACKED', | ||
| }, | ||
| { | ||
| body: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod tempor incididunt ut labore.', | ||
| icon: Server, | ||
| label: 'WHERE IT GOES', | ||
| }, | ||
| { | ||
| body: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod tempor incididunt ut labore.', | ||
| icon: Link2, | ||
| label: 'CROSS-DEVICE ALIAS', | ||
| }, | ||
| { | ||
| body: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod tempor incididunt ut labore.', | ||
| icon: PowerOff, | ||
| label: 'HOW TO DISABLE', | ||
| }, | ||
| ] as const |
There was a problem hiding this comment.
issue (ship-blocking — Finding A, WebUI half still open): 125b5c4 replaced the canonical disclosure markdown (src/shared/assets/analytics-disclosure.md) with real PM/legal-approved copy — good. But this file, which is the other source of disclosure body content, still ships literal Lorem ipsum dolor sit amet... for every one of its five section bodies.
The AnalyticsPanel (analytics-panel.tsx:21) defaults detailsOpen to true (from 5d96619, flagged in the 13th review). So every user landing on the WebUI Privacy tab sees these lorem-ipsum bodies by default — even though oclif + TUI now render the real text via loadAnalyticsDisclosureText(). Two of three consumer paths (oclif collectConsent, TUI confirm-disclosure) get real copy; the third (WebUI default-open Privacy panel) still gets lorem ipsum.
The fix should mirror the canonical markdown's five sections (What is collected / Which surfaces are tracked / Where it goes / Cross-device alias / How to disable) with the same content. Better still: a vite-side markdown loader that single-sources src/shared/assets/analytics-disclosure.md and renders it into the AnalyticsDisclosureSection[] shape — closes the drift hazard architecturally so the next PM/legal copy edit (and there will be one — see the enumeration gap below) touches one file, not two.
When updating, also cover the gaps the canonical markdown still has vs. ALL_EVENT_SCHEMAS:
space_id/team_idare now stamped oncurate_run_completedandquery_completed(ENG-3020, pass 10) but neither the WebUI bodies nor the canonical markdown's "What is collected" section enumerates them.device_idrotation behavior (ENG-3019, pass 12) — rotates on logout, account-switch login, and refresh-failure sign-out — is a privacy-positive feature that softens the "permanently linked" claim in "Cross-device alias" but is currently missing from both surfaces.
| - `cli_version` — the installed CLI version | ||
| - `os` — your operating system platform | ||
| - `node_version` — the Node.js runtime version | ||
| - `environment` — whether the build is development or production |
There was a problem hiding this comment.
praise + content gap (non-blocking but bundle-friendly): Excellent close on the longest-standing finding in this PR's review history. The new copy carries the trust-separation framing all the way through ("the CLI keeps a local record of activity regardless of this setting; enabling analytics only controls whether anonymous events are shared with ByteRover" at lines 20-22), which precisely matches the descriptor honesty at settings.ts:200 and the analytics.share rename — three surfaces now agree linguistically about what the toggle does.
Two specific enumeration gaps worth folding into a follow-up copy round (non-blocking; they're additive):
-
space_idandteam_id. These were added by ENG-3020 (pass 10 review) and now stamp oncurate_run_completedandquery_completedpayloads. Backend-resolvable identifiers materially expand what an authenticated user is sharing vs. the anonymous identifier list above; they deserve enumeration alongsidedevice_id/cli_version/os/node_version/environment. -
device_idrotation behavior under "Cross-device alias". ENG-3019 (pass 12) rotates the device_id on three identity transitions: explicitbrv logout, account-switch login, and refresh-failure sign-out. That's a privacy-positive feature that softens the "permanently linked" claim at line 39-40 — a future anonymous session after logout does NOT inherit the prior device's history. Naming it both honors the engineering work and gives the user a clearer mental model of the alias semantics.
A one-paragraph addition to "Cross-device alias" along the lines of "When you log out, the device's anonymous identifier is retired and a fresh one is generated for the next session — so prior anonymous activity stays linked to the account you used, and the next anonymous run starts fresh" would close the gap cleanly.
| label: string | ||
| } | ||
|
|
||
| export const ANALYTICS_PRIVACY_URL = 'https://docs.byterover.dev/privacy' |
There was a problem hiding this comment.
issue (Finding U from pass 13, still unresolved): Two different canonical privacy URLs now ship:
| Source | URL |
|---|---|
src/shared/constants/privacy.ts:6 |
https://byterover.dev/privacy |
src/shared/assets/analytics-disclosure.md:56 (new copy as of 125b5c4) |
https://byterover.dev/privacy |
| This file | https://docs.byterover.dev/privacy |
analytics-panel.tsx:111 (display text) |
docs.byterover.dev/privacy |
The disclosure-markdown round (great work landing the real copy) explicitly committed to byterover.dev/privacy as the canonical destination. The WebUI Privacy panel still points users to a different host — if docs.byterover.dev/privacy resolves to documentation rather than the canonical legal privacy policy, the consent surfaces are pointing at different pages. Even if both resolve identically today, the divergence is invisible to tests and silently grows the next time either page is renamed.
Cleanest fix: import the canonical constant rather than re-declaring it locally:
| export const ANALYTICS_PRIVACY_URL = 'https://docs.byterover.dev/privacy' | |
| import {PRIVACY_POLICY_URL as ANALYTICS_PRIVACY_URL} from '../../../shared/constants/privacy' |
(Display string at analytics-panel.tsx:111 needs the same alignment — byterover.dev/privacy, not docs.byterover.dev/privacy.)
Also worth catching while the disclosure file is open: shared/constants/privacy.ts:1-5's docblock still annotates the URL as Placeholder until M1.5. Since the M1 disclosure copy has now shipped and points at this URL, the comment is stale — either the URL is canonical (drop the placeholder note) or it's still pending (block on the doc-page landing). Today's text is contradictory between the new disclosure markdown (treats it as canonical) and the constants file (treats it as placeholder).
| @@ -0,0 +1,56 @@ | |||
| # ByteRover CLI Analytics Disclosure | |||
There was a problem hiding this comment.
question (test coverage, non-blocking): 125b5c4 deleted test/unit/shared/assets/analytics-disclosure-content.test.ts with the commit-message rationale "drop brittle content-assertion test." Reading the deleted test (only 36 lines), it was actually quite minimal — six regex matches against section headers, one include check for PRIVACY_POLICY_URL, and one negation that the deleted brv analytics disable command form never reappears.
I don't think it was brittle in the high-maintenance sense — it's a contract test that says "the disclosure has these five sections plus the privacy link." That contract is what PM/legal sign off on at landing time; if a future copy edit accidentally drops "Cross-device alias" or rewords the disable command to a deprecated form, this test would have caught it loudly.
With both this test removed AND the WebUI lorem-ipsum still present (separate finding), the disclosure copy now lives in two unguarded sources of truth. The next copy edit could silently drift them apart without any test failure. Two cheap paths back:
-
Restore the section-header contract — keep just the regex assertions for section names +
PRIVACY_POLICY_URL+ the disable command form. The test is 30 lines; the maintenance burden is "update if PM intentionally renames a section." -
Cross-source consistency test — assert that every section label in
ANALYTICS_DISCLOSURE_SECTIONS(WebUI) matches a section header inanalytics-disclosure.md. Would have prevented the lorem-ipsum-in-WebUI gap from shipping unnoticed.
Either gives back the audit guarantee the deleted test provided. Non-blocking but worth weighing — content-assertion tests on legal-premise copy are usually exactly the kind of low-churn / high-value test that pays compound interest.
5 participants
Summary
lifecycle, CLI invocations, MCP sessions, curate/query funnels,
HITL review, settings, version-control, and onboarding signals were
all invisible to product / on-call.
for the daemon, we cannot prioritise the next round of polish,
catch silent regressions in agent flows, or measure feature
adoption per release.
M15.8. New
brv analytics enable | disable | statuscommands, aWebUI Configuration > Analytics panel, durable JSONL queue, batched
HTTP sender with exponential backoff and identity-transition
flushes, a 47-event catalogue with strict Zod schemas, a privacy
fixture that statically rejects forbidden field names, daemon-side
lifecycle hooks for tasks / curate / query, and producer-side
wiring for daemon_start / cli_invocation / mcp_session_start /
mcp_session_ended / mcp_tool_called.
under
src/agent/(layering rule preserved). No producer foronboarding_auto_setup_started/onboarding_completedyet,pending an agent-team signal (spec at
agent-tmp/m15-8-onboarding-signal-spec.md). Nospace_switchedproducer (waits on the snapshot-removal project). The cli_metadata
outgoing helper currently covers only 6 of around 80 oclif
commands; the daemon middleware emits for all that participate.
Type of change
Scope (select all touched areas)
Also touched: WebUI Configuration > Analytics panel + analytics API
(not in the template list).
Linked issues
analytics-system project. Representative anchors: ENG-2611 (analytics
GlobalConfig field), ENG-2628 (analytics:track transport event),
ENG-2686 (per-event schema catalogue), ENG-2649 (e2e harness),
ENG-2772 (lifecycle-hook frontmatter harvest), ENG-2807 (M13.3
cli_metadata sweep batches 1+2), ENG-2961 (M15.1 foundation),
ENG-2967 (M15.4 HITL + settings + reset), ENG-2963 (M15.5 WebUI
session lifecycle), ENG-2964 (M15.6 tool-mode analytics).
onboarding_auto_setup_started/onboarding_completedsignal, spec atagent-tmp/m15-8-onboarding-signal-spec.md. Not filed yet.Root cause (bug fixes only, otherwise write
N/A)Test plan
dev-beta; manual smoke validates real-backend send + sent
status in the JSONL queue)
test/unit/server/infra/analytics/,test/unit/shared/analytics/,test/unit/infra/process/,test/unit/infra/client/,test/integration/infra/process/analytics-hook-lifecycle-wiring.test.ts,test/e2e/analytics/dev-beta.e2e.ts, plus webui +oclif-command tests.
bounded-queue drop-oldest semantics, HTTP sender 2xx + 5xx + 400
auth-transition force-flush, batched flush (30s tick + 20-event
threshold + shutdown drain), wire-side per-event Zod validation,
privacy fixture (no forbidden field name across any event),
daemon lifecycle hook drains in-flight per-op processing
before terminal emit, WebUI session start + end with reconnect
orphan-end, MCP session start + end with handshake-timing race,
mcp_tool_called for success + error + cancellation,
cli_invocation middleware fires on cli_metadata-bearing requests
and bails on malformed metadata, idempotent middleware attach.
User-visible changes
brv analytics enable,brv analytics disable,brv analytics status(with--format jsonfor scripting).enable / disable toggle with first-run disclosure dialogue,
status line surfacing queue depth + last successful flush +
backoff state.
is collected unless the user accepts.
brv loginand 5 other oclif commands now sendcli_metadatasothe daemon can emit
cli_invocationper command. No promptdifference to the user.
when analytics is disabled.
Evidence
M15.5 added 75 ClientManager session tests; M15.6 added the
lifecycle-wiring integration test; M15.8 added 31 new cases
(client-manager-mcp-analytics + analytics-hook-mcp-tool-called +
task-router-client-identity-snapshot + cli-invocation-middleware).
npm teston the merge head:9726 passing, 16 pending, 0 failing(run during M15.8 verification).git grep "from.*shared/analytics/emit" src/oclif src/agent src/server/infra/mcpreturns empty (daemon-only instrumentationrule holds).
ALL_EVENT_SCHEMASrejects every name on
FORBIDDEN_FIELD_NAMES.Checklist
npm test): 9726 / 16pending / 0 failing as of the M15.8 verification step.
npm run lint): 0 errors, 279 warnings(warnings are pre-existing repo noise, none introduced).
npm run typecheck): root TS check passes;the WebUI sub-project check (
tsc -p src/webui/tsconfig.json)reports 6 errors in 4 files about
TopicViewerPropsmissingbreadcrumb/index/related. These pre-date this branch(confirmed by
git stashrerun on base) and originate from aseparate adopt-callback-based-topic-viewer commit waiting on a
@campfirein/byterover-packagesbump. Flagged under Risksbelow; not a regression introduced by this PR.
npm run build): not re-verified in this PRsession because
build:uiis gated on the same WebUItypecheck above. Verify after the topic-viewer package bump
lands.
branch uses
feat: [ENG-XXXX] ...orfix: [ENG-XXXX] ....sync as milestones landed (analytics commands, env vars,
e2e instructions, settings keys, billing-line). Spot-check
before merging.
default-off feature. No public command shape changed. The
IAnalyticsClientinterface is daemon-internal.main: last merge from main wascommit
0ef90884f Merge branch 'main' into proj/analytics-system-tool-mode. Verify zero diff vs.origin/mainbefore pressing merge.Risks and mitigations
Risk: WebUI typecheck errors on
TopicViewerPropsblock the huskypre-push hook and any CI step that runs the full
npm run typecheck. Six errors acrosscontext-detail-panel.tsx,root-index-detail.tsx,curate-tool-mode-sections.tsx,task-detail-sections.tsx.@campfirein/byterover-packagesis bumped to the version thatexports the callback-based TopicViewer API (or build with
BRV_UI_SOURCE=submoduleaftergit submodule update --init --recursive). Coordinate with the shared-UI submodule ownerbefore merging.
Risk: large surface area (253 files, around 26k LoC, 108 commits)
raises review burden.
the project branch (each independently reviewed and merged).
This PR is the integration-to-main step. Skim the commit list
rather than the line-by-line diff.
Risk: analytics enabled in error could ship PII.
FORBIDDEN_FIELD_NAMESredactor with a fixture that fails CI if any event schema
declares a name on the list, file paths are stripped to
project-relative or sentinel'd, and the disclosure UX makes the
opt-in explicit in both CLI and WebUI.
Risk: backend outage produces unbounded queue growth.
cap (
JsonlCapFullError), exponential backoff with areachability counter, and
brv analytics statussurfaces theoperational state for on-call.
Risk: agent processes acquire stale analytics state mid-task.
(
agent-settings-snapshot.ts); per-task identity snapshot onTaskInfo(clientType+clientName) survives mid-taskclient disconnect.
Risk: M15.8 onboarding events stay empty until the agent team
ships a
skill:invoked/skill:completedsignal.spec at
agent-tmp/m15-8-onboarding-signal-spec.md. Backendfunnel queries should not yet reference these events.