mirror of
https://github.com/qwibitai/nanoclaw.git
synced 2026-06-24 18:31:31 +08:00
refactor(approvals): carry approver on a pending_approvals column, not the payload
Per review: move the assigned approver from the approval payload to a dedicated `approver_user_id` column on pending_approvals. - New migration adds the column; createPendingApproval + requestApproval write it. - isAuthorizedApprovalClick reads approval.approver_user_id directly (drops the payload-parsing helper); when set, only that exact user may resolve. - The gate no longer stuffs `approver` into the payload. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -16,6 +16,7 @@ import { migration015 } from './015-cli-scope.js';
|
||||
import { migration016 } from './016-messaging-group-instance.js';
|
||||
import { moduleApprovalsPendingApprovals } from './module-approvals-pending-approvals.js';
|
||||
import { moduleApprovalsTitleOptions } from './module-approvals-title-options.js';
|
||||
import { moduleApprovalsApprover } from './module-approvals-approver.js';
|
||||
|
||||
export interface Migration {
|
||||
version: number;
|
||||
@@ -39,6 +40,7 @@ export const migrations: Migration[] = [
|
||||
moduleAgentToAgentDestinations,
|
||||
moduleAgentMessagePolicies,
|
||||
moduleApprovalsTitleOptions,
|
||||
moduleApprovalsApprover,
|
||||
migration008,
|
||||
migration009,
|
||||
migration010,
|
||||
|
||||
@@ -0,0 +1,14 @@
|
||||
import type { Migration } from './index.js';
|
||||
|
||||
/**
|
||||
* `approver_user_id` on `pending_approvals`: when an approval names a specific
|
||||
* approver (an a2a message-gate policy's approver), only that exact user may
|
||||
* resolve it. NULL keeps the existing group/owner authorization path.
|
||||
*/
|
||||
export const moduleApprovalsApprover: Migration = {
|
||||
version: 18,
|
||||
name: 'approvals-approver-user-id',
|
||||
up(db) {
|
||||
db.exec(`ALTER TABLE pending_approvals ADD COLUMN approver_user_id TEXT;`);
|
||||
},
|
||||
};
|
||||
+3
-2
@@ -155,11 +155,11 @@ export function createPendingApproval(
|
||||
`INSERT OR IGNORE INTO pending_approvals
|
||||
(approval_id, session_id, request_id, action, payload, created_at,
|
||||
agent_group_id, channel_type, platform_id, platform_message_id, expires_at, status,
|
||||
title, options_json)
|
||||
title, options_json, approver_user_id)
|
||||
VALUES
|
||||
(@approval_id, @session_id, @request_id, @action, @payload, @created_at,
|
||||
@agent_group_id, @channel_type, @platform_id, @platform_message_id, @expires_at, @status,
|
||||
@title, @options_json)`,
|
||||
@title, @options_json, @approver_user_id)`,
|
||||
)
|
||||
.run({
|
||||
session_id: null,
|
||||
@@ -169,6 +169,7 @@ export function createPendingApproval(
|
||||
platform_message_id: null,
|
||||
expires_at: null,
|
||||
status: 'pending',
|
||||
approver_user_id: null,
|
||||
...pa,
|
||||
});
|
||||
return result.changes > 0;
|
||||
|
||||
@@ -243,7 +243,6 @@ export async function routeAgentMessage(msg: RoutableAgentMessage, session: Sess
|
||||
platform_id: targetAgentGroupId,
|
||||
content: msg.content,
|
||||
in_reply_to: msg.in_reply_to,
|
||||
approver,
|
||||
},
|
||||
});
|
||||
log.info('Agent message held for approval', {
|
||||
|
||||
@@ -144,7 +144,7 @@ describe('agent message policies', () => {
|
||||
const opts = vi.mocked(requestApproval).mock.calls[0][0];
|
||||
expect(opts.action).toBe('a2a_message_gate');
|
||||
expect(opts.approverUserId).toBe('telegram:dana');
|
||||
expect(opts.payload).toMatchObject({ id: 'm2', platform_id: B, approver: 'telegram:dana' });
|
||||
expect(opts.payload).toMatchObject({ id: 'm2', platform_id: B });
|
||||
expect(JSON.parse(String(opts.payload.content)).text).toBe('sensitive');
|
||||
});
|
||||
|
||||
|
||||
@@ -237,6 +237,7 @@ export async function requestApproval(opts: RequestApprovalOptions): Promise<voi
|
||||
created_at: new Date().toISOString(),
|
||||
title,
|
||||
options_json: JSON.stringify(normalizedOptions),
|
||||
approver_user_id: approverUserId ?? null,
|
||||
});
|
||||
|
||||
const adapter = getDeliveryAdapter();
|
||||
|
||||
@@ -162,21 +162,22 @@ describe('approval response authorization', () => {
|
||||
expect(getPendingApproval('appr-3')).toBeUndefined();
|
||||
});
|
||||
|
||||
it('an approval naming an approver in its payload is resolvable by that user, not a non-assignee', async () => {
|
||||
it('an approval with approver_user_id is resolvable by that user, not a non-assignee', async () => {
|
||||
const { registerApprovalHandler } = await import('./primitive.js');
|
||||
const { handleApprovalsResponse } = await import('./response-handler.js');
|
||||
const handler = vi.fn().mockResolvedValue(undefined);
|
||||
registerApprovalHandler('payload_approver_action', handler);
|
||||
registerApprovalHandler('assigned_approver_action', handler);
|
||||
|
||||
createPendingApproval({
|
||||
approval_id: 'appr-4',
|
||||
session_id: 'sess-1',
|
||||
request_id: 'appr-4',
|
||||
action: 'payload_approver_action',
|
||||
payload: JSON.stringify({ approver: 'telegram:dana' }),
|
||||
action: 'assigned_approver_action',
|
||||
payload: JSON.stringify({}),
|
||||
created_at: now(),
|
||||
title: 'Assigned approval',
|
||||
options_json: JSON.stringify([]),
|
||||
approver_user_id: 'telegram:dana',
|
||||
});
|
||||
|
||||
// A non-assignee (no global/owner role) cannot resolve it.
|
||||
|
||||
@@ -126,9 +126,8 @@ function isAuthorizedApprovalClick(approval: PendingApproval, payload: ResponseP
|
||||
if (!userId) return false;
|
||||
|
||||
// An approval may name a specific approver; only that exact user may resolve it.
|
||||
const assignee = approvalAssignee(approval);
|
||||
if (assignee) {
|
||||
return userId === assignee;
|
||||
if (approval.approver_user_id) {
|
||||
return userId === approval.approver_user_id;
|
||||
}
|
||||
|
||||
const agentGroupId =
|
||||
@@ -140,13 +139,3 @@ function isAuthorizedApprovalClick(approval: PendingApproval, payload: ResponseP
|
||||
|
||||
return hasAdminPrivilege(userId, agentGroupId);
|
||||
}
|
||||
|
||||
/** The `approver` user-id named in the stored approval payload (not the click payload), if any. */
|
||||
function approvalAssignee(approval: PendingApproval): string | null {
|
||||
try {
|
||||
const parsed = JSON.parse(approval.payload) as { approver?: unknown };
|
||||
return typeof parsed.approver === 'string' ? parsed.approver : null;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -204,6 +204,8 @@ export interface PendingApproval {
|
||||
status: 'pending' | 'approved' | 'rejected' | 'expired';
|
||||
title: string;
|
||||
options_json: string;
|
||||
/** When set, only this exact user may resolve the approval. */
|
||||
approver_user_id: string | null;
|
||||
}
|
||||
|
||||
// ── Agent destinations (central DB) ──
|
||||
|
||||
Reference in New Issue
Block a user