fix(cli-review): stop losing session events to jsonb-hostile characters

session_events INSERTs were failing with

  ERROR: unsupported Unicode escape sequence
  DETAIL: \u0000 cannot be converted to text.

`payload` is whatever the CLI sent us — conversation text, tool output,
LLM responses — and it regularly carries a U+0000. Postgres jsonb refuses
it, the INSERT dies, and the event is dropped. Unpaired surrogates fail
the same way with a different message ("Unicode low surrogate must follow
a high surrogate").

Sanitising happens in SessionEventRepository.create, the single write
path, so every caller is covered rather than just the ingest use case.

It operates on the OBJECT, not on the serialised JSON.
`JSON.stringify(x).replace(/\u0000/g, '')` — the shape this codebase
already uses in pull-request-ingestion.service.ts — is a no-op for the
json case: stringify has already turned the code point into the
six-character text \u0000, so there is nothing left for that regex to
match. Matching the escape as text instead would be worse, because a
string that legitimately contains those six characters is valid jsonb and
rewriting it would corrupt real data. Both cases are pinned by tests.

Verified against Postgres before writing the fix: the raw form errors,
the sanitised form inserts, and the escaped-backslash form was already
fine. The repository test is mutation-checked — dropping the call turns
it red.

Author: Wellington01

libs/cli-review/infrastructure/repositories/session-event.repository.ts

@@ -6,6 +6,7 @@ import {
     ClassificationSource,
 } from './schemas/session-event.model';
 import { CliSessionClassifiedDecision } from '@libs/cli-review/domain/types/cli-session-capture.types';
+import { sanitizeForJsonb } from '@libs/common/utils/jsonb-safe';
 
 @Injectable()
 export class SessionEventRepository {
@@ -15,7 +16,16 @@ export class SessionEventRepository {
     ) {}
 
     async create(data: Partial<SessionEventModel>): Promise<SessionEventModel> {
-        const model = this.repo.create(data);
+        // `payload` is whatever the CLI sent us — conversation text, tool
+        // output, LLM responses. It regularly carries U+0000 and unpaired
+        // surrogates, both of which the jsonb column refuses, and the
+        // rejected INSERT drops the event. Sanitising at the repository
+        // rather than in the use case keeps every writer covered.
+        const model = this.repo.create(
+            data.payload
+                ? { ...data, payload: sanitizeForJsonb(data.payload) }
+                : data,
+        );
         return this.repo.save(model);
     }
 

libs/common/utils/jsonb-safe.ts

@@ -0,0 +1,98 @@
+/**
+ * Makes a value safe to store in a Postgres `jsonb` column.
+ *
+ * Postgres rejects two things that JavaScript strings happily carry, and
+ * both reach us through `session_events.payload` — CLI/LLM conversation
+ * text that nobody sanitised upstream:
+ *
+ *   U+0000     ERROR: unsupported Unicode escape sequence
+ *              DETAIL: \u0000 cannot be converted to text.
+ *
+ *   lone       ERROR: invalid input syntax for type json
+ *   surrogate  DETAIL: Unicode low surrogate must follow a high surrogate.
+ *
+ * The failing INSERT takes the whole row with it, so each occurrence is a
+ * dropped event.
+ *
+ * WHY THIS OPERATES ON THE OBJECT AND NOT ON THE SERIALISED JSON
+ *
+ * The obvious-looking fix — `JSON.stringify(x).replace(/\u0000/g, '')` —
+ * does nothing at all. `JSON.stringify` has already turned the real
+ * U+0000 code point into the six-character text `\u0000`, so no U+0000 is
+ * left in the output for that regex to match. (There is a copy of exactly
+ * that no-op in
+ * `libs/ee/analytics-warehouse/ingestion/pull-request-ingestion.service.ts`;
+ * it does work for the plain-`text` column it also guards, just not for
+ * the json one.)
+ *
+ * Matching the escape as text instead would be worse: a string that
+ * legitimately contains the six characters `\u0000` serialises to
+ * `\\u0000`, which Postgres accepts and stores as text. Rewriting that
+ * would corrupt real data.
+ *
+ * Cleaning the strings before serialisation avoids both traps.
+ */
+
+const NULL_CHAR = /\u0000/g;
+
+/**
+ * A high surrogate with no low surrogate after it, or a low surrogate
+ * with no high surrogate before it. Well-formed pairs are left alone, so
+ * emoji and other astral-plane characters survive untouched.
+ */
+const LONE_SURROGATE =
+    /[\uD800-\uDBFF](?![\uDC00-\uDFFF])|(?<![\uD800-\uDBFF])[\uDC00-\uDFFF]/g;
+
+/** U+FFFD REPLACEMENT CHARACTER — the standard stand-in for undecodable input. */
+const REPLACEMENT = '�';
+
+const sanitizeString = (value: string): string =>
+    value.replace(NULL_CHAR, '').replace(LONE_SURROGATE, REPLACEMENT);
+
+/**
+ * Recursion ceiling. Payloads are parsed request bodies, so they cannot
+ * contain cycles, but a malformed one could still be deep enough to blow
+ * the stack. Past the limit we drop the subtree rather than throw: losing
+ * a nested corner of the payload beats losing the event, which is the
+ * whole point of this module.
+ */
+const MAX_DEPTH = 64;
+
+function sanitize(value: unknown, depth: number): unknown {
+    if (typeof value === 'string') {
+        return sanitizeString(value);
+    }
+
+    if (value === null || typeof value !== 'object') {
+        return value;
+    }
+
+    if (depth >= MAX_DEPTH) {
+        return null;
+    }
+
+    // Dates carry no user text and JSON.stringify already handles them.
+    if (value instanceof Date) {
+        return value;
+    }
+
+    if (Array.isArray(value)) {
+        return value.map((item) => sanitize(item, depth + 1));
+    }
+
+    const out: Record<string, unknown> = {};
+    for (const [key, item] of Object.entries(value)) {
+        // Keys land in the jsonb document too, so they need the same
+        // treatment — a NUL in a key fails the INSERT just as hard.
+        out[sanitizeString(key)] = sanitize(item, depth + 1);
+    }
+    return out;
+}
+
+/**
+ * Returns a copy of `value` with every string cleaned of the characters
+ * Postgres `jsonb` refuses. The input is never mutated.
+ */
+export function sanitizeForJsonb<T>(value: T): T {
+    return sanitize(value, 0) as T;
+}

test/unit/cli-review/session-event.repository.spec.ts

@@ -0,0 +1,74 @@
+import { Test, TestingModule } from '@nestjs/testing';
+import { getRepositoryToken } from '@nestjs/typeorm';
+
+import { SessionEventRepository } from '@libs/cli-review/infrastructure/repositories/session-event.repository';
+import { SessionEventModel } from '@libs/cli-review/infrastructure/repositories/schemas/session-event.model';
+
+/**
+ * Guards the wiring, not the sanitiser itself (that is covered in
+ * test/unit/common/jsonb-safe.spec.ts). What matters here is that the
+ * payload gets cleaned on the way to TypeORM — if the call is dropped
+ * from `create`, these INSERTs go back to failing with
+ * `unsupported Unicode escape sequence` and the event is lost.
+ */
+const NUL = String.fromCharCode(0);
+
+describe('SessionEventRepository', () => {
+    let repository: SessionEventRepository;
+    let typeormRepo: { create: jest.Mock; save: jest.Mock };
+
+    beforeEach(async () => {
+        typeormRepo = {
+            create: jest.fn((x) => x),
+            save: jest.fn(async (x) => x),
+        };
+
+        const module: TestingModule = await Test.createTestingModule({
+            providers: [
+                SessionEventRepository,
+                {
+                    provide: getRepositoryToken(SessionEventModel),
+                    useValue: typeormRepo,
+                },
+            ],
+        }).compile();
+
+        repository = module.get(SessionEventRepository);
+    });
+
+    it('strips jsonb-hostile characters from payload before saving', async () => {
+        await repository.create({
+            sessionId: 's-1',
+            payload: { prompt: `write${NUL} tests`, ok: true },
+        } as Partial<SessionEventModel>);
+
+        const persisted = typeormRepo.create.mock.calls[0][0];
+
+        expect(persisted.payload).toEqual({ prompt: 'write tests', ok: true });
+        expect(JSON.stringify(persisted)).not.toContain('\\u0000');
+    });
+
+    it('leaves the rest of the row untouched', async () => {
+        await repository.create({
+            sessionId: 's-2',
+            branch: 'main',
+            payload: { a: 1 },
+        } as Partial<SessionEventModel>);
+
+        const persisted = typeormRepo.create.mock.calls[0][0];
+
+        expect(persisted.sessionId).toBe('s-2');
+        expect(persisted.branch).toBe('main');
+        expect(persisted.payload).toEqual({ a: 1 });
+    });
+
+    it('handles a row with no payload at all', async () => {
+        await expect(
+            repository.create({
+                sessionId: 's-3',
+            } as Partial<SessionEventModel>),
+        ).resolves.toBeDefined();
+
+        expect(typeormRepo.create.mock.calls[0][0].payload).toBeUndefined();
+    });
+});

test/unit/common/jsonb-safe.spec.ts

@@ -0,0 +1,121 @@
+import { sanitizeForJsonb } from '@libs/common/utils/jsonb-safe';
+
+/**
+ * The characters under test are built with String.fromCharCode instead of
+ * being typed literally: a raw U+0000 in a source file is invisible in
+ * diffs and review, and editors love to eat it.
+ *
+ * Every expectation below was checked against a real Postgres 16 before
+ * being written — `SELECT '{"a":"x\u0000y"}'::jsonb` fails with
+ * `unsupported Unicode escape sequence`, and the sanitised form passes.
+ */
+const NUL = String.fromCharCode(0);
+const HIGH_SURROGATE = String.fromCharCode(0xd800);
+const LOW_SURROGATE = String.fromCharCode(0xdc00);
+const REPLACEMENT = String.fromCharCode(0xfffd);
+
+/** What node-postgres ends up handing to Postgres. */
+const asJsonText = (value: unknown) => JSON.stringify(value);
+
+describe('sanitizeForJsonb', () => {
+    describe('the characters Postgres jsonb rejects', () => {
+        it('removes U+0000 from string values', () => {
+            const out = sanitizeForJsonb({ a: `x${NUL}y` });
+
+            expect(out).toEqual({ a: 'xy' });
+            expect(asJsonText(out)).not.toContain('\\u0000');
+        });
+
+        it('removes U+0000 from object keys', () => {
+            const out = sanitizeForJsonb({ [`k${NUL}ey`]: 'v' });
+
+            expect(Object.keys(out)).toEqual(['key']);
+            expect(asJsonText(out)).not.toContain('\\u0000');
+        });
+
+        it('replaces an unpaired high surrogate', () => {
+            const out = sanitizeForJsonb({ a: `x${HIGH_SURROGATE}y` });
+
+            expect(out).toEqual({ a: `x${REPLACEMENT}y` });
+        });
+
+        it('replaces an unpaired low surrogate', () => {
+            const out = sanitizeForJsonb({ a: `x${LOW_SURROGATE}y` });
+
+            expect(out).toEqual({ a: `x${REPLACEMENT}y` });
+        });
+
+        it('reaches into nested objects and arrays', () => {
+            const out = sanitizeForJsonb({
+                turns: [{ text: `hi${NUL}` }, { text: 'ok' }],
+                meta: { nested: { deep: `a${HIGH_SURROGATE}` } },
+            });
+
+            expect(out).toEqual({
+                turns: [{ text: 'hi' }, { text: 'ok' }],
+                meta: { nested: { deep: `a${REPLACEMENT}` } },
+            });
+        });
+    });
+
+    describe('what it must NOT touch', () => {
+        it('keeps a literal backslash-u-0000 sequence intact', () => {
+            // This is six ordinary characters, not a NUL. Postgres stores it
+            // happily as text, so rewriting it would corrupt real data —
+            // which is what a regex over the serialised JSON would do.
+            const literal = 'C:\\u0000\\path';
+
+            expect(sanitizeForJsonb({ a: literal })).toEqual({ a: literal });
+        });
+
+        it('keeps well-formed surrogate pairs (emoji) intact', () => {
+            const emoji = 'ship it 🚀';
+
+            expect(sanitizeForJsonb({ a: emoji })).toEqual({ a: emoji });
+        });
+
+        it('leaves non-string primitives alone', () => {
+            const input = { n: 1, b: true, z: null, u: undefined };
+
+            expect(sanitizeForJsonb(input)).toEqual(input);
+        });
+
+        it('does not mutate the input', () => {
+            const input = { a: `x${NUL}y` };
+
+            sanitizeForJsonb(input);
+
+            expect(input.a).toBe(`x${NUL}y`);
+        });
+    });
+
+    describe('depth ceiling', () => {
+        it('drops the subtree past the limit instead of throwing', () => {
+            let deep: Record<string, unknown> = { text: `end${NUL}` };
+            for (let i = 0; i < 200; i++) {
+                deep = { nested: deep };
+            }
+
+            expect(() => sanitizeForJsonb(deep)).not.toThrow();
+            expect(asJsonText(sanitizeForJsonb(deep))).not.toContain('\\u0000');
+        });
+    });
+
+    describe('the trap this replaces', () => {
+        it('shows why sanitising after JSON.stringify is a no-op', () => {
+            const raw = { a: `x${NUL}y` };
+
+            // The pattern used elsewhere in the codebase: stringify first,
+            // then strip U+0000. By then there is no U+0000 left to strip —
+            // stringify turned it into the text \u0000.
+            const stringifiedThenStripped = JSON.stringify(raw).replace(
+                new RegExp(NUL, 'g'),
+                '',
+            );
+            expect(stringifiedThenStripped).toContain('\\u0000');
+
+            // Sanitising the object first is what actually removes it.
+            expect(asJsonText(sanitizeForJsonb(raw))).not.toContain('\\u0000');
+        });
+    });
+});