From f523814aa6aa6c17ad274eb058d5ddc01ea9f5c7 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Sun, 26 Jul 2026 16:16:16 -0700 Subject: [PATCH] fix(agents): stop inline base64 avatars blowing the agent context window (#758) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Avatars are stored as `data:` URIs on the user row, so every agent-facing message carried its author's full image bytes — and the same avatar repeated on every message that author had sent. Measured on a real pod, `commonly_get_messages` at the default limit returned 230,170 characters of which 71% was image data. An agent asked to read a busy room exhausted its context on profile pictures before reaching the conversation, saw only truncation, and had no way to tell why. It scales with room activity rather than conversation length, and it hits exactly the thing we market: agents reading the room and handing off. Fixed at the router, not per handler. The leak was spread across getRecentMessages, the post-message echo, thread comments and the create-pod member list; there is no shared user serializer to fix instead; and a per-handler fix would not cover the next endpoint added. One `router.use` on the agent runtime router strips inline avatars from every response. Only `data:` values are dropped — a URL costs nothing and stays useful, so the rule is "no base64 in an agent payload", not "no avatars". The strip returns a copy and never mutates its input. That is load-bearing: the same message object is reused for the human-facing Socket.io broadcast, where the avatar IS rendered, so stripping in place would blank every live avatar in the UI. Deliberately NOT in scope, filed as follow-up: the human read path (`formatMessage` emits two copies of each avatar), the sidebar pod listing (populates before filtering), and migrating the 6 remaining `data:` rows to object storage per ADR-002 with a write-side cap. Closes #758 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01DMeWzgFxsfBcjoLVLewEES --- .../agentsRuntime.noInlineAvatars.test.js | 126 ++++++++++++++++++ .../avatarService.stripInline.test.js | 94 +++++++++++++ backend/routes/agentsRuntime.ts | 19 +++ backend/services/avatarService.ts | 46 +++++++ 4 files changed, 285 insertions(+) create mode 100644 backend/__tests__/unit/routes/agentsRuntime.noInlineAvatars.test.js create mode 100644 backend/__tests__/unit/services/avatarService.stripInline.test.js diff --git a/backend/__tests__/unit/routes/agentsRuntime.noInlineAvatars.test.js b/backend/__tests__/unit/routes/agentsRuntime.noInlineAvatars.test.js new file mode 100644 index 00000000..f32df668 --- /dev/null +++ b/backend/__tests__/unit/routes/agentsRuntime.noInlineAvatars.test.js @@ -0,0 +1,126 @@ +/** + * #758 — the agent runtime router must never emit an inline base64 avatar. + * + * This mounts the REAL router in an express app (rather than pulling a handler + * out of the stack) specifically so the router-level middleware runs. The + * guarantee under test is the choke point itself: individual handlers are + * allowed to hand up whole populated user documents, and the router is what + * makes that safe. A per-handler test would pass even if the middleware were + * deleted. + */ + +// Local Node-26 drift: jsonwebtoken's transitive buffer-equal-constant-time +// throws on import. CI (Node 20) is unaffected; this keeps the suite runnable +// on a dev machine without changing what is under test. +jest.mock('jsonwebtoken', () => ({ sign: jest.fn(), verify: jest.fn(), decode: jest.fn() })); + +jest.mock('../../../middleware/agentRuntimeAuth', () => (req, res, next) => { + req.agentUser = { _id: 'bot-1' }; + req.agentInstallations = [{ podId: 'pod-1', status: 'active' }]; + req.agentAuthorizedPodIds = ['pod-1']; + next(); +}); +jest.mock('../../../middleware/auth', () => (req, res, next) => next()); +jest.mock('../../../middleware/apiTokenScopes', () => ({ + requireApiTokenScopes: () => (req, res, next) => next(), +})); + +jest.mock('../../../services/agentEventService', () => ({})); +jest.mock('../../../services/agentIdentityService', () => ({ + buildAgentUsername: jest.fn((a) => a), +})); +jest.mock('../../../services/agentMessageService', () => ({ + getRecentMessages: jest.fn(), +})); +jest.mock('../../../services/agentThreadService', () => ({})); +jest.mock('../../../services/podContextService', () => ({})); +jest.mock('../../../services/globalModelConfigService', () => ({})); +jest.mock('../../../services/socialPolicyService', () => ({})); +jest.mock('../../../integrations', () => ({ get: jest.fn() })); +jest.mock('../../../models/Activity', () => ({})); +jest.mock('../../../models/User', () => ({ findById: jest.fn() })); +jest.mock('../../../models/Post', () => ({ findById: jest.fn() })); +jest.mock('../../../models/Pod', () => ({ find: jest.fn() })); +jest.mock('../../../services/dmService', () => ({ getOrCreateAgentDM: jest.fn() })); +jest.mock('../../../models/Integration', () => ({ find: jest.fn(), findOne: jest.fn() })); +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { findOne: jest.fn(), find: jest.fn() }, +})); + +const express = require('express'); +const request = require('supertest'); + +const AgentMessageService = require('../../../services/agentMessageService'); +const router = require('../../../routes/agentsRuntime'); + +const DATA_URI = `data:image/jpeg;base64,${'A'.repeat(4000)}`; + +const app = express(); +app.use(express.json()); +app.use('/api/agents/runtime', router); + +describe('agent runtime router strips inline avatars (#758)', () => { + beforeEach(() => jest.clearAllMocks()); + + test('GET /pods/:podId/messages returns no data: URI even when the service supplies one', async () => { + AgentMessageService.getRecentMessages.mockResolvedValue([ + { + _id: 'm1', + id: 'm1', + content: 'hello', + username: 'sam', + isBot: false, + self: false, + userId: { _id: 'u1', username: 'sam', profilePicture: DATA_URI }, + profile_picture: DATA_URI, + }, + ]); + + const res = await request(app).get('/api/agents/runtime/pods/pod-1/messages'); + + expect(res.status).toBe(200); + expect(JSON.stringify(res.body)).not.toContain('data:'); + // The conversation itself must survive untouched. + expect(res.body.messages[0].content).toBe('hello'); + expect(res.body.messages[0].username).toBe('sam'); + // And so must the fields the wrapper's self-post detection depends on (#757). + expect(res.body.messages[0]).toHaveProperty('isBot', false); + expect(res.body.messages[0]).toHaveProperty('self', false); + }); + + test('a URL avatar still reaches the agent — only base64 is dropped', async () => { + AgentMessageService.getRecentMessages.mockResolvedValue([ + { + _id: 'm1', + id: 'm1', + content: 'hi', + username: 'sam', + userId: { _id: 'u1', username: 'sam', profilePicture: '/api/uploads/a.png' }, + }, + ]); + + const res = await request(app).get('/api/agents/runtime/pods/pod-1/messages'); + + expect(res.status).toBe(200); + expect(res.body.messages[0].userId.profilePicture).toBe('/api/uploads/a.png'); + }); + + test('the payload actually shrinks — this is a context-window fix, not cosmetics', async () => { + AgentMessageService.getRecentMessages.mockResolvedValue( + Array.from({ length: 20 }, (_, i) => ({ + _id: `m${i}`, + id: `m${i}`, + content: 'a short line of conversation', + username: 'sam', + userId: { _id: 'u1', username: 'sam', profilePicture: DATA_URI }, + profile_picture: DATA_URI, + })), + ); + + const res = await request(app).get('/api/agents/runtime/pods/pod-1/messages'); + const size = JSON.stringify(res.body).length; + + // 20 messages x 2 copies x ~4KB would be ~160KB unstripped. + expect(size).toBeLessThan(5000); + }); +}); diff --git a/backend/__tests__/unit/services/avatarService.stripInline.test.js b/backend/__tests__/unit/services/avatarService.stripInline.test.js new file mode 100644 index 00000000..30b9f997 --- /dev/null +++ b/backend/__tests__/unit/services/avatarService.stripInline.test.js @@ -0,0 +1,94 @@ +/** + * #758 — no inline base64 avatars in agent-bound payloads. + * + * Avatars are stored as `data:` URIs on the user row, so every message carries + * its author's full image bytes and the same avatar repeats on every message + * that author sent. Measured on a real pod, a 20-message agent read returned + * 230,170 characters of which 71% was image data — an agent asked to read a + * busy room exhausts its context on profile pictures before reaching the + * conversation. + * + * The rule is "no base64 in an agent payload", NOT "no avatars": a URL costs + * nothing and stays useful, so it survives. + */ + +const { stripInlineAvatars } = require('../../../services/avatarService'); + +const DATA_URI = 'data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAAAQABAAD'; + +describe('stripInlineAvatars', () => { + test('drops a data: avatar under either spelling', () => { + const out = stripInlineAvatars({ + userId: { _id: 'u1', username: 'sam', profilePicture: DATA_URI }, + profile_picture: DATA_URI, + content: 'hello', + }); + + expect(out.userId).not.toHaveProperty('profilePicture'); + expect(out).not.toHaveProperty('profile_picture'); + expect(out.content).toBe('hello'); + expect(out.userId.username).toBe('sam'); + }); + + test('keeps a URL avatar — the rule is no base64, not no avatars', () => { + const out = stripInlineAvatars({ + userId: { profilePicture: '/api/uploads/abc.png' }, + profile_picture: 'https://example.com/a.png', + }); + + expect(out.userId.profilePicture).toBe('/api/uploads/abc.png'); + expect(out.profile_picture).toBe('https://example.com/a.png'); + }); + + test('reaches into arrays and nested members (the create-pod member list)', () => { + const out = stripInlineAvatars({ + pod: { + createdBy: { username: 'sam', profilePicture: DATA_URI }, + members: [ + { user: { username: 'a', profilePicture: DATA_URI } }, + { user: { username: 'b', profilePicture: '/api/uploads/b.png' } }, + ], + }, + }); + + expect(JSON.stringify(out)).not.toContain('data:'); + expect(out.pod.members[1].user.profilePicture).toBe('/api/uploads/b.png'); + expect(out.pod.members.map((m) => m.user.username)).toEqual(['a', 'b']); + }); + + test('does NOT mutate the input — the same object feeds the human socket broadcast', () => { + const original = { userId: { profilePicture: DATA_URI }, content: 'x' }; + stripInlineAvatars(original); + + // The Socket.io newMessage frame reuses this object and the UI renders + // that avatar; stripping in place would blank it for every human viewer. + expect(original.userId.profilePicture).toBe(DATA_URI); + }); + + test('passes non-plain objects (Date, ObjectId-like) through untouched', () => { + const when = new Date('2026-07-26T00:00:00Z'); + const oid = { toHexString: () => 'abc' }; + Object.setPrototypeOf(oid, { custom: true }); + + const out = stripInlineAvatars({ createdAt: when, _id: oid }); + + expect(out.createdAt).toBe(when); + expect(out._id).toBe(oid); + }); + + test('survives a self-referential payload without recursing forever', () => { + const node = { username: 'a', profilePicture: DATA_URI }; + node.self = node; + + const out = stripInlineAvatars({ node }); + + expect(out.node).not.toHaveProperty('profilePicture'); + expect(out.node.self).toBe(out.node); + }); + + test('leaves primitives and null alone', () => { + expect(stripInlineAvatars(null)).toBeNull(); + expect(stripInlineAvatars('x')).toBe('x'); + expect(stripInlineAvatars(3)).toBe(3); + }); +}); diff --git a/backend/routes/agentsRuntime.ts b/backend/routes/agentsRuntime.ts index bceae27f..6f8ea1b7 100644 --- a/backend/routes/agentsRuntime.ts +++ b/backend/routes/agentsRuntime.ts @@ -96,7 +96,26 @@ try { PGPod = null; } +const { stripInlineAvatars } = require('../services/avatarService'); + const router = express.Router(); + +// Every response on this router is bound for an agent's context window, so no +// inline base64 avatar may ride along. Enforcing it here rather than at each +// handler is deliberate: the leak was spread across getRecentMessages, the +// post-message echo, thread comments and the create-pod member list, there is +// no shared user serializer to fix instead, and a per-handler fix would not +// cover the next endpoint someone adds. Measured before this: a 20-message +// read returned 230,170 chars, 71% of it image data (#758). +// +// Only `data:` values are dropped — URLs are cheap and stay useful. The +// original objects are never mutated, because the same message object is +// reused for the human-facing Socket.io broadcast where avatars ARE rendered. +router.use((_req: any, res: any, next: any) => { + const originalJson = res.json.bind(res); + res.json = (body: unknown) => originalJson(stripInlineAvatars(body)); + next(); +}); const parseNonNegativeInt = (value: any, fallback: any) => { const parsed = parseInt(value, 10); if (Number.isNaN(parsed)) return fallback; diff --git a/backend/services/avatarService.ts b/backend/services/avatarService.ts index 3e6ea478..e8ea2325 100644 --- a/backend/services/avatarService.ts +++ b/backend/services/avatarService.ts @@ -62,6 +62,52 @@ export function resolveAvatarUrl(user: AvatarUser | null | undefined): string | return normalized; } +// Keys that carry an avatar reference anywhere in a serialized payload. Both +// spellings exist because the PG row is snake_case and the Mongo/populated +// shape is camelCase, and several responses carry BOTH for the same user. +const AVATAR_KEYS = new Set(['profilePicture', 'profile_picture']); + +/** + * Remove inline base64 avatars from a payload bound for an agent. + * + * Avatars are stored as `data:` URIs on the user row, so every message carries + * its author's full image bytes — and the same avatar repeats on every message + * that author has sent. Measured on a real pod, `commonly_get_messages` at the + * default limit returned 230,170 characters of which 71% was image data: an + * agent asked to read a busy room exhausts its context on profile pictures + * before it reaches the conversation (#758). + * + * Only `data:` values are dropped. A URL costs nothing and stays useful, so it + * is preserved — the rule is "no base64 in an agent payload", not "no avatars". + * + * Returns a structurally-shared copy; the input is never mutated, because the + * same message object is reused for the human-facing Socket.io broadcast where + * the avatar IS rendered. + */ +export function stripInlineAvatars(payload: T): T { + const seen = new WeakMap(); + + const walk = (value: unknown): unknown => { + if (Array.isArray(value)) return value.map(walk); + if (!value || typeof value !== 'object') return value; + // Dates, ObjectIds and other non-plain objects must pass through intact. + const proto = Object.getPrototypeOf(value); + if (proto !== Object.prototype && proto !== null) return value; + const cached = seen.get(value as object); + if (cached !== undefined) return cached; + + const out: Record = {}; + seen.set(value as object, out); + for (const [key, val] of Object.entries(value as Record)) { + if (AVATAR_KEYS.has(key) && typeof val === 'string' && /^data:/i.test(val)) continue; + out[key] = walk(val); + } + return out; + }; + + return walk(payload) as T; +} + export async function resolveAvatarUrls( userIds: string[], ): Promise> {