refactor(permissions): preserve pre-PR behavior in three spots

PR #5 review flagged three behavior changes that shouldn't have slipped in. This commit reverts each to match the pre-refactor behavior exactly. 1. User upsert ordering. Split the router hook into two setters: setSenderResolver (runs before agent resolution) and setAccessGate (runs after). Restores the pre-PR sequence where the users row is upserted even if the message is dropped by wiring or trigger rules. 2. dropped_messages audit. Moved src/modules/permissions/db/dropped-messages.ts back to src/db/dropped-messages.ts. The table is core audit infra, not permissions-specific. Router re-writes rows for no_agent_wired and no_trigger_match; the access gate writes rows for policy refusals. 3. Permissionless container fallback. Dropped. poll-loop restores the original deny-all check when NANOCLAW_ADMIN_USER_IDS is empty. Module contract doc updated with the two-hook shape. Validation: host build clean, 137/137 host tests, 17/17 container tests, typecheck clean, service boots to "NanoClaw running" with permissions module registering both hooks and clean SIGTERM shutdown. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-18 18:00:10 +03:00
parent 7cc4ecc3be
commit 32bcc2c5ae
5 changed files with 142 additions and 75 deletions
--- a/container/agent-runner/src/poll-loop.ts
+++ b/container/agent-runner/src/poll-loop.ts
@@ -80,10 +80,6 @@ export async function runPollLoop(config: PollLoopConfig): Promise<void> {

    // Handle commands: categorize chat messages
    const adminUserIds = config.adminUserIds ?? new Set<string>();
-    // Permissionless mode: when the permissions module isn't installed on
-    // the host, NANOCLAW_ADMIN_USER_IDS arrives empty. Treat every sender
-    // with an identifiable senderId as admin so admin commands still work.
-    const permissionless = adminUserIds.size === 0;
    const normalMessages = [];
    const commandIds: string[] = [];

@@ -103,8 +99,7 @@ export async function runPollLoop(config: PollLoopConfig): Promise<void> {
      }

      if (cmdInfo.category === 'admin') {
-        const authorized = permissionless ? !!cmdInfo.senderId : !!cmdInfo.senderId && adminUserIds.has(cmdInfo.senderId);
-        if (!authorized) {
+        if (!cmdInfo.senderId || !adminUserIds.has(cmdInfo.senderId)) {
          log(`Admin command denied: ${cmdInfo.command} from ${cmdInfo.senderId} (msg: ${msg.id})`);
          writeMessageOut({
            id: generateId(),
--- a/docs/module-contract.md
+++ b/docs/module-contract.md
@@ -68,30 +68,42 @@ export function registerDeliveryAction(action: string, handler: ActionHandler):

 **Current consumers:** scheduling (5 actions — `schedule_task`, `cancel_task`, `pause_task`, `resume_task`, `update_task`), approvals (3 actions — `install_packages`, `request_rebuild`, `add_mcp_server`), agent-to-agent (`create_agent`, and the agent-routing branch keyed as a pseudo-action `agent_route`).

-### 2. Router inbound gate
+### 2. Router sender resolver + access gate
+
+Two separate setters, called at different points in `routeInbound`. Preserves the pre-refactor ordering: sender-upsert side effects fire even when the message is ultimately dropped by wiring or trigger rules.

 ```typescript
 // src/router.ts
-type InboundGateResult =
-  | { allowed: true; userId: string | null }
-  | { allowed: false; userId: string | null; reason: string };
+type SenderResolverFn = (event: InboundEvent) => string | null;

-type InboundGateFn = (
+export function setSenderResolver(fn: SenderResolverFn): void;
+
+type AccessGateResult =
+  | { allowed: true }
+  | { allowed: false; reason: string };
+
+type AccessGateFn = (
  event: InboundEvent,
+  userId: string | null,
  mg: MessagingGroup,
  agentGroupId: string,
-) => InboundGateResult;
+) => AccessGateResult;

-export function setInboundGate(fn: InboundGateFn): void;
+export function setAccessGate(fn: AccessGateFn): void;
 ```

-**Purpose:** single-setter gate that owns both sender resolution (user upsert) and access decision. Takes the raw event because the permissions module needs the sender fields inside `event.message.content`.
+**Call order in `routeInbound`:**
+1. Resolve messaging group.
+2. **Sender resolver** (if set). Permissions upserts the users row here so the record exists even if agent resolution drops the message.
+3. Resolve wired agents; `no_agent_wired` → record + drop. (Core writes the dropped_messages row.)
+4. Pick agent by trigger rules; `no_trigger_match` → record + drop.
+5. **Access gate** (if set). On refusal it writes its own `dropped_messages` row keyed by policy reason.

-**Default when unset:** `{ allowed: true, userId: null }`. Every message routes through, no users table is needed, downstream must tolerate `userId=null`.
+**Defaults when unset:** resolver returns null; gate defaults to `{ allowed: true }`. Every message routes through, no users table is needed, downstream tolerates `userId=null`.

-**Current consumer:** permissions module.
+**Current consumer:** permissions module (registers both).

-**Not a registry, a setter.** There is one decision per inbound message and one module that owns it. Calling `setInboundGate` twice overwrites; core does not iterate.
+**Not registries, setters.** There is one sender and one access decision per inbound message and one module that owns both. Calling `setSenderResolver` / `setAccessGate` twice overwrites; core does not iterate.

 ### 3. Response dispatcher

--- a/src/modules/permissions/db/dropped-messages.ts
+++ b/src/modules/permissions/db/dropped-messages.ts
@@ -1,4 +1,4 @@
-import { getDb } from '../../../db/connection.js';
+import { getDb } from './connection.js';

 export interface UnregisteredSender {
  channel_type: string;
--- a/src/modules/permissions/index.ts
+++ b/src/modules/permissions/index.ts
@@ -1,25 +1,25 @@
 /**
 * Permissions module — sender resolution + access gate.
 *
- * Registers a single inbound-gate via setInboundGate(). The gate owns:
- *   1. Sender resolution: parse the channel adapter's payload, derive a
- *      namespaced user id, and upsert the `users` row on first sight so
- *      role/access lookups land on a real record thereafter.
- *   2. Access decision: owners → global admins → scoped admins → members.
- *   3. Unknown-sender policy: strict drops; request_approval is a TODO
- *      (pending the `add_group_member` action kind).
- *   4. Audit trail: drops get logged into `dropped_messages`.
+ * Registers two hooks into the core router:
+ *   1. setSenderResolver — runs before agent resolution. Parses the payload,
+ *      derives a namespaced user id, and upserts the `users` row on first
+ *      sight. Returns null when the payload doesn't carry enough to identify
+ *      a sender.
+ *   2. setAccessGate — runs after agent resolution. Enforces the
+ *      unknown_sender_policy (strict/request_approval/public) and the
+ *      owner/global-admin/scoped-admin/member access hierarchy. Records its
+ *      own `dropped_messages` row on refusal (structural drops are recorded
+ *      by core).
 *
- * Without this module: core's router defaults to allow-all (PR #2), every
- * message routes through, and no users table is needed. Drops are not
- * recorded anywhere. Admin commands inside the container fall back to
- * permissionless mode (see formatter.ts).
+ * Without this module: sender resolution is a no-op (userId=null); the
+ * access gate is not registered and core defaults to allow-all.
 */
-import { setInboundGate, type InboundEvent, type InboundGateResult } from '../../router.js';
+import { recordDroppedMessage } from '../../db/dropped-messages.js';
+import { setAccessGate, setSenderResolver, type AccessGateResult, type InboundEvent } from '../../router.js';
 import { log } from '../../log.js';
 import type { MessagingGroup } from '../../types.js';
 import { canAccessAgentGroup } from './access.js';
-import { recordDroppedMessage } from './db/dropped-messages.js';
 import { getUser, upsertUser } from './db/users.js';

 function extractAndUpsertUser(event: InboundEvent): string | null {
@@ -111,24 +111,24 @@ function handleUnknownSender(
  // 'public' should have been handled before the gate; fall through silently.
 }

-setInboundGate((event, mg, agentGroupId): InboundGateResult => {
-  const userId = extractAndUpsertUser(event);
+setSenderResolver(extractAndUpsertUser);

+setAccessGate((event, userId, mg, agentGroupId): AccessGateResult => {
  // Public channels skip the access check entirely.
  if (mg.unknown_sender_policy === 'public') {
-    return { allowed: true, userId };
+    return { allowed: true };
  }

  if (!userId) {
    handleUnknownSender(mg, null, agentGroupId, 'unknown_user', event);
-    return { allowed: false, userId: null, reason: 'unknown_user' };
+    return { allowed: false, reason: 'unknown_user' };
  }

  const decision = canAccessAgentGroup(userId, agentGroupId);
  if (decision.allowed) {
-    return { allowed: true, userId };
+    return { allowed: true };
  }

  handleUnknownSender(mg, userId, agentGroupId, decision.reason, event);
-  return { allowed: false, userId, reason: decision.reason };
+  return { allowed: false, reason: decision.reason };
 });
--- a/src/router.ts
+++ b/src/router.ts
@@ -1,16 +1,24 @@
 /**
 * Inbound message routing.
 *
- * Channel adapter event → resolve messaging group → pick agent → inbound
- * gate (if set) → resolve/create session → write messages_in → wake
- * container.
+ * Channel adapter event → resolve messaging group → sender resolver →
+ * resolve/pick agent → access gate → resolve/create session → write
+ * messages_in → wake container.
 *
- * Access model lives in the permissions module via `setInboundGate`. Without
- * the module, the gate is unset and every message routes through
- * (downstream code tolerates `userId=null`). Drops by policy are only
- * recorded when the permissions module is loaded; core just logs.
+ * Two module hooks (registered by the permissions module):
+ *   - `setSenderResolver` runs BEFORE agent resolution so user rows get
+ *     upserted even if the message ends up dropped by agent wiring.
+ *     Without the module, userId is null and downstream code tolerates it.
+ *   - `setAccessGate` runs AFTER agent resolution so policy decisions can
+ *     branch on the target agent group. Without the module, access is
+ *     allow-all.
+ *
+ * `dropped_messages` is core audit infra. Core writes rows for structural
+ * drops (no agent wired, no trigger match); the access gate writes rows
+ * for policy refusals.
 */
 import { getChannelAdapter } from './channels/channel-registry.js';
+import { recordDroppedMessage } from './db/dropped-messages.js';
 import { getMessagingGroupByPlatform, createMessagingGroup, getMessagingGroupAgents } from './db/messaging-groups.js';
 import { startTypingRefresh } from './modules/typing/index.js';
 import { log } from './log.js';
@@ -36,29 +44,57 @@ export interface InboundEvent {
 }

 /**
- * Inbound gate hook.
+ * Sender-resolver hook. Runs before agent resolution.
 *
- * The permissions module registers a gate that owns sender resolution +
- * access decision + unknown-sender policy + drop-audit recording. Without
- * a gate, core defaults to allow-all with `userId=null`.
- *
- * Takes the raw event so the gate can read sender fields from
- * `event.message.content`. Returns either allowed=true with a `userId`
- * (null if unresolved) or allowed=false with a reason; core drops on refusal.
+ * The permissions module registers this to extract the sender's namespaced
+ * user id and upsert the users row. Returns null when the payload doesn't
+ * carry enough info to identify a sender. Without the hook, every message
+ * arrives at the gate with userId=null.
 */
-export type InboundGateResult =
-  | { allowed: true; userId: string | null }
-  | { allowed: false; userId: string | null; reason: string };
+export type SenderResolverFn = (event: InboundEvent) => string | null;

-export type InboundGateFn = (event: InboundEvent, mg: MessagingGroup, agentGroupId: string) => InboundGateResult;
+let senderResolver: SenderResolverFn | null = null;

-let inboundGate: InboundGateFn | null = null;
-
-export function setInboundGate(fn: InboundGateFn): void {
-  if (inboundGate) {
-    log.warn('Inbound gate overwritten');
+export function setSenderResolver(fn: SenderResolverFn): void {
+  if (senderResolver) {
+    log.warn('Sender resolver overwritten');
+  }
+  senderResolver = fn;
+}
+
+/**
+ * Access-gate hook. Runs after agent resolution.
+ *
+ * The permissions module registers this; without it, core defaults to
+ * allow-all. The gate receives the raw event so it can extract the sender
+ * name for audit-trail purposes, and it is responsible for recording its
+ * own `dropped_messages` row on refusal (structural drops are already
+ * recorded by core before the gate runs).
+ */
+export type AccessGateResult = { allowed: true } | { allowed: false; reason: string };
+
+export type AccessGateFn = (
+  event: InboundEvent,
+  userId: string | null,
+  mg: MessagingGroup,
+  agentGroupId: string,
+) => AccessGateResult;
+
+let accessGate: AccessGateFn | null = null;
+
+export function setAccessGate(fn: AccessGateFn): void {
+  if (accessGate) {
+    log.warn('Access gate overwritten');
+  }
+  accessGate = fn;
+}
+
+function safeParseContent(raw: string): { text?: string; sender?: string; senderId?: string } {
+  try {
+    return JSON.parse(raw);
+  } catch {
+    return { text: raw };
  }
-  inboundGate = fn;
 }

 /**
@@ -95,7 +131,13 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
    });
  }

-  // 2. Resolve agent groups wired to this messaging group.
+  // 2. Sender resolution (permissions module upserts the users row as a
+  //    side effect so later role/access lookups find a real record).
+  //    Without the module, userId is null — downstream tolerates it.
+  const userId: string | null = senderResolver ? senderResolver(event) : null;
+
+  // 3. Resolve agent groups wired to this messaging group. Structural
+  //    drops record to dropped_messages for audit.
  const agents = getMessagingGroupAgents(mg.id);
  if (agents.length === 0) {
    log.warn('MESSAGE DROPPED — no agent groups wired to this channel. Run setup register step to configure.', {
@@ -103,6 +145,16 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
      channelType: event.channelType,
      platformId: event.platformId,
    });
+    const parsed = safeParseContent(event.message.content);
+    recordDroppedMessage({
+      channel_type: event.channelType,
+      platform_id: event.platformId,
+      user_id: userId,
+      sender_name: parsed.sender ?? null,
+      reason: 'no_agent_wired',
+      messaging_group_id: mg.id,
+      agent_group_id: null,
+    });
    return;
  }

@@ -112,17 +164,25 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
      messagingGroupId: mg.id,
      channelType: event.channelType,
    });
+    const parsed = safeParseContent(event.message.content);
+    recordDroppedMessage({
+      channel_type: event.channelType,
+      platform_id: event.platformId,
+      user_id: userId,
+      sender_name: parsed.sender ?? null,
+      reason: 'no_trigger_match',
+      messaging_group_id: mg.id,
+      agent_group_id: null,
+    });
    return;
  }

-  // 3. Inbound gate (if the permissions module is loaded). Otherwise
-  //    allow-all with userId=null — downstream code tolerates null.
-  let userId: string | null = null;
-  if (inboundGate) {
-    const result = inboundGate(event, mg, match.agent_group_id);
-    userId = result.userId;
+  // 4. Access gate (if the permissions module is loaded). Otherwise
+  //    allow-all.
+  if (accessGate) {
+    const result = accessGate(event, userId, mg, match.agent_group_id);
    if (!result.allowed) {
-      log.info('MESSAGE DROPPED — inbound gate refused', {
+      log.info('MESSAGE DROPPED — access gate refused', {
        messagingGroupId: mg.id,
        agentGroupId: match.agent_group_id,
        userId,
@@ -132,7 +192,7 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
    }
  }

-  // 4. Resolve or create session.
+  // 5. Resolve or create session.
  //
  // Adapter thread policy overrides the wiring's session_mode: if the adapter
  // is threaded, each thread gets its own session regardless of what the
@@ -148,7 +208,7 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
  }
  const { session, created } = resolveSession(match.agent_group_id, mg.id, event.threadId, effectiveSessionMode);

-  // 5. Write message to session DB
+  // 6. Write message to session DB
  writeSessionMessage(session.agent_group_id, session.id, {
    id: event.message.id || generateId(),
    kind: event.message.kind,
@@ -167,10 +227,10 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
    created,
  });

-  // 6. Show typing indicator while the agent processes.
+  // 7. Show typing indicator while the agent processes.
  startTypingRefresh(session.id, session.agent_group_id, event.channelType, event.platformId, event.threadId);

-  // 7. Wake container
+  // 8. Wake container
  const freshSession = getSession(session.id);
  if (freshSession) {
    await wakeContainer(freshSession);