fix(agents): case-fold repair holds; a PG miss still writes Mongo (TASK-551)

backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js

@@ -1,7 +1,9 @@
 // The planner behind scripts/repair-first-party-labels (TASK-551). Each repair
 // rule is pinned on its own, the rows it must leave alone are pinned too, and
 // a label another agent already holds is held rather than written.
-const { planLabelRepairs, splitRepairs, mirrorLabel } = require('../../../scripts/repair-first-party-labels');
+const {
+  planLabelRepairs, splitRepairs, mirrorLabel, takenByLabel, writeRepair,
+} = require('../../../scripts/repair-first-party-labels');
 
 const row = (overrides) => ({
   id: 'u1',
@@ -81,29 +83,39 @@ describe('planLabelRepairs', () => {
   });
 
   it('holds a first-party default row whose label a named peer already holds', () => {
-    const takenBy = new Map([['Scout', ['scout']]]);
-    expect(planLabelRepairs([row({ displayName: 'Scout (Default)' })], takenBy)).toEqual([
-      expect.objectContaining({ to: 'Scout', heldBy: ['scout'] }),
+    const rows = [
+      row({ displayName: 'Scout (Default)' }),
+      row({
+        id: 'u2', agentName: 'openclaw', username: 'scout-named', instanceId: 'scout', displayName: 'Scout',
+      }),
+    ];
+    expect(planLabelRepairs(rows, takenByLabel(rows))).toEqual([
+      expect.objectContaining({ id: 'u1', to: 'Scout', heldBy: ['scout'] }),
     ]);
   });
 
   it('repairs a label that only a per-user row holds', () => {
-    const takenBy = new Map([['Scout', ['u0da521ab41']]]);
-    const [repair] = planLabelRepairs([row({ displayName: 'Scout (Default)' })], takenBy);
+    const rows = [
+      row({ displayName: 'Scout (Default)' }),
+      row({
+        id: 'u2', username: 'scout-u0da521ab41', instanceId: 'u0da521ab41', displayName: 'Scout',
+      }),
+    ];
+    const [repair] = planLabelRepairs(rows, takenByLabel(rows));
     expect(repair.to).toBe('Scout');
     expect(repair.heldBy).toBeUndefined();
   });
 
   it('per-user rows neither hold nor contest a label', () => {
-    const takenBy = new Map([['Scout', ['uabcdef0123']]]);
-    expect(planLabelRepairs([
+    const rows = [
       row({
         id: 'u1', username: 'scout-a', instanceId: 'u0da521ab41', displayName: 'scout',
       }),
       row({
         id: 'u2', username: 'scout-b', instanceId: 'uabcdef0123', displayName: '',
       }),
-    ], takenBy)).toEqual([
+    ];
+    expect(planLabelRepairs(rows, takenByLabel(rows))).toEqual([
       {
         id: 'u1', username: 'scout-a', from: 'scout', to: 'Scout', reason: 'first-party',
       },
@@ -124,25 +136,120 @@ describe('planLabelRepairs', () => {
       expect.objectContaining({ id: 'u2', heldBy: ['default'] }),
     ]);
   });
+  it('a community scout holds the Scout repair, whatever the case of its label', () => {
+    const rows = [
+      row({ displayName: 'scout' }),
+      row({
+        id: 'u2', agentName: 'openclaw', username: 'scout-community', instanceId: 'scout', displayName: 'scout',
+      }),
+    ];
+    expect(planLabelRepairs(rows, takenByLabel(rows))).toEqual([
+      expect.objectContaining({ id: 'u1', to: 'Scout', heldBy: ['scout'] }),
+    ]);
+  });
+
+  it('does not hold a bare scout row against its own repair', () => {
+    const rows = [row({ displayName: 'scout' })];
+    const [repair] = planLabelRepairs(rows, takenByLabel(rows));
+    expect(repair).toEqual({
+      id: 'u1', username: 'scout', from: 'scout', to: 'Scout', reason: 'first-party',
+    });
+    expect(repair.heldBy).toBeUndefined();
+  });
+});
+
+describe('takenByLabel', () => {
+  it('keys each non-blank label by its trimmed, lowercased form, holding row ids', () => {
+    const rows = [
+      row({ id: 'u1', displayName: ' Scout ' }),
+      row({
+        id: 'u2', agentName: 'openclaw', username: 'scout-community', instanceId: 'scout', displayName: 'scout',
+      }),
+      row({ id: 'u3', displayName: '' }),
+    ];
+    expect(takenByLabel(rows)).toEqual(new Map([['scout', ['u1', 'u2']]]));
+  });
 });
 
 describe('splitRepairs', () => {
   it('skips a first-party default row whose target label a named peer holds, and never writes it', () => {
-    const repairs = planLabelRepairs([row({ displayName: 'Scout (Default)' })], new Map([['Scout', ['lab']]]));
-    const { writable, skipped } = splitRepairs(repairs);
+    const rows = [
+      row({ displayName: 'Scout (Default)' }),
+      row({
+        id: 'u2', agentName: 'openclaw', username: 'lab', instanceId: 'lab', displayName: 'Scout',
+      }),
+    ];
+    const { writable, skipped } = splitRepairs(planLabelRepairs(rows, takenByLabel(rows)));
     expect(writable).toEqual([]);
     expect(skipped).toEqual([expect.objectContaining({ id: 'u1', to: 'Scout', heldBy: ['lab'] })]);
   });
 
   it('still writes a per-user repair onto the label a skipped row would have taken', () => {
-    const repairs = planLabelRepairs([
+    const rows = [
       row({ displayName: 'Scout (Default)' }),
       row({
         id: 'u2', username: 'scout-u0da521ab41', instanceId: 'u0da521ab41', displayName: 'scout',
       }),
-    ], new Map([['Scout', ['lab']]]));
-    const { writable, skipped } = splitRepairs(repairs);
+      row({
+        id: 'u3', agentName: 'openclaw', username: 'lab', instanceId: 'lab', displayName: 'Scout',
+      }),
+    ];
+    const { writable, skipped } = splitRepairs(planLabelRepairs(rows, takenByLabel(rows)));
     expect(skipped.map((r) => r.id)).toEqual(['u1']);
     expect(writable.map((r) => r.id)).toEqual(['u2']);
   });
 });
+
+describe('mirrorLabel', () => {
+  it('sets the PG username of the row and reports that a row took it', async () => {
+    const pool = { query: jest.fn().mockResolvedValue({ rowCount: 1 }), end: jest.fn() };
+    await expect(mirrorLabel(pool, 'u1', 'Scout')).resolves.toBe(true);
+    expect(pool.query).toHaveBeenCalledWith(
+      'UPDATE users SET username = $2, updated_at = $3 WHERE _id = $1',
+      ['u1', 'Scout', expect.any(Date)],
+    );
+  });
+
+  it('reports false when the PG update matches no row', async () => {
+    const pool = { query: jest.fn().mockResolvedValue({ rowCount: 0 }), end: jest.fn() };
+    await expect(mirrorLabel(pool, 'u1', 'Scout')).resolves.toBe(false);
+  });
+
+  it('reports false, and touches nothing, when PG is not configured', async () => {
+    await expect(mirrorLabel(null, 'u1', 'Scout')).resolves.toBe(false);
+  });
+});
+
+describe('writeRepair', () => {
+  const repair = {
+    id: 'u1', username: 'scout', from: 'scout', to: 'Scout', reason: 'first-party',
+  };
+
+  it('writes Mongo and reports false when the PG mirror updates no row', async () => {
+    const pool = { query: jest.fn().mockResolvedValue({ rowCount: 0 }), end: jest.fn() };
+    const writeMongo = jest.fn().mockResolvedValue(undefined);
+    await expect(writeRepair(pool, repair, writeMongo)).resolves.toBe(false);
+    expect(writeMongo).toHaveBeenCalledWith('u1', 'Scout');
+  });
+
+  it('throws, and writes no Mongo label, when the PG mirror errors', async () => {
+    const pool = { query: jest.fn().mockRejectedValue(new Error('pg down')), end: jest.fn() };
+    const writeMongo = jest.fn().mockResolvedValue(undefined);
+    await expect(writeRepair(pool, repair, writeMongo)).rejects.toThrow('pg down');
+    expect(writeMongo).not.toHaveBeenCalled();
+  });
+
+  it('writes PG first, then Mongo, when the PG mirror updates the row', async () => {
+    const pool = { query: jest.fn().mockResolvedValue({ rowCount: 1 }), end: jest.fn() };
+    const writeMongo = jest.fn().mockResolvedValue(undefined);
+    await expect(writeRepair(pool, repair, writeMongo)).resolves.toBe(true);
+    expect(pool.query.mock.invocationCallOrder[0]).toBeLessThan(writeMongo.mock.invocationCallOrder[0]);
+    expect(writeMongo).toHaveBeenCalledWith('u1', 'Scout');
+  });
+
+  it('writes Mongo alone when PG is not configured', async () => {
+    const writeMongo = jest.fn().mockResolvedValue(undefined);
+    await expect(writeRepair(null, repair, writeMongo)).resolves.toBe(true);
+    expect(writeMongo).toHaveBeenCalledWith('u1', 'Scout');
+  });
+});

backend/scripts/repair-first-party-labels.ts

@@ -28,6 +28,11 @@
  * between the two writes leaves the Mongo label unrepaired; the next run plans
  * the row again and rewrites both.
  *
+ * A PG error (a thrown query) aborts the run before that row's Mongo write. A
+ * re-run plans from current rows and picks up there. A PG mirror that updates no
+ * row means the bot has never posted, so it has no users row yet: its Mongo label
+ * is still written, and its first post creates the row from that label.
+ *
  * Run:  cd backend && npx ts-node scripts/repair-first-party-labels.ts [--apply]
  */
 import mongoose from 'mongoose';
@@ -58,8 +63,21 @@ export interface LabelRepair {
   heldBy?: string[];
 }
 
-// Pure, so the rules are pinned without a database. `takenBy` maps a label to
-// the instanceIds of every bot already displaying it.
+// Each bot's label, trimmed and lowercased, to the row ids displaying it. A blank
+// label holds nothing. Row ids, not instanceIds, so the planner can leave a
+// repaired row out of its own holders.
+export function takenByLabel(rows: LabelRow[]): Map<string, string[]> {
+  const takenBy = new Map<string, string[]>();
+  for (const row of rows) {
+    const label = (row.displayName || '').trim().toLowerCase();
+    if (!label) continue;
+    takenBy.set(label, [...(takenBy.get(label) || []), row.id]);
+  }
+  return takenBy;
+}
+
+// Pure, so the rules are pinned without a database. `takenBy` is takenByLabel's
+// map, so a holder is found whatever the case of its label.
 export function planLabelRepairs(
   rows: LabelRow[],
   takenBy: ReadonlyMap<string, readonly string[]> = new Map(),
@@ -96,11 +114,14 @@ export function planLabelRepairs(
   );
   const shared = repairs.filter((r) => !isPerUser(instanceOf.get(r.id)));
   for (const r of shared) {
-    const holders = (takenBy.get(r.to) || [])
-      .filter((id) => !isPerUser(id))
-      .map((id) => id || '(none)');
+    // Compared without case, as the rename path does: `scout` holds `Scout`. The
+    // repaired row is never its own holder, or a bare `scout` would hold itself.
+    const key = r.to.toLowerCase();
+    const holders = (takenBy.get(key) || [])
+      .filter((id) => id !== r.id && !isPerUser(instanceOf.get(id)))
+      .map((id) => instanceOf.get(id) || '(none)');
     const others = shared
-      .filter((o) => o.id !== r.id && o.to === r.to)
+      .filter((o) => o.id !== r.id && o.to.toLowerCase() === key)
       .map((o) => instanceOf.get(o.id) || '(none)');
     const heldBy = [...holders, ...others];
     if (heldBy.length > 0) r.heldBy = heldBy;
@@ -125,7 +146,7 @@ interface PgPool {
 
 // Sets the chat-facing PG label to the value the Mongo write sets.
 // syncUserToPostgreSQL writes the same column (chatUsernameFor: a bot's
-// displayName). Returns false when PG is not configured.
+// displayName). Returns false when PG is not configured or no row took the label.
 export async function mirrorLabel(pool: PgPool | null, id: string, label: string): Promise<boolean> {
   if (!pool) return false;
   const result = await pool.query(
@@ -135,6 +156,20 @@ export async function mirrorLabel(pool: PgPool | null, id: string, label: string
   return (result.rowCount || 0) > 0;
 }
 
+// The --apply write for one repair: PG first, then Mongo. A mirror that updates no
+// row means the bot has no users row yet, so there is no PG label to disagree
+// with: the Mongo write still happens, and the bot's first post copies it. Returns
+// false in that case. A PG error throws before the Mongo write.
+export async function writeRepair(
+  pool: PgPool | null,
+  repair: LabelRepair,
+  writeMongo: (id: string, label: string) => Promise<void>,
+): Promise<boolean> {
+  const mirrored = pool ? await mirrorLabel(pool, repair.id, repair.to) : true;
+  await writeMongo(repair.id, repair.to);
+  return mirrored;
+}
+
 interface LabelUser {
   _id: mongoose.Types.ObjectId;
   username?: string;
@@ -167,14 +202,7 @@ async function main() {
     instanceId: u.botMetadata?.instanceId,
     displayName: u.botMetadata?.displayName,
   }));
-  const takenBy = new Map<string, string[]>();
-  for (const row of rows) {
-    const label = (row.displayName || '').trim();
-    if (!label) continue;
-    takenBy.set(label, [...(takenBy.get(label) || []), row.instanceId || '']);
-  }
-
-  const { writable, skipped } = splitRepairs(planLabelRepairs(rows, takenBy));
+  const { writable, skipped } = splitRepairs(planLabelRepairs(rows, takenByLabel(rows)));
   for (const r of skipped) {
     const holders = (r.heldBy || []).join(', ');
     console.log(
@@ -186,15 +214,21 @@ async function main() {
   if (APPLY && !pgPool) {
     console.log('  PG_HOST is not set: users.username is not mirrored; the next agent sync writes it.');
   }
+  const writeMongo = async (id: string, label: string): Promise<void> => {
+    await User.updateOne({ _id: id }, { $set: { 'botMetadata.displayName': label } });
+  };
+  let noPgRow = 0;
   for (const r of writable) {
     console.log(`${APPLY ? '' : '[dry] '}${r.username}: "${r.from}" → "${r.to}" (${r.reason})`);
-    if (APPLY) {
-      await mirrorLabel(pgPool, r.id, r.to);
-      await User.updateOne({ _id: r.id }, { $set: { 'botMetadata.displayName': r.to } });
+    if (APPLY && !(await writeRepair(pgPool, r, writeMongo))) {
+      console.log(`  - ${r.username || r.id}: no PG users row yet; its first post copies this label`);
+      noPgRow += 1;
     }
   }
   const verb = APPLY ? 'repaired' : 'would be repaired (dry run; pass --apply to write)';
-  console.log(`\nDone — ${writable.length} label(s) ${verb}, ${skipped.length} skipped for an operator decision.`);
+  const note = noPgRow > 0 ? `, ${noPgRow} with no PG users row yet` : '';
+  const count = writable.length;
+  console.log(`\nDone — ${count} label(s) ${verb}, ${skipped.length} skipped for an operator decision${note}.`);
   if (pgPool) await pgPool.end();
   await mongoose.disconnect();
 }