From d099a24cb13dc55b1dcbcd18ed10135834f8919c Mon Sep 17 00:00:00 2001 From: Yichen Jiang Date: Fri, 7 Aug 2026 11:45:10 +0800 Subject: [PATCH] fix(web): resume the preset the log records, and serialize the switch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six review findings on the select surface, all reachable from the wire: **Resume read the header, not the log.** The switch was recorded as `agent-preset/selected` and every projection resolved from it, but `agentFor` still composed from `inspected.meta.agentPreset` — the value written once at creation. A blank session that switched and then ran turns came back after a restart under the ORIGINAL preset, restoring that history under the tool set it was not produced with, which is the mismatch this feature exists to prevent. `inspected` already carries the events. **Cold summaries dropped the preset entirely.** `summarizeCold` hand-copied three header fields and omitted the fourth, so a restored session reported no preset and the picker showed the deployment default. It now uses the same projection the attached path does. **`select` had no gate.** Two concurrent selects both passed the blank check; the second `unmountPresetFor` then found no record, because the first had already removed it, and both mounts installed into one agent layer. Selects on one session now queue, and the blank check is re-read inside the queue. This is not turn admission — a `session.prompt` racing a switch is the agent loop's to reserve — but it closes the select-versus-select tear-down. **A same-id restore was skipped.** The roster is a live directory, so "the same inputs that worked a moment ago" does not hold: a changed file is exactly how a same-id reselect fails, and skipping the restore left the agent with no composition at all. **`writable` was dead state**, initialized true and never set, so the row could never disable. It now carries `settings.describe`'s bit — a browser that may not write settings sees the current default and no control, rather than one whose write answers `settings-not-exposed`. **`list` was documented as id-ordered.** It is root-precedence order with each root's own presets sorted, first root to supply an id winning. --- .../src/client/settings-store.ts | 14 +++ .../tests/settings-store.spec.ts | 25 ++++- packages/host/apiproxy/src/api-proxy.ts | 105 +++++++++++------- .../host/apiproxy/src/api/agent-presets.ts | 6 +- .../tests/api-proxy-agent-preset.spec.ts | 42 ++++++- packages/preset/agent-presets/src/index.ts | 11 +- 6 files changed, 159 insertions(+), 44 deletions(-) diff --git a/packages/client/ui-agent-preset/src/client/settings-store.ts b/packages/client/ui-agent-preset/src/client/settings-store.ts index fcc092d179..6c35dcdccb 100644 --- a/packages/client/ui-agent-preset/src/client/settings-store.ts +++ b/packages/client/ui-agent-preset/src/client/settings-store.ts @@ -25,6 +25,12 @@ export interface AgentPresetOption { export interface AgentPresetSettingsState { status: 'idle' | 'loading' | 'ready' | 'saving' | 'unavailable' | 'error' error: string | null + /** + * Whether this browser may persist the choice at all. `settings.describe` is + * loopback-only and reports a read-only provider as `writable: false`; the + * row then shows the current default and disables the control rather than + * offering a write the gateway will refuse. + */ writable: boolean currentValue: string options: readonly AgentPresetOption[] @@ -33,6 +39,8 @@ export interface AgentPresetSettingsState { const INITIAL: AgentPresetSettingsState = { status: 'idle', error: null, + // Assumed until `load()` asks; a row that has not read yet renders nothing + // interactive anyway (status 'idle'). writable: true, currentValue: '', options: [], @@ -69,9 +77,15 @@ export class AgentPresetSettingsController { this.set({ status: 'unavailable', options: [], currentValue: '' }) return } + // The roster says what may be chosen; `settings.describe` says whether + // this browser may write the choice down. A non-loopback browser reaches + // neither method, so a refused describe leaves the row read-only rather + // than offering a control whose write answers `settings-not-exposed`. + const described = await this.api.settings.describe({}) this.set({ status: 'ready', error: null, + writable: described.result.ok && described.result.value.writable, options: presets.map(preset => ({ id: preset.id, trust: preset.trust })), currentValue: presets.find(preset => preset.isDefault)?.id ?? presets[0]?.id ?? '', }) diff --git a/packages/client/ui-agent-preset/tests/settings-store.spec.ts b/packages/client/ui-agent-preset/tests/settings-store.spec.ts index e8d8c19d62..16c7f8cc99 100644 --- a/packages/client/ui-agent-preset/tests/settings-store.spec.ts +++ b/packages/client/ui-agent-preset/tests/settings-store.spec.ts @@ -17,7 +17,7 @@ interface Recorded { ns: string; patch: unknown } /** A client whose roster and write outcome the test controls. */ function fakeApi( presets: { id: string; trust: 'system' | 'user'; isDefault: boolean }[], - options: { writes?: Recorded[]; failWrite?: string; failList?: string } = {}, + options: { writes?: Recorded[]; failWrite?: string; failList?: string; readOnly?: boolean } = {}, ): IApiClient { return { agentPresets: { @@ -26,6 +26,15 @@ function fakeApi( : { rpcId: 'r', result: { ok: false as const, error: { code: 'internal', message: options.failList, details: {} } } }), }, settings: { + // Loopback-only in production; a read-only provider answers writable:false + // and the row disables its control instead of offering a refused write. + describe: () => Promise.resolve({ + rpcId: 'r', + result: { + ok: true as const, + value: { writable: options.readOnly !== true, hasDocument: true, namespaces: [] }, + }, + }), update: (payload: { ns: string; patch: unknown }) => { options.writes?.push({ ns: payload.ns, patch: payload.patch }) if (options.failWrite !== undefined) { @@ -42,6 +51,20 @@ function fakeApi( } describe('the agent-preset settings controller', () => { + it('disables the control when this browser may not write settings', async () => { + const controller = new AgentPresetSettingsController(fakeApi([ + { id: 'standard', trust: 'system', isDefault: true }, + ], { readOnly: true })) + + await controller.load() + + // `settings.describe` is loopback-only and reports a read-only provider; + // offering a control whose write answers `settings-not-exposed` would + // promise a switch the host refuses. + expect(controller.store.getSnapshot().writable).toBe(false) + expect(controller.store.getSnapshot().currentValue).toBe('standard') + }) + it('derives options and the current default from one roster call', async () => { const controller = new AgentPresetSettingsController(fakeApi([ { id: 'standard', trust: 'system', isDefault: true }, diff --git a/packages/host/apiproxy/src/api-proxy.ts b/packages/host/apiproxy/src/api-proxy.ts index dbeffdb608..ba0e2e43be 100644 --- a/packages/host/apiproxy/src/api-proxy.ts +++ b/packages/host/apiproxy/src/api-proxy.ts @@ -319,12 +319,14 @@ async function summarizeCold( // a cold log to check for turns would defeat the index read, so a listed // cold session is served as not-blank (its log holds its conversation). blank: false, - ...meta.parentSession === undefined ? {} : { parentSessionId: meta.parentSession }, - ...meta.origin === undefined ? {} : { origin: meta.origin }, - /* v8 ignore next -- the empty arm needs a cwd-less meta, but list() - filters those out (legacy logs are not served); the conditional mirrors - summarize() shape. */ - ...meta.cwd === undefined ? {} : { cwd: meta.cwd }, + // The same projection the attached path uses. Hand-copying the header here + // is how `agentPreset` went missing from cold rows while `summarize()` + // served it — a restored session then read as preset-less and the picker + // showed the deployment default instead of what the session runs. With no + // events to read (a cold row never loads its log, see `blank` above), this + // resolves to the header's value; a switch recorded while blank surfaces + // once the session attaches. + ...sessionListFields(meta), } } @@ -745,6 +747,14 @@ export function createApiProxy(ctx: Context, defaults: ApiProxyDefaults): ApiPro const targets = new WeakMap() /** Implicit resume of cold sessions, deduplicating concurrent calls (follows the jsonrpc sessionCreations precedent). */ const resumes = new Map>() + /** + * Serializes `agentPreset.select` per session. Two concurrent selects both + * pass the blank check, and the second `unmountPresetFor` then finds nothing + * to unmount because the first already removed the record — leaving two + * compositions registered into one agent layer. The client's `busy` flag is + * not enforcement: the wire is reachable directly. + */ + const presetSwitches = new Map>() /** Client-chosen identity creation/resume, deduplicated across concurrent retries. */ const sessionCreations = new Map>() /** Serializes path ownership and explicit title checks with Workspace mutations. */ @@ -1119,7 +1129,14 @@ export function createApiProxy(ctx: Context, defaults: ApiProxyDefaults): ApiPro const handle = await ctx.agents.resume({ resumeSessionId: sessionId, agentOptions, - setup: (await composeAgent(inspected.meta.agentPreset)).setup, + // Resolved from the LOG, not the header: a session that switched + // while blank ran its turns under the newer composition, and the + // header is written once at creation. Reading the header here + // would silently undo the switch on the next restart and restore + // that history under the old tool set. + setup: (await composeAgent( + resolveSessionPreset({ header: inspected.meta, events: inspected.events }), + )).setup, }) return handle.agent } finally { @@ -2528,39 +2545,51 @@ export function createApiProxy(ctx: Context, defaults: ApiProxyDefaults): ApiPro const found = await agentFor(sessionId) if ('error' in found) return err(request, found.error) const { agent } = found - if (!sessionBlank(agent.session)) { - return err(request, { - code: 'agent-preset-locked', - message: `session "${sessionId}" has already started; its agent preset is fixed`, - details: { sessionId, agentPreset }, - }) + const swap = async (): Promise> => { + // Re-read inside the queue: an earlier switch may have run, and a + // conversation may have started, since this request arrived. + if (!sessionBlank(agent.session)) { + return err(request, { + code: 'agent-preset-locked', + message: `session "${sessionId}" has already started; its agent preset is fixed`, + details: { sessionId, agentPreset }, + }) + } + try { + const preset = await presets.recompose(agent.ctx, agentPreset) + // Recorded only after the swap committed: the log states what the + // agent runs, and a rejected mount leaves the previous composition. + agent.session.append('agent-preset/selected', { agentPreset: preset.id }) + return ok(request, { agentPreset: preset.id }) + } catch (error: unknown) { + if (error instanceof UnknownPresetError) { + return err(request, { + code: 'agent-preset-not-found', + message: error.message, + details: { agentPreset: error.presetId, available: [...error.available] }, + }) + } + if (error instanceof PresetMountError) { + return err(request, { + code: 'agent-preset-invalid', + message: error.message, + details: { agentPreset: error.presetId, reason: error.reason }, + }) + } + return err(request, { + code: 'internal', + message: `failed to select agent preset "${agentPreset}": ${String(error)}`, + details: {}, + }) + } } + const queued = presetSwitches.get(sessionId) ?? Promise.resolve() + const turn = queued.then(swap) + presetSwitches.set(sessionId, turn.catch(() => undefined)) try { - const preset = await presets.recompose(agent.ctx, agentPreset) - // Recorded only after the swap committed: the log states what the - // agent runs, and a rejected mount leaves the previous composition. - agent.session.append('agent-preset/selected', { agentPreset: preset.id }) - return ok(request, { agentPreset: preset.id }) - } catch (error: unknown) { - if (error instanceof UnknownPresetError) { - return err(request, { - code: 'agent-preset-not-found', - message: error.message, - details: { agentPreset: error.presetId, available: [...error.available] }, - }) - } - if (error instanceof PresetMountError) { - return err(request, { - code: 'agent-preset-invalid', - message: error.message, - details: { agentPreset: error.presetId, reason: error.reason }, - }) - } - return err(request, { - code: 'internal', - message: `failed to select agent preset "${agentPreset}": ${String(error)}`, - details: {}, - }) + return await turn + } finally { + if (presetSwitches.get(sessionId) === turn) presetSwitches.delete(sessionId) } }, }, diff --git a/packages/host/apiproxy/src/api/agent-presets.ts b/packages/host/apiproxy/src/api/agent-presets.ts index 83630d1dc3..3471c06f6d 100644 --- a/packages/host/apiproxy/src/api/agent-presets.ts +++ b/packages/host/apiproxy/src/api/agent-presets.ts @@ -24,7 +24,11 @@ export interface AgentPresetEntry { /** agent-preset-domain unary methods (the map key agentPreset.* of RpcMethodMap). */ export interface AgentPresetsApi { /** - * Lists every preset the deployment currently supplies, ordered by id. + * Lists every preset the deployment currently supplies, in root-precedence + * order — the roots as configured, each root's own presets sorted by id, + * and the first root to supply an id wins. The order is not globally + * sorted: a user root's preset sits in that root's block, not among the + * shipped ids. * An empty roster means the deployment composes no presets at all, and * every session shares the host composition. */ diff --git a/packages/host/apiproxy/tests/api-proxy-agent-preset.spec.ts b/packages/host/apiproxy/tests/api-proxy-agent-preset.spec.ts index 5473d3ec63..95810a6c7c 100644 --- a/packages/host/apiproxy/tests/api-proxy-agent-preset.spec.ts +++ b/packages/host/apiproxy/tests/api-proxy-agent-preset.spec.ts @@ -14,7 +14,7 @@ import type { Agent } from '@deepseek-ai/dsh-agent' import SessionStore, { SessionId, type Session } from '@deepseek-ai/dsh-session' import UserInteractionService from '@deepseek-ai/dsh-user-interaction' import { RpcId, type RpcRequest } from '../src/api/rpc.ts' -import { UnknownPresetError } from '@deepseek-ai/dsh-agent-presets' +import { resolveSessionPreset, UnknownPresetError } from '@deepseek-ai/dsh-agent-presets' import { GoalId } from '@deepseek-ai/dsh-goal' import { createApiProxy } from '../src/api-proxy.ts' import { describe, expect, it } from 'vitest' @@ -280,6 +280,46 @@ describe('agentPreset.select', () => { expect(response.result.value.agentPreset).toBe('core-web') }) + it('records the switch in the log, and the list reads it back', async () => { + const { api, ctx } = await harness(['standard', 'core-web']) + await api.sessions.create(request({ sessionId: SessionId('sel-log'), agentPreset: 'standard' })) + + await api.agentPresets.select( + request({ sessionId: SessionId('sel-log'), agentPreset: 'core-web' })) + + // The header is written once at creation, so the switch lives in the log — + // this is what a restart replays and what every projection resolves from. + // Asserting only the RPC's echo would miss a switch that never persisted. + const session = ctx.sessions.get(SessionId('sel-log')) + if (session === undefined) throw new Error('unreachable') + expect(session.header.agentPreset).toBe('standard') + expect(resolveSessionPreset(session)).toBe('core-web') + const listed = await api.sessions.list(request({})) + if (!listed.result.ok) throw new Error('unreachable') + expect(listed.result.value.items.find(item => item.sessionId === 'sel-log')?.agentPreset) + .toBe('core-web') + }) + + it('serializes two concurrent selects on one session', async () => { + const { api, ctx } = await harness(['standard', 'core-web']) + await api.sessions.create(request({ sessionId: SessionId('sel-race'), agentPreset: 'standard' })) + + // Both pass the blank check; unserialized, the second unmount finds no + // record because the first already removed it, and two compositions end up + // in one agent layer. The client's busy flag is not enforcement. + const [first, second] = await Promise.all([ + api.agentPresets.select(request({ sessionId: SessionId('sel-race'), agentPreset: 'core-web' })), + api.agentPresets.select(request({ sessionId: SessionId('sel-race'), agentPreset: 'standard' })), + ]) + + expect(first.result.ok).toBe(true) + expect(second.result.ok).toBe(true) + const session = ctx.sessions.get(SessionId('sel-race')) + if (session === undefined) throw new Error('unreachable') + // One winner, and the log agrees with it: the last committed switch. + expect(resolveSessionPreset(session)).toBe('standard') + }) + it('refuses once the conversation has started', async () => { const { api, ctx } = await harness(['standard', 'core-web']) await api.sessions.create(request({ sessionId: SessionId('sel-2'), agentPreset: 'standard' })) diff --git a/packages/preset/agent-presets/src/index.ts b/packages/preset/agent-presets/src/index.ts index dbc9b1d9ba..f2bc932769 100644 --- a/packages/preset/agent-presets/src/index.ts +++ b/packages/preset/agent-presets/src/index.ts @@ -190,11 +190,16 @@ export class AgentPresets extends Service { try { await mountPreset(agentCtx, preset) } catch (error) { - if (previous !== undefined && previous !== preset.id) { + if (previous !== undefined) { + // Restored unconditionally, same id included: the roster is a live + // directory, so "the same inputs that worked a moment ago" does not + // hold — the file may have changed between the original mount and + // this one, which is exactly how a same-id reselect fails. Skipping + // the restore there left the agent with no composition at all. await this.mount(agentCtx, previous).catch(() => { // The agent now has no composition, but the switch failure below is - // the actionable diagnostic and the restore had the same inputs that - // worked a moment ago; reporting its failure instead would hide why. + // the actionable diagnostic; reporting the restore's instead would + // hide why the switch was attempted and what the operator must fix. }) } throw error