refactor(guard): unwrap the broadcast registries — response handlers and interceptors consult inline

registerResponseHandler and registerMessageInterceptor return to main's
single-argument shape: the guard-spec/unguarded declaration machinery
(claims, onDeny, the registry wrappers) is deleted. The declaration
requirement stays where registration is keyed — every ncl command derives
its guard inside register(), and registerDeliveryAction demands a spec or
unguarded(<reason>) — while broadcast hooks gate inline at the point of
privilege, like the a2a route and the unknown-sender gate:

- handleChannelApprovalResponse consults guard(channelsRegister) inline
  where main had the click-auth if — same decision, same timing.
- The free-text name capture returns to main verbatim: the click arms it,
  the reply is not re-authorized (the body's own pending-row re-fetch
  still refuses a vanished registration). Behavior delta (c) is withdrawn
  — the deliberate deltas are back to (a) approve-then-revoke and
  (b) grant refusals.

Also gone with the wrappers: the claims/body predicate duplication (the
same early-exits existed twice and could drift apart) and the repeated
extractAndUpsertUser upsert per captured event.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Moshe Krupper
2026-07-11 22:16:17 +03:00
parent a9e5231039
commit fe235c7ca0
12 changed files with 47 additions and 161 deletions
+1 -1
View File
@@ -4,7 +4,7 @@ All notable changes to NanoClaw will be documented in this file.
## [Unreleased]
- **The guard seam (guarded-actions phase 2).** Every privileged action crossing the container or channel boundary now passes ONE decision function — `guard()` in the new `src/guard/` leaf — before it executes: `allow`, `hold` (through the flow's existing approval mechanics), or `deny`. The four handler registries (ncl commands, delivery actions, response handlers, message interceptors) wrap their handlers at registration, so the guarded path is the only path by construction: every registration takes a guard spec or an explicit `unguarded(<reason>)` declaration — omission doesn't compile, and `grep "unguarded("` is the complete justified inventory. Consults carry the branded `GuardedAction` value returned by `defineGuardedAction`, so a typo'd or dropped wiring is a compile error and a forged value is denied at runtime — never a fail-open. The structural baseline is today's checks verbatim (cli_scope enforcement, create_agent's scope branch, a2a destination ACL + `agent_message_policies` hold, self-mod unconditional hold, unknown_sender_policy, channel-registration click auth, host-as-trusted-caller in code). Policy-as-data (tighten-only rule sources composing with the baseline) is deferred to phase 3, where the generalized rules table arrives with its first operator-visible consumer. Approved replays now carry the verified approval row as a **grant** — the `approved: true` boolean is deleted — and the guard re-runs the structural baseline on every replay. Three outcome changes are inherent to that replay semantics and ship deliberately: **(a)** approving a held a2a message after its destination was revoked no longer delivers it (the live baseline re-check); **(b)** a forged, already-consumed, or wrong-action/wrong-payload grant refuses the replay instead of executing (previously any in-process caller passing `approved: true` executed); **(c)** the channel-registration free-text name reply re-checks approver eligibility at reply time — a privilege revoked between the click and the reply, or a registration that vanished meanwhile, now refuses instead of acting. Zero rules ship by default — the seeded posture is today's behavior. (The D1/D2/D4 click-auth fixes from the guarded-actions defect inventory are NOT in this change — they belong to the approval-contract PR.)
- **The guard seam (guarded-actions phase 2).** Every privileged action crossing the container or channel boundary now passes ONE decision function — `guard()` in the new `src/guard/` leaf — before it executes: `allow`, `hold` (through the flow's existing approval mechanics), or `deny`. The keyed registries (ncl commands, delivery actions) wrap their handlers at registration, so the guarded path is the only path by construction: every ncl command derives its guard from its own definition inside `register()`, and a delivery action registers with a guard spec or an explicit `unguarded(<reason>)` declaration — omission doesn't compile, and `grep "unguarded("` is the complete justified inventory. The broadcast hooks (response handlers, message interceptors) stay plain registrations; the one privileged click path — channel registration — consults the guard inline in its handler, like the a2a route and the unknown-sender gate. Consults carry the branded `GuardedAction` value returned by `defineGuardedAction`, so a typo'd or dropped wiring is a compile error and a forged value is denied at runtime — never a fail-open. The structural baseline is today's checks verbatim (cli_scope enforcement, create_agent's scope branch, a2a destination ACL + `agent_message_policies` hold, self-mod unconditional hold, unknown_sender_policy, channel-registration click auth, host-as-trusted-caller in code). Policy-as-data (tighten-only rule sources composing with the baseline) is deferred to phase 3, where the generalized rules table arrives with its first operator-visible consumer. Approved replays now carry the verified approval row as a **grant** — the `approved: true` boolean is deleted — and the guard re-runs the structural baseline on every replay. Two outcome changes are inherent to that replay semantics and ship deliberately: **(a)** approving a held a2a message after its destination was revoked no longer delivers it (the live baseline re-check); **(b)** a forged, already-consumed, or wrong-action/wrong-payload grant refuses the replay instead of executing (previously any in-process caller passing `approved: true` executed). Zero rules ship by default — the seeded posture is today's behavior. (The D1/D2/D4 click-auth fixes from the guarded-actions defect inventory are NOT in this change — they belong to the approval-contract PR.)
- **Guard conformance is now a boot invariant, not just a CI test.** The host refuses to start (upgrade-tripwire posture: clear banner + exit 1) when a guarded action that can hold has no registered approve continuation — the one cross-registry invariant the type system can't see, since catalog entries and approval handlers register from different modules. CI can't see skill-installed code — `/add-*` skills register handlers on machines where the test suite never runs — so the check (`src/guard-conformance.ts`) runs in `main()` before the host accepts a message, and a mis-composition crashes at skill-install time instead of at the first approved card. The old registry walk is deleted: everything it detected is now unconstructible at the registration API. Companion hardening: re-registering a guard-wrapped delivery action *without* a guard spec now throws at registration — previously that silently replaced the guarded handler while the catalog still reported the action as guarded.
- **Pre-task script failures back their series off instead of spinning.** A `--script` that errors lands the occurrence as a failed run (`script-skip:error` ack → `failed` status); recurrence reads the series' trailing failed streak and re-arms at `max(cron next, now + 2·2^(n1) min, cap 60)`; after 8 consecutive failures the series is auto-paused with a host-written note in its run log (`ncl tasks resume` revives it). A deliberate `wakeAgent:false` gate is a normal run and never backs off. Also fixed: an explicitly-addressed `<message to>` in a task fire's final text now delivers as a deliberate send (previously suppressed as a turn-reply echo → zero delivery when the agent skipped the MCP tool); identical echoes of an MCP send are dropped in the runner, where the duplication originates.
- [BREAKING] **Scheduled tasks moved from MCP tools to `ncl tasks`.** The six scheduling MCP tools are no longer exposed to agent containers; agents and operators manage tasks with `ncl tasks list/get/create/update/cancel/pause/resume/delete`. New tasks run from a per-agent-group system session rather than waking the chat session that created them, and task writes are not approval-gated inside the owning group. **Migration:** [docs/ncl-tasks-migration.md](docs/ncl-tasks-migration.md).
+1 -1
View File
@@ -66,7 +66,7 @@ For ad-hoc queries from skills or scripts, use the in-tree wrapper rather than t
| `src/session-manager.ts` | Resolves sessions; opens `inbound.db` / `outbound.db`; manages heartbeat path |
| `src/container-runner.ts` | Spawns per-agent-group Docker containers with session DB + outbox mounts, OneCLI `ensureAgent` |
| `src/container-runtime.ts` | Docker CLI wrapper (runtime binary, host-gateway args, mount args), orphan cleanup |
| `src/guard/` | Privileged-action decision seam: `guard(action, input)` → allow \| hold \| deny. Domain-free leaf; domain baselines are `defineGuardedAction` values exported from module-edge `guard.ts` adapters (cli, agent-to-agent, self-mod, permissions) — consults hold the value, so a missing/typo'd wiring is a compile error, never a fail-open. All four handler registries require a guard spec or an explicit `unguarded(<reason>)` declaration at the registration site. Approved replays carry the approval row as a grant and re-check the structural baseline. Boot check = grant continuations only (`src/guard-conformance.ts`); Policy-as-data is deferred to guarded-actions phase 3. Conformance test: `src/guard/conformance.test.ts` |
| `src/guard/` | Privileged-action decision seam: `guard(action, input)` → allow \| hold \| deny. Domain-free leaf; domain baselines are `defineGuardedAction` values exported from module-edge `guard.ts` adapters (cli, agent-to-agent, self-mod, permissions) — consults hold the value, so a missing/typo'd wiring is a compile error, never a fail-open. The keyed registries (ncl commands, delivery actions) demand a guard at registration (derived spec or explicit `unguarded(<reason>)`); broadcast hooks (response handlers, message interceptors) consult the guard inline where privileged. Approved replays carry the approval row as a grant and re-check the structural baseline. Boot check = grant continuations only (`src/guard-conformance.ts`); Policy-as-data is deferred to guarded-actions phase 3. Conformance test: `src/guard/conformance.test.ts` |
| `src/modules/permissions/access.ts` | `canAccessAgentGroup` — owner / global admin / scoped admin / member resolution against `user_roles` + `agent_group_members` |
| `src/modules/approvals/primitive.ts` | `pickApprover`, `pickApprovalDelivery`, `requestApproval`, approval-handler registry |
| `src/command-gate.ts` | Router-side admin command gate — queries `user_roles` directly (no env var, no container-side check) |
+2 -2
View File
@@ -402,8 +402,8 @@ async function deliverMessage(
* (allow / hold / deny), so there is no unguarded route to it. On approve,
* the continuation re-enters the same entry carrying the approval row as its
* grant (`reenterGuardedDeliveryAction`), so the structural baseline is
* re-checked live. Plain actions (scheduling self-actions, the cli_request
* bridge — its inner commands are guarded at dispatch) register with an
* re-checked live. Plain actions (the cli_request bridge — its inner
* commands are guarded at dispatch) register with an
* explicit `unguarded(<reason>)` declaration instead of a spec — omission is
* not representable, so the decision to run unguarded is visible, and
* justified, at the registration site.
+4 -4
View File
@@ -8,10 +8,10 @@
* GuardedAction VALUE returned by defineGuardedAction, so a dropped
* module-edge import or a typo'd action is a compile error (and a forged
* value is denied at runtime), not a silent allow.
* - A handler cannot register unguarded by omission every registry
* (delivery actions, response handlers, interceptors, CLI commands)
* requires a guard spec or an explicit unguarded(<reason>) declaration
* at the registration site.
* - A privileged handler cannot register unguarded by omission the
* delivery-action registry requires a guard spec or an explicit
* unguarded(<reason>) declaration, and every ncl command derives its
* guard inside register().
*
* What remains is completeness ACROSS registries: a guarded action that
* holds via `approvalAction` needs a registered approval handler, or an
+6 -5
View File
@@ -33,11 +33,12 @@ export interface GuardInput {
const unguardedBrand = Symbol('unguarded');
/**
* A registration that deliberately carries no guard. Omission is not
* representable every registry requires either a guard spec or this
* marker, so the decision to run unguarded is visible, and justified, in
* the diff that registers the handler. The reason travels with the
* registration; `grep "unguarded("` is the complete inventory.
* A registration that deliberately carries no guard. Where a registry takes
* a declaration (delivery actions), omission is not representable
* registration requires either a guard spec or this marker, so the decision
* to run unguarded is visible, and justified, in the diff that registers
* the handler. The reason travels with the registration;
* `grep "unguarded("` is the complete inventory.
*/
export type Unguarded = { readonly reason: string; readonly [unguardedBrand]: true };
+1 -5
View File
@@ -23,7 +23,6 @@
* + approval handlers via this module's public API.
*/
import { onDeliveryAdapterReady } from '../../delivery.js';
import { unguarded } from '../../guard/index.js';
import { registerResponseHandler, onShutdown } from '../../response-registry.js';
import { handleApprovalsResponse } from './response-handler.js';
import { startOneCLIApprovalHandler, stopOneCLIApprovalHandler } from './onecli-approvals.js';
@@ -35,10 +34,7 @@ export type { ApprovalHandler, ApprovalHandlerContext, RequestApprovalOptions }
// loads reason-capture.js, registering its message-interceptor on import.
export { sweepAwaitingReasonRejects } from './reason-capture.js';
registerResponseHandler(
handleApprovalsResponse,
unguarded('self-authorizing — resolves each pending_approvals row against its own approver rules before dispatching'),
);
registerResponseHandler(handleApprovalsResponse);
onDeliveryAdapterReady((adapter) => {
startOneCLIApprovalHandler(adapter);
+1 -5
View File
@@ -20,7 +20,6 @@
*/
import type { InboundEvent } from '../../channels/adapter.js';
import { getDeliveryAdapter } from '../../delivery.js';
import { unguarded } from '../../guard/index.js';
import {
deletePendingApproval,
getExpiredAwaitingReasonApprovals,
@@ -152,10 +151,7 @@ export async function captureReasonReply(event: InboundEvent): Promise<boolean>
return true;
}
registerMessageInterceptor(
captureReasonReply,
unguarded('self-authorizing — only captures a DM from the admin whose reject click armed it (keyed by userId + DM)'),
);
registerMessageInterceptor(captureReasonReply);
/**
* Host-sweep finalizer: any reject-with-reason hold whose window elapsed (admin
+1 -5
View File
@@ -13,7 +13,6 @@
import { getDb, hasTable } from '../../db/connection.js';
import { deletePendingQuestion, getPendingQuestion, getSession } from '../../db/sessions.js';
import { wakeContainer } from '../../container-runner.js';
import { unguarded } from '../../guard/index.js';
import { registerResponseHandler, type ResponsePayload } from '../../response-registry.js';
import { log } from '../../log.js';
import { writeSessionMessage } from '../../session-manager.js';
@@ -57,7 +56,4 @@ async function handleInteractiveResponse(payload: ResponsePayload): Promise<bool
return true;
}
registerResponseHandler(
handleInteractiveResponse,
unguarded('not privileged — relays an in-chat answer back into the asking session; only the question row is touched'),
);
registerResponseHandler(handleInteractiveResponse);
@@ -467,8 +467,8 @@ describe('unknown-channel registration flow', () => {
if (claimed) break;
}
// Owner replies with the agent name in the same DM — the guarded
// interceptor allows (still an eligible approver) and creates.
// Owner replies with the agent name in the same DM — the interceptor
// captures it and creates.
await routeInbound({
channelType: 'telegram',
platformId: 'dm-owner',
@@ -520,7 +520,7 @@ describe('unknown-channel registration flow', () => {
}
// The registration disappears between the click and the reply (rejected
// from another card, group delete cascade, …) — the guard's baseline no
// from another card, group delete cascade, …) — the interceptor no
// longer finds a pending registration, so the reply must not create.
getDb()
.prepare('DELETE FROM pending_channel_approvals WHERE messaging_group_id = ?')
+20 -35
View File
@@ -32,7 +32,7 @@ import { registerResponseHandler, type ResponsePayload } from '../../response-re
import { getDeliveryAdapter } from '../../delivery.js';
import { log } from '../../log.js';
import type { MessagingGroup, MessagingGroupAgent } from '../../types.js';
import { guard, unguarded } from '../../guard/index.js';
import { guard } from '../../guard/index.js';
import { channelsRegister, sendersAdmit } from './guard.js';
import { canAccessAgentGroup } from './access.js';
import {
@@ -293,12 +293,7 @@ async function handleSenderApprovalResponse(payload: ResponsePayload): Promise<b
return true;
}
registerResponseHandler(
handleSenderApprovalResponse,
unguarded(
'self-authorizing — verifies the clicker inline (delivered approver or group admin) before adding a member',
),
);
registerResponseHandler(handleSenderApprovalResponse);
// ── Unknown-channel registration flow ──
@@ -324,14 +319,26 @@ async function handleChannelApprovalResponse(payload: ResponsePayload): Promise<
const row = getPendingChannelApproval(payload.questionId);
if (!row) return false;
// Click authorization is the guard's channels.register baseline (./guard.ts),
// consulted by the response-registry wrapper before this handler runs.
// Click authorization is the guard's channels.register baseline (./guard.ts):
// the delivered approver, or an admin of the pending row's anchor agent group.
const clickerId = payload.userId
? payload.userId.includes(':')
? payload.userId
: `${payload.channelType}:${payload.userId}`
: null;
if (!clickerId) return true; // unreachable behind the guard wrapper; fail closed
const decision = guard(channelsRegister, {
actor: { kind: 'human', userId: clickerId ?? '' },
payload: { questionId: payload.questionId },
});
if (!clickerId || decision.effect !== 'allow') {
log.warn('Channel registration click rejected — unauthorized clicker', {
messagingGroupId: row.messaging_group_id,
clickerId,
expectedApprover: row.approver_user_id,
reason: decision.reason,
});
return true;
}
const approverId = clickerId;
// ── Reject / Cancel ──
@@ -521,19 +528,13 @@ async function handleChannelApprovalResponse(payload: ResponsePayload): Promise<
return true;
}
registerResponseHandler(handleChannelApprovalResponse, {
action: channelsRegister,
claims: (payload) => getPendingChannelApproval(payload.questionId) !== undefined,
});
registerResponseHandler(handleChannelApprovalResponse);
// ── Free-text name interceptor ──
// Captures the next DM from an approver who clicked "Create new agent",
// creates the agent immediately, wires the channel, and replays. The router
// wraps it with the guard: the free-texter must still be an eligible
// channel-registration approver at reply time — a privilege revoked between
// the click and the reply now denies, and the arming is disarmed.
// creates the agent immediately, wires the channel, and replays.
const captureAgentNameReply = async (event: InboundEvent): Promise<boolean> => {
registerMessageInterceptor(async (event: InboundEvent): Promise<boolean> => {
const userId = extractAndUpsertUser(event);
if (!userId) return false;
@@ -641,20 +642,4 @@ const captureAgentNameReply = async (event: InboundEvent): Promise<boolean> => {
}
}
return true;
};
registerMessageInterceptor(captureAgentNameReply, {
action: channelsRegister,
claims: (event) => {
const userId = extractAndUpsertUser(event);
if (!userId) return null;
const pending = awaitingNameInput.get(userId);
if (!pending) return null;
if (event.channelType !== pending.dmChannelType || event.platformId !== pending.dmPlatformId) return null;
return { actor: { kind: 'human', userId }, payload: { questionId: pending.channelMgId } };
},
onDeny: (event) => {
const userId = extractAndUpsertUser(event);
if (userId) awaitingNameInput.delete(userId);
},
});
+5 -52
View File
@@ -7,21 +7,10 @@
* which triggers module registrations that would otherwise happen before
* index.ts's own const initializers have run.
*
* Keep this file dependency-free (log.js and the guard leaf are 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.
*
* A handler whose click performs a privileged operation registers with a
* guard spec: the registry wraps it so the guard's decision stands between
* the click and the handler, and the wrapped path is the only path. `claims`
* is the handler's own claim test (does this questionId belong to me?) so an
* unauthorized click is claimed-and-dropped without stealing other handlers'
* responses. The guard argument is not optional a handler that runs
* unguarded must declare so with `unguarded(<reason>)`, at the registration
* site, in the diff that adds it.
* 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.
*/
import { guard, isUnguarded, type GuardActor, type GuardedAction, type Unguarded } from './guard/index.js';
import { log } from './log.js';
export interface ResponsePayload {
questionId: string;
@@ -34,46 +23,10 @@ export interface ResponsePayload {
export type ResponseHandler = (payload: ResponsePayload) => Promise<boolean>;
export interface ResponseGuardSpec {
/** Guard action consulted before the handler runs — the defined value, not a name. */
action: GuardedAction;
/** Would this handler claim the response? (Its own row lookup.) */
claims: (payload: ResponsePayload) => boolean;
}
const responseHandlers: ResponseHandler[] = [];
function responseActor(payload: ResponsePayload): GuardActor {
if (!payload.userId) return { kind: 'human', userId: '' };
const userId = payload.userId.includes(':') ? payload.userId : `${payload.channelType}:${payload.userId}`;
return { kind: 'human', userId };
}
export function registerResponseHandler(handler: ResponseHandler, guardDecl: ResponseGuardSpec | Unguarded): void {
if (isUnguarded(guardDecl)) {
// Explicitly declared unguarded — the carried reason is the reviewable record.
responseHandlers.push(handler);
return;
}
const spec = guardDecl;
responseHandlers.push(async (payload) => {
if (!spec.claims(payload)) return false;
const decision = guard(spec.action, {
actor: responseActor(payload),
payload: { questionId: payload.questionId, value: payload.value },
});
if (decision.effect !== 'allow') {
// Claim the response so it's not unclaimed-logged, but do nothing.
log.warn('Response click rejected by guard', {
action: spec.action.action,
questionId: payload.questionId,
userId: payload.userId,
reason: decision.reason,
});
return true;
}
return handler(payload);
});
export function registerResponseHandler(handler: ResponseHandler): void {
responseHandlers.push(handler);
}
export function getResponseHandlers(): readonly ResponseHandler[] {
+2 -43
View File
@@ -20,7 +20,6 @@
import { getChannelAdapter } from './channels/channel-registry.js';
import { gateCommand } from './command-gate.js';
import { getAgentGroup } from './db/agent-groups.js';
import { guard, isUnguarded, type GuardActor, type GuardedAction, type Unguarded } from './guard/index.js';
import { recordDroppedMessage } from './db/dropped-messages.js';
import {
createMessagingGroup,
@@ -118,53 +117,13 @@ export function setSenderScopeGate(fn: SenderScopeGateFn): void {
* Used by modules to capture free-text DM replies during multi-step approval
* flows the permissions module (agent naming during channel registration)
* and the approvals module (reject-with-reason capture).
*
* An interceptor whose capture performs a privileged operation (the
* channel-registration name capture creates an agent group + wiring)
* registers with a guard spec: the registry wraps it so the guard's decision
* stands between the free-text reply and the handler. `claims` returns the
* guard consult for events the interceptor would act on (null = not mine,
* pass through); a deny consumes the message without acting. The guard
* argument is not optional an interceptor that runs unguarded must declare
* so with `unguarded(<reason>)` at the registration site.
*/
export type MessageInterceptorFn = (event: InboundEvent) => Promise<boolean>;
export interface InterceptorGuardSpec {
/** Guard action consulted before the interceptor acts — the defined value, not a name. */
action: GuardedAction;
/** The guard consult for events this interceptor would act on; null = not mine. */
claims: (event: InboundEvent) => { actor: GuardActor; payload: Record<string, unknown> } | null;
/** Domain cleanup when the guard denies (e.g. disarm the capture). */
onDeny?: (event: InboundEvent) => void;
}
const messageInterceptors: MessageInterceptorFn[] = [];
export function registerMessageInterceptor(
fn: MessageInterceptorFn,
guardDecl: InterceptorGuardSpec | Unguarded,
): void {
if (isUnguarded(guardDecl)) {
// Explicitly declared unguarded — the carried reason is the reviewable record.
messageInterceptors.push(fn);
return;
}
const guardSpec = guardDecl;
messageInterceptors.push(async (event) => {
const consult = guardSpec.claims(event);
if (!consult) return fn(event);
const decision = guard(guardSpec.action, { actor: consult.actor, payload: consult.payload });
if (decision.effect !== 'allow') {
log.warn('Interceptor capture rejected by guard — consuming without acting', {
action: guardSpec.action.action,
reason: decision.reason,
});
guardSpec.onDeny?.(event);
return true;
}
return fn(event);
});
export function registerMessageInterceptor(fn: MessageInterceptorFn): void {
messageInterceptors.push(fn);
}
/**