feat: [ENG-3035] rename user-facing analytics setting key analytics.e…

[bao-byterover]

No description provided.

[github-actions]

Claude finished @bao-byterover's task in 4m 8s —— View job

---

Review: feat: [ENG-3035] rename analytics.enabled -> analytics.share

• Fetch and inspect the PR diff vs origin/proj/analytics-system-tool-mode
• Check that the rename is consistently applied across source, tests, templates, and docs
• Look for any missed references to the old analytics.enabled key
• Check for migration / backwards-compat concerns
• Review related touched files for correctness
• Post inline comments + final summary

---

Summary

Score: 4.5 / 5

Clean, surgical rename. The scope is well-defined in the commit message — value-only swap on the user-facing wire key, with internals (GlobalConfig.analytics, the globalConfig:setAnalytics transport event, in-process method names, and the constant identifier names) intentionally kept. I verified the rename is exhaustive:

• grep -rn "analytics\.enabled" across src/ and test/: zero hits (other than legacy historical mentions of the deleted brv analytics enable command, which are intentional context).
• Internal call sites all go through SETTINGS_KEYS.ANALYTICS_ENABLED (which now interpolates to 'analytics.share'), so user-facing error strings in src/oclif/commands/settings/set.ts:74, 148 auto-derive the new key correctly.
• No on-disk migration risk. The descriptor declares storage: 'global-config', so the value lives in config.json under the analytics field (unchanged), not under a settings.json key — pre-existing values keep loading.
• Test invariant pinned: test/unit/core/domain/entities/settings-registry.test.ts:219 asserts SETTINGS_KEYS.ANALYTICS_ENABLED === 'analytics.share', so an accidental future rename fails loudly.
• Onboarding template, disclosure markdown, e2e tests, oclif command, TUI settings page, and all unit/integration tests are consistently updated.

Praise

• praise: "Hard rename (no alias) — the feature is unreleased" is the right call. Adding an alias for an unreleased key would have been carrying a permanent footgun for no real user benefit.
• praise: PR additions == PR deletions (87/87) — exactly what a value-only rename should look like.

Nits (non-blocking)

Three inline comments posted:

1. src/shared/constants/settings-keys.ts:13 — the constant identifier ANALYTICS_ENABLED_KEY now resolves to 'analytics.share'. Add a one-line comment to spare future readers a double-take and prevent grep misses.
2. test/commands/settings/analytics-enabled.test.ts — file name still references the old key; consider git mv to analytics-share.test.ts.
3. test/commands/settings/analytics-enabled.test.ts:71 — stub description is missing the second sentence 'Local tracking is always on.' that the production descriptor carries. Cosmetic but worth syncing while you're here.

Plus one suggestion not posted inline (the line isn't in the diff):

• src/oclif/commands/settings/set.ts — public static examples = [...] covers agentPool.maxSize and llm.iterationBudgetMs but skips the consent-gated key. Adding an example like '<%= config.bin %> settings set analytics.share true --yes' would surface the consent flow in brv settings set --help.

Correctness / security

No correctness, security, or behavioural regressions identified. The consent gate, JSON-mode requires_consent envelope, --yes scoping, abort-on-disable wiring, and global-config facade routing all remain intact. The LIST ordering in test/unit/infra/storage/file-settings-store.test.ts:57 is still alphabetically correct ('analytics.share' < 'analytics.status').
· Branch: feat/ENG-3035

[github-actions]

nitpick (non-blocking): the constant identifier still reads ANALYTICS_ENABLED_KEY but resolves to the new wire value 'analytics.share'. The commit message explains this is intentional (internals kept, value-only rename), but a one-line comment here would save the next reader from a double-take and grep confusion (e.g. greping ANALYTICS_ENABLED_KEY does not surface the user-facing key name).

Suggested change

	export const ANALYTICS_ENABLED_KEY = 'analytics.share' as const
	// Identifier is intentionally retained from M16.2; the literal value is the
	// user-facing wire key. Value renamed to `analytics.share` in ENG-3035 to
	// reflect that local tracking is always on and the flag only gates remote sharing.
	export const ANALYTICS_ENABLED_KEY = 'analytics.share' as const

[github-actions]

nitpick (non-blocking): the file is still named analytics-enabled.test.ts but every it(...) / describe(...) inside it now targets analytics.share. Consider git mv-ing this to analytics-share.test.ts so file naming matches the surface it covers — otherwise future grep -r analytics.share test/ won't surface this file by path. (Cosmetic; the tests themselves are correct.)

[github-actions]

nitpick (non-blocking): the production descriptor for this key (in src/server/core/domain/entities/settings.ts) reads 'Send anonymous telemetry to ByteRover. Local tracking is always on.'. This stub is missing the second sentence. Since the test only asserts wiring (event + payload), the stale text doesn't break anything — but a future test that does check the description would diverge silently. Worth syncing while you're touching this file.