From d61a6c8e3ceb82704c3d146e08e0fedd6dbed142 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 2 Sep 2026 05:20:04 -0700 Subject: [PATCH 1/4] fix(install): AgentInstallation is canonical for displayName (TASK-032) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sam ruled TASK-032's fork on 2026-09-02: the installation owns a display name; `User.botMetadata.displayName` is a seed for an installation that has none, never a preference over one that does. This is the code half โ€” the schema doc is #1097. Two sites in `routes/registry/install.ts` preferred the curated User row: - `effectiveDisplayName` never consulted AgentInstallation at all, so a reinstall re-seeded the per-pod label from the shared User row and a name curated for THIS pod silently reverted. Precedence is now explicit caller > this pod's existing installation > User-row seed > registry default. - the intro-post label re-queried `User.botMetadata` and put it ahead of `installation.displayName`. That query is dropped: the installation row was resolved through the full chain a few lines above, so the second lookup could only disagree with the row just written. Task #62 / PR #408 is not reinstated. The User-row seed still beats the registry default, so installing an existing identity into a NEW pod โ€” where no installation exists โ€” still writes "Aria" rather than the manifest default to both AgentInstallation.displayName and AgentProfile.name. Only the both-are-curated case changes, and there the per-pod value wins, which is what the read paths (agentMessageService:1461, dmService:555) already do. The reachable shape is uninstall-then-reinstall, not install-twice: an active row 400s at the route, so a second install meets an existing row only on the reactivation path, where ADR-001 ยง3 identity continuity preserves it. The new test covers exactly that and fails against main's install.ts. Co-Authored-By: Claude Opus 5 --- .../install.preserves-displayname.test.js | 62 ++++++++++++ backend/routes/registry/install.ts | 96 +++++++++++-------- 2 files changed, 120 insertions(+), 38 deletions(-) diff --git a/backend/__tests__/service/install.preserves-displayname.test.js b/backend/__tests__/service/install.preserves-displayname.test.js index cc1d1380f..504c9fdc3 100644 --- a/backend/__tests__/service/install.preserves-displayname.test.js +++ b/backend/__tests__/service/install.preserves-displayname.test.js @@ -238,4 +238,66 @@ describe('Install endpoint preserves curated displayName (cycle-of-Aria regressi // Registry default is acceptable when there's no curated identity to preserve. expect(installation.displayName).toBe('Cuz ๐Ÿฆž'); }); + + // TASK-032, ruled 2026-09-02: AgentInstallation is canonical; the User row + // is a seed for an installation that has no name, never a preference over + // one that does. Before the ruling this case reverted a per-pod label to + // the shared identity's label on every re-install, while the read paths + // (agentMessageService, dmService) already preferred the installation. + // + // This is the discriminating case for that precedence and nothing else in + // the file reaches it: every other test starts from an empty + // AgentInstallation collection, where seed and preference are the same + // value and the two orderings agree. + // + // The reachable shape is uninstall-then-reinstall, not install-twice: an + // ACTIVE row makes the route 400 ('Agent already installed in this pod'), + // so the only way a second install meets an existing row is the + // reactivation path โ€” where ADR-001 ยง3 identity continuity keeps the row + // (and its curated name) across the uninstall. + it('keeps an existing installation displayName over the curated User row on re-install', async () => { + await AgentInstallation.create({ + agentName: 'openclaw', + podId: pod._id, + instanceId: 'aria', + installedBy: installer._id, + version: '1.0.0', + status: 'uninstalled', + scopes: ['context:read'], + // Curated for THIS pod, and deliberately different from the User row's + // 'Aria' so the assertion cannot pass by coincidence. + displayName: 'Aria (Sprint Desk)', + }); + + const res = await request(app) + .post('/api/registry/install') + .set('Authorization', `Bearer ${installerToken}`) + .send({ + agentName: 'openclaw', + instanceId: 'aria', + podId: pod._id.toString(), + version: '1.0.0', + scopes: ['context:read', 'summaries:read', 'messages:write'], + // No explicit displayName โ€” the caller is re-installing, not renaming. + }); + + expect(res.status).toBe(200); + + const installation = await AgentInstallation.findOne({ + podId: pod._id, + agentName: 'openclaw', + instanceId: 'aria', + }).lean(); + expect(installation.displayName).toBe('Aria (Sprint Desk)'); + // The two values the old precedence would have written instead. + expect(installation.displayName).not.toBe('Aria'); + expect(installation.displayName).not.toBe('Cuz ๐Ÿฆž'); + + const profile = await AgentProfile.findOne({ + podId: pod._id, + agentName: 'openclaw', + instanceId: 'aria', + }).lean(); + expect(profile.name).toBe('Aria (Sprint Desk)'); + }); }); diff --git a/backend/routes/registry/install.ts b/backend/routes/registry/install.ts index b3576d9f3..082ab5971 100644 --- a/backend/routes/registry/install.ts +++ b/backend/routes/registry/install.ts @@ -418,20 +418,54 @@ installRouter.post('/install', installRateLimit, auth, async (req: any, res: any ...AUTO_GRANTED_INTEGRATION_SCOPES, ])); - // Task #62 (round 2): prefer the curated User.botMetadata.displayName - // for the SAME agentName + instanceId over the registry-default - // (`agent.displayName` e.g. "Cuz ๐Ÿฆž" / "Codex"). PR #408 fixed this - // seam on the intro-post path; this is the install-path equivalent. - // Without this, installing an existing agent identity (e.g. openclaw:nova) - // into a NEW pod writes "Cuz ๐Ÿฆž" to both AgentInstallation.displayName - // AND AgentProfile.name โ€” and the V2 member list reads AgentProfile FIRST, - // so users see "Cuz" on every member row even though the underlying - // identity has the right name. Order: explicit caller intent > existing - // identity > registry default. Resolve the existing identity through the - // same leak guard every display surface uses: historical rows may contain - // a runtime-shaped displayName such as "openclaw (nova)", which must not - // become a higher-precedence installation or profile label. + // TASK-032, ruled 2026-09-02: AgentInstallation is canonical for a + // display name. `User.botMetadata.displayName` is a SEED for an + // installation that has none โ€” never a preference over one that does. + // + // Precedence: explicit caller intent > this pod's existing installation + // > the curated identity on the User row > registry default + // (`agent.displayName` e.g. "Cuz ๐Ÿฆž" / "Codex"). + // + // The installation term is the new one and it is what the ruling adds. + // Without it a re-install re-seeded the per-pod label from the shared + // User row, so a name curated for THIS pod silently reverted โ€” while + // the read paths (agentMessageService, dmService) already preferred the + // installation precisely to stop a sibling pod's name leaking through. + // + // Task #62 / PR #408 is NOT reinstated by this reordering: the User-row + // seed still beats the registry default, so installing an existing + // identity (e.g. openclaw:nova) into a NEW pod โ€” where there is no + // installation yet โ€” still writes "Aria" rather than "Cuz ๐Ÿฆž" to both + // AgentInstallation.displayName and AgentProfile.name. What changes is + // only the case where BOTH are curated, and there the per-pod value wins. + // The seed is still read through the same leak guard every display + // surface uses: historical rows may contain a runtime-shaped displayName + // such as "openclaw (nova)", which must not become a higher-precedence + // installation or profile label. let effectiveDisplayName: string = displayName || ''; + if (!effectiveDisplayName) { + try { + const existingInstallation = await AgentInstallation.findOne({ + agentName: agent.agentName, + podId, + instanceId: normalizedInstanceId, + }).select('displayName').lean() as { displayName?: string } | null; + if (existingInstallation?.displayName) { + effectiveDisplayName = existingInstallation.displayName; + } + } catch (lookupErr) { + // Non-fatal โ€” fall through to the User-row seed below. Logged with + // the agent identity so an operator chasing "the name reverted on + // re-install" can correlate the attempt, in the CodeQL-safe shape + // used by the sibling handlers (identifiers as arguments, not in + // the format-string slot). + console.warn('[install] installation displayName lookup failed', { + agent: agent.agentName, + instance: normalizedInstanceId, + error: (lookupErr as Error).message, + }); + } + } if (!effectiveDisplayName) { try { const existingAgentUser = await User.findOne({ @@ -609,31 +643,17 @@ installRouter.post('/install', installRateLimit, auth, async (req: any, res: any // getOrCreateAgentUser. If we pass the AgentRegistry default // ("Cuz ๐Ÿฆž" / "Codex"), the User row's curated per-instance // displayName ("Aria") gets clobbered or sticky-dedup-suffixed - // to "Cuz ๐Ÿฆž (Aria)". Resolve the intro display label by - // preferring the live agent identity (User.botMetadata.displayName) - // first, so we never overwrite a curated label with a registry default. - let displayName: string; - try { - const existingBot = await User.findOne({ - isBot: true, - 'botMetadata.agentName': agent.agentName, - 'botMetadata.instanceId': normalizedInstanceId, - }).select('botMetadata').lean() as { botMetadata?: { displayName?: string } } | null; - displayName = existingBot?.botMetadata?.displayName - || installation.displayName - || agent.displayName; - } catch (lookupErr: unknown) { - // Fall back to the legacy chain if the identity lookup blew up โ€” - // don't take down the intro flow on a transient mongo hiccup. - // Log with agent identity for operator correlation. - // Same CodeQL-safe shape as the install-path log above. - console.warn('[install] intro displayName lookup failed', { - agent: agent.agentName, - instance: normalizedInstanceId, - error: (lookupErr as Error).message, - }); - displayName = installation.displayName || agent.displayName; - } + // to "Cuz ๐Ÿฆž (Aria)". + // + // This used to re-query User.botMetadata and prefer it. Under + // TASK-032's ruling the installation is canonical, and + // `installation.displayName` was resolved a few lines above through + // the full precedence chain (explicit > installation > User-row seed + // > registry default) โ€” so it already carries the curated label and + // the second lookup could only disagree with the row we just wrote. + // Reading it here keeps the intro post and the member list on one + // source of truth, and drops a per-install User query. + const displayName: string = installation.displayName || agent.displayName; const blurb = (agent.description || '').trim().replace(/\s+/g, ' '); // Skip the blurb when it just repeats the name (the publish step in // older CLI versions seeded description from displayName, producing From 50c5f85055be1e6b37419855662363685bd142b5 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 2 Sep 2026 16:16:52 -0700 Subject: [PATCH 2/4] fix(install): stringify podId in the installation displayName lookup MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL flags the re-install lookup at `install.ts` because `podId` reaches the query straight from the request body: a JSON object there would be interpreted as a query operator rather than a value. The route already 404s on `Pod.findById(podId)` above, so this was not reachable in practice โ€” but the guard is upstream and invisible at the query, and the same file already spells the safe form at `pod: String(podId)`. Coercing at the query makes the operator shape unconstructable regardless of what the upstream guard does later, which is the cheaper invariant. Co-Authored-By: Claude Opus 5 --- backend/routes/registry/install.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/routes/registry/install.ts b/backend/routes/registry/install.ts index 082ab5971..3604cc3aa 100644 --- a/backend/routes/registry/install.ts +++ b/backend/routes/registry/install.ts @@ -447,7 +447,7 @@ installRouter.post('/install', installRateLimit, auth, async (req: any, res: any try { const existingInstallation = await AgentInstallation.findOne({ agentName: agent.agentName, - podId, + podId: String(podId), instanceId: normalizedInstanceId, }).select('displayName').lean() as { displayName?: string } | null; if (existingInstallation?.displayName) { From f1148645dc3e91c13c5d02bc41f6d63fe20c530c Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 2 Sep 2026 16:23:21 -0700 Subject: [PATCH 3/4] fix(install): reject a non-string podId at the entry point MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review ran the CodeQL alert rather than reading it and the false-positive premise it was dismissed on does not hold. The premise was that `Pod.findById(podId)` + 404 above already rejects anything that is not a real id. Mongoose casts an operator object in a filter position rather than throwing, so `Pod.findById({ $ne: null })` matches the first pod in the collection and the 404 never fires. That matters more than the one line the alert names, because `podId` reaches four Mongoose filters on this route unchanged โ€” `Pod.findById`, the already-installed `AgentInstallation.findOne`, the installation displayName lookup, and the `AgentProfile.findOneAndUpdate` upsert KEY. Coercing at one query left the other three. Guard at the entry point instead, in the shape `agentName` already uses directly above. `undefined`/`null` still fall through to the existing 404 and the self-serve 400, so no legitimate path changes status code. The `String(podId)` from the previous commit stays: it is the form CodeQL recognises as a barrier at the site it flagged. The new suite carries the premise as an explicit positive control, so a later refactor cannot turn it green for the wrong reason. Without the guard the operator object gets past the 404 and the request 500s, so the observable behaviour today is a crash rather than a cross-pod write. Co-Authored-By: Claude Opus 5 --- .../install.podid-operator-injection.test.js | 135 ++++++++++++++++++ backend/routes/registry/install.ts | 13 ++ 2 files changed, 148 insertions(+) create mode 100644 backend/__tests__/service/install.podid-operator-injection.test.js diff --git a/backend/__tests__/service/install.podid-operator-injection.test.js b/backend/__tests__/service/install.podid-operator-injection.test.js new file mode 100644 index 000000000..2dc8e771c --- /dev/null +++ b/backend/__tests__/service/install.podid-operator-injection.test.js @@ -0,0 +1,135 @@ +/** + * Regression test for /api/registry/install accepting a query operator as + * `podId`. + * + * CodeQL flagged the `AgentInstallation.findOne` at install.ts and the alert + * was first read as a false positive, on the premise that `Pod.findById(podId)` + * + 404 above it already rejects anything that is not a real id. It does not. + * Mongoose casts an operator object in a filter position rather than throwing, + * so `Pod.findById({ $ne: null })` MATCHES the first pod in the collection and + * the 404 never fires โ€” the raw object then reaches four Mongoose filters on + * this route, one of which (`AgentProfile.findOneAndUpdate`) is an upsert key. + * + * The first test is the positive control for that premise: it asserts the + * upstream guard really does match, so a later refactor cannot quietly turn + * this suite green for the wrong reason. + */ + +const express = require('express'); +const request = require('supertest'); +const jwt = require('jsonwebtoken'); + +const { setupMongoDb, closeMongoDb } = require('../utils/testUtils'); + +const User = require('../../models/User'); +const Pod = require('../../models/Pod'); +const { AgentRegistry, AgentInstallation } = require('../../models/AgentRegistry'); +const AgentProfile = require('../../models/AgentProfile').default || require('../../models/AgentProfile'); + +const registryRoutes = require('../../routes/registry'); + +const JWT_SECRET = 'test-jwt-secret-install-podid-operator'; + +jest.setTimeout(60000); + +describe('Install endpoint rejects a query operator as podId', () => { + let app; + let installer; + let installerToken; + let pod; + + beforeAll(async () => { + process.env.JWT_SECRET = JWT_SECRET; + await setupMongoDb(); + + app = express(); + app.use(express.json()); + app.use('/api/registry', registryRoutes); + + installer = await User.create({ + username: 'operator-installer', + email: 'operator-installer@test.com', + password: 'password123', + entitlements: { cloudAgents: true }, + }); + installerToken = jwt.sign({ id: installer._id.toString() }, JWT_SECRET); + + pod = await Pod.create({ + name: 'Operator-Injection Test Pod', + type: 'chat', + createdBy: installer._id, + members: [installer._id], + }); + + await AgentRegistry.create({ + agentName: 'openclaw', + displayName: 'Cuz ๐Ÿฆž', + description: 'OpenClaw test', + manifest: { + name: 'openclaw', + version: '1.0.0', + capabilities: [], + context: { required: [], optional: [] }, + }, + latestVersion: '1.0.0', + versions: [{ version: '1.0.0', manifest: { name: 'openclaw', version: '1.0.0', capabilities: [], context: { required: [], optional: [] } }, publishedAt: new Date() }], + registry: 'private', + }); + }); + + afterAll(async () => { + await closeMongoDb(); + }); + + beforeEach(async () => { + await AgentInstallation.deleteMany({}); + await AgentProfile.deleteMany({}); + }); + + // Positive control for the premise the fix rests on. If this ever starts + // throwing or returning null, the entry-point guard below is still correct + // but this suite is no longer testing the thing it claims to. + it('control: Pod.findById with an operator object matches rather than 404ing', async () => { + const matched = await Pod.findById({ $ne: null }).lean(); + expect(matched).toBeTruthy(); + expect(String(matched._id)).toBe(String(pod._id)); + }); + + it('400s on an operator object instead of letting it reach the Mongoose filters', async () => { + const res = await request(app) + .post('/api/registry/install') + .set('Authorization', `Bearer ${installerToken}`) + .send({ + agentName: 'openclaw', + instanceId: 'aria', + podId: { $ne: null }, + version: '1.0.0', + scopes: ['context:read'], + }); + + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/podId must be a string/); + + // Nothing may have been written on the way to the rejection โ€” the + // AgentProfile write is an upsert keyed on podId, so a leak here would + // create a row rather than merely read one. + expect(await AgentInstallation.countDocuments({})).toBe(0); + expect(await AgentProfile.countDocuments({})).toBe(0); + }); + + it('still installs normally when podId is the ordinary id string', async () => { + const res = await request(app) + .post('/api/registry/install') + .set('Authorization', `Bearer ${installerToken}`) + .send({ + agentName: 'openclaw', + instanceId: 'aria', + podId: pod._id.toString(), + version: '1.0.0', + scopes: ['context:read'], + }); + + expect(res.status).toBe(200); + expect(await AgentInstallation.countDocuments({ podId: pod._id })).toBe(1); + }); +}); diff --git a/backend/routes/registry/install.ts b/backend/routes/registry/install.ts index 3604cc3aa..64a2502ce 100644 --- a/backend/routes/registry/install.ts +++ b/backend/routes/registry/install.ts @@ -133,6 +133,19 @@ installRouter.post('/install', installRateLimit, auth, async (req: any, res: any return res.status(400).json({ error: 'Invalid agentName: must match /^(@[a-z0-9-]+\\/)?[a-z0-9-]+$/' }); } + // podId reaches four Mongoose filters on this route unchanged + // (`Pod.findById` below, the already-installed `AgentInstallation.findOne`, + // the installation displayName lookup, and the `AgentProfile` upsert key). + // Mongoose does not reject an operator object in a filter position โ€” a + // `{$ne: null}` here casts cleanly and MATCHES, so the 404 below is not a + // guard against one. Reject a non-string at the entry point, the same + // shape agentName already uses above, so no query below can be handed a + // query operator. `undefined` is left to fall through to the existing + // 404 / self-serve 400 so no legitimate path changes status code. + if (podId !== undefined && podId !== null && typeof podId !== 'string') { + return res.status(400).json({ error: 'podId must be a string' }); + } + // Fetched once and reused for the #609 owner-scoping decision below and // the cloud-entitlement gate further down. const installerUser = await User.findById(userId).select('role entitlements isBot').lean(); From 86e5ea9e0589fe4c42f4397ab2cca2aba4796132 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Wed, 2 Sep 2026 16:54:56 -0700 Subject: [PATCH 4/4] docs(install): state the podId guard's coverage as a position, not a count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review enumerated the route and found six Mongoose filters where the comment claimed four, plus one the comment miscategorised: the installation displayName lookup is already `String()`-coerced, so it was never a raw site. Following the helpers the number is larger again โ€” `AgentInstallation.install` is a `findOne` AND a `create`, `ensureAgentInPod` is a third `Pod.findById`, and `postMessage` fans podId out further. So the count was the wrong instrument: it is a claim about how far the reader followed the value, with no natural stopping point, and it decays on the next refactor. The guard is at the entry point and returns before all of them, so the property that holds is positional. Both comment sites restated the four, so both are corrected. Comments only โ€” no executable line changes. 8/8 across the two suites. Co-Authored-By: Claude Opus 5 --- .../install.podid-operator-injection.test.js | 6 +++-- backend/routes/registry/install.ts | 23 ++++++++++++------- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/backend/__tests__/service/install.podid-operator-injection.test.js b/backend/__tests__/service/install.podid-operator-injection.test.js index 2dc8e771c..bf4cefb6c 100644 --- a/backend/__tests__/service/install.podid-operator-injection.test.js +++ b/backend/__tests__/service/install.podid-operator-injection.test.js @@ -7,8 +7,10 @@ * + 404 above it already rejects anything that is not a real id. It does not. * Mongoose casts an operator object in a filter position rather than throwing, * so `Pod.findById({ $ne: null })` MATCHES the first pod in the collection and - * the 404 never fires โ€” the raw object then reaches four Mongoose filters on - * this route, one of which (`AgentProfile.findOneAndUpdate`) is an upsert key. + * the 404 never fires โ€” the raw object then reaches every Mongoose filter + * below, one of which (`AgentProfile.findOneAndUpdate`) is an upsert key. + * The route's own comment says why that set is stated as a position rather + * than a count: podId also flows through helpers which query on it again. * * The first test is the positive control for that premise: it asserts the * upstream guard really does match, so a later refactor cannot quietly turn diff --git a/backend/routes/registry/install.ts b/backend/routes/registry/install.ts index 64a2502ce..d718132f7 100644 --- a/backend/routes/registry/install.ts +++ b/backend/routes/registry/install.ts @@ -133,15 +133,22 @@ installRouter.post('/install', installRateLimit, auth, async (req: any, res: any return res.status(400).json({ error: 'Invalid agentName: must match /^(@[a-z0-9-]+\\/)?[a-z0-9-]+$/' }); } - // podId reaches four Mongoose filters on this route unchanged - // (`Pod.findById` below, the already-installed `AgentInstallation.findOne`, - // the installation displayName lookup, and the `AgentProfile` upsert key). // Mongoose does not reject an operator object in a filter position โ€” a - // `{$ne: null}` here casts cleanly and MATCHES, so the 404 below is not a - // guard against one. Reject a non-string at the entry point, the same - // shape agentName already uses above, so no query below can be handed a - // query operator. `undefined` is left to fall through to the existing - // 404 / self-serve 400 so no legitimate path changes status code. + // `{$ne: null}` here casts cleanly and MATCHES, so the `Pod.findById` + // 404 below is not a guard against one. Reject a non-string at the entry + // point, the same shape agentName already uses above, so no query below + // can be handed a query operator. `undefined` is left to fall through to + // the existing 404 / self-serve 400 so no legitimate path changes status + // code. + // + // Deliberately no count of the filters downstream. podId reaches them + // both directly and through helpers that fan it out again โ€” + // `AgentInstallation.install` (a `findOne` AND a `create`), + // `AgentIdentityService.ensureAgentInPod` (a third `Pod.findById`), + // `AgentMessageService.postMessage` โ€” so any number is a claim about how + // far the reader followed the value, and it decays on the next refactor. + // The property that actually holds is positional: this returns before + // every one of them. if (podId !== undefined && podId !== null && typeof podId !== 'string') { return res.status(400).json({ error: 'podId must be a string' }); }