Merge pull request #1847 from qwibitai/refactor/pr5-permissions

refactor(modules): extract permissions as optional module
2026-04-18 18:41:52 +03:00
parent e75af5e44d 32bcc2c5ae
commit c80a23e24f
14 changed files with 350 additions and 308 deletions
--- a/docs/module-contract.md
+++ b/docs/module-contract.md
@@ -4,23 +4,46 @@ This doc is the authoritative reference for how core and modules connect. Everyt

 ## Principles

- Core runs standalone. The `src/modules/index.ts` barrel can be empty and NanoClaw still routes messages in and delivers responses out.
- Modules are independent. No module imports from another module. Cross-module coordination goes through a core dispatcher.
+- Core runs standalone (modulo default modules — see tiers below). The optional-module portion of the `src/modules/index.ts` barrel can be empty and NanoClaw still routes messages in and delivers responses out.
+- Optional modules are independent. No optional module imports from another optional module. Cross-module coordination goes through a core registry (delivery action, response handler, etc.).
 - Registries exist only when multiple modules plug into the same decision point. Single-consumer integrations use skill edits (`MODULE-HOOK` markers) or stay inline with `sqlite_master` guards.
- Removing a module = delete files + remove barrel imports + revert any `MODULE-HOOK` content. Migration files stay (data is preserved).
+- Removing an optional module = delete files + remove barrel imports + revert any `MODULE-HOOK` content. Migration files stay (data is preserved). Removing a default module is more invasive: it requires editing the core files that import from it.

 ## Module taxonomy

-Three categories:
+Three categories. All three live under `src/modules/` (or equivalent adapter dirs) with the same folder layout; the distinction is about **shipping** and **who can depend on them**.

-1. **Default modules** — ship on `main`, live in `src/modules/` for signaling, core imports them directly. No hook, no registry. Removing requires editing core imports (deliberately less frictionless than registry modules — the friction signals "not really core, but you probably want it").
-2. **Registry-based modules** — live on the `modules` branch, installed via `/add-<name>` skills. Plug into core through one of the four registries below.
-3. **Channel adapters** — live on the `channels` branch, installed via `/add-<channel>` skills. Not covered by this contract; they use the pre-existing `ChannelAdapter` interface and `registerChannelAdapter()`.
+### 1. Default modules

-Current default modules:
+Ship with `main` in `src/modules/`. Imported by the default `src/modules/index.ts` barrel from day one. They are not really core — they live under `src/modules/` specifically to signal "not really core, rippable if needed" — but they're always present on a `main` install. Core imports from them directly. No hook, no registry indirection for the exports themselves.

- `src/modules/typing/` — typing indicator refresh
- `src/modules/mount-security/` — container mount allowlist validation
+Current: `typing`, `mount-security`.
+
+### 2. Optional modules
+
+Live on the `modules` branch. Installed via `/add-<name>` skills that cherry-pick files. Register into core via one of the four registries (or `MODULE-HOOK` skill edits). Core and other optional modules must not statically import an optional module's code.
+
+Current: `interactive`, `approvals`, `scheduling`, `permissions`. Pending: `agent-to-agent`.
+
+### 3. Channel adapters
+
+Live on the `channels` branch, installed via `/add-<channel>` skills. Not covered by this contract; they use the pre-existing `ChannelAdapter` interface and `registerChannelAdapter()`.
+
+## Dependency rule
+
+```
+core ← default modules ← optional modules
+```
+
+- **Core** may import from core and from default modules.
+- **Default modules** may import from core and from other default modules. They must not import from optional modules.
+- **Optional modules** may import from core and from default modules. They must not import from each other.
+
+Peer-to-peer coupling between optional modules goes through a core registry — see "The four registries" below. This keeps the module dependency graph a DAG and install order irrelevant.
+
+### Known transitional violations
+
+- `src/access.ts` (core) imports from `src/modules/permissions/` (optional). Shim left from PR #5; resolved in the planned approvals re-tier (PR #7) which moves approver-picking into a new default `approvals-primitive` module that may then depend on permissions however it likes — at which point `src/access.ts` ceases to exist.

 ## The four registries

