diff --git a/packages/client/connection/src/client/api.ts b/packages/client/connection/src/client/api.ts index 25507c1c08..cc9d2a1eb9 100644 --- a/packages/client/connection/src/client/api.ts +++ b/packages/client/connection/src/client/api.ts @@ -14,7 +14,7 @@ export type { ModelCatalogFailure, ModelCatalogModel, ModelProviderGroup, ModelReasoning, InboxItemId, ModelReasoningEffort, ModelTarget, QueueAction, QueuedInboxItem, SessionModels, GoalsApi, GoalRef, - SettingsApi, SettingsNamespaceView, SettingsSecretView, + SettingsApi, SettingsNamespaceView, SettingsPathOpView, SettingsSecretView, CredentialsApi, CredentialView, ConfigurableProviderView, LlmApi, } from '@deepseek-ai/dsh-host-apiproxy/api' export type { ToolCallView, ToolResultView } from '@deepseek-ai/dsh-tools/presentation' diff --git a/packages/client/connection/src/client/fixture.ts b/packages/client/connection/src/client/fixture.ts index 4e737e4e7f..d3106fef66 100644 --- a/packages/client/connection/src/client/fixture.ts +++ b/packages/client/connection/src/client/fixture.ts @@ -1545,6 +1545,11 @@ export function createFixtureApi(options: FixtureOptions = {}): ApiProxy { message: 'fixture: no settings namespaces are registered', details: { ns: request.payload.ns }, }), + mutate: request => err(request, { + code: 'settings-rejected', + message: 'fixture: no settings namespaces are registered', + details: { ns: request.payload.ns }, + }), }, credentials: { describe: request => ok(request, { @@ -1668,6 +1673,7 @@ export class FixtureApiClient extends AbstractApiClient { case 'settings.describe': return this.api.settings.describe(request) case 'settings.update': return this.api.settings.update(request) case 'settings.replace': return this.api.settings.replace(request) + case 'settings.mutate': return this.api.settings.mutate(request) case 'credentials.describe': return this.api.credentials.describe(request) case 'credentials.set': return this.api.credentials.set(request) case 'credentials.unset': return this.api.credentials.unset(request) diff --git a/packages/client/connection/src/client/index.ts b/packages/client/connection/src/client/index.ts index dabc85dc94..93f5153936 100644 --- a/packages/client/connection/src/client/index.ts +++ b/packages/client/connection/src/client/index.ts @@ -22,7 +22,7 @@ export type { ClientRequest, ServerResponse, ServerRequest, ClientResponse, RpcMessage, RpcReceipt, IApiClient, SessionId, SessionEvent, ContentBlock, StreamChunk, GoalsApi, GoalRef, - SettingsApi, SettingsNamespaceView, SettingsSecretView, + SettingsApi, SettingsNamespaceView, SettingsPathOpView, SettingsSecretView, CredentialsApi, CredentialView, ConfigurableProviderView, LlmApi, } from './api.ts' export { RpcId, AbstractApiClient, transportError } from './api.ts' diff --git a/packages/client/connection/src/index.ts b/packages/client/connection/src/index.ts index b14a591646..cedc7d86e7 100644 --- a/packages/client/connection/src/index.ts +++ b/packages/client/connection/src/index.ts @@ -35,16 +35,26 @@ export const Config: z = z.object({ /** * Methods gated to loopback even on a trusted-host deployment. Native dialogs - * act on the host machine; settings and credential writes mutate the user's - * configuration and secret store. A declared `trustedHosts` authority reaches - * every other method, but these stay loopback-same-origin until a real - * authentication layer exists. + * act on the host machine; the settings and credential domains mutate the + * user's configuration and secret store, and READING them is equally + * privileged — `settings.describe` returns every exposed namespace's + * configuration and `credentials.describe` reports whether an arbitrary + * environment-variable name is configured and where from, which is + * reconnaissance no anonymous caller should have. `trustedHosts` is a + * DNS-rebinding fence, explicitly not authentication, so the whole + * configuration plane stays loopback-same-origin until a real authentication + * layer exists. The model catalog (`llm.providers`, `llm.models`) is + * deliberately NOT here: it carries provider ids, display names, and model + * lists — no endpoints, keys, or key state — and a LAN client's model picker + * legitimately needs it. */ const PRIVILEGED_METHODS = new Set([ 'host.pickDirectory', 'host.openPath', + 'settings.describe', 'settings.update', 'settings.replace', + 'credentials.describe', 'credentials.set', 'credentials.unset', ]) diff --git a/packages/client/connection/tests/fake-api.ts b/packages/client/connection/tests/fake-api.ts index 56053009b6..2ed688d555 100644 --- a/packages/client/connection/tests/fake-api.ts +++ b/packages/client/connection/tests/fake-api.ts @@ -158,6 +158,7 @@ export class FakeApiClient implements IApiClient { describe: payload => this.record('settings.describe', payload, Promise.resolve(ok({ writable: true, namespaces: [] }))), update: payload => this.record('settings.update', payload, Promise.resolve(ok({ ns: 'fake', schema: {}, value: {}, applies: 'live' as const, secrets: [] }))), replace: payload => this.record('settings.replace', payload, Promise.resolve(ok({ ns: 'fake', schema: {}, value: {}, applies: 'live' as const, secrets: [] }))), + mutate: payload => this.record('settings.mutate', payload, Promise.resolve(ok({ ns: 'fake', schema: {}, value: {}, applies: 'live' as const, secrets: [] }))), } readonly credentials: IApiClient['credentials'] = { diff --git a/packages/client/connection/tests/node-half.spec.ts b/packages/client/connection/tests/node-half.spec.ts index db1c987575..6839d8b3ca 100644 --- a/packages/client/connection/tests/node-half.spec.ts +++ b/packages/client/connection/tests/node-half.spec.ts @@ -1,8 +1,10 @@ /** Node half: registers the /api prefix route bridging to the api gateway. */ import { EventEmitter } from 'node:events' +import { createServer, request as httpRequest } from 'node:http' import { Readable } from 'node:stream' import { Context } from 'cordis' import { describe, expect, it } from 'vitest' +import type { AddressInfo } from 'node:net' import type { IncomingMessage, ServerResponse } from 'node:http' import type { ApiProxy } from '@deepseek-ai/dsh-host-apiproxy/api' import type { HttpServerService, WebRoute } from '@deepseek-ai/dsh-host-webserver' @@ -99,14 +101,14 @@ describe('connection node half', () => { it('pins privileged methods to loopback even for a declared trusted authority', async () => { const { routes, dispose } = await mounted({ trustedHosts: ['harness.example'] }) - // The privileged set: native dialogs plus every settings/credential write. - // The same declared authority reaches ordinary reads (carrier-level 404 - // from the empty proxy proves the fence passed), but each privileged - // method stays loopback-only and short-circuits 403. + // The privileged set: native dialogs plus the whole settings/credential + // configuration plane, reads included. The same declared authority reaches + // ordinary reads (carrier-level 404 from the empty proxy proves the fence + // passed), but each privileged method stays loopback-only and 403s. for (const method of [ 'host.pickDirectory', 'host.openPath', - 'settings.update', 'settings.replace', - 'credentials.set', 'credentials.unset', + 'settings.describe', 'settings.update', 'settings.replace', + 'credentials.describe', 'credentials.set', 'credentials.unset', ]) { const denied = fakeResponse() await routes[0]!.handler( @@ -143,3 +145,69 @@ describe('connection node half', () => { await dispose() }) }) + +describe('connection node half over a real HTTP server', () => { + /** Serve the registered prefix route from a real server and return its port. */ + async function serve(routes: WebRoute[]): Promise<{ port: number; close: () => Promise }> { + const server = createServer((request, response) => { + void routes[0]!.handler(request, response) + }) + await new Promise(resolve => server.listen(0, '127.0.0.1', resolve)) + const address = server.address() as AddressInfo + return { + port: address.port, + close: () => new Promise((resolve, reject) => { + server.close((error) => { + if (error === undefined || error === null) resolve() + else reject(error) + }) + }), + } + } + + /** One real request; `host` spoofs the authority the way a LAN client's browser would send it. */ + function call(port: number, method: string, host: string): Promise { + return new Promise((resolve, reject) => { + const request = httpRequest( + { host: '127.0.0.1', port, path: `${API_PATH}/${method}`, method: 'GET', headers: { host } }, + (response) => { + response.resume() + response.on('end', () => { resolve(response.statusCode ?? 0) }) + }, + ) + request.on('error', reject) + request.end() + }) + } + + it('answers a declared LAN authority with 403 on every configuration method, over real HTTP', async () => { + // The fence's input is a real IncomingMessage parsed by Node from the + // wire, not a hand-assembled object: the Host header a LAN browser sends + // is exactly what decides loopback-only here, so the boundary is asserted + // against the parse the server actually performs. + const { routes, dispose } = await mounted({ trustedHosts: ['harness.example'] }) + const { port, close } = await serve(routes) + try { + // Reads are as privileged as writes: describe returns the exposed + // configuration, and credentials.describe probes arbitrary env-var names. + for (const method of [ + 'settings.describe', 'settings.update', 'settings.replace', + 'credentials.describe', 'credentials.set', 'credentials.unset', + 'host.pickDirectory', 'host.openPath', + ]) { + expect([method, await call(port, method, 'harness.example')]).toEqual([method, 403]) + } + // The model catalog stays reachable for the same authority: a LAN + // client's model picker needs it, and it carries no key or endpoint + // state (404 is the empty proxy's carrier answer — the fence passed). + for (const method of ['llm.providers', 'llm.models']) { + expect([method, await call(port, method, 'harness.example')]).toEqual([method, 404]) + } + // Loopback reaches everything, configuration included. + expect(await call(port, 'settings.describe', `127.0.0.1:${String(port)}`)).toBe(404) + } finally { + await close() + await dispose() + } + }) +}) diff --git a/packages/client/runtime/tests/fake-api.ts b/packages/client/runtime/tests/fake-api.ts index eaf215b216..480a6f034d 100644 --- a/packages/client/runtime/tests/fake-api.ts +++ b/packages/client/runtime/tests/fake-api.ts @@ -185,6 +185,7 @@ export class FakeApiClient implements IApiClient { describe: payload => this.record('settings.describe', payload, Promise.resolve(ok({ writable: true, namespaces: [] }))), update: payload => this.record('settings.update', payload, Promise.resolve(ok({ ns: 'fake', schema: {}, value: {}, applies: 'live' as const, secrets: [] }))), replace: payload => this.record('settings.replace', payload, Promise.resolve(ok({ ns: 'fake', schema: {}, value: {}, applies: 'live' as const, secrets: [] }))), + mutate: payload => this.record('settings.mutate', payload, Promise.resolve(ok({ ns: 'fake', schema: {}, value: {}, applies: 'live' as const, secrets: [] }))), } readonly credentials: IApiClient['credentials'] = { diff --git a/packages/client/ui-models/src/client/ModelsSection.tsx b/packages/client/ui-models/src/client/ModelsSection.tsx index d4f485b5cf..20ad4a4e01 100644 --- a/packages/client/ui-models/src/client/ModelsSection.tsx +++ b/packages/client/ui-models/src/client/ModelsSection.tsx @@ -11,7 +11,6 @@ import { useState } from 'react' import type { ReactNode } from 'react' import type { IApiClient, SettingsNamespaceView } from '@deepseek-ai/dsh-client-connection/client' -import { deletePath } from '@deepseek-ai/dsh-client-schema-form' import type { SnapshotSelectorHook } from '@deepseek-ai/dsh-client-web-react' import type { ModelsSettingsState, ModelsSettingsStore, ProviderRow } from './store.ts' import { ProviderEditor } from './ProviderEditor.tsx' @@ -45,24 +44,35 @@ interface EditorTarget { } /** - * Remove one user-added provider profile from its namespace's user section - * (wholesale replace — merge cannot express a removal) and reload on success. + * Remove one user-added provider profile by unsetting its path in the stored + * user section, then reload. The removal names the profile rather than + * rebuilding the section: this page only ever holds the redacted descriptor, + * so a rebuilt section would drop every literal secret stored elsewhere in + * the namespace along with the profile being removed. * @param api - settings wire face. * @param controller - the page store to refresh. * @param target - the provider's settings address. - * @param namespace - the owning namespace view. - * @returns settles when the write and any reload finished. + * @returns the failure message, or undefined once the write and reload landed. */ export async function removeProviderProfile( api: Pick, controller: ModelsSettingsStore, target: { settingsNs: string; settingsPath: readonly string[] }, - namespace: SettingsNamespaceView, -): Promise { - const user = structuredClone((namespace.user ?? {}) as Record) - const next = deletePath(user, [...target.settingsPath]) - const response = await api.settings.replace({ ns: target.settingsNs, section: next }) - if (response.result.ok) await controller.load() +): Promise { + let response + try { + response = await api.settings.mutate({ + ns: target.settingsNs, + ops: [{ op: 'unset', path: [...target.settingsPath] }], + }) + } catch (error) { + // The transport rejected rather than answering; the caller must be able + // to say so instead of the row silently staying put. + return error instanceof Error ? error.message : String(error) + } + if (!response.result.ok) return response.result.error.message + await controller.load() + return undefined } /** @@ -184,7 +194,11 @@ function Loaded({ injected }: { injected: ModelsSectionInjected }): ReactNode { type="button" className={styles['dangerButton']} disabled={!state.writable} - onClick={() => { void removeProviderProfile(api, controller, target, namespace) }} + onClick={() => { + void removeProviderProfile(api, controller, target).then((failure) => { + if (failure !== undefined) controller.fail(failure) + }) + }} > {t('remove')} diff --git a/packages/client/ui-models/src/client/ProviderEditor.tsx b/packages/client/ui-models/src/client/ProviderEditor.tsx index 8ea1fb22fe..7178a75de2 100644 --- a/packages/client/ui-models/src/client/ProviderEditor.tsx +++ b/packages/client/ui-models/src/client/ProviderEditor.tsx @@ -6,15 +6,15 @@ * has none, and the pi-ai profile records that derivation as `apiKeyEnv`); * the collapsed 自定义设置 area carries the per-family extras (`baseURL` for * both families, plus `reasoningEffort` for deepseek / `reasoning` for - * pi-ai). Everything else stays owned by `settings.yaml`. Profile edits land as a - * minimal `settings.update` merge patch; clearing a field back to inherited - * removes its key, so that apply replaces the user section (safe: the section - * stores references, never key values). + * pi-ai). Everything else stays owned by `settings.yaml`. Profile edits land as + * minimal `settings.mutate` path ops against the stored section — the card + * reads the redacted descriptor, so it names only the fields it can see and a + * stored literal secret is never collaterally removed. */ import { useEffect, useMemo, useState } from 'react' import type { ReactNode } from 'react' -import type { CredentialView, IApiClient, SettingsNamespaceView } from '@deepseek-ai/dsh-client-connection/client' +import type { CredentialView, IApiClient, SettingsNamespaceView, SettingsPathOpView } from '@deepseek-ai/dsh-client-connection/client' import { deletePath, getPath, nodeAtPath, rehydrateSchema, setPath, validateDraft, } from '@deepseek-ai/dsh-client-schema-form' @@ -70,21 +70,33 @@ function draftAt(namespace: SettingsNamespaceView, path: readonly string[]): Rec } /** - * Whether any key present in `before` is absent from `after` (a reset - * happened somewhere in the draft, so the apply must replace, not merge). - * @param before - the user-layer subtree the draft started from. - * @param after - the edited draft. - * @returns whether a removal exists at any depth. + * The minimal path ops carrying `after` over `before`, both as the card sees + * them (that is, redacted). Only keys the card observed are named: a stored + * `role('secret')` field appears in neither side, so it produces no op and + * survives the write — the whole reason edits are path-addressed rather than + * a rebuilt section. + * @param base - path of the edited subtree inside the user section. + * @param before - the subtree as loaded, or undefined when it is new. + * @param after - the subtree as edited. + * @returns ordered set/unset ops; empty when nothing changed. */ -export function removedAny(before: unknown, after: unknown): boolean { - if (typeof before !== 'object' || before === null) return false - /* v8 ignore next -- the editor edits containers in place; a container cannot become a primitive */ - if (typeof after !== 'object' || after === null) return true - for (const [key, value] of Object.entries(before)) { - if (!(key in (after as Record))) return true - if (removedAny(value, (after as Record)[key])) return true +export function pathOps( + base: readonly string[], + before: unknown, + after: Record, +): SettingsPathOpView[] { + const previous = typeof before === 'object' && before !== null && !Array.isArray(before) + ? before as Record + : {} + const ops: SettingsPathOpView[] = [] + for (const [key, value] of Object.entries(after)) { + if (JSON.stringify(previous[key]) === JSON.stringify(value)) continue + ops.push({ op: 'set', path: [...base, key], value }) } - return false + for (const key of Object.keys(previous)) { + if (!(key in after)) ops.push({ op: 'unset', path: [...base, key] }) + } + return ops } /** The editor layout the owning namespace selects. */ @@ -140,9 +152,14 @@ export function ProviderEditor(props: ProviderEditorProps): ReactNode { setDraft(current => next === undefined ? deletePath(current, [key]) : setPath(current, [key], next)) } - const apply = async (): Promise => { - setBusy(true) - setFailure(undefined) + /** + * The write for this card, or a failure message. Every edit travels as + * path ops against the STORED section: the draft comes from the redacted + * descriptor, so a wholesale replace rebuilt from it would delete the + * literal secrets the wire never returned. Ops name only the fields this + * card can see, so a stored secret is untouched by construction. + */ + const applyOnce = async (): Promise => { const ns = namespace.ns const original = getPath(namespace.user, settingsPath) // The pi-ai profile must name the reference the key stores under, so a @@ -151,45 +168,42 @@ export function ProviderEditor(props: ProviderEditorProps): ReactNode { && stringAt(fallback, 'apiKeyEnv') === undefined ? setPath(draft, ['apiKeyEnv'], keyRef) : draft - const settingsChanged = JSON.stringify(next) !== JSON.stringify(original ?? {}) - if (settingsChanged) { - const needsReplace = removedAny(original, next) - // Merge patches stay minimal (just this profile); a replace must carry - // the complete next user section because it lands wholesale. - const patch = settingsPath.length === 0 ? next : setPath({}, [...settingsPath], next) - /* v8 ignore next 3 -- a subtree apply implies the join served this namespace's user layer */ - const nextSection = settingsPath.length === 0 - ? next - : setPath(structuredClone((namespace.user ?? {}) as Record), [...settingsPath], next) - /* v8 ignore next -- apply is only reachable from the rendered card, which required a resolved node */ - if (node !== undefined) { - const sectionError = settingsPath.length === 0 ? validateDraft(node, next) : undefined - if (sectionError !== undefined) { - setBusy(false) - setFailure(sectionError) - return - } - } - const response = needsReplace - ? await api.settings.replace({ ns, section: nextSection }) - : await api.settings.update({ ns, patch }) - if (!response.result.ok) { - setBusy(false) - setFailure(response.result.error.message) - return - } + /* v8 ignore next -- apply is only reachable from the rendered card, which required a resolved node */ + if (node !== undefined && settingsPath.length === 0) { + const sectionError = validateDraft(node, next) + if (sectionError !== undefined) return sectionError + } + const ops = pathOps(settingsPath, original, next) + if (ops.length > 0) { + const response = await api.settings.mutate({ ns, ops }) + if (!response.result.ok) return response.result.error.message } if (keyDraft.length > 0) { const stored = await api.credentials.set({ ref: keyRef, value: keyDraft }) - if (!stored.result.ok) { - setBusy(false) - setFailure(stored.result.error.message) + if (!stored.result.ok) return stored.result.error.message + } + setKeyDraft('') + return undefined + } + + const apply = async (): Promise => { + setBusy(true) + setFailure(undefined) + try { + const failure = await applyOnce() + if (failure !== undefined) { + setFailure(failure) return } - setKeyDraft('') + props.onClose(true) + } catch (error) { + // A transport failure (disconnect, a request the host refuses) rejects + // rather than answering; without this the card would stay busy forever + // with no error shown. + setFailure(error instanceof Error ? error.message : String(error)) + } finally { + setBusy(false) } - setBusy(false) - props.onClose(true) } if (node === undefined) { diff --git a/packages/client/ui-models/src/client/store.ts b/packages/client/ui-models/src/client/store.ts index c84d857cad..89625228ea 100644 --- a/packages/client/ui-models/src/client/store.ts +++ b/packages/client/ui-models/src/client/store.ts @@ -75,6 +75,18 @@ export class ModelsSettingsStore { */ constructor(private readonly api: Pick) {} + /** + * Surface a failure from an operation the page ran outside {@link load} — + * a row removal — on the same banner a load failure uses. + * @param message - the failure text to show. + */ + fail(message: string): void { + this.store.update((s) => { + s.status = 'error' + s.error = message + }) + } + /** * Refresh the whole page snapshot: directory and namespaces in parallel, * then one batched credential describe over every referenced ref. A @@ -125,10 +137,12 @@ export class ModelsSettingsStore { const refs = [...new Set(rows.flatMap(row => row.apiKeyEnv === undefined ? [] : [row.apiKeyEnv]))] let credentials: Record = {} if (refs.length > 0) { - const response = await this.api.credentials.describe({ refs }) - // Credential state is an enrichment: rows render without it, so a - // missing credential provider degrades the badge, not the page. - if (response.result.ok) credentials = response.result.value.credentials + // Credential state is an enrichment: rows render without it, so neither + // a business rejection nor a transport failure (disconnect, a request + // the host refuses) may fail the load — an escaping rejection would + // leave the page stuck in `loading` with no error shown. + const response = await this.api.credentials.describe({ refs }).catch(() => undefined) + if (response?.result.ok === true) credentials = response.result.value.credentials } if (generation !== this.generation) return this.store.update((s) => { diff --git a/packages/client/ui-models/tests/components.spec.tsx b/packages/client/ui-models/tests/components.spec.tsx index 79bc4a7c88..24980bbccf 100644 --- a/packages/client/ui-models/tests/components.spec.tsx +++ b/packages/client/ui-models/tests/components.spec.tsx @@ -7,7 +7,7 @@ import { bindSnapshotSelector } from '@deepseek-ai/dsh-client-web-react' import type { RpcResponse, SettingsNamespaceView } from '@deepseek-ai/dsh-client-connection/client' import { ModelsSection, needsSetup, removeProviderProfile } from '../src/client/ModelsSection.tsx' import type { ModelsSectionInjected } from '../src/client/ModelsSection.tsx' -import { removedAny } from '../src/client/ProviderEditor.tsx' +import { pathOps } from '../src/client/ProviderEditor.tsx' import { deriveKeyRef, ModelsSettingsStore } from '../src/client/store.ts' import type { ProviderRow } from '../src/client/store.ts' import { en } from '../src/client/locales.ts' @@ -79,10 +79,12 @@ function fail(message: string, code = 'settings-rejected'): RpcResponse { function scriptedFace(overrides: { update?: ReturnType replace?: ReturnType + mutate?: ReturnType set?: ReturnType } = {}) { const update = overrides.update ?? vi.fn(() => Promise.resolve(ok(wireNamespaces()[2]))) const replace = overrides.replace ?? vi.fn(() => Promise.resolve(ok(wireNamespaces()[2]))) + const mutate = overrides.mutate ?? vi.fn(() => Promise.resolve(ok(wireNamespaces()[2]))) const set = overrides.set ?? vi.fn(() => Promise.resolve(ok({}))) const face = { llm: { @@ -102,6 +104,7 @@ function scriptedFace(overrides: { describe: vi.fn(() => Promise.resolve(ok({ writable: true, namespaces: wireNamespaces() }))), update, replace, + mutate, }, credentials: { describe: vi.fn((payload: { refs: string[] }) => Promise.resolve(ok({ @@ -115,13 +118,13 @@ function scriptedFace(overrides: { unset: vi.fn(() => Promise.resolve(ok({}))), }, } - return { face, update, replace, set } + return { face, update, replace, mutate, set } } type WireFace = ConstructorParameters[0] async function mountSection(overrides: Parameters[0] = {}) { - const { face, update, replace, set } = scriptedFace(overrides) + const { face, update, replace, mutate, set } = scriptedFace(overrides) const controller = new ModelsSettingsStore(face as unknown as WireFace) await controller.load() const injected: ModelsSectionInjected = { @@ -131,7 +134,7 @@ async function mountSection(overrides: Parameters[0] = {}) t, } const view = render() - return { view, face, update, replace, set, controller } + return { view, face, update, replace, mutate, set, controller } } describe('ModelsSection', () => { @@ -184,10 +187,15 @@ describe('ModelsSection', () => { expect(deriveKeyRef('minimax-cn')).toBe('MINIMAX_CN_API_KEY') }) - it('detects removals at any draft depth', () => { - expect(removedAny({ a: { b: 1, c: 2 } }, { a: { b: 1 } })).toBe(true) - expect(removedAny({ a: { b: 1 } }, { a: { b: 2 }, d: 3 })).toBe(false) - expect(removedAny(undefined, {})).toBe(false) + it('names only the fields the card can see, so an unseen secret survives', () => { + // `before` is the REDACTED subtree: a stored literal apiKey is in neither + // side, so no op mentions it and the seam leaves it alone. + expect(pathOps(['providers', 'openai'], { baseURL: 'https://old', reasoning: 'high' }, { reasoning: 'high' })) + .toEqual([{ op: 'unset', path: ['providers', 'openai', 'baseURL'] }]) + expect(pathOps([], { b: 1 }, { b: 2, d: 3 })) + .toEqual([{ op: 'set', path: ['b'], value: 2 }, { op: 'set', path: ['d'], value: 3 }]) + expect(pathOps([], undefined, {})).toEqual([]) + expect(pathOps([], { a: 1 }, { a: 1 })).toEqual([]) }) it('stores a typed key write-only from the setup card without touching settings', async () => { @@ -200,9 +208,9 @@ describe('ModelsSection', () => { await waitFor(() => { expect(face.settings.describe.mock.calls.length).toBeGreaterThan(1) }) }) - it('applies customized deepseek fields as a merge patch', async () => { - const { update } = await mountSection({ - update: vi.fn(() => Promise.resolve(ok(wireNamespaces()[0]))), + it('applies customized deepseek fields as path ops', async () => { + const { mutate } = await mountSection({ + mutate: vi.fn(() => Promise.resolve(ok(wireNamespaces()[0]))), }) fireEvent.click(screen.getByText(en.customized)) const baseURL = screen.getByLabelText(en.baseUrl) @@ -211,23 +219,31 @@ describe('ModelsSection', () => { expect(baseURL.placeholder).toBe('https://api.deepseek.com') fireEvent.change(baseURL, { target: { value: 'https://next2' } }) fireEvent.click(screen.getByText(en.apply)) - await waitFor(() => { expect(update).toHaveBeenCalledTimes(1) }) - expect(update.mock.calls[0]?.[0]).toEqual({ + await waitFor(() => { expect(mutate).toHaveBeenCalledTimes(1) }) + // Only the field that actually changed: reasoningEffort was already + // 'high' in the loaded profile, so it produces no op. + expect(mutate.mock.calls[0]?.[0]).toEqual({ ns: 'llm-deepseek', - patch: { reasoningEffort: 'high', baseURL: 'https://next2' }, + ops: [{ op: 'set', path: ['baseURL'], value: 'https://next2' }], }) }) - it('clears an inherited override through replace so the removal lands', async () => { - const { replace, update } = await mountSection() + it('clears an inherited override with an unset op, never a whole-section replace', async () => { + // The data-loss shape: the old path rebuilt the section from the REDACTED + // user layer and replaced it wholesale, deleting any stored literal key. + const { replace, update, mutate } = await mountSection() fireEvent.click(screen.getByText(en.customized)) const effort = screen.getByLabelText(en.effort) expect(effort.value).toBe('high') fireEvent.change(effort, { target: { value: '' } }) fireEvent.click(screen.getByText(en.apply)) - await waitFor(() => { expect(replace).toHaveBeenCalledTimes(1) }) + await waitFor(() => { expect(mutate).toHaveBeenCalledTimes(1) }) + expect(replace).not.toHaveBeenCalled() expect(update).not.toHaveBeenCalled() - expect(replace.mock.calls[0]?.[0]).toEqual({ ns: 'llm-deepseek', section: {} }) + expect(mutate.mock.calls[0]?.[0]).toEqual({ + ns: 'llm-deepseek', + ops: [{ op: 'unset', path: ['reasoningEffort'] }], + }) }) it('pins the deepseek placeholder and clears typed input back to inherited', async () => { @@ -269,7 +285,7 @@ describe('ModelsSection', () => { }) it('edits a pi-ai profile with the curated fields only', async () => { - const { update } = await mountSection() + const { mutate } = await mountSection() fireEvent.click(screen.getAllByText(en.edit)[0] as HTMLElement) // The configured credential shows as the stored placeholder. const keys = await screen.findAllByLabelText(en.keyInput) @@ -284,19 +300,19 @@ describe('ModelsSection', () => { const effort = screen.getAllByLabelText(en.effort) fireEvent.change(effort[effort.length - 1] as HTMLSelectElement, { target: { value: 'xhigh' } }) fireEvent.click(screen.getAllByText(en.apply)[1] as HTMLElement) - await waitFor(() => { expect(update).toHaveBeenCalledTimes(1) }) - expect(update.mock.calls[0]?.[0]).toEqual({ + await waitFor(() => { expect(mutate).toHaveBeenCalledTimes(1) }) + // Only the edited field travels: apiKeyEnv, baseURL and headers were + // already stored with these values, so no op restates them — and the + // profile's stored literal apiKey, absent from the redacted view the card + // read, is named by nothing at all. + expect(mutate.mock.calls[0]?.[0]).toEqual({ ns: 'llm-pi-ai', - patch: { - providers: { - openai: { apiKeyEnv: 'OPENAI_API_KEY', baseURL: 'https://proxy', headers: { 'X-Team': 'a' }, reasoning: 'xhigh' }, - }, - }, + ops: [{ op: 'set', path: ['providers', 'openai', 'reasoning'], value: 'xhigh' }], }) }) it('adds a dormant provider with a derived reference and stores its key', async () => { - const { update, set } = await mountSection() + const { mutate, set } = await mountSection() fireEvent.click(screen.getByText(`+ ${en.add}`)) const pick = await screen.findByLabelText(en.provider) expect([...pick.options].map(option => option.value)).toEqual(['anthropic', 'broken', 'plain']) @@ -310,10 +326,10 @@ describe('ModelsSection', () => { const addKey = keys[keys.length - 1] as HTMLInputElement fireEvent.change(addKey, { target: { value: 'sk-ant' } }) fireEvent.click(screen.getAllByText(en.apply)[1] as HTMLElement) - await waitFor(() => { expect(update).toHaveBeenCalledTimes(1) }) - expect(update.mock.calls[0]?.[0]).toEqual({ + await waitFor(() => { expect(mutate).toHaveBeenCalledTimes(1) }) + expect(mutate.mock.calls[0]?.[0]).toEqual({ ns: 'llm-pi-ai', - patch: { providers: { anthropic: { apiKeyEnv: 'ANTHROPIC_API_KEY' } } }, + ops: [{ op: 'set', path: ['providers', 'anthropic', 'apiKeyEnv'], value: 'ANTHROPIC_API_KEY' }], }) await waitFor(() => { expect(set).toHaveBeenCalledWith({ ref: 'ANTHROPIC_API_KEY', value: 'sk-ant' }) }) }) @@ -336,7 +352,7 @@ describe('ModelsSection', () => { it('surfaces a rejected settings write and never stores the key after it', async () => { const { set } = await mountSection({ - update: vi.fn(() => Promise.resolve(fail('llm-pi-ai: unknown pi-ai provider "bogus"'))), + mutate: vi.fn(() => Promise.resolve(fail('llm-pi-ai: unknown pi-ai provider "bogus"'))), }) fireEvent.click(screen.getByText(`+ ${en.add}`)) await screen.findByLabelText(en.provider) @@ -383,11 +399,15 @@ describe('ModelsSection', () => { await waitFor(() => { expect(set).toHaveBeenCalledTimes(1) }) }) - it('removes a user-added provider through replace', async () => { - const { replace } = await mountSection() + it('removes a user-added provider by unsetting its path', async () => { + const { replace, mutate } = await mountSection() fireEvent.click(screen.getAllByText(en.remove)[0] as HTMLElement) - await waitFor(() => { expect(replace).toHaveBeenCalledTimes(1) }) - expect(replace.mock.calls[0]?.[0]).toEqual({ ns: 'llm-pi-ai', section: { providers: { zombie: {} } } }) + await waitFor(() => { expect(mutate).toHaveBeenCalledTimes(1) }) + expect(replace).not.toHaveBeenCalled() + expect(mutate.mock.calls[0]?.[0]).toEqual({ + ns: 'llm-pi-ai', + ops: [{ op: 'unset', path: ['providers', 'openai'] }], + }) }) it('renders the load failure with a retry control', async () => { @@ -461,30 +481,45 @@ describe('ModelsSection', () => { await screen.findByText('DeepSeek') }) - it('removes against a namespace with no user layer as an empty-section replace', async () => { - const { face, replace, controller } = await mountSection() - const namespace = controller.store.getSnapshot().namespaces.get('llm-plain') + it('removes by unsetting the profile path, never by rebuilding the section', async () => { + // The section rebuild is what dropped stored literal secrets: this page + // only ever holds the redacted descriptor, so the removal names the path. + const { face, mutate, replace, controller } = await mountSection() await removeProviderProfile( face as unknown as Parameters[0], controller, { settingsNs: 'llm-plain', settingsPath: ['ghost-profile'] }, - namespace as NonNullable, ) - expect(replace.mock.calls[0]?.[0]).toEqual({ ns: 'llm-plain', section: {} }) + expect(mutate.mock.calls[0]?.[0]).toEqual({ + ns: 'llm-plain', + ops: [{ op: 'unset', path: ['ghost-profile'] }], + }) + expect(replace).not.toHaveBeenCalled() }) - it('keeps the snapshot untouched when a removal write is refused', async () => { + it('keeps the snapshot untouched and reports the message when a removal write is refused', async () => { const { face, controller } = await mountSection({ - replace: vi.fn(() => Promise.resolve(fail('read-only'))), + mutate: vi.fn(() => Promise.resolve(fail('read-only'))), }) - const namespace = controller.store.getSnapshot().namespaces.get('llm-pi-ai') const before = controller.store.getSnapshot().rows - await removeProviderProfile( + const failure = await removeProviderProfile( face as unknown as Parameters[0], controller, { settingsNs: 'llm-pi-ai', settingsPath: ['providers', 'openai'] }, - namespace as NonNullable, ) + expect(failure).toBe('read-only') expect(controller.store.getSnapshot().rows).toBe(before) }) + + it('reports a transport rejection instead of failing the removal silently', async () => { + const { face, controller } = await mountSection({ + mutate: vi.fn(() => Promise.reject(new Error('connection lost'))), + }) + const failure = await removeProviderProfile( + face as unknown as Parameters[0], + controller, + { settingsNs: 'llm-pi-ai', settingsPath: ['providers', 'openai'] }, + ) + expect(failure).toBe('connection lost') + }) }) diff --git a/packages/host/apiproxy/src/api-proxy.ts b/packages/host/apiproxy/src/api-proxy.ts index 3b919f79d2..c707f9fa71 100644 --- a/packages/host/apiproxy/src/api-proxy.ts +++ b/packages/host/apiproxy/src/api-proxy.ts @@ -42,7 +42,7 @@ import type {} from '@deepseek-ai/dsh-skill' // service reads stay optional (`ctx.get`) so a composition without either // provider still serves every other domain. import { settingsNamespace } from '@deepseek-ai/dsh-settings' -import type { SettingsDescriptor, SettingsNamespace } from '@deepseek-ai/dsh-settings' +import type { SettingsDescriptor, SettingsNamespace, SettingsPathOp } from '@deepseek-ai/dsh-settings' import { credentialRef } from '@deepseek-ai/dsh-credentials' // Value edge: the rename impl narrows the title service's validation failure; the import also resolves `ctx.get('sessionTitle')`. import { SessionTitleInvalidError } from '@deepseek-ai/dsh-session-title' @@ -1010,16 +1010,39 @@ export function createApiProxy(ctx: Context, defaults: ApiProxyDefaults): ApiPro } } + /** + * The settings namespaces this proxy serves: exactly those a registered + * configurable provider addresses. The settings seam itself is general — + * any plugin may register a namespace for its own configuration — but the + * Web configuration plane is scoped to model providers, and that boundary + * has to be enforced here rather than assumed from the current plugin set. + * Without it, every future `settings.register()` would silently become + * remotely readable and writable configuration. + */ + function exposedNamespaces(): Set { + return new Set(ctx.llm.listConfigurableProviders().map(entry => entry.settingsNs)) + } + + /** Refuse a namespace outside the model-provider boundary, naming why. */ + function notExposed(request: RpcRequest, ns: string): RpcResponse { + return err(request, { + code: 'settings-not-exposed', + message: `settings namespace "${ns}" is not exposed to configuration clients; only a namespace a registered model provider addresses is`, + details: { ns }, + }) + } + /** * Run one settings write (merge or wholesale replace) and acknowledge with - * the namespace's new redacted view. Every seam refusal — unknown or - * invalid namespace, read-only provider, schema validation, storage — - * becomes one `settings-rejected` carrying the seam's own message. + * the namespace's new redacted view. A namespace outside the model-provider + * boundary is refused before the seam is touched; every seam refusal — + * unknown or invalid namespace, read-only provider, schema validation, + * storage — becomes one `settings-rejected` carrying the seam's own message. */ async function settingsWrite( request: RpcRequest, ns: string, - mode: 'update' | 'replace', + mode: 'update' | 'replace' | 'mutate', section: object, ): Promise> { const settings = ctx.get('settings') @@ -1033,11 +1056,15 @@ export function createApiProxy(ctx: Context, defaults: ApiProxyDefaults): ApiPro try { branded = settingsNamespace(ns) } catch (error: unknown) { + // A malformed name is a client bug, reported as such; it could never be + // in the exposed set either, so naming the real fault costs no ground. return rejected(error) } + if (!exposedNamespaces().has(ns)) return notExposed(request, ns) try { if (mode === 'update') await settings.update(branded, section) - else await settings.replace(branded, section) + else if (mode === 'replace') await settings.replace(branded, section) + else await settings.mutate(branded, section as SettingsPathOp[]) } catch (error: unknown) { return rejected(error) } @@ -1632,13 +1659,17 @@ export function createApiProxy(ctx: Context, defaults: ApiProxyDefaults): ApiPro describe(request) { const settings = ctx.get('settings') if (settings === undefined) return Promise.resolve(err(request, settingsAbsent())) + const exposed = exposedNamespaces() return Promise.resolve(ok(request, { writable: settings.writable, - namespaces: settings.describe({ redactSecrets: true }).map(namespaceView), + namespaces: settings.describe({ redactSecrets: true }) + .filter(descriptor => exposed.has(String(descriptor.ns))) + .map(namespaceView), })) }, update: request => settingsWrite(request, request.payload.ns, 'update', request.payload.patch), replace: request => settingsWrite(request, request.payload.ns, 'replace', request.payload.section), + mutate: request => settingsWrite(request, request.payload.ns, 'mutate', request.payload.ops), }, credentials: { diff --git a/packages/host/apiproxy/src/api/index.ts b/packages/host/apiproxy/src/api/index.ts index 9c5269840e..4eda35007d 100644 --- a/packages/host/apiproxy/src/api/index.ts +++ b/packages/host/apiproxy/src/api/index.ts @@ -43,7 +43,7 @@ export type { CommandsApi, CommandDescriptor } from './commands.ts' export type { SkillsApi, SkillEntry } from './skills.ts' export type { EventsApi, MuxFrame, HostFrame, QueuedInboxItem, ToolCallView, ToolEventView, ToolResultView } from './events.ts' export type { GoalsApi, GoalId, GoalRef } from './goals.ts' -export type { SettingsApi, SettingsNamespaceView, SettingsSecretView } from './settings.ts' +export type { SettingsApi, SettingsNamespaceView, SettingsPathOpView, SettingsSecretView } from './settings.ts' export type { CredentialsApi, CredentialView } from './credentials.ts' export type { ConfigurableProviderView, LlmApi } from './llm.ts' export type { ApprovalResponsePayload } from './approvals.ts' diff --git a/packages/host/apiproxy/src/api/rpc-map.ts b/packages/host/apiproxy/src/api/rpc-map.ts index 409f463dbb..911e3008eb 100644 --- a/packages/host/apiproxy/src/api/rpc-map.ts +++ b/packages/host/apiproxy/src/api/rpc-map.ts @@ -52,6 +52,7 @@ export interface RpcMethodMap { 'settings.describe': SettingsApi['describe'] 'settings.update': SettingsApi['update'] 'settings.replace': SettingsApi['replace'] + 'settings.mutate': SettingsApi['mutate'] 'credentials.describe': CredentialsApi['describe'] 'credentials.set': CredentialsApi['set'] 'credentials.unset': CredentialsApi['unset'] diff --git a/packages/host/apiproxy/src/api/rpc.schema.ts b/packages/host/apiproxy/src/api/rpc.schema.ts index 68e4b3fc66..85ecc1618b 100644 --- a/packages/host/apiproxy/src/api/rpc.schema.ts +++ b/packages/host/apiproxy/src/api/rpc.schema.ts @@ -51,6 +51,7 @@ export const rpcErrorSchema: z.ZodType = z.discriminatedUnion('code', z.object({ code: z.literal('command-error'), message: z.string(), details: z.object({}) }), z.object({ code: z.literal('unknown-command'), message: z.string(), details: z.object({}) }), z.object({ code: z.literal('settings-rejected'), message: z.string(), details: z.object({ ns: z.string() }) }), + z.object({ code: z.literal('settings-not-exposed'), message: z.string(), details: z.object({ ns: z.string() }) }), z.object({ code: z.literal('credential-rejected'), message: z.string(), details: z.object({ ref: z.string() }) }), z.object({ code: z.literal('title-invalid'), message: z.string(), details: z.object({ sessionId: z.string() }) }), z.object({ code: z.literal('internal'), message: z.string(), details: z.object({}) }), diff --git a/packages/host/apiproxy/src/api/rpc.ts b/packages/host/apiproxy/src/api/rpc.ts index 572ca66efd..d215f7f6fa 100644 --- a/packages/host/apiproxy/src/api/rpc.ts +++ b/packages/host/apiproxy/src/api/rpc.ts @@ -55,6 +55,12 @@ export interface RpcErrorDetailsMap { * read-only provider, or storage failure); the message is the seam's text. */ 'settings-rejected': { ns: string } + /** + * A settings namespace exists in the seam but is outside the configuration + * plane's model-provider boundary, so this proxy neither reads nor writes + * it; the message names the namespace. + */ + 'settings-not-exposed': { ns: string } /** A credential write was refused (read-only shadowing layer or storage failure); the message is the seam's own text. */ 'credential-rejected': { ref: string } 'title-invalid': { sessionId: SessionId } diff --git a/packages/host/apiproxy/src/api/settings.schema.ts b/packages/host/apiproxy/src/api/settings.schema.ts index 105573b109..2def419e1b 100644 --- a/packages/host/apiproxy/src/api/settings.schema.ts +++ b/packages/host/apiproxy/src/api/settings.schema.ts @@ -6,7 +6,7 @@ import { z } from 'zod' import type { RequestPayload, ResponseValue } from './rpc-map.ts' import type { Wire } from './rpc.schema.ts' -import type { SettingsNamespaceView, SettingsSecretView } from './settings.ts' +import type { SettingsNamespaceView, SettingsPathOpView, SettingsSecretView } from './settings.ts' /** One redacted secret slot. */ export const settingsSecretViewSchema = z.object({ @@ -49,5 +49,20 @@ export const settingsReplaceRequestSchema = z.object({ section: z.record(z.string(), z.unknown()), }) satisfies z.ZodType>> +/** One path-addressed edit of settings.mutate. */ +export const settingsPathOpSchema = z.discriminatedUnion('op', [ + z.object({ op: z.literal('set'), path: z.array(z.string()), value: z.unknown() }), + z.object({ op: z.literal('unset'), path: z.array(z.string()) }), +]) as unknown as z.ZodType> + +/** settings.mutate request payload. */ +export const settingsMutateRequestSchema = z.object({ + ns: z.string().min(1), + ops: z.array(settingsPathOpSchema), +}) satisfies z.ZodType>> + +/** settings.mutate response value: the namespace's new redacted view. */ +export const settingsMutateValueSchema = settingsNamespaceViewSchema satisfies z.ZodType>> + /** settings.replace response value. */ export const settingsReplaceValueSchema = settingsNamespaceViewSchema satisfies z.ZodType>> diff --git a/packages/host/apiproxy/src/api/settings.ts b/packages/host/apiproxy/src/api/settings.ts index 27e4d11156..7bad8c566e 100644 --- a/packages/host/apiproxy/src/api/settings.ts +++ b/packages/host/apiproxy/src/api/settings.ts @@ -34,6 +34,15 @@ export interface SettingsNamespaceView { secrets: SettingsSecretView[] } +/** + * One path-addressed edit carried by `settings.mutate`. `set` writes the + * value at the path (creating intermediate objects); `unset` removes it. The + * empty path addresses the section root. + */ +export type SettingsPathOpView = + | { op: 'set'; path: string[]; value: unknown } + | { op: 'unset'; path: string[] } + /** Settings-domain unary methods (the map keys settings.* of RpcMethodMap). */ export interface SettingsApi { /** @@ -60,4 +69,14 @@ export interface SettingsApi { * keep) or accept the reset. */ replace(request: RpcRequest<{ ns: string; section: object }>): Promise> + + /** + * Apply path-addressed edits to one namespace's user section, resolved + * against the section as stored — NOT against whatever the caller last + * read. This is the removal path for any client holding the redacted + * descriptor: it names the field it means, so a secret the wire never + * returned cannot be deleted as a side effect. `replace` remains the + * deliberate wholesale reset. + */ + mutate(request: RpcRequest<{ ns: string; ops: SettingsPathOpView[] }>): Promise> } diff --git a/packages/host/apiproxy/src/fetch/client.ts b/packages/host/apiproxy/src/fetch/client.ts index d833d462e5..6fc52ab3fe 100644 --- a/packages/host/apiproxy/src/fetch/client.ts +++ b/packages/host/apiproxy/src/fetch/client.ts @@ -46,7 +46,7 @@ import { goalClearValueSchema, } from '../api/goals.schema.ts' import { - settingsDescribeValueSchema, settingsReplaceValueSchema, settingsUpdateValueSchema, + settingsDescribeValueSchema, settingsMutateValueSchema, settingsReplaceValueSchema, settingsUpdateValueSchema, } from '../api/settings.schema.ts' import { credentialsDescribeValueSchema, credentialsSetValueSchema, credentialsUnsetValueSchema, @@ -117,6 +117,7 @@ export interface IApiClient { describe(payload: RequestPayload<'settings.describe'>, signal?: AbortSignal): Promise>> update(payload: RequestPayload<'settings.update'>, signal?: AbortSignal): Promise>> replace(payload: RequestPayload<'settings.replace'>, signal?: AbortSignal): Promise>> + mutate(payload: RequestPayload<'settings.mutate'>, signal?: AbortSignal): Promise>> } credentials: { describe(payload: RequestPayload<'credentials.describe'>, signal?: AbortSignal): Promise>> @@ -167,6 +168,7 @@ const UNARY_VALUE_SCHEMAS: { [K in keyof RpcMethodMap]: z.ZodType this.callUnary('settings.describe', payload, signal), update: (payload, signal) => this.callUnary('settings.update', payload, signal), replace: (payload, signal) => this.callUnary('settings.replace', payload, signal), + mutate: (payload, signal) => this.callUnary('settings.mutate', payload, signal), } readonly credentials: IApiClient['credentials'] = { diff --git a/packages/host/apiproxy/src/fetch/handler.ts b/packages/host/apiproxy/src/fetch/handler.ts index ef22a076d8..970cc16cfb 100644 --- a/packages/host/apiproxy/src/fetch/handler.ts +++ b/packages/host/apiproxy/src/fetch/handler.ts @@ -48,7 +48,7 @@ import { goalClearRequestSchema, } from '../api/goals.schema.ts' import { - settingsDescribeRequestSchema, settingsReplaceRequestSchema, settingsUpdateRequestSchema, + settingsDescribeRequestSchema, settingsMutateRequestSchema, settingsReplaceRequestSchema, settingsUpdateRequestSchema, } from '../api/settings.schema.ts' import { credentialsDescribeRequestSchema, credentialsSetRequestSchema, credentialsUnsetRequestSchema, @@ -103,6 +103,7 @@ const UNARY_ROUTES: UnaryRoutes = { 'settings.describe': { schema: settingsDescribeRequestSchema, invoke: (api, r) => api.settings.describe(r) }, 'settings.update': { schema: settingsUpdateRequestSchema, invoke: (api, r) => api.settings.update(r) }, 'settings.replace': { schema: settingsReplaceRequestSchema, invoke: (api, r) => api.settings.replace(r) }, + 'settings.mutate': { schema: settingsMutateRequestSchema, invoke: (api, r) => api.settings.mutate(r) }, 'credentials.describe': { schema: credentialsDescribeRequestSchema, invoke: (api, r) => api.credentials.describe(r) }, 'credentials.set': { schema: credentialsSetRequestSchema, invoke: (api, r) => api.credentials.set(r) }, 'credentials.unset': { schema: credentialsUnsetRequestSchema, invoke: (api, r) => api.credentials.unset(r) }, diff --git a/packages/host/apiproxy/tests/api-proxy-config.spec.ts b/packages/host/apiproxy/tests/api-proxy-config.spec.ts index 2562de5785..badfad76b3 100644 --- a/packages/host/apiproxy/tests/api-proxy-config.spec.ts +++ b/packages/host/apiproxy/tests/api-proxy-config.spec.ts @@ -148,6 +148,8 @@ const AdapterConfig = z.object({ async function harness(options?: { settings?: false | { doc?: Record; readOnly?: boolean } credentials?: false | { shadowed?: string[] } + /** Skip the directory registration to exercise a namespace the proxy does not expose. */ + configurableProviders?: false }): Promise { const ctx = new Context() await ctx.plugin(SessionStore) @@ -158,6 +160,13 @@ async function harness(options?: { await ctx.plugin(LlmService) if (options?.settings !== false) await ctx.plugin(MemorySettings, options?.settings) if (options?.credentials !== false) await ctx.plugin(MemoryCredentials, options?.credentials) + // The proxy serves only namespaces a configurable provider addresses, which + // is what the real LLM plugins declare at load; the tests mirror that. + if (options?.configurableProviders !== false) { + ctx.llm.registerConfigurableProviders([ + { provider: 'deepseek-official', displayName: 'DeepSeek', settingsNs: 'llm-deepseek', settingsPath: [] }, + ]) + } // Host-stream opener reads the committed-workspace baseline; the stub // suffices — the real workspace composition is api-proxy-workspace.spec's. ctx.provide('workspace', { list: () => [] } as never) @@ -213,6 +222,41 @@ describe('settings domain', () => { expect(JSON.stringify(value)).not.toContain('user-secret') }) + it('serves only namespaces a registered model provider addresses', async () => { + // The settings seam is general: any plugin may register a namespace for + // its own configuration. The Web configuration plane is not — it is the + // model-provider surface, and a namespace nothing in the provider + // directory addresses must be invisible and unwritable here, so a future + // plugin cannot become remotely configurable just by registering. + const ctx = await harness() + ctx.settings.register(NS, AdapterConfig) + ctx.settings.register(settingsNamespace('some-other-plugin'), z.object({ secretPath: z.string() })) + const api = createApiProxy(ctx, DEFAULTS) + + const value = expectOk(await api.settings.describe(request({}))) + expect(value.namespaces.map(view => view.ns)).toEqual(['llm-deepseek']) + + for (const response of [ + await api.settings.update(request({ ns: 'some-other-plugin', patch: { secretPath: '/etc/shadow' } })), + await api.settings.replace(request({ ns: 'some-other-plugin', section: {} })), + ]) { + const error = expectErr(response) + expect(error.code).toBe('settings-not-exposed') + expect(error.details).toEqual({ ns: 'some-other-plugin' }) + } + // The write never reached the seam. + expect(ctx.settings.describe().find(d => String(d.ns) === 'some-other-plugin')?.value).toEqual({}) + }) + + it('refuses even a model-provider namespace once its directory entry is gone', async () => { + const ctx = await harness({ configurableProviders: false }) + ctx.settings.register(NS, AdapterConfig) + const api = createApiProxy(ctx, DEFAULTS) + expect(expectOk(await api.settings.describe(request({}))).namespaces).toEqual([]) + expect(expectErr(await api.settings.update(request({ ns: 'llm-deepseek', patch: { baseURL: 'https://x' } }))).code) + .toBe('settings-not-exposed') + }) + it('updates the user layer, answers with the new redacted view, and broadcasts the frame', async () => { const ctx = await harness() ctx.settings.register(NS, AdapterConfig, { base: { baseURL: 'https://base' } }) @@ -238,7 +282,6 @@ describe('settings domain', () => { it.each([ ['an invalid namespace name', 'Not A Namespace', {}], - ['an unregistered namespace', 'unknown-ns', {}], ['a schema-invalid patch', 'llm-deepseek', { baseURL: 42 }], ])('rejects %s as settings-rejected', async (_case, ns, patch) => { const ctx = await harness() @@ -249,6 +292,21 @@ describe('settings domain', () => { expect(error.details).toEqual({ ns }) }) + it('answers an unregistered namespace exactly like an unexposed one', async () => { + // Deliberately indistinguishable: separating "does not exist" from + // "exists but is not yours to configure" would let a caller enumerate the + // registered namespaces one probe at a time. + const ctx = await harness() + ctx.settings.register(NS, AdapterConfig) + ctx.settings.register(settingsNamespace('some-other-plugin'), z.object({ secretPath: z.string() })) + const api = createApiProxy(ctx, DEFAULTS) + const unknown = expectErr(await api.settings.update(request({ ns: 'unknown-ns', patch: {} }))) + const unexposed = expectErr(await api.settings.update(request({ ns: 'some-other-plugin', patch: {} }))) + expect(unknown.code).toBe('settings-not-exposed') + expect(unexposed.code).toBe(unknown.code) + expect(unexposed.message.replace('some-other-plugin', 'unknown-ns')).toBe(unknown.message) + }) + it('maps a read-only provider refusal onto the same rejection', async () => { const ctx = await harness({ settings: { readOnly: true } }) ctx.settings.register(NS, AdapterConfig) @@ -303,7 +361,7 @@ describe('credentials domain', () => { describe('llm domain', () => { it('merges the configurable directory with live routes and appends undeclared ones', async () => { - const ctx = await harness() + const ctx = await harness({ configurableProviders: false }) ctx.llm.registerConfigurableProviders([ { provider: 'deepseek-official', displayName: 'DeepSeek', settingsNs: 'llm-deepseek', settingsPath: [] }, { provider: 'openai', displayName: 'openai', settingsNs: 'llm-pi-ai', settingsPath: ['providers', 'openai'] }, diff --git a/packages/host/apiproxy/tests/client-handler.spec.ts b/packages/host/apiproxy/tests/client-handler.spec.ts index e481ff8623..dec295ae13 100644 --- a/packages/host/apiproxy/tests/client-handler.spec.ts +++ b/packages/host/apiproxy/tests/client-handler.spec.ts @@ -89,6 +89,7 @@ function scriptedApi(overrides: { describe: r => ok(r, { writable: true, namespaces: [] }), update: err, replace: err, + mutate: err, ...overrides.settings, }, credentials: { @@ -615,6 +616,7 @@ describe('config unary surface', () => { describe: record('settings.describe', r => ok(r, { writable: true, namespaces: [view] })), update: record('settings.update', r => ok(r, view)), replace: record('settings.replace', r => ok(r, view)), + mutate: record('settings.mutate', r => ok(r, view)), }, credentials: { describe: record('credentials.describe', r => ok(r, { credentials: { OPENAI_API_KEY: { configured: true, source: 'file', writable: true } } })), diff --git a/packages/host/apiproxy/tests/fetch-carrier.spec.ts b/packages/host/apiproxy/tests/fetch-carrier.spec.ts index 24474ca4bc..351ed9b20a 100644 --- a/packages/host/apiproxy/tests/fetch-carrier.spec.ts +++ b/packages/host/apiproxy/tests/fetch-carrier.spec.ts @@ -177,6 +177,9 @@ function fakeApi(overrides: Partial<{ muxFrames: MuxFrame[]; hostFrames: HostFra async replace(request) { return { rpcId: request.rpcId, result: { ok: false, error: { code: 'settings-rejected', message: 'stub', details: { ns: request.payload.ns } } } } }, + async mutate(request) { + return { rpcId: request.rpcId, result: { ok: false, error: { code: 'settings-rejected', message: 'stub', details: { ns: request.payload.ns } } } } + }, }, credentials: { async describe(request) { diff --git a/packages/llm/llm-deepseek/src/index.ts b/packages/llm/llm-deepseek/src/index.ts index 56310437c8..16194955d1 100644 --- a/packages/llm/llm-deepseek/src/index.ts +++ b/packages/llm/llm-deepseek/src/index.ts @@ -220,16 +220,17 @@ export function apply(ctx: Context, config: Config): void { ]) // Route effects bind to this apply fiber via the stable `ctx` reference, // even when a swap runs inside the scoped settings callback below. - let disposeRoute = ctx.llm.registerAdapter([PROVIDER], adapter) + const registration = ctx.llm.registerAdapter([PROVIDER], adapter) let registeredPolicy = options().retryPolicy const ensureRegistrationFacts = (): void => { const policy = options().retryPolicy if (deepEqualJson(policy, registeredPolicy)) return // The registry captures the retry policy at registration, so it is the one - // fact per-request resolution cannot refresh: swap the registration in one - // synchronous section (same adapter instance, no NO_ADAPTER window). - disposeRoute() - disposeRoute = ctx.llm.registerAdapter([PROVIDER], adapter) + // fact per-request resolution cannot refresh. `replace` re-reads it in one + // synchronous registry section: disposing and re-registering instead would + // publish an empty route set between the two, and an observer that reacted + // to it would see this provider disappear and come back. + registration.replace([PROVIDER]) registeredPolicy = policy } diff --git a/packages/llm/llm-deepseek/tests/dynamic-config.spec.ts b/packages/llm/llm-deepseek/tests/dynamic-config.spec.ts index f5266f2d91..25cf4f293b 100644 --- a/packages/llm/llm-deepseek/tests/dynamic-config.spec.ts +++ b/packages/llm/llm-deepseek/tests/dynamic-config.spec.ts @@ -113,10 +113,18 @@ describe('request-level dynamic configuration', () => { ]) }) - it('re-registers the route in place when the captured retry policy changes', async () => { + it('re-registers the route in place when the captured retry policy changes, without an empty-registry window', async () => { const dir = await home() const { ctx } = await boot(dir, { apiKey: 'k', baseURL: 'http://127.0.0.1:1' }) + // Observing the topology event, not just the end state: disposing and + // re-registering also lands on the right final registry, but publishes an + // empty route set in between, so an observer sees the provider disappear. + const observed: string[][] = [] + ctx.on('llm/adapters-updated', () => { + observed.push(ctx.llm.listProviders().map(provider => provider.id)) + }) + await ctx.settings.update(NS, { retryPolicy: { mode: 'always', backoff: { initialDelayMs: 25, maxDelayMs: 100, jitterRatio: 0.2 } }, }) @@ -127,6 +135,7 @@ describe('request-level dynamic configuration', () => { jitterRatio: 0.2, }) expect(ctx.llm.listProviders()).toEqual([{ id: 'deepseek-official', name: 'DeepSeek' }]) + expect(observed).toEqual([['deepseek-official']]) }) it('keeps the last good options when a settings snapshot fails beyond-schema validation', async () => { diff --git a/packages/llm/llm/src/index.ts b/packages/llm/llm/src/index.ts index 5c9b091ef0..9a96d2f3d7 100644 --- a/packages/llm/llm/src/index.ts +++ b/packages/llm/llm/src/index.ts @@ -236,19 +236,32 @@ export class LlmService extends Service { let invariantFailure: unknown for (const listener of this.ctx.events.dispatch('emit', ['llm/adapters-updated']) as Array<() => unknown>) { try { - listener() + const returned = listener() + if (returned != null && typeof (returned as PromiseLike).then === 'function') { + // An emit listener may still be an async function; its rejection + // cannot reach the synchronous INVARIANT rethrow below, so it is + // contained here instead of becoming an unhandled rejection. + void Promise.resolve(returned as PromiseLike).then(undefined, (error: unknown) => { + this.warnAdaptersListenerFailure(error) + }) + } } catch (error) { if ((error as { code?: unknown } | null)?.code === 'INVARIANT') { invariantFailure ??= error continue } - this.ctx.logger.warn('llm: an llm/adapters-updated listener failed') - this.ctx.logger.warn(error) + this.warnAdaptersListenerFailure(error) } } if (invariantFailure !== undefined) throw invariantFailure as Error } + /** Contained-listener diagnostic shared by the sync and async failure paths. */ + private warnAdaptersListenerFailure(error: unknown): void { + this.ctx.logger.warn('llm: an llm/adapters-updated listener failed') + this.ctx.logger.warn(error) + } + /** * Register an adapter for the given provider routes. Throws `LlmError` with code * `DUPLICATE_ADAPTER` if any provider already has an adapter (all-or-nothing). diff --git a/packages/llm/llm/tests/topology.spec.ts b/packages/llm/llm/tests/topology.spec.ts index f33cd69399..f07b33af7d 100644 --- a/packages/llm/llm/tests/topology.spec.ts +++ b/packages/llm/llm/tests/topology.spec.ts @@ -53,6 +53,42 @@ describe('llm/adapters-updated', () => { expect(warn).toHaveBeenCalledWith('llm: an llm/adapters-updated listener failed') }) + it('contains an ASYNC listener rejection instead of leaving it unhandled', async () => { + // An emit listener may be an async function; its rejection cannot reach + // the synchronous catch, so an uncontained one escapes the process as an + // unhandled rejection rather than a warned observer failure. + const ctx = await setup() + const warn = vi.spyOn(ctx.logger, 'warn').mockImplementation(() => undefined) + const unhandled = vi.fn() + process.on('unhandledRejection', unhandled) + try { + // Typed as returning unknown so the listener is not a Promise-returning + // function type: the point is exactly that an async one may slip in. + const rejecting = (): unknown => Promise.reject(new Error('async observer')) + ctx.on('llm/adapters-updated', rejecting) + ctx.llm.registerAdapter(['a'], new NoopAdapter()) + expect(ctx.llm.listProviders().map(provider => provider.id)).toEqual(['a']) + await new Promise(resolve => setTimeout(resolve, 10)) + expect(unhandled).not.toHaveBeenCalled() + expect(warn).toHaveBeenCalledWith('llm: an llm/adapters-updated listener failed') + } finally { + process.off('unhandledRejection', unhandled) + } + }) + + it('replaces a route set in one event, never publishing an empty registry between the two', async () => { + // The retry-policy swap in llm-deepseek: disposing and re-registering + // would let an observer see the provider disappear and come back. + const ctx = await setup() + const observed: string[][] = [] + const registration = ctx.llm.registerAdapter(['a'], new NoopAdapter()) + ctx.on('llm/adapters-updated', () => { + observed.push(ctx.llm.listProviders().map(provider => provider.id)) + }) + registration.replace(['a']) + expect(observed).toEqual([['a']]) + }) + it('rethrows the first INVARIANT-coded listener failure after notifying the rest', async () => { const ctx = await setup() const later = vi.fn() diff --git a/packages/settings/settings/src/index.ts b/packages/settings/settings/src/index.ts index f356057d69..2eaf32e9a8 100644 --- a/packages/settings/settings/src/index.ts +++ b/packages/settings/settings/src/index.ts @@ -162,6 +162,44 @@ function isPlainObject(value: unknown): value is Record { return proto === Object.prototype || proto === null } +/** + * One path-addressed edit to a namespace's user section. Path mutation exists + * for a caller holding an INCOMPLETE view of the section — a configuration UI + * reads the redacted descriptor, which by construction never received the + * `role('secret')` fields. Such a caller can name the field it means without + * restating the section: a wholesale `replace` rebuilt from a redacted + * document silently deletes every secret the wire never returned. + */ +export type SettingsPathOp = + | { op: 'set'; path: readonly string[]; value: unknown } + | { op: 'unset'; path: readonly string[] } + +/** Apply one path op to a detached section, returning the next section. */ +function applyPathOp(section: Record, op: SettingsPathOp): Record { + const [head, ...rest] = op.path + // The empty path addresses the section itself. + if (head === undefined) { + if (op.op === 'unset') return {} + if (!isPlainObject(op.value)) { + throw new TypeError('settings mutate: setting the section root requires a plain object') + } + return { ...op.value } + } + if (rest.length === 0) { + if (op.op === 'set') return { ...section, [head]: op.value } + const { [head]: _removed, ...kept } = section + return kept + } + const child = section[head] + if (!isPlainObject(child)) { + // Unsetting through an absent path is already satisfied; setting through + // one creates the intermediate objects it needs. + if (op.op === 'unset') return section + return { ...section, [head]: applyPathOp({}, { ...op, path: rest }) } + } + return { ...section, [head]: applyPathOp(child, { ...op, path: rest }) } +} + /** Human label for a value rejected by the JSON-shape boundary (numbers reject inline). */ function describeRejected(value: unknown): string { if (value === undefined) return 'undefined' @@ -443,9 +481,32 @@ export abstract class Settings extends Service { return this.write(ns, section, 'replace') } + /** + * Apply path-addressed edits to one registered namespace's user section, + * validate, persist, then commit and emit. The ops are applied to the + * section as it stands when the write reaches the front of the queue, so a + * caller never has to restate fields it did not touch — and, crucially, + * cannot delete fields it never saw. This is the write path for any caller + * holding a redacted view; `replace` remains the wholesale reset. + * @param ns - the registered namespace to edit. + * @param ops - ordered path edits; later ops observe earlier ones. + */ + async mutate(ns: SettingsNamespace, ops: readonly SettingsPathOp[]): Promise { + if (!Array.isArray(ops)) throw new TypeError(`settings mutate for "${ns}" must be an array of path ops`) + for (const op of ops) { + if (!isPlainObject(op) || (op['op'] !== 'set' && op['op'] !== 'unset')) { + throw new TypeError(`settings mutate for "${ns}" ops must be {op:'set'|'unset', path}`) + } + if (!Array.isArray(op['path']) || (op['path'] as unknown[]).some(part => typeof part !== 'string')) { + throw new TypeError(`settings mutate for "${ns}" op paths must be arrays of strings`) + } + } + return this.write(ns, ops, 'mutate') + } + /** Validate a write, then queue it on the namespace's serialized write chain. */ - private write(ns: SettingsNamespace, input: object, mode: 'merge' | 'replace'): Promise { - const verb = mode === 'merge' ? 'update' : 'replace' + private write(ns: SettingsNamespace, input: object, mode: 'merge' | 'replace' | 'mutate'): Promise { + const verb = mode === 'merge' ? 'update' : mode === 'replace' ? 'replace' : 'mutate' const registration = this.registrations.get(ns) if (registration === undefined) { throw new Error(`settings namespace "${ns}" is not registered`) @@ -456,13 +517,19 @@ export abstract class Settings extends Service { if (!this.writable) { throw new Error(`settings provider is read-only: "${ns}" cannot be updated in-process`) } - if (!isPlainObject(input)) { - throw new TypeError(`settings ${verb} for "${ns}" must be a plain object`) + // A mutate's ops array is wrapped so one JSON-shape walk covers both + // shapes; merge/replace carry the section itself. + let payload: Record + if (mode === 'mutate') { + payload = { ops: input } + } else { + if (!isPlainObject(input)) throw new TypeError(`settings ${verb} for "${ns}" must be a plain object`) + payload = input } // Snapshot at call time: the queue must never read a caller-owned object // the caller may keep mutating while the write waits its turn. The same // walk is the JSON-shape boundary check (see cloneJsonShaped). - const snapshot = cloneJsonShaped(input, (label, path) => + const snapshot = cloneJsonShaped(payload, (label, path) => new TypeError(`settings ${verb} for "${ns}" must be JSON-shaped data (found ${label} at ${path})`)) const previous = this.writeQueues.get(ns) ?? Promise.resolve() // Chain past a failed predecessor: one rejected write must not poison the @@ -474,9 +541,14 @@ export abstract class Settings extends Service { if (this.registrations.get(ns) !== registration) { throw new Error(`settings namespace "${ns}" registration was disposed before the queued ${verb} ran`) } + // Every mode derives from the section as it stands NOW, at the front of + // the queue — never from whatever the caller last saw. + const current = this.section(ns) ?? {} const section = mode === 'merge' - ? mergeLayers(this.section(ns) ?? {}, snapshot) as Record - : snapshot + ? mergeLayers(current, snapshot) as Record + : mode === 'replace' + ? snapshot + : (snapshot['ops'] as SettingsPathOp[]).reduce(applyPathOp, current) const next = deepFreeze(this.resolve(registration.schema, registration.base, section)) await this.persist(ns, section) // The write reached storage either way; the cache must say so. Commit diff --git a/packages/settings/settings/tests/settings.spec.ts b/packages/settings/settings/tests/settings.spec.ts index 5f9ac7dae7..9fcf841551 100644 --- a/packages/settings/settings/tests/settings.spec.ts +++ b/packages/settings/settings/tests/settings.spec.ts @@ -725,3 +725,89 @@ describe('installSettingsSection', () => { expect(changes).toEqual(['user']) }) }) + +describe('mutate (path-addressed writes)', () => { + interface KeyedConfig { + apiKey: string + baseURL: string + reasoning: string + } + + const KeyedSchema: z = z.object({ + apiKey: z.string().role('secret'), + baseURL: z.string(), + reasoning: z.string(), + }) + + const KEYED = settingsNamespace('keyed') + const NESTED = settingsNamespace('workspace') + + async function mounted(doc: Record) { + const ctx = new Context() + await ctx.plugin(BareProvider, { doc }) + ctx.settings.register(KEYED, KeyedSchema) + return ctx + } + + it('removes one field without touching a secret the caller never saw', async () => { + // The data-loss shape this exists to prevent: a configuration UI reads the + // REDACTED descriptor (no apiKey), the user resets baseURL, and the client + // rebuilds the section from what it holds. A wholesale replace of that + // rebuild deletes the stored literal key; a path unset cannot. + const ctx = await mounted({ keyed: { apiKey: 'sk-stored', baseURL: 'https://user', reasoning: 'high' } }) + const redacted = ctx.settings.describe({ redactSecrets: true }).find(d => d.ns === KEYED)! + expect(redacted.user).toEqual({ baseURL: 'https://user', reasoning: 'high' }) + + await ctx.settings.mutate(KEYED, [{ op: 'unset', path: ['baseURL'] }]) + + const raw = ctx.settings.describe().find(d => d.ns === KEYED)! + expect(raw.user).toEqual({ apiKey: 'sk-stored', reasoning: 'high' }) + }) + + it('applies set and unset in one write, in order', async () => { + const ctx = await mounted({ keyed: { apiKey: 'sk-stored', baseURL: 'https://old' } }) + await ctx.settings.mutate(KEYED, [ + { op: 'set', path: ['baseURL'], value: 'https://new' }, + { op: 'set', path: ['reasoning'], value: 'low' }, + { op: 'unset', path: ['reasoning'] }, + ]) + expect(ctx.settings.describe().find(d => d.ns === KEYED)!.user) + .toEqual({ apiKey: 'sk-stored', baseURL: 'https://new' }) + }) + + it('reads the section as it stands at the front of the queue, not at call time', async () => { + // Two concurrent writers: the mutate is issued against the pre-update + // section but must observe the update that ran before it. + const ctx = await mounted({ keyed: { apiKey: 'sk-stored' } }) + const first = ctx.settings.update(KEYED, { baseURL: 'https://first', reasoning: 'high' }) + const second = ctx.settings.mutate(KEYED, [{ op: 'unset', path: ['reasoning'] }]) + await Promise.all([first, second]) + expect(ctx.settings.describe().find(d => d.ns === KEYED)!.user) + .toEqual({ apiKey: 'sk-stored', baseURL: 'https://first' }) + }) + + it('creates intermediate objects for a nested set and leaves an absent unset alone', async () => { + const ctx = new Context() + await ctx.plugin(BareProvider, { doc: {} }) + ctx.settings.register(NESTED, NestedSchema) + await ctx.settings.mutate(NESTED, [{ op: 'set', path: ['retry', 'attempts'], value: 5 }]) + expect(ctx.settings.describe().find(d => d.ns === NESTED)!.user).toEqual({ retry: { attempts: 5 } }) + await ctx.settings.mutate(NESTED, [{ op: 'unset', path: ['missing', 'deep'] }]) + expect(ctx.settings.describe().find(d => d.ns === NESTED)!.user).toEqual({ retry: { attempts: 5 } }) + }) + + it('rejects a malformed op before anything is queued', async () => { + const ctx = await mounted({ keyed: { apiKey: 'sk-stored' } }) + await expect(ctx.settings.mutate(KEYED, [{ op: 'delete' } as never])) + .rejects.toThrow(/must be \{op:'set'\|'unset', path\}/) + await expect(ctx.settings.mutate(KEYED, [{ op: 'unset', path: ['a', 1] as never }])) + .rejects.toThrow(/op paths must be arrays of strings/) + expect(ctx.settings.describe().find(d => d.ns === KEYED)!.user).toEqual({ apiKey: 'sk-stored' }) + }) + + it('rejects a value the JSON-shape boundary refuses', async () => { + const ctx = await mounted({ keyed: {} }) + await expect(ctx.settings.mutate(KEYED, [{ op: 'set', path: ['baseURL'], value: new Date() }])) + .rejects.toThrow(/must be JSON-shaped data/) + }) +})