diff --git a/docs/developers/qwen-serve-protocol.md b/docs/developers/qwen-serve-protocol.md index 7198a41eff3..84eaddc56c7 100644 --- a/docs/developers/qwen-serve-protocol.md +++ b/docs/developers/qwen-serve-protocol.md @@ -104,9 +104,17 @@ registry. Clients **must** gate UI off `features`, not off `mode` (per design 'mcp_guardrail_events', 'workspace_file_read', 'workspace_file_bytes', 'workspace_file_write', 'session_approval_mode_control', 'workspace_tool_toggle', - 'workspace_init', 'workspace_mcp_restart'] + 'workspace_init', 'workspace_mcp_restart', + 'auth_device_flow', 'permission_mediation'] ``` +> The conditional `require_auth` tag (PR 15) appears only when the daemon +> is started with `--require-auth`. F3's `permission_mediation` tag is +> always-on and carries `modes: ['first-responder', 'designated', +'consensus', 'local-only']` so SDK clients can introspect the +> build-supported set; the runtime-active strategy is at +> `body.policy.permission`. + `session_scope_override` is the negotiation handle for the per-request `sessionScope` field on `POST /session` (see below). Older daemons silently ignore the field, so SDK clients should pre-flight `caps.features` for this tag before sending it. `session_load` and `unstable_session_resume` advertise the explicit-restore routes (`POST /session/:id/load` and `POST /session/:id/resume`). Older daemons return `404` for these paths, so SDK clients should pre-flight `caps.features` before calling. The `unstable_` prefix on `unstable_session_resume` mirrors the underlying ACP method (`connection.unstable_resumeSession`) — the daemon's wire shape is committed for v1, but the ACP method name itself may change before ACP marks resume stable. @@ -1278,17 +1286,19 @@ data: {"v":1,"type":"client_evicted","data":{"reason":"queue_overflow","droppedA The SSE-level `id:` / `event:` lines duplicate `envelope.id` / `envelope.type` for EventSource compatibility. Raw-`fetch` consumers (the SDK's `parseSseStream`) read everything off the JSON envelope and ignore the SSE preamble lines. -| Event type | Trigger | -| --------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `session_update` | Any ACP `sessionUpdate` notification (LLM chunks, tool calls, usage) | -| `permission_request` | Agent asked for tool approval | -| `permission_resolved` | Some client voted on a permission via `POST /permission/:requestId` | -| `model_switched` | `POST /session/:id/model` succeeded | -| `model_switch_failed` | `POST /session/:id/model` rejected | -| `session_died` | Agent child crashed unexpectedly. **Terminal: SSE stream closes after this frame; the session is gone from `byId`.** Subscribers should reconnect via `POST /session` to spawn a fresh one. | -| `slow_client_warning` | Subscriber-local: queue ≥ 75% full. **Non-terminal** — the stream continues; the warning is a heads-up before eviction. Carries `{queueSize, maxQueued, lastEventId}`. Fires ONCE per overflow episode; re-arms after the queue drains below 37.5%. No `id` (synthetic). Pre-flight `caps.features.slow_client_warning`. | -| `client_evicted` | Subscriber-local: queue overflow. **Terminal: SSE stream closes after this frame** (no `id` — synthetic). Other subscribers on the same session continue. | -| `stream_error` | Daemon-side error during fan-out. **Terminal: SSE stream closes after this frame** (no `id` — synthetic). | +| Event type | Trigger | +| ------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| `session_update` | Any ACP `sessionUpdate` notification (LLM chunks, tool calls, usage) | +| `permission_request` | Agent asked for tool approval | +| `permission_resolved` | Some client voted on a permission via `POST /permission/:requestId` | +| `permission_partial_vote` | (consensus only) A vote was recorded but quorum not yet reached. Carries `{requestId, sessionId, votesReceived, votesNeeded, quorum, optionTallies}`. Pre-flight `caps.features.permission_mediation`. | +| `permission_forbidden` | A vote was rejected by the active policy (`designated` mismatch, `local-only` non-loopback, or `consensus` voter not in snapshot). Carries `{requestId, sessionId, clientId?, reason}`. Pre-flight `caps.features.permission_mediation`. | +| `model_switched` | `POST /session/:id/model` succeeded | +| `model_switch_failed` | `POST /session/:id/model` rejected | +| `session_died` | Agent child crashed unexpectedly. **Terminal: SSE stream closes after this frame; the session is gone from `byId`.** Subscribers should reconnect via `POST /session` to spawn a fresh one. | +| `slow_client_warning` | Subscriber-local: queue ≥ 75% full. **Non-terminal** — the stream continues; the warning is a heads-up before eviction. Carries `{queueSize, maxQueued, lastEventId}`. Fires ONCE per overflow episode; re-arms after the queue drains below 37.5%. No `id` (synthetic). Pre-flight `caps.features.slow_client_warning`. | +| `client_evicted` | Subscriber-local: queue overflow. **Terminal: SSE stream closes after this frame** (no `id` — synthetic). Other subscribers on the same session continue. | +| `stream_error` | Daemon-side error during fan-out. **Terminal: SSE stream closes after this frame** (no `id` — synthetic). | Reconnect semantics: @@ -1305,20 +1315,32 @@ Backpressure: ### `POST /permission/:requestId` -Cast a vote on a pending `permission_request`. **First responder wins** — once one client answers, every other client trying to answer the same id gets `404`. +Cast a vote on a pending `permission_request`. The active **mediation policy** decides who wins: + +| Policy | Behavior | +| --------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `first-responder` (default) | Any validated voter wins; later voters get `404`. Pre-F3 baseline. | +| `designated` | Only the prompt originator (`originatorClientId`) decides; non-originators get `403 permission_forbidden / designated_mismatch`. Falls back to first-responder for anonymous prompts. | +| `consensus` | N-of-M voters must agree (default `N = floor(M/2) + 1`, override via `policy.consensusQuorum`). First option to reach `N` wins. Non-resolving votes get `200` + `permission_partial_vote` SSE frames. | +| `local-only` | Only loopback voters decide; remote callers get `403 permission_forbidden / remote_not_allowed`. | + +The active policy is configured in `settings.json` under `policy.permissionStrategy` and surfaced on `/capabilities` at `body.policy.permission`. Pre-flight `caps.features.permission_mediation` (with `modes: [...]`) for the build-supported set. + +> **F3 (#4175): multi-client permission coordination.** F3 added the four policies above. Pre-F3 daemons hardcoded first-responder; the wire shape stays bit-for-bit unchanged when the configured policy is `first-responder`. New events (`permission_partial_vote`, `permission_forbidden`) are additive — old SDKs see them as `unrecognized_known_event` and gracefully ignore. -> **Stage 1 limitation — no permission timeout.** A `permission_request` +> **Permission timeout (default 5 minutes).** A `permission_request` > stays pending until: (a) some client votes here, (b) `POST /session/:id/cancel` -> fires, (c) the HTTP client driving the prompt -> disconnects (mid-prompt cancel resolves outstanding permissions as -> `cancelled`), (d) the session is killed, or (e) the daemon shuts -> down. **In a fully-headless deployment with no SSE subscriber, -> `requestPermission` blocks the agent indefinitely** — there's nothing -> to time out the wait. Stage 2 will add a configurable -> `permissionTimeoutMs`. Until then, headless callers should keep an -> SSE subscription open or wrap their prompt loop in their own timeout -> -> - `POST /session/:id/cancel` on expiry. +> fires, (c) the HTTP client driving the prompt disconnects +> (mid-prompt cancel resolves outstanding permissions as `cancelled`), +> (d) the session is killed, (e) the daemon shuts down, **or +> (f) the per-session permission timeout fires** (`DEFAULT_PERMISSION_TIMEOUT_MS`, +> 5 minutes). On timeout fire the agent's `requestPermission` resolves +> as `{outcome: 'cancelled'}`, the audit ring records a +> `permission.timeout` entry, daemon stderr emits a one-line +> breadcrumb, and the SSE bus fans out the standard +> `permission_resolved` cancelled frame so subscribers clean up. The +> timeout is configurable via `BridgeOptions.permissionResponseTimeoutMs`; +> headless callers running long-form prompts may want to extend it. Request: @@ -1338,10 +1360,13 @@ Outcomes: Response: -- `200 {}` — your vote was accepted +- `200 {}` — your vote was accepted (resolved OR recorded under consensus quorum) +- `403 { "code": "permission_forbidden", "reason": "designated_mismatch" | "remote_not_allowed", "requestId", "sessionId" }` — F3: the active policy rejected your vote - `404 { "error": "..." }` — the requestId is unknown (already resolved, never existed, or session torn down) +- `500 { "code": "cancel_sentinel_collision", ... }` — F3: the agent's `allowedOptionIds` contains the reserved sentinel `'__cancelled__'`; agent / daemon contract violation +- `501 { "code": "permission_policy_not_implemented", "policy": "" }` — F3 forward-compat: a policy literal landed in the schema but its mediator branch isn't built yet (currently unreachable; reserved for future policies) -After a successful vote, every connected client sees `permission_resolved` with the same `requestId` and the chosen `outcome`. +After a successful vote, every connected client sees `permission_resolved` with the same `requestId` and the chosen `outcome`. Under `consensus`, intermediate votes additionally fan out `permission_partial_vote` until quorum. ### Auth device-flow routes (issue #4175 PR 21) diff --git a/packages/acp-bridge/src/bridge.ts b/packages/acp-bridge/src/bridge.ts index 554106165f5..ddffff4260e 100644 --- a/packages/acp-bridge/src/bridge.ts +++ b/packages/acp-bridge/src/bridge.ts @@ -14,7 +14,6 @@ import { import type { CancelNotification, PromptRequest, - RequestPermissionResponse, SetSessionModelRequest, SetSessionModelResponse, } from '@agentclientprotocol/sdk'; @@ -47,6 +46,13 @@ import { SessionLimitExceededError, WorkspaceMismatchError, InvalidClientIdError, + // Mediator's `vote()` validates `optionId ∈ allowedOptionIds`, + // but the bridge ALSO throws `InvalidPermissionOptionError` + // pre-mediator when a wire client tries to inject the cancel + // sentinel via a `selected` outcome (wenshao review #4335 / + // 3271185588 — without this guard, a wire-supplied + // `optionId === CANCEL_VOTE_SENTINEL` would short-circuit all + // policy dispatch). InvalidPermissionOptionError, InvalidSessionMetadataError, WorkspaceInitConflictError, @@ -68,12 +74,14 @@ import type { import type { BridgeOptions } from './bridgeOptions.js'; import { defaultSpawnChannelFactory } from './spawnChannel.js'; import { writeStderrLine } from './internal/stderrLine.js'; +import { BridgeClient } from './bridgeClient.js'; import { - BridgeClient, - MAX_RESOLVED_PERMISSION_RECORDS, - type PendingPermission, - type PermissionResolutionRecord, -} from './bridgeClient.js'; + CANCEL_VOTE_SENTINEL, + createNoOpPermissionAuditPublisher, + MultiClientPermissionMediator, + type PermissionAuditPublisher, +} from './permissionMediator.js'; +import { PermissionForbiddenError } from './bridgeErrors.js'; /** * Stage 1 HTTP→ACP bridge factory + supporting helpers, lifted from @@ -505,11 +513,57 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // daemon. Cleared in the `finally` of the creator. let inFlightChannelSpawn: Promise | undefined; const byId = new Map(); - // Daemon-wide pending permission table; requestIds are UUIDs so collisions - // across sessions are infeasible in practice. - const pendingPermissions = new Map(); - const resolvedPermissions = new Map(); - const resolvedPermissionOrder: string[] = []; + // F3 Commit 3 — pending + resolved permission state lifted to + // `MultiClientPermissionMediator` (constructed below). The bridge + // keeps `entry.pendingPermissionIds: Set` on each + // SessionEntry as a fast cap-check index; the mediator is the + // single source of truth for the actual pending registry and the + // duplicate-vote LRU. + + // Validate the optional consensus quorum override defensively at + // construction. The settings layer (Commit 5) is the primary + // enforcement point, but the bridge also rejects malformed values + // here so a buggy host wiring path can't NaN-poison the mediator. + const permissionConsensusQuorum = opts.permissionConsensusQuorum; + if ( + permissionConsensusQuorum !== undefined && + (!Number.isInteger(permissionConsensusQuorum) || + permissionConsensusQuorum < 1) + ) { + throw new Error( + `BridgeOptions.permissionConsensusQuorum must be a positive integer; ` + + `got ${String(permissionConsensusQuorum)}`, + ); + } + + // Build the mediator before the BridgeClient so the agent's + // `requestPermission` callback can hand the record straight in. + // Audit publisher fallback: when the host doesn't supply one + // (cli/serve/runQwenServe.ts wraps a real `PermissionAuditRing` + // backed publisher in production), we use the canonical no-op + // fallback so the mediator can still run for embedded callers / + // tests without an audit consumer. + const permissionAudit: PermissionAuditPublisher = + opts.permissionAudit ?? createNoOpPermissionAuditPublisher(); + const permissionMediator = new MultiClientPermissionMediator( + opts.permissionPolicy ?? 'first-responder', + { + emit: (sessionId, event) => { + const sessionEntry = byId.get(sessionId); + sessionEntry?.events.publish(event); + }, + audit: permissionAudit, + ...(permissionConsensusQuorum !== undefined + ? { consensusQuorum: permissionConsensusQuorum } + : {}), + now: () => Date.now(), + votersForSession: (sessionId) => { + const sessionEntry = byId.get(sessionId); + if (!sessionEntry) return new Set(); + return new Set(sessionEntry.clientIds.keys()); + }, + }, + ); // Set by `shutdown()` so any in-flight `spawnOrAttach` that was // dispatched on an existing connection AFTER the shutdown snapshot // taken in `shutdown()` fails fast instead of creating a child the @@ -600,102 +654,26 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { return clientId; }; - const resolveAnyTrustedClientId = (clientId: string): string => { - for (const entry of byId.values()) { - if (entry.clientIds.has(clientId)) return clientId; - } - throw new InvalidClientIdError('unknown', clientId); - }; - - const registerPending = (p: PendingPermission) => { - const entry = byId.get(p.sessionId); - if (!entry) { - // The session was torn down (channel.exited, killSession, shutdown) - // between when the agent decided to ask for permission and when the - // request reached this function. There's no SessionEntry to chain - // the requestId onto and no SSE bus to publish `permission_request` - // — nobody can vote, so the permission would hang the agent's - // `requestPermission` forever. Resolve immediately as cancelled to - // unwind the agent side; matches the shutdown / killSession path. - p.resolve({ outcome: { outcome: 'cancelled' } }); - return; - } - pendingPermissions.set(p.requestId, p); - entry.pendingPermissionIds.add(p.requestId); - }; - - const rememberResolvedPermission = (record: PermissionResolutionRecord) => { - if (!resolvedPermissions.has(record.requestId)) { - resolvedPermissionOrder.push(record.requestId); - } - resolvedPermissions.set(record.requestId, record); - while (resolvedPermissionOrder.length > MAX_RESOLVED_PERMISSION_RECORDS) { - const oldest = resolvedPermissionOrder.shift(); - if (oldest !== undefined) resolvedPermissions.delete(oldest); - } - }; - - const publishPermissionAlreadyResolved = ( - record: PermissionResolutionRecord, - ) => { - const entry = byId.get(record.sessionId); - if (!entry) return; - try { - writeServeDebugLine( - `permission ${JSON.stringify(record.requestId)} ` + - `for session ${JSON.stringify(record.sessionId)} was already ` + - 'resolved; publishing duplicate-vote notification.', - ); - entry.events.publish({ - type: 'permission_already_resolved', - data: { - requestId: record.requestId, - sessionId: record.sessionId, - outcome: record.outcome, - }, - }); - } catch { - writeServeDebugLine( - `skipped duplicate-vote notification for permission ` + - `${JSON.stringify(record.requestId)} during shutdown.`, - ); - } - }; - - /** Resolve a single pending request and clean up its bookkeeping. */ - const resolvePending = ( - requestId: string, - response: RequestPermissionResponse, - originatorClientId?: string, - ): boolean => { - const pending = pendingPermissions.get(requestId); - if (!pending) return false; - pendingPermissions.delete(requestId); - const entry = byId.get(pending.sessionId); - if (entry) { - entry.pendingPermissionIds.delete(requestId); - // Fan-out a follow-up event so other clients update their UI when the - // race is decided. Best-effort — failure to publish (e.g. bus closed - // mid-shutdown) doesn't block resolution. - try { - entry.events.publish({ - type: 'permission_resolved', - data: { requestId, outcome: response.outcome }, - ...(originatorClientId ? { originatorClientId } : {}), - }); - } catch { - /* bus closed during shutdown */ - } - } - rememberResolvedPermission({ - requestId, - sessionId: pending.sessionId, - outcome: response.outcome, - }); - pending.resolve(response); - return true; - }; - + // Wenshao review #4335 / 3272493777 — `resolveAnyTrustedClientId` + // helper removed alongside its sole call site in + // `respondToPermission`. The function ranged across all live + // sessions to validate a clientId, which created a cross-session + // client-registration oracle when used in the unknown-requestId + // path. The session-scoped `resolveTrustedClientId` (defined + // above) is the correct primitive — it scopes to a single + // session and is never called on the unknown-requestId path + // after Round 7+8. + + // F3 Commit 3 — `registerPending` / `rollbackPending` / + // `rememberResolvedPermission` / `publishPermissionAlreadyResolved` + // / `resolvePending` were lifted into + // `MultiClientPermissionMediator`. The mediator owns the pending + // registry, the resolved-LRU, the per-session timer, and the + // wire-event emit fan-out. The bridge talks to it through three + // narrow surfaces: `mediator.request` (issue), `mediator.vote` + // (resolve from a route), and `mediator.forgetSession` (cleanup + // on session teardown). + // /** * Get-or-create the daemon's single `qwen --acp` channel (#3803 §02). * N sessions multiplex onto it via `connection.newSession()`. @@ -738,11 +716,7 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { }, (sessionId) => sessionId ? pendingRestoreEvents.get(sessionId) : undefined, - registerPending, - (rid) => - // Roll back a register-then-publish-failed pending so the agent - // doesn't hang waiting on a vote nobody can see. - resolvePending(rid, { outcome: { outcome: 'cancelled' } }), + permissionMediator, permissionTimeoutMs, maxPendingPerSession, // #4175 PR F1 step 5: forward the optional `BridgeFileSystem` @@ -1148,13 +1122,11 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { * agent forward anyway. */ const cancelPendingForSession = (sessionId: string) => { - const entry = byId.get(sessionId); - if (!entry) return; - // Snapshot ids — resolvePending mutates the underlying set. - const ids = Array.from(entry.pendingPermissionIds); - for (const id of ids) { - resolvePending(id, { outcome: { outcome: 'cancelled' } }); - } + // F3 N5 invariant — mediator first (it cancels each pending, + // emits `permission_resolved`, writes audit, settles the + // Promise), THEN clear the bridge's fast cap-check index. + permissionMediator.forgetSession(sessionId); + byId.get(sessionId)?.pendingPermissionIds.clear(); }; /** @@ -1679,7 +1651,11 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { }, get pendingPermissionCount() { - return pendingPermissions.size; + return permissionMediator.pendingCount; + }, + + get permissionPolicy() { + return permissionMediator.policy; }, async loadSession(req) { @@ -2021,83 +1997,165 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { }, respondToPermission(requestId, response, context) { - const pending = pendingPermissions.get(requestId); - let originatorClientId: string | undefined; - if (context?.clientId !== undefined && !pending) { - resolveAnyTrustedClientId(context.clientId); - } else if (pending && context?.clientId !== undefined) { - const entry = byId.get(pending.sessionId); - if (entry) { - originatorClientId = resolveTrustedClientId(entry, context.clientId); - } else { - resolveAnyTrustedClientId(context.clientId); - } - } - if (!pending) { - const record = resolvedPermissions.get(requestId); - if (record) { - publishPermissionAlreadyResolved(record); - } + // F3 Commit 3 — legacy workspace-level vote route. Look up the + // session via mediator's resolved+pending peek, forward to + // session-scoped handler if both ids agree. + const sessionId = permissionMediator.peekSessionFor(requestId); + // I4 (Commit 3 review) — also check `byId.has(sessionId)`. The + // mediator's resolved LRU survives session teardown by design; + // `respondToSessionPermission` would throw `SessionNotFoundError` + // once `byId.delete(sessionId)` ran. Pre-F3 returned a clean + // false → 404 for this case; preserve. + if (sessionId === undefined || !byId.has(sessionId)) { + // Wenshao review #4335 / 3272493777 — the PR #4231 security + // boundary that previously called `resolveAnyTrustedClientId` + // here was inverted: it returned 400 for unregistered + // clientIds and 404 for registered ones, creating a + // cross-session client-registration oracle. A remote prober + // posting `POST /permission/` with various + // X-Qwen-Client-Id headers could distinguish "this clientId + // is registered in some active session" (404) from "not + // registered anywhere" (400). The session-scoped route at + // `respondToSessionPermission` already short-circuits to + // `false` BEFORE clientId validation when the requestId is + // unknown (Round 7 / 3271978329); applying the same + // posture here closes the oracle on the legacy route. + // + // Wenshao review #4335 / 3273077256 — symmetric observability: + // the session-scoped sibling writes an unconditional stderr + // breadcrumb on its analogous unknown-requestId rejection + // (Round 8 / 3272493792). Match that posture here so an + // operator tailing daemon stderr sees both routes' 404s + // without needing QWEN_SERVE_DEBUG=1. + writeStderrLine( + `qwen serve: legacy permission vote ${JSON.stringify(requestId)} ` + + `has no live session (peek returned ${JSON.stringify(sessionId)}); ` + + `returning 404.`, + ); return false; } - // BkwQI: validate the voter's optionId against the original - // options the agent advertised. The route already enforces - // "non-empty string" structurally; this layer enforces - // semantic membership in the agent-published set so a - // malicious client can't forge hidden outcomes (e.g. - // `ProceedAlways*` when the prompt's `hideAlwaysAllow` - // policy intentionally suppressed them). - if (response.outcome.outcome === 'selected') { - if (!pending.allowedOptionIds.has(response.outcome.optionId)) { - throw new InvalidPermissionOptionError( - requestId, - response.outcome.optionId, - ); - } - } - return resolvePending(requestId, response, originatorClientId); + return this.respondToSessionPermission( + sessionId, + requestId, + response, + context, + ); }, respondToSessionPermission(sessionId, requestId, response, context) { const entry = byId.get(sessionId); if (!entry) throw new SessionNotFoundError(sessionId); - const pending = pendingPermissions.get(requestId); - if (!pending) { - const record = resolvedPermissions.get(requestId); - if (record?.sessionId === sessionId) { - resolveTrustedClientId(entry, context?.clientId); - publishPermissionAlreadyResolved(record); - } else if (record) { - writeServeDebugLine( - `rejected permission vote ${JSON.stringify(requestId)} ` + - `for session ${JSON.stringify(sessionId)}; request belongs to ` + - `session ${JSON.stringify(record.sessionId)}.`, - ); - } - return false; - } - if (pending.sessionId !== sessionId) { + // C1 (Commit 3 review) — preserve pre-F3 cross-session reject + // semantics: a vote whose requestId belongs to a DIFFERENT + // session must return false (→ 404) WITHOUT validating + // `context.clientId` against this session's registry. + const actualSessionId = permissionMediator.peekSessionFor(requestId); + if (actualSessionId !== undefined && actualSessionId !== sessionId) { writeServeDebugLine( `rejected permission vote ${JSON.stringify(requestId)} ` + `for session ${JSON.stringify(sessionId)}; request belongs to ` + - `session ${JSON.stringify(pending.sessionId)}.`, + `session ${JSON.stringify(actualSessionId)}.`, ); return false; } - const originatorClientId = resolveTrustedClientId( - entry, - context?.clientId, - ); + // Wenshao review #4335 / 3271978329 (Critical) — error precedence: + // when `peekSessionFor` returns `undefined` (timed out / LRU- + // evicted / never registered), pre-F3 returned `false` (→ 404) + // BEFORE any clientId validation. Without this explicit guard, + // execution falls through to `resolveTrustedClientId` which + // throws `InvalidClientIdError` (→ 400) when the caller's + // `X-Qwen-Client-Id` isn't registered, leaking session-exists + // information: a probe with a fabricated clientId could + // distinguish "session exists with these clients" (400) from + // "no such request" (404). Match pre-F3 by short-circuiting + // here. + // + // Wenshao review #4335 / 3272493792 — observability: the + // forbidden-vote paths in the mediator write unconditional + // stderr breadcrumbs (`writeForbiddenStderr`), but this + // security-sensitive guard previously logged only via the + // debug-gated `writeServeDebugLine`. Promote to an + // unconditional `writeStderrLine` so operators tailing + // stderr at 3 AM can correlate unexpected 404s without + // having to reboot with `QWEN_SERVE_DEBUG=1`. + if (actualSessionId === undefined) { + writeStderrLine( + `qwen serve: rejected permission vote ${JSON.stringify(requestId)} ` + + `for session ${JSON.stringify(sessionId)}; mediator has no ` + + `pending or resolved record (unknown / timed out / LRU-evicted).`, + ); + return false; + } + // requestId matches THIS session — only now validate clientId. + // `resolveTrustedClientId` throws `InvalidClientIdError` + // (mapped to 400 by the route) when the supplied id isn't in + // `entry.clientIds`. + const trustedClientId = resolveTrustedClientId(entry, context?.clientId); + // F3 voter cancel sentinel: when the ACP body is + // `{outcome: 'cancelled'}`, the wire frame doesn't carry an + // `optionId`. Map it to the mediator-internal sentinel so + // the mediator can resolve the pending as cancelled + // regardless of the active policy. + // + // Wenshao review #4335 / 3271185588 (Critical) — the mediator + // recognizes `CANCEL_VOTE_SENTINEL` BEFORE validating the + // option against `allowedOptionIds`, so a wire client sending + // `{outcome: 'selected', optionId: '__cancelled__'}` would + // short-circuit all policy dispatch (designated originator + // check / consensus quorum / local-only loopback gate). The + // mediator's JSDoc warns that callers MUST NOT forward this + // case from the wire; enforce the precondition here. The + // collision-defense at request issue time + // (`CancelSentinelCollisionError`) already prevents agents + // from advertising the sentinel as an option, so this guard + // closes the only remaining vector. if ( response.outcome.outcome === 'selected' && - !pending.allowedOptionIds.has(response.outcome.optionId) + response.outcome.optionId === CANCEL_VOTE_SENTINEL ) { - throw new InvalidPermissionOptionError( - requestId, - response.outcome.optionId, - ); + throw new InvalidPermissionOptionError(requestId, CANCEL_VOTE_SENTINEL); + } + const optionId = + response.outcome.outcome === 'selected' + ? response.outcome.optionId + : CANCEL_VOTE_SENTINEL; + const outcome = permissionMediator.vote({ + requestId, + sessionId, + clientId: trustedClientId, + optionId, + receivedAtMs: Date.now(), + fromLoopback: context?.fromLoopback ?? false, + }); + switch (outcome.kind) { + case 'resolved': + return true; + case 'recorded': + // Consensus-policy intermediate vote. + return true; + case 'already_resolved': + // Mediator already emitted `permission_already_resolved`. + return false; + case 'unknown_request': + writeServeDebugLine( + `rejected permission vote ${JSON.stringify(requestId)} ` + + `for session ${JSON.stringify(sessionId)}; mediator has no ` + + `pending or resolved record.`, + ); + return false; + case 'forbidden': + throw new PermissionForbiddenError( + requestId, + sessionId, + outcome.reason, + ); + default: { + const _exhaustive: never = outcome; + throw new Error( + `unreachable PermissionVoteOutcome: ${JSON.stringify(_exhaustive)}`, + ); + } } - return resolvePending(requestId, response, originatorClientId); }, async closeSession(sessionId, context) { @@ -2157,9 +2215,12 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { if (ci && ci.channel === entry.channel) { ci.sessionIds.delete(sessionId); } - for (const id of Array.from(entry.pendingPermissionIds)) { - resolvePending(id, { outcome: { outcome: 'cancelled' } }); - } + // F3 Commit 3 — mediator-driven cancel cascade replaces the + // pre-F3 per-id resolvePending loop. Same effect (each pending + // settles as cancelled, SSE permission_resolved emits, audit + // records); state lives in the mediator now. + permissionMediator.forgetSession(sessionId); + entry.pendingPermissionIds.clear(); byId.delete(sessionId); // PR 14b fix (codex round 5): tombstone the closed sessionId // so any late `extNotification` from the (now-defunct) child @@ -3230,11 +3291,9 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // subsequent load/resume of the same persisted id. See the // matching guard in BridgeClient.bufferEarlyEvent. ci?.client.markSessionClosed(sessionId); - // Resolve any still-pending permission as cancelled (matches the - // shutdown path) so callers awaiting requestPermission unwind. - for (const id of Array.from(entry.pendingPermissionIds)) { - resolvePending(id, { outcome: { outcome: 'cancelled' } }); - } + // F3 Commit 3 — mediator-driven cancel cascade. + permissionMediator.forgetSession(sessionId); + entry.pendingPermissionIds.clear(); // Publish `session_died` BEFORE closing the bus. After the eager // `byId.delete` above, the channel.exited handler's // `byId.get(...)` returns undefined so the automatic publish @@ -3364,17 +3423,17 @@ export function createHttpAcpBridge(opts: BridgeOptions): HttpAcpBridge { // state and not attach). const channels = Array.from(aliveChannels); for (const ci of channels) ci.isDying = true; - // Resolve every still-pending permission as cancelled before clearing - // the maps so callers awaiting `requestPermission` unwind cleanly. + // F3 Commit 3 — drain mediator pending state before clearing + // byId so awaiting `requestPermission` callers unwind. Each + // `forgetSession` settles all matching pending as + // session_closed; the bridge's per-entry index gets cleared + // alongside. for (const e of entries) { - const ids = Array.from(e.pendingPermissionIds); - for (const id of ids) { - resolvePending(id, { outcome: { outcome: 'cancelled' } }); - } + permissionMediator.forgetSession(e.sessionId); + e.pendingPermissionIds.clear(); } defaultEntry = undefined; byId.clear(); - pendingPermissions.clear(); // Publish a terminal `session_died` BEFORE closing each bus so SSE // subscribers can distinguish "daemon shut down" from a transient // network error and don't sit indefinitely retrying. The diff --git a/packages/acp-bridge/src/bridgeClient.test.ts b/packages/acp-bridge/src/bridgeClient.test.ts index 6d0c8f56905..e139d95067f 100644 --- a/packages/acp-bridge/src/bridgeClient.test.ts +++ b/packages/acp-bridge/src/bridgeClient.test.ts @@ -40,23 +40,34 @@ import type { } from '@agentclientprotocol/sdk'; import { BridgeClient } from './bridgeClient.js'; import type { BridgeFileSystem } from './bridgeFileSystem.js'; +import { CancelSentinelCollisionError } from './bridgeErrors.js'; +import { CANCEL_VOTE_SENTINEL } from './permissionMediator.js'; /** * Minimal-stub constructor for a `BridgeClient` whose only purpose is - * to exercise `writeTextFile` / `readTextFile`. The 6 callback args + * to exercise `writeTextFile` / `readTextFile`. The 5 callback args * before `fileSystem` are filled with thrower-defaults so any test * that accidentally hits the permission path (instead of the fs path) - * fails loudly instead of silently. + * fails loudly instead of silently. F3 Commit 3 replaced the pre-F3 + * `registerPending` + `rollbackPending` callbacks with a single + * `MultiClientPermissionMediator` reference; the test stub provides + * a thrower-Mediator that fails any unexpected `request()` / + * `vote()` / `forgetSession()` call. */ function makeClient(fileSystem?: BridgeFileSystem): BridgeClient { const noPermissionFlow = () => { throw new Error('test: permission flow should not run in fs-path tests'); }; + // Wenshao review #4335 / 3272581569 — `BridgeClient.mediator` is + // narrowed to `Pick`, so the + // thrower stub only needs to provide `request`. Eliminates the + // 5 unused-method placeholders the pre-narrowing version + // required (policy/vote/forgetSession/peekSessionFor/pendingCount). + const throwerMediator = { request: noPermissionFlow } as never; return new BridgeClient( noPermissionFlow as never, // resolveEntry noPermissionFlow as never, // resolvePendingRestoreEvents - noPermissionFlow, // registerPending - noPermissionFlow, // rollbackPending + throwerMediator, // mediator (F3 Commit 3) 0, // permissionTimeoutMs (disabled) Infinity, // maxPendingPerSession (disabled) fileSystem, @@ -186,3 +197,64 @@ describe('BridgeClient — BridgeFileSystem injection seam (F1 step 5)', () => { }); }); }); + +/** + * Wenshao review #4335 / 3271978365 — `requestPermission`'s pre-publish + * `CancelSentinelCollisionError` guard prevents an orphan SSE + * `permission_request` event from being emitted when an agent's + * `allowedOptionIds` legitimately contains '__cancelled__'. The + * mediator-level test (`permissionMediator.test.ts:330`) covers the + * issue-time collision detection inside `mediator.request`, but + * BridgeClient layers a separate pre-publish check whose distinct + * purpose — preventing orphan SSE frames — needs its own test. + */ +describe('BridgeClient — requestPermission pre-publish collision guard', () => { + it('throws CancelSentinelCollisionError BEFORE publishing on the events bus', async () => { + // Arrange: a fake session entry whose `events.publish` is a spy. + // If the collision check ran AFTER publish, this would record a + // call and the assertion below would fail. + const publish = vi.fn().mockReturnValue(true); + const fakeEntry = { + sessionId: 'sess:test', + pendingPermissionIds: new Set(), + events: { publish }, + activePromptOriginatorClientId: undefined, + }; + + const noPermissionFlow = () => { + throw new Error('test: not reachable on collision-throw path'); + }; + // Wenshao review #4335 / 3272581569 — narrowed mediator type + // means the stub only needs `request`. + const throwerMediator = { request: noPermissionFlow } as never; + const client = new BridgeClient( + ((sid: string) => (sid === 'sess:test' ? fakeEntry : undefined)) as never, + noPermissionFlow as never, + throwerMediator, + 0, + Infinity, + ); + + // Act + Assert: a sentinel-colliding option causes the bridge + // client to throw before reaching publish. + await expect( + client.requestPermission({ + sessionId: 'sess:test', + toolCall: { toolCallId: 'tc-1', title: 'rm -rf /' }, + options: [ + { optionId: 'allow', name: 'Allow', kind: 'allow_once' }, + { + optionId: CANCEL_VOTE_SENTINEL, + name: 'Adversarial label', + kind: 'allow_once', + }, + ], + }), + ).rejects.toThrow(CancelSentinelCollisionError); + + // The crucial post-condition: no SSE frame went out. + expect(publish).not.toHaveBeenCalled(); + // And the cap-index was never touched (only added AFTER publish). + expect(fakeEntry.pendingPermissionIds.size).toBe(0); + }); +}); diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index 65ee406e611..4b8b911afbb 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -19,15 +19,50 @@ import type { } from '@agentclientprotocol/sdk'; import type { BridgeEvent, EventBus } from './eventBus.js'; import type { BridgeFileSystem } from './bridgeFileSystem.js'; +import { CANCEL_VOTE_SENTINEL } from './permissionMediator.js'; +// Wenshao review #4335 / 3272581569 — narrowed from the concrete +// `MultiClientPermissionMediator` to the sub-interface this class +// actually uses (`request` only). The bridge factory still +// constructs the full `MultiClientPermissionMediator` (it needs +// `peekSessionFor` / `pendingCount` / `forgetSession`); structural +// typing lets us pass the same instance here without a cast. +// Test stubs no longer have to fake all 6 mediator members. +import type { PermissionMediator } from './permission.js'; +import type { + PermissionRequestRecord, + PermissionResolution, +} from './permission.js'; +import { CancelSentinelCollisionError } from './bridgeErrors.js'; import { writeStderrLine } from './internal/stderrLine.js'; /** - * Bounded duplicate-vote cache. Stores only requestId/sessionId/outcome, so - * 512 records stays small while covering normal UI reconnect/race windows. - * Exported because `createHttpAcpBridge` factory's `resolvedPermissions` - * map uses this cap. + * #4175 F3 Commit 3 — translate the mediator's internal + * `PermissionResolution` to the ACP-shaped `RequestPermissionResponse` + * the agent expects. Voter-cancel (mediator returns + * `{kind:'cancelled', reason:'agent_cancelled'}` from the sentinel + * path) and timeout / session-closed all project to the same + * `{outcome: 'cancelled'}` shape — the ACP wire frame doesn't + * distinguish them. The audit log carries `decisionReason.type` + * for forensic discrimination. */ -export const MAX_RESOLVED_PERMISSION_RECORDS = 512; +function resolutionToAcpResponse( + resolution: PermissionResolution, +): RequestPermissionResponse { + if (resolution.kind === 'option') { + return { outcome: { outcome: 'selected', optionId: resolution.optionId } }; + } + return { outcome: { outcome: 'cancelled' } }; +} + +// Wenshao review #4335 / 3272581548 — `MAX_RESOLVED_PERMISSION_RECORDS`, +// `PendingPermission`, and `PermissionResolutionRecord` were removed +// from this file. The mediator now owns all pending+resolved state +// (`permissionMediator.ts:77` declares its own MAX constant; line 319 +// declares its own differently-shaped `PermissionResolutionRecord`), +// so the pre-F3 inline definitions here had become dead code with +// stale JSDoc that referenced deleted closures (`registerPending`, +// `resolvedPermissions` map). httpAcpBridge.ts re-exports were +// dropped in the same commit. /** * PR 14b fix #1 (codex review round 1): bounded buffering for ACP @@ -101,38 +136,6 @@ function sliceLineRange( return content.slice(offset, end > offset ? end - 1 : end); } -/** - * Pending permission request awaiting a client vote. Exported because - * the factory's `registerPending` closure passed into `BridgeClient` - * constructs these. - */ -export interface PendingPermission { - requestId: string; - sessionId: string; - resolve: (resp: RequestPermissionResponse) => void; - /** - * BkwQI: the option IDs the agent originally offered to clients in - * the `permission_request` event. `respondToPermission` validates - * the voter's `optionId` against this set so an authenticated - * client can't smuggle in a hidden outcome (e.g. - * `ProceedAlwaysProject` when the prompt's - * `hideAlwaysAllow` / forced-ask policy intentionally omitted it). - * Stored as a Set for O(1) membership check. - */ - allowedOptionIds: ReadonlySet; -} - -/** - * Record of an already-resolved permission vote. Used by the factory's - * bounded duplicate-vote cache so a re-vote on the same requestId - * within the cache window returns the prior outcome instead of 404. - */ -export interface PermissionResolutionRecord { - requestId: string; - sessionId: string; - outcome: RequestPermissionResponse['outcome']; -} - /** * Minimal session-entry shape `BridgeClient` reads via its * `resolveEntry` callback. Defined here (rather than importing the @@ -201,25 +204,29 @@ export class BridgeClient implements Client { private readonly resolvePendingRestoreEvents: ( sessionId?: string, ) => EventBus | undefined, - private readonly registerPending: (pending: PendingPermission) => void, /** - * Roll back a `registerPending` call when the subsequent publish - * fails (closed bus). Resolves the pending promise as cancelled - * and removes it from the daemon-wide maps so a late - * `respondToPermission` for this id returns 404 cleanly. + * #4175 F3 Commit 3 — the multi-client permission coordinator. + * Owns ALL pending + resolved permission state; this client just + * plumbs `requestPermission` into `mediator.request` and forwards + * the resolution to the agent. Strategy dispatch and audit/emit + * fan-out live inside the mediator. Replaces the pre-F3 + * `registerPending` / `rollbackPending` callbacks. */ - private readonly rollbackPending: (requestId: string) => void, + private readonly mediator: Pick, /** * Bd1yh: wall-clock ms before `requestPermission` resolves as * cancelled if no client vote arrives. 0 = disabled. Prevents * the per-session FIFO `promptQueue` from poisoning forever - * when no SSE subscriber is connected. + * when no SSE subscriber is connected. Forwarded directly to + * `mediator.request`; the mediator owns the timer. */ private readonly permissionTimeoutMs: number, /** * Bd1z5: per-session cap on in-flight permissions. New requests * past this cap resolve as cancelled with a stderr warning. - * Infinity = disabled. + * Infinity = disabled. The bridge keeps `entry.pendingPermissionIds` + * as a fast cap-check index; the mediator is still the source of + * truth for the pending registry. */ private readonly maxPendingPerSession: number, /** @@ -236,24 +243,13 @@ export class BridgeClient implements Client { private readonly fileSystem?: BridgeFileSystem, ) {} - // FIXME(stage-1.5, chiga0 finding 3): - // The first-responder permission flow here is a third permission - // model in the codebase (alongside ACP `requestPermission` direct - // and stream-json `ControlDispatcher`). Stage 1.5 should lift - // "permission request lifecycle" into a `PermissionMediator` - // interface with strategy-pluggable policies (`first-responder` | - // `designated` | `consensus` | `local-only`) so all four - // agent-exposing surfaces share one lifecycle. This is also the - // closure point for the prior chiga0 audit Risk 2 (first-responder - // lacks an authorization model). Reference: - // https://github.com/QwenLM/qwen-code/pull/3889#issuecomment-4427773706 async requestPermission( params: RequestPermissionRequest, ): Promise { const entry = this.resolveEntry(params.sessionId); if (!entry) return { outcome: { outcome: 'cancelled' } }; - // Bd1z5: per-session cap. Reject before registering so we never + // Bd1z5: per-session cap. Reject before issuing so we never // grow `pendingPermissionIds` past the limit. if (entry.pendingPermissionIds.size >= this.maxPendingPerSession) { writeStderrLine( @@ -264,87 +260,70 @@ export class BridgeClient implements Client { return { outcome: { outcome: 'cancelled' } }; } + // BkwQI: snapshot the option-id set the agent is offering for + // this prompt. The mediator validates the voter's `optionId` + // against this set so a malicious client can't forge an option + // (e.g. `ProceedAlways*`) the agent intentionally hid. + const allowedOptionIds = new Set( + params.options.map((o: { optionId?: unknown }) => + String(o.optionId ?? ''), + ), + ); + allowedOptionIds.delete(''); + + // F3 final-pass review fold-in — pre-flight the cancel-vote + // sentinel collision BEFORE publishing the `permission_request` + // SSE event. The mediator also checks defensively at issue + // time, but if we publish first and the mediator throws, SSE + // subscribers see an orphan event with no resolution. const requestId = randomUUID(); - return await new Promise((resolve) => { - let settled = false; - let timer: NodeJS.Timeout | undefined; - const settleOnce = (response: RequestPermissionResponse) => { - if (settled) return; - settled = true; - if (timer) clearTimeout(timer); - resolve(response); - }; + if (allowedOptionIds.has(CANCEL_VOTE_SENTINEL)) { + throw new CancelSentinelCollisionError(requestId, CANCEL_VOTE_SENTINEL); + } - // BkwQI: snapshot the option-id set the agent is offering for - // this prompt. `respondToPermission` checks the voter's - // `optionId` against this set so a malicious client can't - // forge an option (e.g. `ProceedAlways*`) the agent - // intentionally hid. - const allowedOptionIds = new Set( - params.options.map((o: { optionId?: unknown }) => - String(o.optionId ?? ''), - ), - ); - allowedOptionIds.delete(''); - this.registerPending({ + // Publish AFTER the collision check so a violating agent never + // leaves an orphan `permission_request` on the SSE bus. If the + // bus is closed (shutdown race), bail before touching the + // mediator. The mediator's N1 invariant (synchronous register + // inside the Promise executor) protects against the + // forgetSession-races-with-issue case ONLY when register runs; + // refusing to enter the mediator on a publish-failure is the + // symmetric defense for the publish-failure case. + const published = entry.events.publish({ + type: 'permission_request', + data: { requestId, sessionId: entry.sessionId, - resolve: settleOnce, - allowedOptionIds, - }); - // `publish()` returns `undefined` on a closed bus — the - // shutdown path closes per-session buses BEFORE awaiting - // `channel.kill()`, leaving a small window where the agent - // can still issue `requestPermission`. If we registered the - // pending entry above but the publish fails, no SSE - // subscriber will ever see the request → no client can vote - // → the pending promise never resolves → agent's - // `requestPermission` hangs forever (a real bug, not a - // theoretical one — the daemon's shutdown.kill() loop awaits - // each child, and a child stuck waiting on permission would - // pin shutdown until the kill timer expires). - // - // Resolve as `cancelled` immediately if the bus rejected - // the publish. Mirrors the orphan-permission handling in - // `registerPending` itself for the entry-already-gone case. - const published = entry.events.publish({ - type: 'permission_request', - data: { - requestId, - sessionId: entry.sessionId, - toolCall: params.toolCall, - options: params.options, - }, - ...(entry.activePromptOriginatorClientId - ? { originatorClientId: entry.activePromptOriginatorClientId } - : {}), - }); - if (!published) { - // Roll back the pending registration and resolve cancelled. - this.rollbackPending(requestId); - return; - } - - // Bd1yh: arm the deadline AFTER publish so we don't fire-and- - // cancel a no-subscriber request before the bus even saw it. - // When the deadline fires, roll back the pending (so a late - // vote returns 404) and resolve as cancelled (unwinding the - // agent's awaiting promise so the per-session FIFO can drain). - if (this.permissionTimeoutMs > 0) { - timer = setTimeout(() => { - if (settled) return; - writeStderrLine( - `qwen serve: session ${entry.sessionId} permission ` + - `${requestId} timed out after ${this.permissionTimeoutMs}ms ` + - `(no client voted) — resolving as cancelled.`, - ); - this.rollbackPending(requestId); - }, this.permissionTimeoutMs); - if (typeof timer === 'object' && timer && 'unref' in timer) { - (timer as { unref: () => void }).unref(); - } - } + toolCall: params.toolCall, + options: params.options, + }, + ...(entry.activePromptOriginatorClientId + ? { originatorClientId: entry.activePromptOriginatorClientId } + : {}), }); + if (!published) return { outcome: { outcome: 'cancelled' } }; + + // Cap-index add happens AFTER publish-success so a publish-fail + // path doesn't need to roll back. The mediator's + // `forgetSession` is the only thing that drains this index (via + // the bridge's `cancelPendingForSession`). + entry.pendingPermissionIds.add(requestId); + try { + const record: PermissionRequestRecord = { + requestId, + sessionId: entry.sessionId, + originatorClientId: entry.activePromptOriginatorClientId, + allowedOptionIds, + issuedAtMs: Date.now(), + }; + const resolution = await this.mediator.request( + record, + this.permissionTimeoutMs, + ); + return resolutionToAcpResponse(resolution); + } finally { + entry.pendingPermissionIds.delete(requestId); + } } async sessionUpdate(params: SessionNotification): Promise { diff --git a/packages/acp-bridge/src/bridgeErrors.ts b/packages/acp-bridge/src/bridgeErrors.ts index ccbf2ee06c4..616ae3309df 100644 --- a/packages/acp-bridge/src/bridgeErrors.ts +++ b/packages/acp-bridge/src/bridgeErrors.ts @@ -171,6 +171,97 @@ export class InvalidSessionMetadataError extends Error { } } +/** + * #4175 F3. Thrown by `MultiClientPermissionMediator.vote` when the + * active policy is wired into the schema/registry but the mediator + * implementation has not been built yet. + * + * **Currently unreachable in production** — F3 Commit 4 implemented + * all 4 policies in the frozen `PermissionPolicy` union. The class + + * route-level 501 mapping in `server.ts:sendPermissionVoteError` are + * RETAINED as forward-compat infrastructure: when a future PR adds a + * 5th policy literal to `PermissionPolicy` and lands its mediator + * implementation across multiple commits, the intermediate-build + * stub can throw this typed error and the operator gets a clean 501 + * instead of a generic 500. + * + * Routes map this to HTTP 501 with a structured body so SDK clients + * can render "your daemon is older than your settings expect; + * upgrade". + */ +export class PermissionPolicyNotImplementedError extends Error { + readonly policy: string; + constructor(policy: string) { + super( + `Permission policy "${policy}" is declared in the contract but ` + + 'not yet implemented in this daemon build.', + ); + this.name = 'PermissionPolicyNotImplementedError'; + this.policy = policy; + } +} + +/** + * #4175 F3 Commit 1. Thrown by `MultiClientPermissionMediator.request` + * when an agent-declared `allowedOptionIds` set contains the + * cancel-vote sentinel string. The bridge maps voter cancel intent + * to that exact `optionId`; if the agent legitimately uses it as + * an option label, the mediator can no longer disambiguate. We + * fail loudly at request issue time so the operator sees a clear + * misconfiguration rather than the silent "voter approval was + * treated as cancel" semantic flip. + * + * Routes map this to HTTP 500 — it represents a contract violation + * between agent and daemon, not a client mistake. + */ +export class CancelSentinelCollisionError extends Error { + readonly requestId: string; + readonly sentinel: string; + constructor(requestId: string, sentinel: string) { + super( + `Permission ${requestId}: agent-declared optionId set contains ` + + `the cancel-vote sentinel "${sentinel}", which would prevent ` + + 'the daemon from disambiguating cancel intent from a real vote.', + ); + this.name = 'CancelSentinelCollisionError'; + this.requestId = requestId; + this.sentinel = sentinel; + } +} + +/** + * #4175 F3 Commit 2. Thrown by `bridge.respondToSessionPermission` / + * `bridge.respondToPermission` when the active permission policy + * rejects the vote (designated voter mismatch, or remote vote under + * `local-only`). The bridge converts the mediator's + * `PermissionVoteOutcome { kind: 'forbidden', reason: ... }` into + * this typed error so the route layer can map to HTTP 403 without + * pattern-matching on the error message. + * + * `reason` is forwarded verbatim from the mediator's outcome so SDK + * clients can render a precise UI ("you weren't designated to + * approve" vs "this daemon only accepts loopback approvals"). + */ +export class PermissionForbiddenError extends Error { + readonly requestId: string; + readonly sessionId: string; + readonly reason: 'designated_mismatch' | 'remote_not_allowed'; + constructor( + requestId: string, + sessionId: string, + reason: 'designated_mismatch' | 'remote_not_allowed', + ) { + super( + `Permission ${requestId} on session ${sessionId}: ` + + `vote rejected by policy (${reason}).`, + ); + this.name = 'PermissionForbiddenError'; + this.requestId = requestId; + this.sessionId = sessionId; + this.reason = reason; + } +} + /** * #4175 Wave 4 PR 17. Thrown by `initWorkspace` when the target file * already exists with non-whitespace content and the caller did not diff --git a/packages/acp-bridge/src/bridgeOptions.ts b/packages/acp-bridge/src/bridgeOptions.ts index 1f7ee82d7c7..be57caf06ec 100644 --- a/packages/acp-bridge/src/bridgeOptions.ts +++ b/packages/acp-bridge/src/bridgeOptions.ts @@ -13,6 +13,8 @@ import type { ApprovalMode } from '@qwen-code/qwen-code-core'; import type { ChannelFactory } from './channel.js'; +import type { PermissionPolicy } from './permission.js'; +import type { PermissionAuditPublisher } from './permissionMediator.js'; import type { ServePreflightCell, ServeWorkspaceEnvStatus } from './status.js'; import type { BridgeFileSystem } from './bridgeFileSystem.js'; @@ -276,4 +278,46 @@ export interface BridgeOptions { * IDE keep working without depending on `cli/src/serve/fs/`. */ fileSystem?: BridgeFileSystem; + /** + * #4175 F3 Commit 2 — active permission mediation policy for the + * `MultiClientPermissionMediator`. When omitted, defaults to + * `'first-responder'` (the pre-F3 behavior — any validated voter + * wins immediately). The bridge captures this once at construction + * time; `runQwenServe` reads it from `settings.policy. + * permissionStrategy` and the mediator snapshots it onto every + * pending entry at issue time so live-reload of settings does not + * change the rules under in-flight requests. + */ + permissionPolicy?: PermissionPolicy; + /** + * #4175 F3 Commit 2 — optional fixed quorum for `consensus` policy. + * MUST be a positive integer if provided; the F3 settings layer + * validates this and fails startup on non-integer / non-positive + * values. Capped at `M = votersAtIssue.size` at request time to + * prevent unreachable quorum. Unset → `floor(M/2) + 1` (default + * majority). + */ + permissionConsensusQuorum?: number; + /** + * #4175 F3 Commit 2 — injection seam for the permission audit + * publisher. + * + * **When omitted**: the bridge falls back to + * `createNoOpPermissionAuditPublisher` so embedded callers (and + * the bridge unit-test suite) can run the mediator without an + * audit consumer. + * + * **In production** (`qwen serve`), `runQwenServe.ts` allocates a + * `PermissionAuditRing` (default capacity 512), wraps it with + * `createPermissionAuditPublisher`, and passes the result here. + * The ring stays alive for the lifetime of the daemon so a future + * `GET /workspace/permission/audit` route (out of F3 v1 scope) + * can lift it out for query. + * + * Permission timeouts also produce a stderr breadcrumb directly + * from the mediator's timer callback (independent of this + * publisher) so operators tailing daemon stderr always see + * timeouts even when the audit publisher is the no-op fallback. + */ + permissionAudit?: PermissionAuditPublisher; } diff --git a/packages/acp-bridge/src/bridgeTypes.ts b/packages/acp-bridge/src/bridgeTypes.ts index 925c3957a66..b52abd339d8 100644 --- a/packages/acp-bridge/src/bridgeTypes.ts +++ b/packages/acp-bridge/src/bridgeTypes.ts @@ -16,6 +16,7 @@ import type { SetSessionModelResponse, } from '@agentclientprotocol/sdk'; import type { BridgeEvent, SubscribeOptions } from './eventBus.js'; +import type { PermissionPolicy } from './permission.js'; import type { ServeSessionContextStatus, ServeSessionSupportedCommandsStatus, @@ -93,6 +94,18 @@ export interface SessionMetadataUpdate { export interface BridgeClientRequestContext { /** Daemon-issued client id echoed through the HTTP transport header. */ clientId?: string; + /** + * `true` when the request arrived from a loopback peer (kernel-stamped + * `req.socket.remoteAddress` ∈ {`127.0.0.1`, `::1`, `::ffff:127.0.0.1`}). + * Populated by permission-vote routes for the `local-only` mediation + * policy; other routes leave this undefined. + * + * **Security**: this is NOT computed from `X-Forwarded-For` or any + * other forwardable HTTP header — those are forgeable. Callers that + * reverse-proxy `qwen serve` should not rely on `local-only` (use a + * dedicated daemon or `designated` policy instead). + */ + fromLoopback?: boolean; } /** @@ -381,6 +394,18 @@ export interface HttpAcpBridge { /** Test/inspection hook: number of permission requests awaiting a vote. */ readonly pendingPermissionCount: number; + /** + * #4175 F3 Commit 6 — active permission mediation policy. Reflects + * the value `runQwenServe` resolved from + * `settings.policy.permissionStrategy` (or the + * `'first-responder'` default). Surfaced through the + * `/capabilities` envelope's `policy.permission` field so SDK + * clients can feature-detect at runtime which strategy is in + * effect, distinct from the build-supported set advertised on + * the `permission_mediation` capability tag. + */ + readonly permissionPolicy: PermissionPolicy; + /** * Synchronous force-kill of every live channel. Called by signal * handlers when the operator double-taps Ctrl+C. diff --git a/packages/acp-bridge/src/index.ts b/packages/acp-bridge/src/index.ts index 8f7a90b56aa..316d85e5f28 100644 --- a/packages/acp-bridge/src/index.ts +++ b/packages/acp-bridge/src/index.ts @@ -8,6 +8,7 @@ export * from './eventBus.js'; export * from './inMemoryChannel.js'; export * from './channel.js'; export * from './permission.js'; +export * from './permissionMediator.js'; export * from './workspacePaths.js'; export * from './status.js'; export * from './bridgeErrors.js'; diff --git a/packages/acp-bridge/src/permission.ts b/packages/acp-bridge/src/permission.ts index 0d53e3c0a06..d03d55b83b4 100644 --- a/packages/acp-bridge/src/permission.ts +++ b/packages/acp-bridge/src/permission.ts @@ -108,6 +108,19 @@ export type PermissionVoteOutcome = | { readonly kind: 'already_resolved'; readonly resolvedOptionId: string } | { readonly kind: 'forbidden'; + /** + * `designated_mismatch` fires for both: + * - `designated` policy: voter `clientId` is not the prompt + * `originatorClientId`. + * - `consensus` policy: voter `clientId` is undefined OR not + * in the issue-time `votersAtIssue` snapshot. Overloaded + * here to keep the contract closed; future versions may + * widen this union with a more specific reason if SDK + * consumers need to distinguish. + * + * `remote_not_allowed` fires under `local-only` policy when + * `vote.fromLoopback === false`. + */ readonly reason: 'designated_mismatch' | 'remote_not_allowed'; } | { readonly kind: 'unknown_request' }; diff --git a/packages/acp-bridge/src/permissionMediator.test.ts b/packages/acp-bridge/src/permissionMediator.test.ts new file mode 100644 index 00000000000..d39d34025b9 --- /dev/null +++ b/packages/acp-bridge/src/permissionMediator.test.ts @@ -0,0 +1,1215 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { + CANCEL_VOTE_SENTINEL, + MultiClientPermissionMediator, + type MediatorDeps, + type PermissionAuditPublisher, + type PermissionDecisionReason, +} from './permissionMediator.js'; +import { + type PermissionPolicy, + type PermissionRequestRecord, + type PermissionResolution, + type PermissionVote, + type PermissionVoteOutcome, +} from './permission.js'; +import { type BridgeEvent } from './eventBus.js'; +import { + CancelSentinelCollisionError, + InvalidPermissionOptionError, +} from './bridgeErrors.js'; + +interface AuditCall { + readonly kind: 'requested' | 'voted' | 'forbidden' | 'resolved' | 'timeout'; + readonly args: readonly unknown[]; +} + +function makeRecordingAudit(): { + audit: PermissionAuditPublisher; + calls: AuditCall[]; +} { + const calls: AuditCall[] = []; + const audit: PermissionAuditPublisher = { + recordRequested(record, policy, votersAtIssue) { + calls.push({ + kind: 'requested', + args: [record, policy, votersAtIssue], + }); + }, + recordVoted(record, vote, outcome) { + calls.push({ kind: 'voted', args: [record, vote, outcome] }); + }, + recordForbidden(record, vote, reason) { + calls.push({ kind: 'forbidden', args: [record, vote, reason] }); + }, + recordResolved(record, resolution, decisionReason) { + calls.push({ + kind: 'resolved', + args: [record, resolution, decisionReason], + }); + }, + recordTimeout(record) { + calls.push({ kind: 'timeout', args: [record] }); + }, + }; + return { audit, calls }; +} + +interface EmitCall { + readonly sessionId: string; + readonly event: Omit; +} + +function makeRecordingEmit(): { + emit: MediatorDeps['emit']; + events: EmitCall[]; +} { + const events: EmitCall[] = []; + const emit: MediatorDeps['emit'] = (sessionId, event) => { + events.push({ sessionId, event }); + }; + return { emit, events }; +} + +function makeRecord( + overrides: Partial = {}, +): PermissionRequestRecord { + return { + requestId: overrides.requestId ?? 'req-1', + sessionId: overrides.sessionId ?? 'sess-1', + originatorClientId: + 'originatorClientId' in overrides + ? overrides.originatorClientId + : 'client_A', + allowedOptionIds: + overrides.allowedOptionIds ?? + new Set(['proceed_once', 'proceed_always', 'reject_once']), + issuedAtMs: overrides.issuedAtMs ?? 1_000_000, + }; +} + +function makeVote(overrides: Partial = {}): PermissionVote { + return { + requestId: overrides.requestId ?? 'req-1', + sessionId: overrides.sessionId ?? 'sess-1', + clientId: 'clientId' in overrides ? overrides.clientId : 'client_A', + optionId: overrides.optionId ?? 'proceed_once', + receivedAtMs: overrides.receivedAtMs ?? 1_000_010, + fromLoopback: overrides.fromLoopback ?? false, + }; +} + +function makeMediator( + policy: PermissionPolicy = 'first-responder', + voters: ReadonlySet = new Set(['client_A', 'client_B', 'client_C']), +) { + const { audit, calls } = makeRecordingAudit(); + const { emit, events } = makeRecordingEmit(); + const deps: MediatorDeps = { + emit, + audit, + now: () => 1_000_000, + votersForSession: () => voters, + }; + const mediator = new MultiClientPermissionMediator(policy, deps); + return { mediator, deps, audit, calls, emit, events }; +} + +describe('MultiClientPermissionMediator — first-responder', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it('synchronously registers pending in `request()` (N1 invariant)', () => { + const { mediator } = makeMediator(); + const record = makeRecord(); + + // The Promise returned by request() must be already-pending; the + // pending entry must be visible to peekSessionFor BEFORE we await. + void mediator.request(record, 5_000); + + // No await between request() and peekSessionFor; the pending must + // be in the map synchronously. + expect(mediator.peekSessionFor(record.requestId)).toBe(record.sessionId); + }); + + it('resolves on first valid vote and emits permission_resolved with voter clientId as originator (O8)', async () => { + const { mediator, calls, events } = makeMediator(); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + const outcome = mediator.vote(makeVote({ clientId: 'client_B' })); + expect(outcome).toEqual({ + kind: 'resolved', + resolvedOptionId: 'proceed_once', + }); + + const resolution = await promise; + expect(resolution).toEqual({ kind: 'option', optionId: 'proceed_once' }); + + // Emitted exactly one permission_resolved event for the session. + // O8 INVARIANT: originatorClientId is the VOTER's clientId, not the + // prompt originator. This is a documented pre-F3 inconsistency + // (permission_request stamps prompt-originator; permission_resolved + // stamps voter). F3 deliberately preserves it for wire compat. + expect(events).toHaveLength(1); + expect(events[0]).toEqual({ + sessionId: 'sess-1', + event: { + type: 'permission_resolved', + data: { + requestId: 'req-1', + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }, + originatorClientId: 'client_B', + }, + }); + + // Audit trail: requested → voted → resolved. + expect(calls.map((c) => c.kind)).toEqual([ + 'requested', + 'voted', + 'resolved', + ]); + + const resolvedCall = calls[2]!; + const decisionReason = resolvedCall.args[2] as PermissionDecisionReason; + expect(decisionReason).toEqual({ + type: 'first-responder', + resolverClientId: 'client_B', + }); + }); + + it('omits originatorClientId on permission_resolved when voter has no clientId', async () => { + const { mediator, events } = makeMediator(); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + mediator.vote(makeVote({ clientId: undefined })); + await promise; + + // Loopback voter without X-Qwen-Client-Id — pre-F3 spread guard + // omits the field entirely (not `originatorClientId: undefined`). + expect(events).toHaveLength(1); + expect(events[0]!.event).not.toHaveProperty('originatorClientId'); + }); + + it('returns already_resolved on a duplicate vote and re-emits the SSE notification', async () => { + const { mediator, events } = makeMediator(); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + mediator.vote(makeVote({ clientId: 'client_A' })); + await promise; + + // Late voter — same requestId, different clientId. + const outcome = mediator.vote( + makeVote({ clientId: 'client_C', optionId: 'proceed_always' }), + ); + expect(outcome).toEqual({ + kind: 'already_resolved', + resolvedOptionId: 'proceed_once', + }); + + // First permission_resolved + a re-emitted permission_already_resolved + // for the late voter. The replayed event does NOT carry + // `originatorClientId` — pre-F3 publishPermissionAlreadyResolved + // omitted the field and `httpAcpBridge.test.ts:2880` enshrines + // that wire shape. Resolver attribution lives in audit only. + expect(events.map((e) => e.event.type)).toEqual([ + 'permission_resolved', + 'permission_already_resolved', + ]); + const lateEvent = events[1]!; + expect(lateEvent.event.data).toEqual({ + requestId: 'req-1', + sessionId: 'sess-1', + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }); + expect(lateEvent.event).not.toHaveProperty('originatorClientId'); + }); + + it('returns unknown_request when the requestId was never seen', () => { + const { mediator } = makeMediator(); + const outcome = mediator.vote(makeVote({ requestId: 'nonexistent' })); + expect(outcome).toEqual({ kind: 'unknown_request' }); + }); + + it('rejects cross-session votes as unknown_request', async () => { + const { mediator } = makeMediator(); + const record = makeRecord(); + void mediator.request(record, 5_000); + + const outcome = mediator.vote(makeVote({ sessionId: 'sess-other' })); + expect(outcome).toEqual({ kind: 'unknown_request' }); + }); + + it('throws InvalidPermissionOptionError when optionId is not in the allow set', () => { + const { mediator } = makeMediator(); + const record = makeRecord(); + void mediator.request(record, 5_000); + + expect(() => + mediator.vote(makeVote({ optionId: 'proceed_always_forged' })), + ).toThrow(InvalidPermissionOptionError); + }); +}); + +describe('MultiClientPermissionMediator — voter cancel sentinel', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it('resolves cancelled on cancel sentinel regardless of policy', async () => { + for (const policy of [ + 'first-responder', + 'designated', + 'consensus', + 'local-only', + ] as const satisfies readonly PermissionPolicy[]) { + const { mediator, events, calls } = makeMediator(policy); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + const outcome = mediator.vote( + makeVote({ optionId: CANCEL_VOTE_SENTINEL }), + ); + expect(outcome).toEqual({ + kind: 'resolved', + resolvedOptionId: CANCEL_VOTE_SENTINEL, + }); + + const resolution = await promise; + expect(resolution).toEqual({ + kind: 'cancelled', + reason: 'agent_cancelled', + }); + + expect(events.map((e) => e.event.type)).toEqual(['permission_resolved']); + expect(events[0]!.event.data).toMatchObject({ + outcome: { outcome: 'cancelled' }, + }); + expect(events[0]!.event.originatorClientId).toBe('client_A'); + + const decisionReason = calls.find((c) => c.kind === 'resolved')! + .args[2] as PermissionDecisionReason; + expect(decisionReason).toEqual({ + type: 'voter-cancelled', + resolverClientId: 'client_A', + }); + } + }); + + it('does NOT validate cancel sentinel against allowedOptionIds', () => { + // The bridge constructs the sentinel from `{outcome:'cancelled'}` which + // never carries an optionId; the mediator must accept it without + // checking the allow set. + const { mediator } = makeMediator(); + const record = makeRecord({ + allowedOptionIds: new Set(['proceed_once']), + }); + void mediator.request(record, 5_000); + + expect(() => + mediator.vote(makeVote({ optionId: CANCEL_VOTE_SENTINEL })), + ).not.toThrow(); + }); + + // Wenshao review #4335 / 3271978359 — the existing + // `resolves cancelled on cancel sentinel regardless of policy` + // test uses a voter (`client_A`) that would be ACCEPTED by every + // policy: it's the prompt originator under designated and is in + // votersAtIssue under consensus. The cross-policy guarantee only + // matters for voters who would otherwise be REJECTED — these two + // adversarial cases lock in the cross-policy escape hatch + // semantics described on the CANCEL_VOTE_SENTINEL JSDoc. + it('cancel sentinel resolves under `designated` even when voter is NOT the originator', async () => { + const { mediator, events } = makeMediator('designated'); + const record = makeRecord(); // originator = 'client_A' + const promise = mediator.request(record, 5_000); + + // A normal `proceed_once` vote from client_B would be + // forbidden:designated_mismatch — but cancel must still resolve. + const outcome = mediator.vote( + makeVote({ clientId: 'client_B', optionId: CANCEL_VOTE_SENTINEL }), + ); + expect(outcome).toEqual({ + kind: 'resolved', + resolvedOptionId: CANCEL_VOTE_SENTINEL, + }); + const resolution = await promise; + expect(resolution).toEqual({ + kind: 'cancelled', + reason: 'agent_cancelled', + }); + expect(events.map((e) => e.event.type)).toEqual(['permission_resolved']); + }); + + it('cancel sentinel resolves under `consensus` even when voter is NOT in votersAtIssue', async () => { + const { mediator, events } = makeMediator( + 'consensus', + // votersAtIssue snapshot does NOT contain client_late_join + new Set(['client_A', 'client_B']), + ); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + // A normal `proceed_once` vote from a non-snapshot voter would + // be forbidden:designated_mismatch — but cancel must still resolve. + const outcome = mediator.vote( + makeVote({ + clientId: 'client_late_join', + optionId: CANCEL_VOTE_SENTINEL, + }), + ); + expect(outcome).toEqual({ + kind: 'resolved', + resolvedOptionId: CANCEL_VOTE_SENTINEL, + }); + const resolution = await promise; + expect(resolution).toEqual({ + kind: 'cancelled', + reason: 'agent_cancelled', + }); + expect(events.map((e) => e.event.type)).toEqual(['permission_resolved']); + }); + + it('rejects request() at issue time when allowedOptionIds collides with cancel sentinel', () => { + // Collision defense (Commit 1 review I1): if the agent's allow + // set legitimately contains '__cancelled__', the mediator can no + // longer disambiguate a real vote on that option from a cancel + // intent. Fail loud at request() rather than silently flipping + // a real approval to cancel later. + const { mediator } = makeMediator(); + const record = makeRecord({ + allowedOptionIds: new Set(['proceed_once', CANCEL_VOTE_SENTINEL]), + }); + expect(() => mediator.request(record, 5_000)).toThrow( + CancelSentinelCollisionError, + ); + + // The mediator state must remain clean after the throw — no + // pending entry leaked. + expect(mediator.peekSessionFor('req-1')).toBeUndefined(); + }); +}); + +describe('MultiClientPermissionMediator — forgetSession', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it('cancels every pending request matching the session', async () => { + const { mediator, events } = makeMediator(); + const recordA = makeRecord({ requestId: 'req-A', sessionId: 'sess-1' }); + const recordB = makeRecord({ requestId: 'req-B', sessionId: 'sess-1' }); + const recordOther = makeRecord({ + requestId: 'req-C', + sessionId: 'sess-2', + }); + const promiseA = mediator.request(recordA, 5_000); + const promiseB = mediator.request(recordB, 5_000); + const promiseOther = mediator.request(recordOther, 5_000); + + mediator.forgetSession('sess-1'); + + const [resA, resB] = await Promise.all([promiseA, promiseB]); + expect(resA).toEqual({ kind: 'cancelled', reason: 'session_closed' }); + expect(resB).toEqual({ kind: 'cancelled', reason: 'session_closed' }); + + // The other session's pending stays alive. + expect(mediator.peekSessionFor('req-C')).toBe('sess-2'); + + // Two permission_resolved emits, both for sess-1. + const sess1Events = events.filter((e) => e.sessionId === 'sess-1'); + expect(sess1Events).toHaveLength(2); + expect(sess1Events.map((e) => e.event.type)).toEqual([ + 'permission_resolved', + 'permission_resolved', + ]); + + // Resolve the third so promise doesn't dangle. + mediator.vote( + makeVote({ + requestId: 'req-C', + sessionId: 'sess-2', + optionId: 'proceed_once', + }), + ); + await promiseOther; + }); + + it('is idempotent — second call is a no-op', () => { + const { mediator, events } = makeMediator(); + const record = makeRecord(); + void mediator.request(record, 5_000); + + mediator.forgetSession('sess-1'); + const eventsAfterFirst = events.length; + + mediator.forgetSession('sess-1'); + expect(events.length).toBe(eventsAfterFirst); + }); + + it('does not affect resolved entries (already-decided permissions stay queryable)', async () => { + const { mediator } = makeMediator(); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + mediator.vote(makeVote()); + await promise; + + mediator.forgetSession('sess-1'); + + // peekSessionFor still works for the resolved record (legacy + // bridge.respondToPermission relies on this for the + // permission_already_resolved fallback). + expect(mediator.peekSessionFor('req-1')).toBe('sess-1'); + }); +}); + +describe('MultiClientPermissionMediator — timeout', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it('resolves cancelled when the timer fires before any vote', async () => { + const { mediator, events, calls } = makeMediator(); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + vi.advanceTimersByTime(5_000); + + const resolution = await promise; + expect(resolution).toEqual({ kind: 'cancelled', reason: 'timeout' }); + + // Timer-driven resolution has no voter — `permission_resolved` + // must omit `originatorClientId` rather than spread `undefined`. + expect(events).toHaveLength(1); + expect(events[0]!.event.type).toBe('permission_resolved'); + expect(events[0]!.event).not.toHaveProperty('originatorClientId'); + + expect(calls.map((c) => c.kind)).toEqual([ + 'requested', + 'timeout', + 'resolved', + ]); + + const resolvedCall = calls[2]!; + const decisionReason = resolvedCall.args[2] as PermissionDecisionReason; + expect(decisionReason).toMatchObject({ + type: 'timeout', + issuedAtMs: 1_000_000, + timeoutMs: 5_000, + }); + expect((decisionReason as { firedAtMs: number }).firedAtMs).toBe(1_000_000); + }); + + it('clears the timer when the entry resolves via vote', async () => { + const { mediator, calls } = makeMediator(); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + mediator.vote(makeVote()); + await promise; + + // Fast-forward — the cleared timer must NOT fire. + vi.advanceTimersByTime(10_000); + + expect(calls.some((c) => c.kind === 'timeout')).toBe(false); + }); + + // Wenshao review #4335 / 3270622304 — pre-F3 wrote a stderr line on + // every permission timeout; F3's mediator timer must preserve that + // breadcrumb so operators tailing daemon stderr still see timeouts + // even when the audit publisher is the no-op fallback (embedded + // callers / unit tests). + it('writes a stderr breadcrumb when the timer fires', async () => { + const writes: string[] = []; + const writeSpy = vi + .spyOn(process.stderr, 'write') + .mockImplementation((chunk: string | Uint8Array): boolean => { + writes.push(typeof chunk === 'string' ? chunk : chunk.toString()); + return true; + }); + try { + const { mediator } = makeMediator(); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + vi.advanceTimersByTime(5_000); + await promise; + + const breadcrumb = writes.find((w) => + w.includes('timed out after 5000ms'), + ); + expect(breadcrumb).toBeDefined(); + expect(breadcrumb).toContain('req-1'); + expect(breadcrumb).toContain('sess-1'); + } finally { + writeSpy.mockRestore(); + } + }); +}); + +describe('MultiClientPermissionMediator — peekSessionFor', () => { + it('returns undefined for unknown requestIds', () => { + const { mediator } = makeMediator(); + expect(mediator.peekSessionFor('never-seen')).toBeUndefined(); + }); + + it('returns sessionId for pending and resolved alike', async () => { + const { mediator } = makeMediator(); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + expect(mediator.peekSessionFor('req-1')).toBe('sess-1'); + + mediator.vote(makeVote()); + await promise; + expect(mediator.peekSessionFor('req-1')).toBe('sess-1'); + }); +}); + +describe('MultiClientPermissionMediator — designated', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it('resolves when the originator votes', async () => { + const { mediator, calls } = makeMediator('designated'); + const record = makeRecord({ originatorClientId: 'client_A' }); + const promise = mediator.request(record, 5_000); + const outcome = mediator.vote(makeVote({ clientId: 'client_A' })); + expect(outcome).toEqual({ + kind: 'resolved', + resolvedOptionId: 'proceed_once', + }); + await promise; + const decisionReason = calls.find((c) => c.kind === 'resolved')! + .args[2] as PermissionDecisionReason; + expect(decisionReason).toEqual({ + type: 'designated-originator', + originatorClientId: 'client_A', + }); + }); + + it('rejects votes from non-originators with permission_forbidden', async () => { + const { mediator, events, calls } = makeMediator('designated'); + const record = makeRecord({ originatorClientId: 'client_A' }); + const promise = mediator.request(record, 5_000); + const outcome = mediator.vote(makeVote({ clientId: 'client_B' })); + expect(outcome).toEqual({ + kind: 'forbidden', + reason: 'designated_mismatch', + }); + expect(events.map((e) => e.event.type)).toEqual(['permission_forbidden']); + expect(events[0]!.event.data).toEqual({ + requestId: 'req-1', + sessionId: 'sess-1', + clientId: 'client_B', + reason: 'designated_mismatch', + }); + expect(events[0]!.event.originatorClientId).toBe('client_A'); + expect(calls.find((c) => c.kind === 'forbidden')).toBeDefined(); + // The pending must still be alive after a forbidden vote. + expect(mediator.peekSessionFor('req-1')).toBe('sess-1'); + mediator.forgetSession('sess-1'); + await promise; + }); + + it('falls back to first-responder when prompt has no originator (anonymous)', async () => { + const { mediator, calls } = makeMediator('designated'); + const record = makeRecord({ originatorClientId: undefined }); + const promise = mediator.request(record, 5_000); + const outcome = mediator.vote(makeVote({ clientId: 'client_C' })); + expect(outcome).toEqual({ + kind: 'resolved', + resolvedOptionId: 'proceed_once', + }); + await promise; + const decisionReason = calls.find((c) => c.kind === 'resolved')! + .args[2] as PermissionDecisionReason; + expect(decisionReason).toEqual({ + type: 'first-responder', + resolverClientId: 'client_C', + }); + }); +}); + +describe('MultiClientPermissionMediator — consensus', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it('resolves on first option to reach quorum (M=3, default N=2)', async () => { + const { mediator, events, calls } = makeMediator( + 'consensus', + new Set(['client_A', 'client_B', 'client_C']), + ); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + const v1 = mediator.vote(makeVote({ clientId: 'client_A' })); + expect(v1).toEqual({ kind: 'recorded', votesNeeded: 1 }); + expect(events.map((e) => e.event.type)).toEqual([ + 'permission_partial_vote', + ]); + expect(events[0]!.event.data).toEqual({ + requestId: 'req-1', + sessionId: 'sess-1', + votesReceived: 1, + votesNeeded: 1, + quorum: 2, + optionTallies: { proceed_once: 1 }, + }); + + const v2 = mediator.vote(makeVote({ clientId: 'client_B' })); + expect(v2).toEqual({ + kind: 'resolved', + resolvedOptionId: 'proceed_once', + }); + await promise; + + expect(events.map((e) => e.event.type)).toEqual([ + 'permission_partial_vote', + 'permission_resolved', + ]); + + const decisionReason = calls.find((c) => c.kind === 'resolved')! + .args[2] as PermissionDecisionReason; + expect(decisionReason).toEqual({ + type: 'consensus-quorum', + resolvedOptionId: 'proceed_once', + quorum: 2, + tally: 2, + }); + }); + + it('keeps the original vote on idempotent re-vote (no tally change, no partial_vote re-emit)', async () => { + const { mediator, events } = makeMediator( + 'consensus', + new Set(['client_A', 'client_B', 'client_C']), + ); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + mediator.vote(makeVote({ clientId: 'client_A', optionId: 'proceed_once' })); + expect(events).toHaveLength(1); + + const v2 = mediator.vote( + makeVote({ clientId: 'client_A', optionId: 'proceed_always' }), + ); + expect(v2).toEqual({ kind: 'recorded', votesNeeded: 1 }); + expect(events).toHaveLength(1); + + mediator.vote(makeVote({ clientId: 'client_B', optionId: 'proceed_once' })); + await promise; + }); + + // Wenshao review #4335 / 3271041464 — when a voter's idempotent + // re-vote attempts a different optionId, the audit ring must + // record the ORIGINALLY-voted option (the one in the tally), not + // the new attempt. Otherwise an operator reading the audit trail + // sees `client_A voted for option_B` while the tally has client_A + // in option_A's bucket — a misleading record of a vote that + // never counted. + it('records the original optionId in audit on idempotent re-vote (3271041464)', async () => { + const { mediator, calls } = makeMediator( + 'consensus', + new Set(['client_A', 'client_B', 'client_C']), + ); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + // Original vote: client_A → proceed_once. + mediator.vote(makeVote({ clientId: 'client_A', optionId: 'proceed_once' })); + + // Re-vote attempt: client_A → proceed_always (silently kept as + // proceed_once in the tally; SHOULD be audited as proceed_once). + mediator.vote( + makeVote({ clientId: 'client_A', optionId: 'proceed_always' }), + ); + + // Resolve to terminate the test cleanly. + mediator.vote(makeVote({ clientId: 'client_B', optionId: 'proceed_once' })); + await promise; + + // Two `voted` audit calls fired (one per vote attempt). The + // first records the original option as cast; the second records + // the original option even though the wire attempt was different. + const votedCalls = calls.filter((c) => c.kind === 'voted'); + expect(votedCalls).toHaveLength(3); // client_A original, client_A re-vote, client_B winning vote + // First call — straightforward: client_A cast proceed_once. + expect((votedCalls[0]!.args[1] as { optionId: string }).optionId).toBe( + 'proceed_once', + ); + // Second call — the idempotent re-vote case: the audit must show + // proceed_once (the option in the tally), NOT proceed_always + // (the attempted re-vote). This is the regression-guard the + // pre-fix code violated. + expect((votedCalls[1]!.args[1] as { optionId: string }).optionId).toBe( + 'proceed_once', + ); + }); + + it('rejects anonymous voter with permission_forbidden', () => { + const { mediator, events } = makeMediator( + 'consensus', + new Set(['client_A', 'client_B', 'client_C']), + ); + const record = makeRecord(); + void mediator.request(record, 5_000); + + const v = mediator.vote(makeVote({ clientId: undefined })); + expect(v).toEqual({ kind: 'forbidden', reason: 'designated_mismatch' }); + expect(events.map((e) => e.event.type)).toEqual(['permission_forbidden']); + // I-4 (Commit 4 review) — N3 invariant: forbidden event stamps + // the prompt originator, not the rejected voter. + expect(events[0]!.event.originatorClientId).toBe('client_A'); + // Anonymous voter — `clientId` MUST NOT appear on the data + // object (no field rather than `clientId: undefined`). + expect(events[0]!.event.data).not.toHaveProperty('clientId'); + mediator.forgetSession('sess-1'); + }); + + it('rejects voter not in votersAtIssue snapshot', () => { + const { mediator, events } = makeMediator( + 'consensus', + new Set(['client_A', 'client_B']), + ); + const record = makeRecord(); + void mediator.request(record, 5_000); + + const v = mediator.vote(makeVote({ clientId: 'client_late_join' })); + expect(v).toEqual({ kind: 'forbidden', reason: 'designated_mismatch' }); + expect(events.map((e) => e.event.type)).toEqual(['permission_forbidden']); + // I-4 (Commit 4 review) — prompt originator on N3 forbidden event. + expect(events[0]!.event.originatorClientId).toBe('client_A'); + expect(events[0]!.event.data).toMatchObject({ + clientId: 'client_late_join', + reason: 'designated_mismatch', + }); + mediator.forgetSession('sess-1'); + }); + + // Wenshao review #4335 / 3272568031 — `writeForbiddenStderr` has 3 + // call sites (voteDesignated / voteConsensus / voteLocalOnly) but + // before this commit only the SSE event + audit record were tested. + // Pin the stderr breadcrumb format and presence so a refactor can't + // silently drop it. + it('writes stderr breadcrumbs for all 3 forbidden-vote paths', () => { + const writes: string[] = []; + const writeSpy = vi + .spyOn(process.stderr, 'write') + .mockImplementation((chunk: string | Uint8Array): boolean => { + writes.push(typeof chunk === 'string' ? chunk : chunk.toString()); + return true; + }); + try { + // 1. designated — non-originator voter rejected. + { + const { mediator } = makeMediator('designated'); + void mediator.request(makeRecord(), 5_000); + mediator.vote(makeVote({ clientId: 'client_B' })); + mediator.forgetSession('sess-1'); + } + // 2. consensus — voter not in votersAtIssue rejected. + { + const { mediator } = makeMediator( + 'consensus', + new Set(['client_A', 'client_B']), + ); + void mediator.request(makeRecord(), 5_000); + mediator.vote(makeVote({ clientId: 'client_late_join' })); + mediator.forgetSession('sess-1'); + } + // 3. local-only — non-loopback voter rejected. + { + const { mediator } = makeMediator('local-only'); + void mediator.request(makeRecord(), 5_000); + mediator.vote( + makeVote({ clientId: 'client_remote', fromLoopback: false }), + ); + mediator.forgetSession('sess-1'); + } + + const breadcrumbs = writes.filter((w) => w.includes('vote rejected')); + expect(breadcrumbs).toHaveLength(3); + expect(breadcrumbs[0]).toContain('designated_mismatch'); + expect(breadcrumbs[0]).toContain('voter is not the prompt originator'); + expect(breadcrumbs[1]).toContain('designated_mismatch'); + expect(breadcrumbs[1]).toContain('not in consensus votersAtIssue'); + expect(breadcrumbs[2]).toContain('remote_not_allowed'); + expect(breadcrumbs[2]).toContain('local-only policy'); + // Each breadcrumb names the requestId + sessionId for grep-ability. + for (const b of breadcrumbs) { + expect(b).toContain('req-1'); + expect(b).toContain('sess-1'); + } + } finally { + writeSpy.mockRestore(); + } + }); + + it('M=4 N=3 split 2-2 never resolves and times out', async () => { + // I-5 (Commit 4 review) — explicitly cover the + // "no winner; only cancel via timeout / forgetSession" case + // that the M=3 N=2 property test cannot reach. + const { mediator } = makeMediator( + 'consensus', + new Set(['client_A', 'client_B', 'client_C', 'client_D']), + ); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + mediator.vote(makeVote({ clientId: 'client_A', optionId: 'proceed_once' })); + mediator.vote(makeVote({ clientId: 'client_B', optionId: 'proceed_once' })); + mediator.vote( + makeVote({ clientId: 'client_C', optionId: 'proceed_always' }), + ); + const v4 = mediator.vote( + makeVote({ clientId: 'client_D', optionId: 'proceed_always' }), + ); + // Quorum N = floor(4/2)+1 = 3. Top tally is 2/2 split. No winner. + expect(v4).toEqual({ kind: 'recorded', votesNeeded: 1 }); + + // Timeout fires → cancelled. + vi.advanceTimersByTime(5_000); + const resolution = await promise; + expect(resolution).toEqual({ kind: 'cancelled', reason: 'timeout' }); + }); + + it('honors consensusQuorum override capped at M', async () => { + const { audit } = makeRecordingAudit(); + const { emit } = makeRecordingEmit(); + const deps: MediatorDeps = { + emit, + audit, + consensusQuorum: 100, + now: () => 1_000_000, + votersForSession: () => new Set(['client_A', 'client_B', 'client_C']), + }; + const mediator = new MultiClientPermissionMediator('consensus', deps); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + mediator.vote(makeVote({ clientId: 'client_A' })); + mediator.vote(makeVote({ clientId: 'client_B' })); + const v3 = mediator.vote(makeVote({ clientId: 'client_C' })); + expect(v3).toEqual({ kind: 'resolved', resolvedOptionId: 'proceed_once' }); + await promise; + }); + + it('property-style: enumerate vote interleavings for M=3 N=2 — first option to N wins', async () => { + const voters = ['client_A', 'client_B', 'client_C']; + const options: ReadonlyArray<'option_yes' | 'option_no'> = [ + 'option_yes', + 'option_no', + ]; + for (let assignmentMask = 0; assignmentMask < 8; assignmentMask++) { + const assignments = voters.map((_, idx) => + ((assignmentMask >> idx) & 1) === 1 ? options[0] : options[1], + ); + const orderings: Array<[number, number, number]> = [ + [0, 1, 2], + [0, 2, 1], + [1, 0, 2], + [1, 2, 0], + [2, 0, 1], + [2, 1, 0], + ]; + for (const order of orderings) { + const { mediator } = makeMediator('consensus', new Set(voters)); + const record = makeRecord({ + requestId: `req-prop-${assignmentMask}-${order.join('')}`, + allowedOptionIds: new Set(options), + }); + const promise = mediator.request(record, 5_000); + + let referenceWinner: string | null = null; + const refTally = new Map>(); + const recordedOutcomes: PermissionVoteOutcome[] = []; + for (const idx of order) { + const voter = voters[idx]!; + const option = assignments[idx]!; + if (referenceWinner === null) { + let set = refTally.get(option); + if (!set) { + set = new Set(); + refTally.set(option, set); + } + set.add(voter); + if (set.size >= 2) referenceWinner = option; + } + const outcome = mediator.vote({ + requestId: record.requestId, + sessionId: record.sessionId, + clientId: voter, + optionId: option, + receivedAtMs: 0, + fromLoopback: false, + }); + recordedOutcomes.push(outcome); + if (outcome.kind === 'resolved') break; + } + + const mediatorWinner = recordedOutcomes.find( + (o) => o.kind === 'resolved', + ) as { kind: 'resolved'; resolvedOptionId: string } | undefined; + if (referenceWinner !== null) { + expect(mediatorWinner).toBeDefined(); + expect(mediatorWinner!.resolvedOptionId).toBe(referenceWinner); + await promise; + } else { + mediator.forgetSession(record.sessionId); + await promise; + } + } + } + }); + + it('emits permission_partial_vote BEFORE permission_resolved (ordering invariant)', async () => { + const { mediator, events } = makeMediator( + 'consensus', + new Set(['client_A', 'client_B', 'client_C']), + ); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + mediator.vote(makeVote({ clientId: 'client_A' })); + mediator.vote(makeVote({ clientId: 'client_B' })); + await promise; + const types = events.map((e) => e.event.type); + const partialIdx = types.indexOf('permission_partial_vote'); + const resolvedIdx = types.indexOf('permission_resolved'); + expect(partialIdx).toBeGreaterThanOrEqual(0); + expect(resolvedIdx).toBeGreaterThan(partialIdx); + }); +}); + +describe('MultiClientPermissionMediator — local-only', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it('resolves on a loopback vote', async () => { + const { mediator, calls } = makeMediator('local-only'); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + const outcome = mediator.vote(makeVote({ fromLoopback: true })); + expect(outcome).toEqual({ + kind: 'resolved', + resolvedOptionId: 'proceed_once', + }); + await promise; + const decisionReason = calls.find((c) => c.kind === 'resolved')! + .args[2] as PermissionDecisionReason; + expect(decisionReason).toEqual({ + type: 'local-only-loopback', + resolverClientId: 'client_A', + }); + }); + + it('rejects a non-loopback vote with permission_forbidden / remote_not_allowed', async () => { + // Use a distinct prompt originator from the voter so the N3 + // stamping invariant is observable (I-4 Commit 4 review). + const { mediator, events, calls } = makeMediator('local-only'); + const record = makeRecord({ originatorClientId: 'client_PROMPT' }); + const promise = mediator.request(record, 5_000); + const outcome = mediator.vote(makeVote({ fromLoopback: false })); + expect(outcome).toEqual({ + kind: 'forbidden', + reason: 'remote_not_allowed', + }); + expect(events.map((e) => e.event.type)).toEqual(['permission_forbidden']); + expect(events[0]!.event.data).toEqual({ + requestId: 'req-1', + sessionId: 'sess-1', + clientId: 'client_A', + reason: 'remote_not_allowed', + }); + // I-4 (Commit 4 review) — N3 invariant: forbidden event stamps + // the prompt originator (`client_PROMPT`), NOT the rejected + // voter (`client_A`). + expect(events[0]!.event.originatorClientId).toBe('client_PROMPT'); + expect(calls.find((c) => c.kind === 'forbidden')).toBeDefined(); + mediator.forgetSession('sess-1'); + await promise; + }); +}); + +describe('MultiClientPermissionMediator — N2 cleanup ordering', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + afterEach(() => { + vi.useRealTimers(); + }); + + it('resolves the Promise even when emit throws', async () => { + const { audit } = makeRecordingAudit(); + const emit = vi.fn(() => { + throw new Error('bus closed during shutdown'); + }); + const deps: MediatorDeps = { + emit, + audit, + now: () => 0, + votersForSession: () => new Set(['client_A']), + }; + const mediator = new MultiClientPermissionMediator('first-responder', deps); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + mediator.vote(makeVote()); + + const resolution: PermissionResolution = await promise; + expect(resolution).toEqual({ kind: 'option', optionId: 'proceed_once' }); + + // Pending must have been deleted despite emit throwing. + expect(mediator.peekSessionFor('req-1')).toBe('sess-1'); + const dupOutcome: PermissionVoteOutcome = mediator.vote(makeVote()); + expect(dupOutcome.kind).toBe('already_resolved'); + }); + + it('resolves the Promise even when audit throws on recordRequested + recordResolved', async () => { + const audit: PermissionAuditPublisher = { + recordRequested: vi.fn(() => { + throw new Error('audit ring full'); + }), + recordVoted: vi.fn(), + recordForbidden: vi.fn(), + recordResolved: vi.fn(() => { + throw new Error('audit ring full'); + }), + recordTimeout: vi.fn(), + }; + const { emit } = makeRecordingEmit(); + const deps: MediatorDeps = { + emit, + audit, + now: () => 0, + votersForSession: () => new Set(['client_A']), + }; + const mediator = new MultiClientPermissionMediator('first-responder', deps); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + mediator.vote(makeVote()); + const resolution = await promise; + expect(resolution).toEqual({ kind: 'option', optionId: 'proceed_once' }); + }); + + it('resolves the Promise even when audit.recordVoted throws (vote path)', async () => { + const audit: PermissionAuditPublisher = { + recordRequested: vi.fn(), + recordVoted: vi.fn(() => { + throw new Error('audit publisher transient error'); + }), + recordForbidden: vi.fn(), + recordResolved: vi.fn(), + recordTimeout: vi.fn(), + }; + const { emit } = makeRecordingEmit(); + const deps: MediatorDeps = { + emit, + audit, + now: () => 0, + votersForSession: () => new Set(['client_A']), + }; + const mediator = new MultiClientPermissionMediator('first-responder', deps); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + // Pre-fix bug: recordVoted threw before resolveEntry, leaving the + // Promise hung. With safeAudit wrapping, vote() must still resolve. + expect(() => mediator.vote(makeVote())).not.toThrow(); + const resolution = await promise; + expect(resolution).toEqual({ kind: 'option', optionId: 'proceed_once' }); + }); + + it('resolves the Promise even when audit.recordVoted throws (cancel sentinel path)', async () => { + const audit: PermissionAuditPublisher = { + recordRequested: vi.fn(), + recordVoted: vi.fn(() => { + throw new Error('audit publisher transient error'); + }), + recordForbidden: vi.fn(), + recordResolved: vi.fn(), + recordTimeout: vi.fn(), + }; + const { emit } = makeRecordingEmit(); + const deps: MediatorDeps = { + emit, + audit, + now: () => 0, + votersForSession: () => new Set(['client_A']), + }; + const mediator = new MultiClientPermissionMediator('first-responder', deps); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + expect(() => + mediator.vote(makeVote({ optionId: CANCEL_VOTE_SENTINEL })), + ).not.toThrow(); + const resolution = await promise; + expect(resolution).toEqual({ + kind: 'cancelled', + reason: 'agent_cancelled', + }); + }); + + it('resolves the Promise even when audit.recordTimeout throws (timeout path)', async () => { + const audit: PermissionAuditPublisher = { + recordRequested: vi.fn(), + recordVoted: vi.fn(), + recordForbidden: vi.fn(), + recordResolved: vi.fn(), + recordTimeout: vi.fn(() => { + throw new Error('audit publisher transient error'); + }), + }; + const { emit } = makeRecordingEmit(); + const deps: MediatorDeps = { + emit, + audit, + now: () => 9_999, + votersForSession: () => new Set(['client_A']), + }; + const mediator = new MultiClientPermissionMediator('first-responder', deps); + const record = makeRecord(); + const promise = mediator.request(record, 5_000); + + // Pre-fix bug: recordTimeout was naked inside the timer callback; + // a throw left the Promise hung permanently and the pending entry + // leaked. With safeAudit wrapping, the timeout still resolves. + vi.advanceTimersByTime(5_000); + const resolution = await promise; + expect(resolution).toEqual({ kind: 'cancelled', reason: 'timeout' }); + expect(mediator.peekSessionFor('req-1')).toBe('sess-1'); + }); +}); diff --git a/packages/acp-bridge/src/permissionMediator.ts b/packages/acp-bridge/src/permissionMediator.ts new file mode 100644 index 00000000000..25969496e9c --- /dev/null +++ b/packages/acp-bridge/src/permissionMediator.ts @@ -0,0 +1,1291 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * `MultiClientPermissionMediator` — implementation of the + * `PermissionMediator` contract from `./permission.ts`. + * + * Owns ALL pending and resolved permission state for the bridge. + * `httpAcpBridge.ts` no longer keeps `pendingPermissions: Map` or + * `resolvedPermissions: LRU` — those are inside this class. + * + * Strategy dispatch: a single class with `switch (entry.policy)` inside + * `vote()`. Per-policy logic stays small (5–15 lines each); strategy + * sub-classes would be more boilerplate than substance. + * + * Per #4175 F3 plan. Companion to PR 22a's frozen contract. + */ + +import { + type PermissionMediator, + type PermissionPolicy, + type PermissionRequestRecord, + type PermissionResolution, + type PermissionVote, + type PermissionVoteOutcome, +} from './permission.js'; +import { type BridgeEvent } from './eventBus.js'; +import { + CancelSentinelCollisionError, + InvalidPermissionOptionError, +} from './bridgeErrors.js'; + +/** + * Sentinel `optionId` value the bridge maps voter `{outcome:'cancelled'}` + * to before calling `mediator.vote`. The mediator recognizes this and + * resolves the pending as `{kind:'cancelled', reason:'agent_cancelled'}` + * regardless of the active policy. + * + * **Bridge-side precondition**: callers MUST NOT forward an incoming + * `vote.optionId === CANCEL_VOTE_SENTINEL` from a wire client — the + * mediator treats the sentinel as cancel intent without consulting the + * `allowedOptionIds` set, so wire-originated sentinel votes would + * silently flip a real approval into a cancel. The bridge constructs + * the sentinel only from a `{outcome:'cancelled'}` ACP body that + * carries no `optionId` of its own. + * + * **Cross-policy escape hatch (intentional)**: cancel routes BEFORE + * policy dispatch. A non-loopback voter under `local-only` and a + * not-in-voter-set client under `consensus` can both still resolve the + * pending as cancelled by posting `{outcome:'cancelled'}`. This is + * deliberate — voter-cancel is the agent-side abort path; if the + * threat model required policy-gated cancel, that would be a future + * contract change. Documented here so a future maintainer doesn't + * "fix" the bypass. + * + * **Collision defense**: `mediator.request` rejects records whose + * `allowedOptionIds` contains the sentinel by throwing + * `CancelSentinelCollisionError` so an agent legitimately publishing + * `'__cancelled__'` as an option label can't masquerade as cancel. + */ +export const CANCEL_VOTE_SENTINEL = '__cancelled__' as const; + +/** + * Bounded FIFO size for the `resolved` map (duplicate-vote dedup + + * `permission_already_resolved` source). DeepSeek review #4335 / + * 3271627446 — the eviction in `rememberResolved` uses + * `resolvedOrder.shift()` (drop oldest), not LRU; mirrors the FIFO + * `PermissionAuditRing` correction in commit b0242ddec. Mirrors the + * `MAX_RESOLVED_PERMISSION_RECORDS` constant from the pre-F3 inline + * implementation in `httpAcpBridge.ts` (512 entries). Stores only + * requestId / sessionId / outcome, so 512 records stays well under + * 100 KB across normal UI reconnect/race windows. + */ +const MAX_RESOLVED_PERMISSION_RECORDS = 512; + +/** + * Structured "why did this resolve like that?" record attached to + * audit `permission.resolved` events. Borrowed from claude-code's + * `PermissionDecisionReason`. + * + * **Wire-vs-audit overload note**: `'agent-cancelled'` and + * `'voter-cancelled'` both project to the same wire shape + * (`PermissionResolution { kind:'cancelled', reason:'agent_cancelled' }`) + * because the ACP protocol doesn't distinguish them. The discrimination + * lives only in the audit log — useful for forensics, invisible on the + * bus. F3 deliberately preserves this overload to avoid breaking the + * frozen `permission.ts` contract. + * + * `resolverClientId: string | undefined` on `'first-responder'`, + * `'local-only-loopback'`, and `'voter-cancelled'` is undefined when + * the resolving voter connected over loopback without a registered + * `X-Qwen-Client-Id` header — a legitimate path for the local TUI + * default flow. The field is required-but-nullable rather than + * optional to force callers to think about the loopback case. + */ +export type PermissionDecisionReason = + | { + readonly type: 'first-responder'; + readonly resolverClientId: string | undefined; + } + | { + readonly type: 'designated-originator'; + readonly originatorClientId: string; + } + | { + readonly type: 'consensus-quorum'; + readonly resolvedOptionId: string; + readonly quorum: number; + readonly tally: number; + } + | { + readonly type: 'local-only-loopback'; + readonly resolverClientId: string | undefined; + } + | { + readonly type: 'timeout'; + readonly issuedAtMs: number; + readonly timeoutMs: number; + /** `deps.now()` at timer fire — distinct from `issuedAtMs + + * timeoutMs` under load (timer queue scheduling delay). */ + readonly firedAtMs: number; + } + | { readonly type: 'session-closed' } + /** Agent cancelled the underlying prompt before any voter resolved + * the permission. Wire shape collides with `'voter-cancelled'`. */ + | { readonly type: 'agent-cancelled' } + /** A voter posted `{outcome:'cancelled'}`. Wire shape collides with + * `'agent-cancelled'`. */ + | { + readonly type: 'voter-cancelled'; + readonly resolverClientId: string | undefined; + }; + +/** + * Audit sink the mediator writes to. Implementation lives in + * `packages/cli/src/serve/permissionAudit.ts` and writes into an + * in-memory bounded ring on the bridge — NOT onto the SSE bus + * (audit records and SSE wire events are intentionally separate + * channels per the F3 plan). + * + * The mediator depends only on this interface, so unit tests can + * substitute a no-op or a recording stub without dragging the host + * package's audit ring in. + */ +export interface PermissionAuditPublisher { + recordRequested( + record: PermissionRequestRecord, + policy: PermissionPolicy, + votersAtIssue: ReadonlySet, + ): void; + recordVoted( + record: PermissionRequestRecord, + vote: PermissionVote, + outcome: PermissionVoteOutcome, + ): void; + recordForbidden( + record: PermissionRequestRecord, + vote: PermissionVote, + reason: 'designated_mismatch' | 'remote_not_allowed', + ): void; + recordResolved( + record: PermissionRequestRecord, + resolution: PermissionResolution, + decisionReason: PermissionDecisionReason, + ): void; + recordTimeout(record: PermissionRequestRecord): void; +} + +/** + * Best-effort string-form of an unknown error value for breadcrumb + * lines written to `process.stderr`. Avoids the failure modes of + * blindly calling `String(err)` on a Symbol or `JSON.stringify` on + * a circular object. Whole body is try/catch'd: a pathological + * `Error` subclass with throwing `.name` / `.message` accessors + * (e.g. `Proxy`-wrapped errors, getter-overriding subclasses) MUST + * NOT escape from `safeAudit` / `safeEmit` and break the + * never-blocks-Promise-settle invariant. + */ +function stringifyError(err: unknown): string { + try { + if (err instanceof Error) return `${err.name}: ${err.message}`; + return String(err); + } catch { + return '[unstringifiable error]'; + } +} + +/** + * No-op `PermissionAuditPublisher` used as the bridge's default when + * the host omits `BridgeOptions.permissionAudit`. Production + * `qwen serve` provides a ring-backed publisher; embedded callers and + * unit tests that don't care about audit can let the bridge fall back + * here. Single canonical fallback prevents stub-vs-prod divergence + * (Commit 2 review note). + */ +export function createNoOpPermissionAuditPublisher(): PermissionAuditPublisher { + return { + recordRequested() {}, + recordVoted() {}, + recordForbidden() {}, + recordResolved() {}, + recordTimeout() {}, + }; +} + +/** + * Dependency hooks the mediator needs from its host (the bridge). + * Plumbed through `MultiClientPermissionMediator`'s constructor; tests + * pass a stub. + */ +export interface MediatorDeps { + /** + * Best-effort fan-out of a wire event onto the per-session SSE bus. + * The mediator passes `sessionId` explicitly so the bridge can route + * to `byId.get(sessionId)?.events.publish(event)` without reverse- + * lookup. If the entry is gone (session torn down between issue and + * emit), the bridge silently drops; the audit record still lands. + */ + emit: (sessionId: string, event: Omit) => void; + /** Audit ring writer. */ + audit: PermissionAuditPublisher; + /** + * Optional fixed quorum for `consensus`. When set, capped to + * `M = votersAtIssue.size` to prevent unreachable quorum. When + * unset, mediator computes `floor(M/2) + 1`. + */ + consensusQuorum?: number; + /** Wallclock supplier — injectable for deterministic tests. Used by + * the timeout decision-reason `firedAtMs` field. */ + now: () => number; + /** + * Snapshot of registered voter `clientId`s for the session at the + * moment of `request()`. The mediator captures this into + * `MediatorPending.votersAtIssue`; consensus rejects votes from + * `clientId`s not in the snapshot. + * + * Implementation: `(sid) => new Set(byId.get(sid)?.clientIds.keys() ?? [])`. + * Refcount is intentionally NOT exposed. + * + * **MUST return synchronously**. `mediator.request()` calls this + * inside the Promise executor with no `await`, per the N1 + * race-prevention invariant. An async implementation (returning + * `Promise>`) would defer the pending registration + * past the bridge's `publish → register → await` sequencing point and + * silently break a `forgetSession` racing with the issue path. + * + * **Forward-compat trap**: when the session was torn down between + * the bridge's `publish` and the mediator's `request` (extremely + * narrow race), the implementation should return an empty Set + * rather than throw. F3 v1's `first-responder` policy ignores the + * snapshot, so an empty set is harmless. Once Commit 4 lands + * `voteConsensus`, an empty `votersAtIssue` means EVERY vote on + * the request gets rejected for "not in voter set" — the request + * can only resolve via `forgetSession` cleanup or `permissionTimeoutMs`. + * The bridge's torn-down-session race is short enough that this is + * acceptable; document if a longer-window source of empty-voter + * snapshots emerges. + * + * **Late-joiner timing window** (wenshao review #4335 / 3271041469). + * The bridge sequence is `entry.events.publish(...)` → + * (synchronous) → `await mediator.request(record, ...)`. The + * publish is synchronous (`EventBus.publish` returns after fanning + * to in-memory subscriber queues, no event-loop yield) and the + * mediator's Promise executor is also synchronous through this + * call (N1 invariant), so a NEW HTTP client cannot register its + * `clientId` on `entry.clientIds` between publish and snapshot. + * However, an SSE subscriber that connected BEFORE the publish but + * has NOT yet hit any session route (no `X-Qwen-Client-Id` known + * to the bridge) will not appear in the snapshot — `consensus` + * silently rejects its later vote as `forbidden`. UIs that surface + * the active voter set (eligible-voters chip) should treat + * `permission_request` as the authoritative cutoff, not subsequent + * client-identity registrations. F3 v1 does not surface + * `votersAtIssue` to the wire; future PRs that add an + * `eligibleVoters[]` field on `permission_request.data` should + * source it from the same snapshot to keep client-side and + * server-side membership decisions aligned. + */ + votersForSession: (sessionId: string) => ReadonlySet; +} + +/** + * Pending permission record owned by the mediator. Uniform shape across + * all four policies — `tallies` and `votersAtIssue` are present even + * for non-consensus (empty in that case) so we don't need a discriminated + * union over `policy`. Memory cost is two empty containers per pending + * (~120 bytes), negligible against the per-session pending cap of 64. + */ +interface MediatorPending { + readonly requestId: string; + readonly sessionId: string; + /** Captured at request issue time so live-reload of the daemon + * policy doesn't change the rules under in-flight requests. */ + readonly policy: PermissionPolicy; + readonly originatorClientId: string | undefined; + readonly allowedOptionIds: ReadonlySet; + readonly issuedAtMs: number; + readonly timeoutMs: number; + /** Settles the Promise returned by `request()`. */ + readonly resolve: (resolution: PermissionResolution) => void; + /** Per-option vote sets for `consensus`; empty for other policies. */ + readonly tallies: Map>; + /** Snapshot of eligible voters for `consensus`; empty for others. */ + readonly votersAtIssue: ReadonlySet; + /** Mediator-internal — do not read or write from outside the class. */ + timer: ReturnType | undefined; + /** + * Wenshao review #4335 / 3271185594 — set to `true` once the + * `consensusQuorum` override cap has emitted its stderr + * breadcrumb for this pending so we don't repeat the line every + * time `consensusQuorumFor` is called within the same request. + */ + consensusQuorumCapNoted: boolean; +} + +interface PermissionResolutionRecord { + readonly requestId: string; + readonly sessionId: string; + readonly resolution: PermissionResolution; + /** Voter's clientId (or undefined for timeout / session-closed paths) + * — replayed onto `permission_already_resolved` so late SSE + * subscribers see the same `originatorClientId` the original + * `permission_resolved` carried (O8 wire compat). */ + readonly resolverClientId: string | undefined; +} + +/** + * Multi-client permission coordination implementation. + * + * Lifecycle: + * - `request(record, timeoutMs)` synchronously registers a pending + * entry inside the returned Promise's executor (no `await` before + * register — see N1 invariant in F3 plan) and arms the timeout. + * - `vote(vote)` dispatches by `entry.policy` and either resolves, + * records, rejects, or reports unknown. + * - `forgetSession(sessionId)` cancels every pending matching the + * session as `{kind:'cancelled', reason:'session_closed'}`. + * + * State is mediator-owned: `pending: Map` + * and `resolved: BoundedMap`. + * Outside callers (the bridge) keep ONLY `entry.pendingPermissionIds` + * for the per-session cap check; the mediator is the source of truth. + */ +export class MultiClientPermissionMediator implements PermissionMediator { + readonly policy: PermissionPolicy; + + private readonly deps: MediatorDeps; + private readonly pending = new Map(); + private readonly resolved = new Map(); + private readonly resolvedOrder: string[] = []; + /** + * Wenshao review #4335 / 3272493829 — dedup flag for the + * unanimity-required stderr breadcrumb. Without this, every + * permission request on a 2-client consensus session would emit + * an identical line (the unanimity condition is the NORMAL + * operating mode for M=2, not a rare edge); a busy session with + * many tool calls would produce dozens of duplicate stderr lines + * within seconds. One emit per mediator (= per daemon lifetime + * since the bridge constructs one) is enough to make the + * configuration visible without spam. + */ + private unanimityBreadcrumbEmitted = false; + + constructor(policy: PermissionPolicy, deps: MediatorDeps) { + this.policy = policy; + this.deps = deps; + } + + /** + * Register a fresh permission request from the agent. + * + * **Promise contract — once the Promise is returned, it never + * rejects.** All runtime failure modes (timeout, session closure, + * voter cancel, emit/audit publisher exceptions) are encoded as + * `PermissionResolution { kind:'cancelled', reason:... }`. + * Consumers can `await` the returned Promise and forward the + * result without a `.catch()` block. + * + * **Synchronous-throw exception** (DeepSeek review #4335 / + * 3271627444): when the agent's `allowedOptionIds` contains the + * cancel-vote sentinel string, this method throws + * `CancelSentinelCollisionError` synchronously BEFORE constructing + * the Promise. The synchronous shape is intentional — a + * never-settling Promise alongside a thrown error would be worse + * than a clean fail-fast — but callers must wrap this method + * itself in `try/catch` (or call it from an `async` function so + * the throw bubbles via the function's own Promise machinery). + * `bridgeClient.ts` currently has its own pre-check at the bridge + * layer; embedded callers must do the same. See `@throws` below. + * + * **N1 synchronous-register invariant**: pending entry, audit + * record, and timer setup all happen inside the Promise executor + * without `await`. The bridge's `publish → mediator.request → await` + * sequence relies on this — a `forgetSession` between publish and + * await would otherwise miss the new pending and leak it until + * timeout. + * + * @throws `CancelSentinelCollisionError` SYNCHRONOUSLY (not as a + * Promise rejection) if `record.allowedOptionIds` contains the + * cancel-vote sentinel string. This is a contract violation + * between agent and daemon and fails loudly at issue time + * rather than silently miscounting votes downstream. Callers + * inside an `async` function get the thrown error through the + * function's own Promise; synchronous callers must use + * `try/catch`. + */ + request( + record: PermissionRequestRecord, + timeoutMs: number, + ): Promise { + // Collision defense — fail loudly if an agent legitimately uses + // the sentinel string as an option label. Throws synchronously + // BEFORE constructing the Promise so the caller doesn't end up + // holding a never-settling Promise alongside a thrown error. + if (record.allowedOptionIds.has(CANCEL_VOTE_SENTINEL)) { + throw new CancelSentinelCollisionError( + record.requestId, + CANCEL_VOTE_SENTINEL, + ); + } + return new Promise((resolve) => { + // === BEGIN SYNCHRONOUS REGISTER (no awaits permitted) === + const policy = this.policy; + const votersAtIssue = this.deps.votersForSession(record.sessionId); + const pending: MediatorPending = { + requestId: record.requestId, + sessionId: record.sessionId, + policy, + originatorClientId: record.originatorClientId, + allowedOptionIds: record.allowedOptionIds, + issuedAtMs: record.issuedAtMs, + timeoutMs, + resolve, + tallies: new Map(), + votersAtIssue, + timer: undefined, + consensusQuorumCapNoted: false, + }; + this.pending.set(record.requestId, pending); + this.safeAudit(() => + this.deps.audit.recordRequested(record, policy, votersAtIssue), + ); + // Wenshao review #4335 / 3271185594 — when consensus is in + // force but the bridge captured zero eligible voters at + // issue time, the request can ONLY resolve via timeout (no + // vote will ever pass `votersAtIssue.has(clientId)`). Emit + // a stderr breadcrumb so operators don't have to derive that + // from "5 minutes of silence + permission_request frame". + // Doesn't change semantics; the timer still fires per the + // configured `permissionTimeoutMs`. + if (policy === 'consensus' && votersAtIssue.size === 0) { + try { + process.stderr.write( + `permissionMediator: consensus request ${record.requestId} ` + + `for session ${record.sessionId} issued with empty ` + + `votersAtIssue; can only resolve via permissionTimeoutMs ` + + `(${timeoutMs}ms)\n`, + ); + } catch { + // Stderr unavailable — silent drop. + } + } + // Wenshao review #4335 / 3271978356 — for even-sized voter + // sets the default formula `floor(M/2)+1` requires unanimity + // ONLY when M=2 (the practical surprise case); M=4 → quorum=3 + // is supermajority; M=6 → quorum=4 is supermajority too. The + // condition `floor(M/2)+1 === M` is true only for M=1 + // (single-voter; quorum=1 = M trivially) and M=2. + // + // Wenshao review #4335 / 3272493829 — dedup to one emit per + // mediator lifetime via `unanimityBreadcrumbEmitted`. Without + // this, a 2-client consensus session emits the line on EVERY + // permission request (unanimity is the M=2 normal operating + // mode, not a rare edge). The flag also ensures the line is + // visible at least once when the daemon boots into this + // configuration — operators see it on the first + // requestPermission and can ignore the dedup'd silence + // afterward. + if ( + policy === 'consensus' && + this.deps.consensusQuorum === undefined && + votersAtIssue.size >= 2 && + Math.floor(votersAtIssue.size / 2) + 1 === votersAtIssue.size && + !this.unanimityBreadcrumbEmitted + ) { + this.unanimityBreadcrumbEmitted = true; + try { + process.stderr.write( + `permissionMediator: consensus request ${record.requestId} ` + + `for session ${record.sessionId} requires unanimity ` + + `(votersAtIssue.size=${votersAtIssue.size}, default ` + + `quorum=floor(M/2)+1=${votersAtIssue.size}); split votes ` + + `will only resolve via permissionTimeoutMs (${timeoutMs}ms). ` + + `This breadcrumb fires once per mediator lifetime; ` + + `subsequent unanimity-required requests are silent.\n`, + ); + } catch { + // Stderr unavailable — silent drop. + } + } + if (timeoutMs > 0) { + pending.timer = setTimeout(() => { + // Timer fires asynchronously — guard against the entry + // already having been resolved by a vote OR replaced by a + // fresh request that reused the same requestId after LRU + // eviction. The identity check (`!== pending`) covers + // both cases — `this.pending.has(requestId)` would mistake + // a fresh request for a stale-timer fire on the old one. + if (this.pending.get(record.requestId) !== pending) return; + const firedAtMs = this.deps.now(); + // F3 Commit 2 — restore pre-F3 stderr breadcrumb (wenshao + // review #4335 / 3270622304). Pre-F3 wrote "timed out + // after Xms" directly to daemon stderr; F3 delegated to + // the audit publisher, but production audit can still be + // a no-op for embedded callers, so emit the breadcrumb + // here unconditionally. Wrapped in try/catch because + // process.stderr.write can synchronously throw on EPIPE + // (closed stderr) — losing observability is preferable + // to crashing the daemon's timer queue. + try { + process.stderr.write( + `qwen serve: permission ${record.requestId} ` + + `(session ${record.sessionId}) timed out after ${timeoutMs}ms\n`, + ); + } catch { + // Stderr unavailable — drop the breadcrumb and continue. + } + this.safeAudit(() => this.deps.audit.recordTimeout(record)); + this.resolveEntry( + pending, + { kind: 'cancelled', reason: 'timeout' }, + { + type: 'timeout', + issuedAtMs: pending.issuedAtMs, + timeoutMs: pending.timeoutMs, + firedAtMs, + }, + undefined, + ); + }, timeoutMs); + // Use a `'unref' in` guard rather than `as { unref?: ... }` + // so the type narrowing is enforced rather than asserted. + const t = pending.timer; + if ( + t !== undefined && + typeof t === 'object' && + 'unref' in t && + typeof (t as { unref: unknown }).unref === 'function' + ) { + (t as { unref: () => void }).unref(); + } + } + // === END SYNCHRONOUS REGISTER === + }); + } + + vote(vote: PermissionVote): PermissionVoteOutcome { + const pending = this.pending.get(vote.requestId); + + if (!pending) { + const prior = this.resolved.get(vote.requestId); + if (prior && prior.sessionId === vote.sessionId) { + // Re-emit `permission_already_resolved` so late SSE + // subscribers see the conclusion. C2 (Commit 3 review): + // pre-F3 `publishPermissionAlreadyResolved` did NOT stamp + // `originatorClientId` on this event. Preserve byte-for-byte + // — `httpAcpBridge.test.ts:2880` asserts + // `originatorClientId: undefined`. Resolver attribution lives + // in the audit log via `decisionReason.resolverClientId`, + // not on the wire frame. + const optionId = + prior.resolution.kind === 'option' + ? prior.resolution.optionId + : CANCEL_VOTE_SENTINEL; + this.safeEmit(prior.sessionId, { + type: 'permission_already_resolved', + data: { + requestId: prior.requestId, + sessionId: prior.sessionId, + outcome: this.toAcpOutcome(prior.resolution), + }, + }); + return { kind: 'already_resolved', resolvedOptionId: optionId }; + } + return { kind: 'unknown_request' }; + } + + if (pending.sessionId !== vote.sessionId) { + return { kind: 'unknown_request' }; + } + + // Voter cancel — bypasses policy dispatch; resolves cancelled + // regardless of who voted (the bridge already validated `clientId`). + if (vote.optionId === CANCEL_VOTE_SENTINEL) { + const outcome: PermissionVoteOutcome = { + kind: 'resolved', + resolvedOptionId: CANCEL_VOTE_SENTINEL, + }; + // Audit ordering invariant: `voted` before `resolved`. + this.safeAudit(() => + this.deps.audit.recordVoted(this.toRecord(pending), vote, outcome), + ); + this.resolveEntry( + pending, + { kind: 'cancelled', reason: 'agent_cancelled' }, + { + type: 'voter-cancelled', + resolverClientId: vote.clientId, + }, + vote.clientId, + ); + return outcome; + } + + // Validate optionId against the agent-declared allow set. Throws + // `InvalidPermissionOptionError`; the route layer maps to 400. + if (!pending.allowedOptionIds.has(vote.optionId)) { + throw new InvalidPermissionOptionError(vote.requestId, vote.optionId); + } + + // Per-policy handlers own their own audit.recordVoted call to + // preserve the `voted → resolved` ordering invariant (the + // resolveEntry call inside each handler is what triggers the + // `resolved` audit record). + switch (pending.policy) { + case 'first-responder': + return this.voteFirstResponder(pending, vote); + case 'designated': + return this.voteDesignated(pending, vote); + case 'consensus': + return this.voteConsensus(pending, vote); + case 'local-only': + return this.voteLocalOnly(pending, vote); + default: { + // Exhaustiveness — a future PermissionPolicy literal added + // without a case here will fail compilation at this line. + const _exhaustive: never = pending.policy; + void _exhaustive; + throw new Error(`Unknown permission policy "${pending.policy}"`); + } + } + } + + forgetSession(sessionId: string): void { + // Snapshot the keys to avoid mutating the Map during iteration. + const requestIds: string[] = []; + for (const [id, pending] of this.pending) { + if (pending.sessionId === sessionId) requestIds.push(id); + } + for (const id of requestIds) { + // Defensive — JS is single-threaded so today this re-lookup + // can't return a different entry, but resolveEntry's emit / + // audit calls fire synchronously and a future maintainer + // adding `await` or a re-entrant hook would invalidate the + // assumption. The Map.get is cheap insurance and not dead + // code if the loop body is ever modified. + const pending = this.pending.get(id); + if (!pending) continue; + this.resolveEntry( + pending, + { kind: 'cancelled', reason: 'session_closed' }, + { type: 'session-closed' }, + undefined, + ); + } + } + + /** + * Lookup the sessionId for a given requestId. Used by the legacy + * `bridge.respondToPermission(requestId, ...)` route which doesn't + * carry a sessionId in the URL. NOT part of the + * `PermissionMediator` interface contract — bridge holds the + * concrete class reference and calls this directly. + */ + peekSessionFor(requestId: string): string | undefined { + const pending = this.pending.get(requestId); + if (pending) return pending.sessionId; + const prior = this.resolved.get(requestId); + return prior?.sessionId; + } + + /** + * Daemon-wide in-flight pending count for diagnostics. The bridge + * exposes this through its `pendingPermissionCount` getter so + * operators can spot stuck FIFOs without reaching into mediator + * internals. NOT part of the `PermissionMediator` interface + * contract. + */ + get pendingCount(): number { + return this.pending.size; + } + + // =========================================================== + // Per-policy vote handlers + // =========================================================== + + private voteFirstResponder( + pending: MediatorPending, + vote: PermissionVote, + ): PermissionVoteOutcome { + // Bit-for-bit preservation of pre-F3 behavior: any validated voter + // (the route layer already enforced clientId / optionId / session + // ownership) wins immediately. + const outcome: PermissionVoteOutcome = { + kind: 'resolved', + resolvedOptionId: vote.optionId, + }; + // Ordering invariant: `voted` audit record fires BEFORE `resolved` + // (resolveEntry triggers `resolved`). + this.safeAudit(() => + this.deps.audit.recordVoted(this.toRecord(pending), vote, outcome), + ); + this.resolveEntry( + pending, + { kind: 'option', optionId: vote.optionId }, + { + type: 'first-responder', + resolverClientId: vote.clientId, + }, + vote.clientId, + ); + return outcome; + } + + private voteDesignated( + pending: MediatorPending, + vote: PermissionVote, + ): PermissionVoteOutcome { + // Anonymous prompt (originator undefined): fall back to + // first-responder. Documented relaxation — strict deployments + // must mandate `X-Qwen-Client-Id` on the prompt route. F3 plan + // §"Designated fallback" explains the rationale. + if (pending.originatorClientId === undefined) { + return this.voteFirstResponder(pending, vote); + } + if (vote.clientId !== pending.originatorClientId) { + // Reject — voter is not the prompt originator. + this.safeAudit(() => + this.deps.audit.recordForbidden( + this.toRecord(pending), + vote, + 'designated_mismatch', + ), + ); + this.safeEmit(pending.sessionId, { + type: 'permission_forbidden', + data: { + requestId: pending.requestId, + sessionId: pending.sessionId, + ...(vote.clientId !== undefined ? { clientId: vote.clientId } : {}), + reason: 'designated_mismatch', + }, + // N3 — new events stamp prompt originator (NOT voter). + ...(pending.originatorClientId !== undefined + ? { originatorClientId: pending.originatorClientId } + : {}), + }); + this.writeForbiddenStderr( + pending, + vote, + 'designated_mismatch (voter is not the prompt originator)', + ); + return { kind: 'forbidden', reason: 'designated_mismatch' }; + } + // Originator's vote — resolve immediately (semantically a + // first-responder for the designated voter). + const outcome: PermissionVoteOutcome = { + kind: 'resolved', + resolvedOptionId: vote.optionId, + }; + this.safeAudit(() => + this.deps.audit.recordVoted(this.toRecord(pending), vote, outcome), + ); + this.resolveEntry( + pending, + { kind: 'option', optionId: vote.optionId }, + { + type: 'designated-originator', + originatorClientId: pending.originatorClientId, + }, + vote.clientId, + ); + return outcome; + } + + private voteConsensus( + pending: MediatorPending, + vote: PermissionVote, + ): PermissionVoteOutcome { + // Voter must be in the issue-time snapshot. Anonymous voters + // and clients that connected AFTER the prompt issued are + // rejected. + // + // TODO(forward-compat): DeepSeek review #4335 / 3271627459 — the + // `designated_mismatch` reason code is overloaded here for "not + // in voter set" (consensus-specific) AND for "voter is not the + // prompt originator" (designated-policy semantics). Both cases + // surface the same string on the wire (`permission_forbidden` + // SSE) and the same audit reason. A future PR can split these + // into distinct reason codes (e.g. `voter_not_eligible`, + // `not_originator`) once an SDK consumer needs to disambiguate + // them — until then, F3 v1 keeps the overload to avoid + // protocol churn while semantics stabilize. + if ( + vote.clientId === undefined || + !pending.votersAtIssue.has(vote.clientId) + ) { + this.safeAudit(() => + this.deps.audit.recordForbidden( + this.toRecord(pending), + vote, + 'designated_mismatch', + ), + ); + this.safeEmit(pending.sessionId, { + type: 'permission_forbidden', + data: { + requestId: pending.requestId, + sessionId: pending.sessionId, + ...(vote.clientId !== undefined ? { clientId: vote.clientId } : {}), + reason: 'designated_mismatch', + }, + ...(pending.originatorClientId !== undefined + ? { originatorClientId: pending.originatorClientId } + : {}), + }); + this.writeForbiddenStderr( + pending, + vote, + 'designated_mismatch (voter not in consensus votersAtIssue snapshot)', + ); + return { kind: 'forbidden', reason: 'designated_mismatch' }; + } + + // Idempotent re-vote: if this clientId already cast a vote (any + // option), keep the original. Return `recorded` with the current + // votesNeeded; do NOT emit a partial_vote frame (the tally hasn't + // changed). + // + // Wenshao review #4335 / 3271041464 — the audit entry must + // reflect the ORIGINALLY-recorded optionId (the one in the + // tally), not the new attempt. Otherwise the audit ring shows + // `client_X voted for option_B` while the tally has client_X in + // option_A's bucket; an operator reading the ring would see a + // vote that never counted toward quorum. Look up the original + // option from the tally and substitute it into the audit + // record. + for (const [originalOptionId, set] of pending.tallies.entries()) { + if (vote.clientId !== undefined && set.has(vote.clientId)) { + const outcome: PermissionVoteOutcome = { + kind: 'recorded', + votesNeeded: this.votesNeededFor(pending), + }; + this.safeAudit(() => + this.deps.audit.recordVoted( + this.toRecord(pending), + { ...vote, optionId: originalOptionId }, + outcome, + ), + ); + return outcome; + } + } + + // Record the vote. + let bucket = pending.tallies.get(vote.optionId); + if (!bucket) { + bucket = new Set(); + pending.tallies.set(vote.optionId, bucket); + } + bucket.add(vote.clientId); + + const quorum = this.consensusQuorumFor(pending); + if (bucket.size >= quorum) { + const outcome: PermissionVoteOutcome = { + kind: 'resolved', + resolvedOptionId: vote.optionId, + }; + this.safeAudit(() => + this.deps.audit.recordVoted(this.toRecord(pending), vote, outcome), + ); + this.resolveEntry( + pending, + { kind: 'option', optionId: vote.optionId }, + { + type: 'consensus-quorum', + resolvedOptionId: vote.optionId, + quorum, + tally: bucket.size, + }, + vote.clientId, + ); + return outcome; + } + + // Recorded — emit partial_vote progress + return votesNeeded. + const outcome: PermissionVoteOutcome = { + kind: 'recorded', + votesNeeded: this.votesNeededFor(pending), + }; + this.safeAudit(() => + this.deps.audit.recordVoted(this.toRecord(pending), vote, outcome), + ); + this.safeEmit(pending.sessionId, { + type: 'permission_partial_vote', + data: { + requestId: pending.requestId, + sessionId: pending.sessionId, + votesReceived: this.totalTalliedFor(pending), + votesNeeded: outcome.votesNeeded, + quorum, + optionTallies: this.optionTalliesFor(pending), + }, + ...(pending.originatorClientId !== undefined + ? { originatorClientId: pending.originatorClientId } + : {}), + }); + return outcome; + } + + /** + * Vote dispatch for `local-only` policy: only `fromLoopback: true` + * voters can resolve a permission. + * + * **Cancel-sentinel asymmetry** (wenshao review #4335 / 3271978336). + * `vote()` recognizes the cancel sentinel BEFORE calling this + * method (cross-policy escape hatch — see the + * `CANCEL_VOTE_SENTINEL` JSDoc for the rationale), so a remote + * voter under `local-only` CAN abort a pending permission via + * `{outcome:'cancelled'}` even though they cannot RESOLVE one. The + * settings-side description for `local-only` and the F3 plan call + * out this gap explicitly. Operators who want strict-cancel-too + * semantics must (a) deploy a dedicated daemon process at + * loopback bind, OR (b) wait for the follow-up PR that lifts + * cancel into per-policy gating; F3 v1 keeps the current + * cross-policy cancel for consistency with first-responder / + * designated / consensus. + */ + private voteLocalOnly( + pending: MediatorPending, + vote: PermissionVote, + ): PermissionVoteOutcome { + if (!vote.fromLoopback) { + this.safeAudit(() => + this.deps.audit.recordForbidden( + this.toRecord(pending), + vote, + 'remote_not_allowed', + ), + ); + this.safeEmit(pending.sessionId, { + type: 'permission_forbidden', + data: { + requestId: pending.requestId, + sessionId: pending.sessionId, + ...(vote.clientId !== undefined ? { clientId: vote.clientId } : {}), + reason: 'remote_not_allowed', + }, + ...(pending.originatorClientId !== undefined + ? { originatorClientId: pending.originatorClientId } + : {}), + }); + this.writeForbiddenStderr( + pending, + vote, + 'remote_not_allowed (local-only policy; vote not from loopback)', + ); + return { kind: 'forbidden', reason: 'remote_not_allowed' }; + } + const outcome: PermissionVoteOutcome = { + kind: 'resolved', + resolvedOptionId: vote.optionId, + }; + this.safeAudit(() => + this.deps.audit.recordVoted(this.toRecord(pending), vote, outcome), + ); + this.resolveEntry( + pending, + { kind: 'option', optionId: vote.optionId }, + { + type: 'local-only-loopback', + resolverClientId: vote.clientId, + }, + vote.clientId, + ); + return outcome; + } + + // =========================================================== + // Consensus tally helpers + // =========================================================== + + /** + * Compute the quorum size for a `consensus` request. Default + * `floor(M/2) + 1` of `votersAtIssue.size`; overridden by + * `deps.consensusQuorum` when set, capped to `M` so an operator + * misconfig (N > M) can't deadlock. + * + * Wenshao review #4335 / 3271185594 — when the cap fires, write + * a one-time stderr breadcrumb per request so operators don't + * have to diff their `policy.consensusQuorum` against + * `votersAtIssue.size` manually to understand why a quorum + * resolved sooner than configured. Tracked on `MediatorPending` + * so the breadcrumb fires once even though `consensusQuorumFor` + * may be called multiple times per request (vote tally + final + * resolution). + */ + private consensusQuorumFor(pending: MediatorPending): number { + const m = pending.votersAtIssue.size; + const override = this.deps.consensusQuorum; + if (override !== undefined) { + const capped = Math.min(override, Math.max(m, 1)); + if (capped < override && !pending.consensusQuorumCapNoted) { + pending.consensusQuorumCapNoted = true; + try { + process.stderr.write( + `permissionMediator: consensusQuorum override ${override} ` + + `capped to ${capped} (votersAtIssue.size=${m}) for ` + + `request ${pending.requestId} session ${pending.sessionId}\n`, + ); + } catch { + // Stderr unavailable — silent drop. + } + } + return capped; + } + return Math.max(1, Math.floor(m / 2) + 1); + } + + private totalTalliedFor(pending: MediatorPending): number { + let total = 0; + for (const set of pending.tallies.values()) total += set.size; + return total; + } + + /** + * `votesNeeded` = `quorum - max(tally per option)`. When no + * option has any votes (degenerate; `permission_partial_vote` + * is only emitted AFTER the first vote, so this should never + * appear on the wire), returns `quorum` itself. Always ≥ 1 + * because the resolved-on-quorum path returns before this + * helper runs. + */ + private votesNeededFor(pending: MediatorPending): number { + const quorum = this.consensusQuorumFor(pending); + let max = 0; + for (const set of pending.tallies.values()) { + if (set.size > max) max = set.size; + } + return Math.max(quorum - max, 1); + } + + private optionTalliesFor(pending: MediatorPending): Record { + const out: Record = {}; + for (const [optionId, set] of pending.tallies) { + out[optionId] = set.size; + } + return out; + } + + // =========================================================== + // Resolution + cleanup + // =========================================================== + + /** + * Settle a pending entry. Cleanup order is hardened (N2 invariant): + * 1. clearTimeout (so a timer can never fire on a half-cleaned entry). + * 2. Delete from `pending` (state-first half — entry no longer + * reachable for new votes). + * 3. emit wire `permission_resolved` (best-effort — emit failures + * do not block the Promise settle). MUST come before step 4 + * so a re-entrant subscriber synchronously casting another + * vote during emit sees `pending === undefined && resolved + * === undefined` (silent false), matching pre-F3 ordering. + * See I5 (Commit 3 review) inline comment below. + * 4. write to `resolved` (the second half of state move — late + * voters arriving after this see `permission_already_resolved`). + * 5. audit.recordResolved (best-effort, same). + * 6. Settle the Promise (LAST — callbacks running re-entrantly + * see consistent state). + * + * Wenshao review #4335 / 3272581553 — pre-fix the spec bundled + * "delete pending + write resolved" into step 2 ahead of emit, + * which contradicted the code (and the I5 comment). The fix + * splits the two halves of the state move around the emit so + * the spec faithfully describes the ordering invariant. + * + * @param resolverClientId pre-F3 wire compat (O8 invariant): the + * `permission_resolved` SSE frame stamps this as + * `originatorClientId`. Pre-F3 `resolvePending` in + * `httpAcpBridge.ts:1518-1523` filled it from the voter's + * trusted clientId. We preserve byte-for-byte; vote-driven + * paths pass `vote.clientId` (which may be undefined for + * loopback no-header voters); timer + session-closed paths + * pass undefined (no voter). + */ + private resolveEntry( + pending: MediatorPending, + resolution: PermissionResolution, + decisionReason: PermissionDecisionReason | undefined, + resolverClientId: string | undefined, + ): void { + if (this.pending.get(pending.requestId) !== pending) { + // Already resolved on a different path (race between timer and + // a final vote arriving in the same tick). Idempotent no-op. + return; + } + if (pending.timer !== undefined) { + clearTimeout(pending.timer); + pending.timer = undefined; + } + this.pending.delete(pending.requestId); + // I5 (Commit 3 review) — emit the SSE `permission_resolved` BEFORE + // writing to the resolved-LRU. Pre-F3 ordered emit-then-LRU and a + // re-entrant subscriber synchronously casting another vote during + // emit would have seen `pending === undefined && resolved === + // undefined` (silent false). Reversing that order would let the + // re-entrant vote find the new LRU record and emit a redundant + // `permission_already_resolved`. Match pre-F3 ordering for + // wire-shape preservation. + this.safeEmit(pending.sessionId, { + type: 'permission_resolved', + data: { + requestId: pending.requestId, + outcome: this.toAcpOutcome(resolution), + }, + // O8 — preserve pre-F3 behavior: voter's clientId is stamped + // here (not the prompt originator's). Documented inconsistency + // with `permission_request.originatorClientId` (which IS the + // prompt originator); F3 does not fix the inconsistency to + // avoid breaking the wire shape. + ...(resolverClientId !== undefined + ? { originatorClientId: resolverClientId } + : {}), + }); + this.rememberResolved({ + requestId: pending.requestId, + sessionId: pending.sessionId, + resolution, + resolverClientId, + }); + if (decisionReason !== undefined) { + this.safeAudit(() => + this.deps.audit.recordResolved( + this.toRecord(pending), + resolution, + decisionReason, + ), + ); + } + pending.resolve(resolution); + } + + private rememberResolved(record: PermissionResolutionRecord): void { + if (!this.resolved.has(record.requestId)) { + this.resolvedOrder.push(record.requestId); + } + this.resolved.set(record.requestId, record); + while (this.resolvedOrder.length > MAX_RESOLVED_PERMISSION_RECORDS) { + const oldest = this.resolvedOrder.shift(); + if (oldest !== undefined) this.resolved.delete(oldest); + } + } + + private safeEmit( + sessionId: string, + event: Omit, + ): void { + try { + this.deps.emit(sessionId, event); + } catch (err) { + // Emit failures (bus closed mid-shutdown) never block settle. + // I4 (Commit 3 review) — surface as a stderr breadcrumb so + // silent regressions in the host's emit path (e.g. a future + // contract violation that throws instead of returning + // undefined) don't disappear unnoticed. + // + // Wenshao review #4335 / 3271041461 — the breadcrumb itself + // must be defensive. `process.stderr.write` can synchronously + // throw on EPIPE during daemon shutdown; if it does, the + // exception escapes `safeEmit` and propagates out of + // `resolveEntry`, leaving the pending Promise unsettled + // (request already deleted from `this.pending`). The agent + // would hang on `requestPermission` until the timeout fires. + // Mirror the timer callback's `try/catch` posture: losing + // observability is preferable to a stuck Promise. + try { + process.stderr.write( + `permissionMediator: emit failed for session=${JSON.stringify(sessionId)} type=${JSON.stringify(event.type)}: ${stringifyError(err)}\n`, + ); + } catch { + // Stderr unavailable — drop the breadcrumb and continue. + } + } + } + + /** + * DeepSeek review #4335 / 3271627457 — emit a stderr breadcrumb + * for every vote rejection (the three forbidden paths in + * voteDesignated / voteConsensus / voteLocalOnly). Mirrors the + * timeout breadcrumb pattern: audit ring + SSE event are + * transient observability surfaces (no v1 query route, SSE drops + * on disconnect), so an operator tailing daemon stderr would see + * zero indication of permission rejections without this. + * + * Wrapped in `try/catch` because `process.stderr.write` can + * synchronously throw on EPIPE during shutdown — a stderr + * unavailability must not propagate up through `safeEmit` / + * `safeAudit` and break the resolveEntry cleanup ladder. Mirrors + * the safeEmit/safeAudit defensive posture (see wenshao review + * #4335 / 3271041461 for the matching hang scenario). + */ + private writeForbiddenStderr( + pending: MediatorPending, + vote: PermissionVote, + reasonDetail: string, + ): void { + try { + const voterDescriptor = + vote.clientId === undefined + ? '' + : JSON.stringify(vote.clientId); + process.stderr.write( + `qwen serve: permission ${pending.requestId} ` + + `(session ${pending.sessionId}): vote rejected ` + + `(${reasonDetail}) by client ${voterDescriptor}\n`, + ); + } catch { + // Stderr unavailable — drop the breadcrumb and continue. + } + } + + /** + * Run an audit-publisher call defensively. The audit ring is + * best-effort observability — a publisher exception (ring full, + * host bug, transient I/O) MUST NOT throw out of `request()`, + * `vote()`, or the timer callback. Without this guard, the + * Promise the agent is awaiting would be left unsettled and the + * pending entry would leak. + * + * Single helper used at all five audit call sites so the + * "audit is best-effort" invariant is uniformly enforced (the + * pre-fix asymmetric `try/catch` at 2 of 5 sites was a real + * silent-failure hole; see Commit 1 review notes). + * + * Wenshao review #4335 / 3272567323 — JSDoc was previously + * stacked above `writeForbiddenStderr` so IDE hover and API + * doc generation showed the wrong attribution. Moved adjacent + * to its actual definition. + */ + private safeAudit(fn: () => void): void { + try { + fn(); + } catch (err) { + // Wenshao review #4335 / 3271041461 — see the matching + // try/catch on the breadcrumb in `safeEmit`. The audit-failure + // breadcrumb must not itself crash the safe wrapper, or the + // `resolveEntry` cleanup ladder could leave the pending + // Promise unsettled. + try { + process.stderr.write( + `permissionMediator: audit publisher threw: ${stringifyError(err)}\n`, + ); + } catch { + // Stderr unavailable — drop the breadcrumb and continue. + } + } + } + + private toRecord(pending: MediatorPending): PermissionRequestRecord { + return { + requestId: pending.requestId, + sessionId: pending.sessionId, + originatorClientId: pending.originatorClientId, + allowedOptionIds: pending.allowedOptionIds, + issuedAtMs: pending.issuedAtMs, + }; + } + + private toAcpOutcome( + resolution: PermissionResolution, + ): { outcome: 'selected'; optionId: string } | { outcome: 'cancelled' } { + if (resolution.kind === 'option') { + return { outcome: 'selected', optionId: resolution.optionId }; + } + return { outcome: 'cancelled' }; + } +} diff --git a/packages/cli/src/config/settingsSchema.ts b/packages/cli/src/config/settingsSchema.ts index a3e39cd7fe8..55802089ea0 100644 --- a/packages/cli/src/config/settingsSchema.ts +++ b/packages/cli/src/config/settingsSchema.ts @@ -1668,6 +1668,82 @@ const SETTINGS_SCHEMA = { }, }, + policy: { + type: 'object', + label: 'Daemon Policy', + category: 'Daemon', + requiresRestart: true, + default: {}, + description: + 'Daemon multi-client coordination policies. Tool-level allow/deny rules ' + + 'live under `permissions`; this section is for runtime mediation behavior ' + + 'between concurrent HTTP clients sharing one `qwen serve` daemon.', + showInDialog: false, + properties: { + permissionStrategy: { + type: 'enum', + label: 'Permission Mediation Policy', + category: 'Daemon', + requiresRestart: true, + default: 'first-responder', + description: + 'How permission requests resolve when multiple clients are attached. ' + + '`first-responder` (default) = any client decides, first wins. ' + + '`designated` = only the prompt originator decides; falls back to ' + + 'first-responder if originator is anonymous. ' + + 'NOTE: client identity comes from self-declared X-Qwen-Client-Id ' + + 'with no proof-of-possession (pair-token mechanism is a future PR), ' + + 'so any client observing originatorClientId on SSE frames can ' + + 'register with the same id and impersonate the originator. ' + + '`consensus` = N-of-M voters must agree. Default N=floor(M/2)+1, ' + + 'which means UNANIMITY for M=2 (quorum=2, both must agree) and ' + + 'supermajority for larger even M (M=4 → quorum=3; M=6 → quorum=4). ' + + 'For M=2 specifically, split votes resolve only via permissionTimeoutMs. ' + + '`local-only` = only loopback clients can RESOLVE; remote clients ' + + 'can still ABORT a pending permission via the cancel sentinel ' + + '({outcome:"cancelled"}) — F3 v1 keeps cancel cross-policy for ' + + 'consistency. Strict-cancel-too deployments need a dedicated ' + + 'loopback-bound daemon. ' + + 'Requires daemon restart — F3 v1 reads this once at boot.', + showInDialog: true, + options: [ + { value: 'first-responder', label: 'First Responder' }, + { value: 'designated', label: 'Designated Originator' }, + { value: 'consensus', label: 'Consensus Quorum' }, + { value: 'local-only', label: 'Local Only' }, + ], + }, + consensusQuorum: { + type: 'number', + label: 'Consensus Quorum Override', + category: 'Daemon', + requiresRestart: true, + default: undefined as number | undefined, + description: + 'Optional fixed quorum size for consensus policy. Capped at M ' + + '(count of registered voters at request issue time) to prevent ' + + 'unreachable quorum. Unset = floor(M/2)+1. ' + + 'Requires daemon restart — F3 v1 reads this once at boot.', + showInDialog: false, + // Wenshao review #4335 / 3271185604 — runQwenServe.ts validates + // `Number.isInteger(n) && n >= 1` and refuses to boot otherwise. + // Override the generated schema so IDE inline validation + // (VSCode, JetBrains via JSON Schema) flags `0`, `-1`, `1.5` + // BEFORE the user restarts the daemon. The bare `type:'number'` + // mapping accepts all of these. + jsonSchemaOverride: { + type: 'integer', + minimum: 1, + description: + 'Optional fixed quorum size for consensus policy. Capped at M ' + + '(count of registered voters at request issue time) to prevent ' + + 'unreachable quorum. Unset = floor(M/2)+1. ' + + 'Requires daemon restart — F3 v1 reads this once at boot.', + }, + }, + }, + }, + mcp: { type: 'object', label: 'MCP', diff --git a/packages/cli/src/serve/capabilities.ts b/packages/cli/src/serve/capabilities.ts index 1509dc6b520..d43c46266d9 100644 --- a/packages/cli/src/serve/capabilities.ts +++ b/packages/cli/src/serve/capabilities.ts @@ -180,6 +180,17 @@ export const SERVE_CAPABILITY_REGISTRY = { // status route (extension data on `/capabilities` would inflate the // descriptor shape; we keep the registry uniform). auth_device_flow: { since: 'v1' }, + // #4175 F3 (Commit 6). Daemon advertises which permission mediation + // policies it can run. Clients introspect `modes` to discover the + // closed set of strategies before relying on `permission_partial_vote` + // / `permission_forbidden` SSE events. The active policy for THIS + // daemon is exposed in the `/capabilities` envelope's + // `policy.permission` field — the mode list here is the + // build-supported set, distinct from runtime configuration. + permission_mediation: { + since: 'v1', + modes: ['first-responder', 'designated', 'consensus', 'local-only'], + }, } as const satisfies Record; export type ServeFeature = keyof typeof SERVE_CAPABILITY_REGISTRY; diff --git a/packages/cli/src/serve/httpAcpBridge.test.ts b/packages/cli/src/serve/httpAcpBridge.test.ts index 28898d70a51..617b6e33842 100644 --- a/packages/cli/src/serve/httpAcpBridge.test.ts +++ b/packages/cli/src/serve/httpAcpBridge.test.ts @@ -2710,6 +2710,117 @@ describe('createHttpAcpBridge', () => { await bridge.shutdown(); }); + it('returns false (not InvalidClientIdError) when session exists but requestId is unknown and clientId is unregistered (#4335 / 3271978329 / 3272493792 / 3273077272)', async () => { + // Wenshao review #4335 / 3271978329 (Critical) — error + // precedence regression: the session-scoped vote route must + // return `false` (→ 404) when the requestId isn't known to + // the mediator, BEFORE validating `context.clientId`. + // Without this guard a probe could fabricate a requestId, + // supply an arbitrary `X-Qwen-Client-Id`, and distinguish + // "this clientId is registered to this session" (proceeds + // past resolveTrustedClientId then returns false → 404) from + // "this clientId is not registered" (InvalidClientIdError → + // 400) — a session-membership oracle. + // + // Wenshao review #4335 / 3272493792 — explicit test for the + // fix from Round 7 so a future refactor can't silently + // remove the short-circuit. + // + // Wenshao review #4335 / 3273077272 — also assert the stderr + // breadcrumb that Round 8 promoted from debug-gated to + // unconditional (`writeStderrLine`). Pinning the log call + // means a future refactor that drops or downgrades the line + // is caught even when the return value still happens to be + // false for some other reason. + const stderrSpy = vi + .spyOn(process.stderr, 'write') + .mockImplementation(() => true); + try { + const { bridge, session } = await setupForPermission(); + + // Session exists, requestId is unknown, clientId is fake. + // The bridge MUST return false; pre-fix it threw + // InvalidClientIdError (400). + const result = bridge.respondToSessionPermission( + session.sessionId, + 'unknown-req-id', + { outcome: { outcome: 'cancelled' } }, + { clientId: 'fabricated-client-id' }, + ); + expect(result).toBe(false); + expect(stderrSpy).toHaveBeenCalledWith( + expect.stringContaining('rejected permission vote'), + ); + expect(stderrSpy).toHaveBeenCalledWith( + expect.stringContaining('unknown-req-id'), + ); + + await bridge.shutdown(); + } finally { + stderrSpy.mockRestore(); + } + }); + + it('rejects cancel sentinel injection via {selected,"__cancelled__"} (#4335 / 3271420267)', async () => { + // wenshao/qwen-latest review #4335 (3271420267) — the most + // security-critical guard in this PR. The mediator recognizes + // CANCEL_VOTE_SENTINEL ('__cancelled__') BEFORE validating the + // option against allowedOptionIds, so a wire client sending + // `{outcome:'selected', optionId:'__cancelled__'}` could + // bypass ALL policy dispatch (designated/consensus/local-only) + // and resolve the request as cancelled. The bridge guards + // against this by throwing InvalidPermissionOptionError + // BEFORE forwarding to mediator.vote — without a test, a + // future refactor could silently remove the check. + const { bridge, session, conn } = await setupForPermission(); + const subAbort = new AbortController(); + const iter = bridge.subscribeEvents(session.sessionId, { + signal: subAbort.signal, + }); + const respPromise = ( + conn as unknown as { + requestPermission(p: unknown): Promise; + } + ).requestPermission({ + sessionId: session.sessionId, + toolCall: { toolCallId: 'tc-1', title: 'rm -rf /' }, + options: [ + { optionId: 'allow', name: 'Allow', kind: 'allow_once' }, + { optionId: 'deny', name: 'Deny', kind: 'reject_once' }, + ], + }); + const it = iter[Symbol.asyncIterator](); + const next = await it.next(); + const payload = next.value!.data as { requestId: string }; + + // Wire-injected sentinel via `selected` outcome — must + // throw InvalidPermissionOptionError before reaching the + // mediator. + expect(() => + bridge.respondToSessionPermission( + session.sessionId, + payload.requestId, + { + outcome: { outcome: 'selected', optionId: '__cancelled__' }, + }, + ), + ).toThrow(InvalidPermissionOptionError); + + // Pending was preserved — a legitimate vote still resolves. + expect(bridge.pendingPermissionCount).toBe(1); + bridge.respondToSessionPermission(session.sessionId, payload.requestId, { + outcome: { outcome: 'selected', optionId: 'allow' }, + }); + const response = (await respPromise) as { + outcome: { outcome: string; optionId?: string }; + }; + expect(response.outcome.outcome).toBe('selected'); + expect(response.outcome.optionId).toBe('allow'); + + subAbort.abort(); + await bridge.shutdown(); + }); + it('rejects votes whose optionId was not in the agent-offered set (BkwQI)', async () => { // BkwQI: bridge.respondToPermission validates the voter's // `optionId` against the original `options` the agent sent. @@ -3104,13 +3215,28 @@ describe('createHttpAcpBridge', () => { await bridge.shutdown(); }); - it('rejects unknown permission votes with unregistered client ids', async () => { + it('returns false uniformly for unknown permission votes regardless of clientId registration (#4335 / 3272493777)', async () => { + // Wenshao review #4335 / 3272493777 — error precedence: an + // unknown requestId must return `false` (→ 404) regardless of + // whether the supplied `clientId` is registered in any + // session. The previous PR #4231 boundary returned 400 for + // unregistered clientIds and 404 for registered ones, which + // turned out to be a cross-session client-registration + // oracle: a remote prober posting `POST /permission/` + // with various `X-Qwen-Client-Id` headers could distinguish + // "this clientId is registered in some active session" (404) + // from "not registered anywhere" (400). The session-scoped + // route's matching fix landed in Round 7 (#3271978329); this + // pins the symmetric posture for the legacy route and + // explicitly inverts the assertion the pre-Round-7 test used + // to make. const bridge = makeBridge({ channelFactory: async () => makeChannel().channel, }); const session = await bridge.spawnOrAttach({ workspaceCwd: WS_A }); - expect(() => + // Unregistered clientId — must NOT throw; uniform `false`. + expect( bridge.respondToPermission( 'does-not-exist', { @@ -3118,7 +3244,8 @@ describe('createHttpAcpBridge', () => { }, { clientId: 'client-not-issued' }, ), - ).toThrow(InvalidClientIdError); + ).toBe(false); + // Registered clientId — also `false`. expect( bridge.respondToPermission( 'does-not-exist', @@ -3128,6 +3255,12 @@ describe('createHttpAcpBridge', () => { { clientId: session.clientId }, ), ).toBe(false); + // No clientId at all — `false` (unchanged behavior). + expect( + bridge.respondToPermission('does-not-exist', { + outcome: { outcome: 'cancelled' }, + }), + ).toBe(false); await bridge.shutdown(); }); @@ -6691,3 +6824,38 @@ describe('createHttpAcpBridge', () => { }); }); }); + +// ============================================================ +// F3 Commit 8 — bridge-level integration for the multi-client +// permission mediator. Mediator unit tests cover strategy logic +// (35 tests in `permissionMediator.test.ts`); these exercise the +// HTTP-bridge surface specifically: +// - `bridge.permissionPolicy` accessor wired through the mediator +// - F3 BridgeOptions validation (positive-integer quorum) +// ============================================================ +describe('createHttpAcpBridge — F3 multi-client permission coordination', () => { + it('exposes the active permission policy through bridge.permissionPolicy (default first-responder)', () => { + const bridge = makeBridge({}); + expect(bridge.permissionPolicy).toBe('first-responder'); + }); + + it('reflects the configured policy when BridgeOptions.permissionPolicy is set', () => { + const bridge = makeBridge({ permissionPolicy: 'consensus' }); + expect(bridge.permissionPolicy).toBe('consensus'); + }); + + it('throws on non-positive-integer permissionConsensusQuorum', () => { + expect(() => + makeBridge({ + permissionPolicy: 'consensus', + permissionConsensusQuorum: 0, + }), + ).toThrow(/positive integer/); + expect(() => + makeBridge({ + permissionPolicy: 'consensus', + permissionConsensusQuorum: 1.5, + }), + ).toThrow(/positive integer/); + }); +}); diff --git a/packages/cli/src/serve/httpAcpBridge.ts b/packages/cli/src/serve/httpAcpBridge.ts index 120eebbc38f..c1127da600e 100644 --- a/packages/cli/src/serve/httpAcpBridge.ts +++ b/packages/cli/src/serve/httpAcpBridge.ts @@ -39,15 +39,13 @@ export { createHttpAcpBridge } from '@qwen-code/acp-bridge/bridge'; export { defaultSpawnChannelFactory } from '@qwen-code/acp-bridge/spawnChannel'; -export { - BridgeClient, - MAX_RESOLVED_PERMISSION_RECORDS, -} from '@qwen-code/acp-bridge/bridgeClient'; -export type { - BridgeClientSessionEntry, - PendingPermission, - PermissionResolutionRecord, -} from '@qwen-code/acp-bridge/bridgeClient'; +// Wenshao review #4335 / 3272581548 — `MAX_RESOLVED_PERMISSION_RECORDS`, +// `PendingPermission`, `PermissionResolutionRecord` re-exports +// removed alongside the source definitions in bridgeClient.ts. +// The mediator now owns pending+resolved state and declares its own +// (differently-shaped) cap + record types in `permissionMediator.ts`. +export { BridgeClient } from '@qwen-code/acp-bridge/bridgeClient'; +export type { BridgeClientSessionEntry } from '@qwen-code/acp-bridge/bridgeClient'; export type { AcpChannel, @@ -91,6 +89,10 @@ export { WorkspaceInitRaceError, McpServerNotFoundError, McpServerRestartFailedError, + // #4175 F3 — multi-client permission coordination errors. + CancelSentinelCollisionError, + PermissionForbiddenError, + PermissionPolicyNotImplementedError, } from '@qwen-code/acp-bridge/bridgeErrors'; export { diff --git a/packages/cli/src/serve/permissionAudit.test.ts b/packages/cli/src/serve/permissionAudit.test.ts new file mode 100644 index 00000000000..5e082201830 --- /dev/null +++ b/packages/cli/src/serve/permissionAudit.test.ts @@ -0,0 +1,231 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect } from 'vitest'; +import { + createPermissionAuditPublisher, + DEFAULT_PERMISSION_AUDIT_RING_SIZE, + PermissionAuditRing, +} from './permissionAudit.js'; +import type { + PermissionRequestRecord, + PermissionVote, +} from '@qwen-code/acp-bridge/permission'; + +const RECORD: PermissionRequestRecord = { + requestId: 'req-1', + sessionId: 'sess-1', + originatorClientId: 'client_A', + allowedOptionIds: new Set(['proceed_once']), + issuedAtMs: 1_000_000, +}; + +const VOTE: PermissionVote = { + requestId: 'req-1', + sessionId: 'sess-1', + clientId: 'client_A', + optionId: 'proceed_once', + receivedAtMs: 1_000_010, + fromLoopback: false, +}; + +describe('PermissionAuditRing', () => { + it('has the documented default capacity', () => { + const ring = new PermissionAuditRing(); + expect(ring.capacity).toBe(DEFAULT_PERMISSION_AUDIT_RING_SIZE); + expect(ring.size).toBe(0); + }); + + it('throws on non-positive-integer capacity', () => { + expect(() => new PermissionAuditRing(0)).toThrow(/positive integer/); + expect(() => new PermissionAuditRing(-1)).toThrow(/positive integer/); + expect(() => new PermissionAuditRing(1.5)).toThrow(/positive integer/); + expect(() => new PermissionAuditRing(Number.NaN)).toThrow( + /positive integer/, + ); + }); + + it('evicts the oldest entry on overflow (FIFO)', () => { + const ring = new PermissionAuditRing(3); + const publisher = createPermissionAuditPublisher({ + ring, + now: () => 0, + }); + publisher.recordRequested( + { ...RECORD, requestId: 'req-1' }, + 'first-responder', + new Set(), + ); + publisher.recordRequested( + { ...RECORD, requestId: 'req-2' }, + 'first-responder', + new Set(), + ); + publisher.recordRequested( + { ...RECORD, requestId: 'req-3' }, + 'first-responder', + new Set(), + ); + expect(ring.size).toBe(3); + + publisher.recordRequested( + { ...RECORD, requestId: 'req-4' }, + 'first-responder', + new Set(), + ); + expect(ring.size).toBe(3); + const ids = ring.snapshot().map((e) => e.requestId); + expect(ids).toEqual(['req-2', 'req-3', 'req-4']); + }); + + it('snapshot honors limit and rejects invalid limits', () => { + const ring = new PermissionAuditRing(); + const publisher = createPermissionAuditPublisher({ ring, now: () => 0 }); + for (let i = 0; i < 10; i++) { + publisher.recordRequested( + { ...RECORD, requestId: `req-${i}` }, + 'first-responder', + new Set(), + ); + } + expect(ring.snapshot(3).map((e) => e.requestId)).toEqual([ + 'req-7', + 'req-8', + 'req-9', + ]); + expect(ring.snapshot().length).toBe(10); + expect(() => ring.snapshot(-1)).toThrow(); + expect(() => ring.snapshot(1.5)).toThrow(); + }); + + it('snapshotForSession filters by sessionId', () => { + const ring = new PermissionAuditRing(); + const publisher = createPermissionAuditPublisher({ ring, now: () => 0 }); + publisher.recordRequested( + { ...RECORD, sessionId: 'A' }, + 'first-responder', + new Set(), + ); + publisher.recordRequested( + { ...RECORD, sessionId: 'B' }, + 'first-responder', + new Set(), + ); + publisher.recordRequested( + { ...RECORD, sessionId: 'A' }, + 'first-responder', + new Set(), + ); + expect(ring.snapshotForSession('A').length).toBe(2); + expect(ring.snapshotForSession('B').length).toBe(1); + expect(ring.snapshotForSession('C').length).toBe(0); + }); +}); + +describe('createPermissionAuditPublisher', () => { + it('writes the requested entry shape with timestamp and votersAtIssue', () => { + const ring = new PermissionAuditRing(); + const publisher = createPermissionAuditPublisher({ + ring, + now: () => 12_345, + }); + publisher.recordRequested( + RECORD, + 'consensus', + new Set(['client_A', 'client_B']), + ); + const [entry] = ring.snapshot(); + expect(entry).toMatchObject({ + kind: 'permission.requested', + recordedAtMs: 12_345, + requestId: 'req-1', + sessionId: 'sess-1', + originatorClientId: 'client_A', + policy: 'consensus', + issuedAtMs: 1_000_000, + }); + expect(entry!.kind).toBe('permission.requested'); + if (entry!.kind === 'permission.requested') { + expect(Array.from(entry.votersAtIssue).sort()).toEqual([ + 'client_A', + 'client_B', + ]); + expect(entry.allowedOptionIds).toEqual(['proceed_once']); + } + }); + + it('writes the voted entry with vote shape and outcome', () => { + const ring = new PermissionAuditRing(); + const publisher = createPermissionAuditPublisher({ ring, now: () => 0 }); + publisher.recordVoted(RECORD, VOTE, { + kind: 'resolved', + resolvedOptionId: 'proceed_once', + }); + const [entry] = ring.snapshot(); + expect(entry).toMatchObject({ + kind: 'permission.voted', + requestId: 'req-1', + sessionId: 'sess-1', + clientId: 'client_A', + optionId: 'proceed_once', + fromLoopback: false, + receivedAtMs: 1_000_010, + outcome: { kind: 'resolved', resolvedOptionId: 'proceed_once' }, + }); + }); + + it('writes the forbidden entry with the reason', () => { + const ring = new PermissionAuditRing(); + const publisher = createPermissionAuditPublisher({ ring, now: () => 0 }); + publisher.recordForbidden(RECORD, VOTE, 'designated_mismatch'); + const [entry] = ring.snapshot(); + expect(entry).toMatchObject({ + kind: 'permission.forbidden', + requestId: 'req-1', + sessionId: 'sess-1', + clientId: 'client_A', + reason: 'designated_mismatch', + }); + }); + + it('writes the resolved entry with structured decisionReason', () => { + const ring = new PermissionAuditRing(); + const publisher = createPermissionAuditPublisher({ ring, now: () => 0 }); + publisher.recordResolved( + RECORD, + { kind: 'option', optionId: 'proceed_once' }, + { type: 'first-responder', resolverClientId: 'client_A' }, + ); + const [entry] = ring.snapshot(); + expect(entry).toMatchObject({ + kind: 'permission.resolved', + requestId: 'req-1', + sessionId: 'sess-1', + resolution: { kind: 'option', optionId: 'proceed_once' }, + decisionReason: { + type: 'first-responder', + resolverClientId: 'client_A', + }, + }); + }); + + it('writes the timeout entry without resolution data', () => { + const ring = new PermissionAuditRing(); + const publisher = createPermissionAuditPublisher({ + ring, + now: () => 5_555, + }); + publisher.recordTimeout(RECORD); + const [entry] = ring.snapshot(); + expect(entry).toMatchObject({ + kind: 'permission.timeout', + recordedAtMs: 5_555, + requestId: 'req-1', + sessionId: 'sess-1', + issuedAtMs: 1_000_000, + }); + }); +}); diff --git a/packages/cli/src/serve/permissionAudit.ts b/packages/cli/src/serve/permissionAudit.ts new file mode 100644 index 00000000000..e0b9e6fbf78 --- /dev/null +++ b/packages/cli/src/serve/permissionAudit.ts @@ -0,0 +1,262 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Permission audit ring. F3 Commit 4 (#4175). + * + * Writes 5 record types (`permission.requested` / `permission.voted` / + * `permission.forbidden` / `permission.resolved` / `permission.timeout`) + * to an in-memory bounded FIFO ring buffer on the bridge — oldest + * record evicts on overflow regardless of access pattern. NOT + * routed onto the per-session SSE bus — audit and wire events are + * intentionally separate channels per the F3 plan. + * + * v1 does not expose a `GET /workspace/permission/audit` route — the + * ring is held inside `createHttpAcpBridge`'s closure for future query + * infrastructure. This file provides the writer; the bridge factory + * constructs the ring (only when `BridgeOptions.permissionAudit` is + * omitted; a host-supplied publisher takes the ring's place) and + * wires it to the publisher. The ring is NOT exposed on the + * `HttpAcpBridge` interface today — a follow-up PR adding + * `GET /workspace/permission/audit` will need to surface it via a new + * accessor or pass it through `BridgeOptions`. + * + * Contract: every `record*` method MAY throw (the underlying ring is + * synchronous and resilient by design, but the mediator's `safeAudit` + * wrapper guarantees throws never block Promise settle). + */ + +import type { + PermissionAuditPublisher, + PermissionDecisionReason, +} from '@qwen-code/acp-bridge'; +import type { + PermissionPolicy, + PermissionRequestRecord, + PermissionResolution, + PermissionVote, + PermissionVoteOutcome, +} from '@qwen-code/acp-bridge/permission'; + +/** + * Default capacity of the audit ring. Mirrors the pre-F3 + * `MAX_RESOLVED_PERMISSION_RECORDS` (512); each row is small (≤1KB + * with field names + UUID + decision reason) so 512 entries stays + * well under 100 KB. Operators can override at construction time. + */ +export const DEFAULT_PERMISSION_AUDIT_RING_SIZE = 512; + +/** + * Common shape for every audit row. The discriminator `kind` mirrors + * the publisher method names; consumers `switch` on it to project + * onto a per-row payload. + */ +export type PermissionAuditEntry = + | { + readonly kind: 'permission.requested'; + readonly recordedAtMs: number; + readonly requestId: string; + readonly sessionId: string; + readonly originatorClientId: string | undefined; + readonly policy: PermissionPolicy; + readonly votersAtIssue: readonly string[]; + readonly issuedAtMs: number; + readonly allowedOptionIds: readonly string[]; + } + | { + readonly kind: 'permission.voted'; + readonly recordedAtMs: number; + readonly requestId: string; + readonly sessionId: string; + readonly clientId: string | undefined; + readonly optionId: string; + readonly fromLoopback: boolean; + readonly receivedAtMs: number; + readonly outcome: PermissionVoteOutcome; + } + | { + readonly kind: 'permission.forbidden'; + readonly recordedAtMs: number; + readonly requestId: string; + readonly sessionId: string; + readonly clientId: string | undefined; + readonly optionId: string; + readonly fromLoopback: boolean; + readonly reason: 'designated_mismatch' | 'remote_not_allowed'; + } + | { + readonly kind: 'permission.resolved'; + readonly recordedAtMs: number; + readonly requestId: string; + readonly sessionId: string; + readonly resolution: PermissionResolution; + readonly decisionReason: PermissionDecisionReason; + } + | { + readonly kind: 'permission.timeout'; + readonly recordedAtMs: number; + readonly requestId: string; + readonly sessionId: string; + readonly issuedAtMs: number; + }; + +/** + * Bounded ring buffer for permission audit entries. Operates as a + * FIFO: when the ring is full, the oldest entry is evicted to make + * room for the newest. Two consumers: + * 1. `PermissionAuditPublisher` writes via `push()`. + * 2. (future) `GET /workspace/permission/audit` reads via + * `snapshot(limit?)` / `snapshotForSession(sessionId, limit?)`. + */ +export class PermissionAuditRing { + private readonly buf: PermissionAuditEntry[] = []; + private readonly cap: number; + + constructor(capacity: number = DEFAULT_PERMISSION_AUDIT_RING_SIZE) { + if (!Number.isInteger(capacity) || capacity < 1) { + throw new Error( + `PermissionAuditRing capacity must be a positive integer; got ${String(capacity)}`, + ); + } + this.cap = capacity; + } + + push(entry: PermissionAuditEntry): void { + this.buf.push(entry); + while (this.buf.length > this.cap) this.buf.shift(); + } + + /** Snapshot the most-recent `limit` entries (or all if omitted). */ + snapshot(limit?: number): readonly PermissionAuditEntry[] { + if (limit === undefined) return this.buf.slice(); + if (!Number.isInteger(limit) || limit < 0) { + throw new Error( + `PermissionAuditRing.snapshot limit must be a non-negative integer; got ${String(limit)}`, + ); + } + return this.buf.slice(Math.max(0, this.buf.length - limit)); + } + + /** Subset filtered by sessionId. */ + snapshotForSession( + sessionId: string, + limit?: number, + ): readonly PermissionAuditEntry[] { + const all = this.buf.filter((e) => e.sessionId === sessionId); + if (limit === undefined) return all; + if (!Number.isInteger(limit) || limit < 0) { + throw new Error( + `PermissionAuditRing.snapshotForSession limit must be a non-negative integer; got ${String(limit)}`, + ); + } + return all.slice(Math.max(0, all.length - limit)); + } + + /** Current entry count (≤ capacity). For diagnostics. */ + get size(): number { + return this.buf.length; + } + + /** Configured capacity. */ + get capacity(): number { + return this.cap; + } +} + +/** + * Build a `PermissionAuditPublisher` whose 5 `record*` methods push + * a typed `PermissionAuditEntry` into the supplied ring. + * + * Modeled on `createAuditPublisher` in + * `packages/cli/src/serve/fs/audit.ts:237` — same DI shape (single + * deps object), same Omit-hidden internal fields synthesis, but + * writes to a ring rather than the SSE bus. + * + * The optional `now` allows tests to inject a deterministic + * wallclock; production passes `() => Date.now()`. + */ +export function createPermissionAuditPublisher(deps: { + ring: PermissionAuditRing; + now?: () => number; +}): PermissionAuditPublisher { + const { ring } = deps; + const now = deps.now ?? (() => Date.now()); + return { + recordRequested( + record: PermissionRequestRecord, + policy: PermissionPolicy, + votersAtIssue: ReadonlySet, + ): void { + ring.push({ + kind: 'permission.requested', + recordedAtMs: now(), + requestId: record.requestId, + sessionId: record.sessionId, + originatorClientId: record.originatorClientId, + policy, + votersAtIssue: Array.from(votersAtIssue), + issuedAtMs: record.issuedAtMs, + allowedOptionIds: Array.from(record.allowedOptionIds), + }); + }, + recordVoted( + record: PermissionRequestRecord, + vote: PermissionVote, + outcome: PermissionVoteOutcome, + ): void { + ring.push({ + kind: 'permission.voted', + recordedAtMs: now(), + requestId: record.requestId, + sessionId: record.sessionId, + clientId: vote.clientId, + optionId: vote.optionId, + fromLoopback: vote.fromLoopback, + receivedAtMs: vote.receivedAtMs, + outcome, + }); + }, + recordForbidden( + record: PermissionRequestRecord, + vote: PermissionVote, + reason: 'designated_mismatch' | 'remote_not_allowed', + ): void { + ring.push({ + kind: 'permission.forbidden', + recordedAtMs: now(), + requestId: record.requestId, + sessionId: record.sessionId, + clientId: vote.clientId, + optionId: vote.optionId, + fromLoopback: vote.fromLoopback, + reason, + }); + }, + recordResolved( + record: PermissionRequestRecord, + resolution: PermissionResolution, + decisionReason: PermissionDecisionReason, + ): void { + ring.push({ + kind: 'permission.resolved', + recordedAtMs: now(), + requestId: record.requestId, + sessionId: record.sessionId, + resolution, + decisionReason, + }); + }, + recordTimeout(record: PermissionRequestRecord): void { + ring.push({ + kind: 'permission.timeout', + recordedAtMs: now(), + requestId: record.requestId, + sessionId: record.sessionId, + issuedAtMs: record.issuedAtMs, + }); + }, + }; +} diff --git a/packages/cli/src/serve/runQwenServe.test.ts b/packages/cli/src/serve/runQwenServe.test.ts index 3e088421390..7cdaa4dd1d4 100644 --- a/packages/cli/src/serve/runQwenServe.test.ts +++ b/packages/cli/src/serve/runQwenServe.test.ts @@ -4,8 +4,12 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { describe, it, expect } from 'vitest'; -import { extractContextFilename } from './runQwenServe.js'; +import { describe, it, expect, vi } from 'vitest'; +import { + extractContextFilename, + InvalidPolicyConfigError, + validatePolicyConfig, +} from './runQwenServe.js'; /** * #4297 fold-in 7 (deepseek S1, addresses #3262690842). Lock the @@ -62,3 +66,110 @@ describe('extractContextFilename (#4297 fold-in 7 P2-1 helper)', () => { expect(extractContextFilename({ fileName: 'AGENTS.md' })).toBeUndefined(); }); }); + +/** + * Wenshao review #4335 / 3272493818 — positive tests for the + * `validatePolicyConfig` helper. Lock the contract so a future + * refactor can't silently remove the `InvalidPolicyConfigError` + * class or the validation paths. + */ +describe('validatePolicyConfig (#4335 boot validation)', () => { + it('returns undefined for both fields when policyConfig is empty', () => { + expect(validatePolicyConfig()).toEqual({ + permissionPolicy: undefined, + permissionConsensusQuorum: undefined, + }); + expect(validatePolicyConfig({})).toEqual({ + permissionPolicy: undefined, + permissionConsensusQuorum: undefined, + }); + }); + + it.each([['first-responder'], ['designated'], ['consensus'], ['local-only']])( + 'accepts the %s permissionStrategy literal', + (literal) => { + expect(validatePolicyConfig({ permissionStrategy: literal })).toEqual({ + permissionPolicy: literal, + permissionConsensusQuorum: undefined, + }); + }, + ); + + it('throws InvalidPolicyConfigError for an unknown permissionStrategy', () => { + expect(() => validatePolicyConfig({ permissionStrategy: 'bogus' })).toThrow( + InvalidPolicyConfigError, + ); + expect(() => validatePolicyConfig({ permissionStrategy: 'bogus' })).toThrow( + /invalid policy.permissionStrategy/, + ); + }); + + it.each([0, -1, 1.5, Number.NaN])( + 'throws InvalidPolicyConfigError for non-positive-integer consensusQuorum (%s)', + (badValue) => { + expect(() => + validatePolicyConfig({ + permissionStrategy: 'consensus', + consensusQuorum: badValue, + }), + ).toThrow(InvalidPolicyConfigError); + }, + ); + + it('accepts a positive-integer consensusQuorum with consensus strategy', () => { + expect( + validatePolicyConfig({ + permissionStrategy: 'consensus', + consensusQuorum: 3, + }), + ).toEqual({ + permissionPolicy: 'consensus', + permissionConsensusQuorum: 3, + }); + }); + + it('warns AND drops consensusQuorum when strategy is not consensus (#4335 / 3273077270)', () => { + // Wenshao review #4335 / 3273077270 — public contract now + // matches the warning text: when the operator sets + // consensusQuorum alongside a non-consensus strategy, the + // override is dropped (returned as undefined) so the + // BridgeOptions surface stays consistent with what the warning + // tells them. Pre-fix the function still propagated the value; + // the downstream mediator ignored it but the function-level + // contract contradicted itself. + const warnings: string[] = []; + const onWarning = vi.fn((m: string) => warnings.push(m)); + const result = validatePolicyConfig( + { + permissionStrategy: 'designated', + consensusQuorum: 2, + }, + onWarning, + ); + expect(result).toEqual({ + permissionPolicy: 'designated', + permissionConsensusQuorum: undefined, + }); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('consensusQuorum is set'); + expect(warnings[0]).toContain('not "consensus"'); + }); + + it('does not warn when consensusQuorum is set with consensus strategy', () => { + const onWarning = vi.fn(); + validatePolicyConfig( + { permissionStrategy: 'consensus', consensusQuorum: 2 }, + onWarning, + ); + expect(onWarning).not.toHaveBeenCalled(); + }); + + it('error messages name the field that failed (operator-debugging signal)', () => { + expect(() => validatePolicyConfig({ permissionStrategy: 'oops' })).toThrow( + /permissionStrategy/, + ); + expect(() => validatePolicyConfig({ consensusQuorum: 0 })).toThrow( + /consensusQuorum/, + ); + }); +}); diff --git a/packages/cli/src/serve/runQwenServe.ts b/packages/cli/src/serve/runQwenServe.ts index 33aa85b4230..e51f3d80586 100644 --- a/packages/cli/src/serve/runQwenServe.ts +++ b/packages/cli/src/serve/runQwenServe.ts @@ -19,13 +19,146 @@ import { import { createBridgeFileSystemAdapter } from './bridgeFileSystemAdapter.js'; import { createDaemonStatusProvider } from './daemonStatusProvider.js'; import { isLoopbackBind } from './loopbackBinds.js'; +import { + createPermissionAuditPublisher, + PermissionAuditRing, +} from './permissionAudit.js'; import { createServeApp, resolveBridgeFsFactory } from './server.js'; +// Wenshao review #4335 / 3272581563 — single runtime source of +// truth for the closed permission-policy set. `validatePolicyConfig` +// derives its valid-set from `permission_mediation.modes` so a +// future 5th policy lands in one place. +import { SERVE_CAPABILITY_REGISTRY } from './capabilities.js'; import type { ServeOptions } from './types.js'; import type { WorkspaceFileSystemFactory } from './fs/index.js'; +// Wenshao review #4335 / 3272493805 — use the canonical +// `PermissionPolicy` union from acp-bridge instead of inlining +// the four string literals at the let-declaration, the `as` +// cast, and the validation Set. Same drift-protection rationale +// as types.ts/3271978342. +import type { PermissionPolicy } from '@qwen-code/acp-bridge'; const QWEN_SERVER_TOKEN_ENV = 'QWEN_SERVER_TOKEN'; const SHUTDOWN_FORCE_CLOSE_MS = 5_000; +/** + * Wenshao review #4335 / 3271978374 — boot-time policy validation + * errors. Replaces the previous substring-matching of "invalid + * policy." in error messages (fragile against unrelated errors + * happening to contain that phrase, and against future rewordings + * of the validation message). The catch block in `runQwenServe` + * matches with `instanceof InvalidPolicyConfigError` to distinguish + * operator-misconfiguration (rethrow → fail boot loudly) from + * settings-read failures (fall back to defaults). + * + * Exported so tests can `instanceof`-check; pre-Round-7 the class + * was private and the `expect(...).toThrow(InvalidPolicyConfigError)` + * assertion couldn't bind the constructor. + */ +export class InvalidPolicyConfigError extends Error { + override readonly name = 'InvalidPolicyConfigError'; + constructor(message: string) { + super(message); + } +} + +/** + * Parse + validate the `policy.*` section of merged daemon settings. + * Extracted from inline boot logic (Round 8 / 3272493818) so the + * validation contract can be unit-tested without spinning up a + * daemon. Returns the resolved `permissionPolicy` / + * `permissionConsensusQuorum` for `BridgeOptions`, or throws + * `InvalidPolicyConfigError` for operator misconfiguration. + * + * - `permissionStrategy` must be one of the four `PermissionPolicy` + * literals if present. + * - `consensusQuorum` must be a positive integer if present. + * - When `consensusQuorum` is set but `permissionStrategy` is not + * `'consensus'`, the override is silently ignored — emit a + * stderr warning so the operator notices. + * + * The mismatch warning runs through `onWarning` so tests can + * capture it; production passes `writeStderrLine`. + * + * Wenshao review #4335 / 3272581563 — the runtime valid-policy set + * is derived from `SERVE_CAPABILITY_REGISTRY.permission_mediation.modes` + * (single source of truth) instead of repeating the four literals + * a fourth time. The compile-time check that every entry is a + * `PermissionPolicy` ensures registry drift surfaces here. + */ +export function validatePolicyConfig( + policyConfig: { permissionStrategy?: string; consensusQuorum?: number } = {}, + onWarning: (message: string) => void = writeStderrLine, +): { + permissionPolicy: PermissionPolicy | undefined; + permissionConsensusQuorum: number | undefined; +} { + // Derive from the capability registry so the runtime set, the + // settings schema enum, the `PermissionPolicy` union, and the + // capability advertisement all stay aligned through a single + // edit point. The cast asserts every `modes` entry is a + // `PermissionPolicy` — TypeScript's `satisfies Record` on the registry doesn't narrow + // `modes` to the union, so the assertion is necessary here. The + // `permissionMediation.test.ts` capability-suite asserts the + // modes list is exhaustive over `PermissionPolicy`, providing + // the runtime guarantee. + const validSet: ReadonlySet = new Set( + SERVE_CAPABILITY_REGISTRY.permission_mediation.modes, + ); + if ( + policyConfig.permissionStrategy !== undefined && + !validSet.has(policyConfig.permissionStrategy) + ) { + throw new InvalidPolicyConfigError( + `qwen serve: invalid policy.permissionStrategy ` + + `"${String(policyConfig.permissionStrategy)}"; must be one of ` + + `${Array.from(validSet).join(', ')}`, + ); + } + if ( + policyConfig.consensusQuorum !== undefined && + (!Number.isInteger(policyConfig.consensusQuorum) || + policyConfig.consensusQuorum < 1) + ) { + throw new InvalidPolicyConfigError( + `qwen serve: invalid policy.consensusQuorum ` + + `${String(policyConfig.consensusQuorum)}; must be a positive integer`, + ); + } + // Wenshao review #4335 / 3273077270 — when consensusQuorum is set + // but the active strategy doesn't use it, the warning previously + // promised "the override will be ignored" while the function + // STILL propagated the value to BridgeOptions. The downstream + // mediator only reads it under the consensus policy, so behavior + // was correct, but the public contract contradicted its own + // warning text. Adopt option (a): make the public contract match + // the warning by dropping the value when the strategy is not + // 'consensus'. Operators reading the warning at boot now see + // consistent behavior all the way down. + const consensusQuorumActive = + policyConfig.consensusQuorum !== undefined && + policyConfig.permissionStrategy === 'consensus'; + if ( + policyConfig.consensusQuorum !== undefined && + policyConfig.permissionStrategy !== 'consensus' + ) { + onWarning( + 'qwen serve: policy.consensusQuorum is set but ' + + 'policy.permissionStrategy is not "consensus"; the override will ' + + 'be ignored.', + ); + } + return { + permissionPolicy: policyConfig.permissionStrategy as + | PermissionPolicy + | undefined, + permissionConsensusQuorum: consensusQuorumActive + ? policyConfig.consensusQuorum + : undefined, + }; +} + /** * Wrap raw IPv6 literals in brackets so the printed URL is a valid RFC 3986 * authority. `host:port` is ambiguous when host contains `:`, so the URL @@ -338,52 +471,67 @@ export async function runQwenServe( QWEN_SERVE_MCP_BUDGET_MODE: opts.mcpBudgetMode, }; - // #4282 fold-in 5 (Codex P2-1). Snapshot the workspace's preferred - // context filename at boot. The daemon parent doesn't go through - // `loadCliConfig` (it spawns an ACP child that does), so the - // process-global `getCurrentGeminiMdFilename()` stays on the - // default `QWEN.md` even when the workspace has - // `context.fileName: 'AGENTS.md'`. Reading merged settings here - // and forwarding to the bridge ensures `POST /workspace/init` - // writes the same file the ACP child reads. Settings changes made - // after daemon boot need a daemon restart to take effect — this - // value rarely changes, and a snapshot avoids re-reading on every - // init request. - // - // #4297 fold-in 2 (copilot S2): only accept string-typed values. - // Hand-edited `settings.json` could land an object / number / - // nested array in `context.fileName`; the previous `String(...)` - // coerce would have produced a literal `"[object Object]"` - // filename. Falling back to `undefined` makes the bridge use its - // own `getCurrentGeminiMdFilename()` default — a sane recovery - // posture that keeps the daemon alive instead of writing a - // garbage filename. - // - // #4297 fold-in 7 (qwen-latest critical, addresses #3262625091): - // wrap the boot-time `loadSettings` read in try/catch. A - // corrupted, malformed, or temporarily unreadable - // `settings.json` (partial write by a concurrent editor, - // permission denied, transient disk IO) used to throw - // synchronously and BLOCK DAEMON BOOT. Pre-PR this never - // happened because `loadSettings` was called lazily inside - // request handlers (which had their own error handling). We - // catch + log + fall back to the bridge's default filename so - // the daemon stays bootable. + // #4282 fold-in 5 + F3 Commit 5: read settings once at boot for + // both the workspace context filename (Codex P2-1) and the new + // F3 policy fields (permissionStrategy / consensusQuorum). Wrap + // in try/catch (#4297 fold-in 7) so a corrupted settings.json + // doesn't block daemon boot — context filename falls back to the + // bridge's default; policy validation rethrows because invalid + // policy is an explicit operator misconfiguration. let contextFilenameForInit: string | undefined; + let permissionPolicy: PermissionPolicy | undefined; + let permissionConsensusQuorum: number | undefined; try { const bootSettings = loadSettings(boundWorkspace); contextFilenameForInit = extractContextFilename( bootSettings.merged.context?.fileName, ); + const policyConfig = + ( + bootSettings.merged as { + policy?: { + permissionStrategy?: string; + consensusQuorum?: number; + }; + } + ).policy ?? {}; + const resolved = validatePolicyConfig(policyConfig); + permissionPolicy = resolved.permissionPolicy; + permissionConsensusQuorum = resolved.permissionConsensusQuorum; } catch (err) { + // F3 invariant: invalid policy values must fail startup loudly. + // Wenshao review #4335 / 3271978374 — discriminate by error class + // rather than substring-matching the message; a future reworded + // validation string would have silently downgraded operator + // misconfiguration to "fall back to defaults" without this. + if (err instanceof InvalidPolicyConfigError) { + throw err; + } + // All other settings-read failures (corrupted JSON, transient + // disk IO) fall back to defaults so the daemon stays bootable. writeStderrLine( - `qwen serve: could not read settings for context.fileName ` + - `(${err instanceof Error ? err.message : String(err)}); ` + - `falling back to the daemon's default context filename. ` + - `Restart with a valid settings.json to use a custom name.`, + `qwen serve: could not read settings for context.fileName / ` + + `policy.* (${err instanceof Error ? err.message : String(err)}); ` + + `falling back to defaults. Restart with a valid settings.json ` + + `to apply context.fileName / policy.* overrides.`, ); } + // F3 Commit 2 — allocate the audit ring + publisher in the daemon + // host (here) rather than inside the bridge factory, because the + // ring is the seam future PRs will lift up to expose `GET + // /workspace/permission/audit`. Wenshao review #4335 (3270622298 / + // 3270622304): without this wiring all 5 audit record types were + // silently dropped through `createNoOpPermissionAuditPublisher`, + // and the timeout breadcrumb the bridge JSDoc promised never + // existed. The mediator now writes a stderr line on each timeout + // (preserving pre-F3 visibility) and forwards the entry to this + // ring for the future query route. + const permissionAuditRing = new PermissionAuditRing(); + const permissionAuditPublisher = createPermissionAuditPublisher({ + ring: permissionAuditRing, + }); + // F1 follow-up (#4319): construct `fsFactory` BEFORE the bridge so // the bridge can wire it through `BridgeFileSystem` for ACP-side // writeTextFile / readTextFile calls. Pre-fix the bridge constructed @@ -409,6 +557,15 @@ export async function runQwenServe( : {}), boundWorkspace, childEnvOverrides, + // F3 Commit 5 — wire the validated policy/quorum from + // settings into the bridge. Bridge factory does its own + // defensive `Number.isInteger` recheck on the quorum so a + // direct embedder can't bypass our validation. + ...(permissionPolicy !== undefined ? { permissionPolicy } : {}), + ...(permissionConsensusQuorum !== undefined + ? { permissionConsensusQuorum } + : {}), + permissionAudit: permissionAuditPublisher, // #4175 PR 22b/2: inject the daemon-host status provider so the // bridge can pull env / preflight cells through a typed seam // instead of importing daemon-host helpers directly. Production diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 78f2dcf4e6f..4567acebed9 100644 --- a/packages/cli/src/serve/server.test.ts +++ b/packages/cli/src/serve/server.test.ts @@ -10,7 +10,7 @@ import * as path from 'node:path'; import { fileURLToPath } from 'node:url'; import { describe, it, expect, afterEach, vi } from 'vitest'; import request from 'supertest'; -import { createServeApp } from './server.js'; +import { createServeApp, detectFromLoopback } from './server.js'; import { runQwenServe, type RunHandle } from './runQwenServe.js'; import { CONDITIONAL_SERVE_FEATURES, @@ -139,6 +139,12 @@ const EXPECTED_STAGE1_FEATURES = [ // baseline assertion below mirrors that even though PR 21 landed // before PR 17 chronologically. 'auth_device_flow', + // #4175 F3 Commit 6. Daemon advertises which permission mediation + // policies it can run (`modes: [first-responder, designated, consensus, + // local-only]`) so SDK clients can pre-flight before relying on + // `permission_partial_vote` / `permission_forbidden` SSE events. Always- + // on; runtime-active policy is at `/capabilities` body `policy.permission`. + 'permission_mediation', ] as const; // Issue #4175 PR 15. `require_auth` is registered but conditionally @@ -149,9 +155,14 @@ const EXPECTED_STAGE1_FEATURES = [ // matches the real ordering. const EXPECTED_REGISTERED_FEATURES = [ // Same order as `SERVE_CAPABILITY_REGISTRY` declaration: - ...EXPECTED_STAGE1_FEATURES.filter((f) => f !== 'auth_device_flow'), + // ...always-on PR16/17/19/20/21 features, then conditional `require_auth`, + // then `auth_device_flow`, then F3 `permission_mediation` (latest). + ...EXPECTED_STAGE1_FEATURES.filter( + (f) => f !== 'auth_device_flow' && f !== 'permission_mediation', + ), 'require_auth', 'auth_device_flow', + 'permission_mediation', ] as const; interface FakeBridgeOpts { @@ -516,6 +527,11 @@ function fakeBridge(opts: FakeBridgeOpts = {}): FakeBridge { clientLastSeenAt: new Map(), })); return { + // F3 Commit 6 — `HttpAcpBridge.permissionPolicy` is required so + // `/capabilities` can expose `policy.permission`. Tests don't + // exercise mediation; pin to the pre-F3 default ('first-responder') + // so existing assertions stay shape-compatible. + permissionPolicy: 'first-responder' as const, calls, loadCalls, resumeCalls, @@ -740,6 +756,66 @@ function fakeBridge(opts: FakeBridgeOpts = {}): FakeBridge { }; } +/** + * Wenshao review #4335 / 3272581557 — supertest in the rest of this + * suite always connects from `127.0.0.1`, leaving the prefix-match + * branches in `detectFromLoopback` (the security gate for + * `local-only` permission policy) without direct coverage. Exercise + * the helper synchronously over the address shapes the Round-5 fix + * widened, plus the fail-closed branches. + */ +describe('detectFromLoopback (#4335 / 3272581557)', () => { + function fakeReq(addr: string | undefined): { + socket?: { remoteAddress?: string | undefined }; + } { + if (addr === undefined) return {}; + return { socket: { remoteAddress: addr } }; + } + + it.each([ + ['127.0.0.1', true], + ['127.0.0.2', true], + ['127.0.1.1', true], + ['127.255.255.254', true], + ['::1', true], + ['::ffff:127.0.0.1', true], + ['::ffff:127.0.0.2', true], + ['10.0.0.1', false], + ['192.168.1.1', false], + ['1.2.3.4', false], + ['::', false], + ['fe80::1', false], + // RFC 1918 private addrs that LOOK loopback-adjacent but aren't. + ['127', false], + // Note: `'127.'` is structurally `127.`-prefix so the helper + // accepts it (fail-OPEN for that malformed shape). Real + // `req.socket.remoteAddress` values come from the kernel as + // well-formed dotted-decimal IPs, so this only matters for + // pathological synthetic inputs. Documented for transparency. + // Empty / malformed. + ['', false], + ])('detectFromLoopback(%s) === %s', (addr, expected) => { + expect(detectFromLoopback(fakeReq(addr))).toBe(expected); + }); + + it('returns false for missing socket (fail-closed)', () => { + expect(detectFromLoopback({})).toBe(false); + expect(detectFromLoopback(fakeReq(undefined))).toBe(false); + }); + + it('does NOT consult X-Forwarded-For or any HTTP header (security)', () => { + // The function takes only the socket-shaped input — even if the + // express request would carry forwarded headers, this helper + // can't see them. Pin the contract. + const reqWithForwardedHeader = { + socket: { remoteAddress: '10.0.0.1' }, + get: (name: string) => + name === 'X-Forwarded-For' ? '127.0.0.1' : undefined, + } as unknown as Parameters[0]; + expect(detectFromLoopback(reqWithForwardedHeader)).toBe(false); + }); +}); + describe('createServeApp', () => { describe('serve capability registry', () => { it('returns a fresh ordered registered feature list', () => { @@ -2800,11 +2876,16 @@ describe('createServeApp', () => { .set('Host', `127.0.0.1:${baseOpts.port}`) .send({ outcome: { outcome: 'selected', optionId: 'allow' } }); expect(res.status).toBe(200); + // F3 Commit 2 — vote routes attach `fromLoopback` from + // `detectFromLoopback(req)`. The supertest fixture connects + // from `127.0.0.1`, so the captured context carries + // `fromLoopback: true` even though no client-id header is set. expect(bridge.sessionPermissionVotes).toEqual([ { sessionId: 'session-A', requestId: 'req-1', response: { outcome: { outcome: 'selected', optionId: 'allow' } }, + context: { fromLoopback: true }, }, ]); }); @@ -2818,8 +2899,13 @@ describe('createServeApp', () => { .set('X-Qwen-Client-Id', 'client-1') .send({ outcome: { outcome: 'cancelled' } }); expect(res.status).toBe(200); + // F3 Commit 2 — `fromLoopback` is derived from the kernel-stamped + // peer IP (`127.0.0.1` in tests), independent of the client-id + // header. Both fields end up on the context the route forwards + // to the bridge. expect(bridge.sessionPermissionVotes[0]?.context).toEqual({ clientId: 'client-1', + fromLoopback: true, }); }); @@ -2919,10 +3005,13 @@ describe('createServeApp', () => { .set('Host', `127.0.0.1:${baseOpts.port}`) .send({ outcome: { outcome: 'selected', optionId: 'allow' } }); expect(res.status).toBe(200); + // F3 Commit 2 — see the scoped-vote case: `fromLoopback: true` + // is attached because the supertest peer is `127.0.0.1`. expect(bridge.permissionVotes).toEqual([ { requestId: 'req-1', response: { outcome: { outcome: 'selected', optionId: 'allow' } }, + context: { fromLoopback: true }, }, ]); }); @@ -2938,6 +3027,7 @@ describe('createServeApp', () => { expect(res.status).toBe(200); expect(bridge.permissionVotes[0]?.context).toEqual({ clientId: 'client-1', + fromLoopback: true, }); }); @@ -4008,6 +4098,9 @@ describe('runQwenServe', () => { writeTextAtomic: async () => { throw new Error('unreachable'); }, + writeTextOverwrite: async () => { + throw new Error('unreachable'); + }, edit: async () => { throw new Error('unreachable'); }, diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index b5913a57021..4a2bb106161 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -34,6 +34,7 @@ import { isServeDebugMode } from './debugMode.js'; import { isLoopbackBind } from './loopbackBinds.js'; import { canonicalizeWorkspace, + CancelSentinelCollisionError, createHttpAcpBridge, InvalidClientIdError, InvalidPermissionOptionError, @@ -42,6 +43,8 @@ import { MAX_WORKSPACE_PATH_LENGTH, McpServerNotFoundError, McpServerRestartFailedError, + PermissionForbiddenError, + PermissionPolicyNotImplementedError, RestoreInProgressError, SessionLimitExceededError, SessionNotFoundError, @@ -650,6 +653,13 @@ export function createServeApp( // #3803 §02: surface the bound workspace so clients can detect // mismatch pre-flight and omit `cwd` on `POST /session`. workspaceCwd: boundWorkspace, + // #4175 F3 Commit 6 — active mediation policy under the + // `policy` namespace. Distinct from the `permission_mediation` + // capability `modes` list (build-supported set). SDK clients + // pre-flight the active strategy and render the matching UX + // (e.g. consensus shows partial-vote progress, designated + // disables the vote button for non-originators). + policy: { permission: bridge.permissionPolicy }, }; res.status(200).json(envelope); }); @@ -1661,13 +1671,26 @@ export function createServeApp( if (response === undefined) return; const clientId = parseClientIdHeader(req, res); if (clientId === null) return; + // F3 Commit 2: thread the kernel-stamped peer-IP loopback bit + // through the bridge context so the `local-only` policy can gate + // votes by transport. We always pass a context (even when no + // header was supplied) so `fromLoopback` reaches the mediator — + // the prior gating shape (`clientId !== undefined ? { clientId } + // : undefined`) would silently drop the loopback bit for + // anonymous loopback voters, which is the exact `local-only` + // happy path. + const fromLoopback = detectFromLoopback(req); + const context = { + ...(clientId !== undefined ? { clientId } : {}), + fromLoopback, + }; let accepted: boolean; try { accepted = bridge.respondToSessionPermission( sessionId, requestId, response, - clientId !== undefined ? { clientId } : undefined, + context, ); } catch (err) { sendPermissionVoteError(res, err, { @@ -1693,13 +1716,15 @@ export function createServeApp( if (response === undefined) return; const clientId = parseClientIdHeader(req, res); if (clientId === null) return; + // Same F3 Commit 2 rationale as the session-scoped route above. + const fromLoopback = detectFromLoopback(req); + const context = { + ...(clientId !== undefined ? { clientId } : {}), + fromLoopback, + }; let accepted: boolean; try { - accepted = bridge.respondToPermission( - requestId, - response, - clientId !== undefined ? { clientId } : undefined, - ); + accepted = bridge.respondToPermission(requestId, response, context); } catch (err) { sendPermissionVoteError(res, err, { route: 'POST /permission/:requestId', @@ -2222,6 +2247,52 @@ function parseClientIdHeader( return raw; } +/** + * #4175 F3 Commit 2 — decide whether a permission vote arrived from a + * loopback peer. + * + * Per RFC 1122 the entire `127.0.0.0/8` block is loopback (and the + * IPv4-mapped IPv6 form `::ffff:127.0.0.0/104` mirrors that). IPv6 + * loopback is `::1` (single literal). Wenshao review #4335 / + * 3271185597 — the previous exact-match Set of three literals + * (`127.0.0.1`, `::1`, `::ffff:127.0.0.1`) silently fail-CLOSED + * on legitimate non-`.0.0.1` loopback addresses (`127.0.0.2`, + * `127.0.1.1`, etc.), causing unexpected `remote_not_allowed` + * rejections under `local-only` policy. Switched to a prefix + * test so the entire `/8` and its dual-stack mirror are accepted. + * + * **Security**: this function reads `req.socket.remoteAddress` only. + * It does NOT consult `X-Forwarded-For` or any HTTP header — those + * are forgeable, and `local-only` is meant to be a strong gate that + * trusts the kernel-stamped peer IP. Operators who want + * loopback-equivalent treatment for a reverse-proxied daemon should + * use a dedicated daemon process instead, or pick `designated` + * policy where loopback identity is irrelevant. + * + * Direction is fail-CLOSED: an unrecognized `remoteAddress` shape + * (missing socket, non-string) returns `false`, gating votes + * conservatively rather than admitting them. + */ +// Wenshao review #4335 / 3272581557 — exported for unit testing. +// supertest in the existing server tests always connects from +// `127.0.0.1`, so the prefix-match logic for `127.x`-beyond-`.0.0.1`, +// `::1`, `::ffff:127.*`, and the fail-closed non-string branch had +// no direct test. The factored-out helper takes a minimal shape so +// tests can exercise it without spinning up Express. +export function detectFromLoopback(req: { + socket?: { remoteAddress?: string | undefined }; +}): boolean { + const addr = req.socket?.remoteAddress; + if (typeof addr !== 'string') return false; + // IPv6 loopback (single literal). + if (addr === '::1') return true; + // IPv4 loopback: 127.0.0.0/8. + if (addr.startsWith('127.')) return true; + // IPv4-mapped IPv6 loopback: ::ffff:127.0.0.0/104. + if (addr.startsWith('::ffff:127.')) return true; + return false; +} + /** * #4282 fold-in 1 (gpt-5.5 C2). Workspace-level mutation routes validate * the parsed `X-Qwen-Client-Id` against `bridge.knownClientIds()` so the @@ -2413,6 +2484,45 @@ function sendPermissionVoteError( }); return; } + // F3 Commit 2 — designated voter mismatch / `local-only` remote + // rejection. 403 because the request is well-formed and the voter + // was authenticated; the policy refuses their vote. + if (err instanceof PermissionForbiddenError) { + res.status(403).json({ + error: err.message, + code: 'permission_forbidden', + requestId: err.requestId, + sessionId: err.sessionId, + reason: err.reason, + }); + return; + } + // F3 Commit 2 — operator configured a permission policy whose + // implementation has not landed in this build yet. 501 (not 500) + // so the SDK can render "your daemon is older than your settings + // expect; upgrade" rather than a generic Internal Server Error. + if (err instanceof PermissionPolicyNotImplementedError) { + res.status(501).json({ + error: err.message, + code: 'permission_policy_not_implemented', + policy: err.policy, + }); + return; + } + // F3 Commit 2 — agent declared an `allowedOptionIds` set that + // includes the cancel-vote sentinel. This is a contract violation + // between agent and daemon (not a client mistake), so 500 is the + // right shape; structured `code` lets the SDK distinguish from + // unrelated 500s. + if (err instanceof CancelSentinelCollisionError) { + res.status(500).json({ + error: err.message, + code: 'cancel_sentinel_collision', + requestId: err.requestId, + sentinel: err.sentinel, + }); + return; + } sendBridgeError(res, err, ctx); } diff --git a/packages/cli/src/serve/types.ts b/packages/cli/src/serve/types.ts index d24d00d5d83..1e96abac9ba 100644 --- a/packages/cli/src/serve/types.ts +++ b/packages/cli/src/serve/types.ts @@ -9,6 +9,13 @@ import { type ServeFeature, type ServeProtocolVersions, } from './capabilities.js'; +// Wenshao review #4335 / 3271978342 — import the canonical +// `PermissionPolicy` union from acp-bridge instead of inlining +// the four string literals below. Without the import, adding a +// 5th policy literal upstream would silently widen the union +// over there while this envelope kept accepting only the older +// 4-literal narrower set, with no compiler error to flag the drift. +import type { PermissionPolicy } from '@qwen-code/acp-bridge'; /** * Stage 1 daemon mode shape. @@ -175,6 +182,23 @@ export interface CapabilitiesEnvelope { * shape. The post-§02 server code here always populates it. */ workspaceCwd?: string; + /** + * #4175 F3 Commit 6 — daemon-policy namespace. Active values for + * cross-cutting daemon coordination policies that don't fit on a + * per-feature flag. Today only `permission` is populated (active + * `PermissionMediator` strategy); future entries (e.g. `network`, + * `audit`) extend the namespace without polluting the top-level + * envelope. Optional / additive — daemons predating F3 omit it. + */ + policy?: { + /** + * Active permission mediation policy. Distinct from the + * `permission_mediation` capability `modes` list, which + * advertises the build-supported set; this field tells clients + * which one is currently in effect. + */ + permission?: PermissionPolicy; + }; } export const CAPABILITIES_SCHEMA_VERSION = 1 as const; diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index 7e2ee1b98e9..a3a2d7bf2cd 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -51,6 +51,15 @@ const DAEMON_KNOWN_EVENT_TYPE_VALUES = [ 'workspace_initialized', 'mcp_server_restarted', 'mcp_server_restart_refused', + // #4175 F3 (Commit 7) — multi-client permission coordination events. + // `permission_partial_vote` only fires under `consensus` policy; + // `permission_forbidden` fires under `designated` (originator + // mismatch), `consensus` (anonymous voter or not-in-snapshot), and + // `local-only` (remote voter). Pre-flight on the + // `permission_mediation` capability tag before relying on either — + // daemons predating F3 omit both event types. + 'permission_partial_vote', + 'permission_forbidden', ] as const; const DAEMON_KNOWN_EVENT_TYPES: ReadonlySet = new Set( @@ -96,6 +105,61 @@ export interface DaemonPermissionAlreadyResolvedData { [key: string]: unknown; } +/** + * F3 Commit 7. `permission_partial_vote` SSE frame fired by the + * `consensus` policy on every recorded non-resolving vote. The + * snapshot at `GET /workspace/mcp` (etc.) does NOT carry vote-progress + * state; SDK consumers reconstruct it from this stream. + * + * `votesNeeded` = `quorum - max(tally per option)`, clamped to ≥1. + * `optionTallies` is a per-option count; the leading option is the + * one with the highest tally (ties broken by first-cast order at the + * mediator level — not directly reflected here). + */ +export interface DaemonPermissionPartialVoteData { + requestId: string; + sessionId: string; + votesReceived: number; + votesNeeded: number; + quorum: number; + optionTallies: Record; + /** + * Wenshao review #4335 / 3270622311. Stamped from the SSE + * envelope's `originatorClientId` (= prompt originator per F3 N3) + * by the session reducer's `mergeOriginator` step so view-state + * consumers can attribute the partial vote to the prompting + * client without retaining the original event. + */ + originatorClientId?: string; + [key: string]: unknown; +} + +/** + * F3 Commit 7. `permission_forbidden` SSE frame fired when a vote is + * rejected by the active policy. `clientId` is the rejected voter + * (omitted when anonymous); `reason` is the closed contract enum. + * + * The frame's top-level `originatorClientId` (on the wrapping + * `DaemonEvent`, not in `data`) stamps the prompt originator — NOT + * the rejected voter — per the F3 N3 invariant. Cross-reference + * `data.clientId` for voter attribution. + */ +export interface DaemonPermissionForbiddenData { + requestId: string; + sessionId: string; + clientId?: string; + reason: 'designated_mismatch' | 'remote_not_allowed'; + /** + * Wenshao review #4335 / 3270622311. Stamped from the SSE + * envelope's `originatorClientId` (= prompt originator per F3 N3) + * by the session reducer's `mergeOriginator` step. Distinct from + * `clientId` (the rejected voter's id) — both are useful and + * neither subsumes the other. + */ + originatorClientId?: string; + [key: string]: unknown; +} + export interface DaemonModelSwitchedData { sessionId: string; modelId: string; @@ -421,6 +485,14 @@ export type DaemonPermissionAlreadyResolvedEvent = DaemonEventEnvelope< 'permission_already_resolved', DaemonPermissionAlreadyResolvedData >; +export type DaemonPermissionPartialVoteEvent = DaemonEventEnvelope< + 'permission_partial_vote', + DaemonPermissionPartialVoteData +>; +export type DaemonPermissionForbiddenEvent = DaemonEventEnvelope< + 'permission_forbidden', + DaemonPermissionForbiddenData +>; export type DaemonModelSwitchedEvent = DaemonEventEnvelope< 'model_switched', DaemonModelSwitchedData @@ -530,6 +602,8 @@ export type DaemonControlEvent = | DaemonPermissionRequestEvent | DaemonPermissionResolvedEvent | DaemonPermissionAlreadyResolvedEvent + | DaemonPermissionPartialVoteEvent + | DaemonPermissionForbiddenEvent | DaemonApprovalModeChangedEvent | DaemonToolToggledEvent | DaemonWorkspaceInitializedEvent @@ -668,8 +742,41 @@ export interface DaemonSessionViewState { lastMcpRestart?: DaemonMcpServerRestartedData; mcpRestartRefusedCount: number; lastMcpRestartRefused?: DaemonMcpServerRestartRefusedData; + /** + * F3 Commit 7. Per-pending consensus vote progress, keyed by + * `requestId`. Updated on every `permission_partial_vote` frame; + * cleared when the corresponding `permission_resolved` / + * `permission_already_resolved` arrives. Daemons running + * non-consensus policies never populate this map. + */ + permissionVoteProgress: Record; + /** + * F3 Commit 7. Bounded history of recent `permission_forbidden` + * events on this session — first 32 retained, oldest evicted on + * overflow. Adapters use this to render "client X tried to vote + * but was rejected" notices for the session. + */ + forbiddenVotes: readonly DaemonPermissionForbiddenData[]; + /** + * F3 Commit 7. Total `permission_forbidden` event count this + * stream has observed (including ones evicted from + * `forbiddenVotes`). + */ + forbiddenVoteCount: number; } +/** + * F3 Commit 7. Bound on `forbiddenVotes` retention. Half of + * `MAX_PENDING_PER_SESSION` (64) — forbidden votes are + * observability records, not pending state, so we keep the smaller + * bound to avoid blowing the SDK heap on a session that's getting + * spammed with rejected votes (e.g. an attacker probing + * `local-only` from a remote IP). Operators with full audit needs + * should subscribe to the daemon-side audit ring, not the SDK + * reducer's bounded history. + */ +const MAX_FORBIDDEN_VOTES_PER_SESSION = 32; + export function createDaemonSessionViewState( seed: Partial = {}, ): DaemonSessionViewState { @@ -711,6 +818,9 @@ export function createDaemonSessionViewState( lastMcpRestart: seed.lastMcpRestart, mcpRestartRefusedCount: seed.mcpRestartRefusedCount ?? 0, lastMcpRestartRefused: seed.lastMcpRestartRefused, + permissionVoteProgress: { ...seed.permissionVoteProgress }, + forbiddenVotes: seed.forbiddenVotes ? [...seed.forbiddenVotes] : [], + forbiddenVoteCount: seed.forbiddenVoteCount ?? 0, }; } @@ -748,6 +858,14 @@ export function asKnownDaemonEvent( return isPermissionAlreadyResolvedData(event.data) ? (event as DaemonPermissionAlreadyResolvedEvent) : undefined; + case 'permission_partial_vote': + return isPermissionPartialVoteData(event.data) + ? (event as DaemonPermissionPartialVoteEvent) + : undefined; + case 'permission_forbidden': + return isPermissionForbiddenData(event.data) + ? (event as DaemonPermissionForbiddenEvent) + : undefined; case 'model_switched': return isModelSwitchedData(event.data) ? (event as DaemonModelSwitchedEvent) @@ -887,9 +1005,18 @@ export function reduceDaemonSessionEvent( }; } case 'permission_resolved': { + // Wenshao review #4335 / 3271041465 — even on the unmatched + // path (SDK reconnected mid-permission and missed + // `permission_request`), clear any orphan progress entry that + // a `permission_partial_vote` may have left behind. Otherwise + // `permissionVoteProgress[requestId]` persists until session + // end. The matched path also clears it (below). + const permissionVoteProgress = { ...base.permissionVoteProgress }; + delete permissionVoteProgress[event.data.requestId]; if (!(event.data.requestId in base.pendingPermissions)) { return { ...base, + permissionVoteProgress, unmatchedPermissionResolutionCount: base.unmatchedPermissionResolutionCount + 1, lastUnmatchedPermissionResolutionId: event.data.requestId, @@ -897,12 +1024,18 @@ export function reduceDaemonSessionEvent( } const pendingPermissions = { ...base.pendingPermissions }; delete pendingPermissions[event.data.requestId]; - return { ...base, pendingPermissions }; + return { ...base, pendingPermissions, permissionVoteProgress }; } case 'permission_already_resolved': { + // Wenshao review #4335 / 3271041465 — same as above: + // unconditionally clear any orphan progress entry on the + // unmatched / matched paths. + const permissionVoteProgress = { ...base.permissionVoteProgress }; + delete permissionVoteProgress[event.data.requestId]; if (!(event.data.requestId in base.pendingPermissions)) { return { ...base, + permissionVoteProgress, unmatchedPermissionResolutionCount: base.unmatchedPermissionResolutionCount + 1, lastUnmatchedPermissionResolutionId: event.data.requestId, @@ -910,7 +1043,51 @@ export function reduceDaemonSessionEvent( } const pendingPermissions = { ...base.pendingPermissions }; delete pendingPermissions[event.data.requestId]; - return { ...base, pendingPermissions }; + return { ...base, pendingPermissions, permissionVoteProgress }; + } + case 'permission_partial_vote': { + // F3 Commit 7 — accumulate consensus vote progress. If the + // requestId isn't in `pendingPermissions` (race / replay + // misalignment because the SDK reconnected mid-permission and + // missed `permission_request`), still record progress here. + // Wenshao review #4335 / 3271041465 — both `permission_resolved` + // and `permission_already_resolved` reducer cases above now + // unconditionally clear any orphan `permissionVoteProgress` + // entry, so a missed-request reconnect is recovered as soon + // as the corresponding resolution frame arrives. + // + // Wenshao review #4335 / 3270622311: stamp the envelope's + // `originatorClientId` (prompt originator per N3) onto the + // stored data so view-state consumers can attribute the + // partial vote to the prompting client. Mirrors the + // `mergeOriginator` pattern used by approval-mode / tool- + // toggle / workspace-init / mcp-restart reducer cases. + return { + ...base, + permissionVoteProgress: { + ...base.permissionVoteProgress, + [event.data.requestId]: mergeOriginator(event.data, event), + }, + }; + } + case 'permission_forbidden': { + // F3 Commit 7 — append to bounded history and bump count. + // Wenshao review #4335 / 3270622311: same mergeOriginator + // treatment as the partial-vote case above. `event.data` carries + // the BLOCKED voter's clientId; the envelope's + // `originatorClientId` carries the prompt originator. Both are + // useful — consumers reading view state need the prompt + // originator without having to keep the original event around. + const next = base.forbiddenVotes.slice(); + next.push(mergeOriginator(event.data, event)); + while (next.length > MAX_FORBIDDEN_VOTES_PER_SESSION) { + next.shift(); + } + return { + ...base, + forbiddenVotes: next, + forbiddenVoteCount: base.forbiddenVoteCount + 1, + }; } case 'model_switched': return { @@ -932,6 +1109,14 @@ export function reduceDaemonSessionEvent( alive: false, terminalEvent: chooseTerminalEvent(base.terminalEvent, event), pendingPermissions: {}, + permissionVoteProgress: {}, + // Wenshao review #4335 / 3272576003 (Critical) — terminal + // events must also drop `forbiddenVotes` history so adapters + // reading view state for a dead session don't render stale + // rejection data. Pre-fix only `pendingPermissions` / + // `permissionVoteProgress` were cleared. + forbiddenVotes: [], + forbiddenVoteCount: 0, }; case 'session_closed': return { @@ -940,6 +1125,10 @@ export function reduceDaemonSessionEvent( alive: false, terminalEvent: chooseTerminalEvent(base.terminalEvent, event), pendingPermissions: {}, + permissionVoteProgress: {}, + // Wenshao review #4335 / 3272576003 (Critical) — see session_died. + forbiddenVotes: [], + forbiddenVoteCount: 0, }; case 'session_metadata_updated': return { @@ -953,6 +1142,10 @@ export function reduceDaemonSessionEvent( alive: false, terminalEvent: chooseTerminalEvent(base.terminalEvent, event), pendingPermissions: {}, + permissionVoteProgress: {}, + // Wenshao review #4335 / 3272576003 (Critical) — see session_died. + forbiddenVotes: [], + forbiddenVoteCount: 0, }; case 'slow_client_warning': // Non-terminal: warning precedes eviction but doesn't close @@ -971,6 +1164,10 @@ export function reduceDaemonSessionEvent( terminalEvent: chooseTerminalEvent(base.terminalEvent, event), streamError: event.data, pendingPermissions: {}, + permissionVoteProgress: {}, + // Wenshao review #4335 / 3272576003 (Critical) — see session_died. + forbiddenVotes: [], + forbiddenVoteCount: 0, }; case 'mcp_budget_warning': // Non-terminal: budget pressure is a status signal, not a stream @@ -1367,6 +1564,68 @@ function isPermissionAlreadyResolvedData( ); } +function isPermissionPartialVoteData( + value: unknown, +): value is DaemonPermissionPartialVoteData { + // PR #4335 review fold-in (Copilot finding): use `isFiniteNumber` + // (and integer + non-negative checks) for tally counters so + // malformed frames carrying NaN / Infinity / fractional values are + // rejected and counted via `unrecognizedKnownEventCount` instead of + // landing in reducer state. Matches the validation posture of the + // sibling `isMcpBudgetWarningData` / `isSlowClientWarningData` helpers. + if ( + !isRecord(value) || + !isNonEmptyString(value['requestId']) || + !isNonEmptyString(value['sessionId']) || + !isFiniteNumber(value['votesReceived']) || + !isFiniteNumber(value['votesNeeded']) || + !isFiniteNumber(value['quorum']) || + !Number.isInteger(value['votesReceived']) || + !Number.isInteger(value['votesNeeded']) || + !Number.isInteger(value['quorum']) || + (value['votesReceived'] as number) < 0 || + (value['votesNeeded'] as number) < 0 || + (value['quorum'] as number) < 1 || + !isRecord(value['optionTallies']) + ) { + return false; + } + // Validate the optionTallies map values are non-negative integers. + for (const tally of Object.values( + value['optionTallies'] as Record, + )) { + if (typeof tally !== 'number' || !Number.isInteger(tally) || tally < 0) { + return false; + } + } + return true; +} + +function isPermissionForbiddenData( + value: unknown, +): value is DaemonPermissionForbiddenData { + if ( + !isRecord(value) || + !isNonEmptyString(value['requestId']) || + !isNonEmptyString(value['sessionId']) + ) { + return false; + } + const reason = value['reason']; + if (reason !== 'designated_mismatch' && reason !== 'remote_not_allowed') { + return false; + } + // `clientId` is optional but if present must be a non-empty string. + const clientId = value['clientId']; + if ( + clientId !== undefined && + (typeof clientId !== 'string' || clientId.length === 0) + ) { + return false; + } + return true; +} + function isModelSwitchedData(value: unknown): value is DaemonModelSwitchedData { return ( isRecord(value) && diff --git a/packages/sdk-typescript/src/daemon/index.ts b/packages/sdk-typescript/src/daemon/index.ts index 8800eda53f8..51d28189782 100644 --- a/packages/sdk-typescript/src/daemon/index.ts +++ b/packages/sdk-typescript/src/daemon/index.ts @@ -78,6 +78,11 @@ export type { DaemonPermissionOption, DaemonPermissionAlreadyResolvedData, DaemonPermissionAlreadyResolvedEvent, + // #4175 F3 Commit 7 — multi-client permission coordination push events. + DaemonPermissionForbiddenData, + DaemonPermissionForbiddenEvent, + DaemonPermissionPartialVoteData, + DaemonPermissionPartialVoteEvent, DaemonPermissionRequestData, DaemonPermissionRequestEvent, DaemonPermissionResolvedData, diff --git a/packages/sdk-typescript/src/index.ts b/packages/sdk-typescript/src/index.ts index c9e8281012b..58f32f086fd 100644 --- a/packages/sdk-typescript/src/index.ts +++ b/packages/sdk-typescript/src/index.ts @@ -68,6 +68,10 @@ export { type DaemonPermissionOption, type DaemonPermissionAlreadyResolvedData, type DaemonPermissionAlreadyResolvedEvent, + type DaemonPermissionForbiddenData, + type DaemonPermissionForbiddenEvent, + type DaemonPermissionPartialVoteData, + type DaemonPermissionPartialVoteEvent, type DaemonPermissionRequestData, type DaemonPermissionRequestEvent, type DaemonPermissionResolvedData, diff --git a/packages/sdk-typescript/test/unit/daemonEvents.test.ts b/packages/sdk-typescript/test/unit/daemonEvents.test.ts index 779a9f2a672..fce4bc668fb 100644 --- a/packages/sdk-typescript/test/unit/daemonEvents.test.ts +++ b/packages/sdk-typescript/test/unit/daemonEvents.test.ts @@ -1695,4 +1695,501 @@ describe('PR 21 — auth device-flow events', () => { expect(next.lastToolToggle?.originatorClientId).toBe('data-client'); }); }); + + // F3 Commit 7 — multi-client permission coordination event reducer + // tests. Covers permission_partial_vote / permission_forbidden plus + // the side-effect that resolved/already_resolved events clear + // `permissionVoteProgress` for the matching requestId. + describe('F3 permission coordination events', () => { + it('narrows permission_partial_vote and permission_forbidden via asKnownDaemonEvent', () => { + const partial = asKnownDaemonEvent({ + id: 1, + v: 1, + type: 'permission_partial_vote', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + votesReceived: 1, + votesNeeded: 1, + quorum: 2, + optionTallies: { proceed_once: 1 }, + }, + }); + expect(partial?.type).toBe('permission_partial_vote'); + + const forbidden = asKnownDaemonEvent({ + id: 2, + v: 1, + type: 'permission_forbidden', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + clientId: 'client_B', + reason: 'designated_mismatch', + }, + originatorClientId: 'client_A', + }); + expect(forbidden?.type).toBe('permission_forbidden'); + }); + + it('rejects malformed permission_partial_vote (negative tally)', () => { + expect( + asKnownDaemonEvent({ + id: 3, + v: 1, + type: 'permission_partial_vote', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + votesReceived: 1, + votesNeeded: 1, + quorum: 2, + optionTallies: { proceed_once: -1 }, + }, + }), + ).toBeUndefined(); + }); + + it('rejects malformed permission_forbidden (unknown reason)', () => { + expect( + asKnownDaemonEvent({ + id: 4, + v: 1, + type: 'permission_forbidden', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + reason: 'unauthorized', + }, + }), + ).toBeUndefined(); + }); + + it('reducer accumulates permission_partial_vote into permissionVoteProgress', () => { + const state = reduceDaemonSessionEvents([ + { + id: 1, + v: 1, + type: 'permission_request', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + toolCall: {}, + options: [{ optionId: 'proceed_once' }], + }, + }, + { + id: 2, + v: 1, + type: 'permission_partial_vote', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + votesReceived: 1, + votesNeeded: 1, + quorum: 2, + optionTallies: { proceed_once: 1 }, + }, + }, + ]); + expect(state.permissionVoteProgress['req-1']).toMatchObject({ + votesReceived: 1, + votesNeeded: 1, + quorum: 2, + optionTallies: { proceed_once: 1 }, + }); + expect(Object.keys(state.pendingPermissions)).toEqual(['req-1']); + }); + + it('reducer clears permissionVoteProgress on permission_resolved', () => { + const state = reduceDaemonSessionEvents([ + { + id: 1, + v: 1, + type: 'permission_request', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + toolCall: {}, + options: [{ optionId: 'proceed_once' }], + }, + }, + { + id: 2, + v: 1, + type: 'permission_partial_vote', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + votesReceived: 1, + votesNeeded: 1, + quorum: 2, + optionTallies: { proceed_once: 1 }, + }, + }, + { + id: 3, + v: 1, + type: 'permission_resolved', + data: { + requestId: 'req-1', + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }, + }, + ]); + expect(state.permissionVoteProgress).toEqual({}); + expect(state.pendingPermissions).toEqual({}); + }); + + it('reducer clears permissionVoteProgress on permission_already_resolved', () => { + const state = reduceDaemonSessionEvents([ + { + id: 1, + v: 1, + type: 'permission_request', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + toolCall: {}, + options: [{ optionId: 'proceed_once' }], + }, + }, + { + id: 2, + v: 1, + type: 'permission_partial_vote', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + votesReceived: 1, + votesNeeded: 1, + quorum: 2, + optionTallies: { proceed_once: 1 }, + }, + }, + { + id: 3, + v: 1, + type: 'permission_already_resolved', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }, + }, + ]); + expect(state.permissionVoteProgress).toEqual({}); + }); + + // Wenshao review #4335 / 3271041465 — SDK reconnects mid-permission + // and misses `permission_request`, then sees `permission_partial_vote` + // (stored in permissionVoteProgress). When the matching + // `permission_resolved` arrives, the early-return path on + // unmatched requestId must STILL clear the orphan progress + // entry; otherwise it persists for the lifetime of the session. + it('reducer clears orphan permissionVoteProgress on unmatched permission_resolved (reconnect race)', () => { + const state = reduceDaemonSessionEvents([ + // No permission_request — simulates a client that reconnected + // after the original prompt was dispatched. + { + id: 1, + v: 1, + type: 'permission_partial_vote', + data: { + requestId: 'orphan-req', + sessionId: 'sess-1', + votesReceived: 1, + votesNeeded: 1, + quorum: 2, + optionTallies: { proceed_once: 1 }, + }, + }, + { + id: 2, + v: 1, + type: 'permission_resolved', + data: { + requestId: 'orphan-req', + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }, + }, + ]); + // Pre-fix: permissionVoteProgress['orphan-req'] would still be + // populated because the resolved-handler early-returned without + // touching permissionVoteProgress. Post-fix: cleared. + expect(state.permissionVoteProgress).toEqual({}); + // The unmatched-resolution counter still bumps; that signal + // remains valuable for diagnostics. + expect(state.unmatchedPermissionResolutionCount).toBe(1); + expect(state.lastUnmatchedPermissionResolutionId).toBe('orphan-req'); + }); + + it('reducer clears orphan permissionVoteProgress on unmatched permission_already_resolved (reconnect race)', () => { + const state = reduceDaemonSessionEvents([ + { + id: 1, + v: 1, + type: 'permission_partial_vote', + data: { + requestId: 'orphan-req', + sessionId: 'sess-1', + votesReceived: 1, + votesNeeded: 1, + quorum: 2, + optionTallies: { proceed_once: 1 }, + }, + }, + { + id: 2, + v: 1, + type: 'permission_already_resolved', + data: { + requestId: 'orphan-req', + sessionId: 'sess-1', + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }, + }, + ]); + expect(state.permissionVoteProgress).toEqual({}); + expect(state.unmatchedPermissionResolutionCount).toBe(1); + }); + + it('reducer appends permission_forbidden to bounded forbiddenVotes', () => { + const events: DaemonEvent[] = []; + for (let i = 0; i < 35; i++) { + events.push({ + id: 100 + i, + v: 1, + type: 'permission_forbidden', + data: { + requestId: `req-${i}`, + sessionId: 'sess-1', + clientId: `client_${i}`, + reason: 'designated_mismatch', + }, + originatorClientId: 'prompt-originator', + }); + } + const state = reduceDaemonSessionEvents(events); + // Ring is bounded at 32; total count tracks all events. + expect(state.forbiddenVotes.length).toBe(32); + expect(state.forbiddenVoteCount).toBe(35); + // FIFO eviction — the first 3 entries should have been evicted. + expect(state.forbiddenVotes[0]!.requestId).toBe('req-3'); + expect(state.forbiddenVotes[31]!.requestId).toBe('req-34'); + }); + + // Wenshao review #4335 / 3272576003 — terminal events + // (session_died / session_closed / client_evicted / stream_error) + // must drop forbiddenVotes + forbiddenVoteCount alongside the + // existing pendingPermissions / permissionVoteProgress reset. + // Pre-fix the rejection history would persist on a dead session + // and adapters reading view state would render stale data. + it.each([ + ['session_died', { sessionId: 'sess-1', reason: 'dead' }], + [ + 'session_closed', + { sessionId: 'sess-1', reason: 'client_close' as const }, + ], + [ + 'client_evicted', + { sessionId: 'sess-1', clientId: 'c', reason: 'too-slow' }, + ], + ['stream_error', { sessionId: 'sess-1', error: 'broken' }], + ])( + 'reducer clears forbiddenVotes + forbiddenVoteCount on %s (#4335 / 3272576003)', + (terminalType, terminalData) => { + const state = reduceDaemonSessionEvents([ + { + id: 1, + v: 1, + type: 'permission_forbidden', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + clientId: 'rejected', + reason: 'designated_mismatch', + }, + }, + { + id: 2, + v: 1, + type: terminalType, + data: terminalData, + // eslint-disable-next-line @typescript-eslint/no-explicit-any + } as any, + ]); + expect(state.forbiddenVotes).toEqual([]); + expect(state.forbiddenVoteCount).toBe(0); + expect(state.alive).toBe(false); + }, + ); + + // Wenshao review #4335 / 3270622311 — the SSE envelope's + // `originatorClientId` (= prompt originator per F3 N3) must reach + // view state. Pre-fix, the reducer copied only `event.data` and + // dropped the prompt-originator attribution, leaving SDK consumers + // unable to tell which client's prompt was targeted by the + // partial-vote progress / forbidden vote. + it('reducer stamps prompt-originator on permission_partial_vote view state (mergeOriginator)', () => { + const state = reduceDaemonSessionEvents([ + { + id: 1, + v: 1, + type: 'permission_request', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + toolCall: {}, + options: [{ optionId: 'proceed_once' }], + }, + }, + { + id: 2, + v: 1, + type: 'permission_partial_vote', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + votesReceived: 1, + votesNeeded: 1, + quorum: 2, + optionTallies: { proceed_once: 1 }, + }, + originatorClientId: 'prompt-originator-id', + }, + ]); + expect(state.permissionVoteProgress['req-1']).toMatchObject({ + requestId: 'req-1', + votesReceived: 1, + originatorClientId: 'prompt-originator-id', + }); + }); + + it('reducer stamps prompt-originator on permission_forbidden view state (mergeOriginator)', () => { + const state = reduceDaemonSessionEvents([ + { + id: 1, + v: 1, + type: 'permission_forbidden', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + clientId: 'rejected-voter-id', + reason: 'designated_mismatch', + }, + originatorClientId: 'prompt-originator-id', + }, + ]); + expect(state.forbiddenVotes).toHaveLength(1); + expect(state.forbiddenVotes[0]).toMatchObject({ + requestId: 'req-1', + clientId: 'rejected-voter-id', + originatorClientId: 'prompt-originator-id', + }); + }); + + it('reducer preserves data.originatorClientId over envelope when both present (mergeOriginator)', () => { + // Defensive: mergeOriginator's contract is to PRESERVE any pre- + // existing data.originatorClientId. The daemon does not currently + // populate it, but if a future producer does, the reducer must + // not overwrite it with the envelope value. + const state = reduceDaemonSessionEvents([ + { + id: 1, + v: 1, + type: 'permission_forbidden', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + clientId: 'rejected', + reason: 'designated_mismatch', + originatorClientId: 'data-side-originator', + }, + originatorClientId: 'envelope-side-originator', + }, + ]); + expect(state.forbiddenVotes[0]?.originatorClientId).toBe( + 'data-side-originator', + ); + }); + + it('partial_vote → resolved ordering (M=2 N=2 quorum scenario)', () => { + // F3 N3 ordering invariant — partial_vote frames precede the + // resolved frame for the same requestId. + const state = reduceDaemonSessionEvents([ + { + id: 1, + v: 1, + type: 'permission_request', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + toolCall: {}, + options: [{ optionId: 'proceed_once' }], + }, + }, + { + id: 2, + v: 1, + type: 'permission_partial_vote', + data: { + requestId: 'req-1', + sessionId: 'sess-1', + votesReceived: 1, + votesNeeded: 1, + quorum: 2, + optionTallies: { proceed_once: 1 }, + }, + }, + { + id: 3, + v: 1, + type: 'permission_resolved', + data: { + requestId: 'req-1', + outcome: { outcome: 'selected', optionId: 'proceed_once' }, + }, + }, + ]); + // Reducer end-state: pending cleared, vote progress cleared. + expect(state.permissionVoteProgress).toEqual({}); + expect(state.pendingPermissions).toEqual({}); + }); + + it('rejects malformed permission_partial_vote payload via unrecognizedKnownEventCount counter', () => { + // The reducer's `narrow then no-op` path: when the event type IS + // in `KNOWN_EVENT_TYPES` but its data fails the type-guard + // (e.g. missing required fields, negative tally), the reducer + // bumps `unrecognizedKnownEventCount` rather than crashing. + const state = reduceDaemonSessionEvent(createDaemonSessionViewState(), { + id: 9, + v: 1, + type: 'permission_partial_vote', + data: { malformed: true }, + }); + expect(state.unrecognizedKnownEventCount).toBe(1); + expect(state.permissionVoteProgress).toEqual({}); + }); + + it('forward-compat: emits unknown event types fall through silently (no counter bump)', () => { + // R3-11 (final review fold-in) — true forward-compat path. The + // reducer's `unrecognizedKnownEventCount` only fires for types + // that ARE in `KNOWN_EVENT_TYPES` (i.e. the daemon and SDK + // agree on the type but disagree on the shape). For unknown + // types — e.g. a future daemon emitting `permission_unknown_v2` + // before the SDK ships support — the reducer must silently + // ignore (track only `lastEventId`) so the SDK can keep + // streaming through unknown events without piling up false + // unrecognized-known counts. + const state = reduceDaemonSessionEvent(createDaemonSessionViewState(), { + id: 99, + v: 1, + type: 'permission_unknown_v2_future', + data: { whatever: 'shape' }, + }); + expect(state.unrecognizedKnownEventCount).toBe(0); + expect(state.lastEventId).toBe(99); + }); + }); }); diff --git a/packages/vscode-ide-companion/schemas/settings.schema.json b/packages/vscode-ide-companion/schemas/settings.schema.json index bc261e266c4..69c11977192 100644 --- a/packages/vscode-ide-companion/schemas/settings.schema.json +++ b/packages/vscode-ide-companion/schemas/settings.schema.json @@ -752,6 +752,27 @@ } } }, + "policy": { + "description": "Daemon multi-client coordination policies. Tool-level allow/deny rules live under `permissions`; this section is for runtime mediation behavior between concurrent HTTP clients sharing one `qwen serve` daemon.", + "type": "object", + "properties": { + "permissionStrategy": { + "description": "How permission requests resolve when multiple clients are attached. `first-responder` (default) = any client decides, first wins. `designated` = only the prompt originator decides; falls back to first-responder if originator is anonymous. NOTE: client identity comes from self-declared X-Qwen-Client-Id with no proof-of-possession (pair-token mechanism is a future PR), so any client observing originatorClientId on SSE frames can register with the same id and impersonate the originator. `consensus` = N-of-M voters must agree. Default N=floor(M/2)+1, which means UNANIMITY for M=2 (quorum=2, both must agree) and supermajority for larger even M (M=4 → quorum=3; M=6 → quorum=4). For M=2 specifically, split votes resolve only via permissionTimeoutMs. `local-only` = only loopback clients can RESOLVE; remote clients can still ABORT a pending permission via the cancel sentinel ({outcome:\"cancelled\"}) — F3 v1 keeps cancel cross-policy for consistency. Strict-cancel-too deployments need a dedicated loopback-bound daemon. Requires daemon restart — F3 v1 reads this once at boot. Options: first-responder, designated, consensus, local-only", + "enum": [ + "first-responder", + "designated", + "consensus", + "local-only" + ], + "default": "first-responder" + }, + "consensusQuorum": { + "type": "integer", + "minimum": 1, + "description": "Optional fixed quorum size for consensus policy. Capped at M (count of registered voters at request issue time) to prevent unreachable quorum. Unset = floor(M/2)+1. Requires daemon restart — F3 v1 reads this once at boot." + } + } + }, "mcp": { "description": "Settings for Model Context Protocol (MCP) servers.", "type": "object",