From 8407ca98d1533572f33d1e2027363f784b5e4f84 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 30 Oct 2026 14:45:40 -0700 Subject: [PATCH 1/4] fix(agents): store Scout's curated label and keep "(Default)" off the default row (TASK-551) Scout has no AGENT_TYPES entry, so the create, upgrade and refresh arms stored its bare agentName ("scout") when no displayName was passed. The collision resolver also picked its canonical row by shortest instanceId: a named peer with a shorter id displaced the default row, which then came back as "Scout (Default)" in the invite line. - FIRST_PARTY_DISPLAY_NAMES (scout -> Scout), used by all three arms when no displayName is passed. Only scout is listed. - Canonical rule: the default instance ranks first, then shortest instanceId, then alphabetical. The one-shot dedupe script uses the same rule. On a collision a named instance now takes the suffix. - backend/scripts/repair-first-party-labels.ts: dry run by default, --apply writes. The refresh arm keeps a stored non-empty label, so code alone cannot repair rows that already exist. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: wren Agent-Model: claude/claude-haiku-5-5/max --- .../scripts/repairFirstPartyLabels.test.js | 65 +++++++++ ...tIdentityService.collisionResolver.test.js | 41 ++++++ backend/constants/firstPartyDisplayNames.ts | 14 ++ backend/scripts/repair-first-party-labels.ts | 124 ++++++++++++++++++ backend/services/agentIdentityService.ts | 18 ++- docs/agents/agent-runtime-quick-rules.md | 2 +- scripts/dedupe-agent-display-names.ts | 11 +- 7 files changed, 266 insertions(+), 9 deletions(-) create mode 100644 backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js create mode 100644 backend/constants/firstPartyDisplayNames.ts create mode 100644 backend/scripts/repair-first-party-labels.ts diff --git a/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js b/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js new file mode 100644 index 0000000000..5d79519278 --- /dev/null +++ b/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js @@ -0,0 +1,65 @@ +// The planner behind scripts/repair-first-party-labels (TASK-551). Each repair +// rule is pinned on its own, and the rows it must leave alone are pinned too. +const { planLabelRepairs } = require('../../../scripts/repair-first-party-labels'); + +const row = (overrides) => ({ + id: 'u1', + username: 'scout', + agentName: 'scout', + instanceId: 'default', + displayName: 'scout', + ...overrides, +}); + +describe('planLabelRepairs', () => { + it('gives a first-party agent stored under its bare agentName its curated label', () => { + expect(planLabelRepairs([row({ displayName: 'scout' })])).toEqual([ + { id: 'u1', username: 'scout', from: 'scout', to: 'Scout', reason: 'first-party' }, + ]); + }); + + it('fills a blank first-party label', () => { + expect(planLabelRepairs([row({ displayName: '' })])).toEqual([ + expect.objectContaining({ from: '', to: 'Scout', reason: 'first-party' }), + ]); + }); + + it('strips the (Default) suffix from a default-instance row', () => { + expect(planLabelRepairs([ + row({ agentName: 'pixel', username: 'pixel', displayName: 'Pixel (Default)' }), + ])).toEqual([ + { id: 'u1', username: 'pixel', from: 'Pixel (Default)', to: 'Pixel', reason: 'default-suffix' }, + ]); + }); + + it('repairs a first-party row that also carries the (Default) suffix', () => { + expect(planLabelRepairs([row({ displayName: 'Scout (Default)' })])).toEqual([ + expect.objectContaining({ from: 'Scout (Default)', to: 'Scout' }), + ]); + }); + + it('leaves a named instance alone, even when its label ends in (Default)', () => { + expect(planLabelRepairs([ + row({ instanceId: 'scout-demo', displayName: 'Scout (Default)' }), + ])).toEqual([]); + }); + + it('does not strip a suffix that is the whole label', () => { + expect(planLabelRepairs([ + row({ agentName: 'pixel', displayName: ' (Default)' }), + ])).toEqual([]); + }); + + it('leaves a blank label alone for an agent without a curated name', () => { + expect(planLabelRepairs([ + row({ agentName: 'pixel', username: 'pixel', displayName: '' }), + ])).toEqual([]); + }); + + it('is idempotent: correct labels produce no repairs', () => { + expect(planLabelRepairs([ + row({ displayName: 'Scout' }), + row({ id: 'u2', agentName: 'pixel', username: 'pixel', displayName: 'Pixel' }), + ])).toEqual([]); + }); +}); diff --git a/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js b/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js index 81d105bac2..41d5cda8c6 100644 --- a/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js +++ b/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js @@ -504,6 +504,47 @@ describe('inline displayName collision resolver (sticky dedup)', () => { expect(mockSaved[0].botMetadata.displayName).toBe('Pixel'); }); + test('the default instance keeps its bare name when a shorter named peer shares it', async () => { + // 'scout' is shorter than 'default', so it used to outrank the official + // agent and turn its label into "Scout (Default)" in every invite line. + mockPeers = [ + { + _id: 'named-scout', + botMetadata: { displayName: 'Scout', instanceId: 'scout' }, + }, + ]; + await AgentIdentityService.getOrCreateAgentUser('scout', { displayName: 'Scout' }); + expect(mockSaved.length).toBe(1); + expect(mockSaved[0].botMetadata.displayName).toBe('Scout'); + }); + + test('a named instance that collides with the default agent takes the suffix', async () => { + mockPeers = [ + { + _id: 'default-scout', + botMetadata: { displayName: 'Scout', instanceId: 'default' }, + }, + ]; + await AgentIdentityService.getOrCreateAgentUser('scout', { + instanceId: 'scout-demo', + displayName: 'Scout', + }); + expect(mockSaved.length).toBe(1); + expect(mockSaved[0].botMetadata.displayName).toBe('Scout (Scout-Demo)'); + }); + + test('a first-party agent with no displayName is stored under its curated label, not its agentName', async () => { + await AgentIdentityService.getOrCreateAgentUser('scout', {}); + expect(mockSaved.length).toBe(1); + expect(mockSaved[0].botMetadata.displayName).toBe('Scout'); + }); + + test('the first-party label map agrees with the native scout config', () => { + const { FIRST_PARTY_DISPLAY_NAMES } = require('../../../constants/firstPartyDisplayNames'); + const { scoutApp } = require('../../../config/native-agents/scout'); + expect(FIRST_PARTY_DISPLAY_NAMES.get('scout')).toBe(scoutApp.displayName); + }); + test('refresh (reprovision) on existing collision-suffixed name does NOT double-suffix', async () => { mockExisting = { _id: 'reprovision-id', diff --git a/backend/constants/firstPartyDisplayNames.ts b/backend/constants/firstPartyDisplayNames.ts new file mode 100644 index 0000000000..feac6b4ebc --- /dev/null +++ b/backend/constants/firstPartyDisplayNames.ts @@ -0,0 +1,14 @@ +/** + * Curated labels for first-party agents that have no AGENT_TYPES entry. + * + * getOrCreateAgentUser falls back to the bare agentName when a caller passes + * no displayName and the type has no officialDisplayName. Scout has no + * AGENT_TYPES entry, so its User row was stored as "scout" (TASK-551). Each + * value must equal the agent's native-agents displayName; the collision + * resolver test pins scout's entry to backend/config/native-agents/scout.ts. + * + * No imports: the repair script reads this without loading the service. + */ +export const FIRST_PARTY_DISPLAY_NAMES: ReadonlyMap = new Map([ + ['scout', 'Scout'], +]); diff --git a/backend/scripts/repair-first-party-labels.ts b/backend/scripts/repair-first-party-labels.ts new file mode 100644 index 0000000000..7562b3bce7 --- /dev/null +++ b/backend/scripts/repair-first-party-labels.ts @@ -0,0 +1,124 @@ +/** + * repair-first-party-labels — one-shot repair of stored User labels written + * before the resolver knew the first-party name and the default-instance + * rule (TASK-551). + * + * 1. A first-party agent whose label is blank, or is its bare agentName + * ("scout"), gets its curated label ("Scout"). + * 2. A default-instance row carrying the " (Default)" suffix that the + * collision resolver used to add gets the suffix stripped. + * + * Both are exact-match rules and idempotent. Dry run unless --apply. This + * never suffixes a label: a repaired label that now shares a name with + * another agent is printed with its peers, and the operator decides. + * + * Run: node --import tsx backend/scripts/repair-first-party-labels.ts [--apply] + */ +import mongoose from 'mongoose'; +import { FIRST_PARTY_DISPLAY_NAMES } from '../constants/firstPartyDisplayNames'; + +const APPLY = process.argv.includes('--apply'); +const DEFAULT_SUFFIX = ' (Default)'; + +export interface LabelRow { + id: string; + username?: string; + agentName?: string; + instanceId?: string; + displayName?: string; +} + +export interface LabelRepair { + id: string; + username: string; + from: string; + to: string; + reason: 'first-party' | 'default-suffix'; +} + +// Pure, so the rules are pinned without a database. +export function planLabelRepairs(rows: LabelRow[]): LabelRepair[] { + const repairs: LabelRepair[] = []; + for (const row of rows) { + const display = (row.displayName || '').trim(); + const agentName = (row.agentName || '').trim().toLowerCase(); + const instanceId = (row.instanceId || '').trim(); + let to = display; + let reason: LabelRepair['reason'] | null = null; + if (instanceId === 'default' && display.endsWith(DEFAULT_SUFFIX)) { + to = display.slice(0, -DEFAULT_SUFFIX.length).trim(); + reason = 'default-suffix'; + } + const curated = FIRST_PARTY_DISPLAY_NAMES.get(agentName); + if (curated && (!to || to.toLowerCase() === agentName)) { + to = curated; + reason = 'first-party'; + } + if (reason && to && to !== display) { + repairs.push({ id: row.id, username: row.username || '', from: display, to, reason }); + } + } + return repairs; +} + +interface LabelUser { + _id: mongoose.Types.ObjectId; + username?: string; + botMetadata?: { displayName?: string; agentName?: string; instanceId?: string }; +} + +async function main() { + const uri = process.env.MONGO_URI; + if (!uri) { + console.error('MONGO_URI is required'); + process.exit(2); + } + await mongoose.connect(uri); + const User = mongoose.model( + 'User', + new mongoose.Schema({}, { strict: false }), + 'users', + ); + + const users = await User.find({ + $or: [ + { 'botMetadata.agentName': { $in: [...FIRST_PARTY_DISPLAY_NAMES.keys()] } }, + { 'botMetadata.instanceId': 'default', 'botMetadata.displayName': / \(Default\)$/ }, + ], + }) + .select('username botMetadata') + .lean(); + + const repairs = planLabelRepairs(users.map((u) => ({ + id: String(u._id), + username: u.username, + agentName: u.botMetadata?.agentName, + instanceId: u.botMetadata?.instanceId, + displayName: u.botMetadata?.displayName, + }))); + + for (const r of repairs) { + console.log(`${APPLY ? '' : '[dry] '}${r.username}: "${r.from}" → "${r.to}" (${r.reason})`); + // Report, don't resolve: the new label may now be shared with another agent. + const peers = await User.find({ 'botMetadata.displayName': r.to, _id: { $ne: r.id } }) + .select('botMetadata.instanceId') + .lean(); + if (peers.length) { + const holders = peers.map((p) => p.botMetadata?.instanceId || '(none)').join(', '); + console.log(` ! "${r.to}" is also held by instance(s): ${holders}`); + } + if (APPLY) { + await User.updateOne({ _id: r.id }, { $set: { 'botMetadata.displayName': r.to } }); + } + } + const verb = APPLY ? 'repaired' : 'would be repaired (dry run; pass --apply to write)'; + console.log(`\nDone — ${repairs.length} label(s) ${verb}.`); + await mongoose.disconnect(); +} + +if (require.main === module) { + main().catch((err) => { + console.error('Repair failed:', err); + process.exit(1); + }); +} diff --git a/backend/services/agentIdentityService.ts b/backend/services/agentIdentityService.ts index e198276f0b..9d62c1ca6e 100644 --- a/backend/services/agentIdentityService.ts +++ b/backend/services/agentIdentityService.ts @@ -4,6 +4,7 @@ import Pod from '../models/Pod'; import { AgentRegistry } from '../models/AgentRegistry'; import { normalizeAvatarUrl } from './avatarService'; import { assertAgentRuntimeMayJoinPod, assertInstalledAgentMayJoinPod } from './outsiderGrantAgentJoinPolicy'; +import { FIRST_PARTY_DISPLAY_NAMES } from '../constants/firstPartyDisplayNames'; let dbPg: { pool: { query: (sql: string, params?: unknown[]) => Promise<{ rows: unknown[] }> } } | null; try { @@ -358,7 +359,9 @@ interface GetOrCreateOptions { * * Canonical rule (matches the one-shot script — keep these aligned): * - Group claimants by displayName. - * - Canonical = shortest instanceId; alphabetical tiebreak. + * - Canonical = the default instance if one claims the name, else the + * shortest instanceId; alphabetical tiebreak. The official agent keeps + * its bare name, so a named instance is the one that gets suffixed. * - Non-canonical entries get suffixed `" ()"`. * * This call returns the disambiguated name the CURRENT caller should use. @@ -432,6 +435,10 @@ async function resolveCollisionFreeDisplayName( instanceId, ]; const canonical = [...all].sort((a, b) => { + // The default instance ranks first, so it keeps the bare name and a named + // instance takes the suffix. Length alone let a shorter named peer + // ('scout') win and turned the default row into "Scout (Default)" (TASK-551). + if ((a === 'default') !== (b === 'default')) return a === 'default' ? -1 : 1; if (a.length !== b.length) return a.length - b.length; return a.localeCompare(b); })[0]; @@ -515,7 +522,8 @@ class AgentIdentityService { const isOfficial = instanceId === 'default' && !!typeConfig; if (!agentUser) { - const rawDisplayName = options.displayName || typeConfig?.officialDisplayName || resolvedType; + const rawDisplayName = options.displayName || typeConfig?.officialDisplayName + || FIRST_PARTY_DISPLAY_NAMES.get(resolvedType) || resolvedType; const botMetadata = { displayName: await resolveCollisionFreeDisplayName(rawDisplayName, instanceId), // No invented sentence (#1649): a seat without a description shows no line on @@ -577,7 +585,8 @@ class AgentIdentityService { // Upgrade existing user to bot if not already marked agentUser.isBot = true; agentUser.botType = (typeConfig?.botType || options.botType || 'agent') as typeof agentUser.botType; - const upgradeRawDisplayName = options.displayName || typeConfig?.officialDisplayName || agentUser.username; + const upgradeRawDisplayName = options.displayName || typeConfig?.officialDisplayName + || FIRST_PARTY_DISPLAY_NAMES.get(resolvedType) || agentUser.username; agentUser.botMetadata = { displayName: await resolveCollisionFreeDisplayName(upgradeRawDisplayName, instanceId, agentUser._id), description: options.description || typeConfig?.officialDescription || '', @@ -614,7 +623,8 @@ class AgentIdentityService { if (needsUpdate) { const refreshRawDisplayName = hasStoredDisplayName ? String(existingMeta.displayName) - : (options.displayName || typeConfig?.officialDisplayName || resolvedType); + : (options.displayName || typeConfig?.officialDisplayName + || FIRST_PARTY_DISPLAY_NAMES.get(resolvedType) || resolvedType); agentUser.botMetadata = { ...existingMeta, displayName: await resolveCollisionFreeDisplayName(refreshRawDisplayName, instanceId, agentUser._id), diff --git a/docs/agents/agent-runtime-quick-rules.md b/docs/agents/agent-runtime-quick-rules.md index 595e5b2b22..dcc7a74b8d 100644 --- a/docs/agents/agent-runtime-quick-rules.md +++ b/docs/agents/agent-runtime-quick-rules.md @@ -36,7 +36,7 @@ Rule (unchanged): any new social-presence primitive (typing-indicator, read-rece - **Pod-scoped reads are membership-gated; admin moderation is a separate opt-in (PRs #375 / #377 / #378 / #381, 2026-05-15).** The default sidebar/listing endpoints (`getAllPods`, `getPodsByType`) and the generic `getPodById` filter to caller membership for ALL users including admins — admins do NOT bypass on the default surface, or their sidebar leaks every personal DM in the instance. Cross-instance moderation is an explicit `?scope=all` opt-in on `getAllPods` (admin-only; non-admins silently downgrade to `scope=mine`). Personal pod types (`agent-room`, `agent-admin`) 404 non-members on direct GET; `agent-dm` carves out the §3.7 fan-out (PR #381) so humans sharing a pod with either agent participant can navigate to the a2a DM read-only — the V2 inspector "Direct messages" list links there. Pod-scoped read endpoints for content — `/api/posts?podId=`, `/api/posts/:id`, `/api/pods/:id/external-links`, `/api/pods/:id/announcements`, `/api/pods/:id/files`, `/api/pods/:id/children`, `/api/summaries/pod/:id` — all run through `DMService.canViewPod` (members + admins + agent-dm §3.7 fan-out; everyone else 403). Rule for any new pod-scoped read endpoint: call `canViewPod` before returning content. The §3.7 admin-bypass inside `canViewPod` is intentional for ops/debug observability on contents; the default *existence* surface must not advertise other users' DMs. -- **Agent displayName collisions are disambiguated by suffix, not by render-time logic (2026-05-16).** Two agents with the same `botMetadata.displayName` (e.g. `openclaw:helper` and `openclaw:helper-demo` both labeled "Helper") used to render identically in chat — a real attribution risk. Source of truth fix: a one-shot migration appends `()` to the displayName of every non-canonical sibling (canonical = shortest `instanceId`, alphabetical tiebreak — deterministic + idempotent). Script: `scripts/dedupe-agent-display-names.ts`. After this, `resolveAgentDisplayLabel` returns the disambiguated displayName directly — no peer-context plumbing needed at render sites. Rule for any new agent-install path that sets `botMetadata.displayName`: collisions live in DB, not in display logic; re-run the dedup script after bulk imports. +- **Agent displayName collisions are disambiguated by suffix, not by render-time logic (2026-05-16).** Two agents with the same `botMetadata.displayName` (e.g. `openclaw:helper` and `openclaw:helper-demo` both labeled "Helper") used to render identically in chat — a real attribution risk. Source of truth fix: a one-shot migration appends `()` to the displayName of every non-canonical sibling (canonical = the `default` instance if one claims the name, else shortest `instanceId`, alphabetical tiebreak — deterministic + idempotent). Script: `scripts/dedupe-agent-display-names.ts`. After this, `resolveAgentDisplayLabel` returns the disambiguated displayName directly — no peer-context plumbing needed at render sites. Rule for any new agent-install path that sets `botMetadata.displayName`: collisions live in DB, not in display logic; re-run the dedup script after bulk imports. - **DM display labels — never use `botMetadata.agentName`.** For OpenClaw-driven agents the User row stores `agentName: 'openclaw'` (the runtime) and `instanceId` (the actual identity, e.g. `'helper'`). Pod names + `AgentInstallation.displayName` + chat.mention DM cues all resolve via `agentIdentityService.resolveAgentDisplayLabel(user, fallback)` with the chain: `botMetadata.displayName` → `instanceId` (when not 'default') → `username` → fallback. **Never** falls back to `botMetadata.agentName` — that produces "openclaw ↔ openclaw" pod names. The dmService inline fallback duplicates the helper to avoid an import cycle. Sweep script for stale data: `scripts/rename-agent-dm-pods.ts` (also handles `agent-room`). diff --git a/scripts/dedupe-agent-display-names.ts b/scripts/dedupe-agent-display-names.ts index 4d5f15ec03..424bdc28f8 100644 --- a/scripts/dedupe-agent-display-names.ts +++ b/scripts/dedupe-agent-display-names.ts @@ -12,8 +12,8 @@ * What the script does: * 1. Group every bot User by botMetadata.displayName. * 2. For each group with >1 user, pick ONE canonical to keep as-is - * (preference: shortest instanceId, then alphabetically first - * instanceId — deterministic across re-runs). + * (preference: the default instance, then shortest instanceId, then + * alphabetically first instanceId — deterministic across re-runs). * 3. For every other user in the group, rewrite displayName to * ` ()`. instanceId is humanized: * `pixel-demo` → "Pixel-demo", `inst-315667` → "inst-315667". @@ -69,11 +69,14 @@ const isOpaquePerUserToken = (instanceId: string): boolean => ( ); const pickCanonical = (users: BotUser[]): BotUser => { - // Deterministic preference: shortest instanceId wins (so "pixel" beats - // "pixel-demo"); ties resolved alphabetically. Stable across re-runs. + // Deterministic preference: the default instance keeps the bare name, as + // in the inline resolver (TASK-551); then shortest instanceId wins (so + // "pixel" beats "pixel-demo"); ties resolved alphabetically. Stable + // across re-runs. return [...users].sort((a, b) => { const ai = a.botMetadata?.instanceId || ''; const bi = b.botMetadata?.instanceId || ''; + if ((ai === 'default') !== (bi === 'default')) return ai === 'default' ? -1 : 1; if (ai.length !== bi.length) return ai.length - bi.length; return ai.localeCompare(bi); })[0]; From e0ebd98c6a5fca22284650d54fb5d643b4954090 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 30 Oct 2026 15:00:19 -0700 Subject: [PATCH 2/4] docs(scripts): run the label repair with the declared ts-node (TASK-551) The documented command used `node --import tsx`, which neither package manifest declares, so it could not load on an install of the declared dependencies. The backend declares ts-node, and the scrub and migrate runbooks document `npx ts-node` from backend/, so use the same form. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: wren Agent-Model: claude/claude-haiku-5-5/max --- backend/scripts/repair-first-party-labels.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/scripts/repair-first-party-labels.ts b/backend/scripts/repair-first-party-labels.ts index 7562b3bce7..42c47300f3 100644 --- a/backend/scripts/repair-first-party-labels.ts +++ b/backend/scripts/repair-first-party-labels.ts @@ -12,7 +12,7 @@ * never suffixes a label: a repaired label that now shares a name with * another agent is printed with its peers, and the operator decides. * - * Run: node --import tsx backend/scripts/repair-first-party-labels.ts [--apply] + * Run: cd backend && npx ts-node scripts/repair-first-party-labels.ts [--apply] */ import mongoose from 'mongoose'; import { FIRST_PARTY_DISPLAY_NAMES } from '../constants/firstPartyDisplayNames'; From 8d9723a1b102f67a7d69d1f733d075e415b0aa08 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 30 Oct 2026 16:28:25 -0700 Subject: [PATCH 3/4] fix(agents): store Scout's curated label and keep "(Default)" off the default row (TASK-551) A first-party install could be stored under its bare agentName ("scout"), and on a label collision the default instance was suffixed, so the invite line read "Scout (Default)". - FIRST_PARTY_DISPLAY_NAMES (backend/constants/firstPartyDisplayNames.ts) keys the curated labels by agentName: Pod Welcomer, Task Clerk, Pod Summarizer, Scout, Recorder. isFirstPartyAgentName matches case-insensitively. - Default-first on a label collision applies only to first-party agents. Any other agent keeps the shortest-instanceId rule it has on main. The resolver, the dedupe pickCanonical and the quick-rules line all say so. - The upgrade and refresh arms fall back to the curated label. Refresh keeps a stored user-set label such as "My Scout". - backend/scripts/repair-first-party-labels.ts repairs stored rows. It is a dry run unless --apply is passed. A first-party default row whose target label a named peer already holds is skipped and printed, never written. - backend/scripts/dedupe-agent-display-names.ts run line is npx ts-node. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: wren Agent-Model: claude/claude-haiku-5-5/max --- CLAUDE.md | 2 +- .../constants/firstPartyDisplayNames.test.js | 20 +++ .../scripts/dedupeAgentDisplayNames.test.js | 31 +++++ .../scripts/repairFirstPartyLabels.test.js | 109 ++++++++++++++-- ...tIdentityService.collisionResolver.test.js | 89 ++++++++++++- backend/constants/firstPartyDisplayNames.ts | 28 +++- .../scripts}/dedupe-agent-display-names.ts | 36 +++--- backend/scripts/repair-first-party-labels.ts | 120 +++++++++++++----- backend/services/agentIdentityService.ts | 48 ++++--- docs/agents/agent-runtime-quick-rules.md | 2 +- 10 files changed, 390 insertions(+), 95 deletions(-) create mode 100644 backend/__tests__/unit/constants/firstPartyDisplayNames.test.js create mode 100644 backend/__tests__/unit/scripts/dedupeAgentDisplayNames.test.js rename {scripts => backend/scripts}/dedupe-agent-display-names.ts (79%) diff --git a/CLAUDE.md b/CLAUDE.md index 0629b9ea39..5e99a20246 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -218,7 +218,7 @@ Prescriptive rules not derivable from reading the code. Full text, history and e - **Agent reactions are first-class kernel primitives.** `POST /api/messages/:messageId/reactions` accepts human JWTs and agent runtime tokens via `dualAuth`; agents are gated by an active `AgentInstallation`, then `Pod.members`, and both paths fan out the same `messageReaction` socket event. Reactions live in Postgres (`message_reactions`). **Any new social-presence primitive (typing indicator, read receipt, …) MUST take the dual-auth shape** — never gate on `req.userId` alone, or agents are silently excluded. - **codex MCP tools need an env table.** codex does not pass parent env to the MCP child it spawns, so `[mcp_servers.commonly]` must declare `env = { COMMONLY_API_URL, COMMONLY_AGENT_TOKEN }` or the server crashes at boot and no `commonly_*` tools surface. Check this first when tools stop surfacing. - **Pod-scoped reads are membership-gated; admin moderation is a separate opt-in.** `getAllPods`, `getPodsByType` and `getPodById` filter to caller membership for everyone, admins included; `?scope=all` on `getAllPods` is the admin-only opt-in. `agent-room` and `agent-admin` 404 non-members; `agent-dm` allows the §3.7 read-only fan-out. **Every new pod-scoped read endpoint calls `DMService.canViewPod` before returning content.** -- **Agent displayName collisions are disambiguated in the DB, not at render time.** `scripts/dedupe-agent-display-names.ts` suffixes each non-canonical sibling deterministically; re-run it after bulk imports. +- **Agent displayName collisions are disambiguated in the DB, not at render time.** `backend/scripts/dedupe-agent-display-names.ts` suffixes each non-canonical sibling deterministically; re-run it after bulk imports. - **DM display labels — never use `botMetadata.agentName`** (it holds the runtime, e.g. `openclaw`). Labels resolve via `agentIdentityService.resolveAgentDisplayLabel(user, fallback)`: `botMetadata.displayName` → `instanceId` (when not `default`) → `username` → fallback. Stale-data sweep: `backend/scripts/rename-agent-dm-pods.ts`. - **Autonomous a2a DMs:** `commonly_open_dm({ agentName, instanceId? })` returns a podId, then `commonly_post_message(podId, content)` seeds the conversation. `/api/agents/runtime/agent-dm` enforces the §3.7 rule (caller and target must already share a pod). MCP agents reach the same capability as `commonly_dm_agent`. - **A `commonly_*` tool name does NOT identify a capability.** The MCP server and the openclaw extension tool sets diverge, and one name can mean two things: `commonly_update_task` appends a note on MCP and PATCHes task fields (title included) on the extension. "There is no tool for X" describes one runtime's surface; check the route before concluding X is not permitted, and name the runtime you checked. diff --git a/backend/__tests__/unit/constants/firstPartyDisplayNames.test.js b/backend/__tests__/unit/constants/firstPartyDisplayNames.test.js new file mode 100644 index 0000000000..6db8e5e09d --- /dev/null +++ b/backend/__tests__/unit/constants/firstPartyDisplayNames.test.js @@ -0,0 +1,20 @@ +// isFirstPartyAgentName decides which agents take the default-first collision +// rule (TASK-551). It must match every curated agentName and nothing else. +const { isFirstPartyAgentName, FIRST_PARTY_DISPLAY_NAMES } = require('../../../constants/firstPartyDisplayNames'); + +describe('isFirstPartyAgentName', () => { + it('matches every curated first-party agentName, ignoring case and surrounding space', () => { + FIRST_PARTY_DISPLAY_NAMES.forEach((_label, agentName) => { + expect(isFirstPartyAgentName(agentName)).toBe(true); + expect(isFirstPartyAgentName(` ${agentName.toUpperCase()} `)).toBe(true); + }); + }); + + it('does not match a community agentName, a label, or a blank one', () => { + expect(isFirstPartyAgentName('helper')).toBe(false); + expect(isFirstPartyAgentName('scout-demo')).toBe(false); + expect(isFirstPartyAgentName('Scout (Default)')).toBe(false); + expect(isFirstPartyAgentName('')).toBe(false); + expect(isFirstPartyAgentName(undefined)).toBe(false); + }); +}); diff --git a/backend/__tests__/unit/scripts/dedupeAgentDisplayNames.test.js b/backend/__tests__/unit/scripts/dedupeAgentDisplayNames.test.js new file mode 100644 index 0000000000..cb96f1f13b --- /dev/null +++ b/backend/__tests__/unit/scripts/dedupeAgentDisplayNames.test.js @@ -0,0 +1,31 @@ +// pickCanonical in backend/scripts/dedupe-agent-display-names (TASK-551). A +// group whose default instance belongs to a first-party agent ranks that +// instance first; every other group keeps the shortest-instanceId rule it has +// on main. +const { pickCanonical } = require('../../../scripts/dedupe-agent-display-names'); + +const claimant = (instanceId, agentName) => ({ + _id: `${agentName}-${instanceId}`, + botMetadata: { displayName: 'x', instanceId, agentName }, +}); +const winner = (claims) => pickCanonical( + claims.map(([instanceId, agentName]) => claimant(instanceId, agentName)), +).botMetadata.instanceId; + +describe('pickCanonical', () => { + it("ranks a first-party agent's default instance first, even against a shorter named peer", () => { + expect(winner([['scout', 'scout'], ['default', 'scout']])).toBe('default'); + }); + + it('keeps the shortest instanceId for a community agent, so a shorter named peer beats its default', () => { + expect(winner([['default', 'helper'], ['bot', 'helper']])).toBe('bot'); + }); + + it('does not let a community default outrank a shorter first-party peer on the same label', () => { + expect(winner([['default', 'helper'], ['lab', 'scout']])).toBe('lab'); + }); + + it('breaks shortest-instanceId ties alphabetically', () => { + expect(winner([['pixel-b', 'pixel'], ['pixel-a', 'pixel']])).toBe('pixel-a'); + }); +}); diff --git a/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js b/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js index 5d79519278..4691549205 100644 --- a/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js +++ b/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js @@ -1,6 +1,7 @@ // The planner behind scripts/repair-first-party-labels (TASK-551). Each repair -// rule is pinned on its own, and the rows it must leave alone are pinned too. -const { planLabelRepairs } = require('../../../scripts/repair-first-party-labels'); +// 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 } = require('../../../scripts/repair-first-party-labels'); const row = (overrides) => ({ id: 'u1', @@ -14,7 +15,9 @@ const row = (overrides) => ({ describe('planLabelRepairs', () => { it('gives a first-party agent stored under its bare agentName its curated label', () => { expect(planLabelRepairs([row({ displayName: 'scout' })])).toEqual([ - { id: 'u1', username: 'scout', from: 'scout', to: 'Scout', reason: 'first-party' }, + { + id: 'u1', username: 'scout', from: 'scout', to: 'Scout', reason: 'first-party', + }, ]); }); @@ -24,18 +27,30 @@ describe('planLabelRepairs', () => { ]); }); - it('strips the (Default) suffix from a default-instance row', () => { + it('strips the (Default) suffix from a first-party default-instance row', () => { + expect(planLabelRepairs([row({ displayName: 'Scout (Default)' })])).toEqual([ + { + id: 'u1', username: 'scout', from: 'Scout (Default)', to: 'Scout', reason: 'default-suffix', + }, + ]); + }); + + it('leaves a non-first-party default row alone', () => { expect(planLabelRepairs([ row({ agentName: 'pixel', username: 'pixel', displayName: 'Pixel (Default)' }), - ])).toEqual([ - { id: 'u1', username: 'pixel', from: 'Pixel (Default)', to: 'Pixel', reason: 'default-suffix' }, - ]); + ])).toEqual([]); }); - it('repairs a first-party row that also carries the (Default) suffix', () => { - expect(planLabelRepairs([row({ displayName: 'Scout (Default)' })])).toEqual([ - expect.objectContaining({ from: 'Scout (Default)', to: 'Scout' }), - ]); + it('does not strip a (Default) suffix when the stripped label is not the curated label of its agent', () => { + expect(planLabelRepairs([ + row({ displayName: 'My Scout (Default)' }), + ])).toEqual([]); + }); + + it('leaves a (Default) label alone on an agent with no curated name', () => { + expect(planLabelRepairs([ + row({ agentName: 'helper', username: 'helper', displayName: 'Scout (Default)' }), + ])).toEqual([]); }); it('leaves a named instance alone, even when its label ends in (Default)', () => { @@ -46,7 +61,7 @@ describe('planLabelRepairs', () => { it('does not strip a suffix that is the whole label', () => { expect(planLabelRepairs([ - row({ agentName: 'pixel', displayName: ' (Default)' }), + row({ displayName: ' (Default)' }), ])).toEqual([]); }); @@ -59,7 +74,75 @@ describe('planLabelRepairs', () => { it('is idempotent: correct labels produce no repairs', () => { expect(planLabelRepairs([ row({ displayName: 'Scout' }), - row({ id: 'u2', agentName: 'pixel', username: 'pixel', displayName: 'Pixel' }), + row({ + id: 'u2', agentName: 'pixel', username: 'pixel', displayName: 'Pixel', + }), ])).toEqual([]); }); + + 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'] }), + ]); + }); + + 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); + 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([ + row({ + id: 'u1', username: 'scout-a', instanceId: 'u0da521ab41', displayName: 'scout', + }), + row({ + id: 'u2', username: 'scout-b', instanceId: 'uabcdef0123', displayName: '', + }), + ], takenBy)).toEqual([ + { + id: 'u1', username: 'scout-a', from: 'scout', to: 'Scout', reason: 'first-party', + }, + { + id: 'u2', username: 'scout-b', from: '', to: 'Scout', reason: 'first-party', + }, + ]); + }); + + it('holds both non-per-user repairs that would land on one label in the same run', () => { + expect(planLabelRepairs([ + row({ id: 'u1', instanceId: 'default', displayName: 'Scout (Default)' }), + row({ + id: 'u2', username: 'scout-named', instanceId: 'scout', displayName: '', + }), + ])).toEqual([ + expect.objectContaining({ id: 'u1', heldBy: ['scout'] }), + expect.objectContaining({ id: 'u2', heldBy: ['default'] }), + ]); + }); +}); + +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); + 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([ + row({ displayName: 'Scout (Default)' }), + row({ + id: 'u2', username: 'scout-u0da521ab41', instanceId: 'u0da521ab41', displayName: 'scout', + }), + ], new Map([['Scout', ['lab']]])); + const { writable, skipped } = splitRepairs(repairs); + expect(skipped.map((r) => r.id)).toEqual(['u1']); + expect(writable.map((r) => r.id)).toEqual(['u2']); + }); }); diff --git a/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js b/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js index 41d5cda8c6..58d98722a2 100644 --- a/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js +++ b/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js @@ -1,6 +1,6 @@ // Tests for the inline displayName collision resolver in // getOrCreateAgentUser. This is the sticky-dedup path (counterpart to the -// one-shot offline dedup script scripts/dedupe-agent-display-names.ts) — +// one-shot offline dedup script backend/scripts/dedupe-agent-display-names.ts) — // every fresh install / reprovision now disambiguates inline, so a future // reprovision-all run cannot reintroduce a "Pixel" / "Pixel" collision. // @@ -83,6 +83,8 @@ jest.mock('../../../models/User', () => { }); const { default: AgentIdentityService } = require('../../../services/agentIdentityService'); +const { FIRST_PARTY_DISPLAY_NAMES } = require('../../../constants/firstPartyDisplayNames'); +const { FIRST_PARTY_APPS } = require('../../../config/native-agents/apps'); const reset = () => { mockPeers = []; @@ -522,7 +524,7 @@ describe('inline displayName collision resolver (sticky dedup)', () => { mockPeers = [ { _id: 'default-scout', - botMetadata: { displayName: 'Scout', instanceId: 'default' }, + botMetadata: { displayName: 'Scout', instanceId: 'default', agentName: 'scout' }, }, ]; await AgentIdentityService.getOrCreateAgentUser('scout', { @@ -533,16 +535,91 @@ describe('inline displayName collision resolver (sticky dedup)', () => { expect(mockSaved[0].botMetadata.displayName).toBe('Scout (Scout-Demo)'); }); + test('a non-first-party default does not outrank a shorter named peer', async () => { + // Only a first-party agent's default takes the default-first rule. A + // community agent keeps the shortest-instanceId rule it has on main, so + // 'bot' keeps "Helper". + mockPeers = [ + { _id: 'default-helper', botMetadata: { displayName: 'Helper', instanceId: 'default', agentName: 'helper' } }, + ]; + await AgentIdentityService.getOrCreateAgentUser('helper', { + instanceId: 'bot', + displayName: 'Helper', + }); + expect(mockSaved.length).toBe(1); + expect(mockSaved[0].botMetadata.displayName).toBe('Helper'); + }); + + test('a non-first-party default instance takes the suffix when a shorter named peer holds the label', async () => { + mockPeers = [ + { _id: 'bot-helper', botMetadata: { displayName: 'Helper', instanceId: 'bot', agentName: 'helper' } }, + ]; + await AgentIdentityService.getOrCreateAgentUser('helper', { + instanceId: 'default', + displayName: 'Helper', + }); + expect(mockSaved.length).toBe(1); + expect(mockSaved[0].botMetadata.displayName).toBe('Helper (Default)'); + }); + test('a first-party agent with no displayName is stored under its curated label, not its agentName', async () => { await AgentIdentityService.getOrCreateAgentUser('scout', {}); expect(mockSaved.length).toBe(1); expect(mockSaved[0].botMetadata.displayName).toBe('Scout'); }); - test('the first-party label map agrees with the native scout config', () => { - const { FIRST_PARTY_DISPLAY_NAMES } = require('../../../constants/firstPartyDisplayNames'); - const { scoutApp } = require('../../../config/native-agents/scout'); - expect(FIRST_PARTY_DISPLAY_NAMES.get('scout')).toBe(scoutApp.displayName); + test('upgrade of a legacy agent row stores the curated label, not the username (TASK-551)', async () => { + mockExisting = { + _id: 'legacy-scout-row', + username: 'scout', + email: 'scout@agents.commonly.local', + isBot: false, + }; + await AgentIdentityService.getOrCreateAgentUser('scout', {}); + expect(mockSaved).toHaveLength(1); + expect(mockSaved[0].isBot).toBe(true); + expect(mockSaved[0].botMetadata.displayName).toBe('Scout'); + }); + + test('refresh of a first-party row with no stored label stores the curated label (TASK-551)', async () => { + mockExisting = { + _id: 'blank-scout-bot', + username: 'scout', + isBot: true, + botMetadata: { + agentName: 'scout', + instanceId: 'default', + displayName: '', + }, + }; + await AgentIdentityService.getOrCreateAgentUser('scout', {}); + expect(mockSaved).toHaveLength(1); + expect(mockSaved[0].botMetadata.displayName).toBe('Scout'); + }); + + test('refresh keeps a user-set label ("My Scout") over the displayName a routine post sends (TASK-551)', async () => { + // No runtime on the stored row, so the refresh arm runs. A routine post + // sends the curated "Scout"; the label the user set must survive it. + mockExisting = { + _id: 'my-scout-bot', + username: 'scout', + isBot: true, + botMetadata: { + agentName: 'scout', + instanceId: 'default', + displayName: 'My Scout', + }, + }; + await AgentIdentityService.getOrCreateAgentUser('scout', { displayName: 'Scout' }); + expect(mockSaved).toHaveLength(1); + expect(mockSaved[0].botMetadata.displayName).toBe('My Scout'); + }); + + test('the first-party label map agrees with every native first-party app config', () => { + expect(FIRST_PARTY_DISPLAY_NAMES.size).toBe(FIRST_PARTY_APPS.length); + FIRST_PARTY_APPS.forEach((app) => { + expect(FIRST_PARTY_DISPLAY_NAMES.get(app.agentName.toLowerCase())).toBe(app.displayName); + }); }); test('refresh (reprovision) on existing collision-suffixed name does NOT double-suffix', async () => { diff --git a/backend/constants/firstPartyDisplayNames.ts b/backend/constants/firstPartyDisplayNames.ts index feac6b4ebc..ce85ebd582 100644 --- a/backend/constants/firstPartyDisplayNames.ts +++ b/backend/constants/firstPartyDisplayNames.ts @@ -1,14 +1,28 @@ /** - * Curated labels for first-party agents that have no AGENT_TYPES entry. + * Curated labels for the first-party agents in FIRST_PARTY_APPS + * (backend/config/native-agents/apps.ts), keyed by agentName. * - * getOrCreateAgentUser falls back to the bare agentName when a caller passes - * no displayName and the type has no officialDisplayName. Scout has no - * AGENT_TYPES entry, so its User row was stored as "scout" (TASK-551). Each - * value must equal the agent's native-agents displayName; the collision - * resolver test pins scout's entry to backend/config/native-agents/scout.ts. + * Two readers. getOrCreateAgentUser falls back to the curated label when a + * caller gives no displayName and the type has no officialDisplayName. The + * collision resolver and the dedupe script treat an agent in this map as + * first-party (see isFirstPartyAgentName), so its default instance keeps the + * bare name. The collision test pins every value to its native app config. * - * No imports: the repair script reads this without loading the service. + * No imports: the repair and dedupe scripts read this without loading the service. */ export const FIRST_PARTY_DISPLAY_NAMES: ReadonlyMap = new Map([ + ['pod-welcomer', 'Pod Welcomer'], + ['task-clerk', 'Task Clerk'], + ['pod-summarizer', 'Pod Summarizer'], ['scout', 'Scout'], + ['recorder', 'Recorder'], ]); + +/** + * True when an agentName is a first-party app's (a key of this map), compared + * ignoring case and surrounding space. On a label collision, a first-party + * agent's default instance keeps the bare name; every other agent keeps the + * shortest-instanceId rule. + */ +export const isFirstPartyAgentName = (agentName: string): boolean => FIRST_PARTY_DISPLAY_NAMES + .has((agentName || '').trim().toLowerCase()); diff --git a/scripts/dedupe-agent-display-names.ts b/backend/scripts/dedupe-agent-display-names.ts similarity index 79% rename from scripts/dedupe-agent-display-names.ts rename to backend/scripts/dedupe-agent-display-names.ts index 424bdc28f8..27577cabdb 100644 --- a/scripts/dedupe-agent-display-names.ts +++ b/backend/scripts/dedupe-agent-display-names.ts @@ -11,9 +11,10 @@ * * What the script does: * 1. Group every bot User by botMetadata.displayName. - * 2. For each group with >1 user, pick ONE canonical to keep as-is - * (preference: the default instance, then shortest instanceId, then - * alphabetically first instanceId — deterministic across re-runs). + * 2. For each group with >1 user, pick ONE canonical to keep as-is. If the + * group's default instance is a first-party agent (isFirstPartyAgentName) + * it goes first; otherwise the shortest instanceId wins. Ties: alphabetically + * first instanceId. Deterministic across re-runs. * 3. For every other user in the group, rewrite displayName to * ` ()`. instanceId is humanized: * `pixel-demo` → "Pixel-demo", `inst-315667` → "inst-315667". @@ -25,8 +26,8 @@ * keys; only displayName (the human-facing label) is rewritten. * - `username` — DB-unique already; collisions there are impossible. * - * Run with: `node --import tsx scripts/dedupe-agent-display-names.ts` - * or via `kubectl exec ... node /app/scripts/dedupe-agent-display-names.js`. + * Run with: `cd backend && npx ts-node scripts/dedupe-agent-display-names.ts` + * (from backend/, so ts-node and mongoose resolve from backend/node_modules). * * Idempotent: safe to re-run. New duplicates introduced later can be * cleaned up by re-running. @@ -34,6 +35,7 @@ /* eslint-disable no-console */ import mongoose from 'mongoose'; +import { isFirstPartyAgentName } from '../constants/firstPartyDisplayNames'; interface BotMeta { displayName?: string; @@ -68,15 +70,17 @@ const isOpaquePerUserToken = (instanceId: string): boolean => ( /^u[a-f0-9]{10}([a-f0-9]{14})?$/.test((instanceId || '').toLowerCase()) ); -const pickCanonical = (users: BotUser[]): BotUser => { - // Deterministic preference: the default instance keeps the bare name, as - // in the inline resolver (TASK-551); then shortest instanceId wins (so - // "pixel" beats "pixel-demo"); ties resolved alphabetically. Stable - // across re-runs. +// Deterministic pick. A group whose default instance is a first-party agent +// keeps that instance's bare name, as in the inline resolver (TASK-551). Every +// other group: the shortest instanceId wins (so "pixel" beats "pixel-demo"). +// Ties resolved alphabetically. Stable across re-runs. +export const pickCanonical = (users: BotUser[]): BotUser => { + const defaultFirst = users.some((u) => (u.botMetadata?.instanceId || '') === 'default' + && isFirstPartyAgentName(u.botMetadata?.agentName || '')); return [...users].sort((a, b) => { const ai = a.botMetadata?.instanceId || ''; const bi = b.botMetadata?.instanceId || ''; - if ((ai === 'default') !== (bi === 'default')) return ai === 'default' ? -1 : 1; + if (defaultFirst && ((ai === 'default') !== (bi === 'default'))) return ai === 'default' ? -1 : 1; if (ai.length !== bi.length) return ai.length - bi.length; return ai.localeCompare(bi); })[0]; @@ -133,7 +137,9 @@ async function main() { await mongoose.disconnect(); } -main().catch((err) => { - console.error('Migration failed:', err); - process.exit(1); -}); +if (require.main === module) { + main().catch((err) => { + console.error('Migration failed:', err); + process.exit(1); + }); +} diff --git a/backend/scripts/repair-first-party-labels.ts b/backend/scripts/repair-first-party-labels.ts index 42c47300f3..28c0517aee 100644 --- a/backend/scripts/repair-first-party-labels.ts +++ b/backend/scripts/repair-first-party-labels.ts @@ -1,16 +1,26 @@ /** * repair-first-party-labels — one-shot repair of stored User labels written * before the resolver knew the first-party name and the default-instance - * rule (TASK-551). + * rule (TASK-551). Only first-party agents (FIRST_PARTY_DISPLAY_NAMES) are + * touched: * * 1. A first-party agent whose label is blank, or is its bare agentName * ("scout"), gets its curated label ("Scout"). - * 2. A default-instance row carrying the " (Default)" suffix that the - * collision resolver used to add gets the suffix stripped. + * 2. A first-party default-instance row whose label is its curated label plus + * the " (Default)" suffix gets the suffix stripped. The collision rules + * added that suffix to a non-canonical default row. A label on any other + * agent keeps its suffix: the shortest-instanceId rule produces it on + * purpose. * - * Both are exact-match rules and idempotent. Dry run unless --apply. This - * never suffixes a label: a repaired label that now shares a name with - * another agent is printed with its peers, and the operator decides. + * Both are exact-match rules and idempotent. Dry run unless --apply. + * + * A repair is SKIPPED, never written, when its target label is already taken by + * a non-per-user agent, or when two non-per-user repairs in this run would both + * land on it. A skipped row keeps its current label and is printed as skipped. + * Skipping is the decision, not an automatic suffix: which row keeps the bare + * label is an operator call, and a suffix would silently rename a label someone + * may have chosen. Per-user rows (opaque instance tokens) never skip or contest + * a label: every user's Scout shares the bare name by design. * * Run: cd backend && npx ts-node scripts/repair-first-party-labels.ts [--apply] */ @@ -19,6 +29,9 @@ import { FIRST_PARTY_DISPLAY_NAMES } from '../constants/firstPartyDisplayNames'; const APPLY = process.argv.includes('--apply'); const DEFAULT_SUFFIX = ' (Default)'; +// Keep identical to isOpaquePerUserToken (agentIdentityService) and the dedupe script. +const OPAQUE_PER_USER_TOKEN = /^u[a-f0-9]{10}([a-f0-9]{14})?$/; +const isPerUser = (instanceId = ''): boolean => OPAQUE_PER_USER_TOKEN.test(instanceId.toLowerCase()); export interface LabelRow { id: string; @@ -34,33 +47,70 @@ export interface LabelRepair { from: string; to: string; reason: 'first-party' | 'default-suffix'; + // Set only when the write is skipped: the instance(s) already taking `to`, or + // the other repairs in this run that land on it. + heldBy?: string[]; } -// Pure, so the rules are pinned without a database. -export function planLabelRepairs(rows: LabelRow[]): LabelRepair[] { +// Pure, so the rules are pinned without a database. `takenBy` maps a label to +// the instanceIds of every bot already displaying it. +export function planLabelRepairs( + rows: LabelRow[], + takenBy: ReadonlyMap = new Map(), +): LabelRepair[] { const repairs: LabelRepair[] = []; for (const row of rows) { - const display = (row.displayName || '').trim(); + // Only first-party agents are repaired. A community agent's label is never + // rewritten here, whatever it says. const agentName = (row.agentName || '').trim().toLowerCase(); + const curated = FIRST_PARTY_DISPLAY_NAMES.get(agentName); + if (!curated) continue; + const display = (row.displayName || '').trim(); const instanceId = (row.instanceId || '').trim(); let to = display; let reason: LabelRepair['reason'] | null = null; if (instanceId === 'default' && display.endsWith(DEFAULT_SUFFIX)) { - to = display.slice(0, -DEFAULT_SUFFIX.length).trim(); - reason = 'default-suffix'; + const stripped = display.slice(0, -DEFAULT_SUFFIX.length).trim(); + if (stripped.toLowerCase() === curated.toLowerCase()) { + to = curated; + reason = 'default-suffix'; + } } - const curated = FIRST_PARTY_DISPLAY_NAMES.get(agentName); - if (curated && (!to || to.toLowerCase() === agentName)) { + if (!to || to.toLowerCase() === agentName) { to = curated; - reason = 'first-party'; + reason = reason || 'first-party'; } - if (reason && to && to !== display) { + if (reason && to !== display) { repairs.push({ id: row.id, username: row.username || '', from: display, to, reason }); } } + + const instanceOf = new Map( + rows.map((row): [string, string] => [row.id, (row.instanceId || '').trim()]), + ); + 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)'); + const others = shared + .filter((o) => o.id !== r.id && o.to === r.to) + .map((o) => instanceOf.get(o.id) || '(none)'); + const heldBy = [...holders, ...others]; + if (heldBy.length > 0) r.heldBy = heldBy; + } return repairs; } +// The writes --apply makes, and the repairs it skips. A skipped repair is never +// written, so one run cannot put two non-per-user rows on the same label. +export function splitRepairs(repairs: LabelRepair[]): { writable: LabelRepair[]; skipped: LabelRepair[] } { + return { + writable: repairs.filter((r) => r.heldBy === undefined), + skipped: repairs.filter((r) => r.heldBy !== undefined), + }; +} + interface LabelUser { _id: mongoose.Types.ObjectId; username?: string; @@ -80,39 +130,41 @@ async function main() { 'users', ); - const users = await User.find({ - $or: [ - { 'botMetadata.agentName': { $in: [...FIRST_PARTY_DISPLAY_NAMES.keys()] } }, - { 'botMetadata.instanceId': 'default', 'botMetadata.displayName': / \(Default\)$/ }, - ], - }) + // Every bot with a botMetadata block, not only the labelled ones: a blank + // first-party label is a candidate, and a repair is held by whoever already + // takes its target label. + const users = await User.find({ botMetadata: { $exists: true } }) .select('username botMetadata') .lean(); - - const repairs = planLabelRepairs(users.map((u) => ({ + const rows = users.map((u) => ({ id: String(u._id), username: u.username, agentName: u.botMetadata?.agentName, 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 || '']); + } - for (const r of repairs) { + const { writable, skipped } = splitRepairs(planLabelRepairs(rows, takenBy)); + for (const r of skipped) { + const holders = (r.heldBy || []).join(', '); + console.log( + ` ! skipped ${r.username}: "${r.from}" → "${r.to}" (${r.reason}); "${r.to}" is also taken by ${holders}`, + ); + } + for (const r of writable) { console.log(`${APPLY ? '' : '[dry] '}${r.username}: "${r.from}" → "${r.to}" (${r.reason})`); - // Report, don't resolve: the new label may now be shared with another agent. - const peers = await User.find({ 'botMetadata.displayName': r.to, _id: { $ne: r.id } }) - .select('botMetadata.instanceId') - .lean(); - if (peers.length) { - const holders = peers.map((p) => p.botMetadata?.instanceId || '(none)').join(', '); - console.log(` ! "${r.to}" is also held by instance(s): ${holders}`); - } if (APPLY) { await User.updateOne({ _id: r.id }, { $set: { 'botMetadata.displayName': r.to } }); } } const verb = APPLY ? 'repaired' : 'would be repaired (dry run; pass --apply to write)'; - console.log(`\nDone — ${repairs.length} label(s) ${verb}.`); + console.log(`\nDone — ${writable.length} label(s) ${verb}, ${skipped.length} skipped for an operator decision.`); await mongoose.disconnect(); } diff --git a/backend/services/agentIdentityService.ts b/backend/services/agentIdentityService.ts index 9d62c1ca6e..44196f3eec 100644 --- a/backend/services/agentIdentityService.ts +++ b/backend/services/agentIdentityService.ts @@ -4,7 +4,7 @@ import Pod from '../models/Pod'; import { AgentRegistry } from '../models/AgentRegistry'; import { normalizeAvatarUrl } from './avatarService'; import { assertAgentRuntimeMayJoinPod, assertInstalledAgentMayJoinPod } from './outsiderGrantAgentJoinPolicy'; -import { FIRST_PARTY_DISPLAY_NAMES } from '../constants/firstPartyDisplayNames'; +import { FIRST_PARTY_DISPLAY_NAMES, isFirstPartyAgentName } from '../constants/firstPartyDisplayNames'; let dbPg: { pool: { query: (sql: string, params?: unknown[]) => Promise<{ rows: unknown[] }> } } | null; try { @@ -353,16 +353,18 @@ interface GetOrCreateOptions { /** * Disambiguate an agent's displayName at WRITE time so it doesn't collide * with another bot User row's displayName. Mirrors the offline dedup - * script's logic (scripts/dedupe-agent-display-names.ts) but runs inline + * script's logic (backend/scripts/dedupe-agent-display-names.ts) but runs inline * during getOrCreateAgentUser so a fresh install / reprovision can never * (re-)introduce a "Pixel" / "Pixel" collision. * * Canonical rule (matches the one-shot script — keep these aligned): * - Group claimants by displayName. - * - Canonical = the default instance if one claims the name, else the - * shortest instanceId; alphabetical tiebreak. The official agent keeps - * its bare name, so a named instance is the one that gets suffixed. - * - Non-canonical entries get suffixed `" ()"`. + * - First-party agents only (isFirstPartyAgentName, keyed on agentName): the + * default instance keeps the bare name when it claims one, so a named + * instance gets suffixed. + * - Every other agent: the shortest instanceId keeps the bare name. + * - Alphabetical tiebreak. Non-canonical entries get suffixed + * `" ()"`. * * This call returns the disambiguated name the CURRENT caller should use. * It does NOT rewrite peers — for the "new shorter instanceId arrives @@ -400,6 +402,7 @@ export const endsWithParenthetical = (trimmed: string): boolean => { async function resolveCollisionFreeDisplayName( desiredName: string, instanceId: string, + agentName: string, selfUserId?: unknown, ): Promise { const trimmed = (desiredName || '').trim(); @@ -425,20 +428,25 @@ async function resolveCollisionFreeDisplayName( } const peers = await User.find(peerQuery) .select('botMetadata') - .lean>(); + .lean>(); if (peers.length === 0) return trimmed; // Include `self` in the canonical-pick comparison so the rule is stable // regardless of which row writes first. const all = [ - ...peers.map((p) => p.botMetadata?.instanceId || ''), - instanceId, + ...peers.map((p) => ({ + instanceId: p.botMetadata?.instanceId || '', + agentName: p.botMetadata?.agentName || '', + })), + { instanceId, agentName }, ]; - const canonical = [...all].sort((a, b) => { - // The default instance ranks first, so it keeps the bare name and a named - // instance takes the suffix. Length alone let a shorter named peer - // ('scout') win and turned the default row into "Scout (Default)" (TASK-551). - if ((a === 'default') !== (b === 'default')) return a === 'default' ? -1 : 1; + // Default-first applies when the default instance belongs to a first-party + // agent. Length alone let a shorter named peer ('scout') win and turned the + // default row into "Scout (Default)" (TASK-551). Every other agent keeps the + // shortest-instanceId rule it has on main. + const defaultFirst = all.some((c) => c.instanceId === 'default' && isFirstPartyAgentName(c.agentName)); + const canonical = all.map((c) => c.instanceId).sort((a, b) => { + if (defaultFirst && ((a === 'default') !== (b === 'default'))) return a === 'default' ? -1 : 1; if (a.length !== b.length) return a.length - b.length; return a.localeCompare(b); })[0]; @@ -480,7 +488,7 @@ class AgentIdentityService { if (peers.length) { throw new AgentDisplayNameTakenError(); } - const displayName = await resolveCollisionFreeDisplayName(desiredName, instanceId, user._id); + const displayName = await resolveCollisionFreeDisplayName(desiredName, instanceId, resolvedType, user._id); user.botMetadata = { ...user.botMetadata, displayName }; await user.save(); const mirrorSynced = await AgentIdentityService.syncUserToPostgreSQL(user); @@ -525,7 +533,7 @@ class AgentIdentityService { const rawDisplayName = options.displayName || typeConfig?.officialDisplayName || FIRST_PARTY_DISPLAY_NAMES.get(resolvedType) || resolvedType; const botMetadata = { - displayName: await resolveCollisionFreeDisplayName(rawDisplayName, instanceId), + displayName: await resolveCollisionFreeDisplayName(rawDisplayName, instanceId, resolvedType), // No invented sentence (#1649): a seat without a description shows no line on // Your Team, never " agent". The hire flow supplies the persona's sentence. description: options.description || typeConfig?.officialDescription || '', @@ -588,7 +596,9 @@ class AgentIdentityService { const upgradeRawDisplayName = options.displayName || typeConfig?.officialDisplayName || FIRST_PARTY_DISPLAY_NAMES.get(resolvedType) || agentUser.username; agentUser.botMetadata = { - displayName: await resolveCollisionFreeDisplayName(upgradeRawDisplayName, instanceId, agentUser._id), + displayName: await resolveCollisionFreeDisplayName( + upgradeRawDisplayName, instanceId, resolvedType, agentUser._id, + ), description: options.description || typeConfig?.officialDescription || '', icon: typeConfig?.icon || '🤖', runtimeId: options.runtimeId || agentUser.botMetadata?.runtimeId || undefined, @@ -627,7 +637,9 @@ class AgentIdentityService { || FIRST_PARTY_DISPLAY_NAMES.get(resolvedType) || resolvedType); agentUser.botMetadata = { ...existingMeta, - displayName: await resolveCollisionFreeDisplayName(refreshRawDisplayName, instanceId, agentUser._id), + displayName: await resolveCollisionFreeDisplayName( + refreshRawDisplayName, instanceId, resolvedType, agentUser._id, + ), description: options.description || existingMeta.description || typeConfig?.officialDescription || '', icon: existingMeta.icon || typeConfig?.icon || '🤖', runtimeId: options.runtimeId || existingMeta.runtimeId || undefined, diff --git a/docs/agents/agent-runtime-quick-rules.md b/docs/agents/agent-runtime-quick-rules.md index dcc7a74b8d..2f59d38772 100644 --- a/docs/agents/agent-runtime-quick-rules.md +++ b/docs/agents/agent-runtime-quick-rules.md @@ -36,7 +36,7 @@ Rule (unchanged): any new social-presence primitive (typing-indicator, read-rece - **Pod-scoped reads are membership-gated; admin moderation is a separate opt-in (PRs #375 / #377 / #378 / #381, 2026-05-15).** The default sidebar/listing endpoints (`getAllPods`, `getPodsByType`) and the generic `getPodById` filter to caller membership for ALL users including admins — admins do NOT bypass on the default surface, or their sidebar leaks every personal DM in the instance. Cross-instance moderation is an explicit `?scope=all` opt-in on `getAllPods` (admin-only; non-admins silently downgrade to `scope=mine`). Personal pod types (`agent-room`, `agent-admin`) 404 non-members on direct GET; `agent-dm` carves out the §3.7 fan-out (PR #381) so humans sharing a pod with either agent participant can navigate to the a2a DM read-only — the V2 inspector "Direct messages" list links there. Pod-scoped read endpoints for content — `/api/posts?podId=`, `/api/posts/:id`, `/api/pods/:id/external-links`, `/api/pods/:id/announcements`, `/api/pods/:id/files`, `/api/pods/:id/children`, `/api/summaries/pod/:id` — all run through `DMService.canViewPod` (members + admins + agent-dm §3.7 fan-out; everyone else 403). Rule for any new pod-scoped read endpoint: call `canViewPod` before returning content. The §3.7 admin-bypass inside `canViewPod` is intentional for ops/debug observability on contents; the default *existence* surface must not advertise other users' DMs. -- **Agent displayName collisions are disambiguated by suffix, not by render-time logic (2026-05-16).** Two agents with the same `botMetadata.displayName` (e.g. `openclaw:helper` and `openclaw:helper-demo` both labeled "Helper") used to render identically in chat — a real attribution risk. Source of truth fix: a one-shot migration appends `()` to the displayName of every non-canonical sibling (canonical = the `default` instance if one claims the name, else shortest `instanceId`, alphabetical tiebreak — deterministic + idempotent). Script: `scripts/dedupe-agent-display-names.ts`. After this, `resolveAgentDisplayLabel` returns the disambiguated displayName directly — no peer-context plumbing needed at render sites. Rule for any new agent-install path that sets `botMetadata.displayName`: collisions live in DB, not in display logic; re-run the dedup script after bulk imports. +- **Agent displayName collisions are disambiguated by suffix, not by render-time logic (2026-05-16).** Two agents with the same `botMetadata.displayName` (e.g. `openclaw:helper` and `openclaw:helper-demo` both labeled "Helper") used to render identically in chat — a real attribution risk. Source of truth fix: a one-shot migration appends `()` to the displayName of every non-canonical sibling (canonical = for a first-party agent (its `agentName` is a first-party app), the `default` instance if one claims the label; for any other agent, shortest `instanceId`; alphabetical tiebreak — deterministic + idempotent). Script: `backend/scripts/dedupe-agent-display-names.ts`. After this, `resolveAgentDisplayLabel` returns the disambiguated displayName directly — no peer-context plumbing needed at render sites. Rule for any new agent-install path that sets `botMetadata.displayName`: collisions live in DB, not in display logic; re-run the dedup script after bulk imports. - **DM display labels — never use `botMetadata.agentName`.** For OpenClaw-driven agents the User row stores `agentName: 'openclaw'` (the runtime) and `instanceId` (the actual identity, e.g. `'helper'`). Pod names + `AgentInstallation.displayName` + chat.mention DM cues all resolve via `agentIdentityService.resolveAgentDisplayLabel(user, fallback)` with the chain: `botMetadata.displayName` → `instanceId` (when not 'default') → `username` → fallback. **Never** falls back to `botMetadata.agentName` — that produces "openclaw ↔ openclaw" pod names. The dmService inline fallback duplicates the helper to avoid an import cycle. Sweep script for stale data: `scripts/rename-agent-dm-pods.ts` (also handles `agent-room`). From f72ec16987426a4f25923be75eca7cb2ddbcf612 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 30 Oct 2026 17:19:05 -0700 Subject: [PATCH 4/4] fix(agents): scope default-first labels to first-party agents (TASK-551) The default-first rule (a first-party agent's `default` instance keeps the bare label) now applies only to first-party agentNames, in the inline resolver and in the dedupe pickCanonical. Every other agent keeps the shortest-instanceId rule, with an explicit tiebreak on instanceId, then agentName, so row order never decides a tie. - Repair rule 2 ("(Default)" suffix) is scoped to FIRST_PARTY_DISPLAY_NAMES. - --apply skips a first-party default row whose target label a non-opaque peer already holds, prints it as skipped, and never writes a duplicate. - --apply mirrors the new label to Postgres users.username as well as Mongo, so chat reads the repaired label. - The leak check in resolveAgentDisplayLabel applies only to shared-runtime agentNames (openclaw, codex, claude-code), so "Scout (Scout-Demo)" is kept. - Tests pin the upgrade and refresh curated fallbacks, and that a refresh keeps a stored "My Scout". - The quick-rules canonical line states the first-party scoping and the instanceId-then-agentName tiebreak. Co-Authored-By: Claude Haiku 5.5 Co-authored-by: wren Agent-Model: claude/claude-haiku-5-5/max --- .../scripts/dedupeAgentDisplayNames.test.js | 13 +++++++ .../scripts/repairFirstPartyLabels.test.js | 2 +- ...tIdentityService.collisionResolver.test.js | 26 ++++++++++++++ ...tyService.resolveAgentDisplayLabel.test.js | 10 ++++++ backend/scripts/dedupe-agent-display-names.ts | 36 ++++++++++--------- backend/scripts/repair-first-party-labels.ts | 31 ++++++++++++++++ backend/services/agentIdentityService.ts | 32 ++++++++++------- backend/utils/agentDisplayLabel.ts | 9 +++-- docs/agents/agent-runtime-quick-rules.md | 2 +- 9 files changed, 127 insertions(+), 34 deletions(-) diff --git a/backend/__tests__/unit/scripts/dedupeAgentDisplayNames.test.js b/backend/__tests__/unit/scripts/dedupeAgentDisplayNames.test.js index cb96f1f13b..5bff07e0e9 100644 --- a/backend/__tests__/unit/scripts/dedupeAgentDisplayNames.test.js +++ b/backend/__tests__/unit/scripts/dedupeAgentDisplayNames.test.js @@ -11,6 +11,9 @@ const claimant = (instanceId, agentName) => ({ const winner = (claims) => pickCanonical( claims.map(([instanceId, agentName]) => claimant(instanceId, agentName)), ).botMetadata.instanceId; +const winnerId = (claims) => pickCanonical( + claims.map(([instanceId, agentName]) => claimant(instanceId, agentName)), +)._id; describe('pickCanonical', () => { it("ranks a first-party agent's default instance first, even against a shorter named peer", () => { @@ -28,4 +31,14 @@ describe('pickCanonical', () => { it('breaks shortest-instanceId ties alphabetically', () => { expect(winner([['pixel-b', 'pixel'], ['pixel-a', 'pixel']])).toBe('pixel-a'); }); + + it("ranks a first-party default above a community default on the same label, in either row order", () => { + expect(winnerId([['default', 'helper'], ['default', 'scout']])).toBe('scout-default'); + expect(winnerId([['default', 'scout'], ['default', 'helper']])).toBe('scout-default'); + }); + + it('breaks an instanceId tie by agentName, so row order never decides it', () => { + expect(winnerId([['lab', 'helper'], ['lab', 'bot']])).toBe('bot-lab'); + expect(winnerId([['lab', 'bot'], ['lab', 'helper']])).toBe('bot-lab'); + }); }); diff --git a/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js b/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js index 4691549205..23981682ca 100644 --- a/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js +++ b/backend/__tests__/unit/scripts/repairFirstPartyLabels.test.js @@ -1,7 +1,7 @@ // 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 } = require('../../../scripts/repair-first-party-labels'); +const { planLabelRepairs, splitRepairs, mirrorLabel } = require('../../../scripts/repair-first-party-labels'); const row = (overrides) => ({ id: 'u1', diff --git a/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js b/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js index 58d98722a2..35593180ca 100644 --- a/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js +++ b/backend/__tests__/unit/services/agentIdentityService.collisionResolver.test.js @@ -562,6 +562,32 @@ describe('inline displayName collision resolver (sticky dedup)', () => { expect(mockSaved[0].botMetadata.displayName).toBe('Helper (Default)'); }); + test('a first-party default keeps the bare name over a community default on the same label', async () => { + // Both claimants sit on 'default'. Only the first-party one keeps the bare + // name; the community agent's row takes the suffix. + mockPeers = [ + { _id: 'default-scout', botMetadata: { displayName: 'Scout', instanceId: 'default', agentName: 'scout' } }, + ]; + await AgentIdentityService.getOrCreateAgentUser('helper', { + instanceId: 'default', + displayName: 'Scout', + }); + expect(mockSaved.length).toBe(1); + expect(mockSaved[0].botMetadata.displayName).toBe('Scout (Default)'); + }); + + test('two agents on one instanceId do not both keep the bare name', async () => { + mockPeers = [ + { _id: 'bot-lab', botMetadata: { displayName: 'Helper', instanceId: 'lab', agentName: 'bot' } }, + ]; + await AgentIdentityService.getOrCreateAgentUser('helper', { + instanceId: 'lab', + displayName: 'Helper', + }); + expect(mockSaved.length).toBe(1); + expect(mockSaved[0].botMetadata.displayName).toBe('Helper (Lab)'); + }); + test('a first-party agent with no displayName is stored under its curated label, not its agentName', async () => { await AgentIdentityService.getOrCreateAgentUser('scout', {}); expect(mockSaved.length).toBe(1); diff --git a/backend/__tests__/unit/services/agentIdentityService.resolveAgentDisplayLabel.test.js b/backend/__tests__/unit/services/agentIdentityService.resolveAgentDisplayLabel.test.js index 30d86636b9..e0494199b3 100644 --- a/backend/__tests__/unit/services/agentIdentityService.resolveAgentDisplayLabel.test.js +++ b/backend/__tests__/unit/services/agentIdentityService.resolveAgentDisplayLabel.test.js @@ -92,6 +92,16 @@ describe('resolveAgentDisplayLabel', () => { expect(resolveAgentDisplayLabel(user)).toBe('Scout'); }); + // TASK-551: "Scout (Scout-Demo)" is the collision label the resolver writes + // for a named Scout instance. It is the product's own label, not a leak. + it('keeps a first-party collision label that spells its agentName and instanceId', () => { + const user = { + username: 'scout-demo', + botMetadata: { displayName: 'Scout (Scout-Demo)', agentName: 'scout', instanceId: 'scout-demo' }, + }; + expect(resolveAgentDisplayLabel(user)).toBe('Scout (Scout-Demo)'); + }); + // The shared-runtime set is wider than openclaw: a codex seat that takes // its officialDisplayName ('Codex') must still fall back to its instance. it('still falls back to instanceId when a shared codex runtime name leaks as displayName', () => { diff --git a/backend/scripts/dedupe-agent-display-names.ts b/backend/scripts/dedupe-agent-display-names.ts index 27577cabdb..afe9d2b986 100644 --- a/backend/scripts/dedupe-agent-display-names.ts +++ b/backend/scripts/dedupe-agent-display-names.ts @@ -13,8 +13,8 @@ * 1. Group every bot User by botMetadata.displayName. * 2. For each group with >1 user, pick ONE canonical to keep as-is. If the * group's default instance is a first-party agent (isFirstPartyAgentName) - * it goes first; otherwise the shortest instanceId wins. Ties: alphabetically - * first instanceId. Deterministic across re-runs. + * it goes first; otherwise the shortest instanceId wins. Ties on instanceId: + * alphabetically first agentName, then _id. Deterministic across re-runs. * 3. For every other user in the group, rewrite displayName to * ` ()`. instanceId is humanized: * `pixel-demo` → "Pixel-demo", `inst-315667` → "inst-315667". @@ -70,21 +70,23 @@ const isOpaquePerUserToken = (instanceId: string): boolean => ( /^u[a-f0-9]{10}([a-f0-9]{14})?$/.test((instanceId || '').toLowerCase()) ); -// Deterministic pick. A group whose default instance is a first-party agent -// keeps that instance's bare name, as in the inline resolver (TASK-551). Every -// other group: the shortest instanceId wins (so "pixel" beats "pixel-demo"). -// Ties resolved alphabetically. Stable across re-runs. -export const pickCanonical = (users: BotUser[]): BotUser => { - const defaultFirst = users.some((u) => (u.botMetadata?.instanceId || '') === 'default' - && isFirstPartyAgentName(u.botMetadata?.agentName || '')); - return [...users].sort((a, b) => { - const ai = a.botMetadata?.instanceId || ''; - const bi = b.botMetadata?.instanceId || ''; - if (defaultFirst && ((ai === 'default') !== (bi === 'default'))) return ai === 'default' ? -1 : 1; - if (ai.length !== bi.length) return ai.length - bi.length; - return ai.localeCompare(bi); - })[0]; -}; +// Deterministic pick. A first-party agent's default instance keeps the bare +// name over any peer, as in the inline resolver (TASK-551). Every other +// claimant: the shortest instanceId wins (so "pixel" beats "pixel-demo"). An +// instanceId tie falls to agentName, then _id, so the pick never depends on +// the order Mongo returns rows. +const isFirstPartyDefault = (u: BotUser): boolean => (u.botMetadata?.instanceId || '') === 'default' + && isFirstPartyAgentName(u.botMetadata?.agentName || ''); + +export const pickCanonical = (users: BotUser[]): BotUser => [...users].sort((a, b) => { + if (isFirstPartyDefault(a) !== isFirstPartyDefault(b)) return isFirstPartyDefault(a) ? -1 : 1; + const ai = a.botMetadata?.instanceId || ''; + const bi = b.botMetadata?.instanceId || ''; + if (ai.length !== bi.length) return ai.length - bi.length; + return ai.localeCompare(bi) + || (a.botMetadata?.agentName || '').localeCompare(b.botMetadata?.agentName || '') + || String(a._id).localeCompare(String(b._id)); +})[0]; async function main() { const uri = process.env.MONGO_URI; diff --git a/backend/scripts/repair-first-party-labels.ts b/backend/scripts/repair-first-party-labels.ts index 28c0517aee..a4459fffa9 100644 --- a/backend/scripts/repair-first-party-labels.ts +++ b/backend/scripts/repair-first-party-labels.ts @@ -22,6 +22,12 @@ * may have chosen. Per-user rows (opaque instance tokens) never skip or contest * a label: every user's Scout shares the bare name by design. * + * --apply writes each label twice: `botMetadata.displayName` in Mongo, and the + * `users.username` column in PG, which chat renders. syncUserToPostgreSQL writes + * that column from the same displayName. PG goes first, so a run that stops + * between the two writes leaves the Mongo label unrepaired; the next run plans + * the row again and rewrites both. + * * Run: cd backend && npx ts-node scripts/repair-first-party-labels.ts [--apply] */ import mongoose from 'mongoose'; @@ -111,6 +117,24 @@ export function splitRepairs(repairs: LabelRepair[]): { writable: LabelRepair[]; }; } +// The slice of a pg Pool the repair uses, so the tests pass a fake. +interface PgPool { + query: (sql: string, params: unknown[]) => Promise<{ rowCount?: number | null }>; + end: () => Promise; +} + +// 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. +export async function mirrorLabel(pool: PgPool | null, id: string, label: string): Promise { + if (!pool) return false; + const result = await pool.query( + 'UPDATE users SET username = $2, updated_at = $3 WHERE _id = $1', + [id, label, new Date()], + ); + return (result.rowCount || 0) > 0; +} + interface LabelUser { _id: mongoose.Types.ObjectId; username?: string; @@ -157,14 +181,21 @@ async function main() { ` ! skipped ${r.username}: "${r.from}" → "${r.to}" (${r.reason}); "${r.to}" is also taken by ${holders}`, ); } + // eslint-disable-next-line @typescript-eslint/no-require-imports, global-require + const pgPool: PgPool | null = APPLY ? require('../config/db-pg').pool || null : null; + if (APPLY && !pgPool) { + console.log(' PG_HOST is not set: users.username is not mirrored; the next agent sync writes it.'); + } 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 } }); } } 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.`); + if (pgPool) await pgPool.end(); await mongoose.disconnect(); } diff --git a/backend/services/agentIdentityService.ts b/backend/services/agentIdentityService.ts index 44196f3eec..ea07b76d8a 100644 --- a/backend/services/agentIdentityService.ts +++ b/backend/services/agentIdentityService.ts @@ -431,26 +431,32 @@ async function resolveCollisionFreeDisplayName( .lean>(); if (peers.length === 0) return trimmed; - // Include `self` in the canonical-pick comparison so the rule is stable - // regardless of which row writes first. + // Include `current` in the canonical-pick comparison so the rule is stable + // regardless of which row writes first. It sits first, so an exact tie (the + // same agent on the same instance) still keeps the bare name. + const current = { instanceId, agentName }; const all = [ + current, ...peers.map((p) => ({ instanceId: p.botMetadata?.instanceId || '', agentName: p.botMetadata?.agentName || '', })), - { instanceId, agentName }, ]; - // Default-first applies when the default instance belongs to a first-party - // agent. Length alone let a shorter named peer ('scout') win and turned the - // default row into "Scout (Default)" (TASK-551). Every other agent keeps the - // shortest-instanceId rule it has on main. - const defaultFirst = all.some((c) => c.instanceId === 'default' && isFirstPartyAgentName(c.agentName)); - const canonical = all.map((c) => c.instanceId).sort((a, b) => { - if (defaultFirst && ((a === 'default') !== (b === 'default'))) return a === 'default' ? -1 : 1; - if (a.length !== b.length) return a.length - b.length; - return a.localeCompare(b); + // A first-party agent's default instance keeps the bare name over any peer + // (TASK-551). Length alone let a shorter named peer ('scout') win and turned + // the default row into "Scout (Default)". Every other claimant keeps the + // shortest-instanceId rule it has on main. The pick is a claimant, not an + // instanceId: a first-party and a community agent can both sit on 'default', + // and an instanceId tie falls to agentName, so row order never decides it. + const firstPartyDefault = (c: { instanceId: string; agentName: string }): boolean => ( + c.instanceId === 'default' && isFirstPartyAgentName(c.agentName) + ); + const canonical = all.sort((a, b) => { + if (firstPartyDefault(a) !== firstPartyDefault(b)) return firstPartyDefault(a) ? -1 : 1; + if (a.instanceId.length !== b.instanceId.length) return a.instanceId.length - b.instanceId.length; + return a.instanceId.localeCompare(b.instanceId) || a.agentName.localeCompare(b.agentName); })[0]; - if (canonical === instanceId) { + if (canonical === current) { // Caller is the canonical; let them keep the bare name. Peers may // need rewriting via the offline sweep — see comment above. return trimmed; diff --git a/backend/utils/agentDisplayLabel.ts b/backend/utils/agentDisplayLabel.ts index 7fc1a9be9f..8118c29449 100644 --- a/backend/utils/agentDisplayLabel.ts +++ b/backend/utils/agentDisplayLabel.ts @@ -55,16 +55,21 @@ export function resolveAgentDisplayLabel( // ` ()` form and the bare `` form // (e.g. displayName === 'openclaw') for a non-default instance. A default // agent can have a curated label equal to its name ('Pixel'), so bare - // equality alone does not prove a runtime leak there. + // equality alone does not prove a runtime leak there. The ` ()` + // form counts only for a shared runtime: a product agent's collision label + // ("Scout (Scout-Demo)", TASK-551) is its own label, and treating it as a leak + // would show the raw instance id. + const sharedRuntime = SHARED_RUNTIME_AGENT_NAMES.has(agentName.toLowerCase()); const leakedPattern = ( !!display && !!agentName && ( (!!instanceId && instanceId !== 'default' - && SHARED_RUNTIME_AGENT_NAMES.has(agentName.toLowerCase()) + && sharedRuntime && sameName(display, agentName)) || ( !!instanceId + && sharedRuntime && display.toLowerCase() === `${agentName.toLowerCase()} (${instanceId.toLowerCase()})` ) ) diff --git a/docs/agents/agent-runtime-quick-rules.md b/docs/agents/agent-runtime-quick-rules.md index 2f59d38772..196093137e 100644 --- a/docs/agents/agent-runtime-quick-rules.md +++ b/docs/agents/agent-runtime-quick-rules.md @@ -36,7 +36,7 @@ Rule (unchanged): any new social-presence primitive (typing-indicator, read-rece - **Pod-scoped reads are membership-gated; admin moderation is a separate opt-in (PRs #375 / #377 / #378 / #381, 2026-05-15).** The default sidebar/listing endpoints (`getAllPods`, `getPodsByType`) and the generic `getPodById` filter to caller membership for ALL users including admins — admins do NOT bypass on the default surface, or their sidebar leaks every personal DM in the instance. Cross-instance moderation is an explicit `?scope=all` opt-in on `getAllPods` (admin-only; non-admins silently downgrade to `scope=mine`). Personal pod types (`agent-room`, `agent-admin`) 404 non-members on direct GET; `agent-dm` carves out the §3.7 fan-out (PR #381) so humans sharing a pod with either agent participant can navigate to the a2a DM read-only — the V2 inspector "Direct messages" list links there. Pod-scoped read endpoints for content — `/api/posts?podId=`, `/api/posts/:id`, `/api/pods/:id/external-links`, `/api/pods/:id/announcements`, `/api/pods/:id/files`, `/api/pods/:id/children`, `/api/summaries/pod/:id` — all run through `DMService.canViewPod` (members + admins + agent-dm §3.7 fan-out; everyone else 403). Rule for any new pod-scoped read endpoint: call `canViewPod` before returning content. The §3.7 admin-bypass inside `canViewPod` is intentional for ops/debug observability on contents; the default *existence* surface must not advertise other users' DMs. -- **Agent displayName collisions are disambiguated by suffix, not by render-time logic (2026-05-16).** Two agents with the same `botMetadata.displayName` (e.g. `openclaw:helper` and `openclaw:helper-demo` both labeled "Helper") used to render identically in chat — a real attribution risk. Source of truth fix: a one-shot migration appends `()` to the displayName of every non-canonical sibling (canonical = for a first-party agent (its `agentName` is a first-party app), the `default` instance if one claims the label; for any other agent, shortest `instanceId`; alphabetical tiebreak — deterministic + idempotent). Script: `backend/scripts/dedupe-agent-display-names.ts`. After this, `resolveAgentDisplayLabel` returns the disambiguated displayName directly — no peer-context plumbing needed at render sites. Rule for any new agent-install path that sets `botMetadata.displayName`: collisions live in DB, not in display logic; re-run the dedup script after bulk imports. +- **Agent displayName collisions are disambiguated by suffix, not by render-time logic (2026-05-16).** Two agents with the same `botMetadata.displayName` (e.g. `openclaw:helper` and `openclaw:helper-demo` both labeled "Helper") used to render identically in chat — a real attribution risk. Source of truth fix: a one-shot migration appends `()` to the displayName of every non-canonical sibling (canonical = for a first-party agent (its `agentName` is a first-party app), the `default` instance if one claims the label; for any other agent, shortest `instanceId`; alphabetical tiebreak on `instanceId`, then `agentName` — deterministic + idempotent). Script: `backend/scripts/dedupe-agent-display-names.ts`. After this, `resolveAgentDisplayLabel` returns the disambiguated displayName directly — no peer-context plumbing needed at render sites. Rule for any new agent-install path that sets `botMetadata.displayName`: collisions live in DB, not in display logic; re-run the dedup script after bulk imports. - **DM display labels — never use `botMetadata.agentName`.** For OpenClaw-driven agents the User row stores `agentName: 'openclaw'` (the runtime) and `instanceId` (the actual identity, e.g. `'helper'`). Pod names + `AgentInstallation.displayName` + chat.mention DM cues all resolve via `agentIdentityService.resolveAgentDisplayLabel(user, fallback)` with the chain: `botMetadata.displayName` → `instanceId` (when not 'default') → `username` → fallback. **Never** falls back to `botMetadata.agentName` — that produces "openclaw ↔ openclaw" pod names. The dmService inline fallback duplicates the helper to avoid an import cycle. Sweep script for stale data: `scripts/rename-agent-dm-pods.ts` (also handles `agent-room`).