fix: address ds-review-bot v8 findings
- llm-deepseek: the uncatalogued resolveModel fallback declares text-only modalities — the wire route is text-only regardless of catalog membership, so "unknown" must not let the host persist-then-fail images. - session.selectModel also consults the pending-inbox mirror: a queued image prompt enters the log only when claimed, after a switch would land. - attachment store: ensureDurableDirectory syncs every ancestor entry up to a caller-vouched boundary regardless of what mkdir reports — a raced "already existed" is not "already durable". - One image walker (imageBlockIn/imageInEvent) now serves both attachment authorization and the selection gate; referencedImage therefore also authorizes references inside wrapped message content. - InputHub: the scope disposer resolves the conversation service optionally (teardown/HMR must reach quiescence), and a send failing after its scope died releases the in-flight drafts instead of restoring them onto a disposed shell. - http-bridge destroys declared-oversize requests with connection: close instead of draining a body the client can trickle indefinitely. - LlmService validates AND detaches modality arrays identically on the advisory and exact routes; READMEs record the fourth INVALID_MODEL_INFO rejection reason. - CLI provider docs (JSDoc, README pair, Agent Note pair) describe the reuse behavior; llm-route.spec now parses the SHIPPED cordis.yml through the production extraction, pinning the row coupling. - image-display lane pins gallery/rail shape in inline snapshots and the object-URL scheme this environment must take; stale host.schema comment dropped.
This commit is contained in:
@@ -110,33 +110,48 @@ async function durablePromptContent(ctx: Context, content: readonly PromptConten
|
||||
}))
|
||||
}
|
||||
|
||||
function imageInContent(content: unknown, attachmentId: string): ImageAttachmentRef | undefined {
|
||||
/**
|
||||
* The ONE recursive block walk shared by attachment authorization and the
|
||||
* model-selection gate (nested tool-result content included). Both consumers
|
||||
* must agree on what counts as replayed image content — a route added to one
|
||||
* walker but not the other would silently skip authorization or stranding
|
||||
* protection — so there is exactly one walker, parameterized by match.
|
||||
*/
|
||||
function imageBlockIn(content: unknown, match: (ref: ImageAttachmentRef) => boolean): ImageAttachmentRef | undefined {
|
||||
if (!Array.isArray(content)) return undefined
|
||||
for (const value of content) {
|
||||
if (typeof value !== 'object' || value === null || Array.isArray(value)) continue
|
||||
const block = value as { type?: unknown; attachment?: unknown; content?: unknown }
|
||||
if (block.type === 'image' && typeof block.attachment === 'object' && block.attachment !== null) {
|
||||
const ref = block.attachment as ImageAttachmentRef
|
||||
if (String(ref.attachmentId) === attachmentId) return ref
|
||||
if (match(ref)) return ref
|
||||
}
|
||||
if (block.type === 'tool-result') {
|
||||
const nested = imageInContent(block.content, attachmentId)
|
||||
const nested = imageBlockIn(block.content, match)
|
||||
if (nested !== undefined) return nested
|
||||
}
|
||||
}
|
||||
return undefined
|
||||
}
|
||||
|
||||
/** Every replayed content route of one event: direct content, wrapped message content, streamed block-end. */
|
||||
function imageInEvent(event: SessionEvent, match: (ref: ImageAttachmentRef) => boolean): ImageAttachmentRef | undefined {
|
||||
const data = event.data as { content?: unknown; message?: { content?: unknown }; chunk?: { type?: unknown; block?: unknown } }
|
||||
const direct = imageBlockIn(data.content, match)
|
||||
if (direct !== undefined) return direct
|
||||
if (data.message !== undefined) {
|
||||
const wrapped = imageBlockIn(data.message.content, match)
|
||||
if (wrapped !== undefined) return wrapped
|
||||
}
|
||||
if (event.type === 'assistant/chunk' && data.chunk?.type === 'block-end') {
|
||||
return imageBlockIn([data.chunk.block], match)
|
||||
}
|
||||
return undefined
|
||||
}
|
||||
|
||||
/** True when any block (nested tool-result content included) is an image block. */
|
||||
function contentHasImage(content: unknown): boolean {
|
||||
if (!Array.isArray(content)) return false
|
||||
for (const value of content) {
|
||||
if (typeof value !== 'object' || value === null || Array.isArray(value)) continue
|
||||
const block = value as { type?: unknown; content?: unknown }
|
||||
if (block.type === 'image') return true
|
||||
if (block.type === 'tool-result' && contentHasImage(block.content)) return true
|
||||
}
|
||||
return false
|
||||
return imageBlockIn(content, () => true) !== undefined
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -145,23 +160,13 @@ function contentHasImage(content: unknown): boolean {
|
||||
* The log is immutable, so a true here is permanent for the session's life.
|
||||
*/
|
||||
function sessionHasImage(events: readonly SessionEvent[]): boolean {
|
||||
return events.some((event) => {
|
||||
const data = event.data as { content?: unknown; message?: { content?: unknown }; chunk?: { type?: unknown; block?: unknown } }
|
||||
if (contentHasImage(data.content)) return true
|
||||
if (data.message !== undefined && contentHasImage(data.message.content)) return true
|
||||
return event.type === 'assistant/chunk' && data.chunk?.type === 'block-end' && contentHasImage([data.chunk.block])
|
||||
})
|
||||
return events.some(event => imageInEvent(event, () => true) !== undefined)
|
||||
}
|
||||
|
||||
function referencedImage(events: readonly SessionEvent[], attachmentId: string): ImageAttachmentRef | undefined {
|
||||
for (const event of events) {
|
||||
const data = event.data as { content?: unknown; chunk?: { type?: unknown; block?: unknown } }
|
||||
const direct = imageInContent(data.content, attachmentId)
|
||||
if (direct !== undefined) return direct
|
||||
if (event.type === 'assistant/chunk' && data.chunk?.type === 'block-end') {
|
||||
const streamed = imageInContent([data.chunk.block], attachmentId)
|
||||
if (streamed !== undefined) return streamed
|
||||
}
|
||||
const found = imageInEvent(event, ref => String(ref.attachmentId) === attachmentId)
|
||||
if (found !== undefined) return found
|
||||
}
|
||||
return undefined
|
||||
}
|
||||
@@ -1141,7 +1146,11 @@ export function createApiProxy(ctx: Context, defaults: ApiProxyDefaults): ApiPro
|
||||
// wire routes reject image content on text-only models — accepting
|
||||
// this selection would strand the session (every turn fails, no
|
||||
// in-product recovery). Refuse at the selection boundary instead.
|
||||
if (sessionHasImage(found.agent.session.events)) {
|
||||
// The pending inbox counts too: a queued image prompt enters the log
|
||||
// only when claimed, which would happen AFTER this switch landed.
|
||||
const queuedImage = (queuedMirror.get(sessionId) ?? [])
|
||||
.some(entry => contentHasImage(entry.message.content))
|
||||
if (queuedImage || sessionHasImage(found.agent.session.events)) {
|
||||
const info = await ctx.llm.resolveModelInfo(resolved.provider, resolved.model)
|
||||
if (info.inputModalities !== undefined && !info.inputModalities.includes('image')) {
|
||||
return err(request, {
|
||||
|
||||
@@ -37,8 +37,6 @@ export const hostDescribeValueSchema = z.object({
|
||||
mediaTypes: z.array(imageMediaTypeSchema),
|
||||
}).optional(),
|
||||
attachedSessions: z.number().int().nonnegative(),
|
||||
// Open string, not a literal union: unknown kinds must survive the wire so
|
||||
// a merge-added capability can advertise (the client hides the affordance).
|
||||
}) satisfies z.ZodType<Wire<ResponseValue<'host.describe'>>>
|
||||
|
||||
/** host.pickDirectory request payload (empty object literal). */
|
||||
|
||||
@@ -274,6 +274,52 @@ describe('Web session model selection', () => {
|
||||
await ctx.fiber.dispose()
|
||||
})
|
||||
|
||||
it('refuses a text-only selection while an image prompt is still queued (not yet logged)', async () => {
|
||||
const { ctx, sessionId, agent } = await harness()
|
||||
ctx.llm.registerAdapter(['text-only'], new class extends CatalogAdapter {
|
||||
override resolveModel(provider: string, model: string): Promise<LlmResolvedModelInfo> {
|
||||
return Promise.resolve({ provider, id: model, name: model, inputModalities: ['text'] })
|
||||
}
|
||||
}('Text Only', []))
|
||||
const api = createApiProxy(ctx, { provider: 'deepseek', model: 'deepseek-chat', cwd: '/tmp', workspaceRoot: '/tmp' })
|
||||
// The queued message enters the session log only when claimed — after a
|
||||
// model switch would already have landed. The pending-inbox mirror must
|
||||
// therefore gate the switch too.
|
||||
ctx.emit('agent/inbox/enqueue', agent, {
|
||||
id: 'q-1', role: 'user', source: { kind: 'user' },
|
||||
content: [{ type: 'image', attachment: { attachmentId: 'att-q', mediaType: 'image/png', bytes: 8, width: 1, height: 1 } }],
|
||||
} as never, 'queued')
|
||||
const stranded = await api.sessions.selectModel(request({ sessionId, provider: 'text-only', model: 'plain' }))
|
||||
expect(stranded.result.ok).toBe(false)
|
||||
// Claiming the message drains the mirror; the log now owns the decision.
|
||||
ctx.emit('agent/inbox/dequeue', agent, { id: 'q-1' } as never, 'queued')
|
||||
expect(expectValue(await api.sessions.selectModel(request({
|
||||
sessionId, provider: 'text-only', model: 'plain',
|
||||
}))).selected).toEqual({ provider: 'text-only', model: 'plain' })
|
||||
await ctx.fiber.dispose()
|
||||
})
|
||||
|
||||
it('authorizes an attachment read referenced only from wrapped message content', async () => {
|
||||
const { ctx, sessionId, agent } = await harness()
|
||||
const ref = { attachmentId: 'att-w', mediaType: 'image/png' as const, bytes: 4, width: 1, height: 1 }
|
||||
ctx.provide('attachments', {
|
||||
readImage: () => Promise.resolve({ ref, data: new Uint8Array([1, 2, 3, 4]) }),
|
||||
} as never)
|
||||
const api = createApiProxy(ctx, { provider: 'deepseek', model: 'deepseek-chat', cwd: '/tmp', workspaceRoot: '/tmp' })
|
||||
// The only reference lives inside an assistant/message wrapper — the same
|
||||
// walk that gates model selection must authorize the read, or a real host
|
||||
// denies galleries the fixture (with its own authorization mirror) serves.
|
||||
agent.session.append('assistant/message', {
|
||||
turn: 1, step: 0,
|
||||
message: { id: 'a-1', role: 'assistant', source: { kind: 'model', provider: 'p', model: 'm' }, content: [{ type: 'image', attachment: ref }] },
|
||||
} as never, { surfaceOp: 'append' })
|
||||
const got = await api.sessions.attachment(request({ sessionId, attachmentId: 'att-w' as never }))
|
||||
expect(got.result).toMatchObject({ ok: true, value: { attachment: ref } })
|
||||
const denied = await api.sessions.attachment(request({ sessionId, attachmentId: 'att-other' as never }))
|
||||
expect(denied.result).toMatchObject({ ok: false, error: { details: { reason: 'ATTACHMENT_NOT_REFERENCED' } } })
|
||||
await ctx.fiber.dispose()
|
||||
})
|
||||
|
||||
it('detects images on every replayed route: wrapped messages, streamed blocks, nested tool results', async () => {
|
||||
const image = { type: 'image', attachment: { attachmentId: 'att-x', mediaType: 'image/png', bytes: 8, width: 1, height: 1 } }
|
||||
const cases: { label: string; append: (agent: Agent) => void }[] = [
|
||||
|
||||
Reference in New Issue
Block a user