fix(gui): address ds-review-bot findings on image attachments
- dsh web gains --provider/--model: a non-deepseek provider mounts the matching pi-ai catalog route (ambient credentials), making image input reachable from the shipped Web assembly; requires an explicit --model - attachment-local syncs the publication directories after the hard-link publish so a reported durable reference survives a crash (POSIX; Windows relies on filesystem metadata journaling) - the attachment seam gains storage-free validateImage; the host validates a complete multi-image prompt before persisting any member, so one malformed image cannot strand valid members as unreferenced objects - startSession sends before navigating: a rejected first send keeps the empty state, its error strip, and the complete draft mounted - the webserver rejects an undeclared-length body the moment it crosses the configured limit instead of draining a potentially endless stream to EOF
This commit is contained in:
@@ -1,6 +1,6 @@
|
||||
# @deepseek-ai/dsh-attachment-local
|
||||
|
||||
The private local implementation of [`@deepseek-ai/dsh-attachment`](../attachment). Objects land at `<DSH_HOME>/attachments/v1/objects/<sha256-prefix>/<sha256>` and are addressed by an opaque `sha256:` id. Writes use a private staging directory, owner-only files, a synced temporary file, and an atomic exclusive hard-link publish; reads re-check the digest, media signature, dimensions, and logged metadata. Byte and pixel limits are write-time admission policy, so a later policy reduction does not make already-admitted history unreadable.
|
||||
The private local implementation of [`@deepseek-ai/dsh-attachment`](../attachment). Objects land at `<DSH_HOME>/attachments/v1/objects/<sha256-prefix>/<sha256>` and are addressed by an opaque `sha256:` id. Writes use a private staging directory, owner-only files, a synced temporary file, an atomic exclusive hard-link publish, and a directory sync on the publication directories (POSIX; Windows relies on filesystem metadata journaling) so the reported reference survives a crash; reads re-check the digest, media signature, dimensions, and logged metadata. Byte and pixel limits are write-time admission policy, so a later policy reduction does not make already-admitted history unreadable.
|
||||
|
||||
`DSH_HOME` resolves through the shared path policy: explicit config, `$DSH_HOME`, then `~/.dsh`. Session logs contain only the reference and verified metadata, never this host path.
|
||||
|
||||
|
||||
@@ -6,10 +6,10 @@ import z from 'schemastery'
|
||||
import { AttachmentStore } from '@deepseek-ai/dsh-attachment'
|
||||
import type { ImageAttachmentLimits, ImageAttachmentRef, SaveImageAttachment, StoredImageAttachment } from '@deepseek-ai/dsh-attachment'
|
||||
import { resolveDshHome } from '@deepseek-ai/dsh-paths'
|
||||
import { readImageFile, saveImageFile } from './store.ts'
|
||||
import { readImageFile, saveImageFile, validateImageFile } from './store.ts'
|
||||
|
||||
export { detectImage } from './image.ts'
|
||||
export { readImageFile, saveImageFile } from './store.ts'
|
||||
export { readImageFile, saveImageFile, validateImageFile } from './store.ts'
|
||||
export { AttachmentError } from '@deepseek-ai/dsh-attachment'
|
||||
export type { ImageAttachmentRef } from '@deepseek-ai/dsh-attachment'
|
||||
|
||||
@@ -62,6 +62,10 @@ export class LocalAttachmentStore extends AttachmentStore {
|
||||
})
|
||||
}
|
||||
|
||||
validateImage(input: SaveImageAttachment): void {
|
||||
validateImageFile(input, this.imageLimits)
|
||||
}
|
||||
|
||||
async saveImage(input: SaveImageAttachment): Promise<ImageAttachmentRef> {
|
||||
return saveImageFile(this.root, input, this.imageLimits)
|
||||
}
|
||||
|
||||
@@ -52,6 +52,32 @@ function validateAdmission(metadata: Omit<ImageAttachmentRef, 'attachmentId' | '
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Run the full admission policy for one image without touching storage.
|
||||
* @param input - encoded bytes and declared metadata.
|
||||
* @param limits - resolved storage policy.
|
||||
*/
|
||||
export function validateImageFile(input: SaveImageAttachment, limits: ImageAttachmentLimits): void {
|
||||
validateAdmission(inspectMetadata(input.data, input.mediaType), limits)
|
||||
}
|
||||
|
||||
/**
|
||||
* Make a directory's entries durable (fsync on a read-only directory handle).
|
||||
* A synced file alone does not survive a crash when its directory entry never
|
||||
* reached storage, so the publication directory is synced before a durable
|
||||
* reference is reported.
|
||||
*/
|
||||
async function syncDirectory(path: string): Promise<void> {
|
||||
/* v8 ignore next -- Windows cannot open directory handles; NTFS metadata journaling owns entry durability there. */
|
||||
if (process.platform === 'win32') return
|
||||
const handle = await open(path, constants.O_RDONLY)
|
||||
try {
|
||||
await handle.sync()
|
||||
} finally {
|
||||
await handle.close()
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Save and verify immutable image bytes below a versioned attachment root.
|
||||
* @param root - absolute `DSH_HOME/attachments/v1` root.
|
||||
@@ -86,6 +112,13 @@ export async function saveImageFile(root: string, input: SaveImageAttachment, li
|
||||
const existing = new Uint8Array(await readFile(target))
|
||||
if (digest(existing) !== sha256) throw new AttachmentError('Stored attachment failed integrity verification.', 'ATTACHMENT_CORRUPT')
|
||||
}
|
||||
// The synced file becomes durable only once its directory entries are: sync
|
||||
// the bucket (the new object entry) and its parent (the possibly new bucket
|
||||
// entry) before this reference can reach a session checkpoint. The dedup
|
||||
// path syncs too — the earlier save that created the entry may have crashed
|
||||
// before its own directory sync.
|
||||
await syncDirectory(bucket)
|
||||
await syncDirectory(join(root, 'objects'))
|
||||
await unlink(temporary)
|
||||
} catch (error) {
|
||||
/* v8 ignore next -- A descriptor can remain open only when the underlying write/sync/close operation fails. */
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
import { Context } from 'cordis'
|
||||
import { existsSync } from 'node:fs'
|
||||
import { mkdtemp, rm } from 'node:fs/promises'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { join } from 'node:path'
|
||||
@@ -36,4 +37,22 @@ describe('local attachment service', () => {
|
||||
await rm(dshHome, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
|
||||
it('validates without persisting: a rejected image leaves no storage root behind', async () => {
|
||||
const dshHome = await mkdtemp(join(tmpdir(), 'dsh-attachment-validate-'))
|
||||
try {
|
||||
const service = new LocalAttachmentStore(new Context(), { dshHome })
|
||||
expect(() => { service.validateImage({ data: Uint8Array.of(1, 2, 3), mediaType: 'image/png' }) })
|
||||
.toThrow(/Unsupported or malformed image data/)
|
||||
const valid = Uint8Array.from(Buffer.from(
|
||||
'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII=',
|
||||
'base64',
|
||||
))
|
||||
expect(() => { service.validateImage({ data: valid, mediaType: 'image/png' }) }).not.toThrow()
|
||||
// Validation is storage-free: nothing below the root may exist yet.
|
||||
expect(existsSync(service.root)).toBe(false)
|
||||
} finally {
|
||||
await rm(dshHome, { recursive: true, force: true })
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
@@ -2,7 +2,7 @@
|
||||
|
||||
The durable attachment seam. `ctx.attachments` validates and atomically commits immutable image bytes, then returns a serializable `ImageAttachmentRef`; consumers never persist browser paths, object URLs, provider URLs, or base64 in session events.
|
||||
|
||||
Unsent composer images remain browser-owned temporary drafts. `saveImage` is called only at message submission or while committing structured provider output, before any model-visible session event is published. `readImage` verifies the content-addressed object against its logged metadata.
|
||||
Unsent composer images remain browser-owned temporary drafts. `saveImage` is called only at message submission or while committing structured provider output, before any model-visible session event is published. `validateImage` runs the same admission policy without persisting; batch writers validate every member first so one malformed member cannot strand earlier members as unreferenced objects (there is no garbage collection). `readImage` verifies the content-addressed object against its logged metadata.
|
||||
|
||||
## Model Experience
|
||||
|
||||
|
||||
@@ -33,6 +33,14 @@ export abstract class AttachmentStore extends Service {
|
||||
/** Deployment-resolved image policy used by authoritative and fast-path validation. */
|
||||
abstract readonly imageLimits: ImageAttachmentLimits
|
||||
|
||||
/**
|
||||
* Validate one image against the deployment policy without persisting anything.
|
||||
* Callers persisting a multi-image batch validate every member first so a
|
||||
* malformed member cannot leave earlier members as unreferenced objects.
|
||||
* @param input - encoded bytes, declared media type, and optional display name.
|
||||
*/
|
||||
abstract validateImage(input: SaveImageAttachment): void
|
||||
|
||||
/**
|
||||
* Validate and durably commit one image before its owning session event is appended.
|
||||
* @param input - encoded bytes, declared media type, and optional display name.
|
||||
|
||||
@@ -187,7 +187,11 @@ export interface EmptyStateInjected {
|
||||
releaseDraftImage(id: string): void
|
||||
/** Release all service-owned image previews held by the empty state. */
|
||||
releaseDraftImages(attachments: readonly ComposerAttachment[]): void
|
||||
/** The create → navigate → first-send chain, in one service call. */
|
||||
/**
|
||||
* The create → first-send → navigate chain, in one service call. Navigation
|
||||
* happens only after the send is accepted, so a failure leaves the empty
|
||||
* state and its draft mounted.
|
||||
*/
|
||||
startSession(opts: {
|
||||
cwd?: string
|
||||
text: string
|
||||
|
||||
@@ -209,12 +209,12 @@ export class ConversationService extends Service {
|
||||
|
||||
/**
|
||||
* Empty-state first-send chain (root-context method; does not read scope):
|
||||
* create the session, navigate to it, then send through the new scope.
|
||||
* The create → open ordering is safe: the manager merges the new summary
|
||||
* synchronously before create() resolves, so the list store is projected by
|
||||
* the time open() validates against it (manager notification batching is
|
||||
* microtask-based; SessionsService projects on the same flush that create
|
||||
* awaited through the RPC round trip).
|
||||
* create the session, send through the new scope, and navigate only after
|
||||
* the send is accepted. Navigation is the publication point — opening
|
||||
* earlier would unmount the empty state (releasing its draft previews)
|
||||
* while the send can still fail, leaving the failure with no surface and
|
||||
* the user with a lost draft; on rejection here the still-mounted empty
|
||||
* state keeps the draft and shows the error locally.
|
||||
* @param opts - project directory, prompt text, images, and send mode.
|
||||
*/
|
||||
async startSession(opts: {
|
||||
@@ -226,9 +226,10 @@ export class ConversationService extends Service {
|
||||
const sessions = this.requireSessions()
|
||||
const id = await sessions.create(opts.cwd === undefined ? {} : { cwd: opts.cwd })
|
||||
// The manager notifier flushes per microtask; one await guarantees the
|
||||
// list-store projection landed before sessions.open validates against it.
|
||||
// list-store projection landed before sessions.open validates against it
|
||||
// (the manager merges the new summary synchronously before create()
|
||||
// resolves; batching is microtask-based).
|
||||
await Promise.resolve()
|
||||
sessions.open(id)
|
||||
const scoped = sessions.scope(id)
|
||||
if (scoped === undefined) throw new Error(`conversation.startSession: created session "${id}" resolved no scope`)
|
||||
// ctx.get, not scoped.conversation: property access walks the fiber
|
||||
@@ -237,6 +238,7 @@ export class ConversationService extends Service {
|
||||
const scopedConversation = scoped.get('conversation')
|
||||
if (scopedConversation === undefined) throw new Error('conversation.startSession: conversation service unavailable through the new scope')
|
||||
await scopedConversation.send(opts.text, opts.mode, opts.images ?? [])
|
||||
sessions.open(id)
|
||||
}
|
||||
|
||||
/** Resolve the caller scope's Session or throw on root contexts. */
|
||||
|
||||
@@ -2,7 +2,7 @@
|
||||
/**
|
||||
* ConversationService orchestration half after the store-seat slimming:
|
||||
* scope-addressed send/cancel (result folding, root throw), the startSession
|
||||
* chain (create → sessions.open → scoped send), and the service-unavailable
|
||||
* chain (create → scoped send → sessions.open), and the service-unavailable
|
||||
* loud failures. Selection/draft state left this service for the declared
|
||||
* chat store (chat-store.spec.ts / selection-survival.spec.ts); the view
|
||||
* registry left for the 'conversation.view' slot (views-type-chain.spec.tsx).
|
||||
@@ -280,13 +280,23 @@ describe('image admission and URL lifecycle', () => {
|
||||
})
|
||||
|
||||
describe('startSession chain', () => {
|
||||
it('creates, navigates through sessions.open, then sends through the new scope', async () => {
|
||||
it('creates, sends through the new scope, then navigates through sessions.open', async () => {
|
||||
const b = await bench()
|
||||
await b.svc.startSession({ cwd: '/proj', text: 'first', mode: 'queue' })
|
||||
expect(b.createMock).toHaveBeenCalledWith({ cwd: '/proj' })
|
||||
expect(b.openMock).toHaveBeenCalledWith(sid('new-1'))
|
||||
expect(b.sessionDoubles.get(sid('new-1'))!.prompt).toHaveBeenCalledWith(
|
||||
[{ type: 'text', text: 'first' }], 'queue')
|
||||
const prompt = b.sessionDoubles.get(sid('new-1'))!.prompt
|
||||
expect(prompt).toHaveBeenCalledWith([{ type: 'text', text: 'first' }], 'queue')
|
||||
// Navigation is the publication point: it must not precede send acceptance.
|
||||
expect(b.openMock.mock.invocationCallOrder[0]!).toBeGreaterThan(prompt.mock.invocationCallOrder[0]!)
|
||||
})
|
||||
|
||||
it('does not navigate when the first send is rejected (empty state keeps the draft)', async () => {
|
||||
const b = await bench()
|
||||
const doomed = b.sessionsFake.manager.get(sid('new-1')) as unknown as SessionDouble
|
||||
doomed.prompt.mockResolvedValue({ ok: false, error: { code: 'agent-busy', message: 'nope' } })
|
||||
await expect(b.svc.startSession({ text: 'first', mode: 'queue' })).rejects.toThrow(/agent-busy/)
|
||||
expect(b.openMock).not.toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('omits cwd from create when not chosen', async () => {
|
||||
|
||||
@@ -156,6 +156,10 @@ export const SERVICE_API: readonly ServiceApiEntry[] = [
|
||||
key: 'attachments',
|
||||
summary: 'Immutable binary attachment service.',
|
||||
methods: [
|
||||
{
|
||||
signature: 'abstract validateImage(input: SaveImageAttachment): void',
|
||||
jsDoc: '/**\n * Validate one image against the deployment policy without persisting anything.\n * Callers persisting a multi-image batch validate every member first so a\n * malformed member cannot leave earlier members as unreferenced objects.\n * @param input - encoded bytes, declared media type, and optional display name.\n */',
|
||||
},
|
||||
{
|
||||
signature: 'abstract saveImage(input: SaveImageAttachment): Promise<ImageAttachmentRef>',
|
||||
jsDoc: '/**\n * Validate and durably commit one image before its owning session event is appended.\n * @param input - encoded bytes, declared media type, and optional display name.\n * @returns a durable content-addressed reference.\n */',
|
||||
|
||||
@@ -54,6 +54,16 @@ async function durablePromptContent(ctx: Context, content: readonly PromptConten
|
||||
if (totalBytes > limits.maxMessageImageBytes) {
|
||||
throw new AttachmentError('Prompt exceeds the configured aggregate image-byte limit.', 'IMAGES_TOO_LARGE')
|
||||
}
|
||||
// Validate the complete batch before persisting any member: the store has no
|
||||
// garbage collection, so one malformed image must not leave the batch's
|
||||
// valid members as published objects no message event will ever reference.
|
||||
for (const image of images) {
|
||||
ctx.attachments.validateImage({
|
||||
data: image.data,
|
||||
mediaType: image.part.mediaType,
|
||||
...image.part.name === undefined ? {} : { name: image.part.name },
|
||||
})
|
||||
}
|
||||
return Promise.all(prepared.map(async (item): Promise<ContentBlock> => {
|
||||
if (!('data' in item)) return { type: 'text', text: item.text }
|
||||
const attachment = await ctx.attachments.saveImage({
|
||||
|
||||
@@ -614,6 +614,31 @@ describe('sessions.prompt / cancel', () => {
|
||||
})
|
||||
})
|
||||
|
||||
it('publishes nothing when one member of a multi-image prompt is malformed', async () => {
|
||||
const persistenceRoot = mkdtempSync(join(tmpdir(), 'dsh-batch-session-'))
|
||||
const dshHome = mkdtempSync(join(tmpdir(), 'dsh-batch-home-'))
|
||||
host = await startHost({
|
||||
boot: { persistenceRoot, workspaceContext: false, dshHome, provider: 'scripted', model: 'test-model' },
|
||||
})
|
||||
host.ctx.llm.registerAdapter(['scripted'], new ScriptedAdapter([textResponse('unused')]))
|
||||
const { sessionId } = expectOk(await host.api.sessions.create(request({})))
|
||||
const response = await host.api.sessions.prompt(request({
|
||||
sessionId,
|
||||
mode: 'queue' as const,
|
||||
content: [
|
||||
{ type: 'image' as const, mediaType: 'image/png' as const, data: PNG_BASE64 },
|
||||
// Canonical base64, but the bytes are not a PNG: the whole batch must
|
||||
// be validated before any member persists, or the valid image above
|
||||
// would become a permanently unreferenced object (this store has no GC).
|
||||
{ type: 'image' as const, mediaType: 'image/png' as const, data: 'AQID' },
|
||||
],
|
||||
}))
|
||||
expect(response.result).toMatchObject({
|
||||
ok: false, error: { details: { reason: 'INVALID_IMAGE' } },
|
||||
})
|
||||
expect(existsSync(join(dshHome, 'attachments'))).toBe(false)
|
||||
})
|
||||
|
||||
it('rejects images for an explicitly text-only model without creating a session event', async () => {
|
||||
const persistenceRoot = mkdtempSync(join(tmpdir(), 'dsh-text-session-'))
|
||||
const dshHome = mkdtempSync(join(tmpdir(), 'dsh-text-home-'))
|
||||
|
||||
@@ -214,21 +214,19 @@ async function bridge(
|
||||
}
|
||||
const chunks: Buffer[] = []
|
||||
let received = 0
|
||||
let oversized = false
|
||||
for await (const chunk of req) {
|
||||
const buffer = chunk as Buffer
|
||||
received += buffer.byteLength
|
||||
if (received > maxRequestBodyBytes) {
|
||||
oversized = true
|
||||
chunks.length = 0
|
||||
continue
|
||||
// Reject the moment the threshold is crossed: draining a chunked body to
|
||||
// EOF first would let a client without Content-Length stream
|
||||
// indefinitely while holding the socket and this request task.
|
||||
res.writeHead(413, { connection: 'close' })
|
||||
res.end()
|
||||
req.destroy()
|
||||
return
|
||||
}
|
||||
if (!oversized) chunks.push(buffer)
|
||||
}
|
||||
if (oversized) {
|
||||
res.writeHead(413)
|
||||
res.end()
|
||||
return
|
||||
chunks.push(buffer)
|
||||
}
|
||||
/* v8 ignore next 3 -- `??` arms: node:http always sets url/method on server
|
||||
requests; the fields are only optional on the client-side IncomingMessage type */
|
||||
|
||||
@@ -435,6 +435,27 @@ describe('/api bridge', () => {
|
||||
expect(status).toBe(413)
|
||||
})
|
||||
|
||||
it('rejects an unterminated chunked body at the threshold without draining to EOF', async () => {
|
||||
const base = await boot(() => undefined, 8)
|
||||
const target = new URL(`${base}/api/echo`)
|
||||
// The client never calls end(): the 413 must arrive the moment the limit
|
||||
// is crossed, or a hostile stream would hold the socket open forever.
|
||||
const status = await new Promise<number | undefined>((resolve, reject) => {
|
||||
const request = httpRequest({
|
||||
hostname: target.hostname,
|
||||
port: target.port,
|
||||
path: target.pathname,
|
||||
method: 'POST',
|
||||
}, (response) => {
|
||||
response.resume()
|
||||
response.on('end', () => { resolve(response.statusCode) })
|
||||
})
|
||||
request.on('error', reject)
|
||||
request.write('123456789')
|
||||
})
|
||||
expect(status).toBe(413)
|
||||
})
|
||||
|
||||
it('relays a bodyless response', async () => {
|
||||
const base = await boot()
|
||||
const response = await fetch(`${base}/api/empty`, { method: 'POST' })
|
||||
|
||||
@@ -226,6 +226,10 @@ describe('PiAiAdapter provider routing', () => {
|
||||
mediaTypes: ['image/png'],
|
||||
}
|
||||
|
||||
validateImage(_input: SaveImageAttachment): void {
|
||||
throw new Error('not used')
|
||||
}
|
||||
|
||||
saveImage(_input: SaveImageAttachment): Promise<ImageAttachmentRef> {
|
||||
return Promise.reject(new Error('not used'))
|
||||
}
|
||||
|
||||
@@ -74,6 +74,10 @@ async function harness(image?: StoredImageAttachment): Promise<Context> {
|
||||
mediaTypes: [fixture.ref.mediaType],
|
||||
}
|
||||
|
||||
validateImage(_input: SaveImageAttachment): void {
|
||||
throw new Error('e2e attachment fixture is read-only')
|
||||
}
|
||||
|
||||
saveImage(_input: SaveImageAttachment): Promise<ImageAttachmentRef> {
|
||||
return Promise.reject(new Error('e2e attachment fixture is read-only'))
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user