From c97548db60e1264bd24602d3da8cc90aba8bea0a Mon Sep 17 00:00:00 2001 From: gavrielc Date: Sat, 4 Jul 2026 16:10:30 +0300 Subject: [PATCH] Expire and clean up abandoned pending-approval rows Delete the just-created pending row when the sender/channel approval card can't be delivered (adapter missing or delivery throws), matching each file's header contract, and give module approvals a ~7-day expiry that a new host-sweep finalizer turns into a "request timed out" notice so an unanswered card never strands the requesting agent. Co-Authored-By: Claude Opus 4.8 --- src/db/sessions.ts | 14 ++++++ src/host-sweep.ts | 12 +++++ src/modules/approvals/finalize.ts | 46 +++++++++++++++---- src/modules/approvals/index.ts | 2 + src/modules/approvals/primitive.ts | 10 ++++ src/modules/approvals/reason-capture.test.ts | 39 ++++++++++++++++ src/modules/approvals/timeout-sweep.ts | 36 +++++++++++++++ .../permissions/channel-approval.test.ts | 16 +++++++ src/modules/permissions/channel-approval.ts | 16 ++++++- .../permissions/sender-approval.test.ts | 16 +++++++ src/modules/permissions/sender-approval.ts | 19 ++++++-- 11 files changed, 210 insertions(+), 16 deletions(-) create mode 100644 src/modules/approvals/timeout-sweep.ts diff --git a/src/db/sessions.ts b/src/db/sessions.ts index ef060e36c..ab9703458 100644 --- a/src/db/sessions.ts +++ b/src/db/sessions.ts @@ -207,6 +207,20 @@ export function getExpiredAwaitingReasonApprovals(nowIso: string): PendingApprov .all(nowIso) as PendingApproval[]; } +/** + * Module approvals (session-scoped) that sat unanswered past their expiry — + * the timeout sweep's set. Scoped to `session_id IS NOT NULL` so OneCLI + * credential rows (session-less, resolved in-memory by the gateway callback) + * are never swept here even though they also carry an `expires_at`. + */ +export function getExpiredPendingApprovals(nowIso: string): PendingApproval[] { + return getDb() + .prepare( + "SELECT * FROM pending_approvals WHERE status = 'pending' AND session_id IS NOT NULL AND expires_at IS NOT NULL AND expires_at <= ?", + ) + .all(nowIso) as PendingApproval[]; +} + export function deletePendingApproval(approvalId: string): void { getDb().prepare('DELETE FROM pending_approvals WHERE approval_id = ?').run(approvalId); } diff --git a/src/host-sweep.ts b/src/host-sweep.ts index 56848f642..9a576da64 100644 --- a/src/host-sweep.ts +++ b/src/host-sweep.ts @@ -164,6 +164,18 @@ async function sweep(): Promise { } // MODULE-HOOK:approvals-reason-sweep:end + // Finalize module approvals that timed out unanswered — the pending row + // would otherwise strand the requesting agent forever. Central-DB scan, once + // per tick. + // MODULE-HOOK:approvals-timeout-sweep:start + try { + const { sweepExpiredModuleApprovals } = await import('./modules/approvals/index.js'); + await sweepExpiredModuleApprovals(); + } catch (err) { + log.error('Module-approval timeout sweep failed', { err }); + } + // MODULE-HOOK:approvals-timeout-sweep:end + setTimeout(sweep, SWEEP_INTERVAL_MS); } diff --git a/src/modules/approvals/finalize.ts b/src/modules/approvals/finalize.ts index a7d885f42..3d20fc0f8 100644 --- a/src/modules/approvals/finalize.ts +++ b/src/modules/approvals/finalize.ts @@ -1,11 +1,12 @@ /** - * Shared "finalize a rejected approval" path. + * Shared "finalize a resolved approval" path. * - * Three entry points land here so they relay one message and clean up + * Four entry points land here so they relay one message and clean up * identically: * 1. The instant Reject button (response-handler.ts) * 2. A captured Reject-with-reason reply (reason-capture.ts) * 3. The host-sweep ghost finalizer (reason-capture.ts, via host-sweep) + * 4. The host-sweep timeout finalizer (timeout-sweep.ts, via host-sweep) * * Kept in its own leaf file so both response-handler.ts and reason-capture.ts * can import it without an import cycle (finalize → primitive only). @@ -36,6 +37,37 @@ export async function finalizeReject( ? `Your ${approval.action} request was rejected by admin: "${reason}"` : `Your ${approval.action} request was rejected by admin.`; + writeAgentNote(session, text); + + log.info('Approval rejected', { + approvalId: approval.approval_id, + action: approval.action, + userId, + withReason: reason !== undefined, + }); + + await finalizeResolution(approval, session, userId); +} + +/** + * Finalize a module approval that no admin ever answered — its expiry elapsed. + * Same cleanup as a plain reject (drop the row, fire resolved callbacks, wake + * the container) but tells the agent the request timed out rather than that an + * admin rejected it, since no admin acted. + */ +export async function finalizeTimeout(approval: PendingApproval, session: Session): Promise { + writeAgentNote(session, `Your ${approval.action} request timed out waiting for admin approval.`); + + log.info('Approval timed out', { + approvalId: approval.approval_id, + action: approval.action, + }); + + await finalizeResolution(approval, session, ''); +} + +/** Relay a one-line system note to the requesting agent's session. */ +function writeAgentNote(session: Session, text: string): void { writeSessionMessage(session.agent_group_id, session.id, { id: `appr-note-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`, kind: 'chat', @@ -45,14 +77,10 @@ export async function finalizeReject( threadId: null, content: JSON.stringify({ text, sender: 'system', senderId: 'system' }), }); +} - log.info('Approval rejected', { - approvalId: approval.approval_id, - action: approval.action, - userId, - withReason: reason !== undefined, - }); - +/** Drop the pending row, fire approval-resolved callbacks, and wake the container. */ +async function finalizeResolution(approval: PendingApproval, session: Session, userId: string): Promise { deletePendingApproval(approval.approval_id); await notifyApprovalResolved({ approval, session, outcome: 'reject', userId }); await wakeContainer(session); diff --git a/src/modules/approvals/index.ts b/src/modules/approvals/index.ts index 8dcb274f5..0b7e271e0 100644 --- a/src/modules/approvals/index.ts +++ b/src/modules/approvals/index.ts @@ -33,6 +33,8 @@ export type { ApprovalHandler, ApprovalHandlerContext, RequestApprovalOptions } // Host-sweep hook for ghosted "Reject with reason…" holds. The re-export also // loads reason-capture.js, registering its message-interceptor on import. export { sweepAwaitingReasonRejects } from './reason-capture.js'; +// Host-sweep hook for module approvals that timed out unanswered. +export { sweepExpiredModuleApprovals } from './timeout-sweep.js'; registerResponseHandler(handleApprovalsResponse); diff --git a/src/modules/approvals/primitive.ts b/src/modules/approvals/primitive.ts index 4a0fc3fa7..4bd859f95 100644 --- a/src/modules/approvals/primitive.ts +++ b/src/modules/approvals/primitive.ts @@ -39,6 +39,15 @@ import { ensureUserDm } from '../permissions/user-dm.js'; */ export const REJECT_WITH_REASON_VALUE = 'reject_with_reason'; +/** + * How long a module approval waits for an admin before the host sweep + * finalizes it as a timeout. Without this a card that's never answered (or + * whose delivery silently fell on the floor) leaves a `status='pending'` row + * that blocks the requesting agent forever. Reuses the `expires_at` column; + * the awaiting-reason hold overwrites it with its own short window on transition. + */ +export const MODULE_APPROVAL_TIMEOUT_MS = 7 * 24 * 60 * 60 * 1000; + /** * Three-button approval UI. Plain Reject is the instant fast path; "Reject with * reason…" opts into the reason-capture flow. Shared by every module approval @@ -248,6 +257,7 @@ export async function requestApproval(opts: RequestApprovalOptions): Promise { expect(lastRelayedText()).toBe('Your install_packages request was rejected by admin.'); }); }); + +/** Seed a pending module approval carrying an explicit expiry. */ +function seedApprovalWithExpiry(approvalId: string, expiresAt: string, action = 'create_agent'): void { + createPendingApproval({ + approval_id: approvalId, + session_id: 'sess-1', + request_id: approvalId, + action, + payload: JSON.stringify({ name: 'child' }), + created_at: now(), + expires_at: expiresAt, + title: 'Approval', + options_json: JSON.stringify([]), + }); +} + +describe('module-approval timeout sweep', () => { + it('finalizes an expired pending approval and notifies the agent', async () => { + const { sweepExpiredModuleApprovals } = await import('./timeout-sweep.js'); + const { wakeContainer } = await import('../../container-runner.js'); + seedApprovalWithExpiry('appr-timeout', new Date(Date.now() - 1000).toISOString(), 'install_packages'); + + await sweepExpiredModuleApprovals(); + + expect(getPendingApproval('appr-timeout')).toBeUndefined(); + expect(lastRelayedText()).toBe('Your install_packages request timed out waiting for admin approval.'); + expect(vi.mocked(wakeContainer)).toHaveBeenCalled(); + }); + + it('leaves a not-yet-expired pending approval untouched', async () => { + const { sweepExpiredModuleApprovals } = await import('./timeout-sweep.js'); + seedApprovalWithExpiry('appr-fresh', new Date(Date.now() + 60_000).toISOString()); + + await sweepExpiredModuleApprovals(); + + expect(getPendingApproval('appr-fresh')?.status).toBe('pending'); + expect(vi.mocked(writeSessionMessage)).not.toHaveBeenCalled(); + }); +}); diff --git a/src/modules/approvals/timeout-sweep.ts b/src/modules/approvals/timeout-sweep.ts new file mode 100644 index 000000000..7b674cb47 --- /dev/null +++ b/src/modules/approvals/timeout-sweep.ts @@ -0,0 +1,36 @@ +/** + * Module-approval timeout sweep. + * + * Module approvals (create_agent, install_packages, add_mcp_server, …) are + * created with a `status='pending'` row and a ~7-day `expires_at` (see + * MODULE_APPROVAL_TIMEOUT_MS in primitive.ts). If the card is never answered — + * or its delivery silently fell on the floor — the row would otherwise live + * forever and permanently block the requesting agent behind an unresolved + * approval. This sweep, run once per host-sweep tick, finalizes any such row + * as a timeout via the shared finalize path so the agent always gets a + * decision. + * + * Scope: `getExpiredPendingApprovals` filters to session-scoped rows, so + * session-less OneCLI credential approvals (resolved in-memory by the gateway + * callback) are never touched here even though they carry their own expiry. + */ +import { deletePendingApproval, getExpiredPendingApprovals, getSession } from '../../db/sessions.js'; +import { log } from '../../log.js'; +import { finalizeTimeout } from './finalize.js'; + +/** + * Host-sweep finalizer: any module approval whose expiry elapsed is finalized + * as a timeout. Called once per sweep tick. + */ +export async function sweepExpiredModuleApprovals(): Promise { + const rows = getExpiredPendingApprovals(new Date().toISOString()); + for (const approval of rows) { + const session = approval.session_id ? getSession(approval.session_id) : null; + if (!session) { + // The requesting session is gone — nothing to notify; just drop the row. + deletePendingApproval(approval.approval_id); + continue; + } + await finalizeTimeout(approval, session); + } +} diff --git a/src/modules/permissions/channel-approval.test.ts b/src/modules/permissions/channel-approval.test.ts index 02e87d373..66603f261 100644 --- a/src/modules/permissions/channel-approval.test.ts +++ b/src/modules/permissions/channel-approval.test.ts @@ -176,6 +176,22 @@ describe('unknown-channel registration flow', () => { expect(count).toBe(1); }); + it('drops the pending row when card delivery fails (no permanent block)', async () => { + const { routeInbound } = await import('../../router.js'); + deliverMock.mockRejectedValueOnce(new Error('delivery boom')); + + await routeInbound(groupMention('chat-fail')); + await new Promise((r) => setTimeout(r, 10)); + + // Delivery was attempted but threw... + expect(deliverMock).toHaveBeenCalledTimes(1); + // ...and the row was rolled back so the dedup gate lets a future attempt + // through instead of blocking the channel forever behind a lost card. + const { getDb } = await import('../../db/connection.js'); + const count = (getDb().prepare('SELECT COUNT(*) AS c FROM pending_channel_approvals').get() as { c: number }).c; + expect(count).toBe(0); + }); + it('dedups a second mention while the card is pending', async () => { const { routeInbound } = await import('../../router.js'); await routeInbound(groupMention('chat-busy')); diff --git a/src/modules/permissions/channel-approval.ts b/src/modules/permissions/channel-approval.ts index 9352b8903..d001ce586 100644 --- a/src/modules/permissions/channel-approval.ts +++ b/src/modules/permissions/channel-approval.ts @@ -54,7 +54,11 @@ import { log } from '../../log.js'; import type { InboundEvent } from '../../channels/adapter.js'; import type { AgentGroup } from '../../types.js'; import { pickApprovalDelivery, pickApprover } from '../approvals/primitive.js'; -import { createPendingChannelApproval, hasInFlightChannelApproval } from './db/pending-channel-approvals.js'; +import { + createPendingChannelApproval, + deletePendingChannelApproval, + hasInFlightChannelApproval, +} from './db/pending-channel-approvals.js'; import { hasAdminPrivilege } from './db/user-roles.js'; // ── Value constants (response handler in index.ts parses these) ── @@ -218,7 +222,11 @@ export async function requestChannelApproval(input: RequestChannelApprovalInput) const adapter = getDeliveryAdapter(); if (!adapter) { - log.error('Channel registration row created but no delivery adapter is wired', { messagingGroupId }); + // Without a delivery adapter the card can't be sent, so drop the row we + // just created. Leaving it would block the channel forever behind the + // dedup gate (matches the header contract: adapter-missing → no row). + deletePendingChannelApproval(messagingGroupId); + log.error('Channel registration dropped — no delivery adapter is wired', { messagingGroupId }); return; } @@ -242,6 +250,10 @@ export async function requestChannelApproval(input: RequestChannelApprovalInput) approver: delivery.userId, }); } catch (err) { + // Delivery failed — drop the row so the dedup gate doesn't permanently + // block this channel behind a card that never arrived (matches the header + // contract: delivery failure → no row, a future attempt retries). + deletePendingChannelApproval(messagingGroupId); log.error('Channel registration card delivery failed', { messagingGroupId, err }); } } diff --git a/src/modules/permissions/sender-approval.test.ts b/src/modules/permissions/sender-approval.test.ts index f037fa848..1abebee44 100644 --- a/src/modules/permissions/sender-approval.test.ts +++ b/src/modules/permissions/sender-approval.test.ts @@ -175,6 +175,22 @@ describe('unknown-sender request_approval flow', () => { expect(rows).toHaveLength(1); }); + it('drops the pending row when card delivery fails (no permanent block)', async () => { + const { routeInbound } = await import('../../router.js'); + deliverMock.mockRejectedValueOnce(new Error('delivery boom')); + + await routeInbound(stranger('hi there')); + await new Promise((r) => setTimeout(r, 10)); + + // Delivery was attempted but threw... + expect(deliverMock).toHaveBeenCalledTimes(1); + // ...and the row was rolled back so the dedup gate lets a future attempt + // through instead of stranding the sender forever behind a lost card. + const { getDb } = await import('../../db/connection.js'); + const count = (getDb().prepare('SELECT COUNT(*) AS c FROM pending_sender_approvals').get() as { c: number }).c; + expect(count).toBe(0); + }); + it('dedups a second message from the same stranger while pending', async () => { const { routeInbound } = await import('../../router.js'); await routeInbound(stranger('hello')); diff --git a/src/modules/permissions/sender-approval.ts b/src/modules/permissions/sender-approval.ts index fb3e24e01..2e06932db 100644 --- a/src/modules/permissions/sender-approval.ts +++ b/src/modules/permissions/sender-approval.ts @@ -32,7 +32,11 @@ import { getDeliveryAdapter } from '../../delivery.js'; import { log } from '../../log.js'; import type { InboundEvent } from '../../channels/adapter.js'; import { pickApprovalDelivery, pickApprover } from '../approvals/primitive.js'; -import { createPendingSenderApproval, hasInFlightSenderApproval } from './db/pending-sender-approvals.js'; +import { + createPendingSenderApproval, + deletePendingSenderApproval, + hasInFlightSenderApproval, +} from './db/pending-sender-approvals.js'; const APPROVAL_OPTIONS: RawOption[] = [ { label: 'Allow', selectedLabel: '✅ Allowed', value: 'approve' }, @@ -109,10 +113,11 @@ export async function requestSenderApproval(input: RequestSenderApprovalInput): const adapter = getDeliveryAdapter(); if (!adapter) { - // Without a delivery adapter, the card can't be sent. Log + leave the - // row in place so the admin can see it via DB or manual tooling; the - // dedup gate will suppress further cards until it's cleared. - log.error('Unknown-sender approval row created but no delivery adapter is wired', { + // Without a delivery adapter the card can't be sent, so drop the row we + // just created. Leaving it would strand the sender forever behind the + // dedup gate (matches the header contract: adapter-missing → no row). + deletePendingSenderApproval(approvalId); + log.error('Unknown-sender approval dropped — no delivery adapter is wired', { approvalId, }); return; @@ -140,6 +145,10 @@ export async function requestSenderApproval(input: RequestSenderApprovalInput): agentGroupId, }); } catch (err) { + // Delivery failed — drop the row so the dedup gate doesn't permanently + // block this sender behind a card that never arrived (matches the header + // contract: delivery failure → no row, a future attempt retries). + deletePendingSenderApproval(approvalId); log.error('Unknown-sender approval card delivery failed', { approvalId, err,