From d701ef175d900a780d2c6b277b7268358484290d Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 11 May 2026 07:43:26 -0400 Subject: [PATCH] refactor(services): extract agent-ownership predicate to shared helper (closes #4377) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #4250's review flagged the agent-ownership JOIN as a drift surface — duplicated inline in two places with subtly different semantics: - registry-api.ts:4825 `resolveAgentOwnerOrg(userId, agentUrl)` — "any org the user is a member of that owns the agent" - member-tools.ts:3588 (inline) — "the resolved member-context org IS the owning org" (tighter predicate that adds the org_id constraint) Both queries join `member_profiles.agents` against `organization_ memberships` — same canonical relation, different selectivity. This PR extracts both to `server/src/services/agent-ownership.ts`: - `findOwnerOrgForUser(userId, agentUrl): Promise` — registry-api.ts's "discover ownership" use case - `isOrgOwnerOfAgent(orgId, userId, agentUrl): Promise` — member-tools.ts's "confirm specific org" use case Both call sites updated to use the helpers. `resolveAgentOwnerOrg` is retained as a closure-scoped alias inside the registry-api factory so existing call sites don't need to thread the import. Note on active-membership filtering: `organization_memberships` has no status column in this schema — removed members get their row deleted, not status-flipped. Row existence is the membership signal. Documented in the helper file's module doc. Tests (7 passing): - findOwnerOrgForUser returns org_id, null on no match, null on throw - isOrgOwnerOfAgent returns true/false correctly - semantic distinction pinned: findOwnerOrgForUser uses 2 params (no org filter); isOrgOwnerOfAgent uses 3 params (org_id constraint) Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/extract-agent-ownership-helper.md | 4 + server/src/addie/mcp/member-tools.ts | 29 ++--- server/src/routes/registry-api.ts | 31 +---- server/src/services/agent-ownership.ts | 90 ++++++++++++++ server/tests/unit/agent-ownership.test.ts | 116 +++++++++++++++++++ 5 files changed, 225 insertions(+), 45 deletions(-) create mode 100644 .changeset/extract-agent-ownership-helper.md create mode 100644 server/src/services/agent-ownership.ts create mode 100644 server/tests/unit/agent-ownership.test.ts diff --git a/.changeset/extract-agent-ownership-helper.md b/.changeset/extract-agent-ownership-helper.md new file mode 100644 index 0000000000..f4811e02f5 --- /dev/null +++ b/.changeset/extract-agent-ownership-helper.md @@ -0,0 +1,4 @@ +--- +--- + +Closes #4377. Extracts the agent-ownership predicate into `server/src/services/agent-ownership.ts` so the two distinct semantic uses share one source. `findOwnerOrgForUser(userId, agentUrl)` (registry-api.ts pattern: "any owning org") and `isOrgOwnerOfAgent(orgId, userId, agentUrl)` (member-tools.ts pattern: "this specific org confirmed as owner") both call the same base JOIN with different constraints. Eliminates the inline JOIN copies that PR #4250's review flagged as a drift surface. 7 unit tests pin the semantics + null-on-error behavior. diff --git a/server/src/addie/mcp/member-tools.ts b/server/src/addie/mcp/member-tools.ts index 055f951c2b..c7d308392d 100644 --- a/server/src/addie/mcp/member-tools.ts +++ b/server/src/addie/mcp/member-tools.ts @@ -66,6 +66,7 @@ import { MemberDatabase } from '../../db/member-db.js'; import { ensureMemberProfileExists } from '../../services/member-profile-autopublish.js'; import { updateBrandIdentity, BrandIdentityError } from '../../services/brand-identity.js'; import { canonicalizeBrandDomain } from '../../services/identifier-normalization.js'; +import { isOrgOwnerOfAgent } from '../../services/agent-ownership.js'; import { getBrandPrimaryDomain } from '../../services/brand-domain-resolver.js'; import { ComplianceDatabase } from '../../db/compliance-db.js'; import { AgentSnapshotDatabase } from '../../db/agent-snapshot-db.js'; @@ -3577,28 +3578,18 @@ export function createMemberToolHandlers( // Record result when the user has an org context for this agent. if (organizationId) { // Write to canonical compliance tables when the calling org owns this agent. - // Mirrors resolveAgentOwnerOrg (registry-api.ts:4733) — joins organization_memberships - // to verify the acting user is still an active member of the owning org. - // Non-owner runs skip the canonical write and fall through to the legacy - // agent_test_history path below. + // isOrgOwnerOfAgent verifies the resolved member-context org IS the + // owning org (tighter than findOwnerOrgForUser, which would accept any + // org the user is a member of). Non-owner runs skip the canonical + // write and fall through to the legacy agent_test_history path below. const workosUserId = memberContext?.workos_user?.workos_user_id; let isAgentOwner = false; if (workosUserId) { - try { - const ownerCheck = await query( - `SELECT 1 FROM member_profiles mp - JOIN organization_memberships om - ON om.workos_organization_id = mp.workos_organization_id - WHERE mp.workos_organization_id = $1 - AND mp.agents @> $2::jsonb - AND om.workos_user_id = $3 - LIMIT 1`, - [organizationId, JSON.stringify([{ url: resolved.resolvedUrl }]), workosUserId], - ); - isAgentOwner = ownerCheck.rows.length > 0; - } catch (ownerCheckError) { - logger.warn({ ownerCheckError }, 'evaluate_agent_quality: owner check failed, skipping canonical write'); - } + isAgentOwner = await isOrgOwnerOfAgent( + organizationId, + workosUserId, + resolved.resolvedUrl, + ); } if (isAgentOwner) { diff --git a/server/src/routes/registry-api.ts b/server/src/routes/registry-api.ts index ad3bc2cc0a..727bbdaf8f 100644 --- a/server/src/routes/registry-api.ts +++ b/server/src/routes/registry-api.ts @@ -9,6 +9,7 @@ import { Router } from "express"; import type { RequestHandler } from "express"; import { z } from "zod"; import escapeHtml from "escape-html"; +import { findOwnerOrgForUser } from "../services/agent-ownership.js"; import { CreativeAgentClient, SingleAgentClient, exchangeClientCredentials, ClientCredentialsExchangeError } from "@adcp/sdk"; import { runStoryboardStep, getComplianceStoryboardById, getFirstStepPreview, testCapabilityDiscovery, resolveStoryboardsForCapabilities, loadComplianceIndex } from "@adcp/sdk/testing"; import type { Agent, AgentType, AgentWithStats } from "../types.js"; @@ -4813,32 +4814,10 @@ export function createRegistryApiRouter(config: RegistryApiConfig): Router { const complianceWriteMiddleware = authMiddleware ? [authMiddleware] : []; - /** - * Resolve the workos_organization_id of the org that owns this agent, - * for the authenticated user. Returns null if the user is not a member - * of any org whose member_profile lists the agent (403 case). - * - * Mirrors the query driving the `auth-status` endpoint so the org id the - * UI surfaces ("Auth configured via OAuth") is the one we consult for - * Test-your-agent credentials. - */ - async function resolveAgentOwnerOrg(userId: string, agentUrl: string): Promise { - try { - const result = await query<{ workos_organization_id: string }>( - `SELECT mp.workos_organization_id - FROM member_profiles mp - JOIN organization_memberships om - ON om.workos_organization_id = mp.workos_organization_id - WHERE mp.agents @> $1::jsonb - AND om.workos_user_id = $2 - LIMIT 1`, - [JSON.stringify([{ url: agentUrl }]), userId], - ); - return result.rows[0]?.workos_organization_id ?? null; - } catch { - return null; - } - } + // `resolveAgentOwnerOrg` is now a thin alias for the shared helper. The + // closure-scoped alias is kept so existing call sites inside this factory + // don't need to thread the import. + const resolveAgentOwnerOrg = findOwnerOrgForUser; async function verifyAgentOwnership(userId: string, agentUrl: string): Promise { return (await resolveAgentOwnerOrg(userId, agentUrl)) !== null; diff --git a/server/src/services/agent-ownership.ts b/server/src/services/agent-ownership.ts new file mode 100644 index 0000000000..e6a372ffdc --- /dev/null +++ b/server/src/services/agent-ownership.ts @@ -0,0 +1,90 @@ +/** + * Agent-ownership helpers — single source of truth for "who owns this agent." + * + * The query has two distinct semantic uses: + * + * 1. `findOwnerOrgForUser(userId, agentUrl)` — "what org owns this agent + * for this user?" Returns the org_id (or null) for ANY org the user + * is a member of that has the agent in its member_profile. Used by + * route handlers gating per-agent operations (refresh, applicable- + * storyboards, run-storyboard) on ownership. + * + * 2. `isOrgOwnerOfAgent(orgId, userId, agentUrl)` — "is THIS specific + * org the one that owns the agent for this user?" Tighter predicate + * than (1): requires the resolved org context to match the agent's + * owning org. Used by `evaluate_agent_quality`'s canonical-write + * gate where the calling-context org is known and must be confirmed + * as the owner (not "some org the user belongs to"). + * + * Both queries join `member_profiles.agents` against `organization_memberships` + * — the canonical ownership relation. The two-helper pattern exists because + * inlining the JOIN at every call site is a drift surface (PR #4250 review + * flagged the duplication); a single shared helper keeps the predicate in + * one place. + * + * Note on active-membership filtering: `organization_memberships` has no + * status column in this schema — removed members get their row deleted, not + * status-flipped. Row existence is the membership signal. + */ + +import { query } from '../db/client.js'; + +/** + * Find the org id of any org the user is a member of that owns the agent. + * Returns null if no such org exists (user is not the owner, or anonymous). + * + * Used for permission checks where we don't yet know which org context the + * caller is acting from — the resolver discovers it via the join. + */ +export async function findOwnerOrgForUser( + userId: string, + agentUrl: string, +): Promise { + try { + const result = await query<{ workos_organization_id: string }>( + `SELECT mp.workos_organization_id + FROM member_profiles mp + JOIN organization_memberships om + ON om.workos_organization_id = mp.workos_organization_id + WHERE mp.agents @> $1::jsonb + AND om.workos_user_id = $2 + LIMIT 1`, + [JSON.stringify([{ url: agentUrl }]), userId], + ); + return result.rows[0]?.workos_organization_id ?? null; + } catch { + return null; + } +} + +/** + * Verify the given org is the org that owns the agent for the given user. + * Tighter than `findOwnerOrgForUser` — requires the org_id in the calling + * context (e.g., the resolved member-context organization) to match the + * agent's owning org. + * + * Used by canonical-state writers (owner-test path in evaluate_agent_quality) + * to ensure the acting principal's resolved org is actually the owner before + * persisting public-state changes. + */ +export async function isOrgOwnerOfAgent( + orgId: string, + userId: string, + agentUrl: string, +): Promise { + try { + const result = await query( + `SELECT 1 FROM member_profiles mp + JOIN organization_memberships om + ON om.workos_organization_id = mp.workos_organization_id + WHERE mp.workos_organization_id = $1 + AND mp.agents @> $2::jsonb + AND om.workos_user_id = $3 + LIMIT 1`, + [orgId, JSON.stringify([{ url: agentUrl }]), userId], + ); + return result.rows.length > 0; + } catch { + return false; + } +} diff --git a/server/tests/unit/agent-ownership.test.ts b/server/tests/unit/agent-ownership.test.ts new file mode 100644 index 0000000000..5919e043b4 --- /dev/null +++ b/server/tests/unit/agent-ownership.test.ts @@ -0,0 +1,116 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; + +vi.mock('../../src/db/client.js', () => ({ + query: vi.fn(), +})); + +import { query } from '../../src/db/client.js'; +import { findOwnerOrgForUser, isOrgOwnerOfAgent } from '../../src/services/agent-ownership.js'; + +const queryMock = vi.mocked(query); + +describe('agent-ownership', () => { + beforeEach(() => { + queryMock.mockReset(); + }); + + describe('findOwnerOrgForUser', () => { + it('returns the org_id when the user owns the agent through some org', async () => { + queryMock.mockResolvedValueOnce({ + rows: [{ workos_organization_id: 'org_abc' }], + rowCount: 1, + command: 'SELECT', + oid: 0, + fields: [], + } as never); + + const result = await findOwnerOrgForUser('user_123', 'https://agent.example.com/mcp'); + expect(result).toBe('org_abc'); + expect(queryMock).toHaveBeenCalledOnce(); + const [sql, params] = queryMock.mock.calls[0]; + expect(sql).toContain('member_profiles mp'); + expect(sql).toContain('organization_memberships om'); + expect(params).toEqual([ + JSON.stringify([{ url: 'https://agent.example.com/mcp' }]), + 'user_123', + ]); + }); + + it('returns null when the user is not a member of any owning org', async () => { + queryMock.mockResolvedValueOnce({ + rows: [], + rowCount: 0, + command: 'SELECT', + oid: 0, + fields: [], + } as never); + const result = await findOwnerOrgForUser('user_123', 'https://agent.example.com/mcp'); + expect(result).toBeNull(); + }); + + it('returns null when the query throws', async () => { + queryMock.mockRejectedValueOnce(new Error('connection refused')); + const result = await findOwnerOrgForUser('user_123', 'https://agent.example.com/mcp'); + expect(result).toBeNull(); + }); + }); + + describe('isOrgOwnerOfAgent', () => { + it('returns true when the specific org owns the agent for the user', async () => { + queryMock.mockResolvedValueOnce({ + rows: [{ '?column?': 1 }], + rowCount: 1, + command: 'SELECT', + oid: 0, + fields: [], + } as never); + const result = await isOrgOwnerOfAgent('org_abc', 'user_123', 'https://agent.example.com/mcp'); + expect(result).toBe(true); + const [sql, params] = queryMock.mock.calls[0]; + expect(sql).toContain('mp.workos_organization_id = $1'); + expect(params).toEqual([ + 'org_abc', + JSON.stringify([{ url: 'https://agent.example.com/mcp' }]), + 'user_123', + ]); + }); + + it('returns false when the resolved org is not the owning org', async () => { + queryMock.mockResolvedValueOnce({ + rows: [], + rowCount: 0, + command: 'SELECT', + oid: 0, + fields: [], + } as never); + const result = await isOrgOwnerOfAgent('org_wrong', 'user_123', 'https://agent.example.com/mcp'); + expect(result).toBe(false); + }); + + it('returns false when the query throws', async () => { + queryMock.mockRejectedValueOnce(new Error('query failed')); + const result = await isOrgOwnerOfAgent('org_abc', 'user_123', 'https://agent.example.com/mcp'); + expect(result).toBe(false); + }); + }); + + describe('semantic distinction between the two helpers', () => { + it('findOwnerOrgForUser discovers ownership; isOrgOwnerOfAgent confirms a specific org', async () => { + // Two distinct queries: findOwnerOrgForUser returns ANY owning org for + // the user; isOrgOwnerOfAgent requires the named org to BE the owner. + // The SQL shapes differ — registry-api.ts and member-tools.ts had + // distinct inline copies of these (drift surface) before extraction. + queryMock.mockResolvedValueOnce({ rows: [{ workos_organization_id: 'org_a' }], rowCount: 1, command: 'SELECT', oid: 0, fields: [] } as never); + await findOwnerOrgForUser('user_1', 'https://x/mcp'); + const [findSql, findParams] = queryMock.mock.calls[0]; + expect(findParams).toHaveLength(2); + expect(findSql).not.toContain('mp.workos_organization_id = $1'); + + queryMock.mockResolvedValueOnce({ rows: [{ '?column?': 1 }], rowCount: 1, command: 'SELECT', oid: 0, fields: [] } as never); + await isOrgOwnerOfAgent('org_a', 'user_1', 'https://x/mcp'); + const [isSql, isParams] = queryMock.mock.calls[1]; + expect(isParams).toHaveLength(3); + expect(isSql).toContain('mp.workos_organization_id = $1'); + }); + }); +});