@@ -45,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/access.test.ts
+++ b/src/access.test.ts
@@ -1,23 +1,15 @@
 import { beforeEach, afterEach, describe, expect, it } from 'vitest';

-import { canAccessAgentGroup, pickApprovalDelivery, pickApprover } from './access.js';
+import { pickApprovalDelivery, pickApprover } from './access.js';
 import type { ChannelAdapter, OutboundMessage } from './channels/adapter.js';
 import { initChannelAdapters, registerChannelAdapter, teardownChannelAdapters } from './channels/channel-registry.js';
-import {
-  addMember,
-  closeDb,
-  createAgentGroup,
-  createMessagingGroup,
-  createUser,
-  getUserDm,
-  grantRole,
-  hasAnyOwner,
-  initTestDb,
-  isMember,
-  isOwner,
-  runMigrations,
-} from './db/index.js';
-import { ensureUserDm } from './user-dm.js';
+import { closeDb, createAgentGroup, createMessagingGroup, initTestDb, runMigrations } from './db/index.js';
+import { canAccessAgentGroup } from './modules/permissions/access.js';
+import { addMember, isMember } from './modules/permissions/db/agent-group-members.js';
+import { createUser } from './modules/permissions/db/users.js';
+import { grantRole, hasAnyOwner, isOwner } from './modules/permissions/db/user-roles.js';
+import { getUserDm } from './modules/permissions/db/user-dms.js';
+import { ensureUserDm } from './modules/permissions/user-dm.js';

 function now(): string {
  return new Date().toISOString();
--- a/src/access.ts
+++ b/src/access.ts
@@ -1,56 +1,30 @@
 /**
- * Access control + approval routing.
+ * Approval routing helpers (temporary home).
 *
- * Privilege is user-level, not group-level. A user holds zero or more roles
- * (owner | admin) via `user_roles`, and is optionally "known" in specific
- * agent groups via `agent_group_members`. Admins are implicitly members of
- * the groups they administer.
+ * These functions pick an approver for a sensitive action and resolve the
+ * DM messaging_group they should be delivered to. They're called only from
+ * the approvals module.
 *
- * Sensitive actions trigger an approval flow, routed to the admin of the
- * originating agent group; if none, the owner. Approval delivery lands in
- * the approver's DM on (ideally) the same channel kind as the originating
- * request. DM resolution (including cold DMs) is handled by ensureUserDm.
+ * PR #5 moved the access-decision half of this file (canAccessAgentGroup +
+ * AccessDecision) into src/modules/permissions/. The approver-picking half
+ * stays here as a temporary shim — PR #7 relocates it into a new default
+ * approvals-primitive module alongside the approvals re-tier.
+ *
+ * Tier note: this file lives in core but imports from the permissions
+ * optional module. That's a deliberate temporary violation; see the module
+ * contract + REFACTOR_PLAN open question #3.
 */
-import { getAgentGroup } from './db/agent-groups.js';
-import { isMember } from './db/agent-group-members.js';
 import {
  getAdminsOfAgentGroup,
  getGlobalAdmins,
  getOwners,
-  hasAdminPrivilege,
-  isAdminOfAgentGroup,
-  isGlobalAdmin,
-  isOwner,
-} from './db/user-roles.js';
-import { getUser } from './db/users.js';
-import { ensureUserDm } from './user-dm.js';
+} from './modules/permissions/db/user-roles.js';
+import { ensureUserDm } from './modules/permissions/user-dm.js';
 import type { MessagingGroup } from './types.js';

