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),