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
Two ways the cross-workspace copy preflight told a user "nothing to lose"
when there was, both violations of PLAN-2357 DR-17's "none of this may be
silent".
P1 — the five relationship counters are ACL-filtered by the caller's
collection visibility (correct, and TASK-2364 chose it deliberately), but
"none" and "none that you can see" rendered identically. A caller with
edit rights on the source and none on its relatives could read
`children_orphaned: false` and run a MOVE believing nothing was stranded,
while hidden children were orphaned in place.
The filtering stays; the uncertainty is now surfaced. Every point that
drops a relationship for visibility reasons sets a new
`warnings.relationships_partial` boolean. It is a BARE BOOLEAN by design:
how many are hidden, of what type and in which collection are exactly the
facts the filter exists to withhold, and a marker that varied with the
hidden count would reinstate the leak DR-10a, DR-10b and the moved-to
pointer each closed separately. A negative test asserts byte equality of
the whole warnings block across two workspaces that differ only in how
much is hidden. It is false for an unrestricted caller AND for a
restricted caller with nothing hidden, so the common case renders exactly
as it did before.
P2 — a child reachable only by a lone legacy `plan` edge was invisible to
GetChildItems (its join is restricted to store.ChildLinkTypes), so an
incoming `plan` relationship reported `child_count: 0` /
`children_orphaned: false` even though archiving the source strands it.
The link scan now folds such an edge into the child set, deduplicated
against the two mechanisms already covered and subject to the same
visibility, liveness and workspace guards. The outgoing direction (the
item's own parent) already reported correctly.
The mutating copy reports no relationship counters at all
(ItemCopyResultWarnings is deliberately narrower), so there is nothing for
assertPreflightMatchesCopy to disagree about.
CLI renders the qualifier on the five affected lines plus a plain-language
explanation; TS types carry the field for Phase 3's dialog.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
isDeadlockError matched SQLite's "database is locked" alongside
Postgres' 40P01, and the rollback path logged both as deadlock=true at
ERROR. But SQLite is single-writer with a 30-second busy timeout, so
"database is locked" is an expected saturation mode under burst load —
it says the box is busy. A 40P01 says DR-9's lock ordering, which is
meant to make deadlock impossible, is wrong. Reporting both identically
left an operator unable to tell a lock-ordering bug from ordinary load,
defeating the only signal this log exists to carry.
Split the predicates and add lock_timeout to the log line. Classification
test is mutation-verified: reintroducing the conflation fails it.
Found by the final full-diff Codex pass over PLAN-2357, operability angle.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
The store logged a unique-constraint violation as an unexpected rollback
incident while the HTTP layer mapped the same error to a caller-facing
409. A workspace-unique field colliding in the destination — a playbook's
invocation_slug, say — reaches this on ordinary input, so every routine
409 fired an operator warning and buried the deadlock signal the log
exists to surface.
Found by the final full-diff Codex pass over PLAN-2357 (P2: two
commits classified the same error two ways).
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
The helper's doc mandated that a mutating caller "re-apply the check"
inside its write transaction. Its only mutating consumer deliberately
does not, and is right not to: these functions read through s.store
rather than the caller's tx, so under READ COMMITTED the re-check would
judge locked resources against authorization state read at several
unsynchronised moments — reading as a write-time guarantee while
providing none.
State what a mutating caller actually owes (re-read the authorized
resource IDENTITY in-tx and refuse if it moved) and what it must not do,
so the contract and copyResourceInvariantPreCheck no longer disagree.
Found by the final full-diff Codex pass over PLAN-2357 (P1: a documented
write-time guard was in fact a TOCTOU check).
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
Wraps PLAN-2357's two endpoints behind one command:
pad item copy <ref> --to-workspace <slug> --collection <slug>
[--dry-run] [--archive-source] [--field key=value ...]
--dry-run renders the preflight's three contract buckets (carried /
dropped / needs_value) and DR-15's full warning set. Every bucket header
and every warning line prints unconditionally, zeros and empties
included: omitting a zero would make "no attachments" indistinguishable
from "this CLI does not report attachments", and DR-17's whole point is
that none of it is silent. Schema-supplied strings are escaped and list
members quoted, so a comma or newline in an option value cannot forge an
entry or a row.
--format json emits the endpoint's own response. json.Indent is a lexical
transform, so key order, unmodelled fields and int64 precision all
survive; the bytes are never round-tripped through a Go value.
DR-13, the no-retry obligation. There is no idempotency key, so a blind
re-run duplicates the item. Four mechanisms, each with a test:
1. the mutating copy runs on its own *http.Client AND its own
transport. The transport half is the one that matters: retry in Go
is almost always a RoundTripper wrapper, which a merely-dedicated
http.Client would inherit. A plain *http.Transport is cloned so
proxy/TLS config carries; a wrapper is not used at all;
2. its body is hidden behind an opaque reader, leaving Request.GetBody
nil so net/http's own nothing-written replay cannot fire;
3. redirects are refused rather than followed with the POST body;
4. failures are classified into three exclusive outcomes, because each
licenses a different thing to say. UNKNOWN (transport failure, 500
copy_failed) sends the user to check the destination and never
suggests a retry. COMMITTED-BUT-UNREPORTED (a 2xx whose body could
not be read or decoded) exits ZERO -- a non-zero exit would tell a
script the copy did not happen, which is the DR-13 duplicate
arrived at through the reporting layer. A 4xx is a refusal made
before any write and passes through plainly.
The same asymmetry governs stdout: a write failure on the dry run is an
error (nothing happened), while a write failure after the copy committed
goes to stderr and leaves the exit code at 0.
Refuse to guess. The preflight always runs first (it is read-only), and a
non-empty needs_value refuses before any mutating request, naming each
field and the exact --field flags to add. Mirrors the web dialog's
disabled confirm rather than round-tripping the user into an error they
could have been shown.
--field values are typed against the DESTINATION collection's schema, so
a number lands as a number. A malformed --field is a hard error here
rather than the silent skip `pad item create` does: this command's
contract is "you were told what to supply", and dropping a supplied value
would make the refusal a lie.
The response types in internal/cli mirror internal/server's. That is a
layering choice, not a cycle -- nothing in server imports cli, and the
mirror test imports server freely. It follows the posture already
recorded in internal/cli/bootstrap.go: this package is the HTTP client
and does not depend on the server package. An external cli_test package
walks both response shapes and fails on any JSON contract drift.
MCP is deliberately untouched: no pad_item.action: copy, and
ToolSurfaceVersion stays 0.15.
PLAN-2357 DR-9 / DR-9a / DR-11 / DR-12 / DR-14 / DR-16 / DR-17. One
store operation, one transaction: create in B, clone attachments,
archive A on a move, write provenance.
Lock order (the whole point of DR-9):
1. Both workspaces' advisory locks, sorted and deduplicated by the
hashtext LOCK KEY — sorting the ID strings does not order their
hashes, so two opposing movers could still deadlock.
2. Both collection rows FOR UPDATE, sorted by collection ID —
MigrateFields consumes both schemas.
3. Source item re-read under those locks; that snapshot is copied.
Both primitives are dialect-gated: FOR UPDATE is a syntax error on
SQLite, where BEGIN IMMEDIATE already serializes writers.
Pipeline: migrate -> overrides -> validate (DR-12: MigrateFields'
errors are stale once an override lands) -> quota -> PlanAttachmentCopy
INSIDE the tx -> rewrite content AND fields via the plan's IDMap ->
create in B -> attachment rows (originals before variants, item_id set
from the outset, uploaded_by = the actor) -> archive A -> provenance.
Seq (DR-14): B always advances; A advances only on ArchiveSource, and
a plain copy leaves A completely untouched. Quota (DR-16) runs inside
the transaction after the destination lock so two concurrent copies
cannot jointly exceed the cap.
Cross-backend attachment copies are REFUSED in v1
(ErrCopyCrossBackendAttachments): the store has no AttachmentStore
handle, and a byte transfer under both workspaces' locks would block
every writer in both workspaces on unbounded I/O with no rollback.
Supporting changes:
- CreateAttachmentTx: tx-taking insert (CreateAttachment is
self-committing), sharing one body with the pool form.
- CheckLimitTx: the feature COUNT reads through the caller's tx.
- createItemTxWithID: createItemTx with a caller-supplied id, so the
destination item id exists before the attachment plan is built.
Tests: creation parity, seq on both sides, DR-12 ordering, DR-8/DR-17
scrubs, attachment clone + rewrite (including refs in code fences),
DR-11a unresolvable refs, rollback at all four stages, quota. Postgres
only: opposing A->B / B->A copies do not deadlock, concurrent copies
cannot jointly exceed the cap, and colliding hashtext keys take one
lock. All three verified falsifiable by mutating the production code.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
Implements PLAN-2357 DR-11 / DR-11a. PlanAttachmentCopy takes the copied
content plus the FINAL destination fields and returns the old->new
attachment UUID map, the rows to create (originals followed by their
variants, parent_id remapped), the byte total, and the unresolvable-ref
list. It writes nothing, takes no *sql.Tx, and is shared by the copy
orchestration and the dry-run endpoint so their numbers cannot drift.
DR-11a: every resolution is scoped to workspace_id = A AND deleted_at IS
NULL, and the parent/variant traversal carries the identical scope. The
reference set comes from user-controlled content, so an unscoped lookup
would let a user clone another workspace's blob into a workspace they
control, bypassing the download handler's workspace check. Refs that
resolve to nothing under that scope -- dangling, soft-deleted, or foreign
-- are never cloned and never fatal: they get no map entry, so the
rewrite preserves the literal text and the copy renders exactly as broken
as the source did.
A cross-backend row emits an empty storage_key with the source key in
SourceStorageKey, so the plan never contains a key the target backend
cannot resolve. CreateAttachment now rejects an empty storage_key, which
turns that contract into an enforced invariant: an orchestration that
skips the Get/Put byte transfer fails at insert rather than creating a
live attachment that 404s on download.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
Implements PLAN-2357 DR-9a. CreateItem opens and commits its own
transaction, so the cross-workspace copy path (create in B + attachment
remap + provenance row + optional source archive, all atomic) cannot
call it. A raw in-tx `INSERT INTO items` in its place would silently
break version history, wiki-links, reporting, delta sync and slug
uniqueness -- none of which fail loudly.
Extracted, not duplicated, and CreateItem now goes through the same
function so the two paths cannot drift:
- `insertItemTx` is the write half, lifted verbatim out of the old
tryCreateItem body: the items INSERT (item_number, workspace seq,
content-flush watermarks), the initial item_versions row, wiki-link
indexing + broken-title resolution, and the create-time
status_transitions row.
- `createItemTx(tx, workspaceID, collectionID, input) (*models.Item, error)`
wraps it with the rest of CreateItem's pipeline -- defaults,
assignment-scope validation, workspace-scoped unique slug allocation --
inside a caller-owned transaction. It returns the item read back in-tx
so an orchestrator can consume its slug / item_number / seq (DR-14
fanout) without a post-COMMIT round-trip.
- `tryCreateItem` is now a BEGIN/COMMIT wrapper around createItemTx, and
CreateItem is the retry loop around that.
- `uniqueSlug` and `validateAssignmentScope` gained rowQueryer-
parameterized forms (`uniqueSlugQ` / `validateAssignmentScopeQ`) so
both can run on the caller's transaction. The *sql.DB entry points
delegate to them; behaviour is unchanged.
Content must already be final: wiki-link indexing and the first version
row are written from input.Content as given, so callers doing DR-11
attachment-ref rewriting must rewrite BEFORE calling.
Trust boundary is documented on the function. collectionID/workspaceID
consistency and ParentID scope stay the caller's job, matching the
pre-extraction tryCreateItem -- DR-9 has the orchestrator re-read and
row-lock both collections in-tx, so a check here would be a second,
weaker read of an already-pinned row. Assignee and agent role ARE
validated, as in CreateItem. No internal retry on unique violation: a
failed statement poisons the caller's transaction and an internal retry
would need a savepoint the caller can't see.
Fixes a latent slug race in CreateItem along the way. It used to
allocate the slug ONCE, outside the transaction, and re-submit that
stale value on every retry -- so two concurrent creates of the same
title had the loser burn all ten attempts on a slug the winner had
already committed and then fail with a unique-constraint error. Slug
allocation now happens inside the transaction under the workspace
advisory lock, so each attempt sees the previous scan's outcome. Two
Postgres-falsifiable concurrency tests cover it (createItemTx-only and
mixed CreateItem + createItemTx).
24 tests: one per DR-9a parity-checklist line, a rollback test asserting
no item / version / wiki-link / status transition / seq advance
survives, an in-tx-visibility test, a slug-collision test, and the two
concurrency tests. Every parity assertion verified falsifiable by
mutating the production code.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
An item MOVED to another workspace — copied, then archived — can now say
where it went. GET on a single item gains an optional `moved_to` block
naming each destination in displayable terms (workspace slug + item ref +
title + collection slug), so a consumer can render a link without a second
call. No HTTP redirect, no resolver change.
The ACL gate is the point. A destination is revealed only after the caller
independently passes AuthorizeCrossWorkspaceRead (TASK-2358) with an ITEM
scope on the destination item itself. Workspace-level access is not
sufficient: a restricted member of the destination workspace, or a guest
holding one unrelated item grant there, has a role in that workspace while
having no right to the copied item's collection.
A caller who fails that check sees NO hint a destination exists. The key is
omitted entirely — not a null, not an empty array, not a boolean — so the
response is byte-identical to an archived item with no move record at all.
A structurally distinguishable response is itself the leak.
Restore decision: the block is OMITTED for a non-archived source. Restoring
a moved-out source leaves two live items with the same content in two
workspaces, which is legitimate, but at that instant the source has not
moved anywhere and the response must stop asserting that it did. Past-tense
provenance is the back-pointer question and applies equally to plain copies,
which this field must never claim as moves.
Also honored: DR-2a (only archived_source rows feed the pointer; plain
copies are back-pointer material only), per-destination filtering over the
forward lookup's SET with no short-circuit on the first hit or first denial,
newest-first ordering, a scan bound on the per-GET authorization cost, and
deliberate isolation of the hand-rolled public share-link DTO — pinned by an
explicit negative test that freezes its key set.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
Phase 1 of PLAN-2357. Durable record of "this item was copied/moved from
workspace A to workspace B", backing the forward redirect (TASK-2359) and
the destination's back-pointer. Implements DR-2 / DR-2a.
Paired, dual-dialect, forward-only migrations (migrations/077 +
pgmigrations/055). archived_source distinguishes a move from a plain copy
(INTEGER on SQLite, BOOLEAN on Postgres, written through dialect.BoolToInt).
source_seq is a NULLABLE per-source move ordinal that exists solely so two
moves inside the same second are orderable — created_at is second-precision
RFC3339, so archive -> restore -> move again would otherwise resolve to an
arbitrary destination. Partial index (source_item_id, source_seq DESC) WHERE
archived_source, deliberately NOT unique: restore-then-move-again legitimately
repeats. The back direction IS uniquely indexed — a destination item is
created by exactly one copy, in the same transaction that writes its
provenance row, so a duplicate there would silently change which source the
back-pointer names.
Cascade is asymmetric on purpose, inverting item_collection_moves: the
archived source is precisely the row whose pointer must survive, so
source_item_id carries no FK at all; target_item_id cascades, because a
pointer at a vanished destination is worse than no pointer.
Store accessors: a tx-taking insert helper (no self-committing variant — the
row must land in the copy transaction), a forward lookup returning a SET
newest-first, and a back lookup. The insert rejects an archived row with no
seq and a copy row with one, so DR-2a's ordering invariant is enforced at the
write boundary rather than assumed. NULL ordering is normalized with COALESCE
because SQLite and Postgres disagree on DESC NULL placement.
Also wires workspace purge, which the two-workspace shape requires: both
workspace columns are RESTRICT references, so a purge clearing only one
direction would fail outright when the purged workspace sits on the other end.
Tests cover insert-in-tx, forward lookup with multiple destinations ordered
newest-first and scoped to one source, back lookup, rollback leaving no row,
and both DR-2a criteria. The ordering and scoping tests use fixed row IDs
whose lexical order contradicts the expected answer, so deleting the ordering
term or the WHERE clause under test fails them on every run rather than half
the time; verified by mutating the production query.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
grpc v1.79.3 is affected by GO-2026-6061 (xDS RBAC authorization engine
and HTTP/2 transport server). It reaches us indirectly via
internal/oauth -> ory/fosite -> ory/x/otelx -> otlptracehttp -> grpc.
The bump pulls otel v1.42.0 -> v1.43.0 as grpc's required floor, which
re-exposes GO-2026-5158 (uncapped baggage header parsing). Per the otel
lockstep policy, otel/metric/trace/sdk all move together to v1.44.0,
which clears it.
make vuln is now completely clean (0 call-reachable vulnerabilities).
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
* fix(web): collapse the mobile "Live" badge into the action bar (IDEA-2297)
At <=768px the collection page hid the h1 (the name lives in
MobileContextBar) but left the SSE status badge in .title-group, so that
row rendered a full line containing nothing but "* Live". The badge now
has a mobile mount at the trailing edge of .header-actions, collapsed to
the coloured dot alone, and .title-group drops out of layout entirely --
a 0-height flex item still collected .title-row's 12px column gap, which
was the last of the wasted row. Desktop is unchanged: the labelled badge
stays beside the title.
Compact mode CLIPS the label rather than removing it. The span is a
role="status" live region and live regions announce on text-content
change, so an aria-label-only element wouldn't reliably announce a drop
to "Offline".
Also gives the action bar one control height (IDEA-2297's second half).
The row mixed four: the quick-actions trigger ~22px, New ~24px (no
border), the view dropdown ~26px, icon buttons 28px. All are 28px now.
The trigger is normalised in the page rather than in QuickActionsMenu
because ItemDetail's .meta-actions band sizes the same trigger to its own
padding-based metrics; a height baked into the shared component would
fight it. Same override shape and specificity reasoning that band already
documents -- the child's scoped .trigger-btn.svelte-<hash> is (0,2,0), so
a bare :global(.trigger-btn) would tie and be settled by cross-file
source order.
Mobile gaps go 12px -> 4px. The six controls total ~255px, so the dot's
12px inset didn't fit on one line at 360px (a common Android width) and
wrapped, re-creating the row this change removes. Only the gaps shrink;
the controls stay 28px, so touch targets are untouched.
Verified in the browser against the installed binary: single row with a
12px inset at 430/390/375/360 (wraps at 340, as before); desktop badge
still in .title-group with its label visible and
aria-label="Live updates: Live" intact on both breakpoints; no
horizontal overflow. npm run check clean, 490 web unit tests pass.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
* fix(web): don't convey mobile SSE state by colour alone (IDEA-2297)
Codex review of #1038: with the label clipped in compact mode, hue was
the only thing separating Live from Offline -- colour-alone conveyance
(WCAG 1.4.1), and red/green is the exact pair dichromatic vision
collapses.
Healthy is now a FILLED dot and every unhealthy state is a hollow ring,
so the distinction that matters ("is the stream up?") is carried by
shape. Reconnecting stays separated from Offline by its pulse, and by
hue for anyone running reduced-motion. Scoped to compact mode -- the
labelled desktop variant already names the state in words.
Verified by forcing each status class onto the live badge and reading
computed styles at 390px: connected is a filled green 8px dot
(border-width 0), reconnecting/disconnected/unauthorized are transparent
with a 2px currentColor ring in their own hue, all 8px.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
* fix(web): separate reconnecting from offline without motion (IDEA-2297)
Codex re-review of #1038: the hollow ring told Live apart from the
unhealthy states, but Reconnecting leaned on its pulse to separate itself
from Offline -- and the pulse is switched off under
prefers-reduced-motion, leaving amber-vs-red as the only difference for
those users.
Reconnecting now takes a dashed ring. Three states, three shapes --
filled, dashed ring, solid ring -- independent of both hue and motion.
Verified at 390px by forcing each status class and reading computed
styles under both prefers-reduced-motion settings: connected filled
(border-width 0), reconnecting transparent + 2px dashed, disconnected
transparent + 2px solid; the pulse animation resolves to none under
reduce while the dashed ring persists.
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
The board lane `+` opens a Trello-style draft card (TASK-1676) whose Enter
handler hardcoded a full-page `goto(.../{item}?new=1)`. TASK-1676 predates the
split pane (PLAN-2105), so nothing revisited that destination, leaving the one
create gesture that already knows its title as the only card-open path that
bypasses the pane:
- Clicking an EXISTING card opens the split pane (`onItemOpen` → `?item=`);
creating one navigated the whole page away from the board.
- `?new=1` exists to drop you into the title editor of a fresh "Untitled" item
(`createNewItem`). On this path the title was just typed, so it re-opened the
title editor with that title select-alled.
- There was no viewport branching at all, so mobile — where the lane `+` is
fully present — got ejected off the board too, making a second add a Back
navigation.
`quickCreateInColumn`'s third param becomes `reveal` (caller INTENT) rather than
`navigate` (a destination): the local-index upsert is now unconditional so the
card always lands in its lane, and revealing means `openItemPane(item)` on
desktop and nothing on mobile. The page owns what revealing means, so BoardView
stays unaware the pane exists. The composer closes on submit on every viewport —
on desktop the pane takes focus, so keeping it open for rapid entry would fight
it. Revisit if feedback asks for mobile rapid-add.
The nav-guard's Save-all keeps passing `reveal: false`; saving drafts on the way
out must never open anything.
New e2e pins both destinations and, on both viewports, that the pathname never
changes and `?new=1` is never set. Two traps worth recording: the created card
renders off the SYNCHRONOUS local-index upsert, so it is not a sync point for
the navigation that follows — the first draft of both tests passed against the
reverted fix because the URL assertions raced an unresolved `goto`. Desktop now
waits on `?item=`; mobile can't poll an absence, so it proves the negative
positively by re-opening the lane composer (only possible if the board is still
mounted, and awaiting it gives a would-be navigation time to land). Verified by
mutating the fix back out: both fail for the right reasons, pass on the fix.
Gates: npm run check 0 errors, npm run test 490 passed, new spec 2/2, pane e2e
64/65 (the one failure is the pre-existing BUG-2334 SSE-toast flake, confirmed
by screenshot and passing in isolation). Codex review CLEAN (CONVE-735).
Claude-Session: https://claude.ai/code/session_01E2fRi12n8rARczvdEa2LYT
dependabot[bot]