From fe235c7ca09d9c33fcf6cebc57dda1f852f91022 Mon Sep 17 00:00:00 2001 From: Moshe Krupper Date: Sat, 11 Jul 2026 22:16:17 +0300 Subject: [PATCH] =?UTF-8?q?refactor(guard):=20unwrap=20the=20broadcast=20r?= =?UTF-8?q?egistries=20=E2=80=94=20response=20handlers=20and=20interceptor?= =?UTF-8?q?s=20consult=20inline?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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() — 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 --- CHANGELOG.md | 2 +- CLAUDE.md | 2 +- src/delivery.ts | 4 +- src/guard-conformance.ts | 8 +-- src/guard/types.ts | 11 ++-- src/modules/approvals/index.ts | 6 +- src/modules/approvals/reason-capture.ts | 6 +- src/modules/interactive/index.ts | 6 +- .../permissions/channel-approval.test.ts | 6 +- src/modules/permissions/index.ts | 55 +++++++----------- src/response-registry.ts | 57 ++----------------- src/router.ts | 45 +-------------- 12 files changed, 47 insertions(+), 161 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 01b6c0281..a8b95d150 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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()` 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()` 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^(n−1) 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 `` 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). diff --git a/CLAUDE.md b/CLAUDE.md index 325d8d06d..742380bd8 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -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()` 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()`); 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) | diff --git a/src/delivery.ts b/src/delivery.ts index eab594dbf..311c99567 100644 --- a/src/delivery.ts +++ b/src/delivery.ts @@ -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()` declaration instead of a spec — omission is * not representable, so the decision to run unguarded is visible, and * justified, at the registration site. diff --git a/src/guard-conformance.ts b/src/guard-conformance.ts index 4f7a3fbcd..904563f79 100644 --- a/src/guard-conformance.ts +++ b/src/guard-conformance.ts @@ -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() 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() 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 diff --git a/src/guard/types.ts b/src/guard/types.ts index 93499a547..4e03b643b 100644 --- a/src/guard/types.ts +++ b/src/guard/types.ts @@ -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 }; diff --git a/src/modules/approvals/index.ts b/src/modules/approvals/index.ts index 924145b91..8dcb274f5 100644 --- a/src/modules/approvals/index.ts +++ b/src/modules/approvals/index.ts @@ -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); diff --git a/src/modules/approvals/reason-capture.ts b/src/modules/approvals/reason-capture.ts index 03dcf6074..79724a277 100644 --- a/src/modules/approvals/reason-capture.ts +++ b/src/modules/approvals/reason-capture.ts @@ -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 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 diff --git a/src/modules/interactive/index.ts b/src/modules/interactive/index.ts index 3aada6fa1..324adbe7e 100644 --- a/src/modules/interactive/index.ts +++ b/src/modules/interactive/index.ts @@ -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 { 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 = ?') diff --git a/src/modules/permissions/index.ts b/src/modules/permissions/index.ts index abc64f6d8..15daec1f8 100644 --- a/src/modules/permissions/index.ts +++ b/src/modules/permissions/index.ts @@ -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 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 => { +registerMessageInterceptor(async (event: InboundEvent): Promise => { const userId = extractAndUpsertUser(event); if (!userId) return false; @@ -641,20 +642,4 @@ const captureAgentNameReply = async (event: InboundEvent): Promise => { } } 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); - }, }); diff --git a/src/response-registry.ts b/src/response-registry.ts index 59c8ba884..60e04c998 100644 --- a/src/response-registry.ts +++ b/src/response-registry.ts @@ -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()`, 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; -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[] { diff --git a/src/router.ts b/src/router.ts index 6b7e33ffa..d39c6a064 100644 --- a/src/router.ts +++ b/src/router.ts @@ -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()` at the registration site. */ export type MessageInterceptorFn = (event: InboundEvent) => Promise; -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 } | 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); } /**