mirror of
https://github.com/qwibitai/nanoclaw.git
synced 2026-07-12 19:00:58 +08:00
refactor(agent-to-agent): drop named-approver list from v1
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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 <agent-group-id> --to <agent-group-id> [--approvers <user-id,user-id>]. 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 <agent-group-id> --to <agent-group-id>.',
|
||||
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: {
|
||||
|
||||
@@ -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)
|
||||
);
|
||||
|
||||
@@ -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: {
|
||||
|
||||
@@ -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. */
|
||||
|
||||
@@ -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();
|
||||
|
||||
@@ -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<void> {
|
||||
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;
|
||||
|
||||
+1
-2
@@ -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;
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user