refactor(attachments): let the descriptors own the preview predicate per review

The Open descriptor took `canPreview` from its context because the
descriptor list and the MIME predicate were built in parallel and could
not import each other. Both are on the branch now, so the injection is
just a way for one call site to be handed a looser answer — DR-16 puts
every "what can this MIME do" question in one module precisely so that
cannot happen, and an injected predicate admitting image/svg+xml would
reopen the hole the exact allowlist closes.

Imports canBrowserPreview directly, drops the context field, and pins the
SVG case in the descriptor tests.

Also states plainly what the timeline's transient re-probe does and does
not deliver: it makes the attachment eligible on the next effect run, it
is not a scheduled retry. Both from the orchestrator's cross-task pass.

web/src/lib/attachments/actions.test.ts

@@ -37,18 +37,15 @@ const { ATTACHMENT_ACTIONS, attachmentActionsFor, attachmentLinkUrl } = await im
 
 type Ctx = AttachmentActionContext;
 
-// A browser-preview predicate with the DR-19 shape: PDFs and plain text yes,
-// archives no. The real one is the shared display helper; the descriptors take
-// it as context precisely so this test doesn't need it.
-const PREVIEWABLE = new Set(['application/pdf', 'text/plain', 'image/png']);
-const canPreview = (mime: string) => PREVIEWABLE.has(mime);
+// The descriptors import the real `canBrowserPreview` (DR-16 keeps every
+// "what can this MIME do" question in one module), so these cases assert
+// against the shipping predicate rather than a stand-in.
 
 function ctx(overrides: Partial<Ctx> = {}): Ctx {
 	return {
 		workspaceSlug: 'ws',
 		attachment: { id: 'att-1', filename: 'report.pdf', mime_type: 'application/pdf' },
 		mutationsEnabled: true,
-		canPreview,
 		origin: 'https://pad.example',
 		...overrides,
 	};
@@ -93,6 +90,20 @@ describe('attachment action descriptors', () => {
 		expect(action('open').applies(zip)).toBe(false);
 	});
 
+	it('does not offer Open for an SVG, even though it is labelled image/*', () => {
+		// DR-16: the predicate is an exact allowlist, not an `image/` prefix,
+		// and the descriptors reach it directly — a caller cannot hand them a
+		// looser one. An SVG can carry active content, so it gets Download only.
+		const svg = ctx({
+			attachment: { id: 'att-3', filename: 'diagram.svg', mime_type: 'image/svg+xml' },
+		});
+		expect(attachmentActionsFor(svg).map((a) => a.id)).toEqual([
+			'download',
+			'copy-link',
+			'delete',
+		]);
+	});
+
 	it('offers Open for a PDF, as a new-tab anchor', () => {
 		const pdf = ctx();
 		expect(attachmentActionsFor(pdf).map((a) => a.id)).toEqual([

web/src/lib/attachments/actions.ts

@@ -24,10 +24,13 @@
  * The union is discriminated on `element`, so a renderer that switches on it
  * gets `href` or `run` narrowed for free and cannot reach for the wrong one.
  *
- * `canPreview` is INJECTED rather than imported (see `AttachmentActionContext`):
- * the "what can this MIME do" predicate lives in the display helpers, and
- * taking it as context keeps this module free of that dependency and trivially
- * testable.
+ * "Can the browser preview this?" is answered by importing `canBrowserPreview`
+ * from the display helpers, NOT by taking a predicate from the caller. DR-16
+ * puts every "what can this MIME do" question in one module precisely so a
+ * single call site cannot be given a looser answer — an injected predicate
+ * that admitted `image/svg+xml` would reopen the hole the exact-allowlist
+ * decision exists to close. (It was injected while this module and the
+ * predicate were built in parallel; collapsed at integration.)
  *
  * Not here, deliberately: `state_generation` and Undo. This plan's delete
  * behaves exactly like today's tile delete; the generation token, the event
@@ -37,6 +40,7 @@
 
 import { api } from '$lib/api/client';
 import { announceAttachmentDeleted } from '$lib/attachments/events';
+import { canBrowserPreview } from '$lib/attachments/display';
 import { copyToClipboard } from '$lib/utils/clipboard';
 
 export type AttachmentActionId = 'open' | 'download' | 'copy-link' | 'delete';
@@ -60,14 +64,6 @@ export interface AttachmentActionContext {
 	 * or a pane whose mutation gate is closed. Delete is disabled without it.
 	 */
 	mutationsEnabled: boolean;
-	/**
-	 * "Can the browser preview this MIME natively?" — the predicate that
-	 * decides whether Open exists at all (DR-19's Open note). Injected by the
-	 * host rather than imported here so this module owns actions and the
-	 * display helpers own MIME capability. Hosts pass the shared
-	 * `canBrowserPreview`.
-	 */
-	canPreview: (mime: string) => boolean;
 	/**
 	 * Origin for the absolute copy-link URL. Defaults to `location.origin`;
 	 * present so the URL builder is testable outside a DOM.
@@ -162,7 +158,7 @@ export const ATTACHMENT_ACTIONS: readonly AttachmentAction[] = [
 		// Only for what a browser previews natively. Never a Pad-rendered
 		// preview, and never offered for a .zip or an office document —
 		// those get Download only.
-		applies: (ctx) => ctx.canPreview(ctx.attachment.mime_type),
+		applies: (ctx) => canBrowserPreview(ctx.attachment.mime_type),
 		enabled: addressable,
 		href: (ctx) => api.attachments.downloadUrl(ctx.workspaceSlug, ctx.attachment.id),
 		target: '_blank',

web/src/lib/components/timeline/ItemTimeline.svelte

@@ -133,8 +133,13 @@
 		).then((m) => {
 			// A transient failure (5xx / network) is not evidence about the
 			// row, and the helper deliberately doesn't cache it — so drop the
-			// probed mark too, or this panel would never ask again for the
-			// rest of the session (PLAN-2392 DR-17). A `missing` result IS
+			// probed mark too, or this panel could never ask again for the
+			// rest of the mount (PLAN-2392 DR-17). Note what this does and
+			// does not buy: clearing the mark makes the attachment eligible
+			// again on the NEXT run of the probe effect (a new comment, an
+			// edit, a remount), it does not schedule a retry of its own. A
+			// proactive retry belongs with the timeline's deletion
+			// subscription in phase 3c, not here. A `missing` result IS
 			// authoritative: leave the mark set and leave `attMeta` without an
 			// entry, which is what the renderer already degrades to a missing
 			// placeholder on.