refactor(services): extract agent-ownership predicate to shared helper (closes #4377)

.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.

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) {

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<string | null> {
-    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<boolean> {
     return (await resolveAgentOwnerOrg(userId, agentUrl)) !== null;

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<string | null> {
+  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<boolean> {
+  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;
+  }
+}

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');
+    });
+  });
+});