diff --git a/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js b/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js index 23981682ca..e6962364c9 100644 --- a/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js +++ b/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'); + }); +}); diff --git a/backend/scripts/repair-first-party-labels.ts b/backend/scripts/repair-first-party-labels.ts index a4459fffa9..27af8d5906 100644 --- a/backend/scripts/repair-first-party-labels.ts +++ b/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 { + const takenBy = new Map(); + 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 = 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 { 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, +): Promise { + 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(); - 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 => { + 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(); }