From 8403dd0a37687cde072aa36acff995260b60eacf Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 30 Oct 2026 18:59:30 -0700 Subject: [PATCH 1/2] fix(agents): case-fold repair holds, skip Mongo on a PG miss (TASK-551) Two review points on #3023 (CodeRabbit review body): - takenBy was keyed by the exact label, so a community `scout` did not hold the Scout repair, while the rename path rejects case-only duplicates. Holds now compare case-folded labels. Holders are row ids, so a repaired row is never its own holder (a bare `scout` would otherwise hold itself). - With PG configured, a users mirror that updates no row no longer falls through to the Mongo write. writeRepair stops before Mongo, the row is named, and the run continues; the next run plans it again. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: wren Agent-Model: claude/claude-haiku-5-5/max --- .../scripts/repairFirstPartyLabels.test.js | 128 ++++++++++++++++-- backend/scripts/repair-first-party-labels.ts | 70 +++++++--- 2 files changed, 165 insertions(+), 33 deletions(-) diff --git a/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js b/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js index 23981682ca..28b6dcd2d0 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,113 @@ 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('stops before the Mongo write 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).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..182004d229 100644 --- a/backend/scripts/repair-first-party-labels.ts +++ b/backend/scripts/repair-first-party-labels.ts @@ -28,6 +28,10 @@ * between the two writes leaves the Mongo label unrepaired; the next run plans * the row again and rewrites both. * + * With PG configured, a row whose PG mirror updates no row is named and skipped + * before its Mongo write, so the two stores never disagree. The next run plans + * it again. + * * Run: cd backend && npx ts-node scripts/repair-first-party-labels.ts [--apply] */ import mongoose from 'mongoose'; @@ -58,8 +62,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 +113,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 +145,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 +155,19 @@ 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. With PG configured, a +// mirror that updates no row stops the repair before the Mongo write, so the two +// stores never disagree. Returns false when it stopped. +export async function writeRepair( + pool: PgPool | null, + repair: LabelRepair, + writeMongo: (id: string, label: string) => Promise, +): Promise { + if (pool && !(await mirrorLabel(pool, repair.id, repair.to))) return false; + await writeMongo(repair.id, repair.to); + return true; +} + interface LabelUser { _id: mongoose.Types.ObjectId; username?: string; @@ -167,14 +200,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 +212,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 stopped = 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(` ! not written ${r.username || r.id}: no PG users row took "${r.to}"; Mongo is unchanged`); + stopped += 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 = stopped > 0 ? `, ${stopped} stopped before the Mongo write (no PG row)` : ''; + const written = writable.length - stopped; + console.log(`\nDone — ${written} label(s) ${verb}, ${skipped.length} skipped for an operator decision${note}.`); if (pgPool) await pgPool.end(); await mongoose.disconnect(); } From b7f0974eabcf03899336926f26d0a791ae45aa45 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 30 Oct 2026 19:17:48 -0700 Subject: [PATCH 2/2] fix(repair): a PG miss writes Mongo and logs; a PG error still aborts (TASK-551) A PG mirror that updates no row means the bot has no users row yet, so there is no PG label to disagree with. The Mongo label is now written in that case, and the bot's first post copies it. The run logs "no PG users row yet" for the row and counts it in the Done line, in place of the stopped count. A thrown PG error still aborts before that row's Mongo write. The header says so: a re-run plans from current rows and picks up there. Tests: a PG miss writes Mongo and returns false; a PG error throws with no Mongo write. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: wren Agent-Model: claude/claude-haiku-5-5/max --- .../scripts/repairFirstPartyLabels.test.js | 9 +++++- backend/scripts/repair-first-party-labels.ts | 30 ++++++++++--------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js b/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js index 28b6dcd2d0..e6962364c9 100644 --- a/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js +++ b/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js @@ -225,10 +225,17 @@ describe('writeRepair', () => { id: 'u1', username: 'scout', from: 'scout', to: 'Scout', reason: 'first-party', }; - it('stops before the Mongo write when the PG mirror updates no row', async () => { + 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(); }); diff --git a/backend/scripts/repair-first-party-labels.ts b/backend/scripts/repair-first-party-labels.ts index 182004d229..27af8d5906 100644 --- a/backend/scripts/repair-first-party-labels.ts +++ b/backend/scripts/repair-first-party-labels.ts @@ -28,9 +28,10 @@ * between the two writes leaves the Mongo label unrepaired; the next run plans * the row again and rewrites both. * - * With PG configured, a row whose PG mirror updates no row is named and skipped - * before its Mongo write, so the two stores never disagree. The next run plans - * it again. + * 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] */ @@ -155,17 +156,18 @@ 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. With PG configured, a -// mirror that updates no row stops the repair before the Mongo write, so the two -// stores never disagree. Returns false when it stopped. +// 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 { - if (pool && !(await mirrorLabel(pool, repair.id, repair.to))) return false; + const mirrored = pool ? await mirrorLabel(pool, repair.id, repair.to) : true; await writeMongo(repair.id, repair.to); - return true; + return mirrored; } interface LabelUser { @@ -215,18 +217,18 @@ async function main() { const writeMongo = async (id: string, label: string): Promise => { await User.updateOne({ _id: id }, { $set: { 'botMetadata.displayName': label } }); }; - let stopped = 0; + let noPgRow = 0; for (const r of writable) { console.log(`${APPLY ? '' : '[dry] '}${r.username}: "${r.from}" → "${r.to}" (${r.reason})`); if (APPLY && !(await writeRepair(pgPool, r, writeMongo))) { - console.log(` ! not written ${r.username || r.id}: no PG users row took "${r.to}"; Mongo is unchanged`); - stopped += 1; + 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)'; - const note = stopped > 0 ? `, ${stopped} stopped before the Mongo write (no PG row)` : ''; - const written = writable.length - stopped; - console.log(`\nDone — ${written} label(s) ${verb}, ${skipped.length} skipped for an operator decision${note}.`); + 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(); }