diff --git a/apps/.i18n/native-source.json b/apps/.i18n/native-source.json index a21d44e43754..fc7abde9acb7 100644 --- a/apps/.i18n/native-source.json +++ b/apps/.i18n/native-source.json @@ -371,7 +371,7 @@ }, { "kind": "ui-state-text", - "line": 698, + "line": 701, "path": "apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt", "source": "Offline", "surface": "android", @@ -379,7 +379,7 @@ }, { "kind": "conditional-branch", - "line": 1628, + "line": 1631, "path": "apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt", "source": "Cron job started.", "surface": "android", @@ -387,7 +387,7 @@ }, { "kind": "conditional-branch", - "line": 1628, + "line": 1631, "path": "apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt", "source": "Cron run queued.", "surface": "android", @@ -395,7 +395,7 @@ }, { "kind": "conditional-branch", - "line": 1672, + "line": 1675, "path": "apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt", "source": "Cron job disabled.", "surface": "android", @@ -403,7 +403,7 @@ }, { "kind": "conditional-branch", - "line": 1672, + "line": 1675, "path": "apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt", "source": "Cron job enabled.", "surface": "android", @@ -411,7 +411,7 @@ }, { "kind": "conditional-branch", - "line": 3279, + "line": 3282, "path": "apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt", "source": "Failed: no secure gateway endpoint was detected. Enable gateway TLS or Tailscale Serve, or use a trusted private LAN address with Unencrypted selected.", "surface": "android", @@ -419,7 +419,7 @@ }, { "kind": "conditional-branch", - "line": 3281, + "line": 3284, "path": "apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt", "source": "Failed: secure endpoint reached, but TLS fingerprint verification timed out. Check Tailscale Serve or gateway TLS and retry.", "surface": "android", @@ -427,7 +427,7 @@ }, { "kind": "conditional-branch", - "line": 3283, + "line": 3286, "path": "apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt", "source": "Failed: couldn't reach the secure gateway endpoint for this host.", "surface": "android", @@ -435,7 +435,7 @@ }, { "kind": "conditional-branch", - "line": 4056, + "line": 4059, "path": "apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt", "source": "Update your Gateway to view provider model config.", "surface": "android", @@ -443,7 +443,7 @@ }, { "kind": "conditional-branch", - "line": 4058, + "line": 4061, "path": "apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt", "source": "Could not load provider model config.", "surface": "android", diff --git a/apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt b/apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt index 4f1c01fb0f26..4bcd993746d3 100644 --- a/apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt +++ b/apps/android/app/src/main/java/ai/openclaw/app/NodeRuntime.kt @@ -408,6 +408,9 @@ class NodeRuntime private constructor( val stableId: String, val id: String, val decision: String, + // Captured at registration: canonical readback needs it after a refresh has + // already replaced the visible rows, or the legacy get parse drops the row. + val createdAtMs: Long?, ) { @Volatile var requestInFlight: Boolean = true } @@ -5016,7 +5019,13 @@ class NodeRuntime private constructor( val currentRows = _execApprovals.value if (currentRows.none { it.id == id && it.resolvingDecision == null }) return@synchronized if (pendingExecApprovalWrites.containsKey(id)) return@synchronized - val pendingWrite = PendingExecApprovalWrite(gatewayScope.stableId, id, decision) + val pendingWrite = + PendingExecApprovalWrite( + gatewayScope.stableId, + id, + decision, + currentRows.firstOrNull { it.id == id }?.createdAtMs, + ) pendingExecApprovalWrites[id] = pendingWrite registeredWrite = pendingWrite invalidateExecApprovalRefreshes() @@ -5232,7 +5241,9 @@ class NodeRuntime private constructor( gatewayScope = gatewayScope, methodsSnapshot = methodsSnapshot, id = pendingWrite.id, - createdAtMs = _execApprovals.value.firstOrNull { it.id == pendingWrite.id }?.createdAtMs, + createdAtMs = + pendingWrite.createdAtMs + ?: _execApprovals.value.firstOrNull { it.id == pendingWrite.id }?.createdAtMs, ) } catch (_: Throwable) { return diff --git a/apps/android/app/src/test/java/ai/openclaw/app/GatewayExecApprovalRuntimeTest.kt b/apps/android/app/src/test/java/ai/openclaw/app/GatewayExecApprovalRuntimeTest.kt index edc3692e77d3..ea25083bae41 100644 --- a/apps/android/app/src/test/java/ai/openclaw/app/GatewayExecApprovalRuntimeTest.kt +++ b/apps/android/app/src/test/java/ai/openclaw/app/GatewayExecApprovalRuntimeTest.kt @@ -292,7 +292,7 @@ class GatewayExecApprovalRuntimeTest { } runtime.resolveExecApproval("approval-1", "deny") - withTimeout(2_000) { resolveStarted.await() } + withTimeout(10_000) { resolveStarted.await() } // GatewaySession runs onDisconnected before failing the retired socket's // request waiters. Recreate that production ordering on the same stable ID. @@ -346,7 +346,7 @@ class GatewayExecApprovalRuntimeTest { } runtime.resolveExecApproval("approval-1", "deny") - withTimeout(2_000) { resolveStarted.await() } + withTimeout(10_000) { resolveStarted.await() } runtime.refreshExecApprovals() withTimeout(2_000) { refreshReadCompleted.await() } waitUntil { !runtime.execApprovalsRefreshing.value } @@ -418,7 +418,7 @@ class GatewayExecApprovalRuntimeTest { } runtime.resolveExecApproval("approval-1", "deny") - withTimeout(2_000) { resolveStarted.await() } + withTimeout(10_000) { resolveStarted.await() } runtime.refreshExecApprovals() withTimeout(2_000) { retainedReadStarted.await() } @@ -604,7 +604,7 @@ class GatewayExecApprovalRuntimeTest { } runtime.resolveExecApproval("approval-1", "allow-once") - withTimeout(2_000) { resolveStarted.await() } + withTimeout(10_000) { resolveStarted.await() } invokeApprovalEvent( runtime, "exec.approval.resolved", @@ -653,7 +653,7 @@ class GatewayExecApprovalRuntimeTest { } runtime.resolveExecApproval("approval-1", "allow-once") - withTimeout(2_000) { resolveStarted.await() } + withTimeout(10_000) { resolveStarted.await() } invokeApprovalEvent( runtime, "exec.approval.resolved", @@ -756,7 +756,7 @@ class GatewayExecApprovalRuntimeTest { } runtime.resolveExecApproval("approval-1", "allow-once") - withTimeout(2_000) { resolveStarted.await() } + withTimeout(10_000) { resolveStarted.await() } // Replacement hello on the same stable endpoint: the epoch bump makes the // already-resolved publish a no-op, leaving only the pending-write record. invokeReplaceGatewayMethods(runtime, legacyMethods) @@ -1178,7 +1178,8 @@ class GatewayExecApprovalRuntimeTest { ) private suspend fun waitUntil(condition: () -> Boolean) { - withTimeout(3_000) { + // Generous ceiling for loaded CI runners; passing tests exit on first poll. + withTimeout(10_000) { while (!condition()) delay(10) } } diff --git a/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift b/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift index b19ed87f0abd..02e12af4f282 100644 --- a/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift +++ b/apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift @@ -107,6 +107,26 @@ public enum ApprovalAllowDecision: String, Codable, Sendable { case allowAlways = "allow-always" } +public enum ApprovalAllowedReason: String, Codable, Sendable { + case user = "user" +} + +public enum ApprovalDeniedReason: String, Codable, Sendable { + case user = "user" + case malformedVerdict = "malformed-verdict" + case noRoute = "no-route" + case storageCorrupt = "storage-corrupt" +} + +public enum ApprovalExpiredReason: String, Codable, Sendable { + case timeout = "timeout" +} + +public enum ApprovalCancelledReason: String, Codable, Sendable { + case runAborted = "run-aborted" + case gatewayRestart = "gateway-restart" +} + public enum PluginApprovalSeverity: String, Codable, Sendable { case info = "info" case warning = "warning" @@ -4004,18 +4024,22 @@ public struct SessionsSendParams: Codable, Sendable { public struct SessionsMessagesSubscribeParams: Codable, Sendable { public let key: String public let agentid: String? + public let includeapprovals: Bool? public init( key: String, - agentid: String? = nil) + agentid: String? = nil, + includeapprovals: Bool? = nil) { self.key = key self.agentid = agentid + self.includeapprovals = includeapprovals } private enum CodingKeys: String, CodingKey { case key case agentid = "agentId" + case includeapprovals = "includeApprovals" } } @@ -10104,9 +10128,9 @@ public struct AllowedApprovalSnapshot: Codable, Sendable { public let expiresatms: Int public let presentation: ApprovalPresentation public let resolvedatms: Int - public let reason: ApprovalTerminalReason public let status: String public let decision: ApprovalAllowDecision + public let reason: ApprovalAllowedReason public init( id: String, @@ -10115,9 +10139,9 @@ public struct AllowedApprovalSnapshot: Codable, Sendable { expiresatms: Int, presentation: ApprovalPresentation, resolvedatms: Int, - reason: ApprovalTerminalReason, status: String, - decision: ApprovalAllowDecision) + decision: ApprovalAllowDecision, + reason: ApprovalAllowedReason) { self.id = id self.urlpath = urlpath @@ -10125,9 +10149,9 @@ public struct AllowedApprovalSnapshot: Codable, Sendable { self.expiresatms = expiresatms self.presentation = presentation self.resolvedatms = resolvedatms - self.reason = reason self.status = status self.decision = decision + self.reason = reason } private enum CodingKeys: String, CodingKey { @@ -10137,9 +10161,9 @@ public struct AllowedApprovalSnapshot: Codable, Sendable { case expiresatms = "expiresAtMs" case presentation case resolvedatms = "resolvedAtMs" - case reason case status case decision + case reason } } @@ -10150,9 +10174,9 @@ public struct DeniedApprovalSnapshot: Codable, Sendable { public let expiresatms: Int public let presentation: ApprovalPresentation public let resolvedatms: Int - public let reason: ApprovalTerminalReason public let status: String public let decision: String + public let reason: ApprovalDeniedReason public init( id: String, @@ -10161,9 +10185,9 @@ public struct DeniedApprovalSnapshot: Codable, Sendable { expiresatms: Int, presentation: ApprovalPresentation, resolvedatms: Int, - reason: ApprovalTerminalReason, status: String, - decision: String) + decision: String, + reason: ApprovalDeniedReason) { self.id = id self.urlpath = urlpath @@ -10171,9 +10195,9 @@ public struct DeniedApprovalSnapshot: Codable, Sendable { self.expiresatms = expiresatms self.presentation = presentation self.resolvedatms = resolvedatms - self.reason = reason self.status = status self.decision = decision + self.reason = reason } private enum CodingKeys: String, CodingKey { @@ -10183,9 +10207,9 @@ public struct DeniedApprovalSnapshot: Codable, Sendable { case expiresatms = "expiresAtMs" case presentation case resolvedatms = "resolvedAtMs" - case reason case status case decision + case reason } } @@ -10196,8 +10220,8 @@ public struct ExpiredApprovalSnapshot: Codable, Sendable { public let expiresatms: Int public let presentation: ApprovalPresentation public let resolvedatms: Int - public let reason: ApprovalTerminalReason public let status: String + public let reason: ApprovalExpiredReason public init( id: String, @@ -10206,8 +10230,8 @@ public struct ExpiredApprovalSnapshot: Codable, Sendable { expiresatms: Int, presentation: ApprovalPresentation, resolvedatms: Int, - reason: ApprovalTerminalReason, - status: String) + status: String, + reason: ApprovalExpiredReason) { self.id = id self.urlpath = urlpath @@ -10215,8 +10239,8 @@ public struct ExpiredApprovalSnapshot: Codable, Sendable { self.expiresatms = expiresatms self.presentation = presentation self.resolvedatms = resolvedatms - self.reason = reason self.status = status + self.reason = reason } private enum CodingKeys: String, CodingKey { @@ -10226,8 +10250,8 @@ public struct ExpiredApprovalSnapshot: Codable, Sendable { case expiresatms = "expiresAtMs" case presentation case resolvedatms = "resolvedAtMs" - case reason case status + case reason } } @@ -10238,8 +10262,8 @@ public struct CancelledApprovalSnapshot: Codable, Sendable { public let expiresatms: Int public let presentation: ApprovalPresentation public let resolvedatms: Int - public let reason: ApprovalTerminalReason public let status: String + public let reason: ApprovalCancelledReason public init( id: String, @@ -10248,8 +10272,8 @@ public struct CancelledApprovalSnapshot: Codable, Sendable { expiresatms: Int, presentation: ApprovalPresentation, resolvedatms: Int, - reason: ApprovalTerminalReason, - status: String) + status: String, + reason: ApprovalCancelledReason) { self.id = id self.urlpath = urlpath @@ -10257,8 +10281,8 @@ public struct CancelledApprovalSnapshot: Codable, Sendable { self.expiresatms = expiresatms self.presentation = presentation self.resolvedatms = resolvedatms - self.reason = reason self.status = status + self.reason = reason } private enum CodingKeys: String, CodingKey { @@ -10268,8 +10292,8 @@ public struct CancelledApprovalSnapshot: Codable, Sendable { case expiresatms = "expiresAtMs" case presentation case resolvedatms = "resolvedAtMs" - case reason case status + case reason } } @@ -10341,6 +10365,92 @@ public struct ApprovalResolveResult: Codable, Sendable { } } +public struct PendingSessionApprovalEvent: Codable, Sendable { + public let sessionkey: String + public let sourcesessionkey: String? + public let updatedatms: Int + public let phase: String + public let approval: PendingApprovalSnapshot + + public init( + sessionkey: String, + sourcesessionkey: String? = nil, + updatedatms: Int, + phase: String, + approval: PendingApprovalSnapshot) + { + self.sessionkey = sessionkey + self.sourcesessionkey = sourcesessionkey + self.updatedatms = updatedatms + self.phase = phase + self.approval = approval + } + + private enum CodingKeys: String, CodingKey { + case sessionkey = "sessionKey" + case sourcesessionkey = "sourceSessionKey" + case updatedatms = "updatedAtMs" + case phase + case approval + } +} + +public struct TerminalSessionApprovalEvent: Codable, Sendable { + public let sessionkey: String + public let sourcesessionkey: String? + public let updatedatms: Int + public let phase: String + public let approval: TerminalApprovalSnapshot + + public init( + sessionkey: String, + sourcesessionkey: String? = nil, + updatedatms: Int, + phase: String, + approval: TerminalApprovalSnapshot) + { + self.sessionkey = sessionkey + self.sourcesessionkey = sourcesessionkey + self.updatedatms = updatedatms + self.phase = phase + self.approval = approval + } + + private enum CodingKeys: String, CodingKey { + case sessionkey = "sessionKey" + case sourcesessionkey = "sourceSessionKey" + case updatedatms = "updatedAtMs" + case phase + case approval + } +} + +public struct SessionApprovalReplay: Codable, Sendable { + public let sessionkey: String + public let updatedatms: Int + public let approvals: [PendingApprovalSnapshot] + public let truncated: Bool + + public init( + sessionkey: String, + updatedatms: Int, + approvals: [PendingApprovalSnapshot], + truncated: Bool) + { + self.sessionkey = sessionkey + self.updatedatms = updatedatms + self.approvals = approvals + self.truncated = truncated + } + + private enum CodingKeys: String, CodingKey { + case sessionkey = "sessionKey" + case updatedatms = "updatedAtMs" + case approvals + case truncated + } +} + public struct ExecApprovalsGetParams: Codable, Sendable {} public struct ExecApprovalsSetParams: Codable, Sendable { @@ -12321,6 +12431,37 @@ public enum TerminalApprovalSnapshot: Codable, Sendable { } } +public enum SessionApprovalEvent: Codable, Sendable { + case pending(PendingSessionApprovalEvent) + case terminal(TerminalSessionApprovalEvent) + + private enum CodingKeys: String, CodingKey { + case discriminator = "phase" + } + + public init(from decoder: Decoder) throws { + let container = try decoder.container(keyedBy: CodingKeys.self) + let discriminator = try container.decode(String.self, forKey: .discriminator) + switch discriminator { + case "pending": self = try .pending(PendingSessionApprovalEvent(from: decoder)) + case "terminal": self = try .terminal(TerminalSessionApprovalEvent(from: decoder)) + default: + throw DecodingError.dataCorruptedError( + forKey: .discriminator, + in: container, + debugDescription: "Unknown SessionApprovalEvent discriminator value" + ) + } + } + + public func encode(to encoder: Encoder) throws { + switch self { + case .pending(let value): try value.encode(to: encoder) + case .terminal(let value): try value.encode(to: encoder) + } + } +} + public enum PluginCatalogInstallAction: Codable, Sendable { case clawhub(PluginCatalogClawHubInstall) case official(PluginCatalogOfficialInstall) diff --git a/docs/.generated/plugin-sdk-api-baseline.sha256 b/docs/.generated/plugin-sdk-api-baseline.sha256 index f312aeedd658..894f9cf5e59b 100644 --- a/docs/.generated/plugin-sdk-api-baseline.sha256 +++ b/docs/.generated/plugin-sdk-api-baseline.sha256 @@ -1,2 +1,2 @@ -13fd6a689ea1e559f3e5f0b67463d407a8050679c1b50597789dffb4daefa71b plugin-sdk-api-baseline.json -e593b1cad8dd37c4349b075c6ca1e4cd0c508933c3cca92c0c672bc2fc65ea66 plugin-sdk-api-baseline.jsonl +0400bc7c83db141e6692b5806e0d29daa35436b3363ce7a2666428cd115977f9 plugin-sdk-api-baseline.json +6697812cc06ed03c0fc737c842e9fbd79e1e7057dd0ee72cf5b1c81aab589318 plugin-sdk-api-baseline.jsonl diff --git a/docs/docs_map.md b/docs/docs_map.md index f7440859ad64..4d4e964d0d85 100644 --- a/docs/docs_map.md +++ b/docs/docs_map.md @@ -8328,7 +8328,7 @@ Do not edit it by hand; run `pnpm docs:map:gen`. - H1: Multi-surface operator approvals - H2: Goals - H2: Non-goals - - H2: Existing system and evidence map + - H2: Pre-rollout baseline and evidence map - H2: Prior art - H2: Architecture and ownership - H2: Persistent record @@ -8346,7 +8346,9 @@ Do not edit it by hand; run `pnpm docs:map:gen`. - H3: PR 2: typed actions and channel callbacks - H3: PR 3: Control UI deep link - H3: PR 4: native clients - - H3: PR 5: propagation and fail-closed behavior + - H3: PR 5: ancestor lifecycle propagation + - H3: PR 6: fail-closed behavior + - H3: Follow-up: durable remote-message cleanup - H2: Tests - H2: Observability - H2: Open decisions diff --git a/docs/gateway/index.md b/docs/gateway/index.md index 2948500a8fdd..2ecf2b8e2f39 100644 --- a/docs/gateway/index.md +++ b/docs/gateway/index.md @@ -304,9 +304,9 @@ Defaults include isolated state/config and base gateway port `19001`. a generated dump of every callable helper route. - Requests: `req(method, params)` → `res(ok/payload|error)`. - Common events include `connect.challenge`, `agent`, `chat`, - `session.message`, `session.operation`, `session.tool`, `sessions.changed`, - `presence`, `tick`, `health`, `heartbeat`, pairing/approval lifecycle events, - and `shutdown`. + `session.message`, `session.operation`, `session.tool`, opt-in + `session.approval`, `sessions.changed`, `presence`, `tick`, `health`, + `heartbeat`, pairing/approval lifecycle events, and `shutdown`. Agent runs are two-stage: diff --git a/docs/gateway/protocol.md b/docs/gateway/protocol.md index 07962dfe6a4e..e0ec6687b97c 100644 --- a/docs/gateway/protocol.md +++ b/docs/gateway/protocol.md @@ -514,7 +514,7 @@ methods. Treat this as feature discovery, not a full enumeration of - `sessions.list` returns the current session index, including per-row `agentRuntime` metadata when an agent runtime backend is configured. - `sessions.subscribe` and `sessions.unsubscribe` toggle session change event subscriptions for the current WS client. - - `sessions.messages.subscribe` and `sessions.messages.unsubscribe` toggle transcript/message event subscriptions for one session. + - `sessions.messages.subscribe` and `sessions.messages.unsubscribe` toggle transcript/message event subscriptions for one session. Pass `includeApprovals: true` to also receive sanitized `session.approval` lifecycle events for approvals whose persisted audience includes that exact session and whose reviewer binding authorizes the subscribing client. The subscribe response then includes a bounded pending `approvalReplay`; it is authoritative when `truncated` is false. The opt-in is per subscribe call, not sticky: re-subscribing to the same session without `includeApprovals: true` removes an existing approval subscription. In addition to normal session-read authority, this opt-in requires `operator.admin`, or `operator.approvals` on a paired device. - `sessions.preview` returns bounded transcript previews for specific session keys. - `sessions.describe` returns one gateway session row for an exact session key. - `sessions.resolve` resolves or canonicalizes a session target. @@ -587,6 +587,9 @@ methods. Treat this as feature discovery, not a full enumeration of `replace=true` and use `deltaText` as the replacement text. - `session.message`, `session.operation`, `session.tool`: transcript, in-flight session operation, and event-stream updates for a subscribed session. +- `session.approval`: sanitized pending and terminal approval truth for an + explicitly opted-in exact-session subscriber. Child approvals use the + persisted ancestor audience; events never mutate transcripts or wake agents. - `sessions.changed`: session index or metadata changed. - `presence`: system presence snapshot updates. - `tick`: periodic keepalive/liveness event. diff --git a/docs/plugins/hooks.md b/docs/plugins/hooks.md index 5bb9c74a207c..77d2b9999138 100644 --- a/docs/plugins/hooks.md +++ b/docs/plugins/hooks.md @@ -40,7 +40,6 @@ export default definePluginEntry({ description: `Allow search query: ${String(event.params.query ?? "")}`, severity: "info", timeoutMs: 60_000, - timeoutBehavior: "deny", }, }; }, @@ -251,6 +250,7 @@ type BeforeToolCallResult = { description: string; severity?: "info" | "warning" | "critical"; timeoutMs?: number; + /** @deprecated Unresolved approvals always deny. */ timeoutBehavior?: "allow" | "deny"; allowedDecisions?: Array<"allow-once" | "allow-always" | "deny">; pluginId?: string; diff --git a/docs/plugins/plugin-permission-requests.md b/docs/plugins/plugin-permission-requests.md index 58d9019a2f95..c7f8247eca47 100644 --- a/docs/plugins/plugin-permission-requests.md +++ b/docs/plugins/plugin-permission-requests.md @@ -63,7 +63,6 @@ export default definePluginEntry({ ? ["allow-once", "deny"] : ["allow-once", "allow-always", "deny"], timeoutMs: 120_000, - timeoutBehavior: "deny", onResolution(decision) { console.log(`deploy approval resolved: ${decision}`); }, @@ -99,10 +98,15 @@ available approval surfaces, and waits for a decision. | `allow-once` | The current call continues. | | `allow-always` | The current call continues and the decision is passed to the plugin. | | `deny` | The call is blocked with a denied tool result. | -| Timeout | The call is blocked unless `timeoutBehavior` is `"allow"`. | +| Timeout | The call is blocked. | | Cancellation | The call is blocked when the run is aborted. | | No approval route | The call is blocked because no connected approval surface can resolve it. | +Only the exact `allow-once` and `allow-always` decisions permitted by the +request allow execution. Unknown, malformed, mismatched, missing, and timed-out +decisions fail closed. The legacy `timeoutBehavior` field remains accepted for +plugin compatibility but is deprecated and ignored; do not set it in new hooks. + `allow-always` is only durable when the requesting plugin or runtime implements that persistence. For ordinary `before_tool_call.requireApproval` hooks, OpenClaw treats `allow-once` and `allow-always` as approval decisions for the diff --git a/docs/refactor/operator-approvals.md b/docs/refactor/operator-approvals.md index 52ca5f6901a0..48859b2256f3 100644 --- a/docs/refactor/operator-approvals.md +++ b/docs/refactor/operator-approvals.md @@ -35,16 +35,18 @@ Inline actions and deep links coexist. There is no approval-mode toggle. - Redesigning exec allowlists, plugin policy composition, or `allow-always` persistence except where required to make terminal outcomes unambiguous. - Making a gatewayless embedded TUI remotely reachable in the first increment. It remains local-only and must fail closed when no reviewer exists. -## Existing system and evidence map +## Pre-rollout baseline and evidence map -| Surface | Current entry point and owner | Current behavior and gap | +This table records the implementation state when #103505 was opened. The rollout sections below track the durable registry, typed actions, deep-link page, and native-client increments built on top of that baseline. + +| Surface | Baseline entry point and owner | Baseline behavior and gap | | ----------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | | Agent exec | `src/agents/bash-tools.exec-approval-request.ts`, `src/agents/bash-tools.exec-host-shared.ts` | Two-phase `exec.approval.*` registration prevents an early `/approve` race, but timeout can still become allow through `askFallback`. | | Plugin tool gate | `src/agents/agent-tools.before-tool-call.ts` | Requests `plugin.approval.*`; `timeoutBehavior: "allow"` can approve a timed-out gate. Embedded mode has separate process-local authority in `src/infra/embedded-plugin-approval-broker.ts`. | | Plugin node gate | `src/gateway/node-invoke-plugin-policy.ts` | Creates and broadcasts directly through the plugin manager, duplicating part of the server-method lifecycle. | | Gateway authority | `src/gateway/server-aux-handlers.ts`, `src/gateway/exec-approval-manager.ts`, `src/gateway/server-methods/approval-shared.ts` | Separate exec and plugin managers use process-local maps. Terminal entries survive for 15 seconds. First-answer-wins holds only inside one process. | | Gateway protocol | `packages/gateway-protocol/src/schema/exec-approvals.ts`, `packages/gateway-protocol/src/schema/plugin-approvals.ts`, `src/gateway/methods/core-descriptors.ts` | Exec has pending-only `get`; plugin has no `get`; no kind-agnostic terminal lookup exists for a deep link. | -| Delivery | `src/infra/exec-approval-channel-runtime.ts`, `src/infra/approval-native-runtime.ts`, `src/infra/approval-handler-runtime.ts` | Supports origin routing, approver DMs, pending replay, native handlers, and in-process terminal cleanup. PR 5 adds durable terminal reconciliation. | +| Delivery | `src/infra/exec-approval-channel-runtime.ts`, `src/infra/approval-native-runtime.ts`, `src/infra/approval-handler-runtime.ts` | Supports origin routing, approver DMs, pending replay, native handlers, and in-process terminal cleanup. A separate follow-up adds durable terminal reconciliation. | | Portable actions | `src/interactive/payload.ts`, `src/plugin-sdk/interactive-runtime.ts`, `src/plugin-sdk/approval-reply-runtime.ts` | Approval buttons are command actions containing `/approve ...`; URL and Web App targets are untyped button fields. | | Telegram | `extensions/telegram/src/approval-handler.runtime.ts`, `extensions/telegram/src/button-types.ts` | The renderer parses command text to recognize approval semantics before producing private callback data. | | Control UI | `ui/src/app/exec-approval.ts`, `ui/src/app/overlays.ts`, `ui/src/components/exec-approval.ts` | Approval UI is a global modal. `ui/src/app-route-paths.ts` and `ui/src/app-routes.ts` use exact routes and rewrite unknown paths to Chat. | @@ -200,10 +202,12 @@ Add an approval-scoped `session.approval` projection event. Publish the canonica - `sessionKey`: stream receiving the projection. - `sourceSessionKey`: child/source that raised the gate. -- `phase`: `requested \| terminal`. +- `phase`: `pending \| terminal`, discriminated against the approval status. - one safe `OperatorApproval` projection. -Register the event under `operator.approvals` in `src/gateway/server-broadcast.ts`. Session subscription alone never grants approval visibility. +Clients opt in with `sessions.messages.subscribe { key, agentId?, includeApprovals: true }`. The successful response adds an `approvalReplay` containing up to 1,000 current pending approvals for that exact stream key that the subscribing client is also record-authorized to review. `truncated: false` makes the filtered replay authoritative and reconnecting clients replace their local pending set with it; `truncated: true` is an overload signal and clients must keep unseen local entries until canonical lookup or later lifecycle events settle them. A later durable timeout discovered during replay emits terminal tombstones only to subscribed, record-authorized audiences before the new snapshot is returned. `operator.admin` may opt in directly; narrower clients require both a paired device identity and `operator.approvals`. Session subscription alone never grants approval visibility. + +Register the event under `operator.approvals` in `src/gateway/server-broadcast.ts`. The projection is observational: it never appends transcript rows, emits `sessions.changed`, or wakes an agent. Extend `MessagePresentationAction` in `src/interactive/payload.ts`: @@ -290,13 +294,13 @@ Use a deterministic breadth-first walk: The registry source is `src/agents/subagent-registry-read.ts`; ownership fields are defined in `src/agents/subagent-registry.types.ts`. Session fallback fields are defined in `src/config/sessions/types.ts`. -Requested and terminal projections use the same persisted audience even if focus/controller ownership changes while the approval is pending. This guarantees that every surface that displayed the request receives terminal cleanup. Resolution always targets the source approval ID; audience sessions never receive cloned approval state. +Requested and terminal projections use the same persisted audience even if focus/controller ownership changes while the approval is pending. This guarantees terminal cleanup for every audience session stream that received the request projection. Resolution always targets the source approval ID; audience sessions never receive cloned approval state. Forwarded channel-message cleanup remains the separate delivery-locator follow-up below. Do not write transcript messages, inject system prompts, start owner turns, or emit `sessions.changed` solely for an approval. ## Delivered-surface convergence -Native approval handlers already retain their delivered message entries long enough to replace or retire active controls. Generic forwarded approval messages currently discard the `MessageReceipt`, so a decision on another surface can leave their old controls looking pending. PR 5 closes that gap with an `operator_approval_deliveries` child table in the shared state database. +Native approval handlers already retain their delivered message entries long enough to replace or retire active controls. Generic forwarded approval messages currently discard the `MessageReceipt`, so a decision on another surface can leave their old controls looking pending. A separate follow-up closes that gap with an `operator_approval_deliveries` child table in the shared state database. Each row stores the approval ID, a unique delivery ID, channel/account/exact route, a bounded JSON-validated channel-private message locator, delivery timestamps, and terminalization state. It never stores callback data, decision tokens, or raw approval requests. The channel owns locator encoding and message mutation; core owns canonical status, target selection, retry policy, and fallback terminal text. @@ -331,13 +335,17 @@ Final strict behavior: - malformed trusted verdict -> `denied`, deny; - only an allowed explicit allow decision -> `allowed`. -Current shipped behavior conflicts with this contract: +Current shipped exec behavior still conflicts with this contract: - `src/agents/bash-tools.exec-host-shared.ts` may apply `askFallback`. -- `src/agents/agent-tools.before-tool-call.ts` may honor `timeoutBehavior: "allow"`. -- `docs/tools/exec-approvals.md`, `docs/cli/approvals.md`, and `docs/plugins/plugin-permission-requests.md` document those surfaces. +- `docs/tools/exec-approvals.md` and `docs/cli/approvals.md` document that surface. -Do not silently change them in the storage PR. The strict-semantics PR must update code, types, docs, tests, and changelog together, with explicit owner/security review. `askFallback` may continue to describe pre-gate policy selection during migration, but it must not turn a created pending record's timeout into approval. +Plugin approvals now fail closed on timeout and malformed verdicts; the legacy +`timeoutBehavior` field remains accepted but ignored. The exec strict-semantics +follow-up must update code, types, docs, tests, and changelog together, with +explicit owner/security review. `askFallback` may continue to describe +pre-gate policy selection during migration, but it must not turn a created +pending record's timeout into approval. ## Compatibility plan @@ -369,7 +377,7 @@ Do not silently change them in the storage PR. The strict-semantics PR must upda - Transport-private callback encoding with explicit owner kind. - Durable fixed-size callback references for canonical IDs beyond transport limits. - Bundled channel migration away from command-text and approval-ID inference. -- Canonical first-answer truth on the clicked surface and best-effort active-native terminal updates; durable reconciliation stays in PR 5. +- Canonical first-answer truth on the clicked surface and best-effort active-native terminal updates; durable channel-message terminalization remains a follow-up. - SDK and bundled-channel tests. ### PR 3: Control UI deep link @@ -382,17 +390,32 @@ Do not silently change them in the storage PR. The strict-semantics PR must upda ### PR 4: native clients -- iOS, watchOS, and Android review surfaces use kind-aware `approval.get/resolve`. +- iOS and Android review surfaces use kind-aware `approval.get/resolve`; watchOS relays reviewer-safe prompts and decisions through the paired iPhone. +- Watch offers the exec decisions supported by its compact relay contract: allow once and deny. - Canonical first-answer terminal truth replaces local attempted-decision state. +- Lost or ambiguous resolve acknowledgements freeze controls until canonical readback. +- Previous shipped Gateway v4 instances retain exec review through a narrow legacy-method fallback; retained cross-surface terminal state requires the unified methods. +- Reviewer warnings and owner context remain visible across iPhone, Watch, and Android. - Native unit, build, and platform proof. -### PR 5: propagation and fail-closed behavior +### PR 5: ancestor lifecycle propagation + +- `session.approval` pending/terminal delivery from the audience snapshot persisted in PR 1. +- Exact-session subscription, reconnect replay, and terminal tombstones without transcript mutation or agent wake. +- Lifecycle callbacks run after durable insert/CAS and never become approval authority. +- Nested-subagent and reconnect proof. + +### PR 6: fail-closed behavior -- `session.approval` request/terminal delivery from the audience snapshot persisted in PR 1. -- Durable forwarded-delivery locators and canonical terminal cleanup across every delivered surface, including restart replay. - Migrate `node-invoke-plugin-policy.ts` and the embedded plugin broker away from duplicate authority. -- Strict timeout/malformed/no-route semantics and compatibility docs. -- Multi-surface and nested-subagent end-to-end proof. +- Strict timeout, malformed, no-route, binding, and allow-once consumption semantics. +- Deprecate shipped permissive timeout settings without honoring them after an ask is pending. +- Multi-surface contention and failure-injection proof. + +### Follow-up: durable remote-message cleanup + +- Persist forwarded-delivery locators and terminalize every delivered channel message after restart. +- Keep this transport lifecycle separate from canonical approval authority and typed presentation actions. ## Tests @@ -415,7 +438,11 @@ Required focused coverage: - Owner projections cause no transcript mutation or agent wake. - Control UI route works at `/` and a configured base path; refresh shows pending or terminal truth. - Simultaneous Control UI and Telegram answers show one winner and "resolved elsewhere" on the loser. -- User-path proof through Testbox/Crabbox, including a mobile-width approval page and Telegram action cleanup. +- Native approval identifiers and Gateway owner identifiers preserve exact UTF-8 bytes across routing and reconciliation. +- Native RPC-family negotiation pins one canonical or legacy family per admitted Gateway route and never silently downgrades after use. +- Lost native resolve acknowledgements freeze actions until canonical readback; failed readback cannot fabricate a winner or acknowledge a Watch refresh. +- Watch snapshot request correlation is accepted only for the exact paired Gateway owner and a completed canonical iPhone readback. +- User-path proof through Testbox/Crabbox, including a mobile-width approval page, Telegram action cleanup, and one pending/resolve/late-loser round trip across Android, iPhone, and Watch. ## Observability @@ -432,10 +459,10 @@ Track: - startup-orphan cancellations; - audience size. -A committed transition is success even if later event delivery fails. Delivery failure is logged separately; PR 5 repairs remote state through durable delivery replay and canonical lookup. +A committed transition is success even if later event delivery fails. Lifecycle subscribers recover through PR 5 replay and canonical lookup. Durable channel-message terminalization remains the separate follow-up above. ## Open decisions 1. **Externally reachable Control UI origin.** Every snapshot carries the stable relative `urlPath`. An absolute URL may be advertised only from a cached Tailscale Serve/Funnel location after Gateway exposure succeeds; `allowedOrigins`, request Host headers, `gateway.remote.url`, and display-only loopback/LAN candidates are not canonical origins. Telegram can use its authenticated Mini App wrapper to retain the approval path through bootstrap. Arbitrary reverse proxies remain relative-only until a separately reviewed explicit public-URL contract exists. Never let a channel guess the origin. -2. **Strict timeout compatibility cutover.** The target is fail-closed, but `askFallback` and plugin `timeoutBehavior: "allow"` are shipped contracts. Recommended: make the behavior change in PR 5 with explicit owner/security approval, changelog, docs, and a migration/deprecation decision rather than hiding it in PR 1. +2. **Exec strict timeout compatibility cutover.** Plugin approval timeouts now fail closed and `timeoutBehavior` is deprecated. The remaining shipped `askFallback` contract needs explicit owner/security review, changelog, docs, and a migration/deprecation decision before it stops authorizing execution after a pending ask times out. 3. **Gatewayless embedded mode.** Recommended: keep it local-only initially, then make it a client of the canonical service when a Gateway exists. Do not advertise a deep link that no server can resolve. diff --git a/extensions/codex/src/app-server/approval-bridge.test.ts b/extensions/codex/src/app-server/approval-bridge.test.ts index c817a08e946b..e7d5f9d6b4ed 100644 --- a/extensions/codex/src/app-server/approval-bridge.test.ts +++ b/extensions/codex/src/app-server/approval-bridge.test.ts @@ -2248,6 +2248,36 @@ describe("Codex app-server approval bridge", () => { }); }); + it("ignores waitDecision replies bound to a different approval id", async () => { + const params = createParams(); + const onNativeToolFailureDisposition = vi.fn(); + mockCallGatewayTool + .mockResolvedValueOnce({ id: "plugin:approval-mismatch", status: "accepted" }) + .mockResolvedValueOnce({ id: "plugin:approval-other", decision: "allow-once" }); + + const result = await handleCodexAppServerApprovalRequest({ + method: "item/commandExecution/requestApproval", + requestParams: { + threadId: "thread-1", + turnId: "turn-1", + itemId: "cmd-mismatch", + command: "pnpm test", + }, + paramsForRun: params, + threadId: "thread-1", + turnId: "turn-1", + onNativeToolFailureDisposition, + }); + + // A misrouted allow for another approval must not release this gate. + expect(result).toEqual({ decision: "decline" }); + expect(onNativeToolFailureDisposition).toHaveBeenCalledWith("cmd-mismatch", "failed"); + findApprovalEvent(params, { + status: "unavailable", + approvalId: "plugin:approval-mismatch", + }); + }); + it("sanitizes reason previews before forwarding approval text and events", async () => { const params = createParams(); mockCallGatewayTool.mockResolvedValueOnce({ diff --git a/extensions/codex/src/app-server/plugin-approval-roundtrip.ts b/extensions/codex/src/app-server/plugin-approval-roundtrip.ts index cc5bea9bdc25..827f330fa9c2 100644 --- a/extensions/codex/src/app-server/plugin-approval-roundtrip.ts +++ b/extensions/codex/src/app-server/plugin-approval-roundtrip.ts @@ -93,8 +93,14 @@ export async function waitForPluginApprovalDecision(params: { { timeoutMs: resolveCodexGatewayTimeoutWithGraceMs(timeoutMs) }, { id: params.approvalId }, ); + // Bind the verdict to the approval that parked this prompt. A stale or + // misrouted reply maps to "unavailable" instead of releasing another gate. + const bindDecision = ( + result: ApprovalWaitResult | undefined, + ): ExecApprovalDecision | null | undefined => + result?.id === params.approvalId ? result.decision : undefined; if (!params.signal) { - return (await waitPromise)?.decision; + return bindDecision(await waitPromise); } let onAbort: (() => void) | undefined; const abortPromise = new Promise((_, reject) => { @@ -106,7 +112,7 @@ export async function waitForPluginApprovalDecision(params: { params.signal!.addEventListener("abort", onAbort, { once: true }); }); try { - return (await Promise.race([waitPromise, abortPromise]))?.decision; + return bindDecision(await Promise.race([waitPromise, abortPromise])); } finally { if (onAbort) { params.signal.removeEventListener("abort", onAbort); diff --git a/extensions/file-transfer/src/shared/node-invoke-policy.test.ts b/extensions/file-transfer/src/shared/node-invoke-policy.test.ts index 9b62c1095732..1c0aa7b0e323 100644 --- a/extensions/file-transfer/src/shared/node-invoke-policy.test.ts +++ b/extensions/file-transfer/src/shared/node-invoke-policy.test.ts @@ -270,11 +270,95 @@ describe("file-transfer node invoke policy", () => { expect(invokeNode).not.toHaveBeenCalled(); }); - it("uses plugin approvals for ask-on-miss before invoking the node", async () => { + it.each(["allow-once", "allow-always"] as const)( + "uses exact %s plugin approval once across preflight and final invoke", + async (decision) => { + const policy = createFileTransferNodeInvokePolicy(); + const approvals = { + request: vi.fn(async () => ({ id: "approval-1", decision })), + }; + const { ctx, invokeNode } = createCtx({ + params: { path: "/tmp/new.txt" }, + pluginConfig: { + nodes: { + "node-1": { + ask: "on-miss", + allowReadPaths: ["/allowed/**"], + maxBytes: 256, + }, + }, + }, + approvals, + }); + + const result = await policy.handle(ctx); + + expect(result.ok).toBe(true); + expect(approvals.request).toHaveBeenCalledTimes(1); + expect(invokeNode).toHaveBeenCalledTimes(2); + const approvalCalls = approvals.request.mock.calls as unknown[][]; + const approvalRequest = requireRecord(approvalCalls[0]?.[0], "approval request"); + expectRecordFields(approvalRequest, { + title: "Read file: /tmp/new.txt", + severity: "info", + toolName: "file.fetch", + }); + expect(invokeNode).toHaveBeenNthCalledWith(1, { + params: { + path: "/tmp/new.txt", + followSymlinks: false, + maxBytes: 256, + preflightOnly: true, + }, + }); + expect(invokeNode).toHaveBeenNthCalledWith(2, { + params: { + path: "/tmp/new.txt", + followSymlinks: false, + maxBytes: 256, + }, + }); + }, + ); + + it.each([ + { + label: "explicit deny", + decision: "deny", + code: "APPROVAL_DENIED", + message: "file.fetch APPROVAL_DENIED: operator denied the prompt", + }, + { + label: "null decision", + decision: null, + code: "APPROVAL_UNAVAILABLE", + message: + "file.fetch APPROVAL_UNAVAILABLE: no operator client connected to approve the request", + }, + { + label: "undefined decision", + decision: undefined, + code: "APPROVAL_UNAVAILABLE", + message: + "file.fetch APPROVAL_UNAVAILABLE: no operator client connected to approve the request", + }, + { + label: "arbitrary truthy string", + decision: "accept", + code: "APPROVAL_DENIED", + message: "file.fetch APPROVAL_DENIED: invalid approval decision", + }, + { + label: "arbitrary truthy object", + decision: { action: "accept" }, + code: "APPROVAL_DENIED", + message: "file.fetch APPROVAL_DENIED: invalid approval decision", + }, + ])("fails closed for $label", async ({ decision, code, message }) => { const policy = createFileTransferNodeInvokePolicy(); const approvals = { - request: vi.fn(async () => ({ id: "approval-1", decision: "allow-once" as const })), - }; + request: vi.fn(async () => ({ id: "approval-1", decision })), + } as unknown as NonNullable; const { ctx, invokeNode } = createCtx({ params: { path: "/tmp/new.txt" }, pluginConfig: { @@ -282,7 +366,6 @@ describe("file-transfer node invoke policy", () => { "node-1": { ask: "on-miss", allowReadPaths: ["/allowed/**"], - maxBytes: 256, }, }, }, @@ -291,29 +374,9 @@ describe("file-transfer node invoke policy", () => { const result = await policy.handle(ctx); - expect(result.ok).toBe(true); - const approvalCalls = approvals.request.mock.calls as unknown[][]; - const approvalRequest = requireRecord(approvalCalls[0]?.[0], "approval request"); - expectRecordFields(approvalRequest, { - title: "Read file: /tmp/new.txt", - severity: "info", - toolName: "file.fetch", - }); - expect(invokeNode).toHaveBeenNthCalledWith(1, { - params: { - path: "/tmp/new.txt", - followSymlinks: false, - maxBytes: 256, - preflightOnly: true, - }, - }); - expect(invokeNode).toHaveBeenNthCalledWith(2, { - params: { - path: "/tmp/new.txt", - followSymlinks: false, - maxBytes: 256, - }, - }); + expectResultFields(result, { ok: false, code, message }); + expect(approvals.request).toHaveBeenCalledTimes(1); + expect(invokeNode).not.toHaveBeenCalled(); }); it("marks node transport failures as unavailable", async () => { diff --git a/extensions/file-transfer/src/shared/node-invoke-policy.ts b/extensions/file-transfer/src/shared/node-invoke-policy.ts index 0089206aa8e6..6a4e31b76581 100644 --- a/extensions/file-transfer/src/shared/node-invoke-policy.ts +++ b/extensions/file-transfer/src/shared/node-invoke-policy.ts @@ -154,28 +154,37 @@ async function requestApproval(input: { severity: input.kind === "write" ? "warning" : "info", toolName: input.op, }); + const approvalDecision: unknown = approval.decision; - if (approval.decision === "deny" || approval.decision === null || !approval.decision) { + if (approvalDecision !== "allow-once" && approvalDecision !== "allow-always") { + const unavailable = approvalDecision === null || approvalDecision === undefined; + const deniedByOperator = approvalDecision === "deny"; + const reason = deniedByOperator + ? "operator denied" + : unavailable + ? "no operator available" + : "invalid approval decision"; await appendFileTransferAudit({ op: input.op, nodeId: input.ctx.nodeId, nodeDisplayName, requestedPath: input.path, decision: "denied:approval", - reason: approval.decision === "deny" ? "operator denied" : "no operator available", + reason, durationMs: Date.now() - input.startedAt, }); return { ok: false, - code: approval.decision === "deny" ? "APPROVAL_DENIED" : "APPROVAL_UNAVAILABLE", - message: - approval.decision === "deny" + code: unavailable ? "APPROVAL_UNAVAILABLE" : "APPROVAL_DENIED", + message: unavailable + ? `${input.op} APPROVAL_UNAVAILABLE: no operator client connected to approve the request` + : deniedByOperator ? `${input.op} APPROVAL_DENIED: operator denied the prompt` - : `${input.op} APPROVAL_UNAVAILABLE: no operator client connected to approve the request`, + : `${input.op} APPROVAL_DENIED: invalid approval decision`, }; } - if (approval.decision === "allow-always") { + if (approvalDecision === "allow-always") { try { await persistAllowAlways({ nodeId: input.ctx.nodeId, @@ -228,7 +237,7 @@ async function requestApproval(input: { nodeId: input.ctx.nodeId, nodeDisplayName, requestedPath: input.path, - decision: approval.decision === "allow-always" ? "allowed:always" : "allowed:once", + decision: approvalDecision === "allow-always" ? "allowed:always" : "allowed:once", durationMs: Date.now() - input.startedAt, }); return { diff --git a/packages/gateway-protocol/src/index.ts b/packages/gateway-protocol/src/index.ts index 896bcf920795..753cc9b0b87a 100644 --- a/packages/gateway-protocol/src/index.ts +++ b/packages/gateway-protocol/src/index.ts @@ -302,6 +302,10 @@ import { ApprovalResolveResultSchema, type ApprovalSnapshot, ApprovalSnapshotSchema, + type SessionApprovalEvent, + SessionApprovalEventSchema, + type SessionApprovalReplay, + SessionApprovalReplaySchema, type ApprovalTerminalReason, ApprovalTerminalReasonSchema, type CancelledApprovalSnapshot, @@ -1965,6 +1969,8 @@ export { ApprovalGetResultSchema, ApprovalResolveParamsSchema, ApprovalResolveResultSchema, + SessionApprovalEventSchema, + SessionApprovalReplaySchema, ExecApprovalsGetParamsSchema, ExecApprovalsSetParamsSchema, ExecApprovalGetParamsSchema, @@ -2326,6 +2332,8 @@ export type { ApprovalGetResult, ApprovalResolveParams, ApprovalResolveResult, + SessionApprovalEvent, + SessionApprovalReplay, ExecApprovalsGetParams, ExecApprovalsNodeSnapshot, ExecApprovalsSetParams, diff --git a/packages/gateway-protocol/src/native-protocol-levels.guard.test.ts b/packages/gateway-protocol/src/native-protocol-levels.guard.test.ts index 5ff840ed940d..f34a8714eacc 100644 --- a/packages/gateway-protocol/src/native-protocol-levels.guard.test.ts +++ b/packages/gateway-protocol/src/native-protocol-levels.guard.test.ts @@ -79,7 +79,7 @@ function stringLiteralUnionValues(schema: unknown): string[] | undefined { } const candidate = schema as { anyOf?: unknown; oneOf?: unknown }; const branches = candidate.oneOf ?? candidate.anyOf; - if (!Array.isArray(branches) || branches.length < 2) { + if (!Array.isArray(branches) || branches.length === 0) { return undefined; } @@ -245,4 +245,29 @@ describe("native Gateway protocol levels", () => { } } }); + + it("emits the session approval event as a discriminated Swift union", async () => { + const swiftGeneratedPath = + "apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift"; + const swiftGenerated = await readRepoFile(swiftGeneratedPath); + + assertPattern( + swiftGenerated, + swiftGeneratedPath, + /public enum SessionApprovalEvent: Codable, Sendable \{/, + "missing the generated SessionApprovalEvent union.", + ); + assertPattern( + swiftGenerated, + swiftGeneratedPath, + /case pending\(PendingSessionApprovalEvent\)/, + "SessionApprovalEvent must decode pending transitions.", + ); + assertPattern( + swiftGenerated, + swiftGeneratedPath, + /case terminal\(TerminalSessionApprovalEvent\)/, + "SessionApprovalEvent must decode terminal transitions.", + ); + }); }); diff --git a/packages/gateway-protocol/src/schema/approvals.ts b/packages/gateway-protocol/src/schema/approvals.ts index 681db85b5d35..0e51641fb3d7 100644 --- a/packages/gateway-protocol/src/schema/approvals.ts +++ b/packages/gateway-protocol/src/schema/approvals.ts @@ -39,6 +39,26 @@ export const ApprovalTerminalReasonSchema = Type.Union([ Type.Literal("storage-corrupt"), ]); +/** Terminal reason accepted for an allowed approval. */ +export const ApprovalAllowedReasonSchema = Type.Union([Type.Literal("user")]); + +/** Terminal reasons accepted for a denied approval. */ +export const ApprovalDeniedReasonSchema = Type.Union([ + Type.Literal("user"), + Type.Literal("malformed-verdict"), + Type.Literal("no-route"), + Type.Literal("storage-corrupt"), +]); + +/** Terminal reason accepted for an expired approval. */ +export const ApprovalExpiredReasonSchema = Type.Union([Type.Literal("timeout")]); + +/** Terminal reasons accepted for a cancelled approval. */ +export const ApprovalCancelledReasonSchema = Type.Union([ + Type.Literal("run-aborted"), + Type.Literal("gateway-restart"), +]); + /** Reviewer-facing severity for plugin-owned approval requests. */ export const PluginApprovalSeveritySchema = Type.Union([ Type.Literal("info"), @@ -105,7 +125,6 @@ const ApprovalRecordCommonFields = { const ApprovalResolutionFields = { resolvedAtMs: Type.Integer({ minimum: 0 }), - reason: ApprovalTerminalReasonSchema, }; /** Approval that has not yet accepted a reviewer decision. */ @@ -121,6 +140,7 @@ export const AllowedApprovalSnapshotSchema = Type.Object( ...ApprovalResolutionFields, status: Type.Literal("allowed"), decision: ApprovalAllowDecisionSchema, + reason: ApprovalAllowedReasonSchema, }, { additionalProperties: false }, ); @@ -132,6 +152,7 @@ export const DeniedApprovalSnapshotSchema = Type.Object( ...ApprovalResolutionFields, status: Type.Literal("denied"), decision: Type.Literal("deny"), + reason: ApprovalDeniedReasonSchema, }, { additionalProperties: false }, ); @@ -142,6 +163,7 @@ export const ExpiredApprovalSnapshotSchema = Type.Object( ...ApprovalRecordCommonFields, ...ApprovalResolutionFields, status: Type.Literal("expired"), + reason: ApprovalExpiredReasonSchema, }, { additionalProperties: false }, ); @@ -152,6 +174,7 @@ export const CancelledApprovalSnapshotSchema = Type.Object( ...ApprovalRecordCommonFields, ...ApprovalResolutionFields, status: Type.Literal("cancelled"), + reason: ApprovalCancelledReasonSchema, }, { additionalProperties: false }, ); @@ -204,6 +227,49 @@ export const ApprovalResolveResultSchema = Type.Object( { additionalProperties: false }, ); +const SessionApprovalEventCommonFields = { + sessionKey: NonEmptyString, + sourceSessionKey: Type.Optional(NonEmptyString), + updatedAtMs: Type.Integer({ minimum: 0 }), +}; + +/** Sanitized pending transition delivered only to an opted-in session audience. */ +export const PendingSessionApprovalEventSchema = Type.Object( + { + ...SessionApprovalEventCommonFields, + phase: Type.Literal("pending"), + approval: PendingApprovalSnapshotSchema, + }, + { additionalProperties: false }, +); + +/** Sanitized terminal transition delivered only to an opted-in session audience. */ +export const TerminalSessionApprovalEventSchema = Type.Object( + { + ...SessionApprovalEventCommonFields, + phase: Type.Literal("terminal"), + approval: TerminalApprovalSnapshotSchema, + }, + { additionalProperties: false }, +); + +/** Sanitized approval transition delivered only to an opted-in session audience. */ +export const SessionApprovalEventSchema = Type.Union([ + PendingSessionApprovalEventSchema, + TerminalSessionApprovalEventSchema, +]); + +/** Authoritative pending approval set returned when a session stream subscribes. */ +export const SessionApprovalReplaySchema = Type.Object( + { + sessionKey: NonEmptyString, + updatedAtMs: Type.Integer({ minimum: 0 }), + approvals: Type.Array(PendingApprovalSnapshotSchema), + truncated: Type.Boolean(), + }, + { additionalProperties: false }, +); + // Owner-local wire types derived directly from local schema consts so the // public plugin-sdk declaration graph never pulls in the ProtocolSchemas registry. export type ApprovalKind = Static; @@ -225,3 +291,5 @@ export type DeniedApprovalSnapshot = Static export type ExpiredApprovalSnapshot = Static; export type CancelledApprovalSnapshot = Static; export type TerminalApprovalSnapshot = Static; +export type SessionApprovalEvent = Static; +export type SessionApprovalReplay = Static; diff --git a/packages/gateway-protocol/src/schema/protocol-schemas.ts b/packages/gateway-protocol/src/schema/protocol-schemas.ts index 2589a39b24d5..897e7c292d27 100644 --- a/packages/gateway-protocol/src/schema/protocol-schemas.ts +++ b/packages/gateway-protocol/src/schema/protocol-schemas.ts @@ -94,13 +94,19 @@ import { import { AllowedApprovalSnapshotSchema, ApprovalAllowDecisionSchema, + ApprovalAllowedReasonSchema, + ApprovalCancelledReasonSchema, ApprovalDecisionSchema, + ApprovalDeniedReasonSchema, + ApprovalExpiredReasonSchema, ApprovalGetParamsSchema, ApprovalGetResultSchema, ApprovalKindSchema, ApprovalPresentationSchema, ApprovalResolveParamsSchema, ApprovalResolveResultSchema, + SessionApprovalEventSchema, + SessionApprovalReplaySchema, ApprovalSnapshotSchema, ApprovalTerminalReasonSchema, CancelledApprovalSnapshotSchema, @@ -108,9 +114,11 @@ import { ExecApprovalPresentationSchema, ExpiredApprovalSnapshotSchema, PendingApprovalSnapshotSchema, + PendingSessionApprovalEventSchema, PluginApprovalPresentationSchema, PluginApprovalSeveritySchema, TerminalApprovalSnapshotSchema, + TerminalSessionApprovalEventSchema, } from "./approvals.js"; import { ArtifactSummarySchema, @@ -877,6 +885,10 @@ export const ProtocolSchemas = { ApprovalKind: ApprovalKindSchema, ApprovalDecision: ApprovalDecisionSchema, ApprovalAllowDecision: ApprovalAllowDecisionSchema, + ApprovalAllowedReason: ApprovalAllowedReasonSchema, + ApprovalDeniedReason: ApprovalDeniedReasonSchema, + ApprovalExpiredReason: ApprovalExpiredReasonSchema, + ApprovalCancelledReason: ApprovalCancelledReasonSchema, PluginApprovalSeverity: PluginApprovalSeveritySchema, ExecApprovalPresentation: ExecApprovalPresentationSchema, PluginApprovalPresentation: PluginApprovalPresentationSchema, @@ -893,6 +905,10 @@ export const ProtocolSchemas = { ApprovalGetResult: ApprovalGetResultSchema, ApprovalResolveParams: ApprovalResolveParamsSchema, ApprovalResolveResult: ApprovalResolveResultSchema, + PendingSessionApprovalEvent: PendingSessionApprovalEventSchema, + TerminalSessionApprovalEvent: TerminalSessionApprovalEventSchema, + SessionApprovalEvent: SessionApprovalEventSchema, + SessionApprovalReplay: SessionApprovalReplaySchema, ExecApprovalsGetParams: ExecApprovalsGetParamsSchema, ExecApprovalsSetParams: ExecApprovalsSetParamsSchema, ExecApprovalsNodeGetParams: ExecApprovalsNodeGetParamsSchema, diff --git a/packages/gateway-protocol/src/schema/sessions.ts b/packages/gateway-protocol/src/schema/sessions.ts index 98bf2134e095..894ea72eadc2 100644 --- a/packages/gateway-protocol/src/schema/sessions.ts +++ b/packages/gateway-protocol/src/schema/sessions.ts @@ -446,6 +446,8 @@ export const SessionsMessagesSubscribeParamsSchema = Type.Object( { key: NonEmptyString, agentId: Type.Optional(NonEmptyString), + /** Opt in to sanitized durable approval events for this session and its descendants. */ + includeApprovals: Type.Optional(Type.Literal(true)), }, { additionalProperties: false }, ); diff --git a/packages/gateway-protocol/src/session-approval-validators.test.ts b/packages/gateway-protocol/src/session-approval-validators.test.ts new file mode 100644 index 000000000000..00f5a56718b6 --- /dev/null +++ b/packages/gateway-protocol/src/session-approval-validators.test.ts @@ -0,0 +1,147 @@ +import { Value } from "typebox/value"; +import { describe, expect, it } from "vitest"; +import { + SessionApprovalEventSchema, + SessionApprovalReplaySchema, + validateSessionsMessagesSubscribeParams, +} from "./index.js"; + +const approval = { + id: "approval:01JZ4K6M2X8YQW9N7R3T5V1C0B", + urlPath: "/approve/approval%3A01JZ4K6M2X8YQW9N7R3T5V1C0B", + presentation: { + kind: "exec", + commandText: "git status --short", + commandPreview: "git status", + warningText: null, + host: "gateway", + nodeId: null, + agentId: "main", + allowedDecisions: ["allow-once", "allow-always", "deny"], + }, + createdAtMs: 1_780_000_000_000, + expiresAtMs: 1_780_001_800_000, +} as const; + +const pending = { ...approval, status: "pending" } as const; +const terminal = { + ...approval, + status: "denied", + decision: "deny", + resolvedAtMs: approval.createdAtMs + 1_000, + reason: "user", +} as const; + +describe("session approval protocol validators", () => { + it("keeps approval subscription opt-in additive and literal", () => { + expect(validateSessionsMessagesSubscribeParams({ key: "agent:main:main" })).toBe(true); + expect( + validateSessionsMessagesSubscribeParams({ + key: "agent:main:main", + includeApprovals: true, + }), + ).toBe(true); + expect( + validateSessionsMessagesSubscribeParams({ + key: "agent:main:main", + includeApprovals: false, + }), + ).toBe(false); + }); + + it("requires event phase to match the approval snapshot state", () => { + const common = { + sessionKey: "agent:main:main", + sourceSessionKey: "agent:worker:subagent:child", + updatedAtMs: terminal.resolvedAtMs, + } as const; + + expect( + Value.Check(SessionApprovalEventSchema, { + ...common, + phase: "pending", + approval: pending, + }), + ).toBe(true); + expect( + Value.Check(SessionApprovalEventSchema, { + ...common, + phase: "terminal", + approval: terminal, + }), + ).toBe(true); + expect( + Value.Check(SessionApprovalEventSchema, { + ...common, + phase: "pending", + approval: terminal, + }), + ).toBe(false); + expect( + Value.Check(SessionApprovalEventSchema, { + ...common, + phase: "terminal", + approval: pending, + }), + ).toBe(false); + }); + + it("rejects terminal reasons that contradict the terminal status", () => { + const common = { + sessionKey: "agent:main:main", + phase: "terminal", + updatedAtMs: terminal.resolvedAtMs, + } as const; + const terminalCommon = { + ...approval, + resolvedAtMs: terminal.resolvedAtMs, + } as const; + + expect( + Value.Check(SessionApprovalEventSchema, { + ...common, + approval: { ...terminal, reason: "timeout" }, + }), + ).toBe(false); + expect( + Value.Check(SessionApprovalEventSchema, { + ...common, + approval: { + ...terminalCommon, + status: "allowed", + decision: "allow-once", + reason: "timeout", + }, + }), + ).toBe(false); + expect( + Value.Check(SessionApprovalEventSchema, { + ...common, + approval: { ...terminalCommon, status: "expired", reason: "user" }, + }), + ).toBe(false); + expect( + Value.Check(SessionApprovalEventSchema, { + ...common, + approval: { ...terminalCommon, status: "cancelled", reason: "timeout" }, + }), + ).toBe(false); + }); + + it("replays only authoritative pending approval snapshots", () => { + const replay = { + sessionKey: "agent:main:main", + updatedAtMs: approval.createdAtMs, + approvals: [pending], + truncated: false, + } as const; + + expect(Value.Check(SessionApprovalReplaySchema, replay)).toBe(true); + expect( + Value.Check(SessionApprovalReplaySchema, { + ...replay, + approvals: [terminal], + }), + ).toBe(false); + }); +}); diff --git a/scripts/protocol-event-coverage.allowlist.json b/scripts/protocol-event-coverage.allowlist.json index e57f60502fb7..e951ae4c1f9b 100644 --- a/scripts/protocol-event-coverage.allowlist.json +++ b/scripts/protocol-event-coverage.allowlist.json @@ -19,7 +19,8 @@ "plugin.approval.resolved": "Plugin approval prompts are not implemented on iOS.", "terminal.data": "Embedded terminal is a web/desktop surface; iOS has no terminal client.", "terminal.exit": "Embedded terminal is a web/desktop surface; iOS has no terminal client.", - "update.available": "Gateway self-update notices do not apply to iOS; app updates ship via the App Store." + "update.available": "Gateway self-update notices do not apply to iOS; app updates ship via the App Store.", + "session.approval": "Native approval review uses exec.approval push/nudge delivery; the session-scoped approval stream is a Control UI chat surface." }, "android": { "session.operation": "Chat UI derives run state from chat/agent events; no session.operation consumer yet.", @@ -41,6 +42,7 @@ "plugin.approval.requested": "Plugin approval prompts are not implemented on Android.", "plugin.approval.resolved": "Plugin approval prompts are not implemented on Android.", "terminal.data": "Embedded terminal is a web/desktop surface; Android has no terminal client.", - "terminal.exit": "Embedded terminal is a web/desktop surface; Android has no terminal client." + "terminal.exit": "Embedded terminal is a web/desktop surface; Android has no terminal client.", + "session.approval": "Native approval review uses exec.approval push/nudge delivery; the session-scoped approval stream is a Control UI chat surface." } } diff --git a/src/agents/agent-tools.before-tool-call.e2e.test.ts b/src/agents/agent-tools.before-tool-call.e2e.test.ts index 6321e59f69cb..a88642ba0a42 100644 --- a/src/agents/agent-tools.before-tool-call.e2e.test.ts +++ b/src/agents/agent-tools.before-tool-call.e2e.test.ts @@ -19,6 +19,7 @@ import { } from "../infra/diagnostic-events.js"; import { MAX_PLUGIN_APPROVAL_TIMEOUT_MS } from "../infra/plugin-approvals.js"; import { resetDiagnosticSessionStateForTest } from "../logging/diagnostic-session-state.js"; +import { PluginApprovalResolutions } from "../plugins/hook-before-tool-call-result.js"; import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; import { createEmptyPluginRegistry } from "../plugins/registry-empty.js"; import { setActivePluginRegistry } from "../plugins/runtime.js"; @@ -2037,18 +2038,25 @@ describe("before_tool_call requireApproval handling", () => { ); }); - it("allows on timeout when timeoutBehavior is allow and preserves hook params", async () => { + it.each([ + ["a timeout", null], + ["an explicit timeout decision", PluginApprovalResolutions.TIMEOUT], + ["an unknown decision", "approved"], + ["a malformed truthy decision", true as unknown as string], + ])("blocks on %s even when deprecated timeoutBehavior is allow", async (_label, decision) => { + const onResolution = vi.fn(); hookRunner.runBeforeToolCall.mockResolvedValue({ params: { command: "safe-command" }, requireApproval: { title: "Lenient timeout", - description: "Should allow on timeout", + description: "Must fail closed", timeoutBehavior: "allow", + onResolution, }, }); mockCallGateway.mockResolvedValueOnce({ id: "server-id-4", status: "accepted" }); - mockCallGateway.mockResolvedValueOnce({ id: "server-id-4", decision: null }); + mockCallGateway.mockResolvedValueOnce({ id: "server-id-4", decision }); const result = await runBeforeToolCallHook({ toolName: "bash", @@ -2056,10 +2064,78 @@ describe("before_tool_call requireApproval handling", () => { ctx: { agentId: "main", sessionKey: "main" }, }); - expect(result.blocked).toBe(false); - if (!result.blocked) { - expect(result.params).toEqual({ command: "safe-command" }); - } + expect(result).toMatchObject({ + blocked: true, + kind: "failure", + disposition: "timed_out", + deniedReason: "plugin-approval", + reason: "Approval timed out", + params: { command: "rm -rf /" }, + }); + expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.TIMEOUT); + }); + + it("blocks exact allow decisions excluded by the request", async () => { + const onResolution = vi.fn(); + hookRunner.runBeforeToolCall.mockResolvedValue({ + params: { command: "safe-command" }, + requireApproval: { + title: "Restricted approval", + description: "Allow once only", + allowedDecisions: ["allow-once", "deny"], + onResolution, + }, + }); + mockCallGateway.mockResolvedValueOnce({ id: "server-id-restricted", status: "accepted" }); + mockCallGateway.mockResolvedValueOnce({ + id: "server-id-restricted", + decision: "allow-always", + }); + + const result = await runBeforeToolCallHook({ + toolName: "bash", + params: { command: "unsafe-command" }, + ctx: { agentId: "main", sessionKey: "main" }, + }); + + expect(result).toMatchObject({ + blocked: true, + disposition: "timed_out", + reason: "Approval timed out", + params: { command: "unsafe-command" }, + }); + expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.TIMEOUT); + }); + + it("blocks a wait decision bound to another approval id", async () => { + const onResolution = vi.fn(); + hookRunner.runBeforeToolCall.mockResolvedValue({ + params: { command: "safe-command" }, + requireApproval: { + title: "Bound approval", + description: "Must match the request id", + onResolution, + }, + }); + mockCallGateway.mockResolvedValueOnce({ id: "server-id-bound", status: "accepted" }); + mockCallGateway.mockResolvedValueOnce({ + id: "server-id-other", + decision: "allow-once", + }); + + const result = await runBeforeToolCallHook({ + toolName: "bash", + params: { command: "unsafe-command" }, + ctx: { agentId: "main", sessionKey: "main" }, + }); + + expect(result).toMatchObject({ + blocked: true, + disposition: "timed_out", + reason: "Approval timed out", + params: { command: "unsafe-command" }, + }); + expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.TIMEOUT); }); it("falls back to block on gateway error", async () => { diff --git a/src/agents/agent-tools.before-tool-call.embedded-mode.test.ts b/src/agents/agent-tools.before-tool-call.embedded-mode.test.ts index 58450c9fd194..6d5c55056c88 100644 --- a/src/agents/agent-tools.before-tool-call.embedded-mode.test.ts +++ b/src/agents/agent-tools.before-tool-call.embedded-mode.test.ts @@ -39,6 +39,19 @@ vi.mock("./tools/gateway.js", () => ({ callGatewayTool: vi.fn(), })); +const agentToolsWarnSpy = vi.hoisted(() => vi.fn()); +vi.mock("../logging/subsystem.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + createSubsystemLogger: (subsystem: string) => { + const logger = actual.createSubsystemLogger(subsystem); + // Capture agents/tools warnings so the deprecation signal is assertable. + return subsystem === "agents/tools" ? { ...logger, warn: agentToolsWarnSpy } : logger; + }, + }; +}); + const mockGetGlobalHookRunner = vi.mocked(getGlobalHookRunner); const mockCallGatewayTool = vi.mocked(callGatewayTool); @@ -220,6 +233,120 @@ describe("runBeforeToolCallHook — embedded mode approvals", () => { expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.CANCELLED); }); + it("blocks embedded approvals on timeout even when deprecated timeoutBehavior is allow", async () => { + setEmbeddedMode(true); + const broker = new EmbeddedPluginApprovalBroker(); + setEmbeddedPluginApprovalBroker(broker); + const onResolution = vi.fn(); + runBeforeToolCallMock.mockResolvedValue({ + requireApproval: { + pluginId: "test-plugin", + title: "Needs approval", + description: "Test approval request", + timeoutMs: 1, + timeoutBehavior: "allow", + onResolution, + }, + params: { adjusted: true }, + }); + + const result = await runBeforeToolCallHook({ + toolName: "exec", + params: { command: "ls" }, + toolCallId: "call-skill-timeout", + ctx: { agentId: "main", sessionKey: "agent:main:main" }, + }); + + expect(result).toEqual({ + blocked: true, + kind: "failure", + disposition: "timed_out", + deniedReason: "plugin-approval", + reason: "Approval timed out", + params: { command: "ls" }, + }); + expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.TIMEOUT); + expect(mockCallGatewayTool).not.toHaveBeenCalled(); + }); + + it("warns once per plugin when deprecated timeoutBehavior allow arrives, still failing closed", async () => { + agentToolsWarnSpy.mockClear(); + setEmbeddedMode(true); + const broker = new EmbeddedPluginApprovalBroker(); + setEmbeddedPluginApprovalBroker(broker); + runBeforeToolCallMock.mockResolvedValue({ + requireApproval: { + pluginId: "deprecated-timeout-plugin", + title: "Needs approval", + description: "Test approval request", + timeoutMs: 1, + timeoutBehavior: "allow", + }, + }); + + const first = await runBeforeToolCallHook({ + toolName: "exec", + params: { command: "ls" }, + toolCallId: "call-deprecated-warn-1", + ctx: { agentId: "main", sessionKey: "agent:main:main" }, + }); + const second = await runBeforeToolCallHook({ + toolName: "exec", + params: { command: "ls" }, + toolCallId: "call-deprecated-warn-2", + ctx: { agentId: "main", sessionKey: "agent:main:main" }, + }); + + expect(first).toMatchObject({ blocked: true, disposition: "timed_out" }); + expect(second).toMatchObject({ blocked: true, disposition: "timed_out" }); + const deprecationWarnings = agentToolsWarnSpy.mock.calls.filter( + ([message]) => + typeof message === "string" && + message.includes("deprecated-timeout-plugin") && + message.includes("timeoutBehavior"), + ); + expect(deprecationWarnings).toHaveLength(1); + }); + + it("blocks embedded allow decisions excluded by the request", async () => { + setEmbeddedMode(true); + const broker = new EmbeddedPluginApprovalBroker(); + setEmbeddedPluginApprovalBroker(broker); + vi.spyOn(broker, "request").mockResolvedValue({ + id: "plugin:unexpected-decision", + decision: PluginApprovalResolutions.ALLOW_ALWAYS, + }); + const onResolution = vi.fn(); + runBeforeToolCallMock.mockResolvedValue({ + requireApproval: { + pluginId: "test-plugin", + title: "Restricted approval", + description: "Allow once only", + allowedDecisions: ["allow-once", "deny"], + onResolution, + }, + params: { adjusted: true }, + }); + + const result = await runBeforeToolCallHook({ + toolName: "exec", + params: { command: "unsafe-command" }, + toolCallId: "call-restricted-approval", + ctx: { agentId: "main", sessionKey: "agent:main:main" }, + }); + + expect(result).toEqual({ + blocked: true, + kind: "failure", + disposition: "timed_out", + deniedReason: "plugin-approval", + reason: "Approval timed out", + params: { command: "unsafe-command" }, + }); + expect(onResolution).toHaveBeenCalledWith(PluginApprovalResolutions.TIMEOUT); + expect(mockCallGatewayTool).not.toHaveBeenCalled(); + }); + it("reports approval-required tools without opening an approval request", async () => { runBeforeToolCallMock.mockResolvedValue({ requireApproval: { diff --git a/src/agents/agent-tools.before-tool-call.ts b/src/agents/agent-tools.before-tool-call.ts index fbf19e3d67cd..081e65d41713 100644 --- a/src/agents/agent-tools.before-tool-call.ts +++ b/src/agents/agent-tools.before-tool-call.ts @@ -40,6 +40,7 @@ import { describeNativePluginApprovalClientSetup, resolveApprovalInitiatingSurfaceState, } from "../infra/exec-approval-surface.js"; +import { resolveCanonicalPluginApprovalRequestAllowedDecisions } from "../infra/plugin-approval-canonical-decisions.js"; import { DEFAULT_PLUGIN_APPROVAL_TIMEOUT_MS, MAX_PLUGIN_APPROVAL_TIMEOUT_MS, @@ -828,6 +829,24 @@ function emitToolBlockedSecurityEvent(params: { }); } +// Once-per-plugin-per-process deprecation signal; the field is ignored at +// runtime because unresolved approvals always fail closed on timeout. +const warnedDeprecatedTimeoutBehaviorPluginIds = new Set(); + +function warnDeprecatedApprovalTimeoutBehavior(approval: PluginApprovalRequest): void { + if (approval.timeoutBehavior !== "allow") { + return; + } + const pluginId = approval.pluginId ?? "unknown-plugin"; + if (warnedDeprecatedTimeoutBehaviorPluginIds.has(pluginId)) { + return; + } + warnedDeprecatedTimeoutBehaviorPluginIds.add(pluginId); + log.warn( + `plugin '${pluginId}' sets deprecated requireApproval.timeoutBehavior:"allow"; the field is ignored and approvals fail closed on timeout (see docs/plugins/plugin-permission-requests.md)`, + ); +} + function notifyPluginApprovalResolution( approval: PluginApprovalRequest, resolution: PluginApprovalResolution, @@ -845,6 +864,21 @@ function notifyPluginApprovalResolution( } } +function resolvePermittedPluginApprovalResolution( + decision: unknown, + allowedDecisions: readonly string[], +): PluginApprovalResolution { + if ( + (decision === PluginApprovalResolutions.ALLOW_ONCE || + decision === PluginApprovalResolutions.ALLOW_ALWAYS || + decision === PluginApprovalResolutions.DENY) && + allowedDecisions.includes(decision) + ) { + return decision; + } + return PluginApprovalResolutions.TIMEOUT; +} + function buildPluginApprovalFailureReason(params: { fallbackReason: string; ctx?: HookContext; @@ -894,6 +928,7 @@ async function requestPluginToolApproval(params: { const approval = params.approval; const timeoutMs = resolvePluginToolApprovalTimeoutMs(approval); const gatewayTimeoutMs = resolvePluginToolApprovalGatewayTimeoutMs(timeoutMs); + const allowedDecisions = resolveCanonicalPluginApprovalRequestAllowedDecisions(approval); let gatewayApprovalPhase: "none" | "request" | "wait" = "none"; try { const embeddedApprovalBroker = isEmbeddedMode() ? getEmbeddedPluginApprovalBroker() : null; @@ -918,16 +953,11 @@ async function requestPluginToolApproval(params: { signal: params.signal, }); const decision = result.decision; - const resolution: PluginApprovalResolution = - decision === PluginApprovalResolutions.ALLOW_ONCE || - decision === PluginApprovalResolutions.ALLOW_ALWAYS || - decision === PluginApprovalResolutions.DENY - ? decision - : PluginApprovalResolutions.TIMEOUT; + const resolution = resolvePermittedPluginApprovalResolution(decision, allowedDecisions); notifyPluginApprovalResolution(approval, resolution); if ( - decision === PluginApprovalResolutions.ALLOW_ONCE || - decision === PluginApprovalResolutions.ALLOW_ALWAYS + resolution === PluginApprovalResolutions.ALLOW_ONCE || + resolution === PluginApprovalResolutions.ALLOW_ALWAYS ) { return { blocked: false, @@ -935,7 +965,7 @@ async function requestPluginToolApproval(params: { approvalResolution: resolution, }; } - if (decision === PluginApprovalResolutions.DENY) { + if (resolution === PluginApprovalResolutions.DENY) { return { blocked: true, kind: "failure", @@ -945,13 +975,6 @@ async function requestPluginToolApproval(params: { params: params.baseParams, }; } - if (approval.timeoutBehavior === "allow") { - return { - blocked: false, - params: mergeParamsWithApprovalOverrides(params.baseParams, params.overrideParams), - approvalResolution: resolution, - }; - } // Veto carries the plugin-supplied reason; plain timeouts record a // timed_out failure disposition for the audit ledger. return approval.timeoutReason @@ -976,7 +999,7 @@ async function requestPluginToolApproval(params: { const requestResult: { id?: string; status?: string; - decision?: string | null; + decision?: unknown; deliveryRoute?: string; } = await callGatewayTool( "plugin.approval.request", @@ -1019,7 +1042,7 @@ async function requestPluginToolApproval(params: { }; } const hasImmediateDecision = Object.hasOwn(requestResult ?? {}, "decision"); - let decision: string | null | undefined; + let decision: unknown; if (hasImmediateDecision) { decision = requestResult?.decision; if (decision === null) { @@ -1042,7 +1065,7 @@ async function requestPluginToolApproval(params: { gatewayApprovalPhase = "wait"; const waitPromise: Promise<{ id?: string; - decision?: string | null; + decision?: unknown; }> = callGatewayTool( "plugin.approval.waitDecision", // Buffer beyond the approval timeout so the gateway can clean up @@ -1050,7 +1073,7 @@ async function requestPluginToolApproval(params: { { timeoutMs: gatewayTimeoutMs }, { id }, ); - let waitResult: { id?: string; decision?: string | null } | undefined; + let waitResult: { id?: string; decision?: unknown } | undefined; if (params.signal) { let onAbort: (() => void) | undefined; const abortPromise = new Promise((_, reject) => { @@ -1071,18 +1094,15 @@ async function requestPluginToolApproval(params: { } else { waitResult = await waitPromise; } - decision = waitResult?.decision; + // Bind the verdict to the request that parked this call. A stale or + // misrouted reply must never release a different tool gate. + decision = waitResult?.id === id ? waitResult.decision : undefined; } - const resolution: PluginApprovalResolution = - decision === PluginApprovalResolutions.ALLOW_ONCE || - decision === PluginApprovalResolutions.ALLOW_ALWAYS || - decision === PluginApprovalResolutions.DENY - ? decision - : PluginApprovalResolutions.TIMEOUT; + const resolution = resolvePermittedPluginApprovalResolution(decision, allowedDecisions); notifyPluginApprovalResolution(approval, resolution); if ( - decision === PluginApprovalResolutions.ALLOW_ONCE || - decision === PluginApprovalResolutions.ALLOW_ALWAYS + resolution === PluginApprovalResolutions.ALLOW_ONCE || + resolution === PluginApprovalResolutions.ALLOW_ALWAYS ) { return { blocked: false, @@ -1090,7 +1110,7 @@ async function requestPluginToolApproval(params: { approvalResolution: resolution, }; } - if (decision === PluginApprovalResolutions.DENY) { + if (resolution === PluginApprovalResolutions.DENY) { return { blocked: true, kind: "failure", @@ -1100,14 +1120,6 @@ async function requestPluginToolApproval(params: { params: params.baseParams, }; } - const timeoutBehavior = approval.timeoutBehavior ?? "deny"; - if (timeoutBehavior === "allow") { - return { - blocked: false, - params: mergeParamsWithApprovalOverrides(params.baseParams, params.overrideParams), - approvalResolution: resolution, - }; - } const fallbackTimeoutReason = approval.timeoutReason ?? "Approval timed out"; const timeoutReason = requestResult?.deliveryRoute === "turn-source" @@ -1201,6 +1213,7 @@ async function resolveBeforeToolCallApprovalOutcome(params: { if (!approval) { return undefined; } + warnDeprecatedApprovalTimeoutBehavior(approval); if (params.approvalMode === "defer") { return { blocked: false, diff --git a/src/agents/harness/native-hook-relay.approval-binding.test.ts b/src/agents/harness/native-hook-relay.approval-binding.test.ts new file mode 100644 index 000000000000..217fd2b2586d --- /dev/null +++ b/src/agents/harness/native-hook-relay.approval-binding.test.ts @@ -0,0 +1,86 @@ +// Covers gateway waitDecision id binding for native hook relay permission approvals. +import { afterEach, describe, expect, it, vi } from "vitest"; +import { callGatewayTool } from "../tools/gateway.js"; +import { invokeNativeHookRelay, registerNativeHookRelay, testing } from "./native-hook-relay.js"; + +vi.mock("../tools/gateway.js", async (importOriginal) => ({ + ...(await importOriginal()), + callGatewayTool: vi.fn(), +})); + +const mockCallGatewayTool = vi.mocked(callGatewayTool); + +afterEach(() => { + // restoreAllMocks does not clear call history on module-mock vi.fn()s. + mockCallGatewayTool.mockReset(); + vi.restoreAllMocks(); + testing.clearNativeHookRelaysForTests(); +}); + +function mockGatewayApproval(waitResult: { id?: string; decision?: string | null }) { + mockCallGatewayTool.mockImplementation(async (method: string) => { + if (method === "plugin.approval.request") { + return { id: "approval-1", status: "accepted" }; + } + if (method === "plugin.approval.waitDecision") { + return waitResult; + } + throw new Error(`unexpected gateway method: ${method}`); + }); +} + +async function invokePermissionRequest(relayId: string) { + return invokeNativeHookRelay({ + provider: "codex", + relayId, + event: "permission_request", + rawPayload: { + hook_event_name: "PermissionRequest", + cwd: "/repo", + tool_name: "Bash", + tool_use_id: "native-binding-call-1", + tool_input: { command: "printf binding" }, + }, + }); +} + +describe("native hook relay approval id binding", () => { + it("accepts a waitDecision reply bound to the requested approval id", async () => { + mockGatewayApproval({ id: "approval-1", decision: "allow-once" }); + const relay = registerNativeHookRelay({ + provider: "codex", + relayId: "codex-approval-binding-match", + sessionId: "session-1", + runId: "run-1", + }); + + const response = await invokePermissionRequest(relay.relayId); + + expect(JSON.parse(response.stdout)).toEqual({ + hookSpecificOutput: { + hookEventName: "PermissionRequest", + decision: { behavior: "allow" }, + }, + }); + }); + + it("defers when a waitDecision reply carries a different approval id", async () => { + mockGatewayApproval({ id: "approval-other", decision: "allow-once" }); + const relay = registerNativeHookRelay({ + provider: "codex", + relayId: "codex-approval-binding-mismatch", + sessionId: "session-1", + runId: "run-1", + }); + + const response = await invokePermissionRequest(relay.relayId); + + // A misrouted reply must never release the gate; the relay falls back to + // the provider's own approval path via the noop response. + expect(response).toEqual({ stdout: "", stderr: "", exitCode: 0 }); + expect(mockCallGatewayTool.mock.calls.map(([method]) => method)).toEqual([ + "plugin.approval.request", + "plugin.approval.waitDecision", + ]); + }); +}); diff --git a/src/agents/harness/native-hook-relay.ts b/src/agents/harness/native-hook-relay.ts index fce3379bd243..121854c65001 100644 --- a/src/agents/harness/native-hook-relay.ts +++ b/src/agents/harness/native-hook-relay.ts @@ -2132,7 +2132,9 @@ async function requestNativeHookRelayPermissionApproval( signal: request.signal, timeoutMs, }); - decision = waitResult?.decision; + // Bind the verdict to the request that parked this call. A stale or + // misrouted reply must never release a different tool gate. + decision = waitResult?.id === approvalId ? waitResult.decision : undefined; } if (decision === PluginApprovalResolutions.ALLOW_ONCE) { return "allow"; diff --git a/src/cli/gateway-cli.coverage.test.ts b/src/cli/gateway-cli.coverage.test.ts index 7835a5cf2edf..3be2c437b704 100644 --- a/src/cli/gateway-cli.coverage.test.ts +++ b/src/cli/gateway-cli.coverage.test.ts @@ -335,12 +335,9 @@ describe("gateway-cli coverage", () => { const costCalls = callGateway.mock.calls.map( ([raw]) => raw as { method?: string; timeoutMs?: number }, ); - for (const call of costCalls) { - expect(call.method).toBe("usage.cost"); - expect(typeof call.timeoutMs).toBe("number"); - expect(call.timeoutMs).toBeGreaterThan(0); - expect(call.timeoutMs).toBeLessThanOrEqual(10_000); - } + expect(costCalls.every((call) => call.method === "usage.cost")).toBe(true); + expect(costCalls.every((call) => (call.timeoutMs ?? 0) > 0)).toBe(true); + expect(costCalls.every((call) => (call.timeoutMs ?? 0) <= 10_000)).toBe(true); expect(costCalls[0]?.timeoutMs).toBe(10_000); expect(defaultRuntime.writeJson).toHaveBeenCalledWith( expect.objectContaining({ @@ -371,16 +368,15 @@ describe("gateway-cli coverage", () => { "--json", ]); + // A fast host can fit a second poll inside the 50ms budget; the contract + // is the budget bound on every call, not the poll count. expect(callGateway.mock.calls.length).toBeGreaterThanOrEqual(1); const costCalls = callGateway.mock.calls.map( ([raw]) => raw as { method?: string; timeoutMs?: number }, ); - for (const call of costCalls) { - expect(call.method).toBe("usage.cost"); - expect(typeof call.timeoutMs).toBe("number"); - expect(call.timeoutMs).toBeGreaterThan(0); - expect(call.timeoutMs).toBeLessThanOrEqual(50); - } + expect(costCalls.every((call) => call.method === "usage.cost")).toBe(true); + expect(costCalls.every((call) => (call.timeoutMs ?? 0) > 0)).toBe(true); + expect(costCalls.every((call) => (call.timeoutMs ?? 0) <= 50)).toBe(true); expect(runtimeErrors.join("\n")).toContain("Timed out waiting for usage cost cache refresh"); }, ); diff --git a/src/gateway/approval-session-audience.test.ts b/src/gateway/approval-session-audience.test.ts index 0487e7f66154..b8272bbad4c2 100644 --- a/src/gateway/approval-session-audience.test.ts +++ b/src/gateway/approval-session-audience.test.ts @@ -1,9 +1,23 @@ -import { describe, expect, it } from "vitest"; +import { describe, expect, it, vi } from "vitest"; import { + resolveApprovalFallbackAudienceSessionKey, + resolveApprovalSessionAudience, resolveApprovalSessionAudienceFromSources, + resolveApprovalSourceStreamKey, type ApprovalSessionAudienceSources, } from "./approval-session-audience.js"; +const getRuntimeConfigMock = vi.fn(() => ({}) as object); +vi.mock("../config/io.js", () => ({ + getRuntimeConfig: () => getRuntimeConfigMock(), +})); +vi.mock("../agents/subagent-registry-read.js", () => ({ + buildLatestSubagentRunReadIndex: () => ({ getLatestSubagentRun: () => undefined }), +})); +vi.mock("../config/sessions/session-accessor.js", () => ({ + loadSessionEntry: () => undefined, +})); + type GraphNode = { registry?: { controllerSessionKey?: string | null; @@ -32,6 +46,13 @@ function resolveAudience( } describe("resolveApprovalSessionAudienceFromSources", () => { + it("scopes a global source to the agent-specific stream key", () => { + expect(resolveApprovalSourceStreamKey(" global ", "Work Agent")).toBe( + "agent:work-agent:global", + ); + expect(resolveApprovalSourceStreamKey("agent:work:child", "work")).toBe("agent:work:child"); + }); + it("keeps the canonical source first when it has no ancestors", () => { expect( resolveAudience(" Child ", {}, (key) => `agent:main:${key.trim().toLowerCase()}`), @@ -139,3 +160,46 @@ describe("resolveApprovalSessionAudienceFromSources", () => { expect(audience.at(-1)).toBe("session-63"); }); }); + +describe("resolveApprovalFallbackAudienceSessionKey", () => { + it("canonicalizes configured main-key aliases when config loads", () => { + getRuntimeConfigMock.mockReturnValueOnce({ session: { mainKey: "boss" } }); + expect(resolveApprovalFallbackAudienceSessionKey("main", "work")).toBe("agent:work:boss"); + }); + + it("scopes unscoped aliases even when config loading throws", () => { + getRuntimeConfigMock.mockImplementationOnce(() => { + throw new Error("config unavailable"); + }); + expect(resolveApprovalFallbackAudienceSessionKey("child", "work")).toBe("agent:work:child"); + }); +}); + +describe("resolveApprovalSourceStreamKey fallback scoping", () => { + it("scopes raw fallback aliases to the raising agent", () => { + expect(resolveApprovalSourceStreamKey("child", "work")).toBe("agent:work:child"); + expect(resolveApprovalSourceStreamKey("GLOBAL", "work")).toBe("agent:work:global"); + }); + + it("keeps agent-scoped, unknown, and agent-less keys exact", () => { + expect(resolveApprovalSourceStreamKey("agent:other:child", "work")).toBe("agent:other:child"); + expect(resolveApprovalSourceStreamKey("unknown", "work")).toBe("unknown"); + expect(resolveApprovalSourceStreamKey("child", null)).toBe("child"); + }); +}); + +describe("resolveApprovalSessionAudience runtime scoping", () => { + it("scopes unscoped source aliases to the raising agent", () => { + expect(resolveApprovalSessionAudience("child", "work")).toEqual(["agent:work:child"]); + }); + + it("scopes a global source to the raising agent stream", () => { + expect(resolveApprovalSessionAudience("global", "work")).toEqual(["agent:work:global"]); + }); + + it("keeps explicit cross-agent source keys exact", () => { + expect(resolveApprovalSessionAudience("agent:other:child", "work")).toEqual([ + "agent:other:child", + ]); + }); +}); diff --git a/src/gateway/approval-session-audience.ts b/src/gateway/approval-session-audience.ts index 493e997d108d..efafd694c89d 100644 --- a/src/gateway/approval-session-audience.ts +++ b/src/gateway/approval-session-audience.ts @@ -1,8 +1,10 @@ +import { resolveDefaultAgentId } from "../agents/agent-scope.js"; import { buildLatestSubagentRunReadIndex } from "../agents/subagent-registry-read.js"; import { getRuntimeConfig } from "../config/io.js"; import { loadSessionEntry } from "../config/sessions/session-accessor.js"; import type { SessionEntry } from "../config/sessions/types.js"; import type { OpenClawConfig } from "../config/types.openclaw.js"; +import { normalizeAgentId, parseAgentSessionKey } from "../routing/session-key.js"; import { OPERATOR_APPROVAL_MAX_AUDIENCE_SESSION_KEYS } from "./operator-approval-store.js"; import { canonicalizeSpawnedByForAgent, @@ -102,32 +104,120 @@ export function resolveApprovalSessionAudienceFromSources(params: { function createRuntimeApprovalSessionAudienceSources( cfg: OpenClawConfig, + sourceAgentId?: string | null, ): ApprovalSessionAudienceSources { const subagentRuns = buildLatestSubagentRunReadIndex(); + const resolveStorageTarget = (sessionKey: string): { agentId: string; sessionKey: string } => { + const parsed = parseAgentSessionKey(sessionKey); + if (parsed?.rest.toLowerCase() === "global") { + return { agentId: normalizeAgentId(parsed.agentId), sessionKey: "global" }; + } + return { + agentId: resolveSessionStoreAgentId(cfg, sessionKey), + sessionKey, + }; + }; return { canonicalizeSessionKey: (sessionKey, relativeToSessionKey) => { if (!relativeToSessionKey) { - return resolveSessionStoreKey({ cfg, sessionKey }); + return canonicalizeApprovalSourceStreamKey(cfg, sessionKey, sourceAgentId); } const relativeAgentId = resolveSessionStoreAgentId(cfg, relativeToSessionKey); - return canonicalizeSpawnedByForAgent(cfg, relativeAgentId, sessionKey); + const canonical = canonicalizeSpawnedByForAgent(cfg, relativeAgentId, sessionKey); + return canonical ? resolveApprovalSourceStreamKey(canonical, relativeAgentId) : canonical; }, getLatestSubagentLineage: (sessionKey) => subagentRuns.getLatestSubagentRun(sessionKey), - getStoredSessionLineage: (sessionKey) => - loadSessionEntry({ - agentId: resolveSessionStoreAgentId(cfg, sessionKey), + getStoredSessionLineage: (sessionKey) => { + const target = resolveStorageTarget(sessionKey); + return loadSessionEntry({ + agentId: target.agentId, clone: false, hydrateSkillPromptRefs: false, - sessionKey, - }), + sessionKey: target.sessionKey, + }); + }, }; } /** Resolves an approval audience from the live registry and session stores. */ -export function resolveApprovalSessionAudience(sourceSessionKey: string): string[] { +export function resolveApprovalSessionAudience( + sourceSessionKey: string, + sourceAgentId?: string | null, +): string[] { const cfg = getRuntimeConfig(); return resolveApprovalSessionAudienceFromSources({ sourceSessionKey, - sources: createRuntimeApprovalSessionAudienceSources(cfg), + sources: createRuntimeApprovalSessionAudienceSources(cfg, sourceAgentId), }); } + +/** Canonicalize one source key against config: agent scoping, main-key aliases, global sentinel. */ +function canonicalizeApprovalSourceStreamKey( + cfg: OpenClawConfig, + sessionKey: string, + sourceAgentId?: string | null, +): string { + const ownerAgentId = normalizeAgentId(sourceAgentId ?? resolveDefaultAgentId(cfg)); + // Unscoped source aliases (e.g. "child", "main") must resolve against the + // raising agent's store, not the default agent's, or multi-agent audiences + // route to the wrong session streams. + const lowered = sessionKey.trim().toLowerCase(); + const scoped = + parseAgentSessionKey(sessionKey) || lowered === "global" || lowered === "unknown" + ? sessionKey + : `agent:${ownerAgentId}:${sessionKey}`; + const canonical = resolveSessionStoreKey({ cfg, sessionKey: scoped }); + // Storage uses the bare global sentinel, while live session streams are + // agent-scoped so one agent cannot receive another's global events. + return resolveApprovalSourceStreamKey(canonical, ownerAgentId); +} + +/** + * Fallback audience key when the lineage walk fails. Config-only + * canonicalization (agent scope, configured main-key aliases) still applies + * when the config loads; the pure-string form is the true last resort. + */ +/** Non-throwing audience resolver for injection into the approval manager. + * Lineage is routing metadata, not an approval safety prerequisite; when + * session stores are unavailable this preserves the agent-scoped source. */ +export function resolveApprovalSessionAudienceWithFallback( + sourceSessionKey: string, + sourceAgentId?: string | null, +): string[] { + try { + return resolveApprovalSessionAudience(sourceSessionKey, sourceAgentId); + } catch { + return [resolveApprovalFallbackAudienceSessionKey(sourceSessionKey, sourceAgentId)]; + } +} + +export function resolveApprovalFallbackAudienceSessionKey( + sourceSessionKey: string, + sourceAgentId?: string | null, +): string { + try { + return canonicalizeApprovalSourceStreamKey(getRuntimeConfig(), sourceSessionKey, sourceAgentId); + } catch { + return resolveApprovalSourceStreamKey(sourceSessionKey, sourceAgentId); + } +} + +/** Best-effort stream key used when lineage lookup is unavailable. */ +export function resolveApprovalSourceStreamKey( + sourceSessionKey: string, + sourceAgentId?: string | null, +): string { + const normalizedSessionKey = sourceSessionKey.trim(); + const lowered = normalizedSessionKey.toLowerCase(); + // Subscribers only know agent-scoped stream keys, so raw fallback inputs + // (bare "global", "main", unscoped child aliases) must scope to the raising + // agent or the persisted audience is unreachable exactly when lineage + // lookup already failed. "unknown" has no stream and stays bare. + if (!sourceAgentId || lowered === "unknown" || parseAgentSessionKey(normalizedSessionKey)) { + return normalizedSessionKey; + } + const agentId = normalizeAgentId(sourceAgentId); + return lowered === "global" + ? `agent:${agentId}:global` + : `agent:${agentId}:${normalizedSessionKey}`; +} diff --git a/src/gateway/exec-approval-manager.test.ts b/src/gateway/exec-approval-manager.test.ts index a5d7e9e595a0..e09937cb2789 100644 --- a/src/gateway/exec-approval-manager.test.ts +++ b/src/gateway/exec-approval-manager.test.ts @@ -5,14 +5,18 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; import { afterEach, describe, expect, it, vi } from "vitest"; -import type { ExecApprovalRequestPayload } from "../infra/exec-approvals.js"; +import type { ExecApprovalDecision, ExecApprovalRequestPayload } from "../infra/exec-approvals.js"; import type { PluginApprovalRequestPayload } from "../infra/plugin-approvals.js"; import { MAX_TIMER_TIMEOUT_MS } from "../shared/number-coercion.js"; import { closeOpenClawStateDatabase, openOpenClawStateDatabase, } from "../state/openclaw-state-db.js"; -import { ExecApprovalManager, type ExecApprovalManagerOptions } from "./exec-approval-manager.js"; +import { + ExecApprovalManager, + type ExecApprovalManagerOptions, + type OperatorApprovalLifecycleEvent, +} from "./exec-approval-manager.js"; import { getOperatorApproval, resolveOperatorApproval } from "./operator-approval-store.js"; type TimeoutCallback = Parameters[0]; @@ -35,6 +39,7 @@ describe("ExecApprovalManager", () => { options: { runtimeEpoch?: string; onError?: ExecApprovalManagerOptions["onError"]; + onLifecycle?: ExecApprovalManagerOptions["onLifecycle"]; } = {}, ) { const dir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approval-manager-")); @@ -49,6 +54,7 @@ describe("ExecApprovalManager", () => { resolveAllowedDecisions: () => ["allow-once", "deny"], resolveAudienceSessionKeys: (sessionKey) => [sessionKey, "agent:main:parent"], onError: options.onError, + onLifecycle: options.onLifecycle, }), }; } @@ -302,6 +308,235 @@ describe("ExecApprovalManager", () => { }); }); + it("emits pending only after durable insert and live waiter registration", async () => { + let durableAtCallback: ReturnType = null; + let waiterAtCallback: Promise | null = null; + const lifecycleEvents: OperatorApprovalLifecycleEvent[] = []; + // The lifecycle callback fires only during register(), after + // createPersistentManager has returned, so `created` is initialized. + const created = createPersistentManager({ + onLifecycle: (event) => { + lifecycleEvents.push(event); + if (event.phase === "pending") { + durableAtCallback = getOperatorApproval({ + id: event.record.id, + databaseOptions: created.databaseOptions, + }); + waiterAtCallback = created.manager.awaitDecision(event.record.id); + } + }, + }); + const manager = created.manager; + const record = manager.create( + { command: "echo ordered", sessionKey: "agent:main:child" }, + 60_000, + "approval-lifecycle-ordered", + ); + + const decisionPromise = manager.register(record, 60_000); + + expect(lifecycleEvents).toMatchObject([ + { + phase: "pending", + record: { + id: record.id, + status: "pending", + audienceSessionKeys: ["agent:main:child", "agent:main:parent"], + }, + }, + ]); + expect(durableAtCallback).toEqual(lifecycleEvents[0]?.record); + expect(waiterAtCallback).toBe(decisionPromise); + + manager.resolveDetailed(record.id, "deny", { kind: "system", id: null }); + await expect(decisionPromise).resolves.toBe("deny"); + }); + + it("passes the source agent when deriving a global-session stream audience", async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approval-manager-")); + tempDirs.push(dir); + const databaseOptions = { path: path.join(dir, "state.sqlite") }; + const resolveAudienceSessionKeys = vi.fn((sessionKey: string, agentId?: string | null) => [ + sessionKey === "global" && agentId ? `agent:${agentId}:global` : sessionKey, + ]); + const manager = new ExecApprovalManager({ + approvalKind: "exec", + persistence: { runtimeEpoch: "runtime-a", databaseOptions }, + resolveAllowedDecisions: () => ["allow-once", "deny"], + resolveAudienceSessionKeys, + }); + const record = manager.create( + { command: "echo global", sessionKey: "global", agentId: "work" }, + 60_000, + "approval-global-audience", + ); + const decisionPromise = manager.register(record, 60_000); + + expect(resolveAudienceSessionKeys).toHaveBeenCalledWith("global", "work"); + expect(getOperatorApproval({ id: record.id, databaseOptions })).toMatchObject({ + source: { sessionKey: "global", agentId: "work" }, + audienceSessionKeys: ["agent:work:global"], + }); + + manager.resolveDetailed(record.id, "deny", { kind: "system", id: null }); + await expect(decisionPromise).resolves.toBe("deny"); + }); + + it("emits one terminal event for the winning resolution and none for later answers", async () => { + const lifecycleEvents: OperatorApprovalLifecycleEvent[] = []; + const { manager } = createPersistentManager({ + onLifecycle: (event) => lifecycleEvents.push(event), + }); + const record = manager.create({ command: "echo race" }, 60_000, "approval-lifecycle-race"); + const decisionPromise = manager.register(record, 60_000); + + expect( + manager.resolveDetailed(record.id, "allow-once", { + kind: "device", + id: "control-ui", + }), + ).toMatchObject({ outcome: "resolved" }); + expect( + manager.resolveDetailed(record.id, "deny", { + kind: "channel", + id: "telegram", + }), + ).toMatchObject({ outcome: "already-resolved", retry: "conflict" }); + await expect(decisionPromise).resolves.toBe("allow-once"); + + expect(lifecycleEvents.map((event) => event.phase)).toEqual(["pending", "terminal"]); + expect(lifecycleEvents[1]?.record).toMatchObject({ + id: record.id, + status: "allowed", + decision: "allow-once", + resolver: { kind: "device", id: "control-ui" }, + }); + }); + + it("emits a terminal event when the durable timeout wins", async () => { + const timers = installTimerMocks(); + vi.spyOn(Date, "now").mockReturnValue(1_000); + const lifecycleEvents: OperatorApprovalLifecycleEvent[] = []; + const { manager } = createPersistentManager({ + onLifecycle: (event) => lifecycleEvents.push(event), + }); + const record = manager.create( + { command: "echo timeout" }, + 60_000, + "approval-lifecycle-timeout", + ); + const decisionPromise = manager.register(record, 60_000); + vi.mocked(Date.now).mockReturnValue(record.expiresAtMs); + + runTimer(timers[0]); + + await expect(decisionPromise).resolves.toBeNull(); + expect(lifecycleEvents.map((event) => event.phase)).toEqual(["pending", "terminal"]); + expect(lifecycleEvents[1]?.record).toMatchObject({ + id: record.id, + status: "expired", + decision: "deny", + terminalReason: "timeout", + }); + }); + + it("emits a terminal event for an explicit force-deny transition", async () => { + const lifecycleEvents: OperatorApprovalLifecycleEvent[] = []; + const { manager } = createPersistentManager({ + onLifecycle: (event) => lifecycleEvents.push(event), + }); + const record = manager.create( + { command: "echo malformed" }, + 60_000, + "approval-lifecycle-force-deny", + ); + const decisionPromise = manager.register(record, 60_000); + + expect( + manager.forceDenyDetailed(record.id, "malformed-verdict", { + kind: "system", + id: "invalid-verdict", + }), + ).toMatchObject({ outcome: "denied" }); + await expect(decisionPromise).resolves.toBe("deny"); + + expect(lifecycleEvents.map((event) => event.phase)).toEqual(["pending", "terminal"]); + expect(lifecycleEvents[1]?.record).toMatchObject({ + id: record.id, + status: "denied", + decision: "deny", + terminalReason: "malformed-verdict", + }); + }); + + it("isolates lifecycle callback failures from registration and resolution", async () => { + const onLifecycle = vi.fn(() => { + throw new Error("stream unavailable"); + }); + const { manager, databaseOptions } = createPersistentManager({ onLifecycle }); + const record = manager.create( + { command: "echo isolated" }, + 60_000, + "approval-lifecycle-isolation", + ); + + let decisionPromise!: Promise; + expect(() => { + decisionPromise = manager.register(record, 60_000); + }).not.toThrow(); + expect(() => + manager.resolveDetailed(record.id, "deny", { + kind: "device", + id: "control-ui", + }), + ).not.toThrow(); + await expect(decisionPromise).resolves.toBe("deny"); + + expect(onLifecycle).toHaveBeenCalledTimes(2); + expect(getOperatorApproval({ id: record.id, databaseOptions })).toMatchObject({ + status: "denied", + decision: "deny", + }); + }); + + it("does not re-emit pending for an idempotent persisted registration", () => { + installTimerMocks(); + const { manager, databaseOptions } = createPersistentManager(); + const record = manager.create( + { command: "echo replay", sessionKey: "agent:main:child" }, + 60_000, + "approval-lifecycle-existing", + ); + const originalPromise = manager.register(record, 60_000); + const onLifecycle = vi.fn(); + const replayManager = new ExecApprovalManager({ + approvalKind: "exec", + persistence: { runtimeEpoch: "runtime-a", databaseOptions }, + resolveAllowedDecisions: () => ["allow-once", "deny"], + resolveAudienceSessionKeys: (sessionKey) => [sessionKey, "agent:main:parent"], + onLifecycle, + }); + + const replayPromise = replayManager.register( + { ...record, request: { ...record.request } }, + 60_000, + ); + + expect(replayManager.awaitDecision(record.id)).toBe(replayPromise); + expect(onLifecycle).not.toHaveBeenCalled(); + expect(getOperatorApproval({ id: record.id, databaseOptions })).toMatchObject({ + id: record.id, + status: "pending", + }); + + manager.resolveDetailed(record.id, "deny", { kind: "system", id: null }); + replayManager.resolveDetailed(record.id, "deny", { kind: "system", id: null }); + return Promise.all([ + expect(originalPromise).resolves.toBe("deny"), + expect(replayPromise).resolves.toBe("deny"), + ]); + }); + it("persists only the reviewer-safe presentation while retaining the local request", async () => { const { manager, databaseOptions } = createPersistentManager(); const request: ExecApprovalRequestPayload = { @@ -547,6 +782,47 @@ describe("ExecApprovalManager", () => { }); }); + it("publishes durable expiry when storage recovery crosses the deadline", async () => { + installTimerMocks(); + vi.spyOn(Date, "now").mockReturnValue(1_000); + const lifecycleEvents: OperatorApprovalLifecycleEvent[] = []; + const { manager, databaseOptions, dir } = createPersistentManager({ + onLifecycle: (event) => lifecycleEvents.push(event), + }); + const record = manager.create( + { command: "echo expiry" }, + 1_000, + "approval-storage-recovery-expiry", + ); + const decisionPromise = manager.register(record, 1_000); + const validDatabasePath = databaseOptions.path; + const blocker = path.join(dir, "expiry-storage-blocker"); + fs.writeFileSync(blocker, "blocked"); + databaseOptions.path = path.join(blocker, "state.sqlite"); + + expect(() => + manager.resolveDetailed(record.id, "allow-once", { + kind: "device", + id: "control-ui", + }), + ).toThrow(); + await expect(decisionPromise).resolves.toBe("deny"); + + databaseOptions.path = validDatabasePath; + vi.mocked(Date.now).mockReturnValue(record.expiresAtMs); + expect( + manager.resolveDetailed(record.id, "allow-once", { + kind: "device", + id: "control-ui", + }), + ).toMatchObject({ outcome: "expired", record: { status: "expired" } }); + expect(lifecycleEvents.map((event) => event.phase)).toEqual(["pending", "terminal"]); + expect(lifecycleEvents[1]?.record).toMatchObject({ + status: "expired", + terminalReason: "timeout", + }); + }); + it("reconciles a durable terminal row into the existing local waiter", async () => { const { manager, databaseOptions } = createPersistentManager(); const record = manager.create({ command: "echo ok" }, 60_000, "approval-reconcile"); diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts index dbd8044d7c2a..5f1e7d511fac 100644 --- a/src/gateway/exec-approval-manager.ts +++ b/src/gateway/exec-approval-manager.ts @@ -92,11 +92,23 @@ export type ExecApprovalManagerOptions = { approvalKind?: OperatorApprovalKind; persistence?: OperatorApprovalPersistenceRuntime; resolveAllowedDecisions?: (request: TPayload) => readonly ExecApprovalDecision[]; - resolveAudienceSessionKeys?: (sourceSessionKey: string) => string[]; + /** Session-lineage audience policy is gateway-owned and injected as a + * non-throwing resolver; importing it here would close an agents->gateway + * barrel cycle. Absent resolver (tests) seeds only the raising session. */ + resolveAudienceSessionKeys?: ( + sourceSessionKey: string, + sourceAgentId?: string | null, + ) => string[]; onError?: ( error: Error, context: { approvalId: string; approvalKind: OperatorApprovalKind; operation: "expire" }, ) => void; + onLifecycle?: (event: OperatorApprovalLifecycleEvent) => void; +}; + +export type OperatorApprovalLifecycleEvent = { + phase: "pending" | "terminal"; + record: OperatorApprovalRecord; }; type WithLiveRecord = TResult extends { record: OperatorApprovalRecord } @@ -255,19 +267,17 @@ export class ExecApprovalManager { throw new Error(`approval id '${record.id}' already resolved`); } + let insertedRecord: OperatorApprovalRecord | null = null; if (persistence) { const source = resolveApprovalSource(record.request); let audienceSessionKeys: string[] = []; if (source.sessionKey) { - audienceSessionKeys = [source.sessionKey]; - if (this.options.resolveAudienceSessionKeys) { - try { - audienceSessionKeys = this.options.resolveAudienceSessionKeys(source.sessionKey); - } catch { - // Lineage is routing metadata, not an approval safety prerequisite. - // Preserve at least the source audience when session stores are unavailable. - } - } + // The injected resolver owns lineage lookup plus its own agent-scoped + // fallback and never throws. Without one (tests), seed the raw source. + audienceSessionKeys = this.options.resolveAudienceSessionKeys?.( + source.sessionKey, + source.agentId, + ) ?? [source.sessionKey]; } const inserted = insertOperatorApproval({ approval: { @@ -291,6 +301,9 @@ export class ExecApprovalManager { if (inserted.outcome === "conflict") { throw new Error(`approval id '${record.id}' conflicts with persisted state`); } + if (inserted.outcome === "inserted") { + insertedRecord = inserted.record; + } } let resolvePromise: (decision: ExecApprovalDecision | null) => void; @@ -313,9 +326,21 @@ export class ExecApprovalManager { }; this.pending.set(record.id, entry); this.scheduleExpiryTimer(entry); + if (insertedRecord) { + this.emitLifecycle({ phase: "pending", record: insertedRecord }); + } return promise; } + private emitLifecycle(event: OperatorApprovalLifecycleEvent): void { + try { + this.options.onLifecycle?.(event); + } catch { + // Stream fanout is observational. It must never change approval truth or + // prevent the durable first-answer transition from releasing its waiter. + } + } + private projectLocalRecord(record: ExecApprovalRecord): OperatorApprovalRecord | null { const presentation = buildApprovalPresentation({ kind: this.approvalKind, @@ -535,7 +560,7 @@ export class ExecApprovalManager { localDecision?: ExecApprovalDecision | null, localResolvedBy: string | null = null, localResolutionSource: ExecApprovalResolutionSource = "operator", - ): void { + ): boolean { const persistence = this.options.persistence; if ( record.kind !== this.approvalKind || @@ -543,7 +568,7 @@ export class ExecApprovalManager { record.status === "pending" || record.resolvedAtMs === null ) { - return; + return false; } const decision = localDecision === undefined @@ -551,7 +576,7 @@ export class ExecApprovalManager { ? record.decision : null : localDecision; - this.settleLocalEntry({ + const settled = this.settleLocalEntry({ recordId: record.id, decision, resolvedAtMs: record.resolvedAtMs, @@ -563,6 +588,15 @@ export class ExecApprovalManager { consumedBy: record.consumedBy, resolutionSource: localResolutionSource, }); + if (settled) { + this.emitLifecycle({ phase: "terminal", record }); + } + return settled; + } + + /** Settle one durable terminal transition and report whether this manager published it. */ + reconcileDurableTerminal(record: OperatorApprovalRecord): boolean { + return this.settleLocalFromStore(record); } /** Reconciles durable truth with an existing waiter without rehydrating its request. */ @@ -628,6 +662,9 @@ export class ExecApprovalManager { runtimeEpoch: persistence.runtimeEpoch, databaseOptions: persistence.databaseOptions, }); + if (result.outcome === "denied" || result.outcome === "expired") { + this.emitLifecycle({ phase: "terminal", record: result.record }); + } return attachLiveRecord(result, localEntry.record) as ExecApprovalForceDenyResult; } diff --git a/src/gateway/local-request-context.ts b/src/gateway/local-request-context.ts index a249bffcf1a8..261163c7e9db 100644 --- a/src/gateway/local-request-context.ts +++ b/src/gateway/local-request-context.ts @@ -131,7 +131,7 @@ function createLocalGatewayRequestContext( unsubscribeSessionEvents: (connId) => { sessionEvents.delete(connId); }, - subscribeSessionMessageEvents: () => {}, + subscribeSessionMessageEvents: () => undefined, unsubscribeSessionMessageEvents: () => {}, unsubscribeAllSessionEvents: (connId) => { sessionEvents.delete(connId); diff --git a/src/gateway/node-invoke-plugin-policy.test.ts b/src/gateway/node-invoke-plugin-policy.test.ts index e61a5f546c08..97c849bfb8f6 100644 --- a/src/gateway/node-invoke-plugin-policy.test.ts +++ b/src/gateway/node-invoke-plugin-policy.test.ts @@ -217,6 +217,8 @@ async function expectApprovalResolution( ok: true, payload: { id: record.id, decision: "allow-once" }, }); + expect(manager.getSnapshot(record.id)?.consumedDecision).toBe("allow-once"); + expect(manager.consumeAllowOnce(record.id)).toBe(false); } describe("applyPluginNodeInvokePolicy", () => { @@ -526,6 +528,31 @@ describe("applyPluginNodeInvokePolicy", () => { await expectApprovalResolution(resultPromise, manager, record); }); + it("fails closed when the allow-once claim cannot be consumed", async () => { + const manager = new ExecApprovalManager(); + vi.spyOn(manager, "consumeAllowOnce").mockReturnValue(false); + setDangerousDemoCommandRegistry([createApprovalRequestPolicy()]); + const { context } = createContext({ + pluginApprovalManager: manager, + getApprovalClientConnIds: createApprovalClientLookup([ + createApprovalClient({ + connId: "conn-owner-approval", + clientId: "client-owner", + deviceId: "device-owner", + }), + ]), + }); + const resultPromise = invokeDemoPolicy(context, createOperatorClient()); + + const record = await expectSinglePendingApproval(manager); + expect(manager.resolve(record.id, "allow-once")).toBe(true); + + await expect(resultPromise).resolves.toStrictEqual({ + ok: true, + payload: { id: record.id, decision: null }, + }); + }); + it("fails closed before routing an unrenderable persistent policy approval", async () => { const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-node-policy-approval-")); tempDirs.push(stateDir); diff --git a/src/gateway/node-invoke-plugin-policy.ts b/src/gateway/node-invoke-plugin-policy.ts index 4a510c57cda4..5184f82fbfeb 100644 --- a/src/gateway/node-invoke-plugin-policy.ts +++ b/src/gateway/node-invoke-plugin-policy.ts @@ -145,7 +145,16 @@ function createApprovalRuntime(params: { }); }, }); - return { id: record.id, decision: await decisionPromise }; + const decision = await decisionPromise; + // This return hands execution authority to the plugin policy. Claim a + // one-shot decision here so observation or retry cannot replay it. + if ( + decision === "allow-once" && + !manager.consumeAllowOnce(record.id, `plugin.node.invoke:${record.id}`) + ) { + return { id: record.id, decision: null }; + } + return { id: record.id, decision }; }, }; } diff --git a/src/gateway/operator-approval-session-events.test.ts b/src/gateway/operator-approval-session-events.test.ts new file mode 100644 index 000000000000..8f0550df24a3 --- /dev/null +++ b/src/gateway/operator-approval-session-events.test.ts @@ -0,0 +1,589 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import { buildApprovalResolutionRef } from "../infra/approval-resolution-ref.js"; +import { + closeOpenClawStateDatabaseForTest, + type OpenClawStateDatabaseOptions, +} from "../state/openclaw-state-db.js"; +import { ExecApprovalManager } from "./exec-approval-manager.js"; +import { createOperatorApprovalSessionEventRuntime } from "./operator-approval-session-events.js"; +import { + insertOperatorApproval, + resolveOperatorApproval, + type NewOperatorApproval, + type OperatorApprovalRecord, +} from "./operator-approval-store.js"; +import type { GatewayBroadcastToConnIdsFn } from "./server-broadcast-types.js"; +import { createSessionMessageSubscriberRegistry } from "./server-chat-state.js"; +import type { GatewayClient } from "./server-methods/types.js"; + +const SOURCE_SESSION_KEY = "agent:main:child"; +const PARENT_SESSION_KEY = "agent:main:parent"; +const SIBLING_SESSION_KEY = "agent:main:parent:sibling"; +const tempDirs: string[] = []; + +function createDatabaseOptions(): OpenClawStateDatabaseOptions { + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "openclaw-approval-events-")); + tempDirs.push(stateDir); + return { env: { ...process.env, OPENCLAW_STATE_DIR: stateDir } }; +} + +function createClient(params: { + connId: string; + scopes: string[]; + deviceId?: string; + invalidated?: boolean; +}): GatewayClient { + return { + connId: params.connId, + connect: { + client: { id: "approval-session-events", displayName: "Approval Session Events" }, + scopes: params.scopes, + ...(params.deviceId ? { device: { id: params.deviceId } } : {}), + }, + ...(params.invalidated ? { invalidated: true } : {}), + } as unknown as GatewayClient; +} + +function createPendingRecord( + params: { + id?: string; + audienceSessionKeys?: string[]; + sourceSessionKey?: string | null; + reviewerDeviceIds?: string[]; + createdAtMs?: number; + expiresAtMs?: number; + } = {}, +): OperatorApprovalRecord { + const id = params.id ?? "approval:child/request?1"; + const createdAtMs = params.createdAtMs ?? 1_000; + return { + id, + resolutionRef: buildApprovalResolutionRef({ approvalId: id, approvalKind: "exec" }), + kind: "exec", + status: "pending", + presentation: { + kind: "exec", + commandText: "printf session-approval", + commandPreview: "printf session-approval", + warningText: "Review this command", + host: "gateway", + nodeId: null, + agentId: "main", + allowedDecisions: ["allow-once", "allow-always", "deny"], + }, + requester: { + deviceId: "requester-device", + clientId: "requester-client", + deviceTokenAuth: true, + }, + reviewerDeviceIds: params.reviewerDeviceIds ?? ["reviewer-device"], + source: { + agentId: "main", + sessionKey: params.sourceSessionKey ?? SOURCE_SESSION_KEY, + sessionId: "private-session-id", + runId: "private-run-id", + toolCallId: "private-tool-call-id", + toolName: "exec", + }, + audienceSessionKeys: params.audienceSessionKeys ?? [SOURCE_SESSION_KEY, PARENT_SESSION_KEY], + runtimeEpoch: "private-runtime-epoch", + createdAtMs, + expiresAtMs: params.expiresAtMs ?? 10_000, + updatedAtMs: createdAtMs, + decision: null, + terminalReason: null, + resolvedAtMs: null, + resolver: null, + consumedAtMs: null, + consumedBy: null, + }; +} + +function createTerminalRecord( + pending: OperatorApprovalRecord, + resolvedAtMs = 2_000, +): OperatorApprovalRecord { + return { + ...pending, + status: "denied", + updatedAtMs: resolvedAtMs, + decision: "deny", + terminalReason: "user", + resolvedAtMs, + resolver: { kind: "device", id: "reviewer-device" }, + }; +} + +function createRuntime(params: { + clients: GatewayClient[]; + databaseOptions?: OpenClawStateDatabaseOptions; + now?: () => number; + controlUiBasePath?: string; + reconcileTerminal?: Parameters< + typeof createOperatorApprovalSessionEventRuntime + >[0]["reconcileTerminal"]; +}) { + const subscribers = createSessionMessageSubscriberRegistry(); + const broadcastToConnIds = vi.fn(); + const runtime = createOperatorApprovalSessionEventRuntime({ + clients: params.clients, + sessionMessageSubscribers: subscribers, + broadcastToConnIds, + databaseOptions: params.databaseOptions, + controlUiBasePath: params.controlUiBasePath, + now: params.now, + reconcileTerminal: params.reconcileTerminal, + }); + return { broadcastToConnIds, runtime, subscribers }; +} + +function insertPendingApproval(params: { + databaseOptions: OpenClawStateDatabaseOptions; + id: string; + audienceSessionKeys: string[]; + createdAtMs: number; + expiresAtMs: number; + reviewerDeviceIds?: string[]; +}): OperatorApprovalRecord { + const record = createPendingRecord(params); + const approval: NewOperatorApproval = { + id: record.id, + kind: record.kind, + presentation: record.presentation, + requester: record.requester, + reviewerDeviceIds: params.reviewerDeviceIds ?? record.reviewerDeviceIds, + source: record.source, + audienceSessionKeys: record.audienceSessionKeys, + runtimeEpoch: record.runtimeEpoch, + createdAtMs: record.createdAtMs, + expiresAtMs: record.expiresAtMs, + }; + const inserted = insertOperatorApproval({ approval, databaseOptions: params.databaseOptions }); + if (inserted.outcome !== "inserted") { + throw new Error(`expected approval '${params.id}' to be inserted`); + } + return inserted.record; +} + +describe("operator approval session events", () => { + afterEach(() => { + vi.useRealTimers(); + vi.restoreAllMocks(); + closeOpenClawStateDatabaseForTest(); + for (const dir of tempDirs.splice(0)) { + fs.rmSync(dir, { force: true, recursive: true }); + } + }); + + it("targets exact opted-in source and ancestor audiences with reviewer authorization", () => { + const clients = [ + createClient({ connId: "source-admin", scopes: ["operator.admin"] }), + createClient({ + connId: "source-device", + scopes: ["operator.approvals"], + deviceId: "source-reviewer", + }), + createClient({ connId: "source-no-device", scopes: ["operator.approvals"] }), + createClient({ + connId: "source-unrelated-device", + scopes: ["operator.approvals"], + deviceId: "unrelated-device", + }), + createClient({ + connId: "source-requester-device", + scopes: ["operator.approvals"], + deviceId: "requester-device", + }), + createClient({ + connId: "source-no-scope", + scopes: ["operator.read"], + deviceId: "unprivileged-device", + }), + createClient({ connId: "source-not-opted-in", scopes: ["operator.admin"] }), + createClient({ + connId: "source-invalidated", + scopes: ["operator.admin"], + invalidated: true, + }), + createClient({ + connId: "parent-device", + scopes: ["operator.approvals"], + deviceId: "parent-reviewer", + }), + createClient({ connId: "sibling-admin", scopes: ["operator.admin"] }), + ]; + const { broadcastToConnIds, runtime, subscribers } = createRuntime({ + clients, + controlUiBasePath: "/operator/", + }); + for (const connId of [ + "source-admin", + "source-device", + "source-no-device", + "source-unrelated-device", + "source-requester-device", + "source-no-scope", + "source-invalidated", + ]) { + subscribers.subscribe(connId, SOURCE_SESSION_KEY, { includeApprovals: true }); + } + subscribers.subscribe("source-not-opted-in", SOURCE_SESSION_KEY); + subscribers.subscribe("parent-device", PARENT_SESSION_KEY, { includeApprovals: true }); + subscribers.subscribe("sibling-admin", SIBLING_SESSION_KEY, { includeApprovals: true }); + + const record = createPendingRecord({ + reviewerDeviceIds: ["source-reviewer", "parent-reviewer"], + }); + runtime.publish({ phase: "pending", record }); + + expect(broadcastToConnIds).toHaveBeenCalledTimes(2); + expect(broadcastToConnIds).toHaveBeenNthCalledWith( + 1, + "session.approval", + { + sessionKey: SOURCE_SESSION_KEY, + sourceSessionKey: SOURCE_SESSION_KEY, + phase: "pending", + updatedAtMs: 1_000, + approval: { + id: record.id, + status: "pending", + presentation: record.presentation, + urlPath: "/operator/approve/approval%3Achild%2Frequest%3F1", + createdAtMs: 1_000, + expiresAtMs: 10_000, + }, + }, + new Set(["source-admin", "source-device"]), + ); + expect(broadcastToConnIds).toHaveBeenNthCalledWith( + 2, + "session.approval", + expect.objectContaining({ + sessionKey: PARENT_SESSION_KEY, + sourceSessionKey: SOURCE_SESSION_KEY, + phase: "pending", + }), + new Set(["parent-device"]), + ); + + const payloads = broadcastToConnIds.mock.calls.map((call) => call[1]); + expect(payloads).not.toContainEqual( + expect.objectContaining({ sessionKey: SIBLING_SESSION_KEY }), + ); + const serialized = JSON.stringify(payloads); + expect(serialized).not.toContain("requester-device"); + expect(serialized).not.toContain("requester-client"); + expect(serialized).not.toContain("private-session-id"); + expect(serialized).not.toContain("private-run-id"); + expect(serialized).not.toContain("private-tool-call-id"); + expect(serialized).not.toContain("private-runtime-epoch"); + }); + + it("publishes the agent-scoped stream key for global-scope sources", () => { + const client = createClient({ connId: "admin", scopes: ["operator.admin"] }); + const { broadcastToConnIds, runtime, subscribers } = createRuntime({ clients: [client] }); + subscribers.subscribe("admin", "agent:main:global", { includeApprovals: true }); + + // Storage records the bare "global" sentinel; subscribers only know the + // agent-scoped stream key, so the published event must carry that form. + const pending = createPendingRecord({ + sourceSessionKey: "global", + audienceSessionKeys: ["agent:main:global"], + }); + runtime.publish({ phase: "pending", record: pending }); + runtime.publish({ phase: "terminal", record: createTerminalRecord(pending) }); + + expect(broadcastToConnIds).toHaveBeenCalledTimes(2); + expect(broadcastToConnIds).toHaveBeenNthCalledWith( + 1, + "session.approval", + expect.objectContaining({ + sessionKey: "agent:main:global", + sourceSessionKey: "agent:main:global", + phase: "pending", + }), + new Set(["admin"]), + ); + expect(broadcastToConnIds).toHaveBeenNthCalledWith( + 2, + "session.approval", + expect.objectContaining({ + sessionKey: "agent:main:global", + sourceSessionKey: "agent:main:global", + phase: "terminal", + }), + new Set(["admin"]), + ); + }); + + it("publishes the canonical audience source key for unscoped session aliases", () => { + const client = createClient({ connId: "admin", scopes: ["operator.admin"] }); + const { broadcastToConnIds, runtime, subscribers } = createRuntime({ clients: [client] }); + subscribers.subscribe("admin", "agent:work:child", { includeApprovals: true }); + + // The persisted source may be a raw unscoped alias; subscribers must see + // the canonical stream key the audience walk seeded first. + const pending = createPendingRecord({ + sourceSessionKey: "child", + audienceSessionKeys: ["agent:work:child", "agent:work:parent"], + }); + runtime.publish({ phase: "pending", record: pending }); + + expect(broadcastToConnIds).toHaveBeenCalledWith( + "session.approval", + expect.objectContaining({ + sessionKey: "agent:work:child", + sourceSessionKey: "agent:work:child", + phase: "pending", + }), + new Set(["admin"]), + ); + }); + + it("publishes terminal state and rejects lifecycle phases inconsistent with durable status", () => { + const client = createClient({ connId: "admin", scopes: ["operator.admin"] }); + const { broadcastToConnIds, runtime, subscribers } = createRuntime({ clients: [client] }); + subscribers.subscribe("admin", SOURCE_SESSION_KEY, { includeApprovals: true }); + + const pending = createPendingRecord({ audienceSessionKeys: [SOURCE_SESSION_KEY] }); + const terminal = createTerminalRecord(pending); + runtime.publish({ phase: "terminal", record: pending }); + runtime.publish({ phase: "pending", record: terminal }); + expect(broadcastToConnIds).not.toHaveBeenCalled(); + + runtime.publish({ phase: "terminal", record: terminal }); + + expect(broadcastToConnIds).toHaveBeenCalledOnce(); + expect(broadcastToConnIds).toHaveBeenCalledWith( + "session.approval", + { + sessionKey: SOURCE_SESSION_KEY, + sourceSessionKey: SOURCE_SESSION_KEY, + phase: "terminal", + updatedAtMs: 2_000, + approval: { + id: terminal.id, + status: "denied", + decision: "deny", + reason: "user", + presentation: terminal.presentation, + urlPath: `/approve/${encodeURIComponent(terminal.id)}`, + createdAtMs: 1_000, + expiresAtMs: 10_000, + resolvedAtMs: 2_000, + }, + }, + new Set(["admin"]), + ); + }); + + it("returns the authoritative sanitized pending set for one exact audience", () => { + const databaseOptions = createDatabaseOptions(); + insertPendingApproval({ + databaseOptions, + id: "source-and-parent", + audienceSessionKeys: [SOURCE_SESSION_KEY, PARENT_SESSION_KEY], + createdAtMs: 1_000, + expiresAtMs: 10_000, + }); + const parentOnly = insertPendingApproval({ + databaseOptions, + id: "parent-only", + audienceSessionKeys: [PARENT_SESSION_KEY], + createdAtMs: 1_001, + expiresAtMs: 10_000, + }); + insertPendingApproval({ + databaseOptions, + id: "sibling-only", + audienceSessionKeys: [SIBLING_SESSION_KEY], + createdAtMs: 1_002, + expiresAtMs: 10_000, + }); + const resolved = insertPendingApproval({ + databaseOptions, + id: "already-resolved", + audienceSessionKeys: [PARENT_SESSION_KEY], + createdAtMs: 1_003, + expiresAtMs: 10_000, + }); + expect( + resolveOperatorApproval({ + id: resolved.id, + decision: "deny", + resolver: { kind: "device", id: "reviewer-device" }, + nowMs: 2_000, + databaseOptions, + }), + ).toMatchObject({ outcome: "resolved" }); + const { runtime } = createRuntime({ + clients: [], + databaseOptions, + controlUiBasePath: "/operator", + now: () => 5_000, + }); + + const replayReviewer = createClient({ + connId: "replay-reviewer", + scopes: ["operator.approvals"], + deviceId: "reviewer-device", + }); + expect(runtime.replay(PARENT_SESSION_KEY, replayReviewer)).toEqual({ + sessionKey: PARENT_SESSION_KEY, + updatedAtMs: 5_000, + truncated: false, + approvals: [ + { + id: "source-and-parent", + status: "pending", + presentation: createPendingRecord({ id: "source-and-parent" }).presentation, + urlPath: "/operator/approve/source-and-parent", + createdAtMs: 1_000, + expiresAtMs: 10_000, + }, + { + id: parentOnly.id, + status: "pending", + presentation: parentOnly.presentation, + urlPath: "/operator/approve/parent-only", + createdAtMs: 1_001, + expiresAtMs: 10_000, + }, + ], + }); + expect( + runtime.replay( + PARENT_SESSION_KEY, + createClient({ + connId: "unrelated-replay", + scopes: ["operator.approvals"], + deviceId: "unrelated-device", + }), + ), + ).toEqual({ + sessionKey: PARENT_SESSION_KEY, + updatedAtMs: 5_000, + truncated: false, + approvals: [], + }); + }); + + it("publishes replay-triggered expiry to existing ancestor recipients before an empty replay", () => { + const databaseOptions = createDatabaseOptions(); + insertPendingApproval({ + databaseOptions, + id: "expired-child-approval", + audienceSessionKeys: [SOURCE_SESSION_KEY, PARENT_SESSION_KEY], + createdAtMs: 1_000, + expiresAtMs: 4_000, + reviewerDeviceIds: ["parent-device"], + }); + const parent = createClient({ + connId: "parent-reviewer", + scopes: ["operator.approvals"], + deviceId: "parent-device", + }); + const { broadcastToConnIds, runtime, subscribers } = createRuntime({ + clients: [parent], + databaseOptions, + now: () => 5_000, + }); + subscribers.subscribe("parent-reviewer", PARENT_SESSION_KEY, { includeApprovals: true }); + + const replay = runtime.replay(SOURCE_SESSION_KEY, parent); + + expect(broadcastToConnIds).toHaveBeenCalledOnce(); + expect(broadcastToConnIds).toHaveBeenCalledWith( + "session.approval", + expect.objectContaining({ + sessionKey: PARENT_SESSION_KEY, + sourceSessionKey: SOURCE_SESSION_KEY, + phase: "terminal", + updatedAtMs: 5_000, + approval: expect.objectContaining({ + id: "expired-child-approval", + status: "expired", + reason: "timeout", + resolvedAtMs: 5_000, + }), + }), + new Set(["parent-reviewer"]), + ); + expect(replay).toEqual({ + sessionKey: SOURCE_SESSION_KEY, + updatedAtMs: 5_000, + approvals: [], + truncated: false, + }); + }); + + it("settles the owning waiter and publishes replay-triggered expiry once", async () => { + vi.useFakeTimers(); + vi.setSystemTime(1_000); + const databaseOptions = createDatabaseOptions(); + // Replay reconciliation runs only after the manager exists; route it + // through a holder so both sides can stay const. + const managerHolder: { current?: ExecApprovalManager } = {}; + const parent = createClient({ + connId: "parent-reviewer", + scopes: ["operator.approvals"], + deviceId: "parent-device", + }); + const harness = createRuntime({ + clients: [parent], + databaseOptions, + now: () => Date.now(), + reconcileTerminal: (record) => + managerHolder.current?.reconcileDurableTerminal(record) ?? false, + }); + const runtime = harness.runtime; + const manager = new ExecApprovalManager({ + approvalKind: "exec", + persistence: { runtimeEpoch: "session-events", databaseOptions }, + resolveAllowedDecisions: () => ["allow-once", "deny"], + resolveAudienceSessionKeys: () => [SOURCE_SESSION_KEY, PARENT_SESSION_KEY], + onLifecycle: (event) => runtime.publish(event), + }); + managerHolder.current = manager; + harness.subscribers.subscribe("parent-reviewer", PARENT_SESSION_KEY, { + includeApprovals: true, + }); + const record = manager.create( + { + command: "printf replay-expiry", + sessionKey: SOURCE_SESSION_KEY, + agentId: "main", + }, + 3_000, + "replay-expiry-with-waiter", + ); + const decisionPromise = manager.register(record, 3_000); + harness.broadcastToConnIds.mockClear(); + vi.setSystemTime(record.expiresAtMs); + + expect(runtime.replay(SOURCE_SESSION_KEY, parent)).toEqual({ + sessionKey: SOURCE_SESSION_KEY, + updatedAtMs: record.expiresAtMs, + approvals: [], + truncated: false, + }); + await expect(decisionPromise).resolves.toBeNull(); + expect(harness.broadcastToConnIds).toHaveBeenCalledOnce(); + expect(harness.broadcastToConnIds).toHaveBeenCalledWith( + "session.approval", + expect.objectContaining({ + sessionKey: PARENT_SESSION_KEY, + phase: "terminal", + approval: expect.objectContaining({ status: "expired" }), + }), + new Set(["parent-reviewer"]), + ); + + await vi.advanceTimersByTimeAsync(20_000); + expect(harness.broadcastToConnIds).toHaveBeenCalledOnce(); + }); +}); diff --git a/src/gateway/operator-approval-session-events.ts b/src/gateway/operator-approval-session-events.ts new file mode 100644 index 000000000000..2d22460cd48f --- /dev/null +++ b/src/gateway/operator-approval-session-events.ts @@ -0,0 +1,150 @@ +import type { + PendingApprovalSnapshot, + SessionApprovalEvent, + SessionApprovalReplay, +} from "../../packages/gateway-protocol/src/index.js"; +import type { OpenClawStateDatabaseOptions } from "../state/openclaw-state-db.js"; +import { resolveApprovalSourceStreamKey } from "./approval-session-audience.js"; +import { normalizeControlUiBasePath } from "./control-ui-shared.js"; +import type { OperatorApprovalLifecycleEvent } from "./exec-approval-manager.js"; +import { canAccessOperatorApproval } from "./operator-approval-authorization.js"; +import { projectOperatorApprovalSnapshot } from "./operator-approval-snapshot.js"; +import { + expireDueOperatorApprovals, + listPendingOperatorApprovals, + type OperatorApprovalRecord, +} from "./operator-approval-store.js"; +import type { GatewayBroadcastToConnIdsFn } from "./server-broadcast-types.js"; +import type { SessionMessageSubscriberRegistry } from "./server-chat-state.js"; +import type { GatewayClient } from "./server-methods/types.js"; + +const MAX_SESSION_APPROVAL_REPLAY = 1_000; +type ApprovalSessionClient = GatewayClient & { invalidated?: boolean }; + +export type OperatorApprovalSessionEventRuntime = { + publish: (event: OperatorApprovalLifecycleEvent) => void; + replay: (sessionKey: string, client: GatewayClient | null) => SessionApprovalReplay; +}; + +/** Project durable approval truth to exact, explicitly opted-in session audiences. */ +export function createOperatorApprovalSessionEventRuntime(params: { + clients: Iterable; + sessionMessageSubscribers: Pick; + broadcastToConnIds: GatewayBroadcastToConnIdsFn; + controlUiBasePath?: string; + databaseOptions?: OpenClawStateDatabaseOptions; + now?: () => number; + reconcileTerminal?: (record: OperatorApprovalRecord) => boolean; +}): OperatorApprovalSessionEventRuntime { + const controlUiBasePath = normalizeControlUiBasePath(params.controlUiBasePath); + const now = params.now ?? Date.now; + + const canAccessRecord = (client: GatewayClient | null, record: OperatorApprovalRecord): boolean => + canAccessOperatorApproval({ + client, + binding: { reviewerDeviceIds: record.reviewerDeviceIds }, + }); + + const authorizedRecipients = ( + sessionKey: string, + record: OperatorApprovalRecord, + ): ReadonlySet => { + const subscribed = params.sessionMessageSubscribers.getApprovals(sessionKey); + if (subscribed.size === 0) { + return subscribed; + } + const recipients = new Set(); + for (const client of params.clients) { + const connId = client.connId; + if ( + !client.invalidated && + connId && + subscribed.has(connId) && + canAccessRecord(client, record) + ) { + recipients.add(connId); + } + } + return recipients; + }; + + const publish = (event: OperatorApprovalLifecycleEvent): void => { + const approval = projectOperatorApprovalSnapshot(event.record, controlUiBasePath); + if (!approval || event.record.audienceSessionKeys.length === 0) { + return; + } + // The audience walk seeds the fully canonicalized source stream key as its + // first entry; publish that exact form so parents can correlate the event + // with a stream key they subscribed to. Raw source aliases (bare "global", + // "main", unscoped child keys) never reach subscribers. + const sourceStreamKey = + event.record.audienceSessionKeys[0] ?? + (event.record.source.sessionKey + ? resolveApprovalSourceStreamKey( + event.record.source.sessionKey, + event.record.source.agentId, + ) + : null); + for (const sessionKey of event.record.audienceSessionKeys) { + const recipients = authorizedRecipients(sessionKey, event.record); + if (recipients.size === 0) { + continue; + } + const common = { + sessionKey, + ...(sourceStreamKey ? { sourceSessionKey: sourceStreamKey } : {}), + updatedAtMs: event.record.updatedAtMs, + }; + let payload: SessionApprovalEvent; + if (event.phase === "pending") { + if (approval.status !== "pending") { + continue; + } + payload = { ...common, phase: "pending", approval }; + } else { + if (approval.status === "pending") { + continue; + } + payload = { ...common, phase: "terminal", approval }; + } + params.broadcastToConnIds("session.approval", payload, recipients); + } + }; + + return { + publish, + replay: (sessionKey, client) => { + const snapshotAtMs = now(); + const expired = expireDueOperatorApprovals({ + nowMs: snapshotAtMs, + databaseOptions: params.databaseOptions, + }); + // A replay read can be the first observer after a suspended timer. Emit + // the durable timeout tombstone before returning the authoritative set. + for (const record of expired.records) { + if (params.reconcileTerminal?.(record) !== true) { + publish({ phase: "terminal", record }); + } + } + const approvals: PendingApprovalSnapshot[] = []; + const records = listPendingOperatorApprovals({ + audienceSessionKey: sessionKey, + recordFilter: (record) => canAccessRecord(client, record), + limit: MAX_SESSION_APPROVAL_REPLAY + 1, + nowMs: snapshotAtMs, + databaseOptions: params.databaseOptions, + }); + const truncated = records.length > MAX_SESSION_APPROVAL_REPLAY; + for (const record of records) { + if (approvals.length === MAX_SESSION_APPROVAL_REPLAY) { + return { sessionKey, updatedAtMs: snapshotAtMs, approvals, truncated: true }; + } + const approval = projectOperatorApprovalSnapshot(record, controlUiBasePath); + if (approval?.status === "pending") { + approvals.push(approval); + } + } + return { sessionKey, updatedAtMs: snapshotAtMs, approvals, truncated }; + }, + }; +} diff --git a/src/gateway/operator-approval-snapshot.ts b/src/gateway/operator-approval-snapshot.ts new file mode 100644 index 000000000000..32bed4559ea3 --- /dev/null +++ b/src/gateway/operator-approval-snapshot.ts @@ -0,0 +1,38 @@ +import type { ApprovalSnapshot } from "../../packages/gateway-protocol/src/index.js"; +import type { OperatorApprovalRecord } from "./operator-approval-store.js"; + +/** Project one durable row into the reviewer-safe public approval shape. */ +export function projectOperatorApprovalSnapshot( + record: OperatorApprovalRecord, + controlUiBasePath: string, +): ApprovalSnapshot | null { + const common = { + id: record.id, + status: record.status, + presentation: record.presentation, + urlPath: `${controlUiBasePath}/approve/${encodeURIComponent(record.id)}`, + createdAtMs: record.createdAtMs, + expiresAtMs: record.expiresAtMs, + }; + if (record.status === "pending") { + return common as ApprovalSnapshot; + } + if (record.resolvedAtMs === null || record.terminalReason === null) { + return null; + } + const terminal = { + ...common, + resolvedAtMs: record.resolvedAtMs, + reason: record.terminalReason, + }; + if (record.status === "allowed") { + if (record.decision !== "allow-once" && record.decision !== "allow-always") { + return null; + } + return { ...terminal, decision: record.decision } as ApprovalSnapshot; + } + if (record.status === "denied") { + return { ...terminal, decision: "deny" } as ApprovalSnapshot; + } + return terminal as ApprovalSnapshot; +} diff --git a/src/gateway/operator-approval-store.test.ts b/src/gateway/operator-approval-store.test.ts index c8647e537c75..1d42f5daaa2b 100644 --- a/src/gateway/operator-approval-store.test.ts +++ b/src/gateway/operator-approval-store.test.ts @@ -179,6 +179,74 @@ describe("operator approval store", () => { ]); }); + it("filters an audience before applying the replay limit across scan pages", () => { + const databaseOptions = createDatabaseOptions(); + for (let index = 0; index < 256; index += 1) { + const id = `unrelated-${String(index).padStart(3, "0")}`; + expect( + insertOperatorApproval({ + approval: approval(id, { + audienceSessionKeys: ["agent:main:other"], + createdAtMs: 1_000 + index, + }), + databaseOptions, + }), + ).toMatchObject({ outcome: "inserted" }); + } + expect( + insertOperatorApproval({ + approval: approval("target-after-first-page", { + audienceSessionKeys: ["agent:main:target"], + createdAtMs: 2_000, + }), + databaseOptions, + }), + ).toMatchObject({ outcome: "inserted" }); + + expect( + listPendingOperatorApprovals({ + audienceSessionKey: "agent:main:target", + limit: 1, + nowMs: 3_000, + databaseOptions, + }), + ).toMatchObject([{ id: "target-after-first-page" }]); + }); + + it("applies a record filter before the replay limit across scan pages", () => { + const databaseOptions = createDatabaseOptions(); + for (let index = 0; index < 256; index += 1) { + const id = `unrelated-reviewer-${String(index).padStart(3, "0")}`; + expect( + insertOperatorApproval({ + approval: approval(id, { + reviewerDeviceIds: ["unrelated-device"], + createdAtMs: 1_000 + index, + }), + databaseOptions, + }), + ).toMatchObject({ outcome: "inserted" }); + } + expect( + insertOperatorApproval({ + approval: approval("authorized-after-first-page", { + reviewerDeviceIds: ["authorized-device"], + createdAtMs: 2_000, + }), + databaseOptions, + }), + ).toMatchObject({ outcome: "inserted" }); + + expect( + listPendingOperatorApprovals({ + recordFilter: (record) => record.reviewerDeviceIds.includes("authorized-device"), + limit: 1, + nowMs: 3_000, + databaseOptions, + }), + ).toMatchObject([{ id: "authorized-after-first-page" }]); + }); + it("reads the default clock after waiting for the SQLite write lock", async () => { const databaseOptions = createDatabaseOptions(); const createdAtMs = Date.now(); @@ -284,6 +352,23 @@ describe("operator approval store", () => { } }); + it("preserves protocol-valid boundary whitespace as opaque approval identity", () => { + const databaseOptions = createDatabaseOptions(); + for (const [index, id] of ["\uFEFF", "\u00A0", " approval-edge "].entries()) { + const inserted = insertOperatorApproval({ + approval: approval(id, { createdAtMs: 1_000 + index }), + databaseOptions, + }); + + expect(inserted).toMatchObject({ outcome: "inserted", record: { id } }); + expect(getOperatorApproval({ id, nowMs: 2_000, databaseOptions })).toMatchObject({ + id, + status: "pending", + }); + } + expect(getOperatorApproval({ id: "approval-edge", nowMs: 2_000, databaseOptions })).toBeNull(); + }); + it("keeps canonical ids and transport references in disjoint lookup namespaces", () => { const databaseOptions = createDatabaseOptions(); const inserted = insertOperatorApproval({ diff --git a/src/gateway/operator-approval-store.ts b/src/gateway/operator-approval-store.ts index 62a9087e1119..fa7cb15b86a2 100644 --- a/src/gateway/operator-approval-store.ts +++ b/src/gateway/operator-approval-store.ts @@ -26,6 +26,8 @@ import { export const OPERATOR_APPROVAL_TERMINAL_RETENTION_MS = 30 * 24 * 60 * 60_000; export const OPERATOR_APPROVAL_MAX_AUDIENCE_SESSION_KEYS = 64; +const OPERATOR_APPROVAL_PENDING_SCAN_PAGE_SIZE = 256; +const OPERATOR_APPROVAL_MAX_LIST_LIMIT = 1_001; export type OperatorApprovalKind = "exec" | "plugin"; export type OperatorApprovalStatus = "pending" | "allowed" | "denied" | "expired" | "cancelled"; @@ -719,6 +721,8 @@ export function listPendingOperatorApprovals( params: { kind?: OperatorApprovalKind; sourceSessionKey?: string; + audienceSessionKey?: string; + recordFilter?: (record: OperatorApprovalRecord) => boolean; limit?: number; nowMs?: number; databaseOptions?: OpenClawStateDatabaseOptions; @@ -728,34 +732,73 @@ export function listPendingOperatorApprovals( return runOpenClawStateWriteTransaction((database) => { const nowMs = params.nowMs ?? Date.now(); const stateDb = getNodeSqliteKysely(database.db); - let query = stateDb - .selectFrom("operator_approvals") - .selectAll() - .where("status", "=", "pending") - .where("expires_at_ms", ">", nowMs) - .orderBy("created_at_ms", "asc") - .orderBy("approval_id", "asc") - .limit(Math.max(1, Math.min(params.limit ?? 1_000, 1_000))); - if (params.kind) { - query = query.where("kind", "=", params.kind); - } - if (params.sourceSessionKey) { - query = query.where("source_session_key", "=", params.sourceSessionKey); - } - const rows = executeSqliteQuerySync(database.db, query).rows; + const resultLimit = Math.max( + 1, + Math.min(params.limit ?? 1_000, OPERATOR_APPROVAL_MAX_LIST_LIMIT), + ); + const audienceSessionKey = + params.audienceSessionKey === undefined + ? undefined + : requireString(params.audienceSessionKey, "operator approval audience session key"); + const requiresPostFilter = + audienceSessionKey !== undefined || params.recordFilter !== undefined; const records: OperatorApprovalRecord[] = []; - for (const row of rows) { - const record = decodeOperatorApprovalRow(row); - if (record) { - records.push(record); - } else { - denyCorruptPendingRow({ - database, - id: row.approval_id, - nowMs, - createdAtMs: row.created_at_ms, - }); + let cursor: { createdAtMs: number; id: string } | undefined; + // Audience and reviewer bindings live in validated bounded JSON. Keyset-scan + // first, then apply the limit so unrelated records cannot starve replay. + while (records.length < resultLimit) { + let query = stateDb + .selectFrom("operator_approvals") + .selectAll() + .where("status", "=", "pending") + .where("expires_at_ms", ">", nowMs) + .orderBy("created_at_ms", "asc") + .orderBy("approval_id", "asc") + .limit(requiresPostFilter ? OPERATOR_APPROVAL_PENDING_SCAN_PAGE_SIZE : resultLimit); + if (params.kind) { + query = query.where("kind", "=", params.kind); } + if (params.sourceSessionKey) { + query = query.where("source_session_key", "=", params.sourceSessionKey); + } + if (cursor) { + const pageCursor = cursor; + query = query.where((eb) => + eb.or([ + eb("created_at_ms", ">", pageCursor.createdAtMs), + eb.and([ + eb("created_at_ms", "=", pageCursor.createdAtMs), + eb("approval_id", ">", pageCursor.id), + ]), + ]), + ); + } + const rows = executeSqliteQuerySync(database.db, query).rows; + for (const row of rows) { + const record = decodeOperatorApprovalRow(row); + if (!record) { + denyCorruptPendingRow({ + database, + id: row.approval_id, + nowMs, + createdAtMs: row.created_at_ms, + }); + continue; + } + const matchesAudience = + !audienceSessionKey || record.audienceSessionKeys.includes(audienceSessionKey); + if (matchesAudience && (!params.recordFilter || params.recordFilter(record))) { + records.push(record); + if (records.length === resultLimit) { + break; + } + } + } + const last = rows.at(-1); + if (!requiresPostFilter || rows.length < OPERATOR_APPROVAL_PENDING_SCAN_PAGE_SIZE || !last) { + break; + } + cursor = { createdAtMs: last.created_at_ms, id: last.approval_id }; } return records; }, params.databaseOptions); diff --git a/src/gateway/server-aux-handlers.ts b/src/gateway/server-aux-handlers.ts index 8d516cf6ffbf..eb5e7e385ae8 100644 --- a/src/gateway/server-aux-handlers.ts +++ b/src/gateway/server-aux-handlers.ts @@ -8,10 +8,8 @@ import { resolveExecApprovalRequestAllowedDecisions, type ExecApprovalRequestPayload, } from "../infra/exec-approvals.js"; -import { - resolvePluginApprovalRequestAllowedDecisions, - type PluginApprovalRequestPayload, -} from "../infra/plugin-approvals.js"; +import { resolveCanonicalPluginApprovalRequestAllowedDecisions } from "../infra/plugin-approval-canonical-decisions.js"; +import type { PluginApprovalRequestPayload } from "../infra/plugin-approvals.js"; import { resolveCommandSecretsFromActiveRuntimeSnapshot, type CommandSecretAssignment, @@ -21,7 +19,7 @@ import { type PreparedSecretsRuntimeSnapshot, } from "../secrets/runtime-state.js"; import { createLazyPromise } from "../shared/lazy-runtime.js"; -import { resolveApprovalSessionAudience } from "./approval-session-audience.js"; +import { resolveApprovalSessionAudienceWithFallback } from "./approval-session-audience.js"; import { diffConfigPaths } from "./config-diff.js"; import { buildGatewayReloadPlan, @@ -29,7 +27,10 @@ import { type GatewayReloadPlan, } from "./config-reload-plan.js"; import { createExecApprovalIosPushDelivery } from "./exec-approval-ios-push.js"; -import { ExecApprovalManager } from "./exec-approval-manager.js"; +import { + ExecApprovalManager, + type OperatorApprovalLifecycleEvent, +} from "./exec-approval-manager.js"; import { closeOrphanedOperatorApprovals, pruneTerminalOperatorApprovals, @@ -88,6 +89,7 @@ export function createGatewayAuxHandlers(params: { stopChannel: (name: ChannelKind) => Promise; getChannelAutostartSuppression?: () => ChannelAutostartSuppression | null; logChannels: { info: (msg: string) => void }; + onApprovalLifecycle?: (event: OperatorApprovalLifecycleEvent) => void; }) { // Both approval kinds share one durable first-answer-wins registry and // Gateway-lifetime epoch while retaining separate in-process waiter maps. @@ -103,8 +105,9 @@ export function createGatewayAuxHandlers(params: { const execApprovalManager = new ExecApprovalManager({ approvalKind: "exec", persistence: approvalPersistence, + resolveAudienceSessionKeys: resolveApprovalSessionAudienceWithFallback, resolveAllowedDecisions: resolveExecApprovalRequestAllowedDecisions, - resolveAudienceSessionKeys: resolveApprovalSessionAudience, + onLifecycle: params.onApprovalLifecycle, onError: (error, context) => { params.log.error?.( `${context.approvalKind} approval ${context.operation} failed for ${context.approvalId}: ${String(error)}`, @@ -127,8 +130,9 @@ export function createGatewayAuxHandlers(params: { const pluginApprovalManager = new ExecApprovalManager({ approvalKind: "plugin", persistence: approvalPersistence, - resolveAllowedDecisions: (request) => resolvePluginApprovalRequestAllowedDecisions(request), - resolveAudienceSessionKeys: resolveApprovalSessionAudience, + resolveAudienceSessionKeys: resolveApprovalSessionAudienceWithFallback, + resolveAllowedDecisions: resolveCanonicalPluginApprovalRequestAllowedDecisions, + onLifecycle: params.onApprovalLifecycle, onError: (error, context) => { params.log.error?.( `${context.approvalKind} approval ${context.operation} failed for ${context.approvalId}: ${String(error)}`, diff --git a/src/gateway/server-broadcast.ts b/src/gateway/server-broadcast.ts index ad2bd0b5bde0..8aaf8a350288 100644 --- a/src/gateway/server-broadcast.ts +++ b/src/gateway/server-broadcast.ts @@ -48,6 +48,7 @@ const EVENT_SCOPE_GUARDS: Record = { "node.pair.resolved": [PAIRING_SCOPE], "node.presence": [READ_SCOPE], "sessions.changed": [READ_SCOPE], + "session.approval": [APPROVALS_SCOPE], "session.message": [READ_SCOPE], "session.operation": [READ_SCOPE], "session.tool": [READ_SCOPE], diff --git a/src/gateway/server-chat-state.test.ts b/src/gateway/server-chat-state.test.ts new file mode 100644 index 000000000000..26510d0f02dd --- /dev/null +++ b/src/gateway/server-chat-state.test.ts @@ -0,0 +1,67 @@ +import { describe, expect, it } from "vitest"; +import { createSessionMessageSubscriberRegistry } from "./server-chat-state.js"; + +describe("createSessionMessageSubscriberRegistry", () => { + it("keeps approval delivery opt-in and updates it on resubscribe", () => { + const subscribers = createSessionMessageSubscriberRegistry(); + + subscribers.subscribe("conn-plain", "agent:main:main"); + subscribers.subscribe("conn-reviewer", "agent:main:main", { includeApprovals: true }); + + expect([...subscribers.get("agent:main:main")]).toEqual(["conn-plain", "conn-reviewer"]); + expect([...subscribers.getApprovals("agent:main:main")]).toEqual(["conn-reviewer"]); + + subscribers.subscribe("conn-reviewer", "agent:main:main"); + expect([...subscribers.get("agent:main:main")]).toEqual(["conn-plain", "conn-reviewer"]); + expect([...subscribers.getApprovals("agent:main:main")]).toEqual([]); + + subscribers.subscribe("conn-reviewer", "agent:main:main", { includeApprovals: true }); + expect([...subscribers.getApprovals("agent:main:main")]).toEqual(["conn-reviewer"]); + + subscribers.unsubscribe("conn-reviewer", "agent:main:main"); + expect([...subscribers.get("agent:main:main")]).toEqual(["conn-plain"]); + expect([...subscribers.getApprovals("agent:main:main")]).toEqual([]); + }); + + it("removes approval subscriptions through connection cleanup and registry reset", () => { + const subscribers = createSessionMessageSubscriberRegistry(); + + subscribers.subscribe("conn-reviewer", "agent:main:main", { includeApprovals: true }); + subscribers.subscribe("conn-reviewer", "agent:main:child", { includeApprovals: true }); + subscribers.subscribe("conn-other", "agent:main:child", { includeApprovals: true }); + + subscribers.unsubscribeAll("conn-reviewer"); + expect([...subscribers.get("agent:main:main")]).toEqual([]); + expect([...subscribers.getApprovals("agent:main:main")]).toEqual([]); + expect([...subscribers.get("agent:main:child")]).toEqual(["conn-other"]); + expect([...subscribers.getApprovals("agent:main:child")]).toEqual(["conn-other"]); + + subscribers.clear(); + expect([...subscribers.get("agent:main:child")]).toEqual([]); + expect([...subscribers.getApprovals("agent:main:child")]).toEqual([]); + }); + + it("rolls a provisional subscription back to its exact prior state", () => { + const subscribers = createSessionMessageSubscriberRegistry(); + + const removeNew = subscribers.subscribe("conn-new", "agent:main:main", { + includeApprovals: true, + }); + removeNew?.(); + expect([...subscribers.get("agent:main:main")]).toEqual([]); + expect([...subscribers.getApprovals("agent:main:main")]).toEqual([]); + + subscribers.subscribe("conn-plain", "agent:main:main"); + const restorePlain = subscribers.subscribe("conn-plain", "agent:main:main", { + includeApprovals: true, + }); + restorePlain?.(); + expect([...subscribers.get("agent:main:main")]).toEqual(["conn-plain"]); + expect([...subscribers.getApprovals("agent:main:main")]).toEqual([]); + + subscribers.subscribe("conn-reviewer", "agent:main:main", { includeApprovals: true }); + const restoreReviewer = subscribers.subscribe("conn-reviewer", "agent:main:main"); + restoreReviewer?.(); + expect([...subscribers.getApprovals("agent:main:main")]).toEqual(["conn-reviewer"]); + }); +}); diff --git a/src/gateway/server-chat-state.ts b/src/gateway/server-chat-state.ts index a05813e4195e..28b62a21852b 100644 --- a/src/gateway/server-chat-state.ts +++ b/src/gateway/server-chat-state.ts @@ -229,10 +229,15 @@ export type SessionEventSubscriberRegistry = { }; export type SessionMessageSubscriberRegistry = { - subscribe: (connId: string, sessionKey: string) => void; + subscribe: ( + connId: string, + sessionKey: string, + opts?: { includeApprovals?: boolean }, + ) => (() => void) | undefined; unsubscribe: (connId: string, sessionKey: string) => void; unsubscribeAll: (connId: string) => void; get: (sessionKey: string) => ReadonlySet; + getApprovals: (sessionKey: string) => ReadonlySet; clear: () => void; }; @@ -276,17 +281,23 @@ export function createSessionEventSubscriberRegistry(): SessionEventSubscriberRe export function createSessionMessageSubscriberRegistry(): SessionMessageSubscriberRegistry { const sessionToConnIds = new Map>(); const connToSessionKeys = new Map>(); + const approvalSessionToConnIds = new Map>(); + const connToApprovalSessionKeys = new Map>(); const empty = new Set(); const normalize = (value: string): string => value.trim(); - return { - subscribe: (connId: string, sessionKey: string) => { + const registry: SessionMessageSubscriberRegistry = { + subscribe: (connId: string, sessionKey: string, opts) => { const normalizedConnId = normalize(connId); const normalizedSessionKey = normalize(sessionKey); if (!normalizedConnId || !normalizedSessionKey) { - return; + return undefined; } + const hadMessages = + sessionToConnIds.get(normalizedSessionKey)?.has(normalizedConnId) ?? false; + const hadApprovals = + approvalSessionToConnIds.get(normalizedSessionKey)?.has(normalizedConnId) ?? false; const connIds = sessionToConnIds.get(normalizedSessionKey) ?? new Set(); connIds.add(normalizedConnId); sessionToConnIds.set(normalizedSessionKey, connIds); @@ -294,6 +305,42 @@ export function createSessionMessageSubscriberRegistry(): SessionMessageSubscrib const sessionKeys = connToSessionKeys.get(normalizedConnId) ?? new Set(); sessionKeys.add(normalizedSessionKey); connToSessionKeys.set(normalizedConnId, sessionKeys); + + if (opts?.includeApprovals) { + const approvalConnIds = + approvalSessionToConnIds.get(normalizedSessionKey) ?? new Set(); + approvalConnIds.add(normalizedConnId); + approvalSessionToConnIds.set(normalizedSessionKey, approvalConnIds); + + const approvalSessionKeys = + connToApprovalSessionKeys.get(normalizedConnId) ?? new Set(); + approvalSessionKeys.add(normalizedSessionKey); + connToApprovalSessionKeys.set(normalizedConnId, approvalSessionKeys); + } else { + const approvalConnIds = approvalSessionToConnIds.get(normalizedSessionKey); + approvalConnIds?.delete(normalizedConnId); + if (approvalConnIds?.size === 0) { + approvalSessionToConnIds.delete(normalizedSessionKey); + } + const approvalSessionKeys = connToApprovalSessionKeys.get(normalizedConnId); + approvalSessionKeys?.delete(normalizedSessionKey); + if (approvalSessionKeys?.size === 0) { + connToApprovalSessionKeys.delete(normalizedConnId); + } + } + // Replay setup subscribes before reading its snapshot. Preserve the exact + // prior state so a failed read cannot leave a ghost or remove a retry. + return () => { + if (!hadMessages) { + registry.unsubscribe(normalizedConnId, normalizedSessionKey); + return; + } + registry.subscribe( + normalizedConnId, + normalizedSessionKey, + hadApprovals ? { includeApprovals: true } : undefined, + ); + }; }, unsubscribe: (connId: string, sessionKey: string) => { const normalizedConnId = normalize(connId); @@ -315,6 +362,20 @@ export function createSessionMessageSubscriberRegistry(): SessionMessageSubscrib connToSessionKeys.delete(normalizedConnId); } } + const approvalConnIds = approvalSessionToConnIds.get(normalizedSessionKey); + if (approvalConnIds) { + approvalConnIds.delete(normalizedConnId); + if (approvalConnIds.size === 0) { + approvalSessionToConnIds.delete(normalizedSessionKey); + } + } + const approvalSessionKeys = connToApprovalSessionKeys.get(normalizedConnId); + if (approvalSessionKeys) { + approvalSessionKeys.delete(normalizedSessionKey); + if (approvalSessionKeys.size === 0) { + connToApprovalSessionKeys.delete(normalizedConnId); + } + } }, unsubscribeAll: (connId: string) => { const normalizedConnId = normalize(connId); @@ -336,6 +397,16 @@ export function createSessionMessageSubscriberRegistry(): SessionMessageSubscrib } } connToSessionKeys.delete(normalizedConnId); + + const approvalSessionKeys = connToApprovalSessionKeys.get(normalizedConnId); + for (const sessionKey of approvalSessionKeys ?? []) { + const connIds = approvalSessionToConnIds.get(sessionKey); + connIds?.delete(normalizedConnId); + if (connIds?.size === 0) { + approvalSessionToConnIds.delete(sessionKey); + } + } + connToApprovalSessionKeys.delete(normalizedConnId); }, get: (sessionKey: string) => { const normalizedSessionKey = normalize(sessionKey); @@ -344,11 +415,21 @@ export function createSessionMessageSubscriberRegistry(): SessionMessageSubscrib } return sessionToConnIds.get(normalizedSessionKey) ?? empty; }, + getApprovals: (sessionKey: string) => { + const normalizedSessionKey = normalize(sessionKey); + if (!normalizedSessionKey) { + return empty; + } + return approvalSessionToConnIds.get(normalizedSessionKey) ?? empty; + }, clear: () => { sessionToConnIds.clear(); connToSessionKeys.clear(); + approvalSessionToConnIds.clear(); + connToApprovalSessionKeys.clear(); }, }; + return registry; } /** Create the run-id recipient registry used for streaming tool events. */ diff --git a/src/gateway/server-methods-list.ts b/src/gateway/server-methods-list.ts index 9b5a8b3dfaa3..333129e09fd4 100644 --- a/src/gateway/server-methods-list.ts +++ b/src/gateway/server-methods-list.ts @@ -40,6 +40,7 @@ export const GATEWAY_EVENTS = [ "connect.challenge", "agent", "chat", + "session.approval", "session.message", "session.operation", "session.tool", diff --git a/src/gateway/server-methods/sessions.messages-subscribe-approvals.test.ts b/src/gateway/server-methods/sessions.messages-subscribe-approvals.test.ts new file mode 100644 index 000000000000..e5977d22ff96 --- /dev/null +++ b/src/gateway/server-methods/sessions.messages-subscribe-approvals.test.ts @@ -0,0 +1,254 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { SessionApprovalReplay } from "../../../packages/gateway-protocol/src/index.js"; +import type { + GatewayClient, + GatewayRequestContext, + GatewayRequestHandlerOptions, +} from "./types.js"; + +const loadSessionEntryMock = vi.fn((sessionKey: string, _opts?: { agentId?: string }) => ({ + canonicalKey: sessionKey, +})); + +vi.mock("../session-utils.js", async () => { + const actual = await vi.importActual("../session-utils.js"); + return { + ...actual, + loadSessionEntry: (...args: unknown[]) => + loadSessionEntryMock(...(args as [string, { agentId?: string }?])), + }; +}); + +import { sessionsHandlers } from "./sessions.js"; + +function createClient(params: { + scopes: string[]; + deviceId?: string; + connId?: string; +}): GatewayClient { + return { + connId: params.connId ?? "conn-approval-reviewer", + connect: { + client: { id: "approval-subscribe-test", displayName: "Approval Subscribe Test" }, + scopes: params.scopes, + ...(params.deviceId ? { device: { id: params.deviceId } } : {}), + }, + } as unknown as GatewayClient; +} + +function createContext(params: { + replay?: SessionApprovalReplay; + replayError?: Error; + globalScope?: boolean; + agents?: Array<{ id: string; default?: boolean }>; +}) { + const rollbackSubscription = vi.fn(); + const subscribeSessionMessageEvents = vi.fn(() => rollbackSubscription); + const listSessionPendingApprovals = vi.fn(() => { + if (params.replayError) { + throw params.replayError; + } + return params.replay; + }); + const logError = vi.fn(); + const context = { + getRuntimeConfig: () => ({ + agents: { list: params.agents ?? [{ id: "main", default: true }] }, + ...(params.globalScope ? { session: { scope: "global" as const } } : {}), + }), + listSessionPendingApprovals, + logGateway: { error: logError }, + subscribeSessionMessageEvents, + } as unknown as GatewayRequestContext; + return { + context, + listSessionPendingApprovals, + logError, + rollbackSubscription, + subscribeSessionMessageEvents, + }; +} + +async function subscribe(params: { + body: Record; + client: GatewayClient; + context: GatewayRequestContext; +}) { + const respond = vi.fn(); + await sessionsHandlers["sessions.messages.subscribe"]({ + req: { id: "req-subscribe-approvals" } as never, + params: params.body, + respond, + context: params.context, + client: params.client, + isWebchatConnect: () => false, + } satisfies GatewayRequestHandlerOptions); + return respond; +} + +describe("sessions.messages.subscribe approval opt-in", () => { + beforeEach(() => { + loadSessionEntryMock.mockReset(); + loadSessionEntryMock.mockImplementation((sessionKey: string) => ({ canonicalKey: sessionKey })); + }); + + it("allows an admin without a paired device and uses the exact scoped subscription key", async () => { + loadSessionEntryMock.mockReturnValueOnce({ canonicalKey: "global" }); + const approvalReplay = { + sessionKey: "agent:work:global", + updatedAtMs: 42, + approvals: [], + truncated: false, + } satisfies SessionApprovalReplay; + const { context, listSessionPendingApprovals, subscribeSessionMessageEvents } = createContext({ + replay: approvalReplay, + globalScope: true, + agents: [{ id: "main", default: true }, { id: "work" }], + }); + + const respond = await subscribe({ + body: { key: "agent:work:main", includeApprovals: true }, + client: createClient({ scopes: ["operator.admin"], connId: " conn-admin " }), + context, + }); + + expect(listSessionPendingApprovals).toHaveBeenCalledWith( + "agent:work:global", + expect.objectContaining({ connId: " conn-admin " }), + ); + expect(subscribeSessionMessageEvents.mock.invocationCallOrder[0]).toBeLessThan( + listSessionPendingApprovals.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY, + ); + expect(subscribeSessionMessageEvents).toHaveBeenCalledWith("conn-admin", "agent:work:global", { + includeApprovals: true, + }); + expect(respond).toHaveBeenCalledWith( + true, + { subscribed: true, key: "global", approvalReplay }, + undefined, + ); + }); + + it("allows a paired device with approval scope", async () => { + loadSessionEntryMock.mockReturnValueOnce({ canonicalKey: "agent:main:child" }); + const approvalReplay = { + sessionKey: "agent:main:child", + updatedAtMs: 43, + approvals: [], + truncated: false, + } satisfies SessionApprovalReplay; + const { context, subscribeSessionMessageEvents } = createContext({ replay: approvalReplay }); + + const respond = await subscribe({ + body: { key: "child", includeApprovals: true }, + client: createClient({ scopes: ["operator.approvals"], deviceId: "phone" }), + context, + }); + + expect(subscribeSessionMessageEvents).toHaveBeenCalledWith( + "conn-approval-reviewer", + "agent:main:child", + { includeApprovals: true }, + ); + expect(respond).toHaveBeenCalledWith( + true, + { subscribed: true, key: "agent:main:child", approvalReplay }, + undefined, + ); + }); + + it.each([ + { + name: "approval scope without a paired device", + client: createClient({ scopes: ["operator.approvals"] }), + }, + { + name: "paired device without approval authority", + client: createClient({ scopes: ["operator.read"], deviceId: "phone" }), + }, + ])("rejects $name", async ({ client }) => { + const { context, listSessionPendingApprovals, subscribeSessionMessageEvents } = createContext( + {}, + ); + + const respond = await subscribe({ + body: { key: "agent:main:child", includeApprovals: true }, + client, + context, + }); + + expect(listSessionPendingApprovals).not.toHaveBeenCalled(); + expect(subscribeSessionMessageEvents).not.toHaveBeenCalled(); + expect(respond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ + code: "INVALID_REQUEST", + message: expect.stringContaining("operator.approvals"), + }), + ); + }); + + it("keeps the non-approval response shape and skips replay", async () => { + loadSessionEntryMock.mockReturnValueOnce({ canonicalKey: "agent:main:child" }); + const { context, listSessionPendingApprovals, subscribeSessionMessageEvents } = createContext( + {}, + ); + + const respond = await subscribe({ + body: { key: "child" }, + client: createClient({ scopes: ["operator.read"] }), + context, + }); + + expect(listSessionPendingApprovals).not.toHaveBeenCalled(); + expect(subscribeSessionMessageEvents).toHaveBeenCalled(); + expect(subscribeSessionMessageEvents.mock.calls[0]?.slice(0, 2)).toEqual([ + "conn-approval-reviewer", + "agent:main:child", + ]); + expect(respond).toHaveBeenCalledWith( + true, + { subscribed: true, key: "agent:main:child" }, + undefined, + ); + expect(respond.mock.calls[0]?.[1]).not.toHaveProperty("approvalReplay"); + }); + + it.each([ + { name: "throws", replayError: new Error("database unavailable") }, + { name: "returns no snapshot", replayError: undefined }, + ])("restores the prior subscription when replay $name", async ({ replayError }) => { + const { + context, + listSessionPendingApprovals, + logError, + rollbackSubscription, + subscribeSessionMessageEvents, + } = createContext({ replayError }); + + const respond = await subscribe({ + body: { key: "agent:main:child", includeApprovals: true }, + client: createClient({ scopes: ["operator.admin"] }), + context, + }); + + expect(subscribeSessionMessageEvents).toHaveBeenCalledWith( + "conn-approval-reviewer", + "agent:main:child", + { includeApprovals: true }, + ); + expect(subscribeSessionMessageEvents.mock.invocationCallOrder[0]).toBeLessThan( + listSessionPendingApprovals.mock.invocationCallOrder[0] ?? Number.POSITIVE_INFINITY, + ); + expect(rollbackSubscription).toHaveBeenCalledTimes(1); + expect(respond).toHaveBeenCalledWith( + false, + undefined, + expect.objectContaining({ code: "UNAVAILABLE" }), + ); + if (replayError) { + expect(logError).toHaveBeenCalledWith(expect.stringContaining("database unavailable")); + } + }); +}); diff --git a/src/gateway/server-methods/sessions.ts b/src/gateway/server-methods/sessions.ts index 3ab343d24ef9..798bb6baecd1 100644 --- a/src/gateway/server-methods/sessions.ts +++ b/src/gateway/server-methods/sessions.ts @@ -96,7 +96,8 @@ import { recordSessionCompacted, } from "../../sessions/session-state-events.js"; import { createLazyRuntimeModule } from "../../shared/lazy-runtime.js"; -import { ADMIN_SCOPE } from "../operator-scopes.js"; +import { canReviewOperatorApproval } from "../operator-approval-authorization.js"; +import { ADMIN_SCOPE, APPROVALS_SCOPE } from "../operator-scopes.js"; import { resolveSessionKeyForRun } from "../server-session-key.js"; import { createFileBackedCompactionCheckpointStore, @@ -1071,6 +1072,17 @@ export const sessionsHandlers: GatewayRequestHandlers = { if (!key) { return; } + if (p.includeApprovals === true && !canReviewOperatorApproval(client)) { + respond( + false, + undefined, + errorShape( + ErrorCodes.INVALID_REQUEST, + `sessions.messages.subscribe includeApprovals requires a paired device and gateway scope: ${APPROVALS_SCOPE}`, + ), + ); + return; + } const cfg = context.getRuntimeConfig(); const requestedAgent = resolveRequestedGlobalAgentId(cfg, key, p.agentId); if (!requestedAgent.ok) { @@ -1085,8 +1097,52 @@ export const sessionsHandlers: GatewayRequestHandlers = { defaultAgentId: resolveDefaultAgentId(cfg), }); if (connId) { - context.subscribeSessionMessageEvents(connId, subscriptionKey); - respond(true, { subscribed: true, key: canonicalKey }, undefined); + let approvalReplay; + if (p.includeApprovals === true) { + // Subscribe before the authoritative snapshot so a transition cannot + // land between replay and live delivery. Clients reconcile by id. + const rollbackSubscription = context.subscribeSessionMessageEvents( + connId, + subscriptionKey, + { includeApprovals: true }, + ); + try { + approvalReplay = context.listSessionPendingApprovals?.(subscriptionKey, client); + } catch (error) { + rollbackSubscription?.(); + context.logGateway.error(`session approval replay failed: ${String(error)}`); + respond( + false, + undefined, + errorShape(ErrorCodes.UNAVAILABLE, "session approval replay unavailable"), + ); + return; + } + if (!approvalReplay) { + rollbackSubscription?.(); + respond( + false, + undefined, + errorShape(ErrorCodes.UNAVAILABLE, "session approval replay unavailable"), + ); + return; + } + } else { + context.subscribeSessionMessageEvents(connId, subscriptionKey); + } + respond( + true, + { + subscribed: true, + key: canonicalKey, + ...(p.includeApprovals === true + ? { + approvalReplay, + } + : {}), + }, + undefined, + ); return; } respond(true, { subscribed: false, key: canonicalKey }, undefined); diff --git a/src/gateway/server-methods/shared-types.ts b/src/gateway/server-methods/shared-types.ts index 8ef9f8012fc6..cc4bf720ba2a 100644 --- a/src/gateway/server-methods/shared-types.ts +++ b/src/gateway/server-methods/shared-types.ts @@ -1,3 +1,4 @@ +import type { SessionApprovalReplay } from "../../../packages/gateway-protocol/src/index.js"; // Shared server-method types define the client, context, response, and handler // contracts used by every gateway RPC method module. import type { @@ -96,6 +97,10 @@ export type GatewayRequestContext = { execApprovalManager?: ExecApprovalManager; pluginApprovalManager?: ExecApprovalManager; forwardPluginApprovalRequest?: (request: PluginApprovalRequest) => Promise; + listSessionPendingApprovals?: ( + sessionKey: string, + client: GatewayClient | null, + ) => SessionApprovalReplay; loadGatewayModelCatalog: (params?: { readOnly?: boolean }) => Promise; loadGatewayModelCatalogSnapshot: (params?: { readOnly?: boolean; @@ -157,7 +162,11 @@ export type GatewayRequestContext = { ) => ChatRunEntry | undefined; subscribeSessionEvents: (connId: string) => void; unsubscribeSessionEvents: (connId: string) => void; - subscribeSessionMessageEvents: (connId: string, sessionKey: string) => void; + subscribeSessionMessageEvents: ( + connId: string, + sessionKey: string, + opts?: { includeApprovals?: boolean }, + ) => (() => void) | undefined; unsubscribeSessionMessageEvents: (connId: string, sessionKey: string) => void; unsubscribeAllSessionEvents: (connId: string) => void; getSessionEventSubscriberConnIds: () => ReadonlySet; diff --git a/src/gateway/server-request-context.test.ts b/src/gateway/server-request-context.test.ts index 99a004289f55..9bf529361686 100644 --- a/src/gateway/server-request-context.test.ts +++ b/src/gateway/server-request-context.test.ts @@ -30,6 +30,7 @@ function makeContextParams( isTerminalEnabled: vi.fn(() => false), execApprovalManager: undefined, pluginApprovalManager: undefined, + listSessionPendingApprovals: undefined, loadGatewayModelCatalog: vi.fn(async () => []), loadGatewayModelCatalogSnapshot: vi.fn(async () => ({ entries: [], routeVariants: [] })), getHealthCache: vi.fn(() => null), diff --git a/src/gateway/server-request-context.ts b/src/gateway/server-request-context.ts index 4f0a54d175bd..0a51a22d94a1 100644 --- a/src/gateway/server-request-context.ts +++ b/src/gateway/server-request-context.ts @@ -22,6 +22,7 @@ export type GatewayRequestContextParams = { execApprovalManager: GatewayRequestContext["execApprovalManager"]; forwardPluginApprovalRequest?: GatewayRequestContext["forwardPluginApprovalRequest"]; pluginApprovalManager: GatewayRequestContext["pluginApprovalManager"]; + listSessionPendingApprovals: GatewayRequestContext["listSessionPendingApprovals"]; loadGatewayModelCatalog: GatewayRequestContext["loadGatewayModelCatalog"]; loadGatewayModelCatalogSnapshot: GatewayRequestContext["loadGatewayModelCatalogSnapshot"]; getHealthCache: GatewayRequestContext["getHealthCache"]; @@ -109,6 +110,7 @@ export function createGatewayRequestContext( execApprovalManager: params.execApprovalManager, forwardPluginApprovalRequest: params.forwardPluginApprovalRequest, pluginApprovalManager: params.pluginApprovalManager, + listSessionPendingApprovals: params.listSessionPendingApprovals, loadGatewayModelCatalog: params.loadGatewayModelCatalog, loadGatewayModelCatalogSnapshot: params.loadGatewayModelCatalogSnapshot, getHealthCache: params.getHealthCache, diff --git a/src/gateway/server.impl.ts b/src/gateway/server.impl.ts index aa4e975d752f..19ba71be4a4f 100644 --- a/src/gateway/server.impl.ts +++ b/src/gateway/server.impl.ts @@ -40,6 +40,7 @@ import { } from "../infra/diagnostics-timeline.js"; import { isTruthyEnvValue, isVitestRuntimeEnv, logAcceptedEnvOption } from "../infra/env.js"; import { ensureOpenClawCliOnPath } from "../infra/path-env.js"; +import type { PluginApprovalRequestPayload } from "../infra/plugin-approvals.js"; import { readGatewayRestartHandoffSync } from "../infra/restart-handoff.js"; import { setGatewaySigusr1RestartPolicy, setPreRestartDeferralCheck } from "../infra/restart.js"; import { enqueueSystemEvent } from "../infra/system-events.js"; @@ -73,6 +74,7 @@ import { recordRemoteNodeInfo, removeRemoteNodeInfo } from "../skills/runtime/re import { createAuthRateLimiter, type AuthRateLimiter } from "./auth-rate-limit.js"; import { resolveGatewayAuth } from "./auth.js"; import type { RestartRecoveryCandidate } from "./chat-abort.js"; +import type { ExecApprovalManager } from "./exec-approval-manager.js"; import { ADMIN_SCOPE } from "./method-scopes.js"; import { STARTUP_UNAVAILABLE_GATEWAY_METHODS, @@ -1398,6 +1400,28 @@ export async function startGatewayServer( ); Object.assign(runtimeState, runtimeServices); + const { createOperatorApprovalSessionEventRuntime } = + await import("./operator-approval-session-events.js"); + // Managers publish through this runtime, while replay routes durable + // expiry back through the owning manager to release its parked waiter once. + const approvalManagersForReplay: { + exec?: ExecApprovalManager; + plugin?: ExecApprovalManager; + } = {}; + const approvalSessionEvents = createOperatorApprovalSessionEventRuntime({ + clients, + sessionMessageSubscribers, + broadcastToConnIds, + controlUiBasePath, + reconcileTerminal: (record) => { + const manager = + record.kind === "exec" + ? approvalManagersForReplay.exec + : approvalManagersForReplay.plugin; + return manager?.reconcileDurableTerminal(record) ?? false; + }, + }); + const { execApprovalManager, forwardPluginApprovalRequest, @@ -1418,10 +1442,13 @@ export async function startGatewayServer( stopChannel, getChannelAutostartSuppression: channelManager.getAutostartSuppression, logChannels, + onApprovalLifecycle: approvalSessionEvents.publish, }), coreGatewayHandlers: coreGatewayHandlersLocal, }; }); + approvalManagersForReplay.exec = execApprovalManager; + approvalManagersForReplay.plugin = pluginApprovalManager; const attachedGatewayExtraHandlers: GatewayRequestHandlers = { ...pluginRegistry.gatewayHandlers, ...extraHandlers, @@ -1660,6 +1687,7 @@ export async function startGatewayServer( execApprovalManager, forwardPluginApprovalRequest, pluginApprovalManager, + listSessionPendingApprovals: approvalSessionEvents.replay, loadGatewayModelCatalog, loadGatewayModelCatalogSnapshot, getHealthCache, diff --git a/src/plugins/hook-before-tool-call-result.ts b/src/plugins/hook-before-tool-call-result.ts index f297641b2ec9..d244f7d68382 100644 --- a/src/plugins/hook-before-tool-call-result.ts +++ b/src/plugins/hook-before-tool-call-result.ts @@ -18,6 +18,10 @@ export type PluginHookBeforeToolCallResult = { description: string; severity?: "info" | "warning" | "critical"; timeoutMs?: number; + /** + * @deprecated Unresolved approvals always deny; retained for plugin API + * compatibility. The field will be removed after one deprecation release train. + */ timeoutBehavior?: "allow" | "deny"; /** Override timeout text and return the timeout as a blocked tool result. */ timeoutReason?: string;