diff --git a/CHANGELOG.md b/CHANGELOG.md index 609dc7de0..e35e8f6a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ All notable changes to NanoClaw will be documented in this file. ## [Unreleased] +- **One approval contract across every hold (guarded-actions phase 1).** All approval holds now share one hold-record shape on `pending_approvals` (eligibility rule, approver blast-radius scope, dedup key) and ONE click-authorization rule (`mayResolve` in `src/modules/approvals/eligibility.ts`), replacing three divergent click-auth copies. Unknown-sender admission folds onto the approvals primitive (sessionless hold, action `sender_admit`) — the `pending_sender_approvals` table is dropped by migration; an in-flight sender card does not survive the upgrade (a new message from the same sender re-triggers one). Channel registration and OneCLI keep their flows and adopt the shared eligibility; their resolutions (click / expiry / boot sweep) now announce through the approval-resolved observer. Two absorbed security fixes intentionally change decision outcomes: **(D1)** a hold with global blast radius (e.g. `roles grant`) can only be approved by an owner or global admin — a scoped admin's click is now rejected; **(D4, approver half)** channel-registration approvers are owners/global admins, no longer "admin of whichever agent group sorts first". - **Optional per-container resource caps.** `CONTAINER_CPU_LIMIT` and `CONTAINER_MEMORY_LIMIT` pass through to `docker run` as `--cpus` / `--memory` (`container-runner.ts`). Both empty by default — no flag added, spawn args byte-identical to today — so existing installs are unaffected. Set them to cap an agent container's CPU/memory so one agent can't monopolize the host (e.g. `CONTAINER_CPU_LIMIT=2`, `CONTAINER_MEMORY_LIMIT=8g`). Swap is intentionally not managed here: `--memory` is a hard cap on a swapless host. - [BREAKING] **Chat SDK pinned to `4.29.0` (was `4.26.0` via `^4.24.0`).** `chat` and the `@chat-adapter/*` channel adapters are version-locked — the adapter's `ChatInstance` must match the bridge's, so a mismatched pair fails to typecheck at `createChatSdkBridge(...)`. `chat` is therefore pinned exactly, and the channel-adapter install pins move with it — the `/add-` SKILL.md steps and `setup/*.sh` scripts on `main`, plus the adapter code on the `channels` branch. Core installs with no channel (only `cli`) are unaffected. **Migration:** if any channel is installed (Slack, Discord, Telegram, Teams, …), re-run its `/add-` skill to pull the matching `4.29.0` adapter. - **Budget/billing-exhausted LLM turns now reach the user instead of being silently dropped.** When a turn ends in a non-retryable provider error (e.g. an Anthropic `403 billing_error`) with no `` wrapping, the agent-runner delivers the provider's notice to the originating channel and stops re-nudging the failing gateway. `providers/claude.ts` now surfaces the SDK's `is_error` flag (and the error subtype's `errors[]` text); `poll-loop.ts` delivers that text and skips the re-wrap retry. Fixes the case where a spend-limit notice produced silence plus a turn-after-turn retry loop. diff --git a/src/cli/dispatch.test.ts b/src/cli/dispatch.test.ts index 7afd5320f..b4bd779d3 100644 --- a/src/cli/dispatch.test.ts +++ b/src/cli/dispatch.test.ts @@ -136,6 +136,33 @@ register({ }, }); +register({ + name: 'roles-grant', + description: 'approval command on a non-scoped resource (global blast radius)', + resource: 'roles', + access: 'approval', + parseArgs: (raw) => raw, + handler: async () => ({}), +}); + +register({ + name: 'members-add-gated', + description: 'approval command on a scoped resource', + resource: 'members', + access: 'approval', + parseArgs: (raw) => raw, + handler: async () => ({}), +}); + +register({ + name: 'groups-update', + description: 'approval command on the groups resource (id = agent group)', + resource: 'groups', + access: 'approval', + parseArgs: (raw) => raw, + handler: async () => ({}), +}); + // Commands that return data shaped like real resources (for post-handler filtering tests) register({ name: 'groups-list-data', @@ -473,6 +500,47 @@ describe('CLI scope enforcement', () => { expect(approvalState.requestApproval).toHaveBeenCalledTimes(1); }); + // --- Approver blast radius (D1) --- + + it('holds on non-scoped resources carry approverScope global (roles grant)', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'global' }); + mockGetSession.mockReturnValue({ id: 's1', agent_group_id: 'g1', messaging_group_id: 'mg1' }); + mockGetAgentGroup.mockReturnValue({ id: 'g1', name: 'Group One' }); + + const resp = await dispatch( + { id: '1', command: 'roles-grant', args: { user: 'telegram:mallory', role: 'owner' } }, + agentCtx(), + ); + + expect(resp.ok).toBe(false); + expect(approvalState.requestApproval).toHaveBeenCalledTimes(1); + expect(approvalState.requestApproval.mock.calls[0][0]).toMatchObject({ approverScope: 'global' }); + }); + + it('holds on scoped resources pinned to the caller stay approverScope group', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'group' }); + mockGetSession.mockReturnValue({ id: 's1', agent_group_id: 'g1', messaging_group_id: 'mg1' }); + mockGetAgentGroup.mockReturnValue({ id: 'g1', name: 'Group One' }); + + const resp = await dispatch({ id: '1', command: 'members-add-gated', args: { user: 'telegram:new' } }, agentCtx()); + + expect(resp.ok).toBe(false); + expect(approvalState.requestApproval).toHaveBeenCalledTimes(1); + expect(approvalState.requestApproval.mock.calls[0][0]).toMatchObject({ approverScope: 'group' }); + }); + + it('holds on scoped resources targeting another group escalate to approverScope global', async () => { + mockGetContainerConfig.mockReturnValue({ cli_scope: 'global' }); + mockGetSession.mockReturnValue({ id: 's1', agent_group_id: 'g1', messaging_group_id: 'mg1' }); + mockGetAgentGroup.mockReturnValue({ id: 'g1', name: 'Group One' }); + + const resp = await dispatch({ id: '1', command: 'groups-update', args: { id: 'g2' } }, agentCtx()); + + expect(resp.ok).toBe(false); + expect(approvalState.requestApproval).toHaveBeenCalledTimes(1); + expect(approvalState.requestApproval.mock.calls[0][0]).toMatchObject({ approverScope: 'global' }); + }); + // --- Post-handler filtering --- it('group: groups list filters out other groups', async () => { diff --git a/src/cli/dispatch.ts b/src/cli/dispatch.ts index f0dc430cb..fc3df721b 100644 --- a/src/cli/dispatch.ts +++ b/src/cli/dispatch.ts @@ -12,13 +12,37 @@ import { getSession } from '../db/sessions.js'; import { registerApprovalHandler, requestApproval } from '../modules/approvals/index.js'; import type { CallerContext, ErrorCode, RequestFrame, ResponseFrame } from './frame.js'; import { getResource } from './crud.js'; -import { lookup } from './registry.js'; +import { lookup, type CommandDef } from './registry.js'; type DispatchOptions = { /** True when a command is being replayed after approval. */ approved?: boolean; }; +/** + * Resources reachable under `cli_scope: 'group'` — and, because their rows + * anchor to one agent group, the resources whose held mutations have + * group-local blast radius. + */ +const GROUP_SCOPED_RESOURCES = new Set(['groups', 'sessions', 'destinations', 'members']); + +/** + * Blast radius of a held command, for approver eligibility (D1): a mutation + * of a non-group-scoped resource (roles, users, wirings, messaging-groups, + * policies) — or one explicitly targeting another agent group — needs an + * owner or global admin to approve; a scoped admin's click is rejected. + */ +function approverScopeFor( + cmd: CommandDef, + args: Record, + callerAgentGroupId: string, +): 'group' | 'global' { + if (!cmd.resource || !GROUP_SCOPED_RESOURCES.has(cmd.resource)) return 'global'; + const groupRefs = [args.agent_group_id, args.group]; + if (cmd.resource === 'groups' || cmd.resource === 'destinations') groupRefs.push(args.id); + return groupRefs.some((v) => v !== undefined && v !== callerAgentGroupId) ? 'global' : 'group'; +} + export async function dispatch( req: RequestFrame, ctx: CallerContext, @@ -62,9 +86,8 @@ export async function dispatch( } if (cliScope === 'group') { - const allowed = new Set(['groups', 'sessions', 'destinations', 'members']); // Only allow whitelisted resources and general commands (no resource, like help) - if (cmd.resource && !allowed.has(cmd.resource)) { + if (cmd.resource && !GROUP_SCOPED_RESOURCES.has(cmd.resource)) { return err(req.id, 'forbidden', `CLI access is scoped to this agent group. Cannot access "${cmd.resource}".`); } @@ -134,6 +157,7 @@ export async function dispatch( payload: { frame: { id: req.id, command: req.command, args: req.args }, callerContext: ctx }, title: `CLI: ${req.command}`, question: `Agent "${agentName}" wants to run:\n\`ncl ${req.command}${argSummary ? ' ' + argSummary : ''}\``, + approverScope: approverScopeFor(cmd, req.args, ctx.agentGroupId), }); return err(req.id, 'approval-pending', 'Approval request sent to admin. You will be notified of the result.'); diff --git a/src/cli/resources/approvals.ts b/src/cli/resources/approvals.ts index c67f4bcf9..f0e7059cd 100644 --- a/src/cli/resources/approvals.ts +++ b/src/cli/resources/approvals.ts @@ -48,6 +48,24 @@ registerResource({ }, { name: 'title', type: 'string', description: 'Card title shown to the admin.' }, { name: 'options_json', type: 'json', description: 'Card button options as JSON array.' }, + { + name: 'approver_user_id', + type: 'string', + description: 'Named approver (exclusive) or the admin the card was delivered to (admins-of-scope).', + }, + { + name: 'eligibility', + type: 'string', + description: 'Who may resolve: only the named approver, or the admin chain of the anchoring group.', + enum: ['exclusive', 'admins-of-scope'], + }, + { + name: 'approver_scope', + type: 'string', + description: "Blast radius: 'global' holds require an owner or global admin to resolve.", + enum: ['group', 'global'], + }, + { name: 'dedup_key', type: 'string', description: 'In-flight dedup key (e.g. sender admission per chat+sender).' }, ], operations: { list: 'open', get: 'open' }, }); diff --git a/src/cli/resources/groups.test.ts b/src/cli/resources/groups.test.ts index 0973bbe1d..75ae7bef4 100644 --- a/src/cli/resources/groups.test.ts +++ b/src/cli/resources/groups.test.ts @@ -109,10 +109,13 @@ describe('groups CLI delete cascades dependent rows (#2525)', () => { VALUES (?, ?, 'req-1', 'cli_command', '{}', ?, ?, 'pending', '', '[]')`, ).run('pa-1', SID, now(), GID); + // Sessionless sender-admission hold anchored to the group (the folded + // pending_sender_approvals shape) — covered by the agent_group_id leg of + // the pending_approvals cascade. db.prepare( - `INSERT INTO pending_sender_approvals (id, messaging_group_id, agent_group_id, sender_identity, sender_name, original_message, approver_user_id, created_at) - VALUES ('psa-1', ?, ?, 'tg:99', 'them', '{}', ?, ?)`, - ).run(MGID, GID, UID, now()); + `INSERT INTO pending_approvals (approval_id, session_id, request_id, action, payload, created_at, agent_group_id, status, title, options_json, dedup_key) + VALUES (?, NULL, 'req-2', 'sender_admit', '{}', ?, ?, 'pending', '', '[]', 'sender_admit:mg:tg:99')`, + ).run('pa-2', now(), GID); db.prepare( `INSERT INTO pending_channel_approvals (messaging_group_id, agent_group_id, original_message, approver_user_id, created_at) @@ -148,10 +151,9 @@ describe('groups CLI delete cascades dependent rows (#2525)', () => { expect(data.removed).toMatchObject({ sessions: 1, pending_questions: 1, - pending_approvals: 1, + pending_approvals: 2, agent_destinations_owned: 1, agent_destinations_pointing: 0, - pending_sender_approvals: 1, pending_channel_approvals: 1, messaging_group_agents: 1, agent_group_members: 1, @@ -167,7 +169,6 @@ describe('groups CLI delete cascades dependent rows (#2525)', () => { count('SELECT COUNT(*) AS c FROM pending_approvals WHERE agent_group_id = ? OR session_id = ?', GID, SID), ).toBe(0); expect(count('SELECT COUNT(*) AS c FROM agent_destinations WHERE agent_group_id = ?', GID)).toBe(0); - expect(count('SELECT COUNT(*) AS c FROM pending_sender_approvals WHERE agent_group_id = ?', GID)).toBe(0); expect(count('SELECT COUNT(*) AS c FROM pending_channel_approvals WHERE agent_group_id = ?', GID)).toBe(0); expect(count('SELECT COUNT(*) AS c FROM messaging_group_agents WHERE agent_group_id = ?', GID)).toBe(0); expect(count('SELECT COUNT(*) AS c FROM agent_group_members WHERE agent_group_id = ?', GID)).toBe(0); diff --git a/src/cli/resources/groups.ts b/src/cli/resources/groups.ts index 27da6417f..e148c9b03 100644 --- a/src/cli/resources/groups.ts +++ b/src/cli/resources/groups.ts @@ -124,7 +124,6 @@ registerResource({ pending_approvals: 0, agent_destinations_owned: 0, agent_destinations_pointing: 0, - pending_sender_approvals: 0, pending_channel_approvals: 0, messaging_group_agents: 0, agent_group_members: 0, @@ -153,9 +152,6 @@ registerResource({ .run(groupId, groupId).changes; } counts.sessions = db.prepare('DELETE FROM sessions WHERE agent_group_id = ?').run(groupId).changes; - counts.pending_sender_approvals = db - .prepare('DELETE FROM pending_sender_approvals WHERE agent_group_id = ?') - .run(groupId).changes; counts.pending_channel_approvals = db .prepare('DELETE FROM pending_channel_approvals WHERE agent_group_id = ?') .run(groupId).changes; diff --git a/src/db/holds-eligibility-migration.test.ts b/src/db/holds-eligibility-migration.test.ts new file mode 100644 index 000000000..93b36be8a --- /dev/null +++ b/src/db/holds-eligibility-migration.test.ts @@ -0,0 +1,80 @@ +/** + * Upgrade-path test for migration 019 (holds-eligibility): in-flight + * pending_approvals rows created by the pre-contract code must come out with + * the eligibility the old click-auth gave them, and the sender table must be + * gone. + */ +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import { closeDb, initTestDb, runMigrations } from './index.js'; +import { migrations } from './migrations/index.js'; +import type Database from 'better-sqlite3'; + +let db: Database.Database; + +beforeEach(() => { + db = initTestDb(); + // Everything up to — but not including — the holds-eligibility migration. + runMigrations( + db, + migrations.filter((m) => m.name !== 'holds-eligibility'), + ); +}); + +afterEach(() => { + closeDb(); +}); + +function hasTable(name: string): boolean { + return db.prepare("SELECT 1 FROM sqlite_master WHERE type = 'table' AND name = ?").get(name) !== undefined; +} + +describe('migration 019 — holds-eligibility', () => { + it('backfills eligibility and agent_group_id on in-flight rows and drops the sender table', () => { + const now = new Date().toISOString(); + db.prepare("INSERT INTO agent_groups (id, name, folder, created_at) VALUES ('ag-1', 'One', 'one', ?)").run(now); + db.prepare( + "INSERT INTO sessions (id, agent_group_id, messaging_group_id, thread_id, created_at) VALUES ('sess-1', 'ag-1', NULL, NULL, ?)", + ).run(now); + + // Legacy a2a hold: named approver ⇒ was exclusive. + db.prepare( + `INSERT INTO pending_approvals (approval_id, session_id, request_id, action, payload, created_at, approver_user_id, title, options_json) + VALUES ('appr-a2a', 'sess-1', 'appr-a2a', 'a2a_message_gate', '{}', ?, 'tg:dana', '', '[]')`, + ).run(now); + // Legacy cli hold: no approver, no agent_group_id — click-auth fell back + // to the session's group; the backfill makes that anchoring explicit. + db.prepare( + `INSERT INTO pending_approvals (approval_id, session_id, request_id, action, payload, created_at, title, options_json) + VALUES ('appr-cli', 'sess-1', 'appr-cli', 'cli_command', '{}', ?, '', '[]')`, + ).run(now); + // Legacy OneCLI hold: sessionless, agent_group_id already stamped. + db.prepare( + `INSERT INTO pending_approvals (approval_id, session_id, request_id, action, payload, created_at, agent_group_id, title, options_json) + VALUES ('oa-1', NULL, 'req-uuid', 'onecli_credential', '{}', ?, 'ag-1', '', '[]')`, + ).run(now); + + expect(hasTable('pending_sender_approvals')).toBe(true); + + runMigrations(db); // applies only holds-eligibility + + const rows = db + .prepare('SELECT approval_id, eligibility, approver_scope, agent_group_id FROM pending_approvals') + .all() as Array<{ approval_id: string; eligibility: string; approver_scope: string; agent_group_id: string }>; + const byId = Object.fromEntries(rows.map((r) => [r.approval_id, r])); + + expect(byId['appr-a2a']).toMatchObject({ + eligibility: 'exclusive', + approver_scope: 'group', + agent_group_id: 'ag-1', + }); + expect(byId['appr-cli']).toMatchObject({ + eligibility: 'admins-of-scope', + approver_scope: 'group', + agent_group_id: 'ag-1', + }); + expect(byId['oa-1']).toMatchObject({ eligibility: 'admins-of-scope', agent_group_id: 'ag-1' }); + + expect(hasTable('pending_sender_approvals')).toBe(false); + }); +}); diff --git a/src/db/migrations/019-holds-eligibility.ts b/src/db/migrations/019-holds-eligibility.ts new file mode 100644 index 000000000..ee179753f --- /dev/null +++ b/src/db/migrations/019-holds-eligibility.ts @@ -0,0 +1,44 @@ +import type { Migration } from './index.js'; + +/** + * The hold-record contract lands on `pending_approvals` (guarded-actions + * phase 1 — see the guarded-actions decisions doc, decision 5): + * + * - `eligibility` — who may resolve the hold: 'exclusive' (only + * `approver_user_id`, e.g. an a2a policy's named approver) or + * 'admins-of-scope' (the admin chain of `agent_group_id`, plus the + * specific user the card was delivered to when `approver_user_id` is + * stamped — the sender/channel "named-or-admin" semantic). + * - `approver_scope` — the action's blast radius: 'global' holds (e.g. + * `roles grant`) can only be resolved by an owner or global admin; a + * scoped admin's click is rejected. + * - `dedup_key` — in-flight dedup: while a pending row carries a key, a + * second request with the same key is dropped (replaces the sender + * table's UNIQUE(messaging_group_id, sender_identity)). + * + * Backfills: rows with a named approver were exclusive before this column + * existed; `agent_group_id` is stamped from the requesting session so + * click-auth no longer needs the session fallback. + * + * `pending_sender_approvals` is dropped: sender admission now holds through + * the approvals primitive (action 'sender_admit'). In-flight sender cards at + * upgrade time die with the table — they are transient courtesy cards, and a + * new message from the same sender re-triggers one. + */ +export const migration019: Migration = { + version: 19, + name: 'holds-eligibility', + up(db) { + db.exec(`ALTER TABLE pending_approvals ADD COLUMN eligibility TEXT NOT NULL DEFAULT 'admins-of-scope';`); + db.exec(`ALTER TABLE pending_approvals ADD COLUMN approver_scope TEXT NOT NULL DEFAULT 'group';`); + db.exec(`ALTER TABLE pending_approvals ADD COLUMN dedup_key TEXT;`); + db.exec(`UPDATE pending_approvals SET eligibility = 'exclusive' WHERE approver_user_id IS NOT NULL;`); + db.exec( + `UPDATE pending_approvals + SET agent_group_id = (SELECT s.agent_group_id FROM sessions s WHERE s.id = pending_approvals.session_id) + WHERE agent_group_id IS NULL AND session_id IS NOT NULL;`, + ); + db.exec(`DROP INDEX IF EXISTS idx_pending_sender_approvals_mg;`); + db.exec(`DROP TABLE IF EXISTS pending_sender_approvals;`); + }, +}; diff --git a/src/db/migrations/index.ts b/src/db/migrations/index.ts index d8f619389..dab9e1e33 100644 --- a/src/db/migrations/index.ts +++ b/src/db/migrations/index.ts @@ -17,6 +17,7 @@ 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 { migration018 } from './018-approvals-approver-user-id.js'; +import { migration019 } from './019-holds-eligibility.js'; export interface Migration { version: number; @@ -50,6 +51,7 @@ export const migrations: Migration[] = [ migration014, migration015, migration016, + migration019, ]; /** Row shape of PRAGMA foreign_key_check. Child rowids are stable across a diff --git a/src/db/schema.ts b/src/db/schema.ts index 751ca4a89..7211f7d01 100644 --- a/src/db/schema.ts +++ b/src/db/schema.ts @@ -133,21 +133,6 @@ CREATE TABLE pending_questions ( created_at TEXT NOT NULL ); --- Pending approvals for unknown senders (unknown_sender_policy='request_approval'). --- In-flight dedup via UNIQUE(messaging_group_id, sender_identity): a second --- message from the same unknown sender while a card is pending is silently --- dropped instead of spamming the admin. -CREATE TABLE pending_sender_approvals ( - id TEXT PRIMARY KEY, - messaging_group_id TEXT NOT NULL REFERENCES messaging_groups(id), - agent_group_id TEXT NOT NULL REFERENCES agent_groups(id), - sender_identity TEXT NOT NULL, -- namespaced user id (channel_type:handle) - sender_name TEXT, - original_message TEXT NOT NULL, -- JSON of the original InboundEvent - approver_user_id TEXT NOT NULL, - created_at TEXT NOT NULL, - UNIQUE(messaging_group_id, sender_identity) -); `; /** diff --git a/src/db/sessions.ts b/src/db/sessions.ts index ef060e36c..cba364c70 100644 --- a/src/db/sessions.ts +++ b/src/db/sessions.ts @@ -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, approver_user_id) + title, options_json, approver_user_id, eligibility, approver_scope, dedup_key) 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, @approver_user_id)`, + @title, @options_json, @approver_user_id, @eligibility, @approver_scope, @dedup_key)`, ) .run({ session_id: null, @@ -170,11 +170,21 @@ export function createPendingApproval( expires_at: null, status: 'pending', approver_user_id: null, + eligibility: 'admins-of-scope', + approver_scope: 'group', + dedup_key: null, ...pa, }); return result.changes > 0; } +/** In-flight lookup for `requestApproval`'s dedup: any live row with this key blocks a repeat request. */ +export function getPendingApprovalByDedupKey(dedupKey: string): PendingApproval | undefined { + return getDb().prepare('SELECT * FROM pending_approvals WHERE dedup_key = ? LIMIT 1').get(dedupKey) as + | PendingApproval + | undefined; +} + export function getPendingApproval(approvalId: string): PendingApproval | undefined { return getDb().prepare('SELECT * FROM pending_approvals WHERE approval_id = ?').get(approvalId) as | PendingApproval @@ -230,8 +240,9 @@ export function getAskQuestionRender( | undefined; if (a?.title) return { title: a.title, options: JSON.parse(a.options_json) }; - // Channel-registration + unknown-sender approvals persist title/options_json - // the same way pending_approvals does — just SELECT and return. + // Channel-registration approvals persist title/options_json the same way + // pending_approvals does — just SELECT and return. (Unknown-sender approvals + // are pending_approvals rows since the sender fold.) if (hasTable(getDb(), 'pending_channel_approvals')) { const c = getDb() .prepare('SELECT title, options_json FROM pending_channel_approvals WHERE messaging_group_id = ?') @@ -239,12 +250,5 @@ export function getAskQuestionRender( if (c?.title) return { title: c.title, options: JSON.parse(c.options_json) }; } - if (hasTable(getDb(), 'pending_sender_approvals')) { - const s = getDb().prepare('SELECT title, options_json FROM pending_sender_approvals WHERE id = ?').get(id) as - | { title: string; options_json: string } - | undefined; - if (s?.title) return { title: s.title, options: JSON.parse(s.options_json) }; - } - return undefined; } diff --git a/src/modules/agent-to-agent/create-agent.ts b/src/modules/agent-to-agent/create-agent.ts index b8044da39..26ab56c4c 100644 --- a/src/modules/agent-to-agent/create-agent.ts +++ b/src/modules/agent-to-agent/create-agent.ts @@ -94,6 +94,10 @@ export async function handleCreateAgent(content: Record, sessio * a confined (non-global) agent group. `session` is the requesting parent. */ export const applyCreateAgent: ApprovalHandler = async ({ session, payload, notify }) => { + if (!session) { + log.warn('create_agent approval resolved without a session — dropping'); + return; + } const name = typeof payload.name === 'string' ? payload.name : ''; const instructions = typeof payload.instructions === 'string' ? payload.instructions : null; diff --git a/src/modules/agent-to-agent/message-gate.ts b/src/modules/agent-to-agent/message-gate.ts index 74c20c4cb..ceae27f8a 100644 --- a/src/modules/agent-to-agent/message-gate.ts +++ b/src/modules/agent-to-agent/message-gate.ts @@ -4,6 +4,10 @@ import type { ApprovalHandler } from '../approvals/index.js'; import { performAgentRoute, type RoutableAgentMessage } from './agent-route.js'; export const applyA2aMessageGate: ApprovalHandler = async ({ session, payload, notify }) => { + if (!session) { + log.warn('a2a_message_gate approval resolved without a session — dropping'); + return; + } const { id, platform_id, content, in_reply_to } = payload; if (typeof platform_id !== 'string' || !platform_id) { notify('Message approved but the target agent group was missing from the request.'); diff --git a/src/modules/approvals/approval-resolved.test.ts b/src/modules/approvals/approval-resolved.test.ts index 59f45637a..d27cb08e1 100644 --- a/src/modules/approvals/approval-resolved.test.ts +++ b/src/modules/approvals/approval-resolved.test.ts @@ -100,7 +100,7 @@ describe('approval-resolved callbacks', () => { expect(events[0].outcome).toBe('reject'); expect(events[0].approval.approval_id).toBe('appr-reject-1'); expect(events[0].approval.action).toBe('test_reject_action'); - expect(events[0].session.id).toBe('sess-1'); + expect(events[0].session?.id).toBe('sess-1'); expect(events[0].userId).toBe('slack:admin-1'); }); diff --git a/src/modules/approvals/eligibility.test.ts b/src/modules/approvals/eligibility.test.ts new file mode 100644 index 000000000..c1aa886f4 --- /dev/null +++ b/src/modules/approvals/eligibility.test.ts @@ -0,0 +1,222 @@ +/** + * mayResolve matrix — the one click-authorization rule for every hold. + * + * Covers each eligibility kind × clicker role × approver scope, including: + * - exclusive named approvers (a2a policy semantics: nobody else, not even + * an owner, may resolve) + * - admins-of-scope with and without a delivered approver (the + * sender/channel "named-or-admin" semantic) + * - the null-anchor variant (owners + global admins only) + * - the D1 fix: a 'global'-scope hold rejects a scoped admin's click even + * though the eligibility rule would otherwise accept it + * + * Plus an end-to-end D1 regression through the real response handler: a + * global-blast CLI hold (e.g. roles grant) clicked by a scoped admin is + * ignored; the owner's click resolves it. + */ +import * as fs from 'fs'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { initTestDb, closeDb, runMigrations } from '../../db/index.js'; +import { createAgentGroup } from '../../db/agent-groups.js'; +import { createSession, createPendingApproval, getPendingApproval } from '../../db/sessions.js'; +import { upsertUser } from '../permissions/db/users.js'; +import { grantRole } from '../permissions/db/user-roles.js'; +import { initSessionFolder } from '../../session-manager.js'; +import { eligibilityOf, mayResolve } from './eligibility.js'; +import { registerApprovalHandler } from './primitive.js'; +import { handleApprovalsResponse } from './response-handler.js'; + +vi.mock('../../container-runner.js', () => ({ + wakeContainer: vi.fn().mockResolvedValue(undefined), +})); + +vi.mock('../../config.js', async () => { + const actual = await vi.importActual('../../config.js'); + return { ...actual, DATA_DIR: '/tmp/nanoclaw-test-eligibility' }; +}); + +const TEST_DIR = '/tmp/nanoclaw-test-eligibility'; + +function now() { + return new Date().toISOString(); +} + +const OWNER = 'slack:owner'; +const GLOBAL_ADMIN = 'slack:global-admin'; +const SCOPED_ADMIN = 'slack:scoped-admin'; // admin @ ag-1 +const OTHER_ADMIN = 'slack:other-admin'; // admin @ ag-2 +const DELIVEREE = 'slack:deliveree'; // no role — the user a card was delivered to +const RANDO = 'slack:rando'; // no role + +beforeEach(() => { + if (fs.existsSync(TEST_DIR)) fs.rmSync(TEST_DIR, { recursive: true, force: true }); + fs.mkdirSync(TEST_DIR, { recursive: true }); + const db = initTestDb(); + runMigrations(db); + + createAgentGroup({ id: 'ag-1', name: 'One', folder: 'one', agent_provider: null, created_at: now() }); + createAgentGroup({ id: 'ag-2', name: 'Two', folder: 'two', agent_provider: null, created_at: now() }); + + for (const id of [OWNER, GLOBAL_ADMIN, SCOPED_ADMIN, OTHER_ADMIN, DELIVEREE, RANDO]) { + upsertUser({ id, kind: 'slack', display_name: id, created_at: now() }); + } + grantRole({ user_id: OWNER, role: 'owner', agent_group_id: null, granted_by: null, granted_at: now() }); + grantRole({ user_id: GLOBAL_ADMIN, role: 'admin', agent_group_id: null, granted_by: null, granted_at: now() }); + grantRole({ user_id: SCOPED_ADMIN, role: 'admin', agent_group_id: 'ag-1', granted_by: null, granted_at: now() }); + grantRole({ user_id: OTHER_ADMIN, role: 'admin', agent_group_id: 'ag-2', granted_by: null, granted_at: now() }); +}); + +afterEach(() => { + closeDb(); + if (fs.existsSync(TEST_DIR)) fs.rmSync(TEST_DIR, { recursive: true, force: true }); +}); + +describe('mayResolve matrix', () => { + it('exclusive: only the named user, regardless of rank', () => { + const e = { kind: 'exclusive', approverUserId: DELIVEREE } as const; + expect(mayResolve(e, 'group', DELIVEREE)).toBe(true); + expect(mayResolve(e, 'group', OWNER)).toBe(false); + expect(mayResolve(e, 'group', GLOBAL_ADMIN)).toBe(false); + expect(mayResolve(e, 'group', SCOPED_ADMIN)).toBe(false); + expect(mayResolve(e, 'group', RANDO)).toBe(false); + expect(mayResolve(e, 'group', null)).toBe(false); + }); + + it('exclusive ∩ global scope: the named user must also be owner/global admin', () => { + expect(mayResolve({ kind: 'exclusive', approverUserId: DELIVEREE }, 'global', DELIVEREE)).toBe(false); + expect(mayResolve({ kind: 'exclusive', approverUserId: OWNER }, 'global', OWNER)).toBe(true); + expect(mayResolve({ kind: 'exclusive', approverUserId: GLOBAL_ADMIN }, 'global', GLOBAL_ADMIN)).toBe(true); + }); + + it('admins-of-scope(group) with a delivered approver: named-or-admin', () => { + const e = { kind: 'admins-of-scope', agentGroupId: 'ag-1', deliveredTo: DELIVEREE } as const; + expect(mayResolve(e, 'group', DELIVEREE)).toBe(true); // delivered-to shortcut + expect(mayResolve(e, 'group', SCOPED_ADMIN)).toBe(true); + expect(mayResolve(e, 'group', GLOBAL_ADMIN)).toBe(true); + expect(mayResolve(e, 'group', OWNER)).toBe(true); + expect(mayResolve(e, 'group', OTHER_ADMIN)).toBe(false); // admin of another group + expect(mayResolve(e, 'group', RANDO)).toBe(false); + expect(mayResolve(e, 'group', null)).toBe(false); + }); + + it('admins-of-scope(group) without a delivered approver: pure admin chain', () => { + const e = { kind: 'admins-of-scope', agentGroupId: 'ag-1', deliveredTo: null } as const; + expect(mayResolve(e, 'group', SCOPED_ADMIN)).toBe(true); + expect(mayResolve(e, 'group', GLOBAL_ADMIN)).toBe(true); + expect(mayResolve(e, 'group', OWNER)).toBe(true); + expect(mayResolve(e, 'group', DELIVEREE)).toBe(false); + expect(mayResolve(e, 'group', OTHER_ADMIN)).toBe(false); + }); + + it('admins-of-scope(null): owners and global admins only', () => { + const e = { kind: 'admins-of-scope', agentGroupId: null, deliveredTo: null } as const; + expect(mayResolve(e, 'group', OWNER)).toBe(true); + expect(mayResolve(e, 'group', GLOBAL_ADMIN)).toBe(true); + expect(mayResolve(e, 'group', SCOPED_ADMIN)).toBe(false); + expect(mayResolve(e, 'group', RANDO)).toBe(false); + }); + + it('admins-of-scope(null) with a delivered approver keeps the delivered-to shortcut (channel semantics)', () => { + const e = { kind: 'admins-of-scope', agentGroupId: null, deliveredTo: DELIVEREE } as const; + expect(mayResolve(e, 'group', DELIVEREE)).toBe(true); + expect(mayResolve(e, 'group', SCOPED_ADMIN)).toBe(false); + }); + + it('D1 overlay: global scope rejects everyone below owner/global admin', () => { + const e = { kind: 'admins-of-scope', agentGroupId: 'ag-1', deliveredTo: DELIVEREE } as const; + expect(mayResolve(e, 'global', SCOPED_ADMIN)).toBe(false); // the D1 exploit, closed + expect(mayResolve(e, 'global', DELIVEREE)).toBe(false); + expect(mayResolve(e, 'global', OTHER_ADMIN)).toBe(false); + expect(mayResolve(e, 'global', OWNER)).toBe(true); + expect(mayResolve(e, 'global', GLOBAL_ADMIN)).toBe(true); + }); + + it('eligibilityOf maps row columns onto the rule', () => { + const base = { agent_group_id: 'ag-1' }; + expect(eligibilityOf({ ...base, eligibility: 'exclusive', approver_user_id: DELIVEREE })).toEqual({ + kind: 'exclusive', + approverUserId: DELIVEREE, + }); + expect(eligibilityOf({ ...base, eligibility: 'admins-of-scope', approver_user_id: DELIVEREE })).toEqual({ + kind: 'admins-of-scope', + agentGroupId: 'ag-1', + deliveredTo: DELIVEREE, + }); + expect(eligibilityOf({ ...base, eligibility: 'admins-of-scope', approver_user_id: null })).toEqual({ + kind: 'admins-of-scope', + agentGroupId: 'ag-1', + deliveredTo: null, + }); + // Malformed exclusive (no named user) falls back to the admin chain + // instead of bricking the hold. + expect(eligibilityOf({ ...base, eligibility: 'exclusive', approver_user_id: null })).toEqual({ + kind: 'admins-of-scope', + agentGroupId: 'ag-1', + deliveredTo: null, + }); + }); +}); + +describe('D1 regression — global-blast hold through the real response handler', () => { + beforeEach(() => { + createSession({ + id: 'sess-1', + agent_group_id: 'ag-1', + messaging_group_id: null, + thread_id: null, + agent_provider: null, + status: 'active', + container_status: 'stopped', + last_active: now(), + created_at: now(), + }); + initSessionFolder('ag-1', 'sess-1'); + }); + + it("a scoped admin's click on a roles-grant-style hold is ignored; the owner's click resolves it", async () => { + const applied: string[] = []; + registerApprovalHandler('test_global_blast', async ({ userId }) => { + applied.push(userId); + }); + + createPendingApproval({ + approval_id: 'appr-global-1', + session_id: 'sess-1', + request_id: 'appr-global-1', + action: 'test_global_blast', + payload: JSON.stringify({}), + created_at: now(), + agent_group_id: 'ag-1', + title: 'CLI: roles-grant', + options_json: JSON.stringify([]), + approver_scope: 'global', + }); + + // Scoped admin of the requesting group clicks approve — pre-D1 this + // resolved a global privilege grant; now it is ignored and the hold stays. + const claimedByScoped = await handleApprovalsResponse({ + questionId: 'appr-global-1', + value: 'approve', + userId: 'scoped-admin', + channelType: 'slack', + platformId: 'dm-scoped', + threadId: null, + }); + expect(claimedByScoped).toBe(true); + expect(applied).toEqual([]); + expect(getPendingApproval('appr-global-1')).toBeDefined(); + + // The owner's click resolves it. + await handleApprovalsResponse({ + questionId: 'appr-global-1', + value: 'approve', + userId: 'owner', + channelType: 'slack', + platformId: 'dm-owner', + threadId: null, + }); + expect(applied).toEqual([OWNER]); + expect(getPendingApproval('appr-global-1')).toBeUndefined(); + }); +}); diff --git a/src/modules/approvals/eligibility.ts b/src/modules/approvals/eligibility.ts new file mode 100644 index 000000000..30a94b023 --- /dev/null +++ b/src/modules/approvals/eligibility.ts @@ -0,0 +1,68 @@ +/** + * Approver eligibility — the one click-authorization rule for every hold. + * + * The hold-record contract (guarded-actions phase 1) is carried on the + * existing tables: a hold has an id, an action, a payload, an eligibility + * rule (who may resolve it), an approver scope (the action's blast radius), + * a restart policy, and an optional expiry. On `pending_approvals` these map + * to `approval_id` / `action` / `payload` / (`eligibility` + + * `approver_user_id` + `agent_group_id`) / `approver_scope` / `expires_at`; + * the restart policy is derived from the action (`onecli_credential` rows are + * swept-and-denied on boot, everything else is durable and keeps waiting). + * `pending_channel_approvals` maps through a synthesized view + * (channel-approval.ts) — the channel flow keeps its own table. + * + * Two eligibility kinds: + * - `exclusive` — only the named user may resolve (an a2a message policy's + * approver). Nobody else, including owners. + * - `admins-of-scope` — the admin chain of the anchoring agent group + * (scoped admin / global admin / owner), or owners + global admins when + * the anchor is null. When the hold records the user the card was + * delivered to, that user may also resolve — the sender/channel + * "named-or-admin" semantic, preserved verbatim from the pre-fold tables. + * + * The approver-scope overlay is the D1 fix: a hold whose action has global + * blast radius (e.g. `roles grant`) can only be resolved by an owner or + * global admin — a scoped admin's click is rejected regardless of the + * eligibility rule. + * + * `mayResolve` replaces the three divergent click-auth copies (approvals + * response handler, sender handler, channel handler) with one function. + */ +import type { ApproverEligibility, ApproverScope, PendingApproval } from '../../types.js'; +import { hasAdminPrivilege, isGlobalAdmin, isOwner } from '../permissions/db/user-roles.js'; + +export type { ApproverEligibility, ApproverScope } from '../../types.js'; + +/** May `clickerUserId` (namespaced `:`) resolve a hold with this eligibility + scope? */ +export function mayResolve(e: ApproverEligibility, scope: ApproverScope, clickerUserId: string | null): boolean { + if (!clickerUserId) return false; + + const globalScopeOk = scope !== 'global' || isOwner(clickerUserId) || isGlobalAdmin(clickerUserId); + + if (e.kind === 'exclusive') { + return clickerUserId === e.approverUserId && globalScopeOk; + } + + const eligible = + (e.deliveredTo !== null && clickerUserId === e.deliveredTo) || + (e.agentGroupId + ? hasAdminPrivilege(clickerUserId, e.agentGroupId) + : isOwner(clickerUserId) || isGlobalAdmin(clickerUserId)); + + return eligible && globalScopeOk; +} + +/** The eligibility rule a `pending_approvals` row encodes. */ +export function eligibilityOf( + approval: Pick, +): ApproverEligibility { + if (approval.eligibility === 'exclusive' && approval.approver_user_id) { + return { kind: 'exclusive', approverUserId: approval.approver_user_id }; + } + return { + kind: 'admins-of-scope', + agentGroupId: approval.agent_group_id, + deliveredTo: approval.eligibility === 'exclusive' ? null : approval.approver_user_id, + }; +} diff --git a/src/modules/approvals/finalize.ts b/src/modules/approvals/finalize.ts index a7d885f42..438b941b5 100644 --- a/src/modules/approvals/finalize.ts +++ b/src/modules/approvals/finalize.ts @@ -25,26 +25,31 @@ import { notifyApprovalResolved } from './primitive.js'; * attribution — the why, not the who (the rejecting admin may belong to a * different owner than the requesting agent). Callers are responsible for * clamping the reason length before passing it in. + * + * `session` is null for sessionless holds (e.g. sender admission) — there is + * no agent to notify or wake, so only the row delete + resolved callbacks run. */ export async function finalizeReject( approval: PendingApproval, - session: Session, + session: Session | null, userId: string, reason?: string, ): Promise { - const text = reason - ? `Your ${approval.action} request was rejected by admin: "${reason}"` - : `Your ${approval.action} request was rejected by admin.`; + if (session) { + const text = reason + ? `Your ${approval.action} request was rejected by admin: "${reason}"` + : `Your ${approval.action} request was rejected by admin.`; - writeSessionMessage(session.agent_group_id, session.id, { - id: `appr-note-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`, - kind: 'chat', - timestamp: new Date().toISOString(), - platformId: session.agent_group_id, - channelType: 'agent', - threadId: null, - content: JSON.stringify({ text, sender: 'system', senderId: 'system' }), - }); + writeSessionMessage(session.agent_group_id, session.id, { + id: `appr-note-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`, + kind: 'chat', + timestamp: new Date().toISOString(), + platformId: session.agent_group_id, + channelType: 'agent', + threadId: null, + content: JSON.stringify({ text, sender: 'system', senderId: 'system' }), + }); + } log.info('Approval rejected', { approvalId: approval.approval_id, @@ -55,5 +60,5 @@ export async function finalizeReject( deletePendingApproval(approval.approval_id); await notifyApprovalResolved({ approval, session, outcome: 'reject', userId }); - await wakeContainer(session); + if (session) await wakeContainer(session); } diff --git a/src/modules/approvals/onecli-approvals.ts b/src/modules/approvals/onecli-approvals.ts index 6d7183b88..df8d4057d 100644 --- a/src/modules/approvals/onecli-approvals.ts +++ b/src/modules/approvals/onecli-approvals.ts @@ -19,12 +19,13 @@ */ import { OneCLI, type ApprovalRequest, type ManualApprovalHandle } from '@onecli-sh/sdk'; -import { pickApprovalDelivery, pickApprover } from './primitive.js'; +import { notifyApprovalResolved, pickApprovalDelivery, pickApprover } from './primitive.js'; import { ONECLI_API_KEY, ONECLI_URL } from '../../config.js'; import { getAgentGroup } from '../../db/agent-groups.js'; import { createPendingApproval, deletePendingApproval, + getPendingApproval, getPendingApprovalsByAction, updatePendingApprovalStatus, } from '../../db/sessions.js'; @@ -64,20 +65,29 @@ function shortApprovalId(): string { return `oa-${Math.random().toString(36).slice(2, 10)}`; } -/** Called from the approvals response handler when a card button is clicked. */ -export function resolveOneCLIApproval(approvalId: string, selectedOption: string): boolean { +/** Called from the approvals response handler when a card button is clicked. `resolvedBy` = namespaced clicker id. */ +export function resolveOneCLIApproval(approvalId: string, selectedOption: string, resolvedBy = ''): boolean { const state = pending.get(approvalId); if (!state) return false; pending.delete(approvalId); clearTimeout(state.timer); const decision: Decision = selectedOption === 'approve' ? 'approve' : 'deny'; + const row = getPendingApproval(approvalId); updatePendingApprovalStatus(approvalId, decision === 'approve' ? 'approved' : 'rejected'); // Card is auto-edited to "✅