Tapping a file used to do the most destructive-adjacent thing available:
a strip tile was a bare `<a download>`, so one tap put the file in your
Downloads folder with no way to see what it was first. This is what a tap
opens instead — `AttachmentDetailsPanel`, plus the one host that owns it.
PLAN-2392 phase 2, wave B. Nothing routes into the panel yet; the strip's
tiles and the editor's chips start emitting the open event in TASK-2424,
so the panel is driven here through its host and the events bus.
Presentation is the existing `Menu` with `sheetOnMobile` — a popover on
desktop, a BottomSheet at the mobile breakpoint (DR-6). No new overlay
primitive, so ESC ordering, outside-click, placement and the sheet's focus
handling are the app's existing ones rather than second implementations.
The actions are NOT defined here: they are rendered from the shared
descriptor list (DR-5), choosing between MenuItem's anchor and button
branches on the descriptor's own `element` discriminant and never calling
`run()` on an anchor. Adding an action stays a one-descriptor change.
It opens IMMEDIATELY and completes the metadata after (DR-2, DR-10). The
event's filename / mime / size are nullable by contract, so the panel
paints what it was handed and fetches the rest itself: `ok` fills the gaps,
`missing` (404) latches an authoritative "no longer available" with every
action inert, and `transient` shows an inline error beside the row it
already knows, with a Retry that goes through `revalidateAttachmentMetadata`
— a plain refetch would replay the cached failure and look broken.
Delete is an in-app drill-down sub-view (DR-18), the item menu's shape
exactly: prompt as `role="presentation"` with an aria-describedby
back-reference, Cancel FIRST, destructive row last, and the strip's
contextual "still used in this item's content" warning carried through
(read at confirm time from the LIVE editor markdown, since the persisted
body lags). It is wired as the delete descriptor's `confirmDelete` promise
rather than as a bespoke path, so the descriptor's identity snapshot and
permission re-check across the confirmation stay in force. The unreferenced
arm stays hedged: this can only speak for the HOST's content, and the
event's `itemId` is routing, not ownership.
The host is `ItemDetail`, through a small `AttachmentPanelHost` it mounts
beside the strip. It consumes an event only when BOTH `itemId` and
`hostToken` are its own (DR-8), and supplies `mutationsEnabled` itself —
never the NodeView's (it has no mutation context) and never the timeline's
`canEdit` (which ignores `peeking` and would let a peeked pane mutate). The
host is a component rather than a block inside ItemDetail because the
addressing rule has to be testable with two hosts mounted at once, which is
what the pane host does at runtime.
Parent lifecycle (DR-14): an archived parent's attachment fetch returns a
generic 404, so archive CLOSES the panel and restore REVALIDATES it rather
than assuming the previous state holds. The strip sits outside ItemDetail's
keyed lifecycle block, so this is added, not inherited; it arrives
declaratively as `parentArchived`, following the item ItemDetail already
refetches on the SSE lifecycle events.
Long filenames and RTL are handled with logical properties throughout,
`min-width: 0` on every flex child holding the name, and the full unelided
filename in both `title` and the panel's accessible name (DR-13).
No `state_generation` and no Undo (DR-19) — Delete behaves exactly like
today's tile Delete; PLAN-2411 adds the generation token and the Undo toast
to all three entry points at once.
Also here:
- `describeAttachmentType` in the shared display helpers, built on
`iconForAttachment` so the words and the icon beside them cannot disagree
about what a file is.
- `liveEditorMarkdown` extracted in ItemDetail — the strip and the panel now
read the live body through one accessor instead of two copies.
Tested through the host (20 jsdom cases): addressing with two hosts mounted,
open-with-partial-then-complete, all three metadata arms, Retry's
invalidate-before-refetch, host-supplied permission for peeked vs master,
the full confirm/cancel/failure delete paths, both warning arms, archive-
closes / restore-revalidates, item switch, and re-targeting in place. Focus
entry and return, background inertness, real placement, the sheet swap and
Enter/Space activation are browser-only and belong to phase 3d.
The addressing fix in the previous commit could not have worked. Tiptap's
`options` is a getter returning a fresh spread on every access
(@tiptap/core 3.22.5, dist/index.cjs:3452), so `ext.options.itemId = next`
mutates a temporary and is discarded — an assignment that looks exactly
like working code.
So the address stops being two strings pushed in after the fact and
becomes a reader the host supplies once and keeps honest: a closure over
its own live props, called at emit time. That is correct for a host that
is remounted per item (the body editor) and one that is reused across an
item switch (the comment composer) without either knowing which it is.
New $lib/attachments/hostAddress.ts states the contract; its test pins
the dependency behaviour that forces it, so a future Tiptap bump that
makes options writable fails a test instead of quietly inviting the
mutation approach back. The event predicate now reuses isAddressable so
'both halves required' is stated once.
Also completes the NodeView teardown fence (both MIME probe
continuations and swapNodeUuid could run after destroy) and adds the
stable-async-confirm and ordering cases the delete tests were missing —
without them, a regression dropping every async-confirmed delete passed.
The same dependency trap makes the editor's existing capabilities push a
no-op; that is pre-existing and independent, filed as BUG-2426.
All three from the orchestrator's second fresh-angle Codex pass.
From the orchestrator's fresh-angle Codex pass. All four are the same
shape — something read after an await, or captured once and never
refreshed — on a component tree built around a no-{#key} item switch.
- The delete descriptor snapshotted identity AFTER its confirmation, so
an async in-app confirm (which is what DR-18 asks for) left a window
where the user could switch items and delete the attachment they were
no longer looking at. Snapshot first, re-check the gate and the
identity on the way out.
- MenuItem's anchor rows had no Space activation. Native anchors take
Enter only, and role=menuitem does not add it, so Space would do
nothing on Download and Open while working on every button row beside
them.
- The image NodeView had no destroyed flag, so a HEAD probe in flight at
teardown could latch a placeholder onto detached DOM. The chip NodeView
has always had one.
- CommentEditor configures its extensions once in onMount, but the
composer is deliberately reused across an item switch, so its chips
kept emitting events addressed to the PREVIOUS item — which the host
then correctly ignored, i.e. a tap that silently did nothing. Push the
addressing onto the live options, the same way capabilities are pushed.
The first two fixes are mutation-tested: reverting either fails the new
test.
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.
An <img> whose load just failed asks probeForMissing whether the row is
gone. It was reading fetchAttachmentMetadata, whose page-lifetime cache
holds a prior `ok` observation — so an attachment deleted after that
observation still read as live and the permanent placeholder could never
latch. A cache of "what is this?" structurally cannot answer "is this
still there?".
Adds revalidateAttachmentMetadata (invalidate, then fetch) and routes the
existence probe through it. Caching of ok/missing is unchanged for the
metadata question, and DR-10's Retry gets the invalidate-before-refetch
primitive it needs.
Found by the orchestrator's Codex pass on TASK-2420.
PLAN-2392 DR-5: the panel and the viewer share one action list only if the
list IS the source of truth, so open / download / copy link / delete become
descriptors in web/src/lib/attachments/actions.ts. Adding an action means
adding one descriptor; both renderers consume the same set.
The element is part of the contract: Download stays a real `<a download>`
because the server sends an inline disposition for most accepted types (a
plain navigation would view rather than save — DR-16), and Open needs new-tab
/ middle-click semantics. So the descriptor type is a union discriminated on
`element`: anchors carry href/download/target/rel and no run() (the browser
performs the action; a renderer calling both would fire it twice), buttons
carry run(). Open is omitted entirely — not disabled — for types a browser
cannot preview, via a `canPreview` predicate taken from the context rather
than imported, keeping MIME capability with the display helpers.
Copy link copies location.origin + downloadUrl(...) because downloadUrl is
relative, and names itself "Copy workspace link" so the semantics are honest:
it is not a share link (DR-5a). Delete behaves exactly like today's tile
delete — api.attachments.delete plus announceAttachmentDeleted, with a 404
treated as authoritative — and deliberately carries no state_generation and
no undo; that wiring lands in PLAN-2411 across all three entry points at
once (DR-19).
MenuItem gains the two capabilities the panel needs, both additive: an icon
SNIPPET alongside the string icon (the string is interpolated as text, so SVG
markup would render as literal angle brackets — DR-3b), and an anchor branch.
A disabled anchor falls back to a disabled button: `<a>` ignores `disabled`,
stays focusable and still navigates, and Menu's keyboard navigation skips
rows via `[role^="menuitem"]:not(:disabled)`, which no anchor can match.
Tests cover the descriptor contract (open absent for a .zip, present for a
PDF; download's filename attribute; the absolute same-origin copy URL and its
clipboard-failure path; delete disabled without mutations, its 404-as-success
path and its error propagation) and MenuItem's unchanged button rendering
alongside the new snippet and anchor branches.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
The addressing layer the attachment options panel needs (PLAN-2392 DR-2 /
DR-8). No visible behaviour change — nothing consumes the channel yet.
`$lib/attachments/events.ts` gains the panel channel:
`AttachmentPanelOpenEvent` ({attachmentId, itemId, hostToken, anchor,
filename, mime_type, size_bytes} — the three metadata fields nullable
because a chip fills them from an async HEAD probe that may be incomplete
or failed, while the strip always has all three from its list row),
`notifyAttachmentPanelOpen`, `registerAttachmentPanelListener`,
`createAttachmentHostToken` and `isAttachmentPanelEventForHost`.
The two identity fields are the point. The bus is module-global, but
`ItemDetail` is mounted more than once at a time — the pane host runs a
master plus a peeked pane, which can be showing the same item. `itemId`
alone would let both hosts consume one NodeView's event (two panels for
one tap, one of them permissioned by the wrong host's mutationsEnabled).
So a host consumes an event only when BOTH fields are its own, and a
null/empty token on either side matches nothing — an unconfigured
NodeView must not be able to address every host at once.
`ItemDetail` mints ONE token per mount (a plain const, stable across the
no-{#key} item switch) and passes it to every attachment surface it owns:
the strip (which had no token path at all), both `Editor` branches, and —
through `ItemTimeline` and `TimelineCommentCard` — every `CommentEditor`.
`itemId` + `hostToken` are threaded into `AttachmentChipOptions` and
`AttachmentImageOptions` and wired at both configure sites.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
`fetchAttachmentMetadata` collapsed 404, every other non-2xx and network
throws into a single `null` — and cached it. Two consequences: nothing
could treat "gone" as authoritative (so editor undo resurrected deleted
attachments as live-looking nodes), and a one-off blip was sticky for the
page's lifetime.
It now returns a discriminated result: `ok` carries mime/size, `missing`
is the authoritative 404, `transient` is everything else. `ok` and
`missing` stay cached; `transient` is evicted the moment it settles, so a
retry re-issues the HEAD while concurrent callers still share one
in-flight request (PLAN-2392 DR-17).
Both NodeView consumers act on the split. The chip latches the
missing/deleted treatment on `missing` and leaves the filename-guess icon
alone on `transient`. The image NodeView probes on load failure — an
<img> error event carries no status code, so a deleted row and a network
blip are indistinguishable there — and only a 404 latches the permanent
placeholder; the toolbar's MIME probe latches too, since it may beat the
image to the answer. ItemTimeline drops its probed-mark on `transient` so
a blip doesn't permanently strand an entry's metadata.
Adds `canOpenInViewer` (DR-16: exact five-type raster allowlist, not an
`image/` prefix — SVG carries active content and TIFF/HEIC may not decode)
and `canBrowserPreview` (DR-5: that set plus PDF and text/plain) next to
`isImage`, which survives unchanged as the general picture predicate.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
Four review rounds running found the same bug class: a continuation
resuming after an await and writing state that belongs to a view the
user has already left. Each round fixed instances; the next found more.
The tripwire an earlier round set has been hit, so the invariant is
hoisted into one implementation instead of N call sites agreeing by
convention.
web/src/lib/attachments/viewFence.ts owns it now:
- viewIdentity(read) — the ONE place a component states what names its
view. Returns tokens carrying a SNAPSHOT of the parts, so a
continuation reads the workspace it was issued for off the token
rather than off the live prop. A missing part voids the whole key,
and a null key never matches — a half-identified view cannot pass a
fence.
- createFence(identity) — generation + identity. begin() coexists with
its siblings; restart() supersedes them; invalidate() ends them.
Used twice per surface, for fences 1 and 2.
- createPaintFence(identity) — fence 3, the paint-time entry check.
All three stay distinct: a prior round established that collapsing any
two loses either A→B suppression or same-item-Retry reconciliation.
Both consumers now build all three from a single identity declaration,
so no call site can restate a shorter one — which was the recurring
mistake (the workspace half kept going missing).
Two outstanding findings fixed alongside:
- StorageTab delete was not workspace-fenced: it used the live wsSlug
after its await, so an A→B switch mid-request let the success/404
handling toast and reload against B. It now takes the workspace off
the PAINTED identity (so the DELETE targets the row the user
actually clicked), refuses a click whose paint is already stale, and
fences the toast + reload. The broadcast stays ahead of the fence: a
global (workspace, id) side effect, not a write into this view.
- The strip's pendingUploads could resurrect externally deleted rows:
the buffer was retained indefinitely and no successful response ever
consumed it, so a deletion from another tab, followed by a load that
legitimately returned no row, merged the stale upload back in — and
kept doing so. A response is now treated as authoritative about the
entries the buffer already held when that request went OUT; entries
announced while it was in flight (the buffer's actual purpose) are
untouched.
No behaviour change from the refactor: all 592 existing tests pass
unmodified. 18 added — 14 unit tests on the module, 2 per fix. Every new
test mutation-tested.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
Second final-review round on the attachment strip. The previous commit split
request identity from view identity; this closes the holes that split left.
- handleDelete is now fenced at ENTRY against `paintedView`, the (workspace,
item) the clicked tile was actually painted for. Props update synchronously
and the load effect repaints later, so a click in that window could send a
DELETE while the user was already on another item or workspace — and the
`viewChanged` check in the catch runs after the request, so it could suppress
the rollback but never unsend the call.
- A 404 from the delete now broadcasts BEFORE the view fence. The row really is
gone; that fact is global, and skipping the broadcast left every other mounted
surface stale with nothing left to correct it. Only the local rollback/toast
stay view-scoped.
- The upload listener consults the tombstone set, so a re-announced upload can
no longer resurrect a deleted row into `attachments` and `pendingUploads`.
- `switchedAway` compares the (workspace, item) pair, like `viewChanged` and
`viewKey` already did.
Documents the resulting model at the top of the component: three fences for
three distinct questions — is this response current, may this continuation
reconcile, does this control belong to what is on screen.
Final full-diff review of PLAN-2392 phases 1/1b found four issues the
per-task reviews structurally could not see.
P2 — the strip fenced its delete mutation on the LOAD generation, which a
Retry also bumps. A delete failure landing during a same-item Retry was
therefore mistaken for an A→B item switch and silently swallowed: no
rollback, no toast, no 404 broadcast. Split the single counter in two —
`loadGeneration` (per request, bumped by every effect run including
Retry) still fences load responses; a new `viewGeneration` (bumped only
when the view actually changes, plus onDestroy) fences mutations. The
effect cleanup deliberately bumps only the request generation, since it
also runs before a Retry's re-run.
View identity is the (workspace, item) pair, not the item: `wsSlug` is
reactive and the strip survives a workspace change, so an item-only key
would read that change as a same-item Retry. `viewKey()` now backs the
retry marker, the painted-error owner, and the mutation fence alike.
Exposed by the new test: the rollback splice was not idempotent. A
reload that restored the row while the delete was in flight made it
duplicate the id and the keyed each block threw. The rollback now skips
the re-insert when the row is already back, and still toasts.
P2 — Storage's Retry now performs the same attachment metadata-cache
invalidation the strip's does, so a failed HEAD cached during the outage
doesn't stay poisoned on chips and inline images after recovery. While
there, loadList captures and re-checks its request workspace: the
generation alone couldn't tell a superseded workspace from the current
one, and both the rows and the cache keys are workspace-scoped.
P3 — StorageTab's `anyFilterActive` re-listed the filter fields instead
of reusing `selections()`; one projection now, so they cannot drift.
P3 — the deletion tombstone set survives a Retry and rides a
workspace-wide bus, so it is bounded like the list buffers, shedding
oldest-first (the newest tombstone is the one still racing a response)
and refreshing an id's age when it is re-announced.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
MAX_FETCH was documented as a cap but only ever reached the fetch `limit`:
the upload path prepended unconditionally and `pendingUploads` was itself
unbounded, so a long paste session grew the in-memory list — and the
lightbox set it feeds — without limit. Every growth path now runs through
`capped()`: the load-time merge, the upload event, the pending buffer that
rides on top of the merge, and the failed-delete rollback (PLAN-2392
DR-11).
A failed list fetch used to render as "no attachments", making a broken
strip and an empty one indistinguishable. It now shows a compact
"Couldn't load attachments · Retry", and Retry invalidates the shared
per-attachment HEAD-metadata cache before refetching — that cache latches
`null` on failure for the page lifetime, so a naive retry would replay the
cached failure on every surface that probed during the same outage
(DR-10). Retry also keeps what the failure preserved (optimistic uploads,
deletion tombstones) rather than clearing like an item switch. A delayed
loading row makes loading, empty and failed three distinguishable states
without flashing a block above the editor on the common un-attached item;
empty still renders no section at all (DR-18).
The header shows the true count, `50+` once rows exist past the bound, and
the overflow continuation is now item-scoped: "View all (N)" links to
`?attachment_item=<id>#storage`, the settings route passes it through and
owns clearing it, and StorageTab seeds its existing `item_id` filter from
it with a visible, clearable scope chip plus its own loading/error states.
The count is tracked as a delta beyond the strip, anchored on the server's
`total` and corrected for rows the page reported as deleted and for
uploads the page predates — so local deletes and uploads keep it honest.
Two residuals are deliberate and commented at the call site: a deletion of
a row PAST the bound can't be attributed (the bus is workspace-wide) and
may overstate by one until the next load, and uploads shed by the pending
buffer during a single in-flight request are uncounted because counting
them double-counts the ordinary case.
The test asserting a failed fetch shows no visible error is deliberately
falsified and replaced.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
Replaces the three independent emoji icon helpers on the live attachment
surfaces with one mapper and one monochrome SVG icon set (PLAN-2392 DR-3,
DR-3a, DR-3b).
- display.ts: categoryIcon -> iconForAttachment(mime, filename), returning
an icon identifier rather than an emoji. MIME first, filename extension
second, generic file last -- never a question mark. isImage and
formatBytes keep their signatures; StorageTab imports all three.
- attachments/icons/: one currentColor-driven icon per format family, with
TWO render paths over one path table -- AttachmentIcon.svelte for Svelte
call sites, iconSvg() for the editor chip, which builds DOM imperatively
and cannot mount a component.
- attachment-chip.ts: iconForMime, iconForFilename and its local formatBytes
deleted. The call site keeps its hide-zero/unknown-size conditional; the
shared formatter renders "0 B" and does not grow a mode (DR-3b).
- mime-families.json: the shared MIME -> family map, inside the web root
because vitest cannot read outside it. A Go test asserts the server upload
allowlist is fully covered by it (and carries no strays), so the two lists
cannot drift silently; the web test covers one representative MIME per
family plus the unknown-MIME and no-extension cases.
CopyItemDialog and markdown/attachments.ts are deliberately untouched.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
Cross-workspace copy authorized the source item and the destination
collection but never the individual attachments it cloned.
PlanAttachmentCopy scoped every lookup to `workspace_id =
SourceWorkspaceID AND deleted_at IS NULL` — but the workspace is not the
caller, so a restricted member who could edit any item in the source
workspace could paste `pad-attachment:<uuid>` for an attachment on an
item they could not see, copy that item into a workspace they own, and
read the bytes through the ordinary blob endpoint (BUG-2407).
The planner now consults an AttachmentAuthorizer supplied by the caller,
applied to every row it resolves: the referenced rows, the parents it
adopts as clone roots, and the variants it follows. A denial DELETES the
row from the resolution map, so it is indistinguishable from a row that
was never there — the reference lands in UnresolvableRefs beside
dangling, soft-deleted and foreign ids, and attachment_count /
attachment_bytes / unresolvable_ref_count read identically. The
preflight's numbers stay oracle-free.
It is a callback because the rule is the read path's — resolve the
parent, reject a foreign or non-live one, check item visibility, apply
the orphan rule — and every input to it lives in package server. It
cannot run BEFORE planning either: the copy re-reads the source content
under its locks and computes destination fields inside its transaction,
so a reference set enumerated beforehand is not the set the planner
resolves. Authorizing the rows the planner actually resolved keeps the
dry run and the copy on one path, which is the property DR-11 exists to
protect. Both endpoints take the authorizer off the same shared
resolution (resolveAuthorizedCopy), so what the preview calls
unresolvable is what the copy refuses to clone.
Mutation-verified: without the authorizer the secret PNG is cloned into
the destination, referenced by the rewritten body, and served byte-identical
to the attacker through the destination workspace.
Found by the convergence sweep of this branch, which enumerated every
attachment-touching path and compared each against its siblings' gates.
The delete handler answered 400 derived_attachment as soon as it saw a
ParentID, before any visibility, restriction, role or edit gate. That 400
is reachable only for a row that exists and is live, so a guessed
thumbnail UUID answered 400 while an absent, foreign, or deleted id
answered the shared 404 — and a caller who could not see the parent, or
was restricted out of its collection, learned about the row anyway. Fifth
instance of this handler family's existence oracle.
Moved after the authorization switch. The classification is a usage
error, so it may only be reported to someone already entitled to act on
the row; the test pins BOTH halves, so the fix cannot regress into
blanket-404ing a legitimate mistake by an authorized caller.
Mutation-verified: restoring the previous position makes the restricted
caller receive 400 again.
Gates: make check exit 0, make test-pg exit 0, zero failures.
Found by the convergence review of this branch. The orphan branch of the
delete path called requireMinRole("editor") before
attachmentCallerIsRestricted, so a restricted member who guessed a live
orphan's UUID got 403 while a bad UUID got 404 — confirming the row
exists. Fourth instance of the same existence oracle on this branch, and
the one path whose gate ORDER the refactor did not re-check.
Notable because attachmentCallerIsRestricted's own contract, added in the
previous commit, states that callers must apply it ahead of any role gate
that would answer 403. Centralizing the invariant did not fix call-site
ordering; only re-reviewing did.
Test covers both restricted roles: a viewer and an editor answer
differently at the role gate (403 vs success), and NEITHER may be
distinguishable from the lookup miss. Mutation-verified — restoring the
previous order yields exactly "status = 403, want 404".
Gates: make check exit 0, make test-pg exit 0, zero failures.
Per the final full-diff review of this branch. The six task commits each
added authorization to a different attachment path, and each was reviewed
CLEAN on its own — but they hand-rolled the same invariant four ways, and
the drift between them opened two real gaps that no per-task review could
see.
Root cause: the blob read, transform, thumbnail derivation and delete
paths each loaded the parent item, checked workspace identity and checked
liveness in their own shape. resolveAttachmentParentItem is now the one
place that invariant lives, returning a four-way outcome (orphan / ok /
gone / foreign) so callers keep their own denial behaviour — which is
deliberate, not accidental: the HTTP paths must not distinguish the
outcomes (any split is an existence oracle), derivation logs a distinct
WARN per outcome (greppable ahead of PLAN-2397's repair), and delete
passes includeArchived because the storage listing intentionally surfaces
archived-parent rows so their quota can be reclaimed.
Gaps the drift opened, both closed here:
- Orphan GET lacked the full-access gate transform and delete apply, so a
restricted member who guessed an orphan attachment's UUID could download
it — while transform, delete and the listing all refused. Now shared as
attachmentCallerIsRestricted, applied ahead of any role gate, since a
403 reached only for rows that exist is itself the oracle.
- The delete path still routed invisible parents through requireItemVisible
("Item not found") while missing and foreign attachments got "Attachment
not found" — the same existence oracle already closed twice on this
branch, left inconsistent on the one path the tasks did not touch. Every
delete denial now goes through the shared writer, asserted byte-identical.
Also folds in the live-parent write invariant on upload, which had been
applied to transform only: upload validated the item before spooling and
then inserted with plain CreateAttachment, so archiving during the upload
window bound a row to an archived parent. Derivation deliberately still
does NOT take the lock — that trade is documented on deriveThumbnails.
Gates: make check exit 0, make test-pg exit 0 (zero failures). Both new
guards mutation-verified; attachment authz suite clean under -race -count=2.
ResolveUserPermission matched item grants on item_id alone, so a grant on
an item in workspace B resolved for a request scoped to workspace A. This
is the underlying lookup behind the delete escalation PLAN-2382 fixed at
the handler; closing it here means the next caller does not have to
remember the workspace-identity guard.
The adjacent collection-grant lookup had the identical defect and the
identical safety argument, so it is scoped in the same commit rather than
leaving a second unscoped lookup three lines below the one DR-5 names.
Safe for every caller: all three (requireEditPermission, the collab
access check, crossWorkspaceEditAllowed) already pass the workspace the
item/collection was resolved in, and grant rows carry the workspace they
were minted in — the same scoping listUserItemGrants already uses.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
deriveThumbnails checked only that the parent ATTACHMENT row was live and
then copied parent.ItemID verbatim into every derived row. After TASK-2401's
read gate that is a waste with a cost: a variant of an archived item's
attachment is quota-counted storage that the blob path (DR-13) refuses to
serve, so the bytes are written, charged, and unreadable until the item is
restored. The same holds for a malformed item_id — the column has no FK and
no same-workspace constraint, so a row can name a foreign-workspace item or
no item at all.
Derivation now resolves the parent item at entry, before the blob is even
opened, and skips when it is soft-deleted, unresolvable, or in another
workspace. GetItem, not GetItemIncludeDeleted, so "live" means the same
thing here as on the read path. Orphan rows (item_id NULL) have no item to
check and still derive. This is internal background work with no HTTP
response, so there is no 404 shape to match: it skips and logs a WARN
alongside the existing decode/resize/persist skip logs, with the malformed
cases carrying distinct messages so they are greppable ahead of PLAN-2397's
repair.
The post-check window is DELIBERATELY ACCEPTED, and the comment on
thumbnailParentItemLive says so at length so the next reader does not file
it as a bug. The check is point-in-time — item deletion commits in its own
transaction and the read/decode/resize/encode/Put in between is unbounded
work — so an item archived mid-flight can still get a variant. Transform
(TASK-2402) closes its equivalent window with store.CreateAttachmentForLiveItem;
derivation deliberately does NOT, and makes the opposite trade: transform is
user-initiated and low-volume, whereas derivation is a background worker
fanning out from every image upload, so an item lock here is disproportionate
to the harm. What leaks through is a thumbnail — small, unreadable for as
long as its item stays archived, and tombstoned by the delete cascade with
its parent attachment.
Tests cover the sequential cases only: already-archived (with a sanity check
that DeleteItem really is a soft delete), unresolvable item_id, and a
foreign-workspace item_id that resolution alone would accept. The raced case
is deliberately not asserted — it is permitted behaviour, and pinning it
either way would constrain what the design leaves free. Two controls keep
the skips honest: a live parent and an orphan row must both still derive
from the same fixture and the same bytes, so a fixture that stopped
deriving at all would fail loudly rather than pass the skip assertions
vacuously. All three skip tests were mutation-verified against a
short-circuited guard, and the file passes -race -count=3 and make test-pg.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
handleTransformAttachment opened with a flat requireMinRole("editor") and
never looked at the attachment's parent item at all. A restricted editor —
one whose collection access excludes that item — could transform an
attachment on an item they cannot see, given only the attachment id: the
handler read the source blob and returned output metadata plus a new row.
The output URL inherits ItemID and is gated by TASK-2401's read gate, so
this was not direct byte exfiltration, but it crossed the same boundary and
leaked processing behaviour and metadata for an invisible item.
The handler now authorizes per-attachment, in the order the read path uses
(PLAN-2391 DR-10): load the row -> workspace identity -> load the parent
with GetItem -> parent workspace identity -> checkItemVisible -> edit
permission -> transform. Every denial goes through writeAttachmentNotFound,
so a missing attachment, a foreign parent, a soft-deleted parent and an
invisible item are byte-identical; a distinguishable code or message would
be an existence oracle. Malformed non-null parents that resolve nowhere are
rejected by the same guard.
Edit permission is requireEditPermission rather than the flat editor role:
an item- or collection-grant editor can already attach to the item
(BUG-1661), so refusing them a rotate on their own upload would be an
inconsistency, not a boundary. Orphan rows keep the flat editor gate and,
matching the DELETE path (PLAN-2382 DR-4), require unrestricted workspace
access — the storage listing hides orphans from restricted members, so the
transform must not confirm one exists.
DR-14's race is closed, not narrowed. The parent check is point-in-time:
item deletion commits in its own transaction, and the blob read, decode,
transform, encode and Put in between are unbounded work, so the item can be
archived mid-flight and the insert then writes a quota-counted live row
against an archived item whose bytes DR-13 refuses to serve. The new
store.CreateAttachmentForLiveItem re-checks the parent under a row lock
inside the insert's own transaction: the row is written against a live item
or not written at all. FOR NO KEY UPDATE, not FOR UPDATE — DeleteItem's
UPDATE touches no key column so the archival still blocks, while the many
tables with a REFERENCES items(id) foreign key (comments, stars, the Yjs
op-log) keep taking FOR KEY SHARE on the parent uncontended. SQLite skips
the clause: _txlock=immediate already serializes writers there.
Tests fail against the pre-fix code: the restricted-editor transform
returns 404 with a body byte-identical to the missing-attachment body, and
the mid-flight test archives the item from inside the processor's Encode —
between the up-front check and the insert — asserting the hook actually ran
so it cannot pass vacuously. The Postgres lock test polls pg_stat_activity
until the statement is registered as lock-blocked rather than sleeping, and
watches the completion channel so a missing lock fails immediately. Both
were mutation-verified.
Recorded, not fixed here: a refused insert leaves a rowless blob on disk,
and the orphan GC is row-driven so nothing reclaims it. Pre-existing on the
upload and thumbnail paths too; filed as BUG-2406 with the dedupe guard a
correct fix needs. The comment claiming GC reclaims a transform's original
was wrong and is corrected — only an orphan original is GC-eligible.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
handleGetAttachment opened with a flat requireMinRole("viewer").
roleLevel("guest") is 0, below viewer's 1, so every grant-based guest
was rejected before any item-level check ran and inline images broke in
items shared with them (BUG-2386).
The handler now authorizes per-attachment, in the order PLAN-2391 DR-10
fixes: load the row -> verify the parent item's workspace identity ->
check item visibility -> serve. Orphan rows keep the flat viewer+ gate;
the workspace-wide storage listing is untouched.
Also closes two defects sitting immediately around that gate:
DR-16 - GetAttachmentVariant scoped on parent_id/variant/deleted_at but
not workspace_id, so a foreign-workspace variant sharing a parent id
would be served after the local parent was authorized. Fixed at the
store API rather than in the handler because the other caller,
thumbnail derivation, has its own stake in the scope: an unscoped
"does this variant exist?" probe lets a foreign row suppress generation
of a legitimate local one.
DR-13 - the parent is loaded with GetItem, so a soft-deleted parent
404s. The DELETE path keeps GetItemIncludeDeleted, unchanged.
Denial paths now carry Cache-Control: private, no-store, set as the
handler's first statement (writeError calls WriteHeader immediately, so
anything later never reaches the wire); the positive private,
max-age=3600 is set only after authorization succeeds. Every
authorization-dependent refusal goes through one writer so the
responses are byte-identical and can't be used as an existence oracle.
The MCP image resource pad://workspace/{ws}/attachments/{id} inherits
the gate; asserted against a real server rather than assumed.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
The upload handler read item_id from two places with different rules:
authorization resolved only the query-string value, while the association
step fell back to the multipart-form value and persisted it verbatim. Since
ResolveItem accepts a UUID, a ref, or a slug, a form-supplied ref or a
foreign-workspace id could land in attachments.item_id unauthorized and
unresolvable — the malformed-row invariant BUG-2387's cross-workspace leak
rests on.
Three coupled changes (PLAN-2391 DR-2):
1. One effective item_id. Each non-empty channel is resolved in the request
workspace and the RESOLVED canonical ids are compared — not the caller's
spelling, so query "TASK-12" + form "<uuid>" is agreement, not conflict.
Absent and explicitly-empty both mean "no value" (compared after
TrimSpace). item.ID is what gets persisted. The form value is read from
r.MultipartForm.Value rather than r.FormValue, which merges the query
string back in and would collapse the two channels into one. A channel
that repeats item_id has every value resolved rather than first-wins,
since net/http otherwise silently discards the rest; the value count per
channel is capped, because exact-string dedup can't bound the lookups on
its own (TASK-7 / task-7 / TASK-0007 resolve alike).
2. Auth ordering. The no-item workspace-editor gate is deferred until after
multipart parsing; firing it pre-parse 403'd a form-only item-grant guest
(the CLI's shape) before the association that authorizes them was read.
The query channel is still resolved and authorized pre-parse so a doomed
upload never spools. The route's auth/workspace-access middleware chain
is unchanged.
3. Spool cleanup. file.Close() closes the spooled multipart temp file but
never removes it; added r.MultipartForm.RemoveAll() on every exit path,
including success, where it leaked today too.
Status codes (the pinned contract): an item_id that does not resolve in the
request workspace → 404 item_not_found on either channel, cross-workspace
UUIDs included; two channels — or two values on one channel — that each
resolve but to different items → 400 item_id_conflict.
Folded in from review: each resolved item is gated on requireItemVisible
(404) before the values are compared and before requireEditPermission (403).
Without that, the status split is an existence oracle for items a restricted
member or ungranted guest can't see — directly via 404-vs-403, or by pairing
a visible id with the id being probed and reading 400-vs-404. It also closes
requireEditPermission's editor/owner fast path, which never consults
collection visibility, so a collection_access="specific" member could
otherwise attach to an item in a collection hidden from them.
Two intentional behaviour narrowings, both following from DR-2's "reject a
non-empty value that does not resolve": an item_id for a soft-deleted item
now 404s where a workspace editor previously got a 201 (ResolveItem is
live-only) — consistent with DR-13/DR-14 keeping archived parents from
accruing new bytes; and an unresolvable item_id no longer falls back to the
flat editor gate and silently stores the caller's string.
Tests: extends TestUpload_GrantBasedEditorCanAttach with the form-only and
both-channel grant-guest cases, the ungranted-item 404, and the paired-probe
oracle check; adds canonical-UUID persistence, 404/400 rejection with no row
written, repeated conflicting values, the value-count cap, and a >1 MiB
isolated-TMPDIR fixture for the spool (a tiny in-memory body never spills to
disk, so it would pass either way). The auth-ordering and spool tests were
mutation-checked against the pre-fix behaviour.
Gates: make check (exit 0), make test-pg (exit 0).
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
WorkspaceAttachments joined `items` (and, through it, `collections`)
on item_id alone, so an attachment whose item_id points at another
workspace's item borrowed that item's title, slug, and collection
into the storage listing.
Both queries — the count and the result — now join with
`ON i.id = a.item_id AND i.workspace_id = a.workspace_id`. The
predicate is deliberately in ON, not WHERE: in WHERE the LEFT JOIN
degenerates into an inner join and the malformed row would vanish
from the listing entirely, hiding a row that still consumes quota
and that the PLAN-2397 repair has to be able to see. In ON the row
survives with NULL item/collection metadata.
Keeping the two queries in step matters — they are separate SQL and
a restricted caller's count must not diverge from their rows.
Review turned up a second hop of the same leak, folded in here:
items.collection_id has no composite workspace foreign key, so a
LOCAL item can reference a FOREIGN collection and surface its slug
even through a scoped item join. The collections join now carries
its own workspace predicate, same ON-clause rule.
Two fixtures pin both hops, each verified by mutation to fail when
its predicate is moved to WHERE or removed.
PLAN-2391 DR-3.
Two duplications the per-commit reviews couldn't see, caught by the
final full-diff pass.
announceAttachmentDeleted(wsSlug, id) replaces the notifyAttachmentDeleted
+ invalidateAttachmentMetadata pair that four call sites were repeating
(the strip's 204 and authoritative-404 paths, and StorageTab's two). Both
halves are needed every time, so a future delete surface calling only one
would silently stop propagating.
toUploadedAttachment() replaces the identical hand-written mapping of
AttachmentUploadResult to the bus DTO in Editor.svelte and
CommentEditor.svelte — the shape they had already been duplicating is
exactly how two upload paths drift.
No behavior change; gates and the e2e are unchanged and green.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
A file dropped or pasted into the editor now appears in the item
attachment strip immediately, instead of waiting for the next load of
the item (PLAN-2382 phase 3).
The task specified threading an onAttachmentUploaded callback down
through both <Editor> branches. Implemented on the attachment event bus
instead: TASK-2384 already introduced one for deletions, the strip
already subscribes to it, and reusing it avoids prop-drilling a second
channel through a component that has no other reason to know about the
strip. The deletion module is renamed $lib/attachments/events.ts to
cover both directions.
The upload closure captures the item id at upload START -- the promise
outlives an A->B switch even though <Editor> is keyed on item.id, and
AttachmentUploadResult carries no item_id, so that is the only point
where the association is known. Uploads without item context are not
announced: the server leaves item_id NULL for those, so an optimistic
tile would vanish on refresh.
The strip's internal row type is narrowed to what a tile renders. The
upload response has no storage_key / content_hash / created_at, and
fabricating them to satisfy AttachmentListItem would be worse than not
modelling columns nothing displays.
Also adds the browser-level coverage this plan was missing. The
component suite mounts the strip directly, so it passes even if the
ItemDetail mount is deleted or mis-wired; e2e/item-attachment-strip.spec.ts
pins in a real browser: the strip is mounted and shows only the current
item across an A->B switch, a dropped file appears with no refetch,
delete removes the tile and degrades the inline image to the missing
placeholder, the delete control genuinely takes keyboard focus (jsdom
applies no scoped CSS, so a regression to visibility:hidden is invisible
there), and a peeking master shows tiles with NO delete control. That
last one was mutation-verified: passing canEdit instead of
mutationsEnabled fails it.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
Adds the first in-item delete path for an attachment (PLAN-2382 phase 2).
Before this the only surface was Settings > Storage, which is
workspace-wide and disconnected from the item you're looking at.
Server: handleDeleteWorkspaceAttachment no longer opens with a flat
requireMinRole("editor"). That gate contradicted the UI's grant-aware
canEdit (permissions.ts::canEditItem), which is true for a viewer holding
an item- or collection-level edit grant -- so that user saw the affordance
and got a 403, even though upload already admits them (BUG-1661).
Authorization is now per-attachment, mirroring the upload handler:
- item-bound: requireItemVisible THEN requireEditPermission. The order
is load-bearing -- an attachment on an item the caller can't see must
keep returning 404, not the 403 that would confirm it exists.
- orphans: unchanged flat editor-role gate plus the guest filter, since
there's no item context to authorize against.
UI: per-tile delete control, in the DOM unconditionally so it's keyboard
reachable (CSS reveals it on hover/focus-within). Gated on ItemDetail's
mutationsEnabled, not raw canEdit, so a peeking master stays a complete
read-only freeze. Optimistic removal with rollback + toast on failure,
fenced so a switch mid-delete can't resurrect A's tile under B.
The confirm warns when the id is referenced in this item's body, and
deliberately hedges otherwise -- comment bodies, other items' content and
fields JSON are not visible client-side, so it says "may still be
referenced" rather than claiming non-use.
Editor: the attachment-image NodeView assigned img.src with no error
path, so a delete left the browser's broken-image glyph until reload --
reading as a network blip for what is a permanent state. It now degrades
to the same .attachment-missing placeholder the markdown renderer uses,
re-armed on uuid swap so rotate/crop clears a stale placeholder.
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
Surfaces an item's attachments as a compact, read-only icon row between
the Properties panel and the editor (PLAN-2382 phase 1).
- Extract categoryIcon / isImage / formatBytes out of StorageTab into
$lib/attachments/display so the strip shares one mime table.
- Add the item_id filter to AttachmentListFilters + api.attachments.list
(server already supports it; no Go change).
- New ItemAttachmentStrip.svelte: fetch bounded at 50, +N derived from
fetched rows not the response total, renders nothing when empty,
images open the existing Lightbox, other types download.
- Mounted OUTSIDE ItemDetail's {#key itemSlug}, so the fetch is fenced
on a load generation + item id (PLAN-2105 / TASK-2112 bug class).
Claude-Session: https://claude.ai/code/session_01LmbFxQFDjcYKBLcTnor6DC
Reopening the copy dialog after changing the destination workspace
wedged Svelte's effect scheduler. The dialog silently failed to appear
and every other control on the item pane died with it — the ⋯ menu
stopped opening, the split view could not be closed, the selected item
could not be changed. No console error, because a production build
reports none.
`$effect.pre` called `resetForOpen()` inside its tracked scope.
`resetForOpen` writes `destWs = sourceWsSlug` and then reads `destWs`
back to start the collection load, so the effect depended on a value it
had just written: the write invalidated the effect performing it, the
flush aborted, and the aborted flush stranded unrelated reactivity
across the pane. That is the CONVE-1688 hazard, and the comment
directly above the effect asserted the opposite — that `open` was its
only dependency.
It could not bite on the first open. `destWs` already equals
`sourceWsSlug` there, so the reset is a no-op write and nothing
invalidates. It needs a real destination change, a close, and a reopen.
Both branches now run inside `untrack`, so `open` really is the only
dependency.
Why the review missed it: all ten e2e cases opened the dialog exactly
once. Thirteen plan-review rounds, per-task Codex loops and four
full-diff rounds all reasoned about the effect from its comment, which
claimed the property that was untrue. Adds the reopen case, which
asserts the pane is still alive afterwards rather than only that the
dialog returned — mutation-verified: it fails with the untrack removed.
Final review round 2. PRE_WRITE_CODES whitelisted only the copy
handler's own business refusals, so csrf_error, email_not_verified and
a structured internal_error fell through to the outcome-unknown
fallback — telling the user their copy may have committed, sending
them to inspect the destination, and forbidding a retry that is in
fact safe. That is the inverse of the DR-13 hazard and just as wrong:
it sends someone hunting for an item that was never created.
All three are provably pre-write on this route:
- csrf_error and email_not_verified are rejected by the middleware
stack before handleCopyItem runs at all.
- internal_error is emitted here only by resolveAuthorizedCopy
(handlers_items_copy_resolve.go:128,184), both before the store
call. A post-commit panic deliberately does NOT emit it —
afterCopyCommit logs and lets the response stand — and chi's
Recoverer returns a bodiless 500, which carries no code and so
still lands in outcome-unknown, which is correct for it.
The ambiguous fallback is unchanged and still catches copy_failed, an
unstructured non-JSON response, a rejected fetch, a timeout, and any
code this list does not name.
The final full-diff review caught a stale-dispatch race. handleConfirm
captures the request up front, then awaits a collab flush and a final
preflight. Override controls stayed interactive across that window and
handleOverrideChange did not advance any generation the confirm was
fenced against, so an edit landing mid-flight left superseded() false
and dispatched the PRE-EDIT values — the user watching their new value
on screen while the old one was copied. On the move path that commits
wrong data with no retry available (DR-13).
Two parts, because either alone is incomplete:
- overrideGen, bumped on every override edit and checked by
superseded(). Deliberately NOT previewGen: that one cancels
in-flight preflights, which an override edit must not do — the
debounce and single-flight runner already collapse rapid edits.
- the needs-a-value controls are now read-only while preparing, not
only while submitting, so the edit cannot be started in the first
place.
Per final review.
Three accessors read a full collection row and each carried a verbatim
copy of the same 15-column projection and scan/hydration block:
GetCollection, GetCollectionAnyState, and the transactional
getCollectionInWorkspaceTx used by the cross-workspace copy. A column
added to the model had to be added in three places, and the copy path
drifted silently if only GetCollection was updated.
Extract collectionColumns plus scanCollectionRow, parameterized over
rowQueryer (the uniqueSlugQ / validateAssignmentScopeQ pattern from
TASK-2362) so the same read runs against *sql.DB or inside a caller's
*sql.Tx. Each accessor's full statement is assembled from constants, so
the WHERE predicate is the only per-caller difference, the SQL is built
at compile time rather than per call, and no runtime-assembled fragment
is ever handed to s.q.
Preserved deliberately: s.q placeholder rewriting (applied once, inside
the helper, so no call site can skip it); nil-on-sql.ErrNoRows at every
accessor -- the helper returns real errors unwrapped so each keeps its
own distinct prefix; the transactional lookup stays workspace-scoped and
active-only, which is the security boundary that makes a foreign
collection a not-found rather than a cross-workspace write.
lockCollectionRows is untouched: its SELECT id ... FOR UPDATE is a
locking primitive that duplicates nothing, and its sorted acquisition is
load-bearing.
ListCollections is deliberately left out and documented as such: it is
an aggregate multi-row query with aliased columns, a trailing COUNT and
no deleted_at, so sharing a projection would need a second count-aware
scanner and would reshape a hot query for no correctness gain.
TestCollectionAccessorsShareOneHydration pins all three to one
hydration. Every scanned column except deleted_at is asserted against a
literal, distinct value rather than against another accessor's output,
since cross-accessor equality alone cannot catch a mutation in the
shared projection; created_at and updated_at are set to different
instants so transposing them fails, and deleted_at is pinned by the
soft-delete branch, the only state in which it is non-nil. Verified by
mutation: a transposed slug/prefix projection, a transposed
created_at/updated_at projection, a dropped workspace scope on the
transactional read, a flattened deleted-state predicate, and a miss
turned into an error each fail the test.
Postgres' BOOLEAN admits exactly two values; SQLite's bare INTEGER admits
any. A stray 2 would scan as true through BoolToInt while the partial
index the moved-to lookup uses is WHERE archived_source = 1 — a row that
reads as a move but is invisible to the query that finds moves, which the
Postgres schema cannot represent. Add the CHECK, and make id NOT NULL
explicit since SQLite does not imply it for a TEXT PRIMARY KEY.
Migration 077 is unreleased, so amending it in place is safe.
The test is mutation-verified. Its first draft was NOT: it used
placeholder ids and passed against a schema with no CHECK at all, because
the foreign keys rejected the insert before the constraint under test was
reached. It now uses real fixture rows and asserts the same row inserts
cleanly with archived_source = 1.
Found by the final full-diff Codex pass over PLAN-2357, data-at-rest angle.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
xarmian