From b2160a56aa2d18655b90cf6091e91f10b66e92f2 Mon Sep 17 00:00:00 2001 From: Moshe Krupper Date: Wed, 17 Jun 2026 12:03:54 +0300 Subject: [PATCH] refactor(agent-to-agent): drop named-approver list from v1 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address PR review: remove the `approvers` option entirely for v1 — the approver is always the target group's admins/owners. Drops the `approvers` DB column, the `--approvers` flag + its set-time validation, the now-unused `approverUserIds` param on requestApproval, and the related tests. The target-scoped approver pick (`approverAgentGroupId`) stays. Named approvers can be re-added later via a migration when needed. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/cli/resources/policies.ts | 31 ++----------------- .../module-agent-message-policies.ts | 3 +- src/modules/agent-to-agent/agent-route.ts | 1 - .../db/agent-message-policies.ts | 23 ++++---------- .../agent-to-agent/message-gate.test.ts | 29 ++++++----------- src/modules/approvals/primitive.ts | 7 ++--- src/types.ts | 3 +- 7 files changed, 22 insertions(+), 75 deletions(-) diff --git a/src/cli/resources/policies.ts b/src/cli/resources/policies.ts index 3efc4f101..0bcd5d054 100644 --- a/src/cli/resources/policies.ts +++ b/src/cli/resources/policies.ts @@ -1,18 +1,7 @@ import { getAgentGroup } from '../../db/agent-groups.js'; -import { hasAdminPrivilege } from '../../modules/permissions/db/user-roles.js'; import { removeMessagePolicy, setMessagePolicy } from '../../modules/agent-to-agent/db/agent-message-policies.js'; import { registerResource } from '../crud.js'; -/** Parse `--approvers` (comma-separated user-ids) into a list, or null when omitted. */ -function parseApprovers(raw: unknown): string[] | null { - if (raw === undefined || raw === null || raw === '') return null; - const ids = String(raw) - .split(',') - .map((s) => s.trim()) - .filter(Boolean); - return ids.length > 0 ? ids : null; -} - registerResource({ name: 'policy', plural: 'policies', @@ -23,11 +12,6 @@ registerResource({ columns: [ { name: 'from_agent_group_id', type: 'string', description: 'Source agent group. References agent_groups.id.' }, { name: 'to_agent_group_id', type: 'string', description: 'Target agent group. References agent_groups.id.' }, - { - name: 'approvers', - type: 'string', - description: 'JSON array of user-ids allowed to approve. Empty/NULL = the target group’s admins/owners.', - }, { name: 'created_at', type: 'string', description: 'Auto-set.' }, ], operations: { list: 'open' }, @@ -35,7 +19,7 @@ registerResource({ set: { access: 'approval', description: - 'Require approval for messages from one agent to another. Use --from --to [--approvers ]. Named approvers must be admins/owners of the target.', + 'Require approval (by the target’s admins/owners) for messages from one agent to another. Use --from --to .', handler: async (args) => { const from = args.from as string; const to = args.to as string; @@ -45,17 +29,8 @@ registerResource({ if (!getAgentGroup(from)) throw new Error(`source agent group not found: ${from}`); if (!getAgentGroup(to)) throw new Error(`target agent group not found: ${to}`); - const approvers = parseApprovers(args.approvers); - if (approvers) { - for (const userId of approvers) { - if (!hasAdminPrivilege(userId, to)) { - throw new Error(`approver "${userId}" is not an admin/owner of the target agent group`); - } - } - } - - setMessagePolicy(from, to, approvers ? JSON.stringify(approvers) : null, new Date().toISOString()); - return { from_agent_group_id: from, to_agent_group_id: to, approvers: approvers ?? null }; + setMessagePolicy(from, to, new Date().toISOString()); + return { from_agent_group_id: from, to_agent_group_id: to }; }, }, remove: { diff --git a/src/db/migrations/module-agent-message-policies.ts b/src/db/migrations/module-agent-message-policies.ts index 08a4ac668..303511b49 100644 --- a/src/db/migrations/module-agent-message-policies.ts +++ b/src/db/migrations/module-agent-message-policies.ts @@ -5,7 +5,7 @@ import type { Migration } from './index.js'; /** * Per-message approval gate on an agent-to-agent connection. A row gates * messages from→to (PK enforces one per direction); no row = free flow. - * `approvers` is a JSON array of user-ids, NULL = the target's admins. + * Approver is always the target's admins/owners in v1. */ export const moduleAgentMessagePolicies: Migration = { version: 17, @@ -15,7 +15,6 @@ export const moduleAgentMessagePolicies: Migration = { CREATE TABLE agent_message_policies ( from_agent_group_id TEXT NOT NULL REFERENCES agent_groups(id), to_agent_group_id TEXT NOT NULL REFERENCES agent_groups(id), - approvers TEXT, created_at TEXT NOT NULL, PRIMARY KEY (from_agent_group_id, to_agent_group_id) ); diff --git a/src/modules/agent-to-agent/agent-route.ts b/src/modules/agent-to-agent/agent-route.ts index 5305a27f6..f71740b65 100644 --- a/src/modules/agent-to-agent/agent-route.ts +++ b/src/modules/agent-to-agent/agent-route.ts @@ -237,7 +237,6 @@ export async function routeAgentMessage(msg: RoutableAgentMessage, session: Sess agentName: sourceName, action: A2A_MESSAGE_GATE_ACTION, approverAgentGroupId: targetAgentGroupId, - approverUserIds: policy.approvers ? (JSON.parse(policy.approvers) as string[]) : undefined, title: 'Message approval', question: buildGateQuestion(sourceName, targetName, msg.content), payload: { diff --git a/src/modules/agent-to-agent/db/agent-message-policies.ts b/src/modules/agent-to-agent/db/agent-message-policies.ts index 7e5ce0c02..919970603 100644 --- a/src/modules/agent-to-agent/db/agent-message-policies.ts +++ b/src/modules/agent-to-agent/db/agent-message-policies.ts @@ -12,26 +12,15 @@ export function getMessagePolicy(fromAgentGroupId: string, toAgentGroupId: strin .get(fromAgentGroupId, toAgentGroupId) as AgentMessagePolicy | undefined; } -/** Upsert a policy for `from → to`. `approvers` is a JSON array string, or null. */ -export function setMessagePolicy( - fromAgentGroupId: string, - toAgentGroupId: string, - approvers: string | null, - createdAt: string, -): void { +/** Upsert (idempotent) a gate for `from → to`. */ +export function setMessagePolicy(fromAgentGroupId: string, toAgentGroupId: string, createdAt: string): void { getDb() .prepare( - `INSERT INTO agent_message_policies (from_agent_group_id, to_agent_group_id, approvers, created_at) - VALUES (@from_agent_group_id, @to_agent_group_id, @approvers, @created_at) - ON CONFLICT (from_agent_group_id, to_agent_group_id) - DO UPDATE SET approvers = excluded.approvers`, + `INSERT INTO agent_message_policies (from_agent_group_id, to_agent_group_id, created_at) + VALUES (@from_agent_group_id, @to_agent_group_id, @created_at) + ON CONFLICT (from_agent_group_id, to_agent_group_id) DO NOTHING`, ) - .run({ - from_agent_group_id: fromAgentGroupId, - to_agent_group_id: toAgentGroupId, - approvers, - created_at: createdAt, - }); + .run({ from_agent_group_id: fromAgentGroupId, to_agent_group_id: toAgentGroupId, created_at: createdAt }); } /** Remove the policy for `from → to`. Returns true if a row was deleted. */ diff --git a/src/modules/agent-to-agent/message-gate.test.ts b/src/modules/agent-to-agent/message-gate.test.ts index b0d195a1c..fe4112247 100644 --- a/src/modules/agent-to-agent/message-gate.test.ts +++ b/src/modules/agent-to-agent/message-gate.test.ts @@ -100,13 +100,12 @@ describe('agent message policies', () => { it('set / get / remove round-trip', () => { expect(getMessagePolicy(A, B)).toBeUndefined(); - setMessagePolicy(A, B, null, now()); - expect(getMessagePolicy(A, B)).toMatchObject({ from_agent_group_id: A, to_agent_group_id: B, approvers: null }); + setMessagePolicy(A, B, now()); + expect(getMessagePolicy(A, B)).toMatchObject({ from_agent_group_id: A, to_agent_group_id: B }); expect(policyCount()).toBe(1); - // Upsert overwrites approvers without inserting a duplicate row. - setMessagePolicy(A, B, JSON.stringify(['slack:dana']), now()); - expect(JSON.parse(getMessagePolicy(A, B)!.approvers!)).toEqual(['slack:dana']); + // Idempotent upsert — no duplicate row. + setMessagePolicy(A, B, now()); expect(policyCount()).toBe(1); expect(removeMessagePolicy(A, B)).toBe(true); @@ -126,7 +125,7 @@ describe('agent message policies', () => { }); it('policy present → holds the message (no route) and requests approval scoped to the target', async () => { - setMessagePolicy(A, B, null, now()); + setMessagePolicy(A, B, now()); await routeAgentMessage( { id: 'm2', platform_id: B, content: JSON.stringify({ text: 'sensitive' }), in_reply_to: null }, @@ -144,18 +143,8 @@ describe('agent message policies', () => { expect(JSON.parse(String(opts.payload.content)).text).toBe('sensitive'); }); - it('policy with named approvers passes them through to requestApproval', async () => { - setMessagePolicy(A, B, JSON.stringify(['slack:dana']), now()); - await routeAgentMessage( - { id: 'm3', platform_id: B, content: JSON.stringify({ text: 'x' }), in_reply_to: null }, - SA, - ); - const opts = vi.mocked(requestApproval).mock.calls[0][0]; - expect(opts.approverUserIds).toEqual(['slack:dana']); - }); - it('self-message is never gated even if a policy row somehow exists', async () => { - setMessagePolicy(A, A, null, now()); // pathological, but must be ignored + setMessagePolicy(A, A, now()); // pathological, but must be ignored await routeAgentMessage( { id: 'self', platform_id: A, content: JSON.stringify({ text: 'note' }), in_reply_to: null }, SA, @@ -184,14 +173,14 @@ describe('agent message policies', () => { // ── ghost-gate cleanup ── it('deleting the connection drops its policy', () => { - setMessagePolicy(A, B, null, now()); + setMessagePolicy(A, B, now()); deleteDestination(A, 'b'); // removes the A→B agent destination expect(getMessagePolicy(A, B)).toBeUndefined(); }); it('deleteAllDestinationsTouching drops policies on both sides', () => { - setMessagePolicy(A, B, null, now()); - setMessagePolicy(B, A, null, now()); + setMessagePolicy(A, B, now()); + setMessagePolicy(B, A, now()); deleteAllDestinationsTouching(A); expect(getMessagePolicy(A, B)).toBeUndefined(); expect(getMessagePolicy(B, A)).toBeUndefined(); diff --git a/src/modules/approvals/primitive.ts b/src/modules/approvals/primitive.ts index 205aab751..f8afab32f 100644 --- a/src/modules/approvals/primitive.ts +++ b/src/modules/approvals/primitive.ts @@ -199,8 +199,6 @@ export interface RequestApprovalOptions { question: string; /** Pick approvers from this group instead of the session's, and stamp it on the row for click-auth. Defaults to the session's group. */ approverAgentGroupId?: string; - /** Explicit approver user-ids, used verbatim instead of `pickApprover` when non-empty. */ - approverUserIds?: string[]; } /** @@ -212,10 +210,9 @@ export interface RequestApprovalOptions { export async function requestApproval(opts: RequestApprovalOptions): Promise { const { session, action, payload, title, question, agentName } = opts; - // Explicit user list wins; else pick from the approver group (defaults to the session's). + // Pick from the approver group (defaults to the session's own group). const approverGroupId = opts.approverAgentGroupId ?? session.agent_group_id; - const approvers = - opts.approverUserIds && opts.approverUserIds.length > 0 ? opts.approverUserIds : pickApprover(approverGroupId); + const approvers = pickApprover(approverGroupId); if (approvers.length === 0) { notifyAgent(session, `${action} failed: no owner or admin configured to approve.`); return; diff --git a/src/types.ts b/src/types.ts index a28521862..d0adaf018 100644 --- a/src/types.ts +++ b/src/types.ts @@ -216,10 +216,9 @@ export interface AgentDestination { created_at: string; } -/** Directed per-message approval gate; `approvers` is a JSON user-id array (NULL = target admins). */ +/** Directed per-message approval gate; a row's existence requires approval (approver = target admins). */ export interface AgentMessagePolicy { from_agent_group_id: string; to_agent_group_id: string; - approvers: string | null; created_at: string; }