fix(web-config): close the wire boundary, the redacted-replace data loss, and three P2s
Five findings from the #939 review, each reproduced before being fixed. **Configuration reads are as privileged as writes.** `settings.describe` returns every exposed namespace's configuration and `credentials.describe` reports whether an arbitrary environment-variable name is configured and from where — reconnaissance no anonymous caller should have. Both join PRIVILEGED_METHODS, so the whole configuration plane is loopback-only until real authentication exists; `trustedHosts` was never authentication. The model catalog stays reachable: it carries no endpoints or key state, and a LAN client's model picker legitimately needs it. Asserted over a real HTTP server, because the Host header a browser actually sends is what decides this. **The proxy serves only namespaces a registered model provider addresses.** The settings seam is general — any plugin may register one — but the Web configuration plane is the model-provider surface. Without the gate, every future `settings.register()` would silently become remotely readable and writable configuration. An unregistered namespace and an unexposed one answer identically, so no caller can enumerate the registry one probe at a time. **Path-addressed writes replace the redacted-document rebuild.** The editor reads the REDACTED descriptor, so rebuilding a section from it and replacing wholesale deleted every literal secret the wire never returned — reproduced as `{baseURL, reasoning}` in, stored `apiKey` gone out. `settings.mutate` applies set/unset ops to the section as it stands at the front of the seam's write queue, and the client names only fields it can see, so an unseen secret is untouched by construction rather than by care. P2s in the same pass: `llm/adapters-updated` now contains async listener rejections (an uncontained one escaped as unhandledRejection, contradicting the documented "observer failures are contained"); llm-deepseek's retry-policy swap uses the atomic `registration.replace` instead of dispose-then-register, which published `[]` then `["deepseek-official"]` so an observer saw the provider disappear and come back; and a transport rejection no longer strands the page in `loading` or a card in `busy`, with removal failures surfaced on the page banner instead of swallowed.
This commit is contained in:
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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<unknown>).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<unknown>).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).
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user