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}
…ompleted Bundles team_id alongside space_id (added in b94829c) so PMs can slice the curate/query funnel by team as well as space. Schemas: optional `team_id: z.string().min(1).max(64).optional()` on both CurateRunCompletedSchema and QueryCompletedSchema. Same opaque-ID shape, same independent-optional semantics — a project may have a team without a space (mid-onboarding) and either / both / neither field can be present. AnalyticsHook: replaced the `getSpaceId` accessor with `getIdentity: (projectPath) => Promise<{spaceId?, teamId?}>` so a single config read serves both stamps at terminal-emit time. The defensive `resolveIdentity` wrapper normalizes empty strings to undefined per-field and swallows producer throws — a missing / broken identity NEVER blocks a terminal emit. Daemon: wires getIdentity through `projectStateLoader.getProjectConfig`, returning {spaceId: config?.spaceId, teamId: config?.teamId}. Tests: - Schema: accepts populated team_id, both together, neither; rejects empty + over-64-char (per-field) - Hook: stamps team_id alone (no space), space alone (no team), both, neither; normalizes empty strings; survives throw; stamps on failure-path emits Validated end-to-end via real curate + query in vibing-zone (a connected project with both spaceId + teamId): curate_run_completed | space_id=019e4505-... | team_id=019b8213-... query_completed | space_id=019e4505-... | team_id=019b8213-... rows landed in telemetry postgres with both fields populated.
Three findings from the Claude bot review. #1 — Move `ProjectIdentity` + its docblock OUT of the gap between the `AnalyticsHook` class docblock and the `class AnalyticsHook` declaration. The previous layout left the class docblock orphaned — attaching visually to `type ProjectIdentity` instead of the class. Result: doc generators and IDE hover now resolve the class docblock correctly. #3 — Short-circuit `resolveIdentity` on `!this.isEnabled()`. Mirrors the existing `readFrontmatterFields` precedent (lines 699-700). When analytics is off the daemon no longer touches `projectStateLoader` on every task termination just to discard the result. Added a unit test that verifies getIdentity is NOT invoked when isEnabled() returns false — locks the short-circuit so a future refactor can't quietly re-introduce the wasted I/O. #2 — Acknowledged the staleness window in the `getIdentity` docblock. `projectStateLoader` only invalidates on the GET_PROJECT_CONFIG transport event (agent-process startup); mid-session config rewrites (`brv login`, `brv space switch`) keep returning the last-known-good identity until the next invalidation. Funnel analytics accepts last-known-good as the contract — a doc-line is enough. Anyone who later wants this accessor for billing or audit attribution is now explicitly warned to route through `shouldInvalidate`. #4 was praise on the test matrix — no code change. Full unit suite still green.
Feat/eng 3020
|
Claude finished @cuongdo-byterover's task in 3m 48s —— View job Claude PR Review — Analytics System (M1 → M17) — Tenth passOverall score: 3 / 5 — Same engineering substance carrying forward (durable JSONL queue, careful failure-mode taxonomy, exceptional test coverage). The new ENG-3020 work bundles Todos
What's new since the last review (6 commits — all ENG-3020)
New findings on this pass
Prior findings — status
What's good on this push
Items outside the inline scope (carried forward)
VerdictOne ship-blocker, unchanged for the tenth review in a row. Replace |
* feat: [ENG-3004] expose analytics opt-in flag as brv settings analytics.enabled
Surfaces the analytics opt-in via a new `analytics.enabled` boolean
descriptor. SettingsHandler acts as a facade: GET/SET/RESET/LIST for the
key route through GlobalConfigHandler instead of FileSettingsStore, so
the canonical storage in config.json, the device-id seeding race fix,
the analytics cache, and the abort-on-disable side-effect all stay
unchanged. The descriptor carries a new `storage: 'global-config'` flag;
the file store refuses to persist such keys and the validator pushes any
hand-edited override into the invalid bucket.
The disclosure prompt moves into `brv settings set analytics.enabled true`
with a `--yes` flag for CI / non-interactive runs (CLIError + non-zero
exit when no TTY and no --yes). The TUI settings page gains a
confirm-disclosure mode that renders the markdown inline on the
false-to-true toggle and waits for Enter / Esc.
Disclosure markdown moved from `src/server/templates/sections/` to
`src/shared/assets/` so the TUI can read it without crossing the
`tui` -> `server` import boundary. Both surfaces consume it via the new
canonical loader at `src/shared/utils/load-analytics-disclosure.ts`.
The legacy `brv analytics enable` command delegates to the extracted
disclosure helper at `src/oclif/lib/analytics-disclosure.ts` so the
command file can be safely deleted in a follow-up.
* fix: [ENG-3004] make TUI analytics-disclosure scroll in short terminals
The disclosure overlay rendered the full markdown as a single Text block,
which let wrapped long lines push the sticky `Enter / Esc` footer off
the visible area in short terminal windows. Now the body lives in a
sized viewport: each markdown line is its own `wrap="truncate-end"` Text
so source-to-visual row mapping is 1:1, the body height is clamped to
`terminalRows - 12` (defensive for the REPL's own bottom bar), and
overflow shows as yellow `↑ more above` / `↓ more below` indicators.
↑/↓ scroll by line; PgUp/PgDn (and `b` / `Space`) scroll by page.
* chore: [ENG-3007] delete the brv analytics command tree
The `brv analytics enable | disable | status` commands are folded into
the unified `brv settings` surface (`analytics.enabled` boolean facade
and `analytics.status` readonly-info snapshot, shipped in earlier
milestone tickets). The legacy command files and their test directory
are removed; `brv analytics <anything>` now exits with oclif's
"Command not found" message.
Lib re-exports survive the deletion so the M16.2 + M16.3 work keeps
working:
- `src/oclif/lib/analytics-disclosure.ts` (already extracted earlier)
- `src/oclif/lib/analytics-status-formatter.ts` (NEW thin re-export
from the canonical home at `src/shared/utils/format-analytics-status.ts`,
so the AC's "importable from src/oclif/lib/" surface is preserved
while the TUI keeps consuming the same renderer via shared/).
Two replacement smoke tests confirm the oclif command path resolves
both analytics keys to the `settings:get` transport event:
- test/commands/settings/analytics-enabled.test.ts
- test/commands/settings/analytics-status.test.ts
Heavy lifting stays in the unit test suites that already cover the
facade routing, disclosure flow, and formatter parity.
The disclosure markdown content contract (5 required sections + privacy
URL) is rescued from the deleted enable.test.ts into
test/unit/shared/assets/analytics-disclosure-content.test.ts so a
future copy edit cannot silently break the section headers.
* fix: [ENG-3007] address PR #734 bot review
- Disclosure markdown: brv analytics disable -> brv settings set analytics.enabled false
- Tighten disclosure-content regex to require the new command form
- settings set: warn when --yes is passed for non-analytics keys
- settings set: JSON mode refuses interactive consent, returns requires_consent envelope
- analytics-disclosure: translate inquirer ExitPromptError to declined consent
- settings-page TUI: import ANALYTICS_ENABLED_KEY from shared/constants
- settings-page TUI: surface transport rejection on disclosure-confirm toggle
- settings-handler: facade-missing branches use code: 'misconfigured'
- settings-handler: explicit guard for non-boolean global-config RESET
- shared/transport: widen SettingsErrorDTO.code union with 'misconfigured'
- file-settings-store: drop trailing blank line
- rewrite historical doc-comment refs from brv analytics to brv settings
|
Claude finished @bao-byterover's task in 6m 3s —— View job Claude PR Review — Analytics System (M1 → M17) — Eleventh passOverall score: 3 / 5 — Same engineering substance carrying forward (durable JSONL queue, careful failure-mode taxonomy, exceptional test coverage). The new ENG-3004 + ENG-3007 work ( Todos
What's new since the last review (1 substantive multi-commit squash)
New findings on this pass
Prior findings — status
What's good on this push
Items outside the inline scope (carried forward)
VerdictOne ship-blocker, eleventh review in a row. Replace Every other finding is recommended-but-not-blocking. The ENG-3004 + ENG-3007 consolidation itself is well-engineered: cleaner surface, cleaner code paths, defensive guards in the right places, and a contract test that future copy edits can't silently break. |
* feat: [ENG-3019] rotate device_id on logout, account-switch, and refresh-failure
Rotate the global device_id after user-initiated identity transitions so
the machine's analytics identity does not survive a session boundary.
Mirrors PostHog/Mixpanel/Amplitude/Segment reset() semantics.
Triggers:
- Explicit brv logout when previously authenticated
- brv login account switch (userA to userB with no logout in between)
- Refresh-token exchange failure (treated as full sign-out: clear token,
emit auth_logout {failure_kind:'refresh_failed'}, broadcast
STATE_CHANGED, rotate)
Does NOT trigger on:
- Logout while already anonymous (or with an expired stored token)
- Polling-observed token expiry without an attempted refresh
- Same-user re-login
Rotation runs AFTER the auth_login / auth_logout emit so the transition
event still carries the OLD device_id, preserving attribution to the
departing user's history. identity-resolver re-reads per call, so the
next track() picks up the new id automatically.
New IGlobalConfigRotator interface; GlobalConfigHandler implements it.
rotateDeviceId() chains through the same writeChain as setAnalytics to
serialize against concurrent flag toggles. AuthHandler depends on the
narrow interface (optional, mirroring the existing optional
analyticsClient pattern).
* fix: [ENG-3019] address PR #740 bot review
Narrow the setupRefresh sign-out catch to refreshToken() ONLY. A failure
there means the auth server rejected the refresh (definitive sign-out).
Failures AFTER refresh succeeded (user fetch 5xx, save disk error) are
post-refresh APPLICATION failures and must not burn a device_id rotation
or be attributed as refresh_failed in the analytics funnel: return
{success:false} silently instead. Adds a processLog breadcrumb on each
branch so refresh sign-outs leave at least one daemon-log line.
Other PR-bot feedback addressed:
- Move IGlobalConfigRotator from core/interfaces/state/ to
core/interfaces/storage/ (rotator writes to disk via the config
store, not in-memory state).
- Loosen failure_kind assertion regex from [a-z_] to [a-z0-9_] so
future tags like oauth_v2_flow or http_429 are not rejected at the
test boundary.
- Pin UUID v4 shape on the rotation test so a regression that swaps
randomUUID for a non-UUID source fails loudly.
- Document the safeLoadToken trade-off (returning undefined on a
transient token-store error silently skips one rotation; the
alternative would discard a freshly-exchanged OAuth token).
|
Claude finished @bao-byterover's task in 5m 49s —— View job Claude PR Review — Analytics System (M1 → M17) — Twelfth passOverall score: 3 / 5 — Same engineering substance carrying forward (durable JSONL queue, careful failure-mode taxonomy, exceptional test coverage). The new ENG-3019 work ( Todos
What's new since the eleventh review (1 substantive commit)
New findings on this pass
Prior findings — status
What's good on this push
Items outside the inline scope (carried forward)
VerdictOne ship-blocker, unchanged for the twelfth review in a row. Replace |
…acy tab (#742) Default detailsOpen to true so the "What data will be collected?" panel is expanded on first render. Helps the user see the disclosure without having to click to reveal it, which matches the spirit of an opt-in privacy surface.
|
Claude finished @bao-byterover's task in 2m 56s —— View job Claude PR Review — Analytics System (M1 → M17) — Thirteenth passOverall score: 3 / 5 — Same engineering substance carrying forward (durable JSONL queue, careful failure-mode taxonomy, exceptional test coverage). The new ENG-2621 change ( Todos
What's new since the twelfth review (1 commit)
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 thirteenth review in a row, with growing exposure. Replace lorem-ipsum disclosure copy in both Every other finding is recommended-but-not-blocking. The ENG-2621 work itself is a correct one-line UX fix — its only side effect is to make the still-unresolved disclosure-body blocker more visible, which arguably accelerates rather than impedes the path to merge. |
| const {data, error, isError, isLoading, refetch} = useGetGlobalConfig() | ||
| const setAnalytics = useSetAnalytics() | ||
| const [pendingIntent, setPendingIntent] = useState<'disable' | 'enable' | undefined>() | ||
| const [detailsOpen, setDetailsOpen] = useState(true) |
There was a problem hiding this comment.
issue (escalates the ship-blocker — thirteenth flag): Defaulting detailsOpen to true is the right UX call for an opt-in privacy surface — the whole point of the Privacy tab is to make data-handling transparent rather than tucked behind a click. But this lands the lorem-ipsum disclosure as the default landing state of the WebUI Privacy tab, rather than something the user has to click "Show details" to reach.
The disclosure-body content for the WebUI lives in src/webui/features/analytics/constants.ts:13-39 and every section's body field is literal lorem ipsum:
body: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Sed do eiusmod tempor incididunt ut labore.',…repeated for WHAT IS COLLECTED, WHICH SURFACES ARE TRACKED, WHERE IT GOES, CROSS-DEVICE ALIAS, and HOW TO DISABLE. Before this commit, a user on the Privacy tab saw a clean toggle + a "Show details" button — clicking it would reveal lorem ipsum. After this commit, every user landing on the tab sees lorem ipsum without any interaction.
This commit's spirit is right (a privacy page should not hide its disclosure), but the precondition for that spirit is real disclosure copy. The lorem-ipsum prior reviews flagged 12 times now affects two distinct surfaces that need fixing in lockstep:
src/shared/assets/analytics-disclosure.md(the canonical markdown, consumed by oclifbrv settings set analytics.enabled true+ TUIconfirm-disclosureoverlay + agent-driven--yesflow).src/webui/features/analytics/constants.ts:13-39(the structured grid in the Privacy panel, consumed byDisclosureDetails/AnalyticsPanel).
They are not sharing source today — the markdown is parsed by oclif/TUI and the constants array is rendered by React. So replacement requires PM/legal to provide the body for both formats. Keep the section labels stable (load-bearing for analytics-disclosure-content.test.ts).
| label: string | ||
| } | ||
|
|
||
| export const ANALYTICS_PRIVACY_URL = 'https://docs.byterover.dev/privacy' |
There was a problem hiding this comment.
issue (privacy URL inconsistency, non-blocking but worth catching before the lorem-ipsum replacement round): Two different canonical privacy URLs now ship in the codebase:
| Source | URL | Surface |
|---|---|---|
src/shared/constants/privacy.ts:6 |
https://byterover.dev/privacy |
(referenced by analytics-disclosure.md:36) |
src/webui/features/analytics/constants.ts:11 (this line) |
https://docs.byterover.dev/privacy |
WebUI Privacy panel link at analytics-panel.tsx:104-112 |
The WebUI panel's "docs.byterover.dev/privacy" link target diverges from the canonical disclosure-md's byterover.dev/privacy. A user reading the disclosure markdown via brv settings set analytics.enabled true reaches one URL; a user clicking the WebUI Privacy panel's link reaches a different one. If both resolve to the same legal page on the backend's DNS, it's invisible — but if they're separate routes (a docs.byterover.dev host typically serves documentation, not the canonical legal privacy policy), the consent surfaces are pointing at different pages.
When PM/legal land the disclosure copy round, the right resolution is one canonical URL (recommend hoisting from shared/constants/privacy.ts and importing here, so the two surfaces stay in lockstep). Today, fixing one and forgetting the other silently produces drift no test catches.
| export const ANALYTICS_PRIVACY_URL = 'https://docs.byterover.dev/privacy' | |
| import {PRIVACY_POLICY_URL as ANALYTICS_PRIVACY_URL} from '../../../shared/constants/privacy' |
(or — re-export under the WebUI-local name if the renaming would churn callers).
| 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 (observed during the thirteenth review — code-quality / drift-risk, non-blocking): Disclosure copy is now duplicated across two sources of truth:
src/shared/assets/analytics-disclosure.md— markdown body parsed by oclifcollectConsent(src/oclif/lib/analytics-disclosure.ts:71) and the TUI scroll viewport (src/tui/features/settings/components/settings-page.tsx:347-378).ANALYTICS_DISCLOSURE_SECTIONShere — structured object array consumed by<DisclosureDetails>in the WebUI Privacy panel.
When PM/legal land approved copy, replacement has to happen in both places, with no test pinning them in sync. The disclosure-content test at test/unit/shared/assets/analytics-disclosure-content.test.ts:25-34 checks only the markdown — a future edit that updates the markdown but forgets the WebUI constants (or vice versa) silently produces drift. The lorem-ipsum body content itself happens to be identical across all five sections here, which obscures the drift risk until the replacement happens.
Two practical resolutions, in order of preference:
- Single source. Parse
analytics-disclosure.mdat build time (or via a vite-side text loader) into the structured shape<DisclosureDetails>consumes. Section headers in the markdown become the labels; section bodies become thebodyfield. Closes the drift hazard architecturally — one edit, one test target. - Cross-source test. Extend
analytics-disclosure-content.test.tsto verify that every section's body in this array matches the corresponding section in the markdown (after some canonical normalization). Lighter but keeps two-source maintenance burden.
The icon mapping (Database, Eye, Server, Link2, PowerOff) and the WHAT-IS-COLLECTED-style upper-case labels are WebUI-specific affordances, so option (1) would need a section→icon map living next to the loader. The section body content is the part that drifts; the visual scaffolding is fine to keep here.
Non-blocking on the merge premise, but worth flagging now since the lorem-ipsum-replacement round is the one moment where the two sources are guaranteed to diverge if you forget to touch both.
4 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.