From 6793b89b90ce907489e0e2af8e83a992e082108e Mon Sep 17 00:00:00 2001 From: doudouOUC Date: Wed, 20 May 2026 17:56:59 +0800 Subject: [PATCH] =?UTF-8?q?fix(f3):=20Round=209=20=E2=80=94=20terminal-eve?= =?UTF-8?q?nt=20forbidden=20cleanup=20+=207=20hardening=20fixes=20(#4335)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 8 follow-up findings from wenshao Round 9 (4 separate review records: 4326832742 / 4326833568 / 4326844430 / 4326851074, the last one a non-blocking comment review). 1 Critical + 7 Suggestions. **[Critical] Terminal events leaked forbiddenVotes history** (3272576003). `session_died` / `session_closed` / `client_evicted` / `stream_error` reducer cases cleared `pendingPermissions` and `permissionVoteProgress` but not `forbiddenVotes` / `forbiddenVoteCount`. Adapters reading view state for a dead session would render stale rejection data. All 4 cases now zero out the rejection ring + counter. Parameterized regression test asserts the cleanup contract. **[Suggestion] safeAudit JSDoc was orphaned over writeForbiddenStderr** (3272567323). Two consecutive JSDoc blocks were stacked back-to-back but the method definitions followed in the opposite order, so IDE hover and API doc generation showed `safeAudit`'s docs as `writeForbiddenStderr`'s. Reordered method definitions so each JSDoc precedes its actual method. **[Suggestion] writeForbiddenStderr had no test coverage** (3272568031). Added a 3-path test (designated / consensus / local-only) that spies on `process.stderr.write` and asserts each breadcrumb contains the expected reason fragment plus the requestId + sessionId for grep-ability. Pins the format so a future refactor can't silently drop the line. **[Suggestion] resolveEntry numbered list contradicted code** (3272581553). The N2-invariant cleanup ladder docstring bundled "delete from pending + write to resolved" into step 2 ahead of the SSE emit, but the actual code defers `rememberResolved` until AFTER `safeEmit` (the I5 inline comment on line 1103 correctly explains this). Split step 2 into two halves around the emit so the spec faithfully describes the ordering invariant. **[Suggestion] Dead exports in bridgeClient.ts** (3272581548). `MAX_RESOLVED_PERMISSION_RECORDS`, `PendingPermission`, and `PermissionResolutionRecord` were defined and exported but no longer referenced — the mediator owns the same state under different names (`permissionMediator.ts:77` / `:319`). The JSDoc still pointed at deleted closures (`registerPending`, `resolvedPermissions` map). Removed all three definitions and the matching re-exports in `cli/src/serve/httpAcpBridge.ts`. **[Suggestion] detectFromLoopback prefix-match had no direct test** (3272581557). Supertest in the broader server.test.ts suite always connects from `127.0.0.1`, so the Round-5 prefix-match fix for `127.x`-beyond-`.0.0.1`, `::1`, `::ffff:127.*`, and the fail-closed branches had no coverage. Exported the helper from `server.ts` (loosened parameter type to a minimal shape so tests don't need to spin up Express) and added an `it.each` table covering the variants the fix targets, plus an explicit "does NOT consult X-Forwarded-For" assertion as a security pin. **[Suggestion] Validate-policies set is a 4th hardcoded copy** (3272581563). The policy literals already exist in 3 places — `PermissionPolicy` type, `SERVE_CAPABILITY_REGISTRY.permission_ mediation.modes`, and `settingsSchema.ts` enum options. `validatePolicyConfig` now derives its valid-set from `SERVE_CAPABILITY_REGISTRY.permission_mediation.modes` (single runtime source of truth). Adding a 5th policy upstream lands in one place; a future drift between the registry and the type union would still surface at the `as PermissionPolicy` cast. **[Suggestion] BridgeClient over-coupled to MultiClientPermissionMediator** (3272581569). `BridgeClient` only ever calls `mediator.request()` but its field was typed as the concrete class, forcing every test stub to fake all 6 mediator members. Narrowed the field type to `Pick` (the frozen interface from `permission.ts`); the bridge factory still passes the full `MultiClientPermissionMediator` instance via structural typing. Test stubs simplified from 6 placeholder members to 1. 🤖 Generated with [Qwen Code](https://github.com/QwenLM/qwen-code) --- packages/acp-bridge/src/bridgeClient.test.ts | 25 +++----- packages/acp-bridge/src/bridgeClient.ts | 59 ++++++------------ .../acp-bridge/src/permissionMediator.test.ts | 59 ++++++++++++++++++ packages/acp-bridge/src/permissionMediator.ts | 56 ++++++++++------- packages/cli/src/serve/httpAcpBridge.ts | 16 +++-- packages/cli/src/serve/runQwenServe.ts | 31 +++++++--- packages/cli/src/serve/server.test.ts | 62 ++++++++++++++++++- packages/cli/src/serve/server.ts | 10 ++- packages/sdk-typescript/src/daemon/events.ts | 16 +++++ .../test/unit/daemonEvents.test.ts | 46 ++++++++++++++ 10 files changed, 284 insertions(+), 96 deletions(-) diff --git a/packages/acp-bridge/src/bridgeClient.test.ts b/packages/acp-bridge/src/bridgeClient.test.ts index f7200d9fce..e139d95067 100644 --- a/packages/acp-bridge/src/bridgeClient.test.ts +++ b/packages/acp-bridge/src/bridgeClient.test.ts @@ -58,14 +58,12 @@ function makeClient(fileSystem?: BridgeFileSystem): BridgeClient { const noPermissionFlow = () => { throw new Error('test: permission flow should not run in fs-path tests'); }; - const throwerMediator = { - policy: 'first-responder', - request: noPermissionFlow, - vote: noPermissionFlow, - forgetSession: noPermissionFlow, - peekSessionFor: noPermissionFlow, - pendingCount: 0, - } as never; + // 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 @@ -226,14 +224,9 @@ describe('BridgeClient — requestPermission pre-publish collision guard', () => const noPermissionFlow = () => { throw new Error('test: not reachable on collision-throw path'); }; - const throwerMediator = { - policy: 'first-responder', - request: noPermissionFlow, - vote: noPermissionFlow, - forgetSession: noPermissionFlow, - peekSessionFor: noPermissionFlow, - pendingCount: 0, - } as never; + // 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, diff --git a/packages/acp-bridge/src/bridgeClient.ts b/packages/acp-bridge/src/bridgeClient.ts index 77f3cdd1d3..4b8b911afb 100644 --- a/packages/acp-bridge/src/bridgeClient.ts +++ b/packages/acp-bridge/src/bridgeClient.ts @@ -20,7 +20,14 @@ import type { import type { BridgeEvent, EventBus } from './eventBus.js'; import type { BridgeFileSystem } from './bridgeFileSystem.js'; import { CANCEL_VOTE_SENTINEL } from './permissionMediator.js'; -import type { MultiClientPermissionMediator } 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, @@ -47,13 +54,15 @@ function resolutionToAcpResponse( return { outcome: { outcome: 'cancelled' } }; } -/** - * 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. - */ -export const MAX_RESOLVED_PERMISSION_RECORDS = 512; +// 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 @@ -127,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 @@ -235,7 +212,7 @@ export class BridgeClient implements Client { * fan-out live inside the mediator. Replaces the pre-F3 * `registerPending` / `rollbackPending` callbacks. */ - private readonly mediator: MultiClientPermissionMediator, + private readonly mediator: Pick, /** * Bd1yh: wall-clock ms before `requestPermission` resolves as * cancelled if no client vote arrives. 0 = disabled. Prevents diff --git a/packages/acp-bridge/src/permissionMediator.test.ts b/packages/acp-bridge/src/permissionMediator.test.ts index 7f7fdb9938..d39d34025b 100644 --- a/packages/acp-bridge/src/permissionMediator.test.ts +++ b/packages/acp-bridge/src/permissionMediator.test.ts @@ -814,6 +814,65 @@ describe('MultiClientPermissionMediator — consensus', () => { 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 diff --git a/packages/acp-bridge/src/permissionMediator.ts b/packages/acp-bridge/src/permissionMediator.ts index 55de743f10..25969496e9 100644 --- a/packages/acp-bridge/src/permissionMediator.ts +++ b/packages/acp-bridge/src/permissionMediator.ts @@ -1068,13 +1068,26 @@ export class MultiClientPermissionMediator implements PermissionMediator { /** * 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` and write to `resolved` (state first). + * 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). - * 4. audit.recordResolved (best-effort, same). - * 5. Settle the Promise (LAST — callbacks running re-entrantly + * 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 @@ -1184,23 +1197,6 @@ export class MultiClientPermissionMediator implements PermissionMediator { } } - /** - * 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). - * - * I4 (Commit 3 review) — log unexpected throws as `console.error` - * so audit-publisher bugs are visible without breaking the - * Promise-settle invariant. - */ /** * DeepSeek review #4335 / 3271627457 — emit a stderr breadcrumb * for every vote rejection (the three forbidden paths in @@ -1237,6 +1233,24 @@ export class MultiClientPermissionMediator implements PermissionMediator { } } + /** + * 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(); diff --git a/packages/cli/src/serve/httpAcpBridge.ts b/packages/cli/src/serve/httpAcpBridge.ts index 0a7c4b9a00..c1127da600 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, diff --git a/packages/cli/src/serve/runQwenServe.ts b/packages/cli/src/serve/runQwenServe.ts index d991a42bc3..7f9a200bab 100644 --- a/packages/cli/src/serve/runQwenServe.ts +++ b/packages/cli/src/serve/runQwenServe.ts @@ -24,6 +24,11 @@ import { 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 @@ -74,6 +79,12 @@ export class InvalidPolicyConfigError extends Error { * * 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 } = {}, @@ -82,13 +93,19 @@ export function validatePolicyConfig( permissionPolicy: PermissionPolicy | undefined; permissionConsensusQuorum: number | undefined; } { - const _validPolicies: readonly PermissionPolicy[] = [ - 'first-responder', - 'designated', - 'consensus', - 'local-only', - ]; - const validSet: ReadonlySet = new Set(_validPolicies); + // 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) diff --git a/packages/cli/src/serve/server.test.ts b/packages/cli/src/serve/server.test.ts index 7739a570e0..4567acebed 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, @@ -756,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', () => { diff --git a/packages/cli/src/serve/server.ts b/packages/cli/src/serve/server.ts index 2db07c07e4..4a2bb10616 100644 --- a/packages/cli/src/serve/server.ts +++ b/packages/cli/src/serve/server.ts @@ -2273,7 +2273,15 @@ function parseClientIdHeader( * (missing socket, non-string) returns `false`, gating votes * conservatively rather than admitting them. */ -function detectFromLoopback(req: import('express').Request): boolean { +// 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). diff --git a/packages/sdk-typescript/src/daemon/events.ts b/packages/sdk-typescript/src/daemon/events.ts index f2e8c1919a..a3a2d7bf2c 100644 --- a/packages/sdk-typescript/src/daemon/events.ts +++ b/packages/sdk-typescript/src/daemon/events.ts @@ -1110,6 +1110,13 @@ export function reduceDaemonSessionEvent( 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 { @@ -1119,6 +1126,9 @@ export function reduceDaemonSessionEvent( terminalEvent: chooseTerminalEvent(base.terminalEvent, event), pendingPermissions: {}, permissionVoteProgress: {}, + // Wenshao review #4335 / 3272576003 (Critical) — see session_died. + forbiddenVotes: [], + forbiddenVoteCount: 0, }; case 'session_metadata_updated': return { @@ -1133,6 +1143,9 @@ export function reduceDaemonSessionEvent( 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 @@ -1152,6 +1165,9 @@ export function reduceDaemonSessionEvent( 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 diff --git a/packages/sdk-typescript/test/unit/daemonEvents.test.ts b/packages/sdk-typescript/test/unit/daemonEvents.test.ts index 456385b239..fce4bc668f 100644 --- a/packages/sdk-typescript/test/unit/daemonEvents.test.ts +++ b/packages/sdk-typescript/test/unit/daemonEvents.test.ts @@ -1979,6 +1979,52 @@ describe('PR 21 — auth device-flow events', () => { 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