From d6958e1120de6a6e91904aaba90c8496e8ead1b3 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:25:06 -0700 Subject: [PATCH] fix(analytics): stop counting bot User rows as humans MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HUMAN_FILTER keyed only on `botMetadata.agentName` absence, via an $or that also passed anything with no botMetadata object at all. It never checked `isBot`. Bot rows written by the gateway bridge, the summarizer and several openclaw install paths carry botMetadata WITHOUT an agentName key, so they satisfied the second clause and were counted as humans. On the dev instance that is 8 rows of 85 (clawdbot-bridge, commonly-summarizer, four openclaw-inst-*, socialpulse-adasdsad): totalUsers / DAU / WAU / signups inflated ~10%, and every funnel rate deflated, because a bot in the denominator can never convert. Corrected human count is 77. Three sites shared the flaw and are now one definition: - the /funnel route inlined a duplicate of the filter - HUMAN_FILTER itself (used by /usage signups, DAU, WAU, totalUsers) - the botIds query feeding "distinct human posters", which is the same criterion inverted and so missed the same 8 rows HUMAN_FILTER and BOT_FILTER are now exact logical complements, hoisted above first use, with a test pinning that property — if they drift, a row is either counted twice or dropped from both. Nothing else in the dashboard was wrong: messages read PostgreSQL, and the JS dayKey and the SQL UTC day-cast agree, so no days are silently dropped. The funnel fixture keyed "is this the bot query?" on mere presence of the botMetadata.agentName field. HUMAN_FILTER now names that field too, so the signup query took the bot branch and returned docs with no createdAt, throwing on new Date(undefined).toISOString(). Retargeted to the real discriminator. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01RWkf1SuYRHosxnPqsXwaVa --- .../routes/admin.analytics.funnel.test.js | 5 +- .../admin.analytics.human-filter.test.js | 127 ++++++++++++++++++ backend/routes/admin/analytics.ts | 42 ++++-- 3 files changed, 159 insertions(+), 15 deletions(-) create mode 100644 backend/__tests__/unit/routes/admin.analytics.human-filter.test.js diff --git a/backend/__tests__/unit/routes/admin.analytics.funnel.test.js b/backend/__tests__/unit/routes/admin.analytics.funnel.test.js index 447dc5e1..5b58c3c9 100644 --- a/backend/__tests__/unit/routes/admin.analytics.funnel.test.js +++ b/backend/__tests__/unit/routes/admin.analytics.funnel.test.js @@ -132,8 +132,11 @@ describe('GET /api/admin/analytics/usage', () => { const chain = (docs) => ({ select: jest.fn().mockReturnValue({ lean: jest.fn().mockResolvedValue(docs) }) }); it('merges Mongo signups with PG message/poster counts per day', async () => { + // The bot query is BOT_FILTER ($or of isBot / agentName-present); the + // human query names the same field with $exists:false, so key off $or — + // mere presence of the field no longer discriminates. mockUserFind.mockImplementation((q) => { - if (q && q['botMetadata.agentName']) return chain([{ _id: 'bot1' }]); + if (q && Array.isArray(q.$or)) return chain([{ _id: 'bot1' }]); return chain([userDoc('u1', 3), userDoc('u2', 3), userDoc('u3', 0)]); }); // dau, wau, totalUsers in Promise.all order diff --git a/backend/__tests__/unit/routes/admin.analytics.human-filter.test.js b/backend/__tests__/unit/routes/admin.analytics.human-filter.test.js new file mode 100644 index 00000000..4be22108 --- /dev/null +++ b/backend/__tests__/unit/routes/admin.analytics.human-filter.test.js @@ -0,0 +1,127 @@ +// Regression: analytics must not count bot User rows as humans. +// +// The original HUMAN_FILTER keyed only on `botMetadata.agentName` absence, +// using an $or that ALSO passed anything without a botMetadata object. Bot +// rows written by the gateway bridge / summarizer / several openclaw install +// paths carry botMetadata WITHOUT an agentName key, so they satisfied the +// second clause and were counted as humans. On the dev instance that was 8 +// rows out of 85 — totalUsers/DAU/WAU inflated ~10%, and every funnel rate +// deflated, because a bot sitting in the denominator can never convert. +// +// These run against a real in-memory Mongo so the FILTER SEMANTICS are what's +// under test, not a mock's recorded call args. + +const request = require('supertest'); +const express = require('express'); +const mongoose = require('mongoose'); + +jest.mock('../../../middleware/auth', () => (req, res, next) => { + req.user = { id: 'admin1' }; + req.userId = 'admin1'; + next(); +}); +jest.mock('../../../middleware/adminAuth', () => (req, res, next) => next()); +jest.mock('../../../middleware/ipRateLimit', () => ({ + cloudflareIpRateLimitKeyGenerator: () => 'test-key', +})); + +// No PostgreSQL in this tier — the message-sourced columns are covered by +// admin.analytics.funnel.test.js. +const mockPgQuery = jest.fn().mockResolvedValue({ rows: [] }); +jest.mock('../../../config/db-pg', () => ({ + pool: { query: (...args) => mockPgQuery(...args) }, +})); + +const User = require('../../../models/User'); +const routes = require('../../../routes/admin/analytics'); + +const DAY = 24 * 60 * 60 * 1000; +const recently = () => new Date(Date.now() - 2 * 60 * 60 * 1000); + +let mongod; +let app; + +const seed = (over) => ({ + username: over.username, + email: `${over.username}@example.test`, + password: 'x', + createdAt: new Date(Date.now() - DAY), + lastActive: recently(), + ...over, +}); + +describe('admin analytics — bot rows must never be counted as humans', () => { + beforeAll(async () => { + // eslint-disable-next-line global-require + const { MongoMemoryServer } = require('mongodb-memory-server'); + mongod = await MongoMemoryServer.create(); + await mongoose.connect(mongod.getUri()); + + await User.create([ + // The only real human. + seed({ username: 'real-human' }), + + // Classic agent row — already excluded before this fix. + seed({ + username: 'openclaw-theo', + isBot: true, + botMetadata: { agentName: 'openclaw', instanceId: 'theo' }, + }), + + // THE LEAK: botMetadata present, no agentName key. Passed the old + // $or filter as a human. Mirrors clawdbot-bridge / commonly-summarizer + // / openclaw-inst-* / socialpulse-* on the live instance. + seed({ + username: 'clawdbot-bridge', + isBot: true, + botMetadata: { displayName: 'clawdbot-bridge', capabilities: [] }, + }), + + // Same leak shape, no botMetadata at all — only isBot marks it. + seed({ username: 'bare-bot', isBot: true }), + ]); + }, 60000); + + afterAll(async () => { + await mongoose.disconnect(); + if (mongod) await mongod.stop(); + }); + + beforeEach(() => { + app = express(); + app.use('/api/admin/analytics', routes); + }); + + it('counts one human out of four rows, not three', async () => { + const res = await request(app).get('/api/admin/analytics/usage?days=30'); + expect(res.status).toBe(200); + expect(res.body.totals.totalUsers).toBe(1); + }); + + it('excludes bots from DAU and WAU even though all four are freshly active', async () => { + const res = await request(app).get('/api/admin/analytics/usage?days=30'); + expect(res.status).toBe(200); + expect(res.body.totals.dau).toBe(1); + expect(res.body.totals.wau).toBe(1); + }); + + it('excludes bots from signup cohorts, so the funnel denominator is human-only', async () => { + const res = await request(app).get('/api/admin/analytics/funnel?days=30'); + expect(res.status).toBe(200); + expect(res.body.totals.signups).toBe(1); + }); + + it('treats HUMAN_FILTER and BOT_FILTER as exact complements', async () => { + // Every row is classified exactly once. If these two ever drift apart, a + // row is either counted twice or dropped from both — and the "distinct + // human posters" metric silently double-subtracts. + const usage = await request(app).get('/api/admin/analytics/usage?days=30'); + expect(usage.status).toBe(200); + + const humans = usage.body.totals.totalUsers; + const bots = await User.countDocuments({ + $or: [{ isBot: true }, { 'botMetadata.agentName': { $exists: true } }], + }); + expect(humans + bots).toBe(await User.countDocuments({})); + }); +}); diff --git a/backend/routes/admin/analytics.ts b/backend/routes/admin/analytics.ts index 8cf41518..79ad449b 100644 --- a/backend/routes/admin/analytics.ts +++ b/backend/routes/admin/analytics.ts @@ -1,7 +1,7 @@ // Admin activation-funnel analytics (Wave 0 "See & Hear", GH#661). // Everything is DERIVED from existing collections — no new tracking, no -// third-party analytics, nothing leaves the instance. Human users only -// (bot User rows are excluded by botMetadata.agentName absence). +// third-party analytics, nothing leaves the instance. Human users only — +// see HUMAN_FILTER below for what "human" means and why it takes two signals. // // Honest approximations, documented: // - "returned D1/D7" uses User.lastActive >= createdAt + 1d/7d — i.e. "was @@ -35,6 +35,30 @@ const adminReadLimiter = rateLimit({ const DAY_MS = 24 * 60 * 60 * 1000; const dayKey = (d: Date) => d.toISOString().slice(0, 10); +// Who counts as a human, and why it takes TWO signals. +// +// Not every bot User row carries botMetadata.agentName. Rows created by the +// gateway bridge, the summarizer, and several openclaw install paths set +// botMetadata WITHOUT an agentName key. A filter keyed only on agentName +// therefore passes them as humans — on the dev instance that was 8 rows +// (clawdbot-bridge, commonly-summarizer, openclaw-inst-*, socialpulse-*), +// inflating totalUsers/DAU/WAU/signups ~10% and deflating every funnel rate, +// because a bot in the denominator can never convert. +// +// These two are exact logical complements (De Morgan) — keep them that way. +// Anything counted as a human here must NOT be counted as a bot there, or +// the "distinct human posters" metric double-subtracts. +const HUMAN_FILTER = { + isBot: { $ne: true }, + 'botMetadata.agentName': { $exists: false }, +}; +const BOT_FILTER = { + $or: [ + { isBot: true }, + { 'botMetadata.agentName': { $exists: true } }, + ], +}; + // Distinct PG user_ids with at least one message, within the candidate set. // Isolated so tests can run without a live PostgreSQL (mocked module). const pgUserIdsWithMessages = async (userIds: string[]): Promise> => { @@ -58,10 +82,7 @@ router.get('/funnel', adminReadLimiter, auth, adminAuth, async (req: any, res: a const users = await User.find({ createdAt: { $gte: since }, - $or: [ - { botMetadata: { $exists: false } }, - { 'botMetadata.agentName': { $exists: false } }, - ], + ...HUMAN_FILTER, }).select('_id createdAt lastActive').lean(); const ids = users.map((u: any) => String(u._id)); @@ -131,13 +152,6 @@ router.get('/funnel', adminReadLimiter, auth, adminAuth, async (req: any, res: a } }); -const HUMAN_FILTER = { - $or: [ - { botMetadata: { $exists: false } }, - { 'botMetadata.agentName': { $exists: false } }, - ], -}; - // GET /api/admin/analytics/usage?days=30 // Instance-level activity for the admin dashboard: signups/day (Mongo, // humans), messages/day + distinct human posters/day (PG), rolling DAU/WAU @@ -149,7 +163,7 @@ router.get('/usage', adminReadLimiter, auth, adminAuth, async (req: any, res: an const [signupUsers, botUsers, dau, wau, totalUsers] = await Promise.all([ User.find({ createdAt: { $gte: since }, ...HUMAN_FILTER }).select('createdAt').lean(), - User.find({ 'botMetadata.agentName': { $exists: true } }).select('_id').lean(), + User.find(BOT_FILTER).select('_id').lean(), User.countDocuments({ lastActive: { $gte: new Date(Date.now() - DAY_MS) }, ...HUMAN_FILTER }), User.countDocuments({ lastActive: { $gte: new Date(Date.now() - 7 * DAY_MS) }, ...HUMAN_FILTER }), User.countDocuments(HUMAN_FILTER),