-export type AccessDecision =
-  | { allowed: true; reason: 'owner' | 'global_admin' | 'admin_of_group' | 'member' }
-  | { allowed: false; reason: 'unknown_user' | 'not_member' };
-
-/** Can this user interact with this agent group? */
-export function canAccessAgentGroup(userId: string, agentGroupId: string): AccessDecision {
-  if (!getUser(userId)) return { allowed: false, reason: 'unknown_user' };
-  if (isOwner(userId)) return { allowed: true, reason: 'owner' };
-  if (isGlobalAdmin(userId)) return { allowed: true, reason: 'global_admin' };
-  if (isAdminOfAgentGroup(userId, agentGroupId)) return { allowed: true, reason: 'admin_of_group' };
-  if (isMember(userId, agentGroupId)) return { allowed: true, reason: 'member' };
-  return { allowed: false, reason: 'not_member' };
-}
-
-/** Can this user perform privileged (admin) operations on this agent group? */
-export function canAdminAgentGroup(userId: string, agentGroupId: string): boolean {
-  return hasAdminPrivilege(userId, agentGroupId);
-}
-
 /**
 * Ordered list of user IDs eligible to approve an action for the given agent
 * group. Preference: admins @ that group → global admins → owners.
- *
- * The approver-picking policy is to try local admins first (they have direct
- * context for the group), then fall back to global scope.
 */
 export function pickApprover(agentGroupId: string | null): string[] {
  const approvers: string[] = [];
@@ -100,15 +74,6 @@ export async function pickApprovalDelivery(
  return null;
 }

-/**
- * Resolve the agent group id for a session's originating request. Used by
- * approval routing so we know which scope to pick admins from.
- */
-export function agentGroupIdForSession(sessionAgentGroupId: string | null): string | null {
-  if (!sessionAgentGroupId) return null;
-  return getAgentGroup(sessionAgentGroupId)?.id ?? null;
-}
-
 function channelTypeOf(userId: string): string {
  const idx = userId.indexOf(':');
  return idx < 0 ? '' : userId.slice(0, idx);
--- a/src/container-runner.ts
+++ b/src/container-runner.ts
@@ -14,7 +14,6 @@ import { readContainerConfig, writeContainerConfig } from './container-config.js
 import { CONTAINER_RUNTIME_BIN, hostGatewayArgs, readonlyMountArgs, stopContainer } from './container-runtime.js';
 import { getAgentGroup } from './db/agent-groups.js';
 import { getDb, hasTable } from './db/connection.js';
-import { getAdminsOfAgentGroup, getGlobalAdmins, getOwners } from './db/user-roles.js';
 import { initGroupFilesystem } from './group-init.js';
 import { stopTypingRefresh } from './modules/typing/index.js';
 import { log } from './log.js';
@@ -288,14 +287,26 @@ async function buildContainerArgs(
  // Computed at wake time: owners + global admins + admins scoped to this
  // agent group. Role changes take effect on next container spawn.
  //
-  // Guarded: if the permissions module isn't installed, `user_roles`
-  // doesn't exist and the set stays empty — the formatter treats an
-  // empty admin set as permissionless (every sender is admin).
+  // SQL inlined to keep core independent of the permissions module — we
+  // guard on the `user_roles` table directly. If the permissions module
+  // isn't installed, the table doesn't exist and the set stays empty; the
+  // formatter treats an empty admin set as permissionless mode (every
+  // sender is admin).
  const adminUserIds = new Set<string>();
  if (hasTable(getDb(), 'user_roles')) {
-    for (const r of getOwners()) adminUserIds.add(r.user_id);
-    for (const r of getGlobalAdmins()) adminUserIds.add(r.user_id);
-    for (const r of getAdminsOfAgentGroup(agentGroup.id)) adminUserIds.add(r.user_id);
+    const db = getDb();
+    const owners = db
+      .prepare("SELECT user_id FROM user_roles WHERE role = 'owner' AND agent_group_id IS NULL")
+      .all() as Array<{ user_id: string }>;
+    const globalAdmins = db
+      .prepare("SELECT user_id FROM user_roles WHERE role = 'admin' AND agent_group_id IS NULL")
+      .all() as Array<{ user_id: string }>;
+    const scopedAdmins = db
+      .prepare("SELECT user_id FROM user_roles WHERE role = 'admin' AND agent_group_id = ?")
+      .all(agentGroup.id) as Array<{ user_id: string }>;
+    for (const r of owners) adminUserIds.add(r.user_id);
+    for (const r of globalAdmins) adminUserIds.add(r.user_id);
+    for (const r of scopedAdmins) adminUserIds.add(r.user_id);
  }
  if (adminUserIds.size > 0) {
    args.push('-e', `NANOCLAW_ADMIN_USER_IDS=${Array.from(adminUserIds).join(',')}`);
--- a/src/db/index.ts
+++ b/src/db/index.ts
@@ -8,22 +8,6 @@ export {
  updateAgentGroup,
  deleteAgentGroup,
 } from './agent-groups.js';
-export { createUser, upsertUser, getUser, getAllUsers, updateDisplayName, deleteUser } from './users.js';
-export {
-  grantRole,
-  revokeRole,
-  getUserRoles,
-  isOwner,
-  isGlobalAdmin,
-  isAdminOfAgentGroup,
-  hasAdminPrivilege,
-  getOwners,
-  hasAnyOwner,
-  getGlobalAdmins,
-  getAdminsOfAgentGroup,
-} from './user-roles.js';
-export { addMember, removeMember, getMembers, isMember, hasMembershipRow } from './agent-group-members.js';
-export { upsertUserDm, getUserDm, getUserDmsForUser, deleteUserDm } from './user-dms.js';
 export {
  createMessagingGroup,
  getMessagingGroup,
--- a/src/modules/index.ts
+++ b/src/modules/index.ts
@@ -16,4 +16,5 @@
 import './interactive/index.js';
 import './approvals/index.js';
 import './scheduling/index.js';
+import './permissions/index.js';

--- a/src/modules/permissions/access.ts
+++ b/src/modules/permissions/access.ts
@@ -0,0 +1,29 @@
+/**
+ * Access control (permissions module half of src/access.ts).
+ *
+ * Privilege is user-level, not group-level. A user holds zero or more roles
+ * (owner | admin) via `user_roles`, and is optionally "known" in specific
+ * agent groups via `agent_group_members`. Admins are implicitly members of
+ * the groups they administer.
+ *
+ * The approver-picking functions (pickApprover, pickApprovalDelivery) stay
+ * in src/access.ts for now — they move into the approvals module in the
+ * planned PR #7 re-tier.
+ */
+import { isMember } from './db/agent-group-members.js';
+import { isAdminOfAgentGroup, isGlobalAdmin, isOwner } from './db/user-roles.js';
+import { getUser } from './db/users.js';
+
+export type AccessDecision =
+  | { allowed: true; reason: 'owner' | 'global_admin' | 'admin_of_group' | 'member' }
+  | { allowed: false; reason: 'unknown_user' | 'not_member' };
+
+/** Can this user interact with this agent group? */
+export function canAccessAgentGroup(userId: string, agentGroupId: string): AccessDecision {
+  if (!getUser(userId)) return { allowed: false, reason: 'unknown_user' };
+  if (isOwner(userId)) return { allowed: true, reason: 'owner' };
+  if (isGlobalAdmin(userId)) return { allowed: true, reason: 'global_admin' };
+  if (isAdminOfAgentGroup(userId, agentGroupId)) return { allowed: true, reason: 'admin_of_group' };
+  if (isMember(userId, agentGroupId)) return { allowed: true, reason: 'member' };
+  return { allowed: false, reason: 'not_member' };
+}
--- a/src/modules/permissions/db/agent-group-members.ts
+++ b/src/modules/permissions/db/agent-group-members.ts
@@ -1,5 +1,5 @@
-import type { AgentGroupMember } from '../types.js';
-import { getDb } from './connection.js';
+import type { AgentGroupMember } from '../../../types.js';
+import { getDb } from '../../../db/connection.js';
 import { isAdminOfAgentGroup, isGlobalAdmin, isOwner } from './user-roles.js';

 export function addMember(row: AgentGroupMember): void {
--- a/src/modules/permissions/db/user-dms.ts
+++ b/src/modules/permissions/db/user-dms.ts
@@ -1,5 +1,5 @@
-import type { UserDm } from '../types.js';
-import { getDb } from './connection.js';
+import type { UserDm } from '../../../types.js';
+import { getDb } from '../../../db/connection.js';

 export function upsertUserDm(row: UserDm): void {
  getDb()
--- a/src/modules/permissions/db/user-roles.ts
+++ b/src/modules/permissions/db/user-roles.ts
@@ -1,5 +1,5 @@
-import type { UserRole, UserRoleKind } from '../types.js';
-import { getDb } from './connection.js';
+import type { UserRole, UserRoleKind } from '../../../types.js';
+import { getDb } from '../../../db/connection.js';

 /**
 * Grant a role. Owner rows must have agent_group_id = null (enforced here,
--- a/src/modules/permissions/db/users.ts
+++ b/src/modules/permissions/db/users.ts
@@ -1,5 +1,5 @@
-import type { User } from '../types.js';
-import { getDb } from './connection.js';
+import type { User } from '../../../types.js';
+import { getDb } from '../../../db/connection.js';

 export function createUser(user: User): void {
  getDb()
--- a/src/modules/permissions/index.ts
+++ b/src/modules/permissions/index.ts
@@ -0,0 +1,134 @@
+/**
+ * Permissions module — sender resolution + access gate.
+ *
+ * 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: sender resolution is a no-op (userId=null); the
+ * access gate is not registered and core defaults to allow-all.
+ */
+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 { getUser, upsertUser } from './db/users.js';
+
+function extractAndUpsertUser(event: InboundEvent): string | null {
+  let content: Record<string, unknown>;
+  try {
+    content = JSON.parse(event.message.content) as Record<string, unknown>;
+  } catch {
+    return null;
+  }
+
+  // chat-sdk-bridge serializes author info as a nested `author.userId` and
+  // does NOT populate top-level `senderId`. Older adapters (v1, native) put
+  // `senderId` or `sender` directly at the top level. Check all three.
+  const senderIdField = typeof content.senderId === 'string' ? content.senderId : undefined;
+  const senderField = typeof content.sender === 'string' ? content.sender : undefined;
+  const author =
+    typeof content.author === 'object' && content.author !== null
+      ? (content.author as Record<string, unknown>)
+      : undefined;
+  const authorUserId = typeof author?.userId === 'string' ? (author.userId as string) : undefined;
+  const senderName =
+    (typeof content.senderName === 'string' ? content.senderName : undefined) ??
+    (typeof author?.fullName === 'string' ? (author.fullName as string) : undefined) ??
+    (typeof author?.userName === 'string' ? (author.userName as string) : undefined);
+
+  const rawHandle = senderIdField ?? senderField ?? authorUserId;
+  if (!rawHandle) return null;
+
+  const userId = rawHandle.includes(':') ? rawHandle : `${event.channelType}:${rawHandle}`;
+  if (!getUser(userId)) {
+    upsertUser({
+      id: userId,
+      kind: event.channelType,
+      display_name: senderName ?? null,
+      created_at: new Date().toISOString(),
+    });
+  }
+  return userId;
+}
+
+function safeParseContent(raw: string): { text?: string; sender?: string; senderId?: string } {
+  try {
+    return JSON.parse(raw);
+  } catch {
+    return { text: raw };
+  }
+}
+
+function handleUnknownSender(
+  mg: MessagingGroup,
+  userId: string | null,
+  agentGroupId: string,
+  accessReason: string,
+  event: InboundEvent,
+): void {
+  const parsed = safeParseContent(event.message.content);
+  const dropRecord = {
+    channel_type: event.channelType,
+    platform_id: event.platformId,
+    user_id: userId,
+    sender_name: parsed.sender ?? null,
+    reason: `unknown_sender_${mg.unknown_sender_policy}`,
+    messaging_group_id: mg.id,
+    agent_group_id: agentGroupId,
+  };
+
+  if (mg.unknown_sender_policy === 'strict') {
+    log.info('MESSAGE DROPPED — unknown sender (strict policy)', {
+      messagingGroupId: mg.id,
+      agentGroupId,
+      userId,
+      accessReason,
+    });
+    recordDroppedMessage(dropRecord);
+    return;
+  }
+
+  if (mg.unknown_sender_policy === 'request_approval') {
+    log.info('MESSAGE DROPPED — unknown sender (approval flow TODO)', {
+      messagingGroupId: mg.id,
+      agentGroupId,
+      userId,
+      accessReason,
+    });
+    recordDroppedMessage(dropRecord);
+    return;
+  }
+
+  // 'public' should have been handled before the gate; fall through silently.
+}
+
+setSenderResolver(extractAndUpsertUser);
+
+setAccessGate((event, userId, mg, agentGroupId): AccessGateResult => {
+  // Public channels skip the access check entirely.
+  if (mg.unknown_sender_policy === 'public') {
+    return { allowed: true };
+  }
+
+  if (!userId) {
+    handleUnknownSender(mg, null, agentGroupId, 'unknown_user', event);
+    return { allowed: false, reason: 'unknown_user' };
+  }
+
+  const decision = canAccessAgentGroup(userId, agentGroupId);
+  if (decision.allowed) {
+    return { allowed: true };
+  }
+
+  handleUnknownSender(mg, userId, agentGroupId, decision.reason, event);
+  return { allowed: false, reason: decision.reason };
+});
--- a/src/modules/permissions/user-dm.ts
+++ b/src/modules/permissions/user-dm.ts
@@ -32,12 +32,12 @@
 * channel on repeated calls, so re-resolving after a cache miss is always
 * safe — worst case we round-trip redundantly.
 */
-import { getChannelAdapter } from './channels/channel-registry.js';
-import { getMessagingGroup, getMessagingGroupByPlatform, createMessagingGroup } from './db/messaging-groups.js';
+import { getChannelAdapter } from '../../channels/channel-registry.js';
+import { getMessagingGroup, getMessagingGroupByPlatform, createMessagingGroup } from '../../db/messaging-groups.js';
+import { log } from '../../log.js';
+import type { MessagingGroup, User } from '../../types.js';
 import { getUser } from './db/users.js';
 import { getUserDm, upsertUserDm } from './db/user-dms.js';
-import { log } from './log.js';
-import type { MessagingGroup, User } from './types.js';

 /**
 * Return a messaging_group usable to DM this user, creating it lazily if
--- a/src/router.ts
+++ b/src/router.ts
@@ -1,32 +1,30 @@
 /**
- * Inbound message routing for v2.
+ * Inbound message routing.
 *
- * Channel adapter event → resolve messaging group → access gate → resolve
- * agent group → 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.
 *
- * Privilege / access model:
- *   - Owners and global admins: always allowed
- *   - Scoped admins: allowed in their agent group
- *   - Known members (agent_group_members row): allowed in that agent group
- *   - Everyone else: message is dropped per `messaging_groups.unknown_sender_policy`
- *     (strict / request_approval / public)
+ * 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.
 *
- * Sender normalization: we derive a namespaced user id from the message
- * content. This is best-effort — native adapters put `sender` in content,
- * chat-sdk-bridge adapters put `senderId`. Adapters should populate both
- * wherever possible so the gate can land on a real user row.
+ * `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 { canAccessAgentGroup } from './access.js';
 import { getChannelAdapter } from './channels/channel-registry.js';
-import { isMember } from './db/agent-group-members.js';
+import { recordDroppedMessage } from './db/dropped-messages.js';
 import { getMessagingGroupByPlatform, createMessagingGroup, getMessagingGroupAgents } from './db/messaging-groups.js';
-import { upsertUser, getUser } from './db/users.js';
 import { startTypingRefresh } from './modules/typing/index.js';
 import { log } from './log.js';
 import { resolveSession, writeSessionMessage } from './session-manager.js';
 import { wakeContainer } from './container-runner.js';
 import { getSession } from './db/sessions.js';
-import { recordDroppedMessage } from './db/dropped-messages.js';
 import type { MessagingGroup, MessagingGroupAgent } from './types.js';

 function generateId(): string {
@@ -46,31 +44,57 @@ export interface InboundEvent {
 }

 /**
- * Inbound gate registry.
+ * Sender-resolver hook. Runs before agent resolution.
 *
- * A module (permissions, today) can register a single gate function that
- * owns sender resolution + access decision. Without a registered gate,
- * core falls back to the inline `extractAndUpsertUser` +
- * `enforceAccess` + `handleUnknownSender` chain.
- *
- * 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 the
- * message 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;
 }

 /**
@@ -79,9 +103,7 @@ export function setInboundGate(fn: InboundGateFn): void {
 */
 export async function routeInbound(event: InboundEvent): Promise<void> {
  // 0. Apply the adapter's thread policy. Non-threaded adapters (Telegram,
-  //    WhatsApp, iMessage, email) collapse threads to the channel — the
-  //    agent always replies to the main channel regardless of where the
-  //    inbound came from.
+  //    WhatsApp, iMessage, email) collapse threads to the channel.
  const adapter = getChannelAdapter(event.channelType);
  if (adapter && !adapter.supportsThreads) {
    event = { ...event, threadId: null };
@@ -109,8 +131,13 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
    });
  }

-  // 2. Resolve agent groups wired to this messaging group. (The gate runs
-  //    after this so it can decide based on the target agent 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.', {
@@ -122,7 +149,7 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
    recordDroppedMessage({
      channel_type: event.channelType,
      platform_id: event.platformId,
-      user_id: parsed.senderId ?? null,
+      user_id: userId,
      sender_name: parsed.sender ?? null,
      reason: 'no_agent_wired',
      messaging_group_id: mg.id,
@@ -131,7 +158,6 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
    return;
  }

-  // Pick the best matching agent (highest priority, trigger matching in future)
  const match = pickAgent(agents, event);
  if (!match) {
    log.warn('MESSAGE DROPPED — no agent matched trigger rules', {
@@ -142,7 +168,7 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
    recordDroppedMessage({
      channel_type: event.channelType,
      platform_id: event.platformId,
-      user_id: parsed.senderId ?? null,
+      user_id: userId,
      sender_name: parsed.sender ?? null,
      reason: 'no_trigger_match',
      messaging_group_id: mg.id,
@@ -151,15 +177,12 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
    return;
  }

-  // 3. Inbound gate: sender resolution + access decision. If a module
-  //    registered a gate, it owns the whole thing (it can upsert users,
-  //    check roles, etc.). Otherwise fall back to the inline chain.
-  let userId: string | 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,
@@ -167,23 +190,13 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
      });
      return;
    }
-  } else {
-    userId = extractAndUpsertUser(event);
-    if (mg.unknown_sender_policy !== 'public') {
-      const gate = enforceAccess(userId, match.agent_group_id);
-      if (!gate.allowed) {
-        handleUnknownSender(mg, userId, match.agent_group_id, gate.reason, event);
-        return;
-      }
-    }
  }

  // 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
-  // wiring says, because "thread = session" is the first-class model for
-  // threaded platforms. Agent-shared is preserved because it expresses a
+  // wiring says. Agent-shared is preserved because it expresses a
  // cross-channel intent the adapter can't know about.
  //
  // Exception: DMs (is_group=0). Sub-threads within a DM are a UX affordance,
@@ -214,13 +227,7 @@ export async function routeInbound(event: InboundEvent): Promise<void> {
    created,
  });

-  // 7. Show typing indicator while the agent processes. Refresh on a short
-  // interval so platforms like Discord (which auto-expire typing after
-  // ~10s) keep showing it for the full thinking window. Gated on the
-  // heartbeat file's mtime after an initial grace period, so typing stops
-  // as soon as the agent goes idle — not when the container eventually
-  // exits. Container-runner also calls stopTypingRefresh on exit as a
-  // fast-path cleanup.
+  // 7. Show typing indicator while the agent processes.
  startTypingRefresh(session.id, session.agent_group_id, event.channelType, event.platformId, event.threadId);

  // 8. Wake container
@@ -239,119 +246,3 @@ function pickAgent(agents: MessagingGroupAgent[], _event: InboundEvent): Messagi
  // TODO: apply trigger_rules matching (pattern, mentionOnly, etc.)
  return agents[0] ?? null;
 }
-
-/**
- * Best-effort sender extraction. Returns a namespaced user id like
- * `telegram:123` or null if nothing usable is present.
- *
- * Side-effect: upserts the user into the `users` table so access/approval
- * lookups can find them on subsequent messages.
- *
- * The namespace uses the channel_type as `kind` for now — e.g. `whatsapp:...`
- * rather than `phone:...`. That's imprecise (a phone number is really the
- * identifier, not the channel) but it keeps the first cut simple. A proper
- * kind mapping (channel → kind) can happen when we start linking identities
- * across channels.
- */
-function extractAndUpsertUser(event: InboundEvent): string | null {
-  let content: Record<string, unknown>;
-  try {
-    content = JSON.parse(event.message.content) as Record<string, unknown>;
-  } catch {
-    return null;
-  }
-
-  // chat-sdk-bridge serializes author info as a nested `author.userId` and
-  // does NOT populate top-level `senderId`. Older adapters (v1, native) put
-  // `senderId` or `sender` directly at the top level. Check all three.
-  const senderIdField = typeof content.senderId === 'string' ? content.senderId : undefined;
-  const senderField = typeof content.sender === 'string' ? content.sender : undefined;
-  const author =
-    typeof content.author === 'object' && content.author !== null
-      ? (content.author as Record<string, unknown>)
-      : undefined;
-  const authorUserId = typeof author?.userId === 'string' ? (author.userId as string) : undefined;
-  const senderName =
-    (typeof content.senderName === 'string' ? content.senderName : undefined) ??
-    (typeof author?.fullName === 'string' ? (author.fullName as string) : undefined) ??
-    (typeof author?.userName === 'string' ? (author.userName as string) : undefined);
-
-  const rawHandle = senderIdField ?? senderField ?? authorUserId;
-  if (!rawHandle) return null;
-
-  // If the raw handle already contains ':' it's pre-namespaced (the older
-  // adapters put it in that form). Otherwise prepend the channel type.
-  const userId = rawHandle.includes(':') ? rawHandle : `${event.channelType}:${rawHandle}`;
-  if (!getUser(userId)) {
-    upsertUser({
-      id: userId,
-      kind: event.channelType,
-      display_name: senderName ?? null,
-      created_at: new Date().toISOString(),
-    });
-  }
-  return userId;
-}
-
-function enforceAccess(userId: string | null, agentGroupId: string): { allowed: boolean; reason: string } {
-  if (!userId) return { allowed: false, reason: 'unknown_user' };
-  const decision = canAccessAgentGroup(userId, agentGroupId);
-  if (decision.allowed) return { allowed: true, reason: decision.reason };
-  return { allowed: false, reason: decision.reason };
-}
-
-function handleUnknownSender(
-  mg: MessagingGroup,
-  userId: string | null,
-  agentGroupId: string,
-  accessReason: string,
-  event: InboundEvent,
-): void {
-  const parsed = safeParseContent(event.message.content);
-  const dropRecord = {
-    channel_type: event.channelType,
-    platform_id: event.platformId,
-    user_id: userId,
-    sender_name: parsed.sender ?? null,
-    reason: `unknown_sender_${mg.unknown_sender_policy}`,
-    messaging_group_id: mg.id,
-    agent_group_id: agentGroupId,
-  };
-
-  // In 'strict' mode we just drop. In 'request_approval' mode we log and
-  // queue an approval to add the sender as a member — the approval flow
-  // itself is a follow-up (needs an action kind like `add_group_member`).
-  if (mg.unknown_sender_policy === 'strict') {
-    log.info('MESSAGE DROPPED — unknown sender (strict policy)', {
-      messagingGroupId: mg.id,
-      agentGroupId,
-      userId,
-      accessReason,
-    });
-    recordDroppedMessage(dropRecord);
-    return;
-  }
-
-  if (mg.unknown_sender_policy === 'request_approval') {
-    log.info('MESSAGE DROPPED — unknown sender (approval flow TODO)', {
-      messagingGroupId: mg.id,
-      agentGroupId,
-      userId,
-      accessReason,
-    });
-    recordDroppedMessage(dropRecord);
-    return;
-  }
-
-  // Should be unreachable — 'public' was handled before the gate.
-  // Ensure the membership invariant isn't in an odd state.
-  void isMember;
-}
-
-function safeParseContent(raw: string): { text?: string; sender?: string; senderId?: string } {
-  try {
-    return JSON.parse(raw);
-  } catch {
-    return { text: raw };
-  }
